| Message ID | 20240213-msm8996-fix-ufs-v2-0-650758c26458@linaro.org |
|---|---|
| Headers | show |
| Series | scsi: ufs: qcom: fix UFSDHCD support on MSM8996 platform | expand |
On 13.02.2024 12:22, Dmitry Baryshkov wrote: > Follow the example of other platforms and specify core_clk frequencies > in the frequency table in addition to the core_clk_src frequencies. The > driver should be setting the leaf frequency instead of some interim > clock freq. > > Suggested-by: Nitin Rawat <quic_nitirawa@quicinc.com> > Fixes: 57fc67ef0d35 ("arm64: dts: qcom: msm8996: Add ufs related nodes") > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > arch/arm64/boot/dts/qcom/msm8996.dtsi | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi > index 80d83e01bb4d..401c6cce9fec 100644 > --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi > +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi > @@ -2072,7 +2072,7 @@ ufshc: ufshc@624000 { > <&gcc GCC_UFS_RX_SYMBOL_0_CLK>; > freq-table-hz = > <100000000 200000000>, > - <0 0>, > + <100000000 200000000>, That's bus_clk, no? Konrad
On Wed, 14 Feb 2024 at 23:24, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > On 13.02.2024 12:22, Dmitry Baryshkov wrote: > > Follow the example of other platforms and specify core_clk frequencies > > in the frequency table in addition to the core_clk_src frequencies. The > > driver should be setting the leaf frequency instead of some interim > > clock freq. > > > > Suggested-by: Nitin Rawat <quic_nitirawa@quicinc.com> > > Fixes: 57fc67ef0d35 ("arm64: dts: qcom: msm8996: Add ufs related nodes") > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > arch/arm64/boot/dts/qcom/msm8996.dtsi | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi > > index 80d83e01bb4d..401c6cce9fec 100644 > > --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi > > +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi > > @@ -2072,7 +2072,7 @@ ufshc: ufshc@624000 { > > <&gcc GCC_UFS_RX_SYMBOL_0_CLK>; > > freq-table-hz = > > <100000000 200000000>, > > - <0 0>, > > + <100000000 200000000>, > > That's bus_clk, no? No, it's a core_clk. The "core_clk_src" is removed in one of the next patches. > > Konrad >
On 13.02.2024 12:22, Dmitry Baryshkov wrote: > Since the commit b4e13e1ae95e ("scsi: ufs: qcom: Add multiple frequency > support for MAX_CORE_CLK_1US_CYCLES") the Qualcomm UFS driver uses > core_clk_unipro values from frequency table to calculate cycles_in_1us. > The DT file for MSM8996 passed 0 HZ frequencies there, resulting in > broken UFS support on that platform. Fix the corresponding clock values > in the frequency table. > > Suggested-by: Nitin Rawat <quic_nitirawa@quicinc.com> > Fixes: 57fc67ef0d35 ("arm64: dts: qcom: msm8996: Add ufs related nodes") > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- Okay, so this one apparently doesn't apply, seems like commit 68c4c20848d71b0e69c3403becb5dd23e89e5896 ("arm64: dts: qcom: msm8996: Define UFS UniPro clock limits") did the exact same Konrad
On 15.02.2024 09:19, Dmitry Baryshkov wrote: > On Wed, 14 Feb 2024 at 23:24, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: >> >> On 13.02.2024 12:22, Dmitry Baryshkov wrote: >>> Follow the example of other platforms and specify core_clk frequencies >>> in the frequency table in addition to the core_clk_src frequencies. The >>> driver should be setting the leaf frequency instead of some interim >>> clock freq. >>> >>> Suggested-by: Nitin Rawat <quic_nitirawa@quicinc.com> >>> Fixes: 57fc67ef0d35 ("arm64: dts: qcom: msm8996: Add ufs related nodes") >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> arch/arm64/boot/dts/qcom/msm8996.dtsi | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi >>> index 80d83e01bb4d..401c6cce9fec 100644 >>> --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi >>> +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi >>> @@ -2072,7 +2072,7 @@ ufshc: ufshc@624000 { >>> <&gcc GCC_UFS_RX_SYMBOL_0_CLK>; >>> freq-table-hz = >>> <100000000 200000000>, >>> - <0 0>, >>> + <100000000 200000000>, >> >> That's bus_clk, no? > > No, it's a core_clk. The "core_clk_src" is removed in one of the next patches. Just confirmed what you're saying is true, reading the raw diff was apparently not convincing enough for my brain.. Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> Konrad
On 13.02.2024 12:22, Dmitry Baryshkov wrote: > Instead of setting the frequency of the interim UFS_ICE_CORE_CLK_SRC > clokc, set the freency of the leaf GCC_UFS_ICE_CORE_CLK clock directly. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> Konrad
On 13.02.2024 12:22, Dmitry Baryshkov wrote: > There is no need to mention and/or to touch in any way the intermediate > (source) clocks. Drop them from MSM8996 UFSHCD schema, making it follow > the example lead by all other platforms. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> Konrad
On Tue, Feb 13, 2024 at 01:22:17PM +0200, Dmitry Baryshkov wrote: > The MSM8996 DT doesn't provide frequency limits for the core_clk_unipro > clock, which results in miscalculation of the cycles_in_1us value. > Provide the backwards-compatible default to support existing MSM8996 > DT files. > > Fixes: b4e13e1ae95e ("scsi: ufs: qcom: Add multiple frequency support for MAX_CORE_CLK_1US_CYCLES") > Cc: Nitin Rawat <quic_nitirawa@quicinc.com> > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> CC stable list? Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> - Mani > --- > drivers/ufs/host/ufs-qcom.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index 0aeaee1c564c..79f8cb377710 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -1210,8 +1210,10 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up) > > list_for_each_entry(clki, head, list) { > if (!IS_ERR_OR_NULL(clki->clk) && > - !strcmp(clki->name, "core_clk_unipro")) { > - if (is_scale_up) > + !strcmp(clki->name, "core_clk_unipro")) { > + if (!clki->max_freq) > + cycles_in_1us = 150; /* default for backwards compatibility */ > + else if (is_scale_up) > cycles_in_1us = ceil(clki->max_freq, (1000 * 1000)); > else > cycles_in_1us = ceil(clk_get_rate(clki->clk), (1000 * 1000)); > > -- > 2.39.2 >
On Tue, Feb 13, 2024 at 01:22:18PM +0200, Dmitry Baryshkov wrote: > Since the commit b4e13e1ae95e ("scsi: ufs: qcom: Add multiple frequency > support for MAX_CORE_CLK_1US_CYCLES") the Qualcomm UFS driver uses > core_clk_unipro values from frequency table to calculate cycles_in_1us. > The DT file for MSM8996 passed 0 HZ frequencies there, resulting in > broken UFS support on that platform. Fix the corresponding clock values > in the frequency table. > > Suggested-by: Nitin Rawat <quic_nitirawa@quicinc.com> > Fixes: 57fc67ef0d35 ("arm64: dts: qcom: msm8996: Add ufs related nodes") > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> - Mani > --- > arch/arm64/boot/dts/qcom/msm8996.dtsi | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi > index f6b6fdc12f44..80d83e01bb4d 100644 > --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi > +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi > @@ -2077,7 +2077,7 @@ ufshc: ufshc@624000 { > <0 0>, > <0 0>, > <150000000 300000000>, > - <0 0>, > + <75000000 150000000>, > <0 0>, > <0 0>, > <0 0>, > > -- > 2.39.2 >
On Tue, Feb 13, 2024 at 01:22:19PM +0200, Dmitry Baryshkov wrote: > Follow the example of other platforms and specify core_clk frequencies > in the frequency table in addition to the core_clk_src frequencies. The > driver should be setting the leaf frequency instead of some interim > clock freq. > > Suggested-by: Nitin Rawat <quic_nitirawa@quicinc.com> > Fixes: 57fc67ef0d35 ("arm64: dts: qcom: msm8996: Add ufs related nodes") > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> - Mani > --- > arch/arm64/boot/dts/qcom/msm8996.dtsi | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi > index 80d83e01bb4d..401c6cce9fec 100644 > --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi > +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi > @@ -2072,7 +2072,7 @@ ufshc: ufshc@624000 { > <&gcc GCC_UFS_RX_SYMBOL_0_CLK>; > freq-table-hz = > <100000000 200000000>, > - <0 0>, > + <100000000 200000000>, > <0 0>, > <0 0>, > <0 0>, > > -- > 2.39.2 >
On Tue, Feb 13, 2024 at 01:22:20PM +0200, Dmitry Baryshkov wrote: > Instead of setting the frequency of the interim UFS_ICE_CORE_CLK_SRC > clokc, set the freency of the leaf GCC_UFS_ICE_CORE_CLK clock directly. > "clock", "frequency" > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > arch/arm64/boot/dts/qcom/msm8996.dtsi | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi > index 401c6cce9fec..ce94e2af6bc5 100644 > --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi > +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi > @@ -2076,9 +2076,9 @@ ufshc: ufshc@624000 { > <0 0>, > <0 0>, > <0 0>, > - <150000000 300000000>, > - <75000000 150000000>, > <0 0>, > + <75000000 150000000>, > + <150000000 300000000>, Btw, I noticed that this platform is passing UFS_ICE_CORE_CLK_SRC as "core_clk_unipro_src" which looks wrong. - Mani
On Tue, Feb 13, 2024 at 01:22:22PM +0200, Dmitry Baryshkov wrote: > There is no need to mention and/or to touch in any way the intermediate > (source) clocks. Drop them from MSM8996 UFSHCD schema, making it follow > the example lead by all other platforms. > Okay, here you are dropping the "core_clk_unipro_src" anyway. So my earlier comment can be ignored. > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> - Mani > --- > arch/arm64/boot/dts/qcom/msm8996.dtsi | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi > index ce94e2af6bc5..f18d80a97bbf 100644 > --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi > +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi > @@ -2047,24 +2047,20 @@ ufshc: ufshc@624000 { > power-domains = <&gcc UFS_GDSC>; > > clock-names = > - "core_clk_src", > "core_clk", > "bus_clk", > "bus_aggr_clk", > "iface_clk", > - "core_clk_unipro_src", > "core_clk_unipro", > "core_clk_ice", > "ref_clk", > "tx_lane0_sync_clk", > "rx_lane0_sync_clk"; > clocks = > - <&gcc UFS_AXI_CLK_SRC>, > <&gcc GCC_UFS_AXI_CLK>, > <&gcc GCC_SYS_NOC_UFS_AXI_CLK>, > <&gcc GCC_AGGRE2_UFS_AXI_CLK>, > <&gcc GCC_UFS_AHB_CLK>, > - <&gcc UFS_ICE_CORE_CLK_SRC>, > <&gcc GCC_UFS_UNIPRO_CORE_CLK>, > <&gcc GCC_UFS_ICE_CORE_CLK>, > <&rpmcc RPM_SMD_LN_BB_CLK>, > @@ -2072,8 +2068,6 @@ ufshc: ufshc@624000 { > <&gcc GCC_UFS_RX_SYMBOL_0_CLK>; > freq-table-hz = > <100000000 200000000>, > - <100000000 200000000>, > - <0 0>, > <0 0>, > <0 0>, > <0 0>, > > -- > 2.39.2 >
First several patches target fixing the UFS support on the Qualcomm MSM8996 / APQ8096 platforms, broken by the commit b4e13e1ae95e ("scsi: ufs: qcom: Add multiple frequency support for MAX_CORE_CLK_1US_CYCLES"). Last two patches clean up the UFS DT device on that platform to follow the bindings on other MSM8969 platforms. If such breaking change is unacceptable, they can be simply ignored, merging fixes only. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- Changes in v2: - Dropped patches adding RX_SYMBOL_1_CLK, MSM8996 uses single lane (Krzysztof). - Link to v1: https://lore.kernel.org/r/20240209-msm8996-fix-ufs-v1-0-107b52e57420@linaro.org --- Dmitry Baryshkov (6): scsi: ufs: qcom: provide default cycles_in_1us value arm64: dts: qcom: msm8996: unbreak UFS HCD support arm64: dts: qcom: msm8996: specify UFS core_clk frequencies arm64: dts: qcom: msm8996: set GCC_UFS_ICE_CORE_CLK freq directly dt-bindings: ufs: qcom,ufs: drop source clock entries arm64: dts: qcom: msm8996: drop source clock entries from the UFS node Documentation/devicetree/bindings/ufs/qcom,ufs.yaml | 12 +++++------- arch/arm64/boot/dts/qcom/msm8996.dtsi | 8 +------- drivers/ufs/host/ufs-qcom.c | 6 ++++-- 3 files changed, 10 insertions(+), 16 deletions(-) --- base-commit: 4c4f1563cc49472e85613c1e4875258f6ec87105 change-id: 20240209-msm8996-fix-ufs-f80ae6d4d8cf Best regards,