Skip to contentSkip to conversation

mmc: sdhci-pxav2: Add support for PXA168

PATCH Superseded Doug Brown 4 participants 15 messages Nov 28, 2022 – Dec 1, 2022

Subsystems (3)

v1 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Doug Brown

author
#0 lore raw

cover mmc: sdhci-pxav2: Add support for PXA168

This is a revival of an earlier patch series from 2013 to add support for the PXA168 SDHC controller, with an additional SDIO IRQ errata fix. It also cleans up the clock naming to be consistent with the existing DT schema shared with the pxav3 driver (in a backwards-compatible way).

Here is the original patch series this is based on:
https://lore.kernel.org/linux-mmc/[email protected]/

Note that I left out the platform_specific_completion and clock gating changes from the original patches. They both seemed controversial, and don't seem necessary based on my testing. I've been running this code on a PXA168 for months without any issues.

The SDIO IRQ workaround of sending a dummy CMD0 isn't pretty, but it works just as the errata said it would. Should I export sdhci_post_req and refer to it directly, rather than saving a pointer to it in this driver like I'm doing now? Or is there a cleaner way to send the dummy command after every SDIO command? It felt hacky, but I wasn't sure how else to do it.

Doug Brown (8):
mmc: sdhci-pxav2: add initial support for PXA168 V1 controller
mmc: sdhci-pxav2: enable CONFIG_MMC_SDHCI_IO_ACCESSORS
mmc: sdhci-pxav2: add register workaround for PXA168 silicon bug
mmc: sdhci-pxav2: change clock name to match DT bindings
mmc: sdhci-pxav2: add optional core clock
mmc: sdhci-pxav2: add SDIO card IRQ workaround for PXA168 V1
controller
mmc: sdhci-pxav2: add optional pinctrl for SDIO IRQ workaround
dt-bindings: mmc: sdhci-pxa: add pxav1

 .../devicetree/bindings/mmc/sdhci-pxa.yaml    |  22 ++-
 drivers/mmc/host/Kconfig                      |   1 +
 drivers/mmc/host/sdhci-pxav2.c                | 138 +++++++++++++++++-
 3 files changed, 154 insertions(+), 7 deletions(-)
Signature · 1 line
2.34.1
author
#1 lore raw

PATCH 1/8 mmc: sdhci-pxav2: add initial support for PXA168 V1 controller

Add a new compatible string for the version 1 controller used in the PXA168, along with necessary quirks. Use a separate ops struct in preparation for a silicon bug workaround only necessary on V1.

---
 drivers/mmc/host/sdhci-pxav2.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)
drivers/mmc/host/sdhci-pxav2.c +17 -1
index f18906b5575f..2f9fa0ecbddd 100644
@@ -101,6 +101,14 @@ static void pxav2_mmc_set_bus_width(struct sdhci_host *host, int width)
 	writeb(ctrl, host->ioaddr + SDHCI_HOST_CONTROL);
 }
 
+static const struct sdhci_ops pxav1_sdhci_ops = {
+	.set_clock     = sdhci_set_clock,
+	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
+	.set_bus_width = pxav2_mmc_set_bus_width,
+	.reset         = pxav2_reset,
+	.set_uhs_signaling = sdhci_set_uhs_signaling,
+};
+
 static const struct sdhci_ops pxav2_sdhci_ops = {
 	.set_clock     = sdhci_set_clock,
 	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
@@ -114,6 +122,9 @@ static const struct of_device_id sdhci_pxav2_of_match[] = {
 	{
 		.compatible = "mrvl,pxav2-mmc",
 	},
+	{
+		.compatible = "mrvl,pxav1-mmc",
+	},
 	{},
 };
 MODULE_DEVICE_TABLE(of, sdhci_pxav2_of_match);
@@ -208,7 +219,12 @@ static int sdhci_pxav2_probe(struct platform_device *pdev)
 			host->mmc->pm_caps |= pdata->pm_caps;
 	}
 
-	host->ops = &pxav2_sdhci_ops;
+	if (match && of_device_is_compatible(dev->of_node, "mrvl,pxav1-mmc")) {
+		host->quirks |= SDHCI_QUIRK_NO_BUSY_IRQ | SDHCI_QUIRK_32BIT_DMA_SIZE;
+		host->ops = &pxav1_sdhci_ops;
+	} else {
+		host->ops = &pxav2_sdhci_ops;
+	}
 
 	ret = sdhci_add_host(host);
 	if (ret)
Signature · 1 line
2.34.1
author
#2 lore raw

PATCH 2/8 mmc: sdhci-pxav2: enable CONFIG_MMC_SDHCI_IO_ACCESSORS

In reply to Doug Brown

Enable CONFIG_MMC_SDHCI_IO_ACCESSORS for the pxav2 driver. The read_w callback is needed for a silicon bug workaround in the PXA168.

---
 drivers/mmc/host/Kconfig | 1 +
 1 file changed, 1 insertion(+)
drivers/mmc/host/Kconfig +1 -0
index 5e19a961c34d..b9e9185c86a6 100644
@@ -360,6 +360,7 @@ config MMC_SDHCI_PXAV2
 	depends on MMC_SDHCI_PLTFM
 	depends on ARCH_MMP || COMPILE_TEST
 	default CPU_PXA910
+	select MMC_SDHCI_IO_ACCESSORS
 	help
 	  This selects the Marvell(R) PXAV2 SD Host Controller.
 	  If you have a PXA9XX platform with SD Host Controller
Signature · 1 line
2.34.1
author
#3 lore raw

PATCH 3/8 mmc: sdhci-pxav2: add register workaround for PXA168 silicon bug

In reply to Doug Brown

The PXA168 has a documented silicon bug that results in a data abort exception when accessing the SDHCI_HOST_VERSION register on SDH2 and SDH4 through a 16-bit read. Implement the workaround described in the errata, which performs a 32-bit read from a lower address instead. This is safe to use on all four SDH peripherals.

---
 drivers/mmc/host/sdhci-pxav2.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)
drivers/mmc/host/sdhci-pxav2.c +13 -0
index 2f9fa0ecbddd..d76131b8986b 100644
@@ -80,6 +80,18 @@ static void pxav2_reset(struct sdhci_host *host, u8 mask)
 	}
 }
 
+static inline u16 pxav1_readw(struct sdhci_host *host, int reg)
+{
+	u32 temp;
+	/* Workaround for data abort exception on SDH2 and SDH4 on PXA168 */
+	if (reg == SDHCI_HOST_VERSION) {
+		temp = readl(host->ioaddr + SDHCI_HOST_VERSION - 2) >> 16;
+		return temp & 0xffff;
+	}
+
+	return readw(host->ioaddr + reg);
+}
+
 static void pxav2_mmc_set_bus_width(struct sdhci_host *host, int width)
 {
 	u8 ctrl;
@@ -102,6 +114,7 @@ static void pxav2_mmc_set_bus_width(struct sdhci_host *host, int width)
 }
 
 static const struct sdhci_ops pxav1_sdhci_ops = {
+	.read_w        = pxav1_readw,
 	.set_clock     = sdhci_set_clock,
 	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
 	.set_bus_width = pxav2_mmc_set_bus_width,
Signature · 1 line
2.34.1
maintainer
#4 lore raw

Re: [PATCH 3/8] mmc: sdhci-pxav2: add register workaround for PXA168 silicon bug

Quoted 16 lines · Doug Brown

The PXA168 has a documented silicon bug that results in a data abort exception when accessing the SDHCI_HOST_VERSION register on SDH2 and SDH4 through a 16-bit read. Implement the workaround described in the errata, which performs a 32-bit read from a lower address instead. This is safe to use on all four SDH peripherals.

---
 drivers/mmc/host/sdhci-pxav2.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)
drivers/mmc/host/sdhci-pxav2.c +0 -0
index 2f9fa0ecbddd..d76131b8986b 100644

drivers/mmc/host/sdhci-pxav2.c static void pxav2_reset(struct sdhci_host *host, u8 mask) · line 89

Quoted 10 lines
drivers/mmc/host/sdhci-pxav2.c +7 -0
@@ -80,6 +80,18 @@ static void pxav2_reset(struct sdhci_host *host, u8 mask)
 	}
 }
 
+static inline u16 pxav1_readw(struct sdhci_host *host, int reg)
+{
+	u32 temp;
+	/* Workaround for data abort exception on SDH2 and SDH4 on PXA168 */
+	if (reg == SDHCI_HOST_VERSION) {
+		temp = readl(host->ioaddr + SDHCI_HOST_VERSION - 2) >> 16;
+		return temp & 0xffff;

Isn't this the same as just:

		return readl(host->ioaddr + SDHCI_HOST_VERSION - 2) >> 16;
Quoted 16 lines
drivers/mmc/host/sdhci-pxav2.c +6 -0
+	}
+
+	return readw(host->ioaddr + reg);
+}
+
 static void pxav2_mmc_set_bus_width(struct sdhci_host *host, int width)
 {
 	u8 ctrl;
@@ -102,6 +114,7 @@ static void pxav2_mmc_set_bus_width(struct sdhci_host *host, int width)
 }
 
 static const struct sdhci_ops pxav1_sdhci_ops = {
+	.read_w        = pxav1_readw,
 	.set_clock     = sdhci_set_clock,
 	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
 	.set_bus_width = pxav2_mmc_set_bus_width,
author
#5 lore raw

PATCH 4/8 mmc: sdhci-pxav2: change clock name to match DT bindings

In reply to Doug Brown

The devicetree bindings for this driver specify that the two allowed clock names are io and core. Change this driver to look for io, but allow any name if it fails for backwards compatibility. Follow the same pattern used in sdhci-pxav3.

---
 drivers/mmc/host/sdhci-pxav2.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/mmc/host/sdhci-pxav2.c +3 -1
index d76131b8986b..a0c882d03d0b 100644
@@ -192,7 +192,9 @@ static int sdhci_pxav2_probe(struct platform_device *pdev)
 
 	pltfm_host = sdhci_priv(host);
 
-	clk = devm_clk_get(dev, "PXA-SDHCLK");
+	clk = devm_clk_get(dev, "io");
+	if (IS_ERR(clk))
+		clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(clk)) {
 		dev_err(dev, "failed to get io clock\n");
 		ret = PTR_ERR(clk);
Signature · 1 line
2.34.1
author
#6 lore raw

PATCH 5/8 mmc: sdhci-pxav2: add optional core clock

In reply to Doug Brown

Add ability to have an optional core clock just like the pxav3 driver. The PXA168 needs this because its SDHC controllers have separate core and io clocks that both need to be enabled. This also correctly matches the documented devicetree bindings for this driver.

---
 drivers/mmc/host/sdhci-pxav2.c | 40 ++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)
drivers/mmc/host/sdhci-pxav2.c +36 -4
index a0c882d03d0b..4996d72c6d23 100644
@@ -41,6 +41,10 @@
 #define MMC_CARD		0x1000
 #define MMC_WIDTH		0x0100
 
+struct sdhci_pxav2_host {
+	struct clk *clk_core;
+};
+
 static void pxav2_reset(struct sdhci_host *host, u8 mask)
 {
 	struct platform_device *pdev = to_platform_device(mmc_dev(host->mmc));
@@ -179,6 +183,7 @@ static int sdhci_pxav2_probe(struct platform_device *pdev)
 {
 	struct sdhci_pltfm_host *pltfm_host;
 	struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data;
+	struct sdhci_pxav2_host *pxav2_host;
 	struct device *dev = &pdev->dev;
 	struct sdhci_host *host = NULL;
 	const struct of_device_id *match;
@@ -186,11 +191,12 @@ static int sdhci_pxav2_probe(struct platform_device *pdev)
 	int ret;
 	struct clk *clk;
 
-	host = sdhci_pltfm_init(pdev, NULL, 0);
+	host = sdhci_pltfm_init(pdev, NULL, sizeof(*pxav2_host));
 	if (IS_ERR(host))
 		return PTR_ERR(host);
 
 	pltfm_host = sdhci_priv(host);
+	pxav2_host = sdhci_pltfm_priv(pltfm_host);
 
 	clk = devm_clk_get(dev, "io");
 	if (IS_ERR(clk))
@@ -207,6 +213,15 @@ static int sdhci_pxav2_probe(struct platform_device *pdev)
 		goto free;
 	}
 
+	pxav2_host->clk_core = devm_clk_get(dev, "core");
+	if (!IS_ERR(pxav2_host->clk_core)) {
+		ret = clk_prepare_enable(pxav2_host->clk_core);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to enable core clock\n");
+			goto disable_io_clk;
+		}
+	}
+
 	host->quirks = SDHCI_QUIRK_BROKEN_ADMA
 		| SDHCI_QUIRK_BROKEN_TIMEOUT_VAL
 		| SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN;
@@ -243,17 +258,34 @@ static int sdhci_pxav2_probe(struct platform_device *pdev)
 
 	ret = sdhci_add_host(host);
 	if (ret)
-		goto disable_clk;
+		goto disable_core_clk;
 
 	return 0;
 
-disable_clk:
+disable_core_clk:
+	if (!IS_ERR(pxav2_host->clk_core))
+		clk_disable_unprepare(pxav2_host->clk_core);
+disable_io_clk:
 	clk_disable_unprepare(clk);
 free:
 	sdhci_pltfm_free(pdev);
 	return ret;
 }
 
+static int sdhci_pxav2_remove(struct platform_device *pdev)
+{
+	struct sdhci_host *host = platform_get_drvdata(pdev);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_pxav2_host *pxav2_host = sdhci_pltfm_priv(pltfm_host);
+
+	int ret = sdhci_pltfm_unregister(pdev);
+
+	if (!IS_ERR(pxav2_host->clk_core))
+		clk_disable_unprepare(pxav2_host->clk_core);
+
+	return ret;
+}
+
 static struct platform_driver sdhci_pxav2_driver = {
 	.driver		= {
 		.name	= "sdhci-pxav2",
@@ -262,7 +294,7 @@ static struct platform_driver sdhci_pxav2_driver = {
 		.pm	= &sdhci_pltfm_pmops,
 	},
 	.probe		= sdhci_pxav2_probe,
-	.remove		= sdhci_pltfm_unregister,
+	.remove		= sdhci_pxav2_remove,
 };
 
 module_platform_driver(sdhci_pxav2_driver);
Signature · 1 line
2.34.1
author
#7 lore raw

PATCH 6/8 mmc: sdhci-pxav2: add SDIO card IRQ workaround for PXA168 V1 controller

In reply to Doug Brown

The PXA168 has a documented silicon bug that causes SDIO card IRQs to be missed. Implement the first half of the suggested workaround, which involves resetting the data port logic and issuing a dummy CMD0 to restart the clock.

---
 drivers/mmc/host/sdhci-pxav2.c | 36 ++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)
drivers/mmc/host/sdhci-pxav2.c +36 -0
index 4996d72c6d23..0b9b2e4b2153 100644
@@ -20,6 +20,8 @@
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/mmc/sdio.h>
+#include <linux/mmc/mmc.h>
 
 #include "sdhci.h"
 #include "sdhci-pltfm.h"
@@ -43,6 +45,7 @@
 
 struct sdhci_pxav2_host {
 	struct clk *clk_core;
+	void (*orig_post_req)(struct mmc_host *mmc, struct mmc_request *mrq, int err);
 };
 
 static void pxav2_reset(struct sdhci_host *host, u8 mask)
@@ -96,6 +99,37 @@ static inline u16 pxav1_readw(struct sdhci_host *host, int reg)
 	return readw(host->ioaddr + reg);
 }
 
