[PATCH 0/7] arm64: qcom-sm8[456]50: properly describe the PCIe Gen4x2 PHY AUX clock

[PATCH 0/7] arm64: qcom-sm8[456]50: properly describe the PCIe Gen4x2 PHY AUX clock

[Neil Armstrong]

The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock named
"PHY_AUX_CLK" which is an input of the Global Clock Controller (GCC) which
is muxed & gated then returned to the PHY as an input.

Document the clock IDs to select the PIPE clock or the AUX clock,
also enforce a second clock-output-names and a #clock-cells value of 1
for the PCIe Gen4x2 PHY found in the SM8[456]50 SoCs.

The PHY driver needs a light refactoring to support a second clock,
and finally the DT is changed to connect the PHY second clock to the
corresponding GCC input then drop the dummy fixed rate clock.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
Neil Armstrong (7):
      dt-bindings: phy: qcom,sc8280xp-qmp-pcie-phy: document PHY AUX clock on SM8[456]50 SoCs
      phy: qcom: qmp-pcie: refactor clock register code
      phy: qcom: qmp-pcie: register second optional PHY AUX clock
      phy: qcom: qmp-pcie: register PHY AUX clock for SM8[456]50 4x2 PCIe PHY
      arm64: dts: qcom: sm8450: remove pcie-1-phy-aux-clk and add pcie1_phy pcie1_phy_aux_clk
      arm64: dts: qcom: sm8550: remove pcie-1-phy-aux-clk and add pcie1_phy pcie1_phy_aux_clk
      arm64: dts: qcom: sm8650: remove pcie-1-phy-aux-clk and add pcie1_phy pcie1_phy_aux_clk

 .../bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml   |  27 +++++-
 arch/arm64/boot/dts/qcom/sm8450.dtsi               |   8 +-
 arch/arm64/boot/dts/qcom/sm8550-hdk.dts            |   4 -
 arch/arm64/boot/dts/qcom/sm8550-mtp.dts            |   4 -
 arch/arm64/boot/dts/qcom/sm8550-qrd.dts            |   8 --
 arch/arm64/boot/dts/qcom/sm8550.dtsi               |  13 +--
 arch/arm64/boot/dts/qcom/sm8650-mtp.dts            |   4 -
 arch/arm64/boot/dts/qcom/sm8650-qrd.dts            |   4 -
 arch/arm64/boot/dts/qcom/sm8650.dtsi               |  13 +--
 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c           | 104 ++++++++++++++++++---
 include/dt-bindings/phy/phy-qcom-qmp.h             |   4 +
 11 files changed, 129 insertions(+), 64 deletions(-)
---
base-commit: 2e93f143ca010a5013528e1cfdc895f024fe8c21
change-id: 20240319-topic-sm8x50-upstream-pcie-1-phy-aux-clk-4b35169707dd

Best regards,
-- 
Neil Armstrong <neil.armstrong@linaro.org>

[PATCH 1/7] dt-bindings: phy: qcom,sc8280xp-qmp-pcie-phy: document PHY AUX clock on SM8[456]50 SoCs

[Neil Armstrong]

The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock named
"PHY_AUX_CLK" which is an input of the Global Clock Controller (GCC) which
is muxed & gated then returned to the PHY as an input.

Document the clock IDs to select the PIPE clock or the AUX clock,
also enforce a second clock-output-names and a #clock-cells value of 1
for the PCIe Gen4x2 PHY found in the SM8[456]50 SoCs.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 .../bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml   | 27 +++++++++++++++++++---
 include/dt-bindings/phy/phy-qcom-qmp.h             |  4 ++++
 2 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml
index ba966a78a128..14ac341b1577 100644
--- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml
@@ -88,11 +88,11 @@ properties:
           - description: offset of PCIe 4-lane configuration register
           - description: offset of configuration bit for this PHY
 
-  "#clock-cells":
-    const: 0
+  "#clock-cells": true
 
   clock-output-names:
-    maxItems: 1
+    minItems: 1
+    maxItems: 2
 
   "#phy-cells":
     const: 0
@@ -213,6 +213,27 @@ allOf:
         reset-names:
           maxItems: 1
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,sm8450-qmp-gen4x2-pcie-phy
+              - qcom,sm8550-qmp-gen4x2-pcie-phy
+              - qcom,sm8650-qmp-gen4x2-pcie-phy
+    then:
+      properties:
+        clock-output-names:
+          minItems: 2
+        "#clock-cells":
+          const: 1
+    else:
+      properties:
+        clock-output-names:
+          maxItems: 1
+        "#clock-cells":
+          const: 0
+
 examples:
   - |
     #include <dt-bindings/clock/qcom,gcc-sc8280xp.h>
diff --git a/include/dt-bindings/phy/phy-qcom-qmp.h b/include/dt-bindings/phy/phy-qcom-qmp.h
index 4edec4c5b224..6b43ea9e0051 100644
--- a/include/dt-bindings/phy/phy-qcom-qmp.h
+++ b/include/dt-bindings/phy/phy-qcom-qmp.h
@@ -17,4 +17,8 @@
 #define QMP_USB43DP_USB3_PHY		0
 #define QMP_USB43DP_DP_PHY		1
 
+/* QMP PCIE PHYs */
+#define QMP_PCIE_PIPE_CLK		0
+#define QMP_PCIE_PHY_AUX_CLK		1
+
 #endif /* _DT_BINDINGS_PHY_QMP */

-- 
2.34.1

Re: [PATCH 1/7] dt-bindings: phy: qcom,sc8280xp-qmp-pcie-phy: document PHY AUX clock on SM8[456]50 SoCs

[Krzysztof Kozlowski]

On 19/03/2024 11:44, Neil Armstrong wrote:
> The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock named
> "PHY_AUX_CLK" which is an input of the Global Clock Controller (GCC) which
> is muxed & gated then returned to the PHY as an input.
> 
> Document the clock IDs to select the PIPE clock or the AUX clock,
> also enforce a second clock-output-names and a #clock-cells value of 1
> for the PCIe Gen4x2 PHY found in the SM8[456]50 SoCs.
> 
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH 2/7] phy: qcom: qmp-pcie: refactor clock register code

[Neil Armstrong]

The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock,
in order to expose it, split the current clock registering in two parts:
- CCF clock registering
- DT clock registering

Also switch to devm_of_clk_add_hw_provider().

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
index 8836bb1ff0cc..079b3e306489 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
@@ -3635,11 +3635,6 @@ static int qmp_pcie_clk_init(struct qmp_pcie *qmp)
 	return devm_clk_bulk_get_optional(dev, num, qmp->clks);
 }
 
-static void phy_clk_release_provider(void *res)
-{
-	of_clk_del_provider(res);
-}
-
 /*
  * Register a fixed rate pipe clock.
  *
@@ -3664,7 +3659,7 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np)
 	struct clk_init_data init = { };
 	int ret;
 
-	ret = of_property_read_string(np, "clock-output-names", &init.name);
+	ret = of_property_read_string_index(np, "clock-output-names", 0, &init.name);
 	if (ret) {
 		dev_err(qmp->dev, "%pOFn: No clock-output-names\n", np);
 		return ret;
@@ -3683,19 +3678,19 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np)
 
 	fixed->hw.init = &init;
 
-	ret = devm_clk_hw_register(qmp->dev, &fixed->hw);
-	if (ret)
-		return ret;
+	return devm_clk_hw_register(qmp->dev, &fixed->hw);
+}
 
-	ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw);
+static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np)
+{
+	int ret;
+
+	ret = phy_pipe_clk_register(qmp, np);
 	if (ret)
 		return ret;
 
-	/*
-	 * Roll a devm action because the clock provider is the child node, but
-	 * the child node is not actually a device.
-	 */
-	return devm_add_action_or_reset(qmp->dev, phy_clk_release_provider, np);
+	return devm_of_clk_add_hw_provider(qmp->dev, of_clk_hw_simple_get,
+					   &qmp->pipe_clk_fixed.hw);
 }
 
 static int qmp_pcie_parse_dt_legacy(struct qmp_pcie *qmp, struct device_node *np)
@@ -3899,7 +3894,7 @@ static int qmp_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_node_put;
 
-	ret = phy_pipe_clk_register(qmp, np);
+	ret = qmp_pcie_register_clocks(qmp, np);
 	if (ret)
 		goto err_node_put;
 

-- 
2.34.1

Re: [PATCH 2/7] phy: qcom: qmp-pcie: refactor clock register code

[Dmitry Baryshkov]

On Tue, 19 Mar 2024 at 12:45, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>
> The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock,
> in order to expose it, split the current clock registering in two parts:
> - CCF clock registering
> - DT clock registering
>
> Also switch to devm_of_clk_add_hw_provider().
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 27 +++++++++++----------------
>  1 file changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> index 8836bb1ff0cc..079b3e306489 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> @@ -3635,11 +3635,6 @@ static int qmp_pcie_clk_init(struct qmp_pcie *qmp)
>         return devm_clk_bulk_get_optional(dev, num, qmp->clks);
>  }
>
> -static void phy_clk_release_provider(void *res)
> -{
> -       of_clk_del_provider(res);
> -}
> -
>  /*
>   * Register a fixed rate pipe clock.
>   *
> @@ -3664,7 +3659,7 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np)
>         struct clk_init_data init = { };
>         int ret;
>
> -       ret = of_property_read_string(np, "clock-output-names", &init.name);
> +       ret = of_property_read_string_index(np, "clock-output-names", 0, &init.name);
>         if (ret) {
>                 dev_err(qmp->dev, "%pOFn: No clock-output-names\n", np);
>                 return ret;
> @@ -3683,19 +3678,19 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np)
>
>         fixed->hw.init = &init;
>
> -       ret = devm_clk_hw_register(qmp->dev, &fixed->hw);
> -       if (ret)
> -               return ret;
> +       return devm_clk_hw_register(qmp->dev, &fixed->hw);
> +}
>
> -       ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw);
> +static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np)
> +{
> +       int ret;
> +
> +       ret = phy_pipe_clk_register(qmp, np);
>         if (ret)
>                 return ret;
>
> -       /*
> -        * Roll a devm action because the clock provider is the child node, but
> -        * the child node is not actually a device.
> -        */
> -       return devm_add_action_or_reset(qmp->dev, phy_clk_release_provider, np);
> +       return devm_of_clk_add_hw_provider(qmp->dev, of_clk_hw_simple_get,
> +                                          &qmp->pipe_clk_fixed.hw);

No. The driver has to register a clock provider at the np rather than
at dev->of_node. Otherwise legacy DT will be broken.

>  }
>
>  static int qmp_pcie_parse_dt_legacy(struct qmp_pcie *qmp, struct device_node *np)
> @@ -3899,7 +3894,7 @@ static int qmp_pcie_probe(struct platform_device *pdev)
>         if (ret)
>                 goto err_node_put;
>
> -       ret = phy_pipe_clk_register(qmp, np);
> +       ret = qmp_pcie_register_clocks(qmp, np);
>         if (ret)
>                 goto err_node_put;
>
>
> --
> 2.34.1
>
>


-- 
With best wishes
Dmitry

Re: [PATCH 2/7] phy: qcom: qmp-pcie: refactor clock register code

[Neil Armstrong]

On 19/03/2024 11:50, Dmitry Baryshkov wrote:
> On Tue, 19 Mar 2024 at 12:45, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>
>> The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock,
>> in order to expose it, split the current clock registering in two parts:
>> - CCF clock registering
>> - DT clock registering
>>
>> Also switch to devm_of_clk_add_hw_provider().
>>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>>   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 27 +++++++++++----------------
>>   1 file changed, 11 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>> index 8836bb1ff0cc..079b3e306489 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>> @@ -3635,11 +3635,6 @@ static int qmp_pcie_clk_init(struct qmp_pcie *qmp)
>>          return devm_clk_bulk_get_optional(dev, num, qmp->clks);
>>   }
>>
>> -static void phy_clk_release_provider(void *res)
>> -{
>> -       of_clk_del_provider(res);
>> -}
>> -
>>   /*
>>    * Register a fixed rate pipe clock.
>>    *
>> @@ -3664,7 +3659,7 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np)
>>          struct clk_init_data init = { };
>>          int ret;
>>
>> -       ret = of_property_read_string(np, "clock-output-names", &init.name);
>> +       ret = of_property_read_string_index(np, "clock-output-names", 0, &init.name);
>>          if (ret) {
>>                  dev_err(qmp->dev, "%pOFn: No clock-output-names\n", np);
>>                  return ret;
>> @@ -3683,19 +3678,19 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np)
>>
>>          fixed->hw.init = &init;
>>
>> -       ret = devm_clk_hw_register(qmp->dev, &fixed->hw);
>> -       if (ret)
>> -               return ret;
>> +       return devm_clk_hw_register(qmp->dev, &fixed->hw);
>> +}
>>
>> -       ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw);
>> +static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np)
>> +{
>> +       int ret;
>> +
>> +       ret = phy_pipe_clk_register(qmp, np);
>>          if (ret)
>>                  return ret;
>>
>> -       /*
>> -        * Roll a devm action because the clock provider is the child node, but
>> -        * the child node is not actually a device.
>> -        */
>> -       return devm_add_action_or_reset(qmp->dev, phy_clk_release_provider, np);
>> +       return devm_of_clk_add_hw_provider(qmp->dev, of_clk_hw_simple_get,
>> +                                          &qmp->pipe_clk_fixed.hw);
> 
> No. The driver has to register a clock provider at the np rather than
> at dev->of_node. Otherwise legacy DT will be broken.

Indeed

Thx
Neil

> 
>>   }
>>
>>   static int qmp_pcie_parse_dt_legacy(struct qmp_pcie *qmp, struct device_node *np)
>> @@ -3899,7 +3894,7 @@ static int qmp_pcie_probe(struct platform_device *pdev)
>>          if (ret)
>>                  goto err_node_put;
>>
>> -       ret = phy_pipe_clk_register(qmp, np);
>> +       ret = qmp_pcie_register_clocks(qmp, np);
>>          if (ret)
>>                  goto err_node_put;
>>
>>
>> --
>> 2.34.1
>>
>>
> 
> 

[PATCH 3/7] phy: qcom: qmp-pcie: register second optional PHY AUX clock

[Neil Armstrong]

The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock,
add the code to register it for PHYs configs that sets a aux_clock_rate.

In order to get the right clock, add qmp_pcie_clk_hw_get() which uses
the newly introduced QMP_PCIE_PIPE_CLK & QMP_PCIE_PHY_AUX_CLK clock
IDs and also supports the legacy bindings by returning the PIPE clock.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 70 ++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
index 079b3e306489..2d05226ae200 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
@@ -22,6 +22,8 @@
 #include <linux/reset.h>
 #include <linux/slab.h>
 
+#include <dt-bindings/phy/phy-qcom-qmp.h>
+
 #include "phy-qcom-qmp-common.h"
 
 #include "phy-qcom-qmp.h"
@@ -2389,6 +2391,9 @@ struct qmp_phy_cfg {
 
 	/* QMP PHY pipe clock interface rate */
 	unsigned long pipe_clock_rate;
+
+	/* QMP PHY AUX clock interface rate */
+	unsigned long aux_clock_rate;
 };
 
 struct qmp_pcie {
@@ -2420,6 +2425,7 @@ struct qmp_pcie {
 	int mode;
 
 	struct clk_fixed_rate pipe_clk_fixed;
+	struct clk_fixed_rate aux_clk_fixed;
 };
 
 static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val)
@@ -3681,6 +3687,62 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np)
 	return devm_clk_hw_register(qmp->dev, &fixed->hw);
 }
 
+/*
+ * Register a fixed rate PHY aux clock.
+ *
+ * The <s>_phy_aux_clksrc generated by PHY goes to the GCC that gate
+ * controls it. The <s>_phy_aux_clk coming out of the GCC is requested
+ * by the PHY driver for its operations.
+ * We register the <s>_phy_aux_clksrc here. The gcc driver takes care
+ * of assigning this <s>_phy_aux_clksrc as parent to <s>_phy_aux_clk.
+ * Below picture shows this relationship.
+ *
+ *         +---------------+
+ *         |   PHY block   |<<---------------------------------------------+
+ *         |               |                                               |
+ *         |   +-------+   |                      +-----+                  |
+ *   I/P---^-->|  PLL  |---^--->phy_aux_clksrc--->| GCC |--->phy_aux_clk---+
+ *    clk  |   +-------+   |                      +-----+
+ *         +---------------+
+ */
+static int phy_aux_clk_register(struct qmp_pcie *qmp, struct device_node *np)
+{
+	struct clk_fixed_rate *fixed = &qmp->aux_clk_fixed;
+	struct clk_init_data init = { };
+	int ret;
+
+	ret = of_property_read_string_index(np, "clock-output-names", 1, &init.name);
+	if (ret) {
+		dev_err(qmp->dev, "%pOFn: No clock-output-names index 1\n", np);
+		return ret;
+	}
+
+	init.ops = &clk_fixed_rate_ops;
+
+	fixed->fixed_rate = qmp->cfg->aux_clock_rate;
+	fixed->hw.init = &init;
+
+	return devm_clk_hw_register(qmp->dev, &fixed->hw);
+}
+
+static struct clk_hw *qmp_pcie_clk_hw_get(struct of_phandle_args *clkspec, void *data)
+{
+	struct qmp_pcie *qmp = data;
+
+	/* Support legacy bindings */
+	if (!clkspec->args_count)
+		return &qmp->pipe_clk_fixed.hw;
+
+	switch (clkspec->args[0]) {
+	case QMP_PCIE_PIPE_CLK:
+		return &qmp->pipe_clk_fixed.hw;
+	case QMP_PCIE_PHY_AUX_CLK:
+		return &qmp->aux_clk_fixed.hw;
+	}
+
+	return ERR_PTR(-EINVAL);
+}
+
 static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np)
 {
 	int ret;
@@ -3689,6 +3751,14 @@ static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np
 	if (ret)
 		return ret;
 
+	if (qmp->cfg->aux_clock_rate) {
+		ret = phy_aux_clk_register(qmp, np);
+		if (ret)
+			return ret;
+
+		return devm_of_clk_add_hw_provider(qmp->dev, qmp_pcie_clk_hw_get, qmp);
+	}
+
 	return devm_of_clk_add_hw_provider(qmp->dev, of_clk_hw_simple_get,
 					   &qmp->pipe_clk_fixed.hw);
 }

-- 
2.34.1

Re: [PATCH 3/7] phy: qcom: qmp-pcie: register second optional PHY AUX clock

[Dmitry Baryshkov]

On Tue, 19 Mar 2024 at 12:45, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>
> The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock,
> add the code to register it for PHYs configs that sets a aux_clock_rate.
>
> In order to get the right clock, add qmp_pcie_clk_hw_get() which uses
> the newly introduced QMP_PCIE_PIPE_CLK & QMP_PCIE_PHY_AUX_CLK clock
> IDs and also supports the legacy bindings by returning the PIPE clock.
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 70 ++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> index 079b3e306489..2d05226ae200 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> @@ -22,6 +22,8 @@
>  #include <linux/reset.h>
>  #include <linux/slab.h>
>
> +#include <dt-bindings/phy/phy-qcom-qmp.h>
> +
>  #include "phy-qcom-qmp-common.h"
>
>  #include "phy-qcom-qmp.h"
> @@ -2389,6 +2391,9 @@ struct qmp_phy_cfg {
>
>         /* QMP PHY pipe clock interface rate */
>         unsigned long pipe_clock_rate;
> +
> +       /* QMP PHY AUX clock interface rate */
> +       unsigned long aux_clock_rate;
>  };
>
>  struct qmp_pcie {
> @@ -2420,6 +2425,7 @@ struct qmp_pcie {
>         int mode;
>
>         struct clk_fixed_rate pipe_clk_fixed;
> +       struct clk_fixed_rate aux_clk_fixed;
>  };
>
>  static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val)
> @@ -3681,6 +3687,62 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np)
>         return devm_clk_hw_register(qmp->dev, &fixed->hw);
>  }
>
> +/*
> + * Register a fixed rate PHY aux clock.
> + *
> + * The <s>_phy_aux_clksrc generated by PHY goes to the GCC that gate
> + * controls it. The <s>_phy_aux_clk coming out of the GCC is requested
> + * by the PHY driver for its operations.
> + * We register the <s>_phy_aux_clksrc here. The gcc driver takes care
> + * of assigning this <s>_phy_aux_clksrc as parent to <s>_phy_aux_clk.
> + * Below picture shows this relationship.
> + *
> + *         +---------------+
> + *         |   PHY block   |<<---------------------------------------------+
> + *         |               |                                               |
> + *         |   +-------+   |                      +-----+                  |
> + *   I/P---^-->|  PLL  |---^--->phy_aux_clksrc--->| GCC |--->phy_aux_clk---+
> + *    clk  |   +-------+   |                      +-----+
> + *         +---------------+
> + */
> +static int phy_aux_clk_register(struct qmp_pcie *qmp, struct device_node *np)
> +{
> +       struct clk_fixed_rate *fixed = &qmp->aux_clk_fixed;
> +       struct clk_init_data init = { };
> +       int ret;
> +
> +       ret = of_property_read_string_index(np, "clock-output-names", 1, &init.name);
> +       if (ret) {
> +               dev_err(qmp->dev, "%pOFn: No clock-output-names index 1\n", np);
> +               return ret;
> +       }
> +
> +       init.ops = &clk_fixed_rate_ops;
> +
> +       fixed->fixed_rate = qmp->cfg->aux_clock_rate;
> +       fixed->hw.init = &init;
> +
> +       return devm_clk_hw_register(qmp->dev, &fixed->hw);
> +}
> +
> +static struct clk_hw *qmp_pcie_clk_hw_get(struct of_phandle_args *clkspec, void *data)
> +{
> +       struct qmp_pcie *qmp = data;
> +
> +       /* Support legacy bindings */
> +       if (!clkspec->args_count)
> +               return &qmp->pipe_clk_fixed.hw;
> +
> +       switch (clkspec->args[0]) {
> +       case QMP_PCIE_PIPE_CLK:
> +               return &qmp->pipe_clk_fixed.hw;
> +       case QMP_PCIE_PHY_AUX_CLK:
> +               return &qmp->aux_clk_fixed.hw;
> +       }
> +
> +       return ERR_PTR(-EINVAL);
> +}

Can we use of_clk_hw_onecell_get() instead? I think it even should be
possible to use onecell for both cases, it will look at the first arg,
which will be 0 in case of #clock-cells equal to 0.

> +
>  static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np)
>  {
>         int ret;
> @@ -3689,6 +3751,14 @@ static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np
>         if (ret)
>                 return ret;
>
> +       if (qmp->cfg->aux_clock_rate) {
> +               ret = phy_aux_clk_register(qmp, np);
> +               if (ret)
> +                       return ret;
> +
> +               return devm_of_clk_add_hw_provider(qmp->dev, qmp_pcie_clk_hw_get, qmp);
> +       }
> +
>         return devm_of_clk_add_hw_provider(qmp->dev, of_clk_hw_simple_get,
>                                            &qmp->pipe_clk_fixed.hw);
>  }
>
> --
> 2.34.1
>
>


-- 
With best wishes
Dmitry

Re: [PATCH 3/7] phy: qcom: qmp-pcie: register second optional PHY AUX clock

[Neil Armstrong]

On 19/03/2024 11:55, Dmitry Baryshkov wrote:
> On Tue, 19 Mar 2024 at 12:45, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>
>> The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock,
>> add the code to register it for PHYs configs that sets a aux_clock_rate.
>>
>> In order to get the right clock, add qmp_pcie_clk_hw_get() which uses
>> the newly introduced QMP_PCIE_PIPE_CLK & QMP_PCIE_PHY_AUX_CLK clock
>> IDs and also supports the legacy bindings by returning the PIPE clock.
>>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>>   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 70 ++++++++++++++++++++++++++++++++
>>   1 file changed, 70 insertions(+)
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>> index 079b3e306489..2d05226ae200 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>> @@ -22,6 +22,8 @@
>>   #include <linux/reset.h>
>>   #include <linux/slab.h>
>>
>> +#include <dt-bindings/phy/phy-qcom-qmp.h>
>> +
>>   #include "phy-qcom-qmp-common.h"
>>
>>   #include "phy-qcom-qmp.h"
>> @@ -2389,6 +2391,9 @@ struct qmp_phy_cfg {
>>
>>          /* QMP PHY pipe clock interface rate */
>>          unsigned long pipe_clock_rate;
>> +
>> +       /* QMP PHY AUX clock interface rate */
>> +       unsigned long aux_clock_rate;
>>   };
>>
>>   struct qmp_pcie {
>> @@ -2420,6 +2425,7 @@ struct qmp_pcie {
>>          int mode;
>>
>>          struct clk_fixed_rate pipe_clk_fixed;
>> +       struct clk_fixed_rate aux_clk_fixed;
>>   };
>>
>>   static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val)
>> @@ -3681,6 +3687,62 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np)
>>          return devm_clk_hw_register(qmp->dev, &fixed->hw);
>>   }
>>
>> +/*
>> + * Register a fixed rate PHY aux clock.
>> + *
>> + * The <s>_phy_aux_clksrc generated by PHY goes to the GCC that gate
>> + * controls it. The <s>_phy_aux_clk coming out of the GCC is requested
>> + * by the PHY driver for its operations.
>> + * We register the <s>_phy_aux_clksrc here. The gcc driver takes care
>> + * of assigning this <s>_phy_aux_clksrc as parent to <s>_phy_aux_clk.
>> + * Below picture shows this relationship.
>> + *
>> + *         +---------------+
>> + *         |   PHY block   |<<---------------------------------------------+
>> + *         |               |                                               |
>> + *         |   +-------+   |                      +-----+                  |
>> + *   I/P---^-->|  PLL  |---^--->phy_aux_clksrc--->| GCC |--->phy_aux_clk---+
>> + *    clk  |   +-------+   |                      +-----+
>> + *         +---------------+
>> + */
>> +static int phy_aux_clk_register(struct qmp_pcie *qmp, struct device_node *np)
>> +{
>> +       struct clk_fixed_rate *fixed = &qmp->aux_clk_fixed;
>> +       struct clk_init_data init = { };
>> +       int ret;
>> +
>> +       ret = of_property_read_string_index(np, "clock-output-names", 1, &init.name);
>> +       if (ret) {
>> +               dev_err(qmp->dev, "%pOFn: No clock-output-names index 1\n", np);
>> +               return ret;
>> +       }
>> +
>> +       init.ops = &clk_fixed_rate_ops;
>> +
>> +       fixed->fixed_rate = qmp->cfg->aux_clock_rate;
>> +       fixed->hw.init = &init;
>> +
>> +       return devm_clk_hw_register(qmp->dev, &fixed->hw);
>> +}
>> +
>> +static struct clk_hw *qmp_pcie_clk_hw_get(struct of_phandle_args *clkspec, void *data)
>> +{
>> +       struct qmp_pcie *qmp = data;
>> +
>> +       /* Support legacy bindings */
>> +       if (!clkspec->args_count)
>> +               return &qmp->pipe_clk_fixed.hw;
>> +
>> +       switch (clkspec->args[0]) {
>> +       case QMP_PCIE_PIPE_CLK:
>> +               return &qmp->pipe_clk_fixed.hw;
>> +       case QMP_PCIE_PHY_AUX_CLK:
>> +               return &qmp->aux_clk_fixed.hw;
>> +       }
>> +
>> +       return ERR_PTR(-EINVAL);
>> +}
> 
> Can we use of_clk_hw_onecell_get() instead? I think it even should be
> possible to use onecell for both cases, it will look at the first arg,
> which will be 0 in case of #clock-cells equal to 0.

Let me investigate if it's possible

> 
>> +
>>   static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np)
>>   {
>>          int ret;
>> @@ -3689,6 +3751,14 @@ static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np
>>          if (ret)
>>                  return ret;
>>
>> +       if (qmp->cfg->aux_clock_rate) {
>> +               ret = phy_aux_clk_register(qmp, np);
>> +               if (ret)
>> +                       return ret;
>> +
>> +               return devm_of_clk_add_hw_provider(qmp->dev, qmp_pcie_clk_hw_get, qmp);
>> +       }
>> +
>>          return devm_of_clk_add_hw_provider(qmp->dev, of_clk_hw_simple_get,
>>                                             &qmp->pipe_clk_fixed.hw);
>>   }
>>
>> --
>> 2.34.1
>>
>>
> 
> 

Re: [PATCH 3/7] phy: qcom: qmp-pcie: register second optional PHY AUX clock

[Neil Armstrong]

On 19/03/2024 11:59, Neil Armstrong wrote:
> On 19/03/2024 11:55, Dmitry Baryshkov wrote:
>> On Tue, 19 Mar 2024 at 12:45, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>>
>>> The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock,
>>> add the code to register it for PHYs configs that sets a aux_clock_rate.
>>>
>>> In order to get the right clock, add qmp_pcie_clk_hw_get() which uses
>>> the newly introduced QMP_PCIE_PIPE_CLK & QMP_PCIE_PHY_AUX_CLK clock
>>> IDs and also supports the legacy bindings by returning the PIPE clock.
>>>
>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>> ---
>>>   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 70 ++++++++++++++++++++++++++++++++
>>>   1 file changed, 70 insertions(+)
>>>
>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>>> index 079b3e306489..2d05226ae200 100644
>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>>> @@ -22,6 +22,8 @@
>>>   #include <linux/reset.h>
>>>   #include <linux/slab.h>
>>>
>>> +#include <dt-bindings/phy/phy-qcom-qmp.h>
>>> +
>>>   #include "phy-qcom-qmp-common.h"
>>>
>>>   #include "phy-qcom-qmp.h"
>>> @@ -2389,6 +2391,9 @@ struct qmp_phy_cfg {
>>>
>>>          /* QMP PHY pipe clock interface rate */
>>>          unsigned long pipe_clock_rate;
>>> +
>>> +       /* QMP PHY AUX clock interface rate */
>>> +       unsigned long aux_clock_rate;
>>>   };
>>>
>>>   struct qmp_pcie {
>>> @@ -2420,6 +2425,7 @@ struct qmp_pcie {
>>>          int mode;
>>>
>>>          struct clk_fixed_rate pipe_clk_fixed;
>>> +       struct clk_fixed_rate aux_clk_fixed;
>>>   };
>>>
>>>   static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val)
>>> @@ -3681,6 +3687,62 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np)
>>>          return devm_clk_hw_register(qmp->dev, &fixed->hw);
>>>   }
>>>
>>> +/*
>>> + * Register a fixed rate PHY aux clock.
>>> + *
>>> + * The <s>_phy_aux_clksrc generated by PHY goes to the GCC that gate
>>> + * controls it. The <s>_phy_aux_clk coming out of the GCC is requested
>>> + * by the PHY driver for its operations.
>>> + * We register the <s>_phy_aux_clksrc here. The gcc driver takes care
>>> + * of assigning this <s>_phy_aux_clksrc as parent to <s>_phy_aux_clk.
>>> + * Below picture shows this relationship.
>>> + *
>>> + *         +---------------+
>>> + *         |   PHY block   |<<---------------------------------------------+
>>> + *         |               |                                               |
>>> + *         |   +-------+   |                      +-----+                  |
>>> + *   I/P---^-->|  PLL  |---^--->phy_aux_clksrc--->| GCC |--->phy_aux_clk---+
>>> + *    clk  |   +-------+   |                      +-----+
>>> + *         +---------------+
>>> + */
>>> +static int phy_aux_clk_register(struct qmp_pcie *qmp, struct device_node *np)
>>> +{
>>> +       struct clk_fixed_rate *fixed = &qmp->aux_clk_fixed;
>>> +       struct clk_init_data init = { };
>>> +       int ret;
>>> +
>>> +       ret = of_property_read_string_index(np, "clock-output-names", 1, &init.name);
>>> +       if (ret) {
>>> +               dev_err(qmp->dev, "%pOFn: No clock-output-names index 1\n", np);
>>> +               return ret;
>>> +       }
>>> +
>>> +       init.ops = &clk_fixed_rate_ops;
>>> +
>>> +       fixed->fixed_rate = qmp->cfg->aux_clock_rate;
>>> +       fixed->hw.init = &init;
>>> +
>>> +       return devm_clk_hw_register(qmp->dev, &fixed->hw);
>>> +}
>>> +
>>> +static struct clk_hw *qmp_pcie_clk_hw_get(struct of_phandle_args *clkspec, void *data)
>>> +{
>>> +       struct qmp_pcie *qmp = data;
>>> +
>>> +       /* Support legacy bindings */
>>> +       if (!clkspec->args_count)
>>> +               return &qmp->pipe_clk_fixed.hw;
>>> +
>>> +       switch (clkspec->args[0]) {
>>> +       case QMP_PCIE_PIPE_CLK:
>>> +               return &qmp->pipe_clk_fixed.hw;
>>> +       case QMP_PCIE_PHY_AUX_CLK:
>>> +               return &qmp->aux_clk_fixed.hw;
>>> +       }
>>> +
>>> +       return ERR_PTR(-EINVAL);
>>> +}
>>
>> Can we use of_clk_hw_onecell_get() instead? I think it even should be
>> possible to use onecell for both cases, it will look at the first arg,
>> which will be 0 in case of #clock-cells equal to 0.
> 
> Let me investigate if it's possible

Ok, it would work but it would require building a clk_hw_onecell_data a runtime,
while we could simply provide this qmp_pcie_clk_hw_get() and avoid runtime 2 allocations.

I'm not sure it's worth it.

Neil

> 
>>
>>> +
>>>   static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np)
>>>   {
>>>          int ret;
>>> @@ -3689,6 +3751,14 @@ static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np
>>>          if (ret)
>>>                  return ret;
>>>
>>> +       if (qmp->cfg->aux_clock_rate) {
>>> +               ret = phy_aux_clk_register(qmp, np);
>>> +               if (ret)
>>> +                       return ret;
>>> +
>>> +               return devm_of_clk_add_hw_provider(qmp->dev, qmp_pcie_clk_hw_get, qmp);
>>> +       }
>>> +
>>>          return devm_of_clk_add_hw_provider(qmp->dev, of_clk_hw_simple_get,
>>>                                             &qmp->pipe_clk_fixed.hw);
>>>   }
>>>
>>> -- 
>>> 2.34.1
>>>
>>>
>>
>>
> 

Re: [PATCH 3/7] phy: qcom: qmp-pcie: register second optional PHY AUX clock

[Dmitry Baryshkov]

On Tue, 19 Mar 2024 at 16:35, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>
> On 19/03/2024 11:59, Neil Armstrong wrote:
> > On 19/03/2024 11:55, Dmitry Baryshkov wrote:
> >> On Tue, 19 Mar 2024 at 12:45, Neil Armstrong <neil.armstrong@linaro.org> wrote:
> >>>
> >>> The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock,
> >>> add the code to register it for PHYs configs that sets a aux_clock_rate.
> >>>
> >>> In order to get the right clock, add qmp_pcie_clk_hw_get() which uses
> >>> the newly introduced QMP_PCIE_PIPE_CLK & QMP_PCIE_PHY_AUX_CLK clock
> >>> IDs and also supports the legacy bindings by returning the PIPE clock.
> >>>
> >>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> >>> ---
> >>>   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 70 ++++++++++++++++++++++++++++++++
> >>>   1 file changed, 70 insertions(+)
> >>>
> >>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> >>> index 079b3e306489..2d05226ae200 100644
> >>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> >>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> >>> @@ -22,6 +22,8 @@
> >>>   #include <linux/reset.h>
> >>>   #include <linux/slab.h>
> >>>
> >>> +#include <dt-bindings/phy/phy-qcom-qmp.h>
> >>> +
> >>>   #include "phy-qcom-qmp-common.h"
> >>>
> >>>   #include "phy-qcom-qmp.h"
> >>> @@ -2389,6 +2391,9 @@ struct qmp_phy_cfg {
> >>>
> >>>          /* QMP PHY pipe clock interface rate */
> >>>          unsigned long pipe_clock_rate;
> >>> +
> >>> +       /* QMP PHY AUX clock interface rate */
> >>> +       unsigned long aux_clock_rate;
> >>>   };
> >>>
> >>>   struct qmp_pcie {
> >>> @@ -2420,6 +2425,7 @@ struct qmp_pcie {
> >>>          int mode;
> >>>
> >>>          struct clk_fixed_rate pipe_clk_fixed;
> >>> +       struct clk_fixed_rate aux_clk_fixed;
> >>>   };
> >>>
> >>>   static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val)
> >>> @@ -3681,6 +3687,62 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np)
> >>>          return devm_clk_hw_register(qmp->dev, &fixed->hw);
> >>>   }
> >>>
> >>> +/*
> >>> + * Register a fixed rate PHY aux clock.
> >>> + *
> >>> + * The <s>_phy_aux_clksrc generated by PHY goes to the GCC that gate
> >>> + * controls it. The <s>_phy_aux_clk coming out of the GCC is requested
> >>> + * by the PHY driver for its operations.
> >>> + * We register the <s>_phy_aux_clksrc here. The gcc driver takes care
> >>> + * of assigning this <s>_phy_aux_clksrc as parent to <s>_phy_aux_clk.
> >>> + * Below picture shows this relationship.
> >>> + *
> >>> + *         +---------------+
> >>> + *         |   PHY block   |<<---------------------------------------------+
> >>> + *         |               |                                               |
> >>> + *         |   +-------+   |                      +-----+                  |
> >>> + *   I/P---^-->|  PLL  |---^--->phy_aux_clksrc--->| GCC |--->phy_aux_clk---+
> >>> + *    clk  |   +-------+   |                      +-----+
> >>> + *         +---------------+
> >>> + */
> >>> +static int phy_aux_clk_register(struct qmp_pcie *qmp, struct device_node *np)
> >>> +{
> >>> +       struct clk_fixed_rate *fixed = &qmp->aux_clk_fixed;
> >>> +       struct clk_init_data init = { };
> >>> +       int ret;
> >>> +
> >>> +       ret = of_property_read_string_index(np, "clock-output-names", 1, &init.name);
> >>> +       if (ret) {
> >>> +               dev_err(qmp->dev, "%pOFn: No clock-output-names index 1\n", np);
> >>> +               return ret;
> >>> +       }
> >>> +
> >>> +       init.ops = &clk_fixed_rate_ops;
> >>> +
> >>> +       fixed->fixed_rate = qmp->cfg->aux_clock_rate;
> >>> +       fixed->hw.init = &init;
> >>> +
> >>> +       return devm_clk_hw_register(qmp->dev, &fixed->hw);
> >>> +}
> >>> +
> >>> +static struct clk_hw *qmp_pcie_clk_hw_get(struct of_phandle_args *clkspec, void *data)
> >>> +{
> >>> +       struct qmp_pcie *qmp = data;
> >>> +
> >>> +       /* Support legacy bindings */
> >>> +       if (!clkspec->args_count)
> >>> +               return &qmp->pipe_clk_fixed.hw;
> >>> +
> >>> +       switch (clkspec->args[0]) {
> >>> +       case QMP_PCIE_PIPE_CLK:
> >>> +               return &qmp->pipe_clk_fixed.hw;
> >>> +       case QMP_PCIE_PHY_AUX_CLK:
> >>> +               return &qmp->aux_clk_fixed.hw;
> >>> +       }
> >>> +
> >>> +       return ERR_PTR(-EINVAL);
> >>> +}
> >>
> >> Can we use of_clk_hw_onecell_get() instead? I think it even should be
> >> possible to use onecell for both cases, it will look at the first arg,
> >> which will be 0 in case of #clock-cells equal to 0.
> >
> > Let me investigate if it's possible
>
> Ok, it would work but it would require building a clk_hw_onecell_data a runtime,
> while we could simply provide this qmp_pcie_clk_hw_get() and avoid runtime 2 allocations.
>
> I'm not sure it's worth it.

Single allocation (or even 0 allocations if you embed it into struct
qmp_pcie) for the sake of using standard helpers.



-- 
With best wishes
Dmitry

Re: [PATCH 3/7] phy: qcom: qmp-pcie: register second optional PHY AUX clock

[neil.armstrong]

On 19/03/2024 15:46, Dmitry Baryshkov wrote:
> On Tue, 19 Mar 2024 at 16:35, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>
>> On 19/03/2024 11:59, Neil Armstrong wrote:
>>> On 19/03/2024 11:55, Dmitry Baryshkov wrote:
>>>> On Tue, 19 Mar 2024 at 12:45, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>>>>
>>>>> The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock,
>>>>> add the code to register it for PHYs configs that sets a aux_clock_rate.
>>>>>
>>>>> In order to get the right clock, add qmp_pcie_clk_hw_get() which uses
>>>>> the newly introduced QMP_PCIE_PIPE_CLK & QMP_PCIE_PHY_AUX_CLK clock
>>>>> IDs and also supports the legacy bindings by returning the PIPE clock.
>>>>>
>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>>> ---
>>>>>    drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 70 ++++++++++++++++++++++++++++++++
>>>>>    1 file changed, 70 insertions(+)
>>>>>
>>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>>>>> index 079b3e306489..2d05226ae200 100644
>>>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>>>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>>>>> @@ -22,6 +22,8 @@
>>>>>    #include <linux/reset.h>
>>>>>    #include <linux/slab.h>
>>>>>
>>>>> +#include <dt-bindings/phy/phy-qcom-qmp.h>
>>>>> +
>>>>>    #include "phy-qcom-qmp-common.h"
>>>>>
>>>>>    #include "phy-qcom-qmp.h"
>>>>> @@ -2389,6 +2391,9 @@ struct qmp_phy_cfg {
>>>>>
>>>>>           /* QMP PHY pipe clock interface rate */
>>>>>           unsigned long pipe_clock_rate;
>>>>> +
>>>>> +       /* QMP PHY AUX clock interface rate */
>>>>> +       unsigned long aux_clock_rate;
>>>>>    };
>>>>>
>>>>>    struct qmp_pcie {
>>>>> @@ -2420,6 +2425,7 @@ struct qmp_pcie {
>>>>>           int mode;
>>>>>
>>>>>           struct clk_fixed_rate pipe_clk_fixed;
>>>>> +       struct clk_fixed_rate aux_clk_fixed;
>>>>>    };
>>>>>
>>>>>    static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val)
>>>>> @@ -3681,6 +3687,62 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np)
>>>>>           return devm_clk_hw_register(qmp->dev, &fixed->hw);
>>>>>    }
>>>>>
>>>>> +/*
>>>>> + * Register a fixed rate PHY aux clock.
>>>>> + *
>>>>> + * The <s>_phy_aux_clksrc generated by PHY goes to the GCC that gate
>>>>> + * controls it. The <s>_phy_aux_clk coming out of the GCC is requested
>>>>> + * by the PHY driver for its operations.
>>>>> + * We register the <s>_phy_aux_clksrc here. The gcc driver takes care
>>>>> + * of assigning this <s>_phy_aux_clksrc as parent to <s>_phy_aux_clk.
>>>>> + * Below picture shows this relationship.
>>>>> + *
>>>>> + *         +---------------+
>>>>> + *         |   PHY block   |<<---------------------------------------------+
>>>>> + *         |               |                                               |
>>>>> + *         |   +-------+   |                      +-----+                  |
>>>>> + *   I/P---^-->|  PLL  |---^--->phy_aux_clksrc--->| GCC |--->phy_aux_clk---+
>>>>> + *    clk  |   +-------+   |                      +-----+
>>>>> + *         +---------------+
>>>>> + */
>>>>> +static int phy_aux_clk_register(struct qmp_pcie *qmp, struct device_node *np)
>>>>> +{
>>>>> +       struct clk_fixed_rate *fixed = &qmp->aux_clk_fixed;
>>>>> +       struct clk_init_data init = { };
>>>>> +       int ret;
>>>>> +
>>>>> +       ret = of_property_read_string_index(np, "clock-output-names", 1, &init.name);
>>>>> +       if (ret) {
>>>>> +               dev_err(qmp->dev, "%pOFn: No clock-output-names index 1\n", np);
>>>>> +               return ret;
>>>>> +       }
>>>>> +
>>>>> +       init.ops = &clk_fixed_rate_ops;
>>>>> +
>>>>> +       fixed->fixed_rate = qmp->cfg->aux_clock_rate;
>>>>> +       fixed->hw.init = &init;
>>>>> +
>>>>> +       return devm_clk_hw_register(qmp->dev, &fixed->hw);
>>>>> +}
>>>>> +
>>>>> +static struct clk_hw *qmp_pcie_clk_hw_get(struct of_phandle_args *clkspec, void *data)
>>>>> +{
>>>>> +       struct qmp_pcie *qmp = data;
>>>>> +
>>>>> +       /* Support legacy bindings */
>>>>> +       if (!clkspec->args_count)
>>>>> +               return &qmp->pipe_clk_fixed.hw;
>>>>> +
>>>>> +       switch (clkspec->args[0]) {
>>>>> +       case QMP_PCIE_PIPE_CLK:
>>>>> +               return &qmp->pipe_clk_fixed.hw;
>>>>> +       case QMP_PCIE_PHY_AUX_CLK:
>>>>> +               return &qmp->aux_clk_fixed.hw;
>>>>> +       }
>>>>> +
>>>>> +       return ERR_PTR(-EINVAL);
>>>>> +}
>>>>
>>>> Can we use of_clk_hw_onecell_get() instead? I think it even should be
>>>> possible to use onecell for both cases, it will look at the first arg,
>>>> which will be 0 in case of #clock-cells equal to 0.

I didn't find evidence this is the case, while following of_parse_clkspec() called
from of_clk_get_hw() or clk_core_get(), where clkspec is not initialized, the
__of_parse_phandle_with_args() and of_phandle_iterator_args() don't touch
clkspec->args if it->cur_count is 0. So clkspec->args[0] may be left initialized
and it would be abusingthe API to use it with #clocks-cells = 0.

>>>
>>> Let me investigate if it's possible
>>
>> Ok, it would work but it would require building a clk_hw_onecell_data a runtime,
>> while we could simply provide this qmp_pcie_clk_hw_get() and avoid runtime 2 allocations.
>>
>> I'm not sure it's worth it.
> 
> Single allocation (or even 0 allocations if you embed it into struct
> qmp_pcie) for the sake of using standard helpers.
> 
Well, sure

Thanks,
Neil

Re: [PATCH 3/7] phy: qcom: qmp-pcie: register second optional PHY AUX clock

[Dmitry Baryshkov]

On Tue, 19 Mar 2024 at 17:10, <neil.armstrong@linaro.org> wrote:
>
> On 19/03/2024 15:46, Dmitry Baryshkov wrote:
> > On Tue, 19 Mar 2024 at 16:35, Neil Armstrong <neil.armstrong@linaro.org> wrote:
> >>
> >> On 19/03/2024 11:59, Neil Armstrong wrote:
> >>> On 19/03/2024 11:55, Dmitry Baryshkov wrote:
> >>>> On Tue, 19 Mar 2024 at 12:45, Neil Armstrong <neil.armstrong@linaro.org> wrote:
> >>>>>
> >>>>> The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock,
> >>>>> add the code to register it for PHYs configs that sets a aux_clock_rate.
> >>>>>
> >>>>> In order to get the right clock, add qmp_pcie_clk_hw_get() which uses
> >>>>> the newly introduced QMP_PCIE_PIPE_CLK & QMP_PCIE_PHY_AUX_CLK clock
> >>>>> IDs and also supports the legacy bindings by returning the PIPE clock.
> >>>>>
> >>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> >>>>> ---
> >>>>>    drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 70 ++++++++++++++++++++++++++++++++
> >>>>>    1 file changed, 70 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> >>>>> index 079b3e306489..2d05226ae200 100644
> >>>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> >>>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> >>>>> @@ -22,6 +22,8 @@
> >>>>>    #include <linux/reset.h>
> >>>>>    #include <linux/slab.h>
> >>>>>
> >>>>> +#include <dt-bindings/phy/phy-qcom-qmp.h>
> >>>>> +
> >>>>>    #include "phy-qcom-qmp-common.h"
> >>>>>
> >>>>>    #include "phy-qcom-qmp.h"
> >>>>> @@ -2389,6 +2391,9 @@ struct qmp_phy_cfg {
> >>>>>
> >>>>>           /* QMP PHY pipe clock interface rate */
> >>>>>           unsigned long pipe_clock_rate;
> >>>>> +
> >>>>> +       /* QMP PHY AUX clock interface rate */
> >>>>> +       unsigned long aux_clock_rate;
> >>>>>    };
> >>>>>
> >>>>>    struct qmp_pcie {
> >>>>> @@ -2420,6 +2425,7 @@ struct qmp_pcie {
> >>>>>           int mode;
> >>>>>
> >>>>>           struct clk_fixed_rate pipe_clk_fixed;
> >>>>> +       struct clk_fixed_rate aux_clk_fixed;
> >>>>>    };
> >>>>>
> >>>>>    static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val)
> >>>>> @@ -3681,6 +3687,62 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np)
> >>>>>           return devm_clk_hw_register(qmp->dev, &fixed->hw);
> >>>>>    }
> >>>>>
> >>>>> +/*
> >>>>> + * Register a fixed rate PHY aux clock.
> >>>>> + *
> >>>>> + * The <s>_phy_aux_clksrc generated by PHY goes to the GCC that gate
> >>>>> + * controls it. The <s>_phy_aux_clk coming out of the GCC is requested
> >>>>> + * by the PHY driver for its operations.
> >>>>> + * We register the <s>_phy_aux_clksrc here. The gcc driver takes care
> >>>>> + * of assigning this <s>_phy_aux_clksrc as parent to <s>_phy_aux_clk.
> >>>>> + * Below picture shows this relationship.
> >>>>> + *
> >>>>> + *         +---------------+
> >>>>> + *         |   PHY block   |<<---------------------------------------------+
> >>>>> + *         |               |                                               |
> >>>>> + *         |   +-------+   |                      +-----+                  |
> >>>>> + *   I/P---^-->|  PLL  |---^--->phy_aux_clksrc--->| GCC |--->phy_aux_clk---+
> >>>>> + *    clk  |   +-------+   |                      +-----+
> >>>>> + *         +---------------+
> >>>>> + */
> >>>>> +static int phy_aux_clk_register(struct qmp_pcie *qmp, struct device_node *np)
> >>>>> +{
> >>>>> +       struct clk_fixed_rate *fixed = &qmp->aux_clk_fixed;
> >>>>> +       struct clk_init_data init = { };
> >>>>> +       int ret;
> >>>>> +
> >>>>> +       ret = of_property_read_string_index(np, "clock-output-names", 1, &init.name);
> >>>>> +       if (ret) {
> >>>>> +               dev_err(qmp->dev, "%pOFn: No clock-output-names index 1\n", np);
> >>>>> +               return ret;
> >>>>> +       }
> >>>>> +
> >>>>> +       init.ops = &clk_fixed_rate_ops;
> >>>>> +
> >>>>> +       fixed->fixed_rate = qmp->cfg->aux_clock_rate;
> >>>>> +       fixed->hw.init = &init;
> >>>>> +
> >>>>> +       return devm_clk_hw_register(qmp->dev, &fixed->hw);
> >>>>> +}
> >>>>> +
> >>>>> +static struct clk_hw *qmp_pcie_clk_hw_get(struct of_phandle_args *clkspec, void *data)
> >>>>> +{
> >>>>> +       struct qmp_pcie *qmp = data;
> >>>>> +
> >>>>> +       /* Support legacy bindings */
> >>>>> +       if (!clkspec->args_count)
> >>>>> +               return &qmp->pipe_clk_fixed.hw;
> >>>>> +
> >>>>> +       switch (clkspec->args[0]) {
> >>>>> +       case QMP_PCIE_PIPE_CLK:
> >>>>> +               return &qmp->pipe_clk_fixed.hw;
> >>>>> +       case QMP_PCIE_PHY_AUX_CLK:
> >>>>> +               return &qmp->aux_clk_fixed.hw;
> >>>>> +       }
> >>>>> +
> >>>>> +       return ERR_PTR(-EINVAL);
> >>>>> +}
> >>>>
> >>>> Can we use of_clk_hw_onecell_get() instead? I think it even should be
> >>>> possible to use onecell for both cases, it will look at the first arg,
> >>>> which will be 0 in case of #clock-cells equal to 0.
>
> I didn't find evidence this is the case, while following of_parse_clkspec() called
> from of_clk_get_hw() or clk_core_get(), where clkspec is not initialized, the
> __of_parse_phandle_with_args() and of_phandle_iterator_args() don't touch
> clkspec->args if it->cur_count is 0. So clkspec->args[0] may be left initialized
> and it would be abusingthe API to use it with #clocks-cells = 0.

