Message ID | 1431067818-41378-1-git-send-email-yangbo.lu@freescale.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
On Friday 08 May 2015 14:50:18 Yangbo Lu wrote: > -static inline u32 sdhci_be32bs_readl(struct sdhci_host *host, int reg) > +static inline void sdhci_clrsetbits(struct sdhci_host *host, u32 mask, > + u32 val, int reg) > { > - return in_be32(host->ioaddr + reg); > + void __iomem *base = host->ioaddr + (reg & ~0x3); > + u32 shift = (reg & 0x3) * 8; > + > + if (host->mmc->big_endian_mode) > + iowrite32be(((ioread32be(base) & ~(mask << shift)) | > + (val << shift)), base); > + else > + iowrite32(((ioread32(base) & ~(mask << shift)) | > + (val << shift)), base); > } > > I think it would be better to use function pointers that are assigned at probe time than run-time checks here. Have a look at drivers/mmc/host/sdhci-of-hlwd.c: static const struct sdhci_ops sdhci_hlwd_ops = { .read_l = sdhci_be32bs_readl, .read_w = sdhci_be32bs_readw, .read_b = sdhci_be32bs_readb, .write_l = sdhci_hlwd_writel, .write_w = sdhci_hlwd_writew, .write_b = sdhci_hlwd_writeb, .set_clock = sdhci_set_clock, .set_bus_width = sdhci_set_bus_width, .reset = sdhci_reset, .set_uhs_signaling = sdhci_set_uhs_signaling, }; You can copy this method into your own driver. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks. The drivers/mmc/host/sdhci-of-esdhc.c also have same struct. static const struct sdhci_ops sdhci_esdhc_ops = { .read_l = esdhc_readl, .read_w = esdhc_readw, .read_b = esdhc_readb, .write_l = esdhc_writel, .write_w = esdhc_writew, .write_b = esdhc_writeb, .set_clock = esdhc_of_set_clock, .enable_dma = esdhc_of_enable_dma, .get_max_clock = esdhc_of_get_max_clock, .get_min_clock = esdhc_of_get_min_clock, .platform_init = esdhc_of_platform_init, .adma_workaround = esdhci_of_adma_workaround, .set_bus_width = esdhc_pltfm_set_bus_width, .reset = esdhc_reset, .set_uhs_signaling = esdhc_set_uhs_signaling, }; Do you mean defining functions for little-endian and big-endian separately in sdhci-pltm.c? But it seems to need adding much code in drivers/mmc/host/sdhci-of-esdhc.c if don't use run-time checks. -----Original Message----- From: Arnd Bergmann [mailto:arnd@arndb.de] Sent: Friday, May 08, 2015 3:41 PM To: Lu Yangbo-B47093 Cc: linux-mmc@vger.kernel.org; chris@printf.net; ulf.hansson@linaro.org Subject: Re: [v3, 2/6] mmc: sdhci-pltfm: support both LE and BE mode for SDHCI platform On Friday 08 May 2015 14:50:18 Yangbo Lu wrote: > -static inline u32 sdhci_be32bs_readl(struct sdhci_host *host, int > reg) > +static inline void sdhci_clrsetbits(struct sdhci_host *host, u32 mask, > + u32 val, int reg) > { > - return in_be32(host->ioaddr + reg); > + void __iomem *base = host->ioaddr + (reg & ~0x3); > + u32 shift = (reg & 0x3) * 8; > + > + if (host->mmc->big_endian_mode) > + iowrite32be(((ioread32be(base) & ~(mask << shift)) | > + (val << shift)), base); > + else > + iowrite32(((ioread32(base) & ~(mask << shift)) | > + (val << shift)), base); > } > > I think it would be better to use function pointers that are assigned at probe time than run-time checks here. Have a look at drivers/mmc/host/sdhci-of-hlwd.c: static const struct sdhci_ops sdhci_hlwd_ops = { .read_l = sdhci_be32bs_readl, .read_w = sdhci_be32bs_readw, .read_b = sdhci_be32bs_readb, .write_l = sdhci_hlwd_writel, .write_w = sdhci_hlwd_writew, .write_b = sdhci_hlwd_writeb, .set_clock = sdhci_set_clock, .set_bus_width = sdhci_set_bus_width, .reset = sdhci_reset, .set_uhs_signaling = sdhci_set_uhs_signaling, }; You can copy this method into your own driver. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11 May 2015 at 08:46, yangbo.lu@freescale.com <yangbo.lu@freescale.com> wrote: > Thanks. > The drivers/mmc/host/sdhci-of-esdhc.c also have same struct. > static const struct sdhci_ops sdhci_esdhc_ops = { > .read_l = esdhc_readl, > .read_w = esdhc_readw, > .read_b = esdhc_readb, > .write_l = esdhc_writel, > .write_w = esdhc_writew, > .write_b = esdhc_writeb, > .set_clock = esdhc_of_set_clock, > .enable_dma = esdhc_of_enable_dma, > .get_max_clock = esdhc_of_get_max_clock, > .get_min_clock = esdhc_of_get_min_clock, > .platform_init = esdhc_of_platform_init, > .adma_workaround = esdhci_of_adma_workaround, > .set_bus_width = esdhc_pltfm_set_bus_width, > .reset = esdhc_reset, > .set_uhs_signaling = esdhc_set_uhs_signaling, > }; > > Do you mean defining functions for little-endian and big-endian separately in sdhci-pltm.c? > But it seems to need adding much code in drivers/mmc/host/sdhci-of-esdhc.c if don't use run-time checks. On the other hand it will give you an optimized execution path. It's better to check it once in ->probe() once, instead of every time. Kind regards Uffe > > > -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: Friday, May 08, 2015 3:41 PM > To: Lu Yangbo-B47093 > Cc: linux-mmc@vger.kernel.org; chris@printf.net; ulf.hansson@linaro.org > Subject: Re: [v3, 2/6] mmc: sdhci-pltfm: support both LE and BE mode for SDHCI platform > > On Friday 08 May 2015 14:50:18 Yangbo Lu wrote: >> -static inline u32 sdhci_be32bs_readl(struct sdhci_host *host, int >> reg) >> +static inline void sdhci_clrsetbits(struct sdhci_host *host, u32 mask, >> + u32 val, int reg) >> { >> - return in_be32(host->ioaddr + reg); >> + void __iomem *base = host->ioaddr + (reg & ~0x3); >> + u32 shift = (reg & 0x3) * 8; >> + >> + if (host->mmc->big_endian_mode) >> + iowrite32be(((ioread32be(base) & ~(mask << shift)) | >> + (val << shift)), base); >> + else >> + iowrite32(((ioread32(base) & ~(mask << shift)) | >> + (val << shift)), base); >> } >> >> > > I think it would be better to use function pointers that are assigned at probe time than run-time checks here. > > Have a look at drivers/mmc/host/sdhci-of-hlwd.c: > > static const struct sdhci_ops sdhci_hlwd_ops = { > .read_l = sdhci_be32bs_readl, > .read_w = sdhci_be32bs_readw, > .read_b = sdhci_be32bs_readb, > .write_l = sdhci_hlwd_writel, > .write_w = sdhci_hlwd_writew, > .write_b = sdhci_hlwd_writeb, > .set_clock = sdhci_set_clock, > .set_bus_width = sdhci_set_bus_width, > .reset = sdhci_reset, > .set_uhs_signaling = sdhci_set_uhs_signaling, }; > > > You can copy this method into your own driver. > > Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12 May 2015 at 11:53, Lu Y.B. <yangbo.lu@freescale.com> wrote: > Thanks Uffe. > I also think checking every time is not better idea but am not so clear about your method. > I think the plan you wanting should be one of the bellowing, which one is the plan you want? > Plan 1, > Define sdhci_be32bs_readl for big-endian > Define sdhci_le32bs_readl for little-endian > Define a function pointer sdhci_32bs_readl to be assigned when checking endian mode in probe. No, thanks. > Plan 2, > For esdhc, > Define struct sdhci_ops sdhci_esdhc_ops_1 for big-endian > Define struct sdhci_ops sdhci_esdhc_ops_2 for little-endian > Define struct sdhci_ops sdhci_esdhc_ops to be assigned when checking endian mode in probe. This is what I had in mind. Though I now realize that it then doesn't make sense to extend the common MMC DT parser, mmc_of_parse(), to fetch the endian mode. That's because the endian mode needs to be known to pick the correct sdhci_ops, which is the parameter you provide to sdhci_pltfm_init(). Forgive me for pushing you back an forth. So, yes I like you to implement plan2 and thus patch1 should only update the documentation of the DT bindings. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" 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/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c index a207f5a..8b6450e 100644 --- a/drivers/mmc/host/sdhci-pltfm.c +++ b/drivers/mmc/host/sdhci-pltfm.c @@ -224,7 +224,7 @@ int sdhci_pltfm_unregister(struct platform_device *pdev) { struct sdhci_host *host = platform_get_drvdata(pdev); struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); - int dead = (readl(host->ioaddr + SDHCI_INT_STATUS) == 0xffffffff); + int dead = (sdhci_readl(host, SDHCI_INT_STATUS) == 0xffffffff); sdhci_remove_host(host, dead); clk_disable_unprepare(pltfm_host->clk); diff --git a/drivers/mmc/host/sdhci-pltfm.h b/drivers/mmc/host/sdhci-pltfm.h index 04bc248..d377aeb 100644 --- a/drivers/mmc/host/sdhci-pltfm.h +++ b/drivers/mmc/host/sdhci-pltfm.h @@ -37,33 +37,54 @@ struct sdhci_pltfm_host { * These accessors are designed for big endian hosts doing I/O to * little endian controllers incorporating a 32-bit hardware byte swapper. */ -static inline u32 sdhci_be32bs_readl(struct sdhci_host *host, int reg) +static inline void sdhci_clrsetbits(struct sdhci_host *host, u32 mask, + u32 val, int reg) { - return in_be32(host->ioaddr + reg); + void __iomem *base = host->ioaddr + (reg & ~0x3); + u32 shift = (reg & 0x3) * 8; + + if (host->mmc->big_endian_mode) + iowrite32be(((ioread32be(base) & ~(mask << shift)) | + (val << shift)), base); + else + iowrite32(((ioread32(base) & ~(mask << shift)) | + (val << shift)), base); } -static inline u16 sdhci_be32bs_readw(struct sdhci_host *host, int reg) +static inline u32 sdhci_32bs_readl(struct sdhci_host *host, int reg) { - return in_be16(host->ioaddr + (reg ^ 0x2)); + if (host->mmc->big_endian_mode) + return ioread32be(host->ioaddr + reg); + else + return ioread32(host->ioaddr + reg); } -static inline u8 sdhci_be32bs_readb(struct sdhci_host *host, int reg) +static inline u16 sdhci_32bs_readw(struct sdhci_host *host, int reg) { - return in_8(host->ioaddr + (reg ^ 0x3)); + if (host->mmc->big_endian_mode) + return ioread16be(host->ioaddr + (reg ^ 0x2)); + else + return ioread16(host->ioaddr + (reg ^ 0x2)); } -static inline void sdhci_be32bs_writel(struct sdhci_host *host, - u32 val, int reg) +static inline u8 sdhci_32bs_readb(struct sdhci_host *host, int reg) { - out_be32(host->ioaddr + reg, val); + return ioread8(host->ioaddr + (reg ^ 0x3)); } -static inline void sdhci_be32bs_writew(struct sdhci_host *host, +static inline void sdhci_32bs_writel(struct sdhci_host *host, + u32 val, int reg) +{ + if (host->mmc->big_endian_mode) + iowrite32be(val, host->ioaddr + reg); + else + iowrite32(val, host->ioaddr + reg); +} + +static inline void sdhci_32bs_writew(struct sdhci_host *host, u16 val, int reg) { struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); - int base = reg & ~0x3; - int shift = (reg & 0x2) * 8; switch (reg) { case SDHCI_TRANSFER_MODE: @@ -74,20 +95,20 @@ static inline void sdhci_be32bs_writew(struct sdhci_host *host, pltfm_host->xfer_mode_shadow = val; return; case SDHCI_COMMAND: - sdhci_be32bs_writel(host, - val << 16 | pltfm_host->xfer_mode_shadow, - SDHCI_TRANSFER_MODE); + if (host->mmc->big_endian_mode) + iowrite32be(val << 16 | pltfm_host->xfer_mode_shadow, + host->ioaddr + SDHCI_TRANSFER_MODE); + else + iowrite32(val << 16 | pltfm_host->xfer_mode_shadow, + host->ioaddr + SDHCI_TRANSFER_MODE); return; } - clrsetbits_be32(host->ioaddr + base, 0xffff << shift, val << shift); + sdhci_clrsetbits(host, 0xffff, val, reg); } -static inline void sdhci_be32bs_writeb(struct sdhci_host *host, u8 val, int reg) +static inline void sdhci_32bs_writeb(struct sdhci_host *host, u8 val, int reg) { - int base = reg & ~0x3; - int shift = (reg & 0x3) * 8; - - clrsetbits_be32(host->ioaddr + base , 0xff << shift, val << shift); + sdhci_clrsetbits(host, 0xff, val, reg); } #endif /* CONFIG_MMC_SDHCI_BIG_ENDIAN_32BIT_BYTE_SWAPPER */
There has been only big-endian mode support for SDHCI platform before. This patch makes the driver support both little-endian mode and big-endian mode. Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com> --- Changes for v3: - Moved endian mode checking to generic mmc host driver - Modified the commit message --- drivers/mmc/host/sdhci-pltfm.c | 2 +- drivers/mmc/host/sdhci-pltfm.h | 63 ++++++++++++++++++++++++++++-------------- 2 files changed, 43 insertions(+), 22 deletions(-)