[PATCH] arm: mmci: Add ARM variant with extended FIFO

[PATCH] arm: mmci: Add ARM variant with extended FIFO

[Pawel Moll]

New IO FPGA implementation for Versatile Express boards contain
MMCI (PL180) cell with FIFO extended to 128 words (512 bytes).

Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
 drivers/mmc/host/mmci.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 2d6de3e..e7197b0 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -66,6 +66,12 @@ static struct variant_data variant_arm = {
 	.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,
@@ -1019,10 +1025,15 @@ static int mmci_resume(struct amba_device *dev)
 static struct amba_id mmci_ids[] = {
 	{
 		.id	= 0x00041180,
-		.mask	= 0x000fffff,
+		.mask	= 0xff0fffff,
 		.data	= &variant_arm,
 	},
 	{
+		.id	= 0x01041180,
+		.mask	= 0xff0fffff,
+		.data	= &variant_arm_extended_fifo,
+	},
+	{
 		.id	= 0x00041181,
 		.mask	= 0x000fffff,
 		.data	= &variant_arm,
-- 
1.6.3.3

[PATCH] arm: mmci: Add ARM variant with extended FIFO

[Matt Waddel]

On 03/11/2011 10:18 AM, Pawel Moll wrote:
> New IO FPGA implementation for Versatile Express boards contain
> MMCI (PL180) cell with FIFO extended to 128 words (512 bytes).
> 
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>

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

This patch improves MMC performance on my vexpress system. Also
fixes "mmcblk0: error -5 transferring data" errors.

> ---
>  drivers/mmc/host/mmci.c |   13 ++++++++++++-
>  1 files changed, 12 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 2d6de3e..e7197b0 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -66,6 +66,12 @@ static struct variant_data variant_arm = {
>  	.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,
> @@ -1019,10 +1025,15 @@ static int mmci_resume(struct amba_device *dev)
>  static struct amba_id mmci_ids[] = {
>  	{
>  		.id	= 0x00041180,
> -		.mask	= 0x000fffff,
> +		.mask	= 0xff0fffff,
>  		.data	= &variant_arm,
>  	},
>  	{
> +		.id	= 0x01041180,
> +		.mask	= 0xff0fffff,
> +		.data	= &variant_arm_extended_fifo,
> +	},
> +	{
>  		.id	= 0x00041181,
>  		.mask	= 0x000fffff,
>  		.data	= &variant_arm,

[PATCH] arm: mmci: Add ARM variant with extended FIFO

[Russell King - ARM Linux]

On Wed, Mar 16, 2011 at 10:25:16AM -0600, Matt Waddel wrote:
> On 03/11/2011 10:18 AM, Pawel Moll wrote:
> > New IO FPGA implementation for Versatile Express boards contain
> > MMCI (PL180) cell with FIFO extended to 128 words (512 bytes).
> > 
> > Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> 
> Tested-by: Matt Waddel <matt.waddel@linaro.org>
> 
> This patch improves MMC performance on my vexpress system. Also
> fixes "mmcblk0: error -5 transferring data" errors.

You should use the patch I sent to do dynamic clocking if you're
concerned about performance.  That patch improves performance by
a factor of 8 compared to anything possible by increasing the FIFO
depth.

I am very much of the opinion that deepening the FIFOs is just a
sticky plaster over the problem - and I said as much when it was
first proposed, pointing out that there's other better solutions.

[PATCH] arm: mmci: Add ARM variant with extended FIFO

[Pawel Moll]

> > > New IO FPGA implementation for Versatile Express boards contain
> > > MMCI (PL180) cell with FIFO extended to 128 words (512 bytes).
> > > 
> > > Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> > 
> > Tested-by: Matt Waddel <matt.waddel@linaro.org>
> > 
> > This patch improves MMC performance on my vexpress system. Also
> > fixes "mmcblk0: error -5 transferring data" errors.
> 
> You should use the patch I sent to do dynamic clocking if you're
> concerned about performance.  That patch improves performance by
> a factor of 8 compared to anything possible by increasing the FIFO
> depth.
> 
> I am very much of the opinion that deepening the FIFOs is just a
> sticky plaster over the problem - and I said as much when it was
> first proposed, pointing out that there's other better solutions.

# cat /sys/devices/mb\:mmci/id                             
01041180
# cat /sys/module/mmci/parameters/fmax                     
12000000                                                                        
# dd if=/dev/mmcblk0 of=/dev/null bs=128k count=100        
100+0 records in                                                                
100+0 records out                                                               
13107200 bytes (13 MB) copied, 9.04218 s, 1.4 MB/s

Of course without the proper error handling, as introduced by your
patch, one will face the same problems with interrupt latency as
previously. But the peak performance seems to noticeably better...

Regards

Pawe?

PS. The FPGA with extended MMCI FIFO will be available on the next VE CD
update, which is going to be released next month.

[PATCH] arm: mmci: Add ARM variant with extended FIFO

[Linus Walleij]

On Fri, Mar 11, 2011 at 6:18 PM, Pawel Moll <pawel.moll@arm.com> wrote:

> New IO FPGA implementation for Versatile Express boards contain
> MMCI (PL180) cell with FIFO extended to 128 words (512 bytes).

OK that's the quick fix, and deeper FIFOs are always nice.

If your hardware engineers are listening, I suggest that hardware clock
gating (as suggested in some earlier mail) plus DMA engine support will
yet improve this by orders of magnitude and also bring you less errors.

Linus Walleij

[PATCH] arm: mmci: Add ARM variant with extended FIFO

[Pawel Moll]

> If your hardware engineers are listening, I suggest that hardware clock
> gating (as suggested in some earlier mail)

I've discussed this idea with our FPGA magician at the very beginning,
but for some reason we wasn't very happy about this... I can't really
blame him - it's not really his job to maintain individual components.
And the original cell authors are long time... em, not dead probably ;-)
just not available.

> plus DMA engine support will
> yet improve this by orders of magnitude and

Some of future test chips will have DMA330 (aka PL330), some won't. It's
all about size, and as most of those chips are some kind of ASIC or
semi-FPGA designs, the area is scarce...

>  also bring you less errors.

One thing is not to generate the errors, second thing is to handle them
correctly. And I must agree with Russell that error handling in the MMC
stack is far from ideal.

Cheers!

Pawe?

[PATCH] arm: mmci: Add ARM variant with extended FIFO

[Chris Ball]

Hi Pawel,

On Fri, Mar 11 2011, Pawel Moll wrote:
> New IO FPGA implementation for Versatile Express boards contain
> MMCI (PL180) cell with FIFO extended to 128 words (512 bytes).
>
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> ---
>  drivers/mmc/host/mmci.c |   13 ++++++++++++-
>  1 files changed, 12 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 2d6de3e..e7197b0 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -66,6 +66,12 @@ static struct variant_data variant_arm = {
>  	.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,
> @@ -1019,10 +1025,15 @@ static int mmci_resume(struct amba_device *dev)
>  static struct amba_id mmci_ids[] = {
>  	{
>  		.id	= 0x00041180,
> -		.mask	= 0x000fffff,
> +		.mask	= 0xff0fffff,
>  		.data	= &variant_arm,
>  	},
>  	{
> +		.id	= 0x01041180,
> +		.mask	= 0xff0fffff,
> +		.data	= &variant_arm_extended_fifo,
> +	},
> +	{
>  		.id	= 0x00041181,
>  		.mask	= 0x000fffff,
>  		.data	= &variant_arm,

Pushed to mmc-next for .39-rc2, thanks.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child