ARM: OMAP3: Add NAND support
Subsystems (2)
PATCH v3 8/13 ARM: OMAP3: Add NAND support
Subject: [PATCH 08/13 v3] ARM: OMAP3: Add NAND support
From: Dirk Behme <[email protected]>
Add NAND support
---
Changes in version v3:
- Fix/update NAND driver and seperate it into an own patch as proposed by Scott Wood
drivers/mtd/nand/Makefile | 1 drivers/mtd/nand/omap3.c | 380 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 381 insertions(+)
drivers/mtd/nand/Makefile +1 -0
@@ -38,6 +38,7 @@ endif COBJS-$(CONFIG_NAND_FSL_ELBC) += fsl_elbc_nand.o COBJS-$(CONFIG_NAND_FSL_UPM) += fsl_upm.o COBJS-$(CONFIG_NAND_S3C64XX) += s3c64xx.o +COBJS-$(CONFIG_NAND_OMAP3) += omap3.o endif COBJS := $(COBJS-y)
drivers/mtd/nand/omap3.c new file +380 -0
380 lines not on this page · raw message
PATCH v3 8/13 ARM: OMAP3: Add NAND support
drivers/mtd/nand/omap3.c line 31
Quoted 2 lines · dirk.behme at googlemail.com
drivers/mtd/nand/omap3.c new file +2
-0+unsigned char cs; +volatile unsigned long gpmc_cs_base_add;
Make these static. gpmc_cs_base_add should be a pointer, not "unsigned long". Volatile isn't needed since you use I/O accessors, and definitely isn't needed on the address itself.
Quoted 15 lines
drivers/mtd/nand/omap3.c new file +15
-0+/* +* omap_nand_hwcontrol - Set the address pointers corretly for the +* following address/data/command operation +*/ +static void omap_nand_hwcontrol (struct mtd_info *mtd,int cmd, +unsigned int ctrl) +{ + registerstruct nand_chip *this = mtd->priv; + +/* Point the IO_ADDR to DATA and ADDRESS registers instead +of chip address */ +switch (ctrl) { +case NAND_CTRL_CHANGE |NAND_CTRL_CLE : + this->IO_ADDR_W = (void *) gpmc_cs_base_add +GPMC_NAND_CMD ; + this->IO_ADDR_R = (void *) gpmc_cs_base_add +GPMC_NAND_DAT ;
drivers/mtd/nand/omap3.c line 60
Quoted 10 lines
drivers/mtd/nand/omap3.c new file +10
-0+break ; +case NAND_CTRL_CHANGE |NAND_CTRL_ALE : + this->IO_ADDR_W = (void *) gpmc_cs_base_add +GPMC_NAND_ADR ; + this->IO_ADDR_R = (void *) gpmc_cs_base_add +GPMC_NAND_DAT ; +break ; +case NAND_CTRL_CHANGE |NAND_NCE : + this->IO_ADDR_W = (void *) gpmc_cs_base_add +GPMC_NAND_DAT ; + this->IO_ADDR_R = (void *) gpmc_cs_base_add +GPMC_NAND_DAT ; +break ; + }
IO_ADDR_R never seems to change; you can leave it out of here and omap_nand_wait.
Quoted 9 lines
drivers/mtd/nand/omap3.c new file +9
-0+/* +* omap_nand_wait - called primarily after a program/erase operation +* so that we access NAND again only after the device +* is ready again. +* @mtd: MTD device structure +* @chip: nand_chip structure +*/ +static int omap_nand_wait (struct mtd_info *mtd,struct nand_chip *chip) +{
drivers/mtd/nand/omap3.c line 84
Quoted 10 lines
drivers/mtd/nand/omap3.c new file +10
-0+ registerstruct nand_chip *this = mtd->priv; +int status =0 ; + + this->IO_ADDR_W = (void *) gpmc_cs_base_add +GPMC_NAND_CMD ; + this->IO_ADDR_R = (void *) gpmc_cs_base_add +GPMC_NAND_DAT ; +/* Send the status command and loop until the device is free */ +while (!(status &0x40 )) { +writeb (NAND_CMD_STATUS &0xFF , this->IO_ADDR_W); + status =readb (this->IO_ADDR_R); + }
Maybe should just do this, to avoid changing client-visible state:writeb(NAND_CMD_STATUS, &gpmc_cs_base_add[GPMC_NAND_CMD]);
No need for the "& 0xFF".
drivers/mtd/nand/omap3.c line 124
Quoted 5 lines
drivers/mtd/nand/omap3.c new file +5
-0+/* Init ECC Control Register */ +/* Clear all ECC | Enable Reg1 */ + val = ((0x00000001 <<8 ) |0x00000001 ); +writel (val,GPMC_BASE +GPMC_ECC_CONTROL ); +writel (0x3fcff000 ,GPMC_BASE +GPMC_ECC_SIZE_CONFIG );
Symbolic constants for the bit values would be nice.
Quoted 8 lines
+/*
+ *omap_calculate_ecc- Generate non-inverted ECC bytes.
+ *+ * Using noninverted ECC can be considered ugly since writing a blank + * page ie. padding will clear the ECC bytes. This is no problem as + * long nobody is trying to write data on the seemingly unused page. + * Reading an erased page will produce an ECC mismatch between + * generated and read ECC bytes that has to be dealt with separately.
Where is it dealt with separately?
Review
Quoted 1 line
(quoted diff) +1
-0+ unsigned long val = 0x0;
Unnecessary initialization.
drivers/mtd/nand/omap3.c line 214
Quoted 5 lines
drivers/mtd/nand/omap3.c new file +5
-0+unsigned long reg; + +/* Start Reading from HW ECC1_Result = 0x200 */ + reg = (unsigned long ) (GPMC_BASE +GPMC_ECC1_RESULT ); + val =readl (reg);
readl() takes a pointer. ARM gets away without a warning here because it uses macros rather than inline functions, but it's bad practice.
drivers/mtd/nand/omap3.c line 223
Quoted 4 lines
drivers/mtd/nand/omap3.c new file +4
-0+/* Stop reading anymore ECC vals and clear old results +* enable will be called if more reads are required */ + reg = (unsigned long ) (GPMC_BASE +GPMC_ECC_CONFIG ); +writel (0x000 , reg);
Likewise.
Quoted 23 lines
drivers/mtd/nand/omap3.c new file +23
-0+void omap_nand_switch_ecc (int hardware) +{ +struct nand_chip *nand; + +if (nand_curr_device <0 || + nand_curr_device >=CONFIG_SYS_MAX_NAND_DEVICE || + !nand_info[nand_curr_device].name) { +printf ("Error: Can't switch ecc, no devices available\n" ); +return ; + } + + nand = (&nand_info[nand_curr_device])->priv; + +if (!hardware) { + nand->ecc.mode =NAND_ECC_SOFT ; + nand->ecc.layout = &sw_nand_oob_64; + nand->ecc.size =256 ;/* set default eccsize */ + nand->ecc.bytes =3 ; + nand->ecc.steps =8 ; + nand->ecc.hwctl =0 ; + nand->ecc.calculate = nand_calculate_ecc; + nand->ecc.correct = nand_correct_data; + }else {
drivers/mtd/nand/omap3.c line 309
Quoted 10 lines
drivers/mtd/nand/omap3.c new file +10
-0+ nand->ecc.mode =NAND_ECC_HW ; + nand->ecc.layout = &hw_nand_oob_64; + nand->ecc.size =512 ; + nand->ecc.bytes =3 ; + nand->ecc.steps =4 ; + nand->ecc.hwctl = omap_enable_hwecc; + nand->ecc.correct = omap_correct_data; + nand->ecc.calculate = omap_calculate_ecc; +omap_hwecc_init (nand); + }
Do you need to do anything similar to omap_hwecc_init() when switching to SW ECC to tell the hardware to stop doing ECC?
-Scott
PATCH v3 8/13 ARM: OMAP3: Add NAND support
Quoted 9 lines · Scott Wood
drivers/mtd/nand/omap3.c line 31
Quoted 2 lines · dirk.behme at googlemail.com
drivers/mtd/nand/omap3.c new file +2
-0+unsigned char cs; +volatile unsigned long gpmc_cs_base_add;Make these static.
gpmc_cs_base_addshould be a pointer, not "unsigned long". Volatile isn't needed since you use I/O accessors, and definitely isn't needed on the address itself.
Changed in attachment.
Quoted 21 lines
Quoted 21 lines
drivers/mtd/nand/omap3.c new file +21
-0+/* +* omap_nand_hwcontrol - Set the address pointers corretly for the +* following address/data/command operation +*/ +static void omap_nand_hwcontrol (struct mtd_info *mtd,int cmd, +unsigned int ctrl) +{ + registerstruct nand_chip *this = mtd->priv; + +/* Point the IO_ADDR to DATA and ADDRESS registers instead +of chip address */ +switch (ctrl) { +case NAND_CTRL_CHANGE |NAND_CTRL_CLE : + this->IO_ADDR_W = (void *) gpmc_cs_base_add +GPMC_NAND_CMD ; + this->IO_ADDR_R = (void *) gpmc_cs_base_add +GPMC_NAND_DAT ; +break ; +case NAND_CTRL_CHANGE |NAND_CTRL_ALE : + this->IO_ADDR_W = (void *) gpmc_cs_base_add +GPMC_NAND_ADR ; + this->IO_ADDR_R = (void *) gpmc_cs_base_add +GPMC_NAND_DAT ; +break ; +case NAND_CTRL_CHANGE |NAND_NCE :
Quoted 8 lines
drivers/mtd/nand/omap3.c line 60
Quoted 4 lines
drivers/mtd/nand/omap3.c new file +4
-0+ this->IO_ADDR_W = (void *) gpmc_cs_base_add +GPMC_NAND_DAT ; + this->IO_ADDR_R = (void *) gpmc_cs_base_add +GPMC_NAND_DAT ; +break ; + }
IO_ADDR_Rnever seems to change; you can leave it out of here andomap_nand_wait.
Yes, thanks. Removed in attachment.
Quoted 24 lines
Quoted 9 lines
drivers/mtd/nand/omap3.c new file +9
-0+/* +* omap_nand_wait - called primarily after a program/erase operation +* so that we access NAND again only after the device +* is ready again. +* @mtd: MTD device structure +* @chip: nand_chip structure +*/ +static int omap_nand_wait (struct mtd_info *mtd,struct nand_chip *chip) +{drivers/mtd/nand/omap3.c line 84
Quoted 10 lines
drivers/mtd/nand/omap3.c new file +10
-0+ registerstruct nand_chip *this = mtd->priv; +int status =0 ; + + this->IO_ADDR_W = (void *) gpmc_cs_base_add +GPMC_NAND_CMD ; + this->IO_ADDR_R = (void *) gpmc_cs_base_add +GPMC_NAND_DAT ; +/* Send the status command and loop until the device is free */ +while (!(status &0x40 )) { +writeb (NAND_CMD_STATUS &0xFF , this->IO_ADDR_W); + status =readb (this->IO_ADDR_R); + }Maybe should just do this, to avoid changing client-visible state:
writeb(NAND_CMD_STATUS, &gpmc_cs_base_add[GPMC_NAND_CMD]);
Quoted 1 line
No need for the "&
0xFF".
Modified.
Quoted 8 lines
drivers/mtd/nand/omap3.c line 124
Quoted 5 lines
drivers/mtd/nand/omap3.c new file +5
-0+/* Init ECC Control Register */ +/* Clear all ECC | Enable Reg1 */ + val = ((0x00000001 <<8 ) |0x00000001 ); +writel (val,GPMC_BASE +GPMC_ECC_CONTROL ); +writel (0x3fcff000 ,GPMC_BASE +GPMC_ECC_SIZE_CONFIG );Symbolic constants for the bit values would be nice.
Added. Hope you like them ;)
Quoted 11 lines
Quoted 8 lines
+/*
+ *omap_calculate_ecc- Generate non-inverted ECC bytes.
+ *+ * Using noninverted ECC can be considered ugly since writing a blank + * page ie. padding will clear the ECC bytes. This is no problem as + * long nobody is trying to write data on the seemingly unused page. + * Reading an erased page will produce an ECC mismatch between + * generated and read ECC bytes that has to be dealt with separately.Where is it dealt with separately?
We already talked about this and extended the comment. To my understanding this special handling can't be done in omap_calculate_ecc() as it is called from generic NAND code and doesn't know if ECC it calculates is correct or not?
Do you have any proposals where and how to handle this?
Any propsals from OMAP experts?
Easiest short term solution would be to add a "FIXME" to comment?
Quoted 4 lines
Quoted 1 line
+ unsigned long val =
0x0;Unnecessary initialization.
Removed.
Quoted 9 lines
drivers/mtd/nand/omap3.c line 214
Quoted 5 lines
drivers/mtd/nand/omap3.c new file +5
-0+unsigned long reg; + +/* Start Reading from HW ECC1_Result = 0x200 */ + reg = (unsigned long ) (GPMC_BASE +GPMC_ECC1_RESULT ); + val =readl (reg);
readl()takes a pointer. ARM gets away without a warning here because it uses macros rather than inline functions, but it's bad practice.
Helper variable removed.
Quoted 7 lines
drivers/mtd/nand/omap3.c line 223
Quoted 4 lines
drivers/mtd/nand/omap3.c new file +4
-0+/* Stop reading anymore ECC vals and clear old results +* enable will be called if more reads are required */ + reg = (unsigned long ) (GPMC_BASE +GPMC_ECC_CONFIG ); +writel (0x000 , reg);Likewise.
Ditto.
Quoted 28 lines
Quoted 28 lines
drivers/mtd/nand/omap3.c new file +28
-0+void omap_nand_switch_ecc (int hardware) +{ +struct nand_chip *nand; + +if (nand_curr_device <0 || + nand_curr_device >=CONFIG_SYS_MAX_NAND_DEVICE || + !nand_info[nand_curr_device].name) { +printf ("Error: Can't switch ecc, no devices available\n" ); +return ; + } + + nand = (&nand_info[nand_curr_device])->priv; + +if (!hardware) { + nand->ecc.mode =NAND_ECC_SOFT ; + nand->ecc.layout = &sw_nand_oob_64; + nand->ecc.size =256 ;/* set default eccsize */ + nand->ecc.bytes =3 ; + nand->ecc.steps =8 ; + nand->ecc.hwctl =0 ; + nand->ecc.calculate = nand_calculate_ecc; + nand->ecc.correct = nand_correct_data; + }else { + nand->ecc.mode =NAND_ECC_HW ; + nand->ecc.layout = &hw_nand_oob_64; + nand->ecc.size =512 ; + nand->ecc.bytes =3 ; + nand->ecc.steps =4 ;
Quoted 8 lines
drivers/mtd/nand/omap3.c line 309
Quoted 5 lines
drivers/mtd/nand/omap3.c new file +5
-0+ nand->ecc.hwctl = omap_enable_hwecc; + nand->ecc.correct = omap_correct_data; + nand->ecc.calculate = omap_calculate_ecc; +omap_hwecc_init (nand); + }Do you need to do anything similar to
omap_hwecc_init()when switching to SW ECC to tell the hardware to stop doing ECC?
I don't think so, but maybe the experts can help here.
Comment in omap_calculate_ecc() tells us:
/* Stop reading anymore ECC vals and clear old result, enable will be called if more reads are required */
Sounds like an "auto stop and re-init if needed" (in omap_enable_hwecc()).
To summarize: If you agree with changes in attachment, last open point is the "ECC mismatch" issue. Do you agree?
Thanks for re-review!
Dirk
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 08_omap3_nand.txt
Url: http://lists.denx.de/pipermail/u-boot/attachments/20081010/2da0dbef/attachment-0001.txt
PATCH v3 8/13 ARM: OMAP3: Add NAND support
Dear Dirk Behme,
Quoted 2 lines · In message <[email protected]> you wrote:
Changed in attachment.
That doesn't work. Please post patches in-line, and using a matching
Subject: line.
Best regards,
Wolfgang Denk
Signature · 5 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de Uncertain fortune is thoroughly mastered by the equity of the calcu- lation. - Blaise Pascal
PATCH v3 8/13 ARM: OMAP3: Add NAND support
Quoted 8 lines · Wolfgang Denk
Dear Dirk Behme,
Quoted 1 line · In message <[email protected]> you wrote:
Changed in attachment.
That doesn't work. Please post patches in-line, and using a matching
I know, I know ;)
This is preview/RFC only while we are discussing about the final version. Sorry, forgot to mention that. Final patches for inclusion will be in-line again, as with v1 & v2 & v3 already done ;)
Dirk
PATCH v3 8/13 ARM: OMAP3: Add NAND support
In reply to Dirk BehmeQuoted 10 lines · Dirk Behme
Quoted 10 lines
Quoted 8 lines
+/*
+ *omap_calculate_ecc- Generate non-inverted ECC bytes.
+ *+ * Using noninverted ECC can be considered ugly since writing a blank + * page ie. padding will clear the ECC bytes. This is no problem as + * long nobody is trying to write data on the seemingly unused page. + * Reading an erased page will produce an ECC mismatch between + * generated and read ECC bytes that has to be dealt with separately.
Quoted 8 lines
Quoted 1 line
Where is it dealt with separately?
We already talked about this and extended the comment. To my understanding this special handling can't be done in
omap_calculate_ecc()as it is called from generic NAND code and doesn't know if ECC it calculates is correct or not?Do you have any proposals where and how to handle this?
Perhaps it could be done in omap_correct_data()? If you get a calc_ecc that corresponds to a blank page, treat a read_ecc of all-bits-set as correct.
Quoted 2 lines
To summarize: If you agree with changes in attachment, last open point is the "ECC mismatch" issue. Do you agree?
It's looking decent. I have some concerns about the ECC switching, though.
Review
Quoted 2 lines
(quoted diff) +2
-0+static unsigned char cs; +static void __iomem *gpmc_cs_base_add;
I'd prefer an actual data type rather than void, but I won't hold it up for that.
drivers/mtd/nand/omap3.c line 125
Quoted 8 lines
drivers/mtd/nand/omap3.c new file +8
-0+static void omap_hwecc_init (struct nand_chip *chip) +{ +/* Init ECC Control Register */ +/* Clear all ECC | Enable Reg1 */ +writel (ECCCLEAR |ECCRESULTREG1 ,GPMC_BASE +GPMC_ECC_CONTROL ); +writel (ECCSIZE1 |ECCSIZE0 |ECCSIZE0SEL , +GPMC_BASE +GPMC_ECC_SIZE_CONFIG ); +}
Is GPMC_BASE an integer or a pointer?
Quoted 11 lines
drivers/mtd/nand/omap3.c new file +11
-0+if (!hardware) { + nand->ecc.mode =NAND_ECC_SOFT ; + nand->ecc.layout = &sw_nand_oob_64; + nand->ecc.size =256 ;/* set default eccsize */ + nand->ecc.bytes =3 ; + nand->ecc.steps =8 ; + nand->ecc.hwctl =0 ; + nand->ecc.calculate = nand_calculate_ecc; + nand->ecc.correct = nand_correct_data; + }else { + nand->ecc.mode =NAND_ECC_HW ;
drivers/mtd/nand/omap3.c line 310
Quoted 10 lines
drivers/mtd/nand/omap3.c new file +10
-0+ nand->ecc.layout = &hw_nand_oob_64; + nand->ecc.size =512 ; + nand->ecc.bytes =3 ; + nand->ecc.steps =4 ; + nand->ecc.hwctl = omap_enable_hwecc; + nand->ecc.correct = omap_correct_data; + nand->ecc.calculate = omap_calculate_ecc; +omap_hwecc_init (nand); + } +}
Make sure that anything that the generic layer calculates that would change is updated (e.g. oobavail, read_page, write_page, read_oob, write_oob).
-Scott
PATCH v3 8/13 ARM: OMAP3: Add NAND support
Quoted 19 lines · Scott Wood
Quoted 17 lines · Dirk Behme
Quoted 11 lines
Quoted 8 lines
+/*
+ *omap_calculate_ecc- Generate non-inverted ECC bytes.
+ *+ * Using noninverted ECC can be considered ugly since writing a blank + * page ie. padding will clear the ECC bytes. This is no problem as + * long nobody is trying to write data on the seemingly unused page. + * Reading an erased page will produce an ECC mismatch between + * generated and read ECC bytes that has to be dealt with separately.Where is it dealt with separately?
We already talked about this and extended the comment. To my understanding this special handling can't be done in
omap_calculate_ecc()as it is called from generic NAND code and doesn't know if ECC it calculates is correct or not?
Quoted 6 lines
Quoted 1 line
Do you have any proposals where and how to handle this?
Perhaps it could be done in
omap_correct_data()? If you get acalc_eccthat corresponds to a blank page, treat aread_eccof all-bits-set as correct.
Thanks for this idea (you really help a lot)! Proposal in attachment (omap_correct_data()).
Quoted 6 lines
Quoted 2 lines
To summarize: If you agree with changes in attachment, last open point is the "ECC mismatch" issue. Do you agree?
It's looking decent. I have some concerns about the ECC switching, though.
Yes, I know, agreed. But changing existing kernels isn't easy, too.
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+static unsigned char cs; +static void __iomem *gpmc_cs_base_add;I'd prefer an actual data type rather than void, but I won't hold it up for that.
I took the data type of IO_ADDR_W & IO_ADDR_R where gpmc_cs_base_add is assigned to.
void __iomem *IO_ADDR_W;
Quoted 11 lines
drivers/mtd/nand/omap3.c line 125
Quoted 8 lines
drivers/mtd/nand/omap3.c new file +8
-0+static void omap_hwecc_init (struct nand_chip *chip) +{ +/* Init ECC Control Register */ +/* Clear all ECC | Enable Reg1 */ +writel (ECCCLEAR |ECCRESULTREG1 ,GPMC_BASE +GPMC_ECC_CONTROL ); +writel (ECCSIZE1 |ECCSIZE0 |ECCSIZE0SEL , +GPMC_BASE +GPMC_ECC_SIZE_CONFIG ); +}Is
GPMC_BASEan integer or a pointer?
Nothing. A macro:
#define OMAP34XX_GPMC_BASE (0x6E000000)
#define GPMC_BASE (OMAP34XX_GPMC_BASE)
It's then casted to volatile pointer by ARM's readx/writex.
Quoted 18 lines
Quoted 18 lines
drivers/mtd/nand/omap3.c new file +18
-0+if (!hardware) { + nand->ecc.mode =NAND_ECC_SOFT ; + nand->ecc.layout = &sw_nand_oob_64; + nand->ecc.size =256 ;/* set default eccsize */ + nand->ecc.bytes =3 ; + nand->ecc.steps =8 ; + nand->ecc.hwctl =0 ; + nand->ecc.calculate = nand_calculate_ecc; + nand->ecc.correct = nand_correct_data; + }else { + nand->ecc.mode =NAND_ECC_HW ; + nand->ecc.layout = &hw_nand_oob_64; + nand->ecc.size =512 ; + nand->ecc.bytes =3 ; + nand->ecc.steps =4 ; + nand->ecc.hwctl = omap_enable_hwecc; + nand->ecc.correct = omap_correct_data; + nand->ecc.calculate = omap_calculate_ecc;
Quoted 8 lines
drivers/mtd/nand/omap3.c line 310
Quoted 3 lines
drivers/mtd/nand/omap3.c new file +3
-0+omap_hwecc_init (nand); + } +}Make sure that anything that the generic layer calculates that would change is updated (e.g. oobavail,
read_page,write_page,read_oob,write_oob).
What about calling nand_scan_tail() for this? Proposal in attachment (omap_nand_switch_ecc()).
Many thanks for your help, reviews and patience,
Dirk
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 08_omap3_nand.txt
Url: http://lists.denx.de/pipermail/u-boot/attachments/20081011/9374d331/attachment.txt
PATCH v3 8/13 ARM: OMAP3: Add NAND support
Quoted 4 lines · Dirk Behme
Quoted 2 lines
It's looking decent. I have some concerns about the ECC switching, though.
Yes, I know, agreed. But changing existing kernels isn't easy, too.
I meant the implementation (specifically, the re-init of things done by nand_scan_tail()).
Quoted 11 lines
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+static unsigned char cs; +static void __iomem *gpmc_cs_base_add;I'd prefer an actual data type rather than void, but I won't hold it up for that.
I took the data type of
IO_ADDR_W&IO_ADDR_Rwheregpmc_cs_base_addis assigned to.void
__iomem*IO_ADDR_W;
But the actual data is of a certain size, not void.
Quoted 6 lines
Quoted 1 line
Is
GPMC_BASEan integer or a pointer?Nothing. A macro:
#define
OMAP34XX_GPMC_BASE(0x6E000000)
#defineGPMC_BASE(OMAP34XX_GPMC_BASE)
So it's an integer.
Quoted 1 line
It's then casted to volatile pointer by ARM's readx/writex.
The cast should be done by the driver, or you'll get warnings if readx/writex ever become inline functions (as they are on other arches).
Quoted 6 lines
Quoted 3 lines
Make sure that anything that the generic layer calculates that would change is updated (e.g. oobavail,
read_page,write_page,read_oob,write_oob).What about calling
nand_scan_tail()for this? Proposal in attachment (omap_nand_switch_ecc()).
You'll leak chip->buffers. Better to factor nand_scan_tail() into two functions, one which allocates memory and any other non-repeatable init, and another which does init based on ECC type. The former function would call the latter.
drivers/mtd/nand/omap3.c line 43
Quoted 4 lines
drivers/mtd/nand/omap3.c new file +4
-0+static void omap_nand_hwcontrol (struct mtd_info *mtd,int cmd, +unsigned int ctrl) +{ + registerstruct nand_chip *this = mtd->priv;
The "register" keyword is superfluous when not specifying a specific register (such as for use with inline assembly).
Review
Quoted 6 lines
(quoted diff) +6
-0+ /* Check if calc_ecc corresponds to a blank page. + * If so, treat read_ecc as correct. See comment at omap_calculate_ecc. + */ + if((*(unsigned int *)calc_ecc == 0x00000000) && + (*(unsigned int *)read_ecc == 0xFFFFFFFF)) + return 0;
This assumes 32-bit alignment of the ECC -- and aren't there four steps of 3 bytes each?
Also, not being overly familiar with ECC, is it possible for one or two bit flips to cause the computed ECC to go from all-ones to all-zeroes? If it is, then we may want to follow this up by checking the data.
-Scott