| Message ID | 20240506144015.2409715-2-kamilh@axis.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: bcm5481x: add support for BroadR-Reach mode | expand |
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
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.
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
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
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
> > 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
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
> 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
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
> > 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
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
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;