MCP251x SPI CAN controller on Cavium ThunderX

MCP251x SPI CAN controller on Cavium ThunderX

[Tim Harvey]

Mark/Jan,

I have been unsuccessful getting a MCP251x SPI based CAN controller
working on a CN80xx using Linux mainline.

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.

The cavium CN80xx MPI_TX register has fields for 'Number of bytes to
transmit' (TXNUM) and 'Total number of bytes to shift (transmit and
receive)' (TOTNUM) and these are both getting set to 3 by
octeon_spi_do_transfer() but I find that this causes unexpected data
in the shifted in response unless I make TOTNUM = TXNUM + 1.

I should also note that Cavium has a software suite called the 'BDK'
which provides a CLI to SPI transfers which allows you to set the
TXNUM and TOTNUM fields uniquely and if I send a 2-byte command
(TXNUM=2) to read a register (READ command followed by the register)
and a 1 byte read (thus TOTNUM=3) then I get the response from the
mcp251x I expect.

Is there something I'm misunderstanding about the Linux SPI API and
perhaps is there a bug in the cavium SPI controller driver or do I
perhaps have it configured wrong with respect to CPHA and/or CPOL?

Thanks,

Tim

Re: MCP251x SPI CAN controller on Cavium ThunderX

[Mark Brown]

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

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

[-- Attachment #2: signature.asc --]
[-- 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: 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

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

Re: MCP251x SPI CAN controller on Cavium ThunderX

[David Daney]

On 11/13/2017 01:17 PM, Tim Harvey wrote:
> Mark/Jan,
> 
> I have been unsuccessful getting a MCP251x SPI based CAN controller
> working on a CN80xx using Linux mainline.
> 
> 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.
> 
> The cavium CN80xx MPI_TX register has fields for 'Number of bytes to
> transmit' (TXNUM) and 'Total number of bytes to shift (transmit and
> receive)' (TOTNUM) and these are both getting set to 3 by
> octeon_spi_do_transfer() but I find that this causes unexpected data
> in the shifted in response unless I make TOTNUM = TXNUM + 1.
> 
> I should also note that Cavium has a software suite called the 'BDK'
> which provides a CLI to SPI transfers which allows you to set the
> TXNUM and TOTNUM fields uniquely and if I send a 2-byte command
> (TXNUM=2) to read a register (READ command followed by the register)
> and a 1 byte read (thus TOTNUM=3) then I get the response from the
> mcp251x I expect.
> 

By looking at the driver, and from my recollection, I think that 
SPI_3WIRE may never have been tested, so there could be bugs in this mode.

The driver as is works with various SPI eeprom devices, so any proposed 
changes would need to be validated against things that currently work.

It could be that you need the CN80xx Hardware Reference Manual, board 
schematics and a logic analyzer to be able to figure out what is happening.

David.



> Is there something I'm misunderstanding about the Linux SPI API and
> perhaps is there a bug in the cavium SPI controller driver or do I
> perhaps have it configured wrong with respect to CPHA and/or CPOL?
> 
> Thanks,
> 
> Tim
> 

Re: MCP251x SPI CAN controller on Cavium ThunderX

[Tim Harvey]

On Wed, Nov 15, 2017 at 8:02 AM, David Daney <ddaney@caviumnetworks.com> wrote:
> On 11/13/2017 01:17 PM, Tim Harvey wrote:
>>
>> Mark/Jan,
>>
>> I have been unsuccessful getting a MCP251x SPI based CAN controller
>> working on a CN80xx using Linux mainline.
>>
>> 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.
>>
>> The cavium CN80xx MPI_TX register has fields for 'Number of bytes to
>> transmit' (TXNUM) and 'Total number of bytes to shift (transmit and
>> receive)' (TOTNUM) and these are both getting set to 3 by
>> octeon_spi_do_transfer() but I find that this causes unexpected data
>> in the shifted in response unless I make TOTNUM = TXNUM + 1.
>>
>> I should also note that Cavium has a software suite called the 'BDK'
>> which provides a CLI to SPI transfers which allows you to set the
>> TXNUM and TOTNUM fields uniquely and if I send a 2-byte command
>> (TXNUM=2) to read a register (READ command followed by the register)
>> and a 1 byte read (thus TOTNUM=3) then I get the response from the
>> mcp251x I expect.
>>
>
> By looking at the driver, and from my recollection, I think that SPI_3WIRE
> may never have been tested, so there could be bugs in this mode.
>
> The driver as is works with various SPI eeprom devices, so any proposed
> changes would need to be validated against things that currently work.
>
> It could be that you need the CN80xx Hardware Reference Manual, board
> schematics and a logic analyzer to be able to figure out what is happening.
>

David,

I have all three here and can debug. This isn't hooked up as SPI_3WIRE
(wireor) - its got full a 4 wire connection.

So thanks to the discussion here I now understand we are doing a
3-byte full-duplex transfer (the third dummy byte threw me off) and
that is what the spi-cavium.c driver is setting up.

So the transfer from the cavium side looks like this and TXNUM=3
TOTNUM=3 makes sense to me for a 3-byte full duplex transfer (shift a
total of 3 bytes).

// configure spi: 10MHz (clockdiv=0x11; cshi=0 wireor=0 cslate=0)
mpi_cfg => 0x112001
// send three bytes (0x03 = READ, 0x0f = CANSTAT, 0x00 = dummy byte)
mpi_dat0 => 0x03
mpi_dat1 => 0x0f
mpi_dat2 => 0x00
// do the transfer (CS1, leavecs=0  Deassert SPI_CSn_L after the
transaction is done, TXNUM=3 TOTNUM=3)
mpi_tx => 0x100303
// read response
mpi_dat0 <= 0xff
mpi_dat1 <= 0xff
mpi_dat2 <= 0x00
^^^^ I expect mpi_dat2 to be 0x80

Looking at the scope of CLK and MSIO I do see 3-bytes of CLK cycles
and the 0x80 on the wire and I'm wondering now if the cavium isn't
latching the 1st bit because of clock polarity (MPI_CFG[CSHI]) or
phase (MPI_CFG[CSLATE]).

Regardless of scope shots though, what is strange to me is that if I
increase TOTNUM to 4 (write 3 bytes, read 1 bytes, shift a total of 4
bytes) I get:
// configure spi: 10MHz (clockdiv=0x11; cshi=0 wireor=0 cslate=0)
mpi_cfg => 0x112001
// send three bytes (0x03 = READ, 0x0f = CANSTAT, 0x00 = dummy byte)
mpi_dat0 => 0x03
mpi_dat1 => 0x0f
mpi_dat2 => 0x00
// do the transfer (CS1, leavecs=0  Deassert SPI_CSn_L after the
transaction is done, TXNUM=3 TOTNUM=4)
mpi_tx => 0x100304
// read response
mpi_dat0 <= 0xff
mpi_dat1 <= 0xff
mpi_dat2 <= 0x80
^^^^^ 0x80 'is' the response I expect

Regards,

Tim

Re: MCP251x SPI CAN controller on Cavium ThunderX

[Tim Harvey]

On Wed, Nov 15, 2017 at 10:23 AM, Tim Harvey <tharvey@gateworks.com> wrote:
> On Wed, Nov 15, 2017 at 8:02 AM, David Daney <ddaney@caviumnetworks.com> wrote:
>> On 11/13/2017 01:17 PM, Tim Harvey wrote:
>>>
>>> Mark/Jan,
>>>
>>> I have been unsuccessful getting a MCP251x SPI based CAN controller
>>> working on a CN80xx using Linux mainline.
>>>
>>> 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.
>>>
>>> The cavium CN80xx MPI_TX register has fields for 'Number of bytes to
>>> transmit' (TXNUM) and 'Total number of bytes to shift (transmit and
>>> receive)' (TOTNUM) and these are both getting set to 3 by
>>> octeon_spi_do_transfer() but I find that this causes unexpected data
>>> in the shifted in response unless I make TOTNUM = TXNUM + 1.
>>>
>>> I should also note that Cavium has a software suite called the 'BDK'
>>> which provides a CLI to SPI transfers which allows you to set the
>>> TXNUM and TOTNUM fields uniquely and if I send a 2-byte command
>>> (TXNUM=2) to read a register (READ command followed by the register)
>>> and a 1 byte read (thus TOTNUM=3) then I get the response from the
>>> mcp251x I expect.
>>>
>>
>> By looking at the driver, and from my recollection, I think that SPI_3WIRE
>> may never have been tested, so there could be bugs in this mode.
>>
>> The driver as is works with various SPI eeprom devices, so any proposed
>> changes would need to be validated against things that currently work.
>>
>> It could be that you need the CN80xx Hardware Reference Manual, board
>> schematics and a logic analyzer to be able to figure out what is happening.
>>
>
> David,
>
> I have all three here and can debug. This isn't hooked up as SPI_3WIRE
> (wireor) - its got full a 4 wire connection.
>
> So thanks to the discussion here I now understand we are doing a
> 3-byte full-duplex transfer (the third dummy byte threw me off) and
> that is what the spi-cavium.c driver is setting up.
>
> So the transfer from the cavium side looks like this and TXNUM=3
> TOTNUM=3 makes sense to me for a 3-byte full duplex transfer (shift a
> total of 3 bytes).
>
> // configure spi: 10MHz (clockdiv=0x11; cshi=0 wireor=0 cslate=0)
> mpi_cfg => 0x112001
> // send three bytes (0x03 = READ, 0x0f = CANSTAT, 0x00 = dummy byte)
> mpi_dat0 => 0x03
> mpi_dat1 => 0x0f
> mpi_dat2 => 0x00
> // do the transfer (CS1, leavecs=0  Deassert SPI_CSn_L after the
> transaction is done, TXNUM=3 TOTNUM=3)
> mpi_tx => 0x100303
> // read response
> mpi_dat0 <= 0xff
> mpi_dat1 <= 0xff
> mpi_dat2 <= 0x00
> ^^^^ I expect mpi_dat2 to be 0x80
>
> Looking at the scope of CLK and MSIO I do see 3-bytes of CLK cycles
> and the 0x80 on the wire and I'm wondering now if the cavium isn't
> latching the 1st bit because of clock polarity (MPI_CFG[CSHI]) or
> phase (MPI_CFG[CSLATE]).
>
> Regardless of scope shots though, what is strange to me is that if I
> increase TOTNUM to 4 (write 3 bytes, read 1 bytes, shift a total of 4
> bytes) I get:
> // configure spi: 10MHz (clockdiv=0x11; cshi=0 wireor=0 cslate=0)
> mpi_cfg => 0x112001
> // send three bytes (0x03 = READ, 0x0f = CANSTAT, 0x00 = dummy byte)
> mpi_dat0 => 0x03
> mpi_dat1 => 0x0f
> mpi_dat2 => 0x00
> // do the transfer (CS1, leavecs=0  Deassert SPI_CSn_L after the
> transaction is done, TXNUM=3 TOTNUM=4)
> mpi_tx => 0x100304
> // read response
> mpi_dat0 <= 0xff
> mpi_dat1 <= 0xff
> mpi_dat2 <= 0x80
> ^^^^^ 0x80 'is' the response I expect
>

David / Jan,

For reference, the HM describes TXNUM/TOTNUM as:
 TXNUM - Number of bytes to transmit
 TOTNUM - Total number of bytes to shift (transmit and receive)

Here are some experiments that show somewhat inconsistent results:
- full duplex 3byte tx / 3byte rx to MCP251x
mpi_dat0 => 0x03 // READ
mpi_dat1 => 0x0e // CANSTAT
mip_dat2 => 0xa5 // dummy (but making it 0xa5 instead of 0x00 to prove a point)
mpi_tx => 0x100303 // TXNUM=3 TOTNUM=3; we see 24 clock cycles
// wait for completion
mpi_dat0 <= 0xff
mpi_dat1 <= 0xff
mpi_dat2 <=0xa5 // this the dummy byte we sent out MOSI not what came
in on MISO which the scope shows as 0x80

if I set TXNUM=3 TOTNUM=4:
mpi_dat0 => 0x03 // READ
mpi_dat1 => 0x0e // CANSTAT
mip_dat2 => 0xa5 // dummy
mpi_tx => 0x100304 // TXNUM=3 TOTNUM=4; we see 32 clock cycles
// wait for completion
mpi_dat0 <= 0xff
mpi_dat1 <= 0xff
mpi_dat2 <= 0x80 // response for CANSTAT reg 0x0e
mpi_dat3 <= 0x87 // response for CANCTRL reg 0x0f (because we shifted
32 clock cycles)

if I set TXNUM=2 TOTNUM=3:
mpi_dat0 => 0x03 // READ
mpi_dat1 => 0x0e // CANSTAT
mpi_tx => 0x100203 // TXNUM=2 TOTNUM=3; we see 24 clock cycles
// wait for completion
mpi_dat0 <= 0xff
mpi_dat1 <= 0xff
mpi_dat2 <= 0x80 // response for CANSTAT reg 0x0e

if I set TXNUM=1 TOTNUM=1 to send a RESET command:
mpi_dat0 => 0xc0 // RESET
mpi_tx => 0x100101 // TXNUM=1 TOTNUM=1; we see 8 clock cycles
// wait for completion
mpi_dat0 <= 0xc0

In all cases above what is seen on MISO in relation to CLK matches the
expectations of the mcp251x but the CN80xx MPI_DAT registers don't
return what I see on MISO. Am I missing a consistent pattern of
MPI_DAT vs TXNUM/TOTNUM here that would allow us to work-around this?
Is this a CN80xx chip errata? There is no known errata for the CN80XX
MPI engine.

I could re-write the mcp251x driver to not use full-duplex but I'm
assuming most SPI drivers use full-duplex transactions.

Regards,

Tim

Re: MCP251x SPI CAN controller on Cavium ThunderX

[Marc Kleine-Budde]

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

On 11/16/2017 12:18 AM, Tim Harvey wrote:
> David / Jan,
> 
> For reference, the HM describes TXNUM/TOTNUM as:
>  TXNUM - Number of bytes to transmit
>  TOTNUM - Total number of bytes to shift (transmit and receive)
> 
> Here are some experiments that show somewhat inconsistent results:
> - full duplex 3byte tx / 3byte rx to MCP251x
> mpi_dat0 => 0x03 // READ
> mpi_dat1 => 0x0e // CANSTAT
> mip_dat2 => 0xa5 // dummy (but making it 0xa5 instead of 0x00 to prove a point)
> mpi_tx => 0x100303 // TXNUM=3 TOTNUM=3; we see 24 clock cycles
> // wait for completion
> mpi_dat0 <= 0xff
> mpi_dat1 <= 0xff

Do you see 0xff 0xff on the MISO wire for byte 1 and 2?

> mpi_dat2 <=0xa5 // this the dummy byte we sent out MOSI not what came
> in on MISO which the scope shows as 0x80

Oh, the device reads what you have shifed out? This is not good.

> if I set TXNUM=3 TOTNUM=4:
> mpi_dat0 => 0x03 // READ
> mpi_dat1 => 0x0e // CANSTAT
> mip_dat2 => 0xa5 // dummy
> mpi_tx => 0x100304 // TXNUM=3 TOTNUM=4; we see 32 clock cycles

What's on MOSI for byte 4?

> // wait for completion
> mpi_dat0 <= 0xff
> mpi_dat1 <= 0xff
> mpi_dat2 <= 0x80 // response for CANSTAT reg 0x0e
> mpi_dat3 <= 0x87 // response for CANCTRL reg 0x0f (because we shifted
> 32 clock cycles)

Looks correct.

> if I set TXNUM=2 TOTNUM=3:
> mpi_dat0 => 0x03 // READ
> mpi_dat1 => 0x0e // CANSTAT
> mpi_tx => 0x100203 // TXNUM=2 TOTNUM=3; we see 24 clock cycles

What's on MOSI for byte 3?

> // wait for completion
> mpi_dat0 <= 0xff
> mpi_dat1 <= 0xff
> mpi_dat2 <= 0x80 // response for CANSTAT reg 0x0e
> 
> if I set TXNUM=1 TOTNUM=1 to send a RESET command:
> mpi_dat0 => 0xc0 // RESET
> mpi_tx => 0x100101 // TXNUM=1 TOTNUM=1; we see 8 clock cycles
> // wait for completion
> mpi_dat0 <= 0xc0
> 
> In all cases above what is seen on MISO in relation to CLK matches the
> expectations of the mcp251x but the CN80xx MPI_DAT registers don't
> return what I see on MISO. Am I missing a consistent pattern of
> MPI_DAT vs TXNUM/TOTNUM here that would allow us to work-around this?
> Is this a CN80xx chip errata? There is no known errata for the CN80XX
> MPI engine.
> 
> I could re-write the mcp251x driver to not use full-duplex but I'm
> assuming most SPI drivers use full-duplex transactions.

ACK.

You don't want to use the mcp251x chip anyways for CAN, the SPI is quite
slow and the load of the CPU is relatively high. Don't expect to get
many messages over CAN and prepare for dropped frames. Better use a
USB-CAN dongle. Or a proper card with some sort of PCI interface.

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

[Tim Harvey]

On Thu, Nov 16, 2017 at 4:12 AM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 11/16/2017 12:18 AM, Tim Harvey wrote:
>> David / Jan,
>>
>> For reference, the HM describes TXNUM/TOTNUM as:
>>  TXNUM - Number of bytes to transmit
>>  TOTNUM - Total number of bytes to shift (transmit and receive)
>>
>> Here are some experiments that show somewhat inconsistent results:
>> - full duplex 3byte tx / 3byte rx to MCP251x
>> mpi_dat0 => 0x03 // READ
>> mpi_dat1 => 0x0e // CANSTAT
>> mip_dat2 => 0xa5 // dummy (but making it 0xa5 instead of 0x00 to prove a point)
>> mpi_tx => 0x100303 // TXNUM=3 TOTNUM=3; we see 24 clock cycles
>> // wait for completion
>> mpi_dat0 <= 0xff
>> mpi_dat1 <= 0xff
>
> Do you see 0xff 0xff on the MISO wire for byte 1 and 2?

Yes

>
>> mpi_dat2 <=0xa5 // this the dummy byte we sent out MOSI not what came
>> in on MISO which the scope shows as 0x80
>
> Oh, the device reads what you have shifed out? This is not good.

Yes... its not good. My first theory was that until TXNUM bytes have
been shifted out it shifts in the data it's sending (there's a mux
that can select from MOSI or MISO) but this doesn't hold true on some
of the other test cases.

if you perform a SPI transaction on a CN80xx that has no SPI slave
device you can see this as well.

>
>> if I set TXNUM=3 TOTNUM=4:
>> mpi_dat0 => 0x03 // READ
>> mpi_dat1 => 0x0e // CANSTAT
>> mip_dat2 => 0xa5 // dummy
>> mpi_tx => 0x100304 // TXNUM=3 TOTNUM=4; we see 32 clock cycles
>
> What's on MOSI for byte 4?

