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
>
>
>
