Re: MCP251x SPI CAN controller on Cavium ThunderX

Re: MCP251x SPI CAN controller on Cavium ThunderX

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1272 bytes --]

On 11/14/2017 01:02 PM, Mark Brown wrote:
> On Mon, Nov 13, 2017 at 01:17:42PM -0800, Tim Harvey wrote:
> 
>> When a register is read from the mcp251x driver the
>> octeon_spi_do_transfer() gets a spi_message with a single spi_xfer of
>> len=3, a tx_buf, and an rx_buf which I believe is supposed to shift
>> out 3 bytes out MOSI and shift in 3 bytes from MISO where the last
>> byte shifted in would be the response.
> 
> No, that will simultaneously transmit and recieve three bytes.

That's what the driver supposed to do.

> If you want to transmit two bytes and then recieve one byte you need
> two xfers, one with a len of 2 and a tx_buf, the other with a len of
> 1 and a rx_buf.
To read a register (mcp251x_read_reg()) the mcp251x does a 3 byte full
duplex transfer. The first byte send is the command (read register) the
second byte the register number the third byte is a dummy. The first 2
bytes received are ignored the 3rd byte is the register contents.

Marc

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


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

Re: MCP251x SPI CAN controller on Cavium ThunderX

[Jan Glauber]

On Wed, Nov 15, 2017 at 11:54:20AM +0100, Marc Kleine-Budde wrote:
> On 11/14/2017 01:02 PM, Mark Brown wrote:
> > On Mon, Nov 13, 2017 at 01:17:42PM -0800, Tim Harvey wrote:
> > 
> >> When a register is read from the mcp251x driver the
> >> octeon_spi_do_transfer() gets a spi_message with a single spi_xfer of
> >> len=3, a tx_buf, and an rx_buf which I believe is supposed to shift
> >> out 3 bytes out MOSI and shift in 3 bytes from MISO where the last
> >> byte shifted in would be the response.
> > 
> > No, that will simultaneously transmit and recieve three bytes.
> 
> That's what the driver supposed to do.
> 
> > If you want to transmit two bytes and then recieve one byte you need
> > two xfers, one with a len of 2 and a tx_buf, the other with a len of
> > 1 and a rx_buf.
> To read a register (mcp251x_read_reg()) the mcp251x does a 3 byte full
> duplex transfer. The first byte send is the command (read register) the
> second byte the register number the third byte is a dummy. The first 2
> bytes received are ignored the 3rd byte is the register contents.

To support this full duplex transfer the Cavium SPI controller needs
to know the receive lenght before setting up the transaction.

spi_transfer only includes the total length, so I don't see how this
should work.

--Jan
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: MCP251x SPI CAN controller on Cavium ThunderX

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2598 bytes --]

On 11/15/2017 01:07 PM, Jan Glauber wrote:
> On Wed, Nov 15, 2017 at 11:54:20AM +0100, Marc Kleine-Budde wrote:
>> On 11/14/2017 01:02 PM, Mark Brown wrote:
>>> On Mon, Nov 13, 2017 at 01:17:42PM -0800, Tim Harvey wrote:
>>>
>>>> When a register is read from the mcp251x driver the
>>>> octeon_spi_do_transfer() gets a spi_message with a single spi_xfer of
>>>> len=3, a tx_buf, and an rx_buf which I believe is supposed to shift
>>>> out 3 bytes out MOSI and shift in 3 bytes from MISO where the last
>>>> byte shifted in would be the response.
>>>
>>> No, that will simultaneously transmit and recieve three bytes.
>>
>> That's what the driver supposed to do.
>>
>>> If you want to transmit two bytes and then recieve one byte you need
>>> two xfers, one with a len of 2 and a tx_buf, the other with a len of
>>> 1 and a rx_buf.
>> To read a register (mcp251x_read_reg()) the mcp251x does a 3 byte full
>> duplex transfer. The first byte send is the command (read register) the
>> second byte the register number the third byte is a dummy. The first 2
>> bytes received are ignored the 3rd byte is the register contents.
> 
> To support this full duplex transfer the Cavium SPI controller needs
> to know the receive lenght before setting up the transaction.
> 
> spi_transfer only includes the total length, so I don't see how this
> should work.

