> -----Original Message----- > From: Tom Rini [mailto:tr...@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:tr...@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...@nxp.com> > wrote: > > > > >> Fill the right command type when using CMD12 to stop data > transfer. > > > > >> > > > > >> Signed-off-by: Yangbo Lu <yangbo...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot