* Re: [PATCH 04/12] mmc: sdhci-omap: Add tuning support
From: Adrian Hunter @ 2017-12-21 9:09 UTC (permalink / raw)
To: Kishon Vijay Abraham I, Ulf Hansson, Rob Herring, Tony Lindgren
Cc: Mark Rutland, Russell King, linux-mmc, devicetree, linux-kernel,
linux-omap, linux-arm-kernel, nsekhar
In-Reply-To: <20171214130941.26666-5-kishon@ti.com>
On 14/12/17 15:09, Kishon Vijay Abraham I wrote:
> MMC tuning procedure is required to support SD card
> UHS1-SDR104 mode and EMMC HS200 mode.
>
> SDR104/HS200 DLL Tuning Procedure for AM572x platform is mentioned
> in Figure 25-51. SDR104/HS200 DLL Tuning Procedure of
> AM572x Sitara Processors Silicon Revision 2.0, 1.1 TRM
> (SPRUHZ6I - October 2014–Revised April 2017 [1]).
>
> The tuning function sdhci_omap_execute_tuning() will only be
> called by the MMC/SD core if the corresponding speed modes
> are supported by the OMAP silicon which is set in the mmc
> host "caps" field.
>
> [1] -> http://www.ti.com/lit/ug/spruhz6i/spruhz6i.pdf
>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Apart from 1 minor comment below:
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
> drivers/mmc/host/sdhci-omap.c | 130 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 130 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
> index 8f7239e2edc2..df8a0a472996 100644
> --- a/drivers/mmc/host/sdhci-omap.c
> +++ b/drivers/mmc/host/sdhci-omap.c
> @@ -37,6 +37,13 @@
> #define CON_INIT BIT(1)
> #define CON_OD BIT(0)
>
> +#define SDHCI_OMAP_DLL 0x0134
> +#define DLL_SWT BIT(20)
> +#define DLL_FORCE_SR_C_SHIFT 13
> +#define DLL_FORCE_SR_C_MASK (0x7f << DLL_FORCE_SR_C_SHIFT)
> +#define DLL_FORCE_VALUE BIT(12)
> +#define DLL_CALIB BIT(1)
> +
> #define SDHCI_OMAP_CMD 0x20c
>
> #define SDHCI_OMAP_PSTATE 0x0224
> @@ -66,12 +73,16 @@
>
> #define SDHCI_OMAP_AC12 0x23c
> #define AC12_V1V8_SIGEN BIT(19)
> +#define AC12_SCLK_SEL BIT(23)
>
> #define SDHCI_OMAP_CAPA 0x240
> #define CAPA_VS33 BIT(24)
> #define CAPA_VS30 BIT(25)
> #define CAPA_VS18 BIT(26)
>
> +#define SDHCI_OMAP_CAPA2 0x0244
> +#define CAPA2_TSDR50 BIT(13)
> +
> #define SDHCI_OMAP_TIMEOUT 1 /* 1 msec */
>
> #define SYSCTL_CLKD_MAX 0x3FF
> @@ -80,6 +91,8 @@
> #define IOV_3V0 3000000 /* 300000 uV */
> #define IOV_3V3 3300000 /* 330000 uV */
>
> +#define MAX_PHASE_DELAY 0x7C
> +
> struct sdhci_omap_data {
> u32 offset;
> };
> @@ -204,6 +217,120 @@ static void sdhci_omap_conf_bus_power(struct sdhci_omap_host *omap_host,
> }
> }
>
> +static inline void sdhci_omap_set_dll(struct sdhci_omap_host *omap_host,
> + int count)
> +{
> + int i;
> + u32 reg;
> +
> + reg = sdhci_omap_readl(omap_host, SDHCI_OMAP_DLL);
> + reg |= DLL_FORCE_VALUE;
> + reg &= ~DLL_FORCE_SR_C_MASK;
> + reg |= (count << DLL_FORCE_SR_C_SHIFT);
> + sdhci_omap_writel(omap_host, SDHCI_OMAP_DLL, reg);
> +
> + reg |= DLL_CALIB;
> + sdhci_omap_writel(omap_host, SDHCI_OMAP_DLL, reg);
> + for (i = 0; i < 1000; i++) {
> + reg = sdhci_omap_readl(omap_host, SDHCI_OMAP_DLL);
> + if (reg & DLL_CALIB)
> + break;
> + }
> + reg &= ~DLL_CALIB;
> + sdhci_omap_writel(omap_host, SDHCI_OMAP_DLL, reg);
> +}
> +
> +static void sdhci_omap_disable_tuning(struct sdhci_omap_host *omap_host)
> +{
> + u32 reg;
> +
> + reg = sdhci_omap_readl(omap_host, SDHCI_OMAP_AC12);
> + reg &= ~AC12_SCLK_SEL;
> + sdhci_omap_writel(omap_host, SDHCI_OMAP_AC12, reg);
> +
> + reg = sdhci_omap_readl(omap_host, SDHCI_OMAP_DLL);
> + reg &= ~(DLL_FORCE_VALUE | DLL_SWT);
> + sdhci_omap_writel(omap_host, SDHCI_OMAP_DLL, reg);
> +}
> +
> +static int sdhci_omap_execute_tuning(struct mmc_host *mmc, u32 opcode)
> +{
> + u32 reg;
> + int ret = 0;
> + u8 cur_match, prev_match = 0;
> + u32 phase_delay = 0;
> + u32 start_window = 0, max_window = 0;
> + u32 length = 0, max_len = 0;
> + struct mmc_ios *ios = &mmc->ios;
> + struct sdhci_host *host = mmc_priv(mmc);
> + struct sdhci_pltfm_host *pltfm_host;
> + struct sdhci_omap_host *omap_host;
> + struct device *dev;
> +
> + pltfm_host = sdhci_priv(host);
> + omap_host = sdhci_pltfm_priv(pltfm_host);
> + dev = omap_host->dev;
These initializations can be combined with the declarations. Also try to
arrange local variable declaration lines in descending order of length e.g.
struct sdhci_host *host = mmc_priv(mmc);
struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
struct device *dev = omap_host->dev;
struct mmc_ios *ios = &mmc->ios;
u32 start_window = 0, max_window = 0;
u8 cur_match, prev_match = 0;
u32 length = 0, max_len = 0;
u32 phase_delay = 0;
int ret = 0;
u32 reg;
> +
> + /* clock tuning is not needed for upto 52MHz */
> + if (ios->clock <= 52000000)
> + return 0;
> +
> + reg = sdhci_omap_readl(omap_host, SDHCI_OMAP_CAPA2);
> + if (ios->timing == MMC_TIMING_UHS_SDR50 && !(reg & CAPA2_TSDR50))
> + return 0;
> +
> + reg = sdhci_omap_readl(omap_host, SDHCI_OMAP_DLL);
> + reg |= DLL_SWT;
> + sdhci_omap_writel(omap_host, SDHCI_OMAP_DLL, reg);
> +
> + while (phase_delay <= MAX_PHASE_DELAY) {
> + sdhci_omap_set_dll(omap_host, phase_delay);
> +
> + cur_match = !mmc_send_tuning(mmc, opcode, NULL);
> + if (cur_match) {
> + if (prev_match) {
> + length++;
> + } else {
> + start_window = phase_delay;
> + length = 1;
> + }
> + }
> +
> + if (length > max_len) {
> + max_window = start_window;
> + max_len = length;
> + }
> +
> + prev_match = cur_match;
> + phase_delay += 4;
> + }
> +
> + if (!max_len) {
> + dev_err(dev, "Unable to find match\n");
> + ret = -EIO;
> + goto tuning_error;
> + }
> +
> + reg = sdhci_omap_readl(omap_host, SDHCI_OMAP_AC12);
> + if (!(reg & AC12_SCLK_SEL)) {
> + ret = -EIO;
> + goto tuning_error;
> + }
> +
> + phase_delay = max_window + 4 * (max_len >> 1);
> + sdhci_omap_set_dll(omap_host, phase_delay);
> +
> + goto ret;
> +
> +tuning_error:
> + dev_err(dev, "Tuning failed\n");
> + sdhci_omap_disable_tuning(omap_host);
> +
> +ret:
> + sdhci_reset(host, SDHCI_RESET_CMD | SDHCI_RESET_DATA);
> + return ret;
> +}
> +
> static int sdhci_omap_card_busy(struct mmc_host *mmc)
> {
> int i;
> @@ -312,6 +439,8 @@ static int sdhci_omap_start_signal_voltage_switch(struct mmc_host *mmc,
> static void sdhci_omap_set_power_mode(struct sdhci_omap_host *omap_host,
> u8 power_mode)
> {
> + if (omap_host->bus_mode == MMC_POWER_OFF)
> + sdhci_omap_disable_tuning(omap_host);
> omap_host->power_mode = power_mode;
> }
>
> @@ -648,6 +777,7 @@ static int sdhci_omap_probe(struct platform_device *pdev)
> sdhci_omap_start_signal_voltage_switch;
> host->mmc_host_ops.set_ios = sdhci_omap_set_ios;
> host->mmc_host_ops.card_busy = sdhci_omap_card_busy;
> + host->mmc_host_ops.execute_tuning = sdhci_omap_execute_tuning;
>
> sdhci_read_caps(host);
> host->caps |= SDHCI_CAN_DO_ADMA2;
>
^ permalink raw reply
* Re: [PATCH 03/12] mmc: sdhci-omap: Add custom set_uhs_signaling sdhci_host ops
From: Adrian Hunter @ 2017-12-21 9:01 UTC (permalink / raw)
To: Kishon Vijay Abraham I, Ulf Hansson, Rob Herring, Tony Lindgren
Cc: Mark Rutland, Russell King, linux-mmc, devicetree, linux-kernel,
linux-omap, linux-arm-kernel, nsekhar
In-Reply-To: <20171214130941.26666-4-kishon@ti.com>
On 14/12/17 15:09, Kishon Vijay Abraham I wrote:
> UHS-1 DDR50 and MMC DDR52 mode require DDR bit to be
> set in the configuration register (MMCHS_CON). Add
> sdhci-omap specific set_uhs_signaling ops to set
> this bit. Also while setting the UHSMS bit, clock should be
> disabled.
>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Apart from 1 minor comment below:
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
> drivers/mmc/host/sdhci-omap.c | 26 +++++++++++++++++++++++++-
> 1 file changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
> index defe4eac020d..8f7239e2edc2 100644
> --- a/drivers/mmc/host/sdhci-omap.c
> +++ b/drivers/mmc/host/sdhci-omap.c
> @@ -31,6 +31,7 @@
> #define SDHCI_OMAP_CON 0x12c
> #define CON_DW8 BIT(5)
> #define CON_DMA_MASTER BIT(20)
> +#define CON_DDR BIT(19)
> #define CON_CLKEXTFREE BIT(16)
> #define CON_PADEN BIT(15)
> #define CON_INIT BIT(1)
> @@ -93,6 +94,9 @@ struct sdhci_omap_host {
> u8 power_mode;
> };
>
> +static void sdhci_omap_start_clock(struct sdhci_omap_host *omap_host);
> +static void sdhci_omap_stop_clock(struct sdhci_omap_host *omap_host);
These forward declarations aren't needed are they.
> +
> static inline u32 sdhci_omap_readl(struct sdhci_omap_host *host,
> unsigned int offset)
> {
> @@ -471,6 +475,26 @@ static void sdhci_omap_init_74_clocks(struct sdhci_host *host, u8 power_mode)
> enable_irq(host->irq);
> }
>
> +static void sdhci_omap_set_uhs_signaling(struct sdhci_host *host,
> + unsigned int timing)
> +{
> + u32 reg;
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
> +
> + sdhci_omap_stop_clock(omap_host);
> +
> + reg = sdhci_omap_readl(omap_host, SDHCI_OMAP_CON);
> + if (timing == MMC_TIMING_UHS_DDR50 || timing == MMC_TIMING_MMC_DDR52)
> + reg |= CON_DDR;
> + else
> + reg &= ~CON_DDR;
> + sdhci_omap_writel(omap_host, SDHCI_OMAP_CON, reg);
> +
> + sdhci_set_uhs_signaling(host, timing);
> + sdhci_omap_start_clock(omap_host);
> +}
> +
> static struct sdhci_ops sdhci_omap_ops = {
> .set_clock = sdhci_omap_set_clock,
> .set_power = sdhci_omap_set_power,
> @@ -480,7 +504,7 @@ static struct sdhci_ops sdhci_omap_ops = {
> .set_bus_width = sdhci_omap_set_bus_width,
> .platform_send_init_74_clocks = sdhci_omap_init_74_clocks,
> .reset = sdhci_reset,
> - .set_uhs_signaling = sdhci_set_uhs_signaling,
> + .set_uhs_signaling = sdhci_omap_set_uhs_signaling,
> };
>
> static int sdhci_omap_set_capabilities(struct sdhci_omap_host *omap_host)
>
^ permalink raw reply
* Re: [PATCH 02/12] mmc: sdhci-omap: Add card_busy host ops
From: Adrian Hunter @ 2017-12-21 8:59 UTC (permalink / raw)
To: Kishon Vijay Abraham I, Ulf Hansson, Rob Herring, Tony Lindgren
Cc: Mark Rutland, Russell King, linux-mmc, devicetree, linux-kernel,
linux-omap, linux-arm-kernel, nsekhar
In-Reply-To: <20171214130941.26666-3-kishon@ti.com>
On 14/12/17 15:09, Kishon Vijay Abraham I wrote:
> card_busy ops is used by mmc core in
> 1) mmc_set_uhs_voltage to verify voltage switch
> 2) __mmc_start_request/mmc_poll_for_busy to check the card busy status
>
> While only DAT0 can be used to check the card busy status (in '2' above),
> CMD and DAT[0..3] is used to verify voltage switch (in '1' above).
>
> The voltage switching sequence for AM572x platform is mentioned
> in Figure 25-48. eMMC/SD/SDIO Power Switching Procedure of
> AM572x Sitara Processors Silicon Revision 2.0, 1.1 TRM
> (SPRUHZ6I - October 2014–Revised April 2017 [1]).
>
> Add card_busy host ops in sdhci_omap that checks for both CMD and
> DAT[0..3]. card_busy here returns true if one of CMD and DAT[0..3] is
> low though during voltage switch sequence all of CMD and DAT[0..3] has
> to be low (however haven't observed a case where some DAT lines are low
> and some are high).
Isn't it better to check DAT0 only since that is all that is defined for 'busy'.
>
> In the voltage switching sequence, CLKEXTFREE bit in MMCHS_CON
> should also be set after switching to 1.8v which is also taken
> care in the card_busy ops.
>
> [1] -> http://www.ti.com/lit/ug/spruhz6i/spruhz6i.pdf
>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
> drivers/mmc/host/sdhci-omap.c | 62 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
> index 96985786cadf..defe4eac020d 100644
> --- a/drivers/mmc/host/sdhci-omap.c
> +++ b/drivers/mmc/host/sdhci-omap.c
> @@ -31,11 +31,20 @@
> #define SDHCI_OMAP_CON 0x12c
> #define CON_DW8 BIT(5)
> #define CON_DMA_MASTER BIT(20)
> +#define CON_CLKEXTFREE BIT(16)
> +#define CON_PADEN BIT(15)
> #define CON_INIT BIT(1)
> #define CON_OD BIT(0)
>
> #define SDHCI_OMAP_CMD 0x20c
>
> +#define SDHCI_OMAP_PSTATE 0x0224
> +#define PSTATE_CLEV BIT(24)
> +#define PSTATE_DLEV_SHIFT 20
> +#define PSTATE_DLEV_DAT(x) (1 << (PSTATE_DLEV_SHIFT + (x)))
> +#define PSTATE_DLEV (PSTATE_DLEV_DAT(0) | PSTATE_DLEV_DAT(1) | \
> + PSTATE_DLEV_DAT(2) | PSTATE_DLEV_DAT(3))
> +
> #define SDHCI_OMAP_HCTL 0x228
> #define HCTL_SDBP BIT(8)
> #define HCTL_SDVS_SHIFT 9
> @@ -191,6 +200,58 @@ static void sdhci_omap_conf_bus_power(struct sdhci_omap_host *omap_host,
> }
> }
>
> +static int sdhci_omap_card_busy(struct mmc_host *mmc)
> +{
> + int i;
> + u32 reg, ac12;
> + int ret = true;
> + struct sdhci_host *host = mmc_priv(mmc);
> + struct sdhci_pltfm_host *pltfm_host;
> + struct sdhci_omap_host *omap_host;
> + u32 ier = host->ier;
> +
> + pltfm_host = sdhci_priv(host);
> + omap_host = sdhci_pltfm_priv(pltfm_host);
> +
> + reg = sdhci_omap_readl(omap_host, SDHCI_OMAP_CON);
> + ac12 = sdhci_omap_readl(omap_host, SDHCI_OMAP_AC12);
> + reg &= ~CON_CLKEXTFREE;
> + if (ac12 & AC12_V1V8_SIGEN)
> + reg |= CON_CLKEXTFREE;
> + reg |= CON_PADEN;
> + sdhci_omap_writel(omap_host, SDHCI_OMAP_CON, reg);
> +
> + disable_irq(host->irq);
> + ier |= SDHCI_INT_CARD_INT;
> + sdhci_writel(host, ier, SDHCI_INT_ENABLE);
> + sdhci_writel(host, ier, SDHCI_SIGNAL_ENABLE);
> +
> + for (i = 0; i < 5; i++) {
> + /*
> + * Delay is required for PSTATE to correctly reflect
> + * DLEV/CLEV values after PADEM is set.
> + */
> + usleep_range(100, 200);
> + reg = sdhci_omap_readl(omap_host, SDHCI_OMAP_PSTATE);
> + if ((reg & PSTATE_CLEV) &&
> + ((reg & PSTATE_DLEV) == PSTATE_DLEV)) {
> + ret = false;
> + goto ret;
'break' is better than 'goto'
> + }
> + }
> +
> +ret:
> + reg = sdhci_omap_readl(omap_host, SDHCI_OMAP_CON);
> + reg &= ~(CON_CLKEXTFREE | CON_PADEN);
> + sdhci_omap_writel(omap_host, SDHCI_OMAP_CON, reg);
> +
> + sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
> + sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
> + enable_irq(host->irq);
> +
> + return ret;
> +}
> +
> static int sdhci_omap_start_signal_voltage_switch(struct mmc_host *mmc,
> struct mmc_ios *ios)
> {
> @@ -562,6 +623,7 @@ static int sdhci_omap_probe(struct platform_device *pdev)
> host->mmc_host_ops.start_signal_voltage_switch =
> sdhci_omap_start_signal_voltage_switch;
> host->mmc_host_ops.set_ios = sdhci_omap_set_ios;
> + host->mmc_host_ops.card_busy = sdhci_omap_card_busy;
>
> sdhci_read_caps(host);
> host->caps |= SDHCI_CAN_DO_ADMA2;
>
^ permalink raw reply
* Re: [PATCH 01/12] mmc: sdhci-omap: Update 'power_mode' outside sdhci_omap_init_74_clocks
From: Adrian Hunter @ 2017-12-21 8:57 UTC (permalink / raw)
To: Kishon Vijay Abraham I, Ulf Hansson, Rob Herring, Tony Lindgren
Cc: Mark Rutland, Russell King, linux-mmc, devicetree, linux-kernel,
linux-omap, linux-arm-kernel, nsekhar
In-Reply-To: <20171214130941.26666-2-kishon@ti.com>
On 14/12/17 15:09, Kishon Vijay Abraham I wrote:
> Updating 'power_mode' in sdhci_omap_init_74_clocks results in
> 'power_mode' never updated to MMC_POWER_OFF during card
> removal. This results in initialization sequence not sent to the
> card during re-insertion.
> Fix it here by adding sdhci_omap_set_power_mode to update power_mode.
> This function can also be used later to perform operations that
> are specific to a power mode (e.g, disable tuning during power off).
>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
> drivers/mmc/host/sdhci-omap.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
> index 628bfe9a3d17..96985786cadf 100644
> --- a/drivers/mmc/host/sdhci-omap.c
> +++ b/drivers/mmc/host/sdhci-omap.c
> @@ -244,6 +244,12 @@ static int sdhci_omap_start_signal_voltage_switch(struct mmc_host *mmc,
> return 0;
> }
>
> +static void sdhci_omap_set_power_mode(struct sdhci_omap_host *omap_host,
> + u8 power_mode)
> +{
> + omap_host->power_mode = power_mode;
> +}
> +
> static void sdhci_omap_set_bus_mode(struct sdhci_omap_host *omap_host,
> unsigned int mode)
> {
> @@ -273,6 +279,7 @@ static void sdhci_omap_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>
> sdhci_omap_set_bus_mode(omap_host, ios->bus_mode);
> sdhci_set_ios(mmc, ios);
> + sdhci_omap_set_power_mode(omap_host, ios->power_mode);
> }
>
> static u16 sdhci_omap_calc_divisor(struct sdhci_pltfm_host *host,
> @@ -401,8 +408,6 @@ static void sdhci_omap_init_74_clocks(struct sdhci_host *host, u8 power_mode)
> sdhci_omap_writel(omap_host, SDHCI_OMAP_STAT, INT_CC_EN);
>
> enable_irq(host->irq);
> -
> - omap_host->power_mode = power_mode;
> }
>
> static struct sdhci_ops sdhci_omap_ops = {
> @@ -504,6 +509,7 @@ static int sdhci_omap_probe(struct platform_device *pdev)
> omap_host->host = host;
> omap_host->base = host->ioaddr;
> omap_host->dev = dev;
> + omap_host->power_mode = MMC_POWER_UNDEFINED;
> host->ioaddr += offset;
>
> mmc = host->mmc;
>
^ permalink raw reply
* Re: [PATCH v6 2/4] ath6kl: change to use mmc api for accessing host supported maximum segment count and size
From: Kalle Valo @ 2017-12-20 15:40 UTC (permalink / raw)
To: Xinming Hu
Cc: Linux MMC, Ulf Hansson, Arend van Spriel, Franky Lin,
Hante Meuleman, Chi-Hsien Lin, Wright Feng, Zhiyuan Yang,
Tim Song, Cathy Luo, James Cao, Ganapathi Bhat, Xu Wang, Bob Tan
In-Reply-To: <1513685211-640-2-git-send-email-huxm@marvell.com>
Xinming Hu <huxm@marvell.com> writes:
> Using mmc standard api to get the host sg capability.
>
> Signed-off-by: Xinming Hu <huxm@marvell.com>
> ---
> v6: separate driver patch from patch 1/4.
> ---
> drivers/net/wireless/ath/ath6kl/sdio.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
I assume this will go via some other tree and I should not take this.
So:
Acked-by: Kalle Valo <kvalo@qca.qualcomm.com>
--
Kalle Valo
^ permalink raw reply
* Re: [RFC PATCH 09/12] mmc: sdhci: Use software timer when timeout greater than hardware capablility
From: Adrian Hunter @ 2017-12-20 14:11 UTC (permalink / raw)
To: Kishon Vijay Abraham I, Ulf Hansson, Rob Herring, Tony Lindgren
Cc: Mark Rutland, Russell King, linux-mmc, devicetree, linux-kernel,
linux-omap, linux-arm-kernel, nsekhar
In-Reply-To: <20171214130941.26666-10-kishon@ti.com>
On 14/12/17 15:09, Kishon Vijay Abraham I wrote:
> Errata i834 in AM572x Sitara Processors Silicon Revision 2.0, 1.1
> (SPRZ429K July 2014–Revised March 2017 [1]) mentions
> Under high speed HS200 and SDR104 modes, the functional clock for MMC
> modules will reach up to 192 MHz. At this frequency, the maximum obtainable
> timeout (DTO = 0xE) through MMC host controller is (1/192MHz)*2^27 = 700ms.
> Commands taking longer than 700ms may be affected by this small window
> frame. Workaround for this errata is use a software timer instead of
> hardware timer to provide the delay requested by the upper layer.
>
> While this errata is specific to AM572x, it is applicable to all sdhci
> based controllers when a particular request require timeout greater
> than hardware capability.
It doesn't work for our controllers. Even if the data timeout interrupt is
disabled, it seems like the timeout still "happens" in some fashion - after
which the host controller starts misbehaving.
So you will need to add a quirk.
>
> Re-use the software timer already implemented in sdhci to program the
> correct timeout value and also disable the hardware timeout when
> the required timeout is greater than hardware capabiltiy in order to
> avoid spurious timeout interrupts.
>
> This patch is based on the earlier patch implemented for omap_hsmmc [2]
>
> [1] -> http://www.ti.com/lit/er/sprz429k/sprz429k.pdf
> [2] -> https://patchwork.kernel.org/patch/9791449/
>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
> drivers/mmc/host/sdhci.c | 41 +++++++++++++++++++++++++++++++++++++++--
> drivers/mmc/host/sdhci.h | 11 +++++++++++
> 2 files changed, 50 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index e9290a3439d5..d0655e1d2cc7 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -673,6 +673,27 @@ static void sdhci_adma_table_post(struct sdhci_host *host,
> }
> }
>
> +static void sdhci_calc_sw_timeout(struct sdhci_host *host,
> + struct mmc_command *cmd,
> + unsigned int target_timeout)
> +{
> + struct mmc_host *mmc = host->mmc;
> + struct mmc_ios *ios = &mmc->ios;
> + struct mmc_data *data = cmd->data;
> + unsigned long long transfer_time;
> +
> + if (data) {
> + transfer_time = MMC_BLOCK_TRANSFER_TIME_MS(data->blksz,
> + ios->bus_width,
> + ios->clock);
If it has a value, actual_clock is better than ios->clock.
> + /* calculate timeout for the entire data */
> + host->data_timeout = (data->blocks * (target_timeout +
> + transfer_time));
> + } else if (cmd->flags & MMC_RSP_BUSY) {
> + host->data_timeout = cmd->busy_timeout * MSEC_PER_SEC;
Doesn't need MSEC_PER_SEC multiplier.
> + }
> +}
> +
> static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
> {
> u8 count;
> @@ -732,8 +753,12 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
> }
>
> if (count >= 0xF) {
> - DBG("Too large timeout 0x%x requested for CMD%d!\n",
> - count, cmd->opcode);
> + DBG("Too large timeout.. using SW timeout for CMD%d!\n",
> + cmd->opcode);
> + sdhci_calc_sw_timeout(host, cmd, target_timeout);
> + host->ier &= ~SDHCI_INT_DATA_TIMEOUT;
> + sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
> + sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
> count = 0xE;
> }
>
> @@ -1198,6 +1223,14 @@ static void sdhci_finish_command(struct sdhci_host *host)
> {
> struct mmc_command *cmd = host->cmd;
>
> + if (host->data_timeout) {
> + unsigned long timeout;
> +
> + timeout = jiffies +
> + msecs_to_jiffies(host->data_timeout);
> + sdhci_mod_timer(host, host->cmd->mrq, timeout);
cmd could be the sbc or a stop cmd or a command during transfer, so this
needs more logic.
> + }
> +
> host->cmd = NULL;
>
> if (cmd->flags & MMC_RSP_PRESENT) {
> @@ -2341,6 +2374,10 @@ static bool sdhci_request_done(struct sdhci_host *host)
> return true;
> }
>
> + host->data_timeout = 0;
> + host->ier |= SDHCI_INT_DATA_TIMEOUT;
> + sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
> + sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
sdhci can have 2 requests in progress to allow for commands to be sent while
a data transfer is in progress, so this is not necessarily the data transfer
request that is done. Also we want to avoid unnecessary register writes.
> sdhci_del_timer(host, mrq);
>
> /*
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 54bc444c317f..e6e0278bea1a 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -332,6 +332,15 @@ struct sdhci_adma2_64_desc {
> /* Allow for a a command request and a data request at the same time */
> #define SDHCI_MAX_MRQS 2
>
> +/*
> + * Time taken for transferring one block. It is multiplied by a constant
> + * factor '2' to account for any errors
> + */
> +#define MMC_BLOCK_TRANSFER_TIME_MS(blksz, bus_width, freq) \
> + ((unsigned long long) \
> + (2 * (((blksz) * MSEC_PER_SEC * \
> + (8 / (bus_width))) / (freq))))
I don't think the macro helps make the code more readable. Might just as
well write a nice function to calculate the entire data request timeout.
> +
> enum sdhci_cookie {
> COOKIE_UNMAPPED,
> COOKIE_PRE_MAPPED, /* mapped by sdhci_pre_req() */
> @@ -546,6 +555,8 @@ struct sdhci_host {
> /* Host SDMA buffer boundary. */
> u32 sdma_boundary;
>
> + unsigned long long data_timeout;
msecs_to_jiffies() will truncate to 'unsigned int' anyway, so this might as
well be 'unsigned int'.
> +
> unsigned long private[0] ____cacheline_aligned;
> };
>
>
^ permalink raw reply
* RE: [EXT] Re: [PATCH v6 4/4] mmc: sdio support external scatter gather list
From: Xinming Hu @ 2017-12-20 12:59 UTC (permalink / raw)
To: Ulf Hansson
Cc: Linux MMC, Kalle Valo, Arend van Spriel, Franky Lin,
Hante Meuleman, Chi-Hsien Lin, Wright Feng, Zhiyuan Yang,
Tim Song, Cathy Luo, James Cao, Ganapathi Bhat,
Xu (Shanghai) Wang, Bob Tan, Amitkumar Karwar
In-Reply-To: <CAPDyKFrCjsUhmh3ccwemkGoUuXJ2RkjNZHuAf0J_ddHaAMrqyg@mail.gmail.com>
Hi Ulf,
> -----Original Message-----
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: 2017年12月19日 20:40
> To: Xinming Hu <huxm@marvell.com>
> Cc: Linux MMC <linux-mmc@vger.kernel.org>; Kalle Valo
> <kvalo@qca.qualcomm.com>; Arend van Spriel
> <arend.vanspriel@broadcom.com>; Franky Lin <franky.lin@broadcom.com>;
> Hante Meuleman <hante.meuleman@broadcom.com>; Chi-Hsien Lin
> <chi-hsien.lin@cypress.com>; Wright Feng <wright.feng@cypress.com>;
> Zhiyuan Yang <yangzy@marvell.com>; Tim Song <songtao@marvell.com>;
> Cathy Luo <cluo@marvell.com>; James Cao <jcao@marvell.com>; Ganapathi
> Bhat <gbhat@marvell.com>; Xu (Shanghai) Wang <xwang4@marvell.com>;
> Bob Tan <tanbo@marvell.com>; Amitkumar Karwar <akarwar@marvell.com>
> Subject: [EXT] Re: [PATCH v6 4/4] mmc: sdio support external scatter gather list
>
> External Email
>
> ----------------------------------------------------------------------
> On 19 December 2017 at 13:06, Xinming Hu <huxm@marvell.com> wrote:
> > Currently sdio device drivers need to prepare a big continuous
> > physical memory for large data transfer. mmc stack will construct
> > scatter/gather buffer list to make use of ADMA2.
> >
> > According to the sdio host controller specification version 4.00
> > chapter 1.13.2, sdio device drivers have the ability to construct
> > scatter/gather DMA buffer list. In this way, they do not need to
> > allocate big memory.
> >
> > This patch refactors current sdio command 53 wrapper
> > mmc_io_rw_extended, so that it can accept external scatter/gather
> > buffer list from its caller, such as the sdio device driver. This
> > patch is very useful on some embedded systems where large memory
> > allocation fails sometimes. It gives 10 to 15% better throughput
> > performance compared to a case in which small single data transfers are
> done.
> >
> > Signed-off-by: Xinming Hu <huxm@marvell.com>
> > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> > ---
> > v4: same as v3/v2/v1
> > v5: rebased on latest codebase
> > v6: same as v5
> > ---
> > drivers/mmc/core/sdio_io.c | 22 ++++++++++++++++--
> > drivers/mmc/core/sdio_ops.c | 53
> +++++++++++++++++++++++++++++++++++++++----
> > drivers/mmc/core/sdio_ops.h | 3 ++-
> > include/linux/mmc/sdio_func.h | 6 +++++
> > 4 files changed, 76 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
> > index 4806521..6046511 100644
> > --- a/drivers/mmc/core/sdio_io.c
> > +++ b/drivers/mmc/core/sdio_io.c
> > @@ -327,7 +327,7 @@ static int sdio_io_rw_ext_helper(struct sdio_func
> *func, int write,
> > size = blocks * func->cur_blksize;
> >
> > ret = mmc_io_rw_extended(func->card, write,
> > - func->num, addr, incr_addr, buf,
> > + func->num, addr, incr_addr, false,
> > + buf,
> > blocks, func->cur_blksize);
> > if (ret)
> > return ret; @@ -345,7 +345,7 @@
> static
> > int sdio_io_rw_ext_helper(struct sdio_func *func, int write,
> >
> > /* Indicate byte mode by setting "blocks" = 0 */
> > ret = mmc_io_rw_extended(func->card, write,
> func->num, addr,
> > - incr_addr, buf, 0, size);
> > + incr_addr, false, buf, 0, size);
> > if (ret)
> > return ret;
> >
> > @@ -513,6 +513,24 @@ int sdio_writesb(struct sdio_func *func, unsigned
> > int addr, void *src, } EXPORT_SYMBOL_GPL(sdio_writesb);
> >
> > +int sdio_readsb_sg_enh(struct sdio_func *func, unsigned int addr,
>
> Could you please rename this to sdio_readsb_sg() instead?
Ok, sure.
>
> > + struct sg_table *dst)
>
> I think what Christoph suggested and I agree with, is to use a "struct
> scatterlist *sg" instead of "struct sg_table *dst" as the in-parameter.
>
> The similar applies for the write API, of course.
Oh, I am not completely understand this point, could you share more information?
If using scatter_list, we need travel each node using sg_next()
Sg_table with nents could be able to use for_each_sg in the API, looks better here.
I guess, maybe scatter_list is a more generic data structure to be encouraged to use, right ?
If so, I will be fine.
>
> > +{
> > + return mmc_io_rw_extended(func->card, 0, func->num,
> > + addr, 0, true,
> > + dst, 0, func->cur_blksize); }
> > +EXPORT_SYMBOL_GPL(sdio_readsb_sg_enh);
> > +
> > +int sdio_writesb_sg_enh(struct sdio_func *func, unsigned int addr,
>
> Could you please rename this to sdio_writesb_sg() instead?
Ok.
>
> > + struct sg_table *src) {
> > + return mmc_io_rw_extended(func->card, 1, func->num,
> > + addr, 0, true,
> > + src, 0, func->cur_blksize); }
> > +EXPORT_SYMBOL_GPL(sdio_writesb_sg_enh);
> > +
> > /**
> > * sdio_readw - read a 16 bit integer from a SDIO function
> > * @func: SDIO function to access
> > diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
> > index abaaba3..2310a2a 100644
> > --- a/drivers/mmc/core/sdio_ops.c
> > +++ b/drivers/mmc/core/sdio_ops.c
> > @@ -115,16 +115,39 @@ int mmc_io_rw_direct(struct mmc_card *card, int
> write, unsigned fn,
> > return mmc_io_rw_direct_host(card->host, write, fn, addr, in,
> > out); }
> >
> > +/**
> > + * mmc_io_rw_extended - wrapper for sdio command 53 read/write
> operation
> > + * @card: destination device for read/write
> > + * @write: zero indcate read, non-zero indicate write.
> > + * @fn: SDIO function to access
> > + * @addr: address of (single byte) FIFO
> > + * @incr_addr: set to 1 indicate read or write multiple bytes
> > + of data to/from an IO register address that
> > + increment by 1 after each operation.
> > + * @sg_enhance: if set true, the caller of this function will
> > + prepare scatter gather dma buffer list.
> > + * @buf: buffer that contains the data to write. if sg_enhance
> > + is set, it point to SG dma buffer list.
> > + * @blocks: number of blocks of data transfer.
> > + if set zero, indicate byte mode
> > + if set non-zero, indicate block mode
> > + if sg_enhance is set, this parameter will not be used.
> > + * @blksz: block size for block mode data transfer.
> > + *
> > + */
>
> The descriptive function header isn't required here, because this is an internal
> function for the mmc core.
>
> However, if you really want this, I suggest you make it a separate patch.
>
Ok.
> > int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
> > - unsigned addr, int incr_addr, u8 *buf, unsigned blocks, unsigned
> blksz)
> > + unsigned int addr, int incr_addr, bool
> > + sg_enhance,
>
> Instead of letting this take a new bool variable, let's instead provide it with a
> "struct scatterlist *sg". In cases when it isn't used, just provide NULL.
>
Okay, still need choose between sg_table and scatter_list
> Depending on the end result, we may even consider to make a separate
> function dealing with the scatterlist case, perhaps via sharing some common
> functionality from mmc_io_rw_extended() instead.
>
Oh, I have thought of the new API, and finally resue the old one, since less changes compared with the common part.
I will be fine if you really want a new API.
> > + void *buf, unsigned int blocks, unsigned int
> > + blksz)
> > {
> > struct mmc_request mrq = {};
> > struct mmc_command cmd = {};
> > struct mmc_data data = {};
> > struct scatterlist sg, *sg_ptr;
> > struct sg_table sgtable;
> > + struct sg_table *sgtable_external;
> > unsigned int nents, left_size, i;
> > unsigned int seg_size = card->host->max_seg_size;
> > + unsigned int sg_blocks = 0;
> >
> > WARN_ON(blksz == 0);
> >
> > @@ -140,16 +163,35 @@ int mmc_io_rw_extended(struct mmc_card *card,
> int write, unsigned fn,
> > cmd.arg |= fn << 28;
> > cmd.arg |= incr_addr ? 0x04000000 : 0x00000000;
> > cmd.arg |= addr << 9;
> > + cmd.flags = MMC_RSP_SPI_R5 | MMC_RSP_R5 |
> MMC_CMD_ADTC;
> > +
> > + data.flags = write ? MMC_DATA_WRITE : MMC_DATA_READ;
> > + data.blksz = blksz;
> > +
> > + if (sg_enhance) {
> > + sgtable_external = buf;
> > +
> > + for_each_sg(sgtable_external->sgl, sg_ptr,
> > + sgtable_external->nents, i) {
> > + if (sg_ptr->length > card->host->max_seg_size)
> > + return -EINVAL;
> > + sg_blocks += DIV_ROUND_UP(sg_ptr->length,
> > + blksz);
>
> This looks wrong. For each iteration the number of sg_blocks will be increased,
> with the round up method.
>
> This may lead to that the data.blocks could get a in-correct value
> (bigger) when assigned below, right?
>
Per my understand, caller driver should keep each sg_ptr->length multiple of block size.
So divide should be fine for most place.
However if used wrong, sg_ptr->length is not multiple of block size, remainder memory part will be
Lost if using divide.
Please correct me if I have any misunderstanding here.
> > + }
> > +
> > + cmd.arg |= 0x08000000 | sg_blocks;
>
> This isn't going to work for byte mode transfers. Seems like that should be
> possible too when using a pre-allocated scatterlist, right!?
>
Aha, good suggestions.
Yes, I think it is possible, as there is actually a scatterlist generated for byte mode transfer in current API.
But it seems not the proper use case.
Do we really expect driver caller use sg in byte mode, it looks strange to me.
Tough in this way, the API looks more generic(with useless byte mode sg).
> > + data.blocks = sg_blocks;
> > + data.sg = sgtable_external->sgl;
> > + data.sg_len = sgtable_external->nents;
> > + goto mmc_data_req;
> > + }
> > +
> > if (blocks == 0)
> > cmd.arg |= (blksz == 512) ? 0 : blksz; /* byte mode */
> > else
> > cmd.arg |= 0x08000000 | blocks; /* block
> mode */
> > - cmd.flags = MMC_RSP_SPI_R5 | MMC_RSP_R5 |
> MMC_CMD_ADTC;
> >
> > - data.blksz = blksz;
> > /* Code in host drivers/fwk assumes that "blocks" always is >=1 */
> > data.blocks = blocks ? blocks : 1;
> > - data.flags = write ? MMC_DATA_WRITE : MMC_DATA_READ;
> >
> > left_size = data.blksz * data.blocks;
> > nents = DIV_ROUND_UP(left_size, seg_size); @@ -172,11
> +214,12
> > @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned
> fn,
> > sg_init_one(&sg, buf, left_size);
> > }
> >
> > +mmc_data_req:
> > mmc_set_data_timeout(&data, card);
> >
> > mmc_wait_for_req(card->host, &mrq);
> >
> > - if (nents > 1)
> > + if (!sg_enhance && nents > 1)
> > sg_free_table(&sgtable);
> >
> > if (cmd.error)
> > diff --git a/drivers/mmc/core/sdio_ops.h b/drivers/mmc/core/sdio_ops.h
> > index 96945ca..2d75100 100644
> > --- a/drivers/mmc/core/sdio_ops.h
> > +++ b/drivers/mmc/core/sdio_ops.h
> > @@ -23,7 +23,8 @@
> > int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn,
> > unsigned addr, u8 in, u8* out); int mmc_io_rw_extended(struct
> > mmc_card *card, int write, unsigned fn,
> > - unsigned addr, int incr_addr, u8 *buf, unsigned blocks, unsigned
> blksz);
> > + unsigned int addr, int incr_addr, bool
> sg_enhance,
> > + void *buf, unsigned int blocks, unsigned int
> > + blksz);
> > int sdio_reset(struct mmc_host *host); unsigned int
> > mmc_align_data_size(struct mmc_card *card, unsigned int sz); void
> > sdio_irq_work(struct work_struct *work); diff --git
> > a/include/linux/mmc/sdio_func.h b/include/linux/mmc/sdio_func.h index
> > 72d4de4..d36ba47 100644
> > --- a/include/linux/mmc/sdio_func.h
> > +++ b/include/linux/mmc/sdio_func.h
> > @@ -14,6 +14,7 @@
> >
> > #include <linux/device.h>
> > #include <linux/mod_devicetable.h>
> > +#include <linux/scatterlist.h>
> >
> > #include <linux/mmc/pm.h>
> >
> > @@ -136,6 +137,11 @@ extern int sdio_memcpy_fromio(struct sdio_func
> > *func, void *dst, extern int sdio_readsb(struct sdio_func *func, void *dst,
> > unsigned int addr, int count);
> >
> > +int sdio_readsb_sg_enh(struct sdio_func *func, unsigned int addr,
> > + struct sg_table *dst); int
> > +sdio_writesb_sg_enh(struct sdio_func *func, unsigned int addr,
> > + struct sg_table *src);
> > +
> > extern void sdio_writeb(struct sdio_func *func, u8 b,
> > unsigned int addr, int *err_ret); extern void
> > sdio_writew(struct sdio_func *func, u16 b,
> > --
> > 1.9.1
> >
>
> Kind regards
> Uffe
^ permalink raw reply
* [PATCH 2/2] mmc: sunxi: Add runtime_pm support
From: Maxime Ripard @ 2017-12-20 10:50 UTC (permalink / raw)
To: Chen-Yu Tsai, Maxime Ripard, Ulf Hansson; +Cc: linux-arm-kernel, linux-mmc
In-Reply-To: <cover.ac66a754953bedbb7529436b40a7996f4ba3cb95.1513766964.git-series.maxime.ripard@free-electrons.com>
So far, even if our card was not in use, we didn't shut down our main
clock, which meant that it was still output on the MMC bus.
While this obviously means that we could save some power there, it also
created issues when it comes with EMC control since we'll have a perfect
peak at the card clock rate.
Let's implement runtime_pm with autosuspend so that we will shut down the
clock when it's not been in use for quite some time.
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
drivers/mmc/host/sunxi-mmc.c | 90 ++++++++++++++++++++++++-------------
1 file changed, 60 insertions(+), 30 deletions(-)
diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 3ce46ebd3488..c6a0bd0e0476 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -35,6 +35,7 @@
#include <linux/of_gpio.h>
#include <linux/of_platform.h>
#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
#include <linux/regulator/consumer.h>
#include <linux/reset.h>
#include <linux/scatterlist.h>
@@ -1217,29 +1218,11 @@ static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
return ret;
}
- ret = clk_prepare_enable(host->clk_mmc);
- if (ret) {
- dev_err(&pdev->dev, "Enable mmc clk err %d\n", ret);
- goto error_disable_clk_ahb;
- }
-
- ret = clk_prepare_enable(host->clk_output);
- if (ret) {
- dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
- goto error_disable_clk_mmc;
- }
-
- ret = clk_prepare_enable(host->clk_sample);
- if (ret) {
- dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
- goto error_disable_clk_output;
- }
-
if (!IS_ERR(host->reset)) {
ret = reset_control_reset(host->reset);
if (ret) {
dev_err(&pdev->dev, "reset err %d\n", ret);
- goto error_disable_clk_sample;
+ goto error_disable_clk_ahb;
}
}
@@ -1258,12 +1241,6 @@ static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
error_assert_reset:
if (!IS_ERR(host->reset))
reset_control_assert(host->reset);
-error_disable_clk_sample:
- clk_disable_unprepare(host->clk_sample);
-error_disable_clk_output:
- clk_disable_unprepare(host->clk_output);
-error_disable_clk_mmc:
- clk_disable_unprepare(host->clk_mmc);
error_disable_clk_ahb:
clk_disable_unprepare(host->clk_ahb);
return ret;
@@ -1280,6 +1257,7 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
dev_err(&pdev->dev, "mmc alloc host failed\n");
return -ENOMEM;
}
+ platform_set_drvdata(pdev, mmc);
host = mmc_priv(mmc);
host->mmc = mmc;
@@ -1340,12 +1318,16 @@ static int sunxi_mmc_probe(struct platform_device *pdev)
if (ret)
goto error_free_dma;
+ pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
+ pm_runtime_use_autosuspend(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
+
ret = mmc_add_host(mmc);
if (ret)
goto error_free_dma;
dev_info(&pdev->dev, "base:0x%p irq:%u\n", host->reg_base, host->irq);
- platform_set_drvdata(pdev, mmc);
+
return 0;
error_free_dma:
@@ -1361,27 +1343,75 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
struct sunxi_mmc_host *host = mmc_priv(mmc);
mmc_remove_host(mmc);
+ pm_runtime_force_suspend(&pdev->dev);
disable_irq(host->irq);
sunxi_mmc_reset_host(host);
if (!IS_ERR(host->reset))
reset_control_assert(host->reset);
- clk_disable_unprepare(host->clk_sample);
+ dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma);
+ mmc_free_host(mmc);
+
+ return 0;
+}
+
+static int sunxi_mmc_runtime_resume(struct device *dev)
+{
+ struct mmc_host *mmc = dev_get_drvdata(dev);
+ struct sunxi_mmc_host *host = mmc_priv(mmc);
+ int ret;
+
+ ret = clk_prepare_enable(host->clk_mmc);
+ if (ret) {
+ dev_err(dev, "Enable mmc clk err %d\n", ret);
+ return ret;
+ }
+
+ ret = clk_prepare_enable(host->clk_output);
+ if (ret) {
+ dev_err(dev, "Enable output clk err %d\n", ret);
+ goto error_disable_clk_mmc;
+ }
+
+ ret = clk_prepare_enable(host->clk_sample);
+ if (ret) {
+ dev_err(dev, "Enable sample clk err %d\n", ret);
+ goto error_disable_clk_output;
+ }
+
+ return 0;
+
+error_disable_clk_output:
clk_disable_unprepare(host->clk_output);
+error_disable_clk_mmc:
clk_disable_unprepare(host->clk_mmc);
- clk_disable_unprepare(host->clk_ahb);
+ return ret;
+}
- dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma);
- mmc_free_host(mmc);
+static int sunxi_mmc_runtime_suspend(struct device *dev)
+{
+ struct mmc_host *mmc = dev_get_drvdata(dev);
+ struct sunxi_mmc_host *host = mmc_priv(mmc);
+
+ clk_disable_unprepare(host->clk_sample);
+ clk_disable_unprepare(host->clk_output);
+ clk_disable_unprepare(host->clk_mmc);
return 0;
}
+static const struct dev_pm_ops sunxi_mmc_pm_ops = {
+ SET_RUNTIME_PM_OPS(sunxi_mmc_runtime_suspend,
+ sunxi_mmc_runtime_resume,
+ NULL)
+};
+
static struct platform_driver sunxi_mmc_driver = {
.driver = {
.name = "sunxi-mmc",
.of_match_table = of_match_ptr(sunxi_mmc_of_match),
+ .pm = &sunxi_mmc_pm_ops,
},
.probe = sunxi_mmc_probe,
.remove = sunxi_mmc_remove,
--
git-series 0.9.1
^ permalink raw reply related
* [PATCH 1/2] mmc: sunxi: Reorder the headers
From: Maxime Ripard @ 2017-12-20 10:50 UTC (permalink / raw)
To: Chen-Yu Tsai, Maxime Ripard, Ulf Hansson; +Cc: linux-arm-kernel, linux-mmc
In-Reply-To: <cover.ac66a754953bedbb7529436b40a7996f4ba3cb95.1513766964.git-series.maxime.ripard@free-electrons.com>
Our headers sort algorithm has had pretty chaotic results. Let's fix that.
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
drivers/mmc/host/sunxi-mmc.c | 43 +++++++++++++++++--------------------
1 file changed, 20 insertions(+), 23 deletions(-)
diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index cc98355dbdb9..3ce46ebd3488 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -13,36 +13,33 @@
* the License, or (at your option) any later version.
*/
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/io.h>
-#include <linux/device.h>
-#include <linux/interrupt.h>
-#include <linux/delay.h>
-#include <linux/err.h>
-
#include <linux/clk.h>
#include <linux/clk/sunxi-ng.h>
-#include <linux/gpio.h>
-#include <linux/platform_device.h>
-#include <linux/spinlock.h>
-#include <linux/scatterlist.h>
+#include <linux/delay.h>
+#include <linux/device.h>
#include <linux/dma-mapping.h>
-#include <linux/slab.h>
-#include <linux/reset.h>
-#include <linux/regulator/consumer.h>
-
-#include <linux/of_address.h>
-#include <linux/of_gpio.h>
-#include <linux/of_platform.h>
-
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mmc/card.h>
+#include <linux/mmc/core.h>
+#include <linux/mmc/mmc.h>
#include <linux/mmc/host.h>
#include <linux/mmc/sd.h>
#include <linux/mmc/sdio.h>
-#include <linux/mmc/mmc.h>
-#include <linux/mmc/core.h>
-#include <linux/mmc/card.h>
#include <linux/mmc/slot-gpio.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_gpio.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/reset.h>
+#include <linux/scatterlist.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
/* register offset definitions */
#define SDXC_REG_GCTRL (0x00) /* SMC Global Control Register */
--
git-series 0.9.1
^ permalink raw reply related
* [PATCH 0/2] mmc: sunxi: Add runtime PM support
From: Maxime Ripard @ 2017-12-20 10:50 UTC (permalink / raw)
To: Chen-Yu Tsai, Maxime Ripard, Ulf Hansson; +Cc: linux-arm-kernel, linux-mmc
Hi,
Since it's introduction, our MMC controller has had its external clocks
running all the time.
While that was working great, the power usage and most importantly the EMI
that it generated was pretty bad.
Let's implement some runtime_pm hooks with an autosuspend to cut the
external clock when the MMC is not active.
I didn't get all the way to also shutdown the bus clocks since on some
older SoCs (before the A31), it also means that the controler will be reset
which has some quite important implications obviously. And I couldn't make
it work reliably at the moment. Anyway, it can always be implemented as a
second step if needed.
(Quite simple) benchmarks have shown no noticeable regressions since the
controller state is maintained.
Let me know what you think,
Maxime
Maxime Ripard (2):
mmc: sunxi: Reorder the headers
mmc: sunxi: Add runtime_pm support
drivers/mmc/host/sunxi-mmc.c | 133 +++++++++++++++++++++---------------
1 file changed, 80 insertions(+), 53 deletions(-)
base-commit: 4fbd8d194f06c8a3fd2af1ce560ddb31f7ec8323
--
git-series 0.9.1
^ permalink raw reply
* [PATCH 9/9] mmc: sdhci-pci: Respect PM flags when enabling card detect GPIO IRQ wakeup
From: Adrian Hunter @ 2017-12-20 8:18 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Haridhar Kalvala
In-Reply-To: <1513757933-11232-1-git-send-email-adrian.hunter@intel.com>
Commit 03dbaa04a2e5 ("mmc: slot-gpio: Add support to enable irq wake on
cd_irq") enabled wakeup irrespective of the host controller's PM flags.
However, users also want to control it from sysfs power/wakeup attribute.
That means the driver needs to check the PM flags before enabling it in the
suspend callback. Add support for that in sdhci-pci, which is the only
driver presently using the MMC_CAP_CD_WAKE flag, and remove the enabling in
mmc_gpiod_request_cd_irq().
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
drivers/mmc/core/slot-gpio.c | 2 --
drivers/mmc/host/sdhci-pci-core.c | 18 ++++++++++++++----
drivers/mmc/host/sdhci.c | 4 ++++
3 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
index 3a30535c2f31..74dd534905f7 100644
--- a/drivers/mmc/core/slot-gpio.c
+++ b/drivers/mmc/core/slot-gpio.c
@@ -149,8 +149,6 @@ void mmc_gpiod_request_cd_irq(struct mmc_host *host)
if (irq < 0)
host->caps |= MMC_CAP_NEEDS_POLL;
- else if ((host->caps & MMC_CAP_CD_WAKE) && !enable_irq_wake(irq))
- host->slot.cd_wake_enabled = true;
}
EXPORT_SYMBOL(mmc_gpiod_request_cd_irq);
diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
index 96ec4aa7cb27..eeb4bd5797a0 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -42,18 +42,25 @@
static int sdhci_pci_init_wakeup(struct sdhci_pci_chip *chip)
{
mmc_pm_flag_t pm_flags = 0;
+ bool cap_cd_wake = false;
int i;
for (i = 0; i < chip->num_slots; i++) {
struct sdhci_pci_slot *slot = chip->slots[i];
- if (slot)
+ if (slot) {
pm_flags |= slot->host->mmc->pm_flags;
+ if (slot->host->mmc->caps & MMC_CAP_CD_WAKE)
+ cap_cd_wake = true;
+ }
}
- return device_set_wakeup_enable(&chip->pdev->dev,
- (pm_flags & MMC_PM_KEEP_POWER) &&
- (pm_flags & MMC_PM_WAKE_SDIO_IRQ));
+ if ((pm_flags & MMC_PM_KEEP_POWER) && (pm_flags & MMC_PM_WAKE_SDIO_IRQ))
+ return device_wakeup_enable(&chip->pdev->dev);
+ else if (!cap_cd_wake)
+ return device_wakeup_disable(&chip->pdev->dev);
+
+ return 0;
}
static int sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
@@ -1687,6 +1694,9 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot(
if (device_can_wakeup(&pdev->dev))
host->mmc->pm_caps |= MMC_PM_WAKE_SDIO_IRQ;
+ if (host->mmc->caps & MMC_CAP_CD_WAKE)
+ device_init_wakeup(&pdev->dev, true);
+
if (slot->cd_idx >= 0) {
ret = mmc_gpiod_request_cd(host->mmc, NULL, slot->cd_idx,
slot->cd_override_level, 0, NULL);
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c82fd16dd14e..883b55077231 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2891,6 +2891,8 @@ int sdhci_suspend_host(struct sdhci_host *host)
free_irq(host->irq, host);
}
+ mmc_gpio_set_cd_wake(host->mmc, true);
+
return 0;
}
@@ -2918,6 +2920,8 @@ int sdhci_resume_host(struct sdhci_host *host)
mmiowb();
}
+ mmc_gpio_set_cd_wake(host->mmc, false);
+
if (host->irq_wake_enabled) {
sdhci_disable_irq_wakeups(host);
} else {
--
1.9.1
^ permalink raw reply related
* [PATCH 8/9] mmc: slot-gpio: Add a function to enable/disable card detect IRQ wakeup
From: Adrian Hunter @ 2017-12-20 8:18 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Haridhar Kalvala
In-Reply-To: <1513757933-11232-1-git-send-email-adrian.hunter@intel.com>
Commit 03dbaa04a2e5 ("mmc: slot-gpio: Add support to enable irq wake on
cd_irq") enabled wakeup irrespective of the host controller's PM flags.
However, users also want to control it from sysfs power/wakeup attribute.
That means the driver needs to check the PM flags before enabling it in the
suspend callback. Add helper function mmc_gpio_set_cd_wake() to make it
easy for drivers to do that.
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
drivers/mmc/core/core.c | 3 +--
drivers/mmc/core/slot-gpio.c | 23 +++++++++++++++++++++++
include/linux/mmc/slot-gpio.h | 1 +
3 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index c0ba6d8823b7..7bed23c877de 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2655,8 +2655,7 @@ void mmc_start_host(struct mmc_host *host)
void mmc_stop_host(struct mmc_host *host)
{
if (host->slot.cd_irq >= 0) {
- if (host->slot.cd_wake_enabled)
- disable_irq_wake(host->slot.cd_irq);
+ mmc_gpio_set_cd_wake(host, false);
disable_irq(host->slot.cd_irq);
}
diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
index f7c6e0542de7..3a30535c2f31 100644
--- a/drivers/mmc/core/slot-gpio.c
+++ b/drivers/mmc/core/slot-gpio.c
@@ -154,6 +154,29 @@ void mmc_gpiod_request_cd_irq(struct mmc_host *host)
}
EXPORT_SYMBOL(mmc_gpiod_request_cd_irq);
+int mmc_gpio_set_cd_wake(struct mmc_host *host, bool on)
+{
+ int ret = 0;
+
+ if (!(host->caps & MMC_CAP_CD_WAKE) ||
+ host->slot.cd_irq < 0 ||
+ on == host->slot.cd_wake_enabled)
+ return 0;
+
+ if (on) {
+ if (device_may_wakeup(mmc_dev(host))) {
+ ret = enable_irq_wake(host->slot.cd_irq);
+ host->slot.cd_wake_enabled = !ret;
+ }
+ } else {
+ disable_irq_wake(host->slot.cd_irq);
+ host->slot.cd_wake_enabled = false;
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL(mmc_gpio_set_cd_wake);
+
/* Register an alternate interrupt service routine for
* the card-detect GPIO.
*/
diff --git a/include/linux/mmc/slot-gpio.h b/include/linux/mmc/slot-gpio.h
index 82f0d289f110..1ee9f15bf2c0 100644
--- a/include/linux/mmc/slot-gpio.h
+++ b/include/linux/mmc/slot-gpio.h
@@ -33,5 +33,6 @@ void mmc_gpio_set_cd_isr(struct mmc_host *host,
irqreturn_t (*isr)(int irq, void *dev_id));
void mmc_gpiod_request_cd_irq(struct mmc_host *host);
bool mmc_can_gpio_cd(struct mmc_host *host);
+int mmc_gpio_set_cd_wake(struct mmc_host *host, bool on);
#endif
--
1.9.1
^ permalink raw reply related
* [PATCH 7/9] mmc: sdhci: Do not unnecessarily enable wakeup for SDIO card interrupt
From: Adrian Hunter @ 2017-12-20 8:18 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Haridhar Kalvala
In-Reply-To: <1513757933-11232-1-git-send-email-adrian.hunter@intel.com>
Do not enable wakeup for SDIO card interrupt unless the SDIO function
driver has requested it which is indicated by mmc_card_wake_sdio_irq().
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
drivers/mmc/host/sdhci.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index f16df253cc59..c82fd16dd14e 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2842,8 +2842,13 @@ static bool sdhci_enable_irq_wakeups(struct sdhci_host *host)
irq_val |= SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE;
}
- wake_val |= SDHCI_WAKE_ON_INT;
- irq_val |= SDHCI_INT_CARD_INT;
+ if (mmc_card_wake_sdio_irq(host->mmc)) {
+ wake_val |= SDHCI_WAKE_ON_INT;
+ irq_val |= SDHCI_INT_CARD_INT;
+ }
+
+ if (!irq_val)
+ return false;
val = sdhci_readb(host, SDHCI_WAKE_UP_CONTROL);
val &= ~mask;
--
1.9.1
^ permalink raw reply related
* [PATCH 6/9] mmc: sdhci: Do not unnecessarily enable wakeup for card detect interrupt
From: Adrian Hunter @ 2017-12-20 8:18 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Haridhar Kalvala
In-Reply-To: <1513757933-11232-1-git-send-email-adrian.hunter@intel.com>
Do not unnecessarily enable card detect wakeup in the cases that the card
is not removable or a GPIO is used for card detect.
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
drivers/mmc/host/sdhci.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index dd922923019a..f16df253cc59 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2813,6 +2813,14 @@ static irqreturn_t sdhci_thread_irq(int irq, void *dev_id)
\*****************************************************************************/
#ifdef CONFIG_PM
+
+static bool sdhci_cd_irq_can_wakeup(struct sdhci_host *host)
+{
+ return mmc_card_is_removable(host->mmc) &&
+ !(host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
+ host->mmc->slot.cd_irq < 0;
+}
+
/*
* To enable wakeup events, the corresponding events have to be enabled in
* the Interrupt Status Enable register too. See 'Table 1-6: Wakeup Signal
@@ -2829,7 +2837,7 @@ static bool sdhci_enable_irq_wakeups(struct sdhci_host *host)
u8 wake_val = 0;
u8 val;
- if (!(host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)) {
+ if (sdhci_cd_irq_can_wakeup(host)) {
wake_val |= SDHCI_WAKE_ON_INSERT | SDHCI_WAKE_ON_REMOVE;
irq_val |= SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE;
}
--
1.9.1
^ permalink raw reply related
* [PATCH 5/9] mmc: sdhci: Rework sdhci_enable_irq_wakeups()
From: Adrian Hunter @ 2017-12-20 8:18 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Haridhar Kalvala
In-Reply-To: <1513757933-11232-1-git-send-email-adrian.hunter@intel.com>
In preparation for adding more conditions for whether IRQ wakeup is
enabled, rework sdhci_enable_irq_wakeups() so that needed bits are added
instead of adding them all and then removing the unneeded bits.
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
drivers/mmc/host/sdhci.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index a8129d091207..dd922923019a 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2823,20 +2823,25 @@ static irqreturn_t sdhci_thread_irq(int irq, void *dev_id)
*/
static bool sdhci_enable_irq_wakeups(struct sdhci_host *host)
{
+ u8 mask = SDHCI_WAKE_ON_INSERT | SDHCI_WAKE_ON_REMOVE |
+ SDHCI_WAKE_ON_INT;
+ u32 irq_val = 0;
+ u8 wake_val = 0;
u8 val;
- u8 mask = SDHCI_WAKE_ON_INSERT | SDHCI_WAKE_ON_REMOVE
- | SDHCI_WAKE_ON_INT;
- u32 irq_val = SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE |
- SDHCI_INT_CARD_INT;
- val = sdhci_readb(host, SDHCI_WAKE_UP_CONTROL);
- val |= mask ;
- /* Avoid fake wake up */
- if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) {
- val &= ~(SDHCI_WAKE_ON_INSERT | SDHCI_WAKE_ON_REMOVE);
- irq_val &= ~(SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE);
+ if (!(host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)) {
+ wake_val |= SDHCI_WAKE_ON_INSERT | SDHCI_WAKE_ON_REMOVE;
+ irq_val |= SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE;
}
+
+ wake_val |= SDHCI_WAKE_ON_INT;
+ irq_val |= SDHCI_INT_CARD_INT;
+
+ val = sdhci_readb(host, SDHCI_WAKE_UP_CONTROL);
+ val &= ~mask;
+ val |= wake_val;
sdhci_writeb(host, val, SDHCI_WAKE_UP_CONTROL);
+
sdhci_writel(host, irq_val, SDHCI_INT_ENABLE);
host->irq_wake_enabled = !enable_irq_wake(host->irq);
--
1.9.1
^ permalink raw reply related
* [PATCH 4/9] mmc: sdhci: Handle failure of enable_irq_wake()
From: Adrian Hunter @ 2017-12-20 8:18 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Haridhar Kalvala
In-Reply-To: <1513757933-11232-1-git-send-email-adrian.hunter@intel.com>
Now that sdhci_enable_irq_wakeups() is a local function, change it to
return whether the IRQ wakeup was successfully enabled. This is in
preparation for adding more conditions for whether IRQ wakeup is enabled.
Note it is assumed, for SDHCI devices, that suspend is more important than
wakeup, so we continue to suspend regardless.
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
drivers/mmc/host/sdhci.c | 24 +++++++++++++++---------
drivers/mmc/host/sdhci.h | 1 +
2 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 8b2ccf7795ec..a8129d091207 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2821,7 +2821,7 @@ static irqreturn_t sdhci_thread_irq(int irq, void *dev_id)
* sdhci_disable_irq_wakeups() since it will be set by
* sdhci_enable_card_detection() or sdhci_init().
*/
-static void sdhci_enable_irq_wakeups(struct sdhci_host *host)
+static bool sdhci_enable_irq_wakeups(struct sdhci_host *host)
{
u8 val;
u8 mask = SDHCI_WAKE_ON_INSERT | SDHCI_WAKE_ON_REMOVE
@@ -2838,6 +2838,10 @@ static void sdhci_enable_irq_wakeups(struct sdhci_host *host)
}
sdhci_writeb(host, val, SDHCI_WAKE_UP_CONTROL);
sdhci_writel(host, irq_val, SDHCI_INT_ENABLE);
+
+ host->irq_wake_enabled = !enable_irq_wake(host->irq);
+
+ return host->irq_wake_enabled;
}
static void sdhci_disable_irq_wakeups(struct sdhci_host *host)
@@ -2849,6 +2853,10 @@ static void sdhci_disable_irq_wakeups(struct sdhci_host *host)
val = sdhci_readb(host, SDHCI_WAKE_UP_CONTROL);
val &= ~mask;
sdhci_writeb(host, val, SDHCI_WAKE_UP_CONTROL);
+
+ disable_irq_wake(host->irq);
+
+ host->irq_wake_enabled = false;
}
int sdhci_suspend_host(struct sdhci_host *host)
@@ -2857,15 +2865,14 @@ int sdhci_suspend_host(struct sdhci_host *host)
mmc_retune_timer_stop(host->mmc);
- if (!device_may_wakeup(mmc_dev(host->mmc))) {
+ if (!device_may_wakeup(mmc_dev(host->mmc)) ||
+ !sdhci_enable_irq_wakeups(host)) {
host->ier = 0;
sdhci_writel(host, 0, SDHCI_INT_ENABLE);
sdhci_writel(host, 0, SDHCI_SIGNAL_ENABLE);
free_irq(host->irq, host);
- } else {
- sdhci_enable_irq_wakeups(host);
- enable_irq_wake(host->irq);
}
+
return 0;
}
@@ -2893,15 +2900,14 @@ int sdhci_resume_host(struct sdhci_host *host)
mmiowb();
}
- if (!device_may_wakeup(mmc_dev(host->mmc))) {
+ if (host->irq_wake_enabled) {
+ sdhci_disable_irq_wakeups(host);
+ } else {
ret = request_threaded_irq(host->irq, sdhci_irq,
sdhci_thread_irq, IRQF_SHARED,
mmc_hostname(host->mmc), host);
if (ret)
return ret;
- } else {
- sdhci_disable_irq_wakeups(host);
- disable_irq_wake(host->irq);
}
sdhci_enable_card_detection(host);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 7393b3a54772..afab26fd70e6 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -484,6 +484,7 @@ struct sdhci_host {
bool bus_on; /* Bus power prevents runtime suspend */
bool preset_enabled; /* Preset is enabled */
bool pending_reset; /* Cmd/data reset is pending */
+ bool irq_wake_enabled; /* IRQ wakeup is enabled */
struct mmc_request *mrqs_done[SDHCI_MAX_MRQS]; /* Requests done */
struct mmc_command *cmd; /* Current command */
--
1.9.1
^ permalink raw reply related
* [PATCH 3/9] mmc: sdhci: Stop exporting sdhci_enable_irq_wakeups()
From: Adrian Hunter @ 2017-12-20 8:18 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Haridhar Kalvala
In-Reply-To: <1513757933-11232-1-git-send-email-adrian.hunter@intel.com>
Now that it is not being used by any drivers, stop exporting it.
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
drivers/mmc/host/sdhci.c | 3 +--
drivers/mmc/host/sdhci.h | 1 -
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index e9290a3439d5..8b2ccf7795ec 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2821,7 +2821,7 @@ static irqreturn_t sdhci_thread_irq(int irq, void *dev_id)
* sdhci_disable_irq_wakeups() since it will be set by
* sdhci_enable_card_detection() or sdhci_init().
*/
-void sdhci_enable_irq_wakeups(struct sdhci_host *host)
+static void sdhci_enable_irq_wakeups(struct sdhci_host *host)
{
u8 val;
u8 mask = SDHCI_WAKE_ON_INSERT | SDHCI_WAKE_ON_REMOVE
@@ -2839,7 +2839,6 @@ void sdhci_enable_irq_wakeups(struct sdhci_host *host)
sdhci_writeb(host, val, SDHCI_WAKE_UP_CONTROL);
sdhci_writel(host, irq_val, SDHCI_INT_ENABLE);
}
-EXPORT_SYMBOL_GPL(sdhci_enable_irq_wakeups);
static void sdhci_disable_irq_wakeups(struct sdhci_host *host)
{
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 54bc444c317f..7393b3a54772 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -718,7 +718,6 @@ int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
#ifdef CONFIG_PM
int sdhci_suspend_host(struct sdhci_host *host);
int sdhci_resume_host(struct sdhci_host *host);
-void sdhci_enable_irq_wakeups(struct sdhci_host *host);
int sdhci_runtime_suspend_host(struct sdhci_host *host);
int sdhci_runtime_resume_host(struct sdhci_host *host);
#endif
--
1.9.1
^ permalink raw reply related
* [PATCH 2/9] mmc: sdhci-pci: Use device wakeup capability to determine MMC_PM_WAKE_SDIO_IRQ capability
From: Adrian Hunter @ 2017-12-20 8:18 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Haridhar Kalvala
In-Reply-To: <1513757933-11232-1-git-send-email-adrian.hunter@intel.com>
PCI and ACPI determine if a device is wakeup capable, so use that to
determine the MMC_PM_WAKE_SDIO_IRQ capability correctly.
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
drivers/mmc/host/sdhci-pci-core.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
index 2ffc09f088ee..96ec4aa7cb27 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -51,9 +51,9 @@ static int sdhci_pci_init_wakeup(struct sdhci_pci_chip *chip)
pm_flags |= slot->host->mmc->pm_flags;
}
- return device_init_wakeup(&chip->pdev->dev,
- (pm_flags & MMC_PM_KEEP_POWER) &&
- (pm_flags & MMC_PM_WAKE_SDIO_IRQ));
+ return device_set_wakeup_enable(&chip->pdev->dev,
+ (pm_flags & MMC_PM_KEEP_POWER) &&
+ (pm_flags & MMC_PM_WAKE_SDIO_IRQ));
}
static int sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
@@ -1680,10 +1680,13 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot(
}
}
- host->mmc->pm_caps = MMC_PM_KEEP_POWER | MMC_PM_WAKE_SDIO_IRQ;
+ host->mmc->pm_caps = MMC_PM_KEEP_POWER;
host->mmc->slotno = slotno;
host->mmc->caps2 |= MMC_CAP2_NO_PRESCAN_POWERUP;
+ if (device_can_wakeup(&pdev->dev))
+ host->mmc->pm_caps |= MMC_PM_WAKE_SDIO_IRQ;
+
if (slot->cd_idx >= 0) {
ret = mmc_gpiod_request_cd(host->mmc, NULL, slot->cd_idx,
slot->cd_override_level, 0, NULL);
--
1.9.1
^ permalink raw reply related
* [PATCH 1/9] mmc: sdhci-pci: Stop calling sdhci_enable_irq_wakeups()
From: Adrian Hunter @ 2017-12-20 8:18 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Haridhar Kalvala
In-Reply-To: <1513757933-11232-1-git-send-email-adrian.hunter@intel.com>
sdhci_enable_irq_wakeups() is already called by sdhci_suspend_host() so
sdhci-pci should not need to call it. However sdhci_suspend_host() only
calls it if wakeups are enabled, and sdhci-pci does not enable them until
after calling sdhci_suspend_host(). So move the calls to
sdhci_pci_init_wakeup() before calling sdhci_suspend_host(), and
stop calling sdhci_enable_irq_wakeups(). That results in some
simplification because sdhci_pci_suspend_host() and
__sdhci_pci_suspend_host() no longer need to be separate functions.
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
drivers/mmc/host/sdhci-pci-core.c | 58 ++++++++++++++-------------------------
1 file changed, 21 insertions(+), 37 deletions(-)
diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
index 110c634cfb43..2ffc09f088ee 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -39,10 +39,29 @@
static void sdhci_pci_hw_reset(struct sdhci_host *host);
#ifdef CONFIG_PM_SLEEP
-static int __sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
+static int sdhci_pci_init_wakeup(struct sdhci_pci_chip *chip)
+{
+ mmc_pm_flag_t pm_flags = 0;
+ int i;
+
+ for (i = 0; i < chip->num_slots; i++) {
+ struct sdhci_pci_slot *slot = chip->slots[i];
+
+ if (slot)
+ pm_flags |= slot->host->mmc->pm_flags;
+ }
+
+ return device_init_wakeup(&chip->pdev->dev,
+ (pm_flags & MMC_PM_KEEP_POWER) &&
+ (pm_flags & MMC_PM_WAKE_SDIO_IRQ));
+}
+
+static int sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
{
int i, ret;
+ sdhci_pci_init_wakeup(chip);
+
for (i = 0; i < chip->num_slots; i++) {
struct sdhci_pci_slot *slot = chip->slots[i];
struct sdhci_host *host;
@@ -58,9 +77,6 @@ static int __sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
ret = sdhci_suspend_host(host);
if (ret)
goto err_pci_suspend;
-
- if (host->mmc->pm_flags & MMC_PM_WAKE_SDIO_IRQ)
- sdhci_enable_irq_wakeups(host);
}
return 0;
@@ -71,36 +87,6 @@ static int __sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
return ret;
}
-static int sdhci_pci_init_wakeup(struct sdhci_pci_chip *chip)
-{
- mmc_pm_flag_t pm_flags = 0;
- int i;
-
- for (i = 0; i < chip->num_slots; i++) {
- struct sdhci_pci_slot *slot = chip->slots[i];
-
- if (slot)
- pm_flags |= slot->host->mmc->pm_flags;
- }
-
- return device_init_wakeup(&chip->pdev->dev,
- (pm_flags & MMC_PM_KEEP_POWER) &&
- (pm_flags & MMC_PM_WAKE_SDIO_IRQ));
-}
-
-static int sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
-{
- int ret;
-
- ret = __sdhci_pci_suspend_host(chip);
- if (ret)
- return ret;
-
- sdhci_pci_init_wakeup(chip);
-
- return 0;
-}
-
int sdhci_pci_resume_host(struct sdhci_pci_chip *chip)
{
struct sdhci_pci_slot *slot;
@@ -1108,7 +1094,7 @@ static int jmicron_suspend(struct sdhci_pci_chip *chip)
{
int i, ret;
- ret = __sdhci_pci_suspend_host(chip);
+ ret = sdhci_pci_suspend_host(chip);
if (ret)
return ret;
@@ -1118,8 +1104,6 @@ static int jmicron_suspend(struct sdhci_pci_chip *chip)
jmicron_enable_mmc(chip->slots[i]->host, 0);
}
- sdhci_pci_init_wakeup(chip);
-
return 0;
}
--
1.9.1
^ permalink raw reply related
* [PATCH 0/9] mmc: sdhci-pci: Respect PM flags when enabling card detect GPIO IRQ wakeup
From: Adrian Hunter @ 2017-12-20 8:18 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Haridhar Kalvala
Hi
Commit 03dbaa04a2e5 ("mmc: slot-gpio: Add support to enable irq wake on
cd_irq") enabled wakeup irrespective of the host controller's PM flags.
However, users also want to control it from sysfs power/wakeup attribute.
That means the driver needs to check the PM flags before enabling it in the
suspend callback. Patch 9 adds support for that in sdhci-pci, which is the
only driver presently using the MMC_CAP_CD_WAKE flag. Patches 1 - 7 tidy
up aspects of sdhci and sdhci-pci wakeup handling, and patch 8 adds a
helper function to make it easy for drivers.
Adrian Hunter (9):
mmc: sdhci-pci: Stop calling sdhci_enable_irq_wakeups()
mmc: sdhci-pci: Use device wakeup capability to determine MMC_PM_WAKE_SDIO_IRQ capability
mmc: sdhci: Stop exporting sdhci_enable_irq_wakeups()
mmc: sdhci: Handle failure of enable_irq_wake()
mmc: sdhci: Rework sdhci_enable_irq_wakeups()
mmc: sdhci: Do not unnecessarily enable wakeup for card detect interrupt
mmc: sdhci: Do not unnecessarily enable wakeup for SDIO card interrupt
mmc: slot-gpio: Add a function to enable/disable card detect IRQ wakeup
mmc: sdhci-pci: Respect PM flags when enabling card detect GPIO IRQ wakeup
drivers/mmc/core/core.c | 3 +-
drivers/mmc/core/slot-gpio.c | 25 ++++++++++++--
drivers/mmc/host/sdhci-pci-core.c | 73 +++++++++++++++++++--------------------
drivers/mmc/host/sdhci.c | 67 ++++++++++++++++++++++++-----------
drivers/mmc/host/sdhci.h | 2 +-
include/linux/mmc/slot-gpio.h | 1 +
6 files changed, 108 insertions(+), 63 deletions(-)
Regards
Adrian
^ permalink raw reply
* Re: [PATCH] mmc: core: Send SLEEP_NOTIFICATION for eMMC 5.x device
From: Gwendal Grignou @ 2017-12-20 1:57 UTC (permalink / raw)
To: Ulf Hansson
Cc: Gwendal Grignou, linux-mmc@vger.kernel.org, Avi Shchislowski,
Chris Ball, Alex Lemberg, Thierry Escande
In-Reply-To: <CAPDyKFqH3w4j-K0QJj5o9ag42BTDxkpjT5DbqErcZxNK8-Da+w@mail.gmail.com>
Ufe,
I have been side-tracked but will get back on it. I will be the worst
case scenario (lot of random write before suspend for different eMMC).
I also need to rework the wait for DAT0 to deassert instead of the
busy line as usual.
Gwendal.
On Fri, Dec 15, 2017 at 1:24 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 29 November 2017 at 15:14, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> [...]
>>
>>>> static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type)
>>>> {
>>>> - unsigned int timeout = card->ext_csd.generic_cmd6_time;
>>>> + unsigned int timeout;
>>>> + bool use_busy_signal = true;
>>>> int err;
>>>>
>>>> - /* Use EXT_CSD_POWER_OFF_SHORT as default notification type. */
>>>> - if (notify_type == EXT_CSD_POWER_OFF_LONG)
>>>> + switch (notify_type) {
>>>> + case EXT_CSD_POWER_OFF_LONG:
>>>> timeout = card->ext_csd.power_off_longtime;
>>>> + break;
>>>> + case EXT_CSD_SLEEP_NOTIFICATION:
>>>> + timeout = card->ext_csd.sleep_notification_time;
>>>> + use_busy_signal = false;
>>>
>>> This is wrong.
>>>
>>> If you set use_busy_signal to false, it means that we don't care about
>>> waiting for the card to stop signaling busy on DAT0 after setting
>>> EXT_CSD_SLEEP_NOTIFICATION. This then becomes a violation of the eMMC
>>> spec, because we must not issue a CMD5 before the card have stopped
>>> signaling busy.
>>>
>>> I realize that for those devices that don't support
>>> MMC_CAP_WAIT_WHILE_BUSY or have the ->card_busy() callback
>>> implemented, the consequence may be that mmc_poll_for_busy() may call
>>> mmc_delay() with a very big timeout. This could be a big problem, I
>>> guess.
>>
>> Well, it may also be a big problem even if the host supports
>> ->card_busy() and/or MMC_CAP_WAIT_WHILE_BUSY (but maybe not as big),
>> simply because we can't be waiting here for several seconds to allow
>> the card to deal with background operations.
>>
>> I guess we need to try and see what happens. Perhaps you can share
>> some data about the timeouts you get?
>>
>>>
>>>> + break;
>>>> + default:
>>>> + /* Use EXT_CSD_POWER_OFF_SHORT as default notification type. */
>>>> + timeout = card->ext_csd.generic_cmd6_time;
>>>> + }
>>>>
>>>> err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>>> EXT_CSD_POWER_OFF_NOTIFICATION,
>>>> - notify_type, timeout, 0, true, false, false);
>>>> + notify_type, timeout, 0, use_busy_signal, false, false);
>>>> if (err)
>>>> pr_err("%s: Power Off Notification timed out, %u\n",
>>>> mmc_hostname(card->host), timeout);
>>
>> [...]
>>
>> Kind regards
>> Uffe
>
> Gwendal, any news on this?
>
> As changes for the SLEEP_NOTIFICATION keeps being posted, clearly
> there is a need for us to deal with it in some way. I have been
> thinking of a fall-back solution, if we can't get it it work, but
> anyway I am eager to find a solution so we finally can sort this out.
>
> Kind regards
> Uffe
^ permalink raw reply
* Re: [PATCH V5] mmc:host:sdhci-pci:Addition of Arasan PCI Controller with integrated phy.
From: Philippe Ombredanne @ 2017-12-19 16:00 UTC (permalink / raw)
To: Atul Garg
Cc: linux-mmc, Kishon Vijay Abraham I, rk, nm, nsekhar, Ulf Hansson,
LKML, Adrian Hunter
In-Reply-To: <1f026a6e-aabc-66b3-2753-00c23bf61a27@intel.com>
Atul,
On Tue, Dec 19, 2017 at 1:18 PM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 12/12/17 19:02, Atul Garg wrote:
>> The Arasan Controller is based on a FPGA platform and has integrated phy
>> with specific registers used during initialization and
>> management of different modes. The phy and the controller are integrated
>> and registers are very specific to Arasan.
>>
>> Arasan being an IP provider, licenses these IPs to various companies for
>> integration of IP in custom SOCs. The custom SOCs define own register
>> map depending on how bits are tied inside the SOC for phy registers,
>> depending on SOC memory plan and hence will require own platform drivers.
>>
<snip>
>> --- /dev/null
>> +++ b/drivers/mmc/host/sdhci-pci-arasan.c
>> @@ -0,0 +1,341 @@
>> +/*
>> + * Copyright (C) 2017 Arasan Chip Systems Inc.
>> + *
>> + * Author: Atul Garg <agarg@arasan.com>
>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
Could you consider using the new SPDX tags documented in Thomas patch set
[1] rather than this fine but longer legalese?
Thank you!
[1] https://lkml.org/lkml/2017/12/4/934
--
Cordially
Philippe Ombredanne
^ permalink raw reply
* [PATCH] mmc: tmio: use io* accessors consistently
From: Wolfram Sang @ 2017-12-19 13:34 UTC (permalink / raw)
To: linux-mmc; +Cc: linux-renesas-soc, Masahiro Yamada, Simon Horman, Wolfram Sang
Because we started using io*_rep accessors previously because they are
more widely defined across architectures, let's be consistent and use
this family for all accessor wrappers.
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
And here is the follow-up patch. Tested on a R-Car M3-W; still could checksum a
huge file from SD without performance regressions.
drivers/mmc/host/tmio_mmc.h | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 03519c4ca0aa1a..e7d651352dc90f 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -227,7 +227,7 @@ int tmio_mmc_host_runtime_resume(struct device *dev);
static inline u16 sd_ctrl_read16(struct tmio_mmc_host *host, int addr)
{
- return readw(host->ctl + (addr << host->bus_shift));
+ return ioread16(host->ctl + (addr << host->bus_shift));
}
static inline void sd_ctrl_read16_rep(struct tmio_mmc_host *host, int addr,
@@ -239,8 +239,8 @@ static inline void sd_ctrl_read16_rep(struct tmio_mmc_host *host, int addr,
static inline u32 sd_ctrl_read16_and_16_as_32(struct tmio_mmc_host *host,
int addr)
{
- return readw(host->ctl + (addr << host->bus_shift)) |
- readw(host->ctl + ((addr + 2) << host->bus_shift)) << 16;
+ return ioread16(host->ctl + (addr << host->bus_shift)) |
+ ioread16(host->ctl + ((addr + 2) << host->bus_shift)) << 16;
}
static inline void sd_ctrl_read32_rep(struct tmio_mmc_host *host, int addr,
@@ -257,7 +257,7 @@ static inline void sd_ctrl_write16(struct tmio_mmc_host *host, int addr,
*/
if (host->write16_hook && host->write16_hook(host, addr))
return;
- writew(val, host->ctl + (addr << host->bus_shift));
+ iowrite16(val, host->ctl + (addr << host->bus_shift));
}
static inline void sd_ctrl_write16_rep(struct tmio_mmc_host *host, int addr,
@@ -269,8 +269,8 @@ static inline void sd_ctrl_write16_rep(struct tmio_mmc_host *host, int addr,
static inline void sd_ctrl_write32_as_16_and_16(struct tmio_mmc_host *host,
int addr, u32 val)
{
- writew(val & 0xffff, host->ctl + (addr << host->bus_shift));
- writew(val >> 16, host->ctl + ((addr + 2) << host->bus_shift));
+ iowrite16(val & 0xffff, host->ctl + (addr << host->bus_shift));
+ iowrite16(val >> 16, host->ctl + ((addr + 2) << host->bus_shift));
}
static inline void sd_ctrl_write32_rep(struct tmio_mmc_host *host, int addr,
--
2.11.0
^ permalink raw reply related
* Re: [RFC PATCH] mmc: tmio: use ioread* for repeated access to a register
From: Wolfram Sang @ 2017-12-19 13:20 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Ulf Hansson, Wolfram Sang, linux-mmc@vger.kernel.org,
Linux-Renesas, Simon Horman
In-Reply-To: <CAK7LNAS_rRp0RDbmEhahKPkcYJ=Yopj7WGRcuhQLz5WP+FFE1w@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 643 bytes --]
Yamada-san,
> BTW, is ioread* preferred to read*?
Technically, yes. In reality, mostly not.
> We need to eliminate the root cause.
> Other drivers using reads* cannot enable COMPILE_TEST for the same reason.
Sure, I understand this. There are not many left, however. I'd assume
most switched to io*_rep. The sparc64 issue is known for at least two
years.
> Wolfram,
> Can you send a patch for sparc64?
As I wrote here already [1], this is too much of a side task for me. But
if you like, please have a go...
But I'll send the consistency patch for tmio in a second.
Kind regards,
Wolfram
[1] https://lkml.org/lkml/2017/12/15/816
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v6 4/4] mmc: sdio support external scatter gather list
From: Ulf Hansson @ 2017-12-19 12:39 UTC (permalink / raw)
To: Xinming Hu
Cc: Linux MMC, Kalle Valo, Arend van Spriel, Franky Lin,
Hante Meuleman, Chi-Hsien Lin, Wright Feng, Zhiyuan Yang,
Tim Song, Cathy Luo, James Cao, Ganapathi Bhat, Xu Wang, Bob Tan,
Amitkumar Karwar
In-Reply-To: <1513685211-640-4-git-send-email-huxm@marvell.com>
On 19 December 2017 at 13:06, Xinming Hu <huxm@marvell.com> wrote:
> Currently sdio device drivers need to prepare a big continuous
> physical memory for large data transfer. mmc stack will construct
> scatter/gather buffer list to make use of ADMA2.
>
> According to the sdio host controller specification version 4.00
> chapter 1.13.2, sdio device drivers have the ability to construct
> scatter/gather DMA buffer list. In this way, they do not need to allocate
> big memory.
>
> This patch refactors current sdio command 53 wrapper mmc_io_rw_extended,
> so that it can accept external scatter/gather buffer list from its caller,
> such as the sdio device driver. This patch is very useful on some embedded
> systems where large memory allocation fails sometimes. It gives 10 to 15%
> better throughput performance compared to a case in which small single
> data transfers are done.
>
> Signed-off-by: Xinming Hu <huxm@marvell.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> ---
> v4: same as v3/v2/v1
> v5: rebased on latest codebase
> v6: same as v5
> ---
> drivers/mmc/core/sdio_io.c | 22 ++++++++++++++++--
> drivers/mmc/core/sdio_ops.c | 53 +++++++++++++++++++++++++++++++++++++++----
> drivers/mmc/core/sdio_ops.h | 3 ++-
> include/linux/mmc/sdio_func.h | 6 +++++
> 4 files changed, 76 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
> index 4806521..6046511 100644
> --- a/drivers/mmc/core/sdio_io.c
> +++ b/drivers/mmc/core/sdio_io.c
> @@ -327,7 +327,7 @@ static int sdio_io_rw_ext_helper(struct sdio_func *func, int write,
> size = blocks * func->cur_blksize;
>
> ret = mmc_io_rw_extended(func->card, write,
> - func->num, addr, incr_addr, buf,
> + func->num, addr, incr_addr, false, buf,
> blocks, func->cur_blksize);
> if (ret)
> return ret;
> @@ -345,7 +345,7 @@ static int sdio_io_rw_ext_helper(struct sdio_func *func, int write,
>
> /* Indicate byte mode by setting "blocks" = 0 */
> ret = mmc_io_rw_extended(func->card, write, func->num, addr,
> - incr_addr, buf, 0, size);
> + incr_addr, false, buf, 0, size);
> if (ret)
> return ret;
>
> @@ -513,6 +513,24 @@ int sdio_writesb(struct sdio_func *func, unsigned int addr, void *src,
> }
> EXPORT_SYMBOL_GPL(sdio_writesb);
>
> +int sdio_readsb_sg_enh(struct sdio_func *func, unsigned int addr,
Could you please rename this to sdio_readsb_sg() instead?
> + struct sg_table *dst)
I think what Christoph suggested and I agree with, is to use a "struct
scatterlist *sg" instead of "struct sg_table *dst" as the
in-parameter.
The similar applies for the write API, of course.
> +{
> + return mmc_io_rw_extended(func->card, 0, func->num,
> + addr, 0, true,
> + dst, 0, func->cur_blksize);
> +}
> +EXPORT_SYMBOL_GPL(sdio_readsb_sg_enh);
> +
> +int sdio_writesb_sg_enh(struct sdio_func *func, unsigned int addr,
Could you please rename this to sdio_writesb_sg() instead?
> + struct sg_table *src)
> +{
> + return mmc_io_rw_extended(func->card, 1, func->num,
> + addr, 0, true,
> + src, 0, func->cur_blksize);
> +}
> +EXPORT_SYMBOL_GPL(sdio_writesb_sg_enh);
> +
> /**
> * sdio_readw - read a 16 bit integer from a SDIO function
> * @func: SDIO function to access
> diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
> index abaaba3..2310a2a 100644
> --- a/drivers/mmc/core/sdio_ops.c
> +++ b/drivers/mmc/core/sdio_ops.c
> @@ -115,16 +115,39 @@ int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn,
> return mmc_io_rw_direct_host(card->host, write, fn, addr, in, out);
> }
>
> +/**
> + * mmc_io_rw_extended - wrapper for sdio command 53 read/write operation
> + * @card: destination device for read/write
> + * @write: zero indcate read, non-zero indicate write.
> + * @fn: SDIO function to access
> + * @addr: address of (single byte) FIFO
> + * @incr_addr: set to 1 indicate read or write multiple bytes
> + of data to/from an IO register address that
> + increment by 1 after each operation.
> + * @sg_enhance: if set true, the caller of this function will
> + prepare scatter gather dma buffer list.
> + * @buf: buffer that contains the data to write. if sg_enhance
> + is set, it point to SG dma buffer list.
> + * @blocks: number of blocks of data transfer.
> + if set zero, indicate byte mode
> + if set non-zero, indicate block mode
> + if sg_enhance is set, this parameter will not be used.
> + * @blksz: block size for block mode data transfer.
> + *
> + */
The descriptive function header isn't required here, because this is
an internal function for the mmc core.
However, if you really want this, I suggest you make it a separate patch.
> int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
> - unsigned addr, int incr_addr, u8 *buf, unsigned blocks, unsigned blksz)
> + unsigned int addr, int incr_addr, bool sg_enhance,
Instead of letting this take a new bool variable, let's instead
provide it with a "struct scatterlist *sg". In cases when it isn't
used, just provide NULL.
Depending on the end result, we may even consider to make a separate
function dealing with the scatterlist case, perhaps via sharing some
common functionality from mmc_io_rw_extended() instead.
> + void *buf, unsigned int blocks, unsigned int blksz)
> {
> struct mmc_request mrq = {};
> struct mmc_command cmd = {};
> struct mmc_data data = {};
> struct scatterlist sg, *sg_ptr;
> struct sg_table sgtable;
> + struct sg_table *sgtable_external;
> unsigned int nents, left_size, i;
> unsigned int seg_size = card->host->max_seg_size;
> + unsigned int sg_blocks = 0;
>
> WARN_ON(blksz == 0);
>
> @@ -140,16 +163,35 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
> cmd.arg |= fn << 28;
> cmd.arg |= incr_addr ? 0x04000000 : 0x00000000;
> cmd.arg |= addr << 9;
> + cmd.flags = MMC_RSP_SPI_R5 | MMC_RSP_R5 | MMC_CMD_ADTC;
> +
> + data.flags = write ? MMC_DATA_WRITE : MMC_DATA_READ;
> + data.blksz = blksz;
> +
> + if (sg_enhance) {
> + sgtable_external = buf;
> +
> + for_each_sg(sgtable_external->sgl, sg_ptr,
> + sgtable_external->nents, i) {
> + if (sg_ptr->length > card->host->max_seg_size)
> + return -EINVAL;
> + sg_blocks += DIV_ROUND_UP(sg_ptr->length, blksz);
This looks wrong. For each iteration the number of sg_blocks will be
increased, with the round up method.
This may lead to that the data.blocks could get a in-correct value
(bigger) when assigned below, right?
> + }
> +
> + cmd.arg |= 0x08000000 | sg_blocks;
This isn't going to work for byte mode transfers. Seems like that
should be possible too when using a pre-allocated scatterlist, right!?
> + data.blocks = sg_blocks;
> + data.sg = sgtable_external->sgl;
> + data.sg_len = sgtable_external->nents;
> + goto mmc_data_req;
> + }
> +
> if (blocks == 0)
> cmd.arg |= (blksz == 512) ? 0 : blksz; /* byte mode */
> else
> cmd.arg |= 0x08000000 | blocks; /* block mode */
> - cmd.flags = MMC_RSP_SPI_R5 | MMC_RSP_R5 | MMC_CMD_ADTC;
>
> - data.blksz = blksz;
> /* Code in host drivers/fwk assumes that "blocks" always is >=1 */
> data.blocks = blocks ? blocks : 1;
> - data.flags = write ? MMC_DATA_WRITE : MMC_DATA_READ;
>
> left_size = data.blksz * data.blocks;
> nents = DIV_ROUND_UP(left_size, seg_size);
> @@ -172,11 +214,12 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
> sg_init_one(&sg, buf, left_size);
> }
>
> +mmc_data_req:
> mmc_set_data_timeout(&data, card);
>
> mmc_wait_for_req(card->host, &mrq);
>
> - if (nents > 1)
> + if (!sg_enhance && nents > 1)
> sg_free_table(&sgtable);
>
> if (cmd.error)
> diff --git a/drivers/mmc/core/sdio_ops.h b/drivers/mmc/core/sdio_ops.h
> index 96945ca..2d75100 100644
> --- a/drivers/mmc/core/sdio_ops.h
> +++ b/drivers/mmc/core/sdio_ops.h
> @@ -23,7 +23,8 @@
> int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn,
> unsigned addr, u8 in, u8* out);
> int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
> - unsigned addr, int incr_addr, u8 *buf, unsigned blocks, unsigned blksz);
> + unsigned int addr, int incr_addr, bool sg_enhance,
> + void *buf, unsigned int blocks, unsigned int blksz);
> int sdio_reset(struct mmc_host *host);
> unsigned int mmc_align_data_size(struct mmc_card *card, unsigned int sz);
> void sdio_irq_work(struct work_struct *work);
> diff --git a/include/linux/mmc/sdio_func.h b/include/linux/mmc/sdio_func.h
> index 72d4de4..d36ba47 100644
> --- a/include/linux/mmc/sdio_func.h
> +++ b/include/linux/mmc/sdio_func.h
> @@ -14,6 +14,7 @@
>
> #include <linux/device.h>
> #include <linux/mod_devicetable.h>
> +#include <linux/scatterlist.h>
>
> #include <linux/mmc/pm.h>
>
> @@ -136,6 +137,11 @@ extern int sdio_memcpy_fromio(struct sdio_func *func, void *dst,
> extern int sdio_readsb(struct sdio_func *func, void *dst,
> unsigned int addr, int count);
>
> +int sdio_readsb_sg_enh(struct sdio_func *func, unsigned int addr,
> + struct sg_table *dst);
> +int sdio_writesb_sg_enh(struct sdio_func *func, unsigned int addr,
> + struct sg_table *src);
> +
> extern void sdio_writeb(struct sdio_func *func, u8 b,
> unsigned int addr, int *err_ret);
> extern void sdio_writew(struct sdio_func *func, u16 b,
> --
> 1.9.1
>
Kind regards
Uffe
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox