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 <[email protected]>
Cc: Alexey Malahov <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: [email protected]
Cc: [email protected]
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
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
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 <[email protected]> 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
On Fri, Feb 16, 2024 at 07:00:28PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 16, 2024 at 5:36 PM Serge Semin <[email protected]> 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