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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87efmw7oiq.fsf@free-electrons.com>
Date:   Thu, 11 Jan 2018 10:06:37 +0100
From:   Gregory CLEMENT <gregory.clement@...e-electrons.com>
To:     Chris Packham <Chris.Packham@...iedtelesis.co.nz>
Cc:     "jlu\@pengutronix.de" <jlu@...gutronix.de>,
        "linux\@armlinux.org.uk" <linux@...linux.org.uk>,
        "bp\@alien8.de" <bp@...en8.de>,
        "linux-arm-kernel\@lists.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-edac\@vger.kernel.org" <linux-edac@...r.kernel.org>,
        "linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
        Jason Cooper <jason@...edaemon.net>,
        "Andrew Lunn" <andrew@...n.ch>,
        Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        "Mark Rutland" <mark.rutland@....com>,
        "devicetree\@vger.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH 2/3] ARM: dts: mvebu: add sdram controller node to Armada-38x

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

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