lkml.org 
[lkml]   [2018]   [Jan]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 2/3] ARM: dts: mvebu: add sdram controller node to Armada-38x
Date
Hi Chris,

On mer., janv. 10 2018, Chris Packham <Chris.Packham@alliedtelesis.co.nz> wrote:

> On 10/01/18 21:31, Gregory CLEMENT wrote:
>> Hi Chris,
>>
>> On mar., janv. 09 2018, Chris Packham <chris.packham@alliedtelesis.co.nz> 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@alliedtelesis.co.nz>
>>> ---
>>> 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

\
 
 \ /
  Last update: 2018-01-14 23:22    [W:0.079 / U:0.556 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site