Patchwork [U-Boot] powerpc/esdhc: force the bus width to 4bit

login
register
mail settings
Submitter Jerry Huang
Date Oct. 23, 2012, 6:26 a.m.
Message ID <1350973580-31726-1-git-send-email-Chang-Ming.Huang@freescale.com>
Download mbox | patch
Permalink /patch/193382/
State Rejected
Delegated to: Andy Fleming
Headers show

Comments

Jerry Huang - Oct. 23, 2012, 6:26 a.m.
From: Jerry Huang <Chang-Ming.Huang@freescale.com>

For the current u-boot codes, only 4bit/1bit SD/SDHC bus width is support.
So for MMC card, we also support 4bit bus width,
otherwiase, we will get the 12bit bus width, which is not correct:
=> mmcinfo
Device: FSL_SDHC
Manufacturer ID: 1e
OEM: ffff
Name: MMC
Tran Speed: 52000000
Rd Block Len: 512
MMC version 4.0
High Capacity: No
Capacity: 1.9 GiB
Bus Width: 12-bit

Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescalecom>
CC: Andy Fleming <afleming@gmail.com>
CC: Marek Vasut <marex@denx.de>
---
 drivers/mmc/fsl_esdhc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Marek Vasut - Oct. 23, 2012, 7:24 a.m.
Dear Chang-Ming.Huang@freescale.com,

> From: Jerry Huang <Chang-Ming.Huang@freescale.com>
> 
> For the current u-boot codes, only 4bit/1bit SD/SDHC bus width is support.
> So for MMC card, we also support 4bit bus width,
> otherwiase, we will get the 12bit bus width, which is not correct:

Andy ... can you please explain? I don't quite understand the problem, I thought 
we had no problem supporting 8bit mmc (esp. if the controller handles that for 
us mostly).

> => mmcinfo
> Device: FSL_SDHC
> Manufacturer ID: 1e
> OEM: ffff
> Name: MMC
> Tran Speed: 52000000
> Rd Block Len: 512
> MMC version 4.0
> High Capacity: No
> Capacity: 1.9 GiB
> Bus Width: 12-bit
> 
> Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescalecom>
> CC: Andy Fleming <afleming@gmail.com>
> CC: Marek Vasut <marex@denx.de>
> ---
>  drivers/mmc/fsl_esdhc.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> index 3f8d30d..7b83dd2 100644
> --- a/drivers/mmc/fsl_esdhc.c
> +++ b/drivers/mmc/fsl_esdhc.c
> @@ -577,7 +577,7 @@ int fsl_esdhc_initialize(bd_t *bis, struct
> fsl_esdhc_cfg *cfg) return -1;
>  	}
> 
> -	mmc->host_caps = MMC_MODE_4BIT | MMC_MODE_8BIT;
> +	mmc->host_caps = MMC_MODE_4BIT;
> 
>  	if (caps & ESDHC_HOSTCAPBLT_HSS)
>  		mmc->host_caps |= MMC_MODE_HS_52MHz | MMC_MODE_HS;

Best regards,
Marek Vasut
Changming Huang - Oct. 23, 2012, 9:23 a.m.
Best Regards
Jerry Huang


> -----Original Message-----
> From: Marek Vasut [mailto:marex@denx.de]
> Sent: Tuesday, October 23, 2012 3:24 PM
> To: Huang Changming-R66093
> Cc: u-boot@lists.denx.de; Andy Fleming
> Subject: Re: [PATCH] powerpc/esdhc: force the bus width to 4bit
> 
> Dear Chang-Ming.Huang@freescale.com,
> 
> > From: Jerry Huang <Chang-Ming.Huang@freescale.com>
> >
> > For the current u-boot codes, only 4bit/1bit SD/SDHC bus width is
> support.
> > So for MMC card, we also support 4bit bus width, otherwiase, we will
> > get the 12bit bus width, which is not correct:
> 
> Andy ... can you please explain? I don't quite understand the problem, I
> thought we had no problem supporting 8bit mmc (esp. if the controller
> handles that for us mostly).


Yes, the controller support 8bit MMC.

FSL ESDHC driver set the host_caps = MMC_MODE_4BIT | MMC_MODE_8BIT;
But, the current codes for MMC card has been changed to:

} else {
	width = ((mmc->host_caps & MMC_MODE_MASK_WIDTH_BITS) >>
                         MMC_MODE_WIDTH_BITS_SHIFT);
	for (; width >= 0; width--) {
	....

So for FSL ESDHC, the width = 3, after implement mmc_switch successfully, will set the bus to 4 * width.
Therefore, I will get the 12bit (4 x 3) bus width.

Below is the old codes (width = 2):
} else {
                for (width = EXT_CSD_BUS_WIDTH_8; width >= 0; width--) {

> > => mmcinfo
> > Device: FSL_SDHC
> > Manufacturer ID: 1e
> > OEM: ffff
> > Name: MMC
> > Tran Speed: 52000000
> > Rd Block Len: 512
> > MMC version 4.0
> > High Capacity: No
> > Capacity: 1.9 GiB
> > Bus Width: 12-bit
> >
> > Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescalecom>
> > CC: Andy Fleming <afleming@gmail.com>
> > CC: Marek Vasut <marex@denx.de>
> > ---
> >  drivers/mmc/fsl_esdhc.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c index
> > 3f8d30d..7b83dd2 100644
> > --- a/drivers/mmc/fsl_esdhc.c
> > +++ b/drivers/mmc/fsl_esdhc.c
> > @@ -577,7 +577,7 @@ int fsl_esdhc_initialize(bd_t *bis, struct
> > fsl_esdhc_cfg *cfg) return -1;
> >  	}
> >
> > -	mmc->host_caps = MMC_MODE_4BIT | MMC_MODE_8BIT;
> > +	mmc->host_caps = MMC_MODE_4BIT;
> >
> >  	if (caps & ESDHC_HOSTCAPBLT_HSS)
> >  		mmc->host_caps |= MMC_MODE_HS_52MHz | MMC_MODE_HS;
> 
> Best regards,
> Marek Vasut
Marek Vasut - Oct. 23, 2012, 9:50 a.m.
Dear Huang Changming-R66093,

> Best Regards
> Jerry Huang
> 
> > -----Original Message-----
> > From: Marek Vasut [mailto:marex@denx.de]
> > Sent: Tuesday, October 23, 2012 3:24 PM
> > To: Huang Changming-R66093
> > Cc: u-boot@lists.denx.de; Andy Fleming
> > Subject: Re: [PATCH] powerpc/esdhc: force the bus width to 4bit
> > 
> > Dear Chang-Ming.Huang@freescale.com,
> > 
> > > From: Jerry Huang <Chang-Ming.Huang@freescale.com>
> > > 
> > > For the current u-boot codes, only 4bit/1bit SD/SDHC bus width is
> > 
> > support.
> > 
> > > So for MMC card, we also support 4bit bus width, otherwiase, we will
> > 
> > > get the 12bit bus width, which is not correct:
> > Andy ... can you please explain? I don't quite understand the problem, I
> > thought we had no problem supporting 8bit mmc (esp. if the controller
> > handles that for us mostly).
> 
> Yes, the controller support 8bit MMC.
> 
> FSL ESDHC driver set the host_caps = MMC_MODE_4BIT | MMC_MODE_8BIT;
> But, the current codes for MMC card has been changed to:
> 
> } else {
> 	width = ((mmc->host_caps & MMC_MODE_MASK_WIDTH_BITS) >>
>                          MMC_MODE_WIDTH_BITS_SHIFT);
> 	for (; width >= 0; width--) {
> 	....
> 
> So for FSL ESDHC, the width = 3, after implement mmc_switch successfully,
> will set the bus to 4 * width. Therefore, I will get the 12bit (4 x 3) bus
> width.
> 
> Below is the old codes (width = 2):
> } else {
>                 for (width = EXT_CSD_BUS_WIDTH_8; width >= 0; width--) {
[...]


Uh, so it's a bug in the MMC subsystem?
Best regards,
Marek Vasut
Changming Huang - Oct. 24, 2012, 3:11 a.m.
> > > Dear Chang-Ming.Huang@freescale.com,
> > >
> > > > From: Jerry Huang <Chang-Ming.Huang@freescale.com>
> > > >
> > > > For the current u-boot codes, only 4bit/1bit SD/SDHC bus width is
> > >
> > > support.
> > >
> > > > So for MMC card, we also support 4bit bus width, otherwiase, we
> > > > will
> > >
> > > > get the 12bit bus width, which is not correct:
> > > Andy ... can you please explain? I don't quite understand the
> > > problem, I thought we had no problem supporting 8bit mmc (esp. if
> > > the controller handles that for us mostly).
> >
> > Yes, the controller support 8bit MMC.
> >
> > FSL ESDHC driver set the host_caps = MMC_MODE_4BIT | MMC_MODE_8BIT;
> > But, the current codes for MMC card has been changed to:
> >
> > } else {
> > 	width = ((mmc->host_caps & MMC_MODE_MASK_WIDTH_BITS) >>
> >                          MMC_MODE_WIDTH_BITS_SHIFT);
> > 	for (; width >= 0; width--) {
> > 	....
> >
> > So for FSL ESDHC, the width = 3, after implement mmc_switch
> > successfully, will set the bus to 4 * width. Therefore, I will get the
> > 12bit (4 x 3) bus width.
> >
> > Below is the old codes (width = 2):
> > } else {
> >                 for (width = EXT_CSD_BUS_WIDTH_8; width >= 0; width--)
> > {
> [...]
> 
> 
> Uh, so it's a bug in the MMC subsystem?
> Best regards,

I don't know. Maybe Andy can give some comment.
Jaehoon Chung - Oct. 30, 2012, 8:43 a.m.
On 10/23/2012 06:50 PM, Marek Vasut wrote:
> Dear Huang Changming-R66093,
> 
>> Best Regards
>> Jerry Huang
>>
>>> -----Original Message-----
>>> From: Marek Vasut [mailto:marex@denx.de]
>>> Sent: Tuesday, October 23, 2012 3:24 PM
>>> To: Huang Changming-R66093
>>> Cc: u-boot@lists.denx.de; Andy Fleming
>>> Subject: Re: [PATCH] powerpc/esdhc: force the bus width to 4bit
>>>
>>> Dear Chang-Ming.Huang@freescale.com,
>>>
>>>> From: Jerry Huang <Chang-Ming.Huang@freescale.com>
>>>>
>>>> For the current u-boot codes, only 4bit/1bit SD/SDHC bus width is
>>>
>>> support.
>>>
>>>> So for MMC card, we also support 4bit bus width, otherwiase, we will
>>>
>>>> get the 12bit bus width, which is not correct:
>>> Andy ... can you please explain? I don't quite understand the problem, I
>>> thought we had no problem supporting 8bit mmc (esp. if the controller
>>> handles that for us mostly).
>>
>> Yes, the controller support 8bit MMC.
>>
>> FSL ESDHC driver set the host_caps = MMC_MODE_4BIT | MMC_MODE_8BIT;
>> But, the current codes for MMC card has been changed to:
>>
>> } else {
>> 	width = ((mmc->host_caps & MMC_MODE_MASK_WIDTH_BITS) >>
>>                          MMC_MODE_WIDTH_BITS_SHIFT);
>> 	for (; width >= 0; width--) {
>> 	....
>>
>> So for FSL ESDHC, the width = 3, after implement mmc_switch successfully,
>> will set the bus to 4 * width. Therefore, I will get the 12bit (4 x 3) bus
>> width.
This problem is MMC subsystem's bug.
I think good that will modify the code in mmc.c.
If caps is set to MMC_MODE_4BIT | MMC_MODE_8BIT,
we can see the 12bit support with using "mmcinfo" command

Best Regards,
Jaehoon Chung

>>
>> Below is the old codes (width = 2):
>> } else {
>>                 for (width = EXT_CSD_BUS_WIDTH_8; width >= 0; width--) {
> [...]
> 
> 
> Uh, so it's a bug in the MMC subsystem?
> Best regards,
> Marek Vasut
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Łukasz Majewski - Oct. 30, 2012, 5:35 p.m.
Hi Jaehoon,

> On 10/23/2012 06:50 PM, Marek Vasut wrote:
> > Dear Huang Changming-R66093,
> > 
> >> Best Regards
> >> Jerry Huang
> >>
> >>> -----Original Message-----
> >>> From: Marek Vasut [mailto:marex@denx.de]
> >>> Sent: Tuesday, October 23, 2012 3:24 PM
> >>> To: Huang Changming-R66093
> >>> Cc: u-boot@lists.denx.de; Andy Fleming
> >>> Subject: Re: [PATCH] powerpc/esdhc: force the bus width to 4bit
> >>>
> >>> Dear Chang-Ming.Huang@freescale.com,
> >>>
> >>>> From: Jerry Huang <Chang-Ming.Huang@freescale.com>
> >>>>
> >>>> For the current u-boot codes, only 4bit/1bit SD/SDHC bus width is
> >>>
> >>> support.
> >>>
> >>>> So for MMC card, we also support 4bit bus width, otherwiase, we
> >>>> will
> >>>
> >>>> get the 12bit bus width, which is not correct:
> >>> Andy ... can you please explain? I don't quite understand the
> >>> problem, I thought we had no problem supporting 8bit mmc (esp. if
> >>> the controller handles that for us mostly).
> >>
> >> Yes, the controller support 8bit MMC.
> >>
> >> FSL ESDHC driver set the host_caps = MMC_MODE_4BIT | MMC_MODE_8BIT;
> >> But, the current codes for MMC card has been changed to:
> >>
> >> } else {
> >> 	width = ((mmc->host_caps & MMC_MODE_MASK_WIDTH_BITS) >>
> >>                          MMC_MODE_WIDTH_BITS_SHIFT);

Hmm... looks like it is code done by me :-)
So little explanation shall be given.

This code is necessary for some targets (like Samsung's Goni) which can
only support 4 bit MMC mode.

> >> 	for (; width >= 0; width--) {
> >> 	....
> >>
> >> So for FSL ESDHC, the width = 3, after implement mmc_switch
> >> successfully, will set the bus to 4 * width. Therefore, I will get
> >> the 12bit (4 x 3) bus width.
> This problem is MMC subsystem's bug.
> I think good that will modify the code in mmc.c.
> If caps is set to MMC_MODE_4BIT | MMC_MODE_8BIT,
> we can see the 12bit support with using "mmcinfo" command
> 

The mmc_set_bus_width(mmc, 4 * width) in conjunction to above code
causes the problem.

I agree, that this code shall be refactored.
Lei, what do you think?


> Best Regards,
> Jaehoon Chung
> 
> >>
> >> Below is the old codes (width = 2):
> >> } else {
> >>                 for (width = EXT_CSD_BUS_WIDTH_8; width >= 0;
> >> width--) {
> > [...]
> > 
> > 
> > Uh, so it's a bug in the MMC subsystem?
> > Best regards,
> > Marek Vasut
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot@lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
> > 
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Jaehoon Chung - Oct. 31, 2012, 4:51 a.m.
Hi, Lukasz,
>>>>>> For the current u-boot codes, only 4bit/1bit SD/SDHC bus width is
>>>>>
>>>>> support.
>>>>>
>>>>>> So for MMC card, we also support 4bit bus width, otherwiase, we
>>>>>> will
>>>>>
>>>>>> get the 12bit bus width, which is not correct:
>>>>> Andy ... can you please explain? I don't quite understand the
>>>>> problem, I thought we had no problem supporting 8bit mmc (esp. if
>>>>> the controller handles that for us mostly).
>>>>
>>>> Yes, the controller support 8bit MMC.
>>>>
>>>> FSL ESDHC driver set the host_caps = MMC_MODE_4BIT | MMC_MODE_8BIT;
>>>> But, the current codes for MMC card has been changed to:
>>>>
>>>> } else {
>>>> 	width = ((mmc->host_caps & MMC_MODE_MASK_WIDTH_BITS) >>
>>>>                          MMC_MODE_WIDTH_BITS_SHIFT);
> 
> Hmm... looks like it is code done by me :-)
> So little explanation shall be given.
> 
> This code is necessary for some targets (like Samsung's Goni) which can
> only support 4 bit MMC mode.
> 
>>>> 	for (; width >= 0; width--) {
>>>> 	....
>>>>
>>>> So for FSL ESDHC, the width = 3, after implement mmc_switch
>>>> successfully, will set the bus to 4 * width. Therefore, I will get
>>>> the 12bit (4 x 3) bus width.
>> This problem is MMC subsystem's bug.
>> I think good that will modify the code in mmc.c.
>> If caps is set to MMC_MODE_4BIT | MMC_MODE_8BIT,
>> we can see the 12bit support with using "mmcinfo" command
>>
> 
> The mmc_set_bus_width(mmc, 4 * width) in conjunction to above code
> causes the problem.
then how about using the width[idx] like kernel?

Best Regards,
Jaehoon Chung
> 
> I agree, that this code shall be refactored.
> Lei, what do you think?
> 
> 
>> Best Regards,
>> Jaehoon Chung
>>
>>>>
>>>> Below is the old codes (width = 2):
>>>> } else {
>>>>                 for (width = EXT_CSD_BUS_WIDTH_8; width >= 0;
>>>> width--) {
>>> [...]
>>>
>>>
>>> Uh, so it's a bug in the MMC subsystem?
>>> Best regards,
>>> Marek Vasut
>>> _______________________________________________
>>> U-Boot mailing list
>>> U-Boot@lists.denx.de
>>> http://lists.denx.de/mailman/listinfo/u-boot
>>>
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
> 
> 
>
Łukasz Majewski - Oct. 31, 2012, 8:20 a.m.
Hi Jaehoon,

> Hi, Lukasz,
> >>>>>> For the current u-boot codes, only 4bit/1bit SD/SDHC bus width
> >>>>>> is
> >>>>>
> >>>>> support.
> >>>>>
> >>>>>> So for MMC card, we also support 4bit bus width, otherwiase, we
> >>>>>> will
> >>>>>
> >>>>>> get the 12bit bus width, which is not correct:
> >>>>> Andy ... can you please explain? I don't quite understand the
> >>>>> problem, I thought we had no problem supporting 8bit mmc (esp.
> >>>>> if the controller handles that for us mostly).
> >>>>
> >>>> Yes, the controller support 8bit MMC.
> >>>>
> >>>> FSL ESDHC driver set the host_caps = MMC_MODE_4BIT |
> >>>> MMC_MODE_8BIT; But, the current codes for MMC card has been
> >>>> changed to:
> >>>>
> >>>> } else {
> >>>> 	width = ((mmc->host_caps & MMC_MODE_MASK_WIDTH_BITS) >>
> >>>>                          MMC_MODE_WIDTH_BITS_SHIFT);
> > 
> > Hmm... looks like it is code done by me :-)
> > So little explanation shall be given.
> > 
> > This code is necessary for some targets (like Samsung's Goni) which
> > can only support 4 bit MMC mode.
> > 
> >>>> 	for (; width >= 0; width--) {
> >>>> 	....
> >>>>
> >>>> So for FSL ESDHC, the width = 3, after implement mmc_switch
> >>>> successfully, will set the bus to 4 * width. Therefore, I will
> >>>> get the 12bit (4 x 3) bus width.
> >> This problem is MMC subsystem's bug.
> >> I think good that will modify the code in mmc.c.
> >> If caps is set to MMC_MODE_4BIT | MMC_MODE_8BIT,
> >> we can see the 12bit support with using "mmcinfo" command
> >>
> > 
> > The mmc_set_bus_width(mmc, 4 * width) in conjunction to above code
> > causes the problem.
> then how about using the width[idx] like kernel?

I don't mind :-) if it works at kernel.

> 
> Best Regards,
> Jaehoon Chung
> > 
> > I agree, that this code shall be refactored.
> > Lei, what do you think?
> > 
> > 
> >> Best Regards,
> >> Jaehoon Chung
> >>
> >>>>
> >>>> Below is the old codes (width = 2):
> >>>> } else {
> >>>>                 for (width = EXT_CSD_BUS_WIDTH_8; width >= 0;
> >>>> width--) {
> >>> [...]
> >>>
> >>>
> >>> Uh, so it's a bug in the MMC subsystem?
> >>> Best regards,
> >>> Marek Vasut
> >>> _______________________________________________
> >>> U-Boot mailing list
> >>> U-Boot@lists.denx.de
> >>> http://lists.denx.de/mailman/listinfo/u-boot
> >>>
> >>
> >> _______________________________________________
> >> U-Boot mailing list
> >> U-Boot@lists.denx.de
> >> http://lists.denx.de/mailman/listinfo/u-boot
> > 
> > 
> > 
>
Jaehoon Chung - Oct. 31, 2012, 10:43 a.m.
Hi Lukasz,

	width = ((mmc->host_caps & MMC_MODE_MASK_WIDTH_BITS) >>
	                          MMC_MODE_WIDTH_BITS_SHIFT);

This code has the problem. If width is set to 0x3,
then BUS_WIDTH field of ext_csd register is set to 0x3.
Value 0x3 is nothing.(It's reserved)
If we want to set 4-bit, then that value is set to 0x2.
This problem need to fix.
I will send the patch for this problem.

Best Regards,
Jaehoon Chung

On 10/31/2012 05:20 PM, Lukasz Majewski wrote:
> Hi Jaehoon,
> 
>> Hi, Lukasz,
>>>>>>>> For the current u-boot codes, only 4bit/1bit SD/SDHC bus width
>>>>>>>> is
>>>>>>>
>>>>>>> support.
>>>>>>>
>>>>>>>> So for MMC card, we also support 4bit bus width, otherwiase, we
>>>>>>>> will
>>>>>>>
>>>>>>>> get the 12bit bus width, which is not correct:
>>>>>>> Andy ... can you please explain? I don't quite understand the
>>>>>>> problem, I thought we had no problem supporting 8bit mmc (esp.
>>>>>>> if the controller handles that for us mostly).
>>>>>>
>>>>>> Yes, the controller support 8bit MMC.
>>>>>>
>>>>>> FSL ESDHC driver set the host_caps = MMC_MODE_4BIT |
>>>>>> MMC_MODE_8BIT; But, the current codes for MMC card has been
>>>>>> changed to:
>>>>>>
>>>>>> } else {
>>>>>> 	width = ((mmc->host_caps & MMC_MODE_MASK_WIDTH_BITS) >>
>>>>>>                          MMC_MODE_WIDTH_BITS_SHIFT);
>>>
>>> Hmm... looks like it is code done by me :-)
>>> So little explanation shall be given.
>>>
>>> This code is necessary for some targets (like Samsung's Goni) which
>>> can only support 4 bit MMC mode.
>>>
>>>>>> 	for (; width >= 0; width--) {
>>>>>> 	....
>>>>>>
>>>>>> So for FSL ESDHC, the width = 3, after implement mmc_switch
>>>>>> successfully, will set the bus to 4 * width. Therefore, I will
>>>>>> get the 12bit (4 x 3) bus width.
>>>> This problem is MMC subsystem's bug.
>>>> I think good that will modify the code in mmc.c.
>>>> If caps is set to MMC_MODE_4BIT | MMC_MODE_8BIT,
>>>> we can see the 12bit support with using "mmcinfo" command
>>>>
>>>
>>> The mmc_set_bus_width(mmc, 4 * width) in conjunction to above code
>>> causes the problem.
>> then how about using the width[idx] like kernel?
> 
> I don't mind :-) if it works at kernel.
> 
>>
>> Best Regards,
>> Jaehoon Chung
>>>
>>> I agree, that this code shall be refactored.
>>> Lei, what do you think?
>>>
>>>
>>>> Best Regards,
>>>> Jaehoon Chung
>>>>
>>>>>>
>>>>>> Below is the old codes (width = 2):
>>>>>> } else {
>>>>>>                 for (width = EXT_CSD_BUS_WIDTH_8; width >= 0;
>>>>>> width--) {
>>>>> [...]
>>>>>
>>>>>
>>>>> Uh, so it's a bug in the MMC subsystem?
>>>>> Best regards,
>>>>> Marek Vasut
>>>>> _______________________________________________
>>>>> U-Boot mailing list
>>>>> U-Boot@lists.denx.de
>>>>> http://lists.denx.de/mailman/listinfo/u-boot
>>>>>
>>>>
>>>> _______________________________________________
>>>> U-Boot mailing list
>>>> U-Boot@lists.denx.de
>>>> http://lists.denx.de/mailman/listinfo/u-boot
>>>
>>>
>>>
>>
> 
> 
>
Andy Fleming - Nov. 1, 2012, 4:09 a.m.
>
> Hmm... looks like it is code done by me :-)
> So little explanation shall be given.
>
> This code is necessary for some targets (like Samsung's Goni) which can
> only support 4 bit MMC mode.
>
>> >>    for (; width >= 0; width--) {
>> >>    ....
>> >>
>> >> So for FSL ESDHC, the width = 3, after implement mmc_switch
>> >> successfully, will set the bus to 4 * width. Therefore, I will get
>> >> the 12bit (4 x 3) bus width.
>> This problem is MMC subsystem's bug.
>> I think good that will modify the code in mmc.c.
>> If caps is set to MMC_MODE_4BIT | MMC_MODE_8BIT,
>> we can see the 12bit support with using "mmcinfo" command
>>
>
> The mmc_set_bus_width(mmc, 4 * width) in conjunction to above code
> causes the problem.
>
> I agree, that this code shall be refactored.
> Lei, what do you think?


I am... very confused by this whole thread. And the code associated
with it. The host_caps field has a bitmask which declares the widths
supported by a given controller.

What would possess you to index them by addition, and convert their
values by multiplication?? It's a bitfield! I'm embarrassed that I
allowed this code in, and will review future submissions from you with
a very skeptical eye.

Ah, and further review indicates it is Lei Wen who introduced the idea
of iterating through a bitfield by subtraction, though I can see how
iterating through the EXT_CSD *field* definition (which looks a lot
like a bitfield, but may not be) *might* be considered reasonable.

Meanwhile, Huang Changming, why would you see this broken code, and
then decide the best workaround was to cripple our controller by
eliminating support for 8-bit?

I'm going to fix this right now. Probably in the quite sensible way
that Jaehoon Chung suggested.

Andy
Changming Huang - Nov. 2, 2012, 1:54 a.m.
Best Regards
Jerry Huang


> -----Original Message-----
> From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces@lists.denx.de]
> On Behalf Of Andy Fleming
> Sent: Thursday, November 01, 2012 12:09 PM
> To: Lukasz Majewski
> Cc: Jaehoon Chung; Marek Vasut; Lei Wen; u-boot@lists.denx.de
> Subject: Re: [U-Boot] [PATCH] powerpc/esdhc: force the bus width to 4bit
> 
> >
> > Hmm... looks like it is code done by me :-) So little explanation
> > shall be given.
> >
> > This code is necessary for some targets (like Samsung's Goni) which
> > can only support 4 bit MMC mode.
> >
> >> >>    for (; width >= 0; width--) {
> >> >>    ....
> >> >>
> >> >> So for FSL ESDHC, the width = 3, after implement mmc_switch
> >> >> successfully, will set the bus to 4 * width. Therefore, I will get
> >> >> the 12bit (4 x 3) bus width.
> >> This problem is MMC subsystem's bug.
> >> I think good that will modify the code in mmc.c.
> >> If caps is set to MMC_MODE_4BIT | MMC_MODE_8BIT, we can see the 12bit
> >> support with using "mmcinfo" command
> >>
> >
> > The mmc_set_bus_width(mmc, 4 * width) in conjunction to above code
> > causes the problem.
> >
> > I agree, that this code shall be refactored.
> > Lei, what do you think?
> 
> 
> I am... very confused by this whole thread. And the code associated with
> it. The host_caps field has a bitmask which declares the widths supported
> by a given controller.
> 
> What would possess you to index them by addition, and convert their
> values by multiplication?? It's a bitfield! I'm embarrassed that I
> allowed this code in, and will review future submissions from you with a
> very skeptical eye.
> 
> Ah, and further review indicates it is Lei Wen who introduced the idea of
> iterating through a bitfield by subtraction, though I can see how
> iterating through the EXT_CSD *field* definition (which looks a lot like
> a bitfield, but may not be) *might* be considered reasonable.
> 
> Meanwhile, Huang Changming, why would you see this broken code, and then
> decide the best workaround was to cripple our controller by eliminating
> support for 8-bit?
I see 12bit width when using mmcinfo, then found out the mmc.c has been changed, and I assume this change is right,
So I have to force our controller to just support 4bit width, otherwise, the 12bit bus width is not correct.
Jaehoon Chung - Nov. 2, 2012, 2:03 a.m.
Hi,

Andy has sent the patch related with this problem.
Check the patch "[RFC] mmc: Properly determine maximum supported bus width"

Best Regards,
Jaehoon Chung
>> decide the best workaround was to cripple our controller by eliminating
>> support for 8-bit?
> I see 12bit width when using mmcinfo, then found out the mmc.c has been changed, and I assume this change is right,
> So I have to force our controller to just support 4bit width, otherwise, the 12bit bus width is not correct.
> 
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Changming Huang - Nov. 2, 2012, 2:15 a.m.
Thanks, Jaehoon, I saw it.

Best Regards
Jerry Huang


> -----Original Message-----
> From: Jaehoon Chung [mailto:jh80.chung@samsung.com]
> Sent: Friday, November 02, 2012 10:03 AM
> To: Huang Changming-R66093
> Cc: Andy Fleming; Lukasz Majewski; Jaehoon Chung; Marek Vasut; u-
> boot@lists.denx.de; Wen; Lei@theia.denx.de
> Subject: Re: [U-Boot] [PATCH] powerpc/esdhc: force the bus width to 4bit
> 
> Hi,
> 
> Andy has sent the patch related with this problem.
> Check the patch "[RFC] mmc: Properly determine maximum supported bus
> width"
> 
> Best Regards,
> Jaehoon Chung
> >> decide the best workaround was to cripple our controller by
> >> eliminating support for 8-bit?
> > I see 12bit width when using mmcinfo, then found out the mmc.c has
> > been changed, and I assume this change is right, So I have to force our
> controller to just support 4bit width, otherwise, the 12bit bus width is
> not correct.
> >
> >
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot@lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
> >
>
Marek Vasut - Nov. 2, 2012, 3:40 a.m.
Dear Huang Changming-R66093,

> Thanks, Jaehoon, I saw it.
> 
> Best Regards
> Jerry Huang

I wonder if you're just making fun of all of us or what you're actually trying 
to acomplish here with the constant top-posting ... I'm really stunned :-(

> > -----Original Message-----
> > From: Jaehoon Chung [mailto:jh80.chung@samsung.com]
> > Sent: Friday, November 02, 2012 10:03 AM
> > To: Huang Changming-R66093
> > Cc: Andy Fleming; Lukasz Majewski; Jaehoon Chung; Marek Vasut; u-
> > boot@lists.denx.de; Wen; Lei@theia.denx.de
> > Subject: Re: [U-Boot] [PATCH] powerpc/esdhc: force the bus width to 4bit
> > 
> > Hi,
> > 
> > Andy has sent the patch related with this problem.
> > Check the patch "[RFC] mmc: Properly determine maximum supported bus
> > width"
> > 
> > Best Regards,
> > Jaehoon Chung
> > 
> > >> decide the best workaround was to cripple our controller by
> > >> eliminating support for 8-bit?
> > > 
> > > I see 12bit width when using mmcinfo, then found out the mmc.c has
> > > been changed, and I assume this change is right, So I have to force our
> > 
> > controller to just support 4bit width, otherwise, the 12bit bus width is
> > not correct.
> > 
> > > _______________________________________________
> > > U-Boot mailing list
> > > U-Boot@lists.denx.de
> > > http://lists.denx.de/mailman/listinfo/u-boot

Best regards,
Marek Vasut
Łukasz Majewski - Nov. 2, 2012, 7:38 a.m.
Hi Andy,

> >
> > Hmm... looks like it is code done by me :-)
> > So little explanation shall be given.
> >
> > This code is necessary for some targets (like Samsung's Goni) which
> > can only support 4 bit MMC mode.
> >
> >> >>    for (; width >= 0; width--) {
> >> >>    ....
> >> >>
> >> >> So for FSL ESDHC, the width = 3, after implement mmc_switch
> >> >> successfully, will set the bus to 4 * width. Therefore, I will
> >> >> get the 12bit (4 x 3) bus width.
> >> This problem is MMC subsystem's bug.
> >> I think good that will modify the code in mmc.c.
> >> If caps is set to MMC_MODE_4BIT | MMC_MODE_8BIT,
> >> we can see the 12bit support with using "mmcinfo" command
> >>
> >
> > The mmc_set_bus_width(mmc, 4 * width) in conjunction to above code
> > causes the problem.
> >
> > I agree, that this code shall be refactored.
> > Lei, what do you think?
> 
> 
> I am... very confused by this whole thread. And the code associated
> with it. The host_caps field has a bitmask which declares the widths
> supported by a given controller.
> 
> What would possess you to index them by addition, and convert their
> values by multiplication?? It's a bitfield! I'm embarrassed that I
> allowed this code in, and will review future submissions from you with
> a very skeptical eye.

I admit, that this code was a hack, I will even say more (after the "big
picture" presented by You) - I've delivered very low quality code.

Shame on me - my fault. Period. 



> 
> Ah, and further review indicates it is Lei Wen who introduced the idea
> of iterating through a bitfield by subtraction, though I can see how
> iterating through the EXT_CSD *field* definition (which looks a lot
> like a bitfield, but may not be) *might* be considered reasonable.
> 
> Meanwhile, Huang Changming, why would you see this broken code, and
> then decide the best workaround was to cripple our controller by
> eliminating support for 8-bit?
> 
> I'm going to fix this right now. Probably in the quite sensible way
> that Jaehoon Chung suggested.
> 
> Andy

Patch

diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
index 3f8d30d..7b83dd2 100644
--- a/drivers/mmc/fsl_esdhc.c
+++ b/drivers/mmc/fsl_esdhc.c
@@ -577,7 +577,7 @@  int fsl_esdhc_initialize(bd_t *bis, struct fsl_esdhc_cfg *cfg)
 		return -1;
 	}
 
-	mmc->host_caps = MMC_MODE_4BIT | MMC_MODE_8BIT;
+	mmc->host_caps = MMC_MODE_4BIT;
 
 	if (caps & ESDHC_HOSTCAPBLT_HSS)
 		mmc->host_caps |= MMC_MODE_HS_52MHz | MMC_MODE_HS;