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

[Serge Semin <fancer.lancer@xxxxxxxxx>]

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@xxxxxxxxx> 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