Ack, true.

>
> >>>
> >>> Let me investigate if it's possible
> >>
> >> Ok, it would work but it would require building a clk_hw_onecell_data a runtime,
> >> while we could simply provide this qmp_pcie_clk_hw_get() and avoid runtime 2 allocations.
> >>
> >> I'm not sure it's worth it.
> >
> > Single allocation (or even 0 allocations if you embed it into struct
> > qmp_pcie) for the sake of using standard helpers.
> >
> Well, sure
>
> Thanks,
> Neil
>


-- 
With best wishes
Dmitry

Re: [PATCH 3/7] phy: qcom: qmp-pcie: register second optional PHY AUX clock

[neil.armstrong]

On 19/03/2024 15:46, Dmitry Baryshkov wrote:
> On Tue, 19 Mar 2024 at 16:35, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>
>> On 19/03/2024 11:59, Neil Armstrong wrote:
>>> On 19/03/2024 11:55, Dmitry Baryshkov wrote:
>>>> On Tue, 19 Mar 2024 at 12:45, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>>>>
>>>>> The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock,
>>>>> add the code to register it for PHYs configs that sets a aux_clock_rate.
>>>>>
>>>>> In order to get the right clock, add qmp_pcie_clk_hw_get() which uses
>>>>> the newly introduced QMP_PCIE_PIPE_CLK & QMP_PCIE_PHY_AUX_CLK clock
>>>>> IDs and also supports the legacy bindings by returning the PIPE clock.
>>>>>
>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>>> ---
>>>>>    drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 70 ++++++++++++++++++++++++++++++++
>>>>>    1 file changed, 70 insertions(+)
>>>>>
>>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>>>>> index 079b3e306489..2d05226ae200 100644
>>>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>>>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>>>>> @@ -22,6 +22,8 @@
>>>>>    #include <linux/reset.h>
>>>>>    #include <linux/slab.h>
>>>>>
>>>>> +#include <dt-bindings/phy/phy-qcom-qmp.h>
>>>>> +
>>>>>    #include "phy-qcom-qmp-common.h"
>>>>>
>>>>>    #include "phy-qcom-qmp.h"
>>>>> @@ -2389,6 +2391,9 @@ struct qmp_phy_cfg {
>>>>>
>>>>>           /* QMP PHY pipe clock interface rate */
>>>>>           unsigned long pipe_clock_rate;
>>>>> +
>>>>> +       /* QMP PHY AUX clock interface rate */
>>>>> +       unsigned long aux_clock_rate;
>>>>>    };
>>>>>
>>>>>    struct qmp_pcie {
>>>>> @@ -2420,6 +2425,7 @@ struct qmp_pcie {
>>>>>           int mode;
>>>>>
>>>>>           struct clk_fixed_rate pipe_clk_fixed;
>>>>> +       struct clk_fixed_rate aux_clk_fixed;
>>>>>    };
>>>>>
>>>>>    static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val)
>>>>> @@ -3681,6 +3687,62 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np)
>>>>>           return devm_clk_hw_register(qmp->dev, &fixed->hw);
>>>>>    }
>>>>>
>>>>> +/*
>>>>> + * Register a fixed rate PHY aux clock.
>>>>> + *
>>>>> + * The <s>_phy_aux_clksrc generated by PHY goes to the GCC that gate
>>>>> + * controls it. The <s>_phy_aux_clk coming out of the GCC is requested
>>>>> + * by the PHY driver for its operations.
>>>>> + * We register the <s>_phy_aux_clksrc here. The gcc driver takes care
>>>>> + * of assigning this <s>_phy_aux_clksrc as parent to <s>_phy_aux_clk.
>>>>> + * Below picture shows this relationship.
>>>>> + *
>>>>> + *         +---------------+
>>>>> + *         |   PHY block   |<<---------------------------------------------+
>>>>> + *         |               |                                               |
>>>>> + *         |   +-------+   |                      +-----+                  |
>>>>> + *   I/P---^-->|  PLL  |---^--->phy_aux_clksrc--->| GCC |--->phy_aux_clk---+
>>>>> + *    clk  |   +-------+   |                      +-----+
>>>>> + *         +---------------+
>>>>> + */
>>>>> +static int phy_aux_clk_register(struct qmp_pcie *qmp, struct device_node *np)
>>>>> +{
>>>>> +       struct clk_fixed_rate *fixed = &qmp->aux_clk_fixed;
>>>>> +       struct clk_init_data init = { };
>>>>> +       int ret;
>>>>> +
>>>>> +       ret = of_property_read_string_index(np, "clock-output-names", 1, &init.name);
>>>>> +       if (ret) {
>>>>> +               dev_err(qmp->dev, "%pOFn: No clock-output-names index 1\n", np);
>>>>> +               return ret;
>>>>> +       }
>>>>> +
>>>>> +       init.ops = &clk_fixed_rate_ops;
>>>>> +
>>>>> +       fixed->fixed_rate = qmp->cfg->aux_clock_rate;
>>>>> +       fixed->hw.init = &init;
>>>>> +
>>>>> +       return devm_clk_hw_register(qmp->dev, &fixed->hw);
>>>>> +}
>>>>> +
>>>>> +static struct clk_hw *qmp_pcie_clk_hw_get(struct of_phandle_args *clkspec, void *data)
>>>>> +{
>>>>> +       struct qmp_pcie *qmp = data;
>>>>> +
>>>>> +       /* Support legacy bindings */
>>>>> +       if (!clkspec->args_count)
>>>>> +               return &qmp->pipe_clk_fixed.hw;
>>>>> +
>>>>> +       switch (clkspec->args[0]) {
>>>>> +       case QMP_PCIE_PIPE_CLK:
>>>>> +               return &qmp->pipe_clk_fixed.hw;
>>>>> +       case QMP_PCIE_PHY_AUX_CLK:
>>>>> +               return &qmp->aux_clk_fixed.hw;
>>>>> +       }
>>>>> +
>>>>> +       return ERR_PTR(-EINVAL);
>>>>> +}
>>>>
>>>> Can we use of_clk_hw_onecell_get() instead? I think it even should be
>>>> possible to use onecell for both cases, it will look at the first arg,
>>>> which will be 0 in case of #clock-cells equal to 0.
>>>
>>> Let me investigate if it's possible
>>
>> Ok, it would work but it would require building a clk_hw_onecell_data a runtime,
>> while we could simply provide this qmp_pcie_clk_hw_get() and avoid runtime 2 allocations.
>>
>> I'm not sure it's worth it.
> 
> Single allocation (or even 0 allocations if you embed it into struct
> qmp_pcie) for the sake of using standard helpers.

And I just recall I tried the same for Amlogic clocks, but the clk_hw_onecell_data hws
field is a flexible array member you can't set at runtime, if you try you'll get:
drivers/phy/qualcomm/phy-qcom-qmp-pcie.c:3753:38: error: invalid use of flexible array member
  3753 |                 qmp->clk_hw_data.hws = qmp->clk_hws;

Neil

> 
> 
> 

Re: [PATCH 3/7] phy: qcom: qmp-pcie: register second optional PHY AUX clock

[Dmitry Baryshkov]

