Re: [PATCH] ARM: mach-moxart: platform port for MOXA ART SoC

[Arnd Bergmann <arnd@arndb.de>]

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@98800000 {
> +			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@98400000 {
> +			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@98700000 {
> +			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