[PATCH v3 0/3] net: phy: bcm5481x: add support for BroadR-Reach mode

[PATCH v3 0/3] net: phy: bcm5481x: add support for BroadR-Reach mode

[Kamil Horák - 2N]

PATCH 1 - Add the 1BR10 link mode and capability to switch to 
   BroadR-Reach as a PHY tunable value

PATCH 2 - Add the definitions of LRE registers, necessary to use
   BroadR-Reach modes on the BCM5481x PHY

PATCH 3 - Implementation of the BroadR-Reach modes for the Broadcom
   PHYs

Changes in v2:
  - Divided into multiple patches, removed useless link modes

Changes in v3:
  - Fixed uninitialized variable in bcm5481x_config_delay_swap function


Kamil Horák - 2N (3):
  net: phy: bcm54811: New link mode for BroadR-Reach
  net: phy: bcm54811: Add LRE registers definitions
  net: phy: bcm-phy-lib: Implement BroadR-Reach link modes

 drivers/net/phy/bcm-phy-lib.c | 122 ++++++++++++
 drivers/net/phy/bcm-phy-lib.h |   4 +
 drivers/net/phy/broadcom.c    | 338 ++++++++++++++++++++++++++++++++--
 drivers/net/phy/phy-core.c    |   9 +-
 include/linux/brcmphy.h       |  91 ++++++++-
 include/uapi/linux/ethtool.h  |   9 +-
 net/ethtool/common.c          |   7 +
 net/ethtool/ioctl.c           |   1 +
 8 files changed, 560 insertions(+), 21 deletions(-)

-- 
2.39.2

[PATCH v3 1/3] net: phy: bcm54811: New link mode for BroadR-Reach

[Kamil Horák - 2N]

Introduce new link modes necessary for the BroadR-Reach mode on
bcm5481x PHY by Broadcom and new PHY tunable to choose between
normal (IEEE) ethernet and BroadR-Reach modes of the PHY.
---
 drivers/net/phy/phy-core.c   | 9 +++++----
 include/uapi/linux/ethtool.h | 9 ++++++++-
 net/ethtool/common.c         | 7 +++++++
 net/ethtool/ioctl.c          | 1 +
 4 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 15f349e5995a..129e223d8985 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -13,10 +13,9 @@
  */
 const char *phy_speed_to_str(int speed)
 {
-	BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 102,
-		"Enum ethtool_link_mode_bit_indices and phylib are out of sync. "
-		"If a speed or mode has been added please update phy_speed_to_str "
-		"and the PHY settings array.\n");
+	BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 103,
+			 "Enum ethtool_link_mode_bit_indices and phylib are out of sync. If a speed or mode has been added please update phy_speed_to_str and the PHY settings array.\n"
+			);
 
 	switch (speed) {
 	case SPEED_10:
@@ -265,6 +264,8 @@ static const struct phy_setting settings[] = {
 	PHY_SETTING(     10, FULL,     10baseT1S_Full		),
 	PHY_SETTING(     10, HALF,     10baseT1S_Half		),
 	PHY_SETTING(     10, HALF,     10baseT1S_P2MP_Half	),
+	PHY_SETTING(     10, FULL,     1BR10			),
+
 };
 #undef PHY_SETTING
 
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 041e09c3515d..105432565e6d 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -289,11 +289,18 @@ struct ethtool_tunable {
 #define ETHTOOL_PHY_EDPD_NO_TX			0xfffe
 #define ETHTOOL_PHY_EDPD_DISABLE		0
 
+/*
+ *	BroadR-Reach Mode Control
+ */
+#define ETHTOOL_PHY_BRR_MODE_ON		1
+#define ETHTOOL_PHY_BRR_MODE_OFF	0
+
 enum phy_tunable_id {
 	ETHTOOL_PHY_ID_UNSPEC,
 	ETHTOOL_PHY_DOWNSHIFT,
 	ETHTOOL_PHY_FAST_LINK_DOWN,
 	ETHTOOL_PHY_EDPD,
+	ETHTOOL_PHY_BRR_MODE,
 	/*
 	 * Add your fresh new phy tunable attribute above and remember to update
 	 * phy_tunable_strings[] in net/ethtool/common.c
@@ -1845,7 +1852,7 @@ enum ethtool_link_mode_bit_indices {
 	ETHTOOL_LINK_MODE_10baseT1S_Full_BIT		 = 99,
 	ETHTOOL_LINK_MODE_10baseT1S_Half_BIT		 = 100,
 	ETHTOOL_LINK_MODE_10baseT1S_P2MP_Half_BIT	 = 101,
-
+	ETHTOOL_LINK_MODE_1BR10_BIT			 = 102,
 	/* must be last entry */
 	__ETHTOOL_LINK_MODE_MASK_NBITS
 };
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 6b2a360dcdf0..5e37804958e9 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -98,6 +98,7 @@ phy_tunable_strings[__ETHTOOL_PHY_TUNABLE_COUNT][ETH_GSTRING_LEN] = {
 	[ETHTOOL_PHY_DOWNSHIFT]	= "phy-downshift",
 	[ETHTOOL_PHY_FAST_LINK_DOWN] = "phy-fast-link-down",
 	[ETHTOOL_PHY_EDPD]	= "phy-energy-detect-power-down",
+	[ETHTOOL_PHY_BRR_MODE]	= "phy-broadrreach-mode",
 };
 
 #define __LINK_MODE_NAME(speed, type, duplex) \
@@ -211,6 +212,7 @@ const char link_mode_names[][ETH_GSTRING_LEN] = {
 	__DEFINE_LINK_MODE_NAME(10, T1S, Full),
 	__DEFINE_LINK_MODE_NAME(10, T1S, Half),
 	__DEFINE_LINK_MODE_NAME(10, T1S_P2MP, Half),
+	__DEFINE_SPECIAL_MODE_NAME(1BR10, "1BR10"),
 };
 static_assert(ARRAY_SIZE(link_mode_names) == __ETHTOOL_LINK_MODE_MASK_NBITS);
 
@@ -374,6 +376,11 @@ const struct link_mode_info link_mode_params[] = {
 	__DEFINE_LINK_MODE_PARAMS(10, T1S, Full),
 	__DEFINE_LINK_MODE_PARAMS(10, T1S, Half),
 	__DEFINE_LINK_MODE_PARAMS(10, T1S_P2MP, Half),
+	[ETHTOOL_LINK_MODE_1BR10_BIT] = {
+		.speed	= SPEED_10,
+		.lanes  = 1,
+		.duplex = DUPLEX_FULL,
+	},
 };
 static_assert(ARRAY_SIZE(link_mode_params) == __ETHTOOL_LINK_MODE_MASK_NBITS);
 
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 5a55270aa86e..9e68c8562fa3 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -2722,6 +2722,7 @@ static int ethtool_phy_tunable_valid(const struct ethtool_tunable *tuna)
 	switch (tuna->id) {
 	case ETHTOOL_PHY_DOWNSHIFT:
 	case ETHTOOL_PHY_FAST_LINK_DOWN:
+	case ETHTOOL_PHY_BRR_MODE:
 		if (tuna->len != sizeof(u8) ||
 		    tuna->type_id != ETHTOOL_TUNABLE_U8)
 			return -EINVAL;
-- 
2.39.2

Re: [PATCH v3 1/3] net: phy: bcm54811: New link mode for BroadR-Reach

[Andrew Lunn]

On Mon, May 06, 2024 at 04:40:13PM +0200, Kamil Horák - 2N wrote:
> Introduce new link modes necessary for the BroadR-Reach mode on
> bcm5481x PHY by Broadcom and new PHY tunable to choose between
> normal (IEEE) ethernet and BroadR-Reach modes of the PHY.

I would of split this into two patches. The reason being, we need the
new link mode. But do we need the tunable? Why don't i just use the
link mode to select it?

ethtool -s eth42 advertise 1BR10

Once you have split this up, you can explain the link mode patch in a
bit more detail. That because the name does not fit 802.3, the normal
macros cannot be used, so everything needs to be hand crafted.

    Andrew

---
pw-bot: cr

Re: [PATCH v3 1/3] net: phy: bcm54811: New link mode for BroadR-Reach

[Kamil Horák, 2N]

On 5/6/24 21:14, Andrew Lunn wrote:
> On Mon, May 06, 2024 at 04:40:13PM +0200, Kamil Horák - 2N wrote:
>> Introduce new link modes necessary for the BroadR-Reach mode on
>> bcm5481x PHY by Broadcom and new PHY tunable to choose between
>> normal (IEEE) ethernet and BroadR-Reach modes of the PHY.
> I would of split this into two patches. The reason being, we need the
> new link mode. But do we need the tunable? Why don't i just use the
> link mode to select it?
>
> ethtool -s eth42 advertise 1BR10

Tried to find a way to do the link mode selection this way but the 
advertised modes are only applicable when there is auto-negotiation, 
which is only partially the case of BCM54811: it only has 
auto-negotiation in IEEE mode.
Thus, to avoid choosing between BroadR-Reach and IEEE mode using the PHY 
Tunable, we would need something else and I am already running out of 
ideas...
Is there any other possibility?

In addition, we would have to check for incompatible link modes selected 
to advertise (cannot choose one BRR and one IEEE mode to advertise), or 
perhaps the BRR modes would take precedence, if there is any BRR mode 
selected to advertise, IEEE modes would be ignored.

>
> Once you have split this up, you can explain the link mode patch in a
> bit more detail. That because the name does not fit 802.3, the normal
> macros cannot be used, so everything needs to be hand crafted.
>
>      Andrew
>
> ---
> pw-bot: cr


Kamil

Re: [PATCH v3 1/3] net: phy: bcm54811: New link mode for BroadR-Reach

[Andrew Lunn]

On Wed, May 22, 2024 at 09:34:05AM +0200, Kamil Horák, 2N wrote:
> 
> On 5/6/24 21:14, Andrew Lunn wrote:
> > On Mon, May 06, 2024 at 04:40:13PM +0200, Kamil Horák - 2N wrote:
> > > Introduce new link modes necessary for the BroadR-Reach mode on
> > > bcm5481x PHY by Broadcom and new PHY tunable to choose between
> > > normal (IEEE) ethernet and BroadR-Reach modes of the PHY.
> > I would of split this into two patches. The reason being, we need the
> > new link mode. But do we need the tunable? Why don't i just use the
> > link mode to select it?
> > 
> > ethtool -s eth42 advertise 1BR10
> 
> Tried to find a way to do the link mode selection this way but the
> advertised modes are only applicable when there is auto-negotiation, which
> is only partially the case of BCM54811: it only has auto-negotiation in IEEE
> mode.
> Thus, to avoid choosing between BroadR-Reach and IEEE mode using the PHY
> Tunable, we would need something else and I am already running out of
> ideas...
> Is there any other possibility?
> 
> In addition, we would have to check for incompatible link modes selected to
> advertise (cannot choose one BRR and one IEEE mode to advertise), or perhaps
> the BRR modes would take precedence, if there is any BRR mode selected to
> advertise, IEEE modes would be ignored.

So it sounds like multiple problems.

1) Passing a specific link mode when not using auto-neg. The current
API is:

ethtool -s eth42 autoneg off speed 10 duplex full

Maybe we want to extend that with

ethtool -s eth42 autoneg off speed 10 duplex full linkmode 1BR10

or just

ethtool -s eth42 autoneg off linkmode 1BR10

You can probably add a new member to ethtool_link_ksettings to pass it
to phylib. From there, it probably needs putting into a phy_device
member, next to speed and duplex. The PHY driver can then use it to
configure the hardware.

2) Invalid combinations of link modes when auto-neg is
enabled. Probably the first question to answer is, is this specific to
this PHY, or generic across all PHYs which support BR and IEEE
modes. If it is generic, we can add tests in
phy_ethtool_ksettings_set() to return EINVAL. If this is specific to
this PHY, it gets messy. When phylib call phy_start_aneg() to
configure the hardware, it does not expect it to return an error. We
might need to add an additional op to phy_driver to allow the PHY
driver to validate settings when phy_ethtool_ksettings_set() is
called. This would help solve a similar problem with a new mediatek
PHY which is broken with forced modes.

    Andrew

Re: [PATCH v3 1/3] net: phy: bcm54811: New link mode for BroadR-Reach

[Kamil Horák, 2N]

On 5/22/24 16:05, Andrew Lunn wrote:
> On Wed, May 22, 2024 at 09:34:05AM +0200, Kamil Horák, 2N wrote:
>> On 5/6/24 21:14, Andrew Lunn wrote:
>>> On Mon, May 06, 2024 at 04:40:13PM +0200, Kamil Horák - 2N wrote:
>>>> Introduce new link modes necessary for the BroadR-Reach mode on
>>>> bcm5481x PHY by Broadcom and new PHY tunable to choose between
>>>> normal (IEEE) ethernet and BroadR-Reach modes of the PHY.
>>> I would of split this into two patches. The reason being, we need the
>>> new link mode. But do we need the tunable? Why don't i just use the
>>> link mode to select it?
>>>
>>> ethtool -s eth42 advertise 1BR10
>> Tried to find a way to do the link mode selection this way but the
>> advertised modes are only applicable when there is auto-negotiation, 
>> which
>> is only partially the case of BCM54811: it only has auto-negotiation 
>> in IEEE
>> mode.
>> Thus, to avoid choosing between BroadR-Reach and IEEE mode using the PHY
>> Tunable, we would need something else and I am already running out of
>> ideas...
>> Is there any other possibility?
>>
>> In addition, we would have to check for incompatible link modes 
>> selected to
>> advertise (cannot choose one BRR and one IEEE mode to advertise), or 
>> perhaps
>> the BRR modes would take precedence, if there is any BRR mode 
>> selected to
>> advertise, IEEE modes would be ignored.
> So it sounds like multiple problems.
>
> 1) Passing a specific link mode when not using auto-neg. The current
> API is:
>
> ethtool -s eth42 autoneg off speed 10 duplex full
>
> Maybe we want to extend that with
>
> ethtool -s eth42 autoneg off speed 10 duplex full linkmode 1BR10
>
> or just
>
> ethtool -s eth42 autoneg off linkmode 1BR10

This sounds perfect to me. The second (shorter) way is better because, 
at least with BCM54811, given the link mode, the duplex and speed are 
also determined. All BroadR-Reach link modes are full duplex, anyway.

>
> You can probably add a new member to ethtool_link_ksettings to pass it
> to phylib. From there, it probably needs putting into a phy_device
> member, next to speed and duplex. The PHY driver can then use it to
> configure the hardware.
I did not dare to cut this deep so far, but as I see there is a demand, 
let's go for it!
>
> 2) Invalid combinations of link modes when auto-neg is
> enabled. Probably the first question to answer is, is this specific to
> this PHY, or generic across all PHYs which support BR and IEEE
> modes. If it is generic, we can add tests in
> phy_ethtool_ksettings_set() to return EINVAL. If this is specific to
> this PHY, it gets messy. When phylib call phy_start_aneg() to
> configure the hardware, it does not expect it to return an error. We
> might need to add an additional op to phy_driver to allow the PHY
> driver to validate settings when phy_ethtool_ksettings_set() is
> called. This would help solve a similar problem with a new mediatek
> PHY which is broken with forced modes.
Regarding the specificity, it definitely touches the BCM54811 and even 
more BCM54810, because the ...810 supports autoneg  in BroadR-Reach mode 
too.
I unfortunately do not have any device using BCM54810 (not even a 
devkit) available to test it, thus I only can do the BCM54811 driver.

With BCM54811 and considering no explicit BRR / IEEE switching (as it 
was originally intended by adding a tunable), we definitely have to 
check the set of selected link modes for applicability and return (and 
handle)  the error caused by impossible combination. Originally, I 
thought about abusing the autoneg with only one mode to select that mode 
without negotiation but that would apply only to BCM54811.
Thus, for BCM54811 I suggest to ignore BRR modes if there is at least 
one IEEE one in the set and report an error for not applicable set (BRR 
modes only).  For BCM54810 we should accept only sets consisting of link 
modes of same type (IEEE or BRR) and switch between IEEE and BRR as 
needed, but this would be likely a task for someone else. I do not have 
the hardware at hand. I'll do it with anticipation of such possibility, 
right?

>
>      Andrew


Kamil

Re: [PATCH v3 1/3] net: phy: bcm54811: New link mode for BroadR-Reach

[Andrew Lunn]

> > ethtool -s eth42 autoneg off linkmode 1BR10
> 
> This sounds perfect to me. The second (shorter) way is better because, at
> least with BCM54811, given the link mode, the duplex and speed are also
> determined. All BroadR-Reach link modes are full duplex, anyway.

Great.

> > You can probably add a new member to ethtool_link_ksettings to pass it
> > to phylib. From there, it probably needs putting into a phy_device
> > member, next to speed and duplex. The PHY driver can then use it to
> > configure the hardware.
> I did not dare to cut this deep so far, but as I see there is a demand,
> let's go for it!

It also seems quite a generic feature. e.g. to select between
1000BaseT_FULL and 1000BaseT1_FULL. So it should get reused for other
use cases.

> > 
> > 2) Invalid combinations of link modes when auto-neg is
> > enabled. Probably the first question to answer is, is this specific to
> > this PHY, or generic across all PHYs which support BR and IEEE
> > modes. If it is generic, we can add tests in
> > phy_ethtool_ksettings_set() to return EINVAL. If this is specific to
> > this PHY, it gets messy. When phylib call phy_start_aneg() to
> > configure the hardware, it does not expect it to return an error. We
> > might need to add an additional op to phy_driver to allow the PHY
> > driver to validate settings when phy_ethtool_ksettings_set() is
> > called. This would help solve a similar problem with a new mediatek
> > PHY which is broken with forced modes.
> Regarding the specificity, it definitely touches the BCM54811 and even more
> BCM54810, because the ...810 supports autoneg  in BroadR-Reach mode too.

That was what i did not know. Does 802.3 allow auto-neg for these
BroadR-Reach modes at the same time as 'normal' modes. And it seems
like the ..810 supports is, and i assume it conforms to 802.3? So we
cannot globally return an error in ethtool_ksetting_set() with a mix
or modes, it needs to be the driver which decides.

   Andrew

Re: [PATCH v3 1/3] net: phy: bcm54811: New link mode for BroadR-Reach

[Kamil Horák, 2N]

On 5/23/24 16:27, Andrew Lunn wrote:
>>> ethtool -s eth42 autoneg off linkmode 1BR10
>> This sounds perfect to me. The second (shorter) way is better because, at
>> least with BCM54811, given the link mode, the duplex and speed are also
>> determined. All BroadR-Reach link modes are full duplex, anyway.
> Great.
>
>>> You can probably add a new member to ethtool_link_ksettings to pass it
>>> to phylib. From there, it probably needs putting into a phy_device
>>> member, next to speed and duplex. The PHY driver can then use it to
>>> configure the hardware.
>> I did not dare to cut this deep so far, but as I see there is a demand,
>> let's go for it!
> It also seems quite a generic feature. e.g. to select between
> 1000BaseT_FULL and 1000BaseT1_FULL. So it should get reused for other
> use cases.
>
>>> 2) Invalid combinations of link modes when auto-neg is
>>> enabled. Probably the first question to answer is, is this specific to
>>> this PHY, or generic across all PHYs which support BR and IEEE
>>> modes. If it is generic, we can add tests in
>>> phy_ethtool_ksettings_set() to return EINVAL. If this is specific to
>>> this PHY, it gets messy. When phylib call phy_start_aneg() to
>>> configure the hardware, it does not expect it to return an error. We
>>> might need to add an additional op to phy_driver to allow the PHY
>>> driver to validate settings when phy_ethtool_ksettings_set() is
>>> called. This would help solve a similar problem with a new mediatek
>>> PHY which is broken with forced modes.
>> Regarding the specificity, it definitely touches the BCM54811 and even more
>> BCM54810, because the ...810 supports autoneg  in BroadR-Reach mode too.
> That was what i did not know. Does 802.3 allow auto-neg for these
> BroadR-Reach modes at the same time as 'normal' modes. And it seems
> like the ..810 supports is, and i assume it conforms to 802.3? So we
> cannot globally return an error in ethtool_ksetting_set() with a mix
> or modes, it needs to be the driver which decides.

As far I understand it, the chip is not capable of attempting IEEE and 
BroadR-Reach modes at the same time, not even the BCM54810, which is 
capable of autoneg in BRR. One has to choose IEEE or BRR first then 
start the auto-negotiation (or attempt the link with forced 
master-slave/speed setting for BRR). There are two separate "link is up" 
bits, one if the IEEE registers, second in the BRR one. Theoretically, 
it should be possible to have kind of auto-detection of hardware - for 
example start with IEEE, if there is no link after a timeout, try BRR as 
well. But as for the circuitry necessary to do that, there would have to 
be something like hardware plug-in, as I was told by our HW team. In 
other words, it is not probable to have a device capable of both 
(IEEE+BRR) modes at once. Thus, having the driver to choose from a set 
containing IEEE and BRR modes makes little sense.
Our use case is fixed master/slave and fixed speed (10/100), and BRR on 
1 pair only with BCM54811.  I can imagine autoneg master/slave and 
10/100 in the same physical media (one pair) but that would require 
BCM54810.

Back to the ethtool_ksetting_set(), both BCM54810 and 54811 sure support 
the IEEE modes and this would function even with a generic driver. With 
BRR and no autoneg, it seems that if there is one of 10, 100, 1000 
speeds and half or full duplex, it would be sufficient to have 
config_aneg method of the phy driver implemented correctly to do the 
thing (start IEEE (generic) or BRR auto-negotiation, which would include 
set up the PHY for selected link mode and wait for the link to appear). 
Not sure about how many other drivers regularly used fit this scheme, it 
seems that vast majority prefers auto-negotiation... However, it could 
be even made so that direct linkmode selection would work everywhere, 
leaving to the phy driver the choice of whether start autoneg with only 
one option or force that option directly when there is no aneg at all 
(BCM54811 in BRR mode).

>
>     Andrew
Kamil

Re: [PATCH v3 1/3] net: phy: bcm54811: New link mode for BroadR-Reach

[Andrew Lunn]

> As far I understand it, the chip is not capable of attempting IEEE and
> BroadR-Reach modes at the same time, not even the BCM54810, which is capable
> of autoneg in BRR. One has to choose IEEE or BRR first then start the
> auto-negotiation (or attempt the link with forced master-slave/speed setting
> for BRR). There are two separate "link is up" bits, one if the IEEE
> registers, second in the BRR one. Theoretically, it should be possible to
> have kind of auto-detection of hardware - for example start with IEEE, if
> there is no link after a timeout, try BRR as well. But as for the circuitry
> necessary to do that, there would have to be something like hardware
> plug-in, as I was told by our HW team. In other words, it is not probable to
> have a device capable of both (IEEE+BRR) modes at once. Thus, having the
> driver to choose from a set containing IEEE and BRR modes makes little
> sense.

So IEEE and BRR autoneg are mutually exclusive. It would be good to
see if 802.3 actually says or implies that. Generic functions like
ksetting_set/get should be based on 802.3, so when designing the API
we should focus on that, not what the particular devices you are
interested in support.

We probably want phydev->supports listing all modes, IEEE and BRR. Is
there a bit equivalent to BMSR_ANEGCAPABLE indicating the hardware can
do BRR autoneg? If there is, we probably want to add a
ETHTOOL_LINK_MODE_Autoneg_BRR_BIT.

ksetting_set should enforce this mutual exclusion. So
phydev->advertise should never be set containing invalid combination,
ksetting_set() should return an error.

I guess we need to initialize phydev->advertise to IEEE link modes in
order to not cause regressions. However, if the PHY does not support
any IEEE modes, it can then default to BRR link modes. It would also
make sense to have a standardized DT property to indicate BRR should
be used by default.

> Our use case is fixed master/slave and fixed speed (10/100), and BRR on 1
> pair only with BCM54811.  I can imagine autoneg master/slave and 10/100 in
> the same physical media (one pair) but that would require BCM54810.
 


> Not sure about how many other drivers
> regularly used fit this scheme, it seems that vast majority prefers
> auto-negotiation... However, it could be even made so that direct linkmode
> selection would work everywhere, leaving to the phy driver the choice of
> whether start autoneg with only one option or force that option directly
> when there is no aneg at all (BCM54811 in BRR mode).

No, this is not correct. There is a difference between autoneg with a
single mode, and forced. forced does not attempt to perform autoneg,
the hardware is just configured to a particular link mode. autoneg
with a single link mode does perform autoneg, you get to see what the
link partner is advertising etc. Either you can resolve to a common
link code and autoneg is successful, or there are no common modes and
autoneg fails.

A driver does not have a choice here, it need to do what it is told to
do.

	Andrew

Re: [PATCH v3 1/3] net: phy: bcm54811: New link mode for BroadR-Reach

[Kamil Horák, 2N]

On 5/25/24 19:12, Andrew Lunn wrote:
>> As far I understand it, the chip is not capable of attempting IEEE and
>> BroadR-Reach modes at the same time, not even the BCM54810, which is capable
>> of autoneg in BRR. One has to choose IEEE or BRR first then start the
>> auto-negotiation (or attempt the link with forced master-slave/speed setting
>> for BRR). There are two separate "link is up" bits, one if the IEEE
>> registers, second in the BRR one. Theoretically, it should be possible to
>> have kind of auto-detection of hardware - for example start with IEEE, if
>> there is no link after a timeout, try BRR as well. But as for the circuitry
>> necessary to do that, there would have to be something like hardware
>> plug-in, as I was told by our HW team. In other words, it is not probable to
>> have a device capable of both (IEEE+BRR) modes at once. Thus, having the
>> driver to choose from a set containing IEEE and BRR modes makes little
>> sense.
> So IEEE and BRR autoneg are mutually exclusive. It would be good to
> see if 802.3 actually says or implies that. Generic functions like
> ksetting_set/get should be based on 802.3, so when designing the API
> we should focus on that, not what the particular devices you are
> interested in support.
I am not sure about how to determine whether IEEE 802.3 says anything 
about the IEEE and BRR modes auto-negotiation mutual exclusivity - it is 
purely question of the implementation, in our case in the Broadcom PHYs. 
One of the BRR modes (1BR100) is direct equivalent of 100Base-T1 as 
specified in IEEE 802.3bw. As it requests different hardware to be 
connected, I doubt there is any (even theoretical) possibility to 
negotiate with a set of supported modes including let's say 100Base-T1 
and 100Base-T.
>
> We probably want phydev->supports listing all modes, IEEE and BRR. Is
> there a bit equivalent to BMSR_ANEGCAPABLE indicating the hardware can
> do BRR autoneg? If there is, we probably want to add a
> ETHTOOL_LINK_MODE_Autoneg_BRR_BIT.
There is "LDS Ability" (LRESR_LDSABILITY) bit in the LRE registers set 
of BCM54810, which is equivalent to BMSR_ANEGCAPABLE and it is at same 
position (bit 3 of the status register), so that just this could work.

But just in our case, the LDS Ability bit is "reserved" and "reads as 1" 
(BCM54811, BCM54501). So at least for these two it cannot be used as an 
indication of aneg capability.

LDS is "long-distance signaling" int he Broadcom's terminology, "a 
special new type of auto-negotiation"....

>
> ksetting_set should enforce this mutual exclusion. So
> phydev->advertise should never be set containing invalid combination,
> ksetting_set() should return an error.
>
> I guess we need to initialize phydev->advertise to IEEE link modes in
> order to not cause regressions. However, if the PHY does not support
> any IEEE modes, it can then default to BRR link modes. It would also
> make sense to have a standardized DT property to indicate BRR should
> be used by default.

With device tree property it would be about the same situation as with 
phy tunable, wouldn't? The tunable was already in the first version of 
this patch and it (or DT property) is same type of solution, one knows 
in advance which set of link modes to use. I personally feel the DT as 
better method, because the IEEE/BRR selection is of hardware nature and 
cannot be easily auto-detected - exactly what the DT is for.

There is description of the LDS negotioation in BCM54810 datasheet 
saying that if the PHY detects standard Ethernet link pulses on a wire 
pair, it transitions automatically from BRR-LDS to Clause 28 
auto-negotioation mode. Thus, at least the 54810 can be set so that it 
starts in BRR mode and if there is no BRR PHY at the other end and the 
other end is also set to auto-negotiate (Clause-28), the 
auto-negotiation continues in IEEE mode and potentially results in the 
PHY in IEEE mode. In this case, it would make sense to have both BRR and 
IEEE link modes in same list and just start with BRR, leaving on the PHY 
itself the decision to fall back to IEEE. The process would be 
sub-optimal in most use cases - who would use BRR PHY in hardwired IEEE 
circuit..?

However, I cannot promise to do such a driver because I do not have the 
BCM54810 available nor it is my task here.

>
>> Our use case is fixed master/slave and fixed speed (10/100), and BRR on 1
>> pair only with BCM54811.  I can imagine autoneg master/slave and 10/100 in
>> the same physical media (one pair) but that would require BCM54810.
>   
>
>
>> Not sure about how many other drivers
>> regularly used fit this scheme, it seems that vast majority prefers
>> auto-negotiation... However, it could be even made so that direct linkmode
>> selection would work everywhere, leaving to the phy driver the choice of
>> whether start autoneg with only one option or force that option directly
>> when there is no aneg at all (BCM54811 in BRR mode).
> No, this is not correct. There is a difference between autoneg with a
> single mode, and forced. forced does not attempt to perform autoneg,
> the hardware is just configured to a particular link mode. autoneg
> with a single link mode does perform autoneg, you get to see what the
> link partner is advertising etc. Either you can resolve to a common
> link code and autoneg is successful, or there are no common modes and
> autoneg fails.
>
> A driver does not have a choice here, it need to do what it is told to
> do.

OK so back to the proposed new parameter for ethtool, the "linkmode" 
would mean forced setting of  given link mode - so use the 
link_mode_masks as 1 of N or just pass the link mode number as another 
parameter?

For the BRR, this forced setting includes the duplex option (always 
full) but still requires additional parameter to determine master/slave 
(likely using the command - eg. "master-slave forced-slave")

For IEEE modes (or any other supported modes on other PHYs) this would 
set speed and duplex as requested.

>
> 	Andrew
Kamil

Re: [PATCH v3 1/3] net: phy: bcm54811: New link mode for BroadR-Reach

[Andrew Lunn]

> > So IEEE and BRR autoneg are mutually exclusive. It would be good to
> > see if 802.3 actually says or implies that. Generic functions like
> > ksetting_set/get should be based on 802.3, so when designing the API
> > we should focus on that, not what the particular devices you are
> > interested in support.
> I am not sure about how to determine whether IEEE 802.3 says anything about
> the IEEE and BRR modes auto-negotiation mutual exclusivity - it is purely
> question of the implementation, in our case in the Broadcom PHYs.

CLause 22 and clause 45 might say something. e.g. the documentation
about BMSR_ANEGCAPABLE might indicate what link modes it covers.

> One of the
> BRR modes (1BR100) is direct equivalent of 100Base-T1 as specified in IEEE
> 802.3bw. As it requests different hardware to be connected, I doubt there is
> any (even theoretical) possibility to negotiate with a set of supported
> modes including let's say 100Base-T1 and 100Base-T.
> > 
> > We probably want phydev->supports listing all modes, IEEE and BRR. Is
> > there a bit equivalent to BMSR_ANEGCAPABLE indicating the hardware can
> > do BRR autoneg? If there is, we probably want to add a
> > ETHTOOL_LINK_MODE_Autoneg_BRR_BIT.
> There is "LDS Ability" (LRESR_LDSABILITY) bit in the LRE registers set of
> BCM54810, which is equivalent to BMSR_ANEGCAPABLE and it is at same position
> (bit 3 of the status register), so that just this could work.
> 
> But just in our case, the LDS Ability bit is "reserved" and "reads as 1"
> (BCM54811, BCM54501). So at least for these two it cannot be used as an
> indication of aneg capability.
> 
> LDS is "long-distance signaling" int he Broadcom's terminology, "a special
> new type of auto-negotiation"....

For generic code, we should go from what 802.3 says. Does clause 22 or
clause 45 define anything like LDS Ability? If you look at how 802.3
C22/C45 works, it is mostly self describing. You can read registers to
determine what the PHY supports. So it is possible to have generic
genphy_read_abilities() and genphy_c45_pma_read_abilities which does
most of the work. Ideally we just want to extend them to cover BBR
link modes.

> > ksetting_set should enforce this mutual exclusion. So
> > phydev->advertise should never be set containing invalid combination,
> > ksetting_set() should return an error.
> > 
> > I guess we need to initialize phydev->advertise to IEEE link modes in
> > order to not cause regressions. However, if the PHY does not support
> > any IEEE modes, it can then default to BRR link modes. It would also
> > make sense to have a standardized DT property to indicate BRR should
> > be used by default.
> 
> With device tree property it would be about the same situation as with phy
> tunable, wouldn't? The tunable was already in the first version of this
> patch and it (or DT property) is same type of solution, one knows in advance
> which set of link modes to use. I personally feel the DT as better method,
> because the IEEE/BRR selection is of hardware nature and cannot be easily
> auto-detected - exactly what the DT is for.

If we decide IEEE and BRR are mutually exclusive because of the
coupling, then this is clearly a hardware property. So DT, and maybe
sometime in the future ACPI, is the correct way to describe this.

> There is description of the LDS negotioation in BCM54810 datasheet saying
> that if the PHY detects standard Ethernet link pulses on a wire pair, it
> transitions automatically from BRR-LDS to Clause 28 auto-negotioation mode.
> Thus, at least the 54810 can be set so that it starts in BRR mode and if
> there is no BRR PHY at the other end and the other end is also set to
> auto-negotiate (Clause-28), the auto-negotiation continues in IEEE mode and
> potentially results in the PHY in IEEE mode. In this case, it would make
> sense to have both BRR and IEEE link modes in same list and just start with
> BRR, leaving on the PHY itself the decision to fall back to IEEE. The
> process would be sub-optimal in most use cases - who would use BRR PHY in
> hardwired IEEE circuit..?
> 
> However, I cannot promise to do such a driver because I do not have the
> BCM54810 available nor it is my task here.

That is fine. At the moment, we are just trying to explore all the
corners before we decide how this should work. 802.3 should be our
main guide, but also look at real hardware.

> OK so back to the proposed new parameter for ethtool, the "linkmode" would
> mean forced setting of  given link mode - so use the link_mode_masks as 1 of
> N or just pass the link mode number as another parameter?

The autoneg off should be enough to indicate what the passed link mode
means. However, it could also be placed into a new property it that
seems more logical for the API. When it comes to the internal API, i
think it will be a new member anyway.

	Andrew

Re: [PATCH v3 1/3] net: phy: bcm54811: New link mode for BroadR-Reach

[Kamil Horák, 2N]

On 5/27/24 15:20, Andrew Lunn wrote:
>>> So IEEE and BRR autoneg are mutually exclusive. It would be good to
>>> see if 802.3 actually says or implies that. Generic functions like
>>> ksetting_set/get should be based on 802.3, so when designing the API
>>> we should focus on that, not what the particular devices you are
>>> interested in support.
>> I am not sure about how to determine whether IEEE 802.3 says anything about
>> the IEEE and BRR modes auto-negotiation mutual exclusivity - it is purely
>> question of the implementation, in our case in the Broadcom PHYs.
> CLause 22 and clause 45 might say something. e.g. the documentation
> about BMSR_ANEGCAPABLE might indicate what link modes it covers.
There is nothing new in IEEE 802.3 clause 22, compared to what can be 
found in a datasheet of any PHY complying the standard... As for Clause 
45, I'd say that it does not handle the BRR case, nor the 100Base-T1 aka 
1BR100.
>
>> One of the
>> BRR modes (1BR100) is direct equivalent of 100Base-T1 as specified in IEEE
>> 802.3bw. As it requests different hardware to be connected, I doubt there is
>> any (even theoretical) possibility to negotiate with a set of supported
>> modes including let's say 100Base-T1 and 100Base-T.
>>> We probably want phydev->supports listing all modes, IEEE and BRR. Is
>>> there a bit equivalent to BMSR_ANEGCAPABLE indicating the hardware can
>>> do BRR autoneg? If there is, we probably want to add a
>>> ETHTOOL_LINK_MODE_Autoneg_BRR_BIT.
>> There is "LDS Ability" (LRESR_LDSABILITY) bit in the LRE registers set of
>> BCM54810, which is equivalent to BMSR_ANEGCAPABLE and it is at same position
>> (bit 3 of the status register), so that just this could work.
>>
>> But just in our case, the LDS Ability bit is "reserved" and "reads as 1"
>> (BCM54811, BCM54501). So at least for these two it cannot be used as an
>> indication of aneg capability.
>>
>> LDS is "long-distance signaling" int he Broadcom's terminology, "a special
>> new type of auto-negotiation"....
> For generic code, we should go from what 802.3 says. Does clause 22 or
> clause 45 define anything like LDS Ability? If you look at how 802.3
> C22/C45 works, it is mostly self describing. You can read registers to
> determine what the PHY supports. So it is possible to have generic
> genphy_read_abilities() and genphy_c45_pma_read_abilities which does
> most of the work. Ideally we just want to extend them to cover BBR
> link modes.
>
This sounds to me like we should not rely on common properties of IEEE 
and BRR register sets and rather implement it separately for the 
BroadR-Reach mode.

In other words, on initialization, decide whether there will be IEEE or 
BRR mode and behave according to that. The IEEE mode is already 
implemented in current state of Broadcom PHY library and broadcom.c and 
it does not nothing special in addition to make sure that the BRR mode 
is off. The rest is IEEE compatible.

If there were fully separated handling of IEEE and BRR, it would be 
difficult to do IEEE/BRR auto-detection or even try to issue aneg start 
command in both modes at once then check which one succeeds. However, 
this is not I would like to implement anyway (also for lack of hardware 
capable of doing so).

All code in bcm-phy-lib should handle PHY in LRE (or BRR) mode. For 
example, bcm54811_config_aneg in my last patch version basically calls 
bcm_config_aneg or genphy_config_aneg based on whether the PHY is in BRR 
or IEEE mode. The bcm_config_aneg then calls some genphy_... functions 
and thus relies on the fact that the LRE (BRR mode) registers do mostly 
the same as IEEE. This should probably be avoided and all control that 
can be done in the LRE register set only be done there and of course use 
the register definitions from brcmphy.h (LRECR_RESET to reset the chip, 
although it is same bit 15 as BMCR_RESET in Basic mode control register 
etc.). Only this shall be a pure solution.

For example, regarding the auto-negotiation, in BRR mode it shall mean 
LDS, in IEEE mode the usual auto-negotiation as described in IEEE802.3.

>>> ksetting_set should enforce this mutual exclusion. So
>>> phydev->advertise should never be set containing invalid combination,
>>> ksetting_set() should return an error.
>>>
>>> I guess we need to initialize phydev->advertise to IEEE link modes in
>>> order to not cause regressions. However, if the PHY does not support
>>> any IEEE modes, it can then default to BRR link modes. It would also
>>> make sense to have a standardized DT property to indicate BRR should
>>> be used by default.
>> With device tree property it would be about the same situation as with phy
>> tunable, wouldn't? The tunable was already in the first version of this
>> patch and it (or DT property) is same type of solution, one knows in advance
>> which set of link modes to use. I personally feel the DT as better method,
>> because the IEEE/BRR selection is of hardware nature and cannot be easily
>> auto-detected - exactly what the DT is for.
> If we decide IEEE and BRR are mutually exclusive because of the
> coupling, then this is clearly a hardware property. So DT, and maybe
> sometime in the future ACPI, is the correct way to describe this.

yes, see previous paragraph - IEEE and BRR to be mutually exclusive, 
neglect the possibility existing in some chips to do kind of 
super-auto-negotiation and thus make the chip to detect the type of 
connected physical network. I cannot test it and anyway, the BCM54810 
(with BRR aneg) seems to be deprecated in favor of BCM54811 (no BRR 
aneg, or at least not documented).

For our use case this is irrelevant, we have fixed master-slave and 
speed selection.

>
>> There is description of the LDS negotioation in BCM54810 datasheet saying
>> that if the PHY detects standard Ethernet link pulses on a wire pair, it
>> transitions automatically from BRR-LDS to Clause 28 auto-negotioation mode.
>> Thus, at least the 54810 can be set so that it starts in BRR mode and if
>> there is no BRR PHY at the other end and the other end is also set to
>> auto-negotiate (Clause-28), the auto-negotiation continues in IEEE mode and
>> potentially results in the PHY in IEEE mode. In this case, it would make
>> sense to have both BRR and IEEE link modes in same list and just start with
>> BRR, leaving on the PHY itself the decision to fall back to IEEE. The
>> process would be sub-optimal in most use cases - who would use BRR PHY in
>> hardwired IEEE circuit..?
>>
>> However, I cannot promise to do such a driver because I do not have the
>> BCM54810 available nor it is my task here.
> That is fine. At the moment, we are just trying to explore all the
> corners before we decide how this should work. 802.3 should be our
> main guide, but also look at real hardware.
>
>> OK so back to the proposed new parameter for ethtool, the "linkmode" would
>> mean forced setting of  given link mode - so use the link_mode_masks as 1 of
>> N or just pass the link mode number as another parameter?
> The autoneg off should be enough to indicate what the passed link mode
> means. However, it could also be placed into a new property it that
> seems more logical for the API. When it comes to the internal API, i
> think it will be a new member anyway.
OK then the only change to ethtool itself would be adding the 
possibility of eg. "linkmode 100BaseT1/Full", let the other end process 
it together with other parameters such as master-slave etc.
>
> 	Andrew

So now, maybe it's time to try to implement the solution discussed above 
and try another patch version...?


Kamil

Re: [PATCH v3 1/3] net: phy: bcm54811: New link mode for BroadR-Reach

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 1402 bytes --]

On 5/6/24 07:40, Kamil Horák - 2N wrote:
> Introduce new link modes necessary for the BroadR-Reach mode on
> bcm5481x PHY by Broadcom and new PHY tunable to choose between
> normal (IEEE) ethernet and BroadR-Reach modes of the PHY.

Missing Signed-off-by tag.

> ---
>   drivers/net/phy/phy-core.c   | 9 +++++----
>   include/uapi/linux/ethtool.h | 9 ++++++++-
>   net/ethtool/common.c         | 7 +++++++
>   net/ethtool/ioctl.c          | 1 +
>   4 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
> index 15f349e5995a..129e223d8985 100644
> --- a/drivers/net/phy/phy-core.c
> +++ b/drivers/net/phy/phy-core.c
> @@ -13,10 +13,9 @@
>    */
>   const char *phy_speed_to_str(int speed)
>   {
> -	BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 102,
> -		"Enum ethtool_link_mode_bit_indices and phylib are out of sync. "
> -		"If a speed or mode has been added please update phy_speed_to_str "
> -		"and the PHY settings array.\n");
> +	BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 103,
> +			 "Enum ethtool_link_mode_bit_indices and phylib are out of sync. If a speed or mode has been added please update phy_speed_to_str and the PHY settings array.\n"
> +			);

Unrelated change. I like Andrew's suggestion to key off advertising 
1BR10 to enable BroadR-Reach, but let's see what this discussion leads to.
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

[PATCH v3 2/3] net: phy: bcm54811: Add LRE registers definitions

[Kamil Horák - 2N]

Add the definitions of LRE registers for Broadcom BCM5481x PHY
---
 include/linux/brcmphy.h | 91 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 90 insertions(+), 1 deletion(-)

diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index 1394ba302367..9c0b78c1b6fb 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -270,16 +270,105 @@
 #define BCM5482_SSD_SGMII_SLAVE		0x15	/* SGMII Slave Register */
 #define BCM5482_SSD_SGMII_SLAVE_EN	0x0002	/* Slave mode enable */
 #define BCM5482_SSD_SGMII_SLAVE_AD	0x0001	/* Slave auto-detection */
+#define BCM5482_SSD_SGMII_SLAVE_AD	0x0001	/* Slave auto-detection */
+
+/* BroadR-Reach LRE Registers. */
+#define MII_BCM54XX_LRECR		0x00	/* LRE Control Register                    */
+#define MII_BCM54XX_LRESR		0x01	/* LRE Status Register                     */
+#define MII_BCM54XX_LREPHYSID1		0x02	/* LRE PHYS ID 1                           */
+#define MII_BCM54XX_LREPHYSID2		0x03	/* LRE PHYS ID 2                           */
+#define MII_BCM54XX_LREANAA		0x04	/* LDS Auto-Negotiation Advertised Ability */
+#define MII_BCM54XX_LREANAC		0x05	/* LDS Auto-Negotiation Advertised Control */
+#define MII_BCM54XX_LREANPT		0x06	/* LDS Ability Next Page Transmit          */
+#define MII_BCM54XX_LRELPA		0x07	/* LDS Link Partner Ability                */
+#define MII_BCM54XX_LRELPNPM		0x08	/* LDS Link Partner Next Page Message      */
+#define MII_BCM54XX_LRELPNPC		0x09	/* LDS Link Partner Next Page Control      */
+#define MII_BCM54XX_LRELDSE		0x0a	/* LDS Expansion Register                  */
+#define MII_BCM54XX_LREES		0x0f	/* LRE Extended Status                     */
+
+/* LRE control register. */
+#define LRECR_RESET			0x8000	/* Reset to default state      */
+#define LRECR_LOOPBACK			0x4000	/* Internal Loopback           */
+#define LRECR_LDSRES			0x2000	/* Restart LDS Process         */
+#define LRECR_LDSEN			0x1000	/* LDS Enable                  */
+#define LRECR_PDOWN			0x0800	/* Enable low power state      */
+#define LRECR_ISOLATE			0x0400	/* Isolate data paths from MII */
+#define LRECR_SPEED100			0x0200	/* Select 100 Mbps             */
+#define LRECR_SPEED10			0x0000	/* Select 10 Mbps              */
+#define LRECR_4PAIRS			0x0020	/* Select 4 Pairs              */
+#define LRECR_2PAIRS			0x0010	/* Select 2 Pairs              */
+#define LRECR_1PAIR			0x0000	/* Select 1 Pair               */
+#define LRECR_MASTER			0x0008	/* Force Master when LDS disabled */
+#define LRECR_SLAVE			0x0000	/* Force Slave when LDS disabled  */
+
+/* LRE status register. */
+#define LRESR_ERCAP			0x0001	/* Ext-reg capability          */
+#define LRESR_JCD			0x0002	/* Jabber detected             */
+#define LRESR_LSTATUS			0x0004	/* Link status                 */
+#define LRESR_LDSABILITY		0x0008	/* Can do LDS                  */
+#define LRESR_8023			0x0010	/* Has IEEE 802.3 Support      */
+#define LRESR_LDSCOMPLETE		0x0020	/* LDS Auto-negotiation complete */
+#define LRESR_MFPS			0x0040	/* Can suppress Management Frames Preamble */
+#define LRESR_RESV			0x0080	/* Unused...                   */
+#define LRESR_ESTATEN			0x0100	/* Extended Status in R15      */
+#define LRESR_10_1PAIR			0x0200	/* Can do 10Mbps 1 Pair        */
+#define LRESR_10_2PAIR			0x0400	/* Can do 10Mbps 2 Pairs       */
+#define LRESR_100_2PAIR			0x0800	/* Can do 100Mbps 2 Pairs      */
+#define LRESR_100_4PAIR			0x1000	/* Can do 100Mbps 4 Pairs      */
+#define LRESR_100_1PAIR			0x2000	/* Can do 100Mbps 1 Pair       */
+
+/* LDS Auto-Negotiation Advertised Ability. */
+#define LREANAA_PAUSE_ASYM		0x8000	/* Can pause asymmetrically    */
+#define LREANAA_PAUSE			0x4000	/* Can pause                   */
+#define LREANAA_100_1PAIR		0x0020	/* Can do 100Mbps 1 Pair       */
+#define LREANAA_100_4PAIR		0x0010	/* Can do 100Mbps 4 Pair       */
+#define LREANAA_100_2PAIR		0x0008	/* Can do 100Mbps 2 Pair       */
+#define LREANAA_10_2PAIR		0x0004	/* Can do 10Mbps 2 Pair        */
+#define LREANAA_10_1PAIR		0x0002	/* Can do 10Mbps 1 Pair        */
+
+#define LRE_ADVERTISE_FULL		(LREANAA_100_1PAIR | LREANAA_100_4PAIR | \
+					 LREANAA_100_2PAIR | LREANAA_10_2PAIR | \
+					 LREANAA_10_1PAIR)
+
+#define LRE_ADVERTISE_ALL		LRE_ADVERTISE_FULL
+
+/* LDS Link Partner Ability. */
+#define LRELPA_PAUSE_ASYM		0x8000	/* Supports asymmetric pause   */
+#define LRELPA_PAUSE			0x4000	/* Supports pause capability   */
+#define LRELPA_100_1PAIR		0x0020	/* 100Mbps 1 Pair capable      */
+#define LRELPA_100_4PAIR		0x0010	/* 100Mbps 4 Pair capable      */
+#define LRELPA_100_2PAIR		0x0008	/* 100Mbps 2 Pair capable      */
+#define LRELPA_10_2PAIR			0x0004	/* 10Mbps 2 Pair capable       */
+#define LRELPA_10_1PAIR			0x0002	/* 10Mbps 1 Pair capable       */
+
+/* LDS Expansion register. */
+#define LDSE_DOWNGRADE			0x8000	/* Can do LDS Speed Downgrade  */
+#define LDSE_MASTER			0x4000	/* Master / Slave              */
+#define LDSE_PAIRS_MASK			0x3000	/* Pair Count Mask             */
+#define LDSE_4PAIRS			0x2000	/* 4 Pairs Connection          */
+#define LDSE_2PAIRS			0x1000	/* 2 Pairs Connection          */
+#define LDSE_1PAIR			0x0000	/* 1 Pair  Connection          */
+#define LDSE_CABLEN_MASK		0x0FFF	/* Cable Length Mask           */
 
 /* BCM54810 Registers */
 #define BCM54810_EXP_BROADREACH_LRE_MISC_CTL	(MII_BCM54XX_EXP_SEL_ER + 0x90)
 #define BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN	(1 << 0)
 #define BCM54810_SHD_CLK_CTL			0x3
 #define BCM54810_SHD_CLK_CTL_GTXCLK_EN		(1 << 9)
+#define BCM54810_SHD_SCR3_TRDDAPD		0x0100
+
+/* BCM54811 Registers */
+#define BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL	(MII_BCM54XX_EXP_SEL_ER + 0x9A)
+/* Access Control Override Enable */
+#define BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_EN		BIT(15)
+/* Access Control Override Value */
+#define BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_OVERRIDE_VAL	BIT(14)
+/* Access Control Value */
+#define BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_VAL		BIT(13)
 
 /* BCM54612E Registers */
 #define BCM54612E_EXP_SPARE0		(MII_BCM54XX_EXP_SEL_ETC + 0x34)
-#define BCM54612E_LED4_CLK125OUT_EN	(1 << 1)
+#define BCM54612E_LED4_CLK125OUT_EN	BIT(1)
 
 
 /* Wake-on-LAN registers */
-- 
2.39.2

Re: [PATCH v3 2/3] net: phy: bcm54811: Add LRE registers definitions

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 7065 bytes --]

On 5/6/24 07:40, Kamil Horák - 2N wrote:
> Add the definitions of LRE registers for Broadcom BCM5481x PHY

Missing Signed-off-by tag.

> ---
>   include/linux/brcmphy.h | 91 ++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 90 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
> index 1394ba302367..9c0b78c1b6fb 100644
> --- a/include/linux/brcmphy.h
> +++ b/include/linux/brcmphy.h
> @@ -270,16 +270,105 @@
>   #define BCM5482_SSD_SGMII_SLAVE		0x15	/* SGMII Slave Register */
>   #define BCM5482_SSD_SGMII_SLAVE_EN	0x0002	/* Slave mode enable */
>   #define BCM5482_SSD_SGMII_SLAVE_AD	0x0001	/* Slave auto-detection */
> +#define BCM5482_SSD_SGMII_SLAVE_AD	0x0001	/* Slave auto-detection */
> +
> +/* BroadR-Reach LRE Registers. */
> +#define MII_BCM54XX_LRECR		0x00	/* LRE Control Register                    */
> +#define MII_BCM54XX_LRESR		0x01	/* LRE Status Register                     */
> +#define MII_BCM54XX_LREPHYSID1		0x02	/* LRE PHYS ID 1                           */
> +#define MII_BCM54XX_LREPHYSID2		0x03	/* LRE PHYS ID 2                           */
> +#define MII_BCM54XX_LREANAA		0x04	/* LDS Auto-Negotiation Advertised Ability */
> +#define MII_BCM54XX_LREANAC		0x05	/* LDS Auto-Negotiation Advertised Control */
> +#define MII_BCM54XX_LREANPT		0x06	/* LDS Ability Next Page Transmit          */
> +#define MII_BCM54XX_LRELPA		0x07	/* LDS Link Partner Ability                */
> +#define MII_BCM54XX_LRELPNPM		0x08	/* LDS Link Partner Next Page Message      */
> +#define MII_BCM54XX_LRELPNPC		0x09	/* LDS Link Partner Next Page Control      */
> +#define MII_BCM54XX_LRELDSE		0x0a	/* LDS Expansion Register                  */
> +#define MII_BCM54XX_LREES		0x0f	/* LRE Extended Status                     */
> +
> +/* LRE control register. */
> +#define LRECR_RESET			0x8000	/* Reset to default state      */
> +#define LRECR_LOOPBACK			0x4000	/* Internal Loopback           */
> +#define LRECR_LDSRES			0x2000	/* Restart LDS Process         */
> +#define LRECR_LDSEN			0x1000	/* LDS Enable                  */
> +#define LRECR_PDOWN			0x0800	/* Enable low power state      */
> +#define LRECR_ISOLATE			0x0400	/* Isolate data paths from MII */
> +#define LRECR_SPEED100			0x0200	/* Select 100 Mbps             */
> +#define LRECR_SPEED10			0x0000	/* Select 10 Mbps              */
> +#define LRECR_4PAIRS			0x0020	/* Select 4 Pairs              */
> +#define LRECR_2PAIRS			0x0010	/* Select 2 Pairs              */
> +#define LRECR_1PAIR			0x0000	/* Select 1 Pair               */
> +#define LRECR_MASTER			0x0008	/* Force Master when LDS disabled */
> +#define LRECR_SLAVE			0x0000	/* Force Slave when LDS disabled  */

So previous register had definitions ordered from MSB to LSB...

> +
> +/* LRE status register. */
> +#define LRESR_ERCAP			0x0001	/* Ext-reg capability          */

and here we have LSB to MSB, seems like you chose MSB to LSB, so we 
should stick to that.

> +#define LRESR_JCD			0x0002	/* Jabber detected             */
> +#define LRESR_LSTATUS			0x0004	/* Link status                 */
> +#define LRESR_LDSABILITY		0x0008	/* Can do LDS                  */

Comment should be LDS auto-negotiation capable, other comments are fine.

> +#define LRESR_8023			0x0010	/* Has IEEE 802.3 Support      */
> +#define LRESR_LDSCOMPLETE		0x0020	/* LDS Auto-negotiation complete */
> +#define LRESR_MFPS			0x0040	/* Can suppress Management Frames Preamble */
> +#define LRESR_RESV			0x0080	/* Unused...                   */
> +#define LRESR_ESTATEN			0x0100	/* Extended Status in R15      */
> +#define LRESR_10_1PAIR			0x0200	/* Can do 10Mbps 1 Pair        */
> +#define LRESR_10_2PAIR			0x0400	/* Can do 10Mbps 2 Pairs       */
> +#define LRESR_100_2PAIR			0x0800	/* Can do 100Mbps 2 Pairs      */
> +#define LRESR_100_4PAIR			0x1000	/* Can do 100Mbps 4 Pairs      */
> +#define LRESR_100_1PAIR			0x2000	/* Can do 100Mbps 1 Pair       */
> +
> +/* LDS Auto-Negotiation Advertised Ability. */
> +#define LREANAA_PAUSE_ASYM		0x8000	/* Can pause asymmetrically    */
> +#define LREANAA_PAUSE			0x4000	/* Can pause                   */
> +#define LREANAA_100_1PAIR		0x0020	/* Can do 100Mbps 1 Pair       */
> +#define LREANAA_100_4PAIR		0x0010	/* Can do 100Mbps 4 Pair       */
> +#define LREANAA_100_2PAIR		0x0008	/* Can do 100Mbps 2 Pair       */
> +#define LREANAA_10_2PAIR		0x0004	/* Can do 10Mbps 2 Pair        */
> +#define LREANAA_10_1PAIR		0x0002	/* Can do 10Mbps 1 Pair        */
> +
> +#define LRE_ADVERTISE_FULL		(LREANAA_100_1PAIR | LREANAA_100_4PAIR | \
> +					 LREANAA_100_2PAIR | LREANAA_10_2PAIR | \
> +					 LREANAA_10_1PAIR)
> +
> +#define LRE_ADVERTISE_ALL		LRE_ADVERTISE_FULL
> +
> +/* LDS Link Partner Ability. */
> +#define LRELPA_PAUSE_ASYM		0x8000	/* Supports asymmetric pause   */
> +#define LRELPA_PAUSE			0x4000	/* Supports pause capability   */
> +#define LRELPA_100_1PAIR		0x0020	/* 100Mbps 1 Pair capable      */
> +#define LRELPA_100_4PAIR		0x0010	/* 100Mbps 4 Pair capable      */
> +#define LRELPA_100_2PAIR		0x0008	/* 100Mbps 2 Pair capable      */
> +#define LRELPA_10_2PAIR			0x0004	/* 10Mbps 2 Pair capable       */
> +#define LRELPA_10_1PAIR			0x0002	/* 10Mbps 1 Pair capable       */
> +
> +/* LDS Expansion register. */
> +#define LDSE_DOWNGRADE			0x8000	/* Can do LDS Speed Downgrade  */
> +#define LDSE_MASTER			0x4000	/* Master / Slave              */
> +#define LDSE_PAIRS_MASK			0x3000	/* Pair Count Mask             */

Please define LDSE_PAIRS_SHIFT and make the LDSE_%dPAIRS left shift by 12.

> +#define LDSE_4PAIRS			0x2000	/* 4 Pairs Connection          */
> +#define LDSE_2PAIRS			0x1000	/* 2 Pairs Connection          */
> +#define LDSE_1PAIR			0x0000	/* 1 Pair  Connection          */
> +#define LDSE_CABLEN_MASK		0x0FFF	/* Cable Length Mask           */
>   
>   /* BCM54810 Registers */
>   #define BCM54810_EXP_BROADREACH_LRE_MISC_CTL	(MII_BCM54XX_EXP_SEL_ER + 0x90)
>   #define BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN	(1 << 0)
>   #define BCM54810_SHD_CLK_CTL			0x3
>   #define BCM54810_SHD_CLK_CTL_GTXCLK_EN		(1 << 9)
> +#define BCM54810_SHD_SCR3_TRDDAPD		0x0100

Unrelated change?

> +
> +/* BCM54811 Registers */
> +#define BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL	(MII_BCM54XX_EXP_SEL_ER + 0x9A)
> +/* Access Control Override Enable */
> +#define BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_EN		BIT(15)
> +/* Access Control Override Value */
> +#define BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_OVERRIDE_VAL	BIT(14)
> +/* Access Control Value */
> +#define BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_VAL		BIT(13)

This register is not documented in my datasheet, but register 0x0E is, I 
am fine with using that one though.

>   
>   /* BCM54612E Registers */
>   #define BCM54612E_EXP_SPARE0		(MII_BCM54XX_EXP_SEL_ETC + 0x34)
> -#define BCM54612E_LED4_CLK125OUT_EN	(1 << 1)
> +#define BCM54612E_LED4_CLK125OUT_EN	BIT(1)

Unrelated change, and one that is arguably inconsistent with the 
definitions you are adding, I would stick with the style you used, since 
there are precedents for that in brcmphy.h.
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH v3 2/3] net: phy: bcm54811: Add LRE registers definitions

[Andrew Lunn]

On Mon, May 06, 2024 at 04:40:14PM +0200, Kamil Horák - 2N wrote:
> Add the definitions of LRE registers for Broadcom BCM5481x PHY
> ---
>  include/linux/brcmphy.h | 91 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 90 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
> index 1394ba302367..9c0b78c1b6fb 100644
> --- a/include/linux/brcmphy.h
> +++ b/include/linux/brcmphy.h
> @@ -270,16 +270,105 @@
>  #define BCM5482_SSD_SGMII_SLAVE		0x15	/* SGMII Slave Register */
>  #define BCM5482_SSD_SGMII_SLAVE_EN	0x0002	/* Slave mode enable */
>  #define BCM5482_SSD_SGMII_SLAVE_AD	0x0001	/* Slave auto-detection */
> +#define BCM5482_SSD_SGMII_SLAVE_AD	0x0001	/* Slave auto-detection */
> +
> +/* BroadR-Reach LRE Registers. */
> +#define MII_BCM54XX_LRECR		0x00	/* LRE Control Register                    */
> +#define MII_BCM54XX_LRESR		0x01	/* LRE Status Register                     */
> +#define MII_BCM54XX_LREPHYSID1		0x02	/* LRE PHYS ID 1                           */
> +#define MII_BCM54XX_LREPHYSID2		0x03	/* LRE PHYS ID 2                           */
> +#define MII_BCM54XX_LREANAA		0x04	/* LDS Auto-Negotiation Advertised Ability */
> +#define MII_BCM54XX_LREANAC		0x05	/* LDS Auto-Negotiation Advertised Control */
> +#define MII_BCM54XX_LREANPT		0x06	/* LDS Ability Next Page Transmit          */
> +#define MII_BCM54XX_LRELPA		0x07	/* LDS Link Partner Ability                */
> +#define MII_BCM54XX_LRELPNPM		0x08	/* LDS Link Partner Next Page Message      */
> +#define MII_BCM54XX_LRELPNPC		0x09	/* LDS Link Partner Next Page Control      */
> +#define MII_BCM54XX_LRELDSE		0x0a	/* LDS Expansion Register                  */
> +#define MII_BCM54XX_LREES		0x0f	/* LRE Extended Status                     */

Please look at these side by side to standard C22 registers. Which are
different? Is LRECR actually different to BMCR that it needs it own
define? Is LRESR different enought to BMSR that it needs its own
define? Does the ID registers use a different format?

> +/* LRE control register. */
> +#define LRECR_RESET			0x8000	/* Reset to default state      */
> +#define LRECR_LOOPBACK			0x4000	/* Internal Loopback           */
> +#define LRECR_LDSRES			0x2000	/* Restart LDS Process         */
> +#define LRECR_LDSEN			0x1000	/* LDS Enable                  */
> +#define LRECR_PDOWN			0x0800	/* Enable low power state      */
> +#define LRECR_ISOLATE			0x0400	/* Isolate data paths from MII */
> +#define LRECR_SPEED100			0x0200	/* Select 100 Mbps             */
> +#define LRECR_SPEED10			0x0000	/* Select 10 Mbps              */
> +#define LRECR_4PAIRS			0x0020	/* Select 4 Pairs              */
> +#define LRECR_2PAIRS			0x0010	/* Select 2 Pairs              */
> +#define LRECR_1PAIR			0x0000	/* Select 1 Pair               */
> +#define LRECR_MASTER			0x0008	/* Force Master when LDS disabled */
> +#define LRECR_SLAVE			0x0000	/* Force Slave when LDS disabled  */

Reverse the order of these, and then compare them to:

/* Basic mode control register. */
#define BMCR_SPEED1000          0x0040  /* MSB of Speed (1000)         */
#define BMCR_CTST               0x0080  /* Collision test              */
#define BMCR_FULLDPLX           0x0100  /* Full duplex                 */
#define BMCR_ANRESTART          0x0200  /* Auto negotiation restart    */
#define BMCR_ISOLATE            0x0400  /* Isolate data paths from MII */
#define BMCR_PDOWN              0x0800  /* Enable low power state      */
#define BMCR_ANENABLE           0x1000  /* Enable auto negotiation     */
#define BMCR_SPEED100           0x2000  /* Select 100Mbps              */
#define BMCR_LOOPBACK           0x4000  /* TXD loopback bits           */
#define BMCR_RESET              0x8000  /* Reset to default state      */

Drop any which are the same, and just add defined for those which are
different.


> +
> +/* LRE status register. */
> +#define LRESR_ERCAP			0x0001	/* Ext-reg capability          */
> +#define LRESR_JCD			0x0002	/* Jabber detected             */
> +#define LRESR_LSTATUS			0x0004	/* Link status                 */
> +#define LRESR_LDSABILITY		0x0008	/* Can do LDS                  */

What does LDS mean? In BMSR this bit is about auto-neg. How does LDS
differ from auto-neg?

Ideally, where the functionality is the same, please use the existing
definitions. It makes it easier for somebody who knows C22 to look at
the code and understand it. And it makes it easier to spot where the
differences actually are.

	Andrew

[PATCH v3 3/3] net: phy: bcm-phy-lib: Implement BroadR-Reach link modes

[Kamil Horák - 2N]

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.

Signed-off-by: Kamil Horák - 2N <kamilh@axis.com>
---
 drivers/net/phy/bcm-phy-lib.c | 122 ++++++++++++
 drivers/net/phy/bcm-phy-lib.h |   4 +
 drivers/net/phy/broadcom.c    | 338 ++++++++++++++++++++++++++++++++--
 3 files changed, 449 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index 876f28fd8256..9fa2a20e641f 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -794,6 +794,46 @@ static int _bcm_phy_cable_test_get_status(struct phy_device *phydev,
 	return ret;
 }
 
+static int bcm_setup_forced(struct phy_device *phydev)
+{
+	u16 ctl = 0;
+
+	phydev->pause = 0;
+	phydev->asym_pause = 0;
+
+	if (phydev->speed == SPEED_100)
+		ctl |= LRECR_SPEED100;
+
+	if (phydev->duplex != DUPLEX_FULL)
+		return -EOPNOTSUPP;
+
+	return phy_modify(phydev, MII_BCM54XX_LRECR, LRECR_SPEED100, ctl);
+}
+
+/**
+ * bcm_linkmode_adv_to_mii_adv_t
+ * @advertising: the linkmode advertisement settings
+ *
+ * A small helper function that translates linkmode advertisement
+ * settings to phy autonegotiation advertisements for the
+ * MII_BCM54XX_LREANAA register.
+ */
+static inline u32 bcm_linkmode_adv_to_mii_adv_t(unsigned long *advertising)
+{
+	u32 result = 0;
+
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_1BR10_BIT, advertising))
+		result |= LREANAA_10_1PAIR;
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_100baseT1_Full_BIT, advertising))
+		result |= LREANAA_100_1PAIR;
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT, advertising))
+		result |= LRELPA_PAUSE;
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, advertising))
+		result |= LRELPA_PAUSE_ASYM;
+
+	return result;
+}
+
 int bcm_phy_cable_test_start(struct phy_device *phydev)
 {
 	return _bcm_phy_cable_test_start(phydev, false);
@@ -1066,6 +1106,88 @@ int bcm_phy_led_brightness_set(struct phy_device *phydev,
 }
 EXPORT_SYMBOL_GPL(bcm_phy_led_brightness_set);
 
+int bcm_setup_master_slave(struct phy_device *phydev)
+{
+	u16 ctl = 0;
+
+	switch (phydev->master_slave_set) {
+	case MASTER_SLAVE_CFG_MASTER_PREFERRED:
+	case MASTER_SLAVE_CFG_MASTER_FORCE:
+		ctl = LRECR_MASTER;
+		break;
+	case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
+	case MASTER_SLAVE_CFG_SLAVE_FORCE:
+		break;
+	case MASTER_SLAVE_CFG_UNKNOWN:
+	case MASTER_SLAVE_CFG_UNSUPPORTED:
+		return 0;
+	default:
+		phydev_warn(phydev, "Unsupported Master/Slave mode\n");
+		return -EOPNOTSUPP;
+	}
+
+	return phy_modify_changed(phydev, MII_BCM54XX_LRECR, LRECR_MASTER, ctl);
+}
+EXPORT_SYMBOL_GPL(bcm_setup_master_slave);
+
+int bcm_config_aneg(struct phy_device *phydev, bool changed)
+{
+	int err;
+
+	if (genphy_config_eee_advert(phydev))
+		changed = true;
+
+	err = bcm_setup_master_slave(phydev);
+	if (err < 0)
+		return err;
+	else if (err)
+		changed = true;
+
+	if (phydev->autoneg != AUTONEG_ENABLE)
+		return bcm_setup_forced(phydev);
+
+	err = bcm_config_advert(phydev);
+	if (err < 0) /* error */
+		return err;
+	else if (err)
+		changed = true;
+
+	return genphy_check_and_restart_aneg(phydev, changed);
+}
+EXPORT_SYMBOL_GPL(bcm_config_aneg);
+
+/**
+ * bcm_config_advert - sanitize and advertise auto-negotiation parameters
+ * @phydev: target phy_device struct
+ *
+ * Description: Writes MII_BCM54XX_LREANAA with the appropriate values,
+ *   after sanitizing the values to make sure we only advertise
+ *   what is supported.  Returns < 0 on error, 0 if the PHY's advertisement
+ *   hasn't changed, and > 0 if it has changed.
+ */
+int bcm_config_advert(struct phy_device *phydev)
+{
+	int err;
+	u32 adv;
+
+	/* Only allow advertising what this PHY supports */
+	linkmode_and(phydev->advertising, phydev->advertising,
+		     phydev->supported);
+
+	adv = bcm_linkmode_adv_to_mii_adv_t(phydev->advertising);
+
+	/* Setup BroadR-Reach mode advertisement */
+	err = phy_modify_changed(phydev, MII_BCM54XX_LREANAA,
+				 LRE_ADVERTISE_ALL | LREANAA_PAUSE |
+				 LREANAA_PAUSE_ASYM, adv);
+
+	if (err < 0)
+		return err;
+
+	return err > 0 ? 1 : 0;
+}
+EXPORT_SYMBOL_GPL(bcm_config_advert);
+
 MODULE_DESCRIPTION("Broadcom PHY Library");
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Broadcom Corporation");
diff --git a/drivers/net/phy/bcm-phy-lib.h b/drivers/net/phy/bcm-phy-lib.h
index b52189e45a84..0f6d06c0b7af 100644
--- a/drivers/net/phy/bcm-phy-lib.h
+++ b/drivers/net/phy/bcm-phy-lib.h
@@ -121,4 +121,8 @@ irqreturn_t bcm_phy_wol_isr(int irq, void *dev_id);
 int bcm_phy_led_brightness_set(struct phy_device *phydev,
 			       u8 index, enum led_brightness value);
 
+int bcm_setup_master_slave(struct phy_device *phydev);
+int bcm_config_aneg(struct phy_device *phydev, bool changed);
+int bcm_config_advert(struct phy_device *phydev);
+
 #endif /* _LINUX_BCM_PHY_LIB_H */
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 370e4ed45098..2d8898fd2228 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -553,18 +553,46 @@ static int bcm54810_write_mmd(struct phy_device *phydev, int devnum, u16 regnum,
 	return -EOPNOTSUPP;
 }
 
-static int bcm54811_config_init(struct phy_device *phydev)
+static int bcm5481x_get_brrmode(struct phy_device *phydev, u8 *data)
 {
-	int err, reg;
+	int reg;
 
-	/* Disable BroadR-Reach function. */
 	reg = bcm_phy_read_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL);
-	reg &= ~BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN;
-	err = bcm_phy_write_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL,
-				reg);
-	if (err < 0)
+
+	*data = (reg & BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN) ?
+				ETHTOOL_PHY_BRR_MODE_ON : ETHTOOL_PHY_BRR_MODE_OFF;
+
+	return 0;
+}
+
+static int bcm5481x_set_brrmode(struct phy_device *phydev, u8 on)
+{
+	int reg;
+	int err;
+
+	reg = bcm_phy_read_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL);
+
+	if (on)
+		reg |= BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN;
+	else
+		reg &= ~BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN;
+
+	err = bcm_phy_write_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL, reg);
+	if (err)
 		return err;
 
+	/* Ensure LRE or IEEE register set is accessed according to the brr on/off,
+	 *  thus set the override
+	 */
+	return bcm_phy_write_exp(phydev, BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL,
+		BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_EN |
+		on ? 0 : BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_OVERRIDE_VAL);
+}
+
+static int bcm54811_config_init(struct phy_device *phydev)
+{
+	int err, reg;
+
 	err = bcm54xx_config_init(phydev);
 
 	/* Enable CLK125 MUX on LED4 if ref clock is enabled. */
@@ -576,18 +604,16 @@ static int bcm54811_config_init(struct phy_device *phydev)
 			return err;
 	}
 
-	return err;
+	/* Configure BroadR-Reach function. */
+	return  bcm5481x_set_brrmode(phydev, ETHTOOL_PHY_BRR_MODE_OFF);
 }
 
-static int bcm5481_config_aneg(struct phy_device *phydev)
+static int bcm5481x_config_delay_swap(struct phy_device *phydev)
 {
 	struct device_node *np = phydev->mdio.dev.of_node;
-	int ret;
-
-	/* Aneg firstly. */
-	ret = genphy_config_aneg(phydev);
+	int ret = 0;
 
-	/* Then we can set up the delay. */
+	/* Set up the delay. */
 	bcm54xx_config_clock_delay(phydev);
 
 	if (of_property_read_bool(np, "enet-phy-lane-swap")) {
@@ -601,6 +627,56 @@ static int bcm5481_config_aneg(struct phy_device *phydev)
 	return ret;
 }
 
+static int bcm5481_config_aneg(struct phy_device *phydev)
+{
+	int ret;
+	u8 brr_mode;
+
+	/* Aneg firstly. */
+	ret = bcm5481x_get_brrmode(phydev, &brr_mode);
+	if (ret)
+		return ret;
+
+	if (brr_mode == ETHTOOL_PHY_BRR_MODE_ON)
+		ret = bcm_config_aneg(phydev, false);
+	else
+		ret = genphy_config_aneg(phydev);
+
+	if (ret)
+		return ret;
+
+	/* Then we can set up the delay and swap. */
+	return bcm5481x_config_delay_swap(phydev);
+}
+
+static int bcm54811_config_aneg(struct phy_device *phydev)
+{
+	int ret;
+	u8 brr_mode;
+
+	/* Aneg firstly. */
+	ret = bcm5481x_get_brrmode(phydev, &brr_mode);
+	if (ret)
+		return ret;
+
+	if (brr_mode == ETHTOOL_PHY_BRR_MODE_ON) {
+		/* BCM54811 is only capable of autonegotiation in IEEE mode */
+		if (phydev->autoneg)
+			return -EOPNOTSUPP;
+
+		ret = bcm_config_aneg(phydev, false);
+
+	} else {
+		ret = genphy_config_aneg(phydev);
+	}
+
+	if (ret)
+		return ret;
+
+	/* Then we can set up the delay and swap. */
+	return bcm5481x_config_delay_swap(phydev);
+}
+
 struct bcm54616s_phy_priv {
 	bool mode_1000bx_en;
 };
@@ -1062,6 +1138,234 @@ static void bcm54xx_link_change_notify(struct phy_device *phydev)
 	bcm_phy_write_exp(phydev, MII_BCM54XX_EXP_EXP08, ret);
 }
 
+static int bcm54811_read_abilities(struct phy_device *phydev)
+{
+	int val, err;
+	int i;
+	static const int modes_array[] = {ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
+					  ETHTOOL_LINK_MODE_1BR10_BIT,
+					  ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+					  ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
+					  ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
+					  ETHTOOL_LINK_MODE_100baseT_Full_BIT,
+					  ETHTOOL_LINK_MODE_100baseT_Half_BIT,
+					  ETHTOOL_LINK_MODE_10baseT_Full_BIT,
+					  ETHTOOL_LINK_MODE_10baseT_Half_BIT};
+
+	u8 brr_mode;
+
+	for (i = 0; i < ARRAY_SIZE(modes_array); i++)
+		linkmode_clear_bit(modes_array[i], phydev->supported);
+
+	err = bcm5481x_get_brrmode(phydev, &brr_mode);
+
+	if (err)
+		return err;
+
+	if (brr_mode == ETHTOOL_PHY_BRR_MODE_ON) {
+		linkmode_set_bit_array(phy_basic_ports_array,
+				       ARRAY_SIZE(phy_basic_ports_array),
+				       phydev->supported);
+
+		val = phy_read(phydev, MII_BCM54XX_LRESR);
+		if (val < 0)
+			return val;
+
+		linkmode_mod_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+				 phydev->supported, 1);
+		linkmode_mod_bit(ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
+				 phydev->supported,
+				 val & LRESR_100_1PAIR);
+		linkmode_mod_bit(ETHTOOL_LINK_MODE_1BR10_BIT,
+				 phydev->supported,
+				 val & LRESR_10_1PAIR);
+	} else {
+		return genphy_read_abilities(phydev);
+	}
+
+	return err;
+}
+
+static int bcm5481x_get_tunable(struct phy_device *phydev,
+				struct ethtool_tunable *tuna, void *data)
+{
+	switch (tuna->id) {
+	case ETHTOOL_PHY_BRR_MODE:
+		return bcm5481x_get_brrmode(phydev, data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int bcm5481x_set_tunable(struct phy_device *phydev,
+				struct ethtool_tunable *tuna, const void *data)
+{
+	int res;
+
+	switch (tuna->id) {
+	case ETHTOOL_PHY_BRR_MODE:
+		res =  bcm5481x_set_brrmode(phydev, *(const u8 *)data);
+		if (res >= 0)
+			res = bcm54811_read_abilities(phydev);
+		break;
+	default:
+		res = -EOPNOTSUPP;
+	}
+
+	return res;
+}
+
+static int bcm_read_master_slave(struct phy_device *phydev)
+{
+	int cfg, state;
+	int val;
+
+	/* In BroadR-Reach mode we are always capable of master-slave
+	 *  and there is no preferred master or slave configuration
+	 */
+	phydev->master_slave_get = MASTER_SLAVE_CFG_UNKNOWN;
+	phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
+
+	val = phy_read(phydev, MII_BCM54XX_LRECR);
+	if (val < 0)
+		return val;
+
+	if ((val & LRECR_LDSEN) == 0) {
+		if (val & LRECR_MASTER)
+			cfg = MASTER_SLAVE_CFG_MASTER_FORCE;
+		else
+			cfg = MASTER_SLAVE_CFG_SLAVE_FORCE;
+	}
+
+	val = phy_read(phydev, MII_BCM54XX_LRELDSE);
+	if (val < 0)
+		return val;
+
+	if (val & LDSE_MASTER)
+		state = MASTER_SLAVE_STATE_MASTER;
+	else
+		state = MASTER_SLAVE_STATE_SLAVE;
+
+	phydev->master_slave_get = cfg;
+	phydev->master_slave_state = state;
+
+	return 0;
+}
+
+/* Read LDS Link Partner Ability in BroadR-Reach mode */
+static int bcm_read_lpa(struct phy_device *phydev)
+{
+	int i, lrelpa;
+
+	if (phydev->autoneg != AUTONEG_ENABLE) {
+		if (!phydev->autoneg_complete) {
+			/* aneg not yet done, reset all relevant bits */
+			static int br_bits[] = { ETHTOOL_LINK_MODE_Autoneg_BIT,
+						 ETHTOOL_LINK_MODE_Pause_BIT,
+						 ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+						 ETHTOOL_LINK_MODE_1BR10_BIT,
+						 ETHTOOL_LINK_MODE_100baseT1_Full_BIT };
+			for (i = 0; i < ARRAY_SIZE(br_bits); i++)
+				linkmode_clear_bit(br_bits[i], phydev->lp_advertising);
+
+			return 0;
+		}
+
+		/* Long-Distance-Signalling Link Partner Ability */
+		lrelpa = phy_read(phydev, MII_BCM54XX_LRELPA);
+		if (lrelpa < 0)
+			return lrelpa;
+
+		linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+				 phydev->lp_advertising, lrelpa & LRELPA_PAUSE_ASYM);
+		linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+				 phydev->lp_advertising, lrelpa & LRELPA_PAUSE);
+		linkmode_mod_bit(ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
+				 phydev->lp_advertising, lrelpa & LRELPA_100_1PAIR);
+		linkmode_mod_bit(ETHTOOL_LINK_MODE_1BR10_BIT,
+				 phydev->lp_advertising, lrelpa & LRELPA_10_1PAIR);
+	} else {
+		linkmode_zero(phydev->lp_advertising);
+	}
+
+	return 0;
+}
+
+static int bcm_read_status_fixed(struct phy_device *phydev)
+{
+	int lrecr = phy_read(phydev, MII_BCM54XX_LRECR);
+
+	if (lrecr < 0)
+		return lrecr;
+
+	phydev->duplex = DUPLEX_FULL;
+
+	if (lrecr & LRECR_SPEED100)
+		phydev->speed = SPEED_100;
+	else
+		phydev->speed = SPEED_10;
+
+	return 0;
+}
+
+static int bcm54811_read_status(struct phy_device *phydev)
+{
+	int err;
+	u8 brr_mode;
+
+	err = bcm5481x_get_brrmode(phydev, &brr_mode);
+
+	if (err)
+		return err;
+
+	if (brr_mode == ETHTOOL_PHY_BRR_MODE_ON) {
+		/* Get the status in BroadRReach mode just like genphy_read_status
+		 *   does in normal mode
+		 */
+
+		int err, old_link = phydev->link;
+
+		/* Update the link, but return if there was an error
+		 *  genphy_update_link() functions equally on IEEE and LRE
+		 *  register set
+		 */
+
+		err = genphy_update_link(phydev);
+		if (err)
+			return err;
+
+		/* why bother the PHY if nothing can have changed */
+		if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
+			return 0;
+
+		phydev->speed = SPEED_UNKNOWN;
+		phydev->duplex = DUPLEX_UNKNOWN;
+		phydev->pause = 0;
+		phydev->asym_pause = 0;
+
+		err = bcm_read_master_slave(phydev);
+		if (err < 0)
+			return err;
+
+		/* Read LDS Link Partner Ability */
+		err = bcm_read_lpa(phydev);
+		if (err < 0)
+			return err;
+
+		if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
+			phy_resolve_aneg_linkmode(phydev);
+		} else if (phydev->autoneg == AUTONEG_DISABLE) {
+			err = bcm_read_status_fixed(phydev);
+			if (err < 0)
+				return err;
+		}
+	} else {
+		err = genphy_read_status(phydev);
+	}
+
+	return err;
+}
+
 static struct phy_driver broadcom_drivers[] = {
 {
 	.phy_id		= PHY_ID_BCM5411,
@@ -1212,9 +1516,13 @@ static struct phy_driver broadcom_drivers[] = {
 	.get_stats	= bcm54xx_get_stats,
 	.probe		= bcm54xx_phy_probe,
 	.config_init    = bcm54811_config_init,
-	.config_aneg    = bcm5481_config_aneg,
+	.config_aneg    = bcm54811_config_aneg,
 	.config_intr    = bcm_phy_config_intr,
 	.handle_interrupt = bcm_phy_handle_interrupt,
+	.read_status	= bcm54811_read_status,
+	.get_tunable	= bcm5481x_get_tunable,
+	.set_tunable	= bcm5481x_set_tunable,
+	.get_features	= bcm54811_read_abilities,
 	.suspend	= bcm54xx_suspend,
 	.resume		= bcm54xx_resume,
 	.link_change_notify	= bcm54xx_link_change_notify,
-- 
2.39.2

Re: [PATCH v3 3/3] net: phy: bcm-phy-lib: Implement BroadR-Reach link modes

[Andrew Lunn]

On Mon, May 06, 2024 at 04:40:15PM +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.
> 
> Signed-off-by: Kamil Horák - 2N <kamilh@axis.com>
> ---
>  drivers/net/phy/bcm-phy-lib.c | 122 ++++++++++++
>  drivers/net/phy/bcm-phy-lib.h |   4 +
>  drivers/net/phy/broadcom.c    | 338 ++++++++++++++++++++++++++++++++--
>  3 files changed, 449 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
> index 876f28fd8256..9fa2a20e641f 100644
> --- a/drivers/net/phy/bcm-phy-lib.c
> +++ b/drivers/net/phy/bcm-phy-lib.c
> @@ -794,6 +794,46 @@ static int _bcm_phy_cable_test_get_status(struct phy_device *phydev,
>  	return ret;
>  }
>  
> +static int bcm_setup_forced(struct phy_device *phydev)
> +{
> +	u16 ctl = 0;
> +
> +	phydev->pause = 0;
> +	phydev->asym_pause = 0;
> +
> +	if (phydev->speed == SPEED_100)
> +		ctl |= LRECR_SPEED100;
> +
> +	if (phydev->duplex != DUPLEX_FULL)
> +		return -EOPNOTSUPP;

Is this even possible? I don't actually known, but you don't define a
HALF link mode, so how is this requested?

> +/**
> + * bcm_linkmode_adv_to_mii_adv_t
> + * @advertising: the linkmode advertisement settings
> + *
> + * A small helper function that translates linkmode advertisement
> + * settings to phy autonegotiation advertisements for the
> + * MII_BCM54XX_LREANAA register.
> + */
> +static inline u32 bcm_linkmode_adv_to_mii_adv_t(unsigned long *advertising)

No inline functions in .c files please, let the compiler decide.

> +int bcm_setup_master_slave(struct phy_device *phydev);
> +int bcm_config_aneg(struct phy_device *phydev, bool changed);
> +int bcm_config_advert(struct phy_device *phydev);

These are all BroadReach specific, so i would put something in there
name to indicate this. Otherwise somebody is going to try to use them
when not appropriate.

     Andrew

Re: [PATCH v3 3/3] net: phy: bcm-phy-lib: Implement BroadR-Reach link modes

[Christophe JAILLET]

Le 06/05/2024 à 16:40, Kamil Horák - 2N a écrit :
> 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.
> 
> Signed-off-by: Kamil Horák - 2N <kamilh@axis.com>
> ---
>   drivers/net/phy/bcm-phy-lib.c | 122 ++++++++++++
>   drivers/net/phy/bcm-phy-lib.h |   4 +
>   drivers/net/phy/broadcom.c    | 338 ++++++++++++++++++++++++++++++++--
>   3 files changed, 449 insertions(+), 15 deletions(-)

Hi,

a few nitpicks below, should it help.

...

> +int bcm_config_aneg(struct phy_device *phydev, bool changed)
> +{
> +	int err;
> +
> +	if (genphy_config_eee_advert(phydev))
> +		changed = true;
> +
> +	err = bcm_setup_master_slave(phydev);
> +	if (err < 0)
> +		return err;
> +	else if (err)
> +		changed = true;
> +
> +	if (phydev->autoneg != AUTONEG_ENABLE)
> +		return bcm_setup_forced(phydev);
> +
> +	err = bcm_config_advert(phydev);
> +	if (err < 0) /* error */

Nitpick: the comment could be removed, IMHO (otherwise maybe it should 
be added a few lines above too)

> +		return err;
> +	else if (err)
> +		changed = true;
> +
> +	return genphy_check_and_restart_aneg(phydev, changed);
> +}
> +EXPORT_SYMBOL_GPL(bcm_config_aneg);
> +
> +/**
> + * bcm_config_advert - sanitize and advertise auto-negotiation parameters
> + * @phydev: target phy_device struct
> + *
> + * Description: Writes MII_BCM54XX_LREANAA with the appropriate values,
> + *   after sanitizing the values to make sure we only advertise
> + *   what is supported.  Returns < 0 on error, 0 if the PHY's advertisement
> + *   hasn't changed, and > 0 if it has changed.
> + */
> +int bcm_config_advert(struct phy_device *phydev)
> +{
> +	int err;
> +	u32 adv;
> +
> +	/* Only allow advertising what this PHY supports */
> +	linkmode_and(phydev->advertising, phydev->advertising,
> +		     phydev->supported);
> +
> +	adv = bcm_linkmode_adv_to_mii_adv_t(phydev->advertising);
> +
> +	/* Setup BroadR-Reach mode advertisement */
> +	err = phy_modify_changed(phydev, MII_BCM54XX_LREANAA,
> +				 LRE_ADVERTISE_ALL | LREANAA_PAUSE |
> +				 LREANAA_PAUSE_ASYM, adv);
> +
> +	if (err < 0)
> +		return err;
> +
> +	return err > 0 ? 1 : 0;

Nitpick: Could be: return err;
(at this point it can be only 0 or 1 anyway)

> +}
> +EXPORT_SYMBOL_GPL(bcm_config_advert);

...

> @@ -576,18 +604,16 @@ static int bcm54811_config_init(struct phy_device *phydev)
>   			return err;
>   	}
>   
> -	return err;
> +	/* Configure BroadR-Reach function. */
> +	return  bcm5481x_set_brrmode(phydev, ETHTOOL_PHY_BRR_MODE_OFF);

Nitpick: 2 spaces before "bcm5481x_set_brrmode"

>   }
>   
> -static int bcm5481_config_aneg(struct phy_device *phydev)
> +static int bcm5481x_config_delay_swap(struct phy_device *phydev)
>   {
>   	struct device_node *np = phydev->mdio.dev.of_node;
> -	int ret;
> -
> -	/* Aneg firstly. */
> -	ret = genphy_config_aneg(phydev);
> +	int ret = 0;

I think that ret can be left un-initialized and use return 0; at the end 
of the function.

>   
> -	/* Then we can set up the delay. */
> +	/* Set up the delay. */
>   	bcm54xx_config_clock_delay(phydev);
>   
>   	if (of_property_read_bool(np, "enet-phy-lane-swap")) {
> @@ -601,6 +627,56 @@ static int bcm5481_config_aneg(struct phy_device *phydev)
>   	return ret;
>   }

...

> +static int bcm54811_config_aneg(struct phy_device *phydev)
> +{
> +	int ret;
> +	u8 brr_mode;
> +
> +	/* Aneg firstly. */
> +	ret = bcm5481x_get_brrmode(phydev, &brr_mode);
> +	if (ret)
> +		return ret;
> +
> +	if (brr_mode == ETHTOOL_PHY_BRR_MODE_ON) {
> +		/* BCM54811 is only capable of autonegotiation in IEEE mode */
> +		if (phydev->autoneg)
> +			return -EOPNOTSUPP;
> +
> +		ret = bcm_config_aneg(phydev, false);
> +

Nitpick: unneeded new line

> +	} else {
> +		ret = genphy_config_aneg(phydev);
> +	}
> +
> +	if (ret)
> +		return ret;
> +
> +	/* Then we can set up the delay and swap. */
> +	return bcm5481x_config_delay_swap(phydev);
> +}
> +
>   struct bcm54616s_phy_priv {
>   	bool mode_1000bx_en;
>   };
> @@ -1062,6 +1138,234 @@ static void bcm54xx_link_change_notify(struct phy_device *phydev)
>   	bcm_phy_write_exp(phydev, MII_BCM54XX_EXP_EXP08, ret);
>   }
>   
> +static int bcm54811_read_abilities(struct phy_device *phydev)
> +{
> +	int val, err;
> +	int i;
> +	static const int modes_array[] = {ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
> +					  ETHTOOL_LINK_MODE_1BR10_BIT,
> +					  ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> +					  ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
> +					  ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
> +					  ETHTOOL_LINK_MODE_100baseT_Full_BIT,
> +					  ETHTOOL_LINK_MODE_100baseT_Half_BIT,
> +					  ETHTOOL_LINK_MODE_10baseT_Full_BIT,
> +					  ETHTOOL_LINK_MODE_10baseT_Half_BIT};
> +

Nitpick: unneeded new line. Or maybe brr_mode could be defined above 
this struct?

Should there be an extra space after { and before }?
(see br_bits below)

> +	u8 brr_mode;
> +
> +	for (i = 0; i < ARRAY_SIZE(modes_array); i++)
> +		linkmode_clear_bit(modes_array[i], phydev->supported);
> +
> +	err = bcm5481x_get_brrmode(phydev, &brr_mode);
> +

Nitpick: unneeded new line

> +	if (err)
> +		return err;
> +
> +	if (brr_mode == ETHTOOL_PHY_BRR_MODE_ON) {
> +		linkmode_set_bit_array(phy_basic_ports_array,
> +				       ARRAY_SIZE(phy_basic_ports_array),
> +				       phydev->supported);
> +
> +		val = phy_read(phydev, MII_BCM54XX_LRESR);
> +		if (val < 0)
> +			return val;
> +
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> +				 phydev->supported, 1);
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
> +				 phydev->supported,
> +				 val & LRESR_100_1PAIR);
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_1BR10_BIT,
> +				 phydev->supported,
> +				 val & LRESR_10_1PAIR);
> +	} else {
> +		return genphy_read_abilities(phydev);
> +	}
> +
> +	return err;

