netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 0/2] net: phy: dp83822: Add support for changing the transmit amplitude voltage
@ 2025-01-13  5:40 Dimitri Fedrau via B4 Relay
  2025-01-13  5:40 ` [PATCH net-next 1/2] dt-bindings: net: " Dimitri Fedrau via B4 Relay
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Dimitri Fedrau via B4 Relay @ 2025-01-13  5:40 UTC (permalink / raw)
  To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Andrew Davis, Andrew Lunn, Heiner Kallweit, Russell King
  Cc: netdev, devicetree, linux-kernel, Dimitri Fedrau, Dimitri Fedrau

Add support for changing the transmit amplitude voltage in 100BASE-TX mode.
Add support for configuration via DT.

Signed-off-by: Dimitri Fedrau <dimitri.fedrau@liebherr.com>
---
Dimitri Fedrau (2):
      dt-bindings: net: dp83822: Add support for changing the transmit amplitude voltage
      net: phy: dp83822: Add support for changing the transmit amplitude voltage

 .../devicetree/bindings/net/ti,dp83822.yaml        | 11 +++++++
 drivers/net/phy/dp83822.c                          | 35 ++++++++++++++++++++++
 2 files changed, 46 insertions(+)
---
base-commit: 7d0da8f862340c5f42f0062b8560b8d0971a6ac4
change-id: 20241213-dp83822-tx-swing-5ba6c1e9b065

Best regards,
-- 
Dimitri Fedrau <dimitri.fedrau@liebherr.com>



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH net-next 1/2] dt-bindings: net: dp83822: Add support for changing the transmit amplitude voltage
  2025-01-13  5:40 [PATCH net-next 0/2] net: phy: dp83822: Add support for changing the transmit amplitude voltage Dimitri Fedrau via B4 Relay
@ 2025-01-13  5:40 ` Dimitri Fedrau via B4 Relay
  2025-01-13  5:40 ` [PATCH net-next 2/2] net: phy: " Dimitri Fedrau via B4 Relay
  2025-01-13 13:54 ` [PATCH net-next 0/2] " Andrew Lunn
  2 siblings, 0 replies; 10+ messages in thread
From: Dimitri Fedrau via B4 Relay @ 2025-01-13  5:40 UTC (permalink / raw)
  To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Andrew Davis, Andrew Lunn, Heiner Kallweit, Russell King
  Cc: netdev, devicetree, linux-kernel, Dimitri Fedrau, Dimitri Fedrau

From: Dimitri Fedrau <dimitri.fedrau@liebherr.com>

Add support for changing the transmit amplitude voltage in 100BASE-TX mode.
Add binding to support this feature.

Signed-off-by: Dimitri Fedrau <dimitri.fedrau@liebherr.com>
---
 Documentation/devicetree/bindings/net/ti,dp83822.yaml | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
index 50c24248df266f1950371b950cd9c4d417835f97..b44f7068011b8e594f2ba245c526f4f48e3c5963 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83822.yaml
+++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
@@ -122,6 +122,16 @@ properties:
       - free-running
       - recovered
 
+  ti,tx-amplitude-100base-tx-millivolt:
+    description: |
+       DP83822 PHY only.
+       Transmit amplitude voltage for 100BASE-TX in millivolt. When omitted,
+       the PHY's default will be left as is.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [1600, 1633, 1667, 1700, 1733, 1767, 1800, 1833,
+           1867, 1900, 1933, 1967, 2000, 2033, 2067, 2100]
+    default: 2000
+
 required:
   - reg
 
@@ -137,6 +147,7 @@ examples:
         rx-internal-delay-ps = <1>;
         tx-internal-delay-ps = <1>;
         ti,gpio2-clk-out = "xi";
+        ti,tx-amplitude-100base-tx-millivolt = <2100>;
       };
     };
 

