| Message ID | 20200710111054.29562-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:10 pm, Ben Chuang wrote: > From: Ben Chuang <ben.chuang@genesyslogic.com.tw> > > 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 <ben.chuang@genesyslogic.com.tw> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > 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 <linux/mmc/card.h> > #include <linux/mmc/sdio.h> > #include <linux/mmc/slot-gpio.h> > +#include <linux/mmc/uhs2.h> Ideally, we wouldn't need to use any UHS-II definitions in sdhci.c > +#include <linux/pci.h> 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, &too_big); > + > + count = sdhci_calc_timeout(host, cmd, &too_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) { <whatever> } 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(&host->lock, flags); uhs2_send_command(host, cmd); spin_unlock_irqrestore(&host->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 (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) && > + (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); Not needed when instead you provide uhs2_send_command() > + > 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) > + sdhci_uhs2_ops.finish_command(host); > + } else { > + host->cmd = NULL; > > - if (cmd->flags & MMC_RSP_PRESENT) { > - if (cmd->flags & MMC_RSP_136) { > - sdhci_read_rsp_136(host, cmd); > - } else { > - cmd->resp[0] = sdhci_readl(host, SDHCI_RESPONSE); > + if (cmd->flags & MMC_RSP_PRESENT) { > + if (cmd->flags & MMC_RSP_136) { > + sdhci_read_rsp_136(host, cmd); > + } else { > + cmd->resp[0] = sdhci_readl(host, > + SDHCI_RESPONSE); > + } At least to start with, I think it would be better to handle UHS-II cmd and data interrupts completely in sdhci-uhs2.c > } > } > > @@ -1809,6 +1865,7 @@ static void sdhci_finish_command(struct sdhci_host *host) > } else if (!(host->quirks & SDHCI_QUIRK_NO_BUSY_IRQ) && > cmd == host->data_cmd) { > /* Command complete before busy is ended */ > + host->cmd = NULL; host->cmd is set to NULL at the start of this function, so this is not needed. > return; > } > } > @@ -1828,6 +1885,8 @@ static void sdhci_finish_command(struct sdhci_host *host) > if (!cmd->data) > __sdhci_finish_mrq(host, cmd->mrq); > } > + > + host->cmd = NULL; host->cmd is set to NULL at the start of this function, so this is not needed. > } > > static u16 sdhci_get_preset_value(struct sdhci_host *host) > @@ -1855,6 +1914,11 @@ static u16 sdhci_get_preset_value(struct sdhci_host *host) > case MMC_TIMING_MMC_HS400: > preset = sdhci_readw(host, SDHCI_PRESET_FOR_HS400); > break; > +#if IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) Shouldn't need conditional compilation for this > + case MMC_TIMING_UHS2: > + preset = sdhci_readw(host, SDHCI_PRESET_FOR_UHS2); > + break; > +#endif > default: > pr_warn("%s: Invalid UHS-I mode selected\n", > mmc_hostname(host->mmc)); > @@ -2095,7 +2159,6 @@ void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, > sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL); > > pwr |= SDHCI_POWER_ON; > - No white space changes mixed in please > sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL); > > if (host->quirks2 & SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON) > @@ -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() > 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. > 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 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 > if (!dead) > sdhci_do_reset(host, SDHCI_RESET_ALL); > >
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 <ben.chuang@genesyslogic.com.tw> > > > > 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 <ben.chuang@genesyslogic.com.tw> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- (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); > > > > >
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 <ben.chuang@genesyslogic.com.tw> >>> >>> 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 <ben.chuang@genesyslogic.com.tw> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>> --- > > (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); >>> >>> >>
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 <ben.chuang@genesyslogic.com.tw> > >>> > >>> 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 <ben.chuang@genesyslogic.com.tw> > >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > >>> --- > > > > (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); > >>> > >>> > >> >
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 <ben.chuang@genesyslogic.com.tw> >>>>> >>>>> 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 <ben.chuang@genesyslogic.com.tw> >>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>>>> --- >>> >>> (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 >>> >>> >>>> >>>>> if (!dead) >>>>> sdhci_do_reset(host, SDHCI_RESET_ALL); >>>>> >>>>> >>>> >>
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 <ben.chuang@genesyslogic.com.tw> > > > > 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 <ben.chuang@genesyslogic.com.tw> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > 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 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 > > > if (!dead) > > sdhci_do_reset(host, SDHCI_RESET_ALL); > > > > >
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 <ben.chuang@genesyslogic.com.tw> > >>>>> > >>>>> 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 <ben.chuang@genesyslogic.com.tw> > >>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > >>>>> --- > >>> > >>> (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 So you don't mind that UHS-I and UHS-II code are mixed in sdhci-pci-core.c, do you? -Takahiro Akashi > > > > 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 > >>> > >>> > >>>> > >>>>> if (!dead) > >>>>> sdhci_do_reset(host, SDHCI_RESET_ALL); > >>>>> > >>>>> > >>>> > >> >
Hi Takahiro, On Thu, Sep 17, 2020 at 1:12 PM AKASHI Takahiro <takahiro.akashi@linaro.org> 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 <ben.chuang@genesyslogic.com.tw> > > > > > > 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 <ben.chuang@genesyslogic.com.tw> > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > --- > > > 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) && > > > + 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 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 > > > > > if (!dead) > > > sdhci_do_reset(host, SDHCI_RESET_ALL); > > > > > > > >
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 > <takahiro.akashi@linaro.org> 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 <ben.chuang@genesyslogic.com.tw> > > > > > > > > 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 <ben.chuang@genesyslogic.com.tw> > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > --- > > > > 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, 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 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 > > > > > > > if (!dead) > > > > sdhci_do_reset(host, SDHCI_RESET_ALL); > > > > > > > > > > >
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 <ben.chuang@genesyslogic.com.tw> > > > > 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 <ben.chuang@genesyslogic.com.tw> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> (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
On Fri, Sep 18, 2020 at 9:15 AM AKASHI Takahiro <takahiro.akashi@linaro.org> 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 > > <takahiro.akashi@linaro.org> 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 <ben.chuang@genesyslogic.com.tw> > > > > > > > > > > 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 <ben.chuang@genesyslogic.com.tw> > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > --- > > > > > 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 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. Do UHS2_SW_RESET_FULL in sdhci_uhs2_remove_host() and then do SDHCI_RESET_ALL in sdhci_remove_host() is ok. > > -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, 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 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 > > > > > > > > > if (!dead) > > > > > sdhci_do_reset(host, SDHCI_RESET_ALL); > > > > > > > > > > > > > >
On Fri, Sep 18, 2020 at 2:38 PM AKASHI Takahiro <takahiro.akashi@linaro.org> 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 <ben.chuang@genesyslogic.com.tw> > > > > > > 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 <ben.chuang@genesyslogic.com.tw> > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > (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
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 <ben.chuang@genesyslogic.com.tw> > > > > 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(&host->lock, flags); > uhs2_send_command(host, cmd); > spin_unlock_irqrestore(&host->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) && > > + (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); > > Not needed when instead you provide uhs2_send_command() > > > + > > 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) > > + sdhci_uhs2_ops.finish_command(host); > > + } else { > > + host->cmd = NULL; > > > > - if (cmd->flags & MMC_RSP_PRESENT) { > > - if (cmd->flags & MMC_RSP_136) { > > - sdhci_read_rsp_136(host, cmd); > > - } else { > > - cmd->resp[0] = sdhci_readl(host, SDHCI_RESPONSE); > > + if (cmd->flags & MMC_RSP_PRESENT) { > > + if (cmd->flags & MMC_RSP_136) { > > + sdhci_read_rsp_136(host, cmd); > > + } else { > > + cmd->resp[0] = sdhci_readl(host, > > + SDHCI_RESPONSE); > > + } > > At least to start with, I think it would be better to handle UHS-II cmd and data interrupts completely in sdhci-uhs2.c > > > } > > } > > > > @@ -1809,6 +1865,7 @@ static void sdhci_finish_command(struct sdhci_host *host) > > } else if (!(host->quirks & SDHCI_QUIRK_NO_BUSY_IRQ) && > > cmd == host->data_cmd) { > > /* Command complete before busy is ended */ > > + host->cmd = NULL; > > host->cmd is set to NULL at the start of this function, so this is not needed. > > > return; > > } > > } > > @@ -1828,6 +1885,8 @@ static void sdhci_finish_command(struct sdhci_host *host) > > if (!cmd->data) > > __sdhci_finish_mrq(host, cmd->mrq); > > } > > + > > + host->cmd = NULL; > > host->cmd is set to NULL at the start of this function, so this is not needed. > > > } > > > > static u16 sdhci_get_preset_value(struct sdhci_host *host) > > @@ -1855,6 +1914,11 @@ static u16 sdhci_get_preset_value(struct sdhci_host *host) > > case MMC_TIMING_MMC_HS400: > > preset = sdhci_readw(host, SDHCI_PRESET_FOR_HS400); > > break; > > +#if IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) > > Shouldn't need conditional compilation for this > > > + case MMC_TIMING_UHS2: > > + preset = sdhci_readw(host, SDHCI_PRESET_FOR_UHS2); > > + break; > > +#endif > > default: > > pr_warn("%s: Invalid UHS-I mode selected\n", > > mmc_hostname(host->mmc)); > > @@ -2095,7 +2159,6 @@ void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, > > sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL); > > > > pwr |= SDHCI_POWER_ON; > > - > > No white space changes mixed in please > > > sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL); > > > > if (host->quirks2 & SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON) > > @@ -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() > > > 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. > > > 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 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 > > > if (!dead) > > sdhci_do_reset(host, SDHCI_RESET_ALL); > > > > >
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 > <takahiro.akashi@linaro.org> 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 > > > <takahiro.akashi@linaro.org> 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 <ben.chuang@genesyslogic.com.tw> > > > > > > > > > > > > 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 <ben.chuang@genesyslogic.com.tw> > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > > --- > > > > > > 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 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. > > Do UHS2_SW_RESET_FULL in sdhci_uhs2_remove_host() and then do > SDHCI_RESET_ALL in sdhci_remove_host() is ok. > > > > > > -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, 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 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 > > > > > > > > > > > if (!dead) > > > > > > sdhci_do_reset(host, SDHCI_RESET_ALL); > > > > > > > > > > > > > > > > >
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 > <takahiro.akashi@linaro.org> 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 <ben.chuang@genesyslogic.com.tw> > > > > > > > > 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 <ben.chuang@genesyslogic.com.tw> > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > (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
Takahiro, On Thu, Sep 24, 2020 at 5:57 PM AKASHI Takahiro <takahiro.akashi@linaro.org> 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 > > <takahiro.akashi@linaro.org> 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 <ben.chuang@genesyslogic.com.tw> > > > > > > > > > > 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 <ben.chuang@genesyslogic.com.tw> > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > > (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
Takahiro, On Thu, Sep 24, 2020 at 5:46 PM AKASHI Takahiro <takahiro.akashi@linaro.org> 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 > > <takahiro.akashi@linaro.org> 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 > > > > <takahiro.akashi@linaro.org> 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 <ben.chuang@genesyslogic.com.tw> > > > > > > > > > > > > > > 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 <ben.chuang@genesyslogic.com.tw> > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > > > --- > > > > > > > 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. No, sdhci_gl9755_reset() is only for GL9755. > > -Takahiro Akashi > > > > 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. > > > > Do UHS2_SW_RESET_FULL in sdhci_uhs2_remove_host() and then do > > SDHCI_RESET_ALL in sdhci_remove_host() is ok. > > > > > > > > > > -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, 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 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 > > > > > > > > > > > > > if (!dead) > > > > > > > sdhci_do_reset(host, SDHCI_RESET_ALL); > > > > > > > > > > > > > > > > > > > >
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 <ben.chuang@genesyslogic.com.tw> >>> >>> 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(&host->lock, flags); >> uhs2_send_command(host, cmd); >> spin_unlock_irqrestore(&host->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.
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 <linux/mmc/card.h> #include <linux/mmc/sdio.h> #include <linux/mmc/slot-gpio.h> +#include <linux/mmc/uhs2.h> +#include <linux/pci.h> #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, &too_big); + + count = sdhci_calc_timeout(host, cmd, &too_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) + sdhci_uhs2_ops.finish_command(host); + } else { + host->cmd = NULL; - if (cmd->flags & MMC_RSP_PRESENT) { - if (cmd->flags & MMC_RSP_136) { - sdhci_read_rsp_136(host, cmd); - } else { - cmd->resp[0] = sdhci_readl(host, SDHCI_RESPONSE); + if (cmd->flags & MMC_RSP_PRESENT) { + if (cmd->flags & MMC_RSP_136) { + sdhci_read_rsp_136(host, cmd); + } else { + cmd->resp[0] = sdhci_readl(host, + SDHCI_RESPONSE); + } } } @@ -1809,6 +1865,7 @@ static void sdhci_finish_command(struct sdhci_host *host) } else if (!(host->quirks & SDHCI_QUIRK_NO_BUSY_IRQ) && cmd == host->data_cmd) { /* Command complete before busy is ended */ + host->cmd = NULL; return; } } @@ -1828,6 +1885,8 @@ static void sdhci_finish_command(struct sdhci_host *host) if (!cmd->data) __sdhci_finish_mrq(host, cmd->mrq); } + + host->cmd = NULL; } static u16 sdhci_get_preset_value(struct sdhci_host *host) @@ -1855,6 +1914,11 @@ static u16 sdhci_get_preset_value(struct sdhci_host *host) case MMC_TIMING_MMC_HS400: preset = sdhci_readw(host, SDHCI_PRESET_FOR_HS400); break; +#if IS_ENABLED(CONFIG_MMC_SDHCI_UHS2) + case MMC_TIMING_UHS2: + preset = sdhci_readw(host, SDHCI_PRESET_FOR_UHS2); + break; +#endif default: pr_warn("%s: Invalid UHS-I mode selected\n", mmc_hostname(host->mmc)); @@ -2095,7 +2159,6 @@ void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL); pwr |= SDHCI_POWER_ON; - sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL); if (host->quirks2 & SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON) @@ -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; + } + } + 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); + } 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); + } + 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; + } + 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); + if (!dead) sdhci_do_reset(host, SDHCI_RESET_ALL);