From: Shawn Lin <shawn.lin@rock-chips.com>
To: Jaehoon Chung <jh80.chung@samsung.com>,
Ulf Hansson <ulf.hansson@linaro.org>
Cc: shawn.lin@rock-chips.com, linux-mmc@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/3] Convert dw_mci_pltfm_prepare_command into a library call
Date: Wed, 20 Jan 2016 17:21:27 +0800 [thread overview]
Message-ID: <569F5197.8050905@rock-chips.com> (raw)
In-Reply-To: <569F44BC.4010205@samsung.com>
On 2016/1/20 16:26, Jaehoon Chung wrote:
> Hi Shawn.
>
> On 01/19/2016 12:39 PM, Shawn Lin wrote:
>> This patchset expose dw_mci_pltfm_prepare_command towards to
>> library call and remove the redundant code from dw_mmc-rockchip.
>> Meanwhile we make dw_mmc-exynos to call it instead of changing cmdr
>> in its variant drivers.
>>
>
> Actually, I want to remove *_prepare_command from entire dw-mmc driver.
> Since exynos used USE_HOLD_REG bit in case of special condition, maybe you doesn't remove all, right?
>
Hi Jaehoon,
right, the only thing I can see in prepare_command hook is using
USE_HOLD_REG bit.
Rockchip soc : all of them need USE_HOLD_REG.
pistachio platform: all of them need it refering to the code.
dw_mmc-k3 and dw_mmc-pci: never use this bit.
exynos: used in special case.
So if we remove the prepare_command from entire dw-mmc driver, maybe we
should add a new flag in struct dw_mci for all the compatible platform
to decide whether they need use it or not. It that the right way? :)
> I'm figuring out the using the generic solution.
> Before that, how about this? I want to know your opinion.
>
> diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
> index 3a7e835..ed08e4b 100644
> --- a/drivers/mmc/host/dw_mmc-exynos.c
> +++ b/drivers/mmc/host/dw_mmc-exynos.c
> @@ -145,6 +145,16 @@ static void dw_mci_exynos_set_clksel_timing(struct dw_mci *host, u32 timing)
> mci_writel(host, CLKSEL64, clksel);
> else
> mci_writel(host, CLKSEL, clksel);
> +
> + /*
> + * Exynos4412 and Exynos5250 extends the use of CMD register with the
> + * use of bit 29 (which is reserved on standard MSHC controllers) for
> + * optionally bypassing the HOLD register for command and data. The
> + * HOLD register should be bypassed in case there is no phase shift
> + * applied on CMD/DATA that is sent to the card.
> + */
> + if (!SDMMC_CLKSEL_GET_DRV_WD3(clksel))
> + host->no_use_hold = 1;
> }
>
> #ifdef CONFIG_PM_SLEEP
> @@ -202,26 +212,6 @@ static int dw_mci_exynos_resume_noirq(struct device *dev)
> #define dw_mci_exynos_resume_noirq NULL
> #endif /* CONFIG_PM_SLEEP */
>
> -static void dw_mci_exynos_prepare_command(struct dw_mci *host, u32 *cmdr)
> -{
> - struct dw_mci_exynos_priv_data *priv = host->priv;
> - /*
> - * Exynos4412 and Exynos5250 extends the use of CMD register with the
> - * use of bit 29 (which is reserved on standard MSHC controllers) for
> - * optionally bypassing the HOLD register for command and data. The
> - * HOLD register should be bypassed in case there is no phase shift
> - * applied on CMD/DATA that is sent to the card.
> - */
> - if (priv->ctrl_type == DW_MCI_TYPE_EXYNOS7 ||
> - priv->ctrl_type == DW_MCI_TYPE_EXYNOS7_SMU) {
> - if (SDMMC_CLKSEL_GET_DRV_WD3(mci_readl(host, CLKSEL64)))
> - *cmdr |= SDMMC_CMD_USE_HOLD_REG;
> - } else {
> - if (SDMMC_CLKSEL_GET_DRV_WD3(mci_readl(host, CLKSEL)))
> - *cmdr |= SDMMC_CMD_USE_HOLD_REG;
> - }
> -}
> -
> static void dw_mci_exynos_config_hs400(struct dw_mci *host, u32 timing)
> {
> struct dw_mci_exynos_priv_data *priv = host->priv;
> @@ -500,7 +490,6 @@ static const struct dw_mci_drv_data exynos_drv_data = {
> .caps = exynos_dwmmc_caps,
> .init = dw_mci_exynos_priv_init,
> .setup_clock = dw_mci_exynos_setup_clock,
> - .prepare_command = dw_mci_exynos_prepare_command,
> .set_ios = dw_mci_exynos_set_ios,
> .parse_dt = dw_mci_exynos_parse_dt,
> .execute_tuning = dw_mci_exynos_execute_tuning,
> diff --git a/drivers/mmc/host/dw_mmc-pltfm.c b/drivers/mmc/host/dw_mmc-pltfm.c
> index 81bdeeb..c0bb0c7 100644
> --- a/drivers/mmc/host/dw_mmc-pltfm.c
> +++ b/drivers/mmc/host/dw_mmc-pltfm.c
> @@ -26,19 +26,6 @@
> #include "dw_mmc.h"
> #include "dw_mmc-pltfm.h"
>
> -static void dw_mci_pltfm_prepare_command(struct dw_mci *host, u32 *cmdr)
> -{
> - *cmdr |= SDMMC_CMD_USE_HOLD_REG;
> -}
> -
> -static const struct dw_mci_drv_data socfpga_drv_data = {
> - .prepare_command = dw_mci_pltfm_prepare_command,
> -};
> -
> -static const struct dw_mci_drv_data pistachio_drv_data = {
> - .prepare_command = dw_mci_pltfm_prepare_command,
> -};
> -
> int dw_mci_pltfm_register(struct platform_device *pdev,
> const struct dw_mci_drv_data *drv_data)
> {
> @@ -94,10 +81,8 @@ EXPORT_SYMBOL_GPL(dw_mci_pltfm_pmops);
>
> static const struct of_device_id dw_mci_pltfm_match[] = {
> { .compatible = "snps,dw-mshc", },
> - { .compatible = "altr,socfpga-dw-mshc",
> - .data = &socfpga_drv_data },
> - { .compatible = "img,pistachio-dw-mshc",
> - .data = &pistachio_drv_data },
> + { .compatible = "altr,socfpga-dw-mshc", },
> + { .compatible = "img,pistachio-dw-mshc", },
> {},
> };
> MODULE_DEVICE_TABLE(of, dw_mci_pltfm_match);
> diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
> index d9c92f3..84e50f3 100644
> --- a/drivers/mmc/host/dw_mmc-rockchip.c
> +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> @@ -26,11 +26,6 @@ struct dw_mci_rockchip_priv_data {
> int default_sample_phase;
> };
>
> -static void dw_mci_rockchip_prepare_command(struct dw_mci *host, u32 *cmdr)
> -{
> - *cmdr |= SDMMC_CMD_USE_HOLD_REG;
> -}
> -
> static int dw_mci_rk3288_setup_clock(struct dw_mci *host)
> {
> host->bus_hz /= RK3288_CLKGEN_DIV;
> @@ -240,12 +235,10 @@ static int dw_mci_rockchip_init(struct dw_mci *host)
> }
>
> static const struct dw_mci_drv_data rk2928_drv_data = {
> - .prepare_command = dw_mci_rockchip_prepare_command,
> .init = dw_mci_rockchip_init,
> };
>
> static const struct dw_mci_drv_data rk3288_drv_data = {
> - .prepare_command = dw_mci_rockchip_prepare_command,
> .set_ios = dw_mci_rk3288_set_ios,
> .execute_tuning = dw_mci_rk3288_execute_tuning,
> .parse_dt = dw_mci_rk3288_parse_dt,
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index ffef24a..844f8cf 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -234,7 +234,6 @@ static u32 dw_mci_prepare_command(struct mmc_host *mmc, struct mmc_command *cmd)
> struct mmc_data *data;
> struct dw_mci_slot *slot = mmc_priv(mmc);
> struct dw_mci *host = slot->host;
> - const struct dw_mci_drv_data *drv_data = slot->host->drv_data;
> u32 cmdr;
>
> cmd->error = -EINPROGRESS;
> @@ -296,8 +295,8 @@ static u32 dw_mci_prepare_command(struct mmc_host *mmc, struct mmc_command *cmd)
> cmdr |= SDMMC_CMD_DAT_WR;
> }
>
> - if (drv_data && drv_data->prepare_command)
> - drv_data->prepare_command(slot->host, &cmdr);
> + if (!host->no_use_hold)
> + cmdr |= SDMMC_CMD_USE_HOLD_REG;
>
> return cmdr;
> }
> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
> index a14b7fc..c0fc0e3 100644
> --- a/drivers/mmc/host/dw_mmc.h
> +++ b/drivers/mmc/host/dw_mmc.h
> @@ -277,7 +277,6 @@ struct dw_mci_slot {
> * @caps: mmc subsystem specified capabilities of the controller(s).
> * @init: early implementation specific initialization.
> * @setup_clock: implementation specific clock configuration.
> - * @prepare_command: handle CMD register extensions.
> * @set_ios: handle bus specific extensions.
> * @parse_dt: parse implementation specific device tree properties.
> * @execute_tuning: implementation specific tuning procedure.
> @@ -290,7 +289,6 @@ struct dw_mci_drv_data {
> unsigned long *caps;
> int (*init)(struct dw_mci *host);
> int (*setup_clock)(struct dw_mci *host);
> - void (*prepare_command)(struct dw_mci *host, u32 *cmdr);
> void (*set_ios)(struct dw_mci *host, struct mmc_ios *ios);
> int (*parse_dt)(struct dw_mci *host);
> int (*execute_tuning)(struct dw_mci_slot *slot, u32 opcode);
> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
> index 89df7ab..7b1416a 100644
> --- a/include/linux/mmc/dw_mmc.h
> +++ b/include/linux/mmc/dw_mmc.h
> @@ -221,6 +221,8 @@ struct dw_mci {
>
> struct timer_list cmd11_timer;
> struct timer_list dto_timer;
> +
> + int no_use_hold;
> };
>
> /* DMA ops for Internal/External DMAC interface */
>>
>>
>> Shawn Lin (3):
>> mmc: dw_mmc: expose dw_mci_pltfm_prepare_command
>> mmc: dw_mmc-rockchip: remove prepare_command hook
>> mmc: dw_mmc-exynos: wrapper prepare_command
>>
>> drivers/mmc/host/dw_mmc-exynos.c | 4 ++--
>> drivers/mmc/host/dw_mmc-pltfm.c | 3 ++-
>> drivers/mmc/host/dw_mmc-pltfm.h | 2 +-
>> drivers/mmc/host/dw_mmc-rockchip.c | 9 ++-------
>> 4 files changed, 7 insertions(+), 11 deletions(-)
>>
>
>
>
>
--
Best Regards
Shawn Lin
next prev parent reply other threads:[~2016-01-20 9:21 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-01-19 3:39 [PATCH 0/3] Convert dw_mci_pltfm_prepare_command into a library call Shawn Lin
2016-01-19 3:40 ` [PATCH 1/3] mmc: dw_mmc: expose dw_mci_pltfm_prepare_command Shawn Lin
2016-01-19 3:52 ` [PATCH 2/3] mmc: dw_mmc-rockchip: remove prepare_command hook Shawn Lin
2016-01-19 3:53 ` [PATCH 3/3] mmc: dw_mmc-exynos: wrapper prepare_command Shawn Lin
2016-01-19 4:07 ` [PATCH 0/3] Convert dw_mci_pltfm_prepare_command into a library call Jaehoon Chung
2016-01-20 8:26 ` Jaehoon Chung
2016-01-20 9:21 ` Shawn Lin [this message]
2016-01-20 12:35 ` Shawn Lin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=569F5197.8050905@rock-chips.com \
--to=shawn.lin@rock-chips.com \
--cc=jh80.chung@samsung.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mmc@vger.kernel.org \
--cc=ulf.hansson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.