| Message ID | 20240125145007.748295-24-tudor.ambarus@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | spi: s3c64xx: winter cleanup and gs101 support | expand |
On Thu, Jan 25, 2024 at 02:50:01PM +0000, Tudor Ambarus wrote: > Allow SoCs that have multiple instances of the SPI IP with different > FIFO sizes to specify their FIFO size via the "samsung,spi-fifosize" > device tree property. With this we can break the dependency between the > SPI alias, the fifo_lvl_mask and the FIFO size. OK, so we do actually have SoCs with multiple instances of the IP with different FIFO depths (and who knows what else other differences)?
On Thu, Jan 25, 2024 at 11:33 AM Mark Brown <broonie@kernel.org> wrote: > > On Thu, Jan 25, 2024 at 02:50:01PM +0000, Tudor Ambarus wrote: > > > Allow SoCs that have multiple instances of the SPI IP with different > > FIFO sizes to specify their FIFO size via the "samsung,spi-fifosize" > > device tree property. With this we can break the dependency between the > > SPI alias, the fifo_lvl_mask and the FIFO size. > > OK, so we do actually have SoCs with multiple instances of the IP with > different FIFO depths (and who knows what else other differences)? I think that's why we can see .fifo_lvl_mask[] with different values for different IP instances. For example, ExynosAutoV9 has this (in upstream driver, yes): .fifo_lvl_mask = { 0x1ff, 0x1ff, 0x7f, 0x7f, 0x7f, 0x7f, 0x1ff, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f}, And I'm pretty sure the comment (in struct s3c64xx_spi_port_config) for .fifo_lvl_mask field is not correct anymore: * @fifo_lvl_mask: Bit-mask for {TX|RX}_FIFO_LVL bits in SPI_STATUS register. Maybe it used to indicate the bit number in SPI_STATUS register for {TX|RX}_FIFO_LVL fields, but not anymore. Nowadays it looks like .fifo_lvl_mask just specifies FIFO depth for each IP instance.
On Fri, Jan 26, 2024, at 20:23, Sam Protsenko wrote: > On Thu, Jan 25, 2024 at 11:33 AM Mark Brown <broonie@kernel.org> wrote: >> >> On Thu, Jan 25, 2024 at 02:50:01PM +0000, Tudor Ambarus wrote: >> >> > Allow SoCs that have multiple instances of the SPI IP with different >> > FIFO sizes to specify their FIFO size via the "samsung,spi-fifosize" >> > device tree property. With this we can break the dependency between the >> > SPI alias, the fifo_lvl_mask and the FIFO size. >> >> OK, so we do actually have SoCs with multiple instances of the IP with >> different FIFO depths (and who knows what else other differences)? > > I think that's why we can see .fifo_lvl_mask[] with different values > for different IP instances. For example, ExynosAutoV9 has this (in > upstream driver, yes): > > .fifo_lvl_mask = { 0x1ff, 0x1ff, 0x7f, 0x7f, 0x7f, 0x7f, 0x1ff, > 0x7f, 0x7f, 0x7f, 0x7f, 0x7f}, > That sounds like the same bug as in the serial port driver, by assuming that the alias values in the devicetree have a particular meaning in identifying instances. This immediately breaks when there is a dtb file that does not use the same alias values, e.g. because it only needs some of the SPI ports. Arnd
On Fri, Jan 26, 2024 at 2:17 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Fri, Jan 26, 2024, at 20:23, Sam Protsenko wrote: > > On Thu, Jan 25, 2024 at 11:33 AM Mark Brown <broonie@kernel.org> wrote: > >> > >> On Thu, Jan 25, 2024 at 02:50:01PM +0000, Tudor Ambarus wrote: > >> > >> > Allow SoCs that have multiple instances of the SPI IP with different > >> > FIFO sizes to specify their FIFO size via the "samsung,spi-fifosize" > >> > device tree property. With this we can break the dependency between the > >> > SPI alias, the fifo_lvl_mask and the FIFO size. > >> > >> OK, so we do actually have SoCs with multiple instances of the IP with > >> different FIFO depths (and who knows what else other differences)? > > > > I think that's why we can see .fifo_lvl_mask[] with different values > > for different IP instances. For example, ExynosAutoV9 has this (in > > upstream driver, yes): > > > > .fifo_lvl_mask = { 0x1ff, 0x1ff, 0x7f, 0x7f, 0x7f, 0x7f, 0x1ff, > > 0x7f, 0x7f, 0x7f, 0x7f, 0x7f}, > > > > That sounds like the same bug as in the serial port driver, > by assuming that the alias values in the devicetree have > a particular meaning in identifying instances. This immediately > breaks when there is a dtb file that does not use the same > alias values, e.g. because it only needs some of the SPI > ports. > Exactly. I guess that's exactly what Tudor mentioned in his commit message, and he's trying to fix that very problem by relying on corresponding dts property (in his patch series) rather than on .fifo_lvl_mask. > Arnd
On Fri, Jan 26, 2024 at 09:16:53PM +0100, Arnd Bergmann wrote: > That sounds like the same bug as in the serial port driver, > by assuming that the alias values in the devicetree have > a particular meaning in identifying instances. This immediately > breaks when there is a dtb file that does not use the same > alias values, e.g. because it only needs some of the SPI > ports. It'll be the result of a conversion from board files where that was a normal way of doing things.
On 26.01.2024 22:20, Sam Protsenko wrote: > On Fri, Jan 26, 2024 at 2:17 PM Arnd Bergmann <arnd@arndb.de> wrote: >> >> On Fri, Jan 26, 2024, at 20:23, Sam Protsenko wrote: >>> On Thu, Jan 25, 2024 at 11:33 AM Mark Brown <broonie@kernel.org> wrote: >>>> >>>> On Thu, Jan 25, 2024 at 02:50:01PM +0000, Tudor Ambarus wrote: >>>> >>>>> Allow SoCs that have multiple instances of the SPI IP with different >>>>> FIFO sizes to specify their FIFO size via the "samsung,spi-fifosize" >>>>> device tree property. With this we can break the dependency between the >>>>> SPI alias, the fifo_lvl_mask and the FIFO size. >>>> >>>> OK, so we do actually have SoCs with multiple instances of the IP with >>>> different FIFO depths (and who knows what else other differences)? >>> >>> I think that's why we can see .fifo_lvl_mask[] with different values >>> for different IP instances. For example, ExynosAutoV9 has this (in >>> upstream driver, yes): >>> >>> .fifo_lvl_mask = { 0x1ff, 0x1ff, 0x7f, 0x7f, 0x7f, 0x7f, 0x1ff, >>> 0x7f, 0x7f, 0x7f, 0x7f, 0x7f}, >>> >> >> That sounds like the same bug as in the serial port driver, >> by assuming that the alias values in the devicetree have >> a particular meaning in identifying instances. This immediately >> breaks when there is a dtb file that does not use the same >> alias values, e.g. because it only needs some of the SPI >> ports. >> > > Exactly. I guess that's exactly what Tudor mentioned in his commit > message, and he's trying to fix that very problem by relying on > corresponding dts property (in his patch series) rather than on > .fifo_lvl_mask. > Yes, all from above are correct. I'll split the FIFO size patches into a smaller series to be easier to review. Cheers, ta
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c index 7a99f6b02319..3e7797d915c5 100644 --- a/drivers/spi/spi-s3c64xx.c +++ b/drivers/spi/spi-s3c64xx.c @@ -1114,7 +1114,7 @@ static int s3c64xx_spi_get_fifosize(const struct platform_device *pdev, const struct s3c64xx_spi_port_config *port = sdd->port_conf; const int *fifo_lvl_mask = port->fifo_lvl_mask; struct device_node *np = pdev->dev.of_node; - int id; + int id, ret; if (!np) { if (pdev->id < 0) @@ -1130,6 +1130,10 @@ static int s3c64xx_spi_get_fifosize(const struct platform_device *pdev, return 0; } + ret = of_property_read_u32(np, "samsung,spi-fifosize", &sdd->fifosize); + if (ret == 0) + return 0; + id = of_alias_get_id(np, "spi"); if (id < 0) return dev_err_probe(&pdev->dev, id,
Allow SoCs that have multiple instances of the SPI IP with different FIFO sizes to specify their FIFO size via the "samsung,spi-fifosize" device tree property. With this we can break the dependency between the SPI alias, the fifo_lvl_mask and the FIFO size. Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> --- drivers/spi/spi-s3c64xx.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)