[PATCH v3 0/9] mmc: mmci: add support for STM32 SD controller

[PATCH v3 0/9] mmc: mmci: add support for STM32 SD controller

[Andrea Merello]

The SD controller found in STM32 MCUs happens to be yet another variant
of the ARM PrimeCell PL18x SD host controller, for which the mmci driver
exists.

This series adds support for it to the mmci driver.

As other variants, this one need some specific quirks, that this series
address. Most notably this variant has not AMBA PrimeCell id registers,
so we can't probe it using the AMBA PrimeCell generic "compatible" and
mechanism; rather this series adds support to the mmci driver for
register itself also as platform driver, so that specific "compatible"
strings can also be used.

I tested this on my STM32F469-disco board, that is able to boot from an
SD card, and write/read to/from it.

RFT for other variants (to check I didn't broke anything) and with MMC
cards. In particular I tried to implement also support for open-collector
communication mode, that AFAICT is used only on MMC cards, but I couldn't
test it.

Changes since v1:
- fixed compile failure when CONFIG_ARM_AMBA is enabled
- fixed mmc CD pin claimed two times
- trivial whitespace and newline fixes

Changes since v2:
- dropped pinmux cfg for CD pin: it is handled by MMC layer using gpiolib

Andrea Merello (9):
  mmc: mmci: don't pretend IP variants with only one IRQ to have two
    mask regs
  mmc: mmci: add support for not-amba, but still compatible, variants
  mmc: mmci: don't pretend all variants to have MCI_STARBITERR flag
  mmc: mmci: add support for setting pad type via pinctrl
  mmc: mmci: add STM32 variant
  ARM: DTS: stm32: add pin map for SDIO controller on stm32f429
  ARM: DTS: stm32: add node for SDIO controller on stm32f429
  ARM: DTS: enable SDIO controller on stm32f469 disco board
  Documentation: document mmci STM32 DT binding

 Documentation/devicetree/bindings/mmc/mmci.txt |   2 +-
 arch/arm/boot/dts/stm32f429.dtsi               |  47 ++-
 arch/arm/boot/dts/stm32f469-disco.dts          |  18 ++
 drivers/mmc/host/Kconfig                       |   8 +-
 drivers/mmc/host/mmci.c                        | 398 +++++++++++++++++++------
 drivers/mmc/host/mmci.h                        |   7 +-
 6 files changed, 385 insertions(+), 95 deletions(-)

--
2.7.4

[PATCH v3 1/9] mmc: mmci: don't pretend IP variants with only one IRQ to have two mask regs

[Andrea Merello]

Currently the driver supports both devices with one and two IRQ lines.

Two mask registers are used in order to select which events have to
actually generate an interrupt on each IRQ line.

It seems that in the single-IRQ case it's assumed that the IRQs lines
are simply OR-ed, while the two mask registers are still present. The
driver still programs the two mask registers separately.

However the STM32 variant has only one IRQ, and also has only one mask
register.

This patch prepares for STM32 variant support by making the driver using
only one mask register when only one IRQ is available.

Tested only on STM32 variant. RFT for variants other than STM32

Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
 drivers/mmc/host/mmci.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 01a8047..597fab8 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -404,9 +404,9 @@ static void mmci_set_mask1(struct mmci_host *host, unsigned int mask)
 		mask0 |= mask;
 
 		writel(mask0, base + MMCIMASK0);
+	} else {
+		writel(mask, base + MMCIMASK1);
 	}
-
-	writel(mask, base + MMCIMASK1);
 }
 
 static void mmci_stop_data(struct mmci_host *host)
@@ -1274,12 +1274,10 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
 
 	do {
 		status = readl(host->base + MMCISTATUS);
+		status &= readl(host->base + MMCIMASK0);
 
 		if (host->singleirq) {
-			if (status & readl(host->base + MMCIMASK1))
-				mmci_pio_irq(irq, dev_id);
-
-			status &= ~MCI_IRQ1MASK;
+			mmci_pio_irq(irq, dev_id);
 		}
 
 		/*
@@ -1287,7 +1285,6 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
 		 * enabled) since the HW seems to be triggering the IRQ on both
 		 * edges while monitoring DAT0 for busy completion.
 		 */
-		status &= readl(host->base + MMCIMASK0);
 		writel(status, host->base + MMCICLEAR);
 
 		dev_dbg(mmc_dev(host->mmc), "irq0 (data+cmd) %08x\n", status);
@@ -1710,7 +1707,8 @@ static int mmci_probe(struct amba_device *dev,
 	spin_lock_init(&host->lock);
 
 	writel(0, host->base + MMCIMASK0);
-	writel(0, host->base + MMCIMASK1);
+	if (!host->singleirq)
+		writel(0, host->base + MMCIMASK1);
 	writel(0xfff, host->base + MMCICLEAR);
 
 	/*
@@ -1800,7 +1798,8 @@ static int mmci_remove(struct amba_device *dev)
 		mmc_remove_host(mmc);
 
 		writel(0, host->base + MMCIMASK0);
-		writel(0, host->base + MMCIMASK1);
+		if (!host->singleirq)
+			writel(0, host->base + MMCIMASK1);
 
 		writel(0, host->base + MMCICOMMAND);
 		writel(0, host->base + MMCIDATACTRL);
-- 
2.7.4

[PATCH v3 2/9] mmc: mmci: add support for not-amba, but still compatible, variants

[Andrea Merello]

The STM32F4xx family contain a SDIO IP that looks like a variant of
the PL180, however, inspite it's actually attached to a APB bus, it
cannot be handled by the AMBA bus code, because it lacks of the ID
registers that AMBA primecell IPs have.

This patch prepares for supporting the STM32 variant by letting the
driver register also as a platform driver, that can be matched from
the OF with specific "compatible" strings

Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
 drivers/mmc/host/Kconfig |   8 +-
 drivers/mmc/host/mmci.c  | 297 ++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 236 insertions(+), 69 deletions(-)

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 2eb9701..f8f7f289 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -5,12 +5,12 @@
 comment "MMC/SD/SDIO Host Controller Drivers"
 
 config MMC_ARMMMCI
-	tristate "ARM AMBA Multimedia Card Interface support"
-	depends on ARM_AMBA
+	tristate "ARM AMBA Multimedia Card Interface and compatible support"
 	help
 	  This selects the ARM(R) AMBA(R) PrimeCell Multimedia Card
-	  Interface (PL180 and PL181) support.  If you have an ARM(R)
-	  platform with a Multimedia Card slot, say Y or M here.
+	  Interface (PL180, PL181 and compatible) support.
+	  If you have an ARM(R) platform with a Multimedia Card slot,
+	  say Y or M here.
 
 	  If unsure, say N.
 
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 597fab8..54c2fc3 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1,5 +1,6 @@
 /*
- *  linux/drivers/mmc/host/mmci.c - ARM PrimeCell MMCI PL180/1 driver
+ *  linux/drivers/mmc/host/mmci.c - ARM PrimeCell MMCI PL180/1 and
+ *  comaptible driver
  *
  *  Copyright (C) 2003 Deep Blue Solutions, Ltd, All Rights Reserved.
  *  Copyright (C) 2010 ST-Ericsson SA
@@ -37,6 +38,8 @@
 #include <linux/pm_runtime.h>
 #include <linux/types.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/of_device.h>
 
 #include <asm/div64.h>
 #include <asm/io.h>
@@ -113,7 +116,7 @@ struct variant_data {
 	bool			reversed_irq_handling;
 };
 
-static struct variant_data variant_arm = {
+static __maybe_unused struct variant_data variant_arm = {
 	.fifosize		= 16 * 4,
 	.fifohalfsize		= 8 * 4,
 	.datalength_bits	= 16,
@@ -122,7 +125,7 @@ static struct variant_data variant_arm = {
 	.reversed_irq_handling	= true,
 };
 
-static struct variant_data variant_arm_extended_fifo = {
+static __maybe_unused struct variant_data variant_arm_extended_fifo = {
 	.fifosize		= 128 * 4,
 	.fifohalfsize		= 64 * 4,
 	.datalength_bits	= 16,
@@ -130,7 +133,7 @@ static struct variant_data variant_arm_extended_fifo = {
 	.f_max			= 100000000,
 };
 
-static struct variant_data variant_arm_extended_fifo_hwfc = {
+static __maybe_unused struct variant_data variant_arm_extended_fifo_hwfc = {
 	.fifosize		= 128 * 4,
 	.fifohalfsize		= 64 * 4,
 	.clkreg_enable		= MCI_ARM_HWFCEN,
@@ -139,7 +142,7 @@ static struct variant_data variant_arm_extended_fifo_hwfc = {
 	.f_max			= 100000000,
 };
 
-static struct variant_data variant_u300 = {
+static __maybe_unused struct variant_data variant_u300 = {
 	.fifosize		= 16 * 4,
 	.fifohalfsize		= 8 * 4,
 	.clkreg_enable		= MCI_ST_U300_HWFCEN,
@@ -154,7 +157,7 @@ static struct variant_data variant_u300 = {
 	.pwrreg_nopower		= true,
 };
 
-static struct variant_data variant_nomadik = {
+static __maybe_unused struct variant_data variant_nomadik = {
 	.fifosize		= 16 * 4,
 	.fifohalfsize		= 8 * 4,
 	.clkreg			= MCI_CLK_ENABLE,
@@ -170,7 +173,7 @@ static struct variant_data variant_nomadik = {
 	.pwrreg_nopower		= true,
 };
 
-static struct variant_data variant_ux500 = {
+static __maybe_unused struct variant_data variant_ux500 = {
 	.fifosize		= 30 * 4,
 	.fifohalfsize		= 8 * 4,
 	.clkreg			= MCI_CLK_ENABLE,
@@ -192,7 +195,7 @@ static struct variant_data variant_ux500 = {
 	.pwrreg_nopower		= true,
 };
 
-static struct variant_data variant_ux500v2 = {
+static __maybe_unused struct variant_data variant_ux500v2 = {
 	.fifosize		= 30 * 4,
 	.fifohalfsize		= 8 * 4,
 	.clkreg			= MCI_CLK_ENABLE,
@@ -216,7 +219,7 @@ static struct variant_data variant_ux500v2 = {
 	.pwrreg_nopower		= true,
 };
 
-static struct variant_data variant_qcom = {
+static __maybe_unused struct variant_data variant_qcom = {
 	.fifosize		= 16 * 4,
 	.fifohalfsize		= 8 * 4,
 	.clkreg			= MCI_CLK_ENABLE,
@@ -1528,31 +1531,32 @@ static int mmci_of_parse(struct device_node *np, struct mmc_host *mmc)
 	return 0;
 }
 
-static int mmci_probe(struct amba_device *dev,
-	const struct amba_id *id)
+static struct mmci_host *mmci_probe(struct device *dev,
+				    struct variant_data *variant,
+				    struct resource *res,
+				    unsigned int irq0, unsigned int irq1)
 {
-	struct mmci_platform_data *plat = dev->dev.platform_data;
-	struct device_node *np = dev->dev.of_node;
-	struct variant_data *variant = id->data;
-	struct mmci_host *host;
+	struct device_node *np = dev->of_node;
+	struct mmci_platform_data *plat = dev->platform_data;
 	struct mmc_host *mmc;
+	struct mmci_host *host;
 	int ret;
 
 	/* Must have platform data or Device Tree. */
 	if (!plat && !np) {
-		dev_err(&dev->dev, "No plat data or DT found\n");
-		return -EINVAL;
+		dev_err(dev, "No plat data or DT found\n");
+		return ERR_PTR(-EINVAL);
 	}
 
 	if (!plat) {
-		plat = devm_kzalloc(&dev->dev, sizeof(*plat), GFP_KERNEL);
+		plat = devm_kzalloc(dev, sizeof(*plat), GFP_KERNEL);
 		if (!plat)
-			return -ENOMEM;
+			return ERR_PTR(-ENOMEM);
 	}
 
-	mmc = mmc_alloc_host(sizeof(struct mmci_host), &dev->dev);
+	mmc = mmc_alloc_host(sizeof(struct mmci_host), dev);
 	if (!mmc)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	ret = mmci_of_parse(np, mmc);
 	if (ret)
@@ -1561,12 +1565,7 @@ static int mmci_probe(struct amba_device *dev,
 	host = mmc_priv(mmc);
 	host->mmc = mmc;
 
-	host->hw_designer = amba_manf(dev);
-	host->hw_revision = amba_rev(dev);
-	dev_dbg(mmc_dev(mmc), "designer ID = 0x%02x\n", host->hw_designer);
-	dev_dbg(mmc_dev(mmc), "revision = 0x%01x\n", host->hw_revision);
-
-	host->clk = devm_clk_get(&dev->dev, NULL);
+	host->clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(host->clk)) {
 		ret = PTR_ERR(host->clk);
 		goto host_free;
@@ -1598,8 +1597,8 @@ static int mmci_probe(struct amba_device *dev,
 			host->mclk);
 	}
 
-	host->phybase = dev->res.start;
-	host->base = devm_ioremap_resource(&dev->dev, &dev->res);
+	host->phybase = res->start;
+	host->base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(host->base)) {
 		ret = PTR_ERR(host->base);
 		goto clk_disable;
@@ -1742,49 +1741,41 @@ static int mmci_probe(struct amba_device *dev,
 		}
 	}
 
-	ret = devm_request_irq(&dev->dev, dev->irq[0], mmci_irq, IRQF_SHARED,
-			DRIVER_NAME " (cmd)", host);
+	ret = devm_request_irq(dev, irq0, mmci_irq, IRQF_SHARED,
+			       DRIVER_NAME " (cmd)", host);
 	if (ret)
 		goto clk_disable;
 
-	if (!dev->irq[1])
+	if (!irq1) {
 		host->singleirq = true;
-	else {
-		ret = devm_request_irq(&dev->dev, dev->irq[1], mmci_pio_irq,
-				IRQF_SHARED, DRIVER_NAME " (pio)", host);
+	} else {
+		ret = devm_request_irq(dev, irq1, mmci_pio_irq,
+				       IRQF_SHARED, DRIVER_NAME " (pio)", host);
 		if (ret)
 			goto clk_disable;
 	}
 
 	writel(MCI_IRQENABLE, host->base + MMCIMASK0);
 
-	amba_set_drvdata(dev, mmc);
-
-	dev_info(&dev->dev, "%s: PL%03x manf %x rev%u at 0x%08llx irq %d,%d (pio)\n",
-		 mmc_hostname(mmc), amba_part(dev), amba_manf(dev),
-		 amba_rev(dev), (unsigned long long)dev->res.start,
-		 dev->irq[0], dev->irq[1]);
-
 	mmci_dma_setup(host);
 
-	pm_runtime_set_autosuspend_delay(&dev->dev, 50);
-	pm_runtime_use_autosuspend(&dev->dev);
+	pm_runtime_set_autosuspend_delay(dev, 50);
+	pm_runtime_use_autosuspend(dev);
 
 	mmc_add_host(mmc);
 
-	pm_runtime_put(&dev->dev);
-	return 0;
+	pm_runtime_put(dev);
+	return host;
 
  clk_disable:
 	clk_disable_unprepare(host->clk);
  host_free:
 	mmc_free_host(mmc);
-	return ret;
+	return ERR_PTR(ret);
 }
 
-static int mmci_remove(struct amba_device *dev)
+static int mmc_remove(struct mmc_host *mmc)
 {
-	struct mmc_host *mmc = amba_get_drvdata(dev);
 
 	if (mmc) {
 		struct mmci_host *host = mmc_priv(mmc);
@@ -1793,7 +1784,7 @@ static int mmci_remove(struct amba_device *dev)
 		 * Undo pm_runtime_put() in probe.  We use the _sync
 		 * version here so that we can access the primecell.
 		 */
-		pm_runtime_get_sync(&dev->dev);
+		pm_runtime_get_sync(mmc->parent);
 
 		mmc_remove_host(mmc);
 
@@ -1847,14 +1838,11 @@ static void mmci_restore(struct mmci_host *host)
 	spin_unlock_irqrestore(&host->lock, flags);
 }
 
-static int mmci_runtime_suspend(struct device *dev)
+static int mmci_runtime_suspend(struct mmc_host *mmc)
 {
-	struct amba_device *adev = to_amba_device(dev);
-	struct mmc_host *mmc = amba_get_drvdata(adev);
-
 	if (mmc) {
 		struct mmci_host *host = mmc_priv(mmc);
-		pinctrl_pm_select_sleep_state(dev);
+		pinctrl_pm_select_sleep_state(mmc->parent);
 		mmci_save(host);
 		clk_disable_unprepare(host->clk);
 	}
@@ -1862,28 +1850,77 @@ static int mmci_runtime_suspend(struct device *dev)
 	return 0;
 }
 
-static int mmci_runtime_resume(struct device *dev)
+static int mmci_runtime_resume(struct mmc_host *mmc)
 {
-	struct amba_device *adev = to_amba_device(dev);
-	struct mmc_host *mmc = amba_get_drvdata(adev);
 
 	if (mmc) {
 		struct mmci_host *host = mmc_priv(mmc);
 		clk_prepare_enable(host->clk);
 		mmci_restore(host);
-		pinctrl_pm_select_default_state(dev);
+		pinctrl_pm_select_default_state(mmc->parent);
 	}
 
 	return 0;
 }
 #endif
 
-static const struct dev_pm_ops mmci_dev_pm_ops = {
+#ifdef CONFIG_ARM_AMBA
+static int mmci_remove_amba(struct amba_device *dev)
+{
+	struct mmc_host *mmc = amba_get_drvdata(dev);
+
+	return mmc_remove(mmc);
+}
+
+static int mmci_runtime_resume_amba(struct device *dev)
+{
+	struct amba_device *adev = to_amba_device(dev);
+	struct mmc_host *mmc = amba_get_drvdata(adev);
+
+	return mmci_runtime_resume(mmc);
+}
+
+static int mmci_runtime_suspend_amba(struct device *dev)
+{
+	struct amba_device *adev = to_amba_device(dev);
+	struct mmc_host *mmc = amba_get_drvdata(adev);
+
+	return mmci_runtime_suspend(mmc);
+}
+
+static const struct dev_pm_ops mmci_dev_pm_ops_amba = {
 	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
 				pm_runtime_force_resume)
-	SET_RUNTIME_PM_OPS(mmci_runtime_suspend, mmci_runtime_resume, NULL)
+	SET_RUNTIME_PM_OPS(mmci_runtime_suspend_amba, mmci_runtime_resume_amba,
+			   NULL)
 };
 
+static int mmci_probe_amba(struct amba_device *dev, const struct amba_id *id)
+{
+	struct mmci_host *host;
+	struct variant_data *variant = id->data;
+
+	host = mmci_probe(&dev->dev, variant, &dev->res,
+			  dev->irq[0], dev->irq[1]);
+	if (IS_ERR(host))
+		return PTR_ERR(host);
+
+	host->hw_designer = amba_manf(dev);
+	host->hw_revision = amba_rev(dev);
+	dev_dbg(mmc_dev(host->mmc), "designer ID = 0x%02x\n",
+		host->hw_designer);
+	dev_dbg(mmc_dev(host->mmc), "revision = 0x%01x\n", host->hw_revision);
+
+	amba_set_drvdata(dev, host->mmc);
+
+	dev_info(&dev->dev, "%s: PL%03x manf %x rev%u at 0x%08llx irq %d,%d (pio)\n",
+		 mmc_hostname(host->mmc), amba_part(dev), amba_manf(dev),
+		 amba_rev(dev), (unsigned long long)dev->res.start,
+		 dev->irq[0], dev->irq[1]);
+
+	return 0;
+}
+
 static struct amba_id mmci_ids[] = {
 	{
 		.id	= 0x00041180,
@@ -1945,14 +1982,144 @@ MODULE_DEVICE_TABLE(amba, mmci_ids);
 static struct amba_driver mmci_driver = {
 	.drv		= {
 		.name	= DRIVER_NAME,
-		.pm	= &mmci_dev_pm_ops,
+		.pm	= &mmci_dev_pm_ops_amba,
 	},
-	.probe		= mmci_probe,
-	.remove		= mmci_remove,
+	.probe		= mmci_probe_amba,
+	.remove		= mmci_remove_amba,
 	.id_table	= mmci_ids,
 };
+#endif
+
+static const struct of_device_id mmci_pltfm_match[] = {
+	{},
+};
+
+static int mmci_probe_pltfm(struct platform_device *pdev)
+{
+	struct mmci_host *host;
+	struct variant_data *variant;
+	const struct of_device_id *match;
+	int irq0, irq1;
+	struct resource *res;
+
+	irq0 = platform_get_irq(pdev, 0);
+	if (irq0 < 0) {
+		dev_err(&pdev->dev, "Can't get IRQ");
+		return -EINVAL;
+	}
+
+	irq1 = platform_get_irq(pdev, 1);
+	/* optional. set to 0 to be compatible with original amba probe code */
+	if (irq1 < 0)
+		irq1 = 0;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "Can't get MMIO range");
+		return -EINVAL;
+	}
+
+	match = of_match_device(mmci_pltfm_match, &pdev->dev);
+	if (!match) {
+		dev_err(&pdev->dev, "Can't get variant data\n");
+		return -EINVAL;
+	}
+	variant = (struct variant_data *)match->data;
+
+	host = mmci_probe(&pdev->dev, variant, res,
+			  irq0, irq1);
+	if (IS_ERR(host))
+		return PTR_ERR(host);
+
+	platform_set_drvdata(pdev, host->mmc);
+	dev_info(&pdev->dev, "%s: PL81x compatible SDIO at 0x%08llx irq %d,%d (pio)\n",
+		 mmc_hostname(host->mmc),
+		 (unsigned long long)res->start, irq0, irq1);
+
+	return 0;
+}
+
+static int mmci_remove_pltfm(struct platform_device *pdev)
+{
+	struct mmc_host *mmc = platform_get_drvdata(pdev);
+
+	return mmc_remove(mmc);
+}
+
+static int mmci_runtime_resume_pltfm(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct mmc_host *mmc = platform_get_drvdata(pdev);
+
+	return mmci_runtime_resume(mmc);
+}
+
+static int mmci_runtime_suspend_pltfm(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct mmc_host *mmc = platform_get_drvdata(pdev);
+
+	return mmci_runtime_suspend(mmc);
+}
+
+MODULE_DEVICE_TABLE(of, mmci_pltfm_match);
+
+static const struct dev_pm_ops mmci_dev_pm_ops_pltfm = {
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+	SET_RUNTIME_PM_OPS(mmci_runtime_suspend_pltfm,
+			   mmci_runtime_resume_pltfm, NULL)
+};
+
+static struct platform_driver mmci_pltfm_driver = {
+	.probe		= mmci_probe_pltfm,
+	.remove		= mmci_remove_pltfm,
+	.driver		= {
+		.name		= DRIVER_NAME,
+		.of_match_table	= mmci_pltfm_match,
+		.pm		= &mmci_dev_pm_ops_pltfm,
+	},
+};
+
+static int __init mmci_init(void)
+{
+	int ret;
+	__maybe_unused int ret_amba;
+
+	ret = platform_driver_register(&mmci_pltfm_driver);
+
+#ifdef CONFIG_ARM_AMBA
+	ret_amba = amba_driver_register(&mmci_driver);
+
+	/* if both plaform and amba failed. Just fail. */
+	if (ret && ret_amba)
+		return ret;
+
+	/*
+	 * If only one of the two succeeded, don't fail here, because the
+	 * other one is probably usable, but spit a warning..
+	 */
+	if (ret_amba)
+		pr_warn("could not register MMCI amba driver");
+
+	if (ret)
+		pr_warn("could not register MMCI platform driver");
+
+	return 0;
+#endif
+	return ret;
+}
+
+static void __exit mmci_exit(void)
+{
+	platform_driver_unregister(&mmci_pltfm_driver);
+#ifdef CONFIG_ARM_AMBA
+	amba_driver_unregister(&mmci_driver);
+#endif
+}
 
-module_amba_driver(mmci_driver);
+module_init(mmci_init);
+module_exit(mmci_exit);
 
 module_param(fmax, uint, 0444);
 
-- 
2.7.4

[PATCH v3 3/9] mmc: mmci: don't pretend all variants to have MCI_STARBITERR flag

[Andrea Merello]

This patch prepares for supporting the STM32 variant, that has no such bit
in the status register.

Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
 drivers/mmc/host/mmci.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 54c2fc3..fbb5728 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -114,6 +114,7 @@ struct variant_data {
 	bool			qcom_fifo;
 	bool			qcom_dml;
 	bool			reversed_irq_handling;
+	bool			has_start_err;
 };
 
 static __maybe_unused struct variant_data variant_arm = {
@@ -123,6 +124,7 @@ static __maybe_unused struct variant_data variant_arm = {
 	.pwrreg_powerup		= MCI_PWR_UP,
 	.f_max			= 100000000,
 	.reversed_irq_handling	= true,
+	.has_start_err		= true,
 };
 
 static __maybe_unused struct variant_data variant_arm_extended_fifo = {
@@ -131,6 +133,7 @@ static __maybe_unused struct variant_data variant_arm_extended_fifo = {
 	.datalength_bits	= 16,
 	.pwrreg_powerup		= MCI_PWR_UP,
 	.f_max			= 100000000,
+	.has_start_err		= true,
 };
 
 static __maybe_unused struct variant_data variant_arm_extended_fifo_hwfc = {
@@ -140,6 +143,7 @@ static __maybe_unused struct variant_data variant_arm_extended_fifo_hwfc = {
 	.datalength_bits	= 16,
 	.pwrreg_powerup		= MCI_PWR_UP,
 	.f_max			= 100000000,
+	.has_start_err		= true,
 };
 
 static __maybe_unused struct variant_data variant_u300 = {
@@ -155,6 +159,7 @@ static __maybe_unused struct variant_data variant_u300 = {
 	.signal_direction	= true,
 	.pwrreg_clkgate		= true,
 	.pwrreg_nopower		= true,
+	.has_start_err		= true,
 };
 
 static __maybe_unused struct variant_data variant_nomadik = {
@@ -171,6 +176,7 @@ static __maybe_unused struct variant_data variant_nomadik = {
 	.signal_direction	= true,
 	.pwrreg_clkgate		= true,
 	.pwrreg_nopower		= true,
+	.has_start_err		= true,
 };
 
 static __maybe_unused struct variant_data variant_ux500 = {
@@ -193,6 +199,7 @@ static __maybe_unused struct variant_data variant_ux500 = {
 	.busy_detect_flag	= MCI_ST_CARDBUSY,
 	.busy_detect_mask	= MCI_ST_BUSYENDMASK,
 	.pwrreg_nopower		= true,
+	.has_start_err		= true,
 };
 
 static __maybe_unused struct variant_data variant_ux500v2 = {
@@ -217,6 +224,7 @@ static __maybe_unused struct variant_data variant_ux500v2 = {
 	.busy_detect_flag	= MCI_ST_CARDBUSY,
 	.busy_detect_mask	= MCI_ST_BUSYENDMASK,
 	.pwrreg_nopower		= true,
+	.has_start_err		= true,
 };
 
 static __maybe_unused struct variant_data variant_qcom = {
@@ -235,6 +243,7 @@ static __maybe_unused struct variant_data variant_qcom = {
 	.explicit_mclk_control	= true,
 	.qcom_fifo		= true,
 	.qcom_dml		= true,
+	.has_start_err		= true,
 };
 
 /* Busy detection for the ST Micro variant */
@@ -923,8 +932,9 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
 		return;
 
 	/* First check for errors */
-	if (status & (MCI_DATACRCFAIL|MCI_DATATIMEOUT|MCI_STARTBITERR|
-		      MCI_TXUNDERRUN|MCI_RXOVERRUN)) {
+	if (status & (MCI_DATACRCFAIL | MCI_DATATIMEOUT |
+			(host->variant->has_start_err ? MCI_STARTBITERR : 0) |
+			MCI_TXUNDERRUN | MCI_RXOVERRUN)) {
 		u32 remain, success;
 
 		/* Terminate the DMA transfer */
-- 
2.7.4

[PATCH v3 4/9] mmc: mmci: add support for setting pad type via pinctrl

[Andrea Merello]

The STM32 variant  hasn't the control bit to switch pads in opendrain mode.
In this case we can achive the same result by askint to the pinmux driver
to configure pins for us.

This patch make the mmci driver able to do this whenever needed.

Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
 drivers/mmc/host/mmci.c | 52 +++++++++++++++++++++++++++++++++++++++----------
 drivers/mmc/host/mmci.h |  7 +++++--
 2 files changed, 47 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index fbb5728..c581b81 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -115,6 +115,7 @@ struct variant_data {
 	bool			qcom_dml;
 	bool			reversed_irq_handling;
 	bool			has_start_err;
+	bool			has_pad_config;
 };
 
 static __maybe_unused struct variant_data variant_arm = {
@@ -125,6 +126,7 @@ static __maybe_unused struct variant_data variant_arm = {
 	.f_max			= 100000000,
 	.reversed_irq_handling	= true,
 	.has_start_err		= true,
+	.has_pad_config		= true,
 };
 
 static __maybe_unused struct variant_data variant_arm_extended_fifo = {
@@ -134,6 +136,7 @@ static __maybe_unused struct variant_data variant_arm_extended_fifo = {
 	.pwrreg_powerup		= MCI_PWR_UP,
 	.f_max			= 100000000,
 	.has_start_err		= true,
+	.has_pad_config		= true,
 };
 
 static __maybe_unused struct variant_data variant_arm_extended_fifo_hwfc = {
@@ -144,6 +147,7 @@ static __maybe_unused struct variant_data variant_arm_extended_fifo_hwfc = {
 	.pwrreg_powerup		= MCI_PWR_UP,
 	.f_max			= 100000000,
 	.has_start_err		= true,
+	.has_pad_config		= true,
 };
 
 static __maybe_unused struct variant_data variant_u300 = {
@@ -160,6 +164,7 @@ static __maybe_unused struct variant_data variant_u300 = {
 	.pwrreg_clkgate		= true,
 	.pwrreg_nopower		= true,
 	.has_start_err		= true,
+	.has_pad_config		= true,
 };
 
 static __maybe_unused struct variant_data variant_nomadik = {
@@ -177,6 +182,7 @@ static __maybe_unused struct variant_data variant_nomadik = {
 	.pwrreg_clkgate		= true,
 	.pwrreg_nopower		= true,
 	.has_start_err		= true,
+	.has_pad_config		= true,
 };
 
 static __maybe_unused struct variant_data variant_ux500 = {
@@ -200,6 +206,7 @@ static __maybe_unused struct variant_data variant_ux500 = {
 	.busy_detect_mask	= MCI_ST_BUSYENDMASK,
 	.pwrreg_nopower		= true,
 	.has_start_err		= true,
+	.has_pad_config		= true,
 };
 
 static __maybe_unused struct variant_data variant_ux500v2 = {
@@ -225,6 +232,7 @@ static __maybe_unused struct variant_data variant_ux500v2 = {
 	.busy_detect_mask	= MCI_ST_BUSYENDMASK,
 	.pwrreg_nopower		= true,
 	.has_start_err		= true,
+	.has_pad_config		= true,
 };
 
 static __maybe_unused struct variant_data variant_qcom = {
@@ -244,6 +252,7 @@ static __maybe_unused struct variant_data variant_qcom = {
 	.qcom_fifo		= true,
 	.qcom_dml		= true,
 	.has_start_err		= true,
+	.has_pad_config		= true,
 };
 
 /* Busy detection for the ST Micro variant */
@@ -1362,6 +1371,8 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	u32 pwr = 0;
 	unsigned long flags;
 	int ret;
+	struct pinctrl_state *pins;
+	bool is_opendrain;
 
 	if (host->plat->ios_handler &&
 		host->plat->ios_handler(mmc_dev(mmc), ios))
@@ -1420,16 +1431,31 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 				~MCI_ST_DATA2DIREN);
 	}
 
-	if (ios->bus_mode == MMC_BUSMODE_OPENDRAIN) {
-		if (host->hw_designer != AMBA_VENDOR_ST)
-			pwr |= MCI_ROD;
-		else {
-			/*
-			 * The ST Micro variant use the ROD bit for something
-			 * else and only has OD (Open Drain).
-			 */
-			pwr |= MCI_OD;
+	if (host->variant->has_pad_config) {
+		if (ios->bus_mode == MMC_BUSMODE_OPENDRAIN) {
+			if (host->hw_designer != AMBA_VENDOR_ST) {
+				pwr |= MCI_ROD;
+			} else {
+				/*
+				 * The ST Micro variant use the ROD bit for
+				 * something else and only has OD (Open Drain).
+				 */
+				pwr |= MCI_OD;
+			}
 		}
+	} else {
+		/*
+		 * If the variant cannot configure the pads by its own, then we
+		 * expect the pinctrl to be able to do that for us
+		 */
+		is_opendrain = (ios->bus_mode == MMC_BUSMODE_OPENDRAIN);
+		pins = pinctrl_lookup_state(host->pinctrl, is_opendrain ?
+					MMCI_PINCTRL_STATE_OPENDRAIN :
+					MMCI_PINCTRL_STATE_PUSHPULL);
+		if (IS_ERR(pins))
+			dev_warn(mmc_dev(mmc), "Cannot select pin drive type via pinctrl\n");
+		else
+			pinctrl_select_state(host->pinctrl, pins);
 	}
 
 	/*
@@ -1537,7 +1563,6 @@ static int mmci_of_parse(struct device_node *np, struct mmc_host *mmc)
 		mmc->caps |= MMC_CAP_MMC_HIGHSPEED;
 	if (of_get_property(np, "mmc-cap-sd-highspeed", NULL))
 		mmc->caps |= MMC_CAP_SD_HIGHSPEED;
-
 	return 0;
 }
 
@@ -1575,6 +1600,13 @@ static struct mmci_host *mmci_probe(struct device *dev,
 	host = mmc_priv(mmc);
 	host->mmc = mmc;
 
+	if (!variant->has_pad_config) {
+		host->pinctrl = devm_pinctrl_get(dev);
+		if (IS_ERR(host->pinctrl)) {
+			dev_err(dev, "failed to get pinctrl");
+			goto host_free;
+		}
+	}
 	host->clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(host->clk)) {
 		ret = PTR_ERR(host->clk);
diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
index 56322c6..b5be66a 100644
--- a/drivers/mmc/host/mmci.h
+++ b/drivers/mmc/host/mmci.h
@@ -192,6 +192,10 @@
 
 #define NR_SG		128
 
+/* pinctrl configs */
+#define MMCI_PINCTRL_STATE_PUSHPULL "default"
+#define MMCI_PINCTRL_STATE_OPENDRAIN "opendrain"
+
 struct clk;
 struct variant_data;
 struct dma_chan;
@@ -226,7 +230,7 @@ struct mmci_host {
 	bool			vqmmc_enabled;
 	struct mmci_platform_data *plat;
 	struct variant_data	*variant;
-
+	struct pinctrl		*pinctrl;
 	u8			hw_designer;
 	u8			hw_revision:4;
 
@@ -251,4 +255,3 @@ struct mmci_host {
 #define dma_inprogress(host)	(0)
 #endif
 };
-
-- 
2.7.4

[PATCH v3 5/9] mmc: mmci: add STM32 variant

[Andrea Merello]

STM32 mcu has a SDIO controller that looks like an ARM pl810.
This patch adds the STM32 variant so that mmci driver supports it.

Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
 drivers/mmc/host/mmci.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index c581b81..88a350b 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -235,6 +235,23 @@ static __maybe_unused struct variant_data variant_ux500v2 = {
 	.has_pad_config		= true,
 };
 
+static __maybe_unused struct variant_data variant_stm32 = {
+	.fifosize		= 32 * 4,
+	.fifohalfsize		= 8 * 4,
+	.clkreg			= MCI_CLK_ENABLE,
+	.clkreg_enable		= MCI_ST_UX500_HWFCEN,
+	.clkreg_8bit_bus_enable = MCI_ST_8BIT_BUS,
+	.clkreg_neg_edge_enable	= MCI_ST_UX500_NEG_EDGE,
+	.datalength_bits	= 24,
+	.datactrl_mask_sdio	= MCI_DPSM_ST_SDIOEN,
+	.st_sdio		= true,
+	.st_clkdiv		= true,
+	.pwrreg_powerup		= MCI_PWR_ON,
+	.f_max			= 48000000,
+	.pwrreg_clkgate		= true,
+	.pwrreg_nopower		= true,
+};
+
 static __maybe_unused struct variant_data variant_qcom = {
 	.fifosize		= 16 * 4,
 	.fifohalfsize		= 8 * 4,
@@ -2033,6 +2050,7 @@ static struct amba_driver mmci_driver = {
 #endif
 
 static const struct of_device_id mmci_pltfm_match[] = {
+	{ .compatible = "st,stm32f4xx-sdio", .data = &variant_stm32},
 	{},
 };
 
-- 
2.7.4

[PATCH v3 6/9] ARM: DTS: stm32: add pin map for SDIO controller on stm32f429

[Andrea Merello]

This patch adds the pin configuration for SDIO controller on
stm32f429

Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
 arch/arm/boot/dts/stm32f429.dtsi | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
index e4dae0e..de7b9c3 100644
--- a/arch/arm/boot/dts/stm32f429.dtsi
+++ b/arch/arm/boot/dts/stm32f429.dtsi
@@ -336,7 +336,38 @@
 				};
 			};
 
-			ethernet_mii: mii@0 {
+			sdio_pins: sdio_pins@0 {
+				pins {
+					pinmux = <STM32F429_PC8_FUNC_SDIO_D0>,
+						 <STM32F429_PC9_FUNC_SDIO_D1>,
+						 <STM32F429_PC10_FUNC_SDIO_D2>,
+						 <STM32F429_PC11_FUNC_SDIO_D3>,
+						 <STM32F429_PC12_FUNC_SDIO_CK>,
+						 <STM32F429_PD2_FUNC_SDIO_CMD>;
+					drive-push-pull;
+					slew-rate = <2>;
+				};
+			};
+
+			sdio_pins_od: sdio_pins_od@0 {
+				pins1 {
+					pinmux = <STM32F429_PC8_FUNC_SDIO_D0>,
+						 <STM32F429_PC9_FUNC_SDIO_D1>,
+						 <STM32F429_PC10_FUNC_SDIO_D2>,
+						 <STM32F429_PC11_FUNC_SDIO_D3>,
+						 <STM32F429_PC12_FUNC_SDIO_CK>;
+					drive-push-pull;
+					slew-rate = <2>;
+				};
+
+				pins2 {
+					pinmux = <STM32F429_PD2_FUNC_SDIO_CMD>;
+					drive-open-drain;
+					slew-rate = <2>;
+				};
+			};
+
+			ethernet0_mii: mii@0 {
 				pins {
 					pinmux = <STM32F429_PG13_FUNC_ETH_MII_TXD0_ETH_RMII_TXD0>,
 						 <STM32F429_PG14_FUNC_ETH_MII_TXD1_ETH_RMII_TXD1>,
-- 
2.7.4

[PATCH v3 7/9] ARM: DTS: stm32: add node for SDIO controller on stm32f429

[Andrea Merello]

this patch adds the sdio controller DT note on stm32f429

Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
 arch/arm/boot/dts/stm32f429.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
index de7b9c3..c12a64e 100644
--- a/arch/arm/boot/dts/stm32f429.dtsi
+++ b/arch/arm/boot/dts/stm32f429.dtsi
@@ -388,6 +388,18 @@
 			};
 		};
 
+		sdio: sdio@40012c00 {
+			compatible = "st,stm32f4xx-sdio";
+			reg = <0x40012c00 0x400>;
+			clocks = <&rcc 0 171>;
+			interrupts = <49>;
+			status = "disabled";
+			pinctrl-0 = <&sdio_pins>;
+			pinctrl-1 = <&sdio_pins_od>;
+			pinctrl-names = "default", "opendrain";
+			max-frequency = <48000000>;
+		};
+
 		rcc: rcc@40023810 {
 			#reset-cells = <1>;
 			#clock-cells = <2>;
-- 
2.7.4

[PATCH v3 8/9] ARM: DTS: enable SDIO controller on stm32f469 disco board

[Andrea Merello]

This patch adds SDIO-related DT nodes required by stm32f469 board

Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
 arch/arm/boot/dts/stm32f429.dtsi      |  2 +-
 arch/arm/boot/dts/stm32f469-disco.dts | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
index c12a64e..5aba383 100644
--- a/arch/arm/boot/dts/stm32f429.dtsi
+++ b/arch/arm/boot/dts/stm32f429.dtsi
@@ -206,7 +206,7 @@
 			reg = <0x40007000 0x400>;
 		};
 
-		pin-controller {
+		pinctrl:pin-controller {
 			#address-cells = <1>;
 			#size-cells = <1>;
 			compatible = "st,stm32f429-pinctrl";
diff --git a/arch/arm/boot/dts/stm32f469-disco.dts b/arch/arm/boot/dts/stm32f469-disco.dts
index 8877c00..420ccb3 100644
--- a/arch/arm/boot/dts/stm32f469-disco.dts
+++ b/arch/arm/boot/dts/stm32f469-disco.dts
@@ -65,6 +65,13 @@
 		serial0 = &usart3;
 	};
 
+	mmc_vcard: mmc_vcard {
+		compatible = "regulator-fixed";
+		regulator-name = "mmc_vcard";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+	};
+
 	soc {
 		dma-ranges = <0xc0000000 0x0 0x10000000>;
 	};
@@ -78,6 +85,17 @@
 	clock-frequency = <8000000>;
 };
 
+&sdio {
+	status = "okay";
+	vmmc-supply = <&mmc_vcard>;
+	cd-gpios = <&gpiog 2 0>;
+	cd-inverted;
+	pinctrl-names = "default", "opendrain";
+	pinctrl-0 = <&sdio_pins>;
+	pinctrl-1 = <&sdio_pins_od>;
+	bus-width = <4>;
+};
+
 &usart3 {
 	status = "okay";
 };
-- 
2.7.4

[PATCH v3 9/9] Documentation: document mmci STM32 DT binding

[Andrea Merello]

Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
 Documentation/devicetree/bindings/mmc/mmci.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mmc/mmci.txt b/Documentation/devicetree/bindings/mmc/mmci.txt
index 03796cf..d6b5127 100644
--- a/Documentation/devicetree/bindings/mmc/mmci.txt
+++ b/Documentation/devicetree/bindings/mmc/mmci.txt
@@ -8,7 +8,7 @@ by mmc.txt and the properties used by the mmci driver. Using "st" as
 the prefix for a property, indicates support by the ST Micro variant.
 
 Required properties:
-- compatible             : contains "arm,pl18x", "arm,primecell".
+- compatible             : contains "arm,pl18x", "arm,primecell" or "st,stm32f4xx-sdio"
 - vmmc-supply            : phandle to the regulator device tree node, mentioned
                            as the VCC/VDD supply in the eMMC/SD specs.
 
-- 
2.7.4

Re: [PATCH v3 1/9] mmc: mmci: don't pretend IP variants with only one IRQ to have two mask regs

[Ulf Hansson]

On 16 January 2017 at 08:37, Andrea Merello <andrea.merello@gmail.com> wrote:
> Currently the driver supports both devices with one and two IRQ lines.
>
> Two mask registers are used in order to select which events have to
> actually generate an interrupt on each IRQ line.
>
> It seems that in the single-IRQ case it's assumed that the IRQs lines
> are simply OR-ed, while the two mask registers are still present. The
> driver still programs the two mask registers separately.
>
> However the STM32 variant has only one IRQ, and also has only one mask
> register.
>
> This patch prepares for STM32 variant support by making the driver using
> only one mask register when only one IRQ is available.

The approach as such seems okay to try out!

However, I think a minor re-work is needed, but in the end achieving
the same goal. Some more comments below.

>
> Tested only on STM32 variant. RFT for variants other than STM32
>
> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> ---
>  drivers/mmc/host/mmci.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 01a8047..597fab8 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -404,9 +404,9 @@ static void mmci_set_mask1(struct mmci_host *host, unsigned int mask)
>                 mask0 |= mask;
>
>                 writel(mask0, base + MMCIMASK0);
> +       } else {
> +               writel(mask, base + MMCIMASK1);
>         }
> -
> -       writel(mask, base + MMCIMASK1);
>  }
>
>  static void mmci_stop_data(struct mmci_host *host)
> @@ -1274,12 +1274,10 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>
>         do {
>                 status = readl(host->base + MMCISTATUS);
> +               status &= readl(host->base + MMCIMASK0);
>                 if (host->singleirq) {
> -                       if (status & readl(host->base + MMCIMASK1))
> -                               mmci_pio_irq(irq, dev_id);

We want to enter mmci_pio_irq(), *only* when some of those bits are
set, which we have written to in MMCIMASK1.

This changes this behaviour, which I don't think is a good idea!

> -
> -                       status &= ~MCI_IRQ1MASK;
> +                       mmci_pio_irq(irq, dev_id);
>                 }
>
>                 /*
> @@ -1287,7 +1285,6 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>                  * enabled) since the HW seems to be triggering the IRQ on both
>                  * edges while monitoring DAT0 for busy completion.
>                  */
> -               status &= readl(host->base + MMCIMASK0);
>                 writel(status, host->base + MMCICLEAR);
>
>                 dev_dbg(mmc_dev(host->mmc), "irq0 (data+cmd) %08x\n", status);
> @@ -1710,7 +1707,8 @@ static int mmci_probe(struct amba_device *dev,
>         spin_lock_init(&host->lock);
>
>         writel(0, host->base + MMCIMASK0);
> -       writel(0, host->base + MMCIMASK1);
> +       if (!host->singleirq)
> +               writel(0, host->base + MMCIMASK1);

No, this is wrong.

In the case when we use only a single IRQ, we also need to clear
MMCIMASK1, as to make sure it don't contains some "garbage" and thus
triggers spurious IRQs.

>         writel(0xfff, host->base + MMCICLEAR);
>
>         /*
> @@ -1800,7 +1798,8 @@ static int mmci_remove(struct amba_device *dev)
>                 mmc_remove_host(mmc);
>
>                 writel(0, host->base + MMCIMASK0);
> -               writel(0, host->base + MMCIMASK1);
> +               if (!host->singleirq)
> +                       writel(0, host->base + MMCIMASK1);

Ditto.

>
>                 writel(0, host->base + MMCICOMMAND);
>                 writel(0, host->base + MMCIDATACTRL);
> --
> 2.7.4
>

Overall, by looking at this change I think an alternative approach
would be start with some clean-up/micro-optimizations for how the
driver deals with the MMCIMASK1 and MMCIMASK0.

More precisely, add two new variables in the mmci host struct, used
for containg a cache of these two registers. The cache needs to be
updated when new values are written, but more importantly, we don't
need to go and read the corresponding registers to know their content,
when masking/unmasking bits. Instead we can use the cache, which is
more optimal.

Following patches, similar to what you suggested in $subject patch,
can then make use of the cached values. Depending on whether
"singleirq" is set or not, the cached values can be used to write the
correct mask(s).

Kind regards
Uffe

Re: [PATCH v3 2/9] mmc: mmci: add support for not-amba, but still compatible, variants

[Ulf Hansson]

+ Russell

On 16 January 2017 at 08:37, Andrea Merello <andrea.merello@gmail.com> wrote:
> The STM32F4xx family contain a SDIO IP that looks like a variant of
> the PL180, however, inspite it's actually attached to a APB bus, it
> cannot be handled by the AMBA bus code, because it lacks of the ID
> registers that AMBA primecell IPs have.
>
> This patch prepares for supporting the STM32 variant by letting the
> driver register also as a platform driver, that can be matched from
> the OF with specific "compatible" strings

NAK!

Too much code are depending on the amba bus in the driver. I sincerely
hope you haven't tried this approach for other amba drivers, because
it will screw them up and they will become a nightmare to maintain!

Moreover, there is a way to override the use of the AMBA IDs
registers. I think you can use that instead! No?

Kind regards
Uffe

>
> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> ---
>  drivers/mmc/host/Kconfig |   8 +-
>  drivers/mmc/host/mmci.c  | 297 ++++++++++++++++++++++++++++++++++++-----------
>  2 files changed, 236 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 2eb9701..f8f7f289 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -5,12 +5,12 @@
>  comment "MMC/SD/SDIO Host Controller Drivers"
>
>  config MMC_ARMMMCI
> -       tristate "ARM AMBA Multimedia Card Interface support"
> -       depends on ARM_AMBA
> +       tristate "ARM AMBA Multimedia Card Interface and compatible support"
>         help
>           This selects the ARM(R) AMBA(R) PrimeCell Multimedia Card
> -         Interface (PL180 and PL181) support.  If you have an ARM(R)
> -         platform with a Multimedia Card slot, say Y or M here.
> +         Interface (PL180, PL181 and compatible) support.
> +         If you have an ARM(R) platform with a Multimedia Card slot,
> +         say Y or M here.
>
>           If unsure, say N.
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 597fab8..54c2fc3 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -1,5 +1,6 @@
>  /*
> - *  linux/drivers/mmc/host/mmci.c - ARM PrimeCell MMCI PL180/1 driver
> + *  linux/drivers/mmc/host/mmci.c - ARM PrimeCell MMCI PL180/1 and
> + *  comaptible driver
>   *
>   *  Copyright (C) 2003 Deep Blue Solutions, Ltd, All Rights Reserved.
>   *  Copyright (C) 2010 ST-Ericsson SA
> @@ -37,6 +38,8 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/types.h>
>  #include <linux/pinctrl/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_device.h>
>
>  #include <asm/div64.h>
>  #include <asm/io.h>
> @@ -113,7 +116,7 @@ struct variant_data {
>         bool                    reversed_irq_handling;
>  };
>
> -static struct variant_data variant_arm = {
> +static __maybe_unused struct variant_data variant_arm = {
>         .fifosize               = 16 * 4,
>         .fifohalfsize           = 8 * 4,
>         .datalength_bits        = 16,
> @@ -122,7 +125,7 @@ static struct variant_data variant_arm = {
>         .reversed_irq_handling  = true,
>  };
>
> -static struct variant_data variant_arm_extended_fifo = {
> +static __maybe_unused struct variant_data variant_arm_extended_fifo = {
>         .fifosize               = 128 * 4,
>         .fifohalfsize           = 64 * 4,
>         .datalength_bits        = 16,
> @@ -130,7 +133,7 @@ static struct variant_data variant_arm_extended_fifo = {
>         .f_max                  = 100000000,
>  };
>
> -static struct variant_data variant_arm_extended_fifo_hwfc = {
> +static __maybe_unused struct variant_data variant_arm_extended_fifo_hwfc = {
>         .fifosize               = 128 * 4,
>         .fifohalfsize           = 64 * 4,
>         .clkreg_enable          = MCI_ARM_HWFCEN,
> @@ -139,7 +142,7 @@ static struct variant_data variant_arm_extended_fifo_hwfc = {
>         .f_max                  = 100000000,
>  };
>
> -static struct variant_data variant_u300 = {
> +static __maybe_unused struct variant_data variant_u300 = {
>         .fifosize               = 16 * 4,
>         .fifohalfsize           = 8 * 4,
>         .clkreg_enable          = MCI_ST_U300_HWFCEN,
> @@ -154,7 +157,7 @@ static struct variant_data variant_u300 = {
>         .pwrreg_nopower         = true,
>  };
>
> -static struct variant_data variant_nomadik = {
> +static __maybe_unused struct variant_data variant_nomadik = {
>         .fifosize               = 16 * 4,
>         .fifohalfsize           = 8 * 4,
>         .clkreg                 = MCI_CLK_ENABLE,
> @@ -170,7 +173,7 @@ static struct variant_data variant_nomadik = {
>         .pwrreg_nopower         = true,
>  };
>
> -static struct variant_data variant_ux500 = {
> +static __maybe_unused struct variant_data variant_ux500 = {
>         .fifosize               = 30 * 4,
>         .fifohalfsize           = 8 * 4,
>         .clkreg                 = MCI_CLK_ENABLE,
> @@ -192,7 +195,7 @@ static struct variant_data variant_ux500 = {
>         .pwrreg_nopower         = true,
>  };
>
> -static struct variant_data variant_ux500v2 = {
> +static __maybe_unused struct variant_data variant_ux500v2 = {
>         .fifosize               = 30 * 4,
>         .fifohalfsize           = 8 * 4,
>         .clkreg                 = MCI_CLK_ENABLE,
> @@ -216,7 +219,7 @@ static struct variant_data variant_ux500v2 = {
>         .pwrreg_nopower         = true,
>  };
>
> -static struct variant_data variant_qcom = {
> +static __maybe_unused struct variant_data variant_qcom = {
>         .fifosize               = 16 * 4,
>         .fifohalfsize           = 8 * 4,
>         .clkreg                 = MCI_CLK_ENABLE,
> @@ -1528,31 +1531,32 @@ static int mmci_of_parse(struct device_node *np, struct mmc_host *mmc)
>         return 0;
>  }
>
> -static int mmci_probe(struct amba_device *dev,
> -       const struct amba_id *id)
> +static struct mmci_host *mmci_probe(struct device *dev,
> +                                   struct variant_data *variant,
> +                                   struct resource *res,
> +                                   unsigned int irq0, unsigned int irq1)
>  {
> -       struct mmci_platform_data *plat = dev->dev.platform_data;
> -       struct device_node *np = dev->dev.of_node;
> -       struct variant_data *variant = id->data;
> -       struct mmci_host *host;
> +       struct device_node *np = dev->of_node;
> +       struct mmci_platform_data *plat = dev->platform_data;
>         struct mmc_host *mmc;
> +       struct mmci_host *host;
>         int ret;
>
>         /* Must have platform data or Device Tree. */
>         if (!plat && !np) {
> -               dev_err(&dev->dev, "No plat data or DT found\n");
> -               return -EINVAL;
> +               dev_err(dev, "No plat data or DT found\n");
> +               return ERR_PTR(-EINVAL);
>         }
>
>         if (!plat) {
> -               plat = devm_kzalloc(&dev->dev, sizeof(*plat), GFP_KERNEL);
> +               plat = devm_kzalloc(dev, sizeof(*plat), GFP_KERNEL);
>                 if (!plat)
> -                       return -ENOMEM;
> +                       return ERR_PTR(-ENOMEM);
>         }
>
> -       mmc = mmc_alloc_host(sizeof(struct mmci_host), &dev->dev);
> +       mmc = mmc_alloc_host(sizeof(struct mmci_host), dev);
>         if (!mmc)
> -               return -ENOMEM;
> +               return ERR_PTR(-ENOMEM);
>
>         ret = mmci_of_parse(np, mmc);
>         if (ret)
> @@ -1561,12 +1565,7 @@ static int mmci_probe(struct amba_device *dev,
>         host = mmc_priv(mmc);
>         host->mmc = mmc;
>
> -       host->hw_designer = amba_manf(dev);
> -       host->hw_revision = amba_rev(dev);
> -       dev_dbg(mmc_dev(mmc), "designer ID = 0x%02x\n", host->hw_designer);
> -       dev_dbg(mmc_dev(mmc), "revision = 0x%01x\n", host->hw_revision);
> -
> -       host->clk = devm_clk_get(&dev->dev, NULL);
> +       host->clk = devm_clk_get(dev, NULL);
>         if (IS_ERR(host->clk)) {
>                 ret = PTR_ERR(host->clk);
>                 goto host_free;
> @@ -1598,8 +1597,8 @@ static int mmci_probe(struct amba_device *dev,
>                         host->mclk);
>         }
>
> -       host->phybase = dev->res.start;
> -       host->base = devm_ioremap_resource(&dev->dev, &dev->res);
> +       host->phybase = res->start;
> +       host->base = devm_ioremap_resource(dev, res);
>         if (IS_ERR(host->base)) {
>                 ret = PTR_ERR(host->base);
>                 goto clk_disable;
> @@ -1742,49 +1741,41 @@ static int mmci_probe(struct amba_device *dev,
>                 }
>         }
>
> -       ret = devm_request_irq(&dev->dev, dev->irq[0], mmci_irq, IRQF_SHARED,
> -                       DRIVER_NAME " (cmd)", host);
> +       ret = devm_request_irq(dev, irq0, mmci_irq, IRQF_SHARED,
> +                              DRIVER_NAME " (cmd)", host);
>         if (ret)
>                 goto clk_disable;
>
> -       if (!dev->irq[1])
> +       if (!irq1) {
>                 host->singleirq = true;
> -       else {
> -               ret = devm_request_irq(&dev->dev, dev->irq[1], mmci_pio_irq,
> -                               IRQF_SHARED, DRIVER_NAME " (pio)", host);
> +       } else {
> +               ret = devm_request_irq(dev, irq1, mmci_pio_irq,
> +                                      IRQF_SHARED, DRIVER_NAME " (pio)", host);
>                 if (ret)
>                         goto clk_disable;
>         }
>
>         writel(MCI_IRQENABLE, host->base + MMCIMASK0);
>
> -       amba_set_drvdata(dev, mmc);
> -
> -       dev_info(&dev->dev, "%s: PL%03x manf %x rev%u at 0x%08llx irq %d,%d (pio)\n",
> -                mmc_hostname(mmc), amba_part(dev), amba_manf(dev),
> -                amba_rev(dev), (unsigned long long)dev->res.start,
> -                dev->irq[0], dev->irq[1]);
> -
>         mmci_dma_setup(host);
>
> -       pm_runtime_set_autosuspend_delay(&dev->dev, 50);
> -       pm_runtime_use_autosuspend(&dev->dev);
> +       pm_runtime_set_autosuspend_delay(dev, 50);
> +       pm_runtime_use_autosuspend(dev);
>
>         mmc_add_host(mmc);
>
> -       pm_runtime_put(&dev->dev);
> -       return 0;
> +       pm_runtime_put(dev);
> +       return host;
>
>   clk_disable:
>         clk_disable_unprepare(host->clk);
>   host_free:
>         mmc_free_host(mmc);
> -       return ret;
> +       return ERR_PTR(ret);
>  }
>
> -static int mmci_remove(struct amba_device *dev)
> +static int mmc_remove(struct mmc_host *mmc)
>  {
> -       struct mmc_host *mmc = amba_get_drvdata(dev);
>
>         if (mmc) {
>                 struct mmci_host *host = mmc_priv(mmc);
> @@ -1793,7 +1784,7 @@ static int mmci_remove(struct amba_device *dev)
>                  * Undo pm_runtime_put() in probe.  We use the _sync
>                  * version here so that we can access the primecell.
>                  */
> -               pm_runtime_get_sync(&dev->dev);
> +               pm_runtime_get_sync(mmc->parent);
>
>                 mmc_remove_host(mmc);
>
> @@ -1847,14 +1838,11 @@ static void mmci_restore(struct mmci_host *host)
>         spin_unlock_irqrestore(&host->lock, flags);
>  }
>
> -static int mmci_runtime_suspend(struct device *dev)
> +static int mmci_runtime_suspend(struct mmc_host *mmc)
>  {
> -       struct amba_device *adev = to_amba_device(dev);
> -       struct mmc_host *mmc = amba_get_drvdata(adev);
> -
>         if (mmc) {
>                 struct mmci_host *host = mmc_priv(mmc);
> -               pinctrl_pm_select_sleep_state(dev);
> +               pinctrl_pm_select_sleep_state(mmc->parent);
>                 mmci_save(host);
>                 clk_disable_unprepare(host->clk);
>         }
> @@ -1862,28 +1850,77 @@ static int mmci_runtime_suspend(struct device *dev)
>         return 0;
>  }
>
> -static int mmci_runtime_resume(struct device *dev)
> +static int mmci_runtime_resume(struct mmc_host *mmc)
>  {
> -       struct amba_device *adev = to_amba_device(dev);
> -       struct mmc_host *mmc = amba_get_drvdata(adev);
>
>         if (mmc) {
>                 struct mmci_host *host = mmc_priv(mmc);
>                 clk_prepare_enable(host->clk);
>                 mmci_restore(host);
> -               pinctrl_pm_select_default_state(dev);
> +               pinctrl_pm_select_default_state(mmc->parent);
>         }
>
>         return 0;
>  }
>  #endif
>
> -static const struct dev_pm_ops mmci_dev_pm_ops = {
> +#ifdef CONFIG_ARM_AMBA
> +static int mmci_remove_amba(struct amba_device *dev)
> +{
> +       struct mmc_host *mmc = amba_get_drvdata(dev);
> +
> +       return mmc_remove(mmc);
> +}
> +
> +static int mmci_runtime_resume_amba(struct device *dev)
> +{
> +       struct amba_device *adev = to_amba_device(dev);
> +       struct mmc_host *mmc = amba_get_drvdata(adev);
> +
> +       return mmci_runtime_resume(mmc);
> +}
> +
> +static int mmci_runtime_suspend_amba(struct device *dev)
> +{
> +       struct amba_device *adev = to_amba_device(dev);
> +       struct mmc_host *mmc = amba_get_drvdata(adev);
> +
> +       return mmci_runtime_suspend(mmc);
> +}
> +
> +static const struct dev_pm_ops mmci_dev_pm_ops_amba = {
>         SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>                                 pm_runtime_force_resume)
> -       SET_RUNTIME_PM_OPS(mmci_runtime_suspend, mmci_runtime_resume, NULL)
> +       SET_RUNTIME_PM_OPS(mmci_runtime_suspend_amba, mmci_runtime_resume_amba,
> +                          NULL)
>  };
>
> +static int mmci_probe_amba(struct amba_device *dev, const struct amba_id *id)
> +{
> +       struct mmci_host *host;
> +       struct variant_data *variant = id->data;
> +
> +       host = mmci_probe(&dev->dev, variant, &dev->res,
> +                         dev->irq[0], dev->irq[1]);
> +       if (IS_ERR(host))
> +               return PTR_ERR(host);
> +
> +       host->hw_designer = amba_manf(dev);
> +       host->hw_revision = amba_rev(dev);
> +       dev_dbg(mmc_dev(host->mmc), "designer ID = 0x%02x\n",
> +               host->hw_designer);
> +       dev_dbg(mmc_dev(host->mmc), "revision = 0x%01x\n", host->hw_revision);
> +
> +       amba_set_drvdata(dev, host->mmc);
> +
> +       dev_info(&dev->dev, "%s: PL%03x manf %x rev%u at 0x%08llx irq %d,%d (pio)\n",
> +                mmc_hostname(host->mmc), amba_part(dev), amba_manf(dev),
> +                amba_rev(dev), (unsigned long long)dev->res.start,
> +                dev->irq[0], dev->irq[1]);
> +
> +       return 0;
> +}
> +
>  static struct amba_id mmci_ids[] = {
>         {
>                 .id     = 0x00041180,
> @@ -1945,14 +1982,144 @@ MODULE_DEVICE_TABLE(amba, mmci_ids);
>  static struct amba_driver mmci_driver = {
>         .drv            = {
>                 .name   = DRIVER_NAME,
> -               .pm     = &mmci_dev_pm_ops,
> +               .pm     = &mmci_dev_pm_ops_amba,
>         },
> -       .probe          = mmci_probe,
> -       .remove         = mmci_remove,
> +       .probe          = mmci_probe_amba,
> +       .remove         = mmci_remove_amba,
>         .id_table       = mmci_ids,
>  };
> +#endif
> +
> +static const struct of_device_id mmci_pltfm_match[] = {
> +       {},
> +};
> +
> +static int mmci_probe_pltfm(struct platform_device *pdev)
> +{
> +       struct mmci_host *host;
> +       struct variant_data *variant;
> +       const struct of_device_id *match;
> +       int irq0, irq1;
> +       struct resource *res;
> +
> +       irq0 = platform_get_irq(pdev, 0);
> +       if (irq0 < 0) {
> +               dev_err(&pdev->dev, "Can't get IRQ");
> +               return -EINVAL;
> +       }
> +
> +       irq1 = platform_get_irq(pdev, 1);
> +       /* optional. set to 0 to be compatible with original amba probe code */
> +       if (irq1 < 0)
> +               irq1 = 0;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!res) {
> +               dev_err(&pdev->dev, "Can't get MMIO range");
> +               return -EINVAL;
> +       }
> +
> +       match = of_match_device(mmci_pltfm_match, &pdev->dev);
> +       if (!match) {
> +               dev_err(&pdev->dev, "Can't get variant data\n");
> +               return -EINVAL;
> +       }
> +       variant = (struct variant_data *)match->data;
> +
> +       host = mmci_probe(&pdev->dev, variant, res,
> +                         irq0, irq1);
> +       if (IS_ERR(host))
> +               return PTR_ERR(host);
> +
> +       platform_set_drvdata(pdev, host->mmc);
> +       dev_info(&pdev->dev, "%s: PL81x compatible SDIO at 0x%08llx irq %d,%d (pio)\n",
> +                mmc_hostname(host->mmc),
> +                (unsigned long long)res->start, irq0, irq1);
> +
> +       return 0;
> +}
> +
> +static int mmci_remove_pltfm(struct platform_device *pdev)
> +{
> +       struct mmc_host *mmc = platform_get_drvdata(pdev);
> +
> +       return mmc_remove(mmc);
> +}
> +
> +static int mmci_runtime_resume_pltfm(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct mmc_host *mmc = platform_get_drvdata(pdev);
> +
> +       return mmci_runtime_resume(mmc);
> +}
> +
> +static int mmci_runtime_suspend_pltfm(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct mmc_host *mmc = platform_get_drvdata(pdev);
> +
> +       return mmci_runtime_suspend(mmc);
> +}
> +
> +MODULE_DEVICE_TABLE(of, mmci_pltfm_match);
> +
> +static const struct dev_pm_ops mmci_dev_pm_ops_pltfm = {
> +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +                               pm_runtime_force_resume)
> +       SET_RUNTIME_PM_OPS(mmci_runtime_suspend_pltfm,
> +                          mmci_runtime_resume_pltfm, NULL)
> +};
> +
> +static struct platform_driver mmci_pltfm_driver = {
> +       .probe          = mmci_probe_pltfm,
> +       .remove         = mmci_remove_pltfm,
> +       .driver         = {
> +               .name           = DRIVER_NAME,
> +               .of_match_table = mmci_pltfm_match,
> +               .pm             = &mmci_dev_pm_ops_pltfm,
> +       },
> +};
> +
> +static int __init mmci_init(void)
> +{
> +       int ret;
> +       __maybe_unused int ret_amba;
> +
> +       ret = platform_driver_register(&mmci_pltfm_driver);
> +
> +#ifdef CONFIG_ARM_AMBA
> +       ret_amba = amba_driver_register(&mmci_driver);
> +
> +       /* if both plaform and amba failed. Just fail. */
> +       if (ret && ret_amba)
> +               return ret;
> +
> +       /*
> +        * If only one of the two succeeded, don't fail here, because the
> +        * other one is probably usable, but spit a warning..
> +        */
> +       if (ret_amba)
> +               pr_warn("could not register MMCI amba driver");
> +
> +       if (ret)
> +               pr_warn("could not register MMCI platform driver");
> +
> +       return 0;
> +#endif
> +       return ret;
> +}
> +
> +static void __exit mmci_exit(void)
> +{
> +       platform_driver_unregister(&mmci_pltfm_driver);
> +#ifdef CONFIG_ARM_AMBA
> +       amba_driver_unregister(&mmci_driver);
> +#endif
> +}
>
> -module_amba_driver(mmci_driver);
> +module_init(mmci_init);
> +module_exit(mmci_exit);
>
>  module_param(fmax, uint, 0444);
>
> --
> 2.7.4
>

Re: [PATCH v3 2/9] mmc: mmci: add support for not-amba, but still compatible, variants

[Russell King - ARM Linux]

On Thu, Jan 19, 2017 at 05:58:10PM +0100, Ulf Hansson wrote:
> + Russell
> 
> On 16 January 2017 at 08:37, Andrea Merello <andrea.merello@gmail.com> wrote:
> > The STM32F4xx family contain a SDIO IP that looks like a variant of
> > the PL180, however, inspite it's actually attached to a APB bus, it
> > cannot be handled by the AMBA bus code, because it lacks of the ID
> > registers that AMBA primecell IPs have.
> >
> > This patch prepares for supporting the STM32 variant by letting the
> > driver register also as a platform driver, that can be matched from
> > the OF with specific "compatible" strings
> 
> NAK!

Also NAK.

> Too much code are depending on the amba bus in the driver. I sincerely
> hope you haven't tried this approach for other amba drivers, because
> it will screw them up and they will become a nightmare to maintain!
> 
> Moreover, there is a way to override the use of the AMBA IDs
> registers. I think you can use that instead! No?

Exactly right.  Nothing wrong with creating an amba device in response
to probing a platform device - which becomes a shim which keeps the
main driver clean and maintanable.  The shim could also be used to
create amba device for other AMBA drivers too, it doesn't have to be
MMCI specific.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v3 2/9] mmc: mmci: add support for not-amba, but still compatible, variants

[Andrea Merello]

On Thu, Jan 19, 2017 at 6:11 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Thu, Jan 19, 2017 at 05:58:10PM +0100, Ulf Hansson wrote:
>> + Russell
>>
>> On 16 January 2017 at 08:37, Andrea Merello <andrea.merello@gmail.com> wrote:
>> > The STM32F4xx family contain a SDIO IP that looks like a variant of
>> > the PL180, however, inspite it's actually attached to a APB bus, it
>> > cannot be handled by the AMBA bus code, because it lacks of the ID
>> > registers that AMBA primecell IPs have.
>> >
>> > This patch prepares for supporting the STM32 variant by letting the
>> > driver register also as a platform driver, that can be matched from
>> > the OF with specific "compatible" strings
>>
>> NAK!
>
> Also NAK.
>
>> Too much code are depending on the amba bus in the driver. I sincerely
>> hope you haven't tried this approach for other amba drivers, because
>> it will screw them up and they will become a nightmare to maintain!

It's almost only probe/remove code.. Nothing in the core of the driver
gets involved here.. What about splitting the driver in three files
(mmci.c, mmci_amba.c and mmci_plat.c) and compile them conditionally?

BTW This approach is not my invention; there is at least another
driver in mainline kernel that does that: amba-pl011.c

>> Moreover, there is a way to override the use of the AMBA IDs
>> registers. I think you can use that instead! No?
>

I'm aware of it; I used it as a quick hack to get the driver probed,
but that looked just like as an hack to me..

> Exactly right.  Nothing wrong with creating an amba device in response
> to probing a platform device - which becomes a shim which keeps the
> main driver clean and maintanable.  The shim could also be used to
> create amba device for other AMBA drivers too, it doesn't have to be
> MMCI specific.

If we ovverride the amba regs from DT (is this what you meant?) then
we just take a random number, prentend it to be an amba ID (hoping for
it to be spare), and use it to force the amba bus layer to probe our
driver. How can we make this common for other drivers/devices ? We
need it to be unique, since it's used by the amba layer to probe the
correct driver (given the generic "arm,primecell" compatible DT
property), or am I missing something ?

> --
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.

Re: [PATCH v3 1/9] mmc: mmci: don't pretend IP variants with only one IRQ to have two mask regs

[Andrea Merello]

On Thu, Jan 19, 2017 at 5:44 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 16 January 2017 at 08:37, Andrea Merello <andrea.merello@gmail.com> wrote:
>> Currently the driver supports both devices with one and two IRQ lines.
>>
>> Two mask registers are used in order to select which events have to
>> actually generate an interrupt on each IRQ line.
>>
>> It seems that in the single-IRQ case it's assumed that the IRQs lines
>> are simply OR-ed, while the two mask registers are still present. The
>> driver still programs the two mask registers separately.
>>
>> However the STM32 variant has only one IRQ, and also has only one mask
>> register.
>>
>> This patch prepares for STM32 variant support by making the driver using
>> only one mask register when only one IRQ is available.
>
> The approach as such seems okay to try out!
>
> However, I think a minor re-work is needed, but in the end achieving
> the same goal. Some more comments below.
>
>>
>> Tested only on STM32 variant. RFT for variants other than STM32
>>
>> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
>> ---
>>  drivers/mmc/host/mmci.c | 17 ++++++++---------
>>  1 file changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>> index 01a8047..597fab8 100644
>> --- a/drivers/mmc/host/mmci.c
>> +++ b/drivers/mmc/host/mmci.c
>> @@ -404,9 +404,9 @@ static void mmci_set_mask1(struct mmci_host *host, unsigned int mask)
>>                 mask0 |= mask;
>>
>>                 writel(mask0, base + MMCIMASK0);
>> +       } else {
>> +               writel(mask, base + MMCIMASK1);
>>         }
>> -
>> -       writel(mask, base + MMCIMASK1);
>>  }
>>
>>  static void mmci_stop_data(struct mmci_host *host)
>> @@ -1274,12 +1274,10 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>>
>>         do {
>>                 status = readl(host->base + MMCISTATUS);
>> +               status &= readl(host->base + MMCIMASK0);
>>                 if (host->singleirq) {
>> -                       if (status & readl(host->base + MMCIMASK1))
>> -                               mmci_pio_irq(irq, dev_id);
>
> We want to enter mmci_pio_irq(), *only* when some of those bits are
> set, which we have written to in MMCIMASK1.
>
> This changes this behaviour, which I don't think is a good idea!

The idea is that when we have host->singleirq we write all the bits we
need only in MMCIMASK0, so there would be no point in looking at
MMCIMASK1.

>
>> -
>> -                       status &= ~MCI_IRQ1MASK;
>> +                       mmci_pio_irq(irq, dev_id);
>>                 }
>>
>>                 /*
>> @@ -1287,7 +1285,6 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>>                  * enabled) since the HW seems to be triggering the IRQ on both
>>                  * edges while monitoring DAT0 for busy completion.
>>                  */
>> -               status &= readl(host->base + MMCIMASK0);
>>                 writel(status, host->base + MMCICLEAR);
>>
>>                 dev_dbg(mmc_dev(host->mmc), "irq0 (data+cmd) %08x\n", status);
>> @@ -1710,7 +1707,8 @@ static int mmci_probe(struct amba_device *dev,
>>         spin_lock_init(&host->lock);
>>
>>         writel(0, host->base + MMCIMASK0);
>> -       writel(0, host->base + MMCIMASK1);
>> +       if (!host->singleirq)
>> +               writel(0, host->base + MMCIMASK1);
>
> No, this is wrong.
>
> In the case when we use only a single IRQ, we also need to clear
> MMCIMASK1, as to make sure it don't contains some "garbage" and thus
> triggers spurious IRQs.

You are right here. However the whole point of this patch is to avoid
writing/reading MMCMASK1 reg in variants that does not have this reg
at all. It looks like we need another flag like host->singlemask to
skip this writes in this case.

>>         writel(0xfff, host->base + MMCICLEAR);
>>
>>         /*
>> @@ -1800,7 +1798,8 @@ static int mmci_remove(struct amba_device *dev)
>>                 mmc_remove_host(mmc);
>>
>>                 writel(0, host->base + MMCIMASK0);
>> -               writel(0, host->base + MMCIMASK1);
>> +               if (!host->singleirq)
>> +                       writel(0, host->base + MMCIMASK1);
>
> Ditto.
>
>>
>>                 writel(0, host->base + MMCICOMMAND);
>>                 writel(0, host->base + MMCIDATACTRL);
>> --
>> 2.7.4
>>
>
> Overall, by looking at this change I think an alternative approach
> would be start with some clean-up/micro-optimizations for how the
> driver deals with the MMCIMASK1 and MMCIMASK0.
>
> More precisely, add two new variables in the mmci host struct, used
> for containg a cache of these two registers. The cache needs to be
> updated when new values are written, but more importantly, we don't
> need to go and read the corresponding registers to know their content,
> when masking/unmasking bits. Instead we can use the cache, which is
> more optimal.

This seems a good idea in general; it would be a slight improvement
for the MMCI driver in general, however it seems to me quite unrelated
to the point of this patch..

> Following patches, similar to what you suggested in $subject patch,
> can then make use of the cached values. Depending on whether
> "singleirq" is set or not, the cached values can be used to write the
> correct mask(s).
>
> Kind regards
> Uffe

Re: [PATCH v3 2/9] mmc: mmci: add support for not-amba, but still compatible, variants

[Bruno Herrera]

On Thu, Jan 19, 2017 at 3:40 PM, Andrea Merello
<andrea.merello@gmail.com> wrote:
> On Thu, Jan 19, 2017 at 6:11 PM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
>> On Thu, Jan 19, 2017 at 05:58:10PM +0100, Ulf Hansson wrote:
>>> + Russell
>>>
>>> On 16 January 2017 at 08:37, Andrea Merello <andrea.merello@gmail.com> wrote:
>>> > The STM32F4xx family contain a SDIO IP that looks like a variant of
>>> > the PL180, however, inspite it's actually attached to a APB bus, it
>>> > cannot be handled by the AMBA bus code, because it lacks of the ID
>>> > registers that AMBA primecell IPs have.
>>> >
>>> > This patch prepares for supporting the STM32 variant by letting the
>>> > driver register also as a platform driver, that can be matched from
>>> > the OF with specific "compatible" strings
>>>
>>> NAK!
>>
>> Also NAK.
>>
>>> Too much code are depending on the amba bus in the driver. I sincerely
>>> hope you haven't tried this approach for other amba drivers, because
>>> it will screw them up and they will become a nightmare to maintain!
>
> It's almost only probe/remove code.. Nothing in the core of the driver
> gets involved here.. What about splitting the driver in three files
> (mmci.c, mmci_amba.c and mmci_plat.c) and compile them conditionally?
>
> BTW This approach is not my invention; there is at least another
> driver in mainline kernel that does that: amba-pl011.c
>
>>> Moreover, there is a way to override the use of the AMBA IDs
>>> registers. I think you can use that instead! No?
>>
>
> I'm aware of it; I used it as a quick hack to get the driver probed,
> but that looked just like as an hack to me..

I did the same hackish approach and it works, but a first moment
rather than changing the code I would suggest to get this ID from ST.
Does not mean that as we cannot query from the IP core, it does not
exist (my guess).

>
>> Exactly right.  Nothing wrong with creating an amba device in response
>> to probing a platform device - which becomes a shim which keeps the
>> main driver clean and maintanable.  The shim could also be used to
>> create amba device for other AMBA drivers too, it doesn't have to be
>> MMCI specific.
>
> If we ovverride the amba regs from DT (is this what you meant?) then
> we just take a random number, prentend it to be an amba ID (hoping for
> it to be spare), and use it to force the amba bus layer to probe our
> driver. How can we make this common for other drivers/devices ? We
> need it to be unique, since it's used by the amba layer to probe the
> correct driver (given the generic "arm,primecell" compatible DT
> property), or am I missing something ?
>
>> --
>> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
>> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
>> according to speedtest.net.

Re: [PATCH v3 1/9] mmc: mmci: don't pretend IP variants with only one IRQ to have two mask regs

[Ulf Hansson]

[...]

>>> @@ -1274,12 +1274,10 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>>>
>>>         do {
>>>                 status = readl(host->base + MMCISTATUS);
>>> +               status &= readl(host->base + MMCIMASK0);
>>>                 if (host->singleirq) {
>>> -                       if (status & readl(host->base + MMCIMASK1))
>>> -                               mmci_pio_irq(irq, dev_id);
>>
>> We want to enter mmci_pio_irq(), *only* when some of those bits are
>> set, which we have written to in MMCIMASK1.
>>
>> This changes this behaviour, which I don't think is a good idea!
>
> The idea is that when we have host->singleirq we write all the bits we
> need only in MMCIMASK0, so there would be no point in looking at
> MMCIMASK1.

Right, I agree that is the next step to take.

However, we also don't want to invoke mmci_pio_irq() unless we got
some of the pio IRQs. The current method to do that is reading the
MMCIMASK1.

I do understand that doesn't work for your new variant, and perhaps
caching the registers could help to avoid that. Typically you could
just check against a "host->mask1_reg" variable, but having to
actually read the register.

>
>>
>>> -
>>> -                       status &= ~MCI_IRQ1MASK;
>>> +                       mmci_pio_irq(irq, dev_id);
>>>                 }
>>>
>>>                 /*
>>> @@ -1287,7 +1285,6 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>>>                  * enabled) since the HW seems to be triggering the IRQ on both
>>>                  * edges while monitoring DAT0 for busy completion.
>>>                  */
>>> -               status &= readl(host->base + MMCIMASK0);
>>>                 writel(status, host->base + MMCICLEAR);
>>>
>>>                 dev_dbg(mmc_dev(host->mmc), "irq0 (data+cmd) %08x\n", status);
>>> @@ -1710,7 +1707,8 @@ static int mmci_probe(struct amba_device *dev,
>>>         spin_lock_init(&host->lock);
>>>
>>>         writel(0, host->base + MMCIMASK0);
>>> -       writel(0, host->base + MMCIMASK1);
>>> +       if (!host->singleirq)
>>> +               writel(0, host->base + MMCIMASK1);
>>
>> No, this is wrong.
>>
>> In the case when we use only a single IRQ, we also need to clear
>> MMCIMASK1, as to make sure it don't contains some "garbage" and thus
>> triggers spurious IRQs.
>
> You are right here. However the whole point of this patch is to avoid
> writing/reading MMCMASK1 reg in variants that does not have this reg
> at all. It looks like we need another flag like host->singlemask to
> skip this writes in this case.

I understand your point and yes we will need another flag, telling if
the MMCIMASK1 exists or not.

I also like the idea of avoiding to use MMCIMASK1 for cases when it
exists but actually isn't needed. Which is a good first step to take,
in my opinion.

Of course, if you want to skip that step, I am fine with that as well.

>
>>>         writel(0xfff, host->base + MMCICLEAR);
>>>
>>>         /*
>>> @@ -1800,7 +1798,8 @@ static int mmci_remove(struct amba_device *dev)
>>>                 mmc_remove_host(mmc);
>>>
>>>                 writel(0, host->base + MMCIMASK0);
>>> -               writel(0, host->base + MMCIMASK1);
>>> +               if (!host->singleirq)
>>> +                       writel(0, host->base + MMCIMASK1);
>>
>> Ditto.
>>
>>>
>>>                 writel(0, host->base + MMCICOMMAND);
>>>                 writel(0, host->base + MMCIDATACTRL);
>>> --
>>> 2.7.4
>>>
>>
>> Overall, by looking at this change I think an alternative approach
>> would be start with some clean-up/micro-optimizations for how the
>> driver deals with the MMCIMASK1 and MMCIMASK0.
>>
>> More precisely, add two new variables in the mmci host struct, used
>> for containg a cache of these two registers. The cache needs to be
>> updated when new values are written, but more importantly, we don't
>> need to go and read the corresponding registers to know their content,
>> when masking/unmasking bits. Instead we can use the cache, which is
>> more optimal.
>
> This seems a good idea in general; it would be a slight improvement
> for the MMCI driver in general, however it seems to me quite unrelated
> to the point of this patch..

Besides the improvement, I think the code could be quite nice,
especially when we extend it to cover also this new case with a non
existing MMCIMASK1.

Although, it was just a suggestion. So if you can feel adding a
variant flag immediately for a non-existing MMCIMASK1 register and
adopt the code that, I can review such change as well.

Anyway, I can help testing with ux500 v2.

[...]

Kind regards
Uffe

Re: [PATCH v3 2/9] mmc: mmci: add support for not-amba, but still compatible, variants

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2586 bytes --]

Hi Andrea,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.10-rc4 next-20170120]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Andrea-Merello/mmc-mmci-add-support-for-STM32-SD-controller/20170116-163016
config: alpha-allyesconfig (attached as .config)
compiler: alpha-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=alpha 

All errors (new ones prefixed by >>):

   drivers/mmc/host/mmci.c: In function 'mmci_runtime_resume_pltfm':
>> drivers/mmc/host/mmci.c:2054:9: error: implicit declaration of function 'mmci_runtime_resume' [-Werror=implicit-function-declaration]
     return mmci_runtime_resume(mmc);
            ^~~~~~~~~~~~~~~~~~~
   drivers/mmc/host/mmci.c: In function 'mmci_runtime_suspend_pltfm':
>> drivers/mmc/host/mmci.c:2062:9: error: implicit declaration of function 'mmci_runtime_suspend' [-Werror=implicit-function-declaration]
     return mmci_runtime_suspend(mmc);
            ^~~~~~~~~~~~~~~~~~~~
   At top level:
   drivers/mmc/host/mmci.c:2057:12: warning: 'mmci_runtime_suspend_pltfm' defined but not used [-Wunused-function]
    static int mmci_runtime_suspend_pltfm(struct device *dev)
               ^~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/mmc/host/mmci.c:2049:12: warning: 'mmci_runtime_resume_pltfm' defined but not used [-Wunused-function]
    static int mmci_runtime_resume_pltfm(struct device *dev)
               ^~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/mmci_runtime_resume +2054 drivers/mmc/host/mmci.c

  2048	
  2049	static int mmci_runtime_resume_pltfm(struct device *dev)
  2050	{
  2051		struct platform_device *pdev = to_platform_device(dev);
  2052		struct mmc_host *mmc = platform_get_drvdata(pdev);
  2053	
> 2054		return mmci_runtime_resume(mmc);
  2055	}
  2056	
  2057	static int mmci_runtime_suspend_pltfm(struct device *dev)
  2058	{
  2059		struct platform_device *pdev = to_platform_device(dev);
  2060		struct mmc_host *mmc = platform_get_drvdata(pdev);
  2061	
> 2062		return mmci_runtime_suspend(mmc);
  2063	}
  2064	
  2065	MODULE_DEVICE_TABLE(of, mmci_pltfm_match);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 48659 bytes --]