[PATCH 0/8] Armada 38x SDHCI driver improvements

[PATCH 0/8] Armada 38x SDHCI driver improvements

[Marcin Wojtas]

Hi,

This series brings a couple of fixes and improvements to Armada 38x SDHCI
controller driver. First four patches are fixes, of which three are stable
CC'ed.

Another two add DAT3-pin based hardware card detection in the driver, what
should be used by the newest revisions of A388-GP boards.

The last patches enable MMC_CARD bit, using init_card() callback added to
SDHCI hosts.

Any remarks and comments are welcome.

Best regards,
Marcin

Marcin Wojtas (6):
  mmc: sdhci-pxav3: fix error handling of armada_38x_quirks
  mmc: sdhci-pxav3: enable proper resuming on Armada 38x SoC
  mmc: sdhci-pxav3: enable usage of DAT3 pin as HW card detect
  ARM: mvebu: enable SDHCI card detection using DAT3 pin on A388-GP
  mmc: sdhci: add init_card callback to sdhci
  mmc: sdhci-pxav3: enable modifying MMC_CARD bit during card
    initialization

Nadav Haklai (2):
  mmc: sdhci-pxav3: remove broken clock base quirk for Armada 38x sdhci
    driver
  mmc: sdhci-pxav3: disable clock inversion for HS MMC cards

 .../devicetree/bindings/mmc/sdhci-pxa.txt          |   5 +
 arch/arm/boot/dts/armada-388-gp.dts                |   3 +-
 drivers/mmc/host/sdhci-pxav3.c                     | 101 ++++++++++++++++-----
 drivers/mmc/host/sdhci.c                           |  14 ++-
 drivers/mmc/host/sdhci.h                           |   4 +
 5 files changed, 102 insertions(+), 25 deletions(-)

-- 
1.8.3.1

[PATCH 1/8] mmc: sdhci-pxav3: remove broken clock base quirk for Armada 38x sdhci driver

[Marcin Wojtas]

From: Nadav Haklai <nadavh@marvell.com>

shci-pxav3 driver is enabling by default the
SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN quirk. However this quirk is not
required for Armada 38x and leads to wrong clock setting in the divider.

Signed-off-by: Nadav Haklai <nadavh@marvell.com>
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
Cc: <stable@vger.kernel.org> # v4.2
---
 drivers/mmc/host/sdhci-pxav3.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
index 946d37f..976cddd 100644
--- a/drivers/mmc/host/sdhci-pxav3.c
+++ b/drivers/mmc/host/sdhci-pxav3.c
@@ -135,6 +135,7 @@ static int armada_38x_quirks(struct platform_device *pdev,
 	struct sdhci_pxa *pxa = pltfm_host->priv;
 	struct resource *res;
 
+	host->quirks &= ~SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN;
 	host->quirks |= SDHCI_QUIRK_MISSING_CAPS;
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
 					   "conf-sdio3");
-- 
1.8.3.1

[PATCH 1/8] mmc: sdhci-pxav3: remove broken clock base quirk for Armada 38x sdhci driver

[Gregory CLEMENT]

Hi Marcin,
 
 On mar., oct. 06 2015, Marcin Wojtas <mw@semihalf.com> wrote:

> From: Nadav Haklai <nadavh@marvell.com>
>
> shci-pxav3 driver is enabling by default the
> SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN quirk. However this quirk is not
> required for Armada 38x and leads to wrong clock setting in the divider.
>
> Signed-off-by: Nadav Haklai <nadavh@marvell.com>
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> Cc: <stable@vger.kernel.org> # v4.2

Seems OK.

Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Thanks,

Gregory

> ---
>  drivers/mmc/host/sdhci-pxav3.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> index 946d37f..976cddd 100644
> --- a/drivers/mmc/host/sdhci-pxav3.c
> +++ b/drivers/mmc/host/sdhci-pxav3.c
> @@ -135,6 +135,7 @@ static int armada_38x_quirks(struct platform_device *pdev,
>  	struct sdhci_pxa *pxa = pltfm_host->priv;
>  	struct resource *res;
>  
> +	host->quirks &= ~SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN;
>  	host->quirks |= SDHCI_QUIRK_MISSING_CAPS;
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>  					   "conf-sdio3");
> -- 
> 1.8.3.1
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 1/8] mmc: sdhci-pxav3: remove broken clock base quirk for Armada 38x sdhci driver

[Ulf Hansson]

On 6 October 2015 at 03:22, Marcin Wojtas <mw@semihalf.com> wrote:
> From: Nadav Haklai <nadavh@marvell.com>
>
> shci-pxav3 driver is enabling by default the
> SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN quirk. However this quirk is not
> required for Armada 38x and leads to wrong clock setting in the divider.
>
> Signed-off-by: Nadav Haklai <nadavh@marvell.com>
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> Cc: <stable@vger.kernel.org> # v4.2

Thanks, applied for fixes!

Kind regards
Uffe

> ---
>  drivers/mmc/host/sdhci-pxav3.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> index 946d37f..976cddd 100644
> --- a/drivers/mmc/host/sdhci-pxav3.c
> +++ b/drivers/mmc/host/sdhci-pxav3.c
> @@ -135,6 +135,7 @@ static int armada_38x_quirks(struct platform_device *pdev,
>         struct sdhci_pxa *pxa = pltfm_host->priv;
>         struct resource *res;
>
> +       host->quirks &= ~SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN;
>         host->quirks |= SDHCI_QUIRK_MISSING_CAPS;
>         res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>                                            "conf-sdio3");
> --
> 1.8.3.1
>

[PATCH 2/8] mmc: sdhci-pxav3: disable clock inversion for HS MMC cards

[Marcin Wojtas]

From: Nadav Haklai <nadavh@marvell.com>

According to 'FE-2946959' erratum the clock inversion option is
needed to support slow frequencies when the card input hold time
requirement is high. This setting is not required for high speed
MMC and might cause timing violation.

Signed-off-by: Nadav Haklai <nadavh@marvell.com>
Cc: <stable@vger.kernel.org> # v4.2
---
 drivers/mmc/host/sdhci-pxav3.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
