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

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

[Marek Vasut]

Dear Huang Changming-R66093,

Fixing the ML address ... and reposting the rest.

> Andy,
> Do you agree my suggest to revert these codes to fix this issue?

Please ... do us all a favor, read [1] and stop being so inconsiderate of 
others! Given that I can no longer make any sense of this thread because of your 
top-posting making it complete utmost mess, my opinion is NAK on reverting them.

[1] http://www.ietf.org/rfc/rfc1855.txt

> Best Regards
> Jerry Huang
> 
> > -----Original Message-----
> > From: u-boot-bounces at linux.freescale.net [mailto:u-boot-
> > bounces at linux.freescale.net] On Behalf Of Huang Changming-R66093
> > Sent: Wednesday, October 31, 2012 10:08 AM
> > To: Andy Fleming; Marek Vasut
> > Cc: u-boot at linux.freescale.net
> > Subject: Re: [u-boot] [PATCH] powerpc/esdhc: force the bus width to 4bit
> > 
> > Below is the old codes (width = 2):
> > 
> > } else {
> > 
> >                 for (width = EXT_CSD_BUS_WIDTH_8; width >= 0; width--) {
> > 
> > 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--) {
> > 	....
> > 
> > FSL ESDHC driver set the host_caps = MMC_MODE_4BIT | MMC_MODE_8BIT;
> > 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.
> > 
> > I don't know why do this modification?
> > If to fix the stack bug, then I need to revert these codes.
> > Andy, do you agree it?
> > 
> > Best Regards
> > Jerry Huang
> > 
> > > -----Original Message-----
> > > From: Andy Fleming [mailto:afleming at gmail.com]
> > > Sent: Tuesday, October 30, 2012 10:34 PM
> > > To: Marek Vasut
> > > Cc: Huang Changming-R66093; u-boot at linux.freescale.net
> > > Subject: Re: [PATCH] powerpc/esdhc: force the bus width to 4bit
> > > 
> > > On Oct 30, 2012, at 5:50, Marek Vasut <marex@denx.de> wrote:
> > > > Dear Chang-Ming.Huang at 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:
> > > > [...]
> > > > 
> > > >> --- 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;
> > > > 
> > > > This fix is not correct, fix the MMC subsystem please.
> > > 
> > > Agreed. Bit width just needs smarter printing. Unless this patch is
> > 
> > doing
> > 
> > > more than ensuring the printout is correct
> > > 
> > > Andy
> > 
> > _______________________________________________
> > u-boot mailing list
> > u-boot at linux.freescale.net
> > http://linux.freescale.net/mailman/listinfo/u-boot

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

[Marek Vasut]

Dear Huang Changming-R66093,

Reposting _AGAIN_ because you again discarded the U-Boot ML address.

> Best Regards
> Jerry Huang

Can you please fix your mailer?

> > -----Original Message-----
> > From: Marek Vasut [mailto:marex at denx.de]
> > Sent: Thursday, November 01, 2012 10:26 AM
> > To: Huang Changming-R66093
> > Cc: Andy Fleming; u-boot at linux.freescale.net
> > Subject: Re: [PATCH] powerpc/esdhc: force the bus width to 4bit
> > 
> > Dear Huang Changming-R66093,
> > 
> > > Andy,
> > > Do you agree my suggest to revert these codes to fix this issue?
> > 
> > Please ... do us all a favor, read [1] and stop being so inconsiderate of
> > others! Given that I can no longer make any sense of this thread because
> > of your
> > top-posting making it complete utmost mess, my opinion is NAK on
> > reverting them.
> > 
> > [1] http://www.ietf.org/rfc/rfc1855.txt
> 
> Then, could you give some suggest, how to fix the 12bit-width issue?

see drivers/mmc/mmc.c:

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

Fix this snipped of code and the related code below ... that's the way to go 
about it.

Best regards,
Marek Vasut

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

[Huang Changming-R66093]

> -----Original Message-----
> From: Marek Vasut [mailto:marex at denx.de]
> Sent: Thursday, November 01, 2012 11:06 AM
> To: Huang Changming-R66093
> Cc: Andy Fleming; u-boot at linux.freescale.net; u-boot at lists.denx.de
> Subject: Re: [PATCH] powerpc/esdhc: force the bus width to 4bit
> 
> Dear Huang Changming-R66093,
> 
> Reposting _AGAIN_ because you again discarded the U-Boot ML address.

I don't know why, I replied to all, not discard any address.

> 
> > > -----Original Message-----
> > > From: Marek Vasut [mailto:marex at denx.de]
> > > Sent: Thursday, November 01, 2012 10:26 AM
> > > To: Huang Changming-R66093
> > > Cc: Andy Fleming; u-boot at linux.freescale.net
> > > Subject: Re: [PATCH] powerpc/esdhc: force the bus width to 4bit
> > >
> > > Dear Huang Changming-R66093,
> > >
> > > > Andy,
> > > > Do you agree my suggest to revert these codes to fix this issue?
> > >
> > > Please ... do us all a favor, read [1] and stop being so
> > > inconsiderate of others! Given that I can no longer make any sense
> > > of this thread because of your top-posting making it complete utmost
> > > mess, my opinion is NAK on reverting them.
> > >
> > > [1] http://www.ietf.org/rfc/rfc1855.txt
> >
> > Then, could you give some suggest, how to fix the 12bit-width issue?
> 
> see drivers/mmc/mmc.c:
> 
> 1089 width = ((mmc->host_caps & MMC_MODE_MASK_WIDTH_BITS) >>
> 1090          MMC_MODE_WIDTH_BITS_SHIFT);
> 
> Fix this snipped of code and the related code below ... that's the way to
> go about it.
> 
Yes, I can change these codes to:
Width = EXT_CSD_BUS_WIDTH_8;

Then, first value of width will be 2.

But, those modification is I said, the codes is reverted to the old codes.

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

[Chang-Ming.Huang at 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:
=> 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;
-- 
1.7.9.5

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

[Marek Vasut]

Dear Chang-Ming.Huang at 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

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

[Huang Changming-R66093]

Best Regards
Jerry Huang


> -----Original Message-----
> From: Marek Vasut [mailto:marex at denx.de]
> Sent: Tuesday, October 23, 2012 3:24 PM
> To: Huang Changming-R66093
> Cc: u-boot at lists.denx.de; Andy Fleming
> Subject: Re: [PATCH] powerpc/esdhc: force the bus width to 4bit
> 
> Dear Chang-Ming.Huang at 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

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

[Marek Vasut]

Dear Huang Changming-R66093,

> Best Regards
> Jerry Huang
> 
> > -----Original Message-----
> > From: Marek Vasut [mailto:marex at denx.de]
> > Sent: Tuesday, October 23, 2012 3:24 PM
> > To: Huang Changming-R66093
> > Cc: u-boot at lists.denx.de; Andy Fleming
> > Subject: Re: [PATCH] powerpc/esdhc: force the bus width to 4bit
> > 
> > Dear Chang-Ming.Huang at 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

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

[Huang Changming-R66093]

> > > Dear Chang-Ming.Huang at 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.

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

[Jaehoon Chung]

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 at denx.de]
>>> Sent: Tuesday, October 23, 2012 3:24 PM
>>> To: Huang Changming-R66093
>>> Cc: u-boot at lists.denx.de; Andy Fleming
>>> Subject: Re: [PATCH] powerpc/esdhc: force the bus width to 4bit
>>>
>>> Dear Chang-Ming.Huang at 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 at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
> 

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

[Lukasz Majewski]

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 at denx.de]
> >>> Sent: Tuesday, October 23, 2012 3:24 PM
> >>> To: Huang Changming-R66093
> >>> Cc: u-boot at lists.denx.de; Andy Fleming
> >>> Subject: Re: [PATCH] powerpc/esdhc: force the bus width to 4bit
> >>>
> >>> Dear Chang-Ming.Huang at 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 at lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
> > 
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot



-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group

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

[Jaehoon Chung]

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 at lists.denx.de
>>> http://lists.denx.de/mailman/listinfo/u-boot
>>>
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
> 
> 
> 

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

[Lukasz Majewski]

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 at lists.denx.de
> >>> http://lists.denx.de/mailman/listinfo/u-boot
> >>>
> >>
> >> _______________________________________________
> >> U-Boot mailing list
> >> U-Boot at lists.denx.de
> >> http://lists.denx.de/mailman/listinfo/u-boot
> > 
> > 
> > 
> 



-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group

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

[Jaehoon Chung]

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 at lists.denx.de
>>>>> http://lists.denx.de/mailman/listinfo/u-boot
>>>>>
>>>>
>>>> _______________________________________________
>>>> U-Boot mailing list
>>>> U-Boot at lists.denx.de
>>>> http://lists.denx.de/mailman/listinfo/u-boot
>>>
>>>
>>>
>>
> 
> 
> 

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

[Andy Fleming]

>
> 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

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

[Huang Changming-R66093]

Best Regards
Jerry Huang


> -----Original Message-----
> From: u-boot-bounces at lists.denx.de [mailto:u-boot-bounces at 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 at 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.

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

[Jaehoon Chung]

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 at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
> 

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

[Huang Changming-R66093]

Thanks, Jaehoon, I saw it.

Best Regards
Jerry Huang


> -----Original Message-----
> From: Jaehoon Chung [mailto:jh80.chung at 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 at lists.denx.de; Wen; Lei at 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 at lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
> >
> 

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

[Marek Vasut]

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 at 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 at lists.denx.de; Wen; Lei at 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 at lists.denx.de
> > > http://lists.denx.de/mailman/listinfo/u-boot

Best regards,
Marek Vasut

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

[Lukasz Majewski]

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


-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group