Re: [PATCH v5 3/5] usb: dwc3: add phyif_utmi_quirk[Involving remittance information, please pay attention to the safety of property]

[William Wu <william.wu@rock-chips.com>]

[-- Attachment #1: Type: text/plain, Size: 3989 bytes --]

Dear Rob,

On 07/01/2016 10:35 AM, Rob Herring wrote:
> On Thu, Jun 30, 2016 at 07:12:54PM +0800, William Wu wrote:
>> Add a quirk to configure the core to support the
>> UTMI+ PHY with an 8- or 16-bit interface. UTMI+ PHY
>> interface is hardware property, and it's platform
>> dependent. Normall, the PHYIf can be configured
>> during coreconsultant. But for some specific usb
>> cores(e.g. rk3399 soc dwc3), the default PHYIf
>> configuration value is fault, so we need to
>> reconfigure it by software.
>>
>> And refer to the dwc3 databook, the GUSB2PHYCFG.USBTRDTIM
>> must be set to the corresponding value according to
>> the UTMI+ PHY interface.
>>
>> Signed-off-by: William Wu <william.wu@rock-chips.com>
>> ---
>> Changes in v5:
>> - None
>>
>> Changes in v4:
>> - rebase on top of balbi testing/next, remove pdata (balbi)
>>
>> Changes in v3:
>> - None
>>
>> Changes in v2:
>> - add a quirk for phyif_utmi (balbi)
>>
>>   Documentation/devicetree/bindings/usb/dwc3.txt |  4 ++++
>>   drivers/usb/dwc3/core.c                        | 19 +++++++++++++++++++
>>   drivers/usb/dwc3/core.h                        | 12 ++++++++++++
>>   3 files changed, 35 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>> index 1ada121..34d13a5 100644
>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>> @@ -42,6 +42,10 @@ Optional properties:
>>    - snps,dis_u2_freeclk_exists_quirk: when set, clear the u2_freeclk_exists
>>   			in GUSB2PHYCFG, specify that USB2 PHY doesn't provide
>>   			a free-running PHY clock.
>> + - snps,phyif_utmi_quirk: when set core will set phyif UTMI+ interface.
> This isn't really what I'd call a quirk.
I'm sorry that add you in the review list too late.

Actually, I have discussed the property "snps,phyif_utmi_quirk"
with balbi in patch v2.

As we know, UTMI+ PHY interface is hardware property, and
it can be configured correctly during coreconsulttant according
to dwc3 TRM, section 8.1.1 Table 8-1 where itstates:

|-------------+-------------------------------------------------------------------------------------|
| GUSB2PHYCFG | Program the following PHY configuration fields: USBTrdTim, |
|             | FSIntf, PHYIf, TOUTCal, or leave the default values if  
                        |
|             | the correct power-on values were selected 
during                             |
|             | coreConsultant configuration.  Note: The PHY must 
not                     |
|             | be enabled for auto-resume in device mode. Hence 
the                    |
|             | field GUSB2PHYCFG[15] (ULPIAutoRes) must be 
written                    |
|             | with '0' during the power-on initialization in case     
                          |
|             | the reset value is '1'. |
| | |
|-------------+------------------------------------------------------------------------------------|

But for some specific usb cores(e.g. rk3399 soc dwc3),  the default
PHYIf configuration value is fault after core init, ad we need to
reconfigure it by software. so I think maybe a quirk is more proper.

>
>> + - snps,phyif_utmi: the value to configure the core to support a UTMI+ PHY
>> +			with an 8- or 16-bit interface. Value 0 select 8-bit
>> +			interface, value 1 select 16-bit interface.
> These seem like they should be standard properties for setting the phy
> type/mode. I think we already have something defined in fact.
Yes, it's standard properties for setting the phy interface.
But as I describe above, for most of dwc3 cores designed
by IC vendors, the phy interface can be automatically set
with correct value during coreConsultant configuration,
and don't need to reconfigure it by software. For now,
I haven't found any SoC had this issue except rk3399,
and dwc3 driver also doesn't have any defined similar to
what I defined here.

Best Regards
      William Wu
>
> Rob
>
>
>


[-- Attachment #2: Type: text/html, Size: 7329 bytes --]