On Mon, Nov 08, 2021 at 02:01:58PM -0800, Trent Piepho wrote: > On Mon, Nov 8, 2021 at 1:10 AM Sascha Hauer <sha@xxxxxxxxxxxxxx> wrote: > > On Thu, Nov 04, 2021 at 02:40:08PM -0700, Trent Piepho wrote: > > > > > > I think the specific situation you are concerned about is where: > > > A) the dts does define a phy for usb > > > B) This phy does not work in Barebox, e.g. no driver > > > C) Despite B, USB will still operate with the desired level > > > functionality without a working phy driver. > > > > > > With the patch and CONFIG_GENERIC_PHY disabled, we get a non-fatal > > > return at step A and everything is good. But once enabled, we now get > > > a fatal error at step B and this is not good. > > > > > > Could this be fixed by making the error at B non-fatal? This is more > > > how Linux works here: errors that are non-fatal in Linux's > > > phy_optional_get() path are fatal for Barebox. > > > > In Linux phy_optional_get() returns NULL (which is a valid phy) when > > a) there is no "phys" property > > b) The phy os compatible to "usb-nop-xceiv" > > c) The phy has a status = "disabled" property > > Add to this: > d) Missing phy-names property. > e) Named phy is not in phy-names. > f) Malformed phys property. > g) PHY's index from phy-names does not exist in phys property. > h) PHY exists in phys property, but has empty phandle > i) No '#phy-cells' property for phy. > j) No of_node for device and phy is not in the global phy list. No > driver, not defined, error adding, anything could cause this to > happen. > > And then we go to device specific cases: > An Allwinner sun4i usb phy provider is given a phy index that is > greater than the number of defined phys. > A phy from a sun4i phy provider is marked as missing in the device > configuration data, i.e. phy 1 or 2 on an Allwinner H6. > A Broadcom stingray phy can not ioremap its registers with ENODEV. > And the list goes on, but I think my point is made. You are right, there are many more cases that lead to a -ENODEV which causes phy_optional_get() to return a NULL phy. I don't think this is intended and it doesn't make much sense to me. > > > When there is a phy specified in the device tree then it's mandatory > > and failures in getting it must lead to an error. the "optional" in > > "Must lead to an error" Can this be said with certainty? I can find > no real case where a warning results in a different end than an error. > Put another way, suppose it does not lead to an error? What specific > bad thing happens? > > > phy_optional_get() is only meant for the "there is no phy specified in > > the device tree" case, it's only a shortcut for > > > > if (is_there_a_phy_in_device_tree()) > > phy = phy_get(); > > else > > phy = NULL; > > Perhaps that is what it was supposed to mean, I agree these seem like > sensible semantics, but see the various errors I tracked down above, > it's far far more complex than that. IMHO, this complexity is > evidence of a bad design. No one can keep straight what errors are > fatal and which are not. It is better to just make all errors > non-fatal and be done with it. Then at least one knows what to > expect. > > > Once you interpret "optional" as something different you end up > > returning NULL for cases where you really need a phy. Only you as a > > human know that, in your special case, on your special board, there is a > > phy specified, but it can do without initialisation. That's why I > > suggested you should add a status = "disabled" property to the phy to > > explicitly state that it can work without the phy. By making it explicit > > you also document that you thought about the case and not just exploited > > a heuristic in barebox. > > I think returning NULL on every error, with a warning message, will > allow this. The warning lets the user know there may be an issue with > the usb phy. If it works, they can decide the warning is spurious and > silence it, via existing dts editing methods (/delete-property/, > status = disabled, etc.). If it doesn't work, they know they need to > enable a driver, write a driver, or perhaps disable USB entirely as > unsupported. Given that phy_optional_get() returns rather random results I tend to agree now. Let's insert a warning, return successfully and see what happens. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/barebox
