diff mbox series

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

Message ID 20230119035136.21603-7-blarson@amd.com
State New
Headers
Series Support AMD Pensando Elba SoC |

Commit Message

Brad Larson Jan. 19, 2023, 3:51 a.m. UTC 
  Add support for the AMD Pensando SoC System Resource chip using the
SPI interface.  The device functions are accessed using four
chip-selects.  This device is present for all Pensando SoC designs.

Signed-off-by: Brad Larson <blarson@amd.com>
---

Changes since v6:
- Instead of four nodes, one per chip-select, a single
  node is used with reset-cells in the parent.
- No MFD API is used anymore in the driver so it made
  sense to move this to drivers/spi.
- This driver is common for all Pensando SoC based designs
  so changed the name to pensando-sr.c to not make it Elba
  SoC specific.
- Added property cs for the chip-select number which is used
  by the driver to create /dev/pensr0.<cs> 

---
 .../bindings/spi/amd,pensando-sr.yaml         | 68 +++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/amd,pensando-sr.yaml

Comments

Krzysztof Kozlowski Jan. 19, 2023, 7:58 a.m. UTC | #1 
On 19/01/2023 04:51, Brad Larson wrote:
> Add support for the AMD Pensando SoC System Resource chip using the
> SPI interface.  The device functions are accessed using four
> chip-selects.  This device is present for all Pensando SoC designs.
> 
> Signed-off-by: Brad Larson <blarson@amd.com>
> ---
> 
> Changes since v6:
> - Instead of four nodes, one per chip-select, a single
>   node is used with reset-cells in the parent.
> - No MFD API is used anymore in the driver so it made
>   sense to move this to drivers/spi.
> - This driver is common for all Pensando SoC based designs
>   so changed the name to pensando-sr.c to not make it Elba
>   SoC specific.
> - Added property cs for the chip-select number which is used
>   by the driver to create /dev/pensr0.<cs> 
> 
> ---
>  .../bindings/spi/amd,pensando-sr.yaml         | 68 +++++++++++++++++++
>  1 file changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/amd,pensando-sr.yaml
> 
> diff --git a/Documentation/devicetree/bindings/spi/amd,pensando-sr.yaml b/Documentation/devicetree/bindings/spi/amd,pensando-sr.yaml
> new file mode 100644
> index 000000000000..8504652f6e19
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/amd,pensando-sr.yaml
> @@ -0,0 +1,68 @@
> +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spi/amd,pensando-sr.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: AMD Pensando SoC Resource Controller
> +
> +description: |
> +  AMD Pensando SoC Resource Controller is a set of
> +  control/status registers accessed on four chip-selects.
> +  This device is present in all Pensando SoC based designs.
> +
> +maintainers:
> +  - Brad Larson <blarson@amd.com>
> +
> +properties:
> +  compatible:
> +    contains:

That's not correct syntax. Please start from existing schema or
example-schema. Drop contains.

> +      enum:
> +        - amd,pensando-sr
> +
> +  reg:
> +    minItems: 1

maxItems. Which example or existing schema pointed you to use minItems?

> +
> +  cs:
> +    minItems: 1
> +    maxItems: 4
> +    description:
> +      Device chip select

Drop entire property. Isn't reg for this on SPI bus?

> +
> +  '#reset-cells':
> +    const: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  spi-max-frequency: true

Drop. Missing reference to spi-peripheral-props.

> +
> +required:
> +  - compatible
> +  - cs
> +  - spi-max-frequency
> +  - '#reset-cells'
> +
> +unevaluatedProperties: false

This does not make sense on its own. It works with additional ref. When
you add ref to spi props, it will be fine. But without it you should use
additionalProperties: false.


Best regards,
Krzysztof
Lee Jones Jan. 19, 2023, 2:17 p.m. UTC | #2 
On Wed, 18 Jan 2023, Brad Larson wrote:

> Add support for the AMD Pensando SoC System Resource chip using the
> SPI interface.  The device functions are accessed using four
> chip-selects.  This device is present for all Pensando SoC designs.
> 
> Signed-off-by: Brad Larson <blarson@amd.com>
> ---
> 
> Changes since v6:
> - Instead of four nodes, one per chip-select, a single
>   node is used with reset-cells in the parent.
> - No MFD API is used anymore in the driver so it made
>   sense to move this to drivers/spi.
> - This driver is common for all Pensando SoC based designs
>   so changed the name to pensando-sr.c to not make it Elba
>   SoC specific.
> - Added property cs for the chip-select number which is used
>   by the driver to create /dev/pensr0.<cs> 
> 
> ---
>  .../bindings/spi/amd,pensando-sr.yaml         | 68 +++++++++++++++++++
>  1 file changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/amd,pensando-sr.yaml

Please change the subject line - this doesn't appear to have anything to
do with MFD.
Brad Larson Jan. 26, 2023, 2:59 a.m. UTC | #3 
>> diff --git a/Documentation/devicetree/bindings/spi/amd,pensando-sr.yaml b/Documentation/devicetree/bindings/spi/amd,pensando-sr.yaml
>> new file mode 100644
>> index 000000000000..8504652f6e19
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/spi/amd,pensando-sr.yaml
>> @@ -0,0 +1,68 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/spi/amd,pensando-sr.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: AMD Pensando SoC Resource Controller
>> +
>> +description: |
>> +  AMD Pensando SoC Resource Controller is a set of
>> +  control/status registers accessed on four chip-selects.
>> +  This device is present in all Pensando SoC based designs.
>> +
>> +maintainers:
>> +  - Brad Larson <blarson@amd.com>
>> +
>> +properties:
>> +  compatible:
>> +    contains:
>
> That's not correct syntax. Please start from existing schema or
> example-schema. Drop contains.

Fixed, see update below.

>> +      enum:
>> +        - amd,pensando-sr
>> +
>> +  reg:
>> +    minItems: 1
>
> maxItems. Which example or existing schema pointed you to use minItems?

Should have been maxItems.  cs below is dropped and reg is used
as discussed for the chip selects but throws a too long error, see below.

>> +
>> +  cs:
>> +    minItems: 1
>> +    maxItems: 4
>> +    description:
>> +      Device chip select
>
> Drop entire property. Isn't reg for this on SPI bus?

Dropped and using reg, results in too long error for schema snps,dw-apb-ssi.yaml

>> +
>> +  '#reset-cells':
>> +    const: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  spi-max-frequency: true
>
>Drop. Missing reference to spi-peripheral-props.

Removed and added spi-peripheral-props

>> +
>> +required:
>> +  - compatible
>> +  - cs
>> +  - spi-max-frequency
>> +  - '#reset-cells'
>> +
>> +unevaluatedProperties: false
>
> This does not make sense on its own. It works with additional ref. When
> you add ref to spi props, it will be fine. But without it you should use
> additionalProperties: false.

The updated binding

--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/amd,pensando-sr.yaml
@@ -0,0 +1,68 @@
+# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/amd,pensando-sr.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AMD Pensando SoC Resource Controller
+
+description: |
+  AMD Pensando SoC Resource Controller is a set of control/status
+  registers accessed on four chip-selects.  This device is present
+  in all Pensando SoC based designs.
+
+  CS0 is a set of miscellaneous control/status registers to
+  include reset control.  CS1/CS2 are for I2C peripherals.
+  CS3 is to access resource controller internal storage.
+
+maintainers:
+  - Brad Larson <blarson@amd.com>
+
+properties:
+  compatible:
+    const: amd,pensando-sr
+
+  reg:
+    maxItems: 4
+    minimum: 0
+    maximum: 3
+    description:
+      Device chip select number
+
+  '#reset-cells':
+    const: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - spi-max-frequency
+  - '#reset-cells'
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        num-cs = <4>;
+
+        system-controller@0 {
+            compatible = "amd,pensando-sr";
+            reg = <0 1 2 3>;
+            spi-max-frequency = <12000000>;
+            interrupt-parent = <&porta>;
+            interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
+            #reset-cells = <1>;
+        };
+    };
+
+...

