[PATCH v5 00/15] Support AMD Pensando Elba SoC

[PATCH v5 00/15] Support AMD Pensando Elba SoC

[Brad Larson]

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

-- 
2.17.1

[PATCH v5 01/15] dt-bindings: arm: add AMD Pensando boards

[Brad Larson]

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

diff --git a/Documentation/devicetree/bindings/arm/amd,pensando.yaml b/Documentation/devicetree/bindings/arm/amd,pensando.yaml
new file mode 100644
index 000000000000..9f3dea681d24
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/amd,pensando.yaml
@@ -0,0 +1,26 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/amd,pensando.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AMD Pensando SoC Platforms Device Tree Bindings
+
+maintainers:
+  - Brad Larson <blarson@amd.com>
+
+properties:
+  $nodename:
+    const: "/"
+  compatible:
+    oneOf:
+
+      - description: Boards with Pensando Elba SoC
+        items:
+          - enum:
+              - amd,pensando-elba-ortano
+          - const: amd,pensando-elba
+
+additionalProperties: true
+
+...
-- 
2.17.1

Re: [PATCH v5 01/15] dt-bindings: arm: add AMD Pensando boards

[Rob Herring]

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>

Re: [PATCH v5 01/15] dt-bindings: arm: add AMD Pensando boards

[Krzysztof Kozlowski]

On 13/06/2022 21:56, 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>

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v5 02/15] dt-bindings: mmc: cdns: Add AMD Pensando Elba SoC binding

[Brad Larson]

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
+
 properties:
   compatible:
     items:
       - enum:
+          - amd,pensando-elba-sd4hc
           - microchip,mpfs-sd4hc
           - socionext,uniphier-sd4hc
       - const: cdns,sd4hc
-- 
2.17.1

Re: [PATCH v5 02/15] dt-bindings: mmc: cdns: Add AMD Pensando Elba SoC binding

[Rob Herring]

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
> 
> 

Re: [PATCH v5 02/15] dt-bindings: mmc: cdns: Add AMD Pensando Elba SoC binding

[Brad Larson]

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

Re: [PATCH v5 02/15] dt-bindings: mmc: cdns: Add AMD Pensando Elba SoC binding

[Krzysztof Kozlowski]

On 13/06/2022 21:56, 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
> +

Except Rob's comment, the entire section now should be moved to bottom -
just before unevaluated/additionalProperties:false


Best regards,
Krzysztof

Re: [PATCH v5 02/15] dt-bindings: mmc: cdns: Add AMD Pensando Elba SoC binding

[Brad Larson]

Hi Krzysztof,

On Mon, Jun 20, 2022 at 5:50 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 13/06/2022 21:56, Brad Larson wrote:
> ...
> > --- 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
> > +
>
> Except Rob's comment, the entire section now should be moved to bottom -
> just before unevaluated/additionalProperties:false

Yes, moved it to just before unevaluatedProperties

Regards,
Brad

[PATCH v5 03/15] dt-bindings: spi: cdns: Add compatible for AMD Pensando Elba SoC

[Brad Larson]

From: Brad Larson <blarson@amd.com>

Document the cadence qspi controller compatible for AMD Pensando
Elba SoC boards.  The Elba qspi fifo size is 1024.

Signed-off-by: Brad Larson <blarson@amd.com>
---
 .../devicetree/bindings/spi/cdns,qspi-nor.yaml       | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
index 0a537fa3a641..9268a4882bfd 100644
--- a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
+++ b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
@@ -20,11 +20,23 @@ allOf:
       required:
         - power-domains
 
+  - if:
+      properties:
+        compatible:
+          enum:
+            - amd,pensando-elba-qspi
+    then:
+      properties:
+        cdns,fifo-depth:
+          enum: [ 128, 256, 1024 ]
+          default: 1024
+
 properties:
   compatible:
     oneOf:
       - items:
           - enum:
+              - amd,pensando-elba-qspi
               - ti,k2g-qspi
               - ti,am654-ospi
               - intel,lgm-qspi
-- 
2.17.1

Re: [PATCH v5 03/15] dt-bindings: spi: cdns: Add compatible for AMD Pensando Elba SoC

[Rob Herring]

On Mon, 13 Jun 2022 12:56:46 -0700, Brad Larson wrote:
> From: Brad Larson <blarson@amd.com>
> 
> Document the cadence qspi controller compatible for AMD Pensando
> Elba SoC boards.  The Elba qspi fifo size is 1024.
> 
> Signed-off-by: Brad Larson <blarson@amd.com>
> ---
>  .../devicetree/bindings/spi/cdns,qspi-nor.yaml       | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v5 03/15] dt-bindings: spi: cdns: Add compatible for AMD Pensando Elba SoC

[Krzysztof Kozlowski]

On 13/06/2022 21:56, Brad Larson wrote:
> From: Brad Larson <blarson@amd.com>
> 
> Document the cadence qspi controller compatible for AMD Pensando
> Elba SoC boards.  The Elba qspi fifo size is 1024.
> 
> Signed-off-by: Brad Larson <blarson@amd.com>
> ---
>  .../devicetree/bindings/spi/cdns,qspi-nor.yaml       | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v5 04/15] dt-bindings: spi: dw: Add AMD Pensando Elba SoC SPI Controller bindings

[Brad Larson]

From: Brad Larson <blarson@amd.com>

The AMD Pensando Elba SoC has integrated the DW APB SPI Controller

Signed-off-by: Brad Larson <blarson@amd.com>
---
 Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
index e25d44c218f2..2a55b947cffc 100644
--- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
+++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
@@ -73,6 +73,8 @@ properties:
               - renesas,r9a06g032-spi # RZ/N1D
               - renesas,r9a06g033-spi # RZ/N1S
           - const: renesas,rzn1-spi   # RZ/N1
+      - description: AMD Pensando Elba SoC SPI Controller
+        const: amd,pensando-elba-spi
 
   reg:
     minItems: 1
-- 
2.17.1

Re: [PATCH v5 04/15] dt-bindings: spi: dw: Add AMD Pensando Elba SoC SPI Controller bindings

[Rob Herring]

On Mon, 13 Jun 2022 12:56:47 -0700, Brad Larson wrote:
> From: Brad Larson <blarson@amd.com>
> 
> The AMD Pensando Elba SoC has integrated the DW APB SPI Controller
> 
> Signed-off-by: Brad Larson <blarson@amd.com>
> ---
>  Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v5 04/15] dt-bindings: spi: dw: Add AMD Pensando Elba SoC SPI Controller bindings

[Serge Semin]

On Mon, Jun 13, 2022 at 12:56:47PM -0700, Brad Larson wrote:
> From: Brad Larson <blarson@amd.com>
> 
> The AMD Pensando Elba SoC has integrated the DW APB SPI Controller
> 
> Signed-off-by: Brad Larson <blarson@amd.com>
> ---
>  Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> index e25d44c218f2..2a55b947cffc 100644
> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> @@ -73,6 +73,8 @@ properties:
>                - renesas,r9a06g032-spi # RZ/N1D
>                - renesas,r9a06g033-spi # RZ/N1S
>            - const: renesas,rzn1-spi   # RZ/N1

> +      - description: AMD Pensando Elba SoC SPI Controller
> +        const: amd,pensando-elba-spi

Not enough. The driver requires to have a phandle reference to the
Pensando System Controller. So the property like
"amd,pensando-elba-syscon" is also needed to be added to the DT schema
otherwise should the dt-schema tool correctly handle the
"unevaluatedProperties: false" setting (Rob says it isn't fully
supported at the moment), the dtbs_check procedure will fail on your
dts evaluation.

-Sergey

>  
>    reg:
>      minItems: 1
> -- 
> 2.17.1
> 

Re: [PATCH v5 04/15] dt-bindings: spi: dw: Add AMD Pensando Elba SoC SPI Controller bindings

[Krzysztof Kozlowski]

On 20/06/2022 21:30, Serge Semin wrote:
> On Mon, Jun 13, 2022 at 12:56:47PM -0700, Brad Larson wrote:
>> From: Brad Larson <blarson@amd.com>
>>
>> The AMD Pensando Elba SoC has integrated the DW APB SPI Controller
>>
>> Signed-off-by: Brad Larson <blarson@amd.com>
>> ---
>>  Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>> index e25d44c218f2..2a55b947cffc 100644
>> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>> @@ -73,6 +73,8 @@ properties:
>>                - renesas,r9a06g032-spi # RZ/N1D
>>                - renesas,r9a06g033-spi # RZ/N1S
>>            - const: renesas,rzn1-spi   # RZ/N1
> 
>> +      - description: AMD Pensando Elba SoC SPI Controller
>> +        const: amd,pensando-elba-spi
> 
> Not enough. The driver requires to have a phandle reference to the
> Pensando System Controller. So the property like
> "amd,pensando-elba-syscon" is also needed to be added to the DT schema
> otherwise should the dt-schema tool correctly handle the
> "unevaluatedProperties: false" setting (Rob says it isn't fully
> supported at the moment), the dtbs_check procedure will fail on your
> dts evaluation.

The property was here before, now removed, so I assume it was also
removed from the driver and DTS. Isn't that the case?

Otherwise this is incomplete binding...

Best regards,
Krzysztof

Re: [PATCH v5 04/15] dt-bindings: spi: dw: Add AMD Pensando Elba SoC SPI Controller bindings

[Serge Semin]

On Mon, Jun 20, 2022 at 09:46:25PM +0200, Krzysztof Kozlowski wrote:
> On 20/06/2022 21:30, Serge Semin wrote:
> > On Mon, Jun 13, 2022 at 12:56:47PM -0700, Brad Larson wrote:
> >> From: Brad Larson <blarson@amd.com>
> >>
> >> The AMD Pensando Elba SoC has integrated the DW APB SPI Controller
> >>
> >> Signed-off-by: Brad Larson <blarson@amd.com>
> >> ---
> >>  Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> >> index e25d44c218f2..2a55b947cffc 100644
> >> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> >> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> >> @@ -73,6 +73,8 @@ properties:
> >>                - renesas,r9a06g032-spi # RZ/N1D
> >>                - renesas,r9a06g033-spi # RZ/N1S
> >>            - const: renesas,rzn1-spi   # RZ/N1
> > 
> >> +      - description: AMD Pensando Elba SoC SPI Controller
> >> +        const: amd,pensando-elba-spi
> > 
> > Not enough. The driver requires to have a phandle reference to the
> > Pensando System Controller. So the property like
> > "amd,pensando-elba-syscon" is also needed to be added to the DT schema
> > otherwise should the dt-schema tool correctly handle the
> > "unevaluatedProperties: false" setting (Rob says it isn't fully
> > supported at the moment), the dtbs_check procedure will fail on your
> > dts evaluation.
> 

> The property was here before, now removed, so I assume it was also
> removed from the driver and DTS. Isn't that the case?

Ah, the property has been indeed removed. The driver now searches for
the system controller by the next compatible string:
"amd,pensando-elba-syscon" using the
syscon_regmap_lookup_by_compatible() method. My mistake. Sorry for the
noise.

* Though personally I'd prefer to have a property with the phandle
reference in order to signify the connection between the system controller
and the SPI-controller. Otherwise the implicit DT bindings like having
the "amd,pensando-elba-syscon"-compatible syscon gets to be
hidden behind the DT scene. But seeing we have already got the Microsemi
platform with such semantic, I can't insist on fixing this.

-Sergey

> 
> Otherwise this is incomplete binding...
> 
> Best regards,
> Krzysztof

Re: [PATCH v5 04/15] dt-bindings: spi: dw: Add AMD Pensando Elba SoC SPI Controller bindings

[Krzysztof Kozlowski]

On 20/06/2022 22:04, Serge Semin wrote:
> On Mon, Jun 20, 2022 at 09:46:25PM +0200, Krzysztof Kozlowski wrote:
>> On 20/06/2022 21:30, Serge Semin wrote:
>>> On Mon, Jun 13, 2022 at 12:56:47PM -0700, Brad Larson wrote:
>>>> From: Brad Larson <blarson@amd.com>
>>>>
>>>> The AMD Pensando Elba SoC has integrated the DW APB SPI Controller
>>>>
>>>> Signed-off-by: Brad Larson <blarson@amd.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>>>> index e25d44c218f2..2a55b947cffc 100644
>>>> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>>>> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>>>> @@ -73,6 +73,8 @@ properties:
>>>>                - renesas,r9a06g032-spi # RZ/N1D
>>>>                - renesas,r9a06g033-spi # RZ/N1S
>>>>            - const: renesas,rzn1-spi   # RZ/N1
>>>
>>>> +      - description: AMD Pensando Elba SoC SPI Controller
>>>> +        const: amd,pensando-elba-spi
>>>
>>> Not enough. The driver requires to have a phandle reference to the
>>> Pensando System Controller. So the property like
>>> "amd,pensando-elba-syscon" is also needed to be added to the DT schema
>>> otherwise should the dt-schema tool correctly handle the
>>> "unevaluatedProperties: false" setting (Rob says it isn't fully
>>> supported at the moment), the dtbs_check procedure will fail on your
>>> dts evaluation.
>>
> 
>> The property was here before, now removed, so I assume it was also
>> removed from the driver and DTS. Isn't that the case?
> 
> Ah, the property has been indeed removed. The driver now searches for
> the system controller by the next compatible string:
> "amd,pensando-elba-syscon" using the
> syscon_regmap_lookup_by_compatible() method. My mistake. Sorry for the
> noise.
> 
> * Though personally I'd prefer to have a property with the phandle
> reference in order to signify the connection between the system controller
> and the SPI-controller. Otherwise the implicit DT bindings like having
> the "amd,pensando-elba-syscon"-compatible syscon gets to be
> hidden behind the DT scene. But seeing we have already got the Microsemi
> platform with such semantic, I can't insist on fixing this.

I agree entirely, this should be explicit syscon-type property. Looking
up for compatibles:
 - creates hidden (not expressed via bindings) dependency between nodes,
 - is not portable and several people struggled with it later and needed
