Re: [PATCH 7/8] arm64: dts: Add Pensando Elba SoC support
On Thu, Mar 4, 2021 at 12:03 AM Serge Semin wrote:
>
> On Wed, Mar 03, 2021 at 07:41:40PM -0800, Brad Larson wrote:
> > Add Pensando common and Elba SoC specific device nodes
> > and corresponding binding documentation.
>
> This also needs to be split up into sub-patches seeing these are
> unrelated changes like device bindings update, new platform DT file.
In patchset v2 this is split into sub-patches.
> What about converting this file to DT-schema and adding new HW
> bindings in there?
Converted existing file devicetree/bindings/spi/cadence-quadspi.txt to
YAML schema.
> > + {
> > + num-cs = <4>;
>
> > + cs-gpios = < 0 0>, < 1 0>, < 1 0>, < 7 0>;
>
> Oh, you've got four peripheral SPI devices connected with only two native CS
> available. Hmm, then I don't really know a better way, but just to forget
> about
> the native DW APB CS functionality and activate the direct driving of
> all the CS-pins at the moment of the DW APB SPI controller probe
> procedure. Then indeed you'll need a custom CS function defined in the DW APB
> SPI driver to handle that.
Yes, with an Elba SoC specific gpio driver.
> So that GPIO-controller is just a single register which provides a way
> to toggle the DW APB SPI CS-mode together with their output value.
> If so and seeing there are a few more tiny spaces of config
> registers added to eMMC, PCI, etc DT node, I suppose all of them
> belong to some bigger config space of the SoC. Thus I'd suggest to at
> least implement them as part of a System Controller DT node. Then use
> that device service to switch on/off corresponding functionality.
> See [2] and the rest of added to the kernel DTS files with
> syscon-nodes for example.
>
> [2] Documentation/devicetree/bindings/mfd/syscon.yaml
To us it was more understandable to implement a standard gpio driver
for the spi chip-selects.
Re: [PATCH 7/8] arm64: dts: Add Pensando Elba SoC support
On Thu, Mar 4, 2021 at 12:52 AM Linus Walleij wrote: > > On Thu, Mar 4, 2021 at 4:42 AM Brad Larson wrote: > > > Add Pensando common and Elba SoC specific device nodes > > and corresponding binding documentation. > > > > Signed-off-by: Brad Larson > (...) > > .../bindings/gpio/pensando,elba-spics.txt | 24 ++ > > Please use YAML schema for this. In patchset v2 changed to YAML schema and passed dt_binding_check and dtbs_check.
Re: [PATCH 7/8] arm64: dts: Add Pensando Elba SoC support
On 04/03/2021 04:41, Brad Larson wrote:
> Add Pensando common and Elba SoC specific device nodes
> and corresponding binding documentation.
>
> Signed-off-by: Brad Larson
> ---
> .../bindings/gpio/pensando,elba-spics.txt | 24 ++
> .../devicetree/bindings/mmc/cdns,sdhci.yaml | 2 +-
> .../bindings/spi/cadence-quadspi.txt | 1 +
> .../devicetree/bindings/vendor-prefixes.yaml | 2 +
Hi,
dt-bindings go always to separate patches, at beginning of patchset.
> arch/arm64/boot/dts/Makefile | 1 +
> arch/arm64/boot/dts/pensando/Makefile | 6 +
> arch/arm64/boot/dts/pensando/elba-16core.dtsi | 171 ++
> .../boot/dts/pensando/elba-asic-common.dtsi | 113 +++
> arch/arm64/boot/dts/pensando/elba-asic.dts| 8 +
> .../boot/dts/pensando/elba-flash-parts.dtsi | 80 +
> arch/arm64/boot/dts/pensando/elba.dtsi| 310 ++
You need the board/vendor YAML file. See:
Documentation/devicetree/bindings/arm/
> 11 files changed, 717 insertions(+), 1 deletion(-)
> create mode 100644
> Documentation/devicetree/bindings/gpio/pensando,elba-spics.txt
> create mode 100644 arch/arm64/boot/dts/pensando/Makefile
> create mode 100644 arch/arm64/boot/dts/pensando/elba-16core.dtsi
> create mode 100644 arch/arm64/boot/dts/pensando/elba-asic-common.dtsi
> create mode 100644 arch/arm64/boot/dts/pensando/elba-asic.dts
> create mode 100644 arch/arm64/boot/dts/pensando/elba-flash-parts.dtsi
> create mode 100644 arch/arm64/boot/dts/pensando/elba.dtsi
>
> diff --git a/Documentation/devicetree/bindings/gpio/pensando,elba-spics.txt
> b/Documentation/devicetree/bindings/gpio/pensando,elba-spics.txt
> new file mode 100644
> index ..30f5f3275238
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/pensando,elba-spics.txt
> @@ -0,0 +1,24 @@
> +Pensando Elba SPI Chip Select Driver
> +
> +The Pensando Elba ASIC provides four SPI bus chip selects
> +
> +Required properties:
> +- compatible: Should be "pensando,elba-spics"
> +- reg: Address range of spics controller
> +- gpio-controller: Marks the device node as gpio controller
> +- #gpio-cells: Must be 2
> +
> +Example:
> +---
> +spics: spics@307c2468 {
> +compatible = "pensando,elba-spics";
> +reg = <0x0 0x307c2468 0x0 0x4>;
> +gpio-controller;
> +#gpio-cells = <2>;
> +};
> +
> + {
> +num-cs = <4>;
> +cs-gpios = < 0 0>, < 1 0>, < 1 0>, < 7 0>;
> + ...
> +}
> diff --git a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> index af7442f73881..645ae696ba24 100644
> --- a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> +++ b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> @@ -122,7 +122,7 @@ unevaluatedProperties: false
> examples:
>- |
> emmc: mmc@5a00 {
> -compatible = "socionext,uniphier-sd4hc", "cdns,sd4hc";
> +compatible = "socionext,uniphier-sd4hc", "cdns,sd4hc",
> "pensando,elba-emmc";
Why are you doing this?
> reg = <0x5a00 0x400>;
> interrupts = <0 78 4>;
> clocks = < 4>;
> diff --git a/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
> b/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
> index 8ace832a2d80..dbb346b2b1d7 100644
> --- a/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
> +++ b/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
> @@ -6,6 +6,7 @@ Required properties:
> For TI 66AK2G SoC - "ti,k2g-qspi", "cdns,qspi-nor".
> For TI AM654 SoC - "ti,am654-ospi", "cdns,qspi-nor".
> For Intel LGM SoC - "intel,lgm-qspi", "cdns,qspi-nor".
> + For Pensando SoC - "pensando,cdns-qspi".
> - reg : Contains two entries, each of which is a tuple consisting of a
> physical address and length. The first entry is the address and
> length of the controller register set. The second entry is the
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index f6064d84a424..9a21d780c5e1 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -850,6 +850,8 @@ patternProperties:
> description: Parallax Inc.
>"^pda,.*":
> description: Precision Design Associates, Inc.
> + "^pensando,.*":
> +description: Pensando Systems Inc.
>"^pericom,.*":
> description: Pericom Technology Inc.
>"^pervasive,.*":
> diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile
> index f1173cd93594..c85db0a097fe 100644
> --- a/arch/arm64/boot/dts/Makefile
> +++ b/arch/arm64/boot/dts/Makefile
> @@ -19,6 +19,7 @@ subdir-y += marvell
> subdir-y += mediatek
> subdir-y += microchip
> subdir-y += nvidia
> +subdir-y += pensando
> subdir-y += qcom
> subdir-y += realtek
> subdir-y += renesas
> diff --git
Re: [PATCH 7/8] arm64: dts: Add Pensando Elba SoC support
On Wed, 03 Mar 2021 19:41:40 -0800, Brad Larson wrote:
> Add Pensando common and Elba SoC specific device nodes
> and corresponding binding documentation.
>
> Signed-off-by: Brad Larson
> ---
> .../bindings/gpio/pensando,elba-spics.txt | 24 ++
> .../devicetree/bindings/mmc/cdns,sdhci.yaml | 2 +-
> .../bindings/spi/cadence-quadspi.txt | 1 +
> .../devicetree/bindings/vendor-prefixes.yaml | 2 +
> arch/arm64/boot/dts/Makefile | 1 +
> arch/arm64/boot/dts/pensando/Makefile | 6 +
> arch/arm64/boot/dts/pensando/elba-16core.dtsi | 171 ++
> .../boot/dts/pensando/elba-asic-common.dtsi | 113 +++
> arch/arm64/boot/dts/pensando/elba-asic.dts| 8 +
> .../boot/dts/pensando/elba-flash-parts.dtsi | 80 +
> arch/arm64/boot/dts/pensando/elba.dtsi| 310 ++
> 11 files changed, 717 insertions(+), 1 deletion(-)
> create mode 100644
> Documentation/devicetree/bindings/gpio/pensando,elba-spics.txt
> create mode 100644 arch/arm64/boot/dts/pensando/Makefile
> create mode 100644 arch/arm64/boot/dts/pensando/elba-16core.dtsi
> create mode 100644 arch/arm64/boot/dts/pensando/elba-asic-common.dtsi
> create mode 100644 arch/arm64/boot/dts/pensando/elba-asic.dts
> create mode 100644 arch/arm64/boot/dts/pensando/elba-flash-parts.dtsi
> create mode 100644 arch/arm64/boot/dts/pensando/elba.dtsi
>
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mmc/cdns,sdhci.example.dt.yaml:
mmc@5a00: compatible: ['socionext,uniphier-sd4hc', 'cdns,sd4hc',
'pensando,elba-emmc'] is too long
From schema:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mmc/cdns,sdhci.example.dt.yaml:
mmc@5a00: compatible: Additional items are not allowed
('pensando,elba-emmc' was unexpected)
From schema:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
See https://patchwork.ozlabs.org/patch/1447072
This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit.
Re: [PATCH 7/8] arm64: dts: Add Pensando Elba SoC support
On Thu, Mar 4, 2021 at 4:41 AM Brad Larson wrote:
>
> Add Pensando common and Elba SoC specific device nodes
> and corresponding binding documentation.
>
> Signed-off-by: Brad Larson
> ---
> .../bindings/gpio/pensando,elba-spics.txt | 24 ++
> .../devicetree/bindings/mmc/cdns,sdhci.yaml | 2 +-
> .../bindings/spi/cadence-quadspi.txt | 1 +
It would be better to split each of the above out into a separate patch for
easier review, and send them along with the driver changes.
> diff --git a/Documentation/devicetree/bindings/gpio/pensando,elba-spics.txt
> b/Documentation/devicetree/bindings/gpio/pensando,elba-spics.txt
> new file mode 100644
> index ..30f5f3275238
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/pensando,elba-spics.txt
> @@ -0,0 +1,24 @@
> +Pensando Elba SPI Chip Select Driver
> +
> +The Pensando Elba ASIC provides four SPI bus chip selects
> +
> +Required properties:
> +- compatible: Should be "pensando,elba-spics"
> +- reg: Address range of spics controller
> +- gpio-controller: Marks the device node as gpio controller
> +- #gpio-cells: Must be 2
You need to document what each of the cells are for. In your
example, the second cell is always zero, is that intentional?
> diff --git a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> index af7442f73881..645ae696ba24 100644
> --- a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> +++ b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> @@ -122,7 +122,7 @@ unevaluatedProperties: false
> examples:
>- |
> emmc: mmc@5a00 {
> -compatible = "socionext,uniphier-sd4hc", "cdns,sd4hc";
> +compatible = "socionext,uniphier-sd4hc", "cdns,sd4hc",
> "pensando,elba-emmc";
> reg = <0x5a00 0x400>;
> interrupts = <0 78 4>;
> clocks = < 4>;
These are in the wrong order, the most generic one (cdns,sd4hc) always
comes last.
If you add the string in the example, it also has to be an option in the
actual binding, otherwise neither the example nor your dtb would
be valid.
You also wouldn't find a controller that is compatible with both the uniphier
variant and the elba variant, unless your 'elba' SoC is strictly derived from
Socionext's Uniphier products and inherits all the quirks in its sdhci
implementation that were not already part of Cadence's IP block.
> diff --git a/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
> b/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
> index 8ace832a2d80..dbb346b2b1d7 100644
> --- a/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
> +++ b/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
> @@ -6,6 +6,7 @@ Required properties:
> For TI 66AK2G SoC - "ti,k2g-qspi", "cdns,qspi-nor".
> For TI AM654 SoC - "ti,am654-ospi", "cdns,qspi-nor".
> For Intel LGM SoC - "intel,lgm-qspi", "cdns,qspi-nor".
> + For Pensando SoC - "pensando,cdns-qspi".
This does not look specific enough: There is no guarantee that this
is the only time Pensando uses any Cadenci qspi block. If the company
is not yet out of business, you should be prepared for future products
and have the name of the chip in there as well.
> + cpu0: cpu@0 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a72", "arm,armv8";
> + reg = <0 0x0>;
> + enable-method = "spin-table";
> + next-level-cache = <_0>;
spin-table is not really something we want to see for new machines.
Please add proper psci support to your boot loader.
> index ..9623df208131
> --- /dev/null
> +++ b/arch/arm64/boot/dts/pensando/elba-asic-common.dtsi
> @@ -0,0 +1,113 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/ {
> + model = "Elba ASIC Board";
> +
> + aliases {
> + serial0 =
> +spi0 =
> +spi1 =
> + };
> +
> + chosen {
> + stdout-path = "serial0:19200n8";
> + };
> +};
These properties are usually board specific, and should be moved into the
.dts file.
> + spi@0 {
> + compatible = "pensando,cpld";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + spi-max-frequency = <1200>;
> + reg = <0>;
> + };
> + spi@2 {
> + compatible = "pensando,cpld-rd1173";
These don't seem to have a binding document, which needs to be added
first. What is a Pensando "cpld"? Is it possible that there will be multiple
versions of it that need to be uniquely identified?
> +
> + /* Common UIO device for MSI drivers */
> + uio_penmsi {
> + compatible = "pensando,uio_penmsi";
> + name = "uio_penmsi";
> + };
Missing binding again. Since you name this a UIO device, I assume this
is actually tied
Re: [PATCH 7/8] arm64: dts: Add Pensando Elba SoC support
On Thu, Mar 4, 2021 at 4:42 AM Brad Larson wrote: > Add Pensando common and Elba SoC specific device nodes > and corresponding binding documentation. > > Signed-off-by: Brad Larson (...) > .../bindings/gpio/pensando,elba-spics.txt | 24 ++ Please use YAML schema for this. See Documentation/devicetree/writing-schema.rst for instructions, you need to install some python pip packages to test your schema. Yours, Linus Walleij
Re: [PATCH 7/8] arm64: dts: Add Pensando Elba SoC support
On Wed, Mar 03, 2021 at 07:41:40PM -0800, Brad Larson wrote:
> Add Pensando common and Elba SoC specific device nodes
> and corresponding binding documentation.
This also needs to be split up into sub-patches seeing these are
unrelated changes like device bindings update, new platform DT file.
Note text-based bindings are deprecated in favor of the DT schemas.
Also note dts needs to pass dtbs_check validation. So all new HW/DT
nodes need to be reflected in the DT-schemas. See [1] for details.
[1] Documentation/devicetree/writing-schema.rst
>
> Signed-off-by: Brad Larson
> ---
> .../bindings/gpio/pensando,elba-spics.txt | 24 ++
> .../devicetree/bindings/mmc/cdns,sdhci.yaml | 2 +-
> .../bindings/spi/cadence-quadspi.txt | 1 +
> .../devicetree/bindings/vendor-prefixes.yaml | 2 +
> arch/arm64/boot/dts/Makefile | 1 +
> arch/arm64/boot/dts/pensando/Makefile | 6 +
> arch/arm64/boot/dts/pensando/elba-16core.dtsi | 171 ++
> .../boot/dts/pensando/elba-asic-common.dtsi | 113 +++
> arch/arm64/boot/dts/pensando/elba-asic.dts| 8 +
> .../boot/dts/pensando/elba-flash-parts.dtsi | 80 +
> arch/arm64/boot/dts/pensando/elba.dtsi| 310 ++
> 11 files changed, 717 insertions(+), 1 deletion(-)
> create mode 100644
> Documentation/devicetree/bindings/gpio/pensando,elba-spics.txt
> create mode 100644 arch/arm64/boot/dts/pensando/Makefile
> create mode 100644 arch/arm64/boot/dts/pensando/elba-16core.dtsi
> create mode 100644 arch/arm64/boot/dts/pensando/elba-asic-common.dtsi
> create mode 100644 arch/arm64/boot/dts/pensando/elba-asic.dts
> create mode 100644 arch/arm64/boot/dts/pensando/elba-flash-parts.dtsi
> create mode 100644 arch/arm64/boot/dts/pensando/elba.dtsi
>
> diff --git a/Documentation/devicetree/bindings/gpio/pensando,elba-spics.txt
> b/Documentation/devicetree/bindings/gpio/pensando,elba-spics.txt
> new file mode 100644
> index ..30f5f3275238
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/pensando,elba-spics.txt
> @@ -0,0 +1,24 @@
> +Pensando Elba SPI Chip Select Driver
> +
> +The Pensando Elba ASIC provides four SPI bus chip selects
> +
> +Required properties:
> +- compatible: Should be "pensando,elba-spics"
> +- reg: Address range of spics controller
> +- gpio-controller: Marks the device node as gpio controller
> +- #gpio-cells: Must be 2
> +
> +Example:
> +---
> +spics: spics@307c2468 {
> +compatible = "pensando,elba-spics";
> +reg = <0x0 0x307c2468 0x0 0x4>;
> +gpio-controller;
> +#gpio-cells = <2>;
> +};
> +
> + {
> +num-cs = <4>;
> +cs-gpios = < 0 0>, < 1 0>, < 1 0>, < 7 0>;
> + ...
> +}
> diff --git a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> index af7442f73881..645ae696ba24 100644
> --- a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> +++ b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> @@ -122,7 +122,7 @@ unevaluatedProperties: false
> examples:
>- |
> emmc: mmc@5a00 {
> -compatible = "socionext,uniphier-sd4hc", "cdns,sd4hc";
> +compatible = "socionext,uniphier-sd4hc", "cdns,sd4hc",
> "pensando,elba-emmc";
Alas it's not enough. New HW compatible strings shall be defined in the
binding schema.
> reg = <0x5a00 0x400>;
> interrupts = <0 78 4>;
> clocks = < 4>;
> diff --git a/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
> b/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
> index 8ace832a2d80..dbb346b2b1d7 100644
> --- a/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
> +++ b/Documentation/devicetree/bindings/spi/cadence-quadspi.txt
> @@ -6,6 +6,7 @@ Required properties:
> For TI 66AK2G SoC - "ti,k2g-qspi", "cdns,qspi-nor".
> For TI AM654 SoC - "ti,am654-ospi", "cdns,qspi-nor".
> For Intel LGM SoC - "intel,lgm-qspi", "cdns,qspi-nor".
> + For Pensando SoC - "pensando,cdns-qspi".
What about converting this file to DT-schema and adding new HW
bindings in there?
> - reg : Contains two entries, each of which is a tuple consisting of a
> physical address and length. The first entry is the address and
> length of the controller register set. The second entry is the
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index f6064d84a424..9a21d780c5e1 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -850,6 +850,8 @@ patternProperties:
> description: Parallax Inc.
>"^pda,.*":
> description: Precision Design Associates, Inc.
> + "^pensando,.*":
> +description: Pensando Systems Inc.
>"^pericom,.*":
> description: Pericom Technology Inc.
>"^pervasive,.*":
> diff --git
