Re: [PATCH 06/10] dt-bindings: spi: spi-dw-mchp: Add Sparx5 support

[Lars Povlsen <lars.povlsen@xxxxxxxxxxxxx>]

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@xxxxxxxxxxx>
>> Signed-off-by: Lars Povlsen <lars.povlsen@xxxxxxxxxxxxx>
>> ---
>>  .../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@xxxxxxxxxxxxxxxxxxx
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Lars Povlsen,
Microchip