Re: [PATCH v7 1/2] mmc: sdhci-msm: Initial SDHCI MSM driver documentation

[Georgi Djakov <gdjakov@mm-sol.com>]

Hi,

Apologies for the delayed reply.

On 12/09/2013 11:38 AM, Mark Rutland wrote:
> On Fri, Dec 06, 2013 at 11:59:46AM +0000, Georgi Djakov wrote:
>> On 12/05/2013 11:52 AM, Mark Rutland wrote:
[...]
>
>>>> +
>>>> +- qcom,{vdd,vdd-io}-lpm-sup - specifies whether the supply can be kept in low power mode.
>>>
>>> Boolean? Please specify types on properties.
>>
>> Yes, it is boolean. I'll note it in the documentation. Thanks!
>>
>>>
>>> Can you elaborate on what this means? When can a supply not be kept in
>>> low power mode?
>>
>> The vdd-io for example is a regulator that is always-on and may be
>> shared with multiple other peripherals as well. It should not be
>> disabled by the driver, but instead put in low power mode when unused.
>
> The fact that the regulator is driving other peripherals doesn't seem
> like a property of the SDHCI to me. What are these other peripherals?
>

Agree! I'll drop this property.

> When you say should not be disabled by the driver, do you mean that
> another peripheral's driver shouldn't be able to disable the regulator
> feeding the SDHCI, or the SDHCI driver shouldn't be able to disable a
> regulator in use by another peripheral?
>

The regulator will not be disabled in any case as it will be marked as 
always-on.

> When in low power mode, is the SDHCI functional?
>
>>>
>>>> +- qcom,{vdd,vdd-io}-voltage-level - specifies voltage levels for the supply.
>>>> +	Should be specified in pairs (min, max), units uV.
>>>> +- qcom,{vdd,vdd-io}-current-level - specifies load levels for the supply in lpm
>>>> +	or high power mode (hpm). Should be specified in pairs (lpm, hpm), units uA.
>>>
>>> Can you not query these from the regulator framework?
>>>
>>> If these are configuration, why are they necessary?
>>>
>>
>> As some regulators are shared and can have multiple consumers, these
>> properties can be used for voltage and load current aggregation.
>> The voltage-level is the "supported voltage" by the controller, that
>> also (at least on my platform) matches the corresponding regulator
>> voltage. I probably can drop the voltage-level property and get voltage
>> via the regulator framework helper functions, but the load current
>> values are different for each sdhc.
>>   From the very limited documentation that i have, i think this is
>> describing the hardware configuration and should be in the device-tree.
>
> If these are the voltages / currents supported by the SDHCI, then that
> seems like it makes sense to have in DT, if they're likely to be
> variable in practice. How variable do you expect these to be?
>

The voltages vary depending on the function. For example the vdd-io for 
eMMC is 1.2 - 1.8v, for SD cards 1.8 - 2.95v and for SDIO 1.8v.
So, one way is using -min/-max suffixes and the other can be introducing 
a for ex. "function" property (emmc/card/sdio) and moving the voltage 
range definitions into the driver.

> Also, I'd recommend splitting them in to separate -min and -max
> properties, it makes it far clearer what they're actually for.
>

Thanks,
Georgi