| Message ID | 20240215180102.13887-4-fancer.lancer@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | spi: dw: Auto-detect number of native CS | expand |
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 > > >
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!
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 --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) {
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(-)