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
