Possible bug in libchip/i2c/spi-sd-card.c [Finally works!]

[Gene Smith]

Chris Johns wrote, On 01/15/2009 07:20 PM:
> Gene Smith wrote:
>> I don't have a way to check or verify this right now but I may see the 
>> reason for the problem I am having doing shell copies on my MMC cards.
>>
>> In my previous email 
>> http://www.rtems.com/ml/rtems-users/2009/january/msg00078.html
>> I mentioned that 4 sectors (flash blocks) were written sequentially that 
>> should have been scattered about in flash. Only the first one was at the 
>> right place (the first FAT entry).
>>
>> Also in this email, before I had a clue as to what I was seeing, I 
>> reported 4 multiple blocks being written on file copy and the last 
>> contained the copied file data: 
>> data:http://www.rtems.com/ml/rtems-users/2009/january/msg00040.html
>>
>> In both cases I think I am seeing the same thing.
>>
>> The multiple block write occurs in spi-sd-card.c in function 
>> sd_card_disk_block_write(..., r) where prm r is a struct/array of 
>> scattered buffers (each at a random address). There are 4 block to be 
>> written at address 0x200 (main FAT), 0x1ec0 (redundant FAT), 0x3d600 
>> (root dir entry) and 0x41600 (cluster 2 with file data). Since there are 
>> 4 buffers (r->bufnum > 1) in the scatter array, a multiple block write 
>> to MMC/SD is selected. The multiple write sets the starting address to 
>> the first block (0x200) and writes all 4 blocks back to back beginning 
>> at that address, which is wrong in this case. That's why my blocks end 
>> up in the FAT area instead of at the right place.
>>
>> So, unless the blocks are known to be truly sequential and not scattered 
>> or random, only then should the multiple blocks method be used. I don't 
>> know if there is a way to know without parsing the scatter array each 
>> time. So it might be better (but slower) to always just use single block 
>> write method multiple times if bufnum > 1.
>>
> 
> Does the driver set the multi-sector continuous flag when the cache asks for 
> capabilities ?

It tries but still doesn't set the flag. There is a bug in the ioctl for 
sd card I think.

> 
> The driver in CVS has the code in 'sd_card_disk_ioctl':

My 4.9.1 code appears the same as the cvs HEAD for this function, I think.

> 
> 	case RTEMS_BLKDEV_CAPABILITIES:
>   		*((uint32_t*) arg)  = RTEMS_BLKDEV_CAP_MULTISECTOR_CONT;
>   		return 0;
> 
> If this is being set and you are getting sectors that are not continuous I 
> would like to know.

I saw this feature too and it looked like this should cause only writes 
of single blocks and no multiple blocks. But I do see multiple block 
being written (block 33, 278, 523 and a 4th one I didn't write down 
which was the sector of the data cluster). But when the sd card is 
init'd the function sd_card_disk_ioctl() *is* called but the 
capabilities are never set due to the enclosing "if (req == 
RTEMS_BLKIO_REQUEST) {" around the case. The check for 
req==RTEMS_BLKDEV_CAPABILITES needs to moved outside the case like this:

  	if (req == RTEMS_BLKIO_REQUEST) {
  		rtems_device_minor_number minor = 				rtems_filesystem_dev_minor_t( 
dev);
		sd_card_driver_entry *e = &sd_card_driver_table [minor];
  		rtems_blkdev_request *r = (rtems_blkdev_request *) arg;
  		switch (r->req) {
  			case RTEMS_BLKDEV_REQ_READ:
  				return sd_card_disk_block_read( e, r);
  			case RTEMS_BLKDEV_REQ_WRITE:
  				return sd_card_disk_block_write( e, r);
  			default:
  				errno = EBADRQC;
  				return -1;
  		}
	} else if (req == RTEMS_BLKDEV_CAPABILITIES) {
		/* causes bdbuf to only do single block writes to flash */
  		*((uint32_t*) arg)  = RTEMS_BLKDEV_CAP_MULTISECTOR_CONT;
  		return 0;
  	} else {
  		errno = EBADRQC;
  		return -1;
  	}

When I change this I am finally able to copy a file on my MMC cards!!!!
For hard drives (ata.c) the equivalent ioctl fn looks OK so that is why 
other FATxx users have not seen this.

Also, don't have to do a cat of the file for it to show up since the 
blocks are now all written properly to MMC/SD one at a time.
-gene

> 
> Regards
> Chris
> 
>> Again, this is a theory that has yet to be verified.

Yes, I think I have verified it.

>>
>> -gene