[PATCH v2 0/8] mmc: sdhci-pxav2: Add support for PXA168

[PATCH v2 0/8] mmc: sdhci-pxav2: Add support for PXA168

[Doug Brown]

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/1363544206-3671-1-git-send-email-tanmay.upadhyay@einfochips.com/

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.

Changes in v2:
- Fix mistakes in devicetree binding
- Use cleaner code for pxav1_readw suggested by Adrian
- Switch to request_done() and irq() for SDIO workaround CMD0 handling

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    |  19 ++-
 drivers/mmc/host/Kconfig                      |   1 +
 drivers/mmc/host/sdhci-pxav2.c                | 150 +++++++++++++++++-
 3 files changed, 163 insertions(+), 7 deletions(-)

-- 
2.34.1

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

[Doug Brown]

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.

Signed-off-by: Doug Brown <doug@schmorgal.com>
---
 drivers/mmc/host/sdhci-pxav2.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-pxav2.c b/drivers/mmc/host/sdhci-pxav2.c
index f18906b5575f..2f9fa0ecbddd 100644
--- a/drivers/mmc/host/sdhci-pxav2.c
+++ b/drivers/mmc/host/sdhci-pxav2.c
@@ -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)
-- 
2.34.1

[PATCH v2 2/8] mmc: sdhci-pxav2: enable CONFIG_MMC_SDHCI_IO_ACCESSORS

[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.

Signed-off-by: Doug Brown <doug@schmorgal.com>
---
 drivers/mmc/host/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 5e19a961c34d..b9e9185c86a6 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -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
-- 
2.34.1

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

[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.

Signed-off-by: Doug Brown <doug@schmorgal.com>
---
 drivers/mmc/host/sdhci-pxav2.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/mmc/host/sdhci-pxav2.c b/drivers/mmc/host/sdhci-pxav2.c
index 2f9fa0ecbddd..0a16098b963f 100644
--- a/drivers/mmc/host/sdhci-pxav2.c
+++ b/drivers/mmc/host/sdhci-pxav2.c
@@ -80,6 +80,15 @@ static void pxav2_reset(struct sdhci_host *host, u8 mask)
 	}
 }
 
+static u16 pxav1_readw(struct sdhci_host *host, int reg)
+{
+	/* Workaround for data abort exception on SDH2 and SDH4 on PXA168 */
+	if (reg == SDHCI_HOST_VERSION)
+		return readl(host->ioaddr + SDHCI_HOST_VERSION - 2) >> 16;
+
+	return readw(host->ioaddr + reg);
+}
+
 static void pxav2_mmc_set_bus_width(struct sdhci_host *host, int width)
 {
 	u8 ctrl;
@@ -102,6 +111,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,
-- 
2.34.1

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

[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.

Signed-off-by: Doug Brown <doug@schmorgal.com>
---
 drivers/mmc/host/sdhci-pxav2.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-pxav2.c b/drivers/mmc/host/sdhci-pxav2.c
index 0a16098b963f..509ba5dd4a4a 100644
--- a/drivers/mmc/host/sdhci-pxav2.c
+++ b/drivers/mmc/host/sdhci-pxav2.c
@@ -189,7 +189,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);
-- 
2.34.1

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

[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.

Signed-off-by: Doug Brown <doug@schmorgal.com>
---
 drivers/mmc/host/sdhci-pxav2.c | 40 ++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pxav2.c b/drivers/mmc/host/sdhci-pxav2.c
index 509ba5dd4a4a..1f0c3028987a 100644
--- a/drivers/mmc/host/sdhci-pxav2.c
+++ b/drivers/mmc/host/sdhci-pxav2.c
@@ -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));
@@ -176,6 +180,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;
@@ -183,11 +188,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))
@@ -204,6 +210,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;
@@ -240,17 +255,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",
@@ -259,7 +291,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);
-- 
2.34.1

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

