[PATCH 0/3] spi: new feature and fix for Marvell Orion driver

[PATCH 0/3] spi: new feature and fix for Marvell Orion driver

[kostap]

From: Konstantin Porotchkin <kostap@marvell.com>

This patch set contains the following:
- support for switching CS for every transferred byte
- fix for the clock by enabling it for every call to spi_setup()

Konstantin Porotchkin (1):
  dt-bindings: spi: add support for spi-1byte-cs

Marcin Wojtas (2):
  spi: orion: enable clocks before spi_setup
  spi: orion: enable support for switching CS every transferred byte

 .../bindings/spi/spi-controller.yaml          |  5 ++++
 drivers/spi/spi-orion.c                       | 30 ++++++++++++++++++-
 drivers/spi/spi.c                             |  6 ++--
 include/linux/spi/spi.h                       |  1 +
 4 files changed, 39 insertions(+), 3 deletions(-)

-- 
2.17.1

[PATCH 1/3] spi: orion: enable clocks before spi_setup

[kostap]

From: Marcin Wojtas <mw@semihalf.com>

The spi-orion driver disables its clocks whenever it is not used.
In usual case during boot (i.e. using SPI flash) it is not a problem,
as the child device driver is present and probed along with
spi_register_master() execution.

However in case the child device driver is not ready
(e.g. when its type is module_spi_driver) the spi_setup() callback
can be called after the spi-orion probe. It may happen,
that as a result there will be an attempt to access controller's
registers with the clocks disabled.

Prevent such situations and make sure the clocks are on,
each time the spi_setup() is called.

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
Reviewed-by: Stefan Chulski <Stefan.Chulski@cavium.com>
---
 drivers/spi/spi-orion.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c
