[PATCH 0/5 RFC] mmc: sdhci-iproc: add bcm2835 support

[PATCH 0/5 RFC] mmc: sdhci-iproc: add bcm2835 support

[Stefan Wahren]

This patch series based on the suggestion of Scott Branden to implement
bcm2835 support into sdhci-iproc [1].

These changes provide the following advantages:
  * no need to maintain a separate driver for bcm2835
  * improve support and test coverage of bcm2835 mmc support
  * get the rid of SDHCI_CAPABILITIES hack in bcm2835_sdhci_readl
  * increase write performance by avoiding delays in write ops

Patch 1 and 2 adopt two sdhci-bcm2835 fixes to sdhci-iproc. The third patch
tries to specify as much quirks as possible in the bcm283x.dtsi as possible.
Patch 4 and 5 are the actually changes for bcm2835 support.

These series has been tested on Raspberry Pi B with dd (direct I/O):

sdhci-bcm2835

378 kB/s WRITE
10,6 MB/s READ

sdhci-iproc (after applying patch series)

881 kB/s WRITE
10,8 MB/s READ

[1] - https://lkml.org/lkml/2015/12/22/525

Stefan Wahren (5):
  mmc: sdhci-iproc: Clean up platform allocations if shdci init fails
  mmc: sdhci-iproc: Actually enable the clock
  ARM: bcm283x: specify sdhci quirks in dtsi file
  mmc: sdhci-iproc: add bcm2835 support
  DT: sdhci-iproc: add bcm2835 compatible

 .../devicetree/bindings/mmc/brcm,sdhci-iproc.txt   |    5 ++-
 arch/arm/boot/dts/bcm283x.dtsi                     |    2 ++
 drivers/mmc/host/Kconfig                           |    6 ++--
 drivers/mmc/host/sdhci-iproc.c                     |   33 ++++++++++++++++++--
 4 files changed, 39 insertions(+), 7 deletions(-)

-- 
1.7.9.5

[PATCH 1/5 RFC] mmc: sdhci-iproc: Clean up platform allocations if shdci init fails

[Stefan Wahren]

This patch adopts the changes from 475c9e43bfa7 ("mmc: sdhci-bcm2835: 
Clean up platform allocations if sdhci init fails") to sdhci-iproc.

Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
---
 drivers/mmc/host/sdhci-iproc.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
index 3b423b0..e22060a 100644
--- a/drivers/mmc/host/sdhci-iproc.c
+++ b/drivers/mmc/host/sdhci-iproc.c
@@ -213,7 +213,11 @@ static int sdhci_iproc_probe(struct platform_device *pdev)
 		host->caps1 = iproc_host->data->caps1;
 	}
 
-	return sdhci_add_host(host);
+	ret = sdhci_add_host(host);
+	if (ret)
+		goto err;
+
+	return 0;
 
 err:
 	sdhci_pltfm_free(pdev);
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5 RFC] mmc: sdhci-iproc: Actually enable the clock

[Stefan Wahren]

The RPi firmware-based clocks driver can actually disable
unused clocks, so when switching to use it we ended up losing
our MMC clock once all devices were probed.

This patch adopts the changes from 1e5a0a9a58e2 ("mmc: sdhci-bcm2835:
Actually enable the clock") to sdhci-iproc.

Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
---
 drivers/mmc/host/sdhci-iproc.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
index e22060a..55bc348 100644
--- a/drivers/mmc/host/sdhci-iproc.c
+++ b/drivers/mmc/host/sdhci-iproc.c
@@ -207,6 +207,11 @@ static int sdhci_iproc_probe(struct platform_device *pdev)
 		ret = PTR_ERR(pltfm_host->clk);
 		goto err;
 	}
+	ret = clk_prepare_enable(pltfm_host->clk);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to enable host clk\n");
+		goto err;
+	}
 
 	if (iproc_host->data->pdata->quirks & SDHCI_QUIRK_MISSING_CAPS) {
 		host->caps = iproc_host->data->caps;
@@ -215,10 +220,12 @@ static int sdhci_iproc_probe(struct platform_device *pdev)
 
 	ret = sdhci_add_host(host);
 	if (ret)
-		goto err;
+		goto err_clk;
 
 	return 0;
 
+err_clk:
+	clk_disable_unprepare(pltfm_host->clk);
 err:
 	sdhci_pltfm_free(pdev);
 	return ret;
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5 RFC] ARM: bcm283x: specify sdhci quirks in dtsi file

[Stefan Wahren]

This patch defines the known quirks for BCM283x in the dtsi file:
* SDHCI_QUIRK_BROKEN_CARD_DETECTION
* SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12

So we don't need to define them in the driver.

Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
---
 arch/arm/boot/dts/bcm283x.dtsi |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/bcm283x.dtsi b/arch/arm/boot/dts/bcm283x.dtsi
index 971e741..75451cd 100644
--- a/arch/arm/boot/dts/bcm283x.dtsi
+++ b/arch/arm/boot/dts/bcm283x.dtsi
@@ -164,6 +164,8 @@
 			reg = <0x7e300000 0x100>;
 			interrupts = <2 30>;
 			clocks = <&clocks BCM2835_CLOCK_EMMC>;
+			broken-cd;
+			sdhci,auto-cmd12;
 			status = "disabled";
 		};
 
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/5 RFC] mmc: sdhci-iproc: add bcm2835 support

[Stefan Wahren]

Scott Branden from Broadcom said that the BCM2835 eMMC IP core is
very similar to IPROC and share most of the quirks. So use this driver
instead of separate one.

The sdhci-iproc contains a better workaround for the clock domain
crossing problem which doesn't need any delays. This results in a
better write performance.

Btw we get the rid of the SDHCI_CAPABILITIES hack in the sdhci_readl
function.

Suggested-by: Scott Branden <sbranden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
---
 drivers/mmc/host/Kconfig       |    6 +++---
 drivers/mmc/host/sdhci-iproc.c |   20 ++++++++++++++++++--
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 1526b8a..60de1e4 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -318,15 +318,15 @@ config MMC_SDHCI_F_SDH30
 	  If unsure, say N.
 
 config MMC_SDHCI_IPROC
-	tristate "SDHCI platform support for the iProc SD/MMC Controller"
-	depends on ARCH_BCM_IPROC || COMPILE_TEST
+	tristate "SDHCI support for the BCM2835 & iProc SD/MMC Controller"
+	depends on ARCH_BCM2835 || ARCH_BCM_IPROC || COMPILE_TEST
 	depends on MMC_SDHCI_PLTFM
 	default ARCH_BCM_IPROC
 	select MMC_SDHCI_IO_ACCESSORS
 	help
 	  This selects the iProc SD/MMC controller.
 
-	  If you have an IPROC platform with SD or MMC devices,
+	  If you have a BCM2835 or IPROC platform with SD or MMC devices,
 	  say Y or M here.
 
 	  If unsure, say N.
diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
index 55bc348..88399eb 100644
--- a/drivers/mmc/host/sdhci-iproc.c
+++ b/drivers/mmc/host/sdhci-iproc.c
@@ -167,7 +167,20 @@ static const struct sdhci_iproc_data iproc_data = {
 	.caps1 = 0x00000064,
 };
 
+static const struct sdhci_pltfm_data sdhci_bcm2835_pltfm_data = {
+	.quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK
+		| SDHCI_QUIRK_MISSING_CAPS,
+	.ops = &sdhci_iproc_ops,
+};
+
+static const struct sdhci_iproc_data bcm2835_data = {
+	.pdata = &sdhci_bcm2835_pltfm_data,
+	.caps = SDHCI_CAN_VDD_330,
+	.caps1 = 0x00000000,
+};
+
 static const struct of_device_id sdhci_iproc_of_match[] = {
+	{ .compatible = "brcm,bcm2835-sdhci", .data = &bcm2835_data },
 	{ .compatible = "brcm,sdhci-iproc-cygnus", .data = &iproc_data },
 	{ }
 };
@@ -180,6 +193,7 @@ static int sdhci_iproc_probe(struct platform_device *pdev)
 	struct sdhci_host *host;
 	struct sdhci_iproc_host *iproc_host;
 	struct sdhci_pltfm_host *pltfm_host;
+	struct device_node *np = pdev->dev.of_node;
 	int ret;
 
 	match = of_match_device(sdhci_iproc_of_match, &pdev->dev);
@@ -199,8 +213,10 @@ static int sdhci_iproc_probe(struct platform_device *pdev)
 	mmc_of_parse(host->mmc);
 	sdhci_get_of_property(pdev);
 
-	/* Enable EMMC 1/8V DDR capable */
-	host->mmc->caps |= MMC_CAP_1_8V_DDR;
+	if (of_device_is_compatible(np, "brcm,sdhci-iproc-cygnus")) {
+		/* Enable EMMC 1/8V DDR capable */
+		host->mmc->caps |= MMC_CAP_1_8V_DDR;
+	}
 
 	pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(pltfm_host->clk)) {
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/5 RFC] DT: sdhci-iproc: add bcm2835 compatible

[Stefan Wahren]

Since sdhci-iproc can support bcm2835 we need to add the
compatible string to the binding.

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 .../devicetree/bindings/mmc/brcm,sdhci-iproc.txt   |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt b/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
index 72cc9cc..a9330a4 100644
--- a/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
+++ b/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
@@ -4,7 +4,10 @@ This file documents differences between the core properties described
 by mmc.txt and the properties that represent the IPROC SDHCI controller.
 
 Required properties:
-- compatible : Should be "brcm,sdhci-iproc-cygnus".
+- compatible : Should be one of the following
+	       "brcm,bcm2835-sdhci"
+	       "brcm,sdhci-iproc-cygnus"
+
 - clocks : The clock feeding the SDHCI controller.
 
 Optional properties:
-- 
1.7.9.5

Re: [PATCH 1/5 RFC] mmc: sdhci-iproc: Clean up platform allocations if shdci init fails

[Scott Branden]

This patch looks fine.

On 16-01-17 06:59 AM, Stefan Wahren wrote:
> This patch adopts the changes from 475c9e43bfa7 ("mmc: sdhci-bcm2835:
> Clean up platform allocations if sdhci init fails") to sdhci-iproc.
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
Acked-by: Scott Branden <sbranden@broadcom.com>

> ---
>   drivers/mmc/host/sdhci-iproc.c |    6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
> index 3b423b0..e22060a 100644
> --- a/drivers/mmc/host/sdhci-iproc.c
> +++ b/drivers/mmc/host/sdhci-iproc.c
> @@ -213,7 +213,11 @@ static int sdhci_iproc_probe(struct platform_device *pdev)
>   		host->caps1 = iproc_host->data->caps1;
>   	}
>
> -	return sdhci_add_host(host);
> +	ret = sdhci_add_host(host);
> +	if (ret)
> +		goto err;
> +
> +	return 0;
>
>   err:
>   	sdhci_pltfm_free(pdev);
>

Re: [PATCH 2/5 RFC] mmc: sdhci-iproc: Actually enable the clock

[Scott Branden]

This patch looks good as well.

