[U-Boot] [PATCH v2 3/4] usb: dwc3: add support for 16 bit UTMI+ interface

[Kever Yang]

Hi Marek,

On 08/26/2016 05:09 PM, Marek Vasut wrote:
> On 08/25/2016 03:17 AM, Kever Yang wrote:
>> Hi Marek,
>>
>> On 08/24/2016 07:38 PM, Marek Vasut wrote:
>>> On 08/24/2016 05:46 AM, Kever Yang wrote:
>>>> The dwc3 controller is using 8 bit UTMI+ interface for USB2.0 PHY,
>>>> add one variable in dwc3/dwc3_device struct to support 16 bit
>>>> UTMI+ interface on some SoCs like Rockchip rk3399.
>>>>
>>>> Signed-off-by: Kever Yang <kever.yang at rock-chips.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - use a variable to identify utmi+ bus width instead of CONFIG MACRO
>>>>
>>>>    drivers/usb/dwc3/core.c |  6 ++++++
>>>>    drivers/usb/dwc3/core.h | 12 ++++++++++++
>>>>    include/dwc3-uboot.h    |  1 +
>>>>    3 files changed, 19 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 85cc96a..0613508 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -388,6 +388,11 @@ static void dwc3_phy_setup(struct dwc3 *dwc)
>>>>        if (dwc->dis_u2_susphy_quirk)
>>>>            reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
>>>>    +    if (dwc->usb2_phyif_utmi_width == 16) {
>>>> +        reg &= ~DWC3_GUSB2PHYCFG_USBTRDTIM_MASK;
>>>> +        reg |= DWC3_GUSB2PHYCFG_USBTRDTIM_16BIT;
>>>> +        reg |= DWC3_GUSB2PHYCFG_PHYIF_16BIT;
>>>> +    }
>>> Didn't we agree to pull this info from OF ?
>> Yes, the dwc->usb2_phyif_utmi_width is from OF,  but I make the DT parse
>> in board file instead of in the dwc3 driver,  see my patch:
>> [PATCH v2 2/4] board: evb-rk3399: add api to support dwc3 gadget
> So this is basically a passing of platform data ?

Yes, this is what other platform do, I just extend one more setting.

>
>> I think implement in this way won't affect other soc also using dwc3
>> controller,
>> the DT parse would cost some time which may not necessary for other soc.
> The time needed to run the DT parsing is completely insignificant.
> This sounds like a premature optimization.

Which way of implement this DT parse do you prefer and more reasonable,
in dwc3 driver or in board init and dwc3 driver use it as platform data?

Thanks,
- Kever

>
>>>>        dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>>>>          mdelay(100);
>>>> @@ -676,6 +681,7 @@ int dwc3_uboot_init(struct dwc3_device *dwc3_dev)
>>>>          dwc->lpm_nyet_threshold = lpm_nyet_threshold;
>>>>        dwc->tx_de_emphasis = tx_de_emphasis;
>>>> +    dwc->usb2_phyif_utmi_width = dwc3_dev->usb2_phyif_utmi_width;
>>>>          dwc->hird_threshold = hird_threshold
>>>>            | (dwc->is_utmi_l1_suspend << 4);
>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>>> index 72d2fcd..d5bdf97 100644
>>>> --- a/drivers/usb/dwc3/core.h
>>>> +++ b/drivers/usb/dwc3/core.h
>>>> @@ -74,6 +74,7 @@
>>>>    #define DWC3_GCTL        0xc110
>>>>    #define DWC3_GEVTEN        0xc114
>>>>    #define DWC3_GSTS        0xc118
>>>> +#define DWC3_GUCTL1        0xc11c
>>>>    #define DWC3_GSNPSID        0xc120
>>>>    #define DWC3_GGPIO        0xc124
>>>>    #define DWC3_GUID        0xc128
>>>> @@ -162,7 +163,17 @@
>>>>      /* Global USB2 PHY Configuration Register */
>>>>    #define DWC3_GUSB2PHYCFG_PHYSOFTRST    (1 << 31)
>>>> +#define DWC3_GUSB2PHYCFG_ENBLSLPM   (1 << 8)
>>>>    #define DWC3_GUSB2PHYCFG_SUSPHY        (1 << 6)
>>>> +#define DWC3_GUSB2PHYCFG_PHYIF_8BIT    (0 << 3)
>>>> +#define DWC3_GUSB2PHYCFG_PHYIF_16BIT    (1 << 3)
>>>> +#define DWC3_GUSB2PHYCFG_USBTRDTIM_SHIFT    (10)
>>>> +#define DWC3_GUSB2PHYCFG_USBTRDTIM_MASK    (0xf << \
>>>> +        DWC3_GUSB2PHYCFG_USBTRDTIM_SHIFT)
>>>> +#define DWC3_GUSB2PHYCFG_USBTRDTIM_16BIT (0x5 << \
>>>> +        DWC3_GUSB2PHYCFG_USBTRDTIM_SHIFT)
>>>> +#define DWC3_GUSB2PHYCFG_USBTRDTIM_8BIT (0x9 << \
>>>> +        DWC3_GUSB2PHYCFG_USBTRDTIM_SHIFT)
>>>>      /* Global USB3 PIPE Control Register */
>>>>    #define DWC3_GUSB3PIPECTL_PHYSOFTRST    (1 << 31)
>>>> @@ -813,6 +824,7 @@ struct dwc3 {
>>>>          unsigned        tx_de_emphasis_quirk:1;
>>>>        unsigned        tx_de_emphasis:2;
>>>> +    unsigned        usb2_phyif_utmi_width;
>>>>        int            index;
>>>>        struct list_head        list;
>>>>    };
>>>> diff --git a/include/dwc3-uboot.h b/include/dwc3-uboot.h
>>>> index 7af2ad1..cc9ffe8 100644
>>>> --- a/include/dwc3-uboot.h
>>>> +++ b/include/dwc3-uboot.h
>>>> @@ -33,6 +33,7 @@ struct dwc3_device {
>>>>        unsigned dis_u2_susphy_quirk;
>>>>        unsigned tx_de_emphasis_quirk;
>>>>        unsigned tx_de_emphasis;
>>>> +    unsigned usb2_phyif_utmi_width;
>>> The utmi width is either 8 or 16, so you can continue the bitfield
>>> instead of wasting 32 bits.
>> I think you mean the usb2_phyif_utmi_width in struct dwc3, but not
>> struct dwc3_device, right?
> Yes, sorry.
>
>> Thanks,
>> -Kever
>>>>        int index;
>>>>    };
>>>>   
>>
>