[v4,13/13] mmc: mmci: Add Qcom specific pio_read function.

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 | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 drivers/mmc/host/mmci.h |  1 +
 2 files changed, 45 insertions(+), 1 deletion(-)

On 28 May 2014 15:48,  <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 | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/mmc/host/mmci.h |  1 +
>  2 files changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 6eb0a29..b68223a 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -73,6 +73,7 @@ static unsigned int fmax = 515633;
>   * @busy_detect: true if busy detection on dat0 is supported
>   * @pwrreg_nopower: bits in MMCIPOWER don't controls ext. power supply
>   * @explicit_mclk_control: enable explicit mclk control in driver.
> + * @qcom_fifo: enables qcom specific fifo pio read function.
>   */
>  struct variant_data {
>         unsigned int            clkreg;
> @@ -95,6 +96,7 @@ struct variant_data {
>         bool                    busy_detect;
>         bool                    pwrreg_nopower;
>         bool                    explicit_mclk_control;
> +       bool                    qcom_fifo;
>  };
>
>  static struct variant_data variant_arm = {
> @@ -202,6 +204,7 @@ static struct variant_data variant_qcom = {
>         .pwrreg_powerup         = MCI_PWR_UP,
>         .f_max                  = 208000000,
>         .explicit_mclk_control  = true,
> +       .qcom_fifo              = true,
>  };
>
>  static int mmci_card_busy(struct mmc_host *mmc)
> @@ -1006,6 +1009,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;

Instead of u32 ptr above, I suggest to use a char *ptr. You need it
anyway further down below where you move in step of bytes instead of
words.

> +       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) {

How about while (words && (readl(host->base + MMCISTATUS) & MCI_RXDATAAVLBL)

That would make it possible the remove the "if", both above and below.

> +                       *ptr = readl(host->base + MMCIFIFO + (count % fsize));

This looks strange. :-) Depending on the count you will read an offset
into the FIFO? Seems like a very awkward implementation of a FIFO in
the HW. :-)

BTW, what does "MCI_RXDATAAVLBL" actually mean for the Qcom variant?
How much data could you expect in the FIFO when this status is
triggered?

Are there no option of reading a number of words, depending on what
type FIFO IRQ that was raised?

> +                       ptr++;
> +                       count += 4;
> +                       words--;
> +                       if (!words)
> +                               break;
> +               }
> +       }
> +
> +       if (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;
> @@ -1129,7 +1166,8 @@ static irqreturn_t mmci_pio_irq(int irq, void *dev_id)
>
>                 len = 0;
>                 if (status & MCI_RXACTIVE)
> -                       len = mmci_pio_read(host, buffer, remain);
> +                       len = host->pio_read(host, buffer, remain);
> +
>                 if (status & MCI_TXACTIVE)
>                         len = mmci_pio_write(host, buffer, remain, status);
>
> @@ -1504,6 +1542,11 @@ static int mmci_probe(struct amba_device *dev,
>         if (ret)
>                 goto host_free;
>
> +       if (variant->qcom_fifo)
> +               host->pio_read = mmci_qcom_pio_read;
> +       else
> +               host->pio_read = mmci_pio_read;
> +
>         host->plat = plat;
>         host->variant = variant;
>         host->mclk = clk_get_rate(host->clk);
> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
> index 1882e20..dc9fe0a 100644
> --- a/drivers/mmc/host/mmci.h
> +++ b/drivers/mmc/host/mmci.h
> @@ -229,6 +229,7 @@ struct mmci_host {
>         /* pio stuff */
>         struct sg_mapping_iter  sg_miter;
>         unsigned int            size;
> +       int (*pio_read)(struct mmci_host *h, char *buf, unsigned int remain);
>
>  #ifdef CONFIG_DMA_ENGINE
>         /* DMA stuff */
> --
> 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

Hi Ulf,

On 30/05/14 12:27, Ulf Hansson wrote:
> On 28 May 2014 15:48,  <srinivas.kandagatla@linaro.org> wrote:
...

>>          .f_max                  = 208000000,
>>          .explicit_mclk_control  = true,
>> +       .qcom_fifo              = true,
>>   };
>>
>>   static int mmci_card_busy(struct mmc_host *mmc)
>> @@ -1006,6 +1009,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;
>
> Instead of u32 ptr above, I suggest to use a char *ptr. You need it
> anyway further down below where you move in step of bytes instead of
> words.
>
>> +       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) {
>
> How about while (words && (readl(host->base + MMCISTATUS) & MCI_RXDATAAVLBL)
>
> That would make it possible the remove the "if", both above and below.

That sounds sensible.. I will try it.
>
>> +                       *ptr = readl(host->base + MMCIFIFO + (count % fsize));
>
> This looks strange. :-) Depending on the count you will read an offset
> into the FIFO? Seems like a very awkward implementation of a FIFO in
> the HW. :-)
>
I got into weird issues when I tried using the mmci_pio_read, the fifo 
implementation seems to be different. I dont have full details on the 
fifo behaviour.

Most of this logic is inherited from 3.4 qcom andriod kernel.

> BTW, what does "MCI_RXDATAAVLBL" actually mean for the Qcom variant?

It means, At least 1 word in the RX FIFO. SW can read 1 word only from 
the FIFO.

> How much data could you expect in the FIFO when this status is
> triggered?

> Are there no option of reading a number of words, depending on what
> type FIFO IRQ that was raised?
There are RXFIFO_HALF_FULL and RXFIFO_FULL bits in status register, but 
I never tried to use them. Might be worth a try before I send next 
version patches.


I will give a try and keep you posted.
Thanks,
srini
>
>> +                       ptr++;
>> +                       count += 4;
>> +                       words--;
>> +                       if (!words)
>> +                               break;
>> +               }
>> +       }
>> +
>> +       if (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;
>> +}
>> +

>>
>
> Kind regards
> Ulf Hansson
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/