RE: OMAP2+: gpmc: fix gpmc_hwecc_bch_capable()
From: Christoph Fritz [mailto:chf.fr...@googlemail.com]
This patch adds bch8 ecc software fallback which is mostly used by
omap3s because they lack hardware elm support.
Fixes: 0611c41934ab35ce84dea34ab291897ad3cbc7be (ARM: OMAP2+: gpmc:
update gpmc_hwecc_bch_capable() for new platforms and ECC schemes)
Cc: sta...@vger.kernel.org # 3.15.x+
Signed-off-by: Christoph Fritz chf.fr...@googlemail.com
---
arch/arm/mach-omap2/gpmc-nand.c | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c
index 17cd393..93914d2 100644
--- a/arch/arm/mach-omap2/gpmc-nand.c
+++ b/arch/arm/mach-omap2/gpmc-nand.c
@@ -50,6 +50,16 @@ static bool gpmc_hwecc_bch_capable(enum omap_ecc ecc_opt)
soc_is_omap54xx() || soc_is_dra7xx())
return 1;
+ if (ecc_opt == OMAP_ECC_BCH4_CODE_HW_DETECTION_SW ||
+ ecc_opt == OMAP_ECC_BCH8_CODE_HW_DETECTION_SW) {
+ if (cpu_is_omap24xx())
+ return 0;
+ else if (cpu_is_omap3630() (GET_OMAP_REVISION() == 0))
+ return 0;
+ else
+ return 1;
+ }
+
/* OMAP3xxx do not have ELM engine, so cannot support ECC schemes
* which require H/W based ECC error detection */
if ((cpu_is_omap34xx() || cpu_is_omap3630())
@@ -57,14 +67,6 @@ static bool gpmc_hwecc_bch_capable(enum omap_ecc ecc_opt)
(ecc_opt == OMAP_ECC_BCH8_CODE_HW)))
return 0;
- /*
- * For now, assume 4-bit mode is only supported on OMAP3630 ES1.x, x=1
- * and AM33xx derivates. Other chips may be added if confirmed to work.
- */
- if ((ecc_opt == OMAP_ECC_BCH4_CODE_HW_DETECTION_SW)
- (!cpu_is_omap3630() || (GET_OMAP_REVISION() == 0)))
- return 0;
-
/* legacy platforms support only HAM1 (1-bit Hamming) ECC scheme */
if (ecc_opt == OMAP_ECC_HAM1_CODE_HW)
return 1;
--
1.7.10.4
Thanks much for this fix.
Reviewed-by: Pekon Gupta pe...@ti.com
with regards, pekon
RE: [RFC PATCH 00/10] OMAP: GPMC: NAND: Introduce GPMC APIs for OMAP NAND
Hi Roger, From: Tony Lindgren [mailto:t...@atomide.com] * Roger Quadros rog...@ti.com [140709 05:39]: Hi, The following hardware modules/registers are meant for NAND controller driver usage: - NAND I/O control (NAND address, data, command registers) - Prefetch/Write-post engine - ECC/BCH engine However, these registers sit in the GPMC controller's register space and there need to be some sane way to access them from the OMAP NAND controller driver. Till now the GPMC driver was populating a data structure (struct gpmc_nand_regs) with the register addresses and passing it to the OMAP NAND driver via platform data. This mechanism cannot be used for true Device tree support as custom platform data passing mechanism doesn't seem to work. Moreover, direct access to these registers must be limited to the GPMC driver. This calls for a few custom OMAP GPMC specific APIs that the OMAP NAND driver can use to access these GPMC space registers. This series attempts to add the following new APIs and gets rid of 'struct gpmc_nand_regs' and 'gpmc_update_nand_regs()'. -For NAND I/O control registers u32 omap_gpmc_read_reg(int cs, enum omap_gpmc_reg reg); void omap_gpmc_write_reg(int cs, enum omap_gpmc_reg reg, u32 val); -For Prefetch engine int omap_gpmc_prefetch_start(int cs, int fifo_th, bool dma, u32 count, int is_write); int omap_gpmc_prefetch_stop(int cs); u32 omap_gpmc_get_prefetch_count(void); u32 omap_gpmc_get_prefetch_fifo_count(void); -For ECC/BCH engine void omap_gpmc_ecc_disable(void); void omap_gpmc_ecc_configure_enable(int cs, bool ecc16, u8 ecc_size0, u8 ecc_size1, bool use_bch, enum omap_gpmc_bch_type bch_type, u8 bch_sectors, u8 bch_wrap_mode); I think this one has too big argument list. And also this interface will become inconsistent when you will expand the NAND driver to support devices with larger page-size(like 8K NAND devices) Why can't we just use omap_gpmc_write_reg(int cs, enum omap_gpmc_reg reg); as already defined above? This is one-time configuration per read/write cycle so using 'omap_gpmc_write_reg()' shouldn't be much of issue. And this will automatically plugin to current chip-ecc.hwctl() calls. void omap_gpmc_ecc_get_result(int length, u32 *result); Can you please rename it to omap_gpmc_ecc_get_hamming_result() Just to keep it consistent with omap_gpmc_ecc_get_bch_result() void omap_gpmc_ecc_get_bch_result(int length, u8 sector, u32 *result); This one looks good, but you should also take in int 'ecc-scheme'. Actually you can just move omap_calculate_ecc_bch(...) out of NAND driver into GPMC driver and rename it, because support of ECC scheme is property of hardware controller not NAND driver. What ecc-schemes GPMC controller supports should be inside GPMC driver, and NAND driver should just attach them to appropriate interfaces. Right ? Same way just think of moving chip-ecc.hwctl() callbacks implementations out of NAND driver into GPMC driver. Then you would _not_ need to export any GPMC registers into NAND driver. with regards, pekon These seem fine to me. At least I don't have any better ideas to expose these GPMC registers to the NAND driver(s). These APIs don't implement any logic to serialize access to the NAND/Prefetch/ECC registers. It is upto the NAND controller driver to ensure that. As these modules can only handle one NAND controller context at a time, we set the nand_chip-hwcontrol to point to a single controller instance even if there are multiple NAND chips on different Chip select spaces. The NAND base driver then takes care of serializing access to the NAND controller (and ECC) through nandchip-hwcontrol-lock. NOTE: Patches are still untested and only meant for review. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH 02/10] mtd: nand: omap: Always use chip-ecc.steps for BCH sector count
From: Quadros, Roger
Instead of hardcoding use the pre-calculated chip-ecc.steps for
configuring number of sectors to process with the BCH algorithm.
This also avoids unnecessary access to the ECC_CONFIG register in
omap_calculate_ecc_bch().
Signed-off-by: Roger Quadros rog...@ti.com
---
drivers/mtd/nand/omap2.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 5b8739c..6f3d7cd 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1066,10 +1066,10 @@ static void __maybe_unused
omap_enable_hwecc_bch(struct mtd_info
*mtd, int mode)
unsigned int ecc_size1, ecc_size0;
/* GPMC configurations for calculating ECC */
+ nsectors = chip-ecc.steps;
switch (ecc_opt) {
case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW:
bch_type = 0;
- nsectors = 1;
if (mode == NAND_ECC_READ) {
wr_mode = BCH_WRAPMODE_6;
ecc_size0 = BCH_ECC_SIZE0;
@@ -1082,7 +1082,6 @@ static void __maybe_unused omap_enable_hwecc_bch(struct
mtd_info
*mtd, int mode)
break;
case OMAP_ECC_BCH4_CODE_HW:
bch_type = 0;
- nsectors = chip-ecc.steps;
if (mode == NAND_ECC_READ) {
wr_mode = BCH_WRAPMODE_1;
ecc_size0 = BCH4R_ECC_SIZE0;
@@ -1095,7 +1094,6 @@ static void __maybe_unused omap_enable_hwecc_bch(struct
mtd_info
*mtd, int mode)
break;
case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW:
bch_type = 1;
- nsectors = 1;
if (mode == NAND_ECC_READ) {
wr_mode = BCH_WRAPMODE_6;
ecc_size0 = BCH_ECC_SIZE0;
@@ -1108,7 +1106,6 @@ static void __maybe_unused omap_enable_hwecc_bch(struct
mtd_info
*mtd, int mode)
break;
case OMAP_ECC_BCH8_CODE_HW:
bch_type = 1;
- nsectors = chip-ecc.steps;
if (mode == NAND_ECC_READ) {
wr_mode = BCH_WRAPMODE_1;
ecc_size0 = BCH8R_ECC_SIZE0;
@@ -1121,7 +1118,6 @@ static void __maybe_unused omap_enable_hwecc_bch(struct
mtd_info
*mtd, int mode)
break;
case OMAP_ECC_BCH16_CODE_HW:
bch_type = 0x2;
- nsectors = chip-ecc.steps;
if (mode == NAND_ECC_READ) {
wr_mode = 0x01;
ecc_size0 = 52; /* ECC bits in nibbles per sector */
@@ -1176,6 +1172,7 @@ static int __maybe_unused omap_calculate_ecc_bch(struct
mtd_info *mtd,
{
struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
mtd);
+ struct nand_chip *chip = mtd-priv;
int eccbytes= info-nand.ecc.bytes;
struct gpmc_nand_regs *gpmc_regs = info-reg;
u8 *ecc_code;
@@ -1183,7 +1180,7 @@ static int __maybe_unused omap_calculate_ecc_bch(struct
mtd_info *mtd,
u32 val;
int i, j;
- nsectors = ((readl(info-reg.gpmc_ecc_config) 4) 0x7) + 1;
+ nsectors = chip-ecc.steps;
Sorry NAK.. I'm sure you are breaking something here :-)
NAND driver supports multiple ECC schemes like;
OMAP_ECC_CODE_HAM1_HW (support for legacy reasons)
OMAP_ECC_CODE_BCH4_HW_DETECTION_SW (needed for OMAP3 and AM35xx)
OMAP_ECC_CODE_BCH4_HW
OMAP_ECC_CODE_BCH8_HW
OMAP_ECC_CODE_BCH8_HW_DETECTION_SW (needed for OMAP3 and AM35xx)
OMAP_ECC_CODE_BCH16_HW
IIRC ..
- software based ecc-schemes OMAP_ECC_CODE_BCHx_HW_DETECTION_SW
Reads/Write in per-sector granularity. (here nsector != chip-ecc.steps)
- hardware based ecc-schemes OMAP_ECC_CODE_BCHx_HW, perform
Reads/Write in per-page granularity (or something like this).
Therefore you have custom implementation of
chip-ecc.read_page = omap_read_page_bch()
Also if you change the configurations here, it will break the compatibility
with
u-boot, so images flashed via u-boot will stop to boot in kernel and
vice-versa.
I suggest, please refrain from these tweaks for now..
All these optimizations have been tested on multiple scenario so please test
all ecc-schemes before doing anything, otherwise you will end-up in
a bad loop of breaking and fixing NAND driver :-).
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH 07/10] OMAP: GPMC: Introduce APIs for Configuring ECC Engine
Hi Roger, From: Quadros, Roger Even though the ECC/BCH engine is meant for exclusive use by the OMAP NAND controller, the ECC/BCH registers belong to the GPMC controller's register space Add omap_gpmc_ecc_configure_enable() and omap_gpmc_ecc_disable() to manage the ECC engine. OMAP NAND driver must use these APIs instead of directly accessing the ECC Engine registers. Signed-off-by: Roger Quadros rog...@ti.com --- As suggested in http://lists.infradead.org/pipermail/linux-mtd/2014-July/054595.html Please try to move chip-ecc.calculate() implementations like - omap_calculate_ecc_bch() - omap_calculate_ecc() From NAND driver To GPMC driver, as that should be easy, And solve most of your work (no need to export GPMC registers). - You may rename them appropriately and change the argument list if needed. - And NAND driver can just have some wrapper functions to match the MTD interface arguments. with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH 09/10] mtd: nand: omap: Use GPMC APIs for accessing ECC/BCH engine
Roger,
From: Quadros, Roger
Don't access the ECC/BCH engine registers directly as they belong
to the GPMC controller's register space. Use the relevant
GPMC APIs instead.
Signed-off-by: Roger Quadros rog...@ti.com
---
drivers/mtd/nand/omap2.c | 191 +++
1 file changed, 76 insertions(+), 115 deletions(-)
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 420ef0b..6b0f953 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1123,71 +1088,67 @@ static int __maybe_unused
omap_calculate_ecc_bch(struct mtd_info
*mtd,
switch (info-ecc_opt) {
case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW:
case OMAP_ECC_BCH8_CODE_HW:
- bch_val1 = readl(gpmc_regs-gpmc_bch_result0[i]);
- bch_val2 = readl(gpmc_regs-gpmc_bch_result1[i]);
- bch_val3 = readl(gpmc_regs-gpmc_bch_result2[i]);
- bch_val4 = readl(gpmc_regs-gpmc_bch_result3[i]);
- *ecc_code++ = (bch_val4 0xFF);
- *ecc_code++ = ((bch_val3 24) 0xFF);
- *ecc_code++ = ((bch_val3 16) 0xFF);
- *ecc_code++ = ((bch_val3 8) 0xFF);
- *ecc_code++ = (bch_val3 0xFF);
- *ecc_code++ = ((bch_val2 24) 0xFF);
- *ecc_code++ = ((bch_val2 16) 0xFF);
- *ecc_code++ = ((bch_val2 8) 0xFF);
- *ecc_code++ = (bch_val2 0xFF);
- *ecc_code++ = ((bch_val1 24) 0xFF);
- *ecc_code++ = ((bch_val1 16) 0xFF);
- *ecc_code++ = ((bch_val1 8) 0xFF);
- *ecc_code++ = (bch_val1 0xFF);
+ omap_gpmc_ecc_get_bch_result(4, i, bch_val);
+ *ecc_code++ = (bch_val[3] 0xFF);
+ *ecc_code++ = ((bch_val[2] 24) 0xFF);
+ *ecc_code++ = ((bch_val[2] 16) 0xFF);
+ *ecc_code++ = ((bch_val[2] 8) 0xFF);
+ *ecc_code++ = (bch_val[2] 0xFF);
+ *ecc_code++ = ((bch_val[1] 24) 0xFF);
+ *ecc_code++ = ((bch_val[1] 16) 0xFF);
+ *ecc_code++ = ((bch_val[1] 8) 0xFF);
+ *ecc_code++ = (bch_val[1] 0xFF);
+ *ecc_code++ = ((bch_val[0] 24) 0xFF);
+ *ecc_code++ = ((bch_val[0] 16) 0xFF);
+ *ecc_code++ = ((bch_val[0] 8) 0xFF);
+ *ecc_code++ = (bch_val[0] 0xFF);
break;
case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW:
case OMAP_ECC_BCH4_CODE_HW:
- bch_val1 = readl(gpmc_regs-gpmc_bch_result0[i]);
- bch_val2 = readl(gpmc_regs-gpmc_bch_result1[i]);
- *ecc_code++ = ((bch_val2 12) 0xFF);
- *ecc_code++ = ((bch_val2 4) 0xFF);
- *ecc_code++ = ((bch_val2 0xF) 4) |
- ((bch_val1 28) 0xF);
- *ecc_code++ = ((bch_val1 20) 0xFF);
- *ecc_code++ = ((bch_val1 12) 0xFF);
- *ecc_code++ = ((bch_val1 4) 0xFF);
- *ecc_code++ = ((bch_val1 0xF) 4);
+ omap_gpmc_ecc_get_bch_result(2, i, bch_val);
+ *ecc_code++ = ((bch_val[1] 12) 0xFF);
+ *ecc_code++ = ((bch_val[1] 4) 0xFF);
+ *ecc_code++ = ((bch_val[1] 0xF) 4) |
+ ((bch_val[0] 28) 0xF);
+ *ecc_code++ = ((bch_val[0] 20) 0xFF);
+ *ecc_code++ = ((bch_val[0] 12) 0xFF);
+ *ecc_code++ = ((bch_val[0] 4) 0xFF);
+ *ecc_code++ = ((bch_val[0] 0xF) 4);
break;
case OMAP_ECC_BCH16_CODE_HW:
- val = readl(gpmc_regs-gpmc_bch_result6[i]);
- ecc_code[0] = ((val 8) 0xFF);
- ecc_code[1] = ((val 0) 0xFF);
- val = readl(gpmc_regs-gpmc_bch_result5[i]);
- ecc_code[2] = ((val 24) 0xFF);
- ecc_code[3] = ((val 16) 0xFF);
- ecc_code[4] = ((val 8) 0xFF);
- ecc_code[5] = ((val 0) 0xFF);
- val = readl(gpmc_regs-gpmc_bch_result4[i]);
- ecc_code[6] = ((val 24) 0xFF);
- ecc_code[7] = ((val 16) 0xFF);
- ecc_code[8] = ((val 8) 0xFF);
- ecc_code[9] = ((val 0) 0xFF);
- val = readl(gpmc_regs-gpmc_bch_result3[i]);
- ecc_code[10] = ((val 24) 0xFF);
- ecc_code[11] = ((val 16) 0xFF);
-
RE: [RFC PATCH 00/10] OMAP: GPMC: NAND: Introduce GPMC APIs for OMAP NAND
From: Quadros, Roger On 07/11/2014 10:27 AM, Gupta, Pekon wrote: From: Tony Lindgren [mailto:t...@atomide.com] * Roger Quadros rog...@ti.com [140709 05:39]: Hi, The following hardware modules/registers are meant for NAND controller driver usage: - NAND I/O control (NAND address, data, command registers) - Prefetch/Write-post engine - ECC/BCH engine However, these registers sit in the GPMC controller's register space and there need to be some sane way to access them from the OMAP NAND controller driver. Till now the GPMC driver was populating a data structure (struct gpmc_nand_regs) with the register addresses and passing it to the OMAP NAND driver via platform data. This mechanism cannot be used for true Device tree support as custom platform data passing mechanism doesn't seem to work. Moreover, direct access to these registers must be limited to the GPMC driver. This calls for a few custom OMAP GPMC specific APIs that the OMAP NAND driver can use to access these GPMC space registers. This series attempts to add the following new APIs and gets rid of 'struct gpmc_nand_regs' and 'gpmc_update_nand_regs()'. -For NAND I/O control registers u32 omap_gpmc_read_reg(int cs, enum omap_gpmc_reg reg); void omap_gpmc_write_reg(int cs, enum omap_gpmc_reg reg, u32 val); -For Prefetch engine int omap_gpmc_prefetch_start(int cs, int fifo_th, bool dma, u32 count, int is_write); int omap_gpmc_prefetch_stop(int cs); u32 omap_gpmc_get_prefetch_count(void); u32 omap_gpmc_get_prefetch_fifo_count(void); -For ECC/BCH engine void omap_gpmc_ecc_disable(void); void omap_gpmc_ecc_configure_enable(int cs, bool ecc16, u8 ecc_size0, u8 ecc_size1, bool use_bch, enum omap_gpmc_bch_type bch_type, u8 bch_sectors, u8 bch_wrap_mode); I think this one has too big argument list. And also this interface will become inconsistent when you will expand the NAND driver to support devices with larger page-size(like 8K NAND devices) Why can't we just use omap_gpmc_write_reg(int cs, enum omap_gpmc_reg reg); as already defined above? omap_gpmc_write_reg will not be optimal for reading larger result blocks and does not create enough abstraction between the clearly separate blocks i.e. prefetch engine and ecc engine. This is one-time configuration per read/write cycle so using 'omap_gpmc_write_reg()' shouldn't be much of issue. And this will automatically plugin to current chip-ecc.hwctl() calls. hwctl() doesn't take care of ECC. Those are done by chip-ecc.correct() and chip-ecc.calculate(). Incorrect assumption :-). chip-ecc.hwctl() is used to configure ecc_size0 and ecc_size1, which in-turn depend on ecc-scheme selected. Also, ecc_wrap_mode differs from ecc-scheme for software and hardware implementations. void omap_gpmc_ecc_get_result(int length, u32 *result); Can you please rename it to omap_gpmc_ecc_get_hamming_result() Just to keep it consistent with omap_gpmc_ecc_get_bch_result() OK. Sounds reasonable. void omap_gpmc_ecc_get_bch_result(int length, u8 sector, u32 *result); This one looks good, but you should also take in int 'ecc-scheme'. Could you please explain why? Actually you can just move omap_calculate_ecc_bch(...) out of NAND driver into GPMC driver and rename it, because support of ECC scheme is property of hardware controller not NAND driver. What ecc-schemes GPMC controller supports should be inside GPMC driver, and NAND driver should just attach them to appropriate interfaces. Right ? Same way just think of moving chip-ecc.hwctl() callbacks implementations out of NAND driver into GPMC driver. Then you would _not_ need to export any GPMC registers into NAND driver. I don't agree with moving anything nand_chip related (i.e. ecc.hwctl(), ecc.calculate() or ecc.correct()) to GMPC driver because they are supposed to be implemented by the NAND driver. ECC is a functionality of NAND controller not of GPMC controller. It is just a IP design mistake that they smudged the ECC registers so badly in the GPMC register space. Not really, I don't think that's a problem. That's TI's way of looking at controller architecture. If you read discussion of GPMI (freescale's NAND controller) and NAND controllers from other vendors, there are other complexities. Anyways that's not the discussion here, so converging back :-) .. And, yes, you should _not_ touch chip-ecc.correct() or any other such Implementations because those are NAND framework specific. But below code isn't related to NAND framework, all NAND needs is how you calcaluate the ECC, whether you do by - reading GPMC registers or - by using software bch library, or - mixed of both. Therefore, I suggest you to move following code from NAND driver to GPMC driver, nothing other than. And this exported function does not need any NAND framework information, except number_of_sectors
RE: [RFC PATCH 02/10] mtd: nand: omap: Always use chip-ecc.steps for BCH sector count
From: Quadros, Roger
On 07/11/2014 10:43 AM, Gupta, Pekon wrote:
From: Quadros, Roger
[...]
@@ -1176,6 +1172,7 @@ static int __maybe_unused
omap_calculate_ecc_bch(struct mtd_info
*mtd,
{
struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
mtd);
+ struct nand_chip *chip = mtd-priv;
int eccbytes= info-nand.ecc.bytes;
struct gpmc_nand_regs *gpmc_regs = info-reg;
u8 *ecc_code;
@@ -1183,7 +1180,7 @@ static int __maybe_unused
omap_calculate_ecc_bch(struct mtd_info
*mtd,
u32 val;
int i, j;
- nsectors = ((readl(info-reg.gpmc_ecc_config) 4) 0x7) + 1;
+ nsectors = chip-ecc.steps;
Sorry NAK.. I'm sure you are breaking something here :-)
NAND driver supports multiple ECC schemes like;
OMAP_ECC_CODE_HAM1_HW (support for legacy reasons)
OMAP_ECC_CODE_BCH4_HW_DETECTION_SW (needed for OMAP3 and AM35xx)
OMAP_ECC_CODE_BCH4_HW
OMAP_ECC_CODE_BCH8_HW
OMAP_ECC_CODE_BCH8_HW_DETECTION_SW (needed for OMAP3 and AM35xx)
OMAP_ECC_CODE_BCH16_HW
IIRC ..
- software based ecc-schemes OMAP_ECC_CODE_BCHx_HW_DETECTION_SW
Reads/Write in per-sector granularity. (here nsector != chip-ecc.steps)
OK. I still don't have a full understanding about the ECC schemes so to ensure
we
don't break anything I can just leave nsectors as it is and probably just
store a
copy of it in omap_nand_info to avoid reading it back from gpmc_ecc_config.
I still have a few questions to clarify my understanding.
The only difference between OMAP_ECC_CODE_BCHx_HW_DETECTION_SW and
OMAP_ECC_CODE_BCHx_HW is that in the former the _correction_ is done by
software
and in the latter the _correction_ is done by hardware (i.e. ELM module).
In both cases the _detection_ is done by the same hardware IP via
ecc.calculate(),
i.e. omap_calculate_ecc_bch().
so why should nsector be different for both in the _detection_ stage?
Again IIRC, That is because of ELM driver.
ELM hardware engine has 8 channels with each of them having 512Bytes capacity.
Now, there can be two approaches:
(1) SECTOR_MODE: Process only one sector of 512 bytes at a time, and iterate
chip-ecc.steps times.
(2) PAGE_MODE: Process complete page at a time, enabling multiple channels
simultaneously. This improves some throughput, especially for large-page
NAND devices and MLC NAND where bit-flips are common.
As ELM uses (2)nd approach, so the GPMC also has to calculate and store
ECC for complete page at a time. That is why trace NAND driver you will find
- OMAP_ECC_CODE_BCHx_HW_DETECTION_SW use generic implementation
chip-ecc.read_page= nand_read_page_hwecc() defined in nand_base.c
whereas,
- OMAP_ECC_CODE_BCHx_HW use custom implementation
chip-ecc.read_page= omap_read_page_bch() defined in omap.c
An I right that ecc_steps is nothing but number of sub-blocks ECC calculation
and correction
needs to be done for larger pages. This is a function of ECC hw capability
(chip-ecc.size)
and NAND flash capability (mtd-writesize). i.e. ecc_steps = mtd-writesize /
chip-ecc.size
We hardcode chip-ecc.size to 512 for all the ECC schemes in omap_nand_probe()
so
calculate and correct will always be called for 512 byte sized blocks. So when
does
the usecase for nsector 1 come in?
Ok.. I'll try to explain above details again in probably simplified version
- OMAP_ECC_CODE_BCHx_HW_DETECTION_SW
uses lib/bch.c (via nand_bch.c) to correct ECC. And is generic implementation
so here each sector (data chunk of ecc_size) is corrected independently.
So nsector = 1;
- OMAP_ECC_CODE_BCHx_HW
Uses ELM to correct ECC. But the way ELM driver is written today. It corrects
the whole page in single go. Not individual sectors (ecc_size chunks).
So, its doable to make it same like OMAP_ECC_CODE_BCHx_HW_DETECTION_SW
But then you _may_ have performance penalty for new technology NAND and MLC
NAND on which bit-flips are very common.
So to keep ELM driver as it is (performance tweaked), we use different
configurations in GPMC to read complete page in single go. This is where
nsector = chip-ecc.steps;
Hope that clarifies the implementation..
Good to document this detail somewhere for OMAP drivers both (u-boot and
kernel).
Any thoughts ?
with regards, pekon
cheers,
-roger
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 4/5] [RFC] ARM: OMAP2+: gpmc: fix gpmc_hwecc_bch_capable
Hi Cristoph,
From: Pekon Gupta pe...@ti.com
From: Christoph Fritz [mailto:chf.fr...@googlemail.com]
Most dt omap3 boards configure nand-ecc-opt as bch8. Due
to lack of hardware elm support, bch8 software implementation
gets set.
Since commit 0611c41934ab35ce84dea ARM: OMAP2+: gpmc: update
gpmc_hwecc_bch_capable() for new platforms and ECC schemes,
nand support stops working.
This patch allows ecc software fallback.
Pleas add following tag at top of commit message.
Fixes: commit 0611c41934ab35ce84dea34ab291897ad3cbc7be
ARM: OMAP2+: gpmc: update gpmc_hwecc_bch_capable() for new platforms and ECC
schemes
And please mark it for 3.15 stable if this gets accepted after 3.15-rcx.
Cc: sta...@vger.kernel.org # 3.15.x+
---
arch/arm/mach-omap2/gpmc-nand.c | 16 ++--
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c
index 4349e82..52c4834 100644
--- a/arch/arm/mach-omap2/gpmc-nand.c
+++ b/arch/arm/mach-omap2/gpmc-nand.c
@@ -43,13 +43,17 @@ static struct platform_device gpmc_nand_device = {
.resource = gpmc_nand_resource,
};
-static bool gpmc_hwecc_bch_capable(enum omap_ecc ecc_opt)
+static bool gpmc_ecc_bch_capable(enum omap_ecc ecc_opt)
I don't think this renaming is really required with this fix, so please
drop it and just keep it simple only for OMAP3 fix.
{
/* platforms which support all ECC schemes */
if (soc_is_am33xx() || cpu_is_omap44xx() ||
soc_is_omap54xx() || soc_is_dra7xx())
return 1;
+ if ((ecc_opt == OMAP_ECC_BCH4_CODE_HW_DETECTION_SW) ||
+ (ecc_opt == OMAP_ECC_BCH8_CODE_HW_DETECTION_SW))
+ return 1;
+
Note: Some very old legacy platforms have initial version of GPMC engine
which only supports Hamming ECC, and not BCH ECC scheme, so lets
filter them out to maintain backward compatibility.
(1) As per TRM, OMAP2 platform does not support BCH ECC schemes,
So please filter out OMAP2 devices from this check ...
(2) I don't know the history but below comment says:
* For now, assume 4-bit mode is only supported on OMAP3630 ES1.x, x=1
if ((ecc_opt == OMAP_ECC_BCH4_CODE_HW_DETECTION_SW) ||
(ecc_opt == OMAP_ECC_BCH8_CODE_HW_DETECTION_SW)) {
if (cpu_is_omap24xx())
return 0;
else if (cpu_is_omap3630() (GET_OMAP_REVISION() == 0))
return 0;
else
return 1;
}
/* OMAP3xxx do not have ELM engine, so cannot support ECC schemes
* which require H/W based ECC error detection */
if ((cpu_is_omap34xx() || cpu_is_omap3630())
@@ -57,14 +61,6 @@ static bool gpmc_hwecc_bch_capable(enum omap_ecc ecc_opt)
(ecc_opt == OMAP_ECC_BCH8_CODE_HW)))
return 0;
- /*
- * For now, assume 4-bit mode is only supported on OMAP3630 ES1.x, x=1
- * and AM33xx derivates. Other chips may be added if confirmed to work.
- */
- if ((ecc_opt == OMAP_ECC_BCH4_CODE_HW_DETECTION_SW)
- (!cpu_is_omap3630() || (GET_OMAP_REVISION() == 0)))
- return 0;
-
Thanks for removing this, I too wasn't confident of this either.
/* legacy platforms support only HAM1 (1-bit Hamming) ECC scheme */
if (ecc_opt == OMAP_ECC_HAM1_CODE_HW)
return 1;
@@ -140,7 +136,7 @@ int gpmc_nand_init(struct omap_nand_platform_data
*gpmc_nand_data,
gpmc_update_nand_reg(gpmc_nand_data-reg, gpmc_nand_data-cs);
- if (!gpmc_hwecc_bch_capable(gpmc_nand_data-ecc_opt)) {
+ if (!gpmc_ecc_bch_capable(gpmc_nand_data-ecc_opt)) {
Please drop this renaming from this patch.
dev_err(dev, Unsupported NAND ECC scheme selected\n);
return -EINVAL;
}
--
1.7.10.4
You may send this patch separately apart from the series, so that
this gets accepted in current 3.15-rcx.
with regards, Pekon
Do you plan to re-spin this patch with above comments, and mark it for stable?
It would be helpful for all OMAP3 users.
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v1 1/3] ARM: dts: am335x-bone: add support for beaglebone NAND cape
Hi Guido,
From: Guido Martínez
On Thu, Jun 26, 2014 at 03:40:44AM -0700, Tony Lindgren wrote:
* Pekon Gupta pe...@ti.com [140624 05:26]:
+
+gpmc {
+ ranges = 0 0 0 0x0100;/* address range = 16MB (minimum GPMC
partition) */
+ nand@0,0 {
+ status = disabled;
+ reg = 0 0 4; /* device IO registers */
Hmm so what about other capes potentially also using GPMC CS0?
Can you please do a check with two .dtsi files trying to use
GPMC CS0 and see what the produced .dtb file looks like after
decompiling it with dtc?
I'd assume the 16MB GPMC partition will be just fine for many
devices and can be also be larger if NOR needs it so maybe it
can be handled for almost all the cases.
The names for devices must be unique though to avoid them
getting merged. And I don't know if gpmc.c skips disabled devices
properly.
It doesn't. I've just sent a patch for it.
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266966.html
I don't think we need above patch.
Helper macro for_each_available_child_of_node internally takes care
of skipping nodes with status=disabled.
$KERNEL/include/linux/of.h
#define for_each_available_child_of_node(parent, child) \
for (child = of_get_next_available_child(parent, NULL); child != NULL; \
child = of_get_next_available_child(parent, child))
$KERNEL/drivers/of/base.c @@ of_get_next_available_child(...) {
...
if (!__of_device_is_available(next))
continue;
...
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v1 1/3] ARM: dts: am335x-bone: add support for beaglebone NAND cape
From: Tony Lindgren [mailto:t...@atomide.com]
* Gupta, Pekon pe...@ti.com [140627 14:08]:
From: Guido Martínez [mailto:gu...@vanguardiasur.com.ar]
On Tue, Jun 24, 2014 at 05:54:24PM +0530, Pekon Gupta wrote:
[...]
+gpmc {
+ ranges = 0 0 0 0x0100;/* address range = 16MB (minimum GPMC
partition) */
+ nand@0,0 {
+ status = disabled;
+ reg = 0 0 4; /* device IO registers */
+ pinctrl-names = default;
+ pinctrl-0 = bbcape_nand_flash_pins;
This doesn't seem to work as pinctrl properties are not parsed for
childs of the gpmc node. Did this work for you?
Putting this in the gpmc node makes it work, but how will we control
pins for the nand and nor independently? I believe there is currently no
support for muxing individual gpmc devices. If we want to add both
devices to the DT and enable them as needed we'd need to add support for
this, right?
Yes, And that should be the case, because different devices would be
connected to different chip-selects, so there should be support of
providing individual pin-mux for different GPMC devices.
Currently both NAND and NOR cape share GPMC_CS0, so both NAND and NOR
capes will not work simultaneously. But I'm planning to modify NOR cape
hardware at my end to use GPMC_CS1 instead of GPMC_CS0.
- NAND on GPMC_CS0
- NOR on GPMC_CS1
Hmm but we should have these working with both using CS0 without
any need to modify the hardware though?
Yes, but one at a time. That mean either of 'NAND cape' or 'NOR cape'.
If you need both working simultaneously as 2 separate devices attached
to GPMC, then you need 2 separate chip-selects, which is what I'm trying
to test with [1].
In that case we should make sure we always set large enough GPMC
partition
Yes, this is taken care with introduction of NOR cape in
[PATCH v1 2/3] ARM: dts: am335x-bone: add support for beaglebone NOR cape
gpmc {
- ranges = 0 0 0 0x0100;/* address range = 16MB (minimum GPMC
partition) */
+ ranges = 0 0 0x0800 0x0100; /* address offset=128MB,
range=128Mb=16MB */
This GPMC partition suffices for both NAND and NOR requirements.
and that the pins are muxed for the connected GPMC
devices only.
Pin mux is something I need to re-work, because currently
I've tested either NAND or NOR individually, not together.
Also as mentioned above by Guido, pin-ctrl property is not parsed individually
for GPMC children, Instead pin-ctrl is set once for GPMC as a whole.
Do you have any suggestions on how pin-ctrl should be set if we have
multiple devices connected to GPMC like;
All these devices will share:
- control lines (gpmc_wait, gpmc_ben_cle, gpmc_advn_ale, gpmc_we, gpmc_oe_ren)
- *some* data lines (gpmc_ad[])
- *some* address lines (gpmc_a[])
- but chip-selects will be different:
gpmc_cs0: NAND
gpmc_cs1: NOR
gpmc_cs2: SMSC91xx
gpmc_cs3: Camera
with regards, Pekon
Regards,
Tony
[1] http://www.spinics.net/lists/linux-omap/msg107950.html
RE: [PATCH v1 1/3] ARM: dts: am335x-bone: add support for beaglebone NAND cape
From: Guido Martínez [mailto:gu...@vanguardiasur.com.ar]
On Tue, Jul 01, 2014 at 07:01:03AM +, Gupta, Pekon wrote:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266966.html
I don't think we need above patch.
Helper macro for_each_available_child_of_node internally takes care
of skipping nodes with status=disabled.
$KERNEL/include/linux/of.h
#define for_each_available_child_of_node(parent, child) \
for (child = of_get_next_available_child(parent, NULL); child != NULL; \
child = of_get_next_available_child(parent, child))
$KERNEL/drivers/of/base.c @@ of_get_next_available_child(...) {
...
if (!__of_device_is_available(next))
continue;
...
Yes, that's why I suggest using that macro. It's not currently used
in gpmc.c and my patch changes the 'for_each_child_of_node' to a
'for_each_available_child_of_node'. Or am I missing something?
Oops my bad. I was probably dreaming, I already had your patch in my tree
before reviewing the code. So please ignore my previous email.
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] arm: mach-omap2: gpmc: ignore non-available nodes
From: Ezequiel García [mailto:ezequ...@vanguardiasur.com.ar] On 26 Jun 12:02 PM, Guido Martínez wrote: Currently, child nodes of the gpmc node are iterated and probed regardless of their 'status' property. This means adding 'status = disabled;' has no effect. This patch changes the iteration to only probe nodes marked as available. Signed-off-by: Guido Martínez gu...@vanguardiasur.com.ar Just a nit: the commit title doesn't match the recent commits. If you run git log on this file, you'll find the pattern should be something like: ARM: OMAP2+: GPMC should only probe enabled devices Other than this, the patch looks correct. Yes, plz keep patch title consistent as in other gpmc.c patches. And thanks for this fix. Tested-by: Pekon Gupta pe...@ti.com with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v1 3/3] ARM: dts: am335x-bone: add support for beaglebone LCD4 cape
Hi Daren, Guido,
From: Etheridge, Darren
On 06/26/2014 10:40 AM, Guido Martínez wrote:
I had some issues with this patch. Booting linux-next on a BeagleBone
Black with this exact LCD left me with an unusable white screen. Please
see below for some details.
On Tue, Jun 24, 2014 at 05:54:26PM +0530, Pekon Gupta wrote:
This patch adds support for LCD4 cape as advertised on
http://elinux.org/CircuitCo:BeagleBone_LCD4
This cape has:
* 480x272 TFT-LCD panel
- LCD panel datasheet and timing information are sourced from [1]
- LCD backlight is connected to 'EHRPWM1A' on cape board, but its used for
enabling backlight power-supply. So 'gpio-backlight' driver is used
instead
of 'pwm-backlight' driver (Kconfig: BACKLIGHT_GPIO=y).
* 4-wire resistive Touchscreen
*Known constrains*
As LCD panel pins (lcd_data, hsync, vsync, pclk) are shared with on-board
NXP HDMI framer, so either HDMI or LCD-cape can be used at time. Thus while
using this cape 'hdmi' DT node needs to be disabled in am335x-boneblack.dts
[1] www.newhavendisplay.com/specs/NHD-4.3-480272MF-ATXI-T-1.pdf
www.newhavendisplay.com/app_notes/OTA5180A.pdf
Signed-off-by: Pekon Gupta pe...@ti.com
---
snip
+
+ panel {
+ status = disabled;
+ compatible = ti,tilcdc,panel;
+ pinctrl-names = default;
+ pinctrl-0 = bbcape_lcd_pins;
+ panel-info {
+ ac-bias = 255;
+ ac-bias-intrpt= 0;
+ dma-burst-sz = 16;
+ bpp = 16;
+ fdd = 0x80;
+ sync-edge = 0;
+ sync-ctrl = 0;
I had to set this to 1. Does that make sense?
Yes, I'll check this one again at my end.
+ raster-order = 0;
+ fifo-th = 0;
+ };
+ display-timings {
+ native-mode = timing0;
+ /* www.newhavendisplay.com/app_notes/OTA5180A.pdf */
+ timing0: 480x272 {
+ clock-frequency = 3000;
+ hactive = 480;
+ vactive = 272;
+ hfront-porch = 8;
+ hback-porch = 47;
+ hsync-len = 41;
+ vback-porch = 2;
+ vfront-porch = 3;
+ vsync-len = 10;
+ hsync-active = 0;
+ vsync-active = 0;
+ de-active = 1;
+ pixelclk-active = 0;
Are you sure these timings are ok? When enabling the sync control I get
an usable display, but it looks a bit funny. For example an all black
display looks very clear and has a dotted pattern.
I tried to take a picture of it but it doesn't show.
I could run and test 'fbtest' and 'fbconsole' on this cape, using following
configs
CONFIG_DRM=y
CONFIG_DRM_TILCDC=y
[...]
CONFIG_FB=y
CONFIG_FIRMWARE_EDID=y
CONFIG_FRAMEBUFFER_CONSOLE=y
These timings look wrong to me, for instance this sets a clock frequency
of 30MHz where as the linked app-note says 9MHz. I think the LCD4 cape
uses the same LCD panel as is used on the AM335x EVMSK. Therefore the
display timings from my DT patch for the EVMSK should work:
https://patchwork.kernel.org/patch/4144801/
Yes, probably I got the pixel-clock timing wrong, I'll fix this.
It should be 9.2MHz as per
www.newhavendisplay.com/specs/NHD-4.3-480272MF-ATXI-T-1.pdf
Table: Electrical Characteristics
But as 'hsync' timings are relative to pixel-clock somehow it worked for me.
I got other vsync and hsync timings from following reference:
www.newhavendisplay.com/app_notes/OTA5180A.pdf
Table: 8.4.1 480XRGBX272 Vertical Timing
Table: 8.4.2 480XRGBX272 Horizontal Timing
And I/O signal polarity for vsync-active and hsync-active from
Table: 5. SIGNAL DESCRIPTIONS
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v1 1/3] ARM: dts: am335x-bone: add support for beaglebone NAND cape
From: Guido Martínez [mailto:gu...@vanguardiasur.com.ar]
On Tue, Jun 24, 2014 at 05:54:24PM +0530, Pekon Gupta wrote:
[...]
+gpmc {
+ranges = 0 0 0 0x0100;/* address range = 16MB (minimum GPMC
partition) */
+nand@0,0 {
+status = disabled;
+reg = 0 0 4; /* device IO registers */
+pinctrl-names = default;
+pinctrl-0 = bbcape_nand_flash_pins;
This doesn't seem to work as pinctrl properties are not parsed for
childs of the gpmc node. Did this work for you?
Putting this in the gpmc node makes it work, but how will we control
pins for the nand and nor independently? I believe there is currently no
support for muxing individual gpmc devices. If we want to add both
devices to the DT and enable them as needed we'd need to add support for
this, right?
Yes, And that should be the case, because different devices would be
connected to different chip-selects, so there should be support of
providing individual pin-mux for different GPMC devices.
Currently both NAND and NOR cape share GPMC_CS0, so both NAND and NOR
capes will not work simultaneously. But I'm planning to modify NOR cape
hardware at my end to use GPMC_CS1 instead of GPMC_CS0.
- NAND on GPMC_CS0
- NOR on GPMC_CS1
In addition to pin-mux you may also require following patch:
http://www.spinics.net/lists/linux-omap/msg107950.html
Also, I should have marked this series as RFC as its not fully tested.
My main intention was to get acknowledgement about cape DTS from
various users and Tony Lindgren t...@atomide.com.
Now as Tony has given some acceptance for these kind of cape DTS,
I'll clean-up and re-send these patches with better testing and GPMC fixes.
with regards, pekon
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v1 1/3] ARM: dts: am335x-bone: add support for beaglebone NAND cape
From: Ezequiel Garcia [mailto:ezequ...@vanguardiasur.com.ar]
On 24 Jun 05:54 PM, Pekon Gupta wrote:
+gpmc {
+ranges = 0 0 0 0x0100;/* address range = 16MB (minimum GPMC
partition) */
+nand@0,0 {
+status = disabled;
+reg = 0 0 4; /* device IO registers */
+pinctrl-names = default;
+pinctrl-0 = bbcape_nand_flash_pins;
+ti,nand-ecc-opt = bch8;
+ti,elm-id = elm;
Don't you need something like this to turn on the elm device?
elm {
status = okay;
};
No, This was the intend of these patches to keep cape DTS as disabled
so that:
(1)These patches should not change the behavior of existing DTS
(am335x-boneblack.dts or am335x-bone.dts), or conflict with
any existing node or other cape DTS.
(2) In-case of share-pin mux where multiple capes use same pins
Cape DTS should not conflict each other.
Now, to enable the choice of cape without hacking any source file,
user can use u-boot commands (refer to example in cover-letter):
/* load DTB */
u-boot tftp 0x8100 am335x-boneblack.dtb
u-boot fdt addr 0x8100
/* disabled conflicting MMC2 (eMMC) */
u-boot fdt set /ocp/mmc@481d8000 status \d\i\s\a\b\l\e\d
/* enable NAND cape*/
u-boot fdt set /ocp/elm status \o\k\a\y
u-boot fdt set /ocp/gpmc status \o\k\a\y
u-boot fdt set /ocp/gpmc/nand status \o\k\a\y
And then boot using this DTB loaded at 0x8100
(you can put all these commands in uEnv as part of $bootcmd)
with regards, Pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v7 1/4] ARM: dts: am335x-bone: add support for beaglebone NAND cape
From: Jason Kridner [mailto:jkrid...@gmail.com]
On Mon, Jun 23, 2014 at 1:48 AM, Tony Lindgren t...@atomide.com wrote:
* Gupta, Pekon pe...@ti.com [140622 22:42]:
From: Tony Lindgren [mailto:t...@atomide.com]
[...]
Based on the recent discussions on the capes, it seems that nobody
wants to implement toggling of the capes in u-boot. And as there
can be other capes using GPMC bus, we can't merge this.
I have been able to get toggling of capes (enabling and disabling of DT
nodes)
in u-boot. It was already there in u-boot mainline [1], may be no-body
tried it.
Example: Below sequence works for NAND cape patch mentioned in this thread.
---
/* load DTB */
u-boot tftp 0x8100 am335x-boneblack.dtb
u-boot fdt addr 0x8100
/* disable MMC2 node */
u-boot fdt list /ocp/mmc@481d8000
u-boot fdt set /ocp/mmc@481d8000 status \d\i\s\a\b\l\e\d
u-boot fdt list /ocp/mmc@481d8000 status
/* enable GPMC node */
u-boot fdt list /ocp/gpmc
u-boot fdt set /ocp/gpmc status \o\k\a\y
u-boot fdt list /ocp/gpmc status
/* enable ELM node */
u-boot fdt list /ocp/elm
u-boot fdt set /ocp/elm status \o\k\a\y
u-boot fdt list /ocp/elm status
/* boot uImage */
tftp 0x8200 uImage
bootm 0x8200 - 0x8100
Note: fdt set command does not accept string literals
as binding values, it internally converts them to string, so
escape sequenced characters were used here..
okay == \o\k\a\y
disabled == \d\i\s\a\b\l\e\d
Cool. Now all we need is a few helper functions in u-boot
so it can be done a bit easier :)
The association of devicetree overlay files in /lib/firmware to cape
IDs made it where the kernel-based cape overlay manager could modify
the devicetree as needed without putting extensive cape-specific logic
in the kernel or bootloader. Throwing a bunch of capes into a single
class of capes won't save any work there.
I'm classifying capes based on functionality to reduce the number of
DTS files, because in general people will not both LCD4 and LCD7 capes
together, similarly not many will use NAND, NOR and eMMC simulataneously.
- am335x-bone-memory-cape.dts: will have NAND, NOR and storage related capes
- am335x-bone-display-cape.dts: will have LCD4, LCD7, ... etc capes.
If we can get the bootloader
to support the .dtbo files, then we can continue to use the ID in the
cape to provide all the DT modifications we need. If we need to do
scripts for the modifications, we'd still need to associate the cape
ID to that script.
With ID based auto-identification of Capes, we need EEPROM or
e-fuses on every cape. And today I don't think all third-party capes
have EEPROM on them.
Also, with increase in number of cape, it will be hard to maintain
ID grouping and updating firmware/software for each new cape.
This still doesn't cover the need for run-time devicetree
modification, but I'll leave that for the other on-going thread.
I'm just proposing the u-boot way of enabling/disabling DT.
This is certainly not replacing requirement for DT overlay manager,
But till the time DT overlay is available in kernel, this would at-least
be an placeholder and would encourage Beaglebone users to
continue using mainline kernel.
This why I'm trying to get basic cape DTS in mainline, so that
Beaglebone community continues to be in touch with mainline.
Tomorrow if DT overlay comes, then we may choose to either keep
these cape DTS, or delete them from mainline without breaking any
backward compatibility.
I do agree with the idea of moving more of the potential sources of
conflict that can be resolved out of the overlays and into the main
devicetree, leaving the overlays or scripts simply to deal with the
conflicts that cannot be resolved at run-time given the current
infrastructure. I just think they should go into .dtsi (include) files
for the main .dts/.dtb files for the boards, rather than additional
overlays or alternative .dts files.
Yes, that is the main motive, to resolve _known_ conflicts before
Run-time. And keep things working till 'DT overlay' is mainlined.
Another thing which I'm not sure is how DT overlay will work
with capes which are required during boot?
Example: NAND, NOR capes themselves containing file-system.
I think in such scenarios using u-boot approach for enabling/disabling
capes would be beneficial.
If you and others are okay with this approach, I would request you
to please provide your Tested-by for few cape DTS patches I
will be posting soon. This would give confidence that cape DTS
patches are not breaking anything in existing DTS.
Thanks..
with regards, pekon
N�r��yb�X��ǧv�^�){.n�+{��f��{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj��!�i
RE: [PATCH v1 3/3] ARM: dts: am335x-bone: add support for beaglebone LCD4 cape
From: Ezequiel Garcia On 24 Jun 05:54 PM, Pekon Gupta wrote: This patch adds support for LCD4 cape as advertised on http://elinux.org/CircuitCo:BeagleBone_LCD4 This cape has: * 480x272 TFT-LCD panel - LCD panel datasheet and timing information are sourced from [1] - LCD backlight is connected to 'EHRPWM1A' on cape board, but its used for enabling backlight power-supply. So 'gpio-backlight' driver is used instead of 'pwm-backlight' driver (Kconfig: BACKLIGHT_GPIO=y). I'm confused about this, can you clarify why you are not using pwm-backlight? As per the schematics of this LCD4 cape board, EHRPWM1A pin controls enabling/disabling of the power to backlight LED. It does not control the voltage levels (brightness levels) of the LED. Thus it wasn't making sense to use pwm-backlight driver which more suitable in cases where you have multiple levels of brightness. Here, you have only 2 levels backlight=off | on, so I used gpio-backlight driver. Though you can use pwm-backlight driver also, but the backlight turns ON only when you set the /sys/class/backlight/backlight_device/brighteness to maximum level (as set in DT). with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v1 3/3] ARM: dts: am335x-bone: add support for beaglebone LCD4 cape
From: Jason Kridner [mailto:jkrid...@gmail.com]
On Tue, Jun 24, 2014 at 8:24 AM, Pekon Gupta pe...@ti.com wrote:
This patch adds support for LCD4 cape as advertised on
http://elinux.org/CircuitCo:BeagleBone_LCD4
[...]
diff --git a/arch/arm/boot/dts/am335x-bone-display-cape.dts
b/arch/arm/boot/dts/am335x-bone-
display-cape.dts
If this is mean to be included, why not use the .dtsi extension rather
than .dts. .dts implies it is a top-level file.
I followed the ideology given in in below discussion
http://comments.gmane.org/gmane.linux.drivers.devicetree/13921
As I understood .dtsi file extension should be used for silicon DT data which
which remains common for all the boards using that silicon (like am33xx.dtsi).
And .dts is for board specific DT data.
However, anything works for me, as its just matter of file-name-extension.
So should I rename all files to .dtsi ?
[...]
+/ {
+ backlight {
+ status = disabled;
+ compatible = gpio-backlight;
+ pinctrl-names = default;
+ pinctrl-0 = bbcape_backlight_pins;
+ gpios = gpio1 18 GPIO_ACTIVE_HIGH;
+ default-on;
+ };
+
+ panel {
+ status = disabled;
Where are you expecting to enable this? I'm guessing through your
u-boot hacking? Certainly I can see editing the boot script to make
this work, but I don't see how this results in the average user simply
plugging in the cape and having it work without having to edit files.
Yes, using u-boot commands, *without touching any source file*.
I'm planning to send patch for adding following helper commands in u-boot
u-boot fdt node enable node-path
u-boot fdt node disable node-path
If that's accepted, enabling disabling capes will become very easy.
And, as you said user can put these commands in scripts (like Uenv.txt)
or environment variable, and execute them automatically at each boot.
Perhaps you can put some real logic into the bootloader that looks at
the cape EEPROMs?
No, I don't want to go the EEPROM way, because of reasons suggested
in earlier mail. It's not scalable, especially when you don't have control
over what type and how third-party is developing the capes.
Have you considered using tools like pinmux-helper as is done with the
cape-universal overlay?
https://github.com/beagleboard/devicetree-source/blob/master/arch/arm/boot/dts/cape-universal-
00A0.dts
Yes, I have seen that. In pin-mux helper pins are configured not based on
their actual names but as they appear on P8 and P9 headers of Beaglebone.
- For shared pins, you have defined all possible functionality.
- For dedicated pins, there is single fragment.
Now there are two questions..
(1) Do we need to update this cape-universal-xx.dts everytime a new
Cape in market uses some dedicated pin for some muxed functionality ?
(2) I'm not sure but kernel does _not_ free the memory allocated to
DT fragment. If so, this universal fragment which has pin-mux for all
P8 and P9 pins will block the memory forever. Would be good to know
the in-memory size required for this universal fragment ?
I've been hacking with adding all of these pinmux helpers to the main
.dts. I think you are encouraging me to add it to include files to
make it a bit more granular.
http://paste.debian.net/106319/
Yes, using include files would be good.
Also one minor feedback. Please use macros for Pin directions, Pulls, etc
instead of hex numbers, it make it more readable. And then you don't need
to specify that in comments.
- 0xa0 0x08/* lcd_data0.lcd_data0, OMAP_MUX_MODE0 | AM33XX_PIN_OUTPUT |
AM33XX_PULL_DISA */
+ 0xa0 (OMAP_MUX_MODE0 | AM33XX_PIN_OUTPUT | AM33XX_PULL_DISA)/*
lcd_data0.lcd_data0 */
I appreciate the enthusiasm to get this support into the mainline. If
it goes, I'll try to follow with a bunch of other outstanding changes
we have sitting in the vendor tree right now.
Thanks. Waiting for Tony to at-least 'Ack' or say if this is acceptable
approach.
Have you looked at the features in the devicetree for this cape that
is currently pushed in the vendor tree?
https://github.com/beagleboard/devicetree-source/blob/master/arch/arm/boot/dts/BB-BONE-LCD4-
01-00A1.dts
Yes, I have not added touchscreen support.
I developed this DTS independently while debugging some Display v/s NAND issue.
If these DTS patches are acceptable, then I'll refine them further.
with regards, pekon
RE: [PATCH v7 1/4] ARM: dts: am335x-bone: add support for beaglebone NAND cape
Hi Tony,
Just reviving this thread for some information..
From: Tony Lindgren [mailto:t...@atomide.com]
Sent: Monday, May 19, 2014 9:56 PM
To: Gupta, Pekon
Cc: linux-omap; Ezequiel Garcia; Stefan Roese; Javier Martinez Canillas;
Quadros, Roger
Subject: Re: [PATCH v7 1/4] ARM: dts: am335x-bone: add support for beaglebone
NAND cape
* Pekon Gupta pe...@ti.com [140519 02:16]:
--- a/arch/arm/boot/dts/am335x-bone.dts
+++ b/arch/arm/boot/dts/am335x-bone.dts
@@ -9,6 +9,7 @@
#include am33xx.dtsi
#include am335x-bone-common.dtsi
+#include am335x-bone-memory-cape.dts
ldo3_reg {
regulator-min-microvolt = 180;
--- a/arch/arm/boot/dts/am335x-boneblack.dts
+++ b/arch/arm/boot/dts/am335x-boneblack.dts
@@ -9,6 +9,7 @@
#include am33xx.dtsi
#include am335x-bone-common.dtsi
+#include am335x-bone-memory-cape.dts
ldo3_reg {
regulator-min-microvolt = 180;
Based on the recent discussions on the capes, it seems that nobody
wants to implement toggling of the capes in u-boot. And as there
can be other capes using GPMC bus, we can't merge this.
I have been able to get toggling of capes (enabling and disabling of DT nodes)
in u-boot. It was already there in u-boot mainline [1], may be no-body tried it.
Example: Below sequence works for NAND cape patch mentioned in this thread.
---
/* load DTB */
u-boot tftp 0x8100 am335x-boneblack.dtb
u-boot fdt addr 0x8100
/* disable MMC2 node */
u-boot fdt list /ocp/mmc@481d8000
u-boot fdt set /ocp/mmc@481d8000 status \d\i\s\a\b\l\e\d
u-boot fdt list /ocp/mmc@481d8000 status
/* enable GPMC node */
u-boot fdt list /ocp/gpmc
u-boot fdt set /ocp/gpmc status \o\k\a\y
u-boot fdt list /ocp/gpmc status
/* enable ELM node */
u-boot fdt list /ocp/elm
u-boot fdt set /ocp/elm status \o\k\a\y
u-boot fdt list /ocp/elm status
/* boot uImage */
tftp 0x8200 uImage
bootm 0x8200 - 0x8100
Note: fdt set command does not accept string literals
as binding values, it internally converts them to string, so
escape sequenced characters were used here..
okay == \o\k\a\y
disabled == \d\i\s\a\b\l\e\d
---
And because
the capes are stackable, we can't really have .dts files for all
the combinations. And no, we're not merging any unconnected .dts
files either, sorry.
As per earlier proposal, we can have single DT files for similar
functionality capes like;
- am335x-bone-memory-cape.dts: for all Flash/Memory related capes like NAND,
NOR, eMMC, etc.
- am335x-bone-display-cape.dts: for all display related capes like LCD4, LCD7..
This way you will have countable number of DTS files to maintain
And any conflict if ever in between capes can be contained within these files.
Does this suffice the requirement to accept cape DTS in mainline
(with default state as disabled) ?
I'm re-invoking this thread because there are quite a number of
hobbyist and developers stuck without proper DT support of these
capes, so having them in mainline is better than providing a internal
or separately maintained tree.
Regards,
Tony
with regards, pekon
[1] http://www.denx.de/wiki/view/DULG/UBootCmdFDT
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Unifying cape overlays into boot .dtb for BeagleBoard.org boards
Hi,
From: Jason Kridner [mailto:jkrid...@gmail.com]
On Tue, Jun 17, 2014 at 3:11 AM, Gupta, Pekon pe...@ti.com wrote:
From: Jason Kridner
[...]
* The devicetree sources, including the primary boot .dts files, will
eventually be removed from the kernel source tree. I'm not too sure if
and when it'll really happen, but starting up a project to maintain
the definitive beagleboard.org board devicetree files outside the
kernel seems to make sense. Given the interdependency of the boot .dtb
and the overlay .dtbo files, combining them into a single repository
where every distribution can pick them up seems like a natural and
obvious choice. There are of course some dependencies on kernel
versions, but I believe most of those have settled out by now and we
should be OK moving forward.
+1
Yes, moving cape DTS out of kernel tree should help developers.
There are quite a few cape patches floating in mail-lists, but as cape
DTS is still not accepted in mainline so they get lost and forgotten.
So one place for collecting all this is a good idea.
However, somehow the universal I/O DTS looked seemed complicated:
(1)
All capes work standalone, but due to share pin-mux some cape
combinations cannot be used simultaneously. But most users of
BeagleBone are already well-versed with DT and kernel infrastructure,
so they need not be spoon fed to get a out-of-box working solution
for each combination. If there is proper documentation is available
about compatibility of capes with each other, then users will figure
out themselves.
I think you have too much confidence in users. If this doesn't hurt
power users, then why is it bad have an option to spoon feed? This
doesn't prevent anyone with knowledge of DT from doing their own
thing.
Fair enough.
But plz give a try to u-boot alternative below. It works at my end.
(2)
Also, there was a talk of enabling and disabling DT fragments via u-boot.
That should also be explored instead of complicating cape DTS.
Link? Relevance?
we can modify DT from u-boot itself [1].
Example: MMC2 pin-mux conflicts with NAND and NOR capes.
But using following sequence of commands, you can modify DTB via
u-boot and make NAND cape work _without_any_hack_ in patch [2].
/* load DTB */
u-boot tftp 0x8100 am335x-boneblack.dtb
u-boot fdt addr 0x8100
/* disable MMC2 node */
u-boot fdt list /ocp/mmc@481d8000
u-boot fdt set /ocp/mmc@481d8000 status \d\i\s\a\b\l\e\d
u-boot fdt list /ocp/mmc@481d8000 status
/* enable GPMC node */
u-boot fdt list /ocp/gpmc
u-boot fdt set /ocp/gpmc status \o\k\a\y
u-boot fdt list /ocp/gpmc status
/* enable ELM node */
u-boot fdt list /ocp/elm
u-boot fdt set /ocp/elm status \o\k\a\y
u-boot fdt list /ocp/elm status
/* boot uImage */
tftp 0x8200 uImage
bootm 0x8200 - 0x8100
Note: fdt set command does not accept string literals
as binding values, it internally converts them to string, so
escape sequenced characters were used here..
okay == \o\k\a\y
disabled == \d\i\s\a\b\l\e\d
Hope above solves the pre-requisite because of which 'Tony Lindgren
t...@atomide.com'
was unable to accept cape related DTS into his tree [3]
[1] http://www.denx.de/wiki/view/DULG/UBootCmdFDT
[2] http://www.spinics.net/lists/linux-omap/msg107307.html
[3] http://www.spinics.net/lists/linux-omap/msg107354.html
with regards, pekon
N�r��yb�X��ǧv�^�){.n�+{��f��{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj��!�i
RE: Unifying cape overlays into boot .dtb for BeagleBoard.org boards
Hi Jason, From: Jason Kridner Adding devicetree and linux-arm-kernel lists based on feedback on IRC... On Tue, Jun 10, 2014 at 12:46 PM, Jason Kridner jkrid...@gmail.com wrote: I'd like to discuss moving our current library of cape devicetree overlay sources into a single tree, including the boot .dtb files for BeagleBoard.org boards and moving towards enabling as much of the cape support into a single boot-time .dtb file with an approach similar to the cape-universal overlay (https://github.com/cdsteinkuehler/beaglebone-universal-io), but not in an overlay. First of all, I want to note this doesn't change my view on the importance of mainline support for devicetree overlays. They are still absolutely critical and highly useful, solving problems that cannot be solved through boot-time devicetrees. I'm simply looking for an approach that will complement the availability of overlays and provide the best user experience. Robert has been talking about the actions required to clean-up Debian Jessie support in another thread (https://groups.google.com/d/msg/beagleboard/2b8rArtfABY/A8d1JzmJa4IJ) and I suggested we should add a bit of a detour by cleaning up the cape support for the mainline kernel and switching away our primary process of supporting capes from using overlays to using a single devicetree file provided at boot. I promised a pull-request and hadn't gotten around to sending it until now (below). No architecture changes have been made in my pull-request, just bringing in the kernel devicetree source history. This suggestion is based on several assumptions, any number of which might be wrong. The assumptions (for which I'm looking for feedback/corrections): * The overlays pretty much all need to be compiled into the kernel if they are going to be loaded using kernel command-line arguments or /etc/capemgr for the majority of distros. While many cape devicetree use cases are perfectly happy with loading at run-time rather than boot-time, it seems there should be a mechanism for pushing cape support into the category of being available at boot-time across distributions. * The devicetree sources, including the primary boot .dts files, will eventually be removed from the kernel source tree. I'm not too sure if and when it'll really happen, but starting up a project to maintain the definitive beagleboard.org board devicetree files outside the kernel seems to make sense. Given the interdependency of the boot .dtb and the overlay .dtbo files, combining them into a single repository where every distribution can pick them up seems like a natural and obvious choice. There are of course some dependencies on kernel versions, but I believe most of those have settled out by now and we should be OK moving forward. +1 Yes, moving cape DTS out of kernel tree should help developers. There are quite a few cape patches floating in mail-lists, but as cape DTS is still not accepted in mainline so they get lost and forgotten. So one place for collecting all this is a good idea. However, somehow the universal I/O DTS looked seemed complicated: (1) All capes work standalone, but due to share pin-mux some cape combinations cannot be used simultaneously. But most users of BeagleBone are already well-versed with DT and kernel infrastructure, so they need not be spoon fed to get a out-of-box working solution for each combination. If there is proper documentation is available about compatibility of capes with each other, then users will figure out themselves. (2) Also, there was a talk of enabling and disabling DT fragments via u-boot. That should also be explored instead of complicating cape DTS. (3) BeagleBone community and outside are coming out with new featured capes, which might possess a challenge to get a absolute non-conflicting universal I/O DTS. What might be working today might have conflicts tomorrow with introduction of new capes. We should be open to be compatible to capes developed outside Beaglebone.org. In my view this is important for longer term. Example: http://valentfx.com/logi-bone/ So, plz review my feedback and if you plan to create an external tree for beaglebone Capes, I would be happy to submit some cape patches. with regards, pekon
RE: [PATCH 05/36] mtd: nand: omap: Move IRQ handling from GPMC to NAND driver
From: Tony Lindgren [mailto:t...@atomide.com] * Roger Quadros rog...@ti.com [140613 00:40]: On 06/13/2014 10:18 AM, Tony Lindgren wrote: * Roger Quadros rog...@ti.com [140611 01:58]: Since the Interrupt Events are used only by the NAND driver, there is no point in managing the Interrupt registers in the GPMC driver and complicating it with irqchip modeling. Let's manage the interrupt registers directly in the NAND driver and get rid of irqchip model from GPMC driver. Get rid of IRQ commands and unused commands from gpmc_configure() in the GPMC driver. This seems like a step backward to me. The GPMC interrupt enable register can do edge detection on the wait pins, how is that limited to NAND? OK. But wait pin edge detection was not yet being used and I couldn't think of how it would ever be used. Any ideas? Maybe to wake-up the system on bus activity or something? Sorry, I wasn't able to review this series. But just as pointer, GPMC driver was used for interfacing many non-memory devices like Ethernet (smc91x) and in past GPMC has been proved to work with camera devices too, but that's wasn't mainlined. So keeping IRQ and few other things in GPMC driver is helpful. Further, let's not start mixing GPMC hardware module register access with the NAND driver register access. They can be clocked separately. And bugs in the NAND driver can cause issues in other GPMC using drivers. I understood that NAND controller is integrated into the GPMC module and they are clocked the same. Not sure why the hardware designers would keep the registers so closely knit. Yeah. Maybe regmap could provide some abstraction to the the NAND registers. As you mentioned, GPMC has two set of registers: (a) Chip-select registers (CONFIGx_cs) for device specific parameters (like device-width, signal-timings, etc) which are statically programmed during probe or via DT. (b) ECC registers which are continuously reconfigured based on ECC engine. *Ideal Scenario* NAND driver should be considered equivalent to protocol driver, Therefore ideally it should use only those registers which are specific to NAND (b). *Actual Scenario* But most NAND device today are ONFI compliant and they have almost all device parameters like device-width, signal-timings burned on-die in an ONFI page. These values are read back from NAND device during device_probe() and then re-configured back Chip-select registers (a). Hence NAND driver needs access of both (a) and (b), which is why You need to export complete GPMC register set to NAND driver. However this is not the case and has been discussed earlier too.. http://lists.infradead.org/pipermail/linux-mtd/2013-October/049284.html http://lists.infradead.org/pipermail/linux-mtd/2013-October/049347.html (Just pointing out my version of history, would be good to read the entire discussion. But the summary was that we need to re-configure some GPMC chip-select registers (a) based on probe done in NAND driver. So we need all GPMC registers exposed to NAND driver). FYI. memory/ti-amif.c and mtd/nand/davinci_nand.c share the AMIF register space in the same way. I thought it'd be nice to be consistent across TI drivers. Probably they did not yet learn the problems caused by it :) I havn't reviewed the ti-amif.c driver completely but I think they too configure device signal timing statically based on DT. But as per today this is frowned upon because: (1) Its difficult for layman user to decipher NAND signal timings from datasheet and then convert it into controller understandable DT (2) ONFI parameter page on NAND has these timings specified on-die itself, and these timings are characterized for best performance so NAND driver should re-configure these timings after probe. Refer below mail from 'Rob Herring robherri...@gmail.com' http://lists.infradead.org/pipermail/linux-mtd/2014-April/053488.html Considering all these details, please re-review the changes you plan for GPMC driver. with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: am335x: system doesn't reboot after flashing NAND
Hi Roger, Yegor,
From: Quadros, Roger
On 06/04/2014 10:45 PM, Yegor Yefremov wrote:
[...]
The 0x8000 address seems to be the beginning of NAND region:
ranges = 0 0 0x0800 0x1000; /* CS0: NAND */
I've taken this example from am335x_evm.dts. I have tried to change
the mapping to 0x9000, but kernel still uses 0x8000, Where in
the kernel will ranges be evaluated? I'm digging thorugh
arch/arm/mach-omap2/gpmc.c and gpmc-nand.c, but didn't really found
the place.
Well it doesn't. It just uses whatever was setup by the bootloader or
randomly allocated by gpmc_cs_request().
I'm working on fixing this up. I should be posting v2 of the GPMC refactor
series by this week and you should be able to map the CS region as specified
in the DT.
Till then, maybe you can pre-configure CS0 in the bootloader to wherever you
want
or alternatively call gpmc_cs_remap() after the gpmc_cs_request() in
gpmc_nand_init();
cheers,
-roger
I had a fix for this gpmc_cs_remap issue, it worked for beaglebone NOR,
but havn't checked on other devices. So please see if it works for you.
However, I don't suspect XIP boot here because even if CPU is detecting
garbage data while reading from 0x8000 it won't find a valid image
header there, and so XIP boot should have failed, and boot sequence
should fall back to MMC. It shouldn't have hung.
Still you can try below patch for gpmc_cs_remap().
---
From: Pekon Gupta pe...@ti.com
Date: Fri, 9 May 2014 15:17:32 +0530
Subject: [PATCH 12/14] ARM: OMAP2+: fix gpmc_cs_remap: re-allocating
chip-select address space based on DT
Each GPMC chip-select needs to be configured for (base-address,CS-size) so that
GPMC understands the address-space allocated to device connected externally.
These chip-select configurations (base-address, CS-size) follow some basic
mapping rules like:
- The CS size is programmable from 256 MBytes to 16 MBytes (must be a power of
2)
and is defined by the mask field. Attached memory smaller than the programmed
CS region size is accessed through the entire CS region (aliasing).
- The programmed 'base-address' must be aligned to the 'CS-size' boundary and
be a power of 2.
- Valid CS-size values are {256MB(max), 128MB, 64MB, 32MB and 16MB (min)}
Any intermediate values creates holes in the chip-select memory-map.
This patch adds above checks in gpmc_cs_remap() so that any invalid value
passed by DT reg property can be filtered before actually allocating the
address space.
Signed-off-by: Pekon Gupta pe...@ti.com
---
arch/arm/mach-omap2/gpmc.c | 42 +-
1 file changed, 29 insertions(+), 13 deletions(-)
diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 4fc4963..224550e 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -521,26 +521,42 @@ static int gpmc_cs_delete_mem(int cs)
* base. Returns 0 on success and appropriate negative error code
* on failure.
*/
-static int gpmc_cs_remap(int cs, u32 base)
+static int gpmc_cs_remap(int cs, u32 base, u32 size)
{
int ret;
- u32 old_base, size;
if (cs gpmc_cs_num) {
pr_err(%s: requested chip-select is disabled\n, __func__);
return -ENODEV;
}
- /*
-* Make sure we ignore any device offsets from the GPMC partition
-* allocated for the chip select and that the new base confirms
-* to the GPMC 16MB minimum granularity.
-*/
- base = ~(SZ_16M - 1);
-
- gpmc_cs_get_memconf(cs, old_base, size);
- if (base == old_base)
- return 0;
+ /* allocate enough address-space under GPMC chip-select to device */
+ if (size SZ_256M) {
+ pr_err(%s: memory device 256MB not supported\n, __func__);
+ return -ENODEV;
+ } else if (size SZ_128M) {
+ WARN((size != SZ_256M), cs=%d: allocating 256MB\n, cs);
+ size = SZ_256M;
+ } else if (size SZ_64M) {
+ WARN((size != SZ_128M), cs=%d: allocating 128MB\n, cs);
+ size = SZ_128M;
+ } else if (size SZ_32M) {
+ WARN((size != SZ_64M), cs=%d: allocating 64MB\n, cs);
+ size = SZ_64M;
+ } else if (size SZ_16M) {
+ WARN((size != SZ_32M), cs=%d: allocating 64MB\n, cs);
+ size = SZ_32M;
+ } else {
+ WARN((size != SZ_16M), cs=%d: allocating 64MB\n, cs);
+ size = SZ_16M;
+ }
+
+ /* base address should be aligned with address-space size */
+ if (base (size - 1)) {
+ pr_err(base-addr=%x should be aligned to size=%x, base, size);
+ return -EINVAL;
+ }
+
gpmc_cs_disable_mem(cs);
ret = gpmc_cs_delete_mem(cs);
if (ret 0)
@@ -1551,7 +1567,7 @@ static int gpmc_probe_generic_child(struct
platform_device *pdev,
* CS to this location. Once DT migration is
RE: am335x: system doesn't reboot after flashing NAND
Hi Yegor,
From: Gupta, Pekon
However, I don't suspect XIP boot here because even if CPU is detecting
garbage data while reading from 0x8000 it won't find a valid image
header there, and so XIP boot should have failed, and boot sequence
should fall back to MMC. It shouldn't have hung.
I checked with Sekhar on this, and my understanding was incorrect here.
There is no image verification (or marker) check done for XIP boot.
As mentioned by Sekhar earlier, XIP boot just checks for non-0x0 and
non-0xFF data on 0x8000. And it treats any value as valid code image
and starts executing it.
So, please ignore my above statement.
with regards, pekon
N�r��yb�X��ǧv�^�){.n�+{��f��{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj��!�i
RE: am335x: system doesn't reboot after flashing NAND
On Wednesday 04 June 2014 04:00 PM, Yegor Yefremov wrote: [...] The way ROM works for XIP boot is: 1) Set chip select 0 base address to 0x0800' 2) Read memory at 0x0800' 3) If something else other than 0x0 or ~0x0 is found, jump to 0x0800' and start executing. Can you check to see the contents of 0x0800' before and after nand write using mtdblock? Before writing: # devmem 0x0800 32 0x After writing: # devmem 0x0800 32 0xE0E0E0E0 Okay, so this is the cause of failure to boot. I am not sure what operation by NAND driver causes this value to change. Perhaps you could bisect a bit by dumping this address at various points during the write operation? If you have a debugger it will become easy to do this. I'm not sure, if a NAND device is connected as CS0 how is ROM code detecting it as NOR and proceeding for an XIP read. As its mentioned in XIP boot process in AM335x TRM that if an invalid boot image is found then ROM code falls back to next boot device which is in this case MMC boot. A hung code here mean - XIP boot is not failing OR - ROM code is waiting on GPMC_WAIT0 pin a part of XIP read access. So please check if the GPMC: (1) This may happen because GPMC pins are shared for both NOR and NAND so plz check conflicting pins as given in AM335x TRM: Table 26-9. Pins Used for Non-Muxed NOR Boot (Please see that GPMC_WAIT0 and GPMC_WAIT1 are pulled high on the board) (2) Some SYSBOOT configuration might also be confusing ROM code. If you are using x8 NAND device, then please try with setting SYSBOOT[8] = 1 SYSBOOT[11: 10] == 01 or 11 (reserved values) This should fail XIP boot and make the ROM code fall back to next boot device which is MMC in this case. with regards, pekon
RE: [PATCH 4/5] [RFC] ARM: OMAP2+: gpmc: fix gpmc_hwecc_bch_capable
From: Christoph Fritz [mailto:chf.fr...@googlemail.com]
Most dt omap3 boards configure nand-ecc-opt as bch8. Due
to lack of hardware elm support, bch8 software implementation
gets set.
Since commit 0611c41934ab35ce84dea ARM: OMAP2+: gpmc: update
gpmc_hwecc_bch_capable() for new platforms and ECC schemes,
nand support stops working.
This patch allows ecc software fallback.
Pleas add following tag at top of commit message.
Fixes: commit 0611c41934ab35ce84dea34ab291897ad3cbc7be
ARM: OMAP2+: gpmc: update gpmc_hwecc_bch_capable() for new platforms and ECC
schemes
And please mark it for 3.15 stable if this gets accepted after 3.15-rcx.
Cc: sta...@vger.kernel.org # 3.15.x+
---
arch/arm/mach-omap2/gpmc-nand.c | 16 ++--
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c
index 4349e82..52c4834 100644
--- a/arch/arm/mach-omap2/gpmc-nand.c
+++ b/arch/arm/mach-omap2/gpmc-nand.c
@@ -43,13 +43,17 @@ static struct platform_device gpmc_nand_device = {
.resource = gpmc_nand_resource,
};
-static bool gpmc_hwecc_bch_capable(enum omap_ecc ecc_opt)
+static bool gpmc_ecc_bch_capable(enum omap_ecc ecc_opt)
I don't think this renaming is really required with this fix, so please
drop it and just keep it simple only for OMAP3 fix.
{
/* platforms which support all ECC schemes */
if (soc_is_am33xx() || cpu_is_omap44xx() ||
soc_is_omap54xx() || soc_is_dra7xx())
return 1;
+ if ((ecc_opt == OMAP_ECC_BCH4_CODE_HW_DETECTION_SW) ||
+ (ecc_opt == OMAP_ECC_BCH8_CODE_HW_DETECTION_SW))
+ return 1;
+
Note: Some very old legacy platforms have initial version of GPMC engine
which only supports Hamming ECC, and not BCH ECC scheme, so lets
filter them out to maintain backward compatibility.
(1) As per TRM, OMAP2 platform does not support BCH ECC schemes,
So please filter out OMAP2 devices from this check ...
(2) I don't know the history but below comment says:
* For now, assume 4-bit mode is only supported on OMAP3630 ES1.x, x=1
if ((ecc_opt == OMAP_ECC_BCH4_CODE_HW_DETECTION_SW) ||
(ecc_opt == OMAP_ECC_BCH8_CODE_HW_DETECTION_SW)) {
if (cpu_is_omap24xx())
return 0;
else if (cpu_is_omap3630() (GET_OMAP_REVISION() == 0))
return 0;
else
return 1;
}
/* OMAP3xxx do not have ELM engine, so cannot support ECC schemes
* which require H/W based ECC error detection */
if ((cpu_is_omap34xx() || cpu_is_omap3630())
@@ -57,14 +61,6 @@ static bool gpmc_hwecc_bch_capable(enum omap_ecc ecc_opt)
(ecc_opt == OMAP_ECC_BCH8_CODE_HW)))
return 0;
- /*
- * For now, assume 4-bit mode is only supported on OMAP3630 ES1.x, x=1
- * and AM33xx derivates. Other chips may be added if confirmed to work.
- */
- if ((ecc_opt == OMAP_ECC_BCH4_CODE_HW_DETECTION_SW)
- (!cpu_is_omap3630() || (GET_OMAP_REVISION() == 0)))
- return 0;
-
Thanks for removing this, I too wasn't confident of this either.
/* legacy platforms support only HAM1 (1-bit Hamming) ECC scheme */
if (ecc_opt == OMAP_ECC_HAM1_CODE_HW)
return 1;
@@ -140,7 +136,7 @@ int gpmc_nand_init(struct omap_nand_platform_data
*gpmc_nand_data,
gpmc_update_nand_reg(gpmc_nand_data-reg, gpmc_nand_data-cs);
- if (!gpmc_hwecc_bch_capable(gpmc_nand_data-ecc_opt)) {
+ if (!gpmc_ecc_bch_capable(gpmc_nand_data-ecc_opt)) {
Please drop this renaming from this patch.
dev_err(dev, Unsupported NAND ECC scheme selected\n);
return -EINVAL;
}
--
1.7.10.4
You may send this patch separately apart from the series, so that
this gets accepted in current 3.15-rcx.
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4 3/4] mtd: nand: omap: add support for BCH16_ECC - NAND driver updates
Hi Brian, From: Brian Norris On Mon, May 19, 2014 at 01:24:41PM +0530, Pekon Gupta wrote: --- a/drivers/mtd/nand/omap2.c +++ b/drivers/mtd/nand/omap2.c @@ -1201,6 +1219,41 @@ static int __maybe_unused omap_calculate_ecc_bch(struct mtd_info *mtd, *ecc_code++ = ((bch_val1 4) 0xFF); *ecc_code++ = ((bch_val1 0xF) 4); break; +case OMAP_ECC_BCH16_CODE_HW: +val = readl(gpmc_regs-gpmc_bch_result6[i]); For all of these 'gpmc_bch_resultX' fields, couldn't you make this into a 2-D array? So to access BCH result 6 at sector i, it would be: val = readl(gpmc_regs-gpmc_bch_result[6][i]; This could help you to rewrite some of this stuff as loops, instead of giant blocks of copy-paste-modify. Thanks for excepting the series. With this series OMAP NAND driver comes to a stable point supporting NAND boot on all latest platforms. I had earlier experimented with something similar to your suggestions but these clean-ups require update in multiple drivers (GPMC, NAND, ...), and it was becoming messy, so I dropped it. There are some practical challenges to this type of clean-up like; (1) GPMC register addresses here gpmc_regs are initialized in GPMC driver So your suggested changes will affect multiple subsystems, hence This type of clean-up is bit deferred from sometime. (2) The register space for gpmc_bch_results is not contiguous. GPMC_BCH_RESULT_0 RW 0x240-0x2B0 GPMC_BCH_RESULT_1 RW 0x244-0x2B4 GPMC_BCH_RESULT_2 RW 0x248-0x2B8 GPMC_BCH_RESULT_3 RW 0x24C-0x2BC [...] GPMC_BCH_RESULT_4 RW 0x300-0x370 GPMC_BCH_RESULT_5 RW 0x304-0x374 GPMC_BCH_RESULT_6 RW 0x308-0x378 This is because older version of GPMC hardware IP (like in OMAP3) supported only till BCH8 ecc-scheme. Later same hardware IP was extended to support BCH16. So newer platforms have extended register-map. So it was felt that instead of having separate for..loops lets continue with replicating all 7 GPMC_BCH_RESULT registers. +ecc_code[0] = ((val 8) 0xFF); +ecc_code[1] = ((val 0) 0xFF); +val = readl(gpmc_regs-gpmc_bch_result5[i]); +ecc_code[2] = ((val 24) 0xFF); +ecc_code[3] = ((val 16) 0xFF); +ecc_code[4] = ((val 8) 0xFF); +ecc_code[5] = ((val 0) 0xFF); A lot of this code can be rewritten to use the endian swapping macros, I expect. Something like this looks equivalent: *((uint32_t *)ecc_code[2]) = cpu_to_be32(val); You could probably fix the types up to make this look a little nicer. (3) BCH4 use different alignment than BCH8 and BCH16 as ECC syndrome is not bytewise aligned (it's of 6 1/2 bytes = 13 nibbles). So for BCH4 higher-nibble or reg1 is shifted and ORed with lower-nibble of reg2. There is no byte-to-byte mapping. So generic implementation becomes messy. However, Roger Quadros is planning GPMC driver clean-up, so looping him in-case he can incorporate some of these things while re-factoring GPMC code. with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v7 2/4] ARM: dts: am437x-gp-evm: add support for parallel NAND flash
From: Tony Lindgren [mailto:t...@atomide.com]
* Gupta, Pekon pe...@ti.com [140519 21:07]:
From: Tony Lindgren [mailto:t...@atomide.com]
* Pekon Gupta pe...@ti.com [140519 02:16]:
Adds pinmux and DT node for Micron (MT29F4G08AB) x8 NAND device present on
am437x-gp-evm board.
(1) As NAND Flash data lines are muxed with eMMC, Thus at a given time
either
eMMC or NAND can be enabled. Selection between eMMC and NAND is
controlled:
(a) By dynamically driving following GPIO pin from software
SPI2_CS0(GPIO) == 0 NAND is selected (default)
SPI2_CS0(GPIO) == 1 eMMC is selected
(b) By statically using Jumper (J89) on the board
So which MMC controller has eMMC then? How do we select which one we
have enabled in the am437x-gp-evm.dts by default?
If there is no Jumper on the board, then driving SPI2_CS0 before device
probe decides the selection between NAND and eMMC. Therefore NAND
pin-mux also includes SPI2_CS0 and enables PULLDOWN on it to select NAND.
So do they share lines outside omap, not inside omap?
The reason I'm asking is I'm worried about the conflicting
pinctrl settings if we try to use both. And I guess that's
not an issue if the muxing of lines is done outside the
omap?
Yes, the muxing is outside OMAP SoC, so if SPI2_CS0 is correctly
driven it will not allow contention on GPMC line (as per board design).
On am437x-gp-evm board, SPI2_CS0 is used to:
- route GPMC signals to selected device {eMMC | NAND}.
- keep the de-selected device {eMMC | NAND} in reset.
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2] ARM: OMAP2+: gpmc: enable wait-pin monitoring for NAND devices via DT
Hi Roger,
From: Quadros, Roger
On 05/20/2014 09:24 AM, Pekon Gupta wrote:
This patch enables 'wait-pin' monitoring in NAND driver if following
properties
are present under NAND DT node
gpmc,wait-pin = wait-pin number
gpmc,wait-on-read
gpmc,wait-on-write
As NAND generic framework uses common path nand_chip-dev_ready() for
monitoring
completion of Read and Write status, so wait-pin monitoring is enabled only
when
*both* 'gpmc,wait-on-read' and 'gpmc,wait-on-write' are specified.
CC: devicet...@vger.kernel.org
Signed-off-by: Pekon Gupta pe...@ti.com
---
Documentation/devicetree/bindings/mtd/gpmc-nand.txt | 8
arch/arm/mach-omap2/gpmc-nand.c | 8 +---
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
index eb81435..4039032 100644
--- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
@@ -45,6 +45,14 @@ Optional properties:
ELM hardware engines should specify this device node in .dtsi
Using ELM for ECC error correction frees some CPU cycles.
+ - gpmc,wait-pin=pin number Specifies GPMC wait-pin number to
monitor
+ - gpmc,wait-on-readEnable wait-pin monitoring for Read
accesses
+ - gpmc,wait-on-write Enable wait-pin monitoring for Write
accesses
+As NAND generic framework uses single common function
+nand_chip-dev_ready() for polling wait-pin both for Read and
+Write accesses. So for NAND devices both 'gpmc,wait-on-read' and
+'gpmc,wait-on-write' need to be specified together.
+
For inline partiton table parsing (optional):
- #address-cells: should be set to 1
diff --git a/arch/arm/mach-omap2/gpmc-nand.c
b/arch/arm/mach-omap2/gpmc-nand.c
index 17cd393..62bc3de 100644
--- a/arch/arm/mach-omap2/gpmc-nand.c
+++ b/arch/arm/mach-omap2/gpmc-nand.c
@@ -123,11 +123,13 @@ int gpmc_nand_init(struct omap_nand_platform_data
*gpmc_nand_data,
}
}
-if (gpmc_nand_data-of_node)
+if (gpmc_nand_data-of_node) {
gpmc_read_settings_dt(gpmc_nand_data-of_node, s);
-else
+if (s.wait_on_read s.wait_on_write)
+gpmc_nand_data-dev_ready = true;
+} else {
gpmc_set_legacy(gpmc_nand_data, s);
-
+}
s.device_nand = true;
NACK.
For NAND, we only need the wait-pin property. The wait-on-read/wait-on-write
flags are meaningless.
There is a check in arch/arm/mach-omap2/gpmc.c : @@gpmc_read_settings_dt()
which expects at-least one of the 'gpmc,wait-on-read' or 'gpmc,wait-on-write'
to be enabled.
if (!p-wait_on_read !p-wait_on_write)
pr_warn(%s: read/write wait monitoring not enabled!\n,
__func__);
And gpmc_read_settings_dt() is common for all GPMC devices (NAND, NOR, .. )
Now, if you remove that check it means you are deviating from the behavior of
DT binding, so you need to be backward compatible.
In practice, I agree that a single gpmc,wait-pin binding was enough to both
- Select the wait-pin
- Enable wait-pin monitoring for GPMC devices.
But now as we have two extra bindings, you have to maintain backward
compatibility.
If you have better solution, then please send a patch, I'll be happy to test it.
Also, the wait-pin number needs to be communicated to the NAND driver and
omap_dev_ready()
function updated so that it checks for the right wait pin status.
Yes, that's true.
And this was my plan to have it as separate patch.
Also, the real benefit of wait-pin monitoring will be seen only
when its implemented as IRQ source. [1]
with regards, pekon
[1] http://www.spinics.net/lists/linux-omap/msg107236.html
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v7 1/4] ARM: dts: am335x-bone: add support for beaglebone NAND cape
Hi Tony,
From: Tony Lindgren [mailto:t...@atomide.com]
* Pekon Gupta pe...@ti.com [140519 02:16]:
--- a/arch/arm/boot/dts/am335x-bone.dts
+++ b/arch/arm/boot/dts/am335x-bone.dts
@@ -9,6 +9,7 @@
#include am33xx.dtsi
#include am335x-bone-common.dtsi
+#include am335x-bone-memory-cape.dts
ldo3_reg {
regulator-min-microvolt = 180;
--- a/arch/arm/boot/dts/am335x-boneblack.dts
+++ b/arch/arm/boot/dts/am335x-boneblack.dts
@@ -9,6 +9,7 @@
#include am33xx.dtsi
#include am335x-bone-common.dtsi
+#include am335x-bone-memory-cape.dts
ldo3_reg {
regulator-min-microvolt = 180;
Based on the recent discussions on the capes, it seems that nobody
wants to implement toggling of the capes in u-boot. And as there
can be other capes using GPMC bus, we can't merge this. And because
the capes are stackable, we can't really have .dts files for all
the combinations. And no, we're not merging any unconnected .dts
files either, sorry.
Yes, I fully understand. This is like death for most of the capes from point
of view of mainline support. But I have submitted this patch just for sake
of completion. And to provide reference if someone wants to locally work
on beaglebone NAND cape.
You may ignore this patch, and take the remaining ones if they are suitable.
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v7 1/4] ARM: dts: am335x-bone: add support for beaglebone NAND cape
From: Ezequiel Garcia [mailto:ezequiel.gar...@free-electrons.com] On 19 May 05:52 PM, Gupta, Pekon wrote: From: Tony Lindgren [mailto:t...@atomide.com] Based on the recent discussions on the capes, it seems that nobody wants to implement toggling of the capes in u-boot. And as there can be other capes using GPMC bus, we can't merge this. And because the capes are stackable, we can't really have .dts files for all the combinations. And no, we're not merging any unconnected .dts files either, sorry. Yes, I fully understand. This is like death for most of the capes from point of view of mainline support. But I have submitted this patch just for sake of completion. And to provide reference if someone wants to locally work on beaglebone NAND cape. While I fully agree with Tony on this, I think the reference is much appreciated. Do you think you can push the NAND cape DT example somewhere? Maybe some TI wiki page? Yes, that's my intent. I usually keep updating following TI wiki pages [1] and [2] which I think are helpful for NAND users on OMAP platforms. [1] https://processors.wiki.ti.com/index.php/Linux_Core_NAND_User%27s_Guide#Board_specific_configurations [2] https://processors.wiki.ti.com/index.php/Linux_Core_NAND_User%27s_Guide with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] ARM: OMAP2+: gpmc: enable wait-pin monitoring for NAND devices via DT
Hi Javier,
From: Javier Martinez Canillas [mailto:jav...@dowhile0.org]
Hello Pekon,
On Fri, May 16, 2014 at 9:07 PM, Pekon Gupta pe...@ti.com wrote:
This patch enables 'wait-pin' monitoring in NAND driver if following
properties
are present under NAND DT node
gpmc,wait-pin = wait-pin number
gpmc,wait-on-read;
gpmc,wait-on-write;
As NAND generic framework uses common path nand_chip-dev_ready() for
monitoring
completion of Read and Write status, so wait-pin monitoring is enabled only
when
both 'gpmc,wait-on-read' and 'gpmc,wait-on-write' are specified.
I see that the GPMC driver checks if gpmc,wait-pin property is
defined and only in that case tries to read both gpmc,wait-on-read
and gpmc,wait-on-write and prints a read/write wait monitoring not
enabled! warning if one of those is not defined.
So my question is, it's a requirement and these 3 properties need to
always be defined together? If that is the case then I wonder why
there are 3 different properties and why not just defining
gpmc,wait-pin implies gpmc,wait-on-{read,write} ?
GPMC as a hardware engine allows selection of wait-pin independently
for both Read and Write paths, but NAND generic framework uses single
common function nand_chip-dev_ready() which is used for both
Read and Write paths to poll wait-pin.
So, in case of NAND both 'gpmc,wait-on-read' and 'gpmc,wait-on-write'
should be simultaneously defined to enable wait-pin monitoring.
But GPMC being generic hardware engine for NOR, OneNAND and other
parallel interfaces like Camera, Ethernet the two separate bindings allow
flexibility to use wait-pin monitoring for only one of the paths {Read | Write}.
Therefore this check is added in gpmc_nand_init(), and not in
gpmc_read_settings_dt()
which is common for all type of GPMC devices (NAND, NOR, .. )
Or if is valid to define gpmc-wait-pin without
gpmc-wait-on-{read,write} or only one those then why that scary
warning is printed?
Whatever is the case I think that the GPMC DT binding documentation
(Documentation/devicetree/bindings/bus/ti-gpmc.txt) should be more
clear on this regard.
Agree. But I think this clarification fits better in NAND specific documentation
Documentation/devicetree/bindings/mtd/gpmc-nand.txt
Signed-off-by: Pekon Gupta pe...@ti.com
---
arch/arm/mach-omap2/gpmc-nand.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-omap2/gpmc-nand.c
b/arch/arm/mach-omap2/gpmc-nand.c
index 4349e82..7dc742d 100644
--- a/arch/arm/mach-omap2/gpmc-nand.c
+++ b/arch/arm/mach-omap2/gpmc-nand.c
@@ -123,11 +123,13 @@ int gpmc_nand_init(struct omap_nand_platform_data
*gpmc_nand_data,
}
}
- if (gpmc_nand_data-of_node)
+ if (gpmc_nand_data-of_node) {
gpmc_read_settings_dt(gpmc_nand_data-of_node, s);
- else
+ if (s.wait_on_read s.wait_on_write)
You said that wait-on-pin monitoring is only enabled if both
gpmc,wait-on-read and gpmc,wait-on-write are specified. But in
that case I think we should clear the wait_pin on
gpmc_read_settings_dt() if either gpmc,wait-on-read or
gpmc,wait-on-write were not specified and check s.wait_pin instead.
Or is this semantic only for NAND and other devices connected to the
GPMC behave differently wrt gpmc,wait-pin?
True, this is only for NAND. Other device drivers may use wait-pin
either 'only for Read' or 'only for Write' access. However I doubt why
anyone would do so, unless there is some design bug | constrain.
+ gpmc_nand_data-dev_ready = true;
You should really use gpmc_nand_data-dev_ready = true here. The C99
standard allows you to assign a string literal to a _Bool type and
will be evaluated to 1 but that is confusing and I haven't seen that
used in other part of the kernel.
Sorry, my bad. I over looked it.
I'll fix it in next version along with Documentation update.
With regards, pekon
RE: [PATCH v7 1/4] ARM: dts: am335x-bone: add support for beaglebone NAND cape
From: Robert Nelson [mailto:robertcnel...@gmail.com] [...] Since the capes are always some way tied with bb.org hardware, why don't we put them up on https://github.com/beagleboard/ ? am335x-capes.git ? I envision, we should just mirror the base ./arch/arm/boot/dts/ directory (as someday the dts will be external anyways). In that repo, we will keep these synced with mainline am335x-bone-common.dtsi am335x-bone.dts am335x-boneblack.dts and add the capes as: baseboard-cape.dts as a staging ground till a mainline overlay/etc system appears? Thoughs? If you know and can convince 'Gerald Coley' then plz try. I failed :-) with regards, pekon
RE: [PATCH v7 2/4] ARM: dts: am437x-gp-evm: add support for parallel NAND flash
From: Tony Lindgren [mailto:t...@atomide.com]
* Pekon Gupta pe...@ti.com [140519 02:16]:
Adds pinmux and DT node for Micron (MT29F4G08AB) x8 NAND device present on
am437x-gp-evm board.
(1) As NAND Flash data lines are muxed with eMMC, Thus at a given time either
eMMC or NAND can be enabled. Selection between eMMC and NAND is
controlled:
(a) By dynamically driving following GPIO pin from software
SPI2_CS0(GPIO) == 0 NAND is selected (default)
SPI2_CS0(GPIO) == 1 eMMC is selected
(b) By statically using Jumper (J89) on the board
So which MMC controller has eMMC then? How do we select which one we
have enabled in the am437x-gp-evm.dts by default?
If there is no Jumper on the board, then driving SPI2_CS0 before device
probe decides the selection between NAND and eMMC. Therefore NAND
pin-mux also includes SPI2_CS0 and enables PULLDOWN on it to select NAND.
+ 0x26c(PIN_OUTPUT_PULLDOWN | MUX_MODE7)/* spi2_cs0.gpio/eMMCorNANDsel
*/
Regards,
Tony
(2) As NAND device connnected to this board has page-size=4K and
oob-size=224,
So ROM code expects boot-loaders to be flashed in BCH16 ECC scheme for
NAND boot.
Signed-off-by: Pekon Gupta pe...@ti.com
Reviewed-by: Javier Martinez Canillas jav...@dowhile0.org
---
arch/arm/boot/dts/am437x-gp-evm.dts | 108
1 file changed, 108 insertions(+)
diff --git a/arch/arm/boot/dts/am437x-gp-evm.dts
b/arch/arm/boot/dts/am437x-gp-evm.dts
index 30ace1b..f432685 100644
--- a/arch/arm/boot/dts/am437x-gp-evm.dts
+++ b/arch/arm/boot/dts/am437x-gp-evm.dts
@@ -150,6 +150,27 @@
0x14c (PIN_INPUT_PULLDOWN | MUX_MODE7)
;
};
+
+nand_flash_x8: nand_flash_x8 {
+pinctrl-single,pins =
+0x26c(PIN_OUTPUT_PULLDOWN | MUX_MODE7) /* --
spi2_cs0.gpio/eMMCorNANDsel */
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v6 2/4] ARM: dts: am437x-gp-evm: add support for parallel NAND flash
Hi Roger,
From: Quadros, Roger
[...]
+gpmc {
+status = okay;
+pinctrl-names = default;
+pinctrl-0 = nand_flash_x8;
+ranges = 0 0 0 0x0100;/* minimum GPMC partition = 16MB */
+nand@0,0 {
+reg = 0 0 0x37c; /* device IO registers */
This register space is used by the parent GPMC node as well as the NAND
controller. So doing a
request_and_ioremap() on this space will fail as it is already taken by the
GPMC driver.
Further, the GPMC register space doesn't map to the GPMC memory map created by
this Chip select
but it is mapped to L3_IO space. i.e. (physical address 0x6e00 )
We could have split the GPMC register space into GPMC part and NAND part but
to add to the
complexity, the register spaces for GPMC vs NAND are interleaved so it can't
be easily split up.
Sorry I din't get this part.
NAND is one of the devices supported by GPMC controller.
Hardware wise the it's the same engine is used for NAND, NOR and OneNAND
and even other parallel interfaces like Camera, Ethernet, etc..
So how can be NAND and GPMC registers space be splitted ?
I see gpmc.c as generic controller driver, which just does initializations
and registering the device. Then it's upto the individual protocol drivers
to probe the device and attach it to respective sub-system; like;
(a) drivers/mtd/nand/omap2.c for NAND
(b) drivers/mtd/chips/cfi_probe.c for NOR
...
The way
the NAND driver is currently written is that it expects the register addresses
to come via platform data,
primarily to get around this address interleaving issue.
Yes, that is right.
I don't know the historical reasons but that is correct also because,
(1) large set of GPMC register configurations are common across all
types of devices (like NAND, NOR, OneNAND, Ethernet). These
register configurations define the signal timings and physical attributes
of device (like device width, etc).
(2) Individual protocol drivers (a) and (b) only uses a sub-set of registers
for transferring data, much of which is based on type of protocol.
So, large part of GPMC configurations remains static and can be
Shared across all types of devices, hence those are put in gpmc.c
Apart from the GPMC register space, the NAND controller uses 4 bytes of GPMC
memory map for I/O,
and that is something that could be reflected here.
e.g.
reg = 0 0 4; /* NAND I/O space */
But still, the start address can't be 0 and has to be assigned when this CS
region is mapped. It is still
unclear to me how that can be done.
Yes, NAND is not directly mapped. So only 4-bytes of I/O size will do.
But where to map these 4 bytes ? and which 4-bytes to map.
So I have included complete GPMC register space-size.
Also since GPMC has a constrain that every chip-select must have
minimum of 16MB memory. So we specify it via range property
to keep GPMC mapping consistent across all NAND, NOR, OneNAND, etc..
If you have any better approach in mind for keeping consistent
memory mapping across different types of devices, then please suggest ..
I'll try to get this done in next set of patches, if not these.
Or
May be you can take over as part of GPMC transition.
cheers,
-roger
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v6 1/4] ARM: dts: am335x-bone: add support for beaglebone NAND cape
Hi Ezequiel, From: Ezequiel Garcia [mailto:ezequ...@vanguardiasur.com.ar] Hello Pekon, On 16 May 04:33 PM, Pekon Gupta wrote: [..] +gpmc,wait-pin = 0; +gpmc,wait-on-read; +gpmc,wait-on-write; Thanks for adding the wait-pin property. I think the other boards also needs this change. Javier, do you have a IGEP Aquila with NAND to try? Pekon, have you noticed that there's no devicetree property to enable ready/busy? In other words, the NAND driver dev_ready field will never be true when probed from DT. Yes, that is know, therefore I din't reply to your earlier email. I was just fixing your DT comments and have a small patch to set dev_ready=true. I'll send it soon. But still you need to consider that nand/omap2.c driver only supports polling way of monitoring READY/BUSY pin. That is instead of polling for status-bit | waiting for predefined delay, NAND driver now polls for READY/BUSY pin. However, actual benefit of wait-pin monitoring might be visible when READY/BUSY pin is used with interrupt. with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v6 2/4] ARM: dts: am437x-gp-evm: add support for parallel NAND flash
Hi Tony, Roger,
From: Tony Lindgren [mailto:t...@atomide.com]
* Roger Quadros rog...@ti.com [140516 05:23]:
On 05/16/2014 02:03 PM, Pekon Gupta wrote:
+gpmc {
+ status = okay;
+ pinctrl-names = default;
+ pinctrl-0 = nand_flash_x8;
+ ranges = 0 0 0 0x0100;/* minimum GPMC partition = 16MB */
+ nand@0,0 {
+ reg = 0 0 0x37c; /* device IO registers */
This register space is used by the parent GPMC node as well as the NAND
controller. So doing a
request_and_ioremap() on this space will fail as it is already taken by the
GPMC driver.
Further, the GPMC register space doesn't map to the GPMC memory map created
by this Chip select
but it is mapped to L3_IO space. i.e. (physical address 0x6e00 )
We could have split the GPMC register space into GPMC part and NAND part but
to add to the
complexity, the register spaces for GPMC vs NAND are interleaved so it can't
be easily split up. The way
the NAND driver is currently written is that it expects the register addresses
to come via platform data,
primarily to get around this address interleaving issue.
Apart from the GPMC register space, the NAND controller uses 4 bytes of GPMC
memory map for
I/O, and that is something that could be reflected here.
e.g.
reg = 0 0 4; /* NAND I/O space */
Guys, the reg size here is size of the IO region for the
NAND driver, it should not have anything to do with the GPMC
registers for the GPMC functions that the NAND driver may call.
So it sounds like 0x37c is wrong, and 4 is the right value if
the NAND chip has only one IO register.
Yes, Roger and myself both agree on the actual size of 4.
But what I'm not sure is what should be offset for a io-remapped region.
Apologies for my ignorance here, as I'm not good in this memory mapping
concept..
- I understand that for 'memory mapped' device offset is with respect to
base-address of chip-select.
- But for indirectly mapped device (like NAND) how is offset calculated?
Example: For CS0 in GPMC ..
GPMC_NAND_COMMAND_0 = 0x007c
GPMC_NAND_ADDRESS_0 = 0x0080
GPMC_NAND_DATA_0 = 0x0084
(1) Now, should the 'offset' for reg property used for NAND node
connected at CS=0. reg = 0 ?? 4
(a) Should it be equal to 'GPMC_NAND_ADDRESS_0'?
OR
(b) There is no relation between 'GPMC register' and 'NAND partition' offset?
(2) If considering (a) then should size consider only GPMC_NAND_ADDRESS_0
or also include GPMC_NAND_COMMAND_0 and GPMC_NAND_DATA_0?
This is why I used the size of complete GPMC register space for NAND region
also.
But still, the start address can't be 0 and has to be assigned when this CS
region is mapped. It is still
unclear to me how that can be done.
If the offset for the NAND driver needs to be different from 0,
it can be in the offset. For example, the smsc,lan91c94 driver
wants the IO address space to be at offset 0x300 to avoid some
extra warnings during the boot. So for that, the reg entry is:
reg = 1 0x300 0xf;
Where we have:
1 = chip select
0x300 = offset of smsc device register IO space from the start of
16MB minimum GPMC partition
This is where my confusion lies, where is the 0x300 coming from?
Is this the offset of I/O register in given Ethernet IP register space?
0xf = size of smsc device register IO space that the Ethernet
driver ioremaps
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2 1/2] ARM: dts: am335x-bone: add support for beaglebone NAND cape
Hi Ezequiel, Sorry for delayed replies.. From: Ezequiel Garcia [mailto:ezequiel.gar...@free-electrons.com] Pekon, A few questions regarding this patch, despite the fact I'm not sure it will ever be included. On 20 Mar 01:04 PM, Pekon Gupta wrote: +0x70 (MUX_MODE0 | PIN_INPUT_PULLUP )/* gpmc_wait0.gpmc_wait0 */ +0x74 (MUX_MODE7 | PIN_OUTPUT_PULLUP)/* gpmc_wpn.gpio0_30 */ Is this output pin? Yes, it's the Write Protect (WP) pin. And goes controller - device. +0x7c (MUX_MODE0 | PIN_OUTPUT_PULLUP)/* gpmc_csn0.gpmc_csn0 */ Again: is this output pin? Yes, this is Chip-select and goes from controller - device. Though it should already have external pull-up resistor on board, to avoid glitches due to noise, and avoid device getting confused when SoC is not driving anything (like before pin-muxing). +gpmc,wait-on-read = true; +gpmc,wait-on-write = true; The devicetree binding says these wait properties are bool, so the above should be: gpmc,wait-on-read; gpmc,wait-on-write; But the problem is that there's no wait-pin defined, so this not enabled. Could you explain what should be the correct binding? This is very confusing for me. I have fixed the above DT mapping in newer version of the patch [1]. Sorry, yes the wait-pin monitoring implementation is not cleanly done, there is mix of platform_data and DT stuff. So in addition to updated DT patch you need following patch to enable wait-pin monitoring in NAND driver. http://www.spinics.net/lists/linux-omap/msg107253.html with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4 3/6] ARM: dts: dra7: add support for parallel NAND flash
Hi Roger,
For now, I'll use GPMC address-space size = 0x380 as it matches with
actual hardware and is working.
How did you get 0x380?
From DRA7 TRM, GPMC address range is 0x5000 : 0x5000 02D0
So the address-space size should be 0x2D4 (as last register@2D0 is 32-bits
wide)
I think that is copy-paste error in documentation.
In the same TRM, you 'll find the correct address offsets for GPMC Registers in
below
*Section: 15.4.7.1 GPMC Register Summary*
Register Starting Offset
End Offset
GPMC_BCH_RESULT4_i0x 0300 + (0x 0010 * i) 0x5000 0300 + (0x
0010 * i)
GPMC_BCH_RESULT5_i0x 0304 + (0x 0010 * i) 0x5000 0304 + (0x
0010 * i)
GPMC_BCH_RESULT6_i0x 0308 + (0x 0010 * i) 0x5000 0308 + (0x
0010 * i)
Where i = 0 to 7 ..
So that makes last address 0x5000_0378 (for GPMC_BCH_RESULT6_7)
As the each register bank (i) is incrementing at 0x10, so last accessible
address is 0x37F.
I have already raised documentation bug for AM335x TRM,
Need to raise the same for DRA7xx TRM.
For the ELM module it should be 4KB i.e. 0x1000
Yes, that is correct. I have fixed that now.
cheers,
-roger
[1] http://www.ti.com/lit/gpn/am3359(Section 7.4 to 7.4.5)
[2] http://www.ti.com/lit/gpn/am3359(Section 7.1 to 7.1.5)
(Though the AM335x address space mentions 0x368 as last address,
it should be 0x378. I have raised documentation bug for it).
with regards, pekon
N�r��yb�X��ǧv�^�){.n�+{��f��{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj��!�i
RE: [PATCH v4 3/6] ARM: dts: dra7: add support for parallel NAND flash
From: Quadros, Roger
[...]
For now, I'll use GPMC address-space size = 0x380 as it matches with
actual hardware and is working.
How did you get 0x380?
From DRA7 TRM, GPMC address range is 0x5000 : 0x5000 02D0
So the address-space size should be 0x2D4 (as last register@2D0 is 32-bits
wide)
Sorry for the noise.
Just figured out that the register space is not numerically arranged in the
TRM.
The last register is P GPMC_BCH_RESULT6_i
0x5000 0308 + (0x 0010 * i)
i = 0 to 7
So size should be 0x37C.
Yes, as each {GPMC_BCH_RESULTx_i} group is incremented by 0x10,
so I aligned the last address to 0x380 boundary. Hope leaving room for
extra 4 bytes (0x380 - 0x37C) will not matter much?
All platforms from OMAP4 onwards share the same version of GPMC engine.
So this remains consistent. Only OMAP3 has older version of GPMC engine
which has register-space till 0x2d0.
cheers,
-roger
with regards, pekon
RE: [PATCH v4 1/6] ARM: dts: am335x-bone: add support for beaglebone NAND cape
Hi Roger,
On 05/09/2014 11:40 PM, Pekon Gupta wrote:
[...]
+
+elm {
+ status = disabled;
+};
+
+gpmc {
+ status = disabled;
+};
Why are you disabling the elm and gpmc modules here?
Shouldn't they be disabled by default in the soc.dtsi file?
Yes both GPMC and ELM are 'disabled' by default in am33xx.dtsi.
I'll remove these duplicates in next version.
Thanks for pointing. Similar issue was also pointed out by Tony [1].
with regards, pekon
cheers,
-roger
[1] http://www.spinics.net/lists/linux-omap/msg106920.html
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4 3/6] ARM: dts: dra7: add support for parallel NAND flash
Hello,
From: Javier Martinez Canillas [mailto:jav...@dowhile0.org]
On Fri, May 9, 2014 at 10:46 PM, Pekon Gupta pe...@ti.com wrote:
From: Minal Shah minalks...@gmail.com
[...]
+gpmc {
+ status = okay;
+ pinctrl-names = default;
+ pinctrl-0 = nand_flash_x16;
+ ranges = 0 0 0 0x100;
+ nand@0,0 {
+ reg = 0 0 0x380;
[...]
diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index 37a0595..6af775a 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -776,6 +776,26 @@
interrupts = 0 343 0x4;
status = disabled;
};
+
+ elm: elm@48078000 {
+ compatible = ti,am3352-elm;
+ reg = 0x48078000 0x2000;
It is really necessary to map all this 8 KB address space for GPMC registers?
I don't have access to the DRA7 TRM but for example the OMAP3 TRM says
that the GPMC module register address space size is 16 MB while in
practice the registers use less than 1 KB (0..0x02d0 to be exact) so
in arch/arm/boot/dts/omap3.dtsi we have:
These are not GPMC registers. Platforms from OMAP4 and beyond
(like AM335x, OMAP5) have another small hardware engine called ELM [1]
(Error Locater Module) in addition to GPMC, which is used for detecting
ECC errors in hardware. ELM Driver $KERNEL/drivers/mtd/devices/elm.c
However, thanks for pointing out, this address-space for ELM is incorrect.
Last ELM register offset is 0xFC0 (ELM_ERROR_LOCATION_15_7) [1].
gpmc: gpmc@6e00 {
...
reg = 0x6e00 0x02d0;
...
};
Shouldn't this be similar (the same?) for DRA7 GPMC device node?
Newer platforms have upgraded version of GPMC engine which supports
BCH16 ECC scheme in hardware. Thus the GPMC address space was
expanded to include some extra registers required for BCH16 ECC [2].
Thanks a lot and best regards,
Javier
[1] http://www.ti.com/lit/gpn/am3359(Section 7.4 to 7.4.5)
[2] http://www.ti.com/lit/gpn/am3359(Section 7.1 to 7.1.5)
(Though the AM335x address space mentions 0x368 as last address,
it should be 0x378. I have raised documentation bug for it).
with regards, pekon
N�r��yb�X��ǧv�^�){.n�+{��f��{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj��!�i
RE: [PATCH v4 3/6] ARM: dts: dra7: add support for parallel NAND flash
From: Javier Martinez Canillas [mailto:jav...@dowhile0.org] [...] Newer platforms have upgraded version of GPMC engine which supports BCH16 ECC scheme in hardware. Thus the GPMC address space was expanded to include some extra registers required for BCH16 ECC [2]. I see and did the GPMC register space became that big to need to map 8KB? Although the smallest unit for ioremap is PAGE_SIZE and using any of these reg sizes: reg = 0x6e00 0x02d0; reg = 0x6e00 0x0400; reg = 0x6e00 0x1000; in practice have the same effect, DTS should describe the hardware and not an implementation detail so I think that we should use only the register size that is defined in the TRM. Yes, I agree with you. I have fixed this in newer version of the patch and will be sending it soon. But this series will only contain updates for new platforms with addition of NAND node in DTS, so that this series is not stalled for any reason. For fixing existing platform/boards DTS I'll send another series soon. For now, I'll use GPMC address-space size = 0x380 as it matches with actual hardware and is working. [1] http://www.ti.com/lit/gpn/am3359(Section 7.4 to 7.4.5) [2] http://www.ti.com/lit/gpn/am3359(Section 7.1 to 7.1.5) (Though the AM335x address space mentions 0x368 as last address, it should be 0x378. I have raised documentation bug for it). with regards, pekon Best regards, Javier [0]: http://lxr.free-electrons.com/source/arch/arm/mm/ioremap.c#L334
RE: [PATCH v3 1/4] ARM: dts: am335x-bone: add support for beaglebone NAND cape
From: Tony Lindgren [mailto:t...@atomide.com]
* Pekon Gupta pe...@ti.com [140422 00:34]:
--- a/arch/arm/boot/dts/am335x-bone.dts
+++ b/arch/arm/boot/dts/am335x-bone.dts
@@ -9,6 +9,7 @@
#include am33xx.dtsi
#include am335x-bone-common.dtsi
+#include am335x-bone-memory-cape.dts
ldo3_reg {
regulator-min-microvolt = 180;
--- a/arch/arm/boot/dts/am335x-boneblack.dts
+++ b/arch/arm/boot/dts/am335x-boneblack.dts
@@ -9,6 +9,7 @@
#include am33xx.dtsi
#include am335x-bone-common.dtsi
+#include am335x-bone-memory-cape.dts
ldo3_reg {
regulator-min-microvolt = 180;
Have you checked that including the capes unconditionally for
non-integrated devices is safe? Maybe decompile the dtb using
dtc and see what is in the produced dts file?
I'm mostly worried about pinmux and GPMC as the pins can be used
by other capes and GPMC can have other devices.
I checked by de-compiling beaglebone.dtb with this patch included,
where GPMC pinmux conflicts with eMMC (MMC2). It shows that
both the pin-mux are present, and GPMC node is disabled by
default, whereas the eMMC (MMC2) node.
So I think this patch is safe pin-muxing wise.
---
gpmc@5000 {
...
status = disabled;
pinctrl-names = default;
pinctrl-0 = 0x38;
}
mmc@481d8000 {
ti,hwmods = mmc2;
...
status = okay;
pinctrl-0 = 0x2e;
bus-width = 0x8;
}
pinmux_emmc_pins {
pinctrl-single,pins = 0x80 0x32 0x84 0x32 0x0 0x31 0x4
0x31 0x8 0x31 0xc 0x31 0x10 0x31 0x14 0x31 0x18 0x31 0x1c 0x31;
linux,phandle = 0x2e;
phandle = 0x2e;
};
nand_flash_x16 {
pinctrl-single,pins = 0x0 0x28 0x4 0x28 0x8 0x28 0xc
0x28 0x10 0x28 0x14 0x28 0x18 0x28 0x1c 0x28 0x20 0x28 0x24 0x28 0x28 0x28 0x2c
0x28 0x30 0x28 0x34 0x28 0x38 0x28 0x3c 0x28 0x70 0x30 0x74 0x17 0x7c 0x10 0x90
0x8 0x94 0x8 0x98 0x8 0x9c 0x8;
linux,phandle = 0x38;
phandle = 0x38;
};
--
Also, this should probably also wait until u-boot has been
confirmed of being able to enable these devices?
Would be good if you accept this one, as this will act as reference for
beaglebone cape users for pin-mux and other GPMC related bindings
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 2/4] ARM: dts: am437x-gp-evm: add support for parallel NAND flash
Hi Tony, And looks like we have a build warning in the -rc cycle with omap2plus_defconfig: drivers/mtd/nand/omap2.c:1250:12: warning: ‘erased_sector_bitflips’ defined but not used [- Wunused-function] Can you please fix that if not already fixed? Yes, this is already fixed and is in MTD Maintainer's tree [1]. As this warning was introduced in MTD patches, so fix is also coming via that sub-system. http://lists.infradead.org/pipermail/linux-mtd/2014-April/053330.html with regards, pekon
RE: [PATCH v3 2/4] ARM: dts: am437x-gp-evm: add support for parallel NAND flash
From: Tony Lindgren [mailto:t...@atomide.com]
* Pekon Gupta pe...@ti.com [140422 00:34]:
+gpmc {
+status = okay;
+pinctrl-names = default;
+pinctrl-0 = nand_flash_x8;
+ranges = 0 0 0x0800 0x1000; /* CS0: NAND */
Please use the minimum size 16MB GPMC range here, NAND only
has few registers addressable unlike NOR that actually uses the
whole range.
+nand@0,0 {
+reg = 0 0 0; /* CS0, offset 0 */
Then here map the true size of the NAND device IO register area.
BTW, we should do the similar changes to other files so we can
unify the GPMC partitioning a bit. But that's unsafe to do until
we have fixed the issue of mapping GPMC devices to a different
location from the bootloader location.
I have found the fix of this issue in gpmc_cs_remap() just testing it
using beaglebone NOR cape. I'll post that separately, once I'm confident.
But for now, I'll re-send just these patches for NAND DT node with
your feedbacks incorporated, so that NAND is stable on these
platforms / boards from 3.16 onwards.
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 0/4] add parallel NAND support for TI's new OMAPx and AMxx platforms (Part-2)
Hello Tony, From: Gupta, Pekon *changes v2 - v3* rebased on git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap :master merged leftover patches (dra7-evm and am43x-epos-evm fix) from Part-1 series Can you please see if this series can be taken in for 3.16 ? As this series concludes GPMC-NAND support for all major OMAP boards and brings it to a stable point. with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] ARM: dts: Add support for the BeagleBoard xM A/B
Tony,
From: Tony Lindgren
* Robert Nelson robertcnel...@gmail.com [140418 16:42]:
On Fri, Apr 18, 2014 at 5:51 PM, Tony Lindgren t...@atomide.com wrote:
* Robert Nelson robertcnel...@gmail.com [140415 08:46]:
Background: i also tried getting this having this fixed in u-boot:
Do we still need to apply this patch then?
Yeah, Tom want's it done in the kernel:
Here's my proposed u-boot patch:
http://lists.denx.de/pipermail/u-boot/2014-January/172154.html
and Tom's recommendation:
http://lists.denx.de/pipermail/u-boot/2014-January/172274.html
Once this hits mainline, i'll submit a patch to u-boot to check for
the presence of this version and drop to the old dtb if not found.
OK applying into omap-for-v3.15/fixes thanks.
Tony
You probably missed fixing below typo before applying this patch.
omap3-beagle-xm-ab.dts breaks without this.
+/ {
+ /* HS USB Port 2 Power enable was inverted with the xM C */
+ hsusb2_power: hsusb2_power_reg {
+ enable-active-high;
};
+};
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2 0/2] add parallel NAND support for TI's new OMAPx and AMxx platforms
HI Tony, From: Gupta, Pekon [...] I see following patches in your tree/branch omap-for-v3.15/dt But probably they were omitted in the pull request. Is there something, I need to do for these ? f68e355 2014-03-02 ARM: dts: am43xx: add support for parallel NAND flash c06c527 2014-03-02 ARM: dts: AM33xx: updated default ECC scheme in nand-ecc-opt 91994fa 2014-03-02 ARM: dts: am335x-evm: NAND: update MTD partition table 0611c41 2014-03-02 ARM: OMAP2+: gpmc: update gpmc_hwecc_bch_capable() for new platforms and ECC schemes Also, I had sent a [v2] for one of the patches, dropped by you. Please see if that can be picked [Patch v2 1/2] ARM: dts: dra7: add support for parallel NAND flash [Patch v2 1/2] ARM: dts: am43xx: fix starting offset of NAND.filesystem MTD partition Please let me know, if I need to rebase any of the patches in follow series against your for_3.15/dt or any other branch. (1) [Partly Merged] add parallel NAND support for TI's new OMAPx and AMxx platforms http://www.spinics.net/lists/linux-omap/msg104305.html (2) [New Patches] add parallel NAND support for TI's new OMAPx and AMxx platforms (part-2) http://www.spinics.net/lists/linux-omap/msg104668.html with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v8 0/4] mtd: devices: elm: add checks ELM H/W constrains, driver code cleanup
From: Gupta, Pekon Subject: [PATCH v8 0/4] mtd: devices: elm: add checks ELM H/W constrains, driver code cleanup Sorry, sent to the wrong list.. was intended for linux-mtd list Please ignore the whole series. with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2 0/2] add parallel NAND support for TI's new OMAPx and AMxx platforms
Hi Tony, From: Gupta, Pekon *changes v1 - v2* - AM335x (am335x-evm): already accepted, so dropping in v2 - DRA7xx (dra7-evm): resending - AM43xx (am43X-epos-evm): fix MTD NAND.filesystem partition offset I see following patches in your tree/branch omap-for-v3.15/dt But probably they were omitted in the pull request. Is there something, I need to do for these ? f68e355 2014-03-02 ARM: dts: am43xx: add support for parallel NAND flash c06c527 2014-03-02 ARM: dts: AM33xx: updated default ECC scheme in nand-ecc-opt 91994fa 2014-03-02 ARM: dts: am335x-evm: NAND: update MTD partition table 0611c41 2014-03-02 ARM: OMAP2+: gpmc: update gpmc_hwecc_bch_capable() for new platforms and ECC schemes Also, I had sent a [v2] for one of the patches, dropped by you. Please see if that can be picked [Patch v2 1/2] ARM: dts: dra7: add support for parallel NAND flash [Patch v2 1/2] ARM: dts: am43xx: fix starting offset of NAND.filesystem MTD partition with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v1 1/3] ARM: dts: am335x-bone: add support for beaglebone NAND cape
[...] arch/arm/boot/dts/Makefile|1 + arch/arm/boot/dts/am335x-bone-memory-cape.dts | 123 + 2 files changed, 124 insertions(+) create mode 100644 arch/arm/boot/dts/am335x-bone-memory-cape.dts diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile index 0320303..c5e9bfa 100644 --- a/arch/arm/boot/dts/Makefile +++ b/arch/arm/boot/dts/Makefile @@ -226,6 +226,7 @@ dtb-$(CONFIG_ARCH_OMAP2PLUS) += omap2420-h4.dtb \ am335x-evmsk.dtb \ am335x-bone.dtb \ am335x-boneblack.dtb \ + am335x-bone-memory-cape.dtb\ am335x-nano.dtb \ am335x-base0033.dtb \ am3517-evm.dtb \ diff --git a/arch/arm/boot/dts/am335x-bone-memory-cape.dts b/arch/arm/boot/dts/am335x-bone-memory-cape.dts new file mode 100644 index 000..7ab088d --- /dev/null +++ b/arch/arm/boot/dts/am335x-bone-memory-cape.dts From discussions, option I could think are.. (a) NAND cape node added in both 'am335x-bone.dts' and 'am335x-boneblack.dts' but disabled by default. (b) NAND cape node in new '.dts' file (as mentioned above), and generate a separate blob individual for cape. (c) NAND cape node in existing 'am335x-bone-common.dtsi', disabled by default. But there is no guarantee that future boards remain compatible and same 'common_xx.dtsi' can be reused later. I'll wait for Tony/Benoit-C. to decide on what suits them, as they are the ones who have to maintain all these. Tony ? with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v1 3/3] ARM: dts: am437x-gp-evm: add support for parallel NAND flash
From: Menon, Nishanth On 03/12/2014 05:49 AM, Pekon Gupta wrote: Adds pinmux and DT node for Micron (MT29F4G08AB) x8 NAND device present on am437x-gp-evm board. (1) As NAND Flash data lines are muxed with eMMC, Thus at a given time either eMMC or NAND can be enabled. Selection between eMMC and NAND is controlled: (a) By dynamically driving following GPIO pin from software SPI2_CS0(GPIO) == 0 NAND is selected (default) SPI2_CS0(GPIO) == 1 eMMC is selected (b) By statically using Jumper (J89) on the board (2) As NAND device connnected to this board has page-size=4K and oob-size=224, So ROM code expects boot-loaders to be flashed in BCH16 ECC scheme for NAND boot. Signed-off-by: Pekon Gupta pe...@ti.com --- arch/arm/boot/dts/am437x-gp-evm.dts | 107 1 file changed, 107 insertions(+) diff --git a/arch/arm/boot/dts/am437x-gp-evm.dts b/arch/arm/boot/dts/am437x-gp-evm.dts index df8798e..0027ea7 100644 --- a/arch/arm/boot/dts/am437x-gp-evm.dts +++ b/arch/arm/boot/dts/am437x-gp-evm.dts which branch does this apply on? I assume you mean this for Tony's omap-for-v3.15/dt branch? it would be nice if you'd make that clear in cover letter. Sorry, I missed that. I'll keep a note of this next time. Yes, this patch series is rebased on git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap omap-for-v3.15/dt with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v1 1/3] ARM: dts: am335x-bone: add support for beaglebone NAND cape
Hi, From: Menon, Nishanth On 03/12/2014 05:49 AM, Pekon Gupta wrote: Beaglebone Board can be connected to expansion boards to add devices to them. These expansion boards are called 'capes'. This patch adds support for following versions of Beaglebone(AM335x) NAND capes (a) NAND Device with bus-width=16, block-size=128k, page-size=2k, oob-size=64 (b) NAND Device with bus-width=16, block-size=256k, page-size=4k, oob-size=224 Further information and datasheets can be found at [1] and [2] [...] [1] http://beagleboardtoys.info/index.php?title=BeagleBone_Memory_Expansion [2] http://beagleboardtoys.info/index.php?title=BeagleBone_4Gb_16-Bit_NAND_Module Signed-off-by: Pekon Gupta pe...@ti.com --- arch/arm/boot/dts/am335x-bone.dts | 123 ++ 1 file changed, 123 insertions(+) diff --git a/arch/arm/boot/dts/am335x-bone.dts b/arch/arm/boot/dts/am335x-bone.dts index 94ee427..be2c572 100644 --- a/arch/arm/boot/dts/am335x-bone.dts +++ b/arch/arm/boot/dts/am335x-bone.dts better to make a nand_cape dts which includes the am335x-bone.dts? Actually, I'm not in favor of having too many xx_board_common.dts files, because it un-necessarily complexes things. We already have arch/arm/boot/dts/am335x-bone-common.dtsi which just saves some lines common to DTS of both Beaglebone-LT(white) and Beaglebone-Black. And, there is no guarantee that Beaglebone-LT(white) will remain compatible to Beaglebone-black in future. Example: some capes are not compatible to beaglebone-black [1], [2]. So, I prefer to keep separate GPMC NAND nodes separately in both DTS, unless there is a strong convincing reason otherwise. [1] http://elinux.org/CircuitCo:BeagleBoardToys [2] http://elinux.org/BeagleBone_Black_Capes with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v1 1/3] ARM: dts: am335x-bone: add support for beaglebone NAND cape
-Original Message-
From: Menon, Nishanth
Sent: Thursday, March 13, 2014 2:21 AM
To: Tony Lindgren; Robert Nelson
Cc: Gupta, Pekon; bcous...@baylibre.com; linux-omap; linux-mtd
Subject: Re: [PATCH v1 1/3] ARM: dts: am335x-bone: add support for beaglebone
NAND cape
On 03/12/2014 02:28 PM, Tony Lindgren wrote:
* Robert Nelson robertcnel...@gmail.com [140312 12:11]:
On Wed, Mar 12, 2014 at 1:57 PM, Nishanth Menon n...@ti.com wrote:
On Wed, Mar 12, 2014 at 1:26 PM, Gupta, Pekon pe...@ti.com wrote:
Hi,
From: Menon, Nishanth
On 03/12/2014 05:49 AM, Pekon Gupta wrote:
Beaglebone Board can be connected to expansion boards to add devices to
them.
These expansion boards are called 'capes'. This patch adds support for
following versions of Beaglebone(AM335x) NAND capes
(a) NAND Device with bus-width=16, block-size=128k, page-size=2k,
oob-size=64
(b) NAND Device with bus-width=16, block-size=256k, page-size=4k,
oob-size=224
Further information and datasheets can be found at [1] and [2]
[...]
[1]
http://beagleboardtoys.info/index.php?title=BeagleBone_Memory_Expansion
[2]
http://beagleboardtoys.info/index.php?title=BeagleBone_4Gb_16-Bit_NAND_Module
Signed-off-by: Pekon Gupta pe...@ti.com
---
arch/arm/boot/dts/am335x-bone.dts | 123
++
1 file changed, 123 insertions(+)
diff --git a/arch/arm/boot/dts/am335x-bone.dts
b/arch/arm/boot/dts/am335x-bone.dts
index 94ee427..be2c572 100644
--- a/arch/arm/boot/dts/am335x-bone.dts
+++ b/arch/arm/boot/dts/am335x-bone.dts
better to make a nand_cape dts which includes the am335x-bone.dts?
Actually, I'm not in favor of having too many xx_board_common.dts files,
because it un-necessarily complexes things.
We already have arch/arm/boot/dts/am335x-bone-common.dtsi which just
saves
some lines common to DTS of both Beaglebone-LT(white) and
Beaglebone-Black.
And, there is no guarantee that Beaglebone-LT(white) will remain
compatible to
Beaglebone-black in future.
Example: some capes are not compatible to beaglebone-black [1], [2].
So, I prefer to keep separate GPMC NAND nodes separately in both DTS,
unless there is a strong convincing reason otherwise.
[1] http://elinux.org/CircuitCo:BeagleBoardToys
[2] http://elinux.org/BeagleBone_Black_Capes
Right, and adding NAND, GPMC nodes and asking folks to uncomment
sections into a generic board file (which by default has none) makes
more sense? I dont use NAND capes or might create my own cape. overo
has the same challenges as bone family has.I dont see asking folks to
uncomment entries to use the cape is a nicer alternative to having
more dts entries.
I think this is different discussion from previous one ..
common DTS file v/s replicated DTS entries in individual board files
because 'uncomment' issue will remain in both scenarios. (please read below)
This is just going to get more messy with every cape addition. Should
we maybe just leave a basic BeagleBone BeagleBone Black dts file in
mainline kernel.org.
Then create a repo on github.com/beagleboard/ with every
bone/black-first level cape.dts option?
Or just include all devices on the most common capes but have their
status as disabled by default.
For the pinctrl, we need to make sure the pins for a device are not
muxed if it's set to disabled state.
Yes, this is exactly what I intended. Therefore if you revisit the
uncomment section, then you would find that there is mmc2=disabled
because on beaglebone-black eMMC (MMC2) pinctrl conflicts with
NAND pin-ctrl.
+/*
+ * uncomment following to enable NAND cape support
+ * mmc2 {
+ * status = disabled;
+ * };
+ * elm {
+ * status = okay;
+ * };
+ * gpmc {
+ * status = okay;
+ * };
And I agree 'uncommenting' is not cleaner approach here, so if everyone
agrees, I can remove the above comment and just keep NAND DT node
disabled by default.
You do have a bunch of if you want nand, disable mmc2 configuration
in the usage of cape configurations such as this patch, or in the case
of [1] (audio cape), different regulator usage etc. Maintaining out of
tree cape dts might be an option, but that is prone to maintenance
burden as device tree conversion goes on (yeah, all the dt as ABI
stuff aside).
Keeping aside the subjective nature of what entitles a cape being a
common cape,even introducing nodes that belong to three or four
different first level capes will definitely have it's own sets of
problems.
The ideal solution would be some folks pitching in on what Matt
pointed in [2] - basically upstream a resource manager on
top of the basic overlay support that's in progress and introduce
capes as part of that overlay support once it is upstream.
I'm not sure how much capemgr or DT overlay is useful to larger
audience ?
(1) *Usually* actual custom board going to production will have
all devices populated on the board. fixed. It's very rare that an end
RE: [PATCH v1 1/2] mtd: nand: omap: fix ecclayout-oobfree-offset
Hi Brian,
From: Brian Norris [mailto:computersforpe...@gmail.com]
On Fri, Dec 13, 2013 at 02:42:57PM +0530, Pekon Gupta wrote:
This patch updates starting offset for free bytes in OOB which can be used by
file-systems to store their metadata (like clean-marker in case of JFFS2).
This should be describing a regression fix, right? We don't just
arbitrarily change the OOB free layout; we need a reason. So please
describe it here.
(It seems like an off-by-one, or off-by-N error, where the oobfree
region was miscalculated.)
Possibly you can paste an example intended ecclayout as well as an
incorrect layout that was calculated before this fix.
Signed-off-by: Pekon Gupta pe...@ti.com
---
drivers/mtd/nand/omap2.c | 17 -
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
[...]
-/* populate remaining ECC layout data */
-ecclayout-oobfree-length = mtd-oobsize - (BADBLOCK_MARKER_LENGTH +
-ecclayout-eccbytes);
for (i = 1; i ecclayout-eccbytes; i++)
ecclayout-eccpos[i] = ecclayout-eccpos[0] + i;
/* check if NAND device's OOB is enough to store ECC signatures */
@@ -1990,6 +1977,10 @@ static int omap_nand_probe(struct platform_device
*pdev)
err = -EINVAL;
goto return_error;
}
+/* populate remaining ECC layout data */
+ecclayout-oobfree-offset = ecclayout-eccpos[ecclayout-eccbytes] + 1;
Hmm, this line seems a little odd. The ecclayout-eccpos[] array and the
value of ecclayout-eccbytes sould be related as follows:
let N = ecclayout-eccbytes
This means the eccpos[] array should have N entries, indexed 0 to N-1,
and eccpos[N] is out of bounds. But you are accessing eccpos[N] above
(i.e., eccpos[ecclayout-eccbytes]). Are you sure this is correct? It
seems like it should be:
ecclayout-oobfree-offset = ecclayout-eccpos[ecclayout-eccbytes - 1]
+ 1;
Thanks for this catch. Yes, you are correct. It's a typo.
This wasn't caught as I had tested everything on UBIFS which does not use
'oobfree'.
Also, as ecclayout-eccpos is defined as large static array, so this dint
caused problems either.
#define MTD_MAX_ECCPOS_ENTRIES_LARGE640
struct nand_ecclayout {
__u32 eccpos[MTD_MAX_ECCPOS_ENTRIES_LARGE];
But, I think mtd_tests.nand_oobtest would have caught this. I'll include this
change in next version.
stripping down the CC list to avoid getting moderated by u-boot mailman
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v1 0/5] add parallel NAND support for TI's new OMAPx and AMxx platforms
Hi Benoit, Tony, From: Gupta, Pekon This patch-set adds and updates parallel NAND support on following TI platforms - AM335x (am335x-evm) - DRA7xx (dra7-evm - AM43xx (am43X-epos-evm) In addition, following OMAP2+/GPMC patch is also added in this series as it add checks DRA7xx and AM43xxx platforms for non-DT kernels. ARM: OMAP2+: gpmc: update gpmc_hwecc_bch_capable() for new platforms Please let me know, if this patch-set is in acceptable state, Or should I rebase it against any specific branch at your side ? with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v1 0/2] mtd: nand: omap: booting from NAND using u-boot
Hi Brian, From: Gupta, Pekon I'm preparing a 3.14 pull request soon, and since you seem committed to fixing and properly testing a known regression here, I'd like to see this go in. But given the late timing and the unanswered questions, I think it's unlikely to go in -rc1. Perhaps I can send a later pull request if we can get it together properly. Yes, if the above details are sufficient, then I'll: (1) re-word [PATCH 1/2] commit log title to mention that it's a clean-up (2) Add above description and details of regression into commit log of [PATCH 2/2] (3) Include your other comments on individual patches. So I'd like to first of all get a few answers to my questions, and then I think we might want to refresh this patch series with a little better commit messages and perhaps a separate documentation file (not necessarily in the same patch series, as the fix is more urgent). Do my previous responses look complete to you for re-submission ? with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v1 0/2] mtd: nand: omap: booting from NAND using u-boot
Hi Brian, From: Brian Norris Hi Pekon, Sorry, I'm revisiting your patch series a bit late. There are a few factors that contributed to this, though. 1. This patch series talks extensively about U-Boot. U-Boot is not my interest, nor should it be the focus of kernel (driver) development. Any work done here should be framed in the kernel driver context. [1] 2. You have previously CC'd me on U-Boot patches. Or at least, I think so. More about this in the next point... 3. The U-Boot source code structure is rather similar to pieces of Linux subsystems. So without additional effort, it is hard to discern whether a patch is intended for Linux MTD or U-Boot MTD. 4. You cross-posted to both the U-Boot and Linux-MTD mailing lists. So the sum of all this is that I ignored these patches at first, as they weren't clearly intended for me. On Fri, Dec 13, 2013 at 02:42:56PM +0530, Pekon Gupta wrote: As there were parallel set of patches running between u-boot and kernel. I don't know what patches you're talking about. Following patch-sets touched ecc.layout while cleaning up OMAP NAND driver. u-boot: http://lists.denx.de/pipermail/u-boot/2013-November/167551.html kernel: http://lists.infradead.org/pipermail/linux-mtd/2013-October/049414.html hence, some patch-sets caused regression for OMAP3x platforms when booting Hence is not the right word. The previous sentence doesn't imply that there were regressions. using u-boot specifically for ecc-schemes (like BCH4_SW). Huh? What does specifically for ecc-schemes mean? You mean that only some schemes had regressions? Can you be more specific? What are these regressions? It was reported by Enric Balletbo Serra eballe...@iseebcn.com [2] that when using 'OMAP_ECC_BCH8_CODE_HW_DETECTION_SW' ecc-scheme there was a mismatch in ECC layout between mainline u-boot and kernel. This caused boot failure when kernel tried to mount UBIFS with image flashed from u-boot. This was missed while testing earlier patch-sets because only OMAP3 boards use above ecc-scheme as OMAP3 SoC do not have ELM hardware engine. All the later platforms use 'OMAP_ECC_BCH8_CODE_HW' ecc-scheme. Both ecc-scheme are based on same algorithm of BCH8 ECC code except that: (1) 'OMAP_ECC_BCH8_CODE_HW_DETECTION_SW' uses /lib/bch.c software library for ecc-correction. (2) 'OMAP_ECC_BCH8_CODE_HW' uses ELM hardware engine for ecc-correction. This regression affects following ecc-schemes, as ecc.layout logic is just copy-paste of each-other. - 'OMAP_ECC_BCH4_CODE_HW_DETECTION_SW' and - 'OMAP_ECC_BCH8_CODE_HW_DETECTION_SW' The rest of this cover letter (and most of the patch descriptions) describes the configurations and testing (which is all very good, don't get me wrong), with a lot of focus on things I don't follow (namely, U-Boot), but you omit many of the important details, like * What is the regression? I honestly don't see a description of the actual regression. (I can read the code to intuit that, but what's the point of your pages of description if it doesn't mention this?) Please give some logical description of the problem [Patch 1/2] of this series actually simplification of 'ecclayout-oobfree-offset' It does not fix the regression, but it's important to clean it, before any new feature additions. [Patch 2/2] Actually fixes the regression for affected ecc-schemes. But this patch also touches other ecc-schemes as the fix required refactoring common code, into ecc-scheme specific code. * What are the effects of the regression? ECC bytes in the wrong place, so you see correction failures/corruption? JFFS2 failures? (The oobfree changes, for instance, should only affect something like JFFS2.) As mentioned above [PATCH 1/2] which touches 'ecclayout-oobfree-offset' is a clean-up not affecting regression, but it should be taken in before any further feature or ecc-scheme changes. Now, what is your preference? (a) Take [PATCH 1/2] ecc.layout clean-up separately. (b) Take ecc.layout as part of this series, but may be with different patch title. * What Linux commit caused the regression? * Should this patch set be marked for -stable? And for what kernel release? (the previous question would help answer this) This regression was caused in commit b491da7233d5dc1a24d46ca1ad0209900329c5d0 mtd: nand: omap: clean-up ecc layout for BCH ecc schemes So, this should be applicable from 3.13.x+ Hence this patch series fixes those regressions, and tests complete [...] snip I'm preparing a 3.14 pull request soon, and since you seem committed to fixing and properly testing a known regression here, I'd like to see this go in. But given the late timing and the unanswered questions, I think it's unlikely to go in -rc1. Perhaps I can send a later pull request if we can get it together properly. Yes, if the above details are sufficient, then I'll: (1) re-word [PATCH 1/2] commit log title to mention that it's a clean-up (2) Add above description
RE: [PATCH v1 0/2] mtd: nand: omap: booting from NAND using u-boot
Hi Brian,
From: Enric Balletbo Serra [mailto:eballe...@gmail.com]
2014/1/6 Stefan Roese s...@denx.de:
...
As there were parallel set of patches running between u-boot and kernel.
hence, some patch-sets caused regression for OMAP3x platforms when booting
using u-boot specifically for ecc-schemes (like BCH4_SW).
Hence this patch series fixes those regressions, and tests complete
NAND boot sequence for multiple ecc-schemes on AM335x-EVM board.
(following configurations are required for building u-boot driver which is
compatible to kernel ecclayout for selected ecc-scheme)
(BCH8_HW) (HAM1_HW) (HAM1_HW) (HAM1_HW) (UBIFS)
ROM - SPL - U-Boot - Kernel -
File-System
(BCH8_HW) (BCH8_SW) (BCH8_SW) (BCH8_SW) (UBIFS)
ROM - SPL - U-Boot - Kernel -
File-System
(BCH8_HW) (BCH8_HW) (BCH8_HW) (BCH8_HW) (UBIFS)
ROM - SPL - U-Boot - Kernel -
File-System
*Configurations used to build u-boot and kernel for end-to-end NAND boot*
+++--+
| ecc-scheme | u-boot/SPL configs| kernel DTS
|
+++--+
|||
|
| HAM1_HW| #define CONFIG_NAND_OMAP_ECCSCHEME \
|ti,nand-ecc-opts= |
|| OMAP_ECC_HAM1_CODE_HW |ham1
|
| (1-bit | #define CONFIG_SYS_NAND_ECCBYTES 3 |
|
| Hamming| #define CONFIG_SYS_NAND_ECCPOS \ |
|
| using h/w) | { 1, 2, 3, 4, 5, 6, 7, 8, 9, \|
|
||10, 11, 12 }|
|
|| (for NAND page-size=2048) |
|
|||
|
+++--+
|||
|
| BCH8_SW| #define CONFIG_NAND_OMAP_ECCSCHEME\
|ti,nand-ecc-opts= |
|| OMAP_ECC_BCH8_CODE_HW_DETECTION_SW |bch8
|
|(8-bit BCH | #define CONFIG_SYS_NAND_ECCBYTES 13 |(without ELM
node)|
| using s/w | #define CONFIG_BCH |
|
|library for | #undef CONFIG_SPL_NAND_AM33XX_BCH |
|
|for ECC | #define CONFIG_SPL_NAND_SIMPLE |
|
| error | #define CONFIG_SYS_NAND_ECCPOS \ |
|
|correction) | {2, 3, 4, 5, 6, 7, 8, 9, 10, \ |
|
|| 11, 12, 13, 14, \ |
|
|| 16, 17, 18, 19, 20, 21, 22, 23, 24, \ |
|
|| 25, 26, 27, 28, \ |
|
|| 30, 31, 32, 33, 34, 35, 36, 37, 38, \ |
|
|| 39, 40, 41, 42, \ |
|
|| 44, 45, 46, 47, 48, 49, 50, 51, 52, \ |
|
|| 53, 54, 55, 56, } |
|
|| (for NAND page-size=2048) |
|
|| #define CONFIG_SYS_NAND_ECCSIZE 512 |
|
|||
|
+++--+
|||
|
| BCH8_HW| #define CONFIG_NAND_OMAP_ECCSCHEME\
|ti,nand-ecc-opts= |
|| OMAP_ECC_BCH8_CODE_HW |bch8
|
|(8-bit BCH | #define CONFIG_SYS_NAND_ECCBYTES 14 |
|
| using ELM | #define CONFIG_SPL_NAND_AM33XX_BCH |(with ELM node)
|
| h/w engine | #define CONFIG_SYS_NAND_ECCPOS \
|ti,elm-id=elm |
|for ECC | {2, 3, 4, 5, 6, 7, 8, 9, \|
|
| error | 10, 11, 12, 13, 14, 15, 16, 17, \|
|
|correction) | 18, 19, 20, 21, 22, 23, 24, 25, \|
|
|| 26, 27, 28, 29, 30, 31, 32, 33, \|
|
|| 34, 35, 36, 37, 38, 39, 40, 41, \|
|
|| 42, 43, 44, 45, 46, 47, 48, 49, \|
|
|| 50, 51, 52, 53, 54, 55, 56, 57, }|
|
|| (for NAND page-size=2048) |
|
|| #define
RE: [PATCH v1 0/2] mtd: nand: omap: booting from NAND using u-boot
Hello Enric, Nikita, and other OMAP3 users,
As there were parallel set of patches running between u-boot and kernel.
hence, some patch-sets caused regression for OMAP3x platforms when booting
using u-boot specifically for ecc-schemes (like BCH4_SW).
Hence this patch series fixes those regressions, and tests complete
NAND boot sequence for multiple ecc-schemes on AM335x-EVM board.
(following configurations are required for building u-boot driver which is
compatible to kernel ecclayout for selected ecc-scheme)
(BCH8_HW) (HAM1_HW) (HAM1_HW) (HAM1_HW) (UBIFS)
ROM - SPL - U-Boot - Kernel - File-System
(BCH8_HW) (BCH8_SW) (BCH8_SW) (BCH8_SW) (UBIFS)
ROM - SPL - U-Boot - Kernel - File-System
(BCH8_HW) (BCH8_HW) (BCH8_HW) (BCH8_HW) (UBIFS)
ROM - SPL - U-Boot - Kernel - File-System
*Configurations used to build u-boot and kernel for end-to-end NAND boot*
+++--+
| ecc-scheme | u-boot/SPL configs| kernel DTS |
+++--+
||| |
| HAM1_HW| #define CONFIG_NAND_OMAP_ECCSCHEME \ |ti,nand-ecc-opts= |
|| OMAP_ECC_HAM1_CODE_HW |ham1|
| (1-bit | #define CONFIG_SYS_NAND_ECCBYTES 3 | |
| Hamming| #define CONFIG_SYS_NAND_ECCPOS \ | |
| using h/w) | { 1, 2, 3, 4, 5, 6, 7, 8, 9, \| |
||10, 11, 12 }| |
|| (for NAND page-size=2048) | |
||| |
+++--+
||| |
| BCH8_SW| #define CONFIG_NAND_OMAP_ECCSCHEME\|ti,nand-ecc-opts= |
|| OMAP_ECC_BCH8_CODE_HW_DETECTION_SW |bch8|
|(8-bit BCH | #define CONFIG_SYS_NAND_ECCBYTES 13 |(without ELM node)|
| using s/w | #define CONFIG_BCH | |
|library for | #undef CONFIG_SPL_NAND_AM33XX_BCH | |
|for ECC | #define CONFIG_SPL_NAND_SIMPLE | |
| error | #define CONFIG_SYS_NAND_ECCPOS \ | |
|correction) | {2, 3, 4, 5, 6, 7, 8, 9, 10, \ | |
|| 11, 12, 13, 14, \ | |
|| 16, 17, 18, 19, 20, 21, 22, 23, 24, \ | |
|| 25, 26, 27, 28, \ | |
|| 30, 31, 32, 33, 34, 35, 36, 37, 38, \ | |
|| 39, 40, 41, 42, \ | |
|| 44, 45, 46, 47, 48, 49, 50, 51, 52, \ | |
|| 53, 54, 55, 56, } | |
|| (for NAND page-size=2048) | |
|| #define CONFIG_SYS_NAND_ECCSIZE 512 | |
||| |
+++--+
||| |
| BCH8_HW| #define CONFIG_NAND_OMAP_ECCSCHEME\|ti,nand-ecc-opts= |
|| OMAP_ECC_BCH8_CODE_HW |bch8|
|(8-bit BCH | #define CONFIG_SYS_NAND_ECCBYTES 14 | |
| using ELM | #define CONFIG_SPL_NAND_AM33XX_BCH |(with ELM node) |
| h/w engine | #define CONFIG_SYS_NAND_ECCPOS \ |ti,elm-id=elm |
|for ECC | {2, 3, 4, 5, 6, 7, 8, 9, \| |
| error | 10, 11, 12, 13, 14, 15, 16, 17, \| |
|correction) | 18, 19, 20, 21, 22, 23, 24, 25, \| |
|| 26, 27, 28, 29, 30, 31, 32, 33, \| |
|| 34, 35, 36, 37, 38, 39, 40, 41, \| |
|| 42, 43, 44, 45, 46, 47, 48, 49, \| |
|| 50, 51, 52, 53, 54, 55, 56, 57, }| |
|| (for NAND page-size=2048) | |
|| #define CONFIG_SYS_NAND_ECCSIZE 512 | |
||| |
+++--+
#* In addition
RE: OMAP3 NAND ECC selection
From: Matthieu CASTET [mailto:matthieu.cas...@parrot.com] From: Pekon Gupta [mailto: pe...@ti.com] From: Thomas Petazzoni [mailto:thomas.petazz...@free-electrons.com] On Thu, 5 Dec 2013 11:24:18 -0800, Brian Norris wrote: [...] Using 1-bit ECC on NAND is not a long-term solution. Given that fact, I think your ROM code is what may need to change, not the entire MTD subsystem. As someone (Tom Rini maybe?) pointed out, today the shift is 1-bit ECC supported by ROM code vs. 4 or 8 bits required by NAND. But we can very well imagine that tomorrow ROM code will support BCH4 (and the NAND will ensure block 0 is OK for use with BCH4) but the rest of the NAND will require BCH16 or something like that. I'm not designing ROM code, and the fact that they today have this limitation, should be an indication that Linux should be capable of handling different ECC schemes to handle those hardware constraints. Just to highlight few more points: (1) ROM code on newer OMAP platforms like AM33xx do have the ability to select ECC scheme by reading a specific location from EEPROM connected to I2C0. AFAIK on omap3, the rom code first try to read data with bch and if it doesn't work it fallback on haming 1 bit ecc. I'm afraid, the OMAP35xx TRM does give much information about BCH ecc-scheme being used by ROM code. Though it says that BCH4 ecc-scheme would be selected for booting in cases when NAND_ID2 matches a particular value. But nothing much is clear (Reference [1]). Can you please point me to any other document or link which gives more info about the same ? (2) And going forward, ECC based NAND devices may be phased out, and BE-NAND (Built-in) NAND devices are becoming popular. As both cost and density wise they are same to SLC NANDs today. Thus issue of un-compatibility of ecc-scheme with ROM code, will not hold true. We already have some BE-NAND support in our generic driver. http://patchwork.ozlabs.org/patch/222186/ Yes but these flash are not always compatible with the ROM. If the rom is expecting some ECC and the internal controller expect other ecc you are stuck. For AM33xx and newer DRA7xx platforms, allows user to explicitly select between no ECC, BCH8 or BCH16. Thus ROM code of newer OMAP devices supports BE-NAND. (refer [2]). [1] http://www.ti.com/product/omap3530 http://www.ti.com/litv/pdf/spruf98x Chapter 25: Initialization Section 25.4.7.4 NAND Table 25-34. ID2 Byte Description [2] http://www.ti.com/product/am3359 http://www.ti.com/litv/pdf/spruh73i Chapter 26: Initialization Section: 26.1.7.4 Memory Booting NAND Table 26-17. NAND Geometry Information on I2C EEPROM with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: OMAP3 NAND ECC selection
Hi, From: Thomas Petazzoni [mailto:thomas.petazz...@free-electrons.com] On Thu, 5 Dec 2013 11:24:18 -0800, Brian Norris wrote: [...] Using 1-bit ECC on NAND is not a long-term solution. Given that fact, I think your ROM code is what may need to change, not the entire MTD subsystem. As someone (Tom Rini maybe?) pointed out, today the shift is 1-bit ECC supported by ROM code vs. 4 or 8 bits required by NAND. But we can very well imagine that tomorrow ROM code will support BCH4 (and the NAND will ensure block 0 is OK for use with BCH4) but the rest of the NAND will require BCH16 or something like that. I'm not designing ROM code, and the fact that they today have this limitation, should be an indication that Linux should be capable of handling different ECC schemes to handle those hardware constraints. Just to highlight few more points: (1) ROM code on newer OMAP platforms like AM33xx do have the ability to select ECC scheme by reading a specific location from EEPROM connected to I2C0. Reference: http://www.ti.com/product/am3359 http://www.ti.com/litv/pdf/spruh73i Chapter 26: Initialization Section: 26.1.7.4 Memory Booting NAND Table 26-17. NAND Geometry Information on I2C EEPROM (2) And going forward, ECC based NAND devices may be phased out, and BE-NAND (Built-in) NAND devices are becoming popular. As both cost and density wise they are same to SLC NANDs today. Thus issue of un-compatibility of ecc-scheme with ROM code, will not hold true. We already have some BE-NAND support in our generic driver. http://patchwork.ozlabs.org/patch/222186/ However, I also support user space tool approach rather than having this included in NAND driver. with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: OMAP3 NAND ECC selection
Hi Ezequiel, From: Ezequiel Garcia [mailto:ezequiel.gar...@free-electrons.com] [...] AFAIK, there's no hardware limitation that would prevent us from setting a per-partition ECC, keep in mind this effort is not reduced to make devicetree accept ECC on the partitions. I had some reservations in doing so.. (as mentioned in previous email also [2]) I would rather like to understand long term benefits of such implementation. Also, any constrain due to ROM code, or upgrading from remote can be handled using various alternative approaches like [a] and [b]. [2]: http://permalink.gmane.org/gmane.linux.ports.arm.omap/108136 [a]: http://www.mail-archive.com/linux-omap@vger.kernel.org/msg99025.html [b]: http://git.isee.biz/?p=pub/scm/writeloader.git;a=summary with regards, pekon
RE: [PATCH] ARM: dts: omap3-igep00x0: Fix nand ECC to maintain backward compatibility.
Hi, From: Enric Balletbo Serra [mailto:eballe...@gmail.com] CCing Pekon Gupta pe...@ti.com 2013/12/2 Thomas Petazzoni thomas.petazz...@free-electrons.com: Dear Javier Martinez Canillas, On Sun, 1 Dec 2013 13:27:25 +0100, Javier Martinez Canillas wrote: diff --git a/arch/arm/boot/dts/omap3-igep0020.dts b/arch/arm/boot/dts/omap3-igep0020.dts index d5cc792..4229e94 100644 --- a/arch/arm/boot/dts/omap3-igep0020.dts +++ b/arch/arm/boot/dts/omap3-igep0020.dts @@ -116,7 +116,7 @@ linux,mtd-name= micron,mt29c4g96maz; reg = 0 0 0; nand-bus-width = 16; - ti,nand-ecc-opt = bch8; + ti,nand-ecc-opt = ham1; A query Why are you going backward from BCH8 to HAM1 ? HAM1 is just kept for legacy reasons, it's not at all recommended for new development. As some field results have shown that devices with HAM1 become un-usable very soon and start reporting uncorrectable errors because HAM1 can only handle single bit-flip, which is inadequate in field conditions and large page device wears-n-tears. (especially considering your device density is of order of 4Gb - mt29c4g96maz). Also, just to inform that BCH8_SW ecc-scheme is implemented in such a way that *only* error correction is handled using s/w library (lib/bch.c). Rest all ECC calculation is handled using GPMC hardware. So, the CPU penalty will be seen only when there are bit-flips found during Read access, which are of rare conditions, occurring only few times in multi-megabit transfers. Also, On top of that ecc-schemes implementations have been optimized. And the patch-series is under review on mainline kernel. http://lists.infradead.org/pipermail/linux-mtd/2013-November/thread.html (Apologies for long suggestion, but in summary please don't use HAM1 for any new development. And with BCH8_SW you should see better bit-flip handling (longer device life) with no hit in performance). [...] Pekon, the old ti,nand-ecc-opt = sw should be replaced by ti,nand-ecc-opt = ham1 ? Should be the same ? In that case, why this different behavior ? In addition, please use HAM1 ecc-scheme on mainline u-boot also to flash. (following patches were accepted by domain maintainer 'Scott Woods'). http://lists.denx.de/pipermail/u-boot/2013-November/167548.html So, Kernel ham1 and u-boot ham1 should be in sync.. Once above is clean, you may like to pull another set of patches below (these are kernel equivalent of driver optimization for u-boot driver) http://lists.denx.de/pipermail/u-boot/2013-November/167445.html Also, for JFFS2, please erase the flash using -j option. '-j' option adds a clean marker to erased blocks. with regards, pekon
RE: [PATCH] ARM: dts: omap3-igep00x0: Fix nand ECC to maintain backward compatibility.
From: Thomas Petazzoni [mailto:thomas.petazz...@free-electrons.com] On Mon, 2 Dec 2013 14:56:09 +, Gupta, Pekon wrote: A query Why are you going backward from BCH8 to HAM1 ? HAM1 is just kept for legacy reasons, it's not at all recommended for new development. As some field results have shown that devices with HAM1 become un-usable very soon and start reporting uncorrectable errors because HAM1 can only handle single bit-flip, which is inadequate in field conditions and large page device wears-n-tears. (especially considering your device density is of order of 4Gb - mt29c4g96maz). Also, just to inform that BCH8_SW ecc-scheme is implemented in such a way that *only* error correction is handled using s/w library (lib/bch.c). Rest all ECC calculation is handled using GPMC hardware. So, the CPU penalty will be seen only when there are bit-flips found during Read access, which are of rare conditions, occurring only few times in multi-megabit transfers. Also, On top of that ecc-schemes implementations have been optimized. And the patch-series is under review on mainline kernel. http://lists.infradead.org/pipermail/linux-mtd/2013-November/thread.html (Apologies for long suggestion, but in summary please don't use HAM1 for any new development. And with BCH8_SW you should see better bit-flip handling (longer device life) with no hit in performance). The crucial point here is that the interaction between the bootloader and the kernel. The use case I have is that I'm flashing a filesystem image from the bootloader, and mounting it from the kernel. Using a mainline 2013.10 U-Boot for IGEPv2 + the mainline kernel booted in legacy mode (no Device Tree) works fine. Using the same 2013.10 U-Boot for IGEPv2 + the mainline kernel booted in DT no longer works, because the kernel disagrees with the bootloader on the ECC scheme to use. So I'm not saying that Hamming is better than BCH, certainly not. I'm just saying that changing ECC scheme in the kernel without looking at the more global picture of what support the bootloaders are offering is not nice. At least, the bootloader should provide a command, or option, to be able to use an ECC scheme that is compatible with what the kernel expects. Yes, there is a way to change ecc-scheme in u-boot.. Also, you can modify to any ecc-scheme in u-boot using CONFIG_NAND_OMAP_ECCSCHEME as documented in doc/README.nand Also your board should boot seamlessly from mainline u-boot in sync with mainline kernel. As per my knowledge following patch is already in mainline u-boot. And touches your board as well. (omap3_igep00x0.h) http://lists.denx.de/pipermail/u-boot/2013-November/167550.html AFAIK these patches should be in u-boot mainline. Though I have taken at-most care that no existing board should break. But, I'm sorry if there is something broken in between the u-boot and Kernel builds. Let me know if I can help in fixing that somehow. with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] ARM: dts: omap3-igep00x0: Fix nand ECC to maintain backward compatibility.
From: Thomas Petazzoni [mailto:thomas.petazz...@free-electrons.com] On Mon, 2 Dec 2013 11:00:35 -0500, Tom Rini wrote: Although the new ECC schema breaks the compatibility between the board files and new DT based kernel, I think we should use BCH8 scheme. Sorry, because I had not realized that this was configurable in u-boot, so I think, if Thomas is also agree, the better fix in that case is change CONFIG_NAND_OMAP_ECCSCHEME to OMAP_ECC_BCH8_CODE_HW_DETECTION_SW in u-boot. If this works we can discard this patch. I theoretically don't have anything against that, but if I do this change in U-Boot, and then use U-Boot to reflash to NAND the SPL and U-Boot itself, will the OMAP ROM code still be able to read the SPL from NAND ? I'm not sure which ECC scheme does the OMAP ROM code support, and how it detects (or not) which ECC scheme to use to read the SPL. Yes, this brings us back to one of the old and long-standing problems. The ROM on these devices will generally speak one format and that means using NAND chips that say for the first block (or N blocks or whatever) you only need 1bit ECC but for the rest 4/8/16/whatever. And then informing the kernel (and anything else) that partitions N need this format and the rest need that. As long as U-Boot provides separate commands, or a nandecc command that allows to switch between ECC scheme, and select the ECC scheme expected by the ROM code when flashing the SPL, and then the ECC scheme expected by the SPL and the kernel to flash U-Boot itself, the kernel image, and the various filesystem images, then it's all fine, we can leave with different ECC schemes used for different things on the NAND flash. Yes, at-least OMAP3 arch u-boot should still supports 'nandecc'. The infrastructure is still in place, however the command 'nandecc' is deprecated in newer versions. References in mainline u-boot: arch/arm/cpu/armv7/omap3/board.c @@do_switch_ecc() driver/mtd/nand/omap_gpmc.c @@omap_nand_switch_ecc() So with minor hacks, you should be able to bring-back 'nandecc'. But for all these, images need to be flashed from u-boot. As kernel cannot switch ecc-schemes on-the-fly. with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] ARM: dts: omap3-igep00x0: Fix nand ECC to maintain backward compatibility.
From: Thomas Petazzoni [mailto:thomas.petazz...@free-electrons.com] Dear Gupta, Pekon, On Mon, 2 Dec 2013 16:13:56 +, Gupta, Pekon wrote: Yes, at-least OMAP3 arch u-boot should still supports 'nandecc'. The infrastructure is still in place, however the command 'nandecc' is deprecated in newer versions. References in mainline u-boot: arch/arm/cpu/armv7/omap3/board.c @@do_switch_ecc() driver/mtd/nand/omap_gpmc.c @@omap_nand_switch_ecc() So with minor hacks, you should be able to bring-back 'nandecc'. So in short, what it means is that indeed the fact of switching to BCH8 on the kernel side is really breaking things, because U-Boot users now have the choice between: * Configuring U-Boot to use Hamming ECC, and be able to reflash their SPL, but not their filesystem images. * Configuring U-Boot to use BCH8, and be able to reflash their filesystem images, but not their SPL. Seems a little bit annoying for users, no? Yes, agree .. But this is only because we have mis-match in ecc-scheme between ROM-code (while reading SPL) v/s rest of system. However, if you continue using 'HAM1' for *both* u-boot and kernel you should not face any issue. And with latest patch on u-boot your board file should also remain unchanged. [...] But for all these, images need to be flashed from u-boot. As kernel cannot switch ecc-schemes on-the-fly. Which as I was saying, is a bit of shame. There is technically nothing that makes the ECC scheme something that needs to be applied globally on the entire flash. No, I don't think that kernel needs to ever dynamically switch ecc-schemes. This feature should be limited only to u-boot (or bootloaders) because: (1) Adding support for dynamic switching of ecc-schemes will require all code to be compiled in driver which increases the kernel driver footprint. (example adding BCH8_SW means you need to compile in lib/bch.c) (2) Also selection of ecc-scheme mainly depends on NAND device parameter (like density, page-size, oobsize) which remain constant for a device (all NAND partitions). Thus all partitions should use *same* ecc-scheme preferable highest possible available with NAND device kernel. (3) Kernel uses same driver instance to handle all MTD partitions, so if one partition uses HAM1 while other uses BCH8, and both are simultaneously mounted, then it would be difficult for driver to switch ecc-schemes while doing interleaved Read/Write between the partitions. (though it can be added in framework, but then it's too much over-head). In my opinion, kernel driver should be free from all over-heads, And should be *as lite as possible, while continuing to be strong in catching bit-flips.* Because there are lot of file-system layers over it, to handle more severe failures. Example: even if you use HAM1 and report un-correctable errors, still UBIFS has too much redundancy of critical metadata, that it can still recover your volume. Therefore, I preferred having ecc-scheme selectable via DT (static) for kernel. However these are purely my opinions based on my assessments. And we see real practical cases where being able to specify a different ECC scheme per partition would make sense: when the ROM code uses a weaker ECC scheme than the one used for most other partitions. This constrain of changing ecc-scheme has come because ROM code ecc-scheme is hardened to select HAM1. And so u-boot (bootloaders) is used to between bridge gaps between ROM code and kernel. - This could have been avoided, if u-boot still supported 'nandecc' OR - ROM code had mechanism to change its ecc-scheme based on some Boot-mode setting (sysboot pins on board). So coming back to the specific problem here. I think we need 'nandecc' back in u-boot till atleast all systems have migrated to BCH16 or whatever highest ecc-scheme which can be supported on OMAP devices. with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] ARM: dts: omap3-igep00x0: Fix nand ECC to maintain backward compatibility.
So coming back to the specific problem here. I think we need 'nandecc' back in u-boot till atleast all systems have migrated to BCH16 or whatever highest ecc-scheme which can be supported on OMAP devices. Forgot to mention, one more way of updating boot-loaders with different ecc-scheme via kernel. This can be helpful when: - you want to remotely upgrade your u-boot, but your kernel is statically build with different ecc-scheme. - In production environment, where you boot multiple devices in parallel (using say NFS boot), and then flash multiple devices without bothering about ecc-schemes.. *Method* (1) Flash the u-boot image on one *sample* device selecting appropriate ecc-scheme which ROM code understands. (2) Dump the complete image along with OOB appended (as a binary) (3) Use this binary image (with OOB included) to flash other devices from kernel as *raw* data (that means kernel will not append ECC while writing data, it will blindly write the image as-it-is on the partition). This way the ECC with which u-boot image was built in (1) will get programmed, irrespective of what kernel supports.. - I have seen at-least one customer going into production this way. - And I have been using this often too, though with older mtd-utils. Hope this helps .. with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC/PATCH] NAND bus-width detection extreme makeover
Hi Brian, Ezequiel, Sorry Im bit confused on below, so few queries .. From: Brian Norris [computersforpe...@gmail.com] So I think the problem may need to be divided into 2 parts: 1) How do we best handle ONFI transactions, so that they are always performed on the lower 8 bits of the bus **regardless of actual buswidth**? (I think Uwe's patch + my response in [1] might solve this.) 2) Can/should we relax the old restrictions where drivers have to configure the buswidth correctly before running nand_scan_ident(), in the spirit of NAND_BUSWIDTH_AUTO? And why do we need this flexibility? I think problem (1) is solvable independently of (2). So we don't inherently *need* BUSWIDTH_AUTO just to support ONFI correctly; we can just enforce that we ignore the upper 8 bits. Now, what do we have NAND_BUSWIDTH_AUTO for, anyway? It seems like its best use case is for a driver that (a) knows it might encounter either x8 or x16 flash chips and (b) is equipped to handle this change at runtime (e.g., it only needs to re-assign some call-backs after nand_scan_ident()). This means, for one, that it should have at least all 16 data lines connected. However, not all hardware/drivers fit (a) and (b). For such systems, we probably want to just indicate the expected buswidth and error out automatically if we detect this is incorrect. Sorry I'm bit confused here.. Where do you want to error out ? (a) In nand_base.c (generic driver) OR (b) you want the controller driver (callee of nand_scan_ident) to handle the bus-width mismatch on its own. I prefer (b), which is the basis of my first proposal patch. In my opinion following sequence should be followed by each controller driver during probe. Step-1: assign basic callbacks and parameters which are required for basic NAND device I/O (like chip-ctrl, chip-delay, etc) Step-2: call nand_scan_ident() nand_scan_ident() would populate detect NAND device parameter by - reading ONFI parameters in x8 bit mode (ignoring pre-configurations) - look-up from nand_flash_id[] And populate chip-options, mtd-writesize, etc... Step-3: On return from nand_scan_ident, each controller driver should check whether (chip_options NAND_BUSWIDTH_16) as set by nand_scan_ident() matches its controller board configurations or not. Based on which it can take corrective action or error out. If we follow above sequence then (1) NAND_BUSWIDTH_AUTO becomes implicit. And controller driver does not need to set any such flags before calling nand_scan_ident(). (2) Currently, we are error-out inside nand_base.c when (busw != chip-options BUS_WIDTH_16). Now this would be handled by individual controller drivers, as they know their hardware best. (If you remember, this was the reason of omap2,c calling nand_scan_ident() twice. If instead of failing the first call to nand_scan_ident would have just returned with *corrected* bus-width, then omap2,c could have handled that situation by re-configuring its controller). So I think we still want to support the following three configurations: !NAND_BUSWIDTH_16 !NAND_BUSWIDTH_AUTO: device must use 8-bit buswidth NAND_BUSWIDTH_16 !NAND_BUSWIDTH_AUTO: device must use 16-bit buswidth NAND_BUSWIDTH_AUTO: can support either buswidth (auto-detectable) But I think these selections should only be restricted by hardware limitations (lack of I/O pin connections, inflexible controller) and not enforced because of software limitations in handling ONFI. This brings be back to problem (1), where I think we need resolve it along the lines of Uwe's + my patch (see [1]), not by forcing NAND_BUSWIDTH_AUTO on all drivers. That's exactly what this patch is doing. Sorry, I'll re-review Uwe's + ur patch. But do you agree on following: (1) somehow we need to do away with NAND_BUSWIDTH_AUTO *macro*. And instead make its functionality, built-in the nand_base.c driver ? (2) And nand-bus-width DT binding will still be required for defining controller and board level description ... Is my understanding correct ? with regards, pekon-- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 0/1] Fix OMAP2 NAND ONFI device detection
From: Ezequiel Garcia [mailto:ezequiel.gar...@free-electrons.com] On Wed, Oct 30, 2013 at 09:18:53PM +, Gupta, Pekon wrote: I'm not sure, of course, but I don't see why not. It's more likely to break for x16 than it is for x8. Another question here is .. The above patch assumes that user has configured GPMC bus-width correctly, so if user is already providing GPMC bus-width information via DT, then do we really need to detect NAND device bus-width again using NAND_BUSWIDTH_AUTO ? Hm.. I think you're forgetting the original motivation for this patch, which was initially explained by you :-) The problem is ONFI doesn't work in 16-bit mode. Let me clarify. Since GPMC and NAND widths *must* match (otherwise, it wouldn't make much sense, right?) we don't really need to autodetect the NAND width. However, since ONFI doesn't work in 16-bit mode, we would have to do something like this (untested pseudo code): nand_scan_ident(...); if (platform_data-devsize == 16) nand_chip-options |= NAND_BUSWIDTH_16; And in some cases we might even get away with such solution. However, we can't guarantee nand_scan_ident() doesn't need to swith to 16-bit mode inside to perform other commands (maybe some ONFI extended parameter page). Therefore, and given the width can be autodetected in any case (array or ONFI based carry such information) it's much cleaner to simply do: nand_chip-options |= NAND_BUSWIDTH_AUTOMAGIC; nand_scan_ident(...); See? Much cleaner, no? but still dependent on DT property for GPMC configurations... I preferred NAND bus-width detection to be completely independent of 'any' DT property. And remember: the fact that we must know the device width to configure the GPMC correctly (which being a hardware parameter belongs to the DT) is OMAP specific. Other SoCs might not have such memory controller and thus won't need such knowledge before hand, although that sounds unlikely to me. The outcome of this discussion seems to be that no driver should ever need to specify the 'nand-bus-width' DT property, as Brian previously suggested, right? Right.. And this is where I'm suggesting that in-order to completely eliminate the dependency on 'nand-bus-width' DT property, you need to also update GPMC driver, so that its registers can be re-configured with correct NAND bus-width after nand_scan_ident(). I must admit I'm a bit puzzled by seeing most drivers setting 16-bit before calling nand_scan_ident(). I guess none of them relies on ONFI? May be, but going forward we should make NAND_BUSWIDTH_AUTO as a mandatory option, because most of the NAND devices have adopted to ONFI, and remaining legacy (if any) are part of nand_flash_id[] table. So either way the device would get detected, if you start with x8 (lowest minimum capability bus-width). Thus there is no need for user to specify NAND device bus-width property via DT. with regards, pekon
RE: [PATCH 0/1] Fix OMAP2 NAND ONFI device detection
From: Ezequiel Garcia
[...]
But: on the other hand, I'd really like you to convince me as to
why is it so bad to require the DTB to have the proper GPMC bus width.
No its not at all bad, all I want is either of the one way (not mixture of
both).
- Either depend on DT completely (which is current case for all drivers)
- OR depend on ONFI and nand_flash_id[] for bus-width detection.
Once again:
1. the NAND devices aren't hot-pluggable
2. the user (who is actually an engineer, not some regular dummy user)
knows perfectly well the width of the device.
What's the problem with describing the hardware in the DT and saving us
lots of runtime re-configuration trouble?
I agree with both your arguments above.
So shouldn't we kill NAND_BUSWIDTH_AUTO ?
And probably therefore NAND_BUSWIDTH_AUTO isn't that popular.
Now what remains is ONFI probe, which should always happen in x8 mode.
So for that below patch should be sufficient ..
--
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index ec1db1e..d1220fb 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2942,14 +2942,8 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd,
struct nand_chip *chip,
chip-read_byte(mtd) != 'F' || chip-read_byte(mtd) != 'I')
return 0;
- /*
-* ONFI must be probed in 8-bit mode or with NAND_BUSWIDTH_AUTO, not
-* with NAND_BUSWIDTH_16
-*/
- if (chip-options NAND_BUSWIDTH_16) {
- pr_err(ONFI cannot be probed in 16-bit mode; aborting\n);
- return 0;
- }
+ /* ONFI must be probed in 8-bit mode only */
+ nand_set_defaults(chip, 0);
chip-cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
for (i = 0; i 3; i++) {
@@ -2962,7 +2956,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd,
struct nand_chip *chip,
if (i == 3) {
pr_err(Could not find valid ONFI parameter page; aborting\n);
- return 0;
+ goto return_error;
}
/* Check version */
@@ -2980,7 +2974,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd,
struct nand_chip *chip,
if (!chip-onfi_version) {
pr_info(%s: unsupported ONFI version: %d\n, __func__, val);
- return 0;
+ goto return_error;
}
sanitize_string(p-manufacturer, sizeof(p-manufacturer));
@@ -3033,6 +3027,12 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd,
struct nand_chip *chip,
}
return 1;
+
+return_error:
+ /* revert original bus-width */
+ nand_set_defaults( chip-options NAND_BUSWIDTH_16);
+ return 0;
+
}
/*
-
with regards, pekon
RE: [PATCH v11 00/10] [PATCH v10 00/10] mtd:nand:omap2: clean-up of supported ECC schemes
Hi Tony, From: Tony Lindgren * Brian Norris computersforpe...@gmail.com [131029 21:00]: Tony, you mentioned the DTS update in patch 8 going in via an ARM tree? This patch is not urgent, and it should probably wait until we know what release the rest of the series makes it into. This may depend on David Woodhouse's recommendation, but I'm not sure this series will have enough time baking in linux-next before entering mainline in 3.13 (the merge window is approaching). Yes Benoit or I can apply that patch if Pekon pings me or resends that patch when it's OK to merge it. Yes, I'll keep track of this and would resend you and Benoit the .dts patch separately when this these binding updates are merged in kernel. with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 0/1] Fix OMAP2 NAND ONFI device detection
Hi Brian, Ezequiel Garcia, Some replies to your queries... From: Brian Norris [mailto:computersforpe...@gmail.com] On Wed, Oct 30, 2013 at 06:53:07AM -0300, Ezequiel Garcia wrote: [...] I do have one curiosity here: omap2.c looks like it's essentially defaulting to the NAND_OMAP_POLLED callbacks during nand_scan_ident(), since omap2.c only initializes the read_buf callback after nand_scan_ident(). Is this ever a problem? Or should omap2.c be initializing its callbacks both before and after nand_scan_ident() (similar to how nand_base calls nand_set_defaults() twice for the AUTO case)? There is no issue if _default_ functions present in nand_base.c are used for chip-read_byte() or chip-read_buf() while probing the device. The different callbacks defined in omap2.c like - omap_read_buf() - omap_read_buf_pref() - omap_read_buf_dma_pref() - omap_read_buf_irq_pref() Above are alternatives for having better throughput in different use-cases. These are not tied to hardware. So it's okay if these callbacks are assigned post nand_scan_ident(). What value do you use for ti,nand-xfer-type in your BeagleBone device tree? Is the xfer type a hardware requirement, or just a software configuration? IOW, does the hardware care if I simply use POLLED, even temporarily? (A potential side issue: does ti,nand-xfer-type even belong in the device tree, if it is not actually a hardware property?) DMA and IRQ based callbacks have better throughput numbers than POLLED ones. Though its not related to hardware, but as it's there in DT now so we should maintain it. Also considering that: - some older platforms might not support xx_dma_pref(). - some related pieces of information (like IRQ number, etc) comes from DT. It was added as part of following patch.. http://www.spinics.net/lists/linux-omap/msg90250.html This time I've decided to submit this patch alone, so we can focus the discussion on this issue. The other cleanups can wait. AFAICS, the remaining questions are: 1. Does this work in the 8-bit case? (I'm not able to test such case for lack of hardware) I'm not sure, of course, but I don't see why not. It's more likely to break for x16 than it is for x8. Another question here is .. The above patch assumes that user has configured GPMC bus-width correctly, so if user is already providing GPMC bus-width information via DT, then do we really need to detect NAND device bus-width again using NAND_BUSWIDTH_AUTO ? 2. Do we want to re-configure the GPMC one the NAND core detects the device bus width? I'm not quite sure here, as I don't know if I know enough about the GPMC to give an educated response here. Additionally, I'm not quite sure how re-configurable GPMC really is. It seems like we would be restricted physically if GPMC is hooked up as x8 for an x16 NAND (there are not enough pins connected). So even if we detect the NAND, it cannot function. I'm not sure about the reverse. Now, this returns me to my rejection of Pekon's patch here: http://lists.infradead.org/pipermail/linux-mtd/2013-October/049154.html (This patch addresses the question of checking the GPMC configuration, not correcting it.) I'll admit that I was underinformed regarding the need for this type of patch. Since that time, I am less sure of my criticism. But really, I just have more questions now. Does the GPMC intrinsically (physically; according to its pin configuration) restrict an attached device (e.g., NAND) to use x16 or x8? Yes.. Or does it simply specify a maximum data width that is wired up? (I'm presuming that you could wire an x8 device to an x16 interface and just leave the upper 8 unconnected...) Or is there some third option? GPMC engine uses bus-width info to drive its data-lines. - If GPMC is configured as x8, then it will only perform I/O on D0.. D7, even if all D0 .. D15 were wired on board. - If GPMC is configured as x16, then it will perform I/O on D0.. D15, even if only D0 .. D7 were wired on board. There by reading 0x0 or garbage on D8 .. D15 lines. This does not affect the probe, because chip-read_byte() would return only single byte whether it's a x8 device or x16 device. So READID_CMD works fine and you can read maf_id and dev_id. And based on that device parameters can be looked from nand_flash_id[]. However when it comes to reading or writing complete page, then the mismatch between GPMC configuration and actual NAND device bus-width is seen, because there chip-read_buf() or chip-write_buf() is used. The DT entry says: gpmc,device-width Total width of device(s) connected to a GPMC chip-select in bytes. The GPMC supports 8-bit and 16-bit devices and so this property must be 1 or 2. So, this *sounds* like it specifies the exact width. But as I read it, this property could more optimally be specified as the *maximum* width... Yes, its exact
RE: [PATCH v11 00/10] [PATCH v10 00/10] mtd:nand:omap2: clean-up of supported ECC schemes
From: Brian Norris [mailto:computersforpe...@gmail.com] [...] I agree with Ezequiel's thoughts, since the excessive amount of noise in this patch series has delayed it significantly. But at this point, I think it has stabilized; we have reviews from the DT folks (thanks guys; please comment if you have an official ack to give), and I think we've retained backwards compatibility properly; I've combed through it a few times over the months; we have a third-party tester; and at this point, I'm sure we're all sick of this. So, without further delay: pushed all patches except path 8 to l2-mtd.git. Thanks much .. I'll ensure that my next series are more logically aligned. Tony, you mentioned the DTS update in patch 8 going in via an ARM tree? This patch is not urgent, and it should probably wait until we know what release the rest of the series makes it into. This may depend on David Woodhouse's recommendation, but I'm not sure this series will have enough time baking in linux-next before entering mainline in 3.13 (the merge window is approaching). Pekon/Ezequiel/others: please feel free to send any follow up cleanups for this driver. I'll take a look at what Ezequiel has already sent out and see if it's still applicable on top. Yes, I have other pending series, which I'll resend after rebasing on this v11. But those are limited to internal NAND driver clean-up only And do not touch DT or any other dependent driver. with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2 0/5] Assorted OMAP2 NAND clean-ups
Hi Ezequiel Garcia, Sorry I'm bit out of my place.. so not able to sync often with mails.. However plz see my replies below.. [...] Pekon, Brian: Do you think this solution might work for 8-bit and 16-bit devices? I think NAND_BUSWIDTH_AUTO (without GPMC changes) would fail in following scenarios.. Case-1: configuring gpmc,device-width=1 from DT when using x16 device. ... which is wrong. That's why we have a DT property to configure that. The GPMC *must* be properly configured. No, I think the main intention of using NAND_BUSWIDTH_AUTO is that your controller should not depend on DT or any other user-input to detect NAND bus-width, (whether its gpmc,device-width or anything else). As your NAND driver is using NAND_BUSWIDTH_AUTO, it should ignore this DT config, and based on ONFI params it should work as x16 Hm.. I don't think so. The auto-stuff is just for the NAND driver, not for the memory controller. I don't know much about hardware, but in my mind I imagine them as different controllers. TI's GPMC hardware engine can do much more than supporting only NAND devices, therefore there is an independent GPMC driver, and dependent NAND driver. So when a NAND device is connected all GPMC driver does is initializes GPMC hardware based on static DT bindings, and pass the control to NAND driver. But there should be a mechanism to override these static DT configurations done in GPMC driver by NAND driver. (it is this piece which is missing today). Case-2: configuring gpmc,device-width=2 from DT when using x8 device. ... which is also wrong. Once again, you're mis-configuring the GPMC. We cannot expect the NAND driver to work properly if the GPMC is not properly initialized, don't you think? Actually having NAND_BUSWIDTH_AUTO would require change in GPMC driver also.. please refer my comments in.. http://lists.infradead.org/pipermail/linux-mtd/2013-October/049284.html Well, I think the approach should be different and much simpler: GPMC *must* be properly configured and then NAND can do ONFI detection starting in 8-bit and then switching to 16-bit if needed. This is what this patch is doing: it _fixes_ the NAND driver ONFI detection, _provided_ the GPMC is well-prepared. You seem to think that GPMC + NAND should be able to automagically detect the device and work, but I don't think that's even physically possible, for the reasons you have just exposed. I think this fix is simple enough. No, I still think there should be a way to tell the user that there is a mis-match between bus-width configured in DT, and that detected by ONFI probe. If it was straight forward, this would have been already accepted earlier :-) http://lists.infradead.org/pipermail/linux-mtd/2013-October/049154.html (read the comments from Brian by following above patch thread) Problem is if there is a mis-match in your bus-width due to incorrect GPMC DT binding configurations, still NAND probe and everything will pass correctly, because nand_scan_ident() uses read_byte() which returns same for both x8 and x16 devices. You will find issues when you actually do page reads and writes. And this is where you would see corrupted half-page data. And user would be clueless on why he is seeing such erratic behavior. This is why I added checks for mismatch of DT binding right during nand probe(), in above mentioned patch. I could just remove calling nand_scan_ident() second-time and it becomes your patch, but then it would be just validating the DT setting, not pure auto-detection. BTW: The GPMC code ignores the DT value in 'gpmc,device-width' and uses 'nand-bus-width' instead, but that's a different bug and a different fix :) I think you mean the opposie.. GPMC driver uses gpmc,device-width.. Refer: $KERNEL/arch/arm/mach-omap2/gpmc.c @@gpmc_read_settings_dt(...) of_property_read_u32(np, gpmc,device-width, p-device_width); 'nand-bus-width' is not used anywhere instead.. with regards, pekon
RE: [PATCH v2 4/5] mtd: nand: omap2: Use devm_kzalloc
Hi, This simplifies the error path and makes the code less error-prone. Signed-off-by: Ezequiel Garcia ezequiel.gar...@free-electrons.com --- drivers/mtd/nand/omap2.c | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c index e01a936..d3155b2 100644 --- a/drivers/mtd/nand/omap2.c +++ b/drivers/mtd/nand/omap2.c @@ -1837,7 +1837,7 @@ static int omap_nand_probe(struct platform_device *pdev) return -ENODEV; } - info = kzalloc(sizeof(struct omap_nand_info), GFP_KERNEL); + info = devm_kzalloc(pdev-dev, sizeof(*info), GFP_KERNEL); if (!info) return -ENOMEM; @@ -2067,8 +2067,6 @@ out_release_mem_region: free_irq(info-gpmc_irq_fifo, info); release_mem_region(info-phys_base, info-mem_size); out_free_info: - kfree(info); - return err; } @@ -2091,7 +2089,6 @@ static int omap_nand_remove(struct platform_device *pdev) nand_release(info-mtd); iounmap(info-nand.IO_ADDR_R); release_mem_region(info-phys_base, info-mem_size); - kfree(info); return 0; } -- 1.8.1.5 I think these changes are already done as part of following patch.. http://lists.infradead.org/pipermail/linux-mtd/2013-October/049418.html Did your rebase on my patch-set ? with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2 4/5] mtd: nand: omap2: Use devm_kzalloc
From: Ezequiel Garcia [mailto:ezequiel.gar...@free-electrons.com] Hm.. well the problem with that patch is that it's in the middle of an unrelated series. As I already told you, I think you should have pushed that as a one-patch fix. Have you seen that suggestion? Yes, I know.. actually the original patch series, when it started somewhere April (or before) is very different from the version v11 now :-). This devm_ update was added in middle of v6-v7 version change (Most of the changes since first version of this patchset is captured in Cover-letter). On the other side, you're fixing too many things in that single patch, for my taste. Maybe I'm not the smarter developer, but going through that patch is not easy to catch if there's no mistake done. Usually if it's possible to split a patch (maintaining consistency) it makes the reviewing process easier. If you'd rather send this devm_xxx change yourself that's fine by me, Ahh nothing like that.. Brian had already reviewed these couple of times And it was only [Patch 04/10] which was last one remaining.. I just said it because this might show up in merge conflict .. or rejects.. but *please* split the patch in two and write proper commit messages. Anyway: this is just a silly change, the important one is the other nand_scan_ident() fix. Could you help me review that? I'm interested in knowing how will that work with 8-bit and 16-bit devices. -- Yes, I'm just preparing the scenario where BUSWIDTH_AUTO would fail.. unless you do GPMC driver changes also.. same issue was found by Matthieu CASTET (matthieu.cas...@parrot.com) (please see my other mail) with regards, pekon
RE: [PATCH v2 0/5] Assorted OMAP2 NAND clean-ups
Hi, -Original Message- From: Ezequiel Garcia [mailto:ezequiel.gar...@free-electrons.com] [...] Pekon, Brian: Do you think this solution might work for 8-bit and 16-bit devices? I think NAND_BUSWIDTH_AUTO (without GPMC changes) would fail in following scenarios.. Case-1: configuring gpmc,device-width=1 from DT when using x16 device. As your NAND driver is using NAND_BUSWIDTH_AUTO, it should ignore this DT config, and based on ONFI params it should work as x16 Case-2: configuring gpmc,device-width=2 from DT when using x8 device. As your NAND driver is using NAND_BUSWIDTH_AUTO, it should ignore this DT config, and based on ONFI params it should work as x8 NAND device may get detected correctly, but try doing write and read to NAND, and I think, it would fail for Case-2 at-least.. Can you please check ? I've been going through the GPMC code again, and it looks like some cleaning is needed there two, but that's a different story! Actually having NAND_BUSWIDTH_AUTO would require change in GPMC driver also.. please refer my comments in.. http://lists.infradead.org/pipermail/linux-mtd/2013-October/049284.html reason of failure is given above... with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2 3/5] mtd: nand: omap2: Fix OMAP_BCH option dependency
From: Ezequiel Garcia [mailto:ezequiel.gar...@free-electrons.com] Subject: [PATCH v2 3/5] mtd: nand: omap2: Fix OMAP_BCH option dependency This option does not need to depend in MTD_NAND, for it's enclosed under it. Also, it's wrong to make it depend in ARCH_OMAP3 only since the controller is used in a wider range of SoCs. Instead, just leave the dependency on the OMAP2 driver option. Signed-off-by: Ezequiel Garcia ezequiel.gar...@free-electrons.com --- drivers/mtd/nand/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig index d885298..8187466 100644 --- a/drivers/mtd/nand/Kconfig +++ b/drivers/mtd/nand/Kconfig @@ -95,7 +95,7 @@ config MTD_NAND_OMAP2 platforms. config MTD_NAND_OMAP_BCH - depends on MTD_NAND MTD_NAND_OMAP2 ARCH_OMAP3 + depends on MTD_NAND_OMAP2 tristate Enable support for hardware BCH error correction default n select BCH -- 1.8.1.5 Acked-by: Pekon Gupta pe...@ti.com Reported-by: Ezequiel Garcia ezequiel.gar...@free-electrons.com with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v10 04/10] mtd: nand: omap: fix device scan: NAND_CMD_READID, NAND_CMD_RESET, CMD_CMD_PARAM use only x8 bus
Hi Ezequiel, From: Ezequiel Garcia [mailto:ezequiel.gar...@free-electrons.com] I won't be able to make too much progress without some help or without squeezing my brains out :P Care to push some git branch on some random repo with DT support for the NAND cape in the Beaglebone? Apologies for delay, I was testing and preparing a newer version of patch-set.. Please find the beagle-bone DTS attached. However, consider this as RFC only, as I'm waiting for current series which has some DT-binding updates to get accepted first. Commit log in the patch would also guide you for some board tweaks for enabling NAND cape on beagle-bone (LT/white).. (I have recently sent v11 of the patch series incorporating last few comments from Brian about nand_scan_ident().) Thanks.. with regards, pekon 0001-ARM-DTS-am335x-bone-enable-support-for-NAND-cape.patch Description: 0001-ARM-DTS-am335x-bone-enable-support-for-NAND-cape.patch
RE: [PATCH v10 04/10] mtd: nand: omap: fix device scan: NAND_CMD_READID, NAND_CMD_RESET, CMD_CMD_PARAM use only x8 bus
Hi Brian,
From: Brian Norris [mailto:computersforpe...@gmail.com]
On 10/22/2013 10:07 PM, Gupta, Pekon wrote:
From: Brian Norris [mailto:computersforpe...@gmail.com]
On Sat, Oct 19, 2013 at 02:14:08PM +0530, Pekon Gupta wrote:
[...]
So are you saying that the driver currently doesn't work if you started
in x16 buswidth? Are you having problems with a particular setup? What
sort of devices are you testing?
...
Since you didn't answer the other 2 questions there: are you testing any
x16 devices?
Ans-1: Yes, I'm testing on following boards:
(a) AM335x-EVM which has x8 Micron device.
http://www.ti.com/tool/tmdxevm3358
(b) beaglebone with 'NAND cape', which has x16 Micron device.
http://beagleboardtoys.info/index.php?title=BeagleBone_4Gb_16-Bit_NAND_Module
(c) AM437x board (which has 4K page NAND from Macronix).
(d) Also would be testing on DRA7xx again having Micron Device.
Ans-2:
Its not the setup but rather constrain in nand_base.c which prevents
reading of ONFI params in x16 mode. Please read below..
Ans-3:
Mostly are either x8 or x16 SLC NAND device.
[...]
You NEVER need to call nand_scan_ident() twice for the same chip.
Period. I will reject any patch that retains this pattern. It is wrong,
and I seriously doubt the code does what you think it does when you do this.
I think nand_scan_ident() may have a weak point where it won't support
ONFI properly for x16 devices. I believe NAND_BUSWIDTH_AUTO was added
to
help with this fact. (I don't have any x16 devices to test it.) But if
this is a problem for you, fix it. Don't work around it.
So, here comes the conflict..
If I'm _not_ using NAND_BUSWIDTH_AUTO, how should I read the ONFI
params on x16 device ? I don't think there is any way because of following
code in generic nand_base.c
@@ nand_flash_onfi_detect()
/*
* ONFI must be probed in 8-bit mode or with NAND_BUSWIDTH_AUTO, not
* with NAND_BUSWIDTH_16
*/
if (chip-options NAND_BUSWIDTH_16) {
pr_err(ONFI cannot be probed in 16-bit mode; aborting\n);
return 0;
}
Introduced in commit..
commit 0ce82b7f7b7373b16ecf7b5725e21e2975204500
Author: Matthieu CASTET matthieu.cas...@parrot.com
AuthorDate: 2013-01-16
And, I think this commit has implicitly made NAND_BUSWIDTH_AUTO
*mandatory* to be supported by every driver for interfacing with
x16 NAND devices. Isn't it ?
(unless you add every x16 device present in market to nand_flash_id[])
[...]
nand_base is designed (and it's documented in the comments) that the
driver must set the buswidth correctly BEFORE calling nand_scan_ident().
You may not use nand_scan_ident() as a trial-and-error identification
function.
So, to properly do (1), you should only have something like this, just
like all the other NAND drivers:
nand_chip-options = pdata-devsize NAND_BUSWIDTH_16;
ret = nand_scan_ident(...);
if (ret) {
// exit with error code...
}
For a x16 device.. This would *always* fail for x16 device, unless device
is listed in nand_flash_id[]. isn't it ?
because you cannot read ONFI params in x16 mode, and your device is
not listed in nand_flash_ids[], so your device would not get identified.
And I sincerely don't know how other NAND drivers are probing
x16 NAND devices _without_ enabling NAND_BUSWIDTH_AUTO.
(may be by adding every supported NAND device to nand_flash_id[]
look-up table, which is not recommended).
If there is a problem with this, then you have to fix your driver or
nand_scan_ident(). Perhaps you have to adjust your readbuf() or
cmdfunc() callbacks to do this. But do not add complicated workaround
logic in your driver.
chip-cmdfunc() and chip-readbufs() are all fine. Rather I let the
generic driver set them for nand_scan_ident().
So, all this calling nand_scan_ident() twice was done because of
previously mentioned reasons..
[snipping the rest]
I read your patch, and I gave you my review. I will not accept this
patch, nor any patch that works around nand_scan_ident() by calling it
twice. Fix the framework if the framework is giving you problems.
It's a chicken and egg problem,
I have no solution but all I can say is the above commit, which converted
WARNING into ERROR, until all drivers adapt to NAND_BUSWIDTH_AUTO.
Anyway I have to do nand_scan_ident() somewhere before populating
the chip-ecc.layout and other fields.. so can't drop the patch..
But I'll put your suggestion, instead of my mine.
I believe that this patch is not integral to the rest of the series, so
I will repeat: separate this patch out so I can take the rest of this
series without it.
I'll replace the patch with your suggestions,
So, that you have both versions. Please pick whichever you like :-)
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v10 04/10] mtd: nand: omap: fix device scan: NAND_CMD_READID, NAND_CMD_RESET, CMD_CMD_PARAM use only x8 bus
Hi,
From: Ezequiel Garcia [mailto:ezequiel.gar...@free-electrons.com]
[...]
FWIW, I have a Beaglebone with a 16-bit bus NAND attached to it.
Coincidentally, yesterday I was doing some tests as I'm ramping up the
NAND and I found that weird double nand_scan_ident() call.
The whole thing looks buggy to me, so I'm happy to help, review, test
and patches to take care of this.
Yes, thanks .. that would be of great help..
And may be your experience of Atmel drivers would help me here..
*Correct, should not be double calls to nand_scan_ident()..*
But there is a constrain in nand_base.c, that it does not allow ONFI
page reading in x16 mode.. So how to overcome that..
I see the similar implementation in your ATMEL driver, it does not use
NAND_BUSWIDTH_AUTO so how do you perform ONFI read
for x16 devices ?
drivers/mtd/nand/atmel_nand.c @@atmel_nand_probe()
/* here you move to x16 mode based on your DT or platform data */
if (host-board.bus_width_16) /* 16-bit bus width */
nand_chip-options |= NAND_BUSWIDTH_16;
/* And then you call nand_scan_ident */
/* first scan to find the device and get the page size */
if (nand_scan_ident(mtd, 1, NULL)) {
res = -ENXIO;
goto err_scan_ident;
}
Wouldn't this fail, _unless_ your device is listed in nand_flash_id[] ?
because it would not be able to read ONFI params..
Refer below commit..
commit 0ce82b7f7b7373b16ecf7b5725e21e2975204500
Author: Matthieu CASTET matthieu.cas...@parrot.com
AuthorDate: 2013-01-16
Thanks for pitching in, this would help me to understand this better.
I'm using some TI SDK with some ancient v3.2.x (with no git history!),
but from this discussion it seems the issue is still present in
mainline.
Aah sorry, then you might have some problem here in rebasing the
patches. But still if you can, thanks much ..
with regards, pekon
RE: [PATCH v10 04/10] mtd: nand: omap: fix device scan: NAND_CMD_READID, NAND_CMD_RESET, CMD_CMD_PARAM use only x8 bus
Hi Brian, From: Brian Norris [mailto:computersforpe...@gmail.com] On Sat, Oct 19, 2013 at 02:14:08PM +0530, Pekon Gupta wrote: [...] Thus this patch run nand_scan_ident() with driver configured as x8 device. So are you saying that the driver currently doesn't work if you started in x16 buswidth? Are you having problems with a particular setup? What sort of devices are you testing? No, I'm saying that you cannot read ONFI params in x16 mode. And, that is what was pointed out in following commit log also .. (Reference to 3.3.2. Target Initialization: given above) So, if I run nand_scan_ident() in x16 mode, my ONFI detection would fail for sure .. Running nand_scan_ident() with x8 when the device is actualy x16 will just cause nand_scan_ident() to abort with an error. It doesn't help you with the fact that RESET/READID/PARAM need special 8-bit handling on x16 devices, so you're not solving the problem alluded to by Matthieu. Yes, absolutely agree.. The original code was calling nand_scan_ident() twice, without taking into consideration whether the first nand_scan_ident() failed because of bus-width mismatch or something else. I'm also calling nand_scan_ident() twice, but only when the failure may be due to bus-width mis-match. I'm just avoiding an extra call to nand_scan_ident() if the failure was genuine. If the device actually was x8 then nand_scan_ident() should not fail for the first-time (in my patch), but if it still fails, then it's a genuine failure _not_ because of bus-width mismatch. I'm avoiding the call to nand_scan_ident() again .. (same is in my comments below..) [...] What is your patch trying to solve? It seems like it's just a distortion of the same code that existed previously. It tries running nand_scan_ident() in one buswidth configuration, and then it tries the other if it failed. You still aren't doing either of the options we talked about previously. I'll repeat them: Absolutely.. probably you missed my replies in [PATCH v9 4/9]... (1) You specify the buswidth given by device-tree/platform-data; if this is incorrect, you fail Absolutely this is what I'm doing. But tell me how would you know the actual device-width if nand_scan_ident() fails (a) to probe ONFI params and - your device is not in nand_ids[] So get the actual device width, I call the first nand_scan_ident() in x8 mode. so that ONFI params are read. Now, if it nand_scan_ident() fails then it means actual device *may* be x16 So, I re-invoke nand_scan_ident() with chip-option |= pdata-devsize; (2) You auto-configure using NAND_BUSWIDTH_AUTO, and you use that to validate the platform data; you just warn if the buswidth provided by device-tree/platform-data was incorrect I have removed NAND_BUSWIDTH_AUTO implementation, as per feedbacks This is new patch. May be you are confusing it with earlier version... *please re-review* Am I sensing that you are having some implementation problem with one of these? You really shouldn't need to run nand_scan_ident() twice. Or perhaps the solution in this patch is just that you're moving around the reassignment of the callback functions? If so, this is not obvious... see below. I'm repost my replies from [PATCH v9 4/9] in-case you missed... The GPMC driver will be touched by NAND_BUSWIDTH_AUTO change. There are two set of configurations in GPMC controller.. (a) device based configurations: These are static configurations derived on DT bindings for each chip-select (like NAND signal timings, etc). These done on GPMC probe. (b) ecc-scheme based configurations: These are dynamic configurations done in NAND driver in chip-ecc.hwctl() and are refreshed at each NAND accesss. However, 'nand-bus-width' configuration is part of both (1) and (2). Thus, both 'OMAP NAND driver' and 'GPMC driver' need to be consistent in programming 'nand-bus-width' otherwise ecc-engine would not work. Thus, I'm proceeding with (1) DT based parsing of 'nand-bus-width'. I have re-posted [PATCH v10 4/10] with this updates. However, please take into account that some limitation of dual programming require such probe. New patch also call nand_scan_ident() twice, but only on certain pre-determined conditions, not in all failure. Once I convert to NAND_BUSWIDTH_AUTO these should get clean, as I would update both GPMC and NAND driver for this. (Till then this was most optimal solution I could think of).. --- drivers/mtd/nand/omap2.c | 45 + 1 file changed, 33 insertions(+), 12 deletions(-) diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c index 5596368..d29edda 100644 --- a/drivers/mtd/nand/omap2.c +++ b/drivers/mtd/nand/omap2.c @@ -1856,7 +1856,6 @@ static int omap_nand_probe(struct platform_device *pdev) mtd-name = dev_name(pdev-dev); mtd-owner
RE: [PATCH v10 00/10] mtd:nand:omap2: clean-up of supported ECC schemes
Hi Brian, From: Brian Norris [mailto:computersforpe...@gmail.com] [PATCH 4/10] dropped [PATCH v9 4/9] introducing NAND_BUSWIDTH_AUTO, instead using DT 'nand-bus-width' for device bus-width As mentioned in reply to the patch, I don't think this patch belongs in this series now, and it is still not adequate (the original code there is wrong in some ways, but your changes are not clearly better). Probably there was some confusion in review this patch.. I have added my comments and replies to you. Request you to please re-review.. And as I suppose this is the only remaining piece in this series, so If that's ok, request you to please pull-in this series.. So, I can re-work on next pending series.. with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v9 4/9] mtd: nand: omap: enable auto-detection of bus-width for omap-nand drivers
Hi Brian, From: Brian Norris [mailto:computersforpe...@gmail.com] On Thu, Oct 17, 2013 at 09:00:27PM +, Pekon Gupta wrote: From: Brian Norris [mailto:computersforpe...@gmail.com] On Thu, Oct 17, 2013 at 04:42:23AM +, Pekon Gupta wrote: [...] But the real point: you need to clearly communicate what you are choosing in this patch. Either you want to (1) strictly follow the buswidth provided by the platform or (2) use the nand_base.c BUSWIDTH_AUTO automatic configuration. Ideally, I would like to go with (2), but that would need other changes in-order to re-configure GPMC with newly parsed ONFI data, which can be a separate patch-set. What exactly needs changed to support this? It looks like this omap2 NAND driver doesn't make assumptions about 8-bit vs. 16-bit until after nand_scan_ident(). But maybe there is something I missed. The GPMC driver will be touched by NAND_BUSWIDTH_AUTO change. There are two set of configurations in GPMC controller.. (a) device based configurations: These are static configurations derived on DT bindings for each chip-select (like NAND signal timings, etc). These done on GPMC probe. (b) ecc-scheme based configurations: These are dynamic configurations done in NAND driver in chip-ecc.hwctl() and are refreshed at each NAND accesss. However, 'nand-bus-width' configuration is part of both (1) and (2). Thus, both 'OMAP NAND driver' and 'GPMC driver' need to be consistent in programming 'nand-bus-width' otherwise ecc-engine would not work. Thus, I'm proceeding with (1) DT based parsing of 'nand-bus-width'. Thus, I would drop this patch for now. And go with (1), OK, but to drop this patch entirely would still not be (1); it would still leave the possibility of calling nand_scan_ident() twice, and it puts you in a middle ground between (1) and (2). That's fine if it works for you, but I just want to acknowledge that now: this driver requires changes to become either (1) or (2). I have re-posted [PATCH v10 4/10] with this updates. However, please take into account that some limitation of dual programming require such probe. New patch also call nand_scan_ident() twice, but only on certain pre-determined conditions, not in all failure. Once I convert to NAND_BUSWIDTH_AUTO these should get clean, as I would update both GPMC and NAND driver for this. (Till then this was most optimal solution I could think of).. Does your series need a rebase if we're removing this patch? Or you're rewriting/simplifying it to the following two points? (Perhaps it's best to separate this portion to its own patch (set) and discussion, since it is independent of your ECC rewrite.) with following updates in omap_nand_probe(). (a) perform nand_scan_ident() in x8 mode, so that ONFI params are read and device-info (chip-writesize, chip-oobsize) are populated. OK. (b) Then switch to x8 or x16 mode based on nand-bus-width as passed from GPMC driver to NAND driver via platform-data. And re-populate mtd-byte, mtd-read_buf, mtd-write_buf. I'm not sure if switching buswidth after nand_scan_ident() is really supported, but I'll hold off until I see the code you're proposing. I have re-posted [PATCH v10 4/10] with this updates. Please accept this ... with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v9 1/9] mtd: nand: omap: combine different flavours of 1-bit hamming ecc schemes
Hi Mark,
From: Mark Rutland
[...]
diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-
omap2/gpmc.c
index 579697a..c9fb353 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -1342,9 +1342,7 @@ static void __maybe_unused
gpmc_read_timings_dt(struct device_node *np,
#ifdef CONFIG_MTD_NAND
static const char * const nand_ecc_opts[] = {
- [OMAP_ECC_HAMMING_CODE_DEFAULT] = sw,
- [OMAP_ECC_HAMMING_CODE_HW] = hw,
- [OMAP_ECC_HAMMING_CODE_HW_ROMCODE] = hw-
romcode,
+ [OMAP_ECC_HAM1_CODE_HW] = ham1,
[OMAP_ECC_BCH4_CODE_HW] = bch4,
[OMAP_ECC_BCH8_CODE_HW] = bch8,
};
As the parsing isn't updated until the next patch, doesn't this
temporarily break DTBs with the deprecated ti,nand-ecc-opt values?
Ok then I'll swap the [Patch 1/9] and [Patch 2/9] so that DT parsing updates
(with backward compatibility) happen before the deprecation of DT values.
This way old DT binding would work all through.
And as DT parsing is kept backward compatible, so .dts changes would not
break the DTBs even if updated in separate patch.
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v9 3/9] mtd: nand: omap: cleanup: replace local references with generic framework names
From: Brian Norris [mailto:computersforpe...@gmail.com] On Tue, Oct 15, 2013 at 11:19:51AM +0530, Pekon Gupta wrote: This patch updates following in omap_nand_probe() and omap_nand_remove() - replaces info-nand with nand_chip (struct nand_chip *nand_chip) - replaces info-mtd with mtd (struct mtd_info *mtd) - white-space and formatting cleanup Signed-off-by: Pekon Gupta pe...@ti.com This patch looks good. Thanks for being consistent. diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c index 8d521aa..5596368 100644 --- a/drivers/mtd/nand/omap2.c +++ b/drivers/mtd/nand/omap2.c ... @@ -1846,17 +1848,16 @@ static int omap_nand_probe(struct - - info-nand.options = pdata-devsize; - info-nand.options |= NAND_SKIP_BBTSCAN; + mtd = info-mtd; + mtd-priv = info-nand; + mtd-name = dev_name(pdev-dev); + mtd-owner = THIS_MODULE; + nand_chip = info-nand; + nand_chip-options = pdata-devsize; Trivial side comment: the 'devsize' field is not named very informatively. It is apparently just a way to specify nand_chip.options, and it is only actually used for setting a buswidth. (It is also badly named nand_type in other places.) Not sure if it's worth improving, or even dropping at some later point in time. For now I'm keeping this name consistent for now, as this would require touching GPMC driver also, where 'devsize' is used to configure GPMC controller registers. I'll keep this feedback as pending for independent patch where I probe driver with NAND_BUSWIDTH_AUTO. with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v9 7/9] mtd: nand: omap: use drivers/mtd/nand/nand_bch.c wrapper for BCH ECC instead of lib/bch.c
From: Brian Norris [mailto:computersforpe...@gmail.com]
On Tue, Oct 15, 2013 at 11:19:55AM +0530, Pekon Gupta wrote:
[snip]
diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index d885298..5836039 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -96,35 +96,13 @@ config MTD_NAND_OMAP2
config MTD_NAND_OMAP_BCH
depends on MTD_NAND MTD_NAND_OMAP2 ARCH_OMAP3
- tristate Enable support for hardware BCH error correction
+ tristate Support hardware based BCH error correction
default n
select BCH
- select BCH_CONST_PARAMS
Do you know what will happen now if someone configures
BCH_CONST_PARAMS?
Would this cause problems?
As per comments in lib/bch.c
---
* Option CONFIG_BCH_CONST_PARAMS can be used to force fixed values of
* parameters m and t; thus allowing extra compiler optimizations and providing
* better (up to 2x) encoding performance. Using this option makes sense when
* (m,t) are fixed and known in advance, e.g. when using BCH error correction
* on a particular NAND flash device.
---
'BCH_CONST_PARAMS' is required for optimization when BCH algorithm
is fixed. But in omap-nand case selection of type of BCH algorithm
(BCH4 or BCH8) comes from DT binding ti,nand-ecc-opts.
If enabled (CONFIG_BCH_CONST_PARAMS) will optimize lib/bch.c
for BCH8 algorithm by default, so
CASE: if BCH8 is selected by DT, then no issues
CASE: if BCH4 is selected then nand_bch_init() fails with following error
+ if (!info-nand.ecc.priv) {
pr_err(nand: error: unable to use s/w BCH library\n);
err = -EINVAL;
goto out_release_mem_region;
}
[snip]
if MTD_NAND_OMAP_BCH
config BCH_CONST_M
Do you need to to also kill of the Kconfig stuff for BCH_CONST_M and
BCH_CONST_T, which were tied to the MTD_NAND_OMAP_BCH4 and
MTD_NAND_OMAP_BCH8 configs you just removed?
Thanks, good catch. I dint really notice.
So, the driver is now updated to separate out two flavours of BCHx scheme.
(a) OMAP_ECC_BCHx_CODE_HW: which uses ELM hardware
(b) OMAP_ECC_BCHx_CODE_HW_DETECTION_SW: which uses lib/bch.c
These BCH_CONST_M and BCH_CONST_T now belongs to (b) family only.
- if MTD_NAND_OMAP_BCH
+ if MTD_NAND_ECC_BCH
config BCH_CONST_M
(I'll update that)..
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
[snip]
- info-bch = init_bch(nand_chip-ecc.bytes,
- nand_chip-ecc.strength,
- OMAP_ECC_BCH8_POLYNOMIAL);
- if (!info-bch) {
+ info-nand.ecc.priv = nand_bch_init(mtd,
+ info-nand.ecc.size,
+ info-nand.ecc.bytes,
+ info-nand.ecc.layout);
Are you sure nand_bch_init() is a proper drop-in replacement for the
implementation you had based on init_bch()? It looks to me like they at
least use a differnt polynomial value (0x201b vs. 0). Is this a problem
for backwards compatibility?
It's not the polynomial value = 0. Rather 0x201b is selected in both cases
Refer below code.
---
When init_bch(m,t, 0) is called from nand_bch_init() then,
lib/bch.c @@ init_bch(int m, int t, unsigned int prim_poly)
(a) /* default primitive polynomials */
static const unsigned int prim_poly_tab[] = {
0x25, 0x43, 0x83, 0x11d, 0x211, 0x409, 0x805, 0x1053, 0x201b,
0x402b, 0x8003,
};
(b) /* select a primitive polynomial for generating GF(2^m) */
if (prim_poly == 0)
prim_poly = prim_poly_tab[m-min_m];
(c) And, const int min_m = 5;
So, for BCH8 m=13, min_m=5; So
prim_poly = prim_poly_tab[13-5] = prim_poly_tab[8] = 0x201b
---
Hence, there is no change in polynomial, it's just that instead of
hard-coding the value, polynomial selection depends on 'm' and 't'.
[...]
A related question: do we have any current users of this driver that can
provide testing results for this series? Or is this work just tested
with new hardware?
Got a tested-by: jp.franc...@cynove.com for BCH4
But that was in May,2013 :-)
http://lists.infradead.org/pipermail/linux-mtd/2013-May/047065.html
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v9 4/9] mtd: nand: omap: enable auto-detection of bus-width for omap-nand drivers
Hi Brain, From: Brian Norris [mailto:computersforpe...@gmail.com] On Thu, Oct 17, 2013 at 04:42:23AM +, Pekon Gupta wrote: From: Brian Norris [mailto:computersforpe...@gmail.com] On Tue, Oct 15, 2013 at 11:19:52AM +0530, Pekon Gupta wrote: [snip] So this approach is other way round, where controller is configured based on DT binding nand-bus-width and then it checks whether device actual buswidth matches DT binding value or not. If this is the case, then you really don't want to use NAND_BUSWIDTH_AUTO. The default nand_base.c just *validates* the pre-selected buswidth and exits with an error, in much the same way that you're doing here (you're just duplicating the functionality). NAND_BUSWIDTH_AUTO is useful if you want to turn control of the buswidth configuration entirely over to nand_base.c. - GPMC controller is pre-configured to support x8 or x16 device based on DT binding nand-bus-width. Reference: $LINUX/arch/arm/mach-omap2/gpmc.c @@gpmc_probe_nand_child() val = of_get_nand_bus_width(child); - But, bus-width of NAND device is only known during NAND driver Probe in nand_scan_ident(). Going forward when all NAND devices are compliant to ONFI, then we can deprecate nand-bus-width. Buswidth auto-detection is not actually restricted to ONFI. nand_base.c has (corretly, AFAIK) been detecting buswidths for a long time, using some form of ID decode detection. But it was just that: detection. It did not actually configure the buswidth, since it left that responsibility to the low-level driver to get right. Another thing to consider: I think this current patch is a regression from previous behavior. Previously, you would run nand_scan_ident() twice if the first one failed; once with the platform-configured buswidth and once with the opposite. But now, you only run nand_scan_ident() once, and then you quit if it detects differently than you expected. Actually having nand_scan_ident() called again if first time it fails, is _not_ the right way, it was a quick-fix work-around. It should not have been approved.. My opinion: the platform- and device-tree-provided buswidth is unnecessary; it was previouisly only a suggestion which your driver would readily discard, and it isn't really needed now. You can probably get by with just NAND_BUSWIDTH_AUTO and (at most) a warning if the auto-configured buswidth is different than the platform specified. But the real point: you need to clearly communicate what you are choosing in this patch. Either you want to (1) strictly follow the buswidth provided by the platform or (2) use the nand_base.c BUSWIDTH_AUTO automatic configuration. Ideally, I would like to go with (2), but that would need other changes in-order to re-configure GPMC with newly parsed ONFI data, which can be a separate patch-set. Thus, I would drop this patch for now. And go with (1), with following updates in omap_nand_probe(). (a) perform nand_scan_ident() in x8 mode, so that ONFI params are read and device-info (chip-writesize, chip-oobsize) are populated. (b) Then switch to x8 or x16 mode based on nand-bus-width as passed from GPMC driver to NAND driver via platform-data. And re-populate mtd-byte, mtd-read_buf, mtd-write_buf. with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v9 4/9] mtd: nand: omap: enable auto-detection of bus-width for omap-nand drivers
Hi Brain, Oop sorry .. s/Brain/Brian synonymous though :-) with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v9 0/9] mtd:nand:omap2: clean-up of supported ECC schemes
Hi Brian, *changes v8 - v9* [PATCH 1/9] no update from [PATCH v8 1/6] [PATCH 2/9] only commit log updated from [PATCH v8 2/6] As per feedbacks from Brian Norris computersforpe...@gmail.com previous revision [PATCH v8 3/6] and [PATCH 4/6] are split into following sub-patches: - [PATCH 3/9] new replaces local reference with generic names (mtd, nand_chip) - [PATCH 4/9] new enables auto-detection of bus-width - [PATCH 5/9] new removes omap3_init_bch: populates ecc-scheme data - [PATCH 6/9] new removes omap3_init_bch_tail: populates ecc-layout - [PATCH 7/9] new replaces lib/bch.c with nand_bch.c wrapper [PATCH 8/9] no update same as [PATCH v8 5/6] [PATCH 9/9] removed devm_free_xx functions Request you to have a look at v9. If this is acceptable, then I would work to rebase the other pending series on top of this one. with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v9 4/9] mtd: nand: omap: enable auto-detection of bus-width for omap-nand drivers
Hi Brian,
From: Brian Norris [mailto:computersforpe...@gmail.com]
On Tue, Oct 15, 2013 at 11:19:52AM +0530, Pekon Gupta wrote:
Autodetection of NAND device bus-width was added in generic NAND
driver as
[...]
@@ -1904,6 +1903,21 @@ static int omap_nand_probe(struct
platform_device *pdev)
nand_chip-chip_delay = 50;
}
+ /* scan for NAND device connected to chip controller */
+ if (nand_scan_ident(mtd, 1, NULL)) {
+ err = -ENXIO;
+ goto out_release_mem_region;
+ }
+ if ((nand_chip-options NAND_BUSWIDTH_16) !=
+ (pdata-devsize NAND_BUSWIDTH_16)) {
+ pr_err(%s: detected %s device but driver configured for
%s\n,
+ DRIVER_NAME,
+ (nand_chip-options NAND_BUSWIDTH_16) ?
x16 : x8,
+ (pdata-devsize NAND_BUSWIDTH_16) ? x16 :
x8);
I'm not sure on the policy choice here; if you're using BUSWIDTH_AUTO,
do you even want to try to configure the buswidth by platform data?
You're not really getting the full use out of NAND_BUSWIDTH_AUTO because
the platform data has to guess the same buswidth that nand_base.c
determines. This is worse than the previous behavior, in which you would
try both buswidths if the specified type failed.
IOW, what are you trying to achieve with auto-buswidth? Automatic
determination of the buswidth or just automatic validation? What do you
achieve with platform data's selection of buswidth? Specification of the
buswidth or just a hint? Do the goals conflict? Perhaps you can just
warn, and not error out if the two selections don't match? Or maybe you
really wanted to replace the platform data selection mechanism entirely
with auto-buswidth?
This is 'automatic validation' of value set in DT binding nand-bus-width
So this approach is other way round, where controller is configured
based on DT binding nand-bus-width and then it checks whether
device actual buswidth matches DT binding value or not.
- GPMC controller is pre-configured to support x8 or x16 device based
on DT binding nand-bus-width.
Reference: $LINUX/arch/arm/mach-omap2/gpmc.c
@@gpmc_probe_nand_child()
val = of_get_nand_bus_width(child);
- But, bus-width of NAND device is only known during NAND driver
Probe in nand_scan_ident().
Going forward when all NAND devices are compliant to ONFI,
then we can deprecate nand-bus-width.
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v8 0/6] mtd:nand:omap2: clean-up of supported ECC schemes
Hi Brain,
Hi Pekon,
I will try to summarize the standing of your patch series.
Patches 1 and 2 look good and have addressed all of the DT maintainers'
comments, AFAICT. They are ready to go in, except that the following
patches are not ready; they should probably go in together.
You ignored most of my comments to patch 3, and it is insufficiently
documented. Please take a look at my comments, both here (in v8) and in
v6. It still should be split into more patches.
I tried splitting the earlier [PATCH 3/6], therefore a new patch for merging
various Hamming ECC schemes was spawned. But, I could not clean more
because I would have broken the NAND functionality in between the
patches.
My apologies, I missed some of you other comments, so I'm preparing
next version by splitting [PATCH 3/6] into more sub-patches for ease of
review.
Most #ifdef were put to suppress warning of un-used functions during
compile time. So those cannot be removed, otherwise this patch would
give compile warnings under randconfig.
Patch 4 does too much without describing it. Why are you dropping the
old omap3_correct_data_bch() code? Is this just refactoring? If so, you
should say so. And this also suggests that you have two logical changes
going on that should be separated into different patches; one to
refactor the open-coded NAND/BCH library to replace it with the
nand_bch.{c,h} support library and one for the new ECC modes.
Agreed, I update commit log to be more explicit that here nand_bch.c
actually encapsulates lib/bch.c.
Here also I cannot remove #ifdef across nand_bch_free() because
it frees some bch control data, which would not be defined for all
ecc-schemes (it is specific to xx_SW ecc schemes). Hence removing
#ifdef here would give null-pointer exception.
Patch 5 is good but should wait until the other DT parts are acceptable
and merged into MTD.
Patch 6 needs rewriting to use devm_* functions properly, but it was
never integral to this patch series. You can improve it and resend with
this series or just send it separately afterward.
Yes I understood this, but I think it would be still good to explicitly
free the resources in case of module_remove(), because that would
free all resources instantaneously without waiting for devm_ to
iterate thru the list when it wakes-up (I guess).
I'm not knowledgeable of exact implementation of devm_ and how
often does it iterates the list and frees the resources for corresponding
un-registered devices. So trusting you I'll include your feedbacks here.
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v8 3/6] mtd:nand:omap2: clean-up BCHx_HW and BCHx_SW ECC configurations in device_probe
Hi Brian,
Thanks for such detailed review, please see some replies below..
From: Brian Norris [mailto:computersforpe...@gmail.com]
On Fri, Oct 11, 2013 at 07:06:40PM +0530, Pekon Gupta wrote:
[...]
Why do you even need the #ifdef's for the #include's? It is not harmful
to include headers for stuff that is only conditionally used. In fact,
this creates a problem later when you try to use nand_bch_free(), and
you have to surround it with extra #ifdef's.
In short: these #ifdef's just breed more #ifdef's and cause the code to
become harder to read and less maintainable.
There are 2 problems here:
(1)
I have removed #ifdef across headers in next version. But I cannot remove
#ifdef across some callbacks for cc.hwctl(), ecc.calculate(), ecc.correct(),
because then compilation would throw warnings for un-used functions.
(2)
OMAP driver uses generic lib/bch.c which is compiled only when
CONFIG_MTD_NAND_ECC_BCH is enabled. So to avoid compilation
issues, I had to put #ifdefs on all wrapper functions which use lib/bch.c
or nand_bch.c
I myself have tried in previous versions to avoid #ifdefs, but I ended
up in one or the other problem like (1) | (2) above.
And this is where randconfig test failed earlier when Arnd Bergmann
commented, so some #ifdef would hv to be carried till be deprecate
some legacy ecc-schemes.
However, with patch split many redundant #ifdefs are now removed.
(I commented about the #ifdef's around nand_bch_free() in v6, and you
did not address the comment.)
Done now.. My bad again, I somehow mis-interpreted nand_bch.c earlier.
@@ -1846,20 +1726,18 @@ static int omap_nand_probe(struct
- info-nand.options |= NAND_SKIP_BBTSCAN;
-#ifdef CONFIG_MTD_NAND_OMAP_BCH
+ nand_chip-options |= NAND_SKIP_BBTSCAN |
NAND_BUSWIDTH_AUTO;
I recommended (in v6) you split the NAND_BUSWIDTH_AUTO change to a
new
patch and to describe it in the commit. You did neither.
Sorry missed this purposely because I could not separate out the changes.
But now I have re-worked this in next revision as a separate patch.
+ /* scan NAND device conncted to controller */
+ if (nand_scan_ident(mtd, 1, NULL)) {
+ err = -ENXIO;
+ goto out_release_mem_region;
+ }
+ pr_info(%s: detected %s NAND flash\n, DRIVER_NAME,
+ (nand_chip-options NAND_BUSWIDTH_16) ?
x16 : x8);
I recommended you kill this in v6, and you did not address my comments.
Sorry, this was my bad.
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v7 1/6] mtd: nand: omap: combine different flavours of 1-bit hamming ecc schemes
Dear MTD Maintainers,
If I have my NAND formatted with one of the existing ECC schemes (e.g.
OMAP_ECC_HAMMING_CODE_DEFAULT) will it work with the new
OMAP_ECC_HAM1_CODE_HW scheme?
Are they all compatible?
Yes, they all are 1-bit hamming code, the only difference between
xx_Default and xx_HW was who was doing the ECC calculation.
For xx_DEFAULT: ECC calculation was done on CPU via s/w library
For xx_HW: ECC calculation was done by in-build h/w engine.
So, all HAMMING_xx can be replaced by HAM1_HW.
[snip]
@@ -1342,9 +1342,7 @@ static void __maybe_unused
gpmc_read_timings_dt(struct device_node *np,
#ifdef CONFIG_MTD_NAND
static const char * const nand_ecc_opts[] = {
- [OMAP_ECC_HAMMING_CODE_DEFAULT] = sw,
- [OMAP_ECC_HAMMING_CODE_HW] = hw,
- [OMAP_ECC_HAMMING_CODE_HW_ROMCODE] = hw-
romcode,
+ [OMAP_ECC_HAM1_CODE_HW] = ham1,
[OMAP_ECC_BCH4_CODE_HW] = bch4,
[OMAP_ECC_BCH8_CODE_HW] = bch8,
Won't this break existing dts which have sw, hw, or hw-romcode?
Someone may try to use a new kernel with an old dt, and we marked them
as deprecated, not removed.
HAMMING_xx ECC scheme was used only on legacy platforms, when
BCH8 was not available, I have not seen anyone using this scheme
*from mainline kernel* from quite a long time. So, it's safe to remove them.
This is what was concluded as per below email.
http://lists.infradead.org/pipermail/linux-mtd/2013-September/048876.html
This patch-series and its follow-on series has already missed many merge
windows, And the above discussion has reached a stalled state (infinite loop)
where, I cannot revert some DT binding updates to and fro to keep all legacy
DT bindings backward compatible forever.
However, I can assure that these DT updates make binding stable for long-term.
So now it's your discretion to whether to accept or leave following 2 series:
http://lists.infradead.org/pipermail/linux-mtd/2013-October/048983.html
http://lists.infradead.org/pipermail/linux-mtd/2013-October/049008.html
AFAIK no-one is using Hamming based ecc-scheme on OMAP platforms
*from mainline kernel*. So this DT update actually does not affect users
I know of. Rather these patch series was intended for long term scalability
and clean-up so that more OMAP users migrate to mainline kernel easily.
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v7 1/6] mtd: nand: omap: combine different flavours of 1-bit hamming ecc schemes
Hi,
From: Mark Rutland [mailto:mark.rutl...@arm.com]
On Fri, Oct 04, 2013 at 08:49:43PM +0100, Pekon Gupta wrote:
OMAP NAND driver currently supports multiple flavours of 1-bit Hamming
ecc-scheme, like:
- OMAP_ECC_HAMMING_CODE_DEFAULT
1-bit hamming ecc code using software library
- OMAP_ECC_HAMMING_CODE_HW
1-bit hamming ecc-code using GPMC h/w engine
- OMAP_ECC_HAMMING_CODE_HW_ROMCODE
1-bit hamming ecc-code using GPMC h/w engin with ecc-layout
compatible
to ROM code.
This patch combines above multiple ecc-schemes into single
implementation:
- OMAP_ECC_HAM1_CODE_HW
1-bit hamming ecc-code using GPMC h/w engine with ROM-code
compatible
ecc-layout.
If I have my NAND formatted with one of the existing ECC schemes (e.g.
OMAP_ECC_HAMMING_CODE_DEFAULT) will it work with the new
OMAP_ECC_HAM1_CODE_HW scheme?
Are they all compatible?
Yes, they all are 1-bit hamming code, the only difference between
xx_Default and xx_HW was who was doing the ECC calculation.
For xx_DEFAULT: ECC calculation was done on CPU via s/w library
For xx_HW: ECC calculation was done by in-build h/w engine.
So, all HAMMING_xx can be replaced by HAM1_HW.
[snip]
@@ -1342,9 +1342,7 @@ static void __maybe_unused
gpmc_read_timings_dt(struct device_node *np,
#ifdef CONFIG_MTD_NAND
static const char * const nand_ecc_opts[] = {
- [OMAP_ECC_HAMMING_CODE_DEFAULT] = sw,
- [OMAP_ECC_HAMMING_CODE_HW] = hw,
- [OMAP_ECC_HAMMING_CODE_HW_ROMCODE] = hw-
romcode,
+ [OMAP_ECC_HAM1_CODE_HW] = ham1,
[OMAP_ECC_BCH4_CODE_HW] = bch4,
[OMAP_ECC_BCH8_CODE_HW] = bch8,
Won't this break existing dts which have sw, hw, or hw-romcode?
Someone may try to use a new kernel with an old dt, and we marked them
as deprecated, not removed.
HAMMING_xx ECC scheme was used only on legacy platforms, when
BCH8 was not available, I have not seen anyone using this scheme
*from mainline kernel* from quite a long time. So, it's safe to remove them.
This is what was concluded as per below email.
http://lists.infradead.org/pipermail/linux-mtd/2013-September/048876.html
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v7 5/6] ARM: dts: AM33xx: updated default ECC scheme in nand-ecc-opt
From: Mark Rutland [mailto:mark.rutl...@arm.com] On Fri, Oct 04, 2013 at 08:49:47PM +0100, Pekon Gupta wrote: DT property values for OMAP based gpmc-nand have been updated to match changes in commit: 6faf096 ARM: OMAP2+: cleaned-up DT support of various ECC schemes Refer: Documentation/devicetree/bindings/mtd/gpmc-nand.txt Sorry that's an old commit. I probably missed out updating the commit log. So please ignore it here, I should remove that from commit log. Doesn't this mean these dts were broken between the changes to the driver and the changes to the dts? But the DTS updates are kept separate from driver updates because - DTS updates go into Benoit's tree or Tony's tree - whereas driver updates go into MTD tree. So, it was suggested in earlier patch revisions that I keep the driver and the DTS patches separate, so that there is no merge conflict between the trees when they are finally accepted. I think the existing properties need to continue to be supoprted for backwards compatibility. The dts can be fixed up to have the preferred binding style, but they shouldn't _have_ to be modified to continue to function... However, there should be a way to remove legacy code which can no longer be maintained. Here following ecc-schemes are such legacy code, which are neither supported nor encouraged to use since long time. - OMAP_ECC_CODE_HAMMING_HW, - OMAP_ECC_CODE_HAMMING_DEFAULT, - OMAP_ECC_CODE_HAMMING_HW_ROMCODE Also backward compatibility is maintained by keeping a common symbol for all 3 above implementations OMAP_ECC_CODE_HAM1_HW I don't think there is a point to continue keeping these symbols in kernel driver if they are unused, and make driver bulky and unreadable. In actual it shouldn't matter to user what symbols I use inside the driver, what matters is backward compatibility right ? with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v7 2/6] ARM: OMAP2+: cleaned-up DT support of various ECC schemes
From: Mark Rutland [mailto:mark.rutl...@arm.com]
On Fri, Oct 04, 2013 at 08:49:44PM +0100, Pekon Gupta wrote:
[snip]
diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-
omap2/gpmc.c
index 1c45b72..5a607fa 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -1341,12 +1341,6 @@ static void __maybe_unused
gpmc_read_timings_dt(struct device_node *np,
#ifdef CONFIG_MTD_NAND
-static const char * const nand_ecc_opts[] = {
- [OMAP_ECC_HAM1_CODE_HW] = ham1,
- [OMAP_ECC_BCH4_CODE_HW] = bch4,
- [OMAP_ECC_BCH8_CODE_HW] = bch8,
-};
-
static const char * const nand_xfer_types[] = {
[NAND_OMAP_PREFETCH_POLLED] = prefetch-polled,
[NAND_OMAP_POLLED] = polled,
@@ -1361,6 +1355,8 @@ static int gpmc_probe_nand_child(struct
platform_device *pdev,
const char *s;
struct gpmc_timings gpmc_t;
struct omap_nand_platform_data *gpmc_nand_data;
+ const __be32 *phandle;
I don't think you need this. With of_parse_phandle you should never need
to see the raw phandle value.
+ int lenp;
if (of_property_read_u32(child, reg, val) 0) {
dev_err(pdev-dev, %s has no 'reg' property\n,
@@ -1376,12 +1372,39 @@ static int gpmc_probe_nand_child(struct
platform_device *pdev,
gpmc_nand_data-cs = val;
gpmc_nand_data-of_node = child;
- if (!of_property_read_string(child, ti,nand-ecc-opt, s))
- for (val = 0; val ARRAY_SIZE(nand_ecc_opts); val++)
- if (!strcasecmp(s, nand_ecc_opts[val])) {
- gpmc_nand_data-ecc_opt = val;
- break;
- }
+ /* Detect availability of ELM module */
+ phandle = of_get_property(child, ti,elm-id, lenp);
+ if ((phandle == NULL) || (lenp != sizeof(void *))) {
+ pr_warn(%s: ti,elm-id property not found\n, __func__);
+ gpmc_nand_data-elm_of_node = NULL;
+ } else {
+ gpmc_nand_data-elm_of_node =
+
of_find_node_by_phandle(be32_to_cpup(phandle));
+ }
Use of_parse_handle rather than open-coding it:
gpmc_nand_data-elm_of_node =
of_parse_phandle(child, ti,elm-id, 0);
Was elm_id ever handled previously? I don't see any code handling it
being remove or modified.
Yes, this piece of code has been moved from omap2.c (nand-driver)
to gpmc.c (GPMC driver), so as to perform all DT related parsing
at single place.
This change was needed as per feedbacks from Olof to simply
DT bindings so that they include only H/W related stuff.
Please refer omap2.c: omap3_init_bch() in [PATCH 3/6] of same series
where 'elm_id' DT parsing was original present.
- /* Detect availability of ELM module */
- parp = of_get_property(info-of_node, elm_id, lenp);
- if ((parp == NULL) (lenp != (sizeof(void *) * 2))) {
- pr_err(Missing elm_id property, fall back to Software BCH\n);
- info-is_elm_used = false;
- } else {
- struct platform_device *pdev;
-
- elm_node = of_find_node_by_phandle(be32_to_cpup(parp));
- pdev = of_find_device_by_node(elm_node);
- info-elm_dev = pdev-dev;
-
- if (elm_config(info-elm_dev, bch_type) == 0)
- info-is_elm_used = true;
with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes
Hi All, So, based on feedbacks from everyone, I could come to following conclusions. Please confirm, if those are acceptable ? From: Mark Rutland [mailto:mark.rutl...@arm.com] On Thu, Sep 26, 2013 at 11:54:39AM +0100, Gupta, Pekon wrote: From: Rob Herring [mailto:robherri...@gmail.com] From: Pekon Gupta [mailto:pe...@ti.com] diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt index df338cb..958e5d5 100644 --- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt @@ -21,11 +21,8 @@ Optional properties: is wired that way. If not specified, a bus width of 8 is assumed. - - ti,nand-ecc-opt:A string setting the ECC layout to use. One of: - - swSoftware method (default) - hwHardware method - hw-romcodegpmc hamming mode method romcode layout + - ti,nand-ecc-scheme: A string setting the ECC layout to use. One of: + ham1 1-bit Hamming ecc code As has been pointed out, this breaks compatibility which should be avoided unless you know the state of use of this binding. I fail to see the advantage of using scheme over opt. You could simply add ham1 here and maintain compatibility. Instead of removing sw, hw, etc. you can simply deprecate them or decide that the kernel doesn't support those options. Renaming 'ti,nand-ecc-opt' to 'ti,nand-ecc-scheme' was as per earlier comments from Olof. So either way is fine with me. Should I revert it back to 'ti,nand-ecc-opt' ? I think that if the binding is already in use then we shouldn't break it, unless you're _definitely_ the only user. Agreed, would revert back to 'ti,nand-ecc-scheme' Also, sw, hw_romcode have been deprecated, they are no longer supported in driver. So shouldn't they be removed from the documentation ? Deprecated properties should be marked as deprecated, but continue to be documented. That at least prevents the names being repurposed in an incompatible way, and explains to anyone who looks at the document that the proeprty is deprecated rather than simply undocumented. Agreed. - keep existing values in documentation (sw, hw, hw_romcode) but mark them as deprecated. - add new values (ham1, bch4, bch8,..) However, since you are willing to break compatibility, then you should the generic NAND binding and use nand-ecc-mode adding the values you need. Documentation/devicetree/bindings/mtd/nand.txt: * MTD generic binding - nand-ecc-mode : String, operation mode of the NAND ecc mode. Supported values are: none, soft, hw, hw_syndrome, hw_oob_first, soft_bch. Yes I can use generic 'nand-ecc-mode', But the binding values like soft, hw, soft_bch are too generic to describe the hardware. - BCH ECC scheme can itself be of multiple types, bch4/bch8/bch16 so soft_bch is ambiguous. It does indeed seem like the generic properties are not generic enough, at least from my quick look other them. However, I am not familiar with NAND, so I may have misunderstood. Would not use generic 'nand-ecc-mode' property, instead continue with 'ti,nand-ecc-opt'. - Similarly soft and hw do not determine the algorithm used like Hamming or BCH. (a) Should I update the generic binding values (if no one else is using) ? like sw - ham1_sw hw - ham1_hw soft_bch- soft_bch4, soft_bch8 What do the current property names actually correspond to logically? If we did this and there are existing users, the old DTs need to continue functioning. OR (b) Should I add newer ones to it (like ham1, bch4, bch8, bch16) ? keeping the old ones intact. And how should I handle un-supported values, (Is pr_err during kernel probe enough ?). I think something like this, but with the names from (a) would be preferrable. As Brian pointed, implementation of ecc-scheme can be very different from vendor to vendor, and even SoC to SoC within same vendor, Thus its difficult to generalize as common DT binding for everyone. - Even if we try to do, there would be too many values, out of which only selectable would be applicable for a given SoC. - And some platforms might need extra DT information to get driver working, because h/w was designed that way. So it would be mess. So, its better not to have a generic ecc-scheme binding, instead let every vendor define it specifically. So for TI OMAP NAND driver, I'm continuing to use 'ti,nand-ecc-opt' as described above. Is this acceptable ? [...] - - elm_id: Specifies elm device node. This is required to support BCH
