[PATCH 1/5] spi: spi-nxp-fspi: enable runtime pm for fspi

[PATCH 1/5] spi: spi-nxp-fspi: enable runtime pm for fspi

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

Enable the runtime PM in fspi driver.
Also for system PM, On some board like i.MX8ULP-EVK board,
after system suspend, IOMUX module will lost power, so all
the pinctrl setting will lost when system resume back, need
driver to save/restore the pinctrl setting.

Signed-off-by: Han Xu <han.xu@nxp.com>
Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/spi/spi-nxp-fspi.c | 111 ++++++++++++++++++++++++++++++++-----
 1 file changed, 97 insertions(+), 14 deletions(-)

diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
index e13f678f2395..0feecf5ba010 100644
--- a/drivers/spi/spi-nxp-fspi.c
+++ b/drivers/spi/spi-nxp-fspi.c
@@ -48,6 +48,8 @@
 #include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/pm_runtime.h>
 #include <linux/pm_qos.h>
 #include <linux/regmap.h>
 #include <linux/sizes.h>
@@ -57,6 +59,8 @@
 #include <linux/spi/spi.h>
 #include <linux/spi/spi-mem.h>
 
+/* runtime pm timeout */
+#define FSPI_RPM_TIMEOUT 50	/* 50ms */
 /*
  * The driver only uses one single LUT entry, that is updated on
  * each call of exec_op(). Index 0 is preset at boot with a basic
@@ -390,6 +394,8 @@ struct nxp_fspi {
 	struct mutex lock;
 	struct pm_qos_request pm_qos_req;
 	int selected;
+#define FSPI_INITILIZED		(1 << 0)
+	int flags;
 };
 
 static inline int needs_ip_only(struct nxp_fspi *f)
@@ -917,6 +923,12 @@ static int nxp_fspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 
 	mutex_lock(&f->lock);
 
+	err = pm_runtime_get_sync(f->dev);
+	if (err < 0) {
+		dev_err(f->dev, "Failed to enable clock %d\n", __LINE__);
+		goto err_mutex;
+	}
+
 	/* Wait for controller being ready. */
 	err = fspi_readl_poll_tout(f, f->iobase + FSPI_STS0,
 				   FSPI_STS0_ARB_IDLE, 1, POLL_TOUT, true);
@@ -945,8 +957,14 @@ static int nxp_fspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 	/* Invalidate the data in the AHB buffer. */
 	nxp_fspi_invalid(f);
 
+	pm_runtime_mark_last_busy(f->dev);
+	pm_runtime_put_autosuspend(f->dev);
+
 	mutex_unlock(&f->lock);
+	return err;
 
+err_mutex:
+	mutex_unlock(&f->lock);
 	return err;
 }
 
@@ -1201,12 +1219,17 @@ static int nxp_fspi_probe(struct platform_device *pdev)
 			ret = PTR_ERR(f->clk);
 			goto err_put_ctrl;
 		}
+	}
 
-		ret = nxp_fspi_clk_prep_enable(f);
-		if (ret) {
-			dev_err(dev, "can not enable the clock\n");
-			goto err_put_ctrl;
-		}
+	pm_runtime_enable(dev);
+	pm_runtime_set_autosuspend_delay(dev, FSPI_RPM_TIMEOUT);
+	pm_runtime_use_autosuspend(dev);
+
+	/* enable clock */
+	ret = pm_runtime_get_sync(f->dev);
+	if (ret < 0) {
+		dev_err(f->dev, "Failed to enable clock %d\n", __LINE__);
+		goto err_put_ctrl;
 	}
 
 	/* Clear potential interrupts */
@@ -1240,13 +1263,19 @@ static int nxp_fspi_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_destroy_mutex;
 
+	pm_runtime_mark_last_busy(f->dev);
+	pm_runtime_put_autosuspend(f->dev);
+
+	/* indicate the controller has been initialized */
+	f->flags |= FSPI_INITILIZED;
+
 	return 0;
 
 err_destroy_mutex:
 	mutex_destroy(&f->lock);
 
 err_disable_clk:
-	nxp_fspi_clk_disable_unprep(f);
+	pm_runtime_disable(dev);
 
 err_put_ctrl:
 	spi_controller_put(ctlr);
@@ -1270,20 +1299,79 @@ static void nxp_fspi_remove(struct platform_device *pdev)
 		iounmap(f->ahb_addr);
 }
 
-static int nxp_fspi_suspend(struct device *dev)
+#ifdef CONFIG_PM
+static int nxp_fspi_initialized(struct nxp_fspi *f)
+{
+	return f->flags & FSPI_INITILIZED;
+}
+
+static int nxp_fspi_need_reinit(struct nxp_fspi *f)
+{
+	/*
+	 * we always use the controller in combination mode, so we check
+	 * this register bit to determine if the controller once lost power,
+	 * such as suspend/resume, and need to be re-init.
+	 */
+
+	return !(readl(f->iobase + FSPI_MCR0) & FSPI_MCR0_OCTCOMB_EN);
+}
+
+static int nxp_fspi_runtime_suspend(struct device *dev)
 {
+	struct nxp_fspi *f = dev_get_drvdata(dev);
+
+	nxp_fspi_clk_disable_unprep(f);
+
 	return 0;
 }
 
-static int nxp_fspi_resume(struct device *dev)
+static int nxp_fspi_runtime_resume(struct device *dev)
 {
 	struct nxp_fspi *f = dev_get_drvdata(dev);
 
-	nxp_fspi_default_setup(f);
+	nxp_fspi_clk_prep_enable(f);
+
+	if (nxp_fspi_initialized(f) && nxp_fspi_need_reinit(f))
+		nxp_fspi_default_setup(f);
 
 	return 0;
 }
 
+static int nxp_fspi_suspend(struct device *dev)
+{
+	int ret;
+
+	ret = pinctrl_pm_select_sleep_state(dev);
+	if (ret) {
+		dev_err(dev, "select flexspi sleep pinctrl failed!\n");
+		return ret;
+	}
+
+	return pm_runtime_force_suspend(dev);
+}
+
+static int nxp_fspi_resume(struct device *dev)
+{
+	int ret;
+
+	ret = pm_runtime_force_resume(dev);
+	if (ret)
+		return ret;
+
+	ret = pinctrl_pm_select_default_state(dev);
+	if (ret)
+		dev_err(dev, "select flexspi default pinctrl failed!\n");
+
+	return ret;
+}
+
+
+static const struct dev_pm_ops nxp_fspi_pm_ops = {
+	SET_RUNTIME_PM_OPS(nxp_fspi_runtime_suspend, nxp_fspi_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(nxp_fspi_suspend, nxp_fspi_resume)
+};
+#endif	/* CONFIG_PM */
+
 static const struct of_device_id nxp_fspi_dt_ids[] = {
 	{ .compatible = "nxp,lx2160a-fspi", .data = (void *)&lx2160a_data, },
 	{ .compatible = "nxp,imx8mm-fspi", .data = (void *)&imx8mm_data, },
@@ -1302,11 +1390,6 @@ static const struct acpi_device_id nxp_fspi_acpi_ids[] = {
 MODULE_DEVICE_TABLE(acpi, nxp_fspi_acpi_ids);
 #endif
 
-static const struct dev_pm_ops nxp_fspi_pm_ops = {
-	.suspend	= nxp_fspi_suspend,
-	.resume		= nxp_fspi_resume,
-};
-
 static struct platform_driver nxp_fspi_driver = {
 	.driver = {
 		.name	= "nxp-fspi",
-- 
2.34.1

[PATCH 2/5] spi: spi-nxp-fspi: change the default lut index

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

The fspi dynamic lut use the last lut for all IPS operations, the
imx8ulp only supports 15 luts, so change the last lut index from
31 to 15.

Signed-off-by: Han Xu <han.xu@nxp.com>
Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/spi/spi-nxp-fspi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
index 0feecf5ba010..9d6b4d22263c 100644
--- a/drivers/spi/spi-nxp-fspi.c
+++ b/drivers/spi/spi-nxp-fspi.c
@@ -64,9 +64,9 @@
 /*
  * The driver only uses one single LUT entry, that is updated on
  * each call of exec_op(). Index 0 is preset at boot with a basic
- * read operation, so let's use the last entry (31).
+ * read operation, so let's use the last entry (15).
  */
-#define	SEQID_LUT			31
+#define	SEQID_LUT			15
 
 /* Registers used by the driver */
 #define FSPI_MCR0			0x00
-- 
2.34.1

Re: [PATCH 2/5] spi: spi-nxp-fspi: change the default lut index

[Michael Walle]

> The fspi dynamic lut use the last lut for all IPS operations, the
> imx8ulp only supports 15 luts, so change the last lut index from

It's 16 LUTs, no? There's also index 0.

> 31 to 15.
> 
> Signed-off-by: Han Xu <han.xu@nxp.com>
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> ---
>  drivers/spi/spi-nxp-fspi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
> index 0feecf5ba010..9d6b4d22263c 100644
> --- a/drivers/spi/spi-nxp-fspi.c
> +++ b/drivers/spi/spi-nxp-fspi.c
> @@ -64,9 +64,9 @@
>  /*
>   * The driver only uses one single LUT entry, that is updated on
>   * each call of exec_op(). Index 0 is preset at boot with a basic
> - * read operation, so let's use the last entry (31).
> + * read operation, so let's use the last entry (15).

Please add the information about the imx8mulp to the comment.
Otherwise, the comment will be confusing for SoCs where there
are 32 LUTs.

-michael

>   */
> -#define	SEQID_LUT			31
> +#define	SEQID_LUT			15
>  
>  /* Registers used by the driver */
>  #define FSPI_MCR0			0x00
> -- 
2.34.1

RE: [PATCH 2/5] spi: spi-nxp-fspi: change the default lut index

[Bough Chen]

> -----Original Message-----
> From: Michael Walle <mwalle@kernel.org>
> Sent: 2023年12月14日 0:25
> To: Bough Chen <haibo.chen@nxp.com>
> Cc: broonie@kernel.org; Han Xu <han.xu@nxp.com>; dl-linux-imx
> <linux-imx@nxp.com>; linux-spi@vger.kernel.org; yogeshgaur.83@gmail.com;
> Michael Walle <mwalle@kernel.org>
> Subject: Re: [PATCH 2/5] spi: spi-nxp-fspi: change the default lut index
> 
> > The fspi dynamic lut use the last lut for all IPS operations, the
> > imx8ulp only supports 15 luts, so change the last lut index from
> 
> It's 16 LUTs, no? There's also index 0.

Yes, should be 16 LUTs.

> 
> > 31 to 15.
> >
> > Signed-off-by: Han Xu <han.xu@nxp.com>
> > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > ---
> >  drivers/spi/spi-nxp-fspi.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
> > index 0feecf5ba010..9d6b4d22263c 100644
> > --- a/drivers/spi/spi-nxp-fspi.c
> > +++ b/drivers/spi/spi-nxp-fspi.c
> > @@ -64,9 +64,9 @@
> >  /*
> >   * The driver only uses one single LUT entry, that is updated on
> >   * each call of exec_op(). Index 0 is preset at boot with a basic
> > - * read operation, so let's use the last entry (31).
> > + * read operation, so let's use the last entry (15).
> 
> Please add the information about the imx8mulp to the comment.
> Otherwise, the comment will be confusing for SoCs where there are 32 LUTs.

Yes, will add.

Best Regards
Haibo Chen
> 
> -michael
> 
> >   */
> > -#define	SEQID_LUT			31
> > +#define	SEQID_LUT			15
> >
> >  /* Registers used by the driver */
> >  #define FSPI_MCR0			0x00
> > --
> 2.34.1
> 

Re: [PATCH 2/5] spi: spi-nxp-fspi: change the default lut index

[Adam Ford]

On Wed, Dec 13, 2023 at 3:08 AM <haibo.chen@nxp.com> wrote:
>
> From: Haibo Chen <haibo.chen@nxp.com>
>
> The fspi dynamic lut use the last lut for all IPS operations, the
> imx8ulp only supports 15 luts, so change the last lut index from
> 31 to 15.
>
> Signed-off-by: Han Xu <han.xu@nxp.com>
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> ---
>  drivers/spi/spi-nxp-fspi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
> index 0feecf5ba010..9d6b4d22263c 100644
> --- a/drivers/spi/spi-nxp-fspi.c
> +++ b/drivers/spi/spi-nxp-fspi.c
> @@ -64,9 +64,9 @@
>  /*
>   * The driver only uses one single LUT entry, that is updated on
>   * each call of exec_op(). Index 0 is preset at boot with a basic
> - * read operation, so let's use the last entry (31).
> + * read operation, so let's use the last entry (15).
>   */
> -#define        SEQID_LUT                       31
> +#define        SEQID_LUT                       15

What impact does this have on other SoC's with the FlexSPI with 32?

adam
>
>  /* Registers used by the driver */
>  #define FSPI_MCR0                      0x00
> --
> 2.34.1
>
>

RE: [PATCH 2/5] spi: spi-nxp-fspi: change the default lut index

[Bough Chen]

> -----Original Message-----
> From: Adam Ford <aford173@gmail.com>
> Sent: 2023年12月14日 11:04
> To: Bough Chen <haibo.chen@nxp.com>
> Cc: broonie@kernel.org; yogeshgaur.83@gmail.com; linux-spi@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>; Han Xu <han.xu@nxp.com>
> Subject: Re: [PATCH 2/5] spi: spi-nxp-fspi: change the default lut index
> 
> On Wed, Dec 13, 2023 at 3:08 AM <haibo.chen@nxp.com> wrote:
> >
> > From: Haibo Chen <haibo.chen@nxp.com>
> >
> > The fspi dynamic lut use the last lut for all IPS operations, the
> > imx8ulp only supports 15 luts, so change the last lut index from
> > 31 to 15.
> >
> > Signed-off-by: Han Xu <han.xu@nxp.com>
> > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > ---
> >  drivers/spi/spi-nxp-fspi.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
> > index 0feecf5ba010..9d6b4d22263c 100644
> > --- a/drivers/spi/spi-nxp-fspi.c
> > +++ b/drivers/spi/spi-nxp-fspi.c
> > @@ -64,9 +64,9 @@
> >  /*
> >   * The driver only uses one single LUT entry, that is updated on
> >   * each call of exec_op(). Index 0 is preset at boot with a basic
> > - * read operation, so let's use the last entry (31).
> > + * read operation, so let's use the last entry (15).
> >   */
> > -#define        SEQID_LUT                       31
> > +#define        SEQID_LUT                       15
> 
> What impact does this have on other SoC's with the FlexSPI with 32?

No impact, just use another LUT to send command queue.

Best Regards
Haibo Chen
> 
> adam
> >
> >  /* Registers used by the driver */
> >  #define FSPI_MCR0                      0x00
> > --
> > 2.34.1
> >
> >

[PATCH 3/5] spi: spi-nxp-fspi: add DTR mode support

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

For LUT, add DTR command support.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/spi/spi-nxp-fspi.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
index 9d6b4d22263c..2562d524149e 100644
--- a/drivers/spi/spi-nxp-fspi.c
+++ b/drivers/spi/spi-nxp-fspi.c
@@ -552,12 +552,22 @@ static void nxp_fspi_prepare_lut(struct nxp_fspi *f,
 	int lutidx = 1, i;
 
 	/* cmd */
-	lutval[0] |= LUT_DEF(0, LUT_CMD, LUT_PAD(op->cmd.buswidth),
-			     op->cmd.opcode);
+	if (op->cmd.dtr) {
+		lutval[0] |= LUT_DEF(0, LUT_CMD_DDR, LUT_PAD(op->cmd.buswidth),
+				     op->cmd.opcode >> 8);
+		lutval[lutidx / 2] |= LUT_DEF(lutidx, LUT_CMD_DDR,
+					      LUT_PAD(op->cmd.buswidth),
+					      op->cmd.opcode & 0x00ff);
+		lutidx++;
+	} else {
+		lutval[0] |= LUT_DEF(0, LUT_CMD, LUT_PAD(op->cmd.buswidth),
+				     op->cmd.opcode);
+	}
 
 	/* addr bytes */
 	if (op->addr.nbytes) {
-		lutval[lutidx / 2] |= LUT_DEF(lutidx, LUT_ADDR,
+		lutval[lutidx / 2] |= LUT_DEF(lutidx, op->addr.dtr ?
+					      LUT_ADDR_DDR : LUT_ADDR,
 					      LUT_PAD(op->addr.buswidth),
 					      op->addr.nbytes * 8);
 		lutidx++;
@@ -565,7 +575,8 @@ static void nxp_fspi_prepare_lut(struct nxp_fspi *f,
 
 	/* dummy bytes, if needed */
 	if (op->dummy.nbytes) {
-		lutval[lutidx / 2] |= LUT_DEF(lutidx, LUT_DUMMY,
+		lutval[lutidx / 2] |= LUT_DEF(lutidx, op->dummy.dtr ?
+					      LUT_DUMMY_DDR : LUT_DUMMY,
 		/*
 		 * Due to FlexSPI controller limitation number of PAD for dummy
 		 * buswidth needs to be programmed as equal to data buswidth.
@@ -580,7 +591,8 @@ static void nxp_fspi_prepare_lut(struct nxp_fspi *f,
 	if (op->data.nbytes) {
 		lutval[lutidx / 2] |= LUT_DEF(lutidx,
 					      op->data.dir == SPI_MEM_DATA_IN ?
-					      LUT_NXP_READ : LUT_NXP_WRITE,
+					      (op->data.dtr ? LUT_READ_DDR : LUT_NXP_READ) :
+					      (op->data.dtr ? LUT_WRITE_DDR : LUT_NXP_WRITE),
 					      LUT_PAD(op->data.buswidth),
 					      0);
 		lutidx++;
@@ -1152,6 +1164,10 @@ static const struct spi_controller_mem_ops nxp_fspi_mem_ops = {
 	.get_name = nxp_fspi_get_name,
 };
 
+static struct spi_controller_mem_caps nxp_fspi_mem_caps = {
+	.dtr = true,
+};
+
 static int nxp_fspi_probe(struct platform_device *pdev)
 {
 	struct spi_controller *ctlr;
@@ -1254,6 +1270,7 @@ static int nxp_fspi_probe(struct platform_device *pdev)
 	ctlr->bus_num = -1;
 	ctlr->num_chipselect = NXP_FSPI_MAX_CHIPSELECT;
 	ctlr->mem_ops = &nxp_fspi_mem_ops;
+	ctlr->mem_caps = &nxp_fspi_mem_caps;
 
 	nxp_fspi_default_setup(f);
 
-- 
2.34.1

Re: [PATCH 3/5] spi: spi-nxp-fspi: add DTR mode support

[Michael Walle]

> From: Haibo Chen <haibo.chen@nxp.com>
> 
> For LUT, add DTR command support.

Please elaborate a bit more.


> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> ---
>  drivers/spi/spi-nxp-fspi.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
> index 9d6b4d22263c..2562d524149e 100644
> --- a/drivers/spi/spi-nxp-fspi.c
> +++ b/drivers/spi/spi-nxp-fspi.c
> @@ -552,12 +552,22 @@ static void nxp_fspi_prepare_lut(struct nxp_fspi *f,
>  	int lutidx = 1, i;
>  
>  	/* cmd */
> -	lutval[0] |= LUT_DEF(0, LUT_CMD, LUT_PAD(op->cmd.buswidth),
> -			     op->cmd.opcode);
> +	if (op->cmd.dtr) {
> +		lutval[0] |= LUT_DEF(0, LUT_CMD_DDR, LUT_PAD(op->cmd.buswidth),
> +				     op->cmd.opcode >> 8);

Shouldn't we check cmd.nbytes here? You seem to mix dtr with cmd.nbytes ==
2 here.

> +		lutval[lutidx / 2] |= LUT_DEF(lutidx, LUT_CMD_DDR,
> +					      LUT_PAD(op->cmd.buswidth),
> +					      op->cmd.opcode & 0x00ff);

And you seem to assume dtr is always octal mode?

-michael

> +		lutidx++;
> +	} else {
> +		lutval[0] |= LUT_DEF(0, LUT_CMD, LUT_PAD(op->cmd.buswidth),
> +				     op->cmd.opcode);
> +	}
>  
>  	/* addr bytes */
>  	if (op->addr.nbytes) {
> -		lutval[lutidx / 2] |= LUT_DEF(lutidx, LUT_ADDR,
> +		lutval[lutidx / 2] |= LUT_DEF(lutidx, op->addr.dtr ?
> +					      LUT_ADDR_DDR : LUT_ADDR,
>  					      LUT_PAD(op->addr.buswidth),
>  					      op->addr.nbytes * 8);
>  		lutidx++;
> @@ -565,7 +575,8 @@ static void nxp_fspi_prepare_lut(struct nxp_fspi *f,
>  
>  	/* dummy bytes, if needed */
>  	if (op->dummy.nbytes) {
> -		lutval[lutidx / 2] |= LUT_DEF(lutidx, LUT_DUMMY,
> +		lutval[lutidx / 2] |= LUT_DEF(lutidx, op->dummy.dtr ?
> +					      LUT_DUMMY_DDR : LUT_DUMMY,
>  		/*
>  		 * Due to FlexSPI controller limitation number of PAD for dummy
>  		 * buswidth needs to be programmed as equal to data buswidth.
> @@ -580,7 +591,8 @@ static void nxp_fspi_prepare_lut(struct nxp_fspi *f,
>  	if (op->data.nbytes) {
>  		lutval[lutidx / 2] |= LUT_DEF(lutidx,
>  					      op->data.dir == SPI_MEM_DATA_IN ?
> -					      LUT_NXP_READ : LUT_NXP_WRITE,
> +					      (op->data.dtr ? LUT_READ_DDR : LUT_NXP_READ) :
> +					      (op->data.dtr ? LUT_WRITE_DDR : LUT_NXP_WRITE),
>  					      LUT_PAD(op->data.buswidth),
>  					      0);
>  		lutidx++;
> @@ -1152,6 +1164,10 @@ static const struct spi_controller_mem_ops nxp_fspi_mem_ops = {
>  	.get_name = nxp_fspi_get_name,
>  };
>  
> +static struct spi_controller_mem_caps nxp_fspi_mem_caps = {
> +	.dtr = true,
> +};
> +
>  static int nxp_fspi_probe(struct platform_device *pdev)
>  {
>  	struct spi_controller *ctlr;
> @@ -1254,6 +1270,7 @@ static int nxp_fspi_probe(struct platform_device *pdev)
>  	ctlr->bus_num = -1;
>  	ctlr->num_chipselect = NXP_FSPI_MAX_CHIPSELECT;
>  	ctlr->mem_ops = &nxp_fspi_mem_ops;
> +	ctlr->mem_caps = &nxp_fspi_mem_caps;
>  
>  	nxp_fspi_default_setup(f);
>  
> -- 
> 2.34.1

RE: [PATCH 3/5] spi: spi-nxp-fspi: add DTR mode support

[Bough Chen]

> -----Original Message-----
> From: Michael Walle <mwalle@kernel.org>
> Sent: 2023年12月14日 0:53
> To: Bough Chen <haibo.chen@nxp.com>
> Cc: broonie@kernel.org; Han Xu <han.xu@nxp.com>; dl-linux-imx
> <linux-imx@nxp.com>; linux-spi@vger.kernel.org; yogeshgaur.83@gmail.com;
> Michael Walle <mwalle@kernel.org>
> Subject: Re: [PATCH 3/5] spi: spi-nxp-fspi: add DTR mode support
> 
> > From: Haibo Chen <haibo.chen@nxp.com>
> >
> > For LUT, add DTR command support.
> 
> Please elaborate a bit more.

Okay.

> 
> 
> > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > ---
> >  drivers/spi/spi-nxp-fspi.c | 27 ++++++++++++++++++++++-----
> >  1 file changed, 22 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
> > index 9d6b4d22263c..2562d524149e 100644
> > --- a/drivers/spi/spi-nxp-fspi.c
> > +++ b/drivers/spi/spi-nxp-fspi.c
> > @@ -552,12 +552,22 @@ static void nxp_fspi_prepare_lut(struct nxp_fspi *f,
> >  	int lutidx = 1, i;
> >
> >  	/* cmd */
> > -	lutval[0] |= LUT_DEF(0, LUT_CMD, LUT_PAD(op->cmd.buswidth),
> > -			     op->cmd.opcode);
> > +	if (op->cmd.dtr) {
> > +		lutval[0] |= LUT_DEF(0, LUT_CMD_DDR,
> LUT_PAD(op->cmd.buswidth),
> > +				     op->cmd.opcode >> 8);
> 
> Shouldn't we check cmd.nbytes here? You seem to mix dtr with cmd.nbytes ==
> 2 here.

Currently, for DTR mode, all cmd.nbytes == 2. Refer to drivers/mtd/spi-nor/core.c : spi_nor_spimem_setup_op()
But better to check the cmd.nbytes here to make the code more strong.

> 
> > +		lutval[lutidx / 2] |= LUT_DEF(lutidx, LUT_CMD_DDR,
> > +					      LUT_PAD(op->cmd.buswidth),
> > +					      op->cmd.opcode & 0x00ff);
> 
> And you seem to assume dtr is always octal mode?

Currently, I only test the octa dtr mode(8D-8D-8D). but here, we config the op->cmd.buswidth, op->addr.buswidth, op->dummy.buswidth, op->data.buswidth.
So I think current LUT config can cover other dts mode, like 1D-8D-8D, 1D-4D-4D, 1D-2D-2D, 1D-1D-1D

Best Regards
Haibo Chen
> 
> -michael
> 
> > +		lutidx++;
> > +	} else {
> > +		lutval[0] |= LUT_DEF(0, LUT_CMD, LUT_PAD(op->cmd.buswidth),
> > +				     op->cmd.opcode);
> > +	}
> >
> >  	/* addr bytes */
> >  	if (op->addr.nbytes) {
> > -		lutval[lutidx / 2] |= LUT_DEF(lutidx, LUT_ADDR,
> > +		lutval[lutidx / 2] |= LUT_DEF(lutidx, op->addr.dtr ?
> > +					      LUT_ADDR_DDR : LUT_ADDR,
> >  					      LUT_PAD(op->addr.buswidth),
> >  					      op->addr.nbytes * 8);
> >  		lutidx++;
> > @@ -565,7 +575,8 @@ static void nxp_fspi_prepare_lut(struct nxp_fspi
> > *f,
> >
> >  	/* dummy bytes, if needed */
> >  	if (op->dummy.nbytes) {
> > -		lutval[lutidx / 2] |= LUT_DEF(lutidx, LUT_DUMMY,
> > +		lutval[lutidx / 2] |= LUT_DEF(lutidx, op->dummy.dtr ?
> > +					      LUT_DUMMY_DDR : LUT_DUMMY,
> >  		/*
> >  		 * Due to FlexSPI controller limitation number of PAD for dummy
> >  		 * buswidth needs to be programmed as equal to data buswidth.
> > @@ -580,7 +591,8 @@ static void nxp_fspi_prepare_lut(struct nxp_fspi *f,
> >  	if (op->data.nbytes) {
> >  		lutval[lutidx / 2] |= LUT_DEF(lutidx,
> >  					      op->data.dir == SPI_MEM_DATA_IN ?
> > -					      LUT_NXP_READ : LUT_NXP_WRITE,
> > +					      (op->data.dtr ? LUT_READ_DDR :
> LUT_NXP_READ) :
> > +					      (op->data.dtr ? LUT_WRITE_DDR :
> LUT_NXP_WRITE),
> >  					      LUT_PAD(op->data.buswidth),
> >  					      0);
> >  		lutidx++;
> > @@ -1152,6 +1164,10 @@ static const struct spi_controller_mem_ops
> nxp_fspi_mem_ops = {
> >  	.get_name = nxp_fspi_get_name,
> >  };
> >
> > +static struct spi_controller_mem_caps nxp_fspi_mem_caps = {
> > +	.dtr = true,
> > +};
> > +
> >  static int nxp_fspi_probe(struct platform_device *pdev)  {
> >  	struct spi_controller *ctlr;
> > @@ -1254,6 +1270,7 @@ static int nxp_fspi_probe(struct platform_device
> *pdev)
> >  	ctlr->bus_num = -1;
> >  	ctlr->num_chipselect = NXP_FSPI_MAX_CHIPSELECT;
> >  	ctlr->mem_ops = &nxp_fspi_mem_ops;
> > +	ctlr->mem_caps = &nxp_fspi_mem_caps;
> >
> >  	nxp_fspi_default_setup(f);
> >
> > --
> > 2.34.1

Re: [PATCH 3/5] spi: spi-nxp-fspi: add DTR mode support

[Michael Walle]

Hi,

>> > diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
>> > index 9d6b4d22263c..2562d524149e 100644
>> > --- a/drivers/spi/spi-nxp-fspi.c
>> > +++ b/drivers/spi/spi-nxp-fspi.c
>> > @@ -552,12 +552,22 @@ static void nxp_fspi_prepare_lut(struct nxp_fspi *f,
>> >  	int lutidx = 1, i;
>> >
>> >  	/* cmd */
>> > -	lutval[0] |= LUT_DEF(0, LUT_CMD, LUT_PAD(op->cmd.buswidth),
>> > -			     op->cmd.opcode);
>> > +	if (op->cmd.dtr) {
>> > +		lutval[0] |= LUT_DEF(0, LUT_CMD_DDR,
>> LUT_PAD(op->cmd.buswidth),
>> > +				     op->cmd.opcode >> 8);
>> 
>> Shouldn't we check cmd.nbytes here? You seem to mix dtr with 
>> cmd.nbytes ==
>> 2 here.
> 
> Currently, for DTR mode, all cmd.nbytes == 2. Refer to
> drivers/mtd/spi-nor/core.c : spi_nor_spimem_setup_op()
> But better to check the cmd.nbytes here to make the code more strong.

I'm aware of that, but that might change.

>> > +		lutval[lutidx / 2] |= LUT_DEF(lutidx, LUT_CMD_DDR,
>> > +					      LUT_PAD(op->cmd.buswidth),
>> > +					      op->cmd.opcode & 0x00ff);
>> 
>> And you seem to assume dtr is always octal mode?
> 
> Currently, I only test the octa dtr mode(8D-8D-8D). but here, we config 
> the
> op->cmd.buswidth, op->addr.buswidth, op->dummy.buswidth, 
> op->data.buswidth.
> So I think current LUT config can cover other dts mode, like 1D-8D-8D,
> 1D-4D-4D, 1D-2D-2D, 1D-1D-1D.

Agreed, for the code that follows this. But the 16bit opcode, ie. the
LUT above only makes sense for 8d8d8d. There you have to have
16bit because thats what transferred in one clock cycle.

You could add that to your .supports_op(). I.e. restrict this
driver to only support 8d8d8d. I know that the default op will
already check that, but better be safe.

-michael

[PATCH 4/5] spi: spi-nxp-fspi: add function to select sample clock source for flash reading

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

fspi define four mode for sample clock source selection.

Here is the list of modes:
mode 0: Dummy Read strobe generated by FlexSPI Controller and loopback internally
mode 1: Dummy Read strobe generated by FlexSPI Controller and loopback from DQS pad
mode 2: Reserved
mode 3: Flash provided Read strobe and input from DQS pad

In default, fspi use mode 0 after reset.
For 8-8-8-DTR mode, need to use mode 3, otherwise 8-8-8-DTR read always
get incorrect data.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/spi/spi-nxp-fspi.c | 47 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
index 2562d524149e..0330454b76c6 100644
--- a/drivers/spi/spi-nxp-fspi.c
+++ b/drivers/spi/spi-nxp-fspi.c
@@ -395,6 +395,7 @@ struct nxp_fspi {
 	struct pm_qos_request pm_qos_req;
 	int selected;
 #define FSPI_INITILIZED		(1 << 0)
+#define FSPI_RXCLKSRC_3		(1 << 1)
 	int flags;
 };
 
@@ -928,6 +929,50 @@ static int nxp_fspi_do_op(struct nxp_fspi *f, const struct spi_mem_op *op)
 	return err;
 }
 
+/*
+ * Sample Clock source selection for Flash Reading
+ * Four modes defined by fspi:
+ * mode 0: Dummy Read strobe generated by FlexSPI Controller
+ *         and loopback internally
+ * mode 1: Dummy Read strobe generated by FlexSPI Controller
+ *         and loopback from DQS pad
+ * mode 2: Reserved
+ * mode 3: Flash provided Read strobe and input from DQS pad
+ *
+ * fspi default use mode 0 after reset
+ */
+static void nxp_fspi_select_rx_sample_clk_source(struct nxp_fspi *f,
+						 const struct spi_mem_op *op)
+{
+	u32 reg;
+
+	/*
+	 * For 8-8-8-DTR mode, need to use mode 3 (Flash provided Read
+	 * strobe and input from DQS pad), otherwise read operaton may
+	 * meet issue.
+	 * This mode require flash device connect the DQS pad on board.
+	 * For other modes, still use mode 0, keep align with before.
+	 * spi_nor_suspend will disable 8-8-8-DTR mode, also need to
+	 * change the mode back to mode 0.
+	 */
+	if (!(f->flags & FSPI_RXCLKSRC_3) &&
+			op->cmd.dtr && op->addr.dtr &&
+			op->dummy.dtr && op->data.dtr) {
+		reg = fspi_readl(f, f->iobase + FSPI_MCR0);
+		reg |= FSPI_MCR0_RXCLKSRC(3);
+		fspi_writel(f, reg, f->iobase + FSPI_MCR0);
+		f->flags |= FSPI_RXCLKSRC_3;
+	} else if ((f->flags & FSPI_RXCLKSRC_3) &&
+			!op->cmd.dtr && !op->addr.dtr &&
+			!op->dummy.dtr && !op->data.dtr) {
+		reg = fspi_readl(f, f->iobase + FSPI_MCR0);
+		reg &= ~FSPI_MCR0_RXCLKSRC(3);	/* select mode 0 */
+		fspi_writel(f, reg, f->iobase + FSPI_MCR0);
+		f->flags &= ~FSPI_RXCLKSRC_3;
+	}
+
+}
+
 static int nxp_fspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 {
 	struct nxp_fspi *f = spi_controller_get_devdata(mem->spi->controller);
@@ -948,6 +993,8 @@ static int nxp_fspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 
 	nxp_fspi_select_mem(f, mem->spi);
 
+	nxp_fspi_select_rx_sample_clk_source(f, op);
+
 	nxp_fspi_prepare_lut(f, op);
 	/*
 	 * If we have large chunks of data, we read them through the AHB bus by
-- 
2.34.1

Re: [PATCH 4/5] spi: spi-nxp-fspi: add function to select sample clock source for flash reading

[Michael Walle]

> From: Haibo Chen <haibo.chen@nxp.com>
> 
> fspi define four mode for sample clock source selection.
> 
> Here is the list of modes:
> mode 0: Dummy Read strobe generated by FlexSPI Controller and loopback internally
> mode 1: Dummy Read strobe generated by FlexSPI Controller and loopback from DQS pad
> mode 2: Reserved
> mode 3: Flash provided Read strobe and input from DQS pad
> 
> In default, fspi use mode 0 after reset.
> For 8-8-8-DTR mode, need to use mode 3, otherwise 8-8-8-DTR read always
> get incorrect data.

I'd say this is board dependant, right? If you now hardcode 8d8d8d
to always use mode 3. I'm not sure how a board which doesn't have
the DQS connected to the flash can change this to another mode
again. Looks like we'd need a (DT) property which tells you if
there is actually a DQS line connected to the flash.

Btw you don't check buswidth, so you'll enable that mode for any
DTR mode.

-michael

RE: [PATCH 4/5] spi: spi-nxp-fspi: add function to select sample clock source for flash reading

[Bough Chen]

> -----Original Message-----
> From: Michael Walle <mwalle@kernel.org>
> Sent: 2023年12月14日 1:21
> To: Bough Chen <haibo.chen@nxp.com>
> Cc: broonie@kernel.org; Han Xu <han.xu@nxp.com>; dl-linux-imx
> <linux-imx@nxp.com>; linux-spi@vger.kernel.org; yogeshgaur.83@gmail.com;
> Michael Walle <mwalle@kernel.org>
> Subject: Re: [PATCH 4/5] spi: spi-nxp-fspi: add function to select sample clock
> source for flash reading
> 
> > From: Haibo Chen <haibo.chen@nxp.com>
> >
> > fspi define four mode for sample clock source selection.
> >
> > Here is the list of modes:
> > mode 0: Dummy Read strobe generated by FlexSPI Controller and loopback
> > internally mode 1: Dummy Read strobe generated by FlexSPI Controller
> > and loopback from DQS pad mode 2: Reserved mode 3: Flash provided Read
> > strobe and input from DQS pad
> >
> > In default, fspi use mode 0 after reset.
> > For 8-8-8-DTR mode, need to use mode 3, otherwise 8-8-8-DTR read
> > always get incorrect data.
> 
> I'd say this is board dependant, right? If you now hardcode 8d8d8d to always use
> mode 3. I'm not sure how a board which doesn't have the DQS connected to the
> flash can change this to another mode again. Looks like we'd need a (DT)
> property which tells you if there is actually a DQS line connected to the flash.

Currently we distinguish through SoC chip, not board. Like patch5.
If SoC contain the DQS, but the board do not connect it to flash device, then this is a real issue.
But I think if user use one octal flash device which support dtr mode, they should connect this DQS pad if want to work in DTR mode.
If forget to connect the DQS pad, they can limit the tx/rx buswidth to 4 or 1 in dts.

Anyway, add a DT property seems better. DQS is a must requirement for octal dtr mode, if detect no DQS, we can also disable the DTR mode support.
Will add in next version.


>
> Btw you don't check buswidth, so you'll enable that mode for any DTR mode.

Seems current spi-nor code only support one DTR mode, that is 8d-8d-8d.

Best Regards
Haibo Chen

> 
> -michael

Re: [PATCH 4/5] spi: spi-nxp-fspi: add function to select sample clock source for flash reading

[Michael Walle]

Hi,

>> > From: Haibo Chen <haibo.chen@nxp.com>
>> >
>> > fspi define four mode for sample clock source selection.
>> >
>> > Here is the list of modes:
>> > mode 0: Dummy Read strobe generated by FlexSPI Controller and loopback
>> > internally mode 1: Dummy Read strobe generated by FlexSPI Controller
>> > and loopback from DQS pad mode 2: Reserved mode 3: Flash provided Read
>> > strobe and input from DQS pad
>> >
>> > In default, fspi use mode 0 after reset.
>> > For 8-8-8-DTR mode, need to use mode 3, otherwise 8-8-8-DTR read
>> > always get incorrect data.
>> 
>> I'd say this is board dependant, right? If you now hardcode 8d8d8d to 
>> always use
>> mode 3. I'm not sure how a board which doesn't have the DQS connected 
>> to the
>> flash can change this to another mode again. Looks like we'd need a 
>> (DT)
>> property which tells you if there is actually a DQS line connected to 
>> the flash.
> 
> Currently we distinguish through SoC chip, not board. Like patch5.
> If SoC contain the DQS, but the board do not connect it to flash 
> device, then this is a real issue.

See below, there are alternatives to the DQS pin.

But it really is frequency dependent. I'd bet you can use mode 0
with a slow frequency in 8d8d8d mode. I.e. the LS1028ARM will always
refer you to mode 3 if you want to archive "the highest frequency".

> But I think if user use one octal flash device which support dtr mode, 
> they should
> connect this DQS pad if want to work in DTR mode.
> If forget to connect the DQS pad, they can limit the tx/rx buswidth to 
> 4 or 1 in dts.
> 
> Anyway, add a DT property seems better.
> 
> DQS is a must requirement for octal dtr mode, if detect no DQS, we can 
> also disable the DTR mode support.

I don't think this is true in general. I haven't checked with the FSPI
controller. But DQS is used for higher frequencies which isn't 
necessarily
related to DTR.

Also, there are other methods, like manual calibration of the delay 
lines
on the I/Os. There was once a patchset to add that calibration for a TI 
(?)
controller, but it was never merged. I've seen the fspi also supports
some DLL configuration and some kind of data learning feature. As far as
I understand that, these are all alternatives. I.e.
  (1) don't use high frequencies
  (2) use DQS (driven by the flash)
  (3) manually/automatically set the DLL

> Will add in next version.
> 
>> Btw you don't check buswidth, so you'll enable that mode for any DTR 
>> mode.
> 
> Seems current spi-nor code only support one DTR mode, that is 8d-8d-8d.

Yes. But that doesn't mean the spi controller should assume that. If you
don't want to support any other mode, you should probably check that in
your .supports_op() callback.

-michael

[PATCH 5/5] spi: spi-nxp-fspi: Add quirk to disable DTR support

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

Not all platform currently supports octal DTR mode. lx2160a do not
implement DQS, this causes flash probe failure and therefore, provide
an option of quirk FSPI_QUIRK_DISABLE_DTR for platforms not support
DTR mode.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/spi/spi-nxp-fspi.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
index 0330454b76c6..3d470129a477 100644
--- a/drivers/spi/spi-nxp-fspi.c
+++ b/drivers/spi/spi-nxp-fspi.c
@@ -340,6 +340,9 @@
 /* Access flash memory using IP bus only */
 #define FSPI_QUIRK_USE_IP_ONLY	BIT(0)
 
+/* Disable DTR */
+#define FSPI_QUIRK_DISABLE_DTR	BIT(1)
+
 struct nxp_fspi_devtype_data {
 	unsigned int rxfifo;
 	unsigned int txfifo;
@@ -352,7 +355,7 @@ static struct nxp_fspi_devtype_data lx2160a_data = {
 	.rxfifo = SZ_512,       /* (64  * 64 bits)  */
 	.txfifo = SZ_1K,        /* (128 * 64 bits)  */
 	.ahb_buf_size = SZ_2K,  /* (256 * 64 bits)  */
-	.quirks = 0,
+	.quirks = FSPI_QUIRK_DISABLE_DTR,
 	.little_endian = true,  /* little-endian    */
 };
 
@@ -1211,10 +1214,14 @@ static const struct spi_controller_mem_ops nxp_fspi_mem_ops = {
 	.get_name = nxp_fspi_get_name,
 };
 
-static struct spi_controller_mem_caps nxp_fspi_mem_caps = {
+static const struct spi_controller_mem_caps nxp_fspi_mem_caps = {
 	.dtr = true,
 };
 
+static const struct spi_controller_mem_caps nxp_fspi_mem_caps_quirks = {
+	.dtr = false,
+};
+
 static int nxp_fspi_probe(struct platform_device *pdev)
 {
 	struct spi_controller *ctlr;
@@ -1317,7 +1324,10 @@ static int nxp_fspi_probe(struct platform_device *pdev)
 	ctlr->bus_num = -1;
 	ctlr->num_chipselect = NXP_FSPI_MAX_CHIPSELECT;
 	ctlr->mem_ops = &nxp_fspi_mem_ops;
-	ctlr->mem_caps = &nxp_fspi_mem_caps;
+	if (f->devtype_data->quirks & FSPI_QUIRK_DISABLE_DTR)
+		ctlr->mem_caps = &nxp_fspi_mem_caps_quirks;
+	else
+		ctlr->mem_caps = &nxp_fspi_mem_caps;
 
 	nxp_fspi_default_setup(f);
 
-- 
2.34.1