diff mbox

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

Message ID 20170806023954.wwaredjwcm2p5pw7@wens.csie.org (mailing list archive)
State New, archived
Headers show

Commit Message

Chen-Yu Tsai Aug. 6, 2017, 2:39 a.m. UTC
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(+)

Comments

Icenowy Zheng Aug. 6, 2017, 3:20 a.m. UTC | #1
于 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;
Icenowy Zheng Aug. 7, 2017, 6:59 a.m. UTC | #2
在 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@googlegroups.com.
>> For more options, visit https://groups.google.com/d/optout.
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff mbox

Patch

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;