Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
On 24/09/20 12:35 pm, AKASHI Takahiro wrote: > Adrian, > > This is, hopefully, my last reply to your comments on this patch#12. > > Regarding _request() and _send_command() (and more), > > On Fri, Aug 21, 2020 at 05:08:32PM +0300, Adrian Hunter wrote: >> On 10/07/20 2:10 pm, Ben Chuang wrote: >>> From: Ben Chuang >>> >>> In this commit, UHS-II related operations will be called via a function >>> pointer array, sdhci_uhs2_ops, in order to make UHS-II support as >>> a kernel module. >>> This array will be initialized only if CONFIG_MMC_SDHCI_UHS2 is enabled >>> and when the UHS-II module is loaded. Otherwise, all the functions >>> stay void. >>> > (snip) > >> Again, this is what I want to avoid. I would like to have 3 kinds of >> functions: >> - SD mode only >> - UHS-II only >> - SD functions with no UHS-II code, that can also be used by UHS-II >> i.e. I don't want to mix UHS-II code and SD mode code in the same function. >> >> I think sdhci-uhs2.c should provide a request function and a send_command >> function. >> I would start by removing everything you may not need, and then see if you >> have any problems. >> e.g. >> >> void uhs2_request(struct mmc_host *mmc, struct mmc_request *mrq) >> { >> struct sdhci_host *host = mmc_priv(mmc); >> struct mmc_command *cmd; >> unsigned long flags; >> >> if (!host->uhs2_mode) { >> sdhci_request(mmc, mrq); >> return; >> } >> >> spin_lock_irqsave(>lock, flags); >> uhs2_send_command(host, cmd); >> spin_unlock_irqrestore(>lock, flags); >> } >> EXPORT_SYMBOL_GPL(uhs2_request); >> >> For sdhci_prepare_data(), I would factor out the dma part, so >> >> static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command >> *cmd) >> { >> struct mmc_data *data = cmd->data; >> >> sdhci_initialize_data(host, data); >> >> sdhci_prepare_dma(host, data); >> >> sdhci_set_block_info(host, data); >> } >> >> The you could export sdhci_initialize_data() and sdhci_prepare_dma() for >> uhs2. >> >>> } >>> >>> #if IS_ENABLED(CONFIG_MMC_SDHCI_EXTERNAL_DMA) >>> @@ -1439,6 +1463,13 @@ static void sdhci_set_transfer_mode(struct >>> sdhci_host *host, >>> u16 mode = 0; >>> struct mmc_data *data = cmd->data; >>> >>> + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && >>> + host->mmc->flags & MMC_UHS2_SUPPORT) { >>> + if (sdhci_uhs2_ops.set_transfer_mode) >>> + sdhci_uhs2_ops.set_transfer_mode(host, cmd); >>> + return; >>> + } >>> + >> >> Once you provide uhs2_request() and uhs2_send_command(), the transfer mode >> setting can be done in sdhci-uhs2.c > > If I try to make changes as you suggested above, a lot of other uhs2-flavored > functions will also be created due to calling dependency/sequences > and for "completeness" compared to uhs counterparts. > They probably include > sdhci_uhs2_prepare_data() > sdhci_uhs2_external_dma_prepare_data() > sdhci_uhs2_send_command() > sdhci_uhs2_send_command_try() > sdhci_uhs2_send_tuning() > sdhci_uhs2_request() > sdhci_uhs2_request_atomic() > sdhci_uhs2_thread_irq() > sdhci_uhs2_irq() > sdhci_uhs2_cmd_irq() > sdhci_uhs2_finish_command() > sdhci_uhs2_resume_host() > __sdhci_uhs2_add_host() > sdhci_uhs2_add_host() > (Some may not be used under the current drivers.) > > In addition, a bunch of functions in sdhci.c will also have to be exported > to uhs2 as "global" functions instead of "static." > > Is this all that you expect to see? Yes. Add what you need.
Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
Takahiro, On Thu, Sep 24, 2020 at 5:46 PM AKASHI Takahiro wrote: > > Ben, > > On Fri, Sep 18, 2020 at 06:27:01PM +0800, Ben Chuang wrote: > > On Fri, Sep 18, 2020 at 9:15 AM AKASHI Takahiro > > wrote: > > > > > > Ben, > > > > > > On Thu, Sep 17, 2020 at 06:12:27PM +0800, Ben Chuang wrote: > > > > Hi Takahiro, > > > > > > > > On Thu, Sep 17, 2020 at 1:12 PM AKASHI Takahiro > > > > wrote: > > > > > > > > > > Adrian, Ben, > > > > > > > > > > Regarding _reset() function, > > > > > > > > > > On Fri, Aug 21, 2020 at 05:08:32PM +0300, Adrian Hunter wrote: > > > > > > On 10/07/20 2:10 pm, Ben Chuang wrote: > > > > > > > From: Ben Chuang > > > > > > > > > > > > > > In this commit, UHS-II related operations will be called via a > > > > > > > function > > > > > > > pointer array, sdhci_uhs2_ops, in order to make UHS-II support as > > > > > > > a kernel module. > > > > > > > This array will be initialized only if CONFIG_MMC_SDHCI_UHS2 is > > > > > > > enabled > > > > > > > and when the UHS-II module is loaded. Otherwise, all the functions > > > > > > > stay void. > > > > > > > > > > > > > > Signed-off-by: Ben Chuang > > > > > > > Signed-off-by: AKASHI Takahiro > > > > > > > --- > > > > > > > drivers/mmc/host/sdhci.c | 152 > > > > > > > ++- > > > > > > > 1 file changed, 136 insertions(+), 16 deletions(-) > > > > > > > > > > > > > > > > > (snip) > > > > > > > > > > > > if (host->ops->platform_send_init_74_clocks) > > > > > > > host->ops->platform_send_init_74_clocks(host, > > > > > > > ios->power_mode); > > > > > > > > > > > > > > @@ -2331,7 +2411,7 @@ void sdhci_set_ios(struct mmc_host *mmc, > > > > > > > struct mmc_ios *ios) > > > > > > > } > > > > > > > > > > > > > > if (host->version >= SDHCI_SPEC_300) { > > > > > > > - u16 clk, ctrl_2; > > > > > > > + u16 clk; > > > > > > > > > > > > > > if (!host->preset_enabled) { > > > > > > > sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL); > > > > > > > @@ -3173,11 +3253,19 @@ static bool sdhci_request_done(struct > > > > > > > sdhci_host *host) > > > > > > > /* This is to force an update */ > > > > > > > host->ops->set_clock(host, host->clock); > > > > > > > > > > > > > > - /* Spec says we should do both at the same time, but > > > > > > > Ricoh > > > > > > > - controllers do not like that. */ > > > > > > > - sdhci_do_reset(host, SDHCI_RESET_CMD); > > > > > > > - sdhci_do_reset(host, SDHCI_RESET_DATA); > > > > > > > - > > > > > > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > > > > > > + host->mmc->flags & MMC_UHS2_INITIALIZED) { > > > > > > > + if (sdhci_uhs2_ops.reset) > > > > > > > + sdhci_uhs2_ops.reset(host, > > > > > > > + > > > > > > > SDHCI_UHS2_SW_RESET_SD); > > > > > > > + } else { > > > > > > > + /* > > > > > > > +* Spec says we should do both at the same > > > > > > > time, but > > > > > > > +* Ricoh controllers do not like that. > > > > > > > +*/ > > > > > > > + sdhci_do_reset(host, SDHCI_RESET_CMD); > > > > > > > + sdhci_do_reset(host, SDHCI_RESET_DATA); > > > > > > > + } > > > > > > > > > > > > Please look at using the existing ->reset() sdhci host op instead. > > > > > > > > > > Well, the second argument to those reset functions is a bit-wise value > > > > > to different "reset" registers, SDHCI_SOFTWARE_RESET and > > > > > SDHCI_UHS2_SW_RESET, > > > > > respectively. > > > > > > > > > > This fact raises a couple of questions to me: > > > > > > > > > > 1) Does it make sense to merge two functionality into one, i.e. > > > > >sdhci_do_reset(), which is set to call ->reset hook? > > > > > > > > > > -> Adrian > > > > > > > > > > 2) UHS2_SW_RESET_SD is done only at this place while there are many > > > > > callsites > > > > >of reset(RESET_CMD|RESET_DATA) in sdhci.c. > > > > >Why does the current code work? > > > > > > > > > >I found, in sdhci-pci-gli.c, > > > > >sdhci_gl9755_reset() > > > > > /* reset sd-tran on UHS2 mode if need to reset cmd/data */ > > > > > if ((mask & SDHCI_RESET_CMD) | (mask & SDHCI_RESET_DATA)) > > > > > gl9755_uhs2_reset_sd_tran(host); > > > > > > (A) > > > > > > > > > > > > >Is this the trick to avoid the issue? > > > > >(It looks redundant in terms of the hack above in > > > > > sdhci_request_done() > > > > >and even quite dirty to me. Moreover, no corresponding code for > > > > > gl9750 > > > > >and gl9763.) > > > > > > > > GL9755 currently does SD reset and UHS-II reset together. > > > > > > Do you mean that, in UHS-II operations, you need only the reset on > > > SDHCI_UHS2_SW_RESET register? > > > >
Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
Takahiro, On Thu, Sep 24, 2020 at 5:57 PM AKASHI Takahiro wrote: > > Ben, > > On Fri, Sep 18, 2020 at 06:50:24PM +0800, Ben Chuang wrote: > > On Fri, Sep 18, 2020 at 2:38 PM AKASHI Takahiro > > wrote: > > > > > > Adrian, Ben, > > > > > > Regarding _set_ios() function, > > > > > > On Fri, Aug 21, 2020 at 05:08:32PM +0300, Adrian Hunter wrote: > > > > On 10/07/20 2:10 pm, Ben Chuang wrote: > > > > > From: Ben Chuang > > > > > > > > > > In this commit, UHS-II related operations will be called via a > > > > > function > > > > > pointer array, sdhci_uhs2_ops, in order to make UHS-II support as > > > > > a kernel module. > > > > > This array will be initialized only if CONFIG_MMC_SDHCI_UHS2 is > > > > > enabled > > > > > and when the UHS-II module is loaded. Otherwise, all the functions > > > > > stay void. > > > > > > > > > > Signed-off-by: Ben Chuang > > > > > Signed-off-by: AKASHI Takahiro > > > > > > (snip) > > > > > > > > @@ -2261,6 +2324,7 @@ void sdhci_set_ios(struct mmc_host *mmc, struct > > > > > mmc_ios *ios) > > > > > { > > > > > struct sdhci_host *host = mmc_priv(mmc); > > > > > u8 ctrl; > > > > > + u16 ctrl_2; > > > > > > > > > > if (ios->power_mode == MMC_POWER_UNDEFINED) > > > > > return; > > > > > @@ -2287,6 +2351,10 @@ void sdhci_set_ios(struct mmc_host *mmc, > > > > > struct mmc_ios *ios) > > > > > sdhci_enable_preset_value(host, false); > > > > > > > > > > if (!ios->clock || ios->clock != host->clock) { > > > > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > > > > + ios->timing == MMC_TIMING_UHS2) > > > > > + host->timing = ios->timing; > > > > > + > > > > > host->ops->set_clock(host, ios->clock); > > > > > host->clock = ios->clock; > > > > > > > > > > @@ -2308,6 +2376,18 @@ void sdhci_set_ios(struct mmc_host *mmc, > > > > > struct mmc_ios *ios) > > > > > else > > > > > sdhci_set_power(host, ios->power_mode, ios->vdd); > > > > > > > > > > + /* 4.0 host support */ > > > > > + if (host->version >= SDHCI_SPEC_400) { > > > > > + /* UHS2 Support */ > > > > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > > > > + host->mmc->flags & MMC_UHS2_SUPPORT && > > > > > + host->mmc->caps & MMC_CAP_UHS2) { > > > > > + if (sdhci_uhs2_ops.do_set_ios) > > > > > + sdhci_uhs2_ops.do_set_ios(host, ios); > > > > > + return; > > > > > + } > > > > > + } > > > > > + > > > > > > > > Please look at using existing callbacks instead, maybe creating > > > > uhs2_set_ios(), uhs2_set_clock(), uhs2_set_power() > > > > > > I think that we will create uhs2_set_ios() (and uhs2_set_power() > > > as we discussed on patch#15/21), but not uhs_set_clock(). > > > > > > Since we have a hook only in struct mmc_host_ops, but not in struct > > > sdhci_ops, all the drivers who want to support UHS-II need to > > > set host->mmc_host_ops->set_ios to sdhci_uhs2_set_ios explicitly > > > in their own init (or probe) function. > > > (Again, sdhci_uhs2_set_ios() seems to be generic though.) > > > > > > Is this okay for you? > > > -> Adrian > > > > > > During refactoring the code, I found that sdhci_set_power() is called > > > twice in sdhci_set_ios(): > > > sdhci_set_ios(host, power_mode, vdd1, -1); in sdhci_set_ios(), and > > > sdhci_set_ios(host, power_mode, vdd1, vdd2) in ush2_do_set_ios() > > > > > > Can you please confirm that those are redundant? > > > > Yes, uhs2 set power is independent with uhs1. > > But set uhs2 power process should meet uhs2 spec. > > Can you elaborate a bit more about the last sentence, please? > > What I meant above is that > sdhci_set_ios(host, power_mode, vdd1, vdd2) in ush2_do_set_ios() > > this code will 'set_power' both vdd and vdd2 anyway and so > sdhci_set_ios(host, power_mode, vdd1, -1); in sdhci_set_ios(), and > is just redundant. > Yes, for uhs-2 flow, sdhci_set_ios(host, power_mode, vdd1, -1) is redundant. > > > > -> Ben > > > > > > I also wonder why we need spin locks in uhs2_do_set_ios() while > > > not in sdhci_set_ios(). > > > > You can check if spin locks in uhs2_do_set_ios() is necessary. > > I'm asking you. > > While calling set_ios() doesn't require spin locks, are you aware of > any cases where we need spin locks in calling set_ios() for uhs-2? > (I mean that callers/contexts are the same either for uhs or uhs-2.) I agree that it can be removed. I just didn't modify intel's original codes. > > -Takahiro Akashi > > > If set/clear irq can be execute safely without spin locks, you can > > remove spin locks. > > > > > > > > -> Ben > > > > > > -Takahiro Akashi
Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
Ben, On Fri, Sep 18, 2020 at 06:50:24PM +0800, Ben Chuang wrote: > On Fri, Sep 18, 2020 at 2:38 PM AKASHI Takahiro > wrote: > > > > Adrian, Ben, > > > > Regarding _set_ios() function, > > > > On Fri, Aug 21, 2020 at 05:08:32PM +0300, Adrian Hunter wrote: > > > On 10/07/20 2:10 pm, Ben Chuang wrote: > > > > From: Ben Chuang > > > > > > > > In this commit, UHS-II related operations will be called via a function > > > > pointer array, sdhci_uhs2_ops, in order to make UHS-II support as > > > > a kernel module. > > > > This array will be initialized only if CONFIG_MMC_SDHCI_UHS2 is enabled > > > > and when the UHS-II module is loaded. Otherwise, all the functions > > > > stay void. > > > > > > > > Signed-off-by: Ben Chuang > > > > Signed-off-by: AKASHI Takahiro > > > > (snip) > > > > > > @@ -2261,6 +2324,7 @@ void sdhci_set_ios(struct mmc_host *mmc, struct > > > > mmc_ios *ios) > > > > { > > > > struct sdhci_host *host = mmc_priv(mmc); > > > > u8 ctrl; > > > > + u16 ctrl_2; > > > > > > > > if (ios->power_mode == MMC_POWER_UNDEFINED) > > > > return; > > > > @@ -2287,6 +2351,10 @@ void sdhci_set_ios(struct mmc_host *mmc, struct > > > > mmc_ios *ios) > > > > sdhci_enable_preset_value(host, false); > > > > > > > > if (!ios->clock || ios->clock != host->clock) { > > > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > > > + ios->timing == MMC_TIMING_UHS2) > > > > + host->timing = ios->timing; > > > > + > > > > host->ops->set_clock(host, ios->clock); > > > > host->clock = ios->clock; > > > > > > > > @@ -2308,6 +2376,18 @@ void sdhci_set_ios(struct mmc_host *mmc, struct > > > > mmc_ios *ios) > > > > else > > > > sdhci_set_power(host, ios->power_mode, ios->vdd); > > > > > > > > + /* 4.0 host support */ > > > > + if (host->version >= SDHCI_SPEC_400) { > > > > + /* UHS2 Support */ > > > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > > > + host->mmc->flags & MMC_UHS2_SUPPORT && > > > > + host->mmc->caps & MMC_CAP_UHS2) { > > > > + if (sdhci_uhs2_ops.do_set_ios) > > > > + sdhci_uhs2_ops.do_set_ios(host, ios); > > > > + return; > > > > + } > > > > + } > > > > + > > > > > > Please look at using existing callbacks instead, maybe creating > > > uhs2_set_ios(), uhs2_set_clock(), uhs2_set_power() > > > > I think that we will create uhs2_set_ios() (and uhs2_set_power() > > as we discussed on patch#15/21), but not uhs_set_clock(). > > > > Since we have a hook only in struct mmc_host_ops, but not in struct > > sdhci_ops, all the drivers who want to support UHS-II need to > > set host->mmc_host_ops->set_ios to sdhci_uhs2_set_ios explicitly > > in their own init (or probe) function. > > (Again, sdhci_uhs2_set_ios() seems to be generic though.) > > > > Is this okay for you? > > -> Adrian > > > > During refactoring the code, I found that sdhci_set_power() is called > > twice in sdhci_set_ios(): > > sdhci_set_ios(host, power_mode, vdd1, -1); in sdhci_set_ios(), and > > sdhci_set_ios(host, power_mode, vdd1, vdd2) in ush2_do_set_ios() > > > > Can you please confirm that those are redundant? > > Yes, uhs2 set power is independent with uhs1. > But set uhs2 power process should meet uhs2 spec. Can you elaborate a bit more about the last sentence, please? What I meant above is that sdhci_set_ios(host, power_mode, vdd1, vdd2) in ush2_do_set_ios() this code will 'set_power' both vdd and vdd2 anyway and so sdhci_set_ios(host, power_mode, vdd1, -1); in sdhci_set_ios(), and is just redundant. > > -> Ben > > > > I also wonder why we need spin locks in uhs2_do_set_ios() while > > not in sdhci_set_ios(). > > You can check if spin locks in uhs2_do_set_ios() is necessary. I'm asking you. While calling set_ios() doesn't require spin locks, are you aware of any cases where we need spin locks in calling set_ios() for uhs-2? (I mean that callers/contexts are the same either for uhs or uhs-2.) -Takahiro Akashi > If set/clear irq can be execute safely without spin locks, you can > remove spin locks. > > > > > -> Ben > > > > -Takahiro Akashi
Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
Ben, On Fri, Sep 18, 2020 at 06:27:01PM +0800, Ben Chuang wrote: > On Fri, Sep 18, 2020 at 9:15 AM AKASHI Takahiro > wrote: > > > > Ben, > > > > On Thu, Sep 17, 2020 at 06:12:27PM +0800, Ben Chuang wrote: > > > Hi Takahiro, > > > > > > On Thu, Sep 17, 2020 at 1:12 PM AKASHI Takahiro > > > wrote: > > > > > > > > Adrian, Ben, > > > > > > > > Regarding _reset() function, > > > > > > > > On Fri, Aug 21, 2020 at 05:08:32PM +0300, Adrian Hunter wrote: > > > > > On 10/07/20 2:10 pm, Ben Chuang wrote: > > > > > > From: Ben Chuang > > > > > > > > > > > > In this commit, UHS-II related operations will be called via a > > > > > > function > > > > > > pointer array, sdhci_uhs2_ops, in order to make UHS-II support as > > > > > > a kernel module. > > > > > > This array will be initialized only if CONFIG_MMC_SDHCI_UHS2 is > > > > > > enabled > > > > > > and when the UHS-II module is loaded. Otherwise, all the functions > > > > > > stay void. > > > > > > > > > > > > Signed-off-by: Ben Chuang > > > > > > Signed-off-by: AKASHI Takahiro > > > > > > --- > > > > > > drivers/mmc/host/sdhci.c | 152 > > > > > > ++- > > > > > > 1 file changed, 136 insertions(+), 16 deletions(-) > > > > > > > > > > > > > > (snip) > > > > > > > > > > if (host->ops->platform_send_init_74_clocks) > > > > > > host->ops->platform_send_init_74_clocks(host, > > > > > > ios->power_mode); > > > > > > > > > > > > @@ -2331,7 +2411,7 @@ void sdhci_set_ios(struct mmc_host *mmc, > > > > > > struct mmc_ios *ios) > > > > > > } > > > > > > > > > > > > if (host->version >= SDHCI_SPEC_300) { > > > > > > - u16 clk, ctrl_2; > > > > > > + u16 clk; > > > > > > > > > > > > if (!host->preset_enabled) { > > > > > > sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL); > > > > > > @@ -3173,11 +3253,19 @@ static bool sdhci_request_done(struct > > > > > > sdhci_host *host) > > > > > > /* This is to force an update */ > > > > > > host->ops->set_clock(host, host->clock); > > > > > > > > > > > > - /* Spec says we should do both at the same time, but > > > > > > Ricoh > > > > > > - controllers do not like that. */ > > > > > > - sdhci_do_reset(host, SDHCI_RESET_CMD); > > > > > > - sdhci_do_reset(host, SDHCI_RESET_DATA); > > > > > > - > > > > > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > > > > > + host->mmc->flags & MMC_UHS2_INITIALIZED) { > > > > > > + if (sdhci_uhs2_ops.reset) > > > > > > + sdhci_uhs2_ops.reset(host, > > > > > > + > > > > > > SDHCI_UHS2_SW_RESET_SD); > > > > > > + } else { > > > > > > + /* > > > > > > +* Spec says we should do both at the same > > > > > > time, but > > > > > > +* Ricoh controllers do not like that. > > > > > > +*/ > > > > > > + sdhci_do_reset(host, SDHCI_RESET_CMD); > > > > > > + sdhci_do_reset(host, SDHCI_RESET_DATA); > > > > > > + } > > > > > > > > > > Please look at using the existing ->reset() sdhci host op instead. > > > > > > > > Well, the second argument to those reset functions is a bit-wise value > > > > to different "reset" registers, SDHCI_SOFTWARE_RESET and > > > > SDHCI_UHS2_SW_RESET, > > > > respectively. > > > > > > > > This fact raises a couple of questions to me: > > > > > > > > 1) Does it make sense to merge two functionality into one, i.e. > > > >sdhci_do_reset(), which is set to call ->reset hook? > > > > > > > > -> Adrian > > > > > > > > 2) UHS2_SW_RESET_SD is done only at this place while there are many > > > > callsites > > > >of reset(RESET_CMD|RESET_DATA) in sdhci.c. > > > >Why does the current code work? > > > > > > > >I found, in sdhci-pci-gli.c, > > > >sdhci_gl9755_reset() > > > > /* reset sd-tran on UHS2 mode if need to reset cmd/data */ > > > > if ((mask & SDHCI_RESET_CMD) | (mask & SDHCI_RESET_DATA)) > > > > gl9755_uhs2_reset_sd_tran(host); > > > > (A) > > > > > > > > > >Is this the trick to avoid the issue? > > > >(It looks redundant in terms of the hack above in > > > > sdhci_request_done() > > > >and even quite dirty to me. Moreover, no corresponding code for > > > > gl9750 > > > >and gl9763.) > > > > > > GL9755 currently does SD reset and UHS-II reset together. > > > > Do you mean that, in UHS-II operations, you need only the reset on > > SDHCI_UHS2_SW_RESET register? > > No, GL9755 does SD reset and UHS-II reset together. Is this also true for all sdhci controller drivers in general? As I said, I didn't find any precise description about this in SD specification. -Takahiro Akashi > > But the hunk above (A) does the UHS-II reset along with UHS-I reset. > > > > > There is no
Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
Adrian, This is, hopefully, my last reply to your comments on this patch#12. Regarding _request() and _send_command() (and more), On Fri, Aug 21, 2020 at 05:08:32PM +0300, Adrian Hunter wrote: > On 10/07/20 2:10 pm, Ben Chuang wrote: > > From: Ben Chuang > > > > In this commit, UHS-II related operations will be called via a function > > pointer array, sdhci_uhs2_ops, in order to make UHS-II support as > > a kernel module. > > This array will be initialized only if CONFIG_MMC_SDHCI_UHS2 is enabled > > and when the UHS-II module is loaded. Otherwise, all the functions > > stay void. > > (snip) > Again, this is what I want to avoid. I would like to have 3 kinds of > functions: > - SD mode only > - UHS-II only > - SD functions with no UHS-II code, that can also be used by UHS-II > i.e. I don't want to mix UHS-II code and SD mode code in the same function. > > I think sdhci-uhs2.c should provide a request function and a send_command > function. > I would start by removing everything you may not need, and then see if you > have any problems. > e.g. > > void uhs2_request(struct mmc_host *mmc, struct mmc_request *mrq) > { > struct sdhci_host *host = mmc_priv(mmc); > struct mmc_command *cmd; > unsigned long flags; > > if (!host->uhs2_mode) { > sdhci_request(mmc, mrq); > return; > } > > spin_lock_irqsave(>lock, flags); > uhs2_send_command(host, cmd); > spin_unlock_irqrestore(>lock, flags); > } > EXPORT_SYMBOL_GPL(uhs2_request); > > For sdhci_prepare_data(), I would factor out the dma part, so > > static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command > *cmd) > { > struct mmc_data *data = cmd->data; > > sdhci_initialize_data(host, data); > > sdhci_prepare_dma(host, data); > > sdhci_set_block_info(host, data); > } > > The you could export sdhci_initialize_data() and sdhci_prepare_dma() for uhs2. > > > } > > > > #if IS_ENABLED(CONFIG_MMC_SDHCI_EXTERNAL_DMA) > > @@ -1439,6 +1463,13 @@ static void sdhci_set_transfer_mode(struct > > sdhci_host *host, > > u16 mode = 0; > > struct mmc_data *data = cmd->data; > > > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > + host->mmc->flags & MMC_UHS2_SUPPORT) { > > + if (sdhci_uhs2_ops.set_transfer_mode) > > + sdhci_uhs2_ops.set_transfer_mode(host, cmd); > > + return; > > + } > > + > > Once you provide uhs2_request() and uhs2_send_command(), the transfer mode > setting can be done in sdhci-uhs2.c If I try to make changes as you suggested above, a lot of other uhs2-flavored functions will also be created due to calling dependency/sequences and for "completeness" compared to uhs counterparts. They probably include sdhci_uhs2_prepare_data() sdhci_uhs2_external_dma_prepare_data() sdhci_uhs2_send_command() sdhci_uhs2_send_command_try() sdhci_uhs2_send_tuning() sdhci_uhs2_request() sdhci_uhs2_request_atomic() sdhci_uhs2_thread_irq() sdhci_uhs2_irq() sdhci_uhs2_cmd_irq() sdhci_uhs2_finish_command() sdhci_uhs2_resume_host() __sdhci_uhs2_add_host() sdhci_uhs2_add_host() (Some may not be used under the current drivers.) In addition, a bunch of functions in sdhci.c will also have to be exported to uhs2 as "global" functions instead of "static." Is this all that you expect to see? -Takahiro Akashi > > if (data == NULL) { > > if (host->quirks2 & > > SDHCI_QUIRK2_CLEAR_TRANSFERMODE_REG_BEFORE_CMD) { > > @@ -1570,6 +1601,12 @@ static void __sdhci_finish_data(struct sdhci_host > > *host, bool sw_data_timeout) > > else > > data->bytes_xfered = data->blksz * data->blocks; > > > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > + host->mmc->flags & MMC_UHS2_INITIALIZED) { > > + __sdhci_finish_mrq(host, data->mrq); > > + return; > > + } > > At least to start with, I think it would be better to handle UHS-II cmd and > data interrupts completely in sdhci-uhs2.c > > > + > > /* > > * Need to send CMD12 if - > > * a) open-ended multiblock transfer not using auto CMD12 (no CMD23) > > @@ -1654,7 +1691,8 @@ static bool sdhci_send_command(struct sdhci_host > > *host, struct mmc_command *cmd) > > sdhci_prepare_data(host, cmd); > > } > > > > - sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT); > > + if (!IS_ENABLED(CONFIG_MMC_SDHCI_UHS2)) > > + sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT); > > Not needed when instead you provide uhs2_send_command() > > > > sdhci_set_transfer_mode(host, cmd); > > > > @@ -1699,6 +1737,17 @@ static bool sdhci_send_command(struct sdhci_host > > *host, struct mmc_command *cmd) > > if (host->use_external_dma) > > sdhci_external_dma_pre_transfer(host, cmd); > > > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > +
Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
On Fri, Sep 18, 2020 at 2:38 PM AKASHI Takahiro wrote: > > Adrian, Ben, > > Regarding _set_ios() function, > > On Fri, Aug 21, 2020 at 05:08:32PM +0300, Adrian Hunter wrote: > > On 10/07/20 2:10 pm, Ben Chuang wrote: > > > From: Ben Chuang > > > > > > In this commit, UHS-II related operations will be called via a function > > > pointer array, sdhci_uhs2_ops, in order to make UHS-II support as > > > a kernel module. > > > This array will be initialized only if CONFIG_MMC_SDHCI_UHS2 is enabled > > > and when the UHS-II module is loaded. Otherwise, all the functions > > > stay void. > > > > > > Signed-off-by: Ben Chuang > > > Signed-off-by: AKASHI Takahiro > > (snip) > > > > @@ -2261,6 +2324,7 @@ void sdhci_set_ios(struct mmc_host *mmc, struct > > > mmc_ios *ios) > > > { > > > struct sdhci_host *host = mmc_priv(mmc); > > > u8 ctrl; > > > + u16 ctrl_2; > > > > > > if (ios->power_mode == MMC_POWER_UNDEFINED) > > > return; > > > @@ -2287,6 +2351,10 @@ void sdhci_set_ios(struct mmc_host *mmc, struct > > > mmc_ios *ios) > > > sdhci_enable_preset_value(host, false); > > > > > > if (!ios->clock || ios->clock != host->clock) { > > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > > + ios->timing == MMC_TIMING_UHS2) > > > + host->timing = ios->timing; > > > + > > > host->ops->set_clock(host, ios->clock); > > > host->clock = ios->clock; > > > > > > @@ -2308,6 +2376,18 @@ void sdhci_set_ios(struct mmc_host *mmc, struct > > > mmc_ios *ios) > > > else > > > sdhci_set_power(host, ios->power_mode, ios->vdd); > > > > > > + /* 4.0 host support */ > > > + if (host->version >= SDHCI_SPEC_400) { > > > + /* UHS2 Support */ > > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > > + host->mmc->flags & MMC_UHS2_SUPPORT && > > > + host->mmc->caps & MMC_CAP_UHS2) { > > > + if (sdhci_uhs2_ops.do_set_ios) > > > + sdhci_uhs2_ops.do_set_ios(host, ios); > > > + return; > > > + } > > > + } > > > + > > > > Please look at using existing callbacks instead, maybe creating > > uhs2_set_ios(), uhs2_set_clock(), uhs2_set_power() > > I think that we will create uhs2_set_ios() (and uhs2_set_power() > as we discussed on patch#15/21), but not uhs_set_clock(). > > Since we have a hook only in struct mmc_host_ops, but not in struct > sdhci_ops, all the drivers who want to support UHS-II need to > set host->mmc_host_ops->set_ios to sdhci_uhs2_set_ios explicitly > in their own init (or probe) function. > (Again, sdhci_uhs2_set_ios() seems to be generic though.) > > Is this okay for you? > -> Adrian > > During refactoring the code, I found that sdhci_set_power() is called > twice in sdhci_set_ios(): > sdhci_set_ios(host, power_mode, vdd1, -1); in sdhci_set_ios(), and > sdhci_set_ios(host, power_mode, vdd1, vdd2) in ush2_do_set_ios() > > Can you please confirm that those are redundant? Yes, uhs2 set power is independent with uhs1. But set uhs2 power process should meet uhs2 spec. > -> Ben > > I also wonder why we need spin locks in uhs2_do_set_ios() while > not in sdhci_set_ios(). You can check if spin locks in uhs2_do_set_ios() is necessary. If set/clear irq can be execute safely without spin locks, you can remove spin locks. > > -> Ben > > -Takahiro Akashi
Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
On Fri, Sep 18, 2020 at 9:15 AM AKASHI Takahiro wrote: > > Ben, > > On Thu, Sep 17, 2020 at 06:12:27PM +0800, Ben Chuang wrote: > > Hi Takahiro, > > > > On Thu, Sep 17, 2020 at 1:12 PM AKASHI Takahiro > > wrote: > > > > > > Adrian, Ben, > > > > > > Regarding _reset() function, > > > > > > On Fri, Aug 21, 2020 at 05:08:32PM +0300, Adrian Hunter wrote: > > > > On 10/07/20 2:10 pm, Ben Chuang wrote: > > > > > From: Ben Chuang > > > > > > > > > > In this commit, UHS-II related operations will be called via a > > > > > function > > > > > pointer array, sdhci_uhs2_ops, in order to make UHS-II support as > > > > > a kernel module. > > > > > This array will be initialized only if CONFIG_MMC_SDHCI_UHS2 is > > > > > enabled > > > > > and when the UHS-II module is loaded. Otherwise, all the functions > > > > > stay void. > > > > > > > > > > Signed-off-by: Ben Chuang > > > > > Signed-off-by: AKASHI Takahiro > > > > > --- > > > > > drivers/mmc/host/sdhci.c | 152 > > > > > ++- > > > > > 1 file changed, 136 insertions(+), 16 deletions(-) > > > > > > > > > > > (snip) > > > > > > > > if (host->ops->platform_send_init_74_clocks) > > > > > host->ops->platform_send_init_74_clocks(host, > > > > > ios->power_mode); > > > > > > > > > > @@ -2331,7 +2411,7 @@ void sdhci_set_ios(struct mmc_host *mmc, struct > > > > > mmc_ios *ios) > > > > > } > > > > > > > > > > if (host->version >= SDHCI_SPEC_300) { > > > > > - u16 clk, ctrl_2; > > > > > + u16 clk; > > > > > > > > > > if (!host->preset_enabled) { > > > > > sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL); > > > > > @@ -3173,11 +3253,19 @@ static bool sdhci_request_done(struct > > > > > sdhci_host *host) > > > > > /* This is to force an update */ > > > > > host->ops->set_clock(host, host->clock); > > > > > > > > > > - /* Spec says we should do both at the same time, but Ricoh > > > > > - controllers do not like that. */ > > > > > - sdhci_do_reset(host, SDHCI_RESET_CMD); > > > > > - sdhci_do_reset(host, SDHCI_RESET_DATA); > > > > > - > > > > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > > > > + host->mmc->flags & MMC_UHS2_INITIALIZED) { > > > > > + if (sdhci_uhs2_ops.reset) > > > > > + sdhci_uhs2_ops.reset(host, > > > > > + > > > > > SDHCI_UHS2_SW_RESET_SD); > > > > > + } else { > > > > > + /* > > > > > +* Spec says we should do both at the same time, > > > > > but > > > > > +* Ricoh controllers do not like that. > > > > > +*/ > > > > > + sdhci_do_reset(host, SDHCI_RESET_CMD); > > > > > + sdhci_do_reset(host, SDHCI_RESET_DATA); > > > > > + } > > > > > > > > Please look at using the existing ->reset() sdhci host op instead. > > > > > > Well, the second argument to those reset functions is a bit-wise value > > > to different "reset" registers, SDHCI_SOFTWARE_RESET and > > > SDHCI_UHS2_SW_RESET, > > > respectively. > > > > > > This fact raises a couple of questions to me: > > > > > > 1) Does it make sense to merge two functionality into one, i.e. > > >sdhci_do_reset(), which is set to call ->reset hook? > > > > > > -> Adrian > > > > > > 2) UHS2_SW_RESET_SD is done only at this place while there are many > > > callsites > > >of reset(RESET_CMD|RESET_DATA) in sdhci.c. > > >Why does the current code work? > > > > > >I found, in sdhci-pci-gli.c, > > >sdhci_gl9755_reset() > > > /* reset sd-tran on UHS2 mode if need to reset cmd/data */ > > > if ((mask & SDHCI_RESET_CMD) | (mask & SDHCI_RESET_DATA)) > > > gl9755_uhs2_reset_sd_tran(host); > > (A) > > > > > > >Is this the trick to avoid the issue? > > >(It looks redundant in terms of the hack above in sdhci_request_done() > > >and even quite dirty to me. Moreover, no corresponding code for gl9750 > > >and gl9763.) > > > > GL9755 currently does SD reset and UHS-II reset together. > > Do you mean that, in UHS-II operations, you need only the reset on > SDHCI_UHS2_SW_RESET register? No, GL9755 does SD reset and UHS-II reset together. > But the hunk above (A) does the UHS-II reset along with UHS-I reset. > > > There is no UHS-II interface on gl9750 and gl9763e. > > > > > > > > -> Ben > > > > > > 3) (More or less SD specification issue) > > >In UHS-II mode, do we have to call reset(SHCI_RESET_ALL) along with > > >reset(UHS2_SW_RESET_FULL)? > > >Under the current implementation, both will be called at the end. > > > > > > > As I know, the UHS2_SW_RESET_FULL is only for UHS-II. > > Can you list the lines that reset(SHCI_RESET_ALL) and > > reset(UHS2_SW_RESET_FULL) are both called? > > I was not clear
Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
Adrian, Ben, Regarding _set_ios() function, On Fri, Aug 21, 2020 at 05:08:32PM +0300, Adrian Hunter wrote: > On 10/07/20 2:10 pm, Ben Chuang wrote: > > From: Ben Chuang > > > > In this commit, UHS-II related operations will be called via a function > > pointer array, sdhci_uhs2_ops, in order to make UHS-II support as > > a kernel module. > > This array will be initialized only if CONFIG_MMC_SDHCI_UHS2 is enabled > > and when the UHS-II module is loaded. Otherwise, all the functions > > stay void. > > > > Signed-off-by: Ben Chuang > > Signed-off-by: AKASHI Takahiro (snip) > > @@ -2261,6 +2324,7 @@ void sdhci_set_ios(struct mmc_host *mmc, struct > > mmc_ios *ios) > > { > > struct sdhci_host *host = mmc_priv(mmc); > > u8 ctrl; > > + u16 ctrl_2; > > > > if (ios->power_mode == MMC_POWER_UNDEFINED) > > return; > > @@ -2287,6 +2351,10 @@ void sdhci_set_ios(struct mmc_host *mmc, struct > > mmc_ios *ios) > > sdhci_enable_preset_value(host, false); > > > > if (!ios->clock || ios->clock != host->clock) { > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > + ios->timing == MMC_TIMING_UHS2) > > + host->timing = ios->timing; > > + > > host->ops->set_clock(host, ios->clock); > > host->clock = ios->clock; > > > > @@ -2308,6 +2376,18 @@ void sdhci_set_ios(struct mmc_host *mmc, struct > > mmc_ios *ios) > > else > > sdhci_set_power(host, ios->power_mode, ios->vdd); > > > > + /* 4.0 host support */ > > + if (host->version >= SDHCI_SPEC_400) { > > + /* UHS2 Support */ > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > + host->mmc->flags & MMC_UHS2_SUPPORT && > > + host->mmc->caps & MMC_CAP_UHS2) { > > + if (sdhci_uhs2_ops.do_set_ios) > > + sdhci_uhs2_ops.do_set_ios(host, ios); > > + return; > > + } > > + } > > + > > Please look at using existing callbacks instead, maybe creating > uhs2_set_ios(), uhs2_set_clock(), uhs2_set_power() I think that we will create uhs2_set_ios() (and uhs2_set_power() as we discussed on patch#15/21), but not uhs_set_clock(). Since we have a hook only in struct mmc_host_ops, but not in struct sdhci_ops, all the drivers who want to support UHS-II need to set host->mmc_host_ops->set_ios to sdhci_uhs2_set_ios explicitly in their own init (or probe) function. (Again, sdhci_uhs2_set_ios() seems to be generic though.) Is this okay for you? -> Adrian During refactoring the code, I found that sdhci_set_power() is called twice in sdhci_set_ios(): sdhci_set_ios(host, power_mode, vdd1, -1); in sdhci_set_ios(), and sdhci_set_ios(host, power_mode, vdd1, vdd2) in ush2_do_set_ios() Can you please confirm that those are redundant? -> Ben I also wonder why we need spin locks in uhs2_do_set_ios() while not in sdhci_set_ios(). -> Ben -Takahiro Akashi
Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
Ben, On Thu, Sep 17, 2020 at 06:12:27PM +0800, Ben Chuang wrote: > Hi Takahiro, > > On Thu, Sep 17, 2020 at 1:12 PM AKASHI Takahiro > wrote: > > > > Adrian, Ben, > > > > Regarding _reset() function, > > > > On Fri, Aug 21, 2020 at 05:08:32PM +0300, Adrian Hunter wrote: > > > On 10/07/20 2:10 pm, Ben Chuang wrote: > > > > From: Ben Chuang > > > > > > > > In this commit, UHS-II related operations will be called via a function > > > > pointer array, sdhci_uhs2_ops, in order to make UHS-II support as > > > > a kernel module. > > > > This array will be initialized only if CONFIG_MMC_SDHCI_UHS2 is enabled > > > > and when the UHS-II module is loaded. Otherwise, all the functions > > > > stay void. > > > > > > > > Signed-off-by: Ben Chuang > > > > Signed-off-by: AKASHI Takahiro > > > > --- > > > > drivers/mmc/host/sdhci.c | 152 ++- > > > > 1 file changed, 136 insertions(+), 16 deletions(-) > > > > > > > > (snip) > > > > > > if (host->ops->platform_send_init_74_clocks) > > > > host->ops->platform_send_init_74_clocks(host, > > > > ios->power_mode); > > > > > > > > @@ -2331,7 +2411,7 @@ void sdhci_set_ios(struct mmc_host *mmc, struct > > > > mmc_ios *ios) > > > > } > > > > > > > > if (host->version >= SDHCI_SPEC_300) { > > > > - u16 clk, ctrl_2; > > > > + u16 clk; > > > > > > > > if (!host->preset_enabled) { > > > > sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL); > > > > @@ -3173,11 +3253,19 @@ static bool sdhci_request_done(struct > > > > sdhci_host *host) > > > > /* This is to force an update */ > > > > host->ops->set_clock(host, host->clock); > > > > > > > > - /* Spec says we should do both at the same time, but Ricoh > > > > - controllers do not like that. */ > > > > - sdhci_do_reset(host, SDHCI_RESET_CMD); > > > > - sdhci_do_reset(host, SDHCI_RESET_DATA); > > > > - > > > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > > > + host->mmc->flags & MMC_UHS2_INITIALIZED) { > > > > + if (sdhci_uhs2_ops.reset) > > > > + sdhci_uhs2_ops.reset(host, > > > > + > > > > SDHCI_UHS2_SW_RESET_SD); > > > > + } else { > > > > + /* > > > > +* Spec says we should do both at the same time, but > > > > +* Ricoh controllers do not like that. > > > > +*/ > > > > + sdhci_do_reset(host, SDHCI_RESET_CMD); > > > > + sdhci_do_reset(host, SDHCI_RESET_DATA); > > > > + } > > > > > > Please look at using the existing ->reset() sdhci host op instead. > > > > Well, the second argument to those reset functions is a bit-wise value > > to different "reset" registers, SDHCI_SOFTWARE_RESET and > > SDHCI_UHS2_SW_RESET, > > respectively. > > > > This fact raises a couple of questions to me: > > > > 1) Does it make sense to merge two functionality into one, i.e. > >sdhci_do_reset(), which is set to call ->reset hook? > > > > -> Adrian > > > > 2) UHS2_SW_RESET_SD is done only at this place while there are many > > callsites > >of reset(RESET_CMD|RESET_DATA) in sdhci.c. > >Why does the current code work? > > > >I found, in sdhci-pci-gli.c, > >sdhci_gl9755_reset() > > /* reset sd-tran on UHS2 mode if need to reset cmd/data */ > > if ((mask & SDHCI_RESET_CMD) | (mask & SDHCI_RESET_DATA)) > > gl9755_uhs2_reset_sd_tran(host); (A) > > > >Is this the trick to avoid the issue? > >(It looks redundant in terms of the hack above in sdhci_request_done() > >and even quite dirty to me. Moreover, no corresponding code for gl9750 > >and gl9763.) > > GL9755 currently does SD reset and UHS-II reset together. Do you mean that, in UHS-II operations, you need only the reset on SDHCI_UHS2_SW_RESET register? But the hunk above (A) does the UHS-II reset along with UHS-I reset. > There is no UHS-II interface on gl9750 and gl9763e. > > > > > -> Ben > > > > 3) (More or less SD specification issue) > >In UHS-II mode, do we have to call reset(SHCI_RESET_ALL) along with > >reset(UHS2_SW_RESET_FULL)? > >Under the current implementation, both will be called at the end. > > > > As I know, the UHS2_SW_RESET_FULL is only for UHS-II. > Can you list the lines that reset(SHCI_RESET_ALL) and > reset(UHS2_SW_RESET_FULL) are both called? I was not clear here. (The above is also another example.) Look at sdhci_remove_host() and shdci_uhs2_remote_host(). If the argument 'dead' is 0, we will do both of the resets for UHS-II. -Takahiro Akashi > > -> Adrian, Ben > > > > 4) (Not directly linked to UHS-II support) > > In some places, we see the sequence: > > sdhci_do_reset(host, SDHCI_RESET_CMD); > > sdhci_do_reset(host,
Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
Hi Takahiro, On Thu, Sep 17, 2020 at 1:12 PM AKASHI Takahiro wrote: > > Adrian, Ben, > > Regarding _reset() function, > > On Fri, Aug 21, 2020 at 05:08:32PM +0300, Adrian Hunter wrote: > > On 10/07/20 2:10 pm, Ben Chuang wrote: > > > From: Ben Chuang > > > > > > In this commit, UHS-II related operations will be called via a function > > > pointer array, sdhci_uhs2_ops, in order to make UHS-II support as > > > a kernel module. > > > This array will be initialized only if CONFIG_MMC_SDHCI_UHS2 is enabled > > > and when the UHS-II module is loaded. Otherwise, all the functions > > > stay void. > > > > > > Signed-off-by: Ben Chuang > > > Signed-off-by: AKASHI Takahiro > > > --- > > > drivers/mmc/host/sdhci.c | 152 ++- > > > 1 file changed, 136 insertions(+), 16 deletions(-) > > > > > (snip) > > > > if (host->ops->platform_send_init_74_clocks) > > > host->ops->platform_send_init_74_clocks(host, > > > ios->power_mode); > > > > > > @@ -2331,7 +2411,7 @@ void sdhci_set_ios(struct mmc_host *mmc, struct > > > mmc_ios *ios) > > > } > > > > > > if (host->version >= SDHCI_SPEC_300) { > > > - u16 clk, ctrl_2; > > > + u16 clk; > > > > > > if (!host->preset_enabled) { > > > sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL); > > > @@ -3173,11 +3253,19 @@ static bool sdhci_request_done(struct sdhci_host > > > *host) > > > /* This is to force an update */ > > > host->ops->set_clock(host, host->clock); > > > > > > - /* Spec says we should do both at the same time, but Ricoh > > > - controllers do not like that. */ > > > - sdhci_do_reset(host, SDHCI_RESET_CMD); > > > - sdhci_do_reset(host, SDHCI_RESET_DATA); > > > - > > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > > + host->mmc->flags & MMC_UHS2_INITIALIZED) { > > > + if (sdhci_uhs2_ops.reset) > > > + sdhci_uhs2_ops.reset(host, > > > +SDHCI_UHS2_SW_RESET_SD); > > > + } else { > > > + /* > > > +* Spec says we should do both at the same time, but > > > +* Ricoh controllers do not like that. > > > +*/ > > > + sdhci_do_reset(host, SDHCI_RESET_CMD); > > > + sdhci_do_reset(host, SDHCI_RESET_DATA); > > > + } > > > > Please look at using the existing ->reset() sdhci host op instead. > > Well, the second argument to those reset functions is a bit-wise value > to different "reset" registers, SDHCI_SOFTWARE_RESET and SDHCI_UHS2_SW_RESET, > respectively. > > This fact raises a couple of questions to me: > > 1) Does it make sense to merge two functionality into one, i.e. >sdhci_do_reset(), which is set to call ->reset hook? > > -> Adrian > > 2) UHS2_SW_RESET_SD is done only at this place while there are many callsites >of reset(RESET_CMD|RESET_DATA) in sdhci.c. >Why does the current code work? > >I found, in sdhci-pci-gli.c, >sdhci_gl9755_reset() > /* reset sd-tran on UHS2 mode if need to reset cmd/data */ > if ((mask & SDHCI_RESET_CMD) | (mask & SDHCI_RESET_DATA)) > gl9755_uhs2_reset_sd_tran(host); > >Is this the trick to avoid the issue? >(It looks redundant in terms of the hack above in sdhci_request_done() >and even quite dirty to me. Moreover, no corresponding code for gl9750 >and gl9763.) GL9755 currently does SD reset and UHS-II reset together. There is no UHS-II interface on gl9750 and gl9763e. > > -> Ben > > 3) (More or less SD specification issue) >In UHS-II mode, do we have to call reset(SHCI_RESET_ALL) along with >reset(UHS2_SW_RESET_FULL)? >Under the current implementation, both will be called at the end. > As I know, the UHS2_SW_RESET_FULL is only for UHS-II. Can you list the lines that reset(SHCI_RESET_ALL) and reset(UHS2_SW_RESET_FULL) are both called? > -> Adrian, Ben > > 4) (Not directly linked to UHS-II support) > In some places, we see the sequence: > sdhci_do_reset(host, SDHCI_RESET_CMD); > sdhci_do_reset(host, SDHCI_RESET_DATA); > while in other places, > sdhci_do_reset(host, SDHCI_RESET_CMD | SDHCI_RESET_DATA); > > If the statement below is true, > > > - /* Spec says we should do both at the same time, but Ricoh > > > - controllers do not like that. */ > the latter should be wrong. > > -> Adrian > > -Takahiro Akashi > > > > > > host->pending_reset = false; > > > } > > > > > > @@ -3532,6 +3620,13 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id) > > > SDHCI_INT_BUS_POWER); > > > sdhci_writel(host, mask, SDHCI_INT_STATUS); > > > > > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > >
Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
On Thu, Sep 17, 2020 at 07:52:03AM +0300, Adrian Hunter wrote: > On 17/09/20 5:31 am, AKASHI Takahiro wrote: > > Adrian, > > > > On Wed, Sep 16, 2020 at 01:00:35PM +0300, Adrian Hunter wrote: > >> On 16/09/20 11:05 am, AKASHI Takahiro wrote: > >>> Adrian, > >>> > >>> Your comments are scattered over various functions, and so > >>> I would like to address them in separate replies. > >>> > >>> First, I'd like to discuss sdhci_[add|remove]_host(). > >>> > >>> On Fri, Aug 21, 2020 at 05:08:32PM +0300, Adrian Hunter wrote: > On 10/07/20 2:10 pm, Ben Chuang wrote: > > From: Ben Chuang > > > > In this commit, UHS-II related operations will be called via a function > > pointer array, sdhci_uhs2_ops, in order to make UHS-II support as > > a kernel module. > > This array will be initialized only if CONFIG_MMC_SDHCI_UHS2 is enabled > > and when the UHS-II module is loaded. Otherwise, all the functions > > stay void. > > > > Signed-off-by: Ben Chuang > > Signed-off-by: AKASHI Takahiro > > --- > >>> > >>> (snip) > >>> > > if (intmask & (SDHCI_INT_CARD_INSERT | > > SDHCI_INT_CARD_REMOVE)) { > > u32 present = sdhci_readl(host, > > SDHCI_PRESENT_STATE) & > > SDHCI_CARD_PRESENT; > > @@ -4717,6 +4812,14 @@ int sdhci_setup_host(struct sdhci_host *host) > > /* This may alter mmc->*_blk_* parameters */ > > sdhci_allocate_bounce_buffer(host); > > > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > + host->version >= SDHCI_SPEC_400 && > > + sdhci_uhs2_ops.add_host) { > > + ret = sdhci_uhs2_ops.add_host(host, host->caps1); > > + if (ret) > > + goto unreg; > > + } > > + > > I think you should look at creating uhs2_add_host() instead > > > return 0; > > > > unreg: > > @@ -4738,6 +4841,8 @@ void sdhci_cleanup_host(struct sdhci_host *host) > > { > > struct mmc_host *mmc = host->mmc; > > > > + /* FIXME: Do we have to do some cleanup for UHS2 here? */ > > + > > if (!IS_ERR(mmc->supply.vqmmc)) > > regulator_disable(mmc->supply.vqmmc); > > > > @@ -4766,6 +4871,14 @@ int __sdhci_add_host(struct sdhci_host *host) > > mmc->cqe_ops = NULL; > > } > > > > + if ((mmc->caps & MMC_CAP_UHS2) && !host->v4_mode) { > > + /* host doesn't want to enable UHS2 support */ > > + mmc->caps &= ~MMC_CAP_UHS2; > > + mmc->flags &= ~MMC_UHS2_SUPPORT; > > + > > + /* FIXME: Do we have to do some cleanup here? */ > > + } > > + > > host->complete_wq = alloc_workqueue("sdhci", flags, 0); > > if (!host->complete_wq) > > return -ENOMEM; > > @@ -4812,6 +4925,9 @@ int __sdhci_add_host(struct sdhci_host *host) > > unled: > > sdhci_led_unregister(host); > > unirq: > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > + sdhci_uhs2_ops.remove_host) > > + sdhci_uhs2_ops.remove_host(host, 0); > > sdhci_do_reset(host, SDHCI_RESET_ALL); > > sdhci_writel(host, 0, SDHCI_INT_ENABLE); > > sdhci_writel(host, 0, SDHCI_SIGNAL_ENABLE); > > @@ -4869,6 +4985,10 @@ void sdhci_remove_host(struct sdhci_host *host, > > int dead) > > > > sdhci_led_unregister(host); > > > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > + sdhci_uhs2_ops.remove_host) > > + sdhci_uhs2_ops.remove_host(host, dead); > > + > > I think you should look at creating uhs2_remove_host() instead > >>> > >>> You suggest that we will have separate sdhci_uhs2_[add|remove]_host(), > >>> but I don't think it's always convenient. > >>> > >>> UHS-II capable host will be set to call sdhci_uhs2_add_host() explicitly, > >>> but we can't do that in case of pci and pltfm based drivers as they > >>> utilize > >>> common helper functions, sdhci_pci_probe() and sdhci_pltfm_register(), > >>> respectively. > >> > >> sdhci-pci has an add_host op > >> > >> sdhci_pltfm_init can be used instead of sdhci_pltfm_register > >> > >> > >>> Therefore, we inevitably have to call sdhci_uhs2_add_host() there. > >>> > >>> If so, why should we distinguish sdhci_uhs2_add_host from > >>> sdhci_uhs_add_host? > >>> I don't see any good reason. > >>> Moreover, as a result, there exists a mixed usage of sdhci_ interfaces > >>> and sdhci_uhs2_ interfaces in sdhci-pci-core.c and sdhci-pltfm.c. > >>> > >>> It sounds odd to me. > >> > >> It is already done that way for cqhci. > > > > Okay, if it is your policy, I will follow that. > > Then, I'm going to add > > -
Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
Adrian, Ben, Regarding _reset() function, On Fri, Aug 21, 2020 at 05:08:32PM +0300, Adrian Hunter wrote: > On 10/07/20 2:10 pm, Ben Chuang wrote: > > From: Ben Chuang > > > > In this commit, UHS-II related operations will be called via a function > > pointer array, sdhci_uhs2_ops, in order to make UHS-II support as > > a kernel module. > > This array will be initialized only if CONFIG_MMC_SDHCI_UHS2 is enabled > > and when the UHS-II module is loaded. Otherwise, all the functions > > stay void. > > > > Signed-off-by: Ben Chuang > > Signed-off-by: AKASHI Takahiro > > --- > > drivers/mmc/host/sdhci.c | 152 ++- > > 1 file changed, 136 insertions(+), 16 deletions(-) > > (snip) > > if (host->ops->platform_send_init_74_clocks) > > host->ops->platform_send_init_74_clocks(host, ios->power_mode); > > > > @@ -2331,7 +2411,7 @@ void sdhci_set_ios(struct mmc_host *mmc, struct > > mmc_ios *ios) > > } > > > > if (host->version >= SDHCI_SPEC_300) { > > - u16 clk, ctrl_2; > > + u16 clk; > > > > if (!host->preset_enabled) { > > sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL); > > @@ -3173,11 +3253,19 @@ static bool sdhci_request_done(struct sdhci_host > > *host) > > /* This is to force an update */ > > host->ops->set_clock(host, host->clock); > > > > - /* Spec says we should do both at the same time, but Ricoh > > - controllers do not like that. */ > > - sdhci_do_reset(host, SDHCI_RESET_CMD); > > - sdhci_do_reset(host, SDHCI_RESET_DATA); > > - > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > + host->mmc->flags & MMC_UHS2_INITIALIZED) { > > + if (sdhci_uhs2_ops.reset) > > + sdhci_uhs2_ops.reset(host, > > +SDHCI_UHS2_SW_RESET_SD); > > + } else { > > + /* > > +* Spec says we should do both at the same time, but > > +* Ricoh controllers do not like that. > > +*/ > > + sdhci_do_reset(host, SDHCI_RESET_CMD); > > + sdhci_do_reset(host, SDHCI_RESET_DATA); > > + } > > Please look at using the existing ->reset() sdhci host op instead. Well, the second argument to those reset functions is a bit-wise value to different "reset" registers, SDHCI_SOFTWARE_RESET and SDHCI_UHS2_SW_RESET, respectively. This fact raises a couple of questions to me: 1) Does it make sense to merge two functionality into one, i.e. sdhci_do_reset(), which is set to call ->reset hook? -> Adrian 2) UHS2_SW_RESET_SD is done only at this place while there are many callsites of reset(RESET_CMD|RESET_DATA) in sdhci.c. Why does the current code work? I found, in sdhci-pci-gli.c, sdhci_gl9755_reset() /* reset sd-tran on UHS2 mode if need to reset cmd/data */ if ((mask & SDHCI_RESET_CMD) | (mask & SDHCI_RESET_DATA)) gl9755_uhs2_reset_sd_tran(host); Is this the trick to avoid the issue? (It looks redundant in terms of the hack above in sdhci_request_done() and even quite dirty to me. Moreover, no corresponding code for gl9750 and gl9763.) -> Ben 3) (More or less SD specification issue) In UHS-II mode, do we have to call reset(SHCI_RESET_ALL) along with reset(UHS2_SW_RESET_FULL)? Under the current implementation, both will be called at the end. -> Adrian, Ben 4) (Not directly linked to UHS-II support) In some places, we see the sequence: sdhci_do_reset(host, SDHCI_RESET_CMD); sdhci_do_reset(host, SDHCI_RESET_DATA); while in other places, sdhci_do_reset(host, SDHCI_RESET_CMD | SDHCI_RESET_DATA); If the statement below is true, > > - /* Spec says we should do both at the same time, but Ricoh > > - controllers do not like that. */ the latter should be wrong. -> Adrian -Takahiro Akashi > > host->pending_reset = false; > > } > > > > @@ -3532,6 +3620,13 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id) > > SDHCI_INT_BUS_POWER); > > sdhci_writel(host, mask, SDHCI_INT_STATUS); > > > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > + intmask & SDHCI_INT_ERROR && > > + host->mmc->flags & MMC_UHS2_SUPPORT) { > > + if (sdhci_uhs2_ops.irq) > > + sdhci_uhs2_ops.irq(host); > > + } > > + > > Please look at using the existing ->irq() sdhci host op instead > > > if (intmask & (SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE)) { > > u32 present = sdhci_readl(host, SDHCI_PRESENT_STATE) & > > SDHCI_CARD_PRESENT; > > @@ -4717,6 +4812,14 @@ int
Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
On 17/09/20 5:31 am, AKASHI Takahiro wrote: > Adrian, > > On Wed, Sep 16, 2020 at 01:00:35PM +0300, Adrian Hunter wrote: >> On 16/09/20 11:05 am, AKASHI Takahiro wrote: >>> Adrian, >>> >>> Your comments are scattered over various functions, and so >>> I would like to address them in separate replies. >>> >>> First, I'd like to discuss sdhci_[add|remove]_host(). >>> >>> On Fri, Aug 21, 2020 at 05:08:32PM +0300, Adrian Hunter wrote: On 10/07/20 2:10 pm, Ben Chuang wrote: > From: Ben Chuang > > In this commit, UHS-II related operations will be called via a function > pointer array, sdhci_uhs2_ops, in order to make UHS-II support as > a kernel module. > This array will be initialized only if CONFIG_MMC_SDHCI_UHS2 is enabled > and when the UHS-II module is loaded. Otherwise, all the functions > stay void. > > Signed-off-by: Ben Chuang > Signed-off-by: AKASHI Takahiro > --- >>> >>> (snip) >>> > if (intmask & (SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE)) { > u32 present = sdhci_readl(host, SDHCI_PRESENT_STATE) & > SDHCI_CARD_PRESENT; > @@ -4717,6 +4812,14 @@ int sdhci_setup_host(struct sdhci_host *host) > /* This may alter mmc->*_blk_* parameters */ > sdhci_allocate_bounce_buffer(host); > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > + host->version >= SDHCI_SPEC_400 && > + sdhci_uhs2_ops.add_host) { > + ret = sdhci_uhs2_ops.add_host(host, host->caps1); > + if (ret) > + goto unreg; > + } > + I think you should look at creating uhs2_add_host() instead > return 0; > > unreg: > @@ -4738,6 +4841,8 @@ void sdhci_cleanup_host(struct sdhci_host *host) > { > struct mmc_host *mmc = host->mmc; > > + /* FIXME: Do we have to do some cleanup for UHS2 here? */ > + > if (!IS_ERR(mmc->supply.vqmmc)) > regulator_disable(mmc->supply.vqmmc); > > @@ -4766,6 +4871,14 @@ int __sdhci_add_host(struct sdhci_host *host) > mmc->cqe_ops = NULL; > } > > + if ((mmc->caps & MMC_CAP_UHS2) && !host->v4_mode) { > + /* host doesn't want to enable UHS2 support */ > + mmc->caps &= ~MMC_CAP_UHS2; > + mmc->flags &= ~MMC_UHS2_SUPPORT; > + > + /* FIXME: Do we have to do some cleanup here? */ > + } > + > host->complete_wq = alloc_workqueue("sdhci", flags, 0); > if (!host->complete_wq) > return -ENOMEM; > @@ -4812,6 +4925,9 @@ int __sdhci_add_host(struct sdhci_host *host) > unled: > sdhci_led_unregister(host); > unirq: > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > + sdhci_uhs2_ops.remove_host) > + sdhci_uhs2_ops.remove_host(host, 0); > sdhci_do_reset(host, SDHCI_RESET_ALL); > sdhci_writel(host, 0, SDHCI_INT_ENABLE); > sdhci_writel(host, 0, SDHCI_SIGNAL_ENABLE); > @@ -4869,6 +4985,10 @@ void sdhci_remove_host(struct sdhci_host *host, > int dead) > > sdhci_led_unregister(host); > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > + sdhci_uhs2_ops.remove_host) > + sdhci_uhs2_ops.remove_host(host, dead); > + I think you should look at creating uhs2_remove_host() instead >>> >>> You suggest that we will have separate sdhci_uhs2_[add|remove]_host(), >>> but I don't think it's always convenient. >>> >>> UHS-II capable host will be set to call sdhci_uhs2_add_host() explicitly, >>> but we can't do that in case of pci and pltfm based drivers as they utilize >>> common helper functions, sdhci_pci_probe() and sdhci_pltfm_register(), >>> respectively. >> >> sdhci-pci has an add_host op >> >> sdhci_pltfm_init can be used instead of sdhci_pltfm_register >> >> >>> Therefore, we inevitably have to call sdhci_uhs2_add_host() there. >>> >>> If so, why should we distinguish sdhci_uhs2_add_host from >>> sdhci_uhs_add_host? >>> I don't see any good reason. >>> Moreover, as a result, there exists a mixed usage of sdhci_ interfaces >>> and sdhci_uhs2_ interfaces in sdhci-pci-core.c and sdhci-pltfm.c. >>> >>> It sounds odd to me. >> >> It is already done that way for cqhci. > > Okay, if it is your policy, I will follow that. > Then, I'm going to add > - remove_host field to struct sdhci_pci_fixes > - a controller specific helper function to each driver (only pci-gli for now) > even though it looks quite generic. If they seem generic then consider naming them sdhci_pci_uhs2_[add|remove]_host and putting them in sdhci-pci-core.c > > sdhci_gli_[add|remove]_host(struct sdhci_pci_slot *slot) > { > return sdhci_uhs2_[add|remove]_host(slot->host); > } > > # Or do you want to create a file like sdhci-uhs2-pci.c for those functions? No > > -Takahiro Akashi > >>> >>> -Takahiro Akashi >>> >>>
Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
Adrian, On Wed, Sep 16, 2020 at 01:00:35PM +0300, Adrian Hunter wrote: > On 16/09/20 11:05 am, AKASHI Takahiro wrote: > > Adrian, > > > > Your comments are scattered over various functions, and so > > I would like to address them in separate replies. > > > > First, I'd like to discuss sdhci_[add|remove]_host(). > > > > On Fri, Aug 21, 2020 at 05:08:32PM +0300, Adrian Hunter wrote: > >> On 10/07/20 2:10 pm, Ben Chuang wrote: > >>> From: Ben Chuang > >>> > >>> In this commit, UHS-II related operations will be called via a function > >>> pointer array, sdhci_uhs2_ops, in order to make UHS-II support as > >>> a kernel module. > >>> This array will be initialized only if CONFIG_MMC_SDHCI_UHS2 is enabled > >>> and when the UHS-II module is loaded. Otherwise, all the functions > >>> stay void. > >>> > >>> Signed-off-by: Ben Chuang > >>> Signed-off-by: AKASHI Takahiro > >>> --- > > > > (snip) > > > >>> if (intmask & (SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE)) { > >>> u32 present = sdhci_readl(host, SDHCI_PRESENT_STATE) & > >>> SDHCI_CARD_PRESENT; > >>> @@ -4717,6 +4812,14 @@ int sdhci_setup_host(struct sdhci_host *host) > >>> /* This may alter mmc->*_blk_* parameters */ > >>> sdhci_allocate_bounce_buffer(host); > >>> > >>> + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > >>> + host->version >= SDHCI_SPEC_400 && > >>> + sdhci_uhs2_ops.add_host) { > >>> + ret = sdhci_uhs2_ops.add_host(host, host->caps1); > >>> + if (ret) > >>> + goto unreg; > >>> + } > >>> + > >> > >> I think you should look at creating uhs2_add_host() instead > >> > >>> return 0; > >>> > >>> unreg: > >>> @@ -4738,6 +4841,8 @@ void sdhci_cleanup_host(struct sdhci_host *host) > >>> { > >>> struct mmc_host *mmc = host->mmc; > >>> > >>> + /* FIXME: Do we have to do some cleanup for UHS2 here? */ > >>> + > >>> if (!IS_ERR(mmc->supply.vqmmc)) > >>> regulator_disable(mmc->supply.vqmmc); > >>> > >>> @@ -4766,6 +4871,14 @@ int __sdhci_add_host(struct sdhci_host *host) > >>> mmc->cqe_ops = NULL; > >>> } > >>> > >>> + if ((mmc->caps & MMC_CAP_UHS2) && !host->v4_mode) { > >>> + /* host doesn't want to enable UHS2 support */ > >>> + mmc->caps &= ~MMC_CAP_UHS2; > >>> + mmc->flags &= ~MMC_UHS2_SUPPORT; > >>> + > >>> + /* FIXME: Do we have to do some cleanup here? */ > >>> + } > >>> + > >>> host->complete_wq = alloc_workqueue("sdhci", flags, 0); > >>> if (!host->complete_wq) > >>> return -ENOMEM; > >>> @@ -4812,6 +4925,9 @@ int __sdhci_add_host(struct sdhci_host *host) > >>> unled: > >>> sdhci_led_unregister(host); > >>> unirq: > >>> + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > >>> + sdhci_uhs2_ops.remove_host) > >>> + sdhci_uhs2_ops.remove_host(host, 0); > >>> sdhci_do_reset(host, SDHCI_RESET_ALL); > >>> sdhci_writel(host, 0, SDHCI_INT_ENABLE); > >>> sdhci_writel(host, 0, SDHCI_SIGNAL_ENABLE); > >>> @@ -4869,6 +4985,10 @@ void sdhci_remove_host(struct sdhci_host *host, > >>> int dead) > >>> > >>> sdhci_led_unregister(host); > >>> > >>> + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > >>> + sdhci_uhs2_ops.remove_host) > >>> + sdhci_uhs2_ops.remove_host(host, dead); > >>> + > >> > >> I think you should look at creating uhs2_remove_host() instead > > > > You suggest that we will have separate sdhci_uhs2_[add|remove]_host(), > > but I don't think it's always convenient. > > > > UHS-II capable host will be set to call sdhci_uhs2_add_host() explicitly, > > but we can't do that in case of pci and pltfm based drivers as they utilize > > common helper functions, sdhci_pci_probe() and sdhci_pltfm_register(), > > respectively. > > sdhci-pci has an add_host op > > sdhci_pltfm_init can be used instead of sdhci_pltfm_register > > > > Therefore, we inevitably have to call sdhci_uhs2_add_host() there. > > > > If so, why should we distinguish sdhci_uhs2_add_host from > > sdhci_uhs_add_host? > > I don't see any good reason. > > Moreover, as a result, there exists a mixed usage of sdhci_ interfaces > > and sdhci_uhs2_ interfaces in sdhci-pci-core.c and sdhci-pltfm.c. > > > > It sounds odd to me. > > It is already done that way for cqhci. Okay, if it is your policy, I will follow that. Then, I'm going to add - remove_host field to struct sdhci_pci_fixes - a controller specific helper function to each driver (only pci-gli for now) even though it looks quite generic. sdhci_gli_[add|remove]_host(struct sdhci_pci_slot *slot) { return sdhci_uhs2_[add|remove]_host(slot->host); } # Or do you want to create a file like sdhci-uhs2-pci.c for those functions? -Takahiro Akashi > > > > -Takahiro Akashi > > > > > >> > >>> if (!dead) > >>> sdhci_do_reset(host, SDHCI_RESET_ALL); > >>> > >>> > >> >
Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
On 16/09/20 11:05 am, AKASHI Takahiro wrote: > Adrian, > > Your comments are scattered over various functions, and so > I would like to address them in separate replies. > > First, I'd like to discuss sdhci_[add|remove]_host(). > > On Fri, Aug 21, 2020 at 05:08:32PM +0300, Adrian Hunter wrote: >> On 10/07/20 2:10 pm, Ben Chuang wrote: >>> From: Ben Chuang >>> >>> In this commit, UHS-II related operations will be called via a function >>> pointer array, sdhci_uhs2_ops, in order to make UHS-II support as >>> a kernel module. >>> This array will be initialized only if CONFIG_MMC_SDHCI_UHS2 is enabled >>> and when the UHS-II module is loaded. Otherwise, all the functions >>> stay void. >>> >>> Signed-off-by: Ben Chuang >>> Signed-off-by: AKASHI Takahiro >>> --- > > (snip) > >>> if (intmask & (SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE)) { >>> u32 present = sdhci_readl(host, SDHCI_PRESENT_STATE) & >>> SDHCI_CARD_PRESENT; >>> @@ -4717,6 +4812,14 @@ int sdhci_setup_host(struct sdhci_host *host) >>> /* This may alter mmc->*_blk_* parameters */ >>> sdhci_allocate_bounce_buffer(host); >>> >>> + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && >>> + host->version >= SDHCI_SPEC_400 && >>> + sdhci_uhs2_ops.add_host) { >>> + ret = sdhci_uhs2_ops.add_host(host, host->caps1); >>> + if (ret) >>> + goto unreg; >>> + } >>> + >> >> I think you should look at creating uhs2_add_host() instead >> >>> return 0; >>> >>> unreg: >>> @@ -4738,6 +4841,8 @@ void sdhci_cleanup_host(struct sdhci_host *host) >>> { >>> struct mmc_host *mmc = host->mmc; >>> >>> + /* FIXME: Do we have to do some cleanup for UHS2 here? */ >>> + >>> if (!IS_ERR(mmc->supply.vqmmc)) >>> regulator_disable(mmc->supply.vqmmc); >>> >>> @@ -4766,6 +4871,14 @@ int __sdhci_add_host(struct sdhci_host *host) >>> mmc->cqe_ops = NULL; >>> } >>> >>> + if ((mmc->caps & MMC_CAP_UHS2) && !host->v4_mode) { >>> + /* host doesn't want to enable UHS2 support */ >>> + mmc->caps &= ~MMC_CAP_UHS2; >>> + mmc->flags &= ~MMC_UHS2_SUPPORT; >>> + >>> + /* FIXME: Do we have to do some cleanup here? */ >>> + } >>> + >>> host->complete_wq = alloc_workqueue("sdhci", flags, 0); >>> if (!host->complete_wq) >>> return -ENOMEM; >>> @@ -4812,6 +4925,9 @@ int __sdhci_add_host(struct sdhci_host *host) >>> unled: >>> sdhci_led_unregister(host); >>> unirq: >>> + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && >>> + sdhci_uhs2_ops.remove_host) >>> + sdhci_uhs2_ops.remove_host(host, 0); >>> sdhci_do_reset(host, SDHCI_RESET_ALL); >>> sdhci_writel(host, 0, SDHCI_INT_ENABLE); >>> sdhci_writel(host, 0, SDHCI_SIGNAL_ENABLE); >>> @@ -4869,6 +4985,10 @@ void sdhci_remove_host(struct sdhci_host *host, int >>> dead) >>> >>> sdhci_led_unregister(host); >>> >>> + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && >>> + sdhci_uhs2_ops.remove_host) >>> + sdhci_uhs2_ops.remove_host(host, dead); >>> + >> >> I think you should look at creating uhs2_remove_host() instead > > You suggest that we will have separate sdhci_uhs2_[add|remove]_host(), > but I don't think it's always convenient. > > UHS-II capable host will be set to call sdhci_uhs2_add_host() explicitly, > but we can't do that in case of pci and pltfm based drivers as they utilize > common helper functions, sdhci_pci_probe() and sdhci_pltfm_register(), > respectively. sdhci-pci has an add_host op sdhci_pltfm_init can be used instead of sdhci_pltfm_register > Therefore, we inevitably have to call sdhci_uhs2_add_host() there. > > If so, why should we distinguish sdhci_uhs2_add_host from sdhci_uhs_add_host? > I don't see any good reason. > Moreover, as a result, there exists a mixed usage of sdhci_ interfaces > and sdhci_uhs2_ interfaces in sdhci-pci-core.c and sdhci-pltfm.c. > > It sounds odd to me. It is already done that way for cqhci. > > -Takahiro Akashi > > >> >>> if (!dead) >>> sdhci_do_reset(host, SDHCI_RESET_ALL); >>> >>> >>
Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
Adrian, Your comments are scattered over various functions, and so I would like to address them in separate replies. First, I'd like to discuss sdhci_[add|remove]_host(). On Fri, Aug 21, 2020 at 05:08:32PM +0300, Adrian Hunter wrote: > On 10/07/20 2:10 pm, Ben Chuang wrote: > > From: Ben Chuang > > > > In this commit, UHS-II related operations will be called via a function > > pointer array, sdhci_uhs2_ops, in order to make UHS-II support as > > a kernel module. > > This array will be initialized only if CONFIG_MMC_SDHCI_UHS2 is enabled > > and when the UHS-II module is loaded. Otherwise, all the functions > > stay void. > > > > Signed-off-by: Ben Chuang > > Signed-off-by: AKASHI Takahiro > > --- (snip) > > if (intmask & (SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE)) { > > u32 present = sdhci_readl(host, SDHCI_PRESENT_STATE) & > > SDHCI_CARD_PRESENT; > > @@ -4717,6 +4812,14 @@ int sdhci_setup_host(struct sdhci_host *host) > > /* This may alter mmc->*_blk_* parameters */ > > sdhci_allocate_bounce_buffer(host); > > > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > + host->version >= SDHCI_SPEC_400 && > > + sdhci_uhs2_ops.add_host) { > > + ret = sdhci_uhs2_ops.add_host(host, host->caps1); > > + if (ret) > > + goto unreg; > > + } > > + > > I think you should look at creating uhs2_add_host() instead > > > return 0; > > > > unreg: > > @@ -4738,6 +4841,8 @@ void sdhci_cleanup_host(struct sdhci_host *host) > > { > > struct mmc_host *mmc = host->mmc; > > > > + /* FIXME: Do we have to do some cleanup for UHS2 here? */ > > + > > if (!IS_ERR(mmc->supply.vqmmc)) > > regulator_disable(mmc->supply.vqmmc); > > > > @@ -4766,6 +4871,14 @@ int __sdhci_add_host(struct sdhci_host *host) > > mmc->cqe_ops = NULL; > > } > > > > + if ((mmc->caps & MMC_CAP_UHS2) && !host->v4_mode) { > > + /* host doesn't want to enable UHS2 support */ > > + mmc->caps &= ~MMC_CAP_UHS2; > > + mmc->flags &= ~MMC_UHS2_SUPPORT; > > + > > + /* FIXME: Do we have to do some cleanup here? */ > > + } > > + > > host->complete_wq = alloc_workqueue("sdhci", flags, 0); > > if (!host->complete_wq) > > return -ENOMEM; > > @@ -4812,6 +4925,9 @@ int __sdhci_add_host(struct sdhci_host *host) > > unled: > > sdhci_led_unregister(host); > > unirq: > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > + sdhci_uhs2_ops.remove_host) > > + sdhci_uhs2_ops.remove_host(host, 0); > > sdhci_do_reset(host, SDHCI_RESET_ALL); > > sdhci_writel(host, 0, SDHCI_INT_ENABLE); > > sdhci_writel(host, 0, SDHCI_SIGNAL_ENABLE); > > @@ -4869,6 +4985,10 @@ void sdhci_remove_host(struct sdhci_host *host, int > > dead) > > > > sdhci_led_unregister(host); > > > > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > > + sdhci_uhs2_ops.remove_host) > > + sdhci_uhs2_ops.remove_host(host, dead); > > + > > I think you should look at creating uhs2_remove_host() instead You suggest that we will have separate sdhci_uhs2_[add|remove]_host(), but I don't think it's always convenient. UHS-II capable host will be set to call sdhci_uhs2_add_host() explicitly, but we can't do that in case of pci and pltfm based drivers as they utilize common helper functions, sdhci_pci_probe() and sdhci_pltfm_register(), respectively. Therefore, we inevitably have to call sdhci_uhs2_add_host() there. If so, why should we distinguish sdhci_uhs2_add_host from sdhci_uhs_add_host? I don't see any good reason. Moreover, as a result, there exists a mixed usage of sdhci_ interfaces and sdhci_uhs2_ interfaces in sdhci-pci-core.c and sdhci-pltfm.c. It sounds odd to me. -Takahiro Akashi > > > if (!dead) > > sdhci_do_reset(host, SDHCI_RESET_ALL); > > > > >
Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
On 10/07/20 2:10 pm, Ben Chuang wrote: > From: Ben Chuang > > In this commit, UHS-II related operations will be called via a function > pointer array, sdhci_uhs2_ops, in order to make UHS-II support as > a kernel module. > This array will be initialized only if CONFIG_MMC_SDHCI_UHS2 is enabled > and when the UHS-II module is loaded. Otherwise, all the functions > stay void. > > Signed-off-by: Ben Chuang > Signed-off-by: AKASHI Takahiro > --- > drivers/mmc/host/sdhci.c | 152 ++- > 1 file changed, 136 insertions(+), 16 deletions(-) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index aaf41954511a..5511649946b9 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -32,8 +32,12 @@ > #include > #include > #include > +#include Ideally, we wouldn't need to use any UHS-II definitions in sdhci.c > +#include And never PCI. Please remove > > #include "sdhci.h" > +#include "sdhci-uhs2.h" sdhci-uhs2.h must not be included because the point of having it is to separate UHS-II from SD mode, so please remove > +#include "sdhci-pci.h" Also this include must be removed > > #define DRIVER_NAME "sdhci" > > @@ -45,6 +49,11 @@ > > #define MAX_TUNING_LOOP 40 > > +#if IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) > +struct sdhci_uhs2_ops sdhci_uhs2_ops; > +EXPORT_SYMBOL_GPL(sdhci_uhs2_ops); > +#endif As I mentioned in a previous patch, please add to sdhci_ops instead. > + > static unsigned int debug_quirks = 0; > static unsigned int debug_quirks2; > > @@ -1041,8 +1050,11 @@ EXPORT_SYMBOL_GPL(sdhci_set_data_timeout_irq); > > void __sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd) > { > + u8 count; > + > bool too_big = false; > - u8 count = sdhci_calc_timeout(host, cmd, _big); > + > + count = sdhci_calc_timeout(host, cmd, _big); Last 2 chunks do nothing. Please remove > > if (too_big && > host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT) { > @@ -1053,6 +1065,11 @@ void __sdhci_set_timeout(struct sdhci_host *host, > struct mmc_command *cmd) > } > > sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL); > + > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > + host->mmc->flags & MMC_UHS2_SUPPORT && > + sdhci_uhs2_ops.set_timeout) > + sdhci_uhs2_ops.set_timeout(host); This is the kind of thing I want to avoid. We already have a ->set_timeout() callback. I would suggest creating something like: static void __uhs2_set_timeout(struct sdhci_host *host, struct mmc_command *cmd) { } void uhs2_set_timeout(struct sdhci_host *host, struct mmc_command *cmd) { if (host->uhs2_mode) __uhs2_set_timeout(host, cmd); else __sdhci_set_timeout(host, cmd); } EXPORT_SYMBOL_GPL(uhs2_set_timeout); Then uhs2 drivers need to set: .set_timeout = uhs2_set_timeout, > } > EXPORT_SYMBOL_GPL(__sdhci_set_timeout); > > @@ -1191,7 +1208,14 @@ static void sdhci_prepare_data(struct sdhci_host > *host, struct mmc_command *cmd) > > sdhci_set_transfer_irqs(host); > > - sdhci_set_block_info(host, data); > + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && > + host->mmc->flags & MMC_UHS2_SUPPORT && > + host->mmc->flags & MMC_UHS2_INITIALIZED) { > + sdhci_writew(host, data->blksz, SDHCI_UHS2_BLOCK_SIZE); > + sdhci_writew(host, data->blocks, SDHCI_UHS2_BLOCK_COUNT); > + } else { > + sdhci_set_block_info(host, data); > + } Again, this is what I want to avoid. I would like to have 3 kinds of functions: - SD mode only - UHS-II only - SD functions with no UHS-II code, that can also be used by UHS-II i.e. I don't want to mix UHS-II code and SD mode code in the same function. I think sdhci-uhs2.c should provide a request function and a send_command function. I would start by removing everything you may not need, and then see if you have any problems. e.g. void uhs2_request(struct mmc_host *mmc, struct mmc_request *mrq) { struct sdhci_host *host = mmc_priv(mmc); struct mmc_command *cmd; unsigned long flags; if (!host->uhs2_mode) { sdhci_request(mmc, mrq); return; } spin_lock_irqsave(>lock, flags); uhs2_send_command(host, cmd); spin_unlock_irqrestore(>lock, flags); } EXPORT_SYMBOL_GPL(uhs2_request); For sdhci_prepare_data(), I would factor out the dma part, so static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd) { struct mmc_data *data = cmd->data; sdhci_initialize_data(host, data); sdhci_prepare_dma(host, data); sdhci_set_block_info(host, data); } The you could export sdhci_initialize_data() and sdhci_prepare_dma() for uhs2. > } > > #if IS_ENABLED(CONFIG_MMC_SDHCI_EXTERNAL_DMA) > @@ -1439,6 +1463,13 @@ static void
[RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
From: Ben Chuang In this commit, UHS-II related operations will be called via a function pointer array, sdhci_uhs2_ops, in order to make UHS-II support as a kernel module. This array will be initialized only if CONFIG_MMC_SDHCI_UHS2 is enabled and when the UHS-II module is loaded. Otherwise, all the functions stay void. Signed-off-by: Ben Chuang Signed-off-by: AKASHI Takahiro --- drivers/mmc/host/sdhci.c | 152 ++- 1 file changed, 136 insertions(+), 16 deletions(-) diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index aaf41954511a..5511649946b9 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -32,8 +32,12 @@ #include #include #include +#include +#include #include "sdhci.h" +#include "sdhci-uhs2.h" +#include "sdhci-pci.h" #define DRIVER_NAME "sdhci" @@ -45,6 +49,11 @@ #define MAX_TUNING_LOOP 40 +#if IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) +struct sdhci_uhs2_ops sdhci_uhs2_ops; +EXPORT_SYMBOL_GPL(sdhci_uhs2_ops); +#endif + static unsigned int debug_quirks = 0; static unsigned int debug_quirks2; @@ -1041,8 +1050,11 @@ EXPORT_SYMBOL_GPL(sdhci_set_data_timeout_irq); void __sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd) { + u8 count; + bool too_big = false; - u8 count = sdhci_calc_timeout(host, cmd, _big); + + count = sdhci_calc_timeout(host, cmd, _big); if (too_big && host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT) { @@ -1053,6 +1065,11 @@ void __sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd) } sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL); + + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && + host->mmc->flags & MMC_UHS2_SUPPORT && + sdhci_uhs2_ops.set_timeout) + sdhci_uhs2_ops.set_timeout(host); } EXPORT_SYMBOL_GPL(__sdhci_set_timeout); @@ -1191,7 +1208,14 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd) sdhci_set_transfer_irqs(host); - sdhci_set_block_info(host, data); + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && + host->mmc->flags & MMC_UHS2_SUPPORT && + host->mmc->flags & MMC_UHS2_INITIALIZED) { + sdhci_writew(host, data->blksz, SDHCI_UHS2_BLOCK_SIZE); + sdhci_writew(host, data->blocks, SDHCI_UHS2_BLOCK_COUNT); + } else { + sdhci_set_block_info(host, data); + } } #if IS_ENABLED(CONFIG_MMC_SDHCI_EXTERNAL_DMA) @@ -1439,6 +1463,13 @@ static void sdhci_set_transfer_mode(struct sdhci_host *host, u16 mode = 0; struct mmc_data *data = cmd->data; + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && + host->mmc->flags & MMC_UHS2_SUPPORT) { + if (sdhci_uhs2_ops.set_transfer_mode) + sdhci_uhs2_ops.set_transfer_mode(host, cmd); + return; + } + if (data == NULL) { if (host->quirks2 & SDHCI_QUIRK2_CLEAR_TRANSFERMODE_REG_BEFORE_CMD) { @@ -1570,6 +1601,12 @@ static void __sdhci_finish_data(struct sdhci_host *host, bool sw_data_timeout) else data->bytes_xfered = data->blksz * data->blocks; + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && + host->mmc->flags & MMC_UHS2_INITIALIZED) { + __sdhci_finish_mrq(host, data->mrq); + return; + } + /* * Need to send CMD12 if - * a) open-ended multiblock transfer not using auto CMD12 (no CMD23) @@ -1654,7 +1691,8 @@ static bool sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) sdhci_prepare_data(host, cmd); } - sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT); + if (!IS_ENABLED(CONFIG_MMC_SDHCI_UHS2)) + sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT); sdhci_set_transfer_mode(host, cmd); @@ -1699,6 +1737,17 @@ static bool sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) if (host->use_external_dma) sdhci_external_dma_pre_transfer(host, cmd); + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && + (host->mmc->flags & MMC_UHS2_SUPPORT)) { + if (sdhci_uhs2_ops.send_command) + sdhci_uhs2_ops.send_command(host, cmd); + + return true; + } + + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2)) + sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT); + sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND); return true; @@ -1780,13 +1829,20 @@ static void sdhci_finish_command(struct sdhci_host *host) { struct mmc_command *cmd = host->cmd; - host->cmd = NULL; + if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) && + host->mmc->flags & MMC_UHS2_SUPPORT) { + if (sdhci_uhs2_ops.finish_command) +