mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Sascha Hauer <sha@pengutronix.de>
To: Trent Piepho <trent.piepho@igorinstitute.com>
Cc: "Daniel Brát" <danek.brat@gmail.com>, barebox@lists.infradead.org
Subject: Re: [PATCH] phy: core: Make 'phy_optional_get' return NULL when not implemented
Date: Thu, 11 Nov 2021 14:09:18 +0100	[thread overview]
Message-ID: <20211111130918.GV25698@pengutronix.de> (raw)
In-Reply-To: <CAMHeXxNw1qOXS7X+_bE9h7SK5z80=DCsYMopDwJwGCmCe4ROtg@mail.gmail.com>

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@pengutronix.de> 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@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


  reply	other threads:[~2021-11-11 13:11 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-20  8:39 Daniel Brát
2021-11-01  9:00 ` Sascha Hauer
     [not found]   ` <CAMHeXxMJYC2q4Hs7zd9A=5-KsXyFUtTshMVoZZC2nJUah=LtjA@mail.gmail.com>
2021-11-01 19:04     ` Trent Piepho
2021-11-02  7:40     ` Sascha Hauer
2021-11-03 22:35       ` Trent Piepho
2021-11-04 20:02         ` Sascha Hauer
2021-11-04 21:40           ` Trent Piepho
2021-11-08  9:10             ` Sascha Hauer
2021-11-08 22:01               ` Trent Piepho
2021-11-11 13:09                 ` Sascha Hauer [this message]
2022-03-03  9:53 ` Sascha Hauer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211111130918.GV25698@pengutronix.de \
    --to=sha@pengutronix.de \
    --cc=barebox@lists.infradead.org \
    --cc=danek.brat@gmail.com \
    --cc=trent.piepho@igorinstitute.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox