[PATCH] MTD: pxa3xx_nand: enable multiple chip select support

[adrian.wenl@gmail.com (Lei Wen)]

Hi Igor,

On Wed, Jun 22, 2011 at 9:45 PM, Igor Grinberg <grinberg@compulab.co.il> wrote:
> Hi Lei,
>
>
> Some comments from a quick look...
>
>
> On 06/22/11 06:17, Lei Wen wrote:
>
>> Current pxa3xx_nand controller has two chip select which
>> both be workable. This patch enable this feature.
>>
>> Update platform driver to support this feature.
>>
>> Another notice should be taken that:
>> When you want to use this feature, you should not enable the
>> keep configuration feature, for two chip select could be
>> attached with different nand chip. The different page size
>> and timing requirement make the keep configuration impossible.
>
> You should _also_ put this comment inside the pxa3xx_nand.h
> may be even inside the pxa3xx_nand_platform_data structure,
> so people would not have to search the git log to find this problem.

How about just throw out a BUG() to force people cannot set keep_config
when supported cs num more than 1? Also appended with the comments above?

>
>> Signed-off-by: Lei Wen <leiwen@marvell.com>
>> ---
>> ?arch/arm/mach-mmp/aspenite.c ? ? ? ? ? ? ? ? | ? ?5 +-
>> ?arch/arm/mach-pxa/cm-x300.c ? ? ? ? ? ? ? ? ?| ? ?5 +-
>> ?arch/arm/mach-pxa/colibri-pxa3xx.c ? ? ? ? ? | ? ?5 +-
>> ?arch/arm/mach-pxa/littleton.c ? ? ? ? ? ? ? ?| ? ?5 +-
>> ?arch/arm/mach-pxa/mxm8x10.c ? ? ? ? ? ? ? ? ?| ? ?9 +-
>> ?arch/arm/mach-pxa/raumfeld.c ? ? ? ? ? ? ? ? | ? ?5 +-
>> ?arch/arm/mach-pxa/zylonite.c ? ? ? ? ? ? ? ? | ? ?5 +-
>> ?arch/arm/plat-pxa/include/plat/pxa3xx_nand.h | ? ?8 +-
>> ?drivers/mtd/nand/pxa3xx_nand.c ? ? ? ? ? ? ? | ?735 +++++++++++++++-----------
>> ?9 files changed, 444 insertions(+), 338 deletions(-)
>>
>> diff --git a/arch/arm/mach-mmp/aspenite.c b/arch/arm/mach-mmp/aspenite.c
>> index 06b5fa8..b6589d6 100644
>> --- a/arch/arm/mach-mmp/aspenite.c
>> +++ b/arch/arm/mach-mmp/aspenite.c
>> @@ -167,8 +167,9 @@ static struct mtd_partition aspenite_nand_partitions[] = {
>>
>> ?static struct pxa3xx_nand_platform_data aspenite_nand_info = {
>> ? ? ? .enable_arbiter = 1,
>> - ? ? .parts ? ? ? ? ?= aspenite_nand_partitions,
>> - ? ? .nr_parts ? ? ? = ARRAY_SIZE(aspenite_nand_partitions),
>> + ? ? .cs_num = 1,
>> + ? ? .parts[0] ? ? ? = aspenite_nand_partitions,
>> + ? ? .nr_parts[0] ? ?= ARRAY_SIZE(aspenite_nand_partitions),
>> ?};
>>
>> ?static struct i2c_board_info aspenite_i2c_info[] __initdata = {
>> diff --git a/arch/arm/mach-pxa/cm-x300.c b/arch/arm/mach-pxa/cm-x300.c
>> index b2248e7..d67eb7b 100644
>> --- a/arch/arm/mach-pxa/cm-x300.c
>> +++ b/arch/arm/mach-pxa/cm-x300.c
>> @@ -423,8 +423,9 @@ static struct mtd_partition cm_x300_nand_partitions[] = {
>> ?static struct pxa3xx_nand_platform_data cm_x300_nand_info = {
>> ? ? ? .enable_arbiter = 1,
>> ? ? ? .keep_config ? ?= 1,
>> - ? ? .parts ? ? ? ? ?= cm_x300_nand_partitions,
>> - ? ? .nr_parts ? ? ? = ARRAY_SIZE(cm_x300_nand_partitions),
>> + ? ? .cs_num ? ? ? ? = 1,
>> + ? ? .parts[0] ? ? ? = cm_x300_nand_partitions,
>> + ? ? .nr_parts[0] ? ?= ARRAY_SIZE(cm_x300_nand_partitions),
>> ?};
>>
>> ?static void __init cm_x300_init_nand(void)
>> diff --git a/arch/arm/mach-pxa/colibri-pxa3xx.c b/arch/arm/mach-pxa/colibri-pxa3xx.c
>> index 3f9be41..ff7a07b 100644
>> --- a/arch/arm/mach-pxa/colibri-pxa3xx.c
>> +++ b/arch/arm/mach-pxa/colibri-pxa3xx.c
>> @@ -139,8 +139,9 @@ static struct mtd_partition colibri_nand_partitions[] = {
>> ?static struct pxa3xx_nand_platform_data colibri_nand_info = {
>> ? ? ? .enable_arbiter = 1,
>> ? ? ? .keep_config ? ?= 1,
>> - ? ? .parts ? ? ? ? ?= colibri_nand_partitions,
>> - ? ? .nr_parts ? ? ? = ARRAY_SIZE(colibri_nand_partitions),
>> + ? ? .cs_num ? ? ? ? = 1,
>> + ? ? .parts[0] ? ? ? = colibri_nand_partitions,
>> + ? ? .nr_parts[0] ? ?= ARRAY_SIZE(colibri_nand_partitions),
>> ?};
>>
>> ?void __init colibri_pxa3xx_init_nand(void)
>> diff --git a/arch/arm/mach-pxa/littleton.c b/arch/arm/mach-pxa/littleton.c
>> index e5e326d..6eaf852 100644
>> --- a/arch/arm/mach-pxa/littleton.c
>> +++ b/arch/arm/mach-pxa/littleton.c
>> @@ -325,8 +325,9 @@ static struct mtd_partition littleton_nand_partitions[] = {
>>
>> ?static struct pxa3xx_nand_platform_data littleton_nand_info = {
>> ? ? ? .enable_arbiter = 1,
>> - ? ? .parts ? ? ? ? ?= littleton_nand_partitions,
>> - ? ? .nr_parts ? ? ? = ARRAY_SIZE(littleton_nand_partitions),
>> + ? ? .cs_num ? ? ? ? = 1,
>> + ? ? .parts[0] ? ? ? = littleton_nand_partitions,
>> + ? ? .nr_parts[0] ? ?= ARRAY_SIZE(littleton_nand_partitions),
>> ?};
>>
>> ?static void __init littleton_init_nand(void)
>> diff --git a/arch/arm/mach-pxa/mxm8x10.c b/arch/arm/mach-pxa/mxm8x10.c
>> index b5a8fd3..e7ce135 100644
>> --- a/arch/arm/mach-pxa/mxm8x10.c
>> +++ b/arch/arm/mach-pxa/mxm8x10.c
>> @@ -389,10 +389,11 @@ static struct mtd_partition mxm_8x10_nand_partitions[] = {
>> ?};
>>
>> ?static struct pxa3xx_nand_platform_data mxm_8x10_nand_info = {
>> - ? ? .enable_arbiter = 1,
>> - ? ? .keep_config = 1,
>> - ? ? .parts = mxm_8x10_nand_partitions,
>> - ? ? .nr_parts = ARRAY_SIZE(mxm_8x10_nand_partitions)
>> + ? ? .enable_arbiter = 1,
>> + ? ? .keep_config ? ?= 1,
>> + ? ? .cs_num ? ? ? ? = 1,
>> + ? ? .parts[0] ? ? ? = mxm_8x10_nand_partitions,
>> + ? ? .nr_parts[0] ? ?= ARRAY_SIZE(mxm_8x10_nand_partitions)
>> ?};
>>
>> ?static void __init mxm_8x10_nand_init(void)
>> diff --git a/arch/arm/mach-pxa/raumfeld.c b/arch/arm/mach-pxa/raumfeld.c
>> index d130f77..a54846d 100644
>> --- a/arch/arm/mach-pxa/raumfeld.c
>> +++ b/arch/arm/mach-pxa/raumfeld.c
>> @@ -349,8 +349,9 @@ static struct mtd_partition raumfeld_nand_partitions[] = {
>> ?static struct pxa3xx_nand_platform_data raumfeld_nand_info = {
>> ? ? ? .enable_arbiter = 1,
>> ? ? ? .keep_config ? ?= 1,
>> - ? ? .parts ? ? ? ? ?= raumfeld_nand_partitions,
>> - ? ? .nr_parts ? ? ? = ARRAY_SIZE(raumfeld_nand_partitions),
>> + ? ? .cs_num ? ? ? ? = 1,
>> + ? ? .parts[0] ? ? ? = raumfeld_nand_partitions,
>> + ? ? .nr_parts[0] ? ?= ARRAY_SIZE(raumfeld_nand_partitions),
>> ?};
>>
>> ?/**
>> diff --git a/arch/arm/mach-pxa/zylonite.c b/arch/arm/mach-pxa/zylonite.c
>> index 5821185..ea4752a 100644
>> --- a/arch/arm/mach-pxa/zylonite.c
>> +++ b/arch/arm/mach-pxa/zylonite.c
>> @@ -366,8 +366,9 @@ static struct mtd_partition zylonite_nand_partitions[] = {
>>
>> ?static struct pxa3xx_nand_platform_data zylonite_nand_info = {
>> ? ? ? .enable_arbiter = 1,
>> - ? ? .parts ? ? ? ? ?= zylonite_nand_partitions,
>> - ? ? .nr_parts ? ? ? = ARRAY_SIZE(zylonite_nand_partitions),
>> + ? ? .cs_num ? ? ? ? = 1,
>> + ? ? .parts[0] ? ? ? = zylonite_nand_partitions,
>> + ? ? .nr_parts[0] ? ?= ARRAY_SIZE(zylonite_nand_partitions),
>> ?};
>>
>> ?static void __init zylonite_init_nand(void)
>> diff --git a/arch/arm/plat-pxa/include/plat/pxa3xx_nand.h b/arch/arm/plat-pxa/include/plat/pxa3xx_nand.h
>> index 442301f..34a3f52 100644
>> --- a/arch/arm/plat-pxa/include/plat/pxa3xx_nand.h
>> +++ b/arch/arm/plat-pxa/include/plat/pxa3xx_nand.h
>> @@ -41,6 +41,8 @@ struct pxa3xx_nand_flash {
>> ? ? ? struct pxa3xx_nand_timing *timing; ? ? ?/* NAND Flash timing */
>> ?};
>>
>> +/* The max num of chip select current support */
>
> /* The maximum number of chip selects currently supported */
>
>> +#define NUM_CHIP_SELECT ? ? ? ? ? ? ?(2)
>> ?struct pxa3xx_nand_platform_data {
>>
>> ? ? ? /* the data flash bus is shared between the Static Memory
>> @@ -52,8 +54,10 @@ struct pxa3xx_nand_platform_data {
>> ? ? ? /* allow platform code to keep OBM/bootloader defined NFC config */
>> ? ? ? int ? ? keep_config;
>>
>> - ? ? const struct mtd_partition ? ? ? ? ? ? ?*parts;
>> - ? ? unsigned int ? ? ? ? ? ? ? ? ? ? ? ? ? ?nr_parts;
>> + ? ? /* indicate how many chip select would be used for this platform */
>
> /* indicate how many chip selects will be used */
>
>> + ? ? int ? ? cs_num;
>
> This name is too confusing, I think even num_cs is better or cs_count?
> Also, may be align it with the others?

 I prefer the num_cs.

>
>> + ? ? const struct mtd_partition ? ? ? ? ? ? ?*parts[NUM_CHIP_SELECT];
>> + ? ? unsigned int ? ? ? ? ? ? ? ? ? ? ? ? ? ?nr_parts[NUM_CHIP_SELECT];
>>
>> ? ? ? const struct pxa3xx_nand_flash * ? ? ? ?flash;
>> ? ? ? size_t ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?num_flash;
>> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
>> index 30689cc..259b8d5 100644
>> --- a/drivers/mtd/nand/pxa3xx_nand.c
>> +++ b/drivers/mtd/nand/pxa3xx_nand.c
>> @@ -92,11 +92,13 @@
>> ?#define NDCB0_ADDR_CYC_SHIFT (16)
>>
>> ?/* macros for registers read/write */
>> -#define nand_writel(info, off, val) ?\
>> - ? ? __raw_writel((val), (info)->mmio_base + (off))
>> +#define nand_writel(nand, off, val) ?\
>> + ? ? __raw_writel((val), (nand)->mmio_base + (off))
>>
>> -#define nand_readl(info, off) ? ? ? ? ? ? ? ?\
>> - ? ? __raw_readl((info)->mmio_base + (off))
>> +#define nand_readl(nand, off) ? ? ? ? ? ? ? ?\
>> + ? ? __raw_readl((nand)->mmio_base + (off))
>> +#define get_mtd_by_info(info) ? ? ? ? ? ? ? ?\
>> + ? ? (struct mtd_info *)((void *)info - sizeof(struct mtd_info))
>>
>> ?/* error code and state */
>> ?enum {
>> @@ -110,6 +112,7 @@ enum {
>>
>> ?enum {
>> ? ? ? STATE_IDLE = 0,
>> + ? ? STATE_PREPARED,
>> ? ? ? STATE_CMD_HANDLE,
>> ? ? ? STATE_DMA_READING,
>> ? ? ? STATE_DMA_WRITING,
>> @@ -123,63 +126,63 @@ enum {
>> ?struct pxa3xx_nand_info {
>> ? ? ? struct nand_chip ? ? ? ?nand_chip;
>>
>> - ? ? struct nand_hw_control ?controller;
>> - ? ? struct platform_device ? *pdev;
>> ? ? ? struct pxa3xx_nand_cmdset *cmdset;
>> + ? ? /* page size of attached chip */
>> + ? ? uint16_t ? ? ? ? ? ? ? ?page_size;
>> + ? ? uint8_t ? ? ? ? ? ? ? ? chip_select;
>> + ? ? uint8_t ? ? ? ? ? ? ? ? use_ecc;
>> +
>> + ? ? /* calculated from pxa3xx_nand_flash data */
>> + ? ? uint8_t ? ? ? ? ? ? ? ? col_addr_cycles;
>> + ? ? uint8_t ? ? ? ? ? ? ? ? row_addr_cycles;
>> + ? ? uint8_t ? ? ? ? ? ? ? ? read_id_bytes;
>> +
>> + ? ? /* cached register value */
>> + ? ? uint32_t ? ? ? ? ? ? ? ?reg_ndcr;
>> + ? ? uint32_t ? ? ? ? ? ? ? ?ndtr0cs0;
>> + ? ? uint32_t ? ? ? ? ? ? ? ?ndtr1cs0;
>>
>> + ? ? void ? ? ? ? ? ? ? ? ? ?*nand_data;
>> +};
>> +
>> +struct pxa3xx_nand {
>> ? ? ? struct clk ? ? ? ? ? ? ?*clk;
>> ? ? ? void __iomem ? ? ? ? ? ?*mmio_base;
>> ? ? ? unsigned long ? ? ? ? ? mmio_phys;
>> + ? ? struct nand_hw_control ?controller;
>> + ? ? struct completion ? ? ? cmd_complete;
>> + ? ? struct platform_device ? *pdev;
>
> please, align

Do you still mean cs_num? What this comment refer to?

>
>>
>> - ? ? unsigned int ? ? ? ? ? ?buf_start;
>> - ? ? unsigned int ? ? ? ? ? ?buf_count;
>> -
>> - ? ? struct mtd_info ? ? ? ? *mtd;
>> ? ? ? /* DMA information */
>> ? ? ? int ? ? ? ? ? ? ? ? ? ? drcmr_dat;
>> ? ? ? int ? ? ? ? ? ? ? ? ? ? drcmr_cmd;
>> -
>> - ? ? unsigned char ? ? ? ? ? *data_buff;
>> - ? ? unsigned char ? ? ? ? ? *oob_buff;
>> - ? ? dma_addr_t ? ? ? ? ? ? ?data_buff_phys;
>> - ? ? size_t ? ? ? ? ? ? ? ? ?data_buff_size;
>> ? ? ? int ? ? ? ? ? ? ? ? ? ? data_dma_ch;
>> - ? ? struct pxa_dma_desc ? ? *data_desc;
>> + ? ? dma_addr_t ? ? ? ? ? ? ?data_buff_phys;
>> ? ? ? dma_addr_t ? ? ? ? ? ? ?data_desc_addr;
>> + ? ? struct pxa_dma_desc ? ? *data_desc;
>>
>> - ? ? uint32_t ? ? ? ? ? ? ? ?reg_ndcr;
>> -
>> - ? ? /* saved column/page_addr during CMD_SEQIN */
>> - ? ? int ? ? ? ? ? ? ? ? ? ? seqin_column;
>> - ? ? int ? ? ? ? ? ? ? ? ? ? seqin_page_addr;
>> + ? ? struct pxa3xx_nand_info *info[NUM_CHIP_SELECT];
>> + ? ? uint32_t ? ? ? ? ? ? ? ?command;
>> + ? ? uint16_t ? ? ? ? ? ? ? ?data_size; ? ? ?/* data size in FIFO */
>> + ? ? uint16_t ? ? ? ? ? ? ? ?oob_size;
>> + ? ? unsigned char ? ? ? ? ? *data_buff;
>> + ? ? unsigned char ? ? ? ? ? *oob_buff;
>> + ? ? uint32_t ? ? ? ? ? ? ? ?buf_start;
>> + ? ? uint32_t ? ? ? ? ? ? ? ?buf_count;
>>
>> ? ? ? /* relate to the command */
>> ? ? ? unsigned int ? ? ? ? ? ?state;
>> -
>> + ? ? uint8_t ? ? ? ? ? ? ? ? chip_select;
>> ? ? ? int ? ? ? ? ? ? ? ? ? ? use_ecc; ? ? ? ?/* use HW ECC ? */
>> ? ? ? int ? ? ? ? ? ? ? ? ? ? use_dma; ? ? ? ?/* use DMA ? */
>> ? ? ? int ? ? ? ? ? ? ? ? ? ? is_ready;
>> -
>> - ? ? unsigned int ? ? ? ? ? ?page_size; ? ? ?/* page size of attached chip */
>> - ? ? unsigned int ? ? ? ? ? ?data_size; ? ? ?/* data size in FIFO */
>> ? ? ? int ? ? ? ? ? ? ? ? ? ? retcode;
>> - ? ? struct completion ? ? ? cmd_complete;
>>
>> ? ? ? /* generated NDCBx register values */
>> + ? ? uint8_t ? ? ? ? ? ? ? ? total_cmds;
>> ? ? ? uint32_t ? ? ? ? ? ? ? ?ndcb0;
>> ? ? ? uint32_t ? ? ? ? ? ? ? ?ndcb1;
>> ? ? ? uint32_t ? ? ? ? ? ? ? ?ndcb2;
>> -
>> - ? ? /* timing calcuted from setting */
>> - ? ? uint32_t ? ? ? ? ? ? ? ?ndtr0cs0;
>> - ? ? uint32_t ? ? ? ? ? ? ? ?ndtr1cs0;
>> -
>> - ? ? /* calculated from pxa3xx_nand_flash data */
>> - ? ? size_t ? ? ? ? ?oob_size;
>> - ? ? size_t ? ? ? ? ?read_id_bytes;
>> -
>> - ? ? unsigned int ? ?col_addr_cycles;
>> - ? ? unsigned int ? ?row_addr_cycles;
>> ?};
>
> It looks like if you switch the names of the structures above,
> then the patch will be much shorter and cleaner,
> but will it make structures meaning confusion?

Could you elaborate more about this? Do you means no need to create a seperated
structure?

>
>>
>> ?static int use_dma = 1;
>> @@ -225,7 +228,7 @@ static struct pxa3xx_nand_flash builtin_flash_types[] = {
>> ?/* Define a default flash type setting serve as flash detecting only */
>> ?#define DEFAULT_FLASH_TYPE (&builtin_flash_types[0])
>>
>> -const char *mtd_names[] = {"pxa3xx_nand-0", NULL};
>> +const char *mtd_names[] = {"pxa3xx_nand-0", "pxa3xx_nand-1", NULL};
>>
>> ?#define NDTR0_tCH(c) (min((c), 7) << 19)
>> ?#define NDTR0_tCS(c) (min((c), 7) << 16)
>> @@ -244,9 +247,11 @@ const char *mtd_names[] = {"pxa3xx_nand-0", NULL};
>> ?static void pxa3xx_nand_set_timing(struct pxa3xx_nand_info *info,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?const struct pxa3xx_nand_timing *t)
>> ?{
>> - ? ? unsigned long nand_clk = clk_get_rate(info->clk);
>> + ? ? struct pxa3xx_nand *nand = info->nand_data;
>> + ? ? unsigned long nand_clk;
>> ? ? ? uint32_t ndtr0, ndtr1;
>>
>> + ? ? nand_clk = clk_get_rate(nand->clk);
>> ? ? ? ndtr0 = NDTR0_tCH(ns2cycle(t->tCH, nand_clk)) |
>> ? ? ? ? ? ? ? NDTR0_tCS(ns2cycle(t->tCS, nand_clk)) |
>> ? ? ? ? ? ? ? NDTR0_tWH(ns2cycle(t->tWH, nand_clk)) |
>> @@ -260,26 +265,27 @@ static void pxa3xx_nand_set_timing(struct pxa3xx_nand_info *info,
>>
>> ? ? ? info->ndtr0cs0 = ndtr0;
>> ? ? ? info->ndtr1cs0 = ndtr1;
>> - ? ? nand_writel(info, NDTR0CS0, ndtr0);
>> - ? ? nand_writel(info, NDTR1CS0, ndtr1);
>> + ? ? nand_writel(nand, NDTR0CS0, ndtr0);
>> + ? ? nand_writel(nand, NDTR1CS0, ndtr1);
>> ?}
>>
>> ?static void pxa3xx_set_datasize(struct pxa3xx_nand_info *info)
>> ?{
>> + ? ? struct pxa3xx_nand *nand = info->nand_data;
>> ? ? ? int oob_enable = info->reg_ndcr & NDCR_SPARE_EN;
>>
>> - ? ? info->data_size = info->page_size;
>> + ? ? nand->data_size = info->page_size;
>> ? ? ? if (!oob_enable) {
>> - ? ? ? ? ? ? info->oob_size = 0;
>> + ? ? ? ? ? ? nand->oob_size = 0;
>> ? ? ? ? ? ? ? return;
>> ? ? ? }
>>
>> ? ? ? switch (info->page_size) {
>> ? ? ? case 2048:
>> - ? ? ? ? ? ? info->oob_size = (info->use_ecc) ? 40 : 64;
>> + ? ? ? ? ? ? nand->oob_size = (info->use_ecc) ? 40 : 64;
>> ? ? ? ? ? ? ? break;
>> ? ? ? case 512:
>> - ? ? ? ? ? ? info->oob_size = (info->use_ecc) ? 8 : 16;
>> + ? ? ? ? ? ? nand->oob_size = (info->use_ecc) ? 8 : 16;
>> ? ? ? ? ? ? ? break;
>> ? ? ? }
>> ?}
>> @@ -290,185 +296,189 @@ static void pxa3xx_set_datasize(struct pxa3xx_nand_info *info)
>> ? * We enable all the interrupt at the same time, and
>> ? * let pxa3xx_nand_irq to handle all logic.
>> ? */
>> -static void pxa3xx_nand_start(struct pxa3xx_nand_info *info)
>> +static void pxa3xx_nand_start(struct pxa3xx_nand *nand)
>> ?{
>> + ? ? struct pxa3xx_nand_info *info;
>> ? ? ? uint32_t ndcr;
>>
>> + ? ? info = nand->info[nand->chip_select];
>> ? ? ? ndcr = info->reg_ndcr;
>> - ? ? ndcr |= info->use_ecc ? NDCR_ECC_EN : 0;
>> - ? ? ndcr |= info->use_dma ? NDCR_DMA_EN : 0;
>> + ? ? ndcr |= nand->use_ecc ? NDCR_ECC_EN : 0;
>> + ? ? ndcr |= nand->use_dma ? NDCR_DMA_EN : 0;
>> ? ? ? ndcr |= NDCR_ND_RUN;
>>
>> ? ? ? /* clear status bits and run */
>> - ? ? nand_writel(info, NDCR, 0);
>> - ? ? nand_writel(info, NDSR, NDSR_MASK);
>> - ? ? nand_writel(info, NDCR, ndcr);
>> + ? ? nand_writel(nand, NDCR, 0);
>> + ? ? nand_writel(nand, NDSR, NDSR_MASK);
>> + ? ? nand_writel(nand, NDCR, ndcr);
>> ?}
>>
>> -static void pxa3xx_nand_stop(struct pxa3xx_nand_info *info)
>> +static void pxa3xx_nand_stop(struct pxa3xx_nand *nand)
>> ?{
>> ? ? ? uint32_t ndcr;
>> ? ? ? int timeout = NAND_STOP_DELAY;
>>
>> ? ? ? /* wait RUN bit in NDCR become 0 */
>> - ? ? ndcr = nand_readl(info, NDCR);
>> + ? ? ndcr = nand_readl(nand, NDCR);
>> ? ? ? while ((ndcr & NDCR_ND_RUN) && (timeout-- > 0)) {
>> - ? ? ? ? ? ? ndcr = nand_readl(info, NDCR);
>> + ? ? ? ? ? ? ndcr = nand_readl(nand, NDCR);
>> ? ? ? ? ? ? ? udelay(1);
>> ? ? ? }
>>
>> ? ? ? if (timeout <= 0) {
>> ? ? ? ? ? ? ? ndcr &= ~NDCR_ND_RUN;
>> - ? ? ? ? ? ? nand_writel(info, NDCR, ndcr);
>> + ? ? ? ? ? ? nand_writel(nand, NDCR, ndcr);
>> ? ? ? }
>> ? ? ? /* clear status bits */
>> - ? ? nand_writel(info, NDSR, NDSR_MASK);
>> + ? ? nand_writel(nand, NDSR, NDSR_MASK);
>> ?}
>>
>> -static void enable_int(struct pxa3xx_nand_info *info, uint32_t int_mask)
>> +static void enable_int(struct pxa3xx_nand *nand, uint32_t int_mask)
>> ?{
>> ? ? ? uint32_t ndcr;
>>
>> - ? ? ndcr = nand_readl(info, NDCR);
>> - ? ? nand_writel(info, NDCR, ndcr & ~int_mask);
>> + ? ? ndcr = nand_readl(nand, NDCR);
>> + ? ? nand_writel(nand, NDCR, ndcr & ~int_mask);
>> ?}
>>
>> -static void disable_int(struct pxa3xx_nand_info *info, uint32_t int_mask)
>> +static void disable_int(struct pxa3xx_nand *nand, uint32_t int_mask)
>> ?{
>> ? ? ? uint32_t ndcr;
>>
>> - ? ? ndcr = nand_readl(info, NDCR);
>> - ? ? nand_writel(info, NDCR, ndcr | int_mask);
>> + ? ? ndcr = nand_readl(nand, NDCR);
>> + ? ? nand_writel(nand, NDCR, ndcr | int_mask);
>> ?}
>>
>> -static void handle_data_pio(struct pxa3xx_nand_info *info)
>> +static void handle_data_pio(struct pxa3xx_nand *nand)
>> ?{
>> - ? ? switch (info->state) {
>> + ? ? switch (nand->state) {
>> ? ? ? case STATE_PIO_WRITING:
>> - ? ? ? ? ? ? __raw_writesl(info->mmio_base + NDDB, info->data_buff,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? DIV_ROUND_UP(info->data_size, 4));
>> - ? ? ? ? ? ? if (info->oob_size > 0)
>> - ? ? ? ? ? ? ? ? ? ? __raw_writesl(info->mmio_base + NDDB, info->oob_buff,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? DIV_ROUND_UP(info->oob_size, 4));
>> + ? ? ? ? ? ? __raw_writesl(nand->mmio_base + NDDB, nand->data_buff,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? DIV_ROUND_UP(nand->data_size, 4));
>> + ? ? ? ? ? ? if (nand->oob_size > 0)
>> + ? ? ? ? ? ? ? ? ? ? __raw_writesl(nand->mmio_base + NDDB, nand->oob_buff,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? DIV_ROUND_UP(nand->oob_size, 4));
>> ? ? ? ? ? ? ? break;
>> ? ? ? case STATE_PIO_READING:
>> - ? ? ? ? ? ? __raw_readsl(info->mmio_base + NDDB, info->data_buff,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? DIV_ROUND_UP(info->data_size, 4));
>> - ? ? ? ? ? ? if (info->oob_size > 0)
>> - ? ? ? ? ? ? ? ? ? ? __raw_readsl(info->mmio_base + NDDB, info->oob_buff,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? DIV_ROUND_UP(info->oob_size, 4));
>> + ? ? ? ? ? ? __raw_readsl(nand->mmio_base + NDDB, nand->data_buff,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? DIV_ROUND_UP(nand->data_size, 4));
>> + ? ? ? ? ? ? if (nand->oob_size > 0)
>> + ? ? ? ? ? ? ? ? ? ? __raw_readsl(nand->mmio_base + NDDB, nand->oob_buff,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? DIV_ROUND_UP(nand->oob_size, 4));
>> ? ? ? ? ? ? ? break;
>> ? ? ? default:
>> ? ? ? ? ? ? ? printk(KERN_ERR "%s: invalid state %d\n", __func__,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? info->state);
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? nand->state);
>> ? ? ? ? ? ? ? BUG();
>> ? ? ? }
>> ?}
>>
>> -static void start_data_dma(struct pxa3xx_nand_info *info)
>> +static void start_data_dma(struct pxa3xx_nand *nand)
>> ?{
>> - ? ? struct pxa_dma_desc *desc = info->data_desc;
>> - ? ? int dma_len = ALIGN(info->data_size + info->oob_size, 32);
>> + ? ? struct pxa_dma_desc *desc = nand->data_desc;
>> + ? ? int dma_len = ALIGN(nand->data_size + nand->oob_size, 32);
>>
>> ? ? ? desc->ddadr = DDADR_STOP;
>> ? ? ? desc->dcmd = DCMD_ENDIRQEN | DCMD_WIDTH4 | DCMD_BURST32 | dma_len;
>>
>> - ? ? switch (info->state) {
>> + ? ? switch (nand->state) {
>> ? ? ? case STATE_DMA_WRITING:
>> - ? ? ? ? ? ? desc->dsadr = info->data_buff_phys;
>> - ? ? ? ? ? ? desc->dtadr = info->mmio_phys + NDDB;
>> + ? ? ? ? ? ? desc->dsadr = nand->data_buff_phys;
>> + ? ? ? ? ? ? desc->dtadr = nand->mmio_phys + NDDB;
>> ? ? ? ? ? ? ? desc->dcmd |= DCMD_INCSRCADDR | DCMD_FLOWTRG;
>> ? ? ? ? ? ? ? break;
>> ? ? ? case STATE_DMA_READING:
>> - ? ? ? ? ? ? desc->dtadr = info->data_buff_phys;
>> - ? ? ? ? ? ? desc->dsadr = info->mmio_phys + NDDB;
>> + ? ? ? ? ? ? desc->dtadr = nand->data_buff_phys;
>> + ? ? ? ? ? ? desc->dsadr = nand->mmio_phys + NDDB;
>> ? ? ? ? ? ? ? desc->dcmd |= DCMD_INCTRGADDR | DCMD_FLOWSRC;
>> ? ? ? ? ? ? ? break;
>> ? ? ? default:
>> ? ? ? ? ? ? ? printk(KERN_ERR "%s: invalid state %d\n", __func__,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? info->state);
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? nand->state);
>> ? ? ? ? ? ? ? BUG();
>> ? ? ? }
>>
>> - ? ? DRCMR(info->drcmr_dat) = DRCMR_MAPVLD | info->data_dma_ch;
>> - ? ? DDADR(info->data_dma_ch) = info->data_desc_addr;
>> - ? ? DCSR(info->data_dma_ch) |= DCSR_RUN;
>> + ? ? DRCMR(nand->drcmr_dat) = DRCMR_MAPVLD | nand->data_dma_ch;
>> + ? ? DDADR(nand->data_dma_ch) = nand->data_desc_addr;
>> + ? ? DCSR(nand->data_dma_ch) |= DCSR_RUN;
>> ?}
>>
>> ?static void pxa3xx_nand_data_dma_irq(int channel, void *data)
>> ?{
>> - ? ? struct pxa3xx_nand_info *info = data;
>> + ? ? struct pxa3xx_nand *nand = data;
>> ? ? ? uint32_t dcsr;
>>
>> ? ? ? dcsr = DCSR(channel);
>> ? ? ? DCSR(channel) = dcsr;
>>
>> ? ? ? if (dcsr & DCSR_BUSERR) {
>> - ? ? ? ? ? ? info->retcode = ERR_DMABUSERR;
>> + ? ? ? ? ? ? nand->retcode = ERR_DMABUSERR;
>> ? ? ? }
>>
>> - ? ? info->state = STATE_DMA_DONE;
>> - ? ? enable_int(info, NDCR_INT_MASK);
>> - ? ? nand_writel(info, NDSR, NDSR_WRDREQ | NDSR_RDDREQ);
>> + ? ? nand->state = STATE_DMA_DONE;
>> + ? ? enable_int(nand, NDCR_INT_MASK);
>> + ? ? nand_writel(nand, NDSR, NDSR_WRDREQ | NDSR_RDDREQ);
>> ?}
>>
>> ?static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>> ?{
>> - ? ? struct pxa3xx_nand_info *info = devid;
>> - ? ? unsigned int status, is_completed = 0;
>> + ? ? struct pxa3xx_nand *nand = devid;
>> + ? ? struct pxa3xx_nand_info *info;
>> + ? ? unsigned int status, is_completed = 0, cs;
>> + ? ? unsigned int ready, cmd_done, page_done, badblock_detect;
>>
>> - ? ? status = nand_readl(info, NDSR);
>> + ? ? cs ? ? ? ? ? ? ?= nand->chip_select;
>> + ? ? ready ? ? ? ? ? = (cs) ? NDSR_RDY : NDSR_FLASH_RDY;
>> + ? ? cmd_done ? ? ? ?= (cs) ? NDSR_CS1_CMDD : NDSR_CS0_CMDD;
>> + ? ? page_done ? ? ? = (cs) ? NDSR_CS1_PAGED : NDSR_CS0_PAGED;
>> + ? ? badblock_detect = (cs) ? NDSR_CS1_BBD : NDSR_CS0_BBD;
>
> This is confusing... do you use to ?: operator for differentiating between
> cs = 0 and cs = 1?
> I think this is a bad idea.
> Moreover, the use of ?: is discouraged among the kernel.

I could use if/else to replace this statement.

>
>> + ? ? info ? ? ? ? ? ?= nand->info[cs];
>>
>> + ? ? status = nand_readl(nand, NDSR);
>> ? ? ? if (status & NDSR_DBERR)
>> - ? ? ? ? ? ? info->retcode = ERR_DBERR;
>> + ? ? ? ? ? ? nand->retcode = ERR_DBERR;
>> ? ? ? if (status & NDSR_SBERR)
>> - ? ? ? ? ? ? info->retcode = ERR_SBERR;
>> + ? ? ? ? ? ? nand->retcode = ERR_SBERR;
>> ? ? ? if (status & (NDSR_RDDREQ | NDSR_WRDREQ)) {
>> ? ? ? ? ? ? ? /* whether use dma to transfer data */
>> - ? ? ? ? ? ? if (info->use_dma) {
>> - ? ? ? ? ? ? ? ? ? ? disable_int(info, NDCR_INT_MASK);
>> - ? ? ? ? ? ? ? ? ? ? info->state = (status & NDSR_RDDREQ) ?
>> + ? ? ? ? ? ? if (nand->use_dma) {
>> + ? ? ? ? ? ? ? ? ? ? disable_int(nand, NDCR_INT_MASK);
>> + ? ? ? ? ? ? ? ? ? ? nand->state = (status & NDSR_RDDREQ) ?
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? STATE_DMA_READING : STATE_DMA_WRITING;
>> - ? ? ? ? ? ? ? ? ? ? start_data_dma(info);
>> + ? ? ? ? ? ? ? ? ? ? start_data_dma(nand);
>> ? ? ? ? ? ? ? ? ? ? ? goto NORMAL_IRQ_EXIT;
>> ? ? ? ? ? ? ? } else {
>> - ? ? ? ? ? ? ? ? ? ? info->state = (status & NDSR_RDDREQ) ?
>> + ? ? ? ? ? ? ? ? ? ? nand->state = (status & NDSR_RDDREQ) ?
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? STATE_PIO_READING : STATE_PIO_WRITING;
>> - ? ? ? ? ? ? ? ? ? ? handle_data_pio(info);
>> + ? ? ? ? ? ? ? ? ? ? handle_data_pio(nand);
>> ? ? ? ? ? ? ? }
>> ? ? ? }
>> - ? ? if (status & NDSR_CS0_CMDD) {
>> - ? ? ? ? ? ? info->state = STATE_CMD_DONE;
>> + ? ? if (status & cmd_done) {
>> + ? ? ? ? ? ? nand->state = STATE_CMD_DONE;
>> ? ? ? ? ? ? ? is_completed = 1;
>> ? ? ? }
>> - ? ? if (status & NDSR_FLASH_RDY) {
>> - ? ? ? ? ? ? info->is_ready = 1;
>> - ? ? ? ? ? ? info->state = STATE_READY;
>> + ? ? if (status & ready) {
>> + ? ? ? ? ? ? nand->is_ready = 1;
>> + ? ? ? ? ? ? nand->state = STATE_READY;
>> ? ? ? }
>>
>> ? ? ? if (status & NDSR_WRCMDREQ) {
>> - ? ? ? ? ? ? nand_writel(info, NDSR, NDSR_WRCMDREQ);
>> + ? ? ? ? ? ? nand_writel(nand, NDSR, NDSR_WRCMDREQ);
>> ? ? ? ? ? ? ? status &= ~NDSR_WRCMDREQ;
>> - ? ? ? ? ? ? info->state = STATE_CMD_HANDLE;
>> - ? ? ? ? ? ? nand_writel(info, NDCB0, info->ndcb0);
>> - ? ? ? ? ? ? nand_writel(info, NDCB0, info->ndcb1);
>> - ? ? ? ? ? ? nand_writel(info, NDCB0, info->ndcb2);
>> + ? ? ? ? ? ? nand->state = STATE_CMD_HANDLE;
>> + ? ? ? ? ? ? nand_writel(nand, NDCB0, nand->ndcb0);
>> + ? ? ? ? ? ? nand_writel(nand, NDCB0, nand->ndcb1);
>> + ? ? ? ? ? ? nand_writel(nand, NDCB0, nand->ndcb2);
>> ? ? ? }
>>
>> ? ? ? /* clear NDSR to let the controller exit the IRQ */
>> - ? ? nand_writel(info, NDSR, status);
>> + ? ? nand_writel(nand, NDSR, status);
>> ? ? ? if (is_completed)
>> - ? ? ? ? ? ? complete(&info->cmd_complete);
>> + ? ? ? ? ? ? complete(&nand->cmd_complete);
>> ?NORMAL_IRQ_EXIT:
>> ? ? ? return IRQ_HANDLED;
>> ?}
>>
>> -static int pxa3xx_nand_dev_ready(struct mtd_info *mtd)
>> -{
>> - ? ? struct pxa3xx_nand_info *info = mtd->priv;
>> - ? ? return (nand_readl(info, NDSR) & NDSR_RDY) ? 1 : 0;
>> -}
>> -
>> ?static inline int is_buf_blank(uint8_t *buf, size_t len)
>> ?{
>> ? ? ? for (; len > 0; len--)
>> @@ -477,42 +487,49 @@ static inline int is_buf_blank(uint8_t *buf, size_t len)
>> ? ? ? return 1;
>> ?}
>>
>> -static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
>> +static int prepare_command_pool(struct pxa3xx_nand *nand, int command,
>> ? ? ? ? ? ? ? uint16_t column, int page_addr)
>> ?{
>> ? ? ? uint16_t cmd;
>> ? ? ? int addr_cycle, exec_cmd, ndcb0;
>> - ? ? struct mtd_info *mtd = info->mtd;
>> + ? ? struct mtd_info *mtd;
>> + ? ? struct pxa3xx_nand_info *info = nand->info[nand->chip_select];
>>
>> - ? ? ndcb0 = 0;
>> + ? ? mtd = get_mtd_by_info(info);
>> + ? ? ndcb0 = (nand->chip_select) ? NDCB0_CSEL : 0;
>
> This one is confusing too...
> Besides, you don't need the parenthesis.

Got that. I would fix it.

>
>> ? ? ? addr_cycle = 0;
>> ? ? ? exec_cmd = 1;
>>
>> ? ? ? /* reset data and oob column point to handle data */
>> - ? ? info->buf_start ? ? ? ? = 0;
>> - ? ? info->buf_count ? ? ? ? = 0;
>> - ? ? info->oob_size ? ? ? ? ?= 0;
>> - ? ? info->use_ecc ? ? ? ? ? = 0;
>> - ? ? info->is_ready ? ? ? ? ?= 0;
>> - ? ? info->retcode ? ? ? ? ? = ERR_NONE;
>> + ? ? nand->buf_start ? ? ? ? = 0;
>> + ? ? nand->buf_count ? ? ? ? = 0;
>> + ? ? nand->oob_size ? ? ? ? ?= 0;
>> + ? ? nand->use_ecc ? ? ? ? ? = 0;
>> + ? ? nand->is_ready ? ? ? ? ?= 0;
>> + ? ? nand->retcode ? ? ? ? ? = ERR_NONE;
>> + ? ? nand->data_size ? ? ? ? = 0;
>> + ? ? nand->use_dma ? ? ? ? ? = 0;
>> + ? ? nand->command ? ? ? ? ? = command;
>>
>> ? ? ? switch (command) {
>> ? ? ? case NAND_CMD_READ0:
>> ? ? ? case NAND_CMD_PAGEPROG:
>> - ? ? ? ? ? ? info->use_ecc = 1;
>> + ? ? ? ? ? ? nand->use_ecc = 1;
>> ? ? ? case NAND_CMD_READOOB:
>> ? ? ? ? ? ? ? pxa3xx_set_datasize(info);
>> + ? ? ? ? ? ? nand->oob_buff = nand->data_buff + nand->data_size;
>> + ? ? ? ? ? ? nand->use_dma = use_dma;
>> ? ? ? ? ? ? ? break;
>> ? ? ? case NAND_CMD_SEQIN:
>> ? ? ? ? ? ? ? exec_cmd = 0;
>> ? ? ? ? ? ? ? break;
>> ? ? ? default:
>> - ? ? ? ? ? ? info->ndcb1 = 0;
>> - ? ? ? ? ? ? info->ndcb2 = 0;
>> + ? ? ? ? ? ? nand->ndcb1 = 0;
>> + ? ? ? ? ? ? nand->ndcb2 = 0;
>> ? ? ? ? ? ? ? break;
>> ? ? ? }
>>
>> - ? ? info->ndcb0 = ndcb0;
>> + ? ? nand->ndcb0 = ndcb0;
>> ? ? ? addr_cycle = NDCB0_ADDR_CYC(info->row_addr_cycles
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? + info->col_addr_cycles);
>>
>> @@ -521,16 +538,16 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
>> ? ? ? case NAND_CMD_READ0:
>> ? ? ? ? ? ? ? cmd = info->cmdset->read1;
>> ? ? ? ? ? ? ? if (command == NAND_CMD_READOOB)
>> - ? ? ? ? ? ? ? ? ? ? info->buf_start = mtd->writesize + column;
>> + ? ? ? ? ? ? ? ? ? ? nand->buf_start = mtd->writesize + column;
>> ? ? ? ? ? ? ? else
>> - ? ? ? ? ? ? ? ? ? ? info->buf_start = column;
>> + ? ? ? ? ? ? ? ? ? ? nand->buf_start = column;
>>
>> ? ? ? ? ? ? ? if (unlikely(info->page_size < PAGE_CHUNK_SIZE))
>> - ? ? ? ? ? ? ? ? ? ? info->ndcb0 |= NDCB0_CMD_TYPE(0)
>> + ? ? ? ? ? ? ? ? ? ? nand->ndcb0 |= NDCB0_CMD_TYPE(0)
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | addr_cycle
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | (cmd & NDCB0_CMD1_MASK);
>> ? ? ? ? ? ? ? else
>> - ? ? ? ? ? ? ? ? ? ? info->ndcb0 |= NDCB0_CMD_TYPE(0)
>> + ? ? ? ? ? ? ? ? ? ? nand->ndcb0 |= NDCB0_CMD_TYPE(0)
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | NDCB0_DBC
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | addr_cycle
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | cmd;
>> @@ -538,34 +555,34 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
>> ? ? ? case NAND_CMD_SEQIN:
>> ? ? ? ? ? ? ? /* small page addr setting */
>> ? ? ? ? ? ? ? if (unlikely(info->page_size < PAGE_CHUNK_SIZE)) {
>> - ? ? ? ? ? ? ? ? ? ? info->ndcb1 = ((page_addr & 0xFFFFFF) << 8)
>> + ? ? ? ? ? ? ? ? ? ? nand->ndcb1 = ((page_addr & 0xFFFFFF) << 8)
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | (column & 0xFF);
>>
>> - ? ? ? ? ? ? ? ? ? ? info->ndcb2 = 0;
>> + ? ? ? ? ? ? ? ? ? ? nand->ndcb2 = 0;
>> ? ? ? ? ? ? ? } else {
>> - ? ? ? ? ? ? ? ? ? ? info->ndcb1 = ((page_addr & 0xFFFF) << 16)
>> + ? ? ? ? ? ? ? ? ? ? nand->ndcb1 = ((page_addr & 0xFFFF) << 16)
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | (column & 0xFFFF);
>>
>> ? ? ? ? ? ? ? ? ? ? ? if (page_addr & 0xFF0000)
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? info->ndcb2 = (page_addr & 0xFF0000) >> 16;
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? nand->ndcb2 = (page_addr & 0xFF0000) >> 16;
>> ? ? ? ? ? ? ? ? ? ? ? else
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? info->ndcb2 = 0;
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? nand->ndcb2 = 0;
>> ? ? ? ? ? ? ? }
>>
>> - ? ? ? ? ? ? info->buf_count = mtd->writesize + mtd->oobsize;
>> - ? ? ? ? ? ? memset(info->data_buff, 0xFF, info->buf_count);
>> + ? ? ? ? ? ? nand->buf_count = mtd->writesize + mtd->oobsize;
>> + ? ? ? ? ? ? memset(nand->data_buff, 0xFF, nand->buf_count);
>>
>> ? ? ? ? ? ? ? break;
>>
>> ? ? ? case NAND_CMD_PAGEPROG:
>> - ? ? ? ? ? ? if (is_buf_blank(info->data_buff,
>> + ? ? ? ? ? ? if (is_buf_blank(nand->data_buff,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (mtd->writesize + mtd->oobsize))) {
>> ? ? ? ? ? ? ? ? ? ? ? exec_cmd = 0;
>> ? ? ? ? ? ? ? ? ? ? ? break;
>> ? ? ? ? ? ? ? }
>>
>> ? ? ? ? ? ? ? cmd = info->cmdset->program;
>> - ? ? ? ? ? ? info->ndcb0 |= NDCB0_CMD_TYPE(0x1)
>> + ? ? ? ? ? ? nand->ndcb0 |= NDCB0_CMD_TYPE(0x1)
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | NDCB0_AUTO_RS
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | NDCB0_ST_ROW_EN
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | NDCB0_DBC
>> @@ -575,37 +592,37 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
>>
>> ? ? ? case NAND_CMD_READID:
>> ? ? ? ? ? ? ? cmd = info->cmdset->read_id;
>> - ? ? ? ? ? ? info->buf_count = info->read_id_bytes;
>> - ? ? ? ? ? ? info->ndcb0 |= NDCB0_CMD_TYPE(3)
>> + ? ? ? ? ? ? nand->buf_count = info->read_id_bytes;
>> + ? ? ? ? ? ? nand->ndcb0 |= NDCB0_CMD_TYPE(3)
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | NDCB0_ADDR_CYC(1)
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | cmd;
>>
>> - ? ? ? ? ? ? info->data_size = 8;
>> + ? ? ? ? ? ? nand->data_size = 8;
>> ? ? ? ? ? ? ? break;
>> ? ? ? case NAND_CMD_STATUS:
>> ? ? ? ? ? ? ? cmd = info->cmdset->read_status;
>> - ? ? ? ? ? ? info->buf_count = 1;
>> - ? ? ? ? ? ? info->ndcb0 |= NDCB0_CMD_TYPE(4)
>> + ? ? ? ? ? ? nand->buf_count = 1;
>> + ? ? ? ? ? ? nand->ndcb0 |= NDCB0_CMD_TYPE(4)
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | NDCB0_ADDR_CYC(1)
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | cmd;
>>
>> - ? ? ? ? ? ? info->data_size = 8;
>> + ? ? ? ? ? ? nand->data_size = 8;
>> ? ? ? ? ? ? ? break;
>>
>> ? ? ? case NAND_CMD_ERASE1:
>> ? ? ? ? ? ? ? cmd = info->cmdset->erase;
>> - ? ? ? ? ? ? info->ndcb0 |= NDCB0_CMD_TYPE(2)
>> + ? ? ? ? ? ? nand->ndcb0 |= NDCB0_CMD_TYPE(2)
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | NDCB0_AUTO_RS
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | NDCB0_ADDR_CYC(3)
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | NDCB0_DBC
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | cmd;
>> - ? ? ? ? ? ? info->ndcb1 = page_addr;
>> - ? ? ? ? ? ? info->ndcb2 = 0;
>> + ? ? ? ? ? ? nand->ndcb1 = page_addr;
>> + ? ? ? ? ? ? nand->ndcb2 = 0;
>>
>> ? ? ? ? ? ? ? break;
>> ? ? ? case NAND_CMD_RESET:
>> ? ? ? ? ? ? ? cmd = info->cmdset->reset;
>> - ? ? ? ? ? ? info->ndcb0 |= NDCB0_CMD_TYPE(5)
>> + ? ? ? ? ? ? nand->ndcb0 |= NDCB0_CMD_TYPE(5)
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | cmd;
>>
>> ? ? ? ? ? ? ? break;
>> @@ -628,6 +645,7 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int column, int page_addr)
>> ?{
>> ? ? ? struct pxa3xx_nand_info *info = mtd->priv;
>> + ? ? struct pxa3xx_nand *nand = info->nand_data;
>> ? ? ? int ret, exec_cmd;
>>
>> ? ? ? /*
>> @@ -638,20 +656,32 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command,
>> ? ? ? if (info->reg_ndcr & NDCR_DWIDTH_M)
>> ? ? ? ? ? ? ? column /= 2;
>>
>> - ? ? exec_cmd = prepare_command_pool(info, command, column, page_addr);
>> + ? ? /*
>> + ? ? ?* There may be different NAND chip hooked to
>> + ? ? ?* different chip select, so check whether
>> + ? ? ?* chip select has been changed, if yes, reset the timing
>> + ? ? ?*/
>> + ? ? if (nand->chip_select != info->chip_select) {
>> + ? ? ? ? ? ? nand->chip_select = info->chip_select;
>> + ? ? ? ? ? ? nand_writel(nand, NDTR0CS0, info->ndtr0cs0);
>> + ? ? ? ? ? ? nand_writel(nand, NDTR1CS0, info->ndtr1cs0);
>> + ? ? }
>> +
>> + ? ? nand->state = STATE_PREPARED;
>> + ? ? exec_cmd = prepare_command_pool(nand, command, column, page_addr);
>> ? ? ? if (exec_cmd) {
>> - ? ? ? ? ? ? init_completion(&info->cmd_complete);
>> - ? ? ? ? ? ? pxa3xx_nand_start(info);
>> + ? ? ? ? ? ? init_completion(&nand->cmd_complete);
>> + ? ? ? ? ? ? pxa3xx_nand_start(nand);
>>
>> - ? ? ? ? ? ? ret = wait_for_completion_timeout(&info->cmd_complete,
>> + ? ? ? ? ? ? ret = wait_for_completion_timeout(&nand->cmd_complete,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? CHIP_DELAY_TIMEOUT);
>> ? ? ? ? ? ? ? if (!ret) {
>> ? ? ? ? ? ? ? ? ? ? ? printk(KERN_ERR "Wait time out!!!\n");
>> ? ? ? ? ? ? ? ? ? ? ? /* Stop State Machine for next command cycle */
>> - ? ? ? ? ? ? ? ? ? ? pxa3xx_nand_stop(info);
>> + ? ? ? ? ? ? ? ? ? ? pxa3xx_nand_stop(nand);
>> ? ? ? ? ? ? ? }
>> - ? ? ? ? ? ? info->state = STATE_IDLE;
>> ? ? ? }
>> + ? ? nand->state = STATE_IDLE;
>> ?}
>>
>> ?static void pxa3xx_nand_write_page_hwecc(struct mtd_info *mtd,
>> @@ -665,11 +695,12 @@ static int pxa3xx_nand_read_page_hwecc(struct mtd_info *mtd,
>> ? ? ? ? ? ? ? struct nand_chip *chip, uint8_t *buf, int page)
>> ?{
>> ? ? ? struct pxa3xx_nand_info *info = mtd->priv;
>> + ? ? struct pxa3xx_nand *nand = info->nand_data;
>>
>> ? ? ? chip->read_buf(mtd, buf, mtd->writesize);
>> ? ? ? chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
>>
>> - ? ? if (info->retcode == ERR_SBERR) {
>> + ? ? if (nand->retcode == ERR_SBERR) {
>> ? ? ? ? ? ? ? switch (info->use_ecc) {
>> ? ? ? ? ? ? ? case 1:
>> ? ? ? ? ? ? ? ? ? ? ? mtd->ecc_stats.corrected++;
>> @@ -678,14 +709,14 @@ static int pxa3xx_nand_read_page_hwecc(struct mtd_info *mtd,
>> ? ? ? ? ? ? ? default:
>> ? ? ? ? ? ? ? ? ? ? ? break;
>> ? ? ? ? ? ? ? }
>> - ? ? } else if (info->retcode == ERR_DBERR) {
>> + ? ? } else if (nand->retcode == ERR_DBERR) {
>> ? ? ? ? ? ? ? /*
>> ? ? ? ? ? ? ? ?* for blank page (all 0xff), HW will calculate its ECC as
>> ? ? ? ? ? ? ? ?* 0, which is different from the ECC information within
>> ? ? ? ? ? ? ? ?* OOB, ignore such double bit errors
>> ? ? ? ? ? ? ? ?*/
>> ? ? ? ? ? ? ? if (is_buf_blank(buf, mtd->writesize))
>> - ? ? ? ? ? ? ? ? ? ? info->retcode = ERR_NONE;
>> + ? ? ? ? ? ? ? ? ? ? nand->retcode = ERR_NONE;
>> ? ? ? ? ? ? ? else
>> ? ? ? ? ? ? ? ? ? ? ? mtd->ecc_stats.failed++;
>> ? ? ? }
>> @@ -696,11 +727,12 @@ static int pxa3xx_nand_read_page_hwecc(struct mtd_info *mtd,
>> ?static uint8_t pxa3xx_nand_read_byte(struct mtd_info *mtd)
>> ?{
>> ? ? ? struct pxa3xx_nand_info *info = mtd->priv;
>> + ? ? struct pxa3xx_nand *nand = info->nand_data;
>> ? ? ? char retval = 0xFF;
>>
>> - ? ? if (info->buf_start < info->buf_count)
>> + ? ? if (nand->buf_start < nand->buf_count)
>> ? ? ? ? ? ? ? /* Has just send a new command? */
>> - ? ? ? ? ? ? retval = info->data_buff[info->buf_start++];
>> + ? ? ? ? ? ? retval = nand->data_buff[nand->buf_start++];
>>
>> ? ? ? return retval;
>> ?}
>> @@ -708,11 +740,12 @@ static uint8_t pxa3xx_nand_read_byte(struct mtd_info *mtd)
>> ?static u16 pxa3xx_nand_read_word(struct mtd_info *mtd)
>> ?{
>> ? ? ? struct pxa3xx_nand_info *info = mtd->priv;
>> + ? ? struct pxa3xx_nand *nand = info->nand_data;
>> ? ? ? u16 retval = 0xFFFF;
>>
>> - ? ? if (!(info->buf_start & 0x01) && info->buf_start < info->buf_count) {
>> - ? ? ? ? ? ? retval = *((u16 *)(info->data_buff+info->buf_start));
>> - ? ? ? ? ? ? info->buf_start += 2;
>> + ? ? if (!(nand->buf_start & 0x01) && nand->buf_start < nand->buf_count) {
>> + ? ? ? ? ? ? retval = *((u16 *)(nand->data_buff+nand->buf_start));
>> + ? ? ? ? ? ? nand->buf_start += 2;
>> ? ? ? }
>> ? ? ? return retval;
>> ?}
>> @@ -720,20 +753,22 @@ static u16 pxa3xx_nand_read_word(struct mtd_info *mtd)
>> ?static void pxa3xx_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
>> ?{
>> ? ? ? struct pxa3xx_nand_info *info = mtd->priv;
>> - ? ? int real_len = min_t(size_t, len, info->buf_count - info->buf_start);
>> + ? ? struct pxa3xx_nand *nand = info->nand_data;
>> + ? ? int real_len = min_t(size_t, len, nand->buf_count - nand->buf_start);
>>
>> - ? ? memcpy(buf, info->data_buff + info->buf_start, real_len);
>> - ? ? info->buf_start += real_len;
>> + ? ? memcpy(buf, nand->data_buff + nand->buf_start, real_len);
>> + ? ? nand->buf_start += real_len;
>> ?}
>>
>> ?static void pxa3xx_nand_write_buf(struct mtd_info *mtd,
>> ? ? ? ? ? ? ? const uint8_t *buf, int len)
>> ?{
>> ? ? ? struct pxa3xx_nand_info *info = mtd->priv;
>> - ? ? int real_len = min_t(size_t, len, info->buf_count - info->buf_start);
>> + ? ? struct pxa3xx_nand *nand = info->nand_data;
>> + ? ? int real_len = min_t(size_t, len, nand->buf_count - nand->buf_start);
>>
>> - ? ? memcpy(info->data_buff + info->buf_start, buf, real_len);
>> - ? ? info->buf_start += real_len;
>> + ? ? memcpy(nand->data_buff + nand->buf_start, buf, real_len);
>> + ? ? nand->buf_start += real_len;
>> ?}
>>
>> ?static int pxa3xx_nand_verify_buf(struct mtd_info *mtd,
>> @@ -750,10 +785,11 @@ static void pxa3xx_nand_select_chip(struct mtd_info *mtd, int chip)
>> ?static int pxa3xx_nand_waitfunc(struct mtd_info *mtd, struct nand_chip *this)
>> ?{
>> ? ? ? struct pxa3xx_nand_info *info = mtd->priv;
>> + ? ? struct pxa3xx_nand *nand = info->nand_data;
>>
>> ? ? ? /* pxa3xx_nand_send_command has waited for command complete */
>> ? ? ? if (this->state == FL_WRITING || this->state == FL_ERASING) {
>> - ? ? ? ? ? ? if (info->retcode == ERR_NONE)
>> + ? ? ? ? ? ? if (nand->retcode == ERR_NONE)
>> ? ? ? ? ? ? ? ? ? ? ? return 0;
>> ? ? ? ? ? ? ? else {
>> ? ? ? ? ? ? ? ? ? ? ? /*
>> @@ -770,7 +806,8 @@ static int pxa3xx_nand_waitfunc(struct mtd_info *mtd, struct nand_chip *this)
>> ?static int pxa3xx_nand_config_flash(struct pxa3xx_nand_info *info,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? const struct pxa3xx_nand_flash *f)
>> ?{
>> - ? ? struct platform_device *pdev = info->pdev;
>> + ? ? struct pxa3xx_nand *nand = info->nand_data;
>> + ? ? struct platform_device *pdev = nand->pdev;
>> ? ? ? struct pxa3xx_nand_platform_data *pdata = pdev->dev.platform_data;
>> ? ? ? uint32_t ndcr = 0x0; /* enable all interrupts */
>>
>> @@ -804,6 +841,7 @@ static int pxa3xx_nand_config_flash(struct pxa3xx_nand_info *info,
>> ? ? ? ndcr |= NDCR_SPARE_EN; /* enable spare by default */
>>
>> ? ? ? info->reg_ndcr = ndcr;
>> + ? ? info->use_ecc = 1;
>>
>> ? ? ? pxa3xx_nand_set_timing(info, f->timing);
>> ? ? ? return 0;
>> @@ -811,15 +849,22 @@ static int pxa3xx_nand_config_flash(struct pxa3xx_nand_info *info,
>>
>> ?static int pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info)
>> ?{
>> - ? ? uint32_t ndcr = nand_readl(info, NDCR);
>> + ? ? struct pxa3xx_nand *nand = info->nand_data;
>> + ? ? uint32_t ndcr = nand_readl(nand, NDCR);
>> +
>> + ? ? if (info->chip_select > 0) {
>> + ? ? ? ? ? ? printk(KERN_ERR "We could not detect configure"
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? " if more than one cs is supported!!\n");
>> + ? ? ? ? ? ? BUG();
>
> like Daniel already noticed, may be dev_err() is enough?
Would change to dev_err();

>
>> + ? ? }
>> ? ? ? info->page_size = ndcr & NDCR_PAGE_SZ ? 2048 : 512;
>> ? ? ? /* set info fields needed to read id */
>> ? ? ? info->read_id_bytes = (info->page_size == 2048) ? 4 : 2;
>> ? ? ? info->reg_ndcr = ndcr & ~NDCR_INT_MASK;
>> ? ? ? info->cmdset = &default_cmdset;
>>
>> - ? ? info->ndtr0cs0 = nand_readl(info, NDTR0CS0);
>> - ? ? info->ndtr1cs0 = nand_readl(info, NDTR1CS0);
>> + ? ? info->ndtr0cs0 = nand_readl(nand, NDTR0CS0);
>> + ? ? info->ndtr1cs0 = nand_readl(nand, NDTR1CS0);
>>
>> ? ? ? return 0;
>> ?}
>> @@ -830,50 +875,31 @@ static int pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info)
>> ? */
>> ?#define MAX_BUFF_SIZE ? ? ? ?PAGE_SIZE
>>
>> -static int pxa3xx_nand_init_buff(struct pxa3xx_nand_info *info)
>> +static void free_cs_resource(struct pxa3xx_nand_info *info, int cs)
>> ?{
>> - ? ? struct platform_device *pdev = info->pdev;
>> - ? ? int data_desc_offset = MAX_BUFF_SIZE - sizeof(struct pxa_dma_desc);
>> -
>> - ? ? if (use_dma == 0) {
>> - ? ? ? ? ? ? info->data_buff = kmalloc(MAX_BUFF_SIZE, GFP_KERNEL);
>> - ? ? ? ? ? ? if (info->data_buff == NULL)
>> - ? ? ? ? ? ? ? ? ? ? return -ENOMEM;
>> - ? ? ? ? ? ? return 0;
>> - ? ? }
>> -
>> - ? ? info->data_buff = dma_alloc_coherent(&pdev->dev, MAX_BUFF_SIZE,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? &info->data_buff_phys, GFP_KERNEL);
>> - ? ? if (info->data_buff == NULL) {
>> - ? ? ? ? ? ? dev_err(&pdev->dev, "failed to allocate dma buffer\n");
>> - ? ? ? ? ? ? return -ENOMEM;
>> - ? ? }
>> -
>> - ? ? info->data_buff_size = MAX_BUFF_SIZE;
>> - ? ? info->data_desc = (void *)info->data_buff + data_desc_offset;
>> - ? ? info->data_desc_addr = info->data_buff_phys + data_desc_offset;
>> + ? ? struct pxa3xx_nand *nand;
>> + ? ? struct mtd_info *mtd;
>>
>> - ? ? info->data_dma_ch = pxa_request_dma("nand-data", DMA_PRIO_LOW,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? pxa3xx_nand_data_dma_irq, info);
>> - ? ? if (info->data_dma_ch < 0) {
>> - ? ? ? ? ? ? dev_err(&pdev->dev, "failed to request data dma\n");
>> - ? ? ? ? ? ? dma_free_coherent(&pdev->dev, info->data_buff_size,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? info->data_buff, info->data_buff_phys);
>> - ? ? ? ? ? ? return info->data_dma_ch;
>> - ? ? }
>> + ? ? if (!info)
>> + ? ? ? ? ? ? return;
>>
>> - ? ? return 0;
>> + ? ? nand = info->nand_data;
>> + ? ? mtd = get_mtd_by_info(info);
>> + ? ? kfree(mtd);
>> + ? ? nand->info[cs] = NULL;
>> ?}
>>
>> ?static int pxa3xx_nand_sensing(struct pxa3xx_nand_info *info)
>> ?{
>> - ? ? struct mtd_info *mtd = info->mtd;
>> + ? ? struct pxa3xx_nand *nand = info->nand_data;
>> + ? ? struct mtd_info *mtd = get_mtd_by_info(info);
>> ? ? ? struct nand_chip *chip = mtd->priv;
>>
>> ? ? ? /* use the common timing to make a try */
>> - ? ? pxa3xx_nand_config_flash(info, &builtin_flash_types[0]);
>> + ? ? if (pxa3xx_nand_config_flash(info, &builtin_flash_types[0]))
>> + ? ? ? ? ? ? return 0;
>> ? ? ? chip->cmdfunc(mtd, NAND_CMD_RESET, 0, 0);
>> - ? ? if (info->is_ready)
>> + ? ? if (nand->is_ready)
>> ? ? ? ? ? ? ? return 1;
>> ? ? ? else
>> ? ? ? ? ? ? ? return 0;
>
> I think it is time to change this function return convention to propagate
> errors and not just 0 or 1, (may be in separate patch) what do you think?

How about return 0 when sensing the READY signal, or return -ENODEV?

>
>> @@ -882,7 +908,8 @@ static int pxa3xx_nand_sensing(struct pxa3xx_nand_info *info)
>> ?static int pxa3xx_nand_scan(struct mtd_info *mtd)
>> ?{
>> ? ? ? struct pxa3xx_nand_info *info = mtd->priv;
>> - ? ? struct platform_device *pdev = info->pdev;
>> + ? ? struct pxa3xx_nand *nand = info->nand_data;
>> + ? ? struct platform_device *pdev = nand->pdev;
>> ? ? ? struct pxa3xx_nand_platform_data *pdata = pdev->dev.platform_data;
>> ? ? ? struct nand_flash_dev pxa3xx_flash_ids[2], *def = NULL;
>> ? ? ? const struct pxa3xx_nand_flash *f = NULL;
>> @@ -891,27 +918,25 @@ static int pxa3xx_nand_scan(struct mtd_info *mtd)
>> ? ? ? uint64_t chipsize;
>> ? ? ? int i, ret, num;
>>
>> + ? ? nand->chip_select = info->chip_select;
>> ? ? ? if (pdata->keep_config && !pxa3xx_nand_detect_config(info))
>> ? ? ? ? ? ? ? goto KEEP_CONFIG;
>>
>> ? ? ? ret = pxa3xx_nand_sensing(info);
>> ? ? ? if (!ret) {
>> - ? ? ? ? ? ? kfree(mtd);
>> - ? ? ? ? ? ? info->mtd = NULL;
>> - ? ? ? ? ? ? printk(KERN_INFO "There is no nand chip on cs 0!\n");
>> + ? ? ? ? ? ? free_cs_resource(info, nand->chip_select);
>> + ? ? ? ? ? ? printk(KERN_INFO "There is no nand chip on cs %d!\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? nand->chip_select);
>>
>> ? ? ? ? ? ? ? return -EINVAL;
>> ? ? ? }
>>
>> ? ? ? chip->cmdfunc(mtd, NAND_CMD_READID, 0, 0);
>> - ? ? id = *((uint16_t *)(info->data_buff));
>> + ? ? id = *((uint16_t *)(nand->data_buff));
>> ? ? ? if (id != 0)
>> ? ? ? ? ? ? ? printk(KERN_INFO "Detect a flash id %x\n", id);
>> ? ? ? else {
>> - ? ? ? ? ? ? kfree(mtd);
>> - ? ? ? ? ? ? info->mtd = NULL;
>> - ? ? ? ? ? ? printk(KERN_WARNING "Read out ID 0, potential timing set wrong!!\n");
>
> Is this warning no longer needed?
Would readd it in the next post.

>
>> -
>> + ? ? ? ? ? ? free_cs_resource(info, nand->chip_select);
>> ? ? ? ? ? ? ? return -EINVAL;
>> ? ? ? }
>>
>> @@ -928,14 +953,16 @@ static int pxa3xx_nand_scan(struct mtd_info *mtd)
>> ? ? ? }
>>
>> ? ? ? if (i >= (ARRAY_SIZE(builtin_flash_types) + pdata->num_flash - 1)) {
>> - ? ? ? ? ? ? kfree(mtd);
>> - ? ? ? ? ? ? info->mtd = NULL;
>> + ? ? ? ? ? ? free_cs_resource(info, nand->chip_select);
>> ? ? ? ? ? ? ? printk(KERN_ERR "ERROR!! flash not defined!!!\n");
>>
>> ? ? ? ? ? ? ? return -EINVAL;
>> ? ? ? }
>>
>> - ? ? pxa3xx_nand_config_flash(info, f);
>> + ? ? if (pxa3xx_nand_config_flash(info, f)) {
>> + ? ? ? ? ? ? printk(KERN_ERR "ERROR! Configure failed\n");
>> + ? ? ? ? ? ? return -EINVAL;
>> + ? ? }
>
> Although, the pxa3xx_nand_config_flash() returns only 0 or -EINVAL,
> it is better to propagate its return value.

Ok, would apply this.

>
>> ? ? ? pxa3xx_flash_ids[0].name = f->name;
>> ? ? ? pxa3xx_flash_ids[0].id = (f->chip_id >> 8) & 0xffff;
>> ? ? ? pxa3xx_flash_ids[0].pagesize = f->page_size;
>> @@ -950,13 +977,13 @@ KEEP_CONFIG:
>> ? ? ? if (nand_scan_ident(mtd, 1, def))
>> ? ? ? ? ? ? ? return -ENODEV;
>> ? ? ? /* calculate addressing information */
>> + ? ? nand->oob_buff = nand->data_buff + mtd->writesize;
>> ? ? ? info->col_addr_cycles = (mtd->writesize >= 2048) ? 2 : 1;
>> - ? ? info->oob_buff = info->data_buff + mtd->writesize;
>> ? ? ? if ((mtd->size >> chip->page_shift) > 65536)
>> ? ? ? ? ? ? ? info->row_addr_cycles = 3;
>> ? ? ? else
>> ? ? ? ? ? ? ? info->row_addr_cycles = 2;
>> - ? ? mtd->name = mtd_names[0];
>> + ? ? mtd->name = mtd_names[nand->chip_select];
>> ? ? ? chip->ecc.mode = NAND_ECC_HW;
>> ? ? ? chip->ecc.size = info->page_size;
>>
>> @@ -967,51 +994,33 @@ KEEP_CONFIG:
>> ? ? ? return nand_scan_tail(mtd);
>> ?}
>>
>> -static
>> -struct pxa3xx_nand_info *alloc_nand_resource(struct platform_device *pdev)
>> +static int alloc_nand_resource(struct platform_device *pdev)
>> ?{
>> + ? ? struct pxa3xx_nand_platform_data *pdata;
>> ? ? ? struct pxa3xx_nand_info *info;
>> ? ? ? struct nand_chip *chip;
>> ? ? ? struct mtd_info *mtd;
>> + ? ? struct pxa3xx_nand *nand;
>> ? ? ? struct resource *r;
>> - ? ? int ret, irq;
>> + ? ? int ret, irq, cs;
>> + ? ? int data_desc_offset = MAX_BUFF_SIZE - sizeof(struct pxa_dma_desc);
>>
>> - ? ? mtd = kzalloc(sizeof(struct mtd_info) + sizeof(struct pxa3xx_nand_info),
>> - ? ? ? ? ? ? ? ? ? ? GFP_KERNEL);
>> - ? ? if (!mtd) {
>> + ? ? pdata = pdev->dev.platform_data;
>> + ? ? nand = kzalloc(sizeof(struct mtd_info)
>> + ? ? ? ? ? ? ? ? ? ? + sizeof(struct pxa3xx_nand_info), GFP_KERNEL);
>> + ? ? if (!nand) {
>> ? ? ? ? ? ? ? dev_err(&pdev->dev, "failed to allocate memory\n");
>> - ? ? ? ? ? ? return NULL;
>> + ? ? ? ? ? ? return -ENOMEM;
>> ? ? ? }
>>
>> - ? ? info = (struct pxa3xx_nand_info *)(&mtd[1]);
>> - ? ? chip = (struct nand_chip *)(&mtd[1]);
>> - ? ? info->pdev = pdev;
>> - ? ? info->mtd = mtd;
>> - ? ? mtd->priv = info;
>> - ? ? mtd->owner = THIS_MODULE;
>> -
>> - ? ? chip->ecc.read_page ? ? = pxa3xx_nand_read_page_hwecc;
>> - ? ? chip->ecc.write_page ? ?= pxa3xx_nand_write_page_hwecc;
>> - ? ? chip->controller ? ? ? ?= &info->controller;
>> - ? ? chip->waitfunc ? ? ? ? ?= pxa3xx_nand_waitfunc;
>> - ? ? chip->select_chip ? ? ? = pxa3xx_nand_select_chip;
>> - ? ? chip->dev_ready ? ? ? ? = pxa3xx_nand_dev_ready;
>> - ? ? chip->cmdfunc ? ? ? ? ? = pxa3xx_nand_cmdfunc;
>> - ? ? chip->read_word ? ? ? ? = pxa3xx_nand_read_word;
>> - ? ? chip->read_byte ? ? ? ? = pxa3xx_nand_read_byte;
>> - ? ? chip->read_buf ? ? ? ? ?= pxa3xx_nand_read_buf;
>> - ? ? chip->write_buf ? ? ? ? = pxa3xx_nand_write_buf;
>> - ? ? chip->verify_buf ? ? ? ?= pxa3xx_nand_verify_buf;
>> -
>> - ? ? spin_lock_init(&chip->controller->lock);
>> - ? ? init_waitqueue_head(&chip->controller->wq);
>> - ? ? info->clk = clk_get(&pdev->dev, NULL);
>> - ? ? if (IS_ERR(info->clk)) {
>> + ? ? nand->pdev = pdev;
>> + ? ? nand->clk = clk_get(&pdev->dev, NULL);
>> + ? ? if (IS_ERR(nand->clk)) {
>> ? ? ? ? ? ? ? dev_err(&pdev->dev, "failed to get nand clock\n");
>> - ? ? ? ? ? ? ret = PTR_ERR(info->clk);
>> - ? ? ? ? ? ? goto fail_free_mtd;
>> + ? ? ? ? ? ? ret = PTR_ERR(nand->clk);
>> + ? ? ? ? ? ? goto fail_alloc;
>> ? ? ? }
>> - ? ? clk_enable(info->clk);
>> + ? ? clk_enable(nand->clk);
>>
>> ? ? ? r = platform_get_resource(pdev, IORESOURCE_DMA, 0);
>> ? ? ? if (r == NULL) {
>> @@ -1019,7 +1028,7 @@ struct pxa3xx_nand_info *alloc_nand_resource(struct platform_device *pdev)
>> ? ? ? ? ? ? ? ret = -ENXIO;
>> ? ? ? ? ? ? ? goto fail_put_clk;
>> ? ? ? }
>> - ? ? info->drcmr_dat = r->start;
>> + ? ? nand->drcmr_dat = r->start;
>>
>> ? ? ? r = platform_get_resource(pdev, IORESOURCE_DMA, 1);
>> ? ? ? if (r == NULL) {
>> @@ -1027,7 +1036,7 @@ struct pxa3xx_nand_info *alloc_nand_resource(struct platform_device *pdev)
>> ? ? ? ? ? ? ? ret = -ENXIO;
>> ? ? ? ? ? ? ? goto fail_put_clk;
>> ? ? ? }
>> - ? ? info->drcmr_cmd = r->start;
>> + ? ? nand->drcmr_cmd = r->start;
>>
>> ? ? ? irq = platform_get_irq(pdev, 0);
>> ? ? ? if (irq < 0) {
>> @@ -1050,81 +1059,147 @@ struct pxa3xx_nand_info *alloc_nand_resource(struct platform_device *pdev)
>> ? ? ? ? ? ? ? goto fail_put_clk;
>> ? ? ? }
>>
>> - ? ? info->mmio_base = ioremap(r->start, resource_size(r));
>> - ? ? if (info->mmio_base == NULL) {
>> + ? ? nand->mmio_base = ioremap(r->start, resource_size(r));
>> + ? ? if (nand->mmio_base == NULL) {
>> ? ? ? ? ? ? ? dev_err(&pdev->dev, "ioremap() failed\n");
>> ? ? ? ? ? ? ? ret = -ENODEV;
>> ? ? ? ? ? ? ? goto fail_free_res;
>> ? ? ? }
>> - ? ? info->mmio_phys = r->start;
>> -
>> - ? ? ret = pxa3xx_nand_init_buff(info);
>> - ? ? if (ret)
>> - ? ? ? ? ? ? goto fail_free_io;
>> + ? ? nand->mmio_phys = r->start;
>>
>> ? ? ? /* initialize all interrupts to be disabled */
>> - ? ? disable_int(info, NDSR_MASK);
>> + ? ? disable_int(nand, NDSR_MASK);
>>
>> ? ? ? ret = request_irq(irq, pxa3xx_nand_irq, IRQF_DISABLED,
>> - ? ? ? ? ? ? ? ? ? ? ? pdev->name, info);
>> + ? ? ? ? ? ? ? ? ? ? ? pdev->name, nand);
>> ? ? ? if (ret < 0) {
>> ? ? ? ? ? ? ? dev_err(&pdev->dev, "failed to request IRQ\n");
>> - ? ? ? ? ? ? goto fail_free_buf;
>> + ? ? ? ? ? ? ret = -ENXIO;
>> + ? ? ? ? ? ? goto fail_free_io;
>> + ? ? }
>> +
>> + ? ? platform_set_drvdata(pdev, nand);
>> +
>> + ? ? spin_lock_init(&nand->controller.lock);
>> + ? ? init_waitqueue_head(&nand->controller.wq);
>> + ? ? for (cs = 0; cs < pdata->cs_num; cs++) {
>> + ? ? ? ? ? ? mtd = kzalloc(sizeof(struct mtd_info)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? + sizeof(struct pxa3xx_nand_info),
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? GFP_KERNEL);
>> + ? ? ? ? ? ? if (!mtd) {
>> + ? ? ? ? ? ? ? ? ? ? dev_err(&pdev->dev, "failed to allocate memory\n");
>> + ? ? ? ? ? ? ? ? ? ? ret = -ENOMEM;
>> + ? ? ? ? ? ? ? ? ? ? goto fail_free_irq;
>> + ? ? ? ? ? ? }
>> +
>> + ? ? ? ? ? ? info = (struct pxa3xx_nand_info *)(&mtd[1]);
>> + ? ? ? ? ? ? info->nand_data = nand;
>> + ? ? ? ? ? ? info->chip_select = cs;
>> + ? ? ? ? ? ? mtd->priv = info;
>> + ? ? ? ? ? ? mtd->owner = THIS_MODULE;
>> + ? ? ? ? ? ? nand->info[cs] = info;
>> +
>> + ? ? ? ? ? ? chip = (struct nand_chip *)(&mtd[1]);
>> + ? ? ? ? ? ? chip->controller ? ? ? ?= &nand->controller;
>> + ? ? ? ? ? ? chip->ecc.read_page ? ? = pxa3xx_nand_read_page_hwecc;
>> + ? ? ? ? ? ? chip->ecc.write_page ? ?= pxa3xx_nand_write_page_hwecc;
>> + ? ? ? ? ? ? chip->waitfunc ? ? ? ? ?= pxa3xx_nand_waitfunc;
>> + ? ? ? ? ? ? chip->select_chip ? ? ? = pxa3xx_nand_select_chip;
>> + ? ? ? ? ? ? chip->cmdfunc ? ? ? ? ? = pxa3xx_nand_cmdfunc;
>> + ? ? ? ? ? ? chip->read_word ? ? ? ? = pxa3xx_nand_read_word;
>> + ? ? ? ? ? ? chip->read_byte ? ? ? ? = pxa3xx_nand_read_byte;
>> + ? ? ? ? ? ? chip->read_buf ? ? ? ? ?= pxa3xx_nand_read_buf;
>> + ? ? ? ? ? ? chip->write_buf ? ? ? ? = pxa3xx_nand_write_buf;
>> + ? ? ? ? ? ? chip->verify_buf ? ? ? ?= pxa3xx_nand_verify_buf;
>> ? ? ? }
>>
>> - ? ? platform_set_drvdata(pdev, info);
>> + ? ? if (use_dma == 0) {
>> + ? ? ? ? ? ? nand->data_buff = kmalloc(MAX_BUFF_SIZE, GFP_KERNEL);
>> + ? ? ? ? ? ? if (nand->data_buff == NULL) {
>> + ? ? ? ? ? ? ? ? ? ? ret = -ENOMEM;
>> + ? ? ? ? ? ? ? ? ? ? goto fail_free_buf;
>> + ? ? ? ? ? ? }
>> + ? ? ? ? ? ? goto success_exit;
>> + ? ? }
>>
>> - ? ? return info;
>> + ? ? nand->data_buff = dma_alloc_coherent(&pdev->dev, MAX_BUFF_SIZE,
>> + ? ? ? ? ? ? ? ? ? ? &nand->data_buff_phys, GFP_KERNEL);
>> + ? ? if (nand->data_buff == NULL) {
>> + ? ? ? ? ? ? dev_err(&pdev->dev, "failed to allocate dma buffer\n");
>> + ? ? ? ? ? ? ret = -ENOMEM;
>> + ? ? ? ? ? ? goto fail_free_buf;
>> + ? ? }
>>
>> + ? ? nand->data_desc = (void *)nand->data_buff + data_desc_offset;
>> + ? ? nand->data_desc_addr = nand->data_buff_phys + data_desc_offset;
>> + ? ? nand->data_dma_ch = pxa_request_dma("nand-data", DMA_PRIO_LOW,
>> + ? ? ? ? ? ? ? ? ? ? pxa3xx_nand_data_dma_irq, nand);
>> + ? ? if (nand->data_dma_ch < 0) {
>> + ? ? ? ? ? ? dev_err(&pdev->dev, "failed to request data dma\n");
>> + ? ? ? ? ? ? ret = -ENXIO;
>> + ? ? ? ? ? ? goto fail_free_dma_buf;
>> + ? ? }
>> +success_exit:
>> + ? ? return 0;
>> +
>> +fail_free_dma_buf:
>> + ? ? dma_free_writecombine(&pdev->dev, MAX_BUFF_SIZE,
>> + ? ? ? ? ? ? ? ? ? ? nand->data_buff, nand->data_buff_phys);
>> ?fail_free_buf:
>> - ? ? free_irq(irq, info);
>> - ? ? if (use_dma) {
>> - ? ? ? ? ? ? pxa_free_dma(info->data_dma_ch);
>> - ? ? ? ? ? ? dma_free_coherent(&pdev->dev, info->data_buff_size,
>> - ? ? ? ? ? ? ? ? ? ? info->data_buff, info->data_buff_phys);
>> - ? ? } else
>> - ? ? ? ? ? ? kfree(info->data_buff);
>> + ? ? for (cs = 0; cs < pdata->cs_num; cs++) {
>> + ? ? ? ? ? ? info = nand->info[cs];
>> + ? ? ? ? ? ? free_cs_resource(info, cs);
>> + ? ? }
>> +fail_free_irq:
>> + ? ? free_irq(irq, nand);
>> ?fail_free_io:
>> - ? ? iounmap(info->mmio_base);
>> + ? ? iounmap(nand->mmio_base);
>> ?fail_free_res:
>> ? ? ? release_mem_region(r->start, resource_size(r));
>> ?fail_put_clk:
>> - ? ? clk_disable(info->clk);
>> - ? ? clk_put(info->clk);
>> -fail_free_mtd:
>> - ? ? kfree(mtd);
>> - ? ? return NULL;
>> + ? ? clk_disable(nand->clk);
>> + ? ? clk_put(nand->clk);
>> +fail_alloc:
>> + ? ? kfree(nand);
>> + ? ? return ret;
>> ?}
>>
>> ?static int pxa3xx_nand_remove(struct platform_device *pdev)
>> ?{
>> - ? ? struct pxa3xx_nand_info *info = platform_get_drvdata(pdev);
>> - ? ? struct mtd_info *mtd = info->mtd;
>> + ? ? struct pxa3xx_nand *nand = platform_get_drvdata(pdev);
>> + ? ? struct pxa3xx_nand_platform_data *pdata;
>> + ? ? struct pxa3xx_nand_info *info;
>> + ? ? struct mtd_info *mtd;
>> ? ? ? struct resource *r;
>> - ? ? int irq;
>> + ? ? int irq, cs;
>>
>> ? ? ? platform_set_drvdata(pdev, NULL);
>> + ? ? pdata = pdev->dev.platform_data;
>>
>> ? ? ? irq = platform_get_irq(pdev, 0);
>> ? ? ? if (irq >= 0)
>> - ? ? ? ? ? ? free_irq(irq, info);
>> + ? ? ? ? ? ? free_irq(irq, nand);
>> ? ? ? if (use_dma) {
>> - ? ? ? ? ? ? pxa_free_dma(info->data_dma_ch);
>> - ? ? ? ? ? ? dma_free_writecombine(&pdev->dev, info->data_buff_size,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? info->data_buff, info->data_buff_phys);
>> + ? ? ? ? ? ? pxa_free_dma(nand->data_dma_ch);
>> + ? ? ? ? ? ? dma_free_writecombine(&pdev->dev, MAX_BUFF_SIZE,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? nand->data_buff, nand->data_buff_phys);
>> ? ? ? } else
>> - ? ? ? ? ? ? kfree(info->data_buff);
>> + ? ? ? ? ? ? kfree(nand->data_buff);
>>
>> - ? ? iounmap(info->mmio_base);
>> + ? ? iounmap(nand->mmio_base);
>> ? ? ? r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> ? ? ? release_mem_region(r->start, resource_size(r));
>>
>> - ? ? clk_disable(info->clk);
>> - ? ? clk_put(info->clk);
>> + ? ? clk_disable(nand->clk);
>> + ? ? clk_put(nand->clk);
>>
>> - ? ? if (mtd) {
>> + ? ? for (cs = 0; cs < pdata->cs_num; cs++) {
>> + ? ? ? ? ? ? info = nand->info[cs];
>> + ? ? ? ? ? ? if (!info)
>> + ? ? ? ? ? ? ? ? ? ? continue;
>> + ? ? ? ? ? ? mtd = get_mtd_by_info(info);
>> ? ? ? ? ? ? ? mtd_device_unregister(mtd);
>> - ? ? ? ? ? ? kfree(mtd);
>> + ? ? ? ? ? ? free_cs_resource(info, cs);
>> ? ? ? }
>> ? ? ? return 0;
>> ?}
>> @@ -1134,34 +1209,54 @@ static int pxa3xx_nand_probe(struct platform_device *pdev)
>> ? ? ? struct pxa3xx_nand_platform_data *pdata;
>> ? ? ? struct pxa3xx_nand_info *info;
>>
>> + ? ? struct pxa3xx_nand *nand;
>> + ? ? struct mtd_info *mtd;
>> + ? ? int cs, ret, nr_parts, probe_success;
>> +
>> + ? ? probe_success = 0;
>
> Can this be done along with declaration?
Ok, would apply to the next post.

>
>> ? ? ? pdata = pdev->dev.platform_data;
>> ? ? ? if (!pdata) {
>> ? ? ? ? ? ? ? dev_err(&pdev->dev, "no platform data defined\n");
>> ? ? ? ? ? ? ? return -ENODEV;
>> ? ? ? }
>>
>> - ? ? info = alloc_nand_resource(pdev);
>> - ? ? if (info == NULL)
>> + ? ? ret = alloc_nand_resource(pdev);
>> + ? ? if (ret)
>> ? ? ? ? ? ? ? return -ENOMEM;
>
> Why not propagate the return value of alloc_nand_resource()?

Would apply propagate error to the next post.

>
>>
>> - ? ? if (pxa3xx_nand_scan(info->mtd)) {
>> - ? ? ? ? ? ? dev_err(&pdev->dev, "failed to scan nand\n");
>> - ? ? ? ? ? ? pxa3xx_nand_remove(pdev);
>> - ? ? ? ? ? ? return -ENODEV;
>> - ? ? }
>> + ? ? nand = platform_get_drvdata(pdev);
>> + ? ? for (cs = 0; cs < pdata->cs_num; cs++) {
>> + ? ? ? ? ? ? info = nand->info[cs];
>> + ? ? ? ? ? ? mtd = get_mtd_by_info(info);
>> + ? ? ? ? ? ? if (pxa3xx_nand_scan(mtd)) {
>> + ? ? ? ? ? ? ? ? ? ? dev_err(&pdev->dev, "failed to scan nand\n");
>
> I think, it would be useful also to print here the return value of pxa3xx_nand_scan().
Ok, would apply to the next post

>
>> + ? ? ? ? ? ? ? ? ? ? continue;
>> + ? ? ? ? ? ? }
>> +
>> + ? ? ? ? ? ? ret = 0;
>> + ? ? ? ? ? ? nr_parts = 0;
>> + ? ? ? ? ? ? if (mtd_has_cmdlinepart()) {
>> + ? ? ? ? ? ? ? ? ? ? const char *probes[] = { "cmdlinepart", NULL };
>> + ? ? ? ? ? ? ? ? ? ? struct mtd_partition *parts;
>>
>> - ? ? if (mtd_has_cmdlinepart()) {
>> - ? ? ? ? ? ? const char *probes[] = { "cmdlinepart", NULL };
>> - ? ? ? ? ? ? struct mtd_partition *parts;
>> - ? ? ? ? ? ? int nr_parts;
>> + ? ? ? ? ? ? ? ? ? ? nr_parts = parse_mtd_partitions(mtd, probes, &parts, 0);
>>
>> - ? ? ? ? ? ? nr_parts = parse_mtd_partitions(info->mtd, probes, &parts, 0);
>> + ? ? ? ? ? ? ? ? ? ? if (nr_parts)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ret = mtd_device_register(mtd, parts, nr_parts);
>> + ? ? ? ? ? ? }
>>
>> - ? ? ? ? ? ? if (nr_parts)
>> - ? ? ? ? ? ? ? ? ? ? return mtd_device_register(info->mtd, parts, nr_parts);
>> + ? ? ? ? ? ? if (!nr_parts)
>> + ? ? ? ? ? ? ? ? ? ? ret = mtd_device_register(mtd, pdata->parts[cs],
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pdata->nr_parts[cs]);
>> + ? ? ? ? ? ? if (!ret)
>> + ? ? ? ? ? ? ? ? ? ? probe_success = 1;
>> ? ? ? }
>>
>> - ? ? return mtd_device_register(info->mtd, pdata->parts, pdata->nr_parts);
>> + ? ? if (!probe_success) {
>> + ? ? ? ? ? ? pxa3xx_nand_remove(pdev);
>> + ? ? ? ? ? ? return -ENODEV;
>> + ? ? } else
>
> You don't need this else statement

Would apply to the next post.

>
>> + ? ? ? ? ? ? return 0;
>> ?}
>>
>> ?#ifdef CONFIG_PM
>> @@ -1183,8 +1278,8 @@ static int pxa3xx_nand_resume(struct platform_device *pdev)
>> ? ? ? struct pxa3xx_nand_info *info = platform_get_drvdata(pdev);
>> ? ? ? struct mtd_info *mtd = info->mtd;
>>
>> - ? ? nand_writel(info, NDTR0CS0, info->ndtr0cs0);
>> - ? ? nand_writel(info, NDTR1CS0, info->ndtr1cs0);
>> + ? ? nand_writel(nand, NDTR0CS0, info->ndtr0cs0);
>> + ? ? nand_writel(nand, NDTR1CS0, info->ndtr1cs0);
>> ? ? ? clk_enable(info->clk);
>>
>> ? ? ? return 0;
>
> I won't be able to test the patch in the near future, sorry...
>

Thanks for comments.

Best regards,
Lei

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/