This series makes the EOQ and TCFQ operating modes of spi-fsl-dspi work
when no platform IRQ is defined in the DT.
The NXP LS1021A-TSN board needs this setting for performance reasons,
and I am adding the corresponding DTS patch to this series to avoid
runtime errors that may occur if the DTS and the driver patches part
ways and get included through separate trees.
The series also contains a bug fix for the shared IRQ of the DSPI
driver. I am going to respin a version of it as a separate patch for
inclusion in stable trees, independent of this patchset.
Vladimir Oltean (5):
spi: spi-fsl-dspi: Reduce indentation level in dspi_interrupt
spi: spi-fsl-dspi: Exit the ISR with IRQ_NONE when it's not ours
spi: spi-fsl-dspi: Remove impossible to reach error check
spi: spi-fsl-dspi: Use poll mode in case the platform IRQ is missing
ARM: dts: ls1021a-tsn: Use the DSPI controller in poll mode
arch/arm/boot/dts/ls1021a-tsn.dts | 1 +
drivers/spi/spi-fsl-dspi.c | 128 ++++++++++++++++++------------
2 files changed, 78 insertions(+), 51 deletions(-)
--
2.17.1
The DSPI interrupt can be shared between two controllers at least on the
LX2160A. In that case, the driver for one controller might misbehave and
consume the other's interrupt. Fix this by actually checking if any of
the bits in the status register have been asserted.
Fixes: 13aed2392741 ("spi: spi-fsl-dspi: use IRQF_SHARED mode to request IRQ")
Signed-off-by: Vladimir Oltean <[email protected]>
---
drivers/spi/spi-fsl-dspi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index c90db7db4121..6ef2279a3699 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -659,7 +659,7 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
regmap_write(dspi->regmap, SPI_SR, spi_sr);
if (!(spi_sr & (SPI_SR_EOQF | SPI_SR_TCFQF)))
- return IRQ_HANDLED;
+ return IRQ_NONE;
/* Get transfer counter (in number of SPI transfers). It was
* reset to 0 when transfer(s) were started.
--
2.17.1
If the entire function depends on the SPI status register having the
interrupt bits asserted, then just check it and exit early if those bits
aren't set (such as in the case of the shared IRQ being triggered for
the other peripheral). Cosmetic patch.
Signed-off-by: Vladimir Oltean <[email protected]>
---
drivers/spi/spi-fsl-dspi.c | 79 +++++++++++++++++++-------------------
1 file changed, 40 insertions(+), 39 deletions(-)
diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 790cb02fc181..c90db7db4121 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -658,47 +658,48 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
regmap_read(dspi->regmap, SPI_SR, &spi_sr);
regmap_write(dspi->regmap, SPI_SR, spi_sr);
+ if (!(spi_sr & (SPI_SR_EOQF | SPI_SR_TCFQF)))
+ return IRQ_HANDLED;
+
+ /* Get transfer counter (in number of SPI transfers). It was
+ * reset to 0 when transfer(s) were started.
+ */
+ regmap_read(dspi->regmap, SPI_TCR, &spi_tcr);
+ spi_tcnt = SPI_TCR_GET_TCNT(spi_tcr);
+ /* Update total number of bytes that were transferred */
+ msg->actual_length += spi_tcnt * dspi->bytes_per_word;
+
+ trans_mode = dspi->devtype_data->trans_mode;
+ switch (trans_mode) {
+ case DSPI_EOQ_MODE:
+ dspi_eoq_read(dspi);
+ break;
+ case DSPI_TCFQ_MODE:
+ dspi_tcfq_read(dspi);
+ break;
+ default:
+ dev_err(&dspi->pdev->dev, "unsupported trans_mode %u\n",
+ trans_mode);
+ return IRQ_HANDLED;
+ }
- if (spi_sr & (SPI_SR_EOQF | SPI_SR_TCFQF)) {
- /* Get transfer counter (in number of SPI transfers). It was
- * reset to 0 when transfer(s) were started.
- */
- regmap_read(dspi->regmap, SPI_TCR, &spi_tcr);
- spi_tcnt = SPI_TCR_GET_TCNT(spi_tcr);
- /* Update total number of bytes that were transferred */
- msg->actual_length += spi_tcnt * dspi->bytes_per_word;
-
- trans_mode = dspi->devtype_data->trans_mode;
- switch (trans_mode) {
- case DSPI_EOQ_MODE:
- dspi_eoq_read(dspi);
- break;
- case DSPI_TCFQ_MODE:
- dspi_tcfq_read(dspi);
- break;
- default:
- dev_err(&dspi->pdev->dev, "unsupported trans_mode %u\n",
- trans_mode);
- return IRQ_HANDLED;
- }
+ if (!dspi->len) {
+ dspi->waitflags = 1;
+ wake_up_interruptible(&dspi->waitq);
+ return IRQ_HANDLED;
+ }
- if (!dspi->len) {
- dspi->waitflags = 1;
- wake_up_interruptible(&dspi->waitq);
- } else {
- switch (trans_mode) {
- case DSPI_EOQ_MODE:
- dspi_eoq_write(dspi);
- break;
- case DSPI_TCFQ_MODE:
- dspi_tcfq_write(dspi);
- break;
- default:
- dev_err(&dspi->pdev->dev,
- "unsupported trans_mode %u\n",
- trans_mode);
- }
- }
+ switch (trans_mode) {
+ case DSPI_EOQ_MODE:
+ dspi_eoq_write(dspi);
+ break;
+ case DSPI_TCFQ_MODE:
+ dspi_tcfq_write(dspi);
+ break;
+ default:
+ dev_err(&dspi->pdev->dev,
+ "unsupported trans_mode %u\n",
+ trans_mode);
}
return IRQ_HANDLED;
--
2.17.1
On platforms like LS1021A which use TCFQ mode, an interrupt needs to be
processed after each byte is TXed/RXed. I tried to make the DSPI
implementation on this SoC operate in other, more efficient modes (EOQ,
DMA) but it looks like it simply isn't possible.
Therefore allow the driver to operate in poll mode, to ease a bit of
this absurd amount of IRQ load generated in TCFQ mode. Doing so reduces
both the net time it takes to transmit a SPI message, as well as the
inter-frame jitter that occurs while doing so.
Signed-off-by: Vladimir Oltean <[email protected]>
---
drivers/spi/spi-fsl-dspi.c | 87 ++++++++++++++++++++++++++++----------
1 file changed, 64 insertions(+), 23 deletions(-)
diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 6d2c7984ab0e..77db43f1290f 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -647,19 +647,12 @@ static void dspi_eoq_read(struct fsl_dspi *dspi)
dspi_push_rx(dspi, fifo_read(dspi));
}
-static irqreturn_t dspi_interrupt(int irq, void *dev_id)
+static int dspi_rxtx(struct fsl_dspi *dspi)
{
- struct fsl_dspi *dspi = (struct fsl_dspi *)dev_id;
struct spi_message *msg = dspi->cur_msg;
enum dspi_trans_mode trans_mode;
- u32 spi_sr, spi_tcr;
u16 spi_tcnt;
-
- regmap_read(dspi->regmap, SPI_SR, &spi_sr);
- regmap_write(dspi->regmap, SPI_SR, spi_sr);
-
- if (!(spi_sr & (SPI_SR_EOQF | SPI_SR_TCFQF)))
- return IRQ_NONE;
+ u32 spi_tcr;
/* Get transfer counter (in number of SPI transfers). It was
* reset to 0 when transfer(s) were started.
@@ -675,17 +668,55 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
else if (trans_mode == DSPI_TCFQ_MODE)
dspi_tcfq_read(dspi);
- if (!dspi->len) {
- dspi->waitflags = 1;
- wake_up_interruptible(&dspi->waitq);
- return IRQ_HANDLED;
- }
+ if (!dspi->len)
+ /* Success! */
+ return 0;
if (trans_mode == DSPI_EOQ_MODE)
dspi_eoq_write(dspi);
else if (trans_mode == DSPI_TCFQ_MODE)
dspi_tcfq_write(dspi);
+ return -EINPROGRESS;
+}
+
+static int dspi_poll(struct fsl_dspi *dspi)
+{
+ int tries = 1000;
+ u32 spi_sr;
+
+ do {
+ regmap_read(dspi->regmap, SPI_SR, &spi_sr);
+ regmap_write(dspi->regmap, SPI_SR, spi_sr);
+
+ if (spi_sr & (SPI_SR_EOQF | SPI_SR_TCFQF))
+ break;
+ } while (--tries);
+
+ if (!tries)
+ return -ETIMEDOUT;
+
+ return dspi_rxtx(dspi);
+}
+
+static irqreturn_t dspi_interrupt(int irq, void *dev_id)
+{
+ struct fsl_dspi *dspi = (struct fsl_dspi *)dev_id;
+ u32 spi_sr;
+
+ regmap_read(dspi->regmap, SPI_SR, &spi_sr);
+ regmap_write(dspi->regmap, SPI_SR, spi_sr);
+
+ if (!(spi_sr & (SPI_SR_EOQF | SPI_SR_TCFQF)))
+ return IRQ_NONE;
+
+ dspi_rxtx(dspi);
+
+ if (!dspi->len) {
+ dspi->waitflags = 1;
+ wake_up_interruptible(&dspi->waitq);
+ }
+
return IRQ_HANDLED;
}
@@ -773,13 +804,18 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
goto out;
}
- if (trans_mode != DSPI_DMA_MODE) {
- if (wait_event_interruptible(dspi->waitq,
- dspi->waitflags))
- dev_err(&dspi->pdev->dev,
- "wait transfer complete fail!\n");
+ if (!dspi->irq) {
+ do {
+ status = dspi_poll(dspi);
+ } while (status == -EINPROGRESS);
+ } else if (trans_mode != DSPI_DMA_MODE) {
+ status = wait_event_interruptible(dspi->waitq,
+ dspi->waitflags);
dspi->waitflags = 0;
}
+ if (status)
+ dev_err(&dspi->pdev->dev,
+ "Waiting for transfer to complete failed!\n");
if (transfer->delay_usecs)
udelay(transfer->delay_usecs);
@@ -1079,10 +1115,13 @@ static int dspi_probe(struct platform_device *pdev)
goto out_ctlr_put;
dspi_init(dspi);
+
dspi->irq = platform_get_irq(pdev, 0);
- if (dspi->irq < 0) {
- ret = dspi->irq;
- goto out_clk_put;
+ if (dspi->irq <= 0) {
+ dev_info(&pdev->dev,
+ "can't get platform irq, using poll mode\n");
+ dspi->irq = 0;
+ goto poll_mode;
}
ret = devm_request_irq(&pdev->dev, dspi->irq, dspi_interrupt,
@@ -1092,6 +1131,9 @@ static int dspi_probe(struct platform_device *pdev)
goto out_clk_put;
}
+ init_waitqueue_head(&dspi->waitq);
+
+poll_mode:
if (dspi->devtype_data->trans_mode == DSPI_DMA_MODE) {
ret = dspi_request_dma(dspi, res->start);
if (ret < 0) {
@@ -1103,7 +1145,6 @@ static int dspi_probe(struct platform_device *pdev)
ctlr->max_speed_hz =
clk_get_rate(dspi->clk) / dspi->devtype_data->max_clock_factor;
- init_waitqueue_head(&dspi->waitq);
platform_set_drvdata(pdev, ctlr);
ret = spi_register_controller(ctlr);
--
2.17.1
dspi->devtype_data is under the total control of the driver. Therefore,
a bad value is a driver bug and checking it at runtime (and during an
ISR, at that!) is pointless.
The second "else if" check is only for clarity (instead of a broader
"else") in case other transfer modes are added in the future. But the
printing is dead code and can be removed.
Signed-off-by: Vladimir Oltean <[email protected]>
---
drivers/spi/spi-fsl-dspi.c | 24 ++++--------------------
1 file changed, 4 insertions(+), 20 deletions(-)
diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 6ef2279a3699..6d2c7984ab0e 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -670,18 +670,10 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
msg->actual_length += spi_tcnt * dspi->bytes_per_word;
trans_mode = dspi->devtype_data->trans_mode;
- switch (trans_mode) {
- case DSPI_EOQ_MODE:
+ if (trans_mode == DSPI_EOQ_MODE)
dspi_eoq_read(dspi);
- break;
- case DSPI_TCFQ_MODE:
+ else if (trans_mode == DSPI_TCFQ_MODE)
dspi_tcfq_read(dspi);
- break;
- default:
- dev_err(&dspi->pdev->dev, "unsupported trans_mode %u\n",
- trans_mode);
- return IRQ_HANDLED;
- }
if (!dspi->len) {
dspi->waitflags = 1;
@@ -689,18 +681,10 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
return IRQ_HANDLED;
}
- switch (trans_mode) {
- case DSPI_EOQ_MODE:
+ if (trans_mode == DSPI_EOQ_MODE)
dspi_eoq_write(dspi);
- break;
- case DSPI_TCFQ_MODE:
+ else if (trans_mode == DSPI_TCFQ_MODE)
dspi_tcfq_write(dspi);
- break;
- default:
- dev_err(&dspi->pdev->dev,
- "unsupported trans_mode %u\n",
- trans_mode);
- }
return IRQ_HANDLED;
}
--
2.17.1
Connected to the LS1021A DSPI is the SJA1105 DSA switch. This
constitutes 4 of the 6 Ethernet ports on this board.
As the SJA1105 is a PTP switch, constant disciplining of its PTP clock
is necessary, and that translates into a lot of SPI I/O even when
otherwise idle.
Switching to using the DSPI in poll mode has several distinct
benefits:
- With interrupts, the DSPI driver in TCFQ mode raises an IRQ after each
transmitted byte. There is more time wasted for the "waitq" event than
for actual I/O. And the DSPI IRQ count is by far the largest in
/proc/interrupts on this board (larger than Ethernet). I should
mention that due to various LS1021A errata, other operating modes than
TCFQ are not available.
- The SPI I/O time is both lower, and more consistently so. For a TSN
switch it is important that all SPI transfers take a deterministic
time to complete.
Reading the PTP clock is an important example.
Egressing through the switch requires some setup in advance (an SPI
write command). Without this patch, that operation required a
--tx_timestamp_timeout 50 (ms), now it can be done with
--tx_timestamp_timeout 10.
Yet another example is reconstructing timestamps, which has a hard
deadline because the PTP timestamping counter wraps around in 0.135
seconds. Combined with other I/O needed for that to happen, there is
a real risk that the deadline is not always met.
See drivers/net/dsa/sja1105/ for more info about the above.
Cc: Rob Herring <[email protected]>
Cc: Shawn Guo <[email protected]>
Signed-off-by: Vladimir Oltean <[email protected]>
---
arch/arm/boot/dts/ls1021a-tsn.dts | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/boot/dts/ls1021a-tsn.dts b/arch/arm/boot/dts/ls1021a-tsn.dts
index 5b7689094b70..1c09cfc766af 100644
--- a/arch/arm/boot/dts/ls1021a-tsn.dts
+++ b/arch/arm/boot/dts/ls1021a-tsn.dts
@@ -33,6 +33,7 @@
};
&dspi0 {
+ /delete-property/ interrupts;
bus-num = <0>;
status = "okay";
--
2.17.1
On Fri, Aug 23, 2019 at 12:15:11AM +0300, Vladimir Oltean wrote:
> The DSPI interrupt can be shared between two controllers at least on the
> LX2160A. In that case, the driver for one controller might misbehave and
> consume the other's interrupt. Fix this by actually checking if any of
> the bits in the status register have been asserted.
It would be better to have done this as the first patch before
the restructuring, that way we could send this as a fix - the
refactoring while good doesn't really fit with stable.
Hi Mark,
On Fri, 23 Aug 2019 at 13:28, Mark Brown <[email protected]> wrote:
>
> On Fri, Aug 23, 2019 at 12:15:11AM +0300, Vladimir Oltean wrote:
> > The DSPI interrupt can be shared between two controllers at least on the
> > LX2160A. In that case, the driver for one controller might misbehave and
> > consume the other's interrupt. Fix this by actually checking if any of
> > the bits in the status register have been asserted.
>
> It would be better to have done this as the first patch before
> the restructuring, that way we could send this as a fix - the
> refactoring while good doesn't really fit with stable.
Did you see this?
https://lkml.org/lkml/2019/8/22/1542
Regards,
-Vladimir
On Fri, Aug 23, 2019 at 11:50:44AM +0100, Mark Brown wrote:
> On Fri, Aug 23, 2019 at 01:30:27PM +0300, Vladimir Oltean wrote:
> > Did you see this?
> > https://lkml.org/lkml/2019/8/22/1542
> I'm not online enough to readily follow that link right now, I
> did apply another patch for a similar issue though. If that's
> a different version of the same change please don't do that,
> sending multiple conflicting versions of the same thing creates
> conflicts and makes everything harder to work with.
Oh, I guess this was due to there being an existing refactoring
in -next that meant the fix wouldn't apply directly. I sorted
that out now I think, but in general the same thing applies -
it's better to put fixes before anything else in the series,
it'll flag up when reviewing.
On Fri, Aug 23, 2019 at 01:30:27PM +0300, Vladimir Oltean wrote:
> On Fri, 23 Aug 2019 at 13:28, Mark Brown <[email protected]> wrote:
> > It would be better to have done this as the first patch before
> > the restructuring, that way we could send this as a fix - the
> > refactoring while good doesn't really fit with stable.
> Did you see this?
> https://lkml.org/lkml/2019/8/22/1542
I'm not online enough to readily follow that link right now, I
did apply another patch for a similar issue though. If that's
a different version of the same change please don't do that,
sending multiple conflicting versions of the same thing creates
conflicts and makes everything harder to work with.
Please include human readable descriptions of things like commits and
issues being discussed in e-mail in your mails, this makes them much
easier for humans to read especially when they have no internet access.
I do frequently catch up on my mail on flights or while otherwise
travelling so this is even more pressing for me than just being about
making things a bit easier to read.
The patch
spi: spi-fsl-dspi: Exit the ISR with IRQ_NONE when it's not ours
has been applied to the spi tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-5.4
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
From 37b4100180641968056cb4e034cebc38338e8652 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <[email protected]>
Date: Fri, 23 Aug 2019 00:15:11 +0300
Subject: [PATCH] spi: spi-fsl-dspi: Exit the ISR with IRQ_NONE when it's not
ours
The DSPI interrupt can be shared between two controllers at least on the
LX2160A. In that case, the driver for one controller might misbehave and
consume the other's interrupt. Fix this by actually checking if any of
the bits in the status register have been asserted.
Fixes: 13aed2392741 ("spi: spi-fsl-dspi: use IRQF_SHARED mode to request IRQ")
Signed-off-by: Vladimir Oltean <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Mark Brown <[email protected]>
---
drivers/spi/spi-fsl-dspi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index c90db7db4121..6ef2279a3699 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -659,7 +659,7 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
regmap_write(dspi->regmap, SPI_SR, spi_sr);
if (!(spi_sr & (SPI_SR_EOQF | SPI_SR_TCFQF)))
- return IRQ_HANDLED;
+ return IRQ_NONE;
/* Get transfer counter (in number of SPI transfers). It was
* reset to 0 when transfer(s) were started.
--
2.20.1
The patch
spi: spi-fsl-dspi: Use poll mode in case the platform IRQ is missing
has been applied to the spi tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-5.4
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
From c55be305915974db160ce6472722ff74f45b8d4e Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <[email protected]>
Date: Fri, 23 Aug 2019 00:15:13 +0300
Subject: [PATCH] spi: spi-fsl-dspi: Use poll mode in case the platform IRQ is
missing
On platforms like LS1021A which use TCFQ mode, an interrupt needs to be
processed after each byte is TXed/RXed. I tried to make the DSPI
implementation on this SoC operate in other, more efficient modes (EOQ,
DMA) but it looks like it simply isn't possible.
Therefore allow the driver to operate in poll mode, to ease a bit of
this absurd amount of IRQ load generated in TCFQ mode. Doing so reduces
both the net time it takes to transmit a SPI message, as well as the
inter-frame jitter that occurs while doing so.
Signed-off-by: Vladimir Oltean <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Mark Brown <[email protected]>
---
drivers/spi/spi-fsl-dspi.c | 87 ++++++++++++++++++++++++++++----------
1 file changed, 64 insertions(+), 23 deletions(-)
diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 6d2c7984ab0e..77db43f1290f 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -647,19 +647,12 @@ static void dspi_eoq_read(struct fsl_dspi *dspi)
dspi_push_rx(dspi, fifo_read(dspi));
}
-static irqreturn_t dspi_interrupt(int irq, void *dev_id)
+static int dspi_rxtx(struct fsl_dspi *dspi)
{
- struct fsl_dspi *dspi = (struct fsl_dspi *)dev_id;
struct spi_message *msg = dspi->cur_msg;
enum dspi_trans_mode trans_mode;
- u32 spi_sr, spi_tcr;
u16 spi_tcnt;
-
- regmap_read(dspi->regmap, SPI_SR, &spi_sr);
- regmap_write(dspi->regmap, SPI_SR, spi_sr);
-
- if (!(spi_sr & (SPI_SR_EOQF | SPI_SR_TCFQF)))
- return IRQ_NONE;
+ u32 spi_tcr;
/* Get transfer counter (in number of SPI transfers). It was
* reset to 0 when transfer(s) were started.
@@ -675,17 +668,55 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
else if (trans_mode == DSPI_TCFQ_MODE)
dspi_tcfq_read(dspi);
- if (!dspi->len) {
- dspi->waitflags = 1;
- wake_up_interruptible(&dspi->waitq);
- return IRQ_HANDLED;
- }
+ if (!dspi->len)
+ /* Success! */
+ return 0;
if (trans_mode == DSPI_EOQ_MODE)
dspi_eoq_write(dspi);
else if (trans_mode == DSPI_TCFQ_MODE)
dspi_tcfq_write(dspi);
+ return -EINPROGRESS;
+}
+
+static int dspi_poll(struct fsl_dspi *dspi)
+{
+ int tries = 1000;
+ u32 spi_sr;
+
+ do {
+ regmap_read(dspi->regmap, SPI_SR, &spi_sr);
+ regmap_write(dspi->regmap, SPI_SR, spi_sr);
+
+ if (spi_sr & (SPI_SR_EOQF | SPI_SR_TCFQF))
+ break;
+ } while (--tries);
+
+ if (!tries)
+ return -ETIMEDOUT;
+
+ return dspi_rxtx(dspi);
+}
+
+static irqreturn_t dspi_interrupt(int irq, void *dev_id)
+{
+ struct fsl_dspi *dspi = (struct fsl_dspi *)dev_id;
+ u32 spi_sr;
+
+ regmap_read(dspi->regmap, SPI_SR, &spi_sr);
+ regmap_write(dspi->regmap, SPI_SR, spi_sr);
+
+ if (!(spi_sr & (SPI_SR_EOQF | SPI_SR_TCFQF)))
+ return IRQ_NONE;
+
+ dspi_rxtx(dspi);
+
+ if (!dspi->len) {
+ dspi->waitflags = 1;
+ wake_up_interruptible(&dspi->waitq);
+ }
+
return IRQ_HANDLED;
}
@@ -773,13 +804,18 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
goto out;
}
- if (trans_mode != DSPI_DMA_MODE) {
- if (wait_event_interruptible(dspi->waitq,
- dspi->waitflags))
- dev_err(&dspi->pdev->dev,
- "wait transfer complete fail!\n");
+ if (!dspi->irq) {
+ do {
+ status = dspi_poll(dspi);
+ } while (status == -EINPROGRESS);
+ } else if (trans_mode != DSPI_DMA_MODE) {
+ status = wait_event_interruptible(dspi->waitq,
+ dspi->waitflags);
dspi->waitflags = 0;
}
+ if (status)
+ dev_err(&dspi->pdev->dev,
+ "Waiting for transfer to complete failed!\n");
if (transfer->delay_usecs)
udelay(transfer->delay_usecs);
@@ -1079,10 +1115,13 @@ static int dspi_probe(struct platform_device *pdev)
goto out_ctlr_put;
dspi_init(dspi);
+
dspi->irq = platform_get_irq(pdev, 0);
- if (dspi->irq < 0) {
- ret = dspi->irq;
- goto out_clk_put;
+ if (dspi->irq <= 0) {
+ dev_info(&pdev->dev,
+ "can't get platform irq, using poll mode\n");
+ dspi->irq = 0;
+ goto poll_mode;
}
ret = devm_request_irq(&pdev->dev, dspi->irq, dspi_interrupt,
@@ -1092,6 +1131,9 @@ static int dspi_probe(struct platform_device *pdev)
goto out_clk_put;
}
+ init_waitqueue_head(&dspi->waitq);
+
+poll_mode:
if (dspi->devtype_data->trans_mode == DSPI_DMA_MODE) {
ret = dspi_request_dma(dspi, res->start);
if (ret < 0) {
@@ -1103,7 +1145,6 @@ static int dspi_probe(struct platform_device *pdev)
ctlr->max_speed_hz =
clk_get_rate(dspi->clk) / dspi->devtype_data->max_clock_factor;
- init_waitqueue_head(&dspi->waitq);
platform_set_drvdata(pdev, ctlr);
ret = spi_register_controller(ctlr);
--
2.20.1
The patch
spi: spi-fsl-dspi: Remove impossible to reach error check
has been applied to the spi tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-5.4
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
From 1eaeba70738e723be1e5787bdfd9a30f7471d730 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <[email protected]>
Date: Fri, 23 Aug 2019 00:15:12 +0300
Subject: [PATCH] spi: spi-fsl-dspi: Remove impossible to reach error check
dspi->devtype_data is under the total control of the driver. Therefore,
a bad value is a driver bug and checking it at runtime (and during an
ISR, at that!) is pointless.
The second "else if" check is only for clarity (instead of a broader
"else") in case other transfer modes are added in the future. But the
printing is dead code and can be removed.
Signed-off-by: Vladimir Oltean <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Mark Brown <[email protected]>
---
drivers/spi/spi-fsl-dspi.c | 24 ++++--------------------
1 file changed, 4 insertions(+), 20 deletions(-)
diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 6ef2279a3699..6d2c7984ab0e 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -670,18 +670,10 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
msg->actual_length += spi_tcnt * dspi->bytes_per_word;
trans_mode = dspi->devtype_data->trans_mode;
- switch (trans_mode) {
- case DSPI_EOQ_MODE:
+ if (trans_mode == DSPI_EOQ_MODE)
dspi_eoq_read(dspi);
- break;
- case DSPI_TCFQ_MODE:
+ else if (trans_mode == DSPI_TCFQ_MODE)
dspi_tcfq_read(dspi);
- break;
- default:
- dev_err(&dspi->pdev->dev, "unsupported trans_mode %u\n",
- trans_mode);
- return IRQ_HANDLED;
- }
if (!dspi->len) {
dspi->waitflags = 1;
@@ -689,18 +681,10 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
return IRQ_HANDLED;
}
- switch (trans_mode) {
- case DSPI_EOQ_MODE:
+ if (trans_mode == DSPI_EOQ_MODE)
dspi_eoq_write(dspi);
- break;
- case DSPI_TCFQ_MODE:
+ else if (trans_mode == DSPI_TCFQ_MODE)
dspi_tcfq_write(dspi);
- break;
- default:
- dev_err(&dspi->pdev->dev,
- "unsupported trans_mode %u\n",
- trans_mode);
- }
return IRQ_HANDLED;
}
--
2.20.1
The patch
spi: spi-fsl-dspi: Reduce indentation level in dspi_interrupt
has been applied to the spi tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-5.4
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
From 12fb61a973935c63f2580b3b053017cc14b51f42 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <[email protected]>
Date: Fri, 23 Aug 2019 00:15:10 +0300
Subject: [PATCH] spi: spi-fsl-dspi: Reduce indentation level in dspi_interrupt
If the entire function depends on the SPI status register having the
interrupt bits asserted, then just check it and exit early if those bits
aren't set (such as in the case of the shared IRQ being triggered for
the other peripheral). Cosmetic patch.
Signed-off-by: Vladimir Oltean <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Mark Brown <[email protected]>
---
drivers/spi/spi-fsl-dspi.c | 79 +++++++++++++++++++-------------------
1 file changed, 40 insertions(+), 39 deletions(-)
diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 790cb02fc181..c90db7db4121 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -658,47 +658,48 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
regmap_read(dspi->regmap, SPI_SR, &spi_sr);
regmap_write(dspi->regmap, SPI_SR, spi_sr);
+ if (!(spi_sr & (SPI_SR_EOQF | SPI_SR_TCFQF)))
+ return IRQ_HANDLED;
+
+ /* Get transfer counter (in number of SPI transfers). It was
+ * reset to 0 when transfer(s) were started.
+ */
+ regmap_read(dspi->regmap, SPI_TCR, &spi_tcr);
+ spi_tcnt = SPI_TCR_GET_TCNT(spi_tcr);
+ /* Update total number of bytes that were transferred */
+ msg->actual_length += spi_tcnt * dspi->bytes_per_word;
+
+ trans_mode = dspi->devtype_data->trans_mode;
+ switch (trans_mode) {
+ case DSPI_EOQ_MODE:
+ dspi_eoq_read(dspi);
+ break;
+ case DSPI_TCFQ_MODE:
+ dspi_tcfq_read(dspi);
+ break;
+ default:
+ dev_err(&dspi->pdev->dev, "unsupported trans_mode %u\n",
+ trans_mode);
+ return IRQ_HANDLED;
+ }
- if (spi_sr & (SPI_SR_EOQF | SPI_SR_TCFQF)) {
- /* Get transfer counter (in number of SPI transfers). It was
- * reset to 0 when transfer(s) were started.
- */
- regmap_read(dspi->regmap, SPI_TCR, &spi_tcr);
- spi_tcnt = SPI_TCR_GET_TCNT(spi_tcr);
- /* Update total number of bytes that were transferred */
- msg->actual_length += spi_tcnt * dspi->bytes_per_word;
-
- trans_mode = dspi->devtype_data->trans_mode;
- switch (trans_mode) {
- case DSPI_EOQ_MODE:
- dspi_eoq_read(dspi);
- break;
- case DSPI_TCFQ_MODE:
- dspi_tcfq_read(dspi);
- break;
- default:
- dev_err(&dspi->pdev->dev, "unsupported trans_mode %u\n",
- trans_mode);
- return IRQ_HANDLED;
- }
+ if (!dspi->len) {
+ dspi->waitflags = 1;
+ wake_up_interruptible(&dspi->waitq);
+ return IRQ_HANDLED;
+ }
- if (!dspi->len) {
- dspi->waitflags = 1;
- wake_up_interruptible(&dspi->waitq);
- } else {
- switch (trans_mode) {
- case DSPI_EOQ_MODE:
- dspi_eoq_write(dspi);
- break;
- case DSPI_TCFQ_MODE:
- dspi_tcfq_write(dspi);
- break;
- default:
- dev_err(&dspi->pdev->dev,
- "unsupported trans_mode %u\n",
- trans_mode);
- }
- }
+ switch (trans_mode) {
+ case DSPI_EOQ_MODE:
+ dspi_eoq_write(dspi);
+ break;
+ case DSPI_TCFQ_MODE:
+ dspi_tcfq_write(dspi);
+ break;
+ default:
+ dev_err(&dspi->pdev->dev,
+ "unsupported trans_mode %u\n",
+ trans_mode);
}
return IRQ_HANDLED;
--
2.20.1
Hi Mark,
On Fri, 23 Aug 2019 at 13:59, Mark Brown <[email protected]> wrote:
>
> On Fri, Aug 23, 2019 at 11:50:44AM +0100, Mark Brown wrote:
> > On Fri, Aug 23, 2019 at 01:30:27PM +0300, Vladimir Oltean wrote:
>
> > > Did you see this?
> > > https://lkml.org/lkml/2019/8/22/1542
>
> > I'm not online enough to readily follow that link right now, I
> > did apply another patch for a similar issue though. If that's
> > a different version of the same change please don't do that,
> > sending multiple conflicting versions of the same thing creates
> > conflicts and makes everything harder to work with.
>
> Oh, I guess this was due to there being an existing refactoring
> in -next that meant the fix wouldn't apply directly. I sorted
> that out now I think, but in general the same thing applies -
> it's better to put fixes before anything else in the series,
> it'll flag up when reviewing.
I try to require as little attention span as possible from you and I
apologize that I'm sending patches like a noob, but I'm not used to
this sort of interaction with a maintainer. It's taking me some time
to adjust expectations.
- You left change requests in the initial patchset I submitted, but
you partially applied the series anyway. You didn't give me a chance
to respin the whole series and put the shared IRQ fix on top, so it
applies on old trees as well. No problem, I sent two versions of the
patch.
- On my previous series you left this comment:
> Please don't include all the extra tags you've got in your subject
> lines. In my inbox this series looks like:
>
> 790 N T 08/18 Vladimir Oltean ( 16K) ├─>[PATCH spi for-5.4 01/14] spi: spi-f
>
> so I can't tell what it's actually about just from looking at it. Just
> [PATCH 01/14] would be enough, putting a target version in or versioning
> the patch series can be OK but you usually don't use a target version
> for -next and adding spi in there is redundant given that it's also in
> the changelog.
So I didn't put any target version in the patch titles this time,
although arguably it would have been clearer to you that there's a
patch for-5.4 and another version of it for-4.20 (which i *think* is
how I should submit a fix, I don't see any branch for inclusion in
stable trees per se).
No problem, I explained in the cover letters that one patchset is for
-next and the other is for inclusion in stable trees. Maintainers do
read cover letters, right?
Message from the -next cover letter:
> The series also contains a bug fix for the shared IRQ of the DSPI
> driver. I am going to respin a version of it as a separate patch for
> inclusion in stable trees, independent of this patchset.
Message from the fix's cover letter:
> This patch is taken out of the "Poll mode for NXP DSPI driver" series
> and respun against the "for-4.20" branch.
> $(git describe --tags 13aed2392741) shows:
> v4.20-rc1-18-g13aed2392741
Yes, I did send a cover letter for a single patch. I thought it's
harder to miss than a note hidden under patch 2/5 of one series, and
in the note section of the other's. I think you could have also made
an argument about me not doing it the other way around. In the end,
you can not read a note just as well as not read a cover letter, and
there's little I can do.
No problem, you missed the link between the two. I sent you a link to
the lkml archive. You said "I'm not online enough to readily follow
that link right now". Please teach me - I really don't know - how can
I make links between patchsets easier for you to follow, if you don't
read cover letters and you can't access lkml? I promise I'll use that
method next time.
> I do frequently catch up on my mail on flights or while otherwise
> travelling so this is even more pressing for me than just being about
> making things a bit easier to read.
Maybe you simply should do something else while traveling, just saying.
Regards,
-Vladimir
On Fri, Aug 23, 2019 at 03:06:52PM +0300, Vladimir Oltean wrote:
> - You left change requests in the initial patchset I submitted, but
> you partially applied the series anyway. You didn't give me a chance
> to respin the whole series and put the shared IRQ fix on top, so it
> applies on old trees as well. No problem, I sent two versions of the
> patch.
Right, and this is fine. A big part of this is that it's just
generally bad practice to not have fixes at the front of the
series, I'd flag this up as a problem even if the code was all
new and there was no question of applying as a bug fix. It's
something that's noticable just at the level of looking at the
shape of the series without even looking at the contents of the
patches, if the fix is actually a good one or anything like that.
In the context of this it made it look like the reason you'd had
to do two versions.
> So I didn't put any target version in the patch titles this time,
> although arguably it would have been clearer to you that there's a
> patch for-5.4 and another version of it for-4.20 (which i *think* is
> how I should submit a fix, I don't see any branch for inclusion in
> stable trees per se).
Not for 4.20, for v5.3 - we basically only fix Linus' tree
directly, anything else gets backported from there unless it's
super important. I don't think anyone is updating v4.20 at all
these days, the version number change from v4 to v5 was totally
arbatrary.
> Yes, I did send a cover letter for a single patch. I thought it's
> harder to miss than a note hidden under patch 2/5 of one series, and
> in the note section of the other's. I think you could have also made
If you're sending a multi-patch series it's of course good to
send a cover letter, it's just single patches where it's adding
overhead.
> No problem, you missed the link between the two. I sent you a link to
> the lkml archive. You said "I'm not online enough to readily follow
> that link right now". Please teach me - I really don't know - how can
It's not that I missed the link between them, it's that what I'd
expected to see was the fix being the first patch in the series
for -next and for that fix to look substantially the same with at
most some context difference. I wasn't expecting to see a
completely different patch that wasn't at the start of the
series, had the fix been at the start of the series it'd have
been fairly clear what was going on but the refactoring patch
looked like the main reason you'd needed different versions (it's
certainly why they don't visually resemble each other).
In other words it looked like you'd sent a different fix because
the fix you'd done for -next was based on the first patch in the
series rather than there also being some context changes.
> I make links between patchsets easier for you to follow, if you don't
> read cover letters and you can't access lkml? I promise I'll use that
> method next time.
Like I said include a plain text description of what you're
linking to (eg, the subject line from a mail).
> > I do frequently catch up on my mail on flights or while otherwise
> > travelling so this is even more pressing for me than just being about
> > making things a bit easier to read.
> Maybe you simply should do something else while traveling, just saying.
I could also add in the coffee shop I sometimes work from which
doesn't have WiFi or mobile coverage. Besides, like that part of
the text does say it's also a usability thing, having to fire up
a web browser to figure out what's being described is a stumbling
block.
Hi Mark,
On Fri, 23 Aug 2019 at 00:15, Vladimir Oltean <[email protected]> wrote:
>
> Connected to the LS1021A DSPI is the SJA1105 DSA switch. This
> constitutes 4 of the 6 Ethernet ports on this board.
>
> As the SJA1105 is a PTP switch, constant disciplining of its PTP clock
> is necessary, and that translates into a lot of SPI I/O even when
> otherwise idle.
>
> Switching to using the DSPI in poll mode has several distinct
> benefits:
>
> - With interrupts, the DSPI driver in TCFQ mode raises an IRQ after each
> transmitted byte. There is more time wasted for the "waitq" event than
> for actual I/O. And the DSPI IRQ count is by far the largest in
> /proc/interrupts on this board (larger than Ethernet). I should
> mention that due to various LS1021A errata, other operating modes than
> TCFQ are not available.
>
> - The SPI I/O time is both lower, and more consistently so. For a TSN
> switch it is important that all SPI transfers take a deterministic
> time to complete.
> Reading the PTP clock is an important example.
> Egressing through the switch requires some setup in advance (an SPI
> write command). Without this patch, that operation required a
> --tx_timestamp_timeout 50 (ms), now it can be done with
> --tx_timestamp_timeout 10.
> Yet another example is reconstructing timestamps, which has a hard
> deadline because the PTP timestamping counter wraps around in 0.135
> seconds. Combined with other I/O needed for that to happen, there is
> a real risk that the deadline is not always met.
>
> See drivers/net/dsa/sja1105/ for more info about the above.
>
> Cc: Rob Herring <[email protected]>
> Cc: Shawn Guo <[email protected]>
> Signed-off-by: Vladimir Oltean <[email protected]>
> ---
> arch/arm/boot/dts/ls1021a-tsn.dts | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/boot/dts/ls1021a-tsn.dts b/arch/arm/boot/dts/ls1021a-tsn.dts
> index 5b7689094b70..1c09cfc766af 100644
> --- a/arch/arm/boot/dts/ls1021a-tsn.dts
> +++ b/arch/arm/boot/dts/ls1021a-tsn.dts
> @@ -33,6 +33,7 @@
> };
>
> &dspi0 {
> + /delete-property/ interrupts;
> bus-num = <0>;
> status = "okay";
>
> --
> 2.17.1
>
I noticed you skipped applying this patch, and I'm not sure that Shawn
will review it/take it.
Do you have a better suggestion how I can achieve putting the DSPI
driver in poll mode for this board? A Kconfig option maybe?
Regards,
-Vladimir
On Mon, Aug 26, 2019 at 04:10:51PM +0300, Vladimir Oltean wrote:
> I noticed you skipped applying this patch, and I'm not sure that Shawn
> will review it/take it.
> Do you have a better suggestion how I can achieve putting the DSPI
> driver in poll mode for this board? A Kconfig option maybe?
DT changes go through the relevant platform trees, not the
subsystem trees, so it's not something I'd expect to apply.
On Tue, 27 Aug 2019 at 21:05, Mark Brown <[email protected]> wrote:
>
> On Mon, Aug 26, 2019 at 04:10:51PM +0300, Vladimir Oltean wrote:
>
> > I noticed you skipped applying this patch, and I'm not sure that Shawn
> > will review it/take it.
> > Do you have a better suggestion how I can achieve putting the DSPI
> > driver in poll mode for this board? A Kconfig option maybe?
>
> DT changes go through the relevant platform trees, not the
> subsystem trees, so it's not something I'd expect to apply.
But at least is it something that you expect to see done through a
device tree change?
On Tue, Aug 27, 2019 at 09:06:14PM +0300, Vladimir Oltean wrote:
> On Tue, 27 Aug 2019 at 21:05, Mark Brown <[email protected]> wrote:
> > On Mon, Aug 26, 2019 at 04:10:51PM +0300, Vladimir Oltean wrote:
> > > I noticed you skipped applying this patch, and I'm not sure that Shawn
> > > will review it/take it.
> > > Do you have a better suggestion how I can achieve putting the DSPI
> > > driver in poll mode for this board? A Kconfig option maybe?
> > DT changes go through the relevant platform trees, not the
> > subsystem trees, so it's not something I'd expect to apply.
> But at least is it something that you expect to see done through a
> device tree change?
Well, it's not ideal - if it performs better all the time the
driver should probably just do it unconditionally. If there's
some threashold where it tends to perform better then the driver
should check for that but IIRC it sounds like the interrupt just
isn't at all helpful here.
On Tue, 27 Aug 2019 at 21:13, Mark Brown <[email protected]> wrote:
>
> On Tue, Aug 27, 2019 at 09:06:14PM +0300, Vladimir Oltean wrote:
> > On Tue, 27 Aug 2019 at 21:05, Mark Brown <[email protected]> wrote:
> > > On Mon, Aug 26, 2019 at 04:10:51PM +0300, Vladimir Oltean wrote:
>
> > > > I noticed you skipped applying this patch, and I'm not sure that Shawn
> > > > will review it/take it.
> > > > Do you have a better suggestion how I can achieve putting the DSPI
> > > > driver in poll mode for this board? A Kconfig option maybe?
>
> > > DT changes go through the relevant platform trees, not the
> > > subsystem trees, so it's not something I'd expect to apply.
>
> > But at least is it something that you expect to see done through a
> > device tree change?
>
> Well, it's not ideal - if it performs better all the time the
> driver should probably just do it unconditionally. If there's
> some threashold where it tends to perform better then the driver
> should check for that but IIRC it sounds like the interrupt just
> isn't at all helpful here.
I can't seem to find any situation where it performs worse. Hence my
question on whether it's a better idea to condition this behavior on a
Kconfig option rather than a DT blob which may or may not be in sync.
On Tue, Aug 27, 2019 at 09:16:39PM +0300, Vladimir Oltean wrote:
> I can't seem to find any situation where it performs worse. Hence my
> question on whether it's a better idea to condition this behavior on a
> Kconfig option rather than a DT blob which may or may not be in sync.
If it's unconditionally worse then it shouldn't even be a Kconfig
option, just make the driver just never use the interrupt.
On Tue, Aug 27, 2019 at 09:16:39PM +0300, Vladimir Oltean wrote:
> On Tue, 27 Aug 2019 at 21:13, Mark Brown <[email protected]> wrote:
> >
> > On Tue, Aug 27, 2019 at 09:06:14PM +0300, Vladimir Oltean wrote:
> > > On Tue, 27 Aug 2019 at 21:05, Mark Brown <[email protected]> wrote:
> > > > On Mon, Aug 26, 2019 at 04:10:51PM +0300, Vladimir Oltean wrote:
> >
> > > > > I noticed you skipped applying this patch, and I'm not sure that Shawn
> > > > > will review it/take it.
> > > > > Do you have a better suggestion how I can achieve putting the DSPI
> > > > > driver in poll mode for this board? A Kconfig option maybe?
> >
> > > > DT changes go through the relevant platform trees, not the
> > > > subsystem trees, so it's not something I'd expect to apply.
> >
> > > But at least is it something that you expect to see done through a
> > > device tree change?
> >
> > Well, it's not ideal - if it performs better all the time the
> > driver should probably just do it unconditionally. If there's
> > some threashold where it tends to perform better then the driver
> > should check for that but IIRC it sounds like the interrupt just
> > isn't at all helpful here.
>
> I can't seem to find any situation where it performs worse. Hence my
> question on whether it's a better idea to condition this behavior on a
> Kconfig option rather than a DT blob which may or may not be in sync.
DT is a description of hardware not condition for software behavior,
where module parameter is usually used for.
Shawn
On Wed, Sep 11, 2019 at 8:34 AM Shawn Guo <[email protected]> wrote:
> On Tue, Aug 27, 2019 at 09:16:39PM +0300, Vladimir Oltean wrote:
> > On Tue, 27 Aug 2019 at 21:13, Mark Brown <[email protected]> wrote:
> > > On Tue, Aug 27, 2019 at 09:06:14PM +0300, Vladimir Oltean wrote:
> > > > On Tue, 27 Aug 2019 at 21:05, Mark Brown <[email protected]> wrote:
> > > > > On Mon, Aug 26, 2019 at 04:10:51PM +0300, Vladimir Oltean wrote:
> > >
> > > > > > I noticed you skipped applying this patch, and I'm not sure that Shawn
> > > > > > will review it/take it.
> > > > > > Do you have a better suggestion how I can achieve putting the DSPI
> > > > > > driver in poll mode for this board? A Kconfig option maybe?
> > >
> > > > > DT changes go through the relevant platform trees, not the
> > > > > subsystem trees, so it's not something I'd expect to apply.
> > >
> > > > But at least is it something that you expect to see done through a
> > > > device tree change?
> > >
> > > Well, it's not ideal - if it performs better all the time the
> > > driver should probably just do it unconditionally. If there's
> > > some threashold where it tends to perform better then the driver
> > > should check for that but IIRC it sounds like the interrupt just
> > > isn't at all helpful here.
> >
> > I can't seem to find any situation where it performs worse. Hence my
> > question on whether it's a better idea to condition this behavior on a
> > Kconfig option rather than a DT blob which may or may not be in sync.
>
> DT is a description of hardware not condition for software behavior,
> where module parameter is usually used for.
+1
DT says the interrupt line is wired.
The driver should know if it should make use of the interrupt, or not.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds