Re: [LINUX PATCH v8 2/2] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface
From: Miquel Raynal
Date: Mon Mar 26 2018 - 17:54:17 EST
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