Re: [PATCH 3/3] usb: cdns3: Enable workaround for USB2.0 PHY Rx compliance test PHY lockup

[Roger Quadros <rogerq@ti.com>]

On 27/08/2020 03:24, Peter Chen wrote:
> On 20-08-26 15:49:57, Roger Quadros wrote:
>> Peter,
>>
>> On 26/08/2020 11:07, Peter Chen wrote:
>>>>
>>>>
>>>>>
>>>>> On 20-08-26 04:04:01, Pawel Laszczak wrote:
>>>>>>> On 20-08-25 15:00:59, Roger Quadros wrote:
>>>>>>>> From: Pawel Laszczak <pawell@cadence.com>
>>>>>>>>
>>>>>>>> USB2.0 PHY hangs in Rx Compliance test when the incoming packet
>>>>>>>> amplitude is varied below and above the Squelch Level of Receiver
>>>>>>>> during the active packet multiple times.
>>>>>>>>
>>>>>>>> Version 1 of the controller allows PHY to be reset when RX fail
>>>>>>>> condition is detected to work around the above issue. This feature
>>>>>>>> is disabled by default and needs to be enabled using a bit from
>>>>>>>> the newly added PHYRST_CFG register. This patch enables the workaround.
>>>>>>>>
>>>>>>>> As there is no way to distinguish between the controller version
>>>>>>>> before the device controller is started we need to rely on a DT
>>>>>>>> property to decide when to apply the workaround.
>>>>>>>
>>>>>>> Pawel, it could know the controller version at cdns3_gadget_start,
>>>>>>> but the controller starts when it tries to bind gadget driver, at
>>>>>>> that time, it has already known the controller version.
>>>>>>>
>>>>>>> For me, the device controller starts is using USB_CONF.DEVEN (Device
>>>>>>> Enable) through usb_gadget_connect, I am not sure if it is the same
>>>>>>> with yours.
>>>>>>>
>>>>>>
>>>>>> Yes in device mode driver knows controller version but this
>>>>>> workaround Must be enabled also in host mode. In host mode the
>>>>>> controller doesn't have access to device registers. The controller
>>>>>> version is placed in device register.
>>>>>>
>>>>>
>>>>> You may suggest your design team adding CHIP_VER register at global
>>>>> register region, it will easy the software engineer life.
>>>>>
>>>> >From what I read, this register is only enabling USB2 PHY reset
>>>>> software control, it needs for all chips with rev 0x0002450D, and the
>>>>> place you current change is only for 0x0002450D, right?
>>>>
>>>> Even I could say that this workaround should be enabled only for Specific USB2
>>>> PHY  (only 0x0002450D)
>>>>
>>>> This bit should not have any impact for Cadence PHY but it can has Impact for third
>>>> party PHYs.
>>>>
>>>
>>> So, it is related to specific PHY, but enable this specific PHY reset bit is at controller region, why don't
>>> put this enable bit at PHY region?
>>
>> I think this is related to Controller + PHY combination.
>> The fix for the issue is via a bit in the controller, so it needs to be managed by the
>> controller driver.
>>
>>>
>>> So, you use controller's device property to know this specific PHY, can controller know this specific
>>> PHY dynamically?
>>
>> Still the PHY will have to tell the controller the enable that bit. How to do that?
>>
>> Adding a dt-property that vendors can used was the simplest option.
>>
> 
> Ok, does all controllers with ver 0x0002450D need this fix? I just think
> if we introduce a flag stands for ver 0x0002450D in case this ver has
> other issues in future or just using phy reset enable property?
> 
> Pawel & Roger, what's your opinion?
> 
I think it is best to keep the flags specific to the issue rather than a one flag for
all issues with a specific version. This way you can re-use the flag irrespective
of IP version.

But best case is that Cadence put a IP revision register in common area as you
have previously suggested so driver can automatically apply quirks to specific
versions.

cheers,
-roger
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki