diff mbox series

[v3,5/8] dt-bindings: snps,dw-apb-ssi: Add sparx5 support, plus snps,rx-sample-delay-ns property

Message ID 20200702101331.26375-6-lars.povlsen@microchip.com
State Changes Requested
Headers show
Series spi: Adding support for Microchip Sparx5 SoC | expand

Checks

Context Check Description
robh/dt-meta-schema success
robh/checkpatch success

Commit Message

Lars Povlsen July 2, 2020, 10:13 a.m. UTC
This has the following changes for the snps,dw-apb-ss DT bindings:

- Add "microchip,sparx5-spi" as the compatible for the Sparx5 SoC
  controller

- Add the property "mux-controls" for the above compatible string

- Add the property "snps,rx-sample-delay-ns" for SPI slaves

Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
---
 .../bindings/spi/snps,dw-apb-ssi.yaml         | 28 +++++++++++++++++++
 1 file changed, 28 insertions(+)

--
2.27.0

Comments

Rob Herring July 13, 2020, 7:22 p.m. UTC | #1
On Thu, Jul 02, 2020 at 12:13:28PM +0200, Lars Povlsen wrote:
> This has the following changes for the snps,dw-apb-ss DT bindings:
> 
> - Add "microchip,sparx5-spi" as the compatible for the Sparx5 SoC
>   controller
> 
> - Add the property "mux-controls" for the above compatible string
> 
> - Add the property "snps,rx-sample-delay-ns" for SPI slaves
> 
> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
> ---
>  .../bindings/spi/snps,dw-apb-ssi.yaml         | 28 +++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> index c62cbe79f00dd..9d9208391fae3 100644
> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> @@ -36,6 +36,8 @@ properties:
>                - mscc,ocelot-spi
>                - mscc,jaguar2-spi
>            - const: snps,dw-apb-ssi
> +      - description: Microchip Sparx5 SoC SPI Controller
> +        const: microchip,sparx5-spi
>        - description: Amazon Alpine SPI Controller
>          const: amazon,alpine-dw-apb-ssi
>        - description: Renesas RZ/N1 SPI Controller
> @@ -93,6 +95,19 @@ properties:
>        - const: tx
>        - const: rx
> 
> +if:
> +  properties:
> +    compatible:
> +      contains:
> +        const: microchip,sparx5-spi
> +
> +then:
> +  properties:
> +    mux-controls:
> +      description: A mux controller node for selecting SPI bus interface.
> +      maxItems: 1
> +      $ref: '/schemas/types.yaml#/definitions/phandle'

Can drop the type. You can assume common properties already have a 
defined type.

> +
>  patternProperties:
>    "^.*@[0-9a-f]+$":
>      type: object
> @@ -107,6 +122,14 @@ patternProperties:
>        spi-tx-bus-width:
>          const: 1
> 
> +      snps,rx-sample-delay-ns:

We already have 'rx-sample-delay-ns' from Rockchip SPI, so use that. But 
note that it applies to the SPI node. Does this need to be per SPI 
child?

BTW, the Rockchip controller appears to be a version of the DW 
controller.

> +        description: SPI Rx sample delay offset, unit is nanoseconds.
> +          The delay from the default sample time before the actual
> +          sample of the rxd input signal occurs. The "rx_sample_delay"
> +          is an optional feature of the designware controller, and the
> +          upper limit is also subject to controller configuration.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +
>  unevaluatedProperties: false
> 
>  required:
> @@ -129,5 +152,10 @@ examples:
>        num-cs = <2>;
>        cs-gpios = <&gpio0 13 0>,
>                   <&gpio0 14 0>;
> +      spi-flash@1 {
> +        compatible = "spi-nand";
> +        reg = <1>;
> +        snps,rx-sample-delay-ns = <7>;
> +      };
>      };
>  ...
> --
> 2.27.0
Serge Semin July 13, 2020, 7:52 p.m. UTC | #2
On Mon, Jul 13, 2020 at 01:22:59PM -0600, Rob Herring wrote:
> On Thu, Jul 02, 2020 at 12:13:28PM +0200, Lars Povlsen wrote:
...
> 
> > +
> >  patternProperties:
> >    "^.*@[0-9a-f]+$":
> >      type: object
> > @@ -107,6 +122,14 @@ patternProperties:
> >        spi-tx-bus-width:
> >          const: 1
> > 
> > +      snps,rx-sample-delay-ns:
> 

> We already have 'rx-sample-delay-ns' from Rockchip SPI, so use that. But 
> note that it applies to the SPI node. Does this need to be per SPI 
> child?

It was me, who suggested to Lars to have that parameter moved to the SPI
sub-nodes. As I see it the property is highly dependent on the SPI slave device
the controller is communicating to. Some of the them may need the delay some
may not. It's not always the capacitance thing, but also depends on how good
the MISO signal a particular slave generates. So IMO the Rockchip SPI driver
developer should have moved that property to the sub-nodes too.

On the other hand if the Rx errors are caused by the MISO lane capacitance,
then it will be cumbersome to have the same property duplicated for each
sub-node. Then what about having the property supported by both the SPI
controller and the SPI-child nodes? For instance the SPI-controller
"rx-sample-delay-ns" will provide a default sample delay for each sub-node
instead of zero by default, while the individual sub-node "rx-sample-delay-ns"
property can be used to override the default value.

-Sergey

