Skip to content

Enabling PM for udc_mcux_ehci #90812

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+117 −8
Open

Enabling PM for udc_mcux_ehci #90812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 92 additions & 8 deletions drivers/usb/udc/udc_mcux_ehci.c
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The author's goal for this patch is unclear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if this was unclear, my only goal is to support the current Power Management Hook that is presented in Zephyr, adding nxp usb as a PM_DEVICE when CONFIG_PM=y and supporting the pm_action callback api.
At the moment, this driver does not have enough context to reinit any lost context about the zephyr USB stack when waking up from STANDBY so the driver will only go into a lower power mode when in Standby.

Copy link
Member

@decsny decsny Jun 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of the PR is to lock out the system power states that disable the USB peripheral when the USB is expected to be in use.

I think that eventually this should be handled by @bjarki-andreasen 's rework of PM from calling pm_device_runtime_get/put at the subsystem level, but for now we are trying to support the coherency of the "system managed device PM" paradigm that currently exists.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of the PR is to lock out the system power states that disable the USB peripheral when the USB is expected to be in use.

It does not explain why this needs to be done in the driver rather than in the application, where application can determine the state of the USB device controller and set PM accordingly.

I think that eventually this should be handled by @bjarki-andreasen 's rework of PM from calling pm_device_runtime_get/put at the subsystem level, but for now we are trying to support the coherency of the "system managed device PM" paradigm that currently exists.

It looks like we should wait until the PM issue has been resolved. This does not prevent you from using this patch in your downstream repository, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of the PR is to lock out the system power states that disable the USB peripheral when the USB is expected to be in use.

It does not explain why this needs to be done in the driver rather than in the application, where application can determine the state of the USB device controller and set PM accordingly.

Does it strictly need to be addressed by the driver, no, but also, the application doesn't strictly need to use a driver in the first place, does it? It doesn't need to even use the Zephyr ecosystem at all. It can just write bare metal code to control a USB without the stack, so why shouldn't it? So what is the problem with drivers being written robustly by contributing their knowledge to the system state?

I think that eventually this should be handled by @bjarki-andreasen 's rework of PM from calling pm_device_runtime_get/put at the subsystem level, but for now we are trying to support the coherency of the "system managed device PM" paradigm that currently exists.

It looks like we should wait until the PM issue has been resolved. This does not prevent you from using this patch in your downstream repository, though.

I don't agree that we either should or even need to wait. This code will be valid both before and after that PM reworking. What I said was that in my opinion, the subsystem level should contribute here, but I don't think that the driver can't also. Any component that has some information about the state of the system and the implications of that state, should utilize whichever mechanisms are available to it to contribute that knowledge to it's architectural connections.

Also, this fallback to "just use the patch in your downstream repository" is not a real thing. Nobody prefers to use a downstream repository. Everybody wants things in upstream releases. Maybe Nordic customers are used to it now, but it's not true universally.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ain't noone happy to be forced to some downstream that's for sure :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it strictly need to be addressed by the driver, no, but also, the application doesn't strictly need to use a driver in the first place, does it? It doesn't need to even use the Zephyr ecosystem at all. It can just write bare metal code to control a USB without the stack, so why shouldn't it? So what is the problem with drivers being written robustly by contributing their knowledge to the system state?

These changes are anything but robust. You have no idea how the UDC driver and USB device stack work, and you're just poking around. The UDC driver and the stack are already contributing all the bus states to the application. Only the USB application is aware of all the USB bus and USB device related states, such as battery charging. And as I said, the USB application should handle PM on its own in a generic way. Therefore, performing any PM on the driver level is pointless. Changes like these are not going anywhere in USB.

Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
#include "usb_phy.h"

#include <zephyr/logging/log.h>
#include <zephyr/pm/device.h>
#include <zephyr/pm/policy.h>
LOG_MODULE_REGISTER(udc_mcux, CONFIG_UDC_DRIVER_LOG_LEVEL);

/*
Expand All @@ -34,6 +36,7 @@ LOG_MODULE_REGISTER(udc_mcux, CONFIG_UDC_DRIVER_LOG_LEVEL);

#define PRV_DATA_HANDLE(_handle) CONTAINER_OF(_handle, struct udc_mcux_data, mcux_device)


struct udc_mcux_config {
const usb_device_controller_interface_struct_t *mcux_if;
void (*irq_enable_func)(const struct device *dev);
Expand All @@ -51,6 +54,8 @@ struct udc_mcux_data {
usb_device_struct_t mcux_device;
struct k_work work;
struct k_fifo fifo;
volatile bool enabled;
volatile bool vbus_present;
Comment on lines +57 to +58
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the intention behind using volatile here? Is it an LLM suggestion? Are there any side effects that the compiler is not aware of?

Copy link
Contributor

@bjarki-andreasen bjarki-andreasen Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enabled and vbus_present are being used as atomics, accessed from multiple contexts, volatile lets the compiler know that the value can be changed by another context at any time, so it has to be read every time its used, the compiler can't cache the value. For example:

volatile bool foo = false;
volatile bool bar = true;

if (foo) {
        do_foo();
        return;
}

/* ISR happens here and changes the value of foo to true */

