* Re: MCP251x SPI CAN controller on Cavium ThunderX [not found] ` <20171114120207.xbee2cgsai4qka46@sirena.org.uk> @ 2017-11-15 10:54 ` Marc Kleine-Budde [not found] ` <bfa971bb-1217-ed86-d1d8-e353ecc506aa-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> 0 siblings, 1 reply; 6+ messages in thread From: Marc Kleine-Budde @ 2017-11-15 10:54 UTC (permalink / raw) To: Mark Brown, Tim Harvey Cc: Jan Glauber, linux-spi, linux-kernel@vger.kernel.org, Wolfgang Grandegger, linux-can [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <bfa971bb-1217-ed86-d1d8-e353ecc506aa-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]
* Re: MCP251x SPI CAN controller on Cavium ThunderX [not found] ` <bfa971bb-1217-ed86-d1d8-e353ecc506aa-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> @ 2017-11-15 12:07 ` Jan Glauber 2017-11-15 12:40 ` Marc Kleine-Budde 2017-11-15 15:39 ` Mark Brown 0 siblings, 2 replies; 6+ messages in thread From: Jan Glauber @ 2017-11-15 12:07 UTC (permalink / raw) To: Marc Kleine-Budde Cc: Mark Brown, Tim Harvey, linux-spi-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Wolfgang Grandegger, linux-can 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: MCP251x SPI CAN controller on Cavium ThunderX 2017-11-15 12:07 ` Jan Glauber @ 2017-11-15 12:40 ` Marc Kleine-Budde 2017-11-15 13:31 ` Marc Kleine-Budde 2017-11-15 15:39 ` Mark Brown 1 sibling, 1 reply; 6+ messages in thread From: Marc Kleine-Budde @ 2017-11-15 12:40 UTC (permalink / raw) To: Jan Glauber Cc: Mark Brown, Tim Harvey, linux-spi, linux-kernel@vger.kernel.org, Wolfgang Grandegger, linux-can [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: MCP251x SPI CAN controller on Cavium ThunderX 2017-11-15 12:40 ` Marc Kleine-Budde @ 2017-11-15 13:31 ` Marc Kleine-Budde 2017-11-15 14:24 ` Jan Glauber 0 siblings, 1 reply; 6+ messages in thread From: Marc Kleine-Budde @ 2017-11-15 13:31 UTC (permalink / raw) To: Jan Glauber Cc: Mark Brown, Tim Harvey, linux-spi, linux-kernel@vger.kernel.org, Wolfgang Grandegger, linux-can [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: MCP251x SPI CAN controller on Cavium ThunderX 2017-11-15 13:31 ` Marc Kleine-Budde @ 2017-11-15 14:24 ` Jan Glauber 0 siblings, 0 replies; 6+ messages in thread From: Jan Glauber @ 2017-11-15 14:24 UTC (permalink / raw) To: Marc Kleine-Budde Cc: Mark Brown, Tim Harvey, linux-spi, linux-kernel@vger.kernel.org, Wolfgang Grandegger, linux-can, David Daney 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: MCP251x SPI CAN controller on Cavium ThunderX 2017-11-15 12:07 ` Jan Glauber 2017-11-15 12:40 ` Marc Kleine-Budde @ 2017-11-15 15:39 ` Mark Brown 1 sibling, 0 replies; 6+ messages in thread From: Mark Brown @ 2017-11-15 15:39 UTC (permalink / raw) To: Jan Glauber Cc: Marc Kleine-Budde, Tim Harvey, linux-spi, linux-kernel@vger.kernel.org, Wolfgang Grandegger, linux-can [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-11-15 15:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CAJ+vNU3VK=NPKW+119t4MT7w5xs=FS2HWJTqbFb-HSeJhCvwWw@mail.gmail.com>
[not found] ` <20171114120207.xbee2cgsai4qka46@sirena.org.uk>
2017-11-15 10:54 ` MCP251x SPI CAN controller on Cavium ThunderX Marc Kleine-Budde
[not found] ` <bfa971bb-1217-ed86-d1d8-e353ecc506aa-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2017-11-15 12:07 ` Jan Glauber
2017-11-15 12:40 ` Marc Kleine-Budde
2017-11-15 13:31 ` Marc Kleine-Budde
2017-11-15 14:24 ` Jan Glauber
2017-11-15 15:39 ` Mark Brown
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox