[PATCH] usb: gadget: cdns3: Fix cdns3_ep_config() by setting ep.maxpacket

[PATCH] usb: gadget: cdns3: Fix cdns3_ep_config() by setting ep.maxpacket

[Siddharth Vadapalli]

The function cdns3_ep_config() calculates the maximum packet size based
on the Endpoint Type and the Gadget Speed and stores it in the variable
"max_packet_size". This value is then programmed in the USB Controller
for the corresponding Endpoint. This may result in a mismatch between
the maximum packet size programmed in the USB controller and the maximum
packet size seen by the UDC Core via "maxpacket" member of "struct usb_ep".
Additionally, since TD_SIZE is calculated in cdns3_ep_run_transfer() on the
basis of the maximum packet size stored in the "maxpacket" member of
"struct usb_ep", it may lead to an incorrect value of TD_SIZE when compared
with what the USB controller actually expects (max_packet_size).

Fix this by using usb_ep_set_maxpacket_limit().

Fixes: 7e91f6ccdc84 ("usb: Add Cadence USB3 host and gadget driver")
Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---

Hello,

This patch is based on commit
dd4d130c8e clk: renesas: rcar-gen3: Fix SSCG caching replacement with MDSEL/PE caching
of the master branch of U-Boot.

Patch has been tested on AM642-EVM.

Regards,
Siddharth.

 drivers/usb/cdns3/gadget.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
index ac7e469469..b0b08768d9 100644
--- a/drivers/usb/cdns3/gadget.c
+++ b/drivers/usb/cdns3/gadget.c
@@ -1637,6 +1637,14 @@ void cdns3_ep_config(struct cdns3_endpoint *priv_ep)
 	else
 		priv_ep->trb_burst_size = 16;
 
