From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Stuebner 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> References: <1467860066-15142-1-git-send-email-william.wu@rock-chips.com> <87d1moi7g7.fsf@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: Sender: linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: "William.wu" Cc: Felipe Balbi , gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org, linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, briannorris-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org, dianders-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org, kever.yang-TNX95d0MmH7DzftRWevZcw@public.gmane.org, huangtao-TNX95d0MmH7DzftRWevZcw@public.gmane.org, frank.wang-TNX95d0MmH7DzftRWevZcw@public.gmane.org, eddie.cai-TNX95d0MmH7DzftRWevZcw@public.gmane.org, John.Youn-HKixBCOQz3hWk0Htik3J/w@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org, robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, mark.rutland-5wv7dgnIgG8@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-rockchip.vger.kernel.org Am Samstag, 9. Juli 2016, 11:38:00 schrieb William.wu: > Dear Heiko & Balbi=EF=BC=8C >=20 > On 2016/7/8 21:29, Felipe Balbi wrote: > > Hi, > >=20 > > 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. > >>>=20 > >>> And refer to the dwc3 databook, the GUSB2PHYCFG.USBTRDTIM > >>> must be set to the corresponding value according to > >>> the UTMI+ PHY interface. > >>>=20 > >>> Signed-off-by: William Wu > >>> --- > >>=20 > >> [...] > >>=20 > >>> 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 > >>>=20 > >>> @@ -42,6 +42,10 @@ Optional properties: > >>> - snps,dis-u2-freeclk-exists-quirk: when set, clear the > >>>=20 > >>> u2_freeclk_exists in GUSB2PHYCFG, specify that USB2 PHY doesn't > >>> provide > >>>=20 > >>> a free-running PHY clock. > >>>=20 > >>> + - 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=20 select 8-bit > >>> + interface, value 1 select 16-bit interface. > >>=20 > >> maybe > >>=20 > >> snps,phyif-utmi-width =3D <8> or <16>; > >>=20 > >> devicetree is about describing the hardware, not the things that g= et > >> written to registers :-) . The conversion from the described width= to > >> the register value can easily be done in the driver. >=20 > Thanks for your suggestion:-) > Yes=EF=BC=8C =E2=80=9Csnps,phyif-utmi-width =3D <8> or <16>=E2=80=9D = is much clearer and easier to > understand. > And I have considered the same dts property for phyif-utmi, but I hav= e > no good idea about > the conversion from described width to the registers value for the ti= me > being. >=20 > About phyif utmi width configuration=EF=BC=8C we need to set two plac= es in > GUSB2PHYCFG register=EF=BC=8C > according to DWC3 USB3.0 controller databook version3.00a=EF=BC=8C6.3= =2E46 > GUSB2PHYCFG >=20 > ---------------------------------------------------------------------= ----- > -------------------- Bits | Name | Description > ---------------------------------------------------------------------= ----- > -------------------- 13:10 | USBTRDTIM | Sets the turnar= ound > time in PHY clocks. > | | 4'h5: When the MAC >=20 > interface is 16-bit UTMI+ >=20 > | | 4'h9: When the MAC >=20 > interface is 8-bit UTMI+/ULPI. > ---------------------------------------------------------------------= ----- > -------------------- 3 | PHYIF | If UTMI+ = is > selected, the application uses this bit to configure >=20 > | | core to support a UTMI= + >=20 > PHY with an 8- or 16-bit interface. >=20 > | | 1'b0: 8 bits > | | 1'b1: 16 bits >=20 > ---------------------------------------------------------------------= ----- > -------------------- >=20 > And I think maybe I can try to do this: > change it in dts: > snps,phyif-utmi-width =3D <8> or <16>; >=20 > Then convert to register value like this=EF=BC=9A > device_property_read_u8(dev, "snps,phyif-utmi-width", > &phyif_utmi_width); >=20 > dwc->phyif_utmi =3D phyif_utmi_width >> 4; >=20 > Ater the conversion=EF=BC=8C dwc->phyif_utmi value 0 means 8 bits, = value 1 > means 16 bits, > and it's easier for us to config GUSB2PHYCFG. >=20 > Is it OK? or you could just store the actual width value read from the dts and ma= ke=20 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 =3D device_property_read_u8(dev, "snps,phyif-utmi-width", &dwc->phyif_utmi); if (ret < 0) { dwc->phyif_utmi =3D 0; else if (dwc->phyif_utmi !=3D 16 && dwc->phyif_utmi !=3D 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 &=3D ~(DWC3_GUSB2PHYCFG_PHYIF_MASK | DWC3_GUSB2PHYCFG_USBTRDTIM_MASK); usbtrdtim =3D (dwc->phyif_utmi =3D=3D 16) ? USBTRDTIM_UT= MI_16_BIT : USBTRDTIM_UTMI_8_BIT; phyif =3D (dwc->phyif_utmi =3D=3D 16) ? 1 : 0; reg |=3D 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 t= o > >> 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. >=20 > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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