| Message ID | 1460741387-23815-8-git-send-email-aisheng.dong@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 >> >
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 >>> >>
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 --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
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(+)