Messages in this thread | | | Date | Fri, 06 Dec 2013 13:59:46 +0200 | From | Georgi Djakov <> | Subject | Re: [PATCH v7 1/2] mmc: sdhci-msm: Initial SDHCI MSM driver documentation |
| |
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
| |