[PATCH] mtd: spi-nor: fsl-quadspi: add big-endian support

[PATCH] mtd: spi-nor: fsl-quadspi: add big-endian support

[Yuan Yao]

Add R/W functions for big- or little-endian registers:
The qSPI controller's endian is independent of the CPU core's endian.
So far, the qSPI have two versions for big-endian and little-endian.

Signed-off-by: Yuan Yao <yao.yuan@freescale.com>
---
 drivers/mtd/spi-nor/fsl-quadspi.c | 151 +++++++++++++++++++++++---------------
 1 file changed, 91 insertions(+), 60 deletions(-)

diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
index dc38389..cc82379 100644
--- a/drivers/mtd/spi-nor/fsl-quadspi.c
+++ b/drivers/mtd/spi-nor/fsl-quadspi.c
@@ -275,6 +275,7 @@ struct fsl_qspi {
 	u32 clk_rate;
 	unsigned int chip_base_addr; /* We may support two chips. */
 	bool has_second_chip;
+	bool big_endian;
 	struct mutex lock;
 	struct pm_qos_request pm_qos_req;
 };
@@ -300,6 +301,28 @@ static inline int needs_wakeup_wait_mode(struct fsl_qspi *q)
 }
 
 /*
+ * R/W functions for big- or little-endian registers:
+ * The qSPI controller's endian is independent of the CPU core's endian.
+ * So far, although the CPU core is little-endian but the qSPI have two
+ * versions for big-endian and little-endian.
+ */
+static void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem *addr)
+{
+	if (q->big_endian)
+		iowrite32be(val, addr);
+	else
+		iowrite32(val, addr);
+}
+
+static u32 qspi_readl(struct fsl_qspi *q, void __iomem *addr)
+{
+	if (q->big_endian)
+		return ioread32be(addr);
+	else
+		return ioread32(addr);
+}
+
+/*
  * An IC bug makes us to re-arrange the 32-bit data.
  * The following chips, such as IMX6SLX, have fixed this bug.
  */
@@ -310,14 +333,14 @@ static inline u32 fsl_qspi_endian_xchg(struct fsl_qspi *q, u32 a)
 
 static inline void fsl_qspi_unlock_lut(struct fsl_qspi *q)
 {
-	writel(QUADSPI_LUTKEY_VALUE, q->iobase + QUADSPI_LUTKEY);
-	writel(QUADSPI_LCKER_UNLOCK, q->iobase + QUADSPI_LCKCR);
+	qspi_writel(q, QUADSPI_LUTKEY_VALUE, q->iobase + QUADSPI_LUTKEY);
+	qspi_writel(q, QUADSPI_LCKER_UNLOCK, q->iobase + QUADSPI_LCKCR);
 }
 
 static inline void fsl_qspi_lock_lut(struct fsl_qspi *q)
 {
-	writel(QUADSPI_LUTKEY_VALUE, q->iobase + QUADSPI_LUTKEY);
-	writel(QUADSPI_LCKER_LOCK, q->iobase + QUADSPI_LCKCR);
+	qspi_writel(q, QUADSPI_LUTKEY_VALUE, q->iobase + QUADSPI_LUTKEY);
+	qspi_writel(q, QUADSPI_LCKER_LOCK, q->iobase + QUADSPI_LCKCR);
 }
 
 static irqreturn_t fsl_qspi_irq_handler(int irq, void *dev_id)
@@ -326,8 +349,8 @@ static irqreturn_t fsl_qspi_irq_handler(int irq, void *dev_id)
 	u32 reg;
 
 	/* clear interrupt */
-	reg = readl(q->iobase + QUADSPI_FR);
-	writel(reg, q->iobase + QUADSPI_FR);
+	reg = qspi_readl(q, q->iobase + QUADSPI_FR);
+	qspi_writel(q, reg, q->iobase + QUADSPI_FR);
 
 	if (reg & QUADSPI_FR_TFF_MASK)
 		complete(&q->c);
@@ -348,7 +371,7 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
 
 	/* Clear all the LUT table */
 	for (i = 0; i < QUADSPI_LUT_NUM; i++)
-		writel(0, base + QUADSPI_LUT_BASE + i * 4);
+		qspi_writel(q, 0, base + QUADSPI_LUT_BASE + i * 4);
 
 	/* Quad Read */
 	lut_base = SEQID_QUAD_READ * 4;
@@ -364,14 +387,15 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
 		dummy = 8;
 	}
 
-	writel(LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
+	qspi_writel(q, LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
 			base + QUADSPI_LUT(lut_base));
-	writel(LUT0(DUMMY, PAD1, dummy) | LUT1(READ, PAD4, rxfifo),
+	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(READ, PAD4, rxfifo),
 			base + QUADSPI_LUT(lut_base + 1));
 
 	/* Write enable */
 	lut_base = SEQID_WREN * 4;
-	writel(LUT0(CMD, PAD1, SPINOR_OP_WREN), base + QUADSPI_LUT(lut_base));
+	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_WREN),
+			base + QUADSPI_LUT(lut_base));
 
 	/* Page Program */
 	lut_base = SEQID_PP * 4;
@@ -385,13 +409,13 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
 		addrlen = ADDR32BIT;
 	}
 
-	writel(LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
+	qspi_writel(q, LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
 			base + QUADSPI_LUT(lut_base));
-	writel(LUT0(WRITE, PAD1, 0), base + QUADSPI_LUT(lut_base + 1));
+	qspi_writel(q, LUT0(WRITE, PAD1, 0), base + QUADSPI_LUT(lut_base + 1));
 
 	/* Read Status */
 	lut_base = SEQID_RDSR * 4;
-	writel(LUT0(CMD, PAD1, SPINOR_OP_RDSR) | LUT1(READ, PAD1, 0x1),
+	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_RDSR) | LUT1(READ, PAD1, 0x1),
 			base + QUADSPI_LUT(lut_base));
 
 	/* Erase a sector */
@@ -400,40 +424,43 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
 	cmd = q->nor[0].erase_opcode;
 	addrlen = q->nor_size <= SZ_16M ? ADDR24BIT : ADDR32BIT;
 
-	writel(LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
+	qspi_writel(q, LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
 			base + QUADSPI_LUT(lut_base));
 
 	/* Erase the whole chip */
 	lut_base = SEQID_CHIP_ERASE * 4;
-	writel(LUT0(CMD, PAD1, SPINOR_OP_CHIP_ERASE),
+	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_CHIP_ERASE),
 			base + QUADSPI_LUT(lut_base));
 
 	/* READ ID */
 	lut_base = SEQID_RDID * 4;
-	writel(LUT0(CMD, PAD1, SPINOR_OP_RDID) | LUT1(READ, PAD1, 0x8),
+	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_RDID) | LUT1(READ, PAD1, 0x8),
 			base + QUADSPI_LUT(lut_base));
 
 	/* Write Register */
 	lut_base = SEQID_WRSR * 4;
-	writel(LUT0(CMD, PAD1, SPINOR_OP_WRSR) | LUT1(WRITE, PAD1, 0x2),
+	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_WRSR) | LUT1(WRITE, PAD1, 0x2),
 			base + QUADSPI_LUT(lut_base));
 
 	/* Read Configuration Register */
 	lut_base = SEQID_RDCR * 4;
-	writel(LUT0(CMD, PAD1, SPINOR_OP_RDCR) | LUT1(READ, PAD1, 0x1),
+	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_RDCR) | LUT1(READ, PAD1, 0x1),
 			base + QUADSPI_LUT(lut_base));
 
 	/* Write disable */
 	lut_base = SEQID_WRDI * 4;
-	writel(LUT0(CMD, PAD1, SPINOR_OP_WRDI), base + QUADSPI_LUT(lut_base));
+	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_WRDI),
+			base + QUADSPI_LUT(lut_base));
 
 	/* Enter 4 Byte Mode (Micron) */
 	lut_base = SEQID_EN4B * 4;
-	writel(LUT0(CMD, PAD1, SPINOR_OP_EN4B), base + QUADSPI_LUT(lut_base));
+	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_EN4B),
+			base + QUADSPI_LUT(lut_base));
 
 	/* Enter 4 Byte Mode (Spansion) */
 	lut_base = SEQID_BRWR * 4;
-	writel(LUT0(CMD, PAD1, SPINOR_OP_BRWR), base + QUADSPI_LUT(lut_base));
+	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_BRWR),
+			base + QUADSPI_LUT(lut_base));
 
 	fsl_qspi_lock_lut(q);
 }
@@ -488,15 +515,16 @@ fsl_qspi_runcmd(struct fsl_qspi *q, u8 cmd, unsigned int addr, int len)
 			q->chip_base_addr, addr, len, cmd);
 
 	/* save the reg */
-	reg = readl(base + QUADSPI_MCR);
+	reg = qspi_readl(q, base + QUADSPI_MCR);
 
-	writel(q->memmap_phy + q->chip_base_addr + addr, base + QUADSPI_SFAR);
-	writel(QUADSPI_RBCT_WMRK_MASK | QUADSPI_RBCT_RXBRD_USEIPS,
+	qspi_writel(q, q->memmap_phy + q->chip_base_addr + addr,
+			base + QUADSPI_SFAR);
+	qspi_writel(q, QUADSPI_RBCT_WMRK_MASK | QUADSPI_RBCT_RXBRD_USEIPS,
 			base + QUADSPI_RBCT);
-	writel(reg | QUADSPI_MCR_CLR_RXF_MASK, base + QUADSPI_MCR);
+	qspi_writel(q, reg | QUADSPI_MCR_CLR_RXF_MASK, base + QUADSPI_MCR);
 
 	do {
-		reg2 = readl(base + QUADSPI_SR);
+		reg2 = qspi_readl(q, base + QUADSPI_SR);
 		if (reg2 & (QUADSPI_SR_IP_ACC_MASK | QUADSPI_SR_AHB_ACC_MASK)) {
 			udelay(1);
 			dev_dbg(q->dev, "The controller is busy, 0x%x\n", reg2);
@@ -507,21 +535,22 @@ fsl_qspi_runcmd(struct fsl_qspi *q, u8 cmd, unsigned int addr, int len)
 
 	/* trigger the LUT now */
 	seqid = fsl_qspi_get_seqid(q, cmd);
-	writel((seqid << QUADSPI_IPCR_SEQID_SHIFT) | len, base + QUADSPI_IPCR);
+	qspi_writel(q, (seqid << QUADSPI_IPCR_SEQID_SHIFT) | len,
+			base + QUADSPI_IPCR);
 
 	/* Wait for the interrupt. */
 	if (!wait_for_completion_timeout(&q->c, msecs_to_jiffies(1000))) {
 		dev_err(q->dev,
 			"cmd 0x%.2x timeout, addr@%.8x, FR:0x%.8x, SR:0x%.8x\n",
-			cmd, addr, readl(base + QUADSPI_FR),
-			readl(base + QUADSPI_SR));
+			cmd, addr, qspi_readl(q, base + QUADSPI_FR),
+			qspi_readl(q, base + QUADSPI_SR));
 		err = -ETIMEDOUT;
 	} else {
 		err = 0;
 	}
 
 	/* restore the MCR */
-	writel(reg, base + QUADSPI_MCR);
+	qspi_writel(q, reg, base + QUADSPI_MCR);
 
 	return err;
 }
@@ -533,7 +562,7 @@ static void fsl_qspi_read_data(struct fsl_qspi *q, int len, u8 *rxbuf)
 	int i = 0;
 
 	while (len > 0) {
-		tmp = readl(q->iobase + QUADSPI_RBDR + i * 4);
+		tmp = qspi_readl(q, q->iobase + QUADSPI_RBDR + i * 4);
 		tmp = fsl_qspi_endian_xchg(q, tmp);
 		dev_dbg(q->dev, "chip addr:0x%.8x, rcv:0x%.8x\n",
 				q->chip_base_addr, tmp);
@@ -561,9 +590,9 @@ static inline void fsl_qspi_invalid(struct fsl_qspi *q)
 {
 	u32 reg;
 
-	reg = readl(q->iobase + QUADSPI_MCR);
+	reg = qspi_readl(q, q->iobase + QUADSPI_MCR);
 	reg |= QUADSPI_MCR_SWRSTHD_MASK | QUADSPI_MCR_SWRSTSD_MASK;
-	writel(reg, q->iobase + QUADSPI_MCR);
+	qspi_writel(q, reg, q->iobase + QUADSPI_MCR);
 
 	/*
 	 * The minimum delay : 1 AHB + 2 SFCK clocks.
@@ -572,7 +601,7 @@ static inline void fsl_qspi_invalid(struct fsl_qspi *q)
 	udelay(1);
 
 	reg &= ~(QUADSPI_MCR_SWRSTHD_MASK | QUADSPI_MCR_SWRSTSD_MASK);
-	writel(reg, q->iobase + QUADSPI_MCR);
+	qspi_writel(q, reg, q->iobase + QUADSPI_MCR);
 }
 
 static int fsl_qspi_nor_write(struct fsl_qspi *q, struct spi_nor *nor,
@@ -586,20 +615,20 @@ static int fsl_qspi_nor_write(struct fsl_qspi *q, struct spi_nor *nor,
 		q->chip_base_addr, to, count);
 
 	/* clear the TX FIFO. */
-	tmp = readl(q->iobase + QUADSPI_MCR);
-	writel(tmp | QUADSPI_MCR_CLR_TXF_MASK, q->iobase + QUADSPI_MCR);
+	tmp = qspi_readl(q, q->iobase + QUADSPI_MCR);
+	qspi_writel(q, tmp | QUADSPI_MCR_CLR_TXF_MASK, q->iobase + QUADSPI_MCR);
 
 	/* fill the TX data to the FIFO */
 	for (j = 0, i = ((count + 3) / 4); j < i; j++) {
 		tmp = fsl_qspi_endian_xchg(q, *txbuf);
-		writel(tmp, q->iobase + QUADSPI_TBDR);
+		qspi_writel(q, tmp, q->iobase + QUADSPI_TBDR);
 		txbuf++;
 	}
 
 	/* fill the TXFIFO upto 16 bytes for i.MX7d */
 	if (needs_fill_txfifo(q))
 		for (; i < 4; i++)
-			writel(tmp, q->iobase + QUADSPI_TBDR);
+			qspi_writel(q, tmp, q->iobase + QUADSPI_TBDR);
 
 	/* Trigger it */
 	ret = fsl_qspi_runcmd(q, opcode, to, count);
@@ -615,10 +644,10 @@ static void fsl_qspi_set_map_addr(struct fsl_qspi *q)
 	int nor_size = q->nor_size;
 	void __iomem *base = q->iobase;
 
-	writel(nor_size + q->memmap_phy, base + QUADSPI_SFA1AD);
-	writel(nor_size * 2 + q->memmap_phy, base + QUADSPI_SFA2AD);
-	writel(nor_size * 3 + q->memmap_phy, base + QUADSPI_SFB1AD);
-	writel(nor_size * 4 + q->memmap_phy, base + QUADSPI_SFB2AD);
+	qspi_writel(q, nor_size + q->memmap_phy, base + QUADSPI_SFA1AD);
+	qspi_writel(q, nor_size * 2 + q->memmap_phy, base + QUADSPI_SFA2AD);
+	qspi_writel(q, nor_size * 3 + q->memmap_phy, base + QUADSPI_SFB1AD);
+	qspi_writel(q, nor_size * 4 + q->memmap_phy, base + QUADSPI_SFB2AD);
 }
 
 /*
@@ -640,24 +669,25 @@ static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
 	int seqid;
 
 	/* AHB configuration for access buffer 0/1/2 .*/
-	writel(QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF0CR);
-	writel(QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF1CR);
-	writel(QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF2CR);
+	qspi_writel(q, QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF0CR);
+	qspi_writel(q, QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF1CR);
+	qspi_writel(q, QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF2CR);
 	/*
 	 * Set ADATSZ with the maximum AHB buffer size to improve the
 	 * read performance.
 	 */
-	writel(QUADSPI_BUF3CR_ALLMST_MASK | ((q->devtype_data->ahb_buf_size / 8)
-			<< QUADSPI_BUF3CR_ADATSZ_SHIFT), base + QUADSPI_BUF3CR);
+	qspi_writel(q, QUADSPI_BUF3CR_ALLMST_MASK |
+			((q->devtype_data->ahb_buf_size / 8) <<
+			 QUADSPI_BUF3CR_ADATSZ_SHIFT), base + QUADSPI_BUF3CR);
 
 	/* We only use the buffer3 */
-	writel(0, base + QUADSPI_BUF0IND);
-	writel(0, base + QUADSPI_BUF1IND);
-	writel(0, base + QUADSPI_BUF2IND);
+	qspi_writel(q, 0, base + QUADSPI_BUF0IND);
+	qspi_writel(q, 0, base + QUADSPI_BUF1IND);
+	qspi_writel(q, 0, base + QUADSPI_BUF2IND);
 
 	/* Set the default lut sequence for AHB Read. */
 	seqid = fsl_qspi_get_seqid(q, q->nor[0].read_opcode);
-	writel(seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
+	qspi_writel(q, seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
 		q->iobase + QUADSPI_BFGENCR);
 }
 
@@ -713,7 +743,7 @@ static int fsl_qspi_nor_setup(struct fsl_qspi *q)
 		return ret;
 
 	/* Reset the module */
-	writel(QUADSPI_MCR_SWRSTSD_MASK | QUADSPI_MCR_SWRSTHD_MASK,
+	qspi_writel(q, QUADSPI_MCR_SWRSTSD_MASK | QUADSPI_MCR_SWRSTHD_MASK,
 		base + QUADSPI_MCR);
 	udelay(1);
 
@@ -721,24 +751,24 @@ static int fsl_qspi_nor_setup(struct fsl_qspi *q)
 	fsl_qspi_init_lut(q);
 
 	/* Disable the module */
-	writel(QUADSPI_MCR_MDIS_MASK | QUADSPI_MCR_RESERVED_MASK,
+	qspi_writel(q, QUADSPI_MCR_MDIS_MASK | QUADSPI_MCR_RESERVED_MASK,
 			base + QUADSPI_MCR);
 
-	reg = readl(base + QUADSPI_SMPR);
-	writel(reg & ~(QUADSPI_SMPR_FSDLY_MASK
+	reg = qspi_readl(q, base + QUADSPI_SMPR);
+	qspi_writel(q, reg & ~(QUADSPI_SMPR_FSDLY_MASK
 			| QUADSPI_SMPR_FSPHS_MASK
 			| QUADSPI_SMPR_HSENA_MASK
 			| QUADSPI_SMPR_DDRSMP_MASK), base + QUADSPI_SMPR);
 
 	/* Enable the module */
-	writel(QUADSPI_MCR_RESERVED_MASK | QUADSPI_MCR_END_CFG_MASK,
+	qspi_writel(q, QUADSPI_MCR_RESERVED_MASK | QUADSPI_MCR_END_CFG_MASK,
 			base + QUADSPI_MCR);
 
 	/* clear all interrupt status */
-	writel(0xffffffff, q->iobase + QUADSPI_FR);
+	qspi_writel(q, 0xffffffff, q->iobase + QUADSPI_FR);
 
 	/* enable the interrupt */
-	writel(QUADSPI_RSER_TFIE, q->iobase + QUADSPI_RSER);
+	qspi_writel(q, QUADSPI_RSER_TFIE, q->iobase + QUADSPI_RSER);
 
 	return 0;
 }
@@ -955,6 +985,7 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 	if (IS_ERR(q->iobase))
 		return PTR_ERR(q->iobase);
 
+	q->big_endian = of_property_read_bool(np, "big-endian");
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
 					"QuadSPI-memory");
 	if (!devm_request_mem_region(dev, res->start, resource_size(res),
@@ -1103,8 +1134,8 @@ static int fsl_qspi_remove(struct platform_device *pdev)
 	}
 
 	/* disable the hardware */
-	writel(QUADSPI_MCR_MDIS_MASK, q->iobase + QUADSPI_MCR);
-	writel(0x0, q->iobase + QUADSPI_RSER);
+	qspi_writel(q, QUADSPI_MCR_MDIS_MASK, q->iobase + QUADSPI_MCR);
+	qspi_writel(q, 0x0, q->iobase + QUADSPI_RSER);
 
 	mutex_destroy(&q->lock);
 
-- 
2.1.0.27.g96db324

Re: [PATCH] mtd: spi-nor: fsl-quadspi: add big-endian support

[Brian Norris]

+ Han

Make sure to check MAINTAINERS (or scripts/get_maintainers.pl, but you
have to be smart there; sometimes that script gives you too big of a CC
list).

Brian

On Fri, Oct 23, 2015 at 03:53:17PM +0800, Yuan Yao wrote:
> Add R/W functions for big- or little-endian registers:
> The qSPI controller's endian is independent of the CPU core's endian.
> So far, the qSPI have two versions for big-endian and little-endian.
> 
> Signed-off-by: Yuan Yao <yao.yuan@freescale.com>
> ---
>  drivers/mtd/spi-nor/fsl-quadspi.c | 151 +++++++++++++++++++++++---------------
>  1 file changed, 91 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
> index dc38389..cc82379 100644
> --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> @@ -275,6 +275,7 @@ struct fsl_qspi {
>  	u32 clk_rate;
>  	unsigned int chip_base_addr; /* We may support two chips. */
>  	bool has_second_chip;
> +	bool big_endian;
>  	struct mutex lock;
>  	struct pm_qos_request pm_qos_req;
>  };
> @@ -300,6 +301,28 @@ static inline int needs_wakeup_wait_mode(struct fsl_qspi *q)
>  }
>  
>  /*
> + * R/W functions for big- or little-endian registers:
> + * The qSPI controller's endian is independent of the CPU core's endian.
> + * So far, although the CPU core is little-endian but the qSPI have two
> + * versions for big-endian and little-endian.
> + */
> +static void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem *addr)
> +{
> +	if (q->big_endian)
> +		iowrite32be(val, addr);
> +	else
> +		iowrite32(val, addr);
> +}
> +
> +static u32 qspi_readl(struct fsl_qspi *q, void __iomem *addr)
> +{
> +	if (q->big_endian)
> +		return ioread32be(addr);
> +	else
> +		return ioread32(addr);
> +}
> +
> +/*
>   * An IC bug makes us to re-arrange the 32-bit data.
>   * The following chips, such as IMX6SLX, have fixed this bug.
>   */
> @@ -310,14 +333,14 @@ static inline u32 fsl_qspi_endian_xchg(struct fsl_qspi *q, u32 a)
>  
>  static inline void fsl_qspi_unlock_lut(struct fsl_qspi *q)
>  {
> -	writel(QUADSPI_LUTKEY_VALUE, q->iobase + QUADSPI_LUTKEY);
> -	writel(QUADSPI_LCKER_UNLOCK, q->iobase + QUADSPI_LCKCR);
> +	qspi_writel(q, QUADSPI_LUTKEY_VALUE, q->iobase + QUADSPI_LUTKEY);
> +	qspi_writel(q, QUADSPI_LCKER_UNLOCK, q->iobase + QUADSPI_LCKCR);
>  }
>  
>  static inline void fsl_qspi_lock_lut(struct fsl_qspi *q)
>  {
> -	writel(QUADSPI_LUTKEY_VALUE, q->iobase + QUADSPI_LUTKEY);
> -	writel(QUADSPI_LCKER_LOCK, q->iobase + QUADSPI_LCKCR);
> +	qspi_writel(q, QUADSPI_LUTKEY_VALUE, q->iobase + QUADSPI_LUTKEY);
> +	qspi_writel(q, QUADSPI_LCKER_LOCK, q->iobase + QUADSPI_LCKCR);
>  }
>  
>  static irqreturn_t fsl_qspi_irq_handler(int irq, void *dev_id)
> @@ -326,8 +349,8 @@ static irqreturn_t fsl_qspi_irq_handler(int irq, void *dev_id)
>  	u32 reg;
>  
>  	/* clear interrupt */
> -	reg = readl(q->iobase + QUADSPI_FR);
> -	writel(reg, q->iobase + QUADSPI_FR);
> +	reg = qspi_readl(q, q->iobase + QUADSPI_FR);
> +	qspi_writel(q, reg, q->iobase + QUADSPI_FR);
>  
>  	if (reg & QUADSPI_FR_TFF_MASK)
>  		complete(&q->c);
> @@ -348,7 +371,7 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  
>  	/* Clear all the LUT table */
>  	for (i = 0; i < QUADSPI_LUT_NUM; i++)
> -		writel(0, base + QUADSPI_LUT_BASE + i * 4);
> +		qspi_writel(q, 0, base + QUADSPI_LUT_BASE + i * 4);
>  
>  	/* Quad Read */
>  	lut_base = SEQID_QUAD_READ * 4;
> @@ -364,14 +387,15 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  		dummy = 8;
>  	}
>  
> -	writel(LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
> +	qspi_writel(q, LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
>  			base + QUADSPI_LUT(lut_base));
> -	writel(LUT0(DUMMY, PAD1, dummy) | LUT1(READ, PAD4, rxfifo),
> +	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(READ, PAD4, rxfifo),
>  			base + QUADSPI_LUT(lut_base + 1));
>  
>  	/* Write enable */
>  	lut_base = SEQID_WREN * 4;
> -	writel(LUT0(CMD, PAD1, SPINOR_OP_WREN), base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_WREN),
> +			base + QUADSPI_LUT(lut_base));
>  
>  	/* Page Program */
>  	lut_base = SEQID_PP * 4;
> @@ -385,13 +409,13 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  		addrlen = ADDR32BIT;
>  	}
>  
> -	writel(LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
> +	qspi_writel(q, LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
>  			base + QUADSPI_LUT(lut_base));
> -	writel(LUT0(WRITE, PAD1, 0), base + QUADSPI_LUT(lut_base + 1));
> +	qspi_writel(q, LUT0(WRITE, PAD1, 0), base + QUADSPI_LUT(lut_base + 1));
>  
>  	/* Read Status */
>  	lut_base = SEQID_RDSR * 4;
> -	writel(LUT0(CMD, PAD1, SPINOR_OP_RDSR) | LUT1(READ, PAD1, 0x1),
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_RDSR) | LUT1(READ, PAD1, 0x1),
>  			base + QUADSPI_LUT(lut_base));
>  
>  	/* Erase a sector */
> @@ -400,40 +424,43 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	cmd = q->nor[0].erase_opcode;
>  	addrlen = q->nor_size <= SZ_16M ? ADDR24BIT : ADDR32BIT;
>  
> -	writel(LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
> +	qspi_writel(q, LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
>  			base + QUADSPI_LUT(lut_base));
>  
>  	/* Erase the whole chip */
>  	lut_base = SEQID_CHIP_ERASE * 4;
> -	writel(LUT0(CMD, PAD1, SPINOR_OP_CHIP_ERASE),
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_CHIP_ERASE),
>  			base + QUADSPI_LUT(lut_base));
>  
>  	/* READ ID */
>  	lut_base = SEQID_RDID * 4;
> -	writel(LUT0(CMD, PAD1, SPINOR_OP_RDID) | LUT1(READ, PAD1, 0x8),
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_RDID) | LUT1(READ, PAD1, 0x8),
>  			base + QUADSPI_LUT(lut_base));
>  
>  	/* Write Register */
>  	lut_base = SEQID_WRSR * 4;
> -	writel(LUT0(CMD, PAD1, SPINOR_OP_WRSR) | LUT1(WRITE, PAD1, 0x2),
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_WRSR) | LUT1(WRITE, PAD1, 0x2),
>  			base + QUADSPI_LUT(lut_base));
>  
>  	/* Read Configuration Register */
>  	lut_base = SEQID_RDCR * 4;
> -	writel(LUT0(CMD, PAD1, SPINOR_OP_RDCR) | LUT1(READ, PAD1, 0x1),
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_RDCR) | LUT1(READ, PAD1, 0x1),
>  			base + QUADSPI_LUT(lut_base));
>  
>  	/* Write disable */
>  	lut_base = SEQID_WRDI * 4;
> -	writel(LUT0(CMD, PAD1, SPINOR_OP_WRDI), base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_WRDI),
> +			base + QUADSPI_LUT(lut_base));
>  
>  	/* Enter 4 Byte Mode (Micron) */
>  	lut_base = SEQID_EN4B * 4;
> -	writel(LUT0(CMD, PAD1, SPINOR_OP_EN4B), base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_EN4B),
> +			base + QUADSPI_LUT(lut_base));
>  
>  	/* Enter 4 Byte Mode (Spansion) */
>  	lut_base = SEQID_BRWR * 4;
> -	writel(LUT0(CMD, PAD1, SPINOR_OP_BRWR), base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_BRWR),
> +			base + QUADSPI_LUT(lut_base));
>  
>  	fsl_qspi_lock_lut(q);
>  }
> @@ -488,15 +515,16 @@ fsl_qspi_runcmd(struct fsl_qspi *q, u8 cmd, unsigned int addr, int len)
>  			q->chip_base_addr, addr, len, cmd);
>  
>  	/* save the reg */
> -	reg = readl(base + QUADSPI_MCR);
> +	reg = qspi_readl(q, base + QUADSPI_MCR);
>  
> -	writel(q->memmap_phy + q->chip_base_addr + addr, base + QUADSPI_SFAR);
> -	writel(QUADSPI_RBCT_WMRK_MASK | QUADSPI_RBCT_RXBRD_USEIPS,
> +	qspi_writel(q, q->memmap_phy + q->chip_base_addr + addr,
> +			base + QUADSPI_SFAR);
> +	qspi_writel(q, QUADSPI_RBCT_WMRK_MASK | QUADSPI_RBCT_RXBRD_USEIPS,
>  			base + QUADSPI_RBCT);
> -	writel(reg | QUADSPI_MCR_CLR_RXF_MASK, base + QUADSPI_MCR);
> +	qspi_writel(q, reg | QUADSPI_MCR_CLR_RXF_MASK, base + QUADSPI_MCR);
>  
>  	do {
> -		reg2 = readl(base + QUADSPI_SR);
> +		reg2 = qspi_readl(q, base + QUADSPI_SR);
>  		if (reg2 & (QUADSPI_SR_IP_ACC_MASK | QUADSPI_SR_AHB_ACC_MASK)) {
>  			udelay(1);
>  			dev_dbg(q->dev, "The controller is busy, 0x%x\n", reg2);
> @@ -507,21 +535,22 @@ fsl_qspi_runcmd(struct fsl_qspi *q, u8 cmd, unsigned int addr, int len)
>  
>  	/* trigger the LUT now */
>  	seqid = fsl_qspi_get_seqid(q, cmd);
> -	writel((seqid << QUADSPI_IPCR_SEQID_SHIFT) | len, base + QUADSPI_IPCR);
> +	qspi_writel(q, (seqid << QUADSPI_IPCR_SEQID_SHIFT) | len,
> +			base + QUADSPI_IPCR);
>  
>  	/* Wait for the interrupt. */
>  	if (!wait_for_completion_timeout(&q->c, msecs_to_jiffies(1000))) {
>  		dev_err(q->dev,
>  			"cmd 0x%.2x timeout, addr@%.8x, FR:0x%.8x, SR:0x%.8x\n",
> -			cmd, addr, readl(base + QUADSPI_FR),
> -			readl(base + QUADSPI_SR));
> +			cmd, addr, qspi_readl(q, base + QUADSPI_FR),
> +			qspi_readl(q, base + QUADSPI_SR));
>  		err = -ETIMEDOUT;
>  	} else {
>  		err = 0;
>  	}
>  
>  	/* restore the MCR */
> -	writel(reg, base + QUADSPI_MCR);
> +	qspi_writel(q, reg, base + QUADSPI_MCR);
>  
>  	return err;
>  }
> @@ -533,7 +562,7 @@ static void fsl_qspi_read_data(struct fsl_qspi *q, int len, u8 *rxbuf)
>  	int i = 0;
>  
>  	while (len > 0) {
> -		tmp = readl(q->iobase + QUADSPI_RBDR + i * 4);
> +		tmp = qspi_readl(q, q->iobase + QUADSPI_RBDR + i * 4);
>  		tmp = fsl_qspi_endian_xchg(q, tmp);
>  		dev_dbg(q->dev, "chip addr:0x%.8x, rcv:0x%.8x\n",
>  				q->chip_base_addr, tmp);
> @@ -561,9 +590,9 @@ static inline void fsl_qspi_invalid(struct fsl_qspi *q)
>  {
>  	u32 reg;
>  
> -	reg = readl(q->iobase + QUADSPI_MCR);
> +	reg = qspi_readl(q, q->iobase + QUADSPI_MCR);
>  	reg |= QUADSPI_MCR_SWRSTHD_MASK | QUADSPI_MCR_SWRSTSD_MASK;
> -	writel(reg, q->iobase + QUADSPI_MCR);
> +	qspi_writel(q, reg, q->iobase + QUADSPI_MCR);
>  
>  	/*
>  	 * The minimum delay : 1 AHB + 2 SFCK clocks.
> @@ -572,7 +601,7 @@ static inline void fsl_qspi_invalid(struct fsl_qspi *q)
>  	udelay(1);
>  
>  	reg &= ~(QUADSPI_MCR_SWRSTHD_MASK | QUADSPI_MCR_SWRSTSD_MASK);
> -	writel(reg, q->iobase + QUADSPI_MCR);
> +	qspi_writel(q, reg, q->iobase + QUADSPI_MCR);
>  }
>  
>  static int fsl_qspi_nor_write(struct fsl_qspi *q, struct spi_nor *nor,
> @@ -586,20 +615,20 @@ static int fsl_qspi_nor_write(struct fsl_qspi *q, struct spi_nor *nor,
>  		q->chip_base_addr, to, count);
>  
>  	/* clear the TX FIFO. */
> -	tmp = readl(q->iobase + QUADSPI_MCR);
> -	writel(tmp | QUADSPI_MCR_CLR_TXF_MASK, q->iobase + QUADSPI_MCR);
> +	tmp = qspi_readl(q, q->iobase + QUADSPI_MCR);
> +	qspi_writel(q, tmp | QUADSPI_MCR_CLR_TXF_MASK, q->iobase + QUADSPI_MCR);
>  
>  	/* fill the TX data to the FIFO */
>  	for (j = 0, i = ((count + 3) / 4); j < i; j++) {
>  		tmp = fsl_qspi_endian_xchg(q, *txbuf);
> -		writel(tmp, q->iobase + QUADSPI_TBDR);
> +		qspi_writel(q, tmp, q->iobase + QUADSPI_TBDR);
>  		txbuf++;
>  	}
>  
>  	/* fill the TXFIFO upto 16 bytes for i.MX7d */
>  	if (needs_fill_txfifo(q))
>  		for (; i < 4; i++)
> -			writel(tmp, q->iobase + QUADSPI_TBDR);
> +			qspi_writel(q, tmp, q->iobase + QUADSPI_TBDR);
>  
>  	/* Trigger it */
>  	ret = fsl_qspi_runcmd(q, opcode, to, count);
> @@ -615,10 +644,10 @@ static void fsl_qspi_set_map_addr(struct fsl_qspi *q)
>  	int nor_size = q->nor_size;
>  	void __iomem *base = q->iobase;
>  
> -	writel(nor_size + q->memmap_phy, base + QUADSPI_SFA1AD);
> -	writel(nor_size * 2 + q->memmap_phy, base + QUADSPI_SFA2AD);
> -	writel(nor_size * 3 + q->memmap_phy, base + QUADSPI_SFB1AD);
> -	writel(nor_size * 4 + q->memmap_phy, base + QUADSPI_SFB2AD);
> +	qspi_writel(q, nor_size + q->memmap_phy, base + QUADSPI_SFA1AD);
> +	qspi_writel(q, nor_size * 2 + q->memmap_phy, base + QUADSPI_SFA2AD);
> +	qspi_writel(q, nor_size * 3 + q->memmap_phy, base + QUADSPI_SFB1AD);
> +	qspi_writel(q, nor_size * 4 + q->memmap_phy, base + QUADSPI_SFB2AD);
>  }
>  
>  /*
> @@ -640,24 +669,25 @@ static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
>  	int seqid;
>  
>  	/* AHB configuration for access buffer 0/1/2 .*/
> -	writel(QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF0CR);
> -	writel(QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF1CR);
> -	writel(QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF2CR);
> +	qspi_writel(q, QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF0CR);
> +	qspi_writel(q, QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF1CR);
> +	qspi_writel(q, QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF2CR);
>  	/*
>  	 * Set ADATSZ with the maximum AHB buffer size to improve the
>  	 * read performance.
>  	 */
> -	writel(QUADSPI_BUF3CR_ALLMST_MASK | ((q->devtype_data->ahb_buf_size / 8)
> -			<< QUADSPI_BUF3CR_ADATSZ_SHIFT), base + QUADSPI_BUF3CR);
> +	qspi_writel(q, QUADSPI_BUF3CR_ALLMST_MASK |
> +			((q->devtype_data->ahb_buf_size / 8) <<
> +			 QUADSPI_BUF3CR_ADATSZ_SHIFT), base + QUADSPI_BUF3CR);
>  
>  	/* We only use the buffer3 */
> -	writel(0, base + QUADSPI_BUF0IND);
> -	writel(0, base + QUADSPI_BUF1IND);
> -	writel(0, base + QUADSPI_BUF2IND);
> +	qspi_writel(q, 0, base + QUADSPI_BUF0IND);
> +	qspi_writel(q, 0, base + QUADSPI_BUF1IND);
> +	qspi_writel(q, 0, base + QUADSPI_BUF2IND);
>  
>  	/* Set the default lut sequence for AHB Read. */
>  	seqid = fsl_qspi_get_seqid(q, q->nor[0].read_opcode);
> -	writel(seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
> +	qspi_writel(q, seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
>  		q->iobase + QUADSPI_BFGENCR);
>  }
>  
> @@ -713,7 +743,7 @@ static int fsl_qspi_nor_setup(struct fsl_qspi *q)
>  		return ret;
>  
>  	/* Reset the module */
> -	writel(QUADSPI_MCR_SWRSTSD_MASK | QUADSPI_MCR_SWRSTHD_MASK,
> +	qspi_writel(q, QUADSPI_MCR_SWRSTSD_MASK | QUADSPI_MCR_SWRSTHD_MASK,
>  		base + QUADSPI_MCR);
>  	udelay(1);
>  
> @@ -721,24 +751,24 @@ static int fsl_qspi_nor_setup(struct fsl_qspi *q)
>  	fsl_qspi_init_lut(q);
>  
>  	/* Disable the module */
> -	writel(QUADSPI_MCR_MDIS_MASK | QUADSPI_MCR_RESERVED_MASK,
> +	qspi_writel(q, QUADSPI_MCR_MDIS_MASK | QUADSPI_MCR_RESERVED_MASK,
>  			base + QUADSPI_MCR);
>  
> -	reg = readl(base + QUADSPI_SMPR);
> -	writel(reg & ~(QUADSPI_SMPR_FSDLY_MASK
> +	reg = qspi_readl(q, base + QUADSPI_SMPR);
> +	qspi_writel(q, reg & ~(QUADSPI_SMPR_FSDLY_MASK
>  			| QUADSPI_SMPR_FSPHS_MASK
>  			| QUADSPI_SMPR_HSENA_MASK
>  			| QUADSPI_SMPR_DDRSMP_MASK), base + QUADSPI_SMPR);
>  
>  	/* Enable the module */
> -	writel(QUADSPI_MCR_RESERVED_MASK | QUADSPI_MCR_END_CFG_MASK,
> +	qspi_writel(q, QUADSPI_MCR_RESERVED_MASK | QUADSPI_MCR_END_CFG_MASK,
>  			base + QUADSPI_MCR);
>  
>  	/* clear all interrupt status */
> -	writel(0xffffffff, q->iobase + QUADSPI_FR);
> +	qspi_writel(q, 0xffffffff, q->iobase + QUADSPI_FR);
>  
>  	/* enable the interrupt */
> -	writel(QUADSPI_RSER_TFIE, q->iobase + QUADSPI_RSER);
> +	qspi_writel(q, QUADSPI_RSER_TFIE, q->iobase + QUADSPI_RSER);
>  
>  	return 0;
>  }
> @@ -955,6 +985,7 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  	if (IS_ERR(q->iobase))
>  		return PTR_ERR(q->iobase);
>  
> +	q->big_endian = of_property_read_bool(np, "big-endian");
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>  					"QuadSPI-memory");
>  	if (!devm_request_mem_region(dev, res->start, resource_size(res),
> @@ -1103,8 +1134,8 @@ static int fsl_qspi_remove(struct platform_device *pdev)
>  	}
>  
>  	/* disable the hardware */
> -	writel(QUADSPI_MCR_MDIS_MASK, q->iobase + QUADSPI_MCR);
> -	writel(0x0, q->iobase + QUADSPI_RSER);
> +	qspi_writel(q, QUADSPI_MCR_MDIS_MASK, q->iobase + QUADSPI_MCR);
> +	qspi_writel(q, 0x0, q->iobase + QUADSPI_RSER);
>  
>  	mutex_destroy(&q->lock);
>  
> -- 
> 2.1.0.27.g96db324
> 

Re: [PATCH] mtd: spi-nor: fsl-quadspi: add big-endian support

[Fabio Estevam]

On Fri, Oct 23, 2015 at 5:53 AM, Yuan Yao <yao.yuan@freescale.com> wrote:

>  /*
> + * R/W functions for big- or little-endian registers:
> + * The qSPI controller's endian is independent of the CPU core's endian.
> + * So far, although the CPU core is little-endian but the qSPI have two
> + * versions for big-endian and little-endian.
> + */
> +static void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem *addr)
> +{
> +       if (q->big_endian)
> +               iowrite32be(val, addr);
> +       else
> +               iowrite32(val, addr);
> +}

I suggest you to implement regmap support for this driver instead.

Take a look at drivers/watchdog/imx2_wdt.c for a reference.

Then you only need to pass 'big-endian' as a property for the qspi in
the .dtsi file and regmap core will take care of endianness.

RE: [PATCH] mtd: spi-nor: fsl-quadspi: add big-endian support

[Yao Yuan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2111 bytes --]

Hi Fabio Estevam,

Thanks for your suggestion.
We have an internal discussions for that.

We think that:
According to the initial commit message of regmap, it is targeting non-memory mapped buses. (regmap: Add generic non-memory mapped register access API)  But in the imx2_wdt driver, it is used for memory-mapped register space.  So it seems that using such a complex framework just to deal with endian is an over-kill.

when it is not necessary to enable the clock every time we access the register.  
We don't think it is obvious to us how to use it for handling endianness, especially not the way imx2_wdt uses regmap.  __regmap_init_mmio_clk() calls regmap_mmio_gen_context() which errors out if reg_format_endian is not REGMAP_ENDIAN_DEFAULT or REGMAP_ENDIAN_NATIVE, and elsewhere regmap-mmio.c It seems only little-endian accessors.

Although it is possible to add the endianness support in the regmap_mmio driver, we don't see too much value in using it especially 

So we think:
static void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem
*addr) {
      if (q->big_endian)
              iowrite32be(val, addr);
      else
              iowrite32(val, addr);
}
This way is an easier, more effective solution to do the endian issue.

How about your think?

Best Regards,
Yuan Yao

On Sat, Oct 24, 2015 at 11:47 PM, Fabio Estevam <festevam@gmail.com> wrote:
> On Fri, Oct 23, 2015 at 5:53 AM, Yuan Yao <yao.yuan@freescale.com> wrote:
> 
> > +static void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem
> > +*addr) {
> > +       if (q->big_endian)
> > +               iowrite32be(val, addr);
> > +       else
> > +               iowrite32(val, addr);
> > +}
> 
> I suggest you to implement regmap support for this driver instead.
> 
> Take a look at drivers/watchdog/imx2_wdt.c for a reference.
> 
> Then you only need to pass 'big-endian' as a property for the qspi in the .dtsi
> file and regmap core will take care of endianness.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH] mtd: spi-nor: fsl-quadspi: add big-endian support

[Yao Yuan]

Hi Fabio Estevam,

Thanks for your suggestion.
We have an internal discussions for that.

We think that:
According to the initial commit message of regmap, it is targeting non-memory mapped buses. (regmap: Add generic non-memory mapped register access API)  But in the imx2_wdt driver, it is used for memory-mapped register space.  So it seems that using such a complex framework just to deal with endian is an over-kill.

when it is not necessary to enable the clock every time we access the register.  
We don't think it is obvious to us how to use it for handling endianness, especially not the way imx2_wdt uses regmap.  __regmap_init_mmio_clk() calls regmap_mmio_gen_context() which errors out if reg_format_endian is not REGMAP_ENDIAN_DEFAULT or REGMAP_ENDIAN_NATIVE, and elsewhere regmap-mmio.c It seems only little-endian accessors.

Although it is possible to add the endianness support in the regmap_mmio driver, we don't see too much value in using it especially 

So we think:
static void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem
*addr) {
      if (q->big_endian)
              iowrite32be(val, addr);
      else
              iowrite32(val, addr);
}
This way is an easier, more effective solution to do the endian issue.

How about your think?

Best Regards,
Yuan Yao

On Sat, Oct 24, 2015 at 11:47 PM, Fabio Estevam <festevam@gmail.com> wrote:
> On Fri, Oct 23, 2015 at 5:53 AM, Yuan Yao <yao.yuan@freescale.com> wrote:
> 
> > +static void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem
> > +*addr) {
> > +       if (q->big_endian)
> > +               iowrite32be(val, addr);
> > +       else
> > +               iowrite32(val, addr);
> > +}
> 
> I suggest you to implement regmap support for this driver instead.
> 
> Take a look at drivers/watchdog/imx2_wdt.c for a reference.
> 
> Then you only need to pass 'big-endian' as a property for the qspi in the .dtsi
> file and regmap core will take care of endianness.

