Endless loop in usb_dc_stm32_phy_specific_clock_enable for st_stm32n6_otghs?

[modersohn]

I have encountered what looks like a bug, but I lack the expertise to create a proper issue with test-case etc. So I'm looking for help in confirming and ideally fixing the issue.

In #84976 this code was added to usb_dc_stm32_phy_specific_clock_enable:

#elif DT_HAS_COMPAT_STATUS_OKAY(st_stm32n6_otghs)
	/* Enable Vdd USB voltage monitoring */
	LL_PWR_EnableVddUSBMonitoring();
	while (__HAL_PWR_GET_FLAG(PWR_FLAG_USB33RDY)) {
		/* Wait for VDD33USB ready */
	}
	/* Enable VDDUSB */
	LL_PWR_EnableVddUSB();
#endif

Isn't the while condition missing a negation? I have no idea if it is possible that Vdd USB voltage was already enabled, but the code looks like it's not doing what it is intended to do.

Thanks for looking into this.

[erwango]

@modersohn It is not clear if you're actually facing an issue. Can you provide more details: Board in use, Zephyr version (git describe), zephyr application running, logs ...
@mathieuchopstm FYI

[modersohn]

Thanks for looking at this. We're using the stm32n6570_dk board trying to make MCUBoot activate DFU mode with mcuboot-button0. But since I'm not the actual dev working on this, I can't quickly provide a test-case etc.

I was hoping the code would be so obviously wrong that just having a look at it would be maybe enough for someone with more knowledge on the topic.

We're using release 4.2.0 but the code is on main as of today.

[erwango]

I was hoping the code would be so obviously wrong that just having a look at it would be maybe enough for someone with more knowledge on the topic.

Sure, but OTHO it was also tested working, so it's worth understanding why it'd be failing on your side and not ours.

[modersohn]

I was now able to see it hanging endlessly in that loop in the debugger.

This is an excerpt of MCUboot.conf:

CONFIG_USB_DEVICE_STACK=y
CONFIG_USB_DEVICE_VID=0x...
CONFIG_USB_DEVICE_PID=0x....
CONFIG_USB_DEVICE_DFU_PID=0x....
CONFIG_USB_DEVICE_MANUFACTURER="xyz"
CONFIG_BOOT_USB_DFU_GPIO=y
CONFIG_USB_DFU_WILL_DETACH=y
CONFIG_USB_DFU_REBOOT=y

Then, in app.overlay define

aliases {
    mcuboot_button0 = &user_button;
};

Start the MCUBoot binary in the debugger, press the user button and let it run. Log output shows:

*** Booting MCUboot  ***
*** Using Zephyr OS build 4.2.0 ***
I: Starting bootloader
I: Waiting for USB DFU

Now on the host run dfu-util -e to detach.

Expected: device should reattach with different PID
Actual: nothing happens. When stopping the debugger, the app hangs in usb_dc_stm32_phy_specific_clock_enable.

[erwango]

@mathieuchopstm FYI

[mathieuchopstm]

Have you tried inverting the condition (i.e., while (!__HAL...)) and see if problem is solved?

[modersohn]

Yes I did add the inversion and yes it did solve the problem!

[mathieuchopstm]

Ok, I'll open a PR to integrate this change.

[mathieuchopstm]

PR open here: #97805

[mathieuchopstm]

In #84976 this code was added to usb_dc_stm32_phy_specific_clock_enable:

For the record, usb_dc driver is "no longer supported". You should try with the udc driver instead, but it seems to have the same sequence.

#elif DT_HAS_COMPAT_STATUS_OKAY(st_stm32n6_otghs)
	/* Enable Vdd USB voltage monitoring */
	LL_PWR_EnableVddUSBMonitoring();
	while (__HAL_PWR_GET_FLAG(PWR_FLAG_USB33RDY)) {
		/* Wait for VDD33USB ready */
	}
	/* Enable VDDUSB */
	LL_PWR_EnableVddUSB();
#endif

Isn't the while condition missing a negation?

This seems very plausible. According to the N6 Reference Manual (RM0486 §13.5.7 Peripheral voltage monitoring (PVM)):

The following sequence must be done before using the USB HS PHYs:
1. If VDD33USB is independent from VDD:
a) Enable the USB33VM by setting USB33VMEN in PWR_SVMCR3.
b) Wait for the USB33VM wake-up time.
c) Wait until USB33RDY is set in PWR_SVMCR3.
d) Optional: Disable the USB33VM for consumption saving.
2. Set USB33SV in PWR_SVMCR3 to remove the VDD33USB power isolation.

I suspect that since step 1.b) is missing, there is a race condition between the driver and the hardware. If the voltage monitor wakes up fast enough, an infinite loop is entered; if it doesn't, the loop is skipped but the resulting sequence is not correct.

Unfortunately, I haven't been able to find this USB33VM wake-up time in the datasheet yet...

[mathieuchopstm]

Almost all STM32CubeN6 samples have the same sequence as driver: https://github.com/search?q=repo%3ASTMicroelectronics%2FSTM32CubeN6%20%22__HAL_PWR_GET_FLAG(PWR_FLAG_USB33RDY)%22&type=code

Only one uses the "expected" sequence: https://github.com/STMicroelectronics/STM32CubeN6/blob/85170d2c6eaa438622ed879caa4d7bcc6cc3920d/Projects/STM32N6570-DK/Applications/OpenBootloader/USB_Device/Target/usbd_conf.c#L50

[modersohn]

Thanks - yes I'm aware the old USB stack is deprecated. In fact, we actually use the new stack in the application, but for testing MCUBOOT DFU this seemed easier, since device setup, including MSOSv2 Descriptors can all be enabled via config.

Since the endless loop only seems to happen in this specific scenario (i.e. Detach in DFU), my guess would be that only in that case USB33VM is already up?

[mathieuchopstm]

That's quite surprising, but possible.

[modersohn]

I just had a closer look and MCUBOOT only supports the deprecated stack, so there's not really a choice as of now.

[mathieuchopstm]

I have added this fix to the old driver as I think small fixes like these should be accepted. Let's see what maintainer says...