| Message ID | 20250113-dp83822-tx-swing-v1-0-7ed5a9d80010@liebherr.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: phy: dp83822: Add support for changing the transmit amplitude voltage | expand |
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
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
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
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,