[PATCH v2 0/2] rockchip pcie3-phy separate refclk support

[PATCH v2 0/2] rockchip pcie3-phy separate refclk support

[Niklas Cassel]

This series is based on: linux-phy phy/next

Hello all,

The rockchip,pcie3-phy PHY in rk3588 is by default configured to run in
"common reference clock" mode. (Which is a sensible default, as the most
commonly used clock configuration is "common reference clock".)

However, PCIe also defines two other configurations where the Root Complex
and Endpoint uses separate reference clocks: SRNS and SRIS.

Having the Root Complex PHY configured in "common reference clock mode"
while having an Endpoint connected which is supplying its own reference
clock (i.e. SRNS or SRIS configuration), will either result in the link
training failing, or a highly unstable link that continuously jumps
between link states L0 and recovery.

Add a rockchip specific device tree property that can be added to the
rk3588 Root Complex device tree PHY node, if the connected Endpoint device
is using a separate refererence clock. This way we will get a stable link
when using an Endpoint configured in SRNS or SRIS mode.


Kind regards,
Niklas

Changes since V1:
-Picked up tags
-Rebased on phy/next

Niklas Cassel (2):
  dt-bindings: phy: rockchip,pcie3-phy: add
    rockchip,rx-common-refclk-mode
  phy: rockchip-snps-pcie3: add support for
    rockchip,rx-common-refclk-mode

 .../bindings/phy/rockchip,pcie3-phy.yaml      | 10 +++++
 .../phy/rockchip/phy-rockchip-snps-pcie3.c    | 37 +++++++++++++++++++
 2 files changed, 47 insertions(+)

-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Niklas Cassel]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/2] phy: rockchip-snps-pcie3: add support for rockchip,rx-common-refclk-mode

[Niklas Cassel]

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 support for the device tree property rockchip,rx-common-refclk-mode,
such that the PCIe PHY can be used in configurations where the Root
Complex and Endpoint are not using a common reference clock.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 .../phy/rockchip/phy-rockchip-snps-pcie3.c    | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
index dcccee3c211c..4e8ffd173096 100644
--- a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
+++ b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
@@ -35,11 +35,17 @@
 #define RK3588_PCIE3PHY_GRF_CMN_CON0		0x0
 #define RK3588_PCIE3PHY_GRF_PHY0_STATUS1	0x904
 #define RK3588_PCIE3PHY_GRF_PHY1_STATUS1	0xa04
+#define RK3588_PCIE3PHY_GRF_PHY0_LN0_CON1	0x1004
+#define RK3588_PCIE3PHY_GRF_PHY0_LN1_CON1	0x1104
+#define RK3588_PCIE3PHY_GRF_PHY1_LN0_CON1	0x2004
+#define RK3588_PCIE3PHY_GRF_PHY1_LN1_CON1	0x2104
 #define RK3588_SRAM_INIT_DONE(reg)		(reg & BIT(0))
 
 #define RK3588_BIFURCATION_LANE_0_1		BIT(0)
 #define RK3588_BIFURCATION_LANE_2_3		BIT(1)
 #define RK3588_LANE_AGGREGATION		BIT(2)
+#define RK3588_RX_CMN_REFCLK_MODE_EN		((BIT(7) << 16) |  BIT(7))
+#define RK3588_RX_CMN_REFCLK_MODE_DIS		(BIT(7) << 16)
 #define RK3588_PCIE1LN_SEL_EN			(GENMASK(1, 0) << 16)
 #define RK3588_PCIE30_PHY_MODE_EN		(GENMASK(2, 0) << 16)
 
@@ -60,6 +66,7 @@ struct rockchip_p3phy_priv {
 	int num_clks;
 	int num_lanes;
 	u32 lanes[4];
+	u32 rx_cmn_refclk_mode[4];
 };
 
 struct rockchip_p3phy_ops {
@@ -137,6 +144,19 @@ static int rockchip_p3phy_rk3588_init(struct rockchip_p3phy_priv *priv)
 	u8 mode = RK3588_LANE_AGGREGATION; /* default */
 	int ret;
 
+	regmap_write(priv->phy_grf, RK3588_PCIE3PHY_GRF_PHY0_LN0_CON1,
+		     priv->rx_cmn_refclk_mode[0] ? RK3588_RX_CMN_REFCLK_MODE_EN :
+		     RK3588_RX_CMN_REFCLK_MODE_DIS);
+	regmap_write(priv->phy_grf, RK3588_PCIE3PHY_GRF_PHY0_LN1_CON1,
+		     priv->rx_cmn_refclk_mode[1] ? RK3588_RX_CMN_REFCLK_MODE_EN :
+		     RK3588_RX_CMN_REFCLK_MODE_DIS);
+	regmap_write(priv->phy_grf, RK3588_PCIE3PHY_GRF_PHY1_LN0_CON1,
+		     priv->rx_cmn_refclk_mode[2] ? RK3588_RX_CMN_REFCLK_MODE_EN :
+		     RK3588_RX_CMN_REFCLK_MODE_DIS);
+	regmap_write(priv->phy_grf, RK3588_PCIE3PHY_GRF_PHY1_LN1_CON1,
+		     priv->rx_cmn_refclk_mode[3] ? RK3588_RX_CMN_REFCLK_MODE_EN :
+		     RK3588_RX_CMN_REFCLK_MODE_DIS);
+
 	/* Deassert PCIe PMA output clamp mode */
 	regmap_write(priv->phy_grf, RK3588_PCIE3PHY_GRF_CMN_CON0, BIT(8) | BIT(24));
 
@@ -275,6 +295,23 @@ static int rockchip_p3phy_probe(struct platform_device *pdev)
 		return priv->num_lanes;
 	}
 
+	ret = of_property_read_variable_u32_array(dev->of_node,
+						  "rockchip,rx-common-refclk-mode",
+						  priv->rx_cmn_refclk_mode, 1,
+						  ARRAY_SIZE(priv->rx_cmn_refclk_mode));
+	/*
+	 * if no rockchip,rx-common-refclk-mode, assume enabled for all lanes in
+	 * order to be DT backwards compatible. (Since HW reset val is enabled.)
+	 */
+	if (ret == -EINVAL) {
+		for (int i = 0; i < ARRAY_SIZE(priv->rx_cmn_refclk_mode); i++)
+			priv->rx_cmn_refclk_mode[i] = 1;
+	} else if (ret < 0) {
+		dev_err(dev, "failed to read rockchip,rx-common-refclk-mode property %d\n",
+			ret);
+		return ret;
+	}
+
 	priv->phy = devm_phy_create(dev, NULL, &rockchip_p3phy_ops);
 	if (IS_ERR(priv->phy)) {
 		dev_err(dev, "failed to create combphy\n");
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sebastian Reichel]

[-- Attachment #1.1: Type: text/plain, Size: 2765 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Niklas Cassel]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sebastian Reichel]

[-- Attachment #1.1: Type: text/plain, Size: 4091 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] rockchip pcie3-phy separate refclk support

[Vinod Koul]

On Fri, 12 Apr 2024 14:58:14 +0200, Niklas Cassel wrote:
> This series is based on: linux-phy phy/next
> 
> Hello all,
> 
> The rockchip,pcie3-phy PHY in rk3588 is by default configured to run in
> "common reference clock" mode. (Which is a sensible default, as the most
> commonly used clock configuration is "common reference clock".)
> 
> [...]

Applied, thanks!

[1/2] dt-bindings: phy: rockchip,pcie3-phy: add rockchip,rx-common-refclk-mode
      commit: 46492d10067660785a09db4ce9244545126a17b8
[2/2] phy: rockchip-snps-pcie3: add support for rockchip,rx-common-refclk-mode
      commit: a1fe1eca0d8be69ccc1f3d615e5a529df1c82e66

Best regards,
-- 
~Vinod



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel