| Message ID | 20241004080110.4150476-1-quic_kotarake@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | arm64: dts: qcom: qcm6490: Allow UFS regulators load/mode setting | expand |
On Fri, Oct 04, 2024 at 01:31:10PM GMT, Rakesh Kota wrote: > The UFS driver expects to be able to set load (and by extension, mode) > on its supply regulators. Add the necessary properties to make that > possible. > > While at it, UFS rails have different voltage requirement for UFS2.x > v/s UFS3.x. Bootloader sets the proper voltage based on UFS type. > There can be case where the voltage set by bootloader is overridden > by HLOS client. > It's generally not accepted to do "while at it"-changes, but it happens that I accept it. This one however, is controversial. We tend to want to properly describe the voltage constraints, so this departure from that stance definitely isn't an acceptable "while at it"-change... > To prevent above issue, add change to remove voltage voting support > for dedicated UFS rails. You state that the bootloader will configure the appropriate voltage, I don't see a issue description. Either way, please split this in two. Regards, Bjorn > > Signed-off-by: Rakesh Kota <quic_kotarake@quicinc.com> > --- > arch/arm64/boot/dts/qcom/qcm6490-idp.dts | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts > index 84c45419cb8d..8a4df9c2a946 100644 > --- a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts > +++ b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts > @@ -258,13 +258,15 @@ vreg_l6b_1p2: ldo6 { > regulator-name = "vreg_l6b_1p2"; > regulator-min-microvolt = <1140000>; > regulator-max-microvolt = <1260000>; > + regulator-allow-set-load; > + regulator-allowed-modes = <RPMH_REGULATOR_MODE_LPM RPMH_REGULATOR_MODE_HPM>; > regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; > }; > > vreg_l7b_2p952: ldo7 { > regulator-name = "vreg_l7b_2p952"; > - regulator-min-microvolt = <2400000>; > - regulator-max-microvolt = <3544000>; > + regulator-allow-set-load; > + regulator-allowed-modes = <RPMH_REGULATOR_MODE_LPM RPMH_REGULATOR_MODE_HPM>; > regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; > }; > > @@ -277,8 +279,8 @@ vreg_l8b_0p904: ldo8 { > > vreg_l9b_1p2: ldo9 { > regulator-name = "vreg_l9b_1p2"; > - regulator-min-microvolt = <1200000>; > - regulator-max-microvolt = <1304000>; > + regulator-allow-set-load; > + regulator-allowed-modes = <RPMH_REGULATOR_MODE_LPM RPMH_REGULATOR_MODE_HPM>; > regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; > }; > > @@ -467,6 +469,8 @@ vreg_l10c_0p88: ldo10 { > regulator-name = "vreg_l10c_0p88"; > regulator-min-microvolt = <720000>; > regulator-max-microvolt = <1050000>; > + regulator-allow-set-load; > + regulator-allowed-modes = <RPMH_REGULATOR_MODE_LPM RPMH_REGULATOR_MODE_HPM>; > regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; > }; > > -- > 2.34.1 >
On Fri, Oct 04, 2024 at 01:31:10PM GMT, Rakesh Kota wrote: > The UFS driver expects to be able to set load (and by extension, mode) > on its supply regulators. Add the necessary properties to make that > possible. > > While at it, UFS rails have different voltage requirement for UFS2.x > v/s UFS3.x. Bootloader sets the proper voltage based on UFS type. > There can be case where the voltage set by bootloader is overridden > by HLOS client. > > To prevent above issue, add change to remove voltage voting support > for dedicated UFS rails. add change to remove smth doesn't sound correct to me. Please don't depend on the bootloader and describe hardware configuration. If there can be two types of IDP boards and you can not identify the voltage via other means, please create something like qcm6490-idp-ufs3.dts. Please add proper Fixes tags. Last, but not least, as Bjorn wrote, please split into two patches. > > Signed-off-by: Rakesh Kota <quic_kotarake@quicinc.com> > --- > arch/arm64/boot/dts/qcom/qcm6490-idp.dts | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts > index 84c45419cb8d..8a4df9c2a946 100644 > --- a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts > +++ b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts > @@ -258,13 +258,15 @@ vreg_l6b_1p2: ldo6 { > regulator-name = "vreg_l6b_1p2"; > regulator-min-microvolt = <1140000>; > regulator-max-microvolt = <1260000>; > + regulator-allow-set-load; > + regulator-allowed-modes = <RPMH_REGULATOR_MODE_LPM RPMH_REGULATOR_MODE_HPM>; > regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; > }; > > vreg_l7b_2p952: ldo7 { > regulator-name = "vreg_l7b_2p952"; > - regulator-min-microvolt = <2400000>; > - regulator-max-microvolt = <3544000>; > + regulator-allow-set-load; > + regulator-allowed-modes = <RPMH_REGULATOR_MODE_LPM RPMH_REGULATOR_MODE_HPM>; > regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; > }; > > @@ -277,8 +279,8 @@ vreg_l8b_0p904: ldo8 { > > vreg_l9b_1p2: ldo9 { > regulator-name = "vreg_l9b_1p2"; > - regulator-min-microvolt = <1200000>; > - regulator-max-microvolt = <1304000>; > + regulator-allow-set-load; > + regulator-allowed-modes = <RPMH_REGULATOR_MODE_LPM RPMH_REGULATOR_MODE_HPM>; > regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; > }; > > @@ -467,6 +469,8 @@ vreg_l10c_0p88: ldo10 { > regulator-name = "vreg_l10c_0p88"; > regulator-min-microvolt = <720000>; > regulator-max-microvolt = <1050000>; > + regulator-allow-set-load; > + regulator-allowed-modes = <RPMH_REGULATOR_MODE_LPM RPMH_REGULATOR_MODE_HPM>; > regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; > }; > > -- > 2.34.1 >
On 10/7/2024 1:37 AM, Dmitry Baryshkov wrote: > On Fri, Oct 04, 2024 at 01:31:10PM GMT, Rakesh Kota wrote: >> The UFS driver expects to be able to set load (and by extension, mode) >> on its supply regulators. Add the necessary properties to make that >> possible. >> >> While at it, UFS rails have different voltage requirement for UFS2.x >> v/s UFS3.x. Bootloader sets the proper voltage based on UFS type. >> There can be case where the voltage set by bootloader is overridden >> by HLOS client. >> >> To prevent above issue, add change to remove voltage voting support >> for dedicated UFS rails. > > add change to remove smth doesn't sound correct to me. > Please don't depend on the bootloader and describe hardware > configuration. If there can be two types of IDP boards and you can not > identify the voltage via other means, please create something like > qcm6490-idp-ufs3.dts. Please add proper Fixes tags. > Last, but not least, as Bjorn wrote, please split into two patches. > sure, i will split the change into two. Since we can’t differentiate IDP boards based on UFS versions while loading the DT and we have only single board ID for the IDP's, it’s not possible to create separate UFS-based DT files like qcm6490-idp-ufs3.dts and ufs2.dtsi... etc. And also UFS driver does not vote for voltage on UFS rails & they just vote on load only. Hence to support both UFS 2.x and 3.x, we need to remove the voltage min/max voting. if add the min and max voltages in DT, then those initial voltage set by bootloader is overridden by regulator framework with min voltage specified in DT. Note: Bootloader have capability to detect the UFS version (where as HLOS does not have that capability) Thank you for quick review!! >> >> Signed-off-by: Rakesh Kota <quic_kotarake@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/qcm6490-idp.dts | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts >> index 84c45419cb8d..8a4df9c2a946 100644 >> --- a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts >> +++ b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts >> @@ -258,13 +258,15 @@ vreg_l6b_1p2: ldo6 { >> regulator-name = "vreg_l6b_1p2"; >> regulator-min-microvolt = <1140000>; >> regulator-max-microvolt = <1260000>; >> + regulator-allow-set-load; >> + regulator-allowed-modes = <RPMH_REGULATOR_MODE_LPM RPMH_REGULATOR_MODE_HPM>; >> regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; >> }; >> >> vreg_l7b_2p952: ldo7 { >> regulator-name = "vreg_l7b_2p952"; >> - regulator-min-microvolt = <2400000>; >> - regulator-max-microvolt = <3544000>; >> + regulator-allow-set-load; >> + regulator-allowed-modes = <RPMH_REGULATOR_MODE_LPM RPMH_REGULATOR_MODE_HPM>; >> regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; >> }; >> >> @@ -277,8 +279,8 @@ vreg_l8b_0p904: ldo8 { >> >> vreg_l9b_1p2: ldo9 { >> regulator-name = "vreg_l9b_1p2"; >> - regulator-min-microvolt = <1200000>; >> - regulator-max-microvolt = <1304000>; >> + regulator-allow-set-load; >> + regulator-allowed-modes = <RPMH_REGULATOR_MODE_LPM RPMH_REGULATOR_MODE_HPM>; >> regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; >> }; >> >> @@ -467,6 +469,8 @@ vreg_l10c_0p88: ldo10 { >> regulator-name = "vreg_l10c_0p88"; >> regulator-min-microvolt = <720000>; >> regulator-max-microvolt = <1050000>; >> + regulator-allow-set-load; >> + regulator-allowed-modes = <RPMH_REGULATOR_MODE_LPM RPMH_REGULATOR_MODE_HPM>; >> regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; >> }; >> >> -- >> 2.34.1 >> >
On Tue, 8 Oct 2024 at 12:39, Rakesh Kota <quic_kotarake@quicinc.com> wrote: > > > > On 10/7/2024 1:37 AM, Dmitry Baryshkov wrote: > > On Fri, Oct 04, 2024 at 01:31:10PM GMT, Rakesh Kota wrote: > >> The UFS driver expects to be able to set load (and by extension, mode) > >> on its supply regulators. Add the necessary properties to make that > >> possible. > >> > >> While at it, UFS rails have different voltage requirement for UFS2.x > >> v/s UFS3.x. Bootloader sets the proper voltage based on UFS type. > >> There can be case where the voltage set by bootloader is overridden > >> by HLOS client. > >> > >> To prevent above issue, add change to remove voltage voting support > >> for dedicated UFS rails. > > > > add change to remove smth doesn't sound correct to me. > > Please don't depend on the bootloader and describe hardware > configuration. If there can be two types of IDP boards and you can not > > identify the voltage via other means, please create something like > > qcm6490-idp-ufs3.dts. Please add proper Fixes tags. > > Last, but not least, as Bjorn wrote, please split into two patches. > > > sure, i will split the change into two. > > Since we can’t differentiate IDP boards based on UFS versions while > loading the DT and we have only single board ID for the IDP's, it’s not > possible to create separate UFS-based DT files like qcm6490-idp-ufs3.dts > and ufs2.dtsi... etc. It is definitely possible to create a second DT file. And upstream doesn't have board IDs (not to mention that nothing stops you from using another board ID for IDP with a different UFS revision. Qualcomm owns the board ID registry.) > > And also UFS driver does not vote for voltage on UFS rails & they just > vote on load only. > Hence to support both UFS 2.x and 3.x, we need to remove the voltage > min/max voting. if add the min and max voltages in DT, then those > initial voltage set by bootloader is overridden by regulator > framework with min voltage specified in DT. Yes, this is correct. Regulator framework should know min and max voltages. Once the voltage is in the specified range, the UFS driver doesn't have to cast a particular vote on it. So, dropping the regulator min/max is still NAKed. > > Note: Bootloader have capability to detect the UFS version (where as > HLOS does not have that capability) > > Thank you for quick review!! > >> > >> Signed-off-by: Rakesh Kota <quic_kotarake@quicinc.com> > >> --- > >> arch/arm64/boot/dts/qcom/qcm6490-idp.dts | 12 ++++++++---- > >> 1 file changed, 8 insertions(+), 4 deletions(-) > >> > >> diff --git a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts > >> index 84c45419cb8d..8a4df9c2a946 100644 > >> --- a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts > >> +++ b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts > >> @@ -258,13 +258,15 @@ vreg_l6b_1p2: ldo6 { > >> regulator-name = "vreg_l6b_1p2"; > >> regulator-min-microvolt = <1140000>; > >> regulator-max-microvolt = <1260000>; > >> + regulator-allow-set-load; > >> + regulator-allowed-modes = <RPMH_REGULATOR_MODE_LPM RPMH_REGULATOR_MODE_HPM>; > >> regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; > >> }; > >> > >> vreg_l7b_2p952: ldo7 { > >> regulator-name = "vreg_l7b_2p952"; > >> - regulator-min-microvolt = <2400000>; > >> - regulator-max-microvolt = <3544000>; > >> + regulator-allow-set-load; > >> + regulator-allowed-modes = <RPMH_REGULATOR_MODE_LPM RPMH_REGULATOR_MODE_HPM>; > >> regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; > >> }; > >> > >> @@ -277,8 +279,8 @@ vreg_l8b_0p904: ldo8 { > >> > >> vreg_l9b_1p2: ldo9 { > >> regulator-name = "vreg_l9b_1p2"; > >> - regulator-min-microvolt = <1200000>; > >> - regulator-max-microvolt = <1304000>; > >> + regulator-allow-set-load; > >> + regulator-allowed-modes = <RPMH_REGULATOR_MODE_LPM RPMH_REGULATOR_MODE_HPM>; > >> regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; > >> }; > >> > >> @@ -467,6 +469,8 @@ vreg_l10c_0p88: ldo10 { > >> regulator-name = "vreg_l10c_0p88"; > >> regulator-min-microvolt = <720000>; > >> regulator-max-microvolt = <1050000>; > >> + regulator-allow-set-load; > >> + regulator-allowed-modes = <RPMH_REGULATOR_MODE_LPM RPMH_REGULATOR_MODE_HPM>; > >> regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; > >> }; > >> > >> -- > >> 2.34.1 > >> > >
diff --git a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts index 84c45419cb8d..8a4df9c2a946 100644 --- a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts +++ b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts @@ -258,13 +258,15 @@ vreg_l6b_1p2: ldo6 { regulator-name = "vreg_l6b_1p2"; regulator-min-microvolt = <1140000>; regulator-max-microvolt = <1260000>; + regulator-allow-set-load; + regulator-allowed-modes = <RPMH_REGULATOR_MODE_LPM RPMH_REGULATOR_MODE_HPM>; regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; }; vreg_l7b_2p952: ldo7 { regulator-name = "vreg_l7b_2p952"; - regulator-min-microvolt = <2400000>; - regulator-max-microvolt = <3544000>; + regulator-allow-set-load; + regulator-allowed-modes = <RPMH_REGULATOR_MODE_LPM RPMH_REGULATOR_MODE_HPM>; regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; }; @@ -277,8 +279,8 @@ vreg_l8b_0p904: ldo8 { vreg_l9b_1p2: ldo9 { regulator-name = "vreg_l9b_1p2"; - regulator-min-microvolt = <1200000>; - regulator-max-microvolt = <1304000>; + regulator-allow-set-load; + regulator-allowed-modes = <RPMH_REGULATOR_MODE_LPM RPMH_REGULATOR_MODE_HPM>; regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; }; @@ -467,6 +469,8 @@ vreg_l10c_0p88: ldo10 { regulator-name = "vreg_l10c_0p88"; regulator-min-microvolt = <720000>; regulator-max-microvolt = <1050000>; + regulator-allow-set-load; + regulator-allowed-modes = <RPMH_REGULATOR_MODE_LPM RPMH_REGULATOR_MODE_HPM>; regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; };
The UFS driver expects to be able to set load (and by extension, mode) on its supply regulators. Add the necessary properties to make that possible. While at it, UFS rails have different voltage requirement for UFS2.x v/s UFS3.x. Bootloader sets the proper voltage based on UFS type. There can be case where the voltage set by bootloader is overridden by HLOS client. To prevent above issue, add change to remove voltage voting support for dedicated UFS rails. Signed-off-by: Rakesh Kota <quic_kotarake@quicinc.com> --- arch/arm64/boot/dts/qcom/qcm6490-idp.dts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)