Re: [PATCH] mmc: add dependence in Kconfig for pxav2/pxav3
On Monday 20 June 2011, zhangfei gao wrote: Currently, clk_enable, clk_disable, clk_get_rate realization are located at arch. Driver/clk/clkdev.c only realize clk_get/put. I am afraid it may difficult to build the driver on other arch, like x86, so we may still need depends on ARCH_MMP for safety. I would recommend turning this into depends on CLKDEV_LOOKUP instead. Always be specific with your dependencies so you enable the option on all platforms that are able to build it, but not on any of the others. Arnd -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: add dependence in Kconfig for pxav2/pxav3
On Monday 20 June 2011, zhangfei gao wrote: It is really a good suggestion, is include/linux/platform_data/ is newly added? Will move the header file to platform_data/pxa_sdhci.h arch/arm/plat-pxa/include/plat/sdhci.h = include/linux/platform_data/pxa_sdhci.h Yes, it is fairly new. The move looks correct to me. Arnd -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: add dependence in Kconfig for pxav2/pxav3
On Monday 20 June 2011, zhangfei gao wrote: Select CLKDEV_LOOKUP is compile driver/clk/clkdev.c, which only provide clk_get, clk_put. Is it unsafe to assume all clk_get_rate/clk_enable/clk_disable are provided by the arch, which CLKDEV_LOOKUP? Any possibility CLKDEV_LOOKUP is selected but clk_get_rate is not realized. I would consider that a bug, so you can ignore that case. Arnd -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] mmc: sdhi: Add write16_hook
On Mon, Jun 20, 2011 at 08:29:18AM +0200, Guennadi Liakhovetski wrote: On Mon, 20 Jun 2011, Simon Horman wrote: Some controllers require waiting for the bus to become idle before writing to some registers. I have implemented this by adding a hook to sd_ctrl_write16() and implementing a hook for SDHI which waits for the bus to become idle. Cc: Guennadi Liakhovetski g.liakhovet...@gmx.de Cc: Magnus Damm magnus.d...@gmail.com Signed-off-by: Simon Horman ho...@verge.net.au --- Dependenvies: mmc: tmio: Share register access functions --- drivers/mmc/host/sh_mobile_sdhi.c | 34 ++ drivers/mmc/host/tmio_mmc.h |2 ++ include/linux/mfd/tmio.h |8 include/linux/mmc/tmio.h |1 + 4 files changed, 45 insertions(+), 0 deletions(-) diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c index b365429..6c56453 100644 --- a/drivers/mmc/host/sh_mobile_sdhi.c +++ b/drivers/mmc/host/sh_mobile_sdhi.c @@ -27,6 +27,8 @@ #include linux/mfd/tmio.h #include linux/sh_dma.h +#include asm/delay.h linux/delay.h Thanks. + #include tmio_mmc.h struct sh_mobile_sdhi { @@ -55,6 +57,37 @@ static int sh_mobile_sdhi_get_cd(struct platform_device *pdev) return -ENOSYS; } +static void sh_mobile_sdhi_wait_idle(struct tmio_mmc_host *host) +{ + int timeout = 1000; + + while (--timeout !(sd_ctrl_read16(host, CTL_STATUS2) (1 13))) + udelay(1); + + if (!timeout) + dev_warn(host-pdata-dev, timeout waiting for SD bus idle\n); + +} + +static void sh_mobile_sdhi_write16_hook(struct tmio_mmc_host *host, int addr) +{ + if (!(host-pdata-flags TMIO_MMC_HAS_IDLE_WAIT)) + return; + + switch (addr) + { + case CTL_SD_CMD: + case CTL_STOP_INTERNAL_ACTION: + case CTL_XFER_BLK_COUNT: + case CTL_SD_CARD_CLK_CTL: + case CTL_SD_XFER_LEN: + case CTL_SD_MEM_CARD_OPT: + case CTL_TRANSACTION_CTL: + case CTL_DMA_ENABLE: + sh_mobile_sdhi_wait_idle(host); + } +} + static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev) { struct sh_mobile_sdhi *priv; @@ -86,6 +119,7 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev) mmc_data-hclk = clk_get_rate(priv-clk); mmc_data-set_pwr = sh_mobile_sdhi_set_pwr; mmc_data-get_cd = sh_mobile_sdhi_get_cd; + mmc_data-write16_hook = sh_mobile_sdhi_write16_hook; Not sure I like the write16_hook name. You consider this callback as something the host might need to do, when writing to a 16-bit register. In this specific case it is actually waiting for the bus to become idle, which might also be needed in other locations. So, maybe it is better to call this callback wait_idle or something similar? Or you think, other hosts might need a write16 hook doing something different?... I'm not strongly attached to the name, and I do see your point. But as it stands the hook is currently only needed on 16bit register writes; the hook is called from sd_ctrl_write16(): and sh_mobile_sdhi_write16_hook() works by looking at addr, which is specific to register reads and writes. So while it isn't great, I think the current name isn't entirely horrible. I think its a little hard to look into a crystal ball and see where other TMIO hardware might need wait_idle(). Who knows what we might need to do for the next batch of hardware? :-) I had considered other approaches to this problem, such as calling something_wait_idle() from within tmio_mmc_*.c as needed. But the approach in the patch I posted turned out to be quite a lot cleaner and have the advantage of concisely documenting what is being done - that is, writes to which registers need wait_idle treatment. The down side is that the callback is obviously more specific than perhaps a hook ought to be. But I think that it can evolve as the need arises. -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] mmc: sdhi: Add write16_hook
On Mon, Jun 20, 2011 at 04:04:04PM +0900, Magnus Damm wrote: On Mon, Jun 20, 2011 at 3:06 PM, Simon Horman ho...@verge.net.au wrote: Some controllers require waiting for the bus to become idle before writing to some registers. I have implemented this by adding a hook to sd_ctrl_write16() and implementing a hook for SDHI which waits for the bus to become idle. Cc: Guennadi Liakhovetski g.liakhovet...@gmx.de Cc: Magnus Damm magnus.d...@gmail.com Signed-off-by: Simon Horman ho...@verge.net.au --- Hi Simon, Thanks for your work on this! --- a/drivers/mmc/host/sh_mobile_sdhi.c +++ b/drivers/mmc/host/sh_mobile_sdhi.c @@ -55,6 +57,37 @@ static int sh_mobile_sdhi_get_cd(struct platform_device *pdev) return -ENOSYS; } +static void sh_mobile_sdhi_wait_idle(struct tmio_mmc_host *host) +{ + int timeout = 1000; + + while (--timeout !(sd_ctrl_read16(host, CTL_STATUS2) (1 13))) + udelay(1); + + if (!timeout) + dev_warn(host-pdata-dev, timeout waiting for SD bus idle\n); + +} + +static void sh_mobile_sdhi_write16_hook(struct tmio_mmc_host *host, int addr) +{ + if (!(host-pdata-flags TMIO_MMC_HAS_IDLE_WAIT)) + return; and @@ -86,6 +119,7 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev) mmc_data-hclk = clk_get_rate(priv-clk); mmc_data-set_pwr = sh_mobile_sdhi_set_pwr; mmc_data-get_cd = sh_mobile_sdhi_get_cd; + mmc_data-write16_hook = sh_mobile_sdhi_write16_hook; mmc_data-capabilities = MMC_CAP_MMC_HIGHSPEED; if (p) { mmc_data-flags = p-tmio_flags; Doesn't it make more sense to do: if (!(host-pdata-flags TMIO_MMC_HAS_IDLE_WAIT)) mmc_data-write16_hook = sh_mobile_sdhi_write16_hook; and skip the check inside the sh_mobile_sdhi_write16_hook() function? No need to do that check on every register access unless really needed. Sure, good idea. -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] mmc: sdhi: Add write16_hook
On Mon, Jun 20, 2011 at 03:25:22PM +0900, Paul Mundt wrote: On Mon, Jun 20, 2011 at 03:06:53PM +0900, Simon Horman wrote: diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c index b365429..6c56453 100644 --- a/drivers/mmc/host/sh_mobile_sdhi.c +++ b/drivers/mmc/host/sh_mobile_sdhi.c @@ -27,6 +27,8 @@ #include linux/mfd/tmio.h #include linux/sh_dma.h +#include asm/delay.h + #include tmio_mmc.h linux/delay.h will suffice. Thanks. +static void sh_mobile_sdhi_wait_idle(struct tmio_mmc_host *host) +{ + int timeout = 1000; + + while (--timeout !(sd_ctrl_read16(host, CTL_STATUS2) (1 13))) + udelay(1); + + if (!timeout) + dev_warn(host-pdata-dev, timeout waiting for SD bus idle\n); + +} + +static void sh_mobile_sdhi_write16_hook(struct tmio_mmc_host *host, int addr) +{ + if (!(host-pdata-flags TMIO_MMC_HAS_IDLE_WAIT)) + return; + + switch (addr) + { + case CTL_SD_CMD: + case CTL_STOP_INTERNAL_ACTION: + case CTL_XFER_BLK_COUNT: + case CTL_SD_CARD_CLK_CTL: + case CTL_SD_XFER_LEN: + case CTL_SD_MEM_CARD_OPT: + case CTL_TRANSACTION_CTL: + case CTL_DMA_ENABLE: + sh_mobile_sdhi_wait_idle(host); + } +} + The timeout error should really be handed down. diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h index 0c22df0..5912cf3 100644 --- a/drivers/mmc/host/tmio_mmc.h +++ b/drivers/mmc/host/tmio_mmc.h @@ -153,6 +153,8 @@ static inline u32 sd_ctrl_read32(struct tmio_mmc_host *host, int addr) static inline void sd_ctrl_write16(struct tmio_mmc_host *host, int addr, u16 val) { + if (host-pdata-write16_hook) + host-pdata-write16_hook(host, addr); writew(val, host-ctl + (addr host-bus_shift)); } If it times out you are unlikely to want to proceed with the write. At best you end up with a lost write cycle, at worst potentially a bus lockup you can't easily recover from. As discussed off-line, I'll change this around so that write16_hook() may return an error and the write will be skipped if that occurs. -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] mmc: sdhci-pxa move platform data to include/linux/platform_data
As suggested by Arnd, move platform data to include/linux/platform_data, Add 'depends on CLKDEV_LOOKUP' since clk API is used, As a result driver could be built on all platforms which support CLKDEV_LOOKUP This can improve the build test coverage Signed-off-by: Zhangfei Gao zhangfei@marvell.com CC: Arnd Bergmann a...@arndb.de --- arch/arm/mach-mmp/include/mach/mmp2.h |2 +- drivers/mmc/host/Kconfig |2 ++ drivers/mmc/host/sdhci-pxav2.c |2 +- drivers/mmc/host/sdhci-pxav3.c |2 +- .../linux/platform_data/pxa_sdhci.h| 10 +- 5 files changed, 10 insertions(+), 8 deletions(-) rename arch/arm/plat-pxa/include/plat/sdhci.h = include/linux/platform_data/pxa_sdhci.h (92%) diff --git a/arch/arm/mach-mmp/include/mach/mmp2.h b/arch/arm/mach-mmp/include/mach/mmp2.h index 2cbf6df..de7b888 100644 --- a/arch/arm/mach-mmp/include/mach/mmp2.h +++ b/arch/arm/mach-mmp/include/mach/mmp2.h @@ -1,7 +1,7 @@ #ifndef __ASM_MACH_MMP2_H #define __ASM_MACH_MMP2_H -#include plat/sdhci.h +#include linux/platform_data/pxa_sdhci.h struct sys_timer; diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig index 198ddda..204ad7c 100644 --- a/drivers/mmc/host/Kconfig +++ b/drivers/mmc/host/Kconfig @@ -183,6 +183,7 @@ config MMC_SDHCI_S3C config MMC_SDHCI_PXAV3 tristate Marvell MMP2 SD Host Controller support (PXAV3) + depends on CLKDEV_LOOKUP select MMC_SDHCI select MMC_SDHCI_PLTFM default CPU_MMP2 @@ -195,6 +196,7 @@ config MMC_SDHCI_PXAV3 config MMC_SDHCI_PXAV2 tristate Marvell PXA9XX SD Host Controller support (PXAV2) + depends on CLKDEV_LOOKUP select MMC_SDHCI select MMC_SDHCI_PLTFM default CPU_PXA910 diff --git a/drivers/mmc/host/sdhci-pxav2.c b/drivers/mmc/host/sdhci-pxav2.c index 7a6fa8c..38f5899 100644 --- a/drivers/mmc/host/sdhci-pxav2.c +++ b/drivers/mmc/host/sdhci-pxav2.c @@ -25,7 +25,7 @@ #include linux/gpio.h #include linux/mmc/card.h #include linux/mmc/host.h -#include plat/sdhci.h +#include linux/platform_data/pxa_sdhci.h #include linux/slab.h #include sdhci.h #include sdhci-pltfm.h diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c index 901f00f..4198dbb 100644 --- a/drivers/mmc/host/sdhci-pxav3.c +++ b/drivers/mmc/host/sdhci-pxav3.c @@ -24,7 +24,7 @@ #include linux/gpio.h #include linux/mmc/card.h #include linux/mmc/host.h -#include plat/sdhci.h +#include linux/platform_data/pxa_sdhci.h #include linux/slab.h #include linux/delay.h #include sdhci.h diff --git a/arch/arm/plat-pxa/include/plat/sdhci.h b/include/linux/platform_data/pxa_sdhci.h similarity index 92% rename from arch/arm/plat-pxa/include/plat/sdhci.h rename to include/linux/platform_data/pxa_sdhci.h index 800ebc1..51ad099 100644 --- a/arch/arm/plat-pxa/include/plat/sdhci.h +++ b/include/linux/platform_data/pxa_sdhci.h @@ -1,4 +1,5 @@ -/* linux/arch/arm/plat-pxa/include/plat/sdhci.h +/* + * include/linux/platform_data/pxa_sdhci.h * * Copyright 2010 Marvell * Zhangfei Gao zhangfei@marvell.com @@ -10,8 +11,8 @@ * published by the Free Software Foundation. */ -#ifndef __PLAT_PXA_SDHCI_H -#define __PLAT_PXA_SDHCI_H +#ifndef _PXA_SDHCI_H_ +#define _PXA_SDHCI_H_ /* pxa specific flag */ /* Require clock free running */ @@ -56,5 +57,4 @@ struct sdhci_pxa { u8 clk_enable; u8 power_mode; }; - -#endif /* __PLAT_PXA_SDHCI_H */ +#endif /* _PXA_SDHCI_H_ */ -- 1.7.0.4 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] mmc: tmio: fix regression: restore TMIO_MMC_WRPROTECT_DISABLE handling
Commit b6147490e6aac82fa2f4b9d7fce925d9891ebe8f broke handling of the TMIO_MMC_WRPROTECT_DISABLE flag by the tmio-mmc driver. This patch restores the original behaviour. Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de --- Chris, please push for 3.0 Thanks Guennadi drivers/mmc/host/tmio_mmc_pio.c |4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c index ad6347b..0b09e82 100644 --- a/drivers/mmc/host/tmio_mmc_pio.c +++ b/drivers/mmc/host/tmio_mmc_pio.c @@ -824,8 +824,8 @@ static int tmio_mmc_get_ro(struct mmc_host *mmc) struct tmio_mmc_host *host = mmc_priv(mmc); struct tmio_mmc_data *pdata = host-pdata; - return ((pdata-flags TMIO_MMC_WRPROTECT_DISABLE) || - !(sd_ctrl_read32(host, CTL_STATUS) TMIO_STAT_WRPROTECT)); + return !((pdata-flags TMIO_MMC_WRPROTECT_DISABLE) || +(sd_ctrl_read32(host, CTL_STATUS) TMIO_STAT_WRPROTECT)); } static int tmio_mmc_get_cd(struct mmc_host *mmc) -- 1.7.2.5 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] mmc: SDHI: DMA slave ID 0 is invalid
Hi Chris Can we please get this patch too in for 3.0? It is a part of a patch-series, of which all others havs already been applied. This specific patch makes the behaviour of the SDHI driver consistent with other DMA users on sh-mobile, namely, that default platform data with no DMA information provided immediately switches the driver to the PIO mode. This patch fixes this logical error of trying to acquire DMA channels with invalid configuration information and avoids ugly error messages in the kernel log. Thanks Guennadi On Tue, 24 May 2011, Guennadi Liakhovetski wrote: Don't try to allocate DMA resources, if the platform didn't specify positive DMA slave IDs. Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de --- drivers/mmc/host/sh_mobile_sdhi.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c index c05d699..e646741 100644 --- a/drivers/mmc/host/sh_mobile_sdhi.c +++ b/drivers/mmc/host/sh_mobile_sdhi.c @@ -132,7 +132,7 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev) mmc_data-ocr_mask = p-tmio_ocr_mask; mmc_data-capabilities |= p-tmio_caps; - if (p-dma_slave_tx = 0 p-dma_slave_rx = 0) { + if (p-dma_slave_tx 0 p-dma_slave_rx 0) { priv-param_tx.slave_id = p-dma_slave_tx; priv-param_rx.slave_id = p-dma_slave_rx; priv-dma_priv.chan_priv_tx = priv-param_tx; -- 1.7.2.5 --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 11/11] mmc: add handling for two parallel block requests in issue_rw_rq
On Mon, Jun 20, 2011 at 2:47 AM, Per Forlin per.for...@linaro.org wrote: Change mmc_blk_issue_rw_rq() to become asynchronous. The execution flow looks like this: The mmc-queue calls issue_rw_rq(), which sends the request to the host and returns back to the mmc-queue. The mmc-queue calls issue_rw_rq() again with a new request. This new request is prepared, in isuue_rw_rq(), then it waits for the active request to complete before pushing it to the host. When to mmc-queue is empty it will call isuue_rw_rq() with req=NULL to finish off the active request without starting a new request. Signed-off-by: Per Forlin per.for...@linaro.org --- drivers/mmc/card/block.c | 121 +- drivers/mmc/card/queue.c | 17 +-- drivers/mmc/card/queue.h | 1 + 3 files changed, 101 insertions(+), 38 deletions(-) diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c index 6a84a75..66db77a 100644 --- a/drivers/mmc/card/block.c +++ b/drivers/mmc/card/block.c @@ -108,6 +108,7 @@ static DEFINE_MUTEX(open_lock); enum mmc_blk_status { MMC_BLK_SUCCESS = 0, + MMC_BLK_PARTIAL, MMC_BLK_RETRY, MMC_BLK_DATA_ERR, MMC_BLK_CMD_ERR, @@ -668,14 +669,16 @@ static inline void mmc_apply_rel_rw(struct mmc_blk_request *brq, } } -static enum mmc_blk_status mmc_blk_err_check(struct mmc_blk_request *brq, - struct request *req, - struct mmc_card *card, - struct mmc_blk_data *md) +static int mmc_blk_err_check(struct mmc_card *card, + struct mmc_async_req *areq) { struct mmc_command cmd; u32 status = 0; enum mmc_blk_status ret = MMC_BLK_SUCCESS; + struct mmc_queue_req *mq_mrq = container_of(areq, struct mmc_queue_req, + mmc_active); + struct mmc_blk_request *brq = mq_mrq-brq; + struct request *req = mq_mrq-req; /* * Check for errors here, but don't jump to cmd_err @@ -770,7 +773,11 @@ static enum mmc_blk_status mmc_blk_err_check(struct mmc_blk_request *brq, else ret = MMC_BLK_DATA_ERR; } -out: + + if (ret == MMC_BLK_SUCCESS + blk_rq_bytes(req) != brq-data.bytes_xfered) + ret = MMC_BLK_PARTIAL; + out: return ret; } @@ -901,27 +908,59 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq, brq-data.sg_len = i; } + mqrq-mmc_active.mrq = brq-mrq; + mqrq-mmc_active.err_check = mmc_blk_err_check; + mmc_queue_bounce_pre(mqrq); } -static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *req) +static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc) { struct mmc_blk_data *md = mq-data; struct mmc_card *card = md-queue.card; - struct mmc_blk_request *brq = mq-mqrq_cur-brq; - int ret = 1, disable_multi = 0; + struct mmc_blk_request *brq; + int ret = 1; + int disable_multi = 0; enum mmc_blk_status status; + struct mmc_queue_req *mq_rq; + struct request *req; + struct mmc_async_req *areq; + + if (!rqc !mq-mqrq_prev-req) + goto out; do { - mmc_blk_rw_rq_prep(mq-mqrq_cur, card, disable_multi, mq); - mmc_wait_for_req(card-host, brq-mrq); + if (rqc) { + mmc_blk_rw_rq_prep(mq-mqrq_cur, card, 0, mq); + areq = mq-mqrq_cur-mmc_active; + } else + areq = NULL; + areq = mmc_start_req(card-host, areq, (int *) status); I think 'status' is used uninitialized. With this struct mmc_async_req *mmc_start_req in your first patch if (error) *error = err; return data; condition which always passes. You can have enum mmc_blk_status status = MMC_BLK_SUCCESS; struct mmc_async_req *mmc_start_req { err = host-areq-err_check(host-card, host-areq); if (err) { ... ... *error = err; } no need to update * error here in success case return data } Regards, Kishore -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/4] mmc: sdhci: fix interrupt storm from card detection
On Jun 20, 2011, at 3:38 AM, Shawn Guo wrote: The issue was initially found by Eric Benard as below. http://permalink.gmane.org/gmane.linux.ports.arm.kernel/108031 Not sure about other SDHCI based controller, but on Freescale eSDHC, the SDHCI_INT_CARD_INSERT bits will be immediately set again when it gets cleared, if a card is inserted. The driver need to mask the irq to prevent interrupt storm which will freeze the system. And the SDHCI_INT_CARD_REMOVE gets the same situation. The patch fixes the problem based on the initial idea from Eric Benard. Signed-off-by: Shawn Guo shawn@linaro.org Cc: Eric Benard e...@eukrea.com --- drivers/mmc/host/sdhci.c | 27 +++ 1 files changed, 23 insertions(+), 4 deletions(-) diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 91d9892..d94e2b4 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -127,7 +127,9 @@ static void sdhci_mask_irqs(struct sdhci_host *host, u32 irqs) static void sdhci_set_card_detection(struct sdhci_host *host, bool enable) { - u32 irqs = SDHCI_INT_CARD_REMOVE | SDHCI_INT_CARD_INSERT; + u32 present = sdhci_readl(host, SDHCI_PRESENT_STATE) + SDHCI_CARD_PRESENT; + u32 irqs = present ? SDHCI_INT_CARD_REMOVE : SDHCI_INT_CARD_INSERT; if (host-quirks SDHCI_QUIRK_BROKEN_CARD_DETECTION) return; Accessing off chip registers is slow. It would be better to check for the quirk first and then read the present state register. eg u32 present; u32 irqs; if (host-quirks SDHCI_QUIRK_BROKEN_CARD_DETECTION) return; present = sdhci_readl(host, SDHCI_PRESENT_STATE) SDHCI_CARD_PRESENT; irqs = present ? SDHCI_INT_CARD_REMOVE : SDHCI_INT_CARD_INSERT; Philip -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] mmc: tmio: fix recursive spinlock, don't schedule with interrupts disabled
Calling mmc_request_done() under a spinlock with interrupts disabled leads to a recursive spin-lock on request retry path and to scheduling in atomic context. This patch fixes both these problems by moving mmc_request_done() to the scheduler workqueue. Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de --- This is a bug-fix: without it the system Oopses with LOCKDEP enabled, so, it should really go in 3.0. OTOH it is pretty intrusine and non-trivial, so, reviews and tests are highly appreciated! Also, unfortunately, I wasn't able to test it well enough with SDIO, because the driver for the only SDIO card, that I have, reproducibly crashes the kernel: http://www.spinics.net/lists/kernel/msg1203334.html So, mainly tested with SD-cards and only lightly with SDIO. v2: 1. added a mutex to properly complete each .set_ios() call instead of returning an error, when racing with another one. 2. oritect data inside tmio_mmc_finish_request() 3. don't reschedule card detection, if one is already pending drivers/mmc/host/tmio_mmc.h |6 +- drivers/mmc/host/tmio_mmc_pio.c | 35 +-- 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h index 8260bc2..5b83e46 100644 --- a/drivers/mmc/host/tmio_mmc.h +++ b/drivers/mmc/host/tmio_mmc.h @@ -18,6 +18,7 @@ #include linux/highmem.h #include linux/mmc/tmio.h +#include linux/mutex.h #include linux/pagemap.h #include linux/spinlock.h @@ -73,8 +74,11 @@ struct tmio_mmc_host { /* Track lost interrupts */ struct delayed_work delayed_reset_work; - spinlock_t lock; + struct work_struct done; + + spinlock_t lock; /* protect host private data */ unsigned long last_req_ts; + struct mutexios_lock; /* protect set_ios() context */ }; int tmio_mmc_host_probe(struct tmio_mmc_host **host, diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c index 0b09e82..eabda39 100644 --- a/drivers/mmc/host/tmio_mmc_pio.c +++ b/drivers/mmc/host/tmio_mmc_pio.c @@ -284,10 +284,16 @@ static void tmio_mmc_reset_work(struct work_struct *work) /* called with host-lock held, interrupts disabled */ static void tmio_mmc_finish_request(struct tmio_mmc_host *host) { - struct mmc_request *mrq = host-mrq; + struct mmc_request *mrq; + unsigned long flags; - if (!mrq) + spin_lock_irqsave(host-lock, flags); + + mrq = host-mrq; + if (IS_ERR_OR_NULL(mrq)) { + spin_unlock_irqrestore(host-lock, flags); return; + } host-cmd = NULL; host-data = NULL; @@ -296,11 +302,18 @@ static void tmio_mmc_finish_request(struct tmio_mmc_host *host) cancel_delayed_work(host-delayed_reset_work); host-mrq = NULL; + spin_unlock_irqrestore(host-lock, flags); - /* FIXME: mmc_request_done() can schedule! */ mmc_request_done(host-mmc, mrq); } +static void tmio_mmc_done_work(struct work_struct *work) +{ + struct tmio_mmc_host *host = container_of(work, struct tmio_mmc_host, + done); + tmio_mmc_finish_request(host); +} + /* These are the bitmasks the tmio chip requires to implement the MMC response * types. Note that R1 and R6 are the same in this scheme. */ #define APP_CMD0x0040 @@ -467,7 +480,7 @@ void tmio_mmc_do_data_irq(struct tmio_mmc_host *host) BUG(); } - tmio_mmc_finish_request(host); + schedule_work(host-done); } static void tmio_mmc_data_irq(struct tmio_mmc_host *host) @@ -557,7 +570,7 @@ static void tmio_mmc_cmd_irq(struct tmio_mmc_host *host, tasklet_schedule(host-dma_issue); } } else { - tmio_mmc_finish_request(host); + schedule_work(host-done); } out: @@ -618,7 +631,8 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid) if (ireg (TMIO_STAT_CARD_INSERT | TMIO_STAT_CARD_REMOVE)) { tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_CARD_INSERT | TMIO_STAT_CARD_REMOVE); - mmc_detect_change(host-mmc, msecs_to_jiffies(100)); + if (!work_pending(host-mmc-detect.work)) + mmc_detect_change(host-mmc, msecs_to_jiffies(100)); } /* CRC and other errors */ @@ -749,6 +763,8 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) struct tmio_mmc_data *pdata = host-pdata; unsigned long flags; + mutex_lock(host-ios_lock); + spin_lock_irqsave(host-lock, flags); if (host-mrq) { if (IS_ERR(host-mrq)) { @@ -764,6 +780,8 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc,
Re: [PATCH] mmc: sdhci-pxa move platform data to include/linux/platform_data
On Monday 20 June 2011, Zhangfei Gao wrote: As suggested by Arnd, move platform data to include/linux/platform_data, Add 'depends on CLKDEV_LOOKUP' since clk API is used, As a result driver could be built on all platforms which support CLKDEV_LOOKUP This can improve the build test coverage Signed-off-by: Zhangfei Gao zhangfei@marvell.com CC: Arnd Bergmann a...@arndb.de Acked-by: Arnd Bergmann a...@arndb.de BTW, For some reason, your changelog text above is still indented funny. This should not happen if you use git-format-patch rather than git-show. Arnd -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdhci-pxa move platform data to include/linux/platform_data
Zhangfei, Would you mind posting the patch to the mmc mailing list. It has not shown up in my inbox. Philip On Jun 20, 2011, at 9:29 AM, Arnd Bergmann wrote: On Monday 20 June 2011, Zhangfei Gao wrote: As suggested by Arnd, move platform data to include/linux/platform_data, Add 'depends on CLKDEV_LOOKUP' since clk API is used, As a result driver could be built on all platforms which support CLKDEV_LOOKUP This can improve the build test coverage Signed-off-by: Zhangfei Gao zhangfei@marvell.com CC: Arnd Bergmann a...@arndb.de Acked-by: Arnd Bergmann a...@arndb.de BTW, For some reason, your changelog text above is still indented funny. This should not happen if you use git-format-patch rather than git-show. Arnd -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: dw_mmc: set the card_width bit per card.
Hi, On Mon, Jun 20 2011, Will Newton wrote: On Mon, Jun 20, 2011 at 9:23 AM, Seungwon Jeon tgih@samsung.com wrote: This patch sets the card_width bit of CTYPE for the corresponding card. CTYPE[31] and CTYPE[16] correspond respectively to card[15] and card[0] for 8-bit mode. And CTYPE[15] and CTYPE[0] correspond respectively to card[15] and CTYPE[0] for 1-bit or 4-bit mode. Signed-off-by: Seungwon Jeon tgih@samsung.com Acked-by: Will Newton will.new...@imgtec.com Pushed to mmc-next for 3.1, thanks. - Chris. -- Chris Ball c...@laptop.org http://printf.net/ One Laptop Per Child -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] Improve MMC error handling (3rd rev)
This is the third revision of my improvements to the MMC block device error handling, which makes error handling more robust and permits MMC/SD to continue working in the presence of not-quite-perfect setups. Without this, my Versatile Express tends to fail to mount its rootfs on SD. With this, it can successfully read and write data from the card in the presence of FIFO overruns and underruns, and also sensibly recover from command channel errors. There is more to come, but this is the safer bits of the improvements. drivers/mmc/card/block.c | 279 ++--- include/linux/mmc/mmc.h | 10 ++ 2 files changed, 221 insertions(+), 68 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] MMC: allow get_card_status() to return error status
If the MMC_SEND_STATUS command is not successful, we should not return a zero status word, but instead allow the caller to know positively that an error occurred. Convert the open-coded get_card_status() to use the helper function, and provide definitions for the card state field. Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk --- drivers/mmc/card/block.c | 33 + include/linux/mmc/mmc.h | 10 ++ 2 files changed, 19 insertions(+), 24 deletions(-) diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c index 71da564..022edc3 100644 --- a/drivers/mmc/card/block.c +++ b/drivers/mmc/card/block.c @@ -525,7 +525,7 @@ static u32 mmc_sd_num_wr_blocks(struct mmc_card *card) return result; } -static u32 get_card_status(struct mmc_card *card, struct request *req) +static int get_card_status(struct mmc_card *card, u32 *status, int retries) { struct mmc_command cmd = {0}; int err; @@ -534,11 +534,10 @@ static u32 get_card_status(struct mmc_card *card, struct request *req) if (!mmc_host_is_spi(card-host)) cmd.arg = card-rca 16; cmd.flags = MMC_RSP_SPI_R2 | MMC_RSP_R1 | MMC_CMD_AC; - err = mmc_wait_for_cmd(card-host, cmd, 0); - if (err) - printk(KERN_ERR %s: error %d sending status command, - req-rq_disk-disk_name, err); - return cmd.resp[0]; + err = mmc_wait_for_cmd(card-host, cmd, retries); + if (err == 0) + *status = cmd.resp[0]; + return err; } static int mmc_blk_issue_discard_rq(struct mmc_queue *mq, struct request *req) @@ -686,7 +685,6 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *req) (md-flags MMC_BLK_REL_WR); do { - struct mmc_command cmd = {0}; u32 readcmd, writecmd, status = 0; memset(brq, 0, sizeof(struct mmc_blk_request)); @@ -817,7 +815,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *req) disable_multi = 1; continue; } - status = get_card_status(card, req); + get_card_status(card, status, 0); } if (brq.sbc.error) { @@ -854,12 +852,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *req) if (!mmc_host_is_spi(card-host) rq_data_dir(req) != READ) { do { - int err; - - cmd.opcode = MMC_SEND_STATUS; - cmd.arg = card-rca 16; - cmd.flags = MMC_RSP_R1 | MMC_CMD_AC; - err = mmc_wait_for_cmd(card-host, cmd, 5); + int err = get_card_status(card, status, 5); if (err) { printk(KERN_ERR %s: error %d requesting status\n, req-rq_disk-disk_name, err); @@ -870,16 +863,8 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *req) * so make sure to check both the busy * indication and the card state. */ - } while (!(cmd.resp[0] R1_READY_FOR_DATA) || - (R1_CURRENT_STATE(cmd.resp[0]) == 7)); - -#if 0 - if (cmd.resp[0] ~0x0900) - printk(KERN_ERR %s: status = %08x\n, - req-rq_disk-disk_name, cmd.resp[0]); - if (mmc_decode_status(cmd.resp)) - goto cmd_err; -#endif + } while (!(status R1_READY_FOR_DATA) || +(R1_CURRENT_STATE(status) == R1_STATE_PRG)); } if (brq.cmd.error || brq.stop.error || brq.data.error) { diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h index ac26a68..8e425ab 100644 --- a/include/linux/mmc/mmc.h +++ b/include/linux/mmc/mmc.h @@ -140,6 +140,16 @@ static inline bool mmc_op_multi(u32 opcode) #define R1_SWITCH_ERROR(1 7)/* sx, c */ #define R1_APP_CMD (1 5)/* sr, c */ +#define R1_STATE_IDLE 0 +#define R1_STATE_READY 1 +#define R1_STATE_IDENT 2 +#define R1_STATE_STBY 3 +#define R1_STATE_TRAN 4 +#define R1_STATE_DATA 5 +#define R1_STATE_RCV 6 +#define R1_STATE_PRG 7 +#define R1_STATE_DIS 8 + /* * MMC/SD in SPI mode reports R1 status always, and R2 for SEND_STATUS * R1 is the low order byte; R2 is the next highest byte, when present. -- 1.7.4.4 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More
[PATCH 2/3] MMC: improve error recovery from command channel errors
Command channel errors fall into four classes: 1. The command was issued with the card in the wrong state 2. The command failed to be received by the card correctly 3. The cards response failed to be received by the host (CRC error) 4. The card failed to respond to the card For (1), in theory we should know that the card is in the correct state. However, a failed stop command (or other failure) may result in the card remaining in a data transfer state from the previous command. If we detect this condition, we try to recover by sending a stop command. For the initial commands (set block count and the read/write command) no data will have been transferred. All that we need deal with is retrying at this point. A failed stop command can be remedied as above. If we are unable to recover the card (eg, the card ignores our requests for status, or we don't recognise the error code) then we immediately fail the request. Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk --- drivers/mmc/card/block.c | 230 -- 1 files changed, 182 insertions(+), 48 deletions(-) diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c index 022edc3..aa074c8 100644 --- a/drivers/mmc/card/block.c +++ b/drivers/mmc/card/block.c @@ -525,6 +525,19 @@ static u32 mmc_sd_num_wr_blocks(struct mmc_card *card) return result; } +static int send_stop(struct mmc_card *card, u32 *status) +{ + struct mmc_command cmd = {0}; + int err; + + cmd.opcode = MMC_STOP_TRANSMISSION; + cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC; + err = mmc_wait_for_cmd(card-host, cmd, 5); + if (err == 0) + *status = cmd.resp[0]; + return err; +} + static int get_card_status(struct mmc_card *card, u32 *status, int retries) { struct mmc_command cmd = {0}; @@ -540,6 +553,137 @@ static int get_card_status(struct mmc_card *card, u32 *status, int retries) return err; } +#define ERR_RETRY 2 +#define ERR_ABORT 1 +#define ERR_CONTINUE 0 + +static int mmc_blk_cmd_error(struct request *req, const char *name, int error, + bool status_valid, u32 status) +{ + switch (error) { + case -EILSEQ: + /* response crc error, retry the r/w cmd */ + pr_err(%s: %s sending %s command, card status %#x\n, + req-rq_disk-disk_name, response CRC error, + name, status); + return ERR_RETRY; + + case -ETIMEDOUT: + pr_err(%s: %s sending %s command, card status %#x\n, + req-rq_disk-disk_name, timed out, name, status); + + /* If the status cmd initially failed, retry the r/w cmd */ + if (!status_valid) + return ERR_RETRY; + + /* +* If it was a r/w cmd crc error, or illegal command +* (eg, issued in wrong state) then retry - we should +* have corrected the state problem above. +*/ + if (status (R1_COM_CRC_ERROR | R1_ILLEGAL_COMMAND)) + return ERR_RETRY; + + /* Otherwise abort the command */ + return ERR_ABORT; + + default: + /* We don't understand the error code the driver gave us */ + pr_err(%s: unknown error %d sending read/write command, card status %#x\n, + req-rq_disk-disk_name, error, status); + return ERR_ABORT; + } +} + +/* + * Initial r/w and stop cmd error recovery. + * We don't know whether the card received the r/w cmd or not, so try to + * restore things back to a sane state. Essentially, we do this as follows: + * - Obtain card status. If the first attempt to obtain card status fails, + * the status word will reflect the failed status cmd, not the failed + * r/w cmd. If we fail to obtain card status, it suggests we can no + * longer communicate with the card. + * - Check the card state. If the card received the cmd but there was a + * transient problem with the response, it might still be in a data transfer + * mode. Try to send it a stop command. If this fails, we can't recover. + * - If the r/w cmd failed due to a response CRC error, it was probably + * transient, so retry the cmd. + * - If the r/w cmd timed out, but we didn't get the r/w cmd status, retry. + * - If the r/w cmd timed out, and the r/w cmd failed due to CRC error or + * illegal cmd, retry. + * Otherwise we don't understand what happened, so abort. + */ +static int mmc_blk_cmd_recovery(struct mmc_card *card, struct request *req, + struct mmc_blk_request *brq) +{ + bool prev_cmd_status_valid = true; + u32 status, stop_status = 0; + int err, retry; + + /* +* Try to get card status which indicates both the card state +* and why there was no response. If the first attempt fails, +
[PATCH 3/3] MMC: add checking of r/w command response
Check the status bits in the r/w command response for any errors. If error bits are set, then we won't have seen any data transferred, so its pointless doing any further checking. Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk --- drivers/mmc/card/block.c | 26 +- 1 files changed, 25 insertions(+), 1 deletions(-) diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c index aa074c8..e5d1c8a 100644 --- a/drivers/mmc/card/block.c +++ b/drivers/mmc/card/block.c @@ -812,6 +812,14 @@ static inline void mmc_apply_rel_rw(struct mmc_blk_request *brq, } } +#define CMD_ERRORS \ + (R1_OUT_OF_RANGE | /* Command argument out of range */ \ +R1_ADDRESS_ERROR | /* Misaligned address */\ +R1_BLOCK_LEN_ERROR | /* Transferred block length incorrect */\ +R1_WP_VIOLATION | /* Tried to write to protected block */ \ +R1_CC_ERROR | /* Card controller error */ \ +R1_ERROR) /* General/unknown error */ + static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *req) { struct mmc_blk_data *md = mq-data; @@ -967,6 +975,22 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *req) } } + /* +* Check for errors relating to the execution of the +* initial command - such as address errors. No data +* has been transferred. +*/ + if (brq.cmd.resp[0] CMD_ERRORS) { + pr_err(%s: r/w command failed, status = %#x\n, + req-rq_disk-disk_name, brq.cmd.resp[0]); + goto cmd_abort; + } + + /* +* Everything else is either success, or a data error of some +* kind. If it was a write, we may have transitioned to +* program mode, which we have to wait for it to complete. +*/ if (!mmc_host_is_spi(card-host) rq_data_dir(req) != READ) { u32 status; do { @@ -986,7 +1010,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *req) } if (brq.data.error) { - pr_err(%s: error %d transferring data, sector %u nr %u, cmd response %#x card status %#x\n, + pr_err(%s: error %d transferring data, sector %u, nr %u, cmd response %#x, card status %#x\n, req-rq_disk-disk_name, brq.data.error, (unsigned)blk_rq_pos(req), (unsigned)blk_rq_sectors(req), -- 1.7.4.4 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: SDHCI I/O errors
Hi Andy, On Mon, Jun 20 2011, Andrew Lutomirski wrote: I have an SDHC card that works fine in a camera and in several different USB-based readers. On my sdhci reader (the integrated one on a Lenovo X220), though, it spews stuff like: [ 306.103556] mmcblk0: error -110 sending read/write command, response 0x0, card status 0x400b00 [ 306.103559] end_request: I/O error, dev mmcblk0, sector 410272 [ 306.103595] mmc0: Got data interrupt 0x0060 even though no data operation was in progress. [ 306.103597] sdhci: === REGISTER DUMP (mmc0)=== [ 306.103602] sdhci: Sys addr: 0xf9253200 | Version: 0x0502 [ 306.103607] sdhci: Blk size: 0x7200 | Blk cnt: 0x0001 [ 306.103613] sdhci: Argument: 0x000642a1 | Trn mode: 0x0013 [ 306.103618] sdhci: Present: 0x01cf | Host ctl: 0x0003 [ 306.103623] sdhci: Power:0x000f | Blk gap: 0x [ 306.103628] sdhci: Wake-up: 0x | Clock:0x0107 [ 306.103634] sdhci: Timeout: 0x0009 | Int stat: 0x [ 306.103639] sdhci: Int enab: 0x02ff00cb | Sig enab: 0x02ff00cb [ 306.103644] sdhci: AC12 err: 0x | Slot int: 0x [ 306.103649] sdhci: Caps: 0x21e8c8b2 | Caps_1: 0x8073 [ 306.103654] sdhci: Cmd: 0x113a | Max curr: 0x0040 [ 306.103655] sdhci: === all over the logs and fails some (but not all) attempts to read it. The startup messages are: [ 830.308403] sdhci-pci :0d:00.0: SDHCI controller found [1180:e823] (rev 4) [ 830.308632] sdhci-pci :0d:00.0: PCI INT A - GSI 16 (level, low) - IRQ 16 [ 830.308720] sdhci-pci :0d:00.0: setting latency timer to 64 [ 830.308730] mmc0: no vmmc regulator found [ 830.308802] Registered led device: mmc0:: [ 830.309059] mmc0: SDHCI controller on PCI [:0d:00.0] using DMA and lspci says: 0d:00.0 System peripheral: Ricoh Co Ltd Device e823 (rev 04) I see this on 2.6.39.1 and 3.0 git from today. Any idea whether this is present in previous kernels too? It'd be great if you can test a few earlier ones and find out whether we're looking at a regression or just new hardware to add quirks for. Thanks, - Chris. -- Chris Ball c...@laptop.org http://printf.net/ One Laptop Per Child -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Improve MMC error handling (3rd rev)
On Mon, Jun 20, 2011 at 9:09 PM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: This is the third revision of my improvements to the MMC block device error handling, which makes error handling more robust and permits MMC/SD to continue working in the presence of not-quite-perfect setups. All look like perfectly reasonable hardening of the MMC error path, so FWIW: Acked-by: Linus Walleij linus.wall...@linaro.org Thanks, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: irq flood with mmc boot partitions on s3c2416 with 3.0rc1
Heiko, On Sun, Jun 19, 2011 at 9:23 AM, Heiko Stübner he...@sntech.de wrote: Am Samstag 18 Juni 2011, 22:56:11 schrieb Daniel Mack: The log you sent out seems a bit short (it's covers 1s of boot time - usb0: no IPv6 routers present is the last line ). Can you send - 1) A full log with errors without any of my patches. 2) A full log with just the first patch. 3) A full log with just the second patch. I'm interested in knowing what the pattern of access to boot0 and boot1 is that causes these failures - does it only report I/O errors for for certain blocks (say, above some number) or for all of them. A -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] mmc: SDHI: DMA slave ID 0 is invalid
Hi Guennadi, On Mon, Jun 20 2011, Guennadi Liakhovetski wrote: Can we please get this patch too in for 3.0? It is a part of a patch-series, of which all others havs already been applied. This specific patch makes the behaviour of the SDHI driver consistent with other DMA users on sh-mobile, namely, that default platform data with no DMA information provided immediately switches the driver to the PIO mode. This patch fixes this logical error of trying to acquire DMA channels with invalid configuration information and avoids ugly error messages in the kernel log. Thanks Guennadi On Tue, 24 May 2011, Guennadi Liakhovetski wrote: Don't try to allocate DMA resources, if the platform didn't specify positive DMA slave IDs. Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de --- drivers/mmc/host/sh_mobile_sdhi.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c index c05d699..e646741 100644 --- a/drivers/mmc/host/sh_mobile_sdhi.c +++ b/drivers/mmc/host/sh_mobile_sdhi.c @@ -132,7 +132,7 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev) mmc_data-ocr_mask = p-tmio_ocr_mask; mmc_data-capabilities |= p-tmio_caps; -if (p-dma_slave_tx = 0 p-dma_slave_rx = 0) { +if (p-dma_slave_tx 0 p-dma_slave_rx 0) { priv-param_tx.slave_id = p-dma_slave_tx; priv-param_rx.slave_id = p-dma_slave_rx; priv-dma_priv.chan_priv_tx = priv-param_tx; -- 1.7.2.5 Thanks, pushed to mmc-next for 3.0. - Chris. -- Chris Ball c...@laptop.org http://printf.net/ One Laptop Per Child -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: SDHCI I/O errors
Hi Andy, On Mon, Jun 20 2011, Chris Ball wrote: On Mon, Jun 20 2011, Andrew Lutomirski wrote: I have an SDHC card that works fine in a camera and in several different USB-based readers. On my sdhci reader (the integrated one on a Lenovo X220), though, it spews stuff like: [ 306.103556] mmcblk0: error -110 sending read/write command, response 0x0, card status 0x400b00 [ 306.103559] end_request: I/O error, dev mmcblk0, sector 410272 [ 306.103595] mmc0: Got data interrupt 0x0060 even though no data operation was in progress. [ 306.103597] sdhci: === REGISTER DUMP (mmc0)=== [ 306.103602] sdhci: Sys addr: 0xf9253200 | Version: 0x0502 [ 306.103607] sdhci: Blk size: 0x7200 | Blk cnt: 0x0001 [ 306.103613] sdhci: Argument: 0x000642a1 | Trn mode: 0x0013 [ 306.103618] sdhci: Present: 0x01cf | Host ctl: 0x0003 [ 306.103623] sdhci: Power:0x000f | Blk gap: 0x [ 306.103628] sdhci: Wake-up: 0x | Clock:0x0107 [ 306.103634] sdhci: Timeout: 0x0009 | Int stat: 0x [ 306.103639] sdhci: Int enab: 0x02ff00cb | Sig enab: 0x02ff00cb [ 306.103644] sdhci: AC12 err: 0x | Slot int: 0x [ 306.103649] sdhci: Caps: 0x21e8c8b2 | Caps_1: 0x8073 [ 306.103654] sdhci: Cmd: 0x113a | Max curr: 0x0040 [ 306.103655] sdhci: === all over the logs and fails some (but not all) attempts to read it. The startup messages are: [ 830.308403] sdhci-pci :0d:00.0: SDHCI controller found [1180:e823] (rev 4) [ 830.308632] sdhci-pci :0d:00.0: PCI INT A - GSI 16 (level, low) - IRQ 16 [ 830.308720] sdhci-pci :0d:00.0: setting latency timer to 64 [ 830.308730] mmc0: no vmmc regulator found [ 830.308802] Registered led device: mmc0:: [ 830.309059] mmc0: SDHCI controller on PCI [:0d:00.0] using DMA and lspci says: 0d:00.0 System peripheral: Ricoh Co Ltd Device e823 (rev 04) I see this on 2.6.39.1 and 3.0 git from today. Any idea whether this is present in previous kernels too? It'd be great if you can test a few earlier ones and find out whether we're looking at a regression or just new hardware to add quirks for. Ah, could you try this patch, please? http://thread.gmane.org/gmane.linux.kernel.mmc/8254 (mmc: Enable MMC card reader for RICOH [1180:e823]) It's already present in mmc-next and queued for 3.0. - Chris. -- Chris Ball c...@laptop.org http://printf.net/ One Laptop Per Child -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: SDHCI I/O errors
On Mon, Jun 20, 2011 at 3:40 PM, Chris Ball c...@laptop.org wrote: Hi Andy, On Mon, Jun 20 2011, Chris Ball wrote: On Mon, Jun 20 2011, Andrew Lutomirski wrote: I have an SDHC card that works fine in a camera and in several different USB-based readers. On my sdhci reader (the integrated one on a Lenovo X220), though, it spews stuff like: [ 306.103556] mmcblk0: error -110 sending read/write command, response 0x0, card status 0x400b00 [ 306.103559] end_request: I/O error, dev mmcblk0, sector 410272 [ 306.103595] mmc0: Got data interrupt 0x0060 even though no data operation was in progress. [ 306.103597] sdhci: === REGISTER DUMP (mmc0)=== [ 306.103602] sdhci: Sys addr: 0xf9253200 | Version: 0x0502 [ 306.103607] sdhci: Blk size: 0x7200 | Blk cnt: 0x0001 [ 306.103613] sdhci: Argument: 0x000642a1 | Trn mode: 0x0013 [ 306.103618] sdhci: Present: 0x01cf | Host ctl: 0x0003 [ 306.103623] sdhci: Power: 0x000f | Blk gap: 0x [ 306.103628] sdhci: Wake-up: 0x | Clock: 0x0107 [ 306.103634] sdhci: Timeout: 0x0009 | Int stat: 0x [ 306.103639] sdhci: Int enab: 0x02ff00cb | Sig enab: 0x02ff00cb [ 306.103644] sdhci: AC12 err: 0x | Slot int: 0x [ 306.103649] sdhci: Caps: 0x21e8c8b2 | Caps_1: 0x8073 [ 306.103654] sdhci: Cmd: 0x113a | Max curr: 0x0040 [ 306.103655] sdhci: === all over the logs and fails some (but not all) attempts to read it. The startup messages are: [ 830.308403] sdhci-pci :0d:00.0: SDHCI controller found [1180:e823] (rev 4) [ 830.308632] sdhci-pci :0d:00.0: PCI INT A - GSI 16 (level, low) - IRQ 16 [ 830.308720] sdhci-pci :0d:00.0: setting latency timer to 64 [ 830.308730] mmc0: no vmmc regulator found [ 830.308802] Registered led device: mmc0:: [ 830.309059] mmc0: SDHCI controller on PCI [:0d:00.0] using DMA and lspci says: 0d:00.0 System peripheral: Ricoh Co Ltd Device e823 (rev 04) I see this on 2.6.39.1 and 3.0 git from today. Any idea whether this is present in previous kernels too? It'd be great if you can test a few earlier ones and find out whether we're looking at a regression or just new hardware to add quirks for. 2.6.37 doesn't recognize the reader, and if I add the PCI id via new_id, the driver fails with a different error. I'll post that in a bit. I tried a 2.6.38 Fedora 15 kernel but it didn't boot (dracut bug). I can fiddle with it and get a .38 kernel working if you like. Ah, could you try this patch, please? http://thread.gmane.org/gmane.linux.kernel.mmc/8254 (mmc: Enable MMC card reader for RICOH [1180:e823]) Doesn't help. Some tracing shows that the initial (BIOS-provided?) value of config byte 0xCB is 0x87. --Andy -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] PCI: honor child buses add_size in hot plug configuration
From: Yinghai Lu ying...@kernel.org Recent pci_bus_size change will use add_size for minimum resource size for pcie hotplug bridge. But it does not pass children back to parent bridge. that will have problem on some setup like: hot add one chassis with more hot plug slots. for example: if the chassis have 8 slots, we should allocate 8x2M instead of one 1x2M for parent bus. So try to get child add_size and compare the sum with parent bus bridge... Signed-off-by: Yinghai Lu ying...@kernel.org Reviewed-by: Ram Pai linux...@us.ibm.com --- drivers/pci/setup-bus.c | 26 ++ 1 files changed, 26 insertions(+), 0 deletions(-) diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c index 1e9e5a5..e42b89a 100644 --- a/drivers/pci/setup-bus.c +++ b/drivers/pci/setup-bus.c @@ -536,6 +536,20 @@ static resource_size_t calculate_memsize(resource_size_t size, return size; } +static resource_size_t get_res_add_size(struct resource_list_x *add_head, + struct resource *res) +{ + struct resource_list_x *list; + + /* check if it is in add_head list */ + for (list = add_head-next; list list-res != res; + list = list-next); + if (list) + return list-add_size; + + return 0; +} + /** * pbus_size_io() - size the io window of a given bus * @@ -555,6 +569,7 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size, struct pci_dev *dev; struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO); unsigned long size = 0, size0 = 0, size1 = 0; + resource_size_t children_add_size = 0; if (!b_res) return; @@ -575,10 +590,15 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size, size += r_size; else size1 += r_size; + + if (add_head) + children_add_size += get_res_add_size(add_head, r); } } size0 = calculate_iosize(size, min_size, size1, resource_size(b_res), 4096); + if (children_add_size add_size) + add_size = children_add_size; size1 = (!add_head || (add_head !add_size)) ? size0 : calculate_iosize(size, min_size+add_size, size1, resource_size(b_res), 4096); @@ -620,6 +640,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, int order, max_order; struct resource *b_res = find_free_bus_resource(bus, type); unsigned int mem64_mask = 0; + resource_size_t children_add_size = 0; if (!b_res) return 0; @@ -661,6 +682,9 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, if (order max_order) max_order = order; mem64_mask = r-flags IORESOURCE_MEM_64; + + if (add_head) + children_add_size += get_res_add_size(add_head, r); } } align = 0; @@ -677,6 +701,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, align += aligns[order]; } size0 = calculate_memsize(size, min_size, 0, resource_size(b_res), min_align); + if (children_add_size add_size) + add_size = children_add_size; size1 = (!add_head || (add_head !add_size)) ? size0 : calculate_memsize(size, min_size+add_size, 0, resource_size(b_res), min_align); -- 1.7.0.4 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/4] PCI : ability to resize assigned pci-resource
Currently pci-bridges are allocated enough resources to satisfy their immediate requirements. Any additional resource-requests fail if additional free space, contiguous to the one already allocated, is not available. This behavior is not satisfying when sufficient contiguous resources, that can satisfy the request, is available in a different location. This patch provides the ability to expand a allocated resource. This behavior is particularly useful to satisfy larger size nice-to-have resource requests, like SRIOV BARs or cardbus bridges. Signed-off-by: Ram Pai linux...@us.ibm.com --- drivers/pci/setup-bus.c | 30 ++--- drivers/pci/setup-res.c | 152 +-- include/linux/pci.h |1 + kernel/resource.c | 98 --- 4 files changed, 206 insertions(+), 75 deletions(-) diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c index e42b89a..c282c48 100644 --- a/drivers/pci/setup-bus.c +++ b/drivers/pci/setup-bus.c @@ -34,6 +34,7 @@ struct resource_list_x { resource_size_t start; resource_size_t end; resource_size_t add_size; + resource_size_t min_align; unsigned long flags; }; @@ -58,7 +59,7 @@ struct resource_list_x { */ static void add_to_list(struct resource_list_x *head, struct pci_dev *dev, struct resource *res, -resource_size_t add_size) +resource_size_t add_size, resource_size_t min_align) { struct resource_list_x *list = head; struct resource_list_x *ln = list-next; @@ -77,13 +78,16 @@ static void add_to_list(struct resource_list_x *head, tmp-end = res-end; tmp-flags = res-flags; tmp-add_size = add_size; + tmp-min_align = min_align; list-next = tmp; } static void add_to_failed_list(struct resource_list_x *head, struct pci_dev *dev, struct resource *res) { - add_to_list(head, dev, res, 0); + add_to_list(head, dev, res, + 0 /* dont care */, + 0 /* dont care */); } static void __dev_sort_resources(struct pci_dev *dev, @@ -152,13 +156,19 @@ static void adjust_resources_sorted(struct resource_list_x *add_head, idx = res - list-dev-resource[0]; add_size=list-add_size; - if (!resource_size(res) add_size) { -res-end = res-start + add_size - 1; -if(pci_assign_resource(list-dev, idx)) + if (!resource_size(res)) { + res-end = res-start + add_size - 1; + if(pci_assign_resource(list-dev, idx)) reset_resource(res); - } else if (add_size) { - adjust_resource(res, res-start, - resource_size(res) + add_size); + } else { + resource_size_t align = list-min_align; + res-flags |= list-flags + (IORESOURCE_STARTALIGN|IORESOURCE_SIZEALIGN); + if (pci_reassign_resource(list-dev, idx, add_size, + align)) + dev_printk(KERN_DEBUG, list-dev-dev, + failed to add optional resources res=%pR\n, + res); } out: tmp = list; @@ -615,7 +625,7 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size, b_res-end = b_res-start + size0 - 1; b_res-flags |= IORESOURCE_STARTALIGN; if (size1 size0 add_head) - add_to_list(add_head, bus-self, b_res, size1-size0); + add_to_list(add_head, bus-self, b_res, size1-size0, 4096); } /** @@ -718,7 +728,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, b_res-end = size0 + min_align - 1; b_res-flags |= IORESOURCE_STARTALIGN | mem64_mask; if (size1 size0 add_head) - add_to_list(add_head, bus-self, b_res, size1-size0); + add_to_list(add_head, bus-self, b_res, size1-size0, min_align); return 1; } diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c index bc0e6ee..e8a94d5 100644 --- a/drivers/pci/setup-res.c +++ b/drivers/pci/setup-res.c @@ -129,16 +129,16 @@ void pci_disable_bridge_window(struct pci_dev *dev) } #endif /* CONFIG_PCI_QUIRKS */ + + static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev, -int resno) + int resno, resource_size_t size, resource_size_t align) { struct resource *res = dev-resource + resno; - resource_size_t size, min, align; + resource_size_t min; int ret; - size = resource_size(res); min = (res-flags IORESOURCE_IO) ? PCIBIOS_MIN_IO :
[PATCH 3/4] PCI: make SRIOV resources nice-to-have
From: Yinghai Lu ying...@kernel.org Allocate resources to SRIOV BARs only after all other genuine resources requests are satisfied. Dont retry if resource allocation for SRIOV BARs fail. Signed-off-by: Ram Pai linux...@us.ibm.com Signed-off-by: Yinghai Lu ying...@kernel.org --- drivers/pci/setup-bus.c | 10 ++ 1 files changed, 10 insertions(+), 0 deletions(-) diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c index c282c48..4f8873e 100644 --- a/drivers/pci/setup-bus.c +++ b/drivers/pci/setup-bus.c @@ -672,6 +672,16 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, if (r-parent || (r-flags mask) != type) continue; r_size = resource_size(r); +#ifdef CONFIG_PCI_IOV + /* add SRIOV BARs to nice-to-have list */ + if (add_head i = PCI_IOV_RESOURCES + i = PCI_IOV_RESOURCE_END) { + r-end = r-start - 1; + add_to_list(add_head, dev, r, r_size, 1); + children_add_size += r_size; + continue; + } +#endif /* For bridges size != alignment */ align = pci_resource_alignment(dev, r); order = __ffs(align) - 20; -- 1.7.0.4 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/4] PCI: make cardbus-bridge resources nice-to-have
Allocate resources to cardbus bridge only after all other genuine resources requests are satisfied. Dont retry if resource allocation for cardbus-bridge fails. Tested-by: Oliver Hartkopp socket...@hartkopp.net Signed-off-by: Ram Pai linux...@us.ibm.com --- drivers/pci/setup-bus.c | 20 +--- 1 files changed, 13 insertions(+), 7 deletions(-) diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c index 4f8873e..023fc9c 100644 --- a/drivers/pci/setup-bus.c +++ b/drivers/pci/setup-bus.c @@ -742,7 +742,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, return 1; } -static void pci_bus_size_cardbus(struct pci_bus *bus) +static void pci_bus_size_cardbus(struct pci_bus *bus, + struct resource_list_x *add_head) { struct pci_dev *bridge = bus-self; struct resource *b_res = bridge-resource[PCI_BRIDGE_RESOURCES]; @@ -753,12 +754,14 @@ static void pci_bus_size_cardbus(struct pci_bus *bus) * a fixed amount of bus space for CardBus bridges. */ b_res[0].start = 0; - b_res[0].end = pci_cardbus_io_size - 1; + b_res[0].end = 0; b_res[0].flags |= IORESOURCE_IO | IORESOURCE_SIZEALIGN; + add_to_list(add_head, bridge, b_res, pci_cardbus_io_size - 1, 1); b_res[1].start = 0; - b_res[1].end = pci_cardbus_io_size - 1; + b_res[1].end = 0; b_res[1].flags |= IORESOURCE_IO | IORESOURCE_SIZEALIGN; + add_to_list(add_head, bridge, b_res+1, pci_cardbus_io_size - 1, 1); /* * Check whether prefetchable memory is supported @@ -778,16 +781,19 @@ static void pci_bus_size_cardbus(struct pci_bus *bus) */ if (ctrl PCI_CB_BRIDGE_CTL_PREFETCH_MEM0) { b_res[2].start = 0; - b_res[2].end = pci_cardbus_mem_size - 1; + b_res[2].end = 0; b_res[2].flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH | IORESOURCE_SIZEALIGN; + add_to_list(add_head, bridge, b_res+2, pci_cardbus_mem_size - 1, 1); b_res[3].start = 0; - b_res[3].end = pci_cardbus_mem_size - 1; + b_res[3].end = 0; b_res[3].flags |= IORESOURCE_MEM | IORESOURCE_SIZEALIGN; + add_to_list(add_head, bridge, b_res+3, pci_cardbus_mem_size - 1, 1); } else { b_res[3].start = 0; - b_res[3].end = pci_cardbus_mem_size * 2 - 1; + b_res[3].end = 0; b_res[3].flags |= IORESOURCE_MEM | IORESOURCE_SIZEALIGN; + add_to_list(add_head, bridge, b_res+3, pci_cardbus_mem_size * 2 - 1, 1); } } @@ -805,7 +811,7 @@ void __ref __pci_bus_size_bridges(struct pci_bus *bus, switch (dev-class 8) { case PCI_CLASS_BRIDGE_CARDBUS: - pci_bus_size_cardbus(b); + pci_bus_size_cardbus(b, add_head); break; case PCI_CLASS_BRIDGE_PCI: -- 1.7.0.4 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/5 v2] mmc: sdhi: Allow waiting for idle
Some SDHI controllers require waiting for the SD bus to become idle before writing to some registers. This series allows this to occur by: * ensuring all relevant accesses are made via sd_ctrl_write16() * Adding a hook to sd_ctrl_write16() * Supplying a hook that implements waiting for idle The series also includes some clean-ups of related code. The first three patches are intended for the mmc tree * [PATCH 1/5] mmc: tmio: name 0xd8 as CTL_DMA_ENABLE * [PATCH 2/5] mmc: tmio: Share register access functions * [PATCH 3/5] mmc: sdhi: Add write16_hook The remaining two patches are intended for the rmobile tree * [PATCH 4/5] ARM: mach-shmobile: ag5evm: consistently name sdhi info * [PATCH 5/5] ARM: mach-shmobile: ag5evm: SDHI requires waiting for I am not aware of any dependencies on patches outside this series. Dependencies on patches within the series are noted inline. This is v2 of this patch series, issues addressed since v1 are all in [PATCH 3/5] mmc: sdhi: Add write16_hook: * Include linux/delay.h instead of asm/delay.h * Skip write if sh_mobile_sdhi_wait_idle() times out - The bus will probably be in an inconsistent state and writing may lock up the bus * Only set hook if TMIO_MMC_HAS_IDLE_WAIT is set in platform data rather than checking for TMIO_MMC_HAS_IDLE_WAIT each time the hook is called. -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/5] ARM: mach-shmobile: ag5evm: consistently name sdhi info structures
Name the SDHI1 instance sh_sdhi1_info to be consistent with sh_sdhi0_info. Signed-off-by: Simon Horman ho...@verge.net.au --- Dependencies: None known --- arch/arm/mach-shmobile/board-ag5evm.c |4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/arm/mach-shmobile/board-ag5evm.c b/arch/arm/mach-shmobile/board-ag5evm.c index 1e2aba2..ce5c251 100644 --- a/arch/arm/mach-shmobile/board-ag5evm.c +++ b/arch/arm/mach-shmobile/board-ag5evm.c @@ -381,7 +381,7 @@ void ag5evm_sdhi1_set_pwr(struct platform_device *pdev, int state) gpio_set_value(GPIO_PORT114, state); } -static struct sh_mobile_sdhi_info sh_sdhi1_platdata = { +static struct sh_mobile_sdhi_info sh_sdhi1_info = { .tmio_flags = TMIO_MMC_WRPROTECT_DISABLE, .tmio_caps = MMC_CAP_NONREMOVABLE | MMC_CAP_SDIO_IRQ, .tmio_ocr_mask = MMC_VDD_32_33 | MMC_VDD_33_34, @@ -413,7 +413,7 @@ static struct platform_device sdhi1_device = { .name = sh_mobile_sdhi, .id = 1, .dev= { - .platform_data = sh_sdhi1_platdata, + .platform_data = sh_sdhi1_info, }, .num_resources = ARRAY_SIZE(sdhi1_resources), .resource = sdhi1_resources, -- 1.7.4.4 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/5] mmc: tmio: name 0xd8 as CTL_DMA_ENABLE
This reflects at least the current usage of this register and I think it improves the readability of the code ever so slightly. Cc: Guennadi Liakhovetski g.liakhovet...@gmx.de Cc: Magnus Damm magnus.d...@gmail.com Signed-off-by: Simon Horman ho...@verge.net.au --- Dependencies: none known --- drivers/mmc/host/tmio_mmc_dma.c |2 +- include/linux/mmc/tmio.h|1 + 2 files changed, 2 insertions(+), 1 deletions(-) diff --git a/drivers/mmc/host/tmio_mmc_dma.c b/drivers/mmc/host/tmio_mmc_dma.c index 25f1ad6..9c4da66 100644 --- a/drivers/mmc/host/tmio_mmc_dma.c +++ b/drivers/mmc/host/tmio_mmc_dma.c @@ -26,7 +26,7 @@ static void tmio_mmc_enable_dma(struct tmio_mmc_host *host, bool enable) { #if defined(CONFIG_SUPERH) || defined(CONFIG_ARCH_SHMOBILE) /* Switch DMA mode on or off - SuperH specific? */ - writew(enable ? 2 : 0, host-ctl + (0xd8 host-bus_shift)); + writew(enable ? 2 : 0, host-ctl + (CTL_DMA_ENABLE host-bus_shift)); #endif } diff --git a/include/linux/mmc/tmio.h b/include/linux/mmc/tmio.h index 19490b9..0a6e40d 100644 --- a/include/linux/mmc/tmio.h +++ b/include/linux/mmc/tmio.h @@ -30,6 +30,7 @@ #define CTL_TRANSACTION_CTL 0x34 #define CTL_SDIO_STATUS 0x36 #define CTL_SDIO_IRQ_MASK 0x38 +#define CTL_DMA_ENABLE 0xd8 #define CTL_RESET_SD 0xe0 #define CTL_SDIO_REGS 0x100 #define CTL_CLK_AND_WAIT_CTL 0x138 -- 1.7.4.4 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/5] mmc: tmio: Share register access functions
Move register access functions into a shared header. Use sd_ctrl_write16 in tmio_mmc_dma.c:tmio_mmc_enable_dma(). Other than avoiding (trivial) open-coding, the motivation for this is to allow platform-hooks in access functions to be applied across all applicable accesses. Cc: Guennadi Liakhovetski g.liakhovet...@gmx.de Cc: Magnus Damm magnus.d...@gmail.com Signed-off-by: Simon Horman ho...@verge.net.au --- Dependencies: mmc: tmio: name 0xd8 as CTL_DMA_ENABLE --- drivers/mmc/host/tmio_mmc.h | 35 +++ drivers/mmc/host/tmio_mmc_dma.c |2 +- drivers/mmc/host/tmio_mmc_pio.c | 34 -- 3 files changed, 36 insertions(+), 35 deletions(-) diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h index 8260bc2..0c22df0 100644 --- a/drivers/mmc/host/tmio_mmc.h +++ b/drivers/mmc/host/tmio_mmc.h @@ -134,4 +134,39 @@ int tmio_mmc_host_resume(struct device *dev); int tmio_mmc_host_runtime_suspend(struct device *dev); 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)); +} + +static inline void sd_ctrl_read16_rep(struct tmio_mmc_host *host, int addr, + u16 *buf, int count) +{ + readsw(host-ctl + (addr host-bus_shift), buf, count); +} + +static inline u32 sd_ctrl_read32(struct tmio_mmc_host *host, int addr) +{ + return readw(host-ctl + (addr host-bus_shift)) | + readw(host-ctl + ((addr + 2) host-bus_shift)) 16; +} + +static inline void sd_ctrl_write16(struct tmio_mmc_host *host, int addr, u16 val) +{ + writew(val, host-ctl + (addr host-bus_shift)); +} + +static inline void sd_ctrl_write16_rep(struct tmio_mmc_host *host, int addr, + u16 *buf, int count) +{ + writesw(host-ctl + (addr host-bus_shift), buf, count); +} + +static inline void sd_ctrl_write32(struct tmio_mmc_host *host, int addr, u32 val) +{ + writew(val, host-ctl + (addr host-bus_shift)); + writew(val 16, host-ctl + ((addr + 2) host-bus_shift)); +} + + #endif diff --git a/drivers/mmc/host/tmio_mmc_dma.c b/drivers/mmc/host/tmio_mmc_dma.c index 9c4da66..f24a029 100644 --- a/drivers/mmc/host/tmio_mmc_dma.c +++ b/drivers/mmc/host/tmio_mmc_dma.c @@ -26,7 +26,7 @@ static void tmio_mmc_enable_dma(struct tmio_mmc_host *host, bool enable) { #if defined(CONFIG_SUPERH) || defined(CONFIG_ARCH_SHMOBILE) /* Switch DMA mode on or off - SuperH specific? */ - writew(enable ? 2 : 0, host-ctl + (CTL_DMA_ENABLE host-bus_shift)); + sd_ctrl_write16(host, enable ? 2 : 0, CTL_DMA_ENABLE); #endif } diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c index ad6347b..105f720 100644 --- a/drivers/mmc/host/tmio_mmc_pio.c +++ b/drivers/mmc/host/tmio_mmc_pio.c @@ -46,40 +46,6 @@ #include tmio_mmc.h -static u16 sd_ctrl_read16(struct tmio_mmc_host *host, int addr) -{ - return readw(host-ctl + (addr host-bus_shift)); -} - -static void sd_ctrl_read16_rep(struct tmio_mmc_host *host, int addr, - u16 *buf, int count) -{ - readsw(host-ctl + (addr host-bus_shift), buf, count); -} - -static u32 sd_ctrl_read32(struct tmio_mmc_host *host, int addr) -{ - return readw(host-ctl + (addr host-bus_shift)) | - readw(host-ctl + ((addr + 2) host-bus_shift)) 16; -} - -static void sd_ctrl_write16(struct tmio_mmc_host *host, int addr, u16 val) -{ - writew(val, host-ctl + (addr host-bus_shift)); -} - -static void sd_ctrl_write16_rep(struct tmio_mmc_host *host, int addr, - u16 *buf, int count) -{ - writesw(host-ctl + (addr host-bus_shift), buf, count); -} - -static void sd_ctrl_write32(struct tmio_mmc_host *host, int addr, u32 val) -{ - writew(val, host-ctl + (addr host-bus_shift)); - writew(val 16, host-ctl + ((addr + 2) host-bus_shift)); -} - void tmio_mmc_enable_mmc_irqs(struct tmio_mmc_host *host, u32 i) { u32 mask = sd_ctrl_read32(host, CTL_IRQ_MASK) ~(i TMIO_MASK_IRQ); -- 1.7.4.4 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] mmc: sdhi: Add write16_hook
On Tue, Jun 21, 2011 at 09:36:03AM +0900, Magnus Damm wrote: On Tue, Jun 21, 2011 at 8:00 AM, Simon Horman ho...@verge.net.au wrote: Some controllers require waiting for the bus to become idle before writing to some registers. I have implemented this by adding a hook to sd_ctrl_write16() and implementing a hook for SDHI which waits for the bus to become idle. Cc: Guennadi Liakhovetski g.liakhovet...@gmx.de Cc: Magnus Damm magnus.d...@gmail.com Signed-off-by: Simon Horman ho...@verge.net.au --- Dependencies: mmc: tmio: Share register access functions v2: * Include linux/delay.h instead of asm/delay.h * Skip write if sh_mobile_sdhi_wait_idle() times out - The bus will probably be in an inconsistent state and writing may lock up the bus * Only set hook if TMIO_MMC_HAS_IDLE_WAIT is set in platform data rather than checking for TMIO_MMC_HAS_IDLE_WAIT each time the hook is called. --- Thanks Simon, this version looks much better! index 5a90266..0dc9804 100644 --- a/include/linux/mfd/tmio.h +++ b/include/linux/mfd/tmio.h @@ -94,6 +101,7 @@ struct tmio_mmc_data { void (*set_pwr)(struct platform_device *host, int state); void (*set_clk_div)(struct platform_device *host, int state); int (*get_cd)(struct platform_device *host); + int (*write16_hook)(struct tmio_mmc_host *host, int addr); }; static inline void tmio_mmc_cd_wakeup(struct tmio_mmc_data *pdata) What's the reason behind passing struct tmio_mmc_host * as an argument to the new hook? Performance? All other callbacks seem to take a struct platform_device *, so being consistent here may be good unless it comes with too much overhead. The reason is that 1) The hook is called from sd_ctrl_write16 which takes struct tmio_mmc_host * as its first argument and; 2) The hook that has been implemented calls sd_ctrl_read16() which takes a struct tmio_mmc_host * as its first argument. So it seemed logical to pass that down. In the caes of 1) we can get the struct platform_device * using host-pdev. However, in the case of 2) is it less clear to me how we can get the struct tmio_mmc_host * from a struct platform_device *. -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] mmc: sdhi: Add write16_hook
On Tue, Jun 21, 2011 at 9:50 AM, Simon Horman ho...@verge.net.au wrote: On Tue, Jun 21, 2011 at 09:36:03AM +0900, Magnus Damm wrote: On Tue, Jun 21, 2011 at 8:00 AM, Simon Horman ho...@verge.net.au wrote: Some controllers require waiting for the bus to become idle before writing to some registers. I have implemented this by adding a hook to sd_ctrl_write16() and implementing a hook for SDHI which waits for the bus to become idle. Cc: Guennadi Liakhovetski g.liakhovet...@gmx.de Cc: Magnus Damm magnus.d...@gmail.com Signed-off-by: Simon Horman ho...@verge.net.au --- Dependencies: mmc: tmio: Share register access functions v2: * Include linux/delay.h instead of asm/delay.h * Skip write if sh_mobile_sdhi_wait_idle() times out - The bus will probably be in an inconsistent state and writing may lock up the bus * Only set hook if TMIO_MMC_HAS_IDLE_WAIT is set in platform data rather than checking for TMIO_MMC_HAS_IDLE_WAIT each time the hook is called. --- Thanks Simon, this version looks much better! index 5a90266..0dc9804 100644 --- a/include/linux/mfd/tmio.h +++ b/include/linux/mfd/tmio.h @@ -94,6 +101,7 @@ struct tmio_mmc_data { void (*set_pwr)(struct platform_device *host, int state); void (*set_clk_div)(struct platform_device *host, int state); int (*get_cd)(struct platform_device *host); + int (*write16_hook)(struct tmio_mmc_host *host, int addr); }; static inline void tmio_mmc_cd_wakeup(struct tmio_mmc_data *pdata) What's the reason behind passing struct tmio_mmc_host * as an argument to the new hook? Performance? All other callbacks seem to take a struct platform_device *, so being consistent here may be good unless it comes with too much overhead. The reason is that 1) The hook is called from sd_ctrl_write16 which takes struct tmio_mmc_host * as its first argument and; 2) The hook that has been implemented calls sd_ctrl_read16() which takes a struct tmio_mmc_host * as its first argument. So it seemed logical to pass that down. In the caes of 1) we can get the struct platform_device * using host-pdev. However, in the case of 2) is it less clear to me how we can get the struct tmio_mmc_host * from a struct platform_device *. Have a look at the code in tmio_mmc_host_suspend() for some code that does struct device * - struct tmio_mmc_host *: int tmio_mmc_host_suspend(struct device *dev) { struct mmc_host *mmc = dev_get_drvdata(dev); struct tmio_mmc_host *host = mmc_priv(mmc); You can easily change the dev_get_drvdata() to platform_get_drvdata(), see include/linux/platform_device.h I guess a similar conversion can be done in tmio_mmc_enable_dma() to move from writew() to sd_ctrl_write16()? Cheers, / magnus -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] mmc: sdhi: Add write16_hook
On Tue, Jun 21, 2011 at 09:59:37AM +0900, Magnus Damm wrote: On Tue, Jun 21, 2011 at 9:50 AM, Simon Horman ho...@verge.net.au wrote: On Tue, Jun 21, 2011 at 09:36:03AM +0900, Magnus Damm wrote: On Tue, Jun 21, 2011 at 8:00 AM, Simon Horman ho...@verge.net.au wrote: [snip] index 5a90266..0dc9804 100644 --- a/include/linux/mfd/tmio.h +++ b/include/linux/mfd/tmio.h @@ -94,6 +101,7 @@ struct tmio_mmc_data { void (*set_pwr)(struct platform_device *host, int state); void (*set_clk_div)(struct platform_device *host, int state); int (*get_cd)(struct platform_device *host); + int (*write16_hook)(struct tmio_mmc_host *host, int addr); }; static inline void tmio_mmc_cd_wakeup(struct tmio_mmc_data *pdata) What's the reason behind passing struct tmio_mmc_host * as an argument to the new hook? Performance? All other callbacks seem to take a struct platform_device *, so being consistent here may be good unless it comes with too much overhead. The reason is that 1) The hook is called from sd_ctrl_write16 which takes struct tmio_mmc_host * as its first argument and; 2) The hook that has been implemented calls sd_ctrl_read16() which takes a struct tmio_mmc_host * as its first argument. So it seemed logical to pass that down. In the caes of 1) we can get the struct platform_device * using host-pdev. However, in the case of 2) is it less clear to me how we can get the struct tmio_mmc_host * from a struct platform_device *. Have a look at the code in tmio_mmc_host_suspend() for some code that does struct device * - struct tmio_mmc_host *: int tmio_mmc_host_suspend(struct device *dev) { struct mmc_host *mmc = dev_get_drvdata(dev); struct tmio_mmc_host *host = mmc_priv(mmc); You can easily change the dev_get_drvdata() to platform_get_drvdata(), see include/linux/platform_device.h Thanks, I'm happy to make that change if you think it is worth it. (I will need to re-test on AG5, which I could do this afternoon if it is free) I guess a similar conversion can be done in tmio_mmc_enable_dma() to move from writew() to sd_ctrl_write16()? Are you proposing changing tmio_mmc_enable_dma() to take a struct platform_device * as its first argument? tmio_mmc_enable_dma() is already altered in one of the patches in this series to use sd_ctrl_write16() without altering the arguments taht tmio_mmc_enable_dma() takes. static void tmio_mmc_enable_dma(struct tmio_mmc_host *host, bool enable) { #if defined(CONFIG_SUPERH) || defined(CONFIG_ARCH_SHMOBILE) /* Switch DMA mode on or off - SuperH specific? */ sd_ctrl_write16(host, enable ? 2 : 0, CTL_DMA_ENABLE); #endif } -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/4] mmc: sdhci: fix interrupt storm from card detection
On Mon, Jun 20, 2011 at 08:59:38AM -0700, Philip Rakity wrote: On Jun 20, 2011, at 3:38 AM, Shawn Guo wrote: The issue was initially found by Eric Benard as below. http://permalink.gmane.org/gmane.linux.ports.arm.kernel/108031 Not sure about other SDHCI based controller, but on Freescale eSDHC, the SDHCI_INT_CARD_INSERT bits will be immediately set again when it gets cleared, if a card is inserted. The driver need to mask the irq to prevent interrupt storm which will freeze the system. And the SDHCI_INT_CARD_REMOVE gets the same situation. The patch fixes the problem based on the initial idea from Eric Benard. Signed-off-by: Shawn Guo shawn@linaro.org Cc: Eric Benard e...@eukrea.com --- drivers/mmc/host/sdhci.c | 27 +++ 1 files changed, 23 insertions(+), 4 deletions(-) diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 91d9892..d94e2b4 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -127,7 +127,9 @@ static void sdhci_mask_irqs(struct sdhci_host *host, u32 irqs) static void sdhci_set_card_detection(struct sdhci_host *host, bool enable) { - u32 irqs = SDHCI_INT_CARD_REMOVE | SDHCI_INT_CARD_INSERT; + u32 present = sdhci_readl(host, SDHCI_PRESENT_STATE) + SDHCI_CARD_PRESENT; + u32 irqs = present ? SDHCI_INT_CARD_REMOVE : SDHCI_INT_CARD_INSERT; if (host-quirks SDHCI_QUIRK_BROKEN_CARD_DETECTION) return; Accessing off chip registers is slow. It would be better to check for the quirk first and then read the present state register. eg u32 present; u32 irqs; if (host-quirks SDHCI_QUIRK_BROKEN_CARD_DETECTION) return; present = sdhci_readl(host, SDHCI_PRESENT_STATE) SDHCI_CARD_PRESENT; irqs = present ? SDHCI_INT_CARD_REMOVE : SDHCI_INT_CARD_INSERT; Makes sense. Will do. -- Regards, Shawn -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/4] Extend sdhci-esdhc-imx card_detect and write_protect support for mx5
Hi Arnaud, Would you please give a test on the series, as it fixed the issue you reported? TIA. -- Regards, Shawn On Mon, Jun 20, 2011 at 06:38:41PM +0800, Shawn Guo wrote: The card-present polling for sdhci based driver is very expensive in terms of the impact to system performance. We observe a few system performance issues from Freescale and Linaro on mx5 platforms, which have been proved card polling related. The patch set extends the current sdhci-esdhc-imx card_detect and write_protect support to cover mx5 platforms, and solves above performance issues. Changes since v2: * Fix the issue reported by Arnaud Patard: http://article.gmane.org/gmane.linux.ports.arm.kernel/120790 Changes since v1: * Rebase on today's linux-next * Take the suggestion from Arnaud Patard to add default pdata in imx_add_sdhci_esdhc_imx(), to avoid touching every single board file for the platform_data changes * Add comment for sdhci.c change * Change ESDHC_CD(WP)_SIGNAL to ESDHC_CD(WP)_CONTROLLER for a more descriptive name * Add missing NONE case handling in esdhc_pltfm_get_ro * Improve a couple comment wording per suggestion from Wolfram Sang Shawn Guo (4): mmc: sdhci: fix interrupt storm from card detection mmc: sdhci-esdhc-imx: SDHCI_CARD_PRESENT does not get cleared mmc: sdhci-esdhc-imx: remove WP from flag ESDHC_FLAG_GPIO_FOR_CD_WP mmc: sdhci-esdhc-imx: extend card_detect and write_protect support for mx5 arch/arm/mach-imx/eukrea_mbimxsd25-baseboard.c |3 +- arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c |3 +- arch/arm/mach-imx/mach-mx25_3ds.c |2 + arch/arm/mach-imx/mach-pcm043.c|2 + arch/arm/mach-mx5/board-mx51_babbage.c | 14 +- arch/arm/mach-mx5/board-mx53_loco.c|4 + .../plat-mxc/devices/platform-sdhci-esdhc-imx.c| 12 ++ arch/arm/plat-mxc/include/mach/esdhc.h | 25 +++- drivers/mmc/host/sdhci-esdhc-imx.c | 136 drivers/mmc/host/sdhci.c | 27 - 10 files changed, 161 insertions(+), 67 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] mmc: sdhi: Add write16_hook
Hi Simon, On Tue, Jun 21, 2011 at 10:13 AM, Simon Horman ho...@verge.net.au wrote: On Tue, Jun 21, 2011 at 09:59:37AM +0900, Magnus Damm wrote: On Tue, Jun 21, 2011 at 9:50 AM, Simon Horman ho...@verge.net.au wrote: On Tue, Jun 21, 2011 at 09:36:03AM +0900, Magnus Damm wrote: On Tue, Jun 21, 2011 at 8:00 AM, Simon Horman ho...@verge.net.au wrote: [snip] index 5a90266..0dc9804 100644 --- a/include/linux/mfd/tmio.h +++ b/include/linux/mfd/tmio.h @@ -94,6 +101,7 @@ struct tmio_mmc_data { void (*set_pwr)(struct platform_device *host, int state); void (*set_clk_div)(struct platform_device *host, int state); int (*get_cd)(struct platform_device *host); + int (*write16_hook)(struct tmio_mmc_host *host, int addr); }; static inline void tmio_mmc_cd_wakeup(struct tmio_mmc_data *pdata) What's the reason behind passing struct tmio_mmc_host * as an argument to the new hook? Performance? All other callbacks seem to take a struct platform_device *, so being consistent here may be good unless it comes with too much overhead. The reason is that 1) The hook is called from sd_ctrl_write16 which takes struct tmio_mmc_host * as its first argument and; 2) The hook that has been implemented calls sd_ctrl_read16() which takes a struct tmio_mmc_host * as its first argument. So it seemed logical to pass that down. In the caes of 1) we can get the struct platform_device * using host-pdev. However, in the case of 2) is it less clear to me how we can get the struct tmio_mmc_host * from a struct platform_device *. Have a look at the code in tmio_mmc_host_suspend() for some code that does struct device * - struct tmio_mmc_host *: int tmio_mmc_host_suspend(struct device *dev) { struct mmc_host *mmc = dev_get_drvdata(dev); struct tmio_mmc_host *host = mmc_priv(mmc); You can easily change the dev_get_drvdata() to platform_get_drvdata(), see include/linux/platform_device.h Thanks, I'm happy to make that change if you think it is worth it. (I will need to re-test on AG5, which I could do this afternoon if it is free) Hm, perhaps it can be done with incremental patches in the future? I think it's good to be consistent and use the same argument passing style as other callbacks, but at the same time I'm not 100% sure if passing a platform data pointer is the best approach. It probably made sense with the old tmio_mmc driver that only hooked up to MFD, but I'm not sure if that's the case anymore. I'm sure there is room for plenty of cleanups - but exactly what to do I don't know. =) At least passing a struct tmio_mmc_host * requires little conversion which should add minimal overhead. I guess a similar conversion can be done in tmio_mmc_enable_dma() to move from writew() to sd_ctrl_write16()? Are you proposing changing tmio_mmc_enable_dma() to take a struct platform_device * as its first argument? No, not at all. I just recall someone pointing out that tmio_mmc_enable_dma() skipped the tmio_mmc specific I/O routines and used writew() directly. I suspected the reason behind this was the difficulty of converting between different pointer types, but that may not be true. tmio_mmc_enable_dma() is already altered in one of the patches in this series to use sd_ctrl_write16() without altering the arguments taht tmio_mmc_enable_dma() takes. Ok, that's good. static void tmio_mmc_enable_dma(struct tmio_mmc_host *host, bool enable) { #if defined(CONFIG_SUPERH) || defined(CONFIG_ARCH_SHMOBILE) /* Switch DMA mode on or off - SuperH specific? */ sd_ctrl_write16(host, enable ? 2 : 0, CTL_DMA_ENABLE); #endif } Hm, perhaps it's my mail setup that's the issue, but the version of [PATCH 1/5] mmc: tmio: name 0xd8 as CTL_DMA_ENABLE that I'm looking at is still using writew(). Cheers, / magnus -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] mmc: sdhi: Add write16_hook
On Tue, Jun 21, 2011 at 10:36:05AM +0900, Magnus Damm wrote: Hi Simon, On Tue, Jun 21, 2011 at 10:13 AM, Simon Horman ho...@verge.net.au wrote: On Tue, Jun 21, 2011 at 09:59:37AM +0900, Magnus Damm wrote: On Tue, Jun 21, 2011 at 9:50 AM, Simon Horman ho...@verge.net.au wrote: On Tue, Jun 21, 2011 at 09:36:03AM +0900, Magnus Damm wrote: On Tue, Jun 21, 2011 at 8:00 AM, Simon Horman ho...@verge.net.au wrote: [snip] index 5a90266..0dc9804 100644 --- a/include/linux/mfd/tmio.h +++ b/include/linux/mfd/tmio.h @@ -94,6 +101,7 @@ struct tmio_mmc_data { void (*set_pwr)(struct platform_device *host, int state); void (*set_clk_div)(struct platform_device *host, int state); int (*get_cd)(struct platform_device *host); + int (*write16_hook)(struct tmio_mmc_host *host, int addr); }; static inline void tmio_mmc_cd_wakeup(struct tmio_mmc_data *pdata) What's the reason behind passing struct tmio_mmc_host * as an argument to the new hook? Performance? All other callbacks seem to take a struct platform_device *, so being consistent here may be good unless it comes with too much overhead. The reason is that 1) The hook is called from sd_ctrl_write16 which takes struct tmio_mmc_host * as its first argument and; 2) The hook that has been implemented calls sd_ctrl_read16() which takes a struct tmio_mmc_host * as its first argument. So it seemed logical to pass that down. In the caes of 1) we can get the struct platform_device * using host-pdev. However, in the case of 2) is it less clear to me how we can get the struct tmio_mmc_host * from a struct platform_device *. Have a look at the code in tmio_mmc_host_suspend() for some code that does struct device * - struct tmio_mmc_host *: int tmio_mmc_host_suspend(struct device *dev) { struct mmc_host *mmc = dev_get_drvdata(dev); struct tmio_mmc_host *host = mmc_priv(mmc); You can easily change the dev_get_drvdata() to platform_get_drvdata(), see include/linux/platform_device.h Thanks, I'm happy to make that change if you think it is worth it. (I will need to re-test on AG5, which I could do this afternoon if it is free) Hm, perhaps it can be done with incremental patches in the future? I think it's good to be consistent and use the same argument passing style as other callbacks, but at the same time I'm not 100% sure if passing a platform data pointer is the best approach. It probably made sense with the old tmio_mmc driver that only hooked up to MFD, but I'm not sure if that's the case anymore. I'm sure there is room for plenty of cleanups - but exactly what to do I don't know. =) At least passing a struct tmio_mmc_host * requires little conversion which should add minimal overhead. Ok, lets stick with what we have for now. Its a fairly trivial change to update the arguments later if that is wanted. Testing is slightly less trivial due to availability of hardware, but not a major problem. Can you Acked-by, or Reviewed-by the patches in this series if you are now happy with them? I guess a similar conversion can be done in tmio_mmc_enable_dma() to move from writew() to sd_ctrl_write16()? Are you proposing changing tmio_mmc_enable_dma() to take a struct platform_device * as its first argument? No, not at all. I just recall someone pointing out that tmio_mmc_enable_dma() skipped the tmio_mmc specific I/O routines and used writew() directly. I suspected the reason behind this was the difficulty of converting between different pointer types, but that may not be true. I was probably the person who pointed that out to you. I'm unsure of the reason, but at least in its current form it appears not to be related to the parameters involved. tmio_mmc_enable_dma() is already altered in one of the patches in this series to use sd_ctrl_write16() without altering the arguments taht tmio_mmc_enable_dma() takes. Ok, that's good. static void tmio_mmc_enable_dma(struct tmio_mmc_host *host, bool enable) { #if defined(CONFIG_SUPERH) || defined(CONFIG_ARCH_SHMOBILE) /* Switch DMA mode on or off - SuperH specific? */ sd_ctrl_write16(host, enable ? 2 : 0, CTL_DMA_ENABLE); #endif } Hm, perhaps it's my mail setup that's the issue, but the version of [PATCH 1/5] mmc: tmio: name 0xd8 as CTL_DMA_ENABLE that I'm looking at is still using writew(). The writew() - sd_ctrl_write16() change is made by the following patch in the series, [PATCH 2/5] mmc: tmio: Share register access functions. The last hunk looks like this: diff --git a/drivers/mmc/host/tmio_mmc_dma.c b/drivers/mmc/host/tmio_mmc_dma.c index 9c4da66..f24a029 100644 --- a/drivers/mmc/host/tmio_mmc_dma.c +++ b/drivers/mmc/host/tmio_mmc_dma.c @@ -26,7 +26,7 @@ static void tmio_mmc_enable_dma(struct tmio_mmc_host *host, bool enable) { #if defined(CONFIG_SUPERH) ||
Re: [PATCH 3/5] mmc: sdhi: Add write16_hook
Hey Simon, On Tue, Jun 21, 2011 at 11:09 AM, Simon Horman ho...@verge.net.au wrote: On Tue, Jun 21, 2011 at 10:36:05AM +0900, Magnus Damm wrote: On Tue, Jun 21, 2011 at 10:13 AM, Simon Horman ho...@verge.net.au wrote: On Tue, Jun 21, 2011 at 09:59:37AM +0900, Magnus Damm wrote: On Tue, Jun 21, 2011 at 9:50 AM, Simon Horman ho...@verge.net.au wrote: On Tue, Jun 21, 2011 at 09:36:03AM +0900, Magnus Damm wrote: On Tue, Jun 21, 2011 at 8:00 AM, Simon Horman ho...@verge.net.au wrote: [snip] index 5a90266..0dc9804 100644 --- a/include/linux/mfd/tmio.h +++ b/include/linux/mfd/tmio.h @@ -94,6 +101,7 @@ struct tmio_mmc_data { void (*set_pwr)(struct platform_device *host, int state); void (*set_clk_div)(struct platform_device *host, int state); int (*get_cd)(struct platform_device *host); + int (*write16_hook)(struct tmio_mmc_host *host, int addr); }; static inline void tmio_mmc_cd_wakeup(struct tmio_mmc_data *pdata) What's the reason behind passing struct tmio_mmc_host * as an argument to the new hook? Performance? All other callbacks seem to take a struct platform_device *, so being consistent here may be good unless it comes with too much overhead. The reason is that 1) The hook is called from sd_ctrl_write16 which takes struct tmio_mmc_host * as its first argument and; 2) The hook that has been implemented calls sd_ctrl_read16() which takes a struct tmio_mmc_host * as its first argument. So it seemed logical to pass that down. In the caes of 1) we can get the struct platform_device * using host-pdev. However, in the case of 2) is it less clear to me how we can get the struct tmio_mmc_host * from a struct platform_device *. Have a look at the code in tmio_mmc_host_suspend() for some code that does struct device * - struct tmio_mmc_host *: int tmio_mmc_host_suspend(struct device *dev) { struct mmc_host *mmc = dev_get_drvdata(dev); struct tmio_mmc_host *host = mmc_priv(mmc); You can easily change the dev_get_drvdata() to platform_get_drvdata(), see include/linux/platform_device.h Thanks, I'm happy to make that change if you think it is worth it. (I will need to re-test on AG5, which I could do this afternoon if it is free) Hm, perhaps it can be done with incremental patches in the future? I think it's good to be consistent and use the same argument passing style as other callbacks, but at the same time I'm not 100% sure if passing a platform data pointer is the best approach. It probably made sense with the old tmio_mmc driver that only hooked up to MFD, but I'm not sure if that's the case anymore. I'm sure there is room for plenty of cleanups - but exactly what to do I don't know. =) At least passing a struct tmio_mmc_host * requires little conversion which should add minimal overhead. Ok, lets stick with what we have for now. Its a fairly trivial change to update the arguments later if that is wanted. Testing is slightly less trivial due to availability of hardware, but not a major problem. Can you Acked-by, or Reviewed-by the patches in this series if you are now happy with them? Sure, for all patches included in this series: Acked-by: Magnus Damm d...@opensource.se I guess a similar conversion can be done in tmio_mmc_enable_dma() to move from writew() to sd_ctrl_write16()? Are you proposing changing tmio_mmc_enable_dma() to take a struct platform_device * as its first argument? No, not at all. I just recall someone pointing out that tmio_mmc_enable_dma() skipped the tmio_mmc specific I/O routines and used writew() directly. I suspected the reason behind this was the difficulty of converting between different pointer types, but that may not be true. I was probably the person who pointed that out to you. I'm unsure of the reason, but at least in its current form it appears not to be related to the parameters involved. Yeah. Probably me being confused of which patches that modify which functions. =) tmio_mmc_enable_dma() is already altered in one of the patches in this series to use sd_ctrl_write16() without altering the arguments taht tmio_mmc_enable_dma() takes. Ok, that's good. static void tmio_mmc_enable_dma(struct tmio_mmc_host *host, bool enable) { #if defined(CONFIG_SUPERH) || defined(CONFIG_ARCH_SHMOBILE) /* Switch DMA mode on or off - SuperH specific? */ sd_ctrl_write16(host, enable ? 2 : 0, CTL_DMA_ENABLE); #endif } Hm, perhaps it's my mail setup that's the issue, but the version of [PATCH 1/5] mmc: tmio: name 0xd8 as CTL_DMA_ENABLE that I'm looking at is still using writew(). The writew() - sd_ctrl_write16() change is made by the following patch in the series, [PATCH 2/5] mmc: tmio: Share register access functions. The last hunk looks like this: diff --git a/drivers/mmc/host/tmio_mmc_dma.c
[PATCH] mmc: sdhci-pxa move platform data to include/linux/platform_data
resend: As suggested by Arnd, move platform data to include/linux/platform_data. Add 'depends on CLKDEV_LOOKUP' since clk API is used, As a result driver could be built on all platforms which support CLKDEV_LOOKUP, which can improve the build test coverage. CC: Arnd Bergmann a...@arndb.de CC: Stephen Rothwell s...@canb.auug.org.au Signed-off-by: Zhangfei Gao zhangfei@marvell.com Acked-by: Arnd Bergmann a...@arndb.de --- arch/arm/mach-mmp/include/mach/mmp2.h |2 +- drivers/mmc/host/Kconfig |2 ++ drivers/mmc/host/sdhci-pxav2.c |2 +- drivers/mmc/host/sdhci-pxav3.c |2 +- .../linux/platform_data/pxa_sdhci.h| 10 +- 5 files changed, 10 insertions(+), 8 deletions(-) rename arch/arm/plat-pxa/include/plat/sdhci.h = include/linux/platform_data/pxa_sdhci.h (92%) diff --git a/arch/arm/mach-mmp/include/mach/mmp2.h b/arch/arm/mach-mmp/include/mach/mmp2.h index 2cbf6df..de7b888 100644 --- a/arch/arm/mach-mmp/include/mach/mmp2.h +++ b/arch/arm/mach-mmp/include/mach/mmp2.h @@ -1,7 +1,7 @@ #ifndef __ASM_MACH_MMP2_H #define __ASM_MACH_MMP2_H -#include plat/sdhci.h +#include linux/platform_data/pxa_sdhci.h struct sys_timer; diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig index 198ddda..204ad7c 100644 --- a/drivers/mmc/host/Kconfig +++ b/drivers/mmc/host/Kconfig @@ -183,6 +183,7 @@ config MMC_SDHCI_S3C config MMC_SDHCI_PXAV3 tristate Marvell MMP2 SD Host Controller support (PXAV3) + depends on CLKDEV_LOOKUP select MMC_SDHCI select MMC_SDHCI_PLTFM default CPU_MMP2 @@ -195,6 +196,7 @@ config MMC_SDHCI_PXAV3 config MMC_SDHCI_PXAV2 tristate Marvell PXA9XX SD Host Controller support (PXAV2) + depends on CLKDEV_LOOKUP select MMC_SDHCI select MMC_SDHCI_PLTFM default CPU_PXA910 diff --git a/drivers/mmc/host/sdhci-pxav2.c b/drivers/mmc/host/sdhci-pxav2.c index 7a6fa8c..38f5899 100644 --- a/drivers/mmc/host/sdhci-pxav2.c +++ b/drivers/mmc/host/sdhci-pxav2.c @@ -25,7 +25,7 @@ #include linux/gpio.h #include linux/mmc/card.h #include linux/mmc/host.h -#include plat/sdhci.h +#include linux/platform_data/pxa_sdhci.h #include linux/slab.h #include sdhci.h #include sdhci-pltfm.h diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c index 901f00f..4198dbb 100644 --- a/drivers/mmc/host/sdhci-pxav3.c +++ b/drivers/mmc/host/sdhci-pxav3.c @@ -24,7 +24,7 @@ #include linux/gpio.h #include linux/mmc/card.h #include linux/mmc/host.h -#include plat/sdhci.h +#include linux/platform_data/pxa_sdhci.h #include linux/slab.h #include linux/delay.h #include sdhci.h diff --git a/arch/arm/plat-pxa/include/plat/sdhci.h b/include/linux/platform_data/pxa_sdhci.h similarity index 92% rename from arch/arm/plat-pxa/include/plat/sdhci.h rename to include/linux/platform_data/pxa_sdhci.h index 800ebc1..51ad099 100644 --- a/arch/arm/plat-pxa/include/plat/sdhci.h +++ b/include/linux/platform_data/pxa_sdhci.h @@ -1,4 +1,5 @@ -/* linux/arch/arm/plat-pxa/include/plat/sdhci.h +/* + * include/linux/platform_data/pxa_sdhci.h * * Copyright 2010 Marvell * Zhangfei Gao zhangfei@marvell.com @@ -10,8 +11,8 @@ * published by the Free Software Foundation. */ -#ifndef __PLAT_PXA_SDHCI_H -#define __PLAT_PXA_SDHCI_H +#ifndef _PXA_SDHCI_H_ +#define _PXA_SDHCI_H_ /* pxa specific flag */ /* Require clock free running */ @@ -56,5 +57,4 @@ struct sdhci_pxa { u8 clk_enable; u8 power_mode; }; - -#endif /* __PLAT_PXA_SDHCI_H */ +#endif /* _PXA_SDHCI_H_ */ -- 1.7.0.4 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 02/11] omap_hsmmc: add support for pre_req and post_req
snip + +static void omap_hsmmc_pre_req(struct mmc_host *mmc, struct mmc_request *mrq, + bool is_first_req) I don't see the usage of is_first_req below. Is it required? +{ + struct omap_hsmmc_host *host = mmc_priv(mmc); + + if (mrq-data-host_cookie) { + mrq-data-host_cookie = 0; + return ; + } + + if (host-use_dma) + if (omap_hsmmc_pre_dma_transfer(host, mrq-data, + host-next_data)) + mrq-data-host_cookie = 0; +} + /* Regards, Kishore -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html