* [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 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 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 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).