index b57b8b3cc26e..3bfda4225d45 100644
--- a/drivers/spi/spi-orion.c
+++ b/drivers/spi/spi-orion.c
@@ -507,6 +507,16 @@ static int orion_spi_transfer_one(struct spi_master *master,
 
 static int orion_spi_setup(struct spi_device *spi)
 {
+	struct orion_spi *orion_spi = spi_master_get_devdata(spi->master);
+
+	/*
+	 * Make sure the clocks are enabled before
+	 * configuring the SPI controller.
+	 */
+	clk_prepare_enable(orion_spi->clk);
+	if (!IS_ERR(orion_spi->axi_clk))
+		clk_prepare_enable(orion_spi->axi_clk);
+
 	return orion_spi_setup_transfer(spi, NULL);
 }
 
-- 
2.17.1

[PATCH 2/3] spi: orion: enable support for switching CS every transferred byte

[kostap]

From: Marcin Wojtas <mw@semihalf.com>

Some SPI devices, such as SLIC (Subscriber Line Interface Card)
require toggling the CS every transferred byte. Enable such
possibility by creating a new DT property and enabling SPI
device mode update. Add according support in the spi-orion driver.

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
Signed-off-by: Konstantin Porotchkin <kostap@marvell.com>
---
 drivers/spi/spi-orion.c | 20 +++++++++++++++++++-
 drivers/spi/spi.c       |  6 ++++--
 include/linux/spi/spi.h |  1 +
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c
index 3bfda4225d45..7db9034b0879 100644
--- a/drivers/spi/spi-orion.c
+++ b/drivers/spi/spi-orion.c
@@ -369,8 +369,15 @@ orion_spi_write_read_8bit(struct spi_device *spi,
 {
 	void __iomem *tx_reg, *rx_reg, *int_reg;
 	struct orion_spi *orion_spi;
+	bool cs_single_byte;
+
+	cs_single_byte = spi->mode & SPI_1BYTE_CS;
 
 	orion_spi = spi_master_get_devdata(spi->master);
+
+	if (cs_single_byte)
+		orion_spi_set_cs(spi, 0);
+
 	tx_reg = spi_reg(orion_spi, ORION_SPI_DATA_OUT_REG);
 	rx_reg = spi_reg(orion_spi, ORION_SPI_DATA_IN_REG);
 	int_reg = spi_reg(orion_spi, ORION_SPI_INT_CAUSE_REG);
@@ -384,6 +391,11 @@ orion_spi_write_read_8bit(struct spi_device *spi,
 		writel(0, tx_reg);
 
 	if (orion_spi_wait_till_ready(orion_spi) < 0) {
+		if (cs_single_byte) {
+			orion_spi_set_cs(spi, 1);
+			/* Satisfy some SLIC devices requirements */
+			udelay(4);
+		}
 		dev_err(&spi->dev, "TXS timed out\n");
 		return -1;
 	}
@@ -391,6 +403,12 @@ orion_spi_write_read_8bit(struct spi_device *spi,
 	if (rx_buf && *rx_buf)
 		*(*rx_buf)++ = readl(rx_reg);
 
+	if (cs_single_byte) {
+		orion_spi_set_cs(spi, 1);
+		/* Satisfy some SLIC devices requirements */
+		udelay(4);
+	}
+
 	return 1;
 }
 
@@ -626,7 +644,7 @@ static int orion_spi_probe(struct platform_device *pdev)
 	}
 
 	/* we support all 4 SPI modes and LSB first option */
-	master->mode_bits = SPI_CPHA | SPI_CPOL | SPI_LSB_FIRST;
+	master->mode_bits = SPI_CPHA | SPI_CPOL | SPI_LSB_FIRST | SPI_1BYTE_CS;
 	master->set_cs = orion_spi_set_cs;
 	master->transfer_one = orion_spi_transfer_one;
 	master->num_chipselect = ORION_NUM_CHIPSELECTS;
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 51d7c004fbab..998579807a04 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1937,6 +1937,8 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
 		spi->mode |= SPI_LSB_FIRST;
 	if (of_property_read_bool(nc, "spi-cs-high"))
 		spi->mode |= SPI_CS_HIGH;
+	if (of_find_property(nc, "spi-1byte-cs", NULL))
+		spi->mode |= SPI_1BYTE_CS;
 
 	/* Device DUAL/QUAD mode */
 	if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) {
@@ -3419,15 +3421,15 @@ int spi_setup(struct spi_device *spi)
 		spi_set_thread_rt(spi->controller);
 	}
 
-	dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%u bits/w, %u Hz max --> %d\n",
+	dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%s%u bits/w, %u Hz max --> %d\n",
 			(int) (spi->mode & (SPI_CPOL | SPI_CPHA)),
 			(spi->mode & SPI_CS_HIGH) ? "cs_high, " : "",
 			(spi->mode & SPI_LSB_FIRST) ? "lsb, " : "",
 			(spi->mode & SPI_3WIRE) ? "3wire, " : "",
 			(spi->mode & SPI_LOOP) ? "loopback, " : "",
+			(spi->mode & SPI_1BYTE_CS) ? "single_cs_byte, " : "",
 			spi->bits_per_word, spi->max_speed_hz,
 			status);
-
 	return status;
 }
 EXPORT_SYMBOL_GPL(spi_setup);
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index aa09fdc8042d..7f65ff6fc25d 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -186,6 +186,7 @@ struct spi_device {
 #define	SPI_TX_OCTAL	0x2000			/* transmit with 8 wires */
 #define	SPI_RX_OCTAL	0x4000			/* receive with 8 wires */
 #define	SPI_3WIRE_HIZ	0x8000			/* high impedance turnaround */
+#define	SPI_1BYTE_CS	0x10000			/* toggle cs after each byte */
 	int			irq;
 	void			*controller_state;
 	void			*controller_data;
-- 
2.17.1

[PATCH 3/3] dt-bindings: spi: add support for spi-1byte-cs

[kostap]

From: Konstantin Porotchkin <kostap@marvell.com>

add support for enable  switching CS every transferred byte

Signed-off-by: Konstantin Porotchkin <kostap@marvell.com>
---
 Documentation/devicetree/bindings/spi/spi-controller.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml
index 5f505810104d..e6102a29f3cc 100644
--- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
@@ -171,6 +171,11 @@ patternProperties:
         description:
           Delay, in microseconds, after a write transfer.
 
+      spi-1byte-cs:
+        $ref: /schemas/types.yaml#/definitions/flag
+        description:
+          The device requires toggling the CS for every 1 byte of data
+
     required:
       - compatible
       - reg
-- 
2.17.1

Re: [PATCH 1/3] spi: orion: enable clocks before spi_setup

[Mark Brown]

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

On Thu, Dec 17, 2020 at 01:27:06PM +0200, kostap@marvell.com wrote:
> each time the spi_setup() is called.
> 
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> Reviewed-by: Stefan Chulski <Stefan.Chulski@cavium.com>
> ---

You've not provided a Signed-off-by for this so I can't do anything with
it, please see Documentation/process/submitting-patches.rst for details
on what this is and why it's important.

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

Re: [PATCH 3/3] dt-bindings: spi: add support for spi-1byte-cs

[Mark Brown]

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

On Thu, Dec 17, 2020 at 01:27:08PM +0200, kostap@marvell.com wrote:

> +      spi-1byte-cs:
> +        $ref: /schemas/types.yaml#/definitions/flag
> +        description:
> +          The device requires toggling the CS for every 1 byte of data

This is absolutely not something that should be in the DT, if the device
requires this we know that simply from the compatible string.

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

Re: [PATCH 2/3] spi: orion: enable support for switching CS every transferred byte

[Marcin Wojtas]

Hi Mark,

czw., 17 gru 2020 o 12:27 <kostap@marvell.com> napisał(a):
>
> From: Marcin Wojtas <mw@semihalf.com>
>
> Some SPI devices, such as SLIC (Subscriber Line Interface Card)
> require toggling the CS every transferred byte. Enable such
> possibility by creating a new DT property and enabling SPI
> device mode update. Add according support in the spi-orion driver.
>
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> Signed-off-by: Konstantin Porotchkin <kostap@marvell.com>
> ---
>  drivers/spi/spi-orion.c | 20 +++++++++++++++++++-
>  drivers/spi/spi.c       |  6 ++++--
>  include/linux/spi/spi.h |  1 +
>  3 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c
> index 3bfda4225d45..7db9034b0879 100644
> --- a/drivers/spi/spi-orion.c
> +++ b/drivers/spi/spi-orion.c
> @@ -369,8 +369,15 @@ orion_spi_write_read_8bit(struct spi_device *spi,
>  {
>         void __iomem *tx_reg, *rx_reg, *int_reg;
>         struct orion_spi *orion_spi;
> +       bool cs_single_byte;
> +
> +       cs_single_byte = spi->mode & SPI_1BYTE_CS;
>
>         orion_spi = spi_master_get_devdata(spi->master);
> +
> +       if (cs_single_byte)
> +               orion_spi_set_cs(spi, 0);
> +
>         tx_reg = spi_reg(orion_spi, ORION_SPI_DATA_OUT_REG);
>         rx_reg = spi_reg(orion_spi, ORION_SPI_DATA_IN_REG);
>         int_reg = spi_reg(orion_spi, ORION_SPI_INT_CAUSE_REG);
> @@ -384,6 +391,11 @@ orion_spi_write_read_8bit(struct spi_device *spi,
>                 writel(0, tx_reg);
>
>         if (orion_spi_wait_till_ready(orion_spi) < 0) {
> +               if (cs_single_byte) {
> +                       orion_spi_set_cs(spi, 1);
> +                       /* Satisfy some SLIC devices requirements */
> +                       udelay(4);
> +               }
>                 dev_err(&spi->dev, "TXS timed out\n");
>                 return -1;
>         }
> @@ -391,6 +403,12 @@ orion_spi_write_read_8bit(struct spi_device *spi,
>         if (rx_buf && *rx_buf)
>                 *(*rx_buf)++ = readl(rx_reg);
>
> +       if (cs_single_byte) {
> +               orion_spi_set_cs(spi, 1);
> +               /* Satisfy some SLIC devices requirements */
> +               udelay(4);
> +       }
> +
>         return 1;
>  }
>
> @@ -626,7 +644,7 @@ static int orion_spi_probe(struct platform_device *pdev)
>         }
>
>         /* we support all 4 SPI modes and LSB first option */
> -       master->mode_bits = SPI_CPHA | SPI_CPOL | SPI_LSB_FIRST;
> +       master->mode_bits = SPI_CPHA | SPI_CPOL | SPI_LSB_FIRST | SPI_1BYTE_CS;
>         master->set_cs = orion_spi_set_cs;
>         master->transfer_one = orion_spi_transfer_one;
>         master->num_chipselect = ORION_NUM_CHIPSELECTS;
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 51d7c004fbab..998579807a04 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1937,6 +1937,8 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
>                 spi->mode |= SPI_LSB_FIRST;
>         if (of_property_read_bool(nc, "spi-cs-high"))
>                 spi->mode |= SPI_CS_HIGH;
> +       if (of_find_property(nc, "spi-1byte-cs", NULL))
> +               spi->mode |= SPI_1BYTE_CS;

Regarding your comment from patch 3/3 that "spi-1byte-cs" should be
replaced by handling based on the compatible string - do you mean
dropping above parsing and updating SPI bus mode field with
SPI_1BYTE_CS flag in the relevant SPI device driver?

Best regards,
Marcin

>
>         /* Device DUAL/QUAD mode */
>         if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) {
> @@ -3419,15 +3421,15 @@ int spi_setup(struct spi_device *spi)
>                 spi_set_thread_rt(spi->controller);
>         }
>
> -       dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%u bits/w, %u Hz max --> %d\n",
> +       dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%s%u bits/w, %u Hz max --> %d\n",
>                         (int) (spi->mode & (SPI_CPOL | SPI_CPHA)),
>                         (spi->mode & SPI_CS_HIGH) ? "cs_high, " : "",
>                         (spi->mode & SPI_LSB_FIRST) ? "lsb, " : "",
>                         (spi->mode & SPI_3WIRE) ? "3wire, " : "",
>                         (spi->mode & SPI_LOOP) ? "loopback, " : "",
> +                       (spi->mode & SPI_1BYTE_CS) ? "single_cs_byte, " : "",
>                         spi->bits_per_word, spi->max_speed_hz,
>                         status);
> -
>         return status;
>  }
>  EXPORT_SYMBOL_GPL(spi_setup);
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index aa09fdc8042d..7f65ff6fc25d 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -186,6 +186,7 @@ struct spi_device {
>  #define        SPI_TX_OCTAL    0x2000                  /* transmit with 8 wires */
>  #define        SPI_RX_OCTAL    0x4000                  /* receive with 8 wires */
>  #define        SPI_3WIRE_HIZ   0x8000                  /* high impedance turnaround */
> +#define        SPI_1BYTE_CS    0x10000                 /* toggle cs after each byte */
>         int                     irq;
>         void                    *controller_state;
>         void                    *controller_data;
> --
> 2.17.1
>

RE: [EXT] Re: [PATCH 1/3] spi: orion: enable clocks before spi_setup

[Kostya Porotchkin]

Hello, Mark,

> > each time the spi_setup() is called.
> >
> > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > Reviewed-by: Stefan Chulski <Stefan.Chulski@cavium.com>
> > ---
> 
> You've not provided a Signed-off-by for this so I can't do anything with it,
> please see Documentation/process/submitting-patches.rst for details on
> what this is and why it's important.
[KP] Sorry about it, will fix in the next version
Kosta

Re: [PATCH 2/3] spi: orion: enable support for switching CS every transferred byte

[Mark Brown]

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

On Thu, Dec 17, 2020 at 01:27:07PM +0200, kostap@marvell.com wrote:

> Some SPI devices, such as SLIC (Subscriber Line Interface Card)
> require toggling the CS every transferred byte. Enable such
> possibility by creating a new DT property and enabling SPI
> device mode update. Add according support in the spi-orion driver.

I'm pretty sure we already support this - if the client driver sets the
word length to 8 bits then SPI_CS_WORD ought to do what your change
describes as far as I can see.  What's missing there?

> ---
>  drivers/spi/spi-orion.c | 20 +++++++++++++++++++-
>  drivers/spi/spi.c       |  6 ++++--
>  include/linux/spi/spi.h |  1 +
>  3 files changed, 24 insertions(+), 3 deletions(-)

This is introducing something into the core and adding a user of it, it
should be two separate patches.

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

Re: [PATCH 2/3] spi: orion: enable support for switching CS every transferred byte

[Mark Brown]

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

On Thu, Dec 17, 2020 at 02:56:16PM +0100, Marcin Wojtas wrote:

> Regarding your comment from patch 3/3 that "spi-1byte-cs" should be
> replaced by handling based on the compatible string - do you mean
> dropping above parsing and updating SPI bus mode field with
> SPI_1BYTE_CS flag in the relevant SPI device driver?

There's more to it than that but like I said in reply to the patch
AFAICT we already support this.

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

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

Re: [PATCH 2/3] spi: orion: enable support for switching CS every transferred byte

[Marcin Wojtas]

czw., 17 gru 2020 o 15:15 Mark Brown <broonie@kernel.org> napisał(a):
>
> On Thu, Dec 17, 2020 at 01:27:07PM +0200, kostap@marvell.com wrote:
>
> > Some SPI devices, such as SLIC (Subscriber Line Interface Card)
> > require toggling the CS every transferred byte. Enable such
> > possibility by creating a new DT property and enabling SPI
> > device mode update. Add according support in the spi-orion driver.
>
> I'm pretty sure we already support this - if the client driver sets the
> word length to 8 bits then SPI_CS_WORD ought to do what your change
> describes as far as I can see.  What's missing there?

Sure, that needs to be confirmed on HW, but it looks like the required
functionality can be handled with SPI_CS_WORD.

As for the justification, the new flag was developed on a kernel
revision without it and applied easily, hence the possible oversight.

>
> > ---
> >  drivers/spi/spi-orion.c | 20 +++++++++++++++++++-
> >  drivers/spi/spi.c       |  6 ++++--
> >  include/linux/spi/spi.h |  1 +
> >  3 files changed, 24 insertions(+), 3 deletions(-)
>
> This is introducing something into the core and adding a user of it, it
> should be two separate patches.

Agree.

Thanks,
Marcin

Re: [PATCH 2/3] spi: orion: enable support for switching CS every transferred byte

[Marcin Wojtas]

czw., 17 gru 2020 o 15:21 Mark Brown <broonie@kernel.org> napisał(a):
>
> On Thu, Dec 17, 2020 at 02:56:16PM +0100, Marcin Wojtas wrote:
>
> > Regarding your comment from patch 3/3 that "spi-1byte-cs" should be
> > replaced by handling based on the compatible string - do you mean
> > dropping above parsing and updating SPI bus mode field with
> > SPI_1BYTE_CS flag in the relevant SPI device driver?
>
> There's more to it than that but like I said in reply to the patch
> AFAICT we already support this.
>
> Please delete unneeded context from mails when replying.  Doing this
> makes it much easier to find your reply in the message, helping ensure
> it won't be missed by people scrolling through the irrelevant quoted
> material.

Sure, thanks.

Marcin