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

[Marek Vasut <marex@denx.de>]

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.