| Message ID | 1400849589-7626-1-git-send-email-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/23/14 05:53, srinivas.kandagatla@linaro.org wrote: > @@ -1022,6 +1025,40 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, > } > } > > +static int mmci_qcom_pio_read(struct mmci_host *host, char *buffer, > + unsigned int remain) > +{ > + u32 *ptr = (u32 *) buffer; > + unsigned int count = 0; > + unsigned int words, bytes; > + unsigned int fsize = host->variant->fifosize; > + > + words = remain >> 2; > + bytes = remain % 4; > + /* read full words followed by leftover bytes */ > + if (words) { > + while (readl(host->base + MMCISTATUS) & MCI_RXDATAAVLBL) { > + *ptr = readl(host->base + MMCIFIFO + (count % fsize)); This doesn't look endianness agnostic. Shouldn't we use ioread32_rep() to read this fifo? > + ptr++; > + count += 4; > + words--; > + if (!words) > + break; > + } > + } > + > + if (unlikely(bytes)) { > + unsigned char buf[4]; > + if (readl(host->base + MMCISTATUS) & MCI_RXDATAAVLBL) { > + *buf = readl(host->base + MMCIFIFO + (count % fsize)); > + memcpy(ptr, buf, bytes); > + count += bytes; > + } > + } > + > + return count; > +} > + > static int mmci_pio_read(struct mmci_host *host, char *buffer, unsigned int remain) > { > void __iomem *base = host->base; >
On 23 May 2014 14:53, <srinivas.kandagatla@linaro.org> wrote: > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > MCIFIFOCNT register behaviour on Qcom chips is very different than the other > pl180 integrations. MCIFIFOCNT register contains the number of > words that are still waiting to be transferred through the FIFO. It keeps > decrementing once the host CPU reads the MCIFIFO. With the existing logic and > the MCIFIFOCNT behaviour, mmci_pio_read will loop forever, as the FIFOCNT > register will always return transfer size before reading the FIFO. > > Also the data sheet states that "This register is only useful for debug > purposes and should not be used for normal operation since it does not reflect > data which may or may not be in the pipeline". > > This patch implements qcom_pio_read function so as existing mmci_pio_read is > not suitable for Qcom SOCs. qcom_pio_read function is only selected > based on qcom_fifo flag in variant data structure. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > drivers/mmc/host/mmci.c | 46 ++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 44 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c > index f6dfd24..51ce493 100644 > --- a/drivers/mmc/host/mmci.c > +++ b/drivers/mmc/host/mmci.c > @@ -75,6 +75,7 @@ static unsigned int fmax = 515633; > * are not ignored. > * @explicit_mclk_control: enable explicit mclk control in driver. > * @cclk_is_mclk: enable iff card clock is multimedia card adapter clock. > + * @qcom_fifo: enables qcom specific fifo pio read function. > */ > struct variant_data { > unsigned int clkreg; > @@ -98,6 +99,7 @@ struct variant_data { > bool mclk_delayed_writes; > bool explicit_mclk_control; > bool cclk_is_mclk; > + bool qcom_fifo; > }; > > static struct variant_data variant_arm = { > @@ -208,6 +210,7 @@ static struct variant_data variant_qcom = { > .mclk_delayed_writes = true, > .explicit_mclk_control = true, > .cclk_is_mclk = true, > + .qcom_fifo = true, > }; > > static inline u32 mmci_readl(struct mmci_host *host, u32 off) > @@ -1022,6 +1025,40 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, > } > } > > +static int mmci_qcom_pio_read(struct mmci_host *host, char *buffer, > + unsigned int remain) > +{ > + u32 *ptr = (u32 *) buffer; > + unsigned int count = 0; > + unsigned int words, bytes; > + unsigned int fsize = host->variant->fifosize; > + > + words = remain >> 2; > + bytes = remain % 4; > + /* read full words followed by leftover bytes */ > + if (words) { > + while (readl(host->base + MMCISTATUS) & MCI_RXDATAAVLBL) { > + *ptr = readl(host->base + MMCIFIFO + (count % fsize)); > + ptr++; > + count += 4; > + words--; > + if (!words) > + break; > + } > + } > + > + if (unlikely(bytes)) { > + unsigned char buf[4]; > + if (readl(host->base + MMCISTATUS) & MCI_RXDATAAVLBL) { > + *buf = readl(host->base + MMCIFIFO + (count % fsize)); > + memcpy(ptr, buf, bytes); > + count += bytes; > + } > + } > + > + return count; > +} > + > static int mmci_pio_read(struct mmci_host *host, char *buffer, unsigned int remain) > { > void __iomem *base = host->base; > @@ -1143,8 +1180,13 @@ static irqreturn_t mmci_pio_irq(int irq, void *dev_id) > remain = sg_miter->length; > > len = 0; > - if (status & MCI_RXACTIVE) > - len = mmci_pio_read(host, buffer, remain); > + if (status & MCI_RXACTIVE) { > + if (variant->qcom_fifo) > + len = mmci_qcom_pio_read(host, buffer, remain); > + else > + len = mmci_pio_read(host, buffer, remain); > + } This is hot path. As I suggested for the readl and writel wrapper functions, I think it would be better to use a function pointer in the struct mmci host, which you set up in the probe phase. That means the variant data don't need to be checked each an every time this function gets invoked. > + > if (status & MCI_TXACTIVE) > len = mmci_pio_write(host, buffer, remain, status); So no changes needed for pio_write at this point? Or those will come later? > > -- > 1.9.1 > Kind regards Ulf Hansson -- 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 26/05/14 15:34, Ulf Hansson wrote: > This is hot path. > > As I suggested for the readl and writel wrapper functions, I think it > would be better to use a function pointer in the struct mmci host, > which you set up in the probe phase. That means the variant data don't > need to be checked each an every time this function gets invoked. > >> >+ >> > if (status & MCI_TXACTIVE) >> > len = mmci_pio_write(host, buffer, remain, status); > So no changes needed for pio_write at this point? Or those will come later? > Yes, that sounds like more sensible approach to me. I will do this change in next version. Thanks, srini >> > >> >-- -- 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 Fri, May 23, 2014 at 2:53 PM, <srinivas.kandagatla@linaro.org> wrote: > + if (unlikely(bytes)) { > + unsigned char buf[4]; (...) Please think twice about this. http://lwn.net/Articles/70473/ http://lwn.net/Articles/420019/ http://lwn.net/Articles/182369/ Yours, Linus Walleij -- 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 28/05/14 09:08, Linus Walleij wrote: > On Fri, May 23, 2014 at 2:53 PM, <srinivas.kandagatla@linaro.org> wrote: > >> + if (unlikely(bytes)) { >> + unsigned char buf[4]; > (...) > > Please think twice about this. > http://lwn.net/Articles/70473/ > http://lwn.net/Articles/420019/ > http://lwn.net/Articles/182369/ > Thanks for the warning. You are right. I think having likely/unlikely is not always right here. thanks, srini > Yours, > Linus Walleij > -- 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
Sorry Stephen for late reply, Some reason this mail was filtered in other folders. On 24/05/14 00:28, Stephen Boyd wrote: > On 05/23/14 05:53, srinivas.kandagatla@linaro.org wrote: >> @@ -1022,6 +1025,40 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, >> } >> } >> >> +static int mmci_qcom_pio_read(struct mmci_host *host, char *buffer, >> + unsigned int remain) >> +{ >> + u32 *ptr = (u32 *) buffer; >> + unsigned int count = 0; >> + unsigned int words, bytes; >> + unsigned int fsize = host->variant->fifosize; >> + >> + words = remain >> 2; >> + bytes = remain % 4; >> + /* read full words followed by leftover bytes */ >> + if (words) { >> + while (readl(host->base + MMCISTATUS) & MCI_RXDATAAVLBL) { >> + *ptr = readl(host->base + MMCIFIFO + (count % fsize)); > > This doesn't look endianness agnostic. Shouldn't we use ioread32_rep() > to read this fifo? Is'nt readl endianess aware? we can not use ioread32_rep because as we can not reliably know how many words we should read? FIFOCNT would have helped but its not advised to be use as per the datasheet. Thanks, srini > >> + ptr++; >> + count += 4; >> + words--; >> + if (!words) >> + break; >> + } >> + } >> + >> + if (unlikely(bytes)) { >> + unsigned char buf[4]; >> + if (readl(host->base + MMCISTATUS) & MCI_RXDATAAVLBL) { >> + *buf = readl(host->base + MMCIFIFO + (count % fsize)); >> + memcpy(ptr, buf, bytes); >> + count += bytes; >> + } >> + } >> + >> + return count; >> +} >> + >> static int mmci_pio_read(struct mmci_host *host, char *buffer, unsigned int remain) >> { >> void __iomem *base = host->base; >> > -- 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 Wed, May 28, 2014 at 3:57 PM, Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: >> This doesn't look endianness agnostic. Shouldn't we use ioread32_rep() >> to read this fifo? > > Is'nt readl endianess aware? At least once a year read through arch/arm/include/asm/io.h static inline u32 __raw_readl(const volatile void __iomem *addr) { u32 val; asm volatile("ldr %1, %0" : "+Qo" (*(volatile u32 __force *)addr), "=r" (val)); return val; } (...) #define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \ __raw_readl(c)); __r; }) (...) #define readl(c) ({ u32 __v = readl_relaxed(c); __iormb(); __v; }) readl() reads in little endian. All PrimeCells are little endian, trouble would occur if and only if this cell was used on a big endian CPU, like an ARM used in BE mode, if the macros didn't look exactly like this. Thanks to the fact that readl() is always LE things work smoothly. Then to the question whether to use ioread32() instead: #define ioread32(p) ({ unsigned int __v = le32_to_cpu((__force __le32)__raw_readl(p)); __iormb(); __v; }) Same thing as readl(). The only reason to use ioread32() rather than readl() is that some archs do not support readl() but only ioread32(). However for that to be a problem, these archs must be utilizing that primecell! And if they do there are two ways to solve it: either change the entire world to use ioread/write32 or simply just define readl() and friends for that arch in its io.h file. I'd say don't touch it right now. > we can not use ioread32_rep because as we can not reliably know how many > words we should read? FIFOCNT would have helped but its not advised to be > use as per the datasheet. You are right. readsl() or ioread32_rep() isn't used for this reason. Yours, Linus Walleij -- 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 05/29/14 00:43, Linus Walleij wrote: > On Wed, May 28, 2014 at 3:57 PM, Srinivas Kandagatla > <srinivas.kandagatla@linaro.org> wrote: > >>> This doesn't look endianness agnostic. Shouldn't we use ioread32_rep() >>> to read this fifo? >> Is'nt readl endianess aware? > At least once a year read through arch/arm/include/asm/io.h > > static inline u32 __raw_readl(const volatile void __iomem *addr) > { > u32 val; > asm volatile("ldr %1, %0" > : "+Qo" (*(volatile u32 __force *)addr), > "=r" (val)); > return val; > } > (...) > #define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \ > __raw_readl(c)); __r; }) > (...) > #define readl(c) ({ u32 __v = readl_relaxed(c); > __iormb(); __v; }) > > readl() reads in little endian. > > All PrimeCells are little endian, trouble would occur if and only if > this cell was > used on a big endian CPU, like an ARM used in BE mode, if the macros > didn't look exactly like this. > > Thanks to the fact that readl() is always LE things work smoothly. > > Then to the question whether to use ioread32() instead: > > #define ioread32(p) ({ unsigned int __v = le32_to_cpu((__force > __le32)__raw_readl(p)); __iormb(); __v; }) > > Same thing as readl(). The only reason to use ioread32() rather than > readl() is that some archs do not support readl() but only ioread32(). > However for that to be a problem, these archs must be utilizing that > primecell! And if they do there are two ways to solve it: either change > the entire world to use ioread/write32 or simply just define readl() and > friends for that arch in its io.h file. > > I'd say don't touch it right now. Hm... that doesn't sound right. Please see this thread on lkml[1], and also this video from Ben H.[2] My understanding is that this is a fifo so I would think we want to use the ioread32_rep() function here (or other "string" io functionality). Otherwise we're going to byteswap the data that we're trying to interpret as a buffer and big endian CPUs won't work. > >> we can not use ioread32_rep because as we can not reliably know how many >> words we should read? FIFOCNT would have helped but its not advised to be >> use as per the datasheet. > You are right. readsl() or ioread32_rep() isn't used for this reason. > > We could always use a size of 1 right? [1] https://lkml.org/lkml/2012/10/22/671 [2] http://linuxplumbers.ubicast.tv/videos/big-and-little-endian-inside-out/
On Fri, May 30, 2014 at 3:30 AM, Stephen Boyd <sboyd@codeaurora.org> wrote: > Hm... that doesn't sound right. Please see this thread on lkml[1], and > also this video from Ben H.[2] But Benji is talking about the *PCI BUS*, and this is raw register access. He just says that "This is how a BE CPU is normally wired to a LE bus" we have no idea whether that is actually the case on any existing system with this peripheral. Such things have to be tested. > My understanding is that this is a fifo > so I would think we want to use the ioread32_rep() function here (or > other "string" io functionality). Otherwise we're going to byteswap the > data that we're trying to interpret as a buffer and big endian CPUs > won't work. You don't know that until you have tested on real BE ARM hardware with this PrimeCell. How do you know the SoC designers will have been smart enough to insert byte swap logic? >> You are right. readsl() or ioread32_rep() isn't used for this reason. > > We could always use a size of 1 right? What do you mean? Do you mean using: ioread32_rep(io, buf, 1)? Notice this is totally equivalent to readsl() in arm/include/asm/io.h: #define readsl(p,d,l) __raw_readsl(p,d,l) #define ioread32_rep(p,d,c) __raw_readsl(p,d,c) The semantic effect is switching from readl() -> readsl() or conversely ioread32 -> ioread32_rep() which will use __raw accessors in native endianness rather than LE access. Which is what you want if the bus will do byte swapping. Which we don't know if it does, because noone has such a system and can verify. If the native bus is not byte swapping this will just force you to use cpu_to_le32() on the result. That is not helpful at all. We need to experiment with real BE hardware with this PrimeCell to determine whether to do this, and until that is done just don't try to outsmart the hardware by thinking ahead and designing for the unknown, that will just fail. Yours, Linus Walleij -- 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/mmci.c b/drivers/mmc/host/mmci.c index f6dfd24..51ce493 100644 --- a/drivers/mmc/host/mmci.c +++ b/drivers/mmc/host/mmci.c @@ -75,6 +75,7 @@ static unsigned int fmax = 515633; * are not ignored. * @explicit_mclk_control: enable explicit mclk control in driver. * @cclk_is_mclk: enable iff card clock is multimedia card adapter clock. + * @qcom_fifo: enables qcom specific fifo pio read function. */ struct variant_data { unsigned int clkreg; @@ -98,6 +99,7 @@ struct variant_data { bool mclk_delayed_writes; bool explicit_mclk_control; bool cclk_is_mclk; + bool qcom_fifo; }; static struct variant_data variant_arm = { @@ -208,6 +210,7 @@ static struct variant_data variant_qcom = { .mclk_delayed_writes = true, .explicit_mclk_control = true, .cclk_is_mclk = true, + .qcom_fifo = true, }; static inline u32 mmci_readl(struct mmci_host *host, u32 off) @@ -1022,6 +1025,40 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, } } +static int mmci_qcom_pio_read(struct mmci_host *host, char *buffer, + unsigned int remain) +{ + u32 *ptr = (u32 *) buffer; + unsigned int count = 0; + unsigned int words, bytes; + unsigned int fsize = host->variant->fifosize; + + words = remain >> 2; + bytes = remain % 4; + /* read full words followed by leftover bytes */ + if (words) { + while (readl(host->base + MMCISTATUS) & MCI_RXDATAAVLBL) { + *ptr = readl(host->base + MMCIFIFO + (count % fsize)); + ptr++; + count += 4; + words--; + if (!words) + break; + } + } + + if (unlikely(bytes)) { + unsigned char buf[4]; + if (readl(host->base + MMCISTATUS) & MCI_RXDATAAVLBL) { + *buf = readl(host->base + MMCIFIFO + (count % fsize)); + memcpy(ptr, buf, bytes); + count += bytes; + } + } + + return count; +} + static int mmci_pio_read(struct mmci_host *host, char *buffer, unsigned int remain) { void __iomem *base = host->base; @@ -1143,8 +1180,13 @@ static irqreturn_t mmci_pio_irq(int irq, void *dev_id) remain = sg_miter->length; len = 0; - if (status & MCI_RXACTIVE) - len = mmci_pio_read(host, buffer, remain); + if (status & MCI_RXACTIVE) { + if (variant->qcom_fifo) + len = mmci_qcom_pio_read(host, buffer, remain); + else + len = mmci_pio_read(host, buffer, remain); + } + if (status & MCI_TXACTIVE) len = mmci_pio_write(host, buffer, remain, status);