diff mbox series

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

Message ID 20240412125818.17052-2-cassel@kernel.org
State Accepted
Commit 46492d10067660785a09db4ce9244545126a17b8
Headers show
Series rockchip pcie3-phy separate refclk support | expand

Commit Message

Niklas Cassel April 12, 2024, 12:58 p.m. UTC
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(+)

Comments

Sebastian Reichel April 12, 2024, 1:36 p.m. UTC | #1
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:

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?

Greetings,

-- Sebastian
Niklas Cassel April 12, 2024, 2:03 p.m. UTC | #2
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
Sebastian Reichel April 12, 2024, 2:49 p.m. UTC | #3
Hi,

On Fri, Apr 12, 2024 at 04:03:48PM +0200, Niklas Cassel wrote:
> On Fri, Apr 12, 2024 at 03:36:11PM +0200, Sebastian Reichel wrote:
> > 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.)

Works for me:

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian
diff mbox series

Patch

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
+
 required:
   - compatible
   - reg