any guidance on fixing the following?

$ make ARCH=arm64 dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
  LINT    Documentation/devicetree/bindings
  CHKDT   Documentation/devicetree/bindings/processed-schema.json
  SCHEMA  Documentation/devicetree/bindings/processed-schema.json
  DTC_CHK arch/arm64/boot/dts/amd/elba-asic.dtb
/home/brad/linux.v10/arch/arm64/boot/dts/amd/elba-asic.dtb: spi@2800: system-controller@0:reg: [[0], [1], [2], [3]] is too long
	From schema: /home/brad/linux.v10/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml

where the pieces are

arch/arm64/boot/dts/amd/elba.dtsi

                spi0: spi@2800 {
                        compatible = "amd,pensando-elba-spi";
                        reg = <0x0 0x2800 0x0 0x100>;
                        #address-cells = <1>;
                        #size-cells = <0>;
                        amd,pensando-elba-syscon = <&syscon>;
                        clocks = <&ahb_clk>;
                        interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
                        num-cs = <2>;
                        status = "disabled";
                };

                syscon: syscon@307c0000 {
                        compatible = "amd,pensando-elba-syscon", "syscon";
                        reg = <0x0 0x307c0000 0x0 0x3000>;
                };

arch/arm64/boot/dts/amd/elba-asic-common.dtsi

&spi0 {
        #address-cells = <1>;
        #size-cells = <0>;
        num-cs = <4>;
        cs-gpios = <0>, <0>, <&porta 1 GPIO_ACTIVE_LOW>,
                   <&porta 7 GPIO_ACTIVE_LOW>;
        status = "okay";

        rstc: system-controller@0 {
                compatible = "amd,pensando-sr";
                reg = <0 1 2 3>;
                spi-max-frequency = <12000000>;
                interrupt-parent = <&porta>;
                interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
                #reset-cells = <1>;
        };
};

Also should the driver for this SPI device used for every Pensando SoC 
be in drivers/misc, drivers/spi?  Didn't make sense to leave it in 
drivers/mfd once the resets was squashed in the parent and only one n
ode with reg setting which chip selects result in creation of /dev/pensr0.<cs>.  

Regards,
Brad
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/spi/amd,pensando-sr.yaml b/Documentation/devicetree/bindings/spi/amd,pensando-sr.yaml
new file mode 100644
index 000000000000..8504652f6e19
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/amd,pensando-sr.yaml
@@ -0,0 +1,68 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/amd,pensando-sr.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AMD Pensando SoC Resource Controller
+
+description: |
+  AMD Pensando SoC Resource Controller is a set of
+  control/status registers accessed on four chip-selects.
+  This device is present in all Pensando SoC based designs.
+
+maintainers:
+  - Brad Larson <blarson@amd.com>
+
+properties:
+  compatible:
+    contains:
+      enum:
+        - amd,pensando-sr
+
+  reg:
+    minItems: 1
+
+  cs:
+    minItems: 1
+    maxItems: 4
+    description:
+      Device chip select
+
+  '#reset-cells':
+    const: 1
+
+  interrupts:
+    maxItems: 1
+
+  spi-max-frequency: true
+
+required:
+  - compatible
+  - cs
+  - spi-max-frequency
+  - '#reset-cells'
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        num-cs = <4>;
+
+        system-controller@0 {
+            compatible = "amd,pensando-sr";
+            reg = <0>;
+            cs = <0 1 2 3>;
+            spi-max-frequency = <12000000>;
+            interrupt-parent = <&porta>;
+            interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
+            #reset-cells = <1>;
+        };
+    };
+
+...