[PATCH v0] spi: spi-rockchip spi slave mode

[PATCH v0] spi: spi-rockchip spi slave mode

[Chris Ruehl]

The driver spi-rockchip does not support spi slave mode, but the register map
has an entry indicate that the chip support it. An example implementation found
here: https://dev.t-firefly.com/thread-101485-1-1.html
The patch is my first approach to support slave mode which is needed
in one of our projects, the PCBA is not yet available but we think
to have it for testing very soon. Yes, the code in the patch
isn't tested yet.

I found it odd, that the num_chipselect is set fixed to the amount of 
native chip-select lines rather use the max_native_cs.
Changed it.
-   master->num_chipselect = ROCKCHIP_SPI_MAX_CS_NUM;
+       of_property_read_u32(np, "num-cs", &num_cs);
+       master->num_chipselect = num_cs;
+       master->max_native_cs = ROCKCHIP_SPI_MAX_CS_NUM;

That ask to enable cs_gpiods, and support gpio cs
+       master->use_gpio_descriptors = true;

Patch against next-20200505

Thanks for review!

Happy hacking
Chris

Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
---

[PATCH v0] spi: spi-rockchip spi slave mode

[Chris Ruehl]

The driver spi-rockchip does not support spi slave mode, but the register map
has an entry indicate that the chip support it. An example implementation found
here: https://dev.t-firefly.com/thread-101485-1-1.html
The patch is my first approach to support slave mode which is needed
in one of our projects, the PCBA is not yet available but we think
to have it for testing very soon. Yes, the code in the patch
isn't tested yet.

I found it odd, that the num_chipselect is set fixed to the amount of 
native chip-select lines rather use the max_native_cs.
Changed it.
-   master->num_chipselect = ROCKCHIP_SPI_MAX_CS_NUM;
+       of_property_read_u32(np, "num-cs", &num_cs);
+       master->num_chipselect = num_cs;
+       master->max_native_cs = ROCKCHIP_SPI_MAX_CS_NUM;

That ask to enable cs_gpiods, and support gpio cs
+       master->use_gpio_descriptors = true;

Patch against next-20200505

Thanks for review!

Happy hacking
Chris

Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
---


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v0 1/1] spi: spi-rockchip: add support for spi slave_mode

[Chris Ruehl]

This patch aim to add spi slave mode support to the rockchip driver.
Fix the wrong usage of num_cs set fix to ROCKCHIP_SPI_MAX_CS_NUM,
instead use max_native_cs flag to set the limit of native chip-select.
Enable use_gpio_descriptors to have cs_gpiod for gpio based chip-selects.

Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
---
 drivers/spi/spi-rockchip.c | 46 +++++++++++++++++++++++++++++++++-----
 1 file changed, 41 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index 70ef63e0b6b8..9c1ff52c0f85 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -183,6 +183,9 @@ struct rockchip_spi {
 	u8 rsd;
 
 	bool cs_asserted[ROCKCHIP_SPI_MAX_CS_NUM];
+
+	bool slave_mode;
+	bool slave_abort;
 };
 
 static inline void spi_enable_chip(struct rockchip_spi *rs, bool enable)
@@ -359,7 +362,7 @@ static void rockchip_spi_dma_rxcb(void *data)
 	struct rockchip_spi *rs = spi_master_get_devdata(master);
 	int state = atomic_fetch_andnot(RXDMA, &rs->state);
 
-	if (state & TXDMA)
+	if (state & TXDMA && !rs->slave_abort)
 		return;
 
 	spi_enable_chip(rs, false);
@@ -372,7 +375,7 @@ static void rockchip_spi_dma_txcb(void *data)
 	struct rockchip_spi *rs = spi_master_get_devdata(master);
 	int state = atomic_fetch_andnot(TXDMA, &rs->state);
 
-	if (state & RXDMA)
+	if (state & RXDMA && !rs->slave_abort)
 		return;
 
 	/* Wait until the FIFO data completely. */
@@ -466,6 +469,10 @@ static void rockchip_spi_config(struct rockchip_spi *rs,
 	u32 cr1;
 	u32 dmacr = 0;
 
+	if (rs->slavemode)
+		cr0 |= CR0_OPM_SLAVE << CR0_OPM_OFFSET;
+	rs->slave_abort = false;
+
 	cr0 |= rs->rsd << CR0_RSD_OFFSET;
 	cr0 |= (spi->mode & 0x3U) << CR0_SCPH_OFFSET;
 	if (spi->mode & SPI_LSB_FIRST)
@@ -535,6 +542,16 @@ static size_t rockchip_spi_max_transfer_size(struct spi_device *spi)
 	return ROCKCHIP_SPI_MAX_TRANLEN;
 }
 
