[PATCH v9 1/3] MTD : add the common code for GPMI-NAND controller driver
Marek Vasut
marek.vasut at gmail.com
Mon Aug 22 09:09:49 EDT 2011
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.
>
> >> +}
> >> +
> >> +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 ?
>
> >> + 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.
>
> >> + 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?
> >> + 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.
>
> >> + 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?
>
> >> +#define GPMI_DEBUG_INIT 0x0001
> >> +#define GPMI_DEBUG_READ 0x0002
> >> +#define GPMI_DEBUG_WRITE 0x0004
> >> +#define GPMI_DEBUG_ECC_READ 0x0008
> >> +#define GPMI_DEBUG_ECC_WRITE 0x0010
> >> +#define GPMI_DEBUG_CRAZY 0x0020
> >> +
> >> +#ifdef pr_fmt
> >> +#undef pr_fmt
> >> +#endif
> >> +
> >> +#define pr_fmt(fmt) "[ %s : %.3d ] " fmt, __func__, __LINE__
> >> +
> >> +#define logio(level) \
> >> + do { \
> >> + if (gpmi_debug& level) \
> >> + pr_info("\n"); \
> >> + } while (0)
> >
> > Do you really need this ?
>
> not really.
>
> thanks
>
> Huang Shijie
Thanks! You're a great help with this driver. Please don't take my shouting at
you personally ;-)
Cheers!
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
More information about the linux-arm-kernel
mailing list