Date:   Wed, 10 Nov 2021 18:08:05 -0800
From:   Brad Larson <brad@...sando.io>
To:     Rob Herring <robh@...nel.org>
Cc:     Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Arnd Bergmann <arnd@...db.de>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Mark Brown <broonie@...nel.org>,
        Serge Semin <fancer.lancer@...il.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Olof Johansson <olof@...om.net>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        linux-spi <linux-spi@...r.kernel.org>,
        linux-mmc <linux-mmc@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 11/11] arm64: dts: Add Pensando Elba SoC support

Hi Rob,

On Wed, Oct 27, 2021 at 2:37 PM Rob Herring <robh@...nel.org> wrote:
>
> > +always-y     := $(dtb-y)
> > +subdir-y     := $(dts-dirs)
> > +clean-files  := *.dtb
>
> None of these lines should be needed.

Yes, these will be removed.

> > diff --git a/arch/arm64/boot/dts/pensando/elba-16core.dtsi b/arch/arm64/boot/dts/pensando/elba-16core.dtsi
> > new file mode 100644
> > index 000000000000..acf5941afbc1
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/pensando/elba-16core.dtsi
> > @@ -0,0 +1,192 @@
> > +// SPDX-License-Identifier: GPL-2.0
>
> Do you care about using with non-GPL OS? Dual license is preferred.
>
Dual is fine, changing to this:
SPDX-License-Identifier: (GPL-2.0+ OR MIT)

> > +             psci {
>
> This goes at the root level.
>
Moving to the root level

> > +                     compatible = "arm,psci-0.2";
> > +                     method = "smc";
> > +             };
> > +
> > +     };
> > +};
> > diff --git a/arch/arm64/boot/dts/pensando/elba-asic-common.dtsi b/arch/arm64/boot/dts/pensando/elba-asic-common.dtsi
> > +
> > +&spi0 {
> > +     num-cs = <4>;
> > +     cs-gpios = <0>, <0>, <&porta 1 GPIO_ACTIVE_LOW>,
> > +                <&porta 7 GPIO_ACTIVE_LOW>;
> > +     status = "okay";
> > +     spi0_cs0@0 {
>
> Node names should reflect the class of device and use standard name
> defined in the DT spec. This probably doesn't have one. 'lora' perhaps?
>
Right, I didn't see a standard name and found many approaches in other files
so I likely based off of one of these below.   I searched the dts
files for 'lora' and
didn't find it.  Is that an acronym?  I can change it to what the preference is.

./microchip/sparx5_pcb134_board.dtsi:
&spi0 {
   status = "okay";
   spi@0 {
           compatible = "spi-mux";
           ...
};

./rockchip/rk3399.dtsi:
spi5 {
        spi5_clk: spi5-clk {
                rockchip,pins =
                        <2 RK_PC6 2 &pcfg_pull_up>;
        };
        spi5_cs0: spi5-cs0 {
                rockchip,pins =
                        <2 RK_PC7 2 &pcfg_pull_up>;
        };
        spi5_rx: spi5-rx {
                rockchip,pins =
                        <2 RK_PC4 2 &pcfg_pull_up>;
        };
        spi5_tx: spi5-tx {
                rockchip,pins =
                        <2 RK_PC5 2 &pcfg_pull_up>;
        };
};

>
> > +             compatible = "semtech,sx1301";  /* Enable spidev */
>
> What's spidev?
>
It's module drivers/spi/spidev.c which won't populate /dev/spidevB.C unless
there is a match which we need for the system to boot.  An earlier patch added
to the compatible list below and the feedback on that was to remove it.  Later I
noticed the compatible list expanded...

static const struct of_device_id spidev_dt_ids[] = {
        { .compatible = "rohm,dh2228fv" },
        { .compatible = "lineartechnology,ltc2488" },
        { .compatible = "semtech,sx1301" },
        { .compatible = "lwn,bk4" },
        { .compatible = "dh,dhcom-board" },
        { .compatible = "menlo,m53cpld" },
        { .compatible = "cisco,spi-petra" },
        { .compatible = "micron,spi-authenta" },
        {},
};

> > +             #address-cells = <1>;
> > +             #size-cells = <1>;
> > +             spi-max-frequency = <12000000>;
> > +             reg = <0>;
> > +     };
> > +
> > +     spi0_cs1@1 {
> > +             compatible = "semtech,sx1301";
> > +             #address-cells = <1>;
> > +             #size-cells = <1>;
> > +             spi-max-frequency = <12000000>;
> > +             reg = <1>;
> > +     };
> > +
> > +     spi0_cs2@2 {
> > +             compatible = "semtech,sx1301";
> > +             #address-cells = <1>;
> > +             #size-cells = <1>;
> > +             spi-max-frequency = <12000000>;
> > +             reg = <2>;
> > +             interrupt-parent = <&porta>;
> > +             interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> > +     };
> > +
> > +     spi0_cs3@3 {
> > +             compatible = "semtech,sx1301";
> > +             #address-cells = <1>;
> > +             #size-cells = <1>;
> > +             spi-max-frequency = <12000000>;
> > +             reg = <3>;
> > +     };
> > +};
> > diff --git a/arch/arm64/boot/dts/pensando/elba-asic.dts b/arch/arm64/boot/dts/pensando/elba-asic.dts
> > new file mode 100644
> > index 000000000000..131931dc643f
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/pensando/elba-asic.dts
> > @@ -0,0 +1,23 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/dts-v1/;
> > +
> > +/ {
> > +     model = "Elba ASIC Board";
> > +     compatible = "pensando,elba";
>
> Normally we have a compatible for the board plus the soc compatible.

In this case there are currently five different boards/products that have no
variations needing a board level description.

Thanks,
Brad

Powered by blists - more mailing lists