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

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

[Tim Harvey]

On Wed, Feb 26, 2020 at 2:19 AM Marc Kleine-Budde <mkl@pengutronix.de> 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

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

[Mark Brown]

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

On Thu, May 21, 2020 at 01:19:16PM -0700, Tim Harvey wrote:

> 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?

It seems likely that making the transfers explicitly half duplex will
perform better, especially for PIO controllers since there's less FIFO
stuffing to do but also just generally on longer messages.  You will
get some overhead setting up two transfers on write then read messages
which might offset that but my best guess would be that it'll be
negligable on most controllers.  It's also just a more accurate
representation of what the transfers are actually doing which seems
nicer.

If there *is* a performance win for doing full duplex messages on some
controllers we should probably look at optimizing this in the SPI core
since it'll affect a wide range of hardware and we already have some
code for forcing full duplex anyway.

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

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

[Marc Kleine-Budde]

On 5/21/20 10:19 PM, Tim Harvey wrote:
> On Wed, Feb 26, 2020 at 2:19 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> 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)

If the hardware doesn't support full duplex transfers you should add
SPI_CONTROLLER_HALF_DUPLEX to the driver.

If the hardware supports full duplex, but the driver doesn't the driver should
be fixed, or SPI_CONTROLLER_HALF_DUPLEX with the appropriate explanation.

> 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.

nice!

> 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?

Please make half duplex transfers depending on SPI_CONTROLLER_HALF_DUPLEX as
most drivers have a considerable overhead at the end of a transfer.

Most of them wait for a transfer complete interrupt. Which might take longer
than the actual SPI transfer. Splitting one full duplex read-register transfer
(which is a write followed by a read) into two half duplex transfers would kill
performance on full duplex capable controllers.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

[Mark Brown]

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

On Mon, May 25, 2020 at 01:17:01PM +0200, Marc Kleine-Budde wrote:
> On 5/21/20 10:19 PM, Tim Harvey wrote:

> 
> > 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?

> Please make half duplex transfers depending on SPI_CONTROLLER_HALF_DUPLEX as
> most drivers have a considerable overhead at the end of a transfer.

> Most of them wait for a transfer complete interrupt. Which might take longer
> than the actual SPI transfer. Splitting one full duplex read-register transfer
> (which is a write followed by a read) into two half duplex transfers would kill
> performance on full duplex capable controllers.

This isn't something that every individual driver should be doing, such
rewriting should happen in the core so that everything sees the benefit.

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

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

[Marc Kleine-Budde]

On 5/25/20 1:31 PM, Mark Brown wrote:
>>> 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?
> 
>> Please make half duplex transfers depending on SPI_CONTROLLER_HALF_DUPLEX as
>> most drivers have a considerable overhead at the end of a transfer.
> 
>> Most of them wait for a transfer complete interrupt. Which might take longer
>> than the actual SPI transfer. Splitting one full duplex read-register transfer
>> (which is a write followed by a read) into two half duplex transfers would kill
>> performance on full duplex capable controllers.
> 
> This isn't something that every individual driver should be doing, such
> rewriting should happen in the core so that everything sees the benefit.

