LinuxLists.cc - [PATCH 00/10] spi: rzv2m-csi: Code refactoring

[permalink] [raw]

Subject: [PATCH 01/10] spi: rzv2m-csi: Add missing include

Add missing include of bits.h file.

Signed-off-by: Fabrizio Castro <[email protected]>
---
drivers/spi/spi-rzv2m-csi.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c
index c0d9a776770f..b595f3b7294d 100644
--- a/drivers/spi/spi-rzv2m-csi.c
+++ b/drivers/spi/spi-rzv2m-csi.c
@@ -5,6 +5,7 @@
* Copyright (C) 2023 Renesas Electronics Corporation
*/

+#include <linux/bits.h>
#include <linux/clk.h>
#include <linux/count_zeros.h>
#include <linux/interrupt.h>
--
2.34.1

2023-07-15 01:05:47

[permalink] [raw]

Subject: [PATCH 03/10] spi: rzv2m-csi: Rework CSI_CKS_MAX definition

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). CSI_CKS_MAX is used for referring to the setting
corresponding to the maximum frequency divider.
Value 0x3FFF for CSI_CKS_MAX doesn't really means much to the reader
without an explanation and a more readable definition.

Add a comment with a meaningful description and also replace value
0x3FFF with the corresponding GENMASK, to make it very clear what the
macro means.

Signed-off-by: Fabrizio Castro <[email protected]>
---
drivers/spi/spi-rzv2m-csi.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c
index 3931045a85eb..621774949bde 100644
--- a/drivers/spi/spi-rzv2m-csi.c
+++ b/drivers/spi/spi-rzv2m-csi.c
@@ -65,7 +65,12 @@
#define CSI_FIFO_SIZE_BYTES 32
#define CSI_FIFO_HALF_SIZE 16
#define CSI_EN_DIS_TIMEOUT_US 100
-#define CSI_CKS_MAX 0x3FFF
+/*
+ * 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 GENMASK(13, 0)

#define UNDERRUN_ERROR BIT(0)
#define OVERFLOW_ERROR BIT(1)
--
2.34.1

2023-07-15 01:29:12

[permalink] [raw]

Subject: [PATCH 07/10] spi: rzv2m-csi: Switch to using {read,write}s{b,w}

The RX/TX FIFOs implemented by the CSI IP are accessed by
repeatedly reading/writing the same memory address, and
therefore they are the ideal candidate for {read,write}s{b,w}.
The RZ/V2M CSI driver currently implements loops to fill up
the TX FIFO and empty the RX FIFO, differentiating between
8-bit and 16-bit word size.
Switch to using {read,write}s{b,w} to get rid of the bespoke
loops.

Signed-off-by: Fabrizio Castro <[email protected]>
---
drivers/spi/spi-rzv2m-csi.c | 42 +++++++++++++------------------------
1 file changed, 14 insertions(+), 28 deletions(-)

diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c
index faf5898bc3e0..d0d6b183ffaf 100644
--- a/drivers/spi/spi-rzv2m-csi.c
+++ b/drivers/spi/spi-rzv2m-csi.c
@@ -86,8 +86,8 @@ struct rzv2m_csi_priv {
struct clk *pclk;
struct device *dev;
struct spi_controller *controller;
- const u8 *txbuf;
- u8 *rxbuf;
+ const void *txbuf;
+ void *rxbuf;
int buffer_len;
int bytes_sent;
int bytes_received;
@@ -157,22 +157,15 @@ static int rzv2m_csi_start_stop_operation(const struct rzv2m_csi_priv *csi,

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);
- }
+ if (csi->bytes_per_word == 2)
+ writesw(csi->base + CSI_OFIFO, csi->txbuf,
+ csi->words_to_transfer);
+ else
+ writesb(csi->base + CSI_OFIFO, csi->txbuf,
+ csi->words_to_transfer);

csi->txbuf += csi->bytes_to_transfer;
csi->bytes_sent += csi->bytes_to_transfer;
@@ -182,22 +175,15 @@ static int rzv2m_csi_fill_txfifo(struct rzv2m_csi_priv *csi)

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);
- }
+ if (csi->bytes_per_word == 2)
+ readsw(csi->base + CSI_IFIFO, csi->rxbuf,
+ csi->words_to_transfer);
+ else
+ readsb(csi->base + CSI_IFIFO, csi->rxbuf,
+ csi->words_to_transfer);

csi->rxbuf += csi->bytes_to_transfer;
csi->bytes_received += csi->bytes_to_transfer;
--
2.34.1

2023-07-15 01:31:13

[permalink] [raw]

Subject: [PATCH 08/10] spi: rzv2m-csi: Improve data types and alignment

"unsigned int" is more appropriate than "int" for the members
of "struct rzv2m_csi_priv".
Also, members "bytes_per_word" and "errors" introduce gaps
in the structure.
Adjust "struct rzv2m_csi_priv" and its members usage accordingly.
While at it, remove the unnecessary casting of "data" to
"struct rzv2m_csi_priv*" in function "rzv2m_csi_irq_handler".

Signed-off-by: Fabrizio Castro <[email protected]>
---
drivers/spi/spi-rzv2m-csi.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c
index d0d6b183ffaf..1e5ed1089f42 100644
--- a/drivers/spi/spi-rzv2m-csi.c
+++ b/drivers/spi/spi-rzv2m-csi.c
@@ -63,8 +63,8 @@
/* CSI_FIFOTRG */
#define CSI_FIFOTRG_R_TRG GENMASK(2, 0)

-#define CSI_FIFO_SIZE_BYTES 32
-#define CSI_FIFO_HALF_SIZE 16
+#define CSI_FIFO_SIZE_BYTES 32U
+#define CSI_FIFO_HALF_SIZE 16U
#define CSI_EN_DIS_TIMEOUT_US 100
/*
* Clock "csiclk" gets divided by 2 * CSI_CLKSEL_CKS in order to generate the
@@ -88,14 +88,14 @@ struct rzv2m_csi_priv {
struct spi_controller *controller;
const void *txbuf;
void *rxbuf;
- int buffer_len;
- int bytes_sent;
- int bytes_received;
- int bytes_to_transfer;
- int words_to_transfer;
- unsigned char bytes_per_word;
+ unsigned int buffer_len;
+ unsigned int bytes_sent;
+ unsigned int bytes_received;
+ unsigned int bytes_to_transfer;
+ unsigned int words_to_transfer;
+ unsigned int bytes_per_word;
wait_queue_head_t wait;
- u8 errors;
+ u32 errors;
u32 status;
};

@@ -193,9 +193,9 @@ static int rzv2m_csi_read_rxfifo(struct rzv2m_csi_priv *csi)

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 int bytes_transferred = max(csi->bytes_received, csi->bytes_sent);
+ unsigned int bytes_remaining = csi->buffer_len - bytes_transferred;
+ unsigned int to_transfer;

if (csi->txbuf)
/*
@@ -203,9 +203,9 @@ static inline void rzv2m_csi_calc_current_transfer(struct rzv2m_csi_priv *csi)
* 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);
+ to_transfer = min(CSI_FIFO_HALF_SIZE, bytes_remaining);
else
- to_transfer = min_t(int, CSI_FIFO_SIZE_BYTES, bytes_remaining);
+ to_transfer = min(CSI_FIFO_SIZE_BYTES, bytes_remaining);

if (csi->bytes_per_word == 2)
to_transfer >>= 1;
@@ -325,7 +325,7 @@ static inline int rzv2m_csi_wait_for_rx_ready(struct rzv2m_csi_priv *csi)

static irqreturn_t rzv2m_csi_irq_handler(int irq, void *data)
{
- struct rzv2m_csi_priv *csi = (struct rzv2m_csi_priv *)data;
+ struct rzv2m_csi_priv *csi = data;

csi->status = readl(csi->base + CSI_INT);
rzv2m_csi_disable_irqs(csi, csi->status);
--
2.34.1

2023-07-15 01:38:56

[permalink] [raw]

Subject: [PATCH 05/10] spi: rzv2m-csi: Replace unnecessary ternary operators

The ternary operators used to initialize tx_completed and rx_completed
are not necessary, replace them with a better implementation.

Signed-off-by: Fabrizio Castro <[email protected]>
---
drivers/spi/spi-rzv2m-csi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c
index 1c65cd5f2039..038f1486b7d7 100644
--- a/drivers/spi/spi-rzv2m-csi.c
+++ b/drivers/spi/spi-rzv2m-csi.c
@@ -439,8 +439,8 @@ static int rzv2m_csi_setup(struct spi_device *spi)

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;
+ bool tx_completed = !csi->txbuf;
+ bool rx_completed = !csi->rxbuf;
int ret = 0;

/* Make sure the TX FIFO is empty */
--
2.34.1

