Dear Heiko,


On 2016/7/10 7:47, Heiko Stuebner wrote:

Am Samstag, 9. Juli 2016, 11:38:00 schrieb William.wu:

quoted

Dear Heiko & Balbi，

On 2016/7/8 21:29, Felipe Balbi wrote:

quoted

Hi,

Heiko Stuebner [off-list ref] writes:

quoted

Am Donnerstag, 7. Juli 2016, 10:54:24 schrieb William Wu:

quoted

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 <redacted>
---

[...]

quoted

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

quoted

quoted

quoted

quoted

+			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

quoted

quoted

quoted

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip