Re: [PATCH v6 3/5] usb: dwc3: add phyif_utmi_quirk

[Heiko Stuebner <heiko@sntech.de>]

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@sntech.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@rock-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