[PATCH v9 1/3] MTD : add the common code for GPMI-NAND controller driver

[shijie8@gmail.com (Huang Shijie)]

hi,

On Mon, Aug 22, 2011 at 9:09 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On Monday, August 22, 2011 06:59:11 AM Huang Shijie wrote:
>> Hi,
>>
>> thanks for your comments.
>>
>> >> +
>> >> +static inline int get_ecc_chunk_size(struct gpmi_nand_data *this)
>> >> +{
>> >> + ?/* for historical reason */
>> >> + ?return 512;
>> >
>> > Can't we just #define this? Or will there ever be something else possible
>> > here ? I thought this is the only possible behaviour on MXS.
>>
>> Please keep it here, it maybe changed in the future.
>> It ever had some code for ONFI nand, but i removed it.
>
> well if you #define it now, you can always replace the defined value with a
> function later.

ok.
>
>>
>> >> +}
>> >> +
>> >> +int common_nfc_set_geometry(struct gpmi_nand_data *this)
>> >> +{
>> >> + ?struct bch_geometry *geo =&this->bch_geometry;
>> >> + ?struct mtd_info *mtd =&this->mil.mtd;
>> >> + ?unsigned int metadata_size;
>> >> + ?unsigned int status_size;
>> >> + ?unsigned int chunk_data_size_in_bits;
>> >> + ?unsigned int chunk_ecc_size_in_bits;
>> >> + ?unsigned int chunk_total_size_in_bits;
>> >> + ?unsigned int block_mark_chunk_number;
>> >> + ?unsigned int block_mark_chunk_bit_offset;
>> >> + ?unsigned int block_mark_bit_offset;
>> >> + ?int gf_len = 13;/* use GP13 by default */
>> >> +
>> >> + ?/* We only support BCH now. */
>> >> + ?geo->ecc_algorithm = "BCH";
>> >> +
>> >> + ?/*
>> >> + ? * We always choose a metadata size of 10. Don't try to make sense of
>> >> + ? * it -- this is really only for historical compatibility.
>> >> + ? */
>> >
>> > Historical compat or you mean "the chip was designed this way, see
>> > datasheet section x.y.z"? ;-)
>>
>> Just for historical compatibility.
>> it's better to keep it as now, there is no need to change it.
>
> I'm just trying to make sense of it ... from the docs, it seems like a chip
> design thing. So this is compat with STMP37xx and 36xx ? Or even something older
> and more obscure ?
>
it  seems the rom of the chip use the value.  I will check it tomorrow.

>>
>> >> + ?geo->metadata_size_in_bytes = 10;
>> >> +
>> >> + ?/* ECC chunks */
>> >> + ?geo->ecc_chunk_size_in_bytes = get_ecc_chunk_size(this);
>
> ?[ ... ]
>
>> >> +
>> >> +/* Can we use the upper's buffer directly for DMA? */
>> >> +void prepare_data_dma(struct gpmi_nand_data *this, enum
>> >> dma_data_direction dr) +{
>> >> + ?struct mil *mil =&this->mil;
>> >> + ?struct scatterlist *sgl =&mil->data_sgl;
>> >
>> > I still see the "MIL" present -- can't we just merge the library and this
>> > ?
>>
>> the ? `mil` is just a data structure to contain the fields now.
>> Maybe I should change the name of it.
>
> Probably, I still have a feeling of this like it's the old freescale driver
> heritage and doesn't make sense now.

ok, I will change the name.

>
>>
>> >> + ?int ret;
>> >> +
>> >> + ?mil->direct_dma_map_ok = true;
>
>> >> +static void show_bch_geometry(struct bch_geometry *geo)
>> >> +{
>> >> + ?pr_info("---------------------------------------\n");
>> >> + ?pr_info(" ? ? ? BCH Geometry\n");
>> >> + ?pr_info("---------------------------------------\n");
>> >> + ?pr_info("ECC Algorithm ? ? ? ? ?: %s\n", geo->ecc_algorithm);
>> >> + ?pr_info("ECC Strength ? ? ? ? ? : %u\n", geo->ecc_strength);
>> >> + ?pr_info("Page Size in Bytes ? ? : %u\n", geo->page_size_in_bytes);
>> >> + ?pr_info("Metadata Size in Bytes : %u\n", geo->metadata_size_in_bytes);
>> >> + ?pr_info("ECC Chunk Size in Bytes: %u\n",
>> >> geo->ecc_chunk_size_in_bytes); + ? pr_info("ECC Chunk Count ? ? ? ?:
>> >> %u\n", geo->ecc_chunk_count); + ? ?pr_info("Payload Size in Bytes ?:
>> >> %u\n", geo->payload_size_in_bytes); + ? ? ?pr_info("Auxiliary Size in Bytes:
>> >> %u\n", geo->auxiliary_size_in_bytes); + ? ?pr_info("Auxiliary Status
>> >> Offset: %u\n", geo->auxiliary_status_offset); + ? ?pr_info("Block Mark
>> >> Byte Offset : %u\n", geo->block_mark_byte_offset); + ? ? ? pr_info("Block
>> >> Mark Bit Offset ?: %u\n", geo->block_mark_bit_offset); +}
>> >
>> > We don't need this.
>>
>> I just use it for debug.
>> Why do not need it? :)
>> I think it's useful to debug.
>
> I want to use it, not debug it. I don't want to have it in dmesg. pr_info() is
> really unsuitable. Remove it, use pr_debug(), #define it in
> MXS_NAND_VERBOSE_DEBUG, which will be undefined at the begining of the file by
> default (probably the best approach). Someone who wants to debug this thing will
> just enable it.
>
>>
>> >> +
>> >> +int start_dma_without_bch_irq(struct gpmi_nand_data *this,
>> >> + ? ? ? ? ? ? ? ? ? ? ? ? ?struct dma_async_tx_descriptor *desc)
>> >> +{
>> >> + ?struct completion *dma_c =&this->dma_done;
>> >> + ?int err;
>
> [...]
>
>> >> +
>> >> + ?if ((mil->current_chip< ?0)&& ?(chip>= 0))
>
> btw. is the indent here OK?

I checked with the script ./script/checkpatch.
there is no error.

It's ok.

>
>> >> + ? ? ? ? ?gpmi_begin(this);
>> >> + ?else if ((mil->current_chip>= 0)&& ?(chip< ?0))
>> >> + ? ? ? ? ?gpmi_end(this);
>> >> + ?else
>> >> + ? ? ? ? ?;
>> >
>> > Do you need this else branch at all?
>>
>> It needs a warning here.
>>
>> >> +
>> >> + ?mil->current_chip = chip;
>> >> +}
>> >> +
>> >> +static void gpmi_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
>> >> +{
>> >> + ?struct nand_chip *nand = mtd->priv;
>> >> + ?struct gpmi_nand_data *this = nand->priv;
>> >> + ?struct mil *mil =&this->mil;
>> >> +
>> >> + ?logio(GPMI_DEBUG_READ);
>> >> + ?/* save the info in mil{} for future */
>> >> + ?mil->upper_buf ?= buf;
>> >> + ?mil->upper_len ?= len;
>> >> +
>> >> + ?gpmi_read_data(this);
>> >> +}
>> >> +
>> >> +static void gpmi_write_buf(struct mtd_info *mtd, const uint8_t *buf,
>> >> int len) +{
>> >> + ?struct nand_chip *nand = mtd->priv;
>> >> + ?struct gpmi_nand_data *this = nand->priv;
>> >> + ?struct mil *mil =&this->mil;
>> >> +
>> >> + ?logio(GPMI_DEBUG_WRITE);
>> >> + ?/* save the info in mil{} for future */
>> >> + ?mil->upper_buf ?= (uint8_t *)buf;
>> >> + ?mil->upper_len ?= len;
>> >> +
>> >> + ?gpmi_send_data(this);
>> >> +}
>> >> +
>> >> +static uint8_t gpmi_read_byte(struct mtd_info *mtd)
>> >> +{
>> >> + ?struct nand_chip *nand = mtd->priv;
>> >> + ?struct gpmi_nand_data *this = nand->priv;
>> >> + ?struct mil *mil =&this->mil;
>> >> + ?uint8_t *buf = mil->data_buffer_dma;
>> >> +
>> >> + ?gpmi_read_buf(mtd, buf, 1);
>> >> + ?return buf[0];
>> >> +}
>> >> +
>> >>
>> >> +static int gpmi_block_markbad(struct mtd_info *mtd, loff_t ofs)
>> >> +{
>> >> + ?struct nand_chip *nand = mtd->priv;
>> >> + ?struct gpmi_nand_data *this = nand->priv;
>> >> + ?int block, ret = 0;
>> >> +
>> >> + ?/* Get block number */
>> >> + ?block = (int)(ofs>> ?nand->bbt_erase_shift);
>> >> + ?if (nand->bbt)
>> >> + ? ? ? ? ?nand->bbt[block>> ?2] |= 0x01<< ?((block& ?0x03)<< ?1);
>> >> +
>> >> + ?/* Do we have a flash based bad block table ? */
>> >> + ?if (nand->options& ?NAND_USE_FLASH_BBT)
>> >> + ? ? ? ? ?ret = nand_update_bbt(mtd, ofs);
>> >
>> > if (stuff)
>> >
>> > ? ? return nand_update_bbt();
>>
>> I can not return it here, I need to update the ecc_stats too.
>
> You're right.
>
>>
>> > stuff from else branch
>> > .
>> > .
>> > .
>> >
>> > Besides, please don't declare variables in the middle of code.
>>
>> why?
>> it's no harm to declare the variables in the {}.
>
> And to find out what kind of variable it is, you can't just jump at the begining
> of the function, you need to navigate through the code ... that's bad and
> additional work for other people.
>
thanks, got it.

>>
>> >> + ?else {
>> >> + ? ? ? ? ?struct mil *mil =&this->mil;
>> >> + ? ? ? ? ?uint8_t *block_mark;
>> >> + ? ? ? ? ?int column, page, status, chipnr;
>> >> +
>> >> + ? ? ? ? ?chipnr = (int)(ofs>> ?nand->chip_shift);
>> >> + ? ? ? ? ?nand->select_chip(mtd, chipnr);
>> >> +
>> >> + ? ? ? ? ?column = this->swap_block_mark ? mtd->writesize : 0;
>> >> +
>> >> + ? ? ? ? ?/* Write the block mark. */
>> >> + ? ? ? ? ?block_mark = mil->data_buffer_dma;
>> >> + ? ? ? ? ?block_mark[0] = 0; /* bad block marker */
>> >> +
>> >> + ? ? ? ? ?/* Shift to get page */
>> >> + ? ? ? ? ?page = (int)(ofs>> ?nand->page_shift);
>> >> +
>> >> + ? ? ? ? ?nand->cmdfunc(mtd, NAND_CMD_SEQIN, column, page);
>> >> + ? ? ? ? ?nand->write_buf(mtd, block_mark, 1);
>> >> + ? ? ? ? ?nand->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
>> >> +
>> >> + ? ? ? ? ?status = nand->waitfunc(mtd, nand);
>> >> + ? ? ? ? ?if (status& ?NAND_STATUS_FAIL)
>> >> + ? ? ? ? ? ? ? ? ?ret = -EIO;
>> >> +
>> >> + ? ? ? ? ?nand->select_chip(mtd, -1);
>> >> + ?}
>> >> + ?if (!ret)
>> >> + ? ? ? ? ?mtd->ecc_stats.badblocks++;
>> >> +
>> >> + ?return ret;
>> >> +}
>
> [...]
>
>> >> addr_t auxiliary);
>> >> +
>> >> +/* ONFI or TOGGLE nand */
>> >> +bool is_ddr_nand(struct gpmi_nand_data *);
>> >> +
>> >> +/* for log */
>> >> +extern int gpmi_debug;
>> >
>> > Why this extern ?
>>
>> this header can be included by gpmi-nand.c and gpmi-lib.c.
>
> This is a peculiar one ... can't you -- for example -- hide this into driver
> data?
It's a parameter of the driver.
I use it to show different log.
I think it can not be hide into driver data :(

thanks

Huang Shijie