diff mbox series

[4/4] Revert "usb: gadget: f_uvc: change endpoint allocation in uvc_function_bind()"

Message ID 20231221165426.1590866-5-Frank.Li@nxp.com (mailing list archive)
State Superseded
Headers show
Series usb: cdns3: usb uvc iso transfer fix | expand

Commit Message

Frank Li Dec. 21, 2023, 4:54 p.m. UTC
This reverts commit 3c5b006f3ee800b4bd9ed37b3a8f271b8560126e.

gadget_is_{super|dual}speed() API check UDC controller capitblity. It
should pass down highest speed endpoint descriptor to UDC controller. So
UDC controller driver can reserve enough resource at check_config(),
especially mult and maxburst. So UDC driver (such as cdns3) can know need
at least (mult + 1) * (maxburst + 1) * wMaxPacketSize internal memory for
this uvc functions.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/usb/gadget/function/f_uvc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

yuan linyu Dec. 22, 2023, 12:26 p.m. UTC | #1
On 2023/12/22 00:54, Frank Li wrote:
> This reverts commit 3c5b006f3ee800b4bd9ed37b3a8f271b8560126e.
>
> gadget_is_{super|dual}speed() API check UDC controller capitblity. It
> should pass down highest speed endpoint descriptor to UDC controller. So
> UDC controller driver can reserve enough resource at check_config(),
> especially mult and maxburst. So UDC driver (such as cdns3) can know need
> at least (mult + 1) * (maxburst + 1) * wMaxPacketSize internal memory for
> this uvc functions.
>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/usb/gadget/function/f_uvc.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index faa398109431f..cc4e08c8169b4 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -719,13 +719,21 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
>  	}
>  	uvc->enable_interrupt_ep = opts->enable_interrupt_ep;
>  
> -	ep = usb_ep_autoconfig(cdev->gadget, &uvc_fs_streaming_ep);

how about all none-0 endpoint used for all uvc ?

please add some comment if currently this is only way to fix your issue.

need it for stable ?

> +	if (gadget_is_superspeed(c->cdev->gadget))
> +		ep = usb_ep_autoconfig_ss(cdev->gadget, &uvc_ss_streaming_ep,
> +					  &uvc_ss_streaming_comp);
> +	else if (gadget_is_dualspeed(cdev->gadget))
> +		ep = usb_ep_autoconfig(cdev->gadget, &uvc_hs_streaming_ep);
> +	else
> +		ep = usb_ep_autoconfig(cdev->gadget, &uvc_fs_streaming_ep);
> +
>  	if (!ep) {
>  		uvcg_info(f, "Unable to allocate streaming EP\n");
>  		goto error;
>  	}
>  	uvc->video.ep = ep;
>  
> +	uvc_fs_streaming_ep.bEndpointAddress = uvc->video.ep->address;
>  	uvc_hs_streaming_ep.bEndpointAddress = uvc->video.ep->address;
>  	uvc_ss_streaming_ep.bEndpointAddress = uvc->video.ep->address;
>
Frank Li Dec. 23, 2023, 3:42 a.m. UTC | #2
On Fri, Dec 22, 2023 at 08:26:13PM +0800, yuan linyu wrote:
> 
> On 2023/12/22 00:54, Frank Li wrote:
> > This reverts commit 3c5b006f3ee800b4bd9ed37b3a8f271b8560126e.
> >
> > gadget_is_{super|dual}speed() API check UDC controller capitblity. It
> > should pass down highest speed endpoint descriptor to UDC controller. So
> > UDC controller driver can reserve enough resource at check_config(),
> > especially mult and maxburst. So UDC driver (such as cdns3) can know need
> > at least (mult + 1) * (maxburst + 1) * wMaxPacketSize internal memory for
> > this uvc functions.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/usb/gadget/function/f_uvc.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> > index faa398109431f..cc4e08c8169b4 100644
> > --- a/drivers/usb/gadget/function/f_uvc.c
> > +++ b/drivers/usb/gadget/function/f_uvc.c
> > @@ -719,13 +719,21 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
> >  	}
> >  	uvc->enable_interrupt_ep = opts->enable_interrupt_ep;
> >  
> > -	ep = usb_ep_autoconfig(cdev->gadget, &uvc_fs_streaming_ep);
> 
> how about all none-0 endpoint used for all uvc ?

Unit of CDNS UDC is package size, how many package size associated to EP.
Interrupt EP only change package size according to speed.

May have issue with other controller. 

Idealy, usb function should pass down all speeds setting by a API, the
composite driver check UDC again for difference connect speeds.

But it is quite big change. 

> 
> please add some comment if currently this is only way to fix your issue.

I can add commens. 

> 
> need it for stable ?

Suppose yes.

> 
> > +	if (gadget_is_superspeed(c->cdev->gadget))
> > +		ep = usb_ep_autoconfig_ss(cdev->gadget, &uvc_ss_streaming_ep,
> > +					  &uvc_ss_streaming_comp);
> > +	else if (gadget_is_dualspeed(cdev->gadget))
> > +		ep = usb_ep_autoconfig(cdev->gadget, &uvc_hs_streaming_ep);
> > +	else
> > +		ep = usb_ep_autoconfig(cdev->gadget, &uvc_fs_streaming_ep);
> > +
> >  	if (!ep) {
> >  		uvcg_info(f, "Unable to allocate streaming EP\n");
> >  		goto error;
> >  	}
> >  	uvc->video.ep = ep;
> >  
> > +	uvc_fs_streaming_ep.bEndpointAddress = uvc->video.ep->address;
> >  	uvc_hs_streaming_ep.bEndpointAddress = uvc->video.ep->address;
> >  	uvc_ss_streaming_ep.bEndpointAddress = uvc->video.ep->address;
> >  
>
diff mbox series

Patch

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index faa398109431f..cc4e08c8169b4 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -719,13 +719,21 @@  uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
 	}
 	uvc->enable_interrupt_ep = opts->enable_interrupt_ep;
 
-	ep = usb_ep_autoconfig(cdev->gadget, &uvc_fs_streaming_ep);
+	if (gadget_is_superspeed(c->cdev->gadget))
+		ep = usb_ep_autoconfig_ss(cdev->gadget, &uvc_ss_streaming_ep,
+					  &uvc_ss_streaming_comp);
+	else if (gadget_is_dualspeed(cdev->gadget))
+		ep = usb_ep_autoconfig(cdev->gadget, &uvc_hs_streaming_ep);
+	else
+		ep = usb_ep_autoconfig(cdev->gadget, &uvc_fs_streaming_ep);
+
 	if (!ep) {
 		uvcg_info(f, "Unable to allocate streaming EP\n");
 		goto error;
 	}
 	uvc->video.ep = ep;
 
+	uvc_fs_streaming_ep.bEndpointAddress = uvc->video.ep->address;
 	uvc_hs_streaming_ep.bEndpointAddress = uvc->video.ep->address;
 	uvc_ss_streaming_ep.bEndpointAddress = uvc->video.ep->address;