Re: [U-Boot] [PATCH 1/7] usb: xhci-rockchip: add rockchip dwc3 controller driver

[Kever Yang]

Hi Marek,

On 08/17/2016 04:13 PM, Marek Vasut wrote:

On 08/17/2016 09:42 AM, Kever Yang wrote:

From: MengDongyang 

This patch add support for rockchip dwc3 controller, which corresponding
to the two type-C port on rk3399 evb.
Only support usb2.0 currently for we have not enable the usb3.0 phy
driver and PD(fusb302) driver.

Signed-off-by: MengDongyang 
Signed-off-by: Kever Yang 
---

  drivers/usb/host/Makefile|   3 +
  drivers/usb/host/xhci-rockchip.c | 226 +++
  include/linux/usb/dwc3.h |   9 ++
  3 files changed, 238 insertions(+)
  create mode 100644 drivers/usb/host/xhci-rockchip.c

diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index 620d114..b46e8df 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -54,8 +54,11 @@ obj-$(CONFIG_USB_EHCI_RMOBILE) += ehci-rmobile.o
  obj-$(CONFIG_USB_EHCI_ZYNQ) += ehci-zynq.o
  
  # xhci

+ifdef CONFIG_DM_USB
  obj-$(CONFIG_USB_XHCI_HCD) += xhci.o xhci-mem.o xhci-ring.o
  obj-$(CONFIG_USB_XHCI_DWC3) += xhci-dwc3.o
+obj-$(CONFIG_USB_XHCI_ROCKCHIP) += xhci-rockchip.o
+endif

Please explain this (unrelated) ifdef .


This is not need, will remove.




  obj-$(CONFIG_USB_XHCI_ZYNQMP) += xhci-zynqmp.o
  obj-$(CONFIG_USB_XHCI_KEYSTONE) += xhci-keystone.o
  obj-$(CONFIG_USB_XHCI_EXYNOS) += xhci-exynos5.o
diff --git a/drivers/usb/host/xhci-rockchip.c b/drivers/usb/host/xhci-rockchip.c
new file mode 100644
index 000..faefa7e
--- /dev/null
+++ b/drivers/usb/host/xhci-rockchip.c
@@ -0,0 +1,226 @@
+/*
+ * Copyright (c) 2016 Rockchip, Inc.
+ * Authors: Daniel Meng 
+ *
+ * SPDX-License-Identifier:GPL-2.0+
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "xhci.h"
+
+DECLARE_GLOBAL_DATA_PTR;
+
+struct rockchip_xhci_platdata {
+   fdt_addr_t hcd_base;
+   fdt_addr_t phy_base;
+   struct gpio_desc vbus_gpio;
+};
+
+/**

Drop the other asterisk, this isn't javadoc.


Will remove in next version.




+ * Contains pointers to register base addresses
+ * for the usb controller.
+ */
+struct rockchip_xhci {
+   struct usb_platdata usb_plat;
+   struct xhci_ctrl ctrl;
+   struct xhci_hccr *hcd;
+   struct dwc3 *dwc3_reg;
+   struct udevice *dev;
+};
+
+static int xhci_usb_ofdata_to_platdata(struct udevice *dev)
+{
+   struct rockchip_xhci_platdata *plat = dev_get_platdata(dev);
+   struct udevice *child;
+
+   /*
+* Get the base address for XHCI controller from the device node
+*/
+   plat->hcd_base = dev_get_addr(dev);
+   if (plat->hcd_base == FDT_ADDR_T_NONE) {
+   debug("Can't get the XHCI register base address\n");
+   return -ENXIO;
+   }
+
+   /*
+* Get the base address for usbphy from the device node
+*/
+   for (device_find_first_child(dev, ); child;
+device_find_next_child()) {
+   if (!of_device_is_compatible(child, "rockchip,rk3399-usb3-phy"))
+   continue;
+   plat->phy_base = dev_get_addr(child);
+   break;
+   }
+
+   if (plat->phy_base == FDT_ADDR_T_NONE) {
+   debug("Can't get the usbphy register address\n");
+   return -ENXIO;
+   }
+
+   /* Vbus gpio */
+   gpio_request_by_name(dev, "rockchip,vbus-gpio", 0,
+>vbus_gpio, GPIOD_IS_OUT);

You should handle the return value here too


OK, will add in next version.




+   return 0;
+}

[...]


+static int rockchip_xhci_core_init(struct rockchip_xhci *rockchip)
+{
+   int ret;
+
+   /*rockchip_usb3_phy_init(rockchip->usb3_phy);*/

This should be removed or fixed.


Will remove in next version.



+   ret = dwc3_core_init(rockchip->dwc3_reg);
+   if (ret) {
+   debug("failed to initialize core\n");
+   return -EINVAL;
+   }
+
+   rockchip_dwc3_phy_setup(rockchip->dwc3_reg, rockchip);
+
+   /* We are hard-coding DWC3 core to Host Mode */
+   dwc3_set_mode(rockchip->dwc3_reg, DWC3_GCTL_PRTCAP_HOST);
+
+   return 0;
+}

A general/conceptual question -- do we need yet another xhci-foo.c
driver or could we unify them and have one single driver configured
via DT which works for all platforms ?


Basically I don't like to add a new xhci-foo.c, and there are already 
five other xhch-xxx.c

in drivers/usb/host/ which based on dwc3 controller.

I think it's best to use one single driver configured via DT, maybe we 
can update source
code from kernel and all platforms use dwc3-of-simple.c. The kernel code 
has a lot of

