linux-kernel - Re: [PATCH net-next 0/2] net: phy: dp83822: Add support for changing the transmit amplitude voltage

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5118eb9a-ff6e-4e78-8529-d174e09cf52e@lunn.ch>
Date: Mon, 13 Jan 2025 16:59:38 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Dimitri Fedrau <dima.fedrau@...il.com>
Cc: dimitri.fedrau@...bherr.com, Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Andrew Davis <afd@...com>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 0/2] net: phy: dp83822: Add support for changing
 the transmit amplitude voltage

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