mmc: mmci: Improved PM support, cleanup and bugfixes
Subsystems (5)
cover mmc: mmci: Improved PM support, cleanup and bugfixes
This patchserie is based on Linux 3.2-rc3 and contains the following:
Patch 1 -> 4:
Minor changes for bugs and some performance optimizations. Previously sent to the mailing list.
Patch 5 -> 7:
Minor changes affecting how to handle a levelshifter. Previously sent to the mailing list.
Patch 8:
Bugfix for dma. Previously sent to the mailing list, ongoing discussion.
Patch 9 -> 14:
Improvement of PM support.
An earlier patch around extending the PM runtime support for mmci has been discussed on the mailing list rather recently. Those review comment's has been considered in these patches.
This patchstack is becomming quite big. Hopefully it should still be possible review each piece separate. Those patches that earlier have been uploaded into Russell's patchtracker will be replaced with these new rebased versions.
Sebastian Rasmussen (1):
mmc: mmci: Put power register deviations in variant data
Ulf Hansson (13):
mmc: mmci: Support MMC_PM_KEEP_POWER
mmc: mmci: Fixup handling of MCI_STARTBITERR
mmc: mmci: Increase max_segs from 16 to 128
mmc: mmci: Do not release spinlock in request_end
mmc: mmci: Provide option to configure bus signal direction
mmc: mmci: Change vdd_handler to a generic ios_handler
mmc: mmci: Fixup error handling for dma
mmc: mmci: Change from using legacy suspend
mmc: mmci: Cache MMCICLOCK and MMCIPOWER register
mmc: mmci: Fixup use of runtime PM and use autosuspend
mmc: mmci: Decrease current consumption in suspend
mmc: mmci: Implement PM runtime callbacks to save power
mmc: mmci: Use ios_handler to save power
arch/arm/mach-ux500/board-mop500-sdi.c | 21 +-- drivers/mmc/host/mmci.c | 266 ++++++++++++++++++++++++++------ drivers/mmc/host/mmci.h | 15 +-- include/linux/amba/mmci.h | 22 +++- 4 files changed, 252 insertions(+), 72 deletions(-)
Signature · 1 line
1.7.5.4
PATCH 1/14 mmc: mmci: Support MMC_PM_KEEP_POWER
Add MMC_PM_KEEP_POWER to pm_caps so SDIO clients are able to use this option to prevent power off in suspend.
--- drivers/mmc/host/mmci.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
drivers/mmc/host/mmci.c +3 -0
@@ -1240,6 +1240,9 @@ static int __devinit mmci_probe(struct amba_device *dev, mmc->ocr_avail = plat->ocr_mask; mmc->caps = plat->capabilities; +/* We support these PM capabilities. */ + mmc->pm_caps =MMC_PM_KEEP_POWER ; +/* * We can do SGIO */
Signature · 1 line
1.7.5.4
PATCH 2/14 mmc: mmci: Fixup handling of MCI_STARTBITERR
In reply to Ulf HanssonThe interrupt was previously enabled and then correctly cleared. Now we also handle it correctly.
--- drivers/mmc/host/mmci.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-)
drivers/mmc/host/mmci.c +5 -3
@@ -675,7 +675,8 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data, unsigned int status) {/* First check for errors */ -if (status & (MCI_DATACRCFAIL |MCI_DATATIMEOUT |MCI_TXUNDERRUN |MCI_RXOVERRUN )) { +if (status & (MCI_DATACRCFAIL |MCI_DATATIMEOUT |MCI_STARTBITERR | +MCI_TXUNDERRUN |MCI_RXOVERRUN )) {u32 remain, success;/* Terminate the DMA transfer */
@@ -955,8 +956,9 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)dev_dbg (mmc_dev (host->mmc),"irq0 (data+cmd) %08x\n" , status); data = host->data; -if (status & (MCI_DATACRCFAIL |MCI_DATATIMEOUT |MCI_TXUNDERRUN | -MCI_RXOVERRUN |MCI_DATAEND |MCI_DATABLOCKEND ) && data) +if (status & (MCI_DATACRCFAIL |MCI_DATATIMEOUT |MCI_STARTBITERR | +MCI_TXUNDERRUN |MCI_RXOVERRUN |MCI_DATAEND | +MCI_DATABLOCKEND ) && data)mmci_data_irq (host, data, status); cmd = host->cmd;
Signature · 1 line
1.7.5.4
PATCH 2/14 mmc: mmci: Fixup handling of MCI_STARTBITERR
Quoted 5 lines · Ulf Hansson
The interrupt was previously enabled and then correctly cleared. Now we also handle it correctly.
Signed-off-by Ulf Hansson <[email protected]>Acked-by Linus Walleij <[email protected]>
I've queued this in my 'fixes' branch with a Cc to the stable team for previous kernel versions. I believe that to be appropriate. Please confirm.
PATCH 2/14 mmc: mmci: Fixup handling of MCI_STARTBITERR
Quoted 11 lines · Russell King - ARM Linux
Quoted 5 lines · Ulf Hansson
The interrupt was previously enabled and then correctly cleared. Now we also handle it correctly.
Signed-off-by Ulf Hansson <[email protected]>Acked-by Linus Walleij <[email protected]>I've queued this in my 'fixes' branch with a Cc to the stable team for previous kernel versions. I believe that to be appropriate. Please confirm.
Great!
BR
Ulf Hansson
PATCH 3/14 mmc: mmci: Increase max_segs from 16 to 128
In reply to Ulf HanssonA significant increase (10-20%) in performance throughput for USB mass storage is the reason for incrementing the value. By some reason the USB driver allocates buffers which requires a scattergather list to contain a lot more than 16 elements to get optimal performance. This change sets the maximum elements to 128.
Tests with large reads and large writes (100 MiB) show that the throughput increase is significant for write (10% for this test) but not for read. Tests are run on a Linux host with ext4 FS on the gadget mass storage device. The sg-len still exceeds 16 for the read tests but the performance gain is low or nothing.
--- drivers/mmc/host/mmci.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/mmc/host/mmci.h +1 -1
@@ -160,7 +160,7 @@ (MCI_RXFIFOHALFFULLMASK |MCI_RXDATAAVLBLMASK | \ MCI_TXFIFOHALFEMPTYMASK) -#define NR_SG 16 +#define NR_SG 128struct clk ;struct variant_data ;
Signature · 1 line
1.7.5.4
PATCH 4/14 mmc: mmci: Do not release spinlock in request_end
In reply to Ulf HanssonThe patch "mmc: core: move ->request() call from atomic context", is the reason to why this change is possible. This simplifies the error handling code execution path quite a lot and potentially also fixes some error handling hang problems.
--- drivers/mmc/host/mmci.c | 6 ------ 1 files changed, 0 insertions(+), 6 deletions(-)
drivers/mmc/host/mmci.c +0 -6
@@ -166,14 +166,8 @@ mmci_request_end(struct mmci_host *host, struct mmc_request *mrq) host->mrq =NULL ; host->cmd =NULL ; -/* -* Need to drop the host lock here; mmc_request_done may call -* back into the driver... -*/ -spin_unlock (&host->lock);pm_runtime_put (mmc_dev (host->mmc));mmc_request_done (host->mmc, mrq); -spin_lock (&host->lock); }static void mmci_set_mask1 (struct mmci_host *host,unsigned int mask)
Signature · 1 line
1.7.5.4
PATCH 5/14 mmc: mmci: Put power register deviations in variant data
In reply to Ulf HanssonFrom: Sebastian Rasmussen <[email protected]>
Use variant data to store hardware controller deviations concerning power registers to improve readability of the code.
--- drivers/mmc/host/mmci.c | 22 +++++++++++++++++----- 1 files changed, 17 insertions(+), 5 deletions(-)
drivers/mmc/host/mmci.c +17 -5
@@ -53,6 +53,7 @@ static unsigned int fmax = 515633;* @sdio: variant supports SDIO * @st_clkdiv: true if using a ST-specific clock divider algorithm * @blksz_datactrl16: true if Block size is at b16..b30 position in datactrl register +* @pwrreg_powerup: power up value for MMCIPOWER register */ struct variant_data {unsigned int clkreg;
@@ -63,18 +64,21 @@ struct variant_data {bool sdio;bool st_clkdiv;bool blksz_datactrl16; +u32 pwrreg_powerup; };static struct variant_data variant_arm = { .fifosize =16 *4 , .fifohalfsize =8 *4 , .datalength_bits =16 , + .pwrreg_powerup =MCI_PWR_UP , };static struct variant_data variant_arm_extended_fifo = { .fifosize =128 *4 , .fifohalfsize =64 *4 , .datalength_bits =16 , + .pwrreg_powerup =MCI_PWR_UP , };static struct variant_data variant_u300 = {
@@ -83,6 +87,7 @@ static struct variant_data variant_u300 = { .clkreg_enable = MCI_ST_U300_HWFCEN, .datalength_bits = 16, .sdio = true, + .pwrreg_powerup = MCI_PWR_ON, };static struct variant_data variant_ux500 = {
@@ -93,6 +98,7 @@ static struct variant_data variant_ux500 = { .datalength_bits = 24, .sdio = true, .st_clkdiv = true, + .pwrreg_powerup = MCI_PWR_ON, };static struct variant_data variant_ux500v2 = {
@@ -104,6 +110,7 @@ static struct variant_data variant_ux500v2 = { .sdio = true, .st_clkdiv = true, .blksz_datactrl16 = true, + .pwrreg_powerup = MCI_PWR_ON, };/*
@@ -1002,6 +1009,7 @@ static void mmci_request(struct mmc_host *mmc, struct mmc_request *mrq)static void mmci_set_ios (struct mmc_host *mmc,struct mmc_ios *ios) {struct mmci_host *host =mmc_priv (mmc); +struct variant_data *variant = host->variant;u32 pwr =0 ;unsigned long flags;int ret;
@@ -1028,11 +1036,15 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)if (host->plat->vdd_handler) pwr |= host->plat->vdd_handler (mmc_dev (mmc), ios->vdd, ios->power_mode); -/* The ST version does not have this, fall through to POWER_ON */ -if (host->hw_designer !=AMBA_VENDOR_ST ) { - pwr |=MCI_PWR_UP ; -break ; - } + +/* +* The ST Micro variant doesn't have the PL180s MCI_PWR_UP +* and instead uses MCI_PWR_ON so apply whatever value is +* configured in the variant data. +*/ + pwr |= variant->pwrreg_powerup; + +break ;case MMC_POWER_ON : pwr |=MCI_PWR_ON ;break ;
Signature · 1 line
1.7.5.4
PATCH 6/14 mmc: mmci: Provide option to configure bus signal direction
In reply to Ulf HanssonThe ST Micro variant supports bus signal direction indication. A new member in the variant struct is added for this.
Moreover the actual signal direction configuration is board specific, thus the amba mmci platform data is extended with a new member to be able provide mmci with these specific board configurations.
This patch is based upon a patch from Sebastian Rasmussen.
--- drivers/mmc/host/mmci.c | 21 +++++++++++++++++++++ drivers/mmc/host/mmci.h | 10 ---------- include/linux/amba/mmci.h | 16 ++++++++++++++++ 3 files changed, 37 insertions(+), 10 deletions(-)
drivers/mmc/host/mmci.c +21 -0
@@ -54,6 +54,7 @@ static unsigned int fmax = 515633;* @st_clkdiv: true if using a ST-specific clock divider algorithm * @blksz_datactrl16: true if Block size is at b16..b30 position in datactrl register * @pwrreg_powerup: power up value for MMCIPOWER register +* @signal_direction: input/out direction of bus signals can be indicated */ struct variant_data {unsigned int clkreg;
@@ -65,6 +66,7 @@ struct variant_data {bool st_clkdiv;bool blksz_datactrl16;u32 pwrreg_powerup; +bool signal_direction; };static struct variant_data variant_arm = {
@@ -88,6 +90,7 @@ static struct variant_data variant_u300 = { .datalength_bits = 16, .sdio = true, .pwrreg_powerup = MCI_PWR_ON, + .signal_direction = true, };static struct variant_data variant_ux500 = {
@@ -99,6 +102,7 @@ static struct variant_data variant_ux500 = { .sdio = true, .st_clkdiv = true, .pwrreg_powerup = MCI_PWR_ON, + .signal_direction = true, };static struct variant_data variant_ux500v2 = {
@@ -111,6 +115,7 @@ static struct variant_data variant_ux500v2 = { .st_clkdiv = true, .blksz_datactrl16 = true, .pwrreg_powerup = MCI_PWR_ON, + .signal_direction = true, };/*
@@ -1050,6 +1055,22 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)break ; } +if (variant->signal_direction && ios->power_mode !=MMC_POWER_OFF ) { +/* +* The ST Micro variant has some additional bits +* indicating signal direction for the signals in +* the SD/MMC bus and feedback-clock usage. +*/ + pwr |= host->plat->sigdir; + +if (ios->bus_width ==MMC_BUS_WIDTH_4 ) + pwr &= ~MCI_ST_DATA74DIREN ; +else if (ios->bus_width ==MMC_BUS_WIDTH_1 ) + pwr &= (~MCI_ST_DATA74DIREN & + ~MCI_ST_DATA31DIREN & + ~MCI_ST_DATA2DIREN ); + } +if (ios->bus_mode ==MMC_BUSMODE_OPENDRAIN ) {if (host->hw_designer !=AMBA_VENDOR_ST ) pwr |=MCI_ROD ;
drivers/mmc/host/mmci.h +0 -10
@@ -13,16 +13,6 @@#define MCI_PWR_ON 0x03#define MCI_OD (1 << 6)#define MCI_ROD (1 << 7) -/* -* The ST Micro version does not have ROD and reuse the voltage registers -* for direction settings -*/ -#define MCI_ST_DATA2DIREN (1 << 2) -#define MCI_ST_CMDDIREN (1 << 3) -#define MCI_ST_DATA0DIREN (1 << 4) -#define MCI_ST_DATA31DIREN (1 << 5) -#define MCI_ST_FBCLKEN (1 << 7) -#define MCI_ST_DATA74DIREN (1 << 8)#define MMCICLOCK 0x004#define MCI_CLK_ENABLE (1 << 8)
include/linux/amba/mmci.h +16 -0
@@ -6,6 +6,19 @@#include <linux/mmc/host.h> + +/* +* These defines is places here due to access is needed from machine +* configuration files. The ST Micro version does not have ROD and +* reuse the voltage registers for direction settings. +*/ +#define MCI_ST_DATA2DIREN (1 << 2) +#define MCI_ST_CMDDIREN (1 << 3) +#define MCI_ST_DATA0DIREN (1 << 4) +#define MCI_ST_DATA31DIREN (1 << 5) +#define MCI_ST_FBCLKEN (1 << 7) +#define MCI_ST_DATA74DIREN (1 << 8) +/* Just some dummy forwarding */ struct dma_chan ;
@@ -30,6 +43,8 @@ struct dma_chan;* @cd_invert: true if the gpio_cd pin value is active low * @capabilities: the capabilities of the block as implemented in * this platform, signify anything MMC_CAP_* from mmc/host.h +* @sigdir: a bit field indicating for what bits in the MMC bus the host +* should enable signal direction indication. * @dma_filter: function used to select an appropriate RX and TX * DMA channel to be used for DMA, if and only if you're deploying the * generic DMA engine
@@ -52,6 +67,7 @@ struct mmci_platform_data {int gpio_cd;bool cd_invert;unsigned long capabilities; +u32 sigdir;bool (*dma_filter)(struct dma_chan *chan,void *filter_param);void *dma_rx_param;void *dma_tx_param;
Signature · 1 line
1.7.5.4
PATCH 7/14 mmc: mmci: Change vdd_handler to a generic ios_handler
In reply to Ulf HanssonThe purpose of the vdd_handler does not make sense. We remove it and use a generic approach instead. A new ios_handler is added, the purpose of which e.g. can be to control GPIO pins to a levelshifter.
Previously the vdd_handler was also used for making additional changes to the power register bits. This option is superfluous and is therefore removed.
Adaptaptions from the old vdd_handler to the new ios_handler is done for mach-ux500 board, which was the only one using the vdd_handler.
This patch is based upon a patch from Sebastian Rasmussen.
--- arch/arm/mach-ux500/board-mop500-sdi.c | 21 ++++++++------------- drivers/mmc/host/mmci.c | 8 ++++---- include/linux/amba/mmci.h | 6 +++--- 3 files changed, 15 insertions(+), 20 deletions(-)
arch/arm/mach-ux500/board-mop500-sdi.c +8 -13
@@ -25,21 +25,13 @@* SDI 0 (MicroSD slot) */ -/* MMCIPOWER bits */ -#define MCI_DATA2DIREN (1 << 2) -#define MCI_CMDDIREN (1 << 3) -#define MCI_DATA0DIREN (1 << 4) -#define MCI_DATA31DIREN (1 << 5) -#define MCI_FBCLKEN (1 << 7) -/* GPIO pins used by the sdi0 level shifter */ static int sdi0_en =-1 ;static int sdi0_vsel =-1 ; -static u32 mop500_sdi0_vdd_handler (struct device *dev,unsigned int vdd, -unsigned char power_mode) +static int mop500_sdi0_ios_handler (struct device *dev,struct mmc_ios *ios) { -switch (power_mode) { +switch (ios->power_mode) {case MMC_POWER_UP :case MMC_POWER_ON :/*
@@ -59,8 +51,7 @@ static u32 mop500_sdi0_vdd_handler(struct device *dev, unsigned int vdd,break ; } -return MCI_FBCLKEN |MCI_CMDDIREN |MCI_DATA0DIREN | -MCI_DATA2DIREN |MCI_DATA31DIREN ; +return 0 ; }#ifdef CONFIG_STE_DMA40
@@ -84,13 +75,17 @@ static struct stedma40_chan_cfg mop500_sdi0_dma_cfg_tx = {#endif static struct mmci_platform_data mop500_sdi0_data = { - .vdd_handler = mop500_sdi0_vdd_handler, + .ios_handler = mop500_sdi0_ios_handler, .ocr_mask =MMC_VDD_29_30 , .f_max =50000000 , .capabilities =MMC_CAP_4_BIT_DATA |MMC_CAP_SD_HIGHSPEED |MMC_CAP_MMC_HIGHSPEED , .gpio_wp =-1 , + .sigdir =MCI_ST_FBCLKEN | +MCI_ST_CMDDIREN | +MCI_ST_DATA0DIREN | +MCI_ST_DATA2DIREN , #ifdef CONFIG_STE_DMA40 .dma_filter = stedma40_filter, .dma_rx_param = &mop500_sdi0_dma_cfg_rx,
drivers/mmc/host/mmci.c +4 -4
@@ -1019,6 +1019,10 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)unsigned long flags;int ret; +if (host->plat->ios_handler && + host->plat->ios_handler (mmc_dev (mmc), ios)) +dev_err (mmc_dev (mmc),"platform ios_handler failed\n" ); +switch (ios->power_mode) {case MMC_POWER_OFF :if (host->vcc)
@@ -1038,10 +1042,6 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)return ; } } -if (host->plat->vdd_handler) - pwr |= host->plat->vdd_handler (mmc_dev (mmc), ios->vdd, - ios->power_mode); -/* * The ST Micro variant doesn't have the PL180s MCI_PWR_UP * and instead uses MCI_PWR_ON so apply whatever value is
include/linux/amba/mmci.h +3 -3
@@ -31,7 +31,8 @@ struct dma_chan;* @ocr_mask: available voltages on the 4 pins from the block, this * is ignored if a regulator is used, see the MMC_VDD_* masks in * mmc/host.h -* @vdd_handler: a callback function to translate a MMC_VDD_* +* @ios_handler: a callback function to act on specfic ios changes, +* used for example to control a levelshifter * mask into a value to be binary (or set some other custom bits * in MMCIPWR) or:ed and written into the MMCIPWR register of the * block. May also control external power based on the power_mode.
@@ -60,8 +61,7 @@ struct dma_chan;struct mmci_platform_data {unsigned int f_max;unsigned int ocr_mask; -u32 (*vdd_handler)(struct device *,unsigned int vdd, -unsigned char power_mode); +int (*ios_handler)(struct device *,struct mmc_ios *);unsigned int (*status)(struct device *);int gpio_wp;int gpio_cd;
Signature · 1 line
1.7.5.4
PATCH 8/14 mmc: mmci: Fixup error handling for dma
In reply to Ulf HanssonWhen getting a cmd irq during an ongoing data transfer with dma, the dma job were never terminated. This is now corrected.
--- drivers/mmc/host/mmci.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
drivers/mmc/host/mmci.c +5 -1
@@ -761,8 +761,12 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, }if (!cmd->data || cmd->error) { -if (host->data) +if (host->data) { +/* Terminate the DMA transfer */ +if (dma_inprogress (host)) +mmci_dma_data_error (host);mmci_stop_data (host); + }mmci_request_end (host, cmd->mrq); }else if (!(cmd->data->flags &MMC_DATA_READ )) {mmci_start_data (host, cmd->data);
Signature · 1 line
1.7.5.4
Russell King - ARM Linux
AppliedPATCH 8/14 mmc: mmci: Fixup error handling for dma
Quoted 6 lines · Ulf Hansson
When getting a cmd irq during an ongoing data transfer with dma, the dma job were never terminated. This is now corrected.
Signed-off-by Ulf Hansson <[email protected]>Signed-off-by Per Forlin <[email protected]>
I've queued this up in my 'fixes' branch with a Cc to the stable team for previous kernels. I believe this to be appropriate. Please confirm.
PATCH 8/14 mmc: mmci: Fixup error handling for dma
Quoted 11 lines · Russell King - ARM Linux
Quoted 6 lines · Ulf Hansson
When getting a cmd irq during an ongoing data transfer with dma, the dma job were never terminated. This is now corrected.
Signed-off-by Ulf Hansson <[email protected]>Signed-off-by Per Forlin <[email protected]>I've queued this up in my 'fixes' branch with a Cc to the stable team for previous kernels. I believe this to be appropriate. Please confirm.
Great!
BR
Ulf Hansson
PATCH 9/14 mmc: mmci: Change from using legacy suspend
In reply to Ulf HanssonThis patch switch from using the legacy suspend/resume to the new way of registering PM callbacks. No functional change is done.
--- drivers/mmc/host/mmci.c | 22 ++++++++++++---------- 1 files changed, 12 insertions(+), 10 deletions(-)
drivers/mmc/host/mmci.c +12 -10
@@ -1455,10 +1455,11 @@ static int __devexit mmci_remove(struct amba_device *dev)return 0 ; } -#ifdef CONFIG_PM -static int mmci_suspend (struct amba_device *dev,pm_message_t state) +#ifdef CONFIG_SUSPEND +static int mmci_suspend (struct device *dev) { -struct mmc_host *mmc =amba_get_drvdata (dev); +struct amba_device *adev =to_amba_device (dev); +struct mmc_host *mmc =amba_get_drvdata (adev);int ret =0 ;if (mmc) {
@@ -1472,9 +1473,10 @@ static int mmci_suspend(struct amba_device *dev, pm_message_t state)return ret; } -static int mmci_resume (struct amba_device *dev) +static int mmci_resume (struct device *dev) { -struct mmc_host *mmc =amba_get_drvdata (dev); +struct amba_device *adev =to_amba_device (dev); +struct mmc_host *mmc =amba_get_drvdata (adev);int ret =0 ;if (mmc) {
@@ -1487,11 +1489,12 @@ static int mmci_resume(struct amba_device *dev)return ret; } -#else -#define mmci_suspend NULL -#define mmci_resume NULL#endif +static const struct dev_pm_ops mmci_dev_pm_ops = { +SET_SYSTEM_SLEEP_PM_OPS (mmci_suspend, mmci_resume) +}; +static struct amba_id mmci_ids[] = { { .id =0x00041180 ,
@@ -1535,11 +1538,10 @@ static struct amba_id mmci_ids[] = {static struct amba_driver mmci_driver = { .drv = { .name =DRIVER_NAME , + .pm = &mmci_dev_pm_ops, }, .probe = mmci_probe, .remove =__devexit_p (mmci_remove), - .suspend = mmci_suspend, - .resume = mmci_resume, .id_table = mmci_ids, };
Signature · 1 line
1.7.5.4
PATCH 10/14 mmc: mmci: Cache MMCICLOCK and MMCIPOWER register
In reply to Ulf HanssonInstead of reading a register value everytime we need to apply a new value for it, maintain a cached copy for it. This also means we are able to skip writes that are not needed.
--- drivers/mmc/host/mmci.c | 41 +++++++++++++++++++++++++++++------------ drivers/mmc/host/mmci.h | 3 ++- 2 files changed, 31 insertions(+), 13 deletions(-)
drivers/mmc/host/mmci.c +29 -12
@@ -121,6 +121,28 @@ static struct variant_data variant_ux500v2 = {/* * This must be called with host->lock held */ +static void mmci_write_clkreg (struct mmci_host *host,u32 clk) +{ +if (host->clk_reg != clk) { + host->clk_reg = clk; +writel (clk, host->base +MMCICLOCK ); + } +} + +/* +* This must be called with host->lock held +*/ +static void mmci_write_pwrreg (struct mmci_host *host,u32 pwr) +{ +if (host->pwr_reg != pwr) { + host->pwr_reg = pwr; +writel (pwr, host->base +MMCIPOWER ); + } +} + +/* +* This must be called with host->lock held +*/ static void mmci_set_clkreg (struct mmci_host *host,unsigned int desired) {struct variant_data *variant = host->variant;
@@ -165,7 +187,7 @@ static void mmci_set_clkreg(struct mmci_host *host, unsigned int desired)if (host->mmc->ios.bus_width ==MMC_BUS_WIDTH_8 ) clk |=MCI_ST_8BIT_BUS ; -writel (clk, host->base +MMCICLOCK ); +mmci_write_clkreg (host, clk); } static void
@@ -824,14 +846,13 @@ static int mmci_pio_write(struct mmci_host *host, char *buffer, unsigned int rem*/ if (variant->sdio &&mmc_card_sdio (host->mmc->card)) { +u32 clk;if (count <8 ) -writel (readl (host->base +MMCICLOCK ) & - ~variant->clkreg_enable, - host->base +MMCICLOCK ); + clk = host->clk_reg & ~variant->clkreg_enable;else -writel (readl (host->base +MMCICLOCK ) | - variant->clkreg_enable, - host->base +MMCICLOCK ); + clk = host->clk_reg | variant->clkreg_enable; + +mmci_write_clkreg (host, clk); }/*
@@ -1090,11 +1111,7 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)spin_lock_irqsave (&host->lock, flags);mmci_set_clkreg (host, ios->clock); - -if (host->pwr != pwr) { - host->pwr = pwr; -writel (pwr, host->base +MMCIPOWER ); - } +mmci_write_pwrreg (host, pwr);spin_unlock_irqrestore (&host->lock, flags); }
drivers/mmc/host/mmci.h +2 -1
@@ -179,7 +179,8 @@ struct mmci_host {unsigned int mclk;unsigned int cclk; -u32 pwr; +u32 pwr_reg; +u32 clk_reg;struct mmci_platform_data *plat;struct variant_data *variant;
Signature · 1 line
1.7.5.4
PATCH 10/14 mmc: mmci: Cache MMCICLOCK and MMCIPOWER register
Quoted 4 lines · Ulf Hansson
Instead of reading a register value everytime we need to apply a new value for it, maintain a cached copy for it. This also means we are able to skip writes that are not needed.
I'm not sure this is a good idea. The ARM Primecells require a certain number of bus clocks and MCLK periods between writes to both these registers, and reading them back helps to ensure that we conform to that requirement. Maintaining a cached copy of them allows faster writes to these registers which could cause that requirement to be violated.
What you could do is read the register, modify, and check whether the modification has had any effect before writing it back. That will allow unnecessary writes to still be skipped.
PATCH 10/14 mmc: mmci: Cache MMCICLOCK and MMCIPOWER register
Quoted 6 lines · Russell King - ARM Linux
Quoted 4 lines · Ulf Hansson
Instead of reading a register value everytime we need to apply a new value for it, maintain a cached copy for it. This also means we are able to skip writes that are not needed.
Quoted 6 lines
I'm not sure this is a good idea. The ARM Primecells require a certain number of bus clocks and MCLK periods between writes to both these registers, and reading them back helps to ensure that we conform to that requirement. Maintaining a cached copy of them allows faster writes to these registers which could cause that requirement to be violated.
You are definitely right. But how do we know that reading the register value is enough?
Quoted 5 lines
What you could do is read the register, modify, and check whether the modification has had any effect before writing it back. That will allow unnecessary writes to still be skipped.
That could work. Do you want me to fixup the patch to include this "read before write" mechanism then?
BR
Ulf Hansson
PATCH 10/14 mmc: mmci: Cache MMCICLOCK and MMCIPOWER register
In reply to Russell King - ARM LinuxHi again,
Quoted 6 lines · Russell King - ARM Linux
Quoted 4 lines · Ulf Hansson
Instead of reading a register value everytime we need to apply a new value for it, maintain a cached copy for it. This also means we are able to skip writes that are not needed.
Quoted 6 lines
I'm not sure this is a good idea. The ARM Primecells require a certain number of bus clocks and MCLK periods between writes to both these registers, and reading them back helps to ensure that we conform to that requirement. Maintaining a cached copy of them allows faster writes to these registers which could cause that requirement to be violated.
I were just about to update my patch according to your proposal, when I realized the only place were the registers were previously "read before write", were at an SDIO corner case in pio_write.
Earlier the register values were always written, without considering the old value. Thus the impact with this patch is kind of only decreasing the number of writes and affects a corner case for SDIO.
Do you anyway prefer to add a register "read before write" even if it never has been needed before?
Quoted 5 lines
What you could do is read the register, modify, and check whether the modification has had any effect before writing it back. That will allow unnecessary writes to still be skipped.
Sorry for spamming you.
BR
Ulf Hansson
PATCH 11/14 mmc: mmci: Fixup use of runtime PM and use autosuspend
In reply to Ulf HanssonAdded use of runtime PM autosuspend feature, with a fixed timeout of 50 ms. This will prevent adding a latency, although very minor, for _every_ request.
Moreover the runtime_get_sync is now also used in set_ios and suspend since the runtime resourses are needed here as well.
--- drivers/mmc/host/mmci.c | 19 ++++++++++++++++--- 1 files changed, 16 insertions(+), 3 deletions(-)
drivers/mmc/host/mmci.c +16 -3
@@ -200,8 +200,10 @@ mmci_request_end(struct mmci_host *host, struct mmc_request *mrq) host->mrq =NULL ; host->cmd =NULL ; -pm_runtime_put (mmc_dev (host->mmc));mmc_request_done (host->mmc, mrq); + +pm_runtime_mark_last_busy (mmc_dev (host->mmc)); +pm_runtime_put_autosuspend (mmc_dev (host->mmc)); }static void mmci_set_mask1 (struct mmci_host *host,unsigned int mask)
@@ -1044,6 +1046,8 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)unsigned long flags;int ret; +pm_runtime_get_sync (mmc_dev (mmc)); +if (host->plat->ios_handler && host->plat->ios_handler (mmc_dev (mmc), ios))dev_err (mmc_dev (mmc),"platform ios_handler failed\n" );
@@ -1064,7 +1068,7 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)* power should be rare so we print an error * and return here. */ -return ; + goto out; } }/*
@@ -1114,6 +1118,10 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)mmci_write_pwrreg (host, pwr);spin_unlock_irqrestore (&host->lock, flags); + + out: +pm_runtime_mark_last_busy (mmc_dev (mmc)); +pm_runtime_put_autosuspend (mmc_dev (mmc)); }static int mmci_get_ro (struct mmc_host *mmc)
@@ -1388,6 +1396,8 @@ static int __devinit mmci_probe(struct amba_device *dev,mmci_dma_setup (host); +pm_runtime_set_autosuspend_delay (&dev->dev,50 ); +pm_runtime_use_autosuspend (&dev->dev);pm_runtime_put (&dev->dev);mmc_add_host (mmc);
@@ -1483,8 +1493,10 @@ static int mmci_suspend(struct device *dev)struct mmci_host *host =mmc_priv (mmc); ret =mmc_suspend_host (mmc); -if (ret ==0 ) +if (ret ==0 ) { +pm_runtime_get_sync (dev);writel (0 , host->base +MMCIMASK0 ); + } }return ret;
@@ -1500,6 +1512,7 @@ static int mmci_resume(struct device *dev)struct mmci_host *host =mmc_priv (mmc);writel (MCI_IRQENABLE , host->base +MMCIMASK0 ); +pm_runtime_put (dev); ret =mmc_resume_host (mmc); }
Signature · 1 line
1.7.5.4
PATCH 12/14 mmc: mmci: Decrease current consumption in suspend
In reply to Ulf HanssonTo decrease current consumption in suspend state the VCORE regulator, the MCLK and PCLK for the ARM PL18x are now disabled.
When resuming the resourses are re-enabled and register values for MMCICLOCK, MMCIPOWER and MMCIMASK0 are restored.
--- drivers/mmc/host/mmci.c | 64 ++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 58 insertions(+), 6 deletions(-)
drivers/mmc/host/mmci.c +58 -6
@@ -1483,6 +1483,60 @@ static int __devexit mmci_remove(struct amba_device *dev) }#ifdef CONFIG_SUSPEND +static int mmci_save (struct amba_device *dev) +{ +struct mmc_host *mmc =amba_get_drvdata (dev); +unsigned long flags; + +if (mmc) { +struct mmci_host *host =mmc_priv (mmc); + +spin_lock_irqsave (&host->lock, flags); + +/* +* Make sure we do not get any interrupts when we disabled the +* clock and the regulator and as well make sure to clear the +* registers for clock and power. +*/ +writel (0 , host->base +MMCIMASK0 ); +writel (0 , host->base +MMCIPOWER ); +writel (0 , host->base +MMCICLOCK ); + +spin_unlock_irqrestore (&host->lock, flags); + +clk_unprepare (host->clk); +clk_disable (host->clk); +amba_vcore_disable (dev); + } + +return 0 ; +} + +static int mmci_restore (struct amba_device *dev) +{ +struct mmc_host *mmc =amba_get_drvdata (dev); +unsigned long flags; + +if (mmc) { +struct mmci_host *host =mmc_priv (mmc); + +amba_vcore_enable (dev); +clk_prepare (host->clk); +clk_enable (host->clk); + +spin_lock_irqsave (&host->lock, flags); + +/* Restore registers and re-enable interrupts. */ +writel (host->clk_reg, host->base +MMCICLOCK ); +writel (host->pwr_reg, host->base +MMCIPOWER ); +writel (MCI_IRQENABLE , host->base +MMCIMASK0 ); + +spin_unlock_irqrestore (&host->lock, flags); + } + +return 0 ; +} +static int mmci_suspend (struct device *dev) {struct amba_device *adev =to_amba_device (dev);
@@ -1490,12 +1544,11 @@ static int mmci_suspend(struct device *dev)int ret =0 ;if (mmc) { -struct mmci_host *host =mmc_priv (mmc); - ret =mmc_suspend_host (mmc);if (ret ==0 ) {pm_runtime_get_sync (dev); -writel (0 , host->base +MMCIMASK0 ); +mmci_save (adev); +amba_pclk_disable (adev); } }
@@ -1509,9 +1562,8 @@ static int mmci_resume(struct device *dev)int ret =0 ;if (mmc) { -struct mmci_host *host =mmc_priv (mmc); - -writel (MCI_IRQENABLE , host->base +MMCIMASK0 ); +amba_pclk_enable (adev); +mmci_restore (adev);pm_runtime_put (dev); ret =mmc_resume_host (mmc);
Signature · 1 line
1.7.5.4
PATCH 12/14 mmc: mmci: Decrease current consumption in suspend
Quoted 7 lines · Ulf Hansson
To decrease current consumption in suspend state the VCORE regulator, the MCLK and PCLK for the ARM PL18x are now disabled.
When resuming the resourses are re-enabled and register values for MMCICLOCK, MMCIPOWER and MMCIMASK0 are restored.
I still do not agree with this. The MMC core driver should be shutting down the interface (turning off the MMC clock, disabling the MMC power). And indeed it does:
int mmc_suspend_host(struct mmc_host *host)
{
...
if (!err && !mmc_card_keep_power(host))
mmc_power_off(host);
}void mmc_power_off(struct mmc_host *host)
{
...
host->ios.power_mode = MMC_POWER_OFF;
host->ios.bus_width = MMC_BUS_WIDTH_1;
host->ios.timing = MMC_TIMING_LEGACY;
mmc_set_ios(host);...
}
On ARM primecells we can _not_ go from power off to power on without first going through the power up state, so this simple save-and-restore will not do - we have to go through the proper power up sequence.
Why do you think that the MMC core is not shutting down the power and clock registers on suspend?
PATCH 12/14 mmc: mmci: Decrease current consumption in suspend
Quoted 13 lines · Russell King - ARM Linux
Quoted 7 lines · Ulf Hansson
To decrease current consumption in suspend state the VCORE regulator, the MCLK and PCLK for the ARM PL18x are now disabled.
When resuming the resourses are re-enabled and register values for MMCICLOCK, MMCIPOWER and MMCIMASK0 are restored.
I still do not agree with this. The MMC core driver should be shutting down the interface (turning off the MMC clock, disabling the MMC power). And indeed it does:
Quoted 7 lines
int
mmc_suspend_host(struct mmc_host *host)
{
...if (!err && !mmc_card_keep_power(host)) mmc_power_off(host); }
If mmc_power_off gets called, this will mean cutting the power to the card. Either through the MMCIPOWER register(ARM rimecells) or by using the host->vcc regulator (ST-versions). Similar actions is done for MMCICLOCK register, which in the end means the clock to the card gets gated.
The MCLK (host->clk), the PCLK (amba_pclk) and the regulator (amba_vcore) is not disabled. This should only be handled during probe, suspend/resume and remove I believe.
Quoted 7 lines
void
mmc_power_off(struct mmc_host *host)
{
...host->ios.power_mode = MMC_POWER_OFF; host->ios.bus_width = MMC_BUS_WIDTH_1; host->ios.timing = MMC_TIMING_LEGACY; mmc_set_ios(host);
Quoted 6 lines
...
}On ARM primecells we can _not_ go from power off to power on without first going through the power up state, so this simple save-and-restore will not do - we have to go through the proper power up sequence.
This will be handled when doing mmc_resume_host so there should be no problems for ARM Primecells. It might be the naming of these functions that kind of confuses you at this state. For runtime PM, these names (mmci_save and mmci_restore) make more sence. :-) Right now they are kind of only used for "enable"/"disable" clocks and regulators.
I can change the patch to only do the enable/disable part if you like that better at this stage. But since the earlier version of suspend function reset the MMCIMASK0 to zero, I thought reseting the other registers as well would be okay at this stage.
If you review the patch "mmci: Implement PM runtime callbacks to save power" together with this one, it should hopefully bring you more clarity how I have thought around this hole thing.
Quoted 4 lines
Why do you think that the MMC core is not shutting down the power and clock registers on suspend?
Br
Ulf Hansson
PATCH 13/14 mmc: mmci: Implement PM runtime callbacks to save power
In reply to Ulf HanssonIn the runtime_suspend callback we make use of mmci_save to disable the VCORE regulator and the MCLK to decrease current consumption. At runtime resume, we use mmci_restore to re-enable the resourses again.
Quoted 1 line
From now on this will mean that especially the
mmci_restore
function must be fast to execute since otherwise request latency will be introduced.
For the ARM Primcell PL180, the MMCIPOWER register is used to control an external power supply to the card. Thus we need to prevent the runtime callbacks from doing save and restore, otherwise the power to card will be cut. This is done by adding a new flag to the variant data.
--- drivers/mmc/host/mmci.c | 43 ++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 42 insertions(+), 1 deletions(-)
drivers/mmc/host/mmci.c +42 -1
@@ -55,6 +55,7 @@ static unsigned int fmax = 515633;* @blksz_datactrl16: true if Block size is at b16..b30 position in datactrl register * @pwrreg_powerup: power up value for MMCIPOWER register * @signal_direction: input/out direction of bus signals can be indicated +* @pwrreg_ctrl_power: bits in MMCIPOWER register controls ext. power supply */ struct variant_data {unsigned int clkreg;
@@ -67,6 +68,7 @@ struct variant_data {bool blksz_datactrl16;u32 pwrreg_powerup;bool signal_direction; +bool pwrreg_ctrl_power; };static struct variant_data variant_arm = {
@@ -74,6 +76,7 @@ static struct variant_data variant_arm = { .fifohalfsize = 8 * 4, .datalength_bits = 16, .pwrreg_powerup = MCI_PWR_UP, + .pwrreg_ctrl_power = true, };static struct variant_data variant_arm_extended_fifo = {
@@ -81,6 +84,7 @@ static struct variant_data variant_arm_extended_fifo = { .fifohalfsize = 64 * 4, .datalength_bits = 16, .pwrreg_powerup = MCI_PWR_UP, + .pwrreg_ctrl_power = true, };static struct variant_data variant_u300 = {
@@ -1482,7 +1486,7 @@ static int __devexit mmci_remove(struct amba_device *dev)return 0 ; } -#ifdef CONFIG_SUSPEND +#if defined(CONFIG_SUSPEND ) || defined(CONFIG_PM_RUNTIME )static int mmci_save (struct amba_device *dev) {struct mmc_host *mmc =amba_get_drvdata (dev);
@@ -1536,7 +1540,9 @@ static int mmci_restore(struct amba_device *dev)return 0 ; } +#endif +#ifdef CONFIG_SUSPEND static int mmci_suspend (struct device *dev) {struct amba_device *adev =to_amba_device (dev);
@@ -1573,8 +1579,43 @@ static int mmci_resume(struct device *dev) }#endif +#ifdef CONFIG_PM_RUNTIME +static int mmci_runtime_suspend (struct device *dev) +{ +struct amba_device *adev =to_amba_device (dev); +struct mmc_host *mmc =amba_get_drvdata (adev); +int ret =0 ; + +if (mmc) { +struct mmci_host *host =mmc_priv (mmc); +struct variant_data *variant = host->variant; +if (!variant->pwrreg_ctrl_power) + ret =mmci_save (adev); + } + +return ret; +} + +static int mmci_runtime_resume (struct device *dev) +{ +struct amba_device *adev =to_amba_device (dev); +struct mmc_host *mmc =amba_get_drvdata (adev); +int ret =0 ; + +if (mmc) { +struct mmci_host *host =mmc_priv (mmc); +struct variant_data *variant = host->variant; +if (!variant->pwrreg_ctrl_power) + ret =mmci_restore (adev); + } + +return ret; +} +#endif +static const struct dev_pm_ops mmci_dev_pm_ops = { SET_SYSTEM_SLEEP_PM_OPS(mmci_suspend, mmci_resume) +SET_RUNTIME_PM_OPS (mmci_runtime_suspend, mmci_runtime_resume,NULL ) };static struct amba_id mmci_ids[] = {
Signature · 1 line
1.7.5.4
PATCH 14/14 mmc: mmci: Use ios_handler to save power
In reply to Ulf HanssonTo disable a levelshifter when we are in an idle state will decrease current consumption. We make use of the ios_handler at runtime suspend and at regular suspend to accomplish this.
Of course depending on the used levelshifter the decrease of current differs. For ST6G3244ME the value is up to ~200 uA.
--- drivers/mmc/host/mmci.c | 19 ++++++++++++++++++- 1 files changed, 18 insertions(+), 1 deletions(-)
drivers/mmc/host/mmci.c +18 -1
@@ -1491,10 +1491,22 @@ static int mmci_save(struct amba_device *dev) {struct mmc_host *mmc =amba_get_drvdata (dev);unsigned long flags; +struct mmc_ios ios; +int ret =0 ;if (mmc) {struct mmci_host *host =mmc_priv (mmc); +/* Let the ios_handler act on a POWER_OFF to save power. */ +if (host->plat->ios_handler) { +memcpy (&ios, &mmc->ios,sizeof (struct mmc_ios )); + ios.power_mode =MMC_POWER_OFF ; + ret = host->plat->ios_handler (mmc_dev (mmc), + &ios); +if (ret) +return ret; + } +spin_lock_irqsave (&host->lock, flags);/*
@@ -1513,7 +1525,7 @@ static int mmci_save(struct amba_device *dev)amba_vcore_disable (dev); } -return 0 ; +return ret; }static int mmci_restore (struct amba_device *dev)
@@ -1536,6 +1548,11 @@ static int mmci_restore(struct amba_device *dev)writel (MCI_IRQENABLE , host->base +MMCIMASK0 );spin_unlock_irqrestore (&host->lock, flags); + +/* Restore settings done by the ios_handler. */ +if (host->plat->ios_handler) + host->plat->ios_handler (mmc_dev (mmc), + &mmc->ios); }return 0 ;
Signature · 1 line
1.7.5.4
cover mmc: mmci: Improved PM support, cleanup and bugfixes
In reply to Ulf HanssonHi Ulf,
Quoted 10 lines · Ulf Hansson
Patch 1 -> 4:
Minor changes for bugs and some performance optimizations. Previously sent to the mailing list.
Patch 5 -> 7:
Minor changes affecting how to handle a levelshifter. Previously sent to the mailing list.
Patch 8:
Bugfix for dma. Previously sent to the mailing list, ongoing discussion. Patch 9 -> 14:
Improvement of PM support.
I applied all 14 patches on top of current v3.2-rc4 and regression-tested the whole shebang on RealView PB1176 and U300.
- Mounted /dev/mmcblk0p1
- Copy a kernel-sized uImage file from the card to ramdisk
- Copy the file back to the card from ramdisk
- Unmount
- Mount
- Compare the two files with diff (identical)
- Delete copy
- Unmount
- dd if=/dev/mmcblk0 of=/dev/null
(If any of you wants any more specific tests, tell me.)
Both platforms still work like a charm with these patches. PB1176 is as slow on MMC as it always has been but dd:ed the entire card without errors.
I have the branch here:git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-stericsson.git mmci
Ulf you can add my Tested-by: Linus Walleij <[email protected]> for these patches.
Russell if you want a pull request for a subset of the patches I can arrange that, just tell me.
Yours,
Linus Walleij
cover mmc: mmci: Improved PM support, cleanup and bugfixes
Quoted 24 lines · Linus Walleij
I applied all 14 patches on top of current v3.2-rc4 and regression-tested the whole shebang on RealView PB1176 and U300.
- Mounted
/dev/mmcblk0p1
- Copy a kernel-sized uImage file from the card to ramdisk
- Copy the file back to the card from ramdisk
- Unmount
- Mount
- Compare the two files with diff (identical)
- Delete copy
- Unmount
- dd if=/dev/mmcblk0 of=/dev/null(If any of you wants any more specific tests, tell me.)
Both platforms still work like a charm with these patches. PB1176 is as slow on MMC as it always has been but dd:ed the entire card without errors.
I have the branch here:
git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-stericsson.gitmmci
Quoted 9 lines
Ulf you can add my Tested-by: Linus Walleij <[email protected]> for these patches.
Russell if you want a pull request for a subset of the patches I can arrange that, just tell me.
Yours,
Linus Walleij
I just uploaded these patches into Russell's patchtracker tool. All patches has now been rebased for Linux 3.2-rc4, which you were testing with Linus. Moreover, I added "Tested-by: Linus Walleij <[email protected]>".
Br
Ulf Hansson