| Message ID | 20220509074857.195302-4-clabbe@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | arm64: add ethernet to orange pi 3 | expand |
On Mon, May 09, 2022 at 07:48:54AM +0000, Corentin Labbe wrote: > Add entries for the 2 new phy-supply and phy-io-supply. > > Signed-off-by: Corentin Labbe <clabbe@baylibre.com> > --- > .../devicetree/bindings/net/ethernet-phy.yaml | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml > index ed1415a4381f..2a6b45ddf010 100644 > --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml > +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml > @@ -153,6 +153,16 @@ properties: > used. The absence of this property indicates the muxers > should be configured so that the external PHY is used. > > + phy-supply: > + description: > + Phandle to a regulator that provides power to the PHY. This > + regulator will be managed during the PHY power on/off sequence. > + > + phy-io-supply: > + description: > + Phandle to a regulator that provides power to the PHY. This > + regulator will be managed during the PHY power on/off sequence. If you need two differently named regulators, you need to make it clear how they differ. My _guess_ would be, you only need the io variant in order to talk to the PHY registers. However, to talk to a link partner, you need the other one enabled as well. Which means handling that regulator probably should be in the PHY driver, so it is enabled only when the interface is configured up. Andrew
Le Mon, May 09, 2022 at 02:17:27PM +0200, Andrew Lunn a écrit : > On Mon, May 09, 2022 at 07:48:54AM +0000, Corentin Labbe wrote: > > Add entries for the 2 new phy-supply and phy-io-supply. > > > > Signed-off-by: Corentin Labbe <clabbe@baylibre.com> > > --- > > .../devicetree/bindings/net/ethernet-phy.yaml | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml > > index ed1415a4381f..2a6b45ddf010 100644 > > --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml > > +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml > > @@ -153,6 +153,16 @@ properties: > > used. The absence of this property indicates the muxers > > should be configured so that the external PHY is used. > > > > + phy-supply: > > + description: > > + Phandle to a regulator that provides power to the PHY. This > > + regulator will be managed during the PHY power on/off sequence. > > + > > + phy-io-supply: > > + description: > > + Phandle to a regulator that provides power to the PHY. This > > + regulator will be managed during the PHY power on/off sequence. > > If you need two differently named regulators, you need to make it clear > how they differ. My _guess_ would be, you only need the io variant in > order to talk to the PHY registers. However, to talk to a link > partner, you need the other one enabled as well. Which means handling > that regulator probably should be in the PHY driver, so it is enabled > only when the interface is configured up. > If I enable only the IO one, stmmac fail to reset, so both are needed to be up. I tried also to keep the "phy" one handled by stmmac (by removing patch 2), this lead to the PHY to not be found by MDIO scan. Proably because stmmac enable the "phy" before the "phy-io". For the difference between the 2, according to my basic read (I am bad a it) of the shematic https://linux-sunxi.org/images/5/50/OrangePi_3_Schematics_v1.5.pdf phy-io(ephy-vdd25) seems to (at least) power MDIO bus.
On Mon, May 09, 2022 at 03:26:47PM +0200, LABBE Corentin wrote: > Le Mon, May 09, 2022 at 02:17:27PM +0200, Andrew Lunn a écrit : > > On Mon, May 09, 2022 at 07:48:54AM +0000, Corentin Labbe wrote: > > > Add entries for the 2 new phy-supply and phy-io-supply. > > > > > > Signed-off-by: Corentin Labbe <clabbe@baylibre.com> > > > --- > > > .../devicetree/bindings/net/ethernet-phy.yaml | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml > > > index ed1415a4381f..2a6b45ddf010 100644 > > > --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml > > > +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml > > > @@ -153,6 +153,16 @@ properties: > > > used. The absence of this property indicates the muxers > > > should be configured so that the external PHY is used. > > > > > > + phy-supply: > > > + description: > > > + Phandle to a regulator that provides power to the PHY. This > > > + regulator will be managed during the PHY power on/off sequence. > > > + > > > + phy-io-supply: > > > + description: > > > + Phandle to a regulator that provides power to the PHY. This > > > + regulator will be managed during the PHY power on/off sequence. > > > > If you need two differently named regulators, you need to make it clear > > how they differ. My _guess_ would be, you only need the io variant in > > order to talk to the PHY registers. However, to talk to a link > > partner, you need the other one enabled as well. Which means handling > > that regulator probably should be in the PHY driver, so it is enabled > > only when the interface is configured up. > > > > If I enable only the IO one, stmmac fail to reset, so both are needed to be up. > I tried also to keep the "phy" one handled by stmmac (by removing patch 2), this lead to the PHY to not be found by MDIO scan. > Proably because stmmac enable the "phy" before the "phy-io". > > For the difference between the 2, according to my basic read (I am bad a it) of the shematic > https://linux-sunxi.org/images/5/50/OrangePi_3_Schematics_v1.5.pdf > phy-io(ephy-vdd25) seems to (at least) power MDIO bus. So there is nothing in the data sheet of the RTL8211E to suggest you can uses these different power supplies independently. The naming 'phy-io-supply' is very specific to RTL8211E, but you are defining a generic binding here. I don't know the regulator binding, it is possible to make phy-supply a list? Andrew
On Mon, May 09, 2022 at 06:02:55PM +0200, Andrew Lunn wrote: > On Mon, May 09, 2022 at 03:26:47PM +0200, LABBE Corentin wrote: > > For the difference between the 2, according to my basic read (I am bad a it) of the shematic > > https://linux-sunxi.org/images/5/50/OrangePi_3_Schematics_v1.5.pdf > > phy-io(ephy-vdd25) seems to (at least) power MDIO bus. > So there is nothing in the data sheet of the RTL8211E to suggest you > can uses these different power supplies independently. The naming > 'phy-io-supply' is very specific to RTL8211E, but you are defining a > generic binding here. I don't know the regulator binding, it is > possible to make phy-supply a list? No, that's not a thing - the supplies are individual, named properties and even if there were a list we'd still want them to be named so it's clear what's going on.
> No, that's not a thing - the supplies are individual, named properties > and even if there were a list we'd still want them to be named so it's > clear what's going on. So we have a collection of regulators, varying in numbers between different PHYs, with different vendor names and purposes. In general, they all should be turned on. Yet we want them named so it is clear what is going on. Is there a generic solution here so that the phylib core can somehow enumerate them and turn them on, without actually knowing what they are called because they have vendor specific names in order to be clear what they are? There must be a solution to this, phylib cannot be the first subsystem to have this requirement, so if you could point to an example, that would be great. Thanks Andrew
On Mon, May 09, 2022 at 06:38:05PM +0200, Andrew Lunn wrote: > So we have a collection of regulators, varying in numbers between > different PHYs, with different vendor names and purposes. In general, > they all should be turned on. Yet we want them named so it is clear > what is going on. > Is there a generic solution here so that the phylib core can somehow > enumerate them and turn them on, without actually knowing what they > are called because they have vendor specific names in order to be > clear what they are? > There must be a solution to this, phylib cannot be the first subsystem > to have this requirement, so if you could point to an example, that > would be great. No, it's not really come up much before - generally things with regulator control that have generic drivers tend not to be sophisticated enough to have more than one supply, or to be on an enumerable bus where the power is part of the bus specification so have the power specified as part of the bus. You'd need to extend the regulator bindings to support parallel array of phandles and array of names properties like clocks have as an option like you were asking for, which would doubtless be fun for validation but is probably the thing here.
Le Mon, May 09, 2022 at 05:56:34PM +0100, Mark Brown a écrit : > On Mon, May 09, 2022 at 06:38:05PM +0200, Andrew Lunn wrote: > > > So we have a collection of regulators, varying in numbers between > > different PHYs, with different vendor names and purposes. In general, > > they all should be turned on. Yet we want them named so it is clear > > what is going on. > > > Is there a generic solution here so that the phylib core can somehow > > enumerate them and turn them on, without actually knowing what they > > are called because they have vendor specific names in order to be > > clear what they are? > > > There must be a solution to this, phylib cannot be the first subsystem > > to have this requirement, so if you could point to an example, that > > would be great. > > No, it's not really come up much before - generally things with > regulator control that have generic drivers tend not to be sophisticated > enough to have more than one supply, or to be on an enumerable bus where > the power is part of the bus specification so have the power specified > as part of the bus. You'd need to extend the regulator bindings to > support parallel array of phandles and array of names properties like > clocks have as an option like you were asking for, which would doubtless > be fun for validation but is probably the thing here. Does you mean something like this: diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 1e54a833f2cf..404f5b874b59 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -351,6 +351,32 @@ static void regulator_lock_dependent(struct regulator_dev *rdev, mutex_unlock(®ulator_list_mutex); } +/** + * of_get_regulator_from_list - get a regulator device node based on supply name + * from a DT regulators list + * @dev: Device pointer for the consumer (of regulator) device + * @supply: regulator supply name + * + * Extract the regulator device node corresponding to the supply name. + * returns the device node corresponding to the regulator if found, else + * returns NULL. + */ +static struct device_node *of_get_regulator_from_list(struct device *dev, + struct device_node *np, + const char *supply) +{ + int index, ret; + struct of_phandle_args regspec; + + index = of_property_match_string(np, "regulator-names", supply); + if (index >= 0) { + ret = of_parse_phandle_with_args(np, "regulators", NULL, index, ®spec); + if (ret == 0) + return regspec.np; + } + return NULL; +} + /** * of_get_child_regulator - get a child regulator device node * based on supply name @@ -362,17 +388,23 @@ static void regulator_lock_dependent(struct regulator_dev *rdev, * returns the device node corresponding to the regulator if found, else * returns NULL. */ -static struct device_node *of_get_child_regulator(struct device_node *parent, - const char *prop_name) +static struct device_node *of_get_child_regulator(struct device *dev, + struct device_node *parent, + const char *supply) { struct device_node *regnode = NULL; struct device_node *child = NULL; + char prop_name[64]; /* 64 is max size of property name */ + snprintf(prop_name, 64, "%s-supply", supply); for_each_child_of_node(parent, child) { + regnode = of_get_regulator_from_list(dev, child, supply); + if (regnode) + return regnode; regnode = of_parse_phandle(child, prop_name, 0); if (!regnode) { - regnode = of_get_child_regulator(child, prop_name); + regnode = of_get_child_regulator(dev, child, prop_name); if (regnode) goto err_node_put; } else { @@ -401,12 +433,15 @@ static struct device_node *of_get_regulator(struct device *dev, const char *supp char prop_name[64]; /* 64 is max size of property name */ dev_dbg(dev, "Looking up %s-supply from device tree\n", supply); + regnode = of_get_regulator_from_list(dev, dev->of_node, supply); + if (regnode) + return regnode; snprintf(prop_name, 64, "%s-supply", supply); regnode = of_parse_phandle(dev->of_node, prop_name, 0); if (!regnode) { - regnode = of_get_child_regulator(dev->of_node, prop_name); + regnode = of_get_child_regulator(dev, dev->of_node, supply); if (regnode) return regnode; And then for our case, a regulator_get_bulk will be needed. Does I well understood what you mean ?
On Wed, May 11, 2022 at 10:02:58AM +0200, LABBE Corentin wrote: > Le Mon, May 09, 2022 at 05:56:34PM +0100, Mark Brown a écrit : > > as part of the bus. You'd need to extend the regulator bindings to > > support parallel array of phandles and array of names properties like > > clocks have as an option like you were asking for, which would doubtless > > be fun for validation but is probably the thing here. > Does you mean something like this: ... > And then for our case, a regulator_get_bulk will be needed. > Does I well understood what you mean ? Yes.
On Mon, May 09, 2022 at 06:38:05PM +0200, Andrew Lunn wrote: > > No, that's not a thing - the supplies are individual, named properties > > and even if there were a list we'd still want them to be named so it's > > clear what's going on. > > So we have a collection of regulators, varying in numbers between > different PHYs, with different vendor names and purposes. In general, > they all should be turned on. Yet we want them named so it is clear > what is going on. In what order do we turn the supplies on? How much time in between each one? Does an external clock need to be running before or after (and how long after). Oh, and what about resets and the order and timing of them relative to everything else? This always happens in the same order. First, it's just one resource like a regulator or reset. Then one more. Then another device with some timing constraints. If we wanted a generic solution in DT, it would need to be able to describe any power sequencing waveform. But we don't have that because we don't want it. > Is there a generic solution here so that the phylib core can somehow > enumerate them and turn them on, without actually knowing what they > are called because they have vendor specific names in order to be > clear what they are? Other devices have specific compatibles so that the device can be identified and powered up. Once again, MDIO should not be special here. > There must be a solution to this, phylib cannot be the first subsystem > to have this requirement, so if you could point to an example, that > would be great. Well, no one seems to want to make non-discoverable devices on 'discoverable' buses work. Still an issue for PCI and USB. I thought MDIO had a solution here to probe any devices in the DT even if not discovered. Rob
diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml index ed1415a4381f..2a6b45ddf010 100644 --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml @@ -153,6 +153,16 @@ properties: used. The absence of this property indicates the muxers should be configured so that the external PHY is used. + phy-supply: + description: + Phandle to a regulator that provides power to the PHY. This + regulator will be managed during the PHY power on/off sequence. + + phy-io-supply: + description: + Phandle to a regulator that provides power to the PHY. This + regulator will be managed during the PHY power on/off sequence. + resets: maxItems: 1
Add entries for the 2 new phy-supply and phy-io-supply. Signed-off-by: Corentin Labbe <clabbe@baylibre.com> --- .../devicetree/bindings/net/ethernet-phy.yaml | 10 ++++++++++ 1 file changed, 10 insertions(+)