MMC sub-system: SDIO block-mode with increment address issue

MMC sub-system: SDIO block-mode with increment address issue

[Dean Jenkins]

This E-mail is for the attention of Pierre Ossman (MMC sub-system
maintainer)

Hi Pierre,

I've being trying to get SDIO block-mode with incrementing address to
work on an OMAP2430 based reference board with an SDIO card.

Looking at the latest code ( as of 19/11/2007 ) on the mmc-git tree (I'm
not a git expert so I'm not sure how to reference the codebase). I have
a comment to make concerning the following code snippet...

In drivers/mmc/core/sdio_io.c

Function: sdio_io_rw_ext_helper()

<snip>

219                         ret = mmc_io_rw_extended(func->card, write,
220                                 func->num, addr, incr_addr, buf,
221                                 blocks, func->cur_blksize);
222                         if (ret)
223                                 return ret;
224 
225                         remainder -= size;
226                         buf += size;
227                         if (incr_addr)
228                                 addr += size;
229                 }

</snip>

I think the lines

227                         if (incr_addr)
228                                 addr += size;

are incorrect and should be removed. I think the SDIO increment address
parameter relates to the internal operation of the SDIO card and NOT to
the external register address of the FIFO. In other words, I think with
incrementing address enabled in block mode, the register address of the
FIFO in the SDIO function register space will be erroneously changed on
the next block write and will fail (it seems to fail on my card). It
seems strange to change the register address ?

This also relates to byte mode.

What do you think ?

Also, do you have a bug reporting procedure for mmc-git tree ?

Regards,
Dean.


-- 
Dean Jenkins
Embedded Software Engineer
MontaVista Software (UK)
Professional Services Division

Re: MMC sub-system: SDIO block-mode with increment address issue

[Pierre Ossman]

On Mon, 19 Nov 2007 11:44:54 +0000
Dean Jenkins <djenkins@mvista.co.uk> wrote:

> This E-mail is for the attention of Pierre Ossman (MMC sub-system
> maintainer)

A cc helps if you want my attention. ;)

> 
> Hi Pierre,
> 
> I've being trying to get SDIO block-mode with incrementing address to
> work on an OMAP2430 based reference board with an SDIO card.
> 
> Looking at the latest code ( as of 19/11/2007 ) on the mmc-git tree (I'm
> not a git expert so I'm not sure how to reference the codebase). I have
> a comment to make concerning the following code snippet...

git log or git show will give you your current top commit id.
> 
> I think the lines
> 
> 227                         if (incr_addr)
> 228                                 addr += size;
> 
> are incorrect and should be removed. I think the SDIO increment address
> parameter relates to the internal operation of the SDIO card and NOT to
> the external register address of the FIFO. In other words, I think with
> incrementing address enabled in block mode, the register address of the
> FIFO in the SDIO function register space will be erroneously changed on
> the next block write and will fail (it seems to fail on my card). It
> seems strange to change the register address ?
> 

I don't follow. The SDIO specification very clearly defines the behaviour of incrementing address. The referenced code is very much needed to keep that behaviour when we need to split up the transfer.

I assume what you want is multiple transactions with incrementing address, but each transaction restarting at the same base address. In that case you'll have to call the sdio register functions multiple times.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Re: MMC sub-system: SDIO block-mode with increment address issue

[Dean Jenkins]

Hi Pierre,

IMHO the issue is there is no upper bound limit to the valid address
range in sdio_io_rw_ext_helper() in sdio_io.c.

I call sdio_memcpy_toio() as it enables the incrementing address flag in
the CMD53 command but if I try passing too much data then the actual
address of the subsequent CMD53 commands are erroneously incremented out
of range.

The difficulty is the SDIO card can transfer 8 blocks in a single CMD53
command using the incrementing address flag. However
sdio_io_rw_ext_helper() will not prevent the attempt at sending 9 blocks
transferred as 2 CMD53 commands of 8 blocks + 1 block and the last block
goes to the wrong address. This causes a big system crash. I suspect the
SDIO card internally resets and the MMC sub-system can't handle the
error condition.

This means the card driver needs to know that it cannot use
sdio_memcpy_toio() to send any size of data but must ensure it does not
exceed 8 blocks before calling sdio_memcpy_toio(). IMHO this makes the
card driver undesirably tightly coupled with the core driver. OK. I'll
workaround it using multiple calls to sdio_memcpy_toio().

BTW. Is the API for the exported SDIO core functions documented
anywhere ?

Thanks,

Regards,
Dean.


On Tue, 2007-11-20 at 11:58 +0100, Pierre Ossman wrote: 
> On Mon, 19 Nov 2007 11:44:54 +0000
> Dean Jenkins <djenkins@...> wrote:
> 
> > This E-mail is for the attention of Pierre Ossman (MMC sub-system
> > maintainer)
> 
> A cc helps if you want my attention. ;)
> 
> > 
> > Hi Pierre,
> > 
> > I've being trying to get SDIO block-mode with incrementing address to
> > work on an OMAP2430 based reference board with an SDIO card.
> > 
> > Looking at the latest code ( as of 19/11/2007 ) on the mmc-git tree (I'm
> > not a git expert so I'm not sure how to reference the codebase). I have
> > a comment to make concerning the following code snippet...
> 
> git log or git show will give you your current top commit id.
> > 
> > I think the lines
> > 
> > 227                         if (incr_addr)
> > 228                                 addr += size;
> > 
> > are incorrect and should be removed. I think the SDIO increment address
> > parameter relates to the internal operation of the SDIO card and NOT to
> > the external register address of the FIFO. In other words, I think with
> > incrementing address enabled in block mode, the register address of the
> > FIFO in the SDIO function register space will be erroneously changed on
> > the next block write and will fail (it seems to fail on my card). It
> > seems strange to change the register address ?
> > 
> 
> I don't follow. The SDIO specification very clearly defines the behaviour of incrementing address. The referenced code is very much needed to keep that behaviour when we need to split up the transfer.
> 
> I assume what you want is multiple transactions with incrementing address, but each transaction restarting at the same base address. In that case you'll have to call the sdio register functions multiple times.
> 
> Rgds
-- 
Dean Jenkins
Embedded Software Engineer
MontaVista Software (UK)
Professional Services Division

Re: MMC sub-system: SDIO block-mode with increment address issue

[Pierre Ossman]

On Tue, 20 Nov 2007 12:26:11 +0000
Dean Jenkins <djenkins@mvista.co.uk> wrote:

> Hi Pierre,
> 
> IMHO the issue is there is no upper bound limit to the valid address
> range in sdio_io_rw_ext_helper() in sdio_io.c.
> 
> I call sdio_memcpy_toio() as it enables the incrementing address flag in
> the CMD53 command but if I try passing too much data then the actual
> address of the subsequent CMD53 commands are erroneously incremented out
> of range.
> 
> The difficulty is the SDIO card can transfer 8 blocks in a single CMD53
> command using the incrementing address flag. However
> sdio_io_rw_ext_helper() will not prevent the attempt at sending 9 blocks
> transferred as 2 CMD53 commands of 8 blocks + 1 block and the last block
> goes to the wrong address. This causes a big system crash. I suspect the
> SDIO card internally resets and the MMC sub-system can't handle the
> error condition.

I'm afraid I still can't see the problem. If you want to transfer 9 blocks, then the method by which you do so shouldn't matter. So 9, or 8 + 1 should give the same end result.

> 
> This means the card driver needs to know that it cannot use
> sdio_memcpy_toio() to send any size of data but must ensure it does not
> exceed 8 blocks before calling sdio_memcpy_toio(). IMHO this makes the
> card driver undesirably tightly coupled with the core driver. OK. I'll
> workaround it using multiple calls to sdio_memcpy_toio().
> 

Well, the problem is that the abstraction used should work just fine according to how the SDIO standard is defined. The problem seems to be that some card vendors decided to go their own way and started treating the SDIO interface as something other than a simple register interface.

As long as that is the case, there will be a lot of pain supporting these weird cards. We can only debate where to put that pain and what compromises to make.

> BTW. Is the API for the exported SDIO core functions documented
> anywhere ?

Yes, as kerneldoc tags for the relevant functions. Have a look in drivers/core/sdio_io.c if you don't want to build the full document.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Re: MMC sub-system: SDIO block-mode with increment address issue

[Dean Jenkins]

Hi Pierre,

I've looked at the SD Card Association's SDIO Part E1 V2.00
specification concerning the Incrementing Address OP Code field for
CMD53.

The specification indicates that the START ADDRESS is inserted into the
Register Address register field. When the OP Code field has a value of 1
then the during the transfer the IO address is internally incremented
for each byte transferred. This applies to a single CMD53 command.

Looking at the implementation of sdio_io_rw_ext_helper() in sdio_io.c.
This function can send multiple CMD53 commands. My concern is that with
incrementing address mode selected the START ADDRESS is erroneously
changed for subsequent CMD53 commands in the while loop.

What I am trying to say is I don't believe the START ADDRESS should be
changed by the core driver when incrementing address mode is used. I
think incrementing address mode only applies internally to a single
CMD53 command. The SDIO card must physically have a suitable register
present at the START ADDRESS so changing this address to something
dependent on the data size is going to fail I think.

Do you have any evidence that any card drivers will use
sdio_io_rw_ext_helper() in a manner that needs the START ADDRESS to be
changed by the core driver ?

Regards,
Dean.


On Tue, 2007-11-20 at 15:10 +0100, Pierre Ossman wrote:
> On Tue, 20 Nov 2007 12:26:11 +0000
> Dean Jenkins <djenkins@...> wrote:
> 
> > Hi Pierre,
> > 
> > IMHO the issue is there is no upper bound limit to the valid address
> > range in sdio_io_rw_ext_helper() in sdio_io.c.
> > 
> > I call sdio_memcpy_toio() as it enables the incrementing address flag in
> > the CMD53 command but if I try passing too much data then the actual
> > address of the subsequent CMD53 commands are erroneously incremented out
> > of range.
> > 
> > The difficulty is the SDIO card can transfer 8 blocks in a single CMD53
> > command using the incrementing address flag. However
> > sdio_io_rw_ext_helper() will not prevent the attempt at sending 9 blocks
> > transferred as 2 CMD53 commands of 8 blocks + 1 block and the last block
> > goes to the wrong address. This causes a big system crash. I suspect the
> > SDIO card internally resets and the MMC sub-system can't handle the
> > error condition.
> 
> I'm afraid I still can't see the problem. If you want to transfer 9 blocks, then the method by which you do so shouldn't matter. So 9, or 8 + 1 should give the same end result.
> 
> > 
> > This means the card driver needs to know that it cannot use
> > sdio_memcpy_toio() to send any size of data but must ensure it does not
> > exceed 8 blocks before calling sdio_memcpy_toio(). IMHO this makes the
> > card driver undesirably tightly coupled with the core driver. OK. I'll
> > workaround it using multiple calls to sdio_memcpy_toio().
> > 
> 
> Well, the problem is that the abstraction used should work just fine according to how the SDIO standard is defined. The problem seems to be that some card vendors decided to go their own way and started treating the SDIO interface as something other than a simple register interface.
> 
> As long as that is the case, there will be a lot of pain supporting these weird cards. We can only debate where to put that pain and what compromises to make.
> 
> > BTW. Is the API for the exported SDIO core functions documented
> > anywhere ?
> 
> Yes, as kerneldoc tags for the relevant functions. Have a look in drivers/core/sdio_io.c if you don't want to build the full document.
> 
> Rgds
-- 
Dean Jenkins
Embedded Software Engineer
MontaVista Software (UK)
Professional Services Division

Re: MMC sub-system: SDIO block-mode with increment address issue

[Pierre Ossman]

On Wed, 21 Nov 2007 11:57:01 +0000
Dean Jenkins <djenkins@mvista.co.uk> wrote:

> Hi Pierre,
> 
> I've looked at the SD Card Association's SDIO Part E1 V2.00
> specification concerning the Incrementing Address OP Code field for
> CMD53.
> 
> The specification indicates that the START ADDRESS is inserted into the
> Register Address register field. When the OP Code field has a value of 1
> then the during the transfer the IO address is internally incremented
> for each byte transferred. This applies to a single CMD53 command.
> 
> Looking at the implementation of sdio_io_rw_ext_helper() in sdio_io.c.
> This function can send multiple CMD53 commands. My concern is that with
> incrementing address mode selected the START ADDRESS is erroneously
> changed for subsequent CMD53 commands in the while loop.
> 

Since the caller is not supposed to care about the internal operation of sdio_io_rw_ext_helper(), the address increase is a must to maintain a consistent behaviour regardless of what transactions it decides to use.

I.e. if I write 2048 bytes with start address 0x1000, I expect it to do a single write to register 0x1000 through 0x17FF, not and unknown number of writes to some unknown interval.

Now what I suspect you're championing is to support some broken card that treats the address increase bit in CMD53 as "Magic voodoo bit #5" instead of the definition in the SDIO spec. I.e. the only address it cares about is the start address and hence needs it to be the same for each transaction. This kind of blatant disregard for the SDIO register design is of course not what sdio_io_rw_ext_helper() was designed for, and probably never will be.

> What I am trying to say is I don't believe the START ADDRESS should be
> changed by the core driver when incrementing address mode is used. I
> think incrementing address mode only applies internally to a single
> CMD53 command. The SDIO card must physically have a suitable register
> present at the START ADDRESS so changing this address to something
> dependent on the data size is going to fail I think.
> 

The SDIO card must physically have a suitable register present at the entire relevant range, not just the start address. If it doesn't then it isn't following the register interface design of SDIO (having the "increase address" bit would just be silly if the arguments were arbitrary tokens and not part of a consistent address space).

> Do you have any evidence that any card drivers will use
> sdio_io_rw_ext_helper() in a manner that needs the START ADDRESS to be 
> changed by the core driver ?
> 

There are no drivers using "increase address" yet. The ones so far have all used a single byte FIFO port.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Re: MMC sub-system: SDIO block-mode with increment address issue

[Dean Jenkins]

Hi Pierre,

Thanks for explaining.

I guess then I have a crappy SDIO card that needs the "voodoo" bit set
for correct operation. There are other registers close to the START
ADDRESS so it is a FIFO, but the Incrementing Address flag is needed to
read from and write to the FIFO correctly.

Regards,
Dean.


On Wed, 2007-11-21 at 14:08 +0100, Pierre Ossman wrote:
> On Wed, 21 Nov 2007 11:57:01 +0000
> Dean Jenkins <djenkins@...> wrote:
> 
> > Hi Pierre,
> > 
> > I've looked at the SD Card Association's SDIO Part E1 V2.00
> > specification concerning the Incrementing Address OP Code field for
> > CMD53.
> > 
> > The specification indicates that the START ADDRESS is inserted into the
> > Register Address register field. When the OP Code field has a value of 1
> > then the during the transfer the IO address is internally incremented
> > for each byte transferred. This applies to a single CMD53 command.
> > 
> > Looking at the implementation of sdio_io_rw_ext_helper() in sdio_io.c.
> > This function can send multiple CMD53 commands. My concern is that with
> > incrementing address mode selected the START ADDRESS is erroneously
> > changed for subsequent CMD53 commands in the while loop.
> > 
> 
> Since the caller is not supposed to care about the internal operation of sdio_io_rw_ext_helper(), the address increase is a must to maintain a consistent behaviour regardless of what transactions it decides to use.
> 
> I.e. if I write 2048 bytes with start address 0x1000, I expect it to do a single write to register 0x1000 through 0x17FF, not and unknown number of writes to some unknown interval.
> 
> Now what I suspect you're championing is to support some broken card that treats the address increase bit in CMD53 as "Magic voodoo bit #5" instead of the definition in the SDIO spec. I.e. the only address it cares about is the start address and hence needs it to be the same for each transaction. This kind of blatant disregard for the SDIO register design is of course not what sdio_io_rw_ext_helper() was designed for, and probably never will be.
> 
> > What I am trying to say is I don't believe the START ADDRESS should be
> > changed by the core driver when incrementing address mode is used. I
> > think incrementing address mode only applies internally to a single
> > CMD53 command. The SDIO card must physically have a suitable register
> > present at the START ADDRESS so changing this address to something
> > dependent on the data size is going to fail I think.
> > 
> 
> The SDIO card must physically have a suitable register present at the entire relevant range, not just the start address. If it doesn't then it isn't following the register interface design of SDIO (having the "increase address" bit would just be silly if the arguments were arbitrary tokens and not part of a consistent address space).
> 
> > Do you have any evidence that any card drivers will use
> > sdio_io_rw_ext_helper() in a manner that needs the START ADDRESS to be 
> > changed by the core driver ?
> > 
> 
> There are no drivers using "increase address" yet. The ones so far have all used a single byte FIFO port.
> 
> Rgds
-- 
Dean Jenkins
Embedded Software Engineer
MontaVista Software (UK)
Professional Services Division