+static void pxav1_post_req(struct mmc_host *mmc, struct mmc_request *mrq, int err)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	struct sdhci_pxav2_host *pxav2_host;
+	struct mmc_command dummy_cmd = {};
+	u16 tmp;
+
+	/* If this is an SDIO command, perform errata workaround for silicon bug. */
+	if (!err && mrq->cmd && !mrq->cmd->error &&
+	    (mrq->cmd->opcode == SD_IO_RW_DIRECT ||
+	    mrq->cmd->opcode == SD_IO_RW_EXTENDED)) {
+		/* Reset data port */
+		tmp = readw(host->ioaddr + SDHCI_TIMEOUT_CONTROL);
+		tmp |= 0x400;
+		writew(tmp, host->ioaddr + SDHCI_TIMEOUT_CONTROL);
+
+		/* Clock is now stopped, so restart it by sending a dummy CMD0. */
+		pxav2_host = sdhci_pltfm_priv(sdhci_priv(host));
+
+		dummy_cmd.opcode = MMC_GO_IDLE_STATE;
+		dummy_cmd.arg = 0;
+		dummy_cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_NONE | MMC_CMD_BC;
+
+		mmc_wait_for_cmd(host->mmc, &dummy_cmd, 0);
+	}
+
+	/* Pass onto SDHCI host driver now */
+	if (pxav2_host->orig_post_req)
+		pxav2_host->orig_post_req(mmc, mrq, err);
+}
+
 static void pxav2_mmc_set_bus_width(struct sdhci_host *host, int width)
 {
 	u8 ctrl;
@@ -252,6 +286,8 @@ static int sdhci_pxav2_probe(struct platform_device *pdev)
 	if (match && of_device_is_compatible(dev->of_node, "mrvl,pxav1-mmc")) {
 		host->quirks |= SDHCI_QUIRK_NO_BUSY_IRQ | SDHCI_QUIRK_32BIT_DMA_SIZE;
 		host->ops = &pxav1_sdhci_ops;
+		pxav2_host->orig_post_req = host->mmc_host_ops.post_req;
+		host->mmc_host_ops.post_req = pxav1_post_req;
 	} else {
 		host->ops = &pxav2_sdhci_ops;
 	}
Signature · 1 line
2.34.1
maintainer
#8 lore raw

Re: [PATCH 6/8] mmc: sdhci-pxav2: add SDIO card IRQ workaround for PXA168 V1 controller

Quoted 49 lines · Doug Brown

The PXA168 has a documented silicon bug that causes SDIO card IRQs to be missed. Implement the first half of the suggested workaround, which involves resetting the data port logic and issuing a dummy CMD0 to restart the clock.

---
 drivers/mmc/host/sdhci-pxav2.c | 36 ++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)
drivers/mmc/host/sdhci-pxav2.c +17 -0
index 4996d72c6d23..0b9b2e4b2153 100644
@@ -20,6 +20,8 @@
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/mmc/sdio.h>
+#include <linux/mmc/mmc.h>
 
 #include "sdhci.h"
 #include "sdhci-pltfm.h"
@@ -43,6 +45,7 @@
 
 struct sdhci_pxav2_host {
 	struct clk *clk_core;
+	void (*orig_post_req)(struct mmc_host *mmc, struct mmc_request *mrq, int err);
 };
 
 static void pxav2_reset(struct sdhci_host *host, u8 mask)
@@ -96,6 +99,37 @@ static inline u16 pxav1_readw(struct sdhci_host *host, int reg)
 	return readw(host->ioaddr + reg);
 }
 
+static void pxav1_post_req(struct mmc_host *mmc, struct mmc_request *mrq, int err)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	struct sdhci_pxav2_host *pxav2_host;
+	struct mmc_command dummy_cmd = {};
+	u16 tmp;
+
+	/* If this is an SDIO command, perform errata workaround for silicon bug. */
+	if (!err && mrq->cmd && !mrq->cmd->error &&
+	    (mrq->cmd->opcode == SD_IO_RW_DIRECT ||
+	    mrq->cmd->opcode == SD_IO_RW_EXTENDED)) {
+		/* Reset data port */
+		tmp = readw(host->ioaddr + SDHCI_TIMEOUT_CONTROL);
+		tmp |= 0x400;

drivers/mmc/host/sdhci-pxav2.c static inline u16 pxav1_readw(struct sdhci_host *host, int reg) · line 125

Quoted 10 lines
drivers/mmc/host/sdhci-pxav2.c +10 -0
+		writew(tmp, host->ioaddr + SDHCI_TIMEOUT_CONTROL);
+
+		/* Clock is now stopped, so restart it by sending a dummy CMD0. */
+		pxav2_host = sdhci_pltfm_priv(sdhci_priv(host));
+
+		dummy_cmd.opcode = MMC_GO_IDLE_STATE;
+		dummy_cmd.arg = 0;
+		dummy_cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_NONE | MMC_CMD_BC;
+
+		mmc_wait_for_cmd(host->mmc, &dummy_cmd, 0);

This is not what post_req() is for. Instead could you use SDHCI host op ->request_done()? Also, do you really need to wait for the dummy CMD0 - perhaps just write SDHCI_ARGUMENT, SDHCI_TRANSFER_MODE, and SDHCI_COMMAND ?

Quoted 19 lines
drivers/mmc/host/sdhci-pxav2.c +9 -0
+	}
+
+	/* Pass onto SDHCI host driver now */
+	if (pxav2_host->orig_post_req)
+		pxav2_host->orig_post_req(mmc, mrq, err);
+}
+
 static void pxav2_mmc_set_bus_width(struct sdhci_host *host, int width)
 {
 	u8 ctrl;
@@ -252,6 +286,8 @@ static int sdhci_pxav2_probe(struct platform_device *pdev)
 	if (match && of_device_is_compatible(dev->of_node, "mrvl,pxav1-mmc")) {
 		host->quirks |= SDHCI_QUIRK_NO_BUSY_IRQ | SDHCI_QUIRK_32BIT_DMA_SIZE;
 		host->ops = &pxav1_sdhci_ops;
+		pxav2_host->orig_post_req = host->mmc_host_ops.post_req;
+		host->mmc_host_ops.post_req = pxav1_post_req;
 	} else {
 		host->ops = &pxav2_sdhci_ops;
 	}
author
#9 lore raw

Re: [PATCH 6/8] mmc: sdhci-pxav2: add SDIO card IRQ workaround for PXA168 V1 controller

Hi Adrian,

Quoted 15 lines · Adrian Hunter

drivers/mmc/host/sdhci-pxav2.c static inline u16 pxav1_readw(struct sdhci_host *host, int reg) · line 125

Quoted 9 lines · Doug Brown
drivers/mmc/host/sdhci-pxav2.c +9 -0
+
+		/* Clock is now stopped, so restart it by sending a dummy CMD0. */
+		pxav2_host = sdhci_pltfm_priv(sdhci_priv(host));
+
+		dummy_cmd.opcode = MMC_GO_IDLE_STATE;
+		dummy_cmd.arg = 0;
+		dummy_cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_NONE | MMC_CMD_BC;
+
+		mmc_wait_for_cmd(host->mmc, &dummy_cmd, 0);

This is not what post_req() is for. Instead could you use SDHCI host op ->request_done()? Also, do you really need to wait for the dummy CMD0 - perhaps just write SDHCI_ARGUMENT, SDHCI_TRANSFER_MODE, and SDHCI_COMMAND ?

Thanks for the feedback! That makes perfect sense. I do need to know when the dummy CMD0 finishes so I can restore the pinctrl to default in the next patch, but I think I can handle that with the irq() host op, which I need to do anyway so that sdhci_cmd_irq() doesn't get confused when the CMD0 finishes. I'll give that a shot in the next version of the series and will also address your other feedback.

Doug

author
#10 lore raw

PATCH 7/8 mmc: sdhci-pxav2: add optional pinctrl for SDIO IRQ workaround

In reply to Doug Brown

The PXA168 errata recommends that the CMD signal should be detached from the SD bus while performing the dummy CMD0 to restart the clock. Implement this using pinctrl states.

---
 drivers/mmc/host/sdhci-pxav2.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
drivers/mmc/host/sdhci-pxav2.c +27 -0
index 0b9b2e4b2153..6b30f70675e2 100644
@@ -22,6 +22,7 @@
 #include <linux/of_device.h>
 #include <linux/mmc/sdio.h>
 #include <linux/mmc/mmc.h>
+#include <linux/pinctrl/consumer.h>
 
 #include "sdhci.h"
 #include "sdhci-pltfm.h"
@@ -46,6 +47,9 @@
 struct sdhci_pxav2_host {
 	struct clk *clk_core;
 	void (*orig_post_req)(struct mmc_host *mmc, struct mmc_request *mrq, int err);
+	struct pinctrl *pinctrl;
+	struct pinctrl_state *pins_default;
+	struct pinctrl_state *pins_cmd_gpio;
 };
 
 static void pxav2_reset(struct sdhci_host *host, u8 mask)
@@ -118,11 +122,19 @@ static void pxav1_post_req(struct mmc_host *mmc, struct mmc_request *mrq, int er
 		/* Clock is now stopped, so restart it by sending a dummy CMD0. */
 		pxav2_host = sdhci_pltfm_priv(sdhci_priv(host));
 
+		/* Set CMD as high output rather than MMC function while we do CMD0 */
+		if (pxav2_host->pinctrl && pxav2_host->pins_cmd_gpio)
+			pinctrl_select_state(pxav2_host->pinctrl, pxav2_host->pins_cmd_gpio);
+
 		dummy_cmd.opcode = MMC_GO_IDLE_STATE;
 		dummy_cmd.arg = 0;
 		dummy_cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_NONE | MMC_CMD_BC;
 
 		mmc_wait_for_cmd(host->mmc, &dummy_cmd, 0);
+
+		/* Set as MMC function after dummy command is complete */
+		if (pxav2_host->pinctrl && pxav2_host->pins_default)
+			pinctrl_select_state(pxav2_host->pinctrl, pxav2_host->pins_default);
 	}
 
 	/* Pass onto SDHCI host driver now */
@@ -288,6 +300,21 @@ static int sdhci_pxav2_probe(struct platform_device *pdev)
 		host->ops = &pxav1_sdhci_ops;
 		pxav2_host->orig_post_req = host->mmc_host_ops.post_req;
 		host->mmc_host_ops.post_req = pxav1_post_req;
+
+		/* Set up optional pinctrl for PXA168 SDIO IRQ fix */
+		pxav2_host->pinctrl = devm_pinctrl_get(&pdev->dev);
+		if (!IS_ERR(pxav2_host->pinctrl)) {
+			pxav2_host->pins_cmd_gpio = pinctrl_lookup_state(pxav2_host->pinctrl,
+									 "state_cmd_gpio");
+			if (IS_ERR(pxav2_host->pins_cmd_gpio))
+				pxav2_host->pins_cmd_gpio = NULL;
+			pxav2_host->pins_default = pinctrl_lookup_state(pxav2_host->pinctrl,
+									"default");
+			if (IS_ERR(pxav2_host->pins_default))
+				pxav2_host->pins_default = NULL;
+		} else {
+			pxav2_host->pinctrl = NULL;
+		}
 	} else {
 		host->ops = &pxav2_sdhci_ops;
 	}
Signature · 1 line
2.34.1
author
#11 lore raw

PATCH 8/8 dt-bindings: mmc: sdhci-pxa: add pxav1

In reply to Doug Brown

Add a compatible for the pxav1 controller in the PXA168, along with optional pinctrl properties to use for an errata workaround.

---
 .../devicetree/bindings/mmc/sdhci-pxa.yaml    | 22 ++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml +21 -1
index 1c87f4218e18..e3fb34853921 100644
@@ -4,7 +4,7 @@
 $id: http://devicetree.org/schemas/mmc/sdhci-pxa.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Marvell PXA SDHCI v2/v3 bindings
+title: Marvell PXA SDHCI v1/v2/v3 bindings
 
 maintainers:
   - Ulf Hansson <[email protected]>
@@ -34,6 +34,7 @@ allOf:
 properties:
   compatible:
     enum:
+      - mrvl,pxav1-mmc
       - mrvl,pxav2-mmc
       - mrvl,pxav3-mmc
       - marvell,armada-380-sdhci
@@ -61,6 +62,25 @@ properties:
       - const: io
       - const: core
 
+  pinctrl-names:
+    description:
+      Optional for supporting PXA168 SDIO IRQ errata to switch CMD pin between SDIO CMD and
+      GPIO mode.
+    minItems: 2
+    items:
+      - const: default
+      - const: state_cmd_gpio
+
+  pinctrl-0:
+    description:
+      should contain default pinctrl.
+    maxItems: 1
+
+  pinctrl-1:
+    description:
+      should switch CMD pin to GPIO mode as a high output.
+    maxItems: 1
+
   mrvl,clk-delay-cycles:
     description: Specify a number of cycles to delay for tuning.
     $ref: /schemas/types.yaml#/definitions/uint32
Signature · 1 line
2.34.1
#12 lore raw

Re: [PATCH 8/8] dt-bindings: mmc: sdhci-pxa: add pxav1

Quoted 30 lines · Doug Brown

Add a compatible for the pxav1 controller in the PXA168, along with optional pinctrl properties to use for an errata workaround.

---
 .../devicetree/bindings/mmc/sdhci-pxa.yaml    | 22 ++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml +2 -1
index 1c87f4218e18..e3fb34853921 100644
@@ -4,7 +4,7 @@
 $id: http://devicetree.org/schemas/mmc/sdhci-pxa.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Marvell PXA SDHCI v2/v3 bindings
+title: Marvell PXA SDHCI v1/v2/v3 bindings
 
 maintainers:
   - Ulf Hansson <[email protected]>
@@ -34,6 +34,7 @@ allOf:
 properties:
   compatible:
     enum:
+      - mrvl,pxav1-mmc
       - mrvl,pxav2-mmc
       - mrvl,pxav3-mmc
       - marvell,armada-380-sdhci

Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml properties: · line 69

Quoted 8 lines
Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml +5 -0
@@ -61,6 +62,25 @@ properties:
       - const: io
       - const: core
 
+  pinctrl-names:
+    description:
+      Optional for supporting PXA168 SDIO IRQ errata to switch CMD pin between SDIO CMD and
+      GPIO mode.
+    minItems: 2

No need for minItems.

Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml properties: · line 76

Quoted 7 lines
Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml +7 -0
+    items:
+      - const: default
+      - const: state_cmd_gpio
+
+  pinctrl-0:
+    description:
+      should contain default pinctrl.

Start with capital letter if this is a sentence with full stop. It's anyway looking different then the rest of the file, right?

Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml properties: · line 77

Quoted 1 line
Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml +1 -0
+    maxItems: 1

Why maxItems: 1? What if one wants to add here more entries? Drop maxItems.

Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml properties: · line 82

Quoted 5 lines
Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml +5 -0
+
+  pinctrl-1:
+    description:
+      should switch CMD pin to GPIO mode as a high output.
+    maxItems: 1

Ditto

Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml properties: · line 86

Quoted 4 lines
Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml +1 -0
+
   mrvl,clk-delay-cycles:
     description: Specify a number of cycles to delay for tuning.
     $ref: /schemas/types.yaml#/definitions/uint32

Best regards,
Krzysztof

author
#13 lore raw

Re: [PATCH 8/8] dt-bindings: mmc: sdhci-pxa: add pxav1

Hi Krzysztof and Rob,

Quoted 8 lines · Krzysztof Kozlowski

No need for minItems.

Start with capital letter if this is a sentence with full stop. It's anyway looking different then the rest of the file, right?

Why maxItems: 1? What if one wants to add here more entries? Drop maxItems.

Ditto

Thanks for the fast review. Will fix this all in V2 after I receive feedback on the other patches. I was trying to follow the pattern from another similar schema and didn't fully understand.

Quoted 2 lines · Rob Herring

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13)

Well this is embarrassing! I forgot to run dt_binding_check. Sorry about that.

Doug

maintainer
#14 lore raw

Re: [PATCH 8/8] dt-bindings: mmc: sdhci-pxa: add pxav1

In reply to Doug Brown
Quoted 8 lines · Doug Brown

Add a compatible for the pxav1 controller in the PXA168, along with optional pinctrl properties to use for an errata workaround.

---
 .../devicetree/bindings/mmc/sdhci-pxa.yaml    | 22 ++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml: properties:pinctrl-names: 'oneOf' conditional failed, one must be fixed:
[{'const': 'default'}, {'const': 'state_cmd_gpio'}] is too long
[{'const': 'default'}, {'const': 'state_cmd_gpio'}] is too short
False schema does not allow 2
1 was expected
hint: "minItems" is only needed if less than the "items" list length
from schema $id: http://devicetree.org/meta-schemas/items.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]

This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command.