Message ID | 20171106220704.2340-1-miquel.raynal@free-electrons.com |
---|---|
State | Superseded |
Delegated to: | Boris Brezillon |
Headers | show |
Series | mtd: nand: fix interpretation of NAND_CMD_NONE in core ->cmdfunc() | expand |
Hi Miquel, I would change the subject to: mtd: nand: fix interpretation of NAND_CMD_NONE in nand_command[_lp]() On Mon, 6 Nov 2017 23:07:04 +0100 Miquel Raynal <miquel.raynal@free-electrons.com> wrote: > Some drivers (like nand_hynix.c) call ->cmdfunc() with NAND_CMD_NONE > and an address byte in order to only send one address cycle to the > flash chip. > > Fix the current implementation that actually send NAND_CMD_NONE, which > is defined as -1 (cast in a u8), thus sending an 0xFF command to the > chip, which is actually a reset command. Well, that's not exactly true, if any, the cast is done in the ->cmd_ctrl() implementation, not in nand_command[_lp](). > > Add the condition in both nand_command() and nand_command_lp() to > avoid calling ->cmd_ctrl() in that case. How about rewording it like that: " Some drivers (like nand_hynix.c) call ->cmdfunc() with NAND_CMD_NONE and a column address and expect the controller to only send address cycles. Right now, the default ->cmdfunc() implementations provided by the core do not filter out the command cycle in this case and forwards the request to the controller driver through the ->cmd_ctrl() method. The thing is, NAND controller drivers can get is wrong and send a command cycle with a NAND_CMD_NONE opcode and since NAND_CMD_NONE is -1, and the command field is usually casted to an u8, we end up sending the 0xFF command which is actually a RESET operation. Add conditions in nand_command[_lp]() functions to sending the initial command cycle when command == NAND_CMD_NONE. " > > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com> > --- > drivers/mtd/nand/nand_base.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/nand/nand_base.c > b/drivers/mtd/nand/nand_base.c index c63e4a88a653..851f25383622 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -710,7 +710,8 @@ static void nand_command(struct mtd_info *mtd, > unsigned int command, chip->cmd_ctrl(mtd, readcmd, ctrl); > ctrl &= ~NAND_CTRL_CHANGE; > } > - chip->cmd_ctrl(mtd, command, ctrl); > + if (command != NAND_CMD_NONE) > + chip->cmd_ctrl(mtd, command, ctrl); > > /* Address cycle, when necessary */ > ctrl = NAND_CTRL_ALE | NAND_CTRL_CHANGE; You should probably also add a new case in the switch-case block to bail-out when command == NAND_CMD_NONE. > @@ -831,7 +832,9 @@ static void nand_command_lp(struct mtd_info *mtd, > unsigned int command, } > > /* Command latch cycle */ > - chip->cmd_ctrl(mtd, command, NAND_NCE | NAND_CLE | > NAND_CTRL_CHANGE); > + if (command != NAND_CMD_NONE) > + chip->cmd_ctrl(mtd, command, > + NAND_NCE | NAND_CLE | > NAND_CTRL_CHANGE); > if (column != -1 || page_addr != -1) { > int ctrl = NAND_CTRL_CHANGE | NAND_NCE | NAND_ALE; Ditto. Thanks, Boris
Hi Boris, > How about rewording it like that: > > " > Some drivers (like nand_hynix.c) call ->cmdfunc() with NAND_CMD_NONE > and a column address and expect the controller to only send address > cycles. Right now, the default ->cmdfunc() implementations provided by > the core do not filter out the command cycle in this case and forwards > the request to the controller driver through the ->cmd_ctrl() method. > The thing is, NAND controller drivers can get is wrong and send a > command cycle with a NAND_CMD_NONE opcode and since NAND_CMD_NONE is > -1, and the command field is usually casted to an u8, we end up > sending the 0xFF command which is actually a RESET operation. > > > Add conditions in nand_command[_lp]() functions to sending the initial > command cycle when command == NAND_CMD_NONE. > " This is better explained. I put yours, with s/drivers can get is wrong/drivers can get this wrong/ > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com> > > --- > > drivers/mtd/nand/nand_base.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mtd/nand/nand_base.c > > b/drivers/mtd/nand/nand_base.c index c63e4a88a653..851f25383622 > > 100644 --- a/drivers/mtd/nand/nand_base.c > > +++ b/drivers/mtd/nand/nand_base.c > > @@ -710,7 +710,8 @@ static void nand_command(struct mtd_info *mtd, > > unsigned int command, chip->cmd_ctrl(mtd, readcmd, ctrl); > > ctrl &= ~NAND_CTRL_CHANGE; > > } > > - chip->cmd_ctrl(mtd, command, ctrl); > > + if (command != NAND_CMD_NONE) > > + chip->cmd_ctrl(mtd, command, ctrl); > > > > /* Address cycle, when necessary */ > > ctrl = NAND_CTRL_ALE | NAND_CTRL_CHANGE; > > You should probably also add a new case in the switch-case block to > bail-out when command == NAND_CMD_NONE. Oh that is right, there is nothing to wait for in that case. Thank you, Miquèl
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index c63e4a88a653..851f25383622 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -710,7 +710,8 @@ static void nand_command(struct mtd_info *mtd, unsigned int command, chip->cmd_ctrl(mtd, readcmd, ctrl); ctrl &= ~NAND_CTRL_CHANGE; } - chip->cmd_ctrl(mtd, command, ctrl); + if (command != NAND_CMD_NONE) + chip->cmd_ctrl(mtd, command, ctrl); /* Address cycle, when necessary */ ctrl = NAND_CTRL_ALE | NAND_CTRL_CHANGE; @@ -831,7 +832,9 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command, } /* Command latch cycle */ - chip->cmd_ctrl(mtd, command, NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE); + if (command != NAND_CMD_NONE) + chip->cmd_ctrl(mtd, command, + NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE); if (column != -1 || page_addr != -1) { int ctrl = NAND_CTRL_CHANGE | NAND_NCE | NAND_ALE;
Some drivers (like nand_hynix.c) call ->cmdfunc() with NAND_CMD_NONE and an address byte in order to only send one address cycle to the flash chip. Fix the current implementation that actually send NAND_CMD_NONE, which is defined as -1 (cast in a u8), thus sending an 0xFF command to the chip, which is actually a reset command. Add the condition in both nand_command() and nand_command_lp() to avoid calling ->cmd_ctrl() in that case. Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com> --- drivers/mtd/nand/nand_base.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)