whatever was in the buffer - 0 in my test cases. The TOTNUM=4 causes
4*8 clock cycles. The TXNUM somehow steers the MUX that selects if
MOSI or MISO gets shifted into MPI_DAT for the input side but its not
entirely clear what it's logic is. I don't really understand why you
would ever want to read in MOSI and I don't see a way to control this
mux directly.

>
>> // wait for completion
>> mpi_dat0 <= 0xff
>> mpi_dat1 <= 0xff
>> mpi_dat2 <= 0x80 // response for CANSTAT reg 0x0e
>> mpi_dat3 <= 0x87 // response for CANCTRL reg 0x0f (because we shifted
>> 32 clock cycles)
>
> Looks correct.

Right, the data is correct but in this case I didn't want to clock an
extra byte just to be able to get the response.

>
>> if I set TXNUM=2 TOTNUM=3:
>> mpi_dat0 => 0x03 // READ
>> mpi_dat1 => 0x0e // CANSTAT
>> mpi_tx => 0x100203 // TXNUM=2 TOTNUM=3; we see 24 clock cycles
>
> What's on MOSI for byte 3?

whatever was in the buffer - 0 in my case.

>
>> // wait for completion
>> mpi_dat0 <= 0xff
>> mpi_dat1 <= 0xff
>> mpi_dat2 <= 0x80 // response for CANSTAT reg 0x0e
>>
>> if I set TXNUM=1 TOTNUM=1 to send a RESET command:
>> mpi_dat0 => 0xc0 // RESET
>> mpi_tx => 0x100101 // TXNUM=1 TOTNUM=1; we see 8 clock cycles
>> // wait for completion
>> mpi_dat0 <= 0xc0
>>
>> In all cases above what is seen on MISO in relation to CLK matches the
>> expectations of the mcp251x but the CN80xx MPI_DAT registers don't
>> return what I see on MISO. Am I missing a consistent pattern of
>> MPI_DAT vs TXNUM/TOTNUM here that would allow us to work-around this?
>> Is this a CN80xx chip errata? There is no known errata for the CN80XX
>> MPI engine.
>>
>> I could re-write the mcp251x driver to not use full-duplex but I'm
>> assuming most SPI drivers use full-duplex transactions.
>
> ACK.
>
> You don't want to use the mcp251x chip anyways for CAN, the SPI is quite
> slow and the load of the CPU is relatively high. Don't expect to get
> many messages over CAN and prepare for dropped frames. Better use a
> USB-CAN dongle. Or a proper card with some sort of PCI interface.
>

Understood. I'm not clear what the use case is for CAN on our products
but we have customers ask for it and they haven't specified what sort
of throughput they expect. We liked the MCP251x because it was a small
package and has an integrated transceiver. I'm not aware of any USB
based CAN controller chips with transceiver - all the devices I see in
drivers/net/can/usb seem to be non-on-board products implementing CAN
via on-board ARM CPU's.

I see a driver for SPI based HI311x CAN controller in the kernel as
well which at least doubles the clock of the MCP251x to 20MHz (the
CN80xx/CN81xx SPI clock can go up to 50MHz assuming we can get it to
work).

What are typical datarates you see used for CAN applications?

Tim

Re: MCP251x SPI CAN controller on Cavium ThunderX

[Mark Brown]

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

On Wed, Nov 15, 2017 at 03:18:59PM -0800, Tim Harvey wrote:

> I could re-write the mcp251x driver to not use full-duplex but I'm
> assuming most SPI drivers use full-duplex transactions.

I'd not actually say that's the case - the bulk of devices do some kind
of command/response or register/value thing, full duplex is moderately
complex to work with and ends up needing a constant data stream between
the two devices.  It does happen (and some controllers always transfer
data in both directions) but most of the time only one direction has any
meaning.

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