This patchset improves polling mode of s3c64xx driver.
s3cx64xx driver was supporing polling mode using quirk for SOC without DMA.
However there are situations in which spi polling mode should be
supported in some environments. To solve this situation, polling mode
can be used selectively by using device-tree as well as quirk.
When using the existing polling mode, the CPU utilization goes up to
100% becuase it checks register too frequently. In order to improve this
problem, IRQ is used by using the FIFO Trigger Level function.
Jaewon Kim (3):
spi: s3c64xx: support spi polling mode using devicetree
spi: dt-bindings: samsung: add samsung,spi-polling property
spi: s3c64xx: support interrupt based pio mode
.../devicetree/bindings/spi/samsung,spi.yaml | 6 ++
drivers/spi/spi-s3c64xx.c | 77 ++++++++++++++++---
include/linux/platform_data/spi-s3c64xx.h | 1 +
3 files changed, 72 insertions(+), 12 deletions(-)
--
2.17.1
This patch adds "samsung,spi-polling" property.
It is method to check data trans by polling when DMA is not used.
Signed-off-by: Jaewon Kim <[email protected]>
---
Documentation/devicetree/bindings/spi/samsung,spi.yaml | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/spi/samsung,spi.yaml b/Documentation/devicetree/bindings/spi/samsung,spi.yaml
index e0a465d70b0a..b4942fe160f2 100644
--- a/Documentation/devicetree/bindings/spi/samsung,spi.yaml
+++ b/Documentation/devicetree/bindings/spi/samsung,spi.yaml
@@ -69,6 +69,12 @@ properties:
$ref: /schemas/types.yaml#/definitions/uint32
default: 0
+ samsung,spi-polling:
+ description:
+ Polling SPI data without DMA transfer.
+ type: boolean
+ default: 0
+
reg:
maxItems: 1
--
2.17.1
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 <[email protected]>
---
drivers/spi/spi-s3c64xx.c | 23 +++++++++++++++++------
include/linux/platform_data/spi-s3c64xx.h | 1 +
2 files changed, 18 insertions(+), 6 deletions(-)
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);
};
--
2.17.1
This patch adds IRQ based PIO mode instead of cpu polling.
By using the FIFO trigger level, interrupts are received.
CPU consumption is reduced in PIO mode because registers are not
constantly checked.
Signed-off-by: Jaewon Kim <[email protected]>
---
drivers/spi/spi-s3c64xx.c | 54 ++++++++++++++++++++++++++++++++++-----
1 file changed, 48 insertions(+), 6 deletions(-)
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index bf1f3dcca202..f8986b05309e 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -59,6 +59,8 @@
#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_MODE_RX_RDY_LVL GENMASK(16, 11)
+#define S3C64XX_SPI_MODE_RX_RDY_LVL_SHIFT 11
#define S3C64XX_SPI_MODE_SELF_LOOPBACK (1<<3)
#define S3C64XX_SPI_MODE_RXDMA_ON (1<<2)
#define S3C64XX_SPI_MODE_TXDMA_ON (1<<1)
@@ -567,6 +569,7 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
{
void __iomem *regs = sdd->regs;
unsigned long val;
+ unsigned long time;
u32 status;
int loops;
u32 cpy_len;
@@ -577,6 +580,11 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
ms = xfer->len * 8 * 1000 / sdd->cur_speed;
ms += 10; /* some tolerance */
+ val = msecs_to_jiffies(ms);
+ time = wait_for_completion_timeout(&sdd->xfer_completion, val);
+ if (!time)
+ return -EIO;
+
val = msecs_to_loops(ms);
do {
status = readl(regs + S3C64XX_SPI_STATUS);
@@ -743,6 +751,8 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master,
u32 speed;
u8 bpw;
unsigned long flags;
+ u32 rdy_lv;
+ u32 val;
reinit_completion(&sdd->xfer_completion);
@@ -763,17 +773,41 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master,
sdd->rx_dma.ch && sdd->tx_dma.ch) {
use_dma = 1;
- } else if (xfer->len > fifo_len) {
+ } else if (xfer->len >= fifo_len) {
tx_buf = xfer->tx_buf;
rx_buf = xfer->rx_buf;
origin_len = xfer->len;
-
target_len = xfer->len;
- if (xfer->len > fifo_len)
- xfer->len = fifo_len;
+ xfer->len = fifo_len - 1;
}
do {
+ if (!use_dma) {
+ reinit_completion(&sdd->xfer_completion);
+
+ rdy_lv = xfer->len;
+ /* Setup RDY_FIFO trigger Level
+ * RDY_LVL =
+ * fifo_lvl upto 64 byte -> N bytes
+ * 128 byte -> RDY_LVL * 2 bytes
+ * 256 byte -> RDY_LVL * 4 bytes
+ */
+ if (fifo_len == 128)
+ rdy_lv /= 2;
+ else if (fifo_len == 256)
+ rdy_lv /= 4;
+
+ val = readl(sdd->regs + S3C64XX_SPI_MODE_CFG);
+ val &= ~S3C64XX_SPI_MODE_RX_RDY_LVL;
+ val |= (rdy_lv << S3C64XX_SPI_MODE_RX_RDY_LVL_SHIFT);
+ writel(val, sdd->regs + S3C64XX_SPI_MODE_CFG);
+
+ /* Enable FIFO_RDY_EN IRQ */
+ val = readl(sdd->regs + S3C64XX_SPI_INT_EN);
+ writel((val | S3C64XX_SPI_INT_RX_FIFORDY_EN),
+ sdd->regs + S3C64XX_SPI_INT_EN);
+
+ }
spin_lock_irqsave(&sdd->lock, flags);
/* Pending only which is to be done */
@@ -834,8 +868,8 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master,
if (xfer->rx_buf)
xfer->rx_buf += xfer->len;
- if (target_len > fifo_len)
- xfer->len = fifo_len;
+ if (target_len >= fifo_len)
+ xfer->len = fifo_len - 1;
else
xfer->len = target_len;
}
@@ -1005,6 +1039,14 @@ static irqreturn_t s3c64xx_spi_irq(int irq, void *data)
dev_err(&spi->dev, "TX underrun\n");
}
+ if (val & S3C64XX_SPI_ST_RX_FIFORDY) {
+ complete(&sdd->xfer_completion);
+ /* No pending clear irq, turn-off INT_EN_RX_FIFO_RDY */
+ val = readl(sdd->regs + S3C64XX_SPI_INT_EN);
+ writel((val & ~S3C64XX_SPI_INT_RX_FIFORDY_EN),
+ sdd->regs + S3C64XX_SPI_INT_EN);
+ }
+
/* Clear the pending irq by setting and then clearing it */
writel(clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
writel(0, sdd->regs + S3C64XX_SPI_PENDING_CLR);
--
2.17.1
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, 04 Apr 2023 15:00:10 +0900, Jaewon Kim wrote:
> This patch adds "samsung,spi-polling" property.
> It is method to check data trans by polling when DMA is not used.
>
> Signed-off-by: Jaewon Kim <[email protected]>
> ---
> Documentation/devicetree/bindings/spi/samsung,spi.yaml | 6 ++++++
> 1 file changed, 6 insertions(+)
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/spi/samsung,spi.yaml: properties:samsung,spi-polling: 'oneOf' conditional failed, one must be fixed:
Additional properties are not allowed ('default' was unexpected)
hint: A vendor boolean property can use "type: boolean"
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/spi/samsung,spi.yaml: properties:samsung,spi-polling: 'oneOf' conditional failed, one must be fixed:
'enum' is a required property
'const' is a required property
hint: A vendor string property with exact values has an implicit type
from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
Additional properties are not allowed ('default', 'type' were unexpected)
hint: A vendor string property with exact values has an implicit type
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/spi/samsung,spi.yaml: properties:samsung,spi-polling: 'oneOf' conditional failed, one must be fixed:
'$ref' is a required property
'allOf' is a required property
hint: A vendor property needs a $ref to types.yaml
from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
./Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml: Error in referenced schema matching $id: http://devicetree.org/schemas/spi/samsung,spi.yaml
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
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 Tue, Apr 04, 2023 at 03:00:11PM +0900, Jaewon Kim wrote:
> This patch adds IRQ based PIO mode instead of cpu polling.
> By using the FIFO trigger level, interrupts are received.
> CPU consumption is reduced in PIO mode because registers are not
> constantly checked.
Is there some lower limit where it's still worth using polling, for
example for just one or two bytes like a register address? Taking an
interrupt isn't free...
Hello Mark,
On 23. 4. 4. 21:58, Mark Brown wrote:
> On Tue, Apr 04, 2023 at 03:00:11PM +0900, Jaewon Kim wrote:
>
>> This patch adds IRQ based PIO mode instead of cpu polling.
>> By using the FIFO trigger level, interrupts are received.
>> CPU consumption is reduced in PIO mode because registers are not
>> constantly checked.
> Is there some lower limit where it's still worth using polling, for
> example for just one or two bytes like a register address? Taking an
> interrupt isn't free...
I did not considers lower limit.
According to your review, interrupt seems to be called too often.
However, It can't prevent the CPU utilization going to 100% during spi
transmission.
We will give more consideration and deliver a better solution to the
next patch version.
Thanks
Jaewon Kim
On Tue, Apr 04, 2023 at 10:15:05PM +0900, Jaewon Kim wrote:
> On 23. 4. 4. 21:58, Mark Brown wrote:
> > Is there some lower limit where it's still worth using polling, for
> > example for just one or two bytes like a register address? Taking an
> > interrupt isn't free...
> I did not considers lower limit.
> According to your review, interrupt seems to be called too often.
> However, It can't prevent the CPU utilization going to 100% during spi
> transmission.
It's not so much that the interrupt could be called too often as that
the time taken to take the interrupt (including all the overhead the CPU
has) might be large compared to what busy waiting would take if the
transfer is very small. If the FIFO is deep enough and the transfer is
long enough to use that then you start to see a win from interrupts.
> We will give more consideration and deliver a better solution to the
> next patch version.
Great.
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