2023-07-15 01:38:56

[permalink] [raw]

Subject: [PATCH 10/10] spi: rzv2m-csi: Make use of device_set_node

Use device_set_node instead of assigning controller->dev.of_node
directly.

Signed-off-by: Fabrizio Castro <[email protected]>
---
drivers/spi/spi-rzv2m-csi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c
index 1874ca1c2747..ad7ca514eb09 100644
--- a/drivers/spi/spi-rzv2m-csi.c
+++ b/drivers/spi/spi-rzv2m-csi.c
@@ -575,12 +575,13 @@ static int rzv2m_csi_probe(struct platform_device *pdev)
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;

+ device_set_node(&controller->dev, dev_fwnode(dev));
+
ret = devm_request_irq(dev, irq, rzv2m_csi_irq_handler, 0,
dev_name(dev), csi);
if (ret)
--
2.34.1

2023-07-15 01:40:12

[permalink] [raw]

Subject: [PATCH 04/10] spi: rzv2m-csi: Leave readl_poll_timeout calls for last

Both rzv2m_csi_sw_reset and rzv2m_csi_start_stop_operation
call into readl_poll_timeout upon a certain condition, and
return 0 otherwise.
Flip the logic to improve readability.

Signed-off-by: Fabrizio Castro <[email protected]>
---
drivers/spi/spi-rzv2m-csi.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c
index 621774949bde..1c65cd5f2039 100644
--- a/drivers/spi/spi-rzv2m-csi.c
+++ b/drivers/spi/spi-rzv2m-csi.c
@@ -131,13 +131,12 @@ static int rzv2m_csi_sw_reset(struct rzv2m_csi_priv *csi, int assert)

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);
- }
+ if (!assert)
+ return 0;

- return 0;
+ return readl_poll_timeout(csi->base + CSI_MODE, reg,
+ !(reg & CSI_MODE_CSOT), 0,
+ CSI_EN_DIS_TIMEOUT_US);
}

static int rzv2m_csi_start_stop_operation(const struct rzv2m_csi_priv *csi,
@@ -147,12 +146,12 @@ static int rzv2m_csi_start_stop_operation(const struct rzv2m_csi_priv *csi,

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);
+ if (enable || !wait)
+ return 0;

- return 0;
+ return readl_poll_timeout(csi->base + CSI_MODE, reg,
+ !(reg & CSI_MODE_CSOT), 0,
+ CSI_EN_DIS_TIMEOUT_US);
}

static int rzv2m_csi_fill_txfifo(struct rzv2m_csi_priv *csi)
--
2.34.1

2023-07-15 01:41:01

[permalink] [raw]

Subject: [PATCH 02/10] spi: rzv2m-csi: Adopt HZ_PER_MHZ for max spi clock

Make use of HZ_PER_MHZ for CSI_MAX_SPI_SCKO to make it clear
what its value means.

Signed-off-by: Fabrizio Castro <[email protected]>
---
drivers/spi/spi-rzv2m-csi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c
index b595f3b7294d..3931045a85eb 100644
--- a/drivers/spi/spi-rzv2m-csi.c
+++ b/drivers/spi/spi-rzv2m-csi.c
@@ -13,6 +13,7 @@
#include <linux/platform_device.h>
#include <linux/reset.h>
#include <linux/spi/spi.h>
+#include <linux/units.h>

/* Registers */
#define CSI_MODE 0x00 /* CSI mode control */
@@ -71,7 +72,7 @@
#define TX_TIMEOUT_ERROR BIT(2)
#define RX_TIMEOUT_ERROR BIT(3)

-#define CSI_MAX_SPI_SCKO 8000000
+#define CSI_MAX_SPI_SCKO (8 * HZ_PER_MHZ)

