On Thu, 7 May 2020 13:00:33 +0200 Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote: > +/** > + * struct arasan_nfc - Defines the Arasan NAND flash controller driver instance > + * @dev: Pointer to the device structure > + * @base: Remapped register area > + * @controller_clk: Pointer to the system clock > + * @bus_clk: Pointer to the flash clock > + * @controller: Base controller structure > + * @chips: List of all NAND chips attached to the controller > + * @assigned_cs: Bitmask describing already assigned CS lines > + * @cur_clk: Current clock rate > + * @bf: Array of bitflips read in each ECC step > + */ > +struct arasan_nfc { > + struct device *dev; > + void __iomem *base; > + struct clk *controller_clk; > + struct clk *bus_clk; > + struct nand_controller controller; > + struct list_head chips; > + unsigned long assigned_cs; > + unsigned int cur_clk; > + u8 *bf; Looks like this field is never used. > +}; > + > +static struct anand *to_anand(struct nand_chip *nand) > +{ > + return container_of(nand, struct anand, chip); > +} > + > +static struct arasan_nfc *to_anfc(struct nand_controller *ctrl) > +{ > + return container_of(ctrl, struct arasan_nfc, controller); > +} > + > +static void anfc_disable_int(struct arasan_nfc *nfc, u32 mask) > +{ > + mask &= ~EVENT_MASK; > + mask &= EVENT_MASK; > + writel_relaxed(mask, nfc->base + INTR_SIG_EN_REG); > +} > + > +static int anfc_wait_for_event(struct arasan_nfc *nfc, unsigned int event) > +{ > + u32 val; > + int ret; > + > + ret = readl_relaxed_poll_timeout(nfc->base + INTR_STS_REG, val, > + val & event, 0, > + ANFC_DFLT_TIMEOUT_US); > + if (ret) { > + dev_err(nfc->dev, "Timeout waiting for event 0x%x\n", event); > + return -ETIMEDOUT; > + } > + > + writel_relaxed(event, nfc->base + INTR_STS_REG); > + > + return 0; > +} > + > +static int anfc_wait_for_rb(struct arasan_nfc *nfc, struct nand_chip *chip, > + unsigned int timeout_ms) > +{ > + struct anand *anand = to_anand(chip); > + u32 val; > + int ret; > + > + ret = readl_relaxed_poll_timeout(nfc->base + READY_STS_REG, val, > + val & BIT(anand->rb), > + 0, timeout_ms * 1000); You don't have an interrupt on R/B transition? If can, that would probably be better to use interrupts than polling the status reg. > + if (ret) { > + dev_err(nfc->dev, "Timeout waiting for R/B 0x%x\n", > + readl_relaxed(nfc->base + READY_STS_REG)); > + return -ETIMEDOUT; > + } > + > + return 0; > +} > + > +static void anfc_trigger_op(struct arasan_nfc *nfc, struct anfc_op *nfc_op) > +{ > + writel_relaxed(nfc_op->pkt_reg, nfc->base + PKT_REG); > + writel_relaxed(nfc_op->addr1_reg, nfc->base + MEM_ADDR1_REG); > + writel_relaxed(nfc_op->addr2_reg, nfc->base + MEM_ADDR2_REG); > + writel_relaxed(nfc_op->cmd_reg, nfc->base + CMD_REG); > + writel_relaxed(nfc_op->prog_reg, nfc->base + PROG_REG); > +} > + > +static int anfc_len_to_steps(struct nand_chip *chip, unsigned int len) > +{ > + unsigned int steps = 1, pktsize = len; > + > + while (pktsize > ANFC_MAX_PKT_SIZE) { > + steps *= 2; > + pktsize = DIV_ROUND_UP(len, steps); Hm, I'm not sure that's right. What happens if steps * pksize is bigger than the requested number of DATA cycles? Not all operations accept to have more data read/written than requested. I'd rather have something that does: static void anfc_len_to_steps(struct nand_chip *chip, unsigned int len, unsigned int *pktsize, unsigned int *npkts) { if (len <= ANFC_MAX_PKT_SIZE) { *pktsize = len; *npkts = 1; return; } for (*npkts = 2; *npkts < ANFC_MAX_NUM_PKTS; *npkts *= 2) { *pktsize = len / *npkts; if (*pktsize <= ANFC_MAX_PKT_SIZE) break; } } And then, from the call size, you trigger new DATA_OUT/IN operation if len > pktsize * npkts. > + } > + > + return steps; I guess you have a limit on steps. It's probably worth checking that steps is in bounds. > +} > + > +/* NAND framework ->exec_op() hooks and related helpers */ > +static void anfc_parse_instructions(struct nand_chip *chip, > + const struct nand_subop *subop, > + struct anfc_op *nfc_op) > +{ > + struct anand *anand = to_anand(chip); > + const struct nand_op_instr *instr = NULL; > + bool first_cmd = true; > + unsigned int op_id; > + int i; > + > + memset(nfc_op, 0, sizeof(*nfc_op)); > + nfc_op->addr2_reg = ADDR2_CS(anand->cs); > + nfc_op->cmd_reg = CMD_PAGE_SIZE(anand->page_sz); > + > + for (op_id = 0; op_id < subop->ninstrs; op_id++) { > + unsigned int offset, naddrs, pktsize; > + const u8 *addrs; > + u8 *buf; > + > + instr = &subop->instrs[op_id]; > + > + switch (instr->type) { > + case NAND_OP_CMD_INSTR: > + if (first_cmd) > + nfc_op->cmd_reg |= CMD_1(instr->ctx.cmd.opcode); > + else > + nfc_op->cmd_reg |= CMD_2(instr->ctx.cmd.opcode); > + > + first_cmd = false; > + break; > + > + case NAND_OP_ADDR_INSTR: > + offset = nand_subop_get_addr_start_off(subop, op_id); > + naddrs = nand_subop_get_num_addr_cyc(subop, op_id); > + addrs = &instr->ctx.addr.addrs[offset]; > + nfc_op->cmd_reg |= CMD_NADDRS(naddrs); > + > + for (i = 0; i < min(ANFC_MAX_ADDR_CYC, naddrs); i++) { > + if (i < 4) > + nfc_op->addr1_reg |= (u32)addrs[i] << i * 8; > + else > + nfc_op->addr2_reg |= addrs[i]; > + } > + > + break; > + case NAND_OP_DATA_IN_INSTR: > + nfc_op->read = true; > + fallthrough; > + case NAND_OP_DATA_OUT_INSTR: > + offset = nand_subop_get_data_start_off(subop, op_id); > + buf = instr->ctx.data.buf.in; > + nfc_op->buf = &buf[offset]; > + nfc_op->len = nand_subop_get_data_len(subop, op_id); > + nfc_op->steps = anfc_len_to_steps(chip, nfc_op->len); > + pktsize = DIV_ROUND_UP(nfc_op->len, nfc_op->steps); > + nfc_op->pkt_reg |= PKT_SIZE(round_up(pktsize, 4)) | Hm, pktsize has to be aligned on 4? Again, that's not great since you adjust the size without letting the core know you did that. > + PKT_STEPS(nfc_op->steps); > + break; > + case NAND_OP_WAITRDY_INSTR: > + nfc_op->rdy_timeout_ms = instr->ctx.waitrdy.timeout_ms; > + break; > + } > + } > +} > + > +static int anfc_rw_pio_op(struct arasan_nfc *nfc, struct anfc_op *nfc_op) > +{ > + unsigned int dwords = (nfc_op->len / 4) / nfc_op->steps; > + unsigned int last_len = nfc_op->len % 4; > + unsigned int offset, dir; > + u8 *buf = nfc_op->buf; > + int ret, i; > + > + for (i = 0; i < nfc_op->steps; i++) { > + dir = nfc_op->read ? READ_READY : WRITE_READY; > + ret = anfc_wait_for_event(nfc, dir); > + if (ret) { > + dev_err(nfc->dev, "PIO %s ready signal not received\n", > + nfc_op->read ? "Read" : "Write"); > + return ret; > + } > + > + offset = i * (dwords * 4); > + if (nfc_op->read) > + ioread32_rep(nfc->base + DATA_PORT_REG, &buf[offset], > + dwords); > + else > + iowrite32_rep(nfc->base + DATA_PORT_REG, &buf[offset], > + dwords); > + } > + > + if (last_len) { > + u32 remainder; > + > + offset = nfc_op->len - last_len; > + > + if (nfc_op->read) { > + remainder = readl_relaxed(nfc->base + DATA_PORT_REG); > + memcpy(&buf[offset], &remainder, last_len); > + } else { > + memcpy(&remainder, &buf[offset], last_len); > + writel_relaxed(remainder, nfc->base + DATA_PORT_REG); > + } > + } > + > + return anfc_wait_for_event(nfc, XFER_COMPLETE); > +} > + > +static int anfc_misc_data_type_exec(struct nand_chip *chip, > + const struct nand_subop *subop, > + u32 prog_reg) > +{ > + struct arasan_nfc *nfc = to_anfc(chip->controller); > + struct anfc_op nfc_op = {}; > + int ret; > + > + anfc_parse_instructions(chip, subop, &nfc_op); > + nfc_op.prog_reg = prog_reg; > + anfc_trigger_op(nfc, &nfc_op); > + > + if (nfc_op.rdy_timeout_ms) { > + ret = anfc_wait_for_rb(nfc, chip, nfc_op.rdy_timeout_ms); > + if (ret) > + return ret; > + } > + > + return anfc_rw_pio_op(nfc, &nfc_op); > +} > + > +static int anfc_param_read_type_exec(struct nand_chip *chip, > + const struct nand_subop *subop) > +{ > + return anfc_misc_data_type_exec(chip, subop, PROG_RDPARAM); > +} > + > +static int anfc_data_read_type_exec(struct nand_chip *chip, > + const struct nand_subop *subop) > +{ > + return anfc_misc_data_type_exec(chip, subop, PROG_PGRD); > +} > + > +static int anfc_param_write_type_exec(struct nand_chip *chip, > + const struct nand_subop *subop) > +{ > + return anfc_misc_data_type_exec(chip, subop, PROG_SET_FEATURE); > +} > + > +static int anfc_data_write_type_exec(struct nand_chip *chip, > + const struct nand_subop *subop) > +{ > + return anfc_misc_data_type_exec(chip, subop, PROG_PGPROG); > +} > + > +static int anfc_misc_zerolen_type_exec(struct nand_chip *chip, > + const struct nand_subop *subop, > + u32 prog_reg) > +{ > + struct arasan_nfc *nfc = to_anfc(chip->controller); > + struct anfc_op nfc_op = {}; > + int ret; > + > + anfc_parse_instructions(chip, subop, &nfc_op); > + nfc_op.prog_reg = prog_reg; > + anfc_trigger_op(nfc, &nfc_op); > + > + ret = anfc_wait_for_event(nfc, XFER_COMPLETE); > + if (ret) > + return ret; > + > + if (nfc_op.rdy_timeout_ms) > + ret = anfc_wait_for_rb(nfc, chip, nfc_op.rdy_timeout_ms); > + > + return ret; > +} > + > +static int anfc_status_type_exec(struct nand_chip *chip, > + const struct nand_subop *subop) > +{ > + struct arasan_nfc *nfc = to_anfc(chip->controller); > + u32 tmp; > + int ret; > + > + /* > + * This controller does not allow to proceed with a CMD+DATA_IN cycle > + * manually on the bus by reading data from the data register. Instead, > + * the controller abstract the status read operation with its own status > + * register after ordering a read status operation. Hence, the following > + * hack. > + */ > + if (subop->instrs[0].ctx.cmd.opcode != NAND_CMD_STATUS) > + return -ENOTSUPP; > + > + ret = anfc_misc_zerolen_type_exec(chip, subop, PROG_STATUS); > + if (ret) > + return ret; > + > + tmp = readl_relaxed(nfc->base + FLASH_STS_REG); > + memcpy(subop->instrs[1].ctx.data.buf.in, &tmp, 1); > + > + return 0; > +} > + > +static int anfc_reset_type_exec(struct nand_chip *chip, > + const struct nand_subop *subop) > +{ > + return anfc_misc_zerolen_type_exec(chip, subop, PROG_RST); > +} > + > +static int anfc_erase_type_exec(struct nand_chip *chip, > + const struct nand_subop *subop) > +{ > + return anfc_misc_zerolen_type_exec(chip, subop, PROG_ERASE); > +} > + > +static int anfc_wait_type_exec(struct nand_chip *chip, > + const struct nand_subop *subop) > +{ > + struct arasan_nfc *nfc = to_anfc(chip->controller); > + struct anfc_op nfc_op = {}; > + > + anfc_parse_instructions(chip, subop, &nfc_op); > + > + return anfc_wait_for_rb(nfc, chip, nfc_op.rdy_timeout_ms); > +} > + > +static const struct nand_op_parser anfc_op_parser = NAND_OP_PARSER( > + NAND_OP_PARSER_PATTERN( > + anfc_param_read_type_exec, > + NAND_OP_PARSER_PAT_CMD_ELEM(false), > + NAND_OP_PARSER_PAT_ADDR_ELEM(false, ANFC_MAX_ADDR_CYC), > + NAND_OP_PARSER_PAT_WAITRDY_ELEM(true), > + NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, ANFC_MAX_CHUNK_SIZE)), > + NAND_OP_PARSER_PATTERN( > + anfc_param_write_type_exec, > + NAND_OP_PARSER_PAT_CMD_ELEM(false), > + NAND_OP_PARSER_PAT_ADDR_ELEM(false, ANFC_MAX_ADDR_CYC), > + NAND_OP_PARSER_PAT_DATA_OUT_ELEM(false, ANFC_MAX_PARAM_SIZE)), > + NAND_OP_PARSER_PATTERN( > + anfc_data_read_type_exec, > + NAND_OP_PARSER_PAT_CMD_ELEM(false), > + NAND_OP_PARSER_PAT_ADDR_ELEM(false, ANFC_MAX_ADDR_CYC), > + NAND_OP_PARSER_PAT_CMD_ELEM(false), > + NAND_OP_PARSER_PAT_WAITRDY_ELEM(true), > + NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, ANFC_MAX_CHUNK_SIZE)), > + NAND_OP_PARSER_PATTERN( > + anfc_data_write_type_exec, > + NAND_OP_PARSER_PAT_CMD_ELEM(false), > + NAND_OP_PARSER_PAT_ADDR_ELEM(false, ANFC_MAX_ADDR_CYC), > + NAND_OP_PARSER_PAT_DATA_OUT_ELEM(false, ANFC_MAX_CHUNK_SIZE), > + NAND_OP_PARSER_PAT_CMD_ELEM(false)), > + NAND_OP_PARSER_PATTERN( > + anfc_reset_type_exec, > + NAND_OP_PARSER_PAT_CMD_ELEM(false), > + NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)), > + NAND_OP_PARSER_PATTERN( > + anfc_erase_type_exec, > + NAND_OP_PARSER_PAT_CMD_ELEM(false), > + NAND_OP_PARSER_PAT_ADDR_ELEM(false, ANFC_MAX_ADDR_CYC), > + NAND_OP_PARSER_PAT_CMD_ELEM(false), > + NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)), > + NAND_OP_PARSER_PATTERN( > + anfc_status_type_exec, > + NAND_OP_PARSER_PAT_CMD_ELEM(false), > + NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, ANFC_MAX_CHUNK_SIZE)), > + NAND_OP_PARSER_PATTERN( > + anfc_wait_type_exec, > + NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)), > + ); > + Okay, no DATA-only patterns, so my suggestion to split non-aligned data reads doesn't work. I'd suggest to describe data-lengths constraints rather than automatically adjusting the data length to something bigger when we can't do exactly the number of requested DATA cycles. I started doing something similar here [1], except you'd need much more fined-grained constraints, so maybe we should add an optional check hook to data patterns. > +static int anfc_select_target(struct nand_chip *chip, int target) > +{ > + struct anand *anand = to_anand(chip); > + struct arasan_nfc *nfc = to_anfc(chip->controller); > + int ret; > + > + /* Update the controller timings and the potential ECC configuration */ > + writel_relaxed(anand->timings, nfc->base + DATA_INTERFACE_REG); > + > + /* Update clock frequency */ > + if (nfc->cur_clk != anand->clk) { > + clk_disable_unprepare(nfc->controller_clk); > + ret = clk_set_rate(nfc->controller_clk, anand->clk); > + if (ret) { > + dev_err(nfc->dev, "Failed to change clock rate\n"); > + return ret; > + } > + > + ret = clk_prepare_enable(nfc->controller_clk); > + if (ret) { > + dev_err(nfc->dev, > + "Failed to re-enable the controller clock\n"); > + return ret; > + } > + > + nfc->cur_clk = anand->clk; > + } > + > + return 0; > +} > + > +static int anfc_exec_op(struct nand_chip *chip, > + const struct nand_operation *op, > + bool check_only) > +{ > + int ret; > + > + if (!check_only) { > + ret = anfc_select_target(chip, op->cs); > + if (ret) > + return ret; > + } Given you do only one thing in the check_only case, I'd do: if (check_only) nand_op_parser_exec_op(chip, &anfc_op_parser, op, true); ret = anfc_select_target(chip, op->cs); if (ret) return ret; ... > + > + return nand_op_parser_exec_op(chip, &anfc_op_parser, op, check_only); > +} > + > +static int anfc_setup_data_interface(struct nand_chip *chip, int target, > + const struct nand_data_interface *conf) > +{ > + struct anand *anand = to_anand(chip); > + struct arasan_nfc *nfc = to_anfc(chip->controller); > + struct device_node *np = nfc->dev->of_node; > + > + if (target < 0) > + return 0; > + > + anand->timings = DIFACE_SDR | DIFACE_SDR_MODE(conf->timings.mode); > + anand->clk = ANFC_XLNX_SDR_DFLT_CORE_CLK; > + > + /* > + * Due to a hardware bug in the ZynqMP SoC, SDR timing modes 0-1 work > + * with f > 90MHz (default clock is 100MHz) but signals are unstable > + * with higher modes. Hence we decrease a little bit the clock rate to > + * 80MHz when using modes 2-5 with this SoC. > + */ > + if (of_device_is_compatible(np, "xlnx,zynqmp-nand-controller") && > + conf->timings.mode >= 2) > + anand->clk = ANFC_XLNX_SDR_HS_CORE_CLK; > + > + return 0; > +} > + > +static int anfc_attach_chip(struct nand_chip *chip) > +{ > + struct anand *anand = to_anand(chip); > + struct arasan_nfc *nfc = to_anfc(chip->controller); > + struct mtd_info *mtd = nand_to_mtd(chip); > + int ret = 0; > + > + if (mtd->writesize <= SZ_512) > + anand->caddr_cycles = 1; > + else > + anand->caddr_cycles = 2; > + > + if (chip->options & NAND_ROW_ADDR_3) > + anand->raddr_cycles = 3; > + else > + anand->raddr_cycles = 2; > + > + switch (mtd->writesize) { > + case 512: > + anand->page_sz = 0; > + break; > + case 1024: > + anand->page_sz = 5; > + break; > + case 2048: > + anand->page_sz = 1; > + break; > + case 4096: > + anand->page_sz = 2; > + break; > + case 8192: > + anand->page_sz = 3; > + break; > + case 16384: > + anand->page_sz = 4; > + break; > + default: > + return -EINVAL; > + } > + > + /* These hooks are valid for all ECC providers */ > + chip->ecc.read_page_raw = nand_monolithic_read_page_raw; > + chip->ecc.write_page_raw = nand_monolithic_write_page_raw; > + > + switch (chip->ecc.mode) { > + case NAND_ECC_NONE: > + case NAND_ECC_SOFT: > + case NAND_ECC_ON_DIE: > + break; > + case NAND_ECC_HW: > + default: > + dev_err(nfc->dev, "Unsupported ECC mode: %d\n", > + chip->ecc.mode); > + return -EINVAL; > + } > + > + return ret; > +} > + > +static const struct nand_controller_ops anfc_ops = { > + .exec_op = anfc_exec_op, > + .setup_data_interface = anfc_setup_data_interface, > + .attach_chip = anfc_attach_chip, > +}; > + > +static int anfc_chip_init(struct arasan_nfc *nfc, struct device_node *np) > +{ > + struct anand *anand; > + struct nand_chip *chip; > + struct mtd_info *mtd; > + int cs, rb, ret; > + > + anand = devm_kzalloc(nfc->dev, sizeof(*anand), GFP_KERNEL); > + if (!anand) > + return -ENOMEM; > + > + /* Only one CS can be asserted at a time */ You mean the controller only has one CS? Cause that comment doesn't make much sense. > + if (of_property_count_elems_of_size(np, "reg", sizeof(u32)) != 1) { > + dev_err(nfc->dev, "Invalid reg property\n"); > + return -EINVAL; > + } It's only a SW limitation. If your controller has several CS pins (which seems to be the case since you have a list of chips), it can handle multi-CS chips too. I'm perfectly fine with the limitation itself, but the comment should reflect the reality. And if you want to support multi-CS chips, you just have to define an array of CS/RB in anand. > + > + ret = of_property_read_u32(np, "reg", &cs); > + if (ret) > + return ret; > + > + ret = of_property_read_u32(np, "nand-rb", &rb); > + if (ret) > + return ret; > + > + if (cs >= ANFC_MAX_CS || rb >= ANFC_MAX_CS) { > + dev_err(nfc->dev, "Wrong CS %d or RB %d\n", cs, rb); > + return -EINVAL; > + } > + > + if (test_and_set_bit(cs, &nfc->assigned_cs)) { > + dev_err(nfc->dev, "Already assigned CS %d\n", cs); > + return -EINVAL; > + } > + > + anand->cs = cs; > + anand->rb = rb; > + > + chip = &anand->chip; > + mtd = nand_to_mtd(chip); > + mtd->dev.parent = nfc->dev; > + chip->controller = &nfc->controller; > + chip->options = NAND_BUSWIDTH_AUTO | NAND_NO_SUBPAGE_WRITE | > + NAND_USES_DMA; > + > + nand_set_flash_node(chip, np); > + if (!mtd->name) { > + dev_err(nfc->dev, "NAND label property is mandatory\n"); > + return -EINVAL; > + } > + > + ret = nand_scan(chip, 1); > + if (ret) { > + dev_err(nfc->dev, "Scan operation failed\n"); > + return ret; > + } > + > + ret = mtd_device_register(mtd, NULL, 0); > + if (ret) { > + nand_release(chip); nand_cleanup(chip); > + return ret; > + } > + > + list_add_tail(&anand->node, &nfc->chips); > + > + return 0; > +} > + > +static void anfc_chips_cleanup(struct arasan_nfc *nfc) > +{ > + struct anand *anand, *tmp; > + > + list_for_each_entry_safe(anand, tmp, &nfc->chips, node) { > + nand_release(&anand->chip); > + list_del(&anand->node); > + } > +} > + > +static int anfc_chips_init(struct arasan_nfc *nfc) > +{ > + struct device_node *np = nfc->dev->of_node, *nand_np; > + int nchips = of_get_child_count(np); > + int ret; > + > + if (!nchips || nchips > ANFC_MAX_CS) { > + dev_err(nfc->dev, "Incorrect number of NAND chips (%d)\n", > + nchips); > + return -EINVAL; > + } > + > + for_each_child_of_node(np, nand_np) { > + ret = anfc_chip_init(nfc, nand_np); > + if (ret) { > + of_node_put(nand_np); > + anfc_chips_cleanup(nfc); > + break; > + } > + } > + > + return ret; > +} > + > +static void anfc_reset(struct arasan_nfc *nfc) > +{ > + anfc_disable_int(nfc, EVENT_MASK); > + > + /* Enable all interrupt status */ > + writel_relaxed(EVENT_MASK, nfc->base + INTR_STS_EN_REG); That doesn't sounds like a good idea. Interrupts should be enabled only when you need to wait on specific events. > +} > + [1]https://github.com/bbrezillon/linux/commit/02c0ce63be2d43f207bdecafd555c42efdacece2