/* 
 * compiler will "likely" not check foo again here, it will optimize out this branch entirely since
 * since foo had to be false to get here, so foo && bar is not possible without volatile
 */
if (foo && bar) {
        do_foobar()
}

without the volatile keyword, the compiler would not check foo again after the ISR, it will optimize the code to just check foo once, and never check bar :)

now, this should optimally be changed to using an atomic_t, but for archs where you know a reading bool is an atomic op, its not neccesary :)

uint8_t controller_id; /* 0xFF is invalid value */
};

Expand Down Expand Up @@ -92,6 +97,41 @@ static int udc_mcux_control(const struct device *dev, usb_device_control_type_t
return 0;
}

static void udc_mcux_change_state(const struct device *dev, const bool enabled,
const bool vbus_present)
{
struct udc_mcux_data *data = udc_get_private(dev);

if (data->enabled == enabled && data->vbus_present == vbus_present) {
return;
}

if (vbus_present != data->vbus_present) {
udc_submit_event(data->dev,
vbus_present ? UDC_EVT_VBUS_READY : UDC_EVT_VBUS_REMOVED, 0);
}

if (enabled && vbus_present) {

/*
* Block PM when usb is receives VBUS signal or device
* is explicitly told to be enabled.
*/
pm_policy_device_power_lock_get(dev);
} else if (data->enabled && data->vbus_present) {

/*
* USB was previously busy, but now has either lost
* VBUS signal or application has disabled udc.
* UDC will now unblock PM.
*/
pm_policy_device_power_lock_put(dev);
}

data->enabled = enabled;
data->vbus_present = vbus_present;
}

