Hello,
Yes that's the correct commit I found when researching why the issue didn't happen in Linux, I already found that Tegra 2 reference docs had a different information about the register compared to Tegra 3.
I consider that a single variable is enough since the value is only non-zero when we want to set a new address. But if is necessary i can copy like Linux does.
Regards, Ion Agorria
El mar, 15 oct 2024 a las 11:56, Mattijs Korpershoek (mkorpershoek@baylibre.com) escribió:
Hi Svyatoslav,
Thank you for the patch.
On dim., oct. 13, 2024 at 17:58, Svyatoslav Ryhel clamor95@gmail.com wrote:
From: Ion Agorria ion@agorria.com
In the older USB controllers like for example in ChipIdea controller used by the Tegra 2 the "USBADRA: Device Address Advance" bitflag does not exist, so the new device address set during SET_ADDRESS can't be deferred by hardware, which causes the host to not recognize the device and give an error.
Instead store it until ep completes to apply the change into the hw register as Linux kernel does. This should fix regression on old and and be compatible with newer controllers.
Since this is based on a linux commit, can we please mention the revision in the commit message?
Per my understanding, this would be: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
Signed-off-by: Ion Agorria ion@agorria.com Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
drivers/usb/gadget/ci_udc.c | 24 +++++++++++++++++++++++- drivers/usb/gadget/ci_udc.h | 1 + 2 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c index bbe03cfff1f..4bff75da759 100644 --- a/drivers/usb/gadget/ci_udc.c +++ b/drivers/usb/gadget/ci_udc.c @@ -649,12 +649,30 @@ static void flip_ep0_direction(void) } }
+/*
- This function explicitly sets the address, without the "USBADRA" (advance)
- feature, which is not supported by older versions of the controller.
- */
+static void ci_set_address(struct ci_udc *udc, u8 address) +{
DBG("%s %x\n", __func__, address);writel(address << 25, &udc->devaddr);+}
static void handle_ep_complete(struct ci_ep *ci_ep) { struct ept_queue_item *item, *next_td; int num, in, len, j; struct ci_req *ci_req;
/* Set the device address that was previously sent by SET_ADDRESS */if (controller.next_device_address != 0) {struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;ci_set_address(udc, controller.next_device_address);controller.next_device_address = 0;}num = ci_ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK; in = (ci_ep->desc->bEndpointAddress & USB_DIR_IN) != 0; item = ci_get_qtd(num, in);@@ -783,7 +801,7 @@ static void handle_setup(void) * write address delayed (will take effect * after the next IN txn) */
writel((r.wValue << 25) | (1 << 24), &udc->devaddr);
controller.next_device_address = r.wValue; req->length = 0; usb_ep_queue(controller.gadget.ep0, req, 0); return;@@ -814,6 +832,9 @@ static void stop_activity(void) int i, num, in; struct ept_queue_head *head; struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;
ci_set_address(udc, 0);writel(readl(&udc->epcomp), &udc->epcomp);#ifdef CONFIG_CI_UDC_HAS_HOSTPC writel(readl(&udc->epsetupstat), &udc->epsetupstat); @@ -934,6 +955,7 @@ static int ci_pullup(struct usb_gadget *gadget, int is_on) struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor; if (is_on) { /* RESET */
controller.next_device_address = 0; writel(USBCMD_ITC(MICRO_8FRAME) | USBCMD_RST, &udc->usbcmd); udelay(200);diff --git a/drivers/usb/gadget/ci_udc.h b/drivers/usb/gadget/ci_udc.h index bea2f9f3fe3..807f2084c1e 100644 --- a/drivers/usb/gadget/ci_udc.h +++ b/drivers/usb/gadget/ci_udc.h @@ -105,6 +105,7 @@ struct ci_drv { struct ept_queue_head *epts; uint8_t *items_mem; struct ci_ep ep[NUM_ENDPOINTS];
u8 next_device_address;Comparing to https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...,
Can we please keep similar logic ? Having both:
- bool setaddr
- u8 address
This way, we keep the diff with the linux driver a bit lower.
};
struct ept_queue_head {
2.43.0