Hi, > -----Original Message----- > From: Marek Vasut <[email protected]> > Sent: Wednesday, November 20, 2024 2:16 PM > To: Abbarapu, Venkatesh <[email protected]>; [email protected] > Cc: Simek, Michal <[email protected]>; [email protected]; git > (AMD-Xilinx) <[email protected]> > 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 <[email protected]> > >> Sent: Tuesday, November 19, 2024 7:49 PM > >> To: Abbarapu, Venkatesh <[email protected]>; > >> [email protected] > >> Cc: Simek, Michal <[email protected]>; > >> [email protected]; git > >> (AMD-Xilinx) <[email protected]> > >> 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
