| Message ID | 1425972946-9555-1-git-send-email-Peng.Fan@freescale.com |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
On Tuesday, March 10, 2015 at 08:35:46 AM, Peng Fan wrote: > Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces > error register offset. > > Change the "char reserved3[59]" to "char reserved3[56]". > > Signed-off-by: Peng Fan <Peng.Fan@freescale.com> This should probably be applied to 2015.04 . What are the symptoms of this bug please ? Thanks for spotting this! Best regards, Marek Vasut
Hi, Marek On 3/10/2015 9:45 PM, Marek Vasut wrote: > On Tuesday, March 10, 2015 at 08:35:46 AM, Peng Fan wrote: >> Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces >> error register offset. >> >> Change the "char reserved3[59]" to "char reserved3[56]". >> >> Signed-off-by: Peng Fan <Peng.Fan@freescale.com> > This should probably be applied to 2015.04 . > > What are the symptoms of this bug please ? I just found the reserved3 size is wrong, did not do test. From the driver, only the entry 'scr' of fsl_esdhc below reserved3 is used, so the offset of scr is wrong if using `char reserved3[59]` > > Thanks for spotting this! > > Best regards, > Marek Vasut Regards, Peng.
On Wednesday, March 11, 2015 at 01:58:37 AM, Peng Fan wrote: > Hi, Marek Hi! > On 3/10/2015 9:45 PM, Marek Vasut wrote: > > On Tuesday, March 10, 2015 at 08:35:46 AM, Peng Fan wrote: > >> Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces > >> error register offset. > >> > >> Change the "char reserved3[59]" to "char reserved3[56]". > >> > >> Signed-off-by: Peng Fan <Peng.Fan@freescale.com> > > > > This should probably be applied to 2015.04 . > > > > What are the symptoms of this bug please ? > > I just found the reserved3 size is wrong, did not do test. > From the driver, only the entry 'scr' of fsl_esdhc below reserved3 is > used, so the offset of scr is wrong if using `char reserved3[59]` Uh, is the patch tested at all on real hardware ? Best regards, Marek Vasut
Hi, Marek On 3/11/2015 10:03 AM, Marek Vasut wrote: > On Wednesday, March 11, 2015 at 01:58:37 AM, Peng Fan wrote: >> Hi, Marek > Hi! > >> On 3/10/2015 9:45 PM, Marek Vasut wrote: >>> On Tuesday, March 10, 2015 at 08:35:46 AM, Peng Fan wrote: >>>> Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces >>>> error register offset. >>>> >>>> Change the "char reserved3[59]" to "char reserved3[56]". >>>> >>>> Signed-off-by: Peng Fan <Peng.Fan@freescale.com> >>> This should probably be applied to 2015.04 . >>> >>> What are the symptoms of this bug please ? >> I just found the reserved3 size is wrong, did not do test. >> From the driver, only the entry 'scr' of fsl_esdhc below reserved3 is >> used, so the offset of scr is wrong if using `char reserved3[59]` > Uh, is the patch tested at all on real hardware ? Still not test on real hardware. From commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1, " uint adsaddr; /* ADMA system address register */ - char reserved2[160]; /* reserved */ + char reserved2[100]; /* reserved */ + uint vendorspec; /* Vendor Specific register */ + char reserved3[59]; /* reserved */ uint hostver; /* Host controller version register */ " It's clear that 160 bytes does not equal with (100 + 4 + 59)bytes. > > Best regards, > Marek Vasut Regards, Peng.
On Wednesday, March 11, 2015 at 03:17:00 AM, Peng Fan wrote: > Hi, Marek > > On 3/11/2015 10:03 AM, Marek Vasut wrote: > > On Wednesday, March 11, 2015 at 01:58:37 AM, Peng Fan wrote: > >> Hi, Marek > > > > Hi! > > > >> On 3/10/2015 9:45 PM, Marek Vasut wrote: > >>> On Tuesday, March 10, 2015 at 08:35:46 AM, Peng Fan wrote: > >>>> Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces > >>>> error register offset. > >>>> > >>>> Change the "char reserved3[59]" to "char reserved3[56]". > >>>> > >>>> Signed-off-by: Peng Fan <Peng.Fan@freescale.com> > >>> > >>> This should probably be applied to 2015.04 . > >>> > >>> What are the symptoms of this bug please ? > >> > >> I just found the reserved3 size is wrong, did not do test. > >> > >> From the driver, only the entry 'scr' of fsl_esdhc below reserved3 is > >> > >> used, so the offset of scr is wrong if using `char reserved3[59]` > > > > Uh, is the patch tested at all on real hardware ? > > Still not test on real hardware. From commit > f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1, > " > uint adsaddr; /* ADMA system address register */ > - char reserved2[160]; /* reserved */ > + char reserved2[100]; /* reserved */ > + uint vendorspec; /* Vendor Specific register */ > + char reserved3[59]; /* reserved */ > uint hostver; /* Host controller version register */ > " > It's clear that 160 bytes does not equal with (100 + 4 + 59)bytes. Hi! I agree with the change, I'm just not very impressed by patches that are completely untested. If you could do a quick boot on some machine before sending, that'd be nice. Best regards, Marek Vasut
On Tue, Mar 10, 2015 at 4:35 AM, Peng Fan <Peng.Fan@freescale.com> wrote: > Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces > error register offset. > > Change the "char reserved3[59]" to "char reserved3[56]". > > Signed-off-by: Peng Fan <Peng.Fan@freescale.com> On a imx6sl-warp: Tested-by: Fabio Estevam <fabio.estevam@freescale.com>
Hi Peng, > On Mar 11, 2015, at 04:17 , Peng Fan <Peng.Fan@freescale.com> wrote: > > Hi, Marek > > On 3/11/2015 10:03 AM, Marek Vasut wrote: >> On Wednesday, March 11, 2015 at 01:58:37 AM, Peng Fan wrote: >>> Hi, Marek >> Hi! >> >>> On 3/10/2015 9:45 PM, Marek Vasut wrote: >>>> On Tuesday, March 10, 2015 at 08:35:46 AM, Peng Fan wrote: >>>>> Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces >>>>> error register offset. >>>>> >>>>> Change the "char reserved3[59]" to "char reserved3[56]". >>>>> >>>>> Signed-off-by: Peng Fan <Peng.Fan@freescale.com> >>>> This should probably be applied to 2015.04 . >>>> >>>> What are the symptoms of this bug please ? >>> I just found the reserved3 size is wrong, did not do test. >>> From the driver, only the entry 'scr' of fsl_esdhc below reserved3 is >>> used, so the offset of scr is wrong if using `char reserved3[59]` >> Uh, is the patch tested at all on real hardware ? > Still not test on real hardware. From commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1, > " > uint adsaddr; /* ADMA system address register */ > - char reserved2[160]; /* reserved */ > + char reserved2[100]; /* reserved */ > + uint vendorspec; /* Vendor Specific register */ > + char reserved3[59]; /* reserved */ > uint hostver; /* Host controller version register */ > " > It's clear that 160 bytes does not equal with (100 + 4 + 59)bytes. >> >> Best regards, >> Marek Vasut > Regards, > Peng. Although I agree with fixing this, I’m kinda scared about how fragile structs for describing hardware registers are. But we’re stuck with it I guess. Regards — Pantelis
On Wed, 2015-03-11 at 15:55 +0200, Pantelis Antoniou wrote: > Hi Peng, > > > > On Mar 11, 2015, at 04:17 , Peng Fan <Peng.Fan@freescale.com> wrote: > > > > Hi, Marek > > > > On 3/11/2015 10:03 AM, Marek Vasut wrote: > > > On Wednesday, March 11, 2015 at 01:58:37 AM, Peng Fan wrote: > > > > Hi, Marek > > > Hi! > > > > > > > On 3/10/2015 9:45 PM, Marek Vasut wrote: > > > > > On Tuesday, March 10, 2015 at 08:35:46 AM, Peng Fan wrote: > > > > > > Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces > > > > > > error register offset. > > > > > > > > > > > > Change the "char reserved3[59]" to "char reserved3[56]". > > > > > > > > > > > > Signed-off-by: Peng Fan <Peng.Fan@freescale.com> > > > > > This should probably be applied to 2015.04 . > > > > > > > > > > What are the symptoms of this bug please ? > > > > I just found the reserved3 size is wrong, did not do test. > > > > From the driver, only the entry 'scr' of fsl_esdhc below reserved3 is > > > > used, so the offset of scr is wrong if using `char reserved3[59]` > > > Uh, is the patch tested at all on real hardware ? > > Still not test on real hardware. From commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1, > > " > > uint adsaddr; /* ADMA system address register */ > > - char reserved2[160]; /* reserved */ > > + char reserved2[100]; /* reserved */ > > + uint vendorspec; /* Vendor Specific register */ > > + char reserved3[59]; /* reserved */ > > uint hostver; /* Host controller version register */ > > " > > It's clear that 160 bytes does not equal with (100 + 4 + 59)bytes. > > > > > > Best regards, > > > Marek Vasut > > Regards, > > Peng. > > Although I agree with fixing this, I’m kinda scared about how fragile structs for describing hardware > registers are. > > But we’re stuck with it I guess. > Without this patch my emmc (T1042)is broken beyond repair, please commit.
On Wed, Mar 11, 2015 at 10:55 AM, Pantelis Antoniou <panto@antoniou-consulting.com> wrote: > Although I agree with fixing this, I’m kinda scared about how fragile > structs for describing hardware registers are. Agreed! > But we’re stuck with it I guess. Yes, it seems this is mandatory in U-boot. Kernel does not have such requirement and the standard way there is to use (base + offset) for register accesses. Regards, Fabio Estevam
On Wed, Mar 11, 2015 at 01:03:16PM -0300, Fabio Estevam wrote: > On Wed, Mar 11, 2015 at 10:55 AM, Pantelis Antoniou > <panto@antoniou-consulting.com> wrote: > > > Although I agree with fixing this, I’m kinda scared about how fragile > > structs for describing hardware registers are. > > Agreed! > > > But we’re stuck with it I guess. > > Yes, it seems this is mandatory in U-boot. > > Kernel does not have such requirement and the standard way there is to > use (base + offset) for register accesses. So this is one of those topics that long term, I'd like to change U-Boot for but it's both a giant change and something we need to do a lot of prep-work for still. The long ago argument for why U-Boot does things the way it does boils down to type checking. The kernel gets this I think with a combination of sparse and other preprocessor magic / checks. We'll also need to migrate once device model work is farther along and people want more seriously to look at splitting out a runs-many-places U-Boot from a "must be board-centric, pretty much" SPL.
Hi, On 3/11/2015 9:00 PM, Fabio Estevam wrote: > On Tue, Mar 10, 2015 at 4:35 AM, Peng Fan <Peng.Fan@freescale.com> wrote: >> Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces >> error register offset. >> >> Change the "char reserved3[59]" to "char reserved3[56]". >> >> Signed-off-by: Peng Fan <Peng.Fan@freescale.com> > On a imx6sl-warp: > > Tested-by: Fabio Estevam <fabio.estevam@freescale.com> Just kindly ask, will this patch be applied? Regards, Peng.
On Sunday, March 15, 2015 at 06:54:49 AM, Peng Fan wrote: > Hi, > > On 3/11/2015 9:00 PM, Fabio Estevam wrote: > > On Tue, Mar 10, 2015 at 4:35 AM, Peng Fan <Peng.Fan@freescale.com> wrote: > >> Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces > >> error register offset. > >> > >> Change the "char reserved3[59]" to "char reserved3[56]". > >> > >> Signed-off-by: Peng Fan <Peng.Fan@freescale.com> > > > > On a imx6sl-warp: > > > > Tested-by: Fabio Estevam <fabio.estevam@freescale.com> > > Just kindly ask, will this patch be applied? Tom ? Best regards, Marek Vasut
On Sun, 2015-03-15 at 13:54 +0800, Peng Fan wrote: > Hi, > > On 3/11/2015 9:00 PM, Fabio Estevam wrote: > > On Tue, Mar 10, 2015 at 4:35 AM, Peng Fan <Peng.Fan@freescale.com> wrote: > > > Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces > > > error register offset. > > > > > > Change the "char reserved3[59]" to "char reserved3[56]". > > > > > > Signed-off-by: Peng Fan <Peng.Fan@freescale.com> > > On a imx6sl-warp: > > > > Tested-by: Fabio Estevam <fabio.estevam@freescale.com> > Just kindly ask, will this patch be applied? Please do, my fsl emmc(T1042) is broken without this patch. Jocke
On Tue, Mar 10, 2015 at 03:35:46PM +0800, Peng Fan wrote: > Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces > error register offset. > > Change the "char reserved3[59]" to "char reserved3[56]". > > Signed-off-by: Peng Fan <Peng.Fan@freescale.com> > Tested-by: Fabio Estevam <fabio.estevam@freescale.com> Applied to u-boot/master, thanks!
diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c index c5e270d..db4d251 100644 --- a/drivers/mmc/fsl_esdhc.c +++ b/drivers/mmc/fsl_esdhc.c @@ -56,7 +56,7 @@ struct fsl_esdhc { uint adsaddr; /* ADMA system address register */ char reserved2[100]; /* reserved */ uint vendorspec; /* Vendor Specific register */ - char reserved3[59]; /* reserved */ + char reserved3[56]; /* reserved */ uint hostver; /* Host controller version register */ char reserved4[4]; /* reserved */ uint dmaerraddr; /* DMA error address register */
Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces error register offset. Change the "char reserved3[59]" to "char reserved3[56]". Signed-off-by: Peng Fan <Peng.Fan@freescale.com> --- drivers/mmc/fsl_esdhc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)