[RFC PATCH v3 2/3] mtd: nand: vf610_nfc: make use of ->exec_op()
Stefan Agner
stefan at agner.ch
Wed Feb 21 00:30:32 PST 2018
On 21.02.2018 08:18, Boris Brezillon wrote:
> On Wed, 21 Feb 2018 00:15:18 +0100
> Stefan Agner <stefan at agner.ch> wrote:
>
>> On 11.02.2018 11:54, Boris Brezillon wrote:
>> > On Fri, 9 Feb 2018 00:59:20 +0100
>> > Stefan Agner <stefan at 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 at agner.ch>
>> >> ---
>> >> drivers/mtd/nand/vf610_nfc.c | 279 +++++++++++++++++++++++++++++++++++++++++--
>> >> 1 file changed, 267 insertions(+), 12 deletions(-)
>> >>
>> >> diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
>> >> index 2fa61cbdbaf7..eae95877422d 100644
>> >> --- a/drivers/mtd/nand/vf610_nfc.c
>> >> +++ b/drivers/mtd/nand/vf610_nfc.c
>> >> @@ -74,6 +74,21 @@
>> >> #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
>> >> #define ECC_45_BYTE 6
>> >> @@ -97,10 +112,14 @@
>> >> /* NFC_COL_ADDR Field */
>> >> #define COL_ADDR_MASK 0x0000FFFF
>> >> #define COL_ADDR_SHIFT 0
>> >> +#define COL_ADDR(pos, val) ((val & 0xFF) << (8 * (pos)))
>> >> +
>> >>
>> >> /* NFC_ROW_ADDR Field */
>> >> #define ROW_ADDR_MASK 0x00FFFFFF
>> >> #define ROW_ADDR_SHIFT 0
>> >> +#define ROW_ADDR(pos, val) ((val & 0xFF) << (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
>> >> @@ -173,6 +192,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);
>> >> @@ -489,6 +513,184 @@ static int vf610_nfc_dev_ready(struct mtd_info *mtd)
>> >> return 1;
>> >> }
>> >>
>> >> +static inline void vf610_nfc_run(struct vf610_nfc *nfc, u32 col, u32 row, u32 cmd1, u32 cmd2, u32 trfr_sz)
>> >
>> > Nitpick: please wrap the line to make checkpatch happy.
>> >
>>
>> Ok.
>>
>> >> +{
>> >> + vf610_nfc_set_field(nfc, NFC_COL_ADDR, COL_ADDR_MASK,
>> >> + COL_ADDR_SHIFT, col);
>> >> +
>> >> + vf610_nfc_set_field(nfc, NFC_ROW_ADDR, ROW_ADDR_MASK,
>> >> + ROW_ADDR_SHIFT, row);
>> >> +
>> >> + vf610_nfc_write(nfc, NFC_SECTOR_SIZE, trfr_sz);
>> >> + vf610_nfc_write(nfc, NFC_FLASH_CMD1, cmd1);
>> >> + vf610_nfc_write(nfc, NFC_FLASH_CMD2, cmd2);
>> >> +
>> >> + dev_dbg(nfc->dev, "col 0x%08x, row 0x%08x, cmd1 0x%08x, cmd2 0x%08x, trfr_sz %d\n",
>> >> + col, row, cmd1, cmd2, trfr_sz);
>> >> +
>> >> + vf610_nfc_done(nfc);
>> >> +}
>> >> +
>> >> +static inline const struct nand_op_instr *vf610_get_next_instr(
>> >> + const struct nand_subop *subop, int *op_id)
>> >> +{
>> >> + if (*op_id + 1 >= subop->ninstrs)
>> >> + return NULL;
>> >> +
>> >> + (*op_id)++;
>> >> +
>> >> + return &subop->instrs[*op_id];
>> >> +}
>> >> +
>> >> +static int vf610_nfc_cmd(struct nand_chip *chip,
>> >> + const struct nand_subop *subop)
>> >> +{
>> >> + 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, addr = 0, trfr_sz = 0;
>> >> + u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;
>> >> +
>> >> + /* Some ops are optional, but they need to be in order */
>> >> + instr = vf610_get_next_instr(subop, &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);
>> >> + cmd2 |= instr->ctx.cmd.opcode << CMD_BYTE1_SHIFT;
>> >> + code |= COMMAND_CMD_BYTE1;
>> >> +
>> >> + instr = vf610_get_next_instr(subop, &op_id);
>> >> + }
>> >> +
>> >> + if (instr && instr->type == NAND_OP_ADDR_INSTR) {
>> >> +
>> >
>> > Hm, you're still checking the sequencing consistency here. This should
>> > have been taken care by the core parser already, so really, this is not
>> > needed.
>> >
>>
>> Since those operations are optional, I have to check...
>
> Looks like my previous pastebin was not containing all the changes I
> wanted to share with you, so here is the full version [1].
>
> Hm, what do these checks add to the checks done in the parser? Even if
> all instructions are optional in your pattern the core parser still
> check that when they appear in the proper order. So, you can for
> example never have CMD1+CMD2+ADDR*5 passed to vf610_nfc_cmd().
Sure...
>
> The only exception I can think of is when you have DATA_OUT+CMD. In
> this case, you probably want to skip CMD_BYTE1 and use CMD_BYTE2
> directly, but that can easily be taken care of in the alternative
> implementation I proposed:
>
> if (instr->type == NAND_OP_DATA_OUT_INSTR) {
> /*
> * If there was no CMD instruction before the
> * DATA_OUT one, we must skip CMD_BYTE1 and use
> * CMD_BYTE2 directly so that the CMD cycle is
> * really placed after the DATA_OUT one.
> */
> if (!ncmds)
> ncmds++;
> ....
> }
>
>
I fully understand, I do not have to enforce order since I can rely on
the order passed by the stack.
It is also not what I am after. I just check which operation is going to
be next. Like your switch statement.
I just see don't see a real advantage in using a for loop. It makes it
harder to read, since the order of operations will no more be obvious.
It makes this ncmd work around necessary.
Using a for loop just because we can? I haven't seen a convincing
argument so far.
>>
>> >> + if (instr->ctx.addr.naddrs > 1) {
>> >> + col = COL_ADDR(0, instr->ctx.addr.addrs[addr++]);
>> >> + code |= COMMAND_CAR_BYTE1;
>> >> +
>> >> + if (mtd->writesize > 512) {
>> >> + col |= COL_ADDR(1, instr->ctx.addr.addrs[addr++]);
>> >> + code |= COMMAND_CAR_BYTE2;
>> >> + }
>> >
>> > No, you shoudn't do that. Just put as many ADDR cycles as requested
>> > without trying to figure out if they fit in the column or row field. We
>> > really don't care here, as long as the ADDR cycles are issued on the
>> > bus.
>> >
>>
>> Ok, so on a bus level column/row really does not matter?
>
> Nope.
>
>> I wonder why
>> the controller makes a difference then? I remember that the controller
>> has auto increment feature, might that be the reason?
>
> Probably.
>
>> Afaik we cannot
>> use such a feature currently...?
>
> No.
>
>
>> >
>> >> + }
>> >> +
>> >> + return 0;
>> >> +}
>> >> +
>> >> +static const struct nand_op_parser vf610_nfc_op_parser = NAND_OP_PARSER(
>> >> + NAND_OP_PARSER_PATTERN(
>> >> + vf610_nfc_cmd,
>> >> + NAND_OP_PARSER_PAT_CMD_ELEM(true),
>> >> + NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5),
>> >> + NAND_OP_PARSER_PAT_DATA_OUT_ELEM(true, PAGE_2K + OOB_MAX),
>> >> + NAND_OP_PARSER_PAT_CMD_ELEM(true),
>> >> + NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
>> >> + NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, PAGE_2K + OOB_MAX)),
>> >
>> > Are you sure you can do both a read and a write in a single pass? I
>> > doubt this is the case, because the logic seems to share the same SRAM
>> > for both operations, so I'd recommend splitting in 2 patterns:
>> >
>>
>> Hm, yes I currently use the same buffer, so in the current state this
>> would not work. But the controller actually has four buffers, so I
>> *could* use different ones for reading and writing. But is reading and
>> writing in a single go something we need often that such an optimization
>> would make worthwhile?
>
> No, and that's why I suggest to define 2 patterns instead of merging
> everything in a single one.
>
Ok, will leave it using one buffer and use two patterns.
>>
>> >> +
>> >> + cmd1 |= NAND_CMD_READSTART << CMD_BYTE2_SHIFT;
>> >> + code |= COMMAND_CMD_BYTE2;
>> >> +
>> >> + code |= COMMAND_RB_HANDSHAKE;
>> >> + code |= COMMAND_READ_DATA;
>> >> + cmd2 |= code << CMD_CODE_SHIFT;
>> >> +
>> >> + vf610_nfc_ecc_mode(nfc, nfc->ecc_mode);
>> >> + vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
>> >> +
>> >> + vf610_nfc_memcpy(buf, nfc->regs + NFC_MAIN_AREA(0), mtd->writesize);
>> >> 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);
>> >>
>> >> stat = vf610_nfc_correct_data(mtd, buf, chip->oob_poi, page);
>> >
>> > You should disable ECC here and not in vf610_nfc_cmd():
>> >
>> > vf610_nfc_ecc_mode(nfc, ECC_BYPASS);
>> >
>> >
>>
>> Hm, but then we would have to also clear it at initialization time since
>> boot loader could have left it in any state.
>
> Yes.
>
>> I somewhat prefer the
>> current state....
>
> I don't :-). ->exec_op() is only supposed to handle NAND operations
> not extra things like ECC. Since you already enable ECC in this
> function it would be consistent to disable it before leaving the
> function and leave the NFC in a known state so that future NAND
> operations can be executed without taking extra precautions (like
> disabling ECC).
>
My initial thought was that configuring on every operation makes the
driver more stateless.... But give that also other fields are
initialized first, there is already considerable amount of state we rely
on. With that in mind, it makes sense to have disabled ECC as a default
state and have it only enabled in the ECC case. Will change that.
Any thoughts to the raw page read/write endianness issue?
>> I think you have the same endianness problem you have for the READ
>> path. For example, I doubt SET_FEATURES will work properly if you're
>> in LE. So I repeat my initial suggestion: always do the byte swapping
>> when you're transfering data to/from the SRAM from vf610_nfc_cmd()
>> and use vf610_nfc_memcpy() only in the ->read/write_page()
>> implementations.
>>
>
> Hm, but doesn't that leads to wrong order of data when using e.g. raw
> read/write page...?
With that last iteration I used the default implementation of the stack.
I guess I could just implement them too and use vf610_nfc_memcpy()?
This should be fine then for tdoay, but what if we have another data
related access in the future? It then also will make use of
vf610_nfc_cmd and change byte order...
--
Stefan
> Regards,
>
> Boris
>
> [1]http://code.bulix.org/njptts-286584
More information about the linux-mtd
mailing list