lkml.org 
[lkml]   [2021]   [Nov]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [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@kernel.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

    \
     
     \ /
      Last update: 2021-11-11 03:10    [W:3.044 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site