[U-Boot] [v2] mmc: fsl_esdhc: fix mmc read/write error on T4080

[Yangbo Lu]

> -----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 at nxp.com> wrote:
> > >> Fill the right command type when using CMD12 to stop data transfer.
> > >>
> > >> Signed-off-by: Yangbo Lu <yangbo.lu at 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.
:)