Reducing the number of interrupts by page reads, part 1

Reducing the number of interrupts by page reads, part 1

[Sam Lefebvre]

The current implementation for gmpi nand flashes use existing libraries for composing                    
dma command chains in different pieces with mutliple interrupts. This invokes a lot of                   
unnecessary context switches. In order to make more optimal use of dma chaining capabilities
of the gpmi hardware, some rework must be performed to allow gpmi_ecc_read_page() submitting 
a range of dma's to reduce the interrupts from 4 to 1 for every page read.

In the current situation, a page read for large page incurs 4 interrupts:
- DMA for READ0
- DMA for READSTART
- DMA for reading of page data
- BCH
                                                                                                         
This series is the first part which contains some preparation work and primary modifications             
to reduce the number of interrrupts from 4 to 3 for each page read action by removing the
interrupt for READ0.                               

Some changes by Sasha Hauer are also adopted which contain some bugfixes and preliminary
cleanup of several parts in de code.

Sam lefebvre 

----------------------------------------------------------------                                        
Arnout Vandecappelle (Essensium/Mind) (8):                                                              
      mtd: rawnand: make nand_command() and nand_command_lp() more similar                              
      mtd: rawnand: factor nand_command_lp() into nand_command()                                        
      mtd: rawnand: gpmi: instantiate cmdfunc                                                           
      mtd: rawnand: gpmi: gpmi_ccs_delay() is not needed                                                
      mtd: rawnand: gpmi: explicit delays are not needed                                                
      mtd: rawnand: gpmi: no explicit wait is needed after sending a command                            
      mtd: rawnand: gpmi: cmd_ctrl is no longer needed                                                  
      mtd: rawnand: gpmi: inline gpmi_cmd_ctrl()                                                        
                                                                                                        
Sam Lefebvre (Essensium/Mind) (4):                                                                     
      mtd: rawnand: gpmi: return generated errors in gpmi_ecc_read_oob()                                
      mtd: rawnand: gpmi: set aggregate ready/busy signalling                                           
      mtd: rawnand: gpmi: gpmi_nand_command(): use separate sgl for the two commands                    
      mtd: rawnand: gpmi: issue two commands in a single DMA chain                                      
                                                                                                        
Sascha Hauer (6):                                                                                       
      mtd: nand: gpmi: drop dma_ops_type                                                                
      mtd: nand: gpmi: pass buffer and len around                                                       
      mtd: nand: gpmi: put only once used functions inline                                              
      mtd: nand: gpmi: remove direct_dma_map_ok from driver data struct                                 
      mtd: nand: gpmi: return valid value from bch_set_geometry()                                       
      mtd: nand: gpmi: remove unnecessary variables                                                     
                                                                                                        
 drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c  |  71 ++++++------                                          
 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 330 ++++++++++++++++++++++++++++--------------------------
 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h |  34 ++----                                                
 drivers/mtd/nand/raw/gpmi-nand/gpmi-regs.h |   1 +                                                     
 drivers/mtd/nand/raw/nand_base.c           | 259 +++++++++++++-----------------------------            
 5 files changed, 304 insertions(+), 391 deletions(-)                                                   

[PATCH 01/18] mtd: nand: gpmi: drop dma_ops_type

[Sam Lefebvre]

From: Sascha Hauer <s.hauer@pengutronix.de>

The GPMI nand driver puts dma_ops_type in its private data struct. Based
on the ops type the DMA callback handler unmaps previously mapped
buffers. Instead of unmapping the buffers in the DMA callback handler,
do this in the caller directly which waits for the DMA transfer to
finish. This makes the whole dma_ops_type mechanism unnecessary.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
v2: also unmap the cmd_sgl, don't unmap in gpmi_read_page() (it's done
    in the bch interrupt handler). Sascha, in the future better test
    with CONFIG_DMA_API_DEBUG :-)
---
 drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c  | 36 +++++++++++++++++-------------
 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 31 ++-----------------------
 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h | 11 ---------
 3 files changed, 23 insertions(+), 55 deletions(-)

diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
index e94556705dc7..d479358758a0 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
@@ -544,19 +544,13 @@ int gpmi_is_ready(struct gpmi_nand_data *this, unsigned chip)
 	return reg & mask;
 }
 
-static inline void set_dma_type(struct gpmi_nand_data *this,
-					enum dma_ops_type type)
-{
-	this->last_dma_type = this->dma_type;
-	this->dma_type = type;
-}
-
 int gpmi_send_command(struct gpmi_nand_data *this)
 {
 	struct dma_chan *channel = get_dma_chan(this);
 	struct dma_async_tx_descriptor *desc;
 	struct scatterlist *sgl;
 	int chip = this->current_chip;
+	int ret;
 	u32 pio[3];
 
 	/* [1] send out the PIO words */
@@ -586,8 +580,11 @@ int gpmi_send_command(struct gpmi_nand_data *this)
 		return -EINVAL;
 
 	/* [3] submit the DMA */
-	set_dma_type(this, DMA_FOR_COMMAND);
-	return start_dma_without_bch_irq(this, desc);
+	ret = start_dma_without_bch_irq(this, desc);
+
+	dma_unmap_sg(this->dev, sgl, 1, DMA_TO_DEVICE);
+
+	return ret;
 }
 
 int gpmi_send_data(struct gpmi_nand_data *this)
@@ -595,6 +592,7 @@ int gpmi_send_data(struct gpmi_nand_data *this)
 	struct dma_async_tx_descriptor *desc;
 	struct dma_chan *channel = get_dma_chan(this);
 	int chip = this->current_chip;
+	int ret;
 	uint32_t command_mode;
 	uint32_t address;
 	u32 pio[2];
@@ -624,8 +622,11 @@ int gpmi_send_data(struct gpmi_nand_data *this)
 		return -EINVAL;
 
 	/* [3] submit the DMA */
-	set_dma_type(this, DMA_FOR_WRITE_DATA);
-	return start_dma_without_bch_irq(this, desc);
+	ret = start_dma_without_bch_irq(this, desc);
+
+	dma_unmap_sg(this->dev, &this->data_sgl, 1, DMA_TO_DEVICE);
+
+	return ret;
 }
 
 int gpmi_read_data(struct gpmi_nand_data *this)
@@ -633,6 +634,7 @@ int gpmi_read_data(struct gpmi_nand_data *this)
 	struct dma_async_tx_descriptor *desc;
 	struct dma_chan *channel = get_dma_chan(this);
 	int chip = this->current_chip;
+	int ret;
 	u32 pio[2];
 
 	/* [1] : send PIO */
@@ -658,8 +660,14 @@ int gpmi_read_data(struct gpmi_nand_data *this)
 		return -EINVAL;
 
 	/* [3] : submit the DMA */
-	set_dma_type(this, DMA_FOR_READ_DATA);
-	return start_dma_without_bch_irq(this, desc);
+
+	ret = start_dma_without_bch_irq(this, desc);
+
+	dma_unmap_sg(this->dev, &this->data_sgl, 1, DMA_FROM_DEVICE);
+	if (this->direct_dma_map_ok == false)
+		memcpy(this->upper_buf, this->data_buffer_dma, this->upper_len);
+
+	return ret;
 }
 
 int gpmi_send_page(struct gpmi_nand_data *this,
@@ -703,7 +711,6 @@ int gpmi_send_page(struct gpmi_nand_data *this,
 	if (!desc)
 		return -EINVAL;
 
-	set_dma_type(this, DMA_FOR_WRITE_ECC_PAGE);
 	return start_dma_with_bch_irq(this, desc);
 }
 
@@ -785,7 +792,6 @@ int gpmi_read_page(struct gpmi_nand_data *this,
 		return -EINVAL;
 
 	/* [4] submit the DMA */
-	set_dma_type(this, DMA_FOR_READ_ECC_PAGE);
 	return start_dma_with_bch_irq(this, desc);
 }
 
diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
index c2597c8107a0..864fda1901a6 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
@@ -472,31 +472,6 @@ static void dma_irq_callback(void *param)
 	struct gpmi_nand_data *this = param;
 	struct completion *dma_c = &this->dma_done;
 
-	switch (this->dma_type) {
-	case DMA_FOR_COMMAND:
-		dma_unmap_sg(this->dev, &this->cmd_sgl, 1, DMA_TO_DEVICE);
-		break;
-
-	case DMA_FOR_READ_DATA:
-		dma_unmap_sg(this->dev, &this->data_sgl, 1, DMA_FROM_DEVICE);
-		if (this->direct_dma_map_ok == false)
-			memcpy(this->upper_buf, this->data_buffer_dma,
-				this->upper_len);
-		break;
-
-	case DMA_FOR_WRITE_DATA:
-		dma_unmap_sg(this->dev, &this->data_sgl, 1, DMA_TO_DEVICE);
-		break;
-
-	case DMA_FOR_READ_ECC_PAGE:
-	case DMA_FOR_WRITE_ECC_PAGE:
-		/* We have to wait the BCH interrupt to finish. */
-		break;
-
-	default:
-		dev_err(this->dev, "in wrong DMA operation.\n");
-	}
-
 	complete(dma_c);
 }
 
@@ -516,8 +491,7 @@ int start_dma_without_bch_irq(struct gpmi_nand_data *this,
 	/* Wait for the interrupt from the DMA block. */
 	timeout = wait_for_completion_timeout(dma_c, msecs_to_jiffies(1000));
 	if (!timeout) {
-		dev_err(this->dev, "DMA timeout, last DMA :%d\n",
-			this->last_dma_type);
+		dev_err(this->dev, "DMA timeout, last DMA\n");
 		gpmi_dump_info(this);
 		return -ETIMEDOUT;
 	}
@@ -546,8 +520,7 @@ int start_dma_with_bch_irq(struct gpmi_nand_data *this,
 	/* Wait for the interrupt from the BCH block. */
 	timeout = wait_for_completion_timeout(bch_c, msecs_to_jiffies(1000));
 	if (!timeout) {
-		dev_err(this->dev, "BCH timeout, last DMA :%d\n",
-			this->last_dma_type);
+		dev_err(this->dev, "BCH timeout\n");
 		gpmi_dump_info(this);
 		return -ETIMEDOUT;
 	}
diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h
index 62fde59b995f..2397010a8963 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h
@@ -77,15 +77,6 @@ struct boot_rom_geometry {
 	unsigned int  search_area_stride_exponent;
 };
 
-/* DMA operations types */
-enum dma_ops_type {
-	DMA_FOR_COMMAND = 1,
-	DMA_FOR_READ_DATA,
-	DMA_FOR_WRITE_DATA,
-	DMA_FOR_READ_ECC_PAGE,
-	DMA_FOR_WRITE_ECC_PAGE
-};
-
 enum gpmi_type {
 	IS_MX23,
 	IS_MX28,
@@ -178,8 +169,6 @@ struct gpmi_nand_data {
 	/* DMA channels */
 #define DMA_CHANS		8
 	struct dma_chan		*dma_chans[DMA_CHANS];
-	enum dma_ops_type	last_dma_type;
-	enum dma_ops_type	dma_type;
 	struct completion	dma_done;
 
 	/* private */
-- 
2.14.1

[PATCH 02/18] mtd: nand: gpmi: pass buffer and len around

[Sam Lefebvre]

From: Sascha Hauer <s.hauer@pengutronix.de>

Instead of putting the buffer and len passed in from the mtd core
into the private data struct, just pass it around in the GPMI
drivers functions. This makes the lifetime of the variables more
clear and the code easier to follow.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
v2: rebased
---
 drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c  | 14 +++++++-------
 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 20 ++++++++------------
 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h | 11 ++++-------
 3 files changed, 19 insertions(+), 26 deletions(-)

diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
index d479358758a0..447961b798b4 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
@@ -587,7 +587,7 @@ int gpmi_send_command(struct gpmi_nand_data *this)
 	return ret;
 }
 
-int gpmi_send_data(struct gpmi_nand_data *this)
+int gpmi_send_data(struct gpmi_nand_data *this, const void *buf, int len)
 {
 	struct dma_async_tx_descriptor *desc;
 	struct dma_chan *channel = get_dma_chan(this);
@@ -606,7 +606,7 @@ int gpmi_send_data(struct gpmi_nand_data *this)
 		| BF_GPMI_CTRL0_CS(chip, this)
 		| BF_GPMI_CTRL0_LOCK_CS(LOCK_CS_ENABLE, this)
 		| BF_GPMI_CTRL0_ADDRESS(address)
-		| BF_GPMI_CTRL0_XFER_COUNT(this->upper_len);
+		| BF_GPMI_CTRL0_XFER_COUNT(len);
 	pio[1] = 0;
 	desc = dmaengine_prep_slave_sg(channel, (struct scatterlist *)pio,
 					ARRAY_SIZE(pio), DMA_TRANS_NONE, 0);
@@ -614,7 +614,7 @@ int gpmi_send_data(struct gpmi_nand_data *this)
 		return -EINVAL;
 
 	/* [2] send DMA request */
-	prepare_data_dma(this, DMA_TO_DEVICE);
+	prepare_data_dma(this, buf, len, DMA_TO_DEVICE);
 	desc = dmaengine_prep_slave_sg(channel, &this->data_sgl,
 					1, DMA_MEM_TO_DEV,
 					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
@@ -629,7 +629,7 @@ int gpmi_send_data(struct gpmi_nand_data *this)
 	return ret;
 }
 
-int gpmi_read_data(struct gpmi_nand_data *this)
+int gpmi_read_data(struct gpmi_nand_data *this, void *buf, int len)
 {
 	struct dma_async_tx_descriptor *desc;
 	struct dma_chan *channel = get_dma_chan(this);
@@ -643,7 +643,7 @@ int gpmi_read_data(struct gpmi_nand_data *this)
 		| BF_GPMI_CTRL0_CS(chip, this)
 		| BF_GPMI_CTRL0_LOCK_CS(LOCK_CS_ENABLE, this)
 		| BF_GPMI_CTRL0_ADDRESS(BV_GPMI_CTRL0_ADDRESS__NAND_DATA)
-		| BF_GPMI_CTRL0_XFER_COUNT(this->upper_len);
+		| BF_GPMI_CTRL0_XFER_COUNT(len);
 	pio[1] = 0;
 	desc = dmaengine_prep_slave_sg(channel,
 					(struct scatterlist *)pio,
@@ -652,7 +652,7 @@ int gpmi_read_data(struct gpmi_nand_data *this)
 		return -EINVAL;
 
 	/* [2] : send DMA request */
-	prepare_data_dma(this, DMA_FROM_DEVICE);
+	prepare_data_dma(this, buf, len, DMA_FROM_DEVICE);
 	desc = dmaengine_prep_slave_sg(channel, &this->data_sgl,
 					1, DMA_DEV_TO_MEM,
 					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
@@ -665,7 +665,7 @@ int gpmi_read_data(struct gpmi_nand_data *this)
 
 	dma_unmap_sg(this->dev, &this->data_sgl, 1, DMA_FROM_DEVICE);
 	if (this->direct_dma_map_ok == false)
-		memcpy(this->upper_buf, this->data_buffer_dma, this->upper_len);
+		memcpy(buf, this->data_buffer_dma, len);
 
 	return ret;
 }
diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
index 864fda1901a6..5fa9cd4fac4a 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
@@ -437,15 +437,15 @@ struct dma_chan *get_dma_chan(struct gpmi_nand_data *this)
 }
 
 /* Can we use the upper's buffer directly for DMA? */
-void prepare_data_dma(struct gpmi_nand_data *this, enum dma_data_direction dr)
+void prepare_data_dma(struct gpmi_nand_data *this, const void *buf, int len,
+		      enum dma_data_direction dr)
 {
 	struct scatterlist *sgl = &this->data_sgl;
 	int ret;
 
 	/* first try to map the upper buffer directly */
-	if (virt_addr_valid(this->upper_buf) &&
-		!object_is_on_stack(this->upper_buf)) {
-		sg_init_one(sgl, this->upper_buf, this->upper_len);
+	if (virt_addr_valid(buf) && !object_is_on_stack(buf)) {
+		sg_init_one(sgl, buf, len);
 		ret = dma_map_sg(this->dev, sgl, 1, dr);
 		if (ret == 0)
 			goto map_fail;
@@ -456,10 +456,10 @@ void prepare_data_dma(struct gpmi_nand_data *this, enum dma_data_direction dr)
 
 map_fail:
 	/* We have to use our own DMA buffer. */
-	sg_init_one(sgl, this->data_buffer_dma, this->upper_len);
+	sg_init_one(sgl, this->data_buffer_dma, len);
 
 	if (dr == DMA_TO_DEVICE)
-		memcpy(this->data_buffer_dma, this->upper_buf, this->upper_len);
+		memcpy(this->data_buffer_dma, buf, len);
 
 	dma_map_sg(this->dev, sgl, 1, dr);
 
@@ -919,10 +919,8 @@ static void gpmi_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
 	struct gpmi_nand_data *this = nand_get_controller_data(chip);
 
 	dev_dbg(this->dev, "len is %d\n", len);
-	this->upper_buf	= buf;
-	this->upper_len	= len;
 
-	gpmi_read_data(this);
+	gpmi_read_data(this, buf, len);
 }
 
 static void gpmi_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
@@ -931,10 +929,8 @@ static void gpmi_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
 	struct gpmi_nand_data *this = nand_get_controller_data(chip);
 
 	dev_dbg(this->dev, "len is %d\n", len);
-	this->upper_buf	= (uint8_t *)buf;
-	this->upper_len	= len;
 
-	gpmi_send_data(this);
+	gpmi_send_data(this, buf, len);
 }
 
 static uint8_t gpmi_read_byte(struct mtd_info *mtd)
diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h
index 2397010a8963..fba72ad28263 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h
@@ -141,10 +141,6 @@ struct gpmi_nand_data {
 	int			current_chip;
 	unsigned int		command_length;
 
-	/* passed from upper layer */
-	uint8_t			*upper_buf;
-	int			upper_len;
-
 	/* for DMA operations */
 	bool			direct_dma_map_ok;
 
@@ -178,7 +174,7 @@ struct gpmi_nand_data {
 /* Common Services */
 int common_nfc_set_geometry(struct gpmi_nand_data *);
 struct dma_chan *get_dma_chan(struct gpmi_nand_data *);
-void prepare_data_dma(struct gpmi_nand_data *,
+void prepare_data_dma(struct gpmi_nand_data *, const void *buf, int len,
 		      enum dma_data_direction dr);
 int start_dma_without_bch_irq(struct gpmi_nand_data *,
 			      struct dma_async_tx_descriptor *);
@@ -197,8 +193,9 @@ int gpmi_disable_clk(struct gpmi_nand_data *this);
 int gpmi_setup_data_interface(struct mtd_info *mtd, int chipnr,
 			      const struct nand_data_interface *conf);
 void gpmi_nfc_apply_timings(struct gpmi_nand_data *this);
-int gpmi_read_data(struct gpmi_nand_data *);
-int gpmi_send_data(struct gpmi_nand_data *);
+int gpmi_read_data(struct gpmi_nand_data *, void *buf, int len);
+int gpmi_send_data(struct gpmi_nand_data *, const void *buf, int len);
+
 int gpmi_send_page(struct gpmi_nand_data *,
 		   dma_addr_t payload, dma_addr_t auxiliary);
 int gpmi_read_page(struct gpmi_nand_data *,
-- 
2.14.1

[PATCH 03/18] mtd: nand: gpmi: put only once used functions inline

[Sam Lefebvre]

From: Sascha Hauer <s.hauer@pengutronix.de>

read_page_prepare(), read_page_end() and read_page_swap_end() are
trivial functions that are used only once and take 8 arguments each.
De-obfuscate the code by open coding these functions in
gpmi_ecc_read_page()

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
v2: rebased
---
 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 89 ++++++++----------------------
 1 file changed, 23 insertions(+), 66 deletions(-)

diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
index 5fa9cd4fac4a..774ac98fbdb2 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
@@ -668,56 +668,6 @@ static void release_resources(struct gpmi_nand_data *this)
 	release_dma_channels(this);
 }
 
-static int read_page_prepare(struct gpmi_nand_data *this,
-			void *destination, unsigned length,
-			void *alt_virt, dma_addr_t alt_phys, unsigned alt_size,
-			void **use_virt, dma_addr_t *use_phys)
-{
-	struct device *dev = this->dev;
-
-	if (virt_addr_valid(destination)) {
-		dma_addr_t dest_phys;
-
-		dest_phys = dma_map_single(dev, destination,
-						length, DMA_FROM_DEVICE);
-		if (dma_mapping_error(dev, dest_phys)) {
-			if (alt_size < length) {
-				dev_err(dev, "Alternate buffer is too small\n");
-				return -ENOMEM;
-			}
-			goto map_failed;
-		}
-		*use_virt = destination;
-		*use_phys = dest_phys;
-		this->direct_dma_map_ok = true;
-		return 0;
-	}
-
-map_failed:
-	*use_virt = alt_virt;
-	*use_phys = alt_phys;
-	this->direct_dma_map_ok = false;
-	return 0;
-}
-
-static inline void read_page_end(struct gpmi_nand_data *this,
-			void *destination, unsigned length,
-			void *alt_virt, dma_addr_t alt_phys, unsigned alt_size,
-			void *used_virt, dma_addr_t used_phys)
-{
-	if (this->direct_dma_map_ok)
-		dma_unmap_single(this->dev, used_phys, length, DMA_FROM_DEVICE);
-}
-
-static inline void read_page_swap_end(struct gpmi_nand_data *this,
-			void *destination, unsigned length,
-			void *alt_virt, dma_addr_t alt_phys, unsigned alt_size,
-			void *used_virt, dma_addr_t used_phys)
-{
-	if (!this->direct_dma_map_ok)
-		memcpy(destination, alt_virt, length);
-}
-
 static int send_page_prepare(struct gpmi_nand_data *this,
 			const void *source, unsigned length,
 			void *alt_virt, dma_addr_t alt_phys, unsigned alt_size,
@@ -1008,24 +958,33 @@ static int gpmi_ecc_read_page_data(struct nand_chip *chip,
 	int           ret;
 
 	dev_dbg(this->dev, "page number is : %d\n", page);
-	ret = read_page_prepare(this, buf, nfc_geo->payload_size,
-					this->payload_virt, this->payload_phys,
-					nfc_geo->payload_size,
-					&payload_virt, &payload_phys);
-	if (ret) {
-		dev_err(this->dev, "Inadequate DMA buffer\n");
-		ret = -ENOMEM;
-		return ret;
+
+	payload_virt = this->payload_virt;
+	payload_phys = this->payload_phys;
+	this->direct_dma_map_ok = false;
+
+	if (virt_addr_valid(buf)) {
+		dma_addr_t dest_phys;
+
+		dest_phys = dma_map_single(this->dev, buf, nfc_geo->payload_size,
+					   DMA_FROM_DEVICE);
+		if (!dma_mapping_error(this->dev, dest_phys)) {
+			payload_virt = buf;
+			payload_phys = dest_phys;
+			this->direct_dma_map_ok = true;
+		}
 	}
+
 	auxiliary_virt = this->auxiliary_virt;
 	auxiliary_phys = this->auxiliary_phys;
 
 	/* go! */
 	ret = gpmi_read_page(this, payload_phys, auxiliary_phys);
-	read_page_end(this, buf, nfc_geo->payload_size,
-			this->payload_virt, this->payload_phys,
-			nfc_geo->payload_size,
-			payload_virt, payload_phys);
+
+	if (this->direct_dma_map_ok)
+		dma_unmap_single(this->dev, payload_phys, nfc_geo->payload_size,
+				 DMA_FROM_DEVICE);
+
 	if (ret) {
 		dev_err(this->dev, "Error in ECC-based read: %d\n", ret);
 		return ret;
@@ -1034,10 +993,8 @@ static int gpmi_ecc_read_page_data(struct nand_chip *chip,
 	/* Loop over status bytes, accumulating ECC status. */
 	status = auxiliary_virt + nfc_geo->auxiliary_status_offset;
 
-	read_page_swap_end(this, buf, nfc_geo->payload_size,
-			   this->payload_virt, this->payload_phys,
-			   nfc_geo->payload_size,
-			   payload_virt, payload_phys);
+	if (!this->direct_dma_map_ok)
+		memcpy(buf, this->payload_virt, nfc_geo->payload_size);
 
 	for (i = 0; i < nfc_geo->ecc_chunk_count; i++, status++) {
 		if ((*status == STATUS_GOOD) || (*status == STATUS_ERASED))
-- 
2.14.1

[PATCH 04/18] mtd: nand: gpmi: remove direct_dma_map_ok from driver data struct

[Sam Lefebvre]

From: Sascha Hauer <s.hauer@pengutronix.de>

Instead of putting direct_dma_map_ok into driver struct pass it around
between functions to make the code more readable.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
v2: rebased
---
 drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c  |  5 +++--
 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 15 +++++++--------
 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h |  5 +----
 3 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
index 447961b798b4..39834bedf460 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
@@ -636,6 +636,7 @@ int gpmi_read_data(struct gpmi_nand_data *this, void *buf, int len)
 	int chip = this->current_chip;
 	int ret;
 	u32 pio[2];
+	bool direct;
 
 	/* [1] : send PIO */
 	pio[0] = BF_GPMI_CTRL0_COMMAND_MODE(BV_GPMI_CTRL0_COMMAND_MODE__READ)
@@ -652,7 +653,7 @@ int gpmi_read_data(struct gpmi_nand_data *this, void *buf, int len)
 		return -EINVAL;
 
 	/* [2] : send DMA request */
-	prepare_data_dma(this, buf, len, DMA_FROM_DEVICE);
+	direct = prepare_data_dma(this, buf, len, DMA_FROM_DEVICE);
 	desc = dmaengine_prep_slave_sg(channel, &this->data_sgl,
 					1, DMA_DEV_TO_MEM,
 					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
@@ -664,7 +665,7 @@ int gpmi_read_data(struct gpmi_nand_data *this, void *buf, int len)
 	ret = start_dma_without_bch_irq(this, desc);
 
 	dma_unmap_sg(this->dev, &this->data_sgl, 1, DMA_FROM_DEVICE);
-	if (this->direct_dma_map_ok == false)
+	if (!direct)
 		memcpy(buf, this->data_buffer_dma, len);
 
 	return ret;
diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
index 774ac98fbdb2..9ecba7ea0cd5 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
@@ -437,7 +437,7 @@ struct dma_chan *get_dma_chan(struct gpmi_nand_data *this)
 }
 
 /* Can we use the upper's buffer directly for DMA? */
-void prepare_data_dma(struct gpmi_nand_data *this, const void *buf, int len,
+bool prepare_data_dma(struct gpmi_nand_data *this, const void *buf, int len,
 		      enum dma_data_direction dr)
 {
 	struct scatterlist *sgl = &this->data_sgl;
@@ -450,8 +450,7 @@ void prepare_data_dma(struct gpmi_nand_data *this, const void *buf, int len,
 		if (ret == 0)
 			goto map_fail;
 
-		this->direct_dma_map_ok = true;
-		return;
+		return true;
 	}
 
 map_fail:
@@ -463,7 +462,7 @@ void prepare_data_dma(struct gpmi_nand_data *this, const void *buf, int len,
 
 	dma_map_sg(this->dev, sgl, 1, dr);
 
-	this->direct_dma_map_ok = false;
+	return false;
 }
 
 /* This will be called after the DMA operation is finished. */
@@ -956,12 +955,12 @@ static int gpmi_ecc_read_page_data(struct nand_chip *chip,
 	unsigned char *status;
 	unsigned int  max_bitflips = 0;
 	int           ret;
+	bool          direct = false;
 
 	dev_dbg(this->dev, "page number is : %d\n", page);
 
 	payload_virt = this->payload_virt;
 	payload_phys = this->payload_phys;
-	this->direct_dma_map_ok = false;
 
 	if (virt_addr_valid(buf)) {
 		dma_addr_t dest_phys;
@@ -971,7 +970,7 @@ static int gpmi_ecc_read_page_data(struct nand_chip *chip,
 		if (!dma_mapping_error(this->dev, dest_phys)) {
 			payload_virt = buf;
 			payload_phys = dest_phys;
-			this->direct_dma_map_ok = true;
+			direct = true;
 		}
 	}
 
@@ -981,7 +980,7 @@ static int gpmi_ecc_read_page_data(struct nand_chip *chip,
 	/* go! */
 	ret = gpmi_read_page(this, payload_phys, auxiliary_phys);
 
-	if (this->direct_dma_map_ok)
+	if (direct)
 		dma_unmap_single(this->dev, payload_phys, nfc_geo->payload_size,
 				 DMA_FROM_DEVICE);
 
@@ -993,7 +992,7 @@ static int gpmi_ecc_read_page_data(struct nand_chip *chip,
 	/* Loop over status bytes, accumulating ECC status. */
 	status = auxiliary_virt + nfc_geo->auxiliary_status_offset;
 
-	if (!this->direct_dma_map_ok)
+	if (!direct)
 		memcpy(buf, this->payload_virt, nfc_geo->payload_size);
 
 	for (i = 0; i < nfc_geo->ecc_chunk_count; i++, status++) {
diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h
index fba72ad28263..6aa10d6962d6 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h
@@ -141,9 +141,6 @@ struct gpmi_nand_data {
 	int			current_chip;
 	unsigned int		command_length;
 
-	/* for DMA operations */
-	bool			direct_dma_map_ok;
-
 	struct scatterlist	cmd_sgl;
 	char			*cmd_buffer;
 
@@ -174,7 +171,7 @@ struct gpmi_nand_data {
 /* Common Services */
 int common_nfc_set_geometry(struct gpmi_nand_data *);
 struct dma_chan *get_dma_chan(struct gpmi_nand_data *);
-void prepare_data_dma(struct gpmi_nand_data *, const void *buf, int len,
+bool prepare_data_dma(struct gpmi_nand_data *, const void *buf, int len,
 		      enum dma_data_direction dr);
 int start_dma_without_bch_irq(struct gpmi_nand_data *,
 			      struct dma_async_tx_descriptor *);
-- 
2.14.1

[PATCH 05/18] mtd: nand: gpmi: return valid value from bch_set_geometry()

[Sam Lefebvre]

From: Sascha Hauer <s.hauer@pengutronix.de>

The caller of bch_set_geometry() expects the return value to
be an error code, so !0 is not valid. return the error from the
just called function instead.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
v2: rebased
---
 drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
index 39834bedf460..83697b8df871 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
@@ -258,8 +258,9 @@ int bch_set_geometry(struct gpmi_nand_data *this)
 	unsigned int gf_len;
 	int ret;
 
-	if (common_nfc_set_geometry(this))
-		return !0;
+	ret = common_nfc_set_geometry(this);
+	if (ret)
+		return ret;
 
 	block_count   = bch_geo->ecc_chunk_count - 1;
 	block_size    = bch_geo->ecc_chunk_size;
-- 
2.14.1

[PATCH 06/18] mtd: nand: gpmi: remove unnecessary variables

[Sam Lefebvre]

From: Sascha Hauer <s.hauer@pengutronix.de>

Use this->auxiliary_virt and this->auxiliary_phys directly rather
than creating extra local variables for them.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
v2: rebased
---
 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
index 9ecba7ea0cd5..7ba00b8ab288 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
@@ -949,8 +949,6 @@ static int gpmi_ecc_read_page_data(struct nand_chip *chip,
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	void          *payload_virt;
 	dma_addr_t    payload_phys;
-	void          *auxiliary_virt;
-	dma_addr_t    auxiliary_phys;
 	unsigned int  i;
 	unsigned char *status;
 	unsigned int  max_bitflips = 0;
@@ -974,11 +972,8 @@ static int gpmi_ecc_read_page_data(struct nand_chip *chip,
 		}
 	}
 
-	auxiliary_virt = this->auxiliary_virt;
-	auxiliary_phys = this->auxiliary_phys;
-
 	/* go! */
-	ret = gpmi_read_page(this, payload_phys, auxiliary_phys);
+	ret = gpmi_read_page(this, payload_phys, this->auxiliary_phys);
 
 	if (direct)
 		dma_unmap_single(this->dev, payload_phys, nfc_geo->payload_size,
@@ -990,7 +985,7 @@ static int gpmi_ecc_read_page_data(struct nand_chip *chip,
 	}
 
 	/* Loop over status bytes, accumulating ECC status. */
-	status = auxiliary_virt + nfc_geo->auxiliary_status_offset;
+	status = this->auxiliary_virt + nfc_geo->auxiliary_status_offset;
 
 	if (!direct)
 		memcpy(buf, this->payload_virt, nfc_geo->payload_size);
@@ -1048,7 +1043,7 @@ static int gpmi_ecc_read_page_data(struct nand_chip *chip,
 						buf + i * nfc_geo->ecc_chunk_size,
 						nfc_geo->ecc_chunk_size,
 						eccbuf, eccbytes,
-						auxiliary_virt,
+						this->auxiliary_virt,
 						nfc_geo->metadata_size,
 						nfc_geo->ecc_strength);
 			} else {
@@ -1076,7 +1071,7 @@ static int gpmi_ecc_read_page_data(struct nand_chip *chip,
 	}
 
 	/* handle the block mark swapping */
-	block_mark_swapping(this, buf, auxiliary_virt);
+	block_mark_swapping(this, buf, this->auxiliary_virt);
 
 	if (oob_required) {
 		/*
@@ -1090,7 +1085,7 @@ static int gpmi_ecc_read_page_data(struct nand_chip *chip,
 		 * the block mark.
 		 */
 		memset(chip->oob_poi, ~0, mtd->oobsize);
-		chip->oob_poi[0] = ((uint8_t *) auxiliary_virt)[0];
+		chip->oob_poi[0] = ((uint8_t *)this->auxiliary_virt)[0];
 	}
 
 	return max_bitflips;
-- 
2.14.1

[PATCH 07/18] mtd: rawnand: gpmi: return generated errors in gpmi_ecc_read_oob()

[Sam Lefebvre]

Errors can be generated by nand_read_page_op(). It's important
to check the error flags and pass them upwards.

Signed-off-by: Sam Lefebvre <sam.lefebvre@essensium.com>
---
 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
index 7ba00b8ab288..3da4c07ce2d9 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
@@ -1334,13 +1334,18 @@ static int gpmi_ecc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
 				int page)
 {
 	struct gpmi_nand_data *this = nand_get_controller_data(chip);
+	int ret;
 
 	dev_dbg(this->dev, "page number is %d\n", page);
 	/* clear the OOB buffer */
 	memset(chip->oob_poi, ~0, mtd->oobsize);
 
 	/* Read out the conventional OOB. */
-	nand_read_page_op(chip, page, mtd->writesize, NULL, 0);
+	ret = nand_read_page_op(chip, page, mtd->writesize, NULL, 0);
+
+	if (ret)
+		return ret;
+
 	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
 
 	/*
@@ -1350,11 +1355,11 @@ static int gpmi_ecc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
 	 */
 	if (GPMI_IS_MX23(this)) {
 		/* Read the block mark into the first byte of the OOB buffer. */
-		nand_read_page_op(chip, page, 0, NULL, 0);
+		ret = nand_read_page_op(chip, page, 0, NULL, 0);
 		chip->oob_poi[0] = chip->read_byte(mtd);
 	}
 
-	return 0;
+	return ret;
 }
 
 static int
-- 
2.14.1

Re: [PATCH 07/18] mtd: rawnand: gpmi: return generated errors in gpmi_ecc_read_oob()

[Boris Brezillon]

On Fri, 20 Apr 2018 10:19:35 +0200
Sam Lefebvre <sam.lefebvre@essensium.com> wrote:

> Errors can be generated by nand_read_page_op(). It's important
> to check the error flags and pass them upwards.
> 
> Signed-off-by: Sam Lefebvre <sam.lefebvre@essensium.com>
> ---
>  drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> index 7ba00b8ab288..3da4c07ce2d9 100644
> --- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> @@ -1334,13 +1334,18 @@ static int gpmi_ecc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
>  				int page)
>  {
>  	struct gpmi_nand_data *this = nand_get_controller_data(chip);
> +	int ret;
>  
>  	dev_dbg(this->dev, "page number is %d\n", page);
>  	/* clear the OOB buffer */
>  	memset(chip->oob_poi, ~0, mtd->oobsize);
>  
>  	/* Read out the conventional OOB. */
> -	nand_read_page_op(chip, page, mtd->writesize, NULL, 0);
> +	ret = nand_read_page_op(chip, page, mtd->writesize, NULL, 0);
> +
> +	if (ret)
> +		return ret;
> +
>  	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
>  
>  	/*
> @@ -1350,11 +1355,11 @@ static int gpmi_ecc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
>  	 */
>  	if (GPMI_IS_MX23(this)) {
>  		/* Read the block mark into the first byte of the OOB buffer. */
> -		nand_read_page_op(chip, page, 0, NULL, 0);
> +		ret = nand_read_page_op(chip, page, 0, NULL, 0);

In case of error, you should bail out before calling ->read_byte().

>  		chip->oob_poi[0] = chip->read_byte(mtd);
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int

[PATCH 08/18] mtd: rawnand: gpmi: set aggregate ready/busy signalling

[Sam Lefebvre]

The GPMI block by default decouples the ready/busy signal (which is
shared between all chip-selects) to separate internal signals per
chip-select. However, since the gpmi-nand driver uses DMA0 for all
chip-selects, DMA transfers will not be able to see the ready/busy
status for any chip-select other than 0.

Currently, this happens to work because nand_command() ends with an
explicit nand_wait_ready() and the driver only sets up a single command
in a DMA chain. nand_wait_ready() polls for the chip-select specific
ready/busy status bit. Future patches, however, will set up a DMA chain
with several commands, so these will have to wait correctly in the DMA
chain itself.

To fix this, set the GANGED_RDYBUSY bit in the control1 register. This
ties all internal ready/busy signals together, so DMA0 will also see
the ready/busy status of chip selects 1-2-3. It's a bit silly that this
isn't implied in hardware by the DECOUPLE_CS bit.

Signed-off-by: Sam Lefebvre <sam.lefebvre@essensium.com>
Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
Cc: Han Xu <han.xu@nxp.com>
---
Tested on an i.MX6Q with two identical chips on CS0 and CS1.
---
 drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c  | 3 +++
 drivers/mtd/nand/raw/gpmi-nand/gpmi-regs.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
index 83697b8df871..1858afdb400d 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
@@ -193,6 +193,9 @@ int gpmi_init(struct gpmi_nand_data *this)
 	 */
 	writel(BM_GPMI_CTRL1_DECOUPLE_CS, r->gpmi_regs + HW_GPMI_CTRL1_SET);
 
+	/* Aggregate ready busy signalling. */
+	writel(BM_GPMI_CTRL1_GANGED_RDYBUSY, r->gpmi_regs + HW_GPMI_CTRL1_SET);
+
 	gpmi_disable_clk(this);
 	return 0;
 err_out:
diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-regs.h b/drivers/mtd/nand/raw/gpmi-nand/gpmi-regs.h
index d92bf32221ca..e341802c90ac 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-regs.h
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-regs.h
@@ -120,6 +120,7 @@
 #define BV_GPMI_CTRL1_WRN_DLY_SEL_7_TO_12NS		0x2
 #define BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY		0x3
 
+#define BM_GPMI_CTRL1_GANGED_RDYBUSY		(1 << 19)
 #define BM_GPMI_CTRL1_BCH_MODE				(1 << 18)
 
 #define BP_GPMI_CTRL1_DLL_ENABLE			17
-- 
2.14.1

[PATCH 09/18] mtd: rawnand: make nand_command() and nand_command_lp() more similar

[Sam Lefebvre]

From: "Arnout Vandecappelle (Essensium/Mind)" <arnout@mind.be>

The nand_command() and nand_command_lp() functions are very similar,
but there are slight typographic differences. As a preparation for
unifying the two later, make them more similar now.

This includes the following changes.

In nand_command():
- use explicit NAND_NCE | NAND_{A,C}LE instead of NAND_CTRL_{A,C}LE
  (the explicit form seems to be used more often tree-wide).
- proper line-wrapping.

In nand_command_lp():
- use the ctrl local variable wherever appropriate.
- remove the additional 'if (column != -1 || page_addr != -1)' around
  the two individual conditions.

Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
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).

Note that this patch can be removed from the series without affecting
the rest.
---
 drivers/mtd/nand/raw/nand_base.c | 59 +++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 31 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 72f3a89da513..bcc0344b1f27 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -761,7 +761,7 @@ 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_CTRL_CLE | NAND_CTRL_CHANGE;
+	int ctrl = NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE;
 
 	/* Write out the command to the device */
 	if (command == NAND_CMD_SEQIN) {
@@ -785,7 +785,7 @@ static void nand_command(struct mtd_info *mtd, unsigned int command,
 		chip->cmd_ctrl(mtd, command, ctrl);
 
 	/* Address cycle, when necessary */
-	ctrl = NAND_CTRL_ALE | NAND_CTRL_CHANGE;
+	ctrl = NAND_NCE | NAND_ALE | NAND_CTRL_CHANGE;
 	/* Serially input address */
 	if (column != -1) {
 		/* Adjust columns for 16 bit buswidth */
@@ -825,9 +825,9 @@ static void nand_command(struct mtd_info *mtd, unsigned int command,
 			break;
 		udelay(chip->chip_delay);
 		chip->cmd_ctrl(mtd, NAND_CMD_STATUS,
-			       NAND_CTRL_CLE | NAND_CTRL_CHANGE);
-		chip->cmd_ctrl(mtd,
-			       NAND_CMD_NONE, NAND_NCE | NAND_CTRL_CHANGE);
+			       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;
@@ -896,6 +896,7 @@ static void nand_command_lp(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;
 
 	/* Emulate NAND_CMD_READOOB */
 	if (command == NAND_CMD_READOOB) {
@@ -905,33 +906,29 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
 
 	/* Command latch cycle */
 	if (command != NAND_CMD_NONE)
-		chip->cmd_ctrl(mtd, command,
-			       NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE);
+		chip->cmd_ctrl(mtd, command, ctrl);
 
-	if (column != -1 || page_addr != -1) {
-		int ctrl = NAND_CTRL_CHANGE | NAND_NCE | NAND_ALE;
-
-		/* 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 (!nand_opcode_8bits(command))
-				chip->cmd_ctrl(mtd, column >> 8, ctrl);
-		}
-		if (page_addr != -1) {
-			chip->cmd_ctrl(mtd, page_addr, ctrl);
-			chip->cmd_ctrl(mtd, page_addr >> 8,
-				       NAND_NCE | NAND_ALE);
-			if (chip->options & NAND_ROW_ADDR_3)
-				chip->cmd_ctrl(mtd, page_addr >> 16,
-					       NAND_NCE | NAND_ALE);
-		}
+	/* 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 (!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);
 
-- 
2.14.1

[PATCH 10/18] mtd: rawnand: factor nand_command_lp() into nand_command()

[Sam Lefebvre]

From: "Arnout Vandecappelle (Essensium/Mind)" <arnout@mind.be>

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) <arnout@mind.be>
---
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.
---
 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),
-- 
2.14.1

Re: [PATCH 10/18] mtd: rawnand: factor nand_command_lp() into nand_command()

[Boris Brezillon]

Hi Sam, Arnout,

On Fri, 20 Apr 2018 10:19:38 +0200
Sam Lefebvre <sam.lefebvre@essensium.com> wrote:

> From: "Arnout Vandecappelle (Essensium/Mind)" <arnout@mind.be>
> 
> 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) <arnout@mind.be>
> ---
> 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),

Re: [PATCH 10/18] mtd: rawnand: factor nand_command_lp() into nand_command()

[Arnout Vandecappelle]

 Hi Boris,

On 20-04-18 22:34, Boris Brezillon wrote:
> Hi Sam, Arnout,
> 
> On Fri, 20 Apr 2018 10:19:38 +0200
> Sam Lefebvre <sam.lefebvre@essensium.com> wrote:
> 
>> From: "Arnout Vandecappelle (Essensium/Mind)" <arnout@mind.be>[snip]
>> 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().

 Absolutely. This patch was included more to show how patch 11 was constructed.
I completely agree that it's too risky.

 Regards,
 Arnout

> 
> The same goes for patch 9, sorry.
> 
> Regards,
> 
> Boris
[snip]
-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[PATCH 11/18] mtd: rawnand: gpmi: instantiate cmdfunc

[Sam Lefebvre]

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()).

The auxiliary functions nand_ccs_delay() and nand_wait_status_ready()
also need to be copied, together with the includes they need.

Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 205 +++++++++++++++++++++++++++++
 1 file changed, 205 insertions(+)

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;
-- 
2.14.1

Re: [PATCH 11/18] mtd: rawnand: gpmi: instantiate cmdfunc

[Boris Brezillon]

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

Re: [PATCH 11/18] mtd: rawnand: gpmi: instantiate cmdfunc

[Arnout Vandecappelle]

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

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

Re: [PATCH 11/18] mtd: rawnand: gpmi: instantiate cmdfunc

[Boris Brezillon]

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/

[PATCH 12/18] mtd: rawnand: gpmi: gpmi_ccs_delay() is not needed

[Sam Lefebvre]

From: "Arnout Vandecappelle (Essensium/Mind)" <arnout@mind.be>

The controller already takes care of waiting for tCCS when the RNDIN
or RNDOUT command is sent. That is, the NAND_WAIT_TCCS option is not
set for gpmi-nand. Therefore, the gpmi_ccs_delay() calls are not
needed.

Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
index 69bdee0ca679..ccde6500f981 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
@@ -1112,25 +1112,6 @@ static void gpmi_wait_status_ready(struct mtd_info *mtd, unsigned long timeo)
 	} 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
@@ -1227,7 +1208,6 @@ static void gpmi_nand_command(struct mtd_info *mtd, unsigned int command,
 
 	case NAND_CMD_RNDIN:
 		if (is_lp) {
-			gpmi_ccs_delay(chip);
 			return;
 		}
 		break;
@@ -1251,8 +1231,6 @@ static void gpmi_nand_command(struct mtd_info *mtd, unsigned int command,
 				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;
-- 
2.14.1

[PATCH 13/18] mtd: rawnand: gpmi: explicit delays are not needed

[Sam Lefebvre]

From: "Arnout Vandecappelle (Essensium/Mind)" <arnout@mind.be>

gpmi-nand implements the dev_ready function, so explicit delays are not
needed for the RESET and READ0 commands. Therefore, the
gpmi_wait_status_ready() function and the includes it brings in are not
needed.

The RESET command just falls through to the nand_wait_ready() call.

Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 40 ------------------------------
 1 file changed, 40 deletions(-)

diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
index ccde6500f981..8531bc8d531d 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
@@ -25,7 +25,6 @@
 #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"
@@ -1093,25 +1092,6 @@ 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));
-};
-
 /**
  * gpmi_nand_command - Send command to NAND device
  * @mtd: MTD device structure
@@ -1212,18 +1192,6 @@ static void gpmi_nand_command(struct mtd_info *mtd, unsigned int command,
 		}
 		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 */
@@ -1254,14 +1222,6 @@ static void gpmi_nand_command(struct mtd_info *mtd, unsigned int command,
 		/* 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
-- 
2.14.1

[PATCH 14/18] mtd: rawnand: gpmi: no explicit wait is needed after sending a command

[Sam Lefebvre]

From: "Arnout Vandecappelle (Essensium/Mind)" <arnout@mind.be>

Commands are send using DMA, and the DMA already waits for the
ready/busy signal from the NAND. So no explicit delay and call to
nand_wait_ready() is needed.

This makes it possible to simplify the switch in gpmi_nand_command() a
lot.

Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 62 +++++++-----------------------
 1 file changed, 13 insertions(+), 49 deletions(-)

diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
index 8531bc8d531d..5ae5713090a4 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
@@ -19,7 +19,6 @@
  * 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>
@@ -1163,44 +1162,20 @@ static void gpmi_nand_command(struct mtd_info *mtd, unsigned int command,
 		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) {
+	/* This starts the DMA for the command and waits for it to finish. */
+	chip->cmd_ctrl(mtd, NAND_CMD_NONE, NAND_NCE | NAND_CTRL_CHANGE);
 
-	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:
+	if (!is_lp)
 		return;
 
-	case NAND_CMD_CACHEDPROG:
-		if (is_lp)
-			return;
-		break;
-
-	case NAND_CMD_RNDIN:
-		if (is_lp) {
-			return;
-		}
-		break;
-
+	switch (command) {
 	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);
-			return;
-		}
+		/* 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);
 		break;
 
 	case NAND_CMD_READ0:
@@ -1213,23 +1188,12 @@ static void gpmi_nand_command(struct mtd_info *mtd, unsigned int command,
 		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 */
+		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);
 		break;
 	}
-
-	/*
-	 * Apply this short delay always to ensure that we do wait tWB in
-	 * any case on any machine.
-	 */
-	ndelay(100);
-
-	nand_wait_ready(mtd);
 }
 
 
-- 
2.14.1

[PATCH 15/18] mtd: rawnand: gpmi: cmd_ctrl is no longer needed

[Sam Lefebvre]

From: "Arnout Vandecappelle (Essensium/Mind)" <arnout@mind.be>

Since gpmi-nand now implements cmdfunc, it is no longer needed to
implement cmd_ctrl. Call gpmi_cmd_ctrl directly.

Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 33 +++++++++++++++---------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
index 5ae5713090a4..5700ca1d2ae6 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
@@ -1132,13 +1132,13 @@ static void gpmi_nand_command(struct mtd_info *mtd, unsigned int command,
 			column -= 256;
 			readcmd = NAND_CMD_READ1;
 		}
-		chip->cmd_ctrl(mtd, readcmd, ctrl);
+		gpmi_cmd_ctrl(mtd, readcmd, ctrl);
 		ctrl &= ~NAND_CTRL_CHANGE;
 	}
 
 	/* Command latch cycle */
 	if (command != NAND_CMD_NONE)
-		chip->cmd_ctrl(mtd, command, ctrl);
+		gpmi_cmd_ctrl(mtd, command, ctrl);
 
 	/* Address cycle, when necessary */
 	ctrl = NAND_NCE | NAND_ALE | NAND_CTRL_CHANGE;
@@ -1148,23 +1148,23 @@ static void gpmi_nand_command(struct mtd_info *mtd, unsigned int command,
 		if (chip->options & NAND_BUSWIDTH_16 &&
 				!nand_opcode_8bits(command))
 			column >>= 1;
-		chip->cmd_ctrl(mtd, column, ctrl);
+		gpmi_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);
+			gpmi_cmd_ctrl(mtd, column >> 8, ctrl);
 	}
 	if (page_addr != -1) {
-		chip->cmd_ctrl(mtd, page_addr, ctrl);
+		gpmi_cmd_ctrl(mtd, page_addr, ctrl);
 		ctrl &= ~NAND_CTRL_CHANGE;
-		chip->cmd_ctrl(mtd, page_addr >> 8, ctrl);
+		gpmi_cmd_ctrl(mtd, page_addr >> 8, ctrl);
 		if (chip->options & NAND_ROW_ADDR_3)
-			chip->cmd_ctrl(mtd, page_addr >> 16, ctrl);
+			gpmi_cmd_ctrl(mtd, page_addr >> 16, ctrl);
 	}
 
 	/* This starts the DMA for the command and waits for it to finish. */
-	chip->cmd_ctrl(mtd, NAND_CMD_NONE, NAND_NCE | NAND_CTRL_CHANGE);
+	gpmi_cmd_ctrl(mtd, NAND_CMD_NONE, NAND_NCE | NAND_CTRL_CHANGE);
 
 	if (!is_lp)
 		return;
@@ -1172,10 +1172,10 @@ static void gpmi_nand_command(struct mtd_info *mtd, unsigned int command,
 	switch (command) {
 	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);
+		gpmi_cmd_ctrl(mtd, NAND_CMD_RNDOUTSTART,
+			      NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE);
+		gpmi_cmd_ctrl(mtd, NAND_CMD_NONE,
+			      NAND_NCE | NAND_CTRL_CHANGE);
 		break;
 
 	case NAND_CMD_READ0:
@@ -1188,10 +1188,10 @@ static void gpmi_nand_command(struct mtd_info *mtd, unsigned int command,
 		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);
+		gpmi_cmd_ctrl(mtd, NAND_CMD_READSTART,
+			      NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE);
+		gpmi_cmd_ctrl(mtd, NAND_CMD_NONE,
+			      NAND_NCE | NAND_CTRL_CHANGE);
 		break;
 	}
 }
@@ -2000,7 +2000,6 @@ static int gpmi_nand_init(struct gpmi_nand_data *this)
 	nand_set_flash_node(chip, this->pdev->dev.of_node);
 	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;
-- 
2.14.1

[PATCH 16/18] mtd: rawnand: gpmi: inline gpmi_cmd_ctrl()

[Sam Lefebvre]

From: "Arnout Vandecappelle (Essensium/Mind)" <arnout@mind.be>

gpmi_cmd_ctrl() has two "states":

* ALE or CLE is set, in this case the command/control data is buffered.
  These calls are replaced with
	this->cmd_buffer[this->command_length++] = data;

* ALE and CLE are not set, in this case the command is sent (DMA is
  started). These calls are replaced with
	ret = gpmi_send_command(this);
	if (ret)
		dev_err(this->dev, "Chip: %u, Error %d\n",
			this->current_chip, ret);
	this->command_length = 0;

The 'ctrl' variable/parameter is not used.

Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 85 ++++++++++--------------------
 1 file changed, 29 insertions(+), 56 deletions(-)

diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
index 5700ca1d2ae6..97d44fe212c9 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
@@ -786,40 +786,6 @@ static int gpmi_alloc_dma_buffer(struct gpmi_nand_data *this)
 	return -ENOMEM;
 }
 
-static void gpmi_cmd_ctrl(struct mtd_info *mtd, int data, unsigned int ctrl)
-{
-	struct nand_chip *chip = mtd_to_nand(mtd);
-	struct gpmi_nand_data *this = nand_get_controller_data(chip);
-	int ret;
-
-	/*
-	 * Every operation begins with a command byte and a series of zero or
-	 * more address bytes. These are distinguished by either the Address
-	 * Latch Enable (ALE) or Command Latch Enable (CLE) signals being
-	 * asserted. When MTD is ready to execute the command, it will deassert
-	 * both latch enables.
-	 *
-	 * Rather than run a separate DMA operation for every single byte, we
-	 * queue them up and run a single DMA operation for the entire series
-	 * of command and data bytes. NAND_CMD_NONE means the END of the queue.
-	 */
-	if ((ctrl & (NAND_ALE | NAND_CLE))) {
-		if (data != NAND_CMD_NONE)
-			this->cmd_buffer[this->command_length++] = data;
-		return;
-	}
-
-	if (!this->command_length)
-		return;
-
-	ret = gpmi_send_command(this);
-	if (ret)
-		dev_err(this->dev, "Chip: %u, Error %d\n",
-			this->current_chip, ret);
-
-	this->command_length = 0;
-}
-
 static int gpmi_dev_ready(struct mtd_info *mtd)
 {
 	struct nand_chip *chip = mtd_to_nand(mtd);
@@ -1104,7 +1070,8 @@ 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;
+	struct gpmi_nand_data *this = nand_get_controller_data(chip);
+	int ret;
 	/* Large page devices (> 512 bytes) behave slightly differently. */
 	bool is_lp = mtd->writesize > 512;
 
@@ -1132,39 +1099,41 @@ static void gpmi_nand_command(struct mtd_info *mtd, unsigned int command,
 			column -= 256;
 			readcmd = NAND_CMD_READ1;
 		}
-		gpmi_cmd_ctrl(mtd, readcmd, ctrl);
-		ctrl &= ~NAND_CTRL_CHANGE;
+		this->cmd_buffer[this->command_length++] = readcmd;
 	}
 
 	/* Command latch cycle */
 	if (command != NAND_CMD_NONE)
-		gpmi_cmd_ctrl(mtd, command, ctrl);
+		this->cmd_buffer[this->command_length++] = command;
 
-	/* 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;
-		gpmi_cmd_ctrl(mtd, column, ctrl);
-		ctrl &= ~NAND_CTRL_CHANGE;
+		this->cmd_buffer[this->command_length++] = column;
 
 		/* Only output a single addr cycle for 8bits opcodes. */
 		if (is_lp && !nand_opcode_8bits(command))
-			gpmi_cmd_ctrl(mtd, column >> 8, ctrl);
+			this->cmd_buffer[this->command_length++] = column >> 8;
 	}
 	if (page_addr != -1) {
-		gpmi_cmd_ctrl(mtd, page_addr, ctrl);
-		ctrl &= ~NAND_CTRL_CHANGE;
-		gpmi_cmd_ctrl(mtd, page_addr >> 8, ctrl);
+		this->cmd_buffer[this->command_length++] = page_addr;
+		this->cmd_buffer[this->command_length++] = page_addr >> 8;
 		if (chip->options & NAND_ROW_ADDR_3)
-			gpmi_cmd_ctrl(mtd, page_addr >> 16, ctrl);
+			this->cmd_buffer[this->command_length++] = page_addr >> 16;
 	}
 
 	/* This starts the DMA for the command and waits for it to finish. */
-	gpmi_cmd_ctrl(mtd, NAND_CMD_NONE, NAND_NCE | NAND_CTRL_CHANGE);
+	if (this->command_length > 0) {
+		ret = gpmi_send_command(this);
+		if (ret)
+			dev_err(this->dev, "Chip: %u, Error %d\n",
+				this->current_chip, ret);
+
+		this->command_length = 0;
+	}
 
 	if (!is_lp)
 		return;
@@ -1172,10 +1141,12 @@ static void gpmi_nand_command(struct mtd_info *mtd, unsigned int command,
 	switch (command) {
 	case NAND_CMD_RNDOUT:
 		/* No ready / busy check necessary */
-		gpmi_cmd_ctrl(mtd, NAND_CMD_RNDOUTSTART,
-			      NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE);
-		gpmi_cmd_ctrl(mtd, NAND_CMD_NONE,
-			      NAND_NCE | NAND_CTRL_CHANGE);
+		this->cmd_buffer[this->command_length++] = NAND_CMD_RNDOUTSTART;
+		ret = gpmi_send_command(this);
+		if (ret)
+			dev_err(this->dev, "Chip: %u, Error %d\n",
+				this->current_chip, ret);
+		this->command_length = 0;
 		break;
 
 	case NAND_CMD_READ0:
@@ -1188,10 +1159,12 @@ static void gpmi_nand_command(struct mtd_info *mtd, unsigned int command,
 		if (column == -1 && page_addr == -1)
 			return;
 
-		gpmi_cmd_ctrl(mtd, NAND_CMD_READSTART,
-			      NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE);
-		gpmi_cmd_ctrl(mtd, NAND_CMD_NONE,
-			      NAND_NCE | NAND_CTRL_CHANGE);
+		this->cmd_buffer[this->command_length++] = NAND_CMD_READSTART;
+		ret = gpmi_send_command(this);
+		if (ret)
+			dev_err(this->dev, "Chip: %u, Error %d\n",
+				this->current_chip, ret);
+		this->command_length = 0;
 		break;
 	}
 }
-- 
2.14.1

[PATCH 17/18] mtd: rawnand: gpmi: gpmi_nand_command(): use separate sgl for the two commands

[Sam Lefebvre]

To enable chaining the two DMAs corresponding to the two commands that
can be issued by gpmi_nand_command(), duplicate the cmd_sgl and
command_length. For cmd_buffer, instead of duplicating it, we can use
a fixed offset within the buffer.

The appropriate sgl and command_length is passed to gpmi_send_command(),
and the sg_init_one(), dma_map_sg() and dma_unmap_sg() calls are hoisted
out of it. This is needed because the unmapping should only be done
after both commands have finished.

Signed-off-by: Sam Lefebvre <sam.lefebvre@essensium.com>
Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
 drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c  | 12 ++----
 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 68 +++++++++++++++++-------------
 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h |  9 +++-
 3 files changed, 49 insertions(+), 40 deletions(-)

diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
index 1858afdb400d..46b2208df30e 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
@@ -548,11 +548,11 @@ int gpmi_is_ready(struct gpmi_nand_data *this, unsigned chip)
 	return reg & mask;
 }
 
-int gpmi_send_command(struct gpmi_nand_data *this)
+int gpmi_send_command(struct gpmi_nand_data *this, struct scatterlist *sgl,
+		      unsigned int command_length)
 {
 	struct dma_chan *channel = get_dma_chan(this);
 	struct dma_async_tx_descriptor *desc;
-	struct scatterlist *sgl;
 	int chip = this->current_chip;
 	int ret;
 	u32 pio[3];
@@ -564,7 +564,7 @@ int gpmi_send_command(struct gpmi_nand_data *this)
 		| BF_GPMI_CTRL0_LOCK_CS(LOCK_CS_ENABLE, this)
 		| BF_GPMI_CTRL0_ADDRESS(BV_GPMI_CTRL0_ADDRESS__NAND_CLE)
 		| BM_GPMI_CTRL0_ADDRESS_INCREMENT
-		| BF_GPMI_CTRL0_XFER_COUNT(this->command_length);
+		| BF_GPMI_CTRL0_XFER_COUNT(command_length);
 	pio[1] = pio[2] = 0;
 	desc = dmaengine_prep_slave_sg(channel,
 					(struct scatterlist *)pio,
@@ -573,10 +573,6 @@ int gpmi_send_command(struct gpmi_nand_data *this)
 		return -EINVAL;
 
 	/* [2] send out the COMMAND + ADDRESS string stored in @buffer */
-	sgl = &this->cmd_sgl;
-
-	sg_init_one(sgl, this->cmd_buffer, this->command_length);
-	dma_map_sg(this->dev, sgl, 1, DMA_TO_DEVICE);
 	desc = dmaengine_prep_slave_sg(channel,
 				sgl, 1, DMA_MEM_TO_DEV,
 				DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
@@ -586,8 +582,6 @@ int gpmi_send_command(struct gpmi_nand_data *this)
 	/* [3] submit the DMA */
 	ret = start_dma_without_bch_irq(this, desc);
 
-	dma_unmap_sg(this->dev, sgl, 1, DMA_TO_DEVICE);
-
 	return ret;
 }
 
diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
index 97d44fe212c9..4455ea428255 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
@@ -1075,6 +1075,9 @@ static void gpmi_nand_command(struct mtd_info *mtd, unsigned int command,
 	/* Large page devices (> 512 bytes) behave slightly differently. */
 	bool is_lp = mtd->writesize > 512;
 
+	BUG_ON(this->command_length != 0);
+	BUG_ON(this->command_length2 != 0);
+
 	if (is_lp) {
 		/* Large page devices don't have the separate regions as we
 		 * have in the small page devices. We must emulate
@@ -1125,47 +1128,54 @@ static void gpmi_nand_command(struct mtd_info *mtd, unsigned int command,
 			this->cmd_buffer[this->command_length++] = page_addr >> 16;
 	}
 
+	/* Reuse the same cmd_buffer for the possible second command. The address
+	 * must be word-aligned. For convenience, use a fixed offset of 64, much
+	 * larger than the maximum command_length. */
+	if (is_lp)
+		switch (command) {
+		case NAND_CMD_RNDOUT:
+			/* No ready / busy check necessary */
+			this->cmd_buffer[64 + this->command_length2++] = NAND_CMD_RNDOUTSTART;
+			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)
+				this->cmd_buffer[64 + this->command_length2++] = NAND_CMD_READSTART;
+			break;
+		}
+
 	/* This starts the DMA for the command and waits for it to finish. */
 	if (this->command_length > 0) {
-		ret = gpmi_send_command(this);
+		sg_init_one(&this->cmd_sgl, this->cmd_buffer, this->command_length);
+		dma_map_sg(this->dev, &this->cmd_sgl, 1, DMA_TO_DEVICE);
+		ret = gpmi_send_command(this, &this->cmd_sgl, this->command_length);
 		if (ret)
 			dev_err(this->dev, "Chip: %u, Error %d\n",
 				this->current_chip, ret);
-
-		this->command_length = 0;
 	}
 
-	if (!is_lp)
-		return;
-
-	switch (command) {
-	case NAND_CMD_RNDOUT:
-		/* No ready / busy check necessary */
-		this->cmd_buffer[this->command_length++] = NAND_CMD_RNDOUTSTART;
-		ret = gpmi_send_command(this);
+	if (this->command_length2 > 0) {
+		sg_init_one(&this->cmd_sgl2, this->cmd_buffer + 64, this->command_length2);
+		dma_map_sg(this->dev, &this->cmd_sgl2, 1, DMA_TO_DEVICE);
+		ret = gpmi_send_command(this, &this->cmd_sgl2, this->command_length2);
 		if (ret)
 			dev_err(this->dev, "Chip: %u, Error %d\n",
 				this->current_chip, ret);
-		this->command_length = 0;
-		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;
+	}
 
-		this->cmd_buffer[this->command_length++] = NAND_CMD_READSTART;
-		ret = gpmi_send_command(this);
-		if (ret)
-			dev_err(this->dev, "Chip: %u, Error %d\n",
-				this->current_chip, ret);
+	if (this->command_length > 0) {
+		dma_unmap_sg(this->dev, &this->cmd_sgl, 1, DMA_TO_DEVICE);
 		this->command_length = 0;
-		break;
+	}
+	if (this->command_length2 > 0) {
+		dma_unmap_sg(this->dev, &this->cmd_sgl2, 1, DMA_TO_DEVICE);
+		this->command_length2 = 0;
 	}
 }
 
diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h
index 6aa10d6962d6..9dc3dd16fa0b 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h
@@ -139,11 +139,15 @@ struct gpmi_nand_data {
 
 	/* General-use Variables */
 	int			current_chip;
-	unsigned int		command_length;
 
+	unsigned int		command_length;
 	struct scatterlist	cmd_sgl;
 	char			*cmd_buffer;
 
+	unsigned int		command_length2;
+	struct scatterlist	cmd_sgl2;
+	char			*cmd_buffer2;
+
 	struct scatterlist	data_sgl;
 	char			*data_buffer_dma;
 
@@ -184,7 +188,8 @@ void gpmi_clear_bch(struct gpmi_nand_data *);
 void gpmi_dump_info(struct gpmi_nand_data *);
 int bch_set_geometry(struct gpmi_nand_data *);
 int gpmi_is_ready(struct gpmi_nand_data *, unsigned chip);
-int gpmi_send_command(struct gpmi_nand_data *);
+int gpmi_send_command(struct gpmi_nand_data *, struct scatterlist *sgl,
+		      unsigned int command_length);
 int gpmi_enable_clk(struct gpmi_nand_data *this);
 int gpmi_disable_clk(struct gpmi_nand_data *this);
 int gpmi_setup_data_interface(struct mtd_info *mtd, int chipnr,
-- 
2.14.1

[PATCH 18/18] mtd: rawnand: gpmi: issue two commands in a single DMA chain

[Sam Lefebvre]

gpmi_nand_command() may issue two commands. Instead of issuing them as
separate DMAs, chain them together and issue only a single DMA.

This removes one DMA interrupt and associated overhead. For full-page
reads with ECC, it reduces the number of interrupts from 4 per page to
3 per page (2 DMA interrupts + 1 BCH interrupt).

Signed-off-by: Sam Lefebvre <sam.lefebvre@essensium.com>
Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
 drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c  | 12 +++++++-----
 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c |  8 +++++---
 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h |  2 +-
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
index 46b2208df30e..8f5a2a242228 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
@@ -549,12 +549,12 @@ int gpmi_is_ready(struct gpmi_nand_data *this, unsigned chip)
 }
 
 int gpmi_send_command(struct gpmi_nand_data *this, struct scatterlist *sgl,
-		      unsigned int command_length)
+		      unsigned int command_length, bool chain, bool start_dma)
 {
 	struct dma_chan *channel = get_dma_chan(this);
 	struct dma_async_tx_descriptor *desc;
 	int chip = this->current_chip;
-	int ret;
+	int ret = 0;
 	u32 pio[3];
 
 	/* [1] send out the PIO words */
@@ -568,7 +568,8 @@ int gpmi_send_command(struct gpmi_nand_data *this, struct scatterlist *sgl,
 	pio[1] = pio[2] = 0;
 	desc = dmaengine_prep_slave_sg(channel,
 					(struct scatterlist *)pio,
-					ARRAY_SIZE(pio), DMA_TRANS_NONE, 0);
+					ARRAY_SIZE(pio), DMA_TRANS_NONE,
+					chain ? DMA_PREP_INTERRUPT : 0);
 	if (!desc)
 		return -EINVAL;
 
@@ -579,8 +580,9 @@ int gpmi_send_command(struct gpmi_nand_data *this, struct scatterlist *sgl,
 	if (!desc)
 		return -EINVAL;
 
-	/* [3] submit the DMA */
-	ret = start_dma_without_bch_irq(this, desc);
+	if (start_dma)
+		/* [3] submit the DMA */
+		ret = start_dma_without_bch_irq(this, desc);
 
 	return ret;
 }
diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
index 4455ea428255..abf227148b01 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
@@ -1150,11 +1150,12 @@ static void gpmi_nand_command(struct mtd_info *mtd, unsigned int command,
 			break;
 		}
 
-	/* This starts the DMA for the command and waits for it to finish. */
+	/* Chain the two commands, start the DMA and wait for it to finish. */
 	if (this->command_length > 0) {
 		sg_init_one(&this->cmd_sgl, this->cmd_buffer, this->command_length);
 		dma_map_sg(this->dev, &this->cmd_sgl, 1, DMA_TO_DEVICE);
-		ret = gpmi_send_command(this, &this->cmd_sgl, this->command_length);
+		ret = gpmi_send_command(this, &this->cmd_sgl, this->command_length,
+					false, this->command_length2 == 0);
 		if (ret)
 			dev_err(this->dev, "Chip: %u, Error %d\n",
 				this->current_chip, ret);
@@ -1163,7 +1164,8 @@ static void gpmi_nand_command(struct mtd_info *mtd, unsigned int command,
 	if (this->command_length2 > 0) {
 		sg_init_one(&this->cmd_sgl2, this->cmd_buffer + 64, this->command_length2);
 		dma_map_sg(this->dev, &this->cmd_sgl2, 1, DMA_TO_DEVICE);
-		ret = gpmi_send_command(this, &this->cmd_sgl2, this->command_length2);
+		ret = gpmi_send_command(this, &this->cmd_sgl2, this->command_length2,
+					this->command_length > 0, true);
 		if (ret)
 			dev_err(this->dev, "Chip: %u, Error %d\n",
 				this->current_chip, ret);
diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h
index 9dc3dd16fa0b..a09ec300754f 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h
@@ -189,7 +189,7 @@ void gpmi_dump_info(struct gpmi_nand_data *);
 int bch_set_geometry(struct gpmi_nand_data *);
 int gpmi_is_ready(struct gpmi_nand_data *, unsigned chip);
 int gpmi_send_command(struct gpmi_nand_data *, struct scatterlist *sgl,
-		      unsigned int command_length);
+		      unsigned int command_length, bool chain, bool start_dma);
 int gpmi_enable_clk(struct gpmi_nand_data *this);
 int gpmi_disable_clk(struct gpmi_nand_data *this);
 int gpmi_setup_data_interface(struct mtd_info *mtd, int chipnr,
-- 
2.14.1