diff mbox series

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

Message ID 20240215180102.13887-4-fancer.lancer@gmail.com
State New
Headers
Series spi: dw: Auto-detect number of native CS |

Commit Message

Serge Semin Feb. 15, 2024, 6 p.m. UTC 
  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(-)

Comments

Andy Shevchenko Feb. 15, 2024, 7:32 p.m. UTC | #1 
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;
Serge Semin Feb. 16, 2024, 3:36 p.m. UTC | #2 
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
> 
> 
>
Andy Shevchenko Feb. 16, 2024, 5 p.m. UTC | #3 
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!
Serge Semin Feb. 16, 2024, 6:03 p.m. UTC | #4 
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
diff mbox series

Patch

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) {