Re: [PATCH v2 1/2] dt-bindings: phy: rockchip,pcie3-phy: add rockchip,rx-common-refclk-mode

[Niklas Cassel <cassel@kernel.org>]

On Fri, Apr 12, 2024 at 03:36:11PM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Fri, Apr 12, 2024 at 02:58:15PM +0200, Niklas Cassel wrote:
> > From the RK3588 Technical Reference Manual, Part1,
> > section 6.19 PCIe3PHY_GRF Register Description:
> > "rxX_cmn_refclk_mode"
> > RX common reference clock mode for lane X. This mode should be enabled
> > only when the far-end and near-end devices are running with a common
> > reference clock.
> > 
> > The hardware reset value for this field is 0x1 (enabled).
> > Note that this register field is only available on RK3588, not on RK3568.
> > 
> > The link training either fails or is highly unstable (link state will jump
> > continuously between L0 and recovery) when this mode is enabled while
> > using an endpoint running in Separate Reference Clock with No SSC (SRNS)
> > mode or Separate Reference Clock with SSC (SRIS) mode.
> > (Which is usually the case when using a real SoC as endpoint, e.g. the
> > RK3588 PCIe controller can run in both Root Complex and Endpoint mode.)
> > 
> > Add a rockchip specific property to enable/disable the rxX_cmn_refclk_mode
> > per lane. (Since this PHY supports bifurcation.)
> > 
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > ---
> >  .../devicetree/bindings/phy/rockchip,pcie3-phy.yaml    | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml b/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml
> > index c4fbffcde6e4..ba67dca5a446 100644
> > --- a/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml
> > +++ b/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml
> > @@ -54,6 +54,16 @@ properties:
> >      $ref: /schemas/types.yaml#/definitions/phandle
> >      description: phandle to the syscon managing the pipe "general register files"
> >  
> > +  rockchip,rx-common-refclk-mode:
> > +    description: which lanes (by position) should be configured to run in
> > +      RX common reference clock mode. 0 means disabled, 1 means enabled.
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    minItems: 1
> > +    maxItems: 16
> > +    items:
> > +      minimum: 0
> > +      maximum: 1
> 
> Why is this not simply using a single u32 with each bit standing for
> one PCIe lane? I.e. like this:

Hello Sebastian,

The reason for the existing way to specify each lane in an int32-array
is to be consistent with the existing property "data-lanes",
which already uses this representation.

e.g.
data-lanes = <1 1 2 2>;
rockchip,rx-common-refclk-mode = <0 0 1 1>;

It would be very weird if this was instead:
data-lanes = <1 1 2 2>;
rockchip,rx-common-refclk-mode = 0xc;


> 
> rockchip,rx-common-refclk-mode:
>   description: bitmap describing which lanes should be configured to run
>     in RX common reference clock mode. Bit offset maps to PCIe lanes and
>     a bit set means enabled, unset bit means disabled.
>   $ref: /schemas/types.yaml#/definitions/uint32
>   minimum: 0
>   maximum: 0xffff
>   default: 0xffff
> 
> Apart from that the PHY only supports up to 4 lanes on all existing hardware,
> so 0xf should be enough?

Again, in order to be consistent with the existing "data-lanes" property in
this binding, which defines:
    minItems: 2
    maxItems: 16
which means that the binding already supports up to 16 lanes.
(The reason why "data-lanes" specifies minItems:2 is because bifurcation
doesn't make sense if you just have a single lane. The rx-common-refclk-mode
property however makes sense even in the case where there is just a single
lane.)


Kind regards,
Niklas