Patchwork [0/6] AT91 more DT bindings

login
register
mail settings
Submitter Jean-Christophe PLAGNIOL-VILLARD
Date March 2, 2012, 7:28 p.m.
Message ID <20120302192844.GB21255@game.jcrosoft.org>
Download mbox
Permalink /patch/144322/
State New
Headers show

Pull-request

git://github.com/at91linux/linux-at91.git ..BRANCH.NOT.VERIFIED..

Comments

Jean-Christophe PLAGNIOL-VILLARD - March 2, 2012, 7:28 p.m.
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.
Arnd Bergmann - March 2, 2012, 8:24 p.m.
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
Jean-Christophe PLAGNIOL-VILLARD - March 7, 2012, 5:38 p.m.
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.
Jean-Christophe PLAGNIOL-VILLARD - March 7, 2012, 5:39 p.m.
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
Arnd Bergmann - March 7, 2012, 6:49 p.m.
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
Jean-Christophe PLAGNIOL-VILLARD - March 7, 2012, 6:59 p.m.
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.
Rob Herring - March 7, 2012, 7:54 p.m.
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
Rob Herring - March 7, 2012, 7:58 p.m.
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();
Arnd Bergmann - March 7, 2012, 9:16 p.m.
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
Jean-Christophe PLAGNIOL-VILLARD - March 8, 2012, 6:13 a.m.
> > 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.
Jean-Christophe PLAGNIOL-VILLARD - March 8, 2012, 2:10 p.m.
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.
Rob Herring - March 8, 2012, 2:12 p.m.
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
Rob Herring - March 8, 2012, 3:24 p.m.
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
Jean-Christophe PLAGNIOL-VILLARD - March 8, 2012, 4:51 p.m.
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.
Rob Herring - March 8, 2012, 5:13 p.m.
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>