[PATCH 1/4] mmc: Add quirk to disable SDR50 mode

[PATCH 1/4] mmc: Add quirk to disable SDR50 mode

[Al Cooper]

Add quirk to disable SDR50 mode for controllers/boards that have
problems with this mode.

Signed-off-by: Al Cooper <alcooperx@gmail.com>
---
 drivers/mmc/host/sdhci.c | 3 +++
 drivers/mmc/host/sdhci.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index b48565e..71067c7 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3176,6 +3176,9 @@ int sdhci_add_host(struct sdhci_host *host)
 	} else if (caps[1] & SDHCI_SUPPORT_SDR50)
 		mmc->caps |= MMC_CAP_UHS_SDR50;
 
+	if (host->quirks2 & SDHCI_QUIRK2_BROKEN_SDR50)
+		mmc->caps &= ~MMC_CAP_UHS_SDR50;
+
 	if (host->quirks2 & SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400 &&
 	    (caps[1] & SDHCI_SUPPORT_HS400))
 		mmc->caps2 |= MMC_CAP2_HS400;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 9d4aa31..0941c94 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -412,6 +412,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK2_ACMD23_BROKEN			(1<<14)
 /* Broken Clock divider zero in controller */
 #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN		(1<<15)
+/* Controller does not support SDR50 */
+#define SDHCI_QUIRK2_BROKEN_SDR50			(1<<16)
 /*
  * When internal clock is disabled, a delay is needed before modifying the
  * SD clock frequency or enabling back the internal clock.
-- 
1.9.0.138.g2de3478

[PATCH 2/4] mmc: sdhci-pltfm: Add DT properties to set various QUIRKS

[Al Cooper]

Add support for "broken-sdr50", "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 | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index 87fb5ea..81eb4d2 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,10 @@ 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-sdr50", NULL))
+		host->quirks2 |= SDHCI_QUIRK2_BROKEN_SDR50;
+	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 3/4] 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 4/4] 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      |  16 +++
 drivers/mmc/host/Kconfig                           |  12 ++
 drivers/mmc/host/Makefile                          |   1 +
 drivers/mmc/host/sdhci-brcmstb.c                   | 143 +++++++++++++++++++++
 4 files changed, 172 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..df08173
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/sdhci-brcmstb.txt
@@ -0,0 +1,16 @@
+* 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"
+
+Example:
+
+	sdhci@f03e0100 {
+		compatible = "brcm,sdhci-brcmstb";
+		reg = <0xf03e0000 0x100>;
+		interrupts = <0x0 0x26 0x0>;
+		sdhci,auto-cmd12;
+	};
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..fb5d558
--- /dev/null
+++ b/drivers/mmc/host/sdhci-brcmstb.c
@@ -0,0 +1,143 @@
+/*
+ * 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;
+}
+
+#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);
+
+	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 1/4] mmc: Add quirk to disable SDR50 mode

[Ulf Hansson]

On 5 November 2015 at 23:39, Al Cooper <alcooperx@gmail.com> wrote:
> Add quirk to disable SDR50 mode for controllers/boards that have
> problems with this mode.

No thanks! No more quirks please!

Kind regards
Uffe

>
> Signed-off-by: Al Cooper <alcooperx@gmail.com>
> ---
>  drivers/mmc/host/sdhci.c | 3 +++
>  drivers/mmc/host/sdhci.h | 2 ++
>  2 files changed, 5 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index b48565e..71067c7 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -3176,6 +3176,9 @@ int sdhci_add_host(struct sdhci_host *host)
>         } else if (caps[1] & SDHCI_SUPPORT_SDR50)
>                 mmc->caps |= MMC_CAP_UHS_SDR50;
>
> +       if (host->quirks2 & SDHCI_QUIRK2_BROKEN_SDR50)
> +               mmc->caps &= ~MMC_CAP_UHS_SDR50;
> +
>         if (host->quirks2 & SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400 &&
>             (caps[1] & SDHCI_SUPPORT_HS400))
>                 mmc->caps2 |= MMC_CAP2_HS400;
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 9d4aa31..0941c94 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -412,6 +412,8 @@ struct sdhci_host {
>  #define SDHCI_QUIRK2_ACMD23_BROKEN                     (1<<14)
>  /* Broken Clock divider zero in controller */
>  #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN             (1<<15)
> +/* Controller does not support SDR50 */
> +#define SDHCI_QUIRK2_BROKEN_SDR50                      (1<<16)
>  /*
>   * When internal clock is disabled, a delay is needed before modifying the
>   * SD clock frequency or enabling back the internal clock.
> --
> 1.9.0.138.g2de3478
>

Re: [PATCH 1/4] mmc: Add quirk to disable SDR50 mode

[Alan Cooper]

On Fri, Nov 6, 2015 at 3:14 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> No thanks! No more quirks please!

OK, I'll move this functionality into the sdhci-brcmstb driver and
re-submit the patch set.

Thanks
Al

Re: [PATCH 1/4] mmc: Add quirk to disable SDR50 mode

[Scott Branden]

Hi Ulf,

On 15-11-06 12:14 AM, Ulf Hansson wrote:
> On 5 November 2015 at 23:39, Al Cooper <alcooperx@gmail.com> wrote:
>> Add quirk to disable SDR50 mode for controllers/boards that have
>> problems with this mode.
>
> No thanks! No more quirks please!
>

I'm fine with not having this quirk added (I don't need this one but use 
multiple of the other quirks in the driver)  But, what if I also needed 
it in my driver?  When do we determine when a quirk should be added to 
sdhci.c or not.  What about existing quirks - should the current ones be 
moved to multiple existing drivers?
> Kind regards
> Uffe
>
>>
>> Signed-off-by: Al Cooper <alcooperx@gmail.com>
>> ---
>>   drivers/mmc/host/sdhci.c | 3 +++
>>   drivers/mmc/host/sdhci.h | 2 ++
>>   2 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index b48565e..71067c7 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -3176,6 +3176,9 @@ int sdhci_add_host(struct sdhci_host *host)
>>          } else if (caps[1] & SDHCI_SUPPORT_SDR50)
>>                  mmc->caps |= MMC_CAP_UHS_SDR50;
>>
>> +       if (host->quirks2 & SDHCI_QUIRK2_BROKEN_SDR50)
>> +               mmc->caps &= ~MMC_CAP_UHS_SDR50;
>> +
Perhaps a lot of these quirks can be solved by having a generic 
mechanism to override any of the values in the caps registers rather 
than adding all these quirks?

>>          if (host->quirks2 & SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400 &&
>>              (caps[1] & SDHCI_SUPPORT_HS400))
>>                  mmc->caps2 |= MMC_CAP2_HS400;
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index 9d4aa31..0941c94 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -412,6 +412,8 @@ struct sdhci_host {
>>   #define SDHCI_QUIRK2_ACMD23_BROKEN                     (1<<14)
>>   /* Broken Clock divider zero in controller */
>>   #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN             (1<<15)
>> +/* Controller does not support SDR50 */
>> +#define SDHCI_QUIRK2_BROKEN_SDR50                      (1<<16)
>>   /*
>>    * When internal clock is disabled, a delay is needed before modifying the
>>    * SD clock frequency or enabling back the internal clock.
>> --
>> 1.9.0.138.g2de3478
>>

Regards,
  Scott

Re: [PATCH 1/4] mmc: Add quirk to disable SDR50 mode

[Florian Fainelli]

On 06/11/15 15:55, Scott Branden wrote:
> Hi Ulf,
> 
> On 15-11-06 12:14 AM, Ulf Hansson wrote:
>> On 5 November 2015 at 23:39, Al Cooper <alcooperx@gmail.com> wrote:
>>> Add quirk to disable SDR50 mode for controllers/boards that have
>>> problems with this mode.
>>
>> No thanks! No more quirks please!
>>
> 
> I'm fine with not having this quirk added (I don't need this one but use
> multiple of the other quirks in the driver)  But, what if I also needed
> it in my driver?  When do we determine when a quirk should be added to
> sdhci.c or not.  What about existing quirks - should the current ones be
> moved to multiple existing drivers?
>> Kind regards
>> Uffe
>>
>>>
>>> Signed-off-by: Al Cooper <alcooperx@gmail.com>
>>> ---
>>>   drivers/mmc/host/sdhci.c | 3 +++
>>>   drivers/mmc/host/sdhci.h | 2 ++
>>>   2 files changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index b48565e..71067c7 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -3176,6 +3176,9 @@ int sdhci_add_host(struct sdhci_host *host)
>>>          } else if (caps[1] & SDHCI_SUPPORT_SDR50)
>>>                  mmc->caps |= MMC_CAP_UHS_SDR50;
>>>
>>> +       if (host->quirks2 & SDHCI_QUIRK2_BROKEN_SDR50)
>>> +               mmc->caps &= ~MMC_CAP_UHS_SDR50;
>>> +
> Perhaps a lot of these quirks can be solved by having a generic
> mechanism to override any of the values in the caps registers rather
> than adding all these quirks?

Are the capabilities register override specific to the Arasan controller
or is there a generic and well define SDIO configuration register for
these registers? The register information I am looking at seems to
suggest this is part of how you glue your SDIO controller to your SoC.

The entire purpose of Al's changes were precisely so we do not have to
fiddle with these capabilities register like we are currently doing in
some versions of our downstream kernel.
-- 
Florian

Re: [PATCH 1/4] mmc: Add quirk to disable SDR50 mode

[Ulf Hansson]

On 7 November 2015 at 00:55, Scott Branden <sbranden@broadcom.com> wrote:
> Hi Ulf,
>
> On 15-11-06 12:14 AM, Ulf Hansson wrote:
>>
>> On 5 November 2015 at 23:39, Al Cooper <alcooperx@gmail.com> wrote:
>>>
>>> Add quirk to disable SDR50 mode for controllers/boards that have
>>> problems with this mode.
>>
>>
>> No thanks! No more quirks please!
>>
>
> I'm fine with not having this quirk added (I don't need this one but use
> multiple of the other quirks in the driver)  But, what if I also needed it
> in my driver?  When do we determine when a quirk should be added to sdhci.c
> or not.  What about existing quirks - should the current ones be moved to
> multiple existing drivers?

The sdhci driver should turn into a library providing generic helper
functions. Each host can then pick which functions to use and also
deal with its own "quirks", instead of managing those in generic code.

I guess we might end up getting a bit more code in total, but on the
other hand the code would be better optimized and also maintainable.

>>
>> Kind regards
>> Uffe
>>
>>>
>>> Signed-off-by: Al Cooper <alcooperx@gmail.com>
>>> ---
>>>   drivers/mmc/host/sdhci.c | 3 +++
>>>   drivers/mmc/host/sdhci.h | 2 ++
>>>   2 files changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index b48565e..71067c7 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -3176,6 +3176,9 @@ int sdhci_add_host(struct sdhci_host *host)
>>>          } else if (caps[1] & SDHCI_SUPPORT_SDR50)
>>>                  mmc->caps |= MMC_CAP_UHS_SDR50;
>>>
>>> +       if (host->quirks2 & SDHCI_QUIRK2_BROKEN_SDR50)
>>> +               mmc->caps &= ~MMC_CAP_UHS_SDR50;
>>> +
>
> Perhaps a lot of these quirks can be solved by having a generic mechanism to
> override any of the values in the caps registers rather than adding all
> these quirks?

Sure, it makes sense if it can decreases the number of quirks!

>
>>>          if (host->quirks2 & SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400 &&
>>>              (caps[1] & SDHCI_SUPPORT_HS400))
>>>                  mmc->caps2 |= MMC_CAP2_HS400;
>>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>>> index 9d4aa31..0941c94 100644
>>> --- a/drivers/mmc/host/sdhci.h
>>> +++ b/drivers/mmc/host/sdhci.h
>>> @@ -412,6 +412,8 @@ struct sdhci_host {
>>>   #define SDHCI_QUIRK2_ACMD23_BROKEN                     (1<<14)
>>>   /* Broken Clock divider zero in controller */
>>>   #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN             (1<<15)
>>> +/* Controller does not support SDR50 */
>>> +#define SDHCI_QUIRK2_BROKEN_SDR50                      (1<<16)
>>>   /*
>>>    * When internal clock is disabled, a delay is needed before modifying
>>> the
>>>    * SD clock frequency or enabling back the internal clock.
>>> --
>>> 1.9.0.138.g2de3478
>>>
>
> Regards,
>  Scott

Kind regards
Uffe

Re: [PATCH 1/4] mmc: Add quirk to disable SDR50 mode

[Scott Branden]

On 15-11-09 02:45 AM, Ulf Hansson wrote:
> On 7 November 2015 at 00:55, Scott Branden <sbranden@broadcom.com> wrote:
>> Hi Ulf,
>>
>> On 15-11-06 12:14 AM, Ulf Hansson wrote:
>>>
>>> On 5 November 2015 at 23:39, Al Cooper <alcooperx@gmail.com> wrote:
>>>>
>>>> Add quirk to disable SDR50 mode for controllers/boards that have
>>>> problems with this mode.
>>>
>>>
>>> No thanks! No more quirks please!
>>>
>>
>> I'm fine with not having this quirk added (I don't need this one but use
>> multiple of the other quirks in the driver)  But, what if I also needed it
>> in my driver?  When do we determine when a quirk should be added to sdhci.c
>> or not.  What about existing quirks - should the current ones be moved to
>> multiple existing drivers?
>
> The sdhci driver should turn into a library providing generic helper
> functions. Each host can then pick which functions to use and also
> deal with its own "quirks", instead of managing those in generic code.
>
> I guess we might end up getting a bit more code in total, but on the
> other hand the code would be better optimized and also maintainable.
>

OK, if I need to add any new quirks I will look into this a bit more and 
see if it can be done.  I don't have a need to do this right now so if 
anyone else wants to have a look feel free to.

>>>
>>> Kind regards
>>> Uffe
>>>
>>>>
>>>> Signed-off-by: Al Cooper <alcooperx@gmail.com>
>>>> ---
>>>>    drivers/mmc/host/sdhci.c | 3 +++
>>>>    drivers/mmc/host/sdhci.h | 2 ++
>>>>    2 files changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>>> index b48565e..71067c7 100644
>>>> --- a/drivers/mmc/host/sdhci.c
>>>> +++ b/drivers/mmc/host/sdhci.c
>>>> @@ -3176,6 +3176,9 @@ int sdhci_add_host(struct sdhci_host *host)
>>>>           } else if (caps[1] & SDHCI_SUPPORT_SDR50)
>>>>                   mmc->caps |= MMC_CAP_UHS_SDR50;
>>>>
>>>> +       if (host->quirks2 & SDHCI_QUIRK2_BROKEN_SDR50)
>>>> +               mmc->caps &= ~MMC_CAP_UHS_SDR50;
>>>> +
>>
>> Perhaps a lot of these quirks can be solved by having a generic mechanism to
>> override any of the values in the caps registers rather than adding all
>> these quirks?
>
> Sure, it makes sense if it can decreases the number of quirks!
>
I think it can in some cases.  In fact - the hardware designers do not 
even set the correct settings in the caps register on some of the SoCs. 
  The caps register does not appear to be used in the hardware other 
than for "info" purposes to read by the driver - and when the info is 
wrong this may lead to many of the quirks that have been added by others 
over the years.
>>
>>>>           if (host->quirks2 & SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400 &&
>>>>               (caps[1] & SDHCI_SUPPORT_HS400))
>>>>                   mmc->caps2 |= MMC_CAP2_HS400;
>>>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>>>> index 9d4aa31..0941c94 100644
>>>> --- a/drivers/mmc/host/sdhci.h
>>>> +++ b/drivers/mmc/host/sdhci.h
>>>> @@ -412,6 +412,8 @@ struct sdhci_host {
>>>>    #define SDHCI_QUIRK2_ACMD23_BROKEN                     (1<<14)
>>>>    /* Broken Clock divider zero in controller */
>>>>    #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN             (1<<15)
>>>> +/* Controller does not support SDR50 */
>>>> +#define SDHCI_QUIRK2_BROKEN_SDR50                      (1<<16)
>>>>    /*
>>>>     * When internal clock is disabled, a delay is needed before modifying
>>>> the
>>>>     * SD clock frequency or enabling back the internal clock.
>>>> --
>>>> 1.9.0.138.g2de3478
>>>>
>>
>> Regards,
>>   Scott
>
> Kind regards
> Uffe
>
Thanks,
  Scott