On Tue, 19 Mar 2024 at 17:15, <neil.armstrong@linaro.org> wrote:
>
> On 19/03/2024 15:46, Dmitry Baryshkov wrote:
> > On Tue, 19 Mar 2024 at 16:35, Neil Armstrong <neil.armstrong@linaro.org> wrote:
> >>
> >> On 19/03/2024 11:59, Neil Armstrong wrote:
> >>> On 19/03/2024 11:55, Dmitry Baryshkov wrote:
> >>>> On Tue, 19 Mar 2024 at 12:45, Neil Armstrong <neil.armstrong@linaro.org> wrote:
> >>>>>
> >>>>> The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock,
> >>>>> add the code to register it for PHYs configs that sets a aux_clock_rate.
> >>>>>
> >>>>> In order to get the right clock, add qmp_pcie_clk_hw_get() which uses
> >>>>> the newly introduced QMP_PCIE_PIPE_CLK & QMP_PCIE_PHY_AUX_CLK clock
> >>>>> IDs and also supports the legacy bindings by returning the PIPE clock.
> >>>>>
> >>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> >>>>> ---
> >>>>>    drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 70 ++++++++++++++++++++++++++++++++
> >>>>>    1 file changed, 70 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> >>>>> index 079b3e306489..2d05226ae200 100644
> >>>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> >>>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> >>>>> @@ -22,6 +22,8 @@
> >>>>>    #include <linux/reset.h>
> >>>>>    #include <linux/slab.h>
> >>>>>
> >>>>> +#include <dt-bindings/phy/phy-qcom-qmp.h>
> >>>>> +
> >>>>>    #include "phy-qcom-qmp-common.h"
> >>>>>
> >>>>>    #include "phy-qcom-qmp.h"
> >>>>> @@ -2389,6 +2391,9 @@ struct qmp_phy_cfg {
> >>>>>
> >>>>>           /* QMP PHY pipe clock interface rate */
> >>>>>           unsigned long pipe_clock_rate;
> >>>>> +
> >>>>> +       /* QMP PHY AUX clock interface rate */
> >>>>> +       unsigned long aux_clock_rate;
> >>>>>    };
> >>>>>
> >>>>>    struct qmp_pcie {
> >>>>> @@ -2420,6 +2425,7 @@ struct qmp_pcie {
> >>>>>           int mode;
> >>>>>
> >>>>>           struct clk_fixed_rate pipe_clk_fixed;
> >>>>> +       struct clk_fixed_rate aux_clk_fixed;
> >>>>>    };
> >>>>>
> >>>>>    static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val)
> >>>>> @@ -3681,6 +3687,62 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np)
> >>>>>           return devm_clk_hw_register(qmp->dev, &fixed->hw);
> >>>>>    }
> >>>>>
> >>>>> +/*
> >>>>> + * Register a fixed rate PHY aux clock.
> >>>>> + *
> >>>>> + * The <s>_phy_aux_clksrc generated by PHY goes to the GCC that gate
> >>>>> + * controls it. The <s>_phy_aux_clk coming out of the GCC is requested
> >>>>> + * by the PHY driver for its operations.
> >>>>> + * We register the <s>_phy_aux_clksrc here. The gcc driver takes care
> >>>>> + * of assigning this <s>_phy_aux_clksrc as parent to <s>_phy_aux_clk.
> >>>>> + * Below picture shows this relationship.
> >>>>> + *
> >>>>> + *         +---------------+
> >>>>> + *         |   PHY block   |<<---------------------------------------------+
> >>>>> + *         |               |                                               |
> >>>>> + *         |   +-------+   |                      +-----+                  |
> >>>>> + *   I/P---^-->|  PLL  |---^--->phy_aux_clksrc--->| GCC |--->phy_aux_clk---+
> >>>>> + *    clk  |   +-------+   |                      +-----+
> >>>>> + *         +---------------+
> >>>>> + */
> >>>>> +static int phy_aux_clk_register(struct qmp_pcie *qmp, struct device_node *np)
> >>>>> +{
> >>>>> +       struct clk_fixed_rate *fixed = &qmp->aux_clk_fixed;
> >>>>> +       struct clk_init_data init = { };
> >>>>> +       int ret;
> >>>>> +
> >>>>> +       ret = of_property_read_string_index(np, "clock-output-names", 1, &init.name);
> >>>>> +       if (ret) {
> >>>>> +               dev_err(qmp->dev, "%pOFn: No clock-output-names index 1\n", np);
> >>>>> +               return ret;
> >>>>> +       }
> >>>>> +
> >>>>> +       init.ops = &clk_fixed_rate_ops;
> >>>>> +
> >>>>> +       fixed->fixed_rate = qmp->cfg->aux_clock_rate;
> >>>>> +       fixed->hw.init = &init;
> >>>>> +
> >>>>> +       return devm_clk_hw_register(qmp->dev, &fixed->hw);
> >>>>> +}
> >>>>> +
> >>>>> +static struct clk_hw *qmp_pcie_clk_hw_get(struct of_phandle_args *clkspec, void *data)
> >>>>> +{
> >>>>> +       struct qmp_pcie *qmp = data;
> >>>>> +
> >>>>> +       /* Support legacy bindings */
> >>>>> +       if (!clkspec->args_count)
> >>>>> +               return &qmp->pipe_clk_fixed.hw;
> >>>>> +
> >>>>> +       switch (clkspec->args[0]) {
> >>>>> +       case QMP_PCIE_PIPE_CLK:
> >>>>> +               return &qmp->pipe_clk_fixed.hw;
> >>>>> +       case QMP_PCIE_PHY_AUX_CLK:
> >>>>> +               return &qmp->aux_clk_fixed.hw;
> >>>>> +       }
> >>>>> +
> >>>>> +       return ERR_PTR(-EINVAL);
> >>>>> +}
> >>>>
> >>>> Can we use of_clk_hw_onecell_get() instead? I think it even should be
> >>>> possible to use onecell for both cases, it will look at the first arg,
> >>>> which will be 0 in case of #clock-cells equal to 0.
> >>>
> >>> Let me investigate if it's possible
> >>
> >> Ok, it would work but it would require building a clk_hw_onecell_data a runtime,
> >> while we could simply provide this qmp_pcie_clk_hw_get() and avoid runtime 2 allocations.
> >>
> >> I'm not sure it's worth it.
> >
> > Single allocation (or even 0 allocations if you embed it into struct
> > qmp_pcie) for the sake of using standard helpers.
>
> And I just recall I tried the same for Amlogic clocks, but the clk_hw_onecell_data hws
> field is a flexible array member you can't set at runtime, if you try you'll get:
> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c:3753:38: error: invalid use of flexible array member
>   3753 |                 qmp->clk_hw_data.hws = qmp->clk_hws;

Yes, so it's either
devm_kzalloc(dev, struct_size(data, hws, 2), GFP_KERNEL);
or
struct qmp_pcie {
...
  struct {
    struct clk_hw_onecell_data clk_data;
    struct clk_hw clocks[2];
  };
};

>
> Neil
>
> >
> >
> >
>


-- 
With best wishes
Dmitry

Re: [PATCH 3/7] phy: qcom: qmp-pcie: register second optional PHY AUX clock

[neil.armstrong]

