[PATCH 00/14] Enable SD/MMC on JZ4780 SoCs

[PATCH 00/14] Enable SD/MMC on JZ4780 SoCs

[Ezequiel Garcia]

This patchset adds support for SD/MMC on JZ4780 based
platforms, such as the MIPS Creator CI20 board.

Most of the work has been done by Alex, Paul and Zubair,
while I've only prepared the upstream submission, cleaned
some patches, and written some commit logs where needed.

All praises should go to them, all rants to me.

The series is based on v4.16-rc4.

Alex Smith (3):
  mmc: jz4740: Set clock rate to mmc->f_max rather than JZ_MMC_CLK_RATE
  mmc: jz4740: Add support for the JZ4780
  mmc: jz4740: Fix race condition in IRQ mask update

Ezequiel Garcia (9):
  mmc: jz4780: Order headers alphabetically
  mmc: jz4740: Use dev_get_platdata
  mmc: jz4740: Introduce devicetree probe
  mmc: dt-bindings: add MMC support to JZ4740 SoC
  mmc: jz4740: Use dma_request_chan()
  MIPS: dts: jz4780: Add DMA controller node to the devicetree
  MIPS: dts: jz4780: Add MMC controller node to the devicetree
  MIPS: dts: ci20: Enable DMA and MMC in the devicetree
  MIPS: configs: ci20: Enable DMA and MMC support

Paul Cercueil (1):
  mmc: jz4740: Fix error exit path in driver's probe

Zubair Lutfullah Kakakhel (1):
  mmc: jz4740: Reset the device requesting the interrupt

 Documentation/devicetree/bindings/mmc/jz4740.txt |  38 ++++
 arch/mips/boot/dts/ingenic/ci20.dts              |  38 ++++
 arch/mips/boot/dts/ingenic/jz4780.dtsi           |  54 ++++++
 arch/mips/configs/ci20_defconfig                 |   3 +
 drivers/mmc/host/Kconfig                         |   2 +-
 drivers/mmc/host/jz4740_mmc.c                    | 232 ++++++++++++++++-------
 include/dt-bindings/dma/jz4780-dma.h             |  49 +++++
 7 files changed, 349 insertions(+), 67 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mmc/jz4740.txt
 create mode 100644 include/dt-bindings/dma/jz4780-dma.h

-- 
2.16.2

[PATCH 01/14] mmc: jz4780: Order headers alphabetically

[Ezequiel Garcia]

Just a minor cleanup to order the headers alphabetically.
This helps prevent merge conflicts.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/mmc/host/jz4740_mmc.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
index 712e08d9a45e..8da2bfe82781 100644
--- a/drivers/mmc/host/jz4740_mmc.c
+++ b/drivers/mmc/host/jz4740_mmc.c
@@ -13,24 +13,24 @@
  *
  */
 
-#include <linux/mmc/host.h>
-#include <linux/mmc/slot-gpio.h>
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
 #include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/irq.h>
-#include <linux/interrupt.h>
+#include <linux/mmc/host.h>
+#include <linux/mmc/slot-gpio.h>
 #include <linux/module.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
-#include <linux/delay.h>
 #include <linux/scatterlist.h>
-#include <linux/clk.h>
 
-#include <linux/bitops.h>
-#include <linux/gpio.h>
 #include <asm/cacheflush.h>
-#include <linux/dma-mapping.h>
-#include <linux/dmaengine.h>
 
 #include <asm/mach-jz4740/dma.h>
 #include <asm/mach-jz4740/jz4740_mmc.h>
-- 
2.16.2

[PATCH 02/14] mmc: jz4740: Use dev_get_platdata

[Ezequiel Garcia]

Instead of accessing the platform data pointer directly,
use the dev_get_platdata() helper.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/mmc/host/jz4740_mmc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
index 8da2bfe82781..5a85a3017711 100644
--- a/drivers/mmc/host/jz4740_mmc.c
+++ b/drivers/mmc/host/jz4740_mmc.c
@@ -926,7 +926,7 @@ static int jz4740_mmc_request_gpio(struct device *dev, int gpio,
 static int jz4740_mmc_request_gpios(struct mmc_host *mmc,
 	struct platform_device *pdev)
 {
-	struct jz4740_mmc_platform_data *pdata = pdev->dev.platform_data;
+	struct jz4740_mmc_platform_data *pdata = dev_get_platdata(&pdev->dev);
 	int ret = 0;
 
 	if (!pdata)
@@ -955,7 +955,7 @@ static int jz4740_mmc_request_gpios(struct mmc_host *mmc,
 
 static void jz4740_mmc_free_gpios(struct platform_device *pdev)
 {
-	struct jz4740_mmc_platform_data *pdata = pdev->dev.platform_data;
+	struct jz4740_mmc_platform_data *pdata = dev_get_platdata(&pdev->dev);
 
 	if (!pdata)
 		return;
@@ -971,7 +971,7 @@ static int jz4740_mmc_probe(struct platform_device* pdev)
 	struct jz4740_mmc_host *host;
 	struct jz4740_mmc_platform_data *pdata;
 
-	pdata = pdev->dev.platform_data;
+	pdata = dev_get_platdata(&pdev->dev);
 
 	mmc = mmc_alloc_host(sizeof(struct jz4740_mmc_host), &pdev->dev);
 	if (!mmc) {
-- 
2.16.2

[PATCH 03/14] mmc: jz4740: Fix error exit path in driver's probe

[Ezequiel Garcia]

From: Paul Cercueil <paul@crapouillou.net>

Currently, if jz4740_mmc_request_gpios() fails, the driver
tries to release DMA resources. This is wrong because DMA
is requested at a later stage.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
[Ezequiel: cleanup commit message]
Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/mmc/host/jz4740_mmc.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
index 5a85a3017711..636741ac9031 100644
--- a/drivers/mmc/host/jz4740_mmc.c
+++ b/drivers/mmc/host/jz4740_mmc.c
@@ -1006,7 +1006,7 @@ static int jz4740_mmc_probe(struct platform_device* pdev)
 
 	ret = jz4740_mmc_request_gpios(mmc, pdev);
 	if (ret)
-		goto err_release_dma;
+		goto err_free_host;
 
 	mmc->ops = &jz4740_mmc_ops;
 	mmc->f_min = JZ_MMC_CLK_RATE / 128;
@@ -1038,16 +1038,17 @@ static int jz4740_mmc_probe(struct platform_device* pdev)
 	jz4740_mmc_clock_disable(host);
 	timer_setup(&host->timeout_timer, jz4740_mmc_timeout, 0);
 
-	host->use_dma = true;
-	if (host->use_dma && jz4740_mmc_acquire_dma_channels(host) != 0)
-		host->use_dma = false;
+	ret = jz4740_mmc_acquire_dma_channels(host);
+	if (ret == -EPROBE_DEFER)
+		goto err_free_irq;
+	host->use_dma = !ret;
 
 	platform_set_drvdata(pdev, host);
 	ret = mmc_add_host(mmc);
 
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to add mmc host: %d\n", ret);
-		goto err_free_irq;
+		goto err_release_dma;
 	}
 	dev_info(&pdev->dev, "JZ SD/MMC card driver registered\n");
 
@@ -1057,13 +1058,13 @@ static int jz4740_mmc_probe(struct platform_device* pdev)
 
 	return 0;
 
+err_release_dma:
+	if (host->use_dma)
+		jz4740_mmc_release_dma_channels(host);
 err_free_irq:
 	free_irq(host->irq, host);
 err_free_gpios:
 	jz4740_mmc_free_gpios(pdev);
-err_release_dma:
-	if (host->use_dma)
-		jz4740_mmc_release_dma_channels(host);
 err_free_host:
 	mmc_free_host(mmc);
 
-- 
2.16.2

[PATCH 04/14] mmc: jz4740: Reset the device requesting the interrupt

[Ezequiel Garcia]

From: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>

In case a bootloader leaves the device in a bad state,
requesting the interrupt before resetting results in a bad
interrupt loop.

Signed-off-by: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
[Ezequiel: cleanup commit description]
Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/mmc/host/jz4740_mmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
index 636741ac9031..0a8edd5b4052 100644
--- a/drivers/mmc/host/jz4740_mmc.c
+++ b/drivers/mmc/host/jz4740_mmc.c
@@ -1027,6 +1027,8 @@ static int jz4740_mmc_probe(struct platform_device* pdev)
 	spin_lock_init(&host->lock);
 	host->irq_mask = 0xffff;
 
+	jz4740_mmc_reset(host);
+
 	ret = request_threaded_irq(host->irq, jz_mmc_irq, jz_mmc_irq_worker, 0,
 			dev_name(&pdev->dev), host);
 	if (ret) {
@@ -1034,7 +1036,6 @@ static int jz4740_mmc_probe(struct platform_device* pdev)
 		goto err_free_gpios;
 	}
 
-	jz4740_mmc_reset(host);
 	jz4740_mmc_clock_disable(host);
 	timer_setup(&host->timeout_timer, jz4740_mmc_timeout, 0);
 
-- 
2.16.2

[PATCH 05/14] mmc: jz4740: Introduce devicetree probe

[Ezequiel Garcia]

This commit introduces support to probe the device
via devicetree, which will be used to support other
SoCs such as the JZ4780.

Based on commits from the CI20 repo, by Paul Cercueil
and Alex Smith.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/mmc/host/jz4740_mmc.c | 51 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 41 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
index 0a8edd5b4052..c54861037481 100644
--- a/drivers/mmc/host/jz4740_mmc.c
+++ b/drivers/mmc/host/jz4740_mmc.c
@@ -26,6 +26,7 @@
 #include <linux/mmc/host.h>
 #include <linux/mmc/slot-gpio.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/scatterlist.h>
@@ -107,6 +108,10 @@
 
 #define JZ_MMC_CLK_RATE 24000000
 
+enum jz4740_mmc_version {
+	JZ_MMC_JZ4740,
+};
+
 enum jz4740_mmc_state {
 	JZ4740_MMC_STATE_READ_RESPONSE,
 	JZ4740_MMC_STATE_TRANSFER_DATA,
@@ -125,6 +130,8 @@ struct jz4740_mmc_host {
 	struct jz4740_mmc_platform_data *pdata;
 	struct clk *clk;
 
+	enum jz4740_mmc_version version;
+
 	int irq;
 	int card_detect_irq;
 
@@ -857,7 +864,7 @@ static void jz4740_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	switch (ios->power_mode) {
 	case MMC_POWER_UP:
 		jz4740_mmc_reset(host);
-		if (gpio_is_valid(host->pdata->gpio_power))
+		if (host->pdata && gpio_is_valid(host->pdata->gpio_power))
 			gpio_set_value(host->pdata->gpio_power,
 					!host->pdata->power_active_low);
 		host->cmdat |= JZ_MMC_CMDAT_INIT;
@@ -866,7 +873,7 @@ static void jz4740_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	case MMC_POWER_ON:
 		break;
 	default:
-		if (gpio_is_valid(host->pdata->gpio_power))
+		if (host->pdata && gpio_is_valid(host->pdata->gpio_power))
 			gpio_set_value(host->pdata->gpio_power,
 					host->pdata->power_active_low);
 		clk_disable_unprepare(host->clk);
@@ -964,11 +971,18 @@ static void jz4740_mmc_free_gpios(struct platform_device *pdev)
 		gpio_free(pdata->gpio_power);
 }
 
+static const struct of_device_id jz4740_mmc_of_match[] = {
+	{ .compatible = "ingenic,jz4740-mmc", .data = (void *) JZ_MMC_JZ4740 },
+	{},
+};
+MODULE_DEVICE_TABLE(of, jz4740_mmc_of_match);
+
 static int jz4740_mmc_probe(struct platform_device* pdev)
 {
 	int ret;
 	struct mmc_host *mmc;
 	struct jz4740_mmc_host *host;
+	const struct of_device_id *match;
 	struct jz4740_mmc_platform_data *pdata;
 
 	pdata = dev_get_platdata(&pdev->dev);
@@ -982,6 +996,27 @@ static int jz4740_mmc_probe(struct platform_device* pdev)
 	host = mmc_priv(mmc);
 	host->pdata = pdata;
 
+	match = of_match_device(jz4740_mmc_of_match, &pdev->dev);
+	if (match) {
+		host->version = (enum jz4740_mmc_version)match->data;
+		ret = mmc_of_parse(mmc);
+		if (ret) {
+			if (ret != -EPROBE_DEFER)
+				dev_err(&pdev->dev,
+					"could not parse of data: %d\n", ret);
+			goto err_free_host;
+		}
+	} else {
+		/* JZ4740 should be the only one using legacy probe */
+		host->version = JZ_MMC_JZ4740;
+		mmc->caps |= MMC_CAP_SDIO_IRQ;
+		if (!(pdata && pdata->data_1bit))
+			mmc->caps |= MMC_CAP_4_BIT_DATA;
+		ret = jz4740_mmc_request_gpios(mmc, pdev);
+		if (ret)
+			goto err_free_host;
+	}
+
 	host->irq = platform_get_irq(pdev, 0);
 	if (host->irq < 0) {
 		ret = host->irq;
@@ -1004,16 +1039,11 @@ static int jz4740_mmc_probe(struct platform_device* pdev)
 		goto err_free_host;
 	}
 
-	ret = jz4740_mmc_request_gpios(mmc, pdev);
-	if (ret)
-		goto err_free_host;
-
 	mmc->ops = &jz4740_mmc_ops;
-	mmc->f_min = JZ_MMC_CLK_RATE / 128;
-	mmc->f_max = JZ_MMC_CLK_RATE;
+	if (!mmc->f_max)
+		mmc->f_max = JZ_MMC_CLK_RATE;
+	mmc->f_min = mmc->f_max / 128;
 	mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
-	mmc->caps = (pdata && pdata->data_1bit) ? 0 : MMC_CAP_4_BIT_DATA;
-	mmc->caps |= MMC_CAP_SDIO_IRQ;
 
 	mmc->max_blk_size = (1 << 10) - 1;
 	mmc->max_blk_count = (1 << 15) - 1;
@@ -1118,6 +1148,7 @@ static struct platform_driver jz4740_mmc_driver = {
 	.remove = jz4740_mmc_remove,
 	.driver = {
 		.name = "jz4740-mmc",
+		.of_match_table = of_match_ptr(jz4740_mmc_of_match),
 		.pm = JZ4740_MMC_PM_OPS,
 	},
 };
-- 
2.16.2

[PATCH 06/14] mmc: dt-bindings: add MMC support to JZ4740 SoC

[Ezequiel Garcia]

Add the devicetree binding for JZ4740/JZ4780 SoC.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 Documentation/devicetree/bindings/mmc/jz4740.txt | 38 ++++++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/jz4740.txt

diff --git a/Documentation/devicetree/bindings/mmc/jz4740.txt b/Documentation/devicetree/bindings/mmc/jz4740.txt
new file mode 100644
index 000000000000..7cd8c432d7c8
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/jz4740.txt
@@ -0,0 +1,38 @@
+* Ingenic JZ47xx MMC controllers
+
+This file documents the device tree properties used for the MMC controller in
+Ingenic JZ4740/JZ4780 SoCs. These are in addition to the core MMC properties
+described in mmc.txt.
+
+Required properties:
+- compatible: Should be one of the following:
+  - "ingenic,jz4740-mmc" for the JZ4740
+  - "ingenic,jz4780-mmc" for the JZ4780
+- reg: Should contain the MMC controller registers location and length.
+- interrupts: Should contain the interrupt specifier of the MMC controller.
+- clocks: Clock for the MMC controller.
+
+Optional properties:
+- dmas: List of DMA specifiers with the controller specific format
+        as described in the generic DMA client binding. A tx and rx
+        specifier is required.
+- dma-names: RX and TX  DMA request names.
+        Should be "rx" and "tx", in that order.
+
+For additional details on DMA client bindings see ../dma/dma.txt.
+
+Example:
+
+mmc0: mmc@13450000 {
+	compatible = "ingenic,jz4780-mmc";
+	reg = <0x13450000 0x1000>;
+
+	interrupt-parent = <&intc>;
+	interrupts = <37>;
+
+	clocks = <&cgu JZ4780_CLK_MSC0>;
+	clock-names = "mmc";
+
+	dmas = <&dma JZ4780_DMA_MSC0_RX 0xffffffff>, <&dma JZ4780_DMA_MSC0_TX 0xffffffff>;
+	dma-names = "rx", "tx";
+};
-- 
2.16.2

[PATCH 07/14] mmc: jz4740: Set clock rate to mmc->f_max rather than JZ_MMC_CLK_RATE

[Ezequiel Garcia]

From: Alex Smith <alex.smith@imgtec.com>

The maximum clock rate can be overridden by DT. The clock rate should
be set to the DT-specified value rather than the constant JZ_MMC_CLK_RATE
when this is done. If the maximum clock rate is not set by DT then
mmc->f_max will be set to JZ_MMC_CLK_RATE.

Signed-off-by: Alex Smith <alex.smith@imgtec.com>
---
 drivers/mmc/host/jz4740_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
index c54861037481..7d4dcce76cd8 100644
--- a/drivers/mmc/host/jz4740_mmc.c
+++ b/drivers/mmc/host/jz4740_mmc.c
@@ -825,7 +825,7 @@ static int jz4740_mmc_set_clock_rate(struct jz4740_mmc_host *host, int rate)
 	int real_rate;
 
 	jz4740_mmc_clock_disable(host);
-	clk_set_rate(host->clk, JZ_MMC_CLK_RATE);
+	clk_set_rate(host->clk, host->mmc->f_max);
 
 	real_rate = clk_get_rate(host->clk);
 
-- 
2.16.2

[PATCH 08/14] mmc: jz4740: Add support for the JZ4780

[Ezequiel Garcia]

From: Alex Smith <alex.smith@imgtec.com>

Add support for the JZ4780 MMC controller to the jz47xx_mmc driver. There
are a few minor differences from the 4740 to the 4780 that need to be
handled, but otherwise the controllers behave the same. The IREG and IMASK
registers are expanded to 32 bits. Additionally, some error conditions are
now reported in both STATUS and IREG. Writing IREG before reading STATUS
causes the bits in STATUS to be cleared, so STATUS must be read first to
ensure we see and report error conditions correctly.

Signed-off-by: Alex Smith <alex.smith@imgtec.com>
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
[Ezequiel: rebase and introduce register accessors]
Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/mmc/host/Kconfig      |   2 +-
 drivers/mmc/host/jz4740_mmc.c | 111 ++++++++++++++++++++++++++++++++++--------
 2 files changed, 93 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 620c2d90a646..7dd5169a2dfb 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -767,7 +767,7 @@ config MMC_SH_MMCIF
 
 config MMC_JZ4740
 	tristate "JZ4740 SD/Multimedia Card Interface support"
-	depends on MACH_JZ4740
+	depends on MACH_JZ4740 || MACH_JZ4780
 	help
 	  This selects support for the SD/MMC controller on Ingenic JZ4740
 	  SoCs.
diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
index 7d4dcce76cd8..bb1b9114ef53 100644
--- a/drivers/mmc/host/jz4740_mmc.c
+++ b/drivers/mmc/host/jz4740_mmc.c
@@ -1,5 +1,7 @@
 /*
  *  Copyright (C) 2009-2010, Lars-Peter Clausen <lars@metafoo.de>
+ *  Copyright (C) 2013, Imagination Technologies
+ *
  *  JZ4740 SD/MMC controller driver
  *
  *  This program is free software; you can redistribute  it and/or modify it
@@ -52,6 +54,7 @@
 #define JZ_REG_MMC_RESP_FIFO	0x34
 #define JZ_REG_MMC_RXFIFO	0x38
 #define JZ_REG_MMC_TXFIFO	0x3C
+#define JZ_REG_MMC_DMAC		0x44
 
 #define JZ_MMC_STRPCL_EXIT_MULTIPLE BIT(7)
 #define JZ_MMC_STRPCL_EXIT_TRANSFER BIT(6)
@@ -105,11 +108,15 @@
 #define JZ_MMC_IRQ_PRG_DONE BIT(1)
 #define JZ_MMC_IRQ_DATA_TRAN_DONE BIT(0)
 
+#define JZ_MMC_DMAC_DMA_SEL BIT(1)
+#define JZ_MMC_DMAC_DMA_EN BIT(0)
 
 #define JZ_MMC_CLK_RATE 24000000
 
 enum jz4740_mmc_version {
 	JZ_MMC_JZ4740,
+	JZ_MMC_JZ4750,
+	JZ_MMC_JZ4780,
 };
 
 enum jz4740_mmc_state {
@@ -144,7 +151,7 @@ struct jz4740_mmc_host {
 
 	uint32_t cmdat;
 
-	uint16_t irq_mask;
+	uint32_t irq_mask;
 
 	spinlock_t lock;
 
@@ -164,8 +171,46 @@ struct jz4740_mmc_host {
  * trigger is when data words in MSC_TXFIFO is < 8.
  */
 #define JZ4740_MMC_FIFO_HALF_SIZE 8
+
+	void (*write_irq_mask)(struct jz4740_mmc_host *host, uint32_t val);
+	void (*write_irq_reg)(struct jz4740_mmc_host *host, uint32_t val);
+	uint32_t (*read_irq_reg)(struct jz4740_mmc_host *host);
 };
 
+static void jz4750_mmc_write_irq_mask(struct jz4740_mmc_host *host,
+				      uint32_t val)
+{
+	return writel(val, host->base + JZ_REG_MMC_IMASK);
+}
+
+static void jz4740_mmc_write_irq_mask(struct jz4740_mmc_host *host,
+				      uint32_t val)
+{
+	return writew(val, host->base + JZ_REG_MMC_IMASK);
+}
+
+static void jz4740_mmc_write_irq_reg(struct jz4740_mmc_host *host,
+				     uint32_t val)
+{
+	return writew(val, host->base + JZ_REG_MMC_IREG);
+}
+
+static uint32_t jz4740_mmc_read_irq_reg(struct jz4740_mmc_host *host)
+{
+	return readw(host->base + JZ_REG_MMC_IREG);
+}
+
+static void jz4780_mmc_write_irq_reg(struct jz4740_mmc_host *host, uint32_t val)
+{
+	return writel(val, host->base + JZ_REG_MMC_IREG);
+}
+
+/* In the 4780 onwards, IREG is expanded to 32 bits. */
+static uint32_t jz4780_mmc_read_irq_reg(struct jz4740_mmc_host *host)
+{
+	return readl(host->base + JZ_REG_MMC_IREG);
+}
+
 /*----------------------------------------------------------------------------*/
 /* DMA infrastructure */
 
@@ -371,7 +416,7 @@ static void jz4740_mmc_set_irq_enabled(struct jz4740_mmc_host *host,
 		host->irq_mask |= irq;
 	spin_unlock_irqrestore(&host->lock, flags);
 
-	writew(host->irq_mask, host->base + JZ_REG_MMC_IMASK);
+	host->write_irq_mask(host, host->irq_mask);
 }
 
 static void jz4740_mmc_clock_enable(struct jz4740_mmc_host *host,
@@ -422,10 +467,10 @@ static unsigned int jz4740_mmc_poll_irq(struct jz4740_mmc_host *host,
 	unsigned int irq)
 {
 	unsigned int timeout = 0x800;
-	uint16_t status;
+	uint32_t status;
 
 	do {
-		status = readw(host->base + JZ_REG_MMC_IREG);
+		status = host->read_irq_reg(host);
 	} while (!(status & irq) && --timeout);
 
 	if (timeout == 0) {
@@ -525,7 +570,7 @@ static bool jz4740_mmc_read_data(struct jz4740_mmc_host *host,
 	void __iomem *fifo_addr = host->base + JZ_REG_MMC_RXFIFO;
 	uint32_t *buf;
 	uint32_t d;
-	uint16_t status;
+	uint32_t status;
 	size_t i, j;
 	unsigned int timeout;
 
@@ -661,8 +706,25 @@ static void jz4740_mmc_send_command(struct jz4740_mmc_host *host,
 		cmdat |= JZ_MMC_CMDAT_DATA_EN;
 		if (cmd->data->flags & MMC_DATA_WRITE)
 			cmdat |= JZ_MMC_CMDAT_WRITE;
-		if (host->use_dma)
-			cmdat |= JZ_MMC_CMDAT_DMA_EN;
+		if (host->use_dma) {
+			/*
+			 * The 4780's MMC controller has integrated DMA ability
+			 * in addition to being able to use the external DMA
+			 * controller. It moves DMA control bits to a separate
+			 * register. The DMA_SEL bit chooses the external
+			 * controller over the integrated one. Earlier SoCs
+			 * can only use the external controller, and have a
+			 * single DMA enable bit in CMDAT.
+			 */
+			if (host->version >= JZ_MMC_JZ4780) {
+				writel(JZ_MMC_DMAC_DMA_EN | JZ_MMC_DMAC_DMA_SEL,
+				       host->base + JZ_REG_MMC_DMAC);
+			} else {
+				cmdat |= JZ_MMC_CMDAT_DMA_EN;
+			}
+		} else if (host->version >= JZ_MMC_JZ4780) {
+			writel(0, host->base + JZ_REG_MMC_DMAC);
+		}
 
 		writew(cmd->data->blksz, host->base + JZ_REG_MMC_BLKLEN);
 		writew(cmd->data->blocks, host->base + JZ_REG_MMC_NOB);
@@ -743,7 +805,7 @@ static irqreturn_t jz_mmc_irq_worker(int irq, void *devid)
 			host->state = JZ4740_MMC_STATE_SEND_STOP;
 			break;
 		}
-		writew(JZ_MMC_IRQ_DATA_TRAN_DONE, host->base + JZ_REG_MMC_IREG);
+		host->write_irq_reg(host, JZ_MMC_IRQ_DATA_TRAN_DONE);
 
 	case JZ4740_MMC_STATE_SEND_STOP:
 		if (!req->stop)
@@ -773,9 +835,10 @@ static irqreturn_t jz_mmc_irq(int irq, void *devid)
 {
 	struct jz4740_mmc_host *host = devid;
 	struct mmc_command *cmd = host->cmd;
-	uint16_t irq_reg, status, tmp;
+	uint32_t irq_reg, status, tmp;
 
-	irq_reg = readw(host->base + JZ_REG_MMC_IREG);
+	status = readl(host->base + JZ_REG_MMC_STATUS);
+	irq_reg = host->read_irq_reg(host);
 
 	tmp = irq_reg;
 	irq_reg &= ~host->irq_mask;
@@ -784,10 +847,10 @@ static irqreturn_t jz_mmc_irq(int irq, void *devid)
 		JZ_MMC_IRQ_PRG_DONE | JZ_MMC_IRQ_DATA_TRAN_DONE);
 
 	if (tmp != irq_reg)
-		writew(tmp & ~irq_reg, host->base + JZ_REG_MMC_IREG);
+		host->write_irq_reg(host, tmp & ~irq_reg);
 
 	if (irq_reg & JZ_MMC_IRQ_SDIO) {
-		writew(JZ_MMC_IRQ_SDIO, host->base + JZ_REG_MMC_IREG);
+		host->write_irq_reg(host, JZ_MMC_IRQ_SDIO);
 		mmc_signal_sdio_irq(host->mmc);
 		irq_reg &= ~JZ_MMC_IRQ_SDIO;
 	}
@@ -796,8 +859,6 @@ static irqreturn_t jz_mmc_irq(int irq, void *devid)
 		if (test_and_clear_bit(0, &host->waiting)) {
 			del_timer(&host->timeout_timer);
 
-			status = readl(host->base + JZ_REG_MMC_STATUS);
-
 			if (status & JZ_MMC_STATUS_TIMEOUT_RES) {
 					cmd->error = -ETIMEDOUT;
 			} else if (status & JZ_MMC_STATUS_CRC_RES_ERR) {
@@ -810,7 +871,7 @@ static irqreturn_t jz_mmc_irq(int irq, void *devid)
 			}
 
 			jz4740_mmc_set_irq_enabled(host, irq_reg, false);
-			writew(irq_reg, host->base + JZ_REG_MMC_IREG);
+			host->write_irq_reg(host, irq_reg);
 
 			return IRQ_WAKE_THREAD;
 		}
@@ -844,9 +905,7 @@ static void jz4740_mmc_request(struct mmc_host *mmc, struct mmc_request *req)
 
 	host->req = req;
 
-	writew(0xffff, host->base + JZ_REG_MMC_IREG);
-
-	writew(JZ_MMC_IRQ_END_CMD_RES, host->base + JZ_REG_MMC_IREG);
+	host->write_irq_reg(host, ~0);
 	jz4740_mmc_set_irq_enabled(host, JZ_MMC_IRQ_END_CMD_RES, true);
 
 	host->state = JZ4740_MMC_STATE_READ_RESPONSE;
@@ -973,6 +1032,7 @@ static void jz4740_mmc_free_gpios(struct platform_device *pdev)
 
 static const struct of_device_id jz4740_mmc_of_match[] = {
 	{ .compatible = "ingenic,jz4740-mmc", .data = (void *) JZ_MMC_JZ4740 },
+	{ .compatible = "ingenic,jz4780-mmc", .data = (void *) JZ_MMC_JZ4780 },
 	{},
 };
 MODULE_DEVICE_TABLE(of, jz4740_mmc_of_match);
@@ -1017,6 +1077,19 @@ static int jz4740_mmc_probe(struct platform_device* pdev)
 			goto err_free_host;
 	}
 
+	if (host->version >= JZ_MMC_JZ4780) {
+		host->write_irq_reg = jz4780_mmc_write_irq_reg;
+		host->read_irq_reg = jz4780_mmc_read_irq_reg;
+	} else {
+		host->write_irq_reg = jz4740_mmc_write_irq_reg;
+		host->read_irq_reg = jz4740_mmc_read_irq_reg;
+	}
+
+	if (host->version >= JZ_MMC_JZ4750)
+		host->write_irq_mask = jz4750_mmc_write_irq_mask;
+	else
+		host->write_irq_mask = jz4740_mmc_write_irq_mask;
+
 	host->irq = platform_get_irq(pdev, 0);
 	if (host->irq < 0) {
 		ret = host->irq;
@@ -1055,7 +1128,7 @@ static int jz4740_mmc_probe(struct platform_device* pdev)
 	host->mmc = mmc;
 	host->pdev = pdev;
 	spin_lock_init(&host->lock);
-	host->irq_mask = 0xffff;
+	host->irq_mask = ~0;
 
 	jz4740_mmc_reset(host);
 
-- 
2.16.2

Re: [PATCH 08/14] mmc: jz4740: Add support for the JZ4780,

[Paul Cercueil]

Hi,

Le 2018-03-09 16:12, Ezequiel Garcia a écrit :
> From: Alex Smith <alex.smith@imgtec.com>
> 
> Add support for the JZ4780 MMC controller to the jz47xx_mmc driver. 
> There
> are a few minor differences from the 4740 to the 4780 that need to be
> handled, but otherwise the controllers behave the same. The IREG and 
> IMASK
> registers are expanded to 32 bits. Additionally, some error conditions 
> are
> now reported in both STATUS and IREG. Writing IREG before reading 
> STATUS
> causes the bits in STATUS to be cleared, so STATUS must be read first 
> to
> ensure we see and report error conditions correctly.
> 
> Signed-off-by: Alex Smith <alex.smith@imgtec.com>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> [Ezequiel: rebase and introduce register accessors]
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
>  drivers/mmc/host/Kconfig      |   2 +-
>  drivers/mmc/host/jz4740_mmc.c | 111 
> ++++++++++++++++++++++++++++++++++--------
>  2 files changed, 93 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 620c2d90a646..7dd5169a2dfb 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -767,7 +767,7 @@ config MMC_SH_MMCIF
> 
>  config MMC_JZ4740
>  	tristate "JZ4740 SD/Multimedia Card Interface support"
> -	depends on MACH_JZ4740
> +	depends on MACH_JZ4740 || MACH_JZ4780
>  	help
>  	  This selects support for the SD/MMC controller on Ingenic JZ4740
>  	  SoCs.
> diff --git a/drivers/mmc/host/jz4740_mmc.c 
> b/drivers/mmc/host/jz4740_mmc.c
> index 7d4dcce76cd8..bb1b9114ef53 100644
> --- a/drivers/mmc/host/jz4740_mmc.c
> +++ b/drivers/mmc/host/jz4740_mmc.c
> @@ -1,5 +1,7 @@
>  /*
>   *  Copyright (C) 2009-2010, Lars-Peter Clausen <lars@metafoo.de>
> + *  Copyright (C) 2013, Imagination Technologies
> + *
>   *  JZ4740 SD/MMC controller driver
>   *
>   *  This program is free software; you can redistribute  it and/or 
> modify it
> @@ -52,6 +54,7 @@
>  #define JZ_REG_MMC_RESP_FIFO	0x34
>  #define JZ_REG_MMC_RXFIFO	0x38
>  #define JZ_REG_MMC_TXFIFO	0x3C
> +#define JZ_REG_MMC_DMAC		0x44
> 
>  #define JZ_MMC_STRPCL_EXIT_MULTIPLE BIT(7)
>  #define JZ_MMC_STRPCL_EXIT_TRANSFER BIT(6)
> @@ -105,11 +108,15 @@
>  #define JZ_MMC_IRQ_PRG_DONE BIT(1)
>  #define JZ_MMC_IRQ_DATA_TRAN_DONE BIT(0)
> 
> +#define JZ_MMC_DMAC_DMA_SEL BIT(1)
> +#define JZ_MMC_DMAC_DMA_EN BIT(0)
> 
>  #define JZ_MMC_CLK_RATE 24000000
> 
>  enum jz4740_mmc_version {
>  	JZ_MMC_JZ4740,
> +	JZ_MMC_JZ4750,
> +	JZ_MMC_JZ4780,
>  };
> 
>  enum jz4740_mmc_state {
> @@ -144,7 +151,7 @@ struct jz4740_mmc_host {
> 
>  	uint32_t cmdat;
> 
> -	uint16_t irq_mask;
> +	uint32_t irq_mask;
> 
>  	spinlock_t lock;
> 
> @@ -164,8 +171,46 @@ struct jz4740_mmc_host {
>   * trigger is when data words in MSC_TXFIFO is < 8.
>   */
>  #define JZ4740_MMC_FIFO_HALF_SIZE 8
> +
> +	void (*write_irq_mask)(struct jz4740_mmc_host *host, uint32_t val);
> +	void (*write_irq_reg)(struct jz4740_mmc_host *host, uint32_t val);
> +	uint32_t (*read_irq_reg)(struct jz4740_mmc_host *host);
>  };

I'm not 100% fan about the callback idea, the original commit
(https://github.com/gcwnow/linux/commit/62472091) doesn't use them and 
is
30 lines shorter.

I'm not terribly against either, so if nobody else bug on that, feel 
free
to ignore my comment.

> +static void jz4750_mmc_write_irq_mask(struct jz4740_mmc_host *host,
> +				      uint32_t val)
> +{
> +	return writel(val, host->base + JZ_REG_MMC_IMASK);
> +}
> +
> +static void jz4740_mmc_write_irq_mask(struct jz4740_mmc_host *host,
> +				      uint32_t val)
> +{
> +	return writew(val, host->base + JZ_REG_MMC_IMASK);
> +}
> +
> +static void jz4740_mmc_write_irq_reg(struct jz4740_mmc_host *host,
> +				     uint32_t val)
> +{
> +	return writew(val, host->base + JZ_REG_MMC_IREG);
> +}
> +
> +static uint32_t jz4740_mmc_read_irq_reg(struct jz4740_mmc_host *host)
> +{
> +	return readw(host->base + JZ_REG_MMC_IREG);
> +}
> +
> +static void jz4780_mmc_write_irq_reg(struct jz4740_mmc_host *host,
> uint32_t val)
> +{
> +	return writel(val, host->base + JZ_REG_MMC_IREG);
> +}
> +
> +/* In the 4780 onwards, IREG is expanded to 32 bits. */
> +static uint32_t jz4780_mmc_read_irq_reg(struct jz4740_mmc_host *host)
> +{
> +	return readl(host->base + JZ_REG_MMC_IREG);
> +}
> +
> 
> /*----------------------------------------------------------------------------*/
>  /* DMA infrastructure */
> 
> @@ -371,7 +416,7 @@ static void jz4740_mmc_set_irq_enabled(struct
> jz4740_mmc_host *host,
>  		host->irq_mask |= irq;
>  	spin_unlock_irqrestore(&host->lock, flags);
> 
> -	writew(host->irq_mask, host->base + JZ_REG_MMC_IMASK);
> +	host->write_irq_mask(host, host->irq_mask);
>  }
> 
>  static void jz4740_mmc_clock_enable(struct jz4740_mmc_host *host,
> @@ -422,10 +467,10 @@ static unsigned int jz4740_mmc_poll_irq(struct
> jz4740_mmc_host *host,
>  	unsigned int irq)
>  {
>  	unsigned int timeout = 0x800;
> -	uint16_t status;
> +	uint32_t status;
> 
>  	do {
> -		status = readw(host->base + JZ_REG_MMC_IREG);
> +		status = host->read_irq_reg(host);
>  	} while (!(status & irq) && --timeout);
> 
>  	if (timeout == 0) {
> @@ -525,7 +570,7 @@ static bool jz4740_mmc_read_data(struct
> jz4740_mmc_host *host,
>  	void __iomem *fifo_addr = host->base + JZ_REG_MMC_RXFIFO;
>  	uint32_t *buf;
>  	uint32_t d;
> -	uint16_t status;
> +	uint32_t status;
>  	size_t i, j;
>  	unsigned int timeout;
> 
> @@ -661,8 +706,25 @@ static void jz4740_mmc_send_command(struct
> jz4740_mmc_host *host,
>  		cmdat |= JZ_MMC_CMDAT_DATA_EN;
>  		if (cmd->data->flags & MMC_DATA_WRITE)
>  			cmdat |= JZ_MMC_CMDAT_WRITE;
> -		if (host->use_dma)
> -			cmdat |= JZ_MMC_CMDAT_DMA_EN;
> +		if (host->use_dma) {
> +			/*
> +			 * The 4780's MMC controller has integrated DMA ability
> +			 * in addition to being able to use the external DMA
> +			 * controller. It moves DMA control bits to a separate
> +			 * register. The DMA_SEL bit chooses the external
> +			 * controller over the integrated one. Earlier SoCs
> +			 * can only use the external controller, and have a
> +			 * single DMA enable bit in CMDAT.
> +			 */
> +			if (host->version >= JZ_MMC_JZ4780) {
> +				writel(JZ_MMC_DMAC_DMA_EN | JZ_MMC_DMAC_DMA_SEL,
> +				       host->base + JZ_REG_MMC_DMAC);
> +			} else {
> +				cmdat |= JZ_MMC_CMDAT_DMA_EN;
> +			}
> +		} else if (host->version >= JZ_MMC_JZ4780) {
> +			writel(0, host->base + JZ_REG_MMC_DMAC);
> +		}
> 
>  		writew(cmd->data->blksz, host->base + JZ_REG_MMC_BLKLEN);
>  		writew(cmd->data->blocks, host->base + JZ_REG_MMC_NOB);
> @@ -743,7 +805,7 @@ static irqreturn_t jz_mmc_irq_worker(int irq, void 
> *devid)
>  			host->state = JZ4740_MMC_STATE_SEND_STOP;
>  			break;
>  		}
> -		writew(JZ_MMC_IRQ_DATA_TRAN_DONE, host->base + JZ_REG_MMC_IREG);
> +		host->write_irq_reg(host, JZ_MMC_IRQ_DATA_TRAN_DONE);
> 
>  	case JZ4740_MMC_STATE_SEND_STOP:
>  		if (!req->stop)
> @@ -773,9 +835,10 @@ static irqreturn_t jz_mmc_irq(int irq, void 
> *devid)
>  {
>  	struct jz4740_mmc_host *host = devid;
>  	struct mmc_command *cmd = host->cmd;
> -	uint16_t irq_reg, status, tmp;
> +	uint32_t irq_reg, status, tmp;
> 
> -	irq_reg = readw(host->base + JZ_REG_MMC_IREG);
> +	status = readl(host->base + JZ_REG_MMC_STATUS);
> +	irq_reg = host->read_irq_reg(host);
> 
>  	tmp = irq_reg;
>  	irq_reg &= ~host->irq_mask;
> @@ -784,10 +847,10 @@ static irqreturn_t jz_mmc_irq(int irq, void 
> *devid)
>  		JZ_MMC_IRQ_PRG_DONE | JZ_MMC_IRQ_DATA_TRAN_DONE);
> 
>  	if (tmp != irq_reg)
> -		writew(tmp & ~irq_reg, host->base + JZ_REG_MMC_IREG);
> +		host->write_irq_reg(host, tmp & ~irq_reg);
> 
>  	if (irq_reg & JZ_MMC_IRQ_SDIO) {
> -		writew(JZ_MMC_IRQ_SDIO, host->base + JZ_REG_MMC_IREG);
> +		host->write_irq_reg(host, JZ_MMC_IRQ_SDIO);
>  		mmc_signal_sdio_irq(host->mmc);
>  		irq_reg &= ~JZ_MMC_IRQ_SDIO;
>  	}
> @@ -796,8 +859,6 @@ static irqreturn_t jz_mmc_irq(int irq, void *devid)
>  		if (test_and_clear_bit(0, &host->waiting)) {
>  			del_timer(&host->timeout_timer);
> 
> -			status = readl(host->base + JZ_REG_MMC_STATUS);
> -
>  			if (status & JZ_MMC_STATUS_TIMEOUT_RES) {
>  					cmd->error = -ETIMEDOUT;
>  			} else if (status & JZ_MMC_STATUS_CRC_RES_ERR) {
> @@ -810,7 +871,7 @@ static irqreturn_t jz_mmc_irq(int irq, void *devid)
>  			}
> 
>  			jz4740_mmc_set_irq_enabled(host, irq_reg, false);
> -			writew(irq_reg, host->base + JZ_REG_MMC_IREG);
> +			host->write_irq_reg(host, irq_reg);
> 
>  			return IRQ_WAKE_THREAD;
>  		}
> @@ -844,9 +905,7 @@ static void jz4740_mmc_request(struct mmc_host
> *mmc, struct mmc_request *req)
> 
>  	host->req = req;
> 
> -	writew(0xffff, host->base + JZ_REG_MMC_IREG);
> -
> -	writew(JZ_MMC_IRQ_END_CMD_RES, host->base + JZ_REG_MMC_IREG);
> +	host->write_irq_reg(host, ~0);
>  	jz4740_mmc_set_irq_enabled(host, JZ_MMC_IRQ_END_CMD_RES, true);
> 
>  	host->state = JZ4740_MMC_STATE_READ_RESPONSE;
> @@ -973,6 +1032,7 @@ static void jz4740_mmc_free_gpios(struct
> platform_device *pdev)
> 
>  static const struct of_device_id jz4740_mmc_of_match[] = {
>  	{ .compatible = "ingenic,jz4740-mmc", .data = (void *) JZ_MMC_JZ4740 
> },
> +	{ .compatible = "ingenic,jz4780-mmc", .data = (void *) JZ_MMC_JZ4780 
> },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, jz4740_mmc_of_match);
> @@ -1017,6 +1077,19 @@ static int jz4740_mmc_probe(struct 
> platform_device* pdev)
>  			goto err_free_host;
>  	}
> 
> +	if (host->version >= JZ_MMC_JZ4780) {
> +		host->write_irq_reg = jz4780_mmc_write_irq_reg;
> +		host->read_irq_reg = jz4780_mmc_read_irq_reg;
> +	} else {
> +		host->write_irq_reg = jz4740_mmc_write_irq_reg;
> +		host->read_irq_reg = jz4740_mmc_read_irq_reg;
> +	}
> +
> +	if (host->version >= JZ_MMC_JZ4750)
> +		host->write_irq_mask = jz4750_mmc_write_irq_mask;
> +	else
> +		host->write_irq_mask = jz4740_mmc_write_irq_mask;
> +
>  	host->irq = platform_get_irq(pdev, 0);
>  	if (host->irq < 0) {
>  		ret = host->irq;
> @@ -1055,7 +1128,7 @@ static int jz4740_mmc_probe(struct 
> platform_device* pdev)
>  	host->mmc = mmc;
>  	host->pdev = pdev;
>  	spin_lock_init(&host->lock);
> -	host->irq_mask = 0xffff;
> +	host->irq_mask = ~0;
> 
>  	jz4740_mmc_reset(host);

Re: your mail

[James Hogan]

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

On Fri, Mar 09, 2018 at 06:51:36PM +0100, Paul Cercueil wrote:
> Le 2018-03-09 16:12, Ezequiel Garcia a écrit :
> > @@ -164,8 +171,46 @@ struct jz4740_mmc_host {
> >   * trigger is when data words in MSC_TXFIFO is < 8.
> >   */
> >  #define JZ4740_MMC_FIFO_HALF_SIZE 8
> > +
> > +	void (*write_irq_mask)(struct jz4740_mmc_host *host, uint32_t val);
> > +	void (*write_irq_reg)(struct jz4740_mmc_host *host, uint32_t val);
> > +	uint32_t (*read_irq_reg)(struct jz4740_mmc_host *host);
> >  };
> 
> I'm not 100% fan about the callback idea, the original commit
> (https://github.com/gcwnow/linux/commit/62472091) doesn't use them and 
> is
> 30 lines shorter.
> 
> I'm not terribly against either, so if nobody else bug on that, feel 
> free
> to ignore my comment.

I was thinking the same as Paul too to be honest. Unless there is a
measurable benefit to having callbacks, I think its cleaner and more
readable to stick to conditionals and normal abstractions where
appropriate.

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: your mail

[James Hogan]

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

On Fri, Mar 09, 2018 at 06:51:36PM +0100, Paul Cercueil wrote:
> Le 2018-03-09 16:12, Ezequiel Garcia a écrit :
> > @@ -164,8 +171,46 @@ struct jz4740_mmc_host {
> >   * trigger is when data words in MSC_TXFIFO is < 8.
> >   */
> >  #define JZ4740_MMC_FIFO_HALF_SIZE 8
> > +
> > +	void (*write_irq_mask)(struct jz4740_mmc_host *host, uint32_t val);
> > +	void (*write_irq_reg)(struct jz4740_mmc_host *host, uint32_t val);
> > +	uint32_t (*read_irq_reg)(struct jz4740_mmc_host *host);
> >  };
> 
> I'm not 100% fan about the callback idea, the original commit
> (https://github.com/gcwnow/linux/commit/62472091) doesn't use them and 
> is
> 30 lines shorter.
> 
> I'm not terribly against either, so if nobody else bug on that, feel 
> free
> to ignore my comment.

I was thinking the same as Paul too to be honest. Unless there is a
measurable benefit to having callbacks, I think its cleaner and more
readable to stick to conditionals and normal abstractions where
appropriate.

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 08/14] mmc: jz4740: Add support for the JZ4780

[Ezequiel Garcia]

On 9 March 2018 at 14:51, Paul Cercueil <paul@crapouillou.net> wrote:
> Hi,
>
>
> Le 2018-03-09 16:12, Ezequiel Garcia a écrit :
>>
>> From: Alex Smith <alex.smith@imgtec.com>
>>
>> Add support for the JZ4780 MMC controller to the jz47xx_mmc driver. There
>> are a few minor differences from the 4740 to the 4780 that need to be
>> handled, but otherwise the controllers behave the same. The IREG and IMASK
>> registers are expanded to 32 bits. Additionally, some error conditions are
>> now reported in both STATUS and IREG. Writing IREG before reading STATUS
>> causes the bits in STATUS to be cleared, so STATUS must be read first to
>> ensure we see and report error conditions correctly.
>>
>> Signed-off-by: Alex Smith <alex.smith@imgtec.com>
>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>> [Ezequiel: rebase and introduce register accessors]
>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>> ---
>>  drivers/mmc/host/Kconfig      |   2 +-
>>  drivers/mmc/host/jz4740_mmc.c | 111
>> ++++++++++++++++++++++++++++++++++--------
>>  2 files changed, 93 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>> index 620c2d90a646..7dd5169a2dfb 100644
>> --- a/drivers/mmc/host/Kconfig
>> +++ b/drivers/mmc/host/Kconfig
>> @@ -767,7 +767,7 @@ config MMC_SH_MMCIF
>>
>>  config MMC_JZ4740
>>         tristate "JZ4740 SD/Multimedia Card Interface support"
>> -       depends on MACH_JZ4740
>> +       depends on MACH_JZ4740 || MACH_JZ4780
>>         help
>>           This selects support for the SD/MMC controller on Ingenic JZ4740
>>           SoCs.
>> diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
>> index 7d4dcce76cd8..bb1b9114ef53 100644
>> --- a/drivers/mmc/host/jz4740_mmc.c
>> +++ b/drivers/mmc/host/jz4740_mmc.c
>> @@ -1,5 +1,7 @@
>>  /*
>>   *  Copyright (C) 2009-2010, Lars-Peter Clausen <lars@metafoo.de>
>> + *  Copyright (C) 2013, Imagination Technologies
>> + *
>>   *  JZ4740 SD/MMC controller driver
>>   *
>>   *  This program is free software; you can redistribute  it and/or modify
>> it
>> @@ -52,6 +54,7 @@
>>  #define JZ_REG_MMC_RESP_FIFO   0x34
>>  #define JZ_REG_MMC_RXFIFO      0x38
>>  #define JZ_REG_MMC_TXFIFO      0x3C
>> +#define JZ_REG_MMC_DMAC                0x44
>>
>>  #define JZ_MMC_STRPCL_EXIT_MULTIPLE BIT(7)
>>  #define JZ_MMC_STRPCL_EXIT_TRANSFER BIT(6)
>> @@ -105,11 +108,15 @@
>>  #define JZ_MMC_IRQ_PRG_DONE BIT(1)
>>  #define JZ_MMC_IRQ_DATA_TRAN_DONE BIT(0)
>>
>> +#define JZ_MMC_DMAC_DMA_SEL BIT(1)
>> +#define JZ_MMC_DMAC_DMA_EN BIT(0)
>>
>>  #define JZ_MMC_CLK_RATE 24000000
>>
>>  enum jz4740_mmc_version {
>>         JZ_MMC_JZ4740,
>> +       JZ_MMC_JZ4750,
>> +       JZ_MMC_JZ4780,
>>  };
>>
>>  enum jz4740_mmc_state {
>> @@ -144,7 +151,7 @@ struct jz4740_mmc_host {
>>
>>         uint32_t cmdat;
>>
>> -       uint16_t irq_mask;
>> +       uint32_t irq_mask;
>>
>>         spinlock_t lock;
>>
>> @@ -164,8 +171,46 @@ struct jz4740_mmc_host {
>>   * trigger is when data words in MSC_TXFIFO is < 8.
>>   */
>>  #define JZ4740_MMC_FIFO_HALF_SIZE 8
>> +
>> +       void (*write_irq_mask)(struct jz4740_mmc_host *host, uint32_t
>> val);
>> +       void (*write_irq_reg)(struct jz4740_mmc_host *host, uint32_t val);
>> +       uint32_t (*read_irq_reg)(struct jz4740_mmc_host *host);
>>  };
>
>
> I'm not 100% fan about the callback idea, the original commit
> (https://github.com/gcwnow/linux/commit/62472091) doesn't use them and is
> 30 lines shorter.
>
> I'm not terribly against either, so if nobody else bug on that, feel free
> to ignore my comment.
>

On 9 March 2018 at 19:31, James Hogan <james.hogan@mips.com> wrote:
>
> I was thinking the same as Paul too to be honest. Unless there is a
> measurable benefit to having callbacks, I think its cleaner and more
> readable to stick to conditionals and normal abstractions where
> appropriate.

Well, I believe the benefit of having callbacks is scalability and readability,
but perhaps it's only a matter of taste. Once the helpers are there,
adding a new one is just adding the callbacks.

I've always thought this:

static uint32_t jz4740_mmc_read_irq_reg(struct jz4740_mmc_host *host)
{
        return readw(host->base + JZ_REG_MMC_IREG);
}

static uint32_t jz4780_mmc_read_irq_reg(struct jz4740_mmc_host *host)
{
        return readl(host->base + JZ_REG_MMC_IREG);
}

looks so much better than this:

static uint32_t jz_mmc_read_irq_reg(struct jz4740_mmc_host *host)
{
        if (host->version == JZ_MMC_JZ4780) {
                return readl(host->base + JZ_REG_MMC_IREG);
        } else if ... {
                return readw(host->base + JZ_REG_MMC_IREG);
        }
}

In any case, if you guys are strong about the if-else-ism,
I'm fine changing it.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 08/14] mmc: jz4740: Add support for the JZ4780

[Mathieu Malaterre]

On Fri, Mar 9, 2018 at 4:12 PM, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
> From: Alex Smith <alex.smith@imgtec.com>
>
> Add support for the JZ4780 MMC controller to the jz47xx_mmc driver. There
> are a few minor differences from the 4740 to the 4780 that need to be
> handled, but otherwise the controllers behave the same. The IREG and IMASK
> registers are expanded to 32 bits. Additionally, some error conditions are
> now reported in both STATUS and IREG. Writing IREG before reading STATUS
> causes the bits in STATUS to be cleared, so STATUS must be read first to
> ensure we see and report error conditions correctly.
>
> Signed-off-by: Alex Smith <alex.smith@imgtec.com>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> [Ezequiel: rebase and introduce register accessors]
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
>  drivers/mmc/host/Kconfig      |   2 +-
>  drivers/mmc/host/jz4740_mmc.c | 111 ++++++++++++++++++++++++++++++++++--------
>  2 files changed, 93 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 620c2d90a646..7dd5169a2dfb 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -767,7 +767,7 @@ config MMC_SH_MMCIF
>
>  config MMC_JZ4740
>         tristate "JZ4740 SD/Multimedia Card Interface support"
> -       depends on MACH_JZ4740
> +       depends on MACH_JZ4740 || MACH_JZ4780
>         help
>           This selects support for the SD/MMC controller on Ingenic JZ4740
>           SoCs.
> diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
> index 7d4dcce76cd8..bb1b9114ef53 100644
> --- a/drivers/mmc/host/jz4740_mmc.c
> +++ b/drivers/mmc/host/jz4740_mmc.c
> @@ -1,5 +1,7 @@
>  /*
>   *  Copyright (C) 2009-2010, Lars-Peter Clausen <lars@metafoo.de>
> + *  Copyright (C) 2013, Imagination Technologies
> + *
>   *  JZ4740 SD/MMC controller driver
>   *
>   *  This program is free software; you can redistribute  it and/or modify it
> @@ -52,6 +54,7 @@
>  #define JZ_REG_MMC_RESP_FIFO   0x34
>  #define JZ_REG_MMC_RXFIFO      0x38
>  #define JZ_REG_MMC_TXFIFO      0x3C
> +#define JZ_REG_MMC_DMAC                0x44
>
>  #define JZ_MMC_STRPCL_EXIT_MULTIPLE BIT(7)
>  #define JZ_MMC_STRPCL_EXIT_TRANSFER BIT(6)
> @@ -105,11 +108,15 @@
>  #define JZ_MMC_IRQ_PRG_DONE BIT(1)
>  #define JZ_MMC_IRQ_DATA_TRAN_DONE BIT(0)
>
> +#define JZ_MMC_DMAC_DMA_SEL BIT(1)
> +#define JZ_MMC_DMAC_DMA_EN BIT(0)
>
>  #define JZ_MMC_CLK_RATE 24000000
>
>  enum jz4740_mmc_version {
>         JZ_MMC_JZ4740,
> +       JZ_MMC_JZ4750,
> +       JZ_MMC_JZ4780,
>  };

Great to see support for JZ4750, but I did not see it documented in
the commit message.

>  enum jz4740_mmc_state {
> @@ -144,7 +151,7 @@ struct jz4740_mmc_host {
>
>         uint32_t cmdat;
>
> -       uint16_t irq_mask;
> +       uint32_t irq_mask;
>
>         spinlock_t lock;
>
> @@ -164,8 +171,46 @@ struct jz4740_mmc_host {
>   * trigger is when data words in MSC_TXFIFO is < 8.
>   */
>  #define JZ4740_MMC_FIFO_HALF_SIZE 8
> +
> +       void (*write_irq_mask)(struct jz4740_mmc_host *host, uint32_t val);
> +       void (*write_irq_reg)(struct jz4740_mmc_host *host, uint32_t val);
> +       uint32_t (*read_irq_reg)(struct jz4740_mmc_host *host);
>  };
>
> +static void jz4750_mmc_write_irq_mask(struct jz4740_mmc_host *host,
> +                                     uint32_t val)
> +{
> +       return writel(val, host->base + JZ_REG_MMC_IMASK);
> +}
> +
> +static void jz4740_mmc_write_irq_mask(struct jz4740_mmc_host *host,
> +                                     uint32_t val)
> +{
> +       return writew(val, host->base + JZ_REG_MMC_IMASK);
> +}
> +
> +static void jz4740_mmc_write_irq_reg(struct jz4740_mmc_host *host,
> +                                    uint32_t val)
> +{
> +       return writew(val, host->base + JZ_REG_MMC_IREG);
> +}
> +
> +static uint32_t jz4740_mmc_read_irq_reg(struct jz4740_mmc_host *host)
> +{
> +       return readw(host->base + JZ_REG_MMC_IREG);
> +}
> +
> +static void jz4780_mmc_write_irq_reg(struct jz4740_mmc_host *host, uint32_t val)
> +{
> +       return writel(val, host->base + JZ_REG_MMC_IREG);
> +}
> +
> +/* In the 4780 onwards, IREG is expanded to 32 bits. */
> +static uint32_t jz4780_mmc_read_irq_reg(struct jz4740_mmc_host *host)
> +{
> +       return readl(host->base + JZ_REG_MMC_IREG);
> +}
> +
>  /*----------------------------------------------------------------------------*/
>  /* DMA infrastructure */
>
> @@ -371,7 +416,7 @@ static void jz4740_mmc_set_irq_enabled(struct jz4740_mmc_host *host,
>                 host->irq_mask |= irq;
>         spin_unlock_irqrestore(&host->lock, flags);
>
> -       writew(host->irq_mask, host->base + JZ_REG_MMC_IMASK);
> +       host->write_irq_mask(host, host->irq_mask);
>  }
>
>  static void jz4740_mmc_clock_enable(struct jz4740_mmc_host *host,
> @@ -422,10 +467,10 @@ static unsigned int jz4740_mmc_poll_irq(struct jz4740_mmc_host *host,
>         unsigned int irq)
>  {
>         unsigned int timeout = 0x800;
> -       uint16_t status;
> +       uint32_t status;
>
>         do {
> -               status = readw(host->base + JZ_REG_MMC_IREG);
> +               status = host->read_irq_reg(host);
>         } while (!(status & irq) && --timeout);
>
>         if (timeout == 0) {
> @@ -525,7 +570,7 @@ static bool jz4740_mmc_read_data(struct jz4740_mmc_host *host,
>         void __iomem *fifo_addr = host->base + JZ_REG_MMC_RXFIFO;
>         uint32_t *buf;
>         uint32_t d;
> -       uint16_t status;
> +       uint32_t status;
>         size_t i, j;
>         unsigned int timeout;
>
> @@ -661,8 +706,25 @@ static void jz4740_mmc_send_command(struct jz4740_mmc_host *host,
>                 cmdat |= JZ_MMC_CMDAT_DATA_EN;
>                 if (cmd->data->flags & MMC_DATA_WRITE)
>                         cmdat |= JZ_MMC_CMDAT_WRITE;
> -               if (host->use_dma)
> -                       cmdat |= JZ_MMC_CMDAT_DMA_EN;
> +               if (host->use_dma) {
> +                       /*
> +                        * The 4780's MMC controller has integrated DMA ability
> +                        * in addition to being able to use the external DMA
> +                        * controller. It moves DMA control bits to a separate
> +                        * register. The DMA_SEL bit chooses the external
> +                        * controller over the integrated one. Earlier SoCs
> +                        * can only use the external controller, and have a
> +                        * single DMA enable bit in CMDAT.
> +                        */
> +                       if (host->version >= JZ_MMC_JZ4780) {
> +                               writel(JZ_MMC_DMAC_DMA_EN | JZ_MMC_DMAC_DMA_SEL,
> +                                      host->base + JZ_REG_MMC_DMAC);
> +                       } else {
> +                               cmdat |= JZ_MMC_CMDAT_DMA_EN;
> +                       }
> +               } else if (host->version >= JZ_MMC_JZ4780) {
> +                       writel(0, host->base + JZ_REG_MMC_DMAC);
> +               }
>
>                 writew(cmd->data->blksz, host->base + JZ_REG_MMC_BLKLEN);
>                 writew(cmd->data->blocks, host->base + JZ_REG_MMC_NOB);
> @@ -743,7 +805,7 @@ static irqreturn_t jz_mmc_irq_worker(int irq, void *devid)
>                         host->state = JZ4740_MMC_STATE_SEND_STOP;
>                         break;
>                 }
> -               writew(JZ_MMC_IRQ_DATA_TRAN_DONE, host->base + JZ_REG_MMC_IREG);
> +               host->write_irq_reg(host, JZ_MMC_IRQ_DATA_TRAN_DONE);
>
>         case JZ4740_MMC_STATE_SEND_STOP:
>                 if (!req->stop)
> @@ -773,9 +835,10 @@ static irqreturn_t jz_mmc_irq(int irq, void *devid)
>  {
>         struct jz4740_mmc_host *host = devid;
>         struct mmc_command *cmd = host->cmd;
> -       uint16_t irq_reg, status, tmp;
> +       uint32_t irq_reg, status, tmp;
>
> -       irq_reg = readw(host->base + JZ_REG_MMC_IREG);
> +       status = readl(host->base + JZ_REG_MMC_STATUS);
> +       irq_reg = host->read_irq_reg(host);
>
>         tmp = irq_reg;
>         irq_reg &= ~host->irq_mask;
> @@ -784,10 +847,10 @@ static irqreturn_t jz_mmc_irq(int irq, void *devid)
>                 JZ_MMC_IRQ_PRG_DONE | JZ_MMC_IRQ_DATA_TRAN_DONE);
>
>         if (tmp != irq_reg)
> -               writew(tmp & ~irq_reg, host->base + JZ_REG_MMC_IREG);
> +               host->write_irq_reg(host, tmp & ~irq_reg);
>
>         if (irq_reg & JZ_MMC_IRQ_SDIO) {
> -               writew(JZ_MMC_IRQ_SDIO, host->base + JZ_REG_MMC_IREG);
> +               host->write_irq_reg(host, JZ_MMC_IRQ_SDIO);
>                 mmc_signal_sdio_irq(host->mmc);
>                 irq_reg &= ~JZ_MMC_IRQ_SDIO;
>         }
> @@ -796,8 +859,6 @@ static irqreturn_t jz_mmc_irq(int irq, void *devid)
>                 if (test_and_clear_bit(0, &host->waiting)) {
>                         del_timer(&host->timeout_timer);
>
> -                       status = readl(host->base + JZ_REG_MMC_STATUS);
> -
>                         if (status & JZ_MMC_STATUS_TIMEOUT_RES) {
>                                         cmd->error = -ETIMEDOUT;
>                         } else if (status & JZ_MMC_STATUS_CRC_RES_ERR) {
> @@ -810,7 +871,7 @@ static irqreturn_t jz_mmc_irq(int irq, void *devid)
>                         }
>
>                         jz4740_mmc_set_irq_enabled(host, irq_reg, false);
> -                       writew(irq_reg, host->base + JZ_REG_MMC_IREG);
> +                       host->write_irq_reg(host, irq_reg);
>
>                         return IRQ_WAKE_THREAD;
>                 }
> @@ -844,9 +905,7 @@ static void jz4740_mmc_request(struct mmc_host *mmc, struct mmc_request *req)
>
>         host->req = req;
>
> -       writew(0xffff, host->base + JZ_REG_MMC_IREG);
> -
> -       writew(JZ_MMC_IRQ_END_CMD_RES, host->base + JZ_REG_MMC_IREG);
> +       host->write_irq_reg(host, ~0);
>         jz4740_mmc_set_irq_enabled(host, JZ_MMC_IRQ_END_CMD_RES, true);
>
>         host->state = JZ4740_MMC_STATE_READ_RESPONSE;
> @@ -973,6 +1032,7 @@ static void jz4740_mmc_free_gpios(struct platform_device *pdev)
>
>  static const struct of_device_id jz4740_mmc_of_match[] = {
>         { .compatible = "ingenic,jz4740-mmc", .data = (void *) JZ_MMC_JZ4740 },
> +       { .compatible = "ingenic,jz4780-mmc", .data = (void *) JZ_MMC_JZ4780 },
>         {},
>  };
>  MODULE_DEVICE_TABLE(of, jz4740_mmc_of_match);
> @@ -1017,6 +1077,19 @@ static int jz4740_mmc_probe(struct platform_device* pdev)
>                         goto err_free_host;
>         }
>
> +       if (host->version >= JZ_MMC_JZ4780) {
> +               host->write_irq_reg = jz4780_mmc_write_irq_reg;
> +               host->read_irq_reg = jz4780_mmc_read_irq_reg;
> +       } else {
> +               host->write_irq_reg = jz4740_mmc_write_irq_reg;
> +               host->read_irq_reg = jz4740_mmc_read_irq_reg;
> +       }
> +
> +       if (host->version >= JZ_MMC_JZ4750)
> +               host->write_irq_mask = jz4750_mmc_write_irq_mask;
> +       else
> +               host->write_irq_mask = jz4740_mmc_write_irq_mask;
> +
>         host->irq = platform_get_irq(pdev, 0);
>         if (host->irq < 0) {
>                 ret = host->irq;
> @@ -1055,7 +1128,7 @@ static int jz4740_mmc_probe(struct platform_device* pdev)
>         host->mmc = mmc;
>         host->pdev = pdev;
>         spin_lock_init(&host->lock);
> -       host->irq_mask = 0xffff;
> +       host->irq_mask = ~0;
>
>         jz4740_mmc_reset(host);
>
> --
> 2.16.2
>
>

Re: [PATCH 08/14] mmc: jz4740: Add support for the JZ4780

[Mathieu Malaterre]

On Fri, Mar 9, 2018 at 4:12 PM, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
> From: Alex Smith <alex.smith@imgtec.com>
>
> Add support for the JZ4780 MMC controller to the jz47xx_mmc driver. There
> are a few minor differences from the 4740 to the 4780 that need to be
> handled, but otherwise the controllers behave the same. The IREG and IMASK
> registers are expanded to 32 bits. Additionally, some error conditions are
> now reported in both STATUS and IREG. Writing IREG before reading STATUS
> causes the bits in STATUS to be cleared, so STATUS must be read first to
> ensure we see and report error conditions correctly.
>
> Signed-off-by: Alex Smith <alex.smith@imgtec.com>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> [Ezequiel: rebase and introduce register accessors]
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
>  drivers/mmc/host/Kconfig      |   2 +-
>  drivers/mmc/host/jz4740_mmc.c | 111 ++++++++++++++++++++++++++++++++++--------
>  2 files changed, 93 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 620c2d90a646..7dd5169a2dfb 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -767,7 +767,7 @@ config MMC_SH_MMCIF
>
>  config MMC_JZ4740
>         tristate "JZ4740 SD/Multimedia Card Interface support"
> -       depends on MACH_JZ4740
> +       depends on MACH_JZ4740 || MACH_JZ4780
>         help
>           This selects support for the SD/MMC controller on Ingenic JZ4740
>           SoCs.

Nitpick:  on Ingenic JZ4740 & JZ4780 (tristate & help)

> diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
> index 7d4dcce76cd8..bb1b9114ef53 100644
> --- a/drivers/mmc/host/jz4740_mmc.c
> +++ b/drivers/mmc/host/jz4740_mmc.c
> @@ -1,5 +1,7 @@
>  /*
>   *  Copyright (C) 2009-2010, Lars-Peter Clausen <lars@metafoo.de>
> + *  Copyright (C) 2013, Imagination Technologies
> + *
>   *  JZ4740 SD/MMC controller driver
>   *
>   *  This program is free software; you can redistribute  it and/or modify it
> @@ -52,6 +54,7 @@
>  #define JZ_REG_MMC_RESP_FIFO   0x34
>  #define JZ_REG_MMC_RXFIFO      0x38
>  #define JZ_REG_MMC_TXFIFO      0x3C
> +#define JZ_REG_MMC_DMAC                0x44
>
>  #define JZ_MMC_STRPCL_EXIT_MULTIPLE BIT(7)
>  #define JZ_MMC_STRPCL_EXIT_TRANSFER BIT(6)
> @@ -105,11 +108,15 @@
>  #define JZ_MMC_IRQ_PRG_DONE BIT(1)
>  #define JZ_MMC_IRQ_DATA_TRAN_DONE BIT(0)
>
> +#define JZ_MMC_DMAC_DMA_SEL BIT(1)
> +#define JZ_MMC_DMAC_DMA_EN BIT(0)
>
>  #define JZ_MMC_CLK_RATE 24000000
>
>  enum jz4740_mmc_version {
>         JZ_MMC_JZ4740,
> +       JZ_MMC_JZ4750,
> +       JZ_MMC_JZ4780,
>  };
>
>  enum jz4740_mmc_state {
> @@ -144,7 +151,7 @@ struct jz4740_mmc_host {
>
>         uint32_t cmdat;
>
> -       uint16_t irq_mask;
> +       uint32_t irq_mask;
>
>         spinlock_t lock;
>
> @@ -164,8 +171,46 @@ struct jz4740_mmc_host {
>   * trigger is when data words in MSC_TXFIFO is < 8.
>   */
>  #define JZ4740_MMC_FIFO_HALF_SIZE 8
> +
> +       void (*write_irq_mask)(struct jz4740_mmc_host *host, uint32_t val);
> +       void (*write_irq_reg)(struct jz4740_mmc_host *host, uint32_t val);
> +       uint32_t (*read_irq_reg)(struct jz4740_mmc_host *host);
>  };
>
> +static void jz4750_mmc_write_irq_mask(struct jz4740_mmc_host *host,
> +                                     uint32_t val)
> +{
> +       return writel(val, host->base + JZ_REG_MMC_IMASK);
> +}
> +
> +static void jz4740_mmc_write_irq_mask(struct jz4740_mmc_host *host,
> +                                     uint32_t val)
> +{
> +       return writew(val, host->base + JZ_REG_MMC_IMASK);
> +}
> +
> +static void jz4740_mmc_write_irq_reg(struct jz4740_mmc_host *host,
> +                                    uint32_t val)
> +{
> +       return writew(val, host->base + JZ_REG_MMC_IREG);
> +}
> +
> +static uint32_t jz4740_mmc_read_irq_reg(struct jz4740_mmc_host *host)
> +{
> +       return readw(host->base + JZ_REG_MMC_IREG);
> +}
> +
> +static void jz4780_mmc_write_irq_reg(struct jz4740_mmc_host *host, uint32_t val)
> +{
> +       return writel(val, host->base + JZ_REG_MMC_IREG);
> +}
> +
> +/* In the 4780 onwards, IREG is expanded to 32 bits. */
> +static uint32_t jz4780_mmc_read_irq_reg(struct jz4740_mmc_host *host)
> +{
> +       return readl(host->base + JZ_REG_MMC_IREG);
> +}
> +
>  /*----------------------------------------------------------------------------*/
>  /* DMA infrastructure */
>
> @@ -371,7 +416,7 @@ static void jz4740_mmc_set_irq_enabled(struct jz4740_mmc_host *host,
>                 host->irq_mask |= irq;
>         spin_unlock_irqrestore(&host->lock, flags);
>
> -       writew(host->irq_mask, host->base + JZ_REG_MMC_IMASK);
> +       host->write_irq_mask(host, host->irq_mask);
>  }
>
>  static void jz4740_mmc_clock_enable(struct jz4740_mmc_host *host,
> @@ -422,10 +467,10 @@ static unsigned int jz4740_mmc_poll_irq(struct jz4740_mmc_host *host,
>         unsigned int irq)
>  {
>         unsigned int timeout = 0x800;
> -       uint16_t status;
> +       uint32_t status;
>
>         do {
> -               status = readw(host->base + JZ_REG_MMC_IREG);
> +               status = host->read_irq_reg(host);
>         } while (!(status & irq) && --timeout);
>
>         if (timeout == 0) {
> @@ -525,7 +570,7 @@ static bool jz4740_mmc_read_data(struct jz4740_mmc_host *host,
>         void __iomem *fifo_addr = host->base + JZ_REG_MMC_RXFIFO;
>         uint32_t *buf;
>         uint32_t d;
> -       uint16_t status;
> +       uint32_t status;
>         size_t i, j;
>         unsigned int timeout;
>
> @@ -661,8 +706,25 @@ static void jz4740_mmc_send_command(struct jz4740_mmc_host *host,
>                 cmdat |= JZ_MMC_CMDAT_DATA_EN;
>                 if (cmd->data->flags & MMC_DATA_WRITE)
>                         cmdat |= JZ_MMC_CMDAT_WRITE;
> -               if (host->use_dma)
> -                       cmdat |= JZ_MMC_CMDAT_DMA_EN;
> +               if (host->use_dma) {
> +                       /*
> +                        * The 4780's MMC controller has integrated DMA ability
> +                        * in addition to being able to use the external DMA
> +                        * controller. It moves DMA control bits to a separate
> +                        * register. The DMA_SEL bit chooses the external
> +                        * controller over the integrated one. Earlier SoCs
> +                        * can only use the external controller, and have a
> +                        * single DMA enable bit in CMDAT.
> +                        */
> +                       if (host->version >= JZ_MMC_JZ4780) {
> +                               writel(JZ_MMC_DMAC_DMA_EN | JZ_MMC_DMAC_DMA_SEL,
> +                                      host->base + JZ_REG_MMC_DMAC);
> +                       } else {
> +                               cmdat |= JZ_MMC_CMDAT_DMA_EN;
> +                       }
> +               } else if (host->version >= JZ_MMC_JZ4780) {
> +                       writel(0, host->base + JZ_REG_MMC_DMAC);
> +               }
>
>                 writew(cmd->data->blksz, host->base + JZ_REG_MMC_BLKLEN);
>                 writew(cmd->data->blocks, host->base + JZ_REG_MMC_NOB);
> @@ -743,7 +805,7 @@ static irqreturn_t jz_mmc_irq_worker(int irq, void *devid)
>                         host->state = JZ4740_MMC_STATE_SEND_STOP;
>                         break;
>                 }
> -               writew(JZ_MMC_IRQ_DATA_TRAN_DONE, host->base + JZ_REG_MMC_IREG);
> +               host->write_irq_reg(host, JZ_MMC_IRQ_DATA_TRAN_DONE);
>
>         case JZ4740_MMC_STATE_SEND_STOP:
>                 if (!req->stop)
> @@ -773,9 +835,10 @@ static irqreturn_t jz_mmc_irq(int irq, void *devid)
>  {
>         struct jz4740_mmc_host *host = devid;
>         struct mmc_command *cmd = host->cmd;
> -       uint16_t irq_reg, status, tmp;
> +       uint32_t irq_reg, status, tmp;
>
> -       irq_reg = readw(host->base + JZ_REG_MMC_IREG);
> +       status = readl(host->base + JZ_REG_MMC_STATUS);
> +       irq_reg = host->read_irq_reg(host);
>
>         tmp = irq_reg;
>         irq_reg &= ~host->irq_mask;
> @@ -784,10 +847,10 @@ static irqreturn_t jz_mmc_irq(int irq, void *devid)
>                 JZ_MMC_IRQ_PRG_DONE | JZ_MMC_IRQ_DATA_TRAN_DONE);
>
>         if (tmp != irq_reg)
> -               writew(tmp & ~irq_reg, host->base + JZ_REG_MMC_IREG);
> +               host->write_irq_reg(host, tmp & ~irq_reg);
>
>         if (irq_reg & JZ_MMC_IRQ_SDIO) {
> -               writew(JZ_MMC_IRQ_SDIO, host->base + JZ_REG_MMC_IREG);
> +               host->write_irq_reg(host, JZ_MMC_IRQ_SDIO);
>                 mmc_signal_sdio_irq(host->mmc);
>                 irq_reg &= ~JZ_MMC_IRQ_SDIO;
>         }
> @@ -796,8 +859,6 @@ static irqreturn_t jz_mmc_irq(int irq, void *devid)
>                 if (test_and_clear_bit(0, &host->waiting)) {
>                         del_timer(&host->timeout_timer);
>
> -                       status = readl(host->base + JZ_REG_MMC_STATUS);
> -
>                         if (status & JZ_MMC_STATUS_TIMEOUT_RES) {
>                                         cmd->error = -ETIMEDOUT;
>                         } else if (status & JZ_MMC_STATUS_CRC_RES_ERR) {
> @@ -810,7 +871,7 @@ static irqreturn_t jz_mmc_irq(int irq, void *devid)
>                         }
>
>                         jz4740_mmc_set_irq_enabled(host, irq_reg, false);
> -                       writew(irq_reg, host->base + JZ_REG_MMC_IREG);
> +                       host->write_irq_reg(host, irq_reg);
>
>                         return IRQ_WAKE_THREAD;
>                 }
> @@ -844,9 +905,7 @@ static void jz4740_mmc_request(struct mmc_host *mmc, struct mmc_request *req)
>
>         host->req = req;
>
> -       writew(0xffff, host->base + JZ_REG_MMC_IREG);
> -
> -       writew(JZ_MMC_IRQ_END_CMD_RES, host->base + JZ_REG_MMC_IREG);
> +       host->write_irq_reg(host, ~0);
>         jz4740_mmc_set_irq_enabled(host, JZ_MMC_IRQ_END_CMD_RES, true);
>
>         host->state = JZ4740_MMC_STATE_READ_RESPONSE;
> @@ -973,6 +1032,7 @@ static void jz4740_mmc_free_gpios(struct platform_device *pdev)
>
>  static const struct of_device_id jz4740_mmc_of_match[] = {
>         { .compatible = "ingenic,jz4740-mmc", .data = (void *) JZ_MMC_JZ4740 },
> +       { .compatible = "ingenic,jz4780-mmc", .data = (void *) JZ_MMC_JZ4780 },
>         {},
>  };
>  MODULE_DEVICE_TABLE(of, jz4740_mmc_of_match);
> @@ -1017,6 +1077,19 @@ static int jz4740_mmc_probe(struct platform_device* pdev)
>                         goto err_free_host;
>         }
>
> +       if (host->version >= JZ_MMC_JZ4780) {
> +               host->write_irq_reg = jz4780_mmc_write_irq_reg;
> +               host->read_irq_reg = jz4780_mmc_read_irq_reg;
> +       } else {
> +               host->write_irq_reg = jz4740_mmc_write_irq_reg;
> +               host->read_irq_reg = jz4740_mmc_read_irq_reg;
> +       }
> +
> +       if (host->version >= JZ_MMC_JZ4750)
> +               host->write_irq_mask = jz4750_mmc_write_irq_mask;
> +       else
> +               host->write_irq_mask = jz4740_mmc_write_irq_mask;
> +
>         host->irq = platform_get_irq(pdev, 0);
>         if (host->irq < 0) {
>                 ret = host->irq;
> @@ -1055,7 +1128,7 @@ static int jz4740_mmc_probe(struct platform_device* pdev)
>         host->mmc = mmc;
>         host->pdev = pdev;
>         spin_lock_init(&host->lock);
> -       host->irq_mask = 0xffff;
> +       host->irq_mask = ~0;
>
>         jz4740_mmc_reset(host);
>
> --
> 2.16.2
>
>

[PATCH 09/14] mmc: jz4740: Fix race condition in IRQ mask update

[Ezequiel Garcia]

From: Alex Smith <alex.smith@imgtec.com>

A spinlock is held while updating the internal copy of the IRQ mask,
but not while writing it to the actual IMASK register. After the lock
is released, an IRQ can occur before the IMASK register is written.
If handling this IRQ causes the mask to be changed, when the handler
returns back to the middle of the first mask update, a stale value
will be written to the mask register.

If this causes an IRQ to become unmasked that cannot have its status
cleared by writing a 1 to it in the IREG register, e.g. the SDIO IRQ,
then we can end up stuck with the same IRQ repeatedly being fired but
not handled. Normally the MMC IRQ handler attempts to clear any
unexpected IRQs by writing IREG, but for those that cannot be cleared
in this way then the IRQ will just repeatedly fire.

This was resulting in lockups after a while of using Wi-Fi on the
CI20 (GitHub issue #19).

Resolve by holding the spinlock until after the IMASK register has
been updated.

Signed-off-by: Alex Smith <alex.smith@imgtec.com>
---
 drivers/mmc/host/jz4740_mmc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
index bb1b9114ef53..55587d798d47 100644
--- a/drivers/mmc/host/jz4740_mmc.c
+++ b/drivers/mmc/host/jz4740_mmc.c
@@ -410,13 +410,15 @@ static void jz4740_mmc_set_irq_enabled(struct jz4740_mmc_host *host,
 	unsigned long flags;
 
 	spin_lock_irqsave(&host->lock, flags);
+
 	if (enabled)
 		host->irq_mask &= ~irq;
 	else
 		host->irq_mask |= irq;
-	spin_unlock_irqrestore(&host->lock, flags);
 
 	host->write_irq_mask(host, host->irq_mask);
+
+	spin_unlock_irqrestore(&host->lock, flags);
 }
 
 static void jz4740_mmc_clock_enable(struct jz4740_mmc_host *host,
-- 
2.16.2

Re: [PATCH 09/14] mmc: jz4740: Fix race condition in IRQ mask update

[James Hogan]

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

On Fri, Mar 09, 2018 at 12:12:14PM -0300, Ezequiel Garcia wrote:
> From: Alex Smith <alex.smith@imgtec.com>
> 
> A spinlock is held while updating the internal copy of the IRQ mask,
> but not while writing it to the actual IMASK register. After the lock
> is released, an IRQ can occur before the IMASK register is written.
> If handling this IRQ causes the mask to be changed, when the handler
> returns back to the middle of the first mask update, a stale value
> will be written to the mask register.
> 
> If this causes an IRQ to become unmasked that cannot have its status
> cleared by writing a 1 to it in the IREG register, e.g. the SDIO IRQ,
> then we can end up stuck with the same IRQ repeatedly being fired but
> not handled. Normally the MMC IRQ handler attempts to clear any
> unexpected IRQs by writing IREG, but for those that cannot be cleared
> in this way then the IRQ will just repeatedly fire.
> 
> This was resulting in lockups after a while of using Wi-Fi on the
> CI20 (GitHub issue #19).
> 
> Resolve by holding the spinlock until after the IMASK register has
> been updated.
> 

Maybe have a Link tag instead of referencing "github issue #19", i.e.:
Link: https://github.com/MIPS/CI20_linux/issues/19

Since this fixes an older commit, it'd be worth adding:

Fixes: 61bfbdb85687 ("MMC: Add support for the controller on JZ4740 SoCs.")

> Signed-off-by: Alex Smith <alex.smith@imgtec.com>

... and presumably is worthy of backporting (the driver was introduced
in 2.6.36), i.e.:
Cc: stable@vger.kernel.org

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 09/14] mmc: jz4740: Fix race condition in IRQ mask update

[Ezequiel Garcia]

On 9 March 2018 at 19:49, James Hogan <jhogan@kernel.org> wrote:
> On Fri, Mar 09, 2018 at 12:12:14PM -0300, Ezequiel Garcia wrote:
>> From: Alex Smith <alex.smith@imgtec.com>
>>
>> A spinlock is held while updating the internal copy of the IRQ mask,
>> but not while writing it to the actual IMASK register. After the lock
>> is released, an IRQ can occur before the IMASK register is written.
>> If handling this IRQ causes the mask to be changed, when the handler
>> returns back to the middle of the first mask update, a stale value
>> will be written to the mask register.
>>
>> If this causes an IRQ to become unmasked that cannot have its status
>> cleared by writing a 1 to it in the IREG register, e.g. the SDIO IRQ,
>> then we can end up stuck with the same IRQ repeatedly being fired but
>> not handled. Normally the MMC IRQ handler attempts to clear any
>> unexpected IRQs by writing IREG, but for those that cannot be cleared
>> in this way then the IRQ will just repeatedly fire.
>>
>> This was resulting in lockups after a while of using Wi-Fi on the
>> CI20 (GitHub issue #19).
>>
>> Resolve by holding the spinlock until after the IMASK register has
>> been updated.
>>
>
> Maybe have a Link tag instead of referencing "github issue #19", i.e.:
> Link: https://github.com/MIPS/CI20_linux/issues/19
>
> Since this fixes an older commit, it'd be worth adding:
>
> Fixes: 61bfbdb85687 ("MMC: Add support for the controller on JZ4740 SoCs.")
>
>> Signed-off-by: Alex Smith <alex.smith@imgtec.com>
>
> ... and presumably is worthy of backporting (the driver was introduced
> in 2.6.36), i.e.:
> Cc: stable@vger.kernel.org
>

Oh, yes, good catch. I've overlooked this commit.

Thanks,
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

[PATCH 10/14] mmc: jz4740: Use dma_request_chan()

[Ezequiel Garcia]

Replace dma_request_channel() with dma_request_chan(),
which also supports probing from the devicetree.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/mmc/host/jz4740_mmc.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
index 55587d798d47..01016e5cdddc 100644
--- a/drivers/mmc/host/jz4740_mmc.c
+++ b/drivers/mmc/host/jz4740_mmc.c
@@ -225,31 +225,23 @@ static void jz4740_mmc_release_dma_channels(struct jz4740_mmc_host *host)
 
 static int jz4740_mmc_acquire_dma_channels(struct jz4740_mmc_host *host)
 {
-	dma_cap_mask_t mask;
-
-	dma_cap_zero(mask);
-	dma_cap_set(DMA_SLAVE, mask);
-
-	host->dma_tx = dma_request_channel(mask, NULL, host);
-	if (!host->dma_tx) {
+	host->dma_tx = dma_request_chan(mmc_dev(host->mmc), "tx");
+	if (IS_ERR(host->dma_tx)) {
 		dev_err(mmc_dev(host->mmc), "Failed to get dma_tx channel\n");
-		return -ENODEV;
+		return PTR_ERR(host->dma_tx);
 	}
 
-	host->dma_rx = dma_request_channel(mask, NULL, host);
-	if (!host->dma_rx) {
+	host->dma_rx = dma_request_chan(mmc_dev(host->mmc), "rx");
+	if (IS_ERR(host->dma_rx)) {
 		dev_err(mmc_dev(host->mmc), "Failed to get dma_rx channel\n");
-		goto free_master_write;
+		dma_release_channel(host->dma_tx);
+		return PTR_ERR(host->dma_rx);
 	}
 
 	/* Initialize DMA pre request cookie */
 	host->next_data.cookie = 1;
 
 	return 0;
-
-free_master_write:
-	dma_release_channel(host->dma_tx);
-	return -ENODEV;
 }
 
 static inline struct dma_chan *jz4740_mmc_get_dma_chan(struct jz4740_mmc_host *host,
-- 
2.16.2

[PATCH 11/14] MIPS: dts: jz4780: Add DMA controller node to the devicetree

[Ezequiel Garcia]

This commit adds the devicetree node to support the DMA
controller found in JZ480 SoCs.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 arch/mips/boot/dts/ingenic/jz4780.dtsi | 14 ++++++++++
 include/dt-bindings/dma/jz4780-dma.h   | 49 ++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)
 create mode 100644 include/dt-bindings/dma/jz4780-dma.h

diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 9b5794667aee..fd2ef820fbe3 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <dt-bindings/clock/jz4780-cgu.h>
+#include <dt-bindings/dma/jz4780-dma.h>
 
 / {
 	#address-cells = <1>;
@@ -241,6 +242,19 @@
 		status = "disabled";
 	};
 
+	dma: dma@13420000 {
+		compatible = "ingenic,jz4780-dma";
+		reg = <0x13420000 0x10000>;
+		#dma-cells = <2>;
+
+		interrupt-parent = <&intc>;
+		interrupts = <10>;
+
+		clocks = <&cgu JZ4780_CLK_PDMA>;
+
+		status = "disabled";
+	};
+
 	bch: bch@134d0000 {
 		compatible = "ingenic,jz4780-bch";
 		reg = <0x134d0000 0x10000>;
diff --git a/include/dt-bindings/dma/jz4780-dma.h b/include/dt-bindings/dma/jz4780-dma.h
new file mode 100644
index 000000000000..df017fdfb44e
--- /dev/null
+++ b/include/dt-bindings/dma/jz4780-dma.h
@@ -0,0 +1,49 @@
+#ifndef __DT_BINDINGS_DMA_JZ4780_DMA_H__
+#define __DT_BINDINGS_DMA_JZ4780_DMA_H__
+
+/*
+ * Request type numbers for the JZ4780 DMA controller (written to the DRTn
+ * register for the channel).
+ */
+#define JZ4780_DMA_I2S1_TX	0x4
+#define JZ4780_DMA_I2S1_RX	0x5
+#define JZ4780_DMA_I2S0_TX	0x6
+#define JZ4780_DMA_I2S0_RX	0x7
+#define JZ4780_DMA_AUTO		0x8
+#define JZ4780_DMA_SADC_RX	0x9
+#define JZ4780_DMA_UART4_TX	0xc
+#define JZ4780_DMA_UART4_RX	0xd
+#define JZ4780_DMA_UART3_TX	0xe
+#define JZ4780_DMA_UART3_RX	0xf
+#define JZ4780_DMA_UART2_TX	0x10
+#define JZ4780_DMA_UART2_RX	0x11
+#define JZ4780_DMA_UART1_TX	0x12
+#define JZ4780_DMA_UART1_RX	0x13
+#define JZ4780_DMA_UART0_TX	0x14
+#define JZ4780_DMA_UART0_RX	0x15
+#define JZ4780_DMA_SSI0_TX	0x16
+#define JZ4780_DMA_SSI0_RX	0x17
+#define JZ4780_DMA_SSI1_TX	0x18
+#define JZ4780_DMA_SSI1_RX	0x19
+#define JZ4780_DMA_MSC0_TX	0x1a
+#define JZ4780_DMA_MSC0_RX	0x1b
+#define JZ4780_DMA_MSC1_TX	0x1c
+#define JZ4780_DMA_MSC1_RX	0x1d
+#define JZ4780_DMA_MSC2_TX	0x1e
+#define JZ4780_DMA_MSC2_RX	0x1f
+#define JZ4780_DMA_PCM0_TX	0x20
+#define JZ4780_DMA_PCM0_RX	0x21
+#define JZ4780_DMA_SMB0_TX	0x24
+#define JZ4780_DMA_SMB0_RX	0x25
+#define JZ4780_DMA_SMB1_TX	0x26
+#define JZ4780_DMA_SMB1_RX	0x27
+#define JZ4780_DMA_SMB2_TX	0x28
+#define JZ4780_DMA_SMB2_RX	0x29
+#define JZ4780_DMA_SMB3_TX	0x2a
+#define JZ4780_DMA_SMB3_RX	0x2b
+#define JZ4780_DMA_SMB4_TX	0x2c
+#define JZ4780_DMA_SMB4_RX	0x2d
+#define JZ4780_DMA_DES_TX	0x2e
+#define JZ4780_DMA_DES_RX	0x2f
+
+#endif /* __DT_BINDINGS_DMA_JZ4780_DMA_H__ */
-- 
2.16.2

Re: [PATCH 11/14] MIPS: dts: jz4780: Add DMA controller node to the devicetree

[James Hogan]

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

On Fri, Mar 09, 2018 at 12:12:16PM -0300, Ezequiel Garcia wrote:
> +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
...
> +	dma: dma@13420000 {
> +		compatible = "ingenic,jz4780-dma";
> +		reg = <0x13420000 0x10000>;
> +		#dma-cells = <2>;
> +
> +		interrupt-parent = <&intc>;
> +		interrupts = <10>;
> +
> +		clocks = <&cgu JZ4780_CLK_PDMA>;
> +
> +		status = "disabled";
> +	};

Is there any point in disabling the DMAC by default? I thought that was
mainly used for peripheral devices which might not be brought out of the
SoC on all boards.

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 11/14] MIPS: dts: jz4780: Add DMA controller node to the devicetree

[Ezequiel Garcia]

On 9 March 2018 at 20:13, James Hogan <jhogan@kernel.org> wrote:
> On Fri, Mar 09, 2018 at 12:12:16PM -0300, Ezequiel Garcia wrote:
>> +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
> ...
>> +     dma: dma@13420000 {
>> +             compatible = "ingenic,jz4780-dma";
>> +             reg = <0x13420000 0x10000>;
>> +             #dma-cells = <2>;
>> +
>> +             interrupt-parent = <&intc>;
>> +             interrupts = <10>;
>> +
>> +             clocks = <&cgu JZ4780_CLK_PDMA>;
>> +
>> +             status = "disabled";
>> +     };
>
> Is there any point in disabling the DMAC by default? I thought that was
> mainly used for peripheral devices which might not be brought out of the
> SoC on all boards.
>

Oh, yes, you are right. I'll fix that.

Thanks,
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

[PATCH 12/14] MIPS: dts: jz4780: Add MMC controller node to the devicetree

[Ezequiel Garcia]

This commit adds the devicetree node to support the MMC
host controller available in JZ480 SoCs.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 arch/mips/boot/dts/ingenic/jz4780.dtsi | 40 ++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index fd2ef820fbe3..286c3b80d9af 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -255,6 +255,46 @@
 		status = "disabled";
 	};
 
+	mmc0: mmc@13450000 {
+		compatible = "ingenic,jz4780-mmc";
+		reg = <0x13450000 0x1000>;
+
+		interrupt-parent = <&intc>;
+		interrupts = <37>;
+
+		clocks = <&cgu JZ4780_CLK_MSC0>;
+		clock-names = "mmc";
+
+		cap-sd-highspeed;
+		cap-mmc-highspeed;
+		cap-sdio-irq;
+		dmas = <&dma JZ4780_DMA_MSC0_RX 0xffffffff>,
+		       <&dma JZ4780_DMA_MSC0_TX 0xffffffff>;
+		dma-names = "rx", "tx";
+
+		status = "disabled";
+	};
+
+	mmc1: mmc@13460000 {
+		compatible = "ingenic,jz4780-mmc";
+		reg = <0x13460000 0x1000>;
+
+		interrupt-parent = <&intc>;
+		interrupts = <36>;
+
+		clocks = <&cgu JZ4780_CLK_MSC1>;
+		clock-names = "mmc";
+
+		cap-sd-highspeed;
+		cap-mmc-highspeed;
+		cap-sdio-irq;
+		dmas = <&dma JZ4780_DMA_MSC1_RX 0xffffffff>,
+		       <&dma JZ4780_DMA_MSC1_TX 0xffffffff>;
+		dma-names = "rx", "tx";
+
+		status = "disabled";
+	};
+
 	bch: bch@134d0000 {
 		compatible = "ingenic,jz4780-bch";
 		reg = <0x134d0000 0x10000>;
-- 
2.16.2

Re: [PATCH 12/14] MIPS: dts: jz4780: Add MMC controller node to the devicetree

[James Hogan]

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

On Fri, Mar 09, 2018 at 12:12:17PM -0300, Ezequiel Garcia wrote:
> This commit adds the devicetree node to support the MMC
> host controller available in JZ480 SoCs.

Nit: Please don't refer to "this commit" in commit messages. It is
preferable to use the imperative mood, e.g.:
> Add the devicetree node to support the MMC host controller available
> in JZ480 SoCs.

Otherwise

Acked-by: James Hogan <jhogan@kernel.org>

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 13/14] MIPS: dts: ci20: Enable DMA and MMC in the devicetree

[Ezequiel Garcia]

Now that we have support for JZ480 SoCs in the MMC driver,
let's enable it on the devicetree.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 arch/mips/boot/dts/ingenic/ci20.dts | 38 +++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts
index a4cc52214dbd..9c5261dbcc4e 100644
--- a/arch/mips/boot/dts/ingenic/ci20.dts
+++ b/arch/mips/boot/dts/ingenic/ci20.dts
@@ -36,6 +36,32 @@
 	clock-frequency = <48000000>;
 };
 
+&dma {
+	status = "okay";
+};
+
+&mmc0 {
+	status = "okay";
+
+	bus-width = <4>;
+	max-frequency = <50000000>;
+
+	pinctrl-names = "default";
+	pinctrl-0 = <&pins_mmc0>;
+
+	cd-gpios = <&gpf 20 GPIO_ACTIVE_LOW>;
+};
+
+&mmc1 {
+	status = "okay";
+
+	bus-width = <4>;
+	max-frequency = <50000000>;
+
+	pinctrl-names = "default";
+	pinctrl-0 = <&pins_mmc1>;
+};
+
 &uart0 {
 	status = "okay";
 
@@ -203,4 +229,16 @@
 		groups = "nemc-cs6";
 		bias-disable;
 	};
+
+	pins_mmc0: mmc0 {
+		function = "mmc0";
+		groups = "mmc0-1bit-e", "mmc0-4bit-e";
+		bias-disable;
+	};
+
+	pins_mmc1: mmc1 {
+		function = "mmc1";
+		groups = "mmc1-1bit-d", "mmc1-4bit-d";
+		bias-disable;
+	};
 };
-- 
2.16.2

Re: [PATCH 13/14] MIPS: dts: ci20: Enable DMA and MMC in the devicetree

[James Hogan]

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

On Fri, Mar 09, 2018 at 12:12:18PM -0300, Ezequiel Garcia wrote:
> Now that we have support for JZ480 SoCs in the MMC driver,
> let's enable it on the devicetree.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
>  arch/mips/boot/dts/ingenic/ci20.dts | 38 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts
> index a4cc52214dbd..9c5261dbcc4e 100644
> --- a/arch/mips/boot/dts/ingenic/ci20.dts
> +++ b/arch/mips/boot/dts/ingenic/ci20.dts
> @@ -36,6 +36,32 @@
>  	clock-frequency = <48000000>;
>  };
>  
> +&dma {
> +	status = "okay";
> +};

With that removed, it looks good to me in principle:
Acked-by: James Hogan <jhogan@kernel.org>

Thanks
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 14/14] MIPS: configs: ci20: Enable DMA and MMC support

[Ezequiel Garcia]

This commit enables the SD/MMC support, along with DMA
engine support in the CI20 defconfig.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 arch/mips/configs/ci20_defconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/mips/configs/ci20_defconfig b/arch/mips/configs/ci20_defconfig
index b5f4ad8f2c45..f88b05fd3077 100644
--- a/arch/mips/configs/ci20_defconfig
+++ b/arch/mips/configs/ci20_defconfig
@@ -104,8 +104,11 @@ CONFIG_REGULATOR_FIXED_VOLTAGE=y
 # CONFIG_HID is not set
 # CONFIG_USB_SUPPORT is not set
 CONFIG_MMC=y
+CONFIG_MMC_JZ4740=y
 CONFIG_RTC_CLASS=y
 CONFIG_RTC_DRV_JZ4740=y
+CONFIG_DMADEVICES=y
+CONFIG_DMA_JZ4780=y
 # CONFIG_IOMMU_SUPPORT is not set
 CONFIG_MEMORY=y
 # CONFIG_DNOTIFY is not set
-- 
2.16.2

Re: [PATCH 14/14] MIPS: configs: ci20: Enable DMA and MMC support

[James Hogan]

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

On Fri, Mar 09, 2018 at 12:12:19PM -0300, Ezequiel Garcia wrote:
> This commit enables the SD/MMC support, along with DMA
> engine support in the CI20 defconfig.

Please reword to use the imperative mood, e.g. something like:
> Enable SD/MMC support, along with DMA engine support in the CI20
> defconfig.

Otherwise:
Acked-by: James Hogan <jhogan@kernel.org>

Thanks
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 00/14] Enable SD/MMC on JZ4780 SoCs

[Mathieu Malaterre]

On Fri, Mar 9, 2018 at 4:12 PM, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
> This patchset adds support for SD/MMC on JZ4780 based
> platforms, such as the MIPS Creator CI20 board.
>
> Most of the work has been done by Alex, Paul and Zubair,
> while I've only prepared the upstream submission, cleaned
> some patches, and written some commit logs where needed.
>
> All praises should go to them, all rants to me.
>
> The series is based on v4.16-rc4.
>
> Alex Smith (3):
>   mmc: jz4740: Set clock rate to mmc->f_max rather than JZ_MMC_CLK_RATE
>   mmc: jz4740: Add support for the JZ4780
>   mmc: jz4740: Fix race condition in IRQ mask update
>
> Ezequiel Garcia (9):
>   mmc: jz4780: Order headers alphabetically
>   mmc: jz4740: Use dev_get_platdata
>   mmc: jz4740: Introduce devicetree probe
>   mmc: dt-bindings: add MMC support to JZ4740 SoC
>   mmc: jz4740: Use dma_request_chan()
>   MIPS: dts: jz4780: Add DMA controller node to the devicetree
>   MIPS: dts: jz4780: Add MMC controller node to the devicetree
>   MIPS: dts: ci20: Enable DMA and MMC in the devicetree
>   MIPS: configs: ci20: Enable DMA and MMC support
>
> Paul Cercueil (1):
>   mmc: jz4740: Fix error exit path in driver's probe
>
> Zubair Lutfullah Kakakhel (1):
>   mmc: jz4740: Reset the device requesting the interrupt

Nice work. Entire series works just fine on my MIPS Creator CI20 (v1).

Nitpick: could you update the email addresses:

s/imgtec/mips/

thanks

>  Documentation/devicetree/bindings/mmc/jz4740.txt |  38 ++++
>  arch/mips/boot/dts/ingenic/ci20.dts              |  38 ++++
>  arch/mips/boot/dts/ingenic/jz4780.dtsi           |  54 ++++++
>  arch/mips/configs/ci20_defconfig                 |   3 +
>  drivers/mmc/host/Kconfig                         |   2 +-
>  drivers/mmc/host/jz4740_mmc.c                    | 232 ++++++++++++++++-------
>  include/dt-bindings/dma/jz4780-dma.h             |  49 +++++
>  7 files changed, 349 insertions(+), 67 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mmc/jz4740.txt
>  create mode 100644 include/dt-bindings/dma/jz4780-dma.h
>
> --
> 2.16.2
>
>

Re: [PATCH 00/14] Enable SD/MMC on JZ4780 SoCs

[Ezequiel Garcia]

Hi Mathieu,

On 10 March 2018 at 14:02, Mathieu Malaterre <malat@debian.org> wrote:
> On Fri, Mar 9, 2018 at 4:12 PM, Ezequiel Garcia
> <ezequiel@vanguardiasur.com.ar> wrote:
>> This patchset adds support for SD/MMC on JZ4780 based
>> platforms, such as the MIPS Creator CI20 board.
>>
>> Most of the work has been done by Alex, Paul and Zubair,
>> while I've only prepared the upstream submission, cleaned
>> some patches, and written some commit logs where needed.
>>
>> All praises should go to them, all rants to me.
>>
>> The series is based on v4.16-rc4.
>>
>> Alex Smith (3):
>>   mmc: jz4740: Set clock rate to mmc->f_max rather than JZ_MMC_CLK_RATE
>>   mmc: jz4740: Add support for the JZ4780
>>   mmc: jz4740: Fix race condition in IRQ mask update
>>
>> Ezequiel Garcia (9):
>>   mmc: jz4780: Order headers alphabetically
>>   mmc: jz4740: Use dev_get_platdata
>>   mmc: jz4740: Introduce devicetree probe
>>   mmc: dt-bindings: add MMC support to JZ4740 SoC
>>   mmc: jz4740: Use dma_request_chan()
>>   MIPS: dts: jz4780: Add DMA controller node to the devicetree
>>   MIPS: dts: jz4780: Add MMC controller node to the devicetree
>>   MIPS: dts: ci20: Enable DMA and MMC in the devicetree
>>   MIPS: configs: ci20: Enable DMA and MMC support
>>
>> Paul Cercueil (1):
>>   mmc: jz4740: Fix error exit path in driver's probe
>>
>> Zubair Lutfullah Kakakhel (1):
>>   mmc: jz4740: Reset the device requesting the interrupt
>
> Nice work. Entire series works just fine on my MIPS Creator CI20 (v1).
>

Cool. This means a Tested-by for the entire series?

> Nitpick: could you update the email addresses:
>
> s/imgtec/mips/
>

You sure that is appropriate? First of all, the work has done
under Imagination Technologies umbrella (even if now the
developers work for MIPS).

And second, I'd feel better if such request would come
from the authors, or at least acked by them.

Thanks for testing!
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 00/14] Enable SD/MMC on JZ4780 SoCs

[James Hogan]

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

On Sat, Mar 10, 2018 at 07:17:43PM -0300, Ezequiel Garcia wrote:
> Hi Mathieu,
> 
> On 10 March 2018 at 14:02, Mathieu Malaterre <malat@debian.org> wrote:
> > On Fri, Mar 9, 2018 at 4:12 PM, Ezequiel Garcia
> > <ezequiel@vanguardiasur.com.ar> wrote:
> >> This patchset adds support for SD/MMC on JZ4780 based
> >> platforms, such as the MIPS Creator CI20 board.
> >>
> >> Most of the work has been done by Alex, Paul and Zubair,
> >> while I've only prepared the upstream submission, cleaned
> >> some patches, and written some commit logs where needed.
> >>
> >> All praises should go to them, all rants to me.
> >>
> >> The series is based on v4.16-rc4.
> >>
> >> Alex Smith (3):
> >>   mmc: jz4740: Set clock rate to mmc->f_max rather than JZ_MMC_CLK_RATE
> >>   mmc: jz4740: Add support for the JZ4780
> >>   mmc: jz4740: Fix race condition in IRQ mask update
> >>
> >> Ezequiel Garcia (9):
> >>   mmc: jz4780: Order headers alphabetically
> >>   mmc: jz4740: Use dev_get_platdata
> >>   mmc: jz4740: Introduce devicetree probe
> >>   mmc: dt-bindings: add MMC support to JZ4740 SoC
> >>   mmc: jz4740: Use dma_request_chan()
> >>   MIPS: dts: jz4780: Add DMA controller node to the devicetree
> >>   MIPS: dts: jz4780: Add MMC controller node to the devicetree
> >>   MIPS: dts: ci20: Enable DMA and MMC in the devicetree
> >>   MIPS: configs: ci20: Enable DMA and MMC support
> >>
> >> Paul Cercueil (1):
> >>   mmc: jz4740: Fix error exit path in driver's probe
> >>
> >> Zubair Lutfullah Kakakhel (1):
> >>   mmc: jz4740: Reset the device requesting the interrupt
> >
> > Nice work. Entire series works just fine on my MIPS Creator CI20 (v1).
> >
> 
> Cool. This means a Tested-by for the entire series?
> 
> > Nitpick: could you update the email addresses:
> >
> > s/imgtec/mips/
> >
> 
> You sure that is appropriate? First of all, the work has done
> under Imagination Technologies umbrella (even if now the
> developers work for MIPS).

Yeh, I don't think there's much to be gained. And Alex would have been
on a university placement at IMG (before MIPS was sold) to use that
email address anyway.

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 00/14] Enable SD/MMC on JZ4780 SoCs

[Mathieu Malaterre]

On Sat, Mar 10, 2018 at 11:17 PM, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
> Hi Mathieu,
>
> On 10 March 2018 at 14:02, Mathieu Malaterre <malat@debian.org> wrote:
>> On Fri, Mar 9, 2018 at 4:12 PM, Ezequiel Garcia
>> <ezequiel@vanguardiasur.com.ar> wrote:
>>> This patchset adds support for SD/MMC on JZ4780 based
>>> platforms, such as the MIPS Creator CI20 board.
>>>
>>> Most of the work has been done by Alex, Paul and Zubair,
>>> while I've only prepared the upstream submission, cleaned
>>> some patches, and written some commit logs where needed.
>>>
>>> All praises should go to them, all rants to me.
>>>
>>> The series is based on v4.16-rc4.
>>>
>>> Alex Smith (3):
>>>   mmc: jz4740: Set clock rate to mmc->f_max rather than JZ_MMC_CLK_RATE
>>>   mmc: jz4740: Add support for the JZ4780
>>>   mmc: jz4740: Fix race condition in IRQ mask update
>>>
>>> Ezequiel Garcia (9):
>>>   mmc: jz4780: Order headers alphabetically
>>>   mmc: jz4740: Use dev_get_platdata
>>>   mmc: jz4740: Introduce devicetree probe
>>>   mmc: dt-bindings: add MMC support to JZ4740 SoC
>>>   mmc: jz4740: Use dma_request_chan()
>>>   MIPS: dts: jz4780: Add DMA controller node to the devicetree
>>>   MIPS: dts: jz4780: Add MMC controller node to the devicetree
>>>   MIPS: dts: ci20: Enable DMA and MMC in the devicetree
>>>   MIPS: configs: ci20: Enable DMA and MMC support
>>>
>>> Paul Cercueil (1):
>>>   mmc: jz4740: Fix error exit path in driver's probe
>>>
>>> Zubair Lutfullah Kakakhel (1):
>>>   mmc: jz4740: Reset the device requesting the interrupt
>>
>> Nice work. Entire series works just fine on my MIPS Creator CI20 (v1).
>>
>
> Cool. This means a Tested-by for the entire series?

Right, that was not clear, so:

Tested-by: Mathieu Malaterre <malat@debian.org>

for the entire series. thanks !

>> Nitpick: could you update the email addresses:
>>
>> s/imgtec/mips/
>>
>
> You sure that is appropriate? First of all, the work has done
> under Imagination Technologies umbrella (even if now the
> developers work for MIPS).
>
> And second, I'd feel better if such request would come
> from the authors, or at least acked by them.

I did not realize Alex was a special case. So please discard, no need
to update Alex email.

> Thanks for testing!
> --
> Ezequiel García, VanguardiaSur
> www.vanguardiasur.com.ar