| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <620c4bcc-04db-143d-c2a1-74ab5c6c51f0@rock-chips.com>
Date: Wed, 13 Jul 2016 11:21:59 +0800
From: "William.wu" <William.wu@...k-chips.com>
To: Heiko Stuebner <heiko@...ech.de>
Cc: Felipe Balbi <balbi@...nel.org>, gregkh@...uxfoundation.org,
linux-rockchip@...ts.infradead.org, briannorris@...gle.com,
dianders@...gle.com, kever.yang@...k-chips.com,
huangtao@...k-chips.com, frank.wang@...k-chips.com,
eddie.cai@...k-chips.com, John.Youn@...opsys.com,
linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
sergei.shtylyov@...entembedded.com, robh+dt@...nel.org,
mark.rutland@....com, devicetree@...r.kernel.org
Subject: Re: [PATCH v6 3/5] usb: dwc3: add phyif_utmi_quirk
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@...ech.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@...k-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
>
>
>
Powered by blists - more mailing lists