Re: [PATCH 2/3] ARM: dts: mvebu: add sdram controller node to Armada-38x

[Gregory CLEMENT]

Hi Chris,
 
 On mer., janv. 10 2018, Chris Packham <Chris.Packham@xxxxxxxxxxxxxxxxxxx> wrote:

> On 10/01/18 21:31, Gregory CLEMENT wrote:
>> Hi Chris,
>>   
>>   On mar., janv. 09 2018, Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx> wrote:
>> 
>>> The Armada-38x uses an SDRAM controller that is compatible with the
>>> Armada-XP. The key difference is the width of the bus (XP is 64/32, 38x
>>> is 32/16). The SDRAM controller registers are the same between the two
>>> SoCs.
>>>
>>> Signed-off-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
>>> ---
>>>   arch/arm/boot/dts/armada-38x.dtsi | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/armada-38x.dtsi b/arch/arm/boot/dts/armada-38x.dtsi
>>> index 00ff549d4e39..6d34c5ec178f 100644
>>> --- a/arch/arm/boot/dts/armada-38x.dtsi
>>> +++ b/arch/arm/boot/dts/armada-38x.dtsi
>>> @@ -138,6 +138,11 @@
>>>   			#size-cells = <1>;
>>>   			ranges = <0 MBUS_ID(0xf0, 0x01) 0 0x100000>;
>>>   
>>> +			sdramc@1400 {
>> 
>> Could you add a label? Thanks to this it would be possible to
>> enable/disable it at board level in a esay way.
>> 
>
> Sure. Any suggestions for a name better than "sdramc:"?

For me sdramc: is fine.

>
> It's probably worth adding the same label to armada-xp.dtsi and 
> armada-xp-98dx3236.dtsi.

Right.

>
>>> +				compatible = "marvell,armada-xp-sdram-controller";
>>> +				reg = <0x1400 0x500>;
>> 
>> What about adding status = "disabled" ?
>> 
>> Thanks to this we can enable it at board level only if we really want
>> it, it would avoid nasty regression on boards that don't need it, if an
>> issue occurs. Unless you are sure that it is completely safe to enable
>> it for everyone.
>
> The EDAC driver (which is default n) will not probe the device if ECC 
> has not been enabled so that should be safe.

OK in this case no need to disable it by default.

Thanks,

Gregory

>
> Other than the EDAC driver the only other code that looks at this is in 
> arch/arm/mach-mvebu/pm.c and it almost seems like an omission that this 
> code is not active on armada-38x. The armada-38x platforms I have access 
> to don't use suspend/resume so I can't verify this.
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com