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

2007-11-21 Thread 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 +
> Dean Jenkins <[EMAIL PROTECTED]> 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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2007-11-21 Thread Pierre Ossman
On Wed, 21 Nov 2007 11:57:01 +
Dean Jenkins <[EMAIL PROTECTED]> 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 maintainerhttp://www.kernel.org
  PulseAudio, core developer  http://pulseaudio.org
  rdesktop, core developer  http://www.rdesktop.org
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2007-11-21 Thread 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 +
> Dean Jenkins <[EMAIL PROTECTED]> 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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2007-11-21 Thread 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 +
 Dean Jenkins [EMAIL PROTECTED] 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

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2007-11-21 Thread Pierre Ossman
On Wed, 21 Nov 2007 11:57:01 +
Dean Jenkins [EMAIL PROTECTED] 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 maintainerhttp://www.kernel.org
  PulseAudio, core developer  http://pulseaudio.org
  rdesktop, core developer  http://www.rdesktop.org
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2007-11-21 Thread 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 +
 Dean Jenkins [EMAIL PROTECTED] 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

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2007-11-20 Thread Pierre Ossman
On Tue, 20 Nov 2007 14:52:37 +
Dean Jenkins <[EMAIL PROTECTED]> wrote:

> Hi Pierre,
> 
> My card driver needed to set the R/W E4MI bit in the Card Capability
> register (0x08) in CCCR (function 0). Perhaps it is unnecessary ?
> 

That bit is pointless given the current design of the MMC layer, so it 
shouldn't be necessary.

> BTW. It is easy to for the card driver to access function 0 registers by
> doing the following...
> 

Of course. Any driver can do whatever it pleases since it is in kernel space. 
But you won't make any friends if you keep trying to bypass every system in the 
kernel. ;)

(Not to mention you'll have a lot more work on your hands.)

Rgds
-- 
 -- Pierre Ossman

  Linux kernel, MMC maintainerhttp://www.kernel.org
  PulseAudio, core developer  http://pulseaudio.org
  rdesktop, core developer  http://www.rdesktop.org
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2007-11-20 Thread Dean Jenkins
Hi Pierre,

My card driver needed to set the R/W E4MI bit in the Card Capability
register (0x08) in CCCR (function 0). Perhaps it is unnecessary ?

BTW. It is easy to for the card driver to access function 0 registers by
doing the following...

...
int old_num = func->num;   /* note the current function number */

/* force access to function 0 area */
func->num = 0;

/* enable the e4mi in the card capability register */
x = sdio_readb(func, 0x08, );

if ( ret == 0 ) {
x |= 0x20;
sdio_writeb(func, x, 0x08, );
}

/* restore to original function number */
func->num = old_num; 

...

Regards,
Dean.


On Tue, 2007-11-20 at 11:51 +0100, Pierre Ossman wrote:
> On Tue, 20 Nov 2007 10:28:53 +
> Dean Jenkins <[EMAIL PROTECTED]> wrote:
> 
> > Hi Pierre,
> > 
> > I've managed to find your E-mail address in the Linux kernel mailing
> > list. I hope it is OK to directly E-mail you ?
> > 
> 
> You should also find my address in the MAINTAINERS file, which is where you 
> should look for contact info. But yes, you can contact me directly. I prefer 
> that you also add a relevant mailing list though.
> 
> > I work for MontaVista Software in the UK. I guess you know that
> > MontaVista are using the MMC/SDIO sub-system in their latest Mobilinux
> > 5.0 product ? It is using a 2.6.21 kernel.
> > 
> > Yesterday, I sent an E-mail to you (attached) to the Linux kernel
> > mailing list. Do you have any comments to make ?
> > 
> 
> Didn't notice it. I'll have a look.
> 
> > Do you have any bug reporting facility for the MMC/SDIO sub-system ? I
> > would like to report some SDIO API limitations for the SDIO card drivers
> > that I needed to workaround. For example, my SDIO driver needs to modify
> > the contents of the Card Capability register in function 0 to enable
> > block-mode support but the card driver has been restricted to only be
> > able to use function 1. There are other registers as well that the card
> > driver needs access to.
> > 
> 
> That is by design (as you probably can tell). Letting function drivers touch 
> card global stuff under the feet of the SDIO core is a big layering violation 
> and bound to screw things up eventually.
> 
> Could you explain more in detail what it is you need to fiddle with in 
> function 0 and why?
> 
> Rgds
-- 
Dean Jenkins
Embedded Software Engineer
MontaVista Software (UK)
Professional Services Division

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2007-11-20 Thread Pierre Ossman
On Tue, 20 Nov 2007 12:26:11 +
Dean Jenkins <[EMAIL PROTECTED]> 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 maintainerhttp://www.kernel.org
  PulseAudio, core developer  http://pulseaudio.org
  rdesktop, core developer  http://www.rdesktop.org
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2007-11-20 Thread 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 +
> Dean Jenkins <[EMAIL PROTECTED]> 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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2007-11-20 Thread Pierre Ossman
On Mon, 19 Nov 2007 11:44:54 +
Dean Jenkins <[EMAIL PROTECTED]> 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 maintainerhttp://www.kernel.org
  PulseAudio, core developer  http://pulseaudio.org
  rdesktop, core developer  http://www.rdesktop.org
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2007-11-20 Thread Pierre Ossman
On Mon, 19 Nov 2007 11:44:54 +
Dean Jenkins [EMAIL PROTECTED] 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 maintainerhttp://www.kernel.org
  PulseAudio, core developer  http://pulseaudio.org
  rdesktop, core developer  http://www.rdesktop.org
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2007-11-20 Thread 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 +
 Dean Jenkins [EMAIL PROTECTED] 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

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2007-11-20 Thread Pierre Ossman
On Tue, 20 Nov 2007 12:26:11 +
Dean Jenkins [EMAIL PROTECTED] 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 maintainerhttp://www.kernel.org
  PulseAudio, core developer  http://pulseaudio.org
  rdesktop, core developer  http://www.rdesktop.org
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2007-11-20 Thread Dean Jenkins
Hi Pierre,

My card driver needed to set the R/W E4MI bit in the Card Capability
register (0x08) in CCCR (function 0). Perhaps it is unnecessary ?

BTW. It is easy to for the card driver to access function 0 registers by
doing the following...

...
int old_num = func-num;   /* note the current function number */

/* force access to function 0 area */
func-num = 0;

/* enable the e4mi in the card capability register */
x = sdio_readb(func, 0x08, ret);

if ( ret == 0 ) {
x |= 0x20;
sdio_writeb(func, x, 0x08, ret);
}

/* restore to original function number */
func-num = old_num; 

...

Regards,
Dean.


On Tue, 2007-11-20 at 11:51 +0100, Pierre Ossman wrote:
 On Tue, 20 Nov 2007 10:28:53 +
 Dean Jenkins [EMAIL PROTECTED] wrote:
 
  Hi Pierre,
  
  I've managed to find your E-mail address in the Linux kernel mailing
  list. I hope it is OK to directly E-mail you ?
  
 
 You should also find my address in the MAINTAINERS file, which is where you 
 should look for contact info. But yes, you can contact me directly. I prefer 
 that you also add a relevant mailing list though.
 
  I work for MontaVista Software in the UK. I guess you know that
  MontaVista are using the MMC/SDIO sub-system in their latest Mobilinux
  5.0 product ? It is using a 2.6.21 kernel.
  
  Yesterday, I sent an E-mail to you (attached) to the Linux kernel
  mailing list. Do you have any comments to make ?
  
 
 Didn't notice it. I'll have a look.
 
  Do you have any bug reporting facility for the MMC/SDIO sub-system ? I
  would like to report some SDIO API limitations for the SDIO card drivers
  that I needed to workaround. For example, my SDIO driver needs to modify
  the contents of the Card Capability register in function 0 to enable
  block-mode support but the card driver has been restricted to only be
  able to use function 1. There are other registers as well that the card
  driver needs access to.
  
 
 That is by design (as you probably can tell). Letting function drivers touch 
 card global stuff under the feet of the SDIO core is a big layering violation 
 and bound to screw things up eventually.
 
 Could you explain more in detail what it is you need to fiddle with in 
 function 0 and why?
 
 Rgds
-- 
Dean Jenkins
Embedded Software Engineer
MontaVista Software (UK)
Professional Services Division

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2007-11-20 Thread Pierre Ossman
On Tue, 20 Nov 2007 14:52:37 +
Dean Jenkins [EMAIL PROTECTED] wrote:

 Hi Pierre,
 
 My card driver needed to set the R/W E4MI bit in the Card Capability
 register (0x08) in CCCR (function 0). Perhaps it is unnecessary ?
 

That bit is pointless given the current design of the MMC layer, so it 
shouldn't be necessary.

 BTW. It is easy to for the card driver to access function 0 registers by
 doing the following...
 

Of course. Any driver can do whatever it pleases since it is in kernel space. 
But you won't make any friends if you keep trying to bypass every system in the 
kernel. ;)

(Not to mention you'll have a lot more work on your hands.)

Rgds
-- 
 -- Pierre Ossman

  Linux kernel, MMC maintainerhttp://www.kernel.org
  PulseAudio, core developer  http://pulseaudio.org
  rdesktop, core developer  http://www.rdesktop.org
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2007-11-19 Thread 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()



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 }



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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2007-11-19 Thread 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

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/