2018-09-20 06:26:00

by Veerabhadrarao Badiganti

[permalink] [raw]
Subject: [PATCH V2 2/3] dt-bindings: mmc: sdhci-msm: Add entries for passing load values

From: Vijay Viswanath <[email protected]>

The load a particular sdhc controller should request from a regulator
is device specific and hence each device should individually vote for
the required load.

Signed-off-by: Vijay Viswanath <[email protected]>
Signed-off-by: Veerabhadrarao Badiganti <[email protected]>
---
Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
index 502b3b8..3720385 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
@@ -26,6 +26,11 @@ Required properties:
"cal" - reference clock for RCLK delay calibration (optional)
"sleep" - sleep clock for RCLK delay calibration (optional)

+Optional properties:
+- qcom,<supply>-current-level-microamp - specifies load levels for supply during BUS_ON and
+ BUS_OFF states in power irq. Should be specified in
+ pairs (lpm, hpm), for BUS_OFF and BUS_ON respectively.
+ Units uA.
Example:

sdhc_1: sdhci@f9824900 {
@@ -37,6 +42,7 @@ Example:

vmmc-supply = <&pm8941_l20>;
vqmmc-supply = <&pm8941_s3>;
+ qcom,vqmmc-current-level-microamp = <200 22000>;

pinctrl-names = "default";
pinctrl-0 = <&sdc1_clk &sdc1_cmd &sdc1_data>;
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project



2018-09-21 00:18:43

by Evan Green

[permalink] [raw]
Subject: Re: [PATCH V2 2/3] dt-bindings: mmc: sdhci-msm: Add entries for passing load values

On Wed, Sep 19, 2018 at 11:24 PM Veerabhadrarao Badiganti
<[email protected]> wrote:
>
> From: Vijay Viswanath <[email protected]>
>
> The load a particular sdhc controller should request from a regulator
> is device specific and hence each device should individually vote for
> the required load.
>
> Signed-off-by: Vijay Viswanath <[email protected]>
> Signed-off-by: Veerabhadrarao Badiganti <[email protected]>
> ---
> Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> index 502b3b8..3720385 100644
> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> @@ -26,6 +26,11 @@ Required properties:
> "cal" - reference clock for RCLK delay calibration (optional)
> "sleep" - sleep clock for RCLK delay calibration (optional)
>
> +Optional properties:
> +- qcom,<supply>-current-level-microamp - specifies load levels for supply during BUS_ON and
> + BUS_OFF states in power irq. Should be specified in
> + pairs (lpm, hpm), for BUS_OFF and BUS_ON respectively.
> + Units uA.
> Example:
>
> sdhc_1: sdhci@f9824900 {
> @@ -37,6 +42,7 @@ Example:
>
> vmmc-supply = <&pm8941_l20>;
> vqmmc-supply = <&pm8941_s3>;
> + qcom,vqmmc-current-level-microamp = <200 22000>;
>
> pinctrl-names = "default";
> pinctrl-0 = <&sdc1_clk &sdc1_cmd &sdc1_data>;
> --
> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
>

Aren't the regulator load levels pretty coarse? Would it be safe to
say that pretty much all sd/mmc devices need the high powered mode, or
are there really some devices that can get by with LPM all the time?
-Evan

2018-09-21 10:32:35

by Veerabhadrarao Badiganti

[permalink] [raw]
Subject: Re: [PATCH V2 2/3] dt-bindings: mmc: sdhci-msm: Add entries for passing load values

Hi Evan,


On 9/21/2018 5:45 AM, Evan Green wrote:
> On Wed, Sep 19, 2018 at 11:24 PM Veerabhadrarao Badiganti
> <[email protected]> wrote:
>> From: Vijay Viswanath <[email protected]>
>>
>> The load a particular sdhc controller should request from a regulator
>> is device specific and hence each device should individually vote for
>> the required load.
>>
>> Signed-off-by: Vijay Viswanath <[email protected]>
>> Signed-off-by: Veerabhadrarao Badiganti <[email protected]>
>> ---
>> Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> index 502b3b8..3720385 100644
>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> @@ -26,6 +26,11 @@ Required properties:
>> "cal" - reference clock for RCLK delay calibration (optional)
>> "sleep" - sleep clock for RCLK delay calibration (optional)
>>
>> +Optional properties:
>> +- qcom,<supply>-current-level-microamp - specifies load levels for supply during BUS_ON and
>> + BUS_OFF states in power irq. Should be specified in
>> + pairs (lpm, hpm), for BUS_OFF and BUS_ON respectively.
>> + Units uA.
>> Example:
>>
>> sdhc_1: sdhci@f9824900 {
>> @@ -37,6 +42,7 @@ Example:
>>
>> vmmc-supply = <&pm8941_l20>;
>> vqmmc-supply = <&pm8941_s3>;
>> + qcom,vqmmc-current-level-microamp = <200 22000>;
>>
>> pinctrl-names = "default";
>> pinctrl-0 = <&sdc1_clk &sdc1_cmd &sdc1_data>;
>> --
>> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
>>
> Aren't the regulator load levels pretty coarse? Would it be safe to
> say that pretty much all sd/mmc devices need the high powered mode, or
> are there really some devices that can get by with LPM all the time?
> -Evan
The load levels here are min and max supported by the regulator. To
cover all devices
we do set it to max load. We can't make any assumptions on this,  as
peak current may vary
from device to device.

Thanks,
Veera


2018-09-21 20:09:58

by Evan Green

[permalink] [raw]
Subject: Re: [PATCH V2 2/3] dt-bindings: mmc: sdhci-msm: Add entries for passing load values

On Fri, Sep 21, 2018 at 3:32 AM Veerabhadrarao Badiganti
<[email protected]> wrote:
>
> Hi Evan,
>
>
> On 9/21/2018 5:45 AM, Evan Green wrote:
> > On Wed, Sep 19, 2018 at 11:24 PM Veerabhadrarao Badiganti
> > <[email protected]> wrote:
> >> From: Vijay Viswanath <[email protected]>
> >>
> >> The load a particular sdhc controller should request from a regulator
> >> is device specific and hence each device should individually vote for
> >> the required load.
> >>
> >> Signed-off-by: Vijay Viswanath <[email protected]>
> >> Signed-off-by: Veerabhadrarao Badiganti <[email protected]>
> >> ---
> >> Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 6 ++++++
> >> 1 file changed, 6 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> >> index 502b3b8..3720385 100644
> >> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> >> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> >> @@ -26,6 +26,11 @@ Required properties:
> >> "cal" - reference clock for RCLK delay calibration (optional)
> >> "sleep" - sleep clock for RCLK delay calibration (optional)
> >>
> >> +Optional properties:
> >> +- qcom,<supply>-current-level-microamp - specifies load levels for supply during BUS_ON and
> >> + BUS_OFF states in power irq. Should be specified in
> >> + pairs (lpm, hpm), for BUS_OFF and BUS_ON respectively.
> >> + Units uA.
> >> Example:
> >>
> >> sdhc_1: sdhci@f9824900 {
> >> @@ -37,6 +42,7 @@ Example:
> >>
> >> vmmc-supply = <&pm8941_l20>;
> >> vqmmc-supply = <&pm8941_s3>;
> >> + qcom,vqmmc-current-level-microamp = <200 22000>;
> >>
> >> pinctrl-names = "default";
> >> pinctrl-0 = <&sdc1_clk &sdc1_cmd &sdc1_data>;
> >> --
> >> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
> >>
> > Aren't the regulator load levels pretty coarse? Would it be safe to
> > say that pretty much all sd/mmc devices need the high powered mode, or
> > are there really some devices that can get by with LPM all the time?
> > -Evan
> The load levels here are min and max supported by the regulator. To
> cover all devices
> we do set it to max load. We can't make any assumptions on this, as
> peak current may vary
> from device to device.

Hi Veera,
If it were up to me, I would just assume all devices need high power
mode for BUS_ON and low power mode for BUS_OFF, and skip adding this
binding until you actually came up with a device that needed lower
power mode for BUS_ON, or high power mode for BUS_OFF (when would that
be, anyway?) Are there any actual use cases you've seen that need
different values in here?
-Evan

2018-10-04 13:13:59

by Veerabhadrarao Badiganti

[permalink] [raw]
Subject: Re: [PATCH V2 2/3] dt-bindings: mmc: sdhci-msm: Add entries for passing load values



On 9/22/2018 1:36 AM, Evan Green wrote:
> On Fri, Sep 21, 2018 at 3:32 AM Veerabhadrarao Badiganti
> <[email protected]> wrote:
>> Hi Evan,
>>
>>
>> On 9/21/2018 5:45 AM, Evan Green wrote:
>>> On Wed, Sep 19, 2018 at 11:24 PM Veerabhadrarao Badiganti
>>> <[email protected]> wrote:
>>>> From: Vijay Viswanath <[email protected]>
>>>>
>>>> The load a particular sdhc controller should request from a regulator
>>>> is device specific and hence each device should individually vote for
>>>> the required load.
>>>>
>>>> Signed-off-by: Vijay Viswanath <[email protected]>
>>>> Signed-off-by: Veerabhadrarao Badiganti <[email protected]>
>>>> ---
>>>> Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 6 ++++++
>>>> 1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>>>> index 502b3b8..3720385 100644
>>>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>>>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>>>> @@ -26,6 +26,11 @@ Required properties:
>>>> "cal" - reference clock for RCLK delay calibration (optional)
>>>> "sleep" - sleep clock for RCLK delay calibration (optional)
>>>>
>>>> +Optional properties:
>>>> +- qcom,<supply>-current-level-microamp - specifies load levels for supply during BUS_ON and
>>>> + BUS_OFF states in power irq. Should be specified in
>>>> + pairs (lpm, hpm), for BUS_OFF and BUS_ON respectively.
>>>> + Units uA.
>>>> Example:
>>>>
>>>> sdhc_1: sdhci@f9824900 {
>>>> @@ -37,6 +42,7 @@ Example:
>>>>
>>>> vmmc-supply = <&pm8941_l20>;
>>>> vqmmc-supply = <&pm8941_s3>;
>>>> + qcom,vqmmc-current-level-microamp = <200 22000>;
>>>>
>>>> pinctrl-names = "default";
>>>> pinctrl-0 = <&sdc1_clk &sdc1_cmd &sdc1_data>;
>>>> --
>>>> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
>>>>
>>> Aren't the regulator load levels pretty coarse? Would it be safe to
>>> say that pretty much all sd/mmc devices need the high powered mode, or
>>> are there really some devices that can get by with LPM all the time?
>>> -Evan
>> The load levels here are min and max supported by the regulator. To
>> cover all devices
>> we do set it to max load. We can't make any assumptions on this, as
>> peak current may vary
>> from device to device.
> Hi Veera,
> If it were up to me, I would just assume all devices need high power
> mode for BUS_ON and low power mode for BUS_OFF, and skip adding this
> binding until you actually came up with a device that needed lower
> power mode for BUS_ON, or high power mode for BUS_OFF (when would that
> be, anyway?) Are there any actual use cases you've seen that need
> different values in here?
> -Evan

Hi Evan,
With the present implementation if these load values are not supplied in
dt, its not setting
any load at all. Without setting  load, I'm observing CRC/timeout/tuning
errors.
I can update the code to fallback to default values when these are not
supplied in dt, but these values
will be different from SD card and eMMC devices.
So, this dt binding is needed.

Thanks
Veera