[RFC v1 0/7] common: usb_onboard_hub: Add support for Odroid onboard USB hub reset

[RFC v1 0/7] common: usb_onboard_hub: Add support for Odroid onboard USB hub reset

[Anand Moon]

Odroid C1+/C2/C4/N2/N2-plus devices have an onboard USB hub
controlled by a reset GPIO pin. Adds changes to the onboard USB hub
driver to perform a proper reset of the USB hub.

Added conditional compilation for I2C initialization based
on CONFIG_DM_I2C for failed to compile.

Thanks
-Anand

Anand Moon (7):
  usb: onboard-hub: Add support for Genesys GL852G hub
  configs: odorid-c2: Enable Onboard HUB driver
  usb: onboard-hub: Add support for Genesys GL853G
  configs: odorid-n2: Enable Oboard HUB driver
  usb: onboard-hub: Add support for VL817 USB hub
  configs: odorid-c4: Enable Oboard HUB driver
  usb: onboard-hub: Add conditional compilation for I2C initialization

 common/usb_onboard_hub.c    | 28 ++++++++++++++++++++++++++++
 configs/odroid-c2_defconfig |  1 +
 configs/odroid-c4_defconfig |  1 +
 configs/odroid-n2_defconfig |  1 +
 4 files changed, 31 insertions(+)


base-commit: 48db49b0977cc1c9c9abf82c0fb704238fcef4fd
-- 
2.49.0

[RFC v1 1/7] usb: onboard-hub: Add support for Genesys GL852G hub

[Anand Moon]

Add support for the Genesys GL852G USB2.0 Hub on Odroid C1+
and Odroid C2. The GL852G driver trigger hub reset signal
which toggles the gpio.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
Still some issue with USB hub on Odroid C2
=> dm tree

 vidconsole    0  [ + ]   vidconsole0           |   |   `-- vpu@d0100000.vidconsole0
 display       0  [   ]   meson_dw_hdmi         |   |-- hdmi-tx@c883a000
 phy           0  [ + ]   meson_gxbb_usb2_phy   |   |-- phy@c0000020
 usb           0  [ + ]   dwc2_usb              |   `-- usb@c9100000
 usb_hub       0  [ + ]   usb_hub               |       `-- usb_hub
 usb_hub       1  [ + ]   usb_hub               |           `-- usb_hub
 regulator     0  [ + ]   regulator_fixed       |-- regulator-usb-pwrs
---
 common/usb_onboard_hub.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
index 7fe62b043e6..39bbc1aefa2 100644
--- a/common/usb_onboard_hub.c
+++ b/common/usb_onboard_hub.c
@@ -227,6 +227,10 @@ static const struct onboard_hub_data usb5744_data = {
 	.reset_us = 5,
 };
 
