From: Cliff Cai <[email protected]>
Signed-off-by: Cliff Cai <[email protected]>
Signed-off-by: Bryan Wu <[email protected]>
---
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
On Fri, 6 Mar 2009 11:15:21 +0800
Bryan Wu <[email protected]> wrote:
> From: Cliff Cai <[email protected]>
>
> Signed-off-by: Cliff Cai <[email protected]>
> Signed-off-by: Bryan Wu <[email protected]>
> ---
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.
>
>-----Original Message-----
>From: Pierre Ossman [mailto:[email protected]]
>Sent: Sunday, March 15, 2009 4:31 AM
>To: Bryan Wu
>Cc: [email protected]; 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 <[email protected]> wrote:
>
>> From: Cliff Cai <[email protected]>
>>
>> Signed-off-by: Cliff Cai <[email protected]>
>> Signed-off-by: Bryan Wu <[email protected]>
>> ---
>
>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
On Wed, 25 Mar 2009 11:22:25 +0800
"Cai, Cliff" <[email protected]> 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.
>
>-----Original Message-----
>From: Pierre Ossman [mailto:[email protected]]
>Sent: Thursday, March 26, 2009 3:24 AM
>To: Cai, Cliff
>Cc: Bryan Wu; [email protected]
>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" <[email protected]> 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
On Thu, 26 Mar 2009 10:22:54 +0800
"Cai, Cliff" <[email protected]> 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.
>-----Original Message-----
>From: Pierre Ossman [mailto:[email protected]]
>Sent: Monday, April 06, 2009 3:09 AM
>To: Cai, Cliff
>Cc: Bryan Wu; [email protected]
>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" <[email protected]> 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
On Tue, 7 Apr 2009 16:34:37 +0800
"Cai, Cliff" <[email protected]> wrote:
>
>
> >-----Original Message-----
> >From: Pierre Ossman [mailto:[email protected]]
> >Sent: Monday, April 06, 2009 3:09 AM
> >To: Cai, Cliff
> >Cc: Bryan Wu; [email protected]
> >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" <[email protected]> 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.