The core could merge several half duplex transfers (until there's as cs_change)
into a single full duplex transfer.

I think it's not easy to detect and reliable to split a full duplex transfer
into half duplex ones. How can you tell, if the controller is supposed to tx 0x0
or actually receive.

I think spi_write_then_read() can be extended to generate one full duplex
transfer instead on two half duplex ones it does a memcpy() anyways.

To get a feeling for the use cases, this is what I do in the regmap read
function of a (not yet mainlined) CAN SPI driver.

> static int
> mcp25xxfd_regmap_nocrc_read(void *context,
> 			    const void *reg, size_t reg_len,
> 			    void *val_buf, size_t val_len)
> {
> 	struct spi_device *spi = context;
> 	struct mcp25xxfd_priv *priv = spi_get_drvdata(spi);
> 	struct mcp25xxfd_map_buf_nocrc *buf_rx = priv->map_buf_nocrc_rx;
> 	struct mcp25xxfd_map_buf_nocrc *buf_tx = priv->map_buf_nocrc_tx;
> 	struct spi_transfer xfer[2] = { };
> 	struct spi_message msg;
> 	int err;
> 
> 	spi_message_init(&msg);
> 	spi_message_add_tail(&xfer[0], &msg);
> 
> 	if (priv->devtype_data.quirks & MCP25XXFD_QUIRK_HALF_DUPLEX) {
> 		xfer[0].tx_buf = reg;
> 		xfer[0].len = sizeof(buf_tx->cmd);
> 
> 		xfer[1].rx_buf = val_buf;
> 		xfer[1].len = val_len;
> 		spi_message_add_tail(&xfer[1], &msg);
> 	} else {
> 		xfer[0].tx_buf = buf_tx;
> 		xfer[0].rx_buf = buf_rx;
> 		xfer[0].len = sizeof(buf_tx->cmd) + val_len;
> 		memcpy(&buf_tx->cmd, reg, sizeof(buf_tx->cmd));
> 	};
> 
> 	err = spi_sync(spi, &msg);
> 	if (err)
> 		return err;
> 
> 	if (!(priv->devtype_data.quirks & MCP25XXFD_QUIRK_HALF_DUPLEX))
> 		memcpy(val_buf, buf_rx->data, val_len);
> 
> 	return 0;
> }

The tradeoff here is two transfers vs. the the memcpy(). As CAN frames are quite
small the memcpy() is usually faster. Even on the rpi, where the driver is
optimized for small transfers.

regards
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

[Mark Brown]

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

On Mon, May 25, 2020 at 02:41:31PM +0200, Marc Kleine-Budde wrote:
> On 5/25/20 1:31 PM, Mark Brown wrote:

> > This isn't something that every individual driver should be doing, such
> > rewriting should happen in the core so that everything sees the benefit.

> The core could merge several half duplex transfers (until there's as cs_change)
> into a single full duplex transfer.

Yes, that is what I am suggesting.

> I think it's not easy to detect and reliable to split a full duplex transfer
> into half duplex ones. How can you tell, if the controller is supposed to tx 0x0
> or actually receive.

I don't understand how that could possibly work or why it would make
sense?

> I think spi_write_then_read() can be extended to generate one full duplex
> transfer instead on two half duplex ones it does a memcpy() anyways.

This has the same problem as doing it in any other driver code - it
causes a needless incompatibility with three wire and single duplex
devices.  

> To get a feeling for the use cases, this is what I do in the regmap read
> function of a (not yet mainlined) CAN SPI driver.

Like I say it's probably better if code like this gets pushed into the
SPI core where we've got more information about what the controller can
do and there's more win from doing the tuning since more devices and
systems can take advantage of it.

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

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

[Marc Kleine-Budde]

On 5/25/20 2:57 PM, Mark Brown wrote:
> On Mon, May 25, 2020 at 02:41:31PM +0200, Marc Kleine-Budde wrote:
>> On 5/25/20 1:31 PM, Mark Brown wrote:
> 
>>> This isn't something that every individual driver should be doing, such
>>> rewriting should happen in the core so that everything sees the benefit.
> 
>> The core could merge several half duplex transfers (until there's as cs_change)
>> into a single full duplex transfer.
> 
> Yes, that is what I am suggesting.

Where in the SPI stack do you see such a "merge" function? One point to clarify
is when and where to allocate and free the memory for the contiguous full duplex
buffers.

>> I think it's not easy to detect and reliable to split a full duplex transfer
>> into half duplex ones. How can you tell, if the controller is supposed to tx 0x0
>> or actually receive.
> 
> I don't understand how that could possibly work or why it would make
> sense?

ACK, I was just thinking loud about options.

>> I think spi_write_then_read() can be extended to generate one full duplex
>> transfer instead on two half duplex ones it does a memcpy() anyways.
> 
> This has the same problem as doing it in any other driver code - it
> causes a needless incompatibility with three wire and single duplex
> devices.  

What about the note "portable code should never use this for more than 32 bytes"
in spi_write_then_read()? The CAN driver in question may read more than 32 bytes
of data.

>> To get a feeling for the use cases, this is what I do in the regmap read
>> function of a (not yet mainlined) CAN SPI driver.
> 
> Like I say it's probably better if code like this gets pushed into the
> SPI core where we've got more information about what the controller can
> do and there's more win from doing the tuning since more devices and
> systems can take advantage of it.

ACK

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

[Mark Brown]

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

On Mon, May 25, 2020 at 03:12:05PM +0200, Marc Kleine-Budde wrote:
> On 5/25/20 2:57 PM, Mark Brown wrote:
> > On Mon, May 25, 2020 at 02:41:31PM +0200, Marc Kleine-Budde wrote:
> >> On 5/25/20 1:31 PM, Mark Brown wrote:

> >> The core could merge several half duplex transfers (until there's as cs_change)
> >> into a single full duplex transfer.

> > Yes, that is what I am suggesting.

> Where in the SPI stack do you see such a "merge" function? One point to clarify
> is when and where to allocate and free the memory for the contiguous full duplex
> buffers.

My first thought would be about the same point as we're rewriting to
handle MUST_TX and MUST_RX in map_msg() which does similar allocations
and deallocations to insert dummy data for controllers that need them.

> >> I think spi_write_then_read() can be extended to generate one full duplex
> >> transfer instead on two half duplex ones it does a memcpy() anyways.

> > This has the same problem as doing it in any other driver code - it
> > causes a needless incompatibility with three wire and single duplex
> > devices.  

> What about the note "portable code should never use this for more than 32 bytes"
> in spi_write_then_read()? The CAN driver in question may read more than 32 bytes
> of data.

I think that comment is actually not valid any more - we used to use a
fixed statically allocated buffer in write_then_read() but added the
option to fall back onto allocating one dynamically if another user was
running or the transfer was too big.

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