Message-ID: <26dcc3ee-6ea8-435e-b9e9-f22c712e5b4c@axis.com>
Date: Thu, 4 Jul 2024 16:01:13 +0200
From: Kamil Horák (2N) <kamilh@...s.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: florian.fainelli@...adcom.com, bcm-kernel-feedback-list@...adcom.com,
 hkallweit1@...il.com, linux@...linux.org.uk, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, robh@...nel.org,
 krzk+dt@...nel.org, conor+dt@...nel.org, netdev@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v9 4/4] net: phy: bcm-phy-lib: Implement BroadR-Reach link
 modes


On 6/22/24 21:12, Andrew Lunn wrote:
> On Fri, Jun 21, 2024 at 01:26:33PM +0200, Kamil Horák (2N) wrote:
>> Implement single-pair BroadR-Reach modes on bcm5481x PHY by Broadcom.
>> Create set of functions alternative to IEEE 802.3 to handle
>> configuration of these modes on compatible Broadcom PHYs.
> What i've not seen anywhere is a link between BroadR-Reach and LRE.
> Maybe you could explain the relationship here in the commit message?
> And maybe also how LDS fits in.

Tried to extend it a bit... LRE should be for "Long Reach Ethernet" but 
Broadcom

only uses the acronym in the datasheets... LDS is "Long-Distance 
Signaling", really screwed

term for a link auto-negotiation...

>
>> +int bcm_setup_master_slave(struct phy_device *phydev)
> This is missing the lre in the name.
Fixed.
>
>> +static int bcm54811_read_abilities(struct phy_device *phydev)
>> +{
>> +	int i, val, err;
>> +	u8 brr_mode;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(bcm54811_linkmodes); i++)
>> +		linkmode_clear_bit(bcm54811_linkmodes[i], phydev->supported);
> I think that needs a comment since it is not clear what is going on
> here. What set these bits in supported?

This is an equivalent of genphy_read_abilities for an IEEE PHY, that is, 
it fills the phydev->supported bit array exactly

as genphy_read_abilities does. The genphy_read_abilities is even called 
if the PHY is not in BRR mode.

>> +
>> +	err = bcm5481x_get_brrmode(phydev, &brr_mode);
>> +	if (err)
>> +		return err;
>> +
>> +	if (brr_mode) {
> I would expect the DT property to be here somewhere. If the DT
> property is present, set phydev->supported to only the BRR modes,
> otherwise set it to the standard baseT modes. That should then allow
> the core to do most of the validation. This is based on my
> understanding the coupling hardware makes the two modes mutually
> exclusive?

 From my point of view relying on DT property only would imply to 
validate the setting with what is read from the PHY on

all code locations where it is currently read by bcm5481x_get_brrmode. 
This is because the PHY can be reset externally

(at least by power-cycling it) and after reset, it is in IEEE mode. 
Thus, I chose to set the BRR mode on/off  upon initialization

and then read the setting from the chip when necessary.  The PHY can 
also be reset by writing bit 15 to register 0

in both IEEE and BRR modes (LRECR/BMCR).

The device I am developing on has no option for IEEE interface but in 
pure theory, kind of hardware plug-in would be

possible as I was told by our hardware team. However, not even the 
evaluation kit for bcm54811 can be switched

between BRR and IEEE hardware without at least soldering and desoldering 
some components on the PCB.

>
>> +	/* With BCM54811, BroadR-Reach implies no autoneg */
>> +	if (brr)
>> +		phydev->autoneg = 0;
> So long as phydev->supported does not indicate autoneg, this should
> not happen.

I also thought so but unfortunately our batch of bcm54811 indicates 
possible autoneg in its status register

  (LRESR_LDSABILITY) but refuses to negotiate. So this is rather a 
preparation for bcm54810 full support. Unlike bcm54811,

the bcm54810 should be aneg-capable but I cannot verify it without the 
hardware available. The information around

  it is rather fuzzy, we were told by Broadcom tech support that the 
54811 should autonegotiate as well but

  the datasheets from the same source clearly indicates "no". Same for 
the bcm54811 evaluation kit,

there is no autoneg option available (only 10/100Mbit and master/slave).


In conclusion, the idea was to support as much as possible but with 
given hardware, only a subset could be verified

  - that is bcm54811 10 or 100 Mbit on one pair and forced master / 
slave selection. As for the other possibilities, I only

could test that the autoneg is probably not there or at least it does 
not function with identical hardware on both ends.

We also have a BRR switch and some media converters (BRR/Ethernet) from 
other manufacturer with bcm54811 to help

the development and all those are fixed setting only, no autoneg.


>
> 	Andrew


Kamil

Powered by blists - more mailing lists