| Message ID | 20180114220012.30039-1-stefan@agner.ch |
|---|---|
| State | Changes Requested |
| Delegated to: | Boris Brezillon |
| Headers | show |
| Series | [RFC,v2] mtd: nand: vf610_nfc: make use of ->exec_op() | expand |
Hi Stefan, On Sun, 14 Jan 2018 23:00:12 +0100 Stefan Agner <stefan@agner.ch> wrote: > This reworks the driver to make use of ->exec_op() callback. The > command sequencer of the VF610 NFC aligns well with the new ops > interface. > > The ops are translated to a NFC command code while filling the > necessary registers. Instead of using the special status and > read id command codes (which require the status/id form special > registers) the driver now uses the main data buffer for all > commands. This simplifies the driver as no special casing is > needed. > > For control data (status byte, id bytes and parameter page) the > driver needs to reverse byte order for little endian CPUs since > the controller seems to store the bytes in big endian order in > the data buffer. > > The current state seems to pass MTD tests on a Colibri VF61. > > Signed-off-by: Stefan Agner <stefan@agner.ch> > --- > Hi, > > In this second version I was able to remove all special command > handling. As Boris suggested, the special READID/STATUS handling > is really not needed and all return values can be read from the > regular data buffer. This simplified code even more and allowed > to get rid of the hack in the MTD core. The driver is now able > to handle all commands in a single function. Therefor I also got > rid of the command parser, since everything is handled in a single > function anyway. Hm, I'm not sure getting rid of the generic command parser is a good move. AFAICT, your controller has constraints of the command order and you'll have to add tests that are already handled by the generic parser (example, CMD+ADDR+CMD+WAITRB+DATA_OUT is not supported). Another reason is the internal buffer SIZE limitation (2k IIUC), which means, if the core requests more than 2k in one go, you'll have to split the request in several operations. The generic operation parser can do that for you. > > The only thing which is somewhat unfortunate is the byte ordering > which needs to be reversed for commands which read flash data (id, > status and parameter page). All those request 8-bit handling, so > I used this as an indicator to fix byte order. Actually, I think the faulty part is the write/read page handling which is doing things in the wrong endianness, but that's not something you can change. I have one solution for this problem: don't use the generic helpers for the read/write page hooks, and always swap bytes in the ->exec_op() path. Note that "8-bit vs non-8-bit" approach is not such a good idea, because the core might want to read OTP area of the NAND (programmed by the vendor) using non-8-bit mode. > > I did meassured a slight performance drop (before vs. now): > - write speed is 4036 KiB/s vs 3495 KiB/s > - read speed is 13741 KiB/s vs 13490 KiB/s I still need to take a closer look at the code, but it might be a good idea to bypass the ->exec_op() logic in your ecc->[read,write]_page[_raw] handlers and optimize this path. I'll try to have a look at the diff, but it's a bit hard to review because of the unrelated interleaved removed/added lines. Thanks, Boris
On Sun, 14 Jan 2018 23:00:12 +0100 Stefan Agner <stefan@agner.ch> wrote: > This reworks the driver to make use of ->exec_op() callback. The > command sequencer of the VF610 NFC aligns well with the new ops > interface. > > The ops are translated to a NFC command code while filling the > necessary registers. Instead of using the special status and > read id command codes (which require the status/id form special > registers) the driver now uses the main data buffer for all > commands. This simplifies the driver as no special casing is > needed. > > For control data (status byte, id bytes and parameter page) the > driver needs to reverse byte order for little endian CPUs since > the controller seems to store the bytes in big endian order in > the data buffer. > > The current state seems to pass MTD tests on a Colibri VF61. > > Signed-off-by: Stefan Agner <stefan@agner.ch> > --- > Hi, > > In this second version I was able to remove all special command > handling. As Boris suggested, the special READID/STATUS handling > is really not needed and all return values can be read from the > regular data buffer. This simplified code even more and allowed > to get rid of the hack in the MTD core. The driver is now able > to handle all commands in a single function. Therefor I also got > rid of the command parser, since everything is handled in a single > function anyway. > > The only thing which is somewhat unfortunate is the byte ordering > which needs to be reversed for commands which read flash data (id, > status and parameter page). All those request 8-bit handling, so > I used this as an indicator to fix byte order. > > I did meassured a slight performance drop (before vs. now): > - write speed is 4036 KiB/s vs 3495 KiB/s > - read speed is 13741 KiB/s vs 13490 KiB/s > > -- > Stefan > > drivers/mtd/nand/vf610_nfc.c | 444 +++++++++++++++++++------------------------ > 1 file changed, 194 insertions(+), 250 deletions(-) > > diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c > index 80d31a58e558..fb428a94bbd4 100644 > --- a/drivers/mtd/nand/vf610_nfc.c > +++ b/drivers/mtd/nand/vf610_nfc.c > @@ -59,20 +59,20 @@ > #define OOB_64 0x0040 > #define OOB_MAX 0x0100 > > -/* > - * NFC_CMD2[CODE] values. See section: > - * - 31.4.7 Flash Command Code Description, Vybrid manual > - * - 23.8.6 Flash Command Sequencer, MPC5125 manual > - * > - * Briefly these are bitmasks of controller cycles. > - */ > -#define READ_PAGE_CMD_CODE 0x7EE0 > -#define READ_ONFI_PARAM_CMD_CODE 0x4860 > -#define PROGRAM_PAGE_CMD_CODE 0x7FC0 > -#define ERASE_CMD_CODE 0x4EC0 > -#define READ_ID_CMD_CODE 0x4804 > -#define RESET_CMD_CODE 0x4040 > -#define STATUS_READ_CMD_CODE 0x4068 > +/* NFC_CMD2[CODE] controller cycle bit masks */ > +#define COMMAND_CMD_BYTE1 BIT(14) > +#define COMMAND_CAR_BYTE1 BIT(13) > +#define COMMAND_CAR_BYTE2 BIT(12) > +#define COMMAND_RAR_BYTE1 BIT(11) > +#define COMMAND_RAR_BYTE2 BIT(10) > +#define COMMAND_RAR_BYTE3 BIT(9) ADDRESS cycles will always be specified in little-endian (column cycles first and then row cycles), so maybe you can do: #define COMMAND_NADDR_CYCLES(x) GENMASK(14, 14 - (x) - 1) > +#define COMMAND_WRITE_DATA BIT(8) > +#define COMMAND_CMD_BYTE2 BIT(7) > +#define COMMAND_RB_HANDSHAKE BIT(6) > +#define COMMAND_READ_DATA BIT(5) > +#define COMMAND_CMD_BYTE3 BIT(4) > +#define COMMAND_READ_STATUS BIT(3) > +#define COMMAND_READ_ID BIT(2) > > /* NFC ECC mode define */ > #define ECC_BYPASS 0 > @@ -97,10 +97,21 @@ > /* NFC_COL_ADDR Field */ > #define COL_ADDR_MASK 0x0000FFFF > #define COL_ADDR_SHIFT 0 > +#define COL_ADDR_BYTE1_SHIFT 0 > +#define COL_ADDR_BYTE2_SHIFT 8 > +#define COL_ADDR_BYTE1(x) ((x & 0xFF) << COL_ADDR_BYTE1_SHIFT) > +#define COL_ADDR_BYTE2(x) ((x & 0XFF) << COL_ADDR_BYTE2_SHIFT) Or just #define COL_ADDR(pos, val) ((val) << (8 * (pos))) > + > > /* NFC_ROW_ADDR Field */ > #define ROW_ADDR_MASK 0x00FFFFFF > #define ROW_ADDR_SHIFT 0 > +#define ROW_ADDR_BYTE1_SHIFT 0 > +#define ROW_ADDR_BYTE2_SHIFT 8 > +#define ROW_ADDR_BYTE3_SHIFT 16 > +#define ROW_ADDR_BYTE1(x) ((x & 0xFF) << ROW_ADDR_BYTE1_SHIFT) > +#define ROW_ADDR_BYTE2(x) ((x & 0XFF) << ROW_ADDR_BYTE2_SHIFT) > +#define ROW_ADDR_BYTE3(x) ((x & 0XFF) << ROW_ADDR_BYTE3_SHIFT) Same here: #define ROW_ADDR(pos, val) ((val) << (8 * (pos))) > #define ROW_ADDR_CHIP_SEL_RB_MASK 0xF0000000 > #define ROW_ADDR_CHIP_SEL_RB_SHIFT 28 > #define ROW_ADDR_CHIP_SEL_MASK 0x0F000000 > @@ -142,13 +153,6 @@ > #define ECC_STATUS_MASK 0x80 > #define ECC_STATUS_ERR_COUNT 0x3F > > -enum vf610_nfc_alt_buf { > - ALT_BUF_DATA = 0, > - ALT_BUF_ID = 1, > - ALT_BUF_STAT = 2, > - ALT_BUF_ONFI = 3, > -}; > - > enum vf610_nfc_variant { > NFC_VFC610 = 1, > }; > @@ -158,10 +162,7 @@ struct vf610_nfc { > struct device *dev; > void __iomem *regs; > struct completion cmd_done; > - uint buf_offset; > - int write_sz; > /* Status and ID are in alternate locations. */ > - enum vf610_nfc_alt_buf alt_buf; > enum vf610_nfc_variant variant; > struct clk *clk; > bool use_hw_ecc; > @@ -173,6 +174,11 @@ static inline struct vf610_nfc *mtd_to_nfc(struct mtd_info *mtd) > return container_of(mtd_to_nand(mtd), struct vf610_nfc, chip); > } > > +static inline struct vf610_nfc *chip_to_nfc(struct nand_chip *chip) > +{ > + return container_of(chip, struct vf610_nfc, chip); > +} Not related to this patch, but the chip and nfc objects should be separated, and the NFC object should inherit from nand_hw_control. > + > static inline u32 vf610_nfc_read(struct vf610_nfc *nfc, uint reg) > { > return readl(nfc->regs + reg); > @@ -214,7 +220,6 @@ static inline void vf610_nfc_memcpy(void *dst, const void __iomem *src, > memcpy(dst, src, n); > } > > -/* Clear flags for upcoming command */ > static inline void vf610_nfc_clear_status(struct vf610_nfc *nfc) > { > u32 tmp = vf610_nfc_read(nfc, NFC_IRQ_STATUS); > @@ -243,51 +248,22 @@ static void vf610_nfc_done(struct vf610_nfc *nfc) > vf610_nfc_clear_status(nfc); > } > > -static u8 vf610_nfc_get_id(struct vf610_nfc *nfc, int col) > -{ > - u32 flash_id; > - > - if (col < 4) { > - flash_id = vf610_nfc_read(nfc, NFC_FLASH_STATUS1); > - flash_id >>= (3 - col) * 8; > - } else { > - flash_id = vf610_nfc_read(nfc, NFC_FLASH_STATUS2); > - flash_id >>= 24; > - } > - > - return flash_id & 0xff; > -} > - > -static u8 vf610_nfc_get_status(struct vf610_nfc *nfc) > +static void vf610_nfc_send_code(struct vf610_nfc *nfc, u32 cmd_code) > { > - return vf610_nfc_read(nfc, NFC_FLASH_STATUS2) & STATUS_BYTE1_MASK; > + vf610_nfc_set_field(nfc, NFC_FLASH_CMD2, CMD_CODE_MASK, CMD_CODE_SHIFT, > + cmd_code); > } > > -static void vf610_nfc_send_command(struct vf610_nfc *nfc, u32 cmd_byte1, > - u32 cmd_code) > +static void vf610_nfc_set_cmd1(struct vf610_nfc *nfc, u32 cmd_byte1) > { > - u32 tmp; > - > - vf610_nfc_clear_status(nfc); > - > - tmp = vf610_nfc_read(nfc, NFC_FLASH_CMD2); > - tmp &= ~(CMD_BYTE1_MASK | CMD_CODE_MASK | BUFNO_MASK); > - tmp |= cmd_byte1 << CMD_BYTE1_SHIFT; > - tmp |= cmd_code << CMD_CODE_SHIFT; > - vf610_nfc_write(nfc, NFC_FLASH_CMD2, tmp); > + vf610_nfc_set_field(nfc, NFC_FLASH_CMD2, CMD_BYTE1_MASK, > + CMD_BYTE1_SHIFT, cmd_byte1); > } > > -static void vf610_nfc_send_commands(struct vf610_nfc *nfc, u32 cmd_byte1, > - u32 cmd_byte2, u32 cmd_code) > +static void vf610_nfc_set_cmd2(struct vf610_nfc *nfc, u32 cmd_byte2) > { > - u32 tmp; > - > - vf610_nfc_send_command(nfc, cmd_byte1, cmd_code); > - > - tmp = vf610_nfc_read(nfc, NFC_FLASH_CMD1); > - tmp &= ~CMD_BYTE2_MASK; > - tmp |= cmd_byte2 << CMD_BYTE2_SHIFT; > - vf610_nfc_write(nfc, NFC_FLASH_CMD1, tmp); > + vf610_nfc_set_field(nfc, NFC_FLASH_CMD1, CMD_BYTE2_MASK, > + CMD_BYTE2_SHIFT, cmd_byte2); Hm, you should prepare all reg values in an intermediate object, and then push everything to the regs once the whole operation is ready to be submitted. This set_field() might be one the reason you get better perfs before this rework. > } > > static irqreturn_t vf610_nfc_irq(int irq, void *data) > @@ -301,19 +277,6 @@ static irqreturn_t vf610_nfc_irq(int irq, void *data) > return IRQ_HANDLED; > } > > -static void vf610_nfc_addr_cycle(struct vf610_nfc *nfc, int column, int page) > -{ > - if (column != -1) { > - if (nfc->chip.options & NAND_BUSWIDTH_16) > - column = column / 2; > - vf610_nfc_set_field(nfc, NFC_COL_ADDR, COL_ADDR_MASK, > - COL_ADDR_SHIFT, column); > - } > - if (page != -1) > - vf610_nfc_set_field(nfc, NFC_ROW_ADDR, ROW_ADDR_MASK, > - ROW_ADDR_SHIFT, page); > -} > - > static inline void vf610_nfc_ecc_mode(struct vf610_nfc *nfc, int ecc_mode) > { > vf610_nfc_set_field(nfc, NFC_FLASH_CONFIG, > @@ -326,167 +289,158 @@ static inline void vf610_nfc_transfer_size(struct vf610_nfc *nfc, int size) > vf610_nfc_write(nfc, NFC_SECTOR_SIZE, size); > } > > -static void vf610_nfc_command(struct mtd_info *mtd, unsigned command, > - int column, int page) > +static inline const struct nand_op_instr *vf610_get_next_instr( > + const struct nand_operation *op, int *op_id) > { > - struct vf610_nfc *nfc = mtd_to_nfc(mtd); > - int trfr_sz = nfc->chip.options & NAND_BUSWIDTH_16 ? 1 : 0; > + if (*op_id + 1 >= op->ninstrs) > + return NULL; > > - nfc->buf_offset = max(column, 0); > - nfc->alt_buf = ALT_BUF_DATA; > + return &op->instrs[++(*op_id)]; > +} > > - switch (command) { > - case NAND_CMD_SEQIN: > - /* Use valid column/page from preread... */ > - vf610_nfc_addr_cycle(nfc, column, page); > - nfc->buf_offset = 0; > +static int vf610_nfc_exec_op(struct nand_chip *chip, > + const struct nand_operation *op, > + bool check_only) > +{ > + const struct nand_op_instr *instr; > + struct vf610_nfc *nfc = chip_to_nfc(chip); > + struct mtd_info *mtd = nand_to_mtd(chip); > + int op_id = -1, trfr_sz = 0, code = 0; > > - /* > - * SEQIN => data => PAGEPROG sequence is done by the controller > - * hence we do not need to issue the command here... > - */ > - return; > - case NAND_CMD_PAGEPROG: > - trfr_sz += nfc->write_sz; > - vf610_nfc_transfer_size(nfc, trfr_sz); > - vf610_nfc_send_commands(nfc, NAND_CMD_SEQIN, > - command, PROGRAM_PAGE_CMD_CODE); > - if (nfc->use_hw_ecc) > - vf610_nfc_ecc_mode(nfc, nfc->ecc_mode); > - else > - vf610_nfc_ecc_mode(nfc, ECC_BYPASS); > - break; > - > - case NAND_CMD_RESET: > - vf610_nfc_transfer_size(nfc, 0); > - vf610_nfc_send_command(nfc, command, RESET_CMD_CODE); > - break; > - > - case NAND_CMD_READOOB: > - trfr_sz += mtd->oobsize; > - column = mtd->writesize; > - vf610_nfc_transfer_size(nfc, trfr_sz); > - vf610_nfc_send_commands(nfc, NAND_CMD_READ0, > - NAND_CMD_READSTART, READ_PAGE_CMD_CODE); > - vf610_nfc_addr_cycle(nfc, column, page); > - vf610_nfc_ecc_mode(nfc, ECC_BYPASS); > - break; > - > - case NAND_CMD_READ0: > - trfr_sz += mtd->writesize + mtd->oobsize; > - vf610_nfc_transfer_size(nfc, trfr_sz); > - vf610_nfc_send_commands(nfc, NAND_CMD_READ0, > - NAND_CMD_READSTART, READ_PAGE_CMD_CODE); > - vf610_nfc_addr_cycle(nfc, column, page); > - vf610_nfc_ecc_mode(nfc, nfc->ecc_mode); > - break; > - > - case NAND_CMD_PARAM: > - nfc->alt_buf = ALT_BUF_ONFI; > - trfr_sz = 3 * sizeof(struct nand_onfi_params); > - vf610_nfc_transfer_size(nfc, trfr_sz); > - vf610_nfc_send_command(nfc, command, READ_ONFI_PARAM_CMD_CODE); > - vf610_nfc_addr_cycle(nfc, -1, column); > - vf610_nfc_ecc_mode(nfc, ECC_BYPASS); > - break; > - > - case NAND_CMD_ERASE1: > - vf610_nfc_transfer_size(nfc, 0); > - vf610_nfc_send_commands(nfc, command, > - NAND_CMD_ERASE2, ERASE_CMD_CODE); > - vf610_nfc_addr_cycle(nfc, column, page); > - break; > - > - case NAND_CMD_READID: > - nfc->alt_buf = ALT_BUF_ID; > - nfc->buf_offset = 0; > - vf610_nfc_transfer_size(nfc, 0); > - vf610_nfc_send_command(nfc, command, READ_ID_CMD_CODE); > - vf610_nfc_addr_cycle(nfc, -1, column); > - break; > - > - case NAND_CMD_STATUS: > - nfc->alt_buf = ALT_BUF_STAT; > - vf610_nfc_transfer_size(nfc, 0); > - vf610_nfc_send_command(nfc, command, STATUS_READ_CMD_CODE); > - break; > - default: > - return; > + > + /* Some ops are optional, but they need to be in order */ > + instr = vf610_get_next_instr(op, &op_id); > + if (!instr) > + return -EINVAL; > + > + if (instr && instr->type == NAND_OP_CMD_INSTR) { > + dev_dbg(nfc->dev, "OP_CMD: code 0x%02x\n", instr->ctx.cmd.opcode); > + vf610_nfc_set_cmd1(nfc, instr->ctx.cmd.opcode); > + code |= COMMAND_CMD_BYTE1; > + > + instr = vf610_get_next_instr(op, &op_id); > } > > - vf610_nfc_done(nfc); > + if (instr && instr->type == NAND_OP_ADDR_INSTR) { > + int addr = 0; > + u32 col = 0, row; > > - nfc->use_hw_ecc = false; > - nfc->write_sz = 0; > -} > + if (instr->ctx.addr.naddrs > 1) { > + col = COL_ADDR_BYTE1(instr->ctx.addr.addrs[addr++]); > + code |= COMMAND_CAR_BYTE1; > > -static void vf610_nfc_read_buf(struct mtd_info *mtd, u_char *buf, int len) > -{ > - struct vf610_nfc *nfc = mtd_to_nfc(mtd); > - uint c = nfc->buf_offset; > + if (mtd->writesize > 512) { > + col |= COL_ADDR_BYTE2(instr->ctx.addr.addrs[addr++]); > + code |= COMMAND_CAR_BYTE2; > + } > + } > > - /* Alternate buffers are only supported through read_byte */ > - WARN_ON(nfc->alt_buf); > + row = ROW_ADDR_BYTE1(instr->ctx.addr.addrs[addr++]); > + code |= COMMAND_RAR_BYTE1; > + if (addr < instr->ctx.addr.naddrs) { > + row |= ROW_ADDR_BYTE2(instr->ctx.addr.addrs[addr++]); > + code |= COMMAND_RAR_BYTE2; > + } > + if (addr < instr->ctx.addr.naddrs) { > + row |= ROW_ADDR_BYTE3(instr->ctx.addr.addrs[addr++]); > + code |= COMMAND_RAR_BYTE3; > + } > > - vf610_nfc_memcpy(buf, nfc->regs + NFC_MAIN_AREA(0) + c, len); > + dev_dbg(nfc->dev, "OP_ADDR: col %d, row %d\n", col, row); > + vf610_nfc_set_field(nfc, NFC_COL_ADDR, COL_ADDR_MASK, > + COL_ADDR_SHIFT, col); > > - nfc->buf_offset += len; > -} > + vf610_nfc_set_field(nfc, NFC_ROW_ADDR, ROW_ADDR_MASK, > + ROW_ADDR_SHIFT, row); > > -static void vf610_nfc_write_buf(struct mtd_info *mtd, const uint8_t *buf, > - int len) > -{ > - struct vf610_nfc *nfc = mtd_to_nfc(mtd); > - uint c = nfc->buf_offset; > - uint l; > + instr = vf610_get_next_instr(op, &op_id); > + } > > - l = min_t(uint, len, mtd->writesize + mtd->oobsize - c); > - vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + c, buf, l); > + if (instr && instr->type == NAND_OP_DATA_OUT_INSTR) { > + int len = instr->ctx.data.len; > + int offset = 0; > > - nfc->write_sz += l; > - nfc->buf_offset += l; > -} > + dev_dbg(nfc->dev, "OP_DATA_OUT: len %d, offset %d\n", len, offset); > + > + vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + offset, > + instr->ctx.data.buf.in + offset, > + len); > + code |= COMMAND_WRITE_DATA; > + trfr_sz += len; > + > + instr = vf610_get_next_instr(op, &op_id); > + } > + > + if (instr && instr->type == NAND_OP_CMD_INSTR) { > + vf610_nfc_set_cmd2(nfc, instr->ctx.cmd.opcode); > + code |= COMMAND_CMD_BYTE2; > + > + instr = vf610_get_next_instr(op, &op_id); > + } > + > + if (instr && instr->type == NAND_OP_WAITRDY_INSTR) { > + //dev_dbg(nfc->dev, "WAITRDY [max %d ms]\n", > + // instr->ctx.waitrdy.timeout_ms); > + code |= COMMAND_RB_HANDSHAKE; > + > + instr = vf610_get_next_instr(op, &op_id); > + } > + > + if (instr && instr->type == NAND_OP_DATA_IN_INSTR) { > + int len = instr->ctx.data.len; > + code |= COMMAND_READ_DATA; > + trfr_sz += len; > + } > + > + if (nfc->use_hw_ecc) { > + vf610_nfc_ecc_mode(nfc, nfc->ecc_mode); > + > + /* Always transfer complete page with OOB when using HW ECC */ > + trfr_sz = mtd->writesize + mtd->oobsize; > + } else { > + vf610_nfc_ecc_mode(nfc, ECC_BYPASS); > + } > + vf610_nfc_transfer_size(nfc, trfr_sz); > + vf610_nfc_send_code(nfc, code); > + > + dev_dbg(nfc->dev, "Start: code: 0x%04x, hw ecc: %d, trfr_sz %d\n", > + code, nfc->use_hw_ecc, trfr_sz); > + vf610_nfc_done(nfc); > + > + if (instr && instr->type == NAND_OP_DATA_IN_INSTR) { > + int len = instr->ctx.data.len; > + int offset = 0; > + bool fix_byte_order = false; > > -static uint8_t vf610_nfc_read_byte(struct mtd_info *mtd) > -{ > - struct vf610_nfc *nfc = mtd_to_nfc(mtd); > - u8 tmp; > - uint c = nfc->buf_offset; > - > - switch (nfc->alt_buf) { > - case ALT_BUF_ID: > - tmp = vf610_nfc_get_id(nfc, c); > - break; > - case ALT_BUF_STAT: > - tmp = vf610_nfc_get_status(nfc); > - break; > #ifdef __LITTLE_ENDIAN > - case ALT_BUF_ONFI: > - /* Reverse byte since the controller uses big endianness */ > - c = nfc->buf_offset ^ 0x3; > - /* fall-through */ > + fix_byte_order = true; > #endif > - default: > - tmp = *((u8 *)(nfc->regs + NFC_MAIN_AREA(0) + c)); > - break; > - } > - nfc->buf_offset++; > - return tmp; > -} > + dev_dbg(nfc->dev, "OP_DATA_IN: 8-bit: %d, len %d, offset %d\n", > + instr->ctx.data.force_8bit , len, offset); > > -static u16 vf610_nfc_read_word(struct mtd_info *mtd) > -{ > - u16 tmp; > + /* > + * HACK: force_8bit indicates reading of the parameter, status > + * or id data. The controllers seems to store data in big endian > + * byte order to the buffers. Reverse on little endian machines. > + */ > + if (instr->ctx.data.force_8bit && fix_byte_order) { > + u8 *buf = instr->ctx.data.buf.in; > > - vf610_nfc_read_buf(mtd, (u_char *)&tmp, sizeof(tmp)); > - return tmp; > -} > + while (len--) { > + int c = offset ^ 0x3; > > -/* If not provided, upper layers apply a fixed delay. */ > -static int vf610_nfc_dev_ready(struct mtd_info *mtd) > -{ > - /* NFC handles R/B internally; always ready. */ > - return 1; > + *buf = *((u8 *)(nfc->regs + NFC_MAIN_AREA(0) + c)); > + //dev_info(nfc->dev, "[%d]: %02x\n", offset, *buf); > + buf++; offset++; > + } > + } else { > + vf610_nfc_memcpy(instr->ctx.data.buf.in + offset, > + nfc->regs + NFC_MAIN_AREA(0) + offset, > + len); > + } > + } > + > + return 0; > } Hm, it's really hard to review things with these interleaved deletions/additions. For the next version I recommend that you split things in 2 steps: 1/ add ->exec_op() 2/ remove old hooks Anyway, I think you should use the generic parser in order to simplify the vf610_nfc_exec_op() logic and make it more robust/future-proof. Note that you can describe several patterns that all points to the same subop handler. This way the generic parser can isolate the sub-patterns and pass them to your subop handler which no longer has to validate instruction order. > > /* > @@ -511,21 +465,6 @@ static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip) > vf610_nfc_write(nfc, NFC_ROW_ADDR, tmp); > } > > -/* Count the number of 0's in buff up to max_bits */ > -static inline int count_written_bits(uint8_t *buff, int size, int max_bits) > -{ > - uint32_t *buff32 = (uint32_t *)buff; > - int k, written_bits = 0; > - > - for (k = 0; k < (size / 4); k++) { > - written_bits += hweight32(~buff32[k]); > - if (unlikely(written_bits > max_bits)) > - break; > - } > - > - return written_bits; > -} > - > static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat, > uint8_t *oob, int page) > { > @@ -542,8 +481,7 @@ static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat, > return ecc_count; > > /* Read OOB without ECC unit enabled */ > - vf610_nfc_command(mtd, NAND_CMD_READOOB, 0, page); > - vf610_nfc_read_buf(mtd, oob, mtd->oobsize); > + nand_read_page_op(&nfc->chip, page, mtd->writesize, oob, mtd->oobsize); > > /* > * On an erased page, bit count (including OOB) should be zero or > @@ -557,12 +495,17 @@ static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat, > static int vf610_nfc_read_page(struct mtd_info *mtd, struct nand_chip *chip, > uint8_t *buf, int oob_required, int page) > { > + struct vf610_nfc *nfc = mtd_to_nfc(mtd); > int eccsize = chip->ecc.size; > int stat; > > + nfc->use_hw_ecc = true; Why not writing to NFC_CFG here instead of doing it your ->exec_op() implem? Remember that ->exec_op() should be as dumb as possible. > nand_read_page_op(chip, page, 0, buf, eccsize); > if (oob_required) > - vf610_nfc_read_buf(mtd, chip->oob_poi, mtd->oobsize); > + vf610_nfc_memcpy(chip->oob_poi, > + nfc->regs + NFC_MAIN_AREA(0) + mtd->writesize, > + mtd->oobsize); Hm, it should be done with the generic nand_read_data_op() helper. If you want to handle page read differently in order to optimize this path, you shouldn't be using nand_read_page_op() in the first place. Anyway, when you call nand_read_page_op(), all that should go on the bus are the CMD+ADDR cycles + eccsize bytes of DATA_IN. Given that you copy things from the internal SRAM to the ->oob_poi buffer, i suspect you're breaking the first and most important rule behind ->exec_op() => "don't try to be smart, just do what you were asked to do, and if you can't, return an error". > + nfc->use_hw_ecc = false; > > stat = vf610_nfc_correct_data(mtd, buf, chip->oob_poi, page); > > @@ -579,16 +522,20 @@ static int vf610_nfc_write_page(struct mtd_info *mtd, struct nand_chip *chip, > const uint8_t *buf, int oob_required, int page) > { > struct vf610_nfc *nfc = mtd_to_nfc(mtd); > + int ret; > > - nand_prog_page_begin_op(chip, page, 0, buf, mtd->writesize); > - if (oob_required) > - vf610_nfc_write_buf(mtd, chip->oob_poi, mtd->oobsize); > - > - /* Always write whole page including OOB due to HW ECC */ > nfc->use_hw_ecc = true; > - nfc->write_sz = mtd->writesize + mtd->oobsize; > + ret = nand_prog_page_op(chip, page, 0, buf, mtd->writesize); > + nfc->use_hw_ecc = false; Same as above, looks like your ->exec_op() is too smart: it decides to transfer OOB data even if it's not asked to. I really think you want custom handling for page accessors, which is perfectly fine, but then it shouldn't use the generic xxxx_op() helpers. > > - return nand_prog_page_end_op(chip); > + return ret; > +} > + > +static int vf610_nfc_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, > + const uint8_t *buf, int oob_required, int page) > +{ > + return nand_prog_page_op(chip, page, 0, buf, > + mtd->writesize + oob_required ? mtd->oobsize : 0); > } > > static const struct of_device_id vf610_nfc_dt_ids[] = { > @@ -606,6 +553,9 @@ static void vf610_nfc_preinit_controller(struct vf610_nfc *nfc) > vf610_nfc_clear(nfc, NFC_FLASH_CONFIG, CONFIG_DMA_REQ_BIT); > vf610_nfc_set(nfc, NFC_FLASH_CONFIG, CONFIG_FAST_FLASH_BIT); > > + /* Make sure flags are cleared on startup */ > + vf610_nfc_clear_status(nfc); > + > /* Disable virtual pages, only one elementary transfer unit */ > vf610_nfc_set_field(nfc, NFC_FLASH_CONFIG, CONFIG_PAGE_CNT_MASK, > CONFIG_PAGE_CNT_SHIFT, 1); > @@ -695,15 +645,8 @@ static int vf610_nfc_probe(struct platform_device *pdev) > goto err_clk; > } > > - chip->dev_ready = vf610_nfc_dev_ready; > - chip->cmdfunc = vf610_nfc_command; > - chip->read_byte = vf610_nfc_read_byte; > - chip->read_word = vf610_nfc_read_word; > - chip->read_buf = vf610_nfc_read_buf; > - chip->write_buf = vf610_nfc_write_buf; > + chip->exec_op = vf610_nfc_exec_op; > chip->select_chip = vf610_nfc_select_chip; > - chip->onfi_set_features = nand_onfi_get_set_features_notsupp; > - chip->onfi_get_features = nand_onfi_get_set_features_notsupp; > > chip->options |= NAND_NO_SUBPAGE_WRITE; > > @@ -770,6 +713,7 @@ static int vf610_nfc_probe(struct platform_device *pdev) > > chip->ecc.read_page = vf610_nfc_read_page; > chip->ecc.write_page = vf610_nfc_write_page; > + chip->ecc.write_page_raw = vf610_nfc_write_page_raw; > > chip->ecc.size = PAGE_2K; > }
Hello Stefan, On Sun, 14 Jan 2018 23:00:12 +0100 Stefan Agner <stefan@agner.ch> wrote: > This reworks the driver to make use of ->exec_op() callback. The > command sequencer of the VF610 NFC aligns well with the new ops > interface. > > The ops are translated to a NFC command code while filling the > necessary registers. Instead of using the special status and > read id command codes (which require the status/id form special > registers) the driver now uses the main data buffer for all > commands. This simplifies the driver as no special casing is > needed. > > For control data (status byte, id bytes and parameter page) the > driver needs to reverse byte order for little endian CPUs since > the controller seems to store the bytes in big endian order in > the data buffer. > > The current state seems to pass MTD tests on a Colibri VF61. > > Signed-off-by: Stefan Agner <stefan@agner.ch> > --- First, thank you again for implementing this interface in the vf610 driver. Sorry for the very big delay I have been pretty busy these days. My comments are below. > Hi, > > In this second version I was able to remove all special command > handling. As Boris suggested, the special READID/STATUS handling > is really not needed and all return values can be read from the > regular data buffer. This simplified code even more and allowed > to get rid of the hack in the MTD core. The driver is now able That is great. A rule of thumb that I tend to also forget when I write code: logic should not be in the NAND controller driver. > to handle all commands in a single function. Therefor I also got > rid of the command parser, since everything is handled in a single > function anyway. > > The only thing which is somewhat unfortunate is the byte ordering > which needs to be reversed for commands which read flash data (id, > status and parameter page). All those request 8-bit handling, so > I used this as an indicator to fix byte order. > > I did meassured a slight performance drop (before vs. now): > - write speed is 4036 KiB/s vs 3495 KiB/s > - read speed is 13741 KiB/s vs 13490 KiB/s > > -- > Stefan > > drivers/mtd/nand/vf610_nfc.c | 444 +++++++++++++++++++------------------------ > 1 file changed, 194 insertions(+), 250 deletions(-) > > diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c > index 80d31a58e558..fb428a94bbd4 100644 > --- a/drivers/mtd/nand/vf610_nfc.c > +++ b/drivers/mtd/nand/vf610_nfc.c > @@ -59,20 +59,20 @@ > #define OOB_64 0x0040 > #define OOB_MAX 0x0100 > > -/* > - * NFC_CMD2[CODE] values. See section: > - * - 31.4.7 Flash Command Code Description, Vybrid manual > - * - 23.8.6 Flash Command Sequencer, MPC5125 manual > - * > - * Briefly these are bitmasks of controller cycles. > - */ > -#define READ_PAGE_CMD_CODE 0x7EE0 > -#define READ_ONFI_PARAM_CMD_CODE 0x4860 > -#define PROGRAM_PAGE_CMD_CODE 0x7FC0 > -#define ERASE_CMD_CODE 0x4EC0 > -#define READ_ID_CMD_CODE 0x4804 > -#define RESET_CMD_CODE 0x4040 > -#define STATUS_READ_CMD_CODE 0x4068 > +/* NFC_CMD2[CODE] controller cycle bit masks */ > +#define COMMAND_CMD_BYTE1 BIT(14) > +#define COMMAND_CAR_BYTE1 BIT(13) > +#define COMMAND_CAR_BYTE2 BIT(12) > +#define COMMAND_RAR_BYTE1 BIT(11) > +#define COMMAND_RAR_BYTE2 BIT(10) > +#define COMMAND_RAR_BYTE3 BIT(9) > +#define COMMAND_WRITE_DATA BIT(8) > +#define COMMAND_CMD_BYTE2 BIT(7) > +#define COMMAND_RB_HANDSHAKE BIT(6) > +#define COMMAND_READ_DATA BIT(5) > +#define COMMAND_CMD_BYTE3 BIT(4) > +#define COMMAND_READ_STATUS BIT(3) > +#define COMMAND_READ_ID BIT(2) > > /* NFC ECC mode define */ > #define ECC_BYPASS 0 > @@ -97,10 +97,21 @@ > /* NFC_COL_ADDR Field */ > #define COL_ADDR_MASK 0x0000FFFF > #define COL_ADDR_SHIFT 0 > +#define COL_ADDR_BYTE1_SHIFT 0 > +#define COL_ADDR_BYTE2_SHIFT 8 > +#define COL_ADDR_BYTE1(x) ((x & 0xFF) << COL_ADDR_BYTE1_SHIFT) > +#define COL_ADDR_BYTE2(x) ((x & 0XFF) << COL_ADDR_BYTE2_SHIFT) > + > > /* NFC_ROW_ADDR Field */ > #define ROW_ADDR_MASK 0x00FFFFFF > #define ROW_ADDR_SHIFT 0 > +#define ROW_ADDR_BYTE1_SHIFT 0 > +#define ROW_ADDR_BYTE2_SHIFT 8 > +#define ROW_ADDR_BYTE3_SHIFT 16 > +#define ROW_ADDR_BYTE1(x) ((x & 0xFF) << ROW_ADDR_BYTE1_SHIFT) > +#define ROW_ADDR_BYTE2(x) ((x & 0XFF) << ROW_ADDR_BYTE2_SHIFT) > +#define ROW_ADDR_BYTE3(x) ((x & 0XFF) << ROW_ADDR_BYTE3_SHIFT) > #define ROW_ADDR_CHIP_SEL_RB_MASK 0xF0000000 > #define ROW_ADDR_CHIP_SEL_RB_SHIFT 28 > #define ROW_ADDR_CHIP_SEL_MASK 0x0F000000 > @@ -142,13 +153,6 @@ > #define ECC_STATUS_MASK 0x80 > #define ECC_STATUS_ERR_COUNT 0x3F > > -enum vf610_nfc_alt_buf { > - ALT_BUF_DATA = 0, > - ALT_BUF_ID = 1, > - ALT_BUF_STAT = 2, > - ALT_BUF_ONFI = 3, > -}; > - > enum vf610_nfc_variant { > NFC_VFC610 = 1, > }; > @@ -158,10 +162,7 @@ struct vf610_nfc { > struct device *dev; > void __iomem *regs; > struct completion cmd_done; > - uint buf_offset; > - int write_sz; > /* Status and ID are in alternate locations. */ > - enum vf610_nfc_alt_buf alt_buf; > enum vf610_nfc_variant variant; > struct clk *clk; > bool use_hw_ecc; > @@ -173,6 +174,11 @@ static inline struct vf610_nfc *mtd_to_nfc(struct mtd_info *mtd) > return container_of(mtd_to_nand(mtd), struct vf610_nfc, chip); > } > > +static inline struct vf610_nfc *chip_to_nfc(struct nand_chip *chip) > +{ > + return container_of(chip, struct vf610_nfc, chip); > +} > + > static inline u32 vf610_nfc_read(struct vf610_nfc *nfc, uint reg) > { > return readl(nfc->regs + reg); > @@ -214,7 +220,6 @@ static inline void vf610_nfc_memcpy(void *dst, const void __iomem *src, > memcpy(dst, src, n); > } > > -/* Clear flags for upcoming command */ > static inline void vf610_nfc_clear_status(struct vf610_nfc *nfc) > { > u32 tmp = vf610_nfc_read(nfc, NFC_IRQ_STATUS); > @@ -243,51 +248,22 @@ static void vf610_nfc_done(struct vf610_nfc *nfc) > vf610_nfc_clear_status(nfc); > } > > -static u8 vf610_nfc_get_id(struct vf610_nfc *nfc, int col) > -{ > - u32 flash_id; > - > - if (col < 4) { > - flash_id = vf610_nfc_read(nfc, NFC_FLASH_STATUS1); > - flash_id >>= (3 - col) * 8; > - } else { > - flash_id = vf610_nfc_read(nfc, NFC_FLASH_STATUS2); > - flash_id >>= 24; > - } > - > - return flash_id & 0xff; > -} > - > -static u8 vf610_nfc_get_status(struct vf610_nfc *nfc) > +static void vf610_nfc_send_code(struct vf610_nfc *nfc, u32 cmd_code) > { > - return vf610_nfc_read(nfc, NFC_FLASH_STATUS2) & STATUS_BYTE1_MASK; > + vf610_nfc_set_field(nfc, NFC_FLASH_CMD2, CMD_CODE_MASK, CMD_CODE_SHIFT, > + cmd_code); > } > > -static void vf610_nfc_send_command(struct vf610_nfc *nfc, u32 cmd_byte1, > - u32 cmd_code) > +static void vf610_nfc_set_cmd1(struct vf610_nfc *nfc, u32 cmd_byte1) > { > - u32 tmp; > - > - vf610_nfc_clear_status(nfc); > - > - tmp = vf610_nfc_read(nfc, NFC_FLASH_CMD2); > - tmp &= ~(CMD_BYTE1_MASK | CMD_CODE_MASK | BUFNO_MASK); > - tmp |= cmd_byte1 << CMD_BYTE1_SHIFT; > - tmp |= cmd_code << CMD_CODE_SHIFT; > - vf610_nfc_write(nfc, NFC_FLASH_CMD2, tmp); > + vf610_nfc_set_field(nfc, NFC_FLASH_CMD2, CMD_BYTE1_MASK, > + CMD_BYTE1_SHIFT, cmd_byte1); > } > > -static void vf610_nfc_send_commands(struct vf610_nfc *nfc, u32 cmd_byte1, > - u32 cmd_byte2, u32 cmd_code) > +static void vf610_nfc_set_cmd2(struct vf610_nfc *nfc, u32 cmd_byte2) > { > - u32 tmp; > - > - vf610_nfc_send_command(nfc, cmd_byte1, cmd_code); > - > - tmp = vf610_nfc_read(nfc, NFC_FLASH_CMD1); > - tmp &= ~CMD_BYTE2_MASK; > - tmp |= cmd_byte2 << CMD_BYTE2_SHIFT; > - vf610_nfc_write(nfc, NFC_FLASH_CMD1, tmp); > + vf610_nfc_set_field(nfc, NFC_FLASH_CMD1, CMD_BYTE2_MASK, > + CMD_BYTE2_SHIFT, cmd_byte2); > } > > static irqreturn_t vf610_nfc_irq(int irq, void *data) > @@ -301,19 +277,6 @@ static irqreturn_t vf610_nfc_irq(int irq, void *data) > return IRQ_HANDLED; > } > > -static void vf610_nfc_addr_cycle(struct vf610_nfc *nfc, int column, int page) > -{ > - if (column != -1) { > - if (nfc->chip.options & NAND_BUSWIDTH_16) > - column = column / 2; > - vf610_nfc_set_field(nfc, NFC_COL_ADDR, COL_ADDR_MASK, > - COL_ADDR_SHIFT, column); > - } > - if (page != -1) > - vf610_nfc_set_field(nfc, NFC_ROW_ADDR, ROW_ADDR_MASK, > - ROW_ADDR_SHIFT, page); > -} > - > static inline void vf610_nfc_ecc_mode(struct vf610_nfc *nfc, int ecc_mode) > { > vf610_nfc_set_field(nfc, NFC_FLASH_CONFIG, > @@ -326,167 +289,158 @@ static inline void vf610_nfc_transfer_size(struct vf610_nfc *nfc, int size) > vf610_nfc_write(nfc, NFC_SECTOR_SIZE, size); > } > > -static void vf610_nfc_command(struct mtd_info *mtd, unsigned command, > - int column, int page) > +static inline const struct nand_op_instr *vf610_get_next_instr( > + const struct nand_operation *op, int *op_id) > { > - struct vf610_nfc *nfc = mtd_to_nfc(mtd); > - int trfr_sz = nfc->chip.options & NAND_BUSWIDTH_16 ? 1 : 0; > + if (*op_id + 1 >= op->ninstrs) > + return NULL; > > - nfc->buf_offset = max(column, 0); > - nfc->alt_buf = ALT_BUF_DATA; > + return &op->instrs[++(*op_id)]; I don't like very much this syntax because it is kind of unreadable. Would you mind incrementing op_id before the return statement? > +} > > - switch (command) { > - case NAND_CMD_SEQIN: > - /* Use valid column/page from preread... */ > - vf610_nfc_addr_cycle(nfc, column, page); > - nfc->buf_offset = 0; > +static int vf610_nfc_exec_op(struct nand_chip *chip, > + const struct nand_operation *op, > + bool check_only) The soul of ->exec_op() is here :) However this implementation looks very specific to the kind of tasks you are used to do with one specific chip. What if someone wants to support a new command that implies several command/address/data cycles interleaved? Maybe one solution would be to rework a bit by using a case statement in a for loop. If the indentation level is too high, you may use helpers to do small tasks like sending a command or addresse cycle. What do you think? > +{ > + const struct nand_op_instr *instr; > + struct vf610_nfc *nfc = chip_to_nfc(chip); > + struct mtd_info *mtd = nand_to_mtd(chip); > + int op_id = -1, trfr_sz = 0, code = 0; > > - /* > - * SEQIN => data => PAGEPROG sequence is done by the controller > - * hence we do not need to issue the command here... > - */ > - return; > - case NAND_CMD_PAGEPROG: > - trfr_sz += nfc->write_sz; > - vf610_nfc_transfer_size(nfc, trfr_sz); > - vf610_nfc_send_commands(nfc, NAND_CMD_SEQIN, > - command, PROGRAM_PAGE_CMD_CODE); > - if (nfc->use_hw_ecc) > - vf610_nfc_ecc_mode(nfc, nfc->ecc_mode); > - else > - vf610_nfc_ecc_mode(nfc, ECC_BYPASS); > - break; > - > - case NAND_CMD_RESET: > - vf610_nfc_transfer_size(nfc, 0); > - vf610_nfc_send_command(nfc, command, RESET_CMD_CODE); > - break; > - > - case NAND_CMD_READOOB: > - trfr_sz += mtd->oobsize; > - column = mtd->writesize; > - vf610_nfc_transfer_size(nfc, trfr_sz); > - vf610_nfc_send_commands(nfc, NAND_CMD_READ0, > - NAND_CMD_READSTART, READ_PAGE_CMD_CODE); > - vf610_nfc_addr_cycle(nfc, column, page); > - vf610_nfc_ecc_mode(nfc, ECC_BYPASS); > - break; > - > - case NAND_CMD_READ0: > - trfr_sz += mtd->writesize + mtd->oobsize; > - vf610_nfc_transfer_size(nfc, trfr_sz); > - vf610_nfc_send_commands(nfc, NAND_CMD_READ0, > - NAND_CMD_READSTART, READ_PAGE_CMD_CODE); > - vf610_nfc_addr_cycle(nfc, column, page); > - vf610_nfc_ecc_mode(nfc, nfc->ecc_mode); > - break; > - > - case NAND_CMD_PARAM: > - nfc->alt_buf = ALT_BUF_ONFI; > - trfr_sz = 3 * sizeof(struct nand_onfi_params); > - vf610_nfc_transfer_size(nfc, trfr_sz); > - vf610_nfc_send_command(nfc, command, READ_ONFI_PARAM_CMD_CODE); > - vf610_nfc_addr_cycle(nfc, -1, column); > - vf610_nfc_ecc_mode(nfc, ECC_BYPASS); > - break; > - > - case NAND_CMD_ERASE1: > - vf610_nfc_transfer_size(nfc, 0); > - vf610_nfc_send_commands(nfc, command, > - NAND_CMD_ERASE2, ERASE_CMD_CODE); > - vf610_nfc_addr_cycle(nfc, column, page); > - break; > - > - case NAND_CMD_READID: > - nfc->alt_buf = ALT_BUF_ID; > - nfc->buf_offset = 0; > - vf610_nfc_transfer_size(nfc, 0); > - vf610_nfc_send_command(nfc, command, READ_ID_CMD_CODE); > - vf610_nfc_addr_cycle(nfc, -1, column); > - break; > - > - case NAND_CMD_STATUS: > - nfc->alt_buf = ALT_BUF_STAT; > - vf610_nfc_transfer_size(nfc, 0); > - vf610_nfc_send_command(nfc, command, STATUS_READ_CMD_CODE); > - break; > - default: > - return; > + > + /* Some ops are optional, but they need to be in order */ > + instr = vf610_get_next_instr(op, &op_id); > + if (!instr) > + return -EINVAL; > + > + if (instr && instr->type == NAND_OP_CMD_INSTR) { > + dev_dbg(nfc->dev, "OP_CMD: code 0x%02x\n", instr->ctx.cmd.opcode); > + vf610_nfc_set_cmd1(nfc, instr->ctx.cmd.opcode); > + code |= COMMAND_CMD_BYTE1; > + > + instr = vf610_get_next_instr(op, &op_id); > } > > - vf610_nfc_done(nfc); > + if (instr && instr->type == NAND_OP_ADDR_INSTR) { > + int addr = 0; > + u32 col = 0, row; > > - nfc->use_hw_ecc = false; > - nfc->write_sz = 0; > -} > + if (instr->ctx.addr.naddrs > 1) { > + col = COL_ADDR_BYTE1(instr->ctx.addr.addrs[addr++]); > + code |= COMMAND_CAR_BYTE1; > > -static void vf610_nfc_read_buf(struct mtd_info *mtd, u_char *buf, int len) > -{ > - struct vf610_nfc *nfc = mtd_to_nfc(mtd); > - uint c = nfc->buf_offset; > + if (mtd->writesize > 512) { > + col |= COL_ADDR_BYTE2(instr->ctx.addr.addrs[addr++]); > + code |= COMMAND_CAR_BYTE2; > + } > + } > > - /* Alternate buffers are only supported through read_byte */ > - WARN_ON(nfc->alt_buf); > + row = ROW_ADDR_BYTE1(instr->ctx.addr.addrs[addr++]); > + code |= COMMAND_RAR_BYTE1; > + if (addr < instr->ctx.addr.naddrs) { > + row |= ROW_ADDR_BYTE2(instr->ctx.addr.addrs[addr++]); > + code |= COMMAND_RAR_BYTE2; > + } > + if (addr < instr->ctx.addr.naddrs) { > + row |= ROW_ADDR_BYTE3(instr->ctx.addr.addrs[addr++]); > + code |= COMMAND_RAR_BYTE3; > + } > > - vf610_nfc_memcpy(buf, nfc->regs + NFC_MAIN_AREA(0) + c, len); > + dev_dbg(nfc->dev, "OP_ADDR: col %d, row %d\n", col, row); > + vf610_nfc_set_field(nfc, NFC_COL_ADDR, COL_ADDR_MASK, > + COL_ADDR_SHIFT, col); > > - nfc->buf_offset += len; > -} > + vf610_nfc_set_field(nfc, NFC_ROW_ADDR, ROW_ADDR_MASK, > + ROW_ADDR_SHIFT, row); > > -static void vf610_nfc_write_buf(struct mtd_info *mtd, const uint8_t *buf, > - int len) > -{ > - struct vf610_nfc *nfc = mtd_to_nfc(mtd); > - uint c = nfc->buf_offset; > - uint l; > + instr = vf610_get_next_instr(op, &op_id); > + } > > - l = min_t(uint, len, mtd->writesize + mtd->oobsize - c); > - vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + c, buf, l); > + if (instr && instr->type == NAND_OP_DATA_OUT_INSTR) { > + int len = instr->ctx.data.len; > + int offset = 0; > > - nfc->write_sz += l; > - nfc->buf_offset += l; > -} > + dev_dbg(nfc->dev, "OP_DATA_OUT: len %d, offset %d\n", len, offset); > + > + vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + offset, > + instr->ctx.data.buf.in + offset, > + len); > + code |= COMMAND_WRITE_DATA; > + trfr_sz += len; > + > + instr = vf610_get_next_instr(op, &op_id); > + } > + > + if (instr && instr->type == NAND_OP_CMD_INSTR) { > + vf610_nfc_set_cmd2(nfc, instr->ctx.cmd.opcode); > + code |= COMMAND_CMD_BYTE2; > + > + instr = vf610_get_next_instr(op, &op_id); > + } > + > + if (instr && instr->type == NAND_OP_WAITRDY_INSTR) { > + //dev_dbg(nfc->dev, "WAITRDY [max %d ms]\n", > + // instr->ctx.waitrdy.timeout_ms); > + code |= COMMAND_RB_HANDSHAKE; > + > + instr = vf610_get_next_instr(op, &op_id); > + } > + > + if (instr && instr->type == NAND_OP_DATA_IN_INSTR) { > + int len = instr->ctx.data.len; > + code |= COMMAND_READ_DATA; > + trfr_sz += len; > + } > + > + if (nfc->use_hw_ecc) { > + vf610_nfc_ecc_mode(nfc, nfc->ecc_mode); > + > + /* Always transfer complete page with OOB when using HW ECC */ > + trfr_sz = mtd->writesize + mtd->oobsize; > + } else { > + vf610_nfc_ecc_mode(nfc, ECC_BYPASS); > + } > + vf610_nfc_transfer_size(nfc, trfr_sz); > + vf610_nfc_send_code(nfc, code); > + > + dev_dbg(nfc->dev, "Start: code: 0x%04x, hw ecc: %d, trfr_sz %d\n", > + code, nfc->use_hw_ecc, trfr_sz); > + vf610_nfc_done(nfc); > + > + if (instr && instr->type == NAND_OP_DATA_IN_INSTR) { > + int len = instr->ctx.data.len; > + int offset = 0; > + bool fix_byte_order = false; > > -static uint8_t vf610_nfc_read_byte(struct mtd_info *mtd) > -{ > - struct vf610_nfc *nfc = mtd_to_nfc(mtd); > - u8 tmp; > - uint c = nfc->buf_offset; > - > - switch (nfc->alt_buf) { > - case ALT_BUF_ID: > - tmp = vf610_nfc_get_id(nfc, c); > - break; > - case ALT_BUF_STAT: > - tmp = vf610_nfc_get_status(nfc); > - break; > #ifdef __LITTLE_ENDIAN > - case ALT_BUF_ONFI: > - /* Reverse byte since the controller uses big endianness */ > - c = nfc->buf_offset ^ 0x3; > - /* fall-through */ > + fix_byte_order = true; > #endif > - default: > - tmp = *((u8 *)(nfc->regs + NFC_MAIN_AREA(0) + c)); > - break; > - } > - nfc->buf_offset++; > - return tmp; > -} > + dev_dbg(nfc->dev, "OP_DATA_IN: 8-bit: %d, len %d, offset %d\n", > + instr->ctx.data.force_8bit , len, offset); > > -static u16 vf610_nfc_read_word(struct mtd_info *mtd) > -{ > - u16 tmp; > + /* > + * HACK: force_8bit indicates reading of the parameter, status > + * or id data. The controllers seems to store data in big endian Well this is right today, but tomorrow, who knows which command will be executed in 8-bit mode? > + * byte order to the buffers. Reverse on little endian machines. > + */ > + if (instr->ctx.data.force_8bit && fix_byte_order) { > + u8 *buf = instr->ctx.data.buf.in; > > - vf610_nfc_read_buf(mtd, (u_char *)&tmp, sizeof(tmp)); > - return tmp; > -} > + while (len--) { > + int c = offset ^ 0x3; > > -/* If not provided, upper layers apply a fixed delay. */ > -static int vf610_nfc_dev_ready(struct mtd_info *mtd) > -{ > - /* NFC handles R/B internally; always ready. */ > - return 1; > + *buf = *((u8 *)(nfc->regs + NFC_MAIN_AREA(0) + c)); > + //dev_info(nfc->dev, "[%d]: %02x\n", offset, *buf); > + buf++; offset++; > + } > + } else { > + vf610_nfc_memcpy(instr->ctx.data.buf.in + offset, > + nfc->regs + NFC_MAIN_AREA(0) + offset, > + len); > + } > + } > + > + return 0; > } When using ->exec_op, I don't think you actually need a proper ->dev_ready() like before. > > /* > @@ -511,21 +465,6 @@ static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip) > vf610_nfc_write(nfc, NFC_ROW_ADDR, tmp); > } > > -/* Count the number of 0's in buff up to max_bits */ > -static inline int count_written_bits(uint8_t *buff, int size, int max_bits) > -{ > - uint32_t *buff32 = (uint32_t *)buff; > - int k, written_bits = 0; > - > - for (k = 0; k < (size / 4); k++) { > - written_bits += hweight32(~buff32[k]); > - if (unlikely(written_bits > max_bits)) > - break; > - } > - > - return written_bits; > -} > - > static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat, > uint8_t *oob, int page) > { > @@ -542,8 +481,7 @@ static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat, > return ecc_count; > > /* Read OOB without ECC unit enabled */ > - vf610_nfc_command(mtd, NAND_CMD_READOOB, 0, page); > - vf610_nfc_read_buf(mtd, oob, mtd->oobsize); > + nand_read_page_op(&nfc->chip, page, mtd->writesize, oob, mtd->oobsize); Would not a nand_read_oob_op() fit better here if you only need the OOB? > > /* > * On an erased page, bit count (including OOB) should be zero or > @@ -557,12 +495,17 @@ static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat, > static int vf610_nfc_read_page(struct mtd_info *mtd, struct nand_chip *chip, > uint8_t *buf, int oob_required, int page) > { > + struct vf610_nfc *nfc = mtd_to_nfc(mtd); > int eccsize = chip->ecc.size; > int stat; > > + nfc->use_hw_ecc = true; > nand_read_page_op(chip, page, 0, buf, eccsize); > if (oob_required) > - vf610_nfc_read_buf(mtd, chip->oob_poi, mtd->oobsize); > + vf610_nfc_memcpy(chip->oob_poi, > + nfc->regs + NFC_MAIN_AREA(0) + mtd->writesize, > + mtd->oobsize); > + nfc->use_hw_ecc = false; > > stat = vf610_nfc_correct_data(mtd, buf, chip->oob_poi, page); > > @@ -579,16 +522,20 @@ static int vf610_nfc_write_page(struct mtd_info *mtd, struct nand_chip *chip, > const uint8_t *buf, int oob_required, int page) > { > struct vf610_nfc *nfc = mtd_to_nfc(mtd); > + int ret; > > - nand_prog_page_begin_op(chip, page, 0, buf, mtd->writesize); > - if (oob_required) > - vf610_nfc_write_buf(mtd, chip->oob_poi, mtd->oobsize); > - > - /* Always write whole page including OOB due to HW ECC */ > nfc->use_hw_ecc = true; > - nfc->write_sz = mtd->writesize + mtd->oobsize; > + ret = nand_prog_page_op(chip, page, 0, buf, mtd->writesize); > + nfc->use_hw_ecc = false; What about calling the default ->write_page_raw() instead, which I guess should do exactly this by default (must check). > > - return nand_prog_page_end_op(chip); > + return ret; > +} > + > +static int vf610_nfc_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, > + const uint8_t *buf, int oob_required, int page) > +{ > + return nand_prog_page_op(chip, page, 0, buf, > + mtd->writesize + oob_required ? mtd->oobsize : 0); > } I think this is already handled by the core if you don't populate the ->write_page_raw entry. > > static const struct of_device_id vf610_nfc_dt_ids[] = { > @@ -606,6 +553,9 @@ static void vf610_nfc_preinit_controller(struct vf610_nfc *nfc) > vf610_nfc_clear(nfc, NFC_FLASH_CONFIG, CONFIG_DMA_REQ_BIT); > vf610_nfc_set(nfc, NFC_FLASH_CONFIG, CONFIG_FAST_FLASH_BIT); > > + /* Make sure flags are cleared on startup */ > + vf610_nfc_clear_status(nfc); > + This should probably be in another patch? > /* Disable virtual pages, only one elementary transfer unit */ > vf610_nfc_set_field(nfc, NFC_FLASH_CONFIG, CONFIG_PAGE_CNT_MASK, > CONFIG_PAGE_CNT_SHIFT, 1); > @@ -695,15 +645,8 @@ static int vf610_nfc_probe(struct platform_device *pdev) > goto err_clk; > } > > - chip->dev_ready = vf610_nfc_dev_ready; > - chip->cmdfunc = vf610_nfc_command; > - chip->read_byte = vf610_nfc_read_byte; > - chip->read_word = vf610_nfc_read_word; > - chip->read_buf = vf610_nfc_read_buf; > - chip->write_buf = vf610_nfc_write_buf; > + chip->exec_op = vf610_nfc_exec_op; > chip->select_chip = vf610_nfc_select_chip; > - chip->onfi_set_features = nand_onfi_get_set_features_notsupp; > - chip->onfi_get_features = nand_onfi_get_set_features_notsupp; > > chip->options |= NAND_NO_SUBPAGE_WRITE; > > @@ -770,6 +713,7 @@ static int vf610_nfc_probe(struct platform_device *pdev) > > chip->ecc.read_page = vf610_nfc_read_page; > chip->ecc.write_page = vf610_nfc_write_page; > + chip->ecc.write_page_raw = vf610_nfc_write_page_raw; > > chip->ecc.size = PAGE_2K; > } Thank you again for this work. Miquèl
> > +} > > > > - switch (command) { > > - case NAND_CMD_SEQIN: > > - /* Use valid column/page from preread... */ > > - vf610_nfc_addr_cycle(nfc, column, page); > > - nfc->buf_offset = 0; > > +static int vf610_nfc_exec_op(struct nand_chip *chip, > > + const struct nand_operation *op, > > + bool check_only) > > The soul of ->exec_op() is here :) However this implementation looks > very specific to the kind of tasks you are used to do with one specific > chip. What if someone wants to support a new command that implies > several command/address/data cycles interleaved? Maybe one solution > would be to rework a bit by using a case statement in a for loop. If > the indentation level is too high, you may use helpers to do small > tasks like sending a command or addresse cycle. What do you think? Actually, instead of writing your own implementation, maybe you should just write the helpers I mentioned above and fill a parser patterns array to give to the NAND core's parser, simplifying again the implementation. Miquèl
On 08.02.2018 19:23, Miquel Raynal wrote: >> > +} >> > >> > - switch (command) { >> > - case NAND_CMD_SEQIN: >> > - /* Use valid column/page from preread... */ >> > - vf610_nfc_addr_cycle(nfc, column, page); >> > - nfc->buf_offset = 0; >> > +static int vf610_nfc_exec_op(struct nand_chip *chip, >> > + const struct nand_operation *op, >> > + bool check_only) >> >> The soul of ->exec_op() is here :) However this implementation looks >> very specific to the kind of tasks you are used to do with one specific >> chip. What if someone wants to support a new command that implies >> several command/address/data cycles interleaved? Maybe one solution >> would be to rework a bit by using a case statement in a for loop. If >> the indentation level is too high, you may use helpers to do small >> tasks like sending a command or addresse cycle. What do you think? The controller mandates the sequence. If another sequence is required, then multiple controller level command executions need to be issues. > > Actually, instead of writing your own implementation, maybe you should > just write the helpers I mentioned above and fill a parser patterns > array to give to the NAND core's parser, simplifying again the > implementation. I actually worked on a v3 just yesterday which implements Boris suggetion. It also should be easier to review due two independent patches for adding exec_op and removing the old callbacks. I hope we can carry on the discussion there. -- Stefan
diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c index 80d31a58e558..fb428a94bbd4 100644 --- a/drivers/mtd/nand/vf610_nfc.c +++ b/drivers/mtd/nand/vf610_nfc.c @@ -59,20 +59,20 @@ #define OOB_64 0x0040 #define OOB_MAX 0x0100 -/* - * NFC_CMD2[CODE] values. See section: - * - 31.4.7 Flash Command Code Description, Vybrid manual - * - 23.8.6 Flash Command Sequencer, MPC5125 manual - * - * Briefly these are bitmasks of controller cycles. - */ -#define READ_PAGE_CMD_CODE 0x7EE0 -#define READ_ONFI_PARAM_CMD_CODE 0x4860 -#define PROGRAM_PAGE_CMD_CODE 0x7FC0 -#define ERASE_CMD_CODE 0x4EC0 -#define READ_ID_CMD_CODE 0x4804 -#define RESET_CMD_CODE 0x4040 -#define STATUS_READ_CMD_CODE 0x4068 +/* NFC_CMD2[CODE] controller cycle bit masks */ +#define COMMAND_CMD_BYTE1 BIT(14) +#define COMMAND_CAR_BYTE1 BIT(13) +#define COMMAND_CAR_BYTE2 BIT(12) +#define COMMAND_RAR_BYTE1 BIT(11) +#define COMMAND_RAR_BYTE2 BIT(10) +#define COMMAND_RAR_BYTE3 BIT(9) +#define COMMAND_WRITE_DATA BIT(8) +#define COMMAND_CMD_BYTE2 BIT(7) +#define COMMAND_RB_HANDSHAKE BIT(6) +#define COMMAND_READ_DATA BIT(5) +#define COMMAND_CMD_BYTE3 BIT(4) +#define COMMAND_READ_STATUS BIT(3) +#define COMMAND_READ_ID BIT(2) /* NFC ECC mode define */ #define ECC_BYPASS 0 @@ -97,10 +97,21 @@ /* NFC_COL_ADDR Field */ #define COL_ADDR_MASK 0x0000FFFF #define COL_ADDR_SHIFT 0 +#define COL_ADDR_BYTE1_SHIFT 0 +#define COL_ADDR_BYTE2_SHIFT 8 +#define COL_ADDR_BYTE1(x) ((x & 0xFF) << COL_ADDR_BYTE1_SHIFT) +#define COL_ADDR_BYTE2(x) ((x & 0XFF) << COL_ADDR_BYTE2_SHIFT) + /* NFC_ROW_ADDR Field */ #define ROW_ADDR_MASK 0x00FFFFFF #define ROW_ADDR_SHIFT 0 +#define ROW_ADDR_BYTE1_SHIFT 0 +#define ROW_ADDR_BYTE2_SHIFT 8 +#define ROW_ADDR_BYTE3_SHIFT 16 +#define ROW_ADDR_BYTE1(x) ((x & 0xFF) << ROW_ADDR_BYTE1_SHIFT) +#define ROW_ADDR_BYTE2(x) ((x & 0XFF) << ROW_ADDR_BYTE2_SHIFT) +#define ROW_ADDR_BYTE3(x) ((x & 0XFF) << ROW_ADDR_BYTE3_SHIFT) #define ROW_ADDR_CHIP_SEL_RB_MASK 0xF0000000 #define ROW_ADDR_CHIP_SEL_RB_SHIFT 28 #define ROW_ADDR_CHIP_SEL_MASK 0x0F000000 @@ -142,13 +153,6 @@ #define ECC_STATUS_MASK 0x80 #define ECC_STATUS_ERR_COUNT 0x3F -enum vf610_nfc_alt_buf { - ALT_BUF_DATA = 0, - ALT_BUF_ID = 1, - ALT_BUF_STAT = 2, - ALT_BUF_ONFI = 3, -}; - enum vf610_nfc_variant { NFC_VFC610 = 1, }; @@ -158,10 +162,7 @@ struct vf610_nfc { struct device *dev; void __iomem *regs; struct completion cmd_done; - uint buf_offset; - int write_sz; /* Status and ID are in alternate locations. */ - enum vf610_nfc_alt_buf alt_buf; enum vf610_nfc_variant variant; struct clk *clk; bool use_hw_ecc; @@ -173,6 +174,11 @@ static inline struct vf610_nfc *mtd_to_nfc(struct mtd_info *mtd) return container_of(mtd_to_nand(mtd), struct vf610_nfc, chip); } +static inline struct vf610_nfc *chip_to_nfc(struct nand_chip *chip) +{ + return container_of(chip, struct vf610_nfc, chip); +} + static inline u32 vf610_nfc_read(struct vf610_nfc *nfc, uint reg) { return readl(nfc->regs + reg); @@ -214,7 +220,6 @@ static inline void vf610_nfc_memcpy(void *dst, const void __iomem *src, memcpy(dst, src, n); } -/* Clear flags for upcoming command */ static inline void vf610_nfc_clear_status(struct vf610_nfc *nfc) { u32 tmp = vf610_nfc_read(nfc, NFC_IRQ_STATUS); @@ -243,51 +248,22 @@ static void vf610_nfc_done(struct vf610_nfc *nfc) vf610_nfc_clear_status(nfc); } -static u8 vf610_nfc_get_id(struct vf610_nfc *nfc, int col) -{ - u32 flash_id; - - if (col < 4) { - flash_id = vf610_nfc_read(nfc, NFC_FLASH_STATUS1); - flash_id >>= (3 - col) * 8; - } else { - flash_id = vf610_nfc_read(nfc, NFC_FLASH_STATUS2); - flash_id >>= 24; - } - - return flash_id & 0xff; -} - -static u8 vf610_nfc_get_status(struct vf610_nfc *nfc) +static void vf610_nfc_send_code(struct vf610_nfc *nfc, u32 cmd_code) { - return vf610_nfc_read(nfc, NFC_FLASH_STATUS2) & STATUS_BYTE1_MASK; + vf610_nfc_set_field(nfc, NFC_FLASH_CMD2, CMD_CODE_MASK, CMD_CODE_SHIFT, + cmd_code); } -static void vf610_nfc_send_command(struct vf610_nfc *nfc, u32 cmd_byte1, - u32 cmd_code) +static void vf610_nfc_set_cmd1(struct vf610_nfc *nfc, u32 cmd_byte1) { - u32 tmp; - - vf610_nfc_clear_status(nfc); - - tmp = vf610_nfc_read(nfc, NFC_FLASH_CMD2); - tmp &= ~(CMD_BYTE1_MASK | CMD_CODE_MASK | BUFNO_MASK); - tmp |= cmd_byte1 << CMD_BYTE1_SHIFT; - tmp |= cmd_code << CMD_CODE_SHIFT; - vf610_nfc_write(nfc, NFC_FLASH_CMD2, tmp); + vf610_nfc_set_field(nfc, NFC_FLASH_CMD2, CMD_BYTE1_MASK, + CMD_BYTE1_SHIFT, cmd_byte1); } -static void vf610_nfc_send_commands(struct vf610_nfc *nfc, u32 cmd_byte1, - u32 cmd_byte2, u32 cmd_code) +static void vf610_nfc_set_cmd2(struct vf610_nfc *nfc, u32 cmd_byte2) { - u32 tmp; - - vf610_nfc_send_command(nfc, cmd_byte1, cmd_code); - - tmp = vf610_nfc_read(nfc, NFC_FLASH_CMD1); - tmp &= ~CMD_BYTE2_MASK; - tmp |= cmd_byte2 << CMD_BYTE2_SHIFT; - vf610_nfc_write(nfc, NFC_FLASH_CMD1, tmp); + vf610_nfc_set_field(nfc, NFC_FLASH_CMD1, CMD_BYTE2_MASK, + CMD_BYTE2_SHIFT, cmd_byte2); } static irqreturn_t vf610_nfc_irq(int irq, void *data) @@ -301,19 +277,6 @@ static irqreturn_t vf610_nfc_irq(int irq, void *data) return IRQ_HANDLED; } -static void vf610_nfc_addr_cycle(struct vf610_nfc *nfc, int column, int page) -{ - if (column != -1) { - if (nfc->chip.options & NAND_BUSWIDTH_16) - column = column / 2; - vf610_nfc_set_field(nfc, NFC_COL_ADDR, COL_ADDR_MASK, - COL_ADDR_SHIFT, column); - } - if (page != -1) - vf610_nfc_set_field(nfc, NFC_ROW_ADDR, ROW_ADDR_MASK, - ROW_ADDR_SHIFT, page); -} - static inline void vf610_nfc_ecc_mode(struct vf610_nfc *nfc, int ecc_mode) { vf610_nfc_set_field(nfc, NFC_FLASH_CONFIG, @@ -326,167 +289,158 @@ static inline void vf610_nfc_transfer_size(struct vf610_nfc *nfc, int size) vf610_nfc_write(nfc, NFC_SECTOR_SIZE, size); } -static void vf610_nfc_command(struct mtd_info *mtd, unsigned command, - int column, int page) +static inline const struct nand_op_instr *vf610_get_next_instr( + const struct nand_operation *op, int *op_id) { - struct vf610_nfc *nfc = mtd_to_nfc(mtd); - int trfr_sz = nfc->chip.options & NAND_BUSWIDTH_16 ? 1 : 0; + if (*op_id + 1 >= op->ninstrs) + return NULL; - nfc->buf_offset = max(column, 0); - nfc->alt_buf = ALT_BUF_DATA; + return &op->instrs[++(*op_id)]; +} - switch (command) { - case NAND_CMD_SEQIN: - /* Use valid column/page from preread... */ - vf610_nfc_addr_cycle(nfc, column, page); - nfc->buf_offset = 0; +static int vf610_nfc_exec_op(struct nand_chip *chip, + const struct nand_operation *op, + bool check_only) +{ + const struct nand_op_instr *instr; + struct vf610_nfc *nfc = chip_to_nfc(chip); + struct mtd_info *mtd = nand_to_mtd(chip); + int op_id = -1, trfr_sz = 0, code = 0; - /* - * SEQIN => data => PAGEPROG sequence is done by the controller - * hence we do not need to issue the command here... - */ - return; - case NAND_CMD_PAGEPROG: - trfr_sz += nfc->write_sz; - vf610_nfc_transfer_size(nfc, trfr_sz); - vf610_nfc_send_commands(nfc, NAND_CMD_SEQIN, - command, PROGRAM_PAGE_CMD_CODE); - if (nfc->use_hw_ecc) - vf610_nfc_ecc_mode(nfc, nfc->ecc_mode); - else - vf610_nfc_ecc_mode(nfc, ECC_BYPASS); - break; - - case NAND_CMD_RESET: - vf610_nfc_transfer_size(nfc, 0); - vf610_nfc_send_command(nfc, command, RESET_CMD_CODE); - break; - - case NAND_CMD_READOOB: - trfr_sz += mtd->oobsize; - column = mtd->writesize; - vf610_nfc_transfer_size(nfc, trfr_sz); - vf610_nfc_send_commands(nfc, NAND_CMD_READ0, - NAND_CMD_READSTART, READ_PAGE_CMD_CODE); - vf610_nfc_addr_cycle(nfc, column, page); - vf610_nfc_ecc_mode(nfc, ECC_BYPASS); - break; - - case NAND_CMD_READ0: - trfr_sz += mtd->writesize + mtd->oobsize; - vf610_nfc_transfer_size(nfc, trfr_sz); - vf610_nfc_send_commands(nfc, NAND_CMD_READ0, - NAND_CMD_READSTART, READ_PAGE_CMD_CODE); - vf610_nfc_addr_cycle(nfc, column, page); - vf610_nfc_ecc_mode(nfc, nfc->ecc_mode); - break; - - case NAND_CMD_PARAM: - nfc->alt_buf = ALT_BUF_ONFI; - trfr_sz = 3 * sizeof(struct nand_onfi_params); - vf610_nfc_transfer_size(nfc, trfr_sz); - vf610_nfc_send_command(nfc, command, READ_ONFI_PARAM_CMD_CODE); - vf610_nfc_addr_cycle(nfc, -1, column); - vf610_nfc_ecc_mode(nfc, ECC_BYPASS); - break; - - case NAND_CMD_ERASE1: - vf610_nfc_transfer_size(nfc, 0); - vf610_nfc_send_commands(nfc, command, - NAND_CMD_ERASE2, ERASE_CMD_CODE); - vf610_nfc_addr_cycle(nfc, column, page); - break; - - case NAND_CMD_READID: - nfc->alt_buf = ALT_BUF_ID; - nfc->buf_offset = 0; - vf610_nfc_transfer_size(nfc, 0); - vf610_nfc_send_command(nfc, command, READ_ID_CMD_CODE); - vf610_nfc_addr_cycle(nfc, -1, column); - break; - - case NAND_CMD_STATUS: - nfc->alt_buf = ALT_BUF_STAT; - vf610_nfc_transfer_size(nfc, 0); - vf610_nfc_send_command(nfc, command, STATUS_READ_CMD_CODE); - break; - default: - return; + + /* Some ops are optional, but they need to be in order */ + instr = vf610_get_next_instr(op, &op_id); + if (!instr) + return -EINVAL; + + if (instr && instr->type == NAND_OP_CMD_INSTR) { + dev_dbg(nfc->dev, "OP_CMD: code 0x%02x\n", instr->ctx.cmd.opcode); + vf610_nfc_set_cmd1(nfc, instr->ctx.cmd.opcode); + code |= COMMAND_CMD_BYTE1; + + instr = vf610_get_next_instr(op, &op_id); } - vf610_nfc_done(nfc); + if (instr && instr->type == NAND_OP_ADDR_INSTR) { + int addr = 0; + u32 col = 0, row; - nfc->use_hw_ecc = false; - nfc->write_sz = 0; -} + if (instr->ctx.addr.naddrs > 1) { + col = COL_ADDR_BYTE1(instr->ctx.addr.addrs[addr++]); + code |= COMMAND_CAR_BYTE1; -static void vf610_nfc_read_buf(struct mtd_info *mtd, u_char *buf, int len) -{ - struct vf610_nfc *nfc = mtd_to_nfc(mtd); - uint c = nfc->buf_offset; + if (mtd->writesize > 512) { + col |= COL_ADDR_BYTE2(instr->ctx.addr.addrs[addr++]); + code |= COMMAND_CAR_BYTE2; + } + } - /* Alternate buffers are only supported through read_byte */ - WARN_ON(nfc->alt_buf); + row = ROW_ADDR_BYTE1(instr->ctx.addr.addrs[addr++]); + code |= COMMAND_RAR_BYTE1; + if (addr < instr->ctx.addr.naddrs) { + row |= ROW_ADDR_BYTE2(instr->ctx.addr.addrs[addr++]); + code |= COMMAND_RAR_BYTE2; + } + if (addr < instr->ctx.addr.naddrs) { + row |= ROW_ADDR_BYTE3(instr->ctx.addr.addrs[addr++]); + code |= COMMAND_RAR_BYTE3; + } - vf610_nfc_memcpy(buf, nfc->regs + NFC_MAIN_AREA(0) + c, len); + dev_dbg(nfc->dev, "OP_ADDR: col %d, row %d\n", col, row); + vf610_nfc_set_field(nfc, NFC_COL_ADDR, COL_ADDR_MASK, + COL_ADDR_SHIFT, col); - nfc->buf_offset += len; -} + vf610_nfc_set_field(nfc, NFC_ROW_ADDR, ROW_ADDR_MASK, + ROW_ADDR_SHIFT, row); -static void vf610_nfc_write_buf(struct mtd_info *mtd, const uint8_t *buf, - int len) -{ - struct vf610_nfc *nfc = mtd_to_nfc(mtd); - uint c = nfc->buf_offset; - uint l; + instr = vf610_get_next_instr(op, &op_id); + } - l = min_t(uint, len, mtd->writesize + mtd->oobsize - c); - vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + c, buf, l); + if (instr && instr->type == NAND_OP_DATA_OUT_INSTR) { + int len = instr->ctx.data.len; + int offset = 0; - nfc->write_sz += l; - nfc->buf_offset += l; -} + dev_dbg(nfc->dev, "OP_DATA_OUT: len %d, offset %d\n", len, offset); + + vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + offset, + instr->ctx.data.buf.in + offset, + len); + code |= COMMAND_WRITE_DATA; + trfr_sz += len; + + instr = vf610_get_next_instr(op, &op_id); + } + + if (instr && instr->type == NAND_OP_CMD_INSTR) { + vf610_nfc_set_cmd2(nfc, instr->ctx.cmd.opcode); + code |= COMMAND_CMD_BYTE2; + + instr = vf610_get_next_instr(op, &op_id); + } + + if (instr && instr->type == NAND_OP_WAITRDY_INSTR) { + //dev_dbg(nfc->dev, "WAITRDY [max %d ms]\n", + // instr->ctx.waitrdy.timeout_ms); + code |= COMMAND_RB_HANDSHAKE; + + instr = vf610_get_next_instr(op, &op_id); + } + + if (instr && instr->type == NAND_OP_DATA_IN_INSTR) { + int len = instr->ctx.data.len; + code |= COMMAND_READ_DATA; + trfr_sz += len; + } + + if (nfc->use_hw_ecc) { + vf610_nfc_ecc_mode(nfc, nfc->ecc_mode); + + /* Always transfer complete page with OOB when using HW ECC */ + trfr_sz = mtd->writesize + mtd->oobsize; + } else { + vf610_nfc_ecc_mode(nfc, ECC_BYPASS); + } + vf610_nfc_transfer_size(nfc, trfr_sz); + vf610_nfc_send_code(nfc, code); + + dev_dbg(nfc->dev, "Start: code: 0x%04x, hw ecc: %d, trfr_sz %d\n", + code, nfc->use_hw_ecc, trfr_sz); + vf610_nfc_done(nfc); + + if (instr && instr->type == NAND_OP_DATA_IN_INSTR) { + int len = instr->ctx.data.len; + int offset = 0; + bool fix_byte_order = false; -static uint8_t vf610_nfc_read_byte(struct mtd_info *mtd) -{ - struct vf610_nfc *nfc = mtd_to_nfc(mtd); - u8 tmp; - uint c = nfc->buf_offset; - - switch (nfc->alt_buf) { - case ALT_BUF_ID: - tmp = vf610_nfc_get_id(nfc, c); - break; - case ALT_BUF_STAT: - tmp = vf610_nfc_get_status(nfc); - break; #ifdef __LITTLE_ENDIAN - case ALT_BUF_ONFI: - /* Reverse byte since the controller uses big endianness */ - c = nfc->buf_offset ^ 0x3; - /* fall-through */ + fix_byte_order = true; #endif - default: - tmp = *((u8 *)(nfc->regs + NFC_MAIN_AREA(0) + c)); - break; - } - nfc->buf_offset++; - return tmp; -} + dev_dbg(nfc->dev, "OP_DATA_IN: 8-bit: %d, len %d, offset %d\n", + instr->ctx.data.force_8bit , len, offset); -static u16 vf610_nfc_read_word(struct mtd_info *mtd) -{ - u16 tmp; + /* + * HACK: force_8bit indicates reading of the parameter, status + * or id data. The controllers seems to store data in big endian + * byte order to the buffers. Reverse on little endian machines. + */ + if (instr->ctx.data.force_8bit && fix_byte_order) { + u8 *buf = instr->ctx.data.buf.in; - vf610_nfc_read_buf(mtd, (u_char *)&tmp, sizeof(tmp)); - return tmp; -} + while (len--) { + int c = offset ^ 0x3; -/* If not provided, upper layers apply a fixed delay. */ -static int vf610_nfc_dev_ready(struct mtd_info *mtd) -{ - /* NFC handles R/B internally; always ready. */ - return 1; + *buf = *((u8 *)(nfc->regs + NFC_MAIN_AREA(0) + c)); + //dev_info(nfc->dev, "[%d]: %02x\n", offset, *buf); + buf++; offset++; + } + } else { + vf610_nfc_memcpy(instr->ctx.data.buf.in + offset, + nfc->regs + NFC_MAIN_AREA(0) + offset, + len); + } + } + + return 0; } /* @@ -511,21 +465,6 @@ static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip) vf610_nfc_write(nfc, NFC_ROW_ADDR, tmp); } -/* Count the number of 0's in buff up to max_bits */ -static inline int count_written_bits(uint8_t *buff, int size, int max_bits) -{ - uint32_t *buff32 = (uint32_t *)buff; - int k, written_bits = 0; - - for (k = 0; k < (size / 4); k++) { - written_bits += hweight32(~buff32[k]); - if (unlikely(written_bits > max_bits)) - break; - } - - return written_bits; -} - static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat, uint8_t *oob, int page) { @@ -542,8 +481,7 @@ static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat, return ecc_count; /* Read OOB without ECC unit enabled */ - vf610_nfc_command(mtd, NAND_CMD_READOOB, 0, page); - vf610_nfc_read_buf(mtd, oob, mtd->oobsize); + nand_read_page_op(&nfc->chip, page, mtd->writesize, oob, mtd->oobsize); /* * On an erased page, bit count (including OOB) should be zero or @@ -557,12 +495,17 @@ static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat, static int vf610_nfc_read_page(struct mtd_info *mtd, struct nand_chip *chip, uint8_t *buf, int oob_required, int page) { + struct vf610_nfc *nfc = mtd_to_nfc(mtd); int eccsize = chip->ecc.size; int stat; + nfc->use_hw_ecc = true; nand_read_page_op(chip, page, 0, buf, eccsize); if (oob_required) - vf610_nfc_read_buf(mtd, chip->oob_poi, mtd->oobsize); + vf610_nfc_memcpy(chip->oob_poi, + nfc->regs + NFC_MAIN_AREA(0) + mtd->writesize, + mtd->oobsize); + nfc->use_hw_ecc = false; stat = vf610_nfc_correct_data(mtd, buf, chip->oob_poi, page); @@ -579,16 +522,20 @@ static int vf610_nfc_write_page(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf, int oob_required, int page) { struct vf610_nfc *nfc = mtd_to_nfc(mtd); + int ret; - nand_prog_page_begin_op(chip, page, 0, buf, mtd->writesize); - if (oob_required) - vf610_nfc_write_buf(mtd, chip->oob_poi, mtd->oobsize); - - /* Always write whole page including OOB due to HW ECC */ nfc->use_hw_ecc = true; - nfc->write_sz = mtd->writesize + mtd->oobsize; + ret = nand_prog_page_op(chip, page, 0, buf, mtd->writesize); + nfc->use_hw_ecc = false; - return nand_prog_page_end_op(chip); + return ret; +} + +static int vf610_nfc_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, + const uint8_t *buf, int oob_required, int page) +{ + return nand_prog_page_op(chip, page, 0, buf, + mtd->writesize + oob_required ? mtd->oobsize : 0); } static const struct of_device_id vf610_nfc_dt_ids[] = { @@ -606,6 +553,9 @@ static void vf610_nfc_preinit_controller(struct vf610_nfc *nfc) vf610_nfc_clear(nfc, NFC_FLASH_CONFIG, CONFIG_DMA_REQ_BIT); vf610_nfc_set(nfc, NFC_FLASH_CONFIG, CONFIG_FAST_FLASH_BIT); + /* Make sure flags are cleared on startup */ + vf610_nfc_clear_status(nfc); + /* Disable virtual pages, only one elementary transfer unit */ vf610_nfc_set_field(nfc, NFC_FLASH_CONFIG, CONFIG_PAGE_CNT_MASK, CONFIG_PAGE_CNT_SHIFT, 1); @@ -695,15 +645,8 @@ static int vf610_nfc_probe(struct platform_device *pdev) goto err_clk; } - chip->dev_ready = vf610_nfc_dev_ready; - chip->cmdfunc = vf610_nfc_command; - chip->read_byte = vf610_nfc_read_byte; - chip->read_word = vf610_nfc_read_word; - chip->read_buf = vf610_nfc_read_buf; - chip->write_buf = vf610_nfc_write_buf; + chip->exec_op = vf610_nfc_exec_op; chip->select_chip = vf610_nfc_select_chip; - chip->onfi_set_features = nand_onfi_get_set_features_notsupp; - chip->onfi_get_features = nand_onfi_get_set_features_notsupp; chip->options |= NAND_NO_SUBPAGE_WRITE; @@ -770,6 +713,7 @@ static int vf610_nfc_probe(struct platform_device *pdev) chip->ecc.read_page = vf610_nfc_read_page; chip->ecc.write_page = vf610_nfc_write_page; + chip->ecc.write_page_raw = vf610_nfc_write_page_raw; chip->ecc.size = PAGE_2K; }
This reworks the driver to make use of ->exec_op() callback. The command sequencer of the VF610 NFC aligns well with the new ops interface. The ops are translated to a NFC command code while filling the necessary registers. Instead of using the special status and read id command codes (which require the status/id form special registers) the driver now uses the main data buffer for all commands. This simplifies the driver as no special casing is needed. For control data (status byte, id bytes and parameter page) the driver needs to reverse byte order for little endian CPUs since the controller seems to store the bytes in big endian order in the data buffer. The current state seems to pass MTD tests on a Colibri VF61. Signed-off-by: Stefan Agner <stefan@agner.ch> --- Hi, In this second version I was able to remove all special command handling. As Boris suggested, the special READID/STATUS handling is really not needed and all return values can be read from the regular data buffer. This simplified code even more and allowed to get rid of the hack in the MTD core. The driver is now able to handle all commands in a single function. Therefor I also got rid of the command parser, since everything is handled in a single function anyway. The only thing which is somewhat unfortunate is the byte ordering which needs to be reversed for commands which read flash data (id, status and parameter page). All those request 8-bit handling, so I used this as an indicator to fix byte order. I did meassured a slight performance drop (before vs. now): - write speed is 4036 KiB/s vs 3495 KiB/s - read speed is 13741 KiB/s vs 13490 KiB/s -- Stefan drivers/mtd/nand/vf610_nfc.c | 444 +++++++++++++++++++------------------------ 1 file changed, 194 insertions(+), 250 deletions(-)