PATCH: Config option to pad SDIO sizes

[Johan Adolfsson]

> -----Original Message-----
> From: Pierre Ossman [mailto:drzeus-mmc at drzeus.cx] 
> Sent: den 1 juli 2008 13:46
> To: Johan Adolfsson
> Cc: 'Dan Williams'; 'Johan Adolfsson'; 
> libertas-dev at lists.infradead.org
> Subject: Re: PATCH: Config option to pad SDIO sizes
> 
> 
> On Tue, 1 Jul 2008 11:39:00 +0200
> "Johan Adolfsson" <Johan.Adolfsson at axis.com> wrote:
> 
> > 
> > Havent tested, but I guess it makes sence to move it up 
> from the driver,
> > but here are some comments/thoughts:
> > 
> 
> Please do some testing though, as I'd like to have this included in
> 2.6.27.

Seems to work, but I managed to get a "timeout waiting for command 16"
debug message when I did iwconfig just after /etc/init.d/wlan.eth1 start
Not sure it's related though (not that repeatable).

 
> > +unsigned int mmc_align_data_size(struct mmc_card *card, 
> unsigned int sz)
> > ..
> > +	 */
> > +	if (sz & 3)
> > +		sz = ((sz + 3) / 4) * 4;
> > +
> > +	return sz;
> > 
> > I suggest skip the if(), since it doesn't save any instructions
> > compared to always doing the padding (not for x86, cris or 
> arm at least).
> > Isn't: sz = (sz + 3) &~3; clearer or can we trust all 
> compilers to do 
> > the right thing here?
> 
> No idea. I was more concerned about readability than anything else. I
> doubt this code will be a performance problem.

I guess the readability  of /4)*4 vs &~3 is a matter of taste and it
gave the same code for three different platforms.
But removing the if () would IMO improve readability and have the positive
sideeffect of a 22-57% reduction in number of instructions
(it's not many instructions - but it's low hanging fruit and done in 
the network path so if it was my call, the if would go away)


> > Possibly make the function inline ?
> > 
> 
> The ambition is to make it more complex to properly deal with more
> nasty controllers, so it wouldn't stay inline for long anyway.

Ok, could the complex multi controller support be conditional to avoid
overhead for the embedded solution with known hardware?

> > +	/*
> > +	 * If we can still do this with just a byte transfer, then
> > +	 * we're done.
> > +	 */
> > +	if ((sz < func->cur_blksize) && (sz < 512))
> > +		return sz;
> > vs
> > 
> > -	chunk = size;
> > -	if ((chunk > card->func->cur_blksize) || (chunk > 512)) {
> > -		chunk = (chunk + card->func->cur_blksize - 1) /
> > -			card->func->cur_blksize * 
> card->func->cur_blksize;
> > -	}
> > 
> > indicates that it should really be: (note the added =)
> > 
> > +	if ((sz <= func->cur_blksize) && (sz <= 512))
> > +		return sz;
> > 
> > Shouldn't it?
> > 
> 
> Good catch. I've done a separate cleanup of the whole byte/block mode
> mess, but we can leave that for a later discussion.
> 
> > 
> > Is the added multi_block stuff really related to the pad size
> > or just needed anyway ?
> 
> Just trying to solve a related problem at the same time (one that the
> code ripped out of if_sdio sort of solves). Imagine if you have a
> transfer of 1233 bytes. First you'd need to pad it to 1236 bytes to
> handle 32-bit size alignment bugs. But transferring this chunk would
> result in one request of 2 * 512 bytes, and one of 212 bytes. Padding
> it to 1536 bytes allows you to a single request with 3 * 512 bytes.

Aha.
Is it a certain fact that padding to multiple blocksizes is always
better than multiple transfers?
(Considering all the interrupt traffic for a single transfer with an
SDHCI like controller, and with small blocksizes (512) I would guess 
it is soo, but anyway?.

 
> > Isn't it a valid assumption that the cur_blksize is 32 bit 
> aligned so that
> > aligning size would not increase the size over the cur_blksize if 
> > it was not already (and that is handled elsewhere)?
> 
> No, the block size might be anything. Most users will probably have
> some power of two size, but it is not something we can rely on.

ok.

 
> Rgds
> -- 
>      -- Pierre Ossman

/Johan