[PATCH 1/4] mmc: mmci: Bugfix in pio read for small packets

[PATCH 1/4] mmc: mmci: Bugfix in pio read for small packets

[Ulf Hansson]

From: Stefan Nilsson XK <stefan.xk.nilsson@stericsson.com>

Corrects a bug in pio read when reading packets < 4 bytes. These
small packets are only relevant for SDIO transfers.

Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
Signed-off-by: Stefan Nilsson XK <stefan.xk.nilsson@stericsson.com>
---
 drivers/mmc/host/mmci.c |   19 ++++++++++++++++++-
 1 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 40e4c05..8831b2f 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -776,7 +776,24 @@ static int mmci_pio_read(struct mmci_host *host, char *buffer, unsigned int rema
 		if (count <= 0)
 			break;
 
-		readsl(base + MMCIFIFO, ptr, count >> 2);
+		/*
+		 * SDIO especially may want to receive something that is
+		 * not divisible by 4 (as opposed to card sectors
+		 * etc). Therefore make sure we always read the last bytes
+		 * out of the FIFO.
+		 */
+		switch (count) {
+		case 1:
+		case 3:
+			readsb(base + MMCIFIFO, ptr, count);
+			break;
+		case 2:
+			readsw(base + MMCIFIFO, ptr, 1);
+			break;
+		default:
+			readsl(base + MMCIFIFO, ptr, count >> 2);
+			break;
+		}
 
 		ptr += count;
 		remain -= count;
-- 
1.7.5.4

[PATCH 1/4] mmc: mmci: Bugfix in pio read for small packets

[Russell King - ARM Linux]

On Tue, Sep 27, 2011 at 09:46:08AM +0200, Ulf Hansson wrote:
> From: Stefan Nilsson XK <stefan.xk.nilsson@stericsson.com>
> 
> Corrects a bug in pio read when reading packets < 4 bytes. These
> small packets are only relevant for SDIO transfers.

Does this even work?  From the MMCI spec, I see no way for the MMCI
peripheral to know the size of the read/write on the APB bus.

The APB bus signals the MMCI uses are:

PCLK - APB bus clock
PRESETn - APB bus reset
PADDR[11:2] - APB bus address
PSEL - APB bus peripheral select
PENABLE - APB bus enable
PWRITE - APB bus write signal
PWDATA[31:0] - APB bus write data
PRDATA[31:0] - APB bus read data

As you can see, nothing in that set indicates whether it's an 8-bit,
16-bit or 32-bit access.

Moreover, if you read the MMCIFifoCnt register writeup:

  The MMCIFifoCnt register contains the remaining number of words to be
  written to or read from the FIFO. The FIFO counter loads the value
  from the data length register (see Data length register, MMCIDataLength
  on page 3-11) when the Enable bit is set in the data control register.
  If the data length is not word aligned (multiple of 4), the remaining
  1 to 3 bytes are regarded as a word.

This suggests that we should be reading a 32-bit word and then storing
the relevant bytes from it.

The other thing which concerns me is that the MMCI (ARM Ltd one at least)
only supports power-of-two block sizes.  So requesting a transfer of a
single block with a block size of 3 bytes is not supported by the ARM Ltd
MMCI.  (The way you end up with 1 to 3 bytes being received with ARM's
MMCI is if you're using a streaming transfer.)

The last thing I don't like about this patch is that this code is in a
really hot path - one which is absolutely critical for things to work -
and the need for the condition to be dealt with is only at the end of a
transfer, not each time the FIFO needs emptying.

Bear in mind that there are platforms with the ARM MMCI which must read
the data within a certain time or suffer overruns, and which have either
totally broken and useless DMA or no DMA capability at all (which are
the only platforms I have with a MMCI on.)

[PATCH 1/4] mmc: mmci: Bugfix in pio read for small packets

[Stefan Nilsson XK]

Hi Russel,

On 10/01/2011 06:09 PM, Russell King - ARM Linux wrote:
> Does this even work?  From the MMCI spec, I see no way for the MMCI
> peripheral to know the size of the read/write on the APB bus.
>
> The APB bus signals the MMCI uses are:
>
> PCLK - APB bus clock
> PRESETn - APB bus reset
> PADDR[11:2] - APB bus address
> PSEL - APB bus peripheral select
> PENABLE - APB bus enable
> PWRITE - APB bus write signal
> PWDATA[31:0] - APB bus write data
> PRDATA[31:0] - APB bus read data
>
> As you can see, nothing in that set indicates whether it's an 8-bit,
> 16-bit or 32-bit access.

> Moreover, if you read the MMCIFifoCnt register writeup:
>
>    The MMCIFifoCnt register contains the remaining number of words to be
>    written to or read from the FIFO. The FIFO counter loads the value
>    from the data length register (see Data length register, MMCIDataLength
>    on page 3-11) when the Enable bit is set in the data control register.
>    If the data length is not word aligned (multiple of 4), the remaining
>    1 to 3 bytes are regarded as a word.
>
> This suggests that we should be reading a 32-bit word and then storing
> the relevant bytes from it.

Hmm, that is true. I will try to rework the patch to skip the case and 
only do 32 bit reads. We do however need a patch since the current 
"count" calculation is wrong (more info below). But it does in fact work.

> The other thing which concerns me is that the MMCI (ARM Ltd one at least)
> only supports power-of-two block sizes.  So requesting a transfer of a
> single block with a block size of 3 bytes is not supported by the ARM Ltd
> MMCI.  (The way you end up with 1 to 3 bytes being received with ARM's
> MMCI is if you're using a streaming transfer.)

This is true for the standard ARM block. We have however done some 
modifications of the standard block and added support for SDIO which 
also implies streaming transfers which can be non power of 2. So with 
the current implementation:

	readsl(base + MMCIFIFO, ptr, count >> 2);

when we get a request to read 3, 2 or 1 bytes (which our WLAN driver 
actually requests from time to time), this will result in reading 0 
words which is not correct. This was the original problem, but I might 
have "overdone" the solution a bit. I will upload a new patch which 
solves the original problem in a better way.

> The last thing I don't like about this patch is that this code is in a
> really hot path - one which is absolutely critical for things to work -
> and the need for the condition to be dealt with is only at the end of a
> transfer, not each time the FIFO needs emptying.
>
> Bear in mind that there are platforms with the ARM MMCI which must read
> the data within a certain time or suffer overruns, and which have either
> totally broken and useless DMA or no DMA capability at all (which are
> the only platforms I have with a MMCI on.)

I will keep this in mind. We are fortunately blessed with a working DMA 
in our platform, but for small SDIO transfers there is no point in 
setting up a DMA job which is when we fall back to PIO mode.

Best Regards

Stefan Nilsson

[PATCH 1/4] mmc: mmci: Bugfix in pio read for small packets

[Ulf Hansson]

Stefan NILSSON9 wrote:
> Hi Russel,
> 
> On 10/01/2011 06:09 PM, Russell King - ARM Linux wrote:
>> Does this even work?  From the MMCI spec, I see no way for the MMCI
>> peripheral to know the size of the read/write on the APB bus.
>>
>> The APB bus signals the MMCI uses are:
>>
>> PCLK - APB bus clock
>> PRESETn - APB bus reset
>> PADDR[11:2] - APB bus address
>> PSEL - APB bus peripheral select
>> PENABLE - APB bus enable
>> PWRITE - APB bus write signal
>> PWDATA[31:0] - APB bus write data
>> PRDATA[31:0] - APB bus read data
>>
>> As you can see, nothing in that set indicates whether it's an 8-bit,
>> 16-bit or 32-bit access.
> 
>> Moreover, if you read the MMCIFifoCnt register writeup:
>>
>>    The MMCIFifoCnt register contains the remaining number of words to be
>>    written to or read from the FIFO. The FIFO counter loads the value
>>    from the data length register (see Data length register, MMCIDataLength
>>    on page 3-11) when the Enable bit is set in the data control register.
>>    If the data length is not word aligned (multiple of 4), the remaining
>>    1 to 3 bytes are regarded as a word.
>>
>> This suggests that we should be reading a 32-bit word and then storing
>> the relevant bytes from it.
> 
> Hmm, that is true. I will try to rework the patch to skip the case and 
> only do 32 bit reads. We do however need a patch since the current 
> "count" calculation is wrong (more info below). But it does in fact work.
> 
>> The other thing which concerns me is that the MMCI (ARM Ltd one at least)
>> only supports power-of-two block sizes.  So requesting a transfer of a
>> single block with a block size of 3 bytes is not supported by the ARM Ltd
>> MMCI.  (The way you end up with 1 to 3 bytes being received with ARM's
>> MMCI is if you're using a streaming transfer.)
> 
> This is true for the standard ARM block. We have however done some 
> modifications of the standard block and added support for SDIO which 
> also implies streaming transfers which can be non power of 2. So with 
> the current implementation:
> 
> 	readsl(base + MMCIFIFO, ptr, count >> 2);
> 
> when we get a request to read 3, 2 or 1 bytes (which our WLAN driver 
> actually requests from time to time), this will result in reading 0 
> words which is not correct. This was the original problem, but I might 
> have "overdone" the solution a bit. I will upload a new patch which 
> solves the original problem in a better way.
> 
>> The last thing I don't like about this patch is that this code is in a
>> really hot path - one which is absolutely critical for things to work -
>> and the need for the condition to be dealt with is only at the end of a
>> transfer, not each time the FIFO needs emptying.

In this hot path we already do a read of the FIFOCNT register for every 
loop in pio_read, won't this sometimes cause caches to flush and 
similar, thus cost quite a lot - at least a lot more than executing a 
switch/if sentence like Stefan added? Or do I miss something?

I were also thinking of a possible optimization of minimizing the total 
numbers of reads of the FIFOCNT register in pio_read. Basically we can 
make use of the RXFIFOHALFFULL irq/status to know when there is a 
"burst" available in the FIFO. Do you think this will be feasible for 
the ARM MMCI Pl18x IP as well? I mean the consequence of using 
RXFIFOHALFFULL will be less numbers of IRQ raised and then when reading 
data from the FIFO it will be done in larger chunks.

>>
>> Bear in mind that there are platforms with the ARM MMCI which must read
>> the data within a certain time or suffer overruns, and which have either
>> totally broken and useless DMA or no DMA capability at all (which are
>> the only platforms I have with a MMCI on.)

We could make use of the "likely" makro to make compiler optimizations 
of the code, is that a way forward do you think?

> 
> I will keep this in mind. We are fortunately blessed with a working DMA 
> in our platform, but for small SDIO transfers there is no point in 
> setting up a DMA job which is when we fall back to PIO mode.
> 
> Best Regards
> 
> Stefan Nilsson
> 

[PATCH 1/4] mmc: mmci: Bugfix in pio read for small packets

[Russell King - ARM Linux]

On Fri, Oct 07, 2011 at 03:38:44PM +0200, Ulf Hansson wrote:
> In this hot path we already do a read of the FIFOCNT register for every  
> loop in pio_read, won't this sometimes cause caches to flush and  
> similar, thus cost quite a lot - at least a lot more than executing a  
> switch/if sentence like Stefan added? Or do I miss something?

The read of FIFOCNT just reads the counter and does nothing more.  Why
do you think it causes a cache flush, and what cache do you think would
be flushed?

> I were also thinking of a possible optimization of minimizing the total  
> numbers of reads of the FIFOCNT register in pio_read. Basically we can  
> make use of the RXFIFOHALFFULL irq/status to know when there is a  
> "burst" available in the FIFO. Do you think this will be feasible for  
> the ARM MMCI Pl18x IP as well? I mean the consequence of using  
> RXFIFOHALFFULL will be less numbers of IRQ raised and then when reading  
> data from the FIFO it will be done in larger chunks.

We read the FIFOCNT register to allow us to empty as much data from the
FIFO as we possibly can at each interrupt.  We know that it's going to
be at least half full at that time because that's the interrupt which is
triggering us to do the read (except for the final few words).  What we
want to do is not just read half the FIFO, but everything that it
contains.  Hence why we read FIFOCNT.

> We could make use of the "likely" makro to make compiler optimizations  
> of the code, is that a way forward do you think?

That only helps if you look at the assembled code and inspect what the
compiler is doing.  If it's already correctly guessing the best paths,
then the likely macro does nothing for you.

But first, you need to fix your code so you're only reading 32-bit
quantities from the FIFO register.

[PATCH 1/4] mmc: mmci: Bugfix in pio read for small packets

[Stefan Nilsson XK]

On 10/07/2011 09:11 PM, Russell King - ARM Linux wrote:
> But first, you need to fix your code so you're only reading 32-bit
> quantities from the FIFO register.

Hi Russel, what to you think of doing it this way instead:

		/*
		 * SDIO especially may want to send something that is
		 * not divisible by 4 (as opposed to card sectors
		 * etc), and the FIFO only accept full 32-bit reads.
		 */
		if (count < 4) {
			unsigned char buf[4];
			readsl(base + MMCIFIFO, buf, 1);
			memcpy(ptr, buf, count);
		}
		else
			readsl(base + MMCIFIFO, ptr, count >> 2);

This makes sure we only access the FIFO in a 32 bit way, and the only 
overhead for the "standard" case (count >= 4) is the "if" clause. I have 
verified this to work with our WLAN driver.

Best Regards

Stefan Nilsson

[PATCH 1/4] mmc: mmci: Bugfix in pio read for small packets

[Ulf Hansson]

Russell King - ARM Linux wrote:
> On Tue, Sep 27, 2011 at 09:46:08AM +0200, Ulf Hansson wrote:
>> From: Stefan Nilsson XK <stefan.xk.nilsson@stericsson.com>
>>
>> Corrects a bug in pio read when reading packets < 4 bytes. These
>> small packets are only relevant for SDIO transfers.
> 
> Does this even work?  From the MMCI spec, I see no way for the MMCI
> peripheral to know the size of the read/write on the APB bus.
> 

Hi Russel,

My feeling is that since discussing this patch, the rest of the patches 
in this serie and are kind of "put on hold". The reason for keeping them 
together is more of functional purpose. I hope you can have a look at 
them anyway, they shall be possible to apply without this one.

If you prefer we can split up this patchserie...

BR
Ulf Hansson

[PATCH 1/4] mmc: mmci: Bugfix in pio read for small packets

[Russell King - ARM Linux]

On Fri, Oct 07, 2011 at 03:45:48PM +0200, Ulf Hansson wrote:
> My feeling is that since discussing this patch, the rest of the patches  
> in this serie and are kind of "put on hold". The reason for keeping them  
> together is more of functional purpose. I hope you can have a look at  
> them anyway, they shall be possible to apply without this one.

Well, the patch system says that each one depends on the previous one,
and the first one in the series contains the PIO read thing.

Do 7108/1 onwards depend on 7106/1 and 7107/1 ?

[PATCH 1/4] mmc: mmci: Bugfix in pio read for small packets

[Linus Walleij]

On Sat, Oct 8, 2011 at 11:10 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Oct 07, 2011 at 03:45:48PM +0200, Ulf Hansson wrote:
>
> Well, the patch system says that each one depends on the previous one,
> and the first one in the series contains the PIO read thing.
>
> Do 7108/1 onwards depend on 7106/1 and 7107/1 ?

Probably my fault, since I was helping Ulf out with the patch tracker
introduction.

The patches should be pretty much semantically orthogonal as
far as I can tell, the only reason dependency was stated that way
was that I couldn't tell from head-parsing whether there were
syntactical dependencies. (And wanted to avoid the annoyance
of non-applying patches...)

So if they apply, they are independent AFAICT.

Yours,
Linus Walleij

[PATCH 1/4] mmc: mmci: Bugfix in pio read for small packets

[Ulf Hansson]

Linus Walleij wrote:
> On Sat, Oct 8, 2011 at 11:10 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Fri, Oct 07, 2011 at 03:45:48PM +0200, Ulf Hansson wrote:
>>
>> Well, the patch system says that each one depends on the previous one,
>> and the first one in the series contains the PIO read thing.
>>
>> Do 7108/1 onwards depend on 7106/1 and 7107/1 ?
> 
> Probably my fault, since I was helping Ulf out with the patch tracker
> introduction.
> 
> The patches should be pretty much semantically orthogonal as
> far as I can tell, the only reason dependency was stated that way
> was that I couldn't tell from head-parsing whether there were
> syntactical dependencies. (And wanted to avoid the annoyance
> of non-applying patches...)
> 
> So if they apply, they are independent AFAICT.

7108/1 and 7109/1, have real dependencies. Otherwise there none.

BR
Ulf Hansson

[PATCH 1/4] mmc: mmci: Bugfix in pio read for small packets

[Russell King - ARM Linux]

On Mon, Oct 10, 2011 at 10:23:02AM +0200, Ulf Hansson wrote:
> Linus Walleij wrote:
>> On Sat, Oct 8, 2011 at 11:10 AM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>> On Fri, Oct 07, 2011 at 03:45:48PM +0200, Ulf Hansson wrote:
>>>
>>> Well, the patch system says that each one depends on the previous one,
>>> and the first one in the series contains the PIO read thing.
>>>
>>> Do 7108/1 onwards depend on 7106/1 and 7107/1 ?
>>
>> Probably my fault, since I was helping Ulf out with the patch tracker
>> introduction.
>>
>> The patches should be pretty much semantically orthogonal as
>> far as I can tell, the only reason dependency was stated that way
>> was that I couldn't tell from head-parsing whether there were
>> syntactical dependencies. (And wanted to avoid the annoyance
>> of non-applying patches...)
>>
>> So if they apply, they are independent AFAICT.
>
> 7108/1 and 7109/1, have real dependencies. Otherwise there none.

Ok, I assume that those depend on the first two patches.

So, I tried applying 7110/1 .. 7112/1 but the first rejects because it
doesn't have the non-power-of-2 support patch applied (7107/1).  And
it seems sensible that 7107/1 depends on 7106/1 (the PIO patch) which
I believe to be a problem.

Therefore, I don't think any of these can be applied without the
initial PIO patch.

One thing I haven't yet mentioned is about the non-power-of-2 support -
surely this can only be supported if blksz_datactrl16 is set?  If so,
shouldn't it key off that?  I don't see how it could otherwise support
non-power of 2 block sizes with just a log2 of the block size programmed
into the data control register.

[PATCH 1/4] mmc: mmci: Bugfix in pio read for small packets

[Ulf Hansson]

>>> So if they apply, they are independent AFAICT.
>> 7108/1 and 7109/1, have real dependencies. Otherwise there none.
> 
> Ok, I assume that those depend on the first two patches.
> 
> So, I tried applying 7110/1 .. 7112/1 but the first rejects because it
> doesn't have the non-power-of-2 support patch applied (7107/1).  And
> it seems sensible that 7107/1 depends on 7106/1 (the PIO patch) which
> I believe to be a problem.
> 

I see the problem.

The patches from the patch serie "Clarify code paths for how to modify 
the power register" are independent from this "SDIO serie". Although the 
patches has been based upon the patches from the SDIO serie, which is as 
you say the reason to why they not apply cleanly.

I will upload a second version of the serie "Clarify code paths for how 
to modify the power register" which is NOT based on the "SDIO serie". 
Then when can discuss each serie separately.

> Therefore, I don't think any of these can be applied without the
> initial PIO patch.
> 
> One thing I haven't yet mentioned is about the non-power-of-2 support -
> surely this can only be supported if blksz_datactrl16 is set?  If so,
> shouldn't it key off that?  I don't see how it could otherwise support
> non-power of 2 block sizes with just a log2 of the block size programmed
> into the data control register.
> 

Your observation is correct, but at the same time do we really need to 
have such a dependency check in the code? The hardware setup is handled 
in the variant struct completely, so we believe that should be enough.