usb: cdns3: Onchip memory reservation for built-in gadgets

usb: cdns3: Onchip memory reservation for built-in gadgets

[Ravi Gunasekaran]

Hi,

Firstly, I'm not sure if it is alright to post queries this way. 
If it is wrong, I apologize for it. Please let me know the right path/forum to ask the questions.

This is regarding the commit
dce49449e04f usb: cdns3: allocate TX FIFO size according to composite EP number

This commit introduced cdns3_gadget_check_config() which is invoked while binding gadget created via configfs and
also a logic to calculate ep_buf_size (which was CDNS3_EP_BUF_SIZE = 4).

But for gadgets such as g_ether, g_cdc, the checks are not performed. And also for these legacy gadget drivers,
memory needs to be reserved for multiple IN end points and shared memory for OUT end points. So when ep_buf_size = 15,
the memory reservation fails, as it exceeds total onchip memory.

So I was wondering if additional checks need to done in the cadence gadget driver or am I doing something wrong while
loading gadgets such as g_ether. 

-- 
Regards,
Ravi

Re: usb: cdns3: Onchip memory reservation for built-in gadgets

[Roger Quadros]

Hi,

On 27/06/2023 15:56, Ravi Gunasekaran wrote:
> Hi,
> 
> Firstly, I'm not sure if it is alright to post queries this way. 
> If it is wrong, I apologize for it. Please let me know the right path/forum to ask the questions.
> 
> This is regarding the commit
> dce49449e04f usb: cdns3: allocate TX FIFO size according to composite EP number
> 
> This commit introduced cdns3_gadget_check_config() which is invoked while binding gadget created via configfs and
> also a logic to calculate ep_buf_size (which was CDNS3_EP_BUF_SIZE = 4).
> 
> But for gadgets such as g_ether, g_cdc, the checks are not performed. And also for these legacy gadget drivers,
> memory needs to be reserved for multiple IN end points and shared memory for OUT end points. So when ep_buf_size = 15,
> the memory reservation fails, as it exceeds total onchip memory.
> 
> So I was wondering if additional checks need to done in the cadence gadget driver or am I doing something wrong while
> loading gadgets such as g_ether. 
> 

I think Ravi's concern is that.

prior to commit dce49449e04f, priv_dev->ep_buf_size was fixed at 4.
After commit dce49449e04f, it is at 0 if check_config() is not called e.g.
in the legacy gadget cases (e.g. g_ether).
So cdns3_ep_config() fails with dev_err(priv_dev->dev, "onchip mem is full, ep is invalid\n").

A potential fix might be to start with priv_dev->ep_buf_size = 4
instead of 0 so it works for legacy gadgets as well where check_config() is not invoked.

-- 
cheers,
-roger

RE: [EXT] Re: usb: cdns3: Onchip memory reservation for built-in gadgets

[Frank Li]

