linux-mtd.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 00/13] spi: atmel-quadspi: introduce sam9x60 qspi controller
@ 2019-02-02  4:07 Tudor.Ambarus
  2019-02-02  4:07 ` [PATCH v3 01/13] spi: atmel-quadspi: cache MR value to avoid a write access Tudor.Ambarus
                   ` (12 more replies)
  0 siblings, 13 replies; 31+ messages in thread
From: Tudor.Ambarus @ 2019-02-02  4:07 UTC (permalink / raw)
  To: broonie, robh+dt, mark.rutland, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, bbrezillon, Cyrille.Pitchen, bugalski.piotr
  Cc: devicetree, Tudor.Ambarus, linux-kernel, linux-spi, linux-mtd,
	linux-arm-kernel

From: Tudor Ambarus <tudor.ambarus@microchip.com>

Patches from 1 to 11 are minor fixes or cosmetics.
Patches 12 and 13 introduce the sam9x60 qspi controller.

sam9x60 qspi controller tested with sst26vf064b jedec,spi-nor flash.
Backward compatibility test done on sama5d2 qspi controller and
mx25l25635e jedec,spi-nor flash.

The patches are generated on top of for-next branch.

v3:
- update smm value when different.
- treat just regular spi transfers when introducing sam9x60 qspi IP.
  Mem transfers will be added together with dirmap support.
- reorganize the code and change ops functions pointers to avoid code
  duplication.
- rename aq->clk to aq->pclk to indicate that it's a peripheral clock.
- drop unused and NOP transfer macros.
- add Suggested-by tags, reword some commits.

v2:
- cache MR value,
- drop iomem wrappers,
- make "pclk" clock-name mandatory even for sama5d2,
- rework clock handling,
- reorder setting of register values in set_cfg() calls,
- collect R-b tags.

Tudor Ambarus (13):
  spi: atmel-quadspi: cache MR value to avoid a write access
  spi: atmel-quadspi: order header files inclusion alphabetically
  spi: atmel-quadspi: drop wrappers for iomem accesses
  spi: atmel-quadspi: fix naming scheme
  spi: atmel-quadspi: remove unnecessary cast
  spi: atmel-quadspi: return appropriate error code
  spi: atmel-quadspi: switch to SPDX license identifiers
  spi: atmel-quadspi: drop unused and NOP transfer macros
  dt-bindings: spi: atmel-quadspi: update example to new clock binding
  dt-bindings: spi: atmel-quadspi: make "pclk" mandatory
  spi: atmel-quadspi: add support for named peripheral clock
  dt-bindings: spi: atmel-quadspi: QuadSPI driver for Microchip SAM9X60
  spi: atmel-quadspi: add support for sam9x60 qspi controller

 .../devicetree/bindings/spi/atmel-quadspi.txt      |  12 +-
 drivers/spi/atmel-quadspi.c                        | 402 ++++++++++++++-------
 2 files changed, 288 insertions(+), 126 deletions(-)

-- 
2.9.5


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v3 01/13] spi: atmel-quadspi: cache MR value to avoid a write access
  2019-02-02  4:07 [PATCH v3 00/13] spi: atmel-quadspi: introduce sam9x60 qspi controller Tudor.Ambarus
@ 2019-02-02  4:07 ` Tudor.Ambarus
  2019-02-02  7:06   ` Boris Brezillon
  2019-02-02  4:07 ` [PATCH v3 02/13] spi: atmel-quadspi: order header files inclusion alphabetically Tudor.Ambarus
                   ` (11 subsequent siblings)
  12 siblings, 1 reply; 31+ messages in thread
From: Tudor.Ambarus @ 2019-02-02  4:07 UTC (permalink / raw)
  To: broonie, robh+dt, mark.rutland, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, bbrezillon, Cyrille.Pitchen, bugalski.piotr
  Cc: devicetree, Tudor.Ambarus, linux-kernel, linux-spi, linux-mtd,
	linux-arm-kernel

From: Tudor Ambarus <tudor.ambarus@microchip.com>

Cache Serial Memory Mode (SMM) value to avoid write access when
setting the controller in serial memory mode. SMM is set in
exec_op() and not at probe time, to let room for future regular
SPI support.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
v3: update smm value when different. rename mr/smm
v2: cache MR value instead of moving the write access at probe

 drivers/spi/atmel-quadspi.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
index ddc712410812..645284c6ec9a 100644
--- a/drivers/spi/atmel-quadspi.c
+++ b/drivers/spi/atmel-quadspi.c
@@ -155,6 +155,7 @@ struct atmel_qspi {
 	struct clk		*clk;
 	struct platform_device	*pdev;
 	u32			pending;
+	u32			smm;
 	struct completion	cmd_completion;
 };
 
@@ -238,7 +239,11 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 	icr = QSPI_ICR_INST(op->cmd.opcode);
 	ifr = QSPI_IFR_INSTEN;
 
-	qspi_writel(aq, QSPI_MR, QSPI_MR_SMM);
+	/* Set the QSPI controller in Serial Memory Mode */
+	if (aq->smm != QSPI_MR_SMM) {
+		qspi_writel(aq, QSPI_MR, QSPI_MR_SMM);
+		aq->smm = QSPI_MR_SMM;
+	}
 
 	mode = find_mode(op);
 	if (mode < 0)
-- 
2.9.5


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 02/13] spi: atmel-quadspi: order header files inclusion alphabetically
  2019-02-02  4:07 [PATCH v3 00/13] spi: atmel-quadspi: introduce sam9x60 qspi controller Tudor.Ambarus
  2019-02-02  4:07 ` [PATCH v3 01/13] spi: atmel-quadspi: cache MR value to avoid a write access Tudor.Ambarus
@ 2019-02-02  4:07 ` Tudor.Ambarus
  2019-02-02  4:07 ` [PATCH v3 03/13] spi: atmel-quadspi: drop wrappers for iomem accesses Tudor.Ambarus
                   ` (10 subsequent siblings)
  12 siblings, 0 replies; 31+ messages in thread
From: Tudor.Ambarus @ 2019-02-02  4:07 UTC (permalink / raw)
  To: broonie, robh+dt, mark.rutland, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, bbrezillon, Cyrille.Pitchen, bugalski.piotr
  Cc: devicetree, Tudor.Ambarus, linux-kernel, linux-spi, linux-mtd,
	linux-arm-kernel

From: Tudor Ambarus <tudor.ambarus@microchip.com>

Cosmetic change, no functional change.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
Reviewed-by: Boris Brezillon <bbrezillon@kernel.org>
---
v3: no change
v2: collect R-b

 drivers/spi/atmel-quadspi.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
index 645284c6ec9a..feeddcb25e1f 100644
--- a/drivers/spi/atmel-quadspi.c
+++ b/drivers/spi/atmel-quadspi.c
@@ -22,16 +22,15 @@
  * This driver is based on drivers/mtd/spi-nor/fsl-quadspi.c from Freescale.
  */
 
-#include <linux/kernel.h>
 #include <linux/clk.h>
-#include <linux/module.h>
-#include <linux/platform_device.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
-#include <linux/of.h>
-
 #include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
 #include <linux/spi/spi-mem.h>
 
 /* QSPI register offsets */
-- 
2.9.5


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 03/13] spi: atmel-quadspi: drop wrappers for iomem accesses
  2019-02-02  4:07 [PATCH v3 00/13] spi: atmel-quadspi: introduce sam9x60 qspi controller Tudor.Ambarus
  2019-02-02  4:07 ` [PATCH v3 01/13] spi: atmel-quadspi: cache MR value to avoid a write access Tudor.Ambarus
  2019-02-02  4:07 ` [PATCH v3 02/13] spi: atmel-quadspi: order header files inclusion alphabetically Tudor.Ambarus
@ 2019-02-02  4:07 ` Tudor.Ambarus
  2019-02-02  7:11   ` Boris Brezillon
  2019-02-02  4:07 ` [PATCH v3 04/13] spi: atmel-quadspi: fix naming scheme Tudor.Ambarus
                   ` (9 subsequent siblings)
  12 siblings, 1 reply; 31+ messages in thread
From: Tudor.Ambarus @ 2019-02-02  4:07 UTC (permalink / raw)
  To: broonie, robh+dt, mark.rutland, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, bbrezillon, Cyrille.Pitchen, bugalski.piotr
  Cc: devicetree, Tudor.Ambarus, linux-kernel, linux-spi, linux-mtd,
	linux-arm-kernel

From: Tudor Ambarus <tudor.ambarus@microchip.com>

The wrappers hid that the accesses are relaxed. Drop them.

Suggested-by: Boris Brezillon <bbrezillon@kernel.org>
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
v3: no change
v2: new patch

 drivers/spi/atmel-quadspi.c | 47 +++++++++++++++++++--------------------------
 1 file changed, 20 insertions(+), 27 deletions(-)

diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
index feeddcb25e1f..131374db0db4 100644
--- a/drivers/spi/atmel-quadspi.c
+++ b/drivers/spi/atmel-quadspi.c
@@ -175,17 +175,6 @@ static const struct qspi_mode sama5d2_qspi_modes[] = {
 	{ 4, 4, 4, QSPI_IFR_WIDTH_QUAD_CMD },
 };
 
-/* Register access functions */
-static inline u32 qspi_readl(struct atmel_qspi *aq, u32 reg)
-{
-	return readl_relaxed(aq->regs + reg);
-}
-
-static inline void qspi_writel(struct atmel_qspi *aq, u32 reg, u32 value)
-{
-	writel_relaxed(value, aq->regs + reg);
-}
-
 static inline bool is_compatible(const struct spi_mem_op *op,
 				 const struct qspi_mode *mode)
 {
@@ -229,6 +218,7 @@ static bool atmel_qspi_supports_op(struct spi_mem *mem,
 static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 {
 	struct atmel_qspi *aq = spi_controller_get_devdata(mem->spi->master);
+	void __iomem *base = aq->regs;
 	int mode;
 	u32 dummy_cycles = 0;
 	u32 iar, icr, ifr, sr;
@@ -240,7 +230,7 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 
 	/* Set the QSPI controller in Serial Memory Mode */
 	if (aq->smm != QSPI_MR_SMM) {
-		qspi_writel(aq, QSPI_MR, QSPI_MR_SMM);
+		writel_relaxed(QSPI_MR_SMM, base + QSPI_MR);
 		aq->smm = QSPI_MR_SMM;
 	}
 
@@ -300,17 +290,17 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 		ifr |= QSPI_IFR_TFRTYP_TRSFR_WRITE;
 
 	/* Clear pending interrupts */
-	(void)qspi_readl(aq, QSPI_SR);
+	(void)readl_relaxed(base + QSPI_SR);
 
 	/* Set QSPI Instruction Frame registers */
-	qspi_writel(aq, QSPI_IAR, iar);
-	qspi_writel(aq, QSPI_ICR, icr);
-	qspi_writel(aq, QSPI_IFR, ifr);
+	writel_relaxed(iar, base + QSPI_IAR);
+	writel_relaxed(icr, base + QSPI_ICR);
+	writel_relaxed(ifr, base + QSPI_IFR);
 
 	/* Skip to the final steps if there is no data */
 	if (op->data.nbytes) {
 		/* Dummy read of QSPI_IFR to synchronize APB and AHB accesses */
-		(void)qspi_readl(aq, QSPI_IFR);
+		(void)readl_relaxed(base + QSPI_IFR);
 
 		/* Send/Receive data */
 		if (op->data.dir == SPI_MEM_DATA_IN)
@@ -321,22 +311,22 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 				op->data.buf.out, op->data.nbytes);
 
 		/* Release the chip-select */
-		qspi_writel(aq, QSPI_CR, QSPI_CR_LASTXFER);
+		writel_relaxed(QSPI_CR_LASTXFER, base + QSPI_CR);
 	}
 
 	/* Poll INSTRuction End status */
-	sr = qspi_readl(aq, QSPI_SR);
+	sr = readl_relaxed(base + QSPI_SR);
 	if ((sr & QSPI_SR_CMD_COMPLETED) == QSPI_SR_CMD_COMPLETED)
 		return err;
 
 	/* Wait for INSTRuction End interrupt */
 	reinit_completion(&aq->cmd_completion);
 	aq->pending = sr & QSPI_SR_CMD_COMPLETED;
-	qspi_writel(aq, QSPI_IER, QSPI_SR_CMD_COMPLETED);
+	writel_relaxed(QSPI_SR_CMD_COMPLETED, base + QSPI_IER);
 	if (!wait_for_completion_timeout(&aq->cmd_completion,
 					 msecs_to_jiffies(1000)))
 		err = -ETIMEDOUT;
-	qspi_writel(aq, QSPI_IDR, QSPI_SR_CMD_COMPLETED);
+	writel_relaxed(QSPI_SR_CMD_COMPLETED, base + QSPI_IDR);
 
 	return err;
 }
@@ -375,18 +365,20 @@ static int atmel_qspi_setup(struct spi_device *spi)
 		scbr--;
 
 	scr = QSPI_SCR_SCBR(scbr);
-	qspi_writel(aq, QSPI_SCR, scr);
+	writel_relaxed(scr, aq->regs + QSPI_SCR);
 
 	return 0;
 }
 
 static int atmel_qspi_init(struct atmel_qspi *aq)
 {
+	void __iomem *base = aq->regs;
+
 	/* Reset the QSPI controller */
-	qspi_writel(aq, QSPI_CR, QSPI_CR_SWRST);
+	writel_relaxed(QSPI_CR_SWRST, base + QSPI_CR);
 
 	/* Enable the QSPI controller */
-	qspi_writel(aq, QSPI_CR, QSPI_CR_QSPIEN);
+	writel_relaxed(QSPI_CR_QSPIEN, base + QSPI_CR);
 
 	return 0;
 }
@@ -394,10 +386,11 @@ static int atmel_qspi_init(struct atmel_qspi *aq)
 static irqreturn_t atmel_qspi_interrupt(int irq, void *dev_id)
 {
 	struct atmel_qspi *aq = (struct atmel_qspi *)dev_id;
+	void __iomem *base = aq->regs;
 	u32 status, mask, pending;
 
-	status = qspi_readl(aq, QSPI_SR);
-	mask = qspi_readl(aq, QSPI_IMR);
+	status = readl_relaxed(base + QSPI_SR);
+	mask = readl_relaxed(base + QSPI_IMR);
 	pending = status & mask;
 
 	if (!pending)
@@ -503,7 +496,7 @@ static int atmel_qspi_remove(struct platform_device *pdev)
 	struct atmel_qspi *aq = spi_controller_get_devdata(ctrl);
 
 	spi_unregister_controller(ctrl);
-	qspi_writel(aq, QSPI_CR, QSPI_CR_QSPIDIS);
+	writel_relaxed(QSPI_CR_QSPIDIS, aq->regs + QSPI_CR);
 	clk_disable_unprepare(aq->clk);
 	return 0;
 }
-- 
2.9.5


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 04/13] spi: atmel-quadspi: fix naming scheme
  2019-02-02  4:07 [PATCH v3 00/13] spi: atmel-quadspi: introduce sam9x60 qspi controller Tudor.Ambarus
                   ` (2 preceding siblings ...)
  2019-02-02  4:07 ` [PATCH v3 03/13] spi: atmel-quadspi: drop wrappers for iomem accesses Tudor.Ambarus
@ 2019-02-02  4:07 ` Tudor.Ambarus
  2019-02-02  7:12   ` Boris Brezillon
  2019-02-02  4:07 ` [PATCH v3 05/13] spi: atmel-quadspi: remove unnecessary cast Tudor.Ambarus
                   ` (8 subsequent siblings)
  12 siblings, 1 reply; 31+ messages in thread
From: Tudor.Ambarus @ 2019-02-02  4:07 UTC (permalink / raw)
  To: broonie, robh+dt, mark.rutland, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, bbrezillon, Cyrille.Pitchen, bugalski.piotr
  Cc: devicetree, Tudor.Ambarus, linux-kernel, linux-spi, linux-mtd,
	linux-arm-kernel

From: Tudor Ambarus <tudor.ambarus@microchip.com>

Let general names to core drivers.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
v3: no change
v2: update after the removing of iomem access wrappers

 drivers/spi/atmel-quadspi.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
index 131374db0db4..47ed751a91e7 100644
--- a/drivers/spi/atmel-quadspi.c
+++ b/drivers/spi/atmel-quadspi.c
@@ -158,14 +158,14 @@ struct atmel_qspi {
 	struct completion	cmd_completion;
 };
 
-struct qspi_mode {
+struct atmel_qspi_mode {
 	u8 cmd_buswidth;
 	u8 addr_buswidth;
 	u8 data_buswidth;
 	u32 config;
 };
 
-static const struct qspi_mode sama5d2_qspi_modes[] = {
+static const struct atmel_qspi_mode sama5d2_qspi_modes[] = {
 	{ 1, 1, 1, QSPI_IFR_WIDTH_SINGLE_BIT_SPI },
 	{ 1, 1, 2, QSPI_IFR_WIDTH_DUAL_OUTPUT },
 	{ 1, 1, 4, QSPI_IFR_WIDTH_QUAD_OUTPUT },
@@ -175,8 +175,8 @@ static const struct qspi_mode sama5d2_qspi_modes[] = {
 	{ 4, 4, 4, QSPI_IFR_WIDTH_QUAD_CMD },
 };
 
-static inline bool is_compatible(const struct spi_mem_op *op,
-				 const struct qspi_mode *mode)
+static inline bool atmel_qspi_is_compatible(const struct spi_mem_op *op,
+					    const struct atmel_qspi_mode *mode)
 {
 	if (op->cmd.buswidth != mode->cmd_buswidth)
 		return false;
@@ -190,12 +190,12 @@ static inline bool is_compatible(const struct spi_mem_op *op,
 	return true;
 }
 
-static int find_mode(const struct spi_mem_op *op)
+static int atmel_qspi_find_mode(const struct spi_mem_op *op)
 {
 	u32 i;
 
 	for (i = 0; i < ARRAY_SIZE(sama5d2_qspi_modes); i++)
-		if (is_compatible(op, &sama5d2_qspi_modes[i]))
+		if (atmel_qspi_is_compatible(op, &sama5d2_qspi_modes[i]))
 			return i;
 
 	return -1;
@@ -204,7 +204,7 @@ static int find_mode(const struct spi_mem_op *op)
 static bool atmel_qspi_supports_op(struct spi_mem *mem,
 				   const struct spi_mem_op *op)
 {
-	if (find_mode(op) < 0)
+	if (atmel_qspi_find_mode(op) < 0)
 		return false;
 
 	/* special case not supported by hardware */
@@ -234,7 +234,7 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 		aq->smm = QSPI_MR_SMM;
 	}
 
-	mode = find_mode(op);
+	mode = atmel_qspi_find_mode(op);
 	if (mode < 0)
 		return -ENOTSUPP;
 
-- 
2.9.5


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 05/13] spi: atmel-quadspi: remove unnecessary cast
  2019-02-02  4:07 [PATCH v3 00/13] spi: atmel-quadspi: introduce sam9x60 qspi controller Tudor.Ambarus
                   ` (3 preceding siblings ...)
  2019-02-02  4:07 ` [PATCH v3 04/13] spi: atmel-quadspi: fix naming scheme Tudor.Ambarus
@ 2019-02-02  4:07 ` Tudor.Ambarus
  2019-02-02  4:07 ` [PATCH v3 06/13] spi: atmel-quadspi: return appropriate error code Tudor.Ambarus
                   ` (7 subsequent siblings)
  12 siblings, 0 replies; 31+ messages in thread
From: Tudor.Ambarus @ 2019-02-02  4:07 UTC (permalink / raw)
  To: broonie, robh+dt, mark.rutland, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, bbrezillon, Cyrille.Pitchen, bugalski.piotr
  Cc: devicetree, Tudor.Ambarus, linux-kernel, linux-spi, linux-mtd,
	linux-arm-kernel

From: Tudor Ambarus <tudor.ambarus@microchip.com>

The cast is done implicitly.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
Reviewed-by: Boris Brezillon <bbrezillon@kernel.org>
---
v3: no change
v2: collect R-b

 drivers/spi/atmel-quadspi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
index 47ed751a91e7..dbc2c65abc86 100644
--- a/drivers/spi/atmel-quadspi.c
+++ b/drivers/spi/atmel-quadspi.c
@@ -385,7 +385,7 @@ static int atmel_qspi_init(struct atmel_qspi *aq)
 
 static irqreturn_t atmel_qspi_interrupt(int irq, void *dev_id)
 {
-	struct atmel_qspi *aq = (struct atmel_qspi *)dev_id;
+	struct atmel_qspi *aq = dev_id;
 	void __iomem *base = aq->regs;
 	u32 status, mask, pending;
 
-- 
2.9.5


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 06/13] spi: atmel-quadspi: return appropriate error code
  2019-02-02  4:07 [PATCH v3 00/13] spi: atmel-quadspi: introduce sam9x60 qspi controller Tudor.Ambarus
                   ` (4 preceding siblings ...)
  2019-02-02  4:07 ` [PATCH v3 05/13] spi: atmel-quadspi: remove unnecessary cast Tudor.Ambarus
@ 2019-02-02  4:07 ` Tudor.Ambarus
  2019-02-02  4:07 ` [PATCH v3 07/13] spi: atmel-quadspi: switch to SPDX license identifiers Tudor.Ambarus
                   ` (6 subsequent siblings)
  12 siblings, 0 replies; 31+ messages in thread
From: Tudor.Ambarus @ 2019-02-02  4:07 UTC (permalink / raw)
  To: broonie, robh+dt, mark.rutland, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, bbrezillon, Cyrille.Pitchen, bugalski.piotr
  Cc: devicetree, Tudor.Ambarus, linux-kernel, linux-spi, linux-mtd,
	linux-arm-kernel

From: Tudor Ambarus <tudor.ambarus@microchip.com>

Return -ENOTSUPP when atmel_qspi_find_mode() fails. Propagate
the error in atmel_qspi_exec_op().

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
Reviewed-by: Boris Brezillon <bbrezillon@kernel.org>
---
v3: no change
v2: collect R-b

 drivers/spi/atmel-quadspi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
index dbc2c65abc86..80b77eb4ec5e 100644
--- a/drivers/spi/atmel-quadspi.c
+++ b/drivers/spi/atmel-quadspi.c
@@ -198,7 +198,7 @@ static int atmel_qspi_find_mode(const struct spi_mem_op *op)
 		if (atmel_qspi_is_compatible(op, &sama5d2_qspi_modes[i]))
 			return i;
 
-	return -1;
+	return -ENOTSUPP;
 }
 
 static bool atmel_qspi_supports_op(struct spi_mem *mem,
@@ -236,7 +236,7 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 
 	mode = atmel_qspi_find_mode(op);
 	if (mode < 0)
-		return -ENOTSUPP;
+		return mode;
 
 	ifr |= sama5d2_qspi_modes[mode].config;
 
-- 
2.9.5


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 07/13] spi: atmel-quadspi: switch to SPDX license identifiers
  2019-02-02  4:07 [PATCH v3 00/13] spi: atmel-quadspi: introduce sam9x60 qspi controller Tudor.Ambarus
                   ` (5 preceding siblings ...)
  2019-02-02  4:07 ` [PATCH v3 06/13] spi: atmel-quadspi: return appropriate error code Tudor.Ambarus
@ 2019-02-02  4:07 ` Tudor.Ambarus
  2019-02-02  4:07 ` [PATCH v3 08/13] spi: atmel-quadspi: drop unused and NOP transfer macros Tudor.Ambarus
                   ` (5 subsequent siblings)
  12 siblings, 0 replies; 31+ messages in thread
From: Tudor.Ambarus @ 2019-02-02  4:07 UTC (permalink / raw)
  To: broonie, robh+dt, mark.rutland, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, bbrezillon, Cyrille.Pitchen, bugalski.piotr
  Cc: devicetree, Tudor.Ambarus, linux-kernel, linux-spi, linux-mtd,
	linux-arm-kernel

From: Tudor Ambarus <tudor.ambarus@microchip.com>

Adopt the SPDX license identifiers to ease license compliance
management.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
Reviewed-by: Boris Brezillon <bbrezillon@kernel.org>
---
v3: no change
v2: collect R-b

 drivers/spi/atmel-quadspi.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
index 80b77eb4ec5e..d26d4cd0e36b 100644
--- a/drivers/spi/atmel-quadspi.c
+++ b/drivers/spi/atmel-quadspi.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Driver for Atmel QSPI Controller
  *
@@ -7,18 +8,6 @@
  * Author: Cyrille Pitchen <cyrille.pitchen@atmel.com>
  * Author: Piotr Bugalski <bugalski.piotr@gmail.com>
  *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program.  If not, see <http://www.gnu.org/licenses/>.
- *
  * This driver is based on drivers/mtd/spi-nor/fsl-quadspi.c from Freescale.
  */
 
-- 
2.9.5


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 08/13] spi: atmel-quadspi: drop unused and NOP transfer macros
  2019-02-02  4:07 [PATCH v3 00/13] spi: atmel-quadspi: introduce sam9x60 qspi controller Tudor.Ambarus
                   ` (6 preceding siblings ...)
  2019-02-02  4:07 ` [PATCH v3 07/13] spi: atmel-quadspi: switch to SPDX license identifiers Tudor.Ambarus
@ 2019-02-02  4:07 ` Tudor.Ambarus
  2019-02-02  7:13   ` Boris Brezillon
  2019-02-02  4:07 ` [PATCH v3 09/13] dt-bindings: spi: atmel-quadspi: update example to new clock binding Tudor.Ambarus
                   ` (4 subsequent siblings)
  12 siblings, 1 reply; 31+ messages in thread
From: Tudor.Ambarus @ 2019-02-02  4:07 UTC (permalink / raw)
  To: broonie, robh+dt, mark.rutland, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, bbrezillon, Cyrille.Pitchen, bugalski.piotr
  Cc: devicetree, Tudor.Ambarus, linux-kernel, linux-spi, linux-mtd,
	linux-arm-kernel

From: Tudor Ambarus <tudor.ambarus@microchip.com>

Remove NOP when setting read transfer type. Remove useless
setting of write transfer type when
op->data.dir == SPI_MEM_DATA_IN && !op->data.nbytes.

QSPI_IFR_TFRTYP_TRSFR_WRITE is specific just to sama5d2 qspi,
rename it to QSPI_IFR_SAMA5D2_WRITE_TRSFR.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
v3: new patch

 drivers/spi/atmel-quadspi.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
index d26d4cd0e36b..bfa5f5e92d96 100644
--- a/drivers/spi/atmel-quadspi.c
+++ b/drivers/spi/atmel-quadspi.c
@@ -113,11 +113,7 @@
 #define QSPI_IFR_OPTL_4BIT              (2 << 8)
 #define QSPI_IFR_OPTL_8BIT              (3 << 8)
 #define QSPI_IFR_ADDRL                  BIT(10)
-#define QSPI_IFR_TFRTYP_MASK            GENMASK(13, 12)
-#define QSPI_IFR_TFRTYP_TRSFR_READ      (0 << 12)
-#define QSPI_IFR_TFRTYP_TRSFR_READ_MEM  (1 << 12)
-#define QSPI_IFR_TFRTYP_TRSFR_WRITE     (2 << 12)
-#define QSPI_IFR_TFRTYP_TRSFR_WRITE_MEM (3 << 13)
+#define QSPI_IFR_SAMA5D2_WRITE_TRSFR	BIT(13)
 #define QSPI_IFR_CRM                    BIT(14)
 #define QSPI_IFR_NBDUM_MASK             GENMASK(20, 16)
 #define QSPI_IFR_NBDUM(n)               (((n) << 16) & QSPI_IFR_NBDUM_MASK)
@@ -273,10 +269,8 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 	if (op->data.nbytes)
 		ifr |= QSPI_IFR_DATAEN;
 
-	if (op->data.dir == SPI_MEM_DATA_IN && op->data.nbytes)
-		ifr |= QSPI_IFR_TFRTYP_TRSFR_READ;
-	else
-		ifr |= QSPI_IFR_TFRTYP_TRSFR_WRITE;
+	if (op->data.dir == SPI_MEM_DATA_OUT)
+		ifr |= QSPI_IFR_SAMA5D2_WRITE_TRSFR;
 
 	/* Clear pending interrupts */
 	(void)readl_relaxed(base + QSPI_SR);
-- 
2.9.5


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 09/13] dt-bindings: spi: atmel-quadspi: update example to new clock binding
  2019-02-02  4:07 [PATCH v3 00/13] spi: atmel-quadspi: introduce sam9x60 qspi controller Tudor.Ambarus
                   ` (7 preceding siblings ...)
  2019-02-02  4:07 ` [PATCH v3 08/13] spi: atmel-quadspi: drop unused and NOP transfer macros Tudor.Ambarus
@ 2019-02-02  4:07 ` Tudor.Ambarus
  2019-02-02  4:07 ` [PATCH v3 10/13] dt-bindings: spi: atmel-quadspi: make "pclk" mandatory Tudor.Ambarus
                   ` (3 subsequent siblings)
  12 siblings, 0 replies; 31+ messages in thread
From: Tudor.Ambarus @ 2019-02-02  4:07 UTC (permalink / raw)
  To: broonie, robh+dt, mark.rutland, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, bbrezillon, Cyrille.Pitchen, bugalski.piotr
  Cc: devicetree, Tudor.Ambarus, linux-kernel, linux-spi, linux-mtd,
	linux-arm-kernel

From: Tudor Ambarus <tudor.ambarus@microchip.com>

Introduced in:
commit b60557876849 ("ARM: dts: at91: sama5d2: switch to new clock binding")

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
Reviewed-by: Boris Brezillon <bbrezillon@kernel.org>
---
v3: no change
v2: collect R-b

 Documentation/devicetree/bindings/spi/atmel-quadspi.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/spi/atmel-quadspi.txt b/Documentation/devicetree/bindings/spi/atmel-quadspi.txt
index b93c1e2f25dd..e9dae6264d89 100644
--- a/Documentation/devicetree/bindings/spi/atmel-quadspi.txt
+++ b/Documentation/devicetree/bindings/spi/atmel-quadspi.txt
@@ -19,7 +19,7 @@ spi@f0020000 {
 	reg = <0xf0020000 0x100>, <0xd0000000 0x8000000>;
 	reg-names = "qspi_base", "qspi_mmap";
 	interrupts = <52 IRQ_TYPE_LEVEL_HIGH 7>;
-	clocks = <&spi0_clk>;
+	clocks = <&pmc PMC_TYPE_PERIPHERAL 52>;
 	#address-cells = <1>;
 	#size-cells = <0>;
 	pinctrl-names = "default";
-- 
2.9.5


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 10/13] dt-bindings: spi: atmel-quadspi: make "pclk" mandatory
  2019-02-02  4:07 [PATCH v3 00/13] spi: atmel-quadspi: introduce sam9x60 qspi controller Tudor.Ambarus
                   ` (8 preceding siblings ...)
  2019-02-02  4:07 ` [PATCH v3 09/13] dt-bindings: spi: atmel-quadspi: update example to new clock binding Tudor.Ambarus
@ 2019-02-02  4:07 ` Tudor.Ambarus
  2019-02-02  7:15   ` Boris Brezillon
  2019-02-02  4:07 ` [PATCH v3 11/13] spi: atmel-quadspi: add support for named peripheral clock Tudor.Ambarus
                   ` (2 subsequent siblings)
  12 siblings, 1 reply; 31+ messages in thread
From: Tudor.Ambarus @ 2019-02-02  4:07 UTC (permalink / raw)
  To: broonie, robh+dt, mark.rutland, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, bbrezillon, Cyrille.Pitchen, bugalski.piotr
  Cc: devicetree, Tudor.Ambarus, linux-kernel, linux-spi, linux-mtd,
	linux-arm-kernel

From: Tudor Ambarus <tudor.ambarus@microchip.com>

Naming clocks is a good practice. Make "pclk" madatory even if
we support unnamed clock in the driver, to be backward compatible
with old DTs.

Suggested-by: Boris Brezillon <bbrezillon@kernel.org>
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
v3: new patch

 Documentation/devicetree/bindings/spi/atmel-quadspi.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/atmel-quadspi.txt b/Documentation/devicetree/bindings/spi/atmel-quadspi.txt
index e9dae6264d89..1de54e87f5d6 100644
--- a/Documentation/devicetree/bindings/spi/atmel-quadspi.txt
+++ b/Documentation/devicetree/bindings/spi/atmel-quadspi.txt
@@ -9,6 +9,7 @@ Required properties:
                   - qspi_mmap: memory mapped address space
 - interrupts:     Should contain the interrupt for the device.
 - clocks:         The phandle of the clock needed by the QSPI controller.
+- clock-names:    Should contain "pclk" for the peripheral clock.
 - #address-cells: Should be <1>.
 - #size-cells:    Should be <0>.
 
@@ -20,6 +21,7 @@ spi@f0020000 {
 	reg-names = "qspi_base", "qspi_mmap";
 	interrupts = <52 IRQ_TYPE_LEVEL_HIGH 7>;
 	clocks = <&pmc PMC_TYPE_PERIPHERAL 52>;
+	clock-names = "pclk"
 	#address-cells = <1>;
 	#size-cells = <0>;
 	pinctrl-names = "default";
-- 
2.9.5


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 11/13] spi: atmel-quadspi: add support for named peripheral clock
  2019-02-02  4:07 [PATCH v3 00/13] spi: atmel-quadspi: introduce sam9x60 qspi controller Tudor.Ambarus
                   ` (9 preceding siblings ...)
  2019-02-02  4:07 ` [PATCH v3 10/13] dt-bindings: spi: atmel-quadspi: make "pclk" mandatory Tudor.Ambarus
@ 2019-02-02  4:07 ` Tudor.Ambarus
  2019-02-02  7:16   ` Boris Brezillon
  2019-02-02  4:07 ` [PATCH v3 12/13] dt-bindings: spi: atmel-quadspi: QuadSPI driver for Microchip SAM9X60 Tudor.Ambarus
  2019-02-02  4:07 ` [PATCH v3 13/13] spi: atmel-quadspi: add support for sam9x60 qspi controller Tudor.Ambarus
  12 siblings, 1 reply; 31+ messages in thread
From: Tudor.Ambarus @ 2019-02-02  4:07 UTC (permalink / raw)
  To: broonie, robh+dt, mark.rutland, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, bbrezillon, Cyrille.Pitchen, bugalski.piotr
  Cc: devicetree, Tudor.Ambarus, linux-kernel, linux-spi, linux-mtd,
	linux-arm-kernel

From: Tudor Ambarus <tudor.ambarus@microchip.com>

Naming clocks is a good practice. Keep supporting unnamed
peripheral clock, to be backward compatible with old DTs.
While here, rename clk to pclk, to indicate that it is a
peripheral clock.

Suggested-by: Boris Brezillon <bbrezillon@kernel.org>
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
v3: new patch

 drivers/spi/atmel-quadspi.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
index bfa5f5e92d96..c9548942535a 100644
--- a/drivers/spi/atmel-quadspi.c
+++ b/drivers/spi/atmel-quadspi.c
@@ -136,7 +136,7 @@
 struct atmel_qspi {
 	void __iomem		*regs;
 	void __iomem		*mem;
-	struct clk		*clk;
+	struct clk		*pclk;
 	struct platform_device	*pdev;
 	u32			pending;
 	u32			smm;
@@ -338,7 +338,7 @@ static int atmel_qspi_setup(struct spi_device *spi)
 	if (!spi->max_speed_hz)
 		return -EINVAL;
 
-	src_rate = clk_get_rate(aq->clk);
+	src_rate = clk_get_rate(aq->pclk);
 	if (!src_rate)
 		return -EINVAL;
 
@@ -429,15 +429,18 @@ static int atmel_qspi_probe(struct platform_device *pdev)
 	}
 
 	/* Get the peripheral clock */
-	aq->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(aq->clk)) {
+	aq->pclk = devm_clk_get(&pdev->dev, "pclk");
+	if (IS_ERR(aq->pclk))
+		aq->pclk = devm_clk_get(&pdev->dev, NULL);
+
+	if (IS_ERR(aq->pclk)) {
 		dev_err(&pdev->dev, "missing peripheral clock\n");
-		err = PTR_ERR(aq->clk);
+		err = PTR_ERR(aq->pclk);
 		goto exit;
 	}
 
 	/* Enable the peripheral clock */
-	err = clk_prepare_enable(aq->clk);
+	err = clk_prepare_enable(aq->pclk);
 	if (err) {
 		dev_err(&pdev->dev, "failed to enable the peripheral clock\n");
 		goto exit;
@@ -448,25 +451,25 @@ static int atmel_qspi_probe(struct platform_device *pdev)
 	if (irq < 0) {
 		dev_err(&pdev->dev, "missing IRQ\n");
 		err = irq;
-		goto disable_clk;
+		goto disable_pclk;
 	}
 	err = devm_request_irq(&pdev->dev, irq, atmel_qspi_interrupt,
 			       0, dev_name(&pdev->dev), aq);
 	if (err)
-		goto disable_clk;
+		goto disable_pclk;
 
 	err = atmel_qspi_init(aq);
 	if (err)
-		goto disable_clk;
+		goto disable_pclk;
 
 	err = spi_register_controller(ctrl);
 	if (err)
-		goto disable_clk;
+		goto disable_pclk;
 
 	return 0;
 
-disable_clk:
-	clk_disable_unprepare(aq->clk);
+disable_pclk:
+	clk_disable_unprepare(aq->pclk);
 exit:
 	spi_controller_put(ctrl);
 
@@ -480,7 +483,7 @@ static int atmel_qspi_remove(struct platform_device *pdev)
 
 	spi_unregister_controller(ctrl);
 	writel_relaxed(QSPI_CR_QSPIDIS, aq->regs + QSPI_CR);
-	clk_disable_unprepare(aq->clk);
+	clk_disable_unprepare(aq->pclk);
 	return 0;
 }
 
@@ -488,7 +491,7 @@ static int __maybe_unused atmel_qspi_suspend(struct device *dev)
 {
 	struct atmel_qspi *aq = dev_get_drvdata(dev);
 
-	clk_disable_unprepare(aq->clk);
+	clk_disable_unprepare(aq->pclk);
 
 	return 0;
 }
@@ -497,7 +500,7 @@ static int __maybe_unused atmel_qspi_resume(struct device *dev)
 {
 	struct atmel_qspi *aq = dev_get_drvdata(dev);
 
-	clk_prepare_enable(aq->clk);
+	clk_prepare_enable(aq->pclk);
 
 	return atmel_qspi_init(aq);
 }
-- 
2.9.5


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 12/13] dt-bindings: spi: atmel-quadspi: QuadSPI driver for Microchip SAM9X60
  2019-02-02  4:07 [PATCH v3 00/13] spi: atmel-quadspi: introduce sam9x60 qspi controller Tudor.Ambarus
                   ` (10 preceding siblings ...)
  2019-02-02  4:07 ` [PATCH v3 11/13] spi: atmel-quadspi: add support for named peripheral clock Tudor.Ambarus
@ 2019-02-02  4:07 ` Tudor.Ambarus
  2019-02-02  7:17   ` Boris Brezillon
  2019-02-02  4:07 ` [PATCH v3 13/13] spi: atmel-quadspi: add support for sam9x60 qspi controller Tudor.Ambarus
  12 siblings, 1 reply; 31+ messages in thread
From: Tudor.Ambarus @ 2019-02-02  4:07 UTC (permalink / raw)
  To: broonie, robh+dt, mark.rutland, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, bbrezillon, Cyrille.Pitchen, bugalski.piotr
  Cc: devicetree, Tudor.Ambarus, linux-kernel, linux-spi, linux-mtd,
	linux-arm-kernel

From: Tudor Ambarus <tudor.ambarus@microchip.com>

The sam9x60 qspi controller uses 2 clocks, one for the peripheral register
access, the other for the qspi core and phy. Both are mandatory.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
v3: "pclk" was made mandatory in previous patch. Reword clock
descriptions.
v2:
- make "pclk" mandatory even for sama5d2. Unnamed clk will be
supported in the driver.
- drop unneeded example

 Documentation/devicetree/bindings/spi/atmel-quadspi.txt | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/atmel-quadspi.txt b/Documentation/devicetree/bindings/spi/atmel-quadspi.txt
index 1de54e87f5d6..fc7e83adab36 100644
--- a/Documentation/devicetree/bindings/spi/atmel-quadspi.txt
+++ b/Documentation/devicetree/bindings/spi/atmel-quadspi.txt
@@ -1,15 +1,19 @@
 * Atmel Quad Serial Peripheral Interface (QSPI)
 
 Required properties:
-- compatible:     Should be "atmel,sama5d2-qspi".
+- compatible:     Should be one of the following:
+		  - "atmel,sama5d2-qspi"
+		  - "microchip,sam9x60-qspi"
 - reg:            Should contain the locations and lengths of the base registers
                   and the mapped memory.
 - reg-names:      Should contain the resource reg names:
                   - qspi_base: configuration register address space
                   - qspi_mmap: memory mapped address space
 - interrupts:     Should contain the interrupt for the device.
-- clocks:         The phandle of the clock needed by the QSPI controller.
-- clock-names:    Should contain "pclk" for the peripheral clock.
+- clocks:         Should reference the peripheral clock and the QSPI system
+                  clock if available.
+- clock-names:    Should contain "pclk" for the peripheral clock and "qspick"
+                  for the system clock when available.
 - #address-cells: Should be <1>.
 - #size-cells:    Should be <0>.
 
-- 
2.9.5


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 13/13] spi: atmel-quadspi: add support for sam9x60 qspi controller
  2019-02-02  4:07 [PATCH v3 00/13] spi: atmel-quadspi: introduce sam9x60 qspi controller Tudor.Ambarus
                   ` (11 preceding siblings ...)
  2019-02-02  4:07 ` [PATCH v3 12/13] dt-bindings: spi: atmel-quadspi: QuadSPI driver for Microchip SAM9X60 Tudor.Ambarus
@ 2019-02-02  4:07 ` Tudor.Ambarus
  2019-02-02  7:29   ` Boris Brezillon
  12 siblings, 1 reply; 31+ messages in thread
From: Tudor.Ambarus @ 2019-02-02  4:07 UTC (permalink / raw)
  To: broonie, robh+dt, mark.rutland, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, bbrezillon, Cyrille.Pitchen, bugalski.piotr
  Cc: devicetree, Tudor.Ambarus, linux-kernel, linux-spi, linux-mtd,
	linux-arm-kernel

From: Tudor Ambarus <tudor.ambarus@microchip.com>

The sam9x60 qspi controller uses 2 clocks, one for the peripheral register
access, the other for the qspi core and phy. Both are mandatory. It uses
different transfer type bits in IFR register. It has dedicated registers
to specify a read or a write instruction: Read Instruction Code Register
(RICR) and Write Instruction Code Register (WICR). ICR/RICR/WICR have
identical fields.

Tested with sst26vf064b jedec,spi-nor flash. Backward compatibility test
done on sama5d2 qspi controller and mx25l25635e jedec,spi-nor flash.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
v3:
- reorganize the code and change ops functions pointers to avoid code
  duplication. From the IP perspective, the transfer type bits are
  different, and what registers are written: ricr/wicr instead of icr.
- treat just regular spi transfers. Mem transfers will be added together
  with dirmap support.
v2:
- rework clock handling
- reorder setting of register values in set_cfg() calls -> move functions
  that can fail in the upper part of the function body.

 drivers/spi/atmel-quadspi.c | 295 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 234 insertions(+), 61 deletions(-)

diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
index c9548942535a..af1e4e25097a 100644
--- a/drivers/spi/atmel-quadspi.c
+++ b/drivers/spi/atmel-quadspi.c
@@ -19,6 +19,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/spi/spi-mem.h>
 
@@ -35,7 +36,9 @@
 
 #define QSPI_IAR     0x0030  /* Instruction Address Register */
 #define QSPI_ICR     0x0034  /* Instruction Code Register */
+#define QSPI_WICR    0x0034  /* Write Instruction Code Register */
 #define QSPI_IFR     0x0038  /* Instruction Frame Register */
+#define QSPI_RICR    0x003C  /* Read Instruction Code Register */
 
 #define QSPI_SMR     0x0040  /* Scrambling Mode Register */
 #define QSPI_SKR     0x0044  /* Scrambling Key Register */
@@ -88,7 +91,7 @@
 #define QSPI_SCR_DLYBS_MASK             GENMASK(23, 16)
 #define QSPI_SCR_DLYBS(n)               (((n) << 16) & QSPI_SCR_DLYBS_MASK)
 
-/* Bitfields in QSPI_ICR (Instruction Code Register) */
+/* Bitfields in QSPI_ICR (Read/Write Instruction Code Register) */
 #define QSPI_ICR_INST_MASK              GENMASK(7, 0)
 #define QSPI_ICR_INST(inst)             (((inst) << 0) & QSPI_ICR_INST_MASK)
 #define QSPI_ICR_OPT_MASK               GENMASK(23, 16)
@@ -117,6 +120,7 @@
 #define QSPI_IFR_CRM                    BIT(14)
 #define QSPI_IFR_NBDUM_MASK             GENMASK(20, 16)
 #define QSPI_IFR_NBDUM(n)               (((n) << 16) & QSPI_IFR_NBDUM_MASK)
+#define QSPI_IFR_APBTFRTYP_READ		BIT(24)
 
 /* Bitfields in QSPI_SMR (Scrambling Mode Register) */
 #define QSPI_SMR_SCREN                  BIT(0)
@@ -133,16 +137,39 @@
 #define QSPI_WPSR_WPVSRC(src)           (((src) << 8) & QSPI_WPSR_WPVSRC)
 
 
+/* Describes register values. */
+struct atmel_qspi_cfg {
+	u32 icr;
+	u32 iar;
+	u32 ifr;
+};
+
+struct atmel_qspi_caps;
+
 struct atmel_qspi {
 	void __iomem		*regs;
 	void __iomem		*mem;
 	struct clk		*pclk;
+	struct clk		*qspick;
 	struct platform_device	*pdev;
+	const struct atmel_qspi_caps *caps;
 	u32			pending;
 	u32			smm;
 	struct completion	cmd_completion;
 };
 
+struct atmel_qspi_ops {
+	void (*set_tfrtyp)(const struct spi_mem_op *op,
+			   struct atmel_qspi_cfg *cfg);
+	void (*write_regs)(void __iomem *base, const struct spi_mem_op *op,
+			   const struct atmel_qspi_cfg *cfg);
+};
+
+struct atmel_qspi_caps {
+	const struct atmel_qspi_ops *ops;
+	bool has_qspick;
+};
+
 struct atmel_qspi_mode {
 	u8 cmd_buswidth;
 	u8 addr_buswidth;
@@ -200,30 +227,36 @@ static bool atmel_qspi_supports_op(struct spi_mem *mem,
 	return true;
 }
 
-static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
+static int atmel_qspi_set_mode(struct atmel_qspi_cfg *cfg,
+			       const struct spi_mem_op *op)
 {
-	struct atmel_qspi *aq = spi_controller_get_devdata(mem->spi->master);
-	void __iomem *base = aq->regs;
-	int mode;
-	u32 dummy_cycles = 0;
-	u32 iar, icr, ifr, sr;
-	int err = 0;
-
-	iar = 0;
-	icr = QSPI_ICR_INST(op->cmd.opcode);
-	ifr = QSPI_IFR_INSTEN;
+	int mode = atmel_qspi_find_mode(op);
 
-	/* Set the QSPI controller in Serial Memory Mode */
-	if (aq->smm != QSPI_MR_SMM) {
-		writel_relaxed(QSPI_MR_SMM, base + QSPI_MR);
-		aq->smm = QSPI_MR_SMM;
-	}
-
-	mode = atmel_qspi_find_mode(op);
 	if (mode < 0)
 		return mode;
+	cfg->ifr = sama5d2_qspi_modes[mode].config;
+	return 0;
+}
 
-	ifr |= sama5d2_qspi_modes[mode].config;
+/*
+ * atmel_qspi_set_address_mode() - set address mode.
+ * @cfg:	contains register values
+ * @op:		describes a SPI memory operation
+ *
+ * The controller allows 24 and 32-bit addressing while NAND-flash requires
+ * 16-bit long. Handling 8-bit long addresses is done using the option field.
+ * For the 16-bit addresses, the workaround depends of the number of requested
+ * dummy bits. If there are 8 or more dummy cycles, the address is shifted and
+ * sent with the first dummy byte. Otherwise opcode is disabled and the first
+ * byte of the address contains the command opcode (works only if the opcode and
+ * address use the same buswidth). The limitation is when the 16-bit address is
+ * used without enough dummy cycles and the opcode is using a different buswidth
+ * than the address.
+ */
+static int atmel_qspi_set_address_mode(struct atmel_qspi_cfg *cfg,
+				       const struct spi_mem_op *op)
+{
+	u32 dummy_cycles = 0;
 
 	if (op->dummy.buswidth && op->dummy.nbytes)
 		dummy_cycles = op->dummy.nbytes * 8 / op->dummy.buswidth;
@@ -233,28 +266,28 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 		case 0:
 			break;
 		case 1:
-			ifr |= QSPI_IFR_OPTEN | QSPI_IFR_OPTL_8BIT;
-			icr |= QSPI_ICR_OPT(op->addr.val & 0xff);
+			cfg->ifr |= QSPI_IFR_OPTEN | QSPI_IFR_OPTL_8BIT;
+			cfg->icr = QSPI_ICR_OPT(op->addr.val & 0xff);
 			break;
 		case 2:
 			if (dummy_cycles < 8 / op->addr.buswidth) {
-				ifr &= ~QSPI_IFR_INSTEN;
-				ifr |= QSPI_IFR_ADDREN;
-				iar = (op->cmd.opcode << 16) |
-					(op->addr.val & 0xffff);
+				cfg->ifr &= ~QSPI_IFR_INSTEN;
+				cfg->ifr |= QSPI_IFR_ADDREN;
+				cfg->iar = (op->cmd.opcode << 16) |
+					   (op->addr.val & 0xffff);
 			} else {
-				ifr |= QSPI_IFR_ADDREN;
-				iar = (op->addr.val << 8) & 0xffffff;
+				cfg->ifr |= QSPI_IFR_ADDREN;
+				cfg->iar = (op->addr.val << 8) & 0xffffff;
 				dummy_cycles -= 8 / op->addr.buswidth;
 			}
 			break;
 		case 3:
-			ifr |= QSPI_IFR_ADDREN;
-			iar = op->addr.val & 0xffffff;
+			cfg->ifr |= QSPI_IFR_ADDREN;
+			cfg->iar = op->addr.val & 0xffffff;
 			break;
 		case 4:
-			ifr |= QSPI_IFR_ADDREN | QSPI_IFR_ADDRL;
-			iar = op->addr.val & 0x7ffffff;
+			cfg->ifr |= QSPI_IFR_ADDREN | QSPI_IFR_ADDRL;
+			cfg->iar = op->addr.val & 0x7ffffff;
 			break;
 		default:
 			return -ENOTSUPP;
@@ -263,22 +296,99 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 
 	/* Set number of dummy cycles */
 	if (dummy_cycles)
-		ifr |= QSPI_IFR_NBDUM(dummy_cycles);
+		cfg->ifr |= QSPI_IFR_NBDUM(dummy_cycles);
 
-	/* Set data enable */
-	if (op->data.nbytes)
-		ifr |= QSPI_IFR_DATAEN;
+	return 0;
+}
 
+static void atmel_qspi_sama5d2_set_tfrtyp(const struct spi_mem_op *op,
+					  struct atmel_qspi_cfg *cfg)
+{
 	if (op->data.dir == SPI_MEM_DATA_OUT)
-		ifr |= QSPI_IFR_SAMA5D2_WRITE_TRSFR;
+		cfg->ifr |= QSPI_IFR_SAMA5D2_WRITE_TRSFR;
+}
 
+static void atmel_qspi_sama5d2_write_regs(void __iomem *base,
+					  const struct spi_mem_op *op,
+					  const struct atmel_qspi_cfg *cfg)
+{
 	/* Clear pending interrupts */
 	(void)readl_relaxed(base + QSPI_SR);
 
 	/* Set QSPI Instruction Frame registers */
-	writel_relaxed(iar, base + QSPI_IAR);
-	writel_relaxed(icr, base + QSPI_ICR);
-	writel_relaxed(ifr, base + QSPI_IFR);
+	writel_relaxed(cfg->iar, base + QSPI_IAR);
+	writel_relaxed(cfg->icr, base + QSPI_ICR);
+	writel_relaxed(cfg->ifr, base + QSPI_IFR);
+}
+
+static void atmel_qspi_sam9x60_set_tfrtyp(const struct spi_mem_op *op,
+					  struct atmel_qspi_cfg *cfg)
+{
+	if (!op->addr.nbytes && op->data.dir == SPI_MEM_DATA_IN)
+		cfg->ifr |= QSPI_IFR_APBTFRTYP_READ;
+}
+
+static void atmel_qspi_sam9x60_write_regs(void __iomem *base,
+					  const struct spi_mem_op *op,
+					  const struct atmel_qspi_cfg *cfg)
+{
+	/* Clear pending interrupts */
+	(void)readl_relaxed(base + QSPI_SR);
+
+	/* Set QSPI Instruction Frame registers */
+	writel_relaxed(cfg->iar, base + QSPI_IAR);
+	if (op->data.dir == SPI_MEM_DATA_IN)
+		writel_relaxed(cfg->icr, base + QSPI_RICR);
+	else
+		writel_relaxed(cfg->icr, base + QSPI_ICR);
+	writel_relaxed(cfg->ifr, base + QSPI_IFR);
+}
+
+static int atmel_qspi_set_cfg(struct atmel_qspi *aq,
+			      const struct spi_mem_op *op,
+			      struct atmel_qspi_cfg *cfg)
+{
+	void __iomem *base = aq->regs;
+	int ret;
+
+	/* Set the QSPI controller in Serial Memory Mode */
+	if (aq->smm != QSPI_MR_SMM) {
+		writel_relaxed(QSPI_MR_SMM, base + QSPI_MR);
+		aq->smm = QSPI_MR_SMM;
+	}
+
+	ret = atmel_qspi_set_mode(cfg, op);
+	if (ret)
+		return ret;
+
+	ret = atmel_qspi_set_address_mode(cfg, op);
+	if (ret)
+		return ret;
+
+	cfg->ifr |= QSPI_IFR_INSTEN;
+	cfg->icr |= QSPI_ICR_INST(op->cmd.opcode);
+
+	/* Set data enable */
+	if (op->data.nbytes)
+		cfg->ifr |= QSPI_IFR_DATAEN;
+
+	aq->caps->ops->set_tfrtyp(op, cfg);
+	aq->caps->ops->write_regs(base, op, cfg);
+
+	return 0;
+}
+
+static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
+{
+	struct atmel_qspi *aq = spi_controller_get_devdata(mem->spi->master);
+	void __iomem *base = aq->regs;
+	struct atmel_qspi_cfg cfg = {0};
+	u32 sr;
+	int err;
+
+	err = atmel_qspi_set_cfg(aq, op, &cfg);
+	if (err)
+		return err;
 
 	/* Skip to the final steps if there is no data */
 	if (op->data.nbytes) {
@@ -287,11 +397,11 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 
 		/* Send/Receive data */
 		if (op->data.dir == SPI_MEM_DATA_IN)
-			_memcpy_fromio(op->data.buf.in,
-				aq->mem + iar, op->data.nbytes);
+			_memcpy_fromio(op->data.buf.in, aq->mem + cfg.iar,
+				       op->data.nbytes);
 		else
-			_memcpy_toio(aq->mem + iar,
-				op->data.buf.out, op->data.nbytes);
+			_memcpy_toio(aq->mem + cfg.iar, op->data.buf.out,
+				     op->data.nbytes);
 
 		/* Release the chip-select */
 		writel_relaxed(QSPI_CR_LASTXFER, base + QSPI_CR);
@@ -391,9 +501,22 @@ static int atmel_qspi_probe(struct platform_device *pdev)
 	struct spi_controller *ctrl;
 	struct atmel_qspi *aq;
 	struct resource *res;
+	const struct atmel_qspi_caps *caps;
+	struct device *dev = &pdev->dev;
 	int irq, err = 0;
 
-	ctrl = spi_alloc_master(&pdev->dev, sizeof(*aq));
+	caps = of_device_get_match_data(dev);
+	if (!caps) {
+		dev_err(dev, "Could not retrieve QSPI caps\n");
+		return -EINVAL;
+	}
+
+	if (!caps->ops->set_tfrtyp || !caps->ops->write_regs) {
+		dev_err(dev, "Could not retrieve QSPI ops\n");
+		return -EINVAL;
+	}
+
+	ctrl = spi_alloc_master(dev, sizeof(*aq));
 	if (!ctrl)
 		return -ENOMEM;
 
@@ -409,32 +532,33 @@ static int atmel_qspi_probe(struct platform_device *pdev)
 
 	init_completion(&aq->cmd_completion);
 	aq->pdev = pdev;
+	aq->caps = caps;
 
 	/* Map the registers */
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "qspi_base");
-	aq->regs = devm_ioremap_resource(&pdev->dev, res);
+	aq->regs = devm_ioremap_resource(dev, res);
 	if (IS_ERR(aq->regs)) {
-		dev_err(&pdev->dev, "missing registers\n");
+		dev_err(dev, "missing registers\n");
 		err = PTR_ERR(aq->regs);
 		goto exit;
 	}
 
 	/* Map the AHB memory */
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "qspi_mmap");
-	aq->mem = devm_ioremap_resource(&pdev->dev, res);
+	aq->mem = devm_ioremap_resource(dev, res);
 	if (IS_ERR(aq->mem)) {
-		dev_err(&pdev->dev, "missing AHB memory\n");
+		dev_err(dev, "missing AHB memory\n");
 		err = PTR_ERR(aq->mem);
 		goto exit;
 	}
 
 	/* Get the peripheral clock */
-	aq->pclk = devm_clk_get(&pdev->dev, "pclk");
+	aq->pclk = devm_clk_get(dev, "pclk");
 	if (IS_ERR(aq->pclk))
-		aq->pclk = devm_clk_get(&pdev->dev, NULL);
+		aq->pclk = devm_clk_get(dev, NULL);
 
 	if (IS_ERR(aq->pclk)) {
-		dev_err(&pdev->dev, "missing peripheral clock\n");
+		dev_err(dev, "missing peripheral clock\n");
 		err = PTR_ERR(aq->pclk);
 		goto exit;
 	}
@@ -442,32 +566,52 @@ static int atmel_qspi_probe(struct platform_device *pdev)
 	/* Enable the peripheral clock */
 	err = clk_prepare_enable(aq->pclk);
 	if (err) {
-		dev_err(&pdev->dev, "failed to enable the peripheral clock\n");
+		dev_err(dev, "failed to enable the peripheral clock\n");
 		goto exit;
 	}
 
+	if (caps->has_qspick) {
+		/* Get the QSPI system clock */
+		aq->qspick = devm_clk_get(dev, "qspick");
+		if (IS_ERR(aq->qspick)) {
+			dev_err(dev, "missing system clock\n");
+			err = PTR_ERR(aq->qspick);
+			goto disable_pclk;
+		}
+
+		/* Enable the QSPI system clock */
+		err = clk_prepare_enable(aq->qspick);
+		if (err) {
+			dev_err(dev,
+				"failed to enable the QSPI system clock\n");
+			goto disable_pclk;
+		}
+	}
+
 	/* Request the IRQ */
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
-		dev_err(&pdev->dev, "missing IRQ\n");
+		dev_err(dev, "missing IRQ\n");
 		err = irq;
-		goto disable_pclk;
+		goto disable_qspick;
 	}
-	err = devm_request_irq(&pdev->dev, irq, atmel_qspi_interrupt,
-			       0, dev_name(&pdev->dev), aq);
+	err = devm_request_irq(dev, irq, atmel_qspi_interrupt, 0,
+			       dev_name(dev), aq);
 	if (err)
-		goto disable_pclk;
+		goto disable_qspick;
 
 	err = atmel_qspi_init(aq);
 	if (err)
-		goto disable_pclk;
+		goto disable_qspick;
 
 	err = spi_register_controller(ctrl);
 	if (err)
-		goto disable_pclk;
+		goto disable_qspick;
 
 	return 0;
 
+disable_qspick:
+	clk_disable_unprepare(aq->qspick);
 disable_pclk:
 	clk_disable_unprepare(aq->pclk);
 exit:
@@ -483,6 +627,7 @@ static int atmel_qspi_remove(struct platform_device *pdev)
 
 	spi_unregister_controller(ctrl);
 	writel_relaxed(QSPI_CR_QSPIDIS, aq->regs + QSPI_CR);
+	clk_disable_unprepare(aq->qspick);
 	clk_disable_unprepare(aq->pclk);
 	return 0;
 }
@@ -491,6 +636,7 @@ static int __maybe_unused atmel_qspi_suspend(struct device *dev)
 {
 	struct atmel_qspi *aq = dev_get_drvdata(dev);
 
+	clk_disable_unprepare(aq->qspick);
 	clk_disable_unprepare(aq->pclk);
 
 	return 0;
@@ -501,6 +647,7 @@ static int __maybe_unused atmel_qspi_resume(struct device *dev)
 	struct atmel_qspi *aq = dev_get_drvdata(dev);
 
 	clk_prepare_enable(aq->pclk);
+	clk_prepare_enable(aq->qspick);
 
 	return atmel_qspi_init(aq);
 }
@@ -508,8 +655,34 @@ static int __maybe_unused atmel_qspi_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(atmel_qspi_pm_ops, atmel_qspi_suspend,
 			 atmel_qspi_resume);
 
+static const struct atmel_qspi_ops atmel_sama5d2_qspi_ops = {
+	.set_tfrtyp = atmel_qspi_sama5d2_set_tfrtyp,
+	.write_regs = atmel_qspi_sama5d2_write_regs,
+};
+
+static const struct atmel_qspi_caps atmel_sama5d2_qspi_caps = {
+	.ops = &atmel_sama5d2_qspi_ops,
+};
+
+static const struct atmel_qspi_ops atmel_sam9x60_qspi_ops = {
+	.set_tfrtyp = atmel_qspi_sam9x60_set_tfrtyp,
+	.write_regs = atmel_qspi_sam9x60_write_regs,
+};
+
+static const struct atmel_qspi_caps atmel_sam9x60_qspi_caps = {
+	.ops = &atmel_sam9x60_qspi_ops,
+	.has_qspick = true,
+};
+
 static const struct of_device_id atmel_qspi_dt_ids[] = {
-	{ .compatible = "atmel,sama5d2-qspi" },
+	{
+		.compatible = "atmel,sama5d2-qspi",
+		.data = &atmel_sama5d2_qspi_caps,
+	},
+	{
+		.compatible = "microchip,sam9x60-qspi",
+		.data = &atmel_sam9x60_qspi_caps,
+	},
 	{ /* sentinel */ }
 };
 
-- 
2.9.5


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 01/13] spi: atmel-quadspi: cache MR value to avoid a write access
  2019-02-02  4:07 ` [PATCH v3 01/13] spi: atmel-quadspi: cache MR value to avoid a write access Tudor.Ambarus
@ 2019-02-02  7:06   ` Boris Brezillon
  2019-02-02  8:38     ` Tudor.Ambarus
  0 siblings, 1 reply; 31+ messages in thread
From: Boris Brezillon @ 2019-02-02  7:06 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: mark.rutland, devicetree, alexandre.belloni, linux-kernel,
	Cyrille.Pitchen, Nicolas.Ferre, robh+dt, linux-spi,
	Ludovic.Desroches, broonie, linux-mtd, bugalski.piotr,
	linux-arm-kernel

On Sat, 2 Feb 2019 04:07:13 +0000
<Tudor.Ambarus@microchip.com> wrote:

> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> Cache Serial Memory Mode (SMM) value to avoid write access when
> setting the controller in serial memory mode. SMM is set in
> exec_op() and not at probe time, to let room for future regular
> SPI support.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
> v3: update smm value when different. rename mr/smm
> v2: cache MR value instead of moving the write access at probe
> 
>  drivers/spi/atmel-quadspi.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
> index ddc712410812..645284c6ec9a 100644
> --- a/drivers/spi/atmel-quadspi.c
> +++ b/drivers/spi/atmel-quadspi.c
> @@ -155,6 +155,7 @@ struct atmel_qspi {
>  	struct clk		*clk;
>  	struct platform_device	*pdev;
>  	u32			pending;
> +	u32			smm;
>  	struct completion	cmd_completion;
>  };
>  
> @@ -238,7 +239,11 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  	icr = QSPI_ICR_INST(op->cmd.opcode);
>  	ifr = QSPI_IFR_INSTEN;
>  
> -	qspi_writel(aq, QSPI_MR, QSPI_MR_SMM);
> +	/* Set the QSPI controller in Serial Memory Mode */
> +	if (aq->smm != QSPI_MR_SMM) {

Sorry, I think I misunderstood your previous suggestion, I thought the
reg was called SMM. If the reg is called MR and the value you expect in
there is SMM, then the field should be named ->mr as it caches the
whole reg, not only the SMM bit. So it's actually:

	if (aq->mr != QSPI_MR_SMM) {

> +		qspi_writel(aq, QSPI_MR, QSPI_MR_SMM);
> +		aq->smm = QSPI_MR_SMM;
> +	}
>  
>  	mode = find_mode(op);
>  	if (mode < 0)


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 03/13] spi: atmel-quadspi: drop wrappers for iomem accesses
  2019-02-02  4:07 ` [PATCH v3 03/13] spi: atmel-quadspi: drop wrappers for iomem accesses Tudor.Ambarus
@ 2019-02-02  7:11   ` Boris Brezillon
  2019-02-02  8:44     ` Tudor.Ambarus
  0 siblings, 1 reply; 31+ messages in thread
From: Boris Brezillon @ 2019-02-02  7:11 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: mark.rutland, devicetree, alexandre.belloni, linux-kernel,
	Cyrille.Pitchen, Nicolas.Ferre, robh+dt, linux-spi,
	Ludovic.Desroches, broonie, linux-mtd, bugalski.piotr,
	linux-arm-kernel

On Sat, 2 Feb 2019 04:07:19 +0000
<Tudor.Ambarus@microchip.com> wrote:

> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> The wrappers hid that the accesses are relaxed. Drop them.
> 
> Suggested-by: Boris Brezillon <bbrezillon@kernel.org>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
> v3: no change
> v2: new patch
> 
>  drivers/spi/atmel-quadspi.c | 47 +++++++++++++++++++--------------------------
>  1 file changed, 20 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
> index feeddcb25e1f..131374db0db4 100644
> --- a/drivers/spi/atmel-quadspi.c
> +++ b/drivers/spi/atmel-quadspi.c
> @@ -175,17 +175,6 @@ static const struct qspi_mode sama5d2_qspi_modes[] = {
>  	{ 4, 4, 4, QSPI_IFR_WIDTH_QUAD_CMD },
>  };
>  
> -/* Register access functions */
> -static inline u32 qspi_readl(struct atmel_qspi *aq, u32 reg)
> -{
> -	return readl_relaxed(aq->regs + reg);
> -}
> -
> -static inline void qspi_writel(struct atmel_qspi *aq, u32 reg, u32 value)
> -{
> -	writel_relaxed(value, aq->regs + reg);
> -}
> -
>  static inline bool is_compatible(const struct spi_mem_op *op,
>  				 const struct qspi_mode *mode)
>  {
> @@ -229,6 +218,7 @@ static bool atmel_qspi_supports_op(struct spi_mem *mem,
>  static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  {
>  	struct atmel_qspi *aq = spi_controller_get_devdata(mem->spi->master);
> +	void __iomem *base = aq->regs;

Can we name this variable regs instead of base or even get rid of it
and dereference aq->regs in the xxx_relaxed() calls (doesn't look like
the lines would be over 80 chars even when doing that). With this
addressed, you can add:

Reviewed-by: Boris Brezillon <bbrezillon@kernel.org>

>  	int mode;
>  	u32 dummy_cycles = 0;
>  	u32 iar, icr, ifr, sr;
> @@ -240,7 +230,7 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  
>  	/* Set the QSPI controller in Serial Memory Mode */
>  	if (aq->smm != QSPI_MR_SMM) {
> -		qspi_writel(aq, QSPI_MR, QSPI_MR_SMM);
> +		writel_relaxed(QSPI_MR_SMM, base + QSPI_MR);
>  		aq->smm = QSPI_MR_SMM;
>  	}
>  
> @@ -300,17 +290,17 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  		ifr |= QSPI_IFR_TFRTYP_TRSFR_WRITE;
>  
>  	/* Clear pending interrupts */
> -	(void)qspi_readl(aq, QSPI_SR);
> +	(void)readl_relaxed(base + QSPI_SR);
>  
>  	/* Set QSPI Instruction Frame registers */
> -	qspi_writel(aq, QSPI_IAR, iar);
> -	qspi_writel(aq, QSPI_ICR, icr);
> -	qspi_writel(aq, QSPI_IFR, ifr);
> +	writel_relaxed(iar, base + QSPI_IAR);
> +	writel_relaxed(icr, base + QSPI_ICR);
> +	writel_relaxed(ifr, base + QSPI_IFR);
>  
>  	/* Skip to the final steps if there is no data */
>  	if (op->data.nbytes) {
>  		/* Dummy read of QSPI_IFR to synchronize APB and AHB accesses */
> -		(void)qspi_readl(aq, QSPI_IFR);
> +		(void)readl_relaxed(base + QSPI_IFR);
>  
>  		/* Send/Receive data */
>  		if (op->data.dir == SPI_MEM_DATA_IN)
> @@ -321,22 +311,22 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  				op->data.buf.out, op->data.nbytes);
>  
>  		/* Release the chip-select */
> -		qspi_writel(aq, QSPI_CR, QSPI_CR_LASTXFER);
> +		writel_relaxed(QSPI_CR_LASTXFER, base + QSPI_CR);
>  	}
>  
>  	/* Poll INSTRuction End status */
> -	sr = qspi_readl(aq, QSPI_SR);
> +	sr = readl_relaxed(base + QSPI_SR);
>  	if ((sr & QSPI_SR_CMD_COMPLETED) == QSPI_SR_CMD_COMPLETED)
>  		return err;
>  
>  	/* Wait for INSTRuction End interrupt */
>  	reinit_completion(&aq->cmd_completion);
>  	aq->pending = sr & QSPI_SR_CMD_COMPLETED;
> -	qspi_writel(aq, QSPI_IER, QSPI_SR_CMD_COMPLETED);
> +	writel_relaxed(QSPI_SR_CMD_COMPLETED, base + QSPI_IER);
>  	if (!wait_for_completion_timeout(&aq->cmd_completion,
>  					 msecs_to_jiffies(1000)))
>  		err = -ETIMEDOUT;
> -	qspi_writel(aq, QSPI_IDR, QSPI_SR_CMD_COMPLETED);
> +	writel_relaxed(QSPI_SR_CMD_COMPLETED, base + QSPI_IDR);
>  
>  	return err;
>  }
> @@ -375,18 +365,20 @@ static int atmel_qspi_setup(struct spi_device *spi)
>  		scbr--;
>  
>  	scr = QSPI_SCR_SCBR(scbr);
> -	qspi_writel(aq, QSPI_SCR, scr);
> +	writel_relaxed(scr, aq->regs + QSPI_SCR);
>  
>  	return 0;
>  }
>  
>  static int atmel_qspi_init(struct atmel_qspi *aq)
>  {
> +	void __iomem *base = aq->regs;
> +
>  	/* Reset the QSPI controller */
> -	qspi_writel(aq, QSPI_CR, QSPI_CR_SWRST);
> +	writel_relaxed(QSPI_CR_SWRST, base + QSPI_CR);
>  
>  	/* Enable the QSPI controller */
> -	qspi_writel(aq, QSPI_CR, QSPI_CR_QSPIEN);
> +	writel_relaxed(QSPI_CR_QSPIEN, base + QSPI_CR);
>  
>  	return 0;
>  }
> @@ -394,10 +386,11 @@ static int atmel_qspi_init(struct atmel_qspi *aq)
>  static irqreturn_t atmel_qspi_interrupt(int irq, void *dev_id)
>  {
>  	struct atmel_qspi *aq = (struct atmel_qspi *)dev_id;
> +	void __iomem *base = aq->regs;
>  	u32 status, mask, pending;
>  
> -	status = qspi_readl(aq, QSPI_SR);
> -	mask = qspi_readl(aq, QSPI_IMR);
> +	status = readl_relaxed(base + QSPI_SR);
> +	mask = readl_relaxed(base + QSPI_IMR);
>  	pending = status & mask;
>  
>  	if (!pending)
> @@ -503,7 +496,7 @@ static int atmel_qspi_remove(struct platform_device *pdev)
>  	struct atmel_qspi *aq = spi_controller_get_devdata(ctrl);
>  
>  	spi_unregister_controller(ctrl);
> -	qspi_writel(aq, QSPI_CR, QSPI_CR_QSPIDIS);
> +	writel_relaxed(QSPI_CR_QSPIDIS, aq->regs + QSPI_CR);
>  	clk_disable_unprepare(aq->clk);
>  	return 0;
>  }


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 04/13] spi: atmel-quadspi: fix naming scheme
  2019-02-02  4:07 ` [PATCH v3 04/13] spi: atmel-quadspi: fix naming scheme Tudor.Ambarus
@ 2019-02-02  7:12   ` Boris Brezillon
  0 siblings, 0 replies; 31+ messages in thread
From: Boris Brezillon @ 2019-02-02  7:12 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: mark.rutland, devicetree, alexandre.belloni, linux-kernel,
	Cyrille.Pitchen, Nicolas.Ferre, robh+dt, linux-spi,
	Ludovic.Desroches, broonie, linux-mtd, bugalski.piotr,
	linux-arm-kernel

On Sat, 2 Feb 2019 04:07:22 +0000
<Tudor.Ambarus@microchip.com> wrote:

> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> Let general names to core drivers.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Reviewed-by: Boris Brezillon <bbrezillon@kernel.org>

> ---
> v3: no change
> v2: update after the removing of iomem access wrappers
> 
>  drivers/spi/atmel-quadspi.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
> index 131374db0db4..47ed751a91e7 100644
> --- a/drivers/spi/atmel-quadspi.c
> +++ b/drivers/spi/atmel-quadspi.c
> @@ -158,14 +158,14 @@ struct atmel_qspi {
>  	struct completion	cmd_completion;
>  };
>  
> -struct qspi_mode {
> +struct atmel_qspi_mode {
>  	u8 cmd_buswidth;
>  	u8 addr_buswidth;
>  	u8 data_buswidth;
>  	u32 config;
>  };
>  
> -static const struct qspi_mode sama5d2_qspi_modes[] = {
> +static const struct atmel_qspi_mode sama5d2_qspi_modes[] = {
>  	{ 1, 1, 1, QSPI_IFR_WIDTH_SINGLE_BIT_SPI },
>  	{ 1, 1, 2, QSPI_IFR_WIDTH_DUAL_OUTPUT },
>  	{ 1, 1, 4, QSPI_IFR_WIDTH_QUAD_OUTPUT },
> @@ -175,8 +175,8 @@ static const struct qspi_mode sama5d2_qspi_modes[] = {
>  	{ 4, 4, 4, QSPI_IFR_WIDTH_QUAD_CMD },
>  };
>  
> -static inline bool is_compatible(const struct spi_mem_op *op,
> -				 const struct qspi_mode *mode)
> +static inline bool atmel_qspi_is_compatible(const struct spi_mem_op *op,
> +					    const struct atmel_qspi_mode *mode)
>  {
>  	if (op->cmd.buswidth != mode->cmd_buswidth)
>  		return false;
> @@ -190,12 +190,12 @@ static inline bool is_compatible(const struct spi_mem_op *op,
>  	return true;
>  }
>  
> -static int find_mode(const struct spi_mem_op *op)
> +static int atmel_qspi_find_mode(const struct spi_mem_op *op)
>  {
>  	u32 i;
>  
>  	for (i = 0; i < ARRAY_SIZE(sama5d2_qspi_modes); i++)
> -		if (is_compatible(op, &sama5d2_qspi_modes[i]))
> +		if (atmel_qspi_is_compatible(op, &sama5d2_qspi_modes[i]))
>  			return i;
>  
>  	return -1;
> @@ -204,7 +204,7 @@ static int find_mode(const struct spi_mem_op *op)
>  static bool atmel_qspi_supports_op(struct spi_mem *mem,
>  				   const struct spi_mem_op *op)
>  {
> -	if (find_mode(op) < 0)
> +	if (atmel_qspi_find_mode(op) < 0)
>  		return false;
>  
>  	/* special case not supported by hardware */
> @@ -234,7 +234,7 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  		aq->smm = QSPI_MR_SMM;
>  	}
>  
> -	mode = find_mode(op);
> +	mode = atmel_qspi_find_mode(op);
>  	if (mode < 0)
>  		return -ENOTSUPP;
>  


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 08/13] spi: atmel-quadspi: drop unused and NOP transfer macros
  2019-02-02  4:07 ` [PATCH v3 08/13] spi: atmel-quadspi: drop unused and NOP transfer macros Tudor.Ambarus
@ 2019-02-02  7:13   ` Boris Brezillon
  2019-02-02  8:46     ` Tudor.Ambarus
  0 siblings, 1 reply; 31+ messages in thread
From: Boris Brezillon @ 2019-02-02  7:13 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: mark.rutland, devicetree, alexandre.belloni, linux-kernel,
	Cyrille.Pitchen, Nicolas.Ferre, robh+dt, linux-spi,
	Ludovic.Desroches, broonie, linux-mtd, bugalski.piotr,
	linux-arm-kernel

On Sat, 2 Feb 2019 04:07:33 +0000
<Tudor.Ambarus@microchip.com> wrote:

> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> Remove NOP when setting read transfer type. Remove useless
> setting of write transfer type when
> op->data.dir == SPI_MEM_DATA_IN && !op->data.nbytes.
> 
> QSPI_IFR_TFRTYP_TRSFR_WRITE is specific just to sama5d2 qspi,
> rename it to QSPI_IFR_SAMA5D2_WRITE_TRSFR.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
> v3: new patch
> 
>  drivers/spi/atmel-quadspi.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
> index d26d4cd0e36b..bfa5f5e92d96 100644
> --- a/drivers/spi/atmel-quadspi.c
> +++ b/drivers/spi/atmel-quadspi.c
> @@ -113,11 +113,7 @@
>  #define QSPI_IFR_OPTL_4BIT              (2 << 8)
>  #define QSPI_IFR_OPTL_8BIT              (3 << 8)
>  #define QSPI_IFR_ADDRL                  BIT(10)
> -#define QSPI_IFR_TFRTYP_MASK            GENMASK(13, 12)
> -#define QSPI_IFR_TFRTYP_TRSFR_READ      (0 << 12)
> -#define QSPI_IFR_TFRTYP_TRSFR_READ_MEM  (1 << 12)
> -#define QSPI_IFR_TFRTYP_TRSFR_WRITE     (2 << 12)
> -#define QSPI_IFR_TFRTYP_TRSFR_WRITE_MEM (3 << 13)
> +#define QSPI_IFR_SAMA5D2_WRITE_TRSFR	BIT(13)

Can you define QSPI_IFR_TFRTYP_MEM (bit 12) even if it's not used yet?

>  #define QSPI_IFR_CRM                    BIT(14)
>  #define QSPI_IFR_NBDUM_MASK             GENMASK(20, 16)
>  #define QSPI_IFR_NBDUM(n)               (((n) << 16) & QSPI_IFR_NBDUM_MASK)
> @@ -273,10 +269,8 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  	if (op->data.nbytes)
>  		ifr |= QSPI_IFR_DATAEN;
>  
> -	if (op->data.dir == SPI_MEM_DATA_IN && op->data.nbytes)
> -		ifr |= QSPI_IFR_TFRTYP_TRSFR_READ;
> -	else
> -		ifr |= QSPI_IFR_TFRTYP_TRSFR_WRITE;
> +	if (op->data.dir == SPI_MEM_DATA_OUT)
> +		ifr |= QSPI_IFR_SAMA5D2_WRITE_TRSFR;
>  
>  	/* Clear pending interrupts */
>  	(void)readl_relaxed(base + QSPI_SR);


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 10/13] dt-bindings: spi: atmel-quadspi: make "pclk" mandatory
  2019-02-02  4:07 ` [PATCH v3 10/13] dt-bindings: spi: atmel-quadspi: make "pclk" mandatory Tudor.Ambarus
@ 2019-02-02  7:15   ` Boris Brezillon
  2019-02-02  8:47     ` Tudor.Ambarus
  0 siblings, 1 reply; 31+ messages in thread
From: Boris Brezillon @ 2019-02-02  7:15 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: mark.rutland, devicetree, alexandre.belloni, linux-kernel,
	Cyrille.Pitchen, Nicolas.Ferre, robh+dt, linux-spi,
	Ludovic.Desroches, broonie, linux-mtd, bugalski.piotr,
	linux-arm-kernel

On Sat, 2 Feb 2019 04:07:39 +0000
<Tudor.Ambarus@microchip.com> wrote:

> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> Naming clocks is a good practice. Make "pclk" madatory even if
> we support unnamed clock in the driver, to be backward compatible
> with old DTs.
> 
> Suggested-by: Boris Brezillon <bbrezillon@kernel.org>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
> v3: new patch
> 
>  Documentation/devicetree/bindings/spi/atmel-quadspi.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/atmel-quadspi.txt b/Documentation/devicetree/bindings/spi/atmel-quadspi.txt
> index e9dae6264d89..1de54e87f5d6 100644
> --- a/Documentation/devicetree/bindings/spi/atmel-quadspi.txt
> +++ b/Documentation/devicetree/bindings/spi/atmel-quadspi.txt
> @@ -9,6 +9,7 @@ Required properties:
>                    - qspi_mmap: memory mapped address space
>  - interrupts:     Should contain the interrupt for the device.
>  - clocks:         The phandle of the clock needed by the QSPI controller.
> +- clock-names:    Should contain "pclk" for the peripheral clock.
>  - #address-cells: Should be <1>.
>  - #size-cells:    Should be <0>.
>  
> @@ -20,6 +21,7 @@ spi@f0020000 {
>  	reg-names = "qspi_base", "qspi_mmap";
>  	interrupts = <52 IRQ_TYPE_LEVEL_HIGH 7>;
>  	clocks = <&pmc PMC_TYPE_PERIPHERAL 52>;
> +	clock-names = "pclk"

Missing semicolon. Once fixed you can add

Reviewed-by: Boris Brezillon <bbrezillon@kernel.org>

>  	#address-cells = <1>;
>  	#size-cells = <0>;
>  	pinctrl-names = "default";


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 11/13] spi: atmel-quadspi: add support for named peripheral clock
  2019-02-02  4:07 ` [PATCH v3 11/13] spi: atmel-quadspi: add support for named peripheral clock Tudor.Ambarus
@ 2019-02-02  7:16   ` Boris Brezillon
  0 siblings, 0 replies; 31+ messages in thread
From: Boris Brezillon @ 2019-02-02  7:16 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: mark.rutland, devicetree, alexandre.belloni, linux-kernel,
	Cyrille.Pitchen, Nicolas.Ferre, robh+dt, linux-spi,
	Ludovic.Desroches, broonie, linux-mtd, bugalski.piotr,
	linux-arm-kernel

On Sat, 2 Feb 2019 04:07:41 +0000
<Tudor.Ambarus@microchip.com> wrote:

> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> Naming clocks is a good practice. Keep supporting unnamed
> peripheral clock, to be backward compatible with old DTs.
> While here, rename clk to pclk, to indicate that it is a
> peripheral clock.
> 
> Suggested-by: Boris Brezillon <bbrezillon@kernel.org>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Reviewed-by: Boris Brezillon <bbrezillon@kernel.org>

> ---
> v3: new patch
> 
>  drivers/spi/atmel-quadspi.c | 33 ++++++++++++++++++---------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
> index bfa5f5e92d96..c9548942535a 100644
> --- a/drivers/spi/atmel-quadspi.c
> +++ b/drivers/spi/atmel-quadspi.c
> @@ -136,7 +136,7 @@
>  struct atmel_qspi {
>  	void __iomem		*regs;
>  	void __iomem		*mem;
> -	struct clk		*clk;
> +	struct clk		*pclk;
>  	struct platform_device	*pdev;
>  	u32			pending;
>  	u32			smm;
> @@ -338,7 +338,7 @@ static int atmel_qspi_setup(struct spi_device *spi)
>  	if (!spi->max_speed_hz)
>  		return -EINVAL;
>  
> -	src_rate = clk_get_rate(aq->clk);
> +	src_rate = clk_get_rate(aq->pclk);
>  	if (!src_rate)
>  		return -EINVAL;
>  
> @@ -429,15 +429,18 @@ static int atmel_qspi_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Get the peripheral clock */
> -	aq->clk = devm_clk_get(&pdev->dev, NULL);
> -	if (IS_ERR(aq->clk)) {
> +	aq->pclk = devm_clk_get(&pdev->dev, "pclk");
> +	if (IS_ERR(aq->pclk))
> +		aq->pclk = devm_clk_get(&pdev->dev, NULL);
> +
> +	if (IS_ERR(aq->pclk)) {
>  		dev_err(&pdev->dev, "missing peripheral clock\n");
> -		err = PTR_ERR(aq->clk);
> +		err = PTR_ERR(aq->pclk);
>  		goto exit;
>  	}
>  
>  	/* Enable the peripheral clock */
> -	err = clk_prepare_enable(aq->clk);
> +	err = clk_prepare_enable(aq->pclk);
>  	if (err) {
>  		dev_err(&pdev->dev, "failed to enable the peripheral clock\n");
>  		goto exit;
> @@ -448,25 +451,25 @@ static int atmel_qspi_probe(struct platform_device *pdev)
>  	if (irq < 0) {
>  		dev_err(&pdev->dev, "missing IRQ\n");
>  		err = irq;
> -		goto disable_clk;
> +		goto disable_pclk;
>  	}
>  	err = devm_request_irq(&pdev->dev, irq, atmel_qspi_interrupt,
>  			       0, dev_name(&pdev->dev), aq);
>  	if (err)
> -		goto disable_clk;
> +		goto disable_pclk;
>  
>  	err = atmel_qspi_init(aq);
>  	if (err)
> -		goto disable_clk;
> +		goto disable_pclk;
>  
>  	err = spi_register_controller(ctrl);
>  	if (err)
> -		goto disable_clk;
> +		goto disable_pclk;
>  
>  	return 0;
>  
> -disable_clk:
> -	clk_disable_unprepare(aq->clk);
> +disable_pclk:
> +	clk_disable_unprepare(aq->pclk);
>  exit:
>  	spi_controller_put(ctrl);
>  
> @@ -480,7 +483,7 @@ static int atmel_qspi_remove(struct platform_device *pdev)
>  
>  	spi_unregister_controller(ctrl);
>  	writel_relaxed(QSPI_CR_QSPIDIS, aq->regs + QSPI_CR);
> -	clk_disable_unprepare(aq->clk);
> +	clk_disable_unprepare(aq->pclk);
>  	return 0;
>  }
>  
> @@ -488,7 +491,7 @@ static int __maybe_unused atmel_qspi_suspend(struct device *dev)
>  {
>  	struct atmel_qspi *aq = dev_get_drvdata(dev);
>  
> -	clk_disable_unprepare(aq->clk);
> +	clk_disable_unprepare(aq->pclk);
>  
>  	return 0;
>  }
> @@ -497,7 +500,7 @@ static int __maybe_unused atmel_qspi_resume(struct device *dev)
>  {
>  	struct atmel_qspi *aq = dev_get_drvdata(dev);
>  
> -	clk_prepare_enable(aq->clk);
> +	clk_prepare_enable(aq->pclk);
>  
>  	return atmel_qspi_init(aq);
>  }


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 12/13] dt-bindings: spi: atmel-quadspi: QuadSPI driver for Microchip SAM9X60
  2019-02-02  4:07 ` [PATCH v3 12/13] dt-bindings: spi: atmel-quadspi: QuadSPI driver for Microchip SAM9X60 Tudor.Ambarus
@ 2019-02-02  7:17   ` Boris Brezillon
  0 siblings, 0 replies; 31+ messages in thread
From: Boris Brezillon @ 2019-02-02  7:17 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: mark.rutland, devicetree, alexandre.belloni, linux-kernel,
	Cyrille.Pitchen, Nicolas.Ferre, robh+dt, linux-spi,
	Ludovic.Desroches, broonie, linux-mtd, bugalski.piotr,
	linux-arm-kernel

On Sat, 2 Feb 2019 04:07:44 +0000
<Tudor.Ambarus@microchip.com> wrote:

> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> The sam9x60 qspi controller uses 2 clocks, one for the peripheral register
> access, the other for the qspi core and phy. Both are mandatory.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Reviewed-by: Boris Brezillon <bbrezillon@kernel.org>

> ---
> v3: "pclk" was made mandatory in previous patch. Reword clock
> descriptions.
> v2:
> - make "pclk" mandatory even for sama5d2. Unnamed clk will be
> supported in the driver.
> - drop unneeded example
> 
>  Documentation/devicetree/bindings/spi/atmel-quadspi.txt | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/spi/atmel-quadspi.txt b/Documentation/devicetree/bindings/spi/atmel-quadspi.txt
> index 1de54e87f5d6..fc7e83adab36 100644
> --- a/Documentation/devicetree/bindings/spi/atmel-quadspi.txt
> +++ b/Documentation/devicetree/bindings/spi/atmel-quadspi.txt
> @@ -1,15 +1,19 @@
>  * Atmel Quad Serial Peripheral Interface (QSPI)
>  
>  Required properties:
> -- compatible:     Should be "atmel,sama5d2-qspi".
> +- compatible:     Should be one of the following:
> +		  - "atmel,sama5d2-qspi"
> +		  - "microchip,sam9x60-qspi"
>  - reg:            Should contain the locations and lengths of the base registers
>                    and the mapped memory.
>  - reg-names:      Should contain the resource reg names:
>                    - qspi_base: configuration register address space
>                    - qspi_mmap: memory mapped address space
>  - interrupts:     Should contain the interrupt for the device.
> -- clocks:         The phandle of the clock needed by the QSPI controller.
> -- clock-names:    Should contain "pclk" for the peripheral clock.
> +- clocks:         Should reference the peripheral clock and the QSPI system
> +                  clock if available.
> +- clock-names:    Should contain "pclk" for the peripheral clock and "qspick"
> +                  for the system clock when available.
>  - #address-cells: Should be <1>.
>  - #size-cells:    Should be <0>.
>  


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 13/13] spi: atmel-quadspi: add support for sam9x60 qspi controller
  2019-02-02  4:07 ` [PATCH v3 13/13] spi: atmel-quadspi: add support for sam9x60 qspi controller Tudor.Ambarus
@ 2019-02-02  7:29   ` Boris Brezillon
  2019-02-02  8:58     ` Tudor.Ambarus
  0 siblings, 1 reply; 31+ messages in thread
From: Boris Brezillon @ 2019-02-02  7:29 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: mark.rutland, devicetree, alexandre.belloni, linux-kernel,
	Cyrille.Pitchen, Nicolas.Ferre, robh+dt, linux-spi,
	Ludovic.Desroches, broonie, linux-mtd, bugalski.piotr,
	linux-arm-kernel

On Sat, 2 Feb 2019 04:07:46 +0000
<Tudor.Ambarus@microchip.com> wrote:

> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> The sam9x60 qspi controller uses 2 clocks, one for the peripheral register
> access, the other for the qspi core and phy. Both are mandatory. It uses
> different transfer type bits in IFR register. It has dedicated registers
> to specify a read or a write instruction: Read Instruction Code Register
> (RICR) and Write Instruction Code Register (WICR). ICR/RICR/WICR have
> identical fields.
> 
> Tested with sst26vf064b jedec,spi-nor flash. Backward compatibility test
> done on sama5d2 qspi controller and mx25l25635e jedec,spi-nor flash.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
> v3:
> - reorganize the code and change ops functions pointers to avoid code
>   duplication. From the IP perspective, the transfer type bits are
>   different, and what registers are written: ricr/wicr instead of icr.
> - treat just regular spi transfers. Mem transfers will be added together
>   with dirmap support.
> v2:
> - rework clock handling
> - reorder setting of register values in set_cfg() calls -> move functions
>   that can fail in the upper part of the function body.
> 
>  drivers/spi/atmel-quadspi.c | 295 +++++++++++++++++++++++++++++++++++---------
>  1 file changed, 234 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
> index c9548942535a..af1e4e25097a 100644
> --- a/drivers/spi/atmel-quadspi.c
> +++ b/drivers/spi/atmel-quadspi.c
> @@ -19,6 +19,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/spi/spi-mem.h>
>  
> @@ -35,7 +36,9 @@
>  
>  #define QSPI_IAR     0x0030  /* Instruction Address Register */
>  #define QSPI_ICR     0x0034  /* Instruction Code Register */
> +#define QSPI_WICR    0x0034  /* Write Instruction Code Register */
>  #define QSPI_IFR     0x0038  /* Instruction Frame Register */
> +#define QSPI_RICR    0x003C  /* Read Instruction Code Register */
>  
>  #define QSPI_SMR     0x0040  /* Scrambling Mode Register */
>  #define QSPI_SKR     0x0044  /* Scrambling Key Register */
> @@ -88,7 +91,7 @@
>  #define QSPI_SCR_DLYBS_MASK             GENMASK(23, 16)
>  #define QSPI_SCR_DLYBS(n)               (((n) << 16) & QSPI_SCR_DLYBS_MASK)
>  
> -/* Bitfields in QSPI_ICR (Instruction Code Register) */
> +/* Bitfields in QSPI_ICR (Read/Write Instruction Code Register) */
>  #define QSPI_ICR_INST_MASK              GENMASK(7, 0)
>  #define QSPI_ICR_INST(inst)             (((inst) << 0) & QSPI_ICR_INST_MASK)
>  #define QSPI_ICR_OPT_MASK               GENMASK(23, 16)
> @@ -117,6 +120,7 @@
>  #define QSPI_IFR_CRM                    BIT(14)
>  #define QSPI_IFR_NBDUM_MASK             GENMASK(20, 16)
>  #define QSPI_IFR_NBDUM(n)               (((n) << 16) & QSPI_IFR_NBDUM_MASK)
> +#define QSPI_IFR_APBTFRTYP_READ		BIT(24)

Maybe add a comment saying it's only available on SAM9X60 or prefix it
with SAM9X60.

>  
>  /* Bitfields in QSPI_SMR (Scrambling Mode Register) */
>  #define QSPI_SMR_SCREN                  BIT(0)
> @@ -133,16 +137,39 @@
>  #define QSPI_WPSR_WPVSRC(src)           (((src) << 8) & QSPI_WPSR_WPVSRC)
>  
>  
> +/* Describes register values. */
> +struct atmel_qspi_cfg {
> +	u32 icr;
> +	u32 iar;
> +	u32 ifr;
> +};
> +
> +struct atmel_qspi_caps;
> +
>  struct atmel_qspi {
>  	void __iomem		*regs;
>  	void __iomem		*mem;
>  	struct clk		*pclk;
> +	struct clk		*qspick;
>  	struct platform_device	*pdev;
> +	const struct atmel_qspi_caps *caps;
>  	u32			pending;
>  	u32			smm;
>  	struct completion	cmd_completion;
>  };
>  
> +struct atmel_qspi_ops {
> +	void (*set_tfrtyp)(const struct spi_mem_op *op,
> +			   struct atmel_qspi_cfg *cfg);
> +	void (*write_regs)(void __iomem *base, const struct spi_mem_op *op,
> +			   const struct atmel_qspi_cfg *cfg);
> +};
> +
> +struct atmel_qspi_caps {
> +	const struct atmel_qspi_ops *ops;
> +	bool has_qspick;
> +};
> +
>  struct atmel_qspi_mode {
>  	u8 cmd_buswidth;
>  	u8 addr_buswidth;
> @@ -200,30 +227,36 @@ static bool atmel_qspi_supports_op(struct spi_mem *mem,
>  	return true;
>  }
>  
> -static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
> +static int atmel_qspi_set_mode(struct atmel_qspi_cfg *cfg,
> +			       const struct spi_mem_op *op)
>  {
> -	struct atmel_qspi *aq = spi_controller_get_devdata(mem->spi->master);
> -	void __iomem *base = aq->regs;
> -	int mode;
> -	u32 dummy_cycles = 0;
> -	u32 iar, icr, ifr, sr;
> -	int err = 0;
> -
> -	iar = 0;
> -	icr = QSPI_ICR_INST(op->cmd.opcode);
> -	ifr = QSPI_IFR_INSTEN;
> +	int mode = atmel_qspi_find_mode(op);
>  
> -	/* Set the QSPI controller in Serial Memory Mode */
> -	if (aq->smm != QSPI_MR_SMM) {
> -		writel_relaxed(QSPI_MR_SMM, base + QSPI_MR);
> -		aq->smm = QSPI_MR_SMM;
> -	}
> -
> -	mode = atmel_qspi_find_mode(op);
>  	if (mode < 0)
>  		return mode;
> +	cfg->ifr = sama5d2_qspi_modes[mode].config;

Looks like modes are the same for sama5d2 and sam9x60, so maybe you
should rename the sama5d2_qspi_modes variable into something more
generic.

> +	return 0;
> +}
>  
> -	ifr |= sama5d2_qspi_modes[mode].config;
> +/*

Kernel doc headers should starts with /**, but maybe you did that on
purpose to avoid having this header parsed in case someone decides to
add this file to the list of files parsed for doc creation.

> + * atmel_qspi_set_address_mode() - set address mode.
> + * @cfg:	contains register values
> + * @op:		describes a SPI memory operation
> + *
> + * The controller allows 24 and 32-bit addressing while NAND-flash requires
> + * 16-bit long. Handling 8-bit long addresses is done using the option field.
> + * For the 16-bit addresses, the workaround depends of the number of requested
> + * dummy bits. If there are 8 or more dummy cycles, the address is shifted and
> + * sent with the first dummy byte. Otherwise opcode is disabled and the first
> + * byte of the address contains the command opcode (works only if the opcode and
> + * address use the same buswidth). The limitation is when the 16-bit address is
> + * used without enough dummy cycles and the opcode is using a different buswidth
> + * than the address.

This comment would better be placed directly in the function just
before the switch() statement.

> + */
> +static int atmel_qspi_set_address_mode(struct atmel_qspi_cfg *cfg,
> +				       const struct spi_mem_op *op)
> +{
> +	u32 dummy_cycles = 0;
>  
>  	if (op->dummy.buswidth && op->dummy.nbytes)
>  		dummy_cycles = op->dummy.nbytes * 8 / op->dummy.buswidth;
> @@ -233,28 +266,28 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  		case 0:
>  			break;
>  		case 1:
> -			ifr |= QSPI_IFR_OPTEN | QSPI_IFR_OPTL_8BIT;
> -			icr |= QSPI_ICR_OPT(op->addr.val & 0xff);
> +			cfg->ifr |= QSPI_IFR_OPTEN | QSPI_IFR_OPTL_8BIT;
> +			cfg->icr = QSPI_ICR_OPT(op->addr.val & 0xff);
>  			break;
>  		case 2:
>  			if (dummy_cycles < 8 / op->addr.buswidth) {
> -				ifr &= ~QSPI_IFR_INSTEN;
> -				ifr |= QSPI_IFR_ADDREN;
> -				iar = (op->cmd.opcode << 16) |
> -					(op->addr.val & 0xffff);
> +				cfg->ifr &= ~QSPI_IFR_INSTEN;
> +				cfg->ifr |= QSPI_IFR_ADDREN;
> +				cfg->iar = (op->cmd.opcode << 16) |
> +					   (op->addr.val & 0xffff);
>  			} else {
> -				ifr |= QSPI_IFR_ADDREN;
> -				iar = (op->addr.val << 8) & 0xffffff;
> +				cfg->ifr |= QSPI_IFR_ADDREN;
> +				cfg->iar = (op->addr.val << 8) & 0xffffff;
>  				dummy_cycles -= 8 / op->addr.buswidth;
>  			}
>  			break;
>  		case 3:
> -			ifr |= QSPI_IFR_ADDREN;
> -			iar = op->addr.val & 0xffffff;
> +			cfg->ifr |= QSPI_IFR_ADDREN;
> +			cfg->iar = op->addr.val & 0xffffff;
>  			break;
>  		case 4:
> -			ifr |= QSPI_IFR_ADDREN | QSPI_IFR_ADDRL;
> -			iar = op->addr.val & 0x7ffffff;
> +			cfg->ifr |= QSPI_IFR_ADDREN | QSPI_IFR_ADDRL;
> +			cfg->iar = op->addr.val & 0x7ffffff;
>  			break;
>  		default:
>  			return -ENOTSUPP;
> @@ -263,22 +296,99 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  
>  	/* Set number of dummy cycles */
>  	if (dummy_cycles)
> -		ifr |= QSPI_IFR_NBDUM(dummy_cycles);
> +		cfg->ifr |= QSPI_IFR_NBDUM(dummy_cycles);
>  
> -	/* Set data enable */
> -	if (op->data.nbytes)
> -		ifr |= QSPI_IFR_DATAEN;
> +	return 0;
> +}
>  
> +static void atmel_qspi_sama5d2_set_tfrtyp(const struct spi_mem_op *op,
> +					  struct atmel_qspi_cfg *cfg)
> +{
>  	if (op->data.dir == SPI_MEM_DATA_OUT)
> -		ifr |= QSPI_IFR_SAMA5D2_WRITE_TRSFR;
> +		cfg->ifr |= QSPI_IFR_SAMA5D2_WRITE_TRSFR;
> +}
>  
> +static void atmel_qspi_sama5d2_write_regs(void __iomem *base,
> +					  const struct spi_mem_op *op,
> +					  const struct atmel_qspi_cfg *cfg)
> +{
>  	/* Clear pending interrupts */
>  	(void)readl_relaxed(base + QSPI_SR);
>  
>  	/* Set QSPI Instruction Frame registers */
> -	writel_relaxed(iar, base + QSPI_IAR);
> -	writel_relaxed(icr, base + QSPI_ICR);
> -	writel_relaxed(ifr, base + QSPI_IFR);
> +	writel_relaxed(cfg->iar, base + QSPI_IAR);
> +	writel_relaxed(cfg->icr, base + QSPI_ICR);
> +	writel_relaxed(cfg->ifr, base + QSPI_IFR);
> +}
> +
> +static void atmel_qspi_sam9x60_set_tfrtyp(const struct spi_mem_op *op,
> +					  struct atmel_qspi_cfg *cfg)
> +{
> +	if (!op->addr.nbytes && op->data.dir == SPI_MEM_DATA_IN)
> +		cfg->ifr |= QSPI_IFR_APBTFRTYP_READ;
> +}
> +
> +static void atmel_qspi_sam9x60_write_regs(void __iomem *base,
> +					  const struct spi_mem_op *op,
> +					  const struct atmel_qspi_cfg *cfg)
> +{
> +	/* Clear pending interrupts */
> +	(void)readl_relaxed(base + QSPI_SR);
> +
> +	/* Set QSPI Instruction Frame registers */
> +	writel_relaxed(cfg->iar, base + QSPI_IAR);
> +	if (op->data.dir == SPI_MEM_DATA_IN)
> +		writel_relaxed(cfg->icr, base + QSPI_RICR);
> +	else
> +		writel_relaxed(cfg->icr, base + QSPI_ICR);
> +	writel_relaxed(cfg->ifr, base + QSPI_IFR);
> +}
> +
> +static int atmel_qspi_set_cfg(struct atmel_qspi *aq,
> +			      const struct spi_mem_op *op,
> +			      struct atmel_qspi_cfg *cfg)
> +{
> +	void __iomem *base = aq->regs;
> +	int ret;
> +
> +	/* Set the QSPI controller in Serial Memory Mode */
> +	if (aq->smm != QSPI_MR_SMM) {
> +		writel_relaxed(QSPI_MR_SMM, base + QSPI_MR);

					    aq->reqs +

and you can get rid of base.

> +		aq->smm = QSPI_MR_SMM;
> +	}
> +
> +	ret = atmel_qspi_set_mode(cfg, op);
> +	if (ret)
> +		return ret;
> +
> +	ret = atmel_qspi_set_address_mode(cfg, op);
> +	if (ret)
> +		return ret;
> +
> +	cfg->ifr |= QSPI_IFR_INSTEN;
> +	cfg->icr |= QSPI_ICR_INST(op->cmd.opcode);
> +
> +	/* Set data enable */
> +	if (op->data.nbytes)
> +		cfg->ifr |= QSPI_IFR_DATAEN;
> +
> +	aq->caps->ops->set_tfrtyp(op, cfg);
> +	aq->caps->ops->write_regs(base, op, cfg);
> +
> +	return 0;
> +}
> +
> +static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
> +{
> +	struct atmel_qspi *aq = spi_controller_get_devdata(mem->spi->master);
> +	void __iomem *base = aq->regs;
> +	struct atmel_qspi_cfg cfg = {0};
> +	u32 sr;
> +	int err;
> +
> +	err = atmel_qspi_set_cfg(aq, op, &cfg);
> +	if (err)
> +		return err;
>  
>  	/* Skip to the final steps if there is no data */
>  	if (op->data.nbytes) {
> @@ -287,11 +397,11 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  
>  		/* Send/Receive data */
>  		if (op->data.dir == SPI_MEM_DATA_IN)
> -			_memcpy_fromio(op->data.buf.in,
> -				aq->mem + iar, op->data.nbytes);
> +			_memcpy_fromio(op->data.buf.in, aq->mem + cfg.iar,
> +				       op->data.nbytes);
>  		else
> -			_memcpy_toio(aq->mem + iar,
> -				op->data.buf.out, op->data.nbytes);
> +			_memcpy_toio(aq->mem + cfg.iar, op->data.buf.out,
> +				     op->data.nbytes);
>  
>  		/* Release the chip-select */
>  		writel_relaxed(QSPI_CR_LASTXFER, base + QSPI_CR);
> @@ -391,9 +501,22 @@ static int atmel_qspi_probe(struct platform_device *pdev)
>  	struct spi_controller *ctrl;
>  	struct atmel_qspi *aq;
>  	struct resource *res;
> +	const struct atmel_qspi_caps *caps;
> +	struct device *dev = &pdev->dev;

If you really want to do this &pdev->dev -> dev conversion (which I
don't think is necessary given that we don't have over 80 chars lines),
please do it in separate patch.

>  	int irq, err = 0;
>  
> -	ctrl = spi_alloc_master(&pdev->dev, sizeof(*aq));
> +	caps = of_device_get_match_data(dev);
> +	if (!caps) {
> +		dev_err(dev, "Could not retrieve QSPI caps\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!caps->ops->set_tfrtyp || !caps->ops->write_regs) {
> +		dev_err(dev, "Could not retrieve QSPI ops\n");
> +		return -EINVAL;
> +	}
> +
> +	ctrl = spi_alloc_master(dev, sizeof(*aq));
>  	if (!ctrl)
>  		return -ENOMEM;
>  
> @@ -409,32 +532,33 @@ static int atmel_qspi_probe(struct platform_device *pdev)
>  
>  	init_completion(&aq->cmd_completion);
>  	aq->pdev = pdev;
> +	aq->caps = caps;
>  
>  	/* Map the registers */
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "qspi_base");
> -	aq->regs = devm_ioremap_resource(&pdev->dev, res);
> +	aq->regs = devm_ioremap_resource(dev, res);
>  	if (IS_ERR(aq->regs)) {
> -		dev_err(&pdev->dev, "missing registers\n");
> +		dev_err(dev, "missing registers\n");
>  		err = PTR_ERR(aq->regs);
>  		goto exit;
>  	}
>  
>  	/* Map the AHB memory */
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "qspi_mmap");
> -	aq->mem = devm_ioremap_resource(&pdev->dev, res);
> +	aq->mem = devm_ioremap_resource(dev, res);
>  	if (IS_ERR(aq->mem)) {
> -		dev_err(&pdev->dev, "missing AHB memory\n");
> +		dev_err(dev, "missing AHB memory\n");
>  		err = PTR_ERR(aq->mem);
>  		goto exit;
>  	}
>  
>  	/* Get the peripheral clock */
> -	aq->pclk = devm_clk_get(&pdev->dev, "pclk");
> +	aq->pclk = devm_clk_get(dev, "pclk");
>  	if (IS_ERR(aq->pclk))
> -		aq->pclk = devm_clk_get(&pdev->dev, NULL);
> +		aq->pclk = devm_clk_get(dev, NULL);
>  
>  	if (IS_ERR(aq->pclk)) {
> -		dev_err(&pdev->dev, "missing peripheral clock\n");
> +		dev_err(dev, "missing peripheral clock\n");
>  		err = PTR_ERR(aq->pclk);
>  		goto exit;
>  	}
> @@ -442,32 +566,52 @@ static int atmel_qspi_probe(struct platform_device *pdev)
>  	/* Enable the peripheral clock */
>  	err = clk_prepare_enable(aq->pclk);
>  	if (err) {
> -		dev_err(&pdev->dev, "failed to enable the peripheral clock\n");
> +		dev_err(dev, "failed to enable the peripheral clock\n");
>  		goto exit;
>  	}
>  
> +	if (caps->has_qspick) {
> +		/* Get the QSPI system clock */
> +		aq->qspick = devm_clk_get(dev, "qspick");
> +		if (IS_ERR(aq->qspick)) {
> +			dev_err(dev, "missing system clock\n");
> +			err = PTR_ERR(aq->qspick);
> +			goto disable_pclk;
> +		}
> +
> +		/* Enable the QSPI system clock */
> +		err = clk_prepare_enable(aq->qspick);
> +		if (err) {
> +			dev_err(dev,
> +				"failed to enable the QSPI system clock\n");
> +			goto disable_pclk;
> +		}
> +	}
> +
>  	/* Request the IRQ */
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0) {
> -		dev_err(&pdev->dev, "missing IRQ\n");
> +		dev_err(dev, "missing IRQ\n");
>  		err = irq;
> -		goto disable_pclk;
> +		goto disable_qspick;
>  	}
> -	err = devm_request_irq(&pdev->dev, irq, atmel_qspi_interrupt,
> -			       0, dev_name(&pdev->dev), aq);
> +	err = devm_request_irq(dev, irq, atmel_qspi_interrupt, 0,
> +			       dev_name(dev), aq);
>  	if (err)
> -		goto disable_pclk;
> +		goto disable_qspick;
>  
>  	err = atmel_qspi_init(aq);
>  	if (err)
> -		goto disable_pclk;
> +		goto disable_qspick;
>  
>  	err = spi_register_controller(ctrl);
>  	if (err)
> -		goto disable_pclk;
> +		goto disable_qspick;
>  
>  	return 0;
>  
> +disable_qspick:
> +	clk_disable_unprepare(aq->qspick);
>  disable_pclk:
>  	clk_disable_unprepare(aq->pclk);
>  exit:
> @@ -483,6 +627,7 @@ static int atmel_qspi_remove(struct platform_device *pdev)
>  
>  	spi_unregister_controller(ctrl);
>  	writel_relaxed(QSPI_CR_QSPIDIS, aq->regs + QSPI_CR);
> +	clk_disable_unprepare(aq->qspick);
>  	clk_disable_unprepare(aq->pclk);
>  	return 0;
>  }
> @@ -491,6 +636,7 @@ static int __maybe_unused atmel_qspi_suspend(struct device *dev)
>  {
>  	struct atmel_qspi *aq = dev_get_drvdata(dev);
>  
> +	clk_disable_unprepare(aq->qspick);
>  	clk_disable_unprepare(aq->pclk);
>  
>  	return 0;
> @@ -501,6 +647,7 @@ static int __maybe_unused atmel_qspi_resume(struct device *dev)
>  	struct atmel_qspi *aq = dev_get_drvdata(dev);
>  
>  	clk_prepare_enable(aq->pclk);
> +	clk_prepare_enable(aq->qspick);
>  
>  	return atmel_qspi_init(aq);
>  }
> @@ -508,8 +655,34 @@ static int __maybe_unused atmel_qspi_resume(struct device *dev)
>  static SIMPLE_DEV_PM_OPS(atmel_qspi_pm_ops, atmel_qspi_suspend,
>  			 atmel_qspi_resume);
>  
> +static const struct atmel_qspi_ops atmel_sama5d2_qspi_ops = {
> +	.set_tfrtyp = atmel_qspi_sama5d2_set_tfrtyp,
> +	.write_regs = atmel_qspi_sama5d2_write_regs,
> +};
> +
> +static const struct atmel_qspi_caps atmel_sama5d2_qspi_caps = {
> +	.ops = &atmel_sama5d2_qspi_ops,
> +};
> +
> +static const struct atmel_qspi_ops atmel_sam9x60_qspi_ops = {
> +	.set_tfrtyp = atmel_qspi_sam9x60_set_tfrtyp,
> +	.write_regs = atmel_qspi_sam9x60_write_regs,
> +};
> +
> +static const struct atmel_qspi_caps atmel_sam9x60_qspi_caps = {
> +	.ops = &atmel_sam9x60_qspi_ops,
> +	.has_qspick = true,
> +};
> +
>  static const struct of_device_id atmel_qspi_dt_ids[] = {
> -	{ .compatible = "atmel,sama5d2-qspi" },
> +	{
> +		.compatible = "atmel,sama5d2-qspi",
> +		.data = &atmel_sama5d2_qspi_caps,
> +	},
> +	{
> +		.compatible = "microchip,sam9x60-qspi",
> +		.data = &atmel_sam9x60_qspi_caps,
> +	},
>  	{ /* sentinel */ }
>  };
>  


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 01/13] spi: atmel-quadspi: cache MR value to avoid a write access
  2019-02-02  7:06   ` Boris Brezillon
@ 2019-02-02  8:38     ` Tudor.Ambarus
  2019-02-02 13:20       ` Boris Brezillon
  0 siblings, 1 reply; 31+ messages in thread
From: Tudor.Ambarus @ 2019-02-02  8:38 UTC (permalink / raw)
  To: bbrezillon
  Cc: mark.rutland, devicetree, alexandre.belloni, broonie,
	linux-kernel, robh+dt, linux-spi, Ludovic.Desroches,
	Cyrille.Pitchen, linux-mtd, bugalski.piotr, linux-arm-kernel



On 02/02/2019 09:06 AM, Boris Brezillon wrote:
> On Sat, 2 Feb 2019 04:07:13 +0000
> <Tudor.Ambarus@microchip.com> wrote:
> 
>> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>>
>> Cache Serial Memory Mode (SMM) value to avoid write access when
>> setting the controller in serial memory mode. SMM is set in
>> exec_op() and not at probe time, to let room for future regular
>> SPI support.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>> v3: update smm value when different. rename mr/smm
>> v2: cache MR value instead of moving the write access at probe
>>
>>  drivers/spi/atmel-quadspi.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
>> index ddc712410812..645284c6ec9a 100644
>> --- a/drivers/spi/atmel-quadspi.c
>> +++ b/drivers/spi/atmel-quadspi.c
>> @@ -155,6 +155,7 @@ struct atmel_qspi {
>>  	struct clk		*clk;
>>  	struct platform_device	*pdev;
>>  	u32			pending;
>> +	u32			smm;
>>  	struct completion	cmd_completion;
>>  };
>>  
>> @@ -238,7 +239,11 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>>  	icr = QSPI_ICR_INST(op->cmd.opcode);
>>  	ifr = QSPI_IFR_INSTEN;
>>  
>> -	qspi_writel(aq, QSPI_MR, QSPI_MR_SMM);
>> +	/* Set the QSPI controller in Serial Memory Mode */
>> +	if (aq->smm != QSPI_MR_SMM) {
> 
> Sorry, I think I misunderstood your previous suggestion, I thought the
> reg was called SMM. If the reg is called MR and the value you expect in
> there is SMM, then the field should be named ->mr as it caches the
> whole reg, not only the SMM bit. So it's actually:
> 
> 	if (aq->mr != QSPI_MR_SMM) {

No worries. When keeping the reg name, and not the bit itself, I would expect to
do the check as in v2, to let room for checking other bits too:

+	if (!(aq->mr & QSPI_MR_SMM))

I don't have any problems to keep "mr" name, but I would like to understand your
reasons.

Thanks,
ta

> 
>> +		qspi_writel(aq, QSPI_MR, QSPI_MR_SMM);
>> +		aq->smm = QSPI_MR_SMM;
>> +	}
>>  
>>  	mode = find_mode(op);
>>  	if (mode < 0)
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 03/13] spi: atmel-quadspi: drop wrappers for iomem accesses
  2019-02-02  7:11   ` Boris Brezillon
@ 2019-02-02  8:44     ` Tudor.Ambarus
  2019-02-02 13:23       ` Boris Brezillon
  0 siblings, 1 reply; 31+ messages in thread
From: Tudor.Ambarus @ 2019-02-02  8:44 UTC (permalink / raw)
  To: bbrezillon
  Cc: mark.rutland, devicetree, alexandre.belloni, broonie,
	linux-kernel, robh+dt, Nicolas.Ferre, Ludovic.Desroches,
	Cyrille.Pitchen, linux-mtd, bugalski.piotr, linux-spi,
	linux-arm-kernel



On 02/02/2019 09:11 AM, Boris Brezillon wrote:
> On Sat, 2 Feb 2019 04:07:19 +0000
> <Tudor.Ambarus@microchip.com> wrote:
> 
>> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>>
>> The wrappers hid that the accesses are relaxed. Drop them.
>>
>> Suggested-by: Boris Brezillon <bbrezillon@kernel.org>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>> v3: no change
>> v2: new patch
>>
>>  drivers/spi/atmel-quadspi.c | 47 +++++++++++++++++++--------------------------
>>  1 file changed, 20 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
>> index feeddcb25e1f..131374db0db4 100644
>> --- a/drivers/spi/atmel-quadspi.c
>> +++ b/drivers/spi/atmel-quadspi.c
>> @@ -175,17 +175,6 @@ static const struct qspi_mode sama5d2_qspi_modes[] = {
>>  	{ 4, 4, 4, QSPI_IFR_WIDTH_QUAD_CMD },
>>  };
>>  
>> -/* Register access functions */
>> -static inline u32 qspi_readl(struct atmel_qspi *aq, u32 reg)
>> -{
>> -	return readl_relaxed(aq->regs + reg);
>> -}
>> -
>> -static inline void qspi_writel(struct atmel_qspi *aq, u32 reg, u32 value)
>> -{
>> -	writel_relaxed(value, aq->regs + reg);
>> -}
>> -
>>  static inline bool is_compatible(const struct spi_mem_op *op,
>>  				 const struct qspi_mode *mode)
>>  {
>> @@ -229,6 +218,7 @@ static bool atmel_qspi_supports_op(struct spi_mem *mem,
>>  static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>>  {
>>  	struct atmel_qspi *aq = spi_controller_get_devdata(mem->spi->master);
>> +	void __iomem *base = aq->regs;
> 
> Can we name this variable regs instead of base or even get rid of it
> and dereference aq->regs in the xxx_relaxed() calls (doesn't look like
> the lines would be over 80 chars even when doing that). With this
> addressed, you can add:
> 
> Reviewed-by: Boris Brezillon <bbrezillon@kernel.org>

I chose to introduce the "base" variable when I have at least 2 dereferences in
a function, as an optimization. In exec_op() for example, there are 6
dereferences of aq->reqs. Why do you prefer keeping aq->regs?
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 08/13] spi: atmel-quadspi: drop unused and NOP transfer macros
  2019-02-02  7:13   ` Boris Brezillon
@ 2019-02-02  8:46     ` Tudor.Ambarus
  2019-02-02 13:27       ` Boris Brezillon
  0 siblings, 1 reply; 31+ messages in thread
From: Tudor.Ambarus @ 2019-02-02  8:46 UTC (permalink / raw)
  To: bbrezillon
  Cc: mark.rutland, devicetree, alexandre.belloni, linux-kernel,
	Cyrille.Pitchen, Nicolas.Ferre, robh+dt, linux-spi,
	Ludovic.Desroches, broonie, linux-mtd, bugalski.piotr,
	linux-arm-kernel



On 02/02/2019 09:13 AM, Boris Brezillon wrote:
> On Sat, 2 Feb 2019 04:07:33 +0000
> <Tudor.Ambarus@microchip.com> wrote:
> 
>> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>>
>> Remove NOP when setting read transfer type. Remove useless
>> setting of write transfer type when
>> op->data.dir == SPI_MEM_DATA_IN && !op->data.nbytes.
>>
>> QSPI_IFR_TFRTYP_TRSFR_WRITE is specific just to sama5d2 qspi,
>> rename it to QSPI_IFR_SAMA5D2_WRITE_TRSFR.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>> v3: new patch
>>
>>  drivers/spi/atmel-quadspi.c | 12 +++---------
>>  1 file changed, 3 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
>> index d26d4cd0e36b..bfa5f5e92d96 100644
>> --- a/drivers/spi/atmel-quadspi.c
>> +++ b/drivers/spi/atmel-quadspi.c
>> @@ -113,11 +113,7 @@
>>  #define QSPI_IFR_OPTL_4BIT              (2 << 8)
>>  #define QSPI_IFR_OPTL_8BIT              (3 << 8)
>>  #define QSPI_IFR_ADDRL                  BIT(10)
>> -#define QSPI_IFR_TFRTYP_MASK            GENMASK(13, 12)
>> -#define QSPI_IFR_TFRTYP_TRSFR_READ      (0 << 12)
>> -#define QSPI_IFR_TFRTYP_TRSFR_READ_MEM  (1 << 12)
>> -#define QSPI_IFR_TFRTYP_TRSFR_WRITE     (2 << 12)
>> -#define QSPI_IFR_TFRTYP_TRSFR_WRITE_MEM (3 << 13)
>> +#define QSPI_IFR_SAMA5D2_WRITE_TRSFR	BIT(13)
> 
> Can you define QSPI_IFR_TFRTYP_MEM (bit 12) even if it's not used yet?

Shouldn't be introduced with the dir map support?
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 10/13] dt-bindings: spi: atmel-quadspi: make "pclk" mandatory
  2019-02-02  7:15   ` Boris Brezillon
@ 2019-02-02  8:47     ` Tudor.Ambarus
  0 siblings, 0 replies; 31+ messages in thread
From: Tudor.Ambarus @ 2019-02-02  8:47 UTC (permalink / raw)
  To: bbrezillon
  Cc: mark.rutland, devicetree, alexandre.belloni, broonie,
	linux-kernel, robh+dt, Nicolas.Ferre, Ludovic.Desroches,
	Cyrille.Pitchen, linux-mtd, bugalski.piotr, linux-spi,
	linux-arm-kernel



On 02/02/2019 09:15 AM, Boris Brezillon wrote:
> On Sat, 2 Feb 2019 04:07:39 +0000
> <Tudor.Ambarus@microchip.com> wrote:
> 
>> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>>
>> Naming clocks is a good practice. Make "pclk" madatory even if
>> we support unnamed clock in the driver, to be backward compatible
>> with old DTs.
>>
>> Suggested-by: Boris Brezillon <bbrezillon@kernel.org>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>> v3: new patch
>>
>>  Documentation/devicetree/bindings/spi/atmel-quadspi.txt | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/spi/atmel-quadspi.txt b/Documentation/devicetree/bindings/spi/atmel-quadspi.txt
>> index e9dae6264d89..1de54e87f5d6 100644
>> --- a/Documentation/devicetree/bindings/spi/atmel-quadspi.txt
>> +++ b/Documentation/devicetree/bindings/spi/atmel-quadspi.txt
>> @@ -9,6 +9,7 @@ Required properties:
>>                    - qspi_mmap: memory mapped address space
>>  - interrupts:     Should contain the interrupt for the device.
>>  - clocks:         The phandle of the clock needed by the QSPI controller.
>> +- clock-names:    Should contain "pclk" for the peripheral clock.
>>  - #address-cells: Should be <1>.
>>  - #size-cells:    Should be <0>.
>>  
>> @@ -20,6 +21,7 @@ spi@f0020000 {
>>  	reg-names = "qspi_base", "qspi_mmap";
>>  	interrupts = <52 IRQ_TYPE_LEVEL_HIGH 7>;
>>  	clocks = <&pmc PMC_TYPE_PERIPHERAL 52>;
>> +	clock-names = "pclk"
> 
> Missing semicolon. Once fixed you can add
> 
> Reviewed-by: Boris Brezillon <bbrezillon@kernel.org>

will do, thanks!

> 
>>  	#address-cells = <1>;
>>  	#size-cells = <0>;
>>  	pinctrl-names = "default";
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 13/13] spi: atmel-quadspi: add support for sam9x60 qspi controller
  2019-02-02  7:29   ` Boris Brezillon
@ 2019-02-02  8:58     ` Tudor.Ambarus
  2019-02-02 13:30       ` Boris Brezillon
  0 siblings, 1 reply; 31+ messages in thread
From: Tudor.Ambarus @ 2019-02-02  8:58 UTC (permalink / raw)
  To: bbrezillon
  Cc: mark.rutland, devicetree, alexandre.belloni, broonie,
	linux-kernel, robh+dt, Nicolas.Ferre, Ludovic.Desroches,
	Cyrille.Pitchen, linux-mtd, bugalski.piotr, linux-spi,
	linux-arm-kernel



On 02/02/2019 09:29 AM, Boris Brezillon wrote:
> On Sat, 2 Feb 2019 04:07:46 +0000
> <Tudor.Ambarus@microchip.com> wrote:
> 
>> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>>
>> The sam9x60 qspi controller uses 2 clocks, one for the peripheral register
>> access, the other for the qspi core and phy. Both are mandatory. It uses
>> different transfer type bits in IFR register. It has dedicated registers
>> to specify a read or a write instruction: Read Instruction Code Register
>> (RICR) and Write Instruction Code Register (WICR). ICR/RICR/WICR have
>> identical fields.
>>
>> Tested with sst26vf064b jedec,spi-nor flash. Backward compatibility test
>> done on sama5d2 qspi controller and mx25l25635e jedec,spi-nor flash.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>> v3:
>> - reorganize the code and change ops functions pointers to avoid code
>>   duplication. From the IP perspective, the transfer type bits are
>>   different, and what registers are written: ricr/wicr instead of icr.
>> - treat just regular spi transfers. Mem transfers will be added together
>>   with dirmap support.
>> v2:
>> - rework clock handling
>> - reorder setting of register values in set_cfg() calls -> move functions
>>   that can fail in the upper part of the function body.
>>
>>  drivers/spi/atmel-quadspi.c | 295 +++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 234 insertions(+), 61 deletions(-)
>>
>> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
>> index c9548942535a..af1e4e25097a 100644
>> --- a/drivers/spi/atmel-quadspi.c
>> +++ b/drivers/spi/atmel-quadspi.c
>> @@ -19,6 +19,7 @@
>>  #include <linux/kernel.h>
>>  #include <linux/module.h>
>>  #include <linux/of.h>
>> +#include <linux/of_platform.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/spi/spi-mem.h>
>>  
>> @@ -35,7 +36,9 @@
>>  
>>  #define QSPI_IAR     0x0030  /* Instruction Address Register */
>>  #define QSPI_ICR     0x0034  /* Instruction Code Register */
>> +#define QSPI_WICR    0x0034  /* Write Instruction Code Register */
>>  #define QSPI_IFR     0x0038  /* Instruction Frame Register */
>> +#define QSPI_RICR    0x003C  /* Read Instruction Code Register */
>>  
>>  #define QSPI_SMR     0x0040  /* Scrambling Mode Register */
>>  #define QSPI_SKR     0x0044  /* Scrambling Key Register */
>> @@ -88,7 +91,7 @@
>>  #define QSPI_SCR_DLYBS_MASK             GENMASK(23, 16)
>>  #define QSPI_SCR_DLYBS(n)               (((n) << 16) & QSPI_SCR_DLYBS_MASK)
>>  
>> -/* Bitfields in QSPI_ICR (Instruction Code Register) */
>> +/* Bitfields in QSPI_ICR (Read/Write Instruction Code Register) */
>>  #define QSPI_ICR_INST_MASK              GENMASK(7, 0)
>>  #define QSPI_ICR_INST(inst)             (((inst) << 0) & QSPI_ICR_INST_MASK)
>>  #define QSPI_ICR_OPT_MASK               GENMASK(23, 16)
>> @@ -117,6 +120,7 @@
>>  #define QSPI_IFR_CRM                    BIT(14)
>>  #define QSPI_IFR_NBDUM_MASK             GENMASK(20, 16)
>>  #define QSPI_IFR_NBDUM(n)               (((n) << 16) & QSPI_IFR_NBDUM_MASK)
>> +#define QSPI_IFR_APBTFRTYP_READ		BIT(24)
> 
> Maybe add a comment saying it's only available on SAM9X60 or prefix it
> with SAM9X60.

I'll add a comment. The macro name is more generic how it is now and can be used
by future versions of the IP. Hypothetically speaking, if we rename it to
QSPI_IFR_SAM9x60_TFSFR_READ and other sam9x will come out, then I'll have to
rename this macro again, to make it more generic.	

> 
>>  
>>  /* Bitfields in QSPI_SMR (Scrambling Mode Register) */
>>  #define QSPI_SMR_SCREN                  BIT(0)
>> @@ -133,16 +137,39 @@
>>  #define QSPI_WPSR_WPVSRC(src)           (((src) << 8) & QSPI_WPSR_WPVSRC)
>>  
>>  
>> +/* Describes register values. */
>> +struct atmel_qspi_cfg {
>> +	u32 icr;
>> +	u32 iar;
>> +	u32 ifr;
>> +};
>> +
>> +struct atmel_qspi_caps;
>> +
>>  struct atmel_qspi {
>>  	void __iomem		*regs;
>>  	void __iomem		*mem;
>>  	struct clk		*pclk;
>> +	struct clk		*qspick;
>>  	struct platform_device	*pdev;
>> +	const struct atmel_qspi_caps *caps;
>>  	u32			pending;
>>  	u32			smm;
>>  	struct completion	cmd_completion;
>>  };
>>  
>> +struct atmel_qspi_ops {
>> +	void (*set_tfrtyp)(const struct spi_mem_op *op,
>> +			   struct atmel_qspi_cfg *cfg);
>> +	void (*write_regs)(void __iomem *base, const struct spi_mem_op *op,
>> +			   const struct atmel_qspi_cfg *cfg);
>> +};
>> +
>> +struct atmel_qspi_caps {
>> +	const struct atmel_qspi_ops *ops;
>> +	bool has_qspick;
>> +};
>> +
>>  struct atmel_qspi_mode {
>>  	u8 cmd_buswidth;
>>  	u8 addr_buswidth;
>> @@ -200,30 +227,36 @@ static bool atmel_qspi_supports_op(struct spi_mem *mem,
>>  	return true;
>>  }
>>  
>> -static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>> +static int atmel_qspi_set_mode(struct atmel_qspi_cfg *cfg,
>> +			       const struct spi_mem_op *op)
>>  {
>> -	struct atmel_qspi *aq = spi_controller_get_devdata(mem->spi->master);
>> -	void __iomem *base = aq->regs;
>> -	int mode;
>> -	u32 dummy_cycles = 0;
>> -	u32 iar, icr, ifr, sr;
>> -	int err = 0;
>> -
>> -	iar = 0;
>> -	icr = QSPI_ICR_INST(op->cmd.opcode);
>> -	ifr = QSPI_IFR_INSTEN;
>> +	int mode = atmel_qspi_find_mode(op);
>>  
>> -	/* Set the QSPI controller in Serial Memory Mode */
>> -	if (aq->smm != QSPI_MR_SMM) {
>> -		writel_relaxed(QSPI_MR_SMM, base + QSPI_MR);
>> -		aq->smm = QSPI_MR_SMM;
>> -	}
>> -
>> -	mode = atmel_qspi_find_mode(op);
>>  	if (mode < 0)
>>  		return mode;
>> +	cfg->ifr = sama5d2_qspi_modes[mode].config;
> 
> Looks like modes are the same for sama5d2 and sam9x60, so maybe you
> should rename the sama5d2_qspi_modes variable into something more
> generic.

will do

> 
>> +	return 0;
>> +}
>>  
>> -	ifr |= sama5d2_qspi_modes[mode].config;
>> +/*
> 
> Kernel doc headers should starts with /**, but maybe you did that on
> purpose to avoid having this header parsed in case someone decides to
> add this file to the list of files parsed for doc creation.

I'll add /**

> 
>> + * atmel_qspi_set_address_mode() - set address mode.
>> + * @cfg:	contains register values
>> + * @op:		describes a SPI memory operation
>> + *
>> + * The controller allows 24 and 32-bit addressing while NAND-flash requires
>> + * 16-bit long. Handling 8-bit long addresses is done using the option field.
>> + * For the 16-bit addresses, the workaround depends of the number of requested
>> + * dummy bits. If there are 8 or more dummy cycles, the address is shifted and
>> + * sent with the first dummy byte. Otherwise opcode is disabled and the first
>> + * byte of the address contains the command opcode (works only if the opcode and
>> + * address use the same buswidth). The limitation is when the 16-bit address is
>> + * used without enough dummy cycles and the opcode is using a different buswidth
>> + * than the address.
> 
> This comment would better be placed directly in the function just
> before the switch() statement.

ok

> 
>> + */
>> +static int atmel_qspi_set_address_mode(struct atmel_qspi_cfg *cfg,
>> +				       const struct spi_mem_op *op)
>> +{
>> +	u32 dummy_cycles = 0;
>>  
>>  	if (op->dummy.buswidth && op->dummy.nbytes)
>>  		dummy_cycles = op->dummy.nbytes * 8 / op->dummy.buswidth;
>> @@ -233,28 +266,28 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>>  		case 0:
>>  			break;
>>  		case 1:
>> -			ifr |= QSPI_IFR_OPTEN | QSPI_IFR_OPTL_8BIT;
>> -			icr |= QSPI_ICR_OPT(op->addr.val & 0xff);
>> +			cfg->ifr |= QSPI_IFR_OPTEN | QSPI_IFR_OPTL_8BIT;
>> +			cfg->icr = QSPI_ICR_OPT(op->addr.val & 0xff);
>>  			break;
>>  		case 2:
>>  			if (dummy_cycles < 8 / op->addr.buswidth) {
>> -				ifr &= ~QSPI_IFR_INSTEN;
>> -				ifr |= QSPI_IFR_ADDREN;
>> -				iar = (op->cmd.opcode << 16) |
>> -					(op->addr.val & 0xffff);
>> +				cfg->ifr &= ~QSPI_IFR_INSTEN;
>> +				cfg->ifr |= QSPI_IFR_ADDREN;
>> +				cfg->iar = (op->cmd.opcode << 16) |
>> +					   (op->addr.val & 0xffff);
>>  			} else {
>> -				ifr |= QSPI_IFR_ADDREN;
>> -				iar = (op->addr.val << 8) & 0xffffff;
>> +				cfg->ifr |= QSPI_IFR_ADDREN;
>> +				cfg->iar = (op->addr.val << 8) & 0xffffff;
>>  				dummy_cycles -= 8 / op->addr.buswidth;
>>  			}
>>  			break;
>>  		case 3:
>> -			ifr |= QSPI_IFR_ADDREN;
>> -			iar = op->addr.val & 0xffffff;
>> +			cfg->ifr |= QSPI_IFR_ADDREN;
>> +			cfg->iar = op->addr.val & 0xffffff;
>>  			break;
>>  		case 4:
>> -			ifr |= QSPI_IFR_ADDREN | QSPI_IFR_ADDRL;
>> -			iar = op->addr.val & 0x7ffffff;
>> +			cfg->ifr |= QSPI_IFR_ADDREN | QSPI_IFR_ADDRL;
>> +			cfg->iar = op->addr.val & 0x7ffffff;
>>  			break;
>>  		default:
>>  			return -ENOTSUPP;
>> @@ -263,22 +296,99 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>>  
>>  	/* Set number of dummy cycles */
>>  	if (dummy_cycles)
>> -		ifr |= QSPI_IFR_NBDUM(dummy_cycles);
>> +		cfg->ifr |= QSPI_IFR_NBDUM(dummy_cycles);
>>  
>> -	/* Set data enable */
>> -	if (op->data.nbytes)
>> -		ifr |= QSPI_IFR_DATAEN;
>> +	return 0;
>> +}
>>  
>> +static void atmel_qspi_sama5d2_set_tfrtyp(const struct spi_mem_op *op,
>> +					  struct atmel_qspi_cfg *cfg)
>> +{
>>  	if (op->data.dir == SPI_MEM_DATA_OUT)
>> -		ifr |= QSPI_IFR_SAMA5D2_WRITE_TRSFR;
>> +		cfg->ifr |= QSPI_IFR_SAMA5D2_WRITE_TRSFR;
>> +}
>>  
>> +static void atmel_qspi_sama5d2_write_regs(void __iomem *base,
>> +					  const struct spi_mem_op *op,
>> +					  const struct atmel_qspi_cfg *cfg)
>> +{
>>  	/* Clear pending interrupts */
>>  	(void)readl_relaxed(base + QSPI_SR);
>>  
>>  	/* Set QSPI Instruction Frame registers */
>> -	writel_relaxed(iar, base + QSPI_IAR);
>> -	writel_relaxed(icr, base + QSPI_ICR);
>> -	writel_relaxed(ifr, base + QSPI_IFR);
>> +	writel_relaxed(cfg->iar, base + QSPI_IAR);
>> +	writel_relaxed(cfg->icr, base + QSPI_ICR);
>> +	writel_relaxed(cfg->ifr, base + QSPI_IFR);
>> +}
>> +
>> +static void atmel_qspi_sam9x60_set_tfrtyp(const struct spi_mem_op *op,
>> +					  struct atmel_qspi_cfg *cfg)
>> +{
>> +	if (!op->addr.nbytes && op->data.dir == SPI_MEM_DATA_IN)
>> +		cfg->ifr |= QSPI_IFR_APBTFRTYP_READ;
>> +}
>> +
>> +static void atmel_qspi_sam9x60_write_regs(void __iomem *base,
>> +					  const struct spi_mem_op *op,
>> +					  const struct atmel_qspi_cfg *cfg)
>> +{
>> +	/* Clear pending interrupts */
>> +	(void)readl_relaxed(base + QSPI_SR);
>> +
>> +	/* Set QSPI Instruction Frame registers */
>> +	writel_relaxed(cfg->iar, base + QSPI_IAR);
>> +	if (op->data.dir == SPI_MEM_DATA_IN)
>> +		writel_relaxed(cfg->icr, base + QSPI_RICR);
>> +	else
>> +		writel_relaxed(cfg->icr, base + QSPI_ICR);
>> +	writel_relaxed(cfg->ifr, base + QSPI_IFR);
>> +}
>> +
>> +static int atmel_qspi_set_cfg(struct atmel_qspi *aq,
>> +			      const struct spi_mem_op *op,
>> +			      struct atmel_qspi_cfg *cfg)
>> +{
>> +	void __iomem *base = aq->regs;
>> +	int ret;
>> +
>> +	/* Set the QSPI controller in Serial Memory Mode */
>> +	if (aq->smm != QSPI_MR_SMM) {
>> +		writel_relaxed(QSPI_MR_SMM, base + QSPI_MR);
> 
> 					    aq->reqs +
> 
> and you can get rid of base.

I will wait your reasons on this, see 3/13

> 
>> +		aq->smm = QSPI_MR_SMM;
>> +	}
>> +
>> +	ret = atmel_qspi_set_mode(cfg, op);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = atmel_qspi_set_address_mode(cfg, op);
>> +	if (ret)
>> +		return ret;
>> +
>> +	cfg->ifr |= QSPI_IFR_INSTEN;
>> +	cfg->icr |= QSPI_ICR_INST(op->cmd.opcode);
>> +
>> +	/* Set data enable */
>> +	if (op->data.nbytes)
>> +		cfg->ifr |= QSPI_IFR_DATAEN;
>> +
>> +	aq->caps->ops->set_tfrtyp(op, cfg);
>> +	aq->caps->ops->write_regs(base, op, cfg);
>> +
>> +	return 0;
>> +}
>> +
>> +static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>> +{
>> +	struct atmel_qspi *aq = spi_controller_get_devdata(mem->spi->master);
>> +	void __iomem *base = aq->regs;
>> +	struct atmel_qspi_cfg cfg = {0};
>> +	u32 sr;
>> +	int err;
>> +
>> +	err = atmel_qspi_set_cfg(aq, op, &cfg);
>> +	if (err)
>> +		return err;
>>  
>>  	/* Skip to the final steps if there is no data */
>>  	if (op->data.nbytes) {
>> @@ -287,11 +397,11 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>>  
>>  		/* Send/Receive data */
>>  		if (op->data.dir == SPI_MEM_DATA_IN)
>> -			_memcpy_fromio(op->data.buf.in,
>> -				aq->mem + iar, op->data.nbytes);
>> +			_memcpy_fromio(op->data.buf.in, aq->mem + cfg.iar,
>> +				       op->data.nbytes);
>>  		else
>> -			_memcpy_toio(aq->mem + iar,
>> -				op->data.buf.out, op->data.nbytes);
>> +			_memcpy_toio(aq->mem + cfg.iar, op->data.buf.out,
>> +				     op->data.nbytes);
>>  
>>  		/* Release the chip-select */
>>  		writel_relaxed(QSPI_CR_LASTXFER, base + QSPI_CR);
>> @@ -391,9 +501,22 @@ static int atmel_qspi_probe(struct platform_device *pdev)
>>  	struct spi_controller *ctrl;
>>  	struct atmel_qspi *aq;
>>  	struct resource *res;
>> +	const struct atmel_qspi_caps *caps;
>> +	struct device *dev = &pdev->dev;
> 
> If you really want to do this &pdev->dev -> dev conversion (which I
> don't think is necessary given that we don't have over 80 chars lines),
> please do it in separate patch.

ditto. Thanks again!
ta

> 
>>  	int irq, err = 0;
>>  
>> -	ctrl = spi_alloc_master(&pdev->dev, sizeof(*aq));
>> +	caps = of_device_get_match_data(dev);
>> +	if (!caps) {
>> +		dev_err(dev, "Could not retrieve QSPI caps\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!caps->ops->set_tfrtyp || !caps->ops->write_regs) {
>> +		dev_err(dev, "Could not retrieve QSPI ops\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ctrl = spi_alloc_master(dev, sizeof(*aq));
>>  	if (!ctrl)
>>  		return -ENOMEM;
>>  
>> @@ -409,32 +532,33 @@ static int atmel_qspi_probe(struct platform_device *pdev)
>>  
>>  	init_completion(&aq->cmd_completion);
>>  	aq->pdev = pdev;
>> +	aq->caps = caps;
>>  
>>  	/* Map the registers */
>>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "qspi_base");
>> -	aq->regs = devm_ioremap_resource(&pdev->dev, res);
>> +	aq->regs = devm_ioremap_resource(dev, res);
>>  	if (IS_ERR(aq->regs)) {
>> -		dev_err(&pdev->dev, "missing registers\n");
>> +		dev_err(dev, "missing registers\n");
>>  		err = PTR_ERR(aq->regs);
>>  		goto exit;
>>  	}
>>  
>>  	/* Map the AHB memory */
>>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "qspi_mmap");
>> -	aq->mem = devm_ioremap_resource(&pdev->dev, res);
>> +	aq->mem = devm_ioremap_resource(dev, res);
>>  	if (IS_ERR(aq->mem)) {
>> -		dev_err(&pdev->dev, "missing AHB memory\n");
>> +		dev_err(dev, "missing AHB memory\n");
>>  		err = PTR_ERR(aq->mem);
>>  		goto exit;
>>  	}
>>  
>>  	/* Get the peripheral clock */
>> -	aq->pclk = devm_clk_get(&pdev->dev, "pclk");
>> +	aq->pclk = devm_clk_get(dev, "pclk");
>>  	if (IS_ERR(aq->pclk))
>> -		aq->pclk = devm_clk_get(&pdev->dev, NULL);
>> +		aq->pclk = devm_clk_get(dev, NULL);
>>  
>>  	if (IS_ERR(aq->pclk)) {
>> -		dev_err(&pdev->dev, "missing peripheral clock\n");
>> +		dev_err(dev, "missing peripheral clock\n");
>>  		err = PTR_ERR(aq->pclk);
>>  		goto exit;
>>  	}
>> @@ -442,32 +566,52 @@ static int atmel_qspi_probe(struct platform_device *pdev)
>>  	/* Enable the peripheral clock */
>>  	err = clk_prepare_enable(aq->pclk);
>>  	if (err) {
>> -		dev_err(&pdev->dev, "failed to enable the peripheral clock\n");
>> +		dev_err(dev, "failed to enable the peripheral clock\n");
>>  		goto exit;
>>  	}
>>  
>> +	if (caps->has_qspick) {
>> +		/* Get the QSPI system clock */
>> +		aq->qspick = devm_clk_get(dev, "qspick");
>> +		if (IS_ERR(aq->qspick)) {
>> +			dev_err(dev, "missing system clock\n");
>> +			err = PTR_ERR(aq->qspick);
>> +			goto disable_pclk;
>> +		}
>> +
>> +		/* Enable the QSPI system clock */
>> +		err = clk_prepare_enable(aq->qspick);
>> +		if (err) {
>> +			dev_err(dev,
>> +				"failed to enable the QSPI system clock\n");
>> +			goto disable_pclk;
>> +		}
>> +	}
>> +
>>  	/* Request the IRQ */
>>  	irq = platform_get_irq(pdev, 0);
>>  	if (irq < 0) {
>> -		dev_err(&pdev->dev, "missing IRQ\n");
>> +		dev_err(dev, "missing IRQ\n");
>>  		err = irq;
>> -		goto disable_pclk;
>> +		goto disable_qspick;
>>  	}
>> -	err = devm_request_irq(&pdev->dev, irq, atmel_qspi_interrupt,
>> -			       0, dev_name(&pdev->dev), aq);
>> +	err = devm_request_irq(dev, irq, atmel_qspi_interrupt, 0,
>> +			       dev_name(dev), aq);
>>  	if (err)
>> -		goto disable_pclk;
>> +		goto disable_qspick;
>>  
>>  	err = atmel_qspi_init(aq);
>>  	if (err)
>> -		goto disable_pclk;
>> +		goto disable_qspick;
>>  
>>  	err = spi_register_controller(ctrl);
>>  	if (err)
>> -		goto disable_pclk;
>> +		goto disable_qspick;
>>  
>>  	return 0;
>>  
>> +disable_qspick:
>> +	clk_disable_unprepare(aq->qspick);
>>  disable_pclk:
>>  	clk_disable_unprepare(aq->pclk);
>>  exit:
>> @@ -483,6 +627,7 @@ static int atmel_qspi_remove(struct platform_device *pdev)
>>  
>>  	spi_unregister_controller(ctrl);
>>  	writel_relaxed(QSPI_CR_QSPIDIS, aq->regs + QSPI_CR);
>> +	clk_disable_unprepare(aq->qspick);
>>  	clk_disable_unprepare(aq->pclk);
>>  	return 0;
>>  }
>> @@ -491,6 +636,7 @@ static int __maybe_unused atmel_qspi_suspend(struct device *dev)
>>  {
>>  	struct atmel_qspi *aq = dev_get_drvdata(dev);
>>  
>> +	clk_disable_unprepare(aq->qspick);
>>  	clk_disable_unprepare(aq->pclk);
>>  
>>  	return 0;
>> @@ -501,6 +647,7 @@ static int __maybe_unused atmel_qspi_resume(struct device *dev)
>>  	struct atmel_qspi *aq = dev_get_drvdata(dev);
>>  
>>  	clk_prepare_enable(aq->pclk);
>> +	clk_prepare_enable(aq->qspick);
>>  
>>  	return atmel_qspi_init(aq);
>>  }
>> @@ -508,8 +655,34 @@ static int __maybe_unused atmel_qspi_resume(struct device *dev)
>>  static SIMPLE_DEV_PM_OPS(atmel_qspi_pm_ops, atmel_qspi_suspend,
>>  			 atmel_qspi_resume);
>>  
>> +static const struct atmel_qspi_ops atmel_sama5d2_qspi_ops = {
>> +	.set_tfrtyp = atmel_qspi_sama5d2_set_tfrtyp,
>> +	.write_regs = atmel_qspi_sama5d2_write_regs,
>> +};
>> +
>> +static const struct atmel_qspi_caps atmel_sama5d2_qspi_caps = {
>> +	.ops = &atmel_sama5d2_qspi_ops,
>> +};
>> +
>> +static const struct atmel_qspi_ops atmel_sam9x60_qspi_ops = {
>> +	.set_tfrtyp = atmel_qspi_sam9x60_set_tfrtyp,
>> +	.write_regs = atmel_qspi_sam9x60_write_regs,
>> +};
>> +
>> +static const struct atmel_qspi_caps atmel_sam9x60_qspi_caps = {
>> +	.ops = &atmel_sam9x60_qspi_ops,
>> +	.has_qspick = true,
>> +};
>> +
>>  static const struct of_device_id atmel_qspi_dt_ids[] = {
>> -	{ .compatible = "atmel,sama5d2-qspi" },
>> +	{
>> +		.compatible = "atmel,sama5d2-qspi",
>> +		.data = &atmel_sama5d2_qspi_caps,
>> +	},
>> +	{
>> +		.compatible = "microchip,sam9x60-qspi",
>> +		.data = &atmel_sam9x60_qspi_caps,
>> +	},
>>  	{ /* sentinel */ }
>>  };
>>  
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 01/13] spi: atmel-quadspi: cache MR value to avoid a write access
  2019-02-02  8:38     ` Tudor.Ambarus
@ 2019-02-02 13:20       ` Boris Brezillon
  0 siblings, 0 replies; 31+ messages in thread
From: Boris Brezillon @ 2019-02-02 13:20 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: mark.rutland, devicetree, alexandre.belloni, broonie,
	linux-kernel, robh+dt, linux-spi, Ludovic.Desroches,
	Cyrille.Pitchen, linux-mtd, bugalski.piotr, linux-arm-kernel

On Sat, 2 Feb 2019 08:38:40 +0000
<Tudor.Ambarus@microchip.com> wrote:

> On 02/02/2019 09:06 AM, Boris Brezillon wrote:
> > On Sat, 2 Feb 2019 04:07:13 +0000
> > <Tudor.Ambarus@microchip.com> wrote:
> >   
> >> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> >>
> >> Cache Serial Memory Mode (SMM) value to avoid write access when
> >> setting the controller in serial memory mode. SMM is set in
> >> exec_op() and not at probe time, to let room for future regular
> >> SPI support.
> >>
> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> >> ---
> >> v3: update smm value when different. rename mr/smm
> >> v2: cache MR value instead of moving the write access at probe
> >>
> >>  drivers/spi/atmel-quadspi.c | 7 ++++++-
> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
> >> index ddc712410812..645284c6ec9a 100644
> >> --- a/drivers/spi/atmel-quadspi.c
> >> +++ b/drivers/spi/atmel-quadspi.c
> >> @@ -155,6 +155,7 @@ struct atmel_qspi {
> >>  	struct clk		*clk;
> >>  	struct platform_device	*pdev;
> >>  	u32			pending;
> >> +	u32			smm;
> >>  	struct completion	cmd_completion;
> >>  };
> >>  
> >> @@ -238,7 +239,11 @@ static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
> >>  	icr = QSPI_ICR_INST(op->cmd.opcode);
> >>  	ifr = QSPI_IFR_INSTEN;
> >>  
> >> -	qspi_writel(aq, QSPI_MR, QSPI_MR_SMM);
> >> +	/* Set the QSPI controller in Serial Memory Mode */
> >> +	if (aq->smm != QSPI_MR_SMM) {  
> > 
> > Sorry, I think I misunderstood your previous suggestion, I thought the
> > reg was called SMM. If the reg is called MR and the value you expect in
> > there is SMM, then the field should be named ->mr as it caches the
> > whole reg, not only the SMM bit. So it's actually:
> > 
> > 	if (aq->mr != QSPI_MR_SMM) {  
> 
> No worries. When keeping the reg name, and not the bit itself, I would expect to
> do the check as in v2, to let room for checking other bits too:
> 
> +	if (!(aq->mr & QSPI_MR_SMM))
> 
> I don't have any problems to keep "mr" name, but I would like to understand your
> reasons.

Either you want to only set the SMM bit and keep the other bits
untouched or you want to make sure the register contains the value you
expect for all bitfields. If you're trying to achieve the former, you
should only update SMM instead of setting SMM + clearing all other
bits. In the other hand, if you want to apply a new MR setting where
you know exactly that only SMM should be set, that means you should
test the value in the cache (->mr) against the value you expect, and
not only the check that QSPI_MR_SMM is set.

BTW, you should probably initialize ->mr at probe time (using a
readl_relaxed()).

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 03/13] spi: atmel-quadspi: drop wrappers for iomem accesses
  2019-02-02  8:44     ` Tudor.Ambarus
@ 2019-02-02 13:23       ` Boris Brezillon
  0 siblings, 0 replies; 31+ messages in thread
From: Boris Brezillon @ 2019-02-02 13:23 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: mark.rutland, devicetree, alexandre.belloni, broonie,
	linux-kernel, robh+dt, Nicolas.Ferre, Ludovic.Desroches,
	Cyrille.Pitchen, linux-mtd, bugalski.piotr, linux-spi,
	linux-arm-kernel

On Sat, 2 Feb 2019 08:44:27 +0000
<Tudor.Ambarus@microchip.com> wrote:

> On 02/02/2019 09:11 AM, Boris Brezillon wrote:
> > On Sat, 2 Feb 2019 04:07:19 +0000
> > <Tudor.Ambarus@microchip.com> wrote:
> >   
> >> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> >>
> >> The wrappers hid that the accesses are relaxed. Drop them.
> >>
> >> Suggested-by: Boris Brezillon <bbrezillon@kernel.org>
> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> >> ---
> >> v3: no change
> >> v2: new patch
> >>
> >>  drivers/spi/atmel-quadspi.c | 47 +++++++++++++++++++--------------------------
> >>  1 file changed, 20 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
> >> index feeddcb25e1f..131374db0db4 100644
> >> --- a/drivers/spi/atmel-quadspi.c
> >> +++ b/drivers/spi/atmel-quadspi.c
> >> @@ -175,17 +175,6 @@ static const struct qspi_mode sama5d2_qspi_modes[] = {
> >>  	{ 4, 4, 4, QSPI_IFR_WIDTH_QUAD_CMD },
> >>  };
> >>  
> >> -/* Register access functions */
> >> -static inline u32 qspi_readl(struct atmel_qspi *aq, u32 reg)
> >> -{
> >> -	return readl_relaxed(aq->regs + reg);
> >> -}
> >> -
> >> -static inline void qspi_writel(struct atmel_qspi *aq, u32 reg, u32 value)
> >> -{
> >> -	writel_relaxed(value, aq->regs + reg);
> >> -}
> >> -
> >>  static inline bool is_compatible(const struct spi_mem_op *op,
> >>  				 const struct qspi_mode *mode)
> >>  {
> >> @@ -229,6 +218,7 @@ static bool atmel_qspi_supports_op(struct spi_mem *mem,
> >>  static int atmel_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
> >>  {
> >>  	struct atmel_qspi *aq = spi_controller_get_devdata(mem->spi->master);
> >> +	void __iomem *base = aq->regs;  
> > 
> > Can we name this variable regs instead of base or even get rid of it
> > and dereference aq->regs in the xxx_relaxed() calls (doesn't look like
> > the lines would be over 80 chars even when doing that). With this
> > addressed, you can add:
> > 
> > Reviewed-by: Boris Brezillon <bbrezillon@kernel.org>  
> 
> I chose to introduce the "base" variable when I have at least 2 dereferences in
> a function, as an optimization. In exec_op() for example, there are 6
> dereferences of aq->reqs. Why do you prefer keeping aq->regs?

I tend to not add local variables unless they help improve readability
or optimize things. In this case, I'd expect the compiler to be smart
enough to detect that aq->reqs is used several times and store it in a
register. When it comes to readability, I don't think it improve
things, but that's probably a matter of taste.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 08/13] spi: atmel-quadspi: drop unused and NOP transfer macros
  2019-02-02  8:46     ` Tudor.Ambarus
@ 2019-02-02 13:27       ` Boris Brezillon
  0 siblings, 0 replies; 31+ messages in thread
From: Boris Brezillon @ 2019-02-02 13:27 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: mark.rutland, devicetree, alexandre.belloni, linux-kernel,
	Cyrille.Pitchen, Nicolas.Ferre, robh+dt, linux-spi,
	Ludovic.Desroches, broonie, linux-mtd, bugalski.piotr,
	linux-arm-kernel

On Sat, 2 Feb 2019 08:46:38 +0000
<Tudor.Ambarus@microchip.com> wrote:

> On 02/02/2019 09:13 AM, Boris Brezillon wrote:
> > On Sat, 2 Feb 2019 04:07:33 +0000
> > <Tudor.Ambarus@microchip.com> wrote:
> >   
> >> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> >>
> >> Remove NOP when setting read transfer type. Remove useless
> >> setting of write transfer type when
> >> op->data.dir == SPI_MEM_DATA_IN && !op->data.nbytes.
> >>
> >> QSPI_IFR_TFRTYP_TRSFR_WRITE is specific just to sama5d2 qspi,
> >> rename it to QSPI_IFR_SAMA5D2_WRITE_TRSFR.
> >>
> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> >> ---
> >> v3: new patch
> >>
> >>  drivers/spi/atmel-quadspi.c | 12 +++---------
> >>  1 file changed, 3 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
> >> index d26d4cd0e36b..bfa5f5e92d96 100644
> >> --- a/drivers/spi/atmel-quadspi.c
> >> +++ b/drivers/spi/atmel-quadspi.c
> >> @@ -113,11 +113,7 @@
> >>  #define QSPI_IFR_OPTL_4BIT              (2 << 8)
> >>  #define QSPI_IFR_OPTL_8BIT              (3 << 8)
> >>  #define QSPI_IFR_ADDRL                  BIT(10)
> >> -#define QSPI_IFR_TFRTYP_MASK            GENMASK(13, 12)
> >> -#define QSPI_IFR_TFRTYP_TRSFR_READ      (0 << 12)
> >> -#define QSPI_IFR_TFRTYP_TRSFR_READ_MEM  (1 << 12)
> >> -#define QSPI_IFR_TFRTYP_TRSFR_WRITE     (2 << 12)
> >> -#define QSPI_IFR_TFRTYP_TRSFR_WRITE_MEM (3 << 13)
> >> +#define QSPI_IFR_SAMA5D2_WRITE_TRSFR	BIT(13)  
> > 
> > Can you define QSPI_IFR_TFRTYP_MEM (bit 12) even if it's not used yet?  
> 
> Shouldn't be introduced with the dir map support?

I like when all regs/reg-fields are defined, even if they're not used.
In this case, you're cleary splitting the TFRTYP_TRSFR_ bitfields in 2:
one bit encoding the mem/reg transfer type and one bit encoding the
direction of the transfer (read/write). Just think it's better to add
bit 12 definition now.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 13/13] spi: atmel-quadspi: add support for sam9x60 qspi controller
  2019-02-02  8:58     ` Tudor.Ambarus
@ 2019-02-02 13:30       ` Boris Brezillon
  0 siblings, 0 replies; 31+ messages in thread
From: Boris Brezillon @ 2019-02-02 13:30 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: mark.rutland, devicetree, alexandre.belloni, broonie,
	linux-kernel, robh+dt, Nicolas.Ferre, Ludovic.Desroches,
	Cyrille.Pitchen, linux-mtd, bugalski.piotr, linux-spi,
	linux-arm-kernel

On Sat, 2 Feb 2019 08:58:25 +0000
<Tudor.Ambarus@microchip.com> wrote:

> >> @@ -117,6 +120,7 @@
> >>  #define QSPI_IFR_CRM                    BIT(14)
> >>  #define QSPI_IFR_NBDUM_MASK             GENMASK(20, 16)
> >>  #define QSPI_IFR_NBDUM(n)               (((n) << 16) & QSPI_IFR_NBDUM_MASK)
> >> +#define QSPI_IFR_APBTFRTYP_READ		BIT(24)  
> > 
> > Maybe add a comment saying it's only available on SAM9X60 or prefix it
> > with SAM9X60.  
> 
> I'll add a comment. The macro name is more generic how it is now and can be used
> by future versions of the IP. Hypothetically speaking, if we rename it to
> QSPI_IFR_SAM9x60_TFSFR_READ and other sam9x will come out, then I'll have to
> rename this macro again, to make it more generic.	

Okay.


> >> +static int atmel_qspi_set_cfg(struct atmel_qspi *aq,
> >> +			      const struct spi_mem_op *op,
> >> +			      struct atmel_qspi_cfg *cfg)
> >> +{
> >> +	void __iomem *base = aq->regs;
> >> +	int ret;
> >> +
> >> +	/* Set the QSPI controller in Serial Memory Mode */
> >> +	if (aq->smm != QSPI_MR_SMM) {
> >> +		writel_relaxed(QSPI_MR_SMM, base + QSPI_MR);  
> > 
> > 					    aq->reqs +
> > 
> > and you can get rid of base.  
> 
> I will wait your reasons on this, see 3/13

ad->regs is only dereferenced once in this function, so there's even
less reasons to add a local var. 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2019-02-02 13:31 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-02  4:07 [PATCH v3 00/13] spi: atmel-quadspi: introduce sam9x60 qspi controller Tudor.Ambarus
2019-02-02  4:07 ` [PATCH v3 01/13] spi: atmel-quadspi: cache MR value to avoid a write access Tudor.Ambarus
2019-02-02  7:06   ` Boris Brezillon
2019-02-02  8:38     ` Tudor.Ambarus
2019-02-02 13:20       ` Boris Brezillon
2019-02-02  4:07 ` [PATCH v3 02/13] spi: atmel-quadspi: order header files inclusion alphabetically Tudor.Ambarus
2019-02-02  4:07 ` [PATCH v3 03/13] spi: atmel-quadspi: drop wrappers for iomem accesses Tudor.Ambarus
2019-02-02  7:11   ` Boris Brezillon
2019-02-02  8:44     ` Tudor.Ambarus
2019-02-02 13:23       ` Boris Brezillon
2019-02-02  4:07 ` [PATCH v3 04/13] spi: atmel-quadspi: fix naming scheme Tudor.Ambarus
2019-02-02  7:12   ` Boris Brezillon
2019-02-02  4:07 ` [PATCH v3 05/13] spi: atmel-quadspi: remove unnecessary cast Tudor.Ambarus
2019-02-02  4:07 ` [PATCH v3 06/13] spi: atmel-quadspi: return appropriate error code Tudor.Ambarus
2019-02-02  4:07 ` [PATCH v3 07/13] spi: atmel-quadspi: switch to SPDX license identifiers Tudor.Ambarus
2019-02-02  4:07 ` [PATCH v3 08/13] spi: atmel-quadspi: drop unused and NOP transfer macros Tudor.Ambarus
2019-02-02  7:13   ` Boris Brezillon
2019-02-02  8:46     ` Tudor.Ambarus
2019-02-02 13:27       ` Boris Brezillon
2019-02-02  4:07 ` [PATCH v3 09/13] dt-bindings: spi: atmel-quadspi: update example to new clock binding Tudor.Ambarus
2019-02-02  4:07 ` [PATCH v3 10/13] dt-bindings: spi: atmel-quadspi: make "pclk" mandatory Tudor.Ambarus
2019-02-02  7:15   ` Boris Brezillon
2019-02-02  8:47     ` Tudor.Ambarus
2019-02-02  4:07 ` [PATCH v3 11/13] spi: atmel-quadspi: add support for named peripheral clock Tudor.Ambarus
2019-02-02  7:16   ` Boris Brezillon
2019-02-02  4:07 ` [PATCH v3 12/13] dt-bindings: spi: atmel-quadspi: QuadSPI driver for Microchip SAM9X60 Tudor.Ambarus
2019-02-02  7:17   ` Boris Brezillon
2019-02-02  4:07 ` [PATCH v3 13/13] spi: atmel-quadspi: add support for sam9x60 qspi controller Tudor.Ambarus
2019-02-02  7:29   ` Boris Brezillon
2019-02-02  8:58     ` Tudor.Ambarus
2019-02-02 13:30       ` Boris Brezillon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).