All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [v2] mmc: fsl_esdhc: fix mmc read/write error on T4080
@ 2016-01-07  8:50 Yangbo Lu
  2016-01-09  0:23 ` Andy Fleming
  0 siblings, 1 reply; 8+ messages in thread
From: Yangbo Lu @ 2016-01-07  8:50 UTC (permalink / raw)
  To: u-boot

Fill the right command type when using CMD12 to stop data transfer.

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
---
Changes for v2:
	- Removed fix for T4160 because other patch had done that
---
 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 c5054d6..16fb01a 100644
--- a/drivers/mmc/fsl_esdhc.c
+++ b/drivers/mmc/fsl_esdhc.c
@@ -107,7 +107,7 @@ static uint esdhc_xfertyp(struct mmc_cmd *cmd, struct mmc_data *data)
 
 #if defined(CONFIG_MX53) || defined(CONFIG_PPC_T4240) || \
 	defined(CONFIG_LS102XA) || defined(CONFIG_FSL_LAYERSCAPE) || \
-	defined(CONFIG_PPC_T4160)
+	defined(CONFIG_PPC_T4160) || defined(CONFIG_PPC_T4080)
 	if (cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION)
 		xfertyp |= XFERTYP_CMDTYP_ABORT;
 #endif
-- 
2.1.0.27.g96db324

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [U-Boot] [v2] mmc: fsl_esdhc: fix mmc read/write error on T4080
  2016-01-07  8:50 [U-Boot] [v2] mmc: fsl_esdhc: fix mmc read/write error on T4080 Yangbo Lu
@ 2016-01-09  0:23 ` Andy Fleming
  2016-01-14 17:51   ` york sun
  0 siblings, 1 reply; 8+ messages in thread
From: Andy Fleming @ 2016-01-09  0:23 UTC (permalink / raw)
  To: u-boot

On Thu, Jan 7, 2016 at 2:50 AM, Yangbo Lu <yangbo.lu@nxp.com> wrote:
> Fill the right command type when using CMD12 to stop data transfer.
>
> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> ---
> Changes for v2:
>         - Removed fix for T4160 because other patch had done that
> ---
>  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 c5054d6..16fb01a 100644
> --- a/drivers/mmc/fsl_esdhc.c
> +++ b/drivers/mmc/fsl_esdhc.c
> @@ -107,7 +107,7 @@ static uint esdhc_xfertyp(struct mmc_cmd *cmd, struct mmc_data *data)
>
>  #if defined(CONFIG_MX53) || defined(CONFIG_PPC_T4240) || \
>         defined(CONFIG_LS102XA) || defined(CONFIG_FSL_LAYERSCAPE) || \
> -       defined(CONFIG_PPC_T4160)
> +       defined(CONFIG_PPC_T4160) || defined(CONFIG_PPC_T4080)
>         if (cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION)
>                 xfertyp |= XFERTYP_CMDTYP_ABORT;
>  #endif


These sorts of chip-specific #ifdefs are very hard to maintain. It
would be far better if there were a single define, like:

#ifdef CONFIG_FSL_ESDHC_USES_ABORT_TO_STOP

And then define that on any system that needs this code. With the
current version, I have no idea why this code is needed. I have
guesses, but in order to be sure, I'd have to check several reference
manuals. With my suggestion, it is obvious to everyone why this code
is here, and it gives a hint to those who are adding support to new
chips.

Now, I'm not saying you should use my suggestion precisely. Perhaps
every version of the esdhc after some point uses this mechanism. Then
you could use that information. And perhaps my naming doesn't reflect
what is happening. My point is, you're going to have to do this again
when you release LS232XB, and that seems like a poor use of your time.
:)

Andy

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [U-Boot] [v2] mmc: fsl_esdhc: fix mmc read/write error on T4080
  2016-01-09  0:23 ` Andy Fleming
@ 2016-01-14 17:51   ` york sun
  2016-01-14 18:09     ` Tom Rini
  0 siblings, 1 reply; 8+ messages in thread
From: york sun @ 2016-01-14 17:51 UTC (permalink / raw)
  To: u-boot