/* If ep is busy, return busy. Otherwise feed the buf to controller */
static int udc_mcux_ep_feed(const struct device *dev,
struct udc_ep_config *const cfg,
Expand Down Expand Up @@ -525,10 +565,10 @@ usb_status_t USB_DeviceNotificationTrigger(void *handle, void *msg)
case kUSB_DeviceNotifyLPMSleep:
break;
case kUSB_DeviceNotifyDetach:
udc_submit_event(dev, UDC_EVT_VBUS_REMOVED, 0);
udc_mcux_change_state(dev, priv->enabled, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

udc_submit_event(dev, UDC_EVT_VBUS_REMOVED, 0); should be submitted here regardless of other states.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do the change I suggested in change_state() then this would occur in that function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did the change mentioned by @dleach02 on line 109

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/zephyrproject-rtos/zephyr/pull/90812/files#r2115659238 not addressed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be in USB isr context, is it OK to call the pm related functions here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am unaware of the impact with PM and a device being in ISR.
I will state that it WILL be in ISR as these functions are only
called during the USB ISR. The device seems to work as intended
with or without PM from this PR, so nothing immediate sticks out
as a potential problem.

break;
case kUSB_DeviceNotifyAttach:
udc_submit_event(dev, UDC_EVT_VBUS_READY, 0);
udc_mcux_change_state(dev, priv->enabled, 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case github gets busy, the change in question is being addressed on line 109

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, not addressed.

break;
case kUSB_DeviceNotifySOF:
udc_submit_event(dev, UDC_EVT_SOF, 0);
Expand Down Expand Up @@ -676,11 +716,19 @@ static int udc_mcux_set_address(const struct device *dev, const uint8_t addr)

static int udc_mcux_enable(const struct device *dev)
{
struct udc_mcux_data *priv = udc_get_private(dev);

udc_mcux_change_state(dev, 1, priv->vbus_present);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

udc_mcux_enable may be called from application's tasks, and udc_mcux_change_state is called from the USB isr too, I think protection is needed to make sure udc_mcux_change_state is executed one by one and the enabled and vbus_present are updated rightly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made this variables volatile so they are only updated one at a time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, the volatile seems to not resolve this issue. For example: the application task calls udc_mcux_enable and execute to https://github.com/zephyrproject-rtos/zephyr/pull/90812/files#diff-52d8db5d4d6fc08e4356d9e8679426feb806b9cabba356a83f0ed8471a43e77aR131 then usb isr occurs and execute udc_mcux_change_state again. data->enabled and data->vbus_present are set firstly in usb isr and then be overwritten by udc_mcux_enable.

I have a question: Does the USB controller lose the registers' values after PM enter low power and wake up again? If yes, I suggest to call udc_mcux_change_state in udc_mcux_init and udc_mcux_shutdown before/after enabling/disabling the usb isr, so the usb isr is not enabled when calling udc_mcux_change_state. And if the lower power lose the USB controller registers, udc_mcux_init must be called again after wake up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

udc_mcux_enable() should not emit UDC_EVT_VBUS_READY because there is no reason to do it at this point.

Copy link
Member

@decsny decsny Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of the way this is called is that udc_mcux_enable is not trying to affect the state tracking of whether vbus is present. Explain how it could be, otherwise your comment here doesn't make sense to me.


return udc_mcux_control(dev, kUSB_DeviceControlRun, NULL);
}

static int udc_mcux_disable(const struct device *dev)
{
struct udc_mcux_data *priv = udc_get_private(dev);

udc_mcux_change_state(dev, 0, priv->vbus_present);

return udc_mcux_control(dev, kUSB_DeviceControlStop, NULL);
}

Expand Down Expand Up @@ -759,7 +807,9 @@ static inline void udc_mcux_get_hal_driver_id(struct udc_mcux_data *priv,
}
}

static int udc_mcux_driver_preinit(const struct device *dev)


static int udc_mcux_init_common(const struct device *dev)
{
const struct udc_mcux_config *config = dev->config;
struct udc_data *data = dev->data;
Expand All @@ -771,10 +821,6 @@ static int udc_mcux_driver_preinit(const struct device *dev)
return -ENOMEM;
}

k_mutex_init(&data->mutex);
k_fifo_init(&priv->fifo);
k_work_init(&priv->work, udc_mcux_work_handler);

for (int i = 0; i < config->num_of_eps; i++) {
config->ep_cfg_out[i].caps.out = 1;
if (i == 0) {
Expand Down Expand Up @@ -823,11 +869,48 @@ static int udc_mcux_driver_preinit(const struct device *dev)
data->caps.hs = true;
priv->dev = dev;


pinctrl_apply_state(config->pincfg, PINCTRL_STATE_DEFAULT);

return 0;
}

static int udc_mcux_driver_preinit(const struct device *dev)
{
struct udc_data *data = dev->data;
struct udc_mcux_data *priv = data->priv;

k_mutex_init(&data->mutex);
k_fifo_init(&priv->fifo);
k_work_init(&priv->work, udc_mcux_work_handler);
return udc_mcux_init_common(dev);
}
Comment on lines +878 to +887
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preinit here is what the device init function is supposed to do :)


#ifdef CONFIG_PM_DEVICE
static int udc_mcux_pm_action(const struct device *dev, enum pm_device_action action)
{
struct udc_mcux_data *priv = udc_get_private(dev);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from whether PM usage makes sense or not, it should be like

	if (IS_ENABLED(CONFIG_PM)) {
		return -ENOTSUP;
	}

if not possible, then it should be fixed in PM subsytem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jfischer-no This function needs to be wrapped with CONFIG_PM_DEVICE because of the way the PM macros work. At the bottom, PM_DEVICE_DT_INST_DEFINE() is setting things up IF the application has been defined for PM_DEVICE. If it hasn't, then the PM_DEVICE_DT_INST_DEFINE() is a null and to avoid "unused function" we need to wrap this overall function with the #if.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what pm_device_driver_init is for, it allows you to reuse the pm hook implementations if PM_DEVICE is n

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it hasn't, then the PM_DEVICE_DT_INST_DEFINE() is a null and to avoid "unused function" we need to wrap this overall function with the #if.

No.

diff --git a/drivers/usb/udc/udc_mcux_ehci.c b/drivers/usb/udc/udc_mcux_ehci.c
index 1c8ddd12b58..5f0edc0b419 100644
--- a/drivers/usb/udc/udc_mcux_ehci.c
+++ b/drivers/usb/udc/udc_mcux_ehci.c
@@ -886,8 +886,7 @@ static int udc_mcux_driver_preinit(const struct device *dev)
        return udc_mcux_init_common(dev);
 }
 
-#ifdef CONFIG_PM_DEVICE
-static int udc_mcux_pm_action(const struct device *dev, enum pm_device_action action)
+static inline int udc_mcux_pm_action(const struct device *dev, enum pm_device_action action)
 {
        struct udc_mcux_data *priv = udc_get_private(dev);
 
@@ -909,7 +908,6 @@ static int udc_mcux_pm_action(const struct device *dev, enum pm_device_action ac
        }
        return 0;
 }
-#endif /* CONFIG_PM_DEVICE */
 
 static const struct udc_api udc_mcux_api = {
        .device_speed = udc_mcux_device_speed,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jfischer-no No what? Use your words. You did not respond to the point that @dleach02 actually made. Are you claiming that what he said is not true?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Statement "If it hasn't, then the PM_DEVICE_DT_INST_DEFINE() is a null and to avoid "unused function" we need to wrap this overall function with the #if." is false.
Therefore my answer is 'No.', followed by a proof inside a diff that can be applied to the commit.

Copy link
Contributor

@bjarki-andreasen bjarki-andreasen Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This diff is not solving the issue "in spirit" :) making it inline is like adding __maybe_unused :) I will try to review the PR, though, I know nothing about USB, other than what it's shorthand for :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This diff is not solving the issue "in spirit" :) making it inline is like adding __maybe_unused :)

And allow the compiler to optimize and discard it in a similar way to if (IS_ENABLED(CONFIG_PM)) {return ...}. This is nothing new and is used in the project in many places.

switch (action) {
case PM_DEVICE_ACTION_RESUME:
case PM_DEVICE_ACTION_SUSPEND:
break;
case PM_DEVICE_ACTION_TURN_OFF:
udc_mcux_shutdown(dev);
break;
case PM_DEVICE_ACTION_TURN_ON:
udc_mcux_init_common(dev);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just my concern from this part of codes: Why the PM_DEVICE_ACTION_TURN_OFF doesn't need any codes? From the name of the ON/OFF, it seems that it is more matched with the controller functions udc_mcux_init/udc_mcux_shutdown. And I think it is dangerous that enable/disable controller function without going through the stack because the stack's states are mismatched with controller states.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point I can update the code to add udc_mcux_shutdown.

if (priv->enabled) {
udc_mcux_control(dev, kUSB_DeviceControlRun, NULL);
}
break;
default:
return -ENOTSUP;
}
return 0;
}
#endif /* CONFIG_PM_DEVICE */

static const struct udc_api udc_mcux_api = {
.device_speed = udc_mcux_device_speed,
.ep_enqueue = udc_mcux_ep_enqueue,
Expand Down Expand Up @@ -917,7 +1000,8 @@ static usb_phy_config_struct_t phy_config_##n = { \
.priv = &priv_data_##n, \
}; \
\
DEVICE_DT_INST_DEFINE(n, udc_mcux_driver_preinit, NULL, \
PM_DEVICE_DT_INST_DEFINE(n, udc_mcux_pm_action); \
DEVICE_DT_INST_DEFINE(n, udc_mcux_driver_preinit, PM_DEVICE_DT_INST_GET(n), \
&udc_data_##n, &priv_config_##n, \
POST_KERNEL, CONFIG_KERNEL_INIT_PRIORITY_DEVICE, \
&udc_mcux_api);
Expand Down
1 change: 1 addition & 0 deletions dts/arm/nxp/nxp_rw6xx_common.dtsi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@
interrupt-names = "usb_otg";
num-bidir-endpoints = <8>;
power-domains = <&power_mode3_domain>;
zephyr,disabling-power-states = <&suspend &standby>;
status = "disabled";
};

Expand Down
2 changes: 2 additions & 0 deletions modules/hal_nxp/mcux/mcux-sdk-ng/middleware/usb_device_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,8 @@ BUILD_ASSERT(NUM_INSTS <= 1, "Only one USB device supported");
*/
#define USB_DEVICE_CONFIG_SOF_NOTIFICATIONS (1U)

#define USB_DEVICE_CONFIG_DETACH_ENABLE (1U)

#endif

#endif /* __USB_DEVICE_CONFIG_H__ */
17 changes: 17 additions & 0 deletions samples/subsys/usb/console-next/boards/frdm_rw612.overlay
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/*
* Copyright 2025 NXP
*
* SPDX-License-Identifier: Apache-2.0
*/

/ {
chosen {
zephyr,console = &cdc_acm_uart0;
};
};

&zephyr_udc0 {
cdc_acm_uart0: cdc_acm_uart0 {
compatible = "zephyr,cdc-acm-uart";
};
};
Comment on lines +1 to +17
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, no trivial overlays. This sample must not have any board overlays either.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have not answered the question asked to you above of how this is supposed to be done otherwise, since this configuration is specific to this sample. For this board, zephyr,console is not normally this CDC ACM USB. What would you have the author do, specifically, besides remove the overlay, because that alone obviously does not align with the goal of the patch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we establish these nodes if they are not part of the core board/soc DTS?