+static int rockchip_spi_slave_abort(struct spi_master *master)
+{
+	struct rockchip_spi *rs = spi_master_get_devdata(master);
+
+	rs->slave_abort = true;
+	complete(master);
+
+	return 0;
+}
+
 static int rockchip_spi_transfer_one(
 		struct spi_master *master,
 		struct spi_device *spi,
@@ -589,14 +606,25 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 	struct spi_master *master;
 	struct resource *mem;
 	u32 rsd_nsecs;
+	bool slave_mode;
+	u32 num_cs = 1;
+
+	slave_mode = of_property_read_bool(np, "spi-slave");
+
+	if (slave_mode)
+		master = spi_alloc_slave(&pdev->dev,
+				sizeof(struct rockchip_spi));
+	else
+		master = spi_alloc_master(&pdev->dev,
+				sizeof(struct rockchip_spi));
 
-	master = spi_alloc_master(&pdev->dev, sizeof(struct rockchip_spi));
 	if (!master)
 		return -ENOMEM;
 
 	platform_set_drvdata(pdev, master);
 
 	rs = spi_master_get_devdata(master);
+	rs->slave_mode = slave_mode;
 
 	/* Get basic io resource and map it */
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -676,7 +704,16 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 	master->auto_runtime_pm = true;
 	master->bus_num = pdev->id;
 	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP | SPI_LSB_FIRST;
-	master->num_chipselect = ROCKCHIP_SPI_MAX_CS_NUM;
+	if (slave_mode) {
+		master->mode_bits |= SPI_NO_CS;
+		master->slave_abort = rockchip_spi_slave_abort;
+	} else {
+		of_property_read_u32(np, "num-cs", &num_cs);
+		master->num_chipselect = num_cs;
+		master->use_gpio_descriptors = true;
+		master->max_native_cs = ROCKCHIP_SPI_MAX_CS_NUM;
+		master->flags = SPI_MASTER_GPIO_SS;
+	}
 	master->dev.of_node = pdev->dev.of_node;
 	master->bits_per_word_mask = SPI_BPW_MASK(16) | SPI_BPW_MASK(8) | SPI_BPW_MASK(4);
 	master->min_speed_hz = rs->freq / BAUDR_SCKDV_MAX;
@@ -686,7 +723,6 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 	master->transfer_one = rockchip_spi_transfer_one;
 	master->max_transfer_size = rockchip_spi_max_transfer_size;
 	master->handle_err = rockchip_spi_handle_err;
-	master->flags = SPI_MASTER_GPIO_SS;
 
 	master->dma_tx = dma_request_chan(rs->dev, "tx");
 	if (IS_ERR(master->dma_tx)) {
-- 
2.20.1

[PATCH v0 1/1] spi: spi-rockchip: add support for spi slave_mode

[Chris Ruehl]

This patch aim to add spi slave mode support to the rockchip driver.
Fix the wrong usage of num_cs set fix to ROCKCHIP_SPI_MAX_CS_NUM,
instead use max_native_cs flag to set the limit of native chip-select.
Enable use_gpio_descriptors to have cs_gpiod for gpio based chip-selects.

Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
---
 drivers/spi/spi-rockchip.c | 46 +++++++++++++++++++++++++++++++++-----
 1 file changed, 41 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index 70ef63e0b6b8..9c1ff52c0f85 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -183,6 +183,9 @@ struct rockchip_spi {
 	u8 rsd;
 
 	bool cs_asserted[ROCKCHIP_SPI_MAX_CS_NUM];
+
+	bool slave_mode;
+	bool slave_abort;
 };
 
 static inline void spi_enable_chip(struct rockchip_spi *rs, bool enable)
@@ -359,7 +362,7 @@ static void rockchip_spi_dma_rxcb(void *data)
 	struct rockchip_spi *rs = spi_master_get_devdata(master);
 	int state = atomic_fetch_andnot(RXDMA, &rs->state);
 
-	if (state & TXDMA)
+	if (state & TXDMA && !rs->slave_abort)
 		return;
 
 	spi_enable_chip(rs, false);
@@ -372,7 +375,7 @@ static void rockchip_spi_dma_txcb(void *data)
 	struct rockchip_spi *rs = spi_master_get_devdata(master);
 	int state = atomic_fetch_andnot(TXDMA, &rs->state);
 
-	if (state & RXDMA)
+	if (state & RXDMA && !rs->slave_abort)
 		return;
 
 	/* Wait until the FIFO data completely. */
@@ -466,6 +469,10 @@ static void rockchip_spi_config(struct rockchip_spi *rs,
 	u32 cr1;
 	u32 dmacr = 0;
 
+	if (rs->slavemode)
+		cr0 |= CR0_OPM_SLAVE << CR0_OPM_OFFSET;
+	rs->slave_abort = false;
+
 	cr0 |= rs->rsd << CR0_RSD_OFFSET;
 	cr0 |= (spi->mode & 0x3U) << CR0_SCPH_OFFSET;
 	if (spi->mode & SPI_LSB_FIRST)
@@ -535,6 +542,16 @@ static size_t rockchip_spi_max_transfer_size(struct spi_device *spi)
 	return ROCKCHIP_SPI_MAX_TRANLEN;
 }
 
+static int rockchip_spi_slave_abort(struct spi_master *master)
+{
+	struct rockchip_spi *rs = spi_master_get_devdata(master);
+
+	rs->slave_abort = true;
+	complete(master);
+
+	return 0;
+}
+
 static int rockchip_spi_transfer_one(
 		struct spi_master *master,
 		struct spi_device *spi,
@@ -589,14 +606,25 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 	struct spi_master *master;
 	struct resource *mem;
 	u32 rsd_nsecs;
+	bool slave_mode;
+	u32 num_cs = 1;
+
+	slave_mode = of_property_read_bool(np, "spi-slave");
+
+	if (slave_mode)
+		master = spi_alloc_slave(&pdev->dev,
+				sizeof(struct rockchip_spi));
+	else
+		master = spi_alloc_master(&pdev->dev,
+				sizeof(struct rockchip_spi));
 
-	master = spi_alloc_master(&pdev->dev, sizeof(struct rockchip_spi));
 	if (!master)
 		return -ENOMEM;
 
 	platform_set_drvdata(pdev, master);
 
 	rs = spi_master_get_devdata(master);
+	rs->slave_mode = slave_mode;
 
 	/* Get basic io resource and map it */
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -676,7 +704,16 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 	master->auto_runtime_pm = true;
 	master->bus_num = pdev->id;
 	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP | SPI_LSB_FIRST;
-	master->num_chipselect = ROCKCHIP_SPI_MAX_CS_NUM;
+	if (slave_mode) {
+		master->mode_bits |= SPI_NO_CS;
+		master->slave_abort = rockchip_spi_slave_abort;
+	} else {
+		of_property_read_u32(np, "num-cs", &num_cs);
+		master->num_chipselect = num_cs;
+		master->use_gpio_descriptors = true;
+		master->max_native_cs = ROCKCHIP_SPI_MAX_CS_NUM;
+		master->flags = SPI_MASTER_GPIO_SS;
+	}
 	master->dev.of_node = pdev->dev.of_node;
 	master->bits_per_word_mask = SPI_BPW_MASK(16) | SPI_BPW_MASK(8) | SPI_BPW_MASK(4);
 	master->min_speed_hz = rs->freq / BAUDR_SCKDV_MAX;
@@ -686,7 +723,6 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 	master->transfer_one = rockchip_spi_transfer_one;
 	master->max_transfer_size = rockchip_spi_max_transfer_size;
 	master->handle_err = rockchip_spi_handle_err;
-	master->flags = SPI_MASTER_GPIO_SS;
 
 	master->dma_tx = dma_request_chan(rs->dev, "tx");
 	if (IS_ERR(master->dma_tx)) {
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v0 1/1] spi: spi-rockchip: add support for spi slave_mode

[Emil Renner Berthing]

Hi Chris,

On Fri, 8 May 2020 at 10:47, Chris Ruehl <chris.ruehl@gtsys.com.hk> wrote:
>
> This patch aim to add spi slave mode support to the rockchip driver.
> Fix the wrong usage of num_cs set fix to ROCKCHIP_SPI_MAX_CS_NUM,
> instead use max_native_cs flag to set the limit of native chip-select.
> Enable use_gpio_descriptors to have cs_gpiod for gpio based chip-selects.
>
> Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
> ---
>  drivers/spi/spi-rockchip.c | 46 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
> index 70ef63e0b6b8..9c1ff52c0f85 100644
> --- a/drivers/spi/spi-rockchip.c
> +++ b/drivers/spi/spi-rockchip.c
> @@ -183,6 +183,9 @@ struct rockchip_spi {
>         u8 rsd;
>
>         bool cs_asserted[ROCKCHIP_SPI_MAX_CS_NUM];
> +
> +       bool slave_mode;
> +       bool slave_abort;
>  };
>
>  static inline void spi_enable_chip(struct rockchip_spi *rs, bool enable)
> @@ -359,7 +362,7 @@ static void rockchip_spi_dma_rxcb(void *data)
>         struct rockchip_spi *rs = spi_master_get_devdata(master);
>         int state = atomic_fetch_andnot(RXDMA, &rs->state);
>
> -       if (state & TXDMA)
> +       if (state & TXDMA && !rs->slave_abort)
>                 return;
>
>         spi_enable_chip(rs, false);
> @@ -372,7 +375,7 @@ static void rockchip_spi_dma_txcb(void *data)
>         struct rockchip_spi *rs = spi_master_get_devdata(master);
>         int state = atomic_fetch_andnot(TXDMA, &rs->state);
>
> -       if (state & RXDMA)
> +       if (state & RXDMA && !rs->slave_abort)
>                 return;
>
>         /* Wait until the FIFO data completely. */
> @@ -466,6 +469,10 @@ static void rockchip_spi_config(struct rockchip_spi *rs,
>         u32 cr1;
>         u32 dmacr = 0;
>
> +       if (rs->slavemode)
> +               cr0 |= CR0_OPM_SLAVE << CR0_OPM_OFFSET;
> +       rs->slave_abort = false;
> +
>         cr0 |= rs->rsd << CR0_RSD_OFFSET;
>         cr0 |= (spi->mode & 0x3U) << CR0_SCPH_OFFSET;
>         if (spi->mode & SPI_LSB_FIRST)
> @@ -535,6 +542,16 @@ static size_t rockchip_spi_max_transfer_size(struct spi_device *spi)
>         return ROCKCHIP_SPI_MAX_TRANLEN;
>  }
>
> +static int rockchip_spi_slave_abort(struct spi_master *master)
> +{
> +       struct rockchip_spi *rs = spi_master_get_devdata(master);
> +
> +       rs->slave_abort = true;
> +       complete(master);
> +
> +       return 0;
> +}
> +
>  static int rockchip_spi_transfer_one(
>                 struct spi_master *master,
>                 struct spi_device *spi,
> @@ -589,14 +606,25 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>         struct spi_master *master;
>         struct resource *mem;
>         u32 rsd_nsecs;
> +       bool slave_mode;
> +       u32 num_cs = 1;
> +
> +       slave_mode = of_property_read_bool(np, "spi-slave");
> +
> +       if (slave_mode)
> +               master = spi_alloc_slave(&pdev->dev,
> +                               sizeof(struct rockchip_spi));
> +       else
> +               master = spi_alloc_master(&pdev->dev,
> +                               sizeof(struct rockchip_spi));
>
> -       master = spi_alloc_master(&pdev->dev, sizeof(struct rockchip_spi));
>         if (!master)
>                 return -ENOMEM;
>
>         platform_set_drvdata(pdev, master);
>
>         rs = spi_master_get_devdata(master);
> +       rs->slave_mode = slave_mode;

This entry doesn't seem to be read from any of your code, and even it
it was, the same information is available in master->slave, so I don't
see why you need it in the rockchip_spi struct.

Also spi_master is just #defined to spi_controller in spi.h, so maybe
consider changing all 'struct spi_master *master' to 'struct
spi_controller *ctrl' now that the driver supports both modes.

>
>         /* Get basic io resource and map it */
>         mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -676,7 +704,16 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>         master->auto_runtime_pm = true;
>         master->bus_num = pdev->id;
>         master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP | SPI_LSB_FIRST;
> -       master->num_chipselect = ROCKCHIP_SPI_MAX_CS_NUM;
> +       if (slave_mode) {
> +               master->mode_bits |= SPI_NO_CS;
> +               master->slave_abort = rockchip_spi_slave_abort;
> +       } else {
> +               of_property_read_u32(np, "num-cs", &num_cs);
> +               master->num_chipselect = num_cs;

If you do something like this you won't need the temporary num_cs variable:

if (of_property_read_u32(np, "num-cs", &master->num_chipselect))
    master->num_chipselect = 1;

Also it seems like you're changing the default from
ROCKCHIP_SPI_MAX_CS_NUM to 1 if there is no num-cs property. Did you
check that all boards either have the num-cs property defined or only
needs num_chipselect = 1?

> +               master->use_gpio_descriptors = true;
> +               master->max_native_cs = ROCKCHIP_SPI_MAX_CS_NUM;
> +               master->flags = SPI_MASTER_GPIO_SS;
> +       }
>         master->dev.of_node = pdev->dev.of_node;
>         master->bits_per_word_mask = SPI_BPW_MASK(16) | SPI_BPW_MASK(8) | SPI_BPW_MASK(4);
>         master->min_speed_hz = rs->freq / BAUDR_SCKDV_MAX;
> @@ -686,7 +723,6 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>         master->transfer_one = rockchip_spi_transfer_one;
>         master->max_transfer_size = rockchip_spi_max_transfer_size;
>         master->handle_err = rockchip_spi_handle_err;
> -       master->flags = SPI_MASTER_GPIO_SS;
>
>         master->dma_tx = dma_request_chan(rs->dev, "tx");
>         if (IS_ERR(master->dma_tx)) {
> --
> 2.20.1
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v0 1/1] spi: spi-rockchip: add support for spi slave_mode

[Emil Renner Berthing]

Hi Chris,

On Fri, 8 May 2020 at 10:47, Chris Ruehl <chris.ruehl@gtsys.com.hk> wrote:
>
> This patch aim to add spi slave mode support to the rockchip driver.
> Fix the wrong usage of num_cs set fix to ROCKCHIP_SPI_MAX_CS_NUM,
> instead use max_native_cs flag to set the limit of native chip-select.
> Enable use_gpio_descriptors to have cs_gpiod for gpio based chip-selects.
>
> Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
> ---
>  drivers/spi/spi-rockchip.c | 46 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
> index 70ef63e0b6b8..9c1ff52c0f85 100644
> --- a/drivers/spi/spi-rockchip.c
> +++ b/drivers/spi/spi-rockchip.c
> @@ -183,6 +183,9 @@ struct rockchip_spi {
>         u8 rsd;
>
>         bool cs_asserted[ROCKCHIP_SPI_MAX_CS_NUM];
> +
> +       bool slave_mode;
> +       bool slave_abort;
>  };
>
>  static inline void spi_enable_chip(struct rockchip_spi *rs, bool enable)
> @@ -359,7 +362,7 @@ static void rockchip_spi_dma_rxcb(void *data)
>         struct rockchip_spi *rs = spi_master_get_devdata(master);
>         int state = atomic_fetch_andnot(RXDMA, &rs->state);
>
> -       if (state & TXDMA)
> +       if (state & TXDMA && !rs->slave_abort)
>                 return;
>
>         spi_enable_chip(rs, false);
> @@ -372,7 +375,7 @@ static void rockchip_spi_dma_txcb(void *data)
>         struct rockchip_spi *rs = spi_master_get_devdata(master);
>         int state = atomic_fetch_andnot(TXDMA, &rs->state);
>
> -       if (state & RXDMA)
> +       if (state & RXDMA && !rs->slave_abort)
>                 return;
>
>         /* Wait until the FIFO data completely. */
> @@ -466,6 +469,10 @@ static void rockchip_spi_config(struct rockchip_spi *rs,
>         u32 cr1;
>         u32 dmacr = 0;
>
> +       if (rs->slavemode)
> +               cr0 |= CR0_OPM_SLAVE << CR0_OPM_OFFSET;
> +       rs->slave_abort = false;
> +
>         cr0 |= rs->rsd << CR0_RSD_OFFSET;
>         cr0 |= (spi->mode & 0x3U) << CR0_SCPH_OFFSET;
>         if (spi->mode & SPI_LSB_FIRST)
> @@ -535,6 +542,16 @@ static size_t rockchip_spi_max_transfer_size(struct spi_device *spi)
>         return ROCKCHIP_SPI_MAX_TRANLEN;
>  }
>
> +static int rockchip_spi_slave_abort(struct spi_master *master)
> +{
> +       struct rockchip_spi *rs = spi_master_get_devdata(master);
> +
> +       rs->slave_abort = true;
> +       complete(master);
> +
> +       return 0;
> +}
> +
>  static int rockchip_spi_transfer_one(
>                 struct spi_master *master,
>                 struct spi_device *spi,
> @@ -589,14 +606,25 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>         struct spi_master *master;
>         struct resource *mem;
>         u32 rsd_nsecs;
> +       bool slave_mode;
> +       u32 num_cs = 1;
> +
> +       slave_mode = of_property_read_bool(np, "spi-slave");
> +
> +       if (slave_mode)
> +               master = spi_alloc_slave(&pdev->dev,
> +                               sizeof(struct rockchip_spi));
> +       else
> +               master = spi_alloc_master(&pdev->dev,
> +                               sizeof(struct rockchip_spi));
>
> -       master = spi_alloc_master(&pdev->dev, sizeof(struct rockchip_spi));
>         if (!master)
>                 return -ENOMEM;
>
>         platform_set_drvdata(pdev, master);
>
>         rs = spi_master_get_devdata(master);
> +       rs->slave_mode = slave_mode;

This entry doesn't seem to be read from any of your code, and even it
it was, the same information is available in master->slave, so I don't
see why you need it in the rockchip_spi struct.

Also spi_master is just #defined to spi_controller in spi.h, so maybe
consider changing all 'struct spi_master *master' to 'struct
spi_controller *ctrl' now that the driver supports both modes.

>
>         /* Get basic io resource and map it */
>         mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -676,7 +704,16 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>         master->auto_runtime_pm = true;
>         master->bus_num = pdev->id;
>         master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP | SPI_LSB_FIRST;
> -       master->num_chipselect = ROCKCHIP_SPI_MAX_CS_NUM;
> +       if (slave_mode) {
> +               master->mode_bits |= SPI_NO_CS;
> +               master->slave_abort = rockchip_spi_slave_abort;
> +       } else {
> +               of_property_read_u32(np, "num-cs", &num_cs);
> +               master->num_chipselect = num_cs;

If you do something like this you won't need the temporary num_cs variable:

if (of_property_read_u32(np, "num-cs", &master->num_chipselect))
    master->num_chipselect = 1;

Also it seems like you're changing the default from
ROCKCHIP_SPI_MAX_CS_NUM to 1 if there is no num-cs property. Did you
check that all boards either have the num-cs property defined or only
needs num_chipselect = 1?

> +               master->use_gpio_descriptors = true;
> +               master->max_native_cs = ROCKCHIP_SPI_MAX_CS_NUM;
> +               master->flags = SPI_MASTER_GPIO_SS;
> +       }
>         master->dev.of_node = pdev->dev.of_node;
>         master->bits_per_word_mask = SPI_BPW_MASK(16) | SPI_BPW_MASK(8) | SPI_BPW_MASK(4);
>         master->min_speed_hz = rs->freq / BAUDR_SCKDV_MAX;
> @@ -686,7 +723,6 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>         master->transfer_one = rockchip_spi_transfer_one;
>         master->max_transfer_size = rockchip_spi_max_transfer_size;
>         master->handle_err = rockchip_spi_handle_err;
> -       master->flags = SPI_MASTER_GPIO_SS;
>
>         master->dma_tx = dma_request_chan(rs->dev, "tx");
>         if (IS_ERR(master->dma_tx)) {
> --
> 2.20.1
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v0 1/1] spi: spi-rockchip: add support for spi slave_mode

[Emil Renner Berthing]

Hi Chris,

On Fri, 8 May 2020 at 10:47, Chris Ruehl <chris.ruehl-CR359r9tUDPXPF5Rlphj1Q@public.gmane.org> wrote:
>
> This patch aim to add spi slave mode support to the rockchip driver.
> Fix the wrong usage of num_cs set fix to ROCKCHIP_SPI_MAX_CS_NUM,
> instead use max_native_cs flag to set the limit of native chip-select.
> Enable use_gpio_descriptors to have cs_gpiod for gpio based chip-selects.
>
> Signed-off-by: Chris Ruehl <chris.ruehl-CR359r9tUDPXPF5Rlphj1Q@public.gmane.org>
> ---
>  drivers/spi/spi-rockchip.c | 46 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
> index 70ef63e0b6b8..9c1ff52c0f85 100644
> --- a/drivers/spi/spi-rockchip.c
> +++ b/drivers/spi/spi-rockchip.c
> @@ -183,6 +183,9 @@ struct rockchip_spi {
>         u8 rsd;
>
>         bool cs_asserted[ROCKCHIP_SPI_MAX_CS_NUM];
> +
> +       bool slave_mode;
> +       bool slave_abort;
>  };
>
>  static inline void spi_enable_chip(struct rockchip_spi *rs, bool enable)
> @@ -359,7 +362,7 @@ static void rockchip_spi_dma_rxcb(void *data)
>         struct rockchip_spi *rs = spi_master_get_devdata(master);
>         int state = atomic_fetch_andnot(RXDMA, &rs->state);
>
> -       if (state & TXDMA)
> +       if (state & TXDMA && !rs->slave_abort)
>                 return;
>
>         spi_enable_chip(rs, false);
> @@ -372,7 +375,7 @@ static void rockchip_spi_dma_txcb(void *data)
>         struct rockchip_spi *rs = spi_master_get_devdata(master);
>         int state = atomic_fetch_andnot(TXDMA, &rs->state);
>
> -       if (state & RXDMA)
> +       if (state & RXDMA && !rs->slave_abort)
>                 return;
>
>         /* Wait until the FIFO data completely. */
> @@ -466,6 +469,10 @@ static void rockchip_spi_config(struct rockchip_spi *rs,
>         u32 cr1;
>         u32 dmacr = 0;
>
> +       if (rs->slavemode)
> +               cr0 |= CR0_OPM_SLAVE << CR0_OPM_OFFSET;
> +       rs->slave_abort = false;
> +
>         cr0 |= rs->rsd << CR0_RSD_OFFSET;
>         cr0 |= (spi->mode & 0x3U) << CR0_SCPH_OFFSET;
>         if (spi->mode & SPI_LSB_FIRST)
> @@ -535,6 +542,16 @@ static size_t rockchip_spi_max_transfer_size(struct spi_device *spi)
>         return ROCKCHIP_SPI_MAX_TRANLEN;
>  }
>
> +static int rockchip_spi_slave_abort(struct spi_master *master)
> +{
> +       struct rockchip_spi *rs = spi_master_get_devdata(master);
> +
> +       rs->slave_abort = true;
> +       complete(master);
> +
> +       return 0;
> +}
> +
>  static int rockchip_spi_transfer_one(
>                 struct spi_master *master,
>                 struct spi_device *spi,
> @@ -589,14 +606,25 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>         struct spi_master *master;
>         struct resource *mem;
>         u32 rsd_nsecs;
> +       bool slave_mode;
> +       u32 num_cs = 1;
> +
> +       slave_mode = of_property_read_bool(np, "spi-slave");
> +
> +       if (slave_mode)
> +               master = spi_alloc_slave(&pdev->dev,
> +                               sizeof(struct rockchip_spi));
> +       else
> +               master = spi_alloc_master(&pdev->dev,
> +                               sizeof(struct rockchip_spi));
>
> -       master = spi_alloc_master(&pdev->dev, sizeof(struct rockchip_spi));
>         if (!master)
>                 return -ENOMEM;
>
>         platform_set_drvdata(pdev, master);
>
>         rs = spi_master_get_devdata(master);
> +       rs->slave_mode = slave_mode;

This entry doesn't seem to be read from any of your code, and even it
it was, the same information is available in master->slave, so I don't
see why you need it in the rockchip_spi struct.

Also spi_master is just #defined to spi_controller in spi.h, so maybe
consider changing all 'struct spi_master *master' to 'struct
spi_controller *ctrl' now that the driver supports both modes.

>
>         /* Get basic io resource and map it */
>         mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -676,7 +704,16 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>         master->auto_runtime_pm = true;
>         master->bus_num = pdev->id;
>         master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP | SPI_LSB_FIRST;
> -       master->num_chipselect = ROCKCHIP_SPI_MAX_CS_NUM;
> +       if (slave_mode) {
> +               master->mode_bits |= SPI_NO_CS;
> +               master->slave_abort = rockchip_spi_slave_abort;
> +       } else {
> +               of_property_read_u32(np, "num-cs", &num_cs);
> +               master->num_chipselect = num_cs;

If you do something like this you won't need the temporary num_cs variable:

if (of_property_read_u32(np, "num-cs", &master->num_chipselect))
    master->num_chipselect = 1;

Also it seems like you're changing the default from
ROCKCHIP_SPI_MAX_CS_NUM to 1 if there is no num-cs property. Did you
check that all boards either have the num-cs property defined or only
needs num_chipselect = 1?

> +               master->use_gpio_descriptors = true;
> +               master->max_native_cs = ROCKCHIP_SPI_MAX_CS_NUM;
> +               master->flags = SPI_MASTER_GPIO_SS;
> +       }
>         master->dev.of_node = pdev->dev.of_node;
>         master->bits_per_word_mask = SPI_BPW_MASK(16) | SPI_BPW_MASK(8) | SPI_BPW_MASK(4);
>         master->min_speed_hz = rs->freq / BAUDR_SCKDV_MAX;
> @@ -686,7 +723,6 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>         master->transfer_one = rockchip_spi_transfer_one;
>         master->max_transfer_size = rockchip_spi_max_transfer_size;
>         master->handle_err = rockchip_spi_handle_err;
> -       master->flags = SPI_MASTER_GPIO_SS;
>
>         master->dma_tx = dma_request_chan(rs->dev, "tx");
>         if (IS_ERR(master->dma_tx)) {
> --
> 2.20.1
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v0 1/1] spi: spi-rockchip: add support for spi slave_mode

[Emil Renner Berthing]

Hi Chris,

On Fri, 8 May 2020 at 15:13, Emil Renner Berthing
<emil.renner.berthing@gmail.com> wrote:
> If you do something like this you won't need the temporary num_cs variable:
>
> if (of_property_read_u32(np, "num-cs", &master->num_chipselect))
>     master->num_chipselect = 1;

Sorry, that should be of_property_read_u16, since num_chipselect is a u16.

/Emil

Re: [PATCH v0 1/1] spi: spi-rockchip: add support for spi slave_mode

[Emil Renner Berthing]

Hi Chris,

On Fri, 8 May 2020 at 15:13, Emil Renner Berthing
<emil.renner.berthing@gmail.com> wrote:
> If you do something like this you won't need the temporary num_cs variable:
>
> if (of_property_read_u32(np, "num-cs", &master->num_chipselect))
>     master->num_chipselect = 1;

Sorry, that should be of_property_read_u16, since num_chipselect is a u16.

/Emil

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v0 1/1] spi: spi-rockchip: add support for spi slave_mode

[Emil Renner Berthing]

Hi Chris,

On Fri, 8 May 2020 at 15:13, Emil Renner Berthing
<emil.renner.berthing-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> If you do something like this you won't need the temporary num_cs variable:
>
> if (of_property_read_u32(np, "num-cs", &master->num_chipselect))
>     master->num_chipselect = 1;

Sorry, that should be of_property_read_u16, since num_chipselect is a u16.

/Emil

Re: [PATCH v0 1/1] spi: spi-rockchip: add support for spi slave_mode

[Chris Ruehl]

Hi Emil,

thanks for the review and your comments

On 8/5/2020 9:13 pm, Emil Renner Berthing wrote:
> Hi Chris,
>
> On Fri, 8 May 2020 at 10:47, Chris Ruehl <chris.ruehl@gtsys.com.hk> wrote:
>> This patch aim to add spi slave mode support to the rockchip driver.
>> Fix the wrong usage of num_cs set fix to ROCKCHIP_SPI_MAX_CS_NUM,
>> instead use max_native_cs flag to set the limit of native chip-select.
>> Enable use_gpio_descriptors to have cs_gpiod for gpio based chip-selects.
>>
>> Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
>> ---
>>   drivers/spi/spi-rockchip.c | 46 +++++++++++++++++++++++++++++++++-----
>>   1 file changed, 41 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
>> index 70ef63e0b6b8..9c1ff52c0f85 100644
>> --- a/drivers/spi/spi-rockchip.c
>> +++ b/drivers/spi/spi-rockchip.c
>> @@ -183,6 +183,9 @@ struct rockchip_spi {
>>          u8 rsd;
>>
>>          bool cs_asserted[ROCKCHIP_SPI_MAX_CS_NUM];
>> +
>> +       bool slave_mode;
>> +       bool slave_abort;
>>   };
>>
>>   static inline void spi_enable_chip(struct rockchip_spi *rs, bool enable)
>> @@ -359,7 +362,7 @@ static void rockchip_spi_dma_rxcb(void *data)
>>          struct rockchip_spi *rs = spi_master_get_devdata(master);
>>          int state = atomic_fetch_andnot(RXDMA, &rs->state);
>>
>> -       if (state & TXDMA)
>> +       if (state & TXDMA && !rs->slave_abort)
>>                  return;
>>
>>          spi_enable_chip(rs, false);
>> @@ -372,7 +375,7 @@ static void rockchip_spi_dma_txcb(void *data)
>>          struct rockchip_spi *rs = spi_master_get_devdata(master);
>>          int state = atomic_fetch_andnot(TXDMA, &rs->state);
>>
>> -       if (state & RXDMA)
>> +       if (state & RXDMA && !rs->slave_abort)
>>                  return;
>>
>>          /* Wait until the FIFO data completely. */
>> @@ -466,6 +469,10 @@ static void rockchip_spi_config(struct rockchip_spi *rs,
>>          u32 cr1;
>>          u32 dmacr = 0;
>>
>> +       if (rs->slavemode)
>> +               cr0 |= CR0_OPM_SLAVE << CR0_OPM_OFFSET;
>> +       rs->slave_abort = false;
>> +
>>          cr0 |= rs->rsd << CR0_RSD_OFFSET;
>>          cr0 |= (spi->mode & 0x3U) << CR0_SCPH_OFFSET;
>>          if (spi->mode & SPI_LSB_FIRST)
>> @@ -535,6 +542,16 @@ static size_t rockchip_spi_max_transfer_size(struct spi_device *spi)
>>          return ROCKCHIP_SPI_MAX_TRANLEN;
>>   }
>>
>> +static int rockchip_spi_slave_abort(struct spi_master *master)
>> +{
>> +       struct rockchip_spi *rs = spi_master_get_devdata(master);
>> +
>> +       rs->slave_abort = true;
>> +       complete(master);
>> +
>> +       return 0;
>> +}
>> +
>>   static int rockchip_spi_transfer_one(
>>                  struct spi_master *master,
>>                  struct spi_device *spi,
>> @@ -589,14 +606,25 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>>          struct spi_master *master;
>>          struct resource *mem;
>>          u32 rsd_nsecs;
>> +       bool slave_mode;
>> +       u32 num_cs = 1;
>> +
>> +       slave_mode = of_property_read_bool(np, "spi-slave");
>> +
>> +       if (slave_mode)
>> +               master = spi_alloc_slave(&pdev->dev,
>> +                               sizeof(struct rockchip_spi));
>> +       else
>> +               master = spi_alloc_master(&pdev->dev,
>> +                               sizeof(struct rockchip_spi));
>>
>> -       master = spi_alloc_master(&pdev->dev, sizeof(struct rockchip_spi));
>>          if (!master)
>>                  return -ENOMEM;
>>
>>          platform_set_drvdata(pdev, master);
>>
>>          rs = spi_master_get_devdata(master);
>> +       rs->slave_mode = slave_mode;
> This entry doesn't seem to be read from any of your code, and even it
> it was, the same information is available in master->slave, so I don't
> see why you need it in the rockchip_spi struct.
I haven't see the slave flag in the spi_controller struct, I will store the 
information
there.
>
> Also spi_master is just #defined to spi_controller in spi.h, so maybe
> consider changing all 'struct spi_master *master' to 'struct
> spi_controller *ctrl' now that the driver supports both modes.
Can do,  but I think that is better to have a separate patch for it,
make it easier for review.
>
>>          /* Get basic io resource and map it */
>>          mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> @@ -676,7 +704,16 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>>          master->auto_runtime_pm = true;
>>          master->bus_num = pdev->id;
>>          master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP | SPI_LSB_FIRST;
>> -       master->num_chipselect = ROCKCHIP_SPI_MAX_CS_NUM;
>> +       if (slave_mode) {
>> +               master->mode_bits |= SPI_NO_CS;
>> +               master->slave_abort = rockchip_spi_slave_abort;
>> +       } else {
>> +               of_property_read_u32(np, "num-cs", &num_cs);
>> +               master->num_chipselect = num_cs;
> If you do something like this you won't need the temporary num_cs variable:
>
> if (of_property_read_u32(np, "num-cs", &master->num_chipselect))
>      master->num_chipselect = 1;
Like it , can see clearly the fallback to a default if num-cs isn't set in the
dts.
>
> Also it seems like you're changing the default from
> ROCKCHIP_SPI_MAX_CS_NUM to 1 if there is no num-cs property. Did you
> check that all boards either have the num-cs property defined or only
> needs num_chipselect = 1?
Only spi0 of the rockchip has a 2nd native chip select, all others a single only
therefore I find it less evil to use 1 vs. ROCKCHIP_SPI_MAX_CS_NUM

>> +               master->use_gpio_descriptors = true;
>> +               master->max_native_cs = ROCKCHIP_SPI_MAX_CS_NUM;
>> +               master->flags = SPI_MASTER_GPIO_SS;
>> +       }
>>          master->dev.of_node = pdev->dev.of_node;
>>          master->bits_per_word_mask = SPI_BPW_MASK(16) | SPI_BPW_MASK(8) | SPI_BPW_MASK(4);
>>          master->min_speed_hz = rs->freq / BAUDR_SCKDV_MAX;
>> @@ -686,7 +723,6 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>>          master->transfer_one = rockchip_spi_transfer_one;
>>          master->max_transfer_size = rockchip_spi_max_transfer_size;
>>          master->handle_err = rockchip_spi_handle_err;
>> -       master->flags = SPI_MASTER_GPIO_SS;
>>
>>          master->dma_tx = dma_request_chan(rs->dev, "tx");
>>          if (IS_ERR(master->dma_tx)) {
>> --
>> 2.20.1
>>
>>
>> _______________________________________________
>> Linux-rockchip mailing list
>> Linux-rockchip@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-rockchip

-- 
GTSYS Limited RFID Technology
9/F, Unit E, R07, Kwai Shing Industrial Building Phase 2,
42-46 Tai Lin Pai Road, Kwai Chung, N.T., Hong Kong
Tel (852) 9079 9521

Disclaimer: https://www.gtsys.com.hk/email/classified.html

Re: [PATCH v0 1/1] spi: spi-rockchip: add support for spi slave_mode

[Chris Ruehl]

Hi Emil,

thanks for the review and your comments

On 8/5/2020 9:13 pm, Emil Renner Berthing wrote:
> Hi Chris,
>
> On Fri, 8 May 2020 at 10:47, Chris Ruehl <chris.ruehl@gtsys.com.hk> wrote:
>> This patch aim to add spi slave mode support to the rockchip driver.
>> Fix the wrong usage of num_cs set fix to ROCKCHIP_SPI_MAX_CS_NUM,
>> instead use max_native_cs flag to set the limit of native chip-select.
>> Enable use_gpio_descriptors to have cs_gpiod for gpio based chip-selects.
>>
>> Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
>> ---
>>   drivers/spi/spi-rockchip.c | 46 +++++++++++++++++++++++++++++++++-----
>>   1 file changed, 41 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
>> index 70ef63e0b6b8..9c1ff52c0f85 100644
>> --- a/drivers/spi/spi-rockchip.c
>> +++ b/drivers/spi/spi-rockchip.c
>> @@ -183,6 +183,9 @@ struct rockchip_spi {
>>          u8 rsd;
>>
>>          bool cs_asserted[ROCKCHIP_SPI_MAX_CS_NUM];
>> +
>> +       bool slave_mode;
>> +       bool slave_abort;
>>   };
>>
>>   static inline void spi_enable_chip(struct rockchip_spi *rs, bool enable)
>> @@ -359,7 +362,7 @@ static void rockchip_spi_dma_rxcb(void *data)
>>          struct rockchip_spi *rs = spi_master_get_devdata(master);
>>          int state = atomic_fetch_andnot(RXDMA, &rs->state);
>>
>> -       if (state & TXDMA)
>> +       if (state & TXDMA && !rs->slave_abort)
>>                  return;
>>
>>          spi_enable_chip(rs, false);
>> @@ -372,7 +375,7 @@ static void rockchip_spi_dma_txcb(void *data)
>>          struct rockchip_spi *rs = spi_master_get_devdata(master);
>>          int state = atomic_fetch_andnot(TXDMA, &rs->state);
>>
>> -       if (state & RXDMA)
>> +       if (state & RXDMA && !rs->slave_abort)
>>                  return;
>>
>>          /* Wait until the FIFO data completely. */
>> @@ -466,6 +469,10 @@ static void rockchip_spi_config(struct rockchip_spi *rs,
>>          u32 cr1;
>>          u32 dmacr = 0;
>>
>> +       if (rs->slavemode)
>> +               cr0 |= CR0_OPM_SLAVE << CR0_OPM_OFFSET;
>> +       rs->slave_abort = false;
>> +
>>          cr0 |= rs->rsd << CR0_RSD_OFFSET;
>>          cr0 |= (spi->mode & 0x3U) << CR0_SCPH_OFFSET;
>>          if (spi->mode & SPI_LSB_FIRST)
>> @@ -535,6 +542,16 @@ static size_t rockchip_spi_max_transfer_size(struct spi_device *spi)
>>          return ROCKCHIP_SPI_MAX_TRANLEN;
>>   }
>>
>> +static int rockchip_spi_slave_abort(struct spi_master *master)
>> +{
>> +       struct rockchip_spi *rs = spi_master_get_devdata(master);
>> +
>> +       rs->slave_abort = true;
>> +       complete(master);
>> +
>> +       return 0;
>> +}
>> +
>>   static int rockchip_spi_transfer_one(
>>                  struct spi_master *master,
>>                  struct spi_device *spi,
>> @@ -589,14 +606,25 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>>          struct spi_master *master;
>>          struct resource *mem;
>>          u32 rsd_nsecs;
>> +       bool slave_mode;
>> +       u32 num_cs = 1;
>> +
>> +       slave_mode = of_property_read_bool(np, "spi-slave");
>> +
>> +       if (slave_mode)
>> +               master = spi_alloc_slave(&pdev->dev,
>> +                               sizeof(struct rockchip_spi));
>> +       else
>> +               master = spi_alloc_master(&pdev->dev,
>> +                               sizeof(struct rockchip_spi));
>>
>> -       master = spi_alloc_master(&pdev->dev, sizeof(struct rockchip_spi));
>>          if (!master)
>>                  return -ENOMEM;
>>
>>          platform_set_drvdata(pdev, master);
>>
>>          rs = spi_master_get_devdata(master);
>> +       rs->slave_mode = slave_mode;
> This entry doesn't seem to be read from any of your code, and even it
> it was, the same information is available in master->slave, so I don't
> see why you need it in the rockchip_spi struct.
I haven't see the slave flag in the spi_controller struct, I will store the 
information
there.
>
> Also spi_master is just #defined to spi_controller in spi.h, so maybe
> consider changing all 'struct spi_master *master' to 'struct
> spi_controller *ctrl' now that the driver supports both modes.
Can do,  but I think that is better to have a separate patch for it,
make it easier for review.
>
>>          /* Get basic io resource and map it */
>>          mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> @@ -676,7 +704,16 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>>          master->auto_runtime_pm = true;
>>          master->bus_num = pdev->id;
>>          master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP | SPI_LSB_FIRST;
>> -       master->num_chipselect = ROCKCHIP_SPI_MAX_CS_NUM;
>> +       if (slave_mode) {
>> +               master->mode_bits |= SPI_NO_CS;
>> +               master->slave_abort = rockchip_spi_slave_abort;
>> +       } else {
>> +               of_property_read_u32(np, "num-cs", &num_cs);
>> +               master->num_chipselect = num_cs;
> If you do something like this you won't need the temporary num_cs variable:
>
> if (of_property_read_u32(np, "num-cs", &master->num_chipselect))
>      master->num_chipselect = 1;
Like it , can see clearly the fallback to a default if num-cs isn't set in the
dts.
>
> Also it seems like you're changing the default from
> ROCKCHIP_SPI_MAX_CS_NUM to 1 if there is no num-cs property. Did you
> check that all boards either have the num-cs property defined or only
> needs num_chipselect = 1?
Only spi0 of the rockchip has a 2nd native chip select, all others a single only
therefore I find it less evil to use 1 vs. ROCKCHIP_SPI_MAX_CS_NUM

>> +               master->use_gpio_descriptors = true;
>> +               master->max_native_cs = ROCKCHIP_SPI_MAX_CS_NUM;
>> +               master->flags = SPI_MASTER_GPIO_SS;
>> +       }
>>          master->dev.of_node = pdev->dev.of_node;
>>          master->bits_per_word_mask = SPI_BPW_MASK(16) | SPI_BPW_MASK(8) | SPI_BPW_MASK(4);
>>          master->min_speed_hz = rs->freq / BAUDR_SCKDV_MAX;
>> @@ -686,7 +723,6 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>>          master->transfer_one = rockchip_spi_transfer_one;
>>          master->max_transfer_size = rockchip_spi_max_transfer_size;
>>          master->handle_err = rockchip_spi_handle_err;
>> -       master->flags = SPI_MASTER_GPIO_SS;
>>
>>          master->dma_tx = dma_request_chan(rs->dev, "tx");
>>          if (IS_ERR(master->dma_tx)) {
>> --
>> 2.20.1
>>
>>
>> _______________________________________________
>> Linux-rockchip mailing list
>> Linux-rockchip@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-rockchip

-- 
GTSYS Limited RFID Technology
9/F, Unit E, R07, Kwai Shing Industrial Building Phase 2,
42-46 Tai Lin Pai Road, Kwai Chung, N.T., Hong Kong
Tel (852) 9079 9521

Disclaimer: https://www.gtsys.com.hk/email/classified.html


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v0 1/1] spi: spi-rockchip: add support for spi slave_mode

[Emil Renner Berthing]

Hi Chris,

On Sat, 9 May 2020 at 02:10, Chris Ruehl <chris.ruehl@gtsys.com.hk> wrote:
>
> Hi Emil,
>
> thanks for the review and your comments
>
> On 8/5/2020 9:13 pm, Emil Renner Berthing wrote:
> > Hi Chris,
> >
> > On Fri, 8 May 2020 at 10:47, Chris Ruehl <chris.ruehl@gtsys.com.hk> wrote:
> >> This patch aim to add spi slave mode support to the rockchip driver.
> >> Fix the wrong usage of num_cs set fix to ROCKCHIP_SPI_MAX_CS_NUM,
> >> instead use max_native_cs flag to set the limit of native chip-select.
> >> Enable use_gpio_descriptors to have cs_gpiod for gpio based chip-selects.
> >>
> >> Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
> >> ---
> >>   drivers/spi/spi-rockchip.c | 46 +++++++++++++++++++++++++++++++++-----
> >>   1 file changed, 41 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
> >> index 70ef63e0b6b8..9c1ff52c0f85 100644
> >> --- a/drivers/spi/spi-rockchip.c
> >> +++ b/drivers/spi/spi-rockchip.c
> >> @@ -183,6 +183,9 @@ struct rockchip_spi {
> >>          u8 rsd;
> >>
> >>          bool cs_asserted[ROCKCHIP_SPI_MAX_CS_NUM];
> >> +
> >> +       bool slave_mode;
> >> +       bool slave_abort;
> >>   };
> >>
> >>   static inline void spi_enable_chip(struct rockchip_spi *rs, bool enable)
> >> @@ -359,7 +362,7 @@ static void rockchip_spi_dma_rxcb(void *data)
> >>          struct rockchip_spi *rs = spi_master_get_devdata(master);
> >>          int state = atomic_fetch_andnot(RXDMA, &rs->state);
> >>
> >> -       if (state & TXDMA)
> >> +       if (state & TXDMA && !rs->slave_abort)
> >>                  return;
> >>
> >>          spi_enable_chip(rs, false);
> >> @@ -372,7 +375,7 @@ static void rockchip_spi_dma_txcb(void *data)
> >>          struct rockchip_spi *rs = spi_master_get_devdata(master);
> >>          int state = atomic_fetch_andnot(TXDMA, &rs->state);
> >>
> >> -       if (state & RXDMA)
> >> +       if (state & RXDMA && !rs->slave_abort)
> >>                  return;
> >>
> >>          /* Wait until the FIFO data completely. */
> >> @@ -466,6 +469,10 @@ static void rockchip_spi_config(struct rockchip_spi *rs,
> >>          u32 cr1;
> >>          u32 dmacr = 0;
> >>
> >> +       if (rs->slavemode)
> >> +               cr0 |= CR0_OPM_SLAVE << CR0_OPM_OFFSET;
> >> +       rs->slave_abort = false;
> >> +
> >>          cr0 |= rs->rsd << CR0_RSD_OFFSET;
> >>          cr0 |= (spi->mode & 0x3U) << CR0_SCPH_OFFSET;
> >>          if (spi->mode & SPI_LSB_FIRST)
> >> @@ -535,6 +542,16 @@ static size_t rockchip_spi_max_transfer_size(struct spi_device *spi)
> >>          return ROCKCHIP_SPI_MAX_TRANLEN;
> >>   }
> >>
> >> +static int rockchip_spi_slave_abort(struct spi_master *master)
> >> +{
> >> +       struct rockchip_spi *rs = spi_master_get_devdata(master);
> >> +
> >> +       rs->slave_abort = true;
> >> +       complete(master);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >>   static int rockchip_spi_transfer_one(
> >>                  struct spi_master *master,
> >>                  struct spi_device *spi,
> >> @@ -589,14 +606,25 @@ static int rockchip_spi_probe(struct platform_device *pdev)
> >>          struct spi_master *master;
> >>          struct resource *mem;
> >>          u32 rsd_nsecs;
> >> +       bool slave_mode;
> >> +       u32 num_cs = 1;
> >> +
> >> +       slave_mode = of_property_read_bool(np, "spi-slave");
> >> +
> >> +       if (slave_mode)
> >> +               master = spi_alloc_slave(&pdev->dev,
> >> +                               sizeof(struct rockchip_spi));
> >> +       else
> >> +               master = spi_alloc_master(&pdev->dev,
> >> +                               sizeof(struct rockchip_spi));
> >>
> >> -       master = spi_alloc_master(&pdev->dev, sizeof(struct rockchip_spi));
> >>          if (!master)
> >>                  return -ENOMEM;
> >>
> >>          platform_set_drvdata(pdev, master);
> >>
> >>          rs = spi_master_get_devdata(master);
> >> +       rs->slave_mode = slave_mode;
> > This entry doesn't seem to be read from any of your code, and even it
> > it was, the same information is available in master->slave, so I don't
> > see why you need it in the rockchip_spi struct.
> I haven't see the slave flag in the spi_controller struct, I will store the
> information
> there.
> >
> > Also spi_master is just #defined to spi_controller in spi.h, so maybe
> > consider changing all 'struct spi_master *master' to 'struct
> > spi_controller *ctrl' now that the driver supports both modes.
> Can do,  but I think that is better to have a separate patch for it,
> make it easier for review.

Yes, you should probably do something like

patch 1/3: change struct spi_master *master to struct spi_controller *ctrl
patch 2/3: add slave slave mode
patch 3/3: use num-cs property for ctrl->num_chipselect

> >
> >>          /* Get basic io resource and map it */
> >>          mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> @@ -676,7 +704,16 @@ static int rockchip_spi_probe(struct platform_device *pdev)
> >>          master->auto_runtime_pm = true;
> >>          master->bus_num = pdev->id;
> >>          master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP | SPI_LSB_FIRST;
> >> -       master->num_chipselect = ROCKCHIP_SPI_MAX_CS_NUM;
> >> +       if (slave_mode) {
> >> +               master->mode_bits |= SPI_NO_CS;
> >> +               master->slave_abort = rockchip_spi_slave_abort;
> >> +       } else {
> >> +               of_property_read_u32(np, "num-cs", &num_cs);
> >> +               master->num_chipselect = num_cs;
> > If you do something like this you won't need the temporary num_cs variable:
> >
> > if (of_property_read_u32(np, "num-cs", &master->num_chipselect))
> >      master->num_chipselect = 1;
> Like it , can see clearly the fallback to a default if num-cs isn't set in the
> dts.
> >
> > Also it seems like you're changing the default from
> > ROCKCHIP_SPI_MAX_CS_NUM to 1 if there is no num-cs property. Did you
> > check that all boards either have the num-cs property defined or only
> > needs num_chipselect = 1?
> Only spi0 of the rockchip has a 2nd native chip select, all others a single only
> therefore I find it less evil to use 1 vs. ROCKCHIP_SPI_MAX_CS_NUM
>
> >> +               master->use_gpio_descriptors = true;
> >> +               master->max_native_cs = ROCKCHIP_SPI_MAX_CS_NUM;
> >> +               master->flags = SPI_MASTER_GPIO_SS;
> >> +       }
> >>          master->dev.of_node = pdev->dev.of_node;
> >>          master->bits_per_word_mask = SPI_BPW_MASK(16) | SPI_BPW_MASK(8) | SPI_BPW_MASK(4);
> >>          master->min_speed_hz = rs->freq / BAUDR_SCKDV_MAX;
> >> @@ -686,7 +723,6 @@ static int rockchip_spi_probe(struct platform_device *pdev)
> >>          master->transfer_one = rockchip_spi_transfer_one;
> >>          master->max_transfer_size = rockchip_spi_max_transfer_size;
> >>          master->handle_err = rockchip_spi_handle_err;
> >> -       master->flags = SPI_MASTER_GPIO_SS;
> >>
> >>          master->dma_tx = dma_request_chan(rs->dev, "tx");
> >>          if (IS_ERR(master->dma_tx)) {
> >> --
> >> 2.20.1
> >>
> >>
> >> _______________________________________________
> >> Linux-rockchip mailing list
> >> Linux-rockchip@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
> --
> GTSYS Limited RFID Technology
> 9/F, Unit E, R07, Kwai Shing Industrial Building Phase 2,
> 42-46 Tai Lin Pai Road, Kwai Chung, N.T., Hong Kong
> Tel (852) 9079 9521
>
> Disclaimer: https://www.gtsys.com.hk/email/classified.html
>

Re: [PATCH v0 1/1] spi: spi-rockchip: add support for spi slave_mode

[Emil Renner Berthing]

Hi Chris,

On Sat, 9 May 2020 at 02:10, Chris Ruehl <chris.ruehl@gtsys.com.hk> wrote:
>
> Hi Emil,
>
> thanks for the review and your comments
>
> On 8/5/2020 9:13 pm, Emil Renner Berthing wrote:
> > Hi Chris,
> >
> > On Fri, 8 May 2020 at 10:47, Chris Ruehl <chris.ruehl@gtsys.com.hk> wrote:
> >> This patch aim to add spi slave mode support to the rockchip driver.
> >> Fix the wrong usage of num_cs set fix to ROCKCHIP_SPI_MAX_CS_NUM,
> >> instead use max_native_cs flag to set the limit of native chip-select.
> >> Enable use_gpio_descriptors to have cs_gpiod for gpio based chip-selects.
> >>
> >> Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
> >> ---
> >>   drivers/spi/spi-rockchip.c | 46 +++++++++++++++++++++++++++++++++-----
> >>   1 file changed, 41 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
> >> index 70ef63e0b6b8..9c1ff52c0f85 100644
> >> --- a/drivers/spi/spi-rockchip.c
> >> +++ b/drivers/spi/spi-rockchip.c
> >> @@ -183,6 +183,9 @@ struct rockchip_spi {
> >>          u8 rsd;
> >>
> >>          bool cs_asserted[ROCKCHIP_SPI_MAX_CS_NUM];
> >> +
> >> +       bool slave_mode;
> >> +       bool slave_abort;
> >>   };
> >>
> >>   static inline void spi_enable_chip(struct rockchip_spi *rs, bool enable)
> >> @@ -359,7 +362,7 @@ static void rockchip_spi_dma_rxcb(void *data)
> >>          struct rockchip_spi *rs = spi_master_get_devdata(master);
> >>          int state = atomic_fetch_andnot(RXDMA, &rs->state);
> >>
> >> -       if (state & TXDMA)
> >> +       if (state & TXDMA && !rs->slave_abort)
> >>                  return;
> >>
> >>          spi_enable_chip(rs, false);
> >> @@ -372,7 +375,7 @@ static void rockchip_spi_dma_txcb(void *data)
> >>          struct rockchip_spi *rs = spi_master_get_devdata(master);
> >>          int state = atomic_fetch_andnot(TXDMA, &rs->state);
> >>
> >> -       if (state & RXDMA)
> >> +       if (state & RXDMA && !rs->slave_abort)
> >>                  return;
> >>
> >>          /* Wait until the FIFO data completely. */
> >> @@ -466,6 +469,10 @@ static void rockchip_spi_config(struct rockchip_spi *rs,
> >>          u32 cr1;
> >>          u32 dmacr = 0;
> >>
> >> +       if (rs->slavemode)
> >> +               cr0 |= CR0_OPM_SLAVE << CR0_OPM_OFFSET;
> >> +       rs->slave_abort = false;
> >> +
> >>          cr0 |= rs->rsd << CR0_RSD_OFFSET;
> >>          cr0 |= (spi->mode & 0x3U) << CR0_SCPH_OFFSET;
> >>          if (spi->mode & SPI_LSB_FIRST)
> >> @@ -535,6 +542,16 @@ static size_t rockchip_spi_max_transfer_size(struct spi_device *spi)
> >>          return ROCKCHIP_SPI_MAX_TRANLEN;
> >>   }
> >>
> >> +static int rockchip_spi_slave_abort(struct spi_master *master)
> >> +{
> >> +       struct rockchip_spi *rs = spi_master_get_devdata(master);
> >> +
> >> +       rs->slave_abort = true;
> >> +       complete(master);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >>   static int rockchip_spi_transfer_one(
> >>                  struct spi_master *master,
> >>                  struct spi_device *spi,
> >> @@ -589,14 +606,25 @@ static int rockchip_spi_probe(struct platform_device *pdev)
> >>          struct spi_master *master;
> >>          struct resource *mem;
> >>          u32 rsd_nsecs;
> >> +       bool slave_mode;
> >> +       u32 num_cs = 1;
> >> +
> >> +       slave_mode = of_property_read_bool(np, "spi-slave");
> >> +
> >> +       if (slave_mode)
> >> +               master = spi_alloc_slave(&pdev->dev,
> >> +                               sizeof(struct rockchip_spi));
> >> +       else
> >> +               master = spi_alloc_master(&pdev->dev,
> >> +                               sizeof(struct rockchip_spi));
> >>
> >> -       master = spi_alloc_master(&pdev->dev, sizeof(struct rockchip_spi));
> >>          if (!master)
> >>                  return -ENOMEM;
> >>
> >>          platform_set_drvdata(pdev, master);
> >>
> >>          rs = spi_master_get_devdata(master);
> >> +       rs->slave_mode = slave_mode;
> > This entry doesn't seem to be read from any of your code, and even it
> > it was, the same information is available in master->slave, so I don't
> > see why you need it in the rockchip_spi struct.
> I haven't see the slave flag in the spi_controller struct, I will store the
> information
> there.
> >
> > Also spi_master is just #defined to spi_controller in spi.h, so maybe
> > consider changing all 'struct spi_master *master' to 'struct
> > spi_controller *ctrl' now that the driver supports both modes.
> Can do,  but I think that is better to have a separate patch for it,
> make it easier for review.

Yes, you should probably do something like

patch 1/3: change struct spi_master *master to struct spi_controller *ctrl
patch 2/3: add slave slave mode
patch 3/3: use num-cs property for ctrl->num_chipselect

> >
> >>          /* Get basic io resource and map it */
> >>          mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> @@ -676,7 +704,16 @@ static int rockchip_spi_probe(struct platform_device *pdev)
> >>          master->auto_runtime_pm = true;
> >>          master->bus_num = pdev->id;
> >>          master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP | SPI_LSB_FIRST;
> >> -       master->num_chipselect = ROCKCHIP_SPI_MAX_CS_NUM;
> >> +       if (slave_mode) {
> >> +               master->mode_bits |= SPI_NO_CS;
> >> +               master->slave_abort = rockchip_spi_slave_abort;
> >> +       } else {
> >> +               of_property_read_u32(np, "num-cs", &num_cs);
> >> +               master->num_chipselect = num_cs;
> > If you do something like this you won't need the temporary num_cs variable:
> >
> > if (of_property_read_u32(np, "num-cs", &master->num_chipselect))
> >      master->num_chipselect = 1;
> Like it , can see clearly the fallback to a default if num-cs isn't set in the
> dts.
> >
> > Also it seems like you're changing the default from
> > ROCKCHIP_SPI_MAX_CS_NUM to 1 if there is no num-cs property. Did you
> > check that all boards either have the num-cs property defined or only
> > needs num_chipselect = 1?
> Only spi0 of the rockchip has a 2nd native chip select, all others a single only
> therefore I find it less evil to use 1 vs. ROCKCHIP_SPI_MAX_CS_NUM
>
> >> +               master->use_gpio_descriptors = true;
> >> +               master->max_native_cs = ROCKCHIP_SPI_MAX_CS_NUM;
> >> +               master->flags = SPI_MASTER_GPIO_SS;
> >> +       }
> >>          master->dev.of_node = pdev->dev.of_node;
> >>          master->bits_per_word_mask = SPI_BPW_MASK(16) | SPI_BPW_MASK(8) | SPI_BPW_MASK(4);
> >>          master->min_speed_hz = rs->freq / BAUDR_SCKDV_MAX;
> >> @@ -686,7 +723,6 @@ static int rockchip_spi_probe(struct platform_device *pdev)
> >>          master->transfer_one = rockchip_spi_transfer_one;
> >>          master->max_transfer_size = rockchip_spi_max_transfer_size;
> >>          master->handle_err = rockchip_spi_handle_err;
> >> -       master->flags = SPI_MASTER_GPIO_SS;
> >>
> >>          master->dma_tx = dma_request_chan(rs->dev, "tx");
> >>          if (IS_ERR(master->dma_tx)) {
> >> --
> >> 2.20.1
> >>
> >>
> >> _______________________________________________
> >> Linux-rockchip mailing list
> >> Linux-rockchip@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
> --
> GTSYS Limited RFID Technology
> 9/F, Unit E, R07, Kwai Shing Industrial Building Phase 2,
> 42-46 Tai Lin Pai Road, Kwai Chung, N.T., Hong Kong
> Tel (852) 9079 9521
>
> Disclaimer: https://www.gtsys.com.hk/email/classified.html
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v0 1/1] spi: spi-rockchip: add support for spi slave_mode

[Chris Ruehl]

Emil,


>> +       if (rs->slavemode)

here is a mistake  it is :  rs->slave_mode
and the use of rs->slave_mode in the rockchip_spi_config()

Re: [PATCH v0 1/1] spi: spi-rockchip: add support for spi slave_mode

[Chris Ruehl]

Emil,


>> +       if (rs->slavemode)

here is a mistake  it is :  rs->slave_mode
and the use of rs->slave_mode in the rockchip_spi_config()

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel