[PATCH v6 3/5] usb: dwc3: add phyif_utmi_quirk

[William.wu]

Dear Heiko,


On 2016/7/10 7:47, Heiko Stuebner wrote:
> Am Samstag, 9. Juli 2016, 11:38:00 schrieb William.wu:
>> Dear Heiko & Balbi，
>>
>> On 2016/7/8 21:29, Felipe Balbi wrote:
>>> Hi,
>>>
>>> Heiko Stuebner <heiko at sntech.de> writes:
>>>> Am Donnerstag, 7. Juli 2016, 10:54:24 schrieb William Wu:
>>>>> 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 at rock-chips.com>
>>>>> ---
>>>> [...]
>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> b/Documentation/devicetree/bindings/usb/dwc3.txt index
>>>>> 020b0e9..8d7317d
>>>>> 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.
>>>>> + - 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.
>>>> maybe
>>>>
>>>> 	snps,phyif-utmi-width = <8> or <16>;
>>>>
>>>> devicetree is about describing the hardware, not the things that get
>>>> written to registers :-) . The conversion from the described width to
>>>> the register value can easily be done in the driver.
>> Thanks for your suggestion:-)
>> Yes， “snps,phyif-utmi-width = <8> or <16>” is much clearer and easier to
>> understand.
>> And I have considered the same dts property for phyif-utmi, but I have
>> no good idea about
>> the conversion from described width to the registers value for the time
>> being.
>>
>> About phyif utmi width configuration， we need to set two places in
>> GUSB2PHYCFG register，
>> according to DWC3 USB3.0 controller databook version3.00a，6.3.46
>> GUSB2PHYCFG
>>
>> --------------------------------------------------------------------------
>> -------------------- Bits   |  Name                 |     Description
>> --------------------------------------------------------------------------
>> -------------------- 13:10  |   USBTRDTIM       |     Sets the turnaround
>> time in PHY clocks.
>>               |                            |     4'h5: When the MAC
>>
>> interface is 16-bit UTMI+
>>
>>               |                            |     4'h9: When the MAC
>>
>> interface is 8-bit UTMI+/ULPI.
>> --------------------------------------------------------------------------
>> -------------------- 3        |   PHYIF                |    If UTMI+ is
>> selected, the application uses this bit to configure
>>
>>               |                            |    core to support a UTMI+
>>
>> PHY with an 8- or 16-bit interface.
>>
>>               |                            |    1'b0: 8 bits
>>               |                            |    1'b1: 16 bits
>>
>> --------------------------------------------------------------------------
>> --------------------
>>
>> And I think maybe I can try to do this:
>> change it in dts:
>>           snps,phyif-utmi-width = <8> or <16>;
>>
>> Then convert to register value like this：
>>          device_property_read_u8(dev, "snps,phyif-utmi-width",
>>                                                &phyif_utmi_width);
>>
>>          dwc->phyif_utmi = phyif_utmi_width >> 4;
>>
>>    Ater the conversion， dwc->phyif_utmi value 0 means 8 bits, value 1
>> means 16 bits,
>>    and it's easier for us to config GUSB2PHYCFG.
>>
>> Is it OK?
> or you could just store the actual width value read from the dts and make
> the core handle accordingly, making everything a bit more explicit.
>
> I guess personally I'd do something like:
>
> make dwc->phyif_utmi a regular unsigned int
>
> in probe:
>        	ret = device_property_read_u8(dev, "snps,phyif-utmi-width",
>                                                &dwc->phyif_utmi);
> 	if (ret < 0) {
> 		dwc->phyif_utmi = 0;
> 	else if (dwc->phyif_utmi != 16 && dwc->phyif_utmi != 8) {
> 		dev_err(dev, "unsupported utmi interface width %d\n",
> 			dwc->phyif_utmi);
> 		return -EINVAL;
> 	}
>
>
> when setting your GUSB2PHYCFG register:
>
>         if (dwc->phyif_utmi > 0) {
>                 reg &= ~(DWC3_GUSB2PHYCFG_PHYIF_MASK |
>                        DWC3_GUSB2PHYCFG_USBTRDTIM_MASK);
>                 usbtrdtim = (dwc->phyif_utmi == 16) ? USBTRDTIM_UTMI_16_BIT :
>                             USBTRDTIM_UTMI_8_BIT;
> 		phyif = (dwc->phyif_utmi == 16) ? 1 : 0;
>                 reg |= DWC3_GUSB2PHYCFG_PHYIF(phyif) |
>                        DWC3_GUSB2PHYCFG_USBTRDTIM(usbtrdtim) |
>         }
>
Ah yes， it seems very good to me, very explicit.
I'll upload a new patch according to your suggestion today.
Thanks a lot~:-D
>>>> Also I don't think you need two properties for this. If the
>>>> snps,phyif-utmi property is specified it indicates that you want to
>>>> manually set the width and if it is absent you want to use the IC
>>>> default. All functions reading property-values indicate if the
>>>> property is missing.
>> Ah, it seems very good to me. One dts property "snps,phyif-utmi" can help
>> to reconfig phyif utmi width. And it seems that Felipe likes it very much
>> too. :-D
> yes, but please name it snps,phyif-utmi-width :-)

Yeah, thank you very much:-)

Best Regards
         William Wu
>
>
> Heiko
>
>
>