Re: [PATCH net-next v4 4/4] net: phy: dp83867: support setting output-impedance

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new 'ti,output-impedance-ohms' property turns an absolute
  ohm value into a fixed IO_IMPEDANCE_CTRL code with a straight line: 70…
- [Low] After this patch dp83867.c holds two conflicting descriptions of
  the same IO_IMPEDANCE_CTRL field.
- [Low] In dp83867_of_init_io_impedance(), the comment '/* If no nvmem
  cell, check for the boolean properties.

--

net: phy: dp83867: support setting output-impedance

This commit adds support for a ti,output-impedance-ohms devicetree
property to the dp83867 driver. A value in the range 35..70 ohms is mapped
linearly onto the 32 IO_IMPEDANCE_CTRL codes, and values outside that
range fail with -ERANGE.

> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index a3cd5cb11ea0a..ca668e75f0c99 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c

[ ... ]

> @@ -553,12 +557,29 @@ static int dp83867_of_init_io_impedance(struct phy_device *phydev)
>  						"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"))
> +		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"))
> +		} else if (of_property_read_bool(of_node, "ti,min-output-impedance")) {
>  			dp83867->io_impedance = DP83867_IO_MUX_CFG_IO_IMPEDANCE_MIN;
> -		else
> +		} else if (of_property_read_u32(of_node, "ti,output-impedance-ohms", &val) == 0) {

[Severity: Low]
This isn't a bug, but the comment above still says "check for the boolean
properties". The block now also reads the u32 ti,output-impedance-ohms
property and can return -ERANGE. Could the comment be updated to match?

> +			if (val < DP83867_IO_IMPEDANCE_MIN_OHMS ||
> +			    val > DP83867_IO_IMPEDANCE_MAX_OHMS) {
> +				phydev_err(phydev, "'ti,output-impedance-ohms' contents out of range\n");
> +				return -ERANGE;
> +			}
> +			/*
> +			 * 0x00 = 70 ohms ... 0x1f = 35 ohms. The datasheet
> +			 * does not specify the settings in between, map the
> +			 * range linearly to the nearest one.
> +			 */

[Severity: Low]
With this mapping, 50 ohms becomes 0x12 and 0x0f is about 53 ohms. The
non-OF dp83867_of_init() in the same file still says something different:

	/* Per datasheet, IO impedance is default to 50-ohm, so we set the
	 * same here or else the default '0' means highest IO impedance
	 * which is wrong.
	 */
	dp83867->io_impedance = DP83867_IO_MUX_CFG_IO_IMPEDANCE_MIN / 2;

So after this patch the driver has two descriptions of the same field. A
nominal 50 ohms programs a different code depending on whether
CONFIG_OF_MDIO is enabled.

The commit message mentions this mismatch, but both the comment and the
two values are left as they are. Should the two paths agree, or should one
of the comments say that its value is only approximate?

> +			dp83867->io_impedance =
> +				DIV_ROUND_CLOSEST((DP83867_IO_IMPEDANCE_MAX_OHMS - val) *
> +						  DP83867_IO_MUX_CFG_IO_IMPEDANCE_MASK,
> +						  DP83867_IO_IMPEDANCE_MAX_OHMS -
> +						  DP83867_IO_IMPEDANCE_MIN_OHMS);

[Severity: Medium]
Can a fixed linear formula give the requested impedance on a given chip?

Commit ab1e9de84aff ("dt-bindings: dp83867: add binding for
io_impedance_ctrl nvmem cell") says the IO_IMPEDANCE_CTRL reset value is
"factory calibrated to a value corresponding to approximately 50 ohms".
It also says:

  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.

That is why a per-board nvmem cell was added then, rather than an ohms
property.

With this patch, ti,output-impedance-ohms = <50> computes 0x12 on every
part. dp83867_config_init() then writes that code over the factory-trimmed
value on each probe, resume and soft reset:

	/* If specified, set io impedance */
	if (dp83867->io_impedance >= 0)
		phy_modify_mmd(phydev, DP83867_DEVADDR, DP83867_IO_MUX_CFG,
			       DP83867_IO_MUX_CFG_IO_IMPEDANCE_MASK,
			       dp83867->io_impedance);

As a result, the RGMII output impedance can differ from the requested ohms
from one chip to the next, and no error is reported.

If the binding description for ti,output-impedance-ohms also documents
this linear mapping, the ohm semantics become DT ABI that the hardware
cannot guarantee.

Would a raw register code property work better here? Another option would
be to document the ohm value as approximate and keep the reset value as
the 50 ohm point.

> +		} else {
>  			dp83867->io_impedance = -1; /* leave at default */
> +		}
>  
>  		return 0;
>  	}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110-v7-2-topic-dp83867-v4-0-a268f83b1ec1%40pengutronix.de