[PATCH v2 0/5] arm64: add ethernet to orange pi 3

[PATCH v2 0/5] arm64: add ethernet to orange pi 3

[Corentin Labbe]

Hello

2 sunxi board still does not have ethernet working, orangepi 1+ and
orangepi 3.
This is due to the fact thoses boards have a PHY which need 2 regulators.

A first attempt was made to support them was made by adding support in
stmmac driver:
https://lore.kernel.org/lkml/20190820145343.29108-6-megous@megous.com/
Proposal rejected, since regulators need to be handled by the PHY core.

This serie try to handle this.

This serie was tested on whole range of board and PHY architecture:
- internal PHY
  * sun8i-h3-orangepi-pc
- external PHY
  * sun50i-h6-pine-h64
  * sun8i-r40-bananapi-m2-ultra
  * sun8i-a83t-bananapi-m3
  * sun50i-a64-bananapi-m64
  * sun50i-h6-orangepi-3
  * sun50i-h5-nanopi-neo-plus2

The resume/suspend of PHY was tested.

Regards

changes since v1:
- Add regulator_bulk_get_all for ease handling of PHY regulators
- Removed all convertion patchs to keep DT compatibility.

Corentin Labbe (4):
  regulator: Add of_get_regulator_from_list
  regulator: Add regulator_bulk_get_all
  phy: handle optional regulator for PHY
  dt-bindings: net: Add documentation for optional regulators

Ondřej Jirman (1):
  arm64: dts: allwinner: orange-pi-3: Enable ethernet

 .../devicetree/bindings/net/ethernet-phy.yaml |  9 ++
 .../dts/allwinner/sun50i-h6-orangepi-3.dts    | 38 ++++++++
 drivers/net/mdio/Kconfig                      |  1 +
 drivers/net/mdio/fwnode_mdio.c                | 34 ++++++-
 drivers/net/phy/phy_device.c                  | 10 ++
 drivers/regulator/core.c                      | 93 ++++++++++++++++++-
 include/linux/phy.h                           |  3 +
 include/linux/regulator/consumer.h            |  2 +
 8 files changed, 182 insertions(+), 8 deletions(-)

-- 
2.35.1

[PATCH v2 1/5] regulator: Add of_get_regulator_from_list

[Corentin Labbe]

of_get_regulator_from_list() permits to get a regulator from a
regulators list.
Then add support for such list in of_get_regulator()

Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
---
 drivers/regulator/core.c | 44 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 1e54a833f2cf..09578c3595de 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -351,6 +351,33 @@ static void regulator_lock_dependent(struct regulator_dev *rdev,
 	mutex_unlock(&regulator_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
+ * @np: The device node where to search for regulators list
+ * @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)
+{
+	struct of_phandle_args regspec;
+	int index, ret;
+
+	index = of_property_match_string(np, "regulator-names", supply);
+	if (index >= 0) {
+		ret = of_parse_phandle_with_args(np, "regulators", NULL, index, &regspec);
+		if (ret == 0)
+			return regspec.np;
+	}
+	return NULL;
+}
+
 /**
  * of_get_child_regulator - get a child regulator device node
  * based on supply name
@@ -362,17 +389,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 +434,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;
 
-- 
2.35.1

[PATCH v2 2/5] regulator: Add regulator_bulk_get_all

[Corentin Labbe]

It work exactly like regulator_bulk_get() but instead of working on a
provided list of names, it get names from a regulators list.

Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
---
 drivers/regulator/core.c           | 49 ++++++++++++++++++++++++++++++
 include/linux/regulator/consumer.h |  2 ++
 2 files changed, 51 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 09578c3595de..719ce9a0db1b 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -4849,6 +4849,55 @@ static void regulator_bulk_enable_async(void *data, async_cookie_t cookie)
 	bulk->ret = regulator_enable(bulk->consumer);
 }
 
+/**
+ * regulator_bulk_get_all - get multiple regulator consumers
+ *
+ * @dev:           Device to supply
+ * @consumers:     Configuration of consumers; clients are stored here.
+ *
+ * @return number of regulators on success, an errno on failure.
+ *
+ * This helper function allows drivers to get several regulator
+ * consumers in one operation.  If any of the regulators cannot be
+ * acquired then any regulators that were allocated will be freed
+ * before returning to the caller.
+ */
+int regulator_bulk_get_all(struct device *dev, struct device_node *np,
+			   struct regulator_bulk_data **consumers)
+{
+	int num_consumers;
+	int i, ret;
+	struct regulator *tmp;
+	const char *p;
+
+	num_consumers = of_property_count_elems_of_size(np, "regulators",
+							sizeof(phandle));
+	if (num_consumers <= 0)
+		return num_consumers;
+
+	ret = of_property_count_strings(np, "regulator-names");
+	if (ret != num_consumers) {
+		dev_err(dev, "regulators and regulator-names does not have the same size\n");
+		return -EINVAL;
+	}
+	*consumers = kmalloc_array(num_consumers, sizeof(struct regulator_bulk_data), GFP_KERNEL);
+	if (!*consumers)
+		return -ENOMEM;
+	for (i = 0; i < num_consumers; i++) {
+		ret = of_property_read_string_helper(np, "regulator-names", &p, 1, i);
+		if (ret <= 0)
+			goto error;
+		tmp = regulator_get(dev, p);
+		(*consumers)[i].consumer = tmp;
+	}
+	return num_consumers;
+error:
+	while (--i >= 0)
+		regulator_put(consumers[i]->consumer);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(regulator_bulk_get_all);
+
 /**
  * regulator_bulk_enable - enable multiple regulator consumers
  *
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index bbf6590a6dec..b9b1d1cbdd07 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -238,6 +238,8 @@ int regulator_disable_deferred(struct regulator *regulator, int ms);
 
 int __must_check regulator_bulk_get(struct device *dev, int num_consumers,
 				    struct regulator_bulk_data *consumers);
+int __must_check regulator_bulk_get_all(struct device *dev, struct device_node *np,
+					struct regulator_bulk_data **consumers);
 int __must_check devm_regulator_bulk_get(struct device *dev, int num_consumers,
 					 struct regulator_bulk_data *consumers);
 int __must_check regulator_bulk_enable(int num_consumers,
-- 
2.35.1

[PATCH v2 3/5] phy: handle optional regulator for PHY

[Corentin Labbe]

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 <clabbe@baylibre.com>
---
 drivers/net/mdio/Kconfig       |  1 +
 drivers/net/mdio/fwnode_mdio.c | 34 ++++++++++++++++++++++++++++++----
 drivers/net/phy/phy_device.c   | 10 ++++++++++
 include/linux/phy.h            |  3 +++
 4 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/drivers/net/mdio/Kconfig b/drivers/net/mdio/Kconfig
index bfa16826a6e1..3f8098fac74b 100644
--- a/drivers/net/mdio/Kconfig
+++ b/drivers/net/mdio/Kconfig
@@ -22,6 +22,7 @@ config MDIO_BUS
 config FWNODE_MDIO
 	def_tristate PHYLIB
 	depends on (ACPI || OF) || COMPILE_TEST
+	depends on REGULATOR
 	select FIXED_PHY
 	help
 	  FWNODE MDIO bus (Ethernet PHY) accessors
diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index 1c1584fca632..c97ccb0863f9 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -10,6 +10,7 @@
 #include <linux/fwnode_mdio.h>
 #include <linux/of.h>
 #include <linux/phy.h>
+#include <linux/regulator/consumer.h>
 
 MODULE_AUTHOR("Calvin Johnson <calvin.johnson@oss.nxp.com>");
 MODULE_LICENSE("GPL");
@@ -94,7 +95,10 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 	struct phy_device *phy;
 	bool is_c45 = false;
 	u32 phy_id;
-	int rc;
+	int rc, reg_cnt = 0;
+	struct regulator_bulk_data *consumers;
+	struct device_node *nchild = NULL;
+	u32 reg;
 
 	mii_ts = fwnode_find_mii_timestamper(child);
 	if (IS_ERR(mii_ts))
@@ -105,15 +109,33 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 	if (rc >= 0)
 		is_c45 = true;
 
+	for_each_child_of_node(bus->dev.of_node, nchild) {
+		of_property_read_u32(nchild, "reg", &reg);
+		if (reg != addr)
+			continue;
+		reg_cnt = regulator_bulk_get_all(&bus->dev, nchild, &consumers);
+		if (reg_cnt > 0) {
+			rc = regulator_bulk_enable(reg_cnt, consumers);
+			if (rc)
+				return rc;
+		} else {
+			dev_err(&bus->dev, "Fail to regulator_bulk_get_all err=%d\n", reg_cnt);
+		}
+	}
+
 	if (is_c45 || fwnode_get_phy_id(child, &phy_id))
 		phy = get_phy_device(bus, addr, is_c45);
 	else
 		phy = phy_device_create(bus, addr, phy_id, 0, NULL);
 	if (IS_ERR(phy)) {
 		unregister_mii_timestamper(mii_ts);
-		return PTR_ERR(phy);
+		rc = PTR_ERR(phy);
+		goto error;
 	}
 
+	phy->regulator_cnt = reg_cnt;
+	phy->consumers = consumers;
+
 	if (is_acpi_node(child)) {
 		phy->irq = bus->irq[addr];
 
@@ -127,14 +149,14 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 		if (rc) {
 			phy_device_free(phy);
 			fwnode_handle_put(phy->mdio.dev.fwnode);
-			return rc;
+			goto error;
 		}
 	} else if (is_of_node(child)) {
 		rc = fwnode_mdiobus_phy_device_register(bus, phy, child, addr);
 		if (rc) {
 			unregister_mii_timestamper(mii_ts);
 			phy_device_free(phy);
-			return rc;
+			goto error;
 		}
 	}
 
@@ -145,5 +167,9 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 	if (mii_ts)
 		phy->mii_ts = mii_ts;
 	return 0;
+error:
+	if (reg_cnt > 0)
+		regulator_bulk_disable(reg_cnt, consumers);
+	return rc;
 }
 EXPORT_SYMBOL(fwnode_mdiobus_register_phy);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 431a8719c635..711919e40ef7 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -27,6 +27,7 @@
 #include <linux/phy.h>
 #include <linux/phy_led_triggers.h>
 #include <linux/property.h>
+#include <linux/regulator/consumer.h>
 #include <linux/sfp.h>
 #include <linux/skbuff.h>
 #include <linux/slab.h>
@@ -1785,6 +1786,9 @@ int phy_suspend(struct phy_device *phydev)
 	if (!ret)
 		phydev->suspended = true;
 
+	if (phydev->regulator_cnt > 0)
+		regulator_bulk_disable(phydev->regulator_cnt, phydev->consumers);
+
 	return ret;
 }
 EXPORT_SYMBOL(phy_suspend);
@@ -1811,6 +1815,12 @@ int phy_resume(struct phy_device *phydev)
 {
 	int ret;
 
+	if (phydev->regulator_cnt > 0) {
+		ret = regulator_bulk_enable(phydev->regulator_cnt, phydev->consumers);
+		if (ret)
+			return ret;
+	}
+
 	mutex_lock(&phydev->lock);
 	ret = __phy_resume(phydev);
 	mutex_unlock(&phydev->lock);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 508f1149665b..ef4e0ce67194 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -704,6 +704,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.35.1

[PATCH v2 4/5] dt-bindings: net: Add documentation for optional regulators

[Corentin Labbe]

Add entries for the new optional regulators.

Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
---
 Documentation/devicetree/bindings/net/ethernet-phy.yaml | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index ed1415a4381f..bd59e5cc0a55 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -153,6 +153,15 @@ properties:
       used. The absence of this property indicates the muxers
       should be configured so that the external PHY is used.
 
+  regulators:
+    description:
+       List of phandle to regulators needed for the PHY
+
+  regulator-names:
+    description:
+      List of regulator name strings sorted in the same order as the
+      regulators property.
+
   resets:
     maxItems: 1
 
-- 
2.35.1

[PATCH v2 5/5] arm64: dts: allwinner: orange-pi-3: Enable ethernet

[Corentin Labbe]

From: Ondřej Jirman <megi@xff.cz>

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

The timing is set in DT via startup-delay-us.

Signed-off-by: Ondrej Jirman <megi@xff.cz>
Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
---
 .../dts/allwinner/sun50i-h6-orangepi-3.dts    | 38 +++++++++++++++++++
 1 file changed, 38 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 c45d7b7fb39a..c3749b7302ba 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,15 @@ 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 */
+	};
+
 	reg_vcc5v: vcc5v {
 		/* board wide 5V supply directly from the DC jack */
 		compatible = "regulator-fixed";
@@ -113,6 +123,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.
+		 */
+		regulators = <&reg_gmac_2v5>, <&reg_aldo2>;
+		regulator-names = "phy-io", "phy";
+
+		reset-gpios = <&pio 3 14 GPIO_ACTIVE_LOW>; /* PD14 */
+		reset-assert-us = <15000>;
+		reset-deassert-us = <40000>;
+	};
+};
+
 &gpu {
 	mali-supply = <&reg_dcdcc>;
 	status = "okay";
@@ -211,6 +248,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.35.1

Re: [PATCH v2 4/5] dt-bindings: net: Add documentation for optional regulators

[Rob Herring]

On Wed, 18 May 2022 20:09:38 +0000, Corentin Labbe wrote:
> Add entries for the new optional regulators.
> 
> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> ---
>  Documentation/devicetree/bindings/net/ethernet-phy.yaml | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/net/ethernet-phy.yaml:149:8: [warning] wrong indentation: expected 6 but found 7 (indentation)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v2 4/5] dt-bindings: net: Add documentation for optional regulators

[Krzysztof Kozlowski]

On 18/05/2022 22:09, Corentin Labbe wrote:
> Add entries for the new optional regulators.

Please explain why do you need it.

> 
> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> ---
>  Documentation/devicetree/bindings/net/ethernet-phy.yaml | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> index ed1415a4381f..bd59e5cc0a55 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> @@ -153,6 +153,15 @@ properties:
>        used. The absence of this property indicates the muxers
>        should be configured so that the external PHY is used.
>  
> +  regulators:
> +    description:
> +       List of phandle to regulators needed for the PHY

I don't understand that... is your PHY defining the regulators or using
supplies? If it needs a regulator (as a supply), you need to document
supplies, using existing bindings.

> +
> +  regulator-names:
> +    description:
> +      List of regulator name strings sorted in the same order as the
> +      regulators property.
> +
>    resets:
>      maxItems: 1
>  


Best regards,
Krzysztof

Re: [PATCH v2 4/5] dt-bindings: net: Add documentation for optional regulators

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 530 bytes --]

On Thu, May 19, 2022 at 11:55:28AM +0200, Krzysztof Kozlowski wrote:
> On 18/05/2022 22:09, Corentin Labbe wrote:

> > +  regulators:
> > +    description:
> > +       List of phandle to regulators needed for the PHY

