From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756892AbcGIXsT (ORCPT ); Sat, 9 Jul 2016 19:48:19 -0400 Received: from gloria.sntech.de ([95.129.55.99]:53901 "EHLO gloria.sntech.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752846AbcGIXsR convert rfc822-to-8bit (ORCPT ); Sat, 9 Jul 2016 19:48:17 -0400 From: Heiko Stuebner To: "William.wu" 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 Subject: Re: [PATCH v6 3/5] usb: dwc3: add phyif_utmi_quirk Date: Sun, 10 Jul 2016 01:47:48 +0200 Message-ID: <4648785.SDx8KzkhqW@phil> User-Agent: KMail/4.14.10 (Linux/4.3.0-1-amd64; KDE/4.14.14; x86_64; ; ) In-Reply-To: References: <1467860066-15142-1-git-send-email-william.wu@rock-chips.com> <87d1moi7g7.fsf@linux.intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8BIT Content-Type: text/plain; charset="utf-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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) | } > >> 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 :-) Heiko