Re: [PATCH 4/9] usb: renesas: gadget: use generic map/unmap routines

[Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>]

Hi Felipe

Thank you for your hard work

> those routines have everything we need to map/unmap
> USB requests and it's better to use them.
> 
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
>  drivers/usb/renesas_usbhs/mod_gadget.c |   60 ++++----------------------------
>  1 files changed, 7 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
> index 528691d5f..cb92a1d 100644
> --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> @@ -165,69 +165,23 @@ static void usbhsg_queue_push(struct usbhsg_uep *uep,
>  /*
>   *		dma map/unmap
>   */
> -static int usbhsg_dma_map(struct device *dev,
> -			  struct usbhs_pkt *pkt,
> -			  enum dma_data_direction dir)
> -{
> -	struct usbhsg_request *ureq = usbhsg_pkt_to_ureq(pkt);
> -	struct usb_request *req = &ureq->req;
> -
> -	if (pkt->dma != DMA_ADDR_INVALID) {
> -		dev_err(dev, "dma is already mapped\n");
> -		return -EIO;
> -	}
> -
> -	if (req->dma == DMA_ADDR_INVALID) {
> -		pkt->dma = dma_map_single(dev, pkt->buf, pkt->length, dir);
> -	} else {
> -		dma_sync_single_for_device(dev, req->dma, req->length, dir);
> -		pkt->dma = req->dma;
> -	}
> -
> -	if (dma_mapping_error(dev, pkt->dma)) {
> -		dev_err(dev, "dma mapping error %llx\n", (u64)pkt->dma);
> -		return -EIO;
> -	}
> -
> -	return 0;
> -}
> -
> -static int usbhsg_dma_unmap(struct device *dev,
> -			    struct usbhs_pkt *pkt,
> -			    enum dma_data_direction dir)
> +static int usbhsg_dma_map_ctrl(struct usbhs_pkt *pkt, int map)
>  {
>  	struct usbhsg_request *ureq = usbhsg_pkt_to_ureq(pkt);
>  	struct usb_request *req = &ureq->req;
> -
> -	if (pkt->dma == DMA_ADDR_INVALID) {
> -		dev_err(dev, "dma is not mapped\n");
> -		return -EIO;
> -	}
> -
> -	if (req->dma == DMA_ADDR_INVALID)
> -		dma_unmap_single(dev, pkt->dma, pkt->length, dir);
> -	else
> -		dma_sync_single_for_cpu(dev, req->dma, req->length, dir);
> -
> -	pkt->dma = DMA_ADDR_INVALID;
> -
> -	return 0;
> -}
> -
> -static int usbhsg_dma_map_ctrl(struct usbhs_pkt *pkt, int map)
> -{
>  	struct usbhs_pipe *pipe = pkt->pipe;
>  	struct usbhsg_uep *uep = usbhsg_pipe_to_uep(pipe);
>  	struct usbhsg_gpriv *gpriv = usbhsg_uep_to_gpriv(uep);
> -	struct device *dev = usbhsg_gpriv_to_dev(gpriv);
>  	enum dma_data_direction dir;
>  
>  	dir = usbhs_pipe_is_dir_in(pipe) ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
>  
> -	if (map)
> -		return usbhsg_dma_map(dev, pkt, dir);
> -	else
> -		return usbhsg_dma_unmap(dev, pkt, dir);
> +	if (map) {
> +		return usb_gadget_map_request(&gpriv->gadget, req, dir);
> +	} else {
> +		usb_gadget_unmap_request(&gpriv->gadget, req, dir);
> +		return 0;
> +	}
>  }

I tried this patch, but renesas_usbhs didn't work.
It seems have some bugs.

1. renesas_usbhs dma needs pkt->dma, but this patch didn't care it.
2. dma direction seems wrong
   ("dir" needs 0/1, not DMA_xxx_DEVICE for usb_gadget_xx_request())

And, renesas_usbhs can not use scatter/gather type dma

#
# this is not super important, but
#
# int usb_gadget_map_request(struct usb_gadget *gadget,
#		struct usb_request *req, int direction)
#
# this "direction = 0/1" is a little bit un-understandable for me.
# it is difficult to understand "which direction ?" from code.
# if "direction = DMA_TO_DEVICE/DMA_FROM_DEVICE, it is understandable ;P
#

Can you please add below fix patch ?
------------------------------------------------
diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
index cb92a1d..c467067 100644
--- a/drivers/usb/renesas_usbhs/mod_gadget.c
+++ b/drivers/usb/renesas_usbhs/mod_gadget.c
@@ -173,15 +173,32 @@ static int usbhsg_dma_map_ctrl(struct usbhs_pkt *pkt, int map)
 	struct usbhsg_uep *uep = usbhsg_pipe_to_uep(pipe);
 	struct usbhsg_gpriv *gpriv = usbhsg_uep_to_gpriv(uep);
 	enum dma_data_direction dir;
+	int ret;
 
-	dir = usbhs_pipe_is_dir_in(pipe) ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
+	dir = !usbhs_pipe_is_dir_in(pipe);
 
 	if (map) {
-		return usb_gadget_map_request(&gpriv->gadget, req, dir);
+		if (req->num_sgs) /* it can not use scatter/gather */
+			return -EIO;
+
+		if (pkt->dma != DMA_ADDR_INVALID)
+			return -EIO;
+
+		ret = usb_gadget_map_request(&gpriv->gadget, req, dir);
+		if (ret < 0)
+			return ret;
+
+		pkt->dma = req->dma;
 	} else {
+		if (pkt->dma == DMA_ADDR_INVALID)
+			return -EIO;
+
 		usb_gadget_unmap_request(&gpriv->gadget, req, dir);
-		return 0;
+		ret = 0;
+		pkt->dma = DMA_ADDR_INVALID;
 	}
+
+	return ret;
 }
 
 /*
------------------------------------------------

Best regards
---
Kuninori Morimoto