2024-04-22 16:16:51

by Neil Armstrong

[permalink] [raw]
Subject: [PATCH v4 0/3] arm64: qcom-sm8[456]50: properly describe the PCIe Gen4x2 PHY AUX clock

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 <[email protected]>
---
Changes in v4:
- Fixed dtbs check error on sm8550-qrd.dtb after rebase on -next
- Link to v3: https://lore.kernel.org/r/20240422-topic-sm8x50-upstream-pcie-1-phy-aux-clk-v3-0-799475a27cce@linaro.org

Changes in v3:
- Rebased on linux-next, applies now cleanly
- Link to v2: https://lore.kernel.org/r/20240322-topic-sm8x50-upstream-pcie-1-phy-aux-clk-v2-0-3ec0a966d52f@linaro.org

Changes in v2:
- Collected review tags
- Switched back to of_clk_add_hw_provider/devm_add_action_or_reset to maintain compatibility
- Tried to use generic of_clk_hw_onecell_get() but it requires to much boilerplate code
and would still need a local qmp_pcie_clk_hw_get() to support the current #clock-cells=0
when exposing 2 clocks, so it's simpler to just return the clocks in qmp_pcie_clk_hw_get()
- Link to v1: https://lore.kernel.org/r/20240319-topic-sm8x50-upstream-pcie-1-phy-aux-clk-v1-0-926d7a4ccd80@linaro.org

---
Neil Armstrong (3):
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

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 | 19 -------------------
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 ++++---------
8 files changed, 12 insertions(+), 57 deletions(-)
---
base-commit: f529a6d274b3b8c75899e949649d231298f30a32
change-id: 20240319-topic-sm8x50-upstream-pcie-1-phy-aux-clk-4b35169707dd

Best regards,
--
Neil Armstrong <[email protected]>



2024-04-22 16:17:19

by Neil Armstrong

[permalink] [raw]
Subject: [PATCH v4 3/3] arm64: dts: qcom: sm8650: remove pcie-1-phy-aux-clk and add pcie1_phy pcie1_phy_aux_clk

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

Reviewed-by: Dmitry Baryshkov <[email protected]>
Signed-off-by: Neil Armstrong <[email protected]>
---
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 d04ceaa73c2b..ea092f532e5a 100644
--- a/arch/arm64/boot/dts/qcom/sm8650-mtp.dts
+++ b/arch/arm64/boot/dts/qcom/sm8650-mtp.dts
@@ -641,10 +641,6 @@ &mdss_dsi0_phy {
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 4e94f7fe4d2d..bd87aa3aa548 100644
--- a/arch/arm64/boot/dts/qcom/sm8650-qrd.dts
+++ b/arch/arm64/boot/dts/qcom/sm8650-qrd.dts
@@ -835,10 +835,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/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
index 8e0c1841f748..658ad2b41c5a 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>,
@@ -2467,8 +2462,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


2024-04-22 16:17:24

by Neil Armstrong

[permalink] [raw]
Subject: [PATCH v4 2/3] arm64: dts: qcom: sm8550: remove pcie-1-phy-aux-clk and add pcie1_phy pcie1_phy_aux_clk

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

Reviewed-by: Dmitry Baryshkov <[email protected]>
Signed-off-by: Neil Armstrong <[email protected]>
---
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 | 19 -------------------
arch/arm64/boot/dts/qcom/sm8550.dtsi | 13 ++++---------
4 files changed, 4 insertions(+), 36 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..39ba3e9969b7 100644
--- a/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
+++ b/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
@@ -720,17 +720,6 @@ &ipa {
status = "okay";
};

-&gcc {
- clocks = <&bi_tcxo_div2>, <&sleep_clk>,
- <&pcie0_phy>,
- <&pcie1_phy>,
- <0>,
- <&ufs_mem_phy 0>,
- <&ufs_mem_phy 1>,
- <&ufs_mem_phy 2>,
- <&usb_dp_qmpphy QMP_USB43DP_USB3_PIPE_CLK>;
-};
-
&gpi_dma1 {
status = "okay";
};
@@ -810,10 +799,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 +892,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 bc5aeb05ffc3..143994d1e6ca 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>,
@@ -1928,8 +1923,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


2024-04-22 16:19:14

by Neil Armstrong

[permalink] [raw]
Subject: [PATCH v4 1/3] arm64: dts: qcom: sm8450: remove pcie-1-phy-aux-clk and add pcie1_phy pcie1_phy_aux_clk

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

Reviewed-by: Dmitry Baryshkov <[email protected]>
Signed-off-by: Neil Armstrong <[email protected]>
---
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 616461fcbab9..71797f337d19 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>,
@@ -2000,8 +2000,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


2024-04-30 02:53:44

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH v4 1/3] arm64: dts: qcom: sm8450: remove pcie-1-phy-aux-clk and add pcie1_phy pcie1_phy_aux_clk

On Mon, Apr 22, 2024 at 06:16:18PM GMT, Neil Armstrong wrote:
> Remove the dummy pcie-1-phy-aux-clk clock and replace with the pcie1_phy
> provided QMP_PCIE_PHY_AUX_CLK.
>

This looks applicable to the other two patches, but I don't see the
pcie-1-phy-aux-clk being removed in this patch.

Furthermore, the cover letter does not make it into the git history,
which results in rather lacking documentation on the reasoning for the
change.

Can you please update the three commit messages?

Regards,
Bjorn

> Reviewed-by: Dmitry Baryshkov <[email protected]>
> Signed-off-by: Neil Armstrong <[email protected]>
> ---
> 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 616461fcbab9..71797f337d19 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>,
> @@ -2000,8 +2000,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
>