[PATCH v7 0/2] mmc: sdhci-msm: Add support for MSM chipsets

[PATCH v7 0/2] mmc: sdhci-msm: Add support for MSM chipsets

[Georgi Djakov]

This patchset adds basic support of the Secure Digital Host Controller
Interface compliant controller found in Qualcomm MSM chipsets.

Tested with SD card on APQ8074 Dragonboard.

Patchset applies to mmc-next, as it depends on:
4525181 mmc: sdhci: add hooks for platform specific tuning

Changes from v6:
- Fixed wrong pointer in sdhci_msm_pwr_irq().
- Added platform_execute_tuning() callback as the MSM SDHC does not
  support tuning as in SDHC 3.0 spec and will need custom implementation
  in order to support SDR104, HS200 and HS400.
- Removed the always-on devicetree property - if the regulator is
  configured as always-on, it will not be disabled anyway.
- Removed devm_pinctrl_get_select_default() - the default pins are 
  already set from the device core.
- Removed wrapper function sdhci_msm_set_vdd_io_vol() and enum
  vdd_io_level and now calling regulator_set_voltage() directly.
- Converted #defines to use BIT() macro.
- Added IS_ERR(vreg->reg) check at the beginning of sdhci_msm_vreg
  functions.
- Do not print errors when regulators init return -EPROBE_DEFER as the
  deffered init is not an actual error.
- Handle each power irq status bit separately in sdhci_msm_pwr_irq().
- Ensure that any pending power irq is acknowledged before enabling it,
  otherwise the irq handler will be fired prematurely.
- Minor changes.

Changes from v5:
- Driver is split into multiple patches
- Do not initialize variables that are assigned later in code
- Remove some useless comments
- Use shorter variable names
- Change pr_err() to dev_err()
- Optimized sdhci_msm_setup_vreg()
- Some code alignment fixes
- Improved DT values sanity check
- Added dev_dbg print for sdhci controller version in probe()
- Added usleep_range() after SW reset - it can take some time
- Added SDHCI_QUIRK_SINGLE_POWER_WRITE - power handled by PMIC
- Renamed DT property vdd-io to vddio

Changes from v4:
- Simplified sdhci_msm_vreg_disable() and sdhci_msm_set_vdd_io_vol()
- Use devm_ioremap_resource() instead of devm_ioremap()
- Converted IS_ERR_OR_NULL to IS_ERR
- Disable regulators in sdhci_msm_remove()
- Check for DT node at the beginning in sdhci_msm_probe()
- Removed more redundant code
- Changes in some error messages
- Minor fixes

Changes from v3:
- Allocate memory for all required structs at once
- Added termination entry in sdhci_msm_dt_match[]
- Fixed a missing sdhci_pltfm_free() in probe()
- Removed redundant of_match_ptr
- Removed the unneeded function sdhci_msm_vreg_reset()

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

Georgi Djakov (2):
  mmc: sdhci-msm: Initial SDHCI MSM driver documentation
  mmc: sdhci-msm: Initial support for MSM chipsets

 .../devicetree/bindings/mmc/sdhci-msm.txt          |   93 +++
 drivers/mmc/host/Kconfig                           |   13 +
 drivers/mmc/host/Makefile                          |    1 +
 drivers/mmc/host/sdhci-msm.c                       |  655 ++++++++++++++++++++
 4 files changed, 762 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-msm.txt
 create mode 100644 drivers/mmc/host/sdhci-msm.c

-- 
1.7.9.5

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

[Georgi Djakov]

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

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";
+	};
-- 
1.7.9.5

[PATCH v7 2/2] mmc: sdhci-msm: Initial support for MSM chipsets

[Georgi Djakov]

This platform driver adds the initial support of Secure
Digital Host Controller Interface compliant controller
found in Qualcomm MSM chipsets.

Signed-off-by: Georgi Djakov <gdjakov@mm-sol.com>
---
 drivers/mmc/host/Kconfig     |   13 +
 drivers/mmc/host/Makefile    |    1 +
 drivers/mmc/host/sdhci-msm.c |  651 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 665 insertions(+)
 create mode 100644 drivers/mmc/host/sdhci-msm.c

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 7fc5099..e8da488 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 Digital Host Controller Interface (SDHCI)
+	  support present in MSM SOCs from Qualcomm. The controller
+	  supports SD/MMC/SDIO devices.
+
+	  If you have a controller with this interface, say Y or M here.
+
+	  If unsure, say N.
+
 config MMC_MSM
 	tristate "Qualcomm SDCC Controller Support"
 	depends on MMC && ARCH_MSM
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index c41d0c3..5faed14 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -61,6 +61,7 @@ obj-$(CONFIG_MMC_SDHCI_OF_ESDHC)	+= sdhci-of-esdhc.o
 obj-$(CONFIG_MMC_SDHCI_OF_HLWD)		+= sdhci-of-hlwd.o
 obj-$(CONFIG_MMC_SDHCI_BCM_KONA)	+= sdhci-bcm-kona.o
 obj-$(CONFIG_MMC_SDHCI_BCM2835)		+= sdhci-bcm2835.o
+obj-$(CONFIG_MMC_SDHCI_MSM)		+= sdhci-msm.o
 
 ifeq ($(CONFIG_CB710_DEBUG),y)
 	CFLAGS-cb710-mmc	+= -DDEBUG
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
new file mode 100644
index 0000000..57eba64
--- /dev/null
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -0,0 +1,651 @@
+/*
+ * drivers/mmc/host/sdhci-msm.c - Qualcomm MSM SDHCI Platform
+ * driver source file
+ *
+ * Copyright (c) 2013, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/delay.h>
+
+#include "sdhci-pltfm.h"
+
+#define CORE_HC_MODE		0x78
+#define HC_MODE_EN		0x1
+
+#define CORE_POWER		0x0
+#define CORE_SW_RST		BIT(7)
+
+#define CORE_PWRCTL_STATUS	0xdc
+#define CORE_PWRCTL_MASK	0xe0
+#define CORE_PWRCTL_CLEAR	0xe4
+#define CORE_PWRCTL_CTL		0xe8
+
+#define CORE_PWRCTL_BUS_OFF	BIT(0)
+#define CORE_PWRCTL_BUS_ON	BIT(1)
+#define CORE_PWRCTL_IO_LOW	BIT(2)
+#define CORE_PWRCTL_IO_HIGH	BIT(3)
+
+#define CORE_PWRCTL_BUS_SUCCESS	BIT(0)
+#define CORE_PWRCTL_BUS_FAIL	BIT(1)
+#define CORE_PWRCTL_IO_SUCCESS	BIT(2)
+#define CORE_PWRCTL_IO_FAIL	BIT(3)
+
+#define INT_MASK		0xf
+
+
+/* This structure keeps information per regulator */
+struct sdhci_msm_reg_data {
+	struct regulator *reg;
+	const char *name;
+	/* Voltage level values */
+	u32 low_vol_level;
+	u32 high_vol_level;
+	/* Load values for low power and high power mode */
+	u32 lpm_uA;
+	u32 hpm_uA;
+	/* Regulator should be kept in low power mode */
+	bool lpm_sup;
+};
+
+struct sdhci_msm_pltfm_data {
+	u32 caps;				/* Supported UHS-I Modes */
+	u32 caps2;				/* More capabilities */
+	struct sdhci_msm_reg_data vdd;		/* VDD/VCC regulator info */
+	struct sdhci_msm_reg_data vdd_io;	/* VDD IO regulator info */
+};
+
+struct sdhci_msm_host {
+	struct platform_device *pdev;
+	void __iomem *core_mem;	/* MSM SDCC mapped address */
+	int pwr_irq;		/* power irq */
+	struct clk *clk;	/* main SD/MMC bus clock */
+	struct clk *pclk;	/* SDHC peripheral bus clock */
+	struct clk *bus_clk;	/* SDHC bus voter clock */
+	struct sdhci_msm_pltfm_data pdata;
+	struct mmc_host *mmc;
+	struct sdhci_pltfm_data sdhci_msm_pdata;
+};
+
+/* MSM platform specific tuning */
+int sdhci_msm_execute_tuning(struct sdhci_host *host, u32 opcode)
+{
+	return 0;
+}
+
+#define MAX_PROP_SIZE 32
+static int sdhci_msm_dt_parse_vreg_info(struct device *dev,
+					struct sdhci_msm_reg_data *vreg,
+					const char *vreg_name)
+{
+	int len;
+	const __be32 *prop;
+	char prop_name[MAX_PROP_SIZE];
+	struct device_node *np = dev->of_node;
+
+	snprintf(prop_name, MAX_PROP_SIZE, "%s-supply", vreg_name);
+	if (!of_parse_phandle(np, prop_name, 0)) {
+		dev_info(dev, "No vreg data found for %s\n", vreg_name);
+		return -EINVAL;
+	}
+
+	vreg->name = vreg_name;
+
+	snprintf(prop_name, MAX_PROP_SIZE, "qcom,%s-lpm-sup", vreg_name);
+	if (of_get_property(np, prop_name, NULL))
+		vreg->lpm_sup = true;
+
+	snprintf(prop_name, MAX_PROP_SIZE, "qcom,%s-voltage-level", vreg_name);
+	prop = of_get_property(np, prop_name, &len);
+	if (!prop || (len != (2 * sizeof(__be32)))) {
+		dev_warn(dev, "%s %s property\n",
+		prop ? "invalid format" : "no", prop_name);
+	} else {
+		vreg->low_vol_level = be32_to_cpup(&prop[0]);
+		vreg->high_vol_level = be32_to_cpup(&prop[1]);
+	}
+
+	snprintf(prop_name, MAX_PROP_SIZE, "qcom,%s-current-level", vreg_name);
+	prop = of_get_property(np, prop_name, &len);
+	if (!prop || (len != (2 * sizeof(__be32)))) {
+		dev_warn(dev, "%s %s property\n",
+			 prop ? "invalid format" : "no", prop_name);
+	} else {
+		vreg->lpm_uA = be32_to_cpup(&prop[0]);
+		vreg->hpm_uA = be32_to_cpup(&prop[1]);
+	}
+
+	dev_dbg(dev, "%s: %svol=[%d %d]uV, curr=[%d %d]uA\n", vreg->name,
+		vreg->lpm_sup ? "lpm_sup, " : "", vreg->low_vol_level,
+		vreg->high_vol_level, vreg->lpm_uA, vreg->hpm_uA);
+
+	return 0;
+}
+
+/* Parse devicetree data */
+static int sdhci_msm_populate_pdata(struct device *dev,
+					struct sdhci_msm_pltfm_data *pdata)
+{
+	struct device_node *np = dev->of_node;
+	int len, i;
+
+	if (sdhci_msm_dt_parse_vreg_info(dev, &pdata->vdd, "vdd")) {
+		dev_err(dev, "failed parsing vdd data\n");
+		return -EINVAL;
+	}
+	if (sdhci_msm_dt_parse_vreg_info(dev, &pdata->vdd_io, "vdd-io")) {
+		dev_err(dev, "failed parsing vdd-io data\n");
+		return -EINVAL;
+	}
+
+	len = of_property_count_strings(np, "qcom,bus-speed-mode");
+
+	for (i = 0; i < len; i++) {
+		const char *name = NULL;
+
+		of_property_read_string_index(np,
+					"qcom,bus-speed-mode", i, &name);
+		if (!name)
+			continue;
+
+		if (!strncmp(name, "HS200_1p8v", sizeof("HS200_1p8v")))
+			pdata->caps2 |= MMC_CAP2_HS200_1_8V_SDR;
+		else if (!strncmp(name, "HS200_1p2v", sizeof("HS200_1p2v")))
+			pdata->caps2 |= MMC_CAP2_HS200_1_2V_SDR;
+		else if (!strncmp(name, "DDR_1p8v", sizeof("DDR_1p8v")))
+			pdata->caps |= MMC_CAP_1_8V_DDR | MMC_CAP_UHS_DDR50;
+		else if (!strncmp(name, "DDR_1p2v", sizeof("DDR_1p2v")))
+			pdata->caps |= MMC_CAP_1_2V_DDR | MMC_CAP_UHS_DDR50;
+	}
+
+	return 0;
+}
+
+/* Regulator utility functions */
+static int sdhci_msm_vreg_init_reg(struct device *dev,
+				   struct sdhci_msm_reg_data *vreg)
+{
+	vreg->reg = devm_regulator_get(dev, vreg->name);
+	if (IS_ERR(vreg->reg))
+		return PTR_ERR(vreg->reg);
+
+	/* sanity check */
+	if (!vreg->high_vol_level || !vreg->low_vol_level
+		|| vreg->low_vol_level > vreg->high_vol_level
+		|| !vreg->hpm_uA || !vreg->lpm_uA
+		|| vreg->lpm_uA > vreg->hpm_uA) {
+		dev_err(dev, "%s invalid constraints specified\n", vreg->name);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int sdhci_msm_vreg_enable(struct device *dev,
+				struct sdhci_msm_reg_data *vreg)
+{
+	int ret;
+
+	if (IS_ERR(vreg->reg))
+		return 0;
+
+	/* Put regulator in HPM (high power mode) */
+	ret = regulator_set_optimum_mode(vreg->reg, vreg->hpm_uA);
+	if (ret < 0) {
+		dev_err(dev, "regulator_set_optimum_mode(%s,uA_load=%d) fail (%d)\n",
+			vreg->name, vreg->hpm_uA, ret);
+		return ret;
+	}
+
+	if (!regulator_is_enabled(vreg->reg)) {
+		/* Set voltage level */
+		ret = regulator_set_voltage(vreg->reg, vreg->high_vol_level,
+						vreg->high_vol_level);
+		if (ret)
+			return ret;
+	}
+
+	ret = regulator_enable(vreg->reg);
+	if (ret) {
+		dev_err(dev, "regulator_enable(%s) fail (%d)\n",
+			vreg->name, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int sdhci_msm_vreg_disable(struct device *dev,
+				struct sdhci_msm_reg_data *vreg)
+{
+	int ret;
+
+	if (IS_ERR(vreg->reg))
+		return 0;
+
+	if (!regulator_is_enabled(vreg->reg))
+		return 0;
+
+	/* Put regulator in LPM (low power mode) */
+	if (vreg->lpm_sup) {
+		ret = regulator_set_optimum_mode(vreg->reg,
+			vreg->lpm_uA);
+		if (ret < 0)
+			return ret;
+	} else {
+		ret = regulator_set_optimum_mode(vreg->reg, 0);
+		if (ret < 0)
+			return ret;
+
+		/* Set min. voltage level to 0 */
+		ret = regulator_set_voltage(vreg->reg, 0, vreg->high_vol_level);
+		if (ret)
+			return ret;
+	}
+
+	ret = regulator_disable(vreg->reg);
+	if (ret) {
+		dev_err(dev, "regulator_disable(%s) fail (%d)\n",
+			vreg->name, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int sdhci_msm_setup_vreg(struct sdhci_msm_host *msm_host, bool enable)
+{
+	int ret, i;
+	struct sdhci_msm_reg_data *vreg_table[2];
+
+	vreg_table[0] = &msm_host->pdata.vdd;
+	vreg_table[1] = &msm_host->pdata.vdd_io;
+
+	for (i = 0; i < ARRAY_SIZE(vreg_table); i++) {
+		if (!vreg_table[i])
+			continue;
+		if (enable)
+			ret = sdhci_msm_vreg_enable(&msm_host->pdev->dev,
+						vreg_table[i]);
+		else
+			ret = sdhci_msm_vreg_disable(&msm_host->pdev->dev,
+						vreg_table[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+/* This init function should be called only once for each SDHC slot */
+static int sdhci_msm_vreg_init(struct device *dev,
+				struct sdhci_msm_pltfm_data *pdata)
+{
+	struct sdhci_msm_reg_data *curr_vdd_reg = &pdata->vdd;
+	struct sdhci_msm_reg_data *curr_vdd_io_reg = &pdata->vdd_io;
+	int ret;
+
+	ret = sdhci_msm_vreg_init_reg(dev, curr_vdd_reg);
+	if (ret)
+		return ret;
+
+	ret = sdhci_msm_vreg_init_reg(dev, curr_vdd_io_reg);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static irqreturn_t sdhci_msm_pwr_irq(int irq, void *data)
+{
+	struct sdhci_host *host = (struct sdhci_host *)data;
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = pltfm_host->priv;
+	u8 irq_status;
+	u8 irq_ack = 0;
+	int ret = 0;
+
+	irq_status = readb_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
+	dev_dbg(mmc_dev(msm_host->mmc), "%s: Received IRQ(%d), status=0x%x\n",
+		mmc_hostname(msm_host->mmc), irq, irq_status);
+
+	/* Clear the interrupt */
+	writeb_relaxed(irq_status, (msm_host->core_mem + CORE_PWRCTL_CLEAR));
+	/*
+	 * SDHC has core_mem and hc_mem device memory and these memory
+	 * addresses do not fall within 1KB region. Hence, any update to
+	 * core_mem address space would require an mb() to ensure this gets
+	 * completed before its next update to registers within hc_mem.
+	 */
+	mb();
+
+	/* Handle BUS ON/OFF*/
+	if (irq_status & CORE_PWRCTL_BUS_ON) {
+		ret = sdhci_msm_setup_vreg(msm_host, true);
+		if (!ret)
+			ret = regulator_set_voltage(msm_host->pdata.vdd_io.reg,
+				msm_host->pdata.vdd_io.high_vol_level,
+				msm_host->pdata.vdd_io.high_vol_level);
+		if (ret)
+			irq_ack |= CORE_PWRCTL_BUS_FAIL;
+		else
+			irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
+	}
+
+	if (irq_status & CORE_PWRCTL_BUS_OFF) {
+		ret = sdhci_msm_setup_vreg(msm_host, false);
+		if (!ret)
+			ret = regulator_set_voltage(msm_host->pdata.vdd_io.reg,
+				msm_host->pdata.vdd_io.low_vol_level,
+				msm_host->pdata.vdd_io.low_vol_level);
+		if (ret)
+			irq_ack |= CORE_PWRCTL_BUS_FAIL;
+		else
+			irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
+	}
+
+	/* Handle IO LOW/HIGH */
+	if (irq_status & CORE_PWRCTL_IO_LOW) {
+		ret = regulator_set_voltage(msm_host->pdata.vdd_io.reg,
+			msm_host->pdata.vdd_io.low_vol_level,
+			msm_host->pdata.vdd_io.low_vol_level);
+		if (ret)
+			irq_ack |= CORE_PWRCTL_IO_FAIL;
+		else
+			irq_ack |= CORE_PWRCTL_IO_SUCCESS;
+	}
+
+	if (irq_status & CORE_PWRCTL_IO_HIGH) {
+		ret = regulator_set_voltage(msm_host->pdata.vdd_io.reg,
+			msm_host->pdata.vdd_io.high_vol_level,
+			msm_host->pdata.vdd_io.high_vol_level);
+		if (ret)
+			irq_ack |= CORE_PWRCTL_IO_FAIL;
+		else
+			irq_ack |= CORE_PWRCTL_IO_SUCCESS;
+	}
+
+	/* ACK status to the core */
+	writeb_relaxed(irq_ack, (msm_host->core_mem + CORE_PWRCTL_CTL));
+	/*
+	 * SDHC has core_mem and hc_mem device memory and these memory
+	 * addresses do not fall within 1KB region. Hence, any update to
+	 * core_mem address space would require an mb() to ensure this gets
+	 * completed before its next update to registers within hc_mem.
+	 */
+	mb();
+
+	dev_dbg(mmc_dev(msm_host->mmc), "%s: Handled IRQ(%d), ret=%d, ack=0x%x\n",
+		 mmc_hostname(msm_host->mmc), irq, ret, irq_ack);
+	return IRQ_HANDLED;
+}
+
+/* This function returns the max. current supported by VDD rail in mA */
+static u32 sdhci_msm_get_vdd_max_current(struct sdhci_msm_host *host)
+{
+	return host->pdata.vdd.hpm_uA / 1000;
+}
+
+static const struct of_device_id sdhci_msm_dt_match[] = {
+	{.compatible = "qcom,sdhci-msm"},
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
+
+static struct sdhci_ops sdhci_msm_ops = {
+	.platform_execute_tuning = sdhci_msm_execute_tuning,
+};
+
+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, dead;
+	u16 host_version;
+	u32 irq_status, irq_ctl;
+
+	if (!pdev->dev.of_node) {
+		dev_err(&pdev->dev, "No device tree data\n");
+		return -ENOENT;
+	}
+
+	msm_host = devm_kzalloc(&pdev->dev, sizeof(*msm_host), GFP_KERNEL);
+	if (!msm_host)
+		return -ENOMEM;
+
+	msm_host->sdhci_msm_pdata.ops = &sdhci_msm_ops;
+	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) {
+		dev_err(&pdev->dev, "failed parsing mmc device tree\n");
+		goto pltfm_free;
+	}
+
+	ret = sdhci_msm_populate_pdata(&pdev->dev, &msm_host->pdata);
+	if (ret) {
+		dev_err(&pdev->dev, "DT parsing error\n");
+		goto pltfm_free;
+	}
+
+	/* Setup SDCC bus voter clock. */
+	msm_host->bus_clk = devm_clk_get(&pdev->dev, "bus");
+	if (!IS_ERR(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 fail (%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 fail (%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);
+	if (ret) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "Regulator setup fail (%d)\n", ret);
+		goto clk_disable;
+	}
+
+	core_memres = platform_get_resource_byname(pdev,
+						   IORESOURCE_MEM, "core_mem");
+	msm_host->core_mem = devm_ioremap_resource(&pdev->dev, core_memres);
+
+	if (IS_ERR(msm_host->core_mem)) {
+		dev_err(&pdev->dev, "Failed to remap registers\n");
+		ret = PTR_ERR(msm_host->core_mem);
+		goto vreg_disable;
+	}
+
+	/* Reset the core and Enable SDHC mode */
+	writel_relaxed(readl_relaxed(msm_host->core_mem + CORE_POWER) |
+			CORE_SW_RST, msm_host->core_mem + CORE_POWER);
+
+	/* SW reset can take upto 10HCLK + 15MCLK cycles. (min 40us) */
+	usleep_range(1000, 5000);
+	if (readl(msm_host->core_mem + CORE_POWER) & CORE_SW_RST) {
+		dev_err(&pdev->dev, "Stuck in reset\n");
+		ret = -ETIMEDOUT;
+		goto vreg_disable;
+	}
+
+	/* Set HC_MODE_EN bit in HC_MODE register */
+	writel_relaxed(HC_MODE_EN, (msm_host->core_mem + CORE_HC_MODE));
+
+	/*
+	 * CORE_SW_RST above may trigger power irq if previous status of PWRCTL
+	 * was either BUS_ON or IO_HIGH_V. So before we enable the power irq
+	 * interrupt in GIC (by registering the interrupt handler), we need to
+	 * ensure that any pending power irq interrupt status is acknowledged
+	 * otherwise power irq interrupt handler would be fired prematurely.
+	 */
+	irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
+	writel_relaxed(irq_status, (msm_host->core_mem + CORE_PWRCTL_CLEAR));
+	irq_ctl = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_CTL);
+	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
+		irq_ctl |= CORE_PWRCTL_BUS_SUCCESS;
+	if (irq_status & (CORE_PWRCTL_IO_HIGH | CORE_PWRCTL_IO_LOW))
+		irq_ctl |= CORE_PWRCTL_IO_SUCCESS;
+	writel_relaxed(irq_ctl, (msm_host->core_mem + CORE_PWRCTL_CTL));
+	/*
+	 * Ensure that above writes are propogated before interrupt enablement
+	 * in GIC.
+	 */
+	mb();
+
+	/*
+	 * Following are the deviations from SDHC spec v3.0 -
+	 * 1. Card detection is handled using separate GPIO.
+	 * 2. Bus power control is handled by interacting with PMIC.
+	 */
+	host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
+	host->quirks |= SDHCI_QUIRK_SINGLE_POWER_WRITE;
+
+	host_version = readw_relaxed((host->ioaddr + SDHCI_HOST_VERSION));
+	dev_dbg(&pdev->dev, "Host Version: 0x%x Vendor Version 0x%x\n",
+		host_version, ((host_version & SDHCI_VENDOR_VER_MASK) >>
+		SDHCI_VENDOR_VER_SHIFT));
+
+	/* Setup PWRCTL irq */
+	msm_host->pwr_irq = platform_get_irq_byname(pdev, "pwr_irq");
+	if (msm_host->pwr_irq < 0) {
+		dev_err(&pdev->dev, "Failed to get pwr_irq by name (%d)\n",
+			msm_host->pwr_irq);
+		goto vreg_disable;
+	}
+	ret = devm_request_threaded_irq(&pdev->dev, msm_host->pwr_irq, NULL,
+					sdhci_msm_pwr_irq, IRQF_ONESHOT,
+					dev_name(&pdev->dev), host);
+	if (ret) {
+		dev_err(&pdev->dev, "Request threaded irq(%d) fail (%d)\n",
+			msm_host->pwr_irq, ret);
+		goto vreg_disable;
+	}
+
+	/* Enable pwr irq interrupts */
+	writel_relaxed(INT_MASK, (msm_host->core_mem + CORE_PWRCTL_MASK));
+
+	msm_host->mmc->caps |= msm_host->pdata.caps;
+	msm_host->mmc->caps2 |= msm_host->pdata.caps2;
+
+	ret = sdhci_add_host(host);
+	if (ret) {
+		dev_err(&pdev->dev, "Add host fail (%d)\n", ret);
+		goto vreg_disable;
+	}
+
+	ret = clk_set_rate(msm_host->clk, host->max_clk);
+	if (ret) {
+		dev_err(&pdev->dev, "MClk rate set fail (%d)\n", ret);
+		goto remove_host;
+	}
+
+	host->mmc->max_current_180 = host->mmc->max_current_300 =
+	host->mmc->max_current_330 = sdhci_msm_get_vdd_max_current(msm_host);
+
+	return 0;
+
+remove_host:
+	dead = (readl_relaxed(host->ioaddr + SDHCI_INT_STATUS) == 0xffffffff);
+	sdhci_remove_host(host, dead);
+vreg_disable:
+	if (!IS_ERR(msm_host->pdata.vdd.reg))
+		sdhci_msm_vreg_disable(&pdev->dev, &msm_host->pdata.vdd);
+	if (!IS_ERR(msm_host->pdata.vdd_io.reg))
+		sdhci_msm_vreg_disable(&pdev->dev, &msm_host->pdata.vdd_io);
+clk_disable:
+	if (!IS_ERR(msm_host->clk))
+		clk_disable_unprepare(msm_host->clk);
+pclk_disable:
+	if (!IS_ERR(msm_host->pclk))
+		clk_disable_unprepare(msm_host->pclk);
+bus_clk_disable:
+	if (!IS_ERR(msm_host->bus_clk))
+		clk_disable_unprepare(msm_host->bus_clk);
+pltfm_free:
+	sdhci_pltfm_free(pdev);
+	return ret;
+}
+
+static int sdhci_msm_remove(struct platform_device *pdev)
+{
+	struct sdhci_host *host = platform_get_drvdata(pdev);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = pltfm_host->priv;
+	int dead = (readl_relaxed(host->ioaddr + SDHCI_INT_STATUS) ==
+		    0xffffffff);
+
+	sdhci_remove_host(host, dead);
+	sdhci_pltfm_free(pdev);
+	sdhci_msm_vreg_disable(&pdev->dev, &msm_host->pdata.vdd);
+	sdhci_msm_vreg_disable(&pdev->dev, &msm_host->pdata.vdd_io);
+	clk_disable_unprepare(msm_host->clk);
+	clk_disable_unprepare(msm_host->pclk);
+	if (!IS_ERR(msm_host->bus_clk))
+		clk_disable_unprepare(msm_host->bus_clk);
+	return 0;
+}
+
+static struct platform_driver sdhci_msm_driver = {
+	.probe = sdhci_msm_probe,
+	.remove = sdhci_msm_remove,
+	.driver = {
+		   .name = "sdhci_msm",
+		   .owner = THIS_MODULE,
+		   .of_match_table = sdhci_msm_dt_match,
+	},
+};
+
+module_platform_driver(sdhci_msm_driver);
+
+MODULE_DESCRIPTION("Qualcomm Secure Digital Host Controller Interface driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH v7 0/2] mmc: sdhci-msm: Add support for MSM chipsets

[Ivan T. Ivanov]

Hi Georgi,

On Wed, 2013-11-06 at 17:56 +0200, Georgi Djakov wrote: 
> This patchset adds basic support of the Secure Digital Host Controller
> Interface compliant controller found in Qualcomm MSM chipsets.
> 
> Tested with SD card on APQ8074 Dragonboard.

Thanks, Tested with mass-storage gadget :-). Forks fine.

Tested-by: Ivan T. Ivanov <iivanov@mm-sol.com>

Regards.

> 
> Georgi Djakov (2):
>   mmc: sdhci-msm: Initial SDHCI MSM driver documentation
>   mmc: sdhci-msm: Initial support for MSM chipsets
> 
>  .../devicetree/bindings/mmc/sdhci-msm.txt          |   93 +++
>  drivers/mmc/host/Kconfig                           |   13 +
>  drivers/mmc/host/Makefile                          |    1 +
>  drivers/mmc/host/sdhci-msm.c                       |  655 ++++++++++++++++++++
>  4 files changed, 762 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>  create mode 100644 drivers/mmc/host/sdhci-msm.c
> 

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

[Mark Rutland]

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.

Re: [PATCH v7 2/2] mmc: sdhci-msm: Initial support for MSM chipsets

[Mark Rutland]

On Wed, Nov 06, 2013 at 03:56:45PM +0000, Georgi Djakov wrote:
> This platform driver adds the initial support of Secure
> Digital Host Controller Interface compliant controller
> found in Qualcomm MSM chipsets.
> 
> Signed-off-by: Georgi Djakov <gdjakov@mm-sol.com>
> ---
>  drivers/mmc/host/Kconfig     |   13 +
>  drivers/mmc/host/Makefile    |    1 +
>  drivers/mmc/host/sdhci-msm.c |  651 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 665 insertions(+)
>  create mode 100644 drivers/mmc/host/sdhci-msm.c

[...]

> +static int sdhci_msm_dt_parse_vreg_info(struct device *dev,
> +                                       struct sdhci_msm_reg_data *vreg,
> +                                       const char *vreg_name)
> +{
> +       int len;
> +       const __be32 *prop;

Seeing raw property handling in drivers worries me. If there's a reason
to touch the raw DTB we should add helpers to do it rather than leaking
binary format issues into drivers.

> +       char prop_name[MAX_PROP_SIZE];
> +       struct device_node *np = dev->of_node;
> +
> +       snprintf(prop_name, MAX_PROP_SIZE, "%s-supply", vreg_name);
> +       if (!of_parse_phandle(np, prop_name, 0)) {
> +               dev_info(dev, "No vreg data found for %s\n", vreg_name);
> +               return -EINVAL;
> +       }
> +
> +       vreg->name = vreg_name;
> +
> +       snprintf(prop_name, MAX_PROP_SIZE, "qcom,%s-lpm-sup", vreg_name);
> +       if (of_get_property(np, prop_name, NULL))
> +               vreg->lpm_sup = true;
> +
> +       snprintf(prop_name, MAX_PROP_SIZE, "qcom,%s-voltage-level", vreg_name);
> +       prop = of_get_property(np, prop_name, &len);
> +       if (!prop || (len != (2 * sizeof(__be32)))) {
> +               dev_warn(dev, "%s %s property\n",
> +               prop ? "invalid format" : "no", prop_name);
> +       } else {
> +               vreg->low_vol_level = be32_to_cpup(&prop[0]);
> +               vreg->high_vol_level = be32_to_cpup(&prop[1]);
> +       }

You can use of_property_read_u32_array here.

> +
> +       snprintf(prop_name, MAX_PROP_SIZE, "qcom,%s-current-level", vreg_name);
> +       prop = of_get_property(np, prop_name, &len);
> +       if (!prop || (len != (2 * sizeof(__be32)))) {
> +               dev_warn(dev, "%s %s property\n",
> +                        prop ? "invalid format" : "no", prop_name);
> +       } else {
> +               vreg->lpm_uA = be32_to_cpup(&prop[0]);
> +               vreg->hpm_uA = be32_to_cpup(&prop[1]);
> +       }

Likewise.

[...]

> +       /*
> +        * CORE_SW_RST above may trigger power irq if previous status of PWRCTL
> +        * was either BUS_ON or IO_HIGH_V. So before we enable the power irq
> +        * interrupt in GIC (by registering the interrupt handler), we need to
> +        * ensure that any pending power irq interrupt status is acknowledged
> +        * otherwise power irq interrupt handler would be fired prematurely.
> +        */
> +       irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
> +       writel_relaxed(irq_status, (msm_host->core_mem + CORE_PWRCTL_CLEAR));
> +       irq_ctl = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_CTL);
> +       if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
> +               irq_ctl |= CORE_PWRCTL_BUS_SUCCESS;
> +       if (irq_status & (CORE_PWRCTL_IO_HIGH | CORE_PWRCTL_IO_LOW))
> +               irq_ctl |= CORE_PWRCTL_IO_SUCCESS;
> +       writel_relaxed(irq_ctl, (msm_host->core_mem + CORE_PWRCTL_CTL));
> +       /*
> +        * Ensure that above writes are propogated before interrupt enablement
> +        * in GIC.
> +        */
> +       mb();

Does this guarantee that the device has finished responding to the write
and deasserted the interrupt line (i.e. does the device only acknowledge
the write once that is true)?

Thanks,
Mark.

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@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

Re: [PATCH v7 2/2] mmc: sdhci-msm: Initial support for MSM chipsets

[Georgi Djakov]

On 12/05/2013 12:27 PM, Mark Rutland wrote:
> On Wed, Nov 06, 2013 at 03:56:45PM +0000, Georgi Djakov wrote:
>> This platform driver adds the initial support of Secure
>> Digital Host Controller Interface compliant controller
>> found in Qualcomm MSM chipsets.
>>
>> Signed-off-by: Georgi Djakov <gdjakov@mm-sol.com>
>> ---
>>   drivers/mmc/host/Kconfig     |   13 +
>>   drivers/mmc/host/Makefile    |    1 +
>>   drivers/mmc/host/sdhci-msm.c |  651 ++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 665 insertions(+)
>>   create mode 100644 drivers/mmc/host/sdhci-msm.c
>
> [...]
>
>> +static int sdhci_msm_dt_parse_vreg_info(struct device *dev,
>> +                                       struct sdhci_msm_reg_data *vreg,
>> +                                       const char *vreg_name)
>> +{
>> +       int len;
>> +       const __be32 *prop;
>
> Seeing raw property handling in drivers worries me. If there's a reason
> to touch the raw DTB we should add helpers to do it rather than leaking
> binary format issues into drivers.
>
>> +       char prop_name[MAX_PROP_SIZE];
>> +       struct device_node *np = dev->of_node;
>> +
>> +       snprintf(prop_name, MAX_PROP_SIZE, "%s-supply", vreg_name);
>> +       if (!of_parse_phandle(np, prop_name, 0)) {
>> +               dev_info(dev, "No vreg data found for %s\n", vreg_name);
>> +               return -EINVAL;
>> +       }
>> +
>> +       vreg->name = vreg_name;
>> +
>> +       snprintf(prop_name, MAX_PROP_SIZE, "qcom,%s-lpm-sup", vreg_name);
>> +       if (of_get_property(np, prop_name, NULL))
>> +               vreg->lpm_sup = true;
>> +
>> +       snprintf(prop_name, MAX_PROP_SIZE, "qcom,%s-voltage-level", vreg_name);
>> +       prop = of_get_property(np, prop_name, &len);
>> +       if (!prop || (len != (2 * sizeof(__be32)))) {
>> +               dev_warn(dev, "%s %s property\n",
>> +               prop ? "invalid format" : "no", prop_name);
>> +       } else {
>> +               vreg->low_vol_level = be32_to_cpup(&prop[0]);
>> +               vreg->high_vol_level = be32_to_cpup(&prop[1]);
>> +       }
>
> You can use of_property_read_u32_array here.
>
>> +
>> +       snprintf(prop_name, MAX_PROP_SIZE, "qcom,%s-current-level", vreg_name);
>> +       prop = of_get_property(np, prop_name, &len);
>> +       if (!prop || (len != (2 * sizeof(__be32)))) {
>> +               dev_warn(dev, "%s %s property\n",
>> +                        prop ? "invalid format" : "no", prop_name);
>> +       } else {
>> +               vreg->lpm_uA = be32_to_cpup(&prop[0]);
>> +               vreg->hpm_uA = be32_to_cpup(&prop[1]);
>> +       }
>
> Likewise.
>

I will clean this up use only of_property_read_u32_array() and 
of_property_read_bool() for DT parsing. Thanks!


> [...]
>
>> +       /*
>> +        * CORE_SW_RST above may trigger power irq if previous status of PWRCTL
>> +        * was either BUS_ON or IO_HIGH_V. So before we enable the power irq
>> +        * interrupt in GIC (by registering the interrupt handler), we need to
>> +        * ensure that any pending power irq interrupt status is acknowledged
>> +        * otherwise power irq interrupt handler would be fired prematurely.
>> +        */
>> +       irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
>> +       writel_relaxed(irq_status, (msm_host->core_mem + CORE_PWRCTL_CLEAR));
>> +       irq_ctl = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_CTL);
>> +       if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
>> +               irq_ctl |= CORE_PWRCTL_BUS_SUCCESS;
>> +       if (irq_status & (CORE_PWRCTL_IO_HIGH | CORE_PWRCTL_IO_LOW))
>> +               irq_ctl |= CORE_PWRCTL_IO_SUCCESS;
>> +       writel_relaxed(irq_ctl, (msm_host->core_mem + CORE_PWRCTL_CTL));
>> +       /*
>> +        * Ensure that above writes are propogated before interrupt enablement
>> +        * in GIC.
>> +        */
>> +       mb();
>
> Does this guarantee that the device has finished responding to the write
> and deasserted the interrupt line (i.e. does the device only acknowledge
> the write once that is true)?
>

I am not sure that i understand your concern. The write to 
CORE_PWRCTL_CTL should acknowledge and deassert the interrupt.

Thanks,
Georgi

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

[Mark Rutland]

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.

Re: [PATCH v7 2/2] mmc: sdhci-msm: Initial support for MSM chipsets

[Mark Rutland]

> > [...]
> >
> >> +       /*
> >> +        * CORE_SW_RST above may trigger power irq if previous status of PWRCTL
> >> +        * was either BUS_ON or IO_HIGH_V. So before we enable the power irq
> >> +        * interrupt in GIC (by registering the interrupt handler), we need to
> >> +        * ensure that any pending power irq interrupt status is acknowledged
> >> +        * otherwise power irq interrupt handler would be fired prematurely.
> >> +        */
> >> +       irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
> >> +       writel_relaxed(irq_status, (msm_host->core_mem + CORE_PWRCTL_CLEAR));
> >> +       irq_ctl = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_CTL);
> >> +       if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
> >> +               irq_ctl |= CORE_PWRCTL_BUS_SUCCESS;
> >> +       if (irq_status & (CORE_PWRCTL_IO_HIGH | CORE_PWRCTL_IO_LOW))
> >> +               irq_ctl |= CORE_PWRCTL_IO_SUCCESS;
> >> +       writel_relaxed(irq_ctl, (msm_host->core_mem + CORE_PWRCTL_CTL));
> >> +       /*
> >> +        * Ensure that above writes are propogated before interrupt enablement
> >> +        * in GIC.
> >> +        */
> >> +       mb();
> >
> > Does this guarantee that the device has finished responding to the write
> > and deasserted the interrupt line (i.e. does the device only acknowledge
> > the write once that is true)?
> >
> 
> I am not sure that i understand your concern. The write to 
> CORE_PWRCTL_CTL should acknowledge and deassert the interrupt.

The mb() ensures that the write has reached the device, and the device's
slave interface has acknowledged the write. On some devices this
acknowledgement of the write can be asynchronous with respect to the
device changing state in response to the write (i.e. the interrupt might
get deasserted a short time after the write completes). Typically there
is a register that should be polled to see whether the state change has
completed.

Does the acknowledgement of the write only occur once the device has
changed state? Or might it change state in the background?

Thanks,
Mark

Re: [PATCH v7 2/2] mmc: sdhci-msm: Initial support for MSM chipsets

[Courtney Cavin]

On Wed, Nov 06, 2013 at 04:56:45PM +0100, Georgi Djakov wrote:
> This platform driver adds the initial support of Secure
> Digital Host Controller Interface compliant controller
> found in Qualcomm MSM chipsets.
> 
> Signed-off-by: Georgi Djakov <gdjakov@mm-sol.com>
[...]
> +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, dead;
> +       u16 host_version;
> +       u32 irq_status, irq_ctl;
> +
> +       if (!pdev->dev.of_node) {
> +               dev_err(&pdev->dev, "No device tree data\n");
> +               return -ENOENT;
> +       }
> +
> +       msm_host = devm_kzalloc(&pdev->dev, sizeof(*msm_host), GFP_KERNEL);
> +       if (!msm_host)
> +               return -ENOMEM;
> +
> +       msm_host->sdhci_msm_pdata.ops = &sdhci_msm_ops;
> +       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);

Can we please add a call to sdhci_get_of_property(pdev) somewhere around
here too?

> +       if (ret) {
> +               dev_err(&pdev->dev, "failed parsing mmc device tree\n");
> +               goto pltfm_free;
> +       }
> +
> +       ret = sdhci_msm_populate_pdata(&pdev->dev, &msm_host->pdata);
> +       if (ret) {
> +               dev_err(&pdev->dev, "DT parsing error\n");
> +               goto pltfm_free;
> +       }
> +
> +       /* Setup SDCC bus voter clock. */
> +       msm_host->bus_clk = devm_clk_get(&pdev->dev, "bus");
> +       if (!IS_ERR(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 fail (%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 fail (%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);
> +       if (ret) {
> +               if (ret != -EPROBE_DEFER)
> +                       dev_err(&pdev->dev, "Regulator setup fail (%d)\n", ret);
> +               goto clk_disable;
> +       }
> +
> +       core_memres = platform_get_resource_byname(pdev,
> +                                                  IORESOURCE_MEM, "core_mem");
> +       msm_host->core_mem = devm_ioremap_resource(&pdev->dev, core_memres);
> +
> +       if (IS_ERR(msm_host->core_mem)) {
> +               dev_err(&pdev->dev, "Failed to remap registers\n");
> +               ret = PTR_ERR(msm_host->core_mem);
> +               goto vreg_disable;
> +       }
> +
> +       /* Reset the core and Enable SDHC mode */
> +       writel_relaxed(readl_relaxed(msm_host->core_mem + CORE_POWER) |
> +                       CORE_SW_RST, msm_host->core_mem + CORE_POWER);
> +
> +       /* SW reset can take upto 10HCLK + 15MCLK cycles. (min 40us) */
> +       usleep_range(1000, 5000);
> +       if (readl(msm_host->core_mem + CORE_POWER) & CORE_SW_RST) {
> +               dev_err(&pdev->dev, "Stuck in reset\n");
> +               ret = -ETIMEDOUT;
> +               goto vreg_disable;
> +       }
> +
> +       /* Set HC_MODE_EN bit in HC_MODE register */
> +       writel_relaxed(HC_MODE_EN, (msm_host->core_mem + CORE_HC_MODE));
> +
> +       /*
> +        * CORE_SW_RST above may trigger power irq if previous status of PWRCTL
> +        * was either BUS_ON or IO_HIGH_V. So before we enable the power irq
> +        * interrupt in GIC (by registering the interrupt handler), we need to
> +        * ensure that any pending power irq interrupt status is acknowledged
> +        * otherwise power irq interrupt handler would be fired prematurely.
> +        */
> +       irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
> +       writel_relaxed(irq_status, (msm_host->core_mem + CORE_PWRCTL_CLEAR));
> +       irq_ctl = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_CTL);
> +       if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
> +               irq_ctl |= CORE_PWRCTL_BUS_SUCCESS;
> +       if (irq_status & (CORE_PWRCTL_IO_HIGH | CORE_PWRCTL_IO_LOW))
> +               irq_ctl |= CORE_PWRCTL_IO_SUCCESS;
> +       writel_relaxed(irq_ctl, (msm_host->core_mem + CORE_PWRCTL_CTL));
> +       /*
> +        * Ensure that above writes are propogated before interrupt enablement
> +        * in GIC.
> +        */
> +       mb();
> +
> +       /*
> +        * Following are the deviations from SDHC spec v3.0 -
> +        * 1. Card detection is handled using separate GPIO.
> +        * 2. Bus power control is handled by interacting with PMIC.
> +        */
> +       host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
> +       host->quirks |= SDHCI_QUIRK_SINGLE_POWER_WRITE;

I couldn't get v6 running without the 5 quirks you submitted in [1].
Aren't these also attributes of the controller itself? If so, shouldn't they
be part of this patch series, and included here?

> +       host_version = readw_relaxed((host->ioaddr + SDHCI_HOST_VERSION));
> +       dev_dbg(&pdev->dev, "Host Version: 0x%x Vendor Version 0x%x\n",
> +               host_version, ((host_version & SDHCI_VENDOR_VER_MASK) >>
> +               SDHCI_VENDOR_VER_SHIFT));
> +
> +       /* Setup PWRCTL irq */
> +       msm_host->pwr_irq = platform_get_irq_byname(pdev, "pwr_irq");
> +       if (msm_host->pwr_irq < 0) {
> +               dev_err(&pdev->dev, "Failed to get pwr_irq by name (%d)\n",
> +                       msm_host->pwr_irq);
> +               goto vreg_disable;
> +       }
> +       ret = devm_request_threaded_irq(&pdev->dev, msm_host->pwr_irq, NULL,
> +                                       sdhci_msm_pwr_irq, IRQF_ONESHOT,
> +                                       dev_name(&pdev->dev), host);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Request threaded irq(%d) fail (%d)\n",
> +                       msm_host->pwr_irq, ret);
> +               goto vreg_disable;
> +       }
> +
> +       /* Enable pwr irq interrupts */
> +       writel_relaxed(INT_MASK, (msm_host->core_mem + CORE_PWRCTL_MASK));
> +
> +       msm_host->mmc->caps |= msm_host->pdata.caps;
> +       msm_host->mmc->caps2 |= msm_host->pdata.caps2;
> +
> +       ret = sdhci_add_host(host);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Add host fail (%d)\n", ret);
> +               goto vreg_disable;
> +       }
> +
> +       ret = clk_set_rate(msm_host->clk, host->max_clk);
> +       if (ret) {
> +               dev_err(&pdev->dev, "MClk rate set fail (%d)\n", ret);
> +               goto remove_host;
> +       }
> +
> +       host->mmc->max_current_180 = host->mmc->max_current_300 =
> +       host->mmc->max_current_330 = sdhci_msm_get_vdd_max_current(msm_host);
> +
> +       return 0;
> +
> +remove_host:
> +       dead = (readl_relaxed(host->ioaddr + SDHCI_INT_STATUS) == 0xffffffff);
> +       sdhci_remove_host(host, dead);
> +vreg_disable:
> +       if (!IS_ERR(msm_host->pdata.vdd.reg))
> +               sdhci_msm_vreg_disable(&pdev->dev, &msm_host->pdata.vdd);
> +       if (!IS_ERR(msm_host->pdata.vdd_io.reg))
> +               sdhci_msm_vreg_disable(&pdev->dev, &msm_host->pdata.vdd_io);
> +clk_disable:
> +       if (!IS_ERR(msm_host->clk))
> +               clk_disable_unprepare(msm_host->clk);
> +pclk_disable:
> +       if (!IS_ERR(msm_host->pclk))
> +               clk_disable_unprepare(msm_host->pclk);
> +bus_clk_disable:
> +       if (!IS_ERR(msm_host->bus_clk))
> +               clk_disable_unprepare(msm_host->bus_clk);
> +pltfm_free:
> +       sdhci_pltfm_free(pdev);
> +       return ret;
> +}

-Courtney

[1] http://lkml.org/lkml/2013/8/15/254

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

[Georgi Djakov]

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

Re: [PATCH v7 2/2] mmc: sdhci-msm: Initial support for MSM chipsets

[Georgi Djakov]

On 12/09/2013 11:46 AM, Mark Rutland wrote:
>>> [...]
>>>
>>>> +       /*
>>>> +        * CORE_SW_RST above may trigger power irq if previous status of PWRCTL
>>>> +        * was either BUS_ON or IO_HIGH_V. So before we enable the power irq
>>>> +        * interrupt in GIC (by registering the interrupt handler), we need to
>>>> +        * ensure that any pending power irq interrupt status is acknowledged
>>>> +        * otherwise power irq interrupt handler would be fired prematurely.
>>>> +        */
>>>> +       irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
>>>> +       writel_relaxed(irq_status, (msm_host->core_mem + CORE_PWRCTL_CLEAR));
>>>> +       irq_ctl = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_CTL);
>>>> +       if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
>>>> +               irq_ctl |= CORE_PWRCTL_BUS_SUCCESS;
>>>> +       if (irq_status & (CORE_PWRCTL_IO_HIGH | CORE_PWRCTL_IO_LOW))
>>>> +               irq_ctl |= CORE_PWRCTL_IO_SUCCESS;
>>>> +       writel_relaxed(irq_ctl, (msm_host->core_mem + CORE_PWRCTL_CTL));
>>>> +       /*
>>>> +        * Ensure that above writes are propogated before interrupt enablement
>>>> +        * in GIC.
>>>> +        */
>>>> +       mb();
>>>
>>> Does this guarantee that the device has finished responding to the write
>>> and deasserted the interrupt line (i.e. does the device only acknowledge
>>> the write once that is true)?
>>>
>>
>> I am not sure that i understand your concern. The write to
>> CORE_PWRCTL_CTL should acknowledge and deassert the interrupt.
>
> The mb() ensures that the write has reached the device, and the device's
> slave interface has acknowledged the write. On some devices this
> acknowledgement of the write can be asynchronous with respect to the
> device changing state in response to the write (i.e. the interrupt might
> get deasserted a short time after the write completes). Typically there
> is a register that should be polled to see whether the state change has
> completed.
>
> Does the acknowledgement of the write only occur once the device has
> changed state? Or might it change state in the background?
>

Thanks for clarifying this. All this is correct, but it will be 
difficult to answer, because i don't have documentation and i can only 
be guessing how exactly the hardware behaves internally.
I can remove this fragment from the initial version to keep it more 
simple. Meanwhile i can do some tests reading/polling the status 
register to see how and when exactly it changes.

Thanks,
Georgi

Re: [PATCH v7 2/2] mmc: sdhci-msm: Initial support for MSM chipsets

[Georgi Djakov]

Hi,

Apologies for the delayed reply.

On 12/09/2013 07:00 PM, Courtney Cavin wrote:
> On Wed, Nov 06, 2013 at 04:56:45PM +0100, Georgi Djakov wrote:
>> This platform driver adds the initial support of Secure
>> Digital Host Controller Interface compliant controller
>> found in Qualcomm MSM chipsets.
>>
>> Signed-off-by: Georgi Djakov <gdjakov@mm-sol.com>
> [...]
>> +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, dead;
>> +       u16 host_version;
>> +       u32 irq_status, irq_ctl;
>> +
>> +       if (!pdev->dev.of_node) {
>> +               dev_err(&pdev->dev, "No device tree data\n");
>> +               return -ENOENT;
>> +       }
>> +
>> +       msm_host = devm_kzalloc(&pdev->dev, sizeof(*msm_host), GFP_KERNEL);
>> +       if (!msm_host)
>> +               return -ENOMEM;
>> +
>> +       msm_host->sdhci_msm_pdata.ops = &sdhci_msm_ops;
>> +       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);
>
> Can we please add a call to sdhci_get_of_property(pdev) somewhere around
> here too?
>

Thanks, I have added it to v8.

>> +       if (ret) {
>> +               dev_err(&pdev->dev, "failed parsing mmc device tree\n");
>> +               goto pltfm_free;
>> +       }
>> +
>> +       ret = sdhci_msm_populate_pdata(&pdev->dev, &msm_host->pdata);
>> +       if (ret) {
>> +               dev_err(&pdev->dev, "DT parsing error\n");
>> +               goto pltfm_free;
>> +       }
>> +
>> +       /* Setup SDCC bus voter clock. */
>> +       msm_host->bus_clk = devm_clk_get(&pdev->dev, "bus");
>> +       if (!IS_ERR(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 fail (%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 fail (%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);
>> +       if (ret) {
>> +               if (ret != -EPROBE_DEFER)
>> +                       dev_err(&pdev->dev, "Regulator setup fail (%d)\n", ret);
>> +               goto clk_disable;
>> +       }
>> +
>> +       core_memres = platform_get_resource_byname(pdev,
>> +                                                  IORESOURCE_MEM, "core_mem");
>> +       msm_host->core_mem = devm_ioremap_resource(&pdev->dev, core_memres);
>> +
>> +       if (IS_ERR(msm_host->core_mem)) {
>> +               dev_err(&pdev->dev, "Failed to remap registers\n");
>> +               ret = PTR_ERR(msm_host->core_mem);
>> +               goto vreg_disable;
>> +       }
>> +
>> +       /* Reset the core and Enable SDHC mode */
>> +       writel_relaxed(readl_relaxed(msm_host->core_mem + CORE_POWER) |
>> +                       CORE_SW_RST, msm_host->core_mem + CORE_POWER);
>> +
>> +       /* SW reset can take upto 10HCLK + 15MCLK cycles. (min 40us) */
>> +       usleep_range(1000, 5000);
>> +       if (readl(msm_host->core_mem + CORE_POWER) & CORE_SW_RST) {
>> +               dev_err(&pdev->dev, "Stuck in reset\n");
>> +               ret = -ETIMEDOUT;
>> +               goto vreg_disable;
>> +       }
>> +
>> +       /* Set HC_MODE_EN bit in HC_MODE register */
>> +       writel_relaxed(HC_MODE_EN, (msm_host->core_mem + CORE_HC_MODE));
>> +
>> +       /*
>> +        * CORE_SW_RST above may trigger power irq if previous status of PWRCTL
>> +        * was either BUS_ON or IO_HIGH_V. So before we enable the power irq
>> +        * interrupt in GIC (by registering the interrupt handler), we need to
>> +        * ensure that any pending power irq interrupt status is acknowledged
>> +        * otherwise power irq interrupt handler would be fired prematurely.
>> +        */
>> +       irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
>> +       writel_relaxed(irq_status, (msm_host->core_mem + CORE_PWRCTL_CLEAR));
>> +       irq_ctl = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_CTL);
>> +       if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
>> +               irq_ctl |= CORE_PWRCTL_BUS_SUCCESS;
>> +       if (irq_status & (CORE_PWRCTL_IO_HIGH | CORE_PWRCTL_IO_LOW))
>> +               irq_ctl |= CORE_PWRCTL_IO_SUCCESS;
>> +       writel_relaxed(irq_ctl, (msm_host->core_mem + CORE_PWRCTL_CTL));
>> +       /*
>> +        * Ensure that above writes are propogated before interrupt enablement
>> +        * in GIC.
>> +        */
>> +       mb();
>> +
>> +       /*
>> +        * Following are the deviations from SDHC spec v3.0 -
>> +        * 1. Card detection is handled using separate GPIO.
>> +        * 2. Bus power control is handled by interacting with PMIC.
>> +        */
>> +       host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
>> +       host->quirks |= SDHCI_QUIRK_SINGLE_POWER_WRITE;
>
> I couldn't get v6 running without the 5 quirks you submitted in [1].
> Aren't these also attributes of the controller itself? If so, shouldn't they
> be part of this patch series, and included here?
>

Most of the quirks are for older hardware revisions. On what board/chip 
are you trying to run it? I will post v8 shortly. Could you try with it 
please?

Thanks,
Georgi

Re: [PATCH v7 2/2] mmc: sdhci-msm: Initial support for MSM chipsets

[Courtney Cavin]

On Thu, Jan 30, 2014 at 06:21:11PM +0100, Georgi Djakov wrote:
> Hi,
> 
> Apologies for the delayed reply.
> 
> On 12/09/2013 07:00 PM, Courtney Cavin wrote:
> > On Wed, Nov 06, 2013 at 04:56:45PM +0100, Georgi Djakov wrote:
> >> This platform driver adds the initial support of Secure
> >> Digital Host Controller Interface compliant controller
> >> found in Qualcomm MSM chipsets.
> >>
> >> Signed-off-by: Georgi Djakov <gdjakov@mm-sol.com>
[...]
> >
> > I couldn't get v6 running without the 5 quirks you submitted in [1].
> > Aren't these also attributes of the controller itself? If so, shouldn't they
> > be part of this patch series, and included here?
> >
> 
> Most of the quirks are for older hardware revisions. On what board/chip 
> are you trying to run it? I will post v8 shortly. Could you try with it 
> please?

I see.  This was initially checked on a Rev 1 SoC.  I will test v8 on a
Rev 2.

-Courtney