[PATCH 0/3] spi: dw: Auto-detect number of native CS

[PATCH 0/3] spi: dw: Auto-detect number of native CS

[Serge Semin]

The main goal of the short series is to provide a procedure implementing
the auto-detection of the number of native Chip-Select signals supported
by the controller. The suggested algorithm is straightforward. It relies
on the fact that the SER register writable flags reflects the actual
number of available native chip-select signals. So the DW APB/AHB SSI
driver now tests the SER register for having the writable bits,
calculates the number of CS signals based on the number of set flags and
then initializes the num_cs private data field based on that, which then
will be passed to the SPI-core subsystem indicating the number of
supported hardware chip-selects. The implemented procedure will be useful
for the DW SSI device nodes not having the explicitly set "num-cs"
property. In case if the property is specified it will be utilized instead
of the auto-detection procedure.

Besides of that a small cleanup patch is introduced in the head of the
series. It converts the driver to using the BITS_TO_BYTES() macro instead
of the hard-coded DIV_ROUND_UP()-based calculation of the number of
bytes-per-transfer-word.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Andy Shevchenko <andy@kernel.org>
Cc: linux-spi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

Serge Semin (3):
  spi: dw: Convert to using BITS_TO_BYTES() macro
  spi: dw: Add a number of native CS auto-detection
  spi: dw: Drop default number of CS setting

 drivers/spi/spi-dw-core.c | 20 ++++++++++++++++----
 drivers/spi/spi-dw-mmio.c |  7 ++-----
 2 files changed, 18 insertions(+), 9 deletions(-)

-- 
2.43.0

[PATCH 1/3] spi: dw: Convert to using BITS_TO_BYTES() macro

[Serge Semin]

