| Message ID | 20220613195658.5607-1-brad@pensando.io |
|---|---|
| Headers | show |
| Series | Support AMD Pensando Elba SoC | expand |
On Mon, Jun 13, 2022 at 9:57 PM Brad Larson <brad@pensando.io> wrote: > > From: Brad Larson <blarson@amd.com> > > The AMD Pensando Elba SoC includes a DW apb_ssi v4 controller > with device specific chip-select control. The Elba SoC > provides four chip-selects where the native DW IP supports > two chip-selects. The Elba DW_SPI instance has two native > CS signals that are always overridden. ... > +/* > + * Elba SoC does not use ssi, pin override is used for cs 0,1 and > + * gpios for cs 2,3 as defined in the device tree. > + * > + * cs: | 1 0 > + * bit: |---3-------2-------1-------0 > + * | cs1 cs1_ovr cs0 cs0_ovr > + */ > +#define ELBA_SPICS_SHIFT(cs) (2 * (cs)) Useless.It takes much more than simply multiplying each time in two macros. Also see below. > +#define ELBA_SPICS_MASK(cs) (0x3 << ELBA_SPICS_SHIFT(cs)) (GENMASK(1, 0) << ((cs) << 1)) Or ((cs) * 2) to show that it takes 2 bits and not two times of CS', > +#define ELBA_SPICS_SET(cs, val) \ > + ((((val) << 1) | 0x1) << ELBA_SPICS_SHIFT(cs)) BIT(0) So the main point is to use GENMASK() and BIT() the rest is up to you.
On Mon, Jun 13, 2022 at 9:57 PM Brad Larson <brad@pensando.io> wrote: > > From: Brad Larson <blarson@amd.com> > > Add support for AMD Pensando Elba SoC which explicitly controls > byte-lane enables on writes. Add priv_write_l() which is enabling ? > used on Elba platforms for byte-lane control. > > Select MMC_SDHCI_IO_ACCESSORS for MMC_SDHCI_CADENCE which > allows Elba SoC sdhci_elba_ops to overwrite the SDHCI > IO memory accessors. ... > + void (*priv_write_l)(struct sdhci_cdns_priv *priv, u32 val, priv_writel > + void __iomem *reg); And perhaps leave it on one line. I also would swap parameters, so address goes first followed by value. ... > +static inline void sdhci_cdns_priv_writel(struct sdhci_cdns_priv *priv, > + u32 val, void __iomem *reg) > +{ > + if (unlikely(priv->priv_write_l)) First of all, why if (unlikely())-else instead of if (likely())-else? > + priv->priv_write_l(priv, val, reg); > + else > + writel(val, reg); > +} Instead of branching each time you do I/O, make sure that callback is always set and call it unconditionally. In this case you don't need to have this callback, but maybe just a wrapper on `writel()`. As a result you may split this to two patches in the first of which you simply introduce a callback and a writel() wrapper which is assigned unconditionally to all current chips. In the next you add a new chip support. ... > + u32 m = (reg & 0x3); > + u32 msk = (0x3 << (m)); > + > + spin_lock_irqsave(&priv->wrlock, flags); > + writel(msk << 3, priv->ctl_addr); > + writew(val, host->ioaddr + reg); > + spin_unlock_irqrestore(&priv->wrlock, flags); Too many 3:s as magic. Is it GENMASK() or something else? Perhaps it needs a definition. ... > + u32 m = (reg & 0x3); > + u32 msk = (0x1 << (m)); > + > + spin_lock_irqsave(&priv->wrlock, flags); > + writel(msk << 3, priv->ctl_addr); > + writeb(val, host->ioaddr + reg); > + spin_unlock_irqrestore(&priv->wrlock, flags); Ditto. ... > + writel(0x78, priv->ctl_addr); Magic. ... > +static const struct sdhci_cdns_drv_data sdhci_cdns_drv_data = { > + .pltfm_data = { > + .ops = &sdhci_cdns_ops, > + }, > +}; > + > + One blank line is enough. ... > + { > + .compatible = "amd,pensando-elba-sd4hc", > + .data = &sdhci_elba_drv_data Leave a comma here. > },
On Mon, 13 Jun 2022 12:56:44 -0700, Brad Larson wrote: > From: Brad Larson <blarson@amd.com> > > Document the compatible for AMD Pensando Elba SoC boards. > > Signed-off-by: Brad Larson <blarson@amd.com> > --- > .../devicetree/bindings/arm/amd,pensando.yaml | 26 +++++++++++++++++++ > 1 file changed, 26 insertions(+) > create mode 100644 Documentation/devicetree/bindings/arm/amd,pensando.yaml > Reviewed-by: Rob Herring <robh@kernel.org>
On Mon, Jun 13, 2022 at 12:56:45PM -0700, Brad Larson wrote: > From: Brad Larson <blarson@amd.com> > > AMD Pensando Elba ARM 64-bit SoC is integrated with this IP and > explicitly controls byte-lane enables. > > Signed-off-by: Brad Larson <blarson@amd.com> > --- > .../devicetree/bindings/mmc/cdns,sdhci.yaml | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml > index 4207fed62dfe..35bc4cf6f214 100644 > --- a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml > +++ b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml > @@ -13,10 +13,24 @@ maintainers: > allOf: > - $ref: mmc-controller.yaml > > + - if: > + properties: > + compatible: > + enum: > + - amd,pensando-elba-sd4hc > + then: > + properties: > + reg: > + items: > + - description: Cadence host controller registers > + - description: Byte-lane control register > + minItems: 2 This doesn't work. The if/then is additional constraints on the main section which says there is only 1 register region. The main section needs the above, but with 'minItems: 1'. Then the if/then should be: if: properties: compatible: const: amd,pensando-elba-sd4hc then: properties: reg: minItems: 2 else: properties: reg: maxItems: 1 > + > properties: > compatible: > items: > - enum: > + - amd,pensando-elba-sd4hc > - microchip,mpfs-sd4hc > - socionext,uniphier-sd4hc > - const: cdns,sd4hc > -- > 2.17.1 > >
On Mon, 13 Jun 2022 12:56:48 -0700, Brad Larson wrote: > From: Brad Larson <blarson@amd.com> > > Add the AMD Pensando Elba SoC system registers compatible. > > Signed-off-by: Brad Larson <blarson@amd.com> > --- > Documentation/devicetree/bindings/mfd/syscon.yaml | 1 + > 1 file changed, 1 insertion(+) > Acked-by: Rob Herring <robh@kernel.org>
On Mon, Jun 13, 2022 at 12:56:50PM -0700, Brad Larson wrote: > From: Brad Larson <blarson@amd.com> > > Document bindings for AMD Pensando Elba SR Reset Controller > > Signed-off-by: Brad Larson <blarson@amd.com> > --- > .../reset/amd,pensando-elbasr-reset.yaml | 62 +++++++++++++++++++ > 1 file changed, 62 insertions(+) > create mode 100644 Documentation/devicetree/bindings/reset/amd,pensando-elbasr-reset.yaml > > diff --git a/Documentation/devicetree/bindings/reset/amd,pensando-elbasr-reset.yaml b/Documentation/devicetree/bindings/reset/amd,pensando-elbasr-reset.yaml > new file mode 100644 > index 000000000000..03bb86ebcfd3 > --- /dev/null > +++ b/Documentation/devicetree/bindings/reset/amd,pensando-elbasr-reset.yaml > @@ -0,0 +1,62 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/reset/amd,pensando-elbasr-reset.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: AMD Pensando Elba SoC Reset Controller Device Tree Bindings > + > +maintainers: > + - Brad Larson <blarson@amd.com> > + > +description: | > + AMD Pensando Elba SoC reset controller driver which supports a resource > + controller connected to the Elba SoC over a SPI bus. The Elba reset > + controller must be defined as a child node of the Elba SPI bus > + chip-select 0 node. > + > + See also: > + - dt-bindings/reset/amd,pensando-elba-reset.h > + > +properties: > + $nodename: > + pattern: "^reset-controller@[0-9a-f]+$" > + > + compatible: > + const: amd,pensando-elbasr-reset > + > + reg: > + const: 0 > + > + '#reset-cells': > + const: 1 > + > +required: > + - compatible > + - reg > + - '#reset-cells' > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/reset/amd,pensando-elba-reset.h> > + spi0 { > + #address-cells = <1>; > + #size-cells = <0>; > + num-cs = <4>; > + > + spi@0 { 'spi' is reserved for SPI buses. I thought this was an MFD. > + reg = <0>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + rstc: reset-controller@0 { > + compatible = "amd,pensando-elbasr-reset"; > + reg = <0>; > + #reset-cells = <1>; > + }; > + }; > + }; > + > +... > -- > 2.17.1 > >
On 13/06/2022 12:56, Brad Larson wrote: > From: Brad Larson <blarson@amd.com> > > Add AMD Pensando common and Elba SoC specific device nodes > > Signed-off-by: Brad Larson <blarson@amd.com> Thank you for your patch. There is something to discuss/improve. > --- > arch/arm64/boot/dts/amd/Makefile | 1 + > arch/arm64/boot/dts/amd/elba-16core.dtsi | 189 +++++++++++++++++ > arch/arm64/boot/dts/amd/elba-asic-common.dtsi | 103 ++++++++++ > arch/arm64/boot/dts/amd/elba-asic.dts | 28 +++ > arch/arm64/boot/dts/amd/elba-flash-parts.dtsi | 106 ++++++++++ > arch/arm64/boot/dts/amd/elba.dtsi | 191 ++++++++++++++++++ > 6 files changed, 618 insertions(+) > create mode 100644 arch/arm64/boot/dts/amd/elba-16core.dtsi > create mode 100644 arch/arm64/boot/dts/amd/elba-asic-common.dtsi > create mode 100644 arch/arm64/boot/dts/amd/elba-asic.dts > create mode 100644 arch/arm64/boot/dts/amd/elba-flash-parts.dtsi > create mode 100644 arch/arm64/boot/dts/amd/elba.dtsi > > diff --git a/arch/arm64/boot/dts/amd/Makefile b/arch/arm64/boot/dts/amd/Makefile > index 68103a8b0ef5..9bba020fa880 100644 > --- a/arch/arm64/boot/dts/amd/Makefile > +++ b/arch/arm64/boot/dts/amd/Makefile > @@ -1,2 +1,3 @@ > # SPDX-License-Identifier: GPL-2.0 > dtb-$(CONFIG_ARCH_SEATTLE) += amd-overdrive-rev-b0.dtb amd-overdrive-rev-b1.dtb > +dtb-$(CONFIG_ARCH_PENSANDO) += elba-asic.dtb Put it in alphabetical order, so not at the end of file. (...) > + > +&i2c0 { > + clock-frequency = <100000>; > + status = "okay"; > + rtc@51 { > + compatible = "nxp,pcf85263"; > + reg = <0x51>; > + }; > +}; > + > +&spi0 { > + num-cs = <4>; > + cs-gpios = <0>, <0>, <&porta 1 GPIO_ACTIVE_LOW>, > + <&porta 7 GPIO_ACTIVE_LOW>; > + status = "okay"; > + spi@0 { Rob's comment about bindings applies here as well, so please fix both. This has to be sorted out - either it is SPI controller or MFD. Rest looks okay for me. > + compatible = "amd,pensando-elbasr", "simple-mfd"; > + reg = <0>; > + #address-cells = <1>; > + #size-cells = <0>; > + spi-max-frequency = <12000000>; > + Best regards, Krzysztof
Hi Andy, On Tue, Jun 14, 2022 at 4:10 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Jun 13, 2022 at 9:57 PM Brad Larson <brad@pensando.io> wrote: > ... > > > +/* > > + * Elba SoC does not use ssi, pin override is used for cs 0,1 and > > + * gpios for cs 2,3 as defined in the device tree. > > + * > > + * cs: | 1 0 > > + * bit: |---3-------2-------1-------0 > > + * | cs1 cs1_ovr cs0 cs0_ovr > > + */ > > > +#define ELBA_SPICS_SHIFT(cs) (2 * (cs)) > > Useless.It takes much more than simply multiplying each time in two > macros. Also see below. > > > +#define ELBA_SPICS_MASK(cs) (0x3 << ELBA_SPICS_SHIFT(cs)) > > (GENMASK(1, 0) << ((cs) << 1)) > > Or ((cs) * 2) to show that it takes 2 bits and not two times of CS', > > > +#define ELBA_SPICS_SET(cs, val) \ > > + ((((val) << 1) | 0x1) << ELBA_SPICS_SHIFT(cs)) > > BIT(0) > > So the main point is to use GENMASK() and BIT() the rest is up to you. I think you're recommending this approach which I'll change to static void dw_spi_elba_override_cs(struct dw_spi_elba *dwselba, int cs, int enable) { regmap_update_bits(dwselba->syscon, ELBA_SPICS_REG, (GENMASK(1, 0) << ((cs) << 1)), ((enable) << 1 | BIT(0)) << ((cs) << 1)); } Regards, Brad
Hi Andy, On Tue, Jun 14, 2022 at 4:19 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Jun 13, 2022 at 9:57 PM Brad Larson <brad@pensando.io> wrote: > > > Add support for AMD Pensando Elba SoC which explicitly controls > > byte-lane enables on writes. Add priv_write_l() which is > > enabling ? Changed to enabling > ... > > > + void (*priv_write_l)(struct sdhci_cdns_priv *priv, u32 val, > > priv_writel Changed to priv_writel > > > + void __iomem *reg); > > And perhaps leave it on one line. > > I also would swap parameters, so address goes first followed by value. Which is the reverse of writel() parameter ordering which is value, address. Should I do this? > ... > > > +static inline void sdhci_cdns_priv_writel(struct sdhci_cdns_priv *priv, > > + u32 val, void __iomem *reg) > > +{ > > > + if (unlikely(priv->priv_write_l)) > > First of all, why if (unlikely())-else instead of if (likely())-else? > > > + priv->priv_write_l(priv, val, reg); > > + else > > + writel(val, reg); > > +} It was existing code and never looked at it. This construct looks to be widely used however this goes away with the two patch approach below. $ find . -name \*.c | xargs grep if | grep unlikely | wc 18640 > Instead of branching each time you do I/O, make sure that callback is > always set and call it unconditionally. In this case you don't need to > have this callback, but maybe just a wrapper on `writel()`. As a > result you may split this to two patches in the first of which you > simply introduce a callback and a writel() wrapper which is assigned > unconditionally to all current chips. In the next you add a new chip > support. Next version will separate into two patches as described > ... > > > + u32 m = (reg & 0x3); > > + u32 msk = (0x3 << (m)); > > + > > + spin_lock_irqsave(&priv->wrlock, flags); > > + writel(msk << 3, priv->ctl_addr); > > + writew(val, host->ioaddr + reg); > > + spin_unlock_irqrestore(&priv->wrlock, flags); > > Too many 3:s as magic. Is it GENMASK() or something else? Perhaps it > needs a definition. Definitely, changed this to be understandable by inspection. GENMASK() for word and BIT() for byte makes this more clear. The 3's came from bits [6:3] are the byte-lane enables in the control reg where the lower two bits of the address specify the byte(s) to enable. /* Elba control reg bits [6:3] are byte-lane enables */ #define ELBA_BYTE_ENABLE_MASK(x) ((x) << 3) elba_priv_write_l(...): writel(ELBA_BYTE_ENABLE_MASK(0xf), priv->ctl_addr); writel(val, reg); elba_write_w(...): byte_enables = GENMASK(1, 0) << (reg & 0x3); writel(ELBA_BYTE_ENABLE_MASK(byte_enables), priv->ctl_addr); writew(val, host->ioaddr + reg); > ... > > > + u32 m = (reg & 0x3); > > + u32 msk = (0x1 << (m)); > > + > > + spin_lock_irqsave(&priv->wrlock, flags); > > + writel(msk << 3, priv->ctl_addr); > > + writeb(val, host->ioaddr + reg); > > + spin_unlock_irqrestore(&priv->wrlock, flags); > > Ditto. elba_write_b(...): byte_enables = BIT(0) << (reg & 0x3); writel(ELBA_BYTE_ENABLE_MASK(byte_enables), priv->ctl_addr); writeb(val, host->ioaddr + reg); > ... > > > + writel(0x78, priv->ctl_addr); > > Magic. writel(ELBA_BYTE_ENABLE_MASK(0xf), priv->ctl_addr); > ... > > > +static const struct sdhci_cdns_drv_data sdhci_cdns_drv_data = { > > + .pltfm_data = { > > + .ops = &sdhci_cdns_ops, > > + }, > > +}; > > + > > + > > One blank line is enough. Removed extra blank line > ... > > > + { > > + .compatible = "amd,pensando-elba-sd4hc", > > + .data = &sdhci_elba_drv_data > > Leave a comma here. Added comma Regards, Brad
Hi Rob, On Tue, Jun 14, 2022 at 2:22 PM Rob Herring <robh@kernel.org> wrote: > > On Mon, Jun 13, 2022 at 12:56:45PM -0700, Brad Larson wrote: > > From: Brad Larson <blarson@amd.com> > > > > AMD Pensando Elba ARM 64-bit SoC is integrated with this IP and > > explicitly controls byte-lane enables. > > > > Signed-off-by: Brad Larson <blarson@amd.com> > > --- > > .../devicetree/bindings/mmc/cdns,sdhci.yaml | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml > > index 4207fed62dfe..35bc4cf6f214 100644 > > --- a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml > > +++ b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml > > @@ -13,10 +13,24 @@ maintainers: > > allOf: > > - $ref: mmc-controller.yaml > > > > + - if: > > + properties: > > + compatible: > > + enum: > > + - amd,pensando-elba-sd4hc > > + then: > > + properties: > > + reg: > > + items: > > + - description: Cadence host controller registers > > + - description: Byte-lane control register > > + minItems: 2 > > This doesn't work. The if/then is additional constraints on the main > section which says there is only 1 register region. The main section > needs the above, but with 'minItems: 1'. Then the if/then should be: > > if: > properties: > compatible: > const: amd,pensando-elba-sd4hc > then: > properties: > reg: > minItems: 2 > else: > properties: > reg: > maxItems: 1 > ... The proposed change to current version throws dtbs_check error below: --- a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml +++ b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml @@ -13,19 +13,6 @@ maintainers: allOf: - $ref: mmc-controller.yaml - - if: - properties: - compatible: - enum: - - amd,pensando-elba-sd4hc - then: - properties: - reg: - items: - - description: Cadence host controller registers - - description: Byte-lane control register - minItems: 2 - properties: compatible: items: @@ -36,7 +23,7 @@ properties: - const: cdns,sd4hc reg: - maxItems: 1 + minItems: 1 interrupts: maxItems: 1 @@ -132,6 +119,19 @@ required: - interrupts - clocks +if: + properties: + compatible: + const: amd,pensando-elba-sd4hc +then: + properties: + reg: + minItems: 2 +else: + properties: + reg: + maxItems: 1 + unevaluatedProperties: false results in $ make ARCH=arm64 dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml LINT Documentation/devicetree/bindings CHKDT Documentation/devicetree/bindings/processed-schema.json SCHEMA Documentation/devicetree/bindings/processed-schema.json /home/brad/linux-next/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml: ignoring, error in schema: /home/brad/linux-next/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml: ignoring, error in schema: DTC arch/arm64/boot/dts/amd/elba-asic.dtb CHECK arch/arm64/boot/dts/amd/elba-asic.dtb /home/brad/linux-next/arch/arm64/boot/dts/amd/elba-asic.dtb: mmc@30440000: reg: [[0, 809762816, 0, 65536], [0, 810025028, 0, 4]] is too long From schema: /home/brad/linux-next/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml /home/brad/linux-next/arch/arm64/boot/dts/amd/elba-asic.dtb: mmc@30440000: reg: [[0, 809762816, 0, 65536], [0, 810025028, 0, 4]] is too long From schema: /home/brad/linux-next/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml Below modification to proposed change passes dtbs_check: --- a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml +++ b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml @@ -13,19 +13,6 @@ maintainers: allOf: - $ref: mmc-controller.yaml - - if: - properties: - compatible: - enum: - - amd,pensando-elba-sd4hc - then: - properties: - reg: - items: - - description: Cadence host controller registers - - description: Byte-lane control register - minItems: 2 - properties: compatible: items: @@ -36,7 +23,8 @@ properties: - const: cdns,sd4hc reg: - maxItems: 1 + minItems: 1 + maxItems: 2 interrupts: maxItems: 1 @@ -132,6 +120,15 @@ required: - interrupts - clocks +if: + properties: + compatible: + const: amd,pensando-elba-sd4hc +then: + properties: + reg: + minItems: 2 + unevaluatedProperties: false Regards, Brad
Hi Krzysztof, On Tue, Jun 14, 2022 at 3:44 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 13/06/2022 12:56, Brad Larson wrote: > > From: Brad Larson <blarson@amd.com> > > > > Add AMD Pensando common and Elba SoC specific device nodes > > > > Signed-off-by: Brad Larson <blarson@amd.com> > > Thank you for your patch. There is something to discuss/improve. > > > --- > > arch/arm64/boot/dts/amd/Makefile | 1 + > > arch/arm64/boot/dts/amd/elba-16core.dtsi | 189 +++++++++++++++++ > > arch/arm64/boot/dts/amd/elba-asic-common.dtsi | 103 ++++++++++ > > arch/arm64/boot/dts/amd/elba-asic.dts | 28 +++ > > arch/arm64/boot/dts/amd/elba-flash-parts.dtsi | 106 ++++++++++ > > arch/arm64/boot/dts/amd/elba.dtsi | 191 ++++++++++++++++++ > > 6 files changed, 618 insertions(+) > > create mode 100644 arch/arm64/boot/dts/amd/elba-16core.dtsi > > create mode 100644 arch/arm64/boot/dts/amd/elba-asic-common.dtsi > > create mode 100644 arch/arm64/boot/dts/amd/elba-asic.dts > > create mode 100644 arch/arm64/boot/dts/amd/elba-flash-parts.dtsi > > create mode 100644 arch/arm64/boot/dts/amd/elba.dtsi > > > > diff --git a/arch/arm64/boot/dts/amd/Makefile b/arch/arm64/boot/dts/amd/Makefile > > index 68103a8b0ef5..9bba020fa880 100644 > > --- a/arch/arm64/boot/dts/amd/Makefile > > +++ b/arch/arm64/boot/dts/amd/Makefile > > @@ -1,2 +1,3 @@ > > # SPDX-License-Identifier: GPL-2.0 > > dtb-$(CONFIG_ARCH_SEATTLE) += amd-overdrive-rev-b0.dtb amd-overdrive-rev-b1.dtb > > +dtb-$(CONFIG_ARCH_PENSANDO) += elba-asic.dtb > > Put it in alphabetical order, so not at the end of file. Reversed the order in the Makefile > (...) > > > + > > +&spi0 { > > + num-cs = <4>; > > + cs-gpios = <0>, <0>, <&porta 1 GPIO_ACTIVE_LOW>, > > + <&porta 7 GPIO_ACTIVE_LOW>; > > + status = "okay"; > > + spi@0 { > > Rob's comment about bindings applies here as well, so please fix both. > This has to be sorted out - either it is SPI controller or MFD. > > Rest looks okay for me. Proposed a change after reviewing existing drivers in mfd directory Regards, Brad
Hi Rob, On Tue, Jun 14, 2022 at 2:32 PM Rob Herring <robh@kernel.org> wrote: > > On Mon, Jun 13, 2022 at 12:56:50PM -0700, Brad Larson wrote: > > From: Brad Larson <blarson@amd.com> > > > > Document bindings for AMD Pensando Elba SR Reset Controller > > > > Signed-off-by: Brad Larson <blarson@amd.com> > > --- > > .../reset/amd,pensando-elbasr-reset.yaml | 62 +++++++++++++++++++ > > 1 file changed, 62 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/reset/amd,pensando-elbasr-reset.yaml > > > > diff --git a/Documentation/devicetree/bindings/reset/amd,pensando-elbasr-reset.yaml b/Documentation/devicetree/bindings/reset/amd,pensando-elbasr-reset.yaml > > new file mode 100644 > > index 000000000000..03bb86ebcfd3 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/reset/amd,pensando-elbasr-reset.yaml > > @@ -0,0 +1,62 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/reset/amd,pensando-elbasr-reset.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: AMD Pensando Elba SoC Reset Controller Device Tree Bindings > > + > > +maintainers: > > + - Brad Larson <blarson@amd.com> > > + > > +description: | > > + AMD Pensando Elba SoC reset controller driver which supports a resource > > + controller connected to the Elba SoC over a SPI bus. The Elba reset > > + controller must be defined as a child node of the Elba SPI bus > > + chip-select 0 node. > > + > > + See also: > > + - dt-bindings/reset/amd,pensando-elba-reset.h > > + > > +properties: > > + $nodename: > > + pattern: "^reset-controller@[0-9a-f]+$" > > + > > + compatible: > > + const: amd,pensando-elbasr-reset > > + > > + reg: > > + const: 0 > > + > > + '#reset-cells': > > + const: 1 > > + > > +required: > > + - compatible > > + - reg > > + - '#reset-cells' > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/reset/amd,pensando-elba-reset.h> > > + spi0 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + num-cs = <4>; > > + > > + spi@0 { > > 'spi' is reserved for SPI buses. I thought this was an MFD. Looking at other drivers/mfd files the naming convention could allow the following, how about this? spi@0 { sr_regs@0 { rstc: reset-controller@0 { dw_i2c@1 { lattice_i2c@2 { flash@3 { Regards, Brad
From: Brad Larson <blarson@amd.com> This series enables support for AMD Pensando Elba SoC based platforms. The Elba SoC has the following features: - Sixteen ARM64 A72 cores - Dual DDR 4/5 memory controllers - 32 lanes of PCIe Gen3/4 to the Host - Network interfaces: Dual 200GE, Quad 100GE, 50GE, 25GE, 10GE and also a single 1GE management port. - Storage/crypto offloads and 144 programmable P4 cores. - QSPI and EMMC for SoC storage - Two SPI interfaces for peripheral management - I2C bus for platform management This is a respin based on review inputs 1. Change to AMD Pensando instead of Pensando. 2. No reference to spidev in the device tree. Add multi-function driver pensando-elbasr and sub-device reset-elbasr. 3. Rebase to linux-next tag next-20220609 5.19.0-rc1 4. Redo the email list after rebase and using scripts/get_maintainer.pl Brad Larson (15): dt-bindings: arm: add AMD Pensando boards dt-bindings: mmc: cdns: Add AMD Pensando Elba SoC binding dt-bindings: spi: cdns: Add compatible for AMD Pensando Elba SoC dt-bindings: spi: dw: Add AMD Pensando Elba SoC SPI Controller bindings dt-bindings: mfd: syscon: Add amd,pensando-elba-syscon compatible dt-bindings: mfd: amd,pensando-elbasr: Add AMD Pensando Elba System Resource chip dt-bindings: reset: amd,pensando-elbasr-reset: Add AMD Pensando SR Reset Controller bindings MAINTAINERS: Add entry for AMD PENSANDO arm64: Add config for AMD Pensando SoC platforms arm64: dts: Add AMD Pensando Elba SoC support spi: cadence-quadspi: Add compatible for AMD Pensando Elba SoC spi: dw: Add support for AMD Pensando Elba SoC mmc: sdhci-cadence: Add AMD Pensando Elba SoC support mfd: pensando-elbasr: Add AMD Pensando Elba System Resource chip reset: elbasr: Add AMD Pensando Elba SR Reset Controller .../devicetree/bindings/arm/amd,pensando.yaml | 26 + .../bindings/mfd/amd,pensando-elbasr.yaml | 93 ++ .../devicetree/bindings/mfd/syscon.yaml | 1 + .../devicetree/bindings/mmc/cdns,sdhci.yaml | 14 + .../reset/amd,pensando-elbasr-reset.yaml | 62 ++ .../bindings/spi/cdns,qspi-nor.yaml | 12 + .../bindings/spi/snps,dw-apb-ssi.yaml | 2 + MAINTAINERS | 10 + arch/arm64/Kconfig.platforms | 12 + arch/arm64/boot/dts/amd/Makefile | 1 + arch/arm64/boot/dts/amd/elba-16core.dtsi | 189 ++++ arch/arm64/boot/dts/amd/elba-asic-common.dtsi | 103 +++ arch/arm64/boot/dts/amd/elba-asic.dts | 28 + arch/arm64/boot/dts/amd/elba-flash-parts.dtsi | 106 +++ arch/arm64/boot/dts/amd/elba.dtsi | 191 ++++ drivers/mfd/Kconfig | 14 + drivers/mfd/Makefile | 1 + drivers/mfd/pensando-elbasr.c | 862 ++++++++++++++++++ drivers/mmc/host/Kconfig | 1 + drivers/mmc/host/sdhci-cadence.c | 179 +++- drivers/reset/Kconfig | 9 + drivers/reset/Makefile | 1 + drivers/reset/reset-elbasr.c | 94 ++ drivers/spi/spi-cadence-quadspi.c | 19 + drivers/spi/spi-dw-mmio.c | 66 ++ .../reset/amd,pensando-elba-reset.h | 11 + include/linux/mfd/pensando-elbasr.h | 78 ++ 27 files changed, 2171 insertions(+), 14 deletions(-) create mode 100644 Documentation/devicetree/bindings/arm/amd,pensando.yaml create mode 100644 Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml create mode 100644 Documentation/devicetree/bindings/reset/amd,pensando-elbasr-reset.yaml create mode 100644 arch/arm64/boot/dts/amd/elba-16core.dtsi create mode 100644 arch/arm64/boot/dts/amd/elba-asic-common.dtsi create mode 100644 arch/arm64/boot/dts/amd/elba-asic.dts create mode 100644 arch/arm64/boot/dts/amd/elba-flash-parts.dtsi create mode 100644 arch/arm64/boot/dts/amd/elba.dtsi create mode 100644 drivers/mfd/pensando-elbasr.c create mode 100644 drivers/reset/reset-elbasr.c create mode 100644 include/dt-bindings/reset/amd,pensando-elba-reset.h create mode 100644 include/linux/mfd/pensando-elbasr.h