Re: [PATCH] USB: gadget: s3c-hsotg: add isochronous transfers support

[Bartlomiej Zolnierkiewicz]

Hi Robert,

On Monday, September 23, 2013 10:07:12 AM Robert Baldyga wrote:
> This patch adds isochronous transfer support. It adds few modifications:
> - Modify s3c_hsotg_write_fifo() function. It actually calculates transfer
>   size, taking into account Multi Count value, which indicates number of
>   transactions per microframe.
> - Fix s3c_hsotg_start_req() function by setting number of packets to Multi
>   Count field in DIEPTSIZ register for isochronous endpoints.
> - Fix s3c_hsotg_set_ep_maxpacket() function. Field wMaxPacketSize of endpoint
>   descriptor is now splitted into maximum packet size value and number of
>   additional transaction per microframe.
> - Modify s3c_hsotg_epint() function. Some interrupts are ignored for
>   isochronous endpoints, (e.g. INTknTXFEmpMsk) becouse isochronous request is
>   always transfered in single transaction, which ends with XferCompl interrupt.
>   Add Odd/Even microframe toggle to allow data transfering in each microframe.
> - Fix s3c_hsotg_ep_enable() function by supporting isochronous endpoint type.
> 
> Signed-off-by: Robert Baldyga <r.baldyga@xxxxxxxxxxx>
> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> ---
>  drivers/usb/gadget/s3c-hsotg.c |   74 +++++++++++++++++++++++++++++++---------
>  1 file changed, 57 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/usb/gadget/s3c-hsotg.c b/drivers/usb/gadget/s3c-hsotg.c
> index d5d951d..d737452 100644
> --- a/drivers/usb/gadget/s3c-hsotg.c
> +++ b/drivers/usb/gadget/s3c-hsotg.c
> @@ -82,9 +82,12 @@ struct s3c_hsotg_req;
>   * @dir_in: Set to true if this endpoint is of the IN direction, which
>   *	    means that it is sending data to the Host.
>   * @index: The index for the endpoint registers.
> + * @mc: Multi Count - number of transactions per microframe
> + * @interval - Interval for periodic endpoints
>   * @name: The name array passed to the USB core.
>   * @halted: Set if the endpoint has been halted.
>   * @periodic: Set if this is a periodic ep, such as Interrupt
> + * @insochronous: Set if this is a isochronous ep

s/insochronous/isochronous/

>   * @sent_zlp: Set if we've sent a zero-length packet.
>   * @total_data: The total number of data bytes done.
>   * @fifo_size: The size of the FIFO (for periodic IN endpoints)
> @@ -120,9 +123,12 @@ struct s3c_hsotg_ep {
>  
>  	unsigned char		dir_in;
>  	unsigned char		index;
> +	unsigned char		mc;
> +	unsigned char		interval;
>  
>  	unsigned int		halted:1;
>  	unsigned int		periodic:1;
> +	unsigned int		isochronous:1;
>  	unsigned int		sent_zlp:1;
>  
>  	char			name[10];
> @@ -467,6 +473,7 @@ static int s3c_hsotg_write_fifo(struct s3c_hsotg *hsotg,
>  	void *data;
>  	int can_write;
>  	int pkt_round;
> +	int max_transfer;
>  
>  	to_write -= (buf_pos - hs_ep->last_load);
>  
> @@ -534,15 +541,17 @@ static int s3c_hsotg_write_fifo(struct s3c_hsotg *hsotg,
>  		can_write *= 4;	/* fifo size is in 32bit quantities. */
>  	}
>  
> -	dev_dbg(hsotg->dev, "%s: GNPTXSTS=%08x, can=%d, to=%d, mps %d\n",
> -		 __func__, gnptxsts, can_write, to_write, hs_ep->ep.maxpacket);
> +	max_transfer = hs_ep->ep.maxpacket * hs_ep->mc;
> +
> +	dev_dbg(hsotg->dev, "%s: GNPTXSTS=%08x, can=%d, to=%d, max_transfer %d\n",
> +		 __func__, gnptxsts, can_write, to_write, max_transfer);
>  
>  	/*
>  	 * limit to 512 bytes of data, it seems at least on the non-periodic
>  	 * FIFO, requests of >512 cause the endpoint to get stuck with a
>  	 * fragment of the end of the transfer in it.
>  	 */
> -	if (can_write > 512)
> +	if (can_write > 512 && !periodic)
>  		can_write = 512;

Doesn't it also affect non-isochronous transfers?

If so this change should be in a separate preparatory patch.

>  	/*
> @@ -550,8 +559,8 @@ static int s3c_hsotg_write_fifo(struct s3c_hsotg *hsotg,
>  	 * the transfer to return that it did not run out of fifo space
>  	 * doing it.
>  	 */
> -	if (to_write > hs_ep->ep.maxpacket) {
> -		to_write = hs_ep->ep.maxpacket;
> +	if (to_write > max_transfer) {
> +		to_write = max_transfer;
>  
>  		/* it's needed only when we do not use dedicated fifos */
>  		if (!hsotg->dedicated_fifos)
> @@ -564,7 +573,7 @@ static int s3c_hsotg_write_fifo(struct s3c_hsotg *hsotg,
>  
>  	if (to_write > can_write) {
>  		to_write = can_write;
> -		pkt_round = to_write % hs_ep->ep.maxpacket;
> +		pkt_round = to_write % max_transfer;
>  
>  		/*
>  		 * Round the write down to an
> @@ -730,8 +739,16 @@ static void s3c_hsotg_start_req(struct s3c_hsotg *hsotg,
>  	else
>  		packets = 1;	/* send one packet if length is zero. */
>  
> +	if (length > (hs_ep->mc * hs_ep->ep.maxpacket) && hs_ep->isochronous) {

Wouldn't checking hs_ep->isonchronous first be better?

> +		dev_err(hsotg->dev, "req length > maxpacket*mc\n");
> +		return;
> +	}

The rest of the patch looks good to me.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/