> 
> BTW, the Rockchip controller appears to be a version of the DW 
> controller.
> 
> > +        description: SPI Rx sample delay offset, unit is nanoseconds.
> > +          The delay from the default sample time before the actual
> > +          sample of the rxd input signal occurs. The "rx_sample_delay"
> > +          is an optional feature of the designware controller, and the
> > +          upper limit is also subject to controller configuration.
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +
> >  unevaluatedProperties: false
> > 
> >  required:
> > @@ -129,5 +152,10 @@ examples:
> >        num-cs = <2>;
> >        cs-gpios = <&gpio0 13 0>,
> >                   <&gpio0 14 0>;
> > +      spi-flash@1 {
> > +        compatible = "spi-nand";
> > +        reg = <1>;
> > +        snps,rx-sample-delay-ns = <7>;
> > +      };
> >      };
> >  ...
> > --
> > 2.27.0
Lars Povlsen July 14, 2020, 8:30 a.m. UTC | #3
Serge Semin writes:

> On Mon, Jul 13, 2020 at 01:22:59PM -0600, Rob Herring wrote:
>> On Thu, Jul 02, 2020 at 12:13:28PM +0200, Lars Povlsen wrote:
> ...
>>
>> > +
>> >  patternProperties:
>> >    "^.*@[0-9a-f]+$":
>> >      type: object
>> > @@ -107,6 +122,14 @@ patternProperties:
>> >        spi-tx-bus-width:
>> >          const: 1
>> >
>> > +      snps,rx-sample-delay-ns:
>>
>
>> We already have 'rx-sample-delay-ns' from Rockchip SPI, so use that. But
>> note that it applies to the SPI node. Does this need to be per SPI
>> child?
>
> It was me, who suggested to Lars to have that parameter moved to the SPI
> sub-nodes. As I see it the property is highly dependent on the SPI slave device
> the controller is communicating to. Some of the them may need the delay some
> may not. It's not always the capacitance thing, but also depends on how good
> the MISO signal a particular slave generates. So IMO the Rockchip SPI driver
> developer should have moved that property to the sub-nodes too.
>

In the case with sparx5, having two bus interfaces, spi slaves may be on
different busses - making it obvious why you may need different settings.

But I guess even on the same bus the physical length of SPI connections
and device response delay to each device could play in as well.

Nevertheless, it *does* make sense to be able to specify per slave, but
a default could make the DT more terse. Should I add this to my patch?

I will remove the "snps," prefix now that the property is globally
established.

---Lars

> On the other hand if the Rx errors are caused by the MISO lane capacitance,
> then it will be cumbersome to have the same property duplicated for each
> sub-node. Then what about having the property supported by both the SPI
> controller and the SPI-child nodes? For instance the SPI-controller
> "rx-sample-delay-ns" will provide a default sample delay for each sub-node
> instead of zero by default, while the individual sub-node "rx-sample-delay-ns"
> property can be used to override the default value.
>
> -Sergey
>
>>
>> BTW, the Rockchip controller appears to be a version of the DW
>> controller.
>>
>> > +        description: SPI Rx sample delay offset, unit is nanoseconds.
>> > +          The delay from the default sample time before the actual
>> > +          sample of the rxd input signal occurs. The "rx_sample_delay"
>> > +          is an optional feature of the designware controller, and the
>> > +          upper limit is also subject to controller configuration.
>> > +        $ref: /schemas/types.yaml#/definitions/uint32
>> > +
>> >  unevaluatedProperties: false
>> >
>> >  required:
>> > @@ -129,5 +152,10 @@ examples:
>> >        num-cs = <2>;
>> >        cs-gpios = <&gpio0 13 0>,
>> >                   <&gpio0 14 0>;
>> > +      spi-flash@1 {
>> > +        compatible = "spi-nand";
>> > +        reg = <1>;
>> > +        snps,rx-sample-delay-ns = <7>;
>> > +      };
>> >      };
>> >  ...
>> > --
>> > 2.27.0
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
index c62cbe79f00dd..9d9208391fae3 100644
--- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
+++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
@@ -36,6 +36,8 @@  properties:
               - mscc,ocelot-spi
               - mscc,jaguar2-spi
           - const: snps,dw-apb-ssi
+      - description: Microchip Sparx5 SoC SPI Controller
+        const: microchip,sparx5-spi
       - description: Amazon Alpine SPI Controller
         const: amazon,alpine-dw-apb-ssi
       - description: Renesas RZ/N1 SPI Controller
@@ -93,6 +95,19 @@  properties:
       - const: tx
       - const: rx

+if:
+  properties:
+    compatible:
+      contains:
+        const: microchip,sparx5-spi
+
+then:
+  properties:
+    mux-controls:
+      description: A mux controller node for selecting SPI bus interface.
+      maxItems: 1
+      $ref: '/schemas/types.yaml#/definitions/phandle'
+
 patternProperties:
   "^.*@[0-9a-f]+$":
     type: object
@@ -107,6 +122,14 @@  patternProperties:
       spi-tx-bus-width:
         const: 1

+      snps,rx-sample-delay-ns:
+        description: SPI Rx sample delay offset, unit is nanoseconds.
+          The delay from the default sample time before the actual
+          sample of the rxd input signal occurs. The "rx_sample_delay"
+          is an optional feature of the designware controller, and the
+          upper limit is also subject to controller configuration.
+        $ref: /schemas/types.yaml#/definitions/uint32
+
 unevaluatedProperties: false

 required:
@@ -129,5 +152,10 @@  examples:
       num-cs = <2>;
       cs-gpios = <&gpio0 13 0>,
                  <&gpio0 14 0>;
+      spi-flash@1 {
+        compatible = "spi-nand";
+        reg = <1>;
+        snps,rx-sample-delay-ns = <7>;
+      };
     };
 ...