[PATCH v11 0/7] Add the USB5744 hub driver as per new DT binding

[PATCH v11 0/7] Add the USB5744 hub driver as per new DT binding

[Venkatesh Yadav Abbarapu]

Add the usb5744/usb2744 hub driver which does the reset gpio toggling
and the i2c initialization sequence.

Tested the USB5744/USB2744 usb hub for usb0, usb1 with the
DT nodes on KR260 board.

Changes in v2:
- Added the power_on_reset_us variable, for post-reset time.
- Removed the DM_REGULATOR ifdef around the regulator API's.
- Rename the i2c_init and fixed the return for the API's.

Changes in v3:
- Rename i2c_init to init.
- Fixed the return values for the dev_read_phandle_with_args API.
- Removed the unneccessary cast uint8_t *.

Changes in v4:
- Fixed the indentation issues.
- Fixed the Reverse xmas tree for indentation.
- Replaced dev_dbg to dev_err in all places.

Changes in v5:
- Add new API for usb_onboard_hub_reset.

Changes in v6:
- Add return for usb_onboard_hub_reset in probe.

Changes in v7:
- Sort the structure onboard_hub_data data.
- Remove the variable gpio_desc * for dm_gpio_free().

Changes in v8:
- Sort the list for usb5744_data.

Changes in v9:
- Added the Reviewer tag for patches 1/4, which got
missed in v8.

Changes in v10:
- Removed the GPIOD_ACTIVE_LOW flag in the devm_gpiod_get_optional.

Changes in v11:
- Added the usb2514_hub_data and updated the reset_us to 1.

Venkatesh Yadav Abbarapu (7):
  usb: onboard-hub: Add reset-gpio support
  usb: onboard-hub: Fix the return values of regulator APIs
  usb: onboard-hub: add support for Microchip USB5744
  usb: onboard-hub: Add i2c initialization for usb5744 hub
  usb: onboard-hub: Bail out if peer hub is already probed
  configs: zynqmp_kria: Enable the USB onboard hub
  arm64: zynqmp: Update the usb5744 hub node as per binding

 arch/arm/dts/zynqmp-sck-kr-g-revA.dtso |  48 ++++++
 arch/arm/dts/zynqmp-sck-kr-g-revB.dtso |  48 ++++++
 arch/arm/dts/zynqmp-sck-kv-g-revA.dtso |  18 +++
 arch/arm/dts/zynqmp-sck-kv-g-revB.dtso |  25 +++-
 common/usb_onboard_hub.c               | 194 ++++++++++++++++++++++++-
 configs/xilinx_zynqmp_kria_defconfig   |   3 +-
 6 files changed, 329 insertions(+), 7 deletions(-)

-- 
2.17.1

[PATCH v11 1/7] usb: onboard-hub: Add reset-gpio support

[Venkatesh Yadav Abbarapu]

As part of the reset, sets the direction of the pin to output before
toggling the pin. Delay of millisecond is added in between low and
high to meet the setup and hold time requirement of the reset.

Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
Reviewed-by: Marek Vasut <marex@denx.de>
---
 common/usb_onboard_hub.c | 41 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
index 68a04ac041..43f4cee40b 100644
--- a/common/usb_onboard_hub.c
+++ b/common/usb_onboard_hub.c
@@ -7,14 +7,50 @@
  * Mostly inspired by Linux kernel v6.1 onboard_usb_hub driver
  */
 
+#include <asm/gpio.h>
 #include <dm.h>
 #include <dm/device_compat.h>
+#include <linux/delay.h>
 #include <power/regulator.h>
 
 struct onboard_hub {
 	struct udevice *vdd;
+	struct gpio_desc *reset_gpio;
 };
 
+struct onboard_hub_data {
+	unsigned long reset_us;
+	unsigned long power_on_delay_us;
+};
+
+int usb_onboard_hub_reset(struct udevice *dev)
+{
+	struct onboard_hub_data *data =
+		(struct onboard_hub_data *)dev_get_driver_data(dev);
+	struct onboard_hub *hub = dev_get_priv(dev);
+	int ret;
+
+	hub->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_IS_OUT);
+
+	/* property is optional, don't return error! */
+	if (!hub->reset_gpio)
+		return 0;
+
+	ret = dm_gpio_set_value(hub->reset_gpio, 1);
+	if (ret)
+		return ret;
+
+	udelay(data->reset_us);
+
+	ret = dm_gpio_set_value(hub->reset_gpio, 0);
+	if (ret)
+		return ret;
+
+	udelay(data->power_on_delay_us);
+
+	return 0;
+}
+
 static int usb_onboard_hub_probe(struct udevice *dev)
 {
 	struct onboard_hub *hub = dev_get_priv(dev);
@@ -30,7 +66,7 @@ static int usb_onboard_hub_probe(struct udevice *dev)
 	if (ret)
 		dev_err(dev, "can't enable vdd-supply: %d\n", ret);
 
-	return ret;
+	return usb_onboard_hub_reset(dev);
 }
 
 static int usb_onboard_hub_remove(struct udevice *dev)
@@ -38,6 +74,9 @@ static int usb_onboard_hub_remove(struct udevice *dev)
 	struct onboard_hub *hub = dev_get_priv(dev);
 	int ret;
 
+	if (hub->reset_gpio)
+		dm_gpio_free(hub->reset_gpio->dev, hub->reset_gpio);
+
 	ret = regulator_set_enable_if_allowed(hub->vdd, false);
 	if (ret)
 		dev_err(dev, "can't disable vdd-supply: %d\n", ret);
-- 
2.17.1

[PATCH v11 2/7] usb: onboard-hub: Fix the return values of regulator APIs

[Venkatesh Yadav Abbarapu]

Don't error out if there is no vdd regulator supply, as these are
optional properties.

Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
Reviewed-by: Marek Vasut <marex@denx.de>
---
 common/usb_onboard_hub.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
index 43f4cee40b..827ecf9b02 100644
--- a/common/usb_onboard_hub.c
+++ b/common/usb_onboard_hub.c
@@ -57,14 +57,18 @@ static int usb_onboard_hub_probe(struct udevice *dev)
 	int ret;
 
 	ret = device_get_supply_regulator(dev, "vdd-supply", &hub->vdd);
-	if (ret) {
+	if (ret && ret != -ENOENT) {
 		dev_err(dev, "can't get vdd-supply: %d\n", ret);
 		return ret;
 	}
 
-	ret = regulator_set_enable_if_allowed(hub->vdd, true);
-	if (ret)
-		dev_err(dev, "can't enable vdd-supply: %d\n", ret);
+	if (hub->vdd) {
+		ret = regulator_set_enable_if_allowed(hub->vdd, true);
+		if (ret && ret != -ENOSYS) {
+			dev_err(dev, "can't enable vdd-supply: %d\n", ret);
+			return ret;
+		}
+	}
 
 	return usb_onboard_hub_reset(dev);
 }
-- 
2.17.1

[PATCH v11 3/7] usb: onboard-hub: add support for Microchip USB5744

[Venkatesh Yadav Abbarapu]

Add support for the Microchip USB5744 USB3.0 and USB2.0 Hub.
The usb5744 driver trigger hub reset signal after soft reset.
The usb5744 hub need to reset after the phy initialization,
which toggles the gpio.
Also update the usb2514 hub_data with the reset delay as 1us.

Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
Reviewed-by: Marek Vasut <marex@denx.de>
---
 common/usb_onboard_hub.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
index 827ecf9b02..1d146eccee 100644
--- a/common/usb_onboard_hub.c
+++ b/common/usb_onboard_hub.c
@@ -88,10 +88,26 @@ static int usb_onboard_hub_remove(struct udevice *dev)
 	return ret;
 }
 
+static const struct onboard_hub_data usb2514_data = {
+	.reset_us = 1,
+};
+
+static const struct onboard_hub_data usb5744_data = {
+	.power_on_delay_us = 10000,
+	.reset_us = 10000,
+};
+
 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 */
-	{ }
+	{	.compatible = "usb424,2514", /* USB2514B USB 2.0 */
+		.data = (ulong)&usb2514_data,
+	}, {
+		.compatible = "usb424,2744", /* USB2744 USB 2.0 */
+		.data = (ulong)&usb5744_data,
+	}, {
+		.compatible = "usb424,5744", /* USB5744 USB 3.0 */
+		.data = (ulong)&usb5744_data,
+	}
 };
 
 U_BOOT_DRIVER(usb_onboard_hub) = {
-- 
2.17.1

Re: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip USB5744

[Marek Vasut]

On 11/12/24 6:07 AM, Venkatesh Yadav Abbarapu wrote:
> Add support for the Microchip USB5744 USB3.0 and USB2.0 Hub.
> The usb5744 driver trigger hub reset signal after soft reset.
> The usb5744 hub need to reset after the phy initialization,
> which toggles the gpio.
> Also update the usb2514 hub_data with the reset delay as 1us.
> 
> Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
> Reviewed-by: Marek Vasut <marex@denx.de>
> ---
>   common/usb_onboard_hub.c | 20 ++++++++++++++++++--
>   1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
> index 827ecf9b02..1d146eccee 100644
> --- a/common/usb_onboard_hub.c
> +++ b/common/usb_onboard_hub.c
> @@ -88,10 +88,26 @@ static int usb_onboard_hub_remove(struct udevice *dev)
>   	return ret;
>   }
>   
> +static const struct onboard_hub_data usb2514_data = {
> +	.reset_us = 1,
> +};
> +
> +static const struct onboard_hub_data usb5744_data = {
> +	.power_on_delay_us = 10000,
> +	.reset_us = 10000,
> +};
> +
>   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 */
> -	{ }
> +	{	.compatible = "usb424,2514", /* USB2514B USB 2.0 */
> +		.data = (ulong)&usb2514_data,
This ^ hub has to be updated in 1/7 , otherwise if only 1/7 is applied 
(e.g. during bisect), this hub reset will be operated out of specification.

Also, looking at the USB2514 datasheet figure 5-3, it seems the hub 
needs t4=500us recovery time in SMBus mode. Does that mean usb2514_data 
.power_on_delay_us = 500 is missing too ?

Also, it seems t_rstio is 5us in USB5477 datasheet figure 10-4 , where 
do these 10000us figures above come from ?

RE: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip USB5744

[Abbarapu, Venkatesh]

Hi,

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Tuesday, November 12, 2024 11:28 AM
> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>; u-boot@lists.denx.de
> Cc: Simek, Michal <michal.simek@amd.com>; fabrice.gasnier@foss.st.com; git
> (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip
> USB5744
> 
> On 11/12/24 6:07 AM, Venkatesh Yadav Abbarapu wrote:
> > Add support for the Microchip USB5744 USB3.0 and USB2.0 Hub.
> > The usb5744 driver trigger hub reset signal after soft reset.
> > The usb5744 hub need to reset after the phy initialization, which
> > toggles the gpio.
> > Also update the usb2514 hub_data with the reset delay as 1us.
> >
> > Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
> > Reviewed-by: Marek Vasut <marex@denx.de>
> > ---
> >   common/usb_onboard_hub.c | 20 ++++++++++++++++++--
> >   1 file changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c index
> > 827ecf9b02..1d146eccee 100644
> > --- a/common/usb_onboard_hub.c
> > +++ b/common/usb_onboard_hub.c
> > @@ -88,10 +88,26 @@ static int usb_onboard_hub_remove(struct udevice *dev)
> >   	return ret;
> >   }
> >
> > +static const struct onboard_hub_data usb2514_data = {
> > +	.reset_us = 1,
> > +};
> > +
> > +static const struct onboard_hub_data usb5744_data = {
> > +	.power_on_delay_us = 10000,
> > +	.reset_us = 10000,
> > +};
> > +
> >   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 */
> > -	{ }
> > +	{	.compatible = "usb424,2514", /* USB2514B USB 2.0 */
> > +		.data = (ulong)&usb2514_data,
> This ^ hub has to be updated in 1/7 , otherwise if only 1/7 is applied (e.g. during
> bisect), this hub reset will be operated out of specification.
> 
> Also, looking at the USB2514 datasheet figure 5-3, it seems the hub needs t4=500us
> recovery time in SMBus mode. Does that mean usb2514_data .power_on_delay_us
> = 500 is missing too ?
> 
> Also, it seems t_rstio is 5us in USB5477 datasheet figure 10-4 , where do these
> 10000us figures above come from ?

We were seeing i2c failures when we update the reset delay and power on delay values mentioned from the datasheet so updated to 10000us, the linux reference is below
https://github.com/torvalds/linux/commit/908f61bedb2c40c6d856bbfd7f870b967a4cb498

Thanks
Venkatesh

Re: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip USB5744

[Marek Vasut]

On 11/12/24 7:42 AM, Abbarapu, Venkatesh wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Marek Vasut <marex@denx.de>
>> Sent: Tuesday, November 12, 2024 11:28 AM
>> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>; u-boot@lists.denx.de
>> Cc: Simek, Michal <michal.simek@amd.com>; fabrice.gasnier@foss.st.com; git
>> (AMD-Xilinx) <git@amd.com>
>> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip
>> USB5744
>>
>> On 11/12/24 6:07 AM, Venkatesh Yadav Abbarapu wrote:
>>> Add support for the Microchip USB5744 USB3.0 and USB2.0 Hub.
>>> The usb5744 driver trigger hub reset signal after soft reset.
>>> The usb5744 hub need to reset after the phy initialization, which
>>> toggles the gpio.
>>> Also update the usb2514 hub_data with the reset delay as 1us.
>>>
>>> Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
>>> Reviewed-by: Marek Vasut <marex@denx.de>
>>> ---
>>>    common/usb_onboard_hub.c | 20 ++++++++++++++++++--
>>>    1 file changed, 18 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c index
>>> 827ecf9b02..1d146eccee 100644
>>> --- a/common/usb_onboard_hub.c
>>> +++ b/common/usb_onboard_hub.c
>>> @@ -88,10 +88,26 @@ static int usb_onboard_hub_remove(struct udevice *dev)
>>>    	return ret;
>>>    }
>>>
>>> +static const struct onboard_hub_data usb2514_data = {
>>> +	.reset_us = 1,
>>> +};
>>> +
>>> +static const struct onboard_hub_data usb5744_data = {
>>> +	.power_on_delay_us = 10000,
>>> +	.reset_us = 10000,
>>> +};
>>> +
>>>    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 */
>>> -	{ }
>>> +	{	.compatible = "usb424,2514", /* USB2514B USB 2.0 */
>>> +		.data = (ulong)&usb2514_data,
>> This ^ hub has to be updated in 1/7 , otherwise if only 1/7 is applied (e.g. during
>> bisect), this hub reset will be operated out of specification.
>>
>> Also, looking at the USB2514 datasheet figure 5-3, it seems the hub needs t4=500us
>> recovery time in SMBus mode. Does that mean usb2514_data .power_on_delay_us
>> = 500 is missing too ?
>>
>> Also, it seems t_rstio is 5us in USB5477 datasheet figure 10-4 , where do these
>> 10000us figures above come from ?
> 
> We were seeing i2c failures when we update the reset delay and power on delay values mentioned from the datasheet so updated to 10000us, the linux reference is below
> https://github.com/torvalds/linux/commit/908f61bedb2c40c6d856bbfd7f870b967a4cb498
Is there a matching delay requirement specified in the USB hub datasheet 
or is this a workaround for some board-specific behavior ?

RE: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip USB5744

[Abbarapu, Venkatesh]

Hi Marek,

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Wednesday, November 13, 2024 1:15 AM
> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>; u-boot@lists.denx.de
> Cc: Simek, Michal <michal.simek@amd.com>; fabrice.gasnier@foss.st.com; git
> (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip
> USB5744
> 
> On 11/12/24 7:42 AM, Abbarapu, Venkatesh wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Marek Vasut <marex@denx.de>
> >> Sent: Tuesday, November 12, 2024 11:28 AM
> >> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>;
> >> u-boot@lists.denx.de
> >> Cc: Simek, Michal <michal.simek@amd.com>;
> >> fabrice.gasnier@foss.st.com; git
> >> (AMD-Xilinx) <git@amd.com>
> >> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for
> >> Microchip
> >> USB5744
> >>
> >> On 11/12/24 6:07 AM, Venkatesh Yadav Abbarapu wrote:
> >>> Add support for the Microchip USB5744 USB3.0 and USB2.0 Hub.
> >>> The usb5744 driver trigger hub reset signal after soft reset.
> >>> The usb5744 hub need to reset after the phy initialization, which
> >>> toggles the gpio.
> >>> Also update the usb2514 hub_data with the reset delay as 1us.
> >>>
> >>> Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
> >>> Reviewed-by: Marek Vasut <marex@denx.de>
> >>> ---
> >>>    common/usb_onboard_hub.c | 20 ++++++++++++++++++--
> >>>    1 file changed, 18 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
> >>> index 827ecf9b02..1d146eccee 100644
> >>> --- a/common/usb_onboard_hub.c
> >>> +++ b/common/usb_onboard_hub.c
> >>> @@ -88,10 +88,26 @@ static int usb_onboard_hub_remove(struct udevice
> *dev)
> >>>    	return ret;
> >>>    }
> >>>
> >>> +static const struct onboard_hub_data usb2514_data = {
> >>> +	.reset_us = 1,
> >>> +};
> >>> +
> >>> +static const struct onboard_hub_data usb5744_data = {
> >>> +	.power_on_delay_us = 10000,
> >>> +	.reset_us = 10000,
> >>> +};
> >>> +
> >>>    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 */
> >>> -	{ }
> >>> +	{	.compatible = "usb424,2514", /* USB2514B USB 2.0 */
> >>> +		.data = (ulong)&usb2514_data,
> >> This ^ hub has to be updated in 1/7 , otherwise if only 1/7 is
> >> applied (e.g. during bisect), this hub reset will be operated out of specification.
> >>
> >> Also, looking at the USB2514 datasheet figure 5-3, it seems the hub
> >> needs t4=500us recovery time in SMBus mode. Does that mean
> >> usb2514_data .power_on_delay_us = 500 is missing too ?
> >>
> >> Also, it seems t_rstio is 5us in USB5477 datasheet figure 10-4 ,
> >> where do these 10000us figures above come from ?
> >
> > We were seeing i2c failures when we update the reset delay and power
> > on delay values mentioned from the datasheet so updated to 10000us,
> > the linux reference is below
> > https://github.com/torvalds/linux/commit/908f61bedb2c40c6d856bbfd7f870
> > b967a4cb498
> Is there a matching delay requirement specified in the USB hub datasheet or is this a
> workaround for some board-specific behavior ?
The matching delay is not specified in the USB5744 hub document, but based on testing on 2 boards with the above-mentioned delay i2c failures were not observed.

Thanks
Venkatesh 

Re: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip USB5744

[Marek Vasut]

On 11/14/24 5:16 AM, Abbarapu, Venkatesh wrote:
> Hi Marek,
> 
>> -----Original Message-----
>> From: Marek Vasut <marex@denx.de>
>> Sent: Wednesday, November 13, 2024 1:15 AM
>> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>; u-boot@lists.denx.de
>> Cc: Simek, Michal <michal.simek@amd.com>; fabrice.gasnier@foss.st.com; git
>> (AMD-Xilinx) <git@amd.com>
>> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip
>> USB5744
>>
>> On 11/12/24 7:42 AM, Abbarapu, Venkatesh wrote:
>>> Hi,
>>>
>>>> -----Original Message-----
>>>> From: Marek Vasut <marex@denx.de>
>>>> Sent: Tuesday, November 12, 2024 11:28 AM
>>>> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>;
>>>> u-boot@lists.denx.de
>>>> Cc: Simek, Michal <michal.simek@amd.com>;
>>>> fabrice.gasnier@foss.st.com; git
>>>> (AMD-Xilinx) <git@amd.com>
>>>> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for
>>>> Microchip
>>>> USB5744
>>>>
>>>> On 11/12/24 6:07 AM, Venkatesh Yadav Abbarapu wrote:
>>>>> Add support for the Microchip USB5744 USB3.0 and USB2.0 Hub.
>>>>> The usb5744 driver trigger hub reset signal after soft reset.
>>>>> The usb5744 hub need to reset after the phy initialization, which
>>>>> toggles the gpio.
>>>>> Also update the usb2514 hub_data with the reset delay as 1us.
>>>>>
>>>>> Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
>>>>> Reviewed-by: Marek Vasut <marex@denx.de>
>>>>> ---
>>>>>     common/usb_onboard_hub.c | 20 ++++++++++++++++++--
>>>>>     1 file changed, 18 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
>>>>> index 827ecf9b02..1d146eccee 100644
>>>>> --- a/common/usb_onboard_hub.c
>>>>> +++ b/common/usb_onboard_hub.c
>>>>> @@ -88,10 +88,26 @@ static int usb_onboard_hub_remove(struct udevice
>> *dev)
>>>>>     	return ret;
>>>>>     }
>>>>>
>>>>> +static const struct onboard_hub_data usb2514_data = {
>>>>> +	.reset_us = 1,
>>>>> +};
>>>>> +
>>>>> +static const struct onboard_hub_data usb5744_data = {
>>>>> +	.power_on_delay_us = 10000,
>>>>> +	.reset_us = 10000,
>>>>> +};
>>>>> +
>>>>>     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 */
>>>>> -	{ }
>>>>> +	{	.compatible = "usb424,2514", /* USB2514B USB 2.0 */
>>>>> +		.data = (ulong)&usb2514_data,
>>>> This ^ hub has to be updated in 1/7 , otherwise if only 1/7 is
>>>> applied (e.g. during bisect), this hub reset will be operated out of specification.
>>>>
>>>> Also, looking at the USB2514 datasheet figure 5-3, it seems the hub
>>>> needs t4=500us recovery time in SMBus mode. Does that mean
>>>> usb2514_data .power_on_delay_us = 500 is missing too ?
>>>>
>>>> Also, it seems t_rstio is 5us in USB5477 datasheet figure 10-4 ,
>>>> where do these 10000us figures above come from ?
>>>
>>> We were seeing i2c failures when we update the reset delay and power
>>> on delay values mentioned from the datasheet so updated to 10000us,
>>> the linux reference is below
>>> https://github.com/torvalds/linux/commit/908f61bedb2c40c6d856bbfd7f870
>>> b967a4cb498
>> Is there a matching delay requirement specified in the USB hub datasheet or is this a
>> workaround for some board-specific behavior ?
> The matching delay is not specified in the USB5744 hub document, but based on testing on 2 boards with the above-mentioned delay i2c failures were not observed.
Is this 10ms a board-specific reset delay ?
Why is it in a generic driver ?

RE: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip USB5744

[Abbarapu, Venkatesh]

Hi,

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Thursday, November 14, 2024 8:37 PM
> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>; u-boot@lists.denx.de
> Cc: Simek, Michal <michal.simek@amd.com>; fabrice.gasnier@foss.st.com; git
> (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip
> USB5744
> 
> On 11/14/24 5:16 AM, Abbarapu, Venkatesh wrote:
> > Hi Marek,
> >
> >> -----Original Message-----
> >> From: Marek Vasut <marex@denx.de>
> >> Sent: Wednesday, November 13, 2024 1:15 AM
> >> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>;
> >> u-boot@lists.denx.de
> >> Cc: Simek, Michal <michal.simek@amd.com>;
> >> fabrice.gasnier@foss.st.com; git
> >> (AMD-Xilinx) <git@amd.com>
> >> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for
> >> Microchip
> >> USB5744
> >>
> >> On 11/12/24 7:42 AM, Abbarapu, Venkatesh wrote:
> >>> Hi,
> >>>
> >>>> -----Original Message-----
> >>>> From: Marek Vasut <marex@denx.de>
> >>>> Sent: Tuesday, November 12, 2024 11:28 AM
> >>>> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>;
> >>>> u-boot@lists.denx.de
> >>>> Cc: Simek, Michal <michal.simek@amd.com>;
> >>>> fabrice.gasnier@foss.st.com; git
> >>>> (AMD-Xilinx) <git@amd.com>
> >>>> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for
> >>>> Microchip
> >>>> USB5744
> >>>>
> >>>> On 11/12/24 6:07 AM, Venkatesh Yadav Abbarapu wrote:
> >>>>> Add support for the Microchip USB5744 USB3.0 and USB2.0 Hub.
> >>>>> The usb5744 driver trigger hub reset signal after soft reset.
> >>>>> The usb5744 hub need to reset after the phy initialization, which
> >>>>> toggles the gpio.
> >>>>> Also update the usb2514 hub_data with the reset delay as 1us.
> >>>>>
> >>>>> Signed-off-by: Venkatesh Yadav Abbarapu
> >>>>> <venkatesh.abbarapu@amd.com>
> >>>>> Reviewed-by: Marek Vasut <marex@denx.de>
> >>>>> ---
> >>>>>     common/usb_onboard_hub.c | 20 ++++++++++++++++++--
> >>>>>     1 file changed, 18 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
> >>>>> index 827ecf9b02..1d146eccee 100644
> >>>>> --- a/common/usb_onboard_hub.c
> >>>>> +++ b/common/usb_onboard_hub.c
> >>>>> @@ -88,10 +88,26 @@ static int usb_onboard_hub_remove(struct
> >>>>> udevice
> >> *dev)
> >>>>>     	return ret;
> >>>>>     }
> >>>>>
> >>>>> +static const struct onboard_hub_data usb2514_data = {
> >>>>> +	.reset_us = 1,
> >>>>> +};
> >>>>> +
> >>>>> +static const struct onboard_hub_data usb5744_data = {
> >>>>> +	.power_on_delay_us = 10000,
> >>>>> +	.reset_us = 10000,
> >>>>> +};
> >>>>> +
> >>>>>     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 */
> >>>>> -	{ }
> >>>>> +	{	.compatible = "usb424,2514", /* USB2514B USB 2.0 */
> >>>>> +		.data = (ulong)&usb2514_data,
> >>>> This ^ hub has to be updated in 1/7 , otherwise if only 1/7 is
> >>>> applied (e.g. during bisect), this hub reset will be operated out of specification.
> >>>>
> >>>> Also, looking at the USB2514 datasheet figure 5-3, it seems the hub
> >>>> needs t4=500us recovery time in SMBus mode. Does that mean
> >>>> usb2514_data .power_on_delay_us = 500 is missing too ?
> >>>>
> >>>> Also, it seems t_rstio is 5us in USB5477 datasheet figure 10-4 ,
> >>>> where do these 10000us figures above come from ?
> >>>
> >>> We were seeing i2c failures when we update the reset delay and power
> >>> on delay values mentioned from the datasheet so updated to 10000us,
> >>> the linux reference is below
> >>> https://github.com/torvalds/linux/commit/908f61bedb2c40c6d856bbfd7f8
> >>> 70
> >>> b967a4cb498
> >> Is there a matching delay requirement specified in the USB hub
> >> datasheet or is this a workaround for some board-specific behavior ?
> > The matching delay is not specified in the USB5744 hub document, but based on
> testing on 2 boards with the above-mentioned delay i2c failures were not observed.
> Is this 10ms a board-specific reset delay ?
> Why is it in a generic driver ?
On our boards we observed i2c failures when we set the reset delay as 5us and power on delay as 1ms as per the USB5744 datasheet. The reason of adding 10ms delay is because of i2c failures and the linux reference is https://github.com/torvalds/linux/commit/908f61bedb2c40c6d856bbfd7f870b967a4cb498
Do you have anything with respect to these delays, as you might have tested on some other boards? If anything, please let me know.

Thanks
Venkatesh

Re: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip USB5744

[Marek Vasut]

On 11/18/24 4:09 PM, Abbarapu, Venkatesh wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Marek Vasut <marex@denx.de>
>> Sent: Thursday, November 14, 2024 8:37 PM
>> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>; u-boot@lists.denx.de
>> Cc: Simek, Michal <michal.simek@amd.com>; fabrice.gasnier@foss.st.com; git
>> (AMD-Xilinx) <git@amd.com>
>> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip
>> USB5744
>>
>> On 11/14/24 5:16 AM, Abbarapu, Venkatesh wrote:
>>> Hi Marek,
>>>
>>>> -----Original Message-----
>>>> From: Marek Vasut <marex@denx.de>
>>>> Sent: Wednesday, November 13, 2024 1:15 AM
>>>> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>;
>>>> u-boot@lists.denx.de
>>>> Cc: Simek, Michal <michal.simek@amd.com>;
>>>> fabrice.gasnier@foss.st.com; git
>>>> (AMD-Xilinx) <git@amd.com>
>>>> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for
>>>> Microchip
>>>> USB5744
>>>>
>>>> On 11/12/24 7:42 AM, Abbarapu, Venkatesh wrote:
>>>>> Hi,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Marek Vasut <marex@denx.de>
>>>>>> Sent: Tuesday, November 12, 2024 11:28 AM
>>>>>> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>;
>>>>>> u-boot@lists.denx.de
>>>>>> Cc: Simek, Michal <michal.simek@amd.com>;
>>>>>> fabrice.gasnier@foss.st.com; git
>>>>>> (AMD-Xilinx) <git@amd.com>
>>>>>> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for
>>>>>> Microchip
>>>>>> USB5744
>>>>>>
>>>>>> On 11/12/24 6:07 AM, Venkatesh Yadav Abbarapu wrote:
>>>>>>> Add support for the Microchip USB5744 USB3.0 and USB2.0 Hub.
>>>>>>> The usb5744 driver trigger hub reset signal after soft reset.
>>>>>>> The usb5744 hub need to reset after the phy initialization, which
>>>>>>> toggles the gpio.
>>>>>>> Also update the usb2514 hub_data with the reset delay as 1us.
>>>>>>>
>>>>>>> Signed-off-by: Venkatesh Yadav Abbarapu
>>>>>>> <venkatesh.abbarapu@amd.com>
>>>>>>> Reviewed-by: Marek Vasut <marex@denx.de>
>>>>>>> ---
>>>>>>>      common/usb_onboard_hub.c | 20 ++++++++++++++++++--
>>>>>>>      1 file changed, 18 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
>>>>>>> index 827ecf9b02..1d146eccee 100644
>>>>>>> --- a/common/usb_onboard_hub.c
>>>>>>> +++ b/common/usb_onboard_hub.c
>>>>>>> @@ -88,10 +88,26 @@ static int usb_onboard_hub_remove(struct
>>>>>>> udevice
>>>> *dev)
>>>>>>>      	return ret;
>>>>>>>      }
>>>>>>>
>>>>>>> +static const struct onboard_hub_data usb2514_data = {
>>>>>>> +	.reset_us = 1,
>>>>>>> +};
>>>>>>> +
>>>>>>> +static const struct onboard_hub_data usb5744_data = {
>>>>>>> +	.power_on_delay_us = 10000,
>>>>>>> +	.reset_us = 10000,
>>>>>>> +};
>>>>>>> +
>>>>>>>      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 */
>>>>>>> -	{ }
>>>>>>> +	{	.compatible = "usb424,2514", /* USB2514B USB 2.0 */
>>>>>>> +		.data = (ulong)&usb2514_data,
>>>>>> This ^ hub has to be updated in 1/7 , otherwise if only 1/7 is
>>>>>> applied (e.g. during bisect), this hub reset will be operated out of specification.
>>>>>>
>>>>>> Also, looking at the USB2514 datasheet figure 5-3, it seems the hub
>>>>>> needs t4=500us recovery time in SMBus mode. Does that mean
>>>>>> usb2514_data .power_on_delay_us = 500 is missing too ?
>>>>>>
>>>>>> Also, it seems t_rstio is 5us in USB5477 datasheet figure 10-4 ,
>>>>>> where do these 10000us figures above come from ?
>>>>>
>>>>> We were seeing i2c failures when we update the reset delay and power
>>>>> on delay values mentioned from the datasheet so updated to 10000us,
>>>>> the linux reference is below
>>>>> https://github.com/torvalds/linux/commit/908f61bedb2c40c6d856bbfd7f8
>>>>> 70
>>>>> b967a4cb498
>>>> Is there a matching delay requirement specified in the USB hub
>>>> datasheet or is this a workaround for some board-specific behavior ?
>>> The matching delay is not specified in the USB5744 hub document, but based on
>> testing on 2 boards with the above-mentioned delay i2c failures were not observed.
>> Is this 10ms a board-specific reset delay ?
>> Why is it in a generic driver ?
> On our boards we observed i2c failures when we set the reset delay as 5us and power on delay as 1ms as per the USB5744 datasheet. The reason of adding 10ms delay is because of i2c failures and the linux reference is https://github.com/torvalds/linux/commit/908f61bedb2c40c6d856bbfd7f870b967a4cb498
> Do you have anything with respect to these delays, as you might have tested on some other boards? If anything, please let me know.
If the 10ms delay is a board specific delay, then this has to be handled 
in a board specific way, not hard-coded in the driver. Probably add some 
new property which specifies the extra board specific reset delay, but 
be sure to run that property by the Linux kernel maintainers too.

RE: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip USB5744

[Abbarapu, Venkatesh]

Hi,

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Monday, November 18, 2024 10:00 PM
> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>; u-boot@lists.denx.de
> Cc: Simek, Michal <michal.simek@amd.com>; fabrice.gasnier@foss.st.com; git
> (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip
> USB5744
> 
> On 11/18/24 4:09 PM, Abbarapu, Venkatesh wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Marek Vasut <marex@denx.de>
> >> Sent: Thursday, November 14, 2024 8:37 PM
> >> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>;
> >> u-boot@lists.denx.de
> >> Cc: Simek, Michal <michal.simek@amd.com>;
> >> fabrice.gasnier@foss.st.com; git
> >> (AMD-Xilinx) <git@amd.com>
> >> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for
> >> Microchip
> >> USB5744
> >>
> >> On 11/14/24 5:16 AM, Abbarapu, Venkatesh wrote:
> >>> Hi Marek,
> >>>
> >>>> -----Original Message-----
> >>>> From: Marek Vasut <marex@denx.de>
> >>>> Sent: Wednesday, November 13, 2024 1:15 AM
> >>>> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>;
> >>>> u-boot@lists.denx.de
> >>>> Cc: Simek, Michal <michal.simek@amd.com>;
> >>>> fabrice.gasnier@foss.st.com; git
> >>>> (AMD-Xilinx) <git@amd.com>
> >>>> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for
> >>>> Microchip
> >>>> USB5744
> >>>>
> >>>> On 11/12/24 7:42 AM, Abbarapu, Venkatesh wrote:
> >>>>> Hi,
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Marek Vasut <marex@denx.de>
> >>>>>> Sent: Tuesday, November 12, 2024 11:28 AM
> >>>>>> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>;
> >>>>>> u-boot@lists.denx.de
> >>>>>> Cc: Simek, Michal <michal.simek@amd.com>;
> >>>>>> fabrice.gasnier@foss.st.com; git
> >>>>>> (AMD-Xilinx) <git@amd.com>
> >>>>>> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for
> >>>>>> Microchip
> >>>>>> USB5744
> >>>>>>
> >>>>>> On 11/12/24 6:07 AM, Venkatesh Yadav Abbarapu wrote:
> >>>>>>> Add support for the Microchip USB5744 USB3.0 and USB2.0 Hub.
> >>>>>>> The usb5744 driver trigger hub reset signal after soft reset.
> >>>>>>> The usb5744 hub need to reset after the phy initialization,
> >>>>>>> which toggles the gpio.
> >>>>>>> Also update the usb2514 hub_data with the reset delay as 1us.
> >>>>>>>
> >>>>>>> Signed-off-by: Venkatesh Yadav Abbarapu
> >>>>>>> <venkatesh.abbarapu@amd.com>
> >>>>>>> Reviewed-by: Marek Vasut <marex@denx.de>
> >>>>>>> ---
> >>>>>>>      common/usb_onboard_hub.c | 20 ++++++++++++++++++--
> >>>>>>>      1 file changed, 18 insertions(+), 2 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
> >>>>>>> index 827ecf9b02..1d146eccee 100644
> >>>>>>> --- a/common/usb_onboard_hub.c
> >>>>>>> +++ b/common/usb_onboard_hub.c
> >>>>>>> @@ -88,10 +88,26 @@ static int usb_onboard_hub_remove(struct
> >>>>>>> udevice
> >>>> *dev)
> >>>>>>>      	return ret;
> >>>>>>>      }
> >>>>>>>
> >>>>>>> +static const struct onboard_hub_data usb2514_data = {
> >>>>>>> +	.reset_us = 1,
> >>>>>>> +};
> >>>>>>> +
> >>>>>>> +static const struct onboard_hub_data usb5744_data = {
> >>>>>>> +	.power_on_delay_us = 10000,
> >>>>>>> +	.reset_us = 10000,
> >>>>>>> +};
> >>>>>>> +
> >>>>>>>      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 */
> >>>>>>> -	{ }
> >>>>>>> +	{	.compatible = "usb424,2514", /* USB2514B USB 2.0 */
> >>>>>>> +		.data = (ulong)&usb2514_data,
> >>>>>> This ^ hub has to be updated in 1/7 , otherwise if only 1/7 is
> >>>>>> applied (e.g. during bisect), this hub reset will be operated out of
> specification.
> >>>>>>
> >>>>>> Also, looking at the USB2514 datasheet figure 5-3, it seems the
> >>>>>> hub needs t4=500us recovery time in SMBus mode. Does that mean
> >>>>>> usb2514_data .power_on_delay_us = 500 is missing too ?
> >>>>>>
> >>>>>> Also, it seems t_rstio is 5us in USB5477 datasheet figure 10-4 ,
> >>>>>> where do these 10000us figures above come from ?
> >>>>>
> >>>>> We were seeing i2c failures when we update the reset delay and
> >>>>> power on delay values mentioned from the datasheet so updated to
> >>>>> 10000us, the linux reference is below
> >>>>> https://github.com/torvalds/linux/commit/908f61bedb2c40c6d856bbfd7
> >>>>> f8
> >>>>> 70
> >>>>> b967a4cb498
> >>>> Is there a matching delay requirement specified in the USB hub
> >>>> datasheet or is this a workaround for some board-specific behavior ?
> >>> The matching delay is not specified in the USB5744 hub document, but
> >>> based on
> >> testing on 2 boards with the above-mentioned delay i2c failures were not
> observed.
> >> Is this 10ms a board-specific reset delay ?
> >> Why is it in a generic driver ?
> > On our boards we observed i2c failures when we set the reset delay as
> > 5us and power on delay as 1ms as per the USB5744 datasheet. The reason
> > of adding 10ms delay is because of i2c failures and the linux
> > reference is
> > https://github.com/torvalds/linux/commit/908f61bedb2c40c6d856bbfd7f870
> > b967a4cb498 Do you have anything with respect to these delays, as you
> > might have tested on some other boards? If anything, please let me know.
> If the 10ms delay is a board specific delay, then this has to be handled in a board
> specific way, not hard-coded in the driver. Probably add some new property which
> specifies the extra board specific reset delay, but be sure to run that property by the
> Linux kernel maintainers too.
Thanks. I will check how to add the board specific delay as new property.
As this delay might be specific to the boards which we tested.
There won't be any issue if we can add the default "reset_delay"(5us) and "power_on_delay" (1ms) from the USB5744 datasheet, as other boards will be working without any board specific delay? 

Thanks
Venkatesh

Re: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip USB5744

[Marek Vasut]

On 11/19/24 4:22 AM, Abbarapu, Venkatesh wrote:

[...]

>>>>>> Is there a matching delay requirement specified in the USB hub
>>>>>> datasheet or is this a workaround for some board-specific behavior ?
>>>>> The matching delay is not specified in the USB5744 hub document, but
>>>>> based on
>>>> testing on 2 boards with the above-mentioned delay i2c failures were not
>> observed.
>>>> Is this 10ms a board-specific reset delay ?
>>>> Why is it in a generic driver ?
>>> On our boards we observed i2c failures when we set the reset delay as
>>> 5us and power on delay as 1ms as per the USB5744 datasheet. The reason
>>> of adding 10ms delay is because of i2c failures and the linux
>>> reference is
>>> https://github.com/torvalds/linux/commit/908f61bedb2c40c6d856bbfd7f870
>>> b967a4cb498 Do you have anything with respect to these delays, as you
>>> might have tested on some other boards? If anything, please let me know.
>> If the 10ms delay is a board specific delay, then this has to be handled in a board
>> specific way, not hard-coded in the driver. Probably add some new property which
>> specifies the extra board specific reset delay, but be sure to run that property by the
>> Linux kernel maintainers too.
> Thanks. I will check how to add the board specific delay as new property.

Add some reset-...-us , similar to what ethernet PHYs already do in DT.

> As this delay might be specific to the boards which we tested.
> There won't be any issue if we can add the default "reset_delay"(5us) and "power_on_delay" (1ms) from the USB5744 datasheet, as other boards will be working without any board specific delay?
Right, the driver has to be generic , that means it should contain 
delays specified in the datasheet. Board specific delays have to be 
configured in board specific DTs.

RE: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip USB5744

[Abbarapu, Venkatesh]

Hi,

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Tuesday, November 19, 2024 7:49 PM
> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>; u-boot@lists.denx.de
> Cc: Simek, Michal <michal.simek@amd.com>; fabrice.gasnier@foss.st.com; git
> (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip
> USB5744
> 
> On 11/19/24 4:22 AM, Abbarapu, Venkatesh wrote:
> 
> [...]
> 
> >>>>>> Is there a matching delay requirement specified in the USB hub
> >>>>>> datasheet or is this a workaround for some board-specific behavior ?
> >>>>> The matching delay is not specified in the USB5744 hub document,
> >>>>> but based on
> >>>> testing on 2 boards with the above-mentioned delay i2c failures
> >>>> were not
> >> observed.
> >>>> Is this 10ms a board-specific reset delay ?
> >>>> Why is it in a generic driver ?
> >>> On our boards we observed i2c failures when we set the reset delay
> >>> as 5us and power on delay as 1ms as per the USB5744 datasheet. The
> >>> reason of adding 10ms delay is because of i2c failures and the linux
> >>> reference is
> >>> https://github.com/torvalds/linux/commit/908f61bedb2c40c6d856bbfd7f8
> >>> 70
> >>> b967a4cb498 Do you have anything with respect to these delays, as
> >>> you might have tested on some other boards? If anything, please let me know.
> >> If the 10ms delay is a board specific delay, then this has to be
> >> handled in a board specific way, not hard-coded in the driver.
> >> Probably add some new property which specifies the extra board
> >> specific reset delay, but be sure to run that property by the Linux kernel
> maintainers too.
> > Thanks. I will check how to add the board specific delay as new property.
> 
> Add some reset-...-us , similar to what ethernet PHYs already do in DT.
Sure...will check and initiate discussion with the Linux team.

> 
> > As this delay might be specific to the boards which we tested.
> > There won't be any issue if we can add the default "reset_delay"(5us) and
> "power_on_delay" (1ms) from the USB5744 datasheet, as other boards will be
> working without any board specific delay?
> Right, the driver has to be generic , that means it should contain delays specified in
> the datasheet. Board specific delays have to be configured in board specific DTs.
Thanks. I have updated the delays based on usb5744 specification in the 
[PATCH v13 3/7] usb: onboard-hub: add support for Microchip USB5744
Please review.

Thanks
Venkatesh

Re: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip USB5744

[Marek Vasut]

On 11/20/24 5:22 AM, Abbarapu, Venkatesh wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Marek Vasut <marex@denx.de>
>> Sent: Tuesday, November 19, 2024 7:49 PM
>> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>; u-boot@lists.denx.de
>> Cc: Simek, Michal <michal.simek@amd.com>; fabrice.gasnier@foss.st.com; git
>> (AMD-Xilinx) <git@amd.com>
>> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip
>> USB5744
>>
>> On 11/19/24 4:22 AM, Abbarapu, Venkatesh wrote:
>>
>> [...]
>>
>>>>>>>> Is there a matching delay requirement specified in the USB hub
>>>>>>>> datasheet or is this a workaround for some board-specific behavior ?
>>>>>>> The matching delay is not specified in the USB5744 hub document,
>>>>>>> but based on
>>>>>> testing on 2 boards with the above-mentioned delay i2c failures
>>>>>> were not
>>>> observed.
>>>>>> Is this 10ms a board-specific reset delay ?
>>>>>> Why is it in a generic driver ?
>>>>> On our boards we observed i2c failures when we set the reset delay
>>>>> as 5us and power on delay as 1ms as per the USB5744 datasheet. The
>>>>> reason of adding 10ms delay is because of i2c failures and the linux
>>>>> reference is
>>>>> https://github.com/torvalds/linux/commit/908f61bedb2c40c6d856bbfd7f8
>>>>> 70
>>>>> b967a4cb498 Do you have anything with respect to these delays, as
>>>>> you might have tested on some other boards? If anything, please let me know.
>>>> If the 10ms delay is a board specific delay, then this has to be
>>>> handled in a board specific way, not hard-coded in the driver.
>>>> Probably add some new property which specifies the extra board
>>>> specific reset delay, but be sure to run that property by the Linux kernel
>> maintainers too.
>>> Thanks. I will check how to add the board specific delay as new property.
>>
>> Add some reset-...-us , similar to what ethernet PHYs already do in DT.
> Sure...will check and initiate discussion with the Linux team.
> 
>>
>>> As this delay might be specific to the boards which we tested.
>>> There won't be any issue if we can add the default "reset_delay"(5us) and
>> "power_on_delay" (1ms) from the USB5744 datasheet, as other boards will be
>> working without any board specific delay?
>> Right, the driver has to be generic , that means it should contain delays specified in
>> the datasheet. Board specific delays have to be configured in board specific DTs.
> Thanks. I have updated the delays based on usb5744 specification in the
> [PATCH v13 3/7] usb: onboard-hub: add support for Microchip USB5744
> Please review.
V13 also enables the hub driver for Xilinx hardware, does it not ?
If so, won't doing so lead to incorrect behavior due to -- now -- too 
short delay ? I suspect you will need V14 which adds those board 
specific delays via DT for the Xilinx hardware.

Re: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip USB5744

[Michal Simek]

On 11/20/24 09:46, Marek Vasut wrote:
> On 11/20/24 5:22 AM, Abbarapu, Venkatesh wrote:
>> Hi,
>>
>>> -----Original Message-----
>>> From: Marek Vasut <marex@denx.de>
>>> Sent: Tuesday, November 19, 2024 7:49 PM
>>> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>; u-boot@lists.denx.de
>>> Cc: Simek, Michal <michal.simek@amd.com>; fabrice.gasnier@foss.st.com; git
>>> (AMD-Xilinx) <git@amd.com>
>>> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip
>>> USB5744
>>>
>>> On 11/19/24 4:22 AM, Abbarapu, Venkatesh wrote:
>>>
>>> [...]
>>>
>>>>>>>>> Is there a matching delay requirement specified in the USB hub
>>>>>>>>> datasheet or is this a workaround for some board-specific behavior ?
>>>>>>>> The matching delay is not specified in the USB5744 hub document,
>>>>>>>> but based on
>>>>>>> testing on 2 boards with the above-mentioned delay i2c failures
>>>>>>> were not
>>>>> observed.
>>>>>>> Is this 10ms a board-specific reset delay ?
>>>>>>> Why is it in a generic driver ?
>>>>>> On our boards we observed i2c failures when we set the reset delay
>>>>>> as 5us and power on delay as 1ms as per the USB5744 datasheet. The
>>>>>> reason of adding 10ms delay is because of i2c failures and the linux
>>>>>> reference is
>>>>>> https://github.com/torvalds/linux/commit/908f61bedb2c40c6d856bbfd7f8
>>>>>> 70
>>>>>> b967a4cb498 Do you have anything with respect to these delays, as
>>>>>> you might have tested on some other boards? If anything, please let me know.
>>>>> If the 10ms delay is a board specific delay, then this has to be
>>>>> handled in a board specific way, not hard-coded in the driver.
>>>>> Probably add some new property which specifies the extra board
>>>>> specific reset delay, but be sure to run that property by the Linux kernel
>>> maintainers too.
>>>> Thanks. I will check how to add the board specific delay as new property.
>>>
>>> Add some reset-...-us , similar to what ethernet PHYs already do in DT.
>> Sure...will check and initiate discussion with the Linux team.
>>
>>>
>>>> As this delay might be specific to the boards which we tested.
>>>> There won't be any issue if we can add the default "reset_delay"(5us) and
>>> "power_on_delay" (1ms) from the USB5744 datasheet, as other boards will be
>>> working without any board specific delay?
>>> Right, the driver has to be generic , that means it should contain delays 
>>> specified in
>>> the datasheet. Board specific delays have to be configured in board specific 
>>> DTs.
>> Thanks. I have updated the delays based on usb5744 specification in the
>> [PATCH v13 3/7] usb: onboard-hub: add support for Microchip USB5744
>> Please review.
> V13 also enables the hub driver for Xilinx hardware, does it not ?
> If so, won't doing so lead to incorrect behavior due to -- now -- too short 
> delay ? I suspect you will need V14 which adds those board specific delays via 
> DT for the Xilinx hardware.

Feel free to drop these patches from this series.

Thanks,
Michal

RE: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip USB5744

[Abbarapu, Venkatesh]

Hi,

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Wednesday, November 20, 2024 2:16 PM
> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>; u-boot@lists.denx.de
> Cc: Simek, Michal <michal.simek@amd.com>; fabrice.gasnier@foss.st.com; git
> (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip
> USB5744
> 
> On 11/20/24 5:22 AM, Abbarapu, Venkatesh wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Marek Vasut <marex@denx.de>
> >> Sent: Tuesday, November 19, 2024 7:49 PM
> >> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>;
> >> u-boot@lists.denx.de
> >> Cc: Simek, Michal <michal.simek@amd.com>;
> >> fabrice.gasnier@foss.st.com; git
> >> (AMD-Xilinx) <git@amd.com>
> >> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for
> >> Microchip
> >> USB5744
> >>
> >> On 11/19/24 4:22 AM, Abbarapu, Venkatesh wrote:
> >>
> >> [...]
> >>
> >>>>>>>> Is there a matching delay requirement specified in the USB hub
> >>>>>>>> datasheet or is this a workaround for some board-specific behavior ?
> >>>>>>> The matching delay is not specified in the USB5744 hub document,
> >>>>>>> but based on
> >>>>>> testing on 2 boards with the above-mentioned delay i2c failures
> >>>>>> were not
> >>>> observed.
> >>>>>> Is this 10ms a board-specific reset delay ?
> >>>>>> Why is it in a generic driver ?
> >>>>> On our boards we observed i2c failures when we set the reset delay
> >>>>> as 5us and power on delay as 1ms as per the USB5744 datasheet. The
> >>>>> reason of adding 10ms delay is because of i2c failures and the
> >>>>> linux reference is
> >>>>> https://github.com/torvalds/linux/commit/908f61bedb2c40c6d856bbfd7
> >>>>> f8
> >>>>> 70
> >>>>> b967a4cb498 Do you have anything with respect to these delays, as
> >>>>> you might have tested on some other boards? If anything, please let me
> know.
> >>>> If the 10ms delay is a board specific delay, then this has to be
> >>>> handled in a board specific way, not hard-coded in the driver.
> >>>> Probably add some new property which specifies the extra board
> >>>> specific reset delay, but be sure to run that property by the Linux
> >>>> kernel
> >> maintainers too.
> >>> Thanks. I will check how to add the board specific delay as new property.
> >>
> >> Add some reset-...-us , similar to what ethernet PHYs already do in DT.
> > Sure...will check and initiate discussion with the Linux team.
> >
> >>
> >>> As this delay might be specific to the boards which we tested.
> >>> There won't be any issue if we can add the default
> >>> "reset_delay"(5us) and
> >> "power_on_delay" (1ms) from the USB5744 datasheet, as other boards
> >> will be working without any board specific delay?
> >> Right, the driver has to be generic , that means it should contain
> >> delays specified in the datasheet. Board specific delays have to be configured in
> board specific DTs.
> > Thanks. I have updated the delays based on usb5744 specification in
> > the [PATCH v13 3/7] usb: onboard-hub: add support for Microchip
> > USB5744 Please review.
> V13 also enables the hub driver for Xilinx hardware, does it not ?
> If so, won't doing so lead to incorrect behavior due to -- now -- too short delay ? I
> suspect you will need V14 which adds those board specific delays via DT for the
> Xilinx hardware.
There are many customers who use their own boards and they might not see the issue what we observe on Xilinx/AMD boards. Don't you think we are blocking them because of our board issues? What you think?

Thanks
Venkatesh

Re: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip USB5744

[Marek Vasut]

On 11/20/24 2:20 PM, Abbarapu, Venkatesh wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Marek Vasut <marex@denx.de>
>> Sent: Wednesday, November 20, 2024 2:16 PM
>> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>; u-boot@lists.denx.de
>> Cc: Simek, Michal <michal.simek@amd.com>; fabrice.gasnier@foss.st.com; git
>> (AMD-Xilinx) <git@amd.com>
>> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip
>> USB5744
>>
>> On 11/20/24 5:22 AM, Abbarapu, Venkatesh wrote:
>>> Hi,
>>>
>>>> -----Original Message-----
>>>> From: Marek Vasut <marex@denx.de>
>>>> Sent: Tuesday, November 19, 2024 7:49 PM
>>>> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>;
>>>> u-boot@lists.denx.de
>>>> Cc: Simek, Michal <michal.simek@amd.com>;
>>>> fabrice.gasnier@foss.st.com; git
>>>> (AMD-Xilinx) <git@amd.com>
>>>> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for
>>>> Microchip
>>>> USB5744
>>>>
>>>> On 11/19/24 4:22 AM, Abbarapu, Venkatesh wrote:
>>>>
>>>> [...]
>>>>
>>>>>>>>>> Is there a matching delay requirement specified in the USB hub
>>>>>>>>>> datasheet or is this a workaround for some board-specific behavior ?
>>>>>>>>> The matching delay is not specified in the USB5744 hub document,
>>>>>>>>> but based on
>>>>>>>> testing on 2 boards with the above-mentioned delay i2c failures
>>>>>>>> were not
>>>>>> observed.
>>>>>>>> Is this 10ms a board-specific reset delay ?
>>>>>>>> Why is it in a generic driver ?
>>>>>>> On our boards we observed i2c failures when we set the reset delay
>>>>>>> as 5us and power on delay as 1ms as per the USB5744 datasheet. The
>>>>>>> reason of adding 10ms delay is because of i2c failures and the
>>>>>>> linux reference is
>>>>>>> https://github.com/torvalds/linux/commit/908f61bedb2c40c6d856bbfd7
>>>>>>> f8
>>>>>>> 70
>>>>>>> b967a4cb498 Do you have anything with respect to these delays, as
>>>>>>> you might have tested on some other boards? If anything, please let me
>> know.
>>>>>> If the 10ms delay is a board specific delay, then this has to be
>>>>>> handled in a board specific way, not hard-coded in the driver.
>>>>>> Probably add some new property which specifies the extra board
>>>>>> specific reset delay, but be sure to run that property by the Linux
>>>>>> kernel
>>>> maintainers too.
>>>>> Thanks. I will check how to add the board specific delay as new property.
>>>>
>>>> Add some reset-...-us , similar to what ethernet PHYs already do in DT.
>>> Sure...will check and initiate discussion with the Linux team.
>>>
>>>>
>>>>> As this delay might be specific to the boards which we tested.
>>>>> There won't be any issue if we can add the default
>>>>> "reset_delay"(5us) and
>>>> "power_on_delay" (1ms) from the USB5744 datasheet, as other boards
>>>> will be working without any board specific delay?
>>>> Right, the driver has to be generic , that means it should contain
>>>> delays specified in the datasheet. Board specific delays have to be configured in
>> board specific DTs.
>>> Thanks. I have updated the delays based on usb5744 specification in
>>> the [PATCH v13 3/7] usb: onboard-hub: add support for Microchip
>>> USB5744 Please review.
>> V13 also enables the hub driver for Xilinx hardware, does it not ?
>> If so, won't doing so lead to incorrect behavior due to -- now -- too short delay ? I
>> suspect you will need V14 which adds those board specific delays via DT for the
>> Xilinx hardware.
> There are many customers who use their own boards and they might not see the issue what we observe on Xilinx/AMD boards. Don't you think we are blocking them because of our board issues? What you think?
Sure, drop 6 and 7 and the current state of 1..5 can go in I think.

RE: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip USB5744

[Abbarapu, Venkatesh]

Hi,

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Thursday, November 21, 2024 5:26 AM
> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>; u-boot@lists.denx.de
> Cc: Simek, Michal <michal.simek@amd.com>; fabrice.gasnier@foss.st.com; git
> (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip
> USB5744
> 
> On 11/20/24 2:20 PM, Abbarapu, Venkatesh wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Marek Vasut <marex@denx.de>
> >> Sent: Wednesday, November 20, 2024 2:16 PM
> >> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>;
> >> u-boot@lists.denx.de
> >> Cc: Simek, Michal <michal.simek@amd.com>;
> >> fabrice.gasnier@foss.st.com; git
> >> (AMD-Xilinx) <git@amd.com>
> >> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for
> >> Microchip
> >> USB5744
> >>
> >> On 11/20/24 5:22 AM, Abbarapu, Venkatesh wrote:
> >>> Hi,
> >>>
> >>>> -----Original Message-----
> >>>> From: Marek Vasut <marex@denx.de>
> >>>> Sent: Tuesday, November 19, 2024 7:49 PM
> >>>> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>;
> >>>> u-boot@lists.denx.de
> >>>> Cc: Simek, Michal <michal.simek@amd.com>;
> >>>> fabrice.gasnier@foss.st.com; git
> >>>> (AMD-Xilinx) <git@amd.com>
> >>>> Subject: Re: [PATCH v11 3/7] usb: onboard-hub: add support for
> >>>> Microchip
> >>>> USB5744
> >>>>
> >>>> On 11/19/24 4:22 AM, Abbarapu, Venkatesh wrote:
> >>>>
> >>>> [...]
> >>>>
> >>>>>>>>>> Is there a matching delay requirement specified in the USB
> >>>>>>>>>> hub datasheet or is this a workaround for some board-specific
> behavior ?
> >>>>>>>>> The matching delay is not specified in the USB5744 hub
> >>>>>>>>> document, but based on
> >>>>>>>> testing on 2 boards with the above-mentioned delay i2c failures
> >>>>>>>> were not
> >>>>>> observed.
> >>>>>>>> Is this 10ms a board-specific reset delay ?
> >>>>>>>> Why is it in a generic driver ?
> >>>>>>> On our boards we observed i2c failures when we set the reset
> >>>>>>> delay as 5us and power on delay as 1ms as per the USB5744
> >>>>>>> datasheet. The reason of adding 10ms delay is because of i2c
> >>>>>>> failures and the linux reference is
> >>>>>>> https://github.com/torvalds/linux/commit/908f61bedb2c40c6d856bbf
> >>>>>>> d7
> >>>>>>> f8
> >>>>>>> 70
> >>>>>>> b967a4cb498 Do you have anything with respect to these delays,
> >>>>>>> as you might have tested on some other boards? If anything,
> >>>>>>> please let me
> >> know.
> >>>>>> If the 10ms delay is a board specific delay, then this has to be
> >>>>>> handled in a board specific way, not hard-coded in the driver.
> >>>>>> Probably add some new property which specifies the extra board
> >>>>>> specific reset delay, but be sure to run that property by the
> >>>>>> Linux kernel
> >>>> maintainers too.
> >>>>> Thanks. I will check how to add the board specific delay as new property.
> >>>>
> >>>> Add some reset-...-us , similar to what ethernet PHYs already do in DT.
> >>> Sure...will check and initiate discussion with the Linux team.
> >>>
> >>>>
> >>>>> As this delay might be specific to the boards which we tested.
> >>>>> There won't be any issue if we can add the default
> >>>>> "reset_delay"(5us) and
> >>>> "power_on_delay" (1ms) from the USB5744 datasheet, as other boards
> >>>> will be working without any board specific delay?
> >>>> Right, the driver has to be generic , that means it should contain
> >>>> delays specified in the datasheet. Board specific delays have to be
> >>>> configured in
> >> board specific DTs.
> >>> Thanks. I have updated the delays based on usb5744 specification in
> >>> the [PATCH v13 3/7] usb: onboard-hub: add support for Microchip
> >>> USB5744 Please review.
> >> V13 also enables the hub driver for Xilinx hardware, does it not ?
> >> If so, won't doing so lead to incorrect behavior due to -- now -- too
> >> short delay ? I suspect you will need V14 which adds those board
> >> specific delays via DT for the Xilinx hardware.
> > There are many customers who use their own boards and they might not see the
> issue what we observe on Xilinx/AMD boards. Don't you think we are blocking them
> because of our board issues? What you think?
> Sure, drop 6 and 7 and the current state of 1..5 can go in I think.
Rather than dropping these patches...
To not block others, update the default reset delays mentioned in the USB5744 datasheet.
This way the customers can use these patch series and don’t wait for the Xilinx/AMD board issues to be fixed.
We can fix Xilinx/AMD board issues with the reset delays(via DT property) for USB5744 by introducing/ discussing with Linux community.
I think this way...you don't see any issue right?

Thanks
Venkatesh

Re: [PATCH v11 3/7] usb: onboard-hub: add support for Microchip USB5744

[Marek Vasut]

On 11/21/24 6:08 AM, Abbarapu, Venkatesh wrote:

[...]

>>>>>>> As this delay might be specific to the boards which we tested.
>>>>>>> There won't be any issue if we can add the default
>>>>>>> "reset_delay"(5us) and
>>>>>> "power_on_delay" (1ms) from the USB5744 datasheet, as other boards
>>>>>> will be working without any board specific delay?
>>>>>> Right, the driver has to be generic , that means it should contain
>>>>>> delays specified in the datasheet. Board specific delays have to be
>>>>>> configured in
>>>> board specific DTs.
>>>>> Thanks. I have updated the delays based on usb5744 specification in
>>>>> the [PATCH v13 3/7] usb: onboard-hub: add support for Microchip
>>>>> USB5744 Please review.
>>>> V13 also enables the hub driver for Xilinx hardware, does it not ?
>>>> If so, won't doing so lead to incorrect behavior due to -- now -- too
>>>> short delay ? I suspect you will need V14 which adds those board
>>>> specific delays via DT for the Xilinx hardware.
>>> There are many customers who use their own boards and they might not see the
>> issue what we observe on Xilinx/AMD boards. Don't you think we are blocking them
>> because of our board issues? What you think?
>> Sure, drop 6 and 7 and the current state of 1..5 can go in I think.
> Rather than dropping these patches...
> To not block others, update the default reset delays mentioned in the USB5744 datasheet.
> This way the customers can use these patch series and don’t wait for the Xilinx/AMD board issues to be fixed.
> We can fix Xilinx/AMD board issues with the reset delays(via DT property) for USB5744 by introducing/ discussing with Linux community.
> I think this way...you don't see any issue right?
Sure, simply send the first five hub patches separately as v14, without 
the DT patches, and I'll most likely pick them up. The DT patches have 
to go in via Xilinx tree anyway, the USB patches go through USB tree.

[PATCH v11 4/7] usb: onboard-hub: Add i2c initialization for usb5744 hub

[Venkatesh Yadav Abbarapu]

Add i2c initialization hook and set usb5744 platform
data with function having required i2c initialization sequence.

Apart from the USB command attach, prevent the hub from suspend.
when the “USB Attach with SMBUS (0xAA56)” command is issued to the hub,
the hub is getting enumerated and then it puts in a suspend mode.
This causes the hub to NAK any SMBUS access made by the SMBUS Master
during this period and not able to see the hub's slave address while
running the "i2c probe" command.

Prevent the MCU from the putting the HUB in suspend mode through register
write. The BYPASS_UDC_SUSPEND bit (Bit 3) of the RuntimeFlags2 register at
address 0x411D controls this aspect of the hub. The BYPASS_UDC_SUSPEND
bit in register 0x411Dh must be set to ensure that the MCU is always
enabled and ready to respond to SMBus runtime commands. This register
needs to be written before the USB attach command is issued.
The byte sequence is as follows:
Slave addr: 0x2d           00 00 05 00 01 41 1D 08
Slave addr: 0x2d           99 37 00
Slave addr: 0x2d           AA 56 00

Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
Reviewed-by: Marek Vasut <marex@denx.de>
---
 common/usb_onboard_hub.c | 106 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 105 insertions(+), 1 deletion(-)

diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
index 1d146eccee..e209964f17 100644
--- a/common/usb_onboard_hub.c
+++ b/common/usb_onboard_hub.c
@@ -10,9 +10,15 @@
 #include <asm/gpio.h>
 #include <dm.h>
 #include <dm/device_compat.h>
+#include <i2c.h>
 #include <linux/delay.h>
 #include <power/regulator.h>
 
+#define USB5744_COMMAND_ATTACH		0x0056
+#define USB5744_COMMAND_ATTACH_LSB	0xAA
+#define USB5744_CONFIG_REG_ACCESS	0x0037
+#define USB5744_CONFIG_REG_ACCESS_LSB	0x99
+
 struct onboard_hub {
 	struct udevice *vdd;
 	struct gpio_desc *reset_gpio;
@@ -21,8 +27,89 @@ struct onboard_hub {
 struct onboard_hub_data {
 	unsigned long reset_us;
 	unsigned long power_on_delay_us;
+	int (*init)(struct udevice *dev);
 };
 
+static int usb5744_i2c_init(struct udevice *dev)
+{
+	/*
+	 *  Prevent the MCU from the putting the HUB in suspend mode through register write.
+	 *  The BYPASS_UDC_SUSPEND bit (Bit 3) of the RuntimeFlags2 register at address
+	 *  0x411D controls this aspect of the hub.
+	 *  Format to write to hub registers via SMBus- 2D 00 00 05 00 01 41 1D 08
+	 *  Byte 0: Address of slave 2D
+	 *  Byte 1: Memory address 00
+	 *  Byte 2: Memory address 00
+	 *  Byte 3: Number of bytes to write to memory
+	 *  Byte 4: Write configuration register (00)
+	 *  Byte 5: Write the number of data bytes (01- 1 data byte)
+	 *  Byte 6: LSB of register address 0x41
+	 *  Byte 7: MSB of register address 0x1D
+	 *  Byte 8: value to be written to the register
+	 */
+	u8 data_buf[8] = {0x0, 0x5, 0x0, 0x1, 0x41, 0x1D, 0x08};
+	u8 config_reg_access_buf = USB5744_CONFIG_REG_ACCESS;
+	struct udevice *i2c_bus = NULL, *i2c_dev;
+	struct ofnode_phandle_args phandle;
+	u8 buf = USB5744_COMMAND_ATTACH;
+	struct dm_i2c_chip *i2c_chip;
+	int ret, slave_addr;
+
+	ret = dev_read_phandle_with_args(dev, "i2c-bus", NULL, 0, 0, &phandle);
+	if (ret) {
+		dev_err(dev, "i2c-bus not specified\n");
+		return ret;
+	}
+
+	ret = device_get_global_by_ofnode(ofnode_get_parent(phandle.node), &i2c_bus);
+	if (ret) {
+		dev_err(dev, "Failed to get i2c node, err: %d\n", ret);
+		return ret;
+	}
+
+	ret = ofnode_read_u32(phandle.node, "reg", &slave_addr);
+	if (ret)
+		return ret;
+
+	ret = i2c_get_chip(i2c_bus, slave_addr, 1, &i2c_dev);
+	if (ret) {
+		dev_err(dev, "%s: can't find i2c chip device for addr 0x%x\n", __func__,
+			slave_addr);
+		return ret;
+	}
+
+	i2c_chip = dev_get_parent_plat(i2c_dev);
+	if (!i2c_chip) {
+		dev_err(dev, "parent platform data not found\n");
+		return -EINVAL;
+	}
+
+	i2c_chip->flags &= ~DM_I2C_CHIP_WR_ADDRESS;
+	/* SMBus write command */
+	ret = dm_i2c_write(i2c_dev, 0, (uint8_t *)&data_buf, 8);
+	if (ret) {
+		dev_err(dev, "data_buf i2c_write failed, err:%d\n", ret);
+		return ret;
+	}
+
+	/* Configuration register access command */
+	ret = dm_i2c_write(i2c_dev, USB5744_CONFIG_REG_ACCESS_LSB,
+			   &config_reg_access_buf, 2);
+	if (ret) {
+		dev_err(dev, "config_reg_access i2c_write failed, err: %d\n", ret);
+		return ret;
+	}
+
+	/* USB Attach with SMBus */
+	ret = dm_i2c_write(i2c_dev, USB5744_COMMAND_ATTACH_LSB, &buf, 2);
+	if (ret) {
+		dev_err(dev, "usb_attach i2c_write failed, err: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
 int usb_onboard_hub_reset(struct udevice *dev)
 {
 	struct onboard_hub_data *data =
@@ -53,6 +140,8 @@ int usb_onboard_hub_reset(struct udevice *dev)
 
 static int usb_onboard_hub_probe(struct udevice *dev)
 {
+	struct onboard_hub_data *data =
+		(struct onboard_hub_data *)dev_get_driver_data(dev);
 	struct onboard_hub *hub = dev_get_priv(dev);
 	int ret;
 
@@ -70,7 +159,21 @@ static int usb_onboard_hub_probe(struct udevice *dev)
 		}
 	}
 
-	return usb_onboard_hub_reset(dev);
+	ret = usb_onboard_hub_reset(dev);
+	if (ret)
+		return ret;
+
+	if (data->init) {
+		ret = data->init(dev);
+		if (ret) {
+			dev_err(dev, "onboard i2c init failed: %d\n", ret);
+			goto err;
+		}
+	}
+	return 0;
+err:
+	dm_gpio_set_value(hub->reset_gpio, 0);
+	return ret;
 }
 
 static int usb_onboard_hub_remove(struct udevice *dev)
@@ -93,6 +196,7 @@ static const struct onboard_hub_data usb2514_data = {
 };
 
 static const struct onboard_hub_data usb5744_data = {
+	.init = usb5744_i2c_init,
 	.power_on_delay_us = 10000,
 	.reset_us = 10000,
 };
-- 
2.17.1

[PATCH v11 5/7] usb: onboard-hub: Bail out if peer hub is already probed

[Venkatesh Yadav Abbarapu]

The .bind function is implemented to bind the correct
"half" of the hub that the driver wants to bind,
and returning -ENODEV for the other "half".

Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
Reviewed-by: Marek Vasut <marex@denx.de>
---
 common/usb_onboard_hub.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
index e209964f17..3c5f88387f 100644
--- a/common/usb_onboard_hub.c
+++ b/common/usb_onboard_hub.c
@@ -176,6 +176,26 @@ err:
 	return ret;
 }
 
+static int usb_onboard_hub_bind(struct udevice *dev)
+{
+	struct ofnode_phandle_args phandle;
+	const void *fdt = gd->fdt_blob;
+	int ret, off;
+
+	ret = dev_read_phandle_with_args(dev, "peer-hub", NULL, 0, 0, &phandle);
+	if (ret)  {
+		dev_err(dev, "peer-hub not specified\n");
+		return ret;
+	}
+
+	off = ofnode_to_offset(phandle.node);
+	ret = fdt_node_check_compatible(fdt, off, "usb424,5744");
+	if (!ret)
+		return 0;
+
+	return -ENODEV;
+}
+
 static int usb_onboard_hub_remove(struct udevice *dev)
 {
 	struct onboard_hub *hub = dev_get_priv(dev);
@@ -217,6 +237,7 @@ static const struct udevice_id usb_onboard_hub_ids[] = {
 U_BOOT_DRIVER(usb_onboard_hub) = {
 	.name	= "usb_onboard_hub",
 	.id	= UCLASS_USB_HUB,
+	.bind   = usb_onboard_hub_bind,
 	.probe = usb_onboard_hub_probe,
 	.remove = usb_onboard_hub_remove,
 	.of_match = usb_onboard_hub_ids,
-- 
2.17.1

[PATCH v11 6/7] configs: zynqmp_kria: Enable the USB onboard hub

[Venkatesh Yadav Abbarapu]

USB host support on ZYNQMP KRIA SOM needs onboard USB
hub driver for handling reset GPIO and for i2c initialization
sequence.

Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
Acked-by: Michal Simek <michal.simek@amd.com>
---
 configs/xilinx_zynqmp_kria_defconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/configs/xilinx_zynqmp_kria_defconfig b/configs/xilinx_zynqmp_kria_defconfig
index dd4df0b2da..a1d9aaaeb3 100644
--- a/configs/xilinx_zynqmp_kria_defconfig
+++ b/configs/xilinx_zynqmp_kria_defconfig
@@ -117,9 +117,9 @@ CONFIG_ENV_IS_IN_SPI_FLASH=y
 CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
 CONFIG_ENV_FAT_DEVICE_AND_PART=":auto"
 CONFIG_SYS_RELOC_GD_ENV_ADDR=y
-CONFIG_NET_RANDOM_ETHADDR=y
 CONFIG_NETCONSOLE=y
 CONFIG_SYS_FAULT_ECHO_LINK_DOWN=y
+CONFIG_NET_RANDOM_ETHADDR=y
 CONFIG_SPL_DM_SEQ_ALIAS=y
 CONFIG_SIMPLE_PM_BUS=y
 CONFIG_SATA=y
@@ -208,6 +208,7 @@ CONFIG_USB_DWC3=y
 CONFIG_USB_DWC3_GENERIC=y
 CONFIG_USB_ULPI_VIEWPORT=y
 CONFIG_USB_ULPI=y
+CONFIG_USB_ONBOARD_HUB=y
 CONFIG_USB_HOST_ETHER=y
 CONFIG_USB_ETHER_ASIX=y
 CONFIG_USB_GADGET=y
-- 
2.17.1

[PATCH v11 7/7] arm64: zynqmp: Update the usb5744 hub node as per binding

[Venkatesh Yadav Abbarapu]

Updating the usb5744 hub node as per the latest upstream DT binding
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/
tree/Documentation/devicetree/bindings/usb/microchip,usb5744.yaml?h=v6.8.8

Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
Acked-by: Michal Simek <michal.simek@amd.com>
---
 arch/arm/dts/zynqmp-sck-kr-g-revA.dtso | 48 ++++++++++++++++++++++++++
 arch/arm/dts/zynqmp-sck-kr-g-revB.dtso | 48 ++++++++++++++++++++++++++
 arch/arm/dts/zynqmp-sck-kv-g-revA.dtso | 18 ++++++++++
 arch/arm/dts/zynqmp-sck-kv-g-revB.dtso | 25 +++++++++++++-
 4 files changed, 138 insertions(+), 1 deletion(-)

diff --git a/arch/arm/dts/zynqmp-sck-kr-g-revA.dtso b/arch/arm/dts/zynqmp-sck-kr-g-revA.dtso
index 6349a0e108..e40840acb8 100644
--- a/arch/arm/dts/zynqmp-sck-kr-g-revA.dtso
+++ b/arch/arm/dts/zynqmp-sck-kr-g-revA.dtso
@@ -105,11 +105,19 @@
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <0>;
+			hub_1: usb-hub@2d {
+				compatible = "microchip,usb5744";
+				reg = <0x2d>;
+			};
 		};
 		usbhub_i2c1: i2c@1 {
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <1>;
+			hub_2: usb-hub@2d {
+				compatible = "microchip,usb5744";
+				reg = <0x2d>;
+			};
 		};
 		/* Bus 2/3 are not connected */
 	};
@@ -164,6 +172,26 @@
 	dr_mode = "host";
 	snps,usb3_lpm_capable;
 	maximum-speed = "super-speed";
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	/* 2.0 hub on port 1 */
+	hub_2_0: hub@1 {
+		compatible = "usb424,2744";
+		reg = <1>;
+		peer-hub = <&hub_3_0>;
+		i2c-bus = <&hub_1>;
+		reset-gpios = <&slg7xl45106 3 GPIO_ACTIVE_LOW>;
+	};
+
+	/* 3.0 hub on port 2 */
+	hub_3_0: hub@2 {
+		compatible = "usb424,5744";
+		reg = <2>;
+		peer-hub = <&hub_2_0>;
+		i2c-bus = <&hub_1>;
+		reset-gpios = <&slg7xl45106 3 GPIO_ACTIVE_LOW>;
+	};
 };
 
 &usb1 { /* mio64 - mio75 */
@@ -188,6 +216,26 @@
 	dr_mode = "host";
 	snps,usb3_lpm_capable;
 	maximum-speed = "super-speed";
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	/* 2.0 hub on port 1 */
+	hub1_2_0: hub@1 {
+		compatible = "usb424,2744";
+		reg = <1>;
+		peer-hub = <&hub1_3_0>;
+		i2c-bus = <&hub_2>;
+		reset-gpios = <&slg7xl45106 4 GPIO_ACTIVE_LOW>;
+	};
+
+	/* 3.0 hub on port 2 */
+	hub1_3_0: hub@2 {
+		compatible = "usb424,5744";
+		reg = <2>;
+		peer-hub = <&hub1_2_0>;
+		i2c-bus = <&hub_2>;
+		reset-gpios = <&slg7xl45106 4 GPIO_ACTIVE_LOW>;
+	};
 };
 
 &gem0 { /* mdio mio50/51 */
diff --git a/arch/arm/dts/zynqmp-sck-kr-g-revB.dtso b/arch/arm/dts/zynqmp-sck-kr-g-revB.dtso
index b0d737d3ca..ce1ad2b765 100644
--- a/arch/arm/dts/zynqmp-sck-kr-g-revB.dtso
+++ b/arch/arm/dts/zynqmp-sck-kr-g-revB.dtso
@@ -117,11 +117,19 @@
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <0>;
+			hub_1: usb-hub@2d {
+				compatible = "microchip,usb5744";
+				reg = <0x2d>;
+			};
 		};
 		usbhub_i2c1: i2c@1 {
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <1>;
+			hub_2: usb-hub@2d {
+				compatible = "microchip,usb5744";
+				reg = <0x2d>;
+			};
 		};
 		/* Bus 2/3 are not connected */
 	};
@@ -184,6 +192,26 @@
 	dr_mode = "host";
 	snps,usb3_lpm_capable;
 	maximum-speed = "super-speed";
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	/* 2.0 hub on port 1 */
+	hub_2_0: hub@1 {
+		compatible = "usb424,2744";
+		reg = <1>;
+		peer-hub = <&hub_3_0>;
+		i2c-bus = <&hub_1>;
+		reset-gpios = <&slg7xl45106 3 GPIO_ACTIVE_LOW>;
+	};
+
+	/* 3.0 hub on port 2 */
+	hub_3_0: hub@2 {
+		compatible = "usb424,5744";
+		reg = <2>;
+		peer-hub = <&hub_2_0>;
+		i2c-bus = <&hub_1>;
+		reset-gpios = <&slg7xl45106 3 GPIO_ACTIVE_LOW>;
+	};
 };
 
 &usb1 { /* mio64 - mio75 */
@@ -209,6 +237,26 @@
 	dr_mode = "host";
 	snps,usb3_lpm_capable;
 	maximum-speed = "super-speed";
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	/* 2.0 hub on port 1 */
+	hub1_2_0: hub@1 {
+		compatible = "usb424,2744";
+		reg = <1>;
+		peer-hub = <&hub1_3_0>;
+		i2c-bus = <&hub_2>;
+		reset-gpios = <&slg7xl45106 4 GPIO_ACTIVE_LOW>;
+	};
+
+	/* 3.0 hub on port 2 */
+	hub1_3_0: hub@2 {
+		compatible = "usb424,5744";
+		reg = <2>;
+		peer-hub = <&hub1_2_0>;
+		i2c-bus = <&hub_2>;
+		reset-gpios = <&slg7xl45106 4 GPIO_ACTIVE_LOW>;
+	};
 };
 
 &gem0 { /* mdio mio50/51 */
diff --git a/arch/arm/dts/zynqmp-sck-kv-g-revA.dtso b/arch/arm/dts/zynqmp-sck-kv-g-revA.dtso
index 561b546e37..0ef0357bd2 100644
--- a/arch/arm/dts/zynqmp-sck-kv-g-revA.dtso
+++ b/arch/arm/dts/zynqmp-sck-kv-g-revA.dtso
@@ -142,6 +142,24 @@
 	dr_mode = "host";
 	snps,usb3_lpm_capable;
 	maximum-speed = "super-speed";
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	/* 2.0 hub on port 1 */
+	hub_2_0: hub@1 {
+		compatible = "usb424,2744";
+		reg = <1>;
+		peer-hub = <&hub_3_0>;
+		reset-gpios = <&gpio 44 GPIO_ACTIVE_LOW>;
+	};
+
+	/* 3.0 hub on port 2 */
+	hub_3_0: hub@2 {
+		compatible = "usb424,5744";
+		reg = <2>;
+		peer-hub = <&hub_2_0>;
+		reset-gpios = <&gpio 44 GPIO_ACTIVE_LOW>;
+	};
 };
 
 &sdhci1 { /* on CC with tuned parameters */
diff --git a/arch/arm/dts/zynqmp-sck-kv-g-revB.dtso b/arch/arm/dts/zynqmp-sck-kv-g-revB.dtso
index 64683e0ccb..92d8851eb8 100644
--- a/arch/arm/dts/zynqmp-sck-kv-g-revB.dtso
+++ b/arch/arm/dts/zynqmp-sck-kv-g-revB.dtso
@@ -92,7 +92,10 @@
 		label = "ina260-u14";
 		reg = <0x40>;
 	};
-	/* u43 - 0x2d - USB hub */
+	hub: usb-hub@2d {
+		compatible = "microchip,usb5744";
+		reg = <0x2d>;
+	};
 	/* u27 - 0xe0 - STDP4320 DP/HDMI splitter */
 };
 
@@ -146,6 +149,26 @@
 	dr_mode = "host";
 	snps,usb3_lpm_capable;
 	maximum-speed = "super-speed";
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	/* 2.0 hub on port 1 */
+	hub_2_0: hub@1 {
+		compatible = "usb424,2744";
+		reg = <1>;
+		peer-hub = <&hub_3_0>;
+		i2c-bus = <&hub>;
+		reset-gpios = <&gpio 44 GPIO_ACTIVE_LOW>;
+	};
+
+	/* 3.0 hub on port 2 */
+	hub_3_0: hub@2 {
+		compatible = "usb424,5744";
+		reg = <2>;
+		peer-hub = <&hub_2_0>;
+		i2c-bus = <&hub>;
+		reset-gpios = <&gpio 44 GPIO_ACTIVE_LOW>;
+	};
 };
 
 &sdhci1 { /* on CC with tuned parameters */
-- 
2.17.1