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
> Gesendet: Dienstag, 19. April 2022 um 21:43 Uhr
> Von: "Krzysztof Kozlowski" <[email protected]>
> An: "Frank Wunderlich" <[email protected]>
> Cc: "Frank Wunderlich" <[email protected]>, [email protected], "Kishon Vijay Abraham I" <[email protected]>, "Vinod Koul" <[email protected]>, "Rob Herring" <[email protected]>, "Krzysztof Kozlowski" <[email protected]>, "Heiko Stuebner" <[email protected]>, "Lorenzo Pieralisi" <[email protected]>, "Krzysztof Wilczyński" <[email protected]>, "Bjorn Helgaas" <[email protected]>, "Philipp Zabel" <[email protected]>, "Johan Jonker" <[email protected]>, "Peter Geis" <[email protected]>, "Michael Riesch" <[email protected]>, [email protected], [email protected], [email protected], [email protected], [email protected]
> Betreff: Re: Aw: Re: [RFC/RFT 1/6] dt-bindings: phy: rockchip: add pcie3 phy
>
> 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.
currently driver does clk_bulk initialization so i would define it like you suggested (without any SoC specific switch):
clocks:
minItems: 1
maxItems: 3
clock-names:
items:
- const: "refclk_m"
- const: "refclk_n"
- const: "pclk"
minItems: 1
> >>> +
> >>> + "#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.
so like this?
rockchip,pcie30-phymode:
$ref: '/schemas/types.yaml#/definitions/uint32'
description: |
set the phy-mode for enabling bifurcation
bit0: bifurcation for port 0
bit1: bifurcation for port 1
bit2: aggregation
constants are defined in the dt-bindings/phy/phy-rockchip-pcie3.h
minimum: 0x0
maximum: 0x4
default: 0x4
regards Frank