[PATCH] phy: core: Make 'phy_optional_get' return NULL when not implemented

[PATCH] phy: core: Make 'phy_optional_get' return NULL when not implemented

[Daniel Brát]

Make 'phy_optional_get' return NULL instead of ERR_PTR(-ENOSYS) when the
CONFIG_GENERIC_PHY is not enabled. It makes more sense to return NULL instead
of straight up throwing a error since the function has 'optional' in its name.
This also fixes dwc2 usb driver which would previously fail inside its probe
function despite being able to function without a phy just fine.

Signed-off-by: Daniel Brát <danek.brat@gmail.com>
---
 include/linux/phy/phy.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index 679ce6e42..321e546f9 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -195,7 +195,7 @@ static inline struct phy *phy_get(struct device_d *dev, const char *string)
 static inline struct phy *phy_optional_get(struct device_d *dev,
 					   const char *string)
 {
-	return ERR_PTR(-ENOSYS);
+	return NULL;
 }
 
 static inline struct phy *of_phy_get_by_phandle(struct device_d *dev,
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] phy: core: Make 'phy_optional_get' return NULL when not implemented

[Sascha Hauer]

On Wed, Oct 20, 2021 at 10:39:54AM +0200, Daniel Brát wrote:
> Make 'phy_optional_get' return NULL instead of ERR_PTR(-ENOSYS) when the
> CONFIG_GENERIC_PHY is not enabled. It makes more sense to return NULL instead
> of straight up throwing a error since the function has 'optional' in its name.
> This also fixes dwc2 usb driver which would previously fail inside its probe
> function despite being able to function without a phy just fine.

The phy is only optional as long as none is specified in the device
tree. When there is one specified then it's no longer optional. We can't
do the right thing here without checking the device tree. Given that
it's simple to enable CONFIG_GENERIC_PHY I think this is the way to go.

How about issuing a warning in the phy_optional_get stub? That would
point the user in the right direction.

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

Re: [PATCH] phy: core: Make 'phy_optional_get' return NULL when not implemented

[Trent Piepho]

On Mon, Nov 1, 2021 at 2:01 AM Sascha Hauer <sha@pengutronix.de> wrote:
> On Wed, Oct 20, 2021 at 10:39:54AM +0200, Daniel Brát wrote:
> > Make 'phy_optional_get' return NULL instead of ERR_PTR(-ENOSYS) when the
> > CONFIG_GENERIC_PHY is not enabled. It makes more sense to return NULL instead
> > of straight up throwing a error since the function has 'optional' in its name.
> > This also fixes dwc2 usb driver which would previously fail inside its probe
> > function despite being able to function without a phy just fine.
>
> The phy is only optional as long as none is specified in the device
> tree. When there is one specified then it's no longer optional. We can't
> do the right thing here without checking the device tree. Given that
> it's simple to enable CONFIG_GENERIC_PHY I think this is the way to go.

But this force enables GENERIC_PHY when it's not needed.

There are commonly many device nodes in Linux dts files that are not
used by an enabled Barebox driver.  It's normal to turn off a driver
that might be or could be used.  Is it necessarily an error if a phy
is present in the dts but we don't wish to include support for it?

In this Barebox version of this code, I think phy_optional_get() only
returns NULL if there is some error finding the phy OF node, such as
there being none specified or some error in the validity of the dts
data.  Otherwise it's PROBE_DEFER.