[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.

Signed-off-by: Doug Brown <doug@schmorgal.com>
---
 drivers/mmc/host/sdhci-pxav2.c | 49 ++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/mmc/host/sdhci-pxav2.c b/drivers/mmc/host/sdhci-pxav2.c
index 1f0c3028987a..912b2aad9f2e 100644
--- a/drivers/mmc/host/sdhci-pxav2.c
+++ b/drivers/mmc/host/sdhci-pxav2.c
@@ -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;
+	struct mmc_request *sdio_mrq;
 };
 
 static void pxav2_reset(struct sdhci_host *host, u8 mask)
@@ -93,6 +96,50 @@ static u16 pxav1_readw(struct sdhci_host *host, int reg)
 	return readw(host->ioaddr + reg);
 }
 
+static u32 pxav1_irq(struct sdhci_host *host, u32 intmask)
+{
+	struct sdhci_pxav2_host *pxav2_host = sdhci_pltfm_priv(sdhci_priv(host));
+
+	if (pxav2_host->sdio_mrq && (intmask & SDHCI_INT_CMD_MASK)) {
+		/* The dummy CMD0 for the SDIO workaround just completed */
+		sdhci_writel(host, intmask & SDHCI_INT_CMD_MASK, SDHCI_INT_STATUS);
+		intmask &= ~SDHCI_INT_CMD_MASK;
+		mmc_request_done(host->mmc, pxav2_host->sdio_mrq);
+		pxav2_host->sdio_mrq = NULL;
+	}
+
+	return intmask;
+}
+
+static void pxav1_request_done(struct sdhci_host *host, struct mmc_request *mrq)
+{
+	u16 tmp;
+	struct sdhci_pxav2_host *pxav2_host;
+
+	/* If this is an SDIO command, perform errata workaround for silicon bug */
+	if (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));
+		pxav2_host->sdio_mrq = mrq;
+		sdhci_writel(host, 0, SDHCI_ARGUMENT);
+		sdhci_writew(host, 0, SDHCI_TRANSFER_MODE);
+		sdhci_writew(host, SDHCI_MAKE_CMD(MMC_GO_IDLE_STATE, SDHCI_CMD_RESP_NONE),
+			     SDHCI_COMMAND);
+
+		/* Don't finish this request until the dummy CMD0 finishes */
+		return;
+	}
+
+	mmc_request_done(host->mmc, mrq);
+}
+
 static void pxav2_mmc_set_bus_width(struct sdhci_host *host, int width)
 {
 	u8 ctrl;
@@ -117,10 +164,12 @@ 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,
+	.irq           = pxav1_irq,
 	.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,
+	.request_done  = pxav1_request_done,
 };
 
 static const struct sdhci_ops pxav2_sdhci_ops = {
-- 
2.34.1

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

[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.

Signed-off-by: Doug Brown <doug@schmorgal.com>
---
 drivers/mmc/host/sdhci-pxav2.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/mmc/host/sdhci-pxav2.c b/drivers/mmc/host/sdhci-pxav2.c
index 912b2aad9f2e..88927549b425 100644
--- a/drivers/mmc/host/sdhci-pxav2.c
+++ b/drivers/mmc/host/sdhci-pxav2.c
@@ -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;
 	struct mmc_request *sdio_mrq;
+	struct pinctrl *pinctrl;
+	struct pinctrl_state *pins_default;
+	struct pinctrl_state *pins_cmd_gpio;
 };
 
 static void pxav2_reset(struct sdhci_host *host, u8 mask)
@@ -104,6 +108,11 @@ static u32 pxav1_irq(struct sdhci_host *host, u32 intmask)
 		/* The dummy CMD0 for the SDIO workaround just completed */
 		sdhci_writel(host, intmask & SDHCI_INT_CMD_MASK, SDHCI_INT_STATUS);
 		intmask &= ~SDHCI_INT_CMD_MASK;
+
+		/* Restore MMC function to CMD pin */
+		if (pxav2_host->pinctrl && pxav2_host->pins_default)
+			pinctrl_select_state(pxav2_host->pinctrl, pxav2_host->pins_default);
+
 		mmc_request_done(host->mmc, pxav2_host->sdio_mrq);
 		pxav2_host->sdio_mrq = NULL;
 	}
@@ -128,6 +137,11 @@ static void pxav1_request_done(struct sdhci_host *host, struct mmc_request *mrq)
 		/* Clock is now stopped, so restart it by sending a dummy CMD0 */
 		pxav2_host = sdhci_pltfm_priv(sdhci_priv(host));
 		pxav2_host->sdio_mrq = mrq;
+
+		/* 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);
+
 		sdhci_writel(host, 0, SDHCI_ARGUMENT);
 		sdhci_writew(host, 0, SDHCI_TRANSFER_MODE);
 		sdhci_writew(host, SDHCI_MAKE_CMD(MMC_GO_IDLE_STATE, SDHCI_CMD_RESP_NONE),
@@ -298,6 +312,21 @@ 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;
+
+		/* 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;
 	}
-- 
2.34.1

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

[Doug Brown]

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

Signed-off-by: Doug Brown <doug@schmorgal.com>
---
 .../devicetree/bindings/mmc/sdhci-pxa.yaml    | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml b/Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml
index 1c87f4218e18..8bb0eca506e5 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml
+++ b/Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml
@@ -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 <ulf.hansson@linaro.org>
@@ -34,6 +34,7 @@ allOf:
 properties:
   compatible:
     enum:
+      - mrvl,pxav1-mmc
       - mrvl,pxav2-mmc
       - mrvl,pxav3-mmc
       - marvell,armada-380-sdhci
@@ -61,6 +62,22 @@ 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.
+    items:
+      - const: default
+      - const: state_cmd_gpio
+
+  pinctrl-0:
+    description:
+      Should contain default pinctrl.
+
+  pinctrl-1:
+    description:
+      Should switch CMD pin to GPIO mode as a high output.
+
   mrvl,clk-delay-cycles:
     description: Specify a number of cycles to delay for tuning.
     $ref: /schemas/types.yaml#/definitions/uint32
-- 
2.34.1

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

[Krzysztof Kozlowski]

On 02/12/2022 04:13, Doug Brown wrote:
> Add a compatible for the pxav1 controller in the PXA168, along with
> optional pinctrl properties to use for an errata workaround.
> 
> Signed-off-by: Doug Brown <doug@schmorgal.com>
> ---
>  .../devicetree/bindings/mmc/sdhci-pxa.yaml    | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml b/Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml
> index 1c87f4218e18..8bb0eca506e5 100644
> --- a/Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml
> @@ -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

While changing this, drop "bindings". Will save me effort in my cleanup
series.

>  
>  maintainers:
>    - Ulf Hansson <ulf.hansson@linaro.org>
> @@ -34,6 +34,7 @@ allOf:
>  properties:
>    compatible:
>      enum:
> +      - mrvl,pxav1-mmc
>        - mrvl,pxav2-mmc
>        - mrvl,pxav3-mmc
>        - marvell,armada-380-sdhci
> @@ -61,6 +62,22 @@ properties:
>        - const: io
>        - const: core
>  
> +  pinctrl-names:
> +    description:
> +      Optional for supporting PXA168 SDIO IRQ errata to switch CMD pin between SDIO CMD and

The line is a bit more than 80 and there is no benefit in this. Wrap it
like coding style asks - 80.

With above:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

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

[Adrian Hunter]

On 2/12/22 05:13, Doug Brown wrote:
> 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.
> 
> Signed-off-by: Doug Brown <doug@schmorgal.com>
> ---
>  drivers/mmc/host/sdhci-pxav2.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pxav2.c b/drivers/mmc/host/sdhci-pxav2.c
> index f18906b5575f..2f9fa0ecbddd 100644
> --- a/drivers/mmc/host/sdhci-pxav2.c
> +++ b/drivers/mmc/host/sdhci-pxav2.c
> @@ -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;
> +	}

It would be better to put the information above in a structure and
get it with of_device_get_match_data() (instead of of_match_device).
Also drivers typically assume there is always a match since that
is the only way the driver ->probe() will get run.

>  
>  	ret = sdhci_add_host(host);
>  	if (ret)

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

[Adrian Hunter]

On 2/12/22 05:13, Doug Brown wrote:
> 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.
> 
> Signed-off-by: Doug Brown <doug@schmorgal.com>
> ---
>  drivers/mmc/host/sdhci-pxav2.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pxav2.c b/drivers/mmc/host/sdhci-pxav2.c
> index 0a16098b963f..509ba5dd4a4a 100644
> --- a/drivers/mmc/host/sdhci-pxav2.c
> +++ b/drivers/mmc/host/sdhci-pxav2.c
> @@ -189,7 +189,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);

It is nicer to handle EPROBE_DEFER e.g.

	if (IS_ERR(clk) && PTR_ERR(clk) != -EPROBE_DEFER)
		clk = devm_clk_get(dev, NULL);
	if (IS_ERR(clk)) {
		ret = PTR_ERR(clk);
		dev_err_probe(dev, ret, "failed to get io clock\n");

Re: [PATCH v2 5/8] mmc: sdhci-pxav2: add optional core clock

[Adrian Hunter]

On 2/12/22 05:13, Doug Brown wrote:
> 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.
> 
> Signed-off-by: Doug Brown <doug@schmorgal.com>
> ---
>  drivers/mmc/host/sdhci-pxav2.c | 40 ++++++++++++++++++++++++++++++----
>  1 file changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pxav2.c b/drivers/mmc/host/sdhci-pxav2.c
> index 509ba5dd4a4a..1f0c3028987a 100644
> --- a/drivers/mmc/host/sdhci-pxav2.c
> +++ b/drivers/mmc/host/sdhci-pxav2.c
> @@ -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));
> @@ -176,6 +180,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;
> @@ -183,11 +188,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))
> @@ -204,6 +210,15 @@ static int sdhci_pxav2_probe(struct platform_device *pdev)
>  		goto free;
>  	}
>  
> +	pxav2_host->clk_core = devm_clk_get(dev, "core");

This can use devm_clk_get_optional_enabled()

> +	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");

	if (IS_ERR(pxav2_host->clk_core)) {
		dev_err_probe(&pdev->dev, PTR_ERR(pxav2_host->clk_core), "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;
> @@ -240,17 +255,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))

With changes above this would need to be: IS_ERR_OR_NULL


> +		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))

With changes above this would need to be: IS_ERR_OR_NULL

> +		clk_disable_unprepare(pxav2_host->clk_core);
> +
> +	return ret;
> +}
> +
>  static struct platform_driver sdhci_pxav2_driver = {
>  	.driver		= {
>  		.name	= "sdhci-pxav2",
> @@ -259,7 +291,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);

Re: [PATCH v2 5/8] mmc: sdhci-pxav2: add optional core clock

[Adrian Hunter]

On 22/12/22 19:33, Adrian Hunter wrote:
> On 2/12/22 05:13, Doug Brown wrote:
>> 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.
>>
>> Signed-off-by: Doug Brown <doug@schmorgal.com>
>> ---
>>  drivers/mmc/host/sdhci-pxav2.c | 40 ++++++++++++++++++++++++++++++----
>>  1 file changed, 36 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-pxav2.c b/drivers/mmc/host/sdhci-pxav2.c
>> index 509ba5dd4a4a..1f0c3028987a 100644
>> --- a/drivers/mmc/host/sdhci-pxav2.c
>> +++ b/drivers/mmc/host/sdhci-pxav2.c
>> @@ -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));
>> @@ -176,6 +180,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;
>> @@ -183,11 +188,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))
>> @@ -204,6 +210,15 @@ static int sdhci_pxav2_probe(struct platform_device *pdev)
>>  		goto free;
>>  	}
>>  
>> +	pxav2_host->clk_core = devm_clk_get(dev, "core");
> 
> This can use devm_clk_get_optional_enabled()
> 
>> +	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");
> 
> 	if (IS_ERR(pxav2_host->clk_core)) {
> 		dev_err_probe(&pdev->dev, PTR_ERR(pxav2_host->clk_core), "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;
>> @@ -240,17 +255,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))
> 
> With changes above this would need to be: IS_ERR_OR_NULL

Actually with changes above, it is taken care of by devm so
clk_disable_unprepare is not needed.

> 
> 
>> +		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))
> 
> With changes above this would need to be: IS_ERR_OR_NULL

Ditto

> 
>> +		clk_disable_unprepare(pxav2_host->clk_core);
>> +
>> +	return ret;
>> +}
>> +
>>  static struct platform_driver sdhci_pxav2_driver = {
>>  	.driver		= {
>>  		.name	= "sdhci-pxav2",
>> @@ -259,7 +291,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);
> 

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

[Adrian Hunter]

On 2/12/22 05:13, Doug Brown wrote:
> 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.
> 
> Signed-off-by: Doug Brown <doug@schmorgal.com>
> ---
>  drivers/mmc/host/sdhci-pxav2.c | 49 ++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-pxav2.c b/drivers/mmc/host/sdhci-pxav2.c
> index 1f0c3028987a..912b2aad9f2e 100644
> --- a/drivers/mmc/host/sdhci-pxav2.c
> +++ b/drivers/mmc/host/sdhci-pxav2.c
> @@ -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;
> +	struct mmc_request *sdio_mrq;
>  };
>  
>  static void pxav2_reset(struct sdhci_host *host, u8 mask)
> @@ -93,6 +96,50 @@ static u16 pxav1_readw(struct sdhci_host *host, int reg)
>  	return readw(host->ioaddr + reg);
>  }
>  
> +static u32 pxav1_irq(struct sdhci_host *host, u32 intmask)
> +{
> +	struct sdhci_pxav2_host *pxav2_host = sdhci_pltfm_priv(sdhci_priv(host));
> +
> +	if (pxav2_host->sdio_mrq && (intmask & SDHCI_INT_CMD_MASK)) {
> +		/* The dummy CMD0 for the SDIO workaround just completed */
> +		sdhci_writel(host, intmask & SDHCI_INT_CMD_MASK, SDHCI_INT_STATUS);
> +		intmask &= ~SDHCI_INT_CMD_MASK;
> +		mmc_request_done(host->mmc, pxav2_host->sdio_mrq);
> +		pxav2_host->sdio_mrq = NULL;

