* [PATCH v2 01/53] mtd: nand: allow to set only one of ECC size and ECC strength from DT
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
@ 2017-03-22 14:07 ` Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 02/53] mtd: nand: use read_oob() instead of cmdfunc() for bad block check Masahiro Yamada
` (22 subsequent siblings)
23 siblings, 0 replies; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-22 14:07 UTC (permalink / raw)
To: linux-mtd
Cc: laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Boris Brezillon,
Marek Vasut, Brian Norris, Graham Moore, David Woodhouse,
Masami Hiramatsu, Chuanxiao Dong, Jassi Brar, Masahiro Yamada,
linux-kernel, Richard Weinberger, Cyrille Pitchen
Currently, it is valid to specify both "nand-ecc-step-size" and
"nand-ecc-strength", but not allowed to set only one of them.
This requirement has a conflict with "nand-ecc-maximize"; this flag
is used when you want the driver to choose the best ECC strength.
If "nand-ecc-maximize" is set, "nand-ecc-strength" is very likely to
be unset.
It would be possible to make the if-conditional more complex by
adding the check for the NAND_ECC_MAXIMIZE flag, but I chose to drop
the check entirely. I thought of the situation where the hardware
has a fixed ECC step size (so it can be hard-coded in the driver),
whereas the ECC strength is configurable by software. In that case,
we may want to only set "nand-ecc-strength" (or "nand-ecc-maximize")
in DT.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
The Denali NAND is the case.
The ecc.size is fixed when the RTL is delivered, while the
driver can choose ecc.strength from some supported values.
For Intel and Altera, available ecc.strength are 8, 15.
For Socionext UniPhier, available ecc.strength are 8, 16, 24.
Changes in v2: None
drivers/mtd/nand/nand_base.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index b0524f8..a3c0f47 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -4333,12 +4333,6 @@ static int nand_dt_init(struct nand_chip *chip)
ecc_strength = of_get_nand_ecc_strength(dn);
ecc_step = of_get_nand_ecc_step_size(dn);
- if ((ecc_step >= 0 && !(ecc_strength >= 0)) ||
- (!(ecc_step >= 0) && ecc_strength >= 0)) {
- pr_err("must set both strength and step size in DT\n");
- return -EINVAL;
- }
-
if (ecc_mode >= 0)
chip->ecc.mode = ecc_mode;
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [PATCH v2 02/53] mtd: nand: use read_oob() instead of cmdfunc() for bad block check
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 01/53] mtd: nand: allow to set only one of ECC size and ECC strength from DT Masahiro Yamada
@ 2017-03-22 14:07 ` Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 03/53] mtd: nand: denali: remove unused CONFIG option and macros Masahiro Yamada
` (21 subsequent siblings)
23 siblings, 0 replies; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-22 14:07 UTC (permalink / raw)
To: linux-mtd
Cc: laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Boris Brezillon,
Marek Vasut, Brian Norris, Graham Moore, David Woodhouse,
Masami Hiramatsu, Chuanxiao Dong, Jassi Brar, Masahiro Yamada,
linux-kernel, Richard Weinberger, Cyrille Pitchen
The nand_default_block_markbad() and scan_block_fast() use high
level APIs to get access to the BBM.
On the other hand, nand_block_bad (the default implementation of
->block_bad) calls the lower level ->cmdfunc hook. This prevents
drivers from using ->ecc.read_oob() even if optimized read operation
is implemented. Besides, some NAND controllers may protect the BBM
with ECC.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2:
- Error out immediately if the first page access fails
- Use for() {} instead of do {} while()
- Update git-log
drivers/mtd/nand/nand_base.c | 34 +++++++++++++---------------------
1 file changed, 13 insertions(+), 21 deletions(-)
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index a3c0f47..807398d 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -354,40 +354,32 @@ static void nand_read_buf16(struct mtd_info *mtd, uint8_t *buf, int len)
*/
static int nand_block_bad(struct mtd_info *mtd, loff_t ofs)
{
- int page, res = 0, i = 0;
+ int page, page_end, res;
struct nand_chip *chip = mtd_to_nand(mtd);
- u16 bad;
+ u8 bad;
if (chip->bbt_options & NAND_BBT_SCANLASTPAGE)
ofs += mtd->erasesize - mtd->writesize;
page = (int)(ofs >> chip->page_shift) & chip->pagemask;
+ page_end = page + (chip->bbt_options & NAND_BBT_SCAN2NDPAGE ? 2 : 1);
- do {
- if (chip->options & NAND_BUSWIDTH_16) {
- chip->cmdfunc(mtd, NAND_CMD_READOOB,
- chip->badblockpos & 0xFE, page);
- bad = cpu_to_le16(chip->read_word(mtd));
- if (chip->badblockpos & 0x1)
- bad >>= 8;
- else
- bad &= 0xFF;
- } else {
- chip->cmdfunc(mtd, NAND_CMD_READOOB, chip->badblockpos,
- page);
- bad = chip->read_byte(mtd);
- }
+ for (; page < page_end; page++) {
+ res = chip->ecc.read_oob(mtd, chip, page);
+ if (res)
+ return res;
+
+ bad = chip->oob_poi[chip->badblockpos];
if (likely(chip->badblockbits == 8))
res = bad != 0xFF;
else
res = hweight8(bad) < chip->badblockbits;
- ofs += mtd->writesize;
- page = (int)(ofs >> chip->page_shift) & chip->pagemask;
- i++;
- } while (!res && i < 2 && (chip->bbt_options & NAND_BBT_SCAN2NDPAGE));
+ if (res)
+ return res;
+ }
- return res;
+ return 0;
}
/**
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [PATCH v2 03/53] mtd: nand: denali: remove unused CONFIG option and macros
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 01/53] mtd: nand: allow to set only one of ECC size and ECC strength from DT Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 02/53] mtd: nand: use read_oob() instead of cmdfunc() for bad block check Masahiro Yamada
@ 2017-03-22 14:07 ` Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 04/53] mtd: nand: denali: remove redundant define of BANK(x) Masahiro Yamada
` (20 subsequent siblings)
23 siblings, 0 replies; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-22 14:07 UTC (permalink / raw)
To: linux-mtd
Cc: laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Boris Brezillon,
Marek Vasut, Brian Norris, Graham Moore, David Woodhouse,
Masami Hiramatsu, Chuanxiao Dong, Jassi Brar, Masahiro Yamada,
linux-kernel, Richard Weinberger, Cyrille Pitchen
All of these macros are not used at all.
CONFIG_MTD_NAND_DENALI_SCRATCH_REG_ADDR is not used for anything but
defining SCRATCH_REG_ADDR. The config option should go away as well.
I am removing some register macros. They are not used, and do not
exist in recent IP versions.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2:
- Remove more unused macros
drivers/mtd/nand/Kconfig | 11 ------
drivers/mtd/nand/denali.c | 5 ---
drivers/mtd/nand/denali.h | 99 -----------------------------------------------
3 files changed, 115 deletions(-)
diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 6d4d567..817558b 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -60,17 +60,6 @@ config MTD_NAND_DENALI_DT
Enable the driver for NAND flash on platforms using a Denali NAND
controller as a DT device.
-config MTD_NAND_DENALI_SCRATCH_REG_ADDR
- hex "Denali NAND size scratch register address"
- default "0xFF108018"
- depends on MTD_NAND_DENALI_PCI
- help
- Some platforms place the NAND chip size in a scratch register
- because (some versions of) the driver aren't able to automatically
- determine the size of certain chips. Set the address of the
- scratch register here to enable this feature. On Intel Moorestown
- boards, the scratch register is at 0xFF108018.
-
config MTD_NAND_GPIO
tristate "GPIO assisted NAND Flash driver"
depends on GPIOLIB || COMPILE_TEST
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 73b9d4e..f993e13 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -91,11 +91,6 @@ static inline struct denali_nand_info *mtd_to_denali(struct mtd_info *mtd)
#define DENALI_READ 0
#define DENALI_WRITE 0x100
-/* types of device accesses. We can issue commands and get status */
-#define COMMAND_CYCLE 0
-#define ADDR_CYCLE 1
-#define STATUS_CYCLE 2
-
/*
* this is a helper macro that allows us to
* format the bank into the proper bits for the controller
diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h
index ea22191..c4f3a68 100644
--- a/drivers/mtd/nand/denali.h
+++ b/drivers/mtd/nand/denali.h
@@ -257,26 +257,6 @@
#define ERR_PAGE_ADDR(__bank) (0x440 + ((__bank) * 0x50))
#define ERR_BLOCK_ADDR(__bank) (0x450 + ((__bank) * 0x50))
-#define DATA_INTR 0x550
-#define DATA_INTR__WRITE_SPACE_AV 0x0001
-#define DATA_INTR__READ_DATA_AV 0x0002
-
-#define DATA_INTR_EN 0x560
-#define DATA_INTR_EN__WRITE_SPACE_AV 0x0001
-#define DATA_INTR_EN__READ_DATA_AV 0x0002
-
-#define GPREG_0 0x570
-#define GPREG_0__VALUE 0xffff
-
-#define GPREG_1 0x580
-#define GPREG_1__VALUE 0xffff
-
-#define GPREG_2 0x590
-#define GPREG_2__VALUE 0xffff
-
-#define GPREG_3 0x5a0
-#define GPREG_3__VALUE 0xffff
-
#define ECC_THRESHOLD 0x600
#define ECC_THRESHOLD__VALUE 0x03ff
@@ -331,69 +311,15 @@
#define CHNL_ACTIVE__CHANNEL2 0x0004
#define CHNL_ACTIVE__CHANNEL3 0x0008
-#define ACTIVE_SRC_ID 0x800
-#define ACTIVE_SRC_ID__VALUE 0x00ff
-
-#define PTN_INTR 0x810
-#define PTN_INTR__CONFIG_ERROR 0x0001
-#define PTN_INTR__ACCESS_ERROR_BANK0 0x0002
-#define PTN_INTR__ACCESS_ERROR_BANK1 0x0004
-#define PTN_INTR__ACCESS_ERROR_BANK2 0x0008
-#define PTN_INTR__ACCESS_ERROR_BANK3 0x0010
-#define PTN_INTR__REG_ACCESS_ERROR 0x0020
-
-#define PTN_INTR_EN 0x820
-#define PTN_INTR_EN__CONFIG_ERROR 0x0001
-#define PTN_INTR_EN__ACCESS_ERROR_BANK0 0x0002
-#define PTN_INTR_EN__ACCESS_ERROR_BANK1 0x0004
-#define PTN_INTR_EN__ACCESS_ERROR_BANK2 0x0008
-#define PTN_INTR_EN__ACCESS_ERROR_BANK3 0x0010
-#define PTN_INTR_EN__REG_ACCESS_ERROR 0x0020
-
-#define PERM_SRC_ID(__bank) (0x830 + ((__bank) * 0x40))
-#define PERM_SRC_ID__SRCID 0x00ff
-#define PERM_SRC_ID__DIRECT_ACCESS_ACTIVE 0x0800
-#define PERM_SRC_ID__WRITE_ACTIVE 0x2000
-#define PERM_SRC_ID__READ_ACTIVE 0x4000
-#define PERM_SRC_ID__PARTITION_VALID 0x8000
-
-#define MIN_BLK_ADDR(__bank) (0x840 + ((__bank) * 0x40))
-#define MIN_BLK_ADDR__VALUE 0xffff
-
-#define MAX_BLK_ADDR(__bank) (0x850 + ((__bank) * 0x40))
-#define MAX_BLK_ADDR__VALUE 0xffff
-
-#define MIN_MAX_BANK(__bank) (0x860 + ((__bank) * 0x40))
-#define MIN_MAX_BANK__MIN_VALUE 0x0003
-#define MIN_MAX_BANK__MAX_VALUE 0x000c
-
-
-/* ffsdefs.h */
-#define CLEAR 0 /*use this to clear a field instead of "fail"*/
-#define SET 1 /*use this to set a field instead of "pass"*/
#define FAIL 1 /*failed flag*/
#define PASS 0 /*success flag*/
-#define ERR -1 /*error flag*/
-
-/* lld.h */
-#define GOOD_BLOCK 0
-#define DEFECTIVE_BLOCK 1
-#define READ_ERROR 2
#define CLK_X 5
#define CLK_MULTI 4
-/* KBV - Updated to LNW scratch register address */
-#define SCRATCH_REG_ADDR CONFIG_MTD_NAND_DENALI_SCRATCH_REG_ADDR
-#define SCRATCH_REG_SIZE 64
-
-#define GLOB_HWCTL_DEFAULT_BLKS 2048
-
#define SUPPORT_15BITECC 1
#define SUPPORT_8BITECC 1
-#define CUSTOM_CONF_PARAMS 0
-
#define ONFI_BLOOM_TIME 1
#define MODE5_WORKAROUND 0
@@ -403,31 +329,6 @@
#define MODE_10 0x08000000
#define MODE_11 0x0C000000
-
-#define DATA_TRANSFER_MODE 0
-#define PROTECTION_PER_BLOCK 1
-#define LOAD_WAIT_COUNT 2
-#define PROGRAM_WAIT_COUNT 3
-#define ERASE_WAIT_COUNT 4
-#define INT_MONITOR_CYCLE_COUNT 5
-#define READ_BUSY_PIN_ENABLED 6
-#define MULTIPLANE_OPERATION_SUPPORT 7
-#define PRE_FETCH_MODE 8
-#define CE_DONT_CARE_SUPPORT 9
-#define COPYBACK_SUPPORT 10
-#define CACHE_WRITE_SUPPORT 11
-#define CACHE_READ_SUPPORT 12
-#define NUM_PAGES_IN_BLOCK 13
-#define ECC_ENABLE_SELECT 14
-#define WRITE_ENABLE_2_READ_ENABLE 15
-#define ADDRESS_2_DATA 16
-#define READ_ENABLE_2_WRITE_ENABLE 17
-#define TWO_ROW_ADDRESS_CYCLES 18
-#define MULTIPLANE_ADDRESS_RESTRICT 19
-#define ACC_CLOCKS 20
-#define READ_WRITE_ENABLE_LOW_COUNT 21
-#define READ_WRITE_ENABLE_HIGH_COUNT 22
-
#define ECC_SECTOR_SIZE 512
struct nand_buf {
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [PATCH v2 04/53] mtd: nand: denali: remove redundant define of BANK(x)
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
` (2 preceding siblings ...)
2017-03-22 14:07 ` [PATCH v2 03/53] mtd: nand: denali: remove unused CONFIG option and macros Masahiro Yamada
@ 2017-03-22 14:07 ` Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 05/53] mtd: nand: denali: remove more unused struct members Masahiro Yamada
` (19 subsequent siblings)
23 siblings, 0 replies; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-22 14:07 UTC (permalink / raw)
To: linux-mtd
Cc: laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Boris Brezillon,
Marek Vasut, Brian Norris, Graham Moore, David Woodhouse,
Masami Hiramatsu, Chuanxiao Dong, Jassi Brar, Masahiro Yamada,
linux-kernel, Richard Weinberger, Cyrille Pitchen
This macro is defined twice in denali.c (around line 98 and
line 651), so remove the second one.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2: None
drivers/mtd/nand/denali.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index f993e13..c9806e6 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -648,7 +648,6 @@ static irqreturn_t denali_isr(int irq, void *dev_id)
spin_unlock(&denali->irq_lock);
return result;
}
-#define BANK(x) ((x) << 24)
static uint32_t wait_for_irq(struct denali_nand_info *denali, uint32_t irq_mask)
{
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [PATCH v2 05/53] mtd: nand: denali: remove more unused struct members
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
` (3 preceding siblings ...)
2017-03-22 14:07 ` [PATCH v2 04/53] mtd: nand: denali: remove redundant define of BANK(x) Masahiro Yamada
@ 2017-03-22 14:07 ` Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 06/53] mtd: nand: denali: fix comment of denali_nand_info::flash_mem Masahiro Yamada
` (18 subsequent siblings)
23 siblings, 0 replies; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-22 14:07 UTC (permalink / raw)
To: linux-mtd
Cc: laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Boris Brezillon,
Marek Vasut, Brian Norris, Graham Moore, David Woodhouse,
Masami Hiramatsu, Chuanxiao Dong, Jassi Brar, Masahiro Yamada,
linux-kernel, Richard Weinberger, Cyrille Pitchen
These members are not used at all.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2:
- Add git description
drivers/mtd/nand/denali.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h
index c4f3a68..6573ea5 100644
--- a/drivers/mtd/nand/denali.h
+++ b/drivers/mtd/nand/denali.h
@@ -350,7 +350,6 @@ struct denali_nand_info {
struct nand_buf buf;
struct device *dev;
int total_used_banks;
- uint32_t block; /* stored for future use */
uint16_t page;
void __iomem *flash_reg; /* Mapped io reg base address */
void __iomem *flash_mem; /* Mapped io reg base address */
@@ -359,7 +358,6 @@ struct denali_nand_info {
struct completion complete;
spinlock_t irq_lock;
uint32_t irq_status;
- int irq_debug_array[32];
int irq;
uint32_t devnum; /* represent how many nands connected */
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [PATCH v2 06/53] mtd: nand: denali: fix comment of denali_nand_info::flash_mem
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
` (4 preceding siblings ...)
2017-03-22 14:07 ` [PATCH v2 05/53] mtd: nand: denali: remove more unused struct members Masahiro Yamada
@ 2017-03-22 14:07 ` Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 07/53] mtd: nand: denali: consolidate INTR_STATUS__* and INTR_EN__* macros Masahiro Yamada
` (17 subsequent siblings)
23 siblings, 0 replies; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-22 14:07 UTC (permalink / raw)
To: linux-mtd
Cc: laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Boris Brezillon,
Marek Vasut, Brian Norris, Graham Moore, David Woodhouse,
Masami Hiramatsu, Chuanxiao Dong, Jassi Brar, Masahiro Yamada,
linux-kernel, Richard Weinberger, Cyrille Pitchen
The same comment "Mapped io reg base address" for flash_reg and
flash_mem probably due to the mistake of copy-paste work.
Of course, the latter is not the register base address.
Reword the comments using the terminology in the Denali User's Guide.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2: None
drivers/mtd/nand/denali.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h
index 6573ea5..403a7c8 100644
--- a/drivers/mtd/nand/denali.h
+++ b/drivers/mtd/nand/denali.h
@@ -351,8 +351,8 @@ struct denali_nand_info {
struct device *dev;
int total_used_banks;
uint16_t page;
- void __iomem *flash_reg; /* Mapped io reg base address */
- void __iomem *flash_mem; /* Mapped io reg base address */
+ void __iomem *flash_reg; /* Register Interface */
+ void __iomem *flash_mem; /* Host Data/Command Interface */
/* elements used by ISR */
struct completion complete;
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [PATCH v2 07/53] mtd: nand: denali: consolidate INTR_STATUS__* and INTR_EN__* macros
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
` (5 preceding siblings ...)
2017-03-22 14:07 ` [PATCH v2 06/53] mtd: nand: denali: fix comment of denali_nand_info::flash_mem Masahiro Yamada
@ 2017-03-22 14:07 ` Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 08/53] mtd: nand: denali: introduce capability flag Masahiro Yamada
` (16 subsequent siblings)
23 siblings, 0 replies; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-22 14:07 UTC (permalink / raw)
To: linux-mtd
Cc: laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Boris Brezillon,
Marek Vasut, Brian Norris, Graham Moore, David Woodhouse,
Masami Hiramatsu, Chuanxiao Dong, Jassi Brar, Masahiro Yamada,
linux-kernel, Richard Weinberger, Cyrille Pitchen
The interrupts are enabled by INTR_EN register, then asserted
interrupts can be observed via INTR_STATUS register.
The bit fields are identical between INTR_EN and INTR_STATUS, so we
can merge the bit field macros. Likewise for DATA_INTR.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2:
- Newly added
drivers/mtd/nand/denali.c | 56 ++++++++++++++++++++-----------------------
drivers/mtd/nand/denali.h | 61 ++++++++++++++---------------------------------
2 files changed, 44 insertions(+), 73 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index c9806e6..2c59eb3 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -45,16 +45,16 @@ MODULE_PARM_DESC(onfi_timing_mode,
* We define a macro here that combines all interrupts this driver uses into
* a single constant value, for convenience.
*/
-#define DENALI_IRQ_ALL (INTR_STATUS__DMA_CMD_COMP | \
- INTR_STATUS__ECC_TRANSACTION_DONE | \
- INTR_STATUS__ECC_ERR | \
- INTR_STATUS__PROGRAM_FAIL | \
- INTR_STATUS__LOAD_COMP | \
- INTR_STATUS__PROGRAM_COMP | \
- INTR_STATUS__TIME_OUT | \
- INTR_STATUS__ERASE_FAIL | \
- INTR_STATUS__RST_COMP | \
- INTR_STATUS__ERASE_COMP)
+#define DENALI_IRQ_ALL (INTR__DMA_CMD_COMP | \
+ INTR__ECC_TRANSACTION_DONE | \
+ INTR__ECC_ERR | \
+ INTR__PROGRAM_FAIL | \
+ INTR__LOAD_COMP | \
+ INTR__PROGRAM_COMP | \
+ INTR__TIME_OUT | \
+ INTR__ERASE_FAIL | \
+ INTR__RST_COMP | \
+ INTR__ERASE_COMP)
/*
* indicates whether or not the internal value for the flash bank is
@@ -159,7 +159,7 @@ static void read_status(struct denali_nand_info *denali)
static void reset_bank(struct denali_nand_info *denali)
{
uint32_t irq_status;
- uint32_t irq_mask = INTR_STATUS__RST_COMP | INTR_STATUS__TIME_OUT;
+ uint32_t irq_mask = INTR__RST_COMP | INTR__TIME_OUT;
clear_interrupts(denali);
@@ -167,7 +167,7 @@ static void reset_bank(struct denali_nand_info *denali)
irq_status = wait_for_irq(denali, irq_mask);
- if (irq_status & INTR_STATUS__TIME_OUT)
+ if (irq_status & INTR__TIME_OUT)
dev_err(denali->dev, "reset bank failed.\n");
}
@@ -177,22 +177,22 @@ static uint16_t denali_nand_reset(struct denali_nand_info *denali)
int i;
for (i = 0; i < denali->max_banks; i++)
- iowrite32(INTR_STATUS__RST_COMP | INTR_STATUS__TIME_OUT,
+ iowrite32(INTR__RST_COMP | INTR__TIME_OUT,
denali->flash_reg + INTR_STATUS(i));
for (i = 0; i < denali->max_banks; i++) {
iowrite32(1 << i, denali->flash_reg + DEVICE_RESET);
while (!(ioread32(denali->flash_reg + INTR_STATUS(i)) &
- (INTR_STATUS__RST_COMP | INTR_STATUS__TIME_OUT)))
+ (INTR__RST_COMP | INTR__TIME_OUT)))
cpu_relax();
if (ioread32(denali->flash_reg + INTR_STATUS(i)) &
- INTR_STATUS__TIME_OUT)
+ INTR__TIME_OUT)
dev_dbg(denali->dev,
"NAND Reset operation timed out on bank %d\n", i);
}
for (i = 0; i < denali->max_banks; i++)
- iowrite32(INTR_STATUS__RST_COMP | INTR_STATUS__TIME_OUT,
+ iowrite32(INTR__RST_COMP | INTR__TIME_OUT,
denali->flash_reg + INTR_STATUS(i));
return PASS;
@@ -716,7 +716,7 @@ static int denali_send_pipeline_cmd(struct denali_nand_info *denali,
uint32_t addr, cmd, irq_status, irq_mask;
if (op == DENALI_READ)
- irq_mask = INTR_STATUS__LOAD_COMP;
+ irq_mask = INTR__LOAD_COMP;
else if (op == DENALI_WRITE)
irq_mask = 0;
else
@@ -823,8 +823,7 @@ static int write_oob_data(struct mtd_info *mtd, uint8_t *buf, int page)
{
struct denali_nand_info *denali = mtd_to_denali(mtd);
uint32_t irq_status;
- uint32_t irq_mask = INTR_STATUS__PROGRAM_COMP |
- INTR_STATUS__PROGRAM_FAIL;
+ uint32_t irq_mask = INTR__PROGRAM_COMP | INTR__PROGRAM_FAIL;
int status = 0;
denali->page = page;
@@ -851,7 +850,7 @@ static int write_oob_data(struct mtd_info *mtd, uint8_t *buf, int page)
static void read_oob_data(struct mtd_info *mtd, uint8_t *buf, int page)
{
struct denali_nand_info *denali = mtd_to_denali(mtd);
- uint32_t irq_mask = INTR_STATUS__LOAD_COMP;
+ uint32_t irq_mask = INTR__LOAD_COMP;
uint32_t irq_status, addr, cmd;
denali->page = page;
@@ -912,7 +911,7 @@ static bool handle_ecc(struct denali_nand_info *denali, uint8_t *buf,
bool check_erased_page = false;
unsigned int bitflips = 0;
- if (irq_status & INTR_STATUS__ECC_ERR) {
+ if (irq_status & INTR__ECC_ERR) {
/* read the ECC errors. we'll ignore them for now */
uint32_t err_address, err_correction_info, err_byte,
err_sector, err_device, err_correction_value;
@@ -969,7 +968,7 @@ static bool handle_ecc(struct denali_nand_info *denali, uint8_t *buf,
* for a while for this interrupt
*/
while (!(read_interrupt_status(denali) &
- INTR_STATUS__ECC_TRANSACTION_DONE))
+ INTR__ECC_TRANSACTION_DONE))
cpu_relax();
clear_interrupts(denali);
denali_set_intr_modes(denali, true);
@@ -1020,8 +1019,7 @@ static int write_page(struct mtd_info *mtd, struct nand_chip *chip,
dma_addr_t addr = denali->buf.dma_buf;
size_t size = mtd->writesize + mtd->oobsize;
uint32_t irq_status;
- uint32_t irq_mask = INTR_STATUS__DMA_CMD_COMP |
- INTR_STATUS__PROGRAM_FAIL;
+ uint32_t irq_mask = INTR__DMA_CMD_COMP | INTR__PROGRAM_FAIL;
/*
* if it is a raw xfer, we want to disable ecc and send the spare area.
@@ -1119,8 +1117,7 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
size_t size = mtd->writesize + mtd->oobsize;
uint32_t irq_status;
- uint32_t irq_mask = INTR_STATUS__ECC_TRANSACTION_DONE |
- INTR_STATUS__ECC_ERR;
+ uint32_t irq_mask = INTR__ECC_TRANSACTION_DONE | INTR__ECC_ERR;
bool check_erased_page = false;
if (page != denali->page) {
@@ -1168,7 +1165,7 @@ static int denali_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
struct denali_nand_info *denali = mtd_to_denali(mtd);
dma_addr_t addr = denali->buf.dma_buf;
size_t size = mtd->writesize + mtd->oobsize;
- uint32_t irq_mask = INTR_STATUS__DMA_CMD_COMP;
+ uint32_t irq_mask = INTR__DMA_CMD_COMP;
if (page != denali->page) {
dev_err(denali->dev,
@@ -1241,10 +1238,9 @@ static int denali_erase(struct mtd_info *mtd, int page)
index_addr(denali, cmd, 0x1);
/* wait for erase to complete or failure to occur */
- irq_status = wait_for_irq(denali, INTR_STATUS__ERASE_COMP |
- INTR_STATUS__ERASE_FAIL);
+ irq_status = wait_for_irq(denali, INTR__ERASE_COMP | INTR__ERASE_FAIL);
- return irq_status & INTR_STATUS__ERASE_FAIL ? NAND_STATUS_FAIL : PASS;
+ return irq_status & INTR__ERASE_FAIL ? NAND_STATUS_FAIL : PASS;
}
static void denali_cmdfunc(struct mtd_info *mtd, unsigned int cmd, int col,
diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h
index 403a7c8..8df2285 100644
--- a/drivers/mtd/nand/denali.h
+++ b/drivers/mtd/nand/denali.h
@@ -218,40 +218,22 @@
#define INTR_STATUS(__bank) (0x410 + ((__bank) * 0x50))
#define INTR_EN(__bank) (0x420 + ((__bank) * 0x50))
-
-#define INTR_STATUS__ECC_TRANSACTION_DONE 0x0001
-#define INTR_STATUS__ECC_ERR 0x0002
-#define INTR_STATUS__DMA_CMD_COMP 0x0004
-#define INTR_STATUS__TIME_OUT 0x0008
-#define INTR_STATUS__PROGRAM_FAIL 0x0010
-#define INTR_STATUS__ERASE_FAIL 0x0020
-#define INTR_STATUS__LOAD_COMP 0x0040
-#define INTR_STATUS__PROGRAM_COMP 0x0080
-#define INTR_STATUS__ERASE_COMP 0x0100
-#define INTR_STATUS__PIPE_CPYBCK_CMD_COMP 0x0200
-#define INTR_STATUS__LOCKED_BLK 0x0400
-#define INTR_STATUS__UNSUP_CMD 0x0800
-#define INTR_STATUS__INT_ACT 0x1000
-#define INTR_STATUS__RST_COMP 0x2000
-#define INTR_STATUS__PIPE_CMD_ERR 0x4000
-#define INTR_STATUS__PAGE_XFER_INC 0x8000
-
-#define INTR_EN__ECC_TRANSACTION_DONE 0x0001
-#define INTR_EN__ECC_ERR 0x0002
-#define INTR_EN__DMA_CMD_COMP 0x0004
-#define INTR_EN__TIME_OUT 0x0008
-#define INTR_EN__PROGRAM_FAIL 0x0010
-#define INTR_EN__ERASE_FAIL 0x0020
-#define INTR_EN__LOAD_COMP 0x0040
-#define INTR_EN__PROGRAM_COMP 0x0080
-#define INTR_EN__ERASE_COMP 0x0100
-#define INTR_EN__PIPE_CPYBCK_CMD_COMP 0x0200
-#define INTR_EN__LOCKED_BLK 0x0400
-#define INTR_EN__UNSUP_CMD 0x0800
-#define INTR_EN__INT_ACT 0x1000
-#define INTR_EN__RST_COMP 0x2000
-#define INTR_EN__PIPE_CMD_ERR 0x4000
-#define INTR_EN__PAGE_XFER_INC 0x8000
+#define INTR__ECC_TRANSACTION_DONE 0x0001
+#define INTR__ECC_ERR 0x0002
+#define INTR__DMA_CMD_COMP 0x0004
+#define INTR__TIME_OUT 0x0008
+#define INTR__PROGRAM_FAIL 0x0010
+#define INTR__ERASE_FAIL 0x0020
+#define INTR__LOAD_COMP 0x0040
+#define INTR__PROGRAM_COMP 0x0080
+#define INTR__ERASE_COMP 0x0100
+#define INTR__PIPE_CPYBCK_CMD_COMP 0x0200
+#define INTR__LOCKED_BLK 0x0400
+#define INTR__UNSUP_CMD 0x0800
+#define INTR__INT_ACT 0x1000
+#define INTR__RST_COMP 0x2000
+#define INTR__PIPE_CMD_ERR 0x4000
+#define INTR__PAGE_XFER_INC 0x8000
#define PAGE_CNT(__bank) (0x430 + ((__bank) * 0x50))
#define ERR_PAGE_ADDR(__bank) (0x440 + ((__bank) * 0x50))
@@ -284,20 +266,13 @@
#define IGNORE_ECC_DONE__FLAG 0x0001
#define DMA_INTR 0x720
+#define DMA_INTR_EN 0x730
#define DMA_INTR__TARGET_ERROR 0x0001
#define DMA_INTR__DESC_COMP_CHANNEL0 0x0002
#define DMA_INTR__DESC_COMP_CHANNEL1 0x0004
#define DMA_INTR__DESC_COMP_CHANNEL2 0x0008
#define DMA_INTR__DESC_COMP_CHANNEL3 0x0010
-#define DMA_INTR__MEMCOPY_DESC_COMP 0x0020
-
-#define DMA_INTR_EN 0x730
-#define DMA_INTR_EN__TARGET_ERROR 0x0001
-#define DMA_INTR_EN__DESC_COMP_CHANNEL0 0x0002
-#define DMA_INTR_EN__DESC_COMP_CHANNEL1 0x0004
-#define DMA_INTR_EN__DESC_COMP_CHANNEL2 0x0008
-#define DMA_INTR_EN__DESC_COMP_CHANNEL3 0x0010
-#define DMA_INTR_EN__MEMCOPY_DESC_COMP 0x0020
+#define DMA_INTR__MEMCOPY_DESC_COMP 0x0020
#define TARGET_ERR_ADDR_LO 0x740
#define TARGET_ERR_ADDR_LO__VALUE 0xffff
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [PATCH v2 08/53] mtd: nand: denali: introduce capability flag
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
` (6 preceding siblings ...)
2017-03-22 14:07 ` [PATCH v2 07/53] mtd: nand: denali: consolidate INTR_STATUS__* and INTR_EN__* macros Masahiro Yamada
@ 2017-03-22 14:07 ` Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 09/53] mtd: nand: denali: use int where no reason to use fixed width variable Masahiro Yamada
` (15 subsequent siblings)
23 siblings, 0 replies; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-22 14:07 UTC (permalink / raw)
To: linux-mtd
Cc: laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Boris Brezillon,
Marek Vasut, Brian Norris, Graham Moore, David Woodhouse,
Masami Hiramatsu, Chuanxiao Dong, Jassi Brar, Masahiro Yamada,
linux-kernel, Richard Weinberger, Cyrille Pitchen
The Denali NAND controller IP has various customizable features.
SoC vendors can choose desired functions when a delivery RTL is
created. It means there are several variants for this IP. For
example, the Intel version is equipped with 32bit DMA, whereas the
IP for UniPhier SoC family with 64bit DMA.
This driver was originally written for some Intel platforms with
Intel specific things hard-coded. What is worse, the revision
register of this IP does not work to distinguish such features.
We need to do something to make the driver available for other SoCs.
Let's introduce a caps member to the denali_nand_info structure to
switch on/off various features. Also, add struct denali_dt_data to
store the capability associated with compatible string.
Boris suggested this approach in discussion [1] instead of a new DT
property for every feature.
[1] https://lkml.org/lkml/2016/3/29/142
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2: None
drivers/mtd/nand/denali.h | 1 +
drivers/mtd/nand/denali_dt.c | 18 +++++++++---------
2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h
index 8df2285..eed001d 100644
--- a/drivers/mtd/nand/denali.h
+++ b/drivers/mtd/nand/denali.h
@@ -338,6 +338,7 @@ struct denali_nand_info {
uint32_t devnum; /* represent how many nands connected */
uint32_t bbtskipbytes;
uint32_t max_banks;
+ unsigned int caps;
};
extern int denali_init(struct denali_nand_info *denali);
diff --git a/drivers/mtd/nand/denali_dt.c b/drivers/mtd/nand/denali_dt.c
index 5607fcd..293ddb8 100644
--- a/drivers/mtd/nand/denali_dt.c
+++ b/drivers/mtd/nand/denali_dt.c
@@ -29,6 +29,10 @@ struct denali_dt {
struct clk *clk;
};
+struct denali_dt_data {
+ unsigned int caps;
+};
+
static const struct of_device_id denali_nand_dt_ids[] = {
{ .compatible = "denali,denali-nand-dt" },
{ /* sentinel */ }
@@ -42,23 +46,19 @@ static int denali_dt_probe(struct platform_device *ofdev)
{
struct resource *denali_reg, *nand_data;
struct denali_dt *dt;
+ const struct denali_dt_data *data;
struct denali_nand_info *denali;
int ret;
- const struct of_device_id *of_id;
-
- of_id = of_match_device(denali_nand_dt_ids, &ofdev->dev);
- if (of_id) {
- ofdev->id_entry = of_id->data;
- } else {
- pr_err("Failed to find the right device id.\n");
- return -ENOMEM;
- }
dt = devm_kzalloc(&ofdev->dev, sizeof(*dt), GFP_KERNEL);
if (!dt)
return -ENOMEM;
denali = &dt->denali;
+ data = of_device_get_match_data(&ofdev->dev);
+ if (data)
+ denali->caps = data->caps;
+
denali->platform = DT;
denali->dev = &ofdev->dev;
denali->irq = platform_get_irq(ofdev, 0);
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [PATCH v2 09/53] mtd: nand: denali: use int where no reason to use fixed width variable
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
` (7 preceding siblings ...)
2017-03-22 14:07 ` [PATCH v2 08/53] mtd: nand: denali: introduce capability flag Masahiro Yamada
@ 2017-03-22 14:07 ` Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 10/53] mtd: nand: denali: fix erased page checking Masahiro Yamada
` (14 subsequent siblings)
23 siblings, 0 replies; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-22 14:07 UTC (permalink / raw)
To: linux-mtd
Cc: laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Boris Brezillon,
Marek Vasut, Brian Norris, Graham Moore, David Woodhouse,
Masami Hiramatsu, Chuanxiao Dong, Jassi Brar, Masahiro Yamada,
linux-kernel, Richard Weinberger, Cyrille Pitchen
The page number is generally stored in an integer type variable.
The uint16_t does not have enough width. I see no reason to use
uint32_t for other members, either. Just use int.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2: None
drivers/mtd/nand/denali.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h
index eed001d..7b2d785 100644
--- a/drivers/mtd/nand/denali.h
+++ b/drivers/mtd/nand/denali.h
@@ -325,7 +325,7 @@ struct denali_nand_info {
struct nand_buf buf;
struct device *dev;
int total_used_banks;
- uint16_t page;
+ int page;
void __iomem *flash_reg; /* Register Interface */
void __iomem *flash_mem; /* Host Data/Command Interface */
@@ -335,9 +335,9 @@ struct denali_nand_info {
uint32_t irq_status;
int irq;
- uint32_t devnum; /* represent how many nands connected */
- uint32_t bbtskipbytes;
- uint32_t max_banks;
+ int devnum; /* represent how many nands connected */
+ int bbtskipbytes;
+ int max_banks;
unsigned int caps;
};
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [PATCH v2 10/53] mtd: nand: denali: fix erased page checking
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
` (8 preceding siblings ...)
2017-03-22 14:07 ` [PATCH v2 09/53] mtd: nand: denali: use int where no reason to use fixed width variable Masahiro Yamada
@ 2017-03-22 14:07 ` Masahiro Yamada
2017-03-22 20:36 ` Boris Brezillon
2017-03-22 20:56 ` Boris Brezillon
2017-03-22 14:07 ` [PATCH v2 11/53] mtd: nand: denali: fix bitflips calculation in handle_ecc() Masahiro Yamada
` (13 subsequent siblings)
23 siblings, 2 replies; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-22 14:07 UTC (permalink / raw)
To: linux-mtd
Cc: laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Boris Brezillon,
Marek Vasut, Brian Norris, Graham Moore, David Woodhouse,
Masami Hiramatsu, Chuanxiao Dong, Jassi Brar, Masahiro Yamada,
linux-kernel, Richard Weinberger, Cyrille Pitchen
This part is wrong in multiple ways:
[1] is_erased() is called against "buf" twice, so the second one is
meaningless. The second call should check chip->oob_poi.
[2] This code block is nested by double "if (check_erase_page)".
The inner one is redundant.
[3] Erased page checking without threshold is false-positive.
Basically, there are two ways for erased page checking:
- read the whole of page + oob in raw transfer, then check if all
the data are 0xFF.
- read the ECC-corrected page + oob, then check if *almost* all the
data are 0xFF (bit-flips less than ecc.strength are allowed)
While here, it checks if all data in ECC-corrected page are 0xFF.
This is too strong because not all of the data are 0xFF after they
are manipulated by the ECC engine. Proper threshold must be taken
into account to avoid false-positive ecc_stats.failed increments.
[4] positive return value for uncorrectable bitflips
The comment of ecc->read_page() says it should return "0 if bitflips
uncorrectable", but the current code could return a positive value
in the case.
This commit solves the problems above. The nand framework provides
a helper nand_check_erased_ecc_chunk() for erased page check with
threshold. The driver's own helper is unneeded.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2:
- Squash some patches into one.
- Use nand_check_erased_ecc_chunk() with threshold
drivers/mtd/nand/denali.c | 29 ++++++++++-------------------
1 file changed, 10 insertions(+), 19 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 2c59eb3..86381ac 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -883,19 +883,6 @@ static void read_oob_data(struct mtd_info *mtd, uint8_t *buf, int page)
}
}
-/*
- * this function examines buffers to see if they contain data that
- * indicate that the buffer is part of an erased region of flash.
- */
-static bool is_erased(uint8_t *buf, int len)
-{
- int i;
-
- for (i = 0; i < len; i++)
- if (buf[i] != 0xFF)
- return false;
- return true;
-}
#define ECC_SECTOR_SIZE 512
#define ECC_SECTOR(x) (((x) & ECC_ERROR_ADDRESS__SECTOR_NR) >> 12)
@@ -1119,6 +1106,7 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
uint32_t irq_status;
uint32_t irq_mask = INTR__ECC_TRANSACTION_DONE | INTR__ECC_ERR;
bool check_erased_page = false;
+ int stat;
if (page != denali->page) {
dev_err(denali->dev,
@@ -1148,12 +1136,15 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
if (check_erased_page) {
read_oob_data(mtd, chip->oob_poi, denali->page);
- /* check ECC failures that may have occurred on erased pages */
- if (check_erased_page) {
- if (!is_erased(buf, mtd->writesize))
- mtd->ecc_stats.failed++;
- if (!is_erased(buf, mtd->oobsize))
- mtd->ecc_stats.failed++;
+ stat = nand_check_erased_ecc_chunk(
+ buf, mtd->writesize,
+ chip->oob_poi, mtd->oobsize,
+ NULL, 0,
+ chip->ecc.strength * chip->ecc.steps);
+ if (stat < 0) {
+ mtd->ecc_stats.failed++;
+ /* return 0 for uncorrectable bitflips */
+ max_bitflips = 0;
}
}
return max_bitflips;
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v2 10/53] mtd: nand: denali: fix erased page checking
2017-03-22 14:07 ` [PATCH v2 10/53] mtd: nand: denali: fix erased page checking Masahiro Yamada
@ 2017-03-22 20:36 ` Boris Brezillon
2017-03-23 5:15 ` Masahiro Yamada
2017-03-22 20:56 ` Boris Brezillon
1 sibling, 1 reply; 47+ messages in thread
From: Boris Brezillon @ 2017-03-22 20:36 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-mtd, laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Marek Vasut,
Brian Norris, Graham Moore, David Woodhouse, Masami Hiramatsu,
Chuanxiao Dong, Jassi Brar, linux-kernel, Richard Weinberger,
Cyrille Pitchen
On Wed, 22 Mar 2017 23:07:17 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> This part is wrong in multiple ways:
>
> [1] is_erased() is called against "buf" twice, so the second one is
> meaningless. The second call should check chip->oob_poi.
>
> [2] This code block is nested by double "if (check_erase_page)".
> The inner one is redundant.
>
> [3] Erased page checking without threshold is false-positive.
> Basically, there are two ways for erased page checking:
> - read the whole of page + oob in raw transfer, then check if all
> the data are 0xFF.
> - read the ECC-corrected page + oob, then check if *almost* all the
> data are 0xFF (bit-flips less than ecc.strength are allowed)
> While here, it checks if all data in ECC-corrected page are 0xFF.
> This is too strong because not all of the data are 0xFF after they
> are manipulated by the ECC engine. Proper threshold must be taken
> into account to avoid false-positive ecc_stats.failed increments.
Hm, the ECC engine should not introduce extra bitflips. I've seen 3
different cases in the various ECC engine I worked with:
1/ the ECC engine is able to correct bitflips in erased pages. In this
case you should trust it and return the number of corrected
bitflips or increment the ECC failed counter if it reports
uncorrectable errors.
2/ the ECC engine is able to detect erased pages, but fails to detect
those containing bitflips in it. In this case, you should rely on
the default "empty page" detection and only manually check if the
page is almost filled with 0xff when an error is reported.
3/ the ECC engine does not detect empty pages at all. In this case, you
should check if the page empty (or almost empty) each time an ECC
error is reported
In any case, if the ECC engine reports uncorrectable errors, it should
keep the data untouched, which means you don't have to re-read the whole
page in raw mode, only the OOB bytes.
>
> [4] positive return value for uncorrectable bitflips
>
> The comment of ecc->read_page() says it should return "0 if bitflips
> uncorrectable", but the current code could return a positive value
> in the case.
This one should probably be fixed in the core. Returning a negative
error core for uncorrectable errors is forbidden, but reporting the
maximum number of bitflips that have been corrected in each valid
ECC sector of the page (even if the page contains uncorrectable
sectors) does not sound like a bad idea to me.
The reason the core asks drivers to return 0 in case of uncorrectable
errors is because it updates the max_bitflips variable before testing
if the page contains uncorrectable errors [1]. Moving this statement
here [2] (in an else branch) should solve the problem for all drivers
returning positive numbers even when uncorrectable errors are detected
in one of the ECC chunk contained in a page.
[1]http://lxr.free-electrons.com/source/drivers/mtd/nand/nand_base.c#L1999
[2]http://lxr.free-electrons.com/source/drivers/mtd/nand/nand_base.c#L2048
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 10/53] mtd: nand: denali: fix erased page checking
2017-03-22 20:36 ` Boris Brezillon
@ 2017-03-23 5:15 ` Masahiro Yamada
2017-03-23 8:03 ` Boris Brezillon
0 siblings, 1 reply; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-23 5:15 UTC (permalink / raw)
To: Boris Brezillon
Cc: linux-mtd, Laurent Monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Marek Vasut,
Brian Norris, Graham Moore, David Woodhouse, Masami Hiramatsu,
Chuanxiao Dong, Jassi Brar, Linux Kernel Mailing List,
Richard Weinberger, Cyrille Pitchen
Hi Boris,
2017-03-23 5:36 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Wed, 22 Mar 2017 23:07:17 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> This part is wrong in multiple ways:
>>
>> [1] is_erased() is called against "buf" twice, so the second one is
>> meaningless. The second call should check chip->oob_poi.
>>
>> [2] This code block is nested by double "if (check_erase_page)".
>> The inner one is redundant.
>>
>> [3] Erased page checking without threshold is false-positive.
>> Basically, there are two ways for erased page checking:
>> - read the whole of page + oob in raw transfer, then check if all
>> the data are 0xFF.
>> - read the ECC-corrected page + oob, then check if *almost* all the
>> data are 0xFF (bit-flips less than ecc.strength are allowed)
>> While here, it checks if all data in ECC-corrected page are 0xFF.
>> This is too strong because not all of the data are 0xFF after they
>> are manipulated by the ECC engine. Proper threshold must be taken
>> into account to avoid false-positive ecc_stats.failed increments.
>
> Hm, the ECC engine should not introduce extra bitflips. I've seen 3
> different cases in the various ECC engine I worked with:
>
> 1/ the ECC engine is able to correct bitflips in erased pages. In this
> case you should trust it and return the number of corrected
> bitflips or increment the ECC failed counter if it reports
> uncorrectable errors.
> 2/ the ECC engine is able to detect erased pages, but fails to detect
> those containing bitflips in it. In this case, you should rely on
> the default "empty page" detection and only manually check if the
> page is almost filled with 0xff when an error is reported.
> 3/ the ECC engine does not detect empty pages at all. In this case, you
> should check if the page empty (or almost empty) each time an ECC
> error is reported
I think the Denali is case 3.
But, very new versions of this IP support erased page detection by hardware.
Please see 49/53:
http://patchwork.ozlabs.org/patch/742414/
Unfortunately this feature is not exactly what we want.
We want to detect per-sector empty'ness,
but this features is actually page oriented.
If you are unhappy about this,
it is possible to always turn off this feature
and use software detection (with nand_check_erased_ecc_chunk)
> In any case, if the ECC engine reports uncorrectable errors, it should
> keep the data untouched, which means you don't have to re-read the whole
> page in raw mode, only the OOB bytes.
OK. We should respect the result from the ECC engine,
but we still need to fill the buffer with 0xff
if the page turned out to be empty.
(nand_check_erased_ecc_chunk() does this for us.)
>>
>> [4] positive return value for uncorrectable bitflips
>>
>> The comment of ecc->read_page() says it should return "0 if bitflips
>> uncorrectable", but the current code could return a positive value
>> in the case.
>
> This one should probably be fixed in the core. Returning a negative
> error core for uncorrectable errors is forbidden, but reporting the
> maximum number of bitflips that have been corrected in each valid
> ECC sector of the page (even if the page contains uncorrectable
> sectors) does not sound like a bad idea to me.
>
> The reason the core asks drivers to return 0 in case of uncorrectable
> errors is because it updates the max_bitflips variable before testing
> if the page contains uncorrectable errors [1]. Moving this statement
> here [2] (in an else branch) should solve the problem for all drivers
> returning positive numbers even when uncorrectable errors are detected
> in one of the ECC chunk contained in a page.
I understood your idea, but do you want this change in this series?
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 10/53] mtd: nand: denali: fix erased page checking
2017-03-23 5:15 ` Masahiro Yamada
@ 2017-03-23 8:03 ` Boris Brezillon
0 siblings, 0 replies; 47+ messages in thread
From: Boris Brezillon @ 2017-03-23 8:03 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-mtd, Laurent Monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Marek Vasut,
Brian Norris, Graham Moore, David Woodhouse, Masami Hiramatsu,
Chuanxiao Dong, Jassi Brar, Linux Kernel Mailing List,
Richard Weinberger, Cyrille Pitchen
On Thu, 23 Mar 2017 14:15:59 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> Hi Boris,
>
>
> 2017-03-23 5:36 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > On Wed, 22 Mar 2017 23:07:17 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >
> >> This part is wrong in multiple ways:
> >>
> >> [1] is_erased() is called against "buf" twice, so the second one is
> >> meaningless. The second call should check chip->oob_poi.
> >>
> >> [2] This code block is nested by double "if (check_erase_page)".
> >> The inner one is redundant.
> >>
> >> [3] Erased page checking without threshold is false-positive.
> >> Basically, there are two ways for erased page checking:
> >> - read the whole of page + oob in raw transfer, then check if all
> >> the data are 0xFF.
> >> - read the ECC-corrected page + oob, then check if *almost* all the
> >> data are 0xFF (bit-flips less than ecc.strength are allowed)
> >> While here, it checks if all data in ECC-corrected page are 0xFF.
> >> This is too strong because not all of the data are 0xFF after they
> >> are manipulated by the ECC engine. Proper threshold must be taken
> >> into account to avoid false-positive ecc_stats.failed increments.
> >
> > Hm, the ECC engine should not introduce extra bitflips. I've seen 3
> > different cases in the various ECC engine I worked with:
> >
> > 1/ the ECC engine is able to correct bitflips in erased pages. In this
> > case you should trust it and return the number of corrected
> > bitflips or increment the ECC failed counter if it reports
> > uncorrectable errors.
> > 2/ the ECC engine is able to detect erased pages, but fails to detect
> > those containing bitflips in it. In this case, you should rely on
> > the default "empty page" detection and only manually check if the
> > page is almost filled with 0xff when an error is reported.
> > 3/ the ECC engine does not detect empty pages at all. In this case, you
> > should check if the page empty (or almost empty) each time an ECC
> > error is reported
>
>
> I think the Denali is case 3.
> But, very new versions of this IP support erased page detection by hardware.
> Please see 49/53:
> http://patchwork.ozlabs.org/patch/742414/
>
> Unfortunately this feature is not exactly what we want.
> We want to detect per-sector empty'ness,
> but this features is actually page oriented.
>
> If you are unhappy about this,
> it is possible to always turn off this feature
> and use software detection (with nand_check_erased_ecc_chunk)
As long as the engine reports the maximum number of
bitflips-per-ECC-chunk we're good. Of course, if you have an
uncorrectable error reported and your engine does not tell you in which
chunk(s) this happened, you'll have to call
nand_check_erased_ecc_chunk() on all chunks, but that should be fine.
>
>
>
> > In any case, if the ECC engine reports uncorrectable errors, it should
> > keep the data untouched, which means you don't have to re-read the whole
> > page in raw mode, only the OOB bytes.
>
>
> OK. We should respect the result from the ECC engine,
> but we still need to fill the buffer with 0xff
> if the page turned out to be empty.
> (nand_check_erased_ecc_chunk() does this for us.)
Yes, calling nand_check_erased_ecc_chunk() is still needed.
>
>
>
>
>
> >>
> >> [4] positive return value for uncorrectable bitflips
> >>
> >> The comment of ecc->read_page() says it should return "0 if bitflips
> >> uncorrectable", but the current code could return a positive value
> >> in the case.
> >
> > This one should probably be fixed in the core. Returning a negative
> > error core for uncorrectable errors is forbidden, but reporting the
> > maximum number of bitflips that have been corrected in each valid
> > ECC sector of the page (even if the page contains uncorrectable
> > sectors) does not sound like a bad idea to me.
> >
> > The reason the core asks drivers to return 0 in case of uncorrectable
> > errors is because it updates the max_bitflips variable before testing
> > if the page contains uncorrectable errors [1]. Moving this statement
> > here [2] (in an else branch) should solve the problem for all drivers
> > returning positive numbers even when uncorrectable errors are detected
> > in one of the ECC chunk contained in a page.
>
>
> I understood your idea, but do you want this change in this series?
Not necessarily, but I'm pretty sure other drivers are doing the same
mistake, so we'd better fix it in one place and stop requiring drivers
to return 0 if at least one ECC chunk is uncorrectable in the page.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 10/53] mtd: nand: denali: fix erased page checking
2017-03-22 14:07 ` [PATCH v2 10/53] mtd: nand: denali: fix erased page checking Masahiro Yamada
2017-03-22 20:36 ` Boris Brezillon
@ 2017-03-22 20:56 ` Boris Brezillon
2017-03-23 5:04 ` Masahiro Yamada
1 sibling, 1 reply; 47+ messages in thread
From: Boris Brezillon @ 2017-03-22 20:56 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-mtd, laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Marek Vasut,
Brian Norris, Graham Moore, David Woodhouse, Masami Hiramatsu,
Chuanxiao Dong, Jassi Brar, linux-kernel, Richard Weinberger,
Cyrille Pitchen
On Wed, 22 Mar 2017 23:07:17 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> dev_err(denali->dev,
> @@ -1148,12 +1136,15 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
> if (check_erased_page) {
> read_oob_data(mtd, chip->oob_poi, denali->page);
>
> - /* check ECC failures that may have occurred on erased pages */
> - if (check_erased_page) {
> - if (!is_erased(buf, mtd->writesize))
> - mtd->ecc_stats.failed++;
> - if (!is_erased(buf, mtd->oobsize))
> - mtd->ecc_stats.failed++;
> + stat = nand_check_erased_ecc_chunk(
> + buf, mtd->writesize,
> + chip->oob_poi, mtd->oobsize,
> + NULL, 0,
> + chip->ecc.strength * chip->ecc.steps);
That's not how it's supposed to be done. Each chunk should be checked
independently. Here is a simple example explaining why this is
important:
Let's consider the following setup:
- 4k pages
- 16bits/1024bytes ECC
With your approach, you turn this into:
- 4k pages
- 64bits/4096bytes ECC
Now suppose you have 32 bitflips in the first 1024 bytes. The real ECC
config is expected to report uncorrectable errors, but your approach
will just report that 32 bits have been fixed, which is wrong.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v2 10/53] mtd: nand: denali: fix erased page checking
2017-03-22 20:56 ` Boris Brezillon
@ 2017-03-23 5:04 ` Masahiro Yamada
2017-03-23 7:56 ` Boris Brezillon
0 siblings, 1 reply; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-23 5:04 UTC (permalink / raw)
To: Boris Brezillon
Cc: linux-mtd, Laurent Monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Marek Vasut,
Brian Norris, Graham Moore, David Woodhouse, Masami Hiramatsu,
Chuanxiao Dong, Jassi Brar, Linux Kernel Mailing List,
Richard Weinberger, Cyrille Pitchen
Hi Boris,
2017-03-23 5:56 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Wed, 22 Mar 2017 23:07:17 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>> dev_err(denali->dev,
>> @@ -1148,12 +1136,15 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
>> if (check_erased_page) {
>> read_oob_data(mtd, chip->oob_poi, denali->page);
>>
>> - /* check ECC failures that may have occurred on erased pages */
>> - if (check_erased_page) {
>> - if (!is_erased(buf, mtd->writesize))
>> - mtd->ecc_stats.failed++;
>> - if (!is_erased(buf, mtd->oobsize))
>> - mtd->ecc_stats.failed++;
>> + stat = nand_check_erased_ecc_chunk(
>> + buf, mtd->writesize,
>> + chip->oob_poi, mtd->oobsize,
>> + NULL, 0,
>> + chip->ecc.strength * chip->ecc.steps);
>
> That's not how it's supposed to be done. Each chunk should be checked
> independently. Here is a simple example explaining why this is
> important:
>
> Let's consider the following setup:
> - 4k pages
> - 16bits/1024bytes ECC
>
> With your approach, you turn this into:
> - 4k pages
> - 64bits/4096bytes ECC
>
> Now suppose you have 32 bitflips in the first 1024 bytes. The real ECC
> config is expected to report uncorrectable errors, but your approach
> will just report that 32 bits have been fixed, which is wrong.
OK. How about adding a helper like follows:
static int denali_check_erased_page(struct mtd_info *mtd,
struct nand_chip *chip, uint8_t *buf)
{
uint8_t *ecc_code = chip->buffers->ecccode;
int ecc_steps = chip->ecc.steps;
int ecc_size = chip->ecc.size;
int ecc_bytes = chip->ecc.bytes;
int i, ret;
ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, chip->oob_poi, 0,
chip->ecc.total);
if (ret)
return ret;
for (i = 0; i < ecc_steps; i++) {
ret = nand_check_erased_ecc_chunk(buf, ecc_size,
ecc_code, ecc_bytes,
NULL, 0,
chip->ecc.strength);
if (ret < 0)
return ret;
buf += ecc_size;
ecc_code += ecc_bytes;
}
return 0;
}
Then,
stat = denali_check_erased_page(mtd, chip, buf);
if (stat < 0) {
mtd->ecc_stats.failed++;
/* return 0 for uncorrectable bitflips */
stat = 0;
}
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v2 10/53] mtd: nand: denali: fix erased page checking
2017-03-23 5:04 ` Masahiro Yamada
@ 2017-03-23 7:56 ` Boris Brezillon
2017-03-24 2:43 ` Masahiro Yamada
0 siblings, 1 reply; 47+ messages in thread
From: Boris Brezillon @ 2017-03-23 7:56 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-mtd, Laurent Monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Marek Vasut,
Brian Norris, Graham Moore, David Woodhouse, Masami Hiramatsu,
Chuanxiao Dong, Jassi Brar, Linux Kernel Mailing List,
Richard Weinberger, Cyrille Pitchen
On Thu, 23 Mar 2017 14:04:44 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> Hi Boris,
>
> 2017-03-23 5:56 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > On Wed, 22 Mar 2017 23:07:17 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >> dev_err(denali->dev,
> >> @@ -1148,12 +1136,15 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
> >> if (check_erased_page) {
> >> read_oob_data(mtd, chip->oob_poi, denali->page);
> >>
> >> - /* check ECC failures that may have occurred on erased pages */
> >> - if (check_erased_page) {
> >> - if (!is_erased(buf, mtd->writesize))
> >> - mtd->ecc_stats.failed++;
> >> - if (!is_erased(buf, mtd->oobsize))
> >> - mtd->ecc_stats.failed++;
> >> + stat = nand_check_erased_ecc_chunk(
> >> + buf, mtd->writesize,
> >> + chip->oob_poi, mtd->oobsize,
> >> + NULL, 0,
> >> + chip->ecc.strength * chip->ecc.steps);
> >
> > That's not how it's supposed to be done. Each chunk should be checked
> > independently. Here is a simple example explaining why this is
> > important:
> >
> > Let's consider the following setup:
> > - 4k pages
> > - 16bits/1024bytes ECC
> >
> > With your approach, you turn this into:
> > - 4k pages
> > - 64bits/4096bytes ECC
> >
> > Now suppose you have 32 bitflips in the first 1024 bytes. The real ECC
> > config is expected to report uncorrectable errors, but your approach
> > will just report that 32 bits have been fixed, which is wrong.
>
>
> OK. How about adding a helper like follows:
>
> static int denali_check_erased_page(struct mtd_info *mtd,
> struct nand_chip *chip, uint8_t *buf)
> {
> uint8_t *ecc_code = chip->buffers->ecccode;
> int ecc_steps = chip->ecc.steps;
> int ecc_size = chip->ecc.size;
> int ecc_bytes = chip->ecc.bytes;
> int i, ret;
>
> ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, chip->oob_poi, 0,
> chip->ecc.total);
> if (ret)
> return ret;
>
> for (i = 0; i < ecc_steps; i++) {
> ret = nand_check_erased_ecc_chunk(buf, ecc_size,
> ecc_code, ecc_bytes,
> NULL, 0,
> chip->ecc.strength);
> if (ret < 0)
> return ret;
> buf += ecc_size;
> ecc_code += ecc_bytes;
> }
>
> return 0;
> }
>
>
>
> Then,
>
> stat = denali_check_erased_page(mtd, chip, buf);
> if (stat < 0) {
> mtd->ecc_stats.failed++;
> /* return 0 for uncorrectable bitflips */
> stat = 0;
> }
What's the point of checking all ECC chunks if only one contains ECC
errors? I really recommend to put the nand_check_erased_ecc_chunk()
call next to the per-ECC-block correction test.
Also, mtd->ecc_stats.failed is supposed to be incremented each time an
uncorrectable error is detected. In your denali_sw_ecc_fixup()
implementation you can detect errors at the ECC chunk level, so you
should increment ecc_stats.failed for each failure and not once if at
least one chunk is faulty.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v2 10/53] mtd: nand: denali: fix erased page checking
2017-03-23 7:56 ` Boris Brezillon
@ 2017-03-24 2:43 ` Masahiro Yamada
2017-03-24 8:06 ` Boris Brezillon
0 siblings, 1 reply; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-24 2:43 UTC (permalink / raw)
To: Boris Brezillon
Cc: linux-mtd, Laurent Monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Marek Vasut,
Brian Norris, Graham Moore, David Woodhouse, Masami Hiramatsu,
Chuanxiao Dong, Jassi Brar, Linux Kernel Mailing List,
Richard Weinberger, Cyrille Pitchen
Hi Boris,
2017-03-23 16:56 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Thu, 23 Mar 2017 14:04:44 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> Hi Boris,
>>
>> 2017-03-23 5:56 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> > On Wed, 22 Mar 2017 23:07:17 +0900
>> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>> >> dev_err(denali->dev,
>> >> @@ -1148,12 +1136,15 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
>> >> if (check_erased_page) {
>> >> read_oob_data(mtd, chip->oob_poi, denali->page);
>> >>
>> >> - /* check ECC failures that may have occurred on erased pages */
>> >> - if (check_erased_page) {
>> >> - if (!is_erased(buf, mtd->writesize))
>> >> - mtd->ecc_stats.failed++;
>> >> - if (!is_erased(buf, mtd->oobsize))
>> >> - mtd->ecc_stats.failed++;
>> >> + stat = nand_check_erased_ecc_chunk(
>> >> + buf, mtd->writesize,
>> >> + chip->oob_poi, mtd->oobsize,
>> >> + NULL, 0,
>> >> + chip->ecc.strength * chip->ecc.steps);
>> >
>> > That's not how it's supposed to be done. Each chunk should be checked
>> > independently. Here is a simple example explaining why this is
>> > important:
>> >
>> > Let's consider the following setup:
>> > - 4k pages
>> > - 16bits/1024bytes ECC
>> >
>> > With your approach, you turn this into:
>> > - 4k pages
>> > - 64bits/4096bytes ECC
>> >
>> > Now suppose you have 32 bitflips in the first 1024 bytes. The real ECC
>> > config is expected to report uncorrectable errors, but your approach
>> > will just report that 32 bits have been fixed, which is wrong.
>>
>>
>> OK. How about adding a helper like follows:
>>
>> static int denali_check_erased_page(struct mtd_info *mtd,
>> struct nand_chip *chip, uint8_t *buf)
>> {
>> uint8_t *ecc_code = chip->buffers->ecccode;
>> int ecc_steps = chip->ecc.steps;
>> int ecc_size = chip->ecc.size;
>> int ecc_bytes = chip->ecc.bytes;
>> int i, ret;
>>
>> ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, chip->oob_poi, 0,
>> chip->ecc.total);
>> if (ret)
>> return ret;
>>
>> for (i = 0; i < ecc_steps; i++) {
>> ret = nand_check_erased_ecc_chunk(buf, ecc_size,
>> ecc_code, ecc_bytes,
>> NULL, 0,
>> chip->ecc.strength);
>> if (ret < 0)
>> return ret;
>> buf += ecc_size;
>> ecc_code += ecc_bytes;
>> }
>>
>> return 0;
>> }
>>
>>
>>
>> Then,
>>
>> stat = denali_check_erased_page(mtd, chip, buf);
>> if (stat < 0) {
>> mtd->ecc_stats.failed++;
>> /* return 0 for uncorrectable bitflips */
>> stat = 0;
>> }
>
> What's the point of checking all ECC chunks if only one contains ECC
> errors? I really recommend to put the nand_check_erased_ecc_chunk()
> call next to the per-ECC-block correction test.
OK. I can fix it for software ECC fixup.
What should I do for hardware ECC fixup case?
http://patchwork.ozlabs.org/patch/742321/
If at least one ECC sector fails to correct bit-flips,
the controller sets INTR__ECC_UNCOR_ERR flag.
In this case, we can not know the number of uncorrectable errors.
Possible solutions are:
- Increment ecc_stats.failed only by one (compromised solution)
- If the controller IP supports sub-page read,
transfer sectors once again, one by one, checking the register
flag each time.
As far as I see, there are three cases.
[1] SW ECC fixup (Intel)
This can be fixed
[2] HW ECC fixup is supported, but sub-page read is not supported
(old UniPhier SoCs, probably SOCFPGA too)
[3] HW ECC fixup is supported, and sub-page read is supported as well
(new UniPhier SoCs)
I do not know how to precisely increment
ecc_stats.failed and ecc_stats.corrected for [2].
As for [3], we can solve the issue by making more efforts,
but I am not sure this effort is worthwhile.
> Also, mtd->ecc_stats.failed is supposed to be incremented each time an
> uncorrectable error is detected. In your denali_sw_ecc_fixup()
> implementation you can detect errors at the ECC chunk level, so you
> should increment ecc_stats.failed for each failure and not once if at
> least one chunk is faulty.
Yes, I can do this for denali_sw_ecc_fixup().
Can I ask what disadvantage would happen
if ecc_stats.failed / .corrected is incremented only by one,
where actually errors happen in multiple sectors.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v2 10/53] mtd: nand: denali: fix erased page checking
2017-03-24 2:43 ` Masahiro Yamada
@ 2017-03-24 8:06 ` Boris Brezillon
0 siblings, 0 replies; 47+ messages in thread
From: Boris Brezillon @ 2017-03-24 8:06 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-mtd, Laurent Monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Marek Vasut,
Brian Norris, Graham Moore, David Woodhouse, Masami Hiramatsu,
Chuanxiao Dong, Jassi Brar, Linux Kernel Mailing List,
Richard Weinberger, Cyrille Pitchen
On Fri, 24 Mar 2017 11:43:43 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> Hi Boris,
>
>
> 2017-03-23 16:56 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > On Thu, 23 Mar 2017 14:04:44 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >
> >> Hi Boris,
> >>
> >> 2017-03-23 5:56 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> >> > On Wed, 22 Mar 2017 23:07:17 +0900
> >> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >> >> dev_err(denali->dev,
> >> >> @@ -1148,12 +1136,15 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
> >> >> if (check_erased_page) {
> >> >> read_oob_data(mtd, chip->oob_poi, denali->page);
> >> >>
> >> >> - /* check ECC failures that may have occurred on erased pages */
> >> >> - if (check_erased_page) {
> >> >> - if (!is_erased(buf, mtd->writesize))
> >> >> - mtd->ecc_stats.failed++;
> >> >> - if (!is_erased(buf, mtd->oobsize))
> >> >> - mtd->ecc_stats.failed++;
> >> >> + stat = nand_check_erased_ecc_chunk(
> >> >> + buf, mtd->writesize,
> >> >> + chip->oob_poi, mtd->oobsize,
> >> >> + NULL, 0,
> >> >> + chip->ecc.strength * chip->ecc.steps);
> >> >
> >> > That's not how it's supposed to be done. Each chunk should be checked
> >> > independently. Here is a simple example explaining why this is
> >> > important:
> >> >
> >> > Let's consider the following setup:
> >> > - 4k pages
> >> > - 16bits/1024bytes ECC
> >> >
> >> > With your approach, you turn this into:
> >> > - 4k pages
> >> > - 64bits/4096bytes ECC
> >> >
> >> > Now suppose you have 32 bitflips in the first 1024 bytes. The real ECC
> >> > config is expected to report uncorrectable errors, but your approach
> >> > will just report that 32 bits have been fixed, which is wrong.
> >>
> >>
> >> OK. How about adding a helper like follows:
> >>
> >> static int denali_check_erased_page(struct mtd_info *mtd,
> >> struct nand_chip *chip, uint8_t *buf)
> >> {
> >> uint8_t *ecc_code = chip->buffers->ecccode;
> >> int ecc_steps = chip->ecc.steps;
> >> int ecc_size = chip->ecc.size;
> >> int ecc_bytes = chip->ecc.bytes;
> >> int i, ret;
> >>
> >> ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, chip->oob_poi, 0,
> >> chip->ecc.total);
> >> if (ret)
> >> return ret;
> >>
> >> for (i = 0; i < ecc_steps; i++) {
> >> ret = nand_check_erased_ecc_chunk(buf, ecc_size,
> >> ecc_code, ecc_bytes,
> >> NULL, 0,
> >> chip->ecc.strength);
> >> if (ret < 0)
> >> return ret;
> >> buf += ecc_size;
> >> ecc_code += ecc_bytes;
> >> }
> >>
> >> return 0;
> >> }
> >>
> >>
> >>
> >> Then,
> >>
> >> stat = denali_check_erased_page(mtd, chip, buf);
> >> if (stat < 0) {
> >> mtd->ecc_stats.failed++;
> >> /* return 0 for uncorrectable bitflips */
> >> stat = 0;
> >> }
> >
> > What's the point of checking all ECC chunks if only one contains ECC
> > errors? I really recommend to put the nand_check_erased_ecc_chunk()
> > call next to the per-ECC-block correction test.
>
>
> OK. I can fix it for software ECC fixup.
>
>
> What should I do for hardware ECC fixup case?
> http://patchwork.ozlabs.org/patch/742321/
>
>
> If at least one ECC sector fails to correct bit-flips,
> the controller sets INTR__ECC_UNCOR_ERR flag.
>
>
> In this case, we can not know the number of uncorrectable errors.
>
> Possible solutions are:
>
> - Increment ecc_stats.failed only by one (compromised solution)
Let's go for this solution.
>
>
> > Also, mtd->ecc_stats.failed is supposed to be incremented each time an
> > uncorrectable error is detected. In your denali_sw_ecc_fixup()
> > implementation you can detect errors at the ECC chunk level, so you
> > should increment ecc_stats.failed for each failure and not once if at
> > least one chunk is faulty.
>
>
> Yes, I can do this for denali_sw_ecc_fixup().
>
> Can I ask what disadvantage would happen
> if ecc_stats.failed / .corrected is incremented only by one,
> where actually errors happen in multiple sectors.
Reporting wrong stats, which is not such a big deal, but let's try to
keep them correct when we can (the SW ECC fixup case).
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v2 11/53] mtd: nand: denali: fix bitflips calculation in handle_ecc()
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
` (9 preceding siblings ...)
2017-03-22 14:07 ` [PATCH v2 10/53] mtd: nand: denali: fix erased page checking Masahiro Yamada
@ 2017-03-22 14:07 ` Masahiro Yamada
2017-03-22 20:57 ` Boris Brezillon
2017-03-22 14:07 ` [PATCH v2 12/53] mtd: nand: denali: support HW_ECC_FIXUP capability Masahiro Yamada
` (12 subsequent siblings)
23 siblings, 1 reply; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-22 14:07 UTC (permalink / raw)
To: linux-mtd
Cc: laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Boris Brezillon,
Marek Vasut, Brian Norris, Graham Moore, David Woodhouse,
Masami Hiramatsu, Chuanxiao Dong, Jassi Brar, Masahiro Yamada,
linux-kernel, Richard Weinberger, Cyrille Pitchen
This function is wrong in multiple ways:
[1] Counting corrected bytes instead of corrected bits.
The following code is counting the number of corrected _bytes_.
/* correct the ECC error */
buf[offset] ^= err_cor_value;
mtd->ecc_stats.corrected++;
bitflips++;
What the core framework expects is the number of corrected _bits_.
They can be different if multiple bitflips occur within one byte.
[2] total number of errors instead of max of per-sector errors
The core framework expects that corrected errors are counted per
sector, then the max value should be taken. The current code simply
iterates over the whole page, i.e. counts the total number of
correction in the page. This means "too many bitflips" is triggered
earlier than it should be, i.e. the NAND device is worn out sooner.
Besides those bugs, this function is unreadable due to the deep
nesting. Notice the whole code in this function is wrapped in
if (irq_status & INTR__ECC_ERR), so this conditional can be moved
out of the function. Also, use shorter names for local variables.
Re-work the function to fix all the issues.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2:
- Use shorter names for local variables.
- Fix bugs addressed by [1], [2]
drivers/mtd/nand/denali.c | 157 ++++++++++++++++++++++++----------------------
1 file changed, 82 insertions(+), 75 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 86381ac..608fe6f 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -888,80 +888,87 @@ static void read_oob_data(struct mtd_info *mtd, uint8_t *buf, int page)
#define ECC_SECTOR(x) (((x) & ECC_ERROR_ADDRESS__SECTOR_NR) >> 12)
#define ECC_BYTE(x) (((x) & ECC_ERROR_ADDRESS__OFFSET))
#define ECC_CORRECTION_VALUE(x) ((x) & ERR_CORRECTION_INFO__BYTEMASK)
-#define ECC_ERROR_CORRECTABLE(x) (!((x) & ERR_CORRECTION_INFO__ERROR_TYPE))
+#define ECC_ERROR_UNCORRECTABLE(x) ((x) & ERR_CORRECTION_INFO__ERROR_TYPE)
#define ECC_ERR_DEVICE(x) (((x) & ERR_CORRECTION_INFO__DEVICE_NR) >> 8)
#define ECC_LAST_ERR(x) ((x) & ERR_CORRECTION_INFO__LAST_ERR_INFO)
-static bool handle_ecc(struct denali_nand_info *denali, uint8_t *buf,
- uint32_t irq_status, unsigned int *max_bitflips)
+static int handle_ecc(struct mtd_info *mtd,
+ struct denali_nand_info *denali, uint8_t *buf)
{
- bool check_erased_page = false;
unsigned int bitflips = 0;
+ unsigned int max_bitflips = 0;
+ unsigned int total_bitflips = 0;
+ uint32_t err_addr, err_cor_info;
+ unsigned int err_byte, err_sector, err_device;
+ uint8_t err_cor_value;
+ unsigned int prev_sector = 0;
+ int ret = 0;
+
+ /* read the ECC errors. we'll ignore them for now */
+ denali_set_intr_modes(denali, false);
- if (irq_status & INTR__ECC_ERR) {
- /* read the ECC errors. we'll ignore them for now */
- uint32_t err_address, err_correction_info, err_byte,
- err_sector, err_device, err_correction_value;
- denali_set_intr_modes(denali, false);
-
- do {
- err_address = ioread32(denali->flash_reg +
- ECC_ERROR_ADDRESS);
- err_sector = ECC_SECTOR(err_address);
- err_byte = ECC_BYTE(err_address);
-
- err_correction_info = ioread32(denali->flash_reg +
- ERR_CORRECTION_INFO);
- err_correction_value =
- ECC_CORRECTION_VALUE(err_correction_info);
- err_device = ECC_ERR_DEVICE(err_correction_info);
-
- if (ECC_ERROR_CORRECTABLE(err_correction_info)) {
- /*
- * If err_byte is larger than ECC_SECTOR_SIZE,
- * means error happened in OOB, so we ignore
- * it. It's no need for us to correct it
- * err_device is represented the NAND error
- * bits are happened in if there are more
- * than one NAND connected.
- */
- if (err_byte < ECC_SECTOR_SIZE) {
- struct mtd_info *mtd =
- nand_to_mtd(&denali->nand);
- int offset;
-
- offset = (err_sector *
- ECC_SECTOR_SIZE +
- err_byte) *
- denali->devnum +
- err_device;
- /* correct the ECC error */
- buf[offset] ^= err_correction_value;
- mtd->ecc_stats.corrected++;
- bitflips++;
- }
- } else {
- /*
- * if the error is not correctable, need to
- * look at the page to see if it is an erased
- * page. if so, then it's not a real ECC error
- */
- check_erased_page = true;
- }
- } while (!ECC_LAST_ERR(err_correction_info));
- /*
- * Once handle all ecc errors, controller will triger
- * a ECC_TRANSACTION_DONE interrupt, so here just wait
- * for a while for this interrupt
- */
- while (!(read_interrupt_status(denali) &
- INTR__ECC_TRANSACTION_DONE))
- cpu_relax();
- clear_interrupts(denali);
- denali_set_intr_modes(denali, true);
- }
- *max_bitflips = bitflips;
- return check_erased_page;
+ do {
+ err_addr = ioread32(denali->flash_reg + ECC_ERROR_ADDRESS);
+ err_sector = ECC_SECTOR(err_addr);
+ err_byte = ECC_BYTE(err_addr);
+
+ err_cor_info = ioread32(denali->flash_reg + ERR_CORRECTION_INFO);
+ err_cor_value = ECC_CORRECTION_VALUE(err_cor_info);
+ err_device = ECC_ERR_DEVICE(err_cor_info);
+
+ /* reset the bitflip counter when crossing ECC sector */
+ if (err_sector != prev_sector)
+ bitflips = 0;
+
+ if (ECC_ERROR_UNCORRECTABLE(err_cor_info)) {
+ /*
+ * if the error is not correctable, need to look at the
+ * page to see if it is an erased page. if so, then
+ * it's not a real ECC error
+ */
+ ret = -EBADMSG;
+ } else if (err_byte < ECC_SECTOR_SIZE) {
+ /*
+ * If err_byte is larger than ECC_SECTOR_SIZE, means error
+ * happened in OOB, so we ignore it. It's no need for
+ * us to correct it err_device is represented the NAND
+ * error bits are happened in if there are more than
+ * one NAND connected.
+ */
+ int offset;
+ unsigned int flips_in_byte;
+
+ offset = (err_sector * ECC_SECTOR_SIZE + err_byte) *
+ denali->devnum + err_device;
+
+ /* correct the ECC error */
+ flips_in_byte = hweight8(buf[offset] ^ err_cor_value);
+ bitflips += flips_in_byte;
+ total_bitflips += flips_in_byte;
+ buf[offset] ^= err_cor_value;
+
+ max_bitflips = max(max_bitflips, bitflips);
+ }
+
+ prev_sector = err_sector;
+ } while (!ECC_LAST_ERR(err_cor_info));
+
+ /*
+ * Once handle all ecc errors, controller will trigger a
+ * ECC_TRANSACTION_DONE interrupt, so here just wait for
+ * a while for this interrupt
+ */
+ while (!(read_interrupt_status(denali) & INTR__ECC_TRANSACTION_DONE))
+ cpu_relax();
+ clear_interrupts(denali);
+ denali_set_intr_modes(denali, true);
+
+ if (ret)
+ return ret;
+
+ mtd->ecc_stats.corrected += total_bitflips;
+
+ return max_bitflips;
}
/* programs the controller to either enable/disable DMA transfers */
@@ -1097,7 +1104,6 @@ static int denali_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
uint8_t *buf, int oob_required, int page)
{
- unsigned int max_bitflips;
struct denali_nand_info *denali = mtd_to_denali(mtd);
dma_addr_t addr = denali->buf.dma_buf;
@@ -1105,8 +1111,7 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
uint32_t irq_status;
uint32_t irq_mask = INTR__ECC_TRANSACTION_DONE | INTR__ECC_ERR;
- bool check_erased_page = false;
- int stat;
+ int stat = 0;
if (page != denali->page) {
dev_err(denali->dev,
@@ -1130,10 +1135,11 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
memcpy(buf, denali->buf.buf, mtd->writesize);
- check_erased_page = handle_ecc(denali, buf, irq_status, &max_bitflips);
+ if (irq_status & INTR__ECC_ERR)
+ stat = handle_ecc(mtd, denali, buf);
denali_enable_dma(denali, false);
- if (check_erased_page) {
+ if (stat == -EBADMSG) {
read_oob_data(mtd, chip->oob_poi, denali->page);
stat = nand_check_erased_ecc_chunk(
@@ -1144,10 +1150,11 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
if (stat < 0) {
mtd->ecc_stats.failed++;
/* return 0 for uncorrectable bitflips */
- max_bitflips = 0;
+ stat = 0;
}
}
- return max_bitflips;
+
+ return stat;
}
static int denali_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v2 11/53] mtd: nand: denali: fix bitflips calculation in handle_ecc()
2017-03-22 14:07 ` [PATCH v2 11/53] mtd: nand: denali: fix bitflips calculation in handle_ecc() Masahiro Yamada
@ 2017-03-22 20:57 ` Boris Brezillon
2017-03-23 7:02 ` Masahiro Yamada
0 siblings, 1 reply; 47+ messages in thread
From: Boris Brezillon @ 2017-03-22 20:57 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-mtd, laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Marek Vasut,
Brian Norris, Graham Moore, David Woodhouse, Masami Hiramatsu,
Chuanxiao Dong, Jassi Brar, linux-kernel, Richard Weinberger,
Cyrille Pitchen
On Wed, 22 Mar 2017 23:07:18 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> + do {
> + err_addr = ioread32(denali->flash_reg + ECC_ERROR_ADDRESS);
> + err_sector = ECC_SECTOR(err_addr);
> + err_byte = ECC_BYTE(err_addr);
> +
> + err_cor_info = ioread32(denali->flash_reg + ERR_CORRECTION_INFO);
> + err_cor_value = ECC_CORRECTION_VALUE(err_cor_info);
> + err_device = ECC_ERR_DEVICE(err_cor_info);
> +
> + /* reset the bitflip counter when crossing ECC sector */
> + if (err_sector != prev_sector)
> + bitflips = 0;
> +
> + if (ECC_ERROR_UNCORRECTABLE(err_cor_info)) {
> + /*
> + * if the error is not correctable, need to look at the
> + * page to see if it is an erased page. if so, then
> + * it's not a real ECC error
> + */
> + ret = -EBADMSG;
You should never return -EBADMSG directly. Just increment
ecc_stats.failed and let the core return -EBADMSG to the upper layer.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v2 11/53] mtd: nand: denali: fix bitflips calculation in handle_ecc()
2017-03-22 20:57 ` Boris Brezillon
@ 2017-03-23 7:02 ` Masahiro Yamada
2017-03-23 8:12 ` Boris Brezillon
0 siblings, 1 reply; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-23 7:02 UTC (permalink / raw)
To: Boris Brezillon
Cc: linux-mtd, Laurent Monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Marek Vasut,
Brian Norris, Graham Moore, David Woodhouse, Masami Hiramatsu,
Chuanxiao Dong, Jassi Brar, Linux Kernel Mailing List,
Richard Weinberger, Cyrille Pitchen
Hi Boris,
2017-03-23 5:57 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Wed, 22 Mar 2017 23:07:18 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> + do {
>> + err_addr = ioread32(denali->flash_reg + ECC_ERROR_ADDRESS);
>> + err_sector = ECC_SECTOR(err_addr);
>> + err_byte = ECC_BYTE(err_addr);
>> +
>> + err_cor_info = ioread32(denali->flash_reg + ERR_CORRECTION_INFO);
>> + err_cor_value = ECC_CORRECTION_VALUE(err_cor_info);
>> + err_device = ECC_ERR_DEVICE(err_cor_info);
>> +
>> + /* reset the bitflip counter when crossing ECC sector */
>> + if (err_sector != prev_sector)
>> + bitflips = 0;
>> +
>> + if (ECC_ERROR_UNCORRECTABLE(err_cor_info)) {
>> + /*
>> + * if the error is not correctable, need to look at the
>> + * page to see if it is an erased page. if so, then
>> + * it's not a real ECC error
>> + */
>> + ret = -EBADMSG;
>
> You should never return -EBADMSG directly. Just increment
> ecc_stats.failed and let the core return -EBADMSG to the upper layer.
>
Here, -EBADMSG is used like that returned from ->ecc.correct()
Please notice denali_read_page() never returns -EBADMSG.
-EBADMSG is used as a mark "we need erased page check".
I think nand_read_page_syndrome() does similar;
-EBADMSG is used internally.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v2 11/53] mtd: nand: denali: fix bitflips calculation in handle_ecc()
2017-03-23 7:02 ` Masahiro Yamada
@ 2017-03-23 8:12 ` Boris Brezillon
0 siblings, 0 replies; 47+ messages in thread
From: Boris Brezillon @ 2017-03-23 8:12 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-mtd, Laurent Monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Marek Vasut,
Brian Norris, Graham Moore, David Woodhouse, Masami Hiramatsu,
Chuanxiao Dong, Jassi Brar, Linux Kernel Mailing List,
Richard Weinberger, Cyrille Pitchen
On Thu, 23 Mar 2017 16:02:02 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> Hi Boris,
>
> 2017-03-23 5:57 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > On Wed, 22 Mar 2017 23:07:18 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >
> >> + do {
> >> + err_addr = ioread32(denali->flash_reg + ECC_ERROR_ADDRESS);
> >> + err_sector = ECC_SECTOR(err_addr);
> >> + err_byte = ECC_BYTE(err_addr);
> >> +
> >> + err_cor_info = ioread32(denali->flash_reg + ERR_CORRECTION_INFO);
> >> + err_cor_value = ECC_CORRECTION_VALUE(err_cor_info);
> >> + err_device = ECC_ERR_DEVICE(err_cor_info);
> >> +
> >> + /* reset the bitflip counter when crossing ECC sector */
> >> + if (err_sector != prev_sector)
> >> + bitflips = 0;
> >> +
> >> + if (ECC_ERROR_UNCORRECTABLE(err_cor_info)) {
> >> + /*
> >> + * if the error is not correctable, need to look at the
> >> + * page to see if it is an erased page. if so, then
> >> + * it's not a real ECC error
> >> + */
> >> + ret = -EBADMSG;
> >
> > You should never return -EBADMSG directly. Just increment
> > ecc_stats.failed and let the core return -EBADMSG to the upper layer.
> >
>
> Here, -EBADMSG is used like that returned from ->ecc.correct()
>
>
> Please notice denali_read_page() never returns -EBADMSG.
>
> -EBADMSG is used as a mark "we need erased page check".
>
>
> I think nand_read_page_syndrome() does similar;
> -EBADMSG is used internally.
That's not exactly what happens. nand_read_page_syndrome() calls
ecc->correct() for each chunk, and if this method returns -EBADMSG (and
nand_check_erased_ecc_chunk() returns -EBADMSG too) it increments the
ecc_stats.failed counter.
Here you check all chunks in the same function and only increment
ecc_stats.failed once in denali_read_page() even if several chunks are
uncorrectable.
You handle_ecc() should act like nand_read_page_syndrome() WRT ECC
checking: check each block one by one, call
nand_check_erased_ecc_chunk() if needed, increment ecc_stats.failed
when an uncorrectable error is detected, and return max_bitflips at the
end.
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v2 12/53] mtd: nand: denali: support HW_ECC_FIXUP capability
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
` (10 preceding siblings ...)
2017-03-22 14:07 ` [PATCH v2 11/53] mtd: nand: denali: fix bitflips calculation in handle_ecc() Masahiro Yamada
@ 2017-03-22 14:07 ` Masahiro Yamada
2017-03-22 21:09 ` Boris Brezillon
2017-03-22 21:12 ` Boris Brezillon
2017-03-22 14:07 ` [PATCH v2 13/53] mtd: nand: denali_dt: enable HW_ECC_FIXUP for Altera SOCFPGA variant Masahiro Yamada
` (11 subsequent siblings)
23 siblings, 2 replies; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-22 14:07 UTC (permalink / raw)
To: linux-mtd
Cc: laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Boris Brezillon,
Marek Vasut, Brian Norris, Graham Moore, David Woodhouse,
Masami Hiramatsu, Chuanxiao Dong, Jassi Brar, Masahiro Yamada,
linux-kernel, Richard Weinberger, Cyrille Pitchen
Some old versions of the Denali IP (perhaps used only for Intel?)
detects ECC errors and provides correct data via a register, but
does not touch the transferred data. So, the software must fixup
the data in the buffer according to the provided ECC correction
information.
Newer versions perform ECC correction before transferring the data.
No more software intervention is needed. The ECC_ERROR_ADDRESS and
ECC_CORRECTION_INFO registers were deprecated. Instead, the number
of corrected bit-flips can be read from the ECC_COR_INFO register.
When an uncorrectable ECC error happens, a status flag is set to the
INTR_STATUS and ECC_COR_INFO registers.
As is often the case with this IP, the register view of INTR_STATUS
had broken compatibility.
For older versions (SW ECC fixup):
bit 0: ECC_TRANSACTION_DONE
bit 1: ECC_ERR
For newer versions (HW ECC fixup):
bit 0: ECC_UNCOR_ERR
bit 1: Reserved
Due to this difference, the irq_mask must be fixed too. The comment
block in handle_ecc() has been moved to the common part because the
comment applies to both cases. The existing handle_ecc() has been
renamed to denali_sw_ecc_fixup() for clarification.
The U-Boot port of this driver already supports the HW ECC fixup. I
borrowed the comment "Some versions of ..." in denali.h from U-Boot.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2:
- Change the capability prefix DENALI_CAPS_ -> DENALI_CAP_
drivers/mtd/nand/denali.c | 52 ++++++++++++++++++++++++++++++++++++-----------
drivers/mtd/nand/denali.h | 14 +++++++++++++
2 files changed, 54 insertions(+), 12 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 608fe6f..91f0def 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -883,6 +883,32 @@ static void read_oob_data(struct mtd_info *mtd, uint8_t *buf, int page)
}
}
+static int denali_hw_ecc_fixup(struct mtd_info *mtd,
+ struct denali_nand_info *denali)
+{
+ int bank = denali->flash_bank;
+ uint32_t ecc_cor;
+ unsigned int max_bitflips;
+
+ ecc_cor = ioread32(denali->flash_reg + ECC_COR_INFO(bank));
+ ecc_cor >>= ECC_COR_INFO__SHIFT(bank);
+
+ if (ecc_cor & ECC_COR_INFO__UNCOR_ERR)
+ return -EBADMSG;
+
+ max_bitflips = ecc_cor & ECC_COR_INFO__MAX_ERRORS;
+
+ /*
+ * The register holds the maximum of the number of corrected bitflips
+ * per sector. This can be returned from ecc->read_page() as-is.
+ * Unfortunately, we can not know the total number of corrected bits
+ * in the page. mtd->ecc_stats.corrected is compromised here.
+ */
+ mtd->ecc_stats.corrected += max_bitflips;
+
+ return max_bitflips;
+}
+
#define ECC_SECTOR_SIZE 512
#define ECC_SECTOR(x) (((x) & ECC_ERROR_ADDRESS__SECTOR_NR) >> 12)
@@ -892,8 +918,8 @@ static void read_oob_data(struct mtd_info *mtd, uint8_t *buf, int page)
#define ECC_ERR_DEVICE(x) (((x) & ERR_CORRECTION_INFO__DEVICE_NR) >> 8)
#define ECC_LAST_ERR(x) ((x) & ERR_CORRECTION_INFO__LAST_ERR_INFO)
-static int handle_ecc(struct mtd_info *mtd,
- struct denali_nand_info *denali, uint8_t *buf)
+static int denali_sw_ecc_fixup(struct mtd_info *mtd,
+ struct denali_nand_info *denali, uint8_t *buf)
{
unsigned int bitflips = 0;
unsigned int max_bitflips = 0;
@@ -921,11 +947,6 @@ static int handle_ecc(struct mtd_info *mtd,
bitflips = 0;
if (ECC_ERROR_UNCORRECTABLE(err_cor_info)) {
- /*
- * if the error is not correctable, need to look at the
- * page to see if it is an erased page. if so, then
- * it's not a real ECC error
- */
ret = -EBADMSG;
} else if (err_byte < ECC_SECTOR_SIZE) {
/*
@@ -1105,12 +1126,12 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
uint8_t *buf, int oob_required, int page)
{
struct denali_nand_info *denali = mtd_to_denali(mtd);
-
dma_addr_t addr = denali->buf.dma_buf;
size_t size = mtd->writesize + mtd->oobsize;
-
uint32_t irq_status;
- uint32_t irq_mask = INTR__ECC_TRANSACTION_DONE | INTR__ECC_ERR;
+ uint32_t irq_mask = denali->caps & DENALI_CAP_HW_ECC_FIXUP ?
+ INTR__DMA_CMD_COMP | INTR__ECC_UNCOR_ERR :
+ INTR__ECC_TRANSACTION_DONE | INTR__ECC_ERR;
int stat = 0;
if (page != denali->page) {
@@ -1135,11 +1156,18 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
memcpy(buf, denali->buf.buf, mtd->writesize);
- if (irq_status & INTR__ECC_ERR)
- stat = handle_ecc(mtd, denali, buf);
+ if (denali->caps & DENALI_CAP_HW_ECC_FIXUP)
+ stat = denali_hw_ecc_fixup(mtd, denali);
+ else if (irq_status & INTR__ECC_ERR)
+ stat = denali_sw_ecc_fixup(mtd, denali, buf);
denali_enable_dma(denali, false);
if (stat == -EBADMSG) {
+ /*
+ * If the error is not correctable, need to look at the page to
+ * see if it is an erased page. If so, then it's not a real ECC
+ * error.
+ */
read_oob_data(mtd, chip->oob_poi, denali->page);
stat = nand_check_erased_ecc_chunk(
diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h
index 7b2d785..ed42b16 100644
--- a/drivers/mtd/nand/denali.h
+++ b/drivers/mtd/nand/denali.h
@@ -20,6 +20,7 @@
#ifndef __DENALI_H__
#define __DENALI_H__
+#include <linux/bitops.h>
#include <linux/mtd/nand.h>
#define DEVICE_RESET 0x0
@@ -218,6 +219,13 @@
#define INTR_STATUS(__bank) (0x410 + ((__bank) * 0x50))
#define INTR_EN(__bank) (0x420 + ((__bank) * 0x50))
+/*
+ * Some versions of the IP have the ECC fixup handled in hardware. In this
+ * configuration we only get interrupted when the error is uncorrectable.
+ * Unfortunately this bit replaces INTR_STATUS__ECC_TRANSACTION_DONE from the
+ * old IP.
+ */
+#define INTR__ECC_UNCOR_ERR 0x0001
#define INTR__ECC_TRANSACTION_DONE 0x0001
#define INTR__ECC_ERR 0x0002
#define INTR__DMA_CMD_COMP 0x0004
@@ -259,6 +267,11 @@
#define ERR_CORRECTION_INFO__ERROR_TYPE 0x4000
#define ERR_CORRECTION_INFO__LAST_ERR_INFO 0x8000
+#define ECC_COR_INFO(bank) (0x650 + (bank) / 2 * 0x10)
+#define ECC_COR_INFO__SHIFT(bank) ((bank) % 2 * 8)
+#define ECC_COR_INFO__MAX_ERRORS 0x007f
+#define ECC_COR_INFO__UNCOR_ERR 0x0080
+
#define DMA_ENABLE 0x700
#define DMA_ENABLE__FLAG 0x0001
@@ -339,6 +352,7 @@ struct denali_nand_info {
int bbtskipbytes;
int max_banks;
unsigned int caps;
+#define DENALI_CAP_HW_ECC_FIXUP BIT(0)
};
extern int denali_init(struct denali_nand_info *denali);
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v2 12/53] mtd: nand: denali: support HW_ECC_FIXUP capability
2017-03-22 14:07 ` [PATCH v2 12/53] mtd: nand: denali: support HW_ECC_FIXUP capability Masahiro Yamada
@ 2017-03-22 21:09 ` Boris Brezillon
2017-03-23 7:06 ` Masahiro Yamada
2017-03-22 21:12 ` Boris Brezillon
1 sibling, 1 reply; 47+ messages in thread
From: Boris Brezillon @ 2017-03-22 21:09 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-mtd, laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Marek Vasut,
Brian Norris, Graham Moore, David Woodhouse, Masami Hiramatsu,
Chuanxiao Dong, Jassi Brar, linux-kernel, Richard Weinberger,
Cyrille Pitchen
On Wed, 22 Mar 2017 23:07:19 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> @@ -339,6 +352,7 @@ struct denali_nand_info {
> int bbtskipbytes;
> int max_banks;
> unsigned int caps;
> +#define DENALI_CAP_HW_ECC_FIXUP BIT(0)
Can you move this definition outside of the struct denali_nand_info
definition?
> };
>
> extern int denali_init(struct denali_nand_info *denali);
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v2 12/53] mtd: nand: denali: support HW_ECC_FIXUP capability
2017-03-22 21:09 ` Boris Brezillon
@ 2017-03-23 7:06 ` Masahiro Yamada
2017-03-23 8:16 ` Boris Brezillon
0 siblings, 1 reply; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-23 7:06 UTC (permalink / raw)
To: Boris Brezillon
Cc: linux-mtd, Laurent Monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Marek Vasut,
Brian Norris, Graham Moore, David Woodhouse, Masami Hiramatsu,
Chuanxiao Dong, Jassi Brar, Linux Kernel Mailing List,
Richard Weinberger, Cyrille Pitchen
Hi Boris,
2017-03-23 6:09 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Wed, 22 Mar 2017 23:07:19 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> @@ -339,6 +352,7 @@ struct denali_nand_info {
>> int bbtskipbytes;
>> int max_banks;
>> unsigned int caps;
>> +#define DENALI_CAP_HW_ECC_FIXUP BIT(0)
>
> Can you move this definition outside of the struct denali_nand_info
> definition?
My idea was to put relevant macros close to the "caps" member.
Actually, "struct mmc_host" does similar.
If you do not like it, yes, I can move it.
Anyway, this is a matter of taste,
and I do not have a strong opinion about this.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v2 12/53] mtd: nand: denali: support HW_ECC_FIXUP capability
2017-03-23 7:06 ` Masahiro Yamada
@ 2017-03-23 8:16 ` Boris Brezillon
0 siblings, 0 replies; 47+ messages in thread
From: Boris Brezillon @ 2017-03-23 8:16 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-mtd, Laurent Monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Marek Vasut,
Brian Norris, Graham Moore, David Woodhouse, Masami Hiramatsu,
Chuanxiao Dong, Jassi Brar, Linux Kernel Mailing List,
Richard Weinberger, Cyrille Pitchen
On Thu, 23 Mar 2017 16:06:19 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> Hi Boris,
>
> 2017-03-23 6:09 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > On Wed, 22 Mar 2017 23:07:19 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >
> >> @@ -339,6 +352,7 @@ struct denali_nand_info {
> >> int bbtskipbytes;
> >> int max_banks;
> >> unsigned int caps;
> >> +#define DENALI_CAP_HW_ECC_FIXUP BIT(0)
> >
> > Can you move this definition outside of the struct denali_nand_info
> > definition?
>
>
> My idea was to put relevant macros close to the "caps" member.
> Actually, "struct mmc_host" does similar.
>
>
> If you do not like it, yes, I can move it.
> Anyway, this is a matter of taste,
> and I do not have a strong opinion about this.
>
>
Indeed, I prefer when macro and struct definitions are clearly
separated.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 12/53] mtd: nand: denali: support HW_ECC_FIXUP capability
2017-03-22 14:07 ` [PATCH v2 12/53] mtd: nand: denali: support HW_ECC_FIXUP capability Masahiro Yamada
2017-03-22 21:09 ` Boris Brezillon
@ 2017-03-22 21:12 ` Boris Brezillon
2017-03-23 7:05 ` Masahiro Yamada
1 sibling, 1 reply; 47+ messages in thread
From: Boris Brezillon @ 2017-03-22 21:12 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-mtd, laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Marek Vasut,
Brian Norris, Graham Moore, David Woodhouse, Masami Hiramatsu,
Chuanxiao Dong, Jassi Brar, linux-kernel, Richard Weinberger,
Cyrille Pitchen
On Wed, 22 Mar 2017 23:07:19 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> +static int denali_hw_ecc_fixup(struct mtd_info *mtd,
> + struct denali_nand_info *denali)
> +{
> + int bank = denali->flash_bank;
> + uint32_t ecc_cor;
> + unsigned int max_bitflips;
> +
> + ecc_cor = ioread32(denali->flash_reg + ECC_COR_INFO(bank));
> + ecc_cor >>= ECC_COR_INFO__SHIFT(bank);
> +
> + if (ecc_cor & ECC_COR_INFO__UNCOR_ERR)
> + return -EBADMSG;
As previously mentioned, just increment ecc_stats.failed.
> +
> + max_bitflips = ecc_cor & ECC_COR_INFO__MAX_ERRORS;
> +
> + /*
> + * The register holds the maximum of the number of corrected bitflips
> + * per sector. This can be returned from ecc->read_page() as-is.
> + * Unfortunately, we can not know the total number of corrected bits
> + * in the page. mtd->ecc_stats.corrected is compromised here.
> + */
> + mtd->ecc_stats.corrected += max_bitflips;
> +
> + return max_bitflips;
> +}
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v2 12/53] mtd: nand: denali: support HW_ECC_FIXUP capability
2017-03-22 21:12 ` Boris Brezillon
@ 2017-03-23 7:05 ` Masahiro Yamada
0 siblings, 0 replies; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-23 7:05 UTC (permalink / raw)
To: Boris Brezillon
Cc: linux-mtd, Laurent Monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Marek Vasut,
Brian Norris, Graham Moore, David Woodhouse, Masami Hiramatsu,
Chuanxiao Dong, Jassi Brar, Linux Kernel Mailing List,
Richard Weinberger, Cyrille Pitchen
Hi Boris,
2017-03-23 6:12 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Wed, 22 Mar 2017 23:07:19 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> +static int denali_hw_ecc_fixup(struct mtd_info *mtd,
>> + struct denali_nand_info *denali)
>> +{
>> + int bank = denali->flash_bank;
>> + uint32_t ecc_cor;
>> + unsigned int max_bitflips;
>> +
>> + ecc_cor = ioread32(denali->flash_reg + ECC_COR_INFO(bank));
>> + ecc_cor >>= ECC_COR_INFO__SHIFT(bank);
>> +
>> + if (ecc_cor & ECC_COR_INFO__UNCOR_ERR)
>> + return -EBADMSG;
>
> As previously mentioned, just increment ecc_stats.failed.
>
I explained the intention of -EBADMSG in 11/53.
ecc_stats.failed is incremented in denali_read_page().
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v2 13/53] mtd: nand: denali_dt: enable HW_ECC_FIXUP for Altera SOCFPGA variant
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
` (11 preceding siblings ...)
2017-03-22 14:07 ` [PATCH v2 12/53] mtd: nand: denali: support HW_ECC_FIXUP capability Masahiro Yamada
@ 2017-03-22 14:07 ` Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 14/53] mtd: nand: denali: support 64bit capable DMA engine Masahiro Yamada
` (10 subsequent siblings)
23 siblings, 0 replies; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-22 14:07 UTC (permalink / raw)
To: linux-mtd
Cc: laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Boris Brezillon,
Marek Vasut, Brian Norris, Graham Moore, David Woodhouse,
Masami Hiramatsu, Chuanxiao Dong, Jassi Brar, Masahiro Yamada,
devicetree, linux-kernel, Richard Weinberger, Cyrille Pitchen,
Rob Herring, Mark Rutland
There are various customizable parameters, so several variants for
this IP. A generic compatible like "denali,denali-nand-dt" is
useless. Moreover, there are multiple things wrong with this string.
(Refer to Rob's comment [1])
The denali_dt.c was split out and this compatible was added by Altera
for the SOCFPGA port. So, replace it with a more specific string.
The Denali IP on SOCFPGA incorporates the hardware ECC fixup feature.
So, this capability should be associated with the compatible string.
[1] https://lkml.org/lkml/2016/12/1/450
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2:
- Add caps flag to of_device_id data, not hard-code in driver code
- Add Altera-specific compatible.
Documentation/devicetree/bindings/mtd/denali-nand.txt | 5 +++--
drivers/mtd/nand/denali_dt.c | 14 ++++++++++----
2 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
index b04d03a..6f4ab4c 100644
--- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
@@ -1,7 +1,8 @@
* Denali NAND controller
Required properties:
- - compatible : should be "denali,denali-nand-dt"
+ - compatible : should be one of the following:
+ "altr,socfpga-denali-nand" - for Altera SOCFPGA
- reg : should contain registers location and length for data and reg.
- reg-names: Should contain the reg names "nand_data" and "denali_reg"
- interrupts : The interrupt number.
@@ -15,7 +16,7 @@ Examples:
nand: nand@ff900000 {
#address-cells = <1>;
#size-cells = <1>;
- compatible = "denali,denali-nand-dt";
+ compatible = "altr,socfpga-denali-nand";
reg = <0xff900000 0x100000>, <0xffb80000 0x10000>;
reg-names = "nand_data", "denali_reg";
interrupts = <0 144 4>;
diff --git a/drivers/mtd/nand/denali_dt.c b/drivers/mtd/nand/denali_dt.c
index 293ddb8..9577bfd 100644
--- a/drivers/mtd/nand/denali_dt.c
+++ b/drivers/mtd/nand/denali_dt.c
@@ -33,11 +33,17 @@ struct denali_dt_data {
unsigned int caps;
};
-static const struct of_device_id denali_nand_dt_ids[] = {
- { .compatible = "denali,denali-nand-dt" },
- { /* sentinel */ }
- };
+static const struct denali_dt_data denali_socfpga_data = {
+ .caps = DENALI_CAP_HW_ECC_FIXUP,
+};
+static const struct of_device_id denali_nand_dt_ids[] = {
+ {
+ .compatible = "altr,socfpga-denali-nand",
+ .data = &denali_socfpga_data,
+ },
+ { /* sentinel */ }
+};
MODULE_DEVICE_TABLE(of, denali_nand_dt_ids);
static u64 denali_dma_mask;
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [PATCH v2 14/53] mtd: nand: denali: support 64bit capable DMA engine
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
` (12 preceding siblings ...)
2017-03-22 14:07 ` [PATCH v2 13/53] mtd: nand: denali_dt: enable HW_ECC_FIXUP for Altera SOCFPGA variant Masahiro Yamada
@ 2017-03-22 14:07 ` Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 15/53] mtd: nand: denali_dt: remove dma-mask DT property Masahiro Yamada
` (9 subsequent siblings)
23 siblings, 0 replies; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-22 14:07 UTC (permalink / raw)
To: linux-mtd
Cc: laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Boris Brezillon,
Marek Vasut, Brian Norris, Graham Moore, David Woodhouse,
Masami Hiramatsu, Chuanxiao Dong, Jassi Brar, Masahiro Yamada,
linux-kernel, Richard Weinberger, Cyrille Pitchen
The current driver only supports the DMA engine up to 32 bit
physical address, but there also exists 64 bit capable DMA engine
for this IP.
The data DMA setup sequence is completely different, so I added the
64 bit DMA code as a new function denali_setup_dma64(). The 32 bit
one has been renamed to denali_setup_dma32().
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2:
- Change the capability prefix DENALI_CAPS_ -> DENALI_CAP_
drivers/mtd/nand/denali.c | 39 +++++++++++++++++++++++++++++++++++----
drivers/mtd/nand/denali.h | 1 +
2 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 91f0def..513292f 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -999,8 +999,30 @@ static void denali_enable_dma(struct denali_nand_info *denali, bool en)
ioread32(denali->flash_reg + DMA_ENABLE);
}
-/* setups the HW to perform the data DMA */
-static void denali_setup_dma(struct denali_nand_info *denali, int op)
+static void denali_setup_dma64(struct denali_nand_info *denali, int op)
+{
+ uint32_t mode;
+ const int page_count = 1;
+ uint64_t addr = denali->buf.dma_buf;
+
+ mode = MODE_10 | BANK(denali->flash_bank) | denali->page;
+
+ /* DMA is a three step process */
+
+ /*
+ * 1. setup transfer type, interrupt when complete,
+ * burst len = 64 bytes, the number of pages
+ */
+ index_addr(denali, mode, 0x01002000 | (64 << 16) | op | page_count);
+
+ /* 2. set memory low address */
+ index_addr(denali, mode, addr);
+
+ /* 3. set memory high address */
+ index_addr(denali, mode, addr >> 32);
+}
+
+static void denali_setup_dma32(struct denali_nand_info *denali, int op)
{
uint32_t mode;
const int page_count = 1;
@@ -1023,6 +1045,14 @@ static void denali_setup_dma(struct denali_nand_info *denali, int op)
index_addr(denali, mode | 0x14000, 0x2400);
}
+static void denali_setup_dma(struct denali_nand_info *denali, int op)
+{
+ if (denali->caps & DENALI_CAP_DMA_64BIT)
+ denali_setup_dma64(denali, op);
+ else
+ denali_setup_dma32(denali, op);
+}
+
/*
* writes a page. user specifies type, and this function handles the
* configuration details.
@@ -1492,8 +1522,9 @@ int denali_init(struct denali_nand_info *denali)
goto failed_req_irq;
}
- /* Is 32-bit DMA supported? */
- ret = dma_set_mask(denali->dev, DMA_BIT_MASK(32));
+ ret = dma_set_mask(denali->dev,
+ DMA_BIT_MASK(denali->caps & DENALI_CAP_DMA_64BIT ?
+ 64 : 32));
if (ret) {
dev_err(denali->dev, "No usable DMA configuration\n");
goto failed_req_irq;
diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h
index ed42b16..a8e8d68 100644
--- a/drivers/mtd/nand/denali.h
+++ b/drivers/mtd/nand/denali.h
@@ -353,6 +353,7 @@ struct denali_nand_info {
int max_banks;
unsigned int caps;
#define DENALI_CAP_HW_ECC_FIXUP BIT(0)
+#define DENALI_CAP_DMA_64BIT BIT(1)
};
extern int denali_init(struct denali_nand_info *denali);
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [PATCH v2 15/53] mtd: nand: denali_dt: remove dma-mask DT property
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
` (13 preceding siblings ...)
2017-03-22 14:07 ` [PATCH v2 14/53] mtd: nand: denali: support 64bit capable DMA engine Masahiro Yamada
@ 2017-03-22 14:07 ` Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 16/53] mtd: nand: denali_dt: use pdev instead of ofdev for platform_device Masahiro Yamada
` (8 subsequent siblings)
23 siblings, 0 replies; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-22 14:07 UTC (permalink / raw)
To: linux-mtd
Cc: laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Boris Brezillon,
Marek Vasut, Brian Norris, Graham Moore, David Woodhouse,
Masami Hiramatsu, Chuanxiao Dong, Jassi Brar, Masahiro Yamada,
devicetree, linux-kernel, Richard Weinberger, Cyrille Pitchen,
Rob Herring, Mark Rutland
The driver sets appropriate DMA mask. Delete the "dma-mask" DT
property. See [1] for negative comments for this binding.
[1] https://lkml.org/lkml/2016/2/8/57
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Acked-by: Rob Herring <robh@kernel.org>
---
Changes in v2: None
Documentation/devicetree/bindings/mtd/denali-nand.txt | 2 --
drivers/mtd/nand/denali_dt.c | 9 ---------
2 files changed, 11 deletions(-)
diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
index 6f4ab4c..e593bbe 100644
--- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
@@ -6,7 +6,6 @@ Required properties:
- reg : should contain registers location and length for data and reg.
- reg-names: Should contain the reg names "nand_data" and "denali_reg"
- interrupts : The interrupt number.
- - dm-mask : DMA bit mask
The device tree may optionally contain sub-nodes describing partitions of the
address space. See partition.txt for more detail.
@@ -20,5 +19,4 @@ nand: nand@ff900000 {
reg = <0xff900000 0x100000>, <0xffb80000 0x10000>;
reg-names = "nand_data", "denali_reg";
interrupts = <0 144 4>;
- dma-mask = <0xffffffff>;
};
diff --git a/drivers/mtd/nand/denali_dt.c b/drivers/mtd/nand/denali_dt.c
index 9577bfd..b8a8284 100644
--- a/drivers/mtd/nand/denali_dt.c
+++ b/drivers/mtd/nand/denali_dt.c
@@ -46,8 +46,6 @@ static const struct of_device_id denali_nand_dt_ids[] = {
};
MODULE_DEVICE_TABLE(of, denali_nand_dt_ids);
-static u64 denali_dma_mask;
-
static int denali_dt_probe(struct platform_device *ofdev)
{
struct resource *denali_reg, *nand_data;
@@ -83,13 +81,6 @@ static int denali_dt_probe(struct platform_device *ofdev)
if (IS_ERR(denali->flash_mem))
return PTR_ERR(denali->flash_mem);
- if (!of_property_read_u32(ofdev->dev.of_node,
- "dma-mask", (u32 *)&denali_dma_mask)) {
- denali->dev->dma_mask = &denali_dma_mask;
- } else {
- denali->dev->dma_mask = NULL;
- }
-
dt->clk = devm_clk_get(&ofdev->dev, NULL);
if (IS_ERR(dt->clk)) {
dev_err(&ofdev->dev, "no clk available\n");
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [PATCH v2 16/53] mtd: nand: denali_dt: use pdev instead of ofdev for platform_device
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
` (14 preceding siblings ...)
2017-03-22 14:07 ` [PATCH v2 15/53] mtd: nand: denali_dt: remove dma-mask DT property Masahiro Yamada
@ 2017-03-22 14:07 ` Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 17/53] mtd: nand: denali: allow to override revision number Masahiro Yamada
` (7 subsequent siblings)
23 siblings, 0 replies; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-22 14:07 UTC (permalink / raw)
To: linux-mtd
Cc: laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Boris Brezillon,
Marek Vasut, Brian Norris, Graham Moore, David Woodhouse,
Masami Hiramatsu, Chuanxiao Dong, Jassi Brar, Masahiro Yamada,
linux-kernel, Richard Weinberger, Cyrille Pitchen
"pdev" is much more often used to point a platform_device, so this
will help the driver code look consistent across the kernel.
While we are here, fix "line over 80 characters" coding style
violations.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2: None
drivers/mtd/nand/denali_dt.c | 32 +++++++++++++++++---------------
1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/drivers/mtd/nand/denali_dt.c b/drivers/mtd/nand/denali_dt.c
index b8a8284..ada3863 100644
--- a/drivers/mtd/nand/denali_dt.c
+++ b/drivers/mtd/nand/denali_dt.c
@@ -46,7 +46,7 @@ static const struct of_device_id denali_nand_dt_ids[] = {
};
MODULE_DEVICE_TABLE(of, denali_nand_dt_ids);
-static int denali_dt_probe(struct platform_device *ofdev)
+static int denali_dt_probe(struct platform_device *pdev)
{
struct resource *denali_reg, *nand_data;
struct denali_dt *dt;
@@ -54,36 +54,38 @@ static int denali_dt_probe(struct platform_device *ofdev)
struct denali_nand_info *denali;
int ret;
- dt = devm_kzalloc(&ofdev->dev, sizeof(*dt), GFP_KERNEL);
+ dt = devm_kzalloc(&pdev->dev, sizeof(*dt), GFP_KERNEL);
if (!dt)
return -ENOMEM;
denali = &dt->denali;
- data = of_device_get_match_data(&ofdev->dev);
+ data = of_device_get_match_data(&pdev->dev);
if (data)
denali->caps = data->caps;
denali->platform = DT;
- denali->dev = &ofdev->dev;
- denali->irq = platform_get_irq(ofdev, 0);
+ denali->dev = &pdev->dev;
+ denali->irq = platform_get_irq(pdev, 0);
if (denali->irq < 0) {
- dev_err(&ofdev->dev, "no irq defined\n");
+ dev_err(&pdev->dev, "no irq defined\n");
return denali->irq;
}
- denali_reg = platform_get_resource_byname(ofdev, IORESOURCE_MEM, "denali_reg");
- denali->flash_reg = devm_ioremap_resource(&ofdev->dev, denali_reg);
+ denali_reg = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+ "denali_reg");
+ denali->flash_reg = devm_ioremap_resource(&pdev->dev, denali_reg);
if (IS_ERR(denali->flash_reg))
return PTR_ERR(denali->flash_reg);
- nand_data = platform_get_resource_byname(ofdev, IORESOURCE_MEM, "nand_data");
- denali->flash_mem = devm_ioremap_resource(&ofdev->dev, nand_data);
+ nand_data = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+ "nand_data");
+ denali->flash_mem = devm_ioremap_resource(&pdev->dev, nand_data);
if (IS_ERR(denali->flash_mem))
return PTR_ERR(denali->flash_mem);
- dt->clk = devm_clk_get(&ofdev->dev, NULL);
+ dt->clk = devm_clk_get(&pdev->dev, NULL);
if (IS_ERR(dt->clk)) {
- dev_err(&ofdev->dev, "no clk available\n");
+ dev_err(&pdev->dev, "no clk available\n");
return PTR_ERR(dt->clk);
}
clk_prepare_enable(dt->clk);
@@ -92,7 +94,7 @@ static int denali_dt_probe(struct platform_device *ofdev)
if (ret)
goto out_disable_clk;
- platform_set_drvdata(ofdev, dt);
+ platform_set_drvdata(pdev, dt);
return 0;
out_disable_clk:
@@ -101,9 +103,9 @@ static int denali_dt_probe(struct platform_device *ofdev)
return ret;
}
-static int denali_dt_remove(struct platform_device *ofdev)
+static int denali_dt_remove(struct platform_device *pdev)
{
- struct denali_dt *dt = platform_get_drvdata(ofdev);
+ struct denali_dt *dt = platform_get_drvdata(pdev);
denali_remove(&dt->denali);
clk_disable_unprepare(dt->clk);
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [PATCH v2 17/53] mtd: nand: denali: allow to override revision number
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
` (15 preceding siblings ...)
2017-03-22 14:07 ` [PATCH v2 16/53] mtd: nand: denali_dt: use pdev instead of ofdev for platform_device Masahiro Yamada
@ 2017-03-22 14:07 ` Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 18/53] mtd: nand: denali: use nand_chip to hold frequently accessed data Masahiro Yamada
` (6 subsequent siblings)
23 siblings, 0 replies; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-22 14:07 UTC (permalink / raw)
To: linux-mtd
Cc: laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Boris Brezillon,
Marek Vasut, Brian Norris, Graham Moore, David Woodhouse,
Masami Hiramatsu, Chuanxiao Dong, Jassi Brar, Masahiro Yamada,
linux-kernel, Richard Weinberger, Cyrille Pitchen
Commit 271707b1d817 ("mtd: nand: denali: max_banks calculation
changed in revision 5.1") added a revision check to support the
new max_banks encoding. Its git-log states "The encoding of
max_banks changed in Denali revision 5.1".
There are exceptional cases, for example, the revision register on
some UniPhier SoCs says the IP is 5.0 but the max_banks is encoded
in the new format.
This IP updates the resister specification from time to time (often
breaking the backward compatibility), but the revision number is not
incremented correctly.
The max_banks is not only the case that needs revision checking.
Let's allow to override an incorrect revision number.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2:
- Replace the NEW_N_BANKS_FORMAT approach
drivers/mtd/nand/denali.c | 19 +++++++++----------
drivers/mtd/nand/denali.h | 3 +--
drivers/mtd/nand/denali_dt.c | 5 ++++-
3 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 513292f..ff6af1d 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -449,17 +449,12 @@ static void find_valid_banks(struct denali_nand_info *denali)
static void detect_max_banks(struct denali_nand_info *denali)
{
uint32_t features = ioread32(denali->flash_reg + FEATURES);
- /*
- * Read the revision register, so we can calculate the max_banks
- * properly: the encoding changed from rev 5.0 to 5.1
- */
- u32 revision = MAKE_COMPARABLE_REVISION(
- ioread32(denali->flash_reg + REVISION));
- if (revision < REVISION_5_1)
- denali->max_banks = 2 << (features & FEATURES__N_BANKS);
- else
- denali->max_banks = 1 << (features & FEATURES__N_BANKS);
+ denali->max_banks = 1 << (features & FEATURES__N_BANKS);
+
+ /* the encoding changed from rev 5.0 to 5.1 */
+ if (denali->revision < 0x0501)
+ denali->max_banks <<= 1;
}
static uint16_t denali_nand_timing_set(struct denali_nand_info *denali)
@@ -1348,6 +1343,10 @@ static void denali_cmdfunc(struct mtd_info *mtd, unsigned int cmd, int col,
/* Initialization code to bring the device up to a known good state */
static void denali_hw_init(struct denali_nand_info *denali)
{
+ if (!denali->revision)
+ denali->revision =
+ swab16(ioread32(denali->flash_reg + REVISION));
+
/*
* tell driver how many bit controller will skip before
* writing ECC code in OOB, this register may be already
diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h
index a8e8d68..58a8042 100644
--- a/drivers/mtd/nand/denali.h
+++ b/drivers/mtd/nand/denali.h
@@ -179,8 +179,6 @@
#define REVISION 0x370
#define REVISION__VALUE 0xffff
-#define MAKE_COMPARABLE_REVISION(x) swab16((x) & REVISION__VALUE)
-#define REVISION_5_1 0x00000501
#define ONFI_DEVICE_FEATURES 0x380
#define ONFI_DEVICE_FEATURES__VALUE 0x003f
@@ -351,6 +349,7 @@ struct denali_nand_info {
int devnum; /* represent how many nands connected */
int bbtskipbytes;
int max_banks;
+ unsigned int revision;
unsigned int caps;
#define DENALI_CAP_HW_ECC_FIXUP BIT(0)
#define DENALI_CAP_DMA_64BIT BIT(1)
diff --git a/drivers/mtd/nand/denali_dt.c b/drivers/mtd/nand/denali_dt.c
index ada3863..df9ef36 100644
--- a/drivers/mtd/nand/denali_dt.c
+++ b/drivers/mtd/nand/denali_dt.c
@@ -30,6 +30,7 @@ struct denali_dt {
};
struct denali_dt_data {
+ unsigned int revision;
unsigned int caps;
};
@@ -60,8 +61,10 @@ static int denali_dt_probe(struct platform_device *pdev)
denali = &dt->denali;
data = of_device_get_match_data(&pdev->dev);
- if (data)
+ if (data) {
+ denali->revision = data->revision;
denali->caps = data->caps;
+ }
denali->platform = DT;
denali->dev = &pdev->dev;
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [PATCH v2 18/53] mtd: nand: denali: use nand_chip to hold frequently accessed data
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
` (16 preceding siblings ...)
2017-03-22 14:07 ` [PATCH v2 17/53] mtd: nand: denali: allow to override revision number Masahiro Yamada
@ 2017-03-22 14:07 ` Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 19/53] mtd: nand: denali: call nand_set_flash_node() to set DT node Masahiro Yamada
` (5 subsequent siblings)
23 siblings, 0 replies; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-22 14:07 UTC (permalink / raw)
To: linux-mtd
Cc: laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Boris Brezillon,
Marek Vasut, Brian Norris, Graham Moore, David Woodhouse,
Masami Hiramatsu, Chuanxiao Dong, Jassi Brar, Masahiro Yamada,
linux-kernel, Richard Weinberger, Cyrille Pitchen
The denali_init() needs to setup a bunch of parameters of nand_chip.
Replace denali->nand.(member) with chip->(member) for shorter code.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2: None
drivers/mtd/nand/denali.c | 68 +++++++++++++++++++++++------------------------
1 file changed, 34 insertions(+), 34 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index ff6af1d..24c35a6 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1460,7 +1460,8 @@ static void denali_drv_init(struct denali_nand_info *denali)
int denali_init(struct denali_nand_info *denali)
{
- struct mtd_info *mtd = nand_to_mtd(&denali->nand);
+ struct nand_chip *chip = &denali->nand;
+ struct mtd_info *mtd = nand_to_mtd(chip);
int ret;
if (denali->platform == INTEL_CE4100) {
@@ -1497,10 +1498,10 @@ int denali_init(struct denali_nand_info *denali)
mtd->name = "denali-nand";
/* register the driver with the NAND core subsystem */
- denali->nand.select_chip = denali_select_chip;
- denali->nand.cmdfunc = denali_cmdfunc;
- denali->nand.read_byte = denali_read_byte;
- denali->nand.waitfunc = denali_waitfunc;
+ chip->select_chip = denali_select_chip;
+ chip->cmdfunc = denali_cmdfunc;
+ chip->read_byte = denali_read_byte;
+ chip->waitfunc = denali_waitfunc;
/*
* scan for NAND devices attached to the controller
@@ -1544,17 +1545,16 @@ int denali_init(struct denali_nand_info *denali)
* the real pagesize and anything necessery
*/
denali->devnum = ioread32(denali->flash_reg + DEVICES_CONNECTED);
- denali->nand.chipsize <<= denali->devnum - 1;
- denali->nand.page_shift += denali->devnum - 1;
- denali->nand.pagemask = (denali->nand.chipsize >>
- denali->nand.page_shift) - 1;
- denali->nand.bbt_erase_shift += denali->devnum - 1;
- denali->nand.phys_erase_shift = denali->nand.bbt_erase_shift;
- denali->nand.chip_shift += denali->devnum - 1;
+ chip->chipsize <<= denali->devnum - 1;
+ chip->page_shift += denali->devnum - 1;
+ chip->pagemask = (chip->chipsize >> chip->page_shift) - 1;
+ chip->bbt_erase_shift += denali->devnum - 1;
+ chip->phys_erase_shift = chip->bbt_erase_shift;
+ chip->chip_shift += denali->devnum - 1;
mtd->writesize <<= denali->devnum - 1;
mtd->oobsize <<= denali->devnum - 1;
mtd->erasesize <<= denali->devnum - 1;
- mtd->size = denali->nand.numchips * denali->nand.chipsize;
+ mtd->size = chip->numchips * chip->chipsize;
denali->bbtskipbytes *= denali->devnum;
/*
@@ -1564,29 +1564,29 @@ int denali_init(struct denali_nand_info *denali)
*/
/* Bad block management */
- denali->nand.bbt_td = &bbt_main_descr;
- denali->nand.bbt_md = &bbt_mirror_descr;
+ chip->bbt_td = &bbt_main_descr;
+ chip->bbt_md = &bbt_mirror_descr;
/* skip the scan for now until we have OOB read and write support */
- denali->nand.bbt_options |= NAND_BBT_USE_FLASH;
- denali->nand.options |= NAND_SKIP_BBTSCAN;
- denali->nand.ecc.mode = NAND_ECC_HW_SYNDROME;
+ chip->bbt_options |= NAND_BBT_USE_FLASH;
+ chip->options |= NAND_SKIP_BBTSCAN;
+ chip->ecc.mode = NAND_ECC_HW_SYNDROME;
/* no subpage writes on denali */
- denali->nand.options |= NAND_NO_SUBPAGE_WRITE;
+ chip->options |= NAND_NO_SUBPAGE_WRITE;
/*
* Denali Controller only support 15bit and 8bit ECC in MRST,
* so just let controller do 15bit ECC for MLC and 8bit ECC for
* SLC if possible.
* */
- if (!nand_is_slc(&denali->nand) &&
+ if (!nand_is_slc(chip) &&
(mtd->oobsize > (denali->bbtskipbytes +
ECC_15BITS * (mtd->writesize /
ECC_SECTOR_SIZE)))) {
/* if MLC OOB size is large enough, use 15bit ECC*/
- denali->nand.ecc.strength = 15;
- denali->nand.ecc.bytes = ECC_15BITS;
+ chip->ecc.strength = 15;
+ chip->ecc.bytes = ECC_15BITS;
iowrite32(15, denali->flash_reg + ECC_CORRECTION);
} else if (mtd->oobsize < (denali->bbtskipbytes +
ECC_8BITS * (mtd->writesize /
@@ -1594,24 +1594,24 @@ int denali_init(struct denali_nand_info *denali)
pr_err("Your NAND chip OOB is not large enough to contain 8bit ECC correction codes");
goto failed_req_irq;
} else {
- denali->nand.ecc.strength = 8;
- denali->nand.ecc.bytes = ECC_8BITS;
+ chip->ecc.strength = 8;
+ chip->ecc.bytes = ECC_8BITS;
iowrite32(8, denali->flash_reg + ECC_CORRECTION);
}
mtd_set_ooblayout(mtd, &denali_ooblayout_ops);
- denali->nand.ecc.bytes *= denali->devnum;
- denali->nand.ecc.strength *= denali->devnum;
+ chip->ecc.bytes *= denali->devnum;
+ chip->ecc.strength *= denali->devnum;
/* override the default read operations */
- denali->nand.ecc.size = ECC_SECTOR_SIZE * denali->devnum;
- denali->nand.ecc.read_page = denali_read_page;
- denali->nand.ecc.read_page_raw = denali_read_page_raw;
- denali->nand.ecc.write_page = denali_write_page;
- denali->nand.ecc.write_page_raw = denali_write_page_raw;
- denali->nand.ecc.read_oob = denali_read_oob;
- denali->nand.ecc.write_oob = denali_write_oob;
- denali->nand.erase = denali_erase;
+ chip->ecc.size = ECC_SECTOR_SIZE * denali->devnum;
+ chip->ecc.read_page = denali_read_page;
+ chip->ecc.read_page_raw = denali_read_page_raw;
+ chip->ecc.write_page = denali_write_page;
+ chip->ecc.write_page_raw = denali_write_page_raw;
+ chip->ecc.read_oob = denali_read_oob;
+ chip->ecc.write_oob = denali_write_oob;
+ chip->erase = denali_erase;
ret = nand_scan_tail(mtd);
if (ret)
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [PATCH v2 19/53] mtd: nand: denali: call nand_set_flash_node() to set DT node
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
` (17 preceding siblings ...)
2017-03-22 14:07 ` [PATCH v2 18/53] mtd: nand: denali: use nand_chip to hold frequently accessed data Masahiro Yamada
@ 2017-03-22 14:07 ` Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 20/53] mtd: nand: denali: do not set mtd->name Masahiro Yamada
` (4 subsequent siblings)
23 siblings, 0 replies; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-22 14:07 UTC (permalink / raw)
To: linux-mtd
Cc: laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Boris Brezillon,
Marek Vasut, Brian Norris, Graham Moore, David Woodhouse,
Masami Hiramatsu, Chuanxiao Dong, Jassi Brar, Masahiro Yamada,
linux-kernel, Richard Weinberger, Cyrille Pitchen
This will allow nand_dt_init() to parse DT properties in the NAND
controller device node.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2: None
drivers/mtd/nand/denali.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 24c35a6..3badb1d 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1496,6 +1496,7 @@ int denali_init(struct denali_nand_info *denali)
/* now that our ISR is registered, we can enable interrupts */
denali_set_intr_modes(denali, true);
mtd->name = "denali-nand";
+ nand_set_flash_node(chip, denali->dev->of_node);
/* register the driver with the NAND core subsystem */
chip->select_chip = denali_select_chip;
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [PATCH v2 20/53] mtd: nand: denali: do not set mtd->name
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
` (18 preceding siblings ...)
2017-03-22 14:07 ` [PATCH v2 19/53] mtd: nand: denali: call nand_set_flash_node() to set DT node Masahiro Yamada
@ 2017-03-22 14:07 ` Masahiro Yamada
2017-03-27 15:31 ` Boris Brezillon
2017-03-22 14:07 ` [PATCH v2 21/53] mtd: nand: denali: move multi device fixup code to a helper function Masahiro Yamada
` (3 subsequent siblings)
23 siblings, 1 reply; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-22 14:07 UTC (permalink / raw)
To: linux-mtd
Cc: laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Boris Brezillon,
Marek Vasut, Brian Norris, Graham Moore, David Woodhouse,
Masami Hiramatsu, Chuanxiao Dong, Jassi Brar, Masahiro Yamada,
linux-kernel, Richard Weinberger, Cyrille Pitchen
This will be filled by nand_scan_ident() later.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2: None
drivers/mtd/nand/denali.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 3badb1d..1706975 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1495,7 +1495,6 @@ int denali_init(struct denali_nand_info *denali)
/* now that our ISR is registered, we can enable interrupts */
denali_set_intr_modes(denali, true);
- mtd->name = "denali-nand";
nand_set_flash_node(chip, denali->dev->of_node);
/* register the driver with the NAND core subsystem */
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v2 20/53] mtd: nand: denali: do not set mtd->name
2017-03-22 14:07 ` [PATCH v2 20/53] mtd: nand: denali: do not set mtd->name Masahiro Yamada
@ 2017-03-27 15:31 ` Boris Brezillon
2017-03-28 21:32 ` Masahiro Yamada
0 siblings, 1 reply; 47+ messages in thread
From: Boris Brezillon @ 2017-03-27 15:31 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-mtd, laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Marek Vasut,
Brian Norris, Graham Moore, David Woodhouse, Masami Hiramatsu,
Chuanxiao Dong, Jassi Brar, linux-kernel, Richard Weinberger,
Cyrille Pitchen
On Wed, 22 Mar 2017 23:07:27 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> This will be filled by nand_scan_ident() later.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> Changes in v2: None
>
> drivers/mtd/nand/denali.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index 3badb1d..1706975 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -1495,7 +1495,6 @@ int denali_init(struct denali_nand_info *denali)
>
> /* now that our ISR is registered, we can enable interrupts */
> denali_set_intr_modes(denali, true);
> - mtd->name = "denali-nand";
Are you sure this is safe to do that? When mtd->name is NULL, the core
takes the parent name, and in the denali_dt case it's not "denali-nand",
which means you're breaking mtdparts compat.
> nand_set_flash_node(chip, denali->dev->of_node);
>
> /* register the driver with the NAND core subsystem */
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 20/53] mtd: nand: denali: do not set mtd->name
2017-03-27 15:31 ` Boris Brezillon
@ 2017-03-28 21:32 ` Masahiro Yamada
2017-03-28 21:40 ` Boris Brezillon
0 siblings, 1 reply; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-28 21:32 UTC (permalink / raw)
To: Boris Brezillon
Cc: linux-mtd, Laurent Monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Marek Vasut,
Brian Norris, Graham Moore, David Woodhouse, Masami Hiramatsu,
Chuanxiao Dong, Jassi Brar, Linux Kernel Mailing List,
Richard Weinberger, Cyrille Pitchen
Hi Boris,
2017-03-28 0:31 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Wed, 22 Mar 2017 23:07:27 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> This will be filled by nand_scan_ident() later.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>> Changes in v2: None
>>
>> drivers/mtd/nand/denali.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>> index 3badb1d..1706975 100644
>> --- a/drivers/mtd/nand/denali.c
>> +++ b/drivers/mtd/nand/denali.c
>> @@ -1495,7 +1495,6 @@ int denali_init(struct denali_nand_info *denali)
>>
>> /* now that our ISR is registered, we can enable interrupts */
>> denali_set_intr_modes(denali, true);
>> - mtd->name = "denali-nand";
>
> Are you sure this is safe to do that? When mtd->name is NULL, the core
> takes the parent name, and in the denali_dt case it's not "denali-nand",
> which means you're breaking mtdparts compat.
How big impact is this?
I think a bootloader could give mtdparts=denali-nand:...
but, now we are able to have partitions in DT nodes.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 20/53] mtd: nand: denali: do not set mtd->name
2017-03-28 21:32 ` Masahiro Yamada
@ 2017-03-28 21:40 ` Boris Brezillon
2017-03-29 1:19 ` Masahiro Yamada
0 siblings, 1 reply; 47+ messages in thread
From: Boris Brezillon @ 2017-03-28 21:40 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-mtd, Laurent Monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Marek Vasut,
Brian Norris, Graham Moore, David Woodhouse, Masami Hiramatsu,
Chuanxiao Dong, Jassi Brar, Linux Kernel Mailing List,
Richard Weinberger, Cyrille Pitchen
On Wed, 29 Mar 2017 06:32:24 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> Hi Boris,
>
> 2017-03-28 0:31 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > On Wed, 22 Mar 2017 23:07:27 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >
> >> This will be filled by nand_scan_ident() later.
> >>
> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> >> ---
> >>
> >> Changes in v2: None
> >>
> >> drivers/mtd/nand/denali.c | 1 -
> >> 1 file changed, 1 deletion(-)
> >>
> >> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> >> index 3badb1d..1706975 100644
> >> --- a/drivers/mtd/nand/denali.c
> >> +++ b/drivers/mtd/nand/denali.c
> >> @@ -1495,7 +1495,6 @@ int denali_init(struct denali_nand_info *denali)
> >>
> >> /* now that our ISR is registered, we can enable interrupts */
> >> denali_set_intr_modes(denali, true);
> >> - mtd->name = "denali-nand";
> >
> > Are you sure this is safe to do that? When mtd->name is NULL, the core
> > takes the parent name, and in the denali_dt case it's not "denali-nand",
> > which means you're breaking mtdparts compat.
>
> How big impact is this?
Breaking boot on some platforms (those defining partitions through
mtdparts= cmdline parameter), which is not negligible :P.
>
> I think a bootloader could give mtdparts=denali-nand:...
> but, now we are able to have partitions in DT nodes.
Just because you have a new way to describe partitions (using DT) does
not mean people are not using the old one (mtdparts= parameter).
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 20/53] mtd: nand: denali: do not set mtd->name
2017-03-28 21:40 ` Boris Brezillon
@ 2017-03-29 1:19 ` Masahiro Yamada
2017-03-29 7:19 ` Boris Brezillon
0 siblings, 1 reply; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-29 1:19 UTC (permalink / raw)
To: Boris Brezillon
Cc: linux-mtd, Laurent Monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Marek Vasut,
Brian Norris, Graham Moore, David Woodhouse, Masami Hiramatsu,
Chuanxiao Dong, Jassi Brar, Linux Kernel Mailing List,
Richard Weinberger, Cyrille Pitchen
Hi Boris,
2017-03-29 6:40 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Wed, 29 Mar 2017 06:32:24 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> Hi Boris,
>>
>> 2017-03-28 0:31 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> > On Wed, 22 Mar 2017 23:07:27 +0900
>> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>> >
>> >> This will be filled by nand_scan_ident() later.
>> >>
>> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> >> ---
>> >>
>> >> Changes in v2: None
>> >>
>> >> drivers/mtd/nand/denali.c | 1 -
>> >> 1 file changed, 1 deletion(-)
>> >>
>> >> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>> >> index 3badb1d..1706975 100644
>> >> --- a/drivers/mtd/nand/denali.c
>> >> +++ b/drivers/mtd/nand/denali.c
>> >> @@ -1495,7 +1495,6 @@ int denali_init(struct denali_nand_info *denali)
>> >>
>> >> /* now that our ISR is registered, we can enable interrupts */
>> >> denali_set_intr_modes(denali, true);
>> >> - mtd->name = "denali-nand";
>> >
>> > Are you sure this is safe to do that? When mtd->name is NULL, the core
>> > takes the parent name, and in the denali_dt case it's not "denali-nand",
>> > which means you're breaking mtdparts compat.
>>
>> How big impact is this?
>
> Breaking boot on some platforms (those defining partitions through
> mtdparts= cmdline parameter), which is not negligible :P.
>
>>
>> I think a bootloader could give mtdparts=denali-nand:...
>> but, now we are able to have partitions in DT nodes.
>
> Just because you have a new way to describe partitions (using DT) does
> not mean people are not using the old one (mtdparts= parameter).
I thought DT-node derived name can identify the hardware
even if multiple Denali controllers exist on an SoC.
Anyway, I admit this is a kind of breakage.
If this one is reject, I will drop it drop v3.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 20/53] mtd: nand: denali: do not set mtd->name
2017-03-29 1:19 ` Masahiro Yamada
@ 2017-03-29 7:19 ` Boris Brezillon
2017-03-29 11:30 ` Masahiro Yamada
0 siblings, 1 reply; 47+ messages in thread
From: Boris Brezillon @ 2017-03-29 7:19 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-mtd, Laurent Monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Marek Vasut,
Brian Norris, Graham Moore, David Woodhouse, Masami Hiramatsu,
Chuanxiao Dong, Jassi Brar, Linux Kernel Mailing List,
Richard Weinberger, Cyrille Pitchen
On Wed, 29 Mar 2017 10:19:02 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> Hi Boris,
>
> 2017-03-29 6:40 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > On Wed, 29 Mar 2017 06:32:24 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >
> >> Hi Boris,
> >>
> >> 2017-03-28 0:31 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> >> > On Wed, 22 Mar 2017 23:07:27 +0900
> >> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >> >
> >> >> This will be filled by nand_scan_ident() later.
> >> >>
> >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> >> >> ---
> >> >>
> >> >> Changes in v2: None
> >> >>
> >> >> drivers/mtd/nand/denali.c | 1 -
> >> >> 1 file changed, 1 deletion(-)
> >> >>
> >> >> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> >> >> index 3badb1d..1706975 100644
> >> >> --- a/drivers/mtd/nand/denali.c
> >> >> +++ b/drivers/mtd/nand/denali.c
> >> >> @@ -1495,7 +1495,6 @@ int denali_init(struct denali_nand_info *denali)
> >> >>
> >> >> /* now that our ISR is registered, we can enable interrupts */
> >> >> denali_set_intr_modes(denali, true);
> >> >> - mtd->name = "denali-nand";
> >> >
> >> > Are you sure this is safe to do that? When mtd->name is NULL, the core
> >> > takes the parent name, and in the denali_dt case it's not "denali-nand",
> >> > which means you're breaking mtdparts compat.
> >>
> >> How big impact is this?
> >
> > Breaking boot on some platforms (those defining partitions through
> > mtdparts= cmdline parameter), which is not negligible :P.
> >
> >>
> >> I think a bootloader could give mtdparts=denali-nand:...
> >> but, now we are able to have partitions in DT nodes.
> >
> > Just because you have a new way to describe partitions (using DT) does
> > not mean people are not using the old one (mtdparts= parameter).
>
>
> I thought DT-node derived name can identify the hardware
> even if multiple Denali controllers exist on an SoC.
>
> Anyway, I admit this is a kind of breakage.
>
> If this one is reject, I will drop it drop v3.
>
Yes, please keep the existing name, changing that without extra
precautions has proven to be a bad idea [1].
Note that we now have a way to give user-friendly names to MTD devices
through DT definitions [2]. So, if you ever want to assing a specific
name to your NAND, all you have to do is add a label property to the
NAND device node, and then, in the driver:
nand_set_flash_node(chip, denali->dev->of_node);
/*
* Fallback to the default name if no label property was
* defined.
*/
if (!mtd->name)
mtd->name = "denali-nand";
[1]https://patchwork.ozlabs.org/patch/707065/
[2]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/mtd/mtd.h?id=refs/tags/v4.11-rc4#n386
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 20/53] mtd: nand: denali: do not set mtd->name
2017-03-29 7:19 ` Boris Brezillon
@ 2017-03-29 11:30 ` Masahiro Yamada
0 siblings, 0 replies; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-29 11:30 UTC (permalink / raw)
To: Boris Brezillon
Cc: linux-mtd, Laurent Monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Marek Vasut,
Brian Norris, Graham Moore, David Woodhouse, Masami Hiramatsu,
Chuanxiao Dong, Jassi Brar, Linux Kernel Mailing List,
Richard Weinberger, Cyrille Pitchen
Hi Boris,
2017-03-29 16:19 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Wed, 29 Mar 2017 10:19:02 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> Hi Boris,
>>
>> 2017-03-29 6:40 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> > On Wed, 29 Mar 2017 06:32:24 +0900
>> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>> >
>> >> Hi Boris,
>> >>
>> >> 2017-03-28 0:31 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> >> > On Wed, 22 Mar 2017 23:07:27 +0900
>> >> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>> >> >
>> >> >> This will be filled by nand_scan_ident() later.
>> >> >>
>> >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> >> >> ---
>> >> >>
>> >> >> Changes in v2: None
>> >> >>
>> >> >> drivers/mtd/nand/denali.c | 1 -
>> >> >> 1 file changed, 1 deletion(-)
>> >> >>
>> >> >> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>> >> >> index 3badb1d..1706975 100644
>> >> >> --- a/drivers/mtd/nand/denali.c
>> >> >> +++ b/drivers/mtd/nand/denali.c
>> >> >> @@ -1495,7 +1495,6 @@ int denali_init(struct denali_nand_info *denali)
>> >> >>
>> >> >> /* now that our ISR is registered, we can enable interrupts */
>> >> >> denali_set_intr_modes(denali, true);
>> >> >> - mtd->name = "denali-nand";
>> >> >
>> >> > Are you sure this is safe to do that? When mtd->name is NULL, the core
>> >> > takes the parent name, and in the denali_dt case it's not "denali-nand",
>> >> > which means you're breaking mtdparts compat.
>> >>
>> >> How big impact is this?
>> >
>> > Breaking boot on some platforms (those defining partitions through
>> > mtdparts= cmdline parameter), which is not negligible :P.
>> >
>> >>
>> >> I think a bootloader could give mtdparts=denali-nand:...
>> >> but, now we are able to have partitions in DT nodes.
>> >
>> > Just because you have a new way to describe partitions (using DT) does
>> > not mean people are not using the old one (mtdparts= parameter).
>>
>>
>> I thought DT-node derived name can identify the hardware
>> even if multiple Denali controllers exist on an SoC.
>>
>> Anyway, I admit this is a kind of breakage.
>>
>> If this one is reject, I will drop it drop v3.
>>
>
> Yes, please keep the existing name, changing that without extra
> precautions has proven to be a bad idea [1].
>
> Note that we now have a way to give user-friendly names to MTD devices
> through DT definitions [2]. So, if you ever want to assing a specific
> name to your NAND, all you have to do is add a label property to the
> NAND device node, and then, in the driver:
>
> nand_set_flash_node(chip, denali->dev->of_node);
>
> /*
> * Fallback to the default name if no label property was
> * defined.
> */
> if (!mtd->name)
> mtd->name = "denali-nand";
>
>
> [1]https://patchwork.ozlabs.org/patch/707065/
I will replace it with this solution.
Thanks for the tip!
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v2 21/53] mtd: nand: denali: move multi device fixup code to a helper function
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
` (19 preceding siblings ...)
2017-03-22 14:07 ` [PATCH v2 20/53] mtd: nand: denali: do not set mtd->name Masahiro Yamada
@ 2017-03-22 14:07 ` Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 22/53] mtd: nand: denali: simplify multi device fixup code Masahiro Yamada
` (2 subsequent siblings)
23 siblings, 0 replies; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-22 14:07 UTC (permalink / raw)
To: linux-mtd
Cc: laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Boris Brezillon,
Marek Vasut, Brian Norris, Graham Moore, David Woodhouse,
Masami Hiramatsu, Chuanxiao Dong, Jassi Brar, Masahiro Yamada,
linux-kernel, Richard Weinberger, Cyrille Pitchen
Collect multi NAND fixups into a helper function instead of
scattering them in denali_init().
I am rewording the comment block to clearly explain what is called
"multi device".
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2:
- Reword the comment block for clarification
drivers/mtd/nand/denali.c | 54 +++++++++++++++++++++++++++++------------------
1 file changed, 33 insertions(+), 21 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 1706975..4e63d57 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1458,6 +1458,36 @@ static void denali_drv_init(struct denali_nand_info *denali)
denali->irq_status = 0;
}
+static void denali_multidev_fixup(struct denali_nand_info *denali)
+{
+ struct nand_chip *chip = &denali->nand;
+ struct mtd_info *mtd = nand_to_mtd(chip);
+
+ /*
+ * Support for multi device:
+ * When the IP configuration is x16 capable and two x8 chips are
+ * connected in parallel, DEVICES_CONNECTED should be set to 2.
+ * In this case, the core framework knows nothing about this fact,
+ * so we should tell it the _logical_ pagesize and anything necessary.
+ */
+ denali->devnum = ioread32(denali->flash_reg + DEVICES_CONNECTED);
+
+ mtd->size <<= denali->devnum - 1;
+ mtd->erasesize <<= denali->devnum - 1;
+ mtd->writesize <<= denali->devnum - 1;
+ mtd->oobsize <<= denali->devnum - 1;
+ chip->chipsize <<= denali->devnum - 1;
+ chip->page_shift += denali->devnum - 1;
+ chip->phys_erase_shift += denali->devnum - 1;
+ chip->bbt_erase_shift += denali->devnum - 1;
+ chip->chip_shift += denali->devnum - 1;
+ chip->pagemask <<= denali->devnum - 1;
+ chip->ecc.size *= denali->devnum;
+ chip->ecc.bytes *= denali->devnum;
+ chip->ecc.strength *= denali->devnum;
+ denali->bbtskipbytes *= denali->devnum;
+}
+
int denali_init(struct denali_nand_info *denali)
{
struct nand_chip *chip = &denali->nand;
@@ -1540,24 +1570,6 @@ int denali_init(struct denali_nand_info *denali)
}
/*
- * support for multi nand
- * MTD known nothing about multi nand, so we should tell it
- * the real pagesize and anything necessery
- */
- denali->devnum = ioread32(denali->flash_reg + DEVICES_CONNECTED);
- chip->chipsize <<= denali->devnum - 1;
- chip->page_shift += denali->devnum - 1;
- chip->pagemask = (chip->chipsize >> chip->page_shift) - 1;
- chip->bbt_erase_shift += denali->devnum - 1;
- chip->phys_erase_shift = chip->bbt_erase_shift;
- chip->chip_shift += denali->devnum - 1;
- mtd->writesize <<= denali->devnum - 1;
- mtd->oobsize <<= denali->devnum - 1;
- mtd->erasesize <<= denali->devnum - 1;
- mtd->size = chip->numchips * chip->chipsize;
- denali->bbtskipbytes *= denali->devnum;
-
- /*
* second stage of the NAND scan
* this stage requires information regarding ECC and
* bad block management.
@@ -1600,11 +1612,9 @@ int denali_init(struct denali_nand_info *denali)
}
mtd_set_ooblayout(mtd, &denali_ooblayout_ops);
- chip->ecc.bytes *= denali->devnum;
- chip->ecc.strength *= denali->devnum;
/* override the default read operations */
- chip->ecc.size = ECC_SECTOR_SIZE * denali->devnum;
+ chip->ecc.size = ECC_SECTOR_SIZE;
chip->ecc.read_page = denali_read_page;
chip->ecc.read_page_raw = denali_read_page_raw;
chip->ecc.write_page = denali_write_page;
@@ -1613,6 +1623,8 @@ int denali_init(struct denali_nand_info *denali)
chip->ecc.write_oob = denali_write_oob;
chip->erase = denali_erase;
+ denali_multidev_fixup(denali);
+
ret = nand_scan_tail(mtd);
if (ret)
goto failed_req_irq;
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [PATCH v2 22/53] mtd: nand: denali: simplify multi device fixup code
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
` (20 preceding siblings ...)
2017-03-22 14:07 ` [PATCH v2 21/53] mtd: nand: denali: move multi device fixup code to a helper function Masahiro Yamada
@ 2017-03-22 14:07 ` Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 23/53] mtd: nand: denali: set DEVICES_CONNECTED 1 if not set Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 24/53] mtd: nand: denali: remove meaningless writes to read-only registers Masahiro Yamada
23 siblings, 0 replies; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-22 14:07 UTC (permalink / raw)
To: linux-mtd
Cc: laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Boris Brezillon,
Marek Vasut, Brian Norris, Graham Moore, David Woodhouse,
Masami Hiramatsu, Chuanxiao Dong, Jassi Brar, Masahiro Yamada,
linux-kernel, Richard Weinberger, Cyrille Pitchen
The available configuration of the IP bus width is x8 or x16, so the
possible value for denali->devnum is 1 or 2.
If the value is 1, there is nothing to do. Fixup parameters only
when denali->devnum is 2.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2:
- Simplify the code given 1 or 2 is possible for denali->devnum
drivers/mtd/nand/denali.c | 46 ++++++++++++++++++++++++++++++----------------
1 file changed, 30 insertions(+), 16 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 4e63d57..abf8997 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1458,7 +1458,7 @@ static void denali_drv_init(struct denali_nand_info *denali)
denali->irq_status = 0;
}
-static void denali_multidev_fixup(struct denali_nand_info *denali)
+static int denali_multidev_fixup(struct denali_nand_info *denali)
{
struct nand_chip *chip = &denali->nand;
struct mtd_info *mtd = nand_to_mtd(chip);
@@ -1472,20 +1472,32 @@ static void denali_multidev_fixup(struct denali_nand_info *denali)
*/
denali->devnum = ioread32(denali->flash_reg + DEVICES_CONNECTED);
- mtd->size <<= denali->devnum - 1;
- mtd->erasesize <<= denali->devnum - 1;
- mtd->writesize <<= denali->devnum - 1;
- mtd->oobsize <<= denali->devnum - 1;
- chip->chipsize <<= denali->devnum - 1;
- chip->page_shift += denali->devnum - 1;
- chip->phys_erase_shift += denali->devnum - 1;
- chip->bbt_erase_shift += denali->devnum - 1;
- chip->chip_shift += denali->devnum - 1;
- chip->pagemask <<= denali->devnum - 1;
- chip->ecc.size *= denali->devnum;
- chip->ecc.bytes *= denali->devnum;
- chip->ecc.strength *= denali->devnum;
- denali->bbtskipbytes *= denali->devnum;
+ if (denali->devnum == 1)
+ return 0;
+
+ if (denali->devnum != 2) {
+ dev_err(denali->dev, "unsupported number of devices %d\n",
+ denali->devnum);
+ return -EINVAL;
+ }
+
+ /* 2 chips in parallel */
+ mtd->size <<= 1;
+ mtd->erasesize <<= 1;
+ mtd->writesize <<= 1;
+ mtd->oobsize <<= 1;
+ chip->chipsize <<= 1;
+ chip->page_shift += 1;
+ chip->phys_erase_shift += 1;
+ chip->bbt_erase_shift += 1;
+ chip->chip_shift += 1;
+ chip->pagemask <<= 1;
+ chip->ecc.size <<= 1;
+ chip->ecc.bytes <<= 1;
+ chip->ecc.strength <<= 1;
+ denali->bbtskipbytes <<= 1;
+
+ return 0;
}
int denali_init(struct denali_nand_info *denali)
@@ -1623,7 +1635,9 @@ int denali_init(struct denali_nand_info *denali)
chip->ecc.write_oob = denali_write_oob;
chip->erase = denali_erase;
- denali_multidev_fixup(denali);
+ ret = denali_multidev_fixup(denali);
+ if (ret)
+ goto failed_req_irq;
ret = nand_scan_tail(mtd);
if (ret)
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [PATCH v2 23/53] mtd: nand: denali: set DEVICES_CONNECTED 1 if not set
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
` (21 preceding siblings ...)
2017-03-22 14:07 ` [PATCH v2 22/53] mtd: nand: denali: simplify multi device fixup code Masahiro Yamada
@ 2017-03-22 14:07 ` Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 24/53] mtd: nand: denali: remove meaningless writes to read-only registers Masahiro Yamada
23 siblings, 0 replies; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-22 14:07 UTC (permalink / raw)
To: linux-mtd
Cc: laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Boris Brezillon,
Marek Vasut, Brian Norris, Graham Moore, David Woodhouse,
Masami Hiramatsu, Chuanxiao Dong, Jassi Brar, Masahiro Yamada,
linux-kernel, Richard Weinberger, Cyrille Pitchen
Currently, the driver expects DEVICE_CONNECTED is automatically set
by the hardware, but this feature is disabled in some cases.
In such cases, it is the software's responsibility to set up the
DEVICES_CONNECTED register.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2: None
drivers/mtd/nand/denali.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index abf8997..3da0a0e 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1472,6 +1472,15 @@ static int denali_multidev_fixup(struct denali_nand_info *denali)
*/
denali->devnum = ioread32(denali->flash_reg + DEVICES_CONNECTED);
+ /*
+ * On some SoCs, DEVICES_CONNECTED is not auto-detected.
+ * For those, DEVICES_CONNECTED is left to 0. Set 1 if it is the case.
+ */
+ if (denali->devnum == 0) {
+ denali->devnum = 1;
+ iowrite32(1, denali->flash_reg + DEVICES_CONNECTED);
+ }
+
if (denali->devnum == 1)
return 0;
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread* [PATCH v2 24/53] mtd: nand: denali: remove meaningless writes to read-only registers
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
` (22 preceding siblings ...)
2017-03-22 14:07 ` [PATCH v2 23/53] mtd: nand: denali: set DEVICES_CONNECTED 1 if not set Masahiro Yamada
@ 2017-03-22 14:07 ` Masahiro Yamada
23 siblings, 0 replies; 47+ messages in thread
From: Masahiro Yamada @ 2017-03-22 14:07 UTC (permalink / raw)
To: linux-mtd
Cc: laurent.monat, thorsten.christiansson, Enrico Jorns,
Jason Roberts, Artem Bityutskiy, Dinh Nguyen, Boris Brezillon,
Marek Vasut, Brian Norris, Graham Moore, David Woodhouse,
Masami Hiramatsu, Chuanxiao Dong, Jassi Brar, Masahiro Yamada,
linux-kernel, Richard Weinberger, Cyrille Pitchen
The write accesses to LOGICAL_PAGE_{DATA,SPARE}_SIZE have no effect
because the Denali User's Guide says these registers are read-only.
The hardware automatically multiplies the main/spare size by the
number of devices and update LOGICAL_PAGE_{DATA,SPARE}_SIZE.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2: None
drivers/mtd/nand/denali.c | 16 ----------------
1 file changed, 16 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 3da0a0e..5e89709 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -342,8 +342,6 @@ static void get_samsung_nand_para(struct denali_nand_info *denali,
static void get_toshiba_nand_para(struct denali_nand_info *denali)
{
- uint32_t tmp;
-
/*
* Workaround to fix a controller bug which reports a wrong
* spare area size for some kind of Toshiba NAND device
@@ -351,10 +349,6 @@ static void get_toshiba_nand_para(struct denali_nand_info *denali)
if ((ioread32(denali->flash_reg + DEVICE_MAIN_AREA_SIZE) == 4096) &&
(ioread32(denali->flash_reg + DEVICE_SPARE_AREA_SIZE) == 64)) {
iowrite32(216, denali->flash_reg + DEVICE_SPARE_AREA_SIZE);
- tmp = ioread32(denali->flash_reg + DEVICES_CONNECTED) *
- ioread32(denali->flash_reg + DEVICE_SPARE_AREA_SIZE);
- iowrite32(tmp,
- denali->flash_reg + LOGICAL_PAGE_SPARE_SIZE);
#if SUPPORT_15BITECC
iowrite32(15, denali->flash_reg + ECC_CORRECTION);
#elif SUPPORT_8BITECC
@@ -366,22 +360,12 @@ static void get_toshiba_nand_para(struct denali_nand_info *denali)
static void get_hynix_nand_para(struct denali_nand_info *denali,
uint8_t device_id)
{
- uint32_t main_size, spare_size;
-
switch (device_id) {
case 0xD5: /* Hynix H27UAG8T2A, H27UBG8U5A or H27UCG8VFA */
case 0xD7: /* Hynix H27UDG8VEM, H27UCG8UDM or H27UCG8V5A */
iowrite32(128, denali->flash_reg + PAGES_PER_BLOCK);
iowrite32(4096, denali->flash_reg + DEVICE_MAIN_AREA_SIZE);
iowrite32(224, denali->flash_reg + DEVICE_SPARE_AREA_SIZE);
- main_size = 4096 *
- ioread32(denali->flash_reg + DEVICES_CONNECTED);
- spare_size = 224 *
- ioread32(denali->flash_reg + DEVICES_CONNECTED);
- iowrite32(main_size,
- denali->flash_reg + LOGICAL_PAGE_DATA_SIZE);
- iowrite32(spare_size,
- denali->flash_reg + LOGICAL_PAGE_SPARE_SIZE);
iowrite32(0, denali->flash_reg + DEVICE_WIDTH);
#if SUPPORT_15BITECC
iowrite32(15, denali->flash_reg + ECC_CORRECTION);
--
2.7.4
^ permalink raw reply related [flat|nested] 47+ messages in thread