-- 
2.39.5



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net-next 2/2] net: phy: dp83822: Add support for changing the transmit amplitude voltage
  2025-01-13  5:40 [PATCH net-next 0/2] net: phy: dp83822: Add support for changing the transmit amplitude voltage Dimitri Fedrau via B4 Relay
  2025-01-13  5:40 ` [PATCH net-next 1/2] dt-bindings: net: " Dimitri Fedrau via B4 Relay
@ 2025-01-13  5:40 ` Dimitri Fedrau via B4 Relay
  2025-01-14 11:00   ` Simon Horman
                     ` (2 more replies)
  2025-01-13 13:54 ` [PATCH net-next 0/2] " Andrew Lunn
  2 siblings, 3 replies; 10+ messages in thread
From: Dimitri Fedrau via B4 Relay @ 2025-01-13  5:40 UTC (permalink / raw)
  To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Andrew Davis, Andrew Lunn, Heiner Kallweit, Russell King
  Cc: netdev, devicetree, linux-kernel, Dimitri Fedrau, Dimitri Fedrau

From: Dimitri Fedrau <dimitri.fedrau@liebherr.com>

Add support for changing the transmit amplitude voltage in 100BASE-TX mode.
Add support for configuration via DT.

Signed-off-by: Dimitri Fedrau <dimitri.fedrau@liebherr.com>
---
 drivers/net/phy/dp83822.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
index 4262bc31503b20640f19596449325d8a5938e20c..cc2ee9add648bc5f72dd92c7813ceec5e4b6db53 100644
--- a/drivers/net/phy/dp83822.c
+++ b/drivers/net/phy/dp83822.c
@@ -31,6 +31,7 @@
 #define MII_DP83822_RCSR	0x17
 #define MII_DP83822_RESET_CTRL	0x1f
 #define MII_DP83822_MLEDCR	0x25
+#define MII_DP83822_LDCTRL	0x403
 #define MII_DP83822_LEDCFG1	0x460
 #define MII_DP83822_IOCTRL1	0x462
 #define MII_DP83822_IOCTRL2	0x463
@@ -123,6 +124,9 @@
 #define DP83822_IOCTRL1_GPIO1_CTRL		GENMASK(2, 0)
 #define DP83822_IOCTRL1_GPIO1_CTRL_LED_1	BIT(0)
 
+/* LDCTRL bits */
+#define DP83822_100BASE_TX_LINE_DRIVER_SWING	GENMASK(7, 4)
+
 /* IOCTRL2 bits */
 #define DP83822_IOCTRL2_GPIO2_CLK_SRC		GENMASK(6, 4)
 #define DP83822_IOCTRL2_GPIO2_CTRL		GENMASK(2, 0)
@@ -197,6 +201,12 @@ struct dp83822_private {
 	bool set_gpio2_clk_out;
 	u32 gpio2_clk_out;
 	bool led_pin_enable[DP83822_MAX_LED_PINS];
+	int tx_amplitude_100base_tx_index;
+};
+
+static const u32 tx_amplitude_100base_tx[] = {
+	1600, 1633, 1667, 1700, 1733, 1767, 1800, 1833,
+	1867, 1900, 1933, 1967, 2000, 2033, 2067, 2100,
 };
 
 static int dp83822_config_wol(struct phy_device *phydev,
@@ -522,6 +532,12 @@ static int dp83822_config_init(struct phy_device *phydev)
 			       FIELD_PREP(DP83822_IOCTRL2_GPIO2_CLK_SRC,
 					  dp83822->gpio2_clk_out));
 
+	if (dp83822->tx_amplitude_100base_tx_index >= 0)
+		phy_modify_mmd(phydev, MDIO_MMD_VEND2, MII_DP83822_LDCTRL,
+			       DP83822_100BASE_TX_LINE_DRIVER_SWING,
+			       FIELD_PREP(DP83822_100BASE_TX_LINE_DRIVER_SWING,
+					  dp83822->tx_amplitude_100base_tx_index));
+
 	err = dp83822_config_init_leds(phydev);
 	if (err)
 		return err;
@@ -780,6 +796,8 @@ static int dp83822_of_init(struct phy_device *phydev)
 	struct dp83822_private *dp83822 = phydev->priv;
 	struct device *dev = &phydev->mdio.dev;
 	const char *of_val;
+	u32 val;
+	int i;
 
 	/* Signal detection for the PHY is only enabled if the FX_EN and the
 	 * SD_EN pins are strapped. Signal detection can only enabled if FX_EN
@@ -815,6 +833,23 @@ static int dp83822_of_init(struct phy_device *phydev)
 		dp83822->set_gpio2_clk_out = true;
 	}
 
+	dp83822->tx_amplitude_100base_tx_index = -1;
+	if (!device_property_read_u32(dev, "ti,tx-amplitude-100base-tx-millivolt", &val)) {
+		for (i = 0; i < ARRAY_SIZE(tx_amplitude_100base_tx); i++) {
+			if (tx_amplitude_100base_tx[i] == val) {
+				dp83822->tx_amplitude_100base_tx_index = i;
+				break;
+			}
+		}
+
+		if (dp83822->tx_amplitude_100base_tx_index < 0) {
+			phydev_err(phydev,
+				   "Invalid value for ti,tx-amplitude-100base-tx-millivolt property (%u)\n",
+				   val);
+			return -EINVAL;
+		}
+	}
+
 	return dp83822_of_init_leds(phydev);
 }
 

-- 
2.39.5



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next 0/2] net: phy: dp83822: Add support for changing the transmit amplitude voltage
  2025-01-13  5:40 [PATCH net-next 0/2] net: phy: dp83822: Add support for changing the transmit amplitude voltage Dimitri Fedrau via B4 Relay
  2025-01-13  5:40 ` [PATCH net-next 1/2] dt-bindings: net: " Dimitri Fedrau via B4 Relay
  2025-01-13  5:40 ` [PATCH net-next 2/2] net: phy: " Dimitri Fedrau via B4 Relay
@ 2025-01-13 13:54 ` Andrew Lunn
  2025-01-13 14:18   ` Dimitri Fedrau
  2 siblings, 1 reply; 10+ messages in thread
From: Andrew Lunn @ 2025-01-13 13:54 UTC (permalink / raw)
  To: dimitri.fedrau
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Andrew Davis, Heiner Kallweit, Russell King, netdev, devicetree,
	linux-kernel, Dimitri Fedrau

On Mon, Jan 13, 2025 at 06:40:11AM +0100, Dimitri Fedrau via B4 Relay wrote:
> Add support for changing the transmit amplitude voltage in 100BASE-TX mode.
> Add support for configuration via DT.

The commit message is supposed to answer the question "Why?". Isn't
reducing the voltage going to make the device non conforming? Why
would i want to break it? I could understand setting it a bit higher
than required to handle losses on the PCB and connector, so the
voltages measured on the RJ45 pins are conforming.

Also, what makes the dp8382 special? I know other PHYs can actually do
this. So why are we adding some vendor specific property just for
100base-tx?

	Andrew

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next 0/2] net: phy: dp83822: Add support for changing the transmit amplitude voltage
  2025-01-13 13:54 ` [PATCH net-next 0/2] " Andrew Lunn
