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

10 Jan 2023

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

    

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

Herve Codina