Re: [PATCH] mtd: spi-nor: fsl-quadspi: add big-endian support

[Han Xu]

On Fri, Oct 30, 2015 at 04:49:41AM -0500, Yuan Yao-B46683 wrote:
> Hi Fabio Estevam,
> 
> Thanks for your suggestion.
> We have an internal discussions for that.
> 
> We think that:
> According to the initial commit message of regmap, it is targeting non-memory mapped buses. (regmap: Add generic non-memory mapped register access API)  But in the imx2_wdt driver, it is used for memory-mapped register space.  So it seems that using such a complex framework just to deal with endian is an over-kill.
> 
> when it is not necessary to enable the clock every time we access the register.  
> We don't think it is obvious to us how to use it for handling endianness, especially not the way imx2_wdt uses regmap.  __regmap_init_mmio_clk() calls regmap_mmio_gen_context() which errors out if reg_format_endian is not REGMAP_ENDIAN_DEFAULT or REGMAP_ENDIAN_NATIVE, and elsewhere regmap-mmio.c It seems only little-endian accessors.
> 
> Although it is possible to add the endianness support in the regmap_mmio driver, we don't see too much value in using it especially 
> 
> So we think:
> static void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem
> *addr) {
>       if (q->big_endian)
>               iowrite32be(val, addr);
>       else
>               iowrite32(val, addr);
> }
> This way is an easier, more effective solution to do the endian issue.
> 
> How about your think?

I think the implement is fine, but I prefer to use quirk rather than
read from dts? Please also rebase the patch to latest l2-mtd code.

> 
> Best Regards,
> Yuan Yao
> 
> On Sat, Oct 24, 2015 at 11:47 PM, Fabio Estevam <festevam@gmail.com> wrote:
> > On Fri, Oct 23, 2015 at 5:53 AM, Yuan Yao <yao.yuan@freescale.com> wrote:
> > 
> > > +static void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem
> > > +*addr) {
> > > +       if (q->big_endian)
> > > +               iowrite32be(val, addr);
> > > +       else
> > > +               iowrite32(val, addr);
> > > +}
> > 
> > I suggest you to implement regmap support for this driver instead.
> > 
> > Take a look at drivers/watchdog/imx2_wdt.c for a reference.
> > 
> > Then you only need to pass 'big-endian' as a property for the qspi in the .dtsi
> > file and regmap core will take care of endianness.

-- 
Best Regards,

Han "Allen" Xu

Re: [PATCH] mtd: spi-nor: fsl-quadspi: add big-endian support

[Scott Wood]

On Wed, 2015-11-11 at 11:51 -0600, Han Xu wrote:
> On Fri, Oct 30, 2015 at 04:49:41AM -0500, Yuan Yao-B46683 wrote:
> > Hi Fabio Estevam,
> > 
> > Thanks for your suggestion.
> > We have an internal discussions for that.
> > 
> > We think that:
> > According to the initial commit message of regmap, it is targeting non
> > -memory mapped buses. (regmap: Add generic non-memory mapped register
> > access API)  But in the imx2_wdt driver, it is used for memory-mapped
> > register space.  So it seems that using such a complex framework just to
> > deal with endian is an over-kill.
> > 
> > when it is not necessary to enable the clock every time we access the
> > register.  
> > We don't think it is obvious to us how to use it for handling endianness,
> > especially not the way imx2_wdt uses regmap.  __regmap_init_mmio_clk()
> > calls regmap_mmio_gen_context() which errors out if reg_format_endian is
> > not REGMAP_ENDIAN_DEFAULT or REGMAP_ENDIAN_NATIVE, and elsewhere regmap
> > -mmio.c It seems only little-endian accessors.
> > 
> > Although it is possible to add the endianness support in the regmap_mmio
> > driver, we don't see too much value in using it especially 
> > 
> > So we think:
> > static void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem
> > *addr) {
> >       if (q->big_endian)
> >               iowrite32be(val, addr);
> >       else
> >               iowrite32(val, addr);
> > }
> > This way is an easier, more effective solution to do the endian issue.
> > 
> > How about your think?
> 
> I think the implement is fine, but I prefer to use quirk rather than
> read from dts? Please also rebase the patch to latest l2-mtd code.

What specifically do you mean by "use quirk rather than read from dts"?  The
information has to come from the device tree at some point, and it can't be
inferred from the compatible string (and thus the choice of
fsl_qspi_devtype_data instance) because the variance is in how the controller
is hooked up to the SoC, not in the controller itself.

-Scott

Re: [PATCH] mtd: spi-nor: fsl-quadspi: add big-endian support

[Brian Norris]

A few corrections for the record:

On Fri, Oct 30, 2015 at 09:49:41AM +0000, Yao Yuan wrote:
> Hi Fabio Estevam,
> 
> Thanks for your suggestion.
> We have an internal discussions for that.
> 
> We think that:
> According to the initial commit message of regmap, it is targeting
> non-memory mapped buses. (regmap: Add generic non-memory mapped
> register access API)  But in the imx2_wdt driver, it is used for
> memory-mapped register space.  So it seems that using such a complex
> framework just to deal with endian is an over-kill.

It is definitely useful for non-MMIO cases, but it's certainly not
exclusive too it.

> when it is not necessary to enable the clock every time we access the register.  

You don't have to give it a clock. Just pass a NULL clk_id.

> We don't think it is obvious to us how to use it for handling
> endianness, especially not the way imx2_wdt uses regmap.
> __regmap_init_mmio_clk() calls regmap_mmio_gen_context() which errors
> out if reg_format_endian is not REGMAP_ENDIAN_DEFAULT or
> REGMAP_ENDIAN_NATIVE, and elsewhere regmap-mmio.c It seems only
> little-endian accessors.
> 
> Although it is possible to add the endianness support in the regmap_mmio driver, we don't see too much value in using it especially 

It already has DT endianness configuration support. See __regmap_init(),
which reconfigures the endianness according to regmap_get_val_endian().
So you don't need to do anything but just try it... I exepct it'll work
just fine.

> So we think:
> static void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem
> *addr) {
>       if (q->big_endian)
>               iowrite32be(val, addr);
>       else
>               iowrite32(val, addr);
> }
> This way is an easier, more effective solution to do the endian issue.
> 
> How about your think?

I think there's at least one more advantage: you get pretty good
tracing support for free. For debugging, for example, you can turn on
regmap tracing to see all the register reads and writes done in your
driver, all within the nice tracefs event infrastructure. I'm sure there
are other advantages too, but not all are applicable here.

Anyway, I do agree on the complexity argument. It's not actually that
complex to use (the imx2_wdt.c example really does show you everything
you need to know), it is a bit more complex to sort through all its
features and understand exactly what it's doing. But I'm confident it
has everything you need.

So, make your choice. I just wanted to educate on several points, so
that your decision is not just driven by lack of correct information.

For more information, a quick google search shows a few links, but no
official docs:

http://elinux.org/images/a/a3/Regmap-_The_Power_of_Subsystems_and_Abstractions.pdf
https://lwn.net/Articles/451789/

Brian

> Best Regards,
> Yuan Yao
> 
> On Sat, Oct 24, 2015 at 11:47 PM, Fabio Estevam <festevam@gmail.com> wrote:
> > On Fri, Oct 23, 2015 at 5:53 AM, Yuan Yao <yao.yuan@freescale.com> wrote:
> > 
> > > +static void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem
> > > +*addr) {
> > > +       if (q->big_endian)
> > > +               iowrite32be(val, addr);
> > > +       else
> > > +               iowrite32(val, addr);
> > > +}
> > 
> > I suggest you to implement regmap support for this driver instead.
> > 
> > Take a look at drivers/watchdog/imx2_wdt.c for a reference.
> > 
> > Then you only need to pass 'big-endian' as a property for the qspi in the .dtsi
> > file and regmap core will take care of endianness.

Re: [PATCH] mtd: spi-nor: fsl-quadspi: add big-endian support

[Brian Norris]

+ devicetree

On Wed, Nov 11, 2015 at 11:51:13AM -0600, Han Xu wrote:
> On Fri, Oct 30, 2015 at 04:49:41AM -0500, Yuan Yao-B46683 wrote:

(BTW Yuan, replying on top doesn't make the conversation as easy to
follow)

> > Although it is possible to add the endianness support in the
> > regmap_mmio driver, we don't see too much value in using it
> > especially 
> > 
> > So we think:
> > static void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem
> > *addr) {
> >       if (q->big_endian)
> >               iowrite32be(val, addr);
> >       else
> >               iowrite32(val, addr);
> > }
> > This way is an easier, more effective solution to do the endian issue.
> > 
> > How about your think?
> 
> I think the implement is fine, but I prefer to use quirk rather than
> read from dts? Please also rebase the patch to latest l2-mtd code.

If it really is just a endianness difference, then I think it makes
sense to use the existing DT bindings for it, rather than relying on a
new compatible string / quirk option. That doesn't mean you can't have a
new SoC-inspired compatible property in addition...

> > Best Regards,
> > Yuan Yao
> > 
> > On Sat, Oct 24, 2015 at 11:47 PM, Fabio Estevam <festevam@gmail.com> wrote:
> > > I suggest you to implement regmap support for this driver instead.
> > > 
> > > Take a look at drivers/watchdog/imx2_wdt.c for a reference.
> > > 
> > > Then you only need to pass 'big-endian' as a property for the qspi in the .dtsi
> > > file and regmap core will take care of endianness.

To use the standard binding also means that whether or not you choose to
use regmap right now, it's an easy option in the future, and the core
code will already handle it for you. That's really one of the main
reasons for using standardized bindings in the first place.

Brian

RE: [PATCH] mtd: spi-nor: fsl-quadspi: add big-endian support

[Yao Yuan]

On Wed, 2015-11-11 at 11:51 -0600, Han Xu wrote:
> On Fri, Oct 30, 2015 at 04:49:41AM -0500, Yuan Yao-B46683 wrote:
> > Hi Fabio Estevam,
> >
> > Thanks for your suggestion.
> > We have an internal discussions for that.
> >
> > We think that:
> > According to the initial commit message of regmap, it is targeting non-
> memory mapped buses. (regmap: Add generic non-memory mapped register
> access API)  But in the imx2_wdt driver, it is used for memory-mapped register
> space.  So it seems that using such a complex framework just to deal with
> endian is an over-kill.
> >
> > when it is not necessary to enable the clock every time we access the register.
> > We don't think it is obvious to us how to use it for handling endianness,
> especially not the way imx2_wdt uses regmap.  __regmap_init_mmio_clk()
> calls regmap_mmio_gen_context() which errors out if reg_format_endian is
> not REGMAP_ENDIAN_DEFAULT or REGMAP_ENDIAN_NATIVE, and elsewhere
> regmap-mmio.c It seems only little-endian accessors.
> >
> > Although it is possible to add the endianness support in the
> > regmap_mmio driver, we don't see too much value in using it especially
> >
> > So we think:
> > static void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem
> > *addr) {
> >       if (q->big_endian)
> >               iowrite32be(val, addr);
> >       else
> >               iowrite32(val, addr);
> > }
> > This way is an easier, more effective solution to do the endian issue.
> >
> > How about your think?
> 
> I think the implement is fine, but I prefer to use quirk rather than read from dts?
> Please also rebase the patch to latest l2-mtd code.
> 

Ok, I will rebase the patch to latest l2-mtd code in the next version.

Thanks.

RE: [PATCH] mtd: spi-nor: fsl-quadspi: add big-endian support

[Yao Yuan]

Hi Brian Norris,

Thanks for your information and the documents shared by you.
It's very helpful for me to understand the regmap.

But I think if we use:
static void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem
*addr) {
       if (q->big_endian)
               iowrite32be(val, addr);
       else
               iowrite32(val, addr);
 }
It looks like easier to read, use and change.
Is it?

And David Woodhouse, Xu Han, Mark Brown
is there any other comments from you?

Thank.

On Thu, Nov 12, 2015 3:04 AM Brian Norris wrote:
> On Fri, Oct 30, 2015 at 09:49:41AM +0000, Yao Yuan wrote:
> > Hi Fabio Estevam,
> >
> > Thanks for your suggestion.
> > We have an internal discussions for that.
> >
> > We think that:
> > According to the initial commit message of regmap, it is targeting
> > non-memory mapped buses. (regmap: Add generic non-memory mapped
> > register access API)  But in the imx2_wdt driver, it is used for
> > memory-mapped register space.  So it seems that using such a complex
> > framework just to deal with endian is an over-kill.
> 
> It is definitely useful for non-MMIO cases, but it's certainly not exclusive too it.
> 
> > when it is not necessary to enable the clock every time we access the register.
> 
> You don't have to give it a clock. Just pass a NULL clk_id.
> 
> > We don't think it is obvious to us how to use it for handling
> > endianness, especially not the way imx2_wdt uses regmap.
> > __regmap_init_mmio_clk() calls regmap_mmio_gen_context() which errors
> > out if reg_format_endian is not REGMAP_ENDIAN_DEFAULT or
> > REGMAP_ENDIAN_NATIVE, and elsewhere regmap-mmio.c It seems only
> > little-endian accessors.
> >
> > Although it is possible to add the endianness support in the
> > regmap_mmio driver, we don't see too much value in using it especially
> 
> It already has DT endianness configuration support. See __regmap_init(),
> which reconfigures the endianness according to regmap_get_val_endian().
> So you don't need to do anything but just try it... I exepct it'll work just fine.
> 
> > So we think:
> > static void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem
> > *addr) {
> >       if (q->big_endian)
> >               iowrite32be(val, addr);
> >       else
> >               iowrite32(val, addr);
> > }
> > This way is an easier, more effective solution to do the endian issue.
> >
> > How about your think?
> 
> I think there's at least one more advantage: you get pretty good tracing
> support for free. For debugging, for example, you can turn on regmap tracing
> to see all the register reads and writes done in your driver, all within the nice
> tracefs event infrastructure. I'm sure there are other advantages too, but not
> all are applicable here.
> 
> Anyway, I do agree on the complexity argument. It's not actually that complex
> to use (the imx2_wdt.c example really does show you everything you need to
> know), it is a bit more complex to sort through all its features and understand
> exactly what it's doing. But I'm confident it has everything you need.
> 
> So, make your choice. I just wanted to educate on several points, so that your
> decision is not just driven by lack of correct information.
> 
> For more information, a quick google search shows a few links, but no official
> docs:
> 
> http://elinux.org/images/a/a3/Regmap-
> _The_Power_of_Subsystems_and_Abstractions.pdf
> https://lwn.net/Articles/451789/
> 
> Brian
> 
> > Best Regards,
> > Yuan Yao
> >
> > On Sat, Oct 24, 2015 at 11:47 PM, Fabio Estevam <festevam@gmail.com>
> wrote:
> > > On Fri, Oct 23, 2015 at 5:53 AM, Yuan Yao <yao.yuan@freescale.com> wrote:
> > >
> > > > +static void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem
> > > > +*addr) {
> > > > +       if (q->big_endian)
> > > > +               iowrite32be(val, addr);
> > > > +       else
> > > > +               iowrite32(val, addr); }
> > >
> > > I suggest you to implement regmap support for this driver instead.
> > >
> > > Take a look at drivers/watchdog/imx2_wdt.c for a reference.
> > >
> > > Then you only need to pass 'big-endian' as a property for the qspi
> > > in the .dtsi file and regmap core will take care of endianness.

Re: [PATCH] mtd: spi-nor: fsl-quadspi: add big-endian support

[Brian Norris]

On Mon, Nov 16, 2015 at 04:08:39AM +0000, Yao Yuan wrote:
> It looks like easier to read, use and change.
> Is it?

It looks fine either way, IMO.

> And David Woodhouse, Xu Han, Mark Brown
> is there any other comments from you?

FYI, Mark Brown doesn't really have much to do with this. Though he
maintains the SPI subsystem (and regmap, as a matter of fact), SPI *NOR*
code is only a user of the SPI subsystem (and, potentially, regmap). I
hear he's not interested in reviewing MTD work, given the volume of
email he already receives.

Regards,
Brian

RE: [PATCH] mtd: spi-nor: fsl-quadspi: add big-endian support

[Yao Yuan]

Hi Brian,

Thanks for your information.

Best Regards,
Yuan Yao

On Mon, Nov 16, 2015 at 12:22:30PM +0000, Brian Norris wrote:
> On Mon, Nov 16, 2015 at 04:08:39AM +0000, Yao Yuan wrote:
> > It looks like easier to read, use and change.
> > Is it?
> 
> It looks fine either way, IMO.
> 
> > And David Woodhouse, Xu Han, Mark Brown is there any other comments
> > from you?
> 
> FYI, Mark Brown doesn't really have much to do with this. Though he maintains
> the SPI subsystem (and regmap, as a matter of fact), SPI *NOR* code is only a
> user of the SPI subsystem (and, potentially, regmap). I hear he's not interested
> in reviewing MTD work, given the volume of email he already receives.
> 
> Regards,
> Brian