struct rzv2m_csi_priv {
void __iomem *base;
--
2.34.1

2023-07-15 02:19:20

[permalink] [raw]

Subject: [PATCH 09/10] spi: rzv2m-csi: Get rid of the x_trg{_words} tables

Table x_trg can be replaced with ilog2(), and table x_trg_words
can be replaced with rounddown_pow_of_two().
Replace the tables usage with the corresponding macros.
While at it, remove a couple of unnecessary empty lines.

Signed-off-by: Fabrizio Castro <[email protected]>
---
drivers/spi/spi-rzv2m-csi.c | 21 +++------------------
1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c
index 1e5ed1089f42..1874ca1c2747 100644
--- a/drivers/spi/spi-rzv2m-csi.c
+++ b/drivers/spi/spi-rzv2m-csi.c
@@ -10,6 +10,7 @@
#include <linux/count_zeros.h>
#include <linux/interrupt.h>
#include <linux/iopoll.h>
+#include <linux/log2.h>
#include <linux/platform_device.h>
#include <linux/reset.h>
#include <linux/spi/spi.h>
@@ -99,20 +100,6 @@ struct rzv2m_csi_priv {
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)
{
@@ -216,7 +203,7 @@ static inline void rzv2m_csi_calc_current_transfer(struct rzv2m_csi_priv *csi)
* 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];
+ csi->words_to_transfer = rounddown_pow_of_two(to_transfer);

if (csi->bytes_per_word == 2)
csi->bytes_to_transfer = csi->words_to_transfer << 1;
@@ -227,7 +214,7 @@ static inline void rzv2m_csi_calc_current_transfer(struct rzv2m_csi_priv *csi)
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]);
+ ilog2(csi->words_to_transfer));
}

static inline void rzv2m_csi_enable_rx_trigger(struct rzv2m_csi_priv *csi,
@@ -300,7 +287,6 @@ static int rzv2m_csi_wait_for_tx_empty(struct rzv2m_csi_priv *csi)
return 0;

ret = rzv2m_csi_wait_for_interrupt(csi, CSI_INT_TREND, CSI_CNT_TREND_E);
-
if (ret == -ETIMEDOUT)
csi->errors |= TX_TIMEOUT_ERROR;

@@ -316,7 +302,6 @@ static inline int rzv2m_csi_wait_for_rx_ready(struct rzv2m_csi_priv *csi)

ret = rzv2m_csi_wait_for_interrupt(csi, CSI_INT_R_TRGR,
CSI_CNT_R_TRGR_E);
-
if (ret == -ETIMEDOUT)
csi->errors |= RX_TIMEOUT_ERROR;

--
2.34.1

2023-07-15 02:46:44

[permalink] [raw]

Subject: [PATCH 06/10] spi: rzv2m-csi: Squash timing settings into one statement

Register CLKSEL hosts the configuration for both clock polarity
and data phase, and both values can be set in one write operation.

Squash the clock polarity and data phase register writes into
one statement, for efficiency.

Signed-off-by: Fabrizio Castro <[email protected]>
---
drivers/spi/spi-rzv2m-csi.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-rzv2m-csi.c b/drivers/spi/spi-rzv2m-csi.c
index 038f1486b7d7..faf5898bc3e0 100644
--- a/drivers/spi/spi-rzv2m-csi.c
+++ b/drivers/spi/spi-rzv2m-csi.c
@@ -38,6 +38,7 @@
/* CSI_CLKSEL */
#define CSI_CLKSEL_CKP BIT(17)
#define CSI_CLKSEL_DAP BIT(16)
+#define CSI_CLKSEL_MODE (CSI_CLKSEL_CKP|CSI_CLKSEL_DAP)
#define CSI_CLKSEL_SLAVE BIT(15)
#define CSI_CLKSEL_CKS GENMASK(14, 1)

@@ -408,10 +409,8 @@ static int rzv2m_csi_setup(struct spi_device *spi)
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));
+ rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_MODE,
+ ~spi->mode & SPI_MODE_X_MASK);

/* Setup serial data order */
rzv2m_csi_reg_write_bit(csi, CSI_MODE, CSI_MODE_DIR,
--
2.34.1

2023-07-15 08:28:39

[permalink] [raw]

Subject: Re: [PATCH 09/10] spi: rzv2m-csi: Get rid of the x_trg{_words} tables

On Sat, Jul 15, 2023 at 4:04 AM Fabrizio Castro
<[email protected]> wrote:
>
> Table x_trg can be replaced with ilog2(), and table x_trg_words
> can be replaced with rounddown_pow_of_two().
> Replace the tables usage with the corresponding macros.
> While at it, remove a couple of unnecessary empty lines.

Suggested-by: ?

--
With Best Regards,
Andy Shevchenko

2023-07-15 08:41:21

[permalink] [raw]

Subject: Re: [PATCH 10/10] spi: rzv2m-csi: Make use of device_set_node

On Sat, Jul 15, 2023 at 4:04 AM Fabrizio Castro
<[email protected]> wrote:
>
> Use device_set_node instead of assigning controller->dev.of_node
> directly.

"...because it also sets the firmware node."

You probably need to add property.h, if not added yet.

--
With Best Regards,
Andy Shevchenko

2023-07-15 09:11:22

[permalink] [raw]

Subject: Re: [PATCH 00/10] spi: rzv2m-csi: Code refactoring

On Sat, Jul 15, 2023 at 4:04 AM Fabrizio Castro
<[email protected]> wrote:
>
> Dear All,
>
> this series is to follow up on Geert and Andy feedback:
> https://patchwork.kernel.org/project/linux-renesas-soc/patch/[email protected]/

Thank you!

Some comments I sent against individual patches (Suggested-by can be
applied to many, btw), if you address them as suggested, feel free to
add
Reviewed-by: Andy Shevchenko <[email protected]>
to the entire series.

--
With Best Regards,
Andy Shevchenko

2023-07-15 09:15:24

[permalink] [raw]

Subject: Re: [PATCH 06/10] spi: rzv2m-csi: Squash timing settings into one statement

On Sat, Jul 15, 2023 at 4:04 AM Fabrizio Castro
<[email protected]> wrote:
>
> Register CLKSEL hosts the configuration for both clock polarity
> and data phase, and both values can be set in one write operation.
>
> Squash the clock polarity and data phase register writes into
> one statement, for efficiency.

...

> /* 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));
> + rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_MODE,
> + ~spi->mode & SPI_MODE_X_MASK);

I think this now regresses due to the absence of parentheses.

--
With Best Regards,
Andy Shevchenko

2023-07-17 09:40:45

[permalink] [raw]

Subject: Re: [PATCH 04/10] spi: rzv2m-csi: Leave readl_poll_timeout calls for last

On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro
<[email protected]> wrote:
> Both rzv2m_csi_sw_reset and rzv2m_csi_start_stop_operation
> call into readl_poll_timeout upon a certain condition, and
> return 0 otherwise.
> Flip the logic to improve readability.
>
> Signed-off-by: Fabrizio Castro <[email protected]>

Reviewed-by: Geert Uytterhoeven <[email protected]>

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2023-07-17 09:48:15

[permalink] [raw]

Subject: Re: [PATCH 02/10] spi: rzv2m-csi: Adopt HZ_PER_MHZ for max spi clock

On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro
<[email protected]> wrote:
> Make use of HZ_PER_MHZ for CSI_MAX_SPI_SCKO to make it clear
> what its value means.
>
> Signed-off-by: Fabrizio Castro <[email protected]>

Reviewed-by: Geert Uytterhoeven <[email protected]>

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2023-07-17 10:01:43

[permalink] [raw]

Subject: Re: [PATCH 05/10] spi: rzv2m-csi: Replace unnecessary ternary operators

On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro
<[email protected]> wrote:
> The ternary operators used to initialize tx_completed and rx_completed
> are not necessary, replace them with a better implementation.
>
> Signed-off-by: Fabrizio Castro <[email protected]>

Reviewed-by: Geert Uytterhoeven <[email protected]>

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2023-07-17 10:09:34

[permalink] [raw]

Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using {read,write}s{b,w}

Hi Fabrizio,

On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro
<[email protected]> wrote:
> The RX/TX FIFOs implemented by the CSI IP are accessed by
> repeatedly reading/writing the same memory address, and
> therefore they are the ideal candidate for {read,write}s{b,w}.
> The RZ/V2M CSI driver currently implements loops to fill up
> the TX FIFO and empty the RX FIFO, differentiating between
> 8-bit and 16-bit word size.
> Switch to using {read,write}s{b,w} to get rid of the bespoke
> loops.
>
> Signed-off-by: Fabrizio Castro <[email protected]>

Thanks for your patch!

> --- a/drivers/spi/spi-rzv2m-csi.c
> +++ b/drivers/spi/spi-rzv2m-csi.c

> @@ -157,22 +157,15 @@ static int rzv2m_csi_start_stop_operation(const struct rzv2m_csi_priv *csi,
>
> 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);
> - }
> + if (csi->bytes_per_word == 2)
> + writesw(csi->base + CSI_OFIFO, csi->txbuf,
> + csi->words_to_transfer);
> + else
> + writesb(csi->base + CSI_OFIFO, csi->txbuf,
> + csi->words_to_transfer);

According to the hardware documentation[1], the access size for both the
CSI_OFIFO and CSI_IFIFO registers is 32 bits, so you must use writel()
resp. readl(). So please check with the hardware people first.

[1] RZ/V2M User's Manual Hardware, Rev. 1.30.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2023-07-17 10:09:50

[permalink] [raw]

Subject: Re: [PATCH 03/10] spi: rzv2m-csi: Rework CSI_CKS_MAX definition

On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro
<[email protected]> wrote:
> 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). CSI_CKS_MAX is used for referring to the setting
> corresponding to the maximum frequency divider.
> Value 0x3FFF for CSI_CKS_MAX doesn't really means much to the reader
> without an explanation and a more readable definition.
>
> Add a comment with a meaningful description and also replace value
> 0x3FFF with the corresponding GENMASK, to make it very clear what the
> macro means.
>
> Signed-off-by: Fabrizio Castro <[email protected]>

Reviewed-by: Geert Uytterhoeven <[email protected]>

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2023-07-17 10:32:22

[permalink] [raw]

Subject: Re: [PATCH 09/10] spi: rzv2m-csi: Get rid of the x_trg{_words} tables

On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro
<[email protected]> wrote:
> Table x_trg can be replaced with ilog2(), and table x_trg_words
> can be replaced with rounddown_pow_of_two().
> Replace the tables usage with the corresponding macros.
> While at it, remove a couple of unnecessary empty lines.
>
> Signed-off-by: Fabrizio Castro <[email protected]>

Reviewed-by: Geert Uytterhoeven <[email protected]>

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2023-07-17 10:35:26

[permalink] [raw]

Subject: Re: [PATCH 08/10] spi: rzv2m-csi: Improve data types and alignment

Hi Fabrizio,

Thanks for your patch!

On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro
<[email protected]> wrote:
> "unsigned int" is more appropriate than "int" for the members
> of "struct rzv2m_csi_priv".

Agreed.

> Also, members "bytes_per_word" and "errors" introduce gaps
> in the structure.

While enlarging the types does get rid of the gaps, that was not the
intent of my comment ;-)
You can reorder fields to avoid gaps, and reduce the size of the structure.

> Adjust "struct rzv2m_csi_priv" and its members usage accordingly.
> While at it, remove the unnecessary casting of "data" to
> "struct rzv2m_csi_priv*" in function "rzv2m_csi_irq_handler".
>
> Signed-off-by: Fabrizio Castro <[email protected]>

> --- a/drivers/spi/spi-rzv2m-csi.c
> +++ b/drivers/spi/spi-rzv2m-csi.c

> @@ -88,14 +88,14 @@ struct rzv2m_csi_priv {
> struct spi_controller *controller;
> const void *txbuf;
> void *rxbuf;
> - int buffer_len;
> - int bytes_sent;
> - int bytes_received;
> - int bytes_to_transfer;
> - int words_to_transfer;
> - unsigned char bytes_per_word;
> + unsigned int buffer_len;
> + unsigned int bytes_sent;
> + unsigned int bytes_received;
> + unsigned int bytes_to_transfer;
> + unsigned int words_to_transfer;
> + unsigned int bytes_per_word;

bytes_per_word is calculated from spi_transfer.bits_per_word,
so u8 was fine.

> wait_queue_head_t wait;
> - u8 errors;
> + u32 errors;

u8 was sufficiently large to hold all possible values.

> u32 status;
> };
>

Anyway, the code should work fine, so
Reviewed-by: Geert Uytterhoeven <[email protected]>

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2023-07-17 11:05:32

[permalink] [raw]

Subject: RE: [PATCH 07/10] spi: rzv2m-csi: Switch to using {read,write}s{b,w}

Hi Geert,

Thanks for your reply!

> From: Geert Uytterhoeven <[email protected]>
> Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using
> {read,write}s{b,w}
>
> Hi Fabrizio,
>
> On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro
> <[email protected]> wrote:
> > The RX/TX FIFOs implemented by the CSI IP are accessed by
> > repeatedly reading/writing the same memory address, and
> > therefore they are the ideal candidate for {read,write}s{b,w}.
> > The RZ/V2M CSI driver currently implements loops to fill up
> > the TX FIFO and empty the RX FIFO, differentiating between
> > 8-bit and 16-bit word size.
> > Switch to using {read,write}s{b,w} to get rid of the bespoke
> > loops.
> >
> > Signed-off-by: Fabrizio Castro <[email protected]>
>
> Thanks for your patch!
>
> > --- a/drivers/spi/spi-rzv2m-csi.c
> > +++ b/drivers/spi/spi-rzv2m-csi.c
>
> > @@ -157,22 +157,15 @@ static int
> rzv2m_csi_start_stop_operation(const struct rzv2m_csi_priv *csi,
> >
> > 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);
> > - }
> > + if (csi->bytes_per_word == 2)
> > + writesw(csi->base + CSI_OFIFO, csi->txbuf,
> > + csi->words_to_transfer);
> > + else
> > + writesb(csi->base + CSI_OFIFO, csi->txbuf,
> > + csi->words_to_transfer);
>
> According to the hardware documentation[1], the access size for both
> the
> CSI_OFIFO and CSI_IFIFO registers is 32 bits, so you must use writel()
> resp. readl(). So please check with the hardware people first.
>
> [1] RZ/V2M User's Manual Hardware, Rev. 1.30.

You are right, access is 32 bits (and although this patch works fine,
we should avoid accessing those regs any other way). Now I remember
why I decided to go for the bespoke loops in the first place, writesl
and readsl provide the right register access, but the wrong pointer
arithmetic for this use case.
For this patch I ended up selecting writesw/writesb/readsw/readsb to
get the right pointer arithmetic, but the register access is not as
per manual.

I can either fallback to using the bespoke loops (I can still
remove the unnecessary u8* and u16* casting ;-) ), or I can add
new APIs for this sort of access to io.h (e.g. writesbl, writeswl,
readsbl, readswl, in order to get the pointer arithmetic right for
the type of array handled, while keeping memory access as expected).

What are your thoughts on that?

Thanks,
Fab

>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> [email protected]
>
> 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

2023-07-17 11:09:01

[permalink] [raw]

Subject: RE: [PATCH 06/10] spi: rzv2m-csi: Squash timing settings into one statement

Hi Andy,

Thanks for your reply.

> From: Andy Shevchenko <[email protected]>
> Subject: Re: [PATCH 06/10] spi: rzv2m-csi: Squash timing settings into
> one statement
>
> On Sat, Jul 15, 2023 at 4:04 AM Fabrizio Castro
> <[email protected]> wrote:
> >
> > Register CLKSEL hosts the configuration for both clock polarity
> > and data phase, and both values can be set in one write operation.
> >
> > Squash the clock polarity and data phase register writes into
> > one statement, for efficiency.
>
> ...
>
> > /* 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));
> > + rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_MODE,
> > + ~spi->mode & SPI_MODE_X_MASK);
>
> I think this now regresses due to the absence of parentheses.

This looks okay to me. CSI_CLKSEL_CKP needs to contain the inverted value
of SPI_CPOL, and CSI_CLKSEL_DAP needs to contain the inverted value of
SPI_CPHA, and that happens with both use cases.

Thanks,
Fab

>
> --
> With Best Regards,
> Andy Shevchenko

2023-07-17 11:36:57

[permalink] [raw]

Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using {read,write}s{b,w}

On Mon, Jul 17, 2023 at 1:36 PM Fabrizio Castro
<[email protected]> wrote:
> > From: Geert Uytterhoeven <[email protected]>
> > Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using
> > {read,write}s{b,w}
> > On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro
> > <[email protected]> wrote:

...

> > According to the hardware documentation[1], the access size for both
> > the
> > CSI_OFIFO and CSI_IFIFO registers is 32 bits, so you must use writel()
> > resp. readl(). So please check with the hardware people first.
> >
> > [1] RZ/V2M User's Manual Hardware, Rev. 1.30.
>
> You are right, access is 32 bits (and although this patch works fine,
> we should avoid accessing those regs any other way). Now I remember
> why I decided to go for the bespoke loops in the first place, writesl
> and readsl provide the right register access, but the wrong pointer
> arithmetic for this use case.
> For this patch I ended up selecting writesw/writesb/readsw/readsb to
> get the right pointer arithmetic, but the register access is not as
> per manual.
>
> I can either fallback to using the bespoke loops (I can still
> remove the unnecessary u8* and u16* casting ;-) ), or I can add
> new APIs for this sort of access to io.h (e.g. writesbl, writeswl,
> readsbl, readswl, in order to get the pointer arithmetic right for
> the type of array handled, while keeping memory access as expected).
>
> What are your thoughts on that?

I think that you need to use readsl() / writesl() on the custom buffer
with something like

*_sparse() / *_condence() APIs added (perhaps locally to this driver)
as they may be reused by others in the future.
Having all flavours of read*()/write*() does not scale in my opinion.

--
With Best Regards,
Andy Shevchenko

2023-07-17 11:38:49

[permalink] [raw]

Subject: Re: [PATCH 06/10] spi: rzv2m-csi: Squash timing settings into one statement

On Mon, Jul 17, 2023 at 1:44 PM Fabrizio Castro
<[email protected]> wrote:
> > From: Andy Shevchenko <[email protected]>
> > On Sat, Jul 15, 2023 at 4:04 AM Fabrizio Castro
> > <[email protected]> wrote:

...

> > > /* 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));
> > > + rzv2m_csi_reg_write_bit(csi, CSI_CLKSEL, CSI_CLKSEL_MODE,
> > > + ~spi->mode & SPI_MODE_X_MASK);
> >
> > I think this now regresses due to the absence of parentheses.
>
> This looks okay to me. CSI_CLKSEL_CKP needs to contain the inverted value
> of SPI_CPOL, and CSI_CLKSEL_DAP needs to contain the inverted value of
> SPI_CPHA, and that happens with both use cases.

Ah, this is interchangeable since we will get the same bits in the end, indeed.

--
With Best Regards,
Andy Shevchenko

2023-07-17 13:15:46

[permalink] [raw]

Subject: RE: [PATCH 07/10] spi: rzv2m-csi: Switch to using {read,write}s{b,w}

Hi Andy,

Thanks for your reply.

> From: Andy Shevchenko <[email protected]>
> Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using
> {read,write}s{b,w}
>
> On Mon, Jul 17, 2023 at 1:36 PM Fabrizio Castro
> <[email protected]> wrote:
> > > From: Geert Uytterhoeven <[email protected]>
> > > Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using
> > > {read,write}s{b,w}
> > > On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro
> > > <[email protected]> wrote:
>
> ...
>
> > > According to the hardware documentation[1], the access size for
> both
> > > the
> > > CSI_OFIFO and CSI_IFIFO registers is 32 bits, so you must use
> writel()
> > > resp. readl(). So please check with the hardware people first.
> > >
> > > [1] RZ/V2M User's Manual Hardware, Rev. 1.30.
> >
> > You are right, access is 32 bits (and although this patch works
> fine,
> > we should avoid accessing those regs any other way). Now I remember
> > why I decided to go for the bespoke loops in the first place,
> writesl
> > and readsl provide the right register access, but the wrong pointer
> > arithmetic for this use case.
> > For this patch I ended up selecting writesw/writesb/readsw/readsb to
> > get the right pointer arithmetic, but the register access is not as
> > per manual.
> >
> > I can either fallback to using the bespoke loops (I can still
> > remove the unnecessary u8* and u16* casting ;-) ), or I can add
> > new APIs for this sort of access to io.h (e.g. writesbl, writeswl,
> > readsbl, readswl, in order to get the pointer arithmetic right for
> > the type of array handled, while keeping memory access as expected).
> >
> > What are your thoughts on that?
>
> I think that you need to use readsl() / writesl() on the custom buffer
> with something like
>
> *_sparse() / *_condence() APIs added (perhaps locally to this driver)
> as they may be reused by others in the future.
> Having all flavours of read*()/write*() does not scale in my opinion.

Maybe a "generic" macro then (one for reading and one for writing)?
So that we can pass the data type (to get the pointer arithmetic
right) and the function name to use for the read/write operations
(to get the register operations right)?
Maybe that would scale and cater for most needs (including the one
from this patch)?

Cheers,
Fab

>
> --
> With Best Regards,
> Andy Shevchenko

2023-07-17 15:24:03

[permalink] [raw]

Subject: RE: [PATCH 10/10] spi: rzv2m-csi: Make use of device_set_node

Hi Andy,

> From: Andy Shevchenko <[email protected]>
> Subject: Re: [PATCH 10/10] spi: rzv2m-csi: Make use of device_set_node
>
> On Sat, Jul 15, 2023 at 4:04 AM Fabrizio Castro
> <[email protected]> wrote:
> >
> > Use device_set_node instead of assigning controller->dev.of_node
> > directly.
>
> "...because it also sets the firmware node."
>
> You probably need to add property.h, if not added yet.

You are right, I'll send a v2 for this.
I'll also add the related Suggested-by and Reviewed-by tags in v2.

Cheers,
Fab

>
> --
> With Best Regards,
> Andy Shevchenko

2023-07-17 16:52:23