Re: [PATCH v2 01/10] dt-bindings: soc: fsl: cpm_qe: Add TSA controller

[Krzysztof Kozlowski]

On 10/01/2023 09:04, Herve Codina wrote:
> Hi Krzysztof,
> 
> On Sun, 8 Jan 2023 16:10:38 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> 
> [...]
> 
>>> +  '#size-cells':
>>> +    const: 0
>>> +
>>> +patternProperties:
>>> +  "^tdm@[0-1]$":  
>>
>> Use consistent quotes - either ' or "
> 
> Ok, I will change on v3.
> I will also change them on the other bindings present in the
> series.
> 
>>
>>> +    description:
>>> +      The TDM managed by this controller
>>> +    type: object
>>> +
>>> +    properties:
>>> +      reg:
>>> +        minimum: 0
>>> +        maximum: 1
>>> +        description:
>>> +          The TDM number for this TDM, 0 for TDMa and 1 for TDMb
>>> +
>>> +      fsl,common-rxtx-pins:
>>> +        $ref: /schemas/types.yaml#/definitions/flag
>>> +        description:
>>> +          Use common pins for both transmit and receive  
>>
>> What are the "common" pins? Without this property device is using
>> uncommon pins? This does not make sense...
> 
> Common in the "shared" sense.
> The hardware can use dedicated pins for Tx clock, Tx sync,
> Rx clock and Rx sync or use only 2 pins, Tx/Rx clock and
> Rx/Rx sync.
> 
> Without the property, we use the 4 pins and with the property,
> we use 2 pins.

Just use this as description.

> 
>>
>>> +
>>> +      clocks: true
>>> +      clock-names: true  
>>
>> Both need constraints.
> 
> The constraints are present later in the file as the number
> of clocks depends on the 'fsl,common-rxtx-pins' property.

OK, but still top level properties need widest constraints.

> 
> I will remove these two lines in the v3 series as they are
> not needed. 'clocks' and 'clock-names' are handled in the
> conditional part.

No, top level properties must contain them.

> 
>>
> [...]
>>> +
>>> +      fsl,rx-frame-sync-delay:
>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>> +        enum: [0, 1, 2, 3]
>>> +        default: 0
>>> +        description: |
>>> +          Receive frame sync delay.  
>>
>> Delay in what units?
> 
> The unit is a number of bits.
> I will rename to fsl,rx-frame-sync-delay-bits and change the description
> to 'Receive frame sync delay in number of bits'
> 
> I will do also the same for fsl,tx-frame-sync-delay property.

OK

> 
>>
>>> +          Indicates the delay between the Rx sync and the first bit of the
>>> +          Rx frame. 0 for no bit delay. 1, 2 or 3 for 1, 2 or 3 bits delay.
>>> +
>>> +      fsl,tx-frame-sync-delay:
>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>> +        enum: [0, 1, 2, 3]
>>> +        default: 0
>>> +        description: |
>>> +          Transmit frame sync delay.
>>> +          Indicates the delay between the Tx sync and the first bit of the
>>> +          Tx frame. 0 for no bit delay. 1, 2 or 3 for 1, 2 or 3 bits delay.
>>> +
>>> +      fsl,clock-falling-edge:
>>> +        $ref: /schemas/types.yaml#/definitions/flag
>>> +        description: |
>>> +          Data is sent on falling edge of the clock (and received on the
>>> +          rising edge).
>>> +          If 'clock-falling-edge' is not present, data is sent on the
>>> +          rising edge (and received on the falling edge).
>>> +
>>> +      fsl,fsync-rising-edge:
>>> +        $ref: /schemas/types.yaml#/definitions/flag
>>> +        description:
>>> +          Frame sync pulses are sampled with the rising edge of the channel
>>> +          clock. If 'fsync-rising-edge' is not present, pulses are sample
>>> +          with e falling edge.
>>> +
>>> +      fsl,double-speed-clock:
>>> +        $ref: /schemas/types.yaml#/definitions/flag
>>> +        description:
>>> +          The channel clock is twice the data rate.
>>> +
>>> +      fsl,grant-mode:
>>> +        $ref: /schemas/types.yaml#/definitions/flag
>>> +        description:
>>> +          Grant mode enabled.  
>>
>> This we know from property name. You need to describe what it is and
>> what it does.
> 
> Instead of describing it, I will simply remove the property (I should
> have done already).
> I cannot test the 'grant mode' enabled with my hardware and so
> I prefer keeping it disabled.
> This property, if needed, could be add later setting it optional
> with default to 'disabled'.
> 
>>
>>> +
>>> +      tx_ts_routes:  
>>
>> No underscores, missing vendor prefix.
> 
> Indeed, will be change to fsl,tx-ts-routes (idem for rx_ts_routes).
> 
>>
>>> +        $ref: /schemas/types.yaml#/definitions/uint32-matrix
>>> +        description: |
>>> +          A list of tupple that indicates the Tx time-slots routes.
>>> +            tx_ts_routes =
>>> +               < 2 0 0>, /* The first 2 time slots are not used */
>>> +               < 3 1 0>, /* The next 3 ones are route to SCC2 */
>>> +               < 4 0 0>, /* The next 4 ones are not used */
>>> +               < 2 2 0>; /* The nest 2 ones are route to SCC3 */
>>> +        items:
>>> +          items:
>>> +            - description:
>>> +                The number of time-slots
>>> +              minimum: 1
>>> +              maximum: 64
>>> +            - description: |
>>> +                The source serial interface (dt-bindings/soc/fsl-tsa.h
>>> +                defines these values)
>>> +                 - 0: No destination
>>> +                 - 1: SCC2
>>> +                 - 2: SCC3
>>> +                 - 3: SCC4
>>> +                 - 4: SMC1
>>> +                 - 5: SMC2
>>> +              enum: [0, 1, 2, 3, 4, 5]
>>> +            - description:
>>> +                The route flags (reserved)  
>>
>> Why part of binding is reserved?
> 
> The 'reserved' part will be removed in v3.
> Same for the rx route table.
> 
>>
>>> +              const: 0
>>> +        minItems: 1
>>> +        maxItems: 64
>>> +
>>> +      rx_ts_routes:
>>> +        $ref: /schemas/types.yaml#/definitions/uint32-matrix
>>> +        description: |
>>> +          A list of tupple that indicates the Rx time-slots routes.
>>> +            tx_ts_routes =
>>> +               < 2 0 0>, /* The first 2 time slots are not used */
>>> +               < 3 1 0>, /* The next 3 ones are route from SCC2 */
>>> +               < 4 0 0>, /* The next 4 ones are not used */
>>> +               < 2 2 0>; /* The nest 2 ones are route from SCC3 */
>>> +        items:
>>> +          items:
>>> +            - description:
>>> +                The number of time-slots
>>> +              minimum: 1
>>> +              maximum: 64
>>> +            - description: |
>>> +                The destination serial interface (dt-bindings/soc/fsl-tsa.h
>>> +                defines these values)
>>> +                 - 0: No destination
>>> +                 - 1: SCC2
>>> +                 - 2: SCC3
>>> +                 - 3: SCC4
>>> +                 - 4: SMC1
>>> +                 - 5: SMC2
>>> +              enum: [0, 1, 2, 3, 4, 5]
>>> +            - description:
>>> +                The route flags (reserved)
>>> +              const: 0
>>> +        minItems: 1
>>> +        maxItems: 64
>>> +
>>> +    allOf:
>>> +      - if:
>>> +          properties:
>>> +            fsl,common-rxtx-pins:
>>> +              type: 'null'  
>>
>> What is this exactly? If check for property present, it's wrong. Should
>> be test if it is in required.
> 
> Yes, it was a check for the property presence.
> 
> If we not use the 'fsl,common-rxtx-pins', we need 4 clocks.
> If we use the 'fsl,common-rxtx-pins', we need 2 clocks (Rx part and Tx
> part use the same CLK and SYNC clocks).

https://elixir.bootlin.com/linux/v6.2-rc3/source/Documentation/devicetree/bindings/net/qcom,ipa.yaml#L174

> 
> How can I describe this ?
> Is the check for the property presence incorrect ?
> 
> Should I always describe 4 clocks even if only 2 are used ?
> 

>>
>>> +        then:
>>> +          properties:
>>> +            clocks:
>>> +              items:
>>> +                - description: External clock connected to L1RSYNC pin
>>> +                - description: External clock connected to L1RCLK pin
>>> +                - description: External clock connected to L1TSYNC pin
>>> +                - description: External clock connected to L1TCLK pin
>>> +            clock-names:
>>> +              items:
>>> +                - const: l1rsync
>>> +                - const: l1rclk
>>> +                - const: l1tsync
>>> +                - const: l1tclk
>>> +        else:
>>> +          properties:
>>> +            clocks:
>>> +              items:
>>> +                - description: External clock connected to L1RSYNC pin
>>> +                - description: External clock connected to L1RCLK pin
>>> +            clock-names:
>>> +              items:
>>> +                - const: l1rsync
>>> +                - const: l1rclk
>>> +
>>> +    required:
>>> +      - reg
>>> +      - clocks
>>> +      - clock-names
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - reg-names
>>> +  - '#address-cells'
>>> +  - '#size-cells'
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/soc/fsl-tsa.h>
>>> +
>>> +    tsa@ae0 {
>>> +        compatible = "fsl,mpc885-tsa", "fsl,cpm1-tsa";
>>> +        reg = <0xae0 0x10>,
>>> +              <0xc00 0x200>;
>>> +        reg-names = "si_regs", "si_ram";
>>> +
>>> +        #address-cells = <1>;
>>> +        #size-cells = <0>;
>>> +
>>> +        tdm@0 {
>>> +            /* TDMa */
>>> +            reg = <0>;
>>> +
>>> +            clocks = <&clk_l1rsynca>, <&clk_l1rclka>;
>>> +            clock-names = "l1rsync", "l1rclk";
>>> +
>>> +            fsl,common-rxtx-pins;
>>> +            fsl,fsync-rising-edge;
>>> +
>>> +            tx_ts_routes = < 2 0 0>,                 /* TS 0..1 */
>>> +                           < 24 FSL_CPM_TSA_SCC4 0>, /* TS 2..25 */
>>> +                           < 1 0 0>,                 /* TS 26 */
>>> +                           < 5 FSL_CPM_TSA_SCC3 0>;  /* TS 27..31 */
>>> +
>>> +            rx_ts_routes = < 2 0 0>,                 /* TS 0..1 */
>>> +                           < 24 FSL_CPM_TSA_SCC4 0>, /* 2..25 */
>>> +                           < 1 0 0>,                 /* TS 26 */
>>> +                           < 5 FSL_CPM_TSA_SCC3 0>;  /* TS 27..31 */
>>> +        };
>>> +    };
>>> diff --git a/include/dt-bindings/soc/fsl-tsa.h b/include/dt-bindings/soc/fsl-tsa.h
>>> new file mode 100644
>>> index 000000000000..9d09468694a2
>>> --- /dev/null
>>> +++ b/include/dt-bindings/soc/fsl-tsa.h  
>>
>> Filename should match binding filename.
> 
> Right, I will rename to fsl,tsa.h

If your binding was fsl,tsa.yaml.

Best regards,
Krzysztof