| Message ID | 1383753405-23631-2-git-send-email-gdjakov@mm-sol.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi, Apologies for the late reply. 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. > > 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). > +- 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. > +- 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? > + > +- qcom,{vdd,vdd-io}-lpm-sup - specifies whether the supply can be kept in low power mode. Boolean? Please specify types on properties. Can you elaborate on what this means? When can a supply not be kept in low power mode? > +- 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? Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 06, 2013 at 11:59:46AM +0000, Georgi Djakov wrote: > 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. 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? 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? 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? Also, I'd recommend splitting them in to separate -min and -max properties, it makes it far clearer what they're actually for. Thanks Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 +- reg-names: indicates various resources passed to driver (via reg proptery) by name + "reg-names" examples are "hc_mem" and "core_mem" +- 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" +- 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) + +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 + +- qcom,{vdd,vdd-io}-lpm-sup - specifies whether the supply can be kept in low power mode. +- 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. + +Example: + + aliases { + sdhc1 = &sdhc_1; + sdhc2 = &sdhc_2; + }; + + sdhc_1: qcom,sdhc@f9824900 { + compatible = "qcom,sdhci-msm"; + reg = <0xf9824900 0x11c>, <0xf9824000 0x800>; + reg-names = "hc_mem", "core_mem"; + interrupts = <0 123 0>, <0 138 0>; + interrupt-names = "hc_irq", "pwr_irq"; + bus-width = <8>; + non-removable; + + vdd-supply = <&pm8941_l20>; + qcom,vdd-io-lpm-sup; + qcom,vdd-voltage-level = <2950000 2950000>; + qcom,vdd-current-level = <800 500000>; + + vdd-io-supply = <&pm8941_s3>; + qcom,vdd-io-voltage-level = <1800000 1800000>; + qcom,vdd-io-current-level = <250 154000>; + qcom,bus-speed-mode = "HS200_1p8v", "DDR_1p8v"; + + pinctrl-names = "default" + pinctrl-0 = <&sdc1_clk &sdc1_cmd &sdc1_data>; + + clocks = <&gcc GCC_SDCC1_APPS_CLK>, <&gcc GCC_SDCC1_AHB_CLK>; + clock-names = "iface", "core"; + }; + + sdhc_2: sdhci@f98a4900 { + compatible = "qcom,sdhci-msm"; + reg = <0xf98a4900 0x11c>, <0xf98a4000 0x800>; + reg-names = "hc_mem", "core_mem"; + interrupts = <0 125 0>, <0 221 0>; + interrupt-names = "hc_irq", "pwr_irq"; + bus-width = <4>; + + vdd-supply = <&pm8941_l21>; + qcom,vdd-voltage-level = <2950000 2950000>; + qcom,vdd-current-level = <9000 800000>; + + vdd-io-supply = <&pm8941_l13>; + qcom,vdd-io-voltage-level = <1800000 2950000>; + qcom,vdd-io-current-level = <6 22000>; + + pinctrl-names = "default"; + pinctrl-0 = <&sdc2_clk &sdc2_cmd &sdc2_data>; + + clocks = <&gcc GCC_SDCC2_APPS_CLK>, <&gcc GCC_SDCC2_AHB_CLK>; + clock-names = "core", "iface"; + };
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. 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