On 09/01/2016 04:14 AM, Kever Yang wrote:

This patch add board_usb_init() and interrupt callback for dwc3 gadget.

Signed-off-by: Kever Yang kever.yang@rock-chips.com

Changes in v4:

• parse DT for quirk, base address and maximum speed

Changes in v3:

• remove utmi width DT parse from borad init

Changes in v2:

• parse dt for utmi width

board/rockchip/evb_rk3399/evb-rk3399.c | 51 ++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+)

diff --git a/board/rockchip/evb_rk3399/evb-rk3399.c b/board/rockchip/evb_rk3399/evb-rk3399.c index d394276..58bfa78 100644 --- a/board/rockchip/evb_rk3399/evb-rk3399.c +++ b/board/rockchip/evb_rk3399/evb-rk3399.c @@ -7,6 +7,8 @@ #include <dm.h> #include <dm/pinctrl.h> #include <asm/arch/periph.h> +#include <usb.h> +#include <dwc3-uboot.h>

DECLARE_GLOBAL_DATA_PTR;

@@ -54,3 +56,52 @@ void dram_init_banksize(void) gd->bd->bi_dram[0].start = 0x200000; gd->bd->bi_dram[0].size = 0x80000000; }

+#ifdef CONFIG_USB_DWC3 +static struct dwc3_device dwc3_device_data = {

• .index = 0,

+};

+int usb_gadget_handle_interrupts(void) +{

• dwc3_uboot_handle_interrupt(0);
• return 0;

+}

+int board_usb_init(int index, enum usb_init_type init) +{

• const void *blob = gd->fdt_blob;
• int node;
• const char *string;
• node = fdt_node_offset_by_compatible(blob, -1,

• 	"rockchip,rk3399-xhci");
  

• if (node < 0) {

• debug("%s dwc3 node not found\n", __func__);
  

• } else {

• dwc3_device_data.base = fdtdec_get_addr(blob, node, "reg");
  

• dwc3_device_data.dis_u2_susphy_quirk =
  

• 	fdtdec_get_int(blob, node,
  

• 		       "snps,dis-u2-susphy-quirk", -1);
  

• string = fdt_getprop(blob, node, "maximum-speed", NULL);
  

• if (string) {
  

• 	if (0 == strcmp(string, "super-speed"))
  

• 		dwc3_device_data.maximum_speed = USB_SPEED_SUPER;
  

• 	else if (0 == strcmp(string, "high-speed"))
  

• 		dwc3_device_data.maximum_speed = USB_SPEED_HIGH;
  

• 	else if (0 == strcmp(string, "full-speed"))
  

• 		dwc3_device_data.maximum_speed = USB_SPEED_FULL;
  

• 	else
  

• 		debug("%s: Cannot decode speed'%s'\n", __func__,
  

• 		      string);
  

• } else {
  

• 	dwc3_device_data.maximum_speed = USB_SPEED_HIGH;
  

• }
  

• /* Hardcode controller in peripheral mode */
  

• dwc3_device_data.dr_mode = USB_DR_MODE_PERIPHERAL;
  

• }
• return dwc3_uboot_init(&dwc3_device_data);

+} +#endif

So will every board have a copy of this exact code, verbatim ?

On 09/02/2016 11:58 AM, Kever Yang wrote:

Hi Marek,

On 09/01/2016 04:59 PM, Marek Vasut wrote:

...

On 09/01/2016 04:14 AM, Kever Yang wrote:

...

This patch add board_usb_init() and interrupt callback for dwc3 gadget.

Signed-off-by: Kever Yang kever.yang@rock-chips.com

Changes in v4:

• parse DT for quirk, base address and maximum speed

Changes in v3:

• remove utmi width DT parse from borad init

Changes in v2:

• parse dt for utmi width

  board/rockchip/evb_rk3399/evb-rk3399.c | 51

++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+)

diff --git a/board/rockchip/evb_rk3399/evb-rk3399.c b/board/rockchip/evb_rk3399/evb-rk3399.c index d394276..58bfa78 100644 --- a/board/rockchip/evb_rk3399/evb-rk3399.c +++ b/board/rockchip/evb_rk3399/evb-rk3399.c @@ -7,6 +7,8 @@ #include <dm.h> #include <dm/pinctrl.h> #include <asm/arch/periph.h> +#include <usb.h> +#include <dwc3-uboot.h> DECLARE_GLOBAL_DATA_PTR; @@ -54,3 +56,52 @@ void dram_init_banksize(void) gd->bd->bi_dram[0].start = 0x200000; gd->bd->bi_dram[0].size = 0x80000000; }

+#ifdef CONFIG_USB_DWC3 +static struct dwc3_device dwc3_device_data = {

• .index = 0,

+};

+int usb_gadget_handle_interrupts(void) +{

• dwc3_uboot_handle_interrupt(0);
• return 0;

+}

+int board_usb_init(int index, enum usb_init_type init) +{

• const void *blob = gd->fdt_blob;
• int node;
• const char *string;
• node = fdt_node_offset_by_compatible(blob, -1,

•        "rockchip,rk3399-xhci");
  

• if (node < 0) {

•    debug("%s dwc3 node not found\n", __func__);
  

• } else {

•    dwc3_device_data.base = fdtdec_get_addr(blob, node, "reg");
  

•    dwc3_device_data.dis_u2_susphy_quirk =
  

•        fdtdec_get_int(blob, node,
  

•                   "snps,dis-u2-susphy-quirk", -1);
  

•    string = fdt_getprop(blob, node, "maximum-speed", NULL);
  

•    if (string) {
  

•        if (0 == strcmp(string, "super-speed"))
  

•            dwc3_device_data.maximum_speed = USB_SPEED_SUPER;
  

•        else if (0 == strcmp(string, "high-speed"))
  

•            dwc3_device_data.maximum_speed = USB_SPEED_HIGH;
  

•        else if (0 == strcmp(string, "full-speed"))
  

•            dwc3_device_data.maximum_speed = USB_SPEED_FULL;
  

•        else
  

•            debug("%s: Cannot decode speed'%s'\n", __func__,
  

•                  string);
  

•    } else {
  

•        dwc3_device_data.maximum_speed = USB_SPEED_HIGH;
  

•    }
  

•    /* Hardcode controller in peripheral mode */
  

•    dwc3_device_data.dr_mode = USB_DR_MODE_PERIPHERAL;
  

• }
• return dwc3_uboot_init(&dwc3_device_data);

+} +#endif

So will every board have a copy of this exact code, verbatim ?

Yes for rk3399 based board. Not sure why you ask this question, but I guess you still want these parse happen in driver instead of board_init()?

Of course.

I didn't make these code in dwc3 driver because other SoC dts are different now, they are init in platdata data structure.

You can retain this behavior while adding the support for OF probing, see my reply to your v3 patchset.

To be honest, I want to make the upstream code to support rk3399 dwc3, but I don't want to touch too much code in other SoC.

This would result in having many various vendor hacks in board files, which would be almost identical but only slightly broken and nightmare to maintain. We have been there are we're glad we are getting out of there. So no, I will not allow this. Instead, do changes to common code and let others review/test them for you, this is perfectly fine.

btw the way of parsing DT in the driver I outlined in the reply to v3 should be pretty unintrusive.

Thanks,

• Kever

On 09/01/2016 04:14 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@rock-chips.com

Changes in v4:

• use 1 bit for usb2_phyif_utmi_width instead of 5bit

Changes in v3:

• Parse the DT for utmi+ interface width in dwc3 driver

Changes in v2:

• use a variable to identify utmi+ bus width instead of CONFIG MACRO

drivers/usb/dwc3/core.c | 19 +++++++++++++++++++ drivers/usb/dwc3/core.h | 12 ++++++++++++ 2 files changed, 31 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 85cc96a..8792f99 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -16,6 +16,7 @@

#include <common.h> #include <malloc.h> +#include <fdtdec.h> #include <dwc3-uboot.h> #include <asm/dma-mapping.h> #include <linux/ioport.h> @@ -29,6 +30,8 @@

#include "linux-compat.h"

+DECLARE_GLOBAL_DATA_PTR;

static LIST_HEAD(dwc3_list); /* -------------------------------------------------------------------------- */

@@ -388,6 +391,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 == 1) {

• reg &= ~DWC3_GUSB2PHYCFG_USBTRDTIM_MASK;
  

• reg |= DWC3_GUSB2PHYCFG_USBTRDTIM_16BIT;
  

• reg |= DWC3_GUSB2PHYCFG_PHYIF_16BIT;
  

• } dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);

  mdelay(100);

@@ -621,6 +629,8 @@ int dwc3_uboot_init(struct dwc3_device *dwc3_dev) int ret;

void *mem;

• const void *blob = gd->fdt_blob;

• int node;

  mem = devm_kzalloc(dev, sizeof(*dwc) + DWC3_ALIGN_MASK, GFP_KERNEL); if (!mem)

@@ -682,6 +692,15 @@ int dwc3_uboot_init(struct dwc3_device *dwc3_dev)

dwc->index = dwc3_dev->index;

• node = fdt_node_offset_by_compatible(blob, -1,

• 	"rockchip,rk3399-xhci");
  

• if (node < 0)

• debug("%s dwc3 node not found\n", __func__);
  

• else

• dwc->usb2_phyif_utmi_width =
  

• 	(fdtdec_get_int(blob, node, "snps,phyif-utmi-bits", -1)
  

• 	 == 16) ? 1 : 0;
  

What happens if fdtdec_get_int() returns -1 here ? Is that an error?

I just took a look into Linux and they use "phy_type" property to describe the PHY bus, with possible values being "utmi" "utmi_wide" "ulpi" etc . Do we need to invent new vendor-specific property ? See Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt in Linux for example.

dwc3_cache_hwparams(dwc);

ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE); diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 72d2fcd..7484d5f 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:1; int index; struct list_head list;

};