@ 2025-01-13 14:18   ` Dimitri Fedrau
  2025-01-13 15:59     ` Andrew Lunn
  0 siblings, 1 reply; 10+ messages in thread
From: Dimitri Fedrau @ 2025-01-13 14:18 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: dimitri.fedrau, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Andrew Davis, Heiner Kallweit, Russell King, netdev,
	devicetree, linux-kernel

Hi Andrew,

Am Mon, Jan 13, 2025 at 02:54:28PM +0100 schrieb Andrew Lunn:
> On Mon, Jan 13, 2025 at 06:40:11AM +0100, Dimitri Fedrau via B4 Relay wrote:
> > Add support for changing the transmit amplitude voltage in 100BASE-TX mode.
> > Add support for configuration via DT.
> 
> The commit message is supposed to answer the question "Why?". Isn't
> reducing the voltage going to make the device non conforming? Why
> would i want to break it? I could understand setting it a bit higher
> than required to handle losses on the PCB and connector, so the
> voltages measured on the RJ45 pins are conforming.
>
- Will add the "Why?" to the commit description. You already answered it.
- Yes you are right.
- I don't want to break it, the PHY just provides these settings. And I
  just wanted to reflect this in the code, although it probably doesn't
  make sense.
- In my case I want to set it a bit higher to be conforming.

> Also, what makes the dp8382 special? I know other PHYs can actually do
> this. So why are we adding some vendor specific property just for
> 100base-tx?
>
I don't think that the dp83822 is special in this case. I just didn't
know better. Would be removing the vendor specific property enough ?
Or is there already a defined property describing this. Didn't found
anything.

Best regards,
Dimitri

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next 0/2] net: phy: dp83822: Add support for changing the transmit amplitude voltage
  2025-01-13 14:18   ` Dimitri Fedrau
@ 2025-01-13 15:59     ` Andrew Lunn
  2025-01-16  9:56       ` Dimitri Fedrau
  0 siblings, 1 reply; 10+ messages in thread
From: Andrew Lunn @ 2025-01-13 15:59 UTC (permalink / raw)
  To: Dimitri Fedrau
  Cc: dimitri.fedrau, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Andrew Davis, Heiner Kallweit, Russell King, netdev,
	devicetree, linux-kernel

On Mon, Jan 13, 2025 at 03:18:28PM +0100, Dimitri Fedrau wrote:
> Hi Andrew,
> 
> Am Mon, Jan 13, 2025 at 02:54:28PM +0100 schrieb Andrew Lunn:
> > On Mon, Jan 13, 2025 at 06:40:11AM +0100, Dimitri Fedrau via B4 Relay wrote:
> > > Add support for changing the transmit amplitude voltage in 100BASE-TX mode.
> > > Add support for configuration via DT.
> > 
> > The commit message is supposed to answer the question "Why?". Isn't
> > reducing the voltage going to make the device non conforming? Why
> > would i want to break it? I could understand setting it a bit higher
> > than required to handle losses on the PCB and connector, so the
> > voltages measured on the RJ45 pins are conforming.
> >
> - Will add the "Why?" to the commit description. You already answered it.
> - Yes you are right.
> - I don't want to break it, the PHY just provides these settings. And I
>   just wanted to reflect this in the code, although it probably doesn't
>   make sense.
> - In my case I want to set it a bit higher to be conforming.

I have seen use cases for deeply embedded systems where they want to
reduce it, to avoid some EMC issues and save some power/heat. They
know the cable lengths, so know a lower voltage won't cause an
issue. The issue in that case is that the configuration was policy,
not a description of the hardware. So i pushed for it to be a PHY
tunable, which can be set at runtime. Your use case is however about
the hardware, you need a slightly higher voltage because of the
hardware design. So for this case, i think DT is O.K. But you will
need to make this clear in the commit message, you want to make the
device conform by increasing the voltage. And put something in the
binding description to indicate this setting should be used to tune
the PHY for conformance. It is not our problem if somebody misuses it
for EMC/power saving and makes there device non-conform.

> > Also, what makes the dp8382 special? I know other PHYs can actually do
> > this. So why are we adding some vendor specific property just for
> > 100base-tx?
> >
> I don't think that the dp83822 is special in this case. I just didn't
> know better. Would be removing the vendor specific property enough ?
> Or is there already a defined property describing this. Didn't found
> anything.

If i remember correctly, there might be a property for
automotive/safety critical, where there is a choice of two
voltages. But it might be just deciding which of the two voltages are
used?

There is also:

  ti,cfg-dac-minus-one-bp:
    description: |
       DP83826 PHY only.
       Sets the voltage ratio (with respect to the nominal value)
       of the logical level -1 for the MLT-3 encoded TX data.
    enum: [5000, 5625, 6250, 6875, 7500, 8125, 8750, 9375, 10000,
           10625, 11250, 11875, 12500, 13125, 13750, 14375, 15000]
    default: 10000

  ti,cfg-dac-plus-one-bp:
    description: |
       DP83826 PHY only.
       Sets the voltage ratio (with respect to the nominal value)
       of the logical level +1 for the MLT-3 encoded TX data.
    enum: [5000, 5625, 6250, 6875, 7500, 8125, 8750, 9375, 10000,
           10625, 11250, 11875, 12500, 13125, 13750, 14375, 15000]
    default: 10000

I'm not so much an analogue person, but these don't make too much
sense to me. A ratio of 10,000 relative to nominal sounds rather
large. I would of expected a ration of 1 as the default? Since this
makes little sense to me, i don't think it is a good idea to copy it!

I've not looked at 802.3.... Do we need different settings for
different link modes?  Are the losses dependent on the link mode?  Are
the voltages different for different link modes? Is voltage actually
the best unit, if different link modes have different differential
voltages? Would a gain make more sense for a generic binding, and then
let the PHY driver convert that into whatever the hardware uses?

So, please take a step back, think about the general case, not your
specific PHY, and try to come up with a generic binding applicable to
all PHYs.

	    Andrew

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next 2/2] net: phy: dp83822: Add support for changing the transmit amplitude voltage
  2025-01-13  5:40 ` [PATCH net-next 2/2] net: phy: " Dimitri Fedrau via B4 Relay