In the Linux version, there are other ways to get NULL, such as the
phy being disabled:
        if (!of_device_is_available(args.np)) {
               dev_warn(phy_provider->dev, "Requested PHY is disabled\n");
               phy = ERR_PTR(-ENODEV);  // phy_optional_get -> NULL

So it seems like the phy being optional, even if defined in this dts,
might be the intended behavior.

The stub version could check if a phy is in the dts and generate a
warning, if the goal is to reduce problems with people creating broken
configurations and then not understanding why their configuration does
not work.

struct phy *phy_optional_get(struct device_d *dev, const char *string)
{
        if (dev->device_node &&
            !of_parse_phandle_with_args(dev->device_node, "phys",
"#phy-cells", of_property_match_string(dev->device_node, "phy-names",
string), NULL))
                dev_warn(dev, "%s phy specified in device tree but
CONFIG_GENERIC_PHY support is not enabled", string);
        return NULL;
}

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] phy: core: Make 'phy_optional_get' return NULL when not implemented

[Sascha Hauer]

On Mon, Nov 01, 2021 at 11:33:07AM -0700, Trent Piepho wrote:
>    On Mon, Nov 1, 2021 at 2:01 AM Sascha Hauer <[1]sha@pengutronix.de> wrote:
>    > On Wed, Oct 20, 2021 at 10:39:54AM +0200, Daniel Brát wrote:
>    > > Make 'phy_optional_get' return NULL instead of ERR_PTR(-ENOSYS) when
>    the
>    > > CONFIG_GENERIC_PHY is not enabled. It makes more sense to return NULL
>    instead
>    > > of straight up throwing a error since the function has 'optional' in
>    its name.
>    > > This also fixes dwc2 usb driver which would previously fail inside its
>    probe
>    > > function despite being able to function without a phy just fine.
>    >
>    > The phy is only optional as long as none is specified in the device
>    > tree. When there is one specified then it's no longer optional. We can't
>    > do the right thing here without checking the device tree. Given that
>    > it's simple to enable CONFIG_GENERIC_PHY I think this is the way to go.
> 
>    But this force enables GENERIC_PHY when it's not needed.
> 
>    There are commonly many device nodes in Linux dts files that are not used
>    by an enabled Barebox driver.  It's normal to turn off a driver that might
>    be or could be used.  Is it necessarily an error if a phy is present in
>    the dts but we don't wish to include support for it?

We need to distinguish the cases "There is a phy specified, but the
reset defaults are good enough to go without a driver" and "There is a
phy specified and we need driver support for it". barebox can't know
this.

Returning NULL from the phy_optional_get() stub has another problem:
Some devices might work with CONFIG_GENERIC_PHY disabled, but stop
to work when CONFIG_GENERIC_PHY gets enabled, because we might have
no driver for the phy. Having boards that only work with CONFIG_GENERIC_PHY
disabled is rather unexpected as well.

>    struct phy *phy_optional_get(struct device_d *dev, const char *string)
>    {
>            if (dev->device_node &&
>                !of_parse_phandle_with_args(dev->device_node, "phys",
>    "#phy-cells", of_property_match_string(dev->device_node, "phy-names",
>    string), NULL))
>                    dev_warn(dev, "%s phy specified in device tree but
>    CONFIG_GENERIC_PHY support is not enabled", string);
>            return NULL;
>    }

I am fine with this approach. We could add a check if the phy node has a
status = "disabled" property. That would allow a board to explicitly
state that we do not need to support a phy. We could then do the
right thing no matter if CONFIG_GENERIC_PHY is enabled or not.

Thinking about it I would prefer something like barebox,status = "disabled"
to prevent the property from leaking into Linux.

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

Re: [PATCH] phy: core: Make 'phy_optional_get' return NULL when not implemented

[Trent Piepho]

On Tue, Nov 2, 2021 at 12:40 AM Sascha Hauer <sha@pengutronix.de> wrote:
>
> On Mon, Nov 01, 2021 at 11:33:07AM -0700, Trent Piepho wrote:
> >    On Mon, Nov 1, 2021 at 2:01 AM Sascha Hauer <[1]sha@pengutronix.de> wrote:
 >    >
> >    > The phy is only optional as long as none is specified in the device
> >    > tree. When there is one specified then it's no longer optional. We can't
> >    > do the right thing here without checking the device tree. Given that
> >    > it's simple to enable CONFIG_GENERIC_PHY I think this is the way to go.
> >
> >    But this force enables GENERIC_PHY when it's not needed.
> >
> >    There are commonly many device nodes in Linux dts files that are not used
> >    by an enabled Barebox driver.  It's normal to turn off a driver that might
> >    be or could be used.  Is it necessarily an error if a phy is present in
> >    the dts but we don't wish to include support for it?
>
> We need to distinguish the cases "There is a phy specified, but the
> reset defaults are good enough to go without a driver" and "There is a
> phy specified and we need driver support for it". barebox can't know
> this.

Could we say that compiling barebox without CONFIG_GENERIC_PHY means
the driver is not needed and compiling it with the driver means that
is it?

One can have the defconfig include drivers that might be needed, but
at some point an engineer ought to be able to

> Returning NULL from the phy_optional_get() stub has another problem:
> Some devices might work with CONFIG_GENERIC_PHY disabled, but stop
> to work when CONFIG_GENERIC_PHY gets enabled, because we might have
> no driver for the phy. Having boards that only work with CONFIG_GENERIC_PHY
> disabled is rather unexpected as well.

Return NULL if there is no driver.

> >    struct phy *phy_optional_get(struct device_d *dev, const char *string)
> >    {
> >            if (dev->device_node &&
> >                !of_parse_phandle_with_args(dev->device_node, "phys",
> >    "#phy-cells", of_property_match_string(dev->device_node, "phy-names",
> >    string), NULL))
> >                    dev_warn(dev, "%s phy specified in device tree but
> >    CONFIG_GENERIC_PHY support is not enabled", string);
> >            return NULL;
> >    }
>
> I am fine with this approach. We could add a check if the phy node has a
> status = "disabled" property. That would allow a board to explicitly
> state that we do not need to support a phy. We could then do the
> right thing no matter if CONFIG_GENERIC_PHY is enabled or not.
>
> Thinking about it I would prefer something like barebox,status = "disabled"
> to prevent the property from leaking into Linux.

Wouldn't it be easier to just delete the phy-names property to disable the phy?

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] phy: core: Make 'phy_optional_get' return NULL when not implemented

[Sascha Hauer]

On Wed, Nov 03, 2021 at 03:35:08PM -0700, Trent Piepho wrote:
> On Tue, Nov 2, 2021 at 12:40 AM Sascha Hauer <sha@pengutronix.de> wrote:
> >
> > On Mon, Nov 01, 2021 at 11:33:07AM -0700, Trent Piepho wrote:
> > >    On Mon, Nov 1, 2021 at 2:01 AM Sascha Hauer <[1]sha@pengutronix.de> wrote:
>  >    >
> > >    > The phy is only optional as long as none is specified in the device
> > >    > tree. When there is one specified then it's no longer optional. We can't
> > >    > do the right thing here without checking the device tree. Given that
> > >    > it's simple to enable CONFIG_GENERIC_PHY I think this is the way to go.
> > >
> > >    But this force enables GENERIC_PHY when it's not needed.
> > >
> > >    There are commonly many device nodes in Linux dts files that are not used
> > >    by an enabled Barebox driver.  It's normal to turn off a driver that might
> > >    be or could be used.  Is it necessarily an error if a phy is present in
> > >    the dts but we don't wish to include support for it?
> >
> > We need to distinguish the cases "There is a phy specified, but the
> > reset defaults are good enough to go without a driver" and "There is a
> > phy specified and we need driver support for it". barebox can't know
> > this.
> 
> Could we say that compiling barebox without CONFIG_GENERIC_PHY means
> the driver is not needed and compiling it with the driver means that
> is it?

Please no. Enabling Kconfig options ideally gives you additional
features, but it should not break anything. Consider the case when you
need to enable CONFIG_GENERIC_PHY for something else like an LVDS phy or
whatever, you don't want to end up with broken USB support then and have
to choose whether USB or LVDS is working.

> 
> One can have the defconfig include drivers that might be needed, but
> at some point an engineer ought to be able to
> 
> > Returning NULL from the phy_optional_get() stub has another problem:
> > Some devices might work with CONFIG_GENERIC_PHY disabled, but stop
> > to work when CONFIG_GENERIC_PHY gets enabled, because we might have
> > no driver for the phy. Having boards that only work with CONFIG_GENERIC_PHY
> > disabled is rather unexpected as well.
> 
> Return NULL if there is no driver.
> 
> > >    struct phy *phy_optional_get(struct device_d *dev, const char *string)
> > >    {
> > >            if (dev->device_node &&
> > >                !of_parse_phandle_with_args(dev->device_node, "phys",
> > >    "#phy-cells", of_property_match_string(dev->device_node, "phy-names",
> > >    string), NULL))
> > >                    dev_warn(dev, "%s phy specified in device tree but
> > >    CONFIG_GENERIC_PHY support is not enabled", string);
> > >            return NULL;
> > >    }
> >
> > I am fine with this approach. We could add a check if the phy node has a
> > status = "disabled" property. That would allow a board to explicitly
> > state that we do not need to support a phy. We could then do the
> > right thing no matter if CONFIG_GENERIC_PHY is enabled or not.
> >
> > Thinking about it I would prefer something like barebox,status = "disabled"
> > to prevent the property from leaking into Linux.
> 
> Wouldn't it be easier to just delete the phy-names property to disable the phy?

My point is that you sometimes start Linux with the barebox internal
device tree. We should then pass a device tree Linux can handle
properly. A barebox,status would just be ignored by Linux whereas a
status = "disabled" property or deleted phy-names property would disable
the phy for Linux as well.

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

Re: [PATCH] phy: core: Make 'phy_optional_get' return NULL when not implemented

[Trent Piepho]

On Thu, Nov 4, 2021 at 1:02 PM Sascha Hauer <sha@pengutronix.de> wrote:
> On Wed, Nov 03, 2021 at 03:35:08PM -0700, Trent Piepho wrote:
> > On Tue, Nov 2, 2021 at 12:40 AM Sascha Hauer <sha@pengutronix.de> wrote:
> > >
> > > On Mon, Nov 01, 2021 at 11:33:07AM -0700, Trent Piepho wrote:
> > > >    On Mon, Nov 1, 2021 at 2:01 AM Sascha Hauer <[1]sha@pengutronix.de> wrote:
> >  >    >
> > > >    > The phy is only optional as long as none is specified in the device
> > > >    > tree. When there is one specified then it's no longer optional. We can't
> > > >    > do the right thing here without checking the device tree. Given that
> > > >    > it's simple to enable CONFIG_GENERIC_PHY I think this is the way to go.
> > > >
> > > >    But this force enables GENERIC_PHY when it's not needed.
> > > >
> > > >    There are commonly many device nodes in Linux dts files that are not used
> > > >    by an enabled Barebox driver.  It's normal to turn off a driver that might
> > > >    be or could be used.  Is it necessarily an error if a phy is present in
> > > >    the dts but we don't wish to include support for it?
> > >
> > > We need to distinguish the cases "There is a phy specified, but the
> > > reset defaults are good enough to go without a driver" and "There is a
> > > phy specified and we need driver support for it". barebox can't know
> > > this.
> >
> > Could we say that compiling barebox without CONFIG_GENERIC_PHY means
> > the driver is not needed and compiling it with the driver means that
> > is it?
>
> Please no. Enabling Kconfig options ideally gives you additional
> features, but it should not break anything. Consider the case when you
> need to enable CONFIG_GENERIC_PHY for something else like an LVDS phy or
> whatever, you don't want to end up with broken USB support then and have
> to choose whether USB or LVDS is working.

I thought your goal was to prevent less experienced users from
building a non-functional Barebox and then not understanding what they
had done.  Turning off a necessary driver and breaking USB while
producing no warnings nor errors.  But I now gather I was mistaken and
this isn't really the problem.

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.

Let phy_optional_get return NULL for any error.  Create a warning if
it appears the error was not: "no phy defined in dts".

But what if there really is an unexpected error with the PHY?  We
won't trigger the phy failure path in the driver!   I think
realistically, this is never going to make a difference for anyone.
Either way, one gets an error message and non-functional usb.  Does it
really matter where the error comes from?

> > Wouldn't it be easier to just delete the phy-names property to disable the phy?
>
> My point is that you sometimes start Linux with the barebox internal
> device tree. We should then pass a device tree Linux can handle
> properly. A barebox,status would just be ignored by Linux whereas a
> status = "disabled" property or deleted phy-names property would disable
> the phy for Linux as well.

If phy_optional_get does not make an unsupported phy an error, then
there isn't really a need to disable the phy anymore.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] phy: core: Make 'phy_optional_get' return NULL when not implemented

[Sascha Hauer]

On Thu, Nov 04, 2021 at 02:40:08PM -0700, Trent Piepho wrote:
> On Thu, Nov 4, 2021 at 1:02 PM Sascha Hauer <sha@pengutronix.de> wrote:
> > On Wed, Nov 03, 2021 at 03:35:08PM -0700, Trent Piepho wrote:
> > > On Tue, Nov 2, 2021 at 12:40 AM Sascha Hauer <sha@pengutronix.de> wrote:
> > > >
> > > > On Mon, Nov 01, 2021 at 11:33:07AM -0700, Trent Piepho wrote:
> > > > >    On Mon, Nov 1, 2021 at 2:01 AM Sascha Hauer <[1]sha@pengutronix.de> wrote:
> > >  >    >
> > > > >    > The phy is only optional as long as none is specified in the device
> > > > >    > tree. When there is one specified then it's no longer optional. We can't
> > > > >    > do the right thing here without checking the device tree. Given that
> > > > >    > it's simple to enable CONFIG_GENERIC_PHY I think this is the way to go.
> > > > >
> > > > >    But this force enables GENERIC_PHY when it's not needed.
> > > > >
> > > > >    There are commonly many device nodes in Linux dts files that are not used
> > > > >    by an enabled Barebox driver.  It's normal to turn off a driver that might
> > > > >    be or could be used.  Is it necessarily an error if a phy is present in
> > > > >    the dts but we don't wish to include support for it?
> > > >
> > > > We need to distinguish the cases "There is a phy specified, but the
> > > > reset defaults are good enough to go without a driver" and "There is a
> > > > phy specified and we need driver support for it". barebox can't know
> > > > this.
> > >
> > > Could we say that compiling barebox without CONFIG_GENERIC_PHY means
> > > the driver is not needed and compiling it with the driver means that
> > > is it?
> >
> > Please no. Enabling Kconfig options ideally gives you additional
> > features, but it should not break anything. Consider the case when you
> > need to enable CONFIG_GENERIC_PHY for something else like an LVDS phy or
> > whatever, you don't want to end up with broken USB support then and have
> > to choose whether USB or LVDS is working.
> 
> I thought your goal was to prevent less experienced users from
> building a non-functional Barebox and then not understanding what they
> had done.  Turning off a necessary driver and breaking USB while
> producing no warnings nor errors.  But I now gather I was mistaken and
> this isn't really the problem.
> 
> 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

All other errors are fatal.

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
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;

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.

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

Re: [PATCH] phy: core: Make 'phy_optional_get' return NULL when not implemented

[Trent Piepho]

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.

> 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.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] phy: core: Make 'phy_optional_get' return NULL when not implemented

[Sascha Hauer]

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

Re: [PATCH] phy: core: Make 'phy_optional_get' return NULL when not implemented

[Sascha Hauer]

On Wed, Oct 20, 2021 at 10:39:54AM +0200, Daniel Brát wrote:
> Make 'phy_optional_get' return NULL instead of ERR_PTR(-ENOSYS) when the
> CONFIG_GENERIC_PHY is not enabled. It makes more sense to return NULL instead
> of straight up throwing a error since the function has 'optional' in its name.
> This also fixes dwc2 usb driver which would previously fail inside its probe
> function despite being able to function without a phy just fine.
> 
> Signed-off-by: Daniel Brát <danek.brat@gmail.com>
> ---
>  include/linux/phy/phy.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

After I have received a patch which lets the EHCI driver depend on
CONFIG_GENERIC_PHY which effectively disables the EHCI in a bunch of
defconfigs I finally deciced to take this patch as-is.

Sascha

> 
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index 679ce6e42..321e546f9 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -195,7 +195,7 @@ static inline struct phy *phy_get(struct device_d *dev, const char *string)
>  static inline struct phy *phy_optional_get(struct device_d *dev,
>  					   const char *string)
>  {
> -	return ERR_PTR(-ENOSYS);
> +	return NULL;
>  }
>  
>  static inline struct phy *of_phy_get_by_phandle(struct device_d *dev,
> -- 
> 2.17.1
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox

-- 
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