[PATCH v1 0/2] spi: npcm-pspi: add Arbel NPCM8XX and full duplex support

[PATCH v1 0/2] spi: npcm-pspi: add Arbel NPCM8XX and full duplex support

[Tomer Maimon]

This patch set add the following support to the NPCM Peripheral SPI (PSPI) 
driver:
 - Arbel NPCM8XX.
 - Full duplex.

The NPCM PSPI driver tested on NPCM845 evaluation board.

Tomer Maimon (2):
  spi: npcm-pspi: add full duplex support
  dt-binding: spi: npcm-pspi: Add npcm845 compatible

 .../bindings/spi/nuvoton,npcm-pspi.txt        |  3 +-
 drivers/spi/spi-npcm-pspi.c                   | 75 ++++++++-----------
 2 files changed, 32 insertions(+), 46 deletions(-)

-- 
2.33.0

[PATCH v1 1/2] spi: npcm-pspi: add full duplex support

[Tomer Maimon]

The NPCM PSPI handler, on TX-buffer not null, would perform a dummy read
but did not save the rx-data, this was valid only for half duplex.

This patch adds full duplex support for NPCM PSPI driver by storing all
rx-data when the Rx-buffer is defined also for TX-buffer handling.

Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
 drivers/spi/spi-npcm-pspi.c | 75 +++++++++++++++----------------------
 1 file changed, 30 insertions(+), 45 deletions(-)

