Add SDHCI support for STMicroelectronics SoCs

Add SDHCI support for STMicroelectronics SoCs

[Peter Griffin]

This series adds a SDHCI platform driver for ST SoCs, along with
the additional device tree bindings and configuration to enable
the controller to work properly.

Initially it supports the stih416 and stih415 SoCs, and has
been tested on a stih416-b2020 board.

regards,

Peter.

[PATCH 1/8] mmc: sdhci-st: Intial support for ST SDHCI controller

[Peter Griffin]

This platform driver adds initial support for the SDHCI host controller
found on STMicroelectronics SoCs.

It has been tested on STiH41x b2020 platforms currently.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 drivers/mmc/host/Kconfig    |  12 +++
 drivers/mmc/host/Makefile   |   1 +
 drivers/mmc/host/sdhci-st.c | 244 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 257 insertions(+)
 create mode 100644 drivers/mmc/host/sdhci-st.c

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 8aaf8c1..b62c40d 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -283,6 +283,18 @@ config MMC_SDHCI_BCM2835
 
 	  If unsure, say N.
 
+config MMC_SDHCI_ST
+	tristate "SDHCI support on STMicroelectronics SoC"
+	depends on ARCH_STI
+	depends on MMC_SDHCI_PLTFM
+	select MMC_SDHCI_IO_ACCESSORS
+	help
+	  This selects the Secure Digital Host Controller Interface in
+	  STMicroelectronics SoCs.
+
+	  If you have a controller with this interface, say Y or M here.
+	  If unsure, say N.
+
 config MMC_OMAP
 	tristate "TI OMAP Multimedia Card Interface support"
 	depends on ARCH_OMAP
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 0c8aa5e..6e0acf7 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -64,6 +64,7 @@ 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
+obj-$(CONFIG_MMC_SDHCI_ST)		+= sdhci-st.o
 
 ifeq ($(CONFIG_CB710_DEBUG),y)
 	CFLAGS-cb710-mmc	+= -DDEBUG
diff --git a/drivers/mmc/host/sdhci-st.c b/drivers/mmc/host/sdhci-st.c
new file mode 100644
index 0000000..1790fa7
--- /dev/null
+++ b/drivers/mmc/host/sdhci-st.c
@@ -0,0 +1,244 @@
+/*
+ * Support for SDHCI on STMicroelectronics SoCs
+ *
+ * Copyright (C) 2014 STMicroelectronics Ltd
+ * Author: Giuseppe Cavallaro <peppe.cavallaro@st.com>
+ *
+ * Based on sdhci-cns3xxx.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License 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/io.h>
+#include <linux/of.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/of_gpio.h>
+#include <linux/mmc/host.h>
+#include <linux/reset.h>
+
+#include "sdhci-pltfm.h"
+
+struct st_mmc_platform_data {
+	struct  reset_control	*rstc;
+};
+
+static int sdhci_st_8bit_width(struct sdhci_host *host, int width)
+{
+	u8 ctrl;
+
+	ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
+
+	switch (width) {
+	case MMC_BUS_WIDTH_8:
+		ctrl |= SDHCI_CTRL_8BITBUS;
+		ctrl &= ~SDHCI_CTRL_4BITBUS;
+		break;
+	case MMC_BUS_WIDTH_4:
+		ctrl |= SDHCI_CTRL_4BITBUS;
+		ctrl &= ~SDHCI_CTRL_8BITBUS;
+		break;
+	default:
+		ctrl &= ~(SDHCI_CTRL_8BITBUS | SDHCI_CTRL_4BITBUS);
+		break;
+	}
+
+	sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
+
+	return 0;
+}
+
+static unsigned int sdhci_st_get_max_clk(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+
+	return clk_get_rate(pltfm_host->clk);
+}
+
+static u32 sdhci_st_readl(struct sdhci_host *host, int reg)
+{
+	u32 ret;
+
+	switch (reg) {
+	case SDHCI_CAPABILITIES:
+		ret = readl(host->ioaddr + reg);
+		/* Support 3.3V and 1.8V */
+		ret &= ~SDHCI_CAN_VDD_300;
+		break;
+	default:
+		ret = readl(host->ioaddr + reg);
+	}
+	return ret;
+}
+
+static const struct sdhci_ops sdhci_st_ops = {
+	.get_max_clock = sdhci_st_get_max_clk,
+	.platform_bus_width = sdhci_st_8bit_width,
+	.read_l = sdhci_st_readl,
+};
+
+static const struct sdhci_pltfm_data sdhci_st_pdata = {
+	.ops = &sdhci_st_ops,
+	.quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC |
+	    SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
+};
+
+
+static int sdhci_st_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct sdhci_host *host;
+	struct st_mmc_platform_data *pdata;
+	struct sdhci_pltfm_host *pltfm_host;
+	struct clk *clk;
+	int ret = 0;
+	u16 host_version;
+
+	dev_dbg(&pdev->dev, "SDHCI ST platform driver\n");
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	pdata->rstc = devm_reset_control_get(&pdev->dev, NULL);
+	if (IS_ERR(pdata->rstc))
+		pdata->rstc = NULL;
+	else
+		reset_control_deassert(pdata->rstc);
+
+	clk =  devm_clk_get(&pdev->dev, "mmc");
+	if (IS_ERR(clk)) {
+		dev_err(&pdev->dev, "Perpheral clk not found\n");
+		return PTR_ERR(clk);
+	}
+
+	host = sdhci_pltfm_init(pdev, &sdhci_st_pdata, 0);
+	if (IS_ERR(host)) {
+		dev_err(&pdev->dev, "Failed sdhci_pltfm_init\n");
+		return PTR_ERR(host);
+	}
+
+	clk_prepare_enable(clk);
+
+	host->mmc->caps |= MMC_CAP_8_BIT_DATA | MMC_CAP_BUS_WIDTH_TEST
+			| MMC_CAP_1_8V_DDR;
+
+	if (of_property_read_bool(np, "non-removable"))
+		host->mmc->caps |= MMC_CAP_NONREMOVABLE;
+
+	pltfm_host = sdhci_priv(host);
+	pltfm_host->clk = clk;
+
+	ret = sdhci_add_host(host);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed sdhci_add_host\n");
+		goto err_out;
+	}
+
+	pltfm_host->priv = pdata;
+
+	platform_set_drvdata(pdev, host);
+
+	host_version = readw_relaxed((host->ioaddr + SDHCI_HOST_VERSION));
+
+	dev_dbg(&pdev->dev, "SDHCI ST Initialised: Host Version: 0x%x Vendor Version 0x%x\n",
+		host_version, ((host_version & SDHCI_VENDOR_VER_MASK) >>
+			       SDHCI_VENDOR_VER_SHIFT));
+
+	return 0;
+
+err_out:
+	clk_disable_unprepare(clk);
+	sdhci_pltfm_free(pdev);
+
+	return ret;
+}
+
+static int sdhci_st_remove(struct platform_device *pdev)
+{
+	struct sdhci_host *host = platform_get_drvdata(pdev);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct st_mmc_platform_data *pdata = pltfm_host->priv;
+	int ret;
+
+	clk_disable_unprepare(pltfm_host->clk);
+
+	ret = sdhci_pltfm_unregister(pdev);
+
+	if (pdata->rstc)
+		reset_control_assert(pdata->rstc);
+
+	return ret;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int sdhci_st_suspend(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct st_mmc_platform_data *pdata = pltfm_host->priv;
+	int ret = sdhci_suspend_host(host);
+
+	if (ret)
+		goto out;
+
+	if (pdata->rstc)
+		reset_control_assert(pdata->rstc);
+
+	clk_disable_unprepare(pltfm_host->clk);
+out:
+	return ret;
+}
+
+static int sdhci_st_resume(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct st_mmc_platform_data *pdata = pltfm_host->priv;
+
+	clk_prepare_enable(pltfm_host->clk);
+
+	if (pdata->rstc)
+		reset_control_deassert(pdata->rstc);
+
+	return sdhci_resume_host(host);
+}
+
+static SIMPLE_DEV_PM_OPS(sdhci_st_pmops, sdhci_st_suspend, sdhci_st_resume);
+#define SDHCI_ST_PMOPS (&sdhci_st_pmops)
+#else
+#define SDHCI_ST_PMOPS NULL
+#endif
+
+static const struct of_device_id st_sdhci_match[] = {
+	{ .compatible = "st,sdhci" },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, st_sdhci_match);
+
+static struct platform_driver sdhci_st_driver = {
+	.probe = sdhci_st_probe,
+	.remove = sdhci_st_remove,
+	.driver = {
+		   .name = "sdhci-st",
+		   .owner = THIS_MODULE,
+		   .pm = SDHCI_ST_PMOPS,
+		   .of_match_table = of_match_ptr(st_sdhci_match),
+		  },
+};
+
+module_platform_driver(sdhci_st_driver);
+
+MODULE_DESCRIPTION("SDHCI driver for STMicroelectronics SoCs");
+MODULE_AUTHOR("Giuseppe Cavallaro <peppe.cavallaro@st.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:st-sdhci");
-- 
1.9.1

[PATCH 2/8] mmc: sdhci-st: STMicroelectronics SDHCI binding documentation.

[Peter Griffin]

This patch adds the device tree binding documentation for ST
SDHCI driver. It contains the differences between the core properties
in mmc.txt and the properties used by the sdhci-st driver.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 Documentation/devicetree/bindings/mmc/sdhci-st.txt | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-st.txt

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-st.txt b/Documentation/devicetree/bindings/mmc/sdhci-st.txt
new file mode 100644
index 0000000..ae3dae0
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/sdhci-st.txt
@@ -0,0 +1,26 @@
+* STMicroelectronics sdhci-st MMC/SD controller
+
+This file documents differences between the core properties in mmc.txt
+and the properties used by the sdhci-st driver
+
+Required properties:
+- compatible: "st,sdhci"
+- clocks: card clock, must be "mmc".
+
+Optional properties:
+- reset: to provide a reset to the HC.
+- non-removable: non-removable slot (bindings/mmc/mmc.txt).
+
+Example:
+
+mmc0: sdhci@fe81e000 {
+	compatible	= "st,sdhci";
+	status		= "disabled";
+	reg		= <0xfe81e000 0x1000>;
+	interrupts	= <GIC_SPI 127 IRQ_TYPE_NONE>;
+	interrupt-names	= "mmcirq";
+	pinctrl-names	= "default";
+	pinctrl-0	= <&pinctrl_mmc0>;
+	clock-names	= "mmc";
+	clocks		= <&clk_s_a1_ls 1>;
+};
-- 
1.9.1

[PATCH 3/8] ARM: STi: DT: Add sdhci pins for stih416

[Peter Griffin]

This adds the required pin config for both SDHCI controllers on
the stih416 SoC.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 arch/arm/boot/dts/stih416-pinctrl.dtsi | 39 ++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/arch/arm/boot/dts/stih416-pinctrl.dtsi b/arch/arm/boot/dts/stih416-pinctrl.dtsi
index 6252188..140af6b 100644
--- a/arch/arm/boot/dts/stih416-pinctrl.dtsi
+++ b/arch/arm/boot/dts/stih416-pinctrl.dtsi
@@ -467,6 +467,45 @@
 					};
 				};
 			};
+
+			mmc0 {
+				pinctrl_mmc0: mmc0 {
+					st,pins {
+						mmcclk = <&PIO13 4 ALT4 BIDIR_PU NICLK 0 CLK_B>;
+						data0 = <&PIO14 4 ALT4 BIDIR_PU BYPASS 0>;
+						data1 = <&PIO14 5 ALT4 BIDIR_PU BYPASS 0>;
+						data2 = <&PIO14 6 ALT4 BIDIR_PU BYPASS 0>;
+						data3 = <&PIO14 7 ALT4 BIDIR_PU BYPASS 0>;
+						cmd = <&PIO15 1 ALT4 BIDIR_PU BYPASS 0>;
+						wp = <&PIO15 3 ALT4 IN>;
+						data4 = <&PIO16 4 ALT4 BIDIR_PU BYPASS 0>;
+						data5 = <&PIO16 5 ALT4 BIDIR_PU BYPASS 0>;
+						data6 = <&PIO16 6 ALT4 BIDIR_PU BYPASS 0>;
+						data7 = <&PIO16 7 ALT4 BIDIR_PU BYPASS 0>;
+						pwr = <&PIO17 1 ALT4 OUT>;
+						cd = <&PIO17 2 ALT4 IN>;
+						led = <&PIO17 3 ALT4 OUT>;
+					};
+				};
+			};
+			mmc1 {
+				pinctrl_mmc1: mmc1 {
+					st,pins {
+						mmcclk = <&PIO15 0 ALT3 BIDIR_PU NICLK 0 CLK_B>;
+						data0 = <&PIO13 7 ALT3 BIDIR_PU BYPASS 0>;
+						data1 = <&PIO14 1 ALT3 BIDIR_PU BYPASS 0>;
+						data2 = <&PIO14 2 ALT3 BIDIR_PU BYPASS 0>;
+						data3 = <&PIO14 3 ALT3 BIDIR_PU BYPASS 0>;
+						cmd = <&PIO15 4 ALT3 BIDIR_PU BYPASS 0>;
+						data4 = <&PIO15 6 ALT3 BIDIR_PU BYPASS 0>;
+						data5 = <&PIO15 7 ALT3 BIDIR_PU BYPASS 0>;
+						data6 = <&PIO16 0 ALT3 BIDIR_PU BYPASS 0>;
+						data7 = <&PIO16 1 ALT3 BIDIR_PU BYPASS 0>;
+						pwr = <&PIO16 2 ALT3 OUT>;
+						nreset = <&PIO13 6 ALT3 OUT>;
+					};
+				};
+			};
 		};
 
 		pin-controller-fvdp-fe {
-- 
1.9.1

[PATCH 4/8] ARM: STi: DT: Add sdhci controller for stih416

[Peter Griffin]

This patch adds device tree config for both sdhci controllers
on the stih416 SoC.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 arch/arm/boot/dts/stih416.dtsi | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm/boot/dts/stih416.dtsi b/arch/arm/boot/dts/stih416.dtsi
index 06473c5..a035df0 100644
--- a/arch/arm/boot/dts/stih416.dtsi
+++ b/arch/arm/boot/dts/stih416.dtsi
@@ -236,5 +236,29 @@
 			resets	= <&powerdown STIH416_KEYSCAN_POWERDOWN>,
 				  <&softreset STIH416_KEYSCAN_SOFTRESET>;
 		};
+
+		mmc0: sdhci@fe81e000 {
+			compatible = "st,sdhci";
+			status = "disabled";
+			reg = <0xfe81e000 0x1000>;
+			interrupts = <GIC_SPI 127 IRQ_TYPE_NONE>;
+			interrupt-names = "mmcirq";
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_mmc0>;
+			clock-names     = "mmc";
+			clocks = <&clk_s_a1_ls 1>;
+		};
+
+		mmc1: sdhci@fe81f000 {
+			compatible = "st,sdhci";
+			status = "disabled";
+			reg = <0xfe81f000 0x1000>;
+			interrupts = <GIC_SPI 128 IRQ_TYPE_NONE>;
+			interrupt-names = "mmcirq";
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_mmc1>;
+			clock-names     = "mmc";
+			clocks = <&clk_s_a1_ls 8>;
+		};
 	};
 };
-- 
1.9.1

[PATCH 5/8] ARM: STi: DT: Add sdhci pin configuration for stih415

[Peter Griffin]

This patch adds the required pin config for the sdhci controller
present in the stih415 SoC.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 arch/arm/boot/dts/stih415-pinctrl.dtsi | 21 +++++++++++++++++++++
 arch/arm/boot/dts/stih415.dtsi         | 12 ++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/arch/arm/boot/dts/stih415-pinctrl.dtsi b/arch/arm/boot/dts/stih415-pinctrl.dtsi
index caeac7e..eee2373 100644
--- a/arch/arm/boot/dts/stih415-pinctrl.dtsi
+++ b/arch/arm/boot/dts/stih415-pinctrl.dtsi
@@ -429,6 +429,27 @@
 					};
 				};
 			};
+
+			mmc0 {
+				pinctrl_mmc0: mmc0 {
+					st,pins {
+						mmcclk = <&PIO13 4 ALT4 BIDIR_PU NICLK 0 CLK_B>;
+						data0  = <&PIO14 4 ALT4 BIDIR_PU BYPASS 0>;
+						data1  = <&PIO14 5 ALT4 BIDIR_PU BYPASS 0>;
+						data2  = <&PIO14 6 ALT4 BIDIR_PU BYPASS 0>;
+						data3  = <&PIO14 7 ALT4 BIDIR_PU BYPASS 0>;
+						cmd    = <&PIO15 1 ALT4 BIDIR_PU BYPASS 0>;
+						wp     = <&PIO15 3 ALT4 IN>;
+						data4  = <&PIO16 4 ALT4 BIDIR_PU BYPASS 0>;
+						data5  = <&PIO16 5 ALT4 BIDIR_PU BYPASS 0>;
+						data6  = <&PIO16 6 ALT4 BIDIR_PU BYPASS 0>;
+						data7  = <&PIO16 7 ALT4 BIDIR_PU BYPASS 0>;
+						pwr    = <&PIO17 1 ALT4 OUT>;
+						cd     = <&PIO17 2 ALT4 IN>;
+						led    = <&PIO17 3 ALT4 OUT>;
+					};
+				};
+			};
 		};
 
 		pin-controller-left {
diff --git a/arch/arm/boot/dts/stih415.dtsi b/arch/arm/boot/dts/stih415.dtsi
index d6f254f..6579b1d 100644
--- a/arch/arm/boot/dts/stih415.dtsi
+++ b/arch/arm/boot/dts/stih415.dtsi
@@ -218,5 +218,17 @@
 			resets	= <&powerdown STIH415_KEYSCAN_POWERDOWN>,
 				  <&softreset STIH415_KEYSCAN_SOFTRESET>;
 		};
+
+		mmc0: sdhci@fe81e000 {
+			compatible = "st,sdhci";
+			status = "disabled";
+			reg = <0xfe81e000 0x1000>;
+			interrupts = <GIC_SPI 145 IRQ_TYPE_NONE>;
+			interrupt-names = "mmcirq";
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_mmc0>;
+			clock-names = "mmc";
+			clocks = <&clk_s_a1_ls 1>;
+		};
 	};
 };
-- 
1.9.1

[PATCH 6/8] ARM: STi: DT: Enable mmc0 for both stih415 and stih416 SoCs

[Peter Griffin]

Because the first sdhci controller is present on both stih415 and
stih416 SoC which can both populate the b2020 board, it can be
enabled in the generic DT file.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 arch/arm/boot/dts/stih41x-b2020x.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/stih41x-b2020x.dtsi b/arch/arm/boot/dts/stih41x-b2020x.dtsi
index df01c12..f797a06 100644
--- a/arch/arm/boot/dts/stih41x-b2020x.dtsi
+++ b/arch/arm/boot/dts/stih41x-b2020x.dtsi
@@ -8,6 +8,10 @@
  */
 / {
 	soc {
+		mmc0: sdhci@fe81e000 {
+			status = "okay";
+		};
+
 		spifsm: spifsm@fe902000 {
 			#address-cells = <1>;
 			#size-cells    = <1>;
-- 
1.9.1

[PATCH 7/8] ARM: STi: DT: Enable second sdhci controller for stih416 b2020 boards.

[Peter Griffin]

The second controller is only present on the stih416 SoC. Also
mark this as non-removeable as its eMMC.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 arch/arm/boot/dts/stih416-b2020.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/stih416-b2020.dts b/arch/arm/boot/dts/stih416-b2020.dts
index 4e2df66..6cde745 100644
--- a/arch/arm/boot/dts/stih416-b2020.dts
+++ b/arch/arm/boot/dts/stih416-b2020.dts
@@ -12,4 +12,12 @@
 / {
 	model = "STiH416 B2020";
 	compatible = "st,stih416-b2020", "st,stih416";
+
+	soc {
+		mmc1: sdhci@fe81f000 {
+			status = "okay";
+			non-removable;
+		};
+	};
+
 };
-- 
1.9.1

[PATCH 8/8] ARM: update multi_v7_defconfig for STI

[Peter Griffin]

This patch enables SDHCI STI platform driver.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
 arch/arm/configs/multi_v7_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index d4e8a47..48951fc 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -277,6 +277,7 @@ CONFIG_MMC_SDHCI_TEGRA=y
 CONFIG_MMC_SDHCI_DOVE=y
 CONFIG_MMC_SDHCI_SPEAR=y
 CONFIG_MMC_SDHCI_BCM_KONA=y
+CONFIG_MMC_SDHCI_ST=y
 CONFIG_MMC_OMAP=y
 CONFIG_MMC_OMAP_HS=y
 CONFIG_MMC_MVSDIO=y
-- 
1.9.1

Re: [PATCH 8/8] ARM: update multi_v7_defconfig for STI

[Lee Jones]

> This patch enables SDHCI STI platform driver.
> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
>  arch/arm/configs/multi_v7_defconfig | 1 +
>  1 file changed, 1 insertion(+)

Acked-by: Lee Jones <lee.jones@linaro.org>

> diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
> index d4e8a47..48951fc 100644
> --- a/arch/arm/configs/multi_v7_defconfig
> +++ b/arch/arm/configs/multi_v7_defconfig
> @@ -277,6 +277,7 @@ CONFIG_MMC_SDHCI_TEGRA=y
>  CONFIG_MMC_SDHCI_DOVE=y
>  CONFIG_MMC_SDHCI_SPEAR=y
>  CONFIG_MMC_SDHCI_BCM_KONA=y
> +CONFIG_MMC_SDHCI_ST=y
>  CONFIG_MMC_OMAP=y
>  CONFIG_MMC_OMAP_HS=y
>  CONFIG_MMC_MVSDIO=y

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 7/8] ARM: STi: DT: Enable second sdhci controller for stih416 b2020 boards.

[Lee Jones]

On Thu, 22 May 2014, Peter Griffin wrote:

> The second controller is only present on the stih416 SoC. Also
> mark this as non-removeable as its eMMC.
> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
>  arch/arm/boot/dts/stih416-b2020.dts | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/stih416-b2020.dts b/arch/arm/boot/dts/stih416-b2020.dts
> index 4e2df66..6cde745 100644
> --- a/arch/arm/boot/dts/stih416-b2020.dts
> +++ b/arch/arm/boot/dts/stih416-b2020.dts
> @@ -12,4 +12,12 @@
>  / {
>  	model = "STiH416 B2020";
>  	compatible = "st,stih416-b2020", "st,stih416";
> +
> +	soc {
> +		mmc1: sdhci@fe81f000 {
> +			status = "okay";
> +			non-removable;
> +		};
> +	};
> +

Superfluous '\n'.

>  };

Once rectified you can add my:
  Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/8] mmc: sdhci-st: Intial support for ST SDHCI controller

[Maxime Coquelin]

Hi Peter

On 05/22/2014 05:18 PM, Peter Griffin wrote:
> This platform driver adds initial support for the SDHCI host controller
> found on STMicroelectronics SoCs.
>
> It has been tested on STiH41x b2020 platforms currently.
>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
>   drivers/mmc/host/Kconfig    |  12 +++
>   drivers/mmc/host/Makefile   |   1 +
>   drivers/mmc/host/sdhci-st.c | 244 ++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 257 insertions(+)
>   create mode 100644 drivers/mmc/host/sdhci-st.c
>

[...]

> diff --git a/drivers/mmc/host/sdhci-st.c b/drivers/mmc/host/sdhci-st.c
> new file mode 100644
> index 0000000..1790fa7
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-st.c
> @@ -0,0 +1,244 @@
> +/*
> + * Support for SDHCI on STMicroelectronics SoCs
> + *
> + * Copyright (C) 2014 STMicroelectronics Ltd
> + * Author: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> + *
> + * Based on sdhci-cns3xxx.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License 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/io.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/of_gpio.h>
> +#include <linux/mmc/host.h>
> +#include <linux/reset.h>
> +
> +#include "sdhci-pltfm.h"
> +
> +struct st_mmc_platform_data {
> +	struct  reset_control	*rstc;
> +};
Since it uses the generic reset framework, could we imagine moving the 
reset to the sdhci_pltfm_host struct?
Doing this, we could get rid of st_mmc_platform_data.


> +
> +static int sdhci_st_8bit_width(struct sdhci_host *host, int width)
> +{
> +	u8 ctrl;
> +
> +	ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
> +
> +	switch (width) {
> +	case MMC_BUS_WIDTH_8:
> +		ctrl |= SDHCI_CTRL_8BITBUS;
> +		ctrl &= ~SDHCI_CTRL_4BITBUS;
> +		break;
> +	case MMC_BUS_WIDTH_4:
> +		ctrl |= SDHCI_CTRL_4BITBUS;
> +		ctrl &= ~SDHCI_CTRL_8BITBUS;
> +		break;
> +	default:
> +		ctrl &= ~(SDHCI_CTRL_8BITBUS | SDHCI_CTRL_4BITBUS);
> +		break;
You can remove the break here.

> +	}
> +
> +	sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
> +
> +	return 0;
> +}
> +
> +static unsigned int sdhci_st_get_max_clk(struct sdhci_host *host)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +
> +	return clk_get_rate(pltfm_host->clk);
> +}
> +
> +static u32 sdhci_st_readl(struct sdhci_host *host, int reg)
> +{
> +	u32 ret;
> +
> +	switch (reg) {
> +	case SDHCI_CAPABILITIES:
> +		ret = readl(host->ioaddr + reg);
> +		/* Support 3.3V and 1.8V */
> +		ret &= ~SDHCI_CAN_VDD_300;
> +		break;
> +	default:
> +		ret = readl(host->ioaddr + reg);

Could we use readl_relaxed?

> +	}
> +	return ret;
> +}
> +
> +static const struct sdhci_ops sdhci_st_ops = {
> +	.get_max_clock = sdhci_st_get_max_clk,
> +	.platform_bus_width = sdhci_st_8bit_width,
> +	.read_l = sdhci_st_readl,
> +};
> +
> +static const struct sdhci_pltfm_data sdhci_st_pdata = {
> +	.ops = &sdhci_st_ops,
> +	.quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC |
> +	    SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> +};
> +
> +
> +static int sdhci_st_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct sdhci_host *host;
> +	struct st_mmc_platform_data *pdata;
> +	struct sdhci_pltfm_host *pltfm_host;
> +	struct clk *clk;
> +	int ret = 0;
> +	u16 host_version;
> +
> +	dev_dbg(&pdev->dev, "SDHCI ST platform driver\n");
You can remove this I think.

> +
> +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;
> +
> +	pdata->rstc = devm_reset_control_get(&pdev->dev, NULL);
> +	if (IS_ERR(pdata->rstc))
> +		pdata->rstc = NULL;
> +	else
> +		reset_control_deassert(pdata->rstc);
> +
> +	clk =  devm_clk_get(&pdev->dev, "mmc");
> +	if (IS_ERR(clk)) {
> +		dev_err(&pdev->dev, "Perpheral clk not found\n");
> +		return PTR_ERR(clk);
> +	}
> +
> +	host = sdhci_pltfm_init(pdev, &sdhci_st_pdata, 0);
> +	if (IS_ERR(host)) {
> +		dev_err(&pdev->dev, "Failed sdhci_pltfm_init\n");
> +		return PTR_ERR(host);
> +	}
> +
> +	clk_prepare_enable(clk);
> +
> +	host->mmc->caps |= MMC_CAP_8_BIT_DATA | MMC_CAP_BUS_WIDTH_TEST
> +			| MMC_CAP_1_8V_DDR;
> +
> +	if (of_property_read_bool(np, "non-removable"))
> +		host->mmc->caps |= MMC_CAP_NONREMOVABLE;
> +
> +	pltfm_host = sdhci_priv(host);
> +	pltfm_host->clk = clk;
> +
> +	ret = sdhci_add_host(host);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed sdhci_add_host\n");
> +		goto err_out;
> +	}
> +
> +	pltfm_host->priv = pdata;
> +
> +	platform_set_drvdata(pdev, host);
> +
> +	host_version = readw_relaxed((host->ioaddr + SDHCI_HOST_VERSION));
> +
> +	dev_dbg(&pdev->dev, "SDHCI ST Initialised: Host Version: 0x%x Vendor Version 0x%x\n",
> +		host_version, ((host_version & SDHCI_VENDOR_VER_MASK) >>
> +			       SDHCI_VENDOR_VER_SHIFT));
Maybe you could change to dev_info here. It might be interresting to 
always have IP version.

> +
> +	return 0;
> +
> +err_out:
> +	clk_disable_unprepare(clk);
> +	sdhci_pltfm_free(pdev);
> +
> +	return ret;
> +}

[...]

Regards,
Maxime

Re: [PATCH 6/8] ARM: STi: DT: Enable mmc0 for both stih415 and stih416 SoCs

[Lee Jones]

On Thu, 22 May 2014, Peter Griffin wrote:

> Because the first sdhci controller is present on both stih415 and
> stih416 SoC which can both populate the b2020 board, it can be
> enabled in the generic DT file.
> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>

These are the wrong way around.

You can correct that when adding my:

  Acked-by: Lee Jones <lee.jones@linaro.org>

> ---
>  arch/arm/boot/dts/stih41x-b2020x.dtsi | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/stih41x-b2020x.dtsi b/arch/arm/boot/dts/stih41x-b2020x.dtsi
> index df01c12..f797a06 100644
> --- a/arch/arm/boot/dts/stih41x-b2020x.dtsi
> +++ b/arch/arm/boot/dts/stih41x-b2020x.dtsi
> @@ -8,6 +8,10 @@
>   */
>  / {
>  	soc {
> +		mmc0: sdhci@fe81e000 {
> +			status = "okay";
> +		};
> +
>  		spifsm: spifsm@fe902000 {
>  			#address-cells = <1>;
>  			#size-cells    = <1>;

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: Add SDHCI support for STMicroelectronics SoCs

[Maxime Coquelin]

Hi Peter,

On 05/22/2014 05:18 PM, Peter Griffin wrote:
> This series adds a SDHCI platform driver for ST SoCs, along with
> the additional device tree bindings and configuration to enable
> the controller to work properly.
>
> Initially it supports the stih416 and stih415 SoCs, and has
> been tested on a stih416-b2020 board.

In next version, could you also add  drivers/mmc/host/sdhci-st.c in the 
ARM/STI ARCHITECTURE field of MAINTAINERS file?


Thanks,
Maxime


>
> regards,
>
> Peter.
>

Re: [PATCH 2/8] mmc: sdhci-st: STMicroelectronics SDHCI binding documentation.

[Maxime Coquelin]

On 05/22/2014 05:18 PM, Peter Griffin wrote:
> This patch adds the device tree binding documentation for ST
> SDHCI driver. It contains the differences between the core properties
> in mmc.txt and the properties used by the sdhci-st driver.
>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
>   Documentation/devicetree/bindings/mmc/sdhci-st.txt | 26 ++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-st.txt
>

You can add my:
Acked-by: Maxime Coquelin <maxime.coquelin@st.com>

[...]

Re: [PATCH 3/8] ARM: STi: DT: Add sdhci pins for stih416

[Maxime Coquelin]

On 05/22/2014 05:18 PM, Peter Griffin wrote:
> This adds the required pin config for both SDHCI controllers on
> the stih416 SoC.
>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
>   arch/arm/boot/dts/stih416-pinctrl.dtsi | 39 ++++++++++++++++++++++++++++++++++
>   1 file changed, 39 insertions(+)
>


For all the DT patches, once Lee's comments taken into account, you can 
add my:

Acked-by: Maxime Coquelin <maxime.coquelin@st.com>

Thanks!
Maxime

Re: [PATCH 5/8] ARM: STi: DT: Add sdhci pin configuration for stih415

[Lee Jones]

> This patch adds the required pin config for the sdhci controller
> present in the stih415 SoC.
> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>

Switch these round - same with all the other patches.

> ---
>  arch/arm/boot/dts/stih415-pinctrl.dtsi | 21 +++++++++++++++++++++
>  arch/arm/boot/dts/stih415.dtsi         | 12 ++++++++++++
>  2 files changed, 33 insertions(+)

[...]

> diff --git a/arch/arm/boot/dts/stih415.dtsi b/arch/arm/boot/dts/stih415.dtsi
> index d6f254f..6579b1d 100644
> --- a/arch/arm/boot/dts/stih415.dtsi
> +++ b/arch/arm/boot/dts/stih415.dtsi
> @@ -218,5 +218,17 @@
>  			resets	= <&powerdown STIH415_KEYSCAN_POWERDOWN>,
>  				  <&softreset STIH415_KEYSCAN_SOFTRESET>;
>  		};
> +
> +		mmc0: sdhci@fe81e000 {
> +			compatible = "st,sdhci";
> +			status = "disabled";
> +			reg = <0xfe81e000 0x1000>;
> +			interrupts = <GIC_SPI 145 IRQ_TYPE_NONE>;
> +			interrupt-names = "mmcirq";
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&pinctrl_mmc0>;
> +			clock-names = "mmc";
> +			clocks = <&clk_s_a1_ls 1>;

Nit: These would be easier to read if the '='s were lined up (using tabs).

> +		};
>  	};
>  };

Apart from that:

  Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 4/8] ARM: STi: DT: Add sdhci controller for stih416

[Lee Jones]

On Thu, 22 May 2014, Peter Griffin wrote:

> This patch adds device tree config for both sdhci controllers
> on the stih416 SoC.
> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
>  arch/arm/boot/dts/stih416.dtsi | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/stih416.dtsi b/arch/arm/boot/dts/stih416.dtsi
> index 06473c5..a035df0 100644
> --- a/arch/arm/boot/dts/stih416.dtsi
> +++ b/arch/arm/boot/dts/stih416.dtsi
> @@ -236,5 +236,29 @@
>  			resets	= <&powerdown STIH416_KEYSCAN_POWERDOWN>,
>  				  <&softreset STIH416_KEYSCAN_SOFTRESET>;
>  		};
> +
> +		mmc0: sdhci@fe81e000 {
> +			compatible = "st,sdhci";
> +			status = "disabled";
> +			reg = <0xfe81e000 0x1000>;
> +			interrupts = <GIC_SPI 127 IRQ_TYPE_NONE>;
> +			interrupt-names = "mmcirq";
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&pinctrl_mmc0>;
> +			clock-names     = "mmc";
> +			clocks = <&clk_s_a1_ls 1>;
> +		};
> +
> +		mmc1: sdhci@fe81f000 {
> +			compatible = "st,sdhci";
> +			status = "disabled";
> +			reg = <0xfe81f000 0x1000>;
> +			interrupts = <GIC_SPI 128 IRQ_TYPE_NONE>;
> +			interrupt-names = "mmcirq";
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&pinctrl_mmc1>;
> +			clock-names     = "mmc";
> +			clocks = <&clk_s_a1_ls 8>;
> +		};

Line these up and make them look pretty too.

After that:

  Acked-by: Lee Jones <lee.jones@linaro.org>

>  	};
>  };

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/8] ARM: STi: DT: Add sdhci pins for stih416

[Lee Jones]

On Thu, 22 May 2014, Peter Griffin wrote:

> This adds the required pin config for both SDHCI controllers on
> the stih416 SoC.
> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
>  arch/arm/boot/dts/stih416-pinctrl.dtsi | 39 ++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)

Acked-by: Lee Jones <lee.jones@linaro.org>

> diff --git a/arch/arm/boot/dts/stih416-pinctrl.dtsi b/arch/arm/boot/dts/stih416-pinctrl.dtsi
> index 6252188..140af6b 100644
> --- a/arch/arm/boot/dts/stih416-pinctrl.dtsi
> +++ b/arch/arm/boot/dts/stih416-pinctrl.dtsi
> @@ -467,6 +467,45 @@
>  					};
>  				};
>  			};
> +
> +			mmc0 {
> +				pinctrl_mmc0: mmc0 {
> +					st,pins {
> +						mmcclk = <&PIO13 4 ALT4 BIDIR_PU NICLK 0 CLK_B>;
> +						data0 = <&PIO14 4 ALT4 BIDIR_PU BYPASS 0>;
> +						data1 = <&PIO14 5 ALT4 BIDIR_PU BYPASS 0>;
> +						data2 = <&PIO14 6 ALT4 BIDIR_PU BYPASS 0>;
> +						data3 = <&PIO14 7 ALT4 BIDIR_PU BYPASS 0>;
> +						cmd = <&PIO15 1 ALT4 BIDIR_PU BYPASS 0>;
> +						wp = <&PIO15 3 ALT4 IN>;
> +						data4 = <&PIO16 4 ALT4 BIDIR_PU BYPASS 0>;
> +						data5 = <&PIO16 5 ALT4 BIDIR_PU BYPASS 0>;
> +						data6 = <&PIO16 6 ALT4 BIDIR_PU BYPASS 0>;
> +						data7 = <&PIO16 7 ALT4 BIDIR_PU BYPASS 0>;
> +						pwr = <&PIO17 1 ALT4 OUT>;
> +						cd = <&PIO17 2 ALT4 IN>;
> +						led = <&PIO17 3 ALT4 OUT>;
> +					};
> +				};
> +			};
> +			mmc1 {
> +				pinctrl_mmc1: mmc1 {
> +					st,pins {
> +						mmcclk = <&PIO15 0 ALT3 BIDIR_PU NICLK 0 CLK_B>;
> +						data0 = <&PIO13 7 ALT3 BIDIR_PU BYPASS 0>;
> +						data1 = <&PIO14 1 ALT3 BIDIR_PU BYPASS 0>;
> +						data2 = <&PIO14 2 ALT3 BIDIR_PU BYPASS 0>;
> +						data3 = <&PIO14 3 ALT3 BIDIR_PU BYPASS 0>;
> +						cmd = <&PIO15 4 ALT3 BIDIR_PU BYPASS 0>;
> +						data4 = <&PIO15 6 ALT3 BIDIR_PU BYPASS 0>;
> +						data5 = <&PIO15 7 ALT3 BIDIR_PU BYPASS 0>;
> +						data6 = <&PIO16 0 ALT3 BIDIR_PU BYPASS 0>;
> +						data7 = <&PIO16 1 ALT3 BIDIR_PU BYPASS 0>;
> +						pwr = <&PIO16 2 ALT3 OUT>;
> +						nreset = <&PIO13 6 ALT3 OUT>;
> +					};
> +				};
> +			};
>  		};
>  
>  		pin-controller-fvdp-fe {

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/8] mmc: sdhci-st: STMicroelectronics SDHCI binding documentation.

[Lee Jones]

> This patch adds the device tree binding documentation for ST
> SDHCI driver. It contains the differences between the core properties
> in mmc.txt and the properties used by the sdhci-st driver.
> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
>  Documentation/devicetree/bindings/mmc/sdhci-st.txt | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-st.txt
> 
> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-st.txt b/Documentation/devicetree/bindings/mmc/sdhci-st.txt
> new file mode 100644
> index 0000000..ae3dae0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-st.txt
> @@ -0,0 +1,26 @@
> +* STMicroelectronics sdhci-st MMC/SD controller
> +
> +This file documents differences between the core properties in mmc.txt

Where is mmc.txt?

> +and the properties used by the sdhci-st driver
> +
> +Required properties:
> +- compatible: "st,sdhci"
> +- clocks: card clock, must be "mmc".

This isn't correct.  I think you mean it must be a phandle to the
'mmc' clock.  It's also worth referencing the clock DT documentation
here.

> +Optional properties:
> +- reset: to provide a reset to the HC.

What is it; integer, string, phandle, something else?

> +- non-removable: non-removable slot (bindings/mmc/mmc.txt).

This however, _is_ a string, so you need to put it in "'s like you did
with "mmc" above and/or reference the file which documents it.

> +
> +Example:
> +
> +mmc0: sdhci@fe81e000 {
> +	compatible	= "st,sdhci";
> +	status		= "disabled";
> +	reg		= <0xfe81e000 0x1000>;
> +	interrupts	= <GIC_SPI 127 IRQ_TYPE_NONE>;
> +	interrupt-names	= "mmcirq";
> +	pinctrl-names	= "default";
> +	pinctrl-0	= <&pinctrl_mmc0>;
> +	clock-names	= "mmc";

I don't think you need clock-names if you only have one clock.

> +	clocks		= <&clk_s_a1_ls 1>;
> +};

Once fixed you can add my Ack for the next submission:

  Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/8] mmc: sdhci-st: Intial support for ST SDHCI controller

[Lee Jones]

> This platform driver adds initial support for the SDHCI host controller
> found on STMicroelectronics SoCs.
> 
> It has been tested on STiH41x b2020 platforms currently.
> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
>  drivers/mmc/host/Kconfig    |  12 +++
>  drivers/mmc/host/Makefile   |   1 +
>  drivers/mmc/host/sdhci-st.c | 244 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 257 insertions(+)
>  create mode 100644 drivers/mmc/host/sdhci-st.c

[...]

> +static int sdhci_st_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct sdhci_host *host;
> +	struct st_mmc_platform_data *pdata;
> +	struct sdhci_pltfm_host *pltfm_host;
> +	struct clk *clk;
> +	int ret = 0;
> +	u16 host_version;
> +
> +	dev_dbg(&pdev->dev, "SDHCI ST platform driver\n");
> +
> +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;
> +
> +	pdata->rstc = devm_reset_control_get(&pdev->dev, NULL);
> +	if (IS_ERR(pdata->rstc))
> +		pdata->rstc = NULL;
> +	else
> +		reset_control_deassert(pdata->rstc);
> +
> +	clk =  devm_clk_get(&pdev->dev, "mmc");
> +	if (IS_ERR(clk)) {
> +		dev_err(&pdev->dev, "Perpheral clk not found\n");
> +		return PTR_ERR(clk);
> +	}
> +
> +	host = sdhci_pltfm_init(pdev, &sdhci_st_pdata, 0);
> +	if (IS_ERR(host)) {
> +		dev_err(&pdev->dev, "Failed sdhci_pltfm_init\n");
> +		return PTR_ERR(host);
> +	}
> +
> +	clk_prepare_enable(clk);

Move this down as far as it will go.  When do you _need_ the clock
running by?

> +	host->mmc->caps |= MMC_CAP_8_BIT_DATA | MMC_CAP_BUS_WIDTH_TEST
> +			| MMC_CAP_1_8V_DDR;
> +
> +	if (of_property_read_bool(np, "non-removable"))
> +		host->mmc->caps |= MMC_CAP_NONREMOVABLE;
> +
> +	pltfm_host = sdhci_priv(host);
> +	pltfm_host->clk = clk;
> +
> +	ret = sdhci_add_host(host);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed sdhci_add_host\n");
> +		goto err_out;

If it's possible to move the clk_prepare enable down past here, then
you only need to do sdhci_pltfm_free() and you can remove all of the
err_out error path.

> +	}
> +
> +	pltfm_host->priv = pdata;
> +
> +	platform_set_drvdata(pdev, host);
> +
> +	host_version = readw_relaxed((host->ioaddr + SDHCI_HOST_VERSION));

Do we want to be doing any error checking for unsupported devices
here?

[...]

> +static struct platform_driver sdhci_st_driver = {
> +	.probe = sdhci_st_probe,
> +	.remove = sdhci_st_remove,
> +	.driver = {
> +		   .name = "sdhci-st",
> +		   .owner = THIS_MODULE,
> +		   .pm = SDHCI_ST_PMOPS,
> +		   .of_match_table = of_match_ptr(st_sdhci_match),
> +		  },

Tabbing issue here.

> +};
> +
> +module_platform_driver(sdhci_st_driver);
> +
> +MODULE_DESCRIPTION("SDHCI driver for STMicroelectronics SoCs");
> +MODULE_AUTHOR("Giuseppe Cavallaro <peppe.cavallaro@st.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:st-sdhci");

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/8] mmc: sdhci-st: Intial support for ST SDHCI controller

[Srinivas Kandagatla]

Hi Pete,

On 22/05/14 16:18, Peter Griffin wrote:

>
>   ifeq ($(CONFIG_CB710_DEBUG),y)
>   	CFLAGS-cb710-mmc	+= -DDEBUG
> diff --git a/drivers/mmc/host/sdhci-st.c b/drivers/mmc/host/sdhci-st.c
> new file mode 100644
> index 0000000..1790fa7
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-st.c
> @@ -0,0 +1,244 @@
> +/*
> + * Support for SDHCI on STMicroelectronics SoCs
> + *
> + * Copyright (C) 2014 STMicroelectronics Ltd
> + * Author: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> + *
> + * Based on sdhci-cns3xxx.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License 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/io.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/of_gpio.h>
Do you need this header?

> +#include <linux/mmc/host.h>
> +#include <linux/reset.h>
> +
> +#include "sdhci-pltfm.h"
> +
> +struct st_mmc_platform_data {
> +	struct  reset_control	*rstc;
> +};

st_mmc_platform_data name is bit missleading as this data is not part of 
platform data. Probably you could rename it to struct sdhci_st_data.
...
> +
> +static int sdhci_st_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct sdhci_host *host;
> +	struct st_mmc_platform_data *pdata;
> +	struct sdhci_pltfm_host *pltfm_host;
> +	struct clk *clk;
> +	int ret = 0;
> +	u16 host_version;
> +
> +	dev_dbg(&pdev->dev, "SDHCI ST platform driver\n");
> +
> +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;
> +
> +	pdata->rstc = devm_reset_control_get(&pdev->dev, NULL);
> +	if (IS_ERR(pdata->rstc))
> +		pdata->rstc = NULL;
> +	else
> +		reset_control_deassert(pdata->rstc);
> +
> +	clk =  devm_clk_get(&pdev->dev, "mmc");
> +	if (IS_ERR(clk)) {
> +		dev_err(&pdev->dev, "Perpheral clk not found\n");
> +		return PTR_ERR(clk);
> +	}
> +
> +	host = sdhci_pltfm_init(pdev, &sdhci_st_pdata, 0);
> +	if (IS_ERR(host)) {
> +		dev_err(&pdev->dev, "Failed sdhci_pltfm_init\n");
> +		return PTR_ERR(host);
> +	}
> +
> +	clk_prepare_enable(clk);
> +
> +	host->mmc->caps |= MMC_CAP_8_BIT_DATA | MMC_CAP_BUS_WIDTH_TEST
> +			| MMC_CAP_1_8V_DDR;
> +
> +	if (of_property_read_bool(np, "non-removable"))
> +		host->mmc->caps |= MMC_CAP_NONREMOVABLE;
> +
> +	pltfm_host = sdhci_priv(host);
> +	pltfm_host->clk = clk;
> +
> +	ret = sdhci_add_host(host);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed sdhci_add_host\n");
> +		goto err_out;
> +	}
> +
> +	pltfm_host->priv = pdata;
> +
> +	platform_set_drvdata(pdev, host);

Why not platform_set_drvdata(pdev, priv_host);
As you are not using sdhci_host directly, this will reduced few 
indirections in the driver.


> +
> +	host_version = readw_relaxed((host->ioaddr + SDHCI_HOST_VERSION));
> +
> +	dev_dbg(&pdev->dev, "SDHCI ST Initialised: Host Version: 0x%x Vendor Version 0x%x\n",
> +		host_version, ((host_version & SDHCI_VENDOR_VER_MASK) >>
> +			       SDHCI_VENDOR_VER_SHIFT));
> +
> +	return 0;
> +
> +err_out:
> +	clk_disable_unprepare(clk);
> +	sdhci_pltfm_free(pdev);
> +
IP could be left out of reset in this path.
> +	return ret;
> +}
> +
> +static int sdhci_st_remove(struct platform_device *pdev)
> +{
> +	struct sdhci_host *host = platform_get_drvdata(pdev);
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct st_mmc_platform_data *pdata = pltfm_host->priv;
> +	int ret;
> +
> +	clk_disable_unprepare(pltfm_host->clk);
> +
> +	ret = sdhci_pltfm_unregister(pdev);
> +
> +	if (pdata->rstc)
> +		reset_control_assert(pdata->rstc);
> +
> +	return ret;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int sdhci_st_suspend(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct st_mmc_platform_data *pdata = pltfm_host->priv;
> +	int ret = sdhci_suspend_host(host);
> +
> +	if (ret)
> +		goto out;
> +
> +	if (pdata->rstc)
> +		reset_control_assert(pdata->rstc);
> +
> +	clk_disable_unprepare(pltfm_host->clk);
> +out:
> +	return ret;
> +}
> +
> +static int sdhci_st_resume(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct st_mmc_platform_data *pdata = pltfm_host->priv;
> +
> +	clk_prepare_enable(pltfm_host->clk);
> +
> +	if (pdata->rstc)
> +		reset_control_deassert(pdata->rstc);
> +
> +	return sdhci_resume_host(host);
> +}
> +

replace:
[...
> +static SIMPLE_DEV_PM_OPS(sdhci_st_pmops, sdhci_st_suspend, sdhci_st_resume);
> +#define SDHCI_ST_PMOPS (&sdhci_st_pmops)
> +#else
> +#define SDHCI_ST_PMOPS NULL
> +#endif
...]

with :

#endif

static SIMPLE_DEV_PM_OPS(sdhci_st_pmops, sdhci_st_suspend, sdhci_st_resume);

	
> +
> +static const struct of_device_id st_sdhci_match[] = {
> +	{ .compatible = "st,sdhci" },
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(of, st_sdhci_match);
> +
> +static struct platform_driver sdhci_st_driver = {
> +	.probe = sdhci_st_probe,
> +	.remove = sdhci_st_remove,
> +	.driver = {
> +		   .name = "sdhci-st",
> +		   .owner = THIS_MODULE,
> +		   .pm = SDHCI_ST_PMOPS,
use .pm = sdhci_st_pmops,

> +		   .of_match_table = of_match_ptr(st_sdhci_match),
> +		  },
> +};
> +
> +module_platform_driver(sdhci_st_driver);
> +
> +MODULE_DESCRIPTION("SDHCI driver for STMicroelectronics SoCs");
> +MODULE_AUTHOR("Giuseppe Cavallaro <peppe.cavallaro@st.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:st-sdhci");
>

Re: [PATCH 2/8] mmc: sdhci-st: STMicroelectronics SDHCI binding documentation.

[Ulf Hansson]

On 22 May 2014 17:18, Peter Griffin <peter.griffin@linaro.org> wrote:
> This patch adds the device tree binding documentation for ST
> SDHCI driver. It contains the differences between the core properties
> in mmc.txt and the properties used by the sdhci-st driver.
>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>

Assuming this patch adds does what is says; the patch1 - adding the
actual driver will have checkpatch warnings around missing
DT-bindings.

Kind regards
Ulf Hansson

> ---
>  Documentation/devicetree/bindings/mmc/sdhci-st.txt | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-st.txt
>
> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-st.txt b/Documentation/devicetree/bindings/mmc/sdhci-st.txt
> new file mode 100644
> index 0000000..ae3dae0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-st.txt
> @@ -0,0 +1,26 @@
> +* STMicroelectronics sdhci-st MMC/SD controller
> +
> +This file documents differences between the core properties in mmc.txt
> +and the properties used by the sdhci-st driver
> +
> +Required properties:
> +- compatible: "st,sdhci"
> +- clocks: card clock, must be "mmc".
> +
> +Optional properties:
> +- reset: to provide a reset to the HC.
> +- non-removable: non-removable slot (bindings/mmc/mmc.txt).
> +
> +Example:
> +
> +mmc0: sdhci@fe81e000 {
> +       compatible      = "st,sdhci";
> +       status          = "disabled";
> +       reg             = <0xfe81e000 0x1000>;
> +       interrupts      = <GIC_SPI 127 IRQ_TYPE_NONE>;
> +       interrupt-names = "mmcirq";
> +       pinctrl-names   = "default";
> +       pinctrl-0       = <&pinctrl_mmc0>;
> +       clock-names     = "mmc";
> +       clocks          = <&clk_s_a1_ls 1>;
> +};
> --
> 1.9.1
>

Re: [PATCH 1/8] mmc: sdhci-st: Intial support for ST SDHCI controller

[Ulf Hansson]

On 22 May 2014 17:18, Peter Griffin <peter.griffin@linaro.org> wrote:
> This platform driver adds initial support for the SDHCI host controller
> found on STMicroelectronics SoCs.
>
> It has been tested on STiH41x b2020 platforms currently.
>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
>  drivers/mmc/host/Kconfig    |  12 +++
>  drivers/mmc/host/Makefile   |   1 +
>  drivers/mmc/host/sdhci-st.c | 244 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 257 insertions(+)
>  create mode 100644 drivers/mmc/host/sdhci-st.c
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 8aaf8c1..b62c40d 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -283,6 +283,18 @@ config MMC_SDHCI_BCM2835
>
>           If unsure, say N.
>
> +config MMC_SDHCI_ST
> +       tristate "SDHCI support on STMicroelectronics SoC"
> +       depends on ARCH_STI
> +       depends on MMC_SDHCI_PLTFM
> +       select MMC_SDHCI_IO_ACCESSORS
> +       help
> +         This selects the Secure Digital Host Controller Interface in
> +         STMicroelectronics SoCs.
> +
> +         If you have a controller with this interface, say Y or M here.
> +         If unsure, say N.
> +
>  config MMC_OMAP
>         tristate "TI OMAP Multimedia Card Interface support"
>         depends on ARCH_OMAP
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index 0c8aa5e..6e0acf7 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -64,6 +64,7 @@ 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
> +obj-$(CONFIG_MMC_SDHCI_ST)             += sdhci-st.o
>
>  ifeq ($(CONFIG_CB710_DEBUG),y)
>         CFLAGS-cb710-mmc        += -DDEBUG
> diff --git a/drivers/mmc/host/sdhci-st.c b/drivers/mmc/host/sdhci-st.c
> new file mode 100644
> index 0000000..1790fa7
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-st.c
> @@ -0,0 +1,244 @@
> +/*
> + * Support for SDHCI on STMicroelectronics SoCs
> + *
> + * Copyright (C) 2014 STMicroelectronics Ltd
> + * Author: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> + *
> + * Based on sdhci-cns3xxx.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License 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/io.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/of_gpio.h>
> +#include <linux/mmc/host.h>
> +#include <linux/reset.h>
> +
> +#include "sdhci-pltfm.h"
> +
> +struct st_mmc_platform_data {
> +       struct  reset_control   *rstc;
> +};
> +
> +static int sdhci_st_8bit_width(struct sdhci_host *host, int width)
> +{
> +       u8 ctrl;
> +
> +       ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
> +
> +       switch (width) {
> +       case MMC_BUS_WIDTH_8:
> +               ctrl |= SDHCI_CTRL_8BITBUS;
> +               ctrl &= ~SDHCI_CTRL_4BITBUS;
> +               break;
> +       case MMC_BUS_WIDTH_4:
> +               ctrl |= SDHCI_CTRL_4BITBUS;
> +               ctrl &= ~SDHCI_CTRL_8BITBUS;
> +               break;
> +       default:
> +               ctrl &= ~(SDHCI_CTRL_8BITBUS | SDHCI_CTRL_4BITBUS);
> +               break;
> +       }
> +
> +       sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
> +
> +       return 0;
> +}
> +
> +static unsigned int sdhci_st_get_max_clk(struct sdhci_host *host)
> +{
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +
> +       return clk_get_rate(pltfm_host->clk);
> +}

There are sdhci library function for the above:
sdhci_pltfm_clk_get_max_clock()

> +
> +static u32 sdhci_st_readl(struct sdhci_host *host, int reg)
> +{
> +       u32 ret;
> +
> +       switch (reg) {
> +       case SDHCI_CAPABILITIES:
> +               ret = readl(host->ioaddr + reg);
> +               /* Support 3.3V and 1.8V */
> +               ret &= ~SDHCI_CAN_VDD_300;
> +               break;
> +       default:
> +               ret = readl(host->ioaddr + reg);
> +       }
> +       return ret;
> +}
> +
> +static const struct sdhci_ops sdhci_st_ops = {
> +       .get_max_clock = sdhci_st_get_max_clk,
> +       .platform_bus_width = sdhci_st_8bit_width,
> +       .read_l = sdhci_st_readl,
> +};
> +
> +static const struct sdhci_pltfm_data sdhci_st_pdata = {
> +       .ops = &sdhci_st_ops,
> +       .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC |
> +           SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> +};
> +
> +
> +static int sdhci_st_probe(struct platform_device *pdev)
> +{
> +       struct device_node *np = pdev->dev.of_node;
> +       struct sdhci_host *host;
> +       struct st_mmc_platform_data *pdata;
> +       struct sdhci_pltfm_host *pltfm_host;
> +       struct clk *clk;
> +       int ret = 0;
> +       u16 host_version;
> +
> +       dev_dbg(&pdev->dev, "SDHCI ST platform driver\n");
> +
> +       pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +       if (!pdata)
> +               return -ENOMEM;
> +
> +       pdata->rstc = devm_reset_control_get(&pdev->dev, NULL);
> +       if (IS_ERR(pdata->rstc))
> +               pdata->rstc = NULL;
> +       else
> +               reset_control_deassert(pdata->rstc);
> +
> +       clk =  devm_clk_get(&pdev->dev, "mmc");
> +       if (IS_ERR(clk)) {
> +               dev_err(&pdev->dev, "Perpheral clk not found\n");
> +               return PTR_ERR(clk);
> +       }
> +
> +       host = sdhci_pltfm_init(pdev, &sdhci_st_pdata, 0);
> +       if (IS_ERR(host)) {
> +               dev_err(&pdev->dev, "Failed sdhci_pltfm_init\n");
> +               return PTR_ERR(host);
> +       }
> +
> +       clk_prepare_enable(clk);
> +
> +       host->mmc->caps |= MMC_CAP_8_BIT_DATA | MMC_CAP_BUS_WIDTH_TEST
> +                       | MMC_CAP_1_8V_DDR;

Comment below.

> +
> +       if (of_property_read_bool(np, "non-removable"))
> +               host->mmc->caps |= MMC_CAP_NONREMOVABLE;

MMC_CAP_1_8V_DDR, MMC_CAP_8_BIT_DATA, MMC_CAP_NONREMOVABLE aren’t
those board specific capabilities?

Unless there are something that prevents you from using the common mmc
DT parser, I would suggest you to use it. mmc_of_parse(). Thus you can
provide these capabilities through DT instead.

Kind regards
Ulf Hansson

Re: Add SDHCI support for STMicroelectronics SoCs

[Peter Griffin]

Hi Maxime,

On Thu, 22 May 2014, Maxime Coquelin wrote:

> >Initially it supports the stih416 and stih415 SoCs, and has
> >been tested on a stih416-b2020 board.
> 
> In next version, could you also add  drivers/mmc/host/sdhci-st.c in
> the ARM/STI ARCHITECTURE field of MAINTAINERS file?

Was out on hols last week. I have fixed it in v2.

Cheers,

Peter

Re: [PATCH 1/8] mmc: sdhci-st: Intial support for ST SDHCI controller

[Peter Griffin]

Hi Lee,

Thanks for your feedback, all your other comments will be fixed in v2. 
However see comments below for this patch

> > +	clk_prepare_enable(clk);
> 
> Move this down as far as it will go.  When do you _need_ the clock
> running by?
> 
> > +	host->mmc->caps |= MMC_CAP_8_BIT_DATA | MMC_CAP_BUS_WIDTH_TEST
> > +			| MMC_CAP_1_8V_DDR;
> > +
> > +	if (of_property_read_bool(np, "non-removable"))
> > +		host->mmc->caps |= MMC_CAP_NONREMOVABLE;
> > +
> > +	pltfm_host = sdhci_priv(host);
> > +	pltfm_host->clk = clk;
> > +
> > +	ret = sdhci_add_host(host);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed sdhci_add_host\n");
> > +		goto err_out;
> 
> If it's possible to move the clk_prepare enable down past here, then
> you only need to do sdhci_pltfm_free() and you can remove all of the
> err_out error path.

No its not possible. sdhci_add_host() reads registers on
the IP, if the clock isn't enabled the system can hang.

> 
> > +	}
> > +
> > +	pltfm_host->priv = pdata;
> > +
> > +	platform_set_drvdata(pdev, host);
> > +
> > +	host_version = readw_relaxed((host->ioaddr + SDHCI_HOST_VERSION));
> 
> Do we want to be doing any error checking for unsupported devices
> here?

Not that I'm aware of. This is just a debug print I added as it is useful
for debugging. Its not unheard for software folks not to be told that the
IP version has changed in a new SoC, so comparing dmesg traces of working
kernels with non working ones which include IP versions etc can often
shed some light on whats happening.

Arguably if the maintainers think its helpful then it could be added in 
sdhci_add_host(), and every platform would benefit. Or if its deemed 
not helpful then it can be removed!

Cheers,

Peter.

Re: Add SDHCI support for STMicroelectronics SoCs

[Lee Jones]

> > >Initially it supports the stih416 and stih415 SoCs, and has
> > >been tested on a stih416-b2020 board.
> > 
> > In next version, could you also add  drivers/mmc/host/sdhci-st.c in
> > the ARM/STI ARCHITECTURE field of MAINTAINERS file?
> 
> Was out on hols last week. I have fixed it in v2.

This should be in its own patch.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/8] mmc: sdhci-st: Intial support for ST SDHCI controller

[Peter Griffin]

Hi Srini,

Thanks for reviewing, see my comments below: -

On Fri, 23 May 2014, Srinivas Kandagatla wrote:

> >+struct st_mmc_platform_data {
> >+	struct  reset_control	*rstc;
> >+};
> 
> st_mmc_platform_data name is bit missleading as this data is not
> part of platform data. Probably you could rename it to struct
> sdhci_st_data.

I've now removed this reset controller code for v2, as based on Maxime's feedback
I think this would be better off going in the generic code, so all 
platforms could benefit if they have a reset controller implementation.

I plan to send some RFC patches which implement this seperately to this series.

> >+	pltfm_host->priv = pdata;
> >+
> >+	platform_set_drvdata(pdev, host);
> 
> Why not platform_set_drvdata(pdev, priv_host);
> As you are not using sdhci_host directly, this will reduced few
> indirections in the driver.

Your right, and I was going to change this, but with the re-think on the reset
controller code above I will now need sdhci_host so would prefer to leave it as
is for now.

> >+err_out:
> >+	clk_disable_unprepare(clk);
> >+	sdhci_pltfm_free(pdev);
> >+
> IP could be left out of reset in this path.

Good spot, I'll remember this when I add the reset code
back in :-)

> 
> replace:
> [...
> >+static SIMPLE_DEV_PM_OPS(sdhci_st_pmops, sdhci_st_suspend, sdhci_st_resume);
> >+#define SDHCI_ST_PMOPS (&sdhci_st_pmops)
> >+#else
> >+#define SDHCI_ST_PMOPS NULL
> >+#endif
> ...]
> 
> with :
> 
> #endif
> 
> static SIMPLE_DEV_PM_OPS(sdhci_st_pmops, sdhci_st_suspend, sdhci_st_resume);

Fixed in v2

regards,

Peter.

Re: [PATCH 1/8] mmc: sdhci-st: Intial support for ST SDHCI controller

[Peter Griffin]

Hi Maxime,

Thanks fo reviewing, see my comments below: -

> >+struct st_mmc_platform_data {
> >+	struct  reset_control	*rstc;
> >+};
> Since it uses the generic reset framework, could we imagine moving
> the reset to the sdhci_pltfm_host struct?
> Doing this, we could get rid of st_mmc_platform_data.

Yes I agree, I will send some RFC patches which moves the reset controller
stuff into the generic code.

For v2 of this series I've removed the reset code, as stih416 doesn't have
seperate reset line each controller, so it will mainly be 
useful for stih407 SoC, which needs additional patches on top of this 
set.

> >+	switch (width) {
> >+	case MMC_BUS_WIDTH_8:
> >+		ctrl |= SDHCI_CTRL_8BITBUS;
> >+		ctrl &= ~SDHCI_CTRL_4BITBUS;
> >+		break;
> >+	case MMC_BUS_WIDTH_4:
> >+		ctrl |= SDHCI_CTRL_4BITBUS;
> >+		ctrl &= ~SDHCI_CTRL_8BITBUS;
> >+		break;
> >+	default:
> >+		ctrl &= ~(SDHCI_CTRL_8BITBUS | SDHCI_CTRL_4BITBUS);
> >+		break;
> You can remove the break here.

Ok, removed in v2

> >+	switch (reg) {
> >+	case SDHCI_CAPABILITIES:
> >+		ret = readl(host->ioaddr + reg);
> >+		/* Support 3.3V and 1.8V */
> >+		ret &= ~SDHCI_CAN_VDD_300;
> >+		break;
> >+	default:
> >+		ret = readl(host->ioaddr + reg);
> 
> Could we use readl_relaxed?

Yes, I've updated to use readl_relaxed in v2

> >+	dev_dbg(&pdev->dev, "SDHCI ST platform driver\n");
> You can remove this I think.

Removed in v2

> >+		host_version, ((host_version & SDHCI_VENDOR_VER_MASK) >>
> >+			       SDHCI_VENDOR_VER_SHIFT));
> Maybe you could change to dev_info here. It might be interresting to
> always have IP version.

Changed to dev_info in v2

Regards,

Peter.

Re: [PATCH 1/8] mmc: sdhci-st: Intial support for ST SDHCI controller

[Ulf Hansson]

On 4 June 2014 10:48, Peter Griffin <peter.griffin@linaro.org> wrote:
> Hi Ulf,
>
> Thanks for reviewing, see my comments below: -
>
> On Fri, 23 May 2014, Ulf Hansson wrote:
>> > +static unsigned int sdhci_st_get_max_clk(struct sdhci_host *host)
>> > +{
>> > +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> > +
>> > +       return clk_get_rate(pltfm_host->clk);
>> > +}
>>
>> There are sdhci library function for the above:
>> sdhci_pltfm_clk_get_max_clock()
>
> I've fixed in v2 to use the library function
>
>> > +       host->mmc->caps |= MMC_CAP_8_BIT_DATA | MMC_CAP_BUS_WIDTH_TEST
>> > +                       | MMC_CAP_1_8V_DDR;
>>
>> Comment below.
>>
>> > +
>> > +       if (of_property_read_bool(np, "non-removable"))
>> > +               host->mmc->caps |= MMC_CAP_NONREMOVABLE;
>>
>> MMC_CAP_1_8V_DDR, MMC_CAP_8_BIT_DATA, MMC_CAP_NONREMOVABLE aren’t
>> those board specific capabilities?
>
> Yep
>>
>> Unless there are something that prevents you from using the common mmc
>> DT parser, I would suggest you to use it. mmc_of_parse(). Thus you can
>> provide these capabilities through DT instead.
>
> Thanks for pointing that out, I've switched over to using mmc_of_parse,
> and everything works as expected.
>
> Also as an added bonus this will simplify support for the next SoC which
> needs access to the high speed ddr / sdr bindings which
> mmc_of_parse already supports :-)
>
> In v2 I've also removed the reset controller code from this platform driver
> with the intention of adding it back in, in the generic code. The idea
> would be that an additional 'reset' binding could be provided, which if
> present would be used to deassert the IP reset line of the controller at
> probe / resume, and assert reset at remove / sleep.
>
> Is this something you view as useful, if so I will prepare some RFC patches?

Sounds useful. Please go ahead and send patches! :-)

Kind regards
Uffe