@ 2025-01-14 11:00   ` Simon Horman
  2025-01-15 17:23   ` kernel test robot
  2025-01-15 22:44   ` kernel test robot
  2 siblings, 0 replies; 10+ messages in thread
From: Simon Horman @ 2025-01-14 11:00 UTC (permalink / raw)
  To: dimitri.fedrau
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Andrew Davis, Andrew Lunn, Heiner Kallweit, Russell King, netdev,
	devicetree, linux-kernel, Dimitri Fedrau

On Mon, Jan 13, 2025 at 06:40:13AM +0100, Dimitri Fedrau via B4 Relay wrote:
> From: Dimitri Fedrau <dimitri.fedrau@liebherr.com>
> 
> Add support for changing the transmit amplitude voltage in 100BASE-TX mode.
> Add support for configuration via DT.
> 
> Signed-off-by: Dimitri Fedrau <dimitri.fedrau@liebherr.com>
> ---
>  drivers/net/phy/dp83822.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c

...

> @@ -197,6 +201,12 @@ struct dp83822_private {
>  	bool set_gpio2_clk_out;
>  	u32 gpio2_clk_out;
>  	bool led_pin_enable[DP83822_MAX_LED_PINS];
> +	int tx_amplitude_100base_tx_index;
> +};
> +
> +static const u32 tx_amplitude_100base_tx[] = {
> +	1600, 1633, 1667, 1700, 1733, 1767, 1800, 1833,
> +	1867, 1900, 1933, 1967, 2000, 2033, 2067, 2100,
>  };
>  

nit: The use of tx_amplitude_100base_tx seems to be protected by
     #ifdef CONFIG_OF_MDIO, so the definition of tx_amplitude_100base_tx
     probably should be too.

     Flagged by W=1 allmodconfig builds.

...

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next 2/2] net: phy: dp83822: Add support for changing the transmit amplitude voltage
  2025-01-13  5:40 ` [PATCH net-next 2/2] net: phy: " Dimitri Fedrau via B4 Relay
  2025-01-14 11:00   ` Simon Horman
@ 2025-01-15 17:23   ` kernel test robot
  2025-01-15 22:44   ` kernel test robot
  2 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2025-01-15 17:23 UTC (permalink / raw)
  To: Dimitri Fedrau via B4 Relay, Andrew Lunn, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Andrew Davis, Heiner Kallweit,
	Russell King
  Cc: oe-kbuild-all, netdev, devicetree, linux-kernel, Dimitri Fedrau

Hi Dimitri,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 7d0da8f862340c5f42f0062b8560b8d0971a6ac4]

url:    https://github.com/intel-lab-lkp/linux/commits/Dimitri-Fedrau-via-B4-Relay/dt-bindings-net-dp83822-Add-support-for-changing-the-transmit-amplitude-voltage/20250113-134317
base:   7d0da8f862340c5f42f0062b8560b8d0971a6ac4
patch link:    https://lore.kernel.org/r/20250113-dp83822-tx-swing-v1-2-7ed5a9d80010%40liebherr.com
patch subject: [PATCH net-next 2/2] net: phy: dp83822: Add support for changing the transmit amplitude voltage
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20250116/202501160112.KjQc3mDq-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250116/202501160112.KjQc3mDq-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501160112.KjQc3mDq-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/net/phy/dp83822.c:207:18: warning: 'tx_amplitude_100base_tx' defined but not used [-Wunused-const-variable=]
     207 | static const u32 tx_amplitude_100base_tx[] = {
         |                  ^~~~~~~~~~~~~~~~~~~~~~~


vim +/tx_amplitude_100base_tx +207 drivers/net/phy/dp83822.c

   206	
 > 207	static const u32 tx_amplitude_100base_tx[] = {
   208		1600, 1633, 1667, 1700, 1733, 1767, 1800, 1833,
   209		1867, 1900, 1933, 1967, 2000, 2033, 2067, 2100,
   210	};
   211	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next 2/2] net: phy: dp83822: Add support for changing the transmit amplitude voltage
  2025-01-13  5:40 ` [PATCH net-next 2/2] net: phy: " Dimitri Fedrau via B4 Relay
  2025-01-14 11:00   ` Simon Horman
  2025-01-15 17:23   ` kernel test robot
@ 2025-01-15 22:44   ` kernel test robot
  2 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2025-01-15 22:44 UTC (permalink / raw)
  To: Dimitri Fedrau via B4 Relay, Andrew Lunn, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Andrew Davis, Heiner Kallweit,
	Russell King
  Cc: llvm, oe-kbuild-all, netdev, devicetree, linux-kernel,
	Dimitri Fedrau

Hi Dimitri,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 7d0da8f862340c5f42f0062b8560b8d0971a6ac4]

url:    https://github.com/intel-lab-lkp/linux/commits/Dimitri-Fedrau-via-B4-Relay/dt-bindings-net-dp83822-Add-support-for-changing-the-transmit-amplitude-voltage/20250113-134317
base:   7d0da8f862340c5f42f0062b8560b8d0971a6ac4
patch link:    https://lore.kernel.org/r/20250113-dp83822-tx-swing-v1-2-7ed5a9d80010%40liebherr.com
patch subject: [PATCH net-next 2/2] net: phy: dp83822: Add support for changing the transmit amplitude voltage
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20250116/202501160621.sg88rASV-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250116/202501160621.sg88rASV-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501160621.sg88rASV-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/net/phy/dp83822.c:7:
   In file included from include/linux/ethtool.h:18:
   In file included from include/linux/if_ether.h:19:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:10:
   In file included from include/linux/mm.h:2224:
   include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     505 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     512 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     525 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
>> drivers/net/phy/dp83822.c:207:18: warning: unused variable 'tx_amplitude_100base_tx' [-Wunused-const-variable]
     207 | static const u32 tx_amplitude_100base_tx[] = {
         |                  ^~~~~~~~~~~~~~~~~~~~~~~
   4 warnings generated.


vim +/tx_amplitude_100base_tx +207 drivers/net/phy/dp83822.c

   206	
 > 207	static const u32 tx_amplitude_100base_tx[] = {
   208		1600, 1633, 1667, 1700, 1733, 1767, 1800, 1833,
   209		1867, 1900, 1933, 1967, 2000, 2033, 2067, 2100,
   210	};
   211	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next 0/2] net: phy: dp83822: Add support for changing the transmit amplitude voltage
  2025-01-13 15:59     ` Andrew Lunn
