[2/6] dt-bindings: net: sun8i-a83t-emac: Add phy-io-supply property
diff mbox series

Message ID 20190820145343.29108-3-megous@megous.com
State Changes Requested
Headers show
Series
  • Add ethernet support for Orange Pi 3
Related show

Checks

Context Check Description
robh/checkpatch success

Commit Message

Ondřej Jirman Aug. 20, 2019, 2:53 p.m. UTC
From: Ondrej Jirman <megous@megous.com>

Some PHYs require separate power supply for I/O pins in some modes
of operation. Add phy-io-supply property, to allow enabling this
power supply.

Signed-off-by: Ondrej Jirman <megous@megous.com>
---
 .../devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml    | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Rob Herring Aug. 20, 2019, 4:20 p.m. UTC | #1
On Tue, Aug 20, 2019 at 9:53 AM <megous@megous.com> wrote:
>
> From: Ondrej Jirman <megous@megous.com>
>
> Some PHYs require separate power supply for I/O pins in some modes
> of operation. Add phy-io-supply property, to allow enabling this
> power supply.

Perhaps since this is new, such phys should have *-supply in their nodes.

>
> Signed-off-by: Ondrej Jirman <megous@megous.com>
> ---
>  .../devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml    | 4 ++++
>  1 file changed, 4 insertions(+)
Ondřej Jirman Aug. 20, 2019, 4:34 p.m. UTC | #2
On Tue, Aug 20, 2019 at 11:20:22AM -0500, Rob Herring wrote:
> On Tue, Aug 20, 2019 at 9:53 AM <megous@megous.com> wrote:
> >
> > From: Ondrej Jirman <megous@megous.com>
> >
> > Some PHYs require separate power supply for I/O pins in some modes
> > of operation. Add phy-io-supply property, to allow enabling this
> > power supply.
> 
> Perhaps since this is new, such phys should have *-supply in their nodes.

Yes, I just don't understand, since external ethernet phys are so common,
and they require power, how there's no fairly generic mechanism for this
already in the PHY subsystem, or somewhere?

It looks like other ethernet mac drivers also implement supplies on phys
on the EMAC nodes. Just grep phy-supply through dt-bindings/net.

Historical reasons, or am I missing something? It almost seems like I must
be missing something, since putting these properties to phy nodes
seems so obvious.

thank you and regards,
	Ondrej

> >
> > Signed-off-by: Ondrej Jirman <megous@megous.com>
> > ---
> >  .../devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml    | 4 ++++
> >  1 file changed, 4 insertions(+)
Rob Herring Aug. 20, 2019, 4:57 p.m. UTC | #3
On Tue, Aug 20, 2019 at 11:34 AM Ondřej Jirman <megous@megous.com> wrote:
>
> On Tue, Aug 20, 2019 at 11:20:22AM -0500, Rob Herring wrote:
> > On Tue, Aug 20, 2019 at 9:53 AM <megous@megous.com> wrote:
> > >
> > > From: Ondrej Jirman <megous@megous.com>
> > >
> > > Some PHYs require separate power supply for I/O pins in some modes
> > > of operation. Add phy-io-supply property, to allow enabling this
> > > power supply.
> >
> > Perhaps since this is new, such phys should have *-supply in their nodes.
>
> Yes, I just don't understand, since external ethernet phys are so common,
> and they require power, how there's no fairly generic mechanism for this
> already in the PHY subsystem, or somewhere?

Because generic mechanisms for this don't work. For example, what
happens when the 2 supplies need to be turned on in a certain order
and with certain timings? And then add in reset or control lines into
the mix... You can see in the bindings we already have some of that.

> It looks like other ethernet mac drivers also implement supplies on phys
> on the EMAC nodes. Just grep phy-supply through dt-bindings/net.
>
> Historical reasons, or am I missing something? It almost seems like I must
> be missing something, since putting these properties to phy nodes
> seems so obvious.

Things get added one by one and one new property isn't that
controversial. We've generally learned the lesson and avoid this
pattern now, but ethernet phys are one of the older bindings.

Rob
Ondřej Jirman Aug. 20, 2019, 10:36 p.m. UTC | #4
On Tue, Aug 20, 2019 at 11:57:06AM -0500, Rob Herring wrote:
> On Tue, Aug 20, 2019 at 11:34 AM Ondřej Jirman <megous@megous.com> wrote:
> >
> > On Tue, Aug 20, 2019 at 11:20:22AM -0500, Rob Herring wrote:
> > > On Tue, Aug 20, 2019 at 9:53 AM <megous@megous.com> wrote:
> > > >
> > > > From: Ondrej Jirman <megous@megous.com>
> > > >
> > > > Some PHYs require separate power supply for I/O pins in some modes
> > > > of operation. Add phy-io-supply property, to allow enabling this
> > > > power supply.
> > >
> > > Perhaps since this is new, such phys should have *-supply in their nodes.
> >
> > Yes, I just don't understand, since external ethernet phys are so common,
> > and they require power, how there's no fairly generic mechanism for this
> > already in the PHY subsystem, or somewhere?
> 
> Because generic mechanisms for this don't work. For example, what
> happens when the 2 supplies need to be turned on in a certain order
> and with certain timings? And then add in reset or control lines into
> the mix... You can see in the bindings we already have some of that.

I've looked at the emac bindings that have phy-supply, and don't see reason
why this can't be generic for the phy. Just like there's generic reset
properties for phys, now. Some bindings, like fsl-fec.txt even list
custom reset properties for phy as deprecated, and recommend using
generic ones.

From the point of the view of the emac driver, it just wants to power on/power
off the phy, and wait until it's ready to be communicated with.

It's probably better to have power supplies of the phy covered by generic
phy code, because then you don't have to duplicate all this special power
up logic in every emac driver, whenever a HW designer decides to combine
such emac with external phy that requires some special hadnling on powerup.

At the moment, this lack of flexibility is hacked around by adding multiple
regulators to the DTS, and making them dependent on each other (even if one
doesn't supply the other), just because this makes the regulator core driver
enable them all. Power up delays for the PHY are described as enable-ramp-delays
on the regulators (actual regulator ramp delay + wait time for PHY to initialize).

Basically just hacking the DT so that the Linux kernel in the end does what's
necessary, instead of DT describing the actual HW.

Adding a single supply property to the phy node, as you suggest will do nothing
to help this situation. It will just result in a more complicated dwmac-sun8i
driver and will not help anyone in the future.

So I think, maybe phy powerup should be moved to generic code, just like the
phy reset code was. Generic code can have multiple supplies and some generic
way to specify power up order and timings.

But I guess, this patch series is a dead end.

> > It looks like other ethernet mac drivers also implement supplies on phys
> > on the EMAC nodes. Just grep phy-supply through dt-bindings/net.
> >
> > Historical reasons, or am I missing something? It almost seems like I must
> > be missing something, since putting these properties to phy nodes
> > seems so obvious.
> 
> Things get added one by one and one new property isn't that
> controversial. We've generally learned the lesson and avoid this
> pattern now, but ethernet phys are one of the older bindings.

Understood. So maybe the solution suggested above would improve the situation
eventually?

regards,
	o.

> Rob
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml b/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
index 304f244e9ab5..782e202aa124 100644
--- a/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
+++ b/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
@@ -47,6 +47,10 @@  properties:
     description:
       PHY regulator
 
+  phy-io-supply:
+    description:
+      PHY I/O pins regulator
+
 required:
   - compatible
   - reg