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 <[email protected]>
---
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
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.
Hi Mark,
Thanks for your detailed explanation!
On Thu, 21 Jul 2022 at 16:46, Mark Brown <[email protected]> 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
On Sun, Jul 24, 2022 at 12:35:37PM +0300, Tomer Maimon wrote:
> On Thu, 21 Jul 2022 at 16:46, Mark Brown <[email protected]> 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.