Nitpick: return 0; ?

> +}

...

> +/* Read LDS Link Partner Ability in BroadR-Reach mode */
> +static int bcm_read_lpa(struct phy_device *phydev)
> +{
> +	int i, lrelpa;
> +
> +	if (phydev->autoneg != AUTONEG_ENABLE) {
> +		if (!phydev->autoneg_complete) {
> +			/* aneg not yet done, reset all relevant bits */
> +			static int br_bits[] = { ETHTOOL_LINK_MODE_Autoneg_BIT,

Nitpick: add const? (but maybe the line would get too long)

> +						 ETHTOOL_LINK_MODE_Pause_BIT,
> +						 ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> +						 ETHTOOL_LINK_MODE_1BR10_BIT,
> +						 ETHTOOL_LINK_MODE_100baseT1_Full_BIT };
> +			for (i = 0; i < ARRAY_SIZE(br_bits); i++)
> +				linkmode_clear_bit(br_bits[i], phydev->lp_advertising);
> +
> +			return 0;
> +		}
> +
> +		/* Long-Distance-Signalling Link Partner Ability */

Typo? Signaling?

> +		lrelpa = phy_read(phydev, MII_BCM54XX_LRELPA);
> +		if (lrelpa < 0)
> +			return lrelpa;
> +
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> +				 phydev->lp_advertising, lrelpa & LRELPA_PAUSE_ASYM);
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> +				 phydev->lp_advertising, lrelpa & LRELPA_PAUSE);
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
> +				 phydev->lp_advertising, lrelpa & LRELPA_100_1PAIR);
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_1BR10_BIT,
> +				 phydev->lp_advertising, lrelpa & LRELPA_10_1PAIR);
> +	} else {
> +		linkmode_zero(phydev->lp_advertising);
> +	}
> +
> +	return 0;
> +}

...

CJ

Re: [PATCH v3 3/3] net: phy: bcm-phy-lib: Implement BroadR-Reach link modes

[Simon Horman]

On Mon, May 06, 2024 at 04:40:15PM +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.
> 
> Signed-off-by: Kamil Horák - 2N <kamilh@axis.com>

Hi Kamil,

Some minor feedback from my side.

...

> diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c

...

> +/**
> + * bcm_linkmode_adv_to_mii_adv_t
> + * @advertising: the linkmode advertisement settings
> + *
> + * A small helper function that translates linkmode advertisement
> + * settings to phy autonegotiation advertisements for the
> + * MII_BCM54XX_LREANAA register.

Please consider including a Return: section in the Kernel doc.

Flagged by ./scripts/kernel-doc -Wall -none

> + */
> +static inline u32 bcm_linkmode_adv_to_mii_adv_t(unsigned long *advertising)

...

> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c

...

> +static int bcm_read_master_slave(struct phy_device *phydev)
> +{
> +	int cfg, state;
> +	int val;
> +
> +	/* In BroadR-Reach mode we are always capable of master-slave
> +	 *  and there is no preferred master or slave configuration
> +	 */
> +	phydev->master_slave_get = MASTER_SLAVE_CFG_UNKNOWN;
> +	phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
> +
> +	val = phy_read(phydev, MII_BCM54XX_LRECR);
> +	if (val < 0)
> +		return val;
> +
> +	if ((val & LRECR_LDSEN) == 0) {
> +		if (val & LRECR_MASTER)
> +			cfg = MASTER_SLAVE_CFG_MASTER_FORCE;
> +		else
> +			cfg = MASTER_SLAVE_CFG_SLAVE_FORCE;
> +	}
> +
> +	val = phy_read(phydev, MII_BCM54XX_LRELDSE);
> +	if (val < 0)
> +		return val;
> +
> +	if (val & LDSE_MASTER)
> +		state = MASTER_SLAVE_STATE_MASTER;
> +	else
> +		state = MASTER_SLAVE_STATE_SLAVE;
> +
> +	phydev->master_slave_get = cfg;

Perhaps it is not possible, but it appears that if the
((val & LRECR_LDSEN) == 0) condition above is not met then
cfg is used uninitialised here.

Flagged by Smatch.

> +	phydev->master_slave_state = state;
> +
> +	return 0;
> +}

...