[PATCH V2 0/3] Add SDHCI driver for Broadcom BRCMSTB/BMIPS Soc's

[PATCH V2 0/3] Add SDHCI driver for Broadcom BRCMSTB/BMIPS Soc's

[Al Cooper]

This patchset adds the SDHCI driver for the Broadcom BRCMSTB/BMIPS
Soc's. A few new Device Tree properties, needed by this driver,
were added to sdhci-pltfm.c to set various QUIRKS. The mmc.txt
bindings document was update with the both new and existing
properties handled by sdhci-pltfm.c

V2 - Don't create a new QUIRK to handle broken SDR50 mode in
     the controller. The functionality was moved into the
     sdhci-brcmstb driver.

Al Cooper (3):
  mmc: sdhci-pltfm: Add DT properties to set various QUIRKS
  mmc: Add Device Tree binding supported by sdhci-pltfm.c
  mmc: sdhci-brcmstb: Add sdhci driver for Broadcom BRCMSTB/BMIPS SOCs

 Documentation/devicetree/bindings/mmc/mmc.txt      |   8 ++
 .../devicetree/bindings/mmc/sdhci-brcmstb.txt      |  36 +++++
 drivers/mmc/host/Kconfig                           |  12 ++
 drivers/mmc/host/Makefile                          |   1 +
 drivers/mmc/host/sdhci-brcmstb.c                   | 158 +++++++++++++++++++++
 drivers/mmc/host/sdhci-pltfm.c                     |   8 +-
 6 files changed, 222 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-brcmstb.txt
 create mode 100644 drivers/mmc/host/sdhci-brcmstb.c

-- 
1.9.0.138.g2de3478

[PATCH V2 1/3] mmc: sdhci-pltfm: Add DT properties to set various QUIRKS

[Al Cooper]

Add support for "broken-ddr50", "broken-64-bit-dma"
and "broken-timeout-value" device tree properties.
The properties will cause the corresponding quirks bits to be set.
This allows some of the platform specific QUIRKS setting to be
moved out of the driver and into the Device Tree node.

Signed-off-by: Al Cooper <alcooperx@gmail.com>
---
 drivers/mmc/host/sdhci-pltfm.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index 87fb5ea..cc0730c 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -90,10 +90,14 @@ void sdhci_get_of_property(struct platform_device *pdev)
 	if (of_get_property(np, "no-1-8-v", NULL))
 		host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
 
+	if (of_get_property(np, "broken-64-bit-dma", NULL))
+		host->quirks2 |= SDHCI_QUIRK2_BROKEN_64_BIT_DMA;
+
 	if (of_device_is_compatible(np, "fsl,p2020-rev1-esdhc"))
 		host->quirks |= SDHCI_QUIRK_BROKEN_DMA;
 
-	if (of_device_is_compatible(np, "fsl,p2020-esdhc") ||
+	if (of_get_property(np, "broken-timeout-value", NULL) ||
+	    of_device_is_compatible(np, "fsl,p2020-esdhc") ||
 	    of_device_is_compatible(np, "fsl,p1010-esdhc") ||
 	    of_device_is_compatible(np, "fsl,t4240-esdhc") ||
 	    of_device_is_compatible(np, "fsl,mpc8536-esdhc"))
@@ -106,6 +110,8 @@ void sdhci_get_of_property(struct platform_device *pdev)
 
 	if (of_find_property(np, "enable-sdio-wakeup", NULL))
 		host->mmc->pm_caps |= MMC_PM_WAKE_SDIO_IRQ;
+	if (of_find_property(np, "broken-ddr50", NULL))
+		host->quirks2 |= SDHCI_QUIRK2_BROKEN_DDR50;
 }
 #else
 void sdhci_get_of_property(struct platform_device *pdev) {}
-- 
1.9.0.138.g2de3478

[PATCH V2 2/3] mmc: Add Device Tree binding supported by sdhci-pltfm.c

[Al Cooper]

This includes both newly added and previously undocumented
properties.

Signed-off-by: Al Cooper <alcooperx@gmail.com>
---
 Documentation/devicetree/bindings/mmc/mmc.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
index f693baf..516fdf9 100644
--- a/Documentation/devicetree/bindings/mmc/mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc.txt
@@ -48,6 +48,14 @@ Optional properties:
 - mmc-hs400-1_2v: eMMC HS400 mode(1.2V I/O) is supported
 - dsr: Value the card's (optional) Driver Stage Register (DSR) should be
   programmed with. Valid range: [0 .. 0xffff].
+- sdhci,auto-cmd12: the controller supports Auto-CMD12 to stop multiblock
+  transfers.
+- broken-64-bit-dma: the controller does not support 64-bit DMA even if
+  the controller claims it does.
+- broken-timeout-value: the timeout value specified by the controller is
+  incorrect and the MAX timeout will be used instead.
+- broken-sdr50: SDR50 mode is broken.
+- broken-ddr50: DDR50 mode is broken.
 
 *NOTE* on CD and WP polarity. To use common for all SD/MMC host controllers line
 polarity properties, we have to fix the meaning of the "normal" and "inverted"
-- 
1.9.0.138.g2de3478

[PATCH V2 3/3] mmc: sdhci-brcmstb: Add sdhci driver for Broadcom BRCMSTB/BMIPS SOCs

[Al Cooper]

Signed-off-by: Al Cooper <alcooperx@gmail.com>
---
 .../devicetree/bindings/mmc/sdhci-brcmstb.txt      |  36 +++++
 drivers/mmc/host/Kconfig                           |  12 ++
 drivers/mmc/host/Makefile                          |   1 +
 drivers/mmc/host/sdhci-brcmstb.c                   | 158 +++++++++++++++++++++
 4 files changed, 207 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-brcmstb.txt
 create mode 100644 drivers/mmc/host/sdhci-brcmstb.c

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-brcmstb.txt b/Documentation/devicetree/bindings/mmc/sdhci-brcmstb.txt
new file mode 100644
index 0000000..50375a1
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/sdhci-brcmstb.txt
@@ -0,0 +1,36 @@
+* BROADCOM BRCMSTB/BMIPS SDHCI Controller
+
+This file documents differences between the core properties in mmc.txt
+and the properties used by the sdhci-brcmstb driver.
+
+Required properties:
+- compatible: "brcm,sdhci-brcmstb"
+
+Optional properties:
+- broken-sdr50: The Host Controller's SDR50 mode is broken.
+
+Example:
+
+	sdhci@f03e0100 {
+		compatible = "brcm,sdhci-brcmstb";
+		reg = <0xf03e0000 0x100>;
+		interrupts = <0x0 0x26 0x0>;
+		interrupt-names = "sdio0_0";
+		sdhci,auto-cmd12;
+		clocks = <0x13>;
+		clock-names = "sw_sdio";
+		broken-sdr50;
+	};
+
+	sdhci@f03e0300 {
+		no-1-8-v;
+		non-removable;
+		bus-width = <0x8>;
+		compatible = "brcm,sdhci-brcmstb";
+		reg = <0xf03e0200 0x100>;
+		interrupts = <0x0 0x27 0x0>;
+		interrupt-names = "sdio1_0";
+		sdhci,auto-cmd12;
+		clocks = <0x13>;
+		clock-names = "sw_sdio";
+	};
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index af71de5..d99f84a 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -784,3 +784,15 @@ config MMC_MTK
 	  If you have a machine with a integrated SD/MMC card reader, say Y or M here.
 	  This is needed if support for any SD/SDIO/MMC devices is required.
 	  If unsure, say N.
+
+config MMC_SDHCI_BRCMSTB
+	tristate "Broadcom SDIO/SD/MMC support"
+	depends on ARCH_BRCMSTB || BMIPS_GENERIC
+	depends on MMC_SDHCI_PLTFM
+	default y
+	select MMC_SDHCI_IO_ACCESSORS
+	help
+	  This selects support for the SDIO/SD/MMC Host Controller on
+	  Broadcom STB SoCs.
+
+	  If unsure, say Y.
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 3595f83..e3e163f 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -75,6 +75,7 @@ obj-$(CONFIG_MMC_SDHCI_BCM2835)		+= sdhci-bcm2835.o
 obj-$(CONFIG_MMC_SDHCI_IPROC)		+= sdhci-iproc.o
 obj-$(CONFIG_MMC_SDHCI_MSM)		+= sdhci-msm.o
 obj-$(CONFIG_MMC_SDHCI_ST)		+= sdhci-st.o
+obj-$(CONFIG_MMC_SDHCI_BRCMSTB)		+= sdhci-brcmstb.o
 
 ifeq ($(CONFIG_CB710_DEBUG),y)
 	CFLAGS-cb710-mmc	+= -DDEBUG
diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c
new file mode 100644
index 0000000..617cd6e
--- /dev/null
+++ b/drivers/mmc/host/sdhci-brcmstb.c
@@ -0,0 +1,158 @@
+/*
+ * sdhci-brcmstb.c Support for SDHCI on Broadcom BRCMSTB SoC's
+ *
+ * Copyright (C) 2015 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/io.h>
+#include <linux/mmc/host.h>
+#include <linux/module.h>
+#include <linux/of.h>
+
+#include "sdhci-pltfm.h"
+
+static int sdhci_brcmstb_enable_dma(struct sdhci_host *host)
+{
+	if (host->flags & SDHCI_USE_64_BIT_DMA)
+		if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA)
+			host->flags &= ~SDHCI_USE_64_BIT_DMA;
+	return 0;
+}
+
+static void brcmstb_of_parse(struct platform_device *pdev,
+			struct sdhci_host *host)
+{
+	struct device_node *np = pdev->dev.of_node;
+
+	if (!of_get_property(np, "broken-sdr50", NULL))
+		return;
+	host->quirks |= SDHCI_QUIRK_MISSING_CAPS;
+	host->caps = sdhci_readl(host, SDHCI_CAPABILITIES);
+	host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
+	host->caps1 &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50);
+}
+
+
+#ifdef CONFIG_PM_SLEEP
+
+static int sdhci_brcmstb_suspend(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	int res;
+
+	res = sdhci_suspend_host(host);
+	if (res)
+		return res;
+	clk_disable(pltfm_host->clk);
+	return res;
+}
+
+static int sdhci_brcmstb_resume(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	int err;
+
+	err = clk_enable(pltfm_host->clk);
+	if (err)
+		return err;
+	return sdhci_resume_host(host);
+}
+
+#endif /* CONFIG_PM_SLEEP */
+
+static SIMPLE_DEV_PM_OPS(sdhci_brcmstb_pmops, sdhci_brcmstb_suspend,
+			sdhci_brcmstb_resume);
+
+static const struct sdhci_ops sdhci_brcmstb_ops = {
+	.set_clock = sdhci_set_clock,
+	.set_bus_width = sdhci_set_bus_width,
+	.reset = sdhci_reset,
+	.set_uhs_signaling = sdhci_set_uhs_signaling,
+	.enable_dma = sdhci_brcmstb_enable_dma,
+};
+
+static struct sdhci_pltfm_data sdhci_brcmstb_pdata = {
+	.ops = &sdhci_brcmstb_ops,
+};
+
+static int sdhci_brcmstb_probe(struct platform_device *pdev)
+{
+	struct device_node *dn = pdev->dev.of_node;
+	struct sdhci_host *host;
+	struct sdhci_pltfm_host *pltfm_host;
+	struct clk *clk;
+	int res;
+
+	clk = of_clk_get_by_name(dn, "sw_sdio");
+	if (IS_ERR(clk)) {
+		dev_err(&pdev->dev, "Clock not found in Device Tree\n");
+		clk = NULL;
+	}
+	res = clk_prepare_enable(clk);
+	if (res)
+		goto undo_clk_get;
+
+	host = sdhci_pltfm_init(pdev, &sdhci_brcmstb_pdata, 0);
+	if (IS_ERR(host)) {
+		res = PTR_ERR(host);
+		goto undo_clk_prep;
+	}
+
+	/* Enable MMC_CAP2_HC_ERASE_SZ for better max discard calculations */
+	host->mmc->caps2 |= MMC_CAP2_HC_ERASE_SZ;
+
+	sdhci_get_of_property(pdev);
+	mmc_of_parse(host->mmc);
+	brcmstb_of_parse(pdev, host);
+
+	res = sdhci_add_host(host);
+	if (res)
+		goto undo_pltfm_init;
+
+	pltfm_host = sdhci_priv(host);
+	pltfm_host->clk = clk;
+	return res;
+
+undo_pltfm_init:
+	sdhci_pltfm_free(pdev);
+undo_clk_prep:
+	clk_disable_unprepare(clk);
+undo_clk_get:
+	clk_put(clk);
+	return res;
+}
+
+static const struct of_device_id sdhci_brcm_of_match[] = {
+	{ .compatible = "brcm,sdhci-brcmstb" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sdhci_brcm_of_match);
+
+static struct platform_driver sdhci_brcmstb_driver = {
+	.driver		= {
+		.name	= "sdhci-brcmstb",
+		.owner	= THIS_MODULE,
+		.pm	= &sdhci_brcmstb_pmops,
+		.of_match_table = of_match_ptr(sdhci_brcm_of_match),
+	},
+	.probe		= sdhci_brcmstb_probe,
+	.remove		= sdhci_pltfm_unregister,
+};
+
+module_platform_driver(sdhci_brcmstb_driver);
+
+MODULE_DESCRIPTION("SDHCI driver for Broadcom BRCMSTB SoCs");
+MODULE_AUTHOR("Broadcom");
+MODULE_LICENSE("GPL v2");
-- 
1.9.0.138.g2de3478

Re: [PATCH V2 0/3] Add SDHCI driver for Broadcom BRCMSTB/BMIPS Soc's

[Florian Fainelli]

On 06/11/15 10:56, Al Cooper wrote:
> This patchset adds the SDHCI driver for the Broadcom BRCMSTB/BMIPS
> Soc's. A few new Device Tree properties, needed by this driver,
> were added to sdhci-pltfm.c to set various QUIRKS. The mmc.txt
> bindings document was update with the both new and existing
> properties handled by sdhci-pltfm.c

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Looks good to me, thanks!

> 
> V2 - Don't create a new QUIRK to handle broken SDR50 mode in
>      the controller. The functionality was moved into the
>      sdhci-brcmstb driver.
> 
> Al Cooper (3):
>   mmc: sdhci-pltfm: Add DT properties to set various QUIRKS
>   mmc: Add Device Tree binding supported by sdhci-pltfm.c
>   mmc: sdhci-brcmstb: Add sdhci driver for Broadcom BRCMSTB/BMIPS SOCs
> 
>  Documentation/devicetree/bindings/mmc/mmc.txt      |   8 ++
>  .../devicetree/bindings/mmc/sdhci-brcmstb.txt      |  36 +++++
>  drivers/mmc/host/Kconfig                           |  12 ++
>  drivers/mmc/host/Makefile                          |   1 +
>  drivers/mmc/host/sdhci-brcmstb.c                   | 158 +++++++++++++++++++++
>  drivers/mmc/host/sdhci-pltfm.c                     |   8 +-
>  6 files changed, 222 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-brcmstb.txt
>  create mode 100644 drivers/mmc/host/sdhci-brcmstb.c
> 


-- 
Florian

Re: [PATCH V2 1/3] mmc: sdhci-pltfm: Add DT properties to set various QUIRKS

[Ulf Hansson]

On 6 November 2015 at 19:56, Al Cooper <alcooperx@gmail.com> wrote:
> Add support for "broken-ddr50", "broken-64-bit-dma"
> and "broken-timeout-value" device tree properties.
> The properties will cause the corresponding quirks bits to be set.
> This allows some of the platform specific QUIRKS setting to be
> moved out of the driver and into the Device Tree node.
>
> Signed-off-by: Al Cooper <alcooperx@gmail.com>
> ---
>  drivers/mmc/host/sdhci-pltfm.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
> index 87fb5ea..cc0730c 100644
> --- a/drivers/mmc/host/sdhci-pltfm.c
> +++ b/drivers/mmc/host/sdhci-pltfm.c
> @@ -90,10 +90,14 @@ void sdhci_get_of_property(struct platform_device *pdev)
>         if (of_get_property(np, "no-1-8-v", NULL))
>                 host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
>
> +       if (of_get_property(np, "broken-64-bit-dma", NULL))
> +               host->quirks2 |= SDHCI_QUIRK2_BROKEN_64_BIT_DMA;
> +

To me this is yet another step in the wrong direction of sdhci.

Not only have we added a quirk which we likely could avoided, but now
you also suggest to add a DT binding for it.

Please try to avoid this, we shouldn't need to add one DT binding per
quirk, right?

>         if (of_device_is_compatible(np, "fsl,p2020-rev1-esdhc"))
>                 host->quirks |= SDHCI_QUIRK_BROKEN_DMA;
>
> -       if (of_device_is_compatible(np, "fsl,p2020-esdhc") ||
> +       if (of_get_property(np, "broken-timeout-value", NULL) ||
> +           of_device_is_compatible(np, "fsl,p2020-esdhc") ||
>             of_device_is_compatible(np, "fsl,p1010-esdhc") ||
>             of_device_is_compatible(np, "fsl,t4240-esdhc") ||
>             of_device_is_compatible(np, "fsl,mpc8536-esdhc"))
> @@ -106,6 +110,8 @@ void sdhci_get_of_property(struct platform_device *pdev)
>
>         if (of_find_property(np, "enable-sdio-wakeup", NULL))
>                 host->mmc->pm_caps |= MMC_PM_WAKE_SDIO_IRQ;
> +       if (of_find_property(np, "broken-ddr50", NULL))
> +               host->quirks2 |= SDHCI_QUIRK2_BROKEN_DDR50;

Same comment as above.

Let me also refer to the response from Scott Branden to the earlier
version of this patchset.
http://permalink.gmane.org/gmane.linux.kernel.mmc/34614

Perhaps we should invent a sdhci library function, which each sdhci
variant can use to set capabilities through. Typically useful for
those variants that have a non-reliable capabilities register.

>  }
>  #else
>  void sdhci_get_of_property(struct platform_device *pdev) {}
> --
> 1.9.0.138.g2de3478
>

Kind regards
Uffe

Re: [PATCH V2 1/3] mmc: sdhci-pltfm: Add DT properties to set various QUIRKS

[Alan Cooper]

On Thu, Nov 12, 2015 at 4:35 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 6 November 2015 at 19:56, Al Cooper <alcooperx@gmail.com> wrote:
>> Add support for "broken-ddr50", "broken-64-bit-dma"
>> and "broken-timeout-value" device tree properties.
>> The properties will cause the corresponding quirks bits to be set.
>> This allows some of the platform specific QUIRKS setting to be
>> moved out of the driver and into the Device Tree node.
>>
>> Signed-off-by: Al Cooper <alcooperx@gmail.com>
>> ---
>>  drivers/mmc/host/sdhci-pltfm.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
>> index 87fb5ea..cc0730c 100644
>> --- a/drivers/mmc/host/sdhci-pltfm.c
>> +++ b/drivers/mmc/host/sdhci-pltfm.c
>> @@ -90,10 +90,14 @@ void sdhci_get_of_property(struct platform_device *pdev)
>>         if (of_get_property(np, "no-1-8-v", NULL))
>>                 host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
>>
>> +       if (of_get_property(np, "broken-64-bit-dma", NULL))
>> +               host->quirks2 |= SDHCI_QUIRK2_BROKEN_64_BIT_DMA;
>> +
>
> To me this is yet another step in the wrong direction of sdhci.
>
> Not only have we added a quirk which we likely could avoided, but now
> you also suggest to add a DT binding for it.
>
> Please try to avoid this, we shouldn't need to add one DT binding per
> quirk, right?

The BRCMSTB SDHCI driver needs to support a mix of existing and future
ARM and MIPS SoC's on a variety of boards, and many of the
combinations have issues. In older separate MIPS/ARM internal versions
of the driver, issues were handled by a combination of QUIRKS and the
use of custom SDHCI registers that allowed the Host CAPS registers to
be changed on a per chip basis. While working on a unified driver for
4.1, I realized that almost all the issues solved by overriding the
CAPs registers could also be done using an existing QUIRK and if I had
device tree properties that set the QUIRKs, that all the chip/board
specific knowledge would end up in the device tree node instead of in
the driver. This also allowed me to stop using the non-standard CAPS
override registers that tended to change per chip. This approach also
allows the driver to work, without change, on future chips/boards as
long as they don't have any new issues and they have the proper device
tree node.

Would there be any objection to me continuing this approach if I moved
all the functionality into the sdhci-brcm.c driver and left sdhci.c
and sdhci-pltfm.c untouched?

>
>>         if (of_device_is_compatible(np, "fsl,p2020-rev1-esdhc"))
>>                 host->quirks |= SDHCI_QUIRK_BROKEN_DMA;
>>
>> -       if (of_device_is_compatible(np, "fsl,p2020-esdhc") ||
>> +       if (of_get_property(np, "broken-timeout-value", NULL) ||
>> +           of_device_is_compatible(np, "fsl,p2020-esdhc") ||
>>             of_device_is_compatible(np, "fsl,p1010-esdhc") ||
>>             of_device_is_compatible(np, "fsl,t4240-esdhc") ||
>>             of_device_is_compatible(np, "fsl,mpc8536-esdhc"))
>> @@ -106,6 +110,8 @@ void sdhci_get_of_property(struct platform_device *pdev)
>>
>>         if (of_find_property(np, "enable-sdio-wakeup", NULL))
>>                 host->mmc->pm_caps |= MMC_PM_WAKE_SDIO_IRQ;
>> +       if (of_find_property(np, "broken-ddr50", NULL))
>> +               host->quirks2 |= SDHCI_QUIRK2_BROKEN_DDR50;
>
> Same comment as above.
>
> Let me also refer to the response from Scott Branden to the earlier
> version of this patchset.
> http://permalink.gmane.org/gmane.linux.kernel.mmc/34614
>
> Perhaps we should invent a sdhci library function, which each sdhci
> variant can use to set capabilities through. Typically useful for
> those variants that have a non-reliable capabilities register.
>

Do you mean something more than using SDHCI_QUIRK_MISSING_CAPS which
allows the driver to supply the 2 CAPS registers instead of having
sdhci.c get them by reading the Host CAPS registers?

>>  }
>>  #else
>>  void sdhci_get_of_property(struct platform_device *pdev) {}
>> --
>> 1.9.0.138.g2de3478
>>
>
> Kind regards
> Uffe

Thanks
Al

Re: [PATCH V2 1/3] mmc: sdhci-pltfm: Add DT properties to set various QUIRKS

[Ulf Hansson]

On 12 November 2015 at 20:44, Alan Cooper <alcooperx@gmail.com> wrote:
> On Thu, Nov 12, 2015 at 4:35 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 6 November 2015 at 19:56, Al Cooper <alcooperx@gmail.com> wrote:
>>> Add support for "broken-ddr50", "broken-64-bit-dma"
>>> and "broken-timeout-value" device tree properties.
>>> The properties will cause the corresponding quirks bits to be set.
>>> This allows some of the platform specific QUIRKS setting to be
>>> moved out of the driver and into the Device Tree node.
>>>
>>> Signed-off-by: Al Cooper <alcooperx@gmail.com>
>>> ---
>>>  drivers/mmc/host/sdhci-pltfm.c | 8 +++++++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
>>> index 87fb5ea..cc0730c 100644
>>> --- a/drivers/mmc/host/sdhci-pltfm.c
>>> +++ b/drivers/mmc/host/sdhci-pltfm.c
>>> @@ -90,10 +90,14 @@ void sdhci_get_of_property(struct platform_device *pdev)
>>>         if (of_get_property(np, "no-1-8-v", NULL))
>>>                 host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
>>>
>>> +       if (of_get_property(np, "broken-64-bit-dma", NULL))
>>> +               host->quirks2 |= SDHCI_QUIRK2_BROKEN_64_BIT_DMA;
>>> +
>>
>> To me this is yet another step in the wrong direction of sdhci.
>>
>> Not only have we added a quirk which we likely could avoided, but now
>> you also suggest to add a DT binding for it.
>>
>> Please try to avoid this, we shouldn't need to add one DT binding per
>> quirk, right?
>
> The BRCMSTB SDHCI driver needs to support a mix of existing and future
> ARM and MIPS SoC's on a variety of boards, and many of the
> combinations have issues. In older separate MIPS/ARM internal versions
> of the driver, issues were handled by a combination of QUIRKS and the
> use of custom SDHCI registers that allowed the Host CAPS registers to
> be changed on a per chip basis. While working on a unified driver for
> 4.1, I realized that almost all the issues solved by overriding the
> CAPs registers could also be done using an existing QUIRK and if I had
> device tree properties that set the QUIRKs, that all the chip/board
> specific knowledge would end up in the device tree node instead of in
> the driver. This also allowed me to stop using the non-standard CAPS
> override registers that tended to change per chip. This approach also
> allows the driver to work, without change, on future chips/boards as
> long as they don't have any new issues and they have the proper device
> tree node.
>
> Would there be any objection to me continuing this approach if I moved
> all the functionality into the sdhci-brcm.c driver and left sdhci.c
> and sdhci-pltfm.c untouched?

Let me elaborate a bit on what I think you/we need to do.

The problem with the quirks is that these exists because of how the
sdhci code is designed.

Sdhci core should have been implemented as a pure library providing a
set of common functions. Today it's more of a driver than a library.
All, or at least most of the variant specific code, should have been
dealt from each of the sdhci variant drivers, instead of via quirks
and sdhci callbacks.

If you didn't notice my recent statement regarding the above on
linux-mmc, let's me say it again. I have a reached a point where I
think sdhci has become unmaintainable (and non-optimized coding wise)
and I am therefore encouraging people to stop adding new quirks or
sdhci callbacks, but instead "libraryfy" sdhci. I would really
appreciate if you could help moving sdhci in that direction!

That said, you won't be able to leave the sdhci core code untouched,
because that would in principle mean that you will need to duplicate
much of the sdhci code into your sdhci-brcm driver. Instead, try to
split-up/re-factor common sdhci code into library functions, which
your sdhci-brcm driver can use.

I guess one of the hardest part in this effort will be to not break
existing sdhci variant drivers. Let's do our best in that effort, but
we will rely on more people to participate in testing/coding.

>
>>
>>>         if (of_device_is_compatible(np, "fsl,p2020-rev1-esdhc"))
>>>                 host->quirks |= SDHCI_QUIRK_BROKEN_DMA;
>>>
>>> -       if (of_device_is_compatible(np, "fsl,p2020-esdhc") ||
>>> +       if (of_get_property(np, "broken-timeout-value", NULL) ||
>>> +           of_device_is_compatible(np, "fsl,p2020-esdhc") ||
>>>             of_device_is_compatible(np, "fsl,p1010-esdhc") ||
>>>             of_device_is_compatible(np, "fsl,t4240-esdhc") ||
>>>             of_device_is_compatible(np, "fsl,mpc8536-esdhc"))
>>> @@ -106,6 +110,8 @@ void sdhci_get_of_property(struct platform_device *pdev)
>>>
>>>         if (of_find_property(np, "enable-sdio-wakeup", NULL))
>>>                 host->mmc->pm_caps |= MMC_PM_WAKE_SDIO_IRQ;
>>> +       if (of_find_property(np, "broken-ddr50", NULL))
>>> +               host->quirks2 |= SDHCI_QUIRK2_BROKEN_DDR50;
>>
>> Same comment as above.
>>
>> Let me also refer to the response from Scott Branden to the earlier
>> version of this patchset.
>> http://permalink.gmane.org/gmane.linux.kernel.mmc/34614
>>
>> Perhaps we should invent a sdhci library function, which each sdhci
>> variant can use to set capabilities through. Typically useful for
>> those variants that have a non-reliable capabilities register.
>>
>
> Do you mean something more than using SDHCI_QUIRK_MISSING_CAPS which
> allows the driver to supply the 2 CAPS registers instead of having
> sdhci.c get them by reading the Host CAPS registers?

Yes, something like that, but preferably without needing to use *any* quirk.

>From an sdhci core perspective there could be a library function which
parses the Host CAPS register. Some sdhci variants could use that
function and some couldn't since the informations in the CAPS register
isn't reliable.

>
>>>  }
>>>  #else
>>>  void sdhci_get_of_property(struct platform_device *pdev) {}
>>> --
>>> 1.9.0.138.g2de3478
>>>
>>
>> Kind regards
>> Uffe
>
> Thanks
> Al

Kind regards
Uffe

Re: [PATCH V2 1/3] mmc: sdhci-pltfm: Add DT properties to set various QUIRKS

[Alan Cooper]

On Fri, Nov 13, 2015 at 4:31 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 12 November 2015 at 20:44, Alan Cooper <alcooperx@gmail.com> wrote:
>> On Thu, Nov 12, 2015 at 4:35 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> On 6 November 2015 at 19:56, Al Cooper <alcooperx@gmail.com> wrote:
>>>> Add support for "broken-ddr50", "broken-64-bit-dma"
>>>> and "broken-timeout-value" device tree properties.
>>>> The properties will cause the corresponding quirks bits to be set.
>>>> This allows some of the platform specific QUIRKS setting to be
>>>> moved out of the driver and into the Device Tree node.
>>>>
>>>> Signed-off-by: Al Cooper <alcooperx@gmail.com>
>>>> ---
>>>>  drivers/mmc/host/sdhci-pltfm.c | 8 +++++++-
>>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
>>>> index 87fb5ea..cc0730c 100644
>>>> --- a/drivers/mmc/host/sdhci-pltfm.c
>>>> +++ b/drivers/mmc/host/sdhci-pltfm.c
>>>> @@ -90,10 +90,14 @@ void sdhci_get_of_property(struct platform_device *pdev)
>>>>         if (of_get_property(np, "no-1-8-v", NULL))
>>>>                 host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
>>>>
>>>> +       if (of_get_property(np, "broken-64-bit-dma", NULL))
>>>> +               host->quirks2 |= SDHCI_QUIRK2_BROKEN_64_BIT_DMA;
>>>> +
>>>
>>> To me this is yet another step in the wrong direction of sdhci.
>>>
>>> Not only have we added a quirk which we likely could avoided, but now
>>> you also suggest to add a DT binding for it.
>>>
>>> Please try to avoid this, we shouldn't need to add one DT binding per
>>> quirk, right?
>>
>> The BRCMSTB SDHCI driver needs to support a mix of existing and future
>> ARM and MIPS SoC's on a variety of boards, and many of the
>> combinations have issues. In older separate MIPS/ARM internal versions
>> of the driver, issues were handled by a combination of QUIRKS and the
>> use of custom SDHCI registers that allowed the Host CAPS registers to
>> be changed on a per chip basis. While working on a unified driver for
>> 4.1, I realized that almost all the issues solved by overriding the
>> CAPs registers could also be done using an existing QUIRK and if I had
>> device tree properties that set the QUIRKs, that all the chip/board
>> specific knowledge would end up in the device tree node instead of in
>> the driver. This also allowed me to stop using the non-standard CAPS
>> override registers that tended to change per chip. This approach also
>> allows the driver to work, without change, on future chips/boards as
>> long as they don't have any new issues and they have the proper device
>> tree node.
>>
>> Would there be any objection to me continuing this approach if I moved
>> all the functionality into the sdhci-brcm.c driver and left sdhci.c
>> and sdhci-pltfm.c untouched?
>
> Let me elaborate a bit on what I think you/we need to do.
>
> The problem with the quirks is that these exists because of how the
> sdhci code is designed.
>
> Sdhci core should have been implemented as a pure library providing a
> set of common functions. Today it's more of a driver than a library.
> All, or at least most of the variant specific code, should have been
> dealt from each of the sdhci variant drivers, instead of via quirks
> and sdhci callbacks.
>
> If you didn't notice my recent statement regarding the above on
> linux-mmc, let's me say it again. I have a reached a point where I
> think sdhci has become unmaintainable (and non-optimized coding wise)
> and I am therefore encouraging people to stop adding new quirks or
> sdhci callbacks, but instead "libraryfy" sdhci. I would really
> appreciate if you could help moving sdhci in that direction!
>
> That said, you won't be able to leave the sdhci core code untouched,
> because that would in principle mean that you will need to duplicate
> much of the sdhci code into your sdhci-brcm driver. Instead, try to
> split-up/re-factor common sdhci code into library functions, which
> your sdhci-brcm driver can use.
>
> I guess one of the hardest part in this effort will be to not break
> existing sdhci variant drivers. Let's do our best in that effort, but
> we will rely on more people to participate in testing/coding.
>
>>
>>>
>>>>         if (of_device_is_compatible(np, "fsl,p2020-rev1-esdhc"))
>>>>                 host->quirks |= SDHCI_QUIRK_BROKEN_DMA;
>>>>
>>>> -       if (of_device_is_compatible(np, "fsl,p2020-esdhc") ||
>>>> +       if (of_get_property(np, "broken-timeout-value", NULL) ||
>>>> +           of_device_is_compatible(np, "fsl,p2020-esdhc") ||
>>>>             of_device_is_compatible(np, "fsl,p1010-esdhc") ||
>>>>             of_device_is_compatible(np, "fsl,t4240-esdhc") ||
>>>>             of_device_is_compatible(np, "fsl,mpc8536-esdhc"))
>>>> @@ -106,6 +110,8 @@ void sdhci_get_of_property(struct platform_device *pdev)
>>>>
>>>>         if (of_find_property(np, "enable-sdio-wakeup", NULL))
>>>>                 host->mmc->pm_caps |= MMC_PM_WAKE_SDIO_IRQ;
>>>> +       if (of_find_property(np, "broken-ddr50", NULL))
>>>> +               host->quirks2 |= SDHCI_QUIRK2_BROKEN_DDR50;
>>>
>>> Same comment as above.
>>>
>>> Let me also refer to the response from Scott Branden to the earlier
>>> version of this patchset.
>>> http://permalink.gmane.org/gmane.linux.kernel.mmc/34614
>>>
>>> Perhaps we should invent a sdhci library function, which each sdhci
>>> variant can use to set capabilities through. Typically useful for
>>> those variants that have a non-reliable capabilities register.
>>>
>>
>> Do you mean something more than using SDHCI_QUIRK_MISSING_CAPS which
>> allows the driver to supply the 2 CAPS registers instead of having
>> sdhci.c get them by reading the Host CAPS registers?
>
> Yes, something like that, but preferably without needing to use *any* quirk.
>
> From an sdhci core perspective there could be a library function which
> parses the Host CAPS register. Some sdhci variants could use that
> function and some couldn't since the informations in the CAPS register
> isn't reliable.

As a possible first step, what about the following:
Change sdhci_add_host() to take an additional parameter that, if not
NULL, is a pointer to a data structure that contains overrides for
CAPS and CAPS1 in the form of a mask/value pair. These pairs would be
used by sdhci_add_host() to modify any field after reading the CAPS
and CAPS1 registers. With this in place it looks like the following
QUIRKS would no longer be needed because they could be handled in the
variant drivers using overrides:
SDHCI_QUIRK_FORCE_DMA
SDHCI_QUIRK_BROKEN_DMA
SDHCI_QUIRK_BROKEN_ADMA
SDHCI_QUIRK_FORCE_BLK_SZ_2048
SDHCI_QUIRK_MISSING_CAPS
SDHCI_QUIRK2_NO_1_8_V
SDHCI_QUIRK2_BROKEN_DDR50
SDHCI_QUIRK2_BROKEN_64_BIT_DMA

I would add device tree properties for the overrides to my variant
driver. I could also add the overrides as module params. There are
also many drivers that hook the sdhci_readl() routine to modify the
CAPS fields that could be changed to use the overrides.

With this change, my variant driver that supports more than 10
different MIPS/ARM SoC's would be very small with all the SoC
differences handled using overrides in the device tree and without the
use of any QUIRKS.

>
>>
>>>>  }
>>>>  #else
>>>>  void sdhci_get_of_property(struct platform_device *pdev) {}
>>>> --
>>>> 1.9.0.138.g2de3478
>>>>
>>>
>>> Kind regards
>>> Uffe
>>
>> Thanks
>> Al
>
> Kind regards
> Uffe

Thanks
Al