* [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
@ 2020-07-10 11:10 Ben Chuang
2020-08-21 14:08 ` Adrian Hunter
0 siblings, 1 reply; 19+ messages in thread
From: Ben Chuang @ 2020-07-10 11:10 UTC (permalink / raw)
To: adrian.hunter, ulf.hansson
Cc: linux-mmc, linux-kernel, ben.chuang, takahiro.akashi, greg.tu,
Ben Chuang
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>
+#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);
--
2.27.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
2020-07-10 11:10 [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations Ben Chuang
@ 2020-08-21 14:08 ` Adrian Hunter
2020-09-16 8:05 ` AKASHI Takahiro
` (3 more replies)
0 siblings, 4 replies; 19+ messages in thread
From: Adrian Hunter @ 2020-08-21 14:08 UTC (permalink / raw)
To: Ben Chuang, ulf.hansson
Cc: linux-mmc, linux-kernel, ben.chuang, takahiro.akashi, greg.tu
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);
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
2020-08-21 14:08 ` Adrian Hunter
@ 2020-09-16 8:05 ` AKASHI Takahiro
2020-09-16 10:00 ` Adrian Hunter
2020-09-17 5:12 ` AKASHI Takahiro
` (2 subsequent siblings)
3 siblings, 1 reply; 19+ messages in thread
From: AKASHI Takahiro @ 2020-09-16 8:05 UTC (permalink / raw)
To: Adrian Hunter
Cc: Ben Chuang, ulf.hansson, linux-mmc, linux-kernel, ben.chuang, greg.tu
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);
> >
> >
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
2020-09-16 8:05 ` AKASHI Takahiro
@ 2020-09-16 10:00 ` Adrian Hunter
2020-09-17 2:31 ` AKASHI Takahiro
0 siblings, 1 reply; 19+ messages in thread
From: Adrian Hunter @ 2020-09-16 10:00 UTC (permalink / raw)
To: AKASHI Takahiro, Ben Chuang, ulf.hansson, linux-mmc,
linux-kernel, ben.chuang, greg.tu
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);
>>>
>>>
>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
2020-09-16 10:00 ` Adrian Hunter
@ 2020-09-17 2:31 ` AKASHI Takahiro
2020-09-17 4:52 ` Adrian Hunter
0 siblings, 1 reply; 19+ messages in thread
From: AKASHI Takahiro @ 2020-09-17 2:31 UTC (permalink / raw)
To: Adrian Hunter
Cc: Ben Chuang, ulf.hansson, linux-mmc, linux-kernel, ben.chuang, greg.tu
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);
> >>>
> >>>
> >>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
2020-09-17 2:31 ` AKASHI Takahiro
@ 2020-09-17 4:52 ` Adrian Hunter
2020-09-17 5:16 ` AKASHI Takahiro
0 siblings, 1 reply; 19+ messages in thread
From: Adrian Hunter @ 2020-09-17 4:52 UTC (permalink / raw)
To: AKASHI Takahiro, Ben Chuang, ulf.hansson, linux-mmc,
linux-kernel, ben.chuang, greg.tu
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);
>>>>>
>>>>>
>>>>
>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
2020-08-21 14:08 ` Adrian Hunter
2020-09-16 8:05 ` AKASHI Takahiro
@ 2020-09-17 5:12 ` AKASHI Takahiro
2020-09-17 10:12 ` Ben Chuang
2020-09-18 6:38 ` AKASHI Takahiro
2020-09-24 9:35 ` AKASHI Takahiro
3 siblings, 1 reply; 19+ messages in thread
From: AKASHI Takahiro @ 2020-09-17 5:12 UTC (permalink / raw)
To: Adrian Hunter
Cc: Ben Chuang, ulf.hansson, linux-mmc, linux-kernel, ben.chuang, greg.tu
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);
> >
> >
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
2020-09-17 4:52 ` Adrian Hunter
@ 2020-09-17 5:16 ` AKASHI Takahiro
0 siblings, 0 replies; 19+ messages in thread
From: AKASHI Takahiro @ 2020-09-17 5:16 UTC (permalink / raw)
To: Adrian Hunter
Cc: Ben Chuang, ulf.hansson, linux-mmc, linux-kernel, ben.chuang, greg.tu
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);
> >>>>>
> >>>>>
> >>>>
> >>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
2020-09-17 5:12 ` AKASHI Takahiro
@ 2020-09-17 10:12 ` Ben Chuang
2020-09-18 1:15 ` AKASHI Takahiro
0 siblings, 1 reply; 19+ messages in thread
From: Ben Chuang @ 2020-09-17 10:12 UTC (permalink / raw)
To: AKASHI Takahiro, Adrian Hunter, Ben Chuang, Ulf Hansson,
linux-mmc, Linux Kernel Mailing List, Ben Chuang, greg.tu
Cc: Renius.Chen
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);
> > >
> > >
> >
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
2020-09-17 10:12 ` Ben Chuang
@ 2020-09-18 1:15 ` AKASHI Takahiro
2020-09-18 10:27 ` Ben Chuang
0 siblings, 1 reply; 19+ messages in thread
From: AKASHI Takahiro @ 2020-09-18 1:15 UTC (permalink / raw)
To: Ben Chuang
Cc: Adrian Hunter, Ulf Hansson, linux-mmc, Linux Kernel Mailing List,
Ben Chuang, greg.tu, Renius.Chen
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);
> > > >
> > > >
> > >
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
2020-08-21 14:08 ` Adrian Hunter
2020-09-16 8:05 ` AKASHI Takahiro
2020-09-17 5:12 ` AKASHI Takahiro
@ 2020-09-18 6:38 ` AKASHI Takahiro
2020-09-18 10:50 ` Ben Chuang
2020-09-24 9:35 ` AKASHI Takahiro
3 siblings, 1 reply; 19+ messages in thread
From: AKASHI Takahiro @ 2020-09-18 6:38 UTC (permalink / raw)
To: Adrian Hunter
Cc: Ben Chuang, ulf.hansson, linux-mmc, linux-kernel, ben.chuang, greg.tu
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
2020-09-18 1:15 ` AKASHI Takahiro
@ 2020-09-18 10:27 ` Ben Chuang
2020-09-24 9:46 ` AKASHI Takahiro
0 siblings, 1 reply; 19+ messages in thread
From: Ben Chuang @ 2020-09-18 10:27 UTC (permalink / raw)
To: AKASHI Takahiro, Ben Chuang, Adrian Hunter, Ulf Hansson,
linux-mmc, Linux Kernel Mailing List, Ben Chuang, greg.tu,
Renius.Chen
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);
> > > > >
> > > > >
> > > >
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
2020-09-18 6:38 ` AKASHI Takahiro
@ 2020-09-18 10:50 ` Ben Chuang
2020-09-24 9:57 ` AKASHI Takahiro
0 siblings, 1 reply; 19+ messages in thread
From: Ben Chuang @ 2020-09-18 10:50 UTC (permalink / raw)
To: AKASHI Takahiro, Adrian Hunter, Ben Chuang, Ulf Hansson,
linux-mmc, Linux Kernel Mailing List, Ben Chuang, greg.tu,
Renius.Chen
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
2020-08-21 14:08 ` Adrian Hunter
` (2 preceding siblings ...)
2020-09-18 6:38 ` AKASHI Takahiro
@ 2020-09-24 9:35 ` AKASHI Takahiro
2020-09-25 9:02 ` Adrian Hunter
3 siblings, 1 reply; 19+ messages in thread
From: AKASHI Takahiro @ 2020-09-24 9:35 UTC (permalink / raw)
To: Adrian Hunter
Cc: Ben Chuang, ulf.hansson, linux-mmc, linux-kernel, ben.chuang, greg.tu
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);
> >
> >
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
2020-09-18 10:27 ` Ben Chuang
@ 2020-09-24 9:46 ` AKASHI Takahiro
2020-09-25 7:59 ` Ben Chuang
0 siblings, 1 reply; 19+ messages in thread
From: AKASHI Takahiro @ 2020-09-24 9:46 UTC (permalink / raw)
To: Ben Chuang
Cc: Adrian Hunter, Ulf Hansson, linux-mmc, Linux Kernel Mailing List,
Ben Chuang, greg.tu, Renius.Chen
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);
> > > > > >
> > > > > >
> > > > >
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
2020-09-18 10:50 ` Ben Chuang
@ 2020-09-24 9:57 ` AKASHI Takahiro
2020-09-25 7:55 ` Ben Chuang
0 siblings, 1 reply; 19+ messages in thread
From: AKASHI Takahiro @ 2020-09-24 9:57 UTC (permalink / raw)
To: Ben Chuang
Cc: Adrian Hunter, Ulf Hansson, linux-mmc, Linux Kernel Mailing List,
Ben Chuang, greg.tu, Renius.Chen
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
2020-09-24 9:57 ` AKASHI Takahiro
@ 2020-09-25 7:55 ` Ben Chuang
0 siblings, 0 replies; 19+ messages in thread
From: Ben Chuang @ 2020-09-25 7:55 UTC (permalink / raw)
To: AKASHI Takahiro, Ben Chuang, Adrian Hunter, Ulf Hansson,
linux-mmc, Linux Kernel Mailing List, Ben Chuang, greg.tu,
Renius.Chen
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
2020-09-24 9:46 ` AKASHI Takahiro
@ 2020-09-25 7:59 ` Ben Chuang
0 siblings, 0 replies; 19+ messages in thread
From: Ben Chuang @ 2020-09-25 7:59 UTC (permalink / raw)
To: AKASHI Takahiro, Ben Chuang, Adrian Hunter, Ulf Hansson,
linux-mmc, Linux Kernel Mailing List, Ben Chuang, greg.tu,
Renius.Chen
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);
> > > > > > >
> > > > > > >
> > > > > >
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations
2020-09-24 9:35 ` AKASHI Takahiro
@ 2020-09-25 9:02 ` Adrian Hunter
0 siblings, 0 replies; 19+ messages in thread
From: Adrian Hunter @ 2020-09-25 9:02 UTC (permalink / raw)
To: AKASHI Takahiro, Ben Chuang, ulf.hansson, linux-mmc,
linux-kernel, ben.chuang, greg.tu
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2020-09-25 9:03 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-10 11:10 [RFC PATCH V3 12/21] mmc: sdhci: UHS-II support, add hooks for additional operations Ben Chuang
2020-08-21 14:08 ` Adrian Hunter
2020-09-16 8:05 ` AKASHI Takahiro
2020-09-16 10:00 ` Adrian Hunter
2020-09-17 2:31 ` AKASHI Takahiro
2020-09-17 4:52 ` Adrian Hunter
2020-09-17 5:16 ` AKASHI Takahiro
2020-09-17 5:12 ` AKASHI Takahiro
2020-09-17 10:12 ` Ben Chuang
2020-09-18 1:15 ` AKASHI Takahiro
2020-09-18 10:27 ` Ben Chuang
2020-09-24 9:46 ` AKASHI Takahiro
2020-09-25 7:59 ` Ben Chuang
2020-09-18 6:38 ` AKASHI Takahiro
2020-09-18 10:50 ` Ben Chuang
2020-09-24 9:57 ` AKASHI Takahiro
2020-09-25 7:55 ` Ben Chuang
2020-09-24 9:35 ` AKASHI Takahiro
2020-09-25 9:02 ` Adrian Hunter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).