update since this driver port from kernel to uboot.

Thanks,
- Kever


___
U-Boot mailing list
U-Boot@lists.denx.de

Re: [U-Boot] [PATCH 1/7] usb: xhci-rockchip: add rockchip dwc3 controller driver

[Marek Vasut]

On 08/17/2016 09:42 AM, Kever Yang wrote:
> From: MengDongyang 
> 
> This patch add support for rockchip dwc3 controller, which corresponding
> to the two type-C port on rk3399 evb.
> Only support usb2.0 currently for we have not enable the usb3.0 phy
> driver and PD(fusb302) driver.
> 
> Signed-off-by: MengDongyang 
> Signed-off-by: Kever Yang 
> ---
> 
>  drivers/usb/host/Makefile|   3 +
>  drivers/usb/host/xhci-rockchip.c | 226 
> +++
>  include/linux/usb/dwc3.h |   9 ++
>  3 files changed, 238 insertions(+)
>  create mode 100644 drivers/usb/host/xhci-rockchip.c
> 
> diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
> index 620d114..b46e8df 100644
> --- a/drivers/usb/host/Makefile
> +++ b/drivers/usb/host/Makefile
> @@ -54,8 +54,11 @@ obj-$(CONFIG_USB_EHCI_RMOBILE) += ehci-rmobile.o
>  obj-$(CONFIG_USB_EHCI_ZYNQ) += ehci-zynq.o
>  
>  # xhci
> +ifdef CONFIG_DM_USB
>  obj-$(CONFIG_USB_XHCI_HCD) += xhci.o xhci-mem.o xhci-ring.o
>  obj-$(CONFIG_USB_XHCI_DWC3) += xhci-dwc3.o
> +obj-$(CONFIG_USB_XHCI_ROCKCHIP) += xhci-rockchip.o
> +endif

Please explain this (unrelated) ifdef .

>  obj-$(CONFIG_USB_XHCI_ZYNQMP) += xhci-zynqmp.o
>  obj-$(CONFIG_USB_XHCI_KEYSTONE) += xhci-keystone.o
>  obj-$(CONFIG_USB_XHCI_EXYNOS) += xhci-exynos5.o
> diff --git a/drivers/usb/host/xhci-rockchip.c 
> b/drivers/usb/host/xhci-rockchip.c
> new file mode 100644
> index 000..faefa7e
> --- /dev/null
> +++ b/drivers/usb/host/xhci-rockchip.c
> @@ -0,0 +1,226 @@
> +/*
> + * Copyright (c) 2016 Rockchip, Inc.
> + * Authors: Daniel Meng 
> + *
> + * SPDX-License-Identifier:  GPL-2.0+
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "xhci.h"
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +struct rockchip_xhci_platdata {
> + fdt_addr_t hcd_base;
> + fdt_addr_t phy_base;
> + struct gpio_desc vbus_gpio;
> +};
> +
> +/**

Drop the other asterisk, this isn't javadoc.

> + * Contains pointers to register base addresses
> + * for the usb controller.
> + */
> +struct rockchip_xhci {
> + struct usb_platdata usb_plat;
> + struct xhci_ctrl ctrl;
> + struct xhci_hccr *hcd;
> + struct dwc3 *dwc3_reg;
> + struct udevice *dev;
> +};
> +
> +static int xhci_usb_ofdata_to_platdata(struct udevice *dev)
> +{
> + struct rockchip_xhci_platdata *plat = dev_get_platdata(dev);
> + struct udevice *child;
> +
> + /*
> +  * Get the base address for XHCI controller from the device node
> +  */
> + plat->hcd_base = dev_get_addr(dev);
> + if (plat->hcd_base == FDT_ADDR_T_NONE) {
> + debug("Can't get the XHCI register base address\n");
> + return -ENXIO;
> + }
> +
> + /*
> +  * Get the base address for usbphy from the device node
> +  */
> + for (device_find_first_child(dev, ); child;
> +  device_find_next_child()) {
> + if (!of_device_is_compatible(child, "rockchip,rk3399-usb3-phy"))
> + continue;
> + plat->phy_base = dev_get_addr(child);
> + break;
> + }
> +
> + if (plat->phy_base == FDT_ADDR_T_NONE) {
> + debug("Can't get the usbphy register address\n");
> + return -ENXIO;
> + }
> +
> + /* Vbus gpio */
> + gpio_request_by_name(dev, "rockchip,vbus-gpio", 0,
> +  >vbus_gpio, GPIOD_IS_OUT);

You should handle the return value here too

> + return 0;
> +}

[...]

> +static int rockchip_xhci_core_init(struct rockchip_xhci *rockchip)
> +{
> + int ret;
> +
> + /*rockchip_usb3_phy_init(rockchip->usb3_phy);*/

This should be removed or fixed.

> + ret = dwc3_core_init(rockchip->dwc3_reg);
> + if (ret) {
> + debug("failed to initialize core\n");
> + return -EINVAL;
> + }
> +
> + rockchip_dwc3_phy_setup(rockchip->dwc3_reg, rockchip);
> +
> + /* We are hard-coding DWC3 core to Host Mode */
> + dwc3_set_mode(rockchip->dwc3_reg, DWC3_GCTL_PRTCAP_HOST);
> +
> + return 0;
> +}

A general/conceptual question -- do we need yet another xhci-foo.c
driver or could we unify them and have one single driver configured
via DT which works for all platforms ?

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot