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

On 10/01/2023 09:04, Herve Codina wrote:

Hi Krzysztof,

On Sun, 8 Jan 2023 16:10:38 +0100 Krzysztof Kozlowski krzysztof.kozlowski@linaro.org 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/bi...

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