Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations

From: AKASHI Takahiro
Date: Thu Sep 24 2020 - 05:35:48 EST

Next message: Robin Murphy: "Re: arm-smmu 5000000.iommu: Cannot accommodate DMA offset for IOMMU page tables"
Previous message: Alejandro Colomar: "Re: [PATCH 12/24] getgrent_r.3: Use sizeof() to get buffer size (instead of hardcoding macro name)"
Next in thread: Adrian Hunter: "Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxxxxxxxxxx>
> >
> > 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);
> >
> >
>

Next message: Robin Murphy: "Re: arm-smmu 5000000.iommu: Cannot accommodate DMA offset for IOMMU page tables"
Previous message: Alejandro Colomar: "Re: [PATCH 12/24] getgrent_r.3: Use sizeof() to get buffer size (instead of hardcoding macro name)"
Next in thread: Adrian Hunter: "Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]