On 01/08/2016 04:23 PM, Andy Fleming wrote:
> On Thu, Jan 7, 2016 at 2:50 AM, Yangbo Lu <yangbo.lu@nxp.com> wrote:
>> Fill the right command type when using CMD12 to stop data transfer.
>>
>> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
>> ---
>> Changes for v2:
>>         - Removed fix for T4160 because other patch had done that
>> ---
>>  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 c5054d6..16fb01a 100644
>> --- a/drivers/mmc/fsl_esdhc.c
>> +++ b/drivers/mmc/fsl_esdhc.c
>> @@ -107,7 +107,7 @@ static uint esdhc_xfertyp(struct mmc_cmd *cmd, struct mmc_data *data)
>>
>>  #if defined(CONFIG_MX53) || defined(CONFIG_PPC_T4240) || \
>>         defined(CONFIG_LS102XA) || defined(CONFIG_FSL_LAYERSCAPE) || \
>> -       defined(CONFIG_PPC_T4160)
>> +       defined(CONFIG_PPC_T4160) || defined(CONFIG_PPC_T4080)
>>         if (cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION)
>>                 xfertyp |= XFERTYP_CMDTYP_ABORT;
>>  #endif
> 
> 
> These sorts of chip-specific #ifdefs are very hard to maintain. It
> would be far better if there were a single define, like:
> 
> #ifdef CONFIG_FSL_ESDHC_USES_ABORT_TO_STOP
> 
> And then define that on any system that needs this code. With the
> current version, I have no idea why this code is needed. I have
> guesses, but in order to be sure, I'd have to check several reference
> manuals. With my suggestion, it is obvious to everyone why this code
> is here, and it gives a hint to those who are adding support to new
> chips.
> 
> Now, I'm not saying you should use my suggestion precisely. Perhaps
> every version of the esdhc after some point uses this mechanism. Then
> you could use that information. And perhaps my naming doesn't reflect
> what is happening. My point is, you're going to have to do this again
> when you release LS232XB, and that seems like a poor use of your time.
> :)
> 

Yangbo,

I agree with Andy on this. Please make the suggested change.

York

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [U-Boot] [v2] mmc: fsl_esdhc: fix mmc read/write error on T4080
  2016-01-14 17:51   ` york sun
@ 2016-01-14 18:09     ` Tom Rini
  2016-01-18  3:22       ` Yangbo Lu
  2016-01-18  7:18       ` Yangbo Lu
  0 siblings, 2 replies; 8+ messages in thread
From: Tom Rini @ 2016-01-14 18:09 UTC (permalink / raw)
  To: u-boot

On Thu, Jan 14, 2016 at 05:51:32PM +0000, york sun wrote:
> On 01/08/2016 04:23 PM, Andy Fleming wrote:
> > On Thu, Jan 7, 2016 at 2:50 AM, Yangbo Lu <yangbo.lu@nxp.com> wrote:
> >> Fill the right command type when using CMD12 to stop data transfer.
> >>
> >> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> >> ---
> >> Changes for v2:
> >>         - Removed fix for T4160 because other patch had done that
> >> ---
> >>  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 c5054d6..16fb01a 100644
> >> --- a/drivers/mmc/fsl_esdhc.c
> >> +++ b/drivers/mmc/fsl_esdhc.c
> >> @@ -107,7 +107,7 @@ static uint esdhc_xfertyp(struct mmc_cmd *cmd, struct mmc_data *data)
> >>
> >>  #if defined(CONFIG_MX53) || defined(CONFIG_PPC_T4240) || \
> >>         defined(CONFIG_LS102XA) || defined(CONFIG_FSL_LAYERSCAPE) || \
> >> -       defined(CONFIG_PPC_T4160)
> >> +       defined(CONFIG_PPC_T4160) || defined(CONFIG_PPC_T4080)
> >>         if (cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION)
> >>                 xfertyp |= XFERTYP_CMDTYP_ABORT;
> >>  #endif
> > 
> > 
> > These sorts of chip-specific #ifdefs are very hard to maintain. It
> > would be far better if there were a single define, like:
> > 
> > #ifdef CONFIG_FSL_ESDHC_USES_ABORT_TO_STOP
> > 
> > And then define that on any system that needs this code. With the
> > current version, I have no idea why this code is needed. I have
> > guesses, but in order to be sure, I'd have to check several reference
> > manuals. With my suggestion, it is obvious to everyone why this code
> > is here, and it gives a hint to those who are adding support to new
> > chips.
> > 
> > Now, I'm not saying you should use my suggestion precisely. Perhaps
> > every version of the esdhc after some point uses this mechanism. Then
> > you could use that information. And perhaps my naming doesn't reflect
> > what is happening. My point is, you're going to have to do this again
> > when you release LS232XB, and that seems like a poor use of your time.
> > :)
> 
> Yangbo,
> 
> I agree with Andy on this. Please make the suggested change.

... as some sort of Kconfig entry that's ideally selected rather than
prmopted for when applicable.  Like it would be done in the kernel :)

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160114/e072a0a4/attachment.sig>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [U-Boot] [v2] mmc: fsl_esdhc: fix mmc read/write error on T4080
  2016-01-14 18:09     ` Tom Rini
@ 2016-01-18  3:22       ` Yangbo Lu
  2016-01-18  7:18       ` Yangbo Lu
  1 sibling, 0 replies; 8+ messages in thread
From: Yangbo Lu @ 2016-01-18  3:22 UTC (permalink / raw)
  To: u-boot

> -----Original Message-----
> From: Tom Rini [mailto:trini at konsulko.com]
> Sent: Friday, January 15, 2016 2:09 AM
> To: york sun
> Cc: Andy Fleming; Yangbo Lu; U-Boot list
> Subject: Re: [U-Boot] [v2] mmc: fsl_esdhc: fix mmc read/write error on
> T4080
> 
> On Thu, Jan 14, 2016 at 05:51:32PM +0000, york sun wrote:
> > On 01/08/2016 04:23 PM, Andy Fleming wrote:
> > > On Thu, Jan 7, 2016 at 2:50 AM, Yangbo Lu <yangbo.lu@nxp.com> wrote:
> > >> Fill the right command type when using CMD12 to stop data transfer.
> > >>
> > >> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > >> ---
> > >> Changes for v2:
> > >>         - Removed fix for T4160 because other patch had done that
> > >> ---
> > >>  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 c5054d6..16fb01a 100644
> > >> --- a/drivers/mmc/fsl_esdhc.c
> > >> +++ b/drivers/mmc/fsl_esdhc.c
> > >> @@ -107,7 +107,7 @@ static uint esdhc_xfertyp(struct mmc_cmd *cmd,
> > >> struct mmc_data *data)
> > >>
> > >>  #if defined(CONFIG_MX53) || defined(CONFIG_PPC_T4240) || \
> > >>         defined(CONFIG_LS102XA) || defined(CONFIG_FSL_LAYERSCAPE) ||
> \
> > >> -       defined(CONFIG_PPC_T4160)
> > >> +       defined(CONFIG_PPC_T4160) || defined(CONFIG_PPC_T4080)
> > >>         if (cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION)
> > >>                 xfertyp |= XFERTYP_CMDTYP_ABORT;  #endif
> > >
> > >
> > > These sorts of chip-specific #ifdefs are very hard to maintain. It
> > > would be far better if there were a single define, like:
> > >
> > > #ifdef CONFIG_FSL_ESDHC_USES_ABORT_TO_STOP
> > >
> > > And then define that on any system that needs this code. With the
> > > current version, I have no idea why this code is needed. I have
> > > guesses, but in order to be sure, I'd have to check several reference
> > > manuals. With my suggestion, it is obvious to everyone why this code
> > > is here, and it gives a hint to those who are adding support to new
> > > chips.
> > >
> > > Now, I'm not saying you should use my suggestion precisely. Perhaps
> > > every version of the esdhc after some point uses this mechanism. Then
> > > you could use that information. And perhaps my naming doesn't reflect
> > > what is happening. My point is, you're going to have to do this again
> > > when you release LS232XB, and that seems like a poor use of your time.
> > > :)
> >
> > Yangbo,
> >
> > I agree with Andy on this. Please make the suggested change.
> 
> ... as some sort of Kconfig entry that's ideally selected rather than
> prmopted for when applicable.  Like it would be done in the kernel :)
> 
> --
> Tom
[Lu Yangbo-B47093] Thank you so much for your suggestion.
I would prepare a v3 patchset.
:)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [U-Boot] [v2] mmc: fsl_esdhc: fix mmc read/write error on T4080
  2016-01-14 18:09     ` Tom Rini
  2016-01-18  3:22       ` Yangbo Lu
@ 2016-01-18  7:18       ` Yangbo Lu
  2016-01-19 15:58         ` Tom Rini
  1 sibling, 1 reply; 8+ messages in thread
From: Yangbo Lu @ 2016-01-18  7:18 UTC (permalink / raw)
  To: u-boot

> -----Original Message-----
> From: Tom Rini [mailto:trini at konsulko.com]
> Sent: Friday, January 15, 2016 2:09 AM
> To: york sun
> Cc: Andy Fleming; Yangbo Lu; U-Boot list
> Subject: Re: [U-Boot] [v2] mmc: fsl_esdhc: fix mmc read/write error on
> T4080
> 
> On Thu, Jan 14, 2016 at 05:51:32PM +0000, york sun wrote:
> > On 01/08/2016 04:23 PM, Andy Fleming wrote:
> > > On Thu, Jan 7, 2016 at 2:50 AM, Yangbo Lu <yangbo.lu@nxp.com> wrote:
> > >> Fill the right command type when using CMD12 to stop data transfer.
> > >>
> > >> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > >> ---
> > >> Changes for v2:
> > >>         - Removed fix for T4160 because other patch had done that
> > >> ---
> > >>  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 c5054d6..16fb01a 100644
> > >> --- a/drivers/mmc/fsl_esdhc.c
> > >> +++ b/drivers/mmc/fsl_esdhc.c
> > >> @@ -107,7 +107,7 @@ static uint esdhc_xfertyp(struct mmc_cmd *cmd,
> > >> struct mmc_data *data)
> > >>
> > >>  #if defined(CONFIG_MX53) || defined(CONFIG_PPC_T4240) || \
> > >>         defined(CONFIG_LS102XA) || defined(CONFIG_FSL_LAYERSCAPE) ||
> \
> > >> -       defined(CONFIG_PPC_T4160)
> > >> +       defined(CONFIG_PPC_T4160) || defined(CONFIG_PPC_T4080)
> > >>         if (cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION)
> > >>                 xfertyp |= XFERTYP_CMDTYP_ABORT;  #endif
> > >
> > >
> > > These sorts of chip-specific #ifdefs are very hard to maintain. It
> > > would be far better if there were a single define, like:
> > >
> > > #ifdef CONFIG_FSL_ESDHC_USES_ABORT_TO_STOP
> > >
> > > And then define that on any system that needs this code. With the
> > > current version, I have no idea why this code is needed. I have
> > > guesses, but in order to be sure, I'd have to check several reference
> > > manuals. With my suggestion, it is obvious to everyone why this code
> > > is here, and it gives a hint to those who are adding support to new
> > > chips.
> > >
> > > Now, I'm not saying you should use my suggestion precisely. Perhaps
> > > every version of the esdhc after some point uses this mechanism. Then
> > > you could use that information. And perhaps my naming doesn't reflect
> > > what is happening. My point is, you're going to have to do this again
> > > when you release LS232XB, and that seems like a poor use of your time.
> > > :)
> >
> > Yangbo,
> >
> > I agree with Andy on this. Please make the suggested change.
> 
> ... as some sort of Kconfig entry that's ideally selected rather than
> prmopted for when applicable.  Like it would be done in the kernel :)
> 
> --
> Tom
[Lu Yangbo-B47093] Hi Tom, I want to reconfirm whether your suggestion is same with Andy and York's.
Could I define it in include/configs/xxxx.h? Or use Kconfig?

Thanks.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [U-Boot] [v2] mmc: fsl_esdhc: fix mmc read/write error on T4080
  2016-01-18  7:18       ` Yangbo Lu
@ 2016-01-19 15:58         ` Tom Rini
  2016-01-21  9:22           ` Yangbo Lu
  0 siblings, 1 reply; 8+ messages in thread
From: Tom Rini @ 2016-01-19 15:58 UTC (permalink / raw)
  To: u-boot

On Mon, Jan 18, 2016 at 07:18:59AM +0000, Yangbo Lu wrote:
> > -----Original Message-----
> > From: Tom Rini [mailto:trini at konsulko.com]
> > Sent: Friday, January 15, 2016 2:09 AM
> > To: york sun
> > Cc: Andy Fleming; Yangbo Lu; U-Boot list
> > Subject: Re: [U-Boot] [v2] mmc: fsl_esdhc: fix mmc read/write error on
> > T4080
> > 
> > On Thu, Jan 14, 2016 at 05:51:32PM +0000, york sun wrote:
> > > On 01/08/2016 04:23 PM, Andy Fleming wrote:
> > > > On Thu, Jan 7, 2016 at 2:50 AM, Yangbo Lu <yangbo.lu@nxp.com> wrote:
> > > >> Fill the right command type when using CMD12 to stop data transfer.
> > > >>
> > > >> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > > >> ---
> > > >> Changes for v2:
> > > >>         - Removed fix for T4160 because other patch had done that
> > > >> ---
> > > >>  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 c5054d6..16fb01a 100644
> > > >> --- a/drivers/mmc/fsl_esdhc.c
> > > >> +++ b/drivers/mmc/fsl_esdhc.c
> > > >> @@ -107,7 +107,7 @@ static uint esdhc_xfertyp(struct mmc_cmd *cmd,
> > > >> struct mmc_data *data)
> > > >>
> > > >>  #if defined(CONFIG_MX53) || defined(CONFIG_PPC_T4240) || \
> > > >>         defined(CONFIG_LS102XA) || defined(CONFIG_FSL_LAYERSCAPE) ||
> > \
> > > >> -       defined(CONFIG_PPC_T4160)
> > > >> +       defined(CONFIG_PPC_T4160) || defined(CONFIG_PPC_T4080)
> > > >>         if (cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION)
> > > >>                 xfertyp |= XFERTYP_CMDTYP_ABORT;  #endif
> > > >
> > > >
> > > > These sorts of chip-specific #ifdefs are very hard to maintain. It
> > > > would be far better if there were a single define, like:
> > > >
> > > > #ifdef CONFIG_FSL_ESDHC_USES_ABORT_TO_STOP
> > > >
> > > > And then define that on any system that needs this code. With the
> > > > current version, I have no idea why this code is needed. I have
> > > > guesses, but in order to be sure, I'd have to check several reference
> > > > manuals. With my suggestion, it is obvious to everyone why this code
> > > > is here, and it gives a hint to those who are adding support to new
> > > > chips.
> > > >
> > > > Now, I'm not saying you should use my suggestion precisely. Perhaps
> > > > every version of the esdhc after some point uses this mechanism. Then
> > > > you could use that information. And perhaps my naming doesn't reflect
> > > > what is happening. My point is, you're going to have to do this again
> > > > when you release LS232XB, and that seems like a poor use of your time.
> > > > :)
> > >
> > > Yangbo,
> > >
> > > I agree with Andy on this. Please make the suggested change.
> > 
> > ... as some sort of Kconfig entry that's ideally selected rather than
> > prmopted for when applicable.  Like it would be done in the kernel :)
> > 
> > --
> > Tom
> [Lu Yangbo-B47093] Hi Tom, I want to reconfirm whether your suggestion is same with Andy and York's.
> Could I define it in include/configs/xxxx.h? Or use Kconfig?

I'm agreeing with what Andy/York say and noting that the right way to
fix this is not to add something to include/configs/ but to use Kconfig
and have the platforms select the symbol (rather than prompt for it),
similar to how flags like this would work in the Linux kernel.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160119/ddbcfadd/attachment.sig>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [U-Boot] [v2] mmc: fsl_esdhc: fix mmc read/write error on T4080
  2016-01-19 15:58         ` Tom Rini
@ 2016-01-21  9:22           ` Yangbo Lu
  0 siblings, 0 replies; 8+ messages in thread
From: Yangbo Lu @ 2016-01-21  9:22 UTC (permalink / raw)
  To: u-boot

> -----Original Message-----
> From: Tom Rini [mailto:trini at konsulko.com]
> Sent: Tuesday, January 19, 2016 11:59 PM
> To: Yangbo Lu
> Cc: york sun; Andy Fleming; U-Boot list
> Subject: Re: [U-Boot] [v2] mmc: fsl_esdhc: fix mmc read/write error on
> T4080
> 
> On Mon, Jan 18, 2016 at 07:18:59AM +0000, Yangbo Lu wrote:
> > > -----Original Message-----
> > > From: Tom Rini [mailto:trini at konsulko.com]
> > > Sent: Friday, January 15, 2016 2:09 AM
> > > To: york sun
> > > Cc: Andy Fleming; Yangbo Lu; U-Boot list
> > > Subject: Re: [U-Boot] [v2] mmc: fsl_esdhc: fix mmc read/write error
> > > on
> > > T4080
> > >
> > > On Thu, Jan 14, 2016 at 05:51:32PM +0000, york sun wrote:
> > > > On 01/08/2016 04:23 PM, Andy Fleming wrote:
> > > > > On Thu, Jan 7, 2016 at 2:50 AM, Yangbo Lu <yangbo.lu@nxp.com>
> wrote:
> > > > >> Fill the right command type when using CMD12 to stop data
> transfer.
> > > > >>
> > > > >> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > > > >> ---
> > > > >> Changes for v2:
> > > > >>         - Removed fix for T4160 because other patch had done
> > > > >> that
> > > > >> ---
> > > > >>  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 c5054d6..16fb01a 100644
> > > > >> --- a/drivers/mmc/fsl_esdhc.c
> > > > >> +++ b/drivers/mmc/fsl_esdhc.c
> > > > >> @@ -107,7 +107,7 @@ static uint esdhc_xfertyp(struct mmc_cmd
> > > > >> *cmd, struct mmc_data *data)
> > > > >>
> > > > >>  #if defined(CONFIG_MX53) || defined(CONFIG_PPC_T4240) || \
> > > > >>         defined(CONFIG_LS102XA) ||
> > > > >> defined(CONFIG_FSL_LAYERSCAPE) ||
> > > \
> > > > >> -       defined(CONFIG_PPC_T4160)
> > > > >> +       defined(CONFIG_PPC_T4160) || defined(CONFIG_PPC_T4080)
> > > > >>         if (cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION)
> > > > >>                 xfertyp |= XFERTYP_CMDTYP_ABORT;  #endif
> > > > >
> > > > >
> > > > > These sorts of chip-specific #ifdefs are very hard to maintain.
> > > > > It would be far better if there were a single define, like:
> > > > >
> > > > > #ifdef CONFIG_FSL_ESDHC_USES_ABORT_TO_STOP
> > > > >
> > > > > And then define that on any system that needs this code. With
> > > > > the current version, I have no idea why this code is needed. I
> > > > > have guesses, but in order to be sure, I'd have to check several
> > > > > reference manuals. With my suggestion, it is obvious to everyone
> > > > > why this code is here, and it gives a hint to those who are
> > > > > adding support to new chips.
> > > > >
> > > > > Now, I'm not saying you should use my suggestion precisely.
> > > > > Perhaps every version of the esdhc after some point uses this
> > > > > mechanism. Then you could use that information. And perhaps my
> > > > > naming doesn't reflect what is happening. My point is, you're
> > > > > going to have to do this again when you release LS232XB, and that
> seems like a poor use of your time.
> > > > > :)
> > > >
> > > > Yangbo,
> > > >
> > > > I agree with Andy on this. Please make the suggested change.
> > >
> > > ... as some sort of Kconfig entry that's ideally selected rather
> > > than prmopted for when applicable.  Like it would be done in the
> > > kernel :)
> > >
> > > --
> > > Tom
> > [Lu Yangbo-B47093] Hi Tom, I want to reconfirm whether your suggestion
> is same with Andy and York's.
> > Could I define it in include/configs/xxxx.h? Or use Kconfig?
> 
> I'm agreeing with what Andy/York say and noting that the right way to fix
> this is not to add something to include/configs/ but to use Kconfig and
> have the platforms select the symbol (rather than prompt for it), similar
> to how flags like this would work in the Linux kernel.
> 
> --
> Tom

[Lu Yangbo-B47093] Thank you. But I just find I need to remove all the #ifdef rather than add one.
Because the MMC_CMD_STOP_TRANSMISSION command must be set a XFERTYP_CMDTYP_ABORT command type according to SD spec.

A new version patch would be sent later.

Best regards,
Yangbo Lu

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-01-21  9:22 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-07  8:50 [U-Boot] [v2] mmc: fsl_esdhc: fix mmc read/write error on T4080 Yangbo Lu
2016-01-09  0:23 ` Andy Fleming
2016-01-14 17:51   ` york sun
2016-01-14 18:09     ` Tom Rini
2016-01-18  3:22       ` Yangbo Lu
2016-01-18  7:18       ` Yangbo Lu
2016-01-19 15:58         ` Tom Rini
2016-01-21  9:22           ` Yangbo Lu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.