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

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

On 12/05/2013 11:52 AM, Mark Rutland wrote:
> Hi,
>
> Apologies for the late reply.

Hi Mark, thanks for the review!

>
> On Wed, Nov 06, 2013 at 03:56:44PM +0000, Georgi Djakov wrote:
>> This patch adds documentation for Qualcomm SDHCI MSM driver.
>> It contains the differences between the core properties in mmc.txt
>> and the properties used by the sdhci-msm driver.
>
> Nit, but this is documentation of the binding, not the driver.
>

Ok, sure!

>>
>> Signed-off-by: Georgi Djakov <gdjakov@mm-sol.com>
>> ---
>>   .../devicetree/bindings/mmc/sdhci-msm.txt          |   92 ++++++++++++++++++++
>>   1 file changed, 92 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> new file mode 100644
>> index 0000000..8f1a52d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> @@ -0,0 +1,92 @@
>> +* Qualcomm SDHCI controller (sdhci-msm)
>> +
>> +This file documents differences between the core properties in mmc.txt
>> +and the properties used by the sdhci-msm driver.
>> +
>> +Required properties:
>> +- compatible: should contain "qcom,sdhci-msm"
>> +- reg: should contain SDHC, SD Core register map
>
> As this seems to depend on reg-names, it would be nice to write this in
> terms of reg-names (as with clocks and clock-names).
>
>

Ok, thanks!

>> +- reg-names: indicates various resources passed to driver (via reg proptery) by name
>> +	"reg-names" examples are "hc_mem" and "core_mem"
>
> Either there are a fixed set of names you expect (and therefore these
> aren't examples but rather a definition), or this property is useless.
> Please either define the set of names or remove this property.
>
>> +- interrupts: should contain SDHC interrupts
>> +- interrupt-names: indicates interrupts passed to driver (via interrupts property) by name
>> +	"interrupt-names" examples are "hc_irq" and "pwr_irq"
>
> Likewise for both points.
>

Ok!

>> +- vdd-supply: phandle to the regulator for the vdd (flash core voltage) supply.
>> +- vddio-supply: phandle to the regulator for the vdd-io (i/o voltage) supply.
>> +- pinctrl-names: Should contain only one value - "default".
>> +- pinctrl-0: Should specify pin control groups used for this controller.
>> +- clocks: A list of phandle + clock-specifier pairs for the clocks listed in clock-names
>> +- clock-names: Should contain the following:
>> +	"iface" - Main peripheral bus clock (PCLK/HCLK - AHB Bus clock) (required)
>> +	"core"	- SDC MMC clock (MCLK) (required)
>> +	"bus"	- SDCC bus voter clock (optional)
>
> This looks good :)
>
>> +
>> +Optional properties:
>> +- qcom,bus-speed-mode: specifies the supported bus speed modes by host
>> +	The supported bus speed modes are:
>> +	"HS200_1p8v" - indicates that host can support HS200 at 1.8v
>> +	"HS200_1p2v" - indicates that host can support HS200 at 1.2v
>> +	"DDR_1p8v" - indicates that host can support DDR mode at 1.8v
>> +	"DDR_1p2v" - indicates that host can support DDR mode at 1.2v
>
> Is this a list of strings, or does the unit only support one of these?
>
> This could be a set of boolean properties instead, is this likely to
> expand in future?
>

It is meant to be list of strings, and many can be supported. But i will 
drop this property from this initial version, and maybe add it later as 
boolean properties! Thanks!

>> +
>> +- 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.

>
>> +- 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.

Thanks,
Georgi