@ 2025-01-16  9:56       ` Dimitri Fedrau
  0 siblings, 0 replies; 10+ messages in thread
From: Dimitri Fedrau @ 2025-01-16  9:56 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: dimitri.fedrau, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Andrew Davis, Heiner Kallweit, Russell King, netdev,
	devicetree, linux-kernel

Hi Andrew,

Am Mon, Jan 13, 2025 at 04:59:38PM +0100 schrieb Andrew Lunn:
> On Mon, Jan 13, 2025 at 03:18:28PM +0100, Dimitri Fedrau wrote:
> > Hi Andrew,
> > 
> > Am Mon, Jan 13, 2025 at 02:54:28PM +0100 schrieb Andrew Lunn:
> > > On Mon, Jan 13, 2025 at 06:40:11AM +0100, Dimitri Fedrau via B4 Relay wrote:
> > > > Add support for changing the transmit amplitude voltage in 100BASE-TX mode.
> > > > Add support for configuration via DT.
> > > 
> > > The commit message is supposed to answer the question "Why?". Isn't
> > > reducing the voltage going to make the device non conforming? Why
> > > would i want to break it? I could understand setting it a bit higher
> > > than required to handle losses on the PCB and connector, so the
> > > voltages measured on the RJ45 pins are conforming.
> > >
> > - Will add the "Why?" to the commit description. You already answered it.
> > - Yes you are right.
> > - I don't want to break it, the PHY just provides these settings. And I
> >   just wanted to reflect this in the code, although it probably doesn't
> >   make sense.
> > - In my case I want to set it a bit higher to be conforming.
> 
> I have seen use cases for deeply embedded systems where they want to
> reduce it, to avoid some EMC issues and save some power/heat. They
> know the cable lengths, so know a lower voltage won't cause an
> issue. The issue in that case is that the configuration was policy,
> not a description of the hardware. So i pushed for it to be a PHY
> tunable, which can be set at runtime. Your use case is however about
> the hardware, you need a slightly higher voltage because of the
> hardware design. So for this case, i think DT is O.K. But you will
> need to make this clear in the commit message, you want to make the
> device conform by increasing the voltage. And put something in the
> binding description to indicate this setting should be used to tune
> the PHY for conformance. It is not our problem if somebody misuses it
> for EMC/power saving and makes there device non-conform.
> 
Thanks for the explanation. Will add the comment in the commit message.

> > > Also, what makes the dp8382 special? I know other PHYs can actually do
> > > this. So why are we adding some vendor specific property just for
> > > 100base-tx?
> > >
> > I don't think that the dp83822 is special in this case. I just didn't
> > know better. Would be removing the vendor specific property enough ?
> > Or is there already a defined property describing this. Didn't found
> > anything.
> 
> If i remember correctly, there might be a property for
> automotive/safety critical, where there is a choice of two
> voltages. But it might be just deciding which of the two voltages are
> used?
> 
> There is also:
> 
>   ti,cfg-dac-minus-one-bp:
>     description: |
>        DP83826 PHY only.
>        Sets the voltage ratio (with respect to the nominal value)
>        of the logical level -1 for the MLT-3 encoded TX data.
>     enum: [5000, 5625, 6250, 6875, 7500, 8125, 8750, 9375, 10000,
>            10625, 11250, 11875, 12500, 13125, 13750, 14375, 15000]
>     default: 10000
> 
>   ti,cfg-dac-plus-one-bp:
>     description: |
>        DP83826 PHY only.
>        Sets the voltage ratio (with respect to the nominal value)
>        of the logical level +1 for the MLT-3 encoded TX data.
>     enum: [5000, 5625, 6250, 6875, 7500, 8125, 8750, 9375, 10000,
>            10625, 11250, 11875, 12500, 13125, 13750, 14375, 15000]
>     default: 10000
> 
> I'm not so much an analogue person, but these don't make too much
> sense to me. A ratio of 10,000 relative to nominal sounds rather
> large. I would of expected a ration of 1 as the default? Since this
> makes little sense to me, i don't think it is a good idea to copy it!
> 

I think the ratio of 10.000 was chosen to avoid truncation when calculating
register values. These are in steps of 6,25%. Using gain in DT seems to me
the more generic way of describing amplitude modification. But converting
the gain to register values might be more challenging in some cases. And
I think we also need to scale when we want to represent it as gain. The
DP83822 allows to modify the amplitude in terms of gain by 1.65% per
step and for 10BASE-TE gain is 9% per step. I don't have any other PHY that
allows me to modify the amplitude, so I'm quite biased. Do you know any other
PHY supporting this ?

> I've not looked at 802.3.... Do we need different settings for
> different link modes?  Are the losses dependent on the link mode?  Are
> the voltages different for different link modes? Is voltage actually
> the best unit, if different link modes have different differential
> voltages? Would a gain make more sense for a generic binding, and then
> let the PHY driver convert that into whatever the hardware uses?
> 

I had a quick look at 802.3, but I don't have much knowledge regarding
the analogue part:

- Different link modes have different differential voltages.
- I would prefer gain instead of voltage, should also fit for fiber
  optics.
- I would prefer separate settings for the link modes. Specs for the
  link modes are quite different, not that I could really judge. Maybe
  someone can help us out ?
- Maybe something like tx-amplitude-100base-tx-gain-milli or
  tx-amplitude-100base-tx-gain-micro ?

[...]

Best regards,
Dimitri Fedrau

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2025-01-16  9:56 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-13  5:40 [PATCH net-next 0/2] net: phy: dp83822: Add support for changing the transmit amplitude voltage Dimitri Fedrau via B4 Relay
2025-01-13  5:40 ` [PATCH net-next 1/2] dt-bindings: net: " Dimitri Fedrau via B4 Relay
2025-01-13  5:40 ` [PATCH net-next 2/2] net: phy: " Dimitri Fedrau via B4 Relay
2025-01-14 11:00   ` Simon Horman
2025-01-15 17:23   ` kernel test robot
2025-01-15 22:44   ` kernel test robot
2025-01-13 13:54 ` [PATCH net-next 0/2] " Andrew Lunn
2025-01-13 14:18   ` Dimitri Fedrau
2025-01-13 15:59     ` Andrew Lunn
2025-01-16  9:56       ` Dimitri Fedrau

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).