spi_mmc bus concurrency fix

current SPI framework does not allow the MMC operation share the SPI bus with other SPI devices.

--
/* NOTES:
*
* - For now, we won't try to interoperate with a real mmc/sd/sdio

 *   controller, although some of them do have hardware support for
 *   SPI protocol.  The main reason for such configs would be mmc-ish
 *   cards like DataFlash, which don't support that "native" protocol.
 *
 *   We don't have a "DataFlash/MMC/SD/SDIO card slot" abstraction to
 *   switch between driver stacks, and in any case if "native" mode
 *   is available, it will be faster and hence preferable.
 *
 * - MMC depends on a different chipselect management policy than the
 *   SPI interface currently supports for shared bus segments:  it needs
 *   to issue multiple spi_message requests with the chipselect active,
 *   using the results of one message to decide the next one to issue.

*
* Pending updates to the programming interface, this driver expects
* that it not share the bus with other drivers (precluding conflicts).
*
* - We tell the controller to keep the chipselect active from the
* beginning of an mmc_host_ops.request until the end. So beware
* of SPI controller drivers that mis-handle the cs_change flag!
*
* However, many cards seem OK with chipselect flapping up/down
* during that time ... at least on unshared bus segments.
*/
---

So we can investigate with the SPI framework update.

I will copy this email to SPI mail list and mmc_spi maintainers for help. Thanks Hans,
-Bryan

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

I'm not following something here. Is there another MMC-over-SPI driver being developed? Why not use the current one?

Right, and there are plenty of other similar examples. The current drivers/mmc/host/mmc_spi.c code handles that, if the underlying SPI controller driver handles the chipselecct as expected.

As noted above. There are two issues: (a) keeping other drivers off the bus, and (b) making sure chipselect doesn't go inactive at the wrong place.

The *current* assumption is that the bus will be unshared, so (a) isn't an issue ... and that the SPI master controller driver will implement the chipselect hinting mechanism (spi_transfer.cs_change set on the last transfer).

We do need better solutions for (a) though.

- Dave

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

a MMC-over-SPI driver for the Blackfin was developed by Hans long before there was a SPI framework in the kernel let alone the MMC-over-SPI driver that is in there now. i suggested he look at integrating anything from his driver into the one that's in mainline now and while that'll happen at some point, it looks like he can get the issues he has with the old one sorted out sooner. -mike

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Yes, Hans developed a MMC-over-SPI driver before the mainline implementation. His SPI_MMC driver has been in Blackfin local repo for a long time.
http://docs.blackfin.uclinux.org/doku.php?id=mmc_driver&s=spi

At beginning this driver does not use the SPI framework API,
now Hans is working on to more his driver to use kernel SPI framework API. Then He met the bus sharing issue.

As we discussed here, the main issue is
a) upper level spi device driver can submit I/O request to spi-core, spi-core will schedule the real read/write I/O operation to lower level hardware
b) some spi devices require to make sure atomically bus I/O transcation which can not be interrupt by other spi devices sharing the same bus.
spi-core should provide some API to meet this requirement.

-Bryan

That would preclude using chained DMA requests in those cases, but presumably that'd be OK in this context.

Just a few? If you're assuming it would add a new spi_transfer after the existing ones in that message ... that could add up to quite a lot of additional transfers, presumably ones that are dynamically allocated.

I've seen some MMC/SD cards need quite a lot of polling there...

That is, one queue per device. Yes, more complicated than one shared between devices. I was just pointing that out as an option that could be implemented transparently ... albeit not portably.

It doesn't seem like the right approach to me. In the specific use case of an MMC interface, three'd be a *LOT* of those callbacks, each adding another transfer to the current message.

And that breaks assumptions like being able to validate entire messages before starting to process them, bounded memory use, and so on. I'd far rather see something that does less damage to the underlying models.

- Dave

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Hi Dave,

I agree, I did have some concerns about the callback issue. It also looks like we have enough people + momentum on this problem to get a fix in good time. So I'll wait to see what happens.

Thanks for the reply
Phil Wilshire

Actually, I'm not sure I'd agree on that. Folk want a fix, but nobody (including me!) has sent something that's quite mergeable yet.

I think part of the solution may be what someone (Brian?)
observed: that growing the mid-layer to handle some of the queue management functions should help, no matter what eventual solution is adopted.

That might be as little as providing a default "add to queue" and "take from queue" mechanism and starting to phase it into use in the various master controller drivers. When they use that scheme, it should be easy to splice in alternative queue management policies (notably: "only spi_mmc can access the bus for now") without changing the controller drivers.

So it'd be really nice if someone could come up with a patch like that to help get some forward motion ... unless there's a better suggestion for how to move forward?

- Dave

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

David Brownell skrev:

Hi again.

I have not solved the roots to the problem. Im not sure im the man to do that design in the end. But, I have actually MMC/SD cards running concurrently with m25p80 spi flash driver! No fix to either the master driver or the framework. Both reading and writing. No multiple block writings so far. But it is quite cool to see it spin.

It should be easy to move the solution over to the mainline mmc_spi driver once im satisfied with the implementation

you can see how i build the command and response transfers on before hand and then issue a single spi_sync to get it onto the bus under a single CS. I´ll have to add some more error detection and probably a status check after the busy poll phase in the end.

Hope to test it with enc28j60 driver this afternoon and send it to Phil who i believe was eager to test something.

/regards
Hans Eklund

static int spi_mmc_write_mmc_block(mmc_info_t* pdev, unsigned char* buf, unsigned int address)

{
	struct spi_transfer t[4];
	struct spi_message m;
	struct spi_message b;
	struct spi_transfer bt;
	int i=0;
	unsigned char wanted_token;

	#define MMC_CMD_LEN		6
	#define CMD_RESP_LEN		10
	#define TOKEN_AND_DATA_LEN	513
	#define DATA_RESP_LEN		100
	#define BUSY_BLOCK_LEN		64

	unsigned char mmc_cmd[MMC_CMD_LEN];
	unsigned char cmd_resp[CMD_RESP_LEN];
	unsigned char token_and_data[TOKEN_AND_DATA_LEN];
	unsigned char data_resp[DATA_RESP_LEN];
	unsigned char busy_block[BUSY_BLOCK_LEN];

	// Build write block command
	mmc_cmd[0] = 0x40 + 24;
	mmc_cmd[1] = (unsigned char)(address >> 24 & 0xff);
	mmc_cmd[2] = (unsigned char)(address >> 16 & 0xff);
	mmc_cmd[3] = (unsigned char)(address >> 8 & 0xff);
	mmc_cmd[4] = (unsigned char)(address & 0xff);
	mmc_cmd[5] = 0x95;	// CRC from CMD0 actually, but valid for all since 

SPI dont care

	// Build token+data chunk
	token_and_data[0] = (unsigned char)0xFE;	
	memcpy(token_and_data+1, buf, TOKEN_AND_DATA_LEN-1);

	// Zero out messages
	spi_message_init(&m);
	memset(t, 0, (sizeof t));

	#ifdef CONFIG_BLACKFIN
	blackfin_dcache_flush_range((unsigned long)mmc_cmd,(unsigned 
long)(mmc_cmd+MMC_CMD_LEN));
	blackfin_dcache_flush_range((unsigned long)token_and_data,(unsigned 
long)(token_and_data+TOKEN_AND_DATA_LEN));
	#endif

	t[0].tx_buf = mmc_cmd;
	t[0].len = MMC_CMD_LEN;
	spi_message_add_tail(&t[0], &m);

	t[1].rx_buf = cmd_resp;
	t[1].len = CMD_RESP_LEN;
	spi_message_add_tail(&t[1], &m);

	t[2].tx_buf = token_and_data;
	t[2].len = TOKEN_AND_DATA_LEN;
	spi_message_add_tail(&t[2], &m);

	t[3].rx_buf = data_resp;
	t[3].len = DATA_RESP_LEN;
	spi_message_add_tail(&t[3], &m);

	// Send it away, Fingers crossed!
	spi_sync(pdev->spi_dev, &m);

	// Cache coherency stuff
	#ifdef CONFIG_BLACKFIN
	blackfin_dcache_invalidate_range((unsigned long)cmd_resp,(unsigned 
long)(cmd_resp+CMD_RESP_LEN));
	blackfin_dcache_invalidate_range((unsigned long)data_resp,(unsigned 
long)(data_resp+DATA_RESP_LEN));
	#endif

	// Analyze response buffers

	// R1_OK token
	wanted_token = 0x00;
	for(i=0; i<CMD_RESP_LEN; i++) {
		if(cmd_resp[i] == wanted_token) {
			DPRINTK("Found wanted CMD token(0x%02x) at %d\n", cmd_resp[i], i);
			goto found_cmd_token;
		}
	}
	// error case here:
	DPRINTK("Did not find CMD response token!\n");
	return -1;

	// OK!
	found_cmd_token:

	// DR_ACCEPTED
	wanted_token = 0x05;
	for(i=0; i<DATA_RESP_LEN; i++) {
		if(data_resp[i] == wanted_token || data_resp[i] == 0x00 || 
data_resp[i] == 0x01 || data_resp[i] == 0x04) {
			DPRINTK("Found wanted DATA response token(0x%02x) at %d\n", 
data_resp[i], i);
			goto found_acc_token;
		}
	}
	// error case here:
	DPRINTK("Did not find DATA response token!\n");
	return -1;

	// OK!
	found_acc_token:

	// Read until NOT busy anymore
	i=1000;
	while(i--) {
		// it wont be done right away, be nice...
		schedule();
		memset(busy_block, 0, BUSY_BLOCK_LEN);
		spi_message_init(&b);
		memset(&bt, 0, (sizeof bt));
		bt.rx_buf = busy_block;
		bt.len = BUSY_BLOCK_LEN;
		spi_message_add_tail(&bt, &b);
		spi_sync(pdev->spi_dev, &b);
		blackfin_dcache_invalidate_range((unsigned long)busy_block,(unsigned 
long)(busy_block+BUSY_BLOCK_LEN));
		if(busy_block[BUSY_BLOCK_LEN-1] == 0xFF) {
			DPRINTK("Detected transiton from busy to NOT busy, doNE!\n");
			goto done;
		}
	}
	printk("Busy transition timeout! Not good!");
	return -1;

	done:

	// Successfully wrote block to MMC!
	return 0;

}

