by Brad Larson

[permalink] [raw]

Subject: Re: [PATCH v3 11/11] arm64: dts: Add Pensando Elba SoC support

On Mon, Nov 8, 2021 at 11:54 AM Marc Zyngier <[email protected]> wrote:
> >
> > The Elba SoC is an embedded chip and not intended as a SBSA-compliant
> > general platform.
>
> This has nothing to do with following a standard. It has to do with
> following the intended use of the architecture. What you have here is
> the system architecture equivalent of trusting userspace to build the
> kernel page tables. It can work in limited cases. But would you want
> to deploy such construct at scale? Probably not.
>
> > In this implementation the ITS is used to provide message-based
> > interrupts for our (potentially large set) of hardware based
> > platform device instances. Virtualization is not a consideration.
> > We don't have a SMMU. Interrupt isolation isn't a practical
> > consideration for this product.
>
> Because you have foreseen all use cases for this HW ahead of time, and
> can already tell how SW is going to make use of it? Oh well...
>
> > Propose adding a comment to the dts.
> >
> > + /*
> > + * Elba SoC implemented a pre-ITS that happened to
> > + * be the same implementation as synquacer.
> > + */
>
> Which contains zero information. What you really want is: "We have
> decided to ignore the system architecture, good luck".
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.

On the contrary, the confusion of using the existing driver match
"socionext,synquacer-pre-its" is answered, why add new code.
Looks like we are deviating from the norm ;-). I'm not seeing how
this conversation is a productive use of time for a platform in
production.

Thanks,
Brad

2021-11-11 02:09:51

by Brad Larson

[permalink] [raw]

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 <[email protected]> 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