[PATCH 0/4] spi: s3c64xx: add support for google,gs101-spi

[PATCH 0/4] spi: s3c64xx: add support for google,gs101-spi

[Tudor Ambarus]

Depends on the simple cleanup patches from:
https://lore.kernel.org/linux-spi/20240205124513.447875-1-tudor.ambarus@linaro.org/

A slightly different version of the google,gs101-spi support was sent at:
https://lore.kernel.org/linux-spi/20240125145007.748295-1-tudor.ambarus@linaro.org/

Let's add support for gs101-spi so that I have a testing base for the
driver rework patches that will follow.

Tudor Ambarus (4):
  spi: s3c64xx: explicitly include <linux/types.h>
  spi: dt-bindings: samsung: add google,gs101-spi compatible
  spi: s3c64xx: add s3c64xx_iowrite{8,16}_32_rep accessors
  spi: s3c64xx: add support for google,gs101-spi

 .../devicetree/bindings/spi/samsung,spi.yaml  |  1 +
 drivers/spi/spi-s3c64xx.c                     | 89 +++++++++++++++----
 2 files changed, 75 insertions(+), 15 deletions(-)

-- 
2.43.0.594.gd9cf4e227d-goog

[PATCH 1/4] spi: s3c64xx: explicitly include <linux/types.h>

[Tudor Ambarus]

The driver uses u32 and relies on an implicit inclusion of
<linux/types.h>.

It is good practice to directly include all headers used, it avoids
implicit dependencies and spurious breakage if someone rearranges
headers and causes the implicit include to vanish.

Include the missing header.

Fixes: 230d42d422e7 ("spi: Add s3c64xx SPI Controller driver")
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/spi/spi-s3c64xx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 72c35dbe53b2..c15ca6a910dc 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -17,6 +17,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/spi/spi.h>
+#include <linux/types.h>
 
 #define MAX_SPI_PORTS		12
 #define S3C64XX_SPI_QUIRK_CS_AUTO	(1 << 1)
-- 
2.43.0.594.gd9cf4e227d-goog

Re: [PATCH 1/4] spi: s3c64xx: explicitly include <linux/types.h>

[Peter Griffin]

On Tue, 6 Feb 2024 at 08:52, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> The driver uses u32 and relies on an implicit inclusion of
> <linux/types.h>.
>
> It is good practice to directly include all headers used, it avoids
> implicit dependencies and spurious breakage if someone rearranges
> headers and causes the implicit include to vanish.
>
> Include the missing header.
>
> Fixes: 230d42d422e7 ("spi: Add s3c64xx SPI Controller driver")
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

Reviewed-by: Peter Griffin <peter.griffin@linaro.org>

>  drivers/spi/spi-s3c64xx.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 72c35dbe53b2..c15ca6a910dc 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -17,6 +17,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/spi/spi.h>
> +#include <linux/types.h>
>
>  #define MAX_SPI_PORTS          12
>  #define S3C64XX_SPI_QUIRK_CS_AUTO      (1 << 1)
> --
> 2.43.0.594.gd9cf4e227d-goog
>

Re: [PATCH 1/4] spi: s3c64xx: explicitly include <linux/types.h>

[Sam Protsenko]

On Tue, Feb 6, 2024 at 2:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> The driver uses u32 and relies on an implicit inclusion of
> <linux/types.h>.
>
> It is good practice to directly include all headers used, it avoids
> implicit dependencies and spurious breakage if someone rearranges
> headers and causes the implicit include to vanish.
>
> Include the missing header.
>
> Fixes: 230d42d422e7 ("spi: Add s3c64xx SPI Controller driver")

Not sure if Fixes tag is needed here.

> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
>  drivers/spi/spi-s3c64xx.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 72c35dbe53b2..c15ca6a910dc 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -17,6 +17,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/spi/spi.h>
> +#include <linux/types.h>

Is this really needed for the further patches in this series?

>
>  #define MAX_SPI_PORTS          12
>  #define S3C64XX_SPI_QUIRK_CS_AUTO      (1 << 1)
> --
> 2.43.0.594.gd9cf4e227d-goog
>

Re: [PATCH 1/4] spi: s3c64xx: explicitly include <linux/types.h>

[Tudor Ambarus]

On 2/6/24 18:02, Sam Protsenko wrote:
> On Tue, Feb 6, 2024 at 2:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>> The driver uses u32 and relies on an implicit inclusion of
>> <linux/types.h>.
>>
>> It is good practice to directly include all headers used, it avoids
>> implicit dependencies and spurious breakage if someone rearranges
>> headers and causes the implicit include to vanish.
>>
>> Include the missing header.
>>
>> Fixes: 230d42d422e7 ("spi: Add s3c64xx SPI Controller driver")
> 
> Not sure if Fixes tag is needed here.

We have already talked about this. If a patch that causes the implicit
include to vanish is backported to stable, it will reveal the missing
header here and will result in backporting this patch as well. So, as a
good practice let's allow this patch to be queued to stable, it will
avoid possible compilation errors.

I guess Mark has to break the tie again. Or Greg if he cares, I added
him in To:.

> 
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>> ---
>>  drivers/spi/spi-s3c64xx.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> index 72c35dbe53b2..c15ca6a910dc 100644
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/spi/spi.h>
>> +#include <linux/types.h>
> 
> Is this really needed for the further patches in this series?
> 

Yes, because in patch 3/4 I use u8 and u16 and I don't want to use those
without having the header included. Do you find this wrong?

>>
>>  #define MAX_SPI_PORTS          12
>>  #define S3C64XX_SPI_QUIRK_CS_AUTO      (1 << 1)
>> --
>> 2.43.0.594.gd9cf4e227d-goog
>>

Re: [PATCH 1/4] spi: s3c64xx: explicitly include <linux/types.h>

[Greg Kroah-Hartman]

On Wed, Feb 07, 2024 at 06:20:56AM +0000, Tudor Ambarus wrote:
> 
> 
> On 2/6/24 18:02, Sam Protsenko wrote:
> > On Tue, Feb 6, 2024 at 2:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> >>
> >> The driver uses u32 and relies on an implicit inclusion of
> >> <linux/types.h>.
> >>
> >> It is good practice to directly include all headers used, it avoids
> >> implicit dependencies and spurious breakage if someone rearranges
> >> headers and causes the implicit include to vanish.
> >>
> >> Include the missing header.
> >>
> >> Fixes: 230d42d422e7 ("spi: Add s3c64xx SPI Controller driver")
> > 
> > Not sure if Fixes tag is needed here.
> 
> We have already talked about this. If a patch that causes the implicit
> include to vanish is backported to stable, it will reveal the missing
> header here and will result in backporting this patch as well.

So worry about this then, not now.

> So, as a
> good practice let's allow this patch to be queued to stable, it will
> avoid possible compilation errors.

"possible" does not mean "needed".

Please only tag stuff that you know is needed, if the stable developers
break things, they can fix it up when it happens.

Adding .h includes is not a fixes: thing unless it actually fixes
something in Linus's tree, otherwise it's an abuse of the tag.  Please
don't do that.

thanks,

greg k-h

Re: [PATCH 1/4] spi: s3c64xx: explicitly include <linux/types.h>

[Tudor Ambarus]

On 2/7/24 09:58, Greg Kroah-Hartman wrote:
> On Wed, Feb 07, 2024 at 06:20:56AM +0000, Tudor Ambarus wrote:
>>
>>
>> On 2/6/24 18:02, Sam Protsenko wrote:
>>> On Tue, Feb 6, 2024 at 2:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>>>
>>>> The driver uses u32 and relies on an implicit inclusion of
>>>> <linux/types.h>.
>>>>
>>>> It is good practice to directly include all headers used, it avoids
>>>> implicit dependencies and spurious breakage if someone rearranges
>>>> headers and causes the implicit include to vanish.
>>>>
>>>> Include the missing header.
>>>>
>>>> Fixes: 230d42d422e7 ("spi: Add s3c64xx SPI Controller driver")
>>>
>>> Not sure if Fixes tag is needed here.
>>
>> We have already talked about this. If a patch that causes the implicit
>> include to vanish is backported to stable, it will reveal the missing
>> header here and will result in backporting this patch as well.
> 
> So worry about this then, not now.
> 
>> So, as a
>> good practice let's allow this patch to be queued to stable, it will
>> avoid possible compilation errors.
> 
> "possible" does not mean "needed".
> 
> Please only tag stuff that you know is needed, if the stable developers
> break things, they can fix it up when it happens.
> 
> Adding .h includes is not a fixes: thing unless it actually fixes
> something in Linus's tree, otherwise it's an abuse of the tag.  Please
> don't do that.
> 

Okay, thanks for the clarification.

Cheers,
ta

Re: [PATCH 1/4] spi: s3c64xx: explicitly include <linux/types.h>

[Sam Protsenko]

On Wed, Feb 7, 2024 at 12:21 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
>
>
> On 2/6/24 18:02, Sam Protsenko wrote:
> > On Tue, Feb 6, 2024 at 2:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> >>
> >> The driver uses u32 and relies on an implicit inclusion of
> >> <linux/types.h>.
> >>
> >> It is good practice to directly include all headers used, it avoids
> >> implicit dependencies and spurious breakage if someone rearranges
> >> headers and causes the implicit include to vanish.
> >>
> >> Include the missing header.
> >>
> >> Fixes: 230d42d422e7 ("spi: Add s3c64xx SPI Controller driver")
> >
> > Not sure if Fixes tag is needed here.
>
> We have already talked about this. If a patch that causes the implicit
> include to vanish is backported to stable, it will reveal the missing
> header here and will result in backporting this patch as well. So, as a
> good practice let's allow this patch to be queued to stable, it will
> avoid possible compilation errors.
>
> I guess Mark has to break the tie again. Or Greg if he cares, I added
> him in To:.
>
> >
> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> >> ---
> >>  drivers/spi/spi-s3c64xx.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> >> index 72c35dbe53b2..c15ca6a910dc 100644
> >> --- a/drivers/spi/spi-s3c64xx.c
> >> +++ b/drivers/spi/spi-s3c64xx.c
> >> @@ -17,6 +17,7 @@
> >>  #include <linux/platform_device.h>
> >>  #include <linux/pm_runtime.h>
> >>  #include <linux/spi/spi.h>
> >> +#include <linux/types.h>
> >
> > Is this really needed for the further patches in this series?
> >
>
> Yes, because in patch 3/4 I use u8 and u16 and I don't want to use those
> without having the header included. Do you find this wrong?
>

