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

[Abbarapu, Venkatesh]

Hi,

> -----Original Message-----
> From: Marek Vasut <marex at denx.de>
> Sent: Wednesday, November 20, 2024 2:16 PM
> To: Abbarapu, Venkatesh <venkatesh.abbarapu at amd.com>; u-boot at lists.denx.de
> Cc: Simek, Michal <michal.simek at amd.com>; fabrice.gasnier at foss.st.com; git
> (AMD-Xilinx) <git at 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 at denx.de>
> >> Sent: Tuesday, November 19, 2024 7:49 PM
> >> To: Abbarapu, Venkatesh <venkatesh.abbarapu at amd.com>;
> >> u-boot at lists.denx.de
> >> Cc: Simek, Michal <michal.simek at amd.com>;
> >> fabrice.gasnier at foss.st.com; git
> >> (AMD-Xilinx) <git at 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