[U-Boot] [PATCH] mmc: fsl_esdhc fix register offset

[U-Boot] [PATCH] mmc: fsl_esdhc fix register offset

[Peng Fan]

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(-)

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 */
-- 
1.8.4

[U-Boot] [PATCH] mmc: fsl_esdhc fix register offset

[Marek Vasut]

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

[U-Boot] [PATCH] mmc: fsl_esdhc fix register offset

[Peng Fan]

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.

[U-Boot] [PATCH] mmc: fsl_esdhc fix register offset

[Marek Vasut]

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

[U-Boot] [PATCH] mmc: fsl_esdhc fix register offset

[Peng Fan]

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.

[U-Boot] [PATCH] mmc: fsl_esdhc fix register offset

[Marek Vasut]

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

[U-Boot] [PATCH] mmc: fsl_esdhc fix register offset

[Fabio Estevam]

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>

[U-Boot] [PATCH] mmc: fsl_esdhc fix register offset

[Pantelis Antoniou]

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

[U-Boot] [PATCH] mmc: fsl_esdhc fix register offset

[Joakim Tjernlund]

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.

[U-Boot] [PATCH] mmc: fsl_esdhc fix register offset

[Fabio Estevam]

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

[U-Boot] [PATCH] mmc: fsl_esdhc fix register offset

[Tom Rini]

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.

-- 
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/20150311/7bfdda41/attachment.sig>

[U-Boot] [PATCH] mmc: fsl_esdhc fix register offset

[Peng Fan]

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.

[U-Boot] [PATCH] mmc: fsl_esdhc fix register offset

[Marek Vasut]

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

[U-Boot] [PATCH] mmc: fsl_esdhc fix register offset

[Joakim Tjernlund]

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

[U-Boot] mmc: fsl_esdhc fix register offset

[Tom Rini]

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!

-- 
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/20150317/73c173a2/attachment.sig>