index 976cddd..89a9e49 100644
--- a/drivers/mmc/host/sdhci-pxav3.c
+++ b/drivers/mmc/host/sdhci-pxav3.c
@@ -291,6 +291,9 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
 		    uhs == MMC_TIMING_UHS_DDR50) {
 			reg_val &= ~SDIO3_CONF_CLK_INV;
 			reg_val |= SDIO3_CONF_SD_FB_CLK;
+		} else if (uhs == MMC_TIMING_MMC_HS) {
+			reg_val &= ~SDIO3_CONF_CLK_INV;
+			reg_val &= ~SDIO3_CONF_SD_FB_CLK;
 		} else {
 			reg_val |= SDIO3_CONF_CLK_INV;
 			reg_val &= ~SDIO3_CONF_SD_FB_CLK;
-- 
1.8.3.1

[PATCH 2/8] mmc: sdhci-pxav3: disable clock inversion for HS MMC cards

[Gregory CLEMENT]

Hi Marcin,
 
 On mar., oct. 06 2015, Marcin Wojtas <mw@semihalf.com> wrote:

> From: Nadav Haklai <nadavh@marvell.com>
>
> According to 'FE-2946959' erratum the clock inversion option is
> needed to support slow frequencies when the card input hold time
> requirement is high. This setting is not required for high speed
> MMC and might cause timing violation.
>
> Signed-off-by: Nadav Haklai <nadavh@marvell.com>
> Cc: <stable@vger.kernel.org> # v4.2

Seems OK too.

Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Thanks,

Gregory
> ---
>  drivers/mmc/host/sdhci-pxav3.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> index 976cddd..89a9e49 100644
> --- a/drivers/mmc/host/sdhci-pxav3.c
> +++ b/drivers/mmc/host/sdhci-pxav3.c
> @@ -291,6 +291,9 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>  		    uhs == MMC_TIMING_UHS_DDR50) {
>  			reg_val &= ~SDIO3_CONF_CLK_INV;
>  			reg_val |= SDIO3_CONF_SD_FB_CLK;
> +		} else if (uhs == MMC_TIMING_MMC_HS) {
> +			reg_val &= ~SDIO3_CONF_CLK_INV;
> +			reg_val &= ~SDIO3_CONF_SD_FB_CLK;
>  		} else {
>  			reg_val |= SDIO3_CONF_CLK_INV;
>  			reg_val &= ~SDIO3_CONF_SD_FB_CLK;
> -- 
> 1.8.3.1
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 2/8] mmc: sdhci-pxav3: disable clock inversion for HS MMC cards

[Ulf Hansson]

On 6 October 2015 at 03:22, Marcin Wojtas <mw@semihalf.com> wrote:
> From: Nadav Haklai <nadavh@marvell.com>
>
> According to 'FE-2946959' erratum the clock inversion option is
> needed to support slow frequencies when the card input hold time
> requirement is high. This setting is not required for high speed
> MMC and might cause timing violation.
>
> Signed-off-by: Nadav Haklai <nadavh@marvell.com>
> Cc: <stable@vger.kernel.org> # v4.2

Thanks, applied for fixes!

Kind regards
Uffe

> ---
>  drivers/mmc/host/sdhci-pxav3.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> index 976cddd..89a9e49 100644
> --- a/drivers/mmc/host/sdhci-pxav3.c
> +++ b/drivers/mmc/host/sdhci-pxav3.c
> @@ -291,6 +291,9 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>                     uhs == MMC_TIMING_UHS_DDR50) {
>                         reg_val &= ~SDIO3_CONF_CLK_INV;
>                         reg_val |= SDIO3_CONF_SD_FB_CLK;
> +               } else if (uhs == MMC_TIMING_MMC_HS) {
> +                       reg_val &= ~SDIO3_CONF_CLK_INV;
> +                       reg_val &= ~SDIO3_CONF_SD_FB_CLK;
>                 } else {
>                         reg_val |= SDIO3_CONF_CLK_INV;
>                         reg_val &= ~SDIO3_CONF_SD_FB_CLK;
> --
> 1.8.3.1
>

[PATCH 3/8] mmc: sdhci-pxav3: fix error handling of armada_38x_quirks

[Marcin Wojtas]

In case of armada_38x_quirks error, all clocks should be cleaned-up, same
as after mv_conf_mbus_windows failure.

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
Cc: <stable@vger.kernel.org> # v4.2
---
 drivers/mmc/host/sdhci-pxav3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
index 89a9e49..f5edf9d 100644
--- a/drivers/mmc/host/sdhci-pxav3.c
+++ b/drivers/mmc/host/sdhci-pxav3.c
@@ -402,7 +402,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
 	if (of_device_is_compatible(np, "marvell,armada-380-sdhci")) {
 		ret = armada_38x_quirks(pdev, host);
 		if (ret < 0)
-			goto err_clk_get;
+			goto err_mbus_win;
 		ret = mv_conf_mbus_windows(pdev, mv_mbus_dram_info());
 		if (ret < 0)
 			goto err_mbus_win;
-- 
1.8.3.1

[PATCH 3/8] mmc: sdhci-pxav3: fix error handling of armada_38x_quirks

[Gregory CLEMENT]

Hi Marcin,
 
 On mar., oct. 06 2015, Marcin Wojtas <mw@semihalf.com> wrote:

> In case of armada_38x_quirks error, all clocks should be cleaned-up, same
> as after mv_conf_mbus_windows failure.
>
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> Cc: <stable@vger.kernel.org> # v4.2

Good catch.

Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Thanks,

Gregory

> ---
>  drivers/mmc/host/sdhci-pxav3.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> index 89a9e49..f5edf9d 100644
> --- a/drivers/mmc/host/sdhci-pxav3.c
> +++ b/drivers/mmc/host/sdhci-pxav3.c
> @@ -402,7 +402,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
>  	if (of_device_is_compatible(np, "marvell,armada-380-sdhci")) {
>  		ret = armada_38x_quirks(pdev, host);
>  		if (ret < 0)
> -			goto err_clk_get;
> +			goto err_mbus_win;
>  		ret = mv_conf_mbus_windows(pdev, mv_mbus_dram_info());
>  		if (ret < 0)
>  			goto err_mbus_win;
> -- 
> 1.8.3.1
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 3/8] mmc: sdhci-pxav3: fix error handling of armada_38x_quirks

[Ulf Hansson]

On 6 October 2015 at 03:22, Marcin Wojtas <mw@semihalf.com> wrote:
> In case of armada_38x_quirks error, all clocks should be cleaned-up, same
> as after mv_conf_mbus_windows failure.
>
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> Cc: <stable@vger.kernel.org> # v4.2

Thanks, applied for fixes!

Kind regards
Uffe

> ---
>  drivers/mmc/host/sdhci-pxav3.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> index 89a9e49..f5edf9d 100644
> --- a/drivers/mmc/host/sdhci-pxav3.c
> +++ b/drivers/mmc/host/sdhci-pxav3.c
> @@ -402,7 +402,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
>         if (of_device_is_compatible(np, "marvell,armada-380-sdhci")) {
>                 ret = armada_38x_quirks(pdev, host);
>                 if (ret < 0)
> -                       goto err_clk_get;
> +                       goto err_mbus_win;
>                 ret = mv_conf_mbus_windows(pdev, mv_mbus_dram_info());
>                 if (ret < 0)
>                         goto err_mbus_win;
> --
> 1.8.3.1
>

[PATCH 3/8] mmc: sdhci-pxav3: fix error handling of armada_38x_quirks

[Marcin Wojtas]

Thanks for grabbing the three patches!

Best regards,
Marcin

2015-10-08 19:35 GMT+02:00 Ulf Hansson <ulf.hansson@linaro.org>:
> On 6 October 2015 at 03:22, Marcin Wojtas <mw@semihalf.com> wrote:
>> In case of armada_38x_quirks error, all clocks should be cleaned-up, same
>> as after mv_conf_mbus_windows failure.
>>
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> Cc: <stable@vger.kernel.org> # v4.2
>
> Thanks, applied for fixes!
>
> Kind regards
> Uffe
>
>> ---
>>  drivers/mmc/host/sdhci-pxav3.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
>> index 89a9e49..f5edf9d 100644
>> --- a/drivers/mmc/host/sdhci-pxav3.c
>> +++ b/drivers/mmc/host/sdhci-pxav3.c
>> @@ -402,7 +402,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
>>         if (of_device_is_compatible(np, "marvell,armada-380-sdhci")) {
>>                 ret = armada_38x_quirks(pdev, host);
>>                 if (ret < 0)
>> -                       goto err_clk_get;
>> +                       goto err_mbus_win;
>>                 ret = mv_conf_mbus_windows(pdev, mv_mbus_dram_info());
>>                 if (ret < 0)
>>                         goto err_mbus_win;
>> --
>> 1.8.3.1
>>

[PATCH 4/8] mmc: sdhci-pxav3: enable proper resuming on Armada 38x SoC

[Marcin Wojtas]

When resuming from suspend on Armada 38x SoC MBus windows have to be
re-configured and for that purpose mv_conf_mbus_windows function needed
rework. MBus windows register base address obtaining was moved to
armada_38x_quirks function in order to be kept in pxa global structure,
because it is used during a resume.

This commit fixes resuming from suspend by calling MBus windows
configuration routine and therefore enabling proper DMA operation.

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 drivers/mmc/host/sdhci-pxav3.c | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
index f5edf9d..3f71894 100644
--- a/drivers/mmc/host/sdhci-pxav3.c
+++ b/drivers/mmc/host/sdhci-pxav3.c
@@ -63,6 +63,7 @@ struct sdhci_pxa {
 	struct clk *clk_io;
 	u8	power_mode;
 	void __iomem *sdio3_conf_reg;
+	void __iomem *mbus_win_regs;
 };
 
 /*
@@ -81,30 +82,16 @@ struct sdhci_pxa {
 #define SDIO3_CONF_CLK_INV	BIT(0)
 #define SDIO3_CONF_SD_FB_CLK	BIT(2)
 
-static int mv_conf_mbus_windows(struct platform_device *pdev,
+static int mv_conf_mbus_windows(struct device *dev, void __iomem *regs,
 				const struct mbus_dram_target_info *dram)
 {
 	int i;
-	void __iomem *regs;
-	struct resource *res;
 
 	if (!dram) {
-		dev_err(&pdev->dev, "no mbus dram info\n");
-		return -EINVAL;
-	}
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-	if (!res) {
-		dev_err(&pdev->dev, "cannot get mbus registers\n");
+		dev_err(dev, "no mbus dram info\n");
 		return -EINVAL;
 	}
 
-	regs = ioremap(res->start, resource_size(res));
-	if (!regs) {
-		dev_err(&pdev->dev, "cannot map mbus registers\n");
-		return -ENOMEM;
-	}
-
 	for (i = 0; i < SDHCI_MAX_WIN_NUM; i++) {
 		writel(0, regs + SDHCI_WINDOW_CTRL(i));
 		writel(0, regs + SDHCI_WINDOW_BASE(i));
@@ -122,8 +109,6 @@ static int mv_conf_mbus_windows(struct platform_device *pdev,
 		writel(cs->base, regs + SDHCI_WINDOW_BASE(i));
 	}
 
-	iounmap(regs);
-
 	return 0;
 }
 
@@ -135,6 +120,14 @@ static int armada_38x_quirks(struct platform_device *pdev,
 	struct sdhci_pxa *pxa = pltfm_host->priv;
 	struct resource *res;
 
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mbus");
+	pxa->mbus_win_regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(pxa->mbus_win_regs)) {
+		dev_err(mmc_dev(host->mmc),
+			"failed to obtain MBus windows register base\n");
+		return PTR_ERR(pxa->mbus_win_regs);
+	}
+
 	host->quirks &= ~SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN;
 	host->quirks |= SDHCI_QUIRK_MISSING_CAPS;
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
@@ -403,7 +396,8 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
 		ret = armada_38x_quirks(pdev, host);
 		if (ret < 0)
 			goto err_mbus_win;
-		ret = mv_conf_mbus_windows(pdev, mv_mbus_dram_info());
+		ret = mv_conf_mbus_windows(&pdev->dev, pxa->mbus_win_regs,
+					   mv_mbus_dram_info());
 		if (ret < 0)
 			goto err_mbus_win;
 	}
@@ -520,6 +514,13 @@ static int sdhci_pxav3_resume(struct device *dev)
 {
 	int ret;
 	struct sdhci_host *host = dev_get_drvdata(dev);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_pxa *pxa = pltfm_host->priv;
+	struct device_node *np = dev->of_node;
+
+	if (of_device_is_compatible(np, "marvell,armada-380-sdhci"))
+		ret = mv_conf_mbus_windows(dev, pxa->mbus_win_regs,
+					   mv_mbus_dram_info());
 
 	pm_runtime_get_sync(dev);
 	ret = sdhci_resume_host(host);
-- 
1.8.3.1

[PATCH 4/8] mmc: sdhci-pxav3: enable proper resuming on Armada 38x SoC

[Gregory CLEMENT]

Hi Marcin,
 
 On mar., oct. 06 2015, Marcin Wojtas <mw@semihalf.com> wrote:

> When resuming from suspend on Armada 38x SoC MBus windows have to be
> re-configured and for that purpose mv_conf_mbus_windows function needed
> rework. MBus windows register base address obtaining was moved to
> armada_38x_quirks function in order to be kept in pxa global structure,
> because it is used during a resume.
>
> This commit fixes resuming from suspend by calling MBus windows
> configuration routine and therefore enabling proper DMA operation.
>
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  drivers/mmc/host/sdhci-pxav3.c | 39 ++++++++++++++++++++-------------------
>  1 file changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> index f5edf9d..3f71894 100644
> --- a/drivers/mmc/host/sdhci-pxav3.c
> +++ b/drivers/mmc/host/sdhci-pxav3.c
> @@ -63,6 +63,7 @@ struct sdhci_pxa {
>  	struct clk *clk_io;
>  	u8	power_mode;
>  	void __iomem *sdio3_conf_reg;
> +	void __iomem *mbus_win_regs;
>  };


>  
> @@ -135,6 +120,14 @@ static int armada_38x_quirks(struct platform_device *pdev,
>  	struct sdhci_pxa *pxa = pltfm_host->priv;
>  	struct resource *res;
>  
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mbus");
> +	pxa->mbus_win_regs = devm_ioremap_resource(&pdev->dev, res);

[...]

> @@ -520,6 +514,13 @@ static int sdhci_pxav3_resume(struct device *dev)
>  {
>  	int ret;
>  	struct sdhci_host *host = dev_get_drvdata(dev);
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_pxa *pxa = pltfm_host->priv;
> +	struct device_node *np = dev->of_node;
> +
> +	if (of_device_is_compatible(np, "marvell,armada-380-sdhci"))
> +		ret = mv_conf_mbus_windows(dev, pxa->mbus_win_regs,
> +					   mv_mbus_dram_info());

I would find it cleaner to not rely on the device tree outise the probe
function. What about just testing pxa->mbus_win_regs ? As it is set only
if we need it, it should be a good test.

Thanks,

Gregory


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 4/8] mmc: sdhci-pxav3: enable proper resuming on Armada 38x SoC

[Marcin Wojtas]

Gregory,

2015-10-06 16:51 GMT+02:00 Gregory CLEMENT <gregory.clement
>> +     if (of_device_is_compatible(np, "marvell,armada-380-sdhci"))
>> +             ret = mv_conf_mbus_windows(dev, pxa->mbus_win_regs,
>> +                                        mv_mbus_dram_info());
>
> I would find it cleaner to not rely on the device tree outise the probe
> function. What about just testing pxa->mbus_win_regs ? As it is set only
> if we need it, it should be a good test.
>

Sure, it will ease calling the function.

Thanks,
Marcin

[PATCH 4/8] mmc: sdhci-pxav3: enable proper resuming on Armada 38x SoC

[Jisheng Zhang]

Hi Marcin,

On Tue, 6 Oct 2015 03:22:38 +0200
Marcin Wojtas <mw@semihalf.com> wrote:

> When resuming from suspend on Armada 38x SoC MBus windows have to be
> re-configured and for that purpose mv_conf_mbus_windows function needed
> rework. MBus windows register base address obtaining was moved to
> armada_38x_quirks function in order to be kept in pxa global structure,
> because it is used during a resume.
> 
> This commit fixes resuming from suspend by calling MBus windows
> configuration routine and therefore enabling proper DMA operation.
> 
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  drivers/mmc/host/sdhci-pxav3.c | 39 ++++++++++++++++++++-------------------
>  1 file changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> index f5edf9d..3f71894 100644
> --- a/drivers/mmc/host/sdhci-pxav3.c
> +++ b/drivers/mmc/host/sdhci-pxav3.c
> @@ -63,6 +63,7 @@ struct sdhci_pxa {
>  	struct clk *clk_io;
>  	u8	power_mode;
>  	void __iomem *sdio3_conf_reg;
> +	void __iomem *mbus_win_regs;
>  };
>  
>  /*
> @@ -81,30 +82,16 @@ struct sdhci_pxa {
>  #define SDIO3_CONF_CLK_INV	BIT(0)
>  #define SDIO3_CONF_SD_FB_CLK	BIT(2)
>  
> -static int mv_conf_mbus_windows(struct platform_device *pdev,
> +static int mv_conf_mbus_windows(struct device *dev, void __iomem *regs,
>  				const struct mbus_dram_target_info *dram)
>  {
>  	int i;
> -	void __iomem *regs;
> -	struct resource *res;
>  
>  	if (!dram) {
> -		dev_err(&pdev->dev, "no mbus dram info\n");
> -		return -EINVAL;
> -	}
> -
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> -	if (!res) {
> -		dev_err(&pdev->dev, "cannot get mbus registers\n");
> +		dev_err(dev, "no mbus dram info\n");
>  		return -EINVAL;
>  	}
>  
> -	regs = ioremap(res->start, resource_size(res));
> -	if (!regs) {
> -		dev_err(&pdev->dev, "cannot map mbus registers\n");
> -		return -ENOMEM;
> -	}
> -
>  	for (i = 0; i < SDHCI_MAX_WIN_NUM; i++) {
>  		writel(0, regs + SDHCI_WINDOW_CTRL(i));
>  		writel(0, regs + SDHCI_WINDOW_BASE(i));
> @@ -122,8 +109,6 @@ static int mv_conf_mbus_windows(struct platform_device *pdev,
>  		writel(cs->base, regs + SDHCI_WINDOW_BASE(i));
>  	}
>  
> -	iounmap(regs);
> -
>  	return 0;
>  }
>  
> @@ -135,6 +120,14 @@ static int armada_38x_quirks(struct platform_device *pdev,
>  	struct sdhci_pxa *pxa = pltfm_host->priv;
>  	struct resource *res;
>  
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mbus");
> +	pxa->mbus_win_regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(pxa->mbus_win_regs)) {
> +		dev_err(mmc_dev(host->mmc),
> +			"failed to obtain MBus windows register base\n");

devm_ioremap_resource() has warned us if it fails, so is it better to remove
this dev_err() here?

> +		return PTR_ERR(pxa->mbus_win_regs);
> +	}
> +
>  	host->quirks &= ~SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN;
>  	host->quirks |= SDHCI_QUIRK_MISSING_CAPS;
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> @@ -403,7 +396,8 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
>  		ret = armada_38x_quirks(pdev, host);
>  		if (ret < 0)
>  			goto err_mbus_win;
> -		ret = mv_conf_mbus_windows(pdev, mv_mbus_dram_info());
> +		ret = mv_conf_mbus_windows(&pdev->dev, pxa->mbus_win_regs,
> +					   mv_mbus_dram_info());
>  		if (ret < 0)
>  			goto err_mbus_win;
>  	}
> @@ -520,6 +514,13 @@ static int sdhci_pxav3_resume(struct device *dev)
>  {
>  	int ret;
>  	struct sdhci_host *host = dev_get_drvdata(dev);
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_pxa *pxa = pltfm_host->priv;
> +	struct device_node *np = dev->of_node;
> +
> +	if (of_device_is_compatible(np, "marvell,armada-380-sdhci"))

this would increase resume time especially those non armada-380-sdhci host
although it's trivial. Is it better to check "if (pxa->mbus_win_regs)"?

> +		ret = mv_conf_mbus_windows(dev, pxa->mbus_win_regs,
> +					   mv_mbus_dram_info());
>  
>  	pm_runtime_get_sync(dev);
>  	ret = sdhci_resume_host(host);

[PATCH 4/8] mmc: sdhci-pxav3: enable proper resuming on Armada 38x SoC

[Marcin Wojtas]

Hi Jisheng,


>> +     res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mbus");
>> +     pxa->mbus_win_regs = devm_ioremap_resource(&pdev->dev, res);
>> +     if (IS_ERR(pxa->mbus_win_regs)) {
>> +             dev_err(mmc_dev(host->mmc),
>> +                     "failed to obtain MBus windows register base\n");
>
> devm_ioremap_resource() has warned us if it fails, so is it better to remove
> this dev_err() here?
>

Indeed, I'll remove this excessive verbosity.

>>       struct sdhci_host *host = dev_get_drvdata(dev);
>> +     struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +     struct sdhci_pxa *pxa = pltfm_host->priv;
>> +     struct device_node *np = dev->of_node;
>> +
>> +     if (of_device_is_compatible(np, "marvell,armada-380-sdhci"))
>
> this would increase resume time especially those non armada-380-sdhci host
> although it's trivial. Is it better to check "if (pxa->mbus_win_regs)"?
>

Already implemented in v2.

Thanks for the comments,
Marcin

[PATCH 5/8] mmc: sdhci-pxav3: enable usage of DAT3 pin as HW card detect

[Marcin Wojtas]

Marvell Armada 38x SDHCI controller enable using DAT3 pin as a hardware
card detection. According to the SD sdandard this signal can be used for
this purpose combined with a pull-up resistor, implying inverted (active
low) polarization of a card detect. MMC standard does not support this
feature and does not operate with such connectivity of DAT3.

When using DAT3-based detection Armada 38x SDIO IP expects its internal
clock to be always on, which had to be ensured twofold:
- Each time controller is reset by updating appropriate registers. On the
  occasion of adding new register @0x104, register @0x100 name is modified
  in order to the be aligned with Armada 38x documentation.
- Leaving the clock enabled despite power-down. For this purpose a new
  quirk had to be added to SDHCI subsystem - SDHCI_QUIRK2_KEEP_INT_CLK_ON.

In addition to the changes above this commit adds a new property to Armada
38x SDHCI node ('dat3-cd') with an according binding documentation update.

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 .../devicetree/bindings/mmc/sdhci-pxa.txt          |  5 +++
 drivers/mmc/host/sdhci-pxav3.c                     | 38 ++++++++++++++++++++--
 drivers/mmc/host/sdhci.c                           |  5 ++-
 drivers/mmc/host/sdhci.h                           |  3 ++
 4 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt b/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt
index 3d1b449..ffd6b14 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt
@@ -23,6 +23,11 @@ Required properties:
 
 Optional properties:
 - mrvl,clk-delay-cycles: Specify a number of cycles to delay for tuning.
+- dat3-cd: use DAT3 pin as hardware card detect - option available for
+  "marvell,armada-380-sdhci" only. The detection is supposed to work with
+  active high polarity, which implies usage of "cd-inverted" property.
+  Note that according to the specifications DAT3-based card detection can be
+  used with SD cards only. MMC standard doesn't support this feature.
 
 Example:
 
diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
index 3f71894..ce96640 100644
--- a/drivers/mmc/host/sdhci-pxav3.c
+++ b/drivers/mmc/host/sdhci-pxav3.c
@@ -46,10 +46,14 @@
 #define SDCLK_DELAY_SHIFT	9
 #define SDCLK_DELAY_MASK	0x1f
 
-#define SD_CFG_FIFO_PARAM       0x100
+#define SD_EXTRA_PARAM_REG	0x100
 #define SDCFG_GEN_PAD_CLK_ON	(1<<6)
 #define SDCFG_GEN_PAD_CLK_CNT_MASK	0xFF
 #define SDCFG_GEN_PAD_CLK_CNT_SHIFT	24
+#define SD_FIFO_PARAM_REG	0x104
+#define SD_USE_DAT3		BIT(7)
+#define SD_OVRRD_CLK_OEN	BIT(11)
+#define SD_FORCE_CLK_ON		BIT(12)
 
 #define SD_SPI_MODE          0x108
 #define SD_CE_ATA_1          0x10C
@@ -163,6 +167,15 @@ static int armada_38x_quirks(struct platform_device *pdev,
 	}
 	host->caps1 &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_USE_SDR50_TUNING);
 
+	/*
+	 * The interface clock enable is also used as control
+	 * for the A38x SDIO IP, so it can't be powered down
+	 * when using HW-based card detection.
+	 */
+	if (of_property_read_bool(np, "dat3-cd") &&
+	    !of_property_read_bool(np, "broken-cd"))
+		host->quirks2 |= SDHCI_QUIRK2_KEEP_INT_CLK_ON;
+
 	return 0;
 }
 
@@ -170,6 +183,8 @@ static void pxav3_reset(struct sdhci_host *host, u8 mask)
 {
 	struct platform_device *pdev = to_platform_device(mmc_dev(host->mmc));
 	struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data;
+	struct device_node *np = pdev->dev.of_node;
+	u32 reg_val;
 
 	sdhci_reset(host, mask);
 
@@ -187,6 +202,22 @@ static void pxav3_reset(struct sdhci_host *host, u8 mask)
 			tmp |= SDCLK_SEL;
 			writew(tmp, host->ioaddr + SD_CLOCK_BURST_SIZE_SETUP);
 		}
+
+		if (of_device_is_compatible(np, "marvell,armada-380-sdhci") &&
+		    host->quirks2 & SDHCI_QUIRK2_KEEP_INT_CLK_ON) {
+			reg_val = sdhci_readl(host, SD_FIFO_PARAM_REG);
+			reg_val |= SD_USE_DAT3 | SD_OVRRD_CLK_OEN |
+				   SD_FORCE_CLK_ON;
+			sdhci_writel(host, reg_val, SD_FIFO_PARAM_REG);
+
+			/*
+			 * For HW detection based on DAT3 pin keep internal
+			 * clk switched on after controller reset.
+			 */
+			reg_val = sdhci_readl(host, SDHCI_CLOCK_CONTROL);
+			reg_val |= SDHCI_CLOCK_INT_EN;
+			sdhci_writel(host, reg_val, SDHCI_CLOCK_CONTROL);
+		}
 	}
 }
 
@@ -214,9 +245,9 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode)
 		writew(tmp, host->ioaddr + SD_CE_ATA_2);
 
 		/* start sending the 74 clocks */
