On Mon, Jan 18, 2016 at 07:18:59AM +0000, Yangbo Lu wrote: > > -----Original Message----- > > From: Tom Rini [mailto:[email protected]] > > 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 <[email protected]> wrote: > > > >> Fill the right command type when using CMD12 to stop data transfer. > > > >> > > > >> Signed-off-by: Yangbo Lu <[email protected]> > > > >> --- > > > >> 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
signature.asc
Description: Digital signature
_______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