> -----Original Message-----
> From: Roger Quadros <rogerq@kernel.org>
> Sent: Tuesday, June 27, 2023 5:41 AM
> To: Ravi Gunasekaran <r-gunasekaran@ti.com>; Frank Li <frank.li@nxp.com>
> Cc: linux-usb@vger.kernel.org; peter.chen@kernel.org;
> pawell@cadence.com
> Subject: [EXT] Re: usb: cdns3: Onchip memory reservation for built-in
> gadgets
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
> 
> 
> Hi,
> 
> On 27/06/2023 15:56, Ravi Gunasekaran wrote:
> > Hi,
> >
> > Firstly, I'm not sure if it is alright to post queries this way.
> > If it is wrong, I apologize for it. Please let me know the right path/forum to
> ask the questions.
> >
> > This is regarding the commit
> > dce49449e04f usb: cdns3: allocate TX FIFO size according to composite EP
> number
> >
> > This commit introduced cdns3_gadget_check_config() which is invoked
> while binding gadget created via configfs and
> > also a logic to calculate ep_buf_size (which was CDNS3_EP_BUF_SIZE = 4).
> >
> > But for gadgets such as g_ether, g_cdc, the checks are not performed. And
> also for these legacy gadget drivers,
> > memory needs to be reserved for multiple IN end points and shared
> memory for OUT end points. So when ep_buf_size = 15,
> > the memory reservation fails, as it exceeds total onchip memory.
> >
> > So I was wondering if additional checks need to done in the cadence gadget
> driver or am I doing something wrong while
> > loading gadgets such as g_ether.
> >
> 
> I think Ravi's concern is that.
> 
> prior to commit dce49449e04f, priv_dev->ep_buf_size was fixed at 4.
> After commit dce49449e04f, it is at 0 if check_config() is not called e.g.
> in the legacy gadget cases (e.g. g_ether).
> So cdns3_ep_config() fails with dev_err(priv_dev->dev, "onchip mem is full,
> ep is invalid\n").
> 
> A potential fix might be to start with priv_dev->ep_buf_size = 4
> instead of 0 so it works for legacy gadgets as well where check_config() is not
> invoked.

I think it should fix at legacy driver by call usb_gadget_check_config. 
Assume there are a UDC controller, which only 2 endpoint.  g_cdc should
get failure.

Best regards
Frank Li

> 
> --
> cheers,
> -roger

Re: [EXT] Re: usb: cdns3: Onchip memory reservation for built-in gadgets

[Roger Quadros]

On 27/06/2023 20:26, Frank Li wrote:
> 
> 
>> -----Original Message-----
>> From: Roger Quadros <rogerq@kernel.org>
>> Sent: Tuesday, June 27, 2023 5:41 AM
>> To: Ravi Gunasekaran <r-gunasekaran@ti.com>; Frank Li <frank.li@nxp.com>
>> Cc: linux-usb@vger.kernel.org; peter.chen@kernel.org;
>> pawell@cadence.com
>> Subject: [EXT] Re: usb: cdns3: Onchip memory reservation for built-in
>> gadgets
>>
>> Caution: This is an external email. Please take care when clicking links or
>> opening attachments. When in doubt, report the message using the 'Report
>> this email' button
>>
>>
>> Hi,
>>
>> On 27/06/2023 15:56, Ravi Gunasekaran wrote:
>>> Hi,
>>>
>>> Firstly, I'm not sure if it is alright to post queries this way.
>>> If it is wrong, I apologize for it. Please let me know the right path/forum to
>> ask the questions.
>>>
>>> This is regarding the commit
>>> dce49449e04f usb: cdns3: allocate TX FIFO size according to composite EP
>> number
>>>
>>> This commit introduced cdns3_gadget_check_config() which is invoked
>> while binding gadget created via configfs and
>>> also a logic to calculate ep_buf_size (which was CDNS3_EP_BUF_SIZE = 4).
>>>
>>> But for gadgets such as g_ether, g_cdc, the checks are not performed. And
>> also for these legacy gadget drivers,
>>> memory needs to be reserved for multiple IN end points and shared
>> memory for OUT end points. So when ep_buf_size = 15,
>>> the memory reservation fails, as it exceeds total onchip memory.
>>>
>>> So I was wondering if additional checks need to done in the cadence gadget
>> driver or am I doing something wrong while
>>> loading gadgets such as g_ether.
>>>
>>
>> I think Ravi's concern is that.
>>
>> prior to commit dce49449e04f, priv_dev->ep_buf_size was fixed at 4.
>> After commit dce49449e04f, it is at 0 if check_config() is not called e.g.
>> in the legacy gadget cases (e.g. g_ether).
>> So cdns3_ep_config() fails with dev_err(priv_dev->dev, "onchip mem is full,
>> ep is invalid\n").
>>
>> A potential fix might be to start with priv_dev->ep_buf_size = 4
>> instead of 0 so it works for legacy gadgets as well where check_config() is not
>> invoked.
> 
> I think it should fix at legacy driver by call usb_gadget_check_config. 
> Assume there are a UDC controller, which only 2 endpoint.  g_cdc should
> get failure.
> 

commit dce49449e04f has broken legacy gadget drivers for CDNS3 UDC
and that should be fixed first.

No other UDC drivers implement .check_config() yet.

UDC driver should start with a default sane configuration and work even if
usb_gadget_check_config() is not called by gadget driver.

-- 
cheers,
-roger

RE: [EXT] Re: usb: cdns3: Onchip memory reservation for built-in gadgets

[Frank Li]

> >
> > I think it should fix at legacy driver by call usb_gadget_check_config.
> > Assume there are a UDC controller, which only 2 endpoint.  g_cdc should
> > get failure.
> >
> 
> commit dce49449e04f has broken legacy gadget drivers for CDNS3 UDC
> and that should be fixed first.
> 
> No other UDC drivers implement .check_config() yet.

No implement is not explicit correct.  CDNS3 just trigger this problem. 
 
> 
> UDC driver should start with a default sane configuration and work even if
> usb_gadget_check_config() is not called by gadget driver. 

I am preparing a patches add usb_gadget_check_config() in legacy gadget driver.


> 
> --
> cheers,
> -roger