linux-kernel - RE: [RFC PATCH 00/10] OMAP: GPMC: NAND: Introduce GPMC APIs for OMAP NAND

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20980858CB6D3A4BAE95CA194937D5E73EB01DD5@DBDE04.ent.ti.com>
Date:	Fri, 11 Jul 2014 07:27:19 +0000
From:	"Gupta, Pekon" <pekon@...com>
To:	Tony Lindgren <tony@...mide.com>, "Quadros, Roger" <rogerq@...com>
CC:	"computersforpeace@...il.com" <computersforpeace@...il.com>,
	"javier@...hile0.org" <javier@...hile0.org>,
	"ezequiel.garcia@...e-electrons.com" 
	<ezequiel.garcia@...e-electrons.com>,
	"dwmw2@...radead.org" <dwmw2@...radead.org>,
	"jg1.han@...sung.com" <jg1.han@...sung.com>,
	"Nori, Sekhar" <nsekhar@...com>,
	"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
	"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [RFC PATCH 00/10] OMAP: GPMC: NAND: Introduce GPMC APIs for
 OMAP NAND

Hi Roger,

>From: Tony Lindgren [mailto:tony@...mide.com]
>>* Roger Quadros <rogerq@...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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/