From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751293AbcGMDXt (ORCPT ); Tue, 12 Jul 2016 23:23:49 -0400 Received: from regular1.263xmail.com ([211.150.99.140]:35856 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750974AbcGMDXj (ORCPT ); Tue, 12 Jul 2016 23:23:39 -0400 X-263anti-spam: KSV:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-ADDR-CHECKED: 0 X-RL-SENDER: william.wu@rock-chips.com X-FST-TO: devicetree@vger.kernel.org X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: william.wu@rock-chips.com X-UNIQUE-TAG: X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH v6 3/5] usb: dwc3: add phyif_utmi_quirk To: Heiko Stuebner References: <1467860066-15142-1-git-send-email-william.wu@rock-chips.com> <87d1moi7g7.fsf@linux.intel.com> <4648785.SDx8KzkhqW@phil> Cc: Felipe Balbi , gregkh@linuxfoundation.org, linux-rockchip@lists.infradead.org, briannorris@google.com, dianders@google.com, kever.yang@rock-chips.com, huangtao@rock-chips.com, frank.wang@rock-chips.com, eddie.cai@rock-chips.com, John.Youn@synopsys.com, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org, sergei.shtylyov@cogentembedded.com, robh+dt@kernel.org, mark.rutland@arm.com, devicetree@vger.kernel.org From: "William.wu" Message-ID: <620c4bcc-04db-143d-c2a1-74ab5c6c51f0@rock-chips.com> Date: Wed, 13 Jul 2016 11:21:59 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <4648785.SDx8KzkhqW@phil> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 >>>>> --- >>>> [...] >>>> >>>>> 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 > > >