| Message ID | 20250213204044.660-3-wachiturroxd150@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | spi: s3c64xx: add support exynos990-spi to new port config data | expand |
On Thu, Feb 13, 2025 at 2:41 PM Denzeel Oliva <wachiturroxd150@gmail.com> wrote: > > Exynos990 uses the same version of USI SPI (v2.1) as the GS101. > Removed fifo_lvl_mask and rx_lvl_offset, and changed to the new data > configuration port. > > The difference from other new port configuration data is that fifo_depth > is only specified in fifo-depth in DT. > In the code below I can see this bit: /* If not specified in DT, defaults to 64 */ .fifo_depth = 64, Is that intentional or is it some leftover that was meant to be removed before the submission? From s3c64xx_spi_probe() it looks like the "fifo-depth" DT property is ignored if .fifo_depth is set in the port_config: if (sdd->port_conf->fifo_depth) sdd->fifo_depth = sdd->port_conf->fifo_depth; else if (of_property_read_u32(pdev->dev.of_node, "fifo-depth", &sdd->fifo_depth)) sdd->fifo_depth = FIFO_DEPTH(sdd); Btw, wouldn't it be reasonable to flip this probe() code the other way around? So that the fact that the DT property is available is prioritized, not its port_config counterpart. That would make it possible to provide a sensible default in the port_config structure and at the same time be able to override it by specifying the DT property for nodes where it's needed. Just a thought, not strictly related to this patch. > Exynos 990 data for SPI: > - The depth of the FIFO is not the same size on all nodes. > A depth of 64 bytes is used on most nodes, > while a depth of 256 bytes is used on 3 specific nodes (SPI 8/9/10). > - The Exynos 990 only allows access to 32-bit registers. > If access is attempted with a different size, an error interrupt > is generated. Therefore, it is necessary to perform write accesses to > registers in 32-bit blocks. > - To prevent potential issues when fifo-depth is not explicitly set in > DT, a default value of 64 is assigned to ensure stable operation. > > Signed-off-by: Denzeel Oliva <wachiturroxd150@gmail.com> > --- > drivers/spi/spi-s3c64xx.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c > index 389275dbc..5f55763f9 100644 > --- a/drivers/spi/spi-s3c64xx.c > +++ b/drivers/spi/spi-s3c64xx.c > @@ -1586,6 +1586,20 @@ static const struct s3c64xx_spi_port_config exynos850_spi_port_config = { > .quirks = S3C64XX_SPI_QUIRK_CS_AUTO, > }; > > +static const struct s3c64xx_spi_port_config exynos990_spi_port_config = { > + /* If not specified in DT, defaults to 64 */ > + .fifo_depth = 64, Talking about this line here. > + .rx_fifomask = S3C64XX_SPI_ST_RX_FIFO_RDY_V2, > + .tx_fifomask = S3C64XX_SPI_ST_TX_FIFO_RDY_V2, > + .tx_st_done = 25, > + .clk_div = 4, > + .high_speed = true, > + .clk_from_cmu = true, > + .has_loopback = true, > + .use_32bit_io = true, > + .quirks = S3C64XX_SPI_QUIRK_CS_AUTO, > +}; > + > static const struct s3c64xx_spi_port_config exynosautov9_spi_port_config = { > /* fifo_lvl_mask is deprecated. Use {rx, tx}_fifomask instead. */ > .fifo_lvl_mask = { 0x1ff, 0x1ff, 0x7f, 0x7f, 0x7f, 0x7f, 0x1ff, 0x7f, > @@ -1664,6 +1678,9 @@ static const struct of_device_id s3c64xx_spi_dt_match[] = { > { .compatible = "samsung,exynos850-spi", > .data = &exynos850_spi_port_config, > }, > + { .compatible = "samsung,exynos990-spi", > + .data = &exynos990_spi_port_config, > + }, > { .compatible = "samsung,exynosautov9-spi", > .data = &exynosautov9_spi_port_config, > }, > -- > 2.48.1 > >
Hi, Sam, On 2/14/25 12:08 AM, Sam Protsenko wrote: > On Thu, Feb 13, 2025 at 2:41 PM Denzeel Oliva <wachiturroxd150@gmail.com> wrote: >> >> Exynos990 uses the same version of USI SPI (v2.1) as the GS101. >> Removed fifo_lvl_mask and rx_lvl_offset, and changed to the new data >> configuration port. >> >> The difference from other new port configuration data is that fifo_depth >> is only specified in fifo-depth in DT. >> > > In the code below I can see this bit: > > /* If not specified in DT, defaults to 64 */ > .fifo_depth = 64, > > Is that intentional or is it some leftover that was meant to be > removed before the submission? From s3c64xx_spi_probe() it looks like > the "fifo-depth" DT property is ignored if .fifo_depth is set in the > port_config: fifo-depth in port config is intended for IPs where all their instances use the same FIFO depth. fifo-depth from DT is ignored because the compatible knows better than what developers may in DT in this case, it is intentional. > > if (sdd->port_conf->fifo_depth) > sdd->fifo_depth = sdd->port_conf->fifo_depth; > else if (of_property_read_u32(pdev->dev.of_node, "fifo-depth", > &sdd->fifo_depth)) > sdd->fifo_depth = FIFO_DEPTH(sdd); > > Btw, wouldn't it be reasonable to flip this probe() code the other way No, please. IPs that have instances with different FIFO depths shall rely only on DT to specify their FIFO depths. Cheers, ta
On Fri, Feb 14, 2025 at 12:39 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > Hi, Sam, > > On 2/14/25 12:08 AM, Sam Protsenko wrote: > > On Thu, Feb 13, 2025 at 2:41 PM Denzeel Oliva <wachiturroxd150@gmail.com> wrote: > >> > >> Exynos990 uses the same version of USI SPI (v2.1) as the GS101. > >> Removed fifo_lvl_mask and rx_lvl_offset, and changed to the new data > >> configuration port. > >> > >> The difference from other new port configuration data is that fifo_depth > >> is only specified in fifo-depth in DT. > >> > > > > In the code below I can see this bit: > > > > /* If not specified in DT, defaults to 64 */ > > .fifo_depth = 64, > > > > Is that intentional or is it some leftover that was meant to be > > removed before the submission? From s3c64xx_spi_probe() it looks like > > the "fifo-depth" DT property is ignored if .fifo_depth is set in the > > port_config: > > fifo-depth in port config is intended for IPs where all their instances > use the same FIFO depth. fifo-depth from DT is ignored because the > compatible knows better than what developers may in DT in this case, it > is intentional. > > > > > if (sdd->port_conf->fifo_depth) > > sdd->fifo_depth = sdd->port_conf->fifo_depth; > > else if (of_property_read_u32(pdev->dev.of_node, "fifo-depth", > > &sdd->fifo_depth)) > > sdd->fifo_depth = FIFO_DEPTH(sdd); > > > > Btw, wouldn't it be reasonable to flip this probe() code the other way > > No, please. IPs that have instances with different FIFO depths shall > rely only on DT to specify their FIFO depths. > Fair enough. Does it mean the port_config.fifo_depth should be made obsolete? Or it makes sense for older SoCs where FIFO depth is fixed, or something like that? > Cheers, > ta
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c index 389275dbc..5f55763f9 100644 --- a/drivers/spi/spi-s3c64xx.c +++ b/drivers/spi/spi-s3c64xx.c @@ -1586,6 +1586,20 @@ static const struct s3c64xx_spi_port_config exynos850_spi_port_config = { .quirks = S3C64XX_SPI_QUIRK_CS_AUTO, }; +static const struct s3c64xx_spi_port_config exynos990_spi_port_config = { + /* If not specified in DT, defaults to 64 */ + .fifo_depth = 64, + .rx_fifomask = S3C64XX_SPI_ST_RX_FIFO_RDY_V2, + .tx_fifomask = S3C64XX_SPI_ST_TX_FIFO_RDY_V2, + .tx_st_done = 25, + .clk_div = 4, + .high_speed = true, + .clk_from_cmu = true, + .has_loopback = true, + .use_32bit_io = true, + .quirks = S3C64XX_SPI_QUIRK_CS_AUTO, +}; + static const struct s3c64xx_spi_port_config exynosautov9_spi_port_config = { /* fifo_lvl_mask is deprecated. Use {rx, tx}_fifomask instead. */ .fifo_lvl_mask = { 0x1ff, 0x1ff, 0x7f, 0x7f, 0x7f, 0x7f, 0x1ff, 0x7f, @@ -1664,6 +1678,9 @@ static const struct of_device_id s3c64xx_spi_dt_match[] = { { .compatible = "samsung,exynos850-spi", .data = &exynos850_spi_port_config, }, + { .compatible = "samsung,exynos990-spi", + .data = &exynos990_spi_port_config, + }, { .compatible = "samsung,exynosautov9-spi", .data = &exynosautov9_spi_port_config, },
Exynos990 uses the same version of USI SPI (v2.1) as the GS101. Removed fifo_lvl_mask and rx_lvl_offset, and changed to the new data configuration port. The difference from other new port configuration data is that fifo_depth is only specified in fifo-depth in DT. Exynos 990 data for SPI: - The depth of the FIFO is not the same size on all nodes. A depth of 64 bytes is used on most nodes, while a depth of 256 bytes is used on 3 specific nodes (SPI 8/9/10). - The Exynos 990 only allows access to 32-bit registers. If access is attempted with a different size, an error interrupt is generated. Therefore, it is necessary to perform write accesses to registers in 32-bit blocks. - To prevent potential issues when fifo-depth is not explicitly set in DT, a default value of 64 is assigned to ensure stable operation. Signed-off-by: Denzeel Oliva <wachiturroxd150@gmail.com> --- drivers/spi/spi-s3c64xx.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)