> I don't understand that... is your PHY defining the regulators or using
> supplies? If it needs a regulator (as a supply), you need to document
> supplies, using existing bindings.

They're trying to have a generic driver which works with any random PHY
so the binding has no idea what supplies it might need.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 4/5] dt-bindings: net: Add documentation for optional regulators

[Krzysztof Kozlowski]

On 19/05/2022 13:31, Mark Brown wrote:
> On Thu, May 19, 2022 at 11:55:28AM +0200, Krzysztof Kozlowski wrote:
>> On 18/05/2022 22:09, Corentin Labbe wrote:
> 
>>> +  regulators:
>>> +    description:
>>> +       List of phandle to regulators needed for the PHY
> 
>> I don't understand that... is your PHY defining the regulators or using
>> supplies? If it needs a regulator (as a supply), you need to document
>> supplies, using existing bindings.
> 
> They're trying to have a generic driver which works with any random PHY
> so the binding has no idea what supplies it might need.

OK, that makes sense, but then question is why not using existing
naming, so "supplies" and "supply-names"?

Best regards,
Krzysztof

Re: [PATCH v2 4/5] dt-bindings: net: Add documentation for optional regulators

[Andrew Lunn]

On Thu, May 19, 2022 at 01:33:21PM +0200, Krzysztof Kozlowski wrote:
> On 19/05/2022 13:31, Mark Brown wrote:
> > On Thu, May 19, 2022 at 11:55:28AM +0200, Krzysztof Kozlowski wrote:
> >> On 18/05/2022 22:09, Corentin Labbe wrote:
> > 
> >>> +  regulators:
> >>> +    description:
> >>> +       List of phandle to regulators needed for the PHY
> > 
> >> I don't understand that... is your PHY defining the regulators or using
> >> supplies? If it needs a regulator (as a supply), you need to document
> >> supplies, using existing bindings.
> > 
> > They're trying to have a generic driver which works with any random PHY
> > so the binding has no idea what supplies it might need.
> 
> OK, that makes sense, but then question is why not using existing
> naming, so "supplies" and "supply-names"?

I'm not saying it is not possible, but in general, the names are not
interesting. All that is needed is that they are all on, or
potentially all off to save power on shutdown. We don't care how many
there are, or what order they are enabled.

Ethernet PHY can have multiple supplies. For example there can be two
digital voltages and one analogue. Most designs just hard wire them
always on. It would not be unreasonable to have one GPIO which
controls all three. Or there could be one GPIO for the two digital
supplies, and one for the analogue. Or potentially, three GPIOs.

Given all the different ways the board could be designed, i doubt any
driver is going to want to control its supplies in an way other than
all on, or all off. 802.3 clause 22 defines a standardized way to put
a PHY into a low power mode. Using that one bit is much simpler than
trying to figure out how a board is wired.

However, the API/binding should be generic, usable for other use
cases. Nobody has needed an API like this before, but it is not to say
it might have other uses in the future. So maybe "supplies" and
"supply-names" is useful, but we still need a way to enumerate them as
a list without caring how many there are, or what their names are.

  Andrew

Re: [PATCH v2 4/5] dt-bindings: net: Add documentation for optional regulators

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1238 bytes --]

On Thu, May 19, 2022 at 01:58:18PM +0200, Andrew Lunn wrote:
> On Thu, May 19, 2022 at 01:33:21PM +0200, Krzysztof Kozlowski wrote:
> > On 19/05/2022 13:31, Mark Brown wrote:
> > > On Thu, May 19, 2022 at 11:55:28AM +0200, Krzysztof Kozlowski wrote:
> > >> On 18/05/2022 22:09, Corentin Labbe wrote:

> > >>> +  regulators:
> > >>> +    description:
> > >>> +       List of phandle to regulators needed for the PHY

> > >> I don't understand that... is your PHY defining the regulators or using
> > >> supplies? If it needs a regulator (as a supply), you need to document
> > >> supplies, using existing bindings.

> > > They're trying to have a generic driver which works with any random PHY
> > > so the binding has no idea what supplies it might need.

> > OK, that makes sense, but then question is why not using existing
> > naming, so "supplies" and "supply-names"?

> I'm not saying it is not possible, but in general, the names are not
> interesting. All that is needed is that they are all on, or
> potentially all off to save power on shutdown. We don't care how many
> there are, or what order they are enabled.

I think Krzysztof is referring to the name of the property rather than
the contents of the -names property there.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 4/5] dt-bindings: net: Add documentation for optional regulators

[Rob Herring]

On Thu, May 19, 2022 at 01:58:18PM +0200, Andrew Lunn wrote:
> On Thu, May 19, 2022 at 01:33:21PM +0200, Krzysztof Kozlowski wrote:
> > On 19/05/2022 13:31, Mark Brown wrote:
> > > On Thu, May 19, 2022 at 11:55:28AM +0200, Krzysztof Kozlowski wrote:
> > >> On 18/05/2022 22:09, Corentin Labbe wrote:
> > > 
> > >>> +  regulators:
> > >>> +    description:
> > >>> +       List of phandle to regulators needed for the PHY
> > > 
> > >> I don't understand that... is your PHY defining the regulators or using
> > >> supplies? If it needs a regulator (as a supply), you need to document
> > >> supplies, using existing bindings.
> > > 
> > > They're trying to have a generic driver which works with any random PHY
> > > so the binding has no idea what supplies it might need.
> > 
> > OK, that makes sense, but then question is why not using existing
> > naming, so "supplies" and "supply-names"?
> 
> I'm not saying it is not possible, but in general, the names are not
> interesting. All that is needed is that they are all on, or
> potentially all off to save power on shutdown. We don't care how many
> there are, or what order they are enabled.
> 
> Ethernet PHY can have multiple supplies. For example there can be two
> digital voltages and one analogue. Most designs just hard wire them
> always on. It would not be unreasonable to have one GPIO which
> controls all three. Or there could be one GPIO for the two digital
> supplies, and one for the analogue. Or potentially, three GPIOs.

Again, it's not just supplies...

> 
> Given all the different ways the board could be designed, i doubt any
> driver is going to want to control its supplies in an way other than
> all on, or all off. 802.3 clause 22 defines a standardized way to put
> a PHY into a low power mode. Using that one bit is much simpler than
> trying to figure out how a board is wired.
> 
> However, the API/binding should be generic, usable for other use
> cases. 

The binding should not be generic as I explained here and many times 
before...

> Nobody has needed an API like this before, but it is not to say
> it might have other uses in the future. So maybe "supplies" and
> "supply-names" is useful, but we still need a way to enumerate them as
> a list without caring how many there are, or what their names are.

There's 2 standard patterns for how producer/consumer bindings work 
There's how gpio and regulators are done and then there's the
foo/foo-names style. Regulators when with the former and we're not going 
to do both.

You can still do what you want by retrieving all properties ending with 
'-supply'. Not as easy to implement, but works for existing users.

Rob

Re: [PATCH v2 4/5] dt-bindings: net: Add documentation for optional regulators

[Krzysztof Kozlowski]

On 19/05/2022 17:49, Mark Brown wrote:
> On Thu, May 19, 2022 at 01:58:18PM +0200, Andrew Lunn wrote:
>> On Thu, May 19, 2022 at 01:33:21PM +0200, Krzysztof Kozlowski wrote:
>>> On 19/05/2022 13:31, Mark Brown wrote:
>>>> On Thu, May 19, 2022 at 11:55:28AM +0200, Krzysztof Kozlowski wrote:
>>>>> On 18/05/2022 22:09, Corentin Labbe wrote:
> 
>>>>>> +  regulators:
>>>>>> +    description:
>>>>>> +       List of phandle to regulators needed for the PHY
> 
>>>>> I don't understand that... is your PHY defining the regulators or using
>>>>> supplies? If it needs a regulator (as a supply), you need to document
>>>>> supplies, using existing bindings.
> 
>>>> They're trying to have a generic driver which works with any random PHY
>>>> so the binding has no idea what supplies it might need.
> 
>>> OK, that makes sense, but then question is why not using existing
>>> naming, so "supplies" and "supply-names"?
> 
>> I'm not saying it is not possible, but in general, the names are not
>> interesting. All that is needed is that they are all on, or
>> potentially all off to save power on shutdown. We don't care how many
>> there are, or what order they are enabled.
> 
> I think Krzysztof is referring to the name of the property rather than
> the contents of the -names property there.

Yes, exactly. Existing pattern for single regulator supply is
"xxx-supply", so why this uses a bit different pattern instead of
something more consistent ("supplies" and "supply-names")?

Best regards,
Krzysztof

Re: [PATCH v2 4/5] dt-bindings: net: Add documentation for optional regulators

[LABBE Corentin]

Le Fri, May 20, 2022 at 09:57:26AM +0200, Krzysztof Kozlowski a écrit :
> On 19/05/2022 17:49, Mark Brown wrote:
> > On Thu, May 19, 2022 at 01:58:18PM +0200, Andrew Lunn wrote:
> >> On Thu, May 19, 2022 at 01:33:21PM +0200, Krzysztof Kozlowski wrote:
> >>> On 19/05/2022 13:31, Mark Brown wrote:
> >>>> On Thu, May 19, 2022 at 11:55:28AM +0200, Krzysztof Kozlowski wrote:
> >>>>> On 18/05/2022 22:09, Corentin Labbe wrote:
> > 
> >>>>>> +  regulators:
> >>>>>> +    description:
> >>>>>> +       List of phandle to regulators needed for the PHY
> > 
> >>>>> I don't understand that... is your PHY defining the regulators or using
> >>>>> supplies? If it needs a regulator (as a supply), you need to document
> >>>>> supplies, using existing bindings.
> > 
> >>>> They're trying to have a generic driver which works with any random PHY
> >>>> so the binding has no idea what supplies it might need.
> > 
> >>> OK, that makes sense, but then question is why not using existing
> >>> naming, so "supplies" and "supply-names"?
> > 
> >> I'm not saying it is not possible, but in general, the names are not
> >> interesting. All that is needed is that they are all on, or
> >> potentially all off to save power on shutdown. We don't care how many
> >> there are, or what order they are enabled.
> > 
> > I think Krzysztof is referring to the name of the property rather than
> > the contents of the -names property there.
> 
> Yes, exactly. Existing pattern for single regulator supply is
> "xxx-supply", so why this uses a bit different pattern instead of
> something more consistent ("supplies" and "supply-names")?
> 

I agree that supplies and supply-names are better.
But in another answer Rob is against it, so if I understand well, we are stuck to use individual xxx-supply.
I will try to create a new regulator_get_bulk_all() which scan all properties matching xxx-supply

Re: [PATCH v2 4/5] dt-bindings: net: Add documentation for optional regulators

[Krzysztof Kozlowski]

On 20/05/2022 10:15, LABBE Corentin wrote:
> 
> I agree that supplies and supply-names are better.
> But in another answer Rob is against it, so if I understand well, we are stuck to use individual xxx-supply.
> I will try to create a new regulator_get_bulk_all() which scan all properties matching xxx-supply

Yep.


Best regards,
Krzysztof