[LINUX PATCH v8 2/2] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface
Naga Sureshkumar Relli
nagasure at xilinx.com
Tue Apr 24 03:18:24 PDT 2018
Hi Miquel and Haris,
Thanks for reviewing the patch.
> -----Original Message-----
> From: Miquel Raynal [mailto:miquel.raynal at bootlin.com]
> Sent: Sunday, April 22, 2018 8:48 PM
> To: Haris Okanovic <haris.okanovic at ni.com>
> Cc: Naga Sureshkumar Relli <nagasure at xilinx.com>; harisokn at gmail.com; linux-
> mtd at lists.infradead.org; Boris Brezillon <Boris.Brezillon at bootlin.com>
> Subject: Re: [LINUX PATCH v8 2/2] mtd: rawnand: pl353: Add basic driver for arm pl353 smc
> nand interface
>
> Hi Haris, Naga,
>
> -linux-kernel@
> +linux-mtd@
> +Boris
>
> > >>> +/**
> > >>> + * pl353_nand_cmd_function - Send command to NAND device
> > >>> + * @chip: Pointer to the NAND chip info structure
> > >>> + * @subop: Pointer to array of instructions
> > >>> + * Return: Always return zero
> > >>> + */
> > >>> +static int pl353_nand_cmd_function(struct nand_chip *chip,
> > >>> + const struct nand_subop *subop) {
> > >>> + struct mtd_info *mtd = nand_to_mtd(chip);
> > >>> + const struct nand_op_instr *instr;
> > >>> + struct pl353_nfc_op nfc_op;
> > >>
> > >> nfc_op = {}; to initialize to 0.
> > > Ok, I will update it.
> > >>
> > >>> + struct pl353_nand_info *xnand =
> > >>> + container_of(chip, struct pl353_nand_info, chip);
> > >>> + void __iomem *cmd_addr;
> > >>> + unsigned long cmd_data = 0, end_cmd_valid = 0;
> > >>> + unsigned long cmd_phase_addr, data_phase_addr, end_cmd;
> > >>> + unsigned long timeout = jiffies + PL353_NAND_DEV_BUSY_TIMEOUT;
> > >>> + u32 addrcycles = 0;
> > >>> + unsigned int op_id, len, offset;
> > >>> +
> > >>> + pl353_nfc_parse_instructions(chip, subop, &nfc_op);
> > >>> + instr = nfc_op.data_instr;
> > >>> + op_id = nfc_op.data_instr_idx;
> > >>> + len = nand_subop_get_data_len(subop, op_id);
> > >>> + offset = nand_subop_get_data_start_off(subop, op_id);
> > >>> +
> > >>> + if (nfc_op.cmnds[0] != -1) {
> > >>> + if (xnand->end_cmd_pending) {
> > >>> + /*
> > >>> + * Check for end command if this command request is
> > >>> + * same as the pending command then return
> > >>> + */
> > >>> + if (xnand->end_cmd == nfc_op.cmnds[0]) {
> > >>> + xnand->end_cmd = 0;
> > >>> + xnand->end_cmd_pending = 0;
> > >>> + return 0;
> > >>> + }
> > >>> + }
> > >>> +
> > >>> + /* Clear interrupt */
> > >>> + pl353_smc_clr_nand_int();
> > >>> + end_cmd_valid = 0;
> > >>
> > >> Space
> > > Ok, I will correct it.
> > >>
> > >>> + /* Get the command phase address */
> > >>> + if (nfc_op.cmnds[1] != -1) {
> > >>> + end_cmd_valid = 1;
> > >>> + } else {
> > >>> + if (nfc_op.cmnds[0] == NAND_CMD_READ0)
> > >>> + return 0;
> > >>> + }
> > >>> + if (nfc_op.end_cmd == NAND_CMD_NONE)
> > >>> + end_cmd = 0x0;
> > >>> + else
> > >>> + end_cmd = nfc_op.cmnds[1];
> > >>> +
> > >>> + addrcycles = nfc_op.naddrs;
> > >>> + if (nfc_op.cmnds[0] == NAND_CMD_READ0 ||
> > >>> + nfc_op.cmnds[0] == NAND_CMD_SEQIN)
> > >>> + addrcycles = xnand->row_addr_cycles +
> > >>> + xnand->col_addr_cycles;
> > >>> + else if ((nfc_op.cmnds[0] == NAND_CMD_ERASE1) ||
> > >>> + (nfc_op.cmnds[0] == NAND_CMD_ERASE2))
> > >>> + addrcycles = xnand->row_addr_cycles;
> > >>> + else
> > >>> + addrcycles = nfc_op.naddrs;
> > >>
> > >> A switch block would probably be appropriate.
> > > Yes we can use.
> > >>
> > >>> + cmd_phase_addr = (unsigned long __force)xnand->nand_base +
> > >> (
> > >>> + (addrcycles << ADDR_CYCLES_SHIFT) |
> > >>> + (end_cmd_valid << END_CMD_VALID_SHIFT)
> > >> |
> > >>> + (COMMAND_PHASE) |
> > >>> + (end_cmd << END_CMD_SHIFT) |
> > >>> + (nfc_op.cmnds[0] << START_CMD_SHIFT));
> > >>> +
> > >>> + cmd_addr = (void __iomem * __force)cmd_phase_addr;
> > >>
> > >> Space
> > > Ok, I will correc it.
> > >>
> > >>> + /* Get the data phase address */
> > >>> + end_cmd_valid = 0;
> > >>> +
> > >>> + data_phase_addr = (unsigned long __force)xnand->nand_base +
> > >> (
> > >>> + (0x0 << CLEAR_CS_SHIFT) |
> > >>> + (end_cmd_valid << 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;
> > >>
> > >> Do you really need this "__force" ?
> > > Let me check and get back to you on this.
> > >>
> > >>> + chip->IO_ADDR_W = chip->IO_ADDR_R;
> > >>> + /* Command phase AXI write */
> > >>> + /* Read & Write */
> > >>> + if (nfc_op.thirdrow) {
> > >>> + nfc_op.thirdrow = 0;
> > >>> + if (mtd->writesize > PL353_NAND_ECC_SIZE) {
> > >>> + cmd_data |= nfc_op.addrs << 16;
> > >>> + /* Another address cycle for devices > 128MiB
> > >> */
> > >>> + if (chip->chipsize > (128 << 20)) {
> > >>> + pl353_nand_write32(cmd_addr,
> > >> cmd_data);
> > >>> + cmd_data = (nfc_op.addrs >> 16);
> > >>> + }
> > >>> + }
> > >>> + } else {
> > >>> + if (nfc_op.addrs != -1) {
> > >>> + int column = nfc_op.addrs;
> > >>> + /*
> > >>> + * Change read/write column, read id etc
> > >>> + * Adjust columns for 16 bit bus width
> > >>> + */
> > >>> + if ((chip->options & NAND_BUSWIDTH_16) &&
> > >>> + ((nfc_op.cmnds[0] == NAND_CMD_READ0) ||
> > >>> + (nfc_op.cmnds[0] == NAND_CMD_SEQIN) ||
> > >>> + (nfc_op.cmnds[0] == NAND_CMD_RNDOUT) ||
> > >>> + (nfc_op.cmnds[0] == NAND_CMD_RNDIN))) {
> > >>
> > >> Alignment.
> > > Ok, I will update it.
> > >>
> > [...]
> > >>
> > >> Why?
> > > I found some errors during on_die testing, hence I added this delay.
> > > Let me check once
> >
> > Either the controller or nand itself requires a settle period after commands are delivered,
> before data may be read/written. As I understand, this ndelay() is intended to facilitate that.
>
> NAND chips expect a tCCS time (~500ns) after a RNDIN/RNDOUT. Does this controller
> cannot take care of it directly? If yes you might want to implement the ->setup_data_interface()
> hook.
>
> Otherwise if you are sure this is an hardware bug, it would probably be preferable to use Haris'
> barrier together with a comment explaining the situation.
We can take Haris fix, because we don't have any configuration timing parameter for this tCCS.
I will mix this in the next version of patch.
Thanks,
Naga Sureshkumar Relli.
>
> Thanks,
> Miquèl
>
> >
> > I discovered a small bug in this workflow a few years ago and
> > submitted the following patch to linux-xlnx:
> > https://github.com/Xilinx/linux-xlnx/pull/106
> >
> > The first byte of a sub-page read is sometimes corrupted (I.e. READ0 followed by RNDOUT
> command sequence) because (I suspect) RNDOUT commands can linger somewhere on the
> interconnect between the cpu and pl353 during the 100ns delay period, whereas other
> commands are synchronized via status change or interrupt to indicate completion. A data store
> barrier prior to ndelay() seems to address this issue. It hasn't reproduced in about 6 months of
> read-stress testing on a Zynq-7020 SoC with this change.
> >
> > Have you considered pulling this change?
> > If not, I'm curious how (if at all) you'd recommend implementing partial page reads with the
> pl353?
> >
> > Thanks,
> > Haris
> >
> > >>
> > >>> + if (nfc_op.cmnds[0] == 0xef)
> > >>> + nfc_op.wait = false;
> > >>> + if (nfc_op.wait) {
> > >>> + nfc_op.wait = false;
> > >>
> > >> You can remove this line.
> > > Yes, it is initializing to zero in cmd_function.
> > >>
> > >>> + do {
> > >>> + if (chip->dev_ready(mtd))
> > >>> + break;
> > >>> + cpu_relax();
> > >>> + } while (!time_after_eq(jiffies, timeout));
> > >>> + if (time_after_eq(jiffies, timeout)) {
> > >>> + pr_err("%s timed out\n", __func__);
> > >>> + return -ETIMEDOUT;
> > >>> + }
> > >>
> > >> Same comment about the wait.
> > > Ok, I will update it.
> > >>
> > >>> + return 0;
> > >>> + }
> > >>> + }
> > >>> +
> > >>> + if (instr == NULL)
> > >>
> > >> if (!instr)
> > > I will modify it.
> > >>
> > >>> + return 0;
> > >>> + if (instr->type == NAND_OP_DATA_IN_INSTR)
> > >>> + return pl353_nand_read_buf(chip, instr->ctx.data.buf.in, len);
> > >>> +
> > >>> + if (instr->type == NAND_OP_DATA_OUT_INSTR) {
> > >>> + if ((nfc_op.cmnds[0] == NAND_CMD_PAGEPROG) ||
> > >>> + (nfc_op.cmnds[0] == NAND_CMD_SEQIN))
> > >>> + pl353_nand_write_page_raw(mtd, chip,
> > >>> + instr->ctx.data.buf.out, 0, nfc_op.addrs);
> > >>> + else
> > >>> + pl353_nand_write_buf_l(chip, instr->ctx.data.buf.out,
> > >>> + len);
> > >>> + return 0;
> > >>> + }
> > >>> + return 0;
> > >>> +}
> > >>> +
> --
> Miquel Raynal, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering
> https://bootlin.com
More information about the linux-mtd
mailing list