[PATCH v2 01/10] dt-bindings: soc: fsl: cpm_qe: Add TSA controller
Krzysztof Kozlowski
krzysztof.kozlowski at linaro.org
Wed Jan 11 20:56:56 AEDT 2023
On 10/01/2023 09:04, Herve Codina wrote:
> Hi Krzysztof,
>
> On Sun, 8 Jan 2023 16:10:38 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski at 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/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 at 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 at 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
More information about the Linuxppc-dev
mailing list