| Message ID | 1308610812-3479-4-git-send-email-horms@verge.net.au (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Jun 21, 2011 at 8:00 AM, Simon Horman <horms@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.liakhovetski@gmx.de> > Cc: Magnus Damm <magnus.damm@gmail.com> > Signed-off-by: Simon Horman <horms@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. Thanks, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 21, 2011 at 09:36:03AM +0900, Magnus Damm wrote: > On Tue, Jun 21, 2011 at 8:00 AM, Simon Horman <horms@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.liakhovetski@gmx.de> > > Cc: Magnus Damm <magnus.damm@gmail.com> > > Signed-off-by: Simon Horman <horms@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-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 21, 2011 at 9:50 AM, Simon Horman <horms@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 <horms@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.liakhovetski@gmx.de> >> > Cc: Magnus Damm <magnus.damm@gmail.com> >> > Signed-off-by: Simon Horman <horms@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-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 21, 2011 at 09:59:37AM +0900, Magnus Damm wrote: > On Tue, Jun 21, 2011 at 9:50 AM, Simon Horman <horms@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 <horms@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-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Simon, On Tue, Jun 21, 2011 at 10:13 AM, Simon Horman <horms@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 <horms@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 <horms@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-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c index b365429..c8b1f6b 100644 --- a/drivers/mmc/host/sh_mobile_sdhi.c +++ b/drivers/mmc/host/sh_mobile_sdhi.c @@ -26,6 +26,7 @@ #include <linux/mmc/sh_mobile_sdhi.h> #include <linux/mfd/tmio.h> #include <linux/sh_dma.h> +#include <linux/delay.h> #include "tmio_mmc.h" @@ -55,6 +56,39 @@ static int sh_mobile_sdhi_get_cd(struct platform_device *pdev) return -ENOSYS; } +static int 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"); + return -EBUSY; + } + + return 0; +} + +static int sh_mobile_sdhi_write16_hook(struct tmio_mmc_host *host, int addr) +{ + 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: + return sh_mobile_sdhi_wait_idle(host); + } + + return 0; +} + static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev) { struct sh_mobile_sdhi *priv; @@ -86,6 +120,8 @@ 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; + if (mmc_data->flags & TMIO_MMC_HAS_IDLE_WAIT) + mmc_data->write16_hook = sh_mobile_sdhi_write16_hook; mmc_data->capabilities = MMC_CAP_MMC_HIGHSPEED; if (p) { mmc_data->flags = p->tmio_flags; diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h index 0c22df0..211ef6e 100644 --- a/drivers/mmc/host/tmio_mmc.h +++ b/drivers/mmc/host/tmio_mmc.h @@ -153,6 +153,11 @@ 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 there is a hook and it returns non-zero then there + * is an error and the write should be skipped + */ + if (host->pdata->write16_hook && host->pdata->write16_hook(host, addr)) + return; writew(val, host->ctl + (addr << host->bus_shift)); } diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h index 5a90266..0dc9804 100644 --- a/include/linux/mfd/tmio.h +++ b/include/linux/mfd/tmio.h @@ -68,6 +68,11 @@ * controller and report the event to the driver. */ #define TMIO_MMC_HAS_COLD_CD (1 << 3) +/* + * Some controllers require waiting for the SD bus to become + * idle before writing to some registers. + */ +#define TMIO_MMC_HAS_IDLE_WAIT (1 << 4) int tmio_core_mmc_enable(void __iomem *cnf, int shift, unsigned long base); int tmio_core_mmc_resume(void __iomem *cnf, int shift, unsigned long base); @@ -80,6 +85,8 @@ struct tmio_mmc_dma { int alignment_shift; }; +struct tmio_mmc_host; + /* * data for the MMC controller */ @@ -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) diff --git a/include/linux/mmc/tmio.h b/include/linux/mmc/tmio.h index 0a6e40d..f235757 100644 --- a/include/linux/mmc/tmio.h +++ b/include/linux/mmc/tmio.h @@ -21,6 +21,7 @@ #define CTL_XFER_BLK_COUNT 0xa #define CTL_RESPONSE 0x0c #define CTL_STATUS 0x1c +#define CTL_STATUS2 0x1e #define CTL_IRQ_MASK 0x20 #define CTL_SD_CARD_CLK_CTL 0x24 #define CTL_SD_XFER_LEN 0x26
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.liakhovetski@gmx.de> Cc: Magnus Damm <magnus.damm@gmail.com> Signed-off-by: Simon Horman <horms@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. --- drivers/mmc/host/sh_mobile_sdhi.c | 36 ++++++++++++++++++++++++++++++++++++ drivers/mmc/host/tmio_mmc.h | 5 +++++ include/linux/mfd/tmio.h | 8 ++++++++ include/linux/mmc/tmio.h | 1 + 4 files changed, 50 insertions(+), 0 deletions(-)