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

[Michal Simek <michal.simek@amd.com>]

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