RE: [PATCH v6 5/6] usb:cdns3 Add Cadence USB3 DRD Driver

[Pawel Laszczak]

>>> On 10/04/19 1:18 PM, Pawel Laszczak wrote:
>>>> +static void cdns3_wa1_tray_restore_cycle_bit(struct cdns3_device *priv_dev,
>>>> +					     struct cdns3_endpoint *priv_ep)
>>>> +{
>>>> +	int dma_index;
>>>> +	u32 doorbell;
>>>> +
>>>> +	doorbell = !!(readl(&priv_dev->regs->ep_cmd) & EP_CMD_DRDY);
>>>
>>>> +	dma_index = (readl(&priv_dev->regs->ep_traddr) -
>>>> +		    priv_ep->trb_pool_dma) / TRB_SIZE;
>>>
>>> This gets upgraded to 64-bit by 64-bit division whenever dma_addr_t is
>>> 64-bit. That should be avoided. Following diff should fix it.
>>> But please review the logic itself. You are subtracting a 64 bit entity
>>>from a 32-bit entity. What is guaranteeing that priv_ep->trb_pool_dma is
>>> 32-bit?
>>>
>>> There is one more instance of same issue in cdns3_request_handled().
>>>
>>> Thanks,
>>> Sekhar
>>>
>>> [1]
>>> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
>>> index bfd5dbf40c7e..e73b618501fb 100644
>>> --- a/drivers/usb/cdns3/gadget.c
>>> +++ b/drivers/usb/cdns3/gadget.c
>>> @@ -749,8 +749,8 @@ static void cdns3_wa1_tray_restore_cycle_bit(struct cdns3_device *priv_dev,
>>> 	u32 doorbell;
>>>
>>> 	doorbell = !!(readl(&priv_dev->regs->ep_cmd) & EP_CMD_DRDY);
>>> -	dma_index = (readl(&priv_dev->regs->ep_traddr) -
>>> -		    priv_ep->trb_pool_dma) / TRB_SIZE;
>>> +	dma_index = readl(&priv_dev->regs->ep_traddr) - priv_ep->trb_pool_dma;
>>> +	dma_index /= TRB_SIZE;
>>
>> Hi Sekhar,
>>
>> In the next latest version I added setting dma and coherent mask to 32-bits as suggested by Roger.
>> Controller can do only 32-bit access.
>
>I think this should work for now except if in some future version of
>controller the limitation of 32-bit access is fixed. I guess that might
>mean different logic for DMA handling anyway.

I now about new register that allows to set the segment address and extend the address to 48 bit, but 
it rather will not be used on Linux. By means of this register we can set the same segment memory for 
all area of  DMA memory. 
I'm not sure if Linux allow to configure dma and coherent mask in this way. 

But ok, I will add this fix.

Pawel,

>
>> DMA address space should be allocated from first 32 bits of address space. Most significant 32-bit
>> of priv_ep->trb_pool_dma should be zeroed, so I do not assume any issue in this place.
>>
>> Have you seen any issue with this on your platform ?
>
>build fails with
>
>ERROR: "__aeabi_uldivmod" [drivers/usb/cdns3/cdns3.ko] undefined!
>
>on 32-bit platforms with ARM LPAE enabled. So please roll in the fix I
>suggested.
>
>Thanks,
>Sekhar