Re: [PATCH RFC v3] mmc: sdhci-msm: Add support for MSM chipsets
Hi Jaehoon, On 08/27/2013 11:55 AM, Jaehoon Chung wrote: Hi Georgi, I found the sdhci_msm_vreg_reset(). Why do you run enable->disable? +/* + * Reset vreg by ensuring it is off during probe. A call + * to enable vreg is needed to balance disable vreg + */ +static int sdhci_msm_vreg_reset(struct sdhci_msm_pltfm_data *pdata) I think that controller didn't have responsibility to ensure whether power is enabled or not at probing time. If just needed to balance vreg, then i think this function can be removed. How about? Also before using regulator_is_enabled(), i had found the hole for balancing the vreg. Thank you! I'll remove this function. BR, Georgi -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v3] mmc: sdhci-msm: Add support for MSM chipsets
Hi Ivan,
On 08/23/2013 10:34 AM, Ivan T. Ivanov wrote:
Hi Georgi,
On Tue, 2013-08-20 at 19:44 +0300, Georgi Djakov wrote:
This platform driver adds the support of Secure Digital Host
Controller Interface compliant controller in MSM chipsets.
CC: Asutosh Das
CC: Venkat Gopalakrishnan
CC: Sahitya Tummala
CC: Subhash Jadavani
Signed-off-by: Georgi Djakov
---
Changes from v2:
- Added DT bindings for clocks
- Moved voltage regulators data to platform data
- Removed unneeded includes
- Removed obsolete and wrapper functions
- Removed error checking where unnecessary
- Removed redundant _clk suffix from clock names
- Just return instead of goto where possible
- Minor fixes
Is this version intermediate step before you address
all previous comments?
+
+static const struct of_device_id sdhci_msm_dt_match[] = {
+ {.compatible = "qcom,sdhci-msm"},
Missing termination entry
+};
+
+MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
+
+static int sdhci_msm_probe(struct platform_device *pdev)
+{
+ struct sdhci_host *host;
+ struct sdhci_pltfm_host *pltfm_host;
+ struct sdhci_msm_host *msm_host;
+ struct resource *core_memres = NULL;
+ int ret = 0, dead = 0;
+ struct pinctrl *pinctrl;
+
+ msm_host = devm_kzalloc(&pdev->dev, sizeof(struct sdhci_msm_host),
+ GFP_KERNEL);
+ if (!msm_host)
+ return -ENOMEM;
+
+ host = sdhci_pltfm_init(pdev, &msm_host->sdhci_msm_pdata, 0);
+ if (IS_ERR(host)) {
+ dev_err(mmc_dev(host->mmc), "sdhci_pltfm_init error\n");
+ return PTR_ERR(host);
+ }
+
+ pltfm_host = sdhci_priv(host);
+ pltfm_host->priv = msm_host;
+ msm_host->mmc = host->mmc;
+ msm_host->pdev = pdev;
+
+ ret = mmc_of_parse(host->mmc);
+ if (ret)
Shouldn't sdhci_pltfm_init operation be reverted?
+ return ret;
+
+ /* Extract platform data */
+ if (pdev->dev.of_node) {
+ msm_host->pdata = sdhci_msm_populate_pdata(&pdev->dev);
+ if (!msm_host->pdata) {
+ dev_err(&pdev->dev, "DT parsing error\n");
+ goto pltfm_free;
+ }
+ } else {
+ dev_err(&pdev->dev, "No device tree node\n");
+ goto pltfm_free;
+ }
+
+ /* Setup pins */
+ pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+ if (IS_ERR(pinctrl))
+ dev_warn(&pdev->dev, "pins are not configured by the driver\n");
+
+ /* Setup Clocks */
+
+ /* Setup SDCC bus voter clock. */
+ msm_host->bus_clk = devm_clk_get(&pdev->dev, "bus");
+ if (!IS_ERR_OR_NULL(msm_host->bus_clk)) {
+ /* Vote for max. clk rate for max. performance */
+ ret = clk_set_rate(msm_host->bus_clk, INT_MAX);
+ if (ret)
+ goto pltfm_free;
+ ret = clk_prepare_enable(msm_host->bus_clk);
+ if (ret)
+ goto pltfm_free;
+ }
+
+ /* Setup main peripheral bus clock */
+ msm_host->pclk = devm_clk_get(&pdev->dev, "iface");
+ if (!IS_ERR(msm_host->pclk)) {
+ ret = clk_prepare_enable(msm_host->pclk);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "Main peripheral clock setup failed (%d)\n",
+ ret);
+ goto bus_clk_disable;
+ }
+ }
+
+ /* Setup SDC MMC clock */
+ msm_host->clk = devm_clk_get(&pdev->dev, "core");
+ if (IS_ERR(msm_host->clk)) {
+ ret = PTR_ERR(msm_host->clk);
+ dev_err(&pdev->dev, "SDC MMC clock setup failed (%d)\n", ret);
+ goto pclk_disable;
+ }
+
+ ret = clk_prepare_enable(msm_host->clk);
+ if (ret)
+ goto pclk_disable;
+
+ /* Setup regulators */
+ ret = sdhci_msm_vreg_init(&pdev->dev, msm_host->pdata, true);
+ if (ret) {
+ dev_err(&pdev->dev, "Regulator setup failed (%d)\n", ret);
+ goto clk_disable;
+ }
+
+ /* Reset the core and Enable SDHC mode */
+ core_memres = platform_get_resource_byname(pdev,
+ IORESOURCE_MEM, "core_mem");
+ msm_host->core_mem = devm_ioremap(&pdev->dev, core_memres->start,
+ resource_size(core_memres));
+
+ if (!msm_host->core_mem) {
+ dev_err(&pdev->dev, "Failed to remap registers\n");
+ ret = -ENOMEM;
+ goto vreg_deinit;
+ }
+
+ /* Set SW_RST bit in POWER register (Offset 0x0) */
+ writel_relaxed(CORE_SW_RST, msm_host->core_mem + CORE_POWER);
+ /* Set HC_MODE_EN bit in HC_MODE register */
+ writel_relaxed(HC_MODE_EN, (msm_host->core_mem + CORE_HC_MODE));
+
+ /*
+* Following are the
Re: [PATCH RFC v3] mmc: sdhci-msm: Add support for MSM chipsets
Hi Georgi,
I found the sdhci_msm_vreg_reset(). Why do you run enable->disable?
+/*
+ * Reset vreg by ensuring it is off during probe. A call
+ * to enable vreg is needed to balance disable vreg
+ */
+static int sdhci_msm_vreg_reset(struct sdhci_msm_pltfm_data *pdata)
I think that controller didn't have responsibility to ensure whether power is
enabled or not at probing time.
If just needed to balance vreg, then i think this function can be removed.
How about?
Also before using regulator_is_enabled(), i had found the hole for balancing
the vreg.
Best Regards,
Jaehoon Chung
On 08/21/2013 01:44 AM, Georgi Djakov wrote:
> This platform driver adds the support of Secure Digital Host
> Controller Interface compliant controller in MSM chipsets.
>
> CC: Asutosh Das
> CC: Venkat Gopalakrishnan
> CC: Sahitya Tummala
> CC: Subhash Jadavani
> Signed-off-by: Georgi Djakov
> ---
> Changes from v2:
> - Added DT bindings for clocks
> - Moved voltage regulators data to platform data
> - Removed unneeded includes
> - Removed obsolete and wrapper functions
> - Removed error checking where unnecessary
> - Removed redundant _clk suffix from clock names
> - Just return instead of goto where possible
> - Minor fixes
>
> Changes from v1:
> - GPIO references are replaced by pinctrl
> - DT parsing is done mostly by mmc_of_parse()
> - Use of_match_device() for DT matching
> - A few minor changes
>
> .../devicetree/bindings/mmc/sdhci-msm.txt | 71 ++
> drivers/mmc/host/Kconfig | 13 +
> drivers/mmc/host/Makefile |1 +
> drivers/mmc/host/sdhci-msm.c | 687
>
> 4 files changed, 772 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> create mode 100644 drivers/mmc/host/sdhci-msm.c
>
> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> new file mode 100644
> index 000..ee112da
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> @@ -0,0 +1,71 @@
> +* 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 be "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"
> +- -supply: phandle to the regulator device tree node
> + "supply-name" examples are "vdd" and "vdd-io"
> +- pinctrl-names: Should contain only one value - "default".
> +- pinctrl-0: Should specify pin control groups used for this controller.
> +- clocks: phandles to clock instances of the device tree nodes
> +- clock-names:
> + 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 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
> +
> +In the following, can be vdd (flash core voltage) or vdd-io (I/O
> voltage).
> +- qcom,-always-on - specifies whether supply should be kept "on"
> always.
> +- qcom,-lpm-sup - specifies whether supply can be kept in low power
> mode (lpm).
> +- qcom,-voltage-level - specifies voltage levels for supply. Should
> be
> +specified in pairs (min, max), units uV.
> +- qcom,-current-level - specifies load levels for supply in lpm or
> high power mode
> + (hpm). Should be specified in pairs (lpm, hpm), units uA.
> +
> +Example:
> +
> + aliases {
> + sdhc1 = &sdhc_1;
> + };
> +
> + 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 = <4>;
> + non-removable;
> +
> + vdd-supply = <&pm8941_l21>;
> + vdd-io-supply = <&pm8941_l13>;
> + qcom,vdd-voltage-level = <295 295>;
> + qcom,vdd-current-level = <9000 80>;
> + qcom,vdd-io-always-on;
> + qcom,vdd-io-lpm-sup;
> + qcom,vdd-io-voltage-le
Re: [PATCH RFC v3] mmc: sdhci-msm: Add support for MSM chipsets
Hi Georgi,
On Tue, 2013-08-20 at 19:44 +0300, Georgi Djakov wrote:
> This platform driver adds the support of Secure Digital Host
> Controller Interface compliant controller in MSM chipsets.
>
> CC: Asutosh Das
> CC: Venkat Gopalakrishnan
> CC: Sahitya Tummala
> CC: Subhash Jadavani
> Signed-off-by: Georgi Djakov
> ---
> Changes from v2:
> - Added DT bindings for clocks
> - Moved voltage regulators data to platform data
> - Removed unneeded includes
> - Removed obsolete and wrapper functions
> - Removed error checking where unnecessary
> - Removed redundant _clk suffix from clock names
> - Just return instead of goto where possible
> - Minor fixes
>
Is this version intermediate step before you address
all previous comments?
> +
> +static const struct of_device_id sdhci_msm_dt_match[] = {
> + {.compatible = "qcom,sdhci-msm"},
Missing termination entry
> +};
> +
> +MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
> +
> +static int sdhci_msm_probe(struct platform_device *pdev)
> +{
> + struct sdhci_host *host;
> + struct sdhci_pltfm_host *pltfm_host;
> + struct sdhci_msm_host *msm_host;
> + struct resource *core_memres = NULL;
> + int ret = 0, dead = 0;
> + struct pinctrl *pinctrl;
> +
> + msm_host = devm_kzalloc(&pdev->dev, sizeof(struct sdhci_msm_host),
> + GFP_KERNEL);
> + if (!msm_host)
> + return -ENOMEM;
> +
> + host = sdhci_pltfm_init(pdev, &msm_host->sdhci_msm_pdata, 0);
> + if (IS_ERR(host)) {
> + dev_err(mmc_dev(host->mmc), "sdhci_pltfm_init error\n");
> + return PTR_ERR(host);
> + }
> +
> + pltfm_host = sdhci_priv(host);
> + pltfm_host->priv = msm_host;
> + msm_host->mmc = host->mmc;
> + msm_host->pdev = pdev;
> +
> + ret = mmc_of_parse(host->mmc);
> + if (ret)
Shouldn't sdhci_pltfm_init operation be reverted?
> + return ret;
> +
> + /* Extract platform data */
> + if (pdev->dev.of_node) {
> + msm_host->pdata = sdhci_msm_populate_pdata(&pdev->dev);
> + if (!msm_host->pdata) {
> + dev_err(&pdev->dev, "DT parsing error\n");
> + goto pltfm_free;
> + }
> + } else {
> + dev_err(&pdev->dev, "No device tree node\n");
> + goto pltfm_free;
> + }
> +
> + /* Setup pins */
> + pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
> + if (IS_ERR(pinctrl))
> + dev_warn(&pdev->dev, "pins are not configured by the driver\n");
> +
> + /* Setup Clocks */
> +
> + /* Setup SDCC bus voter clock. */
> + msm_host->bus_clk = devm_clk_get(&pdev->dev, "bus");
> + if (!IS_ERR_OR_NULL(msm_host->bus_clk)) {
> + /* Vote for max. clk rate for max. performance */
> + ret = clk_set_rate(msm_host->bus_clk, INT_MAX);
> + if (ret)
> + goto pltfm_free;
> + ret = clk_prepare_enable(msm_host->bus_clk);
> + if (ret)
> + goto pltfm_free;
> + }
> +
> + /* Setup main peripheral bus clock */
> + msm_host->pclk = devm_clk_get(&pdev->dev, "iface");
> + if (!IS_ERR(msm_host->pclk)) {
> + ret = clk_prepare_enable(msm_host->pclk);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "Main peripheral clock setup failed (%d)\n",
> + ret);
> + goto bus_clk_disable;
> + }
> + }
> +
> + /* Setup SDC MMC clock */
> + msm_host->clk = devm_clk_get(&pdev->dev, "core");
> + if (IS_ERR(msm_host->clk)) {
> + ret = PTR_ERR(msm_host->clk);
> + dev_err(&pdev->dev, "SDC MMC clock setup failed (%d)\n", ret);
> + goto pclk_disable;
> + }
> +
> + ret = clk_prepare_enable(msm_host->clk);
> + if (ret)
> + goto pclk_disable;
> +
> + /* Setup regulators */
> + ret = sdhci_msm_vreg_init(&pdev->dev, msm_host->pdata, true);
> + if (ret) {
> + dev_err(&pdev->dev, "Regulator setup failed (%d)\n", ret);
> + goto clk_disable;
> + }
> +
> + /* Reset the core and Enable SDHC mode */
> + core_memres = platform_get_resource_byname(pdev,
> +IORESOURCE_MEM, "core_mem");
> + msm_host->core_mem = devm_ioremap(&pdev->dev, core_memres->start,
> + resource_size(core_memres));
> +
> + if (!msm_host->core_mem) {
> + dev_err(&pdev->dev, "Failed to remap registers\n");
> + ret = -ENOMEM;
> + goto vreg_deinit;
> + }
> +
> + /* Set SW_RST bit in POWER register (Offset 0x0) */
> + writel_relaxed(CORE_SW_RST, msm_host->core_mem + CORE_POWER);
> + /* Set HC_MODE_EN bit in HC_MODE register */
> + writel_relaxed(HC_MODE_EN, (msm_host->core_mem + CORE_HC_MODE));
> +
> + /*
> +
[PATCH RFC v3] mmc: sdhci-msm: Add support for MSM chipsets
This platform driver adds the support of Secure Digital Host
Controller Interface compliant controller in MSM chipsets.
CC: Asutosh Das
CC: Venkat Gopalakrishnan
CC: Sahitya Tummala
CC: Subhash Jadavani
Signed-off-by: Georgi Djakov
---
Changes from v2:
- Added DT bindings for clocks
- Moved voltage regulators data to platform data
- Removed unneeded includes
- Removed obsolete and wrapper functions
- Removed error checking where unnecessary
- Removed redundant _clk suffix from clock names
- Just return instead of goto where possible
- Minor fixes
Changes from v1:
- GPIO references are replaced by pinctrl
- DT parsing is done mostly by mmc_of_parse()
- Use of_match_device() for DT matching
- A few minor changes
.../devicetree/bindings/mmc/sdhci-msm.txt | 71 ++
drivers/mmc/host/Kconfig | 13 +
drivers/mmc/host/Makefile |1 +
drivers/mmc/host/sdhci-msm.c | 687
4 files changed, 772 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-msm.txt
create mode 100644 drivers/mmc/host/sdhci-msm.c
diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
new file mode 100644
index 000..ee112da
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
@@ -0,0 +1,71 @@
+* 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 be "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"
+- -supply: phandle to the regulator device tree node
+ "supply-name" examples are "vdd" and "vdd-io"
+- pinctrl-names: Should contain only one value - "default".
+- pinctrl-0: Should specify pin control groups used for this controller.
+- clocks: phandles to clock instances of the device tree nodes
+- clock-names:
+ 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 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
+
+In the following, can be vdd (flash core voltage) or vdd-io (I/O
voltage).
+- qcom,-always-on - specifies whether supply should be kept "on"
always.
+- qcom,-lpm-sup - specifies whether supply can be kept in low power
mode (lpm).
+- qcom,-voltage-level - specifies voltage levels for supply. Should be
+specified in pairs (min, max), units uV.
+- qcom,-current-level - specifies load levels for supply in lpm or
high power mode
+ (hpm). Should be specified in pairs (lpm, hpm), units uA.
+
+Example:
+
+ aliases {
+ sdhc1 = &sdhc_1;
+ };
+
+ 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 = <4>;
+ non-removable;
+
+ vdd-supply = <&pm8941_l21>;
+ vdd-io-supply = <&pm8941_l13>;
+ qcom,vdd-voltage-level = <295 295>;
+ qcom,vdd-current-level = <9000 80>;
+ qcom,vdd-io-always-on;
+ qcom,vdd-io-lpm-sup;
+ qcom,vdd-io-voltage-level = <180 295>;
+ qcom,vdd-io-current-level = <6 22000>;
+ qcom,bus-speed-mode = "HS200_1p8v", "DDR_1p8v";
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&sdc1_clk &sdc1_cmd &sdc1_data>;
+
+ clocks = <&iface>, <&core>, <&bus>;
+ clock-names = "iface", "core", "bus";
+
+ };
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 8a4c066..2b31471 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -322,6 +322,19 @@ config MMC_ATMELMCI
If unsure, say N.
+config MMC_SDHCI_MSM
+ tristate "Qualcomm SDHCI Controller Support"
+ depends on ARCH_MSM
+ depends on MMC_SDHCI_PLTFM
+ help
+ This selects the Secure Digit
