Re: [PATCH v5 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout

[Ferry Toth <fntoth@gmail.com>]

Op 21-12-2022 om 18:30 schreef Guenter Roeck:
> On Wed, Dec 21, 2022 at 03:29:09PM +0100, Ferry Toth wrote:
>> Hi
>>
>> On 21-12-2022 13:41, Guenter Roeck wrote:
>>> On Wed, Dec 21, 2022 at 11:07:50AM +0100, Ferry Toth wrote:
>>>> Hi,
>>>>
>>>> On 20-12-2022 20:43, Guenter Roeck wrote:
>>>>> On Mon, Dec 05, 2022 at 09:15:26PM +0100, Ferry Toth wrote:
>>>>>> Since commit 0f0101719138 ("usb: dwc3: Don't switch OTG -> peripheral
>>>>>> if extcon is present") Dual Role support on Intel Merrifield platform
>>>>>> broke due to rearranging the call to dwc3_get_extcon().
>>>>>>
>>>>>> It appears to be caused by ulpi_read_id() on the first test write failing
>>>>>> with -ETIMEDOUT. Currently ulpi_read_id() expects to discover the phy via
>>>>>> DT when the test write fails and returns 0 in that case, even if DT does not
>>>>>> provide the phy. As a result usb probe completes without phy.
>>>>>>
>>>>>> Make ulpi_read_id() return -ETIMEDOUT to its user if the first test write
>>>>>> fails. The user should then handle it appropriately. A follow up patch
>>>>>> will make dwc3_core_init() set -EPROBE_DEFER in this case and bail out.
>>>>>>
>>>>>> Fixes: ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT")
>>>>>> Cc: stable@vger.kernel.org
>>>>>> Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
>>>>> Hi,
>>>>>
>>>>> this patch results in some qemu test failures, specifically xilinx-zynq-a9
>>>>> machine and zynq-zc702 as well as zynq-zed devicetree files, when trying
>>>>> to boot from USB drive. The log shows
>>>> I'm not familiar with that platform. Does it use dt to discover the ulpi
>>>> device?
>>>>
>>> The dt usb description includes
>>>
>>> 	usb_phy0: phy0 {
>>>                   compatible = "usb-nop-xceiv";
>>>                   #phy-cells = <0>;
>>>           };
>>>
>>> ...
>>>
>>> &usb0 {
>>>           status = "okay";
>>>           dr_mode = "host";
>>>           usb-phy = <&usb_phy0>;
>>> };
>>>
>>> ...
>>>
>>>                   usb0: usb@e0002000 {
>>>                           compatible = "xlnx,zynq-usb-2.20a", "chipidea,usb2";
>>>                           status = "disabled";
>>>                           clocks = <&clkc 28>;
>>>                           interrupt-parent = <&intc>;
>>>                           interrupts = <0 21 4>;
>>>                           reg = <0xe0002000 0x1000>;
>>>                           phy_type = "ulpi";
>>>                   };
>>>
>>> The chipidea core initialization code includes
>>>
>>>           if (!platdata->phy_mode)
>>>                   platdata->phy_mode = of_usb_get_phy_mode(dev->of_node);
>>>
>>> Does that mean that every chipidea based usb implementation specifying
>>> 	phy_type = "ulpi";
>>> in their devicetree description will now fail, plus maybe others
>>> who determine the phy mode from devicetree ?
>> I don't think so.
>>>> I'm guessing that the problem is actually caused by "usb: ulpi: defer
>>>> ulpi_register on ulpi_read_id timeout".
>>>>
>>> Confused. Isn't that this patch ?
>> Ehem. Yes.
>>>> ulpi_read_id() now returns ETIMEDOUT due to the test write ulpi_write(ulpi,
>>>> ULPI_SCRATCH, 0xaa) failing.
>>>>
>>>> Maybe  we can create a fix by skipping the test write in case dt discovery
>>>> is available and calling of_device_request_module() directly, instead of
>>>> masking the timed out test write as it was before?
>>>>
>>> I have no idea. All I can see is that it appears that there was a reason
>>> for not returning an error if that test write failed.
>>
>> It seems to have been a quick patch to solve a power sequencing issue:
>>
>> "The ULPI bus code supports native enumeration by reading the
>> vendor ID and product ID registers at device creation time, but
>> we can't be certain that those register reads will succeed if the
>> phy is not powered up. To avoid any problems with reading the ID
>> registers before the phy is powered we fallback to DT matching
>> when the ID reads fail.
>>
>> If the ULPI spec had some generic power sequencing for these
>> registers we could put that into the ULPI bus layer and power up
>> the device before reading the ID registers. Unfortunately this
>> doesn't exist and the power sequence is usually device specific.
>> By having the device matched up with DT we can avoid this
>> problem."
>>
>> But as is, the code now requires a DT when there is a power
>> sequencing issue, which is wrong for Merrifield. It seems my patch
>> breaks the OF path, by replacing that by a deferred probe.
>>
>> I'm thinking the correct way would be:
>> - if present use DT
>> - else if test write fails, defer probe
>> - else enumeration by reading the vendor ID and product ID registers
>>
> 
> I think this patch should be reverted until a better solution is found.
> After all, at this point it is effectively unknown if there are other
> users (besides devicetree) depending on ulpi_read_id() returning 0 if
> the communication with the device fails.

I don't see how any code could rely on not having DT and a timeout while 
attempting to enumerate and still expecting success (0). dwc3 in that 
case just assumes ulpi found and continues probe leading to 
non-functional dwc3 host.
I think a fix should be relatively simple to find and resolve 2 bads.

> Guenter