diff mbox

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

Message ID 1400849589-7626-1-git-send-email-srinivas.kandagatla@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Srinivas Kandagatla May 23, 2014, 12:53 p.m. UTC
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(-)

Comments

Stephen Boyd May 23, 2014, 11:28 p.m. UTC | #1
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;
>
Ulf Hansson May 26, 2014, 2:34 p.m. UTC | #2
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
Srinivas Kandagatla May 26, 2014, 5:10 p.m. UTC | #3
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
Linus Walleij May 28, 2014, 8:08 a.m. UTC | #4
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
Srinivas Kandagatla May 28, 2014, 8:51 a.m. UTC | #5
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
Srinivas Kandagatla May 28, 2014, 1:57 p.m. UTC | #6
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
Linus Walleij May 29, 2014, 7:43 a.m. UTC | #7
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
Stephen Boyd May 30, 2014, 1:30 a.m. UTC | #8
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/
Linus Walleij May 30, 2014, 9 a.m. UTC | #9
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 mbox

Patch

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);