[LINUX PATCH v8 2/2] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface
Naga Sureshkumar Relli
nagasure at xilinx.com
Mon Mar 26 21:02:42 PDT 2018
Hi Miquel,
Thanks for clarifying the queries.
I will send next version by addressing all the comments given.
Thanks,
Naga Sureshkumar Relli.
> -----Original Message-----
> From: Miquel Raynal [mailto:miquel.raynal at bootlin.com]
> Sent: Tuesday, March 27, 2018 3:24 AM
> To: Naga Sureshkumar Relli <nagasure at xilinx.com>
> Cc: nagasureshkumarrelli at gmail.com; boris.brezillon at bootlin.com;
> richard at nod.at; dwmw2 at infradead.org; computersforpeace at gmail.com;
> marek.vasut at gmail.com; cyrille.pitchen at wedev4u.fr; linux-
> mtd at lists.infradead.org; linux-kernel at vger.kernel.org; Michal Simek
> <michals at xilinx.com>; Punnaiah Choudary Kalluri <punnaia at xilinx.com>
> Subject: Re: [LINUX PATCH v8 2/2] mtd: rawnand: pl353: Add basic driver for
> arm pl353 smc nand interface
>
> Hi Naga,
>
> > > > +/**
> > > > + * pl353_nand_read_buf_l - read chip data into buffer
> > > > + * @chip: Pointer to the NAND chip info structure
> > > > + * @in: Pointer to the buffer to store read data
> > > > + * @len: Number of bytes to read
> > > > + * Return: Always return zero
> > > > + */
> > > > +static int pl353_nand_read_buf_l(struct nand_chip *chip,
> > > > + uint8_t *in,
> > > > + unsigned int len)
> > > > +{
> > > > + int i;
> > > > + unsigned long *ptr = (unsigned long *)in;
> > > > +
> > > > + len >>= 2;
> > >
> > > Can you please let the compiler optimize things? I don't find this
> > > very readable, I would prefer a division here. And if this division
> > > by 4 is related to the size of *ptr, please use the sizeof() macro.
> Otherwise please document this value.
> > At a time, we are reading 4bytes. Hence >> 2.
> > I didn't get your point.
> > Are you saying instead of shifting, just use divide by 4?
>
> I do.
>
> >
> > >
> > > > + for (i = 0; i < len; i++)
> > > > + ptr[i] = readl(chip->IO_ADDR_R);
> > >
> > > Space
> > Ok, I will update it
> > >
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void pl353_nand_write_buf_l(struct nand_chip *chip, const
> > > > +uint8_t
> > > *buf,
> > > > + int len)
> > > > +{
> > > > + int i;
> > > > + unsigned long *ptr = (unsigned long *)buf;
> > > > +
> > > > + for (i = 0; i < len; i++)
> > > > + writeb(ptr[i], chip->IO_ADDR_W);
> > >
> > > Here you use writeb (as opposed to readl previously). Then, I guess
> > > you can also read byte per byte. If so, you can drop both helpers
> > > and let the core use its defaults ones: nand_read/write_buf().
> > May be the function name I have written wrongly.
> > When using writel, it should be nand_write_buf_l.
> > But the thing is, when using exec_op, core is not calling
> > chip->read_byte(), hence I added Byte reading.
>
> Well, the point of using ->exec_op() is to forget about these hooks.
> ->exec_op() will ask you to read one byte if needed. You should forget
> about ->read/write_byte/word/buf() hooks and delete them entirely.
>
> > >
> > > Same for the next functions. Plus, if you don't use them inside
> > > ->exec_op() implementation, they have to be removed anyway.
> > The name of the function should change to buf_l, to do 4byte writes.
> > The name is creating confusion.
> > >
> > > > +}
> > > > +
> > > > +/**
> > > > + * pl353_nand_write_buf - write buffer to chip
> > > > + * @mtd: Pointer to the mtd info structure
> > > > + * @buf: Pointer to the buffer to store read data
> > > > + * @len: Number of bytes to write
> > > > + */
> > > > +static void pl353_nand_write_buf(struct mtd_info *mtd, const uint8_t
> *buf,
> > > > + int len)
> > > > +{
> > > > + int i;
> > > > + struct nand_chip *chip = mtd_to_nand(mtd);
> > > > + unsigned long *ptr = (unsigned long *)buf;
> > > > +
> > > > + len >>= 2;
> > > > +
> > > > + for (i = 0; i < len; i++)
> > > > + writel(ptr[i], chip->IO_ADDR_W); }
> > > > +
> > > > +/**
> > > > + * pl353_nand_read_buf - read chip data into buffer
> > > > + * @chip: Pointer to the NAND chip info structure
> > > > + * @in: Pointer to the buffer to store read data
> > > > + * @len: Number of bytes to read
> > > > + * Return: 0 on success or error value on failure
> > > > + */
> > > > +static int pl353_nand_read_buf(struct nand_chip *chip,
> > > > + uint8_t *in,
> > > > + unsigned int len)
> > > > +{
> > > > + int i;
> > > > +
> > > > + for (i = 0; i < len; i++)
> > > > + in[i] = readb(chip->IO_ADDR_R);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +/**
> > > > + * pl353_nand_calculate_hwecc - Calculate Hardware ECC
> > > > + * @mtd: Pointer to the mtd_info structure
> > > > + * @data: Pointer to the page data
> > > > + * @ecc_code: Pointer to the ECC buffer where ECC data needs to be
> > > stored
> > >
> > > You store ECC in a variable called "code", can you please make it
> consistent?
> > Miquel, I am not using any variable called "code"
>
> I see "ecc_code", and ECC already stands for "Error Correcting Code".
>
> > >
> > > > + *
> > > > + * This function retrieves the Hardware ECC data from the
> > > > +controller and returns
> > > > + * ECC data back to the MTD subsystem.
> > > > + *
> > > > + * Return: 0 on success or error value on failure
> > > > + */
> > > > +static int pl353_nand_calculate_hwecc(struct mtd_info *mtd,
> > > > + const u8 *data, u8 *ecc_code) {
> > > > + u32 ecc_value, ecc_status;
> > > > + u8 ecc_reg, ecc_byte;
> > > > + unsigned long timeout = jiffies + PL353_NAND_ECC_BUSY_TIMEOUT;
> > > > + /* Wait till the ECC operation is complete or timeout */
> > > > + do {
> > > > + if (pl353_smc_ecc_is_busy())
> > >
> > > Where does this function come from?
> > The pl353 SMC has memory controller driver and this NAND driver is using
> those APIs.
> > I sent patches to add the memory controller driver for pl353.
> > https://www.spinics.net/lists/kernel/msg2748832.html
> > https://www.spinics.net/lists/kernel/msg2748834.html
> > https://www.spinics.net/lists/kernel/msg2748840.html
> >
>
> ok, please add a reference in your cover letter to the functions that are not
> yet merged and you would use in this series.
>
>
> > > > +
> > > > + cmd_phase_addr = (unsigned long __force)xnand->nand_base + (
> > > > + (((xnand->row_addr_cycles) + (xnand-
> > > >col_addr_cycles))
> > > > + << ADDR_CYCLES_SHIFT) |
> > > > + (end_cmd_valid << END_CMD_VALID_SHIFT)
> > > |
> > > > + (COMMAND_PHASE) |
> > > > + (end_cmd << END_CMD_SHIFT)
> |
> > >
> > > Please don't align the '|'
> > You mean, tabbing?
>
> Yes
>
> > >
> > > > + (start_cmd << START_CMD_SHIFT));
> > > > + cmd_addr = (void __iomem * __force)cmd_phase_addr;
> > > > +
> > > > + /* Get the data phase address */
> > > > + data_phase_addr = (unsigned long __force)xnand->nand_base + (
> > > > + (0x0 << CLEAR_CS_SHIFT) |
> > > > + (0 << END_CMD_VALID_SHIFT) |
> > > > + (DATA_PHASE)
> |
> > > > + (end_cmd << END_CMD_SHIFT)
> |
> > > > + (0x0 << ECC_LAST_SHIFT));
> > > > +
> > > > + chip->IO_ADDR_R = (void __iomem * __force)data_phase_addr;
> > > > + chip->IO_ADDR_W = chip->IO_ADDR_R;
> > > > + if (chip->options & NAND_BUSWIDTH_16)
> > > > + column >>= 1;
> > >
> > > / 2
> > >
> > > > + cmd_data = column;
> > > > + if (mtd->writesize > PL353_NAND_ECC_SIZE) {
> > > > + cmd_data |= page << 16;
> > > > + /* Another address cycle for devices > 128MiB */
> > > > + if (chip->chipsize > (128 << 20)) {
> > >
> > > Now there is a flag for that in the core, called NAND_ROW_ADDR_3.
> > I will check and update.
> > >
> > > > + pl353_nand_write32(cmd_addr, cmd_data);
> > > > + cmd_data = (page >> 16);
> > > > + }
> > > > + } else {
> > > > + cmd_data |= page << 8;
> > > > + }
> > >
> > > Space
> > Ok, I will update.
> > >
> > > > + pl353_nand_write32(cmd_addr, cmd_data); }
> > > > +
> > > > +/**
> > > > + * pl353_nand_read_oob - [REPLACEABLE] the most common OOB
> data
> > > > +read
> > > function
> > > > + * @mtd: Pointer to the mtd info structure
> > > > + * @chip: Pointer to the NAND chip info structure
> > > > + * @page: Page number to read
> > > > + *
> > > > + * Return: Always return zero
> > > > + */
> > > > +static int pl353_nand_read_oob(struct mtd_info *mtd, struct
> > > > +nand_chip
> > > *chip,
> > > > + int page)
> > > > +{
> > > > +
> > > > + unsigned long data_phase_addr;
> > > > + uint8_t *p;
> > > > +
> > > > + chip->pagebuf = -1;
> > > > + if (mtd->writesize < PL353_NAND_ECC_SIZE)
> > > > + return 0;
> > > > +
> > > > + pl353_prepare_cmd(mtd, chip, page, mtd->writesize,
> > > NAND_CMD_READ0,
> > > > + NAND_CMD_READSTART, 1);
> > >
> > > Alignment
> > Are you running any script apart from checkpatch?
> > Any way I will correct it.
>
> All my comments have been made "manually" without tool but you should
> definitively run checkpatch.pl --strict on all your patches before sending
> them.
>
>
> > > > +
> > > > + return (status & NAND_STATUS_FAIL) ? -EIO : 0; }
> > > > +
> > > > +/**
> > > > + * pl353_nand_read_page_raw - [Intern] read raw page data without
> ecc
> > > > + * @mtd: Pointer to the mtd info structure
> > > > + * @chip: Pointer to the NAND chip info structure
> > > > + * @buf: Pointer to the data buffer
> > > > + * @oob_required: Caller requires OOB data read to chip-
> >oob_poi
> > > > + * @page: Page number to read
> > > > + *
> > > > + * Return: Always return zero
> > > > + */
> > > > +static int pl353_nand_read_page_raw(struct mtd_info *mtd,
> > > > + struct nand_chip *chip,
> > > > + uint8_t *buf, int oob_required, int page)
> > >
> > > Do you really need raw accessors?
> > Yes, when using on-die ecc, this function is getting called.
> > i.e. nand_micron.c is calling nand_set_features_op, with
> > DATA_OUT_INSTR, and there we are using this.
>
> I don't see any link between doing a nad_set_features_op and calling a raw
> accessor. It's almost like the ->read_byte() hook, if there is nothing specific to
> implement, just forget about it, ->exec_op() is probably enough.
>
> > > > +/**
> > > > + * pl353_nand_select_chip - Select the flash device
> > > > + * @mtd: Pointer to the mtd info structure
> > > > + * @chip: Pointer to the NAND chip info structure
> > > > + *
> > > > + * This function is empty as the NAND controller handles chip
> > > > +select line
> > > > + * internally based on the chip address passed in command and data
> phase.
> > > > + */
> > > > +static void pl353_nand_select_chip(struct mtd_info *mtd, int
> > > > +chip) { }
> > > > +
> > > > +/* NAND framework ->exec_op() hooks and related helpers */ static
> > > > +void pl353_nfc_parse_instructions(struct nand_chip *chip,
> > > > + const struct nand_subop *subop,
> > > > + struct pl353_nfc_op *nfc_op) {
> > > > + const struct nand_op_instr *instr = NULL;
> > > > + unsigned int op_id, offset, naddrs;
> > > > + int i;
> > > > + const u8 *addrs;
> > > > +
> > > > + memset(nfc_op, 0, sizeof(struct pl353_nfc_op));
> > > > + for (op_id = 0; op_id < subop->ninstrs; op_id++) {
> > > > +
> > >
> > > What is this for-loop for? I don't get it as you break the switch in every
> case?
> > I think, breaking switch case only not for loop.
>
> My bad, ok.
>
> > >
> > > > + nfc_op->len = nand_subop_get_data_len(subop, op_id);
> > > > +
> > > > + instr = &subop->instrs[op_id];
> > > > + if (subop->ninstrs == 1)
> > > > + nfc_op->cmnds[0] = -1;
> > > > + switch (instr->type) {
> > > > + case NAND_OP_CMD_INSTR:
> > > > + nfc_op->type = NAND_OP_CMD_INSTR;
> > > > + nfc_op->end_cmd = op_id - 1;
> > > > + if (op_id)
> > >
> > > You should put { } on the if also if the else statement needs braces.
> > Ok, but I didn't see any warning from checkpatch.
>
> Maybe with the --strict option?
> Otherwise this is clearly stated in the kernel coding style documentation.
>
>
> > > > +static const struct nand_op_parser pl353_nfc_op_parser =
> > > NAND_OP_PARSER(
> > > > + NAND_OP_PARSER_PATTERN(
> > > > + pl353_nand_cmd_function,
> > > > + NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > > > + NAND_OP_PARSER_PAT_ADDR_ELEM(false, 7),
> > > > + NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, 8)),
> > > > + NAND_OP_PARSER_PATTERN(
> > > > + pl353_nand_cmd_function,
> > > > + NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > > > + NAND_OP_PARSER_PAT_ADDR_ELEM(false, 7),
> > > > + NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > > > + NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)),
> > > > + NAND_OP_PARSER_PATTERN(
> > > > + pl353_nand_cmd_function,
> > > > + NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > > > + NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, 8)),
> > > > + NAND_OP_PARSER_PATTERN(
> > > > + pl353_nand_cmd_function,
> > > > + NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > > > + NAND_OP_PARSER_PAT_ADDR_ELEM(false, 7),
> > > > + NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)),
> > > > + NAND_OP_PARSER_PATTERN(
> > > > + pl353_nand_cmd_function,
> > > > + NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > > > + NAND_OP_PARSER_PAT_ADDR_ELEM(false, 8),
> > > > + NAND_OP_PARSER_PAT_DATA_OUT_ELEM(false, 2048),
> > > > + NAND_OP_PARSER_PAT_CMD_ELEM(true),
> > > > + NAND_OP_PARSER_PAT_WAITRDY_ELEM(true)),
> > > > + NAND_OP_PARSER_PATTERN(
> > > > + pl353_nand_cmd_function,
> > > > + NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > > > + NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)),
> > > > + NAND_OP_PARSER_PATTERN(
> > > > + pl353_nand_cmd_function,
> > > > + NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, 2048)),
> > > > + NAND_OP_PARSER_PATTERN(
> > > > + pl353_nand_cmd_function,
> > > > + NAND_OP_PARSER_PAT_CMD_ELEM(false)),
> > >
> > > I am pretty sure you can factorize all these patterns now. Use the
> "optional"
> > > parameter for that.
> > Can you explain little bit? I didn't get.
>
> All the patterns refer to the same function. This is fine.
> But maybe you can factorize the parents using the "optional" parameter.
> For example, if you have
> * CMD + ADDR + DATA_IN
> * CMD + DATA_IN
> Maybe you can just state:
> * CMD + [ADDR] + DATA_IN
> With "[]" meaning the element is optional.
>
>
> Thanks for addressing these comments.
>
> Regards,
> Miquèl
>
> --
> Miquel Raynal, Bootlin (formerly Free Electrons) Embedded Linux and Kernel
> engineering https://bootlin.com
More information about the linux-mtd
mailing list