From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.bootlin.com ([62.4.15.54]) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1f9ckM-0002zV-0B for linux-mtd@lists.infradead.org; Fri, 20 Apr 2018 20:35:08 +0000 Date: Fri, 20 Apr 2018 22:34:54 +0200 From: Boris Brezillon To: Sam Lefebvre Cc: linux-mtd@lists.infradead.org, Han Xu , "Arnout Vandecappelle \(Essensium/Mind\)" Subject: Re: [PATCH 10/18] mtd: rawnand: factor nand_command_lp() into nand_command() Message-ID: <20180420223454.445a1858@bbrezillon> In-Reply-To: <20180420081946.16088-11-sam.lefebvre@essensium.com> References: <20180420081946.16088-1-sam.lefebvre@essensium.com> <20180420081946.16088-11-sam.lefebvre@essensium.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Hi Sam, Arnout, On Fri, 20 Apr 2018 10:19:38 +0200 Sam Lefebvre wrote: > From: "Arnout Vandecappelle (Essensium/Mind)" > > The nand_command() and nand_command_lp() functions are very similar. > Factor them into a single function, and use conditions on writesize to > identify the differences. > > is_lp checks are added everywhere to make sure the behaviour is exactly > the same as before. Most likely, the checks in CACHEDPROG, RNDIN and > RNDOUT are not needed since these commands are only valid for LP > devices. But since I'm not sure of that, I'm leaving it as is. > > The only side effect of this patch is that the large-page behaviour is > activated a little bit earlier: as soon as writesize is set. However, > only SEQIN, READOOB, READ0, CACHEDPROG, RNDIN and RNDOUT behave > differently between small and large page. Of these, only RNDOUT is used > in nand_detect(). RNDOUT is used by nand_change_read_column_op() which > is called by nand_flash_detect_ext_param_page(). Before this patch, the > switch to nand_command_lp was already made just before calling that > function so the behaviour doesn't change. > > Signed-off-by: Arnout Vandecappelle (Essensium/Mind) > --- > Note that I don't have access to a small-page device, so only tested on > large-page devices. Also only tested on i.MX6Q (gpmi-nand). > > I only verified the lack of change in behaviour during nand_detect by > reading the code, so it's possible that I missed something. Testing on > various devices (ONFI, JEDEC, non-ONFI/JEDEC) is needed to be really > sure that nothing breaks. > > Note that this patch can be removed from the series without affecting > the rest. Hm, I don't want to risk any regression, so I'm gonna pass on this patch, especially since we're trying to get rid of ->cmdfunc() in favor or ->exec_op(). The same goes for patch 9, sorry. Regards, Boris > --- > drivers/mtd/nand/raw/nand_base.c | 236 ++++++++++++--------------------------- > 1 file changed, 70 insertions(+), 166 deletions(-) > > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c > index bcc0344b1f27..320efbe41bd6 100644 > --- a/drivers/mtd/nand/raw/nand_base.c > +++ b/drivers/mtd/nand/raw/nand_base.c > @@ -747,121 +747,6 @@ int nand_soft_waitrdy(struct nand_chip *chip, unsigned long timeout_ms) > }; > EXPORT_SYMBOL_GPL(nand_soft_waitrdy); > > -/** > - * nand_command - [DEFAULT] 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. This function is used for small page devices > - * (512 Bytes per page). > - */ > -static void 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; > - > - /* Write out the command to the device */ > - if (command == NAND_CMD_SEQIN) { > - 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; > - } > - 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; > - } > - 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 and sequential > - * in needs 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_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 */ > - nand_wait_status_ready(mtd, 250); > - return; > - > - /* This applies to read commands */ > - 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 we should not wait for the > - * device to be ready. > - */ > - if (column == -1 && page_addr == -1) > - return; > - > - default: > - /* > - * 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 void nand_ccs_delay(struct nand_chip *chip) > { > /* > @@ -882,26 +767,48 @@ static void nand_ccs_delay(struct nand_chip *chip) > } > > /** > - * nand_command_lp - [DEFAULT] Send command to NAND large page device > + * nand_command - [DEFAULT] 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. This is the version for the new large page > - * devices. We 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. > + * Send command to NAND device. > */ > -static void nand_command_lp(struct mtd_info *mtd, unsigned int command, > +static void 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; > > - /* Emulate NAND_CMD_READOOB */ > - if (command == NAND_CMD_READOOB) { > - column += mtd->writesize; > - command = NAND_CMD_READ0; > + 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 */ > @@ -920,7 +827,7 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command, > ctrl &= ~NAND_CTRL_CHANGE; > > /* Only output a single addr cycle for 8bits opcodes. */ > - if (!nand_opcode_8bits(command)) > + if (is_lp && !nand_opcode_8bits(command)) > chip->cmd_ctrl(mtd, column >> 8, ctrl); > } > if (page_addr != -1) { > @@ -939,7 +846,6 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command, > switch (command) { > > case NAND_CMD_NONE: > - case NAND_CMD_CACHEDPROG: > case NAND_CMD_PAGEPROG: > case NAND_CMD_ERASE1: > case NAND_CMD_ERASE2: > @@ -949,9 +855,17 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command, > case NAND_CMD_SET_FEATURES: > return; > > + case NAND_CMD_CACHEDPROG: > + if (is_lp) > + return; > + break; > + > case NAND_CMD_RNDIN: > - nand_ccs_delay(chip); > - return; > + if (is_lp) { > + nand_ccs_delay(chip); > + return; > + } > + break; > > case NAND_CMD_RESET: > if (chip->dev_ready) > @@ -966,40 +880,44 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command, > return; > > case NAND_CMD_RNDOUT: > - /* 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); > - > - nand_ccs_delay(chip); > - return; > + 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); > + > + nand_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 READSTART should not be > - * issued. > + * 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; > > - 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); > - > - /* This applies to read commands */ > - default: > - /* > - * 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; > + 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; > } > > /* > @@ -5180,16 +5098,6 @@ static int nand_flash_detect_onfi(struct nand_chip *chip) > chip->ecc_step_ds = 512; > } else if (chip->parameters.onfi.version >= 21 && > (le16_to_cpu(p->features) & ONFI_FEATURE_EXT_PARAM_PAGE)) { > - > - /* > - * The nand_flash_detect_ext_param_page() uses the > - * Change Read Column command which maybe not supported > - * by the chip->cmdfunc. So try to update the chip->cmdfunc > - * now. We do not replace user supplied command function. > - */ > - if (mtd->writesize > 512 && chip->cmdfunc == nand_command) > - chip->cmdfunc = nand_command_lp; > - > /* The Extended Parameter Page is supported since ONFI 2.1. */ > if (nand_flash_detect_ext_param_page(chip, p)) > pr_warn("Failed to detect ONFI extended param page\n"); > @@ -5686,10 +5594,6 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type) > chip->badblockbits = 8; > chip->erase = single_erase; > > - /* Do not replace user supplied command function! */ > - if (mtd->writesize > 512 && chip->cmdfunc == nand_command) > - chip->cmdfunc = nand_command_lp; > - > pr_info("device found, Manufacturer ID: 0x%02x, Chip ID: 0x%02x\n", > maf_id, dev_id); > pr_info("%s %s\n", nand_manufacturer_name(manufacturer),