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

[Dong Aisheng <dongas86@gmail.com>]

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
> >>>
> >>