Since commit dd3e7cba1627 ("ocfs2/dlm: move BITS_TO_BYTES() to bitops.h
for wider use") there is a generic helper available to calculate a number
of bytes needed to accommodate the specified number of bits. Let's use it
instead of the hard-coded DIV_ROUND_UP() macro function.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/spi/spi-dw-core.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index 0274c9295514..722b5eb1f709 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/bitops.h>
 #include <linux/dma-mapping.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
@@ -421,10 +422,7 @@ static int dw_spi_transfer_one(struct spi_controller *host,
 	int ret;
 
 	dws->dma_mapped = 0;
-	dws->n_bytes =
-		roundup_pow_of_two(DIV_ROUND_UP(transfer->bits_per_word,
-						BITS_PER_BYTE));
-
+	dws->n_bytes = roundup_pow_of_two(BITS_TO_BYTES(transfer->bits_per_word));
 	dws->tx = (void *)transfer->tx_buf;
 	dws->tx_len = transfer->len / dws->n_bytes;
 	dws->rx = transfer->rx_buf;
-- 
2.43.0

Re: [PATCH 1/3] spi: dw: Convert to using BITS_TO_BYTES() macro

[Andy Shevchenko]

On Thu, Feb 15, 2024 at 09:00:46PM +0300, Serge Semin wrote:
> Since commit dd3e7cba1627 ("ocfs2/dlm: move BITS_TO_BYTES() to bitops.h
> for wider use") there is a generic helper available to calculate a number
> of bytes needed to accommodate the specified number of bits. Let's use it
> instead of the hard-coded DIV_ROUND_UP() macro function.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko

[PATCH 2/3] spi: dw: Add a number of native CS auto-detection

[Serge Semin]

Aside with the FIFO depth and DFS field size it's possible to auto-detect
a number of native chip-select synthesized in the DW APB/AHB SSI IP-core.
It can be done just by writing ones to the SER register. The number of
writable flags in the register is limited by the SSI_NUM_SLAVES IP-core
synthesize parameter. All the upper flags are read-only and wired to zero.
Based on that let's add the number of native CS auto-detection procedure
so the low-level platform drivers wouldn't need to manually set it up
unless it's required to set a constraint due to platform-specific reasons
(for instance, due to a hardware bug).

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/spi/spi-dw-core.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index 722b5eb1f709..ddfdb903047a 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -834,6 +834,20 @@ static void dw_spi_hw_init(struct device *dev, struct dw_spi *dws)
 			DW_SPI_GET_BYTE(dws->ver, 1));
 	}
 
+	/*
+	 * Try to detect the number of native chip-selects if the platform
+	 * driver didn't set it up. There can be up to 16 lines configured.
+	 */
+	if (!dws->num_cs) {
+		u32 ser;
+
+		dw_writel(dws, DW_SPI_SER, 0xffff);
+		ser = dw_readl(dws, DW_SPI_SER);
+		dw_writel(dws, DW_SPI_SER, 0);
+
+		dws->num_cs = hweight16(ser);
+	}
+
 	/*
 	 * Try to detect the FIFO depth if not set by interface driver,
 	 * the depth could be from 2 to 256 from HW spec
-- 
2.43.0

Re: [PATCH 2/3] spi: dw: Add a number of native CS auto-detection

[Andy Shevchenko]

On Thu, Feb 15, 2024 at 09:00:47PM +0300, Serge Semin wrote:
> Aside with the FIFO depth and DFS field size it's possible to auto-detect
> a number of native chip-select synthesized in the DW APB/AHB SSI IP-core.
> It can be done just by writing ones to the SER register. The number of
> writable flags in the register is limited by the SSI_NUM_SLAVES IP-core
> synthesize parameter. All the upper flags are read-only and wired to zero.
> Based on that let's add the number of native CS auto-detection procedure
> so the low-level platform drivers wouldn't need to manually set it up
> unless it's required to set a constraint due to platform-specific reasons
> (for instance, due to a hardware bug).

...

> +	/*
> +	 * Try to detect the number of native chip-selects if the platform
> +	 * driver didn't set it up. There can be up to 16 lines configured.
> +	 */
> +	if (!dws->num_cs) {
> +		u32 ser;
> +
> +		dw_writel(dws, DW_SPI_SER, 0xffff);

GENMASK() ?

> +		ser = dw_readl(dws, DW_SPI_SER);
> +		dw_writel(dws, DW_SPI_SER, 0);

Would it actually change the physical line state? If so, we may not do this.

> +		dws->num_cs = hweight16(ser);

Why 16 and not u16 & dw_writew()/readw()?

> +	}

I'm wondering why this can't be the default

	num_cs = ...autodetected...
	device_property_read(..., &num_cs);

-- 
With Best Regards,
Andy Shevchenko

[PATCH 3/3] spi: dw: Drop default number of CS setting

[Serge Semin]

DW APB/AHB SSI core now supports the procedure which automatically
determines the number of native CS. Thus there is no longer point in
defaulting to four CS if platform doesn't specify the real number.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/spi/spi-dw-mmio.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index cc74cbe03431..eb335fcc8720 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -364,11 +364,8 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
 				     &dws->reg_io_width))
 		dws->reg_io_width = 4;
 
-	num_cs = 4;
-
-	device_property_read_u32(&pdev->dev, "num-cs", &num_cs);
-
-	dws->num_cs = num_cs;
+	if (!device_property_read_u32(&pdev->dev, "num-cs", &num_cs))
+		dws->num_cs = num_cs;
 
 	init_func = device_get_match_data(&pdev->dev);
 	if (init_func) {
-- 
2.43.0

Re: [PATCH 3/3] spi: dw: Drop default number of CS setting

[Andy Shevchenko]

On Thu, Feb 15, 2024 at 09:00:48PM +0300, Serge Semin wrote:
> DW APB/AHB SSI core now supports the procedure which automatically
> determines the number of native CS. Thus there is no longer point in
> defaulting to four CS if platform doesn't specify the real number.

...

> -	num_cs = 4;

Simply update the default here?

> -	device_property_read_u32(&pdev->dev, "num-cs", &num_cs);
> -
> -	dws->num_cs = num_cs;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] spi: dw: Drop default number of CS setting

