Skip to contentSkip to conversation

mmc: fsl_esdhc fix register offset

PATCH In review Peng Fan 6 participants 15 messages Mar 10, 2015 – Mar 17, 2015 MMC, SD and SDIO

lore
Share & tools
lore

Peng Fan

author
#0 lore raw

PATCH 1/1 mmc: fsl_esdhc fix register offset

Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces error register offset.

Change the "char reserved3[59]" to "char reserved3[56]".

Signed-off-by Peng Fan <[email protected]>
---
 drivers/mmc/fsl_esdhc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/mmc/fsl_esdhc.c +1 -1
index c5e270d..db4d251 100644
@@ -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 */
Signature · 1 line
1.8.4
#1 lore raw

PATCH 1/1 mmc: fsl_esdhc fix register offset

Quoted 6 lines · Peng Fan

Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces error register offset.

Change the "char reserved3[59]" to "char reserved3[56]".

Signed-off-by Peng Fan <[email protected]>

This should probably be applied to 2015.04 .

What are the symptoms of this bug please ?

Thanks for spotting this!

Best regards,
Marek Vasut

author
#2 lore raw

PATCH 1/1 mmc: fsl_esdhc fix register offset

Hi, Marek

Quoted 10 lines · Marek Vasut
Quoted 6 lines · Peng Fan

Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces error register offset.

Change the "char reserved3[59]" to "char reserved3[56]".

Signed-off-by Peng Fan <[email protected]>

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]

Quoted 5 lines

Thanks for spotting this!

Best regards,
Marek Vasut

Regards,
Peng.

#3 lore raw

PATCH 1/1 mmc: fsl_esdhc fix register offset

Quoted 1 line · Peng Fan

Hi, Marek

Hi!

Quoted 9 lines · Marek Vasut
Quoted 8 lines · Peng Fan
Quoted 6 lines

Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces error register offset.

Change the "char reserved3[59]" to "char reserved3[56]".

Signed-off-by Peng Fan <[email protected]>
Quoted 7 lines
Quoted 3 lines

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

author
#4 lore raw

PATCH 1/1 mmc: fsl_esdhc fix register offset

Hi, Marek

Quoted 11 lines · Marek Vasut
Quoted 1 line · Peng Fan

Hi, Marek

Hi!

Quoted 7 lines · Marek Vasut
Quoted 6 lines · Peng Fan
Quoted 5 lines

Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces error register offset.

Change the "char reserved3[59]" to "char reserved3[56]".

Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Peng Fan <[email protected]>

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.

Quoted 3 lines

Best regards,
Marek Vasut

Regards,
Peng.

#5 lore raw

PATCH 1/1 mmc: fsl_esdhc fix register offset

Quoted 38 lines · Peng Fan

Hi, Marek

Quoted 18 lines · Marek Vasut
Quoted 1 line · Peng Fan

Hi, Marek

Hi!

Quoted 13 lines · Marek Vasut
Quoted 11 lines · Peng Fan
Quoted 6 lines

Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces error register offset.

Change the "char reserved3[59]" to "char reserved3[56]".

Signed-off-by Peng Fan <[email protected]>

This should probably be applied to 2015.04 .

What are the symptoms of this bug please ?

Quoted 7 lines
Quoted 5 lines

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

(quoted diff) +3 -1
-       char    reserved2[160]; /* reserved */
+       char    reserved2[100]; /* reserved */
+       uint    vendorspec;     /* Vendor Specific register */
+       char    reserved3[59];  /* reserved */
         uint    hostver;        /* Host controller version register */
Quoted 2 lines

"
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

#6 lore raw

PATCH 1/1 mmc: fsl_esdhc fix register offset

In reply to Peng Fan

Hi Peng,

Quoted 35 lines · Peng Fan

Hi, Marek

Quoted 11 lines · Marek Vasut
Quoted 1 line · Peng Fan

Hi, Marek

Hi!

Quoted 7 lines · Marek Vasut
Quoted 6 lines · Peng Fan
Quoted 5 lines

Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces error register offset.

Change the "char reserved3[59]" to "char reserved3[56]".

Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Peng Fan <[email protected]>

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

(quoted diff) +3 -1
-       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.

Quoted 1 line
Quoted 4 lines
Quoted 2 lines

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

#7 lore raw

PATCH 1/1 mmc: fsl_esdhc fix register offset

Quoted 40 lines · Pantelis Antoniou

Hi Peng,

Quoted 37 lines · Peng Fan

Hi, Marek

Quoted 11 lines · Marek Vasut
Quoted 1 line · Peng Fan

Hi, Marek

Hi!

Quoted 7 lines · Marek Vasut
Quoted 6 lines · Peng Fan
Quoted 5 lines

Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces error register offset.

Change the "char reserved3[59]" to "char reserved3[56]".

Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Peng Fan <[email protected]>

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

(quoted diff) +3 -1
-       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.

Quoted 3 lines

Best regards,
Marek Vasut

Quoted 8 lines
Quoted 2 lines

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.

#8 lore raw

PATCH 1/1 mmc: fsl_esdhc fix register offset

In reply to Pantelis Antoniou
Quoted 2 lines · Pantelis Antoniou

Although I agree with fixing this, I?m kinda scared about how fragile structs for describing hardware registers are.

Agreed!

Quoted 1 line

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

#9 lore raw

PATCH 1/1 mmc: fsl_esdhc fix register offset

Quoted 6 lines · Fabio Estevam
Quoted 2 lines · Pantelis Antoniou

Although I agree with fixing this, I?m kinda scared about how fragile structs for describing hardware registers are.

Quoted 8 lines

Agreed!

Quoted 1 line

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.

Signature · 8 lines
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>
author
#11 lore raw

PATCH 1/1 mmc: fsl_esdhc fix register offset

Hi,

Quoted 10 lines · Fabio Estevam
Quoted 6 lines · Peng Fan

Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces error register offset.

Change the "char reserved3[59]" to "char reserved3[56]".

Signed-off-by Peng Fan <[email protected]>

On a imx6sl-warp:

Tested-by Fabio Estevam <[email protected]>

Just kindly ask, will this patch be applied?

Regards,
Peng.

#12 lore raw

PATCH 1/1 mmc: fsl_esdhc fix register offset

Quoted 9 lines · Peng Fan

Hi,

Quoted 6 lines · Fabio Estevam
Quoted 5 lines · Peng Fan

Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces error register offset.

Change the "char reserved3[59]" to "char reserved3[56]".

Quoted 7 lines
Quoted 5 lines
Quoted 1 line
Signed-off-by Peng Fan <[email protected]>

On a imx6sl-warp:

Tested-by Fabio Estevam <[email protected]>

Just kindly ask, will this patch be applied?

Tom ?

Best regards,
Marek Vasut

#13 lore raw

PATCH 1/1 mmc: fsl_esdhc fix register offset

In reply to Peng Fan
Quoted 7 lines · Peng Fan

Hi,

Quoted 4 lines · Fabio Estevam
Quoted 3 lines · Peng Fan

Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces error register offset.

Quoted 7 lines
Quoted 6 lines
Quoted 3 lines

Change the "char reserved3[59]" to "char reserved3[56]".

Signed-off-by Peng Fan <[email protected]>

On a imx6sl-warp:

Tested-by Fabio Estevam <[email protected]>

Just kindly ask, will this patch be applied?

Please do, my fsl emmc(T1042) is broken without this patch.

Jocke

#14 lore raw
In reply to Peng Fan
Quoted 7 lines · Peng Fan

Commit f022d36e8a4517b2a9d25ff2d75bd2459d0c68b1 introduces error register offset.

Change the "char reserved3[59]" to "char reserved3[56]".

Signed-off-by Peng Fan <[email protected]>
Tested-by Fabio Estevam <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
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>