Re: [RFC PATCH 00/10] OMAP: GPMC: NAND: Introduce GPMC APIs for OMAP NAND
* Roger Quadros rog...@ti.com [140711 03:25]: On 07/11/2014 12:42 PM, Gupta, Pekon wrote: 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. Well I you have still not pointed out what is the benefit of that approach or what are the problems with the approach I am suggesting. I am suggesting that the API just return the ECC/BCH result registers as it is. You are suggesting that the API do the calculations and return the NAND ready ECC buffer. To do the calculations it needs to know about the ECC scheme which is NAND controller specific and this is unnecessary in the GPMC driver. Tony, what is your opinion? As long as the GPMC registers are exposed to drivers in a controlled way and we have only one driver using the ECC registers, I don't care. 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 00/10] OMAP: GPMC: NAND: Introduce GPMC APIs for OMAP NAND
* 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); void omap_gpmc_ecc_get_result(int length, u32 *result); void omap_gpmc_ecc_get_bch_result(int length, u8 sector, u32 *result); 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 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 00/10] OMAP: GPMC: NAND: Introduce GPMC APIs for OMAP NAND
Hi Pekon, On 07/11/2014 10:27 AM, Gupta, Pekon wrote: 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? 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(). 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. Could you please explain your view point as to why you want me to move these ecc.*() to GPMC driver other than being extensively tested? We can extensively test the new changes before merging any code to mainline as well. 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: [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 00/10] OMAP: GPMC: NAND: Introduce GPMC APIs for OMAP NAND
On 07/11/2014 12:42 PM, Gupta, Pekon wrote: 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. Right. so hwctl(), calculate() and correct() go hand in hand and must be implemented by the NAND controller driver. 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 Moreover, that is TI's legacy stuff coming right from OMAP2/3 days. 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
[RFC PATCH 00/10] OMAP: GPMC: NAND: Introduce GPMC APIs for OMAP NAND
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); void omap_gpmc_ecc_get_result(int length, u32 *result); void omap_gpmc_ecc_get_bch_result(int length, u8 sector, u32 *result); 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. cheers, -roger --- Roger Quadros (10): mtd: nand: omap: Use a single hardware controller instance mtd: nand: omap: Always use chip-ecc.steps for BCH sector count OMAP: GPMC: Introduce APIs to access NAND control registers mtd: nand: omap: Use GPMC APIs for NAND control OMAP: GPMC: Introduce APIs for accessing Prefetch/Write-post engine mtd: nand: omap: Use GPMC APIs for accessing Prefetch engine OMAP: GPMC: Introduce APIs for Configuring ECC Engine OMAP: GPMC: Introduce APIs to get ECC/BCH results mtd: nand: omap: Use GPMC APIs for accessing ECC/BCH engine OMAP: GPMC: NAND: Don't pass NAND/ECC/BCH register adresses to NAND driver arch/arm/mach-omap2/gpmc-nand.c | 2 - arch/arm/mach-omap2/gpmc.c | 464 +++ arch/arm/mach-omap2/gpmc.h | 1 - drivers/mtd/nand/omap2.c | 397 +-- include/linux/omap-gpmc-nand.h | 106 ++ include/linux/platform_data/mtd-nand-omap2.h | 5 +- 6 files changed, 663 insertions(+), 312 deletions(-) create mode 100644 include/linux/omap-gpmc-nand.h -- 1.8.3.2 -- 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
