Re: [PATCH] can: mcp251x: convert to half-duplex SPI

[Tim Harvey <tharvey@xxxxxxxxxxxxx>]

On Wed, Feb 26, 2020 at 2:19 AM Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote:
>
> On 2/26/20 8:37 AM, Marc Kleine-Budde wrote:
> >> Your right... there is the mcp251x_hw_rx_frame() call that also uses
> >> spi_rx_buf after a synchronous transfer (I didn't see any others).
> >> I'll look at this again.
> >
> > Have you hardware to test your changes? I think the SPI framework would
> > return an -EINVAL in that case....though the return value is sometimes
> > not checked by the driver :/
>
> See https://elixir.bootlin.com/linux/v5.5.6/source/drivers/spi/spi.c#L3413
>
> If you have really have HW with SPI_CONTROLLER_HALF_DUPLEX (a.k.a
> SPI_MASTER_HALF_DUPLEX) restrictions, you need to convert _every_
> mcp251x_spi_trans() call in the driver, as _always_ both rx_buf _and_
> tx_buf are used.
>

Marc,

Sorry for the long delay... I'm finally getting back to this issue.

I'm told by Marvell/Cavium that the OcteonTX SPI hardware does not
support full duplex although I don't see this in any of their errata
or reference manuals. Perhaps someone familiar with the CN81xx/CN80xx
OcteonTX hardware from Marvell/Cavium can weigh in here as I'm not
clear if this limitation is in all hardware that uses the
spi-cavium-thunderx.c driver (I've added Jan to the list who authored
the driver)

As you point out setting SPI_CONTROLLER_HALF_DUPLEX will cause
spi_{sync,async,async_locked} calls to fail with -EINVAL if they have
both a tx and rx buf, so this should be done to help catch these
issues:
diff --git a/drivers/spi/spi-cavium-thunderx.c
b/drivers/spi/spi-cavium-thunderx.c
index fd6b9ca..76fdb94 100644
--- a/drivers/spi/spi-cavium-thunderx.c
+++ b/drivers/spi/spi-cavium-thunderx.c
@@ -64,6 +65,7 @@ static int thunderx_spi_probe(struct pci_dev *pdev,
                p->sys_freq = SYS_FREQ_DEFAULT;
        dev_info(dev, "Set system clock to %u\n", p->sys_freq);

+       master->flags = SPI_MASTER_HALF_DUPLEX;
        master->num_chipselect = 4;
        master->mode_bits = SPI_CPHA | SPI_CPOL | SPI_CS_HIGH |
                            SPI_LSB_FIRST | SPI_3WIRE;

Now, with regards to the mcp251x.c driver you were correct that I was
missing dealing with the full-duplex call from mcp251x_hw_rx_frame()
which indeed was causing data corruption on recieve.

So the following patch to mcp251x.c properly converts mcp251x to half-duplex:

diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
index 5009ff2..016c1e5 100644
--- a/drivers/net/can/spi/mcp251x.c
+++ b/drivers/net/can/spi/mcp251x.c
@@ -290,23 +290,23 @@ static u8 mcp251x_read_reg(struct spi_device *spi, u8 reg)
        priv->spi_tx_buf[0] = INSTRUCTION_READ;
        priv->spi_tx_buf[1] = reg;

-       mcp251x_spi_trans(spi, 3);
-       val = priv->spi_rx_buf[2];
+       spi_write_then_read(spi, priv->spi_tx_buf, 2, &val, 1);

        return val;
 }

 static void mcp251x_read_2regs(struct spi_device *spi, u8 reg, u8 *v1, u8 *v2)
 {
+       u8 val[2] = {0};
        struct mcp251x_priv *priv = spi_get_drvdata(spi);

        priv->spi_tx_buf[0] = INSTRUCTION_READ;
        priv->spi_tx_buf[1] = reg;

-       mcp251x_spi_trans(spi, 4);
+       spi_write_then_read(spi, priv->spi_tx_buf, 2, val, 2);

-       *v1 = priv->spi_rx_buf[2];
-       *v2 = priv->spi_rx_buf[3];
+       *v1 = val[0];
+       *v2 = val[1];
 }

 static void mcp251x_write_reg(struct spi_device *spi, u8 reg, u8 val)
@@ -409,8 +409,9 @@ static void mcp251x_hw_rx_frame(struct spi_device
*spi, u8 *buf,
                        buf[i] = mcp251x_read_reg(spi, RXBCTRL(buf_idx) + i);
        } else {
                priv->spi_tx_buf[RXBCTRL_OFF] = INSTRUCTION_READ_RXB(buf_idx);
-               mcp251x_spi_trans(spi, SPI_TRANSFER_BUF_LEN);
-               memcpy(buf, priv->spi_rx_buf, SPI_TRANSFER_BUF_LEN);
+               spi_write_then_read(spi, priv->spi_tx_buf, 1, priv->spi_rx_buf,
+                                   SPI_TRANSFER_BUF_LEN);
+               memcpy(buf + 1, priv->spi_rx_buf, SPI_TRANSFER_BUF_LEN - 1);
        }
 }

I do have hardware to test with and without this patch my CN80xx board
with an MCP25625 fails device probing (mcp251x spi0.1: MCP251x didn't
enter in conf mode after reset) because read values are corrupt. With
this patch my the MCP25625 works fine on the CN80xx detecting,
sending, and receiving frames.

Should I be submitting this patch with logic that only does
half-duplex if the spi controller doesn't support it (if
(spi->controller->flags & SPI_CONTROLLER_HALF_DUPLEX)) or is it
acceptable to simply make the driver half-duplex like this for all
cases?

Best Regards,

Tim