diff mbox series

[1/7] Revert "mmc: zynq: parse dt when probing"

Message ID 84f084eee198fb340631c739df6c05550a1a995a.1590144247.git.michal.simek@xilinx.com
State New
Delegated to: Michal Simek
Headers show
Series mmc: zynqmp_sdhci: Add support for Tap delay | expand

Commit Message

Michal Simek May 22, 2020, 10:44 a.m. UTC
From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>

This reverts commit 942b5fc03218d1c94468fc658e7dec65dabcc830.

This is partial revert of the above commit.

mmc_of_parse() is reading no-1-8-v from device tree and if set,
it is clearing the UHS speed capabilities of cfg->host_caps.
cfg->host_caps &= ~(UHS_CAPS | MMC_MODE_HS200 |
		    MMC_MODE_HS400 | MMC_MODE_HS400_ES);

This is still missing to clear UHS speeds like SDHCI_SUPPORT_SDR104,
SDHCI_SUPPORT_SDR50 and SDHCI_SUPPORT_DDR50.

Even if we clear the flags SDHCI_SUPPORT_XXX in mmc_of_parse(),
these speed flags are getting set again in cfg->host_caps in
sdhci_setup_cfg().

The reason for this is, SDHCI_SUPPORT_XXX flags are cleared
only if controller is not capable of supporting MMC_VDD_165_195 volts.

if (caps & SDHCI_CAN_VDD_180)
	cfg->voltages |= MMC_VDD_165_195;

if (!(cfg->voltages & MMC_VDD_165_195))
	caps_1 &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
		    SDHCI_SUPPORT_DDR50);

It means "no-1-8-v", which is read from DT is not coming in to effect.
So it is better we keep the host quirks(SDHCI_QUIRK_NO_1_8_V) to
clear UHS speeds based on no-1-8-v from device tree.

Hence revert the functionality related to no-1-8-v only, rest is fine
in the patch.

Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

 drivers/mmc/sdhci.c      | 3 ++-
 drivers/mmc/zynq_sdhci.c | 5 +++++
 include/sdhci.h          | 1 +
 3 files changed, 8 insertions(+), 1 deletion(-)

Comments

Jaehoon Chung May 27, 2020, 6:44 a.m. UTC | #1
Hi

On 5/22/20 7:44 PM, Michal Simek wrote:
> From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
> 
> This reverts commit 942b5fc03218d1c94468fc658e7dec65dabcc830.
> 
> This is partial revert of the above commit.
> 
> mmc_of_parse() is reading no-1-8-v from device tree and if set,
> it is clearing the UHS speed capabilities of cfg->host_caps.
> cfg->host_caps &= ~(UHS_CAPS | MMC_MODE_HS200 |
> 		    MMC_MODE_HS400 | MMC_MODE_HS400_ES);
> 
> This is still missing to clear UHS speeds like SDHCI_SUPPORT_SDR104,
> SDHCI_SUPPORT_SDR50 and SDHCI_SUPPORT_DDR50.
> 
> Even if we clear the flags SDHCI_SUPPORT_XXX in mmc_of_parse(),
> these speed flags are getting set again in cfg->host_caps in
> sdhci_setup_cfg().
> 
> The reason for this is, SDHCI_SUPPORT_XXX flags are cleared
> only if controller is not capable of supporting MMC_VDD_165_195 volts.
> 
> if (caps & SDHCI_CAN_VDD_180)
> 	cfg->voltages |= MMC_VDD_165_195;
> 
> if (!(cfg->voltages & MMC_VDD_165_195))
> 	caps_1 &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
> 		    SDHCI_SUPPORT_DDR50);

I don't have a target to test with zynq_sdhci.
If there is no effect no-1-8-v property, i think that it needs to fix main problem.
I will check about this.
 
> 
> It means "no-1-8-v", which is read from DT is not coming in to effect.
> So it is better we keep the host quirks(SDHCI_QUIRK_NO_1_8_V) to
> clear UHS speeds based on no-1-8-v from device tree.
> 
> Hence revert the functionality related to no-1-8-v only, rest is fine
> in the patch.

Anyway, I don't have any objection about reverting it.

> 
> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> 
>  drivers/mmc/sdhci.c      | 3 ++-
>  drivers/mmc/zynq_sdhci.c | 5 +++++
>  include/sdhci.h          | 1 +
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
> index 92cc8434af24..099a007984a8 100644
> --- a/drivers/mmc/sdhci.c
> +++ b/drivers/mmc/sdhci.c
> @@ -844,7 +844,8 @@ int sdhci_setup_cfg(struct mmc_config *cfg, struct sdhci_host *host,
>  		cfg->host_caps &= ~MMC_MODE_HS_52MHz;
>  	}
>  
> -	if (!(cfg->voltages & MMC_VDD_165_195))
> +	if (!(cfg->voltages & MMC_VDD_165_195) ||
> +	    (host->quirks & SDHCI_QUIRK_NO_1_8_V))
>  		caps_1 &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
>  			    SDHCI_SUPPORT_DDR50);
>  
> diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c
> index 43b9f215229a..94c69cf1c1bd 100644
> --- a/drivers/mmc/zynq_sdhci.c
> +++ b/drivers/mmc/zynq_sdhci.c
> @@ -28,6 +28,7 @@ struct arasan_sdhci_priv {
>  	struct sdhci_host *host;
>  	u8 deviceid;
>  	u8 bank;
> +	u8 no_1p8;
>  };
>  
>  #if defined(CONFIG_ARCH_ZYNQMP)
> @@ -236,6 +237,9 @@ static int arasan_sdhci_probe(struct udevice *dev)
>  	host->quirks |= SDHCI_QUIRK_BROKEN_HISPD_MODE;
>  #endif
>  
> +	if (priv->no_1p8)
> +		host->quirks |= SDHCI_QUIRK_NO_1_8_V;
> +
>  	plat->cfg.f_max = CONFIG_ZYNQ_SDHCI_MAX_FREQ;
>  
>  	ret = mmc_of_parse(dev, &plat->cfg);
> @@ -277,6 +281,7 @@ static int arasan_sdhci_ofdata_to_platdata(struct udevice *dev)
>  
>  	priv->deviceid = dev_read_u32_default(dev, "xlnx,device_id", -1);
>  	priv->bank = dev_read_u32_default(dev, "xlnx,mio_bank", -1);
> +	priv->no_1p8 = dev_read_bool(dev, "no-1-8-v");
>  
>  	return 0;
>  }
> diff --git a/include/sdhci.h b/include/sdhci.h
> index 94fc3ed56ace..ef84bebcb4aa 100644
> --- a/include/sdhci.h
> +++ b/include/sdhci.h
> @@ -244,6 +244,7 @@
>  #define SDHCI_QUIRK_BROKEN_HISPD_MODE	BIT(5)
>  #define SDHCI_QUIRK_WAIT_SEND_CMD	(1 << 6)
>  #define SDHCI_QUIRK_USE_WIDE8		(1 << 8)
> +#define SDHCI_QUIRK_NO_1_8_V		(1 << 9)
>  
>  /* to make gcc happy */
>  struct sdhci_host;
>
Michal Simek June 8, 2020, 1:16 p.m. UTC | #2
On 27. 05. 20 8:44, Jaehoon Chung wrote:
> Hi
> 
> On 5/22/20 7:44 PM, Michal Simek wrote:
>> From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
>>
>> This reverts commit 942b5fc03218d1c94468fc658e7dec65dabcc830.
>>
>> This is partial revert of the above commit.
>>
>> mmc_of_parse() is reading no-1-8-v from device tree and if set,
>> it is clearing the UHS speed capabilities of cfg->host_caps.
>> cfg->host_caps &= ~(UHS_CAPS | MMC_MODE_HS200 |
>> 		    MMC_MODE_HS400 | MMC_MODE_HS400_ES);
>>
>> This is still missing to clear UHS speeds like SDHCI_SUPPORT_SDR104,
>> SDHCI_SUPPORT_SDR50 and SDHCI_SUPPORT_DDR50.
>>
>> Even if we clear the flags SDHCI_SUPPORT_XXX in mmc_of_parse(),
>> these speed flags are getting set again in cfg->host_caps in
>> sdhci_setup_cfg().
>>
>> The reason for this is, SDHCI_SUPPORT_XXX flags are cleared
>> only if controller is not capable of supporting MMC_VDD_165_195 volts.
>>
>> if (caps & SDHCI_CAN_VDD_180)
>> 	cfg->voltages |= MMC_VDD_165_195;
>>
>> if (!(cfg->voltages & MMC_VDD_165_195))
>> 	caps_1 &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
>> 		    SDHCI_SUPPORT_DDR50);
> 
> I don't have a target to test with zynq_sdhci.
> If there is no effect no-1-8-v property, i think that it needs to fix main problem.
> I will check about this.

Have you had any time to take a look at it?

Thanks,
Michal
Jaehoon Chung June 9, 2020, 11:43 a.m. UTC | #3
Hi,

On 6/8/20 10:16 PM, Michal Simek wrote:
> On 27. 05. 20 8:44, Jaehoon Chung wrote:
>> Hi
>>
>> On 5/22/20 7:44 PM, Michal Simek wrote:
>>> From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
>>>
>>> This reverts commit 942b5fc03218d1c94468fc658e7dec65dabcc830.
>>>
>>> This is partial revert of the above commit.
>>>
>>> mmc_of_parse() is reading no-1-8-v from device tree and if set,
>>> it is clearing the UHS speed capabilities of cfg->host_caps.
>>> cfg->host_caps &= ~(UHS_CAPS | MMC_MODE_HS200 |
>>> 		    MMC_MODE_HS400 | MMC_MODE_HS400_ES);
>>>
>>> This is still missing to clear UHS speeds like SDHCI_SUPPORT_SDR104,
>>> SDHCI_SUPPORT_SDR50 and SDHCI_SUPPORT_DDR50.
>>>
>>> Even if we clear the flags SDHCI_SUPPORT_XXX in mmc_of_parse(),
>>> these speed flags are getting set again in cfg->host_caps in
>>> sdhci_setup_cfg().
>>>
>>> The reason for this is, SDHCI_SUPPORT_XXX flags are cleared
>>> only if controller is not capable of supporting MMC_VDD_165_195 volts.
>>>
>>> if (caps & SDHCI_CAN_VDD_180)
>>> 	cfg->voltages |= MMC_VDD_165_195;
>>>
>>> if (!(cfg->voltages & MMC_VDD_165_195))
>>> 	caps_1 &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
>>> 		    SDHCI_SUPPORT_DDR50);
>>
>> I don't have a target to test with zynq_sdhci.
>> If there is no effect no-1-8-v property, i think that it needs to fix main problem.
>> I will check about this.
> 
> Have you had any time to take a look at it?
Sorry for late. I think it was similar issue on my environment. 
I will share result within this week. 
If it's urgent, then after merging i will find solution to fix it.

Best Regards,
Jaehoon Chung

> 
> Thanks,
> Michal
>
diff mbox series

Patch

diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
index 92cc8434af24..099a007984a8 100644
--- a/drivers/mmc/sdhci.c
+++ b/drivers/mmc/sdhci.c
@@ -844,7 +844,8 @@  int sdhci_setup_cfg(struct mmc_config *cfg, struct sdhci_host *host,
 		cfg->host_caps &= ~MMC_MODE_HS_52MHz;
 	}
 
-	if (!(cfg->voltages & MMC_VDD_165_195))
+	if (!(cfg->voltages & MMC_VDD_165_195) ||
+	    (host->quirks & SDHCI_QUIRK_NO_1_8_V))
 		caps_1 &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
 			    SDHCI_SUPPORT_DDR50);
 
diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c
index 43b9f215229a..94c69cf1c1bd 100644
--- a/drivers/mmc/zynq_sdhci.c
+++ b/drivers/mmc/zynq_sdhci.c
@@ -28,6 +28,7 @@  struct arasan_sdhci_priv {
 	struct sdhci_host *host;
 	u8 deviceid;
 	u8 bank;
+	u8 no_1p8;
 };
 
 #if defined(CONFIG_ARCH_ZYNQMP)
@@ -236,6 +237,9 @@  static int arasan_sdhci_probe(struct udevice *dev)
 	host->quirks |= SDHCI_QUIRK_BROKEN_HISPD_MODE;
 #endif
 
+	if (priv->no_1p8)
+		host->quirks |= SDHCI_QUIRK_NO_1_8_V;
+
 	plat->cfg.f_max = CONFIG_ZYNQ_SDHCI_MAX_FREQ;
 
 	ret = mmc_of_parse(dev, &plat->cfg);
@@ -277,6 +281,7 @@  static int arasan_sdhci_ofdata_to_platdata(struct udevice *dev)
 
 	priv->deviceid = dev_read_u32_default(dev, "xlnx,device_id", -1);
 	priv->bank = dev_read_u32_default(dev, "xlnx,mio_bank", -1);
+	priv->no_1p8 = dev_read_bool(dev, "no-1-8-v");
 
 	return 0;
 }
diff --git a/include/sdhci.h b/include/sdhci.h
index 94fc3ed56ace..ef84bebcb4aa 100644
--- a/include/sdhci.h
+++ b/include/sdhci.h
@@ -244,6 +244,7 @@ 
 #define SDHCI_QUIRK_BROKEN_HISPD_MODE	BIT(5)
 #define SDHCI_QUIRK_WAIT_SEND_CMD	(1 << 6)
 #define SDHCI_QUIRK_USE_WIDE8		(1 << 8)
+#define SDHCI_QUIRK_NO_1_8_V		(1 << 9)
 
 /* to make gcc happy */
 struct sdhci_host;