| Message ID | 20180420081946.16088-12-sam.lefebvre@essensium.com |
|---|---|
| State | Superseded |
| Delegated to: | Boris Brezillon |
| Headers | show |
| Series | [01/18] mtd: nand: gpmi: drop dma_ops_type | expand |
Hi Sam, Arnout, On Fri, 20 Apr 2018 10:19:39 +0200 Sam Lefebvre <sam.lefebvre@essensium.com> wrote: > From: "Arnout Vandecappelle (Essensium/Mind)" <arnout@mind.be> > > Later patches will optimize the command handling for gpmi. This > requires a gpmmi-specific implementation of cmdfunc. As a first step, > nand_command() is copied literally from nand_base.c (after merging > nand_command() and nand_command_lp()). NACK. As said in my review of patch 10, we're trying to move a many drivers as possible to the ->exec_op() approach in order to ease maintenance of the NAND subsystem. What you're doing here is going in the wrong direction. Please work on a solution based on ->exec_op(). Regards, Boris
On 20-04-18 22:38, Boris Brezillon wrote: > Hi Sam, Arnout, > > On Fri, 20 Apr 2018 10:19:39 +0200 > Sam Lefebvre <sam.lefebvre@essensium.com> wrote: > >> From: "Arnout Vandecappelle (Essensium/Mind)" <arnout@mind.be> >> >> Later patches will optimize the command handling for gpmi. This >> requires a gpmmi-specific implementation of cmdfunc. As a first step, >> nand_command() is copied literally from nand_base.c (after merging >> nand_command() and nand_command_lp()). > > NACK. As said in my review of patch 10, we're trying to move a many > drivers as possible to the ->exec_op() approach in order to ease > maintenance of the NAND subsystem. What you're doing here is going in > the wrong direction. > > Please work on a solution based on ->exec_op(). The problem is that exec_op() is a revolutionary change, because you need to change *all* operations at the same time. I personally believe in a step-by-step approach rather than rewriting everything from scratch. That's why patches 11, 12, 13, 14, 15 are all split out, to show for each step how and why it works. In a step-by-step approach, I think that going through cmdfunc is a step towards exec_op. In terms of granularity, I think cmd_ctrl < cmdfunc < exec_op right? It also doesn't help that there is not much documentation of exec_op. There's Miquel's FOSDEM18 talk, there's the struct documentation, and there are just a few drivers that can serve as an example. In particular, I don't understand what happens when a pattern is not included in the nand_op_parser. It's also not clear to me how ECC is supposed to be integrated with it. For example, looking at the marvell driver, it seems the nand_base infra and exec_op are bypassed entirely for hwecc accesses. Finally, we are working on a budget that gets extended on a day-by-day basis, so we wanted to be able to push *something* within budget rather than not upstreaming anything at all. That said, what should be the next steps? I guess continuing with patch 19, 20, 21 using the same approach doesn't make a lot of sense since you're not going to merge it in this form. We can of course repost patches 1-8 with a fix in patch 7. We could start working on exec_op conversion, but there's a big chance that nothing will be ready for submission when the budget dries up, and we probably won't get to the . Any other ideas? If we do work on conversion to exec_op, what's the proper way to do this gradually? Regards, Arnout
Hi Arnout, On Mon, 23 Apr 2018 09:43:48 +0200 Arnout Vandecappelle <arnout@mind.be> wrote: > On 20-04-18 22:38, Boris Brezillon wrote: > > Hi Sam, Arnout, > > > > On Fri, 20 Apr 2018 10:19:39 +0200 > > Sam Lefebvre <sam.lefebvre@essensium.com> wrote: > > > >> From: "Arnout Vandecappelle (Essensium/Mind)" <arnout@mind.be> > >> > >> Later patches will optimize the command handling for gpmi. This > >> requires a gpmmi-specific implementation of cmdfunc. As a first step, > >> nand_command() is copied literally from nand_base.c (after merging > >> nand_command() and nand_command_lp()). > > > > NACK. As said in my review of patch 10, we're trying to move a many > > drivers as possible to the ->exec_op() approach in order to ease > > maintenance of the NAND subsystem. What you're doing here is going in > > the wrong direction. > > > > Please work on a solution based on ->exec_op(). > > The problem is that exec_op() is a revolutionary change, because you need to > change *all* operations at the same time. I personally believe in a step-by-step > approach rather than rewriting everything from scratch. That's why patches 11, > 12, 13, 14, 15 are all split out, to show for each step how and why it works. Well, that's also one of the thing I don't like in this series. Copying things from the core just to remove part of the logic afterwards is not a good idea. But to be honest, that's not my biggest concern right now. > > In a step-by-step approach, I think that going through cmdfunc is a step > towards exec_op. In terms of granularity, I think cmd_ctrl < cmdfunc < exec_op > right? No it's not. ->cmdfunc() is just a pain to maintain, because every time we add a new operation in the core we have to patch all drivers that implement ->cmdfunc() on their own. For me (as a NAND maintainer) it's: cmdfunc < cmd_ctrl < exec_op > > It also doesn't help that there is not much documentation of exec_op. This I agree with. > There's > Miquel's FOSDEM18 talk, there's the struct documentation, and there are just a > few drivers that can serve as an example. In particular, I don't understand what > happens when a pattern is not included in the nand_op_parser. It's also not > clear to me how ECC is supposed to be integrated with it. For example, looking > at the marvell driver, it seems the nand_base infra and exec_op are bypassed > entirely for hwecc accesses. Write and read path are where we care about optimizations, hence the specific logic. If you want to optimize this path, maybe you should try to overload the chip->ecc.read/write_page() functions. > > Finally, we are working on a budget that gets extended on a day-by-day basis, > so we wanted to be able to push *something* within budget rather than not > upstreaming anything at all. Except getting from ->cmd_ctrl() to ->cmdfunc() is like going one step back to me, so that's not something I'm willing to accept. > > That said, what should be the next steps? I guess continuing with patch 19, 20, > 21 using the same approach doesn't make a lot of sense since you're not going to > merge it in this form. We can of course repost patches 1-8 with a fix in patch > 7. We could start working on exec_op conversion, but there's a big chance that > nothing will be ready for submission when the budget dries up, and we probably > won't get to the . Any other ideas? If you don't want/can't invest time on ->exec_op(), I'd suggest overloading ->read/write_page(). > > If we do work on conversion to exec_op, what's the proper way to do this gradually? Not sure what you mean by gradually, but you can have a look at how miquel converted the FSMC driver from ->cmd_ctrl() to ->exec_op() [1]. Regards, Boris [1]https://patchwork.ozlabs.org/cover/874445/
diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c index 3da4c07ce2d9..69bdee0ca679 100644 --- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c +++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c @@ -19,11 +19,13 @@ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ #include <linux/clk.h> +#include <linux/delay.h> #include <linux/slab.h> #include <linux/sched/task_stack.h> #include <linux/interrupt.h> #include <linux/module.h> #include <linux/mtd/partitions.h> +#include <linux/nmi.h> #include <linux/of.h> #include <linux/of_device.h> #include "gpmi-nand.h" @@ -1091,6 +1093,208 @@ static int gpmi_ecc_read_page_data(struct nand_chip *chip, return max_bitflips; } +static void gpmi_wait_status_ready(struct mtd_info *mtd, unsigned long timeo) +{ + register struct nand_chip *chip = mtd_to_nand(mtd); + int ret; + + timeo = jiffies + msecs_to_jiffies(timeo); + do { + u8 status; + + ret = nand_read_data_op(chip, &status, sizeof(status), true); + if (ret) + return; + + if (status & NAND_STATUS_READY) + break; + touch_softlockup_watchdog(); + } while (time_before(jiffies, timeo)); +}; + +static void gpmi_ccs_delay(struct nand_chip *chip) +{ + /* + * The controller already takes care of waiting for tCCS when the RNDIN + * or RNDOUT command is sent, return directly. + */ + if (!(chip->options & NAND_WAIT_TCCS)) + return; + + /* + * Wait tCCS_min if it is correctly defined, otherwise wait 500ns + * (which should be safe for all NANDs). + */ + if (chip->setup_data_interface) + ndelay(chip->data_interface.timings.sdr.tCCS_min / 1000); + else + ndelay(500); +} + +/** + * gpmi_nand_command - Send command to NAND device + * @mtd: MTD device structure + * @command: the command to be sent + * @column: the column address for this command, -1 if none + * @page_addr: the page address for this command, -1 if none + * + * Send command to NAND device. + */ +static void gpmi_nand_command(struct mtd_info *mtd, unsigned int command, + int column, int page_addr) +{ + register struct nand_chip *chip = mtd_to_nand(mtd); + int ctrl = NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE; + /* Large page devices (> 512 bytes) behave slightly differently. */ + bool is_lp = mtd->writesize > 512; + + if (is_lp) { + /* Large page devices don't have the separate regions as we + * have in the small page devices. We must emulate + * NAND_CMD_READOOB to keep the code compatible. + */ + if (command == NAND_CMD_READOOB) { + column += mtd->writesize; + command = NAND_CMD_READ0; + } + } else if (command == NAND_CMD_SEQIN) { + /* Write out the command to the device */ + int readcmd; + + if (column >= mtd->writesize) { + /* OOB area */ + column -= mtd->writesize; + readcmd = NAND_CMD_READOOB; + } else if (column < 256) { + /* First 256 bytes --> READ0 */ + readcmd = NAND_CMD_READ0; + } else { + column -= 256; + readcmd = NAND_CMD_READ1; + } + chip->cmd_ctrl(mtd, readcmd, ctrl); + ctrl &= ~NAND_CTRL_CHANGE; + } + + /* Command latch cycle */ + if (command != NAND_CMD_NONE) + chip->cmd_ctrl(mtd, command, ctrl); + + /* Address cycle, when necessary */ + ctrl = NAND_NCE | NAND_ALE | NAND_CTRL_CHANGE; + /* Serially input address */ + if (column != -1) { + /* Adjust columns for 16 bit buswidth */ + if (chip->options & NAND_BUSWIDTH_16 && + !nand_opcode_8bits(command)) + column >>= 1; + chip->cmd_ctrl(mtd, column, ctrl); + ctrl &= ~NAND_CTRL_CHANGE; + + /* Only output a single addr cycle for 8bits opcodes. */ + if (is_lp && !nand_opcode_8bits(command)) + chip->cmd_ctrl(mtd, column >> 8, ctrl); + } + if (page_addr != -1) { + chip->cmd_ctrl(mtd, page_addr, ctrl); + ctrl &= ~NAND_CTRL_CHANGE; + chip->cmd_ctrl(mtd, page_addr >> 8, ctrl); + if (chip->options & NAND_ROW_ADDR_3) + chip->cmd_ctrl(mtd, page_addr >> 16, ctrl); + } + chip->cmd_ctrl(mtd, NAND_CMD_NONE, NAND_NCE | NAND_CTRL_CHANGE); + + /* + * Program and erase have their own busy handlers status, sequential + * in and status need no delay. + */ + switch (command) { + + case NAND_CMD_NONE: + case NAND_CMD_PAGEPROG: + case NAND_CMD_ERASE1: + case NAND_CMD_ERASE2: + case NAND_CMD_SEQIN: + case NAND_CMD_STATUS: + case NAND_CMD_READID: + case NAND_CMD_SET_FEATURES: + return; + + case NAND_CMD_CACHEDPROG: + if (is_lp) + return; + break; + + case NAND_CMD_RNDIN: + if (is_lp) { + gpmi_ccs_delay(chip); + return; + } + break; + + case NAND_CMD_RESET: + if (chip->dev_ready) + break; + udelay(chip->chip_delay); + chip->cmd_ctrl(mtd, NAND_CMD_STATUS, + NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE); + chip->cmd_ctrl(mtd, NAND_CMD_NONE, + NAND_NCE | NAND_CTRL_CHANGE); + /* EZ-NAND can take upto 250ms as per ONFi v4.0 */ + gpmi_wait_status_ready(mtd, 250); + return; + + case NAND_CMD_RNDOUT: + if (is_lp) { + /* No ready / busy check necessary */ + chip->cmd_ctrl(mtd, NAND_CMD_RNDOUTSTART, + NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE); + chip->cmd_ctrl(mtd, NAND_CMD_NONE, + NAND_NCE | NAND_CTRL_CHANGE); + + gpmi_ccs_delay(chip); + return; + } + break; + + case NAND_CMD_READ0: + /* + * READ0 is sometimes used to exit GET STATUS mode. When this + * is the case no address cycles are requested, and we can use + * this information to detect that that we should not wait for + * the device to be ready and READSTART should not be issued. + */ + if (column == -1 && page_addr == -1) + return; + + if (is_lp) { + chip->cmd_ctrl(mtd, NAND_CMD_READSTART, + NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE); + chip->cmd_ctrl(mtd, NAND_CMD_NONE, + NAND_NCE | NAND_CTRL_CHANGE); + } + /* Read commands must wait */ + break; + } + /* + * If we don't have access to the busy pin, we apply the given command + * delay. + */ + if (!chip->dev_ready) { + udelay(chip->chip_delay); + return; + } + + /* + * Apply this short delay always to ensure that we do wait tWB in + * any case on any machine. + */ + ndelay(100); + + nand_wait_ready(mtd); +} + + static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip, uint8_t *buf, int oob_required, int page) { @@ -1895,6 +2099,7 @@ static int gpmi_nand_init(struct gpmi_nand_data *this) chip->select_chip = gpmi_select_chip; chip->setup_data_interface = gpmi_setup_data_interface; chip->cmd_ctrl = gpmi_cmd_ctrl; + chip->cmdfunc = gpmi_nand_command; chip->dev_ready = gpmi_dev_ready; chip->read_byte = gpmi_read_byte; chip->read_buf = gpmi_read_buf;