Meaning of commented out dma-coherent in ZynqMP DWC3 DT node (Was: Re: [PATCH] arm64: zynqmp: Enable hs termination flag for USB dwc3 controller)

[Michal Simek]

Hi,

On 11/30/22 18:12, Ahmad Fatoum wrote:
> Hello,
> 
> On 30.11.22 18:08, Michal Simek wrote:
>> Hi,
>>
>> On 11/30/22 18:02, Ahmad Fatoum wrote:
>>> Hello!
>>>
>>> On 23.10.22 23:56, Michael Grzeschik wrote:
>>>> Since we need to support legacy phys with the dwc3 controller,
>>>> we enable this quirk on the zynqmp platforms.
>>>
>>> Slightly off-topic question below.
>>>
>>>>
>>>> Signed-off-by: Michael Grzeschik <m.grzeschik at pengutronix.de>
>>>> ---
>>>>    arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
>>>> index a549265e55f6e7..7c1af75f33a05b 100644
>>>> --- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
>>>> +++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
>>>> @@ -825,6 +825,7 @@ dwc3_0: usb at fe200000 {
>>>>                                 clock-names = "bus_early", "ref";
>>>>                                 iommus = <&smmu 0x860>;
>>>>                                 snps,quirk-frame-length-adjustment = <0x20>;
>>>> +                             snps,resume-hs-terminations;
>>>>                                 /* dma-coherent; */
>>>
>>> Is it possible to configure coherent DMA for the device and this is currently
>>> not done or how should this comment be interpreted?
>>
>>
>> I didn't try it for a while but before you start a53 you can enable CCI and then dma-coherent flags should be enabled.
> 
> I see. Wouldn't this apply to all DMA-capable devices then? So this comment would be better
> placed top-level in /soc?

yes of course.

> 
> Also, why did you decide against having cache coherency be the default?
> I'd expect this to be a performance improvement (if it functions correctly).

I haven't seen such a request to enable it by default but from build perspective 
you need to do one more additional step which none is really doing. Definitely 
there is missing code in bootloader which is also sharing the same DT description.
I know that we were trying that configuration in past but not sure if this is in 
regression or if there is any issue with any driver.

Thanks,
Michal