It's a standard 3 byte full duplex transfer. Three bytes are send while
three bytes are received.

> static int mcp251x_spi_trans(struct spi_device *spi, int len)
> {
> 	struct mcp251x_priv *priv = spi_get_drvdata(spi);
> 	struct spi_transfer t = {
> 		.tx_buf = priv->spi_tx_buf,
> 		.rx_buf = priv->spi_rx_buf,
> 		.len = len,
> 		.cs_change = 0,
> 	};
> 	struct spi_message m;
> 	int ret;
> 
> 	spi_message_init(&m);
> 
> 	if (mcp251x_enable_dma) {
> 		t.tx_dma = priv->spi_tx_dma;
> 		t.rx_dma = priv->spi_rx_dma;
> 		m.is_dma_mapped = 1;
> 	}
> 
> 	spi_message_add_tail(&t, &m);
> 
> 	ret = spi_sync(spi, &m);
> 	if (ret)
> 		dev_err(&spi->dev, "spi transfer failed: ret = %d\n", ret);
> 	return ret;
> }

mcp251x_spi_trans() is called with len=3,
priv->spi_tx_buf and priv->spi_rx_buf point to previously allocared memory

priv->spi_tx_buf has been filled before calling mcp251x_spi_trans().

Marc

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


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

Re: MCP251x SPI CAN controller on Cavium ThunderX

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 3446 bytes --]

On 11/15/2017 01:40 PM, Marc Kleine-Budde wrote:
> mcp251x_spi_trans() is called with len=3,
> priv->spi_tx_buf and priv->spi_rx_buf point to previously allocared memory
> 
> priv->spi_tx_buf has been filled before calling mcp251x_spi_trans().

> #define OCTEON_SPI_MAX_BYTES 9

> static int octeon_spi_do_transfer(struct octeon_spi *p,
> 				  struct spi_message *msg,
> 				  struct spi_transfer *xfer,
> 				  bool last_xfer)
> {
> 	struct spi_device *spi = msg->spi;
> 	union cvmx_mpi_cfg mpi_cfg;
> 	union cvmx_mpi_tx mpi_tx;
> 	unsigned int clkdiv;
> 	int mode;
> 	bool cpha, cpol;
> 	const u8 *tx_buf;
> 	u8 *rx_buf;
> 	int len;
> 	int i;
> 
> 	mode = spi->mode;
> 	cpha = mode & SPI_CPHA;
> 	cpol = mode & SPI_CPOL;
> 
> 	clkdiv = p->sys_freq / (2 * xfer->speed_hz);
> 
> 	mpi_cfg.u64 = 0;
> 
> 	mpi_cfg.s.clkdiv = clkdiv;
> 	mpi_cfg.s.cshi = (mode & SPI_CS_HIGH) ? 1 : 0;
> 	mpi_cfg.s.lsbfirst = (mode & SPI_LSB_FIRST) ? 1 : 0;
> 	mpi_cfg.s.wireor = (mode & SPI_3WIRE) ? 1 : 0;
> 	mpi_cfg.s.idlelo = cpha != cpol;
> 	mpi_cfg.s.cslate = cpha ? 1 : 0;
> 	mpi_cfg.s.enable = 1;
> 
> 	if (spi->chip_select < 4)
> 		p->cs_enax |= 1ull << (12 + spi->chip_select);
> 	mpi_cfg.u64 |= p->cs_enax;
> 
> 	if (mpi_cfg.u64 != p->last_cfg) {
> 		p->last_cfg = mpi_cfg.u64;
> 		writeq(mpi_cfg.u64, p->register_base + OCTEON_SPI_CFG(p));
> 	}
> 	tx_buf = xfer->tx_buf;
> 	rx_buf = xfer->rx_buf;
> 	len = xfer->len;
> 	while (len > OCTEON_SPI_MAX_BYTES) {
> 		for (i = 0; i < OCTEON_SPI_MAX_BYTES; i++) {
> 			u8 d;
> 			if (tx_buf)
> 				d = *tx_buf++;
> 			else
> 				d = 0;
> 			writeq(d, p->register_base + OCTEON_SPI_DAT0(p) + (8 * i));
> 		}
> 		mpi_tx.u64 = 0;
> 		mpi_tx.s.csid = spi->chip_select;
> 		mpi_tx.s.leavecs = 1;
> 		mpi_tx.s.txnum = tx_buf ? OCTEON_SPI_MAX_BYTES : 0;

This looks fishy, OCTEON_SPI_MAX_BYTES is 9....

> 		mpi_tx.s.totnum = OCTEON_SPI_MAX_BYTES;
> 		writeq(mpi_tx.u64, p->register_base + OCTEON_SPI_TX(p));
> 
> 		octeon_spi_wait_ready(p);
> 		if (rx_buf)
> 			for (i = 0; i < OCTEON_SPI_MAX_BYTES; i++) {
> 				u64 v = readq(p->register_base + OCTEON_SPI_DAT0(p) + (8 * i));
> 				*rx_buf++ = (u8)v;
> 			}
> 		len -= OCTEON_SPI_MAX_BYTES;
> 	}
> 
> 	for (i = 0; i < len; i++) {
> 		u8 d;
> 		if (tx_buf)
> 			d = *tx_buf++;
> 		else
> 			d = 0;
> 		writeq(d, p->register_base + OCTEON_SPI_DAT0(p) + (8 * i));
> 	}
> 
> 	mpi_tx.u64 = 0;
> 	mpi_tx.s.csid = spi->chip_select;
> 	if (last_xfer)
> 		mpi_tx.s.leavecs = xfer->cs_change;
> 	else
> 		mpi_tx.s.leavecs = !xfer->cs_change;
> 	mpi_tx.s.txnum = tx_buf ? len : 0;
> 	mpi_tx.s.totnum = len;
> 	writeq(mpi_tx.u64, p->register_base + OCTEON_SPI_TX(p));
> 
> 	octeon_spi_wait_ready(p);
> 	if (rx_buf)
> 		for (i = 0; i < len; i++) {
> 			u64 v = readq(p->register_base + OCTEON_SPI_DAT0(p) + (8 * i));
> 			*rx_buf++ = (u8)v;
> 		}

Personally I'd fold this into the while loop, as there's quite some code
duplication. Of course your have to improve the "if (last_xfer)" a bit.

> 
> 	if (xfer->delay_usecs)
> 		udelay(xfer->delay_usecs);
> 
> 	return xfer->len;
> }

Marc

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


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

Re: MCP251x SPI CAN controller on Cavium ThunderX

[Jan Glauber]

On Wed, Nov 15, 2017 at 02:31:45PM +0100, Marc Kleine-Budde wrote:
> On 11/15/2017 01:40 PM, Marc Kleine-Budde wrote:
> > mcp251x_spi_trans() is called with len=3,
> > priv->spi_tx_buf and priv->spi_rx_buf point to previously allocared memory
> > 
> > priv->spi_tx_buf has been filled before calling mcp251x_spi_trans().
> 
> > #define OCTEON_SPI_MAX_BYTES 9
> 
> > static int octeon_spi_do_transfer(struct octeon_spi *p,
> > 				  struct spi_message *msg,
> > 				  struct spi_transfer *xfer,
> > 				  bool last_xfer)
> > {
> > 	struct spi_device *spi = msg->spi;
> > 	union cvmx_mpi_cfg mpi_cfg;
> > 	union cvmx_mpi_tx mpi_tx;
> > 	unsigned int clkdiv;
> > 	int mode;
> > 	bool cpha, cpol;
> > 	const u8 *tx_buf;
> > 	u8 *rx_buf;
> > 	int len;
> > 	int i;
> > 
> > 	mode = spi->mode;
> > 	cpha = mode & SPI_CPHA;
> > 	cpol = mode & SPI_CPOL;
> > 
> > 	clkdiv = p->sys_freq / (2 * xfer->speed_hz);
> > 
> > 	mpi_cfg.u64 = 0;
> > 
> > 	mpi_cfg.s.clkdiv = clkdiv;
> > 	mpi_cfg.s.cshi = (mode & SPI_CS_HIGH) ? 1 : 0;
> > 	mpi_cfg.s.lsbfirst = (mode & SPI_LSB_FIRST) ? 1 : 0;
> > 	mpi_cfg.s.wireor = (mode & SPI_3WIRE) ? 1 : 0;
> > 	mpi_cfg.s.idlelo = cpha != cpol;
> > 	mpi_cfg.s.cslate = cpha ? 1 : 0;
> > 	mpi_cfg.s.enable = 1;
> > 
> > 	if (spi->chip_select < 4)
> > 		p->cs_enax |= 1ull << (12 + spi->chip_select);
> > 	mpi_cfg.u64 |= p->cs_enax;
> > 
> > 	if (mpi_cfg.u64 != p->last_cfg) {
> > 		p->last_cfg = mpi_cfg.u64;
> > 		writeq(mpi_cfg.u64, p->register_base + OCTEON_SPI_CFG(p));
> > 	}
> > 	tx_buf = xfer->tx_buf;
> > 	rx_buf = xfer->rx_buf;
> > 	len = xfer->len;
> > 	while (len > OCTEON_SPI_MAX_BYTES) {
> > 		for (i = 0; i < OCTEON_SPI_MAX_BYTES; i++) {
> > 			u8 d;
> > 			if (tx_buf)
> > 				d = *tx_buf++;
> > 			else
> > 				d = 0;
> > 			writeq(d, p->register_base + OCTEON_SPI_DAT0(p) + (8 * i));
> > 		}
> > 		mpi_tx.u64 = 0;
> > 		mpi_tx.s.csid = spi->chip_select;
> > 		mpi_tx.s.leavecs = 1;
> > 		mpi_tx.s.txnum = tx_buf ? OCTEON_SPI_MAX_BYTES : 0;
> 
> This looks fishy, OCTEON_SPI_MAX_BYTES is 9....

Because there are 9 registers in MPI_DAT(0..8).

> > 		mpi_tx.s.totnum = OCTEON_SPI_MAX_BYTES;
> > 		writeq(mpi_tx.u64, p->register_base + OCTEON_SPI_TX(p));
> > 
> > 		octeon_spi_wait_ready(p);
> > 		if (rx_buf)
> > 			for (i = 0; i < OCTEON_SPI_MAX_BYTES; i++) {
> > 				u64 v = readq(p->register_base + OCTEON_SPI_DAT0(p) + (8 * i));
> > 				*rx_buf++ = (u8)v;
> > 			}
> > 		len -= OCTEON_SPI_MAX_BYTES;
> > 	}
> > 
> > 	for (i = 0; i < len; i++) {
> > 		u8 d;
> > 		if (tx_buf)
> > 			d = *tx_buf++;
> > 		else
> > 			d = 0;
> > 		writeq(d, p->register_base + OCTEON_SPI_DAT0(p) + (8 * i));
> > 	}
> > 
> > 	mpi_tx.u64 = 0;
> > 	mpi_tx.s.csid = spi->chip_select;
> > 	if (last_xfer)
> > 		mpi_tx.s.leavecs = xfer->cs_change;
> > 	else
> > 		mpi_tx.s.leavecs = !xfer->cs_change;
> > 	mpi_tx.s.txnum = tx_buf ? len : 0;
> > 	mpi_tx.s.totnum = len;
> > 	writeq(mpi_tx.u64, p->register_base + OCTEON_SPI_TX(p));
> > 
> > 	octeon_spi_wait_ready(p);
> > 	if (rx_buf)
> > 		for (i = 0; i < len; i++) {
> > 			u64 v = readq(p->register_base + OCTEON_SPI_DAT0(p) + (8 * i));
> > 			*rx_buf++ = (u8)v;
> > 		}
> 
> Personally I'd fold this into the while loop, as there's quite some code
> duplication. Of course your have to improve the "if (last_xfer)" a bit.

I've not written that code, just split it for shared arm64 & mips usage
and avoided re-writing it completely on purpose :) If it turns out that
we need to change this code I might consider making changes like this.

Adding David who might know more about this driver.

--Jan

Re: MCP251x SPI CAN controller on Cavium ThunderX

[Mark Brown]

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

On Wed, Nov 15, 2017 at 01:07:54PM +0100, Jan Glauber wrote:

> To support this full duplex transfer the Cavium SPI controller needs
> to know the receive lenght before setting up the transaction.

> spi_transfer only includes the total length, so I don't see how this
> should work.

For a full duplex transfer the recieve length *is* the total length of
the transfer.  If there is a mix of tx, rx and full duplex then you need
multiple xfers in the transfer.

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