On 16-01-17 06:59 AM, Stefan Wahren wrote:
> The RPi firmware-based clocks driver can actually disable
> unused clocks, so when switching to use it we ended up losing
> our MMC clock once all devices were probed.
>
> This patch adopts the changes from 1e5a0a9a58e2 ("mmc: sdhci-bcm2835:
> Actually enable the clock") to sdhci-iproc.
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
Acked-by: Scott Branden <sbranden@broadcom.com>

> ---
>   drivers/mmc/host/sdhci-iproc.c |    9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
> index e22060a..55bc348 100644
> --- a/drivers/mmc/host/sdhci-iproc.c
> +++ b/drivers/mmc/host/sdhci-iproc.c
> @@ -207,6 +207,11 @@ static int sdhci_iproc_probe(struct platform_device *pdev)
>   		ret = PTR_ERR(pltfm_host->clk);
>   		goto err;
>   	}
> +	ret = clk_prepare_enable(pltfm_host->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to enable host clk\n");
> +		goto err;
> +	}
>
>   	if (iproc_host->data->pdata->quirks & SDHCI_QUIRK_MISSING_CAPS) {
>   		host->caps = iproc_host->data->caps;
> @@ -215,10 +220,12 @@ static int sdhci_iproc_probe(struct platform_device *pdev)
>
>   	ret = sdhci_add_host(host);
>   	if (ret)
> -		goto err;
> +		goto err_clk;
>
>   	return 0;
>
> +err_clk:
> +	clk_disable_unprepare(pltfm_host->clk);
>   err:
>   	sdhci_pltfm_free(pdev);
>   	return ret;
>

Re: [PATCH 3/5 RFC] ARM: bcm283x: specify sdhci quirks in dtsi file

[Scott Branden]

looks good

On 16-01-17 06:59 AM, Stefan Wahren wrote:
> This patch defines the known quirks for BCM283x in the dtsi file:
> * SDHCI_QUIRK_BROKEN_CARD_DETECTION
> * SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12
>
> So we don't need to define them in the driver.
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
Acked-by: Scott Branden <sbranden@broadcom.com>

> ---
>   arch/arm/boot/dts/bcm283x.dtsi |    2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm/boot/dts/bcm283x.dtsi b/arch/arm/boot/dts/bcm283x.dtsi
> index 971e741..75451cd 100644
> --- a/arch/arm/boot/dts/bcm283x.dtsi
> +++ b/arch/arm/boot/dts/bcm283x.dtsi
> @@ -164,6 +164,8 @@
>   			reg = <0x7e300000 0x100>;
>   			interrupts = <2 30>;
>   			clocks = <&clocks BCM2835_CLOCK_EMMC>;
> +			broken-cd;
> +			sdhci,auto-cmd12;
>   			status = "disabled";
>   		};
>
>

Re: [PATCH 4/5 RFC] mmc: sdhci-iproc: add bcm2835 support

[Scott Branden]

Hi Stefan,

The caps likely need to be looked at in greater detail. Some comments 
inline.

Regards,
  Scott

On 16-01-17 06:59 AM, Stefan Wahren wrote:
> Scott Branden from Broadcom said that the BCM2835 eMMC IP core is
> very similar to IPROC and share most of the quirks. So use this driver
> instead of separate one.
>
> The sdhci-iproc contains a better workaround for the clock domain
> crossing problem which doesn't need any delays. This results in a
> better write performance.
>
> Btw we get the rid of the SDHCI_CAPABILITIES hack in the sdhci_readl
> function.
>
> Suggested-by: Scott Branden <sbranden@broadcom.com>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
>   drivers/mmc/host/Kconfig       |    6 +++---
>   drivers/mmc/host/sdhci-iproc.c |   20 ++++++++++++++++++--
>   2 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 1526b8a..60de1e4 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -318,15 +318,15 @@ config MMC_SDHCI_F_SDH30
>   	  If unsure, say N.
>
>   config MMC_SDHCI_IPROC
> -	tristate "SDHCI platform support for the iProc SD/MMC Controller"
> -	depends on ARCH_BCM_IPROC || COMPILE_TEST
> +	tristate "SDHCI support for the BCM2835 & iProc SD/MMC Controller"
> +	depends on ARCH_BCM2835 || ARCH_BCM_IPROC || COMPILE_TEST
>   	depends on MMC_SDHCI_PLTFM
>   	default ARCH_BCM_IPROC
>   	select MMC_SDHCI_IO_ACCESSORS
>   	help
>   	  This selects the iProc SD/MMC controller.
>
> -	  If you have an IPROC platform with SD or MMC devices,
> +	  If you have a BCM2835 or IPROC platform with SD or MMC devices,
>   	  say Y or M here.
>
>   	  If unsure, say N.
> diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
> index 55bc348..88399eb 100644
> --- a/drivers/mmc/host/sdhci-iproc.c
> +++ b/drivers/mmc/host/sdhci-iproc.c
> @@ -167,7 +167,20 @@ static const struct sdhci_iproc_data iproc_data = {
>   	.caps1 = 0x00000064,
>   };
>
> +static const struct sdhci_pltfm_data sdhci_bcm2835_pltfm_data = {
> +	.quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK
> +		| SDHCI_QUIRK_MISSING_CAPS,
> +	.ops = &sdhci_iproc_ops,
> +};
> +
> +static const struct sdhci_iproc_data bcm2835_data = {
> +	.pdata = &sdhci_bcm2835_pltfm_data,
> +	.caps = SDHCI_CAN_VDD_330,
> +	.caps1 = 0x00000000,
caps here may be closer to iproc settings - need to review this.

> +};
> +
>   static const struct of_device_id sdhci_iproc_of_match[] = {
> +	{ .compatible = "brcm,bcm2835-sdhci", .data = &bcm2835_data },
>   	{ .compatible = "brcm,sdhci-iproc-cygnus", .data = &iproc_data },
>   	{ }
>   };
> @@ -180,6 +193,7 @@ static int sdhci_iproc_probe(struct platform_device *pdev)
>   	struct sdhci_host *host;
>   	struct sdhci_iproc_host *iproc_host;
>   	struct sdhci_pltfm_host *pltfm_host;
> +	struct device_node *np = pdev->dev.of_node;
>   	int ret;
>
>   	match = of_match_device(sdhci_iproc_of_match, &pdev->dev);
> @@ -199,8 +213,10 @@ static int sdhci_iproc_probe(struct platform_device *pdev)
>   	mmc_of_parse(host->mmc);
>   	sdhci_get_of_property(pdev);
>
> -	/* Enable EMMC 1/8V DDR capable */
> -	host->mmc->caps |= MMC_CAP_1_8V_DDR;
> +	if (of_device_is_compatible(np, "brcm,sdhci-iproc-cygnus")) {
> +		/* Enable EMMC 1/8V DDR capable */
> +		host->mmc->caps |= MMC_CAP_1_8V_DDR;
It is fine to limit this to cygnus.  But, it may work on RPI as well for 
higher speed SD cards...
> +	}
>
>   	pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
>   	if (IS_ERR(pltfm_host->clk)) {
>

Re: [PATCH 5/5 RFC] DT: sdhci-iproc: add bcm2835 compatible

[Scott Branden]

Looks good.

On 16-01-17 06:59 AM, Stefan Wahren wrote:
> Since sdhci-iproc can support bcm2835 we need to add the
> compatible string to the binding.
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
Acked-by: Scott Branden <sbranden@broadcom.com>

> ---
>   .../devicetree/bindings/mmc/brcm,sdhci-iproc.txt   |    5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt b/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
> index 72cc9cc..a9330a4 100644
> --- a/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
> +++ b/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
> @@ -4,7 +4,10 @@ This file documents differences between the core properties described
>   by mmc.txt and the properties that represent the IPROC SDHCI controller.
>
>   Required properties:
> -- compatible : Should be "brcm,sdhci-iproc-cygnus".
> +- compatible : Should be one of the following
> +	       "brcm,bcm2835-sdhci"
> +	       "brcm,sdhci-iproc-cygnus"
> +
>   - clocks : The clock feeding the SDHCI controller.
>
>   Optional properties:
>

Re: [PATCH 4/5 RFC] mmc: sdhci-iproc: add bcm2835 support

[Jaehoon Chung]

On 01/17/2016 11:59 PM, Stefan Wahren wrote:
> Scott Branden from Broadcom said that the BCM2835 eMMC IP core is
> very similar to IPROC and share most of the quirks. So use this driver
> instead of separate one.
> 
> The sdhci-iproc contains a better workaround for the clock domain
> crossing problem which doesn't need any delays. This results in a
> better write performance.
> 
> Btw we get the rid of the SDHCI_CAPABILITIES hack in the sdhci_readl
> function.
> 
> Suggested-by: Scott Branden <sbranden@broadcom.com>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
>  drivers/mmc/host/Kconfig       |    6 +++---
>  drivers/mmc/host/sdhci-iproc.c |   20 ++++++++++++++++++--
>  2 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 1526b8a..60de1e4 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -318,15 +318,15 @@ config MMC_SDHCI_F_SDH30
>  	  If unsure, say N.
>  
>  config MMC_SDHCI_IPROC
> -	tristate "SDHCI platform support for the iProc SD/MMC Controller"
> -	depends on ARCH_BCM_IPROC || COMPILE_TEST
> +	tristate "SDHCI support for the BCM2835 & iProc SD/MMC Controller"
> +	depends on ARCH_BCM2835 || ARCH_BCM_IPROC || COMPILE_TEST
>  	depends on MMC_SDHCI_PLTFM
>  	default ARCH_BCM_IPROC
>  	select MMC_SDHCI_IO_ACCESSORS
>  	help
>  	  This selects the iProc SD/MMC controller.
>  
> -	  If you have an IPROC platform with SD or MMC devices,
> +	  If you have a BCM2835 or IPROC platform with SD or MMC devices,
>  	  say Y or M here.
>  
>  	  If unsure, say N.
> diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
> index 55bc348..88399eb 100644
> --- a/drivers/mmc/host/sdhci-iproc.c
> +++ b/drivers/mmc/host/sdhci-iproc.c
> @@ -167,7 +167,20 @@ static const struct sdhci_iproc_data iproc_data = {
>  	.caps1 = 0x00000064,
>  };
>  
> +static const struct sdhci_pltfm_data sdhci_bcm2835_pltfm_data = {
> +	.quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK
> +		| SDHCI_QUIRK_MISSING_CAPS,
> +	.ops = &sdhci_iproc_ops,
> +};
> +
> +static const struct sdhci_iproc_data bcm2835_data = {
> +	.pdata = &sdhci_bcm2835_pltfm_data,
> +	.caps = SDHCI_CAN_VDD_330,
> +	.caps1 = 0x00000000,
> +};
> +
>  static const struct of_device_id sdhci_iproc_of_match[] = {
> +	{ .compatible = "brcm,bcm2835-sdhci", .data = &bcm2835_data },
>  	{ .compatible = "brcm,sdhci-iproc-cygnus", .data = &iproc_data },
>  	{ }
>  };
> @@ -180,6 +193,7 @@ static int sdhci_iproc_probe(struct platform_device *pdev)
>  	struct sdhci_host *host;
>  	struct sdhci_iproc_host *iproc_host;
>  	struct sdhci_pltfm_host *pltfm_host;
> +	struct device_node *np = pdev->dev.of_node;
>  	int ret;
>  
>  	match = of_match_device(sdhci_iproc_of_match, &pdev->dev);
> @@ -199,8 +213,10 @@ static int sdhci_iproc_probe(struct platform_device *pdev)
>  	mmc_of_parse(host->mmc);
>  	sdhci_get_of_property(pdev);
>  
> -	/* Enable EMMC 1/8V DDR capable */
> -	host->mmc->caps |= MMC_CAP_1_8V_DDR;
> +	if (of_device_is_compatible(np, "brcm,sdhci-iproc-cygnus")) {
> +		/* Enable EMMC 1/8V DDR capable */
> +		host->mmc->caps |= MMC_CAP_1_8V_DDR;
> +	}

Why don't you use the property in device tree?
It can be parsed MMC_CAP_1_8V_DDR as property of "mmc-ddr-1_8v".

Best Regards,
Jaehoon Chung

>  
>  	pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(pltfm_host->clk)) {
> 

Re: [PATCH 4/5 RFC] mmc: sdhci-iproc: add bcm2835 support

[Stefan Wahren]

Hi Scott,

> Scott Branden <sbranden@broadcom.com> hat am 18. Januar 2016 um 22:47
> geschrieben:
>
>
> Hi Stefan,
>
> The caps likely need to be looked at in greater detail. Some comments
> inline.

yes i'm hopeful that someone from Broadcom knows more about the capabilities.
This patch only copies them from the other driver, because i don't want to
introduce new issues.

Unfortunately the BCM2835 datasheet refers only to a non-public Asaran document
( SD3.0_Host_AHB_eMMC4.4_Usersguide_ver5.9_jan11_10.pdf ).

Best regards
Stefan

[1] -
https://www.raspberrypi.org/wp-content/uploads/2012/02/BCM2835-ARM-Peripherals.pdf

Re: [PATCH 4/5 RFC] mmc: sdhci-iproc: add bcm2835 support

[Scott Branden]

Hi Stefan,

On 16-01-19 11:25 AM, Stefan Wahren wrote:
> Hi Scott,
>
>> Scott Branden <sbranden@broadcom.com> hat am 18. Januar 2016 um 22:47
>> geschrieben:
>>
>>
>> Hi Stefan,
>>
>> The caps likely need to be looked at in greater detail. Some comments
>> inline.
>
> yes i'm hopeful that someone from Broadcom knows more about the capabilities.
> This patch only copies them from the other driver, because i don't want to
> introduce new issues.
This patch can go in as is and we can have another patch when more 
accurate capabilities are determined.  If/when I have time I can go and 
deduce what the capability settings should be on BCM2835.  I should be 
revisiting this IP for other reasons in the next month or so and will 
try to look into it.
>
> Unfortunately the BCM2835 datasheet refers only to a non-public Asaran document
> ( SD3.0_Host_AHB_eMMC4.4_Usersguide_ver5.9_jan11_10.pdf ).
Yes, if Arasan allowed such document to be published that would help...
>
> Best regards
> Stefan
>
> [1] -
> https://www.raspberrypi.org/wp-content/uploads/2012/02/BCM2835-ARM-Peripherals.pdf
>

Regards,
Scott

Re: [PATCH 0/5 RFC] mmc: sdhci-iproc: add bcm2835 support

[Eric Anholt]

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

Stefan Wahren <stefan.wahren@i2se.com> writes:

> This patch series based on the suggestion of Scott Branden to implement
> bcm2835 support into sdhci-iproc [1].
>
> These changes provide the following advantages:
>   * no need to maintain a separate driver for bcm2835
>   * improve support and test coverage of bcm2835 mmc support
>   * get the rid of SDHCI_CAPABILITIES hack in bcm2835_sdhci_readl
>   * increase write performance by avoiding delays in write ops
>
> Patch 1 and 2 adopt two sdhci-bcm2835 fixes to sdhci-iproc. The third patch
> tries to specify as much quirks as possible in the bcm283x.dtsi as possible.
> Patch 4 and 5 are the actually changes for bcm2835 support.
>
> These series has been tested on Raspberry Pi B with dd (direct I/O):
>
> sdhci-bcm2835
>
> 378 kB/s WRITE
> 10,6 MB/s READ
>
> sdhci-iproc (after applying patch series)
>
> 881 kB/s WRITE
> 10,8 MB/s READ

I started a while ago at looking at merging the sdhost driver (for the
other controller on these boards that you can use instead), since
apparently the hardware's a better choice to use.  This looks like a
really nice simplification for us for the Arasan mode, though.  Would
you also want to delete the old 2835 driver after this series?

These patches are all:

Acked-by: Eric Anholt <eric@anholt.net>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 5/5 RFC] DT: sdhci-iproc: add bcm2835 compatible

[Rob Herring]

On Sun, Jan 17, 2016 at 02:59:04PM +0000, Stefan Wahren wrote:
> Since sdhci-iproc can support bcm2835 we need to add the
> compatible string to the binding.
> 
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
>  .../devicetree/bindings/mmc/brcm,sdhci-iproc.txt   |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 0/5 RFC] mmc: sdhci-iproc: add bcm2835 support

[Stefan Wahren]

> Eric Anholt <eric@anholt.net> hat am 19. Januar 2016 um 22:00 geschrieben:
>
>
> Stefan Wahren <stefan.wahren@i2se.com> writes:
>
> > This patch series based on the suggestion of Scott Branden to implement
> > bcm2835 support into sdhci-iproc [1].
> >
> > These changes provide the following advantages:
> > * no need to maintain a separate driver for bcm2835
> > * improve support and test coverage of bcm2835 mmc support
> > * get the rid of SDHCI_CAPABILITIES hack in bcm2835_sdhci_readl
> > * increase write performance by avoiding delays in write ops
> >
> > Patch 1 and 2 adopt two sdhci-bcm2835 fixes to sdhci-iproc. The third patch
> > tries to specify as much quirks as possible in the bcm283x.dtsi as possible.
> > Patch 4 and 5 are the actually changes for bcm2835 support.
> >
> > These series has been tested on Raspberry Pi B with dd (direct I/O):
> >
> > sdhci-bcm2835
> >
> > 378 kB/s WRITE
> > 10,6 MB/s READ
> >
> > sdhci-iproc (after applying patch series)
> >
> > 881 kB/s WRITE
> > 10,8 MB/s READ
>
> I started a while ago at looking at merging the sdhost driver (for the
> other controller on these boards that you can use instead), since
> apparently the hardware's a better choice to use. This looks like a
> really nice simplification for us for the Arasan mode, though. Would
> you also want to delete the old 2835 driver after this series?