[Serge Semin]

On Thu, Feb 15, 2024 at 09:32:23PM +0200, Andy Shevchenko wrote:
> On Thu, Feb 15, 2024 at 09:00:48PM +0300, Serge Semin wrote:
> > DW APB/AHB SSI core now supports the procedure which automatically
> > determines the number of native CS. Thus there is no longer point in
> > defaulting to four CS if platform doesn't specify the real number.
> 
> ...
>
 
> > -	num_cs = 4;
> 
> Simply update the default here?
> 
> > -	device_property_read_u32(&pdev->dev, "num-cs", &num_cs);

Do you suggest to simply:

--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -364,8 +364,9 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
 				     &dws->reg_io_width))
 		dws->reg_io_width = 4;
 
-	num_cs = 4;
+	num_cs = 0;
 
 	device_property_read_u32(&pdev->dev, "num-cs", &num_cs);
 
?

My idea was to make the statement looking closer to what is
implemented for "reg-io-width" property. An alternative to what you
suggest and to my patch can be converting the dw_spi::num_cs type to
u32 and pass it to the device_property_read_u32() method directly:

--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
 	u32			max_freq;	/* max bus freq supported */
 
 	u32			reg_io_width;	/* DR I/O width in bytes */
+	u32			num_cs;		/* supported slave numbers */
 	u16			bus_num;
-	u16			num_cs;		/* supported slave numbers */
 	void (*set_cs)(struct spi_device *spi, bool enable);
 
 	/* Current message transfer state info */
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -320,7 +320,6 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
 	struct resource *mem;
 	struct dw_spi *dws;
 	int ret;
-	int num_cs;
 
 	dwsmmio = devm_kzalloc(&pdev->dev, sizeof(struct dw_spi_mmio),
 			       GFP_KERNEL);
@@ -364,8 +363,7 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
 				     &dws->reg_io_width))
 		dws->reg_io_width = 4;

-	num_cs = 4;
-
-       device_property_read_u32(&pdev->dev, "num-cs", &num_cs);
+       device_property_read_u32(&pdev->dev, "num-cs", &dws->num_cs);
-
-	dws->num_cs = num_cs;
 
 	init_func = device_get_match_data(&pdev->dev);
 	if (init_func) {

What do you think? Would that be better?

-Serge(y)

> > -
> > -	dws->num_cs = num_cs;
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
> 

Re: [PATCH 3/3] spi: dw: Drop default number of CS setting

[Andy Shevchenko]

On Fri, Feb 16, 2024 at 5:36 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> On Thu, Feb 15, 2024 at 09:32:23PM +0200, Andy Shevchenko wrote:
> > On Thu, Feb 15, 2024 at 09:00:48PM +0300, Serge Semin wrote:
> > > DW APB/AHB SSI core now supports the procedure which automatically
> > > determines the number of native CS. Thus there is no longer point in
> > > defaulting to four CS if platform doesn't specify the real number.

the platform


...

> > > -   num_cs = 4;
> >
> > Simply update the default here?
> >
> > > -   device_property_read_u32(&pdev->dev, "num-cs", &num_cs);
>
> Do you suggest to simply:
>
> --- a/drivers/spi/spi-dw-mmio.c
> +++ b/drivers/spi/spi-dw-mmio.c
> @@ -364,8 +364,9 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
>                                      &dws->reg_io_width))
>                 dws->reg_io_width = 4;
>
> -       num_cs = 4;
> +       num_cs = 0;
>
>         device_property_read_u32(&pdev->dev, "num-cs", &num_cs);
>
> ?

Either this or do

num_cs = dw_spi_get_num_cs_from_hw(...);

What would work better WRT hardware?