Pedantically, things should be finished before calling
mmc_request_done() i.e. last 2 line the other way around

		pxav2_host->sdio_mrq = NULL;
		mmc_request_done(host->mmc, pxav2_host->sdio_mrq);

> +	}
> +
> +	return intmask;
> +}
> +
> +static void pxav1_request_done(struct sdhci_host *host, struct mmc_request *mrq)
> +{
> +	u16 tmp;
> +	struct sdhci_pxav2_host *pxav2_host;
> +
> +	/* If this is an SDIO command, perform errata workaround for silicon bug */
> +	if (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));
> +		pxav2_host->sdio_mrq = mrq;
> +		sdhci_writel(host, 0, SDHCI_ARGUMENT);
> +		sdhci_writew(host, 0, SDHCI_TRANSFER_MODE);
> +		sdhci_writew(host, SDHCI_MAKE_CMD(MMC_GO_IDLE_STATE, SDHCI_CMD_RESP_NONE),
> +			     SDHCI_COMMAND);
> +
> +		/* Don't finish this request until the dummy CMD0 finishes */
> +		return;
> +	}
> +
> +	mmc_request_done(host->mmc, mrq);
> +}
> +
>  static void pxav2_mmc_set_bus_width(struct sdhci_host *host, int width)
>  {
>  	u8 ctrl;
> @@ -117,10 +164,12 @@ 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,
> +	.irq           = pxav1_irq,
>  	.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,
> +	.request_done  = pxav1_request_done,
>  };
>  
>  static const struct sdhci_ops pxav2_sdhci_ops = {

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

[Doug Brown]

Hi Adrian,

On 12/22/2022 8:03 AM, Adrian Hunter wrote:
> On 2/12/22 05:13, Doug Brown wrote:
>> 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.
>>
>> Signed-off-by: Doug Brown <doug@schmorgal.com>
>> ---
>>   drivers/mmc/host/sdhci-pxav2.c | 18 +++++++++++++++++-
>>   1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-pxav2.c b/drivers/mmc/host/sdhci-pxav2.c
>> index f18906b5575f..2f9fa0ecbddd 100644
>> --- a/drivers/mmc/host/sdhci-pxav2.c
>> +++ b/drivers/mmc/host/sdhci-pxav2.c
>> @@ -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;
>> +	}
> 
> It would be better to put the information above in a structure and
> get it with of_device_get_match_data() (instead of of_match_device).
> Also drivers typically assume there is always a match since that
> is the only way the driver ->probe() will get run.

Thanks for all of your great feedback on this series. That makes sense.
I did have one question about this suggestion. There are other parts of
sdhci_pxav2_probe() that don't assume there was a match so that it can
be set up the old way as a platform_device without CONFIG_OF. I was
trying to preserve compatibility by defaulting to pxav2_sdhci_ops if
it was set up as a platform_device. Is it all right if I leave a
fallback in place for that, or should I just end compatibility with the
old way at this point and assume a match in all cases? I don't see any
legacy board files that use this driver.

> 
>>   
>>   	ret = sdhci_add_host(host);
>>   	if (ret)
> 

Thanks,
Doug

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

[Adrian Hunter]

On 26/12/22 22:35, Doug Brown wrote:
> Hi Adrian,
> 
> On 12/22/2022 8:03 AM, Adrian Hunter wrote:
>> On 2/12/22 05:13, Doug Brown wrote:
>>> 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.
>>>
>>> Signed-off-by: Doug Brown <doug@schmorgal.com>
>>> ---
>>>   drivers/mmc/host/sdhci-pxav2.c | 18 +++++++++++++++++-
>>>   1 file changed, 17 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-pxav2.c b/drivers/mmc/host/sdhci-pxav2.c
>>> index f18906b5575f..2f9fa0ecbddd 100644
>>> --- a/drivers/mmc/host/sdhci-pxav2.c
>>> +++ b/drivers/mmc/host/sdhci-pxav2.c
>>> @@ -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;
>>> +    }
>>
>> It would be better to put the information above in a structure and
>> get it with of_device_get_match_data() (instead of of_match_device).
>> Also drivers typically assume there is always a match since that
>> is the only way the driver ->probe() will get run.
> 
> Thanks for all of your great feedback on this series. That makes sense.
> I did have one question about this suggestion. There are other parts of
> sdhci_pxav2_probe() that don't assume there was a match so that it can
> be set up the old way as a platform_device without CONFIG_OF. I was
> trying to preserve compatibility by defaulting to pxav2_sdhci_ops if
> it was set up as a platform_device. Is it all right if I leave a
> fallback in place for that, or should I just end compatibility with the
> old way at this point and assume a match in all cases? I don't see any
> legacy board files that use this driver.

I didn't think about matching by name.  As you say, there doesn't seem
to be anything in mainline.  I will leave it to you to decide.

> 
>>
>>>         ret = sdhci_add_host(host);
>>>       if (ret)
>>
> 
> Thanks,
> Doug