SPI114 Driver CS Delay Bug?

TLDR;

It seems as though the spi-tegra114.c has an issue, patch at the bottom vvv

Longer Version

While working through an IIO driver that uses SPI I found that the CS line did not go high for very long between transactions. I needed the CS to go high for16us but it only went high for 0.5uS

DTS

I tried modifying the values within the DTS including the following:

/ {
  spi@3230000{ /* SPI3 in 40 pin conn */
    status = "okay";
    spi-max-frequency = <50000000>;

    /delete-node/ spi@1;
    spi@0 {
      compatible = "adi,adis16507-3";
      reg = <0x0>;
      spi-cpha;
      spi-cpol;
      spi-max-frequency = <2000000>; // 2MHz

      interrupt-parent = <&tegra_main_gpio>;
      interrupts = <TEGRA194_MAIN_GPIO(Q, 5) IRQ_TYPE_EDGE_RISING>;
      reset-gpios = <&tegra_aon_gpio TEGRA194_AON_GPIO(CC, 0) GPIO_ACTIVE_LOW>;
      adi,sync-mode = <0x00>;
      controller-data {
        nvidia,enable-hw-based-cs;
        nvidia,rx-clk-tap-delay = <0x10>;
        nvidia,tx-clk-tap-delay = <0x0>;

        //nvidia,cs-inactive-cycles = <0x08>;
        nvidia,cs-inactive-cycles = <0x10>;
        nvidia,cs-setup-clk-count = <0x10>;
        nvidia,cs-hold-clk-count = <0x10>;
      };
    };
  };

But this had no effect. I then looked at the sp-tegrai114.c and found this function called after a transaction.

SPI Driver

tegra_spi_transfer_delay(xfer->delay_usecs);

This seems to control how long the chip select stays high between transactions. When I added some print statements it kept reporting the ‘delay_usecs == 0’

There are two issues with this.

Issue 1

within the struct spi_transfer there is a member called delay_usecs but there is also a sub structure called struct spi_delay the user can specify either the delay_usecs or the struct spi_delay with a delay and it’s up to the driver to figure out which one to use. A generic call within spi.h handles it.

static inline int
spi_transfer_delay_exec(struct spi_transfer *t)
{
  struct spi_delay d;

  if (t->delay_usecs) {
    d.value = t->delay_usecs;
    d.unit = SPI_DELAY_UNIT_USECS;
    return spi_delay_exec(&d, NULL);
  }

  return spi_delay_exec(&t->delay, t);
}

Issue 2

delay or delay_usecs doesn’t specify the delay between transactions, which is determined by the cs_change_delay member.

In order to specify the length of time the CS is high between packets the following should be changed:

Drver Patch for spi-tegra114.c

index 26aa7cb29cbc..8911da4ddab5 100644
--- a/drivers/spi/spi-tegra114.c
+++ b/drivers/spi/spi-tegra114.c
@@ -1366,17 +1366,6 @@ static void tegra_spi_dump_regs(struct tegra_spi_data *tspi)
                tegra_spi_readl(tspi, SPI_FIFO_STATUS));
 }

-static void tegra_spi_transfer_delay(int delay)
-{
-       if (!delay)
-               return;
-
-       if (delay >= 1000)
-               mdelay(delay / 1000);
-
-       udelay(delay % 1000);
-}
-
 static  int tegra_spi_cs_low(struct spi_device *spi, bool state)
 {
        struct tegra_spi_data *tspi = spi_controller_get_devdata(spi->master);
@@ -1504,7 +1493,7 @@ static int tegra_spi_transfer_one_message(struct spi_controller *ctrl,
                        if (cstate && cstate->cs_gpio_valid)
                                gpio_set_value(spi->cs_gpio, gval);
                        tegra_spi_writel(tspi, cmd1, SPI_COMMAND1);
-                       tegra_spi_transfer_delay(xfer->delay_usecs);
+                       spi_delay_exec(&xfer->cs_change_delay, xfer);
                        goto exit;
                } else if (list_is_last(&xfer->transfer_list,
                                        &msg->transfers)) {
@@ -1514,13 +1503,13 @@ static int tegra_spi_transfer_one_message(struct spi_controller *ctrl,
                                if (cstate && cstate->cs_gpio_valid)
                                        gpio_set_value(spi->cs_gpio, gval);
                                tegra_spi_writel(tspi, cmd1, SPI_COMMAND1);
-                               tegra_spi_transfer_delay(xfer->delay_usecs);
+                               spi_delay_exec(&xfer->cs_change_delay, xfer);
                        }
                } else if (xfer->cs_change) {
                        if (cstate && cstate->cs_gpio_valid)
                                gpio_set_value(spi->cs_gpio, gval);
                        tegra_spi_writel(tspi, cmd1, SPI_COMMAND1);
-                       tegra_spi_transfer_delay(xfer->delay_usecs);
+                       spi_delay_exec(&xfer->cs_change_delay, xfer);
                }

        }

Here is the resulting SPI Transaction; the time isn’t perfect, it’s actually a lot longer than I wanted, perhaps I can bring back the tegra_spi_transfer_delay function with a fix to account for the spi_delay struct as well as Using the ‘udelay’ function instead of the usleep_range might lead to more consistent and accurate timing.

At the moment this seems to be working but if there is anything I am missing please let me know.

Thanks for any feedback,

Dave

Hi dmc,

Are you using the devkit or custom board for Xavier NX?
What;y your Jetpack version in use?

Do you mean it could work as expected after applying the patch?

Hi KevinFFF

Thanks for getting back to me, I should have put that information in the first post.

  1. This is a custom board where we have attached the spibus spi@323000 to an analog devices IMU but this could be recreated using a loopback and logic analyzer on the devboard.
  2. Jetpack version from the command /etc/nv_tegra_release:
# R35 (release), REVISION: 3.1, GCID: 32827747, BOARD: t186ref, EABI: aarch64, DATE: Sun Mar 19 15:19:21 UTC 2023 
  1. Kernel Version from the command uname -a:
Linux nx 5.10.104-tegra #50 SMP PREEMPT Sat Sep 30 23:02:17 EDT 2023 aarch64 aarch64 aarch64 GNU/Linux 

Within the kernel-5.10 repository we applied the patch on top of this commit: 78c2b512f23554067081db44a45ae59309bc6917

Re: Do you mean it could work as expected after applying the patch?
It does work as expected after applying the patch

There’s a fix about CS for R35.3.1.
Could you apply the following patch to check if it could help for your case?
SPI TPM module support fail on jetpack 5.0.2/5.1 on jetson xavier agx - #25 by DaneLLL

Or you could update to the latest JP5.1.2(R35.4.1) to verify.

Kevin,

Thanks for pointing me to the post!

I tried the patch that was in the post and added the num-cs, and cs-gpio to the dts file:

  spi@3230000{ /* SPI3 in 40 pin conn */
    status = "okay";
    spi-max-frequency = <50000000>;
    num-cs = <1>; 
    cs-gpios = <&tegra_main_gpio TEGRA194_MAIN_GPIO(Y, 3) GPIO_ACTIVE_LOW>;

    /delete-node/ spi@1;
    spi@0 {
      compatible = "adi,adis16507-3";
      reg = <0x0>;
      spi-cpha;
      spi-cpol;
      spi-max-frequency = <2000000>; // 2MHz

      interrupt-parent = <&tegra_main_gpio>;
      interrupts = <TEGRA194_MAIN_GPIO(Q, 5) IRQ_TYPE_EDGE_RISING>;
      reset-gpios = <&tegra_aon_gpio TEGRA194_AON_GPIO(CC, 0) GPIO_ACTIVE_LOW>;
      adi,sync-mode = <0x00>;
      //adi,burst-32-enable;
      controller-data {
        nvidia,enable-hw-based-cs;
        nvidia,rx-clk-tap-delay = <0x10>;
        nvidia,tx-clk-tap-delay = <0x0>;

        //nvidia,cs-inactive-cycles = <0x10>;
        nvidia,cs-inactive-cycles = <0x20>; //Both Yield a delay of ~8us
        nvidia,cs-setup-clk-count = <0x10>;
        nvidia,cs-hold-clk-count = <0x10>;
      };
    };
  };

I did see a delay! But I couldn’t change it.
here is where I set nvidia,cs-inactive-cycles = <0x10>;

Here is where I set nvidia,cs-inactive-cycles = <0x20>;

I even set nvidia,cs-inactive-cycles = <0x00>; but still the same result.

Different Permutations

I looked into the driver to see if there were other configuration values I could set within the DTS and found: nvidia,clk-delay-between-packets. According to the driver this and nvidia,cs-inactive-cycles are exclusive so I removed nvidia,cs-inactive-cycles and used nvidia,clk-delay-between-packets = <0x20>; but it led to the same result. Based on the name I don’t want nvidia,clk-delay-between-packets because it implies it will add a delay between packets regardless of whether the cs goes high or not but I wanted to try it anyway. It led to the same delay of ~8us.

I tried disabling the nvidia,enable-hw-based-cs using the /delete-property/ nvidia,enable-hw-based-cs so that it would be software-controlled instead of hardware-controlled and it still led to the same ~8us of delay.

Based on the above it does look like the patch adds a delay for the chip select of about 8us but I couldn’t get it to budge in terms of width.

Add a print statement

I added a dev_info statement near the code of interest to show the value of the delay_usecs or the value used to add a delay. here is a small snippet of my dmesg:

...
[   56.541082] spi-tegra114 3230000.spi: delay_usecs: 0
[   56.541211] spi-tegra114 3230000.spi: delay_usecs: 0
[   56.541313] spi-tegra114 3230000.spi: delay_usecs: 0
[   56.556115] spi-tegra114 3230000.spi: delay_usecs: 0
...

Here is a snippet of adis16475.c where the IIO driver that uses the SPI device to communicate with the IMU specifies a delay of 16us

adis16475.c

#define ADIS16475_DATA(_prod_id, _timeouts)       \
{                 \
  ...
  .cs_change_delay = 16,            \
  .read_delay = 5,            \
  .write_delay = 5,
...
}

A SPI transactions can consist of a sequence of struct spi_transfer, Here is where one of them is configured for a transfer transaction, notice the cs_change_delay.value

adis.c

  struct spi_transfer xfers[] = {
    {
      .tx_buf = adis->tx,
      .bits_per_word = 8,
      .len = 2,
      .cs_change = 1,
      .delay.value = adis->data->write_delay,
      .delay.unit = SPI_DELAY_UNIT_USECS,
      .cs_change_delay.value = adis->data->cs_change_delay,
      .cs_change_delay.unit = SPI_DELAY_UNIT_USECS,
      ...
    }

This value is never propagated or used by spi-tegra114.c with the given patch.

I’ve applied the changes we made on top of this patch and it behaves as expected:

This won’t be feasible for the foreseeable future as we spend a lot of time building on top of the jetpack and it would be some work to update to the next Jetpack.

Thanks,

Dave

Do you mean that the current behavior is expected in your case after applying the patch I provided and your original patch?

Yes,

Perhaps this is more clear. There are four scenarios:

Don’t apply any patches

The chip-select line goes high for 0.5uS and I cannot change the length of the delay

Apply only the patch from the link you sent

The chip-select line goes high for ~8uS but I cannot change the length of the delay

Apply only our patch

The chip-select line goes high for a configurable amount of time with a minimum time of 0.5us

Apply both the patch from the link you sent as well as our patch

The chip-select line goes high for a configurable amount of time with a minimum time of ~8uS

Thanks for your clarification.
I’ll check the issue about changing the length of the delay with internal.

Would it affect you to send/receive the SPI packet failed?

Yes, the IMU requires the chip-select to go high for a minimum of 16uS between packets.

We would suggest just apply both patches for your use case.

This topic was automatically closed 14 days after the last reply. New replies are no longer allowed.