Aw: Re: [RFC/RFT 1/6] dt-bindings: phy: rockchip: add pcie3 phy

[Krzysztof Kozlowski]

On 19/04/2022 19:49, Frank Wunderlich wrote:
>> The list should be strictly ordered (defined), so:
>>   items:
>>     - const: ...
>>     - const: ...
>>     - const: ...
>>   minItems: 1
>>
>> However the question is - why the clocks have different amount? Is it
>> per different SoC implementation?
> 
> i only know the rk3568, which needs the clocks defined here, don't know about rk3588 yet.
> in rk3568 TPM i have the pcie-part seems missing (at least the specific register definition), so i had used the driver as i got it from the downstream kernel.
> 
> not yet looked if i find a rk3588 TPM and if this part is there as i cannot test it (one of the reasons this is a rfc/rft).

You can skip RK3588 compatible or define it this strictly also for that
chip.

> 
>>> +
>>> +  "#phy-cells":
>>> +    const: 0
>>> +
>>> +  resets:
>>> +    maxItems: 1
>>> +
>>> +  reset-names:
>>> +    const: phy
>>> +
>>> +  rockchip,phy-grf:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description: phandle to the syscon managing the phy "general register files"
>>> +
>>> +  rockchip,pipe-grf:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description: phandle to the syscon managing the pipe "general register files"
>>> +
>>> +  rockchip,pcie30-phymode:
>>> +    $ref: '/schemas/types.yaml#/definitions/uint32'
>>> +    description: |
>>> +      use PHY_MODE_PCIE_AGGREGATION if not defined
>>
>> I don't understand the description. Do you mean here a case when the
>> variable is missing?
> 
> yes, if the property is not set, then value is PHY_MODE_PCIE_AGGREGATION = 4

Then just use "default: 4"

> 
>>> +    minimum: 0x0
>>> +    maximum: 0x4
>>
>> Please explain these values. Register values should not be part of
>> bindings, but instead some logical behavior of hardware or its logic.
> 
> it's a bitmask, so maybe
> 
>     description: |
>       bit0: bifurcation for port 0
>       bit1: bifurcation for port 1
>       bit2: aggregation

That's good. I got impression you have a header with these values. If
yes - mention it here.

>       use PHY_MODE_PCIE_AGGREGATION (4) as default

Just use default as I wrote above.

Best regards,
Krzysztof