On 19/03/2024 17:05, Dmitry Baryshkov wrote:
> On Tue, 19 Mar 2024 at 17:15, <neil.armstrong@linaro.org> wrote:
>>
>> On 19/03/2024 15:46, Dmitry Baryshkov wrote:
>>> On Tue, 19 Mar 2024 at 16:35, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>>>
>>>> On 19/03/2024 11:59, Neil Armstrong wrote:
>>>>> On 19/03/2024 11:55, Dmitry Baryshkov wrote:
>>>>>> On Tue, 19 Mar 2024 at 12:45, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>>>>>>
>>>>>>> The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock,
>>>>>>> add the code to register it for PHYs configs that sets a aux_clock_rate.
>>>>>>>
>>>>>>> In order to get the right clock, add qmp_pcie_clk_hw_get() which uses
>>>>>>> the newly introduced QMP_PCIE_PIPE_CLK & QMP_PCIE_PHY_AUX_CLK clock
>>>>>>> IDs and also supports the legacy bindings by returning the PIPE clock.
>>>>>>>
>>>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>>>>> ---
>>>>>>>     drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 70 ++++++++++++++++++++++++++++++++
>>>>>>>     1 file changed, 70 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>>>>>>> index 079b3e306489..2d05226ae200 100644
>>>>>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>>>>>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>>>>>>> @@ -22,6 +22,8 @@
>>>>>>>     #include <linux/reset.h>
>>>>>>>     #include <linux/slab.h>
>>>>>>>
>>>>>>> +#include <dt-bindings/phy/phy-qcom-qmp.h>
>>>>>>> +
>>>>>>>     #include "phy-qcom-qmp-common.h"
>>>>>>>
>>>>>>>     #include "phy-qcom-qmp.h"
>>>>>>> @@ -2389,6 +2391,9 @@ struct qmp_phy_cfg {
>>>>>>>
>>>>>>>            /* QMP PHY pipe clock interface rate */
>>>>>>>            unsigned long pipe_clock_rate;
>>>>>>> +
>>>>>>> +       /* QMP PHY AUX clock interface rate */
>>>>>>> +       unsigned long aux_clock_rate;
>>>>>>>     };
>>>>>>>
>>>>>>>     struct qmp_pcie {
>>>>>>> @@ -2420,6 +2425,7 @@ struct qmp_pcie {
>>>>>>>            int mode;
>>>>>>>
>>>>>>>            struct clk_fixed_rate pipe_clk_fixed;
>>>>>>> +       struct clk_fixed_rate aux_clk_fixed;
>>>>>>>     };
>>>>>>>
>>>>>>>     static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val)
>>>>>>> @@ -3681,6 +3687,62 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np)
>>>>>>>            return devm_clk_hw_register(qmp->dev, &fixed->hw);
>>>>>>>     }
>>>>>>>
>>>>>>> +/*
>>>>>>> + * Register a fixed rate PHY aux clock.
>>>>>>> + *
>>>>>>> + * The <s>_phy_aux_clksrc generated by PHY goes to the GCC that gate
>>>>>>> + * controls it. The <s>_phy_aux_clk coming out of the GCC is requested
>>>>>>> + * by the PHY driver for its operations.
>>>>>>> + * We register the <s>_phy_aux_clksrc here. The gcc driver takes care
>>>>>>> + * of assigning this <s>_phy_aux_clksrc as parent to <s>_phy_aux_clk.
>>>>>>> + * Below picture shows this relationship.
>>>>>>> + *
>>>>>>> + *         +---------------+
>>>>>>> + *         |   PHY block   |<<---------------------------------------------+
>>>>>>> + *         |               |                                               |
>>>>>>> + *         |   +-------+   |                      +-----+                  |
>>>>>>> + *   I/P---^-->|  PLL  |---^--->phy_aux_clksrc--->| GCC |--->phy_aux_clk---+
>>>>>>> + *    clk  |   +-------+   |                      +-----+
>>>>>>> + *         +---------------+
>>>>>>> + */
>>>>>>> +static int phy_aux_clk_register(struct qmp_pcie *qmp, struct device_node *np)
>>>>>>> +{
>>>>>>> +       struct clk_fixed_rate *fixed = &qmp->aux_clk_fixed;
>>>>>>> +       struct clk_init_data init = { };
>>>>>>> +       int ret;
>>>>>>> +
>>>>>>> +       ret = of_property_read_string_index(np, "clock-output-names", 1, &init.name);
>>>>>>> +       if (ret) {
>>>>>>> +               dev_err(qmp->dev, "%pOFn: No clock-output-names index 1\n", np);
>>>>>>> +               return ret;
>>>>>>> +       }
>>>>>>> +
>>>>>>> +       init.ops = &clk_fixed_rate_ops;
>>>>>>> +
>>>>>>> +       fixed->fixed_rate = qmp->cfg->aux_clock_rate;
>>>>>>> +       fixed->hw.init = &init;
>>>>>>> +
>>>>>>> +       return devm_clk_hw_register(qmp->dev, &fixed->hw);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static struct clk_hw *qmp_pcie_clk_hw_get(struct of_phandle_args *clkspec, void *data)
>>>>>>> +{
>>>>>>> +       struct qmp_pcie *qmp = data;
>>>>>>> +
>>>>>>> +       /* Support legacy bindings */
>>>>>>> +       if (!clkspec->args_count)
>>>>>>> +               return &qmp->pipe_clk_fixed.hw;
>>>>>>> +
>>>>>>> +       switch (clkspec->args[0]) {
>>>>>>> +       case QMP_PCIE_PIPE_CLK:
>>>>>>> +               return &qmp->pipe_clk_fixed.hw;
>>>>>>> +       case QMP_PCIE_PHY_AUX_CLK:
>>>>>>> +               return &qmp->aux_clk_fixed.hw;
>>>>>>> +       }
>>>>>>> +
>>>>>>> +       return ERR_PTR(-EINVAL);
>>>>>>> +}
>>>>>>
>>>>>> Can we use of_clk_hw_onecell_get() instead? I think it even should be
>>>>>> possible to use onecell for both cases, it will look at the first arg,
>>>>>> which will be 0 in case of #clock-cells equal to 0.
>>>>>
>>>>> Let me investigate if it's possible
>>>>
>>>> Ok, it would work but it would require building a clk_hw_onecell_data a runtime,
>>>> while we could simply provide this qmp_pcie_clk_hw_get() and avoid runtime 2 allocations.
>>>>
>>>> I'm not sure it's worth it.
>>>
>>> Single allocation (or even 0 allocations if you embed it into struct
>>> qmp_pcie) for the sake of using standard helpers.
>>
>> And I just recall I tried the same for Amlogic clocks, but the clk_hw_onecell_data hws
>> field is a flexible array member you can't set at runtime, if you try you'll get:
>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c:3753:38: error: invalid use of flexible array member
>>    3753 |                 qmp->clk_hw_data.hws = qmp->clk_hws;
> 
> Yes, so it's either
> devm_kzalloc(dev, struct_size(data, hws, 2), GFP_KERNEL);
> or
> struct qmp_pcie {
> ...
>    struct {
>      struct clk_hw_onecell_data clk_data;
>      struct clk_hw clocks[2];
>    };
> };

I won't go down this path because of that mess and using of_clk_hw_onecell_get() would need
to still add a custom getter before calling of_clk_hw_onecell_get() to handle the
#clock-cells=0 legacy case.

Neil

> 
>>
>> Neil
>>
>>>
>>>
>>>
>>
> 
> 

Re: [PATCH 3/7] phy: qcom: qmp-pcie: register second optional PHY AUX clock

[Dmitry Baryshkov]

On Tue, 19 Mar 2024 at 18:46, <neil.armstrong@linaro.org> wrote:
>
> On 19/03/2024 17:05, Dmitry Baryshkov wrote:
> > On Tue, 19 Mar 2024 at 17:15, <neil.armstrong@linaro.org> wrote:
> >>
> >> On 19/03/2024 15:46, Dmitry Baryshkov wrote:
> >>> On Tue, 19 Mar 2024 at 16:35, Neil Armstrong <neil.armstrong@linaro.org> wrote:
> >>>>
> >>>> On 19/03/2024 11:59, Neil Armstrong wrote:
> >>>>> On 19/03/2024 11:55, Dmitry Baryshkov wrote:
> >>>>>> On Tue, 19 Mar 2024 at 12:45, Neil Armstrong <neil.armstrong@linaro.org> wrote:
> >>>>>>>
> >>>>>>> The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock,
> >>>>>>> add the code to register it for PHYs configs that sets a aux_clock_rate.
> >>>>>>>
> >>>>>>> In order to get the right clock, add qmp_pcie_clk_hw_get() which uses
> >>>>>>> the newly introduced QMP_PCIE_PIPE_CLK & QMP_PCIE_PHY_AUX_CLK clock
> >>>>>>> IDs and also supports the legacy bindings by returning the PIPE clock.
> >>>>>>>
> >>>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> >>>>>>> ---
> >>>>>>>     drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 70 ++++++++++++++++++++++++++++++++
> >>>>>>>     1 file changed, 70 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> >>>>>>> index 079b3e306489..2d05226ae200 100644
> >>>>>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> >>>>>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> >>>>>>> @@ -22,6 +22,8 @@
> >>>>>>>     #include <linux/reset.h>
> >>>>>>>     #include <linux/slab.h>
> >>>>>>>
> >>>>>>> +#include <dt-bindings/phy/phy-qcom-qmp.h>
> >>>>>>> +
> >>>>>>>     #include "phy-qcom-qmp-common.h"
> >>>>>>>
> >>>>>>>     #include "phy-qcom-qmp.h"
> >>>>>>> @@ -2389,6 +2391,9 @@ struct qmp_phy_cfg {
> >>>>>>>
> >>>>>>>            /* QMP PHY pipe clock interface rate */
> >>>>>>>            unsigned long pipe_clock_rate;
> >>>>>>> +
> >>>>>>> +       /* QMP PHY AUX clock interface rate */
> >>>>>>> +       unsigned long aux_clock_rate;
> >>>>>>>     };
> >>>>>>>
> >>>>>>>     struct qmp_pcie {
> >>>>>>> @@ -2420,6 +2425,7 @@ struct qmp_pcie {
> >>>>>>>            int mode;
> >>>>>>>
> >>>>>>>            struct clk_fixed_rate pipe_clk_fixed;
> >>>>>>> +       struct clk_fixed_rate aux_clk_fixed;
> >>>>>>>     };
> >>>>>>>
> >>>>>>>     static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val)
> >>>>>>> @@ -3681,6 +3687,62 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np)
> >>>>>>>            return devm_clk_hw_register(qmp->dev, &fixed->hw);
> >>>>>>>     }
> >>>>>>>
> >>>>>>> +/*
> >>>>>>> + * Register a fixed rate PHY aux clock.
> >>>>>>> + *
> >>>>>>> + * The <s>_phy_aux_clksrc generated by PHY goes to the GCC that gate
> >>>>>>> + * controls it. The <s>_phy_aux_clk coming out of the GCC is requested
> >>>>>>> + * by the PHY driver for its operations.
> >>>>>>> + * We register the <s>_phy_aux_clksrc here. The gcc driver takes care
> >>>>>>> + * of assigning this <s>_phy_aux_clksrc as parent to <s>_phy_aux_clk.
> >>>>>>> + * Below picture shows this relationship.
> >>>>>>> + *
> >>>>>>> + *         +---------------+
> >>>>>>> + *         |   PHY block   |<<---------------------------------------------+
> >>>>>>> + *         |               |                                               |
> >>>>>>> + *         |   +-------+   |                      +-----+                  |
> >>>>>>> + *   I/P---^-->|  PLL  |---^--->phy_aux_clksrc--->| GCC |--->phy_aux_clk---+
> >>>>>>> + *    clk  |   +-------+   |                      +-----+
> >>>>>>> + *         +---------------+
> >>>>>>> + */
> >>>>>>> +static int phy_aux_clk_register(struct qmp_pcie *qmp, struct device_node *np)
> >>>>>>> +{
> >>>>>>> +       struct clk_fixed_rate *fixed = &qmp->aux_clk_fixed;
> >>>>>>> +       struct clk_init_data init = { };
> >>>>>>> +       int ret;
> >>>>>>> +
> >>>>>>> +       ret = of_property_read_string_index(np, "clock-output-names", 1, &init.name);
> >>>>>>> +       if (ret) {
> >>>>>>> +               dev_err(qmp->dev, "%pOFn: No clock-output-names index 1\n", np);
> >>>>>>> +               return ret;
> >>>>>>> +       }
> >>>>>>> +
> >>>>>>> +       init.ops = &clk_fixed_rate_ops;
> >>>>>>> +
> >>>>>>> +       fixed->fixed_rate = qmp->cfg->aux_clock_rate;
> >>>>>>> +       fixed->hw.init = &init;
> >>>>>>> +
> >>>>>>> +       return devm_clk_hw_register(qmp->dev, &fixed->hw);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static struct clk_hw *qmp_pcie_clk_hw_get(struct of_phandle_args *clkspec, void *data)
> >>>>>>> +{
> >>>>>>> +       struct qmp_pcie *qmp = data;
> >>>>>>> +
> >>>>>>> +       /* Support legacy bindings */
> >>>>>>> +       if (!clkspec->args_count)
> >>>>>>> +               return &qmp->pipe_clk_fixed.hw;
> >>>>>>> +
> >>>>>>> +       switch (clkspec->args[0]) {
> >>>>>>> +       case QMP_PCIE_PIPE_CLK:
> >>>>>>> +               return &qmp->pipe_clk_fixed.hw;
> >>>>>>> +       case QMP_PCIE_PHY_AUX_CLK:
> >>>>>>> +               return &qmp->aux_clk_fixed.hw;
> >>>>>>> +       }
> >>>>>>> +
> >>>>>>> +       return ERR_PTR(-EINVAL);
> >>>>>>> +}
> >>>>>>
> >>>>>> Can we use of_clk_hw_onecell_get() instead? I think it even should be
> >>>>>> possible to use onecell for both cases, it will look at the first arg,
> >>>>>> which will be 0 in case of #clock-cells equal to 0.
> >>>>>
> >>>>> Let me investigate if it's possible
> >>>>
> >>>> Ok, it would work but it would require building a clk_hw_onecell_data a runtime,
> >>>> while we could simply provide this qmp_pcie_clk_hw_get() and avoid runtime 2 allocations.
> >>>>
> >>>> I'm not sure it's worth it.
> >>>
> >>> Single allocation (or even 0 allocations if you embed it into struct
> >>> qmp_pcie) for the sake of using standard helpers.
> >>
> >> And I just recall I tried the same for Amlogic clocks, but the clk_hw_onecell_data hws
> >> field is a flexible array member you can't set at runtime, if you try you'll get:
> >> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c:3753:38: error: invalid use of flexible array member
> >>    3753 |                 qmp->clk_hw_data.hws = qmp->clk_hws;
> >
> > Yes, so it's either
> > devm_kzalloc(dev, struct_size(data, hws, 2), GFP_KERNEL);
> > or
> > struct qmp_pcie {
> > ...
> >    struct {
> >      struct clk_hw_onecell_data clk_data;
> >      struct clk_hw clocks[2];
> >    };
> > };
>
> I won't go down this path because of that mess and using of_clk_hw_onecell_get() would need
> to still add a custom getter before calling of_clk_hw_onecell_get() to handle the
> #clock-cells=0 legacy case.

No need for custom getters, if you select the getter during DT probe.
But yes, choose whatever seems better.

>
> Neil
>
> >
> >>
> >> Neil
> >>
> >>>
> >>>
> >>>
> >>
> >
> >
>


-- 
With best wishes
Dmitry

[PATCH 4/7] phy: qcom: qmp-pcie: register PHY AUX clock for SM8[456]50 4x2 PCIe PHY

[Neil Armstrong]

The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock,
enable this second clock by setting the proper 20MHz hardware rate in
the Gen4x2 SM8[456]50 aux_clock_rate config fields.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
index 2d05226ae200..cea5655ddc21 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
@@ -3141,6 +3141,9 @@ static const struct qmp_phy_cfg sm8450_qmp_gen4x2_pciephy_cfg = {
 
 	.pwrdn_ctrl		= SW_PWRDN | REFCLK_DRV_DSBL,
 	.phy_status		= PHYSTATUS_4_20,
+
+	/* 20MHz PHY AUX Clock */
+	.aux_clock_rate		= 20000000,
 };
 
 static const struct qmp_phy_cfg sm8550_qmp_gen3x2_pciephy_cfg = {
@@ -3198,6 +3201,9 @@ static const struct qmp_phy_cfg sm8550_qmp_gen4x2_pciephy_cfg = {
 	.pwrdn_ctrl		= SW_PWRDN | REFCLK_DRV_DSBL,
 	.phy_status		= PHYSTATUS_4_20,
 	.has_nocsr_reset	= true,
+
+	/* 20MHz PHY AUX Clock */
+	.aux_clock_rate		= 20000000,
 };
 
 static const struct qmp_phy_cfg sm8650_qmp_gen4x2_pciephy_cfg = {
@@ -3228,6 +3234,9 @@ static const struct qmp_phy_cfg sm8650_qmp_gen4x2_pciephy_cfg = {
 	.pwrdn_ctrl		= SW_PWRDN | REFCLK_DRV_DSBL,
 	.phy_status		= PHYSTATUS_4_20,
 	.has_nocsr_reset	= true,
+
+	/* 20MHz PHY AUX Clock */
+	.aux_clock_rate		= 20000000,
 };
 
 static const struct qmp_phy_cfg sa8775p_qmp_gen4x2_pciephy_cfg = {

-- 
2.34.1

Re: [PATCH 4/7] phy: qcom: qmp-pcie: register PHY AUX clock for SM8[456]50 4x2 PCIe PHY

[Dmitry Baryshkov]

On Tue, 19 Mar 2024 at 12:46, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>
> The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock,
> enable this second clock by setting the proper 20MHz hardware rate in
> the Gen4x2 SM8[456]50 aux_clock_rate config fields.
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 9 +++++++++
>  1 file changed, 9 insertions(+)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

[PATCH 5/7] arm64: dts: qcom: sm8450: remove pcie-1-phy-aux-clk and add pcie1_phy pcie1_phy_aux_clk

[Neil Armstrong]

Remove the dummy pcie-1-phy-aux-clk clock and replace with the pcie1_phy
provided QMP_PCIE_PHY_AUX_CLK.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8450.dtsi | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index b86be34a912b..32361af98936 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -754,8 +754,8 @@ gcc: clock-controller@100000 {
 			clocks = <&rpmhcc RPMH_CXO_CLK>,
 				 <&sleep_clk>,
 				 <&pcie0_phy>,
-				 <&pcie1_phy>,
-				 <0>,
+				 <&pcie1_phy QMP_PCIE_PIPE_CLK>,
+				 <&pcie1_phy QMP_PCIE_PHY_AUX_CLK>,
 				 <&ufs_mem_phy 0>,
 				 <&ufs_mem_phy 1>,
 				 <&ufs_mem_phy 2>,
@@ -1988,8 +1988,8 @@ pcie1_phy: phy@1c0e000 {
 				      "rchng",
 				      "pipe";
 
-			clock-output-names = "pcie_1_pipe_clk";
-			#clock-cells = <0>;
+			clock-output-names = "pcie_1_pipe_clk", "pcie_1_phy_aux_clk";
+			#clock-cells = <1>;
 
 			#phy-cells = <0>;
 

-- 
2.34.1

Re: [PATCH 5/7] arm64: dts: qcom: sm8450: remove pcie-1-phy-aux-clk and add pcie1_phy pcie1_phy_aux_clk

[Dmitry Baryshkov]

On Tue, 19 Mar 2024 at 12:46, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>
> Remove the dummy pcie-1-phy-aux-clk clock and replace with the pcie1_phy
> provided QMP_PCIE_PHY_AUX_CLK.
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sm8450.dtsi | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

[PATCH 6/7] arm64: dts: qcom: sm8550: remove pcie-1-phy-aux-clk and add pcie1_phy pcie1_phy_aux_clk

[Neil Armstrong]

Remove the dummy pcie-1-phy-aux-clk clock and replace with the pcie1_phy
provided QMP_PCIE_PHY_AUX_CLK.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8550-hdk.dts |  4 ----
 arch/arm64/boot/dts/qcom/sm8550-mtp.dts |  4 ----
 arch/arm64/boot/dts/qcom/sm8550-qrd.dts |  8 --------
 arch/arm64/boot/dts/qcom/sm8550.dtsi    | 13 ++++---------
 4 files changed, 4 insertions(+), 25 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8550-hdk.dts b/arch/arm64/boot/dts/qcom/sm8550-hdk.dts
index 12d60a0ee095..ccff744dcd14 100644
--- a/arch/arm64/boot/dts/qcom/sm8550-hdk.dts
+++ b/arch/arm64/boot/dts/qcom/sm8550-hdk.dts
@@ -979,10 +979,6 @@ &pcie1_phy {
 	status = "okay";
 };
 
-&pcie_1_phy_aux_clk {
-	clock-frequency = <1000>;
-};
-
 &pm8550_gpios {
 	sdc2_card_det_n: sdc2-card-det-state {
 		pins = "gpio12";
diff --git a/arch/arm64/boot/dts/qcom/sm8550-mtp.dts b/arch/arm64/boot/dts/qcom/sm8550-mtp.dts
index 3d4ad5aac70f..1fa7c4492057 100644
--- a/arch/arm64/boot/dts/qcom/sm8550-mtp.dts
+++ b/arch/arm64/boot/dts/qcom/sm8550-mtp.dts
@@ -739,10 +739,6 @@ &mdss_dp0_out {
 	remote-endpoint = <&usb_dp_qmpphy_dp_in>;
 };
 
-&pcie_1_phy_aux_clk {
-	clock-frequency = <1000>;
-};
-
 &pcie0 {
 	wake-gpios = <&tlmm 96 GPIO_ACTIVE_HIGH>;
 	perst-gpios = <&tlmm 94 GPIO_ACTIVE_LOW>;
diff --git a/arch/arm64/boot/dts/qcom/sm8550-qrd.dts b/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
index 92f015017418..da3cfa697969 100644
--- a/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
+++ b/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
@@ -810,10 +810,6 @@ &mdss_dp0_out {
 	remote-endpoint = <&usb_dp_qmpphy_dp_in>;
 };
 
-&pcie_1_phy_aux_clk {
-	status = "disabled";
-};
-
 &pcie0 {
 	wake-gpios = <&tlmm 96 GPIO_ACTIVE_HIGH>;
 	perst-gpios = <&tlmm 94 GPIO_ACTIVE_LOW>;
@@ -907,10 +903,6 @@ &pon_resin {
 	status = "okay";
 };
 
-&pcie_1_phy_aux_clk {
-	clock-frequency = <1000>;
-};
-
 &qupv3_id_0 {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
index 3904348075f6..c74455dfd354 100644
--- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
@@ -58,11 +58,6 @@ bi_tcxo_ao_div2: bi-tcxo-ao-div2-clk {
 			clock-mult = <1>;
 			clock-div = <2>;
 		};
-
-		pcie_1_phy_aux_clk: pcie-1-phy-aux-clk {
-			compatible = "fixed-clock";
-			#clock-cells = <0>;
-		};
 	};
 
 	cpus {
@@ -776,8 +771,8 @@ gcc: clock-controller@100000 {
 			#power-domain-cells = <1>;
 			clocks = <&bi_tcxo_div2>, <&sleep_clk>,
 				 <&pcie0_phy>,
-				 <&pcie1_phy>,
-				 <&pcie_1_phy_aux_clk>,
+				 <&pcie1_phy QMP_PCIE_PIPE_CLK>,
+				 <&pcie1_phy QMP_PCIE_PHY_AUX_CLK>,
 				 <&ufs_mem_phy 0>,
 				 <&ufs_mem_phy 1>,
 				 <&ufs_mem_phy 2>,
@@ -1906,8 +1901,8 @@ pcie1_phy: phy@1c0e000 {
 
 			power-domains = <&gcc PCIE_1_PHY_GDSC>;
 
-			#clock-cells = <0>;
-			clock-output-names = "pcie1_pipe_clk";
+			#clock-cells = <1>;
+			clock-output-names = "pcie1_pipe_clk", "pcie1_phy_aux_clk";
 
 			#phy-cells = <0>;
 

-- 
2.34.1

Re: [PATCH 6/7] arm64: dts: qcom: sm8550: remove pcie-1-phy-aux-clk and add pcie1_phy pcie1_phy_aux_clk

[Dmitry Baryshkov]

On Tue, 19 Mar 2024 at 12:46, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>
> Remove the dummy pcie-1-phy-aux-clk clock and replace with the pcie1_phy
> provided QMP_PCIE_PHY_AUX_CLK.
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sm8550-hdk.dts |  4 ----
>  arch/arm64/boot/dts/qcom/sm8550-mtp.dts |  4 ----
>  arch/arm64/boot/dts/qcom/sm8550-qrd.dts |  8 --------
>  arch/arm64/boot/dts/qcom/sm8550.dtsi    | 13 ++++---------
>  4 files changed, 4 insertions(+), 25 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


-- 
With best wishes
Dmitry

[PATCH 7/7] arm64: dts: qcom: sm8650: remove pcie-1-phy-aux-clk and add pcie1_phy pcie1_phy_aux_clk

[Neil Armstrong]

Remove the dummy pcie-1-phy-aux-clk clock and replace with the pcie1_phy
provided QMP_PCIE_PHY_AUX_CLK.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8650-mtp.dts |  4 ----
 arch/arm64/boot/dts/qcom/sm8650-qrd.dts |  4 ----
 arch/arm64/boot/dts/qcom/sm8650.dtsi    | 13 ++++---------
 3 files changed, 4 insertions(+), 17 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8650-mtp.dts b/arch/arm64/boot/dts/qcom/sm8650-mtp.dts
index 4450273f9667..95d0c2baef2b 100644
--- a/arch/arm64/boot/dts/qcom/sm8650-mtp.dts
+++ b/arch/arm64/boot/dts/qcom/sm8650-mtp.dts
@@ -645,10 +645,6 @@ &mdss_mdp {
 	status = "okay";
 };
 
-&pcie_1_phy_aux_clk {
-	clock-frequency = <1000>;
-};
-
 &pcie0 {
 	wake-gpios = <&tlmm 96 GPIO_ACTIVE_HIGH>;
 	perst-gpios = <&tlmm 94 GPIO_ACTIVE_LOW>;
diff --git a/arch/arm64/boot/dts/qcom/sm8650-qrd.dts b/arch/arm64/boot/dts/qcom/sm8650-qrd.dts
index b07cac2e5bc8..c6e907e40af1 100644
--- a/arch/arm64/boot/dts/qcom/sm8650-qrd.dts
+++ b/arch/arm64/boot/dts/qcom/sm8650-qrd.dts
@@ -831,10 +831,6 @@ &mdss_mdp {
 	status = "okay";
 };
 
-&pcie_1_phy_aux_clk {
-	clock-frequency = <1000>;
-};
-
 &pcie0 {
 	wake-gpios = <&tlmm 96 GPIO_ACTIVE_HIGH>;
 	perst-gpios = <&tlmm 94 GPIO_ACTIVE_LOW>;
diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
index ba72d8f38420..6e4362bbcc3a 100644
--- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
@@ -60,11 +60,6 @@ bi_tcxo_ao_div2: bi-tcxo-ao-div2-clk {
 			clock-mult = <1>;
 			clock-div = <2>;
 		};
-
-		pcie_1_phy_aux_clk: pcie-1-phy-aux-clk {
-			compatible = "fixed-clock";
-			#clock-cells = <0>;
-		};
 	};
 
 	cpus {
@@ -758,8 +753,8 @@ gcc: clock-controller@100000 {
 				 <&bi_tcxo_ao_div2>,
 				 <&sleep_clk>,
 				 <&pcie0_phy>,
-				 <&pcie1_phy>,
-				 <&pcie_1_phy_aux_clk>,
+				 <&pcie1_phy QMP_PCIE_PIPE_CLK>,
+				 <&pcie1_phy QMP_PCIE_PHY_AUX_CLK>,
 				 <&ufs_mem_phy 0>,
 				 <&ufs_mem_phy 1>,
 				 <&ufs_mem_phy 2>,
@@ -2449,8 +2444,8 @@ pcie1_phy: phy@1c0e000 {
 
 			power-domains = <&gcc PCIE_1_PHY_GDSC>;
 
-			#clock-cells = <0>;
-			clock-output-names = "pcie1_pipe_clk";
+			#clock-cells = <1>;
+			clock-output-names = "pcie1_pipe_clk", "pcie1_phy_aux_clk";
 
 			#phy-cells = <0>;
 

-- 
2.34.1

Re: [PATCH 7/7] arm64: dts: qcom: sm8650: remove pcie-1-phy-aux-clk and add pcie1_phy pcie1_phy_aux_clk

[Dmitry Baryshkov]

On Tue, 19 Mar 2024 at 12:46, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>
> Remove the dummy pcie-1-phy-aux-clk clock and replace with the pcie1_phy
> provided QMP_PCIE_PHY_AUX_CLK.
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sm8650-mtp.dts |  4 ----
>  arch/arm64/boot/dts/qcom/sm8650-qrd.dts |  4 ----
>  arch/arm64/boot/dts/qcom/sm8650.dtsi    | 13 ++++---------
>  3 files changed, 4 insertions(+), 17 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH 0/7] arm64: qcom-sm8[456]50: properly describe the PCIe Gen4x2 PHY AUX clock

[Rob Herring]

On Tue, 19 Mar 2024 11:44:26 +0100, Neil Armstrong wrote:
> The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock named
> "PHY_AUX_CLK" which is an input of the Global Clock Controller (GCC) which
> is muxed & gated then returned to the PHY as an input.
> 
> Document the clock IDs to select the PIPE clock or the AUX clock,
> also enforce a second clock-output-names and a #clock-cells value of 1
> for the PCIe Gen4x2 PHY found in the SM8[456]50 SoCs.
> 
> The PHY driver needs a light refactoring to support a second clock,
> and finally the DT is changed to connect the PHY second clock to the
> corresponding GCC input then drop the dummy fixed rate clock.
> 
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
> Neil Armstrong (7):
>       dt-bindings: phy: qcom,sc8280xp-qmp-pcie-phy: document PHY AUX clock on SM8[456]50 SoCs
>       phy: qcom: qmp-pcie: refactor clock register code
>       phy: qcom: qmp-pcie: register second optional PHY AUX clock
>       phy: qcom: qmp-pcie: register PHY AUX clock for SM8[456]50 4x2 PCIe PHY
>       arm64: dts: qcom: sm8450: remove pcie-1-phy-aux-clk and add pcie1_phy pcie1_phy_aux_clk
>       arm64: dts: qcom: sm8550: remove pcie-1-phy-aux-clk and add pcie1_phy pcie1_phy_aux_clk
>       arm64: dts: qcom: sm8650: remove pcie-1-phy-aux-clk and add pcie1_phy pcie1_phy_aux_clk
> 
>  .../bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml   |  27 +++++-
>  arch/arm64/boot/dts/qcom/sm8450.dtsi               |   8 +-
>  arch/arm64/boot/dts/qcom/sm8550-hdk.dts            |   4 -
>  arch/arm64/boot/dts/qcom/sm8550-mtp.dts            |   4 -
>  arch/arm64/boot/dts/qcom/sm8550-qrd.dts            |   8 --
>  arch/arm64/boot/dts/qcom/sm8550.dtsi               |  13 +--
>  arch/arm64/boot/dts/qcom/sm8650-mtp.dts            |   4 -
>  arch/arm64/boot/dts/qcom/sm8650-qrd.dts            |   4 -
>  arch/arm64/boot/dts/qcom/sm8650.dtsi               |  13 +--
>  drivers/phy/qualcomm/phy-qcom-qmp-pcie.c           | 104 ++++++++++++++++++---
>  include/dt-bindings/phy/phy-qcom-qmp.h             |   4 +
>  11 files changed, 129 insertions(+), 64 deletions(-)
> ---
> base-commit: 2e93f143ca010a5013528e1cfdc895f024fe8c21
> change-id: 20240319-topic-sm8x50-upstream-pcie-1-phy-aux-clk-4b35169707dd
> 
> Best regards,
> --
> Neil Armstrong <neil.armstrong@linaro.org>
> 
> 
> 


My bot found new DTB warnings on the .dts files added or changed in this
series.

Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not. No need to reply
unless the platform maintainer has comments.

If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:

  pip3 install dtschema --upgrade


New warnings running 'make CHECK_DTBS=y qcom/sm8550-hdk.dtb qcom/sm8550-mtp.dtb qcom/sm8550-qrd.dtb qcom/sm8650-mtp.dtb qcom/sm8650-qrd.dtb' for 20240319-topic-sm8x50-upstream-pcie-1-phy-aux-clk-v1-0-926d7a4ccd80@linaro.org:

arch/arm64/boot/dts/qcom/sm8550-qrd.dtb: clock-controller@100000: clocks: [[41], [42], [43], [44, 0], [45, 0], [45, 1], [45, 2], [46, 0]] is too short
	from schema $id: http://devicetree.org/schemas/clock/qcom,sm8550-gcc.yaml#
arch/arm64/boot/dts/qcom/sm8550-qrd.dtb: clock-controller@100000: Unevaluated properties are not allowed ('clocks' was unexpected)
	from schema $id: http://devicetree.org/schemas/clock/qcom,sm8550-gcc.yaml#