[U-Boot] [PATCH] dwc: ep0: Allocate and flush dwc->ep0_trb in a cache aligned manner

[Faiz Abbas]

Hi,

On Tuesday 10 October 2017 07:19 PM, Marek Vasut wrote:
> On 10/10/2017 12:45 PM, Faiz Abbas wrote:
>> Hi Marek,
>>
>> On Tuesday 10 October 2017 01:30 PM, Marek Vasut wrote:
>>> On 10/10/2017 07:48 AM, Kishon Vijay Abraham I wrote:
>>>> Hi,
>>>
>>> Hi,
>>>
>>> [...]
>>>
>>>>>>>>>>>>>> -	dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
>>>>>>>>>>>>>> +	dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);
>>>>>>>>>>>>>
>>>>>>>>>>>>> Why *2 ?
>>>>>>>>>>>>
>>>>>>>>>>>> Because its allocated as sizeof(*dwc->ep0_trb) * 2 below. This is not
>>>>>>>>>>>> strictly required as dwc3_flush_cache() rounds up the size to
>>>>>>>>>>>> CACHELINE_SIZE but from a caller POV, flush everything we allocated.
>>>>>>>>>>>
>>>>>>>>>>> Can the other TRB be in use ? Maybe aligning the TRBs to cacheline size
>>>>>>>>>>> would be better ?
>>>>>>>>>>>
>>>>>>>>>> A single trb is 16 bytes in size and two of them are allocated
>>>>>>>>>> contiguously.
>>>>>>>>>
>>>>>>>>> Why are two allocated continuously ? (I am not dwc3 expert)
>>>>
>>>> The TRB's should be allocated contiguously for dwc3 and only the base of the
>>>> entire TRB table is programmed in the HW.
>>>>  ________________ <------------------ TRB table base address
>>>> |     TRB0       |
>>>> |________________|
>>>> |     TRB1       |
>>>> |________________|
>>>> |     TRB2       |
>>>> |________________|
>>>> |     TRBn       |
>>>> |________________|
>>>>
>>>>
>>>>>>>>
>>>>>>>> Neither am I. I did try to pad to the dwc_trb structure such that each
>>>>>>>> trb is 64 bytes in size but this leads to failures when testing. I
>>>>>>>> didn't get a chance to debug this though. I suspect its because the code
>>>>>>>> expects the trbs to be contiguous and/or 16 bytes in size.
>>>>
>>>> It's not the code but it's the HW.
>>>
>>> That'd imply we need either some sort of flushing scheme or non-cachable
>>> memory allocation. What does Linux do ?
>>> The dma_alloc_coherent in linux kernel allocates non-cachable memory.
>>
>> Currently, the code is using local variable trb to flush the cache. When
>> the first trb is used, dwc3_flush_cache flushes the complete
>> CACHELINE_SIZE (including the 2nd trb).
>> When the 2nd trb is used to flush cache, since it is an unaligned flush,
>> it will issue a warning and will align it to the lower cache line
>> boundary (flushing the 1st trb in the process).
>>
>> So with or without this patch, both trbs are getting flushed with every
>> call. With the patch, we can just avoid misaligned messages by always
>> flushing using an aligned address.
> 
> What worries me is that you can flush something into the memory while
> the controller is writing into it as well. Is that possible ?
> 
No, control to the hardware is only given after all the trbs have been
configured and flushed to memory. This is done by using the chain
variable in the code.

        dwc3_flush_cache((uintptr_t)buf_dma, len);
        dwc3_flush_cache((uintptr_t)dwc->ep0_trb_addr, sizeof(*trb) * 2);

        if (chain)
                return 0;

        memset(&params, 0, sizeof(params));
        params.param0 = upper_32_bits(dwc->ep0_trb_addr);
        params.param1 = lower_32_bits(dwc->ep0_trb_addr);

        ret = dwc3_send_gadget_ep_cmd(dwc, dep->number,
                        DWC3_DEPCMD_STARTTRANSFER, &params);



Thanks,
Faiz