[U-Boot] [PATCH] MMC: PL180: Fix infinite loop with VExpress extended fifo implementation

[U-Boot] [PATCH] MMC: PL180: Fix infinite loop with VExpress extended fifo implementation

[Jon Medhurst]

From: Jon Medhurst <jon.medhurst@linaro.org>

The new IO FPGA implementation for Versatile Express contains an MMCI
(PL180) cell with the FIFO extended to 128 words. This causes the
read_bytes() function to go into an infinite loop; as it will wait for
for the half-full signal (SDI_STA_RXFIFOBR) if there are more than 8
words remaining (SDI_FIFO_BURST_SIZE), but it won't receive this signal
once there are fewer than 64 words left to transfer.

One possible fix is to add some build time configuration to change
SDI_FIFO_BURST_SIZE for the new implementation. However, the problematic
code only seems to exist as a small performance optimisation, so the
solution implemented by this patch is to simply remove it. The error
checking following the loop is also removed as this will be handled by
code further down the function.

Cc: Andy Fleming <afleming@gmail.com>
Signed-off-by: Jon Medhurst <jon.medhurst@linaro.org>
---

If it is desirable to keep the burst read optimisation, then an
alternative solution would be to keep the loop and add an if clause to
do a single read if the fifo doesn't have enough for a burst.

---
 drivers/mmc/arm_pl180_mmci.c |   26 --------------------------
 1 files changed, 0 insertions(+), 26 deletions(-)

diff --git a/drivers/mmc/arm_pl180_mmci.c b/drivers/mmc/arm_pl180_mmci.c
index ed296ee..e6467a2 100644
--- a/drivers/mmc/arm_pl180_mmci.c
+++ b/drivers/mmc/arm_pl180_mmci.c
@@ -111,7 +111,6 @@ static int do_command(struct mmc *dev, struct mmc_cmd *cmd)
 static int read_bytes(struct mmc *dev, u32 *dest, u32 blkcount, u32 blksize)
 {
        u32 *tempbuff = dest;
-       int i;
        u64 xfercount = blkcount * blksize;
        struct mmc_host *host = dev->priv;
        u32 status, status_err;
@@ -121,31 +120,6 @@ static int read_bytes(struct mmc *dev, u32 *dest, u32 blkcount, u32 blksize)
        status = readl(&host->base->status);
        status_err = status & (SDI_STA_DCRCFAIL | SDI_STA_DTIMEOUT |
                               SDI_STA_RXOVERR);
-       while (!status_err &&
-              (xfercount >= SDI_FIFO_BURST_SIZE * sizeof(u32))) {
-               if (status & SDI_STA_RXFIFOBR) {
-                       for (i = 0; i < SDI_FIFO_BURST_SIZE; i++)
-                               *(tempbuff + i) = readl(&host->base->fifo);
-                       tempbuff += SDI_FIFO_BURST_SIZE;
-                       xfercount -= SDI_FIFO_BURST_SIZE * sizeof(u32);
-               }
-               status = readl(&host->base->status);
-               status_err = status &
-                       (SDI_STA_DCRCFAIL | SDI_STA_DTIMEOUT | SDI_STA_RXOVERR);
-       }
-
-       if (status & SDI_STA_DTIMEOUT) {
-               printf("Read data timed out, xfercount: %llu, status: 0x%08X\n",
-                       xfercount, status);
-               return -ETIMEDOUT;
-       } else if (status & SDI_STA_DCRCFAIL) {
-               printf("Read data blk CRC error: 0x%x\n", status);
-               return -EILSEQ;
-       } else if (status & SDI_STA_RXOVERR) {
-               printf("Read data RX overflow error\n");
-               return -EIO;
-       }
-
        while ((!status_err) && (xfercount >= sizeof(u32))) {
                if (status & SDI_STA_RXDAVL) {
                        *(tempbuff) = readl(&host->base->fifo);
-- 
1.7.4.1

[U-Boot] [PATCH] MMC: PL180: Fix infinite loop with VExpress extended fifo implementation

[Pawel Moll]

Hi Tixy,

> One possible fix is to add some build time configuration to change
> SDI_FIFO_BURST_SIZE for the new implementation. 

You can also detect the configuration in runtime, basing on PeriphID:

http://infocenter.arm.com/help/topic/com.arm.doc.ddi0172a/i1024149.html

Configuration == 0 (ID == 0x00041180) -> FIFO length = 16 * 4
Configuration == 1 (ID == 0x01041180) -> FIFO length = 128 * 4

Cheers!

PAwe?

[U-Boot] [PATCH] MMC: PL180: Fix infinite loop with VExpress extended fifo implementation

[Jon Medhurst]

On Wed, 2011-10-05 at 10:30 +0100, Pawel Moll wrote:
> Hi Tixy,
> 
> > One possible fix is to add some build time configuration to change
> > SDI_FIFO_BURST_SIZE for the new implementation. 
> 
> You can also detect the configuration in runtime, basing on PeriphID:
> 
> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0172a/i1024149.html
> 
> Configuration == 0 (ID == 0x00041180) -> FIFO length = 16 * 4
> Configuration == 1 (ID == 0x01041180) -> FIFO length = 128 * 4

That's useful to know. The PL180 code is also used for U8500, I don't
know if that implements the peripheral ID register; though I guess any
probing could be limited to vexpress anyway.

However, I think that if smaller and simpler code will work on all
hardware then that is preferable.

-- 
Tixy

[U-Boot] [PATCH] MMC: PL180: Fix infinite loop with VExpress extended fifo implementation

[Pawel Moll]

> That's useful to know. The PL180 code is also used for U8500, I don't
> know if that implements the peripheral ID register; though I guess any
> probing could be limited to vexpress anyway.

STE have the same "problems" with FIFO size, see drivers/mmc/host/mmci.c
in kernel sources:

static struct variant_data variant_arm = {
        .fifosize               = 16 * 4,
        .fifohalfsize           = 8 * 4,
        .datalength_bits        = 16, 
};      

static struct variant_data variant_arm_extended_fifo = {
        .fifosize               = 128 * 4,
        .fifohalfsize           = 64 * 4,
        .datalength_bits        = 16,
};      

static struct variant_data variant_u300 = {
        .fifosize               = 16 * 4, 
        .fifohalfsize           = 8 * 4,
        .clkreg_enable          = MCI_ST_U300_HWFCEN,
        .datalength_bits        = 16,
        .sdio                   = true,
};      

static struct variant_data variant_ux500 = {
        .fifosize               = 30 * 4,
        .fifohalfsize           = 8 * 4,
        .clkreg                 = MCI_CLK_ENABLE,
        .clkreg_enable          = MCI_ST_UX500_HWFCEN,
        .datalength_bits        = 24,
        .sdio                   = true,
        .st_clkdiv              = true,
};      

static struct variant_data variant_ux500v2 = {
        .fifosize               = 30 * 4,
        .fifohalfsize           = 8 * 4,
        .clkreg                 = MCI_CLK_ENABLE,
        .clkreg_enable          = MCI_ST_UX500_HWFCEN,
        .datalength_bits        = 24,
        .sdio                   = true,
        .st_clkdiv              = true,
        .blksz_datactrl16       = true,
};      

Cheers!

Pawe?

[U-Boot] [PATCH] MMC: PL180: Fix infinite loop with VExpress extended fifo implementation

[Jon Medhurst]

On Wed, 2011-10-05 at 10:58 +0100, Pawel Moll wrote:
> > That's useful to know. The PL180 code is also used for U8500, I don't
> > know if that implements the peripheral ID register; though I guess any
> > probing could be limited to vexpress anyway.
> 
> STE have the same "problems" with FIFO size, see drivers/mmc/host/mmci.c
> in kernel sources:

Yes, and my proposed fix for U-Boot will work with them all because it
removes a dependency on the fifo size.

-- 
Tixy

[U-Boot] [PATCH] MMC: PL180: Fix infinite loop with VExpress extended fifo implementation

[Matt Waddel]

On 10/04/2011 11:32 AM, Jon Medhurst (Tixy) wrote:
> From: Jon Medhurst <jon.medhurst@linaro.org>
> 
> The new IO FPGA implementation for Versatile Express contains an MMCI
> (PL180) cell with the FIFO extended to 128 words. This causes the
> read_bytes() function to go into an infinite loop; as it will wait for
> for the half-full signal (SDI_STA_RXFIFOBR) if there are more than 8
> words remaining (SDI_FIFO_BURST_SIZE), but it won't receive this signal
> once there are fewer than 64 words left to transfer.
> 
> One possible fix is to add some build time configuration to change
> SDI_FIFO_BURST_SIZE for the new implementation. However, the problematic
> code only seems to exist as a small performance optimisation, so the
> solution implemented by this patch is to simply remove it. The error
> checking following the loop is also removed as this will be handled by
> code further down the function.
> 
> Cc: Andy Fleming <afleming@gmail.com>
> Signed-off-by: Jon Medhurst <jon.medhurst@linaro.org>

Acked-by: Matt Waddel <matt.waddel@linaro.org>

> ---
> 
> If it is desirable to keep the burst read optimisation, then an
> alternative solution would be to keep the loop and add an if clause to
> do a single read if the fifo doesn't have enough for a burst.
> 
> ---
>  drivers/mmc/arm_pl180_mmci.c |   26 --------------------------
>  1 files changed, 0 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/mmc/arm_pl180_mmci.c b/drivers/mmc/arm_pl180_mmci.c
> index ed296ee..e6467a2 100644
> --- a/drivers/mmc/arm_pl180_mmci.c
> +++ b/drivers/mmc/arm_pl180_mmci.c
> @@ -111,7 +111,6 @@ static int do_command(struct mmc *dev, struct mmc_cmd *cmd)
>  static int read_bytes(struct mmc *dev, u32 *dest, u32 blkcount, u32 blksize)
>  {
>         u32 *tempbuff = dest;
> -       int i;
>         u64 xfercount = blkcount * blksize;
>         struct mmc_host *host = dev->priv;
>         u32 status, status_err;
> @@ -121,31 +120,6 @@ static int read_bytes(struct mmc *dev, u32 *dest, u32 blkcount, u32 blksize)
>         status = readl(&host->base->status);
>         status_err = status & (SDI_STA_DCRCFAIL | SDI_STA_DTIMEOUT |
>                                SDI_STA_RXOVERR);
> -       while (!status_err &&
> -              (xfercount >= SDI_FIFO_BURST_SIZE * sizeof(u32))) {
> -               if (status & SDI_STA_RXFIFOBR) {
> -                       for (i = 0; i < SDI_FIFO_BURST_SIZE; i++)
> -                               *(tempbuff + i) = readl(&host->base->fifo);
> -                       tempbuff += SDI_FIFO_BURST_SIZE;
> -                       xfercount -= SDI_FIFO_BURST_SIZE * sizeof(u32);
> -               }
> -               status = readl(&host->base->status);
> -               status_err = status &
> -                       (SDI_STA_DCRCFAIL | SDI_STA_DTIMEOUT | SDI_STA_RXOVERR);
> -       }
> -
> -       if (status & SDI_STA_DTIMEOUT) {
> -               printf("Read data timed out, xfercount: %llu, status: 0x%08X\n",
> -                       xfercount, status);
> -               return -ETIMEDOUT;
> -       } else if (status & SDI_STA_DCRCFAIL) {
> -               printf("Read data blk CRC error: 0x%x\n", status);
> -               return -EILSEQ;
> -       } else if (status & SDI_STA_RXOVERR) {
> -               printf("Read data RX overflow error\n");
> -               return -EIO;
> -       }
> -
>         while ((!status_err) && (xfercount >= sizeof(u32))) {
>                 if (status & SDI_STA_RXDAVL) {
>                         *(tempbuff) = readl(&host->base->fifo);


-- 
Linaro.org ? Open source software for ARM SoCs
Follow Linaro:
http://www.facebook.com/pages/Linaro/155974581091106
http://twitter.com/#!/linaroorg
http://www.linaro.org/linaro-blog/

[U-Boot] [PATCH] MMC: PL180: Fix infinite loop with VExpress extended fifo implementation

[Andy Fleming]

On Tue, Oct 4, 2011 at 12:32 PM, Jon Medhurst (Tixy)
<jon.medhurst@linaro.org> wrote:

> Cc: Andy Fleming <afleming@gmail.com>
> Signed-off-by: Jon Medhurst <jon.medhurst@linaro.org>
> ?drivers/mmc/arm_pl180_mmci.c | ? 26 --------------------------
> ?1 files changed, 0 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/mmc/arm_pl180_mmci.c b/drivers/mmc/arm_pl180_mmci.c
> index ed296ee..e6467a2 100644
> --- a/drivers/mmc/arm_pl180_mmci.c
> +++ b/drivers/mmc/arm_pl180_mmci.c
> @@ -111,7 +111,6 @@ static int do_command(struct mmc *dev, struct mmc_cmd *cmd)
> ?static int read_bytes(struct mmc *dev, u32 *dest, u32 blkcount, u32 blksize)
> ?{
> ? ? ? ?u32 *tempbuff = dest;
> - ? ? ? int i;
> ? ? ? ?u64 xfercount = blkcount * blksize;
> ? ? ? ?struct mmc_host *host = dev->priv;
> ? ? ? ?u32 status, status_err;


Please fix your patch-sending software. This patch has converted all
of the tabs to spaces, and won't apply.

Andy

[U-Boot] [PATCH v2] MMC: PL180: Fix infinite loop with VExpress extended fifo implementation

[Jon Medhurst]

The new IO FPGA implementation for Versatile Express contains an MMCI
(PL180) cell with the FIFO extended to 128 words. This causes the
read_bytes() function to go into an infinite loop; as it will wait for
for the half-full signal (SDI_STA_RXFIFOBR) if there are more than 8
words remaining (SDI_FIFO_BURST_SIZE), but it won't receive this signal
once there are fewer than 64 words left to transfer.

One possible fix is to add some build time configuration to change
SDI_FIFO_BURST_SIZE for the new implementation. However, the problematic
code only seems to exist as a small performance optimisation, so the
solution implemented by this patch is to simply remove it. The error
checking following the loop is also removed as this will be handled by
code further down the function.

Cc: Andy Fleming <afleming@gmail.com>
Signed-off-by: Jon Medhurst <jon.medhurst@linaro.org>
---

Changes for version 2
- Fix broken whitespace in patch

---
 drivers/mmc/arm_pl180_mmci.c |   26 --------------------------
 1 files changed, 0 insertions(+), 26 deletions(-)

diff --git a/drivers/mmc/arm_pl180_mmci.c b/drivers/mmc/arm_pl180_mmci.c
index ed296ee..e6467a2 100644
--- a/drivers/mmc/arm_pl180_mmci.c
+++ b/drivers/mmc/arm_pl180_mmci.c
@@ -111,7 +111,6 @@ static int do_command(struct mmc *dev, struct mmc_cmd *cmd)
 static int read_bytes(struct mmc *dev, u32 *dest, u32 blkcount, u32 blksize)
 {
 	u32 *tempbuff = dest;
-	int i;
 	u64 xfercount = blkcount * blksize;
 	struct mmc_host *host = dev->priv;
 	u32 status, status_err;
@@ -121,31 +120,6 @@ static int read_bytes(struct mmc *dev, u32 *dest, u32 blkcount, u32 blksize)
 	status = readl(&host->base->status);
 	status_err = status & (SDI_STA_DCRCFAIL | SDI_STA_DTIMEOUT |
 			       SDI_STA_RXOVERR);
-	while (!status_err &&
-	       (xfercount >= SDI_FIFO_BURST_SIZE * sizeof(u32))) {
-		if (status & SDI_STA_RXFIFOBR) {
-			for (i = 0; i < SDI_FIFO_BURST_SIZE; i++)
-				*(tempbuff + i) = readl(&host->base->fifo);
-			tempbuff += SDI_FIFO_BURST_SIZE;
-			xfercount -= SDI_FIFO_BURST_SIZE * sizeof(u32);
-		}
-		status = readl(&host->base->status);
-		status_err = status &
-			(SDI_STA_DCRCFAIL | SDI_STA_DTIMEOUT | SDI_STA_RXOVERR);
-	}
-
-	if (status & SDI_STA_DTIMEOUT) {
-		printf("Read data timed out, xfercount: %llu, status: 0x%08X\n",
-			xfercount, status);
-		return -ETIMEDOUT;
-	} else if (status & SDI_STA_DCRCFAIL) {
-		printf("Read data blk CRC error: 0x%x\n", status);
-		return -EILSEQ;
-	} else if (status & SDI_STA_RXOVERR) {
-		printf("Read data RX overflow error\n");
-		return -EIO;
-	}
-
 	while ((!status_err) && (xfercount >= sizeof(u32))) {
 		if (status & SDI_STA_RXDAVL) {
 			*(tempbuff) = readl(&host->base->fifo);
-- 
1.7.4.1

[U-Boot] [PATCH v2] MMC: PL180: Fix infinite loop with VExpress extended fifo implementation

[Andy Fleming]

On Fri, Nov 4, 2011 at 8:06 AM, Jon Medhurst (Tixy)
<jon.medhurst@linaro.org> wrote:
> The new IO FPGA implementation for Versatile Express contains an MMCI
> (PL180) cell with the FIFO extended to 128 words. This causes the
> read_bytes() function to go into an infinite loop; as it will wait for
> for the half-full signal (SDI_STA_RXFIFOBR) if there are more than 8
> words remaining (SDI_FIFO_BURST_SIZE), but it won't receive this signal
> once there are fewer than 64 words left to transfer.
>
> One possible fix is to add some build time configuration to change
> SDI_FIFO_BURST_SIZE for the new implementation. However, the problematic
> code only seems to exist as a small performance optimisation, so the
> solution implemented by this patch is to simply remove it. The error
> checking following the loop is also removed as this will be handled by
> code further down the function.
>
> Cc: Andy Fleming <afleming@gmail.com>
> Signed-off-by: Jon Medhurst <jon.medhurst@linaro.org>

Applied, thx