diff mbox series

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

Message ID 20231213091346.956789-3-haibo.chen@nxp.com (mailing list archive)
State New, archived
Headers show
Series [1/5] spi: spi-nxp-fspi: enable runtime pm for fspi | expand

Commit Message

Bough Chen Dec. 13, 2023, 9:13 a.m. UTC 
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(-)

Comments

Michael Walle Dec. 13, 2023, 4:53 p.m. UTC | #1 
> 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
Bough Chen Dec. 14, 2023, 3:09 a.m. UTC | #2 
> -----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
Michael Walle Dec. 14, 2023, 9:45 a.m. UTC | #3 
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
diff mbox series

Patch

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