Yes, that would be a necessary step. I don't know when it's the right time.

What is the usual procedure for such a necessary driver?

>
> These patches are all:
>
> Acked-by: Eric Anholt <eric@anholt.net>

Re: [PATCH 4/5 RFC] mmc: sdhci-iproc: add bcm2835 support

[Stefan Wahren]

> Jaehoon Chung <jh80.chung-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> hat am 19. Januar 2016 um 02:32
> geschrieben:
>
>
> On 01/17/2016 11:59 PM, Stefan Wahren wrote:
> > diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
> > index 55bc348..88399eb 100644
> > --- a/drivers/mmc/host/sdhci-iproc.c
> > +++ b/drivers/mmc/host/sdhci-iproc.c
> > @@ -167,7 +167,20 @@ static const struct sdhci_iproc_data iproc_data = {
> > .caps1 = 0x00000064,
> > };
> >
> > +static const struct sdhci_pltfm_data sdhci_bcm2835_pltfm_data = {
> > + .quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK
> > + | SDHCI_QUIRK_MISSING_CAPS,
> > + .ops = &sdhci_iproc_ops,
> > +};
> > +
> > +static const struct sdhci_iproc_data bcm2835_data = {
> > + .pdata = &sdhci_bcm2835_pltfm_data,
> > + .caps = SDHCI_CAN_VDD_330,
> > + .caps1 = 0x00000000,
> > +};
> > +
> > static const struct of_device_id sdhci_iproc_of_match[] = {
> > + { .compatible = "brcm,bcm2835-sdhci", .data = &bcm2835_data },
> > { .compatible = "brcm,sdhci-iproc-cygnus", .data = &iproc_data },
> > { }
> > };
> > @@ -180,6 +193,7 @@ static int sdhci_iproc_probe(struct platform_device
> > *pdev)
> > struct sdhci_host *host;
> > struct sdhci_iproc_host *iproc_host;
> > struct sdhci_pltfm_host *pltfm_host;
> > + struct device_node *np = pdev->dev.of_node;
> > int ret;
> >
> > match = of_match_device(sdhci_iproc_of_match, &pdev->dev);
> > @@ -199,8 +213,10 @@ static int sdhci_iproc_probe(struct platform_device
> > *pdev)
> > mmc_of_parse(host->mmc);
> > sdhci_get_of_property(pdev);
> >
> > - /* Enable EMMC 1/8V DDR capable */
> > - host->mmc->caps |= MMC_CAP_1_8V_DDR;
> > + if (of_device_is_compatible(np, "brcm,sdhci-iproc-cygnus")) {
> > + /* Enable EMMC 1/8V DDR capable */
> > + host->mmc->caps |= MMC_CAP_1_8V_DDR;
> > + }
>
> Why don't you use the property in device tree?
> It can be parsed MMC_CAP_1_8V_DDR as property of "mmc-ddr-1_8v".

I agree but i don't have a iProc Cygnus to test it and we need to keep
compatibility to older devicetrees. 

>
> Best Regards,
> Jaehoon Chung
>
> >
> > pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
> > if (IS_ERR(pltfm_host->clk)) {
> >
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5 RFC] ARM: bcm283x: specify sdhci quirks in dtsi file

[Stephen Warren]

On 01/17/2016 07:59 AM, Stefan Wahren wrote:
> This patch defines the known quirks for BCM283x in the dtsi file:
> * SDHCI_QUIRK_BROKEN_CARD_DETECTION
> * SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12
> 
> So we don't need to define them in the driver.

Given that the quirks are currently defined in the driver, I believe
they will forever have to be defined in the driver.

If the (or a different) driver suddenly starts requiring the DT to
specify the quirks, then old DTs won't work with a new kernel, which is
against DT ABI.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/5 RFC] mmc: sdhci-iproc: add bcm2835 support

[Stephen Warren]

On 01/17/2016 07:59 AM, Stefan Wahren wrote:
> Scott Branden from Broadcom said that the BCM2835 eMMC IP core is
> very similar to IPROC and share most of the quirks. So use this driver
> instead of separate one.
> 
> The sdhci-iproc contains a better workaround for the clock domain
> crossing problem which doesn't need any delays. This results in a
> better write performance.
> 
> Btw we get the rid of the SDHCI_CAPABILITIES hack in the sdhci_readl
> function.

> diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c

> +static const struct sdhci_pltfm_data sdhci_bcm2835_pltfm_data = {
> +	.quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK
> +		| SDHCI_QUIRK_MISSING_CAPS,
> +	.ops = &sdhci_iproc_ops,
> +};
> +
> +static const struct sdhci_iproc_data bcm2835_data = {
> +	.pdata = &sdhci_bcm2835_pltfm_data,
> +	.caps = SDHCI_CAN_VDD_330,
> +	.caps1 = 0x00000000,
> +};

> @@ -199,8 +213,10 @@ static int sdhci_iproc_probe(struct platform_device *pdev)

> -	/* Enable EMMC 1/8V DDR capable */
> -	host->mmc->caps |= MMC_CAP_1_8V_DDR;
> +	if (of_device_is_compatible(np, "brcm,sdhci-iproc-cygnus")) {
> +		/* Enable EMMC 1/8V DDR capable */
> +		host->mmc->caps |= MMC_CAP_1_8V_DDR;
> +	}

Rather that placing device-specific of_device_is_compatible() throughout
the driver, I think it'd be better to key off a field in bcm2835_data or
sdhci_bcm2835_pltfm_data. That way, if the driver starts supporting
additional devices that want this MMC_CAP_1_8V_DDR, you can simply set
that up in the relevant data structure, rather than adding a mess of
of_device_is_compatible("a") || of_device_is_compatible("b") || ... to
the code.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5 RFC] mmc: sdhci-iproc: Actually enable the clock

[Ulf Hansson]

On 17 January 2016 at 15:59, Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org> wrote:
> The RPi firmware-based clocks driver can actually disable
> unused clocks, so when switching to use it we ended up losing
> our MMC clock once all devices were probed.
>
> This patch adopts the changes from 1e5a0a9a58e2 ("mmc: sdhci-bcm2835:
> Actually enable the clock") to sdhci-iproc.
>
> Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
> ---
>  drivers/mmc/host/sdhci-iproc.c |    9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
> index e22060a..55bc348 100644
> --- a/drivers/mmc/host/sdhci-iproc.c
> +++ b/drivers/mmc/host/sdhci-iproc.c
> @@ -207,6 +207,11 @@ static int sdhci_iproc_probe(struct platform_device *pdev)
>                 ret = PTR_ERR(pltfm_host->clk);
>                 goto err;
>         }
> +       ret = clk_prepare_enable(pltfm_host->clk);
> +       if (ret) {
> +               dev_err(&pdev->dev, "failed to enable host clk\n");
> +               goto err;
> +       }
>
>         if (iproc_host->data->pdata->quirks & SDHCI_QUIRK_MISSING_CAPS) {
>                 host->caps = iproc_host->data->caps;
> @@ -215,10 +220,12 @@ static int sdhci_iproc_probe(struct platform_device *pdev)
>
>         ret = sdhci_add_host(host);
>         if (ret)
> -               goto err;
> +               goto err_clk;
>
>         return 0;
>
> +err_clk:
> +       clk_disable_unprepare(pltfm_host->clk);
>  err:
>         sdhci_pltfm_free(pdev);
>         return ret;
> --
> 1.7.9.5
>

There's a missing clk_disable_unprepare() from the ->remove()
callback. Otherwise this looks good.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5 RFC] mmc: sdhci-iproc: Clean up platform allocations if shdci init fails

[Ulf Hansson]

On 17 January 2016 at 15:59, Stefan Wahren <stefan.wahren@i2se.com> wrote:
> This patch adopts the changes from 475c9e43bfa7 ("mmc: sdhci-bcm2835:
> Clean up platform allocations if sdhci init fails") to sdhci-iproc.
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>

Thanks, applied for next!

Kind regards
Uffe

> ---
>  drivers/mmc/host/sdhci-iproc.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
> index 3b423b0..e22060a 100644
> --- a/drivers/mmc/host/sdhci-iproc.c
> +++ b/drivers/mmc/host/sdhci-iproc.c
> @@ -213,7 +213,11 @@ static int sdhci_iproc_probe(struct platform_device *pdev)
>                 host->caps1 = iproc_host->data->caps1;
>         }
>
> -       return sdhci_add_host(host);
> +       ret = sdhci_add_host(host);
> +       if (ret)
> +               goto err;
> +
> +       return 0;
>
>  err:
>         sdhci_pltfm_free(pdev);
> --
> 1.7.9.5
>

Re: [PATCH 2/5 RFC] mmc: sdhci-iproc: Actually enable the clock

[Stefan Wahren]

Hi,

