| Message ID | 20250113-sm8750_ufs_master-v1-4-b3774120eb8c@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add UFS support for SM8750 | expand |
On Mon, Jan 13, 2025 at 01:46:27PM -0800, Melody Olvera wrote: > From: Nitin Rawat <quic_nitirawa@quicinc.com> > > Add UFS host controller and PHY nodes for SM8750 SoC. > > Co-developed-by: Manish Pandey <quic_mapa@quicinc.com> > Signed-off-by: Manish Pandey <quic_mapa@quicinc.com> > Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com> > Signed-off-by: Melody Olvera <quic_molvera@quicinc.com> > --- > arch/arm64/boot/dts/qcom/sm8750.dtsi | 81 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 81 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sm8750.dtsi b/arch/arm64/boot/dts/qcom/sm8750.dtsi > index 3bbd7d18598ee0a3a0d5130c03a3166e1fc14d82..20690c102244b337847a6482dd83c37e19746de9 100644 > --- a/arch/arm64/boot/dts/qcom/sm8750.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8750.dtsi > @@ -13,6 +13,7 @@ > #include <dt-bindings/power/qcom,rpmhpd.h> > #include <dt-bindings/power/qcom-rpmpd.h> > #include <dt-bindings/soc/qcom,rpmh-rsc.h> > +#include <dt-bindings/gpio/gpio.h> > > / { > interrupt-parent = <&intc>; > @@ -1939,6 +1940,86 @@ mmss_noc: interconnect@1780000 { > #interconnect-cells = <2>; > }; > > + ufs_mem_phy: phy@1d80000 { > + compatible = "qcom,sm8750-qmp-ufs-phy"; > + reg = <0x0 0x01d80000 0x0 0x2000>; > + > + clocks = <&rpmhcc RPMH_CXO_CLK>, > + <&gcc GCC_UFS_PHY_PHY_AUX_CLK>, > + <&tcsrcc TCSR_UFS_CLKREF_EN>; > + clock-names = "ref", > + "ref_aux", > + "qref"; > + > + resets = <&ufs_mem_hc 0>; > + reset-names = "ufsphy"; > + > + power-domains = <&gcc GCC_UFS_MEM_PHY_GDSC>; > + > + #clock-cells = <1>; > + #phy-cells = <0>; > + > + status = "disabled"; > + }; > + > + ufs_mem_hc: ufs@1d84000 { > + compatible = "qcom,sm8750-ufshc", "qcom,ufshc", "jedec,ufs-2.0"; > + reg = <0x0 0x01d84000 0x0 0x3000>; > + > + interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>; > + > + clocks = <&gcc GCC_UFS_PHY_AXI_CLK>, > + <&gcc GCC_AGGRE_UFS_PHY_AXI_CLK>, > + <&gcc GCC_UFS_PHY_AHB_CLK>, > + <&gcc GCC_UFS_PHY_UNIPRO_CORE_CLK>, > + <&rpmhcc RPMH_LN_BB_CLK3>, > + <&gcc GCC_UFS_PHY_TX_SYMBOL_0_CLK>, > + <&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>, > + <&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>; > + clock-names = "core_clk", > + "bus_aggr_clk", > + "iface_clk", > + "core_clk_unipro", > + "ref_clk", > + "tx_lane0_sync_clk", > + "rx_lane0_sync_clk", > + "rx_lane1_sync_clk"; > + freq-table-hz = <100000000 403000000>, > + <0 0>, > + <0 0>, > + <100000000 403000000>, > + <100000000 403000000>, > + <0 0>, > + <0 0>, > + <0 0>; Use OPP table instead > + > + resets = <&gcc GCC_UFS_PHY_BCR>; > + reset-names = "rst"; > + > + > + interconnects = <&aggre1_noc MASTER_UFS_MEM QCOM_ICC_TAG_ALWAYS > + &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>, > + <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS > + &config_noc SLAVE_UFS_MEM_CFG QCOM_ICC_TAG_ALWAYS>; Shouldn't cpu-ufs be ACTIVE_ONLY? > + interconnect-names = "ufs-ddr", > + "cpu-ufs"; > + > + power-domains = <&gcc GCC_UFS_PHY_GDSC>; > + required-opps = <&rpmhpd_opp_nom>; > + > + iommus = <&apps_smmu 0x60 0>; > + dma-coherent; > + > + lanes-per-direction = <2>; > + > + phys = <&ufs_mem_phy>; > + phy-names = "ufsphy"; > + > + #reset-cells = <1>; > + > + status = "disabled"; > + }; > + > tcsr_mutex: hwlock@1f40000 { > compatible = "qcom,tcsr-mutex"; > reg = <0x0 0x01f40000 0x0 0x20000>; > > -- > 2.46.1 >
On Mon, Jan 13, 2025 at 01:46:27PM -0800, Melody Olvera wrote: > From: Nitin Rawat <quic_nitirawa@quicinc.com> > > Add UFS host controller and PHY nodes for SM8750 SoC. > > Co-developed-by: Manish Pandey <quic_mapa@quicinc.com> > Signed-off-by: Manish Pandey <quic_mapa@quicinc.com> > Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com> > Signed-off-by: Melody Olvera <quic_molvera@quicinc.com> > --- > arch/arm64/boot/dts/qcom/sm8750.dtsi | 81 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 81 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sm8750.dtsi b/arch/arm64/boot/dts/qcom/sm8750.dtsi > index 3bbd7d18598ee0a3a0d5130c03a3166e1fc14d82..20690c102244b337847a6482dd83c37e19746de9 100644 > --- a/arch/arm64/boot/dts/qcom/sm8750.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8750.dtsi > @@ -13,6 +13,7 @@ > #include <dt-bindings/power/qcom,rpmhpd.h> > #include <dt-bindings/power/qcom-rpmpd.h> > #include <dt-bindings/soc/qcom,rpmh-rsc.h> > +#include <dt-bindings/gpio/gpio.h> > > / { > interrupt-parent = <&intc>; > @@ -1939,6 +1940,86 @@ mmss_noc: interconnect@1780000 { > #interconnect-cells = <2>; > }; > > + ufs_mem_phy: phy@1d80000 { > + compatible = "qcom,sm8750-qmp-ufs-phy"; > + reg = <0x0 0x01d80000 0x0 0x2000>; > + > + clocks = <&rpmhcc RPMH_CXO_CLK>, > + <&gcc GCC_UFS_PHY_PHY_AUX_CLK>, > + <&tcsrcc TCSR_UFS_CLKREF_EN>; > + clock-names = "ref", Since there is going to be resend, let's save me one commit afterwards: Incorrect space after '='. There is always only one before and one after. Best regards, Krzysztof
On 13.01.2025 10:46 PM, Melody Olvera wrote: > From: Nitin Rawat <quic_nitirawa@quicinc.com> > > Add UFS host controller and PHY nodes for SM8750 SoC. > > Co-developed-by: Manish Pandey <quic_mapa@quicinc.com> > Signed-off-by: Manish Pandey <quic_mapa@quicinc.com> > Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com> > Signed-off-by: Melody Olvera <quic_molvera@quicinc.com> > --- Please also add this bit: diff --git a/arch/arm64/boot/dts/qcom/sm8750.dtsi b/arch/arm64/boot/dts/qcom/sm8750.dtsi index 3bbd7d18598e..1f79f2adb0a5 100644 --- a/arch/arm64/boot/dts/qcom/sm8750.dtsi +++ b/arch/arm64/boot/dts/qcom/sm8750.dtsi @@ -532,9 +532,9 @@ gcc: clock-controller@100000 { <0>, <&sleep_clk>, <0>, - <0>, - <0>, - <0>, + <&ufs_mem_phy 0>, + <&ufs_mem_phy 1>, + <&ufs_mem_phy 2>, <0>; #clock-cells = <1>; Konrad
On 1/14/2025 4:22 PM, Dmitry Baryshkov wrote: > On Mon, Jan 13, 2025 at 01:46:27PM -0800, Melody Olvera wrote: >> From: Nitin Rawat <quic_nitirawa@quicinc.com> >> >> Add UFS host controller and PHY nodes for SM8750 SoC. >> >> Co-developed-by: Manish Pandey <quic_mapa@quicinc.com> >> Signed-off-by: Manish Pandey <quic_mapa@quicinc.com> >> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com> >> Signed-off-by: Melody Olvera <quic_molvera@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/sm8750.dtsi | 81 ++++++++++++++++++++++++++++++++++++ >> 1 file changed, 81 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sm8750.dtsi b/arch/arm64/boot/dts/qcom/sm8750.dtsi >> index 3bbd7d18598ee0a3a0d5130c03a3166e1fc14d82..20690c102244b337847a6482dd83c37e19746de9 100644 >> --- a/arch/arm64/boot/dts/qcom/sm8750.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sm8750.dtsi >> @@ -13,6 +13,7 @@ >> #include <dt-bindings/power/qcom,rpmhpd.h> >> #include <dt-bindings/power/qcom-rpmpd.h> >> #include <dt-bindings/soc/qcom,rpmh-rsc.h> >> +#include <dt-bindings/gpio/gpio.h> >> >> / { >> interrupt-parent = <&intc>; >> @@ -1939,6 +1940,86 @@ mmss_noc: interconnect@1780000 { >> #interconnect-cells = <2>; >> }; >> >> + ufs_mem_phy: phy@1d80000 { >> + compatible = "qcom,sm8750-qmp-ufs-phy"; >> + reg = <0x0 0x01d80000 0x0 0x2000>; >> + >> + clocks = <&rpmhcc RPMH_CXO_CLK>, >> + <&gcc GCC_UFS_PHY_PHY_AUX_CLK>, >> + <&tcsrcc TCSR_UFS_CLKREF_EN>; >> + clock-names = "ref", >> + "ref_aux", >> + "qref"; >> + >> + resets = <&ufs_mem_hc 0>; >> + reset-names = "ufsphy"; >> + >> + power-domains = <&gcc GCC_UFS_MEM_PHY_GDSC>; >> + >> + #clock-cells = <1>; >> + #phy-cells = <0>; >> + >> + status = "disabled"; >> + }; >> + >> + ufs_mem_hc: ufs@1d84000 { >> + compatible = "qcom,sm8750-ufshc", "qcom,ufshc", "jedec,ufs-2.0"; >> + reg = <0x0 0x01d84000 0x0 0x3000>; >> + >> + interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>; >> + >> + clocks = <&gcc GCC_UFS_PHY_AXI_CLK>, >> + <&gcc GCC_AGGRE_UFS_PHY_AXI_CLK>, >> + <&gcc GCC_UFS_PHY_AHB_CLK>, >> + <&gcc GCC_UFS_PHY_UNIPRO_CORE_CLK>, >> + <&rpmhcc RPMH_LN_BB_CLK3>, >> + <&gcc GCC_UFS_PHY_TX_SYMBOL_0_CLK>, >> + <&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>, >> + <&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>; >> + clock-names = "core_clk", >> + "bus_aggr_clk", >> + "iface_clk", >> + "core_clk_unipro", >> + "ref_clk", >> + "tx_lane0_sync_clk", >> + "rx_lane0_sync_clk", >> + "rx_lane1_sync_clk"; >> + freq-table-hz = <100000000 403000000>, >> + <0 0>, >> + <0 0>, >> + <100000000 403000000>, >> + <100000000 403000000>, >> + <0 0>, >> + <0 0>, >> + <0 0>; > > Use OPP table instead Currently, OPP is not enabled in the device tree for any previous targets. I plan to enable OPP in a separate patch at a later stage. This is because there is an ongoing patch in the upstream that aims to enable multiple-level clock scaling using OPP, which may introduce changes to the device tree entries. To avoid extra efforts, I intend to enable OPP once that patch is merged. Please let me know if you have any concerns. > >> + >> + resets = <&gcc GCC_UFS_PHY_BCR>; >> + reset-names = "rst"; >> + >> + >> + interconnects = <&aggre1_noc MASTER_UFS_MEM QCOM_ICC_TAG_ALWAYS >> + &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>, >> + <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS >> + &config_noc SLAVE_UFS_MEM_CFG QCOM_ICC_TAG_ALWAYS>; > > Shouldn't cpu-ufs be ACTIVE_ONLY? As per ufs driver implementation, Icc voting from ufs driver is removed as part of low power mode (suspend or clock gating) and voted again in resume/ungating path. Hence TAG_ALWAYS will have no power concern. All previous targets have the same configuration. Thanks, Nitin > >> + interconnect-names = "ufs-ddr", >> + "cpu-ufs"; >> + >> + power-domains = <&gcc GCC_UFS_PHY_GDSC>; >> + required-opps = <&rpmhpd_opp_nom>; >> + >> + iommus = <&apps_smmu 0x60 0>; >> + dma-coherent; >> + >> + lanes-per-direction = <2>; >> + >> + phys = <&ufs_mem_phy>; >> + phy-names = "ufsphy"; >> + >> + #reset-cells = <1>; >> + >> + status = "disabled"; >> + }; >> + >> tcsr_mutex: hwlock@1f40000 { >> compatible = "qcom,tcsr-mutex"; >> reg = <0x0 0x01f40000 0x0 0x20000>; >> >> -- >> 2.46.1 >> >
On Sun, Feb 09, 2025 at 12:47:56AM +0530, Nitin Rawat wrote: > > > On 1/14/2025 4:22 PM, Dmitry Baryshkov wrote: > > On Mon, Jan 13, 2025 at 01:46:27PM -0800, Melody Olvera wrote: > > > From: Nitin Rawat <quic_nitirawa@quicinc.com> > > > > > > Add UFS host controller and PHY nodes for SM8750 SoC. > > > > > > Co-developed-by: Manish Pandey <quic_mapa@quicinc.com> > > > Signed-off-by: Manish Pandey <quic_mapa@quicinc.com> > > > Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com> > > > Signed-off-by: Melody Olvera <quic_molvera@quicinc.com> > > > --- > > > arch/arm64/boot/dts/qcom/sm8750.dtsi | 81 ++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 81 insertions(+) > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sm8750.dtsi b/arch/arm64/boot/dts/qcom/sm8750.dtsi > > > index 3bbd7d18598ee0a3a0d5130c03a3166e1fc14d82..20690c102244b337847a6482dd83c37e19746de9 100644 > > > --- a/arch/arm64/boot/dts/qcom/sm8750.dtsi > > > +++ b/arch/arm64/boot/dts/qcom/sm8750.dtsi > > > @@ -13,6 +13,7 @@ > > > #include <dt-bindings/power/qcom,rpmhpd.h> > > > #include <dt-bindings/power/qcom-rpmpd.h> > > > #include <dt-bindings/soc/qcom,rpmh-rsc.h> > > > +#include <dt-bindings/gpio/gpio.h> > > > / { > > > interrupt-parent = <&intc>; > > > @@ -1939,6 +1940,86 @@ mmss_noc: interconnect@1780000 { > > > #interconnect-cells = <2>; > > > }; > > > + ufs_mem_phy: phy@1d80000 { > > > + compatible = "qcom,sm8750-qmp-ufs-phy"; > > > + reg = <0x0 0x01d80000 0x0 0x2000>; > > > + > > > + clocks = <&rpmhcc RPMH_CXO_CLK>, > > > + <&gcc GCC_UFS_PHY_PHY_AUX_CLK>, > > > + <&tcsrcc TCSR_UFS_CLKREF_EN>; > > > + clock-names = "ref", > > > + "ref_aux", > > > + "qref"; > > > + > > > + resets = <&ufs_mem_hc 0>; > > > + reset-names = "ufsphy"; > > > + > > > + power-domains = <&gcc GCC_UFS_MEM_PHY_GDSC>; > > > + > > > + #clock-cells = <1>; > > > + #phy-cells = <0>; > > > + > > > + status = "disabled"; > > > + }; > > > + > > > + ufs_mem_hc: ufs@1d84000 { > > > + compatible = "qcom,sm8750-ufshc", "qcom,ufshc", "jedec,ufs-2.0"; > > > + reg = <0x0 0x01d84000 0x0 0x3000>; > > > + > > > + interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>; > > > + > > > + clocks = <&gcc GCC_UFS_PHY_AXI_CLK>, > > > + <&gcc GCC_AGGRE_UFS_PHY_AXI_CLK>, > > > + <&gcc GCC_UFS_PHY_AHB_CLK>, > > > + <&gcc GCC_UFS_PHY_UNIPRO_CORE_CLK>, > > > + <&rpmhcc RPMH_LN_BB_CLK3>, > > > + <&gcc GCC_UFS_PHY_TX_SYMBOL_0_CLK>, > > > + <&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>, > > > + <&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>; > > > + clock-names = "core_clk", > > > + "bus_aggr_clk", > > > + "iface_clk", > > > + "core_clk_unipro", > > > + "ref_clk", > > > + "tx_lane0_sync_clk", > > > + "rx_lane0_sync_clk", > > > + "rx_lane1_sync_clk"; > > > + freq-table-hz = <100000000 403000000>, > > > + <0 0>, > > > + <0 0>, > > > + <100000000 403000000>, > > > + <100000000 403000000>, > > > + <0 0>, > > > + <0 0>, > > > + <0 0>; > > > > Use OPP table instead > > Currently, OPP is not enabled in the device tree for any previous targets. I Excuse me? ufs_opp_table is present on SM8250, SM8550 and SDM845 (and QCS615). So this is not correct > plan to enable OPP in a separate patch at a later stage. This is because > there is an ongoing patch in the upstream that aims to enable multiple-level > clock scaling using OPP, which may introduce changes to the device tree > entries. To avoid extra efforts, I intend to enable OPP once that patch is > merged. Whatever changes are introduced, old DT must still continue to work. There is no reason to use legacy freq-table-hz if you can use OPP table. > Please let me know if you have any concerns. > > > > > > > + > > > + resets = <&gcc GCC_UFS_PHY_BCR>; > > > + reset-names = "rst"; > > > + > > > + > > > + interconnects = <&aggre1_noc MASTER_UFS_MEM QCOM_ICC_TAG_ALWAYS > > > + &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>, > > > + <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS > > > + &config_noc SLAVE_UFS_MEM_CFG QCOM_ICC_TAG_ALWAYS>; > > > > Shouldn't cpu-ufs be ACTIVE_ONLY? > > As per ufs driver implementation, Icc voting from ufs driver is removed as > part of low power mode (suspend or clock gating) and voted again in > resume/ungating path. Hence TAG_ALWAYS will have no power concern. > All previous targets have the same configuration. arch/arm64/boot/dts/qcom/qcs615.dtsi: &config_noc SLAVE_UFS_MEM_CFG QCOM_ICC_TAG_ACTIVE_ONLY>; It might be a mistake for that target though. Your explanation sounds fine to me. > > Thanks, > Nitin > > > > > > > + interconnect-names = "ufs-ddr", > > > + "cpu-ufs"; > > > + > > > + power-domains = <&gcc GCC_UFS_PHY_GDSC>; > > > + required-opps = <&rpmhpd_opp_nom>; > > > + > > > + iommus = <&apps_smmu 0x60 0>; > > > + dma-coherent; > > > + > > > + lanes-per-direction = <2>; > > > + > > > + phys = <&ufs_mem_phy>; > > > + phy-names = "ufsphy"; > > > + > > > + #reset-cells = <1>; > > > + > > > + status = "disabled"; > > > + }; > > > + > > > tcsr_mutex: hwlock@1f40000 { > > > compatible = "qcom,tcsr-mutex"; > > > reg = <0x0 0x01f40000 0x0 0x20000>; > > > > > > -- > > > 2.46.1 > > > > > > > > -- > linux-phy mailing list > linux-phy@lists.infradead.org > https://lists.infradead.org/mailman/listinfo/linux-phy
On 8.02.2025 11:06 PM, Dmitry Baryshkov wrote: > On Sun, Feb 09, 2025 at 12:47:56AM +0530, Nitin Rawat wrote: >> >> >> On 1/14/2025 4:22 PM, Dmitry Baryshkov wrote: >>> On Mon, Jan 13, 2025 at 01:46:27PM -0800, Melody Olvera wrote: >>>> From: Nitin Rawat <quic_nitirawa@quicinc.com> >>>> >>>> Add UFS host controller and PHY nodes for SM8750 SoC. >>>> >>>> Co-developed-by: Manish Pandey <quic_mapa@quicinc.com> >>>> Signed-off-by: Manish Pandey <quic_mapa@quicinc.com> >>>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com> >>>> Signed-off-by: Melody Olvera <quic_molvera@quicinc.com> >>>> --- [...] >>> Use OPP table instead >> >> Currently, OPP is not enabled in the device tree for any previous targets. I > > Excuse me? ufs_opp_table is present on SM8250, SM8550 and SDM845 (and > QCS615). So this is not correct > >> plan to enable OPP in a separate patch at a later stage. This is because >> there is an ongoing patch in the upstream that aims to enable multiple-level >> clock scaling using OPP, which may introduce changes to the device tree >> entries. To avoid extra efforts, I intend to enable OPP once that patch is >> merged. > > Whatever changes are introduced, old DT must still continue to work. > There is no reason to use legacy freq-table-hz if you can use OPP table. > >> Please let me know if you have any concerns. Go ahead with the OPP table. freq-table-hz is ancient and doesn't describe e.g. the required RPMh levels for core clock frequencies. You should then drop required-opps from the UFS node. >>>> + >>>> + resets = <&gcc GCC_UFS_PHY_BCR>; >>>> + reset-names = "rst"; >>>> + >>>> + >>>> + interconnects = <&aggre1_noc MASTER_UFS_MEM QCOM_ICC_TAG_ALWAYS >>>> + &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>, >>>> + <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS >>>> + &config_noc SLAVE_UFS_MEM_CFG QCOM_ICC_TAG_ALWAYS>; >>> >>> Shouldn't cpu-ufs be ACTIVE_ONLY? >> >> As per ufs driver implementation, Icc voting from ufs driver is removed as >> part of low power mode (suspend or clock gating) and voted again in >> resume/ungating path. Hence TAG_ALWAYS will have no power concern. >> All previous targets have the same configuration. > > arch/arm64/boot/dts/qcom/qcs615.dtsi: &config_noc SLAVE_UFS_MEM_CFG QCOM_ICC_TAG_ACTIVE_ONLY>; > > It might be a mistake for that target though. Your explanation sounds > fine to me. Let's use QCOM_ICC_TAG_ACTIVE_ONLY for the CPU path to clear up confusion. Toggling it from the driver makes sense for UFS-idling-while-CPUs-are-online cases and accidentally also does what RPMh does internally in the other case. Konrad
On Mon, Feb 10, 2025 at 08:20:27PM +0100, Konrad Dybcio wrote: > On 8.02.2025 11:06 PM, Dmitry Baryshkov wrote: > > On Sun, Feb 09, 2025 at 12:47:56AM +0530, Nitin Rawat wrote: > >> > >> > >> On 1/14/2025 4:22 PM, Dmitry Baryshkov wrote: > >>> On Mon, Jan 13, 2025 at 01:46:27PM -0800, Melody Olvera wrote: > >>>> From: Nitin Rawat <quic_nitirawa@quicinc.com> > >>>> > >>>> Add UFS host controller and PHY nodes for SM8750 SoC. > >>>> > >>>> Co-developed-by: Manish Pandey <quic_mapa@quicinc.com> > >>>> Signed-off-by: Manish Pandey <quic_mapa@quicinc.com> > >>>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com> > >>>> Signed-off-by: Melody Olvera <quic_molvera@quicinc.com> > >>>> --- > > [...] > > >>> Use OPP table instead > >> > >> Currently, OPP is not enabled in the device tree for any previous targets. I > > > > Excuse me? ufs_opp_table is present on SM8250, SM8550 and SDM845 (and > > QCS615). So this is not correct > > > >> plan to enable OPP in a separate patch at a later stage. This is because > >> there is an ongoing patch in the upstream that aims to enable multiple-level > >> clock scaling using OPP, which may introduce changes to the device tree > >> entries. To avoid extra efforts, I intend to enable OPP once that patch is > >> merged. > > > > Whatever changes are introduced, old DT must still continue to work. > > There is no reason to use legacy freq-table-hz if you can use OPP table. > > > >> Please let me know if you have any concerns. > > Go ahead with the OPP table. freq-table-hz is ancient and doesn't describe > e.g. the required RPMh levels for core clock frequencies. > > You should then drop required-opps from the UFS node. > > >>>> + > >>>> + resets = <&gcc GCC_UFS_PHY_BCR>; > >>>> + reset-names = "rst"; > >>>> + > >>>> + > >>>> + interconnects = <&aggre1_noc MASTER_UFS_MEM QCOM_ICC_TAG_ALWAYS > >>>> + &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>, > >>>> + <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS > >>>> + &config_noc SLAVE_UFS_MEM_CFG QCOM_ICC_TAG_ALWAYS>; > >>> > >>> Shouldn't cpu-ufs be ACTIVE_ONLY? > >> > >> As per ufs driver implementation, Icc voting from ufs driver is removed as > >> part of low power mode (suspend or clock gating) and voted again in > >> resume/ungating path. Hence TAG_ALWAYS will have no power concern. > >> All previous targets have the same configuration. > > > > arch/arm64/boot/dts/qcom/qcs615.dtsi: &config_noc SLAVE_UFS_MEM_CFG QCOM_ICC_TAG_ACTIVE_ONLY>; > > > > It might be a mistake for that target though. Your explanation sounds > > fine to me. > > Let's use QCOM_ICC_TAG_ACTIVE_ONLY for the CPU path to clear up confusion. > > Toggling it from the driver makes sense for UFS-idling-while-CPUs-are-online > cases and accidentally also does what RPMh does internally in the other case. > Shouldn't it be applied to config path of all peripherals then? If QCOM_ICC_TAG_ACTIVE_ONLY translates to 'resource getting voted only if the CPUSS is active', then the same constraint should apply to all peripherals, isn't it? I'm not sure who is accessing the config path other than the CPUs. - Mani
diff --git a/arch/arm64/boot/dts/qcom/sm8750.dtsi b/arch/arm64/boot/dts/qcom/sm8750.dtsi index 3bbd7d18598ee0a3a0d5130c03a3166e1fc14d82..20690c102244b337847a6482dd83c37e19746de9 100644 --- a/arch/arm64/boot/dts/qcom/sm8750.dtsi +++ b/arch/arm64/boot/dts/qcom/sm8750.dtsi @@ -13,6 +13,7 @@ #include <dt-bindings/power/qcom,rpmhpd.h> #include <dt-bindings/power/qcom-rpmpd.h> #include <dt-bindings/soc/qcom,rpmh-rsc.h> +#include <dt-bindings/gpio/gpio.h> / { interrupt-parent = <&intc>; @@ -1939,6 +1940,86 @@ mmss_noc: interconnect@1780000 { #interconnect-cells = <2>; }; + ufs_mem_phy: phy@1d80000 { + compatible = "qcom,sm8750-qmp-ufs-phy"; + reg = <0x0 0x01d80000 0x0 0x2000>; + + clocks = <&rpmhcc RPMH_CXO_CLK>, + <&gcc GCC_UFS_PHY_PHY_AUX_CLK>, + <&tcsrcc TCSR_UFS_CLKREF_EN>; + clock-names = "ref", + "ref_aux", + "qref"; + + resets = <&ufs_mem_hc 0>; + reset-names = "ufsphy"; + + power-domains = <&gcc GCC_UFS_MEM_PHY_GDSC>; + + #clock-cells = <1>; + #phy-cells = <0>; + + status = "disabled"; + }; + + ufs_mem_hc: ufs@1d84000 { + compatible = "qcom,sm8750-ufshc", "qcom,ufshc", "jedec,ufs-2.0"; + reg = <0x0 0x01d84000 0x0 0x3000>; + + interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>; + + clocks = <&gcc GCC_UFS_PHY_AXI_CLK>, + <&gcc GCC_AGGRE_UFS_PHY_AXI_CLK>, + <&gcc GCC_UFS_PHY_AHB_CLK>, + <&gcc GCC_UFS_PHY_UNIPRO_CORE_CLK>, + <&rpmhcc RPMH_LN_BB_CLK3>, + <&gcc GCC_UFS_PHY_TX_SYMBOL_0_CLK>, + <&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>, + <&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>; + clock-names = "core_clk", + "bus_aggr_clk", + "iface_clk", + "core_clk_unipro", + "ref_clk", + "tx_lane0_sync_clk", + "rx_lane0_sync_clk", + "rx_lane1_sync_clk"; + freq-table-hz = <100000000 403000000>, + <0 0>, + <0 0>, + <100000000 403000000>, + <100000000 403000000>, + <0 0>, + <0 0>, + <0 0>; + + resets = <&gcc GCC_UFS_PHY_BCR>; + reset-names = "rst"; + + + interconnects = <&aggre1_noc MASTER_UFS_MEM QCOM_ICC_TAG_ALWAYS + &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>, + <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS + &config_noc SLAVE_UFS_MEM_CFG QCOM_ICC_TAG_ALWAYS>; + interconnect-names = "ufs-ddr", + "cpu-ufs"; + + power-domains = <&gcc GCC_UFS_PHY_GDSC>; + required-opps = <&rpmhpd_opp_nom>; + + iommus = <&apps_smmu 0x60 0>; + dma-coherent; + + lanes-per-direction = <2>; + + phys = <&ufs_mem_phy>; + phy-names = "ufsphy"; + + #reset-cells = <1>; + + status = "disabled"; + }; + tcsr_mutex: hwlock@1f40000 { compatible = "qcom,tcsr-mutex"; reg = <0x0 0x01f40000 0x0 0x20000>;