| Message ID | 20120302192844.GB21255@game.jcrosoft.org |
|---|---|
| State | New |
| Headers | show |
On Friday 02 March 2012, Jean-Christophe PLAGNIOL-VILLARD wrote: > + } > + > + if (of_device_is_compatible(np, "atmel,at91sam9x5-shdwc")) { > + have_rtt = false; > + have_rtc = true; > + } else if (of_device_is_compatible(np, "atmel,at91sam9rl-shdwc")) { > + have_rtt = true; > + have_rtc = true; > + } else { > + have_rtt = true; > + have_rtc = false; > + } > + > + if (have_rtc && of_property_read_bool(np, "atmel,wakeup-rtc-timer")) > + mode |= AT91_SHDW_RTCWKEN; > + > + if (have_rtt && of_property_read_bool(np, "atmel,wakeup-rtt-timer")) > + mode |= AT91_SHDW_RTTWKEN; > + > + at91_shdwc_write(AT91_SHDW_MR, wakeup_mode | mode); > + Hi Jean-Christophe, I don't understand why you check the specific part here. Isn't it enough to check the property when you already mandate that they can only be present on devices that support the specific wakeup? If there is a good explanation for that, maybe add a code comment why it's required. Arnd
On 20:24 Fri 02 Mar , Arnd Bergmann wrote: > On Friday 02 March 2012, Jean-Christophe PLAGNIOL-VILLARD wrote: > > + } > > + > > + if (of_device_is_compatible(np, "atmel,at91sam9x5-shdwc")) { > > + have_rtt = false; > > + have_rtc = true; > > + } else if (of_device_is_compatible(np, "atmel,at91sam9rl-shdwc")) { > > + have_rtt = true; > > + have_rtc = true; > > + } else { > > + have_rtt = true; > > + have_rtc = false; > > + } > > + > > + if (have_rtc && of_property_read_bool(np, "atmel,wakeup-rtc-timer")) > > + mode |= AT91_SHDW_RTCWKEN; > > + > > + if (have_rtt && of_property_read_bool(np, "atmel,wakeup-rtt-timer")) > > + mode |= AT91_SHDW_RTTWKEN; > > + > > + at91_shdwc_write(AT91_SHDW_MR, wakeup_mode | mode); > > + > > Hi Jean-Christophe, > > I don't understand why you check the specific part here. Isn't it enough to > check the property when you already mandate that they can only be present > on devices that support the specific wakeup? > > If there is a good explanation for that, maybe add a code comment why it's > required. some wake update source exist on few soc and we are not supposed to set the bit otherwise Ill put a comment. Best Regards, J.
Hi, Rob is ok for you? Best Regards, J. On 20:28 Fri 02 Mar , Jean-Christophe PLAGNIOL-VILLARD wrote: > HI, > > The following patch series add the bindings for: > - PMC > - SDRAM/DDR Controller > - Reset Controller > - Shutdown Controller > > The following changes since commit c5ec01650adb1976f928177cd7e71afcb82026c5: > > ARM: at91: dt: enable usb ehci for sam9g45 and sam9x5 (2012-03-02 00:46:37 +0800) > > are available in the git repository at: > git://github.com/at91linux/linux-at91.git ..BRANCH.NOT.VERIFIED.. > > Jean-Christophe PLAGNIOL-VILLARD (6): > ARM: at91/dt: add specific DT soc init > ARM: at91: add pmc DT support > ARM: at91: always enable sam9 restart > ARM: at91: add RSTC (Reset Controller) dt support > ARM: at91: add ram controller DT support > ARM: at91: add Shutdown Controller (SHDWC) DT support > > .../devicetree/bindings/arm/atmel-at91.txt | 60 ++++++++ > .../devicetree/bindings/arm/atmel-pmc.txt | 11 ++ > arch/arm/boot/dts/at91sam9g20.dtsi | 20 +++ > arch/arm/boot/dts/at91sam9g45.dtsi | 21 +++ > arch/arm/boot/dts/at91sam9m10g45ek.dts | 11 ++ > arch/arm/boot/dts/at91sam9x5.dtsi | 20 +++ > arch/arm/boot/dts/at91sam9x5cm.dtsi | 11 ++ > arch/arm/boot/dts/usb_a9g20.dts | 11 ++ > arch/arm/mach-at91/Kconfig | 10 +- > arch/arm/mach-at91/at91sam9x5.c | 7 - > arch/arm/mach-at91/board-dt.c | 8 +- > arch/arm/mach-at91/clock.c | 54 ++++++- > arch/arm/mach-at91/generic.h | 2 + > arch/arm/mach-at91/include/mach/at91_shdwc.h | 4 +- > arch/arm/mach-at91/include/mach/at91sam9x5.h | 5 - > arch/arm/mach-at91/setup.c | 158 ++++++++++++++++++++ > 16 files changed, 380 insertions(+), 33 deletions(-) > create mode 100644 Documentation/devicetree/bindings/arm/atmel-pmc.txt > > Best Regards, > J. > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wednesday 07 March 2012, Jean-Christophe PLAGNIOL-VILLARD wrote: > On 20:24 Fri 02 Mar , Arnd Bergmann wrote: > > On Friday 02 March 2012, Jean-Christophe PLAGNIOL-VILLARD wrote: > > > + } > > > + > > > + if (of_device_is_compatible(np, "atmel,at91sam9x5-shdwc")) { > > > + have_rtt = false; > > > + have_rtc = true; > > > + } else if (of_device_is_compatible(np, "atmel,at91sam9rl-shdwc")) { > > > + have_rtt = true; > > > + have_rtc = true; > > > + } else { > > > + have_rtt = true; > > > + have_rtc = false; > > > + } > > > + > > > + if (have_rtc && of_property_read_bool(np, "atmel,wakeup-rtc-timer")) > > > + mode |= AT91_SHDW_RTCWKEN; > > > + > > > + if (have_rtt && of_property_read_bool(np, "atmel,wakeup-rtt-timer")) > > > + mode |= AT91_SHDW_RTTWKEN; > > > + > > > + at91_shdwc_write(AT91_SHDW_MR, wakeup_mode | mode); > > > + > > > > Hi Jean-Christophe, > > > > I don't understand why you check the specific part here. Isn't it enough to > > check the property when you already mandate that they can only be present > > on devices that support the specific wakeup? > > > > If there is a good explanation for that, maybe add a code comment why it's > > required. > some wake update source exist on few soc and we are not supposed to set the > bit otherwise > I still don't understand: Doesn't the property already give the information? In general, you should try to encode these things in specific properties instead of checking the compatible property. Arnd
On 18:49 Wed 07 Mar , Arnd Bergmann wrote: > On Wednesday 07 March 2012, Jean-Christophe PLAGNIOL-VILLARD wrote: > > On 20:24 Fri 02 Mar , Arnd Bergmann wrote: > > > On Friday 02 March 2012, Jean-Christophe PLAGNIOL-VILLARD wrote: > > > > + } > > > > + > > > > + if (of_device_is_compatible(np, "atmel,at91sam9x5-shdwc")) { > > > > + have_rtt = false; > > > > + have_rtc = true; > > > > + } else if (of_device_is_compatible(np, "atmel,at91sam9rl-shdwc")) { > > > > + have_rtt = true; > > > > + have_rtc = true; > > > > + } else { > > > > + have_rtt = true; > > > > + have_rtc = false; > > > > + } > > > > + > > > > + if (have_rtc && of_property_read_bool(np, "atmel,wakeup-rtc-timer")) > > > > + mode |= AT91_SHDW_RTCWKEN; > > > > + > > > > + if (have_rtt && of_property_read_bool(np, "atmel,wakeup-rtt-timer")) > > > > + mode |= AT91_SHDW_RTTWKEN; > > > > + > > > > + at91_shdwc_write(AT91_SHDW_MR, wakeup_mode | mode); > > > > + > > > > > > Hi Jean-Christophe, > > > > > > I don't understand why you check the specific part here. Isn't it enough to > > > check the property when you already mandate that they can only be present > > > on devices that support the specific wakeup? > > > > > > If there is a good explanation for that, maybe add a code comment why it's > > > required. > > some wake update source exist on few soc and we are not supposed to set the > > bit otherwise > > > > I still don't understand: Doesn't the property already give the information? Yes > In general, you should try to encode these things in specific properties instead of > checking the compatible property. But I check that no mistake is done in the DT as the source of wakeup is availlable on different version of the IP Just more cautious Best Regards, J.
On 03/07/2012 12:49 PM, Arnd Bergmann wrote: > On Wednesday 07 March 2012, Jean-Christophe PLAGNIOL-VILLARD wrote: >> On 20:24 Fri 02 Mar , Arnd Bergmann wrote: >>> On Friday 02 March 2012, Jean-Christophe PLAGNIOL-VILLARD wrote: >>>> + } >>>> + >>>> + if (of_device_is_compatible(np, "atmel,at91sam9x5-shdwc")) { >>>> + have_rtt = false; >>>> + have_rtc = true; >>>> + } else if (of_device_is_compatible(np, "atmel,at91sam9rl-shdwc")) { >>>> + have_rtt = true; >>>> + have_rtc = true; >>>> + } else { >>>> + have_rtt = true; >>>> + have_rtc = false; >>>> + } >>>> + >>>> + if (have_rtc && of_property_read_bool(np, "atmel,wakeup-rtc-timer")) >>>> + mode |= AT91_SHDW_RTCWKEN; >>>> + >>>> + if (have_rtt && of_property_read_bool(np, "atmel,wakeup-rtt-timer")) >>>> + mode |= AT91_SHDW_RTTWKEN; >>>> + >>>> + at91_shdwc_write(AT91_SHDW_MR, wakeup_mode | mode); >>>> + >>> >>> Hi Jean-Christophe, >>> >>> I don't understand why you check the specific part here. Isn't it enough to >>> check the property when you already mandate that they can only be present >>> on devices that support the specific wakeup? >>> >>> If there is a good explanation for that, maybe add a code comment why it's >>> required. >> some wake update source exist on few soc and we are not supposed to set the >> bit otherwise >> > > I still don't understand: Doesn't the property already give the information? > In general, you should try to encode these things in specific properties instead of > checking the compatible property. > Or vice-versa, the compatible properties distinguish things enough that the property is not needed. If it is fixed in the SOC design, then you should distinguish things with the compatible property. Rob > Arnd > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 03/02/2012 01:54 PM, Jean-Christophe PLAGNIOL-VILLARD wrote: > We can now drop the call to ioremap_registers() as we have the binding for the > SDRAM/DDR Controller. > > Drop ioremap_registers() for sam9x5 too. > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> > --- > .../devicetree/bindings/arm/atmel-at91.txt | 19 ++++++++++++++ > arch/arm/boot/dts/at91sam9g20.dtsi | 5 +++ > arch/arm/boot/dts/at91sam9g45.dtsi | 6 ++++ > arch/arm/boot/dts/at91sam9x5.dtsi | 5 +++ > arch/arm/mach-at91/at91sam9x5.c | 6 ---- > arch/arm/mach-at91/include/mach/at91sam9x5.h | 5 --- > arch/arm/mach-at91/setup.c | 27 +++++++++++++++++-- > 7 files changed, 59 insertions(+), 14 deletions(-) > > diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.txt b/Documentation/devicetree/bindings/arm/atmel-at91.txt > index a64f867..1f87820 100644 > --- a/Documentation/devicetree/bindings/arm/atmel-at91.txt > +++ b/Documentation/devicetree/bindings/arm/atmel-at91.txt > @@ -42,3 +42,22 @@ Example: > compatible = "atmel,at91sam9260-rstc"; > reg = <0xfffffd00 0x10>; > }; > + > +RAMC SDRAM/DDR Controller required properties: > +- compatible: Should be "atmel,at91sam9260-sdramc", > + "atmel,at91sam9g45-ddramc", > +- reg: Should contain registers location and length > + For at91sam9263 and at91sam9g45 you must specify 2 entries. > + > +Examples: > + > + ramc0: ramc@ffffe800 { > + compatible = "atmel,at91sam9g45-ddramc"; > + reg = <0xffffe800 0x200>; > + }; > + > + ramc0: ramc@ffffe400 { > + compatible = "atmel,at91sam9g45-ddramc"; > + reg = <0xffffe400 0x200 > + 0xffffe600 0x200>; > + }; > diff --git a/arch/arm/boot/dts/at91sam9g20.dtsi b/arch/arm/boot/dts/at91sam9g20.dtsi > index 5414347..573ac5a 100644 > --- a/arch/arm/boot/dts/at91sam9g20.dtsi > +++ b/arch/arm/boot/dts/at91sam9g20.dtsi > @@ -59,6 +59,11 @@ > reg = <0xfffff000 0x200>; > }; > > + ramc0: ramc@ffffea00 { > + compatible = "atmel,at91sam9260-sdramc"; > + reg = <0xffffea00 0x200>; > + }; > + > pmc: pmc@fffffc00 { > compatible = "atmel,at91rm9200-pmc"; > reg = <0xfffffc00 0x100>; > diff --git a/arch/arm/boot/dts/at91sam9g45.dtsi b/arch/arm/boot/dts/at91sam9g45.dtsi > index e2ccba5..6da07a9 100644 > --- a/arch/arm/boot/dts/at91sam9g45.dtsi > +++ b/arch/arm/boot/dts/at91sam9g45.dtsi > @@ -60,6 +60,12 @@ > reg = <0xfffff000 0x200>; > }; > > + ramc0: ramc@ffffe400 { > + compatible = "atmel,at91sam9g45-ddramc"; > + reg = <0xffffe400 0x200 > + 0xffffe600 0x200>; > + }; > + > pmc: pmc@fffffc00 { > compatible = "atmel,at91rm9200-pmc"; > reg = <0xfffffc00 0x100>; > diff --git a/arch/arm/boot/dts/at91sam9x5.dtsi b/arch/arm/boot/dts/at91sam9x5.dtsi > index 54e3030..09bc806 100644 > --- a/arch/arm/boot/dts/at91sam9x5.dtsi > +++ b/arch/arm/boot/dts/at91sam9x5.dtsi > @@ -58,6 +58,11 @@ > reg = <0xfffff000 0x200>; > }; > > + ramc0: ramc@ffffe800 { > + compatible = "atmel,at91sam9g45-ddramc"; > + reg = <0xffffe800 0x200>; > + }; > + > pmc: pmc@fffffc00 { > compatible = "atmel,at91rm9200-pmc"; > reg = <0xfffffc00 0x100>; > diff --git a/arch/arm/mach-at91/at91sam9x5.c b/arch/arm/mach-at91/at91sam9x5.c > index 0b82c34..b6831ee 100644 > --- a/arch/arm/mach-at91/at91sam9x5.c > +++ b/arch/arm/mach-at91/at91sam9x5.c > @@ -302,11 +302,6 @@ static void __init at91sam9x5_map_io(void) > at91_init_sram(0, AT91SAM9X5_SRAM_BASE, AT91SAM9X5_SRAM_SIZE); > } > > -static void __init at91sam9x5_ioremap_registers(void) > -{ > - at91_ioremap_ramc(0, AT91SAM9X5_BASE_DDRSDRC0, 512); > -} > - > void __init at91sam9x5_initialize(void) > { > at91_extern_irq = (1 << AT91SAM9X5_ID_IRQ0); > @@ -359,7 +354,6 @@ static unsigned int at91sam9x5_default_irq_priority[NR_AIC_IRQS] __initdata = { > struct at91_init_soc __initdata at91sam9x5_soc = { > .map_io = at91sam9x5_map_io, > .default_irq_priority = at91sam9x5_default_irq_priority, > - .ioremap_registers = at91sam9x5_ioremap_registers, > .register_clocks = at91sam9x5_register_clocks, > .init = at91sam9x5_initialize, > }; > diff --git a/arch/arm/mach-at91/include/mach/at91sam9x5.h b/arch/arm/mach-at91/include/mach/at91sam9x5.h > index a297a77..88e43d5 100644 > --- a/arch/arm/mach-at91/include/mach/at91sam9x5.h > +++ b/arch/arm/mach-at91/include/mach/at91sam9x5.h > @@ -55,11 +55,6 @@ > #define AT91SAM9X5_BASE_USART2 0xf8024000 > > /* > - * System Peripherals > - */ > -#define AT91SAM9X5_BASE_DDRSDRC0 0xffffe800 > - > -/* > * Base addresses for early serial code (uncompress.h) > */ > #define AT91_DBGU AT91_BASE_DBGU0 > diff --git a/arch/arm/mach-at91/setup.c b/arch/arm/mach-at91/setup.c > index 3e48b59..f86450d 100644 > --- a/arch/arm/mach-at91/setup.c > +++ b/arch/arm/mach-at91/setup.c > @@ -315,12 +315,33 @@ static void at91_dt_rstc(void) > of_node_put(np); > } > > +static struct of_device_id ramc_ids[] = { > + { .compatible = "atmel,at91sam9260-sdramc" }, > + { .compatible = "atmel,at91sam9g45-ddramc" }, > + { /*sentinel*/ } > +}; > + > +static void at91_dt_ramc(void) > +{ > + struct device_node *np; > + > + np = of_find_matching_node(NULL, ramc_ids); > + if (!np) > + panic("unable to find compatible ram conroller node in dtb\n"); You really can't boot if this fails? A WARN is better if it allows you to boot until at least your console is actually up. > + > + at91_ramc_base[0] = of_iomap(np, 0); > + if (!at91_ramc_base[0]) > + panic("unable to map ramc[0] cpu registers\n"); > + /* the controller may have 2 banks */ > + at91_ramc_base[1] = of_iomap(np, 1); > + > + of_node_put(np); > +} > + > void __init at91_dt_initialize(void) > { > at91_dt_rstc(); > - > - /* temporary until have the ramc binding*/ > - at91_boot_soc.ioremap_registers(); > + at91_dt_ramc(); > > /* Init clock subsystem */ > at91_dt_clock_init();
On Wednesday 07 March 2012, Rob Herring wrote: > > > > I still don't understand: Doesn't the property already give the information? > > In general, you should try to encode these things in specific properties instead of > > checking the compatible property. > > > > Or vice-versa, the compatible properties distinguish things enough that > the property is not needed. If it is fixed in the SOC design, then you > should distinguish things with the compatible property. Well, one of the two, basically. I would suggest using special properties in order to be prepared when other SOCs have the same requirement. If you know for certain that each one will only ever be needed in one specific SOC, then the compatible property is enough, but if it's likely that others will have the same requirement in the future, I think it's much better to have just a single check rather than a list of SOCs. Arnd
> > diff --git a/arch/arm/mach-at91/include/mach/at91sam9x5.h b/arch/arm/mach-at91/include/mach/at91sam9x5.h > > index a297a77..88e43d5 100644 > > --- a/arch/arm/mach-at91/include/mach/at91sam9x5.h > > +++ b/arch/arm/mach-at91/include/mach/at91sam9x5.h > > @@ -55,11 +55,6 @@ > > #define AT91SAM9X5_BASE_USART2 0xf8024000 > > > > /* > > - * System Peripherals > > - */ > > -#define AT91SAM9X5_BASE_DDRSDRC0 0xffffe800 > > - > > -/* > > * Base addresses for early serial code (uncompress.h) > > */ > > #define AT91_DBGU AT91_BASE_DBGU0 > > diff --git a/arch/arm/mach-at91/setup.c b/arch/arm/mach-at91/setup.c > > index 3e48b59..f86450d 100644 > > --- a/arch/arm/mach-at91/setup.c > > +++ b/arch/arm/mach-at91/setup.c > > @@ -315,12 +315,33 @@ static void at91_dt_rstc(void) > > of_node_put(np); > > } > > > > +static struct of_device_id ramc_ids[] = { > > + { .compatible = "atmel,at91sam9260-sdramc" }, > > + { .compatible = "atmel,at91sam9g45-ddramc" }, > > + { /*sentinel*/ } > > +}; > > + > > +static void at91_dt_ramc(void) > > +{ > > + struct device_node *np; > > + > > + np = of_find_matching_node(NULL, ramc_ids); > > + if (!np) > > + panic("unable to find compatible ram conroller node in dtb\n"); > > You really can't boot if this fails? A WARN is better if it allows you > to boot until at least your console is actually up. if the restart is called you will have a oops so no it's a basic mandatory device on at91 Best Regards, J.
On 08:12 Thu 08 Mar , Rob Herring wrote: > On 03/08/2012 12:13 AM, Jean-Christophe PLAGNIOL-VILLARD wrote: > >>> diff --git a/arch/arm/mach-at91/include/mach/at91sam9x5.h b/arch/arm/mach-at91/include/mach/at91sam9x5.h > >>> index a297a77..88e43d5 100644 > >>> --- a/arch/arm/mach-at91/include/mach/at91sam9x5.h > >>> +++ b/arch/arm/mach-at91/include/mach/at91sam9x5.h > >>> @@ -55,11 +55,6 @@ > >>> #define AT91SAM9X5_BASE_USART2 0xf8024000 > >>> > >>> /* > >>> - * System Peripherals > >>> - */ > >>> -#define AT91SAM9X5_BASE_DDRSDRC0 0xffffe800 > >>> - > >>> -/* > >>> * Base addresses for early serial code (uncompress.h) > >>> */ > >>> #define AT91_DBGU AT91_BASE_DBGU0 > >>> diff --git a/arch/arm/mach-at91/setup.c b/arch/arm/mach-at91/setup.c > >>> index 3e48b59..f86450d 100644 > >>> --- a/arch/arm/mach-at91/setup.c > >>> +++ b/arch/arm/mach-at91/setup.c > >>> @@ -315,12 +315,33 @@ static void at91_dt_rstc(void) > >>> of_node_put(np); > >>> } > >>> > >>> +static struct of_device_id ramc_ids[] = { > >>> + { .compatible = "atmel,at91sam9260-sdramc" }, > >>> + { .compatible = "atmel,at91sam9g45-ddramc" }, > >>> + { /*sentinel*/ } > >>> +}; > >>> + > >>> +static void at91_dt_ramc(void) > >>> +{ > >>> + struct device_node *np; > >>> + > >>> + np = of_find_matching_node(NULL, ramc_ids); > >>> + if (!np) > >>> + panic("unable to find compatible ram conroller node in dtb\n"); > >> > >> You really can't boot if this fails? A WARN is better if it allows you > >> to boot until at least your console is actually up. > > if the restart is called you will have a oops so no it's a basic mandatory > > device on at91 > > > > But you may never see the panic message because your console is not up. > If you WARN and can continue to boot, then the user can see the problem > and fix it. Otherwise you get nothing and have to go rebuild and turn on > earlyprintk. yeah agreed but if the restart id use before the console is enable you will have the same issue. the ramc controller are basic device so people usally don't touch it except you add a SoC support. so I prefer to panic Best Regards, J.
On 03/08/2012 12:13 AM, Jean-Christophe PLAGNIOL-VILLARD wrote: >>> diff --git a/arch/arm/mach-at91/include/mach/at91sam9x5.h b/arch/arm/mach-at91/include/mach/at91sam9x5.h >>> index a297a77..88e43d5 100644 >>> --- a/arch/arm/mach-at91/include/mach/at91sam9x5.h >>> +++ b/arch/arm/mach-at91/include/mach/at91sam9x5.h >>> @@ -55,11 +55,6 @@ >>> #define AT91SAM9X5_BASE_USART2 0xf8024000 >>> >>> /* >>> - * System Peripherals >>> - */ >>> -#define AT91SAM9X5_BASE_DDRSDRC0 0xffffe800 >>> - >>> -/* >>> * Base addresses for early serial code (uncompress.h) >>> */ >>> #define AT91_DBGU AT91_BASE_DBGU0 >>> diff --git a/arch/arm/mach-at91/setup.c b/arch/arm/mach-at91/setup.c >>> index 3e48b59..f86450d 100644 >>> --- a/arch/arm/mach-at91/setup.c >>> +++ b/arch/arm/mach-at91/setup.c >>> @@ -315,12 +315,33 @@ static void at91_dt_rstc(void) >>> of_node_put(np); >>> } >>> >>> +static struct of_device_id ramc_ids[] = { >>> + { .compatible = "atmel,at91sam9260-sdramc" }, >>> + { .compatible = "atmel,at91sam9g45-ddramc" }, >>> + { /*sentinel*/ } >>> +}; >>> + >>> +static void at91_dt_ramc(void) >>> +{ >>> + struct device_node *np; >>> + >>> + np = of_find_matching_node(NULL, ramc_ids); >>> + if (!np) >>> + panic("unable to find compatible ram conroller node in dtb\n"); >> >> You really can't boot if this fails? A WARN is better if it allows you >> to boot until at least your console is actually up. > if the restart is called you will have a oops so no it's a basic mandatory > device on at91 > But you may never see the panic message because your console is not up. If you WARN and can continue to boot, then the user can see the problem and fix it. Otherwise you get nothing and have to go rebuild and turn on earlyprintk. Rob
On 03/08/2012 08:10 AM, Jean-Christophe PLAGNIOL-VILLARD wrote: > On 08:12 Thu 08 Mar , Rob Herring wrote: >> On 03/08/2012 12:13 AM, Jean-Christophe PLAGNIOL-VILLARD wrote: >>>>> diff --git a/arch/arm/mach-at91/include/mach/at91sam9x5.h b/arch/arm/mach-at91/include/mach/at91sam9x5.h >>>>> index a297a77..88e43d5 100644 >>>>> --- a/arch/arm/mach-at91/include/mach/at91sam9x5.h >>>>> +++ b/arch/arm/mach-at91/include/mach/at91sam9x5.h >>>>> @@ -55,11 +55,6 @@ >>>>> #define AT91SAM9X5_BASE_USART2 0xf8024000 >>>>> >>>>> /* >>>>> - * System Peripherals >>>>> - */ >>>>> -#define AT91SAM9X5_BASE_DDRSDRC0 0xffffe800 >>>>> - >>>>> -/* >>>>> * Base addresses for early serial code (uncompress.h) >>>>> */ >>>>> #define AT91_DBGU AT91_BASE_DBGU0 >>>>> diff --git a/arch/arm/mach-at91/setup.c b/arch/arm/mach-at91/setup.c >>>>> index 3e48b59..f86450d 100644 >>>>> --- a/arch/arm/mach-at91/setup.c >>>>> +++ b/arch/arm/mach-at91/setup.c >>>>> @@ -315,12 +315,33 @@ static void at91_dt_rstc(void) >>>>> of_node_put(np); >>>>> } >>>>> >>>>> +static struct of_device_id ramc_ids[] = { >>>>> + { .compatible = "atmel,at91sam9260-sdramc" }, >>>>> + { .compatible = "atmel,at91sam9g45-ddramc" }, >>>>> + { /*sentinel*/ } >>>>> +}; >>>>> + >>>>> +static void at91_dt_ramc(void) >>>>> +{ >>>>> + struct device_node *np; >>>>> + >>>>> + np = of_find_matching_node(NULL, ramc_ids); >>>>> + if (!np) >>>>> + panic("unable to find compatible ram conroller node in dtb\n"); >>>> >>>> You really can't boot if this fails? A WARN is better if it allows you >>>> to boot until at least your console is actually up. >>> if the restart is called you will have a oops so no it's a basic mandatory >>> device on at91 >>> >> >> But you may never see the panic message because your console is not up. >> If you WARN and can continue to boot, then the user can see the problem >> and fix it. Otherwise you get nothing and have to go rebuild and turn on >> earlyprintk. > yeah agreed but if the restart id use before the console is enable you will > have the same issue. the ramc controller are basic device so people usally > don't touch it except you add a SoC support. > > so I prefer to panic > Then panic in the restart code if it doesn't have something it needs. Rob
On 09:24 Thu 08 Mar , Rob Herring wrote: > On 03/08/2012 08:10 AM, Jean-Christophe PLAGNIOL-VILLARD wrote: > > On 08:12 Thu 08 Mar , Rob Herring wrote: > >> On 03/08/2012 12:13 AM, Jean-Christophe PLAGNIOL-VILLARD wrote: > >>>>> diff --git a/arch/arm/mach-at91/include/mach/at91sam9x5.h b/arch/arm/mach-at91/include/mach/at91sam9x5.h > >>>>> index a297a77..88e43d5 100644 > >>>>> --- a/arch/arm/mach-at91/include/mach/at91sam9x5.h > >>>>> +++ b/arch/arm/mach-at91/include/mach/at91sam9x5.h > >>>>> @@ -55,11 +55,6 @@ > >>>>> #define AT91SAM9X5_BASE_USART2 0xf8024000 > >>>>> > >>>>> /* > >>>>> - * System Peripherals > >>>>> - */ > >>>>> -#define AT91SAM9X5_BASE_DDRSDRC0 0xffffe800 > >>>>> - > >>>>> -/* > >>>>> * Base addresses for early serial code (uncompress.h) > >>>>> */ > >>>>> #define AT91_DBGU AT91_BASE_DBGU0 > >>>>> diff --git a/arch/arm/mach-at91/setup.c b/arch/arm/mach-at91/setup.c > >>>>> index 3e48b59..f86450d 100644 > >>>>> --- a/arch/arm/mach-at91/setup.c > >>>>> +++ b/arch/arm/mach-at91/setup.c > >>>>> @@ -315,12 +315,33 @@ static void at91_dt_rstc(void) > >>>>> of_node_put(np); > >>>>> } > >>>>> > >>>>> +static struct of_device_id ramc_ids[] = { > >>>>> + { .compatible = "atmel,at91sam9260-sdramc" }, > >>>>> + { .compatible = "atmel,at91sam9g45-ddramc" }, > >>>>> + { /*sentinel*/ } > >>>>> +}; > >>>>> + > >>>>> +static void at91_dt_ramc(void) > >>>>> +{ > >>>>> + struct device_node *np; > >>>>> + > >>>>> + np = of_find_matching_node(NULL, ramc_ids); > >>>>> + if (!np) > >>>>> + panic("unable to find compatible ram conroller node in dtb\n"); > >>>> > >>>> You really can't boot if this fails? A WARN is better if it allows you > >>>> to boot until at least your console is actually up. > >>> if the restart is called you will have a oops so no it's a basic mandatory > >>> device on at91 > >>> > >> > >> But you may never see the panic message because your console is not up. > >> If you WARN and can continue to boot, then the user can see the problem > >> and fix it. Otherwise you get nothing and have to go rebuild and turn on > >> earlyprintk. > > yeah agreed but if the restart id use before the console is enable you will > > have the same issue. the ramc controller are basic device so people usally > > don't touch it except you add a SoC support. > > > > so I prefer to panic > > > > Then panic in the restart code if it doesn't have something it needs. The code is in assembly (mandatory) so difficult The warn on other missing binding agreed but here please let go It's basic device that except the maintainer will never touch People will just include the dtsi and if you really need to debug soc init enable earlyprintk is fine Best Regards, J.
On 03/08/2012 10:51 AM, Jean-Christophe PLAGNIOL-VILLARD wrote: > On 09:24 Thu 08 Mar , Rob Herring wrote: >> On 03/08/2012 08:10 AM, Jean-Christophe PLAGNIOL-VILLARD wrote: >>> On 08:12 Thu 08 Mar , Rob Herring wrote: >>>> On 03/08/2012 12:13 AM, Jean-Christophe PLAGNIOL-VILLARD wrote: >>>>>>> diff --git a/arch/arm/mach-at91/include/mach/at91sam9x5.h b/arch/arm/mach-at91/include/mach/at91sam9x5.h >>>>>>> index a297a77..88e43d5 100644 >>>>>>> --- a/arch/arm/mach-at91/include/mach/at91sam9x5.h >>>>>>> +++ b/arch/arm/mach-at91/include/mach/at91sam9x5.h >>>>>>> @@ -55,11 +55,6 @@ >>>>>>> #define AT91SAM9X5_BASE_USART2 0xf8024000 >>>>>>> >>>>>>> /* >>>>>>> - * System Peripherals >>>>>>> - */ >>>>>>> -#define AT91SAM9X5_BASE_DDRSDRC0 0xffffe800 >>>>>>> - >>>>>>> -/* >>>>>>> * Base addresses for early serial code (uncompress.h) >>>>>>> */ >>>>>>> #define AT91_DBGU AT91_BASE_DBGU0 >>>>>>> diff --git a/arch/arm/mach-at91/setup.c b/arch/arm/mach-at91/setup.c >>>>>>> index 3e48b59..f86450d 100644 >>>>>>> --- a/arch/arm/mach-at91/setup.c >>>>>>> +++ b/arch/arm/mach-at91/setup.c >>>>>>> @@ -315,12 +315,33 @@ static void at91_dt_rstc(void) >>>>>>> of_node_put(np); >>>>>>> } >>>>>>> >>>>>>> +static struct of_device_id ramc_ids[] = { >>>>>>> + { .compatible = "atmel,at91sam9260-sdramc" }, >>>>>>> + { .compatible = "atmel,at91sam9g45-ddramc" }, >>>>>>> + { /*sentinel*/ } >>>>>>> +}; >>>>>>> + >>>>>>> +static void at91_dt_ramc(void) >>>>>>> +{ >>>>>>> + struct device_node *np; >>>>>>> + >>>>>>> + np = of_find_matching_node(NULL, ramc_ids); >>>>>>> + if (!np) >>>>>>> + panic("unable to find compatible ram conroller node in dtb\n"); >>>>>> >>>>>> You really can't boot if this fails? A WARN is better if it allows you >>>>>> to boot until at least your console is actually up. >>>>> if the restart is called you will have a oops so no it's a basic mandatory >>>>> device on at91 >>>>> >>>> >>>> But you may never see the panic message because your console is not up. >>>> If you WARN and can continue to boot, then the user can see the problem >>>> and fix it. Otherwise you get nothing and have to go rebuild and turn on >>>> earlyprintk. >>> yeah agreed but if the restart id use before the console is enable you will >>> have the same issue. the ramc controller are basic device so people usally >>> don't touch it except you add a SoC support. >>> >>> so I prefer to panic >>> >> >> Then panic in the restart code if it doesn't have something it needs. > The code is in assembly (mandatory) so difficult > > The warn on other missing binding agreed but here please let go > It's basic device that except the maintainer will never touch > > People will just include the dtsi and if you really need to debug soc init > enable earlyprintk is fine Okay. Acked-by: Rob Herring <rob.herring@calxeda.com>
HI, The following patch series add the bindings for: - PMC - SDRAM/DDR Controller - Reset Controller - Shutdown Controller The following changes since commit c5ec01650adb1976f928177cd7e71afcb82026c5: ARM: at91: dt: enable usb ehci for sam9g45 and sam9x5 (2012-03-02 00:46:37 +0800) are available in the git repository at: git://github.com/at91linux/linux-at91.git ..BRANCH.NOT.VERIFIED.. Jean-Christophe PLAGNIOL-VILLARD (6): ARM: at91/dt: add specific DT soc init ARM: at91: add pmc DT support ARM: at91: always enable sam9 restart ARM: at91: add RSTC (Reset Controller) dt support ARM: at91: add ram controller DT support ARM: at91: add Shutdown Controller (SHDWC) DT support .../devicetree/bindings/arm/atmel-at91.txt | 60 ++++++++ .../devicetree/bindings/arm/atmel-pmc.txt | 11 ++ arch/arm/boot/dts/at91sam9g20.dtsi | 20 +++ arch/arm/boot/dts/at91sam9g45.dtsi | 21 +++ arch/arm/boot/dts/at91sam9m10g45ek.dts | 11 ++ arch/arm/boot/dts/at91sam9x5.dtsi | 20 +++ arch/arm/boot/dts/at91sam9x5cm.dtsi | 11 ++ arch/arm/boot/dts/usb_a9g20.dts | 11 ++ arch/arm/mach-at91/Kconfig | 10 +- arch/arm/mach-at91/at91sam9x5.c | 7 - arch/arm/mach-at91/board-dt.c | 8 +- arch/arm/mach-at91/clock.c | 54 ++++++- arch/arm/mach-at91/generic.h | 2 + arch/arm/mach-at91/include/mach/at91_shdwc.h | 4 +- arch/arm/mach-at91/include/mach/at91sam9x5.h | 5 - arch/arm/mach-at91/setup.c | 158 ++++++++++++++++++++ 16 files changed, 380 insertions(+), 33 deletions(-) create mode 100644 Documentation/devicetree/bindings/arm/atmel-pmc.txt Best Regards, J.