diff mbox

[07/23] mmc: sdhci: check SDHCI_QUIRK2_NO_1_8_V when do voltage switch

Message ID 1460741387-23815-8-git-send-email-aisheng.dong@nxp.com (mailing list archive)
State New, archived
Headers show

Commit Message

Aisheng Dong April 15, 2016, 5:29 p.m. UTC
Currently when card type supports EXT_CSD_CARD_TYPE_DDR_1_8V
which means it can work on DDR mode with either 3.3v IO or 1.8v
IO voltage. MMC core will first try 1.8v then 3.3v if host claims
MMC_CAP_1_8V_DDR support.
However the host driver voltage switch code does not check NO_1_8_V
quirk which may set a wrong 1.8v and causes the card fixed to 3.3v
VIO un-work.

Checking 1.8V quirk before setting it to avoid such issue.

CC: stable <stable@vger.kernel.org>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/mmc/host/sdhci.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Adrian Hunter April 22, 2016, 12:30 p.m. UTC | #1
On 15/04/16 20:29, Dong Aisheng wrote:
> Currently when card type supports EXT_CSD_CARD_TYPE_DDR_1_8V
> which means it can work on DDR mode with either 3.3v IO or 1.8v
> IO voltage. MMC core will first try 1.8v then 3.3v if host claims
> MMC_CAP_1_8V_DDR support.
> However the host driver voltage switch code does not check NO_1_8_V
> quirk which may set a wrong 1.8v and causes the card fixed to 3.3v
> VIO un-work.
> 
> Checking 1.8V quirk before setting it to avoid such issue.

We need to look forward to when SDHCI_QUIRK2_NO_1_8_V doesn't exist.

There are two possibilities:

1. Add MMC_CAP_3_3V_DDR and support to core and use that instead of
MMC_CAP_1_8V_DDR.  You'll need Ulf's feedback on that.

2. Replace ->start_signal_voltage_switch() i.e.

host->mmc_host_ops.start_signal_voltage_switch =
esdhci_start_signal_voltage_switch;

You will also need to change all calls to
sdhci_start_signal_voltage_switch() with
host->mmc->ops->start_signal_voltage_switch(), but that needs to be done anyway.

> 
> CC: stable <stable@vger.kernel.org>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  drivers/mmc/host/sdhci.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 2338aab..96ccb15 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1683,6 +1683,8 @@ static int sdhci_do_signal_voltage_switch(struct sdhci_host *host,
>  
>  		return -EAGAIN;
>  	case MMC_SIGNAL_VOLTAGE_180:
> +		if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V)
> +			return -EINVAL;
>  		/*
>  		 * Enable 1.8V Signal Enable in the Host Control2
>  		 * register
>
Dong Aisheng April 24, 2016, 9:56 a.m. UTC | #2
On Fri, Apr 22, 2016 at 8:30 PM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 15/04/16 20:29, Dong Aisheng wrote:
>> Currently when card type supports EXT_CSD_CARD_TYPE_DDR_1_8V
>> which means it can work on DDR mode with either 3.3v IO or 1.8v
>> IO voltage. MMC core will first try 1.8v then 3.3v if host claims
>> MMC_CAP_1_8V_DDR support.
>> However the host driver voltage switch code does not check NO_1_8_V
>> quirk which may set a wrong 1.8v and causes the card fixed to 3.3v
>> VIO un-work.
>>
>> Checking 1.8V quirk before setting it to avoid such issue.
>
> We need to look forward to when SDHCI_QUIRK2_NO_1_8_V doesn't exist.
>

Yes, i did try to clean up SDHCI_QUIRK2_NO_1_8_V.
However, it seems not that simply.
Currently we may still need it before we got a better way.

My point is whether we should stop fixing the exist issue in SDHCI driver
just caused by we want to clean up it later?

> There are two possibilities:
>
> 1. Add MMC_CAP_3_3V_DDR and support to core and use that instead of
> MMC_CAP_1_8V_DDR.  You'll need Ulf's feedback on that.
>

Yes, we can do it.
But the point is for DDR50/SD3.0/SDIO3.0/HS200/HS400 cards we have
the same situation and still need QUIRK2_NO_1_8_V.

And i somehow a bit wonder whether we should retrieve the speed mode
support from device tree since it's actually controller capability.
IO range capability is another thing.

Probably we may start another topic to discuss it specificly.

> 2. Replace ->start_signal_voltage_switch() i.e.
>
> host->mmc_host_ops.start_signal_voltage_switch =
> esdhci_start_signal_voltage_switch;
>
> You will also need to change all calls to
> sdhci_start_signal_voltage_switch() with
> host->mmc->ops->start_signal_voltage_switch(), but that needs to be done anyway.
>

esdhc can fully use the common sdhci_start_signal_voltag_switch.
Since it's already support QUIRK_NO_1_8_V, we don't want to
invent imx voltage swith currently, but fix and use it first.

Regards
Dong Aisheng

>>
>> CC: stable <stable@vger.kernel.org>
>> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
>> ---
>>  drivers/mmc/host/sdhci.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 2338aab..96ccb15 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1683,6 +1683,8 @@ static int sdhci_do_signal_voltage_switch(struct sdhci_host *host,
>>
>>               return -EAGAIN;
>>       case MMC_SIGNAL_VOLTAGE_180:
>> +             if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V)
>> +                     return -EINVAL;
>>               /*
>>                * Enable 1.8V Signal Enable in the Host Control2
>>                * register
>>
>
Adrian Hunter April 27, 2016, 8:27 p.m. UTC | #3
On 24/04/2016 12:56 p.m., Dong Aisheng wrote:
> On Fri, Apr 22, 2016 at 8:30 PM, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 15/04/16 20:29, Dong Aisheng wrote:
>>> Currently when card type supports EXT_CSD_CARD_TYPE_DDR_1_8V
>>> which means it can work on DDR mode with either 3.3v IO or 1.8v
>>> IO voltage. MMC core will first try 1.8v then 3.3v if host claims
>>> MMC_CAP_1_8V_DDR support.
>>> However the host driver voltage switch code does not check NO_1_8_V
>>> quirk which may set a wrong 1.8v and causes the card fixed to 3.3v
>>> VIO un-work.
>>>
>>> Checking 1.8V quirk before setting it to avoid such issue.
>>
>> We need to look forward to when SDHCI_QUIRK2_NO_1_8_V doesn't exist.
>>
>
> Yes, i did try to clean up SDHCI_QUIRK2_NO_1_8_V.
> However, it seems not that simply.
> Currently we may still need it before we got a better way.
>
> My point is whether we should stop fixing the exist issue in SDHCI driver
> just caused by we want to clean up it later?

If you want a fix for stable kernels, then I guess it is OK.  In that case
it is better to have a fix that applies cleanly to older kernels
i.e. not on top of re-factoring

>
>> There are two possibilities:
>>
>> 1. Add MMC_CAP_3_3V_DDR and support to core and use that instead of
>> MMC_CAP_1_8V_DDR.  You'll need Ulf's feedback on that.
>>
>
> Yes, we can do it.
> But the point is for DDR50/SD3.0/SDIO3.0/HS200/HS400 cards we have
> the same situation and still need QUIRK2_NO_1_8_V.
>
> And i somehow a bit wonder whether we should retrieve the speed mode
> support from device tree since it's actually controller capability.
> IO range capability is another thing.
>
> Probably we may start another topic to discuss it specificly.
>
>> 2. Replace ->start_signal_voltage_switch() i.e.
>>
>> host->mmc_host_ops.start_signal_voltage_switch =
>> esdhci_start_signal_voltage_switch;
>>
>> You will also need to change all calls to
>> sdhci_start_signal_voltage_switch() with
>> host->mmc->ops->start_signal_voltage_switch(), but that needs to be done anyway.
>>
>
> esdhc can fully use the common sdhci_start_signal_voltag_switch.
> Since it's already support QUIRK_NO_1_8_V, we don't want to
> invent imx voltage swith currently, but fix and use it first.
>
> Regards
> Dong Aisheng
>
>>>
>>> CC: stable <stable@vger.kernel.org>
>>> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
>>> ---
>>>   drivers/mmc/host/sdhci.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 2338aab..96ccb15 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -1683,6 +1683,8 @@ static int sdhci_do_signal_voltage_switch(struct sdhci_host *host,
>>>
>>>                return -EAGAIN;
>>>        case MMC_SIGNAL_VOLTAGE_180:
>>> +             if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V)
>>> +                     return -EINVAL;
>>>                /*
>>>                 * Enable 1.8V Signal Enable in the Host Control2
>>>                 * register
>>>
>>
Dong Aisheng April 28, 2016, 1:24 p.m. UTC | #4
On Wed, Apr 27, 2016 at 11:27:07PM +0300, Adrian Hunter wrote:
> On 24/04/2016 12:56 p.m., Dong Aisheng wrote:
> >On Fri, Apr 22, 2016 at 8:30 PM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>On 15/04/16 20:29, Dong Aisheng wrote:
> >>>Currently when card type supports EXT_CSD_CARD_TYPE_DDR_1_8V
> >>>which means it can work on DDR mode with either 3.3v IO or 1.8v
> >>>IO voltage. MMC core will first try 1.8v then 3.3v if host claims
> >>>MMC_CAP_1_8V_DDR support.
> >>>However the host driver voltage switch code does not check NO_1_8_V
> >>>quirk which may set a wrong 1.8v and causes the card fixed to 3.3v
> >>>VIO un-work.
> >>>
> >>>Checking 1.8V quirk before setting it to avoid such issue.
> >>
> >>We need to look forward to when SDHCI_QUIRK2_NO_1_8_V doesn't exist.
> >>
> >
> >Yes, i did try to clean up SDHCI_QUIRK2_NO_1_8_V.
> >However, it seems not that simply.
> >Currently we may still need it before we got a better way.
> >
> >My point is whether we should stop fixing the exist issue in SDHCI driver
> >just caused by we want to clean up it later?
> 
> If you want a fix for stable kernels, then I guess it is OK.  In that case
> it is better to have a fix that applies cleanly to older kernels
> i.e. not on top of re-factoring
> 

Okay, i would think more about how to remove SDHCI_QUIRK2_NO_1_8_V first.

Regards
Dong Aisheng

> >
> >>There are two possibilities:
> >>
> >>1. Add MMC_CAP_3_3V_DDR and support to core and use that instead of
> >>MMC_CAP_1_8V_DDR.  You'll need Ulf's feedback on that.
> >>
> >
> >Yes, we can do it.
> >But the point is for DDR50/SD3.0/SDIO3.0/HS200/HS400 cards we have
> >the same situation and still need QUIRK2_NO_1_8_V.
> >
> >And i somehow a bit wonder whether we should retrieve the speed mode
> >support from device tree since it's actually controller capability.
> >IO range capability is another thing.
> >
> >Probably we may start another topic to discuss it specificly.
> >
> >>2. Replace ->start_signal_voltage_switch() i.e.
> >>
> >>host->mmc_host_ops.start_signal_voltage_switch =
> >>esdhci_start_signal_voltage_switch;
> >>
> >>You will also need to change all calls to
> >>sdhci_start_signal_voltage_switch() with
> >>host->mmc->ops->start_signal_voltage_switch(), but that needs to be done anyway.
> >>
> >
> >esdhc can fully use the common sdhci_start_signal_voltag_switch.
> >Since it's already support QUIRK_NO_1_8_V, we don't want to
> >invent imx voltage swith currently, but fix and use it first.
> >
> >Regards
> >Dong Aisheng
> >
> >>>
> >>>CC: stable <stable@vger.kernel.org>
> >>>Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> >>>---
> >>>  drivers/mmc/host/sdhci.c | 2 ++
> >>>  1 file changed, 2 insertions(+)
> >>>
> >>>diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> >>>index 2338aab..96ccb15 100644
> >>>--- a/drivers/mmc/host/sdhci.c
> >>>+++ b/drivers/mmc/host/sdhci.c
> >>>@@ -1683,6 +1683,8 @@ static int sdhci_do_signal_voltage_switch(struct sdhci_host *host,
> >>>
> >>>               return -EAGAIN;
> >>>       case MMC_SIGNAL_VOLTAGE_180:
> >>>+             if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V)
> >>>+                     return -EINVAL;
> >>>               /*
> >>>                * Enable 1.8V Signal Enable in the Host Control2
> >>>                * register
> >>>
> >>
diff mbox

Patch

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 2338aab..96ccb15 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1683,6 +1683,8 @@  static int sdhci_do_signal_voltage_switch(struct sdhci_host *host,
 
 		return -EAGAIN;
 	case MMC_SIGNAL_VOLTAGE_180:
+		if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V)
+			return -EINVAL;
 		/*
 		 * Enable 1.8V Signal Enable in the Host Control2
 		 * register