RE: [PATCH v2] ARM: mmp: bring up pxa988 with device tree support

From: Neil Zhang
Date: Wed Jun 05 2013 - 22:52:44 EST


Hi Arnd,

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@xxxxxxxx]
> Sent: 2013年5月31日 19:25
> To: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Cc: Neil Zhang; haojian.zhuang@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2] ARM: mmp: bring up pxa988 with device tree support
>
> On Friday 31 May 2013 10:58:35 Neil Zhang wrote:
> > bring up pxa988 with device tree support.
> >
> > Change-Id: I6fc869b7d5ff8dc6e4eb0042a89429200f7a9fb1
>
> Please don't post silly extra headers like that.

Sorry for the noise, will remove it.

>
> > Signed-off-by: Neil Zhang <zhangwm@xxxxxxxxxxx>
>
> A couple of comments on the DT structure:
>
> > + gic: interrupt-controller@d1dfe100 {
> > + compatible = "arm,cortex-a9-gic";
> > + interrupt-controller;
> > + #interrupt-cells = <3>;
> > + reg = <0xd1dff000 0x1000>,
> > + <0xd1dfe100 0x0100>;
> > + };
> > +
> > + L2: l2-cache-controller@d1dfb000 {
> > + compatible = "arm,pl310-cache";
> > + reg = <0xd1dfb000 0x1000>;
> > + arm,data-latency = <2 1 1>;
> > + arm,tag-latency = <2 1 1>;
> > + arm,pwr-dynamic-clk-gating;
> > + arm,pwr-standby-mode;
> > + cache-unified;
> > + cache-level = <2>;
> > + };
> > +
> > + local-timer@d1dfe600 {
> > + compatible = "arm,cortex-a9-twd-timer";
> > + reg = <0xd1dfe600 0x20>;
> > + interrupts = <1 13 0x304>;
> > + };
>
> Why are these all top-level devices, rather than part of the 'soc' node?

Yes, we can move it as child of soc.

>
> > + soc {
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + compatible = "simple-bus";
> > + interrupt-parent = <&gic>;
> > + ranges;
> > +
> > + axi@d4200000 { /* AXI */
> > + compatible = "mrvl,axi-bus", "simple-bus";
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + reg = <0xd4200000 0x00200000>;
> > + ranges;
> > +
> > + intc: wakeupgen@d4282000 {
> > + compatible = "mrvl,mmp-intc";
> > + reg = <0xd4282000 0x1000>;
> > + mrvl,intc-wakeup = <0x114 0x3
> > + 0x144 0x3>;
> > + };
> > +
> > + };
>
> What is a 'mrvl,axi-bus'? Is that different from ARM's AXI bus?
>
> The documented vendor prefix for Marvell is 'marvell', not 'mrvl', please be
> consistent with that.
>
> What is the purpose of the 'reg' property in the axi bus? I notice that it
> overlaps with its own children, wich seens very strange.
> Maybe you meant this:
>
> axi {
> ranges = <0xd4200000 0xd4200000 0x00200000>;
> ...
> };
>
> > + apb@d4000000 { /* APB */
> > + compatible = "mrvl,apb-bus", "simple-bus";
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + reg = <0xd4000000 0x00200000>;
> > + ranges;
>
> Same comments apply here.
>
Thanks for the comments here, will modify it later.

> > diff --git a/arch/arm/mach-mmp/Kconfig b/arch/arm/mach-mmp/Kconfig
> > index ebdda83..0955191 100644
> > --- a/arch/arm/mach-mmp/Kconfig
> > +++ b/arch/arm/mach-mmp/Kconfig
> > @@ -107,6 +107,16 @@ config MACH_MMP2_DT
> > Include support for Marvell MMP2 based platforms using
> > the device tree.
> >
> > +config MACH_MMPX_DT
> > + bool "Support no-PJ/PJ4(ARMv7) platforms from device tree"
> > + depends on !CPU_MOHAWK && !CPU_PJ4
> > + select CPU_PXA988
> > + select USE_OF
> > + select PINCTRL
> > + select PINCTRL_SINGLE
>
> Why would this be mutually exclusive with PJ4? Cortex-A9 and PJ4 are both
> ARMv7 based, so we should be able to have them in the same kernel.

The MACH_MMPX_DT here is for standard ARM core based SoC.
But PJ4 is modified by Marvell, which includes IWMMXT.

>
> > + help
> > + Include support for Marvell MMP2 based platforms using
> > + the device tree.
> > endmenu
>
> You should probably change the help texts to say different things here, e.g.
> list the models supported under these.

Thanks for the remind, will modify it later.

>
> > diff --git a/arch/arm/mach-mmp/common.c
> b/arch/arm/mach-mmp/common.c
> > index 9292b79..0c621bc 100644
> > --- a/arch/arm/mach-mmp/common.c
> > +++ b/arch/arm/mach-mmp/common.c
> > @@ -11,6 +11,10 @@
> > #include <linux/init.h>
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/io.h>
> > +#include <linux/ioport.h>
> > +#include <linux/of_address.h>
> >
> > #include <asm/page.h>
> > #include <asm/mach/map.h>
> > @@ -36,7 +40,12 @@ static struct map_desc standard_io_desc[]
> __initdata = {
> > .virtual = (unsigned long)AXI_VIRT_BASE,
> > .length = AXI_PHYS_SIZE,
> > .type = MT_DEVICE,
> > - },
> > + }, {
> > + .pfn = __phys_to_pfn(MMP_CORE_PERIPH_PHYS_BASE),
> > + .virtual = (unsigned long)MMP_CORE_PERIPH_VIRT_BASE,
> > + .length = MMP_CORE_PERIPH_PHYS_SIZE,
> > + .type = MT_DEVICE,
> > + }
> > };
> >
> > void __init mmp_map_io(void)
>
> What is this needed for?

