| Message ID | 1544634806-1037-1-git-send-email-loic.poulain@linaro.org |
|---|---|
| State | Accepted |
| Commit | e38161bd325ea541ef2f258d8e28281077dde524 |
| Headers | show |
| Series | arm64: dts: apq8096-db820c: Increase load on l21 for SDCARD | expand |
On 12/12/2018 10:13 AM, Loic Poulain wrote: > In the same way as for msm8974-hammerhead, l21 load, used for SDCARD > VMMC, needs to be increased in order to prevent any voltage drop issues > (due to limited current) happening with some SDCARDS or during specific > operations (e.g. write). > > Fixes: 660a9763c6a9 (arm64: dts: qcom: db820c: Add pm8994 regulator node) > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > --- > arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi > index 104cad9..c15e2c0 100644 > --- a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi > +++ b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi > @@ -634,6 +634,8 @@ > l21 { > regulator-min-microvolt = <2950000>; > regulator-max-microvolt = <2950000>; > + regulator-allow-set-load; > + regulator-system-load = <200000>; > }; > l22 { > regulator-min-microvolt = <3300000>; > I'm curious, why not update sdhci-msm to set the load on the regulator? -- Jeffrey Hugo Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
On Wed 12 Dec 09:13 PST 2018, Loic Poulain wrote: > In the same way as for msm8974-hammerhead, l21 load, used for SDCARD > VMMC, needs to be increased in order to prevent any voltage drop issues > (due to limited current) happening with some SDCARDS or during specific > operations (e.g. write). > > Fixes: 660a9763c6a9 (arm64: dts: qcom: db820c: Add pm8994 regulator node) > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> I agree with Jeff, that the sdhci-msm (or mmc framework even) should take care of this. But I suggest we merge this for now, regardless. Regards, Bjorn > --- > arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi > index 104cad9..c15e2c0 100644 > --- a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi > +++ b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi > @@ -634,6 +634,8 @@ > l21 { > regulator-min-microvolt = <2950000>; > regulator-max-microvolt = <2950000>; > + regulator-allow-set-load; > + regulator-system-load = <200000>; > }; > l22 { > regulator-min-microvolt = <3300000>; > -- > 2.7.4 >
On 12/13/2018 12:55 AM, Loic Poulain wrote: > Hi Jeffrey, > > > On Wed, 12 Dec 2018 at 18:23, Jeffrey Hugo <jhugo@codeaurora.org > <mailto:jhugo@codeaurora.org>> wrote: > > On 12/12/2018 10:13 AM, Loic Poulain wrote: > > In the same way as for msm8974-hammerhead, l21 load, used for SDCARD > > VMMC, needs to be increased in order to prevent any voltage drop > issues > > (due to limited current) happening with some SDCARDS or during > specific > > operations (e.g. write). > > > > Fixes: 660a9763c6a9 (arm64: dts: qcom: db820c: Add pm8994 > regulator node) > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org > <mailto:loic.poulain@linaro.org>> > > --- > > arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi > b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi > > index 104cad9..c15e2c0 100644 > > --- a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi > > +++ b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi > > @@ -634,6 +634,8 @@ > > l21 { > > regulator-min-microvolt = > <2950000>; > > regulator-max-microvolt = > <2950000>; > > + regulator-allow-set-load; > > + regulator-system-load = > <200000>; > > }; > > l22 { > > regulator-min-microvolt = > <3300000>; > > > > I'm curious, why not update sdhci-msm to set the load on the regulator? > > > Yes you're right, and I saw that there is ongoing work: > https://patchwork.kernel.org/patch/10630731/ > > Howerver I thought this change would be a quicker fix and easier to > backport in stable trees. > I assume all the device-tree vmmc loads will be removed at some point > when driven from sdhci. > I hadn't seen that. Ok, seems good to me. -- Jeffrey Hugo Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Hi, On Thu, Dec 13, 2018 at 6:46 AM Jeffrey Hugo <jhugo@codeaurora.org> wrote: > > On 12/13/2018 12:55 AM, Loic Poulain wrote: > > Hi Jeffrey, > > > > > > On Wed, 12 Dec 2018 at 18:23, Jeffrey Hugo <jhugo@codeaurora.org > > <mailto:jhugo@codeaurora.org>> wrote: > > > > On 12/12/2018 10:13 AM, Loic Poulain wrote: > > > In the same way as for msm8974-hammerhead, l21 load, used for SDCARD > > > VMMC, needs to be increased in order to prevent any voltage drop > > issues > > > (due to limited current) happening with some SDCARDS or during > > specific > > > operations (e.g. write). > > > > > > Fixes: 660a9763c6a9 (arm64: dts: qcom: db820c: Add pm8994 > > regulator node) > > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org > > <mailto:loic.poulain@linaro.org>> > > > --- > > > arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi > > b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi > > > index 104cad9..c15e2c0 100644 > > > --- a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi > > > +++ b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi > > > @@ -634,6 +634,8 @@ > > > l21 { > > > regulator-min-microvolt = > > <2950000>; > > > regulator-max-microvolt = > > <2950000>; > > > + regulator-allow-set-load; > > > + regulator-system-load = > > <200000>; > > > }; > > > l22 { > > > regulator-min-microvolt = > > <3300000>; > > > > > > > I'm curious, why not update sdhci-msm to set the load on the regulator? > > > > > > Yes you're right, and I saw that there is ongoing work: > > https://patchwork.kernel.org/patch/10630731/ > > > > Howerver I thought this change would be a quicker fix and easier to > > backport in stable trees. > > I assume all the device-tree vmmc loads will be removed at some point > > when driven from sdhci. > > > > I hadn't seen that. Ok, seems good to me. NOTE: I'm personally not convinced that adding the "set_load" calls into the SDHCI driver actually makes any sense. I believe it adds complexity for no benefit. The only time you ever need to should ever be fiddling with "set_load" calls is if the rail you're controlling has some hope of being able to run at a lower power mode. If there's no hope of it being at a lower power mode then the constraints on the rail should just force it to high power mode and be done with it. The patch here (using regulator-system-load) is one way to force it to a high power mode and seems fine, but there are other ways. See a previous discussion [1]. NOTE: IIRC the "ongoing work" patch you pointed at always sets the load to a fixed level to turn it to "high power mode" when the regulator is turned on and undoes that set_load when the regulator is turned off. That's no longer needed as of commit 5451781dadf8 ("regulator: core: Only count load for enabled consumers"). If someone comes up with a case where it's useful to keep the SD card rail turned on but in "low power mode" _then_ we should actually consider adding set_load to the SD card driver. [1] https://lkml.kernel.org/r/CAD=FV=V4WFYoKLQ72pico4HCGgLDTae7xougivv6VWOSoPhLpg@mail.gmail.com -Doug
Hi Andy, Rob, Could any of you take this patch? On Thu, 13 Dec 2018 at 20:14, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Thu, Dec 13, 2018 at 6:46 AM Jeffrey Hugo <jhugo@codeaurora.org> wrote: > > > > On 12/13/2018 12:55 AM, Loic Poulain wrote: > > > Hi Jeffrey, > > > > > > > > > On Wed, 12 Dec 2018 at 18:23, Jeffrey Hugo <jhugo@codeaurora.org > > > <mailto:jhugo@codeaurora.org>> wrote: > > > > > > On 12/12/2018 10:13 AM, Loic Poulain wrote: > > > > In the same way as for msm8974-hammerhead, l21 load, used for SDCARD > > > > VMMC, needs to be increased in order to prevent any voltage drop > > > issues > > > > (due to limited current) happening with some SDCARDS or during > > > specific > > > > operations (e.g. write). > > > > > > > > Fixes: 660a9763c6a9 (arm64: dts: qcom: db820c: Add pm8994 > > > regulator node) > > > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org > > > <mailto:loic.poulain@linaro.org>> > > > > --- > > > > arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi > > > b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi > > > > index 104cad9..c15e2c0 100644 > > > > --- a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi > > > > +++ b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi > > > > @@ -634,6 +634,8 @@ > > > > l21 { > > > > regulator-min-microvolt = > > > <2950000>; > > > > regulator-max-microvolt = > > > <2950000>; > > > > + regulator-allow-set-load; > > > > + regulator-system-load = > > > <200000>; > > > > }; > > > > l22 { > > > > regulator-min-microvolt = > > > <3300000>; > > > > > > > > > > I'm curious, why not update sdhci-msm to set the load on the regulator? > > > > > > > > > Yes you're right, and I saw that there is ongoing work: > > > https://patchwork.kernel.org/patch/10630731/ > > > > > > Howerver I thought this change would be a quicker fix and easier to > > > backport in stable trees. > > > I assume all the device-tree vmmc loads will be removed at some point > > > when driven from sdhci. > > > > > > > I hadn't seen that. Ok, seems good to me. > > NOTE: I'm personally not convinced that adding the "set_load" calls > into the SDHCI driver actually makes any sense. I believe it adds > complexity for no benefit. The only time you ever need to should ever > be fiddling with "set_load" calls is if the rail you're controlling > has some hope of being able to run at a lower power mode. If there's > no hope of it being at a lower power mode then the constraints on the > rail should just force it to high power mode and be done with it. The > patch here (using regulator-system-load) is one way to force it to a > high power mode and seems fine, but there are other ways. See a > previous discussion [1]. > > NOTE: IIRC the "ongoing work" patch you pointed at always sets the > load to a fixed level to turn it to "high power mode" when the > regulator is turned on and undoes that set_load when the regulator is > turned off. That's no longer needed as of commit 5451781dadf8 > ("regulator: core: Only count load for enabled consumers"). If > someone comes up with a case where it's useful to keep the SD card > rail turned on but in "low power mode" _then_ we should actually > consider adding set_load to the SD card driver. > > [1] https://lkml.kernel.org/r/CAD=FV=V4WFYoKLQ72pico4HCGgLDTae7xougivv6VWOSoPhLpg@mail.gmail.com > > -Doug
On Fri 11 Oct 05:55 PDT 2019, Loic Poulain wrote: > Hi Andy, Rob, > > Could any of you take this patch? > I've applied the patch now. Thanks, Bjorn > On Thu, 13 Dec 2018 at 20:14, Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Thu, Dec 13, 2018 at 6:46 AM Jeffrey Hugo <jhugo@codeaurora.org> wrote: > > > > > > On 12/13/2018 12:55 AM, Loic Poulain wrote: > > > > Hi Jeffrey, > > > > > > > > > > > > On Wed, 12 Dec 2018 at 18:23, Jeffrey Hugo <jhugo@codeaurora.org > > > > <mailto:jhugo@codeaurora.org>> wrote: > > > > > > > > On 12/12/2018 10:13 AM, Loic Poulain wrote: > > > > > In the same way as for msm8974-hammerhead, l21 load, used for SDCARD > > > > > VMMC, needs to be increased in order to prevent any voltage drop > > > > issues > > > > > (due to limited current) happening with some SDCARDS or during > > > > specific > > > > > operations (e.g. write). > > > > > > > > > > Fixes: 660a9763c6a9 (arm64: dts: qcom: db820c: Add pm8994 > > > > regulator node) > > > > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org > > > > <mailto:loic.poulain@linaro.org>> > > > > > --- > > > > > arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi | 2 ++ > > > > > 1 file changed, 2 insertions(+) > > > > > > > > > > diff --git a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi > > > > b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi > > > > > index 104cad9..c15e2c0 100644 > > > > > --- a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi > > > > > +++ b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi > > > > > @@ -634,6 +634,8 @@ > > > > > l21 { > > > > > regulator-min-microvolt = > > > > <2950000>; > > > > > regulator-max-microvolt = > > > > <2950000>; > > > > > + regulator-allow-set-load; > > > > > + regulator-system-load = > > > > <200000>; > > > > > }; > > > > > l22 { > > > > > regulator-min-microvolt = > > > > <3300000>; > > > > > > > > > > > > > I'm curious, why not update sdhci-msm to set the load on the regulator? > > > > > > > > > > > > Yes you're right, and I saw that there is ongoing work: > > > > https://patchwork.kernel.org/patch/10630731/ > > > > > > > > Howerver I thought this change would be a quicker fix and easier to > > > > backport in stable trees. > > > > I assume all the device-tree vmmc loads will be removed at some point > > > > when driven from sdhci. > > > > > > > > > > I hadn't seen that. Ok, seems good to me. > > > > NOTE: I'm personally not convinced that adding the "set_load" calls > > into the SDHCI driver actually makes any sense. I believe it adds > > complexity for no benefit. The only time you ever need to should ever > > be fiddling with "set_load" calls is if the rail you're controlling > > has some hope of being able to run at a lower power mode. If there's > > no hope of it being at a lower power mode then the constraints on the > > rail should just force it to high power mode and be done with it. The > > patch here (using regulator-system-load) is one way to force it to a > > high power mode and seems fine, but there are other ways. See a > > previous discussion [1]. > > > > NOTE: IIRC the "ongoing work" patch you pointed at always sets the > > load to a fixed level to turn it to "high power mode" when the > > regulator is turned on and undoes that set_load when the regulator is > > turned off. That's no longer needed as of commit 5451781dadf8 > > ("regulator: core: Only count load for enabled consumers"). If > > someone comes up with a case where it's useful to keep the SD card > > rail turned on but in "low power mode" _then_ we should actually > > consider adding set_load to the SD card driver. > > > > [1] https://lkml.kernel.org/r/CAD=FV=V4WFYoKLQ72pico4HCGgLDTae7xougivv6VWOSoPhLpg@mail.gmail.com > > > > -Doug
diff --git a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi index 104cad9..c15e2c0 100644 --- a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi +++ b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi @@ -634,6 +634,8 @@ l21 { regulator-min-microvolt = <2950000>; regulator-max-microvolt = <2950000>; + regulator-allow-set-load; + regulator-system-load = <200000>; }; l22 { regulator-min-microvolt = <3300000>;
In the same way as for msm8974-hammerhead, l21 load, used for SDCARD VMMC, needs to be increased in order to prevent any voltage drop issues (due to limited current) happening with some SDCARDS or during specific operations (e.g. write). Fixes: 660a9763c6a9 (arm64: dts: qcom: db820c: Add pm8994 regulator node) Signed-off-by: Loic Poulain <loic.poulain@linaro.org> --- arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi | 2 ++ 1 file changed, 2 insertions(+) -- 2.7.4