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

[Kavyasree.Kotagiri]

> > 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@xxxxxxxxxxxxx>
> >
> > Looks good to me:
> > Reviewed-by: Nicolas Ferre <nicolas.ferre@xxxxxxxxxxxxx>
> 
> I'm not quite sure what to do with this, as this was sent to:soc@xxxxxxxxxx,
> 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@60000000 {
> > > +             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