diff --git a/drivers/spi/spi-npcm-pspi.c b/drivers/spi/spi-npcm-pspi.c
index 1668a347e003..02f0fcceaf19 100644
--- a/drivers/spi/spi-npcm-pspi.c
+++ b/drivers/spi/spi-npcm-pspi.c
@@ -195,22 +195,22 @@ static void npcm_pspi_setup_transfer(struct spi_device *spi,
 static void npcm_pspi_send(struct npcm_pspi *priv)
 {
 	int wsize;
-	u16 val;
+	u16 val = 0;
 
 	wsize = min(bytes_per_word(priv->bits_per_word), priv->tx_bytes);
 	priv->tx_bytes -= wsize;
 
-	if (!priv->tx_buf)
-		return;
-
 	switch (wsize) {
 	case 1:
-		val = *priv->tx_buf++;
+		if (priv->tx_buf)
+			val = *priv->tx_buf++;
 		iowrite8(val, NPCM_PSPI_DATA + priv->base);
 		break;
 	case 2:
-		val = *priv->tx_buf++;
-		val = *priv->tx_buf++ | (val << 8);
+		if (priv->tx_buf) {
+			val = *priv->tx_buf++;
+			val = *priv->tx_buf++ | (val << 8);
+		}
 		iowrite16(val, NPCM_PSPI_DATA + priv->base);
 		break;
 	default:
@@ -222,22 +222,24 @@ static void npcm_pspi_send(struct npcm_pspi *priv)
 static void npcm_pspi_recv(struct npcm_pspi *priv)
 {
 	int rsize;
-	u16 val;
+	u16 val_16;
+	u8  val_8;
 
 	rsize = min(bytes_per_word(priv->bits_per_word), priv->rx_bytes);
 	priv->rx_bytes -= rsize;
 
-	if (!priv->rx_buf)
-		return;
-
 	switch (rsize) {
 	case 1:
-		*priv->rx_buf++ = ioread8(priv->base + NPCM_PSPI_DATA);
+		val_8 = ioread8(priv->base + NPCM_PSPI_DATA);
+		if (priv->rx_buf)
+			*priv->rx_buf++ = val_8;
 		break;
 	case 2:
-		val = ioread16(priv->base + NPCM_PSPI_DATA);
-		*priv->rx_buf++ = (val >> 8);
-		*priv->rx_buf++ = val & 0xff;
+		val_16 = ioread16(priv->base + NPCM_PSPI_DATA);
+		if (priv->rx_buf) {
+			*priv->rx_buf++ = (val_16 >> 8);
+			*priv->rx_buf++ = val_16 & 0xff;
+		}
 		break;
 	default:
 		WARN_ON_ONCE(1);
@@ -296,43 +298,26 @@ static irqreturn_t npcm_pspi_handler(int irq, void *dev_id)
 	struct npcm_pspi *priv = dev_id;
 	u8 stat;
 
-	stat = ioread8(priv->base + NPCM_PSPI_STAT);
-
 	if (!priv->tx_buf && !priv->rx_buf)
 		return IRQ_NONE;
 
-	if (priv->tx_buf) {
-		if (stat & NPCM_PSPI_STAT_RBF) {
-			ioread8(NPCM_PSPI_DATA + priv->base);
-			if (priv->tx_bytes == 0) {
-				npcm_pspi_disable(priv);
-				complete(&priv->xfer_done);
-				return IRQ_HANDLED;
-			}
-		}
-
-		if ((stat & NPCM_PSPI_STAT_BSY) == 0)
-			if (priv->tx_bytes)
-				npcm_pspi_send(priv);
+	if (priv->tx_bytes == 0 && priv->rx_bytes == 0) {
+		npcm_pspi_disable(priv);
+		complete(&priv->xfer_done);
+		return IRQ_HANDLED;
 	}
 
-	if (priv->rx_buf) {
-		if (stat & NPCM_PSPI_STAT_RBF) {
-			if (!priv->rx_bytes)
-				return IRQ_NONE;
-
-			npcm_pspi_recv(priv);
+	stat = ioread8(priv->base + NPCM_PSPI_STAT);
 
-			if (!priv->rx_bytes) {
-				npcm_pspi_disable(priv);
-				complete(&priv->xfer_done);
-				return IRQ_HANDLED;
-			}
-		}
+	/*
+	 * first we do the read since if we do the write we previous read might
+	 * be lost (indeed low chances)
+	 */
+	if ((stat & NPCM_PSPI_STAT_RBF) && priv->rx_bytes)
+		npcm_pspi_recv(priv);
 
-		if (((stat & NPCM_PSPI_STAT_BSY) == 0) && !priv->tx_buf)
-			iowrite8(0x0, NPCM_PSPI_DATA + priv->base);
-	}
+	if (((stat & NPCM_PSPI_STAT_BSY) == 0) && priv->tx_bytes)
+		npcm_pspi_send(priv);
 
 	return IRQ_HANDLED;
 }
-- 
2.33.0

[PATCH v1 2/2] dt-binding: spi: npcm-pspi: Add npcm845 compatible

[Tomer Maimon]

Add a compatible string for Nuvoton BMC NPCM845 PSPI.

Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
 Documentation/devicetree/bindings/spi/nuvoton,npcm-pspi.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/spi/nuvoton,npcm-pspi.txt b/Documentation/devicetree/bindings/spi/nuvoton,npcm-pspi.txt
index b98203ca656d..a4e72e52af59 100644
--- a/Documentation/devicetree/bindings/spi/nuvoton,npcm-pspi.txt
+++ b/Documentation/devicetree/bindings/spi/nuvoton,npcm-pspi.txt
@@ -3,7 +3,8 @@ Nuvoton NPCM Peripheral Serial Peripheral Interface(PSPI) controller driver
 Nuvoton NPCM7xx SOC support two PSPI channels.
 
 Required properties:
- - compatible : "nuvoton,npcm750-pspi" for NPCM7XX BMC
+ - compatible : "nuvoton,npcm750-pspi" for Poleg NPCM7XX.
+				"nuvoton,npcm845-pspi" for Arbel NPCM8XX.
  - #address-cells : should be 1. see spi-bus.txt
  - #size-cells : should be 0. see spi-bus.txt
  - specifies physical base address and size of the register.
-- 
2.33.0

Re: [PATCH v1 2/2] dt-binding: spi: npcm-pspi: Add npcm845 compatible

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 359 bytes --]

On Thu, Jul 21, 2022 at 01:15:56PM +0300, Tomer Maimon wrote:

>  Required properties:
> - - compatible : "nuvoton,npcm750-pspi" for NPCM7XX BMC
> + - compatible : "nuvoton,npcm750-pspi" for Poleg NPCM7XX.
> +				"nuvoton,npcm845-pspi" for Arbel NPCM8XX.

You've not updated the driver to accept this compatible and this doesn't
say anything about fallbacks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 0/2] spi: npcm-pspi: add Arbel NPCM8XX and full duplex support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 817 bytes --]

On Thu, Jul 21, 2022 at 01:15:54PM +0300, Tomer Maimon wrote:

> Tomer Maimon (2):
>   spi: npcm-pspi: add full duplex support
>   dt-binding: spi: npcm-pspi: Add npcm845 compatible

It is not obvious why these are a series, they appear to be entirely
orthogonal.  If there's no relationship between patches it's generally
better to send them separately, that way problems with one patch won't
hold up unrelated patches and reviewers aren't left wondering about why
things are grouped.

Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/2] spi: npcm-pspi: add full duplex support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2126 bytes --]

On Thu, Jul 21, 2022 at 01:15:55PM +0300, Tomer Maimon wrote:

