[PATCH] arm64: dts: qcom: sm8350: Correct UFS symbol clocks

[PATCH] arm64: dts: qcom: sm8350: Correct UFS symbol clocks

[Bjorn Andersson]

The introduction of '9a61f813fcc8 ("clk: qcom: regmap-mux: fix parent
clock lookup")' broke UFS support on SM8350.

The cause for this is that the symbol clocks have a specified rate in
the "freq-table-hz" table in the UFS node, which causes the UFS code to
request a rate change, for which the "bi_tcxo" happens to provide the
closest rate.  Prior to the change in regmap-mux it was determined
(incorrectly) that no change was needed and everything worked. Instead
mimic the configuration found in other platforms, by omitting the rate
for the symbol clocks as well to avoid the rate change.

While at it also fill in the dummy symbol clocks that was dropped from
the GCC driver as it was upstreamed.

Fixes: 59c7cf814783 ("arm64: dts: qcom: sm8350: Add UFS nodes")
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8350.dtsi | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8350.dtsi b/arch/arm64/boot/dts/qcom/sm8350.dtsi
index bc176c252bca..ceb064a83038 100644
--- a/arch/arm64/boot/dts/qcom/sm8350.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8350.dtsi
@@ -38,6 +38,24 @@ sleep_clk: sleep-clk {
 			clock-frequency = <32000>;
 			#clock-cells = <0>;
 		};
