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

[Herve Codina <herve.codina@bootlin.com>]

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.

> 
> > +
> > +      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.

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.

> 
[...]
> > +
> > +      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.

> 
> > +          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).

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

> 
> > @@ -0,0 +1,15 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later OR MIT */  
> 
> A bit weird license... cannot be the same as binding?

Yes, will be change to GPL-2.0-only OR BSD-2-Clause

> 
> > +
> > +#ifndef __DT_BINDINGS_SOC_FSL_TSA_H
> > +#define __DT_BINDINGS_SOC_FSL_TSA_H
> > +
> > +#define FSL_CPM_TSA_NU		0	/* Pseuso Cell Id for not used item */  
> 
> Why defining unused IDs in binding header? These are IDs, not some
> hardware/register values.

It is the binding value for 'No destination' in the tx and rx route table.
This binding value means 'not used' or 'discard' the time-slot.
The data related to an item in the routing table with this value
will be discarded and not used.

> 
> > +#define FSL_CPM_TSA_SCC2	1
> > +#define FSL_CPM_TSA_SCC3	2
> > +#define FSL_CPM_TSA_SCC4	3
> > +#define FSL_CPM_TSA_SMC1	4
> > +#define FSL_CPM_TSA_SMC2	5
> > +
> > +#define FSL_CPM_TSA_NBCELL	6  
> 
> Drop.

Ok, will be removed in v3.

> 
> > +
> > +#endif  
> 
> Best regards,
> Krzysztof
> 

Thanks for your review.

Best regards,
Hervé


-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com