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

[Georgi Djakov]

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@xxxxxxxxxx>
> > ---
> >   .../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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/