Message ID | 20241112050740.15343-4-venkatesh.abbarapu@amd.com |
---|---|
State | Superseded |
Delegated to: | Marek Vasut |
Headers | show |
Series | Add the USB5744 hub driver as per new DT binding | expand |
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 ?
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
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 ?
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
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 ?
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
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.
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
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.
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
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.
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
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
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.
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
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.
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) = {