| Message ID | 20200513140031.25633-7-lars.povlsen@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: Adding support for Microchip Sparx5 SoC | expand |
On Wed, May 13, 2020 at 04:00:27PM +0200, Lars Povlsen wrote: > This add DT bindings for the Sparx5 SPI driver. It makes life much easier if you add new features to the bindings before doing any conversions to YAML, YAML binding conversions can take a long time to get reviewed and really slow things up. > + - description: Direct mapped SPI read area. If provided, the > + driver will register spi_mem_op's to take advantage of it. The implementation detail about what the driver does with this is not relevant to the binding. > + interface-mapping-mask: > + description: | > + On the Sparx5 variant, two different busses are connected to the > + controller. This property is a mask per chip-select, indicating > + whether the CS should go to one or the other interface. > + $ref: /schemas/types.yaml#/definitions/uint32 > + default: 0 > + maxItems: 1 Is this not a property that should go on devices? It looks like this needs some namespacing too.
On Wed, May 13, 2020 at 04:00:27PM +0200, Lars Povlsen wrote: > This add DT bindings for the Sparx5 SPI driver. This whole file can be easily merged in to the generic DW APB SSI DT binding file. Just use "if: properties: compatible: const: ..." construction to distinguish ocelot, jaguar, sparx5 and non-sparx5 nodes. > > Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com> > --- > .../bindings/spi/mscc,ocelot-spi.yaml | 49 +++++++++++++++---- > 1 file changed, 39 insertions(+), 10 deletions(-) > > diff --git a/Documentation/devicetree/bindings/spi/mscc,ocelot-spi.yaml b/Documentation/devicetree/bindings/spi/mscc,ocelot-spi.yaml > index a3ac0fa576553..8beecde4b0880 100644 > --- a/Documentation/devicetree/bindings/spi/mscc,ocelot-spi.yaml > +++ b/Documentation/devicetree/bindings/spi/mscc,ocelot-spi.yaml > @@ -23,15 +23,23 @@ properties: > enum: > - mscc,ocelot-spi > - mscc,jaguar2-spi > + - microchip,sparx5-spi > > interrupts: > maxItems: 1 > > reg: > minItems: 2 > - items: > - - description: Designware SPI registers > - - description: CS override registers > + maxItems: 3 > + oneOf: > + - items: > + - description: Designware SPI registers > + - description: CS override registers (Not sparx5). > + - items: > + - description: Designware SPI registers > + - description: CS override registers (Not sparx5). > + - description: Direct mapped SPI read area. If provided, the > + driver will register spi_mem_op's to take advantage of it. > > clocks: > maxItems: 1 > @@ -43,6 +51,23 @@ properties: > enum: [ 2, 4 ] > maxItems: 1 > > + spi-rx-delay-us: > + description: | > + The delay (in usec) of the RX signal sample position. This can > + be used to tne the RX timing in order to acheive higher > + speeds. This is used for all devices on the bus. > + default: 0 > + maxItems: 1 spi-rx-delay-us is defined for a particular SPI-slave. Please see the DT binding file: Documentation/devicetree/bindings/spi/spi-controller.yaml . Although as I suggested before this delay isn't what the Dw APB SSI RX sample delay functionality does. Probably a vendor-specific property would be better here. But I'd also define it on a SPI-slave basis, not for all devices on the bus. > + > + interface-mapping-mask: > + description: | > + On the Sparx5 variant, two different busses are connected to the > + controller. This property is a mask per chip-select, indicating > + whether the CS should go to one or the other interface. > + $ref: /schemas/types.yaml#/definitions/uint32 > + default: 0 > + maxItems: 1 As Mark rightfully suggested this seems like an SPI-slave related property, then most likely it should be defined on the SPI-slave basis (probably as a bool property). Additionally it's vendor-specific, so the property name should be accordingly prefixed. > + > required: > - compatible > - reg > @@ -50,11 +75,15 @@ required: > > examples: > - | > - spi0: spi@101000 { > - compatible = "mscc,ocelot-spi"; > - #address-cells = <1>; > - #size-cells = <0>; > - reg = <0x101000 0x100>, <0x3c 0x18>; > - interrupts = <9>; > - clocks = <&ahb_clk>; > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + spi0: spi@600104000 { > + #address-cells = <1>; > + #size-cells = <0>; > + compatible = "microchip,sparx5-spi"; > + reg = <0x00104000 0x40>, <0 0>, <0x3000000 0x4000000>; I have a doubt that defining an empty reg region is a good idea, since you can detect the reg requirements by the node compatible string. -Sergey > + num-cs = <16>; > + reg-io-width = <4>; > + reg-shift = <2>; > + clocks = <&ahb_clk>; > + interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>; > }; > -- > 2.26.2 > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Serge Semin writes: > On Wed, May 13, 2020 at 04:00:27PM +0200, Lars Povlsen wrote: >> This add DT bindings for the Sparx5 SPI driver. > > This whole file can be easily merged in to the generic DW APB SSI DT > binding file. Just use "if: properties: compatible: const: ..." construction > to distinguish ocelot, jaguar, sparx5 and non-sparx5 nodes. > >> >> Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com> >> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com> >> --- >> .../bindings/spi/mscc,ocelot-spi.yaml | 49 +++++++++++++++---- >> 1 file changed, 39 insertions(+), 10 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/spi/mscc,ocelot-spi.yaml b/Documentation/devicetree/bindings/spi/mscc,ocelot-spi.yaml >> index a3ac0fa576553..8beecde4b0880 100644 >> --- a/Documentation/devicetree/bindings/spi/mscc,ocelot-spi.yaml >> +++ b/Documentation/devicetree/bindings/spi/mscc,ocelot-spi.yaml >> @@ -23,15 +23,23 @@ properties: >> enum: >> - mscc,ocelot-spi >> - mscc,jaguar2-spi >> + - microchip,sparx5-spi >> >> interrupts: >> maxItems: 1 >> >> reg: >> minItems: 2 >> - items: >> - - description: Designware SPI registers >> - - description: CS override registers >> + maxItems: 3 >> + oneOf: >> + - items: >> + - description: Designware SPI registers >> + - description: CS override registers (Not sparx5). >> + - items: >> + - description: Designware SPI registers >> + - description: CS override registers (Not sparx5). >> + - description: Direct mapped SPI read area. If provided, the >> + driver will register spi_mem_op's to take advantage of it. >> >> clocks: >> maxItems: 1 >> @@ -43,6 +51,23 @@ properties: >> enum: [ 2, 4 ] >> maxItems: 1 >> > >> + spi-rx-delay-us: >> + description: | >> + The delay (in usec) of the RX signal sample position. This can >> + be used to tne the RX timing in order to acheive higher >> + speeds. This is used for all devices on the bus. >> + default: 0 >> + maxItems: 1 > > spi-rx-delay-us is defined for a particular SPI-slave. Please see the > DT binding file: Documentation/devicetree/bindings/spi/spi-controller.yaml . > Although as I suggested before this delay isn't what the Dw APB SSI RX sample > delay functionality does. Probably a vendor-specific property would be better > here. But I'd also define it on a SPI-slave basis, not for all devices on the > bus. Right, I was hunting for something "similar". As pointed out, this is really different in nature, and the unit is also too coarse. I will change this to "snps,rx-sample-delay-ns" as suggested in your other comments. > >> + >> + interface-mapping-mask: >> + description: | >> + On the Sparx5 variant, two different busses are connected to the >> + controller. This property is a mask per chip-select, indicating >> + whether the CS should go to one or the other interface. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + default: 0 >> + maxItems: 1 > > As Mark rightfully suggested this seems like an SPI-slave related property, then > most likely it should be defined on the SPI-slave basis (probably as a bool > property). Additionally it's vendor-specific, so the property name should be > accordingly prefixed. Yes, I'll change this to a per-device property. I need the same for the above as well. > >> + >> required: >> - compatible >> - reg >> @@ -50,11 +75,15 @@ required: >> >> examples: >> - | >> - spi0: spi@101000 { >> - compatible = "mscc,ocelot-spi"; >> - #address-cells = <1>; >> - #size-cells = <0>; >> - reg = <0x101000 0x100>, <0x3c 0x18>; >> - interrupts = <9>; >> - clocks = <&ahb_clk>; >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + spi0: spi@600104000 { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + compatible = "microchip,sparx5-spi"; > >> + reg = <0x00104000 0x40>, <0 0>, <0x3000000 0x4000000>; > > I have a doubt that defining an empty reg region is a good idea, since you can > detect the reg requirements by the node compatible string. Yes, its probably better that way. It looks ugly too :-) Thanks for your comments! ---Lars > -Sergey > >> + num-cs = <16>; >> + reg-io-width = <4>; >> + reg-shift = <2>; >> + clocks = <&ahb_clk>; >> + interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>; >> }; >> -- >> 2.26.2 >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/Documentation/devicetree/bindings/spi/mscc,ocelot-spi.yaml b/Documentation/devicetree/bindings/spi/mscc,ocelot-spi.yaml index a3ac0fa576553..8beecde4b0880 100644 --- a/Documentation/devicetree/bindings/spi/mscc,ocelot-spi.yaml +++ b/Documentation/devicetree/bindings/spi/mscc,ocelot-spi.yaml @@ -23,15 +23,23 @@ properties: enum: - mscc,ocelot-spi - mscc,jaguar2-spi + - microchip,sparx5-spi interrupts: maxItems: 1 reg: minItems: 2 - items: - - description: Designware SPI registers - - description: CS override registers + maxItems: 3 + oneOf: + - items: + - description: Designware SPI registers + - description: CS override registers (Not sparx5). + - items: + - description: Designware SPI registers + - description: CS override registers (Not sparx5). + - description: Direct mapped SPI read area. If provided, the + driver will register spi_mem_op's to take advantage of it. clocks: maxItems: 1 @@ -43,6 +51,23 @@ properties: enum: [ 2, 4 ] maxItems: 1 + spi-rx-delay-us: + description: | + The delay (in usec) of the RX signal sample position. This can + be used to tne the RX timing in order to acheive higher + speeds. This is used for all devices on the bus. + default: 0 + maxItems: 1 + + interface-mapping-mask: + description: | + On the Sparx5 variant, two different busses are connected to the + controller. This property is a mask per chip-select, indicating + whether the CS should go to one or the other interface. + $ref: /schemas/types.yaml#/definitions/uint32 + default: 0 + maxItems: 1 + required: - compatible - reg @@ -50,11 +75,15 @@ required: examples: - | - spi0: spi@101000 { - compatible = "mscc,ocelot-spi"; - #address-cells = <1>; - #size-cells = <0>; - reg = <0x101000 0x100>, <0x3c 0x18>; - interrupts = <9>; - clocks = <&ahb_clk>; + #include <dt-bindings/interrupt-controller/arm-gic.h> + spi0: spi@600104000 { + #address-cells = <1>; + #size-cells = <0>; + compatible = "microchip,sparx5-spi"; + reg = <0x00104000 0x40>, <0 0>, <0x3000000 0x4000000>; + num-cs = <16>; + reg-io-width = <4>; + reg-shift = <2>; + clocks = <&ahb_clk>; + interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>; };