Message-ID: <20250116095624.GA4526@debian>
Date: Thu, 16 Jan 2025 10:56:24 +0100
From: Dimitri Fedrau <dima.fedrau@...il.com>
To: Andrew Lunn <andrew@...n.ch>
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

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

Powered by blists - more mailing lists