linux-kernel - RE: [PATCH v3] ARM: dts: add DT for lan966x SoC and 2-port board pcb8291

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CO1PR11MB48651C0D73189AF010407F9B922E9@CO1PR11MB4865.namprd11.prod.outlook.com>
Date:   Wed, 9 Feb 2022 09:02:20 +0000
From:   <Kavyasree.Kotagiri@...rochip.com>
To:     <arnd@...db.de>, <Nicolas.Ferre@...rochip.com>
CC:     <olof@...om.net>, <soc@...nel.org>, <robh+dt@...nel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <UNGLinuxDriver@...rochip.com>
Subject: RE: [PATCH v3] ARM: dts: add DT for lan966x SoC and 2-port board
 pcb8291

> > On 13/01/2022 at 09:00, Kavyasree Kotagiri wrote:
> > > This patch adds basic DT for Microchip lan966x SoC and associated board
> > > pcb8291(2-port EVB). Adds peripherals required to allow booting: IRQs,
> > > clocks, timers, memory, flexcoms, GPIOs. Also adds other peripherals like
> > > crypto(AES,SHA), DMA and watchdog.
> > >
> > > Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@...rochip.com>
> >
> > Looks good to me:
> > Reviewed-by: Nicolas Ferre <nicolas.ferre@...rochip.com>
> 
> I'm not quite sure what to do with this, as this was sent to:soc@...nel.org,
> which is normally for patches that are already reviewed and should just
> get applied.
> 
> I can apply this, but I would normally expect board files to get picked up
> in the at91 tree first. I'll drop this version from patchwork for now, as I
> also have a couple of comments:
> 
> > > +
> > > +/ {
> > > +     model = "Microchip LAN966x family SoC";
> > > +     compatible = "microchip,lan966x";
> 
> By convention, the 'compatible' strings should not contain 'x'
> as a wildcard character. Just pick one of the models to be
> compatible with. The .dtsi file doesn't really need a top-level
> compatible or model property though, as they need to be
> overridden by teh board anyway.
>
Ok, I will change it.
 
> > > +
> > > +     memory@...00000 {
> > > +             device_type = "memory";
> > > +             reg = <0x60000000 0x40000000>;  /* 1GB */
> > > +     };
> 
> Probably also no memory node. This tends to be filled by the
> boot loader, or it is part of the board when when the boot loader
> is too old for that.
> 
> If the memory is part of the chip package, having it in the .dtsi
> file is probably ok, but I would add a comment for that.
> 
Ok.
> > > + */
> > > +/dts-v1/;
> > > +#include "lan966x.dtsi"
> > > +
> > > +/ {
> > > +     model = "Microchip EVB - LAN9662";
> > > +     compatible = "microchip,lan9662-pcb8291", "microchip,lan9662",
> "microchip,lan966";
> > > +};
> 
> Here I would expect /chosen and /aliases nodes.
> 
I will add them.

> > > +&gpio {
> > > +     fc_shrd7_pins: fc_shrd7-pins {
> > > +             pins = "GPIO_49";
> > > +             function = "fc_shrd7";
> > > +     };
> 
> These properties don't look like most pinctrl nodes, has the binding
> been reviewed?
> I don't see it in Documentation/devicetree/bindings/pinctrl/
>
This is similar to the ones used in Microchip Ocelot and Sparx5 pinctrl.
For example, see usart_pins of gpio nodes in below links: 
https://sbexr.rabexc.org/latest/sources//84/d39b543790ff25.jhtml
https://searchcode.com/file/333750634/arch/mips/boot/dts/mscc/ocelot.dtsi/

 
>           Arnd