This is continuation of the work done by Corentin:
https://lore.kernel.org/linux-sunxi/[email protected]/
In short, Orange Pi 3 and Orange Pi One Plus boards have ethernet PHYs
which are powered by two voltage regulators. They have to be powered on in
correct order or otherwise they are not functional. Please see link above
for previous discussion on how to achieve that.
Best regards,
Jernej
changes since v1:
- Add regulator_bulk_get_all for ease handling of PHY regulators
- Removed all conversion patches to keep DT compatibility.
Changes since v2:
- removed use of regulator-names and regulators list.
Changes since v3:
- fixes kbuild robot report
Changes since v4:
- dropped merged patches
- reworked PHY powering on/off patch
- added Orange Pi One Plus patch, since it has same issue
Corentin Labbe (1):
phy: handle optional regulator for PHY
Jernej Skrabec (1):
arm64: dts: allwinner: orange-pi-one-plus: Fix ethernet
Ondrej Jirman (1):
arm64: dts: allwinner: orange-pi-3: Enable ethernet
.../dts/allwinner/sun50i-h6-orangepi-3.dts | 40 ++++++++++++++
.../allwinner/sun50i-h6-orangepi-one-plus.dts | 29 +++++++---
drivers/net/mdio/fwnode_mdio.c | 53 ++++++++++++++++++-
drivers/net/phy/phy_device.c | 6 +++
include/linux/phy.h | 3 ++
5 files changed, 122 insertions(+), 9 deletions(-)
--
2.43.0
From: Ondrej Jirman <[email protected]>
Orange Pi 3 has two regulators that power the Realtek RTL8211E
PHY. According to the datasheet, both regulators need to be enabled
at the same time, or that "phy-io" should be enabled slightly earlier
than "ephy" regulator.
RTL8211E/RTL8211EG datasheet says:
Note 4: 2.5V (or 1.8/1.5V) RGMII power should be risen simultaneously
or slightly earlier than 3.3V power. Rising 2.5V (or 1.8/1.5V) power
later than 3.3V power may lead to errors.
Signed-off-by: Ondrej Jirman <[email protected]>
Signed-off-by: Corentin Labbe <[email protected]>
Signed-off-by: Jernej Skrabec <[email protected]>
---
.../dts/allwinner/sun50i-h6-orangepi-3.dts | 40 +++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
index 6fc65e8db220..6ed8613a3169 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
@@ -13,6 +13,7 @@ / {
compatible = "xunlong,orangepi-3", "allwinner,sun50i-h6";
aliases {
+ ethernet0 = &emac;
serial0 = &uart0;
serial1 = &uart1;
};
@@ -55,6 +56,17 @@ led-1 {
};
};
+ reg_gmac_2v5: gmac-2v5 {
+ compatible = "regulator-fixed";
+ regulator-name = "gmac-2v5";
+ regulator-min-microvolt = <2500000>;
+ regulator-max-microvolt = <2500000>;
+ enable-active-high;
+ gpio = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* PD6 */
+ off-on-delay-us = <100000>;
+ vin-supply = <®_vcc5v>;
+ };
+
reg_vcc5v: vcc5v {
/* board wide 5V supply directly from the DC jack */
compatible = "regulator-fixed";
@@ -113,6 +125,33 @@ &ehci3 {
status = "okay";
};
+&emac {
+ pinctrl-names = "default";
+ pinctrl-0 = <&ext_rgmii_pins>;
+ phy-mode = "rgmii-id";
+ phy-handle = <&ext_rgmii_phy>;
+ status = "okay";
+};
+
+&mdio {
+ ext_rgmii_phy: ethernet-phy@1 {
+ compatible = "ethernet-phy-ieee802.3-c22";
+ reg = <1>;
+ /*
+ * The board uses 2.5V RGMII signalling. Power sequence to enable
+ * the phy is to enable GMAC-2V5 and GMAC-3V (aldo2) power rails
+ * at the same time and to wait 100ms. The driver enables phy-io
+ * first. Delay is achieved with enable-ramp-delay on reg_aldo2.
+ */
+ phy-io-supply = <®_gmac_2v5>;
+ ephy-supply = <®_aldo2>;
+
+ reset-gpios = <&pio 3 14 GPIO_ACTIVE_LOW>; /* PD14 */
+ reset-assert-us = <15000>;
+ reset-deassert-us = <40000>;
+ };
+};
+
&gpu {
mali-supply = <®_dcdcc>;
status = "okay";
@@ -211,6 +250,7 @@ reg_aldo2: aldo2 {
regulator-min-microvolt = <3300000>;
regulator-max-microvolt = <3300000>;
regulator-name = "vcc33-audio-tv-ephy-mac";
+ regulator-enable-ramp-delay = <100000>;
};
/* ALDO3 is shorted to CLDO1 */
--
2.43.0
From: Corentin Labbe <[email protected]>
Add handling of optional regulators for PHY.
Regulators need to be enabled before PHY scanning, so MDIO bus
initiate this task.
Signed-off-by: Corentin Labbe <[email protected]>
Signed-off-by: Jernej Skrabec <[email protected]>
---
drivers/net/mdio/fwnode_mdio.c | 53 ++++++++++++++++++++++++++++++++--
drivers/net/phy/phy_device.c | 6 ++++
include/linux/phy.h | 3 ++
3 files changed, 60 insertions(+), 2 deletions(-)
diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index fd02f5cbc853..bd5a27eaf40c 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -11,6 +11,7 @@
#include <linux/of.h>
#include <linux/phy.h>
#include <linux/pse-pd/pse.h>
+#include <linux/regulator/consumer.h>
MODULE_AUTHOR("Calvin Johnson <[email protected]>");
MODULE_LICENSE("GPL");
@@ -58,6 +59,40 @@ fwnode_find_mii_timestamper(struct fwnode_handle *fwnode)
return register_mii_timestamper(arg.np, arg.args[0]);
}
+static int
+fwnode_regulator_get_bulk_enabled(struct device *dev,
+ struct fwnode_handle *fwnode,
+ struct regulator_bulk_data **consumers)
+{
+ struct device_node *np;
+ int ret, reg_cnt;
+
+ np = to_of_node(fwnode);
+ if (!np)
+ return 0;
+
+ reg_cnt = of_regulator_bulk_get_all(dev, np, consumers);
+ if (reg_cnt < 0) {
+ ret = reg_cnt;
+ goto clean_consumers;
+ }
+
+ if (reg_cnt == 0)
+ return 0;
+
+ ret = regulator_bulk_enable(reg_cnt, *consumers);
+ if (ret)
+ goto clean_consumers;
+
+ return reg_cnt;
+
+clean_consumers:
+ kfree(*consumers);
+ *consumers = NULL;
+
+ return ret;
+}
+
int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
struct phy_device *phy,
struct fwnode_handle *child, u32 addr)
@@ -113,12 +148,13 @@ EXPORT_SYMBOL(fwnode_mdiobus_phy_device_register);
int fwnode_mdiobus_register_phy(struct mii_bus *bus,
struct fwnode_handle *child, u32 addr)
{
+ struct regulator_bulk_data *consumers = NULL;
struct mii_timestamper *mii_ts = NULL;
struct pse_control *psec = NULL;
struct phy_device *phy;
+ int rc, reg_cnt;
bool is_c45;
u32 phy_id;
- int rc;
psec = fwnode_find_pse_control(child);
if (IS_ERR(psec))
@@ -130,6 +166,12 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
goto clean_pse;
}
+ reg_cnt = fwnode_regulator_get_bulk_enabled(&bus->dev, child, &consumers);
+ if (reg_cnt < 0) {
+ rc = reg_cnt;
+ goto clean_mii_ts;
+ }
+
is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
if (is_c45 || fwnode_get_phy_id(child, &phy_id))
phy = get_phy_device(bus, addr, is_c45);
@@ -137,9 +179,12 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
phy = phy_device_create(bus, addr, phy_id, 0, NULL);
if (IS_ERR(phy)) {
rc = PTR_ERR(phy);
- goto clean_mii_ts;
+ goto clean_regulators;
}
+ phy->regulator_cnt = reg_cnt;
+ phy->consumers = consumers;
+
if (is_acpi_node(child)) {
phy->irq = bus->irq[addr];
@@ -174,6 +219,10 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
clean_phy:
phy_device_free(phy);
+clean_regulators:
+ if (reg_cnt > 0)
+ regulator_bulk_disable(reg_cnt, consumers);
+ kfree(consumers);
clean_mii_ts:
unregister_mii_timestamper(mii_ts);
clean_pse:
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 2ce74593d6e4..31b6913ceed1 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -31,6 +31,7 @@
#include <linux/phy_led_triggers.h>
#include <linux/pse-pd/pse.h>
#include <linux/property.h>
+#include <linux/regulator/consumer.h>
#include <linux/rtnetlink.h>
#include <linux/sfp.h>
#include <linux/skbuff.h>
@@ -3400,6 +3401,11 @@ static int phy_remove(struct device *dev)
phydev->drv = NULL;
+ if (phydev->regulator_cnt > 0)
+ regulator_bulk_disable(phydev->regulator_cnt, phydev->consumers);
+
+ kfree(phydev->consumers);
+
return 0;
}
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 3cc52826f18e..832cb2d4f76a 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -757,6 +757,9 @@ struct phy_device {
void (*phy_link_change)(struct phy_device *phydev, bool up);
void (*adjust_link)(struct net_device *dev);
+ int regulator_cnt;
+ struct regulator_bulk_data *consumers;
+
#if IS_ENABLED(CONFIG_MACSEC)
/* MACsec management functions */
const struct macsec_ops *macsec_ops;
--
2.43.0
Orange Pi One Plus has two regulators that power the Realtek RTL8211E
PHY. According to the datasheet, both regulators need to be enabled
at the same time, or that "phy-io" should be enabled slightly earlier
than "ephy" regulator.
RTL8211E/RTL8211EG datasheet says:
Note 4: 2.5V (or 1.8/1.5V) RGMII power should be risen simultaneously
or slightly earlier than 3.3V power. Rising 2.5V (or 1.8/1.5V) power
later than 3.3V power may lead to errors.
Original submission ignored these rules, so it works in some cases but
not all. On top of that, regulator voltages don't reflect actual ones
in hardware. Rework ethernet and PHY nodes to properly reflect HW.
Fixes: 7ee32a17e0d6 ("arm64: dts: allwinner: h6: orangepi-one-plus: Enable ethernet")
Signed-off-by: Jernej Skrabec <[email protected]>
---
.../allwinner/sun50i-h6-orangepi-one-plus.dts | 29 ++++++++++++++-----
1 file changed, 22 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts
index 29a081e72a9b..9c76eecaacce 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts
@@ -12,15 +12,15 @@ aliases {
ethernet0 = &emac;
};
- reg_gmac_3v3: gmac-3v3 {
+ reg_gmac_2v5: gmac-2v5 {
compatible = "regulator-fixed";
- regulator-name = "vcc-gmac-3v3";
- regulator-min-microvolt = <3300000>;
- regulator-max-microvolt = <3300000>;
- startup-delay-us = <100000>;
+ regulator-name = "gmac-2v5";
+ regulator-min-microvolt = <2500000>;
+ regulator-max-microvolt = <2500000>;
enable-active-high;
gpio = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* PD6 */
- vin-supply = <®_aldo2>;
+ off-on-delay-us = <100000>;
+ vin-supply = <®_vcc5v>;
};
};
@@ -29,7 +29,6 @@ &emac {
pinctrl-0 = <&ext_rgmii_pins>;
phy-mode = "rgmii-id";
phy-handle = <&ext_rgmii_phy>;
- phy-supply = <®_gmac_3v3>;
allwinner,rx-delay-ps = <200>;
allwinner,tx-delay-ps = <200>;
status = "okay";
@@ -39,5 +38,21 @@ &mdio {
ext_rgmii_phy: ethernet-phy@1 {
compatible = "ethernet-phy-ieee802.3-c22";
reg = <1>;
+ /*
+ * The board uses 2.5V RGMII signalling. Power sequence to enable
+ * the phy is to enable GMAC-2V5 and GMAC-3V (aldo2) power rails
+ * at the same time and to wait 100ms. The driver enables phy-io
+ * first. Delay is achieved with enable-ramp-delay on reg_aldo2.
+ */
+ phy-io-supply = <®_gmac_2v5>;
+ ephy-supply = <®_aldo2>;
+
+ reset-gpios = <&pio 3 14 GPIO_ACTIVE_LOW>; /* PD14 */
+ reset-assert-us = <15000>;
+ reset-deassert-us = <40000>;
};
};
+
+®_aldo2 {
+ regulator-enable-ramp-delay = <100000>;
+};
--
2.43.0
> +static int
> +fwnode_regulator_get_bulk_enabled(struct device *dev,
> + struct fwnode_handle *fwnode,
> + struct regulator_bulk_data **consumers)
> +{
> + struct device_node *np;
> + int ret, reg_cnt;
> +
> + np = to_of_node(fwnode);
> + if (!np)
> + return 0;
> +
> + reg_cnt = of_regulator_bulk_get_all(dev, np, consumers);
> + if (reg_cnt < 0) {
> + ret = reg_cnt;
> + goto clean_consumers;
> + }
> +
> + if (reg_cnt == 0)
> + return 0;
I've not used regulators much, but i think you can combine these two
into one. Its guaranteed *consumer is NULL if reg_cnt == 0. And
kfree() is happy with a NULL pointer.
> +
> + ret = regulator_bulk_enable(reg_cnt, *consumers);
> + if (ret)
> + goto clean_consumers;
I would expect this to be part of mdio_device_reset(), same as the
GPIO, and reset controller, but first obviously. And parsing DT should
happen in a similar place to parsing the reset GPIO and reset
controller.
Andrew
On Wed, Dec 20, 2023 at 09:35:35PM +0100, Jernej Skrabec wrote:
> From: Corentin Labbe <[email protected]>
>
> Add handling of optional regulators for PHY.
>
> Regulators need to be enabled before PHY scanning, so MDIO bus
> initiate this task.
>
> Signed-off-by: Corentin Labbe <[email protected]>
> Signed-off-by: Jernej Skrabec <[email protected]>
Hi Jernej,
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 3cc52826f18e..832cb2d4f76a 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -757,6 +757,9 @@ struct phy_device {
> void (*phy_link_change)(struct phy_device *phydev, bool up);
> void (*adjust_link)(struct net_device *dev);
>
> + int regulator_cnt;
> + struct regulator_bulk_data *consumers;
Please add these two new fields to the kernel doc
for struct phy_device which appears a above this hunk in phy.h.
> +
> #if IS_ENABLED(CONFIG_MACSEC)
> /* MACsec management functions */
> const struct macsec_ops *macsec_ops;
--
pw-bot: changes-requested
On Wed, Dec 20, 2023 at 09:35:35PM +0100, Jernej Skrabec wrote:
> From: Corentin Labbe <[email protected]>
>
> Add handling of optional regulators for PHY.
>
> Regulators need to be enabled before PHY scanning, so MDIO bus
> initiate this task.
>
> Signed-off-by: Corentin Labbe <[email protected]>
> Signed-off-by: Jernej Skrabec <[email protected]>
> ---
> drivers/net/mdio/fwnode_mdio.c | 53 ++++++++++++++++++++++++++++++++--
> drivers/net/phy/phy_device.c | 6 ++++
> include/linux/phy.h | 3 ++
> 3 files changed, 60 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> index fd02f5cbc853..bd5a27eaf40c 100644
> --- a/drivers/net/mdio/fwnode_mdio.c
> +++ b/drivers/net/mdio/fwnode_mdio.c
> @@ -11,6 +11,7 @@
> #include <linux/of.h>
> #include <linux/phy.h>
> #include <linux/pse-pd/pse.h>
> +#include <linux/regulator/consumer.h>
>
> MODULE_AUTHOR("Calvin Johnson <[email protected]>");
> MODULE_LICENSE("GPL");
> @@ -58,6 +59,40 @@ fwnode_find_mii_timestamper(struct fwnode_handle *fwnode)
> return register_mii_timestamper(arg.np, arg.args[0]);
> }
>
> +static int
> +fwnode_regulator_get_bulk_enabled(struct device *dev,
> + struct fwnode_handle *fwnode,
> + struct regulator_bulk_data **consumers)
This seems to be a bad name for something living in fwnode_mdio.c - it
looks like something the regulator core should be providing.
> +clean_regulators:
> + if (reg_cnt > 0)
> + regulator_bulk_disable(reg_cnt, consumers);
> + kfree(consumers);
and there really should be a function that undoes the effects of
fwnode_regulator_get_bulk_enabled() rather than having it open-coded,
especially as:
> @@ -3400,6 +3401,11 @@ static int phy_remove(struct device *dev)
>
> phydev->drv = NULL;
>
> + if (phydev->regulator_cnt > 0)
> + regulator_bulk_disable(phydev->regulator_cnt, phydev->consumers);
> +
> + kfree(phydev->consumers);
> +
it also appears here.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Dec 21, 2023 at 10:06:50AM +0100, Andrew Lunn wrote:
> I've not used regulators much, but i think you can combine these two
> into one. Its guaranteed *consumer is NULL if reg_cnt == 0. And
> kfree() is happy with a NULL pointer.
kfree(NULL) is valid (and is a no-op.)
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!