I'd say if this header is really needed for your patch 3/4 to have a
successful build, just merge this patch into the patch 3/4 then.

> >>
> >>  #define MAX_SPI_PORTS          12
> >>  #define S3C64XX_SPI_QUIRK_CS_AUTO      (1 << 1)
> >> --
> >> 2.43.0.594.gd9cf4e227d-goog
> >>

Re: [PATCH 1/4] spi: s3c64xx: explicitly include <linux/types.h>

[Mark Brown]

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

On Wed, Feb 07, 2024 at 09:42:20AM -0600, Sam Protsenko wrote:
> On Wed, Feb 7, 2024 at 12:21 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:

> > Yes, because in patch 3/4 I use u8 and u16 and I don't want to use those
> > without having the header included. Do you find this wrong?

> I'd say if this header is really needed for your patch 3/4 to have a
> successful build, just merge this patch into the patch 3/4 then.

The series was already resent, not worth a new version just for this...

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

[PATCH 2/4] spi: dt-bindings: samsung: add google,gs101-spi compatible

[Tudor Ambarus]

Add "google,gs101-spi" dedicated compatible for representing SPI of
Google GS101 SoC.

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Acked-by: Andi Shyti <andi.shyti@kernel.org>
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 Documentation/devicetree/bindings/spi/samsung,spi.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/spi/samsung,spi.yaml b/Documentation/devicetree/bindings/spi/samsung,spi.yaml
index f71099852653..2f0a0835ecfb 100644
--- a/Documentation/devicetree/bindings/spi/samsung,spi.yaml
+++ b/Documentation/devicetree/bindings/spi/samsung,spi.yaml
@@ -17,6 +17,7 @@ properties:
   compatible:
     oneOf:
       - enum:
+          - google,gs101-spi
           - samsung,s3c2443-spi # for S3C2443, S3C2416 and S3C2450
           - samsung,s3c6410-spi
           - samsung,s5pv210-spi # for S5PV210 and S5PC110
-- 
2.43.0.594.gd9cf4e227d-goog

Re: [PATCH 2/4] spi: dt-bindings: samsung: add google,gs101-spi compatible

[Peter Griffin]

On Tue, 6 Feb 2024 at 08:52, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Add "google,gs101-spi" dedicated compatible for representing SPI of
> Google GS101 SoC.
>
> Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Acked-by: Andi Shyti <andi.shyti@kernel.org>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

Reviewed-by: Peter Griffin <peter.griffin@linaro.org>

>  Documentation/devicetree/bindings/spi/samsung,spi.yaml | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/spi/samsung,spi.yaml b/Documentation/devicetree/bindings/spi/samsung,spi.yaml
> index f71099852653..2f0a0835ecfb 100644
> --- a/Documentation/devicetree/bindings/spi/samsung,spi.yaml
> +++ b/Documentation/devicetree/bindings/spi/samsung,spi.yaml
> @@ -17,6 +17,7 @@ properties:
>    compatible:
>      oneOf:
>        - enum:
> +          - google,gs101-spi
>            - samsung,s3c2443-spi # for S3C2443, S3C2416 and S3C2450
>            - samsung,s3c6410-spi
>            - samsung,s5pv210-spi # for S5PV210 and S5PC110
> --
> 2.43.0.594.gd9cf4e227d-goog
>

[PATCH 3/4] spi: s3c64xx: add s3c64xx_iowrite{8,16}_32_rep accessors

[Tudor Ambarus]

Allow SoCs that require 32 bits register accesses to write data in
chunks of 8 or 16 bits. One SoC that requires 32 bit register accesses
is the google gs101. The operation is rare, thus open code it in the
driver rather than making it generic (through asm-generic/io.h)

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/spi/spi-s3c64xx.c | 70 +++++++++++++++++++++++++++++++--------
 1 file changed, 56 insertions(+), 14 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index c15ca6a910dc..cb45ad615f3d 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -142,6 +142,7 @@ struct s3c64xx_spi_dma_data {
  *	prescaler unit.
  * @clk_ioclk: True if clock is present on this device
  * @has_loopback: True if loopback mode can be supported
+ * @use_32bit_io: True if the SoC allows just 32-bit register accesses.
  *
  * The Samsung s3c64xx SPI controller are used on various Samsung SoC's but
  * differ in some aspects such as the size of the fifo and spi bus clock
@@ -158,6 +159,7 @@ struct s3c64xx_spi_port_config {
 	bool	clk_from_cmu;
 	bool	clk_ioclk;
 	bool	has_loopback;
+	bool	use_32bit_io;
 };
 
 /**
@@ -412,6 +414,59 @@ static bool s3c64xx_spi_can_dma(struct spi_controller *host,
 	return false;
 }
 
+static void s3c64xx_iowrite8_32_rep(volatile void __iomem *addr,
+				    const void *buffer, unsigned int count)
+{
+	if (count) {
+		const u8 *buf = buffer;
+
+		do {
+			__raw_writel(*buf++, addr);
+		} while (--count);
+	}
+}
+
+static void s3c64xx_iowrite16_32_rep(volatile void __iomem *addr,
+				     const void *buffer, unsigned int count)
+{
+	if (count) {
+		const u16 *buf = buffer;
+
+		do {
+			__raw_writel(*buf++, addr);
+		} while (--count);
+	}
+}
+
+static void s3c64xx_iowrite_rep(const struct s3c64xx_spi_driver_data *sdd,
+				struct spi_transfer *xfer)
+{
+	void __iomem *regs = sdd->regs;
+
+	switch (sdd->cur_bpw) {
+	case 32:
+		iowrite32_rep(regs + S3C64XX_SPI_TX_DATA,
+			      xfer->tx_buf, xfer->len / 4);
+		break;
+	case 16:
+		if (sdd->port_conf->use_32bit_io)
+			s3c64xx_iowrite16_32_rep(regs + S3C64XX_SPI_TX_DATA,
+						 xfer->tx_buf, xfer->len / 2);
+		else
+			iowrite16_rep(regs + S3C64XX_SPI_TX_DATA,
+				      xfer->tx_buf, xfer->len / 2);
+		break;
+	default:
+		if (sdd->port_conf->use_32bit_io)
+			s3c64xx_iowrite8_32_rep(regs + S3C64XX_SPI_TX_DATA,
+						xfer->tx_buf, xfer->len);
+		else
+			iowrite8_rep(regs + S3C64XX_SPI_TX_DATA,
+				     xfer->tx_buf, xfer->len);
+		break;
+	}
+}
+
 static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 				    struct spi_transfer *xfer, int dma_mode)
 {
@@ -445,20 +500,7 @@ static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 			modecfg |= S3C64XX_SPI_MODE_TXDMA_ON;
 			ret = s3c64xx_prepare_dma(&sdd->tx_dma, &xfer->tx_sg);
 		} else {
-			switch (sdd->cur_bpw) {
-			case 32:
-				iowrite32_rep(regs + S3C64XX_SPI_TX_DATA,
-					xfer->tx_buf, xfer->len / 4);
-				break;
-			case 16:
-				iowrite16_rep(regs + S3C64XX_SPI_TX_DATA,
-					xfer->tx_buf, xfer->len / 2);
-				break;
-			default:
-				iowrite8_rep(regs + S3C64XX_SPI_TX_DATA,
-					xfer->tx_buf, xfer->len);
-				break;
-			}
+			s3c64xx_iowrite_rep(sdd, xfer);
 		}
 	}
 
-- 
2.43.0.594.gd9cf4e227d-goog

Re: [PATCH 3/4] spi: s3c64xx: add s3c64xx_iowrite{8,16}_32_rep accessors

[Sam Protsenko]

On Tue, Feb 6, 2024 at 2:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Allow SoCs that require 32 bits register accesses to write data in
> chunks of 8 or 16 bits. One SoC that requires 32 bit register accesses
> is the google gs101. The operation is rare, thus open code it in the
> driver rather than making it generic (through asm-generic/io.h)
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
>  drivers/spi/spi-s3c64xx.c | 70 +++++++++++++++++++++++++++++++--------
>  1 file changed, 56 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index c15ca6a910dc..cb45ad615f3d 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -142,6 +142,7 @@ struct s3c64xx_spi_dma_data {
>   *     prescaler unit.
>   * @clk_ioclk: True if clock is present on this device
>   * @has_loopback: True if loopback mode can be supported
> + * @use_32bit_io: True if the SoC allows just 32-bit register accesses.

A matter of taste, but: just -> only.

>   *
>   * The Samsung s3c64xx SPI controller are used on various Samsung SoC's but
>   * differ in some aspects such as the size of the fifo and spi bus clock
> @@ -158,6 +159,7 @@ struct s3c64xx_spi_port_config {
>         bool    clk_from_cmu;
>         bool    clk_ioclk;
>         bool    has_loopback;
> +       bool    use_32bit_io;
>  };
>
>  /**
> @@ -412,6 +414,59 @@ static bool s3c64xx_spi_can_dma(struct spi_controller *host,
>         return false;
>  }
>
> +static void s3c64xx_iowrite8_32_rep(volatile void __iomem *addr,
> +                                   const void *buffer, unsigned int count)
> +{
> +       if (count) {
> +               const u8 *buf = buffer;
> +
> +               do {
> +                       __raw_writel(*buf++, addr);
> +               } while (--count);
> +       }

How about:

    while (count--)
        __raw_writel(*buf++, addr);

This way "if" condition is not needed. The same goes for the function below.

> +}
> +
> +static void s3c64xx_iowrite16_32_rep(volatile void __iomem *addr,
> +                                    const void *buffer, unsigned int count)
> +{
> +       if (count) {
> +               const u16 *buf = buffer;
> +
> +               do {
> +                       __raw_writel(*buf++, addr);
> +               } while (--count);
> +       }
> +}
> +
> +static void s3c64xx_iowrite_rep(const struct s3c64xx_spi_driver_data *sdd,
> +                               struct spi_transfer *xfer)
> +{
> +       void __iomem *regs = sdd->regs;

Suggest declaring aliases here, like this:

    void __iomem *addr = sdd->regs + S3C64XX_SPI_TX_DATA;
    const void *buf = xfer->tx_buf;
    unsigned int len = xfer->len;

Using those in the code below makes it more compact and easier to read.

> +
> +       switch (sdd->cur_bpw) {
> +       case 32:
> +               iowrite32_rep(regs + S3C64XX_SPI_TX_DATA,
> +                             xfer->tx_buf, xfer->len / 4);
> +               break;
> +       case 16:
> +               if (sdd->port_conf->use_32bit_io)
> +                       s3c64xx_iowrite16_32_rep(regs + S3C64XX_SPI_TX_DATA,
> +                                                xfer->tx_buf, xfer->len / 2);
> +               else
> +                       iowrite16_rep(regs + S3C64XX_SPI_TX_DATA,
> +                                     xfer->tx_buf, xfer->len / 2);
> +               break;
> +       default:
> +               if (sdd->port_conf->use_32bit_io)
> +                       s3c64xx_iowrite8_32_rep(regs + S3C64XX_SPI_TX_DATA,
> +                                               xfer->tx_buf, xfer->len);
> +               else
> +                       iowrite8_rep(regs + S3C64XX_SPI_TX_DATA,
> +                                    xfer->tx_buf, xfer->len);
> +               break;
> +       }
> +}
> +
>  static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
>                                     struct spi_transfer *xfer, int dma_mode)
>  {
> @@ -445,20 +500,7 @@ static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
>                         modecfg |= S3C64XX_SPI_MODE_TXDMA_ON;
>                         ret = s3c64xx_prepare_dma(&sdd->tx_dma, &xfer->tx_sg);
>                 } else {
> -                       switch (sdd->cur_bpw) {
> -                       case 32:
> -                               iowrite32_rep(regs + S3C64XX_SPI_TX_DATA,
> -                                       xfer->tx_buf, xfer->len / 4);
> -                               break;
> -                       case 16:
> -                               iowrite16_rep(regs + S3C64XX_SPI_TX_DATA,
> -                                       xfer->tx_buf, xfer->len / 2);
> -                               break;
> -                       default:
> -                               iowrite8_rep(regs + S3C64XX_SPI_TX_DATA,
> -                                       xfer->tx_buf, xfer->len);
> -                               break;
> -                       }
> +                       s3c64xx_iowrite_rep(sdd, xfer);

This extraction (with no functional change yet) could've been a
preceding separate patch, preparing things for this rework.

>                 }
>         }
>
> --
> 2.43.0.594.gd9cf4e227d-goog
>

Re: [PATCH 3/4] spi: s3c64xx: add s3c64xx_iowrite{8,16}_32_rep accessors

[Tudor Ambarus]

On 2/6/24 18:44, Sam Protsenko wrote:
> On Tue, Feb 6, 2024 at 2:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>> Allow SoCs that require 32 bits register accesses to write data in
>> chunks of 8 or 16 bits. One SoC that requires 32 bit register accesses
>> is the google gs101. The operation is rare, thus open code it in the
>> driver rather than making it generic (through asm-generic/io.h)
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>> ---
>>  drivers/spi/spi-s3c64xx.c | 70 +++++++++++++++++++++++++++++++--------
>>  1 file changed, 56 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> index c15ca6a910dc..cb45ad615f3d 100644
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -142,6 +142,7 @@ struct s3c64xx_spi_dma_data {
>>   *     prescaler unit.
>>   * @clk_ioclk: True if clock is present on this device
>>   * @has_loopback: True if loopback mode can be supported
>> + * @use_32bit_io: True if the SoC allows just 32-bit register accesses.
> 
> A matter of taste, but: just -> only.

ok, will change if I send a new version.
> 
>>   *
>>   * The Samsung s3c64xx SPI controller are used on various Samsung SoC's but
>>   * differ in some aspects such as the size of the fifo and spi bus clock
>> @@ -158,6 +159,7 @@ struct s3c64xx_spi_port_config {
>>         bool    clk_from_cmu;
>>         bool    clk_ioclk;
>>         bool    has_loopback;
>> +       bool    use_32bit_io;
>>  };
>>
>>  /**
>> @@ -412,6 +414,59 @@ static bool s3c64xx_spi_can_dma(struct spi_controller *host,
>>         return false;
>>  }
>>
>> +static void s3c64xx_iowrite8_32_rep(volatile void __iomem *addr,
>> +                                   const void *buffer, unsigned int count)
>> +{
>> +       if (count) {
>> +               const u8 *buf = buffer;
>> +
>> +               do {
>> +                       __raw_writel(*buf++, addr);
>> +               } while (--count);
>> +       }
> 
> How about:
> 
>     while (count--)
>         __raw_writel(*buf++, addr);
> 
> This way "if" condition is not needed. The same goes for the function below.

count will overflow, but shouldn't matter as it's not used afterwards.
But I would keep the style as it is, if you don't mind, it follows other
similar implementations from include/asm-generic/io.h. I'd like to be
consistent with the coding style of the generic implementations.

> 
>> +}
>> +
>> +static void s3c64xx_iowrite16_32_rep(volatile void __iomem *addr,
>> +                                    const void *buffer, unsigned int count)
>> +{
>> +       if (count) {
>> +               const u16 *buf = buffer;
>> +
>> +               do {
>> +                       __raw_writel(*buf++, addr);
>> +               } while (--count);
>> +       }
>> +}
>> +
>> +static void s3c64xx_iowrite_rep(const struct s3c64xx_spi_driver_data *sdd,
>> +                               struct spi_transfer *xfer)
>> +{
>> +       void __iomem *regs = sdd->regs;
> 
> Suggest declaring aliases here, like this:
> 
>     void __iomem *addr = sdd->regs + S3C64XX_SPI_TX_DATA;
>     const void *buf = xfer->tx_buf;
>     unsigned int len = xfer->len;
> 
> Using those in the code below makes it more compact and easier to read.

Ok, will add the local variables if I send again. I hope you're fine
with adding the local variables when introducing the method.
> 
>> +
>> +       switch (sdd->cur_bpw) {
>> +       case 32:
>> +               iowrite32_rep(regs + S3C64XX_SPI_TX_DATA,
>> +                             xfer->tx_buf, xfer->len / 4);
>> +               break;
>> +       case 16:
>> +               if (sdd->port_conf->use_32bit_io)
>> +                       s3c64xx_iowrite16_32_rep(regs + S3C64XX_SPI_TX_DATA,
>> +                                                xfer->tx_buf, xfer->len / 2);
>> +               else
>> +                       iowrite16_rep(regs + S3C64XX_SPI_TX_DATA,
>> +                                     xfer->tx_buf, xfer->len / 2);
>> +               break;
>> +       default:
>> +               if (sdd->port_conf->use_32bit_io)
>> +                       s3c64xx_iowrite8_32_rep(regs + S3C64XX_SPI_TX_DATA,
>> +                                               xfer->tx_buf, xfer->len);
>> +               else
>> +                       iowrite8_rep(regs + S3C64XX_SPI_TX_DATA,
>> +                                    xfer->tx_buf, xfer->len);
>> +               break;
>> +       }
>> +}
>> +
>>  static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
>>                                     struct spi_transfer *xfer, int dma_mode)
>>  {
>> @@ -445,20 +500,7 @@ static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
>>                         modecfg |= S3C64XX_SPI_MODE_TXDMA_ON;
>>                         ret = s3c64xx_prepare_dma(&sdd->tx_dma, &xfer->tx_sg);
>>                 } else {
>> -                       switch (sdd->cur_bpw) {
>> -                       case 32:
>> -                               iowrite32_rep(regs + S3C64XX_SPI_TX_DATA,
>> -                                       xfer->tx_buf, xfer->len / 4);
>> -                               break;
>> -                       case 16:
>> -                               iowrite16_rep(regs + S3C64XX_SPI_TX_DATA,
>> -                                       xfer->tx_buf, xfer->len / 2);
>> -                               break;
>> -                       default:
>> -                               iowrite8_rep(regs + S3C64XX_SPI_TX_DATA,
>> -                                       xfer->tx_buf, xfer->len);
>> -                               break;
>> -                       }
>> +                       s3c64xx_iowrite_rep(sdd, xfer);
> 
> This extraction (with no functional change yet) could've been a
> preceding separate patch, preparing things for this rework.
> 

I'm not a fan of having preparation patches for small changes like this,
it hides the scope. As the patch is now one can see that the logic
extends and would have made the indentation horrible if not introducing
a dedicated method. No preparation patch, please.

[PATCH 4/4] spi: s3c64xx: add support for google,gs101-spi

[Tudor Ambarus]

Add support for GS101 SPI. GS101 integrates 16 SPI nodes, all with 64
bytes FIFOs. GS101 allows just 32 bit register accesses, otherwise a
Serror Interrupt is raised. Do the write reg accesses in 32 bits.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/spi/spi-s3c64xx.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index cb45ad615f3d..9ad0d513fb30 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -19,7 +19,7 @@
 #include <linux/spi/spi.h>
 #include <linux/types.h>
 
-#define MAX_SPI_PORTS		12
+#define MAX_SPI_PORTS		16
 #define S3C64XX_SPI_QUIRK_CS_AUTO	(1 << 1)
 #define AUTOSUSPEND_TIMEOUT	2000
 
@@ -1538,6 +1538,19 @@ static const struct s3c64xx_spi_port_config fsd_spi_port_config = {
 	.quirks		= S3C64XX_SPI_QUIRK_CS_AUTO,
 };
 
+static const struct s3c64xx_spi_port_config gs101_spi_port_config = {
+	.fifo_lvl_mask	= { 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f,
+			    0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f},
+	.rx_lvl_offset	= 15,
+	.tx_st_done	= 25,
+	.clk_div	= 4,
+	.high_speed	= true,
+	.clk_from_cmu	= true,
+	.has_loopback	= true,
+	.use_32bit_io	= true,
+	.quirks		= S3C64XX_SPI_QUIRK_CS_AUTO,
+};
+
 static const struct platform_device_id s3c64xx_spi_driver_ids[] = {
 	{
 		.name		= "s3c2443-spi",
@@ -1550,6 +1563,9 @@ static const struct platform_device_id s3c64xx_spi_driver_ids[] = {
 };
 
 static const struct of_device_id s3c64xx_spi_dt_match[] = {
+	{ .compatible = "google,gs101-spi",
+			.data = &gs101_spi_port_config,
+	},
 	{ .compatible = "samsung,s3c2443-spi",
 			.data = &s3c2443_spi_port_config,
 	},
-- 
2.43.0.594.gd9cf4e227d-goog

Re: [PATCH 4/4] spi: s3c64xx: add support for google,gs101-spi

[Peter Griffin]

Hi Tudor,

On Tue, 6 Feb 2024 at 08:52, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Add support for GS101 SPI. GS101 integrates 16 SPI nodes, all with 64
> bytes FIFOs. GS101 allows just 32 bit register accesses, otherwise a
> Serror Interrupt is raised. Do the write reg accesses in 32 bits.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

The patch ordering seems a bit off with this series..I believe it should be
1) dt-bindings patch (docs first)
2) Add the use_32bit_io flag / functionality
3) gs101 support (this patch) that uses the use_32bit_io functionality

Peter.

>  drivers/spi/spi-s3c64xx.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index cb45ad615f3d..9ad0d513fb30 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -19,7 +19,7 @@
>  #include <linux/spi/spi.h>
>  #include <linux/types.h>
>
> -#define MAX_SPI_PORTS          12
> +#define MAX_SPI_PORTS          16
>  #define S3C64XX_SPI_QUIRK_CS_AUTO      (1 << 1)
>  #define AUTOSUSPEND_TIMEOUT    2000
>
> @@ -1538,6 +1538,19 @@ static const struct s3c64xx_spi_port_config fsd_spi_port_config = {
>         .quirks         = S3C64XX_SPI_QUIRK_CS_AUTO,
>  };
>
> +static const struct s3c64xx_spi_port_config gs101_spi_port_config = {
> +       .fifo_lvl_mask  = { 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f,
> +                           0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f},
> +       .rx_lvl_offset  = 15,
> +       .tx_st_done     = 25,
> +       .clk_div        = 4,
> +       .high_speed     = true,
> +       .clk_from_cmu   = true,
> +       .has_loopback   = true,
> +       .use_32bit_io   = true,
> +       .quirks         = S3C64XX_SPI_QUIRK_CS_AUTO,
> +};
> +
>  static const struct platform_device_id s3c64xx_spi_driver_ids[] = {
>         {
>                 .name           = "s3c2443-spi",
> @@ -1550,6 +1563,9 @@ static const struct platform_device_id s3c64xx_spi_driver_ids[] = {
>  };
>
>  static const struct of_device_id s3c64xx_spi_dt_match[] = {
> +       { .compatible = "google,gs101-spi",
> +                       .data = &gs101_spi_port_config,
> +       },
>         { .compatible = "samsung,s3c2443-spi",
>                         .data = &s3c2443_spi_port_config,
>         },
> --
> 2.43.0.594.gd9cf4e227d-goog
>

Re: [PATCH 4/4] spi: s3c64xx: add support for google,gs101-spi

[Mark Brown]

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

On Tue, Feb 06, 2024 at 10:12:30AM +0000, Peter Griffin wrote:

> The patch ordering seems a bit off with this series..I believe it should be
> 1) dt-bindings patch (docs first)
> 2) Add the use_32bit_io flag / functionality
> 3) gs101 support (this patch) that uses the use_32bit_io functionality

That's the ordering the series has?  There's a random cleanup patch
tacked on the front but that really ought to be separate anyway.

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

Re: [PATCH 4/4] spi: s3c64xx: add support for google,gs101-spi

[Tudor Ambarus]

On 2/6/24 11:04, Mark Brown wrote:
> On Tue, Feb 06, 2024 at 10:12:30AM +0000, Peter Griffin wrote:
> 
>> The patch ordering seems a bit off with this series..I believe it should be
>> 1) dt-bindings patch (docs first)
>> 2) Add the use_32bit_io flag / functionality
>> 3) gs101 support (this patch) that uses the use_32bit_io functionality
> 
> That's the ordering the series has?  There's a random cleanup patch
> tacked on the front but that really ought to be separate anyway.

I put the include <linux/types.h> patch first because I considered it a
fix (driver is using u32) and because I need types.h in patch 3/4. Fixes
first, then bindings, then driver.

Was I wrong?

Re: [PATCH 4/4] spi: s3c64xx: add support for google,gs101-spi

[Peter Griffin]

On Tue, 6 Feb 2024 at 11:19, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
>
>
> On 2/6/24 11:04, Mark Brown wrote:
> > On Tue, Feb 06, 2024 at 10:12:30AM +0000, Peter Griffin wrote:
> >
> >> The patch ordering seems a bit off with this series..I believe it should be
> >> 1) dt-bindings patch (docs first)
> >> 2) Add the use_32bit_io flag / functionality
> >> 3) gs101 support (this patch) that uses the use_32bit_io functionality
> >
> > That's the ordering the series has?  There's a random cleanup patch
> > tacked on the front but that really ought to be separate anyway.
>
> I put the include <linux/types.h> patch first because I considered it a
> fix (driver is using u32) and because I need types.h in patch 3/4. Fixes
> first, then bindings, then driver.
>
> Was I wrong?

No my mistake, sorry for the noise. Gmail showed this driver change as
the first patch after the cover letter but the subject was hidden so
it wasn't obvious it was [4/4]

Re: [PATCH 4/4] spi: s3c64xx: add support for google,gs101-spi

[Sam Protsenko]

On Tue, Feb 6, 2024 at 2:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Add support for GS101 SPI. GS101 integrates 16 SPI nodes, all with 64
> bytes FIFOs. GS101 allows just 32 bit register accesses, otherwise a
> Serror Interrupt is raised. Do the write reg accesses in 32 bits.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>  drivers/spi/spi-s3c64xx.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index cb45ad615f3d..9ad0d513fb30 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -19,7 +19,7 @@
>  #include <linux/spi/spi.h>
>  #include <linux/types.h>
>
> -#define MAX_SPI_PORTS          12
> +#define MAX_SPI_PORTS          16
>  #define S3C64XX_SPI_QUIRK_CS_AUTO      (1 << 1)
>  #define AUTOSUSPEND_TIMEOUT    2000
>
> @@ -1538,6 +1538,19 @@ static const struct s3c64xx_spi_port_config fsd_spi_port_config = {
>         .quirks         = S3C64XX_SPI_QUIRK_CS_AUTO,
>  };
>
> +static const struct s3c64xx_spi_port_config gs101_spi_port_config = {
> +       .fifo_lvl_mask  = { 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f,
> +                           0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f},
> +       .rx_lvl_offset  = 15,
> +       .tx_st_done     = 25,
> +       .clk_div        = 4,
> +       .high_speed     = true,
> +       .clk_from_cmu   = true,
> +       .has_loopback   = true,
> +       .use_32bit_io   = true,
> +       .quirks         = S3C64XX_SPI_QUIRK_CS_AUTO,
> +};
> +
>  static const struct platform_device_id s3c64xx_spi_driver_ids[] = {
>         {
>                 .name           = "s3c2443-spi",
> @@ -1550,6 +1563,9 @@ static const struct platform_device_id s3c64xx_spi_driver_ids[] = {
>  };
>
>  static const struct of_device_id s3c64xx_spi_dt_match[] = {
> +       { .compatible = "google,gs101-spi",
> +                       .data = &gs101_spi_port_config,
> +       },
>         { .compatible = "samsung,s3c2443-spi",
>                         .data = &s3c2443_spi_port_config,
>         },
> --
> 2.43.0.594.gd9cf4e227d-goog
>

Re: [PATCH 0/4] spi: s3c64xx: add support for google,gs101-spi

[Sam Protsenko]

On Tue, Feb 6, 2024 at 2:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Depends on the simple cleanup patches from:
> https://lore.kernel.org/linux-spi/20240205124513.447875-1-tudor.ambarus@linaro.org/
>
> A slightly different version of the google,gs101-spi support was sent at:
> https://lore.kernel.org/linux-spi/20240125145007.748295-1-tudor.ambarus@linaro.org/
>
> Let's add support for gs101-spi so that I have a testing base for the
> driver rework patches that will follow.
>
> Tudor Ambarus (4):
>   spi: s3c64xx: explicitly include <linux/types.h>
>   spi: dt-bindings: samsung: add google,gs101-spi compatible
>   spi: s3c64xx: add s3c64xx_iowrite{8,16}_32_rep accessors
>   spi: s3c64xx: add support for google,gs101-spi
>

Just a grumpy note: I wish this series (except for the [PATCH 1/4],
which I'd argue doesn't belong here) was submitted before the rest of
SPI cleanups and reworkings. Would've made reviewing much easier,
because this series doesn't apply without SPI cleanup series that has
to be applied prior to that. There are other benefits to that approach
too, as was discussed earlier.

>  .../devicetree/bindings/spi/samsung,spi.yaml  |  1 +
>  drivers/spi/spi-s3c64xx.c                     | 89 +++++++++++++++----
>  2 files changed, 75 insertions(+), 15 deletions(-)
>
> --
> 2.43.0.594.gd9cf4e227d-goog
>

Re: [PATCH 0/4] spi: s3c64xx: add support for google,gs101-spi

[Tudor Ambarus]

On 2/6/24 18:59, Sam Protsenko wrote:
> On Tue, Feb 6, 2024 at 2:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>> Depends on the simple cleanup patches from:
>> https://lore.kernel.org/linux-spi/20240205124513.447875-1-tudor.ambarus@linaro.org/
>>
>> A slightly different version of the google,gs101-spi support was sent at:
>> https://lore.kernel.org/linux-spi/20240125145007.748295-1-tudor.ambarus@linaro.org/
>>
>> Let's add support for gs101-spi so that I have a testing base for the
>> driver rework patches that will follow.
>>
>> Tudor Ambarus (4):
>>   spi: s3c64xx: explicitly include <linux/types.h>
>>   spi: dt-bindings: samsung: add google,gs101-spi compatible
>>   spi: s3c64xx: add s3c64xx_iowrite{8,16}_32_rep accessors
>>   spi: s3c64xx: add support for google,gs101-spi
>>
> 
> Just a grumpy note: I wish this series (except for the [PATCH 1/4],
> which I'd argue doesn't belong here) was submitted before the rest of
> SPI cleanups and reworkings. Would've made reviewing much easier,
> because this series doesn't apply without SPI cleanup series that has
> to be applied prior to that. There are other benefits to that approach
> too, as was discussed earlier.
> 

I feel we're bike-shedding, it drains my energy. Your reasons were:
1/ easier review
2/ easier backporting of gs101 if that's ever wanted
3/ driver rework takes more review time and I risk not having gs101
integrated for next release

2/ is not true right now, I could cherry-pick the iowrite and gs101
patches on top of v6.7. With 1/ I don't agree as the gs101 patches are
the same with or without the simple cleanup.
3/ is my responsibility and I'm ok with it, I feel there's enough time
for all

What matters, as I specified in the cover letter, is to have the gs101
patches before the functional driver rework which will follow, so that I
can test each functional patch with gs101.

I give up however, I'll do as you want. Will respin all.