[PATCH net-next 0/3] net: phy: dp83867: add binding and support for io_impedance_ctrl nvmem cell

[PATCH net-next 0/3] net: phy: dp83867: add binding and support for io_impedance_ctrl nvmem cell

[Rasmus Villemoes]

We have a board where measurements indicate that the current three
options - leaving IO_IMPEDANCE_CTRL at the (factory calibrated) reset
value or using one of the two boolean properties to set it to the
min/max value - are too coarse.

This series adds a device tree binding for an nvmem cell which can be
populated during production with a suitable value calibrated for each
board, and corresponding support in the driver. The second patch adds
a trivial phy wrapper for dev_err_probe(), used in the third.


Rasmus Villemoes (3):
  dt-bindings: dp83867: add binding for io_impedance_ctrl nvmem cell
  linux/phy.h: add phydev_err_probe() wrapper for dev_err_probe()
  net: phy: dp83867: implement support for io_impedance_ctrl nvmem cell

 .../devicetree/bindings/net/ti,dp83867.yaml   | 18 +++++-
 drivers/net/phy/dp83867.c                     | 55 +++++++++++++++++--
 include/linux/phy.h                           |  3 +
 3 files changed, 67 insertions(+), 9 deletions(-)

-- 
2.31.1

[PATCH net-next 1/3] dt-bindings: dp83867: add binding for io_impedance_ctrl nvmem cell

[Rasmus Villemoes]

We have a board where measurements indicate that the current three
options - leaving IO_IMPEDANCE_CTRL at the (factory calibrated) reset
value or using one of the two boolean properties to set it to the
min/max value - are too coarse.

There is no documented mapping from the 32 possible values of the
IO_IMPEDANCE_CTRL field to values in the range 35-70 ohms, and the
exact mapping is likely to vary from chip to chip. So add a DT binding
for an nvmem cell which can be populated during production with a
value suitable for each specific board.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 .../devicetree/bindings/net/ti,dp83867.yaml    | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.yaml b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
index 047d757e8d82..76ff08a477ba 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83867.yaml
+++ b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
@@ -31,6 +31,16 @@ properties:
   reg:
     maxItems: 1
 
+  nvmem-cells:
+    maxItems: 1
+    description:
+      Nvmem data cell containing the value to write to the
+      IO_IMPEDANCE_CTRL field of the IO_MUX_CFG register.
+
+  nvmem-cell-names:
+    items:
+      - const: io_impedance_ctrl
+
   ti,min-output-impedance:
     type: boolean
     description: |
@@ -42,9 +52,11 @@ properties:
     description: |
       MAC Interface Impedance control to set the programmable output impedance
       to a maximum value (70 ohms).
-      Note: ti,min-output-impedance and ti,max-output-impedance are mutually
-        exclusive. When both properties are present ti,max-output-impedance
-        takes precedence.
+      Note: Specifying an io_impedance_ctrl nvmem cell or one of the
+        ti,min-output-impedance, ti,max-output-impedance properties
+        are mutually exclusive. If more than one is present, an nvmem
+        cell takes precedence over ti,max-output-impedance, which in
+        turn takes precedence over ti,min-output-impedance.
 
   tx-fifo-depth:
     $ref: /schemas/types.yaml#/definitions/uint32
-- 
2.31.1

Re: [PATCH net-next 1/3] dt-bindings: dp83867: add binding for io_impedance_ctrl nvmem cell

[Andrew Lunn]

> There is no documented mapping from the 32 possible values of the
> IO_IMPEDANCE_CTRL field to values in the range 35-70 ohms

There have been a few active TI engineers submitting patches to TI PHY
drivers. Please could you reach out to them and ask if they can
provide documentation.

Having magic values in DT is not the preferred why to use it. Ideally
you should store Ohms in the cell and convert to the register value.

    Andrew

Re: [PATCH net-next 1/3] dt-bindings: dp83867: add binding for io_impedance_ctrl nvmem cell

[Rasmus Villemoes]

On 06/06/2022 23.58, Andrew Lunn wrote:
>> There is no documented mapping from the 32 possible values of the
>> IO_IMPEDANCE_CTRL field to values in the range 35-70 ohms
> 
> There have been a few active TI engineers submitting patches to TI PHY
> drivers. Please could you reach out to them and ask if they can
> provide documentation.
>
> Having magic values in DT is not the preferred why to use it. Ideally
> you should store Ohms in the cell and convert to the register value.

We've already asked TI for more detailed information, but apparently the
data sheet already says all there is to know. I should have worded the
commit message differently. Something like

  There is no fixed mapping from register values to values in the range
  35-70 ohms; it varies from chip to chip, and even that target range is
  approximate.

So AFAICS the only meaningful thing to store in an nvmem cell is an
appropriate (per-board) raw value of that field.

I would think this would be very similar to how various sensors have
nvmem cells defining calibration data.

Rasmus

Re: [PATCH net-next 1/3] dt-bindings: dp83867: add binding for io_impedance_ctrl nvmem cell

[Jakub Kicinski]

On Tue, 7 Jun 2022 13:54:30 +0200 Rasmus Villemoes wrote:
> On 06/06/2022 23.58, Andrew Lunn wrote:
> >> There is no documented mapping from the 32 possible values of the
> >> IO_IMPEDANCE_CTRL field to values in the range 35-70 ohms  
> > 
> > There have been a few active TI engineers submitting patches to TI PHY
> > drivers. Please could you reach out to them and ask if they can
> > provide documentation.
> >
> > Having magic values in DT is not the preferred why to use it. Ideally
> > you should store Ohms in the cell and convert to the register value.  
> 
> We've already asked TI for more detailed information, but apparently the
> data sheet already says all there is to know. I should have worded the
> commit message differently. Something like
> 
>   There is no fixed mapping from register values to values in the range
>   35-70 ohms; it varies from chip to chip, and even that target range is
>   approximate.

The series was waiting for Andrew to come back but it ended up getting
marked as Changes Requested in PW. Would you mind reposting with the
modification to the commit message?

Re: [PATCH net-next 1/3] dt-bindings: dp83867: add binding for io_impedance_ctrl nvmem cell

[Rob Herring]

On Mon, 06 Jun 2022 22:22:18 +0200, Rasmus Villemoes wrote:
> We have a board where measurements indicate that the current three
> options - leaving IO_IMPEDANCE_CTRL at the (factory calibrated) reset
> value or using one of the two boolean properties to set it to the
> min/max value - are too coarse.
> 
> There is no documented mapping from the 32 possible values of the
> IO_IMPEDANCE_CTRL field to values in the range 35-70 ohms, and the
> exact mapping is likely to vary from chip to chip. So add a DT binding
> for an nvmem cell which can be populated during production with a
> value suitable for each specific board.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  .../devicetree/bindings/net/ti,dp83867.yaml    | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH net-next 1/3] dt-bindings: dp83867: add binding for io_impedance_ctrl nvmem cell

[Andrew Lunn]

On Mon, Jun 06, 2022 at 10:22:18PM +0200, Rasmus Villemoes wrote:
> We have a board where measurements indicate that the current three
> options - leaving IO_IMPEDANCE_CTRL at the (factory calibrated) reset
> value or using one of the two boolean properties to set it to the
> min/max value - are too coarse.
> 
> There is no documented mapping from the 32 possible values of the
> IO_IMPEDANCE_CTRL field to values in the range 35-70 ohms, and the
> exact mapping is likely to vary from chip to chip. So add a DT binding
> for an nvmem cell which can be populated during production with a
> value suitable for each specific board.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 2/3] linux/phy.h: add phydev_err_probe() wrapper for dev_err_probe()

[Rasmus Villemoes]

The dev_err_probe() function is quite useful to avoid boilerplate
related to -EPROBE_DEFER handling. Add a phydev_err_probe() helper to
simplify making use of that from phy drivers which otherwise use the
phydev_* helpers.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/phy.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 508f1149665b..bed9a347481b 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1539,6 +1539,9 @@ static inline void phy_device_reset(struct phy_device *phydev, int value)
 #define phydev_err(_phydev, format, args...)	\
 	dev_err(&_phydev->mdio.dev, format, ##args)
 
+#define phydev_err_probe(_phydev, err, format, args...)	\
+	dev_err_probe(&_phydev->mdio.dev, err, format, ##args)
+
 #define phydev_info(_phydev, format, args...)	\
 	dev_info(&_phydev->mdio.dev, format, ##args)
 
-- 
2.31.1

[PATCH net-next 3/3] net: phy: dp83867: implement support for io_impedance_ctrl nvmem cell

[Rasmus Villemoes]

We have a board where measurements indicate that the current three
options - leaving IO_IMPEDANCE_CTRL at the (factory calibrated) reset
value or using one of the two boolean properties to set it to the
min/max value - are too coarse.

Implement support for the newly added binding allowing device tree to
specify an nvmem cell containing an appropriate value for this
specific board.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/net/phy/dp83867.c | 55 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 49 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 8561f2d4443b..45d8a9298251 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -14,6 +14,7 @@
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/bitfield.h>
+#include <linux/nvmem-consumer.h>
 
 #include <dt-bindings/net/ti-dp83867.h>
 
@@ -521,6 +522,51 @@ static int dp83867_verify_rgmii_cfg(struct phy_device *phydev)
 }
 
 #if IS_ENABLED(CONFIG_OF_MDIO)
+static int dp83867_of_init_io_impedance(struct phy_device *phydev)
+{
+	struct dp83867_private *dp83867 = phydev->priv;
+	struct device *dev = &phydev->mdio.dev;
+	struct device_node *of_node = dev->of_node;
+	struct nvmem_cell *cell;
+	u8 *buf, val;
+	int ret;
+
+	cell = of_nvmem_cell_get(of_node, "io_impedance_ctrl");
+	if (IS_ERR(cell)) {
+		ret = PTR_ERR(cell);
+		if (ret != -ENOENT)
+			return phydev_err_probe(phydev, ret,
+						"failed to get nvmem cell io_impedance_ctrl\n");
+
+		/* If no nvmem cell, check for the boolean properties. */
+		if (of_property_read_bool(of_node, "ti,max-output-impedance"))
+			dp83867->io_impedance = DP83867_IO_MUX_CFG_IO_IMPEDANCE_MAX;
+		else if (of_property_read_bool(of_node, "ti,min-output-impedance"))
+			dp83867->io_impedance = DP83867_IO_MUX_CFG_IO_IMPEDANCE_MIN;
+		else
+			dp83867->io_impedance = -1; /* leave at default */
+
+		return 0;
+	}
+
+	buf = nvmem_cell_read(cell, NULL);
+	nvmem_cell_put(cell);
+
+	if (IS_ERR(buf))
+		return PTR_ERR(buf);
+
+	val = *buf;
+	kfree(buf);
+
+	if ((val & DP83867_IO_MUX_CFG_IO_IMPEDANCE_MASK) != val) {
+		phydev_err(phydev, "nvmem cell 'io_impedance_ctrl' contents out of range\n");
+		return -ERANGE;
+	}
+	dp83867->io_impedance = val;
+
+	return 0;
+}
+
 static int dp83867_of_init(struct phy_device *phydev)
 {
 	struct dp83867_private *dp83867 = phydev->priv;
@@ -548,12 +594,9 @@ static int dp83867_of_init(struct phy_device *phydev)
 		}
 	}
 
-	if (of_property_read_bool(of_node, "ti,max-output-impedance"))
-		dp83867->io_impedance = DP83867_IO_MUX_CFG_IO_IMPEDANCE_MAX;
-	else if (of_property_read_bool(of_node, "ti,min-output-impedance"))
-		dp83867->io_impedance = DP83867_IO_MUX_CFG_IO_IMPEDANCE_MIN;
-	else
-		dp83867->io_impedance = -1; /* leave at default */
+	ret = dp83867_of_init_io_impedance(phydev);
+	if (ret)
+		return ret;
 
 	dp83867->rxctrl_strap_quirk = of_property_read_bool(of_node,
 							    "ti,dp83867-rxctrl-strap-quirk");
-- 
2.31.1

[PATCH net-next v2 0/3] dt-bindings: dp83867: add binding for io_impedance_ctrl nvmem cell

[Rasmus Villemoes]

We have a board where measurements indicate that the current three
options - leaving IO_IMPEDANCE_CTRL at the reset value (which is
factory calibrated to a value corresponding to approximately 50 ohms)
or using one of the two boolean properties to set it to the min/max
value - are too coarse.

This series adds a device tree binding for an nvmem cell which can be
populated during production with a suitable value calibrated for each
board, and corresponding support in the driver. The second patch adds
a trivial phy wrapper for dev_err_probe(), used in the third.

v2: Improve commit message for patch 1; add Rob's R-b to patch 1. No
changes in the patches themselves.

Since Dan Murphy's email address bounces, cc'ing two other @ti.com
addresses that have touched the driver in recent years.

Rasmus Villemoes (3):
  dt-bindings: dp83867: add binding for io_impedance_ctrl nvmem cell
  linux/phy.h: add phydev_err_probe() wrapper for dev_err_probe()
  net: phy: dp83867: implement support for io_impedance_ctrl nvmem cell

 .../devicetree/bindings/net/ti,dp83867.yaml   | 18 +++++-
 drivers/net/phy/dp83867.c                     | 55 +++++++++++++++++--
 include/linux/phy.h                           |  3 +
 3 files changed, 67 insertions(+), 9 deletions(-)

-- 
2.31.1

[PATCH net-next v2 1/3] dt-bindings: dp83867: add binding for io_impedance_ctrl nvmem cell

[Rasmus Villemoes]

We have a board where measurements indicate that the current three
options - leaving IO_IMPEDANCE_CTRL at the reset value (which is
factory calibrated to a value corresponding to approximately 50 ohms)
or using one of the two boolean properties to set it to the min/max
value - are too coarse.

There is no fixed mapping from register values to values in the range
35-70 ohms; it varies from chip to chip, and even that target range is
approximate. So add a DT binding for an nvmem cell which can be
populated during production with a value suitable for each specific
board.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 .../devicetree/bindings/net/ti,dp83867.yaml    | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.yaml b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
index 047d757e8d82..76ff08a477ba 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83867.yaml
+++ b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
@@ -31,6 +31,16 @@ properties:
   reg:
     maxItems: 1
 
+  nvmem-cells:
+    maxItems: 1
+    description:
+      Nvmem data cell containing the value to write to the
+      IO_IMPEDANCE_CTRL field of the IO_MUX_CFG register.
+
+  nvmem-cell-names:
+    items:
+      - const: io_impedance_ctrl
+
   ti,min-output-impedance:
     type: boolean
     description: |
@@ -42,9 +52,11 @@ properties:
     description: |
       MAC Interface Impedance control to set the programmable output impedance
       to a maximum value (70 ohms).
-      Note: ti,min-output-impedance and ti,max-output-impedance are mutually
-        exclusive. When both properties are present ti,max-output-impedance
-        takes precedence.
+      Note: Specifying an io_impedance_ctrl nvmem cell or one of the
+        ti,min-output-impedance, ti,max-output-impedance properties
+        are mutually exclusive. If more than one is present, an nvmem
+        cell takes precedence over ti,max-output-impedance, which in
+        turn takes precedence over ti,min-output-impedance.
 
   tx-fifo-depth:
     $ref: /schemas/types.yaml#/definitions/uint32
-- 
2.31.1

[PATCH net-next v2 2/3] linux/phy.h: add phydev_err_probe() wrapper for dev_err_probe()

[Rasmus Villemoes]

The dev_err_probe() function is quite useful to avoid boilerplate
related to -EPROBE_DEFER handling. Add a phydev_err_probe() helper to
simplify making use of that from phy drivers which otherwise use the
phydev_* helpers.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/phy.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 508f1149665b..bed9a347481b 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1539,6 +1539,9 @@ static inline void phy_device_reset(struct phy_device *phydev, int value)
 #define phydev_err(_phydev, format, args...)	\
 	dev_err(&_phydev->mdio.dev, format, ##args)
 
+#define phydev_err_probe(_phydev, err, format, args...)	\
+	dev_err_probe(&_phydev->mdio.dev, err, format, ##args)
+
 #define phydev_info(_phydev, format, args...)	\
 	dev_info(&_phydev->mdio.dev, format, ##args)
 
-- 
2.31.1

[PATCH net-next v2 3/3] net: phy: dp83867: implement support for io_impedance_ctrl nvmem cell

[Rasmus Villemoes]

We have a board where measurements indicate that the current three
options - leaving IO_IMPEDANCE_CTRL at the (factory calibrated) reset
value or using one of the two boolean properties to set it to the
min/max value - are too coarse.

Implement support for the newly added binding allowing device tree to
specify an nvmem cell containing an appropriate value for this
specific board.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/net/phy/dp83867.c | 55 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 49 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 8561f2d4443b..45d8a9298251 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -14,6 +14,7 @@
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/bitfield.h>
+#include <linux/nvmem-consumer.h>
 
 #include <dt-bindings/net/ti-dp83867.h>
 
@@ -521,6 +522,51 @@ static int dp83867_verify_rgmii_cfg(struct phy_device *phydev)
 }
 
 #if IS_ENABLED(CONFIG_OF_MDIO)
+static int dp83867_of_init_io_impedance(struct phy_device *phydev)
+{
+	struct dp83867_private *dp83867 = phydev->priv;
+	struct device *dev = &phydev->mdio.dev;
+	struct device_node *of_node = dev->of_node;
+	struct nvmem_cell *cell;
+	u8 *buf, val;
+	int ret;
+
+	cell = of_nvmem_cell_get(of_node, "io_impedance_ctrl");
+	if (IS_ERR(cell)) {
+		ret = PTR_ERR(cell);
+		if (ret != -ENOENT)
+			return phydev_err_probe(phydev, ret,
+						"failed to get nvmem cell io_impedance_ctrl\n");
+
+		/* If no nvmem cell, check for the boolean properties. */
+		if (of_property_read_bool(of_node, "ti,max-output-impedance"))
+			dp83867->io_impedance = DP83867_IO_MUX_CFG_IO_IMPEDANCE_MAX;
+		else if (of_property_read_bool(of_node, "ti,min-output-impedance"))
+			dp83867->io_impedance = DP83867_IO_MUX_CFG_IO_IMPEDANCE_MIN;
+		else
+			dp83867->io_impedance = -1; /* leave at default */
+
+		return 0;
+	}
+
+	buf = nvmem_cell_read(cell, NULL);
+	nvmem_cell_put(cell);
+
+	if (IS_ERR(buf))
+		return PTR_ERR(buf);
+
+	val = *buf;
+	kfree(buf);
+
+	if ((val & DP83867_IO_MUX_CFG_IO_IMPEDANCE_MASK) != val) {
+		phydev_err(phydev, "nvmem cell 'io_impedance_ctrl' contents out of range\n");
+		return -ERANGE;
+	}
+	dp83867->io_impedance = val;
+
+	return 0;
+}
+
 static int dp83867_of_init(struct phy_device *phydev)
 {
 	struct dp83867_private *dp83867 = phydev->priv;
@@ -548,12 +594,9 @@ static int dp83867_of_init(struct phy_device *phydev)
 		}
 	}
 
-	if (of_property_read_bool(of_node, "ti,max-output-impedance"))
-		dp83867->io_impedance = DP83867_IO_MUX_CFG_IO_IMPEDANCE_MAX;
-	else if (of_property_read_bool(of_node, "ti,min-output-impedance"))
-		dp83867->io_impedance = DP83867_IO_MUX_CFG_IO_IMPEDANCE_MIN;
-	else
-		dp83867->io_impedance = -1; /* leave at default */
+	ret = dp83867_of_init_io_impedance(phydev);
+	if (ret)
+		return ret;
 
 	dp83867->rxctrl_strap_quirk = of_property_read_bool(of_node,
 							    "ti,dp83867-rxctrl-strap-quirk");
-- 
2.31.1

Re: [PATCH net-next v2 0/3] dt-bindings: dp83867: add binding for io_impedance_ctrl nvmem cell

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 14 Jun 2022 10:46:09 +0200 you wrote:
> We have a board where measurements indicate that the current three
> options - leaving IO_IMPEDANCE_CTRL at the reset value (which is
> factory calibrated to a value corresponding to approximately 50 ohms)
> or using one of the two boolean properties to set it to the min/max
> value - are too coarse.
> 
> This series adds a device tree binding for an nvmem cell which can be
> populated during production with a suitable value calibrated for each
> board, and corresponding support in the driver. The second patch adds
> a trivial phy wrapper for dev_err_probe(), used in the third.
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/3] dt-bindings: dp83867: add binding for io_impedance_ctrl nvmem cell
    https://git.kernel.org/netdev/net-next/c/ab1e9de84aff
  - [net-next,v2,2/3] linux/phy.h: add phydev_err_probe() wrapper for dev_err_probe()
    https://git.kernel.org/netdev/net-next/c/a793679827a8
  - [net-next,v2,3/3] net: phy: dp83867: implement support for io_impedance_ctrl nvmem cell
    https://git.kernel.org/netdev/net-next/c/5c2d0a6a0701

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html