+static const struct onboard_hub_data genesys_gl852g_data = {
+	.reset_us = 50,
+};
+
 static const struct udevice_id usb_onboard_hub_ids[] = {
 	/* Use generic usbVID,PID dt-bindings (usb-device.yaml) */
 	{	.compatible = "usb424,2514",	/* USB2514B USB 2.0 */
@@ -237,6 +241,9 @@ static const struct udevice_id usb_onboard_hub_ids[] = {
 	}, {
 		.compatible = "usb424,5744",	/* USB5744 USB 3.0 */
 		.data = (ulong)&usb5744_data,
+	}, {
+		.compatible = "usb5e3,610",     /* GL852G USB 2.0 */
+		.data = (ulong)&genesys_gl852g_data,
 	}
 };
 
-- 
2.49.0

[RFC v1 2/7] configs: odorid-c2: Enable Onboard HUB driver

[Anand Moon]

Enable the onboard USB HUB driver to power the
USB_HUB_GL852G-OHG on the Odroid C2.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 configs/odroid-c2_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/odroid-c2_defconfig b/configs/odroid-c2_defconfig
index bcaab46f8bc..f8a66a923c9 100644
--- a/configs/odroid-c2_defconfig
+++ b/configs/odroid-c2_defconfig
@@ -59,6 +59,7 @@ CONFIG_SYSINFO_SMBIOS=y
 CONFIG_USB=y
 CONFIG_USB_DWC2=y
 CONFIG_USB_KEYBOARD=y
+CONFIG_USB_ONBOARD_HUB=y
 CONFIG_VIDEO=y
 # CONFIG_VIDEO_BPP8 is not set
 # CONFIG_VIDEO_BPP16 is not set
-- 
2.49.0

[RFC v1 3/7] usb: onboard-hub: Add support for Genesys GL853G

[Anand Moon]

Enable support for the Genesys GL853G USB2.0 and USB3.1 Hub on
the Odroid N2 and N2-plus. The GL853G driver activates the hub
reset signal, which toggles the GPIO.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
=> dm tree

 simple_bus   11  [ + ]   dwc3-meson-g12a       |   `-- usb@ffe09000
 usb_gadget    0  [   ]   dwc2-udc-otg          |       |-- usb@ff400000
 usb           0  [ + ]   xhci-dwc3             |       `-- usb@ff500000
 usb_hub       0  [ + ]   usb_hub               |           `-- usb_hub
 usb_hub       1  [ + ]   usb_hub               |               |-- usb_hub
 usb_mass_s    0  [ + ]   usb_mass_storage      |               |   `-- usb_mass_storage
 blk           2  [ + ]   usb_storage_blk       |               |       |-- usb_mass_storage.lun0
 partition     0  [ + ]   blk_partition         |               |       |   `-- usb_mass_storage.lun0:1
 bootdev       3  [   ]   usb_bootdev           |               |       `-- usb_mass_storage.lun0.bootdev
 usb_hub       2  [ + ]   usb_hub               |               `-- usb_hub
 clk           2  [ + ]   fixed_clock           |-- xtal-clk
---
 common/usb_onboard_hub.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
index 39bbc1aefa2..3914e2f773d 100644
--- a/common/usb_onboard_hub.c
+++ b/common/usb_onboard_hub.c
@@ -231,6 +231,10 @@ static const struct onboard_hub_data genesys_gl852g_data = {
 	.reset_us = 50,
 };
 
+static const struct onboard_hub_data genesys_gl853g_data = {
+	.reset_us = 50,
+};
+
 static const struct udevice_id usb_onboard_hub_ids[] = {
 	/* Use generic usbVID,PID dt-bindings (usb-device.yaml) */
 	{	.compatible = "usb424,2514",	/* USB2514B USB 2.0 */
@@ -244,6 +248,9 @@ static const struct udevice_id usb_onboard_hub_ids[] = {
 	}, {
 		.compatible = "usb5e3,610",     /* GL852G USB 2.0 */
 		.data = (ulong)&genesys_gl852g_data,
+	}, {
+		.compatible = "usb5e3,620",     /* GL852G USB 3.1 */
+		.data = (ulong)&genesys_gl853g_data,
 	}
 };
 
-- 
2.49.0

[RFC v1 4/7] configs: odorid-n2: Enable Oboard HUB driver

[Anand Moon]

Enable the onboard USB HUB driver to power the
USB_HUB_GL853G-OHG on the Odroid N2 and Odroid N2-pluss.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 configs/odroid-n2_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/odroid-n2_defconfig b/configs/odroid-n2_defconfig
index a8cbaee3c96..1bf687c1e6d 100644
--- a/configs/odroid-n2_defconfig
+++ b/configs/odroid-n2_defconfig
@@ -64,6 +64,7 @@ CONFIG_USB_DWC3=y
 # CONFIG_USB_DWC3_GADGET is not set
 CONFIG_USB_DWC3_MESON_G12A=y
 CONFIG_USB_KEYBOARD=y
+CONFIG_USB_ONBOARD_HUB=y
 CONFIG_USB_GADGET=y
 CONFIG_USB_GADGET_VENDOR_NUM=0x1b8e
 CONFIG_USB_GADGET_PRODUCT_NUM=0xfada
-- 
2.49.0

[RFC v1 5/7] usb: onboard-hub: Add support for VL817 USB hub

[Anand Moon]

Enable support for the Via labs VL817 USB2.0 and USB3.1 Hub
on the Odroid C4. The Via labs VL817 USB driver activates the
hub reset signal, which toggles the GPIO.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
=> dm tree
 simple_bus   11  [ + ]   dwc3-meson-g12a       |   `-- usb@ffe09000
 usb_gadget    0  [   ]   dwc2-udc-otg          |       |-- usb@ff400000
 usb           0  [ + ]   xhci-dwc3             |       `-- usb@ff500000
 usb_hub       0  [ + ]   usb_hub               |           `-- usb_hub
 usb_hub       1  [ + ]   usb_hub               |               |-- usb_hub
 usb_mass_s    0  [ + ]   usb_mass_storage      |               |   `-- usb_mass_storage
 blk           2  [ + ]   usb_storage_blk       |               |       |-- usb_mass_storage.lun0
 partition     0  [ + ]   blk_partition         |               |       |   |-- usb_mass_storage.lun0:1
 partition     1  [ + ]   blk_partition         |               |       |   |-- usb_mass_storage.lun0:2
 partition     2  [ + ]   blk_partition         |               |       |   |-- usb_mass_storage.lun0:3
 partition     3  [ + ]   blk_partition         |               |       |   `-- usb_mass_storage.lun0:4
 bootdev       3  [   ]   usb_bootdev           |               |       `-- usb_mass_storage.lun0.bootdev
 usb_hub       2  [ + ]   usb_hub               |               `-- usb_hub
---
 common/usb_onboard_hub.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
index 3914e2f773d..1242513c631 100644
--- a/common/usb_onboard_hub.c
+++ b/common/usb_onboard_hub.c
@@ -235,6 +235,10 @@ static const struct onboard_hub_data genesys_gl853g_data = {
 	.reset_us = 50,
 };
 
+static const struct onboard_hub_data vialab_vl817_data = {
+	.reset_us = 10,
+};
+
 static const struct udevice_id usb_onboard_hub_ids[] = {
 	/* Use generic usbVID,PID dt-bindings (usb-device.yaml) */
 	{	.compatible = "usb424,2514",	/* USB2514B USB 2.0 */
@@ -251,6 +255,12 @@ static const struct udevice_id usb_onboard_hub_ids[] = {
 	}, {
 		.compatible = "usb5e3,620",     /* GL852G USB 3.1 */
 		.data = (ulong)&genesys_gl853g_data,
+	}, {
+		.compatible = "usb2109,817",	/* Via labs VL817 2.0 */
+		.data = (ulong)&vialab_vl817_data,
+	}, {
+		.compatible = "usb2109,2817",	/* Via labs VL817 3.1 */
+		.data = (ulong)&vialab_vl817_data,
 	}
 };
 
-- 
2.49.0

[RFC v1 6/7] configs: odorid-c4: Enable Oboard HUB driver

[Anand Moon]

Enable the onboard USB HUB driver to power the
Via labs VL817 USB 3.1 hub on the Odroid C4.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 configs/odroid-c4_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/odroid-c4_defconfig b/configs/odroid-c4_defconfig
index cc3d7fc7484..3c5e53828a1 100644
--- a/configs/odroid-c4_defconfig
+++ b/configs/odroid-c4_defconfig
@@ -64,6 +64,7 @@ CONFIG_USB_DWC3=y
 # CONFIG_USB_DWC3_GADGET is not set
 CONFIG_USB_DWC3_MESON_G12A=y
 CONFIG_USB_KEYBOARD=y
+CONFIG_USB_ONBOARD_HUB=y
 CONFIG_USB_GADGET=y
 CONFIG_USB_GADGET_VENDOR_NUM=0x1b8e
 CONFIG_USB_GADGET_PRODUCT_NUM=0xfada
-- 
2.49.0

[RFC v1 7/7] usb: onboard-hub: Add conditional compilation for I2C initialization

[Anand Moon]

Add conditional compilation for the usb5744_i2c_init() function based
on the CONFIG_DM_I2C configuration, to avoid compilation failure.

  CC      net/net-common.o
  AR      net/built-in.o
  LDS     u-boot.lds
  LD      u-boot
aarch64-linux-gnu-ld.bfd: common/usb_onboard_hub.o: in function `usb5744_i2c_init':
/home/amoon/mainline/u-boot/amlogic/u-boot-maineline/common/usb_onboard_hub.c:74:(.text.usb5744_i2c_init+0xfc): undefined reference to `i2c_get_chip'
aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-maineline/common/usb_onboard_hub.c:89:(.text.usb5744_i2c_init+0x1a0): undefined reference to `dm_i2c_write'
aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-maineline/common/usb_onboard_hub.c:96:(.text.usb5744_i2c_init+0x1c8): undefined reference to `dm_i2c_write'
aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-maineline/common/usb_onboard_hub.c:104:(.text.usb5744_i2c_init+0x1f0): undefined reference to `dm_i2c_write'
Segmentation fault (core dumped)
make: *** [Makefile:1824: u-boot] Error 139
make: *** Deleting file 'u-boot'

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 common/usb_onboard_hub.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
index 1242513c631..92e3f3a92c9 100644
--- a/common/usb_onboard_hub.c
+++ b/common/usb_onboard_hub.c
@@ -30,6 +30,7 @@ struct onboard_hub_data {
 	int (*init)(struct udevice *dev);
 };
 
+#if CONFIG_IS_ENABLED(DM_I2C)
 static int usb5744_i2c_init(struct udevice *dev)
 {
 	/*
@@ -109,6 +110,7 @@ static int usb5744_i2c_init(struct udevice *dev)
 
 	return 0;
 }
+#endif
 
 int usb_onboard_hub_reset(struct udevice *dev)
 {
@@ -222,7 +224,9 @@ static const struct onboard_hub_data usb2514_data = {
 };
 
 static const struct onboard_hub_data usb5744_data = {
+#if CONFIG_IS_ENABLED(DM_I2C)
 	.init = usb5744_i2c_init,
+#endif
 	.power_on_delay_us = 1000,
 	.reset_us = 5,
 };
-- 
2.49.0

Re: [RFC v1 1/7] usb: onboard-hub: Add support for Genesys GL852G hub

[Neil Armstrong]

On 09/05/2025 09:02, Anand Moon wrote:
> Add support for the Genesys GL852G USB2.0 Hub on Odroid C1+
> and Odroid C2. The GL852G driver trigger hub reset signal
> which toggles the gpio.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> Still some issue with USB hub on Odroid C2

What does that mean ?

> => dm tree
> 
>   vidconsole    0  [ + ]   vidconsole0           |   |   `-- vpu@d0100000.vidconsole0
>   display       0  [   ]   meson_dw_hdmi         |   |-- hdmi-tx@c883a000
>   phy           0  [ + ]   meson_gxbb_usb2_phy   |   |-- phy@c0000020
>   usb           0  [ + ]   dwc2_usb              |   `-- usb@c9100000
>   usb_hub       0  [ + ]   usb_hub               |       `-- usb_hub
>   usb_hub       1  [ + ]   usb_hub               |           `-- usb_hub
>   regulator     0  [ + ]   regulator_fixed       |-- regulator-usb-pwrs
> ---
>   common/usb_onboard_hub.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
> index 7fe62b043e6..39bbc1aefa2 100644
> --- a/common/usb_onboard_hub.c
> +++ b/common/usb_onboard_hub.c
> @@ -227,6 +227,10 @@ static const struct onboard_hub_data usb5744_data = {
>   	.reset_us = 5,
>   };
>   
> +static const struct onboard_hub_data genesys_gl852g_data = {
> +	.reset_us = 50,
> +};
> +
>   static const struct udevice_id usb_onboard_hub_ids[] = {
>   	/* Use generic usbVID,PID dt-bindings (usb-device.yaml) */
>   	{	.compatible = "usb424,2514",	/* USB2514B USB 2.0 */
> @@ -237,6 +241,9 @@ static const struct udevice_id usb_onboard_hub_ids[] = {
>   	}, {
>   		.compatible = "usb424,5744",	/* USB5744 USB 3.0 */
>   		.data = (ulong)&usb5744_data,
> +	}, {
> +		.compatible = "usb5e3,610",     /* GL852G USB 2.0 */
> +		.data = (ulong)&genesys_gl852g_data,
>   	}
>   };
>   

Re: [RFC v1 2/7] configs: odorid-c2: Enable Onboard HUB driver

[Neil Armstrong]

s/odorid-c2/odroid-c2/ in the subject

On 09/05/2025 09:02, Anand Moon wrote:
> Enable the onboard USB HUB driver to power the
> USB_HUB_GL852G-OHG on the Odroid C2.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>   configs/odroid-c2_defconfig | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/configs/odroid-c2_defconfig b/configs/odroid-c2_defconfig
> index bcaab46f8bc..f8a66a923c9 100644
> --- a/configs/odroid-c2_defconfig
> +++ b/configs/odroid-c2_defconfig
> @@ -59,6 +59,7 @@ CONFIG_SYSINFO_SMBIOS=y
>   CONFIG_USB=y
>   CONFIG_USB_DWC2=y
>   CONFIG_USB_KEYBOARD=y
> +CONFIG_USB_ONBOARD_HUB=y
>   CONFIG_VIDEO=y
>   # CONFIG_VIDEO_BPP8 is not set
>   # CONFIG_VIDEO_BPP16 is not set

With that fixed:
Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [RFC v1 4/7] configs: odorid-n2: Enable Oboard HUB driver

[Neil Armstrong]

On 09/05/2025 09:02, Anand Moon wrote:
> Enable the onboard USB HUB driver to power the
> USB_HUB_GL853G-OHG on the Odroid N2 and Odroid N2-pluss.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>   configs/odroid-n2_defconfig | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/configs/odroid-n2_defconfig b/configs/odroid-n2_defconfig
> index a8cbaee3c96..1bf687c1e6d 100644
> --- a/configs/odroid-n2_defconfig
> +++ b/configs/odroid-n2_defconfig
> @@ -64,6 +64,7 @@ CONFIG_USB_DWC3=y
>   # CONFIG_USB_DWC3_GADGET is not set
>   CONFIG_USB_DWC3_MESON_G12A=y
>   CONFIG_USB_KEYBOARD=y
> +CONFIG_USB_ONBOARD_HUB=y
>   CONFIG_USB_GADGET=y
>   CONFIG_USB_GADGET_VENDOR_NUM=0x1b8e
>   CONFIG_USB_GADGET_PRODUCT_NUM=0xfada

Same as patch 2, with that fixed:
Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [RFC v1 6/7] configs: odorid-c4: Enable Oboard HUB driver

[Neil Armstrong]

On 09/05/2025 09:02, Anand Moon wrote:
> Enable the onboard USB HUB driver to power the
> Via labs VL817 USB 3.1 hub on the Odroid C4.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>   configs/odroid-c4_defconfig | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/configs/odroid-c4_defconfig b/configs/odroid-c4_defconfig
> index cc3d7fc7484..3c5e53828a1 100644
> --- a/configs/odroid-c4_defconfig
> +++ b/configs/odroid-c4_defconfig
> @@ -64,6 +64,7 @@ CONFIG_USB_DWC3=y
>   # CONFIG_USB_DWC3_GADGET is not set
>   CONFIG_USB_DWC3_MESON_G12A=y
>   CONFIG_USB_KEYBOARD=y
> +CONFIG_USB_ONBOARD_HUB=y
>   CONFIG_USB_GADGET=y
>   CONFIG_USB_GADGET_VENDOR_NUM=0x1b8e
>   CONFIG_USB_GADGET_PRODUCT_NUM=0xfada

Same as patch 2, with that fixed:
Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [RFC v1 7/7] usb: onboard-hub: Add conditional compilation for I2C initialization

[Neil Armstrong]

On 09/05/2025 09:02, Anand Moon wrote:
> Add conditional compilation for the usb5744_i2c_init() function based
> on the CONFIG_DM_I2C configuration, to avoid compilation failure.
> 
>    CC      net/net-common.o
>    AR      net/built-in.o
>    LDS     u-boot.lds
>    LD      u-boot
> aarch64-linux-gnu-ld.bfd: common/usb_onboard_hub.o: in function `usb5744_i2c_init':
> /home/amoon/mainline/u-boot/amlogic/u-boot-maineline/common/usb_onboard_hub.c:74:(.text.usb5744_i2c_init+0xfc): undefined reference to `i2c_get_chip'
> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-maineline/common/usb_onboard_hub.c:89:(.text.usb5744_i2c_init+0x1a0): undefined reference to `dm_i2c_write'
> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-maineline/common/usb_onboard_hub.c:96:(.text.usb5744_i2c_init+0x1c8): undefined reference to `dm_i2c_write'
> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-maineline/common/usb_onboard_hub.c:104:(.text.usb5744_i2c_init+0x1f0): undefined reference to `dm_i2c_write'
> Segmentation fault (core dumped)
> make: *** [Makefile:1824: u-boot] Error 139
> make: *** Deleting file 'u-boot'
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>   common/usb_onboard_hub.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
> index 1242513c631..92e3f3a92c9 100644
> --- a/common/usb_onboard_hub.c
> +++ b/common/usb_onboard_hub.c
> @@ -30,6 +30,7 @@ struct onboard_hub_data {
>   	int (*init)(struct udevice *dev);
>   };
>   
> +#if CONFIG_IS_ENABLED(DM_I2C)
>   static int usb5744_i2c_init(struct udevice *dev)
>   {
>   	/*
> @@ -109,6 +110,7 @@ static int usb5744_i2c_init(struct udevice *dev)
>   
>   	return 0;
>   }
> +#endif
>   
>   int usb_onboard_hub_reset(struct udevice *dev)
>   {
> @@ -222,7 +224,9 @@ static const struct onboard_hub_data usb2514_data = {
>   };
>   
>   static const struct onboard_hub_data usb5744_data = {
> +#if CONFIG_IS_ENABLED(DM_I2C)
>   	.init = usb5744_i2c_init,
> +#endif
>   	.power_on_delay_us = 1000,
>   	.reset_us = 5,
>   };

I think you should completely disable usb5744_data in this case

Neil

Re: [RFC v1 5/7] usb: onboard-hub: Add support for VL817 USB hub

[Neil Armstrong]

On 09/05/2025 09:02, Anand Moon wrote:
> Enable support for the Via labs VL817 USB2.0 and USB3.1 Hub
> on the Odroid C4. The Via labs VL817 USB driver activates the
> hub reset signal, which toggles the GPIO.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> => dm tree
>   simple_bus   11  [ + ]   dwc3-meson-g12a       |   `-- usb@ffe09000
>   usb_gadget    0  [   ]   dwc2-udc-otg          |       |-- usb@ff400000
>   usb           0  [ + ]   xhci-dwc3             |       `-- usb@ff500000
>   usb_hub       0  [ + ]   usb_hub               |           `-- usb_hub
>   usb_hub       1  [ + ]   usb_hub               |               |-- usb_hub
>   usb_mass_s    0  [ + ]   usb_mass_storage      |               |   `-- usb_mass_storage
>   blk           2  [ + ]   usb_storage_blk       |               |       |-- usb_mass_storage.lun0
>   partition     0  [ + ]   blk_partition         |               |       |   |-- usb_mass_storage.lun0:1
>   partition     1  [ + ]   blk_partition         |               |       |   |-- usb_mass_storage.lun0:2
>   partition     2  [ + ]   blk_partition         |               |       |   |-- usb_mass_storage.lun0:3
>   partition     3  [ + ]   blk_partition         |               |       |   `-- usb_mass_storage.lun0:4
>   bootdev       3  [   ]   usb_bootdev           |               |       `-- usb_mass_storage.lun0.bootdev
>   usb_hub       2  [ + ]   usb_hub               |               `-- usb_hub
> ---
>   common/usb_onboard_hub.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
> index 3914e2f773d..1242513c631 100644
> --- a/common/usb_onboard_hub.c
> +++ b/common/usb_onboard_hub.c
> @@ -235,6 +235,10 @@ static const struct onboard_hub_data genesys_gl853g_data = {
>   	.reset_us = 50,
>   };
>   
> +static const struct onboard_hub_data vialab_vl817_data = {
> +	.reset_us = 10,
> +};
> +
>   static const struct udevice_id usb_onboard_hub_ids[] = {
>   	/* Use generic usbVID,PID dt-bindings (usb-device.yaml) */
>   	{	.compatible = "usb424,2514",	/* USB2514B USB 2.0 */
> @@ -251,6 +255,12 @@ static const struct udevice_id usb_onboard_hub_ids[] = {
>   	}, {
>   		.compatible = "usb5e3,620",     /* GL852G USB 3.1 */
>   		.data = (ulong)&genesys_gl853g_data,
> +	}, {
> +		.compatible = "usb2109,817",	/* Via labs VL817 2.0 */
> +		.data = (ulong)&vialab_vl817_data,
> +	}, {
> +		.compatible = "usb2109,2817",	/* Via labs VL817 3.1 */
> +		.data = (ulong)&vialab_vl817_data,
>   	}
>   };
>   

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [RFC v1 3/7] usb: onboard-hub: Add support for Genesys GL853G

[Neil Armstrong]

On 09/05/2025 09:02, Anand Moon wrote:
> Enable support for the Genesys GL853G USB2.0 and USB3.1 Hub on
> the Odroid N2 and N2-plus. The GL853G driver activates the hub
> reset signal, which toggles the GPIO.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> => dm tree
> 
>   simple_bus   11  [ + ]   dwc3-meson-g12a       |   `-- usb@ffe09000
>   usb_gadget    0  [   ]   dwc2-udc-otg          |       |-- usb@ff400000
>   usb           0  [ + ]   xhci-dwc3             |       `-- usb@ff500000
>   usb_hub       0  [ + ]   usb_hub               |           `-- usb_hub
>   usb_hub       1  [ + ]   usb_hub               |               |-- usb_hub
>   usb_mass_s    0  [ + ]   usb_mass_storage      |               |   `-- usb_mass_storage
>   blk           2  [ + ]   usb_storage_blk       |               |       |-- usb_mass_storage.lun0
>   partition     0  [ + ]   blk_partition         |               |       |   `-- usb_mass_storage.lun0:1
>   bootdev       3  [   ]   usb_bootdev           |               |       `-- usb_mass_storage.lun0.bootdev
>   usb_hub       2  [ + ]   usb_hub               |               `-- usb_hub
>   clk           2  [ + ]   fixed_clock           |-- xtal-clk
> ---
>   common/usb_onboard_hub.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
> index 39bbc1aefa2..3914e2f773d 100644
> --- a/common/usb_onboard_hub.c
> +++ b/common/usb_onboard_hub.c
> @@ -231,6 +231,10 @@ static const struct onboard_hub_data genesys_gl852g_data = {
>   	.reset_us = 50,
>   };
>   
> +static const struct onboard_hub_data genesys_gl853g_data = {
> +	.reset_us = 50,
> +};
> +
>   static const struct udevice_id usb_onboard_hub_ids[] = {
>   	/* Use generic usbVID,PID dt-bindings (usb-device.yaml) */
>   	{	.compatible = "usb424,2514",	/* USB2514B USB 2.0 */
> @@ -244,6 +248,9 @@ static const struct udevice_id usb_onboard_hub_ids[] = {
>   	}, {
>   		.compatible = "usb5e3,610",     /* GL852G USB 2.0 */
>   		.data = (ulong)&genesys_gl852g_data,
> +	}, {
> +		.compatible = "usb5e3,620",     /* GL852G USB 3.1 */
> +		.data = (ulong)&genesys_gl853g_data,
>   	}
>   };
>   

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [RFC v1 2/7] configs: odorid-c2: Enable Onboard HUB driver

[Anand Moon]

Hi Neil,

On Fri, 9 May 2025 at 13:26, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>
> s/odorid-c2/odroid-c2/ in the subject
>
Oops, I will fix this in the next version.
> On 09/05/2025 09:02, Anand Moon wrote:
> > Enable the onboard USB HUB driver to power the
> > USB_HUB_GL852G-OHG on the Odroid C2.
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> >   configs/odroid-c2_defconfig | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/configs/odroid-c2_defconfig b/configs/odroid-c2_defconfig
> > index bcaab46f8bc..f8a66a923c9 100644
> > --- a/configs/odroid-c2_defconfig
> > +++ b/configs/odroid-c2_defconfig
> > @@ -59,6 +59,7 @@ CONFIG_SYSINFO_SMBIOS=y
> >   CONFIG_USB=y
> >   CONFIG_USB_DWC2=y
> >   CONFIG_USB_KEYBOARD=y
> > +CONFIG_USB_ONBOARD_HUB=y
> >   CONFIG_VIDEO=y
> >   # CONFIG_VIDEO_BPP8 is not set
> >   # CONFIG_VIDEO_BPP16 is not set
>
> With that fixed:
> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Thanks
-Anand

Re: [RFC v1 7/7] usb: onboard-hub: Add conditional compilation for I2C initialization

[Anand Moon]

Hi Neil,

On Fri, 9 May 2025 at 13:27, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>
> On 09/05/2025 09:02, Anand Moon wrote:
> > Add conditional compilation for the usb5744_i2c_init() function based
> > on the CONFIG_DM_I2C configuration, to avoid compilation failure.
> >
> >    CC      net/net-common.o
> >    AR      net/built-in.o
> >    LDS     u-boot.lds
> >    LD      u-boot
> > aarch64-linux-gnu-ld.bfd: common/usb_onboard_hub.o: in function `usb5744_i2c_init':
> > /home/amoon/mainline/u-boot/amlogic/u-boot-maineline/common/usb_onboard_hub.c:74:(.text.usb5744_i2c_init+0xfc): undefined reference to `i2c_get_chip'
> > aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-maineline/common/usb_onboard_hub.c:89:(.text.usb5744_i2c_init+0x1a0): undefined reference to `dm_i2c_write'
> > aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-maineline/common/usb_onboard_hub.c:96:(.text.usb5744_i2c_init+0x1c8): undefined reference to `dm_i2c_write'
> > aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-maineline/common/usb_onboard_hub.c:104:(.text.usb5744_i2c_init+0x1f0): undefined reference to `dm_i2c_write'
> > Segmentation fault (core dumped)
> > make: *** [Makefile:1824: u-boot] Error 139
> > make: *** Deleting file 'u-boot'
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> >   common/usb_onboard_hub.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
> > index 1242513c631..92e3f3a92c9 100644
> > --- a/common/usb_onboard_hub.c
> > +++ b/common/usb_onboard_hub.c
> > @@ -30,6 +30,7 @@ struct onboard_hub_data {
> >       int (*init)(struct udevice *dev);
> >   };
> >
> > +#if CONFIG_IS_ENABLED(DM_I2C)
> >   static int usb5744_i2c_init(struct udevice *dev)
> >   {
> >       /*
> > @@ -109,6 +110,7 @@ static int usb5744_i2c_init(struct udevice *dev)
> >
> >       return 0;
> >   }
> > +#endif
> >
> >   int usb_onboard_hub_reset(struct udevice *dev)
> >   {
> > @@ -222,7 +224,9 @@ static const struct onboard_hub_data usb2514_data = {
> >   };
> >
> >   static const struct onboard_hub_data usb5744_data = {
> > +#if CONFIG_IS_ENABLED(DM_I2C)
> >       .init = usb5744_i2c_init,
> > +#endif
> >       .power_on_delay_us = 1000,
> >       .reset_us = 5,
> >   };
>
> I think you should completely disable usb5744_data in this case
>
Let me check this. I will update it in the next version.
> Neil

Thanks
-Anand

Re: [RFC v1 7/7] usb: onboard-hub: Add conditional compilation for I2C initialization

[Quentin Schulz]

Hi Neil, Anand,

On 5/9/25 9:57 AM, Neil Armstrong wrote:
> On 09/05/2025 09:02, Anand Moon wrote:
>> Add conditional compilation for the usb5744_i2c_init() function based
>> on the CONFIG_DM_I2C configuration, to avoid compilation failure.
>>
>>    CC      net/net-common.o
>>    AR      net/built-in.o
>>    LDS     u-boot.lds
>>    LD      u-boot
>> aarch64-linux-gnu-ld.bfd: common/usb_onboard_hub.o: in function 
>> `usb5744_i2c_init':
>> /home/amoon/mainline/u-boot/amlogic/u-boot-maineline/common/ 
>> usb_onboard_hub.c:74:(.text.usb5744_i2c_init+0xfc): undefined 
>> reference to `i2c_get_chip'
>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot- 
>> maineline/common/usb_onboard_hub.c:89:(.text.usb5744_i2c_init+0x1a0): 
>> undefined reference to `dm_i2c_write'
>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot- 
>> maineline/common/usb_onboard_hub.c:96:(.text.usb5744_i2c_init+0x1c8): 
>> undefined reference to `dm_i2c_write'
>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot- 
>> maineline/common/usb_onboard_hub.c:104:(.text.usb5744_i2c_init+0x1f0): 
>> undefined reference to `dm_i2c_write'
>> Segmentation fault (core dumped)
>> make: *** [Makefile:1824: u-boot] Error 139
>> make: *** Deleting file 'u-boot'
>>
>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>> ---
>>   common/usb_onboard_hub.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
>> index 1242513c631..92e3f3a92c9 100644
>> --- a/common/usb_onboard_hub.c
>> +++ b/common/usb_onboard_hub.c
>> @@ -30,6 +30,7 @@ struct onboard_hub_data {
>>       int (*init)(struct udevice *dev);
>>   };
>> +#if CONFIG_IS_ENABLED(DM_I2C)
>>   static int usb5744_i2c_init(struct udevice *dev)
>>   {
>>       /*
>> @@ -109,6 +110,7 @@ static int usb5744_i2c_init(struct udevice *dev)
>>       return 0;
>>   }
>> +#endif
>>   int usb_onboard_hub_reset(struct udevice *dev)
>>   {
>> @@ -222,7 +224,9 @@ static const struct onboard_hub_data usb2514_data = {
>>   };
>>   static const struct onboard_hub_data usb5744_data = {
>> +#if CONFIG_IS_ENABLED(DM_I2C)
>>       .init = usb5744_i2c_init,
>> +#endif
>>       .power_on_delay_us = 1000,
>>       .reset_us = 5,
>>   };
> 
> I think you should completely disable usb5744_data in this case
> 

I would recommend to go the same route as the Linux kernel which has a 
special Kconfig option for it, c.f. USB_ONBOARD_DEV_USB5744 which adds 
the necessary dependencies.

We probably should be careful and figure out which boards are actually 
using the usb5744 and add this symbol to their defconfig so they don't 
regress.

Cheers,
Quentin

Re: [RFC v1 7/7] usb: onboard-hub: Add conditional compilation for I2C initialization

[Anand Moon]

Hi Quentin,

On Fri, 9 May 2025 at 17:21, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Neil, Anand,
>
> On 5/9/25 9:57 AM, Neil Armstrong wrote:
> > On 09/05/2025 09:02, Anand Moon wrote:
> >> Add conditional compilation for the usb5744_i2c_init() function based
> >> on the CONFIG_DM_I2C configuration, to avoid compilation failure.
> >>
> >>    CC      net/net-common.o
> >>    AR      net/built-in.o
> >>    LDS     u-boot.lds
> >>    LD      u-boot
> >> aarch64-linux-gnu-ld.bfd: common/usb_onboard_hub.o: in function
> >> `usb5744_i2c_init':
> >> /home/amoon/mainline/u-boot/amlogic/u-boot-maineline/common/
> >> usb_onboard_hub.c:74:(.text.usb5744_i2c_init+0xfc): undefined
> >> reference to `i2c_get_chip'
> >> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
> >> maineline/common/usb_onboard_hub.c:89:(.text.usb5744_i2c_init+0x1a0):
> >> undefined reference to `dm_i2c_write'
> >> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
> >> maineline/common/usb_onboard_hub.c:96:(.text.usb5744_i2c_init+0x1c8):
> >> undefined reference to `dm_i2c_write'
> >> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
> >> maineline/common/usb_onboard_hub.c:104:(.text.usb5744_i2c_init+0x1f0):
> >> undefined reference to `dm_i2c_write'
> >> Segmentation fault (core dumped)
> >> make: *** [Makefile:1824: u-boot] Error 139
> >> make: *** Deleting file 'u-boot'
> >>
> >> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> >> ---
> >>   common/usb_onboard_hub.c | 4 ++++
> >>   1 file changed, 4 insertions(+)
> >>
> >> diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
> >> index 1242513c631..92e3f3a92c9 100644
> >> --- a/common/usb_onboard_hub.c
> >> +++ b/common/usb_onboard_hub.c
> >> @@ -30,6 +30,7 @@ struct onboard_hub_data {
> >>       int (*init)(struct udevice *dev);
> >>   };
> >> +#if CONFIG_IS_ENABLED(DM_I2C)
> >>   static int usb5744_i2c_init(struct udevice *dev)
> >>   {
> >>       /*
> >> @@ -109,6 +110,7 @@ static int usb5744_i2c_init(struct udevice *dev)
> >>       return 0;
> >>   }
> >> +#endif
> >>   int usb_onboard_hub_reset(struct udevice *dev)
> >>   {
> >> @@ -222,7 +224,9 @@ static const struct onboard_hub_data usb2514_data = {
> >>   };
> >>   static const struct onboard_hub_data usb5744_data = {
> >> +#if CONFIG_IS_ENABLED(DM_I2C)
> >>       .init = usb5744_i2c_init,
> >> +#endif
> >>       .power_on_delay_us = 1000,
> >>       .reset_us = 5,
> >>   };
> >
> > I think you should completely disable usb5744_data in this case
> >
>
> I would recommend to go the same route as the Linux kernel which has a
> special Kconfig option for it, c.f. USB_ONBOARD_DEV_USB5744 which adds
> the necessary dependencies.
>
On Linux kernel, it needs to parse the device node with i2c-bus.

> We probably should be careful and figure out which boards are actually
> using the usb5744 and add this symbol to their defconfig so they don't
> regress.
>
From my initial analysis configs/xilinx_zynqmp_kria_defconfig
which use CONFIG_USB_ONBOARD_HUB
I believe using DM_I2C to guard the code is the best approach.
it should be around .init() callback to guard this to work.

> Cheers,
> Quentin
Thanks
-Anand

Re: [RFC v1 7/7] usb: onboard-hub: Add conditional compilation for I2C initialization

[Quentin Schulz]

Hi Anand,

On 5/9/25 7:45 PM, Anand Moon wrote:
> Hi Quentin,
> 
> On Fri, 9 May 2025 at 17:21, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>
>> Hi Neil, Anand,
>>
>> On 5/9/25 9:57 AM, Neil Armstrong wrote:
>>> On 09/05/2025 09:02, Anand Moon wrote:
>>>> Add conditional compilation for the usb5744_i2c_init() function based
>>>> on the CONFIG_DM_I2C configuration, to avoid compilation failure.
>>>>
>>>>     CC      net/net-common.o
>>>>     AR      net/built-in.o
>>>>     LDS     u-boot.lds
>>>>     LD      u-boot
>>>> aarch64-linux-gnu-ld.bfd: common/usb_onboard_hub.o: in function
>>>> `usb5744_i2c_init':
>>>> /home/amoon/mainline/u-boot/amlogic/u-boot-maineline/common/
>>>> usb_onboard_hub.c:74:(.text.usb5744_i2c_init+0xfc): undefined
>>>> reference to `i2c_get_chip'
>>>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
>>>> maineline/common/usb_onboard_hub.c:89:(.text.usb5744_i2c_init+0x1a0):
>>>> undefined reference to `dm_i2c_write'
>>>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
>>>> maineline/common/usb_onboard_hub.c:96:(.text.usb5744_i2c_init+0x1c8):
>>>> undefined reference to `dm_i2c_write'
>>>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
>>>> maineline/common/usb_onboard_hub.c:104:(.text.usb5744_i2c_init+0x1f0):
>>>> undefined reference to `dm_i2c_write'
>>>> Segmentation fault (core dumped)
>>>> make: *** [Makefile:1824: u-boot] Error 139
>>>> make: *** Deleting file 'u-boot'
>>>>
>>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>>> ---
>>>>    common/usb_onboard_hub.c | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
>>>> index 1242513c631..92e3f3a92c9 100644
>>>> --- a/common/usb_onboard_hub.c
>>>> +++ b/common/usb_onboard_hub.c
>>>> @@ -30,6 +30,7 @@ struct onboard_hub_data {
>>>>        int (*init)(struct udevice *dev);
>>>>    };
>>>> +#if CONFIG_IS_ENABLED(DM_I2C)
>>>>    static int usb5744_i2c_init(struct udevice *dev)
>>>>    {
>>>>        /*
>>>> @@ -109,6 +110,7 @@ static int usb5744_i2c_init(struct udevice *dev)
>>>>        return 0;
>>>>    }
>>>> +#endif
>>>>    int usb_onboard_hub_reset(struct udevice *dev)
>>>>    {
>>>> @@ -222,7 +224,9 @@ static const struct onboard_hub_data usb2514_data = {
>>>>    };
>>>>    static const struct onboard_hub_data usb5744_data = {
>>>> +#if CONFIG_IS_ENABLED(DM_I2C)
>>>>        .init = usb5744_i2c_init,
>>>> +#endif
>>>>        .power_on_delay_us = 1000,
>>>>        .reset_us = 5,
>>>>    };
>>>
>>> I think you should completely disable usb5744_data in this case
>>>
>>
>> I would recommend to go the same route as the Linux kernel which has a
>> special Kconfig option for it, c.f. USB_ONBOARD_DEV_USB5744 which adds
>> the necessary dependencies.
>>
> On Linux kernel, it needs to parse the device node with i2c-bus.
> 

Seems like it does in U-Boot too? c.f. usb5744_i2c_init() with 
dev_read_phandle_with_args with i2c-bus?

>> We probably should be careful and figure out which boards are actually
>> using the usb5744 and add this symbol to their defconfig so they don't
>> regress.
>>
>  From my initial analysis configs/xilinx_zynqmp_kria_defconfig
> which use CONFIG_USB_ONBOARD_HUB
> I believe using DM_I2C to guard the code is the best approach.
> it should be around .init() callback to guard this to work.
> 

I disagree. The driver clearly supports the USB5744 and the driver is 
compiled regardless of DM_I2C support which is required (as far as I 
understood, maybe that's incorrect?) for being able to use USB5744? I 
don't like hidden dependencies.

Also, I believe Neil's point is we shouldn't try to probe the USB5744 if 
there is no DM_I2C support, simply excluding .init() callback will 
probably just render this "support" of USB5744 non-functional?

Cheers,
Quentin

>> Cheers,
>> Quentin
> Thanks
> -Anand

Re: [RFC v1 7/7] usb: onboard-hub: Add conditional compilation for I2C initialization

[Anand Moon]

Hi Quentin,

On Mon, 12 May 2025 at 12:36, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Anand,
>
> On 5/9/25 7:45 PM, Anand Moon wrote:
> > Hi Quentin,
> >
> > On Fri, 9 May 2025 at 17:21, Quentin Schulz <quentin.schulz@cherry.de> wrote:
> >>
> >> Hi Neil, Anand,
> >>
> >> On 5/9/25 9:57 AM, Neil Armstrong wrote:
> >>> On 09/05/2025 09:02, Anand Moon wrote:
> >>>> Add conditional compilation for the usb5744_i2c_init() function based
> >>>> on the CONFIG_DM_I2C configuration, to avoid compilation failure.
> >>>>
> >>>>     CC      net/net-common.o
> >>>>     AR      net/built-in.o
> >>>>     LDS     u-boot.lds
> >>>>     LD      u-boot
> >>>> aarch64-linux-gnu-ld.bfd: common/usb_onboard_hub.o: in function
> >>>> `usb5744_i2c_init':
> >>>> /home/amoon/mainline/u-boot/amlogic/u-boot-maineline/common/
> >>>> usb_onboard_hub.c:74:(.text.usb5744_i2c_init+0xfc): undefined
> >>>> reference to `i2c_get_chip'
> >>>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
> >>>> maineline/common/usb_onboard_hub.c:89:(.text.usb5744_i2c_init+0x1a0):
> >>>> undefined reference to `dm_i2c_write'
> >>>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
> >>>> maineline/common/usb_onboard_hub.c:96:(.text.usb5744_i2c_init+0x1c8):
> >>>> undefined reference to `dm_i2c_write'
> >>>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
> >>>> maineline/common/usb_onboard_hub.c:104:(.text.usb5744_i2c_init+0x1f0):
> >>>> undefined reference to `dm_i2c_write'
> >>>> Segmentation fault (core dumped)
> >>>> make: *** [Makefile:1824: u-boot] Error 139
> >>>> make: *** Deleting file 'u-boot'
> >>>>
> >>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> >>>> ---
> >>>>    common/usb_onboard_hub.c | 4 ++++
> >>>>    1 file changed, 4 insertions(+)
> >>>>
> >>>> diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
> >>>> index 1242513c631..92e3f3a92c9 100644
> >>>> --- a/common/usb_onboard_hub.c
> >>>> +++ b/common/usb_onboard_hub.c
> >>>> @@ -30,6 +30,7 @@ struct onboard_hub_data {
> >>>>        int (*init)(struct udevice *dev);
> >>>>    };
> >>>> +#if CONFIG_IS_ENABLED(DM_I2C)
> >>>>    static int usb5744_i2c_init(struct udevice *dev)
> >>>>    {
> >>>>        /*
> >>>> @@ -109,6 +110,7 @@ static int usb5744_i2c_init(struct udevice *dev)
> >>>>        return 0;
> >>>>    }
> >>>> +#endif
> >>>>    int usb_onboard_hub_reset(struct udevice *dev)
> >>>>    {
> >>>> @@ -222,7 +224,9 @@ static const struct onboard_hub_data usb2514_data = {
> >>>>    };
> >>>>    static const struct onboard_hub_data usb5744_data = {
> >>>> +#if CONFIG_IS_ENABLED(DM_I2C)
> >>>>        .init = usb5744_i2c_init,
> >>>> +#endif
> >>>>        .power_on_delay_us = 1000,
> >>>>        .reset_us = 5,
> >>>>    };
> >>>
> >>> I think you should completely disable usb5744_data in this case
> >>>
> >>
> >> I would recommend to go the same route as the Linux kernel which has a
> >> special Kconfig option for it, c.f. USB_ONBOARD_DEV_USB5744 which adds
> >> the necessary dependencies.
> >>
> > On Linux kernel, it needs to parse the device node with i2c-bus.
> >
>
> Seems like it does in U-Boot too? c.f. usb5744_i2c_init() with
> dev_read_phandle_with_args with i2c-bus?
>
ok.
> >> We probably should be careful and figure out which boards are actually
> >> using the usb5744 and add this symbol to their defconfig so they don't
> >> regress.
> >>
> >  From my initial analysis configs/xilinx_zynqmp_kria_defconfig
> > which use CONFIG_USB_ONBOARD_HUB
> > I believe using DM_I2C to guard the code is the best approach.
> > it should be around .init() callback to guard this to work.
> >
>
> I disagree. The driver clearly supports the USB5744 and the driver is
> compiled regardless of DM_I2C support which is required (as far as I
> understood, maybe that's incorrect?) for being able to use USB5744? I
> don't like hidden dependencies.
>
Okay, I will add a new configuration option to guard the code as done
in the Linux kernel.
> Also, I believe Neil's point is we shouldn't try to probe the USB5744 if
> there is no DM_I2C support, simply excluding .init() callback will
> probably just render this "support" of USB5744 non-functional?
>
Let me debug this more, I will update the next version.
I need to check if this binds correctly to the USB hub reset.
> Cheers,
> Quentin
Thanks
-Anand

Re: [RFC v1 7/7] usb: onboard-hub: Add conditional compilation for I2C initialization

[Anand Moon]

Hi All,

On Mon, 12 May 2025 at 13:51, Anand Moon via groups.io
<linux.amoon=gmail.com@groups.io> wrote:
>
> Hi Quentin,
>
> On Mon, 12 May 2025 at 12:36, Quentin Schulz <quentin.schulz@cherry.de> wrote:
> >
> > Hi Anand,
> >
> > On 5/9/25 7:45 PM, Anand Moon wrote:
> > > Hi Quentin,
> > >
> > > On Fri, 9 May 2025 at 17:21, Quentin Schulz <quentin.schulz@cherry.de> wrote:
> > >>
> > >> Hi Neil, Anand,
> > >>
> > >> On 5/9/25 9:57 AM, Neil Armstrong wrote:
> > >>> On 09/05/2025 09:02, Anand Moon wrote:
> > >>>> Add conditional compilation for the usb5744_i2c_init() function based
> > >>>> on the CONFIG_DM_I2C configuration, to avoid compilation failure.
> > >>>>
> > >>>>     CC      net/net-common.o
> > >>>>     AR      net/built-in.o
> > >>>>     LDS     u-boot.lds
> > >>>>     LD      u-boot
> > >>>> aarch64-linux-gnu-ld.bfd: common/usb_onboard_hub.o: in function
> > >>>> `usb5744_i2c_init':
> > >>>> /home/amoon/mainline/u-boot/amlogic/u-boot-maineline/common/
> > >>>> usb_onboard_hub.c:74:(.text.usb5744_i2c_init+0xfc): undefined
> > >>>> reference to `i2c_get_chip'
> > >>>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
> > >>>> maineline/common/usb_onboard_hub.c:89:(.text.usb5744_i2c_init+0x1a0):
> > >>>> undefined reference to `dm_i2c_write'
> > >>>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
> > >>>> maineline/common/usb_onboard_hub.c:96:(.text.usb5744_i2c_init+0x1c8):
> > >>>> undefined reference to `dm_i2c_write'
> > >>>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
> > >>>> maineline/common/usb_onboard_hub.c:104:(.text.usb5744_i2c_init+0x1f0):
> > >>>> undefined reference to `dm_i2c_write'
> > >>>> Segmentation fault (core dumped)
> > >>>> make: *** [Makefile:1824: u-boot] Error 139
> > >>>> make: *** Deleting file 'u-boot'
> > >>>>
> > >>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > >>>> ---
> > >>>>    common/usb_onboard_hub.c | 4 ++++
> > >>>>    1 file changed, 4 insertions(+)
> > >>>>
> > >>>> diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
> > >>>> index 1242513c631..92e3f3a92c9 100644
> > >>>> --- a/common/usb_onboard_hub.c
> > >>>> +++ b/common/usb_onboard_hub.c
> > >>>> @@ -30,6 +30,7 @@ struct onboard_hub_data {
> > >>>>        int (*init)(struct udevice *dev);
> > >>>>    };
> > >>>> +#if CONFIG_IS_ENABLED(DM_I2C)
> > >>>>    static int usb5744_i2c_init(struct udevice *dev)
> > >>>>    {
> > >>>>        /*
> > >>>> @@ -109,6 +110,7 @@ static int usb5744_i2c_init(struct udevice *dev)
> > >>>>        return 0;
> > >>>>    }
> > >>>> +#endif
> > >>>>    int usb_onboard_hub_reset(struct udevice *dev)
> > >>>>    {
> > >>>> @@ -222,7 +224,9 @@ static const struct onboard_hub_data usb2514_data = {
> > >>>>    };
> > >>>>    static const struct onboard_hub_data usb5744_data = {
> > >>>> +#if CONFIG_IS_ENABLED(DM_I2C)
> > >>>>        .init = usb5744_i2c_init,
> > >>>> +#endif
> > >>>>        .power_on_delay_us = 1000,
> > >>>>        .reset_us = 5,
> > >>>>    };
> > >>>
> > >>> I think you should completely disable usb5744_data in this case
> > >>>
> > >>
> > >> I would recommend to go the same route as the Linux kernel which has a
> > >> special Kconfig option for it, c.f. USB_ONBOARD_DEV_USB5744 which adds
> > >> the necessary dependencies.
> > >>
> > > On Linux kernel, it needs to parse the device node with i2c-bus.
> > >
> >
> > Seems like it does in U-Boot too? c.f. usb5744_i2c_init() with
> > dev_read_phandle_with_args with i2c-bus?
> >
> ok.
> > >> We probably should be careful and figure out which boards are actually
> > >> using the usb5744 and add this symbol to their defconfig so they don't
> > >> regress.
> > >>
> > >  From my initial analysis configs/xilinx_zynqmp_kria_defconfig
> > > which use CONFIG_USB_ONBOARD_HUB
> > > I believe using DM_I2C to guard the code is the best approach.
> > > it should be around .init() callback to guard this to work.
> > >
> >
> > I disagree. The driver clearly supports the USB5744 and the driver is
> > compiled regardless of DM_I2C support which is required (as far as I
> > understood, maybe that's incorrect?) for being able to use USB5744? I
> > don't like hidden dependencies.
> >
> Okay, I will add a new configuration option to guard the code as done
> in the Linux kernel.
> > Also, I believe Neil's point is we shouldn't try to probe the USB5744 if
> > there is no DM_I2C support, simply excluding .init() callback will
> > probably just render this "support" of USB5744 non-functional?
> >
> Let me debug this more, I will update the next version.
> I need to check if this binds correctly to the USB hub reset.

I am trying to get the usb_onboard_hub to bind.
but this driver is not getting probed in the first place,
I am missing some configuration to get this working.

Hit any key to stop autoboot:  0
=> usb start
starting USB...
Register 3000140 NbrPorts 3
Starting the controller
USB XHCI 1.10
Bus usb@ff500000: 4 USB Device(s) found
       scanning usb for storage devices... 1 Storage Device(s) found
=> dm tree
...
mmc           1  [ + ]   meson_gx_mmc          |   |-- mmc@ffe07000
 blk           1  [   ]   mmc_blk               |   |   |-- mmc@ffe07000.blk
 bootdev       2  [   ]   mmc_bootdev           |   |   `-- mmc@ffe07000.bootdev
 simple_bus   11  [ + ]   dwc3-meson-g12a       |   `-- usb@ffe09000
 usb_gadget    0  [   ]   dwc2-udc-otg          |       |-- usb@ff400000
 usb           0  [ + ]   xhci-dwc3             |       `-- usb@ff500000
 usb_hub       0  [ + ]   usb_hub               |           `-- usb_hub
 usb_hub       1  [ + ]   usb_hub               |               |-- usb_hub
 usb_hub       2  [ + ]   usb_hub               |               `-- usb_hub
 usb_mass_s    0  [ + ]   usb_mass_storage      |
`-- usb_mass_storage
 blk           2  [ + ]   usb_storage_blk       |
 |-- usb_mass_storage.lun0
 partition     0  [ + ]   blk_partition         |
 |   `-- usb_mass_storage.lun0:1
 bootdev       3  [   ]   usb_bootdev           |
 `-- usb_mass_storage.lun0.bootdev
 clk           2  [ + ]   fixed_clock           |-- xtal-clk

Thanks
-Anand

Re: [RFC v1 7/7] usb: onboard-hub: Add conditional compilation for I2C initialization

[Quentin Schulz]

Hi Anand,

On 5/13/25 6:14 PM, Anand Moon wrote:
> Hi All,
> 
> On Mon, 12 May 2025 at 13:51, Anand Moon via groups.io
> <linux.amoon=gmail.com@groups.io> wrote:
>>
>> Hi Quentin,
>>
>> On Mon, 12 May 2025 at 12:36, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>>
>>> Hi Anand,
>>>
>>> On 5/9/25 7:45 PM, Anand Moon wrote:
>>>> Hi Quentin,
>>>>
>>>> On Fri, 9 May 2025 at 17:21, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>>>>
>>>>> Hi Neil, Anand,
>>>>>
>>>>> On 5/9/25 9:57 AM, Neil Armstrong wrote:
>>>>>> On 09/05/2025 09:02, Anand Moon wrote:
>>>>>>> Add conditional compilation for the usb5744_i2c_init() function based
>>>>>>> on the CONFIG_DM_I2C configuration, to avoid compilation failure.
>>>>>>>
>>>>>>>      CC      net/net-common.o
>>>>>>>      AR      net/built-in.o
>>>>>>>      LDS     u-boot.lds
>>>>>>>      LD      u-boot
>>>>>>> aarch64-linux-gnu-ld.bfd: common/usb_onboard_hub.o: in function
>>>>>>> `usb5744_i2c_init':
>>>>>>> /home/amoon/mainline/u-boot/amlogic/u-boot-maineline/common/
>>>>>>> usb_onboard_hub.c:74:(.text.usb5744_i2c_init+0xfc): undefined
>>>>>>> reference to `i2c_get_chip'
>>>>>>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
>>>>>>> maineline/common/usb_onboard_hub.c:89:(.text.usb5744_i2c_init+0x1a0):
>>>>>>> undefined reference to `dm_i2c_write'
>>>>>>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
>>>>>>> maineline/common/usb_onboard_hub.c:96:(.text.usb5744_i2c_init+0x1c8):
>>>>>>> undefined reference to `dm_i2c_write'
>>>>>>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
>>>>>>> maineline/common/usb_onboard_hub.c:104:(.text.usb5744_i2c_init+0x1f0):
>>>>>>> undefined reference to `dm_i2c_write'
>>>>>>> Segmentation fault (core dumped)
>>>>>>> make: *** [Makefile:1824: u-boot] Error 139
>>>>>>> make: *** Deleting file 'u-boot'
>>>>>>>
>>>>>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>>>>>> ---
>>>>>>>     common/usb_onboard_hub.c | 4 ++++
>>>>>>>     1 file changed, 4 insertions(+)
>>>>>>>
>>>>>>> diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
>>>>>>> index 1242513c631..92e3f3a92c9 100644
>>>>>>> --- a/common/usb_onboard_hub.c
>>>>>>> +++ b/common/usb_onboard_hub.c
>>>>>>> @@ -30,6 +30,7 @@ struct onboard_hub_data {
>>>>>>>         int (*init)(struct udevice *dev);
>>>>>>>     };
>>>>>>> +#if CONFIG_IS_ENABLED(DM_I2C)
>>>>>>>     static int usb5744_i2c_init(struct udevice *dev)
>>>>>>>     {
>>>>>>>         /*
>>>>>>> @@ -109,6 +110,7 @@ static int usb5744_i2c_init(struct udevice *dev)
>>>>>>>         return 0;
>>>>>>>     }
>>>>>>> +#endif
>>>>>>>     int usb_onboard_hub_reset(struct udevice *dev)
>>>>>>>     {
>>>>>>> @@ -222,7 +224,9 @@ static const struct onboard_hub_data usb2514_data = {
>>>>>>>     };
>>>>>>>     static const struct onboard_hub_data usb5744_data = {
>>>>>>> +#if CONFIG_IS_ENABLED(DM_I2C)
>>>>>>>         .init = usb5744_i2c_init,
>>>>>>> +#endif
>>>>>>>         .power_on_delay_us = 1000,
>>>>>>>         .reset_us = 5,
>>>>>>>     };
>>>>>>
>>>>>> I think you should completely disable usb5744_data in this case
>>>>>>
>>>>>
>>>>> I would recommend to go the same route as the Linux kernel which has a
>>>>> special Kconfig option for it, c.f. USB_ONBOARD_DEV_USB5744 which adds
>>>>> the necessary dependencies.
>>>>>
>>>> On Linux kernel, it needs to parse the device node with i2c-bus.
>>>>
>>>
>>> Seems like it does in U-Boot too? c.f. usb5744_i2c_init() with
>>> dev_read_phandle_with_args with i2c-bus?
>>>
>> ok.
>>>>> We probably should be careful and figure out which boards are actually
>>>>> using the usb5744 and add this symbol to their defconfig so they don't
>>>>> regress.
>>>>>
>>>>   From my initial analysis configs/xilinx_zynqmp_kria_defconfig
>>>> which use CONFIG_USB_ONBOARD_HUB
>>>> I believe using DM_I2C to guard the code is the best approach.
>>>> it should be around .init() callback to guard this to work.
>>>>
>>>
>>> I disagree. The driver clearly supports the USB5744 and the driver is
>>> compiled regardless of DM_I2C support which is required (as far as I
>>> understood, maybe that's incorrect?) for being able to use USB5744? I
>>> don't like hidden dependencies.
>>>
>> Okay, I will add a new configuration option to guard the code as done
>> in the Linux kernel.
>>> Also, I believe Neil's point is we shouldn't try to probe the USB5744 if
>>> there is no DM_I2C support, simply excluding .init() callback will
>>> probably just render this "support" of USB5744 non-functional?
>>>
>> Let me debug this more, I will update the next version.
>> I need to check if this binds correctly to the USB hub reset.
> 
> I am trying to get the usb_onboard_hub to bind.
> but this driver is not getting probed in the first place,
> I am missing some configuration to get this working.
> 

Don't know your setup, but maybe have a look at 
https://lore.kernel.org/u-boot/20250425-usb_onboard_hub_cypress_hx3-v1-1-3cc5a06087cf@thaumatec.com/ 
?

Cheers,
Quentin

Re: [RFC v1 7/7] usb: onboard-hub: Add conditional compilation for I2C initialization

[Anand Moon]

Hi Quentin,

On Tue, 13 May 2025 at 21:52, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi Anand,
>
> On 5/13/25 6:14 PM, Anand Moon wrote:
> > Hi All,
> >
> > On Mon, 12 May 2025 at 13:51, Anand Moon via groups.io
> > <linux.amoon=gmail.com@groups.io> wrote:
> >>
> >> Hi Quentin,
> >>
> >> On Mon, 12 May 2025 at 12:36, Quentin Schulz <quentin.schulz@cherry.de> wrote:
> >>>
> >>> Hi Anand,
> >>>
> >>> On 5/9/25 7:45 PM, Anand Moon wrote:
> >>>> Hi Quentin,
> >>>>
> >>>> On Fri, 9 May 2025 at 17:21, Quentin Schulz <quentin.schulz@cherry.de> wrote:
> >>>>>
> >>>>> Hi Neil, Anand,
> >>>>>
> >>>>> On 5/9/25 9:57 AM, Neil Armstrong wrote:
> >>>>>> On 09/05/2025 09:02, Anand Moon wrote:
> >>>>>>> Add conditional compilation for the usb5744_i2c_init() function based
> >>>>>>> on the CONFIG_DM_I2C configuration, to avoid compilation failure.
> >>>>>>>
> >>>>>>>      CC      net/net-common.o
> >>>>>>>      AR      net/built-in.o
> >>>>>>>      LDS     u-boot.lds
> >>>>>>>      LD      u-boot
> >>>>>>> aarch64-linux-gnu-ld.bfd: common/usb_onboard_hub.o: in function
> >>>>>>> `usb5744_i2c_init':
> >>>>>>> /home/amoon/mainline/u-boot/amlogic/u-boot-maineline/common/
> >>>>>>> usb_onboard_hub.c:74:(.text.usb5744_i2c_init+0xfc): undefined
> >>>>>>> reference to `i2c_get_chip'
> >>>>>>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
> >>>>>>> maineline/common/usb_onboard_hub.c:89:(.text.usb5744_i2c_init+0x1a0):
> >>>>>>> undefined reference to `dm_i2c_write'
> >>>>>>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
> >>>>>>> maineline/common/usb_onboard_hub.c:96:(.text.usb5744_i2c_init+0x1c8):
> >>>>>>> undefined reference to `dm_i2c_write'
> >>>>>>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
> >>>>>>> maineline/common/usb_onboard_hub.c:104:(.text.usb5744_i2c_init+0x1f0):
> >>>>>>> undefined reference to `dm_i2c_write'
> >>>>>>> Segmentation fault (core dumped)
> >>>>>>> make: *** [Makefile:1824: u-boot] Error 139
> >>>>>>> make: *** Deleting file 'u-boot'
> >>>>>>>
> >>>>>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> >>>>>>> ---
> >>>>>>>     common/usb_onboard_hub.c | 4 ++++
> >>>>>>>     1 file changed, 4 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
> >>>>>>> index 1242513c631..92e3f3a92c9 100644
> >>>>>>> --- a/common/usb_onboard_hub.c
> >>>>>>> +++ b/common/usb_onboard_hub.c
> >>>>>>> @@ -30,6 +30,7 @@ struct onboard_hub_data {
> >>>>>>>         int (*init)(struct udevice *dev);
> >>>>>>>     };
> >>>>>>> +#if CONFIG_IS_ENABLED(DM_I2C)
> >>>>>>>     static int usb5744_i2c_init(struct udevice *dev)
> >>>>>>>     {
> >>>>>>>         /*
> >>>>>>> @@ -109,6 +110,7 @@ static int usb5744_i2c_init(struct udevice *dev)
> >>>>>>>         return 0;
> >>>>>>>     }
> >>>>>>> +#endif
> >>>>>>>     int usb_onboard_hub_reset(struct udevice *dev)
> >>>>>>>     {
> >>>>>>> @@ -222,7 +224,9 @@ static const struct onboard_hub_data usb2514_data = {
> >>>>>>>     };
> >>>>>>>     static const struct onboard_hub_data usb5744_data = {
> >>>>>>> +#if CONFIG_IS_ENABLED(DM_I2C)
> >>>>>>>         .init = usb5744_i2c_init,
> >>>>>>> +#endif
> >>>>>>>         .power_on_delay_us = 1000,
> >>>>>>>         .reset_us = 5,
> >>>>>>>     };
> >>>>>>
> >>>>>> I think you should completely disable usb5744_data in this case
> >>>>>>
> >>>>>
> >>>>> I would recommend to go the same route as the Linux kernel which has a
> >>>>> special Kconfig option for it, c.f. USB_ONBOARD_DEV_USB5744 which adds
> >>>>> the necessary dependencies.
> >>>>>
> >>>> On Linux kernel, it needs to parse the device node with i2c-bus.
> >>>>
> >>>
> >>> Seems like it does in U-Boot too? c.f. usb5744_i2c_init() with
> >>> dev_read_phandle_with_args with i2c-bus?
> >>>
> >> ok.
> >>>>> We probably should be careful and figure out which boards are actually
> >>>>> using the usb5744 and add this symbol to their defconfig so they don't
> >>>>> regress.
> >>>>>
> >>>>   From my initial analysis configs/xilinx_zynqmp_kria_defconfig
> >>>> which use CONFIG_USB_ONBOARD_HUB
> >>>> I believe using DM_I2C to guard the code is the best approach.
> >>>> it should be around .init() callback to guard this to work.
> >>>>
> >>>
> >>> I disagree. The driver clearly supports the USB5744 and the driver is
> >>> compiled regardless of DM_I2C support which is required (as far as I
> >>> understood, maybe that's incorrect?) for being able to use USB5744? I
> >>> don't like hidden dependencies.
> >>>
> >> Okay, I will add a new configuration option to guard the code as done
> >> in the Linux kernel.
> >>> Also, I believe Neil's point is we shouldn't try to probe the USB5744 if
> >>> there is no DM_I2C support, simply excluding .init() callback will
> >>> probably just render this "support" of USB5744 non-functional?
> >>>
> >> Let me debug this more, I will update the next version.
> >> I need to check if this binds correctly to the USB hub reset.
> >
> > I am trying to get the usb_onboard_hub to bind.
> > but this driver is not getting probed in the first place,
> > I am missing some configuration to get this working.
> >
>
> Don't know your setup, but maybe have a look at
> https://lore.kernel.org/u-boot/20250425-usb_onboard_hub_cypress_hx3-v1-1-3cc5a06087cf@thaumatec.com/
> ?
Thank you for the tip. I've incorporated all the changes as
suggested in this series. However, I'm still unable to get the
usb_onboard_hub_probe to execute, which is preventing me
from further debugging.
>
> Cheers,
> Quentin
Thanks
-Anand

Re: [RFC v1 7/7] usb: onboard-hub: Add conditional compilation for I2C initialization

[Michal Simek]

On 5/12/25 09:06, Quentin Schulz wrote:
> Hi Anand,
> 
> On 5/9/25 7:45 PM, Anand Moon wrote:
>> Hi Quentin,
>>
>> On Fri, 9 May 2025 at 17:21, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>>
>>> Hi Neil, Anand,
>>>
>>> On 5/9/25 9:57 AM, Neil Armstrong wrote:
>>>> On 09/05/2025 09:02, Anand Moon wrote:
>>>>> Add conditional compilation for the usb5744_i2c_init() function based
>>>>> on the CONFIG_DM_I2C configuration, to avoid compilation failure.
>>>>>
>>>>>     CC      net/net-common.o
>>>>>     AR      net/built-in.o
>>>>>     LDS     u-boot.lds
>>>>>     LD      u-boot
>>>>> aarch64-linux-gnu-ld.bfd: common/usb_onboard_hub.o: in function
>>>>> `usb5744_i2c_init':
>>>>> /home/amoon/mainline/u-boot/amlogic/u-boot-maineline/common/
>>>>> usb_onboard_hub.c:74:(.text.usb5744_i2c_init+0xfc): undefined
>>>>> reference to `i2c_get_chip'
>>>>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
>>>>> maineline/common/usb_onboard_hub.c:89:(.text.usb5744_i2c_init+0x1a0):
>>>>> undefined reference to `dm_i2c_write'
>>>>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
>>>>> maineline/common/usb_onboard_hub.c:96:(.text.usb5744_i2c_init+0x1c8):
>>>>> undefined reference to `dm_i2c_write'
>>>>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
>>>>> maineline/common/usb_onboard_hub.c:104:(.text.usb5744_i2c_init+0x1f0):
>>>>> undefined reference to `dm_i2c_write'
>>>>> Segmentation fault (core dumped)
>>>>> make: *** [Makefile:1824: u-boot] Error 139
>>>>> make: *** Deleting file 'u-boot'
>>>>>
>>>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>>>> ---
>>>>>    common/usb_onboard_hub.c | 4 ++++
>>>>>    1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
>>>>> index 1242513c631..92e3f3a92c9 100644
>>>>> --- a/common/usb_onboard_hub.c
>>>>> +++ b/common/usb_onboard_hub.c
>>>>> @@ -30,6 +30,7 @@ struct onboard_hub_data {
>>>>>        int (*init)(struct udevice *dev);
>>>>>    };
>>>>> +#if CONFIG_IS_ENABLED(DM_I2C)
>>>>>    static int usb5744_i2c_init(struct udevice *dev)
>>>>>    {
>>>>>        /*
>>>>> @@ -109,6 +110,7 @@ static int usb5744_i2c_init(struct udevice *dev)
>>>>>        return 0;
>>>>>    }
>>>>> +#endif
>>>>>    int usb_onboard_hub_reset(struct udevice *dev)
>>>>>    {
>>>>> @@ -222,7 +224,9 @@ static const struct onboard_hub_data usb2514_data = {
>>>>>    };
>>>>>    static const struct onboard_hub_data usb5744_data = {
>>>>> +#if CONFIG_IS_ENABLED(DM_I2C)
>>>>>        .init = usb5744_i2c_init,
>>>>> +#endif
>>>>>        .power_on_delay_us = 1000,
>>>>>        .reset_us = 5,
>>>>>    };
>>>>
>>>> I think you should completely disable usb5744_data in this case
>>>>
>>>
>>> I would recommend to go the same route as the Linux kernel which has a
>>> special Kconfig option for it, c.f. USB_ONBOARD_DEV_USB5744 which adds
>>> the necessary dependencies.
>>>
>> On Linux kernel, it needs to parse the device node with i2c-bus.
>>
> 
> Seems like it does in U-Boot too? c.f. usb5744_i2c_init() with 
> dev_read_phandle_with_args with i2c-bus?
> 
>>> We probably should be careful and figure out which boards are actually
>>> using the usb5744 and add this symbol to their defconfig so they don't
>>> regress.
>>>
>>  From my initial analysis configs/xilinx_zynqmp_kria_defconfig
>> which use CONFIG_USB_ONBOARD_HUB
>> I believe using DM_I2C to guard the code is the best approach.
>> it should be around .init() callback to guard this to work.
>>
> 
> I disagree. The driver clearly supports the USB5744 and the driver is compiled 
> regardless of DM_I2C support which is required (as far as I understood, maybe 
> that's incorrect?) for being able to use USB5744? I don't like hidden dependencies.
> 
> Also, I believe Neil's point is we shouldn't try to probe the USB5744 if there 
> is no DM_I2C support, simply excluding .init() callback will probably just 
> render this "support" of USB5744 non-functional?

USB5744 can be configured differently. We are using two configurations.
1. no initialization over i2c
(but still you can have a need to have driver for reset or power)
2. initialization over i2c to start the hub

And there is also one more configuration via SPI.

It means I don't think DM_I2C should be real dependency on getting usb5744 to 
operate.

Thanks,
Michal

Re: [RFC v1 7/7] usb: onboard-hub: Add conditional compilation for I2C initialization

[Quentin Schulz]

Hi Michal,

On 5/21/25 4:34 PM, Michal Simek wrote:
> 
> 
> On 5/12/25 09:06, Quentin Schulz wrote:
>> Hi Anand,
>>
>> On 5/9/25 7:45 PM, Anand Moon wrote:
>>> Hi Quentin,
>>>
>>> On Fri, 9 May 2025 at 17:21, Quentin Schulz 
>>> <quentin.schulz@cherry.de> wrote:
>>>>
>>>> Hi Neil, Anand,
>>>>
>>>> On 5/9/25 9:57 AM, Neil Armstrong wrote:
>>>>> On 09/05/2025 09:02, Anand Moon wrote:
>>>>>> Add conditional compilation for the usb5744_i2c_init() function based
>>>>>> on the CONFIG_DM_I2C configuration, to avoid compilation failure.
>>>>>>
>>>>>>     CC      net/net-common.o
>>>>>>     AR      net/built-in.o
>>>>>>     LDS     u-boot.lds
>>>>>>     LD      u-boot
>>>>>> aarch64-linux-gnu-ld.bfd: common/usb_onboard_hub.o: in function
>>>>>> `usb5744_i2c_init':
>>>>>> /home/amoon/mainline/u-boot/amlogic/u-boot-maineline/common/
>>>>>> usb_onboard_hub.c:74:(.text.usb5744_i2c_init+0xfc): undefined
>>>>>> reference to `i2c_get_chip'
>>>>>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
>>>>>> maineline/common/usb_onboard_hub.c:89:(.text.usb5744_i2c_init+0x1a0):
>>>>>> undefined reference to `dm_i2c_write'
>>>>>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
>>>>>> maineline/common/usb_onboard_hub.c:96:(.text.usb5744_i2c_init+0x1c8):
>>>>>> undefined reference to `dm_i2c_write'
>>>>>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
>>>>>> maineline/common/usb_onboard_hub.c:104: 
>>>>>> (.text.usb5744_i2c_init+0x1f0):
>>>>>> undefined reference to `dm_i2c_write'
>>>>>> Segmentation fault (core dumped)
>>>>>> make: *** [Makefile:1824: u-boot] Error 139
>>>>>> make: *** Deleting file 'u-boot'
>>>>>>
>>>>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>>>>> ---
>>>>>>    common/usb_onboard_hub.c | 4 ++++
>>>>>>    1 file changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
>>>>>> index 1242513c631..92e3f3a92c9 100644
>>>>>> --- a/common/usb_onboard_hub.c
>>>>>> +++ b/common/usb_onboard_hub.c
>>>>>> @@ -30,6 +30,7 @@ struct onboard_hub_data {
>>>>>>        int (*init)(struct udevice *dev);
>>>>>>    };
>>>>>> +#if CONFIG_IS_ENABLED(DM_I2C)
>>>>>>    static int usb5744_i2c_init(struct udevice *dev)
>>>>>>    {
>>>>>>        /*
>>>>>> @@ -109,6 +110,7 @@ static int usb5744_i2c_init(struct udevice *dev)
>>>>>>        return 0;
>>>>>>    }
>>>>>> +#endif
>>>>>>    int usb_onboard_hub_reset(struct udevice *dev)
>>>>>>    {
>>>>>> @@ -222,7 +224,9 @@ static const struct onboard_hub_data 
>>>>>> usb2514_data = {
>>>>>>    };
>>>>>>    static const struct onboard_hub_data usb5744_data = {
>>>>>> +#if CONFIG_IS_ENABLED(DM_I2C)
>>>>>>        .init = usb5744_i2c_init,
>>>>>> +#endif
>>>>>>        .power_on_delay_us = 1000,
>>>>>>        .reset_us = 5,
>>>>>>    };
>>>>>
>>>>> I think you should completely disable usb5744_data in this case
>>>>>
>>>>
>>>> I would recommend to go the same route as the Linux kernel which has a
>>>> special Kconfig option for it, c.f. USB_ONBOARD_DEV_USB5744 which adds
>>>> the necessary dependencies.
>>>>
>>> On Linux kernel, it needs to parse the device node with i2c-bus.
>>>
>>
>> Seems like it does in U-Boot too? c.f. usb5744_i2c_init() with 
>> dev_read_phandle_with_args with i2c-bus?
>>
>>>> We probably should be careful and figure out which boards are actually
>>>> using the usb5744 and add this symbol to their defconfig so they don't
>>>> regress.
>>>>
>>>  From my initial analysis configs/xilinx_zynqmp_kria_defconfig
>>> which use CONFIG_USB_ONBOARD_HUB
>>> I believe using DM_I2C to guard the code is the best approach.
>>> it should be around .init() callback to guard this to work.
>>>
>>
>> I disagree. The driver clearly supports the USB5744 and the driver is 
>> compiled regardless of DM_I2C support which is required (as far as I 
>> understood, maybe that's incorrect?) for being able to use USB5744? I 
>> don't like hidden dependencies.
>>
>> Also, I believe Neil's point is we shouldn't try to probe the USB5744 
>> if there is no DM_I2C support, simply excluding .init() callback will 
>> probably just render this "support" of USB5744 non-functional?
> 
> USB5744 can be configured differently. We are using two configurations.
> 1. no initialization over i2c
> (but still you can have a need to have driver for reset or power)

Fair enough, some hubs are configured "properly" by default so don't 
necessarily need an interface. Such is the case for the Cypress HX3 we 
use on RK3399 Puma for example. The default is enough so we don't need 
the i2c interface even though it's exposed by the IC. Thanks for the 
reminder.

> 2. initialization over i2c to start the hub
> 
> And there is also one more configuration via SPI.
> 
> It means I don't think DM_I2C should be real dependency on getting 
> usb5744 to operate.
> 

Would diverge from how it's handled in the kernel (today) though.

Can we please follow the same logic as in the kernel though? Meaning we 
should report some message if the i2c-bus property is present but DM_I2C 
is not enabled, in which case it's likely a misconfiguration of the 
bootloader.

Cheers,
Quentin

Re: [RFC v1 7/7] usb: onboard-hub: Add conditional compilation for I2C initialization

[Michal Simek]

On 5/21/25 17:18, Quentin Schulz wrote:
> Hi Michal,
> 
> On 5/21/25 4:34 PM, Michal Simek wrote:
>>
>>
>> On 5/12/25 09:06, Quentin Schulz wrote:
>>> Hi Anand,
>>>
>>> On 5/9/25 7:45 PM, Anand Moon wrote:
>>>> Hi Quentin,
>>>>
>>>> On Fri, 9 May 2025 at 17:21, Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>>>>
>>>>> Hi Neil, Anand,
>>>>>
>>>>> On 5/9/25 9:57 AM, Neil Armstrong wrote:
>>>>>> On 09/05/2025 09:02, Anand Moon wrote:
>>>>>>> Add conditional compilation for the usb5744_i2c_init() function based
>>>>>>> on the CONFIG_DM_I2C configuration, to avoid compilation failure.
>>>>>>>
>>>>>>>     CC      net/net-common.o
>>>>>>>     AR      net/built-in.o
>>>>>>>     LDS     u-boot.lds
>>>>>>>     LD      u-boot
>>>>>>> aarch64-linux-gnu-ld.bfd: common/usb_onboard_hub.o: in function
>>>>>>> `usb5744_i2c_init':
>>>>>>> /home/amoon/mainline/u-boot/amlogic/u-boot-maineline/common/
>>>>>>> usb_onboard_hub.c:74:(.text.usb5744_i2c_init+0xfc): undefined
>>>>>>> reference to `i2c_get_chip'
>>>>>>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
>>>>>>> maineline/common/usb_onboard_hub.c:89:(.text.usb5744_i2c_init+0x1a0):
>>>>>>> undefined reference to `dm_i2c_write'
>>>>>>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
>>>>>>> maineline/common/usb_onboard_hub.c:96:(.text.usb5744_i2c_init+0x1c8):
>>>>>>> undefined reference to `dm_i2c_write'
>>>>>>> aarch64-linux-gnu-ld.bfd: /home/amoon/mainline/u-boot/amlogic/u-boot-
>>>>>>> maineline/common/usb_onboard_hub.c:104: (.text.usb5744_i2c_init+0x1f0):
>>>>>>> undefined reference to `dm_i2c_write'
>>>>>>> Segmentation fault (core dumped)
>>>>>>> make: *** [Makefile:1824: u-boot] Error 139
>>>>>>> make: *** Deleting file 'u-boot'
>>>>>>>
>>>>>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>>>>>> ---
>>>>>>>    common/usb_onboard_hub.c | 4 ++++
>>>>>>>    1 file changed, 4 insertions(+)
>>>>>>>
>>>>>>> diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
>>>>>>> index 1242513c631..92e3f3a92c9 100644
>>>>>>> --- a/common/usb_onboard_hub.c
>>>>>>> +++ b/common/usb_onboard_hub.c
>>>>>>> @@ -30,6 +30,7 @@ struct onboard_hub_data {
>>>>>>>        int (*init)(struct udevice *dev);
>>>>>>>    };
>>>>>>> +#if CONFIG_IS_ENABLED(DM_I2C)
>>>>>>>    static int usb5744_i2c_init(struct udevice *dev)
>>>>>>>    {
>>>>>>>        /*
>>>>>>> @@ -109,6 +110,7 @@ static int usb5744_i2c_init(struct udevice *dev)
>>>>>>>        return 0;
>>>>>>>    }
>>>>>>> +#endif
>>>>>>>    int usb_onboard_hub_reset(struct udevice *dev)
>>>>>>>    {
>>>>>>> @@ -222,7 +224,9 @@ static const struct onboard_hub_data usb2514_data = {
>>>>>>>    };
>>>>>>>    static const struct onboard_hub_data usb5744_data = {
>>>>>>> +#if CONFIG_IS_ENABLED(DM_I2C)
>>>>>>>        .init = usb5744_i2c_init,
>>>>>>> +#endif
>>>>>>>        .power_on_delay_us = 1000,
>>>>>>>        .reset_us = 5,
>>>>>>>    };
>>>>>>
>>>>>> I think you should completely disable usb5744_data in this case
>>>>>>
>>>>>
>>>>> I would recommend to go the same route as the Linux kernel which has a
>>>>> special Kconfig option for it, c.f. USB_ONBOARD_DEV_USB5744 which adds
>>>>> the necessary dependencies.
>>>>>
>>>> On Linux kernel, it needs to parse the device node with i2c-bus.
>>>>
>>>
>>> Seems like it does in U-Boot too? c.f. usb5744_i2c_init() with 
>>> dev_read_phandle_with_args with i2c-bus?
>>>
>>>>> We probably should be careful and figure out which boards are actually
>>>>> using the usb5744 and add this symbol to their defconfig so they don't
>>>>> regress.
>>>>>
>>>>  From my initial analysis configs/xilinx_zynqmp_kria_defconfig
>>>> which use CONFIG_USB_ONBOARD_HUB
>>>> I believe using DM_I2C to guard the code is the best approach.
>>>> it should be around .init() callback to guard this to work.
>>>>
>>>
>>> I disagree. The driver clearly supports the USB5744 and the driver is 
>>> compiled regardless of DM_I2C support which is required (as far as I 
>>> understood, maybe that's incorrect?) for being able to use USB5744? I don't 
>>> like hidden dependencies.
>>>
>>> Also, I believe Neil's point is we shouldn't try to probe the USB5744 if 
>>> there is no DM_I2C support, simply excluding .init() callback will probably 
>>> just render this "support" of USB5744 non-functional?
>>
>> USB5744 can be configured differently. We are using two configurations.
>> 1. no initialization over i2c
>> (but still you can have a need to have driver for reset or power)
> 
> Fair enough, some hubs are configured "properly" by default so don't necessarily 
> need an interface. Such is the case for the Cypress HX3 we use on RK3399 Puma 
> for example. The default is enough so we don't need the i2c interface even 
> though it's exposed by the IC. Thanks for the reminder.
> 
>> 2. initialization over i2c to start the hub
>>
>> And there is also one more configuration via SPI.
>>
>> It means I don't think DM_I2C should be real dependency on getting usb5744 to 
>> operate.
>>
> 
> Would diverge from how it's handled in the kernel (today) though.
> 
> Can we please follow the same logic as in the kernel though? Meaning we should 
> report some message if the i2c-bus property is present but DM_I2C is not 
> enabled, in which case it's likely a misconfiguration of the bootloader.

Make sense.

M