Re: [PATCH 1/3] usb: dwc3: allocate setup_buf with dma_alloc_coherent()

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

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