| 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: Sun, 10 Jul 2016 01:47:48 +0200
From: Heiko Stuebner <heiko@...ech.de>
To: "William.wu" <William.wu@...k-chips.com>
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
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) |
}
> >> 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
Powered by blists - more mailing lists