-
Notifications
You must be signed in to change notification settings - Fork 8.1k
drivers: usb: uhc: initial support for UHC driver on Renesas RA family #82730
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.
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
base: main
Are you sure you want to change the base?
drivers: usb: uhc: initial support for UHC driver on Renesas RA family #82730
Conversation
The following west manifest projects have changed revision in this Pull Request:
✅ All manifest checks OK Note: This message is automatically posted and updated by the Manifest GitHub Action. |
9356bc5
to
68f0507
Compare
68f0507
to
00f6cce
Compare
00f6cce
to
3ce519d
Compare
a6ca200
to
02541db
Compare
02541db
to
26c8de6
Compare
Last push to rebase and solve conflict |
Hello @jfischer-no. Is there any chance this PR could be merged? All review items have been fixed but this PR is still blocked due to your change request. Could you please review and verify? |
Hello @thenguyenyf #82730 (comment) still not addressed, , there is not reason to add board overlays. |
26c8de6
to
9c56d39
Compare
Hello @jfischer-no . Please verify it |
9c56d39
to
9a10f10
Compare
Last push just to rebase main |
Hello @jfischer-no . Could you please revisit and verify this change? |
drivers/usb/uhc/uhc_renesas_ra.c
Outdated
struct uhc_renesas_ra_evt event = {.type = UHC_RENESAS_RA_EVT_HAL, | ||
.hal_evt = p_args->event}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
drivers/usb/uhc/uhc_renesas_ra.c
Outdated
int err; | ||
|
||
err = R_USBH_XferStart(&priv->uhc_ctrl, xfer->udev->addr, xfer->ep, buffer_tail, len); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this empty line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
drivers/usb/uhc/uhc_renesas_ra.c
Outdated
} | ||
|
||
static int uhc_renesas_ra_edpt_open(const struct device *dev, uint8_t dev_addr, uint8_t attrib, | ||
uint8_t ep, uint8_t max_packet_size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static int uhc_renesas_ra_edpt_open(const struct device *dev,
const uint8_t dev_addr, const uint8_t attrib,
const uint8_t ep, const uint8_t mps)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
drivers/usb/uhc/uhc_renesas_ra.c
Outdated
return -EINVAL; | ||
} | ||
|
||
usb_desc_endpoint_t ep0_desc = {.bLength = sizeof(usb_desc_endpoint_t), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable definition should be at the top of a block. Please fix style.
Why is the name ep0_desc
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. The name was changed to ep_desc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is now unnecessary type conversion, please change it to
const struct uhc_renesas_ra_config *config = dev->config;
usb_desc_endpoint_t ep_desc = {
.bDescriptorType = USB_DT_ENDPOINT,
.bEndpointAddress = ep,
.bmAttributes = {attrib},
.wMaxPacketSize = mps,
.bInterval = 0,
};
fsp_err_t err;
bus-wait-cycles: | ||
type: int | ||
description: Specifies CPU bus access wait cycles | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stray change? I do not think it is UHC relevant. Where is it used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a setting for this HWIP and being used in the HAL: https://github.com/zephyrproject-rtos/hal_renesas/blob/main/zephyr/ra/ra_cfg/fsp_cfg/r_usb_host_cfg.h#L33

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit adds a new property that is already exclusively used in the HAL driver configuration. Devicetree should be used to configure drivers in Zephyr. The HAL driver or HAL module must not depend on anything from Zephyr. You have to fix it, see https://github.com/zephyrproject-rtos/zephyr/tree/main/modules, also there is no obligation to use HAL drivers.
static const struct uhc_renesas_ra_config uhc_config_##n = { \ | ||
.pcfg = PINCTRL_DT_DEV_CONFIG_GET(DT_INST_PARENT(n)), \ | ||
.make_thread = uhc_renesas_ra_create_thread_##n, \ | ||
.udev_count = DT_PROP_OR(DT_INST_PARENT(node_id), usb_devices_count, 10), \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it a DT property and why does it need to be configured per instance? I see it only used in the shim driver. Why is 10 default value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This HWIP only support up to 10 devices connected to. So, the 0xb or higher address is invalid for this driver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does it need to be configured per instance? Are there SoC variants where it has a different value?
9a10f10
to
dc43286
Compare
First commit to support UHC driver for Renesas RA family Signed-off-by: Minh Hoang <[email protected]> Signed-off-by: The Nguyen <[email protected]>
Add device node to support UHC on Renesas RA SoCs Signed-off-by: Minh Hoang <[email protected]> Signed-off-by: The Nguyen <[email protected]>
Enable USB host support on Renesas RA boards Signed-off-by: Minh Hoang <[email protected]> Signed-off-by: The Nguyen <[email protected]>
Support USBH shell sample for renesas,ra-uhc Signed-off-by: The Nguyen <[email protected]>
dc43286
to
6303874
Compare
|
Hello @jfischer-no . Could you please re-visit this PR and verify the change request? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello all.
Some few practical details which you might be interested in before pursuing.
int ret; | ||
|
||
ret = uhc_xfer_append(dev, xfer); | ||
if (ret == 0) { | ||
uhc_renesas_ra_xfer_request(); | ||
} | ||
|
||
return ret; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is completely valid code, having the "success" exit of the function at the end helps with debugging (i.e. users trying to understand a bug they encounter).
How about flipping the ret == 0
for ret != 0
even if it uses one more line?
int ret; | |
ret = uhc_xfer_append(dev, xfer); | |
if (ret == 0) { | |
uhc_renesas_ra_xfer_request(); | |
} | |
return ret; | |
int ret; | |
ret = uhc_xfer_append(dev, xfer); | |
if (ret != 0) { | |
return ret; | |
} | |
uhc_renesas_ra_xfer_request(); | |
return 0; |
if (priv->first_xfer != NULL) { | ||
uhc_xfer_free(dev, priv->first_xfer); | ||
priv->first_xfer = NULL; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering: is it required to also uhc_xfer_free(dev, priv->last_xfer);
?
I did not follow the entire logic to be able to tell, so maybe this is not needed.
{ | ||
struct uhc_renesas_ra_evt event = {.type = UHC_RENESAS_RA_EVT_XFER}; | ||
|
||
k_msgq_put(&uhc_msgq, &event, K_NO_WAIT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the event queue is full, this could fail as it is K_NO_WAIT
, in particular under heavy load.
It could be interesting to add a LOG_WRN()
, an __ASSERT_NO_MSG()
or return the error, to help with troubleshooting.
.hal_evt = p_args->event, | ||
}; | ||
|
||
k_msgq_put(&uhc_msgq, &event, K_NO_WAIT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too in case that's something you wanted to check.
This PR to initial support for UHC driver on Renesas RA boards.