diff mbox series

spi: fsl-dspi: avoid SCK glitches with continuous transfers

Message ID 20230529223402.1199503-1-vladimir.oltean@nxp.com
State New
Headers
Series spi: fsl-dspi: avoid SCK glitches with continuous transfers |

Commit Message

Vladimir Oltean May 29, 2023, 10:34 p.m. UTC 
  The DSPI controller has configurable timing for

(a) tCSC: the interval between the assertion of the chip select and the
    first clock edge

(b) tASC: the interval between the last clock edge and the deassertion
    of the chip select

What is a bit surprising, but is documented in the figure "Example of
continuous transfer (CPHA=1, CONT=1)" in the datasheet, is that when the
chip select stays asserted between multiple TX FIFO writes, the tCSC and
tASC times still apply. With CONT=1, chip select remains asserted, but
SCK takes a break and goes to the idle state for tASC + tCSC ns.

In other words, the default values (of 0 and 0 ns) result in SCK
glitches where the SCK transition to the idle state, as well as the SCK
transition from the idle state, will have no delay in between, and it
may appear that a SCK cycle has simply gone missing. The resulting
timing violation might cause data corruption in many peripherals, as
their chip select is asserted.

The driver has device tree bindings for tCSC ("fsl,spi-cs-sck-delay")
and tASC ("fsl,spi-sck-cs-delay"), but these are only specified to apply
when the chip select toggles in the first place, and this timing
characteristic depends on each peripheral. Many peripherals do not have
explicit timing requirements, so many device trees do not have these
properties present at all.

Nonetheless, the lack of SCK glitches is a common sense requirement, and
since the SCK stays in the idle state during transfers for tCSC+tASC ns,
and that in itself should look like half a cycle, then let's ensure that
tCSC and tASC are at least a quarter of a SCK period, such that their
sum is at least half of one.

Fixes: 95bf15f38641 ("spi: fsl-dspi: Add ~50ns delay between cs and sck")
Reported-by: Lisa Chen (陈敏捷) <minjie.chen@geekplus.com>
Debugged-by: Lisa Chen (陈敏捷) <minjie.chen@geekplus.com>
Tested-by: Lisa Chen (陈敏捷) <minjie.chen@geekplus.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/spi/spi-fsl-dspi.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Vladimir Oltean June 7, 2023, 12:03 p.m. UTC | #1 
Hi Mark,

On Tue, May 30, 2023 at 01:34:02AM +0300, Vladimir Oltean wrote:
> In other words, the default values (of 0 and 0 ns) result in SCK
> glitches where the SCK transition to the idle state, as well as the SCK
> transition from the idle state, will have no delay in between, and it
> may appear that a SCK cycle has simply gone missing. The resulting
> timing violation might cause data corruption in many peripherals, as
> their chip select is asserted.

I know you don't appreciate content-free pings, but is this patch on
your radar?

Thanks,
Vladimir
Mark Brown June 7, 2023, 12:17 p.m. UTC | #2 
On Tue, 30 May 2023 01:34:02 +0300, Vladimir Oltean wrote:
> The DSPI controller has configurable timing for
> 
> (a) tCSC: the interval between the assertion of the chip select and the
>     first clock edge
> 
> (b) tASC: the interval between the last clock edge and the deassertion
>     of the chip select
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: fsl-dspi: avoid SCK glitches with continuous transfers
      commit: c5c31fb71f16ba75bad4ade208abbae225305b65

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
Mark Brown June 7, 2023, 2:02 p.m. UTC | #3 
On Wed, Jun 07, 2023 at 03:03:44PM +0300, Vladimir Oltean wrote:
> On Tue, May 30, 2023 at 01:34:02AM +0300, Vladimir Oltean wrote:

> I know you don't appreciate content-free pings, but is this patch on
> your radar?

It's only been a week, please allow a reasonable time for review
especially when there may be other people who work on the driver and
should be given a chance to review as is the case here.  Had I not
already put this into my CI I'd most likely give it a bit longer...

Please don't send content free pings and please allow a reasonable time
for review.  People get busy, go on holiday, attend conferences and so 
on so unless there is some reason for urgency (like critical bug fixes)
please allow at least a couple of weeks for review.  If there have been
review comments then people may be waiting for those to be addressed.

Sending content free pings adds to the mail volume (if they are seen at
all) which is often the problem and since they can't be reviewed
directly if something has gone wrong you'll have to resend the patches
anyway, so sending again is generally a better approach though there are
some other maintainers who like them - if in doubt look at how patches
for the subsystem are normally handled.
diff mbox series

Patch

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 4339485d202c..674cfe05f411 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -1002,7 +1002,9 @@  static int dspi_transfer_one_message(struct spi_controller *ctlr,
 static int dspi_setup(struct spi_device *spi)
 {
 	struct fsl_dspi *dspi = spi_controller_get_devdata(spi->controller);
+	u32 period_ns = DIV_ROUND_UP(NSEC_PER_SEC, spi->max_speed_hz);
 	unsigned char br = 0, pbr = 0, pcssck = 0, cssck = 0;
+	u32 quarter_period_ns = DIV_ROUND_UP(period_ns, 4);
 	u32 cs_sck_delay = 0, sck_cs_delay = 0;
 	struct fsl_dspi_platform_data *pdata;
 	unsigned char pasc = 0, asc = 0;
@@ -1031,6 +1033,19 @@  static int dspi_setup(struct spi_device *spi)
 		sck_cs_delay = pdata->sck_cs_delay;
 	}
 
+	/* Since tCSC and tASC apply to continuous transfers too, avoid SCK
+	 * glitches of half a cycle by never allowing tCSC + tASC to go below
+	 * half a SCK period.
+	 */
+	if (cs_sck_delay < quarter_period_ns)
+		cs_sck_delay = quarter_period_ns;
+	if (sck_cs_delay < quarter_period_ns)
+		sck_cs_delay = quarter_period_ns;
+
+	dev_dbg(&spi->dev,
+		"DSPI controller timing params: CS-to-SCK delay %u ns, SCK-to-CS delay %u ns\n",
+		cs_sck_delay, sck_cs_delay);
+
 	clkrate = clk_get_rate(dspi->clk);
 	hz_to_spi_baud(&pbr, &br, spi->max_speed_hz, clkrate);