Just as we have been doing for a long time. Have any of you thought about how this sample is used with other boards?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon my inspection, it looks like, the sample uses DEVICE_DT_GET_ONE, instead of zephyr,console, and uses a special nodelabel for how to define which node the cdc acm uart will go on with app.overlay . So @EmilioCBen you can make that change. However, @jfischer-no your comments are completely not helpful or collaborative. I am obviously not actually the one working on this PR but even if I was, I would be annoyed at how you are leaving vague riddles as reviews instead of just saying what you know that the author didn't and what exactly you want changed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon my inspection, it looks like, the sample uses DEVICE_DT_GET_ONE, instead of zephyr,console, and uses a special nodelabel for how to define which node the cdc acm uart will go on with app.overlay .

If you really inspected it, you would discover that it uses DEVICE_DT_GET(DT_CHOSEN())

zephyr/samples/subsys/usb/console-next/src/main.c

Line 16 in c906d4a

const struct device *const dev = DEVICE_DT_GET(DT_CHOSEN(zephyr_console));

It is also obvious that you don't even need to look at the main.c, the overlay provides all the information you need.

However, @jfischer-no your comments are completely not helpful or collaborative. I am obviously not actually the one working on this PR but even if I was, I would be annoyed at how you are leaving vague riddles as reviews instead of just saying what you know that the author didn't and what exactly you want changed.

It is the responsibility of the PR author to familiarize themselves with the code they are changing. The same applies to colleagues passing by. It is irresponsible and reckless to poke around, waste other people's time, and expect everything to be explained. It is quite strange that none of you looked at the sample and saw that the app.overlay file contains the same content as the board overlay file, or questioned how that works for the other boards. So please hold back with your accusations, I do not accept them.

Finally, it is not too much to ask that people read the fancy manual
https://docs.zephyrproject.org/latest/build/dts/howtos.html#set-devicetree-overlays
https://docs.zephyrproject.org/latest/connectivity/usb/device/usb_device.html#console-over-cdc-acm-uart

Copy link
Member

@decsny decsny Jun 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I guess I looked at the old USB stack sample. Again, not working on the PR myself, but I am looking at this PR from an outside perspective and seeing your comments on it which are clearly not giving any indication of what you know that the PR author might have missed, that you are basically just making them play a game show to figure out what you mean. And I don't need to be an expert on USB or even know what a computer is to see when a person is not "playing nicely in the sandbox".

You might be a maintainer, but you seem to be mistaken in thinking that means you have a permit to be a jackass. In fact, you don't have less responsibility to stewarding the upstream, you have more. From being a maintainer, you become an assignee on PRs in your area, such as this one, which means that you are responsible to moderate this thread and resolve the disputes among the authors, maintainers (including yourself), and reviewers in order to avoid having to escalate. I don't see how you can be trusted to be moderating if you are the one causing the issues by intentionally making it a struggle to even understand what you are saying out of spite for when somebody makes a mistake in your eyes on their PR. That is just immature. https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html

Either way. I agree with you to the extent that a PR author obviously should be responsible for making a good faith effort to properly know what they are doing when they make a PR submitting patches. However, that does not equate to meaning that everybody has to be perfect and omniscient or else F off and ignore them. At the end of the day, humans make mistakes, and don't know what they don't know they don't know. We aren't like the AI which you apparently so vehemently hate (although ironically, I suspect you would get along better with) which seem to have unlimited amounts of knowledge and time.

Lastly, simply just read https://docs.zephyrproject.org/latest/contribute/reviewer_expectations.html to understand that, regardless of your opinion, you have to follow these rules of what is expected of you if you are going to review PRs in the upstream. And I think you have either broken or neglected quite a few of them on this thread already.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just came here to echo, on behalf of the project's Code of Conduct team, what @decsny very articulately said in his comment just above. Please do try to keep your feedback and request for changes clear and actionable.

5 changes: 5 additions & 0 deletions soc/nxp/rw/power.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@

LOG_MODULE_DECLARE(soc, CONFIG_SOC_LOG_LEVEL);

#if CONFIG_PM && CONFIG_UDC_NXP_EHCI && DT_NODE_HAS_STATUS(DT_NODELABEL(standby), okay)
#error "UDC EHCI CURRENTLY DOES NOT SUPPORT STANDBY MODE"
#endif


/* Active mode */
#define POWER_MODE0 0
/* Idle mode */
Expand Down
Loading