| Message ID | 20230622113341.657842-4-fabrizio.castro.jz@renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | spi: Add CSI support for Renesas RZ/V2M | expand |
Hi Fabrizio, On Thu, Jun 22, 2023 at 1:34 PM Fabrizio Castro <fabrizio.castro.jz@renesas.com> wrote: > The RZ/V2M SoC comes with the Clocked Serial Interface (CSI) > IP, which is a master/slave SPI controller. > > This commit adds a driver to support CSI master mode. > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > --- > > v2: edited includes in drivers/spi/spi-rzv2m-csi.c Thanks for your patch! > --- a/drivers/spi/Kconfig > +++ b/drivers/spi/Kconfig > @@ -825,6 +825,12 @@ config SPI_RSPI > help > SPI driver for Renesas RSPI and QSPI blocks. > > +config SPI_RZV2M_CSI > + tristate "Renesas RZV2M CSI controller" RZ/V2M (patch sent) > + depends on ARCH_RENESAS || COMPILE_TEST > + help > + SPI driver for Renesas RZ/V2M Clocked Serial Interface (CSI) > + > config SPI_QCOM_QSPI > tristate "QTI QSPI controller" > depends on ARCH_QCOM || COMPILE_TEST > --- /dev/null > +++ b/drivers/spi/spi-rzv2m-csi.c > @@ -0,0 +1,667 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Renesas RZ/V2M Clocked Serial Interface (CSI) driver > + * > + * Copyright (C) 2023 Renesas Electronics Corporation > + */ > + > +#include <linux/clk.h> > +#include <linux/count_zeros.h> > +#include <linux/interrupt.h> > +#include <linux/iopoll.h> > +#include <linux/platform_device.h> > +#include <linux/reset.h> > +#include <linux/spi/spi.h> > + > +/* Registers */ > +#define CSI_MODE 0x00 /* CSI mode control */ > +#define CSI_CLKSEL 0x04 /* CSI clock select */ > +#define CSI_CNT 0x08 /* CSI control */ > +#define CSI_INT 0x0C /* CSI interrupt status */ > +#define CSI_IFIFOL 0x10 /* CSI receive FIFO level display */ > +#define CSI_OFIFOL 0x14 /* CSI transmit FIFO level display */ > +#define CSI_IFIFO 0x18 /* CSI receive window */ > +#define CSI_OFIFO 0x1C /* CSI transmit window */ > +#define CSI_FIFOTRG 0x20 /* CSI FIFO trigger level */ > + > +/* CSI_MODE */ > +#define CSI_MODE_CSIE BIT(7) > +#define CSI_MODE_TRMD BIT(6) > +#define CSI_MODE_CCL BIT(5) > +#define CSI_MODE_DIR BIT(4) > +#define CSI_MODE_CSOT BIT(0) > + > +#define CSI_MODE_SETUP 0x00000040 > + > +/* CSI_CLKSEL */ > +#define CSI_CLKSEL_CKP BIT(17) > +#define CSI_CLKSEL_DAP BIT(16) > +#define CSI_CLKSEL_SLAVE BIT(15) > +#define CSI_CLKSEL_CKS GENMASK(14, 1) > + > +/* CSI_CNT */ > +#define CSI_CNT_CSIRST BIT(28) > +#define CSI_CNT_R_TRGEN BIT(19) > +#define CSI_CNT_UNDER_E BIT(13) > +#define CSI_CNT_OVERF_E BIT(12) > +#define CSI_CNT_TREND_E BIT(9) > +#define CSI_CNT_CSIEND_E BIT(8) > +#define CSI_CNT_T_TRGR_E BIT(4) > +#define CSI_CNT_R_TRGR_E BIT(0) > + > +/* CSI_INT */ > +#define CSI_INT_UNDER BIT(13) > +#define CSI_INT_OVERF BIT(12) > +#define CSI_INT_TREND BIT(9) > +#define CSI_INT_CSIEND BIT(8) > +#define CSI_INT_T_TRGR BIT(4) > +#define CSI_INT_R_TRGR BIT(0) > + > +/* CSI_FIFOTRG */ > +#define CSI_FIFOTRG_R_TRG GENMASK(2, 0) > + > +#define CSI_FIFO_SIZE_BYTES 32 > +#define CSI_FIFO_HALF_SIZE 16 > +#define CSI_EN_DIS_TIMEOUT_US 100 > +#define CSI_CKS_MAX 0x3FFF > + > +#define UNDERRUN_ERROR BIT(0) > +#define OVERFLOW_ERROR BIT(1) > +#define TX_TIMEOUT_ERROR BIT(2) > +#define RX_TIMEOUT_ERROR BIT(3) > + > +#define CSI_MAX_SPI_SCKO 8000000 > + > +struct rzv2m_csi_priv { > + void __iomem *base; > + struct clk *csiclk; > + struct clk *pclk; > + struct device *dev; > + struct spi_controller *controller; > + const u8 *txbuf; > + u8 *rxbuf; > + int buffer_len; > + int bytes_sent; > + int bytes_received; > + int bytes_to_transfer; > + int words_to_transfer; All these ints should be unsigned. > + unsigned char bytes_per_word; 3-byte gap > + wait_queue_head_t wait; > + u8 errors; 3 byte gap > + u32 status; > +}; > + > +static int rzv2m_csi_fill_txfifo(struct rzv2m_csi_priv *csi) > +{ > + int i; unsigned int > + > + if (readl(csi->base + CSI_OFIFOL)) > + return -EIO; > + > + if (csi->bytes_per_word == 2) { > + u16 *buf = (u16 *)csi->txbuf; I think you can get rid of the casts by making rxbuf a const void *. > + > + for (i = 0; i < csi->words_to_transfer; i++) > + writel(buf[i], csi->base + CSI_OFIFO); > + } else { > + u8 *buf = (u8 *)csi->txbuf; > + > + for (i = 0; i < csi->words_to_transfer; i++) > + writel(buf[i], csi->base + CSI_OFIFO); > + } > + > + csi->txbuf += csi->bytes_to_transfer; > + csi->bytes_sent += csi->bytes_to_transfer; > + > + return 0; > +} > + > +static int rzv2m_csi_read_rxfifo(struct rzv2m_csi_priv *csi) > +{ > + int i; > + > + if (readl(csi->base + CSI_IFIFOL) != csi->bytes_to_transfer) > + return -EIO; > + > + if (csi->bytes_per_word == 2) { > + u16 *buf = (u16 *)csi->rxbuf; Similar for rxbuf. > + > + for (i = 0; i < csi->words_to_transfer; i++) > + buf[i] = (u16)readl(csi->base + CSI_IFIFO); > + } else { > + u8 *buf = (u8 *)csi->rxbuf; > + > + for (i = 0; i < csi->words_to_transfer; i++) > + buf[i] = (u8)readl(csi->base + CSI_IFIFO); > + } > + > + csi->rxbuf += csi->bytes_to_transfer; > + csi->bytes_received += csi->bytes_to_transfer; > + > + return 0; > +} > + > +static inline void rzv2m_csi_calc_current_transfer(struct rzv2m_csi_priv *csi) > +{ > + int bytes_transferred = max_t(int, csi->bytes_received, csi->bytes_sent); > + int bytes_remaining = csi->buffer_len - bytes_transferred; > + int to_transfer; unsigned... > + > + if (csi->txbuf) > + /* > + * Leaving a little bit of headroom in the FIFOs makes it very > + * hard to raise an overflow error (which is only possible > + * when IP transmits and receives at the same time). > + */ > + to_transfer = min_t(int, CSI_FIFO_HALF_SIZE, bytes_remaining); > + else > + to_transfer = min_t(int, CSI_FIFO_SIZE_BYTES, bytes_remaining); Why min_t(int, ...)? Both values are int. It would be better to make both unsigned, though. > + > + if (csi->bytes_per_word == 2) > + to_transfer >>= 1; > + > + /* > + * We can only choose a trigger level from a predefined set of values. > + * This will pick a value that is the greatest possible integer that's > + * less than or equal to the number of bytes we need to transfer. > + * This may result in multiple smaller transfers. > + */ > + csi->words_to_transfer = x_trg_words[to_transfer - 1]; > + > + if (csi->bytes_per_word == 2) > + csi->bytes_to_transfer = csi->words_to_transfer << 1; > + else > + csi->bytes_to_transfer = csi->words_to_transfer; > +} Gr{oetje,eeting}s, Geert
Mon, Jul 03, 2023 at 12:19:26PM +0200, Geert Uytterhoeven kirjoitti: > On Thu, Jun 22, 2023 at 1:34 PM Fabrizio Castro > <fabrizio.castro.jz@renesas.com> wrote: ... > > + if (csi->txbuf) > > + /* > > + * Leaving a little bit of headroom in the FIFOs makes it very > > + * hard to raise an overflow error (which is only possible > > + * when IP transmits and receives at the same time). > > + */ > > + to_transfer = min_t(int, CSI_FIFO_HALF_SIZE, bytes_remaining); > > + else > > + to_transfer = min_t(int, CSI_FIFO_SIZE_BYTES, bytes_remaining); > > Why min_t(int, ...)? Both values are int. min_t() should be used with a great care. > It would be better to make both unsigned, though. I believe you are assuming 3 (three) values and not 2 (two) under "both" (one variable and two definitions).
Thu, Jun 22, 2023 at 12:33:39PM +0100, Fabrizio Castro kirjoitti: > The RZ/V2M SoC comes with the Clocked Serial Interface (CSI) > IP, which is a master/slave SPI controller. > > This commit adds a driver to support CSI master mode. Submitting Patches recommends to use imperative voice. ... + bits.h > +#include <linux/clk.h> > +#include <linux/count_zeros.h> > +#include <linux/interrupt.h> > +#include <linux/iopoll.h> > +#include <linux/platform_device.h> > +#include <linux/reset.h> > +#include <linux/spi/spi.h> ... > +#define CSI_CKS_MAX 0x3FFF If it's limited by number of bits, i would explicitly use that information as (BIT(14) - 1). ... > +#define CSI_MAX_SPI_SCKO 8000000 Units? Also, HZ_PER_MHZ? ... > +static const unsigned char x_trg[] = { > + 0, 1, 1, 2, 2, 2, 2, 3, > + 3, 3, 3, 3, 3, 3, 3, 4, > + 4, 4, 4, 4, 4, 4, 4, 4, > + 4, 4, 4, 4, 4, 4, 4, 5 > +}; > + > +static const unsigned char x_trg_words[] = { > + 1, 2, 2, 4, 4, 4, 4, 8, > + 8, 8, 8, 8, 8, 8, 8, 16, > + 16, 16, 16, 16, 16, 16, 16, 16, > + 16, 16, 16, 16, 16, 16, 16, 32 > +}; Why do you need tables? At the first glance these values can be easily calculated from indexes. ... > + rzv2m_csi_reg_write_bit(csi, CSI_CNT, CSI_CNT_CSIRST, assert); > + > + if (assert) { If (!assert) return 0; > + return readl_poll_timeout(csi->base + CSI_MODE, reg, > + !(reg & CSI_MODE_CSOT), 0, > + CSI_EN_DIS_TIMEOUT_US); > + } > + > + return 0; ... > + rzv2m_csi_reg_write_bit(csi, CSI_MODE, CSI_MODE_CSIE, enable); > + > + if (!enable && wait) In the similar way. > + return readl_poll_timeout(csi->base + CSI_MODE, reg, > + !(reg & CSI_MODE_CSOT), 0, > + CSI_EN_DIS_TIMEOUT_US); > + > + return 0; ... > + for (i = 0; i < csi->words_to_transfer; i++) > + writel(buf[i], csi->base + CSI_OFIFO); writesl()? ... > + for (i = 0; i < csi->words_to_transfer; i++) > + buf[i] = (u16)readl(csi->base + CSI_IFIFO); readsl()? ... > + for (i = 0; i < csi->words_to_transfer; i++) > + buf[i] = (u8)readl(csi->base + CSI_IFIFO); readsl()? ... Yes, in read cases you would need carefully handle the buffer data. Or drop the idea if it looks too monsterous. ... > + ret = rzv2m_csi_wait_for_interrupt(csi, CSI_INT_TREND, CSI_CNT_TREND_E); > + Unneeded blank line. > + if (ret == -ETIMEDOUT) > + csi->errors |= TX_TIMEOUT_ERROR; ... > + struct rzv2m_csi_priv *csi = (struct rzv2m_csi_priv *)data; From/to void * does not need an explicit casting in C. ... > + /* Setup clock polarity and phase timing */ > + rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_CKP, > + !(spi->mode & SPI_CPOL)); > + rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_DAP, > + !(spi->mode & SPI_CPHA)); Is it a must to do in a sequential writes? ... > + bool tx_completed = csi->txbuf ? false : true; > + bool rx_completed = csi->rxbuf ? false : true; Ternaries are redundant in the above. ... > + controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST; SPI_MODE_X_MASK ... > + controller->dev.of_node = pdev->dev.of_node; device_set_node();
Hi Andy, On Wed, Jul 5, 2023 at 12:24 PM <andy.shevchenko@gmail.com> wrote: > Mon, Jul 03, 2023 at 12:19:26PM +0200, Geert Uytterhoeven kirjoitti: > > On Thu, Jun 22, 2023 at 1:34 PM Fabrizio Castro > > <fabrizio.castro.jz@renesas.com> wrote: > > > + if (csi->txbuf) > > > + /* > > > + * Leaving a little bit of headroom in the FIFOs makes it very > > > + * hard to raise an overflow error (which is only possible > > > + * when IP transmits and receives at the same time). > > > + */ > > > + to_transfer = min_t(int, CSI_FIFO_HALF_SIZE, bytes_remaining); > > > + else > > > + to_transfer = min_t(int, CSI_FIFO_SIZE_BYTES, bytes_remaining); > > > > Why min_t(int, ...)? Both values are int. > > min_t() should be used with a great care. > > > It would be better to make both unsigned, though. > > I believe you are assuming 3 (three) values and not 2 (two) under "both" > (one variable and two definitions). :-) I meant "both numerical parametric values of each minimum operation". Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for your feedback! > From: Geert Uytterhoeven <geert@linux-m68k.org> > Subject: Re: [PATCH v2 3/5] spi: Add support for Renesas CSI > > Hi Fabrizio, > > On Thu, Jun 22, 2023 at 1:34 PM Fabrizio Castro > <fabrizio.castro.jz@renesas.com> wrote: > > The RZ/V2M SoC comes with the Clocked Serial Interface (CSI) > > IP, which is a master/slave SPI controller. > > > > This commit adds a driver to support CSI master mode. > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > > --- > > > > v2: edited includes in drivers/spi/spi-rzv2m-csi.c > > Thanks for your patch! > > > --- a/drivers/spi/Kconfig > > +++ b/drivers/spi/Kconfig > > @@ -825,6 +825,12 @@ config SPI_RSPI > > help > > SPI driver for Renesas RSPI and QSPI blocks. > > > > +config SPI_RZV2M_CSI > > + tristate "Renesas RZV2M CSI controller" > > RZ/V2M (patch sent) Thanks for this. > > > + depends on ARCH_RENESAS || COMPILE_TEST > > + help > > + SPI driver for Renesas RZ/V2M Clocked Serial Interface (CSI) > > + > > config SPI_QCOM_QSPI > > tristate "QTI QSPI controller" > > depends on ARCH_QCOM || COMPILE_TEST > > > --- /dev/null > > +++ b/drivers/spi/spi-rzv2m-csi.c > > @@ -0,0 +1,667 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * Renesas RZ/V2M Clocked Serial Interface (CSI) driver > > + * > > + * Copyright (C) 2023 Renesas Electronics Corporation > > + */ > > + > > +#include <linux/clk.h> > > +#include <linux/count_zeros.h> > > +#include <linux/interrupt.h> > > +#include <linux/iopoll.h> > > +#include <linux/platform_device.h> > > +#include <linux/reset.h> > > +#include <linux/spi/spi.h> > > + > > +/* Registers */ > > +#define CSI_MODE 0x00 /* CSI mode control */ > > +#define CSI_CLKSEL 0x04 /* CSI clock select */ > > +#define CSI_CNT 0x08 /* CSI control */ > > +#define CSI_INT 0x0C /* CSI interrupt status */ > > +#define CSI_IFIFOL 0x10 /* CSI receive FIFO level display > */ > > +#define CSI_OFIFOL 0x14 /* CSI transmit FIFO level display > */ > > +#define CSI_IFIFO 0x18 /* CSI receive window */ > > +#define CSI_OFIFO 0x1C /* CSI transmit window */ > > +#define CSI_FIFOTRG 0x20 /* CSI FIFO trigger level */ > > + > > +/* CSI_MODE */ > > +#define CSI_MODE_CSIE BIT(7) > > +#define CSI_MODE_TRMD BIT(6) > > +#define CSI_MODE_CCL BIT(5) > > +#define CSI_MODE_DIR BIT(4) > > +#define CSI_MODE_CSOT BIT(0) > > + > > +#define CSI_MODE_SETUP 0x00000040 > > + > > +/* CSI_CLKSEL */ > > +#define CSI_CLKSEL_CKP BIT(17) > > +#define CSI_CLKSEL_DAP BIT(16) > > +#define CSI_CLKSEL_SLAVE BIT(15) > > +#define CSI_CLKSEL_CKS GENMASK(14, 1) > > + > > +/* CSI_CNT */ > > +#define CSI_CNT_CSIRST BIT(28) > > +#define CSI_CNT_R_TRGEN BIT(19) > > +#define CSI_CNT_UNDER_E BIT(13) > > +#define CSI_CNT_OVERF_E BIT(12) > > +#define CSI_CNT_TREND_E BIT(9) > > +#define CSI_CNT_CSIEND_E BIT(8) > > +#define CSI_CNT_T_TRGR_E BIT(4) > > +#define CSI_CNT_R_TRGR_E BIT(0) > > + > > +/* CSI_INT */ > > +#define CSI_INT_UNDER BIT(13) > > +#define CSI_INT_OVERF BIT(12) > > +#define CSI_INT_TREND BIT(9) > > +#define CSI_INT_CSIEND BIT(8) > > +#define CSI_INT_T_TRGR BIT(4) > > +#define CSI_INT_R_TRGR BIT(0) > > + > > +/* CSI_FIFOTRG */ > > +#define CSI_FIFOTRG_R_TRG GENMASK(2, 0) > > + > > +#define CSI_FIFO_SIZE_BYTES 32 > > +#define CSI_FIFO_HALF_SIZE 16 > > +#define CSI_EN_DIS_TIMEOUT_US 100 > > +#define CSI_CKS_MAX 0x3FFF > > + > > +#define UNDERRUN_ERROR BIT(0) > > +#define OVERFLOW_ERROR BIT(1) > > +#define TX_TIMEOUT_ERROR BIT(2) > > +#define RX_TIMEOUT_ERROR BIT(3) > > + > > +#define CSI_MAX_SPI_SCKO 8000000 > > + > > +struct rzv2m_csi_priv { > > + void __iomem *base; > > + struct clk *csiclk; > > + struct clk *pclk; > > + struct device *dev; > > + struct spi_controller *controller; > > + const u8 *txbuf; > > + u8 *rxbuf; > > + int buffer_len; > > + int bytes_sent; > > + int bytes_received; > > + int bytes_to_transfer; > > + int words_to_transfer; > > All these ints should be unsigned. Will change. > > > + unsigned char bytes_per_word; > > 3-byte gap > > > + wait_queue_head_t wait; > > + u8 errors; > > 3 byte gap > > > + u32 status; > > +}; I'll go over the gaps and improve. > > > + > > +static int rzv2m_csi_fill_txfifo(struct rzv2m_csi_priv *csi) > > +{ > > + int i; > > unsigned int Will change. > > > + > > + if (readl(csi->base + CSI_OFIFOL)) > > + return -EIO; > > + > > + if (csi->bytes_per_word == 2) { > > + u16 *buf = (u16 *)csi->txbuf; > > I think you can get rid of the casts by making rxbuf a const void *. I'll try and see if I can improve this. > > > + > > + for (i = 0; i < csi->words_to_transfer; i++) > > + writel(buf[i], csi->base + CSI_OFIFO); > > + } else { > > + u8 *buf = (u8 *)csi->txbuf; > > + > > + for (i = 0; i < csi->words_to_transfer; i++) > > + writel(buf[i], csi->base + CSI_OFIFO); > > + } > > + > > + csi->txbuf += csi->bytes_to_transfer; > > + csi->bytes_sent += csi->bytes_to_transfer; > > + > > + return 0; > > +} > > + > > +static int rzv2m_csi_read_rxfifo(struct rzv2m_csi_priv *csi) > > +{ > > + int i; > > + > > + if (readl(csi->base + CSI_IFIFOL) != csi->bytes_to_transfer) > > + return -EIO; > > + > > + if (csi->bytes_per_word == 2) { > > + u16 *buf = (u16 *)csi->rxbuf; > > Similar for rxbuf. Okay. > > > + > > + for (i = 0; i < csi->words_to_transfer; i++) > > + buf[i] = (u16)readl(csi->base + CSI_IFIFO); > > + } else { > > + u8 *buf = (u8 *)csi->rxbuf; > > + > > + for (i = 0; i < csi->words_to_transfer; i++) > > + buf[i] = (u8)readl(csi->base + CSI_IFIFO); > > + } > > + > > + csi->rxbuf += csi->bytes_to_transfer; > > + csi->bytes_received += csi->bytes_to_transfer; > > + > > + return 0; > > +} > > + > > +static inline void rzv2m_csi_calc_current_transfer(struct rzv2m_csi_priv > *csi) > > +{ > > + int bytes_transferred = max_t(int, csi->bytes_received, csi- > >bytes_sent); > > + int bytes_remaining = csi->buffer_len - bytes_transferred; > > + int to_transfer; > > unsigned... Okay > > > + > > + if (csi->txbuf) > > + /* > > + * Leaving a little bit of headroom in the FIFOs makes it > very > > + * hard to raise an overflow error (which is only possible > > + * when IP transmits and receives at the same time). > > + */ > > + to_transfer = min_t(int, CSI_FIFO_HALF_SIZE, > bytes_remaining); > > + else > > + to_transfer = min_t(int, CSI_FIFO_SIZE_BYTES, > bytes_remaining); > > Why min_t(int, ...)? Both values are int. > > It would be better to make both unsigned, though. Will do. > > > + > > + if (csi->bytes_per_word == 2) > > + to_transfer >>= 1; > > + > > + /* > > + * We can only choose a trigger level from a predefined set of > values. > > + * This will pick a value that is the greatest possible integer > that's > > + * less than or equal to the number of bytes we need to transfer. > > + * This may result in multiple smaller transfers. > > + */ > > + csi->words_to_transfer = x_trg_words[to_transfer - 1]; > > + > > + if (csi->bytes_per_word == 2) > > + csi->bytes_to_transfer = csi->words_to_transfer << 1; > > + else > > + csi->bytes_to_transfer = csi->words_to_transfer; > > +} Thanks, Fab > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux- > m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like > that. > -- Linus Torvalds
Hi Andy, Thanks for your feedback! > From: andy.shevchenko@gmail.com <andy.shevchenko@gmail.com> > Subject: Re: [PATCH v2 3/5] spi: Add support for Renesas CSI > > Thu, Jun 22, 2023 at 12:33:39PM +0100, Fabrizio Castro kirjoitti: > > The RZ/V2M SoC comes with the Clocked Serial Interface (CSI) > > IP, which is a master/slave SPI controller. > > > > This commit adds a driver to support CSI master mode. > > Submitting Patches recommends to use imperative voice. > > ... > > + bits.h Okay > > > +#include <linux/clk.h> > > +#include <linux/count_zeros.h> > > +#include <linux/interrupt.h> > > +#include <linux/iopoll.h> > > +#include <linux/platform_device.h> > > +#include <linux/reset.h> > > +#include <linux/spi/spi.h> > > ... > > > +#define CSI_CKS_MAX 0x3FFF > > If it's limited by number of bits, i would explicitly use that information > as > (BIT(14) - 1). That value represents the register setting for the maximum clock divider. The maximum divider and corresponding register setting are plainly stated in the HW User Manual, therefore I would like to use either (plain) value to make it easier for the reader. I think perhaps the below makes this clearer: #define CSI_CKS_MAX_DIV_RATIO 32766 #define CSI_CKS_MAX (CSI_CKS_MAX_DIV_RATIO >> 1) > > ... > > > +#define CSI_MAX_SPI_SCKO 8000000 > > Units? > Also, HZ_PER_MHZ? I'll replace that with: #define CSI_MAX_SPI_SCKO (8 * HZ_PER_MHZ) > > ... > > > +static const unsigned char x_trg[] = { > > + 0, 1, 1, 2, 2, 2, 2, 3, > > + 3, 3, 3, 3, 3, 3, 3, 4, > > + 4, 4, 4, 4, 4, 4, 4, 4, > > + 4, 4, 4, 4, 4, 4, 4, 5 > > +}; > > + > > +static const unsigned char x_trg_words[] = { > > + 1, 2, 2, 4, 4, 4, 4, 8, > > + 8, 8, 8, 8, 8, 8, 8, 16, > > + 16, 16, 16, 16, 16, 16, 16, 16, > > + 16, 16, 16, 16, 16, 16, 16, 32 > > +}; > > Why do you need tables? At the first glance these values can be easily > calculated from indexes. I think I am going to replace those tables with the below (and of course adjust the callers accordingly since the argument is not an index anymore): static inline unsigned int x_trg(unsigned int words) { return fls(words) - 1; } static inline unsigned int x_trg_words(unsigned int words) { return 1 << x_trg(words); } > > ... > > > + rzv2m_csi_reg_write_bit(csi, CSI_CNT, CSI_CNT_CSIRST, assert); > > + > > + if (assert) { > > If (!assert) > return 0; Can do > > > + return readl_poll_timeout(csi->base + CSI_MODE, reg, > > + !(reg & CSI_MODE_CSOT), 0, > > + CSI_EN_DIS_TIMEOUT_US); > > + } > > + > > + return 0; > > ... > > > + rzv2m_csi_reg_write_bit(csi, CSI_MODE, CSI_MODE_CSIE, enable); > > + > > + if (!enable && wait) > > In the similar way. Okay > > > + return readl_poll_timeout(csi->base + CSI_MODE, reg, > > + !(reg & CSI_MODE_CSOT), 0, > > + CSI_EN_DIS_TIMEOUT_US); > > + > > + return 0; > > ... > > > + for (i = 0; i < csi->words_to_transfer; i++) > > + writel(buf[i], csi->base + CSI_OFIFO); > > writesl()? I think you mean writesw and writesb. They should simplify things a bit, I'll go for them. > > ... > > > + for (i = 0; i < csi->words_to_transfer; i++) > > + buf[i] = (u16)readl(csi->base + CSI_IFIFO); > > readsl()? I'll use readsw > > ... > > > + for (i = 0; i < csi->words_to_transfer; i++) > > + buf[i] = (u8)readl(csi->base + CSI_IFIFO); > > readsl()? I'll use readsb > > ... > > Yes, in read cases you would need carefully handle the buffer data. > Or drop the idea if it looks too monsterous. It should be okay in my case. Thanks. > > ... > > > + ret = rzv2m_csi_wait_for_interrupt(csi, CSI_INT_TREND, > CSI_CNT_TREND_E); > > > + > > Unneeded blank line. Will take out > > > + if (ret == -ETIMEDOUT) > > + csi->errors |= TX_TIMEOUT_ERROR; > > ... > > > + struct rzv2m_csi_priv *csi = (struct rzv2m_csi_priv *)data; > > From/to void * does not need an explicit casting in C. Of course. > > ... > > > + /* Setup clock polarity and phase timing */ > > + rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_CKP, > > + !(spi->mode & SPI_CPOL)); > > + rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_DAP, > > + !(spi->mode & SPI_CPHA)); > > Is it a must to do in a sequential writes? It's not a must, I'll combine those 2 statements into 1. > > ... > > > + bool tx_completed = csi->txbuf ? false : true; > > + bool rx_completed = csi->rxbuf ? false : true; > > Ternaries are redundant in the above. They are also horrible, the below should do: bool tx_completed = !csi->txbuf; bool rx_completed = !csi->rxbuf; > > ... > > > + controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST; > > SPI_MODE_X_MASK This statement sets the mode_bits. Using a macro meant to be used as a mask in this context is something I would want to avoid if possible. > > ... > > > + controller->dev.of_node = pdev->dev.of_node; > > device_set_node(); Will change. I'll send an enhancement patch shortly to reflect your suggestions and also Geert's. Thanks for your help! Cheers, Fab > > -- > With Best Regards, > Andy Shevchenko >
On Thu, Jul 13, 2023 at 6:52 PM Fabrizio Castro <fabrizio.castro.jz@renesas.com> wrote: ... > > > +#define CSI_CKS_MAX 0x3FFF > > > > If it's limited by number of bits, i would explicitly use that information > > as > > (BIT(14) - 1). > > That value represents the register setting for the maximum clock divider. > The maximum divider and corresponding register setting are plainly stated > in the HW User Manual, therefore I would like to use either (plain) value > to make it easier for the reader. > > I think perhaps the below makes this clearer: > #define CSI_CKS_MAX_DIV_RATIO 32766 Hmm... To me it's a bit confusing now. Shouldn't it be 32767? > #define CSI_CKS_MAX (CSI_CKS_MAX_DIV_RATIO >> 1) Whatever you choose it would be better to add a comment to explain this. Because the above is more clear to me with BIT(14)-1 if the register field is 14-bit long. With this value(s) I'm lost. Definitely needs a comment. ... > > > +static const unsigned char x_trg[] = { > > > + 0, 1, 1, 2, 2, 2, 2, 3, > > > + 3, 3, 3, 3, 3, 3, 3, 4, > > > + 4, 4, 4, 4, 4, 4, 4, 4, > > > + 4, 4, 4, 4, 4, 4, 4, 5 > > > +}; > > > + > > > +static const unsigned char x_trg_words[] = { > > > + 1, 2, 2, 4, 4, 4, 4, 8, > > > + 8, 8, 8, 8, 8, 8, 8, 16, > > > + 16, 16, 16, 16, 16, 16, 16, 16, > > > + 16, 16, 16, 16, 16, 16, 16, 32 > > > +}; > > > > Why do you need tables? At the first glance these values can be easily > > calculated from indexes. > > I think I am going to replace those tables with the below (and of course > adjust the callers accordingly since the argument is not an index anymore): > > static inline unsigned int x_trg(unsigned int words) > { > return fls(words) - 1; > } OK, but I think you can use it just inplace, no need to have such as a standalone function. > static inline unsigned int x_trg_words(unsigned int words) > { > return 1 << x_trg(words); > } Besides a better form of BIT(...) this looks to me like NIH roundup_pow_of_two(). ... > > > + /* Setup clock polarity and phase timing */ > > > + rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_CKP, > > > + !(spi->mode & SPI_CPOL)); > > > + rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_DAP, > > > + !(spi->mode & SPI_CPHA)); > > > > Is it a must to do in a sequential writes? > > It's not a must, I'll combine those 2 statements into 1. If so, you can use SPI_MODE_X_MASK. ... > > > + controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST; > > > > SPI_MODE_X_MASK > > This statement sets the mode_bits. Using a macro meant to be used as a > mask in this context is something I would want to avoid if possible. Hmm... not a big deal, but I think that's what covers all mode_bits, and mode_bits by nature _is_ a mask.
Hi Andy, Thanks for your reply! > From: Andy Shevchenko <andy.shevchenko@gmail.com> > Subject: Re: [PATCH v2 3/5] spi: Add support for Renesas CSI > > On Thu, Jul 13, 2023 at 6:52 PM Fabrizio Castro > <fabrizio.castro.jz@renesas.com> wrote: > > ... > > > > > +#define CSI_CKS_MAX 0x3FFF > > > > > > If it's limited by number of bits, i would explicitly use that > information > > > as > > > (BIT(14) - 1). > > > > That value represents the register setting for the maximum clock > divider. > > The maximum divider and corresponding register setting are plainly > stated > > in the HW User Manual, therefore I would like to use either (plain) > value > > to make it easier for the reader. > > > > I think perhaps the below makes this clearer: > > #define CSI_CKS_MAX_DIV_RATIO 32766 > > Hmm... To me it's a bit confusing now. Shouldn't it be 32767? 32766 is the correct value. Clock "csiclk" gets divided by 2 * CSI_CLKSEL_CKS in order to generate the serial clock (output from master), with CSI_CLKSEL_CKS ranging from 0x1 (that means "csiclk" is divided by 2) to 0x3FFF ("csiclk" is divided by 32766). > > > #define CSI_CKS_MAX (CSI_CKS_MAX_DIV_RATIO >> 1) > > Whatever you choose it would be better to add a comment to explain > this. Because the above is more clear to me with BIT(14)-1 if the > register field is 14-bit long. > With this value(s) I'm lost. Definitely needs a comment. To cater for a wider audience (and not just for those who have read the HW manual), I think perhaps the below would probably be the best compromise: /* * Clock "csiclk" gets divided by 2 * CSI_CLKSEL_CKS in order to generate the * serial clock (output from master), with CSI_CLKSEL_CKS ranging from 0x1 (that * means "csiclk" is divided by 2) to 0x3FFF ("csiclk" is divided by 32766). */ #define CSI_CKS_MAX (BIT(14)-1) > > ... > > > > > static inline unsigned int x_trg(unsigned int words) > > { > > return fls(words) - 1; > > } > > OK, but I think you can use it just inplace, no need to have such as a > standalone function. The above is actually equivalent to ilog2() > > > static inline unsigned int x_trg_words(unsigned int words) > > { > > return 1 << x_trg(words); > > } > > Besides a better form of BIT(...) this looks to me like NIH > roundup_pow_of_two(). rounddown_pow_of_two(). I have tested the driver with s/x_trg/ilog2 and s/x_trg_words/roundup_pow_of_two and it looks like I am losing tiny bit of performance (probably down to the use of ternary operators in both macros) but I think it's okay, let's not reinvent the wheel and let's keep it more readable, I'll switch to using the above macros. > > ... > > > > > + /* Setup clock polarity and phase timing */ > > > > + rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_CKP, > > > > + !(spi->mode & SPI_CPOL)); > > > > + rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_DAP, > > > > + !(spi->mode & SPI_CPHA)); > > > > > > Is it a must to do in a sequential writes? > > > > It's not a must, I'll combine those 2 statements into 1. > > If so, you can use SPI_MODE_X_MASK. That's the plan. Thanks for your help Andy. Cheers, Fab > > ... > > > > > + controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST; > > > > > > SPI_MODE_X_MASK > > > > This statement sets the mode_bits. Using a macro meant to be used as > a > > mask in this context is something I would want to avoid if possible. > > Hmm... not a big deal, but I think that's what covers all mode_bits, > and mode_bits by nature _is_ a mask. > > -- > With Best Regards, > Andy Shevchenko
Hi Fabrizio, On Fri, Jul 14, 2023 at 12:35 AM Fabrizio Castro <fabrizio.castro.jz@renesas.com> wrote: > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > Subject: Re: [PATCH v2 3/5] spi: Add support for Renesas CSI > > > > On Thu, Jul 13, 2023 at 6:52 PM Fabrizio Castro > > <fabrizio.castro.jz@renesas.com> wrote: > > > > ... > > > > > > > +#define CSI_CKS_MAX 0x3FFF > > > > > > > > If it's limited by number of bits, i would explicitly use that > > information > > > > as > > > > (BIT(14) - 1). > > > > > > That value represents the register setting for the maximum clock > > divider. > > > The maximum divider and corresponding register setting are plainly > > stated > > > in the HW User Manual, therefore I would like to use either (plain) > > value > > > to make it easier for the reader. > > > > > > I think perhaps the below makes this clearer: > > > #define CSI_CKS_MAX_DIV_RATIO 32766 > > > > Hmm... To me it's a bit confusing now. Shouldn't it be 32767? > > 32766 is the correct value. > > Clock "csiclk" gets divided by 2 * CSI_CLKSEL_CKS in order to generate the > serial clock (output from master), with CSI_CLKSEL_CKS ranging from 0x1 (that > means "csiclk" is divided by 2) to 0x3FFF ("csiclk" is divided by 32766). > > > > > > #define CSI_CKS_MAX (CSI_CKS_MAX_DIV_RATIO >> 1) > > > > Whatever you choose it would be better to add a comment to explain > > this. Because the above is more clear to me with BIT(14)-1 if the > > register field is 14-bit long. > > With this value(s) I'm lost. Definitely needs a comment. > > To cater for a wider audience (and not just for those who have read the > HW manual), I think perhaps the below would probably be the best compromise: > > /* > * Clock "csiclk" gets divided by 2 * CSI_CLKSEL_CKS in order to generate the > * serial clock (output from master), with CSI_CLKSEL_CKS ranging from 0x1 (that > * means "csiclk" is divided by 2) to 0x3FFF ("csiclk" is divided by 32766). > */ > #define CSI_CKS_MAX (BIT(14)-1) Or GENMASK(13, 0) As we have #define CSI_CLKSEL_CKS GENMASK(14, 1) and bit 0 must of the CLKSEL register must always be zero, the actual divider is incidentally FIELD_GET(GENMASK(14, 0), clksel). No idea if that can be useful to simplify the code, though ;-) > > > static inline unsigned int x_trg(unsigned int words) > > > { > > > return fls(words) - 1; > > > } > > > > OK, but I think you can use it just inplace, no need to have such as a > > standalone function. > > The above is actually equivalent to ilog2() > > > > > > static inline unsigned int x_trg_words(unsigned int words) > > > { > > > return 1 << x_trg(words); > > > } > > > > Besides a better form of BIT(...) this looks to me like NIH > > roundup_pow_of_two(). > > rounddown_pow_of_two(). > > I have tested the driver with s/x_trg/ilog2 and > s/x_trg_words/roundup_pow_of_two and it looks like I am losing tiny bit of > performance (probably down to the use of ternary operators in both macros) > but I think it's okay, let's not reinvent the wheel and let's keep it more > readable, I'll switch to using the above macros. You mean this is not lost in the noise of the big loop in rzv2m_csi_pio_transfer(), which is even waiting on an event? I find that a bit surprising... Gr{oetje,eeting}s, Geert
Hi Geert, Thanks your reply! > From: Geert Uytterhoeven <geert@linux-m68k.org> > Subject: Re: [PATCH v2 3/5] spi: Add support for Renesas CSI > > Hi Fabrizio, > > On Fri, Jul 14, 2023 at 12:35 AM Fabrizio Castro > <fabrizio.castro.jz@renesas.com> wrote: > > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > > Subject: Re: [PATCH v2 3/5] spi: Add support for Renesas CSI > > > > > > On Thu, Jul 13, 2023 at 6:52 PM Fabrizio Castro > > > <fabrizio.castro.jz@renesas.com> wrote: > > > > > > ... > > > > > > > > > +#define CSI_CKS_MAX 0x3FFF > > > > > > > > > > If it's limited by number of bits, i would explicitly use that > > > information > > > > > as > > > > > (BIT(14) - 1). > > > > > > > > That value represents the register setting for the maximum clock > > > divider. > > > > The maximum divider and corresponding register setting are > plainly > > > stated > > > > in the HW User Manual, therefore I would like to use either > (plain) > > > value > > > > to make it easier for the reader. > > > > > > > > I think perhaps the below makes this clearer: > > > > #define CSI_CKS_MAX_DIV_RATIO 32766 > > > > > > Hmm... To me it's a bit confusing now. Shouldn't it be 32767? > > > > 32766 is the correct value. > > > > Clock "csiclk" gets divided by 2 * CSI_CLKSEL_CKS in order to > generate the > > serial clock (output from master), with CSI_CLKSEL_CKS ranging from > 0x1 (that > > means "csiclk" is divided by 2) to 0x3FFF ("csiclk" is divided by > 32766). > > > > > > > > > #define CSI_CKS_MAX (CSI_CKS_MAX_DIV_RATIO >> 1) > > > > > > Whatever you choose it would be better to add a comment to explain > > > this. Because the above is more clear to me with BIT(14)-1 if the > > > register field is 14-bit long. > > > With this value(s) I'm lost. Definitely needs a comment. > > > > To cater for a wider audience (and not just for those who have read > the > > HW manual), I think perhaps the below would probably be the best > compromise: > > > > /* > > * Clock "csiclk" gets divided by 2 * CSI_CLKSEL_CKS in order to > generate the > > * serial clock (output from master), with CSI_CLKSEL_CKS ranging > from 0x1 (that > > * means "csiclk" is divided by 2) to 0x3FFF ("csiclk" is divided by > 32766). > > */ > > #define CSI_CKS_MAX (BIT(14)-1) > > Or GENMASK(13, 0) Yeah. > > As we have > > #define CSI_CLKSEL_CKS GENMASK(14, 1) > > and bit 0 must of the CLKSEL register must always be zero, the actual > divider is incidentally FIELD_GET(GENMASK(14, 0), clksel). > No idea if that can be useful to simplify the code, though ;-) Thanks for pointing this out. Will have a look, but no promises ;-) > > > > > static inline unsigned int x_trg(unsigned int words) > > > > { > > > > return fls(words) - 1; > > > > } > > > > > > OK, but I think you can use it just inplace, no need to have such > as a > > > standalone function. > > > > The above is actually equivalent to ilog2() > > > > > > > > > static inline unsigned int x_trg_words(unsigned int words) > > > > { > > > > return 1 << x_trg(words); > > > > } > > > > > > Besides a better form of BIT(...) this looks to me like NIH > > > roundup_pow_of_two(). > > > > rounddown_pow_of_two(). > > > > I have tested the driver with s/x_trg/ilog2 and > > s/x_trg_words/roundup_pow_of_two and it looks like I am losing tiny > bit of > > performance (probably down to the use of ternary operators in both > macros) > > but I think it's okay, let's not reinvent the wheel and let's keep > it more > > readable, I'll switch to using the above macros. > > You mean this is not lost in the noise of the big loop in > rzv2m_csi_pio_transfer(), which is even waiting on an event? > I find that a bit surprising... Those calculations get done when no TX/RX is in progress, and they are executed for every burst (as they are used to decide how many bytes in the FIFOs to use for the current burst), therefore they add a delay to the whole thing. It's only a tiny drop, about 0.4% . Cheers, Fab > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- > geert@linux-m68k.org > > In personal conversations with technical people, I call myself a > hacker. But > when I'm talking to journalists I just say "programmer" or something > like that. > -- Linus Torvalds
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index 14810d24733b..abbd1fb5fbc0 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -825,6 +825,12 @@ config SPI_RSPI help SPI driver for Renesas RSPI and QSPI blocks. +config SPI_RZV2M_CSI + tristate "Renesas RZV2M CSI controller" + depends on ARCH_RENESAS || COMPILE_TEST + help + SPI driver for Renesas RZ/V2M Clocked Serial Interface (CSI) + config SPI_QCOM_QSPI tristate "QTI QSPI controller" depends on ARCH_QCOM || COMPILE_TEST diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index 28c4817a8a74..080c2c1b3ec1 100644 --- a/drivers/spi/Makefile +++ b/drivers/spi/Makefile @@ -113,6 +113,7 @@ obj-$(CONFIG_SPI_RB4XX) += spi-rb4xx.o obj-$(CONFIG_MACH_REALTEK_RTL) += spi-realtek-rtl.o obj-$(CONFIG_SPI_RPCIF) += spi-rpc-if.o obj-$(CONFIG_SPI_RSPI) += spi-rspi.o +obj-$(CONFIG_SPI_RZV2M_CSI) += spi-rzv2m-csi.o obj-$(CONFIG_SPI_S3C64XX) += spi-s3c64xx.o obj-$(CONFIG_SPI_SC18IS602) += spi-sc18is602.o obj-$(CONFIG_SPI_SH) += spi-sh.o diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c new file mode 100644 index 000000000000..14ad65da930d --- /dev/null +++ b/drivers/spi/spi-rzv2m-csi.c @@ -0,0 +1,667 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Renesas RZ/V2M Clocked Serial Interface (CSI) driver + * + * Copyright (C) 2023 Renesas Electronics Corporation + */ + +#include <linux/clk.h> +#include <linux/count_zeros.h> +#include <linux/interrupt.h> +#include <linux/iopoll.h> +#include <linux/platform_device.h> +#include <linux/reset.h> +#include <linux/spi/spi.h> + +/* Registers */ +#define CSI_MODE 0x00 /* CSI mode control */ +#define CSI_CLKSEL 0x04 /* CSI clock select */ +#define CSI_CNT 0x08 /* CSI control */ +#define CSI_INT 0x0C /* CSI interrupt status */ +#define CSI_IFIFOL 0x10 /* CSI receive FIFO level display */ +#define CSI_OFIFOL 0x14 /* CSI transmit FIFO level display */ +#define CSI_IFIFO 0x18 /* CSI receive window */ +#define CSI_OFIFO 0x1C /* CSI transmit window */ +#define CSI_FIFOTRG 0x20 /* CSI FIFO trigger level */ + +/* CSI_MODE */ +#define CSI_MODE_CSIE BIT(7) +#define CSI_MODE_TRMD BIT(6) +#define CSI_MODE_CCL BIT(5) +#define CSI_MODE_DIR BIT(4) +#define CSI_MODE_CSOT BIT(0) + +#define CSI_MODE_SETUP 0x00000040 + +/* CSI_CLKSEL */ +#define CSI_CLKSEL_CKP BIT(17) +#define CSI_CLKSEL_DAP BIT(16) +#define CSI_CLKSEL_SLAVE BIT(15) +#define CSI_CLKSEL_CKS GENMASK(14, 1) + +/* CSI_CNT */ +#define CSI_CNT_CSIRST BIT(28) +#define CSI_CNT_R_TRGEN BIT(19) +#define CSI_CNT_UNDER_E BIT(13) +#define CSI_CNT_OVERF_E BIT(12) +#define CSI_CNT_TREND_E BIT(9) +#define CSI_CNT_CSIEND_E BIT(8) +#define CSI_CNT_T_TRGR_E BIT(4) +#define CSI_CNT_R_TRGR_E BIT(0) + +/* CSI_INT */ +#define CSI_INT_UNDER BIT(13) +#define CSI_INT_OVERF BIT(12) +#define CSI_INT_TREND BIT(9) +#define CSI_INT_CSIEND BIT(8) +#define CSI_INT_T_TRGR BIT(4) +#define CSI_INT_R_TRGR BIT(0) + +/* CSI_FIFOTRG */ +#define CSI_FIFOTRG_R_TRG GENMASK(2, 0) + +#define CSI_FIFO_SIZE_BYTES 32 +#define CSI_FIFO_HALF_SIZE 16 +#define CSI_EN_DIS_TIMEOUT_US 100 +#define CSI_CKS_MAX 0x3FFF + +#define UNDERRUN_ERROR BIT(0) +#define OVERFLOW_ERROR BIT(1) +#define TX_TIMEOUT_ERROR BIT(2) +#define RX_TIMEOUT_ERROR BIT(3) + +#define CSI_MAX_SPI_SCKO 8000000 + +struct rzv2m_csi_priv { + void __iomem *base; + struct clk *csiclk; + struct clk *pclk; + struct device *dev; + struct spi_controller *controller; + const u8 *txbuf; + u8 *rxbuf; + int buffer_len; + int bytes_sent; + int bytes_received; + int bytes_to_transfer; + int words_to_transfer; + unsigned char bytes_per_word; + wait_queue_head_t wait; + u8 errors; + u32 status; +}; + +static const unsigned char x_trg[] = { + 0, 1, 1, 2, 2, 2, 2, 3, + 3, 3, 3, 3, 3, 3, 3, 4, + 4, 4, 4, 4, 4, 4, 4, 4, + 4, 4, 4, 4, 4, 4, 4, 5 +}; + +static const unsigned char x_trg_words[] = { + 1, 2, 2, 4, 4, 4, 4, 8, + 8, 8, 8, 8, 8, 8, 8, 16, + 16, 16, 16, 16, 16, 16, 16, 16, + 16, 16, 16, 16, 16, 16, 16, 32 +}; + +static void rzv2m_csi_reg_write_bit(const struct rzv2m_csi_priv *csi, + int reg_offs, int bit_mask, u32 value) +{ + int nr_zeros; + u32 tmp; + + nr_zeros = count_trailing_zeros(bit_mask); + value <<= nr_zeros; + + tmp = (readl(csi->base + reg_offs) & ~bit_mask) | value; + writel(tmp, csi->base + reg_offs); +} + +static int rzv2m_csi_sw_reset(struct rzv2m_csi_priv *csi, int assert) +{ + u32 reg; + + rzv2m_csi_reg_write_bit(csi, CSI_CNT, CSI_CNT_CSIRST, assert); + + if (assert) { + return readl_poll_timeout(csi->base + CSI_MODE, reg, + !(reg & CSI_MODE_CSOT), 0, + CSI_EN_DIS_TIMEOUT_US); + } + + return 0; +} + +static int rzv2m_csi_start_stop_operation(const struct rzv2m_csi_priv *csi, + int enable, bool wait) +{ + u32 reg; + + rzv2m_csi_reg_write_bit(csi, CSI_MODE, CSI_MODE_CSIE, enable); + + if (!enable && wait) + return readl_poll_timeout(csi->base + CSI_MODE, reg, + !(reg & CSI_MODE_CSOT), 0, + CSI_EN_DIS_TIMEOUT_US); + + return 0; +} + +static int rzv2m_csi_fill_txfifo(struct rzv2m_csi_priv *csi) +{ + int i; + + if (readl(csi->base + CSI_OFIFOL)) + return -EIO; + + if (csi->bytes_per_word == 2) { + u16 *buf = (u16 *)csi->txbuf; + + for (i = 0; i < csi->words_to_transfer; i++) + writel(buf[i], csi->base + CSI_OFIFO); + } else { + u8 *buf = (u8 *)csi->txbuf; + + for (i = 0; i < csi->words_to_transfer; i++) + writel(buf[i], csi->base + CSI_OFIFO); + } + + csi->txbuf += csi->bytes_to_transfer; + csi->bytes_sent += csi->bytes_to_transfer; + + return 0; +} + +static int rzv2m_csi_read_rxfifo(struct rzv2m_csi_priv *csi) +{ + int i; + + if (readl(csi->base + CSI_IFIFOL) != csi->bytes_to_transfer) + return -EIO; + + if (csi->bytes_per_word == 2) { + u16 *buf = (u16 *)csi->rxbuf; + + for (i = 0; i < csi->words_to_transfer; i++) + buf[i] = (u16)readl(csi->base + CSI_IFIFO); + } else { + u8 *buf = (u8 *)csi->rxbuf; + + for (i = 0; i < csi->words_to_transfer; i++) + buf[i] = (u8)readl(csi->base + CSI_IFIFO); + } + + csi->rxbuf += csi->bytes_to_transfer; + csi->bytes_received += csi->bytes_to_transfer; + + return 0; +} + +static inline void rzv2m_csi_calc_current_transfer(struct rzv2m_csi_priv *csi) +{ + int bytes_transferred = max_t(int, csi->bytes_received, csi->bytes_sent); + int bytes_remaining = csi->buffer_len - bytes_transferred; + int to_transfer; + + if (csi->txbuf) + /* + * Leaving a little bit of headroom in the FIFOs makes it very + * hard to raise an overflow error (which is only possible + * when IP transmits and receives at the same time). + */ + to_transfer = min_t(int, CSI_FIFO_HALF_SIZE, bytes_remaining); + else + to_transfer = min_t(int, CSI_FIFO_SIZE_BYTES, bytes_remaining); + + if (csi->bytes_per_word == 2) + to_transfer >>= 1; + + /* + * We can only choose a trigger level from a predefined set of values. + * This will pick a value that is the greatest possible integer that's + * less than or equal to the number of bytes we need to transfer. + * This may result in multiple smaller transfers. + */ + csi->words_to_transfer = x_trg_words[to_transfer - 1]; + + if (csi->bytes_per_word == 2) + csi->bytes_to_transfer = csi->words_to_transfer << 1; + else + csi->bytes_to_transfer = csi->words_to_transfer; +} + +static inline void rzv2m_csi_set_rx_fifo_trigger_level(struct rzv2m_csi_priv *csi) +{ + rzv2m_csi_reg_write_bit(csi, CSI_FIFOTRG, CSI_FIFOTRG_R_TRG, + x_trg[csi->words_to_transfer - 1]); +} + +static inline void rzv2m_csi_enable_rx_trigger(struct rzv2m_csi_priv *csi, + bool enable) +{ + rzv2m_csi_reg_write_bit(csi, CSI_CNT, CSI_CNT_R_TRGEN, enable); +} + +static void rzv2m_csi_disable_irqs(const struct rzv2m_csi_priv *csi, + u32 enable_bits) +{ + u32 cnt = readl(csi->base + CSI_CNT); + + writel(cnt & ~enable_bits, csi->base + CSI_CNT); +} + +static void rzv2m_csi_disable_all_irqs(struct rzv2m_csi_priv *csi) +{ + rzv2m_csi_disable_irqs(csi, CSI_CNT_R_TRGR_E | CSI_CNT_T_TRGR_E | + CSI_CNT_CSIEND_E | CSI_CNT_TREND_E | + CSI_CNT_OVERF_E | CSI_CNT_UNDER_E); +} + +static inline void rzv2m_csi_clear_irqs(struct rzv2m_csi_priv *csi, u32 irqs) +{ + writel(irqs, csi->base + CSI_INT); +} + +static void rzv2m_csi_clear_all_irqs(struct rzv2m_csi_priv *csi) +{ + rzv2m_csi_clear_irqs(csi, CSI_INT_UNDER | CSI_INT_OVERF | + CSI_INT_TREND | CSI_INT_CSIEND | CSI_INT_T_TRGR | + CSI_INT_R_TRGR); +} + +static void rzv2m_csi_enable_irqs(struct rzv2m_csi_priv *csi, u32 enable_bits) +{ + u32 cnt = readl(csi->base + CSI_CNT); + + writel(cnt | enable_bits, csi->base + CSI_CNT); +} + +static int rzv2m_csi_wait_for_interrupt(struct rzv2m_csi_priv *csi, + u32 wait_mask, u32 enable_bits) +{ + int ret; + + rzv2m_csi_enable_irqs(csi, enable_bits); + + ret = wait_event_timeout(csi->wait, + ((csi->status & wait_mask) == wait_mask) || + csi->errors, HZ); + + rzv2m_csi_disable_irqs(csi, enable_bits); + + if (csi->errors) + return -EIO; + + if (!ret) + return -ETIMEDOUT; + + return 0; +} + +static int rzv2m_csi_wait_for_tx_empty(struct rzv2m_csi_priv *csi) +{ + int ret; + + if (readl(csi->base + CSI_OFIFOL) == 0) + return 0; + + ret = rzv2m_csi_wait_for_interrupt(csi, CSI_INT_TREND, CSI_CNT_TREND_E); + + if (ret == -ETIMEDOUT) + csi->errors |= TX_TIMEOUT_ERROR; + + return ret; +} + +static inline int rzv2m_csi_wait_for_rx_ready(struct rzv2m_csi_priv *csi) +{ + int ret; + + if (readl(csi->base + CSI_IFIFOL) == csi->bytes_to_transfer) + return 0; + + ret = rzv2m_csi_wait_for_interrupt(csi, CSI_INT_R_TRGR, + CSI_CNT_R_TRGR_E); + + if (ret == -ETIMEDOUT) + csi->errors |= RX_TIMEOUT_ERROR; + + return ret; +} + +static irqreturn_t rzv2m_csi_irq_handler(int irq, void *data) +{ + struct rzv2m_csi_priv *csi = (struct rzv2m_csi_priv *)data; + + csi->status = readl(csi->base + CSI_INT); + rzv2m_csi_disable_irqs(csi, csi->status); + + if (csi->status & CSI_INT_OVERF) + csi->errors |= OVERFLOW_ERROR; + if (csi->status & CSI_INT_UNDER) + csi->errors |= UNDERRUN_ERROR; + + wake_up(&csi->wait); + + return IRQ_HANDLED; +} + +static void rzv2m_csi_setup_clock(struct rzv2m_csi_priv *csi, u32 spi_hz) +{ + unsigned long csiclk_rate = clk_get_rate(csi->csiclk); + unsigned long pclk_rate = clk_get_rate(csi->pclk); + unsigned long csiclk_rate_limit = pclk_rate >> 1; + u32 cks; + + /* + * There is a restriction on the frequency of CSICLK, it has to be <= + * PCLK / 2. + */ + if (csiclk_rate > csiclk_rate_limit) { + clk_set_rate(csi->csiclk, csiclk_rate >> 1); + csiclk_rate = clk_get_rate(csi->csiclk); + } else if ((csiclk_rate << 1) <= csiclk_rate_limit) { + clk_set_rate(csi->csiclk, csiclk_rate << 1); + csiclk_rate = clk_get_rate(csi->csiclk); + } + + spi_hz = spi_hz > CSI_MAX_SPI_SCKO ? CSI_MAX_SPI_SCKO : spi_hz; + + cks = DIV_ROUND_UP(csiclk_rate, spi_hz << 1); + if (cks > CSI_CKS_MAX) + cks = CSI_CKS_MAX; + + dev_dbg(csi->dev, "SPI clk rate is %ldHz\n", csiclk_rate / (cks << 1)); + + rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_CKS, cks); +} + +static void rzv2m_csi_setup_operating_mode(struct rzv2m_csi_priv *csi, + struct spi_transfer *t) +{ + if (t->rx_buf && !t->tx_buf) + /* Reception-only mode */ + rzv2m_csi_reg_write_bit(csi, CSI_MODE, CSI_MODE_TRMD, 0); + else + /* Send and receive mode */ + rzv2m_csi_reg_write_bit(csi, CSI_MODE, CSI_MODE_TRMD, 1); + + csi->bytes_per_word = t->bits_per_word / 8; + rzv2m_csi_reg_write_bit(csi, CSI_MODE, CSI_MODE_CCL, + csi->bytes_per_word == 2); +} + +static int rzv2m_csi_setup(struct spi_device *spi) +{ + struct rzv2m_csi_priv *csi = spi_controller_get_devdata(spi->controller); + int ret; + + rzv2m_csi_sw_reset(csi, 0); + + writel(CSI_MODE_SETUP, csi->base + CSI_MODE); + + /* Setup clock polarity and phase timing */ + rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_CKP, + !(spi->mode & SPI_CPOL)); + rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_DAP, + !(spi->mode & SPI_CPHA)); + + /* Setup serial data order */ + rzv2m_csi_reg_write_bit(csi, CSI_MODE, CSI_MODE_DIR, + !!(spi->mode & SPI_LSB_FIRST)); + + /* Set the operation mode as master */ + rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_SLAVE, 0); + + /* Give the IP a SW reset */ + ret = rzv2m_csi_sw_reset(csi, 1); + if (ret) + return ret; + rzv2m_csi_sw_reset(csi, 0); + + /* + * We need to enable the communication so that the clock will settle + * for the right polarity before enabling the CS. + */ + rzv2m_csi_start_stop_operation(csi, 1, false); + udelay(10); + rzv2m_csi_start_stop_operation(csi, 0, false); + + return 0; +} + +static int rzv2m_csi_pio_transfer(struct rzv2m_csi_priv *csi) +{ + bool tx_completed = csi->txbuf ? false : true; + bool rx_completed = csi->rxbuf ? false : true; + int ret = 0; + + /* Make sure the TX FIFO is empty */ + writel(0, csi->base + CSI_OFIFOL); + + csi->bytes_sent = 0; + csi->bytes_received = 0; + csi->errors = 0; + + rzv2m_csi_disable_all_irqs(csi); + rzv2m_csi_clear_all_irqs(csi); + rzv2m_csi_enable_rx_trigger(csi, true); + + while (!tx_completed || !rx_completed) { + /* + * Decide how many words we are going to transfer during + * this cycle (for both TX and RX), then set the RX FIFO trigger + * level accordingly. No need to set a trigger level for the + * TX FIFO, as this IP comes with an interrupt that fires when + * the TX FIFO is empty. + */ + rzv2m_csi_calc_current_transfer(csi); + rzv2m_csi_set_rx_fifo_trigger_level(csi); + + rzv2m_csi_enable_irqs(csi, CSI_INT_OVERF | CSI_INT_UNDER); + + /* Make sure the RX FIFO is empty */ + writel(0, csi->base + CSI_IFIFOL); + + writel(readl(csi->base + CSI_INT), csi->base + CSI_INT); + csi->status = 0; + + rzv2m_csi_start_stop_operation(csi, 1, false); + + /* TX */ + if (csi->txbuf) { + ret = rzv2m_csi_fill_txfifo(csi); + if (ret) + break; + + ret = rzv2m_csi_wait_for_tx_empty(csi); + if (ret) + break; + + if (csi->bytes_sent == csi->buffer_len) + tx_completed = true; + } + + /* + * Make sure the RX FIFO contains the desired number of words. + * We then either flush its content, or we copy it onto + * csi->rxbuf. + */ + ret = rzv2m_csi_wait_for_rx_ready(csi); + if (ret) + break; + + /* RX */ + if (csi->rxbuf) { + rzv2m_csi_start_stop_operation(csi, 0, false); + + ret = rzv2m_csi_read_rxfifo(csi); + if (ret) + break; + + if (csi->bytes_received == csi->buffer_len) + rx_completed = true; + } + + ret = rzv2m_csi_start_stop_operation(csi, 0, true); + if (ret) + goto pio_quit; + + if (csi->errors) { + ret = -EIO; + goto pio_quit; + } + } + + rzv2m_csi_start_stop_operation(csi, 0, true); + +pio_quit: + rzv2m_csi_disable_all_irqs(csi); + rzv2m_csi_enable_rx_trigger(csi, false); + rzv2m_csi_clear_all_irqs(csi); + + return ret; +} + +static int rzv2m_csi_transfer_one(struct spi_controller *controller, + struct spi_device *spi, + struct spi_transfer *transfer) +{ + struct rzv2m_csi_priv *csi = spi_controller_get_devdata(controller); + struct device *dev = csi->dev; + int ret; + + csi->txbuf = transfer->tx_buf; + csi->rxbuf = transfer->rx_buf; + csi->buffer_len = transfer->len; + + rzv2m_csi_setup_operating_mode(csi, transfer); + + rzv2m_csi_setup_clock(csi, transfer->speed_hz); + + ret = rzv2m_csi_pio_transfer(csi); + if (ret) { + if (csi->errors & UNDERRUN_ERROR) + dev_err(dev, "Underrun error\n"); + if (csi->errors & OVERFLOW_ERROR) + dev_err(dev, "Overflow error\n"); + if (csi->errors & TX_TIMEOUT_ERROR) + dev_err(dev, "TX timeout error\n"); + if (csi->errors & RX_TIMEOUT_ERROR) + dev_err(dev, "RX timeout error\n"); + } + + return ret; +} + +static int rzv2m_csi_probe(struct platform_device *pdev) +{ + struct spi_controller *controller; + struct device *dev = &pdev->dev; + struct rzv2m_csi_priv *csi; + struct reset_control *rstc; + int irq; + int ret; + + controller = devm_spi_alloc_master(dev, sizeof(*csi)); + if (!controller) + return -ENOMEM; + + csi = spi_controller_get_devdata(controller); + platform_set_drvdata(pdev, csi); + + csi->dev = dev; + csi->controller = controller; + + csi->base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(csi->base)) + return PTR_ERR(csi->base); + + irq = platform_get_irq(pdev, 0); + if (irq < 0) + return irq; + + csi->csiclk = devm_clk_get(dev, "csiclk"); + if (IS_ERR(csi->csiclk)) + return dev_err_probe(dev, PTR_ERR(csi->csiclk), + "could not get csiclk\n"); + + csi->pclk = devm_clk_get(dev, "pclk"); + if (IS_ERR(csi->pclk)) + return dev_err_probe(dev, PTR_ERR(csi->pclk), + "could not get pclk\n"); + + rstc = devm_reset_control_get_shared(dev, NULL); + if (IS_ERR(rstc)) + return dev_err_probe(dev, PTR_ERR(rstc), "Missing reset ctrl\n"); + + init_waitqueue_head(&csi->wait); + + controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST; + controller->dev.of_node = pdev->dev.of_node; + controller->bits_per_word_mask = SPI_BPW_MASK(16) | SPI_BPW_MASK(8); + controller->setup = rzv2m_csi_setup; + controller->transfer_one = rzv2m_csi_transfer_one; + controller->use_gpio_descriptors = true; + + ret = devm_request_irq(dev, irq, rzv2m_csi_irq_handler, 0, + dev_name(dev), csi); + if (ret) + return dev_err_probe(dev, ret, "cannot request IRQ\n"); + + /* + * The reset also affects other HW that is not under the control + * of Linux. Therefore, all we can do is make sure the reset is + * deasserted. + */ + reset_control_deassert(rstc); + + /* Make sure the IP is in SW reset state */ + ret = rzv2m_csi_sw_reset(csi, 1); + if (ret) + return ret; + + ret = clk_prepare_enable(csi->csiclk); + if (ret) + return dev_err_probe(dev, ret, "could not enable csiclk\n"); + + ret = spi_register_controller(controller); + if (ret) { + clk_disable_unprepare(csi->csiclk); + return dev_err_probe(dev, ret, "register controller failed\n"); + } + + return 0; +} + +static int rzv2m_csi_remove(struct platform_device *pdev) +{ + struct rzv2m_csi_priv *csi = platform_get_drvdata(pdev); + + spi_unregister_controller(csi->controller); + rzv2m_csi_sw_reset(csi, 1); + clk_disable_unprepare(csi->csiclk); + + return 0; +} + +static const struct of_device_id rzv2m_csi_match[] = { + { .compatible = "renesas,rzv2m-csi" }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, rzv2m_csi_match); + +static struct platform_driver rzv2m_csi_drv = { + .probe = rzv2m_csi_probe, + .remove = rzv2m_csi_remove, + .driver = { + .name = "rzv2m_csi", + .of_match_table = rzv2m_csi_match, + }, +}; +module_platform_driver(rzv2m_csi_drv); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Fabrizio Castro <castro.fabrizio.jz@renesas.com>"); +MODULE_DESCRIPTION("Clocked Serial Interface Driver");
The RZ/V2M SoC comes with the Clocked Serial Interface (CSI) IP, which is a master/slave SPI controller. This commit adds a driver to support CSI master mode. Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> --- v2: edited includes in drivers/spi/spi-rzv2m-csi.c drivers/spi/Kconfig | 6 + drivers/spi/Makefile | 1 + drivers/spi/spi-rzv2m-csi.c | 667 ++++++++++++++++++++++++++++++++++++ 3 files changed, 674 insertions(+) create mode 100644 drivers/spi/spi-rzv2m-csi.c