GIGA: after merging previous changes: SPI1.transfer hangs board

[KurtE]

After this last batch of Pull Requests, several of my sketches failed to run. They would hang or look like a reboot
when SPI1.tranfer was called. SPI worked but SPI1 did not.

Most of the time I was using a real simple sketch to debug

#define SPIX SPI1
#include <SPI.h>
#define CS_PIN 10
void setup() {
    Serial.begin(115200);
    while (!Serial && millis() < 5000) {}
    pinMode(CS_PIN, OUTPUT);
    digitalWrite(CS_PIN, HIGH);

    SPIX.begin();
    SPIX.beginTransaction(SPISettings(3000000, MSBFIRST, SPI_MODE0));
    pinMode(LED_BUILTIN, OUTPUT);
}

void loop() {
    digitalWrite(CS_PIN, LOW);
    digitalWrite(LED_BUILTIN, HIGH);
    for (uint8_t i = 'a'; i <= 'f'; i++) SPIX.transfer(i);
    digitalWrite(LED_BUILTIN, LOW);
    digitalWrite(CS_PIN, HIGH);
    delay(50);
    Serial.println("Paused");
    while (Serial.read() == -1) {}
    while (Serial.read() != -1) {}
}

I am not sure if anyone would want to see all of the debug stuff I put into the zephyr spi code:

spi_ll_stm32.zip
But included it just in case.

We found that it hung in the function:

/* Shift a SPI frame as master. */
static void spi_stm32_shift_m(const struct spi_stm32_config *cfg,
			      struct spi_stm32_data *data)
{
	if (cfg->fifo_enabled) {
		spi_stm32_shift_fifo(cfg->spi, data);
	} else {
		printk("@1");
		while (!ll_func_tx_is_not_full(cfg->spi)) {
			/* NOP */
		}

		printk("@2%x", LL_SPI_ReadReg(cfg->spi, SR));
		spi_stm32_send_next_frame(cfg->spi, data);

		printk("@3");
		uint8_t loop_count = 0;
		uint32_t error_count = 0;
		while (!ll_func_rx_is_not_empty(cfg->spi)) {
			loop_count++;
			if ((loop_count == 0) && (error_count < 5)) {
				printk("\t%x\n",LL_SPI_ReadReg(cfg->spi, SR));
				error_count++;
			}
			/* NOP */
		}
		printk("@4");

		spi_stm32_read_next_frame(cfg->spi, data);
		printk("@5\n");
	}
}

in the while (!ll_func_rx_is_not_empty(cfg->spi)) ...

A debug output run:

uart:~$ sketch
Enter transceive(0x805bb18 0x2400f568 0x2400d350 0x2400d360)
spi_stm32_[00:00:12.460,000] <inf> usb_cdc_acm: Device suspended
configure(0x805bb18, 0x2400f568)
        freq: 3000000 clock:120000000 br:6
        hardware CS
[00:00:12.473,000] <dbg> spi_ll_stm32: spi_stm32_configure: Installed config 0x2400f568: freq 1875000Hz (div = 64), mode 0/0/0, slave 0
[00:00:12.485,000] <dbg> spi_ll_stm32: spi_context_buffers_setup: tx_bufs 0x2400d350 - rx_bufs 0x2400d360 - 1
[00:00:12.496,000] <dbg> spi_ll_stm32: spi_context_buffers_setup: current_tx 0x2400d348 (1), current_rx 0x2400d358 (1), tx buf/len 0x2400d33f/1, rx buf/len 0x2400d347/1
        Call LL_SPI_StartMasterTransfer
        returned - Wait while !
        IsActiveMasterTransfer
        Call spi_stm32_cs_control
SPI: 0x40015000 00000201 00000000 50070007 20400000 00000000 00001002 00000000 00000000
RCC: clks:00000000 RST 1:0 5:0 EN 1:1000 5:100000
SPI init called on: 0x40013000 0x40015000 0 0 0
        Call ll_func_enable_int_errors
        Call ll_func_enable_int_tx_empty
I:1002 3e3
SPI pins: 5 5 5 : SPI1 2 3 1
SF M N
@1@21002[00:00:12.548,000] <dbg> spi_ll_stm32: spi_context_update_tx: tx buf/len 0/0
@3      1002
        1002
        1002
        1002
        1002

I also have had it running with Logic Analyzer hooked up to pins 10-13, and only pin 10 showed anything.
So I wondered about what were the IO pins for SPI1 configured for:

So I added the following to the start of the transfer code... Actually to the start of the ISR...

static void spi_stm32_isr(const struct device *dev)
{
	const struct spi_stm32_config *cfg = dev->config;
	struct spi_stm32_data *data = dev->data;
	SPI_TypeDef *spi = cfg->spi;
	int err;

	printk("I:%x %x\n", LL_SPI_ReadReg(spi, SR), LL_SPI_ReadReg(spi, IER));
	// PB3 PD7 PG9 : PH6 PJ10 PJ11
	printk("SPI pins: %x %x %x : SPI1 %x %x %x\n",
		(GPIOB->AFR[0] >> (3 * 4 )) & 0xf, (GPIOD->AFR[0] >> (7 * 4 )) & 0xf, (GPIOG->AFR[1] >> ((9-8) * 4 )) & 0xf,
		(GPIOH->AFR[0] >> (6 * 4 )) & 0xf, (GPIOJ->AFR[1] >> ((10-8) * 4 )) & 0xf, ((GPIOJ->AFR[1] >> ((11-8) * 4 )) & 0xf));

I ran the sketch using only SPI1, but the output was shown above, but:
SPI pins: 5 5 5 : SPI1 2 3 1
The SPI (spi1) pins make sense as the Alternate functions for those pins are:

The one for SPI1 (spi5) - don't, need to double check pin 13s one but the other two are timers

Then remembered that these pins were added to the PWM list, and I am guessing probably all of the pins from pin2-13
currently have their alternate pin function set to the PWM timer used.

I did a quick and dirty removal of the timer entries for pins 11-13 from the overlay and rebuilt and then SPI1 started working again.

Will probably introduce a PR to do this, at least for now. Hopefully at some point there will be a system in place that allows
the sketch to select what usage that want for each pin. I know that there is a discussion going on with zephyr on this.

The good news is that I am learning a lot more about parts of the system :D The bad news is, it took a lot longer to figure this out
than it should have.

[mjs513]

Will attest the amount of time that went into debugging. But as @KurtE said learned a lot more. about how things work. BTW this is related to this post and the following posts: #4 (comment)

[KurtE]

Aargh - ran into this again. My current branch did not include this PR so my quick and dirty test to
see if I could read in an image from camera and display it on the ILI9341 display and it hung again on the first _pspi->transfer() call.

Took me awhile to debug enough to remember this issue/pr...

Some of the device tree stuff is a real PIA! When the device tree has multiple subsystems that can use the same pins, how does
the system decides which of them it will configure the pin to use.

This is one example, where a pin can be used for SPI or could be used for PWM, or could be used for simple GPIO. It appears, that the system handles the case for switching the pin to GPIOs MODER register to 00 or 01 depending on Input versus Output.

But it does not do this for analogRead (switch back to default 0x3).

And I don't believe anywhere it will switch pins used for alternate functions. to 0x2 and the AFL registers, to the appropriate value.
Maybe the Zephyr PR: zephyrproject-rtos/zephyr#84394
Will address some of this?

Or maybe the fully linked stuff?

[pillo79]

Hello guys and sorry for the silence, we've been a little swamped by the release of 4.1 - a few more days and we will be right back!

Yes, the linked PR will help a lot, since Zephyr has never needed to "de-init" something. After all, when designing a board you should know what devices you attached to each pin, right? 🤣 So... for Zephyr a DT should never describe multiple conflicting devices. That's basically "our problem" to handle, but after that PR, if we manage the init/deinit in Wire or SPI classes, we could finally have runtime switching! 🥳

[KurtE]

Thanks @pillo79,

Will be good to get some feedback on some of the issues we have run into plus reviews of some updates.

if we manage the init/deinit in Wire or SPI classes, we could finally have runtime switching! 🥳

That would be good. It will also be interesting to see how it impacts other sub-systems. Example: PWM (analogWrite),
That if a pin was defined within one of the clocks as a PWM pin, it caused it to not work with SPI. Now maybe the init call
for SPI will resolve that one.

But we also ran into issues with analogRead in that if the pin is configured for anything else, the reads do not work. That is the
pin does not switch the MODER. Not sure if something within Zephyr should address this?

I tried a hack in a sketch, where I:

 #include "zephyrInternal.h"
 static const struct gpio_dt_spec arduino_pins[] = {DT_FOREACH_PROP_ELEM_SEP(
   DT_PATH(zephyr_user), digital_pin_gpios, GPIO_DT_SPEC_GET_BY_IDX, (, ))};


  void pinModeAnalog(pin_size_t pinNumber) {
     gpio_pin_configure_dt(&arduino_pins[pinNumber],
                           GPIO_INPUT /*| GPIO_ACTIVE_HIGH */ | GPIO_MODE_ANALOG);
 }

Which did go through and switch the mode, but it still did not appear to work.
Not sure if that is something to look at for 4.1 or later.

Thanks again