...

> My idea was to make the statement looking closer to what is
> implemented for "reg-io-width" property. An alternative to what you
> suggest and to my patch can be converting the dw_spi::num_cs type to
> u32 and pass it to the device_property_read_u32() method directly:

...patch...

> What do you think? Would that be better?

I like the change, but again, are you sure it won't break any setups?
If yes, go for this!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] spi: dw: Drop default number of CS setting

[Serge Semin]

On Fri, Feb 16, 2024 at 07:00:28PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 16, 2024 at 5:36 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> > On Thu, Feb 15, 2024 at 09:32:23PM +0200, Andy Shevchenko wrote:
> > > On Thu, Feb 15, 2024 at 09:00:48PM +0300, Serge Semin wrote:
> > > > DW APB/AHB SSI core now supports the procedure which automatically
> > > > determines the number of native CS. Thus there is no longer point in
> > > > defaulting to four CS if platform doesn't specify the real number.
> 
> the platform

Ok. Thanks.

> 
> 
> ...
> 
> > > > -   num_cs = 4;
> > >
> > > Simply update the default here?
> > >
> > > > -   device_property_read_u32(&pdev->dev, "num-cs", &num_cs);
> >
> > Do you suggest to simply:
> >
> > --- a/drivers/spi/spi-dw-mmio.c
> > +++ b/drivers/spi/spi-dw-mmio.c
> > @@ -364,8 +364,9 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
> >                                      &dws->reg_io_width))
> >                 dws->reg_io_width = 4;
> >
> > -       num_cs = 4;
> > +       num_cs = 0;
> >
> >         device_property_read_u32(&pdev->dev, "num-cs", &num_cs);
> >
> > ?
> 
> Either this or do
> 
> num_cs = dw_spi_get_num_cs_from_hw(...);

This is supposed to be generically done in
dw_spi_add_host()->dw_spi_hw_init() together with some other
auto-detections.

> 
> What would work better WRT hardware?

I'd stick with defaulting the dws->num_cs to zero here.

> 
> ...
> 
> > My idea was to make the statement looking closer to what is
> > implemented for "reg-io-width" property. An alternative to what you
> > suggest and to my patch can be converting the dw_spi::num_cs type to
> > u32 and pass it to the device_property_read_u32() method directly:
> 
> ...patch...
> 
> > What do you think? Would that be better?
> 

> I like the change, but again, are you sure it won't break any setups?

Well, I thought about this for quite some time. Here are the possible
options:
1. If "num-cs" property is specified, then nothing is changed. The
actual number of native chip-selects will be read from there.
2. If "num-cs" property isn't specified, then the auto-detection
procedure will be attempted. Here are some considerations in this
regard:
   2.1 defaulting to "4" hasn't been correct in the first place
       because by default the IP-core is synthesized with a single
       native CS line. So auto-detection would be more portable than
       guessing with a constant value.
   2.2 If some IP-cores have all SER bits writable then we'll just
       get to detect more than there are actual chip-selects. No
       regression in this case.
   2.3 If some IP-cores don't support the SER bits being
       simultaneously set then it violates what is described in the
       HW manuals - broadcasting is supposed to be supported by all DW
       SSI devices (currently I've got DW APB SSI v3.10a, v3.22a,
       v3.22b, v4.02a, v4.03a and DW AHB SSI 1.01a databooks
       confirming that).
   2.4 In case of 2.3 at least one chip-select shall be auto-detected
       unless the SER register doesn't permit an invalid value being
       written, which is also an undocumented case.
   2.5. In case of 2.3 and 2.4 either "num-cs" property or a
        platform-specific compatible string is supposed to be
        specified since the device isn't generic DW APB/AHB SSI.
        But if such device is discovered we'll see what could be done
        then.

So AFAICS the probability to break some setup shall be rather small.

> If yes, go for this!

Ok. Thanks.

-Serge(y)

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