Re: [PATCH RFC v3] mmc: sdhci-msm: Add support for MSM chipsets

2013-09-05 Thread Georgi Djakov 

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 asuto...@codeaurora.org
CC: Venkat Gopalakrishnan venk...@codeaurora.org
CC: Sahitya Tummala stumm...@codeaurora.org
CC: Subhash Jadavani subha...@codeaurora.org
Signed-off-by: Georgi Djakov gdja...@mm-sol.com
---
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));
+
+   /*
+

Re: [PATCH RFC v3] mmc: sdhci-msm: Add support for MSM chipsets

2013-09-05 Thread Georgi Djakov 

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

2013-08-27 Thread Jaehoon Chung 
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 asuto...@codeaurora.org
 CC: Venkat Gopalakrishnan venk...@codeaurora.org
 CC: Sahitya Tummala stumm...@codeaurora.org
 CC: Subhash Jadavani subha...@codeaurora.org
 Signed-off-by: Georgi Djakov gdja...@mm-sol.com
 ---
 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-name-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, supply can be vdd (flash core voltage) or vdd-io (I/O 
 voltage).
 +- qcom,supply-always-on - specifies whether supply should be kept on 
 always.
 +- qcom,supply-lpm-sup - specifies whether supply can be kept in low power 
 mode (lpm).
 +- qcom,supply-voltage-level - specifies voltage levels for supply. Should 
 be
 +specified in pairs (min, max), units uV.
 +- qcom,supply-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;

Re: [PATCH RFC v3] mmc: sdhci-msm: Add support for MSM chipsets

2013-08-23 Thread Ivan T. Ivanov 
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 asuto...@codeaurora.org
 CC: Venkat Gopalakrishnan venk...@codeaurora.org
 CC: Sahitya Tummala stumm...@codeaurora.org
 CC: Subhash Jadavani subha...@codeaurora.org
 Signed-off-by: Georgi Djakov gdja...@mm-sol.com
 ---
 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?

snip

 +
 +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 deviations from SDHC spec v3.0 -
 +  * 1. Card detection is handled using