+
+		ufs_phy_rx_symbol_0_clk: ufs-phy-rx-symbol-0 {
+			compatible = "fixed-clock";
+			clock-frequency = <1000>;
+			#clock-cells = <0>;
+		};
+
+		ufs_phy_rx_symbol_1_clk: ufs-phy-rx-symbol-1 {
+			compatible = "fixed-clock";
+			clock-frequency = <1000>;
+			#clock-cells = <0>;
+		};
+
+		ufs_phy_tx_symbol_0_clk: ufs-phy-tx-symbol-0 {
+			compatible = "fixed-clock";
+			clock-frequency = <1000>;
+			#clock-cells = <0>;
+		};
 	};
 
 	cpus {
@@ -606,9 +624,9 @@ gcc: clock-controller@100000 {
 				 <0>,
 				 <0>,
 				 <0>,
-				 <0>,
-				 <0>,
-				 <0>,
+				 <&ufs_phy_rx_symbol_0_clk>,
+				 <&ufs_phy_rx_symbol_1_clk>,
+				 <&ufs_phy_tx_symbol_0_clk>,
 				 <0>,
 				 <0>;
 		};
@@ -2079,8 +2097,8 @@ ufs_mem_hc: ufshc@1d84000 {
 				<75000000 300000000>,
 				<0 0>,
 				<0 0>,
-				<75000000 300000000>,
-				<75000000 300000000>;
+				<0 0>,
+				<0 0>;
 			status = "disabled";
 		};
 
-- 
2.33.1

Re: [PATCH] arm64: dts: qcom: sm8350: Correct UFS symbol clocks

[Vinod Koul]

On 21-12-21, 16:26, Bjorn Andersson wrote:
> The introduction of '9a61f813fcc8 ("clk: qcom: regmap-mux: fix parent
> clock lookup")' broke UFS support on SM8350.
> 
> The cause for this is that the symbol clocks have a specified rate in
> the "freq-table-hz" table in the UFS node, which causes the UFS code to
> request a rate change, for which the "bi_tcxo" happens to provide the
> closest rate.  Prior to the change in regmap-mux it was determined
> (incorrectly) that no change was needed and everything worked. Instead
> mimic the configuration found in other platforms, by omitting the rate
> for the symbol clocks as well to avoid the rate change.
> 
> While at it also fill in the dummy symbol clocks that was dropped from
> the GCC driver as it was upstreamed.
> 
> Fixes: 59c7cf814783 ("arm64: dts: qcom: sm8350: Add UFS nodes")
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sm8350.dtsi | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8350.dtsi b/arch/arm64/boot/dts/qcom/sm8350.dtsi
> index bc176c252bca..ceb064a83038 100644
> --- a/arch/arm64/boot/dts/qcom/sm8350.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8350.dtsi
> @@ -38,6 +38,24 @@ sleep_clk: sleep-clk {
>  			clock-frequency = <32000>;
>  			#clock-cells = <0>;
>  		};
> +
> +		ufs_phy_rx_symbol_0_clk: ufs-phy-rx-symbol-0 {
> +			compatible = "fixed-clock";
> +			clock-frequency = <1000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		ufs_phy_rx_symbol_1_clk: ufs-phy-rx-symbol-1 {
> +			compatible = "fixed-clock";
> +			clock-frequency = <1000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		ufs_phy_tx_symbol_0_clk: ufs-phy-tx-symbol-0 {
> +			compatible = "fixed-clock";
> +			clock-frequency = <1000>;
> +			#clock-cells = <0>;
> +		};
>  	};
>  
>  	cpus {
> @@ -606,9 +624,9 @@ gcc: clock-controller@100000 {
>  				 <0>,
>  				 <0>,
>  				 <0>,
> -				 <0>,
> -				 <0>,
> -				 <0>,
> +				 <&ufs_phy_rx_symbol_0_clk>,
> +				 <&ufs_phy_rx_symbol_1_clk>,
> +				 <&ufs_phy_tx_symbol_0_clk>,
>  				 <0>,
>  				 <0>;
>  		};
> @@ -2079,8 +2097,8 @@ ufs_mem_hc: ufshc@1d84000 {
>  				<75000000 300000000>,
>  				<0 0>,
>  				<0 0>,
> -				<75000000 300000000>,
> -				<75000000 300000000>;
> +				<0 0>,
> +				<0 0>;

should the rate be zero here?

-- 
~Vinod

Re: [PATCH] arm64: dts: qcom: sm8350: Correct UFS symbol clocks

[Bjorn Andersson]

On Wed 22 Dec 02:37 PST 2021, Vinod Koul wrote:

> On 21-12-21, 16:26, Bjorn Andersson wrote:
> > The introduction of '9a61f813fcc8 ("clk: qcom: regmap-mux: fix parent
> > clock lookup")' broke UFS support on SM8350.
> > 
> > The cause for this is that the symbol clocks have a specified rate in
> > the "freq-table-hz" table in the UFS node, which causes the UFS code to
> > request a rate change, for which the "bi_tcxo" happens to provide the
> > closest rate.  Prior to the change in regmap-mux it was determined
> > (incorrectly) that no change was needed and everything worked. Instead
> > mimic the configuration found in other platforms, by omitting the rate
> > for the symbol clocks as well to avoid the rate change.
> > 
> > While at it also fill in the dummy symbol clocks that was dropped from
> > the GCC driver as it was upstreamed.
> > 
> > Fixes: 59c7cf814783 ("arm64: dts: qcom: sm8350: Add UFS nodes")
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sm8350.dtsi | 28 +++++++++++++++++++++++-----
> >  1 file changed, 23 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sm8350.dtsi b/arch/arm64/boot/dts/qcom/sm8350.dtsi
> > index bc176c252bca..ceb064a83038 100644
> > --- a/arch/arm64/boot/dts/qcom/sm8350.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sm8350.dtsi
> > @@ -38,6 +38,24 @@ sleep_clk: sleep-clk {
> >  			clock-frequency = <32000>;
> >  			#clock-cells = <0>;
> >  		};
> > +
> > +		ufs_phy_rx_symbol_0_clk: ufs-phy-rx-symbol-0 {
> > +			compatible = "fixed-clock";
> > +			clock-frequency = <1000>;
> > +			#clock-cells = <0>;
> > +		};
> > +
> > +		ufs_phy_rx_symbol_1_clk: ufs-phy-rx-symbol-1 {
> > +			compatible = "fixed-clock";
> > +			clock-frequency = <1000>;
> > +			#clock-cells = <0>;
> > +		};
> > +
> > +		ufs_phy_tx_symbol_0_clk: ufs-phy-tx-symbol-0 {
> > +			compatible = "fixed-clock";
> > +			clock-frequency = <1000>;
> > +			#clock-cells = <0>;
> > +		};
> >  	};
> >  
> >  	cpus {
> > @@ -606,9 +624,9 @@ gcc: clock-controller@100000 {
> >  				 <0>,
> >  				 <0>,
> >  				 <0>,
> > -				 <0>,
> > -				 <0>,
> > -				 <0>,
> > +				 <&ufs_phy_rx_symbol_0_clk>,
> > +				 <&ufs_phy_rx_symbol_1_clk>,
> > +				 <&ufs_phy_tx_symbol_0_clk>,
> >  				 <0>,
> >  				 <0>;
> >  		};
> > @@ -2079,8 +2097,8 @@ ufs_mem_hc: ufshc@1d84000 {
> >  				<75000000 300000000>,
> >  				<0 0>,
> >  				<0 0>,
> > -				<75000000 300000000>,
> > -				<75000000 300000000>;
> > +				<0 0>,
> > +				<0 0>;
> 
> should the rate be zero here?
> 

It seems that the numbers (75 and 300MHz) are the correct rates for the
symbol clocks.
It's however not clear to me where they are coming from and hence how we
would represent the change of rate in &ufs_phy_?x_symbol_?_clk. Let's
make sure to document this in the commit message...

Thanks,
Bjorn