lkml.org 
[lkml]   [2021]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 7/8] arm64: dts: Add Pensando Elba SoC support
    On Thu, Mar 4, 2021 at 4:41 AM Brad Larson <brad@pensando.io> wrote:
    >
    > Add Pensando common and Elba SoC specific device nodes
    > and corresponding binding documentation.
    >
    > Signed-off-by: Brad Larson <brad@pensando.io>
    > ---
    > .../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 000000000000..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@5a000000 {
    > - compatible = "socionext,uniphier-sd4hc", "cdns,sd4hc";
    > + compatible = "socionext,uniphier-sd4hc", "cdns,sd4hc", "pensando,elba-emmc";
    > reg = <0x5a000000 0x400>;
    > interrupts = <0 78 4>;
    > clocks = <&clk 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 = <&l2_0>;

    spin-table is not really something we want to see for new machines.
    Please add proper psci support to your boot loader.

    > index 000000000000..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 = &uart0;
    > + spi0 = &spi0;
    > + spi1 = &qspi;
    > + };
    > +
    > + 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 = <12000000>;
    > + 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 to a particular Linux device driver and exported to user
    space. The information in the dts should however not assume a particular
    OS implementation but describe the platform.

    Is this for PCI MSI? If so, I would recommend just using the GICv3 that you
    also have, and leave this device completely unused.

    In either case, please leave out the device node until a binding has
    been agreed and a matching kernel driver was reviewed (if any)

    > +
    > + /*
    > + * Until we know the interrupt domain following this, we
    > + * are forced to use this is the place where interrupts from
    > + * PCI converge. In the ideal case, we use one domain higher,
    > + * where the PCI-ness has been shed.
    > + */
    > + pxc0_intr: intc@20102200 {
    > + compatible = "pensando,soc-ictlr-csrintr";
    > + interrupt-controller;
    > + reg = <0x0 0x20102200 0x0 0x4>;
    > + #interrupt-cells = <3>;
    > + interrupt-parent = <&gic>;
    > + interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>;
    > + interrupt-names = "pxc0_intr";
    > + };

    Leave this one out as well, this has to be reviewed in combination with the
    PCI driver.

    > + pcie@307c2480 {
    > + compatible = "pensando,pcie";
    > + reg = <0x0 0x307c2480 0x0 0x4 /* MS CFG_WDT */
    > + 0x0 0x00001400 0x0 0x10 /* WDT0 */
    > + 0x0 0x20000000 0x0 0x00380000>; /* PXB Base */
    > + };

    This does not follow the PCI host bridge binding. Leave it out for now,
    and bring it back once you have a proper PCI driver.

    Arnd

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