| Message ID | 20240719-u-boot-dwc3-gadget-dcache-fixup-v1-1-58a5f026ea8e@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dwc3: gadget: properly fix cache operations | expand |
Hi Neil, Thank you for the patch. On ven., juil. 19, 2024 at 15:56, Neil Armstrong <neil.armstrong@linaro.org> wrote: > Also allocate the setup_buf with dma_alloc_coherent() since it's The subject of the patch says: "usb: dwc3: allocate setup_buf with dma_alloc_coherent()" Isn't this line just repeating the title? > also consumed by the hardware DMA. > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- > drivers/usb/dwc3/core.h | 2 ++ > drivers/usb/dwc3/ep0.c | 4 ++-- > drivers/usb/dwc3/gadget.c | 8 ++++---- > 3 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 7374ce950da..ce35460c405 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -670,6 +670,7 @@ struct dwc3_scratchpad_array { > * @ep0_trb: dma address of ep0_trb > * @ep0_usb_req: dummy req used while handling STD USB requests > * @ep0_bounce_addr: dma address of ep0_bounce > + * @setup_buf_addr: dma address if setup_buf if -> of Both remarks being minor, please add: Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> > * @scratch_addr: dma address of scratchbuf > * @lock: for synchronizing > * @dev: pointer to our struct device > @@ -757,6 +758,7 @@ struct dwc3 { > dma_addr_t ep0_trb_addr; > dma_addr_t ep0_bounce_addr; > dma_addr_t scratch_addr; > + dma_addr_t setup_buf_addr; > struct dwc3_request ep0_usb_req; > > /* device lock */ > diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c > index 16b11ce3d9f..0c7e0123368 100644 > --- a/drivers/usb/dwc3/ep0.c > +++ b/drivers/usb/dwc3/ep0.c > @@ -381,7 +381,7 @@ static int dwc3_ep0_handle_status(struct dwc3 *dwc, > dep = dwc->eps[0]; > dwc->ep0_usb_req.dep = dep; > dwc->ep0_usb_req.request.length = sizeof(*response_pkt); > - dwc->ep0_usb_req.request.buf = dwc->setup_buf; > + dwc->ep0_usb_req.request.buf = (void *)dwc->setup_buf_addr; > dwc->ep0_usb_req.request.complete = dwc3_ep0_status_cmpl; > > return __dwc3_gadget_ep0_queue(dep, &dwc->ep0_usb_req); > @@ -663,7 +663,7 @@ static int dwc3_ep0_set_sel(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl) > dep = dwc->eps[0]; > dwc->ep0_usb_req.dep = dep; > dwc->ep0_usb_req.request.length = dep->endpoint.maxpacket; > - dwc->ep0_usb_req.request.buf = dwc->setup_buf; > + dwc->ep0_usb_req.request.buf = (void *)dwc->setup_buf_addr; > dwc->ep0_usb_req.request.complete = dwc3_ep0_set_sel_cmpl; > > return __dwc3_gadget_ep0_queue(dep, &dwc->ep0_usb_req); > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 7d6bcc2627f..d41b590afb8 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2622,8 +2622,8 @@ int dwc3_gadget_init(struct dwc3 *dwc) > goto err1; > } > > - dwc->setup_buf = memalign(CONFIG_SYS_CACHELINE_SIZE, > - DWC3_EP0_BOUNCE_SIZE); > + dwc->setup_buf = dma_alloc_coherent(DWC3_EP0_BOUNCE_SIZE, > + (unsigned long *)&dwc->setup_buf_addr); > if (!dwc->setup_buf) { > ret = -ENOMEM; > goto err2; > @@ -2670,7 +2670,7 @@ err4: > dma_free_coherent(dwc->ep0_bounce); > > err3: > - kfree(dwc->setup_buf); > + dma_free_coherent(dwc->setup_buf); > > err2: > dma_free_coherent(dwc->ep0_trb); > @@ -2692,7 +2692,7 @@ void dwc3_gadget_exit(struct dwc3 *dwc) > > dma_free_coherent(dwc->ep0_bounce); > > - kfree(dwc->setup_buf); > + dma_free_coherent(dwc->setup_buf); > > dma_free_coherent(dwc->ep0_trb); > > > -- > 2.34.1
On 24/07/2024 17:03, Mattijs Korpershoek wrote: > Hi Neil, > > Thank you for the patch. > > On ven., juil. 19, 2024 at 15:56, Neil Armstrong <neil.armstrong@linaro.org> wrote: > >> Also allocate the setup_buf with dma_alloc_coherent() since it's > > The subject of the patch says: > > "usb: dwc3: allocate setup_buf with dma_alloc_coherent()" > > Isn't this line just repeating the title? > >> also consumed by the hardware DMA. Yeah it's a verbose rewrite of the subject, I'll rewrite it to be less bad! thanks Neil >> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >> --- >> drivers/usb/dwc3/core.h | 2 ++ >> drivers/usb/dwc3/ep0.c | 4 ++-- >> drivers/usb/dwc3/gadget.c | 8 ++++---- >> 3 files changed, 8 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >> index 7374ce950da..ce35460c405 100644 >> --- a/drivers/usb/dwc3/core.h >> +++ b/drivers/usb/dwc3/core.h >> @@ -670,6 +670,7 @@ struct dwc3_scratchpad_array { >> * @ep0_trb: dma address of ep0_trb >> * @ep0_usb_req: dummy req used while handling STD USB requests >> * @ep0_bounce_addr: dma address of ep0_bounce >> + * @setup_buf_addr: dma address if setup_buf > > if -> of > > Both remarks being minor, please add: > > Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> > >> * @scratch_addr: dma address of scratchbuf >> * @lock: for synchronizing >> * @dev: pointer to our struct device >> @@ -757,6 +758,7 @@ struct dwc3 { >> dma_addr_t ep0_trb_addr; >> dma_addr_t ep0_bounce_addr; >> dma_addr_t scratch_addr; >> + dma_addr_t setup_buf_addr; >> struct dwc3_request ep0_usb_req; >> >> /* device lock */ >> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c >> index 16b11ce3d9f..0c7e0123368 100644 >> --- a/drivers/usb/dwc3/ep0.c >> +++ b/drivers/usb/dwc3/ep0.c >> @@ -381,7 +381,7 @@ static int dwc3_ep0_handle_status(struct dwc3 *dwc, >> dep = dwc->eps[0]; >> dwc->ep0_usb_req.dep = dep; >> dwc->ep0_usb_req.request.length = sizeof(*response_pkt); >> - dwc->ep0_usb_req.request.buf = dwc->setup_buf; >> + dwc->ep0_usb_req.request.buf = (void *)dwc->setup_buf_addr; >> dwc->ep0_usb_req.request.complete = dwc3_ep0_status_cmpl; >> >> return __dwc3_gadget_ep0_queue(dep, &dwc->ep0_usb_req); >> @@ -663,7 +663,7 @@ static int dwc3_ep0_set_sel(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl) >> dep = dwc->eps[0]; >> dwc->ep0_usb_req.dep = dep; >> dwc->ep0_usb_req.request.length = dep->endpoint.maxpacket; >> - dwc->ep0_usb_req.request.buf = dwc->setup_buf; >> + dwc->ep0_usb_req.request.buf = (void *)dwc->setup_buf_addr; >> dwc->ep0_usb_req.request.complete = dwc3_ep0_set_sel_cmpl; >> >> return __dwc3_gadget_ep0_queue(dep, &dwc->ep0_usb_req); >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 7d6bcc2627f..d41b590afb8 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -2622,8 +2622,8 @@ int dwc3_gadget_init(struct dwc3 *dwc) >> goto err1; >> } >> >> - dwc->setup_buf = memalign(CONFIG_SYS_CACHELINE_SIZE, >> - DWC3_EP0_BOUNCE_SIZE); >> + dwc->setup_buf = dma_alloc_coherent(DWC3_EP0_BOUNCE_SIZE, >> + (unsigned long *)&dwc->setup_buf_addr); >> if (!dwc->setup_buf) { >> ret = -ENOMEM; >> goto err2; >> @@ -2670,7 +2670,7 @@ err4: >> dma_free_coherent(dwc->ep0_bounce); >> >> err3: >> - kfree(dwc->setup_buf); >> + dma_free_coherent(dwc->setup_buf); >> >> err2: >> dma_free_coherent(dwc->ep0_trb); >> @@ -2692,7 +2692,7 @@ void dwc3_gadget_exit(struct dwc3 *dwc) >> >> dma_free_coherent(dwc->ep0_bounce); >> >> - kfree(dwc->setup_buf); >> + dma_free_coherent(dwc->setup_buf); >> >> dma_free_coherent(dwc->ep0_trb); >> >> >> -- >> 2.34.1
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 7374ce950da..ce35460c405 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -670,6 +670,7 @@ struct dwc3_scratchpad_array { * @ep0_trb: dma address of ep0_trb * @ep0_usb_req: dummy req used while handling STD USB requests * @ep0_bounce_addr: dma address of ep0_bounce + * @setup_buf_addr: dma address if setup_buf * @scratch_addr: dma address of scratchbuf * @lock: for synchronizing * @dev: pointer to our struct device @@ -757,6 +758,7 @@ struct dwc3 { dma_addr_t ep0_trb_addr; dma_addr_t ep0_bounce_addr; dma_addr_t scratch_addr; + dma_addr_t setup_buf_addr; struct dwc3_request ep0_usb_req; /* device lock */ diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c index 16b11ce3d9f..0c7e0123368 100644 --- a/drivers/usb/dwc3/ep0.c +++ b/drivers/usb/dwc3/ep0.c @@ -381,7 +381,7 @@ static int dwc3_ep0_handle_status(struct dwc3 *dwc, dep = dwc->eps[0]; dwc->ep0_usb_req.dep = dep; dwc->ep0_usb_req.request.length = sizeof(*response_pkt); - dwc->ep0_usb_req.request.buf = dwc->setup_buf; + dwc->ep0_usb_req.request.buf = (void *)dwc->setup_buf_addr; dwc->ep0_usb_req.request.complete = dwc3_ep0_status_cmpl; return __dwc3_gadget_ep0_queue(dep, &dwc->ep0_usb_req); @@ -663,7 +663,7 @@ static int dwc3_ep0_set_sel(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl) dep = dwc->eps[0]; dwc->ep0_usb_req.dep = dep; dwc->ep0_usb_req.request.length = dep->endpoint.maxpacket; - dwc->ep0_usb_req.request.buf = dwc->setup_buf; + dwc->ep0_usb_req.request.buf = (void *)dwc->setup_buf_addr; dwc->ep0_usb_req.request.complete = dwc3_ep0_set_sel_cmpl; return __dwc3_gadget_ep0_queue(dep, &dwc->ep0_usb_req); diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 7d6bcc2627f..d41b590afb8 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2622,8 +2622,8 @@ int dwc3_gadget_init(struct dwc3 *dwc) goto err1; } - dwc->setup_buf = memalign(CONFIG_SYS_CACHELINE_SIZE, - DWC3_EP0_BOUNCE_SIZE); + dwc->setup_buf = dma_alloc_coherent(DWC3_EP0_BOUNCE_SIZE, + (unsigned long *)&dwc->setup_buf_addr); if (!dwc->setup_buf) { ret = -ENOMEM; goto err2; @@ -2670,7 +2670,7 @@ err4: dma_free_coherent(dwc->ep0_bounce); err3: - kfree(dwc->setup_buf); + dma_free_coherent(dwc->setup_buf); err2: dma_free_coherent(dwc->ep0_trb); @@ -2692,7 +2692,7 @@ void dwc3_gadget_exit(struct dwc3 *dwc) dma_free_coherent(dwc->ep0_bounce); - kfree(dwc->setup_buf); + dma_free_coherent(dwc->setup_buf); dma_free_coherent(dwc->ep0_trb);
Also allocate the setup_buf with dma_alloc_coherent() since it's also consumed by the hardware DMA. Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> --- drivers/usb/dwc3/core.h | 2 ++ drivers/usb/dwc3/ep0.c | 4 ++-- drivers/usb/dwc3/gadget.c | 8 ++++---- 3 files changed, 8 insertions(+), 6 deletions(-)