> The NPCM PSPI handler, on TX-buffer not null, would perform a dummy read
> but did not save the rx-data, this was valid only for half duplex.

> This patch adds full duplex support for NPCM PSPI driver by storing all
> rx-data when the Rx-buffer is defined also for TX-buffer handling.

This doesn't seem to entirely correspond to what the patch does, nor to
what the driver currently does?  I can't see any dummy read code in the
current driver.

>  static void npcm_pspi_send(struct npcm_pspi *priv)
>  {
>  	int wsize;
> -	u16 val;
> +	u16 val = 0;
>  
>  	wsize = min(bytes_per_word(priv->bits_per_word), priv->tx_bytes);
>  	priv->tx_bytes -= wsize;
>  
> -	if (!priv->tx_buf)
> -		return;
> -
>  	switch (wsize) {
>  	case 1:
> -		val = *priv->tx_buf++;
> +		if (priv->tx_buf)
> +			val = *priv->tx_buf++;
>  		iowrite8(val, NPCM_PSPI_DATA + priv->base);
>  		break;

These changes appaear to be trying to ensure that when _send() is called
we now always write something out, even if there was no transmit buffer.
Since the device has been supporting half duplex transfers it is not
clear why we'd want to do that, it's adding overhead to the PIO which
isn't great.  This also isn't what the changelog said, the changelog
said we were adding reading of data when there's a transmit buffer.
Similar issues apply on the read side.

AFAICT the bulk of what the change is doing is trying make the driver
unconditionally do both read and writes to the hardware when it would
previously have only read or written data if there was a buffer
provided.  That's basically open coding SPI_CONTROLLER_MUST_TX and
SPI_CONTROLLER_MUST_RX, if that's what the hardware needs then you
should just set those flags and let the core fix things up.

> +       /*
> +        * first we do the read since if we do the write we previous read might
> +        * be lost (indeed low chances)
> +        */

This reordering sounds like it might be needed but should have been
mentioned in the changelog and is a separate patch.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 0/2] spi: npcm-pspi: add Arbel NPCM8XX and full duplex support

[Tomer Maimon]

Hi Mark,

Thanks for your comment, next version I will make sure to send two
separate patches

On Thu, 21 Jul 2022 at 15:19, Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Jul 21, 2022 at 01:15:54PM +0300, Tomer Maimon wrote:
>
> > Tomer Maimon (2):
> >   spi: npcm-pspi: add full duplex support
> >   dt-binding: spi: npcm-pspi: Add npcm845 compatible
>
> It is not obvious why these are a series, they appear to be entirely
> orthogonal.  If there's no relationship between patches it's generally
> better to send them separately, that way problems with one patch won't
> hold up unrelated patches and reviewers aren't left wondering about why
> things are grouped.
>
> Please submit patches using subject lines reflecting the style for the
> subsystem, this makes it easier for people to identify relevant patches.
> Look at what existing commits in the area you're changing are doing and
> make sure your subject lines visually resemble what they're doing.
> There's no need to resubmit to fix this alone.

Best regards,

Tomer

Re: [PATCH v1 2/2] dt-binding: spi: npcm-pspi: Add npcm845 compatible

[Tomer Maimon]

Hi Mark,

Thanks for your comment.

On Thu, 21 Jul 2022 at 15:09, Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Jul 21, 2022 at 01:15:56PM +0300, Tomer Maimon wrote:
>
> >  Required properties:
> > - - compatible : "nuvoton,npcm750-pspi" for NPCM7XX BMC
> > + - compatible : "nuvoton,npcm750-pspi" for Poleg NPCM7XX.
> > +                             "nuvoton,npcm845-pspi" for Arbel NPCM8XX.
>
> You've not updated the driver to accept this compatible and this doesn't
> say anything about fallbacks.

I will make sure to add the compatible to the driver

Thanks,

Tomer

Re: [PATCH v1 1/2] spi: npcm-pspi: add full duplex support

[Tomer Maimon]

Hi Mark,

Thanks for your detailed explanation!

On Thu, 21 Jul 2022 at 16:46, Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Jul 21, 2022 at 01:15:55PM +0300, Tomer Maimon wrote:
>
> > The NPCM PSPI handler, on TX-buffer not null, would perform a dummy read
> > but did not save the rx-data, this was valid only for half duplex.
>
> > This patch adds full duplex support for NPCM PSPI driver by storing all
> > rx-data when the Rx-buffer is defined also for TX-buffer handling.
>
> This doesn't seem to entirely correspond to what the patch does, nor to
> what the driver currently does?  I can't see any dummy read code in the
> current driver.
>
In the current handler file, in the handler function.
static irqreturn_t npcm_pspi_handler(int irq, void *dev_id)
....
-       if (priv->tx_buf) {
-               if (stat & NPCM_PSPI_STAT_RBF) {
-                       ioread8(NPCM_PSPI_DATA + priv->base);
the read above doing a dummy read
-                       if (priv->tx_bytes == 0) {
-                               npcm_pspi_disable(priv);
-                               complete(&priv->xfer_done);
-                               return IRQ_HANDLED;
-                       }
-               }


> >  static void npcm_pspi_send(struct npcm_pspi *priv)
> >  {
> >       int wsize;
> > -     u16 val;
> > +     u16 val = 0;
> >
> >       wsize = min(bytes_per_word(priv->bits_per_word), priv->tx_bytes);
> >       priv->tx_bytes -= wsize;
> >
> > -     if (!priv->tx_buf)
> > -             return;
> > -
> >       switch (wsize) {
> >       case 1:
> > -             val = *priv->tx_buf++;
> > +             if (priv->tx_buf)
> > +                     val = *priv->tx_buf++;
> >               iowrite8(val, NPCM_PSPI_DATA + priv->base);
> >               break;
>
> These changes appaear to be trying to ensure that when _send() is called
> we now always write something out, even if there was no transmit buffer.
> Since the device has been supporting half duplex transfers it is not
> clear why we'd want to do that, it's adding overhead to the PIO which
> isn't great.  This also isn't what the changelog said, the changelog
> said we were adding reading of data when there's a transmit buffer.
> Similar issues apply on the read side.
>
> AFAICT the bulk of what the change is doing is trying make the driver
> unconditionally do both read and writes to the hardware when it would
> previously have only read or written data if there was a buffer
> provided.  That's basically open coding SPI_CONTROLLER_MUST_TX and
> SPI_CONTROLLER_MUST_RX, if that's what the hardware needs then you
> should just set those flags and let the core fix things up.
We will try to use SPI_CONTROLLER_MUST_TX and SPI_CONTROLLER_MUST_RX
>
> > +       /*
> > +        * first we do the read since if we do the write we previous read might
> > +        * be lost (indeed low chances)
> > +        */
>
> This reordering sounds like it might be needed but should have been
> mentioned in the changelog and is a separate patch.

Best regards,

Tomer

Re: [PATCH v1 1/2] spi: npcm-pspi: add full duplex support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1004 bytes --]

On Sun, Jul 24, 2022 at 12:35:37PM +0300, Tomer Maimon wrote:
> On Thu, 21 Jul 2022 at 16:46, Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Jul 21, 2022 at 01:15:55PM +0300, Tomer Maimon wrote:

> > > This patch adds full duplex support for NPCM PSPI driver by storing all
> > > rx-data when the Rx-buffer is defined also for TX-buffer handling.

> > This doesn't seem to entirely correspond to what the patch does, nor to
> > what the driver currently does?  I can't see any dummy read code in the
> > current driver.

> In the current handler file, in the handler function.
> static irqreturn_t npcm_pspi_handler(int irq, void *dev_id)

> -       if (priv->tx_buf) {
> -               if (stat & NPCM_PSPI_STAT_RBF) {
> -                       ioread8(NPCM_PSPI_DATA + priv->base);

> the read above doing a dummy read

That's reading a single byte, not an entire buffer, and from a quick
glance looks more like an ack.  Though perhaps you just end up with a
lot of interrupts and do that anyway.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v1 1/2] spi: npcm-pspi: add full duplex support

[Tomer Maimon]

The NPCM PSPI handler, on TX-buffer not null, would perform a dummy read
but did not save the rx-data, this was valid only for half duplex.

This patch adds full duplex support for NPCM PSPI driver by storing all
rx-data when the Rx-buffer is defined also for TX-buffer handling.

Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
 drivers/spi/spi-npcm-pspi.c | 75 +++++++++++++++----------------------
 1 file changed, 30 insertions(+), 45 deletions(-)

diff --git a/drivers/spi/spi-npcm-pspi.c b/drivers/spi/spi-npcm-pspi.c
index 1668a347e003..02f0fcceaf19 100644
--- a/drivers/spi/spi-npcm-pspi.c
+++ b/drivers/spi/spi-npcm-pspi.c
@@ -195,22 +195,22 @@ static void npcm_pspi_setup_transfer(struct spi_device *spi,
 static void npcm_pspi_send(struct npcm_pspi *priv)
 {
 	int wsize;
-	u16 val;
+	u16 val = 0;
 
 	wsize = min(bytes_per_word(priv->bits_per_word), priv->tx_bytes);
 	priv->tx_bytes -= wsize;
 
-	if (!priv->tx_buf)
-		return;
-
 	switch (wsize) {
 	case 1:
-		val = *priv->tx_buf++;
+		if (priv->tx_buf)
+			val = *priv->tx_buf++;
 		iowrite8(val, NPCM_PSPI_DATA + priv->base);
 		break;
 	case 2:
-		val = *priv->tx_buf++;
-		val = *priv->tx_buf++ | (val << 8);
+		if (priv->tx_buf) {
+			val = *priv->tx_buf++;
+			val = *priv->tx_buf++ | (val << 8);
+		}
 		iowrite16(val, NPCM_PSPI_DATA + priv->base);
 		break;
 	default:
@@ -222,22 +222,24 @@ static void npcm_pspi_send(struct npcm_pspi *priv)
 static void npcm_pspi_recv(struct npcm_pspi *priv)
 {
 	int rsize;
-	u16 val;
+	u16 val_16;
+	u8  val_8;
 
 	rsize = min(bytes_per_word(priv->bits_per_word), priv->rx_bytes);
 	priv->rx_bytes -= rsize;
 
-	if (!priv->rx_buf)
-		return;
-
 	switch (rsize) {
 	case 1:
-		*priv->rx_buf++ = ioread8(priv->base + NPCM_PSPI_DATA);
+		val_8 = ioread8(priv->base + NPCM_PSPI_DATA);
+		if (priv->rx_buf)
+			*priv->rx_buf++ = val_8;
 		break;
 	case 2:
-		val = ioread16(priv->base + NPCM_PSPI_DATA);
-		*priv->rx_buf++ = (val >> 8);
-		*priv->rx_buf++ = val & 0xff;
+		val_16 = ioread16(priv->base + NPCM_PSPI_DATA);
+		if (priv->rx_buf) {
+			*priv->rx_buf++ = (val_16 >> 8);
+			*priv->rx_buf++ = val_16 & 0xff;
+		}
 		break;
 	default:
 		WARN_ON_ONCE(1);
@@ -296,43 +298,26 @@ static irqreturn_t npcm_pspi_handler(int irq, void *dev_id)
 	struct npcm_pspi *priv = dev_id;
 	u8 stat;
 
-	stat = ioread8(priv->base + NPCM_PSPI_STAT);
-
 	if (!priv->tx_buf && !priv->rx_buf)
 		return IRQ_NONE;
 
-	if (priv->tx_buf) {
-		if (stat & NPCM_PSPI_STAT_RBF) {
-			ioread8(NPCM_PSPI_DATA + priv->base);
-			if (priv->tx_bytes == 0) {
-				npcm_pspi_disable(priv);
-				complete(&priv->xfer_done);
-				return IRQ_HANDLED;
-			}
-		}
-
-		if ((stat & NPCM_PSPI_STAT_BSY) == 0)
-			if (priv->tx_bytes)
-				npcm_pspi_send(priv);
+	if (priv->tx_bytes == 0 && priv->rx_bytes == 0) {
+		npcm_pspi_disable(priv);
+		complete(&priv->xfer_done);
+		return IRQ_HANDLED;
 	}
 
-	if (priv->rx_buf) {
-		if (stat & NPCM_PSPI_STAT_RBF) {
-			if (!priv->rx_bytes)
-				return IRQ_NONE;
-
-			npcm_pspi_recv(priv);
+	stat = ioread8(priv->base + NPCM_PSPI_STAT);
 
-			if (!priv->rx_bytes) {
-				npcm_pspi_disable(priv);
-				complete(&priv->xfer_done);
-				return IRQ_HANDLED;
-			}
-		}
+	/*
+	 * first we do the read since if we do the write we previous read might
+	 * be lost (indeed low chances)
+	 */
+	if ((stat & NPCM_PSPI_STAT_RBF) && priv->rx_bytes)
+		npcm_pspi_recv(priv);
 
-		if (((stat & NPCM_PSPI_STAT_BSY) == 0) && !priv->tx_buf)
-			iowrite8(0x0, NPCM_PSPI_DATA + priv->base);
-	}
+	if (((stat & NPCM_PSPI_STAT_BSY) == 0) && priv->tx_bytes)
+		npcm_pspi_send(priv);
 
 	return IRQ_HANDLED;
 }
-- 
2.33.0