+	/*
+	 * The Endpoint is configured to handle a maximum packet size of
+	 * max_packet_size. Hence, set priv_ep->endpoint.maxpacket to
+	 * max_packet_size. This is necessary to ensure that the TD_SIZE
+	 * is calculated correctly in cdns3_ep_run_transfer().
+	 */
+	usb_ep_set_maxpacket_limit(&priv_ep->endpoint, max_packet_size);
+
 	ret = cdns3_ep_onchip_buffer_reserve(priv_dev, buffering + 1,
 					     !!priv_ep->dir);
 	if (ret) {
-- 
2.40.1

Re: [PATCH] usb: gadget: cdns3: Fix cdns3_ep_config() by setting ep.maxpacket

[Roger Quadros]

On 07/10/2024 13:42, Siddharth Vadapalli wrote:
> The function cdns3_ep_config() calculates the maximum packet size based
> on the Endpoint Type and the Gadget Speed and stores it in the variable
> "max_packet_size". This value is then programmed in the USB Controller
> for the corresponding Endpoint. This may result in a mismatch between
> the maximum packet size programmed in the USB controller and the maximum
> packet size seen by the UDC Core via "maxpacket" member of "struct usb_ep".
> Additionally, since TD_SIZE is calculated in cdns3_ep_run_transfer() on the
> basis of the maximum packet size stored in the "maxpacket" member of
> "struct usb_ep", it may lead to an incorrect value of TD_SIZE when compared
> with what the USB controller actually expects (max_packet_size).
> 
> Fix this by using usb_ep_set_maxpacket_limit().
> 
> Fixes: 7e91f6ccdc84 ("usb: Add Cadence USB3 host and gadget driver")
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
> 
> Hello,
> 
> This patch is based on commit
> dd4d130c8e clk: renesas: rcar-gen3: Fix SSCG caching replacement with MDSEL/PE caching
> of the master branch of U-Boot.
> 
> Patch has been tested on AM642-EVM.
> 
> Regards,
> Siddharth.
> 
>  drivers/usb/cdns3/gadget.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
> index ac7e469469..b0b08768d9 100644
> --- a/drivers/usb/cdns3/gadget.c
> +++ b/drivers/usb/cdns3/gadget.c
> @@ -1637,6 +1637,14 @@ void cdns3_ep_config(struct cdns3_endpoint *priv_ep)
>  	else
>  		priv_ep->trb_burst_size = 16;
>  
> +	/*
> +	 * The Endpoint is configured to handle a maximum packet size of
> +	 * max_packet_size. Hence, set priv_ep->endpoint.maxpacket to
> +	 * max_packet_size. This is necessary to ensure that the TD_SIZE
> +	 * is calculated correctly in cdns3_ep_run_transfer().
> +	 */
> +	usb_ep_set_maxpacket_limit(&priv_ep->endpoint, max_packet_size);
> +

from include/linux/usb/gadget.h

 * struct usb_ep - device side representation of USB endpoint
...
 * @maxpacket:The maximum packet size used on this endpoint.  The initial
 *      value can sometimes be reduced (hardware allowing), according to
 *      the endpoint descriptor used to configure the endpoint.
 * @maxpacket_limit:The maximum packet size value which can be handled by this
 *      endpoint. It's set once by UDC driver when endpoint is initialized, and
 *      should not be changed. Should not be confused with maxpacket.

What you want to change is ep->maxpacket and not ep->maxpacket_limit as that is
fixed for a particular UDC.

how about?
	priv-ep->endpoint.maxpacket = max_packet_size;


>  	ret = cdns3_ep_onchip_buffer_reserve(priv_dev, buffering + 1,
>  					     !!priv_ep->dir);
>  	if (ret) {

-- 
cheers,
-roger

Re: [PATCH] usb: gadget: cdns3: Fix cdns3_ep_config() by setting ep.maxpacket

[Siddharth Vadapalli]

On Mon, Oct 07, 2024 at 03:02:34PM +0300, Roger Quadros wrote:

Hello Roger,

> On 07/10/2024 13:42, Siddharth Vadapalli wrote:
> > The function cdns3_ep_config() calculates the maximum packet size based
> > on the Endpoint Type and the Gadget Speed and stores it in the variable
> > "max_packet_size". This value is then programmed in the USB Controller
> > for the corresponding Endpoint. This may result in a mismatch between
> > the maximum packet size programmed in the USB controller and the maximum
> > packet size seen by the UDC Core via "maxpacket" member of "struct usb_ep".
> > Additionally, since TD_SIZE is calculated in cdns3_ep_run_transfer() on the
> > basis of the maximum packet size stored in the "maxpacket" member of
> > "struct usb_ep", it may lead to an incorrect value of TD_SIZE when compared
> > with what the USB controller actually expects (max_packet_size).
> > 

[...]

> > diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
> > index ac7e469469..b0b08768d9 100644
> > --- a/drivers/usb/cdns3/gadget.c
> > +++ b/drivers/usb/cdns3/gadget.c
> > @@ -1637,6 +1637,14 @@ void cdns3_ep_config(struct cdns3_endpoint *priv_ep)
> >  	else
> >  		priv_ep->trb_burst_size = 16;
> >  
> > +	/*
> > +	 * The Endpoint is configured to handle a maximum packet size of
> > +	 * max_packet_size. Hence, set priv_ep->endpoint.maxpacket to
> > +	 * max_packet_size. This is necessary to ensure that the TD_SIZE
> > +	 * is calculated correctly in cdns3_ep_run_transfer().
> > +	 */
> > +	usb_ep_set_maxpacket_limit(&priv_ep->endpoint, max_packet_size);
> > +
> 
> from include/linux/usb/gadget.h
> 
>  * struct usb_ep - device side representation of USB endpoint
> ...
>  * @maxpacket:The maximum packet size used on this endpoint.  The initial
>  *      value can sometimes be reduced (hardware allowing), according to
>  *      the endpoint descriptor used to configure the endpoint.
>  * @maxpacket_limit:The maximum packet size value which can be handled by this
>  *      endpoint. It's set once by UDC driver when endpoint is initialized, and
>  *      should not be changed. Should not be confused with maxpacket.
> 
> What you want to change is ep->maxpacket and not ep->maxpacket_limit as that is
> fixed for a particular UDC.
> 
> how about?
> 	priv-ep->endpoint.maxpacket = max_packet_size;

Yes, this is accurate. I will post the v2 patch with this change. Thank
you for reviewing the patch.

Regards,
Siddharth.

Re: [PATCH] usb: gadget: cdns3: Fix cdns3_ep_config() by setting ep.maxpacket

[Siddharth Vadapalli]

On Mon, Oct 07, 2024 at 05:37:52PM +0530, Siddharth Vadapalli wrote:
> On Mon, Oct 07, 2024 at 03:02:34PM +0300, Roger Quadros wrote:
> 
> Hello Roger,

[...]

> > from include/linux/usb/gadget.h
> > 
> >  * struct usb_ep - device side representation of USB endpoint
> > ...
> >  * @maxpacket:The maximum packet size used on this endpoint.  The initial
> >  *      value can sometimes be reduced (hardware allowing), according to
> >  *      the endpoint descriptor used to configure the endpoint.
> >  * @maxpacket_limit:The maximum packet size value which can be handled by this
> >  *      endpoint. It's set once by UDC driver when endpoint is initialized, and
> >  *      should not be changed. Should not be confused with maxpacket.
> > 
> > What you want to change is ep->maxpacket and not ep->maxpacket_limit as that is
> > fixed for a particular UDC.
> > 
> > how about?
> > 	priv-ep->endpoint.maxpacket = max_packet_size;
> 
> Yes, this is accurate. I will post the v2 patch with this change. Thank
> you for reviewing the patch.

I have implemented your feedback and have posted the v2 patch at:
https://patchwork.ozlabs.org/project/uboot/patch/20241007121927.1680039-1-s-vadapalli@ti.com

Regards,
Siddharth.