| Message ID | 1400146684-30384-1-git-send-email-srinivas.kandagatla@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Thu, May 15, 2014 at 11:38 AM, <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> (...) > +static int mmci_qcom_pio_read(struct mmci_host *host, char *buffer, > + unsigned int remain) > +{ > + uint32_t *ptr = (uint32_t *) buffer; Just use u32 like the rest of the driver does. > + int fifo_size = variant->fifosize; > + > + if (remain % 4) > + remain = ((remain >> 2) + 1) << 2; Insert a comment here so we know what is happening. It seems you are rounding up to the nearest divisible by four reads. It looks complicated. Does this accomplish the same thing? if (remain % 4) { remain &= ~0x03; remain += 4; } > + while (readl(host->base + MMCISTATUS) & MCI_RXDATAAVLBL) { > + *ptr = readl(host->base + MMCIFIFO + (count % fifo_size)); > + ptr++; > + count += sizeof(uint32_t); > + > + remain -= sizeof(uint32_t); I already commented that this looks weird. It is a well known fact that a u32 is 4 bytes. count += 4; remain -= 4; works just fine! > + if (remain == 0) > + break; > + } > + return count; > +} And another variant is to count the *number or words* to read from the FIFO rather than the number of bytes! I would do it like this: 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; unsigned int fifo_size = host->variant->fifosize; words = DIV_ROUND_UP(remain, 4); while (readl(host->base + MMCISTATUS) & MCI_RXDATAAVLBL) { *ptr = readl(host->base + MMCIFIFO + (count % fifo_size)); ptr++; count += 4; remain--; if (!remain) break; } return count; } I guess you will run into additional problems when you come to doing SDIO. This function can return *more* bytes than asked for, as it rounds up. It won't happen with MMC/SD transfers since these are always divisible by 8, but it *will* happen on SDIO! If you look carefully at the comments in mmci_pio_read() you will see how this is handled. Are you sure this function cannot be augmented to handle the qcom variant as well so you don't get this problem further down the road? 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 23/05/14 10:31, Linus Walleij wrote: > On Thu, May 15, 2014 at 11:38 AM, <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> > (...) > >> +static int mmci_qcom_pio_read(struct mmci_host *host, char *buffer, >> + unsigned int remain) >> +{ >> + uint32_t *ptr = (uint32_t *) buffer; > Oops.. Sorry Linus, I think this change-set got something different than what I did. I remember your comments in the last review, I did take care of them but some how It got missed... I will fix these and send a new version. > Just use u32 like the rest of the driver does. > >> + int fifo_size = variant->fifosize; >> + >> + if (remain % 4) > > And another variant is to count the *number or words* > to read from the FIFO rather than the number of bytes! I would do it > like this: > > 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; > unsigned int fifo_size = host->variant->fifosize; > > words = DIV_ROUND_UP(remain, 4); > while (readl(host->base + MMCISTATUS) & MCI_RXDATAAVLBL) { > *ptr = readl(host->base + MMCIFIFO + (count % fifo_size)); > ptr++; > count += 4; > remain--; > if (!remain) > break; > } > return count; > } > > I guess you will run into additional problems when you come to doing > SDIO. This function can return *more* bytes than asked for, as it rounds > up. It won't happen with MMC/SD transfers since these are always > divisible by 8, but it *will* happen on SDIO! > > If you look carefully at the comments in mmci_pio_read() you will > see how this is handled. Are you sure this function cannot be > augmented to handle the qcom variant as well so you don't get this > problem further down the road? I did try to customize the mmci_pio_read function but I failed all the time. The reason being the behaviour of MCI_FIFOCNT register which is totally different to the way the function is written. I will give a try again. Thanks, srini > > Yours, > Linus Walleij > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus W, On 23/05/14 10:31, Linus Walleij wrote: > 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; > unsigned int fifo_size = host->variant->fifosize; > > words = DIV_ROUND_UP(remain, 4); > while (readl(host->base + MMCISTATUS) & MCI_RXDATAAVLBL) { > *ptr = readl(host->base + MMCIFIFO + (count % fifo_size)); > ptr++; > count += 4; > remain--; > if (!remain) > break; > } > return count; > } > > I guess you will run into additional problems when you come to doing > SDIO. This function can return*more* bytes than asked for, as it rounds > up. It won't happen with MMC/SD transfers since these are always > divisible by 8, but it*will* happen on SDIO! That's a good point, Qualcomm will need SDIO support in future, so I have slightly modified the code to address this. Other thing I tried was to fit in this in mmci_pio_read, It became very ugly, as the FIFOCNT register behaviour is totally different and there is no way to tell how many bytes are ready to be consumed. So finally I think having a separate pio read for qualcomm looks much neater. final mmci_qcom_pio_read looks like: 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 fifo_size = 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 % fifo_size)); ptr++; count += 4; words--; if (!words) break; } } /* read leftover bytes */ if (unlikely(bytes)) { unsigned char buf[4]; if (readl(host->base + MMCISTATUS) & MCI_RXDATAAVLBL) { *buf = readl(host->base + MMCIFIFO + (count % fifo_size)); memcpy(ptr, buf, bytes); count += bytes; } } return count; } Thanks, srini -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" 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 cf58fec1..94b99d6 100644 --- a/drivers/mmc/host/mmci.c +++ b/drivers/mmc/host/mmci.c @@ -77,6 +77,7 @@ static unsigned int fmax = 515633; * are not ignored. * @explicit_mclk_control: enable explicit mclk control in driver. * @qcom_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; @@ -101,6 +102,7 @@ struct variant_data { bool mclk_delayed_writes; bool explicit_mclk_control; bool qcom_cclk_is_mclk; + bool qcom_fifo; }; static struct variant_data variant_arm = { @@ -211,6 +213,7 @@ static struct variant_data variant_qcom = { .mclk_delayed_writes = true, .explicit_mclk_control = true, .qcom_cclk_is_mclk = true, + .qcom_fifo = true, }; static inline u32 mmci_readl(struct mmci_host *host, u32 off) @@ -1026,6 +1029,29 @@ 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) +{ + uint32_t *ptr = (uint32_t *) buffer; + int count = 0; + struct variant_data *variant = host->variant; + int fifo_size = variant->fifosize; + + if (remain % 4) + remain = ((remain >> 2) + 1) << 2; + + while (readl(host->base + MMCISTATUS) & MCI_RXDATAAVLBL) { + *ptr = readl(host->base + MMCIFIFO + (count % fifo_size)); + ptr++; + count += sizeof(uint32_t); + + remain -= sizeof(uint32_t); + if (remain == 0) + break; + } + return count; +} + static int mmci_pio_read(struct mmci_host *host, char *buffer, unsigned int remain) { void __iomem *base = host->base; @@ -1147,8 +1173,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);