> Ulf Hansson <ulf.hansson@linaro.org> hat am 27. Januar 2016 um 15:16
> geschrieben:
>
>
> On 17 January 2016 at 15:59, Stefan Wahren <stefan.wahren@i2se.com> wrote:
> > The RPi firmware-based clocks driver can actually disable
> > unused clocks, so when switching to use it we ended up losing
> > our MMC clock once all devices were probed.
> >
> > This patch adopts the changes from 1e5a0a9a58e2 ("mmc: sdhci-bcm2835:
> > Actually enable the clock") to sdhci-iproc.
> >
> > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> > ---
> > drivers/mmc/host/sdhci-iproc.c | 9 ++++++++-
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
> > index e22060a..55bc348 100644
> > --- a/drivers/mmc/host/sdhci-iproc.c
> > +++ b/drivers/mmc/host/sdhci-iproc.c
> > @@ -207,6 +207,11 @@ static int sdhci_iproc_probe(struct platform_device
> > *pdev)
> > ret = PTR_ERR(pltfm_host->clk);
> > goto err;
> > }
> > + ret = clk_prepare_enable(pltfm_host->clk);
> > + if (ret) {
> > + dev_err(&pdev->dev, "failed to enable host clk\n");
> > + goto err;
> > + }
> >
> > if (iproc_host->data->pdata->quirks & SDHCI_QUIRK_MISSING_CAPS) {
> > host->caps = iproc_host->data->caps;
> > @@ -215,10 +220,12 @@ static int sdhci_iproc_probe(struct platform_device
> > *pdev)
> >
> > ret = sdhci_add_host(host);
> > if (ret)
> > - goto err;
> > + goto err_clk;
> >
> > return 0;
> >
> > +err_clk:
> > + clk_disable_unprepare(pltfm_host->clk);
> > err:
> > sdhci_pltfm_free(pdev);
> > return ret;
> > --
> > 1.7.9.5
> >
>
> There's a missing clk_disable_unprepare() from the ->remove()
> callback. Otherwise this looks good.

this shouldn't be necessary, because clk_disable_unprepare() is already called
by sdhci_pltfm_unregister().

Regards
Stefan

>
> Kind regards
> Uffe

Re: [PATCH 2/5 RFC] mmc: sdhci-iproc: Actually enable the clock

[Ulf Hansson]

On 27 January 2016 at 20:11, Stefan Wahren <stefan.wahren@i2se.com> wrote:
> Hi,
>
>> Ulf Hansson <ulf.hansson@linaro.org> hat am 27. Januar 2016 um 15:16
>> geschrieben:
>>
>>
>> On 17 January 2016 at 15:59, Stefan Wahren <stefan.wahren@i2se.com> wrote:
>> > The RPi firmware-based clocks driver can actually disable
>> > unused clocks, so when switching to use it we ended up losing
>> > our MMC clock once all devices were probed.
>> >
>> > This patch adopts the changes from 1e5a0a9a58e2 ("mmc: sdhci-bcm2835:
>> > Actually enable the clock") to sdhci-iproc.
>> >
>> > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>> > ---
>> > drivers/mmc/host/sdhci-iproc.c | 9 ++++++++-
>> > 1 file changed, 8 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
>> > index e22060a..55bc348 100644
>> > --- a/drivers/mmc/host/sdhci-iproc.c
>> > +++ b/drivers/mmc/host/sdhci-iproc.c
>> > @@ -207,6 +207,11 @@ static int sdhci_iproc_probe(struct platform_device
>> > *pdev)
>> > ret = PTR_ERR(pltfm_host->clk);
>> > goto err;
>> > }
>> > + ret = clk_prepare_enable(pltfm_host->clk);
>> > + if (ret) {
>> > + dev_err(&pdev->dev, "failed to enable host clk\n");
>> > + goto err;
>> > + }
>> >
>> > if (iproc_host->data->pdata->quirks & SDHCI_QUIRK_MISSING_CAPS) {
>> > host->caps = iproc_host->data->caps;
>> > @@ -215,10 +220,12 @@ static int sdhci_iproc_probe(struct platform_device
>> > *pdev)
>> >
>> > ret = sdhci_add_host(host);
>> > if (ret)
>> > - goto err;
>> > + goto err_clk;
>> >
>> > return 0;
>> >
>> > +err_clk:
>> > + clk_disable_unprepare(pltfm_host->clk);
>> > err:
>> > sdhci_pltfm_free(pdev);
>> > return ret;
>> > --
>> > 1.7.9.5
>> >
>>
>> There's a missing clk_disable_unprepare() from the ->remove()
>> callback. Otherwise this looks good.
>
> this shouldn't be necessary, because clk_disable_unprepare() is already called
> by sdhci_pltfm_unregister().

Huh. That's yet another strange behaviour of the sdhci core.

So there's no clk enabling done via the registering API, but disabling
is done at unregistering. It's fragile.

That also means there is a clk disable unbalance issue at ->remove()
for sdhci-iproc, right!?

Anyway, the patch is of course correct. Applied for next!

Thanks and kind regards
Uffe

Re: [PATCH 0/5 RFC] mmc: sdhci-iproc: add bcm2835 support

[Eric Anholt]

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

Stefan Wahren <stefan.wahren@i2se.com> writes:

>> Eric Anholt <eric@anholt.net> hat am 19. Januar 2016 um 22:00 geschrieben:
>>
>>
>> Stefan Wahren <stefan.wahren@i2se.com> writes:
>>
>> > This patch series based on the suggestion of Scott Branden to implement
>> > bcm2835 support into sdhci-iproc [1].
>> >
>> > These changes provide the following advantages:
>> > * no need to maintain a separate driver for bcm2835
>> > * improve support and test coverage of bcm2835 mmc support
>> > * get the rid of SDHCI_CAPABILITIES hack in bcm2835_sdhci_readl
>> > * increase write performance by avoiding delays in write ops
>> >
>> > Patch 1 and 2 adopt two sdhci-bcm2835 fixes to sdhci-iproc. The third patch
>> > tries to specify as much quirks as possible in the bcm283x.dtsi as possible.
>> > Patch 4 and 5 are the actually changes for bcm2835 support.
>> >
>> > These series has been tested on Raspberry Pi B with dd (direct I/O):
>> >
>> > sdhci-bcm2835
>> >
>> > 378 kB/s WRITE
>> > 10,6 MB/s READ
>> >
>> > sdhci-iproc (after applying patch series)
>> >
>> > 881 kB/s WRITE
>> > 10,8 MB/s READ
>>
>> I started a while ago at looking at merging the sdhost driver (for the
>> other controller on these boards that you can use instead), since
>> apparently the hardware's a better choice to use. This looks like a
>> really nice simplification for us for the Arasan mode, though. Would
>> you also want to delete the old 2835 driver after this series?
>
> Yes, that would be a necessary step. I don't know when it's the right time.
>
> What is the usual procedure for such a necessary driver?

If the kernel still supports the hardware, just in a different source
file, I don't see any problem with removing it immediately.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]