backward-compatible code (many examples, let's just give recent one: [1])


[1]
https://lore.kernel.org/all/20220619151225.209029-10-tmaimon77@gmail.com/


Best regards,
Krzysztof

Re: [PATCH v5 04/15] dt-bindings: spi: dw: Add AMD Pensando Elba SoC SPI Controller bindings

[Serge Semin]

On Tue, Jun 21, 2022 at 09:00:36AM +0200, Krzysztof Kozlowski wrote:
> On 20/06/2022 22:04, Serge Semin wrote:
> > On Mon, Jun 20, 2022 at 09:46:25PM +0200, Krzysztof Kozlowski wrote:
> >> On 20/06/2022 21:30, Serge Semin wrote:
> >>> On Mon, Jun 13, 2022 at 12:56:47PM -0700, Brad Larson wrote:
> >>>> From: Brad Larson <blarson@amd.com>
> >>>>
> >>>> The AMD Pensando Elba SoC has integrated the DW APB SPI Controller
> >>>>
> >>>> Signed-off-by: Brad Larson <blarson@amd.com>
> >>>> ---
> >>>>  Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml | 2 ++
> >>>>  1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> >>>> index e25d44c218f2..2a55b947cffc 100644
> >>>> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> >>>> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> >>>> @@ -73,6 +73,8 @@ properties:
> >>>>                - renesas,r9a06g032-spi # RZ/N1D
> >>>>                - renesas,r9a06g033-spi # RZ/N1S
> >>>>            - const: renesas,rzn1-spi   # RZ/N1
> >>>
> >>>> +      - description: AMD Pensando Elba SoC SPI Controller
> >>>> +        const: amd,pensando-elba-spi
> >>>
> >>> Not enough. The driver requires to have a phandle reference to the
> >>> Pensando System Controller. So the property like
> >>> "amd,pensando-elba-syscon" is also needed to be added to the DT schema
> >>> otherwise should the dt-schema tool correctly handle the
> >>> "unevaluatedProperties: false" setting (Rob says it isn't fully
> >>> supported at the moment), the dtbs_check procedure will fail on your
> >>> dts evaluation.
> >>
> > 
> >> The property was here before, now removed, so I assume it was also
> >> removed from the driver and DTS. Isn't that the case?
> > 
> > Ah, the property has been indeed removed. The driver now searches for
> > the system controller by the next compatible string:
> > "amd,pensando-elba-syscon" using the
> > syscon_regmap_lookup_by_compatible() method. My mistake. Sorry for the
> > noise.
> > 

> > * Though personally I'd prefer to have a property with the phandle
> > reference in order to signify the connection between the system controller
> > and the SPI-controller. Otherwise the implicit DT bindings like having
> > the "amd,pensando-elba-syscon"-compatible syscon gets to be
> > hidden behind the DT scene. But seeing we have already got the Microsemi
> > platform with such semantic, I can't insist on fixing this.
> 
> I agree entirely, this should be explicit syscon-type property. Looking
> up for compatibles:
>  - creates hidden (not expressed via bindings) dependency between nodes,
>  - is not portable and several people struggled with it later and needed
> backward-compatible code (many examples, let's just give recent one: [1])
> 
> 
> [1]
> https://lore.kernel.org/all/20220619151225.209029-10-tmaimon77@gmail.com/

Seems even more reasonable now. Thanks for providing a bright example
justifying the property-based approach.

@Brad, could you get back the property with a phandle to the syscon
DT-node? (No need in adding the CS CSR address as the phandle argument,
just a phandle.)

-Sergey

> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v5 04/15] dt-bindings: spi: dw: Add AMD Pensando Elba SoC SPI Controller bindings

[Brad Larson]

Hi Sergey,

On Tue, Jun 21, 2022 at 3:12 AM Serge Semin <fancer.lancer@gmail.com> wrote:
>
> On Tue, Jun 21, 2022 at 09:00:36AM +0200, Krzysztof Kozlowski wrote:
> > On 20/06/2022 22:04, Serge Semin wrote:
> > > On Mon, Jun 20, 2022 at 09:46:25PM +0200, Krzysztof Kozlowski wrote:
> > >> On 20/06/2022 21:30, Serge Semin wrote:
> > >>> On Mon, Jun 13, 2022 at 12:56:47PM -0700, Brad Larson wrote:
> > >>>> From: Brad Larson <blarson@amd.com>
> > >>>>
> > >>>> The AMD Pensando Elba SoC has integrated the DW APB SPI Controller
> > >>>>
> > >>>> Signed-off-by: Brad Larson <blarson@amd.com>
> > >>>> ---
> > >>>>  Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml | 2 ++
> > >>>>  1 file changed, 2 insertions(+)
> > >>>>
> > >>>> diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> > >>>> index e25d44c218f2..2a55b947cffc 100644
> > >>>> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> > >>>> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> > >>>> @@ -73,6 +73,8 @@ properties:
> > >>>>                - renesas,r9a06g032-spi # RZ/N1D
> > >>>>                - renesas,r9a06g033-spi # RZ/N1S
> > >>>>            - const: renesas,rzn1-spi   # RZ/N1
> > >>>
> > >>>> +      - description: AMD Pensando Elba SoC SPI Controller
> > >>>> +        const: amd,pensando-elba-spi
> > >>>
> > >>> Not enough. The driver requires to have a phandle reference to the
> > >>> Pensando System Controller. So the property like
> > >>> "amd,pensando-elba-syscon" is also needed to be added to the DT schema
> > >>> otherwise should the dt-schema tool correctly handle the
> > >>> "unevaluatedProperties: false" setting (Rob says it isn't fully
> > >>> supported at the moment), the dtbs_check procedure will fail on your
> > >>> dts evaluation.
> > >>
> > >
> > >> The property was here before, now removed, so I assume it was also
> > >> removed from the driver and DTS. Isn't that the case?
> > >
> > > Ah, the property has been indeed removed. The driver now searches for
> > > the system controller by the next compatible string:
> > > "amd,pensando-elba-syscon" using the
> > > syscon_regmap_lookup_by_compatible() method. My mistake. Sorry for the
> > > noise.
> > >
>
> > > * Though personally I'd prefer to have a property with the phandle
> > > reference in order to signify the connection between the system controller
> > > and the SPI-controller. Otherwise the implicit DT bindings like having
> > > the "amd,pensando-elba-syscon"-compatible syscon gets to be
> > > hidden behind the DT scene. But seeing we have already got the Microsemi
> > > platform with such semantic, I can't insist on fixing this.
> >
> > I agree entirely, this should be explicit syscon-type property. Looking
> > up for compatibles:
> >  - creates hidden (not expressed via bindings) dependency between nodes,
> >  - is not portable and several people struggled with it later and needed
> > backward-compatible code (many examples, let's just give recent one: [1])
> >
> >
> > [1]
> > https://lore.kernel.org/all/20220619151225.209029-10-tmaimon77@gmail.com/
>
> Seems even more reasonable now. Thanks for providing a bright example
> justifying the property-based approach.
>
> @Brad, could you get back the property with a phandle to the syscon
> DT-node? (No need in adding the CS CSR address as the phandle argument,
> just a phandle.)

Replying to the sequence of review inputs.  The below change to
version 5 brings back the phandle without the earlier argument that
complicated the binding (was the motivation for moving in this
direction in earlier version).  Passes dtbs_check.

--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -285,12 +285,17 @@ static void dw_spi_elba_set_cs(struct spi_device
*spi, bool enable)
 static int dw_spi_elba_init(struct platform_device *pdev,
                            struct dw_spi_mmio *dwsmmio)
 {
+       struct device_node *np = pdev->dev.of_node;
+       struct device_node *node;
        struct dw_spi_elba *dwselba;
        struct regmap *regmap;

-       regmap = syscon_regmap_lookup_by_compatible("amd,pensando-elba-syscon");
-       if (IS_ERR(regmap))
-               return PTR_ERR(regmap);
+       node = of_parse_phandle(np, "pensando,elba-syscon-spics", 0);
+       if (node) {
+               regmap = syscon_node_to_regmap(node);
+               if (IS_ERR(regmap))
+                       return PTR_ERR(regmap);
+       }

diff --git a/arch/arm64/boot/dts/amd/elba.dtsi
b/arch/arm64/boot/dts/amd/elba.dtsi
index 9739641261c3..ec48be4cfe48 100644
--- a/arch/arm64/boot/dts/amd/elba.dtsi
+++ b/arch/arm64/boot/dts/amd/elba.dtsi
@@ -98,6 +98,7 @@
                        reg = <0x0 0x2800 0x0 0x100>;
                        #address-cells = <1>;
                        #size-cells = <0>;
+                       pensando,elba-syscon-spics = <&syscon>;
                        clocks = <&ahb_clk>;
                        interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
                        num-cs = <2>;

--- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
+++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
@@ -37,6 +37,15 @@ allOf:
     else:
       required:
         - interrupts
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - amd,pensando-elba-spi
+    then:
+      required:
+        - pensando,elba-syscon-spics

 properties:
   compatible:

Regards,
Brad

Re: [PATCH v5 04/15] dt-bindings: spi: dw: Add AMD Pensando Elba SoC SPI Controller bindings

[Serge Semin]

On Sun, Jul 03, 2022 at 04:58:09PM -0700, Brad Larson wrote:
> Hi Sergey,
> 
> On Tue, Jun 21, 2022 at 3:12 AM Serge Semin <fancer.lancer@gmail.com> wrote:
> >
> > On Tue, Jun 21, 2022 at 09:00:36AM +0200, Krzysztof Kozlowski wrote:
> > > On 20/06/2022 22:04, Serge Semin wrote:
> > > > On Mon, Jun 20, 2022 at 09:46:25PM +0200, Krzysztof Kozlowski wrote:
> > > >> On 20/06/2022 21:30, Serge Semin wrote:
> > > >>> On Mon, Jun 13, 2022 at 12:56:47PM -0700, Brad Larson wrote:
> > > >>>> From: Brad Larson <blarson@amd.com>
> > > >>>>
> > > >>>> The AMD Pensando Elba SoC has integrated the DW APB SPI Controller
> > > >>>>
> > > >>>> Signed-off-by: Brad Larson <blarson@amd.com>
> > > >>>> ---
> > > >>>>  Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml | 2 ++
> > > >>>>  1 file changed, 2 insertions(+)
> > > >>>>
> > > >>>> diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> > > >>>> index e25d44c218f2..2a55b947cffc 100644
> > > >>>> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> > > >>>> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> > > >>>> @@ -73,6 +73,8 @@ properties:
> > > >>>>                - renesas,r9a06g032-spi # RZ/N1D
> > > >>>>                - renesas,r9a06g033-spi # RZ/N1S
> > > >>>>            - const: renesas,rzn1-spi   # RZ/N1
> > > >>>
> > > >>>> +      - description: AMD Pensando Elba SoC SPI Controller
> > > >>>> +        const: amd,pensando-elba-spi
> > > >>>
> > > >>> Not enough. The driver requires to have a phandle reference to the
> > > >>> Pensando System Controller. So the property like
> > > >>> "amd,pensando-elba-syscon" is also needed to be added to the DT schema
> > > >>> otherwise should the dt-schema tool correctly handle the
> > > >>> "unevaluatedProperties: false" setting (Rob says it isn't fully
> > > >>> supported at the moment), the dtbs_check procedure will fail on your
> > > >>> dts evaluation.
> > > >>
> > > >
> > > >> The property was here before, now removed, so I assume it was also
> > > >> removed from the driver and DTS. Isn't that the case?
> > > >
> > > > Ah, the property has been indeed removed. The driver now searches for
> > > > the system controller by the next compatible string:
> > > > "amd,pensando-elba-syscon" using the
> > > > syscon_regmap_lookup_by_compatible() method. My mistake. Sorry for the
> > > > noise.
> > > >
> >
> > > > * Though personally I'd prefer to have a property with the phandle
> > > > reference in order to signify the connection between the system controller
> > > > and the SPI-controller. Otherwise the implicit DT bindings like having
> > > > the "amd,pensando-elba-syscon"-compatible syscon gets to be
> > > > hidden behind the DT scene. But seeing we have already got the Microsemi
> > > > platform with such semantic, I can't insist on fixing this.
> > >
> > > I agree entirely, this should be explicit syscon-type property. Looking
> > > up for compatibles:
> > >  - creates hidden (not expressed via bindings) dependency between nodes,
> > >  - is not portable and several people struggled with it later and needed
> > > backward-compatible code (many examples, let's just give recent one: [1])
> > >
> > >
> > > [1]
> > > https://lore.kernel.org/all/20220619151225.209029-10-tmaimon77@gmail.com/
> >
> > Seems even more reasonable now. Thanks for providing a bright example
> > justifying the property-based approach.
> >
> > @Brad, could you get back the property with a phandle to the syscon
> > DT-node? (No need in adding the CS CSR address as the phandle argument,
> > just a phandle.)
> 
> Replying to the sequence of review inputs.  The below change to
> version 5 brings back the phandle without the earlier argument that
> complicated the binding (was the motivation for moving in this
> direction in earlier version).  Passes dtbs_check.
> 
> --- a/drivers/spi/spi-dw-mmio.c
> +++ b/drivers/spi/spi-dw-mmio.c
> @@ -285,12 +285,17 @@ static void dw_spi_elba_set_cs(struct spi_device
> *spi, bool enable)
>  static int dw_spi_elba_init(struct platform_device *pdev,
>                             struct dw_spi_mmio *dwsmmio)
>  {
> +       struct device_node *np = pdev->dev.of_node;
> +       struct device_node *node;
>         struct dw_spi_elba *dwselba;
>         struct regmap *regmap;
> 
> -       regmap = syscon_regmap_lookup_by_compatible("amd,pensando-elba-syscon");
> -       if (IS_ERR(regmap))
> -               return PTR_ERR(regmap);

> +       node = of_parse_phandle(np, "pensando,elba-syscon-spics", 0);

1. Property name must be structured with the permitted vendor-prefix. In
your case it's "amd," not "pensando,".
2. Having 'spics' in the property name doesn't make much sense since
the property is supposed to contain a phandle reference to the whole
syscon node.

So to speak the property with name like 'amd,pensando-elba-syscon'
seems more appropriate here.

> +       if (node) {
> +               regmap = syscon_node_to_regmap(node);
> +               if (IS_ERR(regmap))
> +                       return PTR_ERR(regmap);
> +       }
> 
> diff --git a/arch/arm64/boot/dts/amd/elba.dtsi
> b/arch/arm64/boot/dts/amd/elba.dtsi
> index 9739641261c3..ec48be4cfe48 100644
> --- a/arch/arm64/boot/dts/amd/elba.dtsi
> +++ b/arch/arm64/boot/dts/amd/elba.dtsi
> @@ -98,6 +98,7 @@
>                         reg = <0x0 0x2800 0x0 0x100>;
>                         #address-cells = <1>;
>                         #size-cells = <0>;

> +                       pensando,elba-syscon-spics = <&syscon>;
  +                       amd,pensando-elba-syscon = <&syscon>;

ditto

>                         clocks = <&ahb_clk>;
>                         interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
>                         num-cs = <2>;
> 
> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> @@ -37,6 +37,15 @@ allOf:
>      else:
>        required:
>          - interrupts
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - amd,pensando-elba-spi
> +    then:
  +      properties:
  +        amd,pensando-elba-syscon
  +          $ref: /schemas/types.yaml#/definitions/phandle
  +          description: AMD Pensando Elba SoC system controller
> +      required:
> +        - amd,pensando-elba-syscon

Please make sure the DT-bindings check passes correctly.

-Sergey

> 
>  properties:
>    compatible:
> 
> Regards,
> Brad

[PATCH v5 05/15] dt-bindings: mfd: syscon: Add amd,pensando-elba-syscon compatible

[Brad Larson]

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(+)

diff --git a/Documentation/devicetree/bindings/mfd/syscon.yaml b/Documentation/devicetree/bindings/mfd/syscon.yaml
index fb784045013f..2267f8828e9e 100644
--- a/Documentation/devicetree/bindings/mfd/syscon.yaml
+++ b/Documentation/devicetree/bindings/mfd/syscon.yaml
@@ -38,6 +38,7 @@ properties:
               - allwinner,sun8i-h3-system-controller
               - allwinner,sun8i-v3s-system-controller
               - allwinner,sun50i-a64-system-controller
+              - amd,pensando-elba-syscon
               - brcm,cru-clkset
               - freecom,fsg-cs2-system-controller
               - hisilicon,dsa-subctrl
-- 
2.17.1

Re: [PATCH v5 05/15] dt-bindings: mfd: syscon: Add amd,pensando-elba-syscon compatible

[Rob Herring]

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>

[PATCH v5 06/15] dt-bindings: mfd: amd,pensando-elbasr: Add AMD Pensando Elba System Resource chip

[Brad Larson]

From: Brad Larson <blarson@amd.com>

Add support for the AMD Pensando Elba SoC System Resource chip
using the SPI interface.  The Elba SR is a Multi-function Device
supporting device register access using CS0, smbus interface for
FRU and board peripherals using CS1, dual Lattice I2C masters for
transceiver management using CS2, and CS3 for flash access.

Signed-off-by: Brad Larson <blarson@amd.com>
---
 .../bindings/mfd/amd,pensando-elbasr.yaml     | 93 +++++++++++++++++++
 1 file changed, 93 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml

diff --git a/Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml b/Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml
new file mode 100644
index 000000000000..13356800b1cf
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml
@@ -0,0 +1,93 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/amd,pensando-elbasr.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AMD Pensando Elba SoC Resource Controller bindings
+
+description: |
+  AMD Pensando Elba SoC Resource Controller bindings attached to a SPI bus.
+
+maintainers:
+  - Brad Larson <blarson@amd.com>
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - amd,pensando-elbasr
+      - const: simple-mfd
+
+  spi-max-frequency:
+    description: Maximum SPI frequency of the device in Hz.
+
+  reg:
+    maxItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - spi-max-frequency
+
+patternProperties:
+  '^reset-controller@[a-f0-9]+$':
+    $ref: ../reset/amd,pensando-elbasr-reset.yaml
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/reset/amd,pensando-elba-reset.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    spi0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        num-cs = <4>;
+
+        spi@0 {
+          compatible = "amd,pensando-elbasr", "simple-mfd";
+          reg = <0>;
+          #address-cells = <1>;
+          #size-cells = <0>;
+          spi-max-frequency = <12000000>;
+
+          rstc: reset-controller@0 {
+            compatible = "amd,pensando-elbasr-reset";
+            reg = <0>;
+            #reset-cells = <1>;
+          };
+        };
+
+        spi@1 {
+          compatible = "amd,pensando-elbasr", "simple-mfd";
+          reg = <1>;
+          spi-max-frequency = <12000000>;
+        };
+
+        spi@2 {
+          compatible = "amd,pensando-elbasr", "simple-mfd";
+          reg = <2>;
+          spi-max-frequency = <12000000>;
+          interrupt-parent = <&porta>;
+          interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
+        };
+
+        spi@3 {
+          compatible = "amd,pensando-elbasr", "simple-mfd";
+          reg = <3>;
+          spi-max-frequency = <12000000>;
+        };
+    };
+
+...
-- 
2.17.1

Re: [PATCH v5 06/15] dt-bindings: mfd: amd,pensando-elbasr: Add AMD Pensando Elba System Resource chip

[Rob Herring]

On Mon, Jun 13, 2022 at 12:56:49PM -0700, Brad Larson wrote:
> From: Brad Larson <blarson@amd.com>
> 
> Add support for the AMD Pensando Elba SoC System Resource chip
> using the SPI interface.  The Elba SR is a Multi-function Device
> supporting device register access using CS0, smbus interface for
> FRU and board peripherals using CS1, dual Lattice I2C masters for
> transceiver management using CS2, and CS3 for flash access.
> 
> Signed-off-by: Brad Larson <blarson@amd.com>
> ---
>  .../bindings/mfd/amd,pensando-elbasr.yaml     | 93 +++++++++++++++++++
>  1 file changed, 93 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml b/Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml
> new file mode 100644
> index 000000000000..13356800b1cf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml
> @@ -0,0 +1,93 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/amd,pensando-elbasr.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: AMD Pensando Elba SoC Resource Controller bindings
> +
> +description: |
> +  AMD Pensando Elba SoC Resource Controller bindings attached to a SPI bus.
> +
> +maintainers:
> +  - Brad Larson <blarson@amd.com>
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - amd,pensando-elbasr
> +      - const: simple-mfd
> +
> +  spi-max-frequency:
> +    description: Maximum SPI frequency of the device in Hz.
> +
> +  reg:
> +    maxItems: 1
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - spi-max-frequency
> +
> +patternProperties:
> +  '^reset-controller@[a-f0-9]+$':
> +    $ref: ../reset/amd,pensando-elbasr-reset.yaml

/schemas/reset/...

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/reset/amd,pensando-elba-reset.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    spi0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        num-cs = <4>;
> +
> +        spi@0 {
> +          compatible = "amd,pensando-elbasr", "simple-mfd";
> +          reg = <0>;
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +          spi-max-frequency = <12000000>;
> +
> +          rstc: reset-controller@0 {

Only one child does not make a MFD...

> +            compatible = "amd,pensando-elbasr-reset";
> +            reg = <0>;
> +            #reset-cells = <1>;
> +          };
> +        };
> +
> +        spi@1 {
> +          compatible = "amd,pensando-elbasr", "simple-mfd";
> +          reg = <1>;
> +          spi-max-frequency = <12000000>;

'simple-mfd' implies there are child nodes, but you have none. Is this 
complete?

> +        };
> +
> +        spi@2 {
> +          compatible = "amd,pensando-elbasr", "simple-mfd";
> +          reg = <2>;
> +          spi-max-frequency = <12000000>;
> +          interrupt-parent = <&porta>;
> +          interrupts = <0 IRQ_TYPE_LEVEL_LOW>;

This one has interrupt but the others don't?

> +        };
> +
> +        spi@3 {
> +          compatible = "amd,pensando-elbasr", "simple-mfd";
> +          reg = <3>;
> +          spi-max-frequency = <12000000>;
> +        };
> +    };
> +
> +...
> -- 
> 2.17.1
> 
> 

Re: [PATCH v5 06/15] dt-bindings: mfd: amd,pensando-elbasr: Add AMD Pensando Elba System Resource chip

[Brad Larson]

Hi Rob,

On Tue, Jun 14, 2022 at 2:30 PM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Jun 13, 2022 at 12:56:49PM -0700, Brad Larson wrote:
> > From: Brad Larson <blarson@amd.com>
> >
> > Add support for the AMD Pensando Elba SoC System Resource chip
> > using the SPI interface.  The Elba SR is a Multi-function Device
> > supporting device register access using CS0, smbus interface for
> > FRU and board peripherals using CS1, dual Lattice I2C masters for
> > transceiver management using CS2, and CS3 for flash access.
> >
> > Signed-off-by: Brad Larson <blarson@amd.com>
> > ---
> >  .../bindings/mfd/amd,pensando-elbasr.yaml     | 93 +++++++++++++++++++
> >  1 file changed, 93 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml b/Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml
> > new file mode 100644
> > index 000000000000..13356800b1cf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml
> > @@ -0,0 +1,93 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mfd/amd,pensando-elbasr.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: AMD Pensando Elba SoC Resource Controller bindings
> ...
> > +patternProperties:
> > +  '^reset-controller@[a-f0-9]+$':
> > +    $ref: ../reset/amd,pensando-elbasr-reset.yaml
>
> /schemas/reset/...

Changed it to
       $ref: /schemas/reset/amd,pensando-elbasr-reset.yaml

> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/reset/amd,pensando-elba-reset.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    spi0 {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +        num-cs = <4>;
> > +
> > +        spi@0 {
> > +          compatible = "amd,pensando-elbasr", "simple-mfd";
> > +          reg = <0>;
> > +          #address-cells = <1>;
> > +          #size-cells = <0>;
> > +          spi-max-frequency = <12000000>;
> > +
> > +          rstc: reset-controller@0 {
>
> Only one child does not make a MFD...

Looking over the approaches for other SoCs with an external
required controller (cpld, fpga) this appeared to be an
acceptable choice.  This device is accessed by several
different utilities/programs via /dev/pensr0.x, CS0 registers
for a variety of control/status, CS1 designware i2c master/slave,
CS2 lattice dual i2c masters, and CS3 for flash.

> > +            compatible = "amd,pensando-elbasr-reset";
> > +            reg = <0>;
> > +            #reset-cells = <1>;
> > +          };
> > +        };
> > +
> > +        spi@1 {
> > +          compatible = "amd,pensando-elbasr", "simple-mfd";
> > +          reg = <1>;
> > +          spi-max-frequency = <12000000>;
>
> 'simple-mfd' implies there are child nodes, but you have none. Is this
> complete?

This function is a designware i2c master/slave for board peripheral
access.  Removed simple-mfd.

> > +        };
> > +
> > +        spi@2 {
> > +          compatible = "amd,pensando-elbasr", "simple-mfd";
> > +          reg = <2>;
> > +          spi-max-frequency = <12000000>;
> > +          interrupt-parent = <&porta>;
> > +          interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
>
> This one has interrupt but the others don't?

Yes, this function is a Lattice dual I2C master for transceiver
management.  The spi to i2c driver is not included in this patch set for
essential Elba SoC support.  Removed simple-mfd.

Regards,
Brad

Re: [PATCH v5 06/15] dt-bindings: mfd: amd,pensando-elbasr: Add AMD Pensando Elba System Resource chip

[Krzysztof Kozlowski]

On 13/06/2022 21:56, Brad Larson wrote:
> From: Brad Larson <blarson@amd.com>
> 
> Add support for the AMD Pensando Elba SoC System Resource chip
> using the SPI interface.  The Elba SR is a Multi-function Device
> supporting device register access using CS0, smbus interface for
> FRU and board peripherals using CS1, dual Lattice I2C masters for
> transceiver management using CS2, and CS3 for flash access.
> 
> Signed-off-by: Brad Larson <blarson@amd.com>
> ---
>  .../bindings/mfd/amd,pensando-elbasr.yaml     | 93 +++++++++++++++++++
>  1 file changed, 93 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml b/Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml
> new file mode 100644
> index 000000000000..13356800b1cf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml
> @@ -0,0 +1,93 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/amd,pensando-elbasr.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: AMD Pensando Elba SoC Resource Controller bindings
> +
> +description: |
> +  AMD Pensando Elba SoC Resource Controller bindings attached to a SPI bus.
> +
> +maintainers:
> +  - Brad Larson <blarson@amd.com>
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - amd,pensando-elbasr
> +      - const: simple-mfd
> +
> +  spi-max-frequency:
> +    description: Maximum SPI frequency of the device in Hz.
> +
> +  reg:
> +    maxItems: 1
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - spi-max-frequency
> +
> +patternProperties:
> +  '^reset-controller@[a-f0-9]+$':
> +    $ref: ../reset/amd,pensando-elbasr-reset.yaml
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/reset/amd,pensando-elba-reset.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    spi0 {

Just "spi"

> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        num-cs = <4>;
> +
> +        spi@0 {

"spi" is for SPI controllers. Use generic name matching the device.
Usually this is "system-controller", however Rob pointed out your
inaccurate bindings and example.

> +          compatible = "amd,pensando-elbasr", "simple-mfd";
> +          reg = <0>;
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +          spi-max-frequency = <12000000>;
> +
> +          rstc: reset-controller@0 {
> +            compatible = "amd,pensando-elbasr-reset";
> +            reg = <0>;
> +            #reset-cells = <1>;
> +          };
> +        };
> +
> +        spi@1 {
> +          compatible = "amd,pensando-elbasr", "simple-mfd";
> +          reg = <1>;
> +          spi-max-frequency = <12000000>;
> +        };
> +
> +        spi@2 {
> +          compatible = "amd,pensando-elbasr", "simple-mfd";
> +          reg = <2>;
> +          spi-max-frequency = <12000000>;
> +          interrupt-parent = <&porta>;
> +          interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> +        };
> +
> +        spi@3 {
> +          compatible = "amd,pensando-elbasr", "simple-mfd";
> +          reg = <3>;
> +          spi-max-frequency = <12000000>;
> +        };
> +    };
> +
> +...


Best regards,
Krzysztof

Re: [PATCH v5 06/15] dt-bindings: mfd: amd,pensando-elbasr: Add AMD Pensando Elba System Resource chip

[Brad Larson]

Hi Krzysztof,

On Mon, Jun 20, 2022 at 5:56 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 13/06/2022 21:56, Brad Larson wrote:
> > From: Brad Larson <blarson@amd.com>
> >
> > Add support for the AMD Pensando Elba SoC System Resource chip
> > using the SPI interface.  The Elba SR is a Multi-function Device
> > supporting device register access using CS0, smbus interface for
> > FRU and board peripherals using CS1, dual Lattice I2C masters for
> > transceiver management using CS2, and CS3 for flash access.
> >
> > Signed-off-by: Brad Larson <blarson@amd.com>
> > ---
> >  .../bindings/mfd/amd,pensando-elbasr.yaml     | 93 +++++++++++++++++++
> >  1 file changed, 93 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml
> ...
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/reset/amd,pensando-elba-reset.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    spi0 {
>
> Just "spi"

Changed to spi

> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +        num-cs = <4>;
> > +
> > +        spi@0 {
>
> "spi" is for SPI controllers. Use generic name matching the device.
> Usually this is "system-controller", however Rob pointed out your
> inaccurate bindings and example.

Proposed the below change node in the reply to Rob.  The model I
followed for this was the Altera mfd/altera-a10sr.c

spi@0 {
        sr_regs@0 {
                rstc: reset-controller@0 {

        dw_i2c@1 {

        lattice_i2c@2 {

        flash@3 {

Regards,
Brad

Re: [PATCH v5 06/15] dt-bindings: mfd: amd,pensando-elbasr: Add AMD Pensando Elba System Resource chip

[Krzysztof Kozlowski]

On 04/07/2022 01:41, Brad Larson wrote:
> Hi Krzysztof,
> 
> On Mon, Jun 20, 2022 at 5:56 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 13/06/2022 21:56, Brad Larson wrote:
>>> From: Brad Larson <blarson@amd.com>
>>>
>>> Add support for the AMD Pensando Elba SoC System Resource chip
>>> using the SPI interface.  The Elba SR is a Multi-function Device
>>> supporting device register access using CS0, smbus interface for
>>> FRU and board peripherals using CS1, dual Lattice I2C masters for
>>> transceiver management using CS2, and CS3 for flash access.
>>>
>>> Signed-off-by: Brad Larson <blarson@amd.com>
>>> ---
>>>  .../bindings/mfd/amd,pensando-elbasr.yaml     | 93 +++++++++++++++++++
>>>  1 file changed, 93 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml
>> ...
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/reset/amd,pensando-elba-reset.h>
>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +
>>> +    spi0 {
>>
>> Just "spi"
> 
> Changed to spi
> 
>>> +        #address-cells = <1>;
>>> +        #size-cells = <0>;
>>> +        num-cs = <4>;
>>> +
>>> +        spi@0 {
>>
>> "spi" is for SPI controllers. Use generic name matching the device.
>> Usually this is "system-controller", however Rob pointed out your
>> inaccurate bindings and example.
> 
> Proposed the below change node in the reply to Rob.  The model I
> followed for this was the Altera mfd/altera-a10sr.c

You pointed driver, so how is it related to bindings? Do not mix Linux
implementation with the bindings.

> 
> spi@0 {
>         sr_regs@0 {
>                 rstc: reset-controller@0 {

No underscores in node names. sr_regs is not generic name.

> 
>         dw_i2c@1 {

Again, not a generic name. If it is i2c controller, should be i2c. If it
is i2c device/client, should be something generic matching class of the
device.

> 
>         lattice_i2c@2 {
> 
>         flash@3 {

This looks ok, depending on compatible.

> 
> Regards,
> Brad


Best regards,
Krzysztof

[PATCH v5 07/15] dt-bindings: reset: amd,pensando-elbasr-reset: Add AMD Pensando SR Reset Controller bindings

[Brad Larson]

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 {
+          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

Re: [PATCH v5 07/15] dt-bindings: reset: amd,pensando-elbasr-reset: Add AMD Pensando SR Reset Controller bindings

[Rob Herring]

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
> 
> 

Re: [PATCH v5 07/15] dt-bindings: reset: amd,pensando-elbasr-reset: Add AMD Pensando SR Reset Controller bindings

[Brad Larson]

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

Re: [PATCH v5 07/15] dt-bindings: reset: amd,pensando-elbasr-reset: Add AMD Pensando SR Reset Controller bindings

[Krzysztof Kozlowski]

On 13/06/2022 21:56, 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

Here and in all other patches:
s/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]+$"

Skip the pattern. No particular need for it and unit address part is not
correct (const: 0).

> +
> +  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>

Missing file:
ls: cannot access 'include/dt-bindings/reset/amd,pensando-elba-reset.h':
No such file or directory


Send complete bindings, not parts of it. Did you test it? I am pretty
sure that this did not happen. :(

> +    spi0 {

spi

> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        num-cs = <4>;
> +
> +        spi@0 {
> +          reg = <0>;
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +          rstc: reset-controller@0 {
> +            compatible = "amd,pensando-elbasr-reset";
> +            reg = <0>;
> +            #reset-cells = <1>;
> +          };
> +        };
> +    };
> +
> +...


Best regards,
Krzysztof

Re: [PATCH v5 07/15] dt-bindings: reset: amd,pensando-elbasr-reset: Add AMD Pensando SR Reset Controller bindings

[Brad Larson]

Hi Krzysztof,

On Mon, Jun 20, 2022 at 6:00 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 13/06/2022 21:56, 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
>
> Here and in all other patches:
> s/Device Tree Bindings//

Removed, must be implicit now, currently 366 files use it
$ find . -name \*.yaml|xargs grep title|grep 'Device Tree Bindings'|wc
    366

> > +
> > +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]+$"
>
> Skip the pattern. No particular need for it and unit address part is not
> correct (const: 0).

Deleted these lines
  $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>
>
> Missing file:
> ls: cannot access 'include/dt-bindings/reset/amd,pensando-elba-reset.h':
> No such file or directory
>
>
> Send complete bindings, not parts of it. Did you test it? I am pretty
> sure that this did not happen. :(

Its in patch v5-0015 with the driver.  I'll check this, the correct
approach should be put all binding changes as individual patches up
front or there are exceptions for new driver.

$ cat v5-0015-reset-elbasr-Add-AMD-Pensando-Elba-SR-Reset-Contr.patch
| grep diff
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
diff --git a/drivers/reset/reset-elbasr.c b/drivers/reset/reset-elbasr.c
diff --git a/include/dt-bindings/reset/amd,pensando-elba-reset.h
b/include/dt-bindings/reset/amd,pensando-elba-reset.h

Yes, tested it with the following and no warnings or errors
make ARCH=arm64 dtbs_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/arm/amd,pensando.yaml
make ARCH=arm64 dtbs_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/mfd/syscon.yaml
make ARCH=arm64 dtbs_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
make ARCH=arm64 dtbs_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
make ARCH=arm64 dtbs_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
make ARCH=arm64 dtbs_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/vendor-prefixes.yaml
make ARCH=arm64 dtbs_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml
make ARCH=arm64 dtbs_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/reset/amd,pensando-elbasr-reset.yaml

make DT_CHECKER_FLAGS=-m dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/arm/amd,pensando.yaml
make DT_CHECKER_FLAGS=-m dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/mfd/syscon.yaml
make DT_CHECKER_FLAGS=-m dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
make DT_CHECKER_FLAGS=-m dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
make DT_CHECKER_FLAGS=-m dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
make DT_CHECKER_FLAGS=-m dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/vendor-prefixes.yaml
make DT_CHECKER_FLAGS=-m dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml
make DT_CHECKER_FLAGS=-m dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/reset/amd,pensando-elbasr-reset.yaml

> > +    spi0 {
>
> spi

Changed to spi

Regards,
Brad

Re: [PATCH v5 07/15] dt-bindings: reset: amd,pensando-elbasr-reset: Add AMD Pensando SR Reset Controller bindings

[Krzysztof Kozlowski]

On 04/07/2022 01:50, Brad Larson wrote:
>> Missing file:
>> ls: cannot access 'include/dt-bindings/reset/amd,pensando-elba-reset.h':
>> No such file or directory
>>
>>
>> Send complete bindings, not parts of it. Did you test it? I am pretty
>> sure that this did not happen. :(
> 
> Its in patch v5-0015 with the driver

Header is part of bindings, not driver.

>.  I'll check this, the correct
> approach should be put all binding changes as individual patches up
> front or there are exceptions for new driver.
> 
> $ cat v5-0015-reset-elbasr-Add-AMD-Pensando-Elba-SR-Reset-Contr.patch
> | grep diff
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> diff --git a/drivers/reset/reset-elbasr.c b/drivers/reset/reset-elbasr.c
> diff --git a/include/dt-bindings/reset/amd,pensando-elba-reset.h
> b/include/dt-bindings/reset/amd,pensando-elba-reset.h
> 
> Yes, tested it with the following and no warnings or errors
> make ARCH=arm64 dtbs_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/arm/amd,pensando.yaml
> make ARCH=arm64 dtbs_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/mfd/syscon.yaml
> make ARCH=arm64 dtbs_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> make ARCH=arm64 dtbs_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> make ARCH=arm64 dtbs_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> make ARCH=arm64 dtbs_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/vendor-prefixes.yaml
> make ARCH=arm64 dtbs_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml
> make ARCH=arm64 dtbs_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/reset/amd,pensando-elbasr-reset.yaml
> 
> make DT_CHECKER_FLAGS=-m dt_binding_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/arm/amd,pensando.yaml
> make DT_CHECKER_FLAGS=-m dt_binding_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/mfd/syscon.yaml
> make DT_CHECKER_FLAGS=-m dt_binding_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml
> make DT_CHECKER_FLAGS=-m dt_binding_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> make DT_CHECKER_FLAGS=-m dt_binding_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> make DT_CHECKER_FLAGS=-m dt_binding_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/vendor-prefixes.yaml
> make DT_CHECKER_FLAGS=-m dt_binding_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml
> make DT_CHECKER_FLAGS=-m dt_binding_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/reset/amd,pensando-elbasr-reset.yaml

So how this test could pass if there is no header file included in the
example here? Are you sure you tested each commit separately (like it
will be included in the kernel)?

Best regards,
Krzysztof

Re: [PATCH v5 07/15] dt-bindings: reset: amd,pensando-elbasr-reset: Add AMD Pensando SR Reset Controller bindings

[Brad Larson]

Hi Krzysztof,

On Sun, Jul 3, 2022 at 11:41 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 04/07/2022 01:50, Brad Larson wrote:
> >> Missing file:
> >> ls: cannot access 'include/dt-bindings/reset/amd,pensando-elba-reset.h':
> >> No such file or directory
> >>
> >>
> >> Send complete bindings, not parts of it. Did you test it? I am pretty
> >> sure that this did not happen. :(
> >
> > Its in patch v5-0015 with the driver
>
> Header is part of bindings, not driver.

That's the reason, the header was not with the bindings.

> ...
> So how this test could pass if there is no header file included in the
> example here? Are you sure you tested each commit separately (like it
> will be included in the kernel)?

and I had applied the patchset before running the checks.  I'll check
each commit separately for next version.

Regards,
Brad

Re: [PATCH v5 07/15] dt-bindings: reset: amd,pensando-elbasr-reset: Add AMD Pensando SR Reset Controller bindings

[Krzysztof Kozlowski]

On 05/07/2022 20:28, Brad Larson wrote:
> Hi Krzysztof,
> 
> On Sun, Jul 3, 2022 at 11:41 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 04/07/2022 01:50, Brad Larson wrote:
>>>> Missing file:
>>>> ls: cannot access 'include/dt-bindings/reset/amd,pensando-elba-reset.h':
>>>> No such file or directory
>>>>
>>>>
>>>> Send complete bindings, not parts of it. Did you test it? I am pretty
>>>> sure that this did not happen. :(
>>>
>>> Its in patch v5-0015 with the driver
>>
>> Header is part of bindings, not driver.
> 
> That's the reason, the header was not with the bindings.

Sorry, I don't understand. The reason header was not with the bindings
is that header is part of bindings? That does not make really sense...

Anyway, don't mix up bindings and driver changes in one commit.

Best regards,
Krzysztof

[PATCH v5 08/15] MAINTAINERS: Add entry for AMD PENSANDO

[Brad Larson]

From: Brad Larson <blarson@amd.com>

Add entry for AMD PENSANDO maintainer and files

Signed-off-by: Brad Larson <blarson@amd.com>
---
 MAINTAINERS | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 91e9cd30326d..09828169c7c5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1777,6 +1777,16 @@ N:	allwinner
 N:	sun[x456789]i
 N:	sun50i
 
+ARM/AMD PENSANDO ARM64 ARCHITECTURE
+M:	Brad Larson <blarson@amd.com>
+L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
+S:	Supported
+F:	Documentation/devicetree/bindings/*/amd,pensando*
+F:	arch/arm64/boot/dts/amd/elba*
+F:	drivers/mfd/pensando*
+F:	drivers/reset/reset-elbasr.c
+F:	include/dt-bindings/reset/amd,pensando*
+
 ARM/Amlogic Meson SoC CLOCK FRAMEWORK
 M:	Neil Armstrong <narmstrong@baylibre.com>
 M:	Jerome Brunet <jbrunet@baylibre.com>
-- 
2.17.1

[PATCH v5 09/15] arm64: Add config for AMD Pensando SoC platforms

[Brad Larson]

From: Brad Larson <blarson@amd.com>

Add ARCH_PENSANDO configuration option for AMD Pensando
SoC based platforms.

Signed-off-by: Brad Larson <blarson@amd.com>
---
 arch/arm64/Kconfig.platforms | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 4e6d635a1731..c650a89d8452 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -202,6 +202,18 @@ config ARCH_MXC
 	  This enables support for the ARMv8 based SoCs in the
 	  NXP i.MX family.
 
+config ARCH_PENSANDO
+	bool "AMD Pensando Platforms"
+	help
+	  This enables support for the ARMv8 based AMD Pensando SoC
+	  family to include the Elba SoC.
+
+	  AMD Pensando SoCs support a range of Distributed Services
+	  Cards in PCIe format installed into servers.  The Elba
+	  SoC includes 16 A-72 CPU cores, 144 programmable P4
+	  cores for a minimal latency/jitter datapath, and network
+	  interfaces up to 200 Gb/s.
+
 config ARCH_QCOM
 	bool "Qualcomm Platforms"
 	select GPIOLIB
-- 
2.17.1

[PATCH v5 10/15] arm64: dts: Add AMD Pensando Elba SoC support

[Brad Larson]

From: Brad Larson <blarson@amd.com>

Add AMD Pensando common and Elba SoC specific device nodes

Signed-off-by: Brad Larson <blarson@amd.com>
---
 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
diff --git a/arch/arm64/boot/dts/amd/elba-16core.dtsi b/arch/arm64/boot/dts/amd/elba-16core.dtsi
new file mode 100644
index 000000000000..274dd80de1a4
--- /dev/null
+++ b/arch/arm64/boot/dts/amd/elba-16core.dtsi
@@ -0,0 +1,189 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2020-2022 AMD Pensando
+ */
+
+/ {
+	cpus {
+		#address-cells = <2>;
+		#size-cells = <0>;
+
+		cpu-map {
+			cluster0 {
+				core0 { cpu = <&cpu0>; };
+				core1 { cpu = <&cpu1>; };
+				core2 { cpu = <&cpu2>; };
+				core3 { cpu = <&cpu3>; };
+			};
+
+			cluster1 {
+				core0 { cpu = <&cpu4>; };
+				core1 { cpu = <&cpu5>; };
+				core2 { cpu = <&cpu6>; };
+				core3 { cpu = <&cpu7>; };
+			};
+
+			cluster2 {
+				core0 { cpu = <&cpu8>; };
+				core1 { cpu = <&cpu9>; };
+				core2 { cpu = <&cpu10>; };
+				core3 { cpu = <&cpu11>; };
+			};
+
+			cluster3 {
+				core0 { cpu = <&cpu12>; };
+				core1 { cpu = <&cpu13>; };
+				core2 { cpu = <&cpu14>; };
+				core3 { cpu = <&cpu15>; };
+			};
+		};
+
+		/* CLUSTER 0 */
+		cpu0: cpu@0 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a72";
+			reg = <0 0x0>;
+			next-level-cache = <&l2_0>;
+			enable-method = "psci";
+		};
+
+		cpu1: cpu@1 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a72";
+			reg = <0 0x1>;
+			next-level-cache = <&l2_0>;
+			enable-method = "psci";
+		};
+
+		cpu2: cpu@2 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a72";
+			reg = <0 0x2>;
+			next-level-cache = <&l2_0>;
+			enable-method = "psci";
+		};
+
+		cpu3: cpu@3 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a72";
+			reg = <0 0x3>;
+			next-level-cache = <&l2_0>;
+			enable-method = "psci";
+		};
+
+		l2_0: l2-cache0 {
+			compatible = "cache";
+		};
+
+		/* CLUSTER 1 */
+		cpu4: cpu@100 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a72";
+			reg = <0 0x100>;
+			next-level-cache = <&l2_1>;
+			enable-method = "psci";
+		};
+
+		cpu5: cpu@101 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a72";
+			reg = <0 0x101>;
+			next-level-cache = <&l2_1>;
+			enable-method = "psci";
+		};
+
+		cpu6: cpu@102 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a72";
+			reg = <0 0x102>;
+			next-level-cache = <&l2_1>;
+			enable-method = "psci";
+		};
+
+		cpu7: cpu@103 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a72";
+			reg = <0 0x103>;
+			next-level-cache = <&l2_1>;
+			enable-method = "psci";
+		};
+
+		l2_1: l2-cache1 {
+			compatible = "cache";
+		};
+
+		/* CLUSTER 2 */
+		cpu8: cpu@200 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a72";
+			reg = <0 0x200>;
+			next-level-cache = <&l2_2>;
+			enable-method = "psci";
+		};
+
+		cpu9: cpu@201 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a72";
+			reg = <0 0x201>;
+			next-level-cache = <&l2_2>;
+			enable-method = "psci";
+		};
+
+		cpu10: cpu@202 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a72";
+			reg = <0 0x202>;
+			next-level-cache = <&l2_2>;
+			enable-method = "psci";
+		};
+
+		cpu11: cpu@203 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a72";
+			reg = <0 0x203>;
+			next-level-cache = <&l2_2>;
+			enable-method = "psci";
+		};
+
+		l2_2: l2-cache2 {
+			compatible = "cache";
+		};
+
+		/* CLUSTER 3 */
+		cpu12: cpu@300 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a72";
+			reg = <0 0x300>;
+			next-level-cache = <&l2_3>;
+			enable-method = "psci";
+		};
+
+		cpu13: cpu@301 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a72";
+			reg = <0 0x301>;
+			next-level-cache = <&l2_3>;
+			enable-method = "psci";
+		};
+
+		cpu14: cpu@302 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a72";
+			reg = <0 0x302>;
+			next-level-cache = <&l2_3>;
+			enable-method = "psci";
+		};
+
+		cpu15: cpu@303 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a72";
+			reg = <0 0x303>;
+			next-level-cache = <&l2_3>;
+			enable-method = "psci";
+		};
+
+		l2_3: l2-cache3 {
+			compatible = "cache";
+		};
+	};
+};
diff --git a/arch/arm64/boot/dts/amd/elba-asic-common.dtsi b/arch/arm64/boot/dts/amd/elba-asic-common.dtsi
new file mode 100644
index 000000000000..155d35b8459f
--- /dev/null
+++ b/arch/arm64/boot/dts/amd/elba-asic-common.dtsi
@@ -0,0 +1,103 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2020-2022, AMD Pensando
+ */
+
+#include <dt-bindings/reset/amd,pensando-elba-reset.h>
+
+&ahb_clk {
+	clock-frequency = <400000000>;
+};
+
+&emmc_clk {
+	clock-frequency = <200000000>;
+};
+
+&flash_clk {
+	clock-frequency = <400000000>;
+};
+
+&ref_clk {
+	clock-frequency = <156250000>;
+};
+
+&qspi {
+	status = "okay";
+	flash0: flash@0 {
+		compatible = "jedec,spi-nor";
+		reg = <0>;
+		spi-max-frequency = <40000000>;
+		spi-rx-bus-width = <2>;
+		m25p,fast-read;
+		cdns,read-delay = <0>;
+		cdns,tshsl-ns = <0>;
+		cdns,tsd2d-ns = <0>;
+		cdns,tchsh-ns = <0>;
+		cdns,tslch-ns = <0>;
+	};
+};
+
+&gpio0 {
+	status = "okay";
+};
+
+&emmc {
+	bus-width = <8>;
+	cap-mmc-hw-reset;
+	reset-names = "hw";
+	resets = <&rstc EMMC_HW_RESET>;
+	status = "okay";
+};
+
+&wdt0 {
+	status = "okay";
+};
+
+&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 {
+		compatible = "amd,pensando-elbasr", "simple-mfd";
+		reg = <0>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		spi-max-frequency = <12000000>;
+
+		rstc: reset-controller@0 {
+			compatible = "amd,pensando-elbasr-reset";
+			reg = <0>;
+			#reset-cells = <1>;
+		};
+	};
+
+	spi@1 {
+		compatible = "amd,pensando-elbasr", "simple-mfd";
+		reg = <1>;
+		spi-max-frequency = <12000000>;
+	};
+
+	spi@2 {
+		compatible = "amd,pensando-elbasr", "simple-mfd";
+		reg = <2>;
+		spi-max-frequency = <12000000>;
+		interrupt-parent = <&porta>;
+		interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
+	};
+
+	spi@3 {
+		compatible = "amd,pensando-elbasr", "simple-mfd";
+		reg = <3>;
+		spi-max-frequency = <12000000>;
+	};
+};
diff --git a/arch/arm64/boot/dts/amd/elba-asic.dts b/arch/arm64/boot/dts/amd/elba-asic.dts
new file mode 100644
index 000000000000..bb64fd042b63
--- /dev/null
+++ b/arch/arm64/boot/dts/amd/elba-asic.dts
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Device Tree file for AMD Pensando Elba Board.
+ *
+ * Copyright (c) 2020-2022 AMD Pensando
+ */
+
+/dts-v1/;
+
+#include "elba.dtsi"
+#include "elba-16core.dtsi"
+#include "elba-asic-common.dtsi"
+#include "elba-flash-parts.dtsi"
+
+/ {
+	model = "AMD Pensando Elba Board";
+	compatible = "amd,pensando-elba-ortano", "amd,pensando-elba";
+
+	aliases {
+		serial0 = &uart0;
+		spi0 = &spi0;
+		spi1 = &qspi;
+	};
+
+	chosen {
+		stdout-path = "serial0:115200n8";
+	};
+};
diff --git a/arch/arm64/boot/dts/amd/elba-flash-parts.dtsi b/arch/arm64/boot/dts/amd/elba-flash-parts.dtsi
new file mode 100644
index 000000000000..2599d1b22026
--- /dev/null
+++ b/arch/arm64/boot/dts/amd/elba-flash-parts.dtsi
@@ -0,0 +1,106 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2020-2022 AMD Pensando
+ */
+
+&flash0 {
+	partitions {
+		compatible = "fixed-partitions";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		partition@0 {
+			label = "flash";
+			reg = <0x10000 0xfff0000>;
+		};
+
+		partition@f0000 {
+			label = "golduenv";
+			reg = <0xf0000 0x10000>;
+		};
+
+		partition@100000 {
+			label = "boot0";
+			reg = <0x100000 0x80000>;
+		};
+
+		partition@180000 {
+			label = "golduboot";
+			reg = <0x180000 0x200000>;
+		};
+
+		partition@380000 {
+			label = "brdcfg0";
+			reg = <0x380000 0x10000>;
+		};
+
+		partition@390000 {
+			label = "brdcfg1";
+			reg = <0x390000 0x10000>;
+		};
+
+		partition@400000 {
+			label = "goldfw";
+			reg = <0x400000 0x3c00000>;
+		};
+
+		partition@4010000 {
+			label = "fwmap";
+			reg = <0x4010000 0x20000>;
+		};
+
+		partition@4030000 {
+			label = "fwsel";
+			reg = <0x4030000 0x20000>;
+		};
+
+		partition@4090000 {
+			label = "bootlog";
+			reg = <0x4090000 0x20000>;
+		};
+
+		partition@40b0000 {
+			label = "panicbuf";
+			reg = <0x40b0000 0x20000>;
+		};
+
+		partition@40d0000 {
+			label = "uservars";
+			reg = <0x40d0000 0x20000>;
+		};
+
+		partition@4200000 {
+			label = "uboota";
+			reg = <0x4200000 0x400000>;
+		};
+
+		partition@4600000 {
+			label = "ubootb";
+			reg = <0x4600000 0x400000>;
+		};
+
+		partition@4a00000 {
+			label = "mainfwa";
+			reg = <0x4a00000 0x1000000>;
+		};
+
+		partition@5a00000 {
+			label = "mainfwb";
+			reg = <0x5a00000 0x1000000>;
+		};
+
+		partition@6a00000 {
+			label = "diaguboot";
+			reg = <0x6a00000 0x400000>;
+		};
+
+		partition@8000000 {
+			label = "diagfw";
+			reg = <0x8000000 0x7fe0000>;
+		};
+
+		partition@ffe0000 {
+			label = "ubootenv";
+			reg = <0xffe0000 0x10000>;
+		};
+	};
+};
diff --git a/arch/arm64/boot/dts/amd/elba.dtsi b/arch/arm64/boot/dts/amd/elba.dtsi
new file mode 100644
index 000000000000..9739641261c3
--- /dev/null
+++ b/arch/arm64/boot/dts/amd/elba.dtsi
@@ -0,0 +1,191 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2020-2022, AMD Pensando
+ */
+
+#include <dt-bindings/gpio/gpio.h>
+#include "dt-bindings/interrupt-controller/arm-gic.h"
+
+/ {
+	model = "Elba ASIC Board";
+	compatible = "amd,pensando-elba";
+	interrupt-parent = <&gic>;
+	#address-cells = <2>;
+	#size-cells = <2>;
+
+	dma-coherent;
+
+	ahb_clk: oscillator0 {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+	};
+
+	emmc_clk: oscillator2 {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+	};
+
+	flash_clk: oscillator3 {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+	};
+
+	ref_clk: oscillator4 {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+	};
+
+	psci {
+		compatible = "arm,psci-0.2";
+		method = "smc";
+	};
+
+	timer {
+		compatible = "arm,armv8-timer";
+		interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_LOW>,
+			     <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>,
+			     <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW>,
+			     <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW>;
+	};
+
+	pmu {
+		compatible = "arm,cortex-a72-pmu";
+		interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_LOW>;
+	};
+
+	soc: soc {
+		compatible = "simple-bus";
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		i2c0: i2c@400 {
+			compatible = "snps,designware-i2c";
+			reg = <0x0 0x400 0x0 0x100>;
+			clocks = <&ahb_clk>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			i2c-sda-hold-time-ns = <480>;
+			snps,sda-timeout-ms = <750>;
+			interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
+			status = "disabled";
+		};
+
+		wdt0: watchdog@1400 {
+			compatible = "snps,dw-wdt";
+			reg = <0x0 0x1400 0x0 0x100>;
+			clocks = <&ahb_clk>;
+			interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
+			status = "disabled";
+		};
+
+		qspi: spi@2400 {
+			compatible = "amd,pensando-elba-qspi", "cdns,qspi-nor";
+			reg = <0x0 0x2400 0x0 0x400>,
+			      <0x0 0x7fff0000 0x0 0x1000>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&flash_clk>;
+			cdns,fifo-depth = <1024>;
+			cdns,fifo-width = <4>;
+			cdns,trigger-address = <0x7fff0000>;
+			status = "disabled";
+		};
+
+		spi0: spi@2800 {
+			compatible = "amd,pensando-elba-spi";
+			reg = <0x0 0x2800 0x0 0x100>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&ahb_clk>;
+			interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
+			num-cs = <2>;
+			status = "disabled";
+		};
+
+		gpio0: gpio@4000 {
+			compatible = "snps,dw-apb-gpio";
+			reg = <0x0 0x4000 0x0 0x78>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+
+			porta: gpio-port@0 {
+				compatible = "snps,dw-apb-gpio-port";
+				reg = <0>;
+				gpio-controller;
+				#gpio-cells = <2>;
+				ngpios = <8>;
+				interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-controller;
+				interrupt-parent = <&gic>;
+				#interrupt-cells = <2>;
+			};
+
+			portb: gpio-port@1 {
+				compatible = "snps,dw-apb-gpio-port";
+				reg = <1>;
+				gpio-controller;
+				#gpio-cells = <2>;
+				ngpios = <8>;
+			};
+		};
+
+		uart0: serial@4800 {
+			compatible = "ns16550a";
+			reg = <0x0 0x4800 0x0 0x100>;
+			clocks = <&ref_clk>;
+			interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
+			reg-shift = <2>;
+			reg-io-width = <4>;
+		};
+
+		gic: interrupt-controller@800000 {
+			compatible = "arm,gic-v3";
+			reg = <0x0 0x800000 0x0 0x200000>,	/* GICD */
+			      <0x0 0xa00000 0x0 0x200000>,	/* GICR */
+			      <0x0 0x60000000 0x0 0x2000>,	/* GICC */
+			      <0x0 0x60010000 0x0 0x1000>,	/* GICH */
+			      <0x0 0x60020000 0x0 0x2000>;	/* GICV */
+			#address-cells = <2>;
+			#size-cells = <2>;
+			#interrupt-cells = <3>;
+			ranges;
+			interrupt-controller;
+			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
+
+			/*
+			 * Elba specific pre-ITS is enabled using the
+			 * existing property socionext,synquacer-pre-its
+			 */
+			gic_its: msi-controller@820000 {
+				compatible = "arm,gic-v3-its";
+				reg = <0x0 0x820000 0x0 0x10000>;
+				msi-controller;
+				#msi-cells = <1>;
+				socionext,synquacer-pre-its =
+							<0xc00000 0x1000000>;
+			};
+		};
+
+		emmc: mmc@30440000 {
+			compatible = "amd,pensando-elba-sd4hc", "cdns,sd4hc";
+			reg = <0x0 0x30440000 0x0 0x10000>,
+			      <0x0 0x30480044 0x0 0x4>;	/* byte-lane ctrl */
+			clocks = <&emmc_clk>;
+			interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
+			cdns,phy-input-delay-sd-highspeed = <0x4>;
+			cdns,phy-input-delay-legacy = <0x4>;
+			cdns,phy-input-delay-sd-uhs-sdr50 = <0x6>;
+			cdns,phy-input-delay-sd-uhs-ddr50 = <0x16>;
+			mmc-ddr-1_8v;
+			status = "disabled";
+		};
+
+		syscon: syscon@307c0000 {
+			compatible = "amd,pensando-elba-syscon", "syscon";
+			reg = <0x0 0x307c0000 0x0 0x3000>;
+		};
+	};
+};
-- 
2.17.1

Re: [PATCH v5 10/15] arm64: dts: Add AMD Pensando Elba SoC support

[Krzysztof Kozlowski]

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

Re: [PATCH v5 10/15] arm64: dts: Add AMD Pensando Elba SoC support

[Brad Larson]

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

[PATCH v5 11/15] spi: cadence-quadspi: Add compatible for AMD Pensando Elba SoC

[Brad Larson]

From: Brad Larson <blarson@amd.com>

The AMD Pensando Elba SoC has the Cadence QSPI controller integrated.

The quirk CQSPI_NEEDS_APB_AHB_HAZARD_WAR is added and if enabled
a dummy readback from the controller is performed to ensure
synchronization.

Signed-off-by: Brad Larson <blarson@amd.com>
---
 drivers/spi/spi-cadence-quadspi.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 72b1a5a2298c..ebb77ea8e6ba 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -39,6 +39,7 @@
 #define CQSPI_DISABLE_DAC_MODE		BIT(1)
 #define CQSPI_SUPPORT_EXTERNAL_DMA	BIT(2)
 #define CQSPI_NO_SUPPORT_WR_COMPLETION	BIT(3)
+#define CQSPI_NEEDS_APB_AHB_HAZARD_WAR	BIT(4)
 
 /* Capabilities */
 #define CQSPI_SUPPORTS_OCTAL		BIT(0)
@@ -87,6 +88,7 @@ struct cqspi_st {
 	bool			use_dma_read;
 	u32			pd_dev_id;
 	bool			wr_completion;
+	bool			apb_ahb_hazard;
 };
 
 struct cqspi_driver_platdata {
@@ -952,6 +954,13 @@ static int cqspi_indirect_write_execute(struct cqspi_flash_pdata *f_pdata,
 	if (cqspi->wr_delay)
 		ndelay(cqspi->wr_delay);
 
+	/*
+	 * If a hazard exists between the APB and AHB interfaces, perform a
+	 * dummy readback from the controller to ensure synchronization.
+	 */
+	if (cqspi->apb_ahb_hazard)
+		(void)readl(reg_base + CQSPI_REG_INDIRECTWR);
+
 	while (remaining > 0) {
 		size_t write_words, mod_bytes;
 
@@ -1667,6 +1676,8 @@ static int cqspi_probe(struct platform_device *pdev)
 			cqspi->use_dma_read = true;
 		if (ddata->quirks & CQSPI_NO_SUPPORT_WR_COMPLETION)
 			cqspi->wr_completion = false;
+		if (ddata->quirks & CQSPI_NEEDS_APB_AHB_HAZARD_WAR)
+			cqspi->apb_ahb_hazard = true;
 
 		if (of_device_is_compatible(pdev->dev.of_node,
 					    "xlnx,versal-ospi-1.0"))
@@ -1789,6 +1800,10 @@ static const struct cqspi_driver_platdata versal_ospi = {
 	.get_dma_status = cqspi_get_versal_dma_status,
 };
 
+static const struct cqspi_driver_platdata pen_cdns_qspi = {
+	.quirks = CQSPI_NEEDS_APB_AHB_HAZARD_WAR | CQSPI_DISABLE_DAC_MODE,
+};
+
 static const struct of_device_id cqspi_dt_ids[] = {
 	{
 		.compatible = "cdns,qspi-nor",
@@ -1814,6 +1829,10 @@ static const struct of_device_id cqspi_dt_ids[] = {
 		.compatible = "intel,socfpga-qspi",
 		.data = &socfpga_qspi,
 	},
+	{
+		.compatible = "amd,pensando-elba-qspi",
+		.data = &pen_cdns_qspi,
+	},
 	{ /* end of table */ }
 };
 
-- 
2.17.1

Re: [PATCH v5 11/15] spi: cadence-quadspi: Add compatible for AMD Pensando Elba SoC

[Pratyush Yadav]

Hi Brad,

On 13/06/22 12:56PM, Brad Larson wrote:
> From: Brad Larson <blarson@amd.com>
> 
> The AMD Pensando Elba SoC has the Cadence QSPI controller integrated.
> 
> The quirk CQSPI_NEEDS_APB_AHB_HAZARD_WAR is added and if enabled
> a dummy readback from the controller is performed to ensure
> synchronization.
> 
> Signed-off-by: Brad Larson <blarson@amd.com>
> ---
>  drivers/spi/spi-cadence-quadspi.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 72b1a5a2298c..ebb77ea8e6ba 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -39,6 +39,7 @@
>  #define CQSPI_DISABLE_DAC_MODE		BIT(1)
>  #define CQSPI_SUPPORT_EXTERNAL_DMA	BIT(2)
>  #define CQSPI_NO_SUPPORT_WR_COMPLETION	BIT(3)
> +#define CQSPI_NEEDS_APB_AHB_HAZARD_WAR	BIT(4)
>  
>  /* Capabilities */
>  #define CQSPI_SUPPORTS_OCTAL		BIT(0)
> @@ -87,6 +88,7 @@ struct cqspi_st {
>  	bool			use_dma_read;
>  	u32			pd_dev_id;
>  	bool			wr_completion;
> +	bool			apb_ahb_hazard;
>  };
>  
>  struct cqspi_driver_platdata {
> @@ -952,6 +954,13 @@ static int cqspi_indirect_write_execute(struct cqspi_flash_pdata *f_pdata,
>  	if (cqspi->wr_delay)
>  		ndelay(cqspi->wr_delay);
>  
> +	/*
> +	 * If a hazard exists between the APB and AHB interfaces, perform a
> +	 * dummy readback from the controller to ensure synchronization.
> +	 */

This is needed for TI's SoCs as well. APB and AHB accesses are 
independent of each other on the interconnect and can be racy. I wrote a 
couple patches [0][1] to fix this on TI's fork. I never got around to 
sending them upstream. It would be great if you can pick those up. They 
fix the race in all paths, not just indirect write.

I would also prefer if we do this unconditionally. I don't think it has 
much downside even on platforms that do not strictly need this.

[0] https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/drivers/spi/spi-cadence-quadspi.c?h=ti-linux-5.10.y&id=027f03a8512086e5ef05dc4e4ff53b2628848f95
[1] https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/drivers/spi/spi-cadence-quadspi.c?h=ti-linux-5.10.y&id=4c367e58bab7d3f9c470c3778441f73546f20398

> +	if (cqspi->apb_ahb_hazard)
> +		(void)readl(reg_base + CQSPI_REG_INDIRECTWR);
> +
>  	while (remaining > 0) {
>  		size_t write_words, mod_bytes;
>  
> @@ -1667,6 +1676,8 @@ static int cqspi_probe(struct platform_device *pdev)
>  			cqspi->use_dma_read = true;
>  		if (ddata->quirks & CQSPI_NO_SUPPORT_WR_COMPLETION)
>  			cqspi->wr_completion = false;
> +		if (ddata->quirks & CQSPI_NEEDS_APB_AHB_HAZARD_WAR)
> +			cqspi->apb_ahb_hazard = true;
>  
>  		if (of_device_is_compatible(pdev->dev.of_node,
>  					    "xlnx,versal-ospi-1.0"))
> @@ -1789,6 +1800,10 @@ static const struct cqspi_driver_platdata versal_ospi = {
>  	.get_dma_status = cqspi_get_versal_dma_status,
>  };
>  
> +static const struct cqspi_driver_platdata pen_cdns_qspi = {
> +	.quirks = CQSPI_NEEDS_APB_AHB_HAZARD_WAR | CQSPI_DISABLE_DAC_MODE,
> +};
> +
>  static const struct of_device_id cqspi_dt_ids[] = {
>  	{
>  		.compatible = "cdns,qspi-nor",
> @@ -1814,6 +1829,10 @@ static const struct of_device_id cqspi_dt_ids[] = {
>  		.compatible = "intel,socfpga-qspi",
>  		.data = &socfpga_qspi,
>  	},
> +	{
> +		.compatible = "amd,pensando-elba-qspi",
> +		.data = &pen_cdns_qspi,
> +	},
>  	{ /* end of table */ }
>  };
>  
> -- 
> 2.17.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v5 11/15] spi: cadence-quadspi: Add compatible for AMD Pensando Elba SoC

[Brad Larson]

Hi Pratyush,

On Tue, Jun 14, 2022 at 1:49 AM Pratyush Yadav <p.yadav@ti.com> wrote:

> This is needed for TI's SoCs as well. APB and AHB accesses are
> independent of each other on the interconnect and can be racy. I wrote a
> couple patches [0][1] to fix this on TI's fork. I never got around to
> sending them upstream. It would be great if you can pick those up. They
> fix the race in all paths, not just indirect write.
>
> I would also prefer if we do this unconditionally. I don't think it has
> much downside even on platforms that do not strictly need this.
>
> [0] https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/drivers/spi/spi-cadence-quadspi.c?h=ti-linux-5.10.y&id=027f03a8512086e5ef05dc4e4ff53b2628848f95
> [1] https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/drivers/spi/spi-cadence-quadspi.c?h=ti-linux-5.10.y&id=4c367e58bab7d3f9c470c3778441f73546f20398

Let's get Elba specific support in first and then in a separate patch
go for the unconditional.  An extra op for devices for which its not
currently done will result in questions I can't answer.

Regards,
Brad

Re: [PATCH v5 11/15] spi: cadence-quadspi: Add compatible for AMD Pensando Elba SoC

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 335 bytes --]

On Mon, Jun 13, 2022 at 12:56:54PM -0700, Brad Larson wrote:

> +	/*
> +	 * If a hazard exists between the APB and AHB interfaces, perform a
> +	 * dummy readback from the controller to ensure synchronization.
> +	 */
> +	if (cqspi->apb_ahb_hazard)
> +		(void)readl(reg_base + CQSPI_REG_INDIRECTWR);

You shouldn't need the cast here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 11/15] spi: cadence-quadspi: Add compatible for AMD Pensando Elba SoC

[Brad Larson]

Hi Mark,

On Tue, Jun 14, 2022 at 5:01 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Jun 13, 2022 at 12:56:54PM -0700, Brad Larson wrote:
>
> > +     /*
> > +      * If a hazard exists between the APB and AHB interfaces, perform a
> > +      * dummy readback from the controller to ensure synchronization.
> > +      */
> > +     if (cqspi->apb_ahb_hazard)
> > +             (void)readl(reg_base + CQSPI_REG_INDIRECTWR);
>
> You shouldn't need the cast here.

Removed (void) cast

Regards,
Brad

[PATCH v5 12/15] spi: dw: Add support for AMD Pensando Elba SoC

[Brad Larson]

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.

Signed-off-by: Brad Larson <blarson@amd.com>
---
 drivers/spi/spi-dw-mmio.c | 66 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index 5101c4c6017b..6b7a557759bd 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -53,6 +53,24 @@ struct dw_spi_mscc {
 	void __iomem        *spi_mst; /* Not sparx5 */
 };
 
+struct dw_spi_elba {
+	struct regmap *syscon;
+};
+
+/*
+ * 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_REG			0x2468
+#define ELBA_SPICS_SHIFT(cs)		(2 * (cs))
+#define ELBA_SPICS_MASK(cs)		(0x3 << ELBA_SPICS_SHIFT(cs))
+#define ELBA_SPICS_SET(cs, val)	\
+			((((val) << 1) | 0x1) << ELBA_SPICS_SHIFT(cs))
+
 /*
  * The Designware SPI controller (referred to as master in the documentation)
  * automatically deasserts chip select when the tx fifo is empty. The chip
@@ -238,6 +256,53 @@ static int dw_spi_canaan_k210_init(struct platform_device *pdev,
 	return 0;
 }
 
+static void dw_spi_elba_override_cs(struct dw_spi_elba *dwselba, int cs, int enable)
+{
+	regmap_update_bits(dwselba->syscon, ELBA_SPICS_REG, ELBA_SPICS_MASK(cs),
+			   ELBA_SPICS_SET(cs, enable));
+}
+
+static void dw_spi_elba_set_cs(struct spi_device *spi, bool enable)
+{
+	struct dw_spi *dws = spi_master_get_devdata(spi->master);
+	struct dw_spi_mmio *dwsmmio = container_of(dws, struct dw_spi_mmio, dws);
+	struct dw_spi_elba *dwselba = dwsmmio->priv;
+	u8 cs;
+
+	cs = spi->chip_select;
+	if (cs < 2)
+		dw_spi_elba_override_cs(dwselba, spi->chip_select, enable);
+
+	/*
+	 * The DW SPI controller needs a native CS bit selected to start
+	 * the serial engine.
+	 */
+	spi->chip_select = 0;
+	dw_spi_set_cs(spi, enable);
+	spi->chip_select = cs;
+}
+
+static int dw_spi_elba_init(struct platform_device *pdev,
+			    struct dw_spi_mmio *dwsmmio)
+{
+	struct dw_spi_elba *dwselba;
+	struct regmap *regmap;
+
+	regmap = syscon_regmap_lookup_by_compatible("amd,pensando-elba-syscon");
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	dwselba = devm_kzalloc(&pdev->dev, sizeof(*dwselba), GFP_KERNEL);
+	if (!dwselba)
+		return -ENOMEM;
+	dwselba->syscon = regmap;
+
+	dwsmmio->priv = dwselba;
+	dwsmmio->dws.set_cs = dw_spi_elba_set_cs;
+
+	return 0;
+}
+
 static int dw_spi_mmio_probe(struct platform_device *pdev)
 {
 	int (*init_func)(struct platform_device *pdev,
@@ -352,6 +417,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
 	{ .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init},
 	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
 	{ .compatible = "canaan,k210-spi", dw_spi_canaan_k210_init},
+	{ .compatible = "amd,pensando-elba-spi", .data = dw_spi_elba_init},
 	{ /* end of table */}
 };
 MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match);
-- 
2.17.1

Re: [PATCH v5 12/15] spi: dw: Add support for AMD Pensando Elba SoC

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 12/15] spi: dw: Add support for AMD Pensando Elba SoC

[Brad Larson]

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

[PATCH v5 13/15] mmc: sdhci-cadence: Add AMD Pensando Elba SoC support

[Brad Larson]

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
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.

Signed-off-by: Brad Larson <blarson@amd.com>
---
 drivers/mmc/host/Kconfig         |   1 +
 drivers/mmc/host/sdhci-cadence.c | 179 ++++++++++++++++++++++++++++---
 2 files changed, 166 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index d6144978e32d..d0a66a74532e 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -243,6 +243,7 @@ config MMC_SDHCI_CADENCE
 	tristate "SDHCI support for the Cadence SD/SDIO/eMMC controller"
 	depends on MMC_SDHCI_PLTFM
 	depends on OF
+	select MMC_SDHCI_IO_ACCESSORS
 	help
 	  This selects the Cadence SD/SDIO/eMMC driver.
 
diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
index 6f2de54a5987..08253357535a 100644
--- a/drivers/mmc/host/sdhci-cadence.c
+++ b/drivers/mmc/host/sdhci-cadence.c
@@ -12,6 +12,7 @@
 #include <linux/mmc/mmc.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/reset.h>
 
 #include "sdhci-pltfm.h"
 
@@ -66,7 +67,12 @@ struct sdhci_cdns_phy_param {
 
 struct sdhci_cdns_priv {
 	void __iomem *hrs_addr;
+	void __iomem *ctl_addr;	/* write control */
+	spinlock_t wrlock;	/* write lock */
 	bool enhanced_strobe;
+	void (*priv_write_l)(struct sdhci_cdns_priv *priv, u32 val,
+			     void __iomem *reg);
+	struct reset_control *rst_hw;
 	unsigned int nr_phy_params;
 	struct sdhci_cdns_phy_param phy_params[];
 };
@@ -76,6 +82,11 @@ struct sdhci_cdns_phy_cfg {
 	u8 addr;
 };
 
+struct sdhci_cdns_drv_data {
+	int (*init)(struct platform_device *pdev);
+	const struct sdhci_pltfm_data pltfm_data;
+};
+
 static const struct sdhci_cdns_phy_cfg sdhci_cdns_phy_cfgs[] = {
 	{ "cdns,phy-input-delay-sd-highspeed", SDHCI_CDNS_PHY_DLY_SD_HS, },
 	{ "cdns,phy-input-delay-legacy", SDHCI_CDNS_PHY_DLY_SD_DEFAULT, },
@@ -90,6 +101,15 @@ static const struct sdhci_cdns_phy_cfg sdhci_cdns_phy_cfgs[] = {
 	{ "cdns,phy-dll-delay-strobe", SDHCI_CDNS_PHY_DLY_STROBE, },
 };
 
+static inline void sdhci_cdns_priv_writel(struct sdhci_cdns_priv *priv,
+					  u32 val, void __iomem *reg)
+{
+	if (unlikely(priv->priv_write_l))
+		priv->priv_write_l(priv, val, reg);
+	else
+		writel(val, reg);
+}
+
 static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
 				    u8 addr, u8 data)
 {
@@ -104,17 +124,17 @@ static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
 
 	tmp = FIELD_PREP(SDHCI_CDNS_HRS04_WDATA, data) |
 	      FIELD_PREP(SDHCI_CDNS_HRS04_ADDR, addr);
-	writel(tmp, reg);
+	sdhci_cdns_priv_writel(priv, tmp, reg);
 
 	tmp |= SDHCI_CDNS_HRS04_WR;
-	writel(tmp, reg);
+	sdhci_cdns_priv_writel(priv, tmp, reg);
 
 	ret = readl_poll_timeout(reg, tmp, tmp & SDHCI_CDNS_HRS04_ACK, 0, 10);
 	if (ret)
 		return ret;
 
 	tmp &= ~SDHCI_CDNS_HRS04_WR;
-	writel(tmp, reg);
+	sdhci_cdns_priv_writel(priv, tmp, reg);
 
 	ret = readl_poll_timeout(reg, tmp, !(tmp & SDHCI_CDNS_HRS04_ACK),
 				 0, 10);
@@ -191,7 +211,7 @@ static void sdhci_cdns_set_emmc_mode(struct sdhci_cdns_priv *priv, u32 mode)
 	tmp = readl(priv->hrs_addr + SDHCI_CDNS_HRS06);
 	tmp &= ~SDHCI_CDNS_HRS06_MODE;
 	tmp |= FIELD_PREP(SDHCI_CDNS_HRS06_MODE, mode);
-	writel(tmp, priv->hrs_addr + SDHCI_CDNS_HRS06);
+	sdhci_cdns_priv_writel(priv, tmp, priv->hrs_addr + SDHCI_CDNS_HRS06);
 }
 
 static u32 sdhci_cdns_get_emmc_mode(struct sdhci_cdns_priv *priv)
@@ -223,7 +243,7 @@ static int sdhci_cdns_set_tune_val(struct sdhci_host *host, unsigned int val)
 	 */
 	for (i = 0; i < 2; i++) {
 		tmp |= SDHCI_CDNS_HRS06_TUNE_UP;
-		writel(tmp, reg);
+		sdhci_cdns_priv_writel(priv, tmp, reg);
 
 		ret = readl_poll_timeout(reg, tmp,
 					 !(tmp & SDHCI_CDNS_HRS06_TUNE_UP),
@@ -309,6 +329,89 @@ static void sdhci_cdns_set_uhs_signaling(struct sdhci_host *host,
 		sdhci_set_uhs_signaling(host, timing);
 }
 
+/*
+ * The Pensando Elba SoC explicitly controls byte-lane enables on writes
+ * which includes writes to the HRS registers.
+ */
+static void elba_priv_write_l(struct sdhci_cdns_priv *priv, u32 val,
+			      void __iomem *reg)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&priv->wrlock, flags);
+	writel(0x78, priv->ctl_addr);
+	writel(val, reg);
+	spin_unlock_irqrestore(&priv->wrlock, flags);
+}
+
+static void elba_write_l(struct sdhci_host *host, u32 val, int reg)
+{
+	elba_priv_write_l(sdhci_cdns_priv(host), val, host->ioaddr + reg);
+}
+
+static void elba_write_w(struct sdhci_host *host, u16 val, int reg)
+{
+	struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
+	unsigned long flags;
+	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);
+}
+
+static void elba_write_b(struct sdhci_host *host, u8 val, int reg)
+{
+	struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
+	unsigned long flags;
+	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);
+}
+
+static const struct sdhci_ops sdhci_elba_ops = {
+	.write_l = elba_write_l,
+	.write_w = elba_write_w,
+	.write_b = elba_write_b,
+	.set_clock = sdhci_set_clock,
+	.get_timeout_clock = sdhci_cdns_get_timeout_clock,
+	.set_bus_width = sdhci_set_bus_width,
+	.reset = sdhci_reset,
+	.set_uhs_signaling = sdhci_cdns_set_uhs_signaling,
+};
+
+static int elba_drv_init(struct platform_device *pdev)
+{
+	struct sdhci_host *host = platform_get_drvdata(pdev);
+	struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
+	struct resource *iomem;
+	void __iomem *ioaddr;
+
+	host->mmc->caps |= (MMC_CAP_1_8V_DDR | MMC_CAP_8_BIT_DATA);
+
+	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!iomem)
+		return -ENOMEM;
+
+	/* Byte-lane control register */
+	ioaddr = devm_platform_ioremap_resource(pdev, 1);
+	if (IS_ERR(ioaddr))
+		return PTR_ERR(ioaddr);
+
+	priv->ctl_addr = ioaddr;
+	priv->priv_write_l = elba_priv_write_l;
+	spin_lock_init(&priv->wrlock);
+	writel(0x78, priv->ctl_addr);
+
+	return 0;
+}
+
 static const struct sdhci_ops sdhci_cdns_ops = {
 	.set_clock = sdhci_set_clock,
 	.get_timeout_clock = sdhci_cdns_get_timeout_clock,
@@ -318,15 +421,27 @@ static const struct sdhci_ops sdhci_cdns_ops = {
 	.set_uhs_signaling = sdhci_cdns_set_uhs_signaling,
 };
 
-static const struct sdhci_pltfm_data sdhci_cdns_uniphier_pltfm_data = {
-	.ops = &sdhci_cdns_ops,
-	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
+static const struct sdhci_cdns_drv_data sdhci_cdns_uniphier_drv_data = {
+	.pltfm_data = {
+		.ops = &sdhci_cdns_ops,
+		.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
+	},
 };
 
-static const struct sdhci_pltfm_data sdhci_cdns_pltfm_data = {
-	.ops = &sdhci_cdns_ops,
+static const struct sdhci_cdns_drv_data sdhci_elba_drv_data = {
+	.init = elba_drv_init,
+	.pltfm_data = {
+		.ops = &sdhci_elba_ops,
+	},
 };
 
+static const struct sdhci_cdns_drv_data sdhci_cdns_drv_data = {
+	.pltfm_data = {
+		.ops = &sdhci_cdns_ops,
+	},
+};
+
+
 static void sdhci_cdns_hs400_enhanced_strobe(struct mmc_host *mmc,
 					     struct mmc_ios *ios)
 {
@@ -347,10 +462,26 @@ static void sdhci_cdns_hs400_enhanced_strobe(struct mmc_host *mmc,
 					 SDHCI_CDNS_HRS06_MODE_MMC_HS400);
 }
 
+static void sdhci_mmc_hw_reset(struct mmc_host *mmc)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
+
+	dev_info(mmc_dev(host->mmc), "emmc hardware reset\n");
+
+	reset_control_assert(priv->rst_hw);
+	/* For eMMC, minimum is 1us but give it 9us for good measure */
+	udelay(9);
+
+	reset_control_deassert(priv->rst_hw);
+	/* For eMMC, minimum is 200us but give it 300us for good measure */
+	usleep_range(300, 1000);
+}
+
 static int sdhci_cdns_probe(struct platform_device *pdev)
 {
 	struct sdhci_host *host;
-	const struct sdhci_pltfm_data *data;
+	const struct sdhci_cdns_drv_data *data;
 	struct sdhci_pltfm_host *pltfm_host;
 	struct sdhci_cdns_priv *priv;
 	struct clk *clk;
@@ -369,10 +500,10 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
 
 	data = of_device_get_match_data(dev);
 	if (!data)
-		data = &sdhci_cdns_pltfm_data;
+		data = &sdhci_cdns_drv_data;
 
 	nr_phy_params = sdhci_cdns_phy_param_count(dev->of_node);
-	host = sdhci_pltfm_init(pdev, data,
+	host = sdhci_pltfm_init(pdev, &data->pltfm_data,
 				struct_size(priv, phy_params, nr_phy_params));
 	if (IS_ERR(host)) {
 		ret = PTR_ERR(host);
@@ -389,6 +520,11 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
 	host->ioaddr += SDHCI_CDNS_SRS_BASE;
 	host->mmc_host_ops.hs400_enhanced_strobe =
 				sdhci_cdns_hs400_enhanced_strobe;
+	if (data->init) {
+		ret = data->init(pdev);
+		if (ret)
+			goto free;
+	}
 	sdhci_enable_v4_mode(host);
 	__sdhci_read_caps(host, &version, NULL, NULL);
 
@@ -404,6 +540,17 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
 	if (ret)
 		goto free;
 
+	if (host->mmc->caps & MMC_CAP_HW_RESET) {
+		priv->rst_hw = devm_reset_control_get_optional_exclusive(dev, "hw");
+		if (IS_ERR(priv->rst_hw)) {
+			ret = PTR_ERR(priv->rst_hw);
+			if (ret == -ENOENT)
+				priv->rst_hw = NULL;
+		} else {
+			host->mmc_host_ops.card_hw_reset = sdhci_mmc_hw_reset;
+		}
+	}
+
 	ret = sdhci_add_host(host);
 	if (ret)
 		goto free;
@@ -453,7 +600,11 @@ static const struct dev_pm_ops sdhci_cdns_pm_ops = {
 static const struct of_device_id sdhci_cdns_match[] = {
 	{
 		.compatible = "socionext,uniphier-sd4hc",
-		.data = &sdhci_cdns_uniphier_pltfm_data,
+		.data = &sdhci_cdns_uniphier_drv_data,
+	},
+	{
+		.compatible = "amd,pensando-elba-sd4hc",
+		.data = &sdhci_elba_drv_data
 	},
 	{ .compatible = "cdns,sd4hc" },
 	{ /* sentinel */ }
-- 
2.17.1

Re: [PATCH v5 13/15] mmc: sdhci-cadence: Add AMD Pensando Elba SoC support

[Andy Shevchenko]

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.

>         },

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 13/15] mmc: sdhci-cadence: Add AMD Pensando Elba SoC support

[Brad Larson]

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

[PATCH v5 14/15] mfd: pensando-elbasr: Add AMD Pensando Elba System Resource chip

[Brad Larson]

From: Brad Larson <blarson@amd.com>

Add support for the AMD Pensando Elba SoC System Resource chip
using the SPI interface.  The Elba SR is a Multi-function Device
supporting device register access using CS0, smbus interface for
FRU and board peripherals using CS1, dual Lattice I2C masters for
transceiver management using CS2, and CS3 for flash access.

Signed-off-by: Brad Larson <blarson@amd.com>
---
 drivers/mfd/Kconfig                 |  14 +
 drivers/mfd/Makefile                |   1 +
 drivers/mfd/pensando-elbasr.c       | 862 ++++++++++++++++++++++++++++
 include/linux/mfd/pensando-elbasr.h |  78 +++
 4 files changed, 955 insertions(+)
 create mode 100644 drivers/mfd/pensando-elbasr.c
 create mode 100644 include/linux/mfd/pensando-elbasr.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 3b59456f5545..c5e10d302586 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1050,6 +1050,20 @@ config UCB1400_CORE
 	  To compile this driver as a module, choose M here: the
 	  module will be called ucb1400_core.
 
+config MFD_PENSANDO_ELBASR
+	bool "AMD Pensando Elba System Resource chip"
+	depends on SPI_MASTER=y
+	depends on (ARCH_PENSANDO && OF) || COMPILE_TEST
+	select REGMAP_SPI
+	select MFD_CORE
+	select MFD_SYSCON
+	help
+	  Support for the AMD Pensando Elba SoC System Resource chip using the
+	  SPI interface.  This driver provides userspace access to four device
+	  functions to include CS0 device registers, CS1 smbus interface for
+	  FRU and board peripherals, CS2 dual Lattice I2C masters for
+	  transceiver management, and CS3 flash for firmware update.
+
 config MFD_PM8XXX
 	tristate "Qualcomm PM8xxx PMIC chips driver"
 	depends on (ARM || HEXAGON || COMPILE_TEST)
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 858cacf659d6..917b128abe5b 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -212,6 +212,7 @@ obj-$(CONFIG_MFD_INTEL_LPSS_PCI)	+= intel-lpss-pci.o
 obj-$(CONFIG_MFD_INTEL_LPSS_ACPI)	+= intel-lpss-acpi.o
 obj-$(CONFIG_MFD_INTEL_PMC_BXT)	+= intel_pmc_bxt.o
 obj-$(CONFIG_MFD_PALMAS)	+= palmas.o
+obj-$(CONFIG_MFD_PENSANDO_ELBASR)	+= pensando-elbasr.o
 obj-$(CONFIG_MFD_VIPERBOARD)    += viperboard.o
 obj-$(CONFIG_MFD_NTXEC)		+= ntxec.o
 obj-$(CONFIG_MFD_RC5T583)	+= rc5t583.o rc5t583-irq.o
diff --git a/drivers/mfd/pensando-elbasr.c b/drivers/mfd/pensando-elbasr.c
new file mode 100644
index 000000000000..f689f68f5377
--- /dev/null
+++ b/drivers/mfd/pensando-elbasr.c
@@ -0,0 +1,862 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * AMD Pensando Elba System Resource MFD Driver
+ *
+ * Userspace interface and reset driver support for SPI connected
+ * Pensando Elba System Resource Chip.
+ *
+ * Adapted from spidev.c
+ *
+ * Copyright (C) 2006 SWAPP
+ *	Andrea Paterniani <a.paterniani@swapp-eng.it>
+ * Copyright (C) 2007 David Brownell (simplification, cleanup)
+ * Copyright (C) 2022 AMD Pensando
+ */
+
+#include <linux/mfd/pensando-elbasr.h>
+#include <linux/mfd/core.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/ioctl.h>
+#include <linux/fs.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/list.h>
+#include <linux/errno.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/compat.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spidev.h>
+#include <linux/delay.h>
+
+#define ELBASR_SPI_CMD_REGRD	0x0b
+#define ELBASR_SPI_CMD_REGWR	0x02
+#define ELBASR_MAX_DEVS		4
+
+/* The main reason to have this class is to make mdev/udev create the
+ * /dev/pensrB.C character device nodes exposing our userspace API.
+ * It also simplifies memory management.  The device nodes
+ * /dev/pensrB.C are common across Pensando boards.
+ */
+static struct class *elbasr_class;
+
+static dev_t elbasr_devt;
+static DECLARE_BITMAP(minors, ELBASR_MAX_DEVS);
+static unsigned int bufsiz = 4096;
+
+static LIST_HEAD(device_list);
+static DEFINE_MUTEX(device_list_lock);
+
+static const struct mfd_cell pensando_elbasr_subdev_info[] = {
+	{
+		.name = "pensando_elbasr_reset",
+		.of_compatible = "amd,pensando-elbasr-reset",
+	},
+};
+
+/* Bit masks for spi_device.mode management.  Note that incorrect
+ * settings for some settings can cause *lots* of trouble for other
+ * devices on a shared bus:
+ *
+ *  - CS_HIGH ... this device will be active when it shouldn't be
+ *  - 3WIRE ... when active, it won't behave as it should
+ *  - NO_CS ... there will be no explicit message boundaries; this
+ *	is completely incompatible with the shared bus model
+ *  - READY ... transfers may proceed when they shouldn't.
+ */
+#define SPI_MODE_MASK		(SPI_CPHA | SPI_CPOL | SPI_CS_HIGH \
+				| SPI_LSB_FIRST | SPI_3WIRE | SPI_LOOP \
+				| SPI_NO_CS | SPI_READY | SPI_TX_DUAL \
+				| SPI_TX_QUAD | SPI_TX_OCTAL | SPI_RX_DUAL \
+				| SPI_RX_QUAD | SPI_RX_OCTAL)
+
+static ssize_t
+elbasr_spi_sync(struct elbasr_data *elbasr_spi, struct spi_message *message)
+{
+	int status;
+	struct spi_device *spi;
+
+	spin_lock_irq(&elbasr_spi->spi_lock);
+	spi = elbasr_spi->spi;
+	spin_unlock_irq(&elbasr_spi->spi_lock);
+
+	if (spi == NULL)
+		status = -ESHUTDOWN;
+	else
+		status = spi_sync(spi, message);
+
+	if (status == 0)
+		status = message->actual_length;
+
+	return status;
+}
+
+static inline ssize_t
+elbasr_spi_sync_write(struct elbasr_data *elbasr, size_t len)
+{
+	struct spi_transfer	t = {
+			.tx_buf		= elbasr->tx_buffer,
+			.len		= len,
+			.speed_hz	= elbasr->speed_hz,
+		};
+	struct spi_message	m;
+
+	spi_message_init(&m);
+	spi_message_add_tail(&t, &m);
+	return elbasr_spi_sync(elbasr, &m);
+}
+
+static inline ssize_t
+elbasr_spi_sync_read(struct elbasr_data *elbasr, size_t len)
+{
+	struct spi_transfer	t = {
+			.rx_buf		= elbasr->rx_buffer,
+			.len		= len,
+			.speed_hz	= elbasr->speed_hz,
+		};
+	struct spi_message	m;
+
+	spi_message_init(&m);
+	spi_message_add_tail(&t, &m);
+	return elbasr_spi_sync(elbasr, &m);
+}
+
+/* Read-only message with current device setup */
+static ssize_t
+elbasr_spi_read(struct file *filp, char __user *buf, size_t count, loff_t *f_pos)
+{
+	struct elbasr_data *elbasr;
+	ssize_t status;
+
+	/* chipselect only toggles at start or end of operation */
+	if (count > bufsiz)
+		return -EMSGSIZE;
+
+	elbasr = filp->private_data;
+
+	mutex_lock(&elbasr->buf_lock);
+	status = elbasr_spi_sync_read(elbasr, count);
+	if (status > 0) {
+		unsigned long missing;
+
+		missing = copy_to_user(buf, elbasr->rx_buffer, status);
+		if (missing == status)
+			status = -EFAULT;
+		else
+			status = status - missing;
+	}
+	mutex_unlock(&elbasr->buf_lock);
+
+	return status;
+}
+
+/* Write-only message with current device setup */
+static ssize_t
+elbasr_spi_write(struct file *filp, const char __user *buf,
+		 size_t count, loff_t *f_pos)
+{
+	struct elbasr_data *elbasr;
+	ssize_t status;
+	unsigned long missing;
+
+	/* chipselect only toggles at start or end of operation */
+	if (count > bufsiz)
+		return -EMSGSIZE;
+
+	elbasr = filp->private_data;
+
+	mutex_lock(&elbasr->buf_lock);
+	missing = copy_from_user(elbasr->tx_buffer, buf, count);
+	if (missing == 0)
+		status = elbasr_spi_sync_write(elbasr, count);
+	else
+		status = -EFAULT;
+	mutex_unlock(&elbasr->buf_lock);
+
+	return status;
+}
+
+static int elbasr_spi_message(struct elbasr_data *elbasr,
+			      struct spi_ioc_transfer *u_xfers,
+			      unsigned int n_xfers)
+{
+	struct spi_message msg;
+	struct spi_transfer *k_xfers;
+	struct spi_transfer *k_tmp;
+	struct spi_ioc_transfer *u_tmp;
+	unsigned int n, total, tx_total, rx_total;
+	u8 *tx_buf, *rx_buf;
+	int status = -EFAULT;
+
+	spi_message_init(&msg);
+	k_xfers = kcalloc(n_xfers, sizeof(*k_tmp), GFP_KERNEL);
+	if (k_xfers == NULL)
+		return -ENOMEM;
+
+	/* Construct spi_message, copying any tx data to bounce buffer.
+	 * We walk the array of user-provided transfers, using each one
+	 * to initialize a kernel version of the same transfer.
+	 */
+	tx_buf = elbasr->tx_buffer;
+	rx_buf = elbasr->rx_buffer;
+	total = 0;
+	tx_total = 0;
+	rx_total = 0;
+	for (n = n_xfers, k_tmp = k_xfers, u_tmp = u_xfers;
+			n;
+			n--, k_tmp++, u_tmp++) {
+		/* Ensure that also following allocations from rx_buf/tx_buf will meet
+		 * DMA alignment requirements.
+		 */
+		unsigned int len_aligned = ALIGN(u_tmp->len,
+						 ARCH_KMALLOC_MINALIGN);
+
+		k_tmp->len = u_tmp->len;
+
+		total += k_tmp->len;
+		/* Since the function returns the total length of transfers
+		 * on success, restrict the total to positive int values to
+		 * avoid the return value looking like an error.  Also check
+		 * each transfer length to avoid arithmetic overflow.
+		 */
+		if (total > INT_MAX || k_tmp->len > INT_MAX) {
+			status = -EMSGSIZE;
+			goto done;
+		}
+
+		if (u_tmp->rx_buf) {
+			/* this transfer needs space in RX bounce buffer */
+			rx_total += len_aligned;
+			if (rx_total > bufsiz) {
+				status = -EMSGSIZE;
+				goto done;
+			}
+			k_tmp->rx_buf = rx_buf;
+			rx_buf += len_aligned;
+		}
+		if (u_tmp->tx_buf) {
+			/* this transfer needs space in TX bounce buffer */
+			tx_total += len_aligned;
+			if (tx_total > bufsiz) {
+				status = -EMSGSIZE;
+				goto done;
+			}
+			k_tmp->tx_buf = tx_buf;
+			if (copy_from_user(tx_buf, (const u8 __user *)
+						(uintptr_t) u_tmp->tx_buf,
+					u_tmp->len))
+				goto done;
+			tx_buf += len_aligned;
+		}
+
+		k_tmp->cs_change = !!u_tmp->cs_change;
+		k_tmp->tx_nbits = u_tmp->tx_nbits;
+		k_tmp->rx_nbits = u_tmp->rx_nbits;
+		k_tmp->bits_per_word = u_tmp->bits_per_word;
+		k_tmp->delay.value = u_tmp->delay_usecs;
+		k_tmp->delay.unit = SPI_DELAY_UNIT_USECS;
+		k_tmp->speed_hz = u_tmp->speed_hz;
+		k_tmp->word_delay.value = u_tmp->word_delay_usecs;
+		k_tmp->word_delay.unit = SPI_DELAY_UNIT_USECS;
+		if (!k_tmp->speed_hz)
+			k_tmp->speed_hz = elbasr->speed_hz;
+#ifdef VERBOSE
+		dev_dbg(&elbasr->spi->dev,
+			" xfer len %u %s%s%s%dbits %u usec %u usec %uHz (%u)\n",
+			k_tmp->len,
+			k_tmp->rx_buf ? "rx " : "",
+			k_tmp->tx_buf ? "tx " : "",
+			k_tmp->cs_change ? "cs " : "",
+			k_tmp->bits_per_word ? : elbasr->spi->bits_per_word,
+			k_tmp->delay.value,
+			k_tmp->word_delay.value,
+			k_tmp->speed_hz ? : elbasr->spi->max_speed_hz);
+#endif
+		spi_message_add_tail(k_tmp, &msg);
+	}
+
+	status = elbasr_spi_sync(elbasr, &msg);
+	if (status < 0)
+		goto done;
+
+	/* copy any rx data out of bounce buffer */
+	for (n = n_xfers, k_tmp = k_xfers, u_tmp = u_xfers;
+			n;
+			n--, k_tmp++, u_tmp++) {
+		if (u_tmp->rx_buf) {
+			if (copy_to_user((u8 __user *)
+					(uintptr_t) u_tmp->rx_buf, k_tmp->rx_buf,
+					u_tmp->len)) {
+				status = -EFAULT;
+				goto done;
+			}
+		}
+	}
+	status = total;
+
+done:
+	kfree(k_xfers);
+	return status;
+}
+
+static struct spi_ioc_transfer *
+elbasr_spi_get_ioc_message(unsigned int cmd,
+			   struct spi_ioc_transfer __user *u_ioc,
+			   unsigned int *n_ioc)
+{
+	u32 tmp;
+
+	/* Check type, command number and direction */
+	if (_IOC_TYPE(cmd) != SPI_IOC_MAGIC
+			|| _IOC_NR(cmd) != _IOC_NR(SPI_IOC_MESSAGE(0))
+			|| _IOC_DIR(cmd) != _IOC_WRITE)
+		return ERR_PTR(-ENOTTY);
+
+	tmp = _IOC_SIZE(cmd);
+	if ((tmp % sizeof(struct spi_ioc_transfer)) != 0)
+		return ERR_PTR(-EINVAL);
+	*n_ioc = tmp / sizeof(struct spi_ioc_transfer);
+	if (*n_ioc == 0)
+		return NULL;
+
+	/* copy into scratch area */
+	return memdup_user(u_ioc, tmp);
+}
+
+static long
+elbasr_spi_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	int retval = 0;
+	struct elbasr_data *elbasr;
+	struct spi_device *spi;
+	u32 tmp;
+	unsigned int n_ioc;
+	struct spi_ioc_transfer	*ioc;
+
+	/* Check type and command number */
+	if (_IOC_TYPE(cmd) != SPI_IOC_MAGIC)
+		return -ENOTTY;
+
+	/* guard against device removal before, or while,
+	 * we issue this ioctl.
+	 */
+	elbasr = filp->private_data;
+	spin_lock_irq(&elbasr->spi_lock);
+	spi = spi_dev_get(elbasr->spi);
+	spin_unlock_irq(&elbasr->spi_lock);
+
+	if (spi == NULL)
+		return -ESHUTDOWN;
+
+	/* use the buffer lock here for triple duty:
+	 *  - prevent I/O (from us) so calling spi_setup() is safe;
+	 *  - prevent concurrent SPI_IOC_WR_* from morphing
+	 *    data fields while SPI_IOC_RD_* reads them;
+	 *  - SPI_IOC_MESSAGE needs the buffer locked "normally".
+	 */
+	mutex_lock(&elbasr->buf_lock);
+
+	switch (cmd) {
+	/* read requests */
+	case SPI_IOC_RD_MODE:
+		retval = put_user(spi->mode & SPI_MODE_MASK,
+					(__u8 __user *)arg);
+		break;
+	case SPI_IOC_RD_MODE32:
+		retval = put_user(spi->mode & SPI_MODE_MASK,
+					(__u32 __user *)arg);
+		break;
+	case SPI_IOC_RD_LSB_FIRST:
+		retval = put_user((spi->mode & SPI_LSB_FIRST) ?  1 : 0,
+					(__u8 __user *)arg);
+		break;
+	case SPI_IOC_RD_BITS_PER_WORD:
+		retval = put_user(spi->bits_per_word, (__u8 __user *)arg);
+		break;
+	case SPI_IOC_RD_MAX_SPEED_HZ:
+		retval = put_user(elbasr->speed_hz, (__u32 __user *)arg);
+		break;
+
+	/* write requests */
+	case SPI_IOC_WR_MODE:
+	case SPI_IOC_WR_MODE32:
+		if (cmd == SPI_IOC_WR_MODE)
+			retval = get_user(tmp, (u8 __user *)arg);
+		else
+			retval = get_user(tmp, (u32 __user *)arg);
+		if (retval == 0) {
+			struct spi_controller *ctlr = spi->controller;
+			u32	save = spi->mode;
+
+			if (tmp & ~SPI_MODE_MASK) {
+				retval = -EINVAL;
+				break;
+			}
+
+			if (ctlr->use_gpio_descriptors && ctlr->cs_gpiods &&
+			    ctlr->cs_gpiods[spi->chip_select])
+				tmp |= SPI_CS_HIGH;
+
+			tmp |= spi->mode & ~SPI_MODE_MASK;
+			spi->mode = (u16)tmp;
+			retval = spi_setup(spi);
+			if (retval < 0)
+				spi->mode = save;
+			else
+				dev_dbg(&spi->dev, "spi mode %x\n", tmp);
+		}
+		break;
+	case SPI_IOC_WR_LSB_FIRST:
+		retval = get_user(tmp, (__u8 __user *)arg);
+		if (retval == 0) {
+			u32	save = spi->mode;
+
+			if (tmp)
+				spi->mode |= SPI_LSB_FIRST;
+			else
+				spi->mode &= ~SPI_LSB_FIRST;
+			retval = spi_setup(spi);
+			if (retval < 0)
+				spi->mode = save;
+			else
+				dev_dbg(&spi->dev, "%csb first\n",
+						tmp ? 'l' : 'm');
+		}
+		break;
+	case SPI_IOC_WR_BITS_PER_WORD:
+		retval = get_user(tmp, (__u8 __user *)arg);
+		if (retval == 0) {
+			u8	save = spi->bits_per_word;
+
+			spi->bits_per_word = tmp;
+			retval = spi_setup(spi);
+			if (retval < 0)
+				spi->bits_per_word = save;
+			else
+				dev_dbg(&spi->dev, "%d bits per word\n", tmp);
+		}
+		break;
+	case SPI_IOC_WR_MAX_SPEED_HZ:
+		retval = get_user(tmp, (__u32 __user *)arg);
+		if (retval == 0) {
+			u32	save = spi->max_speed_hz;
+
+			spi->max_speed_hz = tmp;
+			retval = spi_setup(spi);
+			if (retval == 0) {
+				elbasr->speed_hz = tmp;
+				dev_dbg(&spi->dev, "%d Hz (max)\n",
+					elbasr->speed_hz);
+			}
+			spi->max_speed_hz = save;
+		}
+		break;
+
+	default:
+		/* segmented and/or full-duplex I/O request */
+		/* Check message and copy into scratch area */
+		ioc = elbasr_spi_get_ioc_message(cmd,
+				(struct spi_ioc_transfer __user *)arg, &n_ioc);
+		if (IS_ERR(ioc)) {
+			retval = PTR_ERR(ioc);
+			break;
+		}
+		if (!ioc)
+			break;	/* n_ioc is also 0 */
+
+		/* translate to spi_message, execute */
+		retval = elbasr_spi_message(elbasr, ioc, n_ioc);
+		kfree(ioc);
+		break;
+	}
+
+	mutex_unlock(&elbasr->buf_lock);
+	spi_dev_put(spi);
+	return retval;
+}
+
+#ifdef CONFIG_COMPAT
+static long
+elbasr_spi_compat_ioc_message(struct file *filp, unsigned int cmd,
+			      unsigned long arg)
+{
+	struct spi_ioc_transfer __user *u_ioc;
+	int retval = 0;
+	struct elbasr_data *elbasr;
+	struct spi_device *spi;
+	unsigned int n_ioc, n;
+	struct spi_ioc_transfer *ioc;
+
+	u_ioc = (struct spi_ioc_transfer __user *) compat_ptr(arg);
+
+	/* guard against device removal before, or while,
+	 * we issue this ioctl.
+	 */
+	elbasr = filp->private_data;
+	spin_lock_irq(&elbasr->spi_lock);
+	spi = spi_dev_get(elbasr->spi);
+	spin_unlock_irq(&elbasr->spi_lock);
+
+	if (spi == NULL)
+		return -ESHUTDOWN;
+
+	/* SPI_IOC_MESSAGE needs the buffer locked "normally" */
+	mutex_lock(&elbasr->buf_lock);
+
+	/* Check message and copy into scratch area */
+	ioc = elbasr_spi_get_ioc_message(cmd, u_ioc, &n_ioc);
+	if (IS_ERR(ioc)) {
+		retval = PTR_ERR(ioc);
+		goto done;
+	}
+	if (!ioc)
+		goto done;	/* n_ioc is also 0 */
+
+	/* Convert buffer pointers */
+	for (n = 0; n < n_ioc; n++) {
+		ioc[n].rx_buf = (uintptr_t) compat_ptr(ioc[n].rx_buf);
+		ioc[n].tx_buf = (uintptr_t) compat_ptr(ioc[n].tx_buf);
+	}
+
+	/* translate to spi_message, execute */
+	retval = elbasr_spi_message(elbasr, ioc, n_ioc);
+	kfree(ioc);
+
+done:
+	mutex_unlock(&elbasr->buf_lock);
+	spi_dev_put(spi);
+	return retval;
+}
+
+static long
+elbasr_spi_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	if (_IOC_TYPE(cmd) == SPI_IOC_MAGIC
+			&& _IOC_NR(cmd) == _IOC_NR(SPI_IOC_MESSAGE(0))
+			&& _IOC_DIR(cmd) == _IOC_WRITE)
+		return elbasr_spi_compat_ioc_message(filp, cmd, arg);
+
+	return elbasr_spi_ioctl(filp, cmd, (unsigned long)compat_ptr(arg));
+}
+#else
+#define elbasr_spi_compat_ioctl NULL
+#endif /* CONFIG_COMPAT */
+
+static int elbasr_spi_open(struct inode *inode, struct file *filp)
+{
+	struct elbasr_data *elbasr;
+	int status = -ENXIO;
+
+	mutex_lock(&device_list_lock);
+
+	list_for_each_entry(elbasr, &device_list, device_entry) {
+		if (elbasr->devt == inode->i_rdev) {
+			status = 0;
+			break;
+		}
+	}
+
+	if (status) {
+		pr_debug("elbasr_spi: nothing for minor %d\n", iminor(inode));
+		goto err_find_dev;
+	}
+
+	if (!elbasr->tx_buffer) {
+		elbasr->tx_buffer = kmalloc(bufsiz, GFP_KERNEL);
+		if (!elbasr->tx_buffer) {
+			status = -ENOMEM;
+			goto err_find_dev;
+		}
+	}
+
+	if (!elbasr->rx_buffer) {
+		elbasr->rx_buffer = kmalloc(bufsiz, GFP_KERNEL);
+		if (!elbasr->rx_buffer) {
+			status = -ENOMEM;
+			goto err_alloc_rx_buf;
+		}
+	}
+
+	elbasr->users++;
+	filp->private_data = elbasr;
+	stream_open(inode, filp);
+
+	mutex_unlock(&device_list_lock);
+	return 0;
+
+err_alloc_rx_buf:
+	kfree(elbasr->tx_buffer);
+	elbasr->tx_buffer = NULL;
+err_find_dev:
+	mutex_unlock(&device_list_lock);
+	return status;
+}
+
+static int elbasr_spi_release(struct inode *inode, struct file *filp)
+{
+	struct elbasr_data *elbasr;
+	int dofree;
+
+	mutex_lock(&device_list_lock);
+	elbasr = filp->private_data;
+	filp->private_data = NULL;
+
+	spin_lock_irq(&elbasr->spi_lock);
+	/* ... after we unbound from the underlying device? */
+	dofree = (elbasr->spi == NULL);
+	spin_unlock_irq(&elbasr->spi_lock);
+
+	/* last close? */
+	elbasr->users--;
+	if (!elbasr->users) {
+
+		kfree(elbasr->tx_buffer);
+		elbasr->tx_buffer = NULL;
+
+		kfree(elbasr->rx_buffer);
+		elbasr->rx_buffer = NULL;
+
+		if (dofree)
+			kfree(elbasr);
+		else
+			elbasr->speed_hz = elbasr->spi->max_speed_hz;
+	}
+#ifdef CONFIG_SPI_SLAVE
+	if (!dofree)
+		spi_slave_abort(elbasr->spi);
+#endif
+	mutex_unlock(&device_list_lock);
+
+	return 0;
+}
+
+static const struct file_operations elbasr_spi_fops = {
+	.owner =	THIS_MODULE,
+	.write =	elbasr_spi_write,
+	.read =		elbasr_spi_read,
+	.unlocked_ioctl = elbasr_spi_ioctl,
+	.compat_ioctl = elbasr_spi_compat_ioctl,
+	.open =		elbasr_spi_open,
+	.release =	elbasr_spi_release,
+	.llseek =	no_llseek,
+};
+
+static bool
+elbasr_reg_readable(struct device *dev, unsigned int reg)
+{
+	return reg <= ELBASR_MAX_REG;
+}
+
+static bool
+elbasr_reg_writeable(struct device *dev, unsigned int reg)
+{
+	return reg <= ELBASR_MAX_REG;
+}
+
+static int
+elbasr_regs_read(void *ctx, u32 reg, u32 *val)
+{
+	struct elbasr_data *elbasr = dev_get_drvdata(ctx);
+	struct spi_message m;
+	struct spi_transfer t[2] = { { 0 } };
+	int ret;
+	u8 txbuf[3];
+	u8 rxbuf[1];
+
+	spi_message_init(&m);
+
+	txbuf[0] = ELBASR_SPI_CMD_REGRD;
+	txbuf[1] = reg;
+	txbuf[2] = 0x0;
+	t[0].tx_buf = (u8 *)txbuf;
+	t[0].len = 3;
+
+	rxbuf[0] = 0x0;
+	t[1].rx_buf = rxbuf;
+	t[1].len = 1;
+
+	spi_message_add_tail(&t[0], &m);
+	spi_message_add_tail(&t[1], &m);
+
+	ret = elbasr_spi_sync(elbasr, &m);
+	if (ret == 4) {
+		// 3 Tx + 1 Rx = 4
+		*val = rxbuf[0];
+		return 0;
+	}
+	return -EIO;
+}
+
+static int
+elbasr_regs_write(void *ctx, u32 reg, u32 val)
+{
+	struct elbasr_data *elbasr = dev_get_drvdata(ctx);
+	struct spi_message m;
+	struct spi_transfer t[1] = { { 0 } };
+	u8 txbuf[4];
+
+	spi_message_init(&m);
+	txbuf[0] = ELBASR_SPI_CMD_REGWR;
+	txbuf[1] = reg;
+	txbuf[2] = val;
+	txbuf[3] = 0;
+
+	t[0].tx_buf = txbuf;
+	t[0].len = 4;
+
+	spi_message_add_tail(&t[0], &m);
+
+	return elbasr_spi_sync(elbasr, &m);
+}
+
+static const struct regmap_config pensando_elbasr_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.cache_type = REGCACHE_NONE,
+	.readable_reg = elbasr_reg_readable,
+	.writeable_reg = elbasr_reg_writeable,
+	.reg_read = elbasr_regs_read,
+	.reg_write = elbasr_regs_write,
+	.max_register = ELBASR_MAX_REG
+};
+
+/*
+ * Setup Elba SPI access to System Resource Chip registers on CS0
+ */
+static int
+elbasr_regs_setup(struct spi_device *spi, struct elbasr_data *elbasr)
+{
+	int ret;
+
+	spi->bits_per_word = 8;
+	spi_setup(spi);
+	elbasr->elbasr_regs = devm_regmap_init(&spi->dev, NULL, spi,
+					       &pensando_elbasr_regmap_config);
+	if (IS_ERR(elbasr->elbasr_regs)) {
+		ret = PTR_ERR(elbasr->elbasr_regs);
+		dev_err(&spi->dev, "Failed to allocate register map: %d\n", ret);
+		return ret;
+	}
+
+	ret = devm_mfd_add_devices(&spi->dev, PLATFORM_DEVID_NONE,
+				   pensando_elbasr_subdev_info,
+				   ARRAY_SIZE(pensando_elbasr_subdev_info),
+				   NULL, 0, NULL);
+	if (ret)
+		dev_err(&spi->dev, "Failed to register sub-devices: %d\n", ret);
+
+	return ret;
+}
+
+static int elbasr_spi_probe(struct spi_device *spi)
+{
+	struct elbasr_data *elbasr;
+	unsigned long minor;
+	int status;
+
+	if (spi->chip_select == 0) {
+		status = alloc_chrdev_region(&elbasr_devt, 0, ELBASR_MAX_DEVS,
+					     "elbasr");
+		if (status < 0)
+			return status;
+
+		elbasr_class = class_create(THIS_MODULE, "elbasr");
+		if (IS_ERR(elbasr_class)) {
+			unregister_chrdev(MAJOR(elbasr_devt), "elbasr");
+			return PTR_ERR(elbasr_class);
+		}
+	}
+
+	/* Allocate driver data */
+	elbasr = kzalloc(sizeof(*elbasr), GFP_KERNEL);
+	if (!elbasr)
+		return -ENOMEM;
+
+	/* Initialize the driver data */
+	elbasr->spi = spi;
+	elbasr->speed_hz = spi->max_speed_hz;
+	spin_lock_init(&elbasr->spi_lock);
+	mutex_init(&elbasr->buf_lock);
+
+	INIT_LIST_HEAD(&elbasr->device_entry);
+
+	mutex_lock(&device_list_lock);
+	minor = find_first_zero_bit(minors, ELBASR_MAX_DEVS);
+	if (minor < ELBASR_MAX_DEVS) {
+		struct device *dev;
+
+		elbasr->devt = MKDEV(MAJOR(elbasr_devt), minor);
+		dev = device_create(elbasr_class,
+				    &spi->dev,
+				    elbasr->devt,
+				    elbasr,
+				    "pensr%d.%d",
+				    spi->master->bus_num,
+				    spi->chip_select);
+
+		status = PTR_ERR_OR_ZERO(dev);
+	} else {
+		dev_dbg(&spi->dev, "no minor number available\n");
+		status = -ENODEV;
+		goto minor_failed;
+	}
+
+	set_bit(minor, minors);
+	list_add(&elbasr->device_entry, &device_list);
+	dev_dbg(&spi->dev,
+		"created device for major %d, minor %lu\n",
+		MAJOR(elbasr_devt), minor);
+	mutex_unlock(&device_list_lock);
+
+	/* Create cdev */
+	elbasr->cdev = cdev_alloc();
+	if (!elbasr->cdev) {
+		dev_err(elbasr->dev, "allocation of cdev failed");
+		status = -ENOMEM;
+		goto cdev_failed;
+	}
+	elbasr->cdev->owner = THIS_MODULE;
+	cdev_init(elbasr->cdev, &elbasr_spi_fops);
+
+	status = cdev_add(elbasr->cdev, elbasr->devt, 1);
+	if (status) {
+		dev_err(elbasr->dev, "register of cdev failed");
+		goto cdev_delete;
+	}
+	spi_set_drvdata(spi, elbasr);
+
+	/* Add Elba reset driver sub-device */
+	if (spi->chip_select == 0)
+		elbasr_regs_setup(spi, elbasr);
+
+	return 0;
+
+cdev_delete:
+	if (spi->chip_select == 0)
+		cdev_del(elbasr->cdev);
+cdev_failed:
+	if (spi->chip_select == 0)
+		device_destroy(elbasr_class, elbasr->devt);
+minor_failed:
+	kfree(elbasr);
+
+	return status;
+}
+
+static const struct of_device_id elbasr_spi_of_match[] = {
+	{ .compatible = "amd,pensando-elbasr" },
+	{ /* sentinel */ },
+};
+
+static struct spi_driver elbasr_spi_driver = {
+	.probe = elbasr_spi_probe,
+	.driver = {
+		.name = "elbasr",
+		.of_match_table = of_match_ptr(elbasr_spi_of_match),
+	},
+};
+builtin_driver(elbasr_spi_driver, spi_register_driver)
diff --git a/include/linux/mfd/pensando-elbasr.h b/include/linux/mfd/pensando-elbasr.h
new file mode 100644
index 000000000000..2b794218dca5
--- /dev/null
+++ b/include/linux/mfd/pensando-elbasr.h
@@ -0,0 +1,78 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+/*
+ * Copyright (c) 2022 AMD Pensando
+ *
+ * Declarations for AMD Pensando Elba System Resource Chip
+ */
+
+#ifndef __MFD_AMD_PENSANDO_ELBA_H
+#define __MFD_AMD_PENSANDO_ELBA_H
+
+#include <linux/cdev.h>
+#include <linux/regmap.h>
+
+#define ELBASR_REVISION_REG			0x00
+#define ELBASR_CTRL_REG				0x01
+#define ELBASR_QSFP_CTRL_REG			0x02
+#define ELBASR_INTERRUPT_ENABLE_REG		0x03
+#define ELBASR_INTERRUPT_STATUS_REG		0x04
+#define ELBASR_QSFP_LED_REG			0x05
+#define ELBASR_QSFP_LED_FREQUENCY_REG		0x0F
+#define ELBASR_CTRL0_REG			0x10
+#define ELBASR_CTRL1_REG			0x11
+#define ELBASR_CTRL2_REG			0x12
+#define ELBASR_SYSTEM_LED_REG			0x15
+#define ELBASR_CORE_TEMP_REG			0x16
+#define ELBASR_HBM_TEMP_REG			0x17
+#define ELBASR_BOARD_TEMP_REG			0x18
+#define ELBASR_QSFP_PORT1_TEMP_REG		0x19
+#define ELBASR_QSFP_PORT2_TEMP_REG		0x1a
+#define ELBASR_HBM_WARNING_TEMP_REG		0x1b
+#define ELBASR_HBM_CRITICAL_TEMP_REG		0x1c
+#define ELBASR_HBM_FATAL_TEMP_REG		0x1d
+#define ELBASR_ROT_REG0_CNTL_REG		0x23
+#define ELBASR_PUF_ERROR_LIMITS_REG		0x29
+#define ELBASR_PUF_ERROR_COUNT_REG		0x2a
+#define ELBASR_QSFP_PORT1_ALARM_TEMP_REG	0x34
+#define ELBASR_QSFP_PORT1_WARNING_TEMP_REG	0x35
+#define ELBASR_QSFP_PORT2_ALARM_TEMP_REG	0x36
+#define ELBASR_QSFP_PORT2_WARNING_TEMP_REG	0x37
+#define ELBASR_SYSTEM_HEALTH0_REG		0x38
+#define ELBASR_SYSTEM_HEALTH1_REG		0x39
+#define ELBASR_MAJOR_FW_VER_REG			0x3a
+#define ELBASR_MINOR_FW_VER_REG			0x3b
+#define ELBASR_MAINTANENCE_FW_VER_REG		0x3c
+#define ELBASR_PIPELINE_FW_REG			0x3d
+#define ELBASR_QSFP_PRESENT_REG			0x40
+#define ELBASR_OCP_SLOTID_REG			0x42
+#define ELBASR_OCP_SC_DATA0_REG			0x43
+#define ELBASR_OCP_SC_DATA1_REG			0x44
+#define ELBASR_ID				0x80
+
+#define ELBASR_MAX_REG				0x80
+#define ELBASR_NR_RESETS			1
+
+/*
+ * Pensando Elba System Resource MFD device private data structure
+ */
+struct elbasr_data {
+	dev_t devt;
+	int minor;
+	struct device *dev;
+	struct cdev *cdev;
+	struct spi_device *spi;
+	struct list_head device_entry;
+	spinlock_t spi_lock;
+
+	/* TX/RX buffers are NULL unless this device is open (users > 0) */
+	struct mutex buf_lock;
+	unsigned int users;
+	u8 *tx_buffer;
+	u8 *rx_buffer;
+	u32 speed_hz;
+
+	/* System Resource Chip CS0 register access */
+	struct regmap *elbasr_regs;
+};
+
+#endif /* __MFD_AMD_PENSANDO_ELBA_H */
-- 
2.17.1

Re: [PATCH v5 14/15] mfd: pensando-elbasr: Add AMD Pensando Elba System Resource chip

[Andy Shevchenko]

On Mon, Jun 13, 2022 at 9:57 PM Brad Larson <brad@pensando.io> wrote:
>
> From: Brad Larson <blarson@amd.com>
>
> Add support for the AMD Pensando Elba SoC System Resource chip
> using the SPI interface.  The Elba SR is a Multi-function Device
> supporting device register access using CS0, smbus interface for
> FRU and board peripherals using CS1, dual Lattice I2C masters for
> transceiver management using CS2, and CS3 for flash access.

...

> +#include <linux/mfd/pensando-elbasr.h>
> +#include <linux/mfd/core.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/ioctl.h>
> +#include <linux/fs.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/list.h>
> +#include <linux/errno.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/compat.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spidev.h>
> +#include <linux/delay.h>

Keep it sorted?
It would easily tell that types.h is missed, but maybe other headers
are superfluous.

...

> +/* The main reason to have this class is to make mdev/udev create the
> + * /dev/pensrB.C character device nodes exposing our userspace API.
> + * It also simplifies memory management.  The device nodes
> + * /dev/pensrB.C are common across Pensando boards.
> + */

/*
 * The above style of multi-line
 * comments is for networking,
 * the rest uses a slightly different one.
 */

...

> +static DECLARE_BITMAP(minors, ELBASR_MAX_DEVS);
> +static unsigned int bufsiz = 4096;
> +
> +static LIST_HEAD(device_list);
> +static DEFINE_MUTEX(device_list_lock);

Is it all to reinvent IDA?

...

> +static ssize_t
> +elbasr_spi_sync(struct elbasr_data *elbasr_spi, struct spi_message *message)
> +{
> +       int status;
> +       struct spi_device *spi;
> +
> +       spin_lock_irq(&elbasr_spi->spi_lock);
> +       spi = elbasr_spi->spi;
> +       spin_unlock_irq(&elbasr_spi->spi_lock);

> +

Drop this blank line and see below.

> +       if (spi == NULL)
> +               status = -ESHUTDOWN;

if (!spi)
  return ...

> +       else

> +               status = spi_sync(spi, message);
> +
> +       if (status == 0)
> +               status = message->actual_length;
> +
> +       return status;

status = spi_sync(...);
if (status)
  return status;

return message->actual_length;

> +}

...

> +       if (status) {

> +               pr_debug("elbasr_spi: nothing for minor %d\n", iminor(inode));

We have a device pointer, don't we?

> +               goto err_find_dev;
> +       }

...

> +static const struct file_operations elbasr_spi_fops = {
> +       .owner =        THIS_MODULE,
> +       .write =        elbasr_spi_write,
> +       .read =         elbasr_spi_read,
> +       .unlocked_ioctl = elbasr_spi_ioctl,
> +       .compat_ioctl = elbasr_spi_compat_ioctl,
> +       .open =         elbasr_spi_open,
> +       .release =      elbasr_spi_release,
> +       .llseek =       no_llseek,
> +};


As far as I can see the code looks like a proxy for SPI via SPI. Is
that the correct interpretation? If so, why this code repeating _a
lot_ from SPI framework, including character device IOCTL? This is a
big question here and since there is missed documentation for ABI and
no points to userspace tools which are going to use this ABI (red
flag!) the code is no go.

...

> +static bool
> +elbasr_reg_readable(struct device *dev, unsigned int reg)

It's pretty much one line, can you reduce the number of LoCs by
reindenting your code a bit?

...

> +static bool
> +elbasr_reg_writeable(struct device *dev, unsigned int reg)

Ditto and so on.

...

> +       struct spi_transfer t[2] = { { 0 } };

Isn't  `{ }` enough?

...

> +       spi_message_add_tail(&t[0], &m);
> +       spi_message_add_tail(&t[1], &m);

spi_message_init_with_transfers() ?
Here and elsewhere.

...

> +       struct spi_transfer t[1] = { { 0 } };

Why does `struct spi_transfer t = { };` not work?!

...

> +static const struct regmap_config pensando_elbasr_regmap_config = {
> +       .reg_bits = 8,
> +       .val_bits = 8,
> +       .cache_type = REGCACHE_NONE,
> +       .readable_reg = elbasr_reg_readable,
> +       .writeable_reg = elbasr_reg_writeable,
> +       .reg_read = elbasr_regs_read,
> +       .reg_write = elbasr_regs_write,
> +       .max_register = ELBASR_MAX_REG

Leave a comma here.

> +};

...

> +       elbasr->elbasr_regs = devm_regmap_init(&spi->dev, NULL, spi,
> +                                              &pensando_elbasr_regmap_config);
> +       if (IS_ERR(elbasr->elbasr_regs)) {
> +               ret = PTR_ERR(elbasr->elbasr_regs);
> +               dev_err(&spi->dev, "Failed to allocate register map: %d\n", ret);
> +               return ret;

return dev_err_probe(...);

> +       }
> +
> +       ret = devm_mfd_add_devices(&spi->dev, PLATFORM_DEVID_NONE,
> +                                  pensando_elbasr_subdev_info,
> +                                  ARRAY_SIZE(pensando_elbasr_subdev_info),
> +                                  NULL, 0, NULL);
> +       if (ret)
> +               dev_err(&spi->dev, "Failed to register sub-devices: %d\n", ret);
> +
> +       return ret;

Ditto.

...

> +       /* Add Elba reset driver sub-device */
> +       if (spi->chip_select == 0)
> +               elbasr_regs_setup(spi, elbasr);

You have an awful mixture of devm_ vs. non-devm_ calls. Either move
from devm_ completely, or switch to devm_ in the rest of the ->probe()
code.

...

> +static const struct of_device_id elbasr_spi_of_match[] = {
> +       { .compatible = "amd,pensando-elbasr" },
> +       { /* sentinel */ },

Comma is not needed in terminator entry.

> +};

...

> +static struct spi_driver elbasr_spi_driver = {
> +       .probe = elbasr_spi_probe,
> +       .driver = {
> +               .name = "elbasr",

> +               .of_match_table = of_match_ptr(elbasr_spi_of_match),

of_match_ptr() is useless here (look at your Kconfig) and in some
cases is harmful. No need to use this.

> +       },
> +};

...

> +#include <linux/cdev.h>
> +#include <linux/regmap.h>

mutex.h and types.h are missed, for example.
You need to use headers for direct use of. And in some cases forward
declarations can be used instead of including a header.

--
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 14/15] mfd: pensando-elbasr: Add AMD Pensando Elba System Resource chip

[Brad Larson]

Hi Andy,

On Tue, Jun 14, 2022 at 4:42 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Jun 13, 2022 at 9:57 PM Brad Larson <brad@pensando.io> wrote:
> ...
>
> > +#include <linux/mfd/pensando-elbasr.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/ioctl.h>
> > +#include <linux/fs.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/list.h>
> > +#include <linux/errno.h>
> > +#include <linux/mutex.h>
> > +#include <linux/slab.h>
> > +#include <linux/compat.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/spi/spidev.h>
> > +#include <linux/delay.h>
>
> Keep it sorted?
> It would easily tell that types.h is missed, but maybe other headers
> are superfluous.

Sorted and added types.h

> ...
>
> > +/* The main reason to have this class is to make mdev/udev create the
> > + * /dev/pensrB.C character device nodes exposing our userspace API.
> > + * It also simplifies memory management.  The device nodes
> > + * /dev/pensrB.C are common across Pensando boards.
> > + */
>
> /*
>  * The above style of multi-line
>  * comments is for networking,
>  * the rest uses a slightly different one.
>  */

Changed to non-networking multi-line comments, code reuse from
driver/spi/spidev.c there since ~2007.

> ...
>
> > +static DECLARE_BITMAP(minors, ELBASR_MAX_DEVS);
> > +static unsigned int bufsiz = 4096;
> > +
> > +static LIST_HEAD(device_list);
> > +static DEFINE_MUTEX(device_list_lock);
>
> Is it all to reinvent IDA?

I don't know what IDA is, searching linux IDA yields debugger and some
image viewer.
The whole purpose of adding this driver was to not use spidev.c to
provide Elba specific
access to 4 functions in the fpga and also an emmc reset driver.  The
reuse of code from
spidev.c was to avoid breaking production deployments while adding emmc hardware
reset support as a priority.  I'll strip out any code not used for
Elba deployments for next
version.

> ...
>
> > +static ssize_t
> > +elbasr_spi_sync(struct elbasr_data *elbasr_spi, struct spi_message *message)
> > +{
> > +       int status;
> > +       struct spi_device *spi;
> > +
> > +       spin_lock_irq(&elbasr_spi->spi_lock);
> > +       spi = elbasr_spi->spi;
> > +       spin_unlock_irq(&elbasr_spi->spi_lock);
>
> > +
>
> Drop this blank line and see below.

Removed

> > +       if (spi == NULL)
> > +               status = -ESHUTDOWN;
>
> if (!spi)
>   return ...
>
> > +       else
>
> > +               status = spi_sync(spi, message);
> > +
> > +       if (status == 0)
> > +               status = message->actual_length;
> > +
> > +       return status;
>
> status = spi_sync(...);
> if (status)
>   return status;
>
> return message->actual_length;

Refactored to:
        if (!spi)
                return -ESHUTDOWN;

        status = spi_sync(spi, message);
        if (status)
                return status;

        return message->actual_length;

> ...
> > +       if (status) {
>
> > +               pr_debug("elbasr_spi: nothing for minor %d\n", iminor(inode));

> We have a device pointer, don't we?

Removed

> ...
>
> > +static const struct file_operations elbasr_spi_fops = {
> > +       .owner =        THIS_MODULE,
> > +       .write =        elbasr_spi_write,
> > +       .read =         elbasr_spi_read,
> > +       .unlocked_ioctl = elbasr_spi_ioctl,
> > +       .compat_ioctl = elbasr_spi_compat_ioctl,
> > +       .open =         elbasr_spi_open,
> > +       .release =      elbasr_spi_release,
> > +       .llseek =       no_llseek,
> > +};
>
>
> As far as I can see the code looks like a proxy for SPI via SPI. Is
> that the correct interpretation? If so, why this code repeating _a
> lot_ from SPI framework, including character device IOCTL? This is a
> big question here and since there is missed documentation for ABI and
> no points to userspace tools which are going to use this ABI (red
> flag!) the code is no go.

The patch v5-0006-dt-bindings-mfd-amd-pensando-elbasr-Add-AMD-Pensa.patch
has the documentation where below is the commit message.  This is a required
companion device to the Elba SoC (on spi0) which is a FPGA with four
functions.
One function that isn't filled out (driver not included in base SoC
support) is accessed
at /dev/pensr0.2 which is a Lattice dual I2C master for the
transceiver management.
There are customer utilities and programs that open /dev/pensr0.x in operation.

  dt-bindings: mfd: amd,pensando-elbasr: Add AMD Pensando Elba System
Resource chip

  Add support for the AMD Pensando Elba SoC System Resource chip
  using the SPI interface.  The Elba SR is a Multi-function Device
  supporting device register access using CS0, smbus interface for
  FRU and board peripherals using CS1, dual Lattice I2C masters for
  transceiver management using CS2, and CS3 for flash access.


> ...
>
> > +static bool
> > +elbasr_reg_readable(struct device *dev, unsigned int reg)
>
> It's pretty much one line, can you reduce the number of LoCs by
> reindenting your code a bit?

Moved to one line, here and others

> ...
>
> > +static bool
> > +elbasr_reg_writeable(struct device *dev, unsigned int reg)
>
> Ditto and so on.
>
> ...
>
> > +       struct spi_transfer t[2] = { { 0 } };
>
> Isn't  `{ }` enough?

Yes, changed

> ...
>
> > +       spi_message_add_tail(&t[0], &m);
> > +       spi_message_add_tail(&t[1], &m);
>
> spi_message_init_with_transfers() ?
> Here and elsewhere.

Changed to use of spi_message_init_with_transfers()

> ...
>
> > +       struct spi_transfer t[1] = { { 0 } };
>
> Why does `struct spi_transfer t = { };` not work?!

It does, changed

> ...
>
> > +static const struct regmap_config pensando_elbasr_regmap_config = {
> > +       .reg_bits = 8,
> > +       .val_bits = 8,
> > +       .cache_type = REGCACHE_NONE,
> > +       .readable_reg = elbasr_reg_readable,
> > +       .writeable_reg = elbasr_reg_writeable,
> > +       .reg_read = elbasr_regs_read,
> > +       .reg_write = elbasr_regs_write,
> > +       .max_register = ELBASR_MAX_REG
>
> Leave a comma here.

Added a comma after ELBASR_MAX_REG

> > +};
>
> ...
>
> > +       elbasr->elbasr_regs = devm_regmap_init(&spi->dev, NULL, spi,
> > +                                              &pensando_elbasr_regmap_config);
> > +       if (IS_ERR(elbasr->elbasr_regs)) {
> > +               ret = PTR_ERR(elbasr->elbasr_regs);
> > +               dev_err(&spi->dev, "Failed to allocate register map: %d\n", ret);
> > +               return ret;
>
> return dev_err_probe(...);

Changed to dev_err_probe(...)

> > +       }
> > +
> > +       ret = devm_mfd_add_devices(&spi->dev, PLATFORM_DEVID_NONE,
> > +                                  pensando_elbasr_subdev_info,
> > +                                  ARRAY_SIZE(pensando_elbasr_subdev_info),
> > +                                  NULL, 0, NULL);
> > +       if (ret)
> > +               dev_err(&spi->dev, "Failed to register sub-devices: %d\n", ret);
> > +
> > +       return ret;
>
> Ditto.

Changed to dev_err_probe(...)

> ...
>
> > +       /* Add Elba reset driver sub-device */
> > +       if (spi->chip_select == 0)
> > +               elbasr_regs_setup(spi, elbasr);
>
> You have an awful mixture of devm_ vs. non-devm_ calls. Either move
> from devm_ completely, or switch to devm_ in the rest of the ->probe()
> code.

Moved away from devm_

> ...
>
> > +static const struct of_device_id elbasr_spi_of_match[] = {
> > +       { .compatible = "amd,pensando-elbasr" },
> > +       { /* sentinel */ },
>
> Comma is not needed in terminator entry.

Removed comma

> ...
>
> > +static struct spi_driver elbasr_spi_driver = {
> > +       .probe = elbasr_spi_probe,
> > +       .driver = {
> > +               .name = "elbasr",
>
> > +               .of_match_table = of_match_ptr(elbasr_spi_of_match),
>
> of_match_ptr() is useless here (look at your Kconfig) and in some
> cases is harmful. No need to use this.

Removed use of of_match_ptr()

> > +       },
> > +};
>
> ...
>
> > +#include <linux/cdev.h>
> > +#include <linux/regmap.h>
>
> mutex.h and types.h are missed, for example.
> You need to use headers for direct use of. And in some cases forward
> declarations can be used instead of including a header.

Added missed mutex.h and types.h

Regards,
Brad

[PATCH v5 15/15] reset: elbasr: Add AMD Pensando Elba SR Reset Controller

[Brad Larson]

From: Brad Larson <blarson@amd.com>

This patch adds the reset controller functionality for the
AMD Pensando Elba System Resource Chip.

Signed-off-by: Brad Larson <blarson@amd.com>
---
 drivers/reset/Kconfig                         |  9 ++
 drivers/reset/Makefile                        |  1 +
 drivers/reset/reset-elbasr.c                  | 94 +++++++++++++++++++
 .../reset/amd,pensando-elba-reset.h           | 11 +++
 4 files changed, 115 insertions(+)
 create mode 100644 drivers/reset/reset-elbasr.c
 create mode 100644 include/dt-bindings/reset/amd,pensando-elba-reset.h

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 93c8d07ee328..13f5a8ca0f03 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -66,6 +66,15 @@ config RESET_BRCMSTB_RESCAL
 	  This enables the RESCAL reset controller for SATA, PCIe0, or PCIe1 on
 	  BCM7216.
 
+config RESET_ELBASR
+	tristate "Pensando Elba System Resource reset controller"
+	depends on MFD_PENSANDO_ELBASR || COMPILE_TEST
+	help
+	  This option enables support for the external reset functions
+	  on the Pensando Elba System Resource Chip.  Reset control
+	  of peripherals is accessed over SPI to the system resource
+	  chip device registers using CS0.
+
 config RESET_HSDK
 	bool "Synopsys HSDK Reset Driver"
 	depends on HAS_IOMEM
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index a80a9c4008a7..c0fe12b9950e 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_RESET_BCM6345) += reset-bcm6345.o
 obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
 obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o
 obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
+obj-$(CONFIG_RESET_ELBASR) += reset-elbasr.o
 obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
 obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
 obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
diff --git a/drivers/reset/reset-elbasr.c b/drivers/reset/reset-elbasr.c
new file mode 100644
index 000000000000..6e429cb11466
--- /dev/null
+++ b/drivers/reset/reset-elbasr.c
@@ -0,0 +1,94 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2022 AMD Pensando
+ */
+
+#include <linux/mfd/pensando-elbasr.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/regmap.h>
+#include <linux/err.h>
+#include <linux/of.h>
+
+#include <dt-bindings/reset/amd,pensando-elba-reset.h>
+
+struct elbasr_reset {
+	struct reset_controller_dev rcdev;
+	struct regmap *regmap;
+};
+
+static inline struct elbasr_reset *to_elbasr_rst(struct reset_controller_dev *rc)
+{
+	return container_of(rc, struct elbasr_reset, rcdev);
+}
+
+static inline int elbasr_reset_shift(unsigned long id)
+{
+	switch (id) {
+	case EMMC_HW_RESET:
+		return 6;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int elbasr_reset_assert(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct elbasr_reset *elbar = to_elbasr_rst(rcdev);
+	u32 mask = 1 << elbasr_reset_shift(id);
+
+	return regmap_update_bits(elbar->regmap, ELBASR_CTRL0_REG, mask, mask);
+}
+
+static int elbasr_reset_deassert(struct reset_controller_dev *rcdev,
+				 unsigned long id)
+{
+	struct elbasr_reset *elbar = to_elbasr_rst(rcdev);
+	u32 mask = 1 << elbasr_reset_shift(id);
+
+	return regmap_update_bits(elbar->regmap, ELBASR_CTRL0_REG, mask, 0);
+}
+
+static const struct reset_control_ops elbasr_reset_ops = {
+	.assert	= elbasr_reset_assert,
+	.deassert = elbasr_reset_deassert,
+};
+
+static int elbasr_reset_probe(struct platform_device *pdev)
+{
+	struct elbasr_data *elbasr = dev_get_drvdata(pdev->dev.parent);
+	struct elbasr_reset *elbar;
+	int ret;
+
+	elbar = devm_kzalloc(&pdev->dev, sizeof(struct elbasr_reset),
+			     GFP_KERNEL);
+	if (!elbar)
+		return -ENOMEM;
+
+	elbar->rcdev.owner = THIS_MODULE;
+	elbar->rcdev.nr_resets = ELBASR_NR_RESETS;
+	elbar->rcdev.ops = &elbasr_reset_ops;
+	elbar->rcdev.of_node = pdev->dev.of_node;
+	elbar->regmap = elbasr->elbasr_regs;
+
+	platform_set_drvdata(pdev, elbar);
+
+	ret = devm_reset_controller_register(&pdev->dev, &elbar->rcdev);
+
+	return ret;
+}
+
+static const struct of_device_id elba_reset_dt_match[] = {
+	{ .compatible = "amd,pensando-elbasr-reset", },
+	{ /* sentinel */ },
+};
+
+static struct platform_driver elbasr_reset_driver = {
+	.probe	= elbasr_reset_probe,
+	.driver = {
+		.name = "pensando_elbasr_reset",
+		.of_match_table	= elba_reset_dt_match,
+	},
+};
+builtin_platform_driver(elbasr_reset_driver);
diff --git a/include/dt-bindings/reset/amd,pensando-elba-reset.h b/include/dt-bindings/reset/amd,pensando-elba-reset.h
new file mode 100644
index 000000000000..68d69a98e750
--- /dev/null
+++ b/include/dt-bindings/reset/amd,pensando-elba-reset.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+/*
+ * Copyright (c) 2022, AMD Pensando
+ */
+
+#ifndef _DT_BINDINGS_RESET_AMD_PENSANDO_ELBA_RESET_H
+#define _DT_BINDINGS_RESET_AMD_PENSANDO_ELBA_RESET_H
+
+#define EMMC_HW_RESET		0
+
+#endif
-- 
2.17.1

Re: [PATCH v5 15/15] reset: elbasr: Add AMD Pensando Elba SR Reset Controller

[Andy Shevchenko]

On Mon, Jun 13, 2022 at 9:57 PM Brad Larson <brad@pensando.io> wrote:
>
> From: Brad Larson <blarson@amd.com>
>
> This patch adds the reset controller functionality for the
> AMD Pensando Elba System Resource Chip.

...

> +#include <linux/mfd/pensando-elbasr.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/regmap.h>
> +#include <linux/err.h>

> +#include <linux/of.h>

There is no user of this header. But there are missed ones, such as
mod_devicetable.h.

Keep them ordered to easily find such issues.

...

> +       ret = devm_reset_controller_register(&pdev->dev, &elbar->rcdev);
> +
> +       return ret;

It is simply `return devm_...(...);`. Looking through your patches I
can tell that you may easily drop LoCs by 10%. Please do so in the
next version.

...

> +static const struct of_device_id elba_reset_dt_match[] = {
> +       { .compatible = "amd,pensando-elbasr-reset", },
> +       { /* sentinel */ },

No comma.

> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 15/15] reset: elbasr: Add AMD Pensando Elba SR Reset Controller

[Brad Larson]

Hi Andy,

On Tue, Jun 14, 2022 at 4:47 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Jun 13, 2022 at 9:57 PM Brad Larson <brad@pensando.io> wrote:
> >
> > From: Brad Larson <blarson@amd.com>
> >
> > This patch adds the reset controller functionality for the
> > AMD Pensando Elba System Resource Chip.
>
> ...
>
> > +#include <linux/mfd/pensando-elbasr.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reset-controller.h>
> > +#include <linux/regmap.h>
> > +#include <linux/err.h>
>
> > +#include <linux/of.h>
>
> There is no user of this header. But there are missed ones, such as
> mod_devicetable.h.
>
> Keep them ordered to easily find such issues.

Removed of.h and added mod_devicetable.h.

> ...
> > +       ret = devm_reset_controller_register(&pdev->dev, &elbar->rcdev);
> > +
> > +       return ret;
>
> It is simply `return devm_...(...);`. Looking through your patches I
> can tell that you may easily drop LoCs by 10%. Please do so in the
> next version.

Changed to return devm...(...)

> ...
>
> > +static const struct of_device_id elba_reset_dt_match[] = {
> > +       { .compatible = "amd,pensando-elbasr-reset", },
> > +       { /* sentinel */ },
>
> No comma.

Removed comma

Regards,
Brad

Re: [PATCH v5 15/15] reset: elbasr: Add AMD Pensando Elba SR Reset Controller

[Philipp Zabel]

Hi Brad,

On Mo, 2022-06-13 at 12:56 -0700, Brad Larson wrote:
> From: Brad Larson <blarson@amd.com>
> 
> This patch adds the reset controller functionality for the
> AMD Pensando Elba System Resource Chip.
> 
> Signed-off-by: Brad Larson <blarson@amd.com>
[...]
> diff --git a/drivers/reset/reset-elbasr.c b/drivers/reset/reset-elbasr.c
> new file mode 100644
> index 000000000000..6e429cb11466
> --- /dev/null
> +++ b/drivers/reset/reset-elbasr.c
> @@ -0,0 +1,94 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2022 AMD Pensando
> + */
> +
> +#include <linux/mfd/pensando-elbasr.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/regmap.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +
> +#include <dt-bindings/reset/amd,pensando-elba-reset.h>
> +
> +struct elbasr_reset {
> +	struct reset_controller_dev rcdev;
> +	struct regmap *regmap;
> +};
> +
> +static inline struct elbasr_reset *to_elbasr_rst(struct reset_controller_dev *rc)
> +{
> +	return container_of(rc, struct elbasr_reset, rcdev);
> +}
> +
> +static inline int elbasr_reset_shift(unsigned long id)
> +{
> +	switch (id) {
> +	case EMMC_HW_RESET:

Are there more reset controls than EMMC_HW_RESET?
If so, please list them all.
If not, why is this a function with a switch statement for a single
reset bit?

> +		return 6;
> +	default:
> +		return -EINVAL;

The error return value is never checked.
This can't be reached, since ELBASR_NR_RESETS == 1. So id will only
ever be 0.

> +static int elbasr_reset_probe(struct platform_device *pdev)
> +{
> +	struct elbasr_data *elbasr = dev_get_drvdata(pdev->dev.parent);

Peeking into the MFD driver's private data structure seems unnecessary.
Consider using dev_get_regmap() instead.


regards
Philipp

Re: [PATCH v5 15/15] reset: elbasr: Add AMD Pensando Elba SR Reset Controller

[Brad Larson]

Hi Philipp,

On Tue, Jun 14, 2022 at 7:49 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> Hi Brad,
>
> On Mo, 2022-06-13 at 12:56 -0700, Brad Larson wrote:
> > From: Brad Larson <blarson@amd.com>
> >
> > This patch adds the reset controller functionality for the
> > AMD Pensando Elba System Resource Chip.
> >
> > Signed-off-by: Brad Larson <blarson@amd.com>
> [...]
> > diff --git a/drivers/reset/reset-elbasr.c b/drivers/reset/reset-elbasr.c
> ...
> > +static inline int elbasr_reset_shift(unsigned long id)
> > +{
> > +     switch (id) {
> > +     case EMMC_HW_RESET:
>
> Are there more reset controls than EMMC_HW_RESET?
> If so, please list them all.
> If not, why is this a function with a switch statement for a single
> reset bit?
>
> > +             return 6;
> > +     default:
> > +             return -EINVAL;

There are others but only emmc hardware reset is currently needed/used.  Removed
the switch and just using BIT(6) and removed file amd,pensando-elba-reset.h.

> The error return value is never checked.
> This can't be reached, since ELBASR_NR_RESETS == 1. So id will only
> ever be 0.
>
> > +static int elbasr_reset_probe(struct platform_device *pdev)
> > +{
> > +     struct elbasr_data *elbasr = dev_get_drvdata(pdev->dev.parent);
>
> Peeking into the MFD driver's private data structure seems unnecessary.
> Consider using dev_get_regmap() instead.

Prefer to keep it this way as it follows the approach of existing
driver reset-a10sr.c

Regards,
Brad

Re: [PATCH v5 15/15] reset: elbasr: Add AMD Pensando Elba SR Reset Controller

[Rob Herring]

On Mon, Jun 13, 2022 at 12:56:58PM -0700, Brad Larson wrote:
> From: Brad Larson <blarson@amd.com>
> 
> This patch adds the reset controller functionality for the
> AMD Pensando Elba System Resource Chip.
> 
> Signed-off-by: Brad Larson <blarson@amd.com>
> ---
>  drivers/reset/Kconfig                         |  9 ++
>  drivers/reset/Makefile                        |  1 +
>  drivers/reset/reset-elbasr.c                  | 94 +++++++++++++++++++
>  .../reset/amd,pensando-elba-reset.h           | 11 +++

This goes with the binding patch

>  4 files changed, 115 insertions(+)
>  create mode 100644 drivers/reset/reset-elbasr.c
>  create mode 100644 include/dt-bindings/reset/amd,pensando-elba-reset.h
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 93c8d07ee328..13f5a8ca0f03 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -66,6 +66,15 @@ config RESET_BRCMSTB_RESCAL
>  	  This enables the RESCAL reset controller for SATA, PCIe0, or PCIe1 on
>  	  BCM7216.
>  
> +config RESET_ELBASR
> +	tristate "Pensando Elba System Resource reset controller"
> +	depends on MFD_PENSANDO_ELBASR || COMPILE_TEST
> +	help
> +	  This option enables support for the external reset functions
> +	  on the Pensando Elba System Resource Chip.  Reset control
> +	  of peripherals is accessed over SPI to the system resource
> +	  chip device registers using CS0.
> +
>  config RESET_HSDK
>  	bool "Synopsys HSDK Reset Driver"
>  	depends on HAS_IOMEM
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index a80a9c4008a7..c0fe12b9950e 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_RESET_BCM6345) += reset-bcm6345.o
>  obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
>  obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o
>  obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
> +obj-$(CONFIG_RESET_ELBASR) += reset-elbasr.o
>  obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
>  obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
>  obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
> diff --git a/drivers/reset/reset-elbasr.c b/drivers/reset/reset-elbasr.c
> new file mode 100644
> index 000000000000..6e429cb11466
> --- /dev/null
> +++ b/drivers/reset/reset-elbasr.c
> @@ -0,0 +1,94 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)

Kernel code is GPL-2.0-only generally.

Re: [PATCH v5 15/15] reset: elbasr: Add AMD Pensando Elba SR Reset Controller

[Brad Larson]

Hi Rob,

On Tue, Jun 14, 2022 at 2:34 PM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Jun 13, 2022 at 12:56:58PM -0700, Brad Larson wrote:
> > From: Brad Larson <blarson@amd.com>
> >
> > This patch adds the reset controller functionality for the
> > AMD Pensando Elba System Resource Chip.
> >
> > Signed-off-by: Brad Larson <blarson@amd.com>
> > ---
> >  drivers/reset/Kconfig                         |  9 ++
> >  drivers/reset/Makefile                        |  1 +
> >  drivers/reset/reset-elbasr.c                  | 94 +++++++++++++++++++
> >  .../reset/amd,pensando-elba-reset.h           | 11 +++
>
> This goes with the binding patch

I must have misinterpreted an earlier request to put the bindings
separately up front in the patch set.  For a new driver the binding
and driver should be in one patch which I'll change for the next version.

> ...
> > --- /dev/null
> > +++ b/drivers/reset/reset-elbasr.c
> > @@ -0,0 +1,94 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>
> Kernel code is GPL-2.0-only generally.

Did something change versus earlier request for dual license?

> Re: [PATCH v3 11/11] arm64: dts: Add Pensando Elba SoC support
> - by Rob Herring @ 2021-10-27 21:37 UTC [8%]

> > +// SPDX-License-Identifier: GPL-2.0

> Do you care about using with non-GPL OS? Dual license is preferred.

Regards,
Brad