| Message ID | 20230404060011.108561-2-jaewon02.kim@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: s3c64xx: improve SPI polling mode | expand |
On Tue, Apr 04, 2023 at 03:00:09PM +0900, Jaewon Kim wrote: > This patch adds new 'samsung,spi-polling' property to support polling mode. > In some environments, polling mode is required even if DMA is supported. > Changed it to support not only with quick but also optinally with > devicetree. Why would this be required if we can use DMA? If this is a performance optimisation for small messages the driver should just work out when to choose PIO over DMA like other drivers do. It is hard to see this as a hardware property which should be configured via DT.
Hello Mark, On 23. 4. 4. 19:54, Mark Brown wrote: > On Tue, Apr 04, 2023 at 03:00:09PM +0900, Jaewon Kim wrote: >> This patch adds new 'samsung,spi-polling' property to support polling mode. >> In some environments, polling mode is required even if DMA is supported. >> Changed it to support not only with quick but also optinally with >> devicetree. > Why would this be required if we can use DMA? If this is a performance > optimisation for small messages the driver should just work out when to > choose PIO over DMA like other drivers do. It is hard to see this as a > hardware property which should be configured via DT. We are providing a VM environment in which several Guest OSs are running. If Host OS has DMA, GuestOS should use SPI as polling mode. In order to support s3c64xx in a DMA-less environment, it must be separated with a quirk. However, there is DMA in the Host OS and no DMA in the Guest OS, it is not correct to separate them with quirk. I'm considering supporting this systems with DeviceTree rather than qurik. If 'samsung,spi-polling' looks to be a SW configuration, how about 'samsung,no-dma'. This is not to simply change the mode using DeviceTree, but to support an environment without DMA. Thanks Jaewon Kim
On Tue, Apr 04, 2023 at 08:17:13PM +0900, Jaewon Kim wrote: > On 23. 4. 4. 19:54, Mark Brown wrote: > > On Tue, Apr 04, 2023 at 03:00:09PM +0900, Jaewon Kim wrote: > >> This patch adds new 'samsung,spi-polling' property to support polling mode. > >> In some environments, polling mode is required even if DMA is supported. > >> Changed it to support not only with quick but also optinally with > >> devicetree. > > Why would this be required if we can use DMA? If this is a performance > > optimisation for small messages the driver should just work out when to > > choose PIO over DMA like other drivers do. It is hard to see this as a > > hardware property which should be configured via DT. > We are providing a VM environment in which several Guest OSs are running. > If Host OS has DMA, GuestOS should use SPI as polling mode. This sounds like some sort of virtualised environment with passthrough? If that's the case then the host OS will be in control of the device tree provided to the guest so it simply shouldn't be describing the DMA configuration if it doesn't want the guest to use DMA for some reason. There's no value in describing the DMA the guest shouldn't use then providing an additional property telling the guest not to pay attention to the DMA when we could simply not do the first step.
Hello Mark, On 23. 4. 4. 20:41, Mark Brown wrote: > On Tue, Apr 04, 2023 at 08:17:13PM +0900, Jaewon Kim wrote: >> On 23. 4. 4. 19:54, Mark Brown wrote: >>> On Tue, Apr 04, 2023 at 03:00:09PM +0900, Jaewon Kim wrote: >>>> This patch adds new 'samsung,spi-polling' property to support polling mode. >>>> In some environments, polling mode is required even if DMA is supported. >>>> Changed it to support not only with quick but also optinally with >>>> devicetree. >>> Why would this be required if we can use DMA? If this is a performance >>> optimisation for small messages the driver should just work out when to >>> choose PIO over DMA like other drivers do. It is hard to see this as a >>> hardware property which should be configured via DT. >> We are providing a VM environment in which several Guest OSs are running. >> If Host OS has DMA, GuestOS should use SPI as polling mode. > This sounds like some sort of virtualised environment with passthrough? > If that's the case then the host OS will be in control of the device > tree provided to the guest so it simply shouldn't be describing the DMA > configuration if it doesn't want the guest to use DMA for some reason. > There's no value in describing the DMA the guest shouldn't use then > providing an additional property telling the guest not to pay attention > to the DMA when we could simply not do the first step. Is it correct in your opinion to change to polling mode if there is no DMA describing in DeviceTree? Currently, if there is no DMA, the probe failed in s3c64xx driver. So I added the "samsung,spi-polling" property not to check DMA. If your opinion is to switch to Polling mode if there is no DMA, I will fix it in the next version. Thanks Jaewon Kim
On Tue, Apr 04, 2023 at 09:22:25PM +0900, Jaewon Kim wrote: > On 23. 4. 4. 20:41, Mark Brown wrote: > > There's no value in describing the DMA the guest shouldn't use then > > providing an additional property telling the guest not to pay attention > > to the DMA when we could simply not do the first step. > Is it correct in your opinion to change to polling mode if there is no > DMA describing in DeviceTree? Yes, exactly. > Currently, if there is no DMA, the probe failed in s3c64xx driver. > So I added the "samsung,spi-polling" property not to check DMA. > If your opinion is to switch to Polling mode if there is no DMA, I will > fix it in the next version. Great, that sounds like a better solution. If there is a description of DMA but it can't be fetched then an error should be right, but if there's just no DMA described then switching to polling mode seems better.
On 04/04/2023 08:00, Jaewon Kim wrote: > This patch adds new 'samsung,spi-polling' property to support polling mode. Do not use "This commit/patch", but imperative mood. See: https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 Also, binding should be before its usage. > In some environments, polling mode is required even if DMA is supported. Why? What are these environments? You need to explain all this in commit msg. > Changed it to support not only with quick but also optinally with typo: optionally > devicetree. > Best regards, Krzysztof
On 23. 4. 5. 14:42, Krzysztof Kozlowski wrote: > On 04/04/2023 08:00, Jaewon Kim wrote: >> This patch adds new 'samsung,spi-polling' property to support polling mode. > Do not use "This commit/patch", but imperative mood. See: > https://protect2.fireeye.com/v1/url?k=3cb451b1-5d3f4488-3cb5dafe-000babffae10-4326e9d41dfad262&q=1&e=1125b69d-6d9e-4c91-a8fd-3470cd2278e4&u=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.17.1%2Fsource%2FDocumentation%2Fprocess%2Fsubmitting-patches.rst%23L95 > > Also, binding should be before its usage. thanks. I will refer to it in next version. >> In some environments, polling mode is required even if DMA is supported. > Why? What are these environments? You need to explain all this in commit > msg. > We are providing a VM environment in which several Guest OSs are running. There are cases where DMA exist only in HostOS and not exist in GuestOS. In this case, SPI in GuestOS runs with polling mode. I thought it was correct that the polling mode was supported optional, not quirk. I have plan to change the polling mode if there is no 'dmas' property. How about your opinion? >> Changed it to support not only with quick but also optinally with > typo: optionally > >> devicetree. >> > Best regards, > Krzysztof > > Thanks Jaewon Kim
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c index 71d324ec9a70..bf1f3dcca202 100644 --- a/drivers/spi/spi-s3c64xx.c +++ b/drivers/spi/spi-s3c64xx.c @@ -116,7 +116,6 @@ #define S3C64XX_SPI_TRAILCNT S3C64XX_SPI_MAX_TRAILCNT #define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t) -#define is_polling(x) (x->port_conf->quirks & S3C64XX_SPI_QUIRK_POLL) #define RXBUSY (1<<2) #define TXBUSY (1<<3) @@ -198,6 +197,17 @@ struct s3c64xx_spi_driver_data { unsigned int port_id; }; +static bool s3c64xx_is_polling(struct s3c64xx_spi_driver_data *sdd) +{ + if (sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_POLL) + return true; + + if (sdd->cntrlr_info->polling) + return true; + + return false; +} + static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd) { void __iomem *regs = sdd->regs; @@ -353,7 +363,7 @@ static int s3c64xx_spi_prepare_transfer(struct spi_master *spi) { struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(spi); - if (is_polling(sdd)) + if (s3c64xx_is_polling(sdd)) return 0; /* Requests DMA channels */ @@ -383,7 +393,7 @@ static int s3c64xx_spi_unprepare_transfer(struct spi_master *spi) { struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(spi); - if (is_polling(sdd)) + if (s3c64xx_is_polling(sdd)) return 0; /* Releases DMA channels if they are allocated */ @@ -749,7 +759,7 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master, return status; } - if (!is_polling(sdd) && (xfer->len > fifo_len) && + if (!s3c64xx_is_polling(sdd) && (xfer->len > fifo_len) && sdd->rx_dma.ch && sdd->tx_dma.ch) { use_dma = 1; @@ -1067,6 +1077,7 @@ static struct s3c64xx_spi_info *s3c64xx_spi_parse_dt(struct device *dev) sci->num_cs = temp; } + sci->polling = of_property_read_bool(dev->of_node, "samsung,spi-polling"); sci->no_cs = of_property_read_bool(dev->of_node, "no-cs-readback"); return sci; @@ -1171,7 +1182,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev) if (sdd->port_conf->has_loopback) master->mode_bits |= SPI_LOOP; master->auto_runtime_pm = true; - if (!is_polling(sdd)) + if (!s3c64xx_is_polling(sdd)) master->can_dma = s3c64xx_spi_can_dma; sdd->regs = devm_ioremap_resource(&pdev->dev, mem_res); @@ -1295,7 +1306,7 @@ static int s3c64xx_spi_remove(struct platform_device *pdev) writel(0, sdd->regs + S3C64XX_SPI_INT_EN); - if (!is_polling(sdd)) { + if (!s3c64xx_is_polling(sdd)) { dma_release_channel(sdd->rx_dma.ch); dma_release_channel(sdd->tx_dma.ch); } diff --git a/include/linux/platform_data/spi-s3c64xx.h b/include/linux/platform_data/spi-s3c64xx.h index 5df1ace6d2c9..cb7b8ddc899f 100644 --- a/include/linux/platform_data/spi-s3c64xx.h +++ b/include/linux/platform_data/spi-s3c64xx.h @@ -35,6 +35,7 @@ struct s3c64xx_spi_info { int src_clk_nr; int num_cs; bool no_cs; + bool polling; int (*cfg_gpio)(void); };
This patch adds new 'samsung,spi-polling' property to support polling mode. In some environments, polling mode is required even if DMA is supported. Changed it to support not only with quick but also optinally with devicetree. Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com> --- drivers/spi/spi-s3c64xx.c | 23 +++++++++++++++++------ include/linux/platform_data/spi-s3c64xx.h | 1 + 2 files changed, 18 insertions(+), 6 deletions(-)