[PATCH] mmc: align data size for host which only supports power-of-2 block

[PATCH] mmc: align data size for host which only supports power-of-2 block

[Bryan Wu]

From: Cliff Cai <cliff.cai@analog.com>

Signed-off-by: Cliff Cai <cliff.cai@analog.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 drivers/mmc/core/core.c  |    8 +++++++-
 include/linux/mmc/host.h |    1 +
 2 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index df6ce4a..15119df 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -321,7 +321,13 @@ unsigned int mmc_align_data_size(struct mmc_card *card, unsigned int sz)
 	 * the core about its problems yet, so for now we just 32-bit
 	 * align the size.
 	 */
-	sz = ((sz + 3) / 4) * 4;
+
+	/* Align size for host which only supports power-of-2 block */
+	if (card->host->powerof2_block) {
+		if (sz & (sz - 1))
+			sz = 1 << fls(sz);
+	} else
+		sz = ((sz + 3) / 4) * 4;
 
 	return sz;
 }
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 4e45725..7416ed1 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -162,6 +162,7 @@ struct mmc_host {
 	struct dentry		*debugfs_root;
 
 	unsigned long		private[0] ____cacheline_aligned;
+	unsigned int		powerof2_block;	/* host only supports power-of-2 block */
 };
 
 extern struct mmc_host *mmc_alloc_host(int extra, struct device *);
-- 
1.5.6.3

Re: [PATCH] mmc: align data size for host which only supports power-of-2 block

[Pierre Ossman]

On Fri,  6 Mar 2009 11:15:21 +0800
Bryan Wu <cooloney@kernel.org> wrote:

> From: Cliff Cai <cliff.cai@analog.com>
> 
> Signed-off-by: Cliff Cai <cliff.cai@analog.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>
> ---

This patch seems premature as there is no associated modification of
any of the host drivers.

>  drivers/mmc/core/core.c  |    8 +++++++-
>  include/linux/mmc/host.h |    1 +
>  2 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index df6ce4a..15119df 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -321,7 +321,13 @@ unsigned int mmc_align_data_size(struct mmc_card *card, unsigned int sz)
>  	 * the core about its problems yet, so for now we just 32-bit
>  	 * align the size.
>  	 */
> -	sz = ((sz + 3) / 4) * 4;
> +
> +	/* Align size for host which only supports power-of-2 block */
> +	if (card->host->powerof2_block) {
> +		if (sz & (sz - 1))
> +			sz = 1 << fls(sz);
> +	} else
> +		sz = ((sz + 3) / 4) * 4;
>  
>  	return sz;
>  }

At the very least, the comment at the top of this function must go. But
really, if we want to improve this we should probably do it properly
and have flags for the different limitations that are available.

Padding to a power of two size can also mean a rather large padding. We
might need to check the host data limits after doing the adjustment.

> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 4e45725..7416ed1 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -162,6 +162,7 @@ struct mmc_host {
>  	struct dentry		*debugfs_root;
>  
>  	unsigned long		private[0] ____cacheline_aligned;
> +	unsigned int		powerof2_block;	/* host only supports power-of-2 block */
>  };
>  
>  extern struct mmc_host *mmc_alloc_host(int extra, struct device *);

This is just broken. Putting it after private completely breaks
accesses to the host driver private data.

Also, there are a whole bunch of alignment issues that can occur. We
need some kind of flags field or a bitfield for this.

Rgds
-- 
     -- Pierre Ossman

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

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

RE: [PATCH] mmc: align data size for host which only supports power-of-2 block

[Cai, Cliff]

>
 

>-----Original Message-----
>From: Pierre Ossman [mailto:drzeus-mmc@drzeus.cx] 
>Sent: Sunday, March 15, 2009 4:31 AM
>To: Bryan Wu
>Cc: linux-kernel@vger.kernel.org; Cai, Cliff; Bryan Wu
>Subject: Re: [PATCH] mmc: align data size for host which only 
>supports power-of-2 block
>
>On Fri,  6 Mar 2009 11:15:21 +0800
>Bryan Wu <cooloney@kernel.org> wrote:
>
>> From: Cliff Cai <cliff.cai@analog.com>
>> 
>> Signed-off-by: Cliff Cai <cliff.cai@analog.com>
>> Signed-off-by: Bryan Wu <cooloney@kernel.org>
>> ---
>
>This patch seems premature as there is no associated 
>modification of any of the host drivers.

The SDH on Blackfin BF54x/BF51x only supports power-of-2 block
transfer,we will soon
Send the sdh driver to mainline as well.

>
>>  drivers/mmc/core/core.c  |    8 +++++++-
>>  include/linux/mmc/host.h |    1 +
>>  2 files changed, 8 insertions(+), 1 deletions(-)
>> 
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 
>> df6ce4a..15119df 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -321,7 +321,13 @@ unsigned int mmc_align_data_size(struct 
>mmc_card *card, unsigned int sz)
>>  	 * the core about its problems yet, so for now we just 32-bit
>>  	 * align the size.
>>  	 */
>> -	sz = ((sz + 3) / 4) * 4;
>> +
>> +	/* Align size for host which only supports power-of-2 block */
>> +	if (card->host->powerof2_block) {
>> +		if (sz & (sz - 1))
>> +			sz = 1 << fls(sz);
>> +	} else
>> +		sz = ((sz + 3) / 4) * 4;
>>  
>>  	return sz;
>>  }
>
>At the very least, the comment at the top of this function 
>must go. But really, if we want to improve this we should 
>probably do it properly and have flags for the different 
>limitations that are available.
>Padding to a power of two size can also mean a rather large 
>padding. We might need to check the host data limits after 
>doing the adjustment.

Yes,but it's the higher level driver's job. 

>
>> diff --git a/include/linux/mmc/host.h 
>b/include/linux/mmc/host.h index 
>> 4e45725..7416ed1 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -162,6 +162,7 @@ struct mmc_host {
>>  	struct dentry		*debugfs_root;
>>  
>>  	unsigned long		private[0] ____cacheline_aligned;
>> +	unsigned int		powerof2_block;	/* host only 
>supports power-of-2 block */
>>  };
>>  
>>  extern struct mmc_host *mmc_alloc_host(int extra, struct device *);
>
>This is just broken. Putting it after private completely 
>breaks accesses to the host driver private data.

Sure,it should be added before private.

Thanks

Cliff

Re: [PATCH] mmc: align data size for host which only supports power-of-2 block

[Pierre Ossman]

On Wed, 25 Mar 2009 11:22:25 +0800
"Cai, Cliff" <Cliff.Cai@analog.com> wrote:

> >
> >This patch seems premature as there is no associated 
> >modification of any of the host drivers.
> 
> The SDH on Blackfin BF54x/BF51x only supports power-of-2 block
> transfer,we will soon
> Send the sdh driver to mainline as well.
> 

Ok. Looking forward to it. :)

> >Padding to a power of two size can also mean a rather large 
> >padding. We might need to check the host data limits after 
> >doing the adjustment.
> 
> Yes,but it's the higher level driver's job. 
> 

mmc_align_data_size() shouldn't be adjusting the size to something it
can easily determine that it's invalid.

Rgds
-- 
     -- Pierre Ossman

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

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

RE: [PATCH] mmc: align data size for host which only supports power-of-2 block

[Cai, Cliff]

>
 

>-----Original Message-----
>From: Pierre Ossman [mailto:drzeus-mmc@drzeus.cx] 
>Sent: Thursday, March 26, 2009 3:24 AM
>To: Cai, Cliff
>Cc: Bryan Wu; linux-kernel@vger.kernel.org
>Subject: Re: [PATCH] mmc: align data size for host which only 
>supports power-of-2 block
>
>On Wed, 25 Mar 2009 11:22:25 +0800
>"Cai, Cliff" <Cliff.Cai@analog.com> wrote:
>
>> >
>> >This patch seems premature as there is no associated 
>modification of 
>> >any of the host drivers.
>> 
>> The SDH on Blackfin BF54x/BF51x only supports power-of-2 block 
>> transfer,we will soon Send the sdh driver to mainline as well.
>> 
>
>Ok. Looking forward to it. :)
>
>> >Padding to a power of two size can also mean a rather large 
>> >padding. We might need to check the host data limits after 
>> >doing the adjustment.
>> 
>> Yes,but it's the higher level driver's job. 
>> 
>
>mmc_align_data_size() shouldn't be adjusting the size to something it
>can easily determine that it's invalid.

Should I create a new function to do the power-of-2 adjustment?
Or any ideal way to do it?
Otherwise,the SDH almost can't be used as sdio host.

Thanks 

Cliff

Re: [PATCH] mmc: align data size for host which only supports power-of-2 block

[Pierre Ossman]

On Thu, 26 Mar 2009 10:22:54 +0800
"Cai, Cliff" <Cliff.Cai@analog.com> wrote:

> >
> >mmc_align_data_size() shouldn't be adjusting the size to something it
> >can easily determine that it's invalid.
> 
> Should I create a new function to do the power-of-2 adjustment?
> Or any ideal way to do it?

That function is the correct place. My point was that we need to check
after the adjustment that we didn't violate any of the other
restrictions the driver has set. If we do, we should just return the
original value.

> Otherwise,the SDH almost can't be used as sdio host.

Unfortunately I think this controller will be very unsuitable as an
SDIO controller anyway with that restriction.

Rgds
-- 
     -- Pierre Ossman

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

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

RE: [PATCH] mmc: align data size for host which only supports power-of-2 block

[Cai, Cliff]

 

>-----Original Message-----
>From: Pierre Ossman [mailto:drzeus-mmc@drzeus.cx] 
>Sent: Monday, April 06, 2009 3:09 AM
>To: Cai, Cliff
>Cc: Bryan Wu; linux-kernel@vger.kernel.org
>Subject: Re: [PATCH] mmc: align data size for host which only 
>supports power-of-2 block
>
>On Thu, 26 Mar 2009 10:22:54 +0800
>"Cai, Cliff" <Cliff.Cai@analog.com> wrote:
>
>> >
>> >mmc_align_data_size() shouldn't be adjusting the size to 
>something it 
>> >can easily determine that it's invalid.
>> 
>> Should I create a new function to do the power-of-2 adjustment?
>> Or any ideal way to do it?
>
>That function is the correct place. My point was that we need 
>to check after the adjustment that we didn't violate any of 
>the other restrictions the driver has set. If we do, we should 
>just return the original value.

What are other restrictions?I don't see any other restrictions on data
size in current host driver
Except 4 byte alignment.


>> Otherwise,the SDH almost can't be used as sdio host.
>
>Unfortunately I think this controller will be very unsuitable 
>as an SDIO controller anyway with that restriction.

Yes,But since we need to support some SDIO devices according to our
customers'
Requests ,I think we have no other choice of this.


Thanks

Cliff

Re: [PATCH] mmc: align data size for host which only supports power-of-2 block

[Pierre Ossman]

On Tue, 7 Apr 2009 16:34:37 +0800
"Cai, Cliff" <Cliff.Cai@analog.com> wrote:

>  
> 
> >-----Original Message-----
> >From: Pierre Ossman [mailto:drzeus-mmc@drzeus.cx] 
> >Sent: Monday, April 06, 2009 3:09 AM
> >To: Cai, Cliff
> >Cc: Bryan Wu; linux-kernel@vger.kernel.org
> >Subject: Re: [PATCH] mmc: align data size for host which only 
> >supports power-of-2 block
> >
> >On Thu, 26 Mar 2009 10:22:54 +0800
> >"Cai, Cliff" <Cliff.Cai@analog.com> wrote:
> >
> >> >
> >> >mmc_align_data_size() shouldn't be adjusting the size to 
> >something it 
> >> >can easily determine that it's invalid.
> >> 
> >> Should I create a new function to do the power-of-2 adjustment?
> >> Or any ideal way to do it?
> >
> >That function is the correct place. My point was that we need 
> >to check after the adjustment that we didn't violate any of 
> >the other restrictions the driver has set. If we do, we should 
> >just return the original value.
> 
> What are other restrictions?I don't see any other restrictions on data
> size in current host driver
> Except 4 byte alignment.
> 

The ones that are in the mmc_host structure:

        /* host specific block data */
        unsigned int            max_seg_size;   /* see blk_queue_max_segment_size */
        unsigned short          max_hw_segs;    /* see blk_queue_max_hw_segments */
        unsigned short          max_phys_segs;  /* see blk_queue_max_phys_segments */
        unsigned short          unused;
        unsigned int            max_req_size;   /* maximum number of bytes in one req */
        unsigned int            max_blk_size;   /* maximum size of one mmc block */
        unsigned int            max_blk_count;  /* maximum number of blocks in one req */

If you adjust a transfer from 1100 bytes to 2048, that could exceed the
limit of 2047 bytes some controllers have.

> 
> >> Otherwise,the SDH almost can't be used as sdio host.
> >
> >Unfortunately I think this controller will be very unsuitable 
> >as an SDIO controller anyway with that restriction.
> 
> Yes,But since we need to support some SDIO devices according to our
> customers'
> Requests ,I think we have no other choice of this.
> 

They should be prepared for the fact that many SDIO devices cannot be
made to work with a controller with these bugs though. The function
you're modifying is only to optimise the transfer and is not mandatory
for SDIO drivers to call in any way.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org
  TigerVNC, core developer          http://www.tigervnc.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.