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

[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@xxxxxxxxxxxxxxx
> > > > > Signed-off-by: Ferry Toth <ftoth@xxxxxxxxxxxxxx>
> > > > 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.

Guenter