| Message ID | 20200710111104.29616-1-benchuanggli@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support UHS-II for GL9755 | expand |
On 10/07/20 2:11 pm, Ben Chuang wrote: > From: AKASHI Takahiro <takahiro.akashi@linaro.org> > > sdhci_start_signal_voltage_switch() should be called only in UHS-I mode, > and not for UHS-II mode. > > Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > drivers/mmc/host/sdhci.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index 5511649946b9..7f2537648a08 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -2623,8 +2623,13 @@ int sdhci_start_signal_voltage_switch(struct mmc_host *mmc, > /* > * Signal Voltage Switching is only applicable for Host Controllers > * v3.00 and above. > + * But for UHS2, the signal voltage is supplied by vdd2 which is > + * already 1.8v so no voltage switch required. > */ > - if (host->version < SDHCI_SPEC_300) > + if (host->version < SDHCI_SPEC_300 || > + (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > + host->version >= SDHCI_SPEC_400 && > + host->mmc->flags & MMC_UHS2_SUPPORT)) Please look at hooking ->start_signal_voltage_switch() instead > return 0; > > ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2); >
Adrian, On Fri, Aug 21, 2020 at 05:09:01PM +0300, Adrian Hunter wrote: > On 10/07/20 2:11 pm, Ben Chuang wrote: > > From: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > sdhci_start_signal_voltage_switch() should be called only in UHS-I mode, > > and not for UHS-II mode. > > > > Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > drivers/mmc/host/sdhci.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > > index 5511649946b9..7f2537648a08 100644 > > --- a/drivers/mmc/host/sdhci.c > > +++ b/drivers/mmc/host/sdhci.c > > @@ -2623,8 +2623,13 @@ int sdhci_start_signal_voltage_switch(struct mmc_host *mmc, > > /* > > * Signal Voltage Switching is only applicable for Host Controllers > > * v3.00 and above. > > + * But for UHS2, the signal voltage is supplied by vdd2 which is > > + * already 1.8v so no voltage switch required. > > */ > > - if (host->version < SDHCI_SPEC_300) > > + if (host->version < SDHCI_SPEC_300 || > > + (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > + host->version >= SDHCI_SPEC_400 && > > + host->mmc->flags & MMC_UHS2_SUPPORT)) > > Please look at hooking ->start_signal_voltage_switch() instead Do you mean that you want every platform driver who wants to support UHS-II to set NULL to start_signal_voltage_switch hook even if this hack is platform agnostic? (Another option is that it would be put in sdhci_uhs2_add_host() which is responsible to set MMC_UHS2_SUPPORT.) -Takahiro Akashi > > > > return 0; > > > > ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2); > > >
On 14/09/20 9:40 am, AKASHI Takahiro wrote: > Adrian, > > On Fri, Aug 21, 2020 at 05:09:01PM +0300, Adrian Hunter wrote: >> On 10/07/20 2:11 pm, Ben Chuang wrote: >>> From: AKASHI Takahiro <takahiro.akashi@linaro.org> >>> >>> sdhci_start_signal_voltage_switch() should be called only in UHS-I mode, >>> and not for UHS-II mode. >>> >>> Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>> --- >>> drivers/mmc/host/sdhci.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >>> index 5511649946b9..7f2537648a08 100644 >>> --- a/drivers/mmc/host/sdhci.c >>> +++ b/drivers/mmc/host/sdhci.c >>> @@ -2623,8 +2623,13 @@ int sdhci_start_signal_voltage_switch(struct mmc_host *mmc, >>> /* >>> * Signal Voltage Switching is only applicable for Host Controllers >>> * v3.00 and above. >>> + * But for UHS2, the signal voltage is supplied by vdd2 which is >>> + * already 1.8v so no voltage switch required. >>> */ >>> - if (host->version < SDHCI_SPEC_300) >>> + if (host->version < SDHCI_SPEC_300 || >>> + (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && >>> + host->version >= SDHCI_SPEC_400 && >>> + host->mmc->flags & MMC_UHS2_SUPPORT)) >> >> Please look at hooking ->start_signal_voltage_switch() instead > > Do you mean that you want every platform driver who wants to support UHS-II > to set NULL to start_signal_voltage_switch hook even if this hack is > platform agnostic? No, I see UHS-II as a separate layer i.e. UHS-II host controller driver | | | v | sdhci-uhs2 e.g. sdhci_uhs2_start_signal_voltage_switch | | v v sdhci e.g. sdhci_start_signal_voltage_switch Most things should go through sdhci-uhs2 but not nessarily everything.
Ben, Adrian, On Mon, Sep 14, 2020 at 11:08:14AM +0300, Adrian Hunter wrote: > On 14/09/20 9:40 am, AKASHI Takahiro wrote: > > Adrian, > > > > On Fri, Aug 21, 2020 at 05:09:01PM +0300, Adrian Hunter wrote: > >> On 10/07/20 2:11 pm, Ben Chuang wrote: > >>> From: AKASHI Takahiro <takahiro.akashi@linaro.org> > >>> > >>> sdhci_start_signal_voltage_switch() should be called only in UHS-I mode, > >>> and not for UHS-II mode. > >>> > >>> Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw> > >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > >>> --- > >>> drivers/mmc/host/sdhci.c | 7 ++++++- > >>> 1 file changed, 6 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > >>> index 5511649946b9..7f2537648a08 100644 > >>> --- a/drivers/mmc/host/sdhci.c > >>> +++ b/drivers/mmc/host/sdhci.c > >>> @@ -2623,8 +2623,13 @@ int sdhci_start_signal_voltage_switch(struct mmc_host *mmc, > >>> /* > >>> * Signal Voltage Switching is only applicable for Host Controllers > >>> * v3.00 and above. > >>> + * But for UHS2, the signal voltage is supplied by vdd2 which is > >>> + * already 1.8v so no voltage switch required. I have been confused with this comment. (I know it came from the original Intel code, not from Ben.) If this comment is true, > >>> */ > >>> - if (host->version < SDHCI_SPEC_300) > >>> + if (host->version < SDHCI_SPEC_300 || > >>> + (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > >>> + host->version >= SDHCI_SPEC_400 && > >>> + host->mmc->flags & MMC_UHS2_SUPPORT)) the condition above must be wrong since 'flags & MMC_UHS2_SUPPORT' is one of capabilities for a host controller, not a card while the selection of voltage depends on a card type. So I wonder why this code still works. I guess that it is because set_signal_voltage(), or other variant functions, will never be called for UHS-II cards under the current implementation. Looking at mmc_sd_init_card(), we have added some hack: mmc_sd_init_card() { ... /* For UHS2, skip the UHS-I initialization. */ if ((host->flags & MMC_UHS2_SUPPORT) && (host->flags & MMC_UHS2_INITIALIZED)) goto done; ... if (mmc_sd_card_using_v18(card)) { if (mmc_host_set_uhs_voltage(host) || mmc_sd_init_uhs_card(card)) { ... } Ben, can you confirm this? (There is another callsite of mmc_host_set_uhs_voltage() though.) > >> Please look at hooking ->start_signal_voltage_switch() instead > > > > Do you mean that you want every platform driver who wants to support UHS-II > > to set NULL to start_signal_voltage_switch hook even if this hack is > > platform agnostic? > > No, I see UHS-II as a separate layer i.e. > > UHS-II host controller driver > | | > | v > | sdhci-uhs2 e.g. sdhci_uhs2_start_signal_voltage_switch > | | > v v > sdhci e.g. sdhci_start_signal_voltage_switch > > Most things should go through sdhci-uhs2 but not nessarily everything. What I meant by my previous comment is that we don't have to call any function, sdhci_uhs2_start_signal_voltage_switch in above example, for UHS-II cards in any case since it is always simply empty. -Takahiro Akashi
Hi Takahiro, On Tue, Sep 15, 2020 at 2:03 PM AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > Ben, Adrian, > > On Mon, Sep 14, 2020 at 11:08:14AM +0300, Adrian Hunter wrote: > > On 14/09/20 9:40 am, AKASHI Takahiro wrote: > > > Adrian, > > > > > > On Fri, Aug 21, 2020 at 05:09:01PM +0300, Adrian Hunter wrote: > > >> On 10/07/20 2:11 pm, Ben Chuang wrote: > > >>> From: AKASHI Takahiro <takahiro.akashi@linaro.org> > > >>> > > >>> sdhci_start_signal_voltage_switch() should be called only in UHS-I mode, > > >>> and not for UHS-II mode. > > >>> > > >>> Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw> > > >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > >>> --- > > >>> drivers/mmc/host/sdhci.c | 7 ++++++- > > >>> 1 file changed, 6 insertions(+), 1 deletion(-) > > >>> > > >>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > > >>> index 5511649946b9..7f2537648a08 100644 > > >>> --- a/drivers/mmc/host/sdhci.c > > >>> +++ b/drivers/mmc/host/sdhci.c > > >>> @@ -2623,8 +2623,13 @@ int sdhci_start_signal_voltage_switch(struct mmc_host *mmc, > > >>> /* > > >>> * Signal Voltage Switching is only applicable for Host Controllers > > >>> * v3.00 and above. > > >>> + * But for UHS2, the signal voltage is supplied by vdd2 which is > > >>> + * already 1.8v so no voltage switch required. > > I have been confused with this comment. > (I know it came from the original Intel code, not from Ben.) > > If this comment is true, > > > >>> */ > > >>> - if (host->version < SDHCI_SPEC_300) > > >>> + if (host->version < SDHCI_SPEC_300 || > > >>> + (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > >>> + host->version >= SDHCI_SPEC_400 && > > >>> + host->mmc->flags & MMC_UHS2_SUPPORT)) > > the condition above must be wrong since 'flags & MMC_UHS2_SUPPORT' > is one of capabilities for a host controller, not a card > while the selection of voltage depends on a card type. The flag MMC_UHS2_SUPPORT is set at the beginning of mmc_uhs2_rescan_try_freq(). In UHS-II flow, it stays set. If the attempt to UHS-II fails finally, it will be unset. > > So I wonder why this code still works. > I guess that it is because set_signal_voltage(), or other variant functions, > will never be called for UHS-II cards under the current implementation. > > Looking at mmc_sd_init_card(), we have added some hack: > mmc_sd_init_card() > { > ... > /* For UHS2, skip the UHS-I initialization. */ > if ((host->flags & MMC_UHS2_SUPPORT) && > (host->flags & MMC_UHS2_INITIALIZED)) > goto done; > ... > if (mmc_sd_card_using_v18(card)) { > if (mmc_host_set_uhs_voltage(host) || > mmc_sd_init_uhs_card(card)) { > ... > } > > Ben, can you confirm this? > (There is another callsite of mmc_host_set_uhs_voltage() though.) UHS-II cards use differential signals and don't need to signal voltage switch. But the main task is to set the parameters of UHS-II card interface. > > > >> Please look at hooking ->start_signal_voltage_switch() instead > > > > > > Do you mean that you want every platform driver who wants to support UHS-II > > > to set NULL to start_signal_voltage_switch hook even if this hack is > > > platform agnostic? > > > > No, I see UHS-II as a separate layer i.e. > > > > UHS-II host controller driver > > | | > > | v > > | sdhci-uhs2 e.g. sdhci_uhs2_start_signal_voltage_switch > > | | > > v v > > sdhci e.g. sdhci_start_signal_voltage_switch > > > > Most things should go through sdhci-uhs2 but not nessarily everything. > > What I meant by my previous comment is that we don't have to > call any function, sdhci_uhs2_start_signal_voltage_switch in above example, > for UHS-II cards in any case since it is always simply empty. > > -Takahiro Akashi
On Tue, Sep 15, 2020 at 07:36:14PM +0800, Ben Chuang wrote: > Hi Takahiro, > > On Tue, Sep 15, 2020 at 2:03 PM AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: > > > > Ben, Adrian, > > > > On Mon, Sep 14, 2020 at 11:08:14AM +0300, Adrian Hunter wrote: > > > On 14/09/20 9:40 am, AKASHI Takahiro wrote: > > > > Adrian, > > > > > > > > On Fri, Aug 21, 2020 at 05:09:01PM +0300, Adrian Hunter wrote: > > > >> On 10/07/20 2:11 pm, Ben Chuang wrote: > > > >>> From: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > >>> > > > >>> sdhci_start_signal_voltage_switch() should be called only in UHS-I mode, > > > >>> and not for UHS-II mode. > > > >>> > > > >>> Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw> > > > >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > >>> --- > > > >>> drivers/mmc/host/sdhci.c | 7 ++++++- > > > >>> 1 file changed, 6 insertions(+), 1 deletion(-) > > > >>> > > > >>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > > > >>> index 5511649946b9..7f2537648a08 100644 > > > >>> --- a/drivers/mmc/host/sdhci.c > > > >>> +++ b/drivers/mmc/host/sdhci.c > > > >>> @@ -2623,8 +2623,13 @@ int sdhci_start_signal_voltage_switch(struct mmc_host *mmc, > > > >>> /* > > > >>> * Signal Voltage Switching is only applicable for Host Controllers > > > >>> * v3.00 and above. > > > >>> + * But for UHS2, the signal voltage is supplied by vdd2 which is > > > >>> + * already 1.8v so no voltage switch required. > > > > I have been confused with this comment. > > (I know it came from the original Intel code, not from Ben.) > > > > If this comment is true, > > > > > >>> */ > > > >>> - if (host->version < SDHCI_SPEC_300) > > > >>> + if (host->version < SDHCI_SPEC_300 || > > > >>> + (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > > >>> + host->version >= SDHCI_SPEC_400 && > > > >>> + host->mmc->flags & MMC_UHS2_SUPPORT)) > > > > the condition above must be wrong since 'flags & MMC_UHS2_SUPPORT' > > is one of capabilities for a host controller, not a card > > while the selection of voltage depends on a card type. > > The flag MMC_UHS2_SUPPORT is set at the beginning of mmc_uhs2_rescan_try_freq(). > In UHS-II flow, it stays set. > If the attempt to UHS-II fails finally, it will be unset. Right, but MMC_UHS2_SUPPORT is also set, at least initially, in sdhci_uhs2_add_host(). It is confusing, isn't it? As we discussed before, any card-specific properties, like UHS-II mode, should be placed in a card structure, not a host structure. > > > > So I wonder why this code still works. > > I guess that it is because set_signal_voltage(), or other variant functions, > > will never be called for UHS-II cards under the current implementation. > > > > Looking at mmc_sd_init_card(), we have added some hack: > > mmc_sd_init_card() > > { > > ... > > /* For UHS2, skip the UHS-I initialization. */ > > if ((host->flags & MMC_UHS2_SUPPORT) && > > (host->flags & MMC_UHS2_INITIALIZED)) > > goto done; > > ... > > if (mmc_sd_card_using_v18(card)) { > > if (mmc_host_set_uhs_voltage(host) || > > mmc_sd_init_uhs_card(card)) { > > ... > > } > > > > Ben, can you confirm this? > > (There is another callsite of mmc_host_set_uhs_voltage() though.) > > UHS-II cards use differential signals and don't need to signal voltage switch. > But the main task is to set the parameters of UHS-II card interface. Whoever sets MMC_UHS2_SUPPORT (and MMC_UHS2_INITIALIZED), my assertion above (mmc_host_set_uhs_voltage, and hence [sdhci_]start_signal_voltage_switch(), is never called for UHS-II cards) will be valid, isn't it? -Takahiro Akashi > > > > > >> Please look at hooking ->start_signal_voltage_switch() instead > > > > > > > > Do you mean that you want every platform driver who wants to support UHS-II > > > > to set NULL to start_signal_voltage_switch hook even if this hack is > > > > platform agnostic? > > > > > > No, I see UHS-II as a separate layer i.e. > > > > > > UHS-II host controller driver > > > | | > > > | v > > > | sdhci-uhs2 e.g. sdhci_uhs2_start_signal_voltage_switch > > > | | > > > v v > > > sdhci e.g. sdhci_start_signal_voltage_switch > > > > > > Most things should go through sdhci-uhs2 but not nessarily everything. > > > > What I meant by my previous comment is that we don't have to > > call any function, sdhci_uhs2_start_signal_voltage_switch in above example, > > for UHS-II cards in any case since it is always simply empty. > > > > -Takahiro Akashi
On Wed, Sep 16, 2020 at 09:01:20AM +0300, Adrian Hunter wrote: > On 15/09/20 9:03 am, AKASHI Takahiro wrote: > > Ben, Adrian, > > > > On Mon, Sep 14, 2020 at 11:08:14AM +0300, Adrian Hunter wrote: > >> On 14/09/20 9:40 am, AKASHI Takahiro wrote: > >>> Adrian, > >>> > >>> On Fri, Aug 21, 2020 at 05:09:01PM +0300, Adrian Hunter wrote: > >>>> On 10/07/20 2:11 pm, Ben Chuang wrote: > >>>>> From: AKASHI Takahiro <takahiro.akashi@linaro.org> > >>>>> > >>>>> sdhci_start_signal_voltage_switch() should be called only in UHS-I mode, > >>>>> and not for UHS-II mode. > >>>>> > >>>>> Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw> > >>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > >>>>> --- > >>>>> drivers/mmc/host/sdhci.c | 7 ++++++- > >>>>> 1 file changed, 6 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > >>>>> index 5511649946b9..7f2537648a08 100644 > >>>>> --- a/drivers/mmc/host/sdhci.c > >>>>> +++ b/drivers/mmc/host/sdhci.c > >>>>> @@ -2623,8 +2623,13 @@ int sdhci_start_signal_voltage_switch(struct mmc_host *mmc, > >>>>> /* > >>>>> * Signal Voltage Switching is only applicable for Host Controllers > >>>>> * v3.00 and above. > >>>>> + * But for UHS2, the signal voltage is supplied by vdd2 which is > >>>>> + * already 1.8v so no voltage switch required. > >>>>> */ > >>>>> - if (host->version < SDHCI_SPEC_300) > >>>>> + if (host->version < SDHCI_SPEC_300 || > >>>>> + (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > >>>>> + host->version >= SDHCI_SPEC_400 && > >>>>> + host->mmc->flags & MMC_UHS2_SUPPORT)) > >>>> Please look at hooking ->start_signal_voltage_switch() instead > >>> > >>> Do you mean that you want every platform driver who wants to support UHS-II > >>> to set NULL to start_signal_voltage_switch hook even if this hack is > >>> platform agnostic? > >> > >> No, I see UHS-II as a separate layer i.e. > >> > >> UHS-II host controller driver > >> | | > >> | v > >> | sdhci-uhs2 e.g. sdhci_uhs2_start_signal_voltage_switch > >> | | > >> v v > >> sdhci e.g. sdhci_start_signal_voltage_switch > >> > >> Most things should go through sdhci-uhs2 but not nessarily everything. > > > > What I meant by my previous comment is that we don't have to > > call any function, sdhci_uhs2_start_signal_voltage_switch in above example, > > for UHS-II cards in any case since it is always simply empty. > > Please treat the sdhci_uhs2_... host ops as functions for a UHS-II host > controller in either UHS-II or legacy mode. i.e. it is up to sdhci-uhs2.c to > call through to sdhci.c not the other way around. e.g. > > int sdhci_uhs2_start_signal_voltage_switch(blah) > { > if (sdhci_uhs2_mode(host)) > return 0; > return sdhci_start_signal_valtage_switch(blah); > } Okay, the remaining issue is to clarify the meaning of MMC_UHS2_SUPPORT. -Takhiro Akashi
On Wed, Sep 16, 2020 at 8:52 AM AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > On Tue, Sep 15, 2020 at 07:36:14PM +0800, Ben Chuang wrote: > > Hi Takahiro, > > > > On Tue, Sep 15, 2020 at 2:03 PM AKASHI Takahiro > > <takahiro.akashi@linaro.org> wrote: > > > > > > Ben, Adrian, > > > > > > On Mon, Sep 14, 2020 at 11:08:14AM +0300, Adrian Hunter wrote: > > > > On 14/09/20 9:40 am, AKASHI Takahiro wrote: > > > > > Adrian, > > > > > > > > > > On Fri, Aug 21, 2020 at 05:09:01PM +0300, Adrian Hunter wrote: > > > > >> On 10/07/20 2:11 pm, Ben Chuang wrote: > > > > >>> From: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > >>> > > > > >>> sdhci_start_signal_voltage_switch() should be called only in UHS-I mode, > > > > >>> and not for UHS-II mode. > > > > >>> > > > > >>> Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw> > > > > >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > >>> --- > > > > >>> drivers/mmc/host/sdhci.c | 7 ++++++- > > > > >>> 1 file changed, 6 insertions(+), 1 deletion(-) > > > > >>> > > > > >>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > > > > >>> index 5511649946b9..7f2537648a08 100644 > > > > >>> --- a/drivers/mmc/host/sdhci.c > > > > >>> +++ b/drivers/mmc/host/sdhci.c > > > > >>> @@ -2623,8 +2623,13 @@ int sdhci_start_signal_voltage_switch(struct mmc_host *mmc, > > > > >>> /* > > > > >>> * Signal Voltage Switching is only applicable for Host Controllers > > > > >>> * v3.00 and above. > > > > >>> + * But for UHS2, the signal voltage is supplied by vdd2 which is > > > > >>> + * already 1.8v so no voltage switch required. > > > > > > I have been confused with this comment. > > > (I know it came from the original Intel code, not from Ben.) > > > > > > If this comment is true, > > > > > > > >>> */ > > > > >>> - if (host->version < SDHCI_SPEC_300) > > > > >>> + if (host->version < SDHCI_SPEC_300 || > > > > >>> + (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > > > >>> + host->version >= SDHCI_SPEC_400 && > > > > >>> + host->mmc->flags & MMC_UHS2_SUPPORT)) > > > > > > the condition above must be wrong since 'flags & MMC_UHS2_SUPPORT' > > > is one of capabilities for a host controller, not a card > > > while the selection of voltage depends on a card type. > > > > The flag MMC_UHS2_SUPPORT is set at the beginning of mmc_uhs2_rescan_try_freq(). > > In UHS-II flow, it stays set. > > If the attempt to UHS-II fails finally, it will be unset. > > Right, but MMC_UHS2_SUPPORT is also set, at least initially, > in sdhci_uhs2_add_host(). It is confusing, isn't it? I think it can be removed from sdhci_uhs2_add_host() to avoid making confusion. > > As we discussed before, any card-specific properties, like UHS-II mode, > should be placed in a card structure, not a host structure. > > > > > > > So I wonder why this code still works. > > > I guess that it is because set_signal_voltage(), or other variant functions, > > > will never be called for UHS-II cards under the current implementation. > > > > > > Looking at mmc_sd_init_card(), we have added some hack: > > > mmc_sd_init_card() > > > { > > > ... > > > /* For UHS2, skip the UHS-I initialization. */ > > > if ((host->flags & MMC_UHS2_SUPPORT) && > > > (host->flags & MMC_UHS2_INITIALIZED)) > > > goto done; > > > ... > > > if (mmc_sd_card_using_v18(card)) { > > > if (mmc_host_set_uhs_voltage(host) || > > > mmc_sd_init_uhs_card(card)) { > > > ... > > > } > > > > > > Ben, can you confirm this? > > > (There is another callsite of mmc_host_set_uhs_voltage() though.) > > > > UHS-II cards use differential signals and don't need to signal voltage switch. > > But the main task is to set the parameters of UHS-II card interface. > > Whoever sets MMC_UHS2_SUPPORT (and MMC_UHS2_INITIALIZED), my assertion above > (mmc_host_set_uhs_voltage, and hence [sdhci_]start_signal_voltage_switch(), is > never called for UHS-II cards) will be valid, isn't it? > > -Takahiro Akashi > > > > > > > > >> Please look at hooking ->start_signal_voltage_switch() instead > > > > > > > > > > Do you mean that you want every platform driver who wants to support UHS-II > > > > > to set NULL to start_signal_voltage_switch hook even if this hack is > > > > > platform agnostic? > > > > > > > > No, I see UHS-II as a separate layer i.e. > > > > > > > > UHS-II host controller driver > > > > | | > > > > | v > > > > | sdhci-uhs2 e.g. sdhci_uhs2_start_signal_voltage_switch > > > > | | > > > > v v > > > > sdhci e.g. sdhci_start_signal_voltage_switch > > > > > > > > Most things should go through sdhci-uhs2 but not nessarily everything. > > > > > > What I meant by my previous comment is that we don't have to > > > call any function, sdhci_uhs2_start_signal_voltage_switch in above example, > > > for UHS-II cards in any case since it is always simply empty. > > > > > > -Takahiro Akashi
Ben, On Wed, Sep 16, 2020 at 05:42:07PM +0800, Ben Chuang wrote: > On Wed, Sep 16, 2020 at 8:52 AM AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: > > > > On Tue, Sep 15, 2020 at 07:36:14PM +0800, Ben Chuang wrote: > > > Hi Takahiro, > > > > > > On Tue, Sep 15, 2020 at 2:03 PM AKASHI Takahiro > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > Ben, Adrian, > > > > > > > > On Mon, Sep 14, 2020 at 11:08:14AM +0300, Adrian Hunter wrote: > > > > > On 14/09/20 9:40 am, AKASHI Takahiro wrote: > > > > > > Adrian, > > > > > > > > > > > > On Fri, Aug 21, 2020 at 05:09:01PM +0300, Adrian Hunter wrote: > > > > > >> On 10/07/20 2:11 pm, Ben Chuang wrote: > > > > > >>> From: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > >>> > > > > > >>> sdhci_start_signal_voltage_switch() should be called only in UHS-I mode, > > > > > >>> and not for UHS-II mode. > > > > > >>> > > > > > >>> Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw> > > > > > >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > >>> --- > > > > > >>> drivers/mmc/host/sdhci.c | 7 ++++++- > > > > > >>> 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > >>> > > > > > >>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > > > > > >>> index 5511649946b9..7f2537648a08 100644 > > > > > >>> --- a/drivers/mmc/host/sdhci.c > > > > > >>> +++ b/drivers/mmc/host/sdhci.c > > > > > >>> @@ -2623,8 +2623,13 @@ int sdhci_start_signal_voltage_switch(struct mmc_host *mmc, > > > > > >>> /* > > > > > >>> * Signal Voltage Switching is only applicable for Host Controllers > > > > > >>> * v3.00 and above. > > > > > >>> + * But for UHS2, the signal voltage is supplied by vdd2 which is > > > > > >>> + * already 1.8v so no voltage switch required. > > > > > > > > I have been confused with this comment. > > > > (I know it came from the original Intel code, not from Ben.) > > > > > > > > If this comment is true, > > > > > > > > > >>> */ > > > > > >>> - if (host->version < SDHCI_SPEC_300) > > > > > >>> + if (host->version < SDHCI_SPEC_300 || > > > > > >>> + (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > > > > >>> + host->version >= SDHCI_SPEC_400 && > > > > > >>> + host->mmc->flags & MMC_UHS2_SUPPORT)) > > > > > > > > the condition above must be wrong since 'flags & MMC_UHS2_SUPPORT' > > > > is one of capabilities for a host controller, not a card > > > > while the selection of voltage depends on a card type. > > > > > > The flag MMC_UHS2_SUPPORT is set at the beginning of mmc_uhs2_rescan_try_freq(). > > > In UHS-II flow, it stays set. > > > If the attempt to UHS-II fails finally, it will be unset. > > > > Right, but MMC_UHS2_SUPPORT is also set, at least initially, > > in sdhci_uhs2_add_host(). It is confusing, isn't it? > > I think it can be removed from sdhci_uhs2_add_host() to avoid making confusion. Okay, > > > > As we discussed before, any card-specific properties, like UHS-II mode, > > should be placed in a card structure, not a host structure. Do you have any idea on this? I remember that Ulf also made a similar comment on the "core" side. -Takahiro Akashi > > > > > > > > > > So I wonder why this code still works. > > > > I guess that it is because set_signal_voltage(), or other variant functions, > > > > will never be called for UHS-II cards under the current implementation. > > > > > > > > Looking at mmc_sd_init_card(), we have added some hack: > > > > mmc_sd_init_card() > > > > { > > > > ... > > > > /* For UHS2, skip the UHS-I initialization. */ > > > > if ((host->flags & MMC_UHS2_SUPPORT) && > > > > (host->flags & MMC_UHS2_INITIALIZED)) > > > > goto done; > > > > ... > > > > if (mmc_sd_card_using_v18(card)) { > > > > if (mmc_host_set_uhs_voltage(host) || > > > > mmc_sd_init_uhs_card(card)) { > > > > ... > > > > } > > > > > > > > Ben, can you confirm this? > > > > (There is another callsite of mmc_host_set_uhs_voltage() though.) > > > > > > UHS-II cards use differential signals and don't need to signal voltage switch. > > > But the main task is to set the parameters of UHS-II card interface. > > > > Whoever sets MMC_UHS2_SUPPORT (and MMC_UHS2_INITIALIZED), my assertion above > > (mmc_host_set_uhs_voltage, and hence [sdhci_]start_signal_voltage_switch(), is > > never called for UHS-II cards) will be valid, isn't it? > > > > -Takahiro Akashi > > > > > > > > > > > >> Please look at hooking ->start_signal_voltage_switch() instead > > > > > > > > > > > > Do you mean that you want every platform driver who wants to support UHS-II > > > > > > to set NULL to start_signal_voltage_switch hook even if this hack is > > > > > > platform agnostic? > > > > > > > > > > No, I see UHS-II as a separate layer i.e. > > > > > > > > > > UHS-II host controller driver > > > > > | | > > > > > | v > > > > > | sdhci-uhs2 e.g. sdhci_uhs2_start_signal_voltage_switch > > > > > | | > > > > > v v > > > > > sdhci e.g. sdhci_start_signal_voltage_switch > > > > > > > > > > Most things should go through sdhci-uhs2 but not nessarily everything. > > > > > > > > What I meant by my previous comment is that we don't have to > > > > call any function, sdhci_uhs2_start_signal_voltage_switch in above example, > > > > for UHS-II cards in any case since it is always simply empty. > > > > > > > > -Takahiro Akashi
On Thu, Sep 17, 2020 at 8:56 AM AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > Ben, > > On Wed, Sep 16, 2020 at 05:42:07PM +0800, Ben Chuang wrote: > > On Wed, Sep 16, 2020 at 8:52 AM AKASHI Takahiro > > <takahiro.akashi@linaro.org> wrote: > > > > > > On Tue, Sep 15, 2020 at 07:36:14PM +0800, Ben Chuang wrote: > > > > Hi Takahiro, > > > > > > > > On Tue, Sep 15, 2020 at 2:03 PM AKASHI Takahiro > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > Ben, Adrian, > > > > > > > > > > On Mon, Sep 14, 2020 at 11:08:14AM +0300, Adrian Hunter wrote: > > > > > > On 14/09/20 9:40 am, AKASHI Takahiro wrote: > > > > > > > Adrian, > > > > > > > > > > > > > > On Fri, Aug 21, 2020 at 05:09:01PM +0300, Adrian Hunter wrote: > > > > > > >> On 10/07/20 2:11 pm, Ben Chuang wrote: > > > > > > >>> From: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > > >>> > > > > > > >>> sdhci_start_signal_voltage_switch() should be called only in UHS-I mode, > > > > > > >>> and not for UHS-II mode. > > > > > > >>> > > > > > > >>> Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw> > > > > > > >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > > >>> --- > > > > > > >>> drivers/mmc/host/sdhci.c | 7 ++++++- > > > > > > >>> 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > >>> > > > > > > >>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > > > > > > >>> index 5511649946b9..7f2537648a08 100644 > > > > > > >>> --- a/drivers/mmc/host/sdhci.c > > > > > > >>> +++ b/drivers/mmc/host/sdhci.c > > > > > > >>> @@ -2623,8 +2623,13 @@ int sdhci_start_signal_voltage_switch(struct mmc_host *mmc, > > > > > > >>> /* > > > > > > >>> * Signal Voltage Switching is only applicable for Host Controllers > > > > > > >>> * v3.00 and above. > > > > > > >>> + * But for UHS2, the signal voltage is supplied by vdd2 which is > > > > > > >>> + * already 1.8v so no voltage switch required. > > > > > > > > > > I have been confused with this comment. > > > > > (I know it came from the original Intel code, not from Ben.) > > > > > > > > > > If this comment is true, > > > > > > > > > > > >>> */ > > > > > > >>> - if (host->version < SDHCI_SPEC_300) > > > > > > >>> + if (host->version < SDHCI_SPEC_300 || > > > > > > >>> + (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > > > > > >>> + host->version >= SDHCI_SPEC_400 && > > > > > > >>> + host->mmc->flags & MMC_UHS2_SUPPORT)) > > > > > > > > > > the condition above must be wrong since 'flags & MMC_UHS2_SUPPORT' > > > > > is one of capabilities for a host controller, not a card > > > > > while the selection of voltage depends on a card type. > > > > > > > > The flag MMC_UHS2_SUPPORT is set at the beginning of mmc_uhs2_rescan_try_freq(). > > > > In UHS-II flow, it stays set. > > > > If the attempt to UHS-II fails finally, it will be unset. > > > > > > Right, but MMC_UHS2_SUPPORT is also set, at least initially, > > > in sdhci_uhs2_add_host(). It is confusing, isn't it? > > > > I think it can be removed from sdhci_uhs2_add_host() to avoid making confusion. > > Okay, > > > > > > > As we discussed before, any card-specific properties, like UHS-II mode, > > > should be placed in a card structure, not a host structure. > > Do you have any idea on this? > I remember that Ulf also made a similar comment on the "core" side. We may list those properties to do first. i.e. MMC_UHS2_INITIALIZED. > > -Takahiro Akashi > > > > > > > > > > > > > > So I wonder why this code still works. > > > > > I guess that it is because set_signal_voltage(), or other variant functions, > > > > > will never be called for UHS-II cards under the current implementation. > > > > > > > > > > Looking at mmc_sd_init_card(), we have added some hack: > > > > > mmc_sd_init_card() > > > > > { > > > > > ... > > > > > /* For UHS2, skip the UHS-I initialization. */ > > > > > if ((host->flags & MMC_UHS2_SUPPORT) && > > > > > (host->flags & MMC_UHS2_INITIALIZED)) > > > > > goto done; > > > > > ... > > > > > if (mmc_sd_card_using_v18(card)) { > > > > > if (mmc_host_set_uhs_voltage(host) || > > > > > mmc_sd_init_uhs_card(card)) { > > > > > ... > > > > > } > > > > > > > > > > Ben, can you confirm this? > > > > > (There is another callsite of mmc_host_set_uhs_voltage() though.) > > > > > > > > UHS-II cards use differential signals and don't need to signal voltage switch. > > > > But the main task is to set the parameters of UHS-II card interface. > > > > > > Whoever sets MMC_UHS2_SUPPORT (and MMC_UHS2_INITIALIZED), my assertion above > > > (mmc_host_set_uhs_voltage, and hence [sdhci_]start_signal_voltage_switch(), is > > > never called for UHS-II cards) will be valid, isn't it? > > > > > > -Takahiro Akashi > > > > > > > > > > > > > > >> Please look at hooking ->start_signal_voltage_switch() instead > > > > > > > > > > > > > > Do you mean that you want every platform driver who wants to support UHS-II > > > > > > > to set NULL to start_signal_voltage_switch hook even if this hack is > > > > > > > platform agnostic? > > > > > > > > > > > > No, I see UHS-II as a separate layer i.e. > > > > > > > > > > > > UHS-II host controller driver > > > > > > | | > > > > > > | v > > > > > > | sdhci-uhs2 e.g. sdhci_uhs2_start_signal_voltage_switch > > > > > > | | > > > > > > v v > > > > > > sdhci e.g. sdhci_start_signal_voltage_switch > > > > > > > > > > > > Most things should go through sdhci-uhs2 but not nessarily everything. > > > > > > > > > > What I meant by my previous comment is that we don't have to > > > > > call any function, sdhci_uhs2_start_signal_voltage_switch in above example, > > > > > for UHS-II cards in any case since it is always simply empty. > > > > > > > > > > -Takahiro Akashi
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 5511649946b9..7f2537648a08 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -2623,8 +2623,13 @@ int sdhci_start_signal_voltage_switch(struct mmc_host *mmc, /* * Signal Voltage Switching is only applicable for Host Controllers * v3.00 and above. + * But for UHS2, the signal voltage is supplied by vdd2 which is + * already 1.8v so no voltage switch required. */ - if (host->version < SDHCI_SPEC_300) + if (host->version < SDHCI_SPEC_300 || + (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && + host->version >= SDHCI_SPEC_400 && + host->mmc->flags & MMC_UHS2_SUPPORT)) return 0; ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);