| 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
| ||
|
Date: Sat, 9 Jul 2016 11:38:00 +0800
From: "William.wu" <William.wu@...k-chips.com>
To: Felipe Balbi <balbi@...nel.org>, Heiko Stuebner <heiko@...ech.de>
Cc: 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 & 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?
>>
>>
>> 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
>> But it looks like there is already a precendence in
>> snps,tx_de_emphasis(_quirk), so maybe Felipe has a different opinion here?
>>
>>
>>
>>> - snps,is-utmi-l1-suspend: true when DWC3 asserts output signal
>>> utmi_l1_suspend_n, false when asserts utmi_sleep_n
>>> - snps,hird-threshold: HIRD threshold
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 0b7bfd2..94036b1 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -408,6 +408,7 @@ static void dwc3_cache_hwparams(struct dwc3 *dwc)
>>> static int dwc3_phy_setup(struct dwc3 *dwc)
>>> {
>>> u32 reg;
>>> + u32 usbtrdtim;
>>> int ret;
>>>
>>> reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
>>> @@ -503,6 +504,15 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
>>> if (dwc->dis_u2_freeclk_exists_quirk)
>>> reg &= ~DWC3_GUSB2PHYCFG_U2_FREECLK_EXISTS;
>>>
>>> + if (dwc->phyif_utmi_quirk) {
>>> + reg &= ~(DWC3_GUSB2PHYCFG_PHYIF_MASK |
>>> + DWC3_GUSB2PHYCFG_USBTRDTIM_MASK);
>>> + usbtrdtim = dwc->phyif_utmi ? USBTRDTIM_UTMI_16_BIT :
>>> + USBTRDTIM_UTMI_8_BIT;
>>> + reg |= DWC3_GUSB2PHYCFG_PHYIF(dwc->phyif_utmi) |
>>> + DWC3_GUSB2PHYCFG_USBTRDTIM(usbtrdtim);
>>> + }
>>> +
>>> dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>>>
>>> return 0;
>>> @@ -834,6 +844,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>> struct resource *res;
>>> struct dwc3 *dwc;
>>> u8 lpm_nyet_threshold;
>>> + u8 phyif_utmi;
>>> u8 tx_de_emphasis;
>>> u8 hird_threshold;
>>>
>>> @@ -880,6 +891,9 @@ static int dwc3_probe(struct platform_device *pdev)
>>> /* default to highest possible threshold */
>>> lpm_nyet_threshold = 0xff;
>>>
>>> + /* default to UTMI+ 8-bit interface */
>>> + phyif_utmi = 0;
>>> +
>>> /* default to -3.5dB de-emphasis */
>>> tx_de_emphasis = 1;
>>>
>>> @@ -929,6 +943,10 @@ static int dwc3_probe(struct platform_device *pdev)
>>> "snps,dis_rxdet_inp3_quirk");
>>> dwc->dis_u2_freeclk_exists_quirk = device_property_read_bool(dev,
>>> "snps,dis-u2-freeclk-exists-quirk");
>>> + dwc->phyif_utmi_quirk = device_property_read_bool(dev,
>>> + "snps,phyif-utmi-quirk");
>>> + device_property_read_u8(dev, "snps,phyif-utmi",
>>> + &phyif_utmi);
>>
>> As described above device_property_read_u8 will return an error if the
>> property is not present, so you could fill your dwc->phyif_utmi_quirk from
>> that:
>>
>> ret = device_property_read_u8(dev, "snps,phyif-utmi",
>> &phyif_utmi);
>> dwc->phyif_utmi_quirk = (ret == 0) ? true : false;
> I like this much better :-) Unfortunately can't fix tx_deemphasis due to
> backwards compatibility :-s
I'll try to follow Heiko's suggestion and fix the dwc->phyif_utmi_quirk
next patch.
Best regards,
William.wu
>
Powered by blists - more mailing lists