-		tmp = readw(host->ioaddr + SD_CFG_FIFO_PARAM);
+		tmp = readw(host->ioaddr + SD_EXTRA_PARAM_REG);
 		tmp |= SDCFG_GEN_PAD_CLK_ON;
-		writew(tmp, host->ioaddr + SD_CFG_FIFO_PARAM);
+		writew(tmp, host->ioaddr + SD_EXTRA_PARAM_REG);
 
 		/* slowest speed is about 100KHz or 10usec per clock */
 		udelay(740);
@@ -410,6 +441,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
 		sdhci_get_of_property(pdev);
 		pdata = pxav3_get_mmc_pdata(dev);
 		pdev->dev.platform_data = pdata;
+
 	} else if (pdata) {
 		/* on-chip device */
 		if (pdata->flags & PXA_FLAG_CARD_PERMANENT)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 64b7fdb..cfed695 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1159,7 +1159,10 @@ void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 
 	host->mmc->actual_clock = 0;
 
-	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
+	/* Some controllers need to keep internal clk always enabled */
+	if (host->quirks2 & SDHCI_QUIRK2_KEEP_INT_CLK_ON)
+		clk = SDHCI_CLOCK_INT_EN;
+	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
 
 	if (clock == 0)
 		return;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 7c02ff4..c751b78 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -412,6 +412,9 @@ 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)
+/* Do not disable internal clk on power-off */
+#define SDHCI_QUIRK2_KEEP_INT_CLK_ON			(1<<16)
+
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
1.8.3.1

[PATCH 6/8] ARM: mvebu: enable SDHCI card detection using DAT3 pin on A388-GP

[Marcin Wojtas]

The newest revisions of A388-GP (v1.5 and higher) support only
DAT3-based card detection, which is enabled by this commit. Hitherto
revisions, without such modification, will be impacted with a broken
card detection - in order to operate the cards have to be present
during kernel boot.

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 arch/arm/boot/dts/armada-388-gp.dts | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/armada-388-gp.dts b/arch/arm/boot/dts/armada-388-gp.dts
index 391dea9..4855963 100644
--- a/arch/arm/boot/dts/armada-388-gp.dts
+++ b/arch/arm/boot/dts/armada-388-gp.dts
@@ -213,8 +213,9 @@
 			sdhci at d8000 {
 				pinctrl-names = "default";
 				pinctrl-0 = <&sdhci_pins>;
-				cd-gpios = <&expander0 5 GPIO_ACTIVE_LOW>;
 				no-1-8-v;
+				dat3-cd;
+				cd-inverted;
 				wp-inverted;
 				bus-width = <8>;
 				status = "okay";
-- 
1.8.3.1

[PATCH 6/8] ARM: mvebu: enable SDHCI card detection using DAT3 pin on A388-GP

[Andrew Lunn]

On Tue, Oct 06, 2015 at 03:22:40AM +0200, Marcin Wojtas wrote:
> The newest revisions of A388-GP (v1.5 and higher) support only
> DAT3-based card detection, which is enabled by this commit. Hitherto
> revisions, without such modification, will be impacted with a broken
> card detection - in order to operate the cards have to be present
> during kernel boot.

Humm. Is this acceptable, breaking old boards?

I would say at minimum, there should be a big fat comment at the top
of armada-388-gp.dts explaining that this DTS file is broken on
v0.0-v1.4.

Or we have two .dts files for the 388-gp file, and a dtsi file.

   Andrew

[PATCH 6/8] ARM: mvebu: enable SDHCI card detection using DAT3 pin on A388-GP

[Marcin Wojtas]

Hi Andrew,

2015-10-06 5:31 GMT+02:00 Andrew Lunn <andrew@lunn.ch>:
> On Tue, Oct 06, 2015 at 03:22:40AM +0200, Marcin Wojtas wrote:
>> The newest revisions of A388-GP (v1.5 and higher) support only
>> DAT3-based card detection, which is enabled by this commit. Hitherto
>> revisions, without such modification, will be impacted with a broken
>> card detection - in order to operate the cards have to be present
>> during kernel boot.
>
> Humm. Is this acceptable, breaking old boards?
>
> I would say at minimum, there should be a big fat comment at the top
> of armada-388-gp.dts explaining that this DTS file is broken on
> v0.0-v1.4.
>
> Or we have two .dts files for the 388-gp file, and a dtsi file.
>

I expected this patch would be controversial, hence I propose a
compromise: set A388-GP SDHCI to 'broken-cd' by defeault. However
Marvell insisted on HW card detection, because software polling spoils
the SD/MMC benchmarks, but this way the user would decide whether to
stay with broken-cd or switch to GPIO/DAT3 detection. What do you
think?

Best regards,
Marcin

[PATCH 6/8] ARM: mvebu: enable SDHCI card detection using DAT3 pin on A388-GP

[Andrew Lunn]

On Tue, Oct 06, 2015 at 09:02:25AM +0200, Marcin Wojtas wrote:
> Hi Andrew,
> 
> 2015-10-06 5:31 GMT+02:00 Andrew Lunn <andrew@lunn.ch>:
> > On Tue, Oct 06, 2015 at 03:22:40AM +0200, Marcin Wojtas wrote:
> >> The newest revisions of A388-GP (v1.5 and higher) support only
> >> DAT3-based card detection, which is enabled by this commit. Hitherto
> >> revisions, without such modification, will be impacted with a broken
> >> card detection - in order to operate the cards have to be present
> >> during kernel boot.
> >
> > Humm. Is this acceptable, breaking old boards?
> >
> > I would say at minimum, there should be a big fat comment at the top
> > of armada-388-gp.dts explaining that this DTS file is broken on
> > v0.0-v1.4.
> >
> > Or we have two .dts files for the 388-gp file, and a dtsi file.
> >
> 
> I expected this patch would be controversial, hence I propose a
> compromise: set A388-GP SDHCI to 'broken-cd' by defeault.

So for boards < 1.5, you are introducing a regression. It used to work
via GPIO. Now that is ignored and it is declared broken. Or am i
mis-understanding what broken-cd means, and it actually means, poll
it?

> However Marvell insisted on HW card detection

Marvell does not get to decide what goes into the kernel and what can
be deliberately broken. Marvell can make recommendations, provide
benchmarks reports, etc. But the community, and ultimately the
responsible maintainer decides.

> because software polling spoils the SD/MMC benchmarks, but this way
> the user would decide whether to stay with broken-cd or switch to
> GPIO/DAT3 detection. What do you think?

So rather than presenting a solution, please could you list all the
different options you can think of, and how they would work for < 1.5
and >= 1.5.

One other useful bit of information. How many < 1.5 boards are there,
and who has them. Are they internal to Marvell and Free Electrons, or
do a lot of people have them? We have thrown away fixes which are
requires for A0 stepping SoCs, because very few people have such
devices and they can easily replace them for B1. If nearly nobody
actually has < 1.5, ....

Thanks
	 Andrew

[PATCH 6/8] ARM: mvebu: enable SDHCI card detection using DAT3 pin on A388-GP

[Marcin Wojtas]

Andrew,

2015-10-06 16:45 GMT+02:00 Andrew Lunn <andrew@lunn.ch>:
>> I expected this patch would be controversial, hence I propose a
>> compromise: set A388-GP SDHCI to 'broken-cd' by defeault.
>
> So for boards < 1.5, you are introducing a regression. It used to work
> via GPIO. Now that is ignored and it is declared broken. Or am i
> mis-understanding what broken-cd means, and it actually means, poll
> it?
>

"broken-cd" property means that the mmc subsystem is polling instead
of using hardware detection.

>> However Marvell insisted on HW card detection
>
> Marvell does not get to decide what goes into the kernel and what can
> be deliberately broken. Marvell can make recommendations, provide
> benchmarks reports, etc. But the community, and ultimately the
> responsible maintainer decides.

Sure, this is why I submitted the patch to the list in order to get
review. Please bear in mind, that I do not work in Marvell and I just
wanted to present some justification of the decision, about which I
got informed. Personally I think it would be reasonable to keep GPIO
detect as is, but maybe I'm lacking some background.

>
>> because software polling spoils the SD/MMC benchmarks, but this way
>> the user would decide whether to stay with broken-cd or switch to
>> GPIO/DAT3 detection. What do you think?
>
> So rather than presenting a solution, please could you list all the
> different options you can think of, and how they would work for < 1.5
> and >= 1.5.
>

We have three options here:
- leave GPIO-based card detection and see if in future anyone
complains about not working detection
- switch to DAT3 detection - yes you're right, that it is a regression
for all boards < 1.5
- enable SW polling for detection ("broken-cd") which should satisfy
all kind of boards

> One other useful bit of information. How many < 1.5 boards are there,
> and who has them. Are they internal to Marvell and Free Electrons, or
> do a lot of people have them? We have thrown away fixes which are
> requires for A0 stepping SoCs, because very few people have such
> devices and they can easily replace them for B1. If nearly nobody
> actually has < 1.5, ....

I have no information, nor an access to such data, I can have only
suspicions which cannot be a reference in any way.

Best regards,
Marcin

[PATCH 6/8] ARM: mvebu: enable SDHCI card detection using DAT3 pin on A388-GP

[Andrew Lunn]

> We have three options here:
> - leave GPIO-based card detection and see if in future anyone
> complains about not working detection
> - switch to DAT3 detection - yes you're right, that it is a regression
> for all boards < 1.5
> - enable SW polling for detection ("broken-cd") which should satisfy
> all kind of boards

Then i would suggest either broken-cd, or two .dtb blobs and good
comments in the dts files about how to identify the boards and what
happens if you run the wrong DTB blob.

     Andrew

[PATCH 6/8] ARM: mvebu: enable SDHCI card detection using DAT3 pin on A388-GP

[Gregory CLEMENT]

Hi,
 
 On mar., oct. 06 2015, Marcin Wojtas <mw@semihalf.com> wrote:

> Hi Andrew,
>
> 2015-10-06 5:31 GMT+02:00 Andrew Lunn <andrew@lunn.ch>:
>> On Tue, Oct 06, 2015 at 03:22:40AM +0200, Marcin Wojtas wrote:
>>> The newest revisions of A388-GP (v1.5 and higher) support only
>>> DAT3-based card detection, which is enabled by this commit. Hitherto
>>> revisions, without such modification, will be impacted with a broken
>>> card detection - in order to operate the cards have to be present
>>> during kernel boot.
>>
>> Humm. Is this acceptable, breaking old boards?
>>
>> I would say at minimum, there should be a big fat comment at the top
>> of armada-388-gp.dts explaining that this DTS file is broken on
>> v0.0-v1.4.
>>
>> Or we have two .dts files for the 388-gp file, and a dtsi file.
>>
>
> I expected this patch would be controversial, hence I propose a
> compromise: set A388-GP SDHCI to 'broken-cd' by defeault. However
> Marvell insisted on HW card detection, because software polling spoils
> the SD/MMC benchmarks, but this way the user would decide whether to
> stay with broken-cd or switch to GPIO/DAT3 detection. What do you
> think?

I don't know hwo to correctly handle this case.

>From my point of view in a ideal work it is typically something that
sould be updated by the bootloader. However in the real world, we can't
rely on the bootloader :(

I also don't like having a dts for each new version of the board but as
the end you can see them as different boards. Also now it seems that the
distribution are moving to link the dtb and the kernel: for eaxh new
version of the kernel they also provide a new version of the dtb. That
means, that if we modify the dts, then the old board won't work with the
new kernel. So maybe creating a armada-388-gp-v1.5.dts could be the best
option.

What do you think of it?

Thanks,

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 6/8] ARM: mvebu: enable SDHCI card detection using DAT3 pin on A388-GP

[Marcin Wojtas]

Gregory,

2015-10-06 17:05 GMT+02:00 Gregory CLEMENT <gregory.clement@free-electrons.com>:
> Hi,
>
>  On mar., oct. 06 2015, Marcin Wojtas <mw@semihalf.com> wrote:
>
>> Hi Andrew,
>>
>> 2015-10-06 5:31 GMT+02:00 Andrew Lunn <andrew@lunn.ch>:
>>> On Tue, Oct 06, 2015 at 03:22:40AM +0200, Marcin Wojtas wrote:
>>>> The newest revisions of A388-GP (v1.5 and higher) support only
>>>> DAT3-based card detection, which is enabled by this commit. Hitherto
>>>> revisions, without such modification, will be impacted with a broken
>>>> card detection - in order to operate the cards have to be present
>>>> during kernel boot.
>>>
>>> Humm. Is this acceptable, breaking old boards?
>>>
>>> I would say at minimum, there should be a big fat comment at the top
>>> of armada-388-gp.dts explaining that this DTS file is broken on
>>> v0.0-v1.4.
>>>
>>> Or we have two .dts files for the 388-gp file, and a dtsi file.
>>>
>>
>> I expected this patch would be controversial, hence I propose a
>> compromise: set A388-GP SDHCI to 'broken-cd' by defeault. However
>> Marvell insisted on HW card detection, because software polling spoils
>> the SD/MMC benchmarks, but this way the user would decide whether to
>> stay with broken-cd or switch to GPIO/DAT3 detection. What do you
>> think?
>
> I don't know hwo to correctly handle this case.
>
> From my point of view in a ideal work it is typically something that
> sould be updated by the bootloader. However in the real world, we can't
> rely on the bootloader :(
>
> I also don't like having a dts for each new version of the board but as
> the end you can see them as different boards. Also now it seems that the
> distribution are moving to link the dtb and the kernel: for eaxh new
> version of the kernel they also provide a new version of the dtb. That
> means, that if we modify the dts, then the old board won't work with the
> new kernel. So maybe creating a armada-388-gp-v1.5.dts could be the best
> option.
>
> What do you think of it?
>

My first feeling is that creating brand new dts just for sdhci
detection (afaik there are no other bigger modifications of the PCB)
is not the best idea. However maybe changing armada-388-gp.dts to dtsi
and then add two single-entries' dts files on top is an option? Of
course we can satisfy each type of boards with 'broken-cd' polling
detection. I think it's rather a question of a taste and maintanance
point of view.

Best regards,
Marcin

[PATCH 6/8] ARM: mvebu: enable SDHCI card detection using DAT3 pin on A388-GP

[Andrew Lunn]

> My first feeling is that creating brand new dts just for sdhci
> detection (afaik there are no other bigger modifications of the PCB)
> is not the best idea. However maybe changing armada-388-gp.dts to dtsi
> and then add two single-entries' dts files on top is an option?

Yes, that is the correct way to do this, if we decide to have two DT
blobs for different hardware versions.

      Andrew

[PATCH 7/8] mmc: sdhci: add init_card callback to sdhci

[Marcin Wojtas]

Some sdhci hosts may require handling quirks during card initialization at
the time when its type is already known. Hence a new callback (init_card)
is added in sdhci_ops.

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 drivers/mmc/host/sdhci.c | 9 +++++++++
 drivers/mmc/host/sdhci.h | 1 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index cfed695..a1c308d 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2200,6 +2200,14 @@ static void sdhci_card_event(struct mmc_host *mmc)
 	spin_unlock_irqrestore(&host->lock, flags);
 }
 
+static void sdhci_init_card(struct mmc_host *mmc, struct mmc_card *card)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+
+	if (host->ops->init_card)
+		host->ops->init_card(host, card);
+}
+
 static const struct mmc_host_ops sdhci_ops = {
 	.request	= sdhci_request,
 	.post_req	= sdhci_post_req,
@@ -2215,6 +2223,7 @@ static const struct mmc_host_ops sdhci_ops = {
 	.select_drive_strength		= sdhci_select_drive_strength,
 	.card_event			= sdhci_card_event,
 	.card_busy	= sdhci_card_busy,
+	.init_card	= sdhci_init_card,
 };
 
 /*****************************************************************************\
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index c751b78..365c860 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -547,6 +547,7 @@ struct sdhci_ops {
 					 struct mmc_card *card,
 					 unsigned int max_dtr, int host_drv,
 					 int card_drv, int *drv_type);
+	void	(*init_card)(struct sdhci_host *host, struct mmc_card *card);
 };
 
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
-- 
1.8.3.1

[PATCH 8/8] mmc: sdhci-pxav3: enable modifying MMC_CARD bit during card initialization

[Marcin Wojtas]

On Marvell Armada 38x SoC's the MMC_CARD bit in SD_CE_ATA_1 register must
be set to 0x1 when a MMC card is supposed to work in DDR mode, or when
commands CMD11, CMD14 and CMD20 are used.

This commit enables the above for all MMC cards by modifying the host
registers during card initialization. It is done by using init_card()
callback.

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 drivers/mmc/host/sdhci-pxav3.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
index ce96640..315dc4e 100644
--- a/drivers/mmc/host/sdhci-pxav3.c
+++ b/drivers/mmc/host/sdhci-pxav3.c
@@ -57,6 +57,7 @@
 
 #define SD_SPI_MODE          0x108
 #define SD_CE_ATA_1          0x10C
+#define SDCE_MMC_CARD		BIT(28)
 
 #define SD_CE_ATA_2          0x10E
 #define SDCE_MISC_INT		(1<<2)
@@ -221,6 +222,22 @@ static void pxav3_reset(struct sdhci_host *host, u8 mask)
 	}
 }
 
+static void pxav3_init_card(struct sdhci_host *host, struct mmc_card *card)
+{
+	struct platform_device *pdev = to_platform_device(mmc_dev(host->mmc));
+	struct device_node *np = pdev->dev.of_node;
+	u32 reg_val;
+
+	if (of_device_is_compatible(np, "marvell,armada-380-sdhci")) {
+		reg_val = sdhci_readl(host, SD_CE_ATA_1);
+		if (mmc_card_mmc(card))
+			reg_val |= SDCE_MMC_CARD;
+		else
+			reg_val &= ~SDCE_MMC_CARD;
+		sdhci_writel(host, reg_val, SD_CE_ATA_1);
+	}
+}
+
 #define MAX_WAIT_COUNT 5
 static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode)
 {
@@ -338,6 +355,7 @@ static const struct sdhci_ops pxav3_sdhci_ops = {
 	.set_bus_width = sdhci_set_bus_width,
 	.reset = pxav3_reset,
 	.set_uhs_signaling = pxav3_set_uhs_signaling,
+	.init_card = pxav3_init_card,
 };
 
 static struct sdhci_pltfm_data sdhci_pxav3_pdata = {
-- 
1.8.3.1

[PATCH 0/8] Armada 38x SDHCI driver improvements

[Gregory CLEMENT]

Hi Marcin,
 
 On mar., oct. 06 2015, Marcin Wojtas <mw@semihalf.com> wrote:

> Hi,
>
> This series brings a couple of fixes and improvements to Armada 38x SDHCI
> controller driver. First four patches are fixes, of which three are stable
> CC'ed.
>
> Another two add DAT3-pin based hardware card detection in the driver, what
> should be used by the newest revisions of A388-GP boards.
>
> The last patches enable MMC_CARD bit, using init_card() callback added to
> SDHCI hosts.
>
> Any remarks and comments are welcome.

Thanks for this series, it looks good I have only one or two comments.

I also want to test it, how do you test the resume?
using standby or suspend to ram (by hacking the kernel as currently we
disbaled it) ?

Thanks,

Gregory

>
> Best regards,
> Marcin
>
> Marcin Wojtas (6):
>   mmc: sdhci-pxav3: fix error handling of armada_38x_quirks
>   mmc: sdhci-pxav3: enable proper resuming on Armada 38x SoC
>   mmc: sdhci-pxav3: enable usage of DAT3 pin as HW card detect
>   ARM: mvebu: enable SDHCI card detection using DAT3 pin on A388-GP
>   mmc: sdhci: add init_card callback to sdhci
>   mmc: sdhci-pxav3: enable modifying MMC_CARD bit during card
>     initialization
>
> Nadav Haklai (2):
>   mmc: sdhci-pxav3: remove broken clock base quirk for Armada 38x sdhci
>     driver
>   mmc: sdhci-pxav3: disable clock inversion for HS MMC cards
>
>  .../devicetree/bindings/mmc/sdhci-pxa.txt          |   5 +
>  arch/arm/boot/dts/armada-388-gp.dts                |   3 +-
>  drivers/mmc/host/sdhci-pxav3.c                     | 101 ++++++++++++++++-----
>  drivers/mmc/host/sdhci.c                           |  14 ++-
>  drivers/mmc/host/sdhci.h                           |   4 +
>  5 files changed, 102 insertions(+), 25 deletions(-)
>
> -- 
> 1.8.3.1
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/8] Armada 38x SDHCI driver improvements

[Marcin Wojtas]

Gregory,

>
> Thanks for this series, it looks good I have only one or two comments.
>
> I also want to test it, how do you test the resume?
> using standby or suspend to ram (by hacking the kernel as currently we
> disbaled it) ?
>

Standby works even without the patch, as the registers' contents do
not disappear. I added rejected s2ram support on top of 4.3-rc4 and
used it. Today however I changed my card to another one and got -110
error (timeout) - I have to re-check it. Anyway the patch is needed
for sure, because without MBUS window configuration any access to the
card ends up with a kernel hang.

Best regards,
Marcin

[PATCH 0/8] Armada 38x SDHCI driver improvements

[Marcin Wojtas]

Hi Gregory,

I have an update about s2ram status - after adding suspend/resume
support to pinctrl driver rootfs on SDHCI card survived suspend/resume
sequence without any problem (with broken-cd software polling, I also
have GP < v1.5).

Best regards,
Marcin

2015-10-06 17:48 GMT+02:00 Marcin Wojtas <mw@semihalf.com>:
> Gregory,
>
>>
>> Thanks for this series, it looks good I have only one or two comments.
>>
>> I also want to test it, how do you test the resume?
>> using standby or suspend to ram (by hacking the kernel as currently we
>> disbaled it) ?
>>
>
> Standby works even without the patch, as the registers' contents do
> not disappear. I added rejected s2ram support on top of 4.3-rc4 and
> used it. Today however I changed my card to another one and got -110
> error (timeout) - I have to re-check it. Anyway the patch is needed
> for sure, because without MBUS window configuration any access to the
> card ends up with a kernel hang.
>
> Best regards,
> Marcin