On Wed, Mar 31, 2010 at 10:07 PM, Madhusudhan <[email protected]> wrote:
>
>
>> -----Original Message-----
>> From: kishore kadiyala [mailto:[email protected]]
>> Sent: Wednesday, March 31, 2010 2:03 AM
>> To: Vimal Singh
>> Cc: Madhusudhan; [email protected]; [email protected]; linux-
>> [email protected]; [email protected];
>> [email protected]
>> Subject: Re: [PATCH-V2] OMAP: Fix for bus width which improves SD card's
>> peformance.
>>
>> Sorry for that and here's the Updated one.
>>
>> From: Kishore Kadiyala <[email protected]>
>>
>> This patch improves low speeds for SD cards.
>> OMAP-MMC controller's can support maximum bus width of '8'.
>> when bus width is mentioned as "8" in controller data,the SD
>> stack will check whether bus width is "4" and if not it will
>> set bus width to "1" and there by degrading performance.
>> This patch fixes the issue and improves the performance of
>> SD cards.
>>
>> Signed-off-by: Kishore Kadiyala <[email protected]>
>> Signed-off-by: Venkatraman S <[email protected]>
>> Acked-by: Madhusudhan Chikkature <[email protected]>
>>
>> ---
>> In V2 : Appended Signed-off by Venkat and Ack by Madhu
>>
>> ?Here are my experiment numbers, on a Class 6 SDHC card:
>> ?Read peformance is increased by 220%
>> ?Write Performance is increased by 52%
>>
>> ?drivers/mmc/host/omap_hsmmc.c | ? ?2 +-
>> ?1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
>> index 83f0aff..8c97c22 100644
>> --- a/drivers/mmc/host/omap_hsmmc.c
>> +++ b/drivers/mmc/host/omap_hsmmc.c
>> @@ -2092,7 +2092,7 @@ static int __init omap_hsmmc_probe(struct
>> ? ? ? ? ? ? ? ? ? ?MMC_CAP_WAIT_WHILE_BUSY;
>>
>> ? ? ? if (mmc_slot(host).wires >= 8)
>> - ? ? ? ? ? ? mmc->caps |= MMC_CAP_8_BIT_DATA;
>> + ? ? ? ? ? ? mmc->caps |= (MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA);
>> ? ? ? else if (mmc_slot(host).wires >= 4)
>> ? ? ? ? ? ? ? mmc->caps |= MMC_CAP_4_BIT_DATA;
>>
> Kishore,
>
> Since this patch is not yet pushed it makes sense to fix the readability
> issue.
>
> Since 8-bit is the max how about:
>
> ?? ? ? ?if (mmc_slot(host).wires == 8)
> ?? ? ? ? ? ? ? ?mmc->caps |= MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA;
> ?? ? ? ?if (mmc_slot(host).wires == 4)
> ?? ? ? ? ? ? ? ?mmc->caps |= MMC_CAP_4_BIT_DATA;
>
Madhu,
In the above snippet, it checks whether wires are 8 or 4 and if not
neither set's capability to "1".
Does it make sense to check whether the wires are 8,4,1 and if not
any[8,4,1] throw error and come out.
Regards,
Kishore
> This would be little easy to read the code.
>
> Can you please repost the patch??
>
> Regards,
> Madhu
>
>> --
>> 1.6.3.3
>
>
> -----Original Message-----
> From: kishore kadiyala [mailto:[email protected]]
> Sent: Thursday, April 01, 2010 1:32 AM
> To: Madhusudhan
> Cc: Vimal Singh; [email protected]; [email protected]; linux-
> [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH-V2] OMAP: Fix for bus width which improves SD card's
> peformance.
>
> On Wed, Mar 31, 2010 at 10:07 PM, Madhusudhan <[email protected]> wrote:
> >
> >
> >> -----Original Message-----
> >> From: kishore kadiyala [mailto:[email protected]]
> >> Sent: Wednesday, March 31, 2010 2:03 AM
> >> To: Vimal Singh
> >> Cc: Madhusudhan; [email protected]; [email protected]; linux-
> >> [email protected]; [email protected];
> >> [email protected]
> >> Subject: Re: [PATCH-V2] OMAP: Fix for bus width which improves SD
> card's
> >> peformance.
> >>
> >> Sorry for that and here's the Updated one.
> >>
> >> From: Kishore Kadiyala <[email protected]>
> >>
> >> This patch improves low speeds for SD cards.
> >> OMAP-MMC controller's can support maximum bus width of '8'.
> >> when bus width is mentioned as "8" in controller data,the SD
> >> stack will check whether bus width is "4" and if not it will
> >> set bus width to "1" and there by degrading performance.
> >> This patch fixes the issue and improves the performance of
> >> SD cards.
> >>
> >> Signed-off-by: Kishore Kadiyala <[email protected]>
> >> Signed-off-by: Venkatraman S <[email protected]>
> >> Acked-by: Madhusudhan Chikkature <[email protected]>
> >>
> >> ---
> >> In V2 : Appended Signed-off by Venkat and Ack by Madhu
> >>
> >> ?Here are my experiment numbers, on a Class 6 SDHC card:
> >> ?Read peformance is increased by 220%
> >> ?Write Performance is increased by 52%
> >>
> >> ?drivers/mmc/host/omap_hsmmc.c | ? ?2 +-
> >> ?1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/mmc/host/omap_hsmmc.c
> b/drivers/mmc/host/omap_hsmmc.c
> >> index 83f0aff..8c97c22 100644
> >> --- a/drivers/mmc/host/omap_hsmmc.c
> >> +++ b/drivers/mmc/host/omap_hsmmc.c
> >> @@ -2092,7 +2092,7 @@ static int __init omap_hsmmc_probe(struct
> >> ? ? ? ? ? ? ? ? ? ?MMC_CAP_WAIT_WHILE_BUSY;
> >>
> >> ? ? ? if (mmc_slot(host).wires >= 8)
> >> - ? ? ? ? ? ? mmc->caps |= MMC_CAP_8_BIT_DATA;
> >> + ? ? ? ? ? ? mmc->caps |= (MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA);
> >> ? ? ? else if (mmc_slot(host).wires >= 4)
> >> ? ? ? ? ? ? ? mmc->caps |= MMC_CAP_4_BIT_DATA;
> >>
> > Kishore,
> >
> > Since this patch is not yet pushed it makes sense to fix the readability
> > issue.
> >
> > Since 8-bit is the max how about:
> >
> > ?? ? ? ?if (mmc_slot(host).wires == 8)
> > ?? ? ? ? ? ? ? ?mmc->caps |= MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA;
> > ?? ? ? ?if (mmc_slot(host).wires == 4)
> > ?? ? ? ? ? ? ? ?mmc->caps |= MMC_CAP_4_BIT_DATA;
> >
> Madhu,
>
> In the above snippet, it checks whether wires are 8 or 4 and if not
> neither set's capability to "1".
> Does it make sense to check whether the wires are 8,4,1 and if not
> any[8,4,1] throw error and come out.
>
It is good enough to just check for 8-bit and 4-bit. The 1-bit does not need
any explicit check since it is default.
Regards,
Madhu
> Regards,
> Kishore
> > This would be little easy to read the code.
> >
> > Can you please repost the patch??
> >
> > Regards,
> > Madhu
> >
> >> --
> >> 1.6.3.3
> >
> >
From: Kishore Kadiyala <[email protected]>
This patch improves low speeds for SD cards.
OMAP-MMC controller's can support maximum bus width of '8'.
when bus width is mentioned as "8" in controller data,the SD
stack will check whether bus width is "4" and if not it will
set bus width to "1" and there by degrading performance.
This patch fixes the issue and improves the performance of
SD cards.
Signed-off-by: Kishore Kadiyala <[email protected]>
Signed-off-by: Venkatraman S <[email protected]>
Acked-by: Madhusudhan Chikkature <[email protected]>
Tested-by: Jarkko Nikula <[email protected]>
---
In V3 : Updated with Madhu's comments and appended Tested by Nikula
In V2 : Appended Signed-off by Venkat and Ack by Madhu
Here are my experiment numbers, on a Class 6 SDHC card:
Read peformance is increased by 220%
Write Performance is increased by 52%
drivers/mmc/host/omap_hsmmc.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
drivers/mmc/host/omap_hsmmc.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 83f0aff..44e79f7 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -2091,9 +2091,9 @@ static int __init omap_hsmmc_probe(struct
mmc->caps |= MMC_CAP_MMC_HIGHSPEED | MMC_CAP_SD_HIGHSPEED |
MMC_CAP_WAIT_WHILE_BUSY;
- if (mmc_slot(host).wires >= 8)
- mmc->caps |= MMC_CAP_8_BIT_DATA;
- else if (mmc_slot(host).wires >= 4)
+ if (mmc_slot(host).wires == 8)
+ mmc->caps |= (MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA);
+ else if (mmc_slot(host).wires == 4)
mmc->caps |= MMC_CAP_4_BIT_DATA;
if (mmc_slot(host).nonremovable)
--
1.6.3.3
Hi,
On Mon, Apr 05, 2010 at 06:26:16PM +0530, kishore kadiyala wrote:
> @@ -2091,9 +2091,9 @@ static int __init omap_hsmmc_probe(struct
> mmc->caps |= MMC_CAP_MMC_HIGHSPEED | MMC_CAP_SD_HIGHSPEED |
> MMC_CAP_WAIT_WHILE_BUSY;
>
> - if (mmc_slot(host).wires >= 8)
> - mmc->caps |= MMC_CAP_8_BIT_DATA;
> - else if (mmc_slot(host).wires >= 4)
> + if (mmc_slot(host).wires == 8)
> + mmc->caps |= (MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA);
> + else if (mmc_slot(host).wires == 4)
> mmc->caps |= MMC_CAP_4_BIT_DATA;
I believe it would be enough to just remove the 'else', so the code
would look like:
if (mmc_slot(host).wires >= 8)
mmc->caps |= MMC_CAP_8_BIT_DATA;
if (mmc_slot(host).wires >= 4)
mmc->caps |= MMC_CAP_4_BIT_DATA;
--
balbi
> -----Original Message-----
> From: Felipe Balbi [mailto:[email protected]]
> Sent: Monday, April 05, 2010 11:49 AM
> To: kishore kadiyala
> Cc: Madhusudhan; Vimal Singh; [email protected]; [email protected]; linux-
> [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH v3] OMAP: Fix for bus width which improves SD card's
> peformance.
>
> Hi,
>
> On Mon, Apr 05, 2010 at 06:26:16PM +0530, kishore kadiyala wrote:
> > @@ -2091,9 +2091,9 @@ static int __init omap_hsmmc_probe(struct
> > mmc->caps |= MMC_CAP_MMC_HIGHSPEED | MMC_CAP_SD_HIGHSPEED |
> > MMC_CAP_WAIT_WHILE_BUSY;
> >
> > - if (mmc_slot(host).wires >= 8)
> > - mmc->caps |= MMC_CAP_8_BIT_DATA;
> > - else if (mmc_slot(host).wires >= 4)
> > + if (mmc_slot(host).wires == 8)
> > + mmc->caps |= (MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA);
> > + else if (mmc_slot(host).wires == 4)
> > mmc->caps |= MMC_CAP_4_BIT_DATA;
>
> I believe it would be enough to just remove the 'else', so the code
> would look like:
>
> if (mmc_slot(host).wires >= 8)
> mmc->caps |= MMC_CAP_8_BIT_DATA;
> if (mmc_slot(host).wires >= 4)
Since the first if command already checks for the 8-bit the second check
like >= 4 is definitely not readable in my opinion.
Functionally do you see anything wrong with this patch??
> mmc->caps |= MMC_CAP_4_BIT_DATA;
>
> --
> balbi
> -----Original Message-----
> From: [email protected] [mailto:linux-omap-
> [email protected]] On Behalf Of Madhusudhan
> Sent: Monday, April 05, 2010 12:19 PM
> To: [email protected]; 'kishore kadiyala'
> Cc: 'Vimal Singh'; [email protected]; [email protected]; linux-
> [email protected]; [email protected];
> [email protected]
> Subject: RE: [PATCH v3] OMAP: Fix for bus width which improves SD card's
> peformance.
>
>
>
> > -----Original Message-----
> > From: Felipe Balbi [mailto:[email protected]]
> > Sent: Monday, April 05, 2010 11:49 AM
> > To: kishore kadiyala
> > Cc: Madhusudhan; Vimal Singh; [email protected]; [email protected]; linux-
> > [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [PATCH v3] OMAP: Fix for bus width which improves SD card's
> > peformance.
> >
> > Hi,
> >
> > On Mon, Apr 05, 2010 at 06:26:16PM +0530, kishore kadiyala wrote:
> > > @@ -2091,9 +2091,9 @@ static int __init omap_hsmmc_probe(struct
> > > mmc->caps |= MMC_CAP_MMC_HIGHSPEED | MMC_CAP_SD_HIGHSPEED |
> > > MMC_CAP_WAIT_WHILE_BUSY;
> > >
> > > - if (mmc_slot(host).wires >= 8)
> > > - mmc->caps |= MMC_CAP_8_BIT_DATA;
> > > - else if (mmc_slot(host).wires >= 4)
> > > + if (mmc_slot(host).wires == 8)
> > > + mmc->caps |= (MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA);
> > > + else if (mmc_slot(host).wires == 4)
> > > mmc->caps |= MMC_CAP_4_BIT_DATA;
> >
> > I believe it would be enough to just remove the 'else', so the code
> > would look like:
> >
> > if (mmc_slot(host).wires >= 8)
> > mmc->caps |= MMC_CAP_8_BIT_DATA;
> > if (mmc_slot(host).wires >= 4)
>
> Since the first if command already checks for the 8-bit the second check
> like >= 4 is definitely not readable in my opinion.
>
> Functionally do you see anything wrong with this patch??
>
Just to clarify my earlier comment on the patch was to resubmit like below:
if (mmc_slot(host).wires == 8)
mmc->caps |= (MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA);
if (mmc_slot(host).wires == 4)
mmc->caps |= MMC_CAP_4_BIT_DATA;
IMHO, this should be good enough.
Regards,
Madhu
> > mmc->caps |= MMC_CAP_4_BIT_DATA;
> >
> > --
> > balbi
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi,
On Mon, Apr 05, 2010 at 12:19:29PM -0500, Madhusudhan wrote:
> Since the first if command already checks for the 8-bit the second check
> like >= 4 is definitely not readable in my opinion.
how come ???
> Functionally do you see anything wrong with this patch??
functionally no, but (hypothetical situation) and if on
omap4/5/6/whatever, omap controller supports a bigger bus width then
you'll have to add a line like:
+ if (mmc_slot(host).wires == 16)
+ mmc->caps |= (MMC_CAP_16_BIT_DATA | MMC_CAP_8_BIT_DATA |
+ MMC_CAP_4_BIT_DATA);
- if (mmc_slot(host).wires == 8)
+ else if (mmc_slot(host).wires == 8)
do you see the problem ?? In my opinion it doesn't scale well.
--
balbi
> -----Original Message-----
> From: Felipe Balbi [mailto:[email protected]]
> Sent: Tuesday, April 06, 2010 12:01 AM
> To: Madhusudhan
> Cc: [email protected]; 'kishore kadiyala'; 'Vimal Singh';
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH v3] OMAP: Fix for bus width which improves SD card's
> peformance.
>
> Hi,
>
> On Mon, Apr 05, 2010 at 12:19:29PM -0500, Madhusudhan wrote:
> > Since the first if command already checks for the 8-bit the second check
> > like >= 4 is definitely not readable in my opinion.
>
> how come ???
>
> > Functionally do you see anything wrong with this patch??
>
> functionally no, but (hypothetical situation) and if on
> omap4/5/6/whatever, omap controller supports a bigger bus width then
> you'll have to add a line like:
>
> + if (mmc_slot(host).wires == 16)
> + mmc->caps |= (MMC_CAP_16_BIT_DATA | MMC_CAP_8_BIT_DATA |
> + MMC_CAP_4_BIT_DATA);
> - if (mmc_slot(host).wires == 8)
> + else if (mmc_slot(host).wires == 8)
>
> do you see the problem ?? In my opinion it doesn't scale well.
>
The point we should note here is that MMC spec supports a max bus width of
8-bit. So anything beyond 8-bit is not in the picture as of today.
But, my bad on miss interpreting the snippet Felipe sent earlier.
if (mmc_slot(host).wires >= 8)
mmc->caps |= MMC_CAP_8_BIT_DATA;
if (mmc_slot(host).wires >= 4)
mmc->caps |= MMC_CAP_4_BIT_DATA;
I missed the fact that you removed the setting of 4-bit from the first
check.
I am okay with the above snippet as it is a trivial change that we are
trying to patch here which fixes an important issue.
Regards,
Madhu
> --
> balbi
On Tue, Apr 06, 2010 at 06:16:01PM +0200, ext Madhusudhan wrote:
>
>
>> -----Original Message-----
>> From: Felipe Balbi [mailto:[email protected]]
>> Sent: Tuesday, April 06, 2010 12:01 AM
>> To: Madhusudhan
>> Cc: [email protected]; 'kishore kadiyala'; 'Vimal Singh';
>> [email protected]; [email protected]; [email protected]; linux-
>> [email protected]; [email protected]
>> Subject: Re: [PATCH v3] OMAP: Fix for bus width which improves SD card's
>> peformance.
>>
>> Hi,
>>
>> On Mon, Apr 05, 2010 at 12:19:29PM -0500, Madhusudhan wrote:
>> > Since the first if command already checks for the 8-bit the second check
>> > like >= 4 is definitely not readable in my opinion.
>>
>> how come ???
>>
>> > Functionally do you see anything wrong with this patch??
>>
>> functionally no, but (hypothetical situation) and if on
>> omap4/5/6/whatever, omap controller supports a bigger bus width then
>> you'll have to add a line like:
>>
>> + if (mmc_slot(host).wires == 16)
>> + mmc->caps |= (MMC_CAP_16_BIT_DATA | MMC_CAP_8_BIT_DATA |
>> + MMC_CAP_4_BIT_DATA);
>> - if (mmc_slot(host).wires == 8)
>> + else if (mmc_slot(host).wires == 8)
>>
>> do you see the problem ?? In my opinion it doesn't scale well.
>>
>
>The point we should note here is that MMC spec supports a max bus width of
>8-bit. So anything beyond 8-bit is not in the picture as of today.
in that case, the code could be:
WARN_ON(mmc_slot(host).wires > 8);
if (mmc_slot(host).wires == 8)
mmc->caps |= MMC_CAP_8_BIT_DATA;
if (mmc_slot(host).wires >= 4)
mmc->caps |= MMC_CAP_4_BIT_DATA;
--
balbi
Felipe Balbi had written, on 04/06/2010 11:32 AM, the following:
> On Tue, Apr 06, 2010 at 06:16:01PM +0200, ext Madhusudhan wrote:
>>
>>> -----Original Message-----
>>> From: Felipe Balbi [mailto:[email protected]]
>>> Sent: Tuesday, April 06, 2010 12:01 AM
>>> To: Madhusudhan
>>> Cc: [email protected]; 'kishore kadiyala'; 'Vimal Singh';
>>> [email protected]; [email protected]; [email protected]; linux-
>>> [email protected]; [email protected]
>>> Subject: Re: [PATCH v3] OMAP: Fix for bus width which improves SD card's
>>> peformance.
>>>
>>> Hi,
>>>
>>> On Mon, Apr 05, 2010 at 12:19:29PM -0500, Madhusudhan wrote:
>>>> Since the first if command already checks for the 8-bit the second check
>>>> like >= 4 is definitely not readable in my opinion.
>>> how come ???
>>>
>>>> Functionally do you see anything wrong with this patch??
>>> functionally no, but (hypothetical situation) and if on
>>> omap4/5/6/whatever, omap controller supports a bigger bus width then
>>> you'll have to add a line like:
>>>
>>> + if (mmc_slot(host).wires == 16)
>>> + mmc->caps |= (MMC_CAP_16_BIT_DATA | MMC_CAP_8_BIT_DATA |
>>> + MMC_CAP_4_BIT_DATA);
>>> - if (mmc_slot(host).wires == 8)
>>> + else if (mmc_slot(host).wires == 8)
>>>
>>> do you see the problem ?? In my opinion it doesn't scale well.
>>>
>> The point we should note here is that MMC spec supports a max bus width of
>> 8-bit. So anything beyond 8-bit is not in the picture as of today.
>
> in that case, the code could be:
>
> WARN_ON(mmc_slot(host).wires > 8);
>
> if (mmc_slot(host).wires == 8)
> mmc->caps |= MMC_CAP_8_BIT_DATA;
> if (mmc_slot(host).wires >= 4)
> mmc->caps |= MMC_CAP_4_BIT_DATA;
>
some reasons why i love switch statements ;) since I dont expect other
than precisely 4 and 8 (do we expect 5,6,7 - i might be wrong).. but if
it is so, wont the following be better?
switch (mmc_slot(host).wires)
{
case 8:
mmc->caps |= MMC_CAP_8_BIT_DATA;
/* fall thru*/
case 4:
mmc->caps |= MMC_CAP_4_BIT_DATA;
break;
default:
WARN("bad width");
}
--
Regards,
Nishanth Menon
On Tue, Apr 06, 2010 at 06:55:03PM +0200, ext Nishanth Menon wrote:
>some reasons why i love switch statements ;) since I dont expect other
>than precisely 4 and 8 (do we expect 5,6,7 - i might be wrong).. but if
>it is so, wont the following be better?
>
>switch (mmc_slot(host).wires)
>{
>case 8:
> mmc->caps |= MMC_CAP_8_BIT_DATA;
> /* fall thru*/
>case 4:
> mmc->caps |= MMC_CAP_4_BIT_DATA;
> break;
>default:
> WARN("bad width");
>}
I like that, but I remember Madhu (or someone else) saying he thinks
it's less readable this way. Go figure...
--
balbi
> -----Original Message-----
> From: Felipe Balbi [mailto:[email protected]]
> Sent: Tuesday, April 06, 2010 11:57 AM
> To: ext Nishanth Menon
> Cc: Balbi Felipe (Nokia-D/Helsinki); Chikkature Rajashekar, Madhusudhan;
> [email protected]; 'kishore kadiyala'; 'Vimal Singh'; [email protected];
> S, Venkatraman; [email protected]; [email protected];
> Lavinen Jarkko (Nokia-D/Helsinki)
> Subject: Re: [PATCH v3] OMAP: Fix for bus width which improves SD card's
> peformance.
>
> On Tue, Apr 06, 2010 at 06:55:03PM +0200, ext Nishanth Menon wrote:
> >some reasons why i love switch statements ;) since I dont expect other
> >than precisely 4 and 8 (do we expect 5,6,7 - i might be wrong).. but if
> >it is so, wont the following be better?
> >
> >switch (mmc_slot(host).wires)
> >{
> >case 8:
> > mmc->caps |= MMC_CAP_8_BIT_DATA;
> > /* fall thru*/
> >case 4:
> > mmc->caps |= MMC_CAP_4_BIT_DATA;
> > break;
> >default:
> > WARN("bad width");
> >}
>
> I like that, but I remember Madhu (or someone else) saying he thinks
> it's less readable this way. Go figure...
>
Well, I did not comment on the usage of switch here. Note we only need to
handle 8-bit and 4-bit.The board files need not setup 8-bit or 4-bit if the
configuration of that board is 1-bit. The driver will still work in 1-bit
mode which would mean there is nothing to do in default case and should not
err out.
Regards,
Madhu
> --
> balbi
Chikkature Rajashekar, Madhusudhan had written, on 04/06/2010 06:23 PM,
the following:
>
>> -----Original Message-----
>> From: Felipe Balbi [mailto:[email protected]]
>> Sent: Tuesday, April 06, 2010 11:57 AM
>> To: ext Nishanth Menon
>> Cc: Balbi Felipe (Nokia-D/Helsinki); Chikkature Rajashekar, Madhusudhan;
>> [email protected]; 'kishore kadiyala'; 'Vimal Singh'; [email protected];
>> S, Venkatraman; [email protected]; [email protected];
>> Lavinen Jarkko (Nokia-D/Helsinki)
>> Subject: Re: [PATCH v3] OMAP: Fix for bus width which improves SD card's
>> peformance.
>>
>> On Tue, Apr 06, 2010 at 06:55:03PM +0200, ext Nishanth Menon wrote:
>>> some reasons why i love switch statements ;) since I dont expect other
>>> than precisely 4 and 8 (do we expect 5,6,7 - i might be wrong).. but if
>>> it is so, wont the following be better?
>>>
>>> switch (mmc_slot(host).wires)
>>> {
>>> case 8:
>>> mmc->caps |= MMC_CAP_8_BIT_DATA;
>>> /* fall thru*/
>>> case 4:
>>> mmc->caps |= MMC_CAP_4_BIT_DATA;
>>> break;
>>> default:
>>> WARN("bad width");
>>> }
>> I like that, but I remember Madhu (or someone else) saying he thinks
>> it's less readable this way. Go figure...
>>
> Well, I did not comment on the usage of switch here. Note we only need to
> handle 8-bit and 4-bit.The board files need not setup 8-bit or 4-bit if the
> configuration of that board is 1-bit. The driver will still work in 1-bit
> mode which would mean there is nothing to do in default case and should not
> err out.
check the attachment out.. hope that takes care of it.. just as a
reference alone ofcourse..
--
Regards,
Nishanth Menon
> -----Original Message-----
> From: Nishanth Menon [mailto:[email protected]]
> Sent: Tuesday, April 06, 2010 6:39 PM
> To: Chikkature Rajashekar, Madhusudhan
> Cc: [email protected]; [email protected]; 'kishore kadiyala'; 'Vimal
> Singh'; [email protected]; S, Venkatraman; [email protected];
> [email protected]; 'Lavinen Jarkko (Nokia-D/Helsinki)'
> Subject: Re: [PATCH v3] OMAP: Fix for bus width which improves SD card's
> peformance.
>
> Chikkature Rajashekar, Madhusudhan had written, on 04/06/2010 06:23 PM,
> the following:
> >
> >> -----Original Message-----
> >> From: Felipe Balbi [mailto:[email protected]]
> >> Sent: Tuesday, April 06, 2010 11:57 AM
> >> To: ext Nishanth Menon
> >> Cc: Balbi Felipe (Nokia-D/Helsinki); Chikkature Rajashekar,
> Madhusudhan;
> >> [email protected]; 'kishore kadiyala'; 'Vimal Singh';
> [email protected];
> >> S, Venkatraman; [email protected]; linux-
> [email protected];
> >> Lavinen Jarkko (Nokia-D/Helsinki)
> >> Subject: Re: [PATCH v3] OMAP: Fix for bus width which improves SD
> card's
> >> peformance.
> >>
> >> On Tue, Apr 06, 2010 at 06:55:03PM +0200, ext Nishanth Menon wrote:
> >>> some reasons why i love switch statements ;) since I dont expect other
> >>> than precisely 4 and 8 (do we expect 5,6,7 - i might be wrong).. but
> if
> >>> it is so, wont the following be better?
> >>>
> >>> switch (mmc_slot(host).wires)
> >>> {
> >>> case 8:
> >>> mmc->caps |= MMC_CAP_8_BIT_DATA;
> >>> /* fall thru*/
> >>> case 4:
> >>> mmc->caps |= MMC_CAP_4_BIT_DATA;
> >>> break;
> >>> default:
> >>> WARN("bad width");
> >>> }
> >> I like that, but I remember Madhu (or someone else) saying he thinks
> >> it's less readable this way. Go figure...
> >>
> > Well, I did not comment on the usage of switch here. Note we only need
> to
> > handle 8-bit and 4-bit.The board files need not setup 8-bit or 4-bit if
> the
> > configuration of that board is 1-bit. The driver will still work in 1-
> bit
> > mode which would mean there is nothing to do in default case and should
> not
> > err out.
> check the attachment out.. hope that takes care of it.. just as a
> reference alone ofcourse..
Nope. The default case is wrong. The assumption followed by controller
drivers is that if the board file says 4-bit or 8-bit set the capabilities
otherwise don't do any thing. The host will continue to work in 1-bit mode
which is a must. Your patch violates that (can not design a board without
connecting one data line at least :))
Also you can not say 1-bit is non-optimal because the board file dictates
the configuration based on what it is capable of. Why through a warning?
It is subjective.
Regards,
Madhu
> --
> Regards,
> Nishanth Menon
Chikkature Rajashekar, Madhusudhan had written, on 04/06/2010 07:16 PM,
the following:
>
>> -----Original Message-----
>> From: Nishanth Menon [mailto:[email protected]]
>> Sent: Tuesday, April 06, 2010 6:39 PM
>> To: Chikkature Rajashekar, Madhusudhan
>> Cc: [email protected]; [email protected]; 'kishore kadiyala'; 'Vimal
>> Singh'; [email protected]; S, Venkatraman; [email protected];
>> [email protected]; 'Lavinen Jarkko (Nokia-D/Helsinki)'
>> Subject: Re: [PATCH v3] OMAP: Fix for bus width which improves SD card's
>> peformance.
>>
>> Chikkature Rajashekar, Madhusudhan had written, on 04/06/2010 06:23 PM,
>> the following:
>>>> -----Original Message-----
>>>> From: Felipe Balbi [mailto:[email protected]]
>>>> Sent: Tuesday, April 06, 2010 11:57 AM
>>>> To: ext Nishanth Menon
>>>> Cc: Balbi Felipe (Nokia-D/Helsinki); Chikkature Rajashekar,
>> Madhusudhan;
>>>> [email protected]; 'kishore kadiyala'; 'Vimal Singh';
>> [email protected];
>>>> S, Venkatraman; [email protected]; linux-
>> [email protected];
>>>> Lavinen Jarkko (Nokia-D/Helsinki)
>>>> Subject: Re: [PATCH v3] OMAP: Fix for bus width which improves SD
>> card's
>>>> peformance.
>>>>
>>>> On Tue, Apr 06, 2010 at 06:55:03PM +0200, ext Nishanth Menon wrote:
>>>>> some reasons why i love switch statements ;) since I dont expect other
>>>>> than precisely 4 and 8 (do we expect 5,6,7 - i might be wrong).. but
>> if
>>>>> it is so, wont the following be better?
>>>>>
>>>>> switch (mmc_slot(host).wires)
>>>>> {
>>>>> case 8:
>>>>> mmc->caps |= MMC_CAP_8_BIT_DATA;
>>>>> /* fall thru*/
>>>>> case 4:
>>>>> mmc->caps |= MMC_CAP_4_BIT_DATA;
>>>>> break;
>>>>> default:
>>>>> WARN("bad width");
>>>>> }
>>>> I like that, but I remember Madhu (or someone else) saying he thinks
>>>> it's less readable this way. Go figure...
>>>>
>>> Well, I did not comment on the usage of switch here. Note we only need
>> to
>>> handle 8-bit and 4-bit.The board files need not setup 8-bit or 4-bit if
>> the
>>> configuration of that board is 1-bit. The driver will still work in 1-
>> bit
>>> mode which would mean there is nothing to do in default case and should
>> not
>>> err out.
>> check the attachment out.. hope that takes care of it.. just as a
>> reference alone ofcourse..
>
> Nope. The default case is wrong. The assumption followed by controller
> drivers is that if the board file says 4-bit or 8-bit set the capabilities
> otherwise don't do any thing. The host will continue to work in 1-bit mode
> which is a must. Your patch violates that (can not design a board without
> connecting one data line at least :))
>
> Also you can not say 1-bit is non-optimal because the board file dictates
> the configuration based on what it is capable of. Why through a warning?
> It is subjective.
Point noted. try n++:
switch(mmc_slot(host).wires) {
case 8:
mmc->caps |= MMC_CAP_8_BIT_DATA;
/* Fall through */
case 4:
mmc->caps |= MMC_CAP_4_BIT_DATA;
break;
case 0:
/* assuming nothing was given by board, use 1 */
case 1:
/* nothing to crib here */
break;
default:
/* Completely unexpected.. try 1 bit instead */
dev_crit(mmc_dev(host->mmc), "Invalid width %d"
" used! using 1 instead\n",
mmc_slot(host).wires);
}
note: we should crib if the board file made a mistake here.. say it gave
10 wire or so.. I agree that we can recover by stepping back to 1 bit
mode.. but we gotta tell the log that something aint right..
--
Regards,
Nishanth Menon
<snip>
> Point noted. try n++:
> switch(mmc_slot(host).wires) {
> case 8:
> mmc->caps |= MMC_CAP_8_BIT_DATA;
> /* Fall through */
> case 4:
> mmc->caps |= MMC_CAP_4_BIT_DATA;
> break;
> case 0:
> /* assuming nothing was given by board, use 1 */
> case 1:
> /* nothing to crib here */
> break;
> default:
> /* Completely unexpected.. try 1 bit instead */
> dev_crit(mmc_dev(host->mmc), "Invalid width %d"
> " used! using 1 instead\n",
> mmc_slot(host).wires);
> }
>
> note: we should crib if the board file made a mistake here.. say it gave
> 10 wire or so.. I agree that we can recover by stepping back to 1 bit
> mode.. but we gotta tell the log that something aint right..
>
Sure. It looks fine to me now.
Regards,
Madhu
> --
> Regards,
> Nishanth Menon
From: Kishore Kadiyala <[email protected]>
This patch improves low speeds for SD cards.
OMAP-MMC controller's can support maximum bus width of '8'.
when bus width is mentioned as "8" in controller data,the SD
stack will check whether bus width is "4" and if not it will
set bus width to "1" and there by degrading performance.
This patch fixes the issue and improves the performance of
SD cards.
Signed-off-by: Kishore Kadiyala <[email protected]>
Signed-off-by: Venkatraman S <[email protected]>
Signed-off-by: Nishant Menon <[email protected]>
Acked-by: Madhusudhan Chikkature <[email protected]>
Tested-by: Jarkko Nikula <[email protected]>
---
In V4 : Updated with Nishant's comments and appened his Signed-off
In V3 : Updated with Madhu's comments and appended Tested by Nikula
In V2 : Appended Signed-off by Venkat and Ack by Madhu
Here are my experiment numbers, on a Class 6 SDHC card:
Read peformance is increased by 220%
Write Performance is increased by 52%
drivers/mmc/host/omap_hsmmc.c | 19 ++++++++++++++++---
1 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 8c97c22..9c1a60e 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -2091,10 +2091,23 @@ static int __init omap_hsmmc_probe(struct
platform_device *pdev)
mmc->caps |= MMC_CAP_MMC_HIGHSPEED | MMC_CAP_SD_HIGHSPEED |
MMC_CAP_WAIT_WHILE_BUSY;
- if (mmc_slot(host).wires >= 8)
- mmc->caps |= (MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA);
- else if (mmc_slot(host).wires >= 4)
+ switch (mmc_slot(host).wires) {
+ case 8:
+ mmc->caps |= MMC_CAP_8_BIT_DATA;
+ /* Fall through */
+ case 4:
mmc->caps |= MMC_CAP_4_BIT_DATA;
+ break;
+ case 1:
+ /* Nothing to crib here */
+ case 0:
+ /* Assuming nothing was given by board, Core use's 1-Bit */
+ break;
+ default:
+ /* Completely unexpected.. Core goes with 1-Bit Width */
+ dev_crit(mmc_dev(host->mmc), "Invalid width %d\n used!"
+ "using 1 instead\n", mmc_slot(host).wires);
+ }
if (mmc_slot(host).nonremovable)
mmc->caps |= MMC_CAP_NONREMOVABLE;
--
1.6.3.3
The previous patch was Line wrapped , resending
correct patch.
NM, Sorry I miss spelled your name correcting this time.
Regards,
Kishore
From: Kishore Kadiyala <[email protected]>
This patch improves low speeds for SD cards.
OMAP-MMC controller's can support maximum bus width of '8'.
when bus width is mentioned as "8" in controller data,the SD
stack will check whether bus width is "4" and if not it will
set bus width to "1" and there by degrading performance.
This patch fixes the issue and improves the performance of
SD cards.
Signed-off-by: Kishore Kadiyala <[email protected]>
Signed-off-by: Venkatraman S <[email protected]>
Signed-off-by: Nishanth Menon <[email protected]>
Acked-by: Madhusudhan Chikkature <[email protected]>
Tested-by: Jarkko Nikula <[email protected]>
---
In V4 : Updated with Nishant's comments and appened his Signed-off
In V3 : Updated with Madhu's comments and appended Tested by Nikula
In V2 : Appended Signed-off by Venkat and Ack by Madhu
Here are my experiment numbers, on a Class 6 SDHC card:
Read peformance is increased by 220%
Write Performance is increased by 52%
drivers/mmc/host/omap_hsmmc.c | 19 ++++++++++++++++---
1 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 8c97c22..9c1a60e 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -2091,10 +2091,23 @@ static int __init omap_hsmmc_probe(struct
mmc->caps |= MMC_CAP_MMC_HIGHSPEED | MMC_CAP_SD_HIGHSPEED |
MMC_CAP_WAIT_WHILE_BUSY;
- if (mmc_slot(host).wires >= 8)
- mmc->caps |= (MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA);
- else if (mmc_slot(host).wires >= 4)
+ switch (mmc_slot(host).wires) {
+ case 8:
+ mmc->caps |= MMC_CAP_8_BIT_DATA;
+ /* Fall through */
+ case 4:
mmc->caps |= MMC_CAP_4_BIT_DATA;
+ break;
+ case 1:
+ /* Nothing to crib here */
+ case 0:
+ /* Assuming nothing was given by board, Core use's 1-Bit */
+ break;
+ default:
+ /* Completely unexpected.. Core goes with 1-Bit Width */
+ dev_crit(mmc_dev(host->mmc), "Invalid width %d\n used!"
+ "using 1 instead\n", mmc_slot(host).wires);
+ }
if (mmc_slot(host).nonremovable)
mmc->caps |= MMC_CAP_NONREMOVABLE;
--
1.6.3.3
> -----Original Message-----
> From: kishore kadiyala [mailto:[email protected]]
> Sent: Monday, April 19, 2010 10:52 AM
> To: Madhusudhan
> Cc: Nishanth Menon; [email protected]; [email protected]; Vimal
> Singh; [email protected]; S, Venkatraman; [email protected];
> [email protected]; Lavinen Jarkko (Nokia-D/Helsinki)
> Subject: [PATCH v4] OMAP: Fix for bus width which improves SD card's
> peformance.
>
> The previous patch was Line wrapped , resending
> correct patch.
> NM, Sorry I miss spelled your name correcting this time.
>
> Regards,
> Kishore
>
Tony,
I don't see any further comments on this patch. Can you please push this
patch?
Regards,
Madhu
> From: Kishore Kadiyala <[email protected]>
>
> This patch improves low speeds for SD cards.
> OMAP-MMC controller's can support maximum bus width of '8'.
> when bus width is mentioned as "8" in controller data,the SD
> stack will check whether bus width is "4" and if not it will
> set bus width to "1" and there by degrading performance.
> This patch fixes the issue and improves the performance of
> SD cards.
>
> Signed-off-by: Kishore Kadiyala <[email protected]>
> Signed-off-by: Venkatraman S <[email protected]>
> Signed-off-by: Nishanth Menon <[email protected]>
> Acked-by: Madhusudhan Chikkature <[email protected]>
> Tested-by: Jarkko Nikula <[email protected]>
> ---
> In V4 : Updated with Nishant's comments and appened his Signed-off
> In V3 : Updated with Madhu's comments and appended Tested by Nikula
> In V2 : Appended Signed-off by Venkat and Ack by Madhu
>
> Here are my experiment numbers, on a Class 6 SDHC card:
> Read peformance is increased by 220%
> Write Performance is increased by 52%
>
> drivers/mmc/host/omap_hsmmc.c | 19 ++++++++++++++++---
> 1 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index 8c97c22..9c1a60e 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -2091,10 +2091,23 @@ static int __init omap_hsmmc_probe(struct
> mmc->caps |= MMC_CAP_MMC_HIGHSPEED | MMC_CAP_SD_HIGHSPEED |
> MMC_CAP_WAIT_WHILE_BUSY;
>
> - if (mmc_slot(host).wires >= 8)
> - mmc->caps |= (MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA);
> - else if (mmc_slot(host).wires >= 4)
> + switch (mmc_slot(host).wires) {
> + case 8:
> + mmc->caps |= MMC_CAP_8_BIT_DATA;
> + /* Fall through */
> + case 4:
> mmc->caps |= MMC_CAP_4_BIT_DATA;
> + break;
> + case 1:
> + /* Nothing to crib here */
> + case 0:
> + /* Assuming nothing was given by board, Core use's 1-Bit */
> + break;
> + default:
> + /* Completely unexpected.. Core goes with 1-Bit Width */
> + dev_crit(mmc_dev(host->mmc), "Invalid width %d\n used!"
> + "using 1 instead\n", mmc_slot(host).wires);
> + }
>
> if (mmc_slot(host).nonremovable)
> mmc->caps |= MMC_CAP_NONREMOVABLE;
> --
> 1.6.3.3