[PATCH v2 0/3] phy: rockchip-snps-pcie3: add support for rockchip,phy-ref-use-pad

[PATCH v2 0/3] phy: rockchip-snps-pcie3: add support for rockchip,phy-ref-use-pad

[Rick Wertenbroek]

Add the possibility for the RK3588 PCIe PHYs to use an internal clock instead of
a clock provided on a pin of the device. This allows boards that don't have a
PCIe clock connected to the reference pad to still function in separate clock
mode by using an internal reference clock for the PCIe PHYs.

This was tested with a CM3588 compute module on a custom PCB.

Without the new option, the default behaviour (PHYs using external pad for clock)
is applied, to keep compatibility with existing device trees.

Differences from V1 [1] :

- Documented the phy-ref-use-pad option in the DT bindings.
- Documented the extra optional differential clocks for the PHYs in the DT
  bindings.

[1] https://lore.kernel.org/all/20250715105820.4037272-1-rick.wertenbroek@gmail.com/

Rick Wertenbroek (3):
  dt-bindings: phy: rockchip,pcie3-phy: add optional differential phy
    clocks
  phy: rockchip-snps-pcie3: add support for rockchip,phy-ref-use-pad
  dt-bindings: phy: rockchip,pcie3-phy: add rockchip,phy-ref-use-pad

 .../bindings/phy/rockchip,pcie3-phy.yaml      | 21 ++++++++++--
 .../phy/rockchip/phy-rockchip-snps-pcie3.c    | 32 +++++++++++++++++++
 2 files changed, 50 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] dt-bindings: phy: rockchip,pcie3-phy: add optional differential phy clocks

[Rick Wertenbroek]

Both PHYs can use an alternate reference differential clock, add the clocks
to the DT bindings

Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
---
 .../devicetree/bindings/phy/rockchip,pcie3-phy.yaml   | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml b/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml
index d7de8b527c5c..b747930b18f1 100644
--- a/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml
@@ -20,11 +20,11 @@ properties:
 
   clocks:
     minItems: 1
-    maxItems: 3
+    maxItems: 5
 
   clock-names:
     minItems: 1
-    maxItems: 3
+    maxItems: 5
 
   data-lanes:
     description: which lanes (by position) should be mapped to which
@@ -82,10 +82,15 @@ allOf:
     then:
       properties:
         clocks:
-          maxItems: 1
+          minItems: 1
+          maxItems: 5
         clock-names:
           items:
             - const: pclk
+            - const: phy0_ref_alt_p
+            - const: phy0_ref_alt_m
+            - const: phy1_ref_alt_p
+            - const: phy1_ref_alt_m
     else:
       properties:
         clocks:
-- 
2.25.1

[PATCH v2 2/3] phy: rockchip-snps-pcie3: add support for rockchip,phy-ref-use-pad

[Rick Wertenbroek]

>From the RK3588 Technical Reference Manual, Part1,
section 6.19 PCIe3PHY_GRF Register Description: "ref_use_pad"

"Select reference clock connected to ref_pad_clk_p/ref_pad_clk_m.
Selects the external ref_pad_clk_p and ref_pad_clk_m inputs as the
reference clock source when asserted. When de-asserted, ref_alt_clk_p
and ref_alt_clk_m are the sources of the 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.

Add support for the device tree property rockchip,phy-ref-use-pad,
such that the PCIe PHY can be used on boards where there is no PCIe
reference clock generated or connected to the external pad, by setting
this property to 0 so that the internal clock is used.

DT bindings for internal clocks are CLK_PHY0_REF_ALT_P/M and
CLK_PHY1_REF_ALT_P/M and clock rate should be set to 100MHz in
the RK3588 cru clock controller (PLL_PPLL).

Example DT overlay where PHY0 uses internal clock (the first clock of
the cru (PLL_PPLL) must be set to 100MHz, other values are copied from
rk3588-base.dtsi) and PHY1 uses the external pad (the default):

---
&cru {
        assigned-clock-rates =
                <100000000>, <786432000>,
                <850000000>, <1188000000>,
                <702000000>,
                <400000000>, <500000000>,
                <800000000>, <100000000>,
                <400000000>, <100000000>,
                <200000000>, <500000000>,
                <375000000>, <150000000>,
                <200000000>;
};

&pcie30phy {
        rockchip,rx-common-refclk-mode = <0 0 1 1>;
        rockchip,phy-ref-use-pad = <0 1>;
        clocks = <&cru PCLK_PCIE_COMBO_PIPE_PHY>, <&cru CLK_PHY0_REF_ALT_P>,
                         <&cru CLK_PHY0_REF_ALT_M>, <&cru CLK_PHY1_REF_ALT_P>,
                         <&cru CLK_PHY1_REF_ALT_M>;
        clock-names = "pclk", "phy0_ref_alt_p",
                              "phy0_ref_alt_m", "phy1_ref_alt_p",
                              "phy1_ref_alt_m";
};
---

Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
---
 .../phy/rockchip/phy-rockchip-snps-pcie3.c    | 32 +++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
index 4e8ffd173096..0859c7960167 100644
--- a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
+++ b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
@@ -33,6 +33,8 @@
 /* Register for RK3588 */
 #define PHP_GRF_PCIESEL_CON			0x100
 #define RK3588_PCIE3PHY_GRF_CMN_CON0		0x0
+#define RK3588_PCIE3PHY_GRF_PHY0_CONTROL6	0x118
+#define RK3588_PCIE3PHY_GRF_PHY1_CONTROL6	0x218
 #define RK3588_PCIE3PHY_GRF_PHY0_STATUS1	0x904
 #define RK3588_PCIE3PHY_GRF_PHY1_STATUS1	0xa04
 #define RK3588_PCIE3PHY_GRF_PHY0_LN0_CON1	0x1004
@@ -44,6 +46,8 @@
 #define RK3588_BIFURCATION_LANE_0_1		BIT(0)
 #define RK3588_BIFURCATION_LANE_2_3		BIT(1)
 #define RK3588_LANE_AGGREGATION		BIT(2)
+#define RK3588_PHY_REF_USE_PAD_EN		((BIT(2) << 16 | BIT(2)))
+#define RK3588_PHY_REF_USE_PAD_DIS		((BIT(2) << 16))
 #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)
@@ -67,6 +71,7 @@ struct rockchip_p3phy_priv {
 	int num_lanes;
 	u32 lanes[4];
 	u32 rx_cmn_refclk_mode[4];
+	u32 phy_ref_use_pad[2];
 };
 
 struct rockchip_p3phy_ops {
@@ -157,6 +162,14 @@ static int rockchip_p3phy_rk3588_init(struct rockchip_p3phy_priv *priv)
 		     priv->rx_cmn_refclk_mode[3] ? RK3588_RX_CMN_REFCLK_MODE_EN :
 		     RK3588_RX_CMN_REFCLK_MODE_DIS);
 
+	/* Select PHY reference clock, external pad or internal clock */
+	regmap_write(priv->phy_grf, RK3588_PCIE3PHY_GRF_PHY0_CONTROL6,
+		     priv->phy_ref_use_pad[0] ? RK3588_PHY_REF_USE_PAD_EN :
+		     RK3588_PHY_REF_USE_PAD_DIS);
+	regmap_write(priv->phy_grf, RK3588_PCIE3PHY_GRF_PHY1_CONTROL6,
+		     priv->phy_ref_use_pad[1] ? RK3588_PHY_REF_USE_PAD_EN :
+		     RK3588_PHY_REF_USE_PAD_DIS);
+
 	/* Deassert PCIe PMA output clamp mode */
 	regmap_write(priv->phy_grf, RK3588_PCIE3PHY_GRF_CMN_CON0, BIT(8) | BIT(24));
 
@@ -312,6 +325,25 @@ static int rockchip_p3phy_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = of_property_read_variable_u32_array(dev->of_node,
+						  "rockchip,phy-ref-use-pad",
+						  priv->phy_ref_use_pad, 1,
+						  ARRAY_SIZE(priv->phy_ref_use_pad));
+
+	/*
+	 * if no rockhip,phy-use-internal-clk, assume PHY uses pad for the
+	 * reference clock in order to be DT backwards compatible. (Since HW
+	 * reset val is enabled.)
+	 */
+	if (ret == -EINVAL) {
+		for (int i = 0; i < ARRAY_SIZE(priv->phy_ref_use_pad); i++)
+			priv->phy_ref_use_pad[i] = 1;
+	} else if (ret < 0) {
+		dev_err(dev, "failed to read rockchip,phy-ref-use-pad 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.25.1

[PATCH v2 3/3] dt-bindings: phy: rockchip,pcie3-phy: add rockchip,phy-ref-use-pad

[Rick Wertenbroek]

>From the RK3588 Technical Reference Manual, Part1,
section 6.19 PCIe3PHY_GRF Register Description: "ref_use_pad"

"Select reference clock connected to ref_pad_clk_p/ref_pad_clk_m.
Selects the external ref_pad_clk_p and ref_pad_clk_m inputs as the
reference clock source when asserted. When de-asserted, ref_alt_clk_p
and ref_alt_clk_m are the sources of the 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.

Add support for the device tree property rockchip,phy-ref-use-pad,
such that the PCIe PHY can be used on boards where there is no PCIe
reference clock generated or connected to the external pad, by setting
this property to 0 so that the internal clock is used.

DT bindings for internal clocks are CLK_PHY0_REF_ALT_P/M and
CLK_PHY1_REF_ALT_P/M and clock rate should be set to 100MHz in
the RK3588 cru clock controller (PLL_PPLL).

Example DT overlay where PHY0 uses internal clock (the first clock of
the cru (PLL_PPLL) must be set to 100MHz, other values are copied from
rk3588-base.dtsi) and PHY1 uses the external pad (the default):

---
&cru {
        assigned-clock-rates =
                <100000000>, <786432000>,
                <850000000>, <1188000000>,
                <702000000>,
                <400000000>, <500000000>,
                <800000000>, <100000000>,
                <400000000>, <100000000>,
                <200000000>, <500000000>,
                <375000000>, <150000000>,
                <200000000>;
};

&pcie30phy {
        rockchip,rx-common-refclk-mode = <0 0 1 1>;
        rockchip,phy-ref-use-pad = <0 1>;
        clocks = <&cru PCLK_PCIE_COMBO_PIPE_PHY>, <&cru CLK_PHY0_REF_ALT_P>,
                         <&cru CLK_PHY0_REF_ALT_M>, <&cru CLK_PHY1_REF_ALT_P>,
                         <&cru CLK_PHY1_REF_ALT_M>;
        clock-names = "pclk", "phy0_ref_alt_p",
                              "phy0_ref_alt_m", "phy1_ref_alt_p",
                              "phy1_ref_alt_m";
};
---

Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
---
 .../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 b747930b18f1..d9b9d7eabb81 100644
--- a/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/rockchip,pcie3-phy.yaml
@@ -67,6 +67,16 @@ properties:
       minimum: 0
       maximum: 1
 
+  rockchip,phy-ref-use-pad:
+    description: which PHY should use the external pad as PCIe reference clock.
+      1 means use pad (default), 0 means use internal clock (PLL_PPLL).
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 2
+    maxItems: 2
+    items:
+      minimum: 0
+      maximum: 1
+
 required:
   - compatible
   - reg
-- 
2.25.1

Re: [PATCH v2 1/3] dt-bindings: phy: rockchip,pcie3-phy: add optional differential phy clocks

[Krzysztof Kozlowski]

On Wed, Aug 06, 2025 at 03:38:21PM +0200, Rick Wertenbroek wrote:
> Both PHYs can use an alternate reference differential clock, add the clocks

I do not see any changes in rockchip,rk3588-pcie3-phy, so your "both" is
either incorrect or ambiguous.

...

> to the DT bindings
> 
  
>    data-lanes:
>      description: which lanes (by position) should be mapped to which
> @@ -82,10 +82,15 @@ allOf:
>      then:
>        properties:
>          clocks:
> -          maxItems: 1
> +          minItems: 1
> +          maxItems: 5
>          clock-names:
>            items:
>              - const: pclk
> +            - const: phy0_ref_alt_p
> +            - const: phy0_ref_alt_m
> +            - const: phy1_ref_alt_p
> +            - const: phy1_ref_alt_m

These are different clock inputs?

>      else:
>        properties:
>          clocks:

You need to update the example as well.

> -- 
> 2.25.1
> 

Re: [PATCH v2 1/3] dt-bindings: phy: rockchip,pcie3-phy: add optional differential phy clocks

[Krzysztof Kozlowski]

On 07/08/2025 09:42, Krzysztof Kozlowski wrote:
> On Wed, Aug 06, 2025 at 03:38:21PM +0200, Rick Wertenbroek wrote:
>> Both PHYs can use an alternate reference differential clock, add the clocks
> 
> I do not see any changes in rockchip,rk3588-pcie3-phy, so your "both" is

I meant 3568, the other one.

> either incorrect or ambiguous.
> 
> ...
> 
>> to the DT bindings
>>

Best regards,
Krzysztof

Re: [PATCH v2 3/3] dt-bindings: phy: rockchip,pcie3-phy: add rockchip,phy-ref-use-pad

[Krzysztof Kozlowski]

On Wed, Aug 06, 2025 at 03:38:23PM +0200, Rick Wertenbroek wrote:
> >From the RK3588 Technical Reference Manual, Part1,
> section 6.19 PCIe3PHY_GRF Register Description: "ref_use_pad"
> 
> "Select reference clock connected to ref_pad_clk_p/ref_pad_clk_m.
> Selects the external ref_pad_clk_p and ref_pad_clk_m inputs as the
> reference clock source when asserted. When de-asserted, ref_alt_clk_p
> and ref_alt_clk_m are the sources of the 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.

Then you miss restricting it (:false) in one of if:then: blocks.

Also, binding cannot be after the user. You need to reorder patches.

...

>  
> +  rockchip,phy-ref-use-pad:
> +    description: which PHY should use the external pad as PCIe reference clock.
> +      1 means use pad (default), 0 means use internal clock (PLL_PPLL).

Can't you deduce it from the presence of clock inputs? IOW, if the
clocks are physically connected, is it even reasonable or possible to
use internal clock?

> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 2
> +    maxItems: 2
> +    items:
> +      minimum: 0
> +      maximum: 1

More precise:

items:
  - description: PHY0 reference clock config
  - description: PHY1 reference clock config
  enum: [ 0, 1 ]
default: [ 1, 1 ]

Anyway, default 1, 1 is pretty non-obvious, so this should be just
non-unique-string-array (and property should be like
rockchip,phy-ref-clk-source/sel).


Best regards,
Krzysztof

Re: [PATCH v2 3/3] dt-bindings: phy: rockchip,pcie3-phy: add rockchip,phy-ref-use-pad

[Krzysztof Kozlowski]

On 07/08/2025 09:54, Krzysztof Kozlowski wrote:
> On Wed, Aug 06, 2025 at 03:38:23PM +0200, Rick Wertenbroek wrote:
>> >From the RK3588 Technical Reference Manual, Part1,
>> section 6.19 PCIe3PHY_GRF Register Description: "ref_use_pad"
>>
>> "Select reference clock connected to ref_pad_clk_p/ref_pad_clk_m.
>> Selects the external ref_pad_clk_p and ref_pad_clk_m inputs as the
>> reference clock source when asserted. When de-asserted, ref_alt_clk_p
>> and ref_alt_clk_m are the sources of the 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.
> 
> Then you miss restricting it (:false) in one of if:then: blocks.
> 
> Also, binding cannot be after the user. You need to reorder patches.
> 
> ...
> 
>>  
>> +  rockchip,phy-ref-use-pad:
>> +    description: which PHY should use the external pad as PCIe reference clock.
>> +      1 means use pad (default), 0 means use internal clock (PLL_PPLL).
> 
> Can't you deduce it from the presence of clock inputs? IOW, if the
> clocks are physically connected, is it even reasonable or possible to
> use internal clock?
> 
>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>> +    minItems: 2
>> +    maxItems: 2
>> +    items:
>> +      minimum: 0
>> +      maximum: 1
> 
> More precise:
> 
> items:
>   - description: PHY0 reference clock config
>   - description: PHY1 reference clock config
>   enum: [ 0, 1 ]

Eh, no, rather if this stays as int:

    items:
      - description: PHY0 reference clock config
        enum: [ 0, 1 ]
      - description: PHY1 reference clock config
        enum: [ 0, 1 ]
    default: [ 1, 1 ]


> default: [ 1, 1 ]
> 
> Anyway, default 1, 1 is pretty non-obvious, so this should be just
> non-unique-string-array (and property should be like
> rockchip,phy-ref-clk-source/sel).
> 
> 
> Best regards,
> Krzysztof
> 


Best regards,
Krzysztof

Re: [PATCH v2 1/3] dt-bindings: phy: rockchip,pcie3-phy: add optional differential phy clocks

[Rick Wertenbroek]

On Thu, Aug 7, 2025 at 9:44 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 07/08/2025 09:42, Krzysztof Kozlowski wrote:
> > On Wed, Aug 06, 2025 at 03:38:21PM +0200, Rick Wertenbroek wrote:
> >> Both PHYs can use an alternate reference differential clock, add the clocks
> >
> > I do not see any changes in rockchip,rk3588-pcie3-phy, so your "both" is
>
> I meant 3568, the other one.
>

By "both" I meant both PHYs of the RK3588 as the rk3588-pcie-phy is
actually a dual PHY (PHY0 and PHY1 which both can use independent
clocks).

The RK3588 PHY is a dual PHY with two independent PCIe 3.0 x2
interfaces (that can be combined into an x4 or used independently).
The RK3568 PHY is a single PHY with one PCIe 3.0 x2 interface.

The RK3568 already has the bindings for the extra differential clock
for its PHY, but the RK3588 did not, so I added them.

I should maybe rephrase this to make it clearer it applies only to the
RK3588 and that by both PHYs I mean RK3588 PHY0 and PHY1

> > either incorrect or ambiguous.
> >
> > ...
> >
> >> to the DT bindings
> >>
>
> Best regards,
> Krzysztof

Re: [PATCH v2 3/3] dt-bindings: phy: rockchip,pcie3-phy: add rockchip,phy-ref-use-pad

[Rick Wertenbroek]

On Thu, Aug 7, 2025 at 9:55 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 07/08/2025 09:54, Krzysztof Kozlowski wrote:
> > On Wed, Aug 06, 2025 at 03:38:23PM +0200, Rick Wertenbroek wrote:
> >> >From the RK3588 Technical Reference Manual, Part1,
> >> section 6.19 PCIe3PHY_GRF Register Description: "ref_use_pad"
> >>
> >> "Select reference clock connected to ref_pad_clk_p/ref_pad_clk_m.
> >> Selects the external ref_pad_clk_p and ref_pad_clk_m inputs as the
> >> reference clock source when asserted. When de-asserted, ref_alt_clk_p
> >> and ref_alt_clk_m are the sources of the 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.
> >
> > Then you miss restricting it (:false) in one of if:then: blocks.
> >
> > Also, binding cannot be after the user. You need to reorder patches.
> >
> > ...
> >
> >>
> >> +  rockchip,phy-ref-use-pad:
> >> +    description: which PHY should use the external pad as PCIe reference clock.
> >> +      1 means use pad (default), 0 means use internal clock (PLL_PPLL).
> >
> > Can't you deduce it from the presence of clock inputs? IOW, if the
> > clocks are physically connected, is it even reasonable or possible to
> > use internal clock?

Thank you Krzysztof,
But no, if there is no clock, the driver deadlocks, it needs a clock
to probe correctly.

When there is a clock physically connected on the pad, you can still
choose to use it or use the internal clock, this is no problem.
The problem is when you have no clock on the pad (as it defaults to
using the pad) and the loading the driver deadlocks the system...

> >
> >> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> >> +    minItems: 2
> >> +    maxItems: 2
> >> +    items:
> >> +      minimum: 0
> >> +      maximum: 1
> >
> > More precise:
> >
> > items:
> >   - description: PHY0 reference clock config
> >   - description: PHY1 reference clock config
> >   enum: [ 0, 1 ]
>
> Eh, no, rather if this stays as int:
>
>     items:
>       - description: PHY0 reference clock config
>         enum: [ 0, 1 ]
>       - description: PHY1 reference clock config
>         enum: [ 0, 1 ]
>     default: [ 1, 1 ]
>
>
> > default: [ 1, 1 ]
> >
> > Anyway, default 1, 1 is pretty non-obvious, so this should be just
> > non-unique-string-array (and property should be like
> > rockchip,phy-ref-clk-source/sel).
> >
> >
> > Best regards,
> > Krzysztof
> >
>
>
> Best regards,
> Krzysztof

I based my patch on patch :
46492d10067660785a09db4ce9244545126a17b8
dt-bindings: phy: rockchip,pcie3-phy: add rockchip,rx-common-refclk-mode

As the option I add is extremely similar, it sets a feature in one of
the PHY registers and only applies to RK3588.
That is why I used the same style as rockchip,rx-common-refclk-mode.

Wouldn't it be confusing or at least incoherent to use enum for
rockchip,phy-ref-use-pad but not for rx-common-refclk-mode ?

As for the name, I used phy-ref-use-pad as it is the name used in the
RK3588 technical reference manual.
(Similarly done on rx-common-refclk-mode).

Best regards,
Rick

Re: [PATCH v2 3/3] dt-bindings: phy: rockchip,pcie3-phy: add rockchip,phy-ref-use-pad

[Krzysztof Kozlowski]

On Thu, Aug 07, 2025 at 10:47:18AM +0200, Rick Wertenbroek wrote:
> On Thu, Aug 7, 2025 at 9:55 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > On 07/08/2025 09:54, Krzysztof Kozlowski wrote:
> > > On Wed, Aug 06, 2025 at 03:38:23PM +0200, Rick Wertenbroek wrote:
> > >> >From the RK3588 Technical Reference Manual, Part1,
> > >> section 6.19 PCIe3PHY_GRF Register Description: "ref_use_pad"
> > >>
> > >> "Select reference clock connected to ref_pad_clk_p/ref_pad_clk_m.
> > >> Selects the external ref_pad_clk_p and ref_pad_clk_m inputs as the
> > >> reference clock source when asserted. When de-asserted, ref_alt_clk_p
> > >> and ref_alt_clk_m are the sources of the 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.
> > >
> > > Then you miss restricting it (:false) in one of if:then: blocks.
> > >
> > > Also, binding cannot be after the user. You need to reorder patches.
> > >
> > > ...
> > >
> > >>
> > >> +  rockchip,phy-ref-use-pad:
> > >> +    description: which PHY should use the external pad as PCIe reference clock.
> > >> +      1 means use pad (default), 0 means use internal clock (PLL_PPLL).
> > >
> > > Can't you deduce it from the presence of clock inputs? IOW, if the
> > > clocks are physically connected, is it even reasonable or possible to
> > > use internal clock?
> 
> Thank you Krzysztof,
> But no, if there is no clock, the driver deadlocks, it needs a clock
> to probe correctly.
> 
> When there is a clock physically connected on the pad, you can still
> choose to use it or use the internal clock, this is no problem.

Why would use internal clock for such case? In few other cases this
appeared we usualyl were using the presence of the clock as determining
factor.

> The problem is when you have no clock on the pad (as it defaults to
> using the pad) and the loading the driver deadlocks the system...

This sounds like a driver, not a binding, problem.

> 
> > >
> > >> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > >> +    minItems: 2
> > >> +    maxItems: 2
> > >> +    items:
> > >> +      minimum: 0
> > >> +      maximum: 1
> > >
> > > More precise:
> > >
> > > items:
> > >   - description: PHY0 reference clock config
> > >   - description: PHY1 reference clock config
> > >   enum: [ 0, 1 ]
> >
> > Eh, no, rather if this stays as int:
> >
> >     items:
> >       - description: PHY0 reference clock config
> >         enum: [ 0, 1 ]
> >       - description: PHY1 reference clock config
> >         enum: [ 0, 1 ]
> >     default: [ 1, 1 ]
> >
> >
> > > default: [ 1, 1 ]
> > >
> > > Anyway, default 1, 1 is pretty non-obvious, so this should be just
> > > non-unique-string-array (and property should be like
> > > rockchip,phy-ref-clk-source/sel).
> > >
> > >
> > > Best regards,
> > > Krzysztof
> > >
> >
> >
> > Best regards,
> > Krzysztof
> 
> I based my patch on patch :
> 46492d10067660785a09db4ce9244545126a17b8
> dt-bindings: phy: rockchip,pcie3-phy: add rockchip,rx-common-refclk-mode
> 
> As the option I add is extremely similar, it sets a feature in one of
> the PHY registers and only applies to RK3588.
> That is why I used the same style as rockchip,rx-common-refclk-mode.

So you should have used for example same property.

> 
> Wouldn't it be confusing or at least incoherent to use enum for
> rockchip,phy-ref-use-pad but not for rx-common-refclk-mode ?

Same hardware properties should have same properties - type, name and
meaning - but you did not re-use existing one. Maybe it is too specific?

We do not write properties for registers, but for hardware (like SoC or
board) characteristics.

Best regards,
Krzysztof