TAP and SCC handling is Renesas specific, so it should be moved to the SDHI driver. After previous refactoring, this is possible now. And feasible, too, to simplify further HS400 corrections. IMHO it also makes the driver less complicated. See the patches why this series is still RFC. This is based on top of the series: [RFC PATCH v2 0/5] mmc: renesas_sdhi: add manual correction A branch can be found here: git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/sdhi/new_manual_calib It has been tested with a Renesas Salvator-XS boards, one with a R-Car M3-N and one with H3-ES2.0. Tuning to HS400 still works. Wolfram Sang (6): mmc: tmio: refactor tuning execution into SDHI driver mmc: renesas_sdhi: complain loudly if driver needs update mmc: tmio: give callback a generic name mmc: tmio: enforce retune after runtime suspend mmc: tmio: factor out TAP usage mmc: tmio: remove superfluous callback wrappers drivers/mmc/host/renesas_sdhi.h | 5 ++ drivers/mmc/host/renesas_sdhi_core.c | 90 +++++++++++++++++----------- drivers/mmc/host/tmio_mmc.h | 11 +--- drivers/mmc/host/tmio_mmc_core.c | 77 +++--------------------- 4 files changed, 71 insertions(+), 112 deletions(-) -- 2.20.1
Move Renesas specific code for executing the tuning with a SCC into the SDHI driver and leave only a generic call in the TMIO driver. Simplify the code a little by removing init_tuning() and prepare_tuning() callbacks. The latter is directly folded into the new execute_tuning() callbacks. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/mmc/host/renesas_sdhi_core.c | 45 ++++++++++++++++++++-------- drivers/mmc/host/tmio_mmc.h | 3 +- drivers/mmc/host/tmio_mmc_core.c | 33 +++----------------- 3 files changed, 37 insertions(+), 44 deletions(-) diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c index 6a112454ca26..b3ab66f963f8 100644 --- a/drivers/mmc/host/renesas_sdhi_core.c +++ b/drivers/mmc/host/renesas_sdhi_core.c @@ -321,17 +321,6 @@ static unsigned int renesas_sdhi_init_tuning(struct tmio_mmc_host *host) SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_MASK; } -static void renesas_sdhi_prepare_tuning(struct tmio_mmc_host *host, - unsigned long tap) -{ - struct renesas_sdhi *priv = host_to_priv(host); - - priv->doing_tune = true; - - /* Set sampling clock position */ - sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, tap); -} - static void renesas_sdhi_hs400_complete(struct tmio_mmc_host *host) { struct renesas_sdhi *priv = host_to_priv(host); @@ -500,6 +489,37 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host) return 0; } +static int renesas_sdhi_execute_tuning(struct tmio_mmc_host *host, u32 opcode) +{ + struct renesas_sdhi *priv = host_to_priv(host); + int i, ret; + + host->tap_num = renesas_sdhi_init_tuning(host); + if (!host->tap_num) + return 0; /* Tuning is not supported */ + + if (host->tap_num * 2 >= sizeof(host->taps) * BITS_PER_BYTE) { + dev_warn_once(&host->pdev->dev, + "Too many taps, skipping tuning. Please consider updating size of taps field of tmio_mmc_host\n"); + return 0; + } + + priv->doing_tune = true; + bitmap_zero(host->taps, host->tap_num * 2); + + /* Issue CMD19 twice for each tap */ + for (i = 0; i < 2 * host->tap_num; i++) { + /* Set sampling clock position */ + sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, i % host->tap_num); + + ret = mmc_send_tuning(host->mmc, opcode, NULL); + if (ret == 0) + set_bit(i, host->taps); + } + + return renesas_sdhi_select_tuning(host); +} + static bool renesas_sdhi_manual_correction(struct tmio_mmc_host *host, bool use_4tap) { struct renesas_sdhi *priv = host_to_priv(host); @@ -877,8 +897,7 @@ int renesas_sdhi_probe(struct platform_device *pdev, if (!hit) dev_warn(&host->pdev->dev, "Unknown clock rate for tuning\n"); - host->init_tuning = renesas_sdhi_init_tuning; - host->prepare_tuning = renesas_sdhi_prepare_tuning; + host->execute_tuning = renesas_sdhi_execute_tuning; host->select_tuning = renesas_sdhi_select_tuning; host->check_scc_error = renesas_sdhi_check_scc_error; host->prepare_hs400_tuning = diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h index c5ba13fae399..bfebbe368f02 100644 --- a/drivers/mmc/host/tmio_mmc.h +++ b/drivers/mmc/host/tmio_mmc.h @@ -176,14 +176,13 @@ struct tmio_mmc_host { int (*write16_hook)(struct tmio_mmc_host *host, int addr); void (*reset)(struct tmio_mmc_host *host); void (*hw_reset)(struct tmio_mmc_host *host); - void (*prepare_tuning)(struct tmio_mmc_host *host, unsigned long tap); bool (*check_scc_error)(struct tmio_mmc_host *host); /* * Mandatory callback for tuning to occur which is optional for SDR50 * and mandatory for SDR104. */ - unsigned int (*init_tuning)(struct tmio_mmc_host *host); + int (*execute_tuning)(struct tmio_mmc_host *host, u32 opcode); int (*select_tuning)(struct tmio_mmc_host *host); /* Tuning values: 1 for success, 0 for failure */ diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c index c4a1d49fbea4..593f88cafb6e 100644 --- a/drivers/mmc/host/tmio_mmc_core.c +++ b/drivers/mmc/host/tmio_mmc_core.c @@ -718,38 +718,13 @@ static int tmio_mmc_start_data(struct tmio_mmc_host *host, static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode) { struct tmio_mmc_host *host = mmc_priv(mmc); - int i, ret = 0; - - if (!host->init_tuning || !host->select_tuning) - /* Tuning is not supported */ - goto out; - - host->tap_num = host->init_tuning(host); - if (!host->tap_num) - /* Tuning is not supported */ - goto out; - - if (host->tap_num * 2 >= sizeof(host->taps) * BITS_PER_BYTE) { - dev_warn_once(&host->pdev->dev, - "Too many taps, skipping tuning. Please consider updating size of taps field of tmio_mmc_host\n"); - goto out; - } - - bitmap_zero(host->taps, host->tap_num * 2); - - /* Issue CMD19 twice for each tap */ - for (i = 0; i < 2 * host->tap_num; i++) { - if (host->prepare_tuning) - host->prepare_tuning(host, i % host->tap_num); + int ret; - ret = mmc_send_tuning(mmc, opcode, NULL); - if (ret == 0) - set_bit(i, host->taps); - } + if (!host->execute_tuning) + return 0; - ret = host->select_tuning(host); + ret = host->execute_tuning(host, opcode); -out: if (ret < 0) { dev_warn(&host->pdev->dev, "Tuning procedure failed\n"); tmio_mmc_hw_reset(mmc); -- 2.20.1
When the tap array in the driver is too low, this is not a warning but an error. Also _once is not helpful, we should make sure it is prominently in the logs. It is safe to do this because this will only show up during SoC enablement when we a new SoCs needs more taps (if that ever will happen). Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- Note: 'unsigned long' seems big enough for a while. But, famous last words(tm). We could handle this at runtime by reallocating a bigger buffer. Very unsure if it is worth it, though. drivers/mmc/host/renesas_sdhi_core.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c index b3ab66f963f8..d63aeb35bd0b 100644 --- a/drivers/mmc/host/renesas_sdhi_core.c +++ b/drivers/mmc/host/renesas_sdhi_core.c @@ -499,9 +499,9 @@ static int renesas_sdhi_execute_tuning(struct tmio_mmc_host *host, u32 opcode) return 0; /* Tuning is not supported */ if (host->tap_num * 2 >= sizeof(host->taps) * BITS_PER_BYTE) { - dev_warn_once(&host->pdev->dev, - "Too many taps, skipping tuning. Please consider updating size of taps field of tmio_mmc_host\n"); - return 0; + dev_err(&host->pdev->dev, + "Too many taps, please update 'taps' in tmio_mmc_host!\n"); + return -EINVAL; } priv->doing_tune = true; -- 2.20.1
check_scc_error() is too Renesas specific. Let's just call it check_retune() to make it also easier understandable what it does. Only a rename, no functional change. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/mmc/host/renesas_sdhi_core.c | 2 +- drivers/mmc/host/tmio_mmc.h | 2 +- drivers/mmc/host/tmio_mmc_core.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c index d63aeb35bd0b..24ee8ac1fe21 100644 --- a/drivers/mmc/host/renesas_sdhi_core.c +++ b/drivers/mmc/host/renesas_sdhi_core.c @@ -899,7 +899,7 @@ int renesas_sdhi_probe(struct platform_device *pdev, host->execute_tuning = renesas_sdhi_execute_tuning; host->select_tuning = renesas_sdhi_select_tuning; - host->check_scc_error = renesas_sdhi_check_scc_error; + host->check_retune = renesas_sdhi_check_scc_error; host->prepare_hs400_tuning = renesas_sdhi_prepare_hs400_tuning; host->hs400_downgrade = renesas_sdhi_disable_scc; diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h index bfebbe368f02..bdb9973981ff 100644 --- a/drivers/mmc/host/tmio_mmc.h +++ b/drivers/mmc/host/tmio_mmc.h @@ -176,7 +176,7 @@ struct tmio_mmc_host { int (*write16_hook)(struct tmio_mmc_host *host, int addr); void (*reset)(struct tmio_mmc_host *host); void (*hw_reset)(struct tmio_mmc_host *host); - bool (*check_scc_error)(struct tmio_mmc_host *host); + bool (*check_retune)(struct tmio_mmc_host *host); /* * Mandatory callback for tuning to occur which is optional for SDR50 diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c index 593f88cafb6e..3cacb516a66e 100644 --- a/drivers/mmc/host/tmio_mmc_core.c +++ b/drivers/mmc/host/tmio_mmc_core.c @@ -818,8 +818,8 @@ static void tmio_mmc_finish_request(struct tmio_mmc_host *host) if (mrq->cmd->error || (mrq->data && mrq->data->error)) tmio_mmc_abort_dma(host); - /* SCC error means retune, but executed command was still successful */ - if (host->check_scc_error && host->check_scc_error(host)) + /* Error means retune, but executed command was still successful */ + if (host->check_retune && host->check_retune(host)) mmc_retune_needed(host->mmc); /* If SET_BLOCK_COUNT, continue with main command */ -- 2.20.1
Currently, select_tuning() is called after RPM resume. But select_tuning() needs some additional function calls to work correctly. Instead of reimplementing the whole postprocessing, just enforce retuning. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- I couldn't trigger RPM suspend even with debug code. Shimoda-san said it should only occur with removed cards which is not so easy with soldered eMMC. For those cases, I think the aproach taken here is fine. Needs more discussion, though, to make sure... drivers/mmc/host/renesas_sdhi_core.c | 1 - drivers/mmc/host/tmio_mmc.h | 1 - drivers/mmc/host/tmio_mmc_core.c | 8 +------- 3 files changed, 1 insertion(+), 9 deletions(-) diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c index 24ee8ac1fe21..0c9e5e010bda 100644 --- a/drivers/mmc/host/renesas_sdhi_core.c +++ b/drivers/mmc/host/renesas_sdhi_core.c @@ -898,7 +898,6 @@ int renesas_sdhi_probe(struct platform_device *pdev, dev_warn(&host->pdev->dev, "Unknown clock rate for tuning\n"); host->execute_tuning = renesas_sdhi_execute_tuning; - host->select_tuning = renesas_sdhi_select_tuning; host->check_retune = renesas_sdhi_check_scc_error; host->prepare_hs400_tuning = renesas_sdhi_prepare_hs400_tuning; diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h index bdb9973981ff..b6fffd3d2650 100644 --- a/drivers/mmc/host/tmio_mmc.h +++ b/drivers/mmc/host/tmio_mmc.h @@ -183,7 +183,6 @@ struct tmio_mmc_host { * and mandatory for SDR104. */ int (*execute_tuning)(struct tmio_mmc_host *host, u32 opcode); - int (*select_tuning)(struct tmio_mmc_host *host); /* Tuning values: 1 for success, 0 for failure */ DECLARE_BITMAP(taps, BITS_PER_BYTE * sizeof(long)); diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c index 3cacb516a66e..aeafa1c68ce2 100644 --- a/drivers/mmc/host/tmio_mmc_core.c +++ b/drivers/mmc/host/tmio_mmc_core.c @@ -1302,11 +1302,6 @@ int tmio_mmc_host_runtime_suspend(struct device *dev) } EXPORT_SYMBOL_GPL(tmio_mmc_host_runtime_suspend); -static bool tmio_mmc_can_retune(struct tmio_mmc_host *host) -{ - return host->tap_num && mmc_can_retune(host->mmc); -} - int tmio_mmc_host_runtime_resume(struct device *dev) { struct tmio_mmc_host *host = dev_get_drvdata(dev); @@ -1323,8 +1318,7 @@ int tmio_mmc_host_runtime_resume(struct device *dev) tmio_mmc_enable_dma(host, true); - if (tmio_mmc_can_retune(host) && host->select_tuning(host)) - dev_warn(&host->pdev->dev, "Tuning selection failed\n"); + mmc_retune_needed(host->mmc); return 0; } -- 2.20.1
TAPs are Renesas SDHI specific. Now that we moved all handling to the SDHI core, we can also move the definitions from the TMIO struct to the SDHI one. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/mmc/host/renesas_sdhi.h | 5 ++++ drivers/mmc/host/renesas_sdhi_core.c | 38 ++++++++++++++-------------- drivers/mmc/host/tmio_mmc.h | 5 ---- 3 files changed, 24 insertions(+), 24 deletions(-) diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h index 11a0b2bca3aa..7a1a741547f2 100644 --- a/drivers/mmc/host/renesas_sdhi.h +++ b/drivers/mmc/host/renesas_sdhi.h @@ -58,6 +58,11 @@ struct renesas_sdhi { u32 scc_tappos; u32 scc_tappos_hs400; bool doing_tune; + + /* Tuning values: 1 for success, 0 for failure */ + DECLARE_BITMAP(taps, BITS_PER_BYTE * sizeof(long)); + unsigned int tap_num; + unsigned long tap_set; }; #define host_to_priv(host) \ diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c index 0c9e5e010bda..22eaabe513d0 100644 --- a/drivers/mmc/host/renesas_sdhi_core.c +++ b/drivers/mmc/host/renesas_sdhi_core.c @@ -354,7 +354,7 @@ static void renesas_sdhi_hs400_complete(struct tmio_mmc_host *host) if (priv->quirks && priv->quirks->hs400_4taps) sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, - host->tap_set / 2); + priv->tap_set / 2); sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_CKSEL, SH_MOBILE_SDHI_SCC_CKSEL_DTSEL | @@ -438,11 +438,11 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host) * result requiring the tap to be good in both runs before * considering it for tuning selection. */ - for (i = 0; i < host->tap_num * 2; i++) { - int offset = host->tap_num * (i < host->tap_num ? 1 : -1); + for (i = 0; i < priv->tap_num * 2; i++) { + int offset = priv->tap_num * (i < priv->tap_num ? 1 : -1); - if (!test_bit(i, host->taps)) - clear_bit(i + offset, host->taps); + if (!test_bit(i, priv->taps)) + clear_bit(i + offset, priv->taps); } /* @@ -454,8 +454,8 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host) ntap = 0; tap_start = 0; tap_end = 0; - for (i = 0; i < host->tap_num * 2; i++) { - if (test_bit(i, host->taps)) { + for (i = 0; i < priv->tap_num * 2; i++) { + if (test_bit(i, priv->taps)) { ntap++; } else { if (ntap > tap_cnt) { @@ -474,12 +474,12 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host) } if (tap_cnt >= SH_MOBILE_SDHI_MAX_TAP) - host->tap_set = (tap_start + tap_end) / 2 % host->tap_num; + priv->tap_set = (tap_start + tap_end) / 2 % priv->tap_num; else return -EIO; /* Set SCC */ - sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, host->tap_set); + sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, priv->tap_set); /* Enable auto re-tuning */ sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL, @@ -494,27 +494,27 @@ static int renesas_sdhi_execute_tuning(struct tmio_mmc_host *host, u32 opcode) struct renesas_sdhi *priv = host_to_priv(host); int i, ret; - host->tap_num = renesas_sdhi_init_tuning(host); - if (!host->tap_num) + priv->tap_num = renesas_sdhi_init_tuning(host); + if (!priv->tap_num) return 0; /* Tuning is not supported */ - if (host->tap_num * 2 >= sizeof(host->taps) * BITS_PER_BYTE) { + if (priv->tap_num * 2 >= sizeof(priv->taps) * BITS_PER_BYTE) { dev_err(&host->pdev->dev, "Too many taps, please update 'taps' in tmio_mmc_host!\n"); return -EINVAL; } priv->doing_tune = true; - bitmap_zero(host->taps, host->tap_num * 2); + bitmap_zero(priv->taps, priv->tap_num * 2); /* Issue CMD19 twice for each tap */ - for (i = 0; i < 2 * host->tap_num; i++) { + for (i = 0; i < 2 * priv->tap_num; i++) { /* Set sampling clock position */ - sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, i % host->tap_num); + sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, i % priv->tap_num); ret = mmc_send_tuning(host->mmc, opcode, NULL); if (ret == 0) - set_bit(i, host->taps); + set_bit(i, priv->taps); } return renesas_sdhi_select_tuning(host); @@ -523,7 +523,7 @@ static int renesas_sdhi_execute_tuning(struct tmio_mmc_host *host, u32 opcode) static bool renesas_sdhi_manual_correction(struct tmio_mmc_host *host, bool use_4tap) { struct renesas_sdhi *priv = host_to_priv(host); - unsigned long new_tap = host->tap_set; + unsigned long new_tap = priv->tap_set; u32 val; val = sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSREQ); @@ -560,9 +560,9 @@ static bool renesas_sdhi_manual_correction(struct tmio_mmc_host *host, bool use_ return false; } - host->tap_set = (new_tap % host->tap_num); + priv->tap_set = (new_tap % priv->tap_num); sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, - host->tap_set / (use_4tap ? 2 : 1)); + priv->tap_set / (use_4tap ? 2 : 1)); return false; } diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h index b6fffd3d2650..b4cf10109162 100644 --- a/drivers/mmc/host/tmio_mmc.h +++ b/drivers/mmc/host/tmio_mmc.h @@ -184,11 +184,6 @@ struct tmio_mmc_host { */ int (*execute_tuning)(struct tmio_mmc_host *host, u32 opcode); - /* Tuning values: 1 for success, 0 for failure */ - DECLARE_BITMAP(taps, BITS_PER_BYTE * sizeof(long)); - unsigned int tap_num; - unsigned long tap_set; - void (*prepare_hs400_tuning)(struct tmio_mmc_host *host); void (*hs400_downgrade)(struct tmio_mmc_host *host); void (*hs400_complete)(struct tmio_mmc_host *host); -- 2.20.1
After various refactoring, we can populate the mmc_ops callbacks directly and don't need to have wrappers for them anymore. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/mmc/host/renesas_sdhi_core.c | 18 ++++++++++------ drivers/mmc/host/tmio_mmc_core.c | 32 +--------------------------- 2 files changed, 12 insertions(+), 38 deletions(-) diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c index 22eaabe513d0..0f07cc1aee34 100644 --- a/drivers/mmc/host/renesas_sdhi_core.c +++ b/drivers/mmc/host/renesas_sdhi_core.c @@ -321,8 +321,9 @@ static unsigned int renesas_sdhi_init_tuning(struct tmio_mmc_host *host) SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_MASK; } -static void renesas_sdhi_hs400_complete(struct tmio_mmc_host *host) +static void renesas_sdhi_hs400_complete(struct mmc_host *mmc) { + struct tmio_mmc_host *host = mmc_priv(mmc); struct renesas_sdhi *priv = host_to_priv(host); sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, ~CLK_CTL_SCLKEN & @@ -376,8 +377,9 @@ static void renesas_sdhi_reset_scc(struct tmio_mmc_host *host, SH_MOBILE_SDHI_SCC_CKSEL)); } -static void renesas_sdhi_disable_scc(struct tmio_mmc_host *host) +static void renesas_sdhi_disable_scc(struct mmc_host *mmc) { + struct tmio_mmc_host *host = mmc_priv(mmc); struct renesas_sdhi *priv = host_to_priv(host); renesas_sdhi_reset_scc(host, priv); @@ -412,9 +414,12 @@ static void renesas_sdhi_reset_hs400_mode(struct tmio_mmc_host *host, sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL)); } -static void renesas_sdhi_prepare_hs400_tuning(struct tmio_mmc_host *host) +static int renesas_sdhi_prepare_hs400_tuning(struct mmc_host *mmc, struct mmc_ios *ios) { + struct tmio_mmc_host *host = mmc_priv(mmc); + renesas_sdhi_reset_hs400_mode(host, host_to_priv(host)); + return 0; } #define SH_MOBILE_SDHI_MAX_TAP 3 @@ -899,10 +904,9 @@ int renesas_sdhi_probe(struct platform_device *pdev, host->execute_tuning = renesas_sdhi_execute_tuning; host->check_retune = renesas_sdhi_check_scc_error; - host->prepare_hs400_tuning = - renesas_sdhi_prepare_hs400_tuning; - host->hs400_downgrade = renesas_sdhi_disable_scc; - host->hs400_complete = renesas_sdhi_hs400_complete; + host->ops.prepare_hs400_tuning = renesas_sdhi_prepare_hs400_tuning; + host->ops.hs400_downgrade = renesas_sdhi_disable_scc; + host->ops.hs400_complete = renesas_sdhi_hs400_complete; } num_irqs = platform_irq_count(pdev); diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c index aeafa1c68ce2..a2415fb5dde0 100644 --- a/drivers/mmc/host/tmio_mmc_core.c +++ b/drivers/mmc/host/tmio_mmc_core.c @@ -997,34 +997,7 @@ static int tmio_multi_io_quirk(struct mmc_card *card, return blk_size; } -static int tmio_mmc_prepare_hs400_tuning(struct mmc_host *mmc, - struct mmc_ios *ios) -{ - struct tmio_mmc_host *host = mmc_priv(mmc); - - if (host->prepare_hs400_tuning) - host->prepare_hs400_tuning(host); - - return 0; -} - -static void tmio_mmc_hs400_downgrade(struct mmc_host *mmc) -{ - struct tmio_mmc_host *host = mmc_priv(mmc); - - if (host->hs400_downgrade) - host->hs400_downgrade(host); -} - -static void tmio_mmc_hs400_complete(struct mmc_host *mmc) -{ - struct tmio_mmc_host *host = mmc_priv(mmc); - - if (host->hs400_complete) - host->hs400_complete(host); -} - -static const struct mmc_host_ops tmio_mmc_ops = { +static struct mmc_host_ops tmio_mmc_ops = { .request = tmio_mmc_request, .set_ios = tmio_mmc_set_ios, .get_ro = tmio_mmc_get_ro, @@ -1033,9 +1006,6 @@ static const struct mmc_host_ops tmio_mmc_ops = { .multi_io_quirk = tmio_multi_io_quirk, .hw_reset = tmio_mmc_hw_reset, .execute_tuning = tmio_mmc_execute_tuning, - .prepare_hs400_tuning = tmio_mmc_prepare_hs400_tuning, - .hs400_downgrade = tmio_mmc_hs400_downgrade, - .hs400_complete = tmio_mmc_hs400_complete, }; static int tmio_mmc_init_ocr(struct tmio_mmc_host *host) -- 2.20.1
Hi Wolfram-san,
> From: Wolfram Sang, Sent: Thursday, January 30, 2020 5:37 AM
>
> TAP and SCC handling is Renesas specific, so it should be moved to the
> SDHI driver. After previous refactoring, this is possible now. And
> feasible, too, to simplify further HS400 corrections. IMHO it also makes
> the driver less complicated.
>
> See the patches why this series is still RFC.
>
> This is based on top of the series:
> [RFC PATCH v2 0/5] mmc: renesas_sdhi: add manual correction
>
> A branch can be found here:
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/sdhi/new_manual_calib
>
> It has been tested with a Renesas Salvator-XS boards, one with a R-Car
> M3-N and one with H3-ES2.0. Tuning to HS400 still works.
Thank you for the refactoring!
This patch series looks good to me. So,
Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Best regards,
Yoshihiro Shimoda
Hi Wolfram, Thanks for your work. On 2020-01-29 21:37:04 +0100, Wolfram Sang wrote: > Move Renesas specific code for executing the tuning with a SCC into the > SDHI driver and leave only a generic call in the TMIO driver. Simplify > the code a little by removing init_tuning() and prepare_tuning() > callbacks. The latter is directly folded into the new execute_tuning() > callbacks. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/mmc/host/renesas_sdhi_core.c | 45 ++++++++++++++++++++-------- > drivers/mmc/host/tmio_mmc.h | 3 +- > drivers/mmc/host/tmio_mmc_core.c | 33 +++----------------- > 3 files changed, 37 insertions(+), 44 deletions(-) > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c > index 6a112454ca26..b3ab66f963f8 100644 > --- a/drivers/mmc/host/renesas_sdhi_core.c > +++ b/drivers/mmc/host/renesas_sdhi_core.c > @@ -321,17 +321,6 @@ static unsigned int renesas_sdhi_init_tuning(struct tmio_mmc_host *host) > SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_MASK; > } > > -static void renesas_sdhi_prepare_tuning(struct tmio_mmc_host *host, > - unsigned long tap) > -{ > - struct renesas_sdhi *priv = host_to_priv(host); > - > - priv->doing_tune = true; > - > - /* Set sampling clock position */ > - sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, tap); > -} > - > static void renesas_sdhi_hs400_complete(struct tmio_mmc_host *host) > { > struct renesas_sdhi *priv = host_to_priv(host); > @@ -500,6 +489,37 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host) > return 0; > } > > +static int renesas_sdhi_execute_tuning(struct tmio_mmc_host *host, u32 opcode) > +{ > + struct renesas_sdhi *priv = host_to_priv(host); > + int i, ret; > + > + host->tap_num = renesas_sdhi_init_tuning(host); > + if (!host->tap_num) > + return 0; /* Tuning is not supported */ > + > + if (host->tap_num * 2 >= sizeof(host->taps) * BITS_PER_BYTE) { > + dev_warn_once(&host->pdev->dev, > + "Too many taps, skipping tuning. Please consider updating size of taps field of tmio_mmc_host\n"); > + return 0; > + } > + > + priv->doing_tune = true; > + bitmap_zero(host->taps, host->tap_num * 2); > + > + /* Issue CMD19 twice for each tap */ > + for (i = 0; i < 2 * host->tap_num; i++) { > + /* Set sampling clock position */ > + sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, i % host->tap_num); > + > + ret = mmc_send_tuning(host->mmc, opcode, NULL); > + if (ret == 0) The variable ret is only used here after the refactor so you could possibly drop it and just check mmc_send_tuning() == 0. With or without this small nit addressed, Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > + set_bit(i, host->taps); > + } > + > + return renesas_sdhi_select_tuning(host); > +} > + > static bool renesas_sdhi_manual_correction(struct tmio_mmc_host *host, bool use_4tap) > { > struct renesas_sdhi *priv = host_to_priv(host); > @@ -877,8 +897,7 @@ int renesas_sdhi_probe(struct platform_device *pdev, > if (!hit) > dev_warn(&host->pdev->dev, "Unknown clock rate for tuning\n"); > > - host->init_tuning = renesas_sdhi_init_tuning; > - host->prepare_tuning = renesas_sdhi_prepare_tuning; > + host->execute_tuning = renesas_sdhi_execute_tuning; > host->select_tuning = renesas_sdhi_select_tuning; > host->check_scc_error = renesas_sdhi_check_scc_error; > host->prepare_hs400_tuning = > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h > index c5ba13fae399..bfebbe368f02 100644 > --- a/drivers/mmc/host/tmio_mmc.h > +++ b/drivers/mmc/host/tmio_mmc.h > @@ -176,14 +176,13 @@ struct tmio_mmc_host { > int (*write16_hook)(struct tmio_mmc_host *host, int addr); > void (*reset)(struct tmio_mmc_host *host); > void (*hw_reset)(struct tmio_mmc_host *host); > - void (*prepare_tuning)(struct tmio_mmc_host *host, unsigned long tap); > bool (*check_scc_error)(struct tmio_mmc_host *host); > > /* > * Mandatory callback for tuning to occur which is optional for SDR50 > * and mandatory for SDR104. > */ > - unsigned int (*init_tuning)(struct tmio_mmc_host *host); > + int (*execute_tuning)(struct tmio_mmc_host *host, u32 opcode); > int (*select_tuning)(struct tmio_mmc_host *host); > > /* Tuning values: 1 for success, 0 for failure */ > diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c > index c4a1d49fbea4..593f88cafb6e 100644 > --- a/drivers/mmc/host/tmio_mmc_core.c > +++ b/drivers/mmc/host/tmio_mmc_core.c > @@ -718,38 +718,13 @@ static int tmio_mmc_start_data(struct tmio_mmc_host *host, > static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode) > { > struct tmio_mmc_host *host = mmc_priv(mmc); > - int i, ret = 0; > - > - if (!host->init_tuning || !host->select_tuning) > - /* Tuning is not supported */ > - goto out; > - > - host->tap_num = host->init_tuning(host); > - if (!host->tap_num) > - /* Tuning is not supported */ > - goto out; > - > - if (host->tap_num * 2 >= sizeof(host->taps) * BITS_PER_BYTE) { > - dev_warn_once(&host->pdev->dev, > - "Too many taps, skipping tuning. Please consider updating size of taps field of tmio_mmc_host\n"); > - goto out; > - } > - > - bitmap_zero(host->taps, host->tap_num * 2); > - > - /* Issue CMD19 twice for each tap */ > - for (i = 0; i < 2 * host->tap_num; i++) { > - if (host->prepare_tuning) > - host->prepare_tuning(host, i % host->tap_num); > + int ret; > > - ret = mmc_send_tuning(mmc, opcode, NULL); > - if (ret == 0) > - set_bit(i, host->taps); > - } > + if (!host->execute_tuning) > + return 0; > > - ret = host->select_tuning(host); > + ret = host->execute_tuning(host, opcode); > > -out: > if (ret < 0) { > dev_warn(&host->pdev->dev, "Tuning procedure failed\n"); > tmio_mmc_hw_reset(mmc); > -- > 2.20.1 > -- Regards, Niklas Söderlund
Hi Wolfram, Thanks for your patch. On 2020-01-29 21:37:05 +0100, Wolfram Sang wrote: > When the tap array in the driver is too low, this is not a warning but > an error. Also _once is not helpful, we should make sure it is > prominently in the logs. It is safe to do this because this will only > show up during SoC enablement when we a new SoCs needs more taps (if > that ever will happen). > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > Note: 'unsigned long' seems big enough for a while. But, famous last > words(tm). We could handle this at runtime by reallocating a bigger > buffer. Very unsure if it is worth it, though. I can not tell if it's worth doing this now or not. But if it's a error instead of a warning it will be easier to spot if we should have done so ;-) Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > drivers/mmc/host/renesas_sdhi_core.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c > index b3ab66f963f8..d63aeb35bd0b 100644 > --- a/drivers/mmc/host/renesas_sdhi_core.c > +++ b/drivers/mmc/host/renesas_sdhi_core.c > @@ -499,9 +499,9 @@ static int renesas_sdhi_execute_tuning(struct tmio_mmc_host *host, u32 opcode) > return 0; /* Tuning is not supported */ > > if (host->tap_num * 2 >= sizeof(host->taps) * BITS_PER_BYTE) { > - dev_warn_once(&host->pdev->dev, > - "Too many taps, skipping tuning. Please consider updating size of taps field of tmio_mmc_host\n"); > - return 0; > + dev_err(&host->pdev->dev, > + "Too many taps, please update 'taps' in tmio_mmc_host!\n"); > + return -EINVAL; > } > > priv->doing_tune = true; > -- > 2.20.1 > -- Regards, Niklas Söderlund
Hi Wolfram, Thanks for your work. On 2020-01-29 21:37:06 +0100, Wolfram Sang wrote: > check_scc_error() is too Renesas specific. Let's just call it > check_retune() to make it also easier understandable what it does. > Only a rename, no functional change. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/mmc/host/renesas_sdhi_core.c | 2 +- > drivers/mmc/host/tmio_mmc.h | 2 +- > drivers/mmc/host/tmio_mmc_core.c | 4 ++-- > 3 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c > index d63aeb35bd0b..24ee8ac1fe21 100644 > --- a/drivers/mmc/host/renesas_sdhi_core.c > +++ b/drivers/mmc/host/renesas_sdhi_core.c > @@ -899,7 +899,7 @@ int renesas_sdhi_probe(struct platform_device *pdev, > > host->execute_tuning = renesas_sdhi_execute_tuning; > host->select_tuning = renesas_sdhi_select_tuning; > - host->check_scc_error = renesas_sdhi_check_scc_error; > + host->check_retune = renesas_sdhi_check_scc_error; > host->prepare_hs400_tuning = > renesas_sdhi_prepare_hs400_tuning; > host->hs400_downgrade = renesas_sdhi_disable_scc; > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h > index bfebbe368f02..bdb9973981ff 100644 > --- a/drivers/mmc/host/tmio_mmc.h > +++ b/drivers/mmc/host/tmio_mmc.h > @@ -176,7 +176,7 @@ struct tmio_mmc_host { > int (*write16_hook)(struct tmio_mmc_host *host, int addr); > void (*reset)(struct tmio_mmc_host *host); > void (*hw_reset)(struct tmio_mmc_host *host); > - bool (*check_scc_error)(struct tmio_mmc_host *host); > + bool (*check_retune)(struct tmio_mmc_host *host); > > /* > * Mandatory callback for tuning to occur which is optional for SDR50 > diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c > index 593f88cafb6e..3cacb516a66e 100644 > --- a/drivers/mmc/host/tmio_mmc_core.c > +++ b/drivers/mmc/host/tmio_mmc_core.c > @@ -818,8 +818,8 @@ static void tmio_mmc_finish_request(struct tmio_mmc_host *host) > if (mrq->cmd->error || (mrq->data && mrq->data->error)) > tmio_mmc_abort_dma(host); > > - /* SCC error means retune, but executed command was still successful */ > - if (host->check_scc_error && host->check_scc_error(host)) > + /* Error means retune, but executed command was still successful */ > + if (host->check_retune && host->check_retune(host)) > mmc_retune_needed(host->mmc); > > /* If SET_BLOCK_COUNT, continue with main command */ > -- > 2.20.1 > -- Regards, Niklas Söderlund
Hi Wolfram, Thanks for your patch. On 2020-01-29 21:37:07 +0100, Wolfram Sang wrote: > Currently, select_tuning() is called after RPM resume. But > select_tuning() needs some additional function calls to work correctly. > Instead of reimplementing the whole postprocessing, just enforce > retuning. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > I couldn't trigger RPM suspend even with debug code. Shimoda-san said > it should only occur with removed cards which is not so easy with > soldered eMMC. For those cases, I think the aproach taken here is fine. > Needs more discussion, though, to make sure... I really like the change so I really wanted to provoke a suspend to be able to provide a tested by tag, but I have so far been unable to do so :-( > > drivers/mmc/host/renesas_sdhi_core.c | 1 - > drivers/mmc/host/tmio_mmc.h | 1 - > drivers/mmc/host/tmio_mmc_core.c | 8 +------- > 3 files changed, 1 insertion(+), 9 deletions(-) > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c > index 24ee8ac1fe21..0c9e5e010bda 100644 > --- a/drivers/mmc/host/renesas_sdhi_core.c > +++ b/drivers/mmc/host/renesas_sdhi_core.c > @@ -898,7 +898,6 @@ int renesas_sdhi_probe(struct platform_device *pdev, > dev_warn(&host->pdev->dev, "Unknown clock rate for tuning\n"); > > host->execute_tuning = renesas_sdhi_execute_tuning; > - host->select_tuning = renesas_sdhi_select_tuning; > host->check_retune = renesas_sdhi_check_scc_error; > host->prepare_hs400_tuning = > renesas_sdhi_prepare_hs400_tuning; > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h > index bdb9973981ff..b6fffd3d2650 100644 > --- a/drivers/mmc/host/tmio_mmc.h > +++ b/drivers/mmc/host/tmio_mmc.h > @@ -183,7 +183,6 @@ struct tmio_mmc_host { > * and mandatory for SDR104. > */ > int (*execute_tuning)(struct tmio_mmc_host *host, u32 opcode); > - int (*select_tuning)(struct tmio_mmc_host *host); > > /* Tuning values: 1 for success, 0 for failure */ > DECLARE_BITMAP(taps, BITS_PER_BYTE * sizeof(long)); > diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c > index 3cacb516a66e..aeafa1c68ce2 100644 > --- a/drivers/mmc/host/tmio_mmc_core.c > +++ b/drivers/mmc/host/tmio_mmc_core.c > @@ -1302,11 +1302,6 @@ int tmio_mmc_host_runtime_suspend(struct device *dev) > } > EXPORT_SYMBOL_GPL(tmio_mmc_host_runtime_suspend); > > -static bool tmio_mmc_can_retune(struct tmio_mmc_host *host) > -{ > - return host->tap_num && mmc_can_retune(host->mmc); > -} > - > int tmio_mmc_host_runtime_resume(struct device *dev) > { > struct tmio_mmc_host *host = dev_get_drvdata(dev); > @@ -1323,8 +1318,7 @@ int tmio_mmc_host_runtime_resume(struct device *dev) > > tmio_mmc_enable_dma(host, true); > > - if (tmio_mmc_can_retune(host) && host->select_tuning(host)) > - dev_warn(&host->pdev->dev, "Tuning selection failed\n"); > + mmc_retune_needed(host->mmc); > > return 0; > } > -- > 2.20.1 > -- Regards, Niklas Söderlund
Hi Wolfram, Thanks for your work. On 2020-01-29 21:37:08 +0100, Wolfram Sang wrote: > TAPs are Renesas SDHI specific. Now that we moved all handling to the > SDHI core, we can also move the definitions from the TMIO struct to the > SDHI one. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/mmc/host/renesas_sdhi.h | 5 ++++ > drivers/mmc/host/renesas_sdhi_core.c | 38 ++++++++++++++-------------- > drivers/mmc/host/tmio_mmc.h | 5 ---- > 3 files changed, 24 insertions(+), 24 deletions(-) > > diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h > index 11a0b2bca3aa..7a1a741547f2 100644 > --- a/drivers/mmc/host/renesas_sdhi.h > +++ b/drivers/mmc/host/renesas_sdhi.h > @@ -58,6 +58,11 @@ struct renesas_sdhi { > u32 scc_tappos; > u32 scc_tappos_hs400; > bool doing_tune; > + > + /* Tuning values: 1 for success, 0 for failure */ > + DECLARE_BITMAP(taps, BITS_PER_BYTE * sizeof(long)); > + unsigned int tap_num; > + unsigned long tap_set; > }; > > #define host_to_priv(host) \ > diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c > index 0c9e5e010bda..22eaabe513d0 100644 > --- a/drivers/mmc/host/renesas_sdhi_core.c > +++ b/drivers/mmc/host/renesas_sdhi_core.c > @@ -354,7 +354,7 @@ static void renesas_sdhi_hs400_complete(struct tmio_mmc_host *host) > > if (priv->quirks && priv->quirks->hs400_4taps) > sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, > - host->tap_set / 2); > + priv->tap_set / 2); > > sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_CKSEL, > SH_MOBILE_SDHI_SCC_CKSEL_DTSEL | > @@ -438,11 +438,11 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host) > * result requiring the tap to be good in both runs before > * considering it for tuning selection. > */ > - for (i = 0; i < host->tap_num * 2; i++) { > - int offset = host->tap_num * (i < host->tap_num ? 1 : -1); > + for (i = 0; i < priv->tap_num * 2; i++) { > + int offset = priv->tap_num * (i < priv->tap_num ? 1 : -1); > > - if (!test_bit(i, host->taps)) > - clear_bit(i + offset, host->taps); > + if (!test_bit(i, priv->taps)) > + clear_bit(i + offset, priv->taps); > } > > /* > @@ -454,8 +454,8 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host) > ntap = 0; > tap_start = 0; > tap_end = 0; > - for (i = 0; i < host->tap_num * 2; i++) { > - if (test_bit(i, host->taps)) { > + for (i = 0; i < priv->tap_num * 2; i++) { > + if (test_bit(i, priv->taps)) { > ntap++; > } else { > if (ntap > tap_cnt) { > @@ -474,12 +474,12 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host) > } > > if (tap_cnt >= SH_MOBILE_SDHI_MAX_TAP) > - host->tap_set = (tap_start + tap_end) / 2 % host->tap_num; > + priv->tap_set = (tap_start + tap_end) / 2 % priv->tap_num; > else > return -EIO; > > /* Set SCC */ > - sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, host->tap_set); > + sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, priv->tap_set); > > /* Enable auto re-tuning */ > sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL, > @@ -494,27 +494,27 @@ static int renesas_sdhi_execute_tuning(struct tmio_mmc_host *host, u32 opcode) > struct renesas_sdhi *priv = host_to_priv(host); > int i, ret; > > - host->tap_num = renesas_sdhi_init_tuning(host); > - if (!host->tap_num) > + priv->tap_num = renesas_sdhi_init_tuning(host); > + if (!priv->tap_num) > return 0; /* Tuning is not supported */ > > - if (host->tap_num * 2 >= sizeof(host->taps) * BITS_PER_BYTE) { > + if (priv->tap_num * 2 >= sizeof(priv->taps) * BITS_PER_BYTE) { > dev_err(&host->pdev->dev, > "Too many taps, please update 'taps' in tmio_mmc_host!\n"); > return -EINVAL; > } > > priv->doing_tune = true; > - bitmap_zero(host->taps, host->tap_num * 2); > + bitmap_zero(priv->taps, priv->tap_num * 2); > > /* Issue CMD19 twice for each tap */ > - for (i = 0; i < 2 * host->tap_num; i++) { > + for (i = 0; i < 2 * priv->tap_num; i++) { > /* Set sampling clock position */ > - sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, i % host->tap_num); > + sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, i % priv->tap_num); > > ret = mmc_send_tuning(host->mmc, opcode, NULL); > if (ret == 0) > - set_bit(i, host->taps); > + set_bit(i, priv->taps); > } > > return renesas_sdhi_select_tuning(host); > @@ -523,7 +523,7 @@ static int renesas_sdhi_execute_tuning(struct tmio_mmc_host *host, u32 opcode) > static bool renesas_sdhi_manual_correction(struct tmio_mmc_host *host, bool use_4tap) > { > struct renesas_sdhi *priv = host_to_priv(host); > - unsigned long new_tap = host->tap_set; > + unsigned long new_tap = priv->tap_set; > u32 val; > > val = sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSREQ); > @@ -560,9 +560,9 @@ static bool renesas_sdhi_manual_correction(struct tmio_mmc_host *host, bool use_ > return false; > } > > - host->tap_set = (new_tap % host->tap_num); > + priv->tap_set = (new_tap % priv->tap_num); > sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, > - host->tap_set / (use_4tap ? 2 : 1)); > + priv->tap_set / (use_4tap ? 2 : 1)); > > return false; > } > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h > index b6fffd3d2650..b4cf10109162 100644 > --- a/drivers/mmc/host/tmio_mmc.h > +++ b/drivers/mmc/host/tmio_mmc.h > @@ -184,11 +184,6 @@ struct tmio_mmc_host { > */ > int (*execute_tuning)(struct tmio_mmc_host *host, u32 opcode); > > - /* Tuning values: 1 for success, 0 for failure */ > - DECLARE_BITMAP(taps, BITS_PER_BYTE * sizeof(long)); > - unsigned int tap_num; > - unsigned long tap_set; > - > void (*prepare_hs400_tuning)(struct tmio_mmc_host *host); > void (*hs400_downgrade)(struct tmio_mmc_host *host); > void (*hs400_complete)(struct tmio_mmc_host *host); > -- > 2.20.1 > -- Regards, Niklas Söderlund
Hi Wolfram, Thanks for your patch. On 2020-01-29 21:37:09 +0100, Wolfram Sang wrote: > After various refactoring, we can populate the mmc_ops callbacks > directly and don't need to have wrappers for them anymore. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/mmc/host/renesas_sdhi_core.c | 18 ++++++++++------ > drivers/mmc/host/tmio_mmc_core.c | 32 +--------------------------- > 2 files changed, 12 insertions(+), 38 deletions(-) > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c > index 22eaabe513d0..0f07cc1aee34 100644 > --- a/drivers/mmc/host/renesas_sdhi_core.c > +++ b/drivers/mmc/host/renesas_sdhi_core.c > @@ -321,8 +321,9 @@ static unsigned int renesas_sdhi_init_tuning(struct tmio_mmc_host *host) > SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_MASK; > } > > -static void renesas_sdhi_hs400_complete(struct tmio_mmc_host *host) > +static void renesas_sdhi_hs400_complete(struct mmc_host *mmc) > { > + struct tmio_mmc_host *host = mmc_priv(mmc); > struct renesas_sdhi *priv = host_to_priv(host); > > sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, ~CLK_CTL_SCLKEN & > @@ -376,8 +377,9 @@ static void renesas_sdhi_reset_scc(struct tmio_mmc_host *host, > SH_MOBILE_SDHI_SCC_CKSEL)); > } > > -static void renesas_sdhi_disable_scc(struct tmio_mmc_host *host) > +static void renesas_sdhi_disable_scc(struct mmc_host *mmc) > { > + struct tmio_mmc_host *host = mmc_priv(mmc); > struct renesas_sdhi *priv = host_to_priv(host); > > renesas_sdhi_reset_scc(host, priv); > @@ -412,9 +414,12 @@ static void renesas_sdhi_reset_hs400_mode(struct tmio_mmc_host *host, > sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL)); > } > > -static void renesas_sdhi_prepare_hs400_tuning(struct tmio_mmc_host *host) > +static int renesas_sdhi_prepare_hs400_tuning(struct mmc_host *mmc, struct mmc_ios *ios) > { > + struct tmio_mmc_host *host = mmc_priv(mmc); > + > renesas_sdhi_reset_hs400_mode(host, host_to_priv(host)); > + return 0; > } > > #define SH_MOBILE_SDHI_MAX_TAP 3 > @@ -899,10 +904,9 @@ int renesas_sdhi_probe(struct platform_device *pdev, > > host->execute_tuning = renesas_sdhi_execute_tuning; > host->check_retune = renesas_sdhi_check_scc_error; > - host->prepare_hs400_tuning = > - renesas_sdhi_prepare_hs400_tuning; > - host->hs400_downgrade = renesas_sdhi_disable_scc; > - host->hs400_complete = renesas_sdhi_hs400_complete; > + host->ops.prepare_hs400_tuning = renesas_sdhi_prepare_hs400_tuning; > + host->ops.hs400_downgrade = renesas_sdhi_disable_scc; Would it make sens to rename renesas_sdhi_disable_scc() to renesas_sdhi_hs400_downgrade() to fit the pattern that it's called from the mmc_ops? With or without this nit, Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > + host->ops.hs400_complete = renesas_sdhi_hs400_complete; > } > > num_irqs = platform_irq_count(pdev); > diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c > index aeafa1c68ce2..a2415fb5dde0 100644 > --- a/drivers/mmc/host/tmio_mmc_core.c > +++ b/drivers/mmc/host/tmio_mmc_core.c > @@ -997,34 +997,7 @@ static int tmio_multi_io_quirk(struct mmc_card *card, > return blk_size; > } > > -static int tmio_mmc_prepare_hs400_tuning(struct mmc_host *mmc, > - struct mmc_ios *ios) > -{ > - struct tmio_mmc_host *host = mmc_priv(mmc); > - > - if (host->prepare_hs400_tuning) > - host->prepare_hs400_tuning(host); > - > - return 0; > -} > - > -static void tmio_mmc_hs400_downgrade(struct mmc_host *mmc) > -{ > - struct tmio_mmc_host *host = mmc_priv(mmc); > - > - if (host->hs400_downgrade) > - host->hs400_downgrade(host); > -} > - > -static void tmio_mmc_hs400_complete(struct mmc_host *mmc) > -{ > - struct tmio_mmc_host *host = mmc_priv(mmc); > - > - if (host->hs400_complete) > - host->hs400_complete(host); > -} > - > -static const struct mmc_host_ops tmio_mmc_ops = { > +static struct mmc_host_ops tmio_mmc_ops = { > .request = tmio_mmc_request, > .set_ios = tmio_mmc_set_ios, > .get_ro = tmio_mmc_get_ro, > @@ -1033,9 +1006,6 @@ static const struct mmc_host_ops tmio_mmc_ops = { > .multi_io_quirk = tmio_multi_io_quirk, > .hw_reset = tmio_mmc_hw_reset, > .execute_tuning = tmio_mmc_execute_tuning, > - .prepare_hs400_tuning = tmio_mmc_prepare_hs400_tuning, > - .hs400_downgrade = tmio_mmc_hs400_downgrade, > - .hs400_complete = tmio_mmc_hs400_complete, > }; > > static int tmio_mmc_init_ocr(struct tmio_mmc_host *host) > -- > 2.20.1 > -- Regards, Niklas Söderlund
[-- Attachment #1: Type: text/plain, Size: 334 bytes --] Hi Niklas, thanks for the review! > > + ret = mmc_send_tuning(host->mmc, opcode, NULL); > > + if (ret == 0) > > The variable ret is only used here after the refactor so you could > possibly drop it and just check mmc_send_tuning() == 0. With or without Yeah, makes sense. I will fix it. Regards, Wolfram [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 504 bytes --] > > + host->ops.prepare_hs400_tuning = renesas_sdhi_prepare_hs400_tuning; > > + host->ops.hs400_downgrade = renesas_sdhi_disable_scc; > > Would it make sens to rename renesas_sdhi_disable_scc() to > renesas_sdhi_hs400_downgrade() to fit the pattern that it's called from > the mmc_ops? I also thought about it and came to the conclusion that this is more readable. To downgrade from HS400, we need to disable SCC. But no big deal to change it if you/others think the other pattern... [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
On Wed, 29 Jan 2020 at 21:37, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> TAP and SCC handling is Renesas specific, so it should be moved to the
> SDHI driver. After previous refactoring, this is possible now. And
> feasible, too, to simplify further HS400 corrections. IMHO it also makes
> the driver less complicated.
>
> See the patches why this series is still RFC.
>
> This is based on top of the series:
> [RFC PATCH v2 0/5] mmc: renesas_sdhi: add manual correction
>
> A branch can be found here:
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/sdhi/new_manual_calib
>
> It has been tested with a Renesas Salvator-XS boards, one with a R-Car
> M3-N and one with H3-ES2.0. Tuning to HS400 still works.
>
>
> Wolfram Sang (6):
> mmc: tmio: refactor tuning execution into SDHI driver
> mmc: renesas_sdhi: complain loudly if driver needs update
> mmc: tmio: give callback a generic name
> mmc: tmio: enforce retune after runtime suspend
> mmc: tmio: factor out TAP usage
> mmc: tmio: remove superfluous callback wrappers
>
> drivers/mmc/host/renesas_sdhi.h | 5 ++
> drivers/mmc/host/renesas_sdhi_core.c | 90 +++++++++++++++++-----------
> drivers/mmc/host/tmio_mmc.h | 11 +---
> drivers/mmc/host/tmio_mmc_core.c | 77 +++---------------------
> 4 files changed, 71 insertions(+), 112 deletions(-)
>
> --
> 2.20.1
>
Wolfram, I realize that there were potentially a few minor nitpicks to
address, but let's do that on top.
So, applied for next, thanks!
Kind regards
Uffe