| Message ID | 20240125145007.748295-22-tudor.ambarus@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: s3c64xx: winter cleanup and gs101 support | expand |
On Thu, Jan 25, 2024 at 02:49:59PM +0000, Tudor Ambarus wrote: > Infer the FIFO size from the compatible, where all the instances of the > SPI IP have the same FIFO size. This way we no longer depend on the SPI > alias from the device tree to select the FIFO size, thus we remove the > dependency of the driver on the SPI alias. > static const struct s3c64xx_spi_port_config s3c2443_spi_port_config = { > - .fifo_lvl_mask = { 0x7f }, > + .fifosize = 64, > .rx_lvl_offset = 13, > .tx_st_done = 21, > .clk_div = 2, I'm having real trouble associating the changelog with the change here. This appears to be changing from specifying the mask for the FIFO level register to specifying the size of the FIFO and unrelated to anything to do with looking things up from the compatible?
On 1/25/24 17:18, Mark Brown wrote: > On Thu, Jan 25, 2024 at 02:49:59PM +0000, Tudor Ambarus wrote: > >> Infer the FIFO size from the compatible, where all the instances of the >> SPI IP have the same FIFO size. This way we no longer depend on the SPI >> alias from the device tree to select the FIFO size, thus we remove the >> dependency of the driver on the SPI alias. > >> static const struct s3c64xx_spi_port_config s3c2443_spi_port_config = { >> - .fifo_lvl_mask = { 0x7f }, >> + .fifosize = 64, >> .rx_lvl_offset = 13, >> .tx_st_done = 21, >> .clk_div = 2, > > I'm having real trouble associating the changelog with the change here. > This appears to be changing from specifying the mask for the FIFO level > register to specifying the size of the FIFO and unrelated to anything to > do with looking things up from the compatible? Let me try to explain everything. In the driver there is a weird dependency between the SPI of_alias ID, s3c64xx_spi_port_config.fifo_lvl_mask and the IP's FIFO depth. s3c64xx_spi_port_config.fifo_lvl_mask is not a 1:1 match with the SPI_STATUSn.{RX, TX}_FIFO_LVL register field. Those fields are defined in the datasheet as: +#define S3C64XX_SPI_ST_RX_FIFO_LVL GENMASK(23, 15) +#define S3C64XX_SPI_ST_TX_FIFO_LVL GENMASK(14, 6) Thus the register mask is on 9 bits, but the driver used either 0x1ff or 0x7f, which was not reflecting the real register mask. Patch 10/28 updates the driver to use the full register mask regardless of the FIFO depth configuration. Another problem with s3c64xx_spi_port_config.fifo_lvl_mask is that it was used as a way to determine the FIFO depth. The SPI of_alias ID was used as an index in this array to determine the FIFO depth with something like fifo_depth = (fifo_lvl_mask[alias_id] >> 1) + 1 For example, if one wanted to specify a 64 FIFO length (0x40), it would have configured the FIFO level to 127 (0x7f). The patch set breaks this weird dependencies. Obviously the FIFO depth must be tightly tied by the compatible and not by an alias. I tied the FIFO depth to the compatible in 2 ways: 1/ For SoCs that have all the SPI nodes with the same FIFO depth, I chose to deduce the FIFO depth from the compatible. Instead of specifying "samsung,spi-fifosize" for all the gs101 SPI nodes in the device tree, I chose to infer it from the compatible. I know for sure that all the gs101 SPI nodes have 64 bytes FIFO depths, thus don't pollute the device tree with superfluous info. (patches 20/28 and 21/28) 2/ For SoCs that have instances of the SPI IP with different FIFO depths, specify the node's FIFO depth via the "samsung,spi-fifosize" dt property. (patch 23/28) Hope this helps. Please tell if you want me to elaborate on something.
On Thu, Jan 25, 2024 at 8:50 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > Infer the FIFO size from the compatible, where all the instances of the > SPI IP have the same FIFO size. This way we no longer depend on the SPI > alias from the device tree to select the FIFO size, thus we remove the > dependency of the driver on the SPI alias. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- > drivers/spi/spi-s3c64xx.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c > index 5a93ed4125b0..b86eb0a77b60 100644 > --- a/drivers/spi/spi-s3c64xx.c > +++ b/drivers/spi/spi-s3c64xx.c > @@ -1381,7 +1381,7 @@ static const struct dev_pm_ops s3c64xx_spi_pm = { > }; > > static const struct s3c64xx_spi_port_config s3c2443_spi_port_config = { > - .fifo_lvl_mask = { 0x7f }, How will it work with already existing out-of-tree dts's, if only kernel image gets updated? I wonder if it's considered ok to break that compatibility like this. > + .fifosize = 64, > .rx_lvl_offset = 13, > .tx_st_done = 21, > .clk_div = 2, > @@ -1389,7 +1389,7 @@ static const struct s3c64xx_spi_port_config s3c2443_spi_port_config = { > }; > > static const struct s3c64xx_spi_port_config s3c6410_spi_port_config = { > - .fifo_lvl_mask = { 0x7f, 0x7F }, > + .fifosize = 64, > .rx_lvl_offset = 13, > .tx_st_done = 21, > .clk_div = 2, > @@ -1435,7 +1435,7 @@ static const struct s3c64xx_spi_port_config exynos5433_spi_port_config = { > }; > > static const struct s3c64xx_spi_port_config exynos850_spi_port_config = { > - .fifo_lvl_mask = { 0x7f, 0x7f, 0x7f }, > + .fifosize = 64, > .rx_lvl_offset = 15, > .tx_st_done = 25, > .clk_div = 4, > @@ -1459,7 +1459,7 @@ static const struct s3c64xx_spi_port_config exynosautov9_spi_port_config = { > }; > > static const struct s3c64xx_spi_port_config fsd_spi_port_config = { > - .fifo_lvl_mask = { 0x7f, 0x7f, 0x7f, 0x7f, 0x7f}, > + .fifosize = 64, > .rx_lvl_offset = 15, > .tx_st_done = 25, > .clk_div = 2, > -- > 2.43.0.429.g432eaa2c6b-goog >
On 1/25/24 22:28, Sam Protsenko wrote: > On Thu, Jan 25, 2024 at 8:50 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >> Infer the FIFO size from the compatible, where all the instances of the >> SPI IP have the same FIFO size. This way we no longer depend on the SPI >> alias from the device tree to select the FIFO size, thus we remove the >> dependency of the driver on the SPI alias. >> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> >> --- >> drivers/spi/spi-s3c64xx.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c >> index 5a93ed4125b0..b86eb0a77b60 100644 >> --- a/drivers/spi/spi-s3c64xx.c >> +++ b/drivers/spi/spi-s3c64xx.c >> @@ -1381,7 +1381,7 @@ static const struct dev_pm_ops s3c64xx_spi_pm = { >> }; >> >> static const struct s3c64xx_spi_port_config s3c2443_spi_port_config = { >> - .fifo_lvl_mask = { 0x7f }, > How will it work with already existing out-of-tree dts's, if only > kernel image gets updated? I wonder if it's considered ok to break > that compatibility like this. > ah, good catch, Sam! I prepared everything to not break older device trees and then I removed this :). >> + .fifosize = 64, Adding .fifosize and keeping fifo_lvl_mask will not break backward compatibility. In s3c64xx_spi_get_fifosize() I first check if .fifosize is set and use that, and if not set, use the .fifo_lvl_mask.
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c index 5a93ed4125b0..b86eb0a77b60 100644 --- a/drivers/spi/spi-s3c64xx.c +++ b/drivers/spi/spi-s3c64xx.c @@ -1381,7 +1381,7 @@ static const struct dev_pm_ops s3c64xx_spi_pm = { }; static const struct s3c64xx_spi_port_config s3c2443_spi_port_config = { - .fifo_lvl_mask = { 0x7f }, + .fifosize = 64, .rx_lvl_offset = 13, .tx_st_done = 21, .clk_div = 2, @@ -1389,7 +1389,7 @@ static const struct s3c64xx_spi_port_config s3c2443_spi_port_config = { }; static const struct s3c64xx_spi_port_config s3c6410_spi_port_config = { - .fifo_lvl_mask = { 0x7f, 0x7F }, + .fifosize = 64, .rx_lvl_offset = 13, .tx_st_done = 21, .clk_div = 2, @@ -1435,7 +1435,7 @@ static const struct s3c64xx_spi_port_config exynos5433_spi_port_config = { }; static const struct s3c64xx_spi_port_config exynos850_spi_port_config = { - .fifo_lvl_mask = { 0x7f, 0x7f, 0x7f }, + .fifosize = 64, .rx_lvl_offset = 15, .tx_st_done = 25, .clk_div = 4, @@ -1459,7 +1459,7 @@ static const struct s3c64xx_spi_port_config exynosautov9_spi_port_config = { }; static const struct s3c64xx_spi_port_config fsd_spi_port_config = { - .fifo_lvl_mask = { 0x7f, 0x7f, 0x7f, 0x7f, 0x7f}, + .fifosize = 64, .rx_lvl_offset = 15, .tx_st_done = 25, .clk_div = 2,
Infer the FIFO size from the compatible, where all the instances of the SPI IP have the same FIFO size. This way we no longer depend on the SPI alias from the device tree to select the FIFO size, thus we remove the dependency of the driver on the SPI alias. Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> --- drivers/spi/spi-s3c64xx.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)