This function is used to static map the device registers.

>
> > +/* PXA988 */
> > +static const struct of_dev_auxdata pxa988_auxdata_lookup[] __initconst
> = {
> > + OF_DEV_AUXDATA("mrvl,mmp-uart", 0xd4017000, "pxa2xx-uart.0",
> NULL),
> > + OF_DEV_AUXDATA("mrvl,mmp-uart", 0xd4018000, "pxa2xx-uart.1",
> NULL),
> > + OF_DEV_AUXDATA("mrvl,mmp-uart", 0xd4036000, "pxa2xx-uart.2",
> NULL),
> > + OF_DEV_AUXDATA("mrvl,mmp-twsi", 0xd4011000, "pxa2xx-i2c.0",
> NULL),
> > + OF_DEV_AUXDATA("mrvl,mmp-twsi", 0xd4037000, "pxa2xx-i2c.1",
> NULL),
> > + OF_DEV_AUXDATA("mrvl,mmp-gpio", 0xd4019000, "pxa-gpio", NULL),
> > + OF_DEV_AUXDATA("mrvl,mmp-rtc", 0xd4010000, "sa1100-rtc", NULL),
> > + {}
> > +};
>
> Why do you need auxdata?

Two reasons:
1. some of the device still need platform data at this time.
2. we use device name as clk name.
Will change it later, but need some time.

>
> > +void __init pxa988_dt_init_timer(void) {
> > + extern void __init mmp_dt_init_timer(void);
>
> You should never put 'extern' declarations into a .c file.
>
> > + /*
> > + * Is is a SOC timer, and will be used for broadcasting
> > + * when local timers are disabled.
> > + */
> > + mmp_dt_init_timer();
> > +
> > + clocksource_of_init();
> > +}
>
> Please just change the mmp_dt_init_timer function to use
> CLOCKSOURCE_OF_DECLARE(), and if possible move the file to
> drivers/clocksource.

Yes, we will change to use CLOCKSOURCE_OF_DECLARE later.
But it need time, so let's keep it at this time.

>
> > +static const char *pxa988_dt_board_compat[] __initdata = {
> > + "mrvl,pxa988-dkb",
> > + NULL,
> > +};
> > +
> > +DT_MACHINE_START(PXA988_DT, "Marvell PXA988 (Device Tree
> Support)")
> > + .smp = smp_ops(mmp_smp_ops),
> > + .map_io = mmp_map_io,
> > + .init_irq = irqchip_init,
>
> You can leave out the init_irq, it's implicit.
>
> > + .init_time = pxa988_dt_init_timer,
> > + .init_machine = pxa988_dt_init_machine,
> > + .dt_compat = pxa988_dt_board_compat,
> > +MACHINE_END
>
>
> > +
> > +static int __init mmp_entry_vector_init(void) {
> > + int cpu;
> > +
> > + /* We will reset from DDR directly by default */
> > + writel(__pa(mmp_cpu_reset_entry), CIU_CA9_WARM_RESET_VECTOR);
> > +
> > + for (cpu = 1; cpu < CONFIG_NR_CPUS; cpu++)
> > + mmp_set_entry_vector(cpu, __pa(mmp_secondary_startup)); }
> > +
> > +early_initcall(mmp_entry_vector_init);
>
> You need to check which machine you are running on here. Best just move
> the call into one of the init functions of the machine descriptor, e.g.
> init_machine or init_late.

Thanks for the remind, will use init_early for it.

>
> CONFIG_NR_CPUS is probably the wrong constant to use here, it does not
> have to match the physically present CPU cores.

Thanks, will use nr_cpu_ids here.

>
> Arnd

Best Regards,
Neil Zhang
㈤旃??????+-遍荻?w??笔???dz罐??骅w*jg??????/??罐????璀??摺?囤??????:+v???佶>W?贽i?xPj??? -?+?d?