Hi again.

Lets move on.

I have not solved the roots to the problem. I´ve not been given time to fiddle with the framework internals. Im not sure im the man to do that in the end. But, I have managed to get MMC/SD cards running concurrently with m25p80 spi flash driver. No fix to either the master driver or the framework. Both reading and writing and card init works. No multiple block writing yet though. All done vs uClinux 2008R1-RC8 (kernel 2.6.22) and my old spi_mmc driver which implemented a separate block layer. Written and tested mostly within the Blackfin uClinux community.

About the fix: In short i predefine a sequence of transfers in a single message matching the MMC/SD protocol, simply assuming it will work, checking the tokens and if tokens looked fine return with a smile. It seem to work. It is the only way to get atomicity on the bus for now. It is surley best to patch up the drivers/mmc/host/mmc_spi.c code and use that driver from now on, putting my old spi_mmc driver on the shelf.

Bryan, Mike and other Blackfin-uClinux developers:

What is the status on the drivers/mmc/host/mmc_spi.c when connected to the Blackfin spi master? Mike pointed out that my old spi_mmc should be obsoleted(with all rights), with that statement I got the feeling the "new" mainline driver was tested to some extent, correct? I would need a test/develop environment to move on with this. I´ll try to backport the mmc_spi found in 2.6.24 to fit 2008R1-RC8 as a start, or do you have any other suggestions?

I hope i get the attention of Jan Nikitenko and Hans-Peter Nilsson on this subject also as they seem to have been a part of the mmc_spi driver developing.

/regards
Hans Eklund,
Rubico AB,
Sweden.

David Brownell skrev:

When I did it for USB, I essentially defined a "golden device" that could be the target of such testing. Easy to do with various microcontrollers with I2C; a bit less so with SPI, because you'd also want to test higher data rates than most micros like.

Better coverage than many systems. :)

That can be *part* of the solution, although I'd rather see the controller drivers talking in terms of messages: "get me the next message", with the core knowing how to handle things like this exclusive-access policy.

The rest of the solution involves implementation and interface details, including just how to gracefully migrate to that model.

The original notion was not to have any kind of mid-layer ... in my experience they're magnets for code bloat. But in this case I'm not sure I see a really good solution other than that.

- Dave

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/