[PATCH 1/2] mmc: sunxi: fix support for new timings mode only SoCs

[PATCH 1/2] mmc: sunxi: fix support for new timings mode only SoCs

[Icenowy Zheng]

The A83T MMC support code introduces the timings mode switch, however
such a switch doesn't exist on new SoCs with only new timings mode.

Only execute the switch if the SoC really have the timings mode switch,
to fix the regression shown on new timings mode only SoCs (A64, H5,
etc).

Fixes: b0600daebf31 ("mmc: sunxi: Support controllers that can use both
old and new timings")

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
 drivers/mmc/host/sunxi-mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 3777517982dd..59aba93beffb 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -784,7 +784,7 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host,
 		clock <<= 1;
 	}
 
-	if (host->use_new_timings) {
+	if (host->use_new_timings && host->cfg->has_timings_switch) {
 		ret = sunxi_ccu_set_mmc_timing_mode(host->clk_mmc, true);
 		if (ret) {
 			dev_err(mmc_dev(mmc),
-- 
2.13.0

[PATCH 2/2] mmc: sunxi: fix new timings mode on A64 EMMC (MMC2) controller

[Icenowy Zheng]

The configuration struct of A64 EMMC(MMC2) compatible used to
have the needs_new_timings variable missing, which lead to NULL
pointer dereference now when trying to set up the old timings mode, as
the old timings mode doesn't exist at all on A64.

Fix this issue by adding this variable and setting it to true in
the configuration struct.

Fixes: 4fb3ce07eafa ("mmc: sunxi: Add EMMC (MMC2) controller compatible")

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
 drivers/mmc/host/sunxi-mmc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 59aba93beffb..4ad643e37014 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -1142,6 +1142,7 @@ static const struct sunxi_mmc_cfg sun50i_a64_emmc_cfg = {
 	.idma_des_size_bits = 13,
 	.clk_delays = NULL,
 	.can_calibrate = true,
+	.needs_new_timings = true,
 };
 
 static const struct of_device_id sunxi_mmc_of_match[] = {
-- 
2.13.0

[linux-sunxi] [PATCH 2/2] mmc: sunxi: fix new timings mode on A64 EMMC (MMC2) controller

[Chen-Yu Tsai]

On Sat, Aug 05, 2017 at 05:35:55AM +0800, Icenowy Zheng wrote:
> The configuration struct of A64 EMMC(MMC2) compatible used to
> have the needs_new_timings variable missing, which lead to NULL
> pointer dereference now when trying to set up the old timings mode, as
> the old timings mode doesn't exist at all on A64.

I'm not familiar with the A64's eMMC controller. The datasheet says
it does not have the timing switch register. It does not say whether
it is always in the old or new timing mode. "needs_new_timings"
probably meant that the switch has to be set.

This fix doesn't really fix the underlying issue, which is the check
for clk_delays was incorrectly replaced. sun4i/sun5i is also broken.

Could you try this patch instead:

<---

>From 23b841a11294cb6a0cf1e146616b068f60c2ec7d Mon Sep 17 00:00:00 2001
From: Chen-Yu Tsai <wens@csie.org>
Date: Sun, 6 Aug 2017 10:24:47 +0800
Subject: [PATCH] mmc: sunxi: Fix NULL pointer reference on clk_delays

Some SoCs do not support clk delays for MMC in the clock control unit.
These include the old controllers in A10/A10s/A13/R8, and the new eMMC
controller in A64. The config structure for these controllers do not
specify clk_delays, but the check for this was replaced in commit
b0600daebf31 ("mmc: sunxi: Support controllers that can use both old
and new timings").

This patch adds back the check for clk_delays, and also adds comments
for both checks in sunxi_mmc_clk_set_phase().

Fixes: b0600daebf31 ("mmc: sunxi: Support controllers that can use
		      both old and new timings")
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/mmc/host/sunxi-mmc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 3777517982dd..020547e5fa45 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -722,9 +722,14 @@ static int sunxi_mmc_clk_set_phase(struct sunxi_mmc_host *host,
 {
 	int index;
 
+	/* No need to set clk controller delays under new timings */
 	if (host->use_new_timings)
 		return 0;
 
+	/* Some old controllers don't support delays */
+	if (!host->cfg->clk_delays)
+		return 0;
+
 	/* determine delays */
 	if (rate <= 400000) {
 		index = SDXC_CLK_400K;
-- 
2.13.3

<---

Separately we should ask what is the proper set of config flags that
describes the A64 EMMC controller.

Regards
ChenYu

> 
> Fix this issue by adding this variable and setting it to true in
> the configuration struct.
> 
> Fixes: 4fb3ce07eafa ("mmc: sunxi: Add EMMC (MMC2) controller compatible")
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> ---
>  drivers/mmc/host/sunxi-mmc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> index 59aba93beffb..4ad643e37014 100644
> --- a/drivers/mmc/host/sunxi-mmc.c
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -1142,6 +1142,7 @@ static const struct sunxi_mmc_cfg sun50i_a64_emmc_cfg = {
>  	.idma_des_size_bits = 13,
>  	.clk_delays = NULL,
>  	.can_calibrate = true,
> +	.needs_new_timings = true,
>  };
>  
>  static const struct of_device_id sunxi_mmc_of_match[] = {
> -- 
> 2.13.0
> 
> -- 
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe at googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] [PATCH 2/2] mmc: sunxi: fix new timings mode on A64 EMMC (MMC2) controller

[Icenowy Zheng]

? 2017?8?6? GMT+08:00 ??10:39:54, Chen-Yu Tsai <wens@csie.org> ??:
>On Sat, Aug 05, 2017 at 05:35:55AM +0800, Icenowy Zheng wrote:
>> The configuration struct of A64 EMMC(MMC2) compatible used to
>> have the needs_new_timings variable missing, which lead to NULL
>> pointer dereference now when trying to set up the old timings mode,
>as
>> the old timings mode doesn't exist at all on A64.
>
>I'm not familiar with the A64's eMMC controller. The datasheet says
>it does not have the timing switch register. It does not say whether
>it is always in the old or new timing mode. "needs_new_timings"
>probably meant that the switch has to be set.

As I know, it supports DDR mode, but the CCU has no clk delays.
So it should be the new timings mode.

>
>This fix doesn't really fix the underlying issue, which is the check
>for clk_delays was incorrectly replaced. sun4i/sun5i is also broken.
>
>Could you try this patch instead:
>
><---
>
>From 23b841a11294cb6a0cf1e146616b068f60c2ec7d Mon Sep 17 00:00:00 2001
>From: Chen-Yu Tsai <wens@csie.org>
>Date: Sun, 6 Aug 2017 10:24:47 +0800
>Subject: [PATCH] mmc: sunxi: Fix NULL pointer reference on clk_delays
>
>Some SoCs do not support clk delays for MMC in the clock control unit.
>These include the old controllers in A10/A10s/A13/R8, and the new eMMC
>controller in A64. The config structure for these controllers do not
>specify clk_delays, but the check for this was replaced in commit
>b0600daebf31 ("mmc: sunxi: Support controllers that can use both old
>and new timings").
>
>This patch adds back the check for clk_delays, and also adds comments
>for both checks in sunxi_mmc_clk_set_phase().
>
>Fixes: b0600daebf31 ("mmc: sunxi: Support controllers that can use
>		      both old and new timings")
>Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>---
> drivers/mmc/host/sunxi-mmc.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
>diff --git a/drivers/mmc/host/sunxi-mmc.c
>b/drivers/mmc/host/sunxi-mmc.c
>index 3777517982dd..020547e5fa45 100644
>--- a/drivers/mmc/host/sunxi-mmc.c
>+++ b/drivers/mmc/host/sunxi-mmc.c
>@@ -722,9 +722,14 @@ static int sunxi_mmc_clk_set_phase(struct
>sunxi_mmc_host *host,
> {
> 	int index;
> 
>+	/* No need to set clk controller delays under new timings */
> 	if (host->use_new_timings)
> 		return 0;
> 
>+	/* Some old controllers don't support delays */
>+	if (!host->cfg->clk_delays)
>+		return 0;
>+
> 	/* determine delays */
> 	if (rate <= 400000) {
> 		index = SDXC_CLK_400K;

[linux-sunxi] [PATCH 2/2] mmc: sunxi: fix new timings mode on A64 EMMC (MMC2) controller

[icenowy at aosc.io]

? 2017-08-06 10:39?Chen-Yu Tsai ???
> On Sat, Aug 05, 2017 at 05:35:55AM +0800, Icenowy Zheng wrote:
>> The configuration struct of A64 EMMC(MMC2) compatible used to
>> have the needs_new_timings variable missing, which lead to NULL
>> pointer dereference now when trying to set up the old timings mode, as
>> the old timings mode doesn't exist at all on A64.
> 
> I'm not familiar with the A64's eMMC controller. The datasheet says
> it does not have the timing switch register. It does not say whether
> it is always in the old or new timing mode. "needs_new_timings"
> probably meant that the switch has to be set.
> 
> This fix doesn't really fix the underlying issue, which is the check
> for clk_delays was incorrectly replaced. sun4i/sun5i is also broken.
> 
> Could you try this patch instead:
> 
> <---
> 
> From 23b841a11294cb6a0cf1e146616b068f60c2ec7d Mon Sep 17 00:00:00 2001
> From: Chen-Yu Tsai <wens@csie.org>
> Date: Sun, 6 Aug 2017 10:24:47 +0800
> Subject: [PATCH] mmc: sunxi: Fix NULL pointer reference on clk_delays
> 
> Some SoCs do not support clk delays for MMC in the clock control unit.
> These include the old controllers in A10/A10s/A13/R8, and the new eMMC
> controller in A64. The config structure for these controllers do not
> specify clk_delays, but the check for this was replaced in commit
> b0600daebf31 ("mmc: sunxi: Support controllers that can use both old
> and new timings").
> 
> This patch adds back the check for clk_delays, and also adds comments
> for both checks in sunxi_mmc_clk_set_phase().
> 
> Fixes: b0600daebf31 ("mmc: sunxi: Support controllers that can use
> 		      both old and new timings")
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Successfully tested on A64 (SoPine with baseboard and eMMC module).

> ---
>  drivers/mmc/host/sunxi-mmc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/mmc/host/sunxi-mmc.c 
> b/drivers/mmc/host/sunxi-mmc.c
> index 3777517982dd..020547e5fa45 100644
> --- a/drivers/mmc/host/sunxi-mmc.c
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -722,9 +722,14 @@ static int sunxi_mmc_clk_set_phase(struct
> sunxi_mmc_host *host,
>  {
>  	int index;
> 
> +	/* No need to set clk controller delays under new timings */
>  	if (host->use_new_timings)
>  		return 0;
> 
> +	/* Some old controllers don't support delays */
> +	if (!host->cfg->clk_delays)
> +		return 0;
> +
>  	/* determine delays */
>  	if (rate <= 400000) {
>  		index = SDXC_CLK_400K;
> --
> 2.13.3
> 
> <---
> 
> Separately we should ask what is the proper set of config flags that
> describes the A64 EMMC controller.
> 
> Regards
> ChenYu
> 
>> 
>> Fix this issue by adding this variable and setting it to true in
>> the configuration struct.
>> 
>> Fixes: 4fb3ce07eafa ("mmc: sunxi: Add EMMC (MMC2) controller 
>> compatible")
>> 
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> ---
>>  drivers/mmc/host/sunxi-mmc.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/drivers/mmc/host/sunxi-mmc.c 
>> b/drivers/mmc/host/sunxi-mmc.c
>> index 59aba93beffb..4ad643e37014 100644
>> --- a/drivers/mmc/host/sunxi-mmc.c
>> +++ b/drivers/mmc/host/sunxi-mmc.c
>> @@ -1142,6 +1142,7 @@ static const struct sunxi_mmc_cfg 
>> sun50i_a64_emmc_cfg = {
>>  	.idma_des_size_bits = 13,
>>  	.clk_delays = NULL,
>>  	.can_calibrate = true,
>> +	.needs_new_timings = true,
>>  };
>> 
>>  static const struct of_device_id sunxi_mmc_of_match[] = {
>> --
>> 2.13.0
>> 
>> --
>> You received this message because you are subscribed to the Google 
>> Groups "linux-sunxi" group.
>> To unsubscribe from this group and stop receiving emails from it, send 
>> an email to linux-sunxi+unsubscribe at googlegroups.com.
>> For more options, visit https://groups.google.com/d/optout.
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[linux-sunxi] [PATCH 1/2] mmc: sunxi: fix support for new timings mode only SoCs

[Chen-Yu Tsai]

On Sat, Aug 5, 2017 at 5:35 AM, Icenowy Zheng <icenowy@aosc.io> wrote:
> The A83T MMC support code introduces the timings mode switch, however
> such a switch doesn't exist on new SoCs with only new timings mode.
>
> Only execute the switch if the SoC really have the timings mode switch,
> to fix the regression shown on new timings mode only SoCs (A64, H5,
> etc).
>
> Fixes: b0600daebf31 ("mmc: sunxi: Support controllers that can use both
> old and new timings")

You could make it easier to read by aligning the wrapped line with
opening parenthesis and double quote.

And you don't need the newline here.

>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>

This must have slipped through when I reworked the mmc quirks.

Reviewed-by: Chen-Yu Tsai <wens@csie.org>