| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <201305151516.52389.arnd@arndb.de>
Date: Wed, 15 May 2013 15:16:52 +0200
From: Arnd Bergmann <arnd@...db.de>
To: Jonas Jensen <jonas.jensen@...il.com>
Cc: linux-arm-kernel@...ts.infradead.org,
Daniel Mack <zonque@...il.com>, linux@....linux.org.uk,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ARM: mach-moxart: platform port for MOXA ART SoC
On Wednesday 15 May 2013, Jonas Jensen wrote:
> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
> index 29f7623..d534fce 100644
> --- a/arch/arm/Kconfig.debug
> +++ b/arch/arm/Kconfig.debug
> @@ -429,6 +429,15 @@ choice
> Say Y here if you want kernel low-level debugging support
> on Allwinner A1X based platforms on the UART1.
>
> + config DEBUG_MOXART_UART0
> + bool "Kernel low-level debugging messages via MOXART UART0"
> + depends on ARCH_MOXART
> + help
> + Say Y here if you want kernel low-level debugging support
> + on MOXART based platforms on the UART0.
> + select this to make sure "putc" in arch/arm/boot/compressed/debug.S
> + uses arch/arm/include/debug/moxart.S:s "addruart" macro
> +
> config DEBUG_TEGRA_UART
> depends on ARCH_TEGRA
> bool "Use Tegra UART for low-level debug"
> @@ -651,6 +660,7 @@ config DEBUG_LL_INCLUDE
> default "debug/sirf.S" if DEBUG_SIRFPRIMA2_UART1 || DEBUG_SIRFMARCO_UART1
> default "debug/socfpga.S" if DEBUG_SOCFPGA_UART
> default "debug/sunxi.S" if DEBUG_SUNXI_UART0 || DEBUG_SUNXI_UART1
> + default "debug/moxart.S" if DEBUG_MOXART_UART0
> default "debug/tegra.S" if DEBUG_TEGRA_UART
> default "debug/ux500.S" if DEBUG_UX500_UART
> default "debug/vexpress.S" if DEBUG_VEXPRESS_UART0_DETECT || \
Please split this patch into separate patches. All the debug stuff can go into
one patch that is fairly separate from the rest. You can probably find a few
other things that can be split out, just make sure that the order is so that
we don't have a broken source tree when only applying a few of them.
You can use git-format-patch and git-send-email to send out a series properly.
> +
> + intc: interrupt-controller@...00000 {
> + compatible = "moxart,moxart-interrupt-controller";
> + reg = <0x98800000 0x38>;
> + interrupt-controller;
> + #interrupt-cells = <2>;
> + interrupt-mask = <0x00080000>; /* single register vector,
> interrupts 0-31, 1s signify edge */
> + };
That will also take care of broken line wrapping as above. The current version can not
be applied.
> + timer0: timer@...00000 {
> + compatible = "moxart,moxart-timer0";
> + reg = <0x98400000 0x10>;
> + interrupts = <19 1>;
> + };
"moxart,moxart-timer0" is a strange identifier for a device. First of all, all your
compatible strings should probably start with "moxa," rather than "moxart,".
The part that I don't understand at all is the "timer0" part. Is that a string
from the data sheet?
> + gpio: gpio@...00000 {
> + compatible = "moxart,moxart-gpio";
> + reg = <0x98700000 0x1000>,
> + <0x98100000 0x1000>; /* PMU */
> + };
Can you provide some more detail why what PMU registers are used here? Is that
a "Performance Measurement Unit", "Power Management Unit" or something else?
Are you sure that those registers are only ever needed for GPIO?
> @@ -0,0 +1,34 @@
> +config ARCH_MOXART
> + bool "MOXA ART SoC" if (ARCH_MULTI_V4)
> + select ARCH_REQUIRE_GPIOLIB
> + select USE_OF
> + help
> + Say Y here if you want to run your kernel on hardware
> + with a MOXA ART SoC.
> + These are DIN-rail / wall-mountable embedded PCs sold by MOXA.
> + http://www.moxa.com/product/Embedded_Computers.htm
> +
> +config SOC_MOXART
> + bool "MOXART support"
> + depends on ARCH_MOXART
> + default y
> + select CPU_FA526
> + select ARM_DMA_MEM_BUFFERABLE
> + help
> + Support for the MOXA ART SoC. This is a Faraday FA526 ARMv4 32-bit
> 192 MHz processor with MMU and 16KB/8KB D/I-cache (UC-7112-LX)
> + This perticular SoC is used on models UC-7101, UC-7112/UC-7110,
> IA240/IA241, IA3341.
> + These are DIN-rail / wall-mountable embedded PCs sold by MOXA (
> http://www.moxa.com/product/Embedded_Computers.htm ).
> +
> +if ARCH_MOXART
> +
> +menu "MOXA ART SoC Implementation"
> +
> +config MACH_UC7112LX
> + bool "MOXA UC-7112-LX"
> + depends on ARCH_MOXART && SOC_MOXART
> + help
> + Say Y here if you intend to run this kernel on a MOXA UC-7112-LX
> embedded computer.
There should be no need for three separate symbols here. Just fold it
all into ARCH_MOXART. Note that you messed up the line wrapping above,
so that should be fixed. Hopefully the UC-7112-LX specific portions
can remain small to nonexisting so we don't have a need to make that
a separate option.
> +
> +ccflags-$(CONFIG_ARCH_MULTIPLATFORM) := -I$(srctree)/$(src)/include
Just leave this out and move all header files into the arch/arm/mach-moxart/
directory directly.
> diff --git a/arch/arm/mach-moxart/Makefile.boot
> b/arch/arm/mach-moxart/Makefile.boot
> new file mode 100644
> index 0000000..760a0ef
> --- /dev/null
> +++ b/arch/arm/mach-moxart/Makefile.boot
> @@ -0,0 +1,3 @@
> + zreladdr-y += 0x00008000
> +params_phys-y := 0x00000100
> +initrd_phys-y := 0x00800000
Is this still used?
> diff --git a/arch/arm/mach-moxart/idle.c b/arch/arm/mach-moxart/idle.c
> new file mode 100644
> index 0000000..5970c27
> --- /dev/null
> +++ b/arch/arm/mach-moxart/idle.c
> +static void moxart_idle(void)
> +{
> + /*
> + * Because of broken hardware we have to enable interrupts or the CPU
> + * will never wakeup... Acctualy it is not very good to enable
> + * interrupts first since scheduler can miss a tick, but there is
> + * no other way around this. Platforms that needs it for power saving
> + * should call enable_hlt() in init code, since by default it is
> + * disabled.
> + */
> +/* local_irq_enable();
> + cpu_do_idle();*/
> +}
> +
> +static int __init moxart_idle_init(void)
> +{
> + arm_pm_idle = moxart_idle;
> + return 0;
> +}
> +
> +arch_initcall(moxart_idle_init);
This does not seem to do anything at this point. Does the comment still
apply?
> +
> +static void __init moxart_init(void)
> +{
> + of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> +}
> +
> +void moxart_restart(char mode, const char *cmd)
> +{
> + writel(1, reg_wdt + 4);
> + writel(0x5ab9, reg_wdt + 8);
> + writel(0x03, reg_wdt + 12);
> +}
> +
> +static const char * const moxart_board_dt_compat[] = {
> + "moxart,moxart-uc-7112-lx",
> + NULL,
> +};
> +
> +DT_MACHINE_START(MOXART, "MOXA UC-7112-LX")
> + .init_irq = moxart_init_irq,
> + .init_time = moxart_timer_init,
> + .init_machine = moxart_init,
> + .handle_irq = moxart_handle_irq,
> + .restart = moxart_restart,
> + .dt_compat = moxart_board_dt_compat,
> + .nr_irqs = 32,
> +MACHINE_END
You can leave out moxart_init() now, it's the default implementation.
moxart_init_irq, moxart_handle_irq and nr_irqs should be obsolete if
you did the irqchip driver correctly, same for moxart_timer_init and
the clocksource driver.
I think the only part remaining here is moxart_restart, but that is
broken as long as reg_wdt does not get initialized. I think you could
move that function into the watchdog driver and assign it to
arm_pm_restart when you add that driver.
That would in fact make moxart the first platform that can run without
any platform specific code whatsoever.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists