| Message ID | 20240123153421.715951-1-tudor.ambarus@linaro.org |
|---|---|
| Headers | show |
| Series | spi: s3c64xx: winter cleanup and gs101 support | expand |
On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > I'm working with the samsung SPI driver and I'd like to review further > patches on this driver. Add myself as reviewer. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> > MAINTAINERS | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 8d1052fa6a69..b9cde7ed8489 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -19404,6 +19404,7 @@ F: include/linux/clk/samsung.h > > SAMSUNG SPI DRIVERS > M: Andi Shyti <andi.shyti@kernel.org> > +R: Tudor Ambarus <tudor.ambarus@linaro.org> > L: linux-spi@vger.kernel.org > L: linux-samsung-soc@vger.kernel.org > S: Maintained > -- > 2.43.0.429.g432eaa2c6b-goog >
On Tue, Jan 23, 2024 at 03:33:59PM +0000, Tudor Ambarus wrote: > The patch set cleans a bit the driver and adds support for gs101 SPI. > > Apart of the SPI patches, I added support for iowrite{8,16}_32 accessors > in asm-generic/io.h. This will allow devices that require 32 bits > register accesses to write data in chunks of 8 or 16 bits (a typical use > case is SPI, where clients can request transfers in words of 8 bits for > example). GS101 only allows 32bit register accesses otherwise it raisses > a Serror Interrupt and hangs the system, thus the accessors are needed > here. If the accessors are fine, I expect they'll be queued either to > the SPI tree or to the ASM header files tree, but by providing an > immutable tag, so that the other tree can merge them too. > > The SPI patches were tested with the spi-loopback-test on the gs101 > controller. The reformatting in this series will conflict with the SPI changes in: https://lore.kernel.org/r/20240120012948.8836-1-semen.protsenko@linaro.org Can you please pull those into this series or otherwise coordinate?
On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > Hi, > > The patch set cleans a bit the driver and adds support for gs101 SPI. > It might be more convenient (for review purposes) to extract all the cleanup patches into a separate series, and base it on top of the gs101 SPI enablement series. > Apart of the SPI patches, I added support for iowrite{8,16}_32 accessors > in asm-generic/io.h. This will allow devices that require 32 bits > register accesses to write data in chunks of 8 or 16 bits (a typical use > case is SPI, where clients can request transfers in words of 8 bits for > example). GS101 only allows 32bit register accesses otherwise it raisses > a Serror Interrupt and hangs the system, thus the accessors are needed > here. If the accessors are fine, I expect they'll be queued either to > the SPI tree or to the ASM header files tree, but by providing an > immutable tag, so that the other tree can merge them too. > > The SPI patches were tested with the spi-loopback-test on the gs101 > controller. > > Thanks! > ta > > Tudor Ambarus (21): > spi: dt-bindings: samsung: add google,gs101-spi compatible > spi: s3c64xx: sort headers alphabetically > spi: s3c64xx: remove extra blank line > spi: s3c64xx: remove unneeded (void *) casts in of_match_table > spi: s3c64xx: explicitly include <linux/bits.h> > spi: s3c64xx: remove else after return > spi: s3c64xx: use bitfield access macros > spi: s3c64xx: move error check up to avoid rechecking > spi: s3c64xx: use full mask for {RX, TX}_FIFO_LVL > spi: s3c64xx: move common code outside if else > spi: s3c64xx: check return code of dmaengine_slave_config() > spi: s3c64xx: propagate the dma_submit_error() error code > spi: s3c64xx: rename prepare_dma() to s3c64xx_prepare_dma() > spi: s3c64xx: return ETIMEDOUT for wait_for_completion_timeout() > spi: s3c64xx: simplify s3c64xx_wait_for_pio() > spi: s3c64xx: add missing blank line after declaration > spi: s3c64xx: downgrade dev_warn to dev_dbg for optional dt props > asm-generic/io.h: add iowrite{8,16}_32 accessors > spi: s3c64xx: add support for google,gs101-spi > spi: s3c64xx: make the SPI alias optional for newer SoCs > MAINTAINERS: add Tudor Ambarus as R for the samsung SPI driver > > .../devicetree/bindings/spi/samsung,spi.yaml | 1 + > MAINTAINERS | 1 + > drivers/spi/spi-s3c64xx.c | 447 +++++++++--------- > include/asm-generic/io.h | 50 ++ > 4 files changed, 276 insertions(+), 223 deletions(-) > > -- > 2.43.0.429.g432eaa2c6b-goog >
On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > Remove extra blank line. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> > drivers/spi/spi-s3c64xx.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c > index 187b617e3e14..26d389d95af9 100644 > --- a/drivers/spi/spi-s3c64xx.c > +++ b/drivers/spi/spi-s3c64xx.c > @@ -16,7 +16,6 @@ > #include <linux/pm_runtime.h> > #include <linux/spi/spi.h> > > - > #define MAX_SPI_PORTS 12 > #define S3C64XX_SPI_QUIRK_CS_AUTO (1 << 1) > #define AUTOSUSPEND_TIMEOUT 2000 > -- > 2.43.0.429.g432eaa2c6b-goog >
On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > Else case is not needed after a return, remove it. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> > drivers/spi/spi-s3c64xx.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c > index 9ce56aa792ed..db1e1d6ee732 100644 > --- a/drivers/spi/spi-s3c64xx.c > +++ b/drivers/spi/spi-s3c64xx.c > @@ -406,12 +406,10 @@ static bool s3c64xx_spi_can_dma(struct spi_controller *host, > { > struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host); > > - if (sdd->rx_dma.ch && sdd->tx_dma.ch) { > + if (sdd->rx_dma.ch && sdd->tx_dma.ch) > return xfer->len > FIFO_DEPTH(sdd); > - } else { > - return false; > - } > > + return false; > } > > static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd, > -- > 2.43.0.429.g432eaa2c6b-goog >
On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > Add support for GS101 SPI. All the SPI nodes on GS101 have 64 bytes > FIFOs, infer the FIFO size from the compatible. GS101 allows just 32bit > register accesses, otherwise a Serror Interrupt is raised. Do the write > reg accesses in 32 bits. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- I counted 3 different features in this patch. Would be better to split it correspondingly into 3 patches, to make patches atomic: 1. I/O width 2. FIFO size 3. Adding support for gs101 And I'm not really convinced about FIFO size change. > drivers/spi/spi-s3c64xx.c | 50 +++++++++++++++++++++++++++++++++------ > 1 file changed, 43 insertions(+), 7 deletions(-) > > diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c > index 62671b2d594a..c4ddd2859ba4 100644 > --- a/drivers/spi/spi-s3c64xx.c > +++ b/drivers/spi/spi-s3c64xx.c > @@ -20,6 +20,7 @@ > > #define MAX_SPI_PORTS 12 > #define S3C64XX_SPI_QUIRK_CS_AUTO BIT(1) > +#define S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH BIT(2) > #define AUTOSUSPEND_TIMEOUT 2000 > > /* Registers and bit-fields */ > @@ -131,6 +132,7 @@ struct s3c64xx_spi_dma_data { > * @rx_lvl_offset: Bit offset of RX_FIFO_LVL bits in SPI_STATUS regiter. > * @tx_st_done: Bit offset of TX_DONE bit in SPI_STATUS regiter. > * @clk_div: Internal clock divider > + * @fifosize: size of the FIFO > * @quirks: Bitmask of known quirks > * @high_speed: True, if the controller supports HIGH_SPEED_EN bit. > * @clk_from_cmu: True, if the controller does not include a clock mux and > @@ -149,6 +151,7 @@ struct s3c64xx_spi_port_config { > int tx_st_done; > int quirks; > int clk_div; > + unsigned int fifosize; > bool high_speed; > bool clk_from_cmu; > bool clk_ioclk; > @@ -175,6 +178,7 @@ struct s3c64xx_spi_port_config { > * @tx_dma: Local transmit DMA data (e.g. chan and direction) > * @port_conf: Local SPI port configuartion data > * @port_id: Port identification number > + * @fifosize: size of the FIFO for this port > */ > struct s3c64xx_spi_driver_data { > void __iomem *regs; > @@ -194,6 +198,7 @@ struct s3c64xx_spi_driver_data { > struct s3c64xx_spi_dma_data tx_dma; > const struct s3c64xx_spi_port_config *port_conf; > unsigned int port_id; > + unsigned int fifosize; > }; > > static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd) > @@ -403,7 +408,7 @@ static bool s3c64xx_spi_can_dma(struct spi_controller *host, > struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host); > > if (sdd->rx_dma.ch && sdd->tx_dma.ch) > - return xfer->len > FIFO_DEPTH(sdd); > + return xfer->len > sdd->fifosize; > > return false; > } > @@ -447,12 +452,22 @@ static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd, > xfer->tx_buf, xfer->len / 4); > break; > case 16: > - iowrite16_rep(regs + S3C64XX_SPI_TX_DATA, > - xfer->tx_buf, xfer->len / 2); > + if (sdd->port_conf->quirks & > + S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH) > + iowrite16_32_rep(regs + S3C64XX_SPI_TX_DATA, > + xfer->tx_buf, xfer->len / 2); > + else > + iowrite16_rep(regs + S3C64XX_SPI_TX_DATA, > + xfer->tx_buf, xfer->len / 2); > break; > default: > - iowrite8_rep(regs + S3C64XX_SPI_TX_DATA, > - xfer->tx_buf, xfer->len); > + if (sdd->port_conf->quirks & > + S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH) > + iowrite8_32_rep(regs + S3C64XX_SPI_TX_DATA, > + xfer->tx_buf, xfer->len); > + else > + iowrite8_rep(regs + S3C64XX_SPI_TX_DATA, > + xfer->tx_buf, xfer->len); > break; > } > } > @@ -696,7 +711,7 @@ static int s3c64xx_spi_transfer_one(struct spi_controller *host, > struct spi_transfer *xfer) > { > struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host); > - const unsigned int fifo_len = FIFO_DEPTH(sdd); > + const unsigned int fifo_len = sdd->fifosize; > const void *tx_buf = NULL; > void *rx_buf = NULL; > int target_len = 0, origin_len = 0; > @@ -1145,6 +1160,11 @@ static int s3c64xx_spi_probe(struct platform_device *pdev) > sdd->port_id = pdev->id; > } > > + if (sdd->port_conf->fifosize) > + sdd->fifosize = sdd->port_conf->fifosize; > + else > + sdd->fifosize = FIFO_DEPTH(sdd); > + > sdd->cur_bpw = 8; > > sdd->tx_dma.direction = DMA_MEM_TO_DEV; > @@ -1234,7 +1254,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev) > dev_dbg(&pdev->dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d with %d Targets attached\n", > sdd->port_id, host->num_chipselect); > dev_dbg(&pdev->dev, "\tIOmem=[%pR]\tFIFO %dbytes\n", > - mem_res, FIFO_DEPTH(sdd)); > + mem_res, sdd->fifosize); > > pm_runtime_mark_last_busy(&pdev->dev); > pm_runtime_put_autosuspend(&pdev->dev); > @@ -1362,6 +1382,18 @@ static const struct dev_pm_ops s3c64xx_spi_pm = { > s3c64xx_spi_runtime_resume, NULL) > }; > > +static const struct s3c64xx_spi_port_config gs101_spi_port_config = { > + .fifosize = 64, I think if you rework the the .fifo_lvl_mask, replacing it with .fifosize, you should also do next things in this series: 1. Rework it for all supported (existing) chips in this driver 2. Provide fifosize property for each SPI node for all existing dts that use this driver 3. Get rid of .fifo_lvl_mask for good. But the compatibility with older kernels has to be taken into the account here as well. Otherwise it looks like a half attempt and not finished, only creating a duplicated property/struct field for the same (already existing) thing. Because it's completely possible to do the same using just .fifo_lvl_mask without introducing new fields or properties. If it seems to much -- maybe just use .fifo_lvl_mask for now, and do all that reworking properly later, in a separate patch series? > + .rx_lvl_offset = 15, > + .tx_st_done = 25, > + .clk_div = 4, > + .high_speed = true, > + .clk_from_cmu = true, > + .has_loopback = true, > + .quirks = S3C64XX_SPI_QUIRK_CS_AUTO | > + S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH, > +}; > + > static const struct s3c64xx_spi_port_config s3c2443_spi_port_config = { > .fifo_lvl_mask = { 0x7f }, > .rx_lvl_offset = 13, > @@ -1452,6 +1484,10 @@ static const struct platform_device_id s3c64xx_spi_driver_ids[] = { > }; > > static const struct of_device_id s3c64xx_spi_dt_match[] = { > + { > + .compatible = "google,gs101-spi", > + .data = &gs101_spi_port_config, > + }, > { > .compatible = "samsung,s3c2443-spi", > .data = &s3c2443_spi_port_config, > -- > 2.43.0.429.g432eaa2c6b-goog >
On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > Add missing blank line after declaration. Move initialization in the > body of the function. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- > drivers/spi/spi-s3c64xx.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c > index f5474f3b3920..2abf5994080a 100644 > --- a/drivers/spi/spi-s3c64xx.c > +++ b/drivers/spi/spi-s3c64xx.c > @@ -1273,8 +1273,9 @@ static int s3c64xx_spi_suspend(struct device *dev) > { > struct spi_controller *host = dev_get_drvdata(dev); > struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host); > + int ret; > > - int ret = spi_controller_suspend(host); > + ret = spi_controller_suspend(host); Why not just moving the empty line below the declaration block, keeping the initialization on the variable declaration line? > if (ret) > return ret; > > -- > 2.43.0.429.g432eaa2c6b-goog >
Hi Tudor, On Tue, Jan 23, 2024 at 03:34:01PM +0000, Tudor Ambarus wrote: > Sorting headers alphabetically helps locating duplicates, > and makes it easier to figure out where to insert new headers. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Thanks, Andi
Hi Tudor, On Tue, Jan 23, 2024 at 03:34:02PM +0000, Tudor Ambarus wrote: > Remove extra blank line. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Thanks, Andi
Hi Tudor, On Tue, Jan 23, 2024 at 03:34:03PM +0000, Tudor Ambarus wrote: > of_device_id::data is an opaque pointer. No explicit cast is needed. > Remove unneeded (void *) casts in of_match_table. While here align the > compatible and data members. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Thanks, Andi
Hi Tudor, On Tue, Jan 23, 2024 at 03:34:04PM +0000, Tudor Ambarus wrote: > The driver uses GENMASK() but does not include <linux/bits.h>. > Include the missing header, we shall aim to have the drivers self > contained. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- > drivers/spi/spi-s3c64xx.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c > index b350e70fd179..9ce56aa792ed 100644 > --- a/drivers/spi/spi-s3c64xx.c > +++ b/drivers/spi/spi-s3c64xx.c > @@ -3,6 +3,7 @@ > // Copyright (c) 2009 Samsung Electronics Co., Ltd. > // Jaswinder Singh <jassi.brar@samsung.com> > > +#include <linux/bits.h> I don't see why this should be included. Are there cases when not having bits.h produces any compilation error? Andi > #include <linux/clk.h> > #include <linux/delay.h> > #include <linux/dma-mapping.h> > -- > 2.43.0.429.g432eaa2c6b-goog >
Hi Tudor, On Tue, Jan 23, 2024 at 03:34:05PM +0000, Tudor Ambarus wrote: > Else case is not needed after a return, remove it. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Thanks, Andi
On Tue, Jan 23, 2024 at 11:28:31PM +0100, Andi Shyti wrote: > On Tue, Jan 23, 2024 at 03:34:04PM +0000, Tudor Ambarus wrote: > > +#include <linux/bits.h> > I don't see why this should be included. Are there cases when > not having bits.h produces any compilation error? It is good practice to directly include all headers used, it avoids implicit dependencies and spurious breakage if someone rearranges headers and causes the implicit include to vanish.
On 1/23/24 19:00, Mark Brown wrote: > On Tue, Jan 23, 2024 at 03:33:59PM +0000, Tudor Ambarus wrote: > >> The patch set cleans a bit the driver and adds support for gs101 SPI. >> >> Apart of the SPI patches, I added support for iowrite{8,16}_32 accessors >> in asm-generic/io.h. This will allow devices that require 32 bits >> register accesses to write data in chunks of 8 or 16 bits (a typical use >> case is SPI, where clients can request transfers in words of 8 bits for >> example). GS101 only allows 32bit register accesses otherwise it raisses >> a Serror Interrupt and hangs the system, thus the accessors are needed >> here. If the accessors are fine, I expect they'll be queued either to >> the SPI tree or to the ASM header files tree, but by providing an >> immutable tag, so that the other tree can merge them too. >> >> The SPI patches were tested with the spi-loopback-test on the gs101 >> controller. > > The reformatting in this series will conflict with the SPI changes in: > > https://lore.kernel.org/r/20240120012948.8836-1-semen.protsenko@linaro.org > > Can you please pull those into this series or otherwise coordinate? ah, I haven't noticed Sam's updates. I'll rebase on top of his set and adapt if necessary. I'll review that set in a sec. Cheers, ta
Hi Tudor, On Tue, 2024-01-23 at 15:34 +0000, Tudor Ambarus wrote: > @@ -538,13 +538,8 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd, > cpu_relax(); > status = readl(regs + S3C64XX_SPI_STATUS); > } > - > } > > - /* If timed out while checking rx/tx status return error */ > - if (!val) > - return -EIO; > - This change behaviour of this function. The loop just above adjusts val and it is used to determine if there was a timeout or not: if (val && !xfer->rx_buf) { val = msecs_to_loops(10); status = readl(regs + S3C64XX_SPI_STATUS); while ((TX_FIFO_LVL(status, sdd) || !S3C64XX_SPI_ST_TX_DONE(status, sdd)) && --val) { cpu_relax(); status = readl(regs + S3C64XX_SPI_STATUS); } That doesn't work anymore now. Cheers, Andre'
On 1/24/24 09:21, André Draszik wrote: > Hi Tudor, > Hi! > On Tue, 2024-01-23 at 15:34 +0000, Tudor Ambarus wrote: >> @@ -538,13 +538,8 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd, >> cpu_relax(); >> status = readl(regs + S3C64XX_SPI_STATUS); >> } >> - >> } >> >> - /* If timed out while checking rx/tx status return error */ >> - if (!val) >> - return -EIO; >> - > > This change behaviour of this function. The loop just above adjusts val and it is used to > determine if there was a timeout or not: > > if (val && !xfer->rx_buf) { > val = msecs_to_loops(10); > status = readl(regs + S3C64XX_SPI_STATUS); > while ((TX_FIFO_LVL(status, sdd) > || !S3C64XX_SPI_ST_TX_DONE(status, sdd)) > && --val) { > cpu_relax(); > status = readl(regs + S3C64XX_SPI_STATUS); > } > Oh, yes, the timeout in this block. You're right, I'll drop the patch. Thanks! ta
On 1/23/24 19:28, Sam Protsenko wrote: > On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >> >> Add missing blank line after declaration. Move initialization in the >> body of the function. >> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> >> --- >> drivers/spi/spi-s3c64xx.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c >> index f5474f3b3920..2abf5994080a 100644 >> --- a/drivers/spi/spi-s3c64xx.c >> +++ b/drivers/spi/spi-s3c64xx.c >> @@ -1273,8 +1273,9 @@ static int s3c64xx_spi_suspend(struct device *dev) >> { >> struct spi_controller *host = dev_get_drvdata(dev); >> struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host); >> + int ret; >> >> - int ret = spi_controller_suspend(host); >> + ret = spi_controller_suspend(host); > > Why not just moving the empty line below the declaration block, > keeping the initialization on the variable declaration line? > just a matter of personal preference. I find it ugly to do an initialization at variable declaration and then to immediately check the return value in the body of the function. But I'll do as you say, just cosmetics anyway.
Hi, Sam! Thanks for the review! On 1/23/24 19:25, Sam Protsenko wrote: > On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >> >> Add support for GS101 SPI. All the SPI nodes on GS101 have 64 bytes >> FIFOs, infer the FIFO size from the compatible. GS101 allows just 32bit >> register accesses, otherwise a Serror Interrupt is raised. Do the write >> reg accesses in 32 bits. >> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> >> --- > > I counted 3 different features in this patch. Would be better to split > it correspondingly into 3 patches, to make patches atomic: > > 1. I/O width > 2. FIFO size I kept these 2 in the same patch as gs101 to exemplify their use by gs101. But I'm also fine splitting the patch in 3, will do in v2. > 3. Adding support for gs101 > > And I'm not really convinced about FIFO size change. I'll explain why it's needed below. > >> drivers/spi/spi-s3c64xx.c | 50 +++++++++++++++++++++++++++++++++------ >> 1 file changed, 43 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c >> index 62671b2d594a..c4ddd2859ba4 100644 >> --- a/drivers/spi/spi-s3c64xx.c >> +++ b/drivers/spi/spi-s3c64xx.c >> @@ -20,6 +20,7 @@ >> >> #define MAX_SPI_PORTS 12 >> #define S3C64XX_SPI_QUIRK_CS_AUTO BIT(1) >> +#define S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH BIT(2) >> #define AUTOSUSPEND_TIMEOUT 2000 >> >> /* Registers and bit-fields */ >> @@ -131,6 +132,7 @@ struct s3c64xx_spi_dma_data { >> * @rx_lvl_offset: Bit offset of RX_FIFO_LVL bits in SPI_STATUS regiter. >> * @tx_st_done: Bit offset of TX_DONE bit in SPI_STATUS regiter. >> * @clk_div: Internal clock divider >> + * @fifosize: size of the FIFO >> * @quirks: Bitmask of known quirks >> * @high_speed: True, if the controller supports HIGH_SPEED_EN bit. >> * @clk_from_cmu: True, if the controller does not include a clock mux and >> @@ -149,6 +151,7 @@ struct s3c64xx_spi_port_config { >> int tx_st_done; >> int quirks; >> int clk_div; >> + unsigned int fifosize; >> bool high_speed; >> bool clk_from_cmu; >> bool clk_ioclk; >> @@ -175,6 +178,7 @@ struct s3c64xx_spi_port_config { >> * @tx_dma: Local transmit DMA data (e.g. chan and direction) >> * @port_conf: Local SPI port configuartion data >> * @port_id: Port identification number >> + * @fifosize: size of the FIFO for this port >> */ >> struct s3c64xx_spi_driver_data { >> void __iomem *regs; >> @@ -194,6 +198,7 @@ struct s3c64xx_spi_driver_data { >> struct s3c64xx_spi_dma_data tx_dma; >> const struct s3c64xx_spi_port_config *port_conf; >> unsigned int port_id; >> + unsigned int fifosize; >> }; >> >> static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd) >> @@ -403,7 +408,7 @@ static bool s3c64xx_spi_can_dma(struct spi_controller *host, >> struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host); >> >> if (sdd->rx_dma.ch && sdd->tx_dma.ch) >> - return xfer->len > FIFO_DEPTH(sdd); >> + return xfer->len > sdd->fifosize; >> >> return false; >> } >> @@ -447,12 +452,22 @@ static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd, >> xfer->tx_buf, xfer->len / 4); >> break; >> case 16: >> - iowrite16_rep(regs + S3C64XX_SPI_TX_DATA, >> - xfer->tx_buf, xfer->len / 2); >> + if (sdd->port_conf->quirks & >> + S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH) >> + iowrite16_32_rep(regs + S3C64XX_SPI_TX_DATA, >> + xfer->tx_buf, xfer->len / 2); >> + else >> + iowrite16_rep(regs + S3C64XX_SPI_TX_DATA, >> + xfer->tx_buf, xfer->len / 2); >> break; >> default: >> - iowrite8_rep(regs + S3C64XX_SPI_TX_DATA, >> - xfer->tx_buf, xfer->len); >> + if (sdd->port_conf->quirks & >> + S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH) >> + iowrite8_32_rep(regs + S3C64XX_SPI_TX_DATA, >> + xfer->tx_buf, xfer->len); >> + else >> + iowrite8_rep(regs + S3C64XX_SPI_TX_DATA, >> + xfer->tx_buf, xfer->len); >> break; >> } >> } >> @@ -696,7 +711,7 @@ static int s3c64xx_spi_transfer_one(struct spi_controller *host, >> struct spi_transfer *xfer) >> { >> struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host); >> - const unsigned int fifo_len = FIFO_DEPTH(sdd); >> + const unsigned int fifo_len = sdd->fifosize; >> const void *tx_buf = NULL; >> void *rx_buf = NULL; >> int target_len = 0, origin_len = 0; >> @@ -1145,6 +1160,11 @@ static int s3c64xx_spi_probe(struct platform_device *pdev) >> sdd->port_id = pdev->id; >> } >> >> + if (sdd->port_conf->fifosize) >> + sdd->fifosize = sdd->port_conf->fifosize; >> + else >> + sdd->fifosize = FIFO_DEPTH(sdd); >> + >> sdd->cur_bpw = 8; >> >> sdd->tx_dma.direction = DMA_MEM_TO_DEV; >> @@ -1234,7 +1254,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev) >> dev_dbg(&pdev->dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d with %d Targets attached\n", >> sdd->port_id, host->num_chipselect); >> dev_dbg(&pdev->dev, "\tIOmem=[%pR]\tFIFO %dbytes\n", >> - mem_res, FIFO_DEPTH(sdd)); >> + mem_res, sdd->fifosize); >> >> pm_runtime_mark_last_busy(&pdev->dev); >> pm_runtime_put_autosuspend(&pdev->dev); >> @@ -1362,6 +1382,18 @@ static const struct dev_pm_ops s3c64xx_spi_pm = { >> s3c64xx_spi_runtime_resume, NULL) >> }; >> >> +static const struct s3c64xx_spi_port_config gs101_spi_port_config = { >> + .fifosize = 64, > > I think if you rework the the .fifo_lvl_mask, replacing it with > .fifosize, you should also do next things in this series: > 1. Rework it for all supported (existing) chips in this driver > 2. Provide fifosize property for each SPI node for all existing dts > that use this driver > 3. Get rid of .fifo_lvl_mask for good. But the compatibility with > older kernels has to be taken into the account here as well. We can't get rid of the .fifo_lvl_mask entirely because we need to be backward compatible with the device tree files that we have now. > > Otherwise it looks like a half attempt and not finished, only creating > a duplicated property/struct field for the same (already existing) > thing. Because it's completely possible to do the same using just > .fifo_lvl_mask without introducing new fields or properties. If it Using fifo_lvl_mask works but is wrong on multiple levels. As the code is now, the device tree spi alias is used as an index in the fifo_lvl_mask to determine the FIFO depth. I find it unacceptable to have a dependency on an alias in a driver. Not specifying an alias will make the probe fail, which is even worse. Also, the fifo_lvl_mask value does not reflect the FIFO level reg field. This is incorrect as we use partial register fields and is misleading. Other problem is that the fifo_lvl_mask value is used to determine the FIFO depth which is also incorrect. The FIFO depth is dictated by the SoC implementing the IP, not by the FIFO_LVL register field. Having in mind these reasons I marked the fifo_lvl_mask and the port_id as deprecated in the next patch, we shouldn't use fifo_lvl_mask or the alias anymore. In what concerns your first 2 points, to rework all the compatibles and to introduce a fifosize property, I agree it would be nice to do it, but it's not mandatory, we can work in an incremental fashion. Emphasizing what is wrong, marking things as deprecated and guiding contributors on how things should be handled is good too, which I tried in the next patch. Anyway, I'll check what the reworking would involve, and if I think it wouldn't take me a terrible amount of time, I'll do it. > seems to much -- maybe just use .fifo_lvl_mask for now, and do all > that reworking properly later, in a separate patch series? > But that means to add gs101 and then to come with patches updating what I just proposed, and I'm not thrilled about it. Cheers, ta
On Wed, Jan 24, 2024 at 4:40 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > Hi, Sam! Thanks for the review! > > On 1/23/24 19:25, Sam Protsenko wrote: > > On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > >> > >> Add support for GS101 SPI. All the SPI nodes on GS101 have 64 bytes > >> FIFOs, infer the FIFO size from the compatible. GS101 allows just 32bit > >> register accesses, otherwise a Serror Interrupt is raised. Do the write > >> reg accesses in 32 bits. > >> > >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > >> --- > > > > I counted 3 different features in this patch. Would be better to split > > it correspondingly into 3 patches, to make patches atomic: > > > > 1. I/O width > > 2. FIFO size > > I kept these 2 in the same patch as gs101 to exemplify their use by > gs101. But I'm also fine splitting the patch in 3, will do in v2. > > > 3. Adding support for gs101 > > > > And I'm not really convinced about FIFO size change. > > I'll explain why it's needed below. > > > > >> drivers/spi/spi-s3c64xx.c | 50 +++++++++++++++++++++++++++++++++------ > >> 1 file changed, 43 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c > >> index 62671b2d594a..c4ddd2859ba4 100644 > >> --- a/drivers/spi/spi-s3c64xx.c > >> +++ b/drivers/spi/spi-s3c64xx.c > >> @@ -20,6 +20,7 @@ > >> > >> #define MAX_SPI_PORTS 12 > >> #define S3C64XX_SPI_QUIRK_CS_AUTO BIT(1) > >> +#define S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH BIT(2) > >> #define AUTOSUSPEND_TIMEOUT 2000 > >> > >> /* Registers and bit-fields */ > >> @@ -131,6 +132,7 @@ struct s3c64xx_spi_dma_data { > >> * @rx_lvl_offset: Bit offset of RX_FIFO_LVL bits in SPI_STATUS regiter. > >> * @tx_st_done: Bit offset of TX_DONE bit in SPI_STATUS regiter. > >> * @clk_div: Internal clock divider > >> + * @fifosize: size of the FIFO > >> * @quirks: Bitmask of known quirks > >> * @high_speed: True, if the controller supports HIGH_SPEED_EN bit. > >> * @clk_from_cmu: True, if the controller does not include a clock mux and > >> @@ -149,6 +151,7 @@ struct s3c64xx_spi_port_config { > >> int tx_st_done; > >> int quirks; > >> int clk_div; > >> + unsigned int fifosize; > >> bool high_speed; > >> bool clk_from_cmu; > >> bool clk_ioclk; > >> @@ -175,6 +178,7 @@ struct s3c64xx_spi_port_config { > >> * @tx_dma: Local transmit DMA data (e.g. chan and direction) > >> * @port_conf: Local SPI port configuartion data > >> * @port_id: Port identification number > >> + * @fifosize: size of the FIFO for this port > >> */ > >> struct s3c64xx_spi_driver_data { > >> void __iomem *regs; > >> @@ -194,6 +198,7 @@ struct s3c64xx_spi_driver_data { > >> struct s3c64xx_spi_dma_data tx_dma; > >> const struct s3c64xx_spi_port_config *port_conf; > >> unsigned int port_id; > >> + unsigned int fifosize; > >> }; > >> > >> static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd) > >> @@ -403,7 +408,7 @@ static bool s3c64xx_spi_can_dma(struct spi_controller *host, > >> struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host); > >> > >> if (sdd->rx_dma.ch && sdd->tx_dma.ch) > >> - return xfer->len > FIFO_DEPTH(sdd); > >> + return xfer->len > sdd->fifosize; > >> > >> return false; > >> } > >> @@ -447,12 +452,22 @@ static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd, > >> xfer->tx_buf, xfer->len / 4); > >> break; > >> case 16: > >> - iowrite16_rep(regs + S3C64XX_SPI_TX_DATA, > >> - xfer->tx_buf, xfer->len / 2); > >> + if (sdd->port_conf->quirks & > >> + S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH) > >> + iowrite16_32_rep(regs + S3C64XX_SPI_TX_DATA, > >> + xfer->tx_buf, xfer->len / 2); > >> + else > >> + iowrite16_rep(regs + S3C64XX_SPI_TX_DATA, > >> + xfer->tx_buf, xfer->len / 2); > >> break; > >> default: > >> - iowrite8_rep(regs + S3C64XX_SPI_TX_DATA, > >> - xfer->tx_buf, xfer->len); > >> + if (sdd->port_conf->quirks & > >> + S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH) > >> + iowrite8_32_rep(regs + S3C64XX_SPI_TX_DATA, > >> + xfer->tx_buf, xfer->len); > >> + else > >> + iowrite8_rep(regs + S3C64XX_SPI_TX_DATA, > >> + xfer->tx_buf, xfer->len); > >> break; > >> } > >> } > >> @@ -696,7 +711,7 @@ static int s3c64xx_spi_transfer_one(struct spi_controller *host, > >> struct spi_transfer *xfer) > >> { > >> struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host); > >> - const unsigned int fifo_len = FIFO_DEPTH(sdd); > >> + const unsigned int fifo_len = sdd->fifosize; > >> const void *tx_buf = NULL; > >> void *rx_buf = NULL; > >> int target_len = 0, origin_len = 0; > >> @@ -1145,6 +1160,11 @@ static int s3c64xx_spi_probe(struct platform_device *pdev) > >> sdd->port_id = pdev->id; > >> } > >> > >> + if (sdd->port_conf->fifosize) > >> + sdd->fifosize = sdd->port_conf->fifosize; > >> + else > >> + sdd->fifosize = FIFO_DEPTH(sdd); > >> + > >> sdd->cur_bpw = 8; > >> > >> sdd->tx_dma.direction = DMA_MEM_TO_DEV; > >> @@ -1234,7 +1254,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev) > >> dev_dbg(&pdev->dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d with %d Targets attached\n", > >> sdd->port_id, host->num_chipselect); > >> dev_dbg(&pdev->dev, "\tIOmem=[%pR]\tFIFO %dbytes\n", > >> - mem_res, FIFO_DEPTH(sdd)); > >> + mem_res, sdd->fifosize); > >> > >> pm_runtime_mark_last_busy(&pdev->dev); > >> pm_runtime_put_autosuspend(&pdev->dev); > >> @@ -1362,6 +1382,18 @@ static const struct dev_pm_ops s3c64xx_spi_pm = { > >> s3c64xx_spi_runtime_resume, NULL) > >> }; > >> > >> +static const struct s3c64xx_spi_port_config gs101_spi_port_config = { > >> + .fifosize = 64, > > > > I think if you rework the the .fifo_lvl_mask, replacing it with > > .fifosize, you should also do next things in this series: > > 1. Rework it for all supported (existing) chips in this driver > > 2. Provide fifosize property for each SPI node for all existing dts > > that use this driver > > 3. Get rid of .fifo_lvl_mask for good. But the compatibility with > > older kernels has to be taken into the account here as well. > > We can't get rid of the .fifo_lvl_mask entirely because we need to be > backward compatible with the device tree files that we have now. > > > > > Otherwise it looks like a half attempt and not finished, only creating > > a duplicated property/struct field for the same (already existing) > > thing. Because it's completely possible to do the same using just > > .fifo_lvl_mask without introducing new fields or properties. If it > > Using fifo_lvl_mask works but is wrong on multiple levels. > As the code is now, the device tree spi alias is used as an index in the > fifo_lvl_mask to determine the FIFO depth. I find it unacceptable to > have a dependency on an alias in a driver. Not specifying an alias will > make the probe fail, which is even worse. Also, the fifo_lvl_mask value Ok, I think that's a valid point. I probably missed the alias part when reading the patch description. I also understand we can't just remove .fifo_lvl_mask right now, as we have to keep the compatibility with older/existing out-of-tree device trees, so that the user can update the kernel image separately. > does not reflect the FIFO level reg field. This is incorrect as we use > partial register fields and is misleading. Other problem is that the > fifo_lvl_mask value is used to determine the FIFO depth which is also > incorrect. The FIFO depth is dictated by the SoC implementing the IP, > not by the FIFO_LVL register field. Having in mind these reasons I > marked the fifo_lvl_mask and the port_id as deprecated in the next > patch, we shouldn't use fifo_lvl_mask or the alias anymore. > > In what concerns your first 2 points, to rework all the compatibles and > to introduce a fifosize property, I agree it would be nice to do it, but > it's not mandatory, we can work in an incremental fashion. Emphasizing > what is wrong, marking things as deprecated and guiding contributors on > how things should be handled is good too, which I tried in the next > patch. Anyway, I'll check what the reworking would involve, and if I > think it wouldn't take me a terrible amount of time, I'll do it. > From what I understand, that shouldn't be very hard to do, just a matter of adding fifosize property to all dts's existing upstream. That would also provide a good example to follow for anyone who wants to add the support for new compatibles. But of course I can't ask you to do the extra work. My point is, with that item done, the first transition step would be finished right away. And the remaining step would be to have a strategy for .fifo_lvl_mask removal. I wonder what maintainers can suggest on that matter, and if it's doable at all. Btw, just a thought: maybe also add "deprecated" comment to each line of code where .fifo_lvl_mask is being assigned, just to make sure noone follows that style in the future (as people often tend to copy-paste existing implementation)? Because obviously we can't remove those lines for now. > > seems to much -- maybe just use .fifo_lvl_mask for now, and do all > > that reworking properly later, in a separate patch series? > > > > But that means to add gs101 and then to come with patches updating what > I just proposed, and I'm not thrilled about it. > Got it. That's fine with me. I think we don't have to have everything super-granular w.r.t. patch series split. But I'd still argue that splitting this particular patch by 3 patches would make things more atomic and thus better. > Cheers, > ta
On Wed, Jan 24, 2024 at 3:54 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > > > On 1/23/24 19:28, Sam Protsenko wrote: > > On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > >> > >> Add missing blank line after declaration. Move initialization in the > >> body of the function. > >> > >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > >> --- > >> drivers/spi/spi-s3c64xx.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c > >> index f5474f3b3920..2abf5994080a 100644 > >> --- a/drivers/spi/spi-s3c64xx.c > >> +++ b/drivers/spi/spi-s3c64xx.c > >> @@ -1273,8 +1273,9 @@ static int s3c64xx_spi_suspend(struct device *dev) > >> { > >> struct spi_controller *host = dev_get_drvdata(dev); > >> struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host); > >> + int ret; > >> > >> - int ret = spi_controller_suspend(host); > >> + ret = spi_controller_suspend(host); > > > > Why not just moving the empty line below the declaration block, > > keeping the initialization on the variable declaration line? > > > > just a matter of personal preference. I find it ugly to do an > initialization at variable declaration and then to immediately check the > return value in the body of the function. But I'll do as you say, just > cosmetics anyway. That's not like "do as I say", I'm just a mere reviewer anyway, so it's just my opinion :) You can leave it as is, and I kinda can see your point now (having actual logical operation executed in main body rather than in initialization list): Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
On Wed, Jan 24, 2024 at 01:43:55PM -0600, Sam Protsenko wrote: > On Wed, Jan 24, 2024 at 4:40 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > Using fifo_lvl_mask works but is wrong on multiple levels. > > As the code is now, the device tree spi alias is used as an index in the > > fifo_lvl_mask to determine the FIFO depth. I find it unacceptable to > > have a dependency on an alias in a driver. Not specifying an alias will > > make the probe fail, which is even worse. Also, the fifo_lvl_mask value > Ok, I think that's a valid point. I probably missed the alias part > when reading the patch description. I also understand we can't just > remove .fifo_lvl_mask right now, as we have to keep the compatibility > with older/existing out-of-tree device trees, so that the user can > update the kernel image separately. I don't really agree here, for a given compatible the FIFO depth is known so it's redundant to specify and it's much simpler to correct issues if we're not overspecifying things in the DT.
Hi Tudor, > >> The patch set cleans a bit the driver and adds support for gs101 SPI. > >> > >> Apart of the SPI patches, I added support for iowrite{8,16}_32 accessors > >> in asm-generic/io.h. This will allow devices that require 32 bits > >> register accesses to write data in chunks of 8 or 16 bits (a typical use > >> case is SPI, where clients can request transfers in words of 8 bits for > >> example). GS101 only allows 32bit register accesses otherwise it raisses > >> a Serror Interrupt and hangs the system, thus the accessors are needed > >> here. If the accessors are fine, I expect they'll be queued either to > >> the SPI tree or to the ASM header files tree, but by providing an > >> immutable tag, so that the other tree can merge them too. > >> > >> The SPI patches were tested with the spi-loopback-test on the gs101 > >> controller. > > > > The reformatting in this series will conflict with the SPI changes in: > > > > https://lore.kernel.org/r/20240120012948.8836-1-semen.protsenko@linaro.org > > > > Can you please pull those into this series or otherwise coordinate? > > ah, I haven't noticed Sam's updates. I'll rebase on top of his set and > adapt if necessary. I'll review that set in a sec. it's a long series, please give it a few days before resending it. Thanks, Andi
On Thu, Jan 25, 2024 at 4:25 PM Andi Shyti <andi.shyti@kernel.org> wrote: > > Hi Tudor, > > > >> The patch set cleans a bit the driver and adds support for gs101 SPI. > > >> > > >> Apart of the SPI patches, I added support for iowrite{8,16}_32 accessors > > >> in asm-generic/io.h. This will allow devices that require 32 bits > > >> register accesses to write data in chunks of 8 or 16 bits (a typical use > > >> case is SPI, where clients can request transfers in words of 8 bits for > > >> example). GS101 only allows 32bit register accesses otherwise it raisses > > >> a Serror Interrupt and hangs the system, thus the accessors are needed > > >> here. If the accessors are fine, I expect they'll be queued either to > > >> the SPI tree or to the ASM header files tree, but by providing an > > >> immutable tag, so that the other tree can merge them too. > > >> > > >> The SPI patches were tested with the spi-loopback-test on the gs101 > > >> controller. > > > > > > The reformatting in this series will conflict with the SPI changes in: > > > > > > https://lore.kernel.org/r/20240120012948.8836-1-semen.protsenko@linaro.org > > > > > > Can you please pull those into this series or otherwise coordinate? > > > > ah, I haven't noticed Sam's updates. I'll rebase on top of his set and > > adapt if necessary. I'll review that set in a sec. > > it's a long series, please give it a few days before resending > it. > Also, I recommend splitting it up in a way I suggested before: 1. First add gs101 support with minimal amount of patches (without .fifosize introduction, etc) 2. Then do all those cleanups and reworks on top of that The reason why I think it's better than doing that vice-versa is that I feel (2) can take a lot of time/review rounds to get polished and accepted. So this way you can make sure gs101 support gets applied sooner. It'll also make it easier to do the backporting work later, if that's ever needed. > Thanks, > Andi
Hi Tudor, On Tue, Jan 23, 2024 at 03:34:06PM +0000, Tudor Ambarus wrote: > Use the bitfield access macros in order to clean and to make the driver > easier to read. most of the changes done here are allignment. I would mention it in the log. > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- ... > -#define S3C64XX_SPI_CLKSEL_SRCMSK (3<<9) > -#define S3C64XX_SPI_CLKSEL_SRCSHFT 9 > -#define S3C64XX_SPI_ENCLK_ENABLE (1<<8) > -#define S3C64XX_SPI_PSR_MASK 0xff > - > -#define S3C64XX_SPI_MODE_CH_TSZ_BYTE (0<<29) > -#define S3C64XX_SPI_MODE_CH_TSZ_HALFWORD (1<<29) > -#define S3C64XX_SPI_MODE_CH_TSZ_WORD (2<<29) > -#define S3C64XX_SPI_MODE_CH_TSZ_MASK (3<<29) > -#define S3C64XX_SPI_MODE_BUS_TSZ_BYTE (0<<17) > -#define S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD (1<<17) > -#define S3C64XX_SPI_MODE_BUS_TSZ_WORD (2<<17) > -#define S3C64XX_SPI_MODE_BUS_TSZ_MASK (3<<17) > +#define S3C64XX_SPI_CH_CFG 0x00 > +#define S3C64XX_SPI_CLK_CFG 0x04 > +#define S3C64XX_SPI_MODE_CFG 0x08 > +#define S3C64XX_SPI_CS_REG 0x0C > +#define S3C64XX_SPI_INT_EN 0x10 > +#define S3C64XX_SPI_STATUS 0x14 > +#define S3C64XX_SPI_TX_DATA 0x18 > +#define S3C64XX_SPI_RX_DATA 0x1C > +#define S3C64XX_SPI_PACKET_CNT 0x20 > +#define S3C64XX_SPI_PENDING_CLR 0x24 > +#define S3C64XX_SPI_SWAP_CFG 0x28 > +#define S3C64XX_SPI_FB_CLK 0x2C > + > +#define S3C64XX_SPI_CH_HS_EN BIT(6) /* High Speed Enable */ > +#define S3C64XX_SPI_CH_SW_RST BIT(5) > +#define S3C64XX_SPI_CH_SLAVE BIT(4) > +#define S3C64XX_SPI_CPOL_L BIT(3) > +#define S3C64XX_SPI_CPHA_B BIT(2) > +#define S3C64XX_SPI_CH_RXCH_ON BIT(1) > +#define S3C64XX_SPI_CH_TXCH_ON BIT(0) > + > +#define S3C64XX_SPI_CLKSEL_SRCMSK GENMASK(10, 9) > +#define S3C64XX_SPI_ENCLK_ENABLE BIT(8) > +#define S3C64XX_SPI_PSR_MASK GENMASK(15, 0) I find it easier as 0xff to be honest, but I'm not going to be picky. > + > +#define S3C64XX_SPI_MODE_CH_TSZ_MASK GENMASK(30, 29) > +#define S3C64XX_SPI_MODE_CH_TSZ_BYTE 0 > +#define S3C64XX_SPI_MODE_CH_TSZ_HALFWORD 1 > +#define S3C64XX_SPI_MODE_CH_TSZ_WORD 2 I personally find this pattern harder to read. Perhaps you can already define these as FIELD_PREP here. > +#define S3C64XX_SPI_MAX_TRAILCNT_MASK GENMASK(28, 19) > +#define S3C64XX_SPI_MODE_BUS_TSZ_MASK GENMASK(18, 17) ... > -#define S3C64XX_SPI_FBCLK_MSK (3<<0) > +#define S3C64XX_SPI_FBCLK_MASK GENMASK(1, 0) 0x3 to me is more understandable than (3<<0) and GENMASK(1, 0). Bit operation defines should be used when they really simplify the reading. But when they make it more difficult, then, I don't see the point. Overall looks good, though. Andi
Hi, The patch set cleans a bit the driver and adds support for gs101 SPI. Apart of the SPI patches, I added support for iowrite{8,16}_32 accessors in asm-generic/io.h. This will allow devices that require 32 bits register accesses to write data in chunks of 8 or 16 bits (a typical use case is SPI, where clients can request transfers in words of 8 bits for example). GS101 only allows 32bit register accesses otherwise it raisses a Serror Interrupt and hangs the system, thus the accessors are needed here. If the accessors are fine, I expect they'll be queued either to the SPI tree or to the ASM header files tree, but by providing an immutable tag, so that the other tree can merge them too. The SPI patches were tested with the spi-loopback-test on the gs101 controller. Thanks! ta Tudor Ambarus (21): spi: dt-bindings: samsung: add google,gs101-spi compatible spi: s3c64xx: sort headers alphabetically spi: s3c64xx: remove extra blank line spi: s3c64xx: remove unneeded (void *) casts in of_match_table spi: s3c64xx: explicitly include <linux/bits.h> spi: s3c64xx: remove else after return spi: s3c64xx: use bitfield access macros spi: s3c64xx: move error check up to avoid rechecking spi: s3c64xx: use full mask for {RX, TX}_FIFO_LVL spi: s3c64xx: move common code outside if else spi: s3c64xx: check return code of dmaengine_slave_config() spi: s3c64xx: propagate the dma_submit_error() error code spi: s3c64xx: rename prepare_dma() to s3c64xx_prepare_dma() spi: s3c64xx: return ETIMEDOUT for wait_for_completion_timeout() spi: s3c64xx: simplify s3c64xx_wait_for_pio() spi: s3c64xx: add missing blank line after declaration spi: s3c64xx: downgrade dev_warn to dev_dbg for optional dt props asm-generic/io.h: add iowrite{8,16}_32 accessors spi: s3c64xx: add support for google,gs101-spi spi: s3c64xx: make the SPI alias optional for newer SoCs MAINTAINERS: add Tudor Ambarus as R for the samsung SPI driver .../devicetree/bindings/spi/samsung,spi.yaml | 1 + MAINTAINERS | 1 + drivers/spi/spi-s3c64xx.c | 447 +++++++++--------- include/asm-generic/io.h | 50 ++ 4 files changed, 276 insertions(+), 223 deletions(-)