When autonegotiation completes, the phy interface will be set based on
the global config register for that speed. If the SERDES mode is set to
something which the MAC does not support, then the link will not come
up. To avoid this, validate each combination of interface speed and link
speed which might be configured. This way, we ensure that we only
consider rate adaptation in our advertisement when we can actually use
it.
The API for get_rate_matching requires that PHY_INTERFACE_MODE_NA be
handled properly. To do this, we adopt a structure similar to
phylink_validate. At the top-level, we either validate a particular
interface speed or all of them. Below that, we validate each combination
of serdes speed and link speed.
For some firmwares, not all speeds are supported. In this case, the
global config register for that speed will be initialized to zero
(indicating that rate adaptation is not supported). We can detect this
by reading the PMA/PMD speed register to determine which speeds are
supported. This register is read once in probe and cached for later.
Fixes: 3c42563b3041 ("net: phy: aquantia: Add support for rate matching")
Signed-off-by: Sean Anderson <[email protected]>
---
This commit should not get backported until it soaks in master for a
while.
The names for the bits in MDIO_SPEED are pretty ugly. IMO
they should all use the MDIO_PMA_SPEED_ prefix, but I think since they
are in uapi we are stuck with these names...
Changes in v2:
- Rework to just validate things instead of modifying registers
drivers/net/phy/aquantia_main.c | 156 ++++++++++++++++++++++++++++++--
1 file changed, 150 insertions(+), 6 deletions(-)
diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index 47a76df36b74..998cdbb59ae3 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -109,6 +109,12 @@
#define VEND1_GLOBAL_CFG_RATE_ADAPT_NONE 0
#define VEND1_GLOBAL_CFG_RATE_ADAPT_USX 1
#define VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE 2
+#define VEND1_GLOBAL_CFG_SERDES_MODE GENMASK(2, 0)
+#define VEND1_GLOBAL_CFG_SERDES_MODE_XFI 0
+#define VEND1_GLOBAL_CFG_SERDES_MODE_SGMII 3
+#define VEND1_GLOBAL_CFG_SERDES_MODE_OCSGMII 4
+#define VEND1_GLOBAL_CFG_SERDES_MODE_XFI5G 6
+#define VEND1_GLOBAL_CFG_SERDES_MODE_XFI20G 7
#define VEND1_GLOBAL_RSVD_STAT1 0xc885
#define VEND1_GLOBAL_RSVD_STAT1_FW_BUILD_ID GENMASK(7, 4)
@@ -173,6 +179,7 @@ static const struct aqr107_hw_stat aqr107_hw_stats[] = {
struct aqr107_priv {
u64 sgmii_stats[AQR107_SGMII_STAT_SZ];
+ int supported_speeds;
};
static int aqr107_get_sset_count(struct phy_device *phydev)
@@ -675,13 +682,141 @@ static int aqr107_wait_processor_intensive_op(struct phy_device *phydev)
return 0;
}
+/**
+ * aqr107_rate_adapt_ok_one() - Validate rate adaptation for one configuration
+ * @phydev: The phy to act on
+ * @serdes_speed: The speed of the serdes (aka the phy interface)
+ * @link_speed: The speed of the link
+ *
+ * This function validates whether rate adaptation will work for a particular
+ * combination of @serdes_speed and @link_speed.
+ *
+ * Return: %true if the global config register for @link_speed is configured for
+ * rate adaptation, %true if @link_speed will not be advertised, %false
+ * otherwise.
+ */
+static bool aqr107_rate_adapt_ok_one(struct phy_device *phydev, int serdes_speed,
+ int link_speed)
+{
+ struct aqr107_priv *priv = phydev->priv;
+ int val, speed_bit;
+ u32 reg;
+
+ phydev_dbg(phydev, "validating link_speed=%d serdes_speed=%d\n",
+ link_speed, serdes_speed);
+
+ switch (link_speed) {
+ case SPEED_10000:
+ reg = VEND1_GLOBAL_CFG_10G;
+ speed_bit = MDIO_SPEED_10G;
+ break;
+ case SPEED_5000:
+ reg = VEND1_GLOBAL_CFG_5G;
+ speed_bit = MDIO_PCS_SPEED_5G;
+ break;
+ case SPEED_2500:
+ reg = VEND1_GLOBAL_CFG_2_5G;
+ speed_bit = MDIO_PCS_SPEED_2_5G;
+ break;
+ case SPEED_1000:
+ reg = VEND1_GLOBAL_CFG_1G;
+ speed_bit = MDIO_PMA_SPEED_1000;
+ break;
+ case SPEED_100:
+ reg = VEND1_GLOBAL_CFG_100M;
+ speed_bit = MDIO_PMA_SPEED_100;
+ break;
+ case SPEED_10:
+ reg = VEND1_GLOBAL_CFG_10M;
+ speed_bit = MDIO_PMA_SPEED_10;
+ break;
+ default:
+ WARN_ON_ONCE(1);
+ return false;
+ }
+
+ /* Vacuously OK, since we won't advertise it anyway */
+ if (!(priv->supported_speeds & speed_bit))
+ return true;
+
+ val = phy_read_mmd(phydev, MDIO_MMD_VEND1, reg);
+ if (val < 0) {
+ phydev_warn(phydev, "could not read register %x:%.04x (err = %d)\n",
+ MDIO_MMD_VEND1, reg, val);
+ return false;
+ }
+
+ phydev_dbg(phydev, "%x:%.04x = %.04x\n", MDIO_MMD_VEND1, reg, val);
+ if (FIELD_GET(VEND1_GLOBAL_CFG_RATE_ADAPT, val) !=
+ VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE)
+ return false;
+
+ switch (FIELD_GET(VEND1_GLOBAL_CFG_SERDES_MODE, val)) {
+ case VEND1_GLOBAL_CFG_SERDES_MODE_XFI20G:
+ return serdes_speed == SPEED_20000;
+ case VEND1_GLOBAL_CFG_SERDES_MODE_XFI:
+ return serdes_speed == SPEED_10000;
+ case VEND1_GLOBAL_CFG_SERDES_MODE_XFI5G:
+ return serdes_speed == SPEED_5000;
+ case VEND1_GLOBAL_CFG_SERDES_MODE_OCSGMII:
+ return serdes_speed == SPEED_2500;
+ case VEND1_GLOBAL_CFG_SERDES_MODE_SGMII:
+ return serdes_speed == SPEED_1000;
+ default:
+ return false;
+ }
+}
+
+/**
+ * aqr107_rate_adapt_ok() - Validate rate adaptation for an interface speed
+ * @phydev: The phy device
+ * @speed: The serdes (phy interface) speed
+ *
+ * This validates whether rate adaptation will work for a particular @speed.
+ * All link speeds less than or equal to @speed are validate to ensure they are
+ * configured properly.
+ *
+ * Return: %true if rate adaptation is supported for @speed, %false otherwise.
+ */
+static bool aqr107_rate_adapt_ok(struct phy_device *phydev, int speed)
+{
+ switch (speed) {
+ case SPEED_10000:
+ if (!aqr107_rate_adapt_ok_one(phydev, speed, SPEED_10000) ||
+ !aqr107_rate_adapt_ok_one(phydev, speed, SPEED_5000))
+ return false;
+ fallthrough;
+ case SPEED_2500:
+ if (!aqr107_rate_adapt_ok_one(phydev, speed, SPEED_2500))
+ return false;
+ fallthrough;
+ case SPEED_1000:
+ if (!aqr107_rate_adapt_ok_one(phydev, speed, SPEED_1000) ||
+ !aqr107_rate_adapt_ok_one(phydev, speed, SPEED_100) ||
+ !aqr107_rate_adapt_ok_one(phydev, speed, SPEED_10))
+ return false;
+ return true;
+ default:
+ return false;
+ };
+}
+
static int aqr107_get_rate_matching(struct phy_device *phydev,
phy_interface_t iface)
{
- if (iface == PHY_INTERFACE_MODE_10GBASER ||
- iface == PHY_INTERFACE_MODE_2500BASEX ||
- iface == PHY_INTERFACE_MODE_NA)
+ if (iface != PHY_INTERFACE_MODE_NA) {
+ if (aqr107_rate_adapt_ok(phydev,
+ phy_interface_max_speed(iface)))
+ return RATE_MATCH_PAUSE;
+ else
+ return RATE_MATCH_NONE;
+ }
+
+ if (aqr107_rate_adapt_ok(phydev, SPEED_10000) ||
+ aqr107_rate_adapt_ok(phydev, SPEED_2500) ||
+ aqr107_rate_adapt_ok(phydev, SPEED_1000))
return RATE_MATCH_PAUSE;
+
return RATE_MATCH_NONE;
}
@@ -711,10 +846,19 @@ static int aqr107_resume(struct phy_device *phydev)
static int aqr107_probe(struct phy_device *phydev)
{
- phydev->priv = devm_kzalloc(&phydev->mdio.dev,
- sizeof(struct aqr107_priv), GFP_KERNEL);
- if (!phydev->priv)
+ struct aqr107_priv *priv;
+
+ priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
return -ENOMEM;
+ phydev->priv = priv;
+
+ priv->supported_speeds = phy_read_mmd(phydev, MDIO_MMD_PMAPMD,
+ MDIO_SPEED);
+ if (priv->supported_speeds < 0) {
+ phydev_err(phydev, "could not determine supported speeds\n");
+ return priv->supported_speeds;
+ };
return aqr_hwmon_probe(phydev);
}
--
2.35.1.1320.gc452695387.dirty
On Mon, Nov 28, 2022 at 02:54:09PM -0500, Sean Anderson wrote:
> When autonegotiation completes, the phy interface will be set based on
> the global config register for that speed. If the SERDES mode is set to
> something which the MAC does not support, then the link will not come
> up. To avoid this, validate each combination of interface speed and link
> speed which might be configured. This way, we ensure that we only
> consider rate adaptation in our advertisement when we can actually use
> it.
>
> The API for get_rate_matching requires that PHY_INTERFACE_MODE_NA be
> handled properly. To do this, we adopt a structure similar to
> phylink_validate.
Note that this has all but gone away except for a few legacy cases with
the advent of the supported_interfaces bitmap.
Also note that phy_get_rate_matching() will not be called by phylink
with PHY_INTERFACE_MODE_NA since my recent commit (7642cc28fd37 "net:
phylink: fix PHY validation with rate adaption"), and phylink is
currently the only user of this interface.
> At the top-level, we either validate a particular
> interface speed or all of them. Below that, we validate each combination
> of serdes speed and link speed.
>
> For some firmwares, not all speeds are supported. In this case, the
> global config register for that speed will be initialized to zero
> (indicating that rate adaptation is not supported). We can detect this
> by reading the PMA/PMD speed register to determine which speeds are
> supported. This register is read once in probe and cached for later.
>
> Fixes: 3c42563b3041 ("net: phy: aquantia: Add support for rate matching")
> Signed-off-by: Sean Anderson <[email protected]>
> ---
> This commit should not get backported until it soaks in master for a
> while.
You will have to monitor the emails from stable to achieve that - as you
have a Fixes tag, that will trigger it to be picked up fairly quicky.
> #define VEND1_GLOBAL_CFG_RATE_ADAPT_NONE 0
> #define VEND1_GLOBAL_CFG_RATE_ADAPT_USX 1
> #define VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE 2
> +#define VEND1_GLOBAL_CFG_SERDES_MODE GENMASK(2, 0)
> +#define VEND1_GLOBAL_CFG_SERDES_MODE_XFI 0
> +#define VEND1_GLOBAL_CFG_SERDES_MODE_SGMII 3
> +#define VEND1_GLOBAL_CFG_SERDES_MODE_OCSGMII 4
> +#define VEND1_GLOBAL_CFG_SERDES_MODE_XFI5G 6
> +#define VEND1_GLOBAL_CFG_SERDES_MODE_XFI20G 7
>
> #define VEND1_GLOBAL_RSVD_STAT1 0xc885
> #define VEND1_GLOBAL_RSVD_STAT1_FW_BUILD_ID GENMASK(7, 4)
> @@ -173,6 +179,7 @@ static const struct aqr107_hw_stat aqr107_hw_stats[] = {
>
> struct aqr107_priv {
> u64 sgmii_stats[AQR107_SGMII_STAT_SZ];
> + int supported_speeds;
> };
>
> static int aqr107_get_sset_count(struct phy_device *phydev)
> @@ -675,13 +682,141 @@ static int aqr107_wait_processor_intensive_op(struct phy_device *phydev)
> return 0;
> }
>
> +/**
> + * aqr107_rate_adapt_ok_one() - Validate rate adaptation for one configuration
> + * @phydev: The phy to act on
> + * @serdes_speed: The speed of the serdes (aka the phy interface)
> + * @link_speed: The speed of the link
> + *
> + * This function validates whether rate adaptation will work for a particular
> + * combination of @serdes_speed and @link_speed.
> + *
> + * Return: %true if the global config register for @link_speed is configured for
> + * rate adaptation, %true if @link_speed will not be advertised, %false
> + * otherwise.
> + */
> +static bool aqr107_rate_adapt_ok_one(struct phy_device *phydev, int serdes_speed,
> + int link_speed)
> +{
> + struct aqr107_priv *priv = phydev->priv;
> + int val, speed_bit;
> + u32 reg;
> +
> + phydev_dbg(phydev, "validating link_speed=%d serdes_speed=%d\n",
> + link_speed, serdes_speed);
> +
> + switch (link_speed) {
> + case SPEED_10000:
> + reg = VEND1_GLOBAL_CFG_10G;
> + speed_bit = MDIO_SPEED_10G;
> + break;
> + case SPEED_5000:
> + reg = VEND1_GLOBAL_CFG_5G;
> + speed_bit = MDIO_PCS_SPEED_5G;
> + break;
> + case SPEED_2500:
> + reg = VEND1_GLOBAL_CFG_2_5G;
> + speed_bit = MDIO_PCS_SPEED_2_5G;
> + break;
> + case SPEED_1000:
> + reg = VEND1_GLOBAL_CFG_1G;
> + speed_bit = MDIO_PMA_SPEED_1000;
> + break;
> + case SPEED_100:
> + reg = VEND1_GLOBAL_CFG_100M;
> + speed_bit = MDIO_PMA_SPEED_100;
> + break;
> + case SPEED_10:
> + reg = VEND1_GLOBAL_CFG_10M;
> + speed_bit = MDIO_PMA_SPEED_10;
> + break;
> + default:
> + WARN_ON_ONCE(1);
> + return false;
> + }
> +
> + /* Vacuously OK, since we won't advertise it anyway */
> + if (!(priv->supported_speeds & speed_bit))
> + return true;
This doesn't make any sense. priv->supported_speeds is the set of speeds
read from the PMAPMD. The only bits that are valid for this are the
MDIO_PMA_SPEED_* definitions, but teh above switch makes use of the
MDIO_PCS_SPEED_* definitions. To see why this is wrong, look at these
two definitions:
#define MDIO_PMA_SPEED_10 0x0040 /* 10M capable */
#define MDIO_PCS_SPEED_2_5G 0x0040 /* 2.5G capable */
Note that they are the same value, yet above, you're testing for bit 6
being clear effectively for both 10M and 2.5G speeds. I suspect this
is *not* what you want.
MDIO_PMA_SPEED_* are only valid for the PMAPMD MMD (MMD 1).
MDIO_PCS_SPEED_* are only valid for the PCS MMD (MMD 3).
> +
> + val = phy_read_mmd(phydev, MDIO_MMD_VEND1, reg);
> + if (val < 0) {
> + phydev_warn(phydev, "could not read register %x:%.04x (err = %d)\n",
> + MDIO_MMD_VEND1, reg, val);
> + return false;
> + }
> +
> + phydev_dbg(phydev, "%x:%.04x = %.04x\n", MDIO_MMD_VEND1, reg, val);
> + if (FIELD_GET(VEND1_GLOBAL_CFG_RATE_ADAPT, val) !=
> + VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE)
> + return false;
> +
> + switch (FIELD_GET(VEND1_GLOBAL_CFG_SERDES_MODE, val)) {
> + case VEND1_GLOBAL_CFG_SERDES_MODE_XFI20G:
> + return serdes_speed == SPEED_20000;
> + case VEND1_GLOBAL_CFG_SERDES_MODE_XFI:
> + return serdes_speed == SPEED_10000;
> + case VEND1_GLOBAL_CFG_SERDES_MODE_XFI5G:
> + return serdes_speed == SPEED_5000;
> + case VEND1_GLOBAL_CFG_SERDES_MODE_OCSGMII:
> + return serdes_speed == SPEED_2500;
> + case VEND1_GLOBAL_CFG_SERDES_MODE_SGMII:
> + return serdes_speed == SPEED_1000;
> + default:
> + return false;
> + }
> +}
> +
> +/**
> + * aqr107_rate_adapt_ok() - Validate rate adaptation for an interface speed
> + * @phydev: The phy device
> + * @speed: The serdes (phy interface) speed
> + *
> + * This validates whether rate adaptation will work for a particular @speed.
> + * All link speeds less than or equal to @speed are validate to ensure they are
> + * configured properly.
> + *
> + * Return: %true if rate adaptation is supported for @speed, %false otherwise.
> + */
> +static bool aqr107_rate_adapt_ok(struct phy_device *phydev, int speed)
> +{
static int speeds[] = {
SPEED_10,
SPEED_100,
SPEED_1000,
SPEED_2500,
SPEED_5000,
SPEED_10000,
};
int i;
for (i = 0; i < ARRAY_SIZE(speeds) && speeds[i] <= speed; i++)
if (!aqr107_rate_adapt_ok_one(phydev, speed, speeds[i]))
return false;
/* speed must be in speeds[] */
if (i == ARRAY_SIZE(speeds) || speeds[i] != speed)
return false;
return true;
would be more concise code?
> +}
> +
> static int aqr107_get_rate_matching(struct phy_device *phydev,
> phy_interface_t iface)
> {
> - if (iface == PHY_INTERFACE_MODE_10GBASER ||
> - iface == PHY_INTERFACE_MODE_2500BASEX ||
> - iface == PHY_INTERFACE_MODE_NA)
> + if (iface != PHY_INTERFACE_MODE_NA) {
> + if (aqr107_rate_adapt_ok(phydev,
> + phy_interface_max_speed(iface)))
> + return RATE_MATCH_PAUSE;
> + else
> + return RATE_MATCH_NONE;
> + }
> +
> + if (aqr107_rate_adapt_ok(phydev, SPEED_10000) ||
> + aqr107_rate_adapt_ok(phydev, SPEED_2500) ||
> + aqr107_rate_adapt_ok(phydev, SPEED_1000))
> return RATE_MATCH_PAUSE;
> +
> return RATE_MATCH_NONE;
> }
>
> @@ -711,10 +846,19 @@ static int aqr107_resume(struct phy_device *phydev)
>
> static int aqr107_probe(struct phy_device *phydev)
> {
> - phydev->priv = devm_kzalloc(&phydev->mdio.dev,
> - sizeof(struct aqr107_priv), GFP_KERNEL);
> - if (!phydev->priv)
> + struct aqr107_priv *priv;
> +
> + priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> return -ENOMEM;
> + phydev->priv = priv;
> +
> + priv->supported_speeds = phy_read_mmd(phydev, MDIO_MMD_PMAPMD,
> + MDIO_SPEED);
> + if (priv->supported_speeds < 0) {
Given the above confusion about the MDIO_SPEED register, I'd suggest
this isn't simply named "supported_speeds" but "pmapmd_speeds" to
indicate that it's the pmapmd mmd speed register.
> + phydev_err(phydev, "could not determine supported speeds\n");
> + return priv->supported_speeds;
> + };
>
> return aqr_hwmon_probe(phydev);
> }
> --
> 2.35.1.1320.gc452695387.dirty
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On 11/28/22 18:22, Russell King (Oracle) wrote:
> On Mon, Nov 28, 2022 at 02:54:09PM -0500, Sean Anderson wrote:
>> When autonegotiation completes, the phy interface will be set based on
>> the global config register for that speed. If the SERDES mode is set to
>> something which the MAC does not support, then the link will not come
>> up. To avoid this, validate each combination of interface speed and link
>> speed which might be configured. This way, we ensure that we only
>> consider rate adaptation in our advertisement when we can actually use
>> it.
>>
>> The API for get_rate_matching requires that PHY_INTERFACE_MODE_NA be
>> handled properly. To do this, we adopt a structure similar to
>> phylink_validate.
>
> Note that this has all but gone away except for a few legacy cases with
> the advent of the supported_interfaces bitmap.
This is more of a note about inspiration than anything else.
> Also note that phy_get_rate_matching() will not be called by phylink
> with PHY_INTERFACE_MODE_NA since my recent commit (7642cc28fd37 "net:
> phylink: fix PHY validation with rate adaption"), and phylink is
> currently the only user of this interface.
Neat. Although I didn't get a chance to review that over the holiday...
I suppose I should rebase...
>> At the top-level, we either validate a particular
>> interface speed or all of them. Below that, we validate each combination
>> of serdes speed and link speed.
>>
>> For some firmwares, not all speeds are supported. In this case, the
>> global config register for that speed will be initialized to zero
>> (indicating that rate adaptation is not supported). We can detect this
>> by reading the PMA/PMD speed register to determine which speeds are
>> supported. This register is read once in probe and cached for later.
>>
>> Fixes: 3c42563b3041 ("net: phy: aquantia: Add support for rate matching")
>> Signed-off-by: Sean Anderson <[email protected]>
>> ---
>> This commit should not get backported until it soaks in master for a
>> while.
>
> You will have to monitor the emails from stable to achieve that - as you
> have a Fixes tag, that will trigger it to be picked up fairly quicky.
I know; this is a rather vain attempt :)
If I had not added the fixes tag, someone would have asked me to add it.
>> #define VEND1_GLOBAL_CFG_RATE_ADAPT_NONE 0
>> #define VEND1_GLOBAL_CFG_RATE_ADAPT_USX 1
>> #define VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE 2
>> +#define VEND1_GLOBAL_CFG_SERDES_MODE GENMASK(2, 0)
>> +#define VEND1_GLOBAL_CFG_SERDES_MODE_XFI 0
>> +#define VEND1_GLOBAL_CFG_SERDES_MODE_SGMII 3
>> +#define VEND1_GLOBAL_CFG_SERDES_MODE_OCSGMII 4
>> +#define VEND1_GLOBAL_CFG_SERDES_MODE_XFI5G 6
>> +#define VEND1_GLOBAL_CFG_SERDES_MODE_XFI20G 7
>>
>> #define VEND1_GLOBAL_RSVD_STAT1 0xc885
>> #define VEND1_GLOBAL_RSVD_STAT1_FW_BUILD_ID GENMASK(7, 4)
>> @@ -173,6 +179,7 @@ static const struct aqr107_hw_stat aqr107_hw_stats[] = {
>>
>> struct aqr107_priv {
>> u64 sgmii_stats[AQR107_SGMII_STAT_SZ];
>> + int supported_speeds;
>> };
>>
>> static int aqr107_get_sset_count(struct phy_device *phydev)
>> @@ -675,13 +682,141 @@ static int aqr107_wait_processor_intensive_op(struct phy_device *phydev)
>> return 0;
>> }
>>
>> +/**
>> + * aqr107_rate_adapt_ok_one() - Validate rate adaptation for one configuration
>> + * @phydev: The phy to act on
>> + * @serdes_speed: The speed of the serdes (aka the phy interface)
>> + * @link_speed: The speed of the link
>> + *
>> + * This function validates whether rate adaptation will work for a particular
>> + * combination of @serdes_speed and @link_speed.
>> + *
>> + * Return: %true if the global config register for @link_speed is configured for
>> + * rate adaptation, %true if @link_speed will not be advertised, %false
>> + * otherwise.
>> + */
>> +static bool aqr107_rate_adapt_ok_one(struct phy_device *phydev, int serdes_speed,
>> + int link_speed)
>> +{
>> + struct aqr107_priv *priv = phydev->priv;
>> + int val, speed_bit;
>> + u32 reg;
>> +
>> + phydev_dbg(phydev, "validating link_speed=%d serdes_speed=%d\n",
>> + link_speed, serdes_speed);
>> +
>> + switch (link_speed) {
>> + case SPEED_10000:
>> + reg = VEND1_GLOBAL_CFG_10G;
>> + speed_bit = MDIO_SPEED_10G;
>> + break;
>> + case SPEED_5000:
>> + reg = VEND1_GLOBAL_CFG_5G;
>> + speed_bit = MDIO_PCS_SPEED_5G;
>> + break;
>> + case SPEED_2500:
>> + reg = VEND1_GLOBAL_CFG_2_5G;
>> + speed_bit = MDIO_PCS_SPEED_2_5G;
>> + break;
>> + case SPEED_1000:
>> + reg = VEND1_GLOBAL_CFG_1G;
>> + speed_bit = MDIO_PMA_SPEED_1000;
>> + break;
>> + case SPEED_100:
>> + reg = VEND1_GLOBAL_CFG_100M;
>> + speed_bit = MDIO_PMA_SPEED_100;
>> + break;
>> + case SPEED_10:
>> + reg = VEND1_GLOBAL_CFG_10M;
>> + speed_bit = MDIO_PMA_SPEED_10;
>> + break;
>> + default:
>> + WARN_ON_ONCE(1);
>> + return false;
>> + }
>> +
>> + /* Vacuously OK, since we won't advertise it anyway */
>> + if (!(priv->supported_speeds & speed_bit))
>> + return true;
>
> This doesn't make any sense. priv->supported_speeds is the set of speeds
> read from the PMAPMD. The only bits that are valid for this are the
> MDIO_PMA_SPEED_* definitions, but teh above switch makes use of the
> MDIO_PCS_SPEED_* definitions. To see why this is wrong, look at these
> two definitions:
>
> #define MDIO_PMA_SPEED_10 0x0040 /* 10M capable */
> #define MDIO_PCS_SPEED_2_5G 0x0040 /* 2.5G capable */
>
> Note that they are the same value, yet above, you're testing for bit 6
> being clear effectively for both 10M and 2.5G speeds. I suspect this
> is *not* what you want.
>
> MDIO_PMA_SPEED_* are only valid for the PMAPMD MMD (MMD 1).
> MDIO_PCS_SPEED_* are only valid for the PCS MMD (MMD 3).
Ugh. I almost noticed this from the register naming...
Part of the problem is that all the defines are right next to each other
with no indication of what you just described.
Anyway, what I want are the PCS/PMA speeds from the 2018 revision, which
this phy seems to follow.
>> +
>> + val = phy_read_mmd(phydev, MDIO_MMD_VEND1, reg);
>> + if (val < 0) {
>> + phydev_warn(phydev, "could not read register %x:%.04x (err = %d)\n",
>> + MDIO_MMD_VEND1, reg, val);
>> + return false;
>> + }
>> +
>> + phydev_dbg(phydev, "%x:%.04x = %.04x\n", MDIO_MMD_VEND1, reg, val);
>> + if (FIELD_GET(VEND1_GLOBAL_CFG_RATE_ADAPT, val) !=
>> + VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE)
>> + return false;
>> +
>> + switch (FIELD_GET(VEND1_GLOBAL_CFG_SERDES_MODE, val)) {
>> + case VEND1_GLOBAL_CFG_SERDES_MODE_XFI20G:
>> + return serdes_speed == SPEED_20000;
>> + case VEND1_GLOBAL_CFG_SERDES_MODE_XFI:
>> + return serdes_speed == SPEED_10000;
>> + case VEND1_GLOBAL_CFG_SERDES_MODE_XFI5G:
>> + return serdes_speed == SPEED_5000;
>> + case VEND1_GLOBAL_CFG_SERDES_MODE_OCSGMII:
>> + return serdes_speed == SPEED_2500;
>> + case VEND1_GLOBAL_CFG_SERDES_MODE_SGMII:
>> + return serdes_speed == SPEED_1000;
>> + default:
>> + return false;
>> + }
>> +}
>> +
>> +/**
>> + * aqr107_rate_adapt_ok() - Validate rate adaptation for an interface speed
>> + * @phydev: The phy device
>> + * @speed: The serdes (phy interface) speed
>> + *
>> + * This validates whether rate adaptation will work for a particular @speed.
>> + * All link speeds less than or equal to @speed are validate to ensure they are
>> + * configured properly.
>> + *
>> + * Return: %true if rate adaptation is supported for @speed, %false otherwise.
>> + */
>> +static bool aqr107_rate_adapt_ok(struct phy_device *phydev, int speed)
>> +{
> static int speeds[] = {
> SPEED_10,
> SPEED_100,
> SPEED_1000,
> SPEED_2500,
> SPEED_5000,
> SPEED_10000,
> };
> int i;
>
> for (i = 0; i < ARRAY_SIZE(speeds) && speeds[i] <= speed; i++)
> if (!aqr107_rate_adapt_ok_one(phydev, speed, speeds[i]))
> return false;
>
> /* speed must be in speeds[] */
> if (i == ARRAY_SIZE(speeds) || speeds[i] != speed)
> return false;
>
> return true;
>
> would be more concise code?
Seems reasonable. I could also stick the link_speed switch from above into this array...
>> +}
>> +
>> static int aqr107_get_rate_matching(struct phy_device *phydev,
>> phy_interface_t iface)
>> {
>> - if (iface == PHY_INTERFACE_MODE_10GBASER ||
>> - iface == PHY_INTERFACE_MODE_2500BASEX ||
>> - iface == PHY_INTERFACE_MODE_NA)
>> + if (iface != PHY_INTERFACE_MODE_NA) {
>> + if (aqr107_rate_adapt_ok(phydev,
>> + phy_interface_max_speed(iface)))
>> + return RATE_MATCH_PAUSE;
>> + else
>> + return RATE_MATCH_NONE;
>> + }
>> +
>> + if (aqr107_rate_adapt_ok(phydev, SPEED_10000) ||
>> + aqr107_rate_adapt_ok(phydev, SPEED_2500) ||
>> + aqr107_rate_adapt_ok(phydev, SPEED_1000))
>> return RATE_MATCH_PAUSE;
>> +
>> return RATE_MATCH_NONE;
>> }
>>
>> @@ -711,10 +846,19 @@ static int aqr107_resume(struct phy_device *phydev)
>>
>> static int aqr107_probe(struct phy_device *phydev)
>> {
>> - phydev->priv = devm_kzalloc(&phydev->mdio.dev,
>> - sizeof(struct aqr107_priv), GFP_KERNEL);
>> - if (!phydev->priv)
>> + struct aqr107_priv *priv;
>> +
>> + priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> return -ENOMEM;
>> + phydev->priv = priv;
>> +
>> + priv->supported_speeds = phy_read_mmd(phydev, MDIO_MMD_PMAPMD,
>> + MDIO_SPEED);
>> + if (priv->supported_speeds < 0) {
>
> Given the above confusion about the MDIO_SPEED register, I'd suggest
> this isn't simply named "supported_speeds" but "pmapmd_speeds" to
> indicate that it's the pmapmd mmd speed register.
Sure.
--Sean
>> + phydev_err(phydev, "could not determine supported speeds\n");
>> + return priv->supported_speeds;
>> + };
>>
>> return aqr_hwmon_probe(phydev);
>> }
>> --
>> 2.35.1.1320.gc452695387.dirty
>>
>>
>
On Mon, Nov 28, 2022 at 07:21:56PM -0500, Sean Anderson wrote:
> On 11/28/22 18:22, Russell King (Oracle) wrote:
> > This doesn't make any sense. priv->supported_speeds is the set of speeds
> > read from the PMAPMD. The only bits that are valid for this are the
> > MDIO_PMA_SPEED_* definitions, but teh above switch makes use of the
> > MDIO_PCS_SPEED_* definitions. To see why this is wrong, look at these
> > two definitions:
> >
> > #define MDIO_PMA_SPEED_10 0x0040 /* 10M capable */
> > #define MDIO_PCS_SPEED_2_5G 0x0040 /* 2.5G capable */
> >
> > Note that they are the same value, yet above, you're testing for bit 6
> > being clear effectively for both 10M and 2.5G speeds. I suspect this
> > is *not* what you want.
> >
> > MDIO_PMA_SPEED_* are only valid for the PMAPMD MMD (MMD 1).
> > MDIO_PCS_SPEED_* are only valid for the PCS MMD (MMD 3).
>
> Ugh. I almost noticed this from the register naming...
>
> Part of the problem is that all the defines are right next to each other
> with no indication of what you just described.
That's because they all refer to the speed register which is at the same
address, but for some reason the 802.3 committees decided to make the
register bits mean different things depending on the MMD. That's why the
definition states the MMD name in it.
This is true of all definitions in mdio.h - the naming convention is of
the format "MDIO_mmd_register_bit" where the bit is specific to a MMD,
or "MDIO_register_bit" where it is non-specific (e.g. the status
register 1 link status bit.)
> Anyway, what I want are the PCS/PMA speeds from the 2018 revision, which
> this phy seems to follow.
I think we should add further entries for the PMA/PMD speed register.
For example, 2.5G is bit 13 and 5G is bit 14. (vs bits 6 and 7 for the
PCS MMD version of the speed register.)
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Tue, Nov 29, 2022 at 02:39:59PM +0100, Andrew Lunn wrote:
> > >> This commit should not get backported until it soaks in master for a
> > >> while.
> > >
> > > You will have to monitor the emails from stable to achieve that - as you
> > > have a Fixes tag, that will trigger it to be picked up fairly quicky.
> >
> > I know; this is a rather vain attempt :)
> >
> > If I had not added the fixes tag, someone would have asked me to add it.
>
> Hi Sean
>
> If you had put a comment under the --- that you deliberately did not
> add a Fixes tag because you wanted it to soak for a while, you
> probably would not be asked.
>
> I think the bot also looks at the subject to decide if it is a fix. So
> you need to word the subject so it sounds like continuing development,
> not a fix.
Sasha makes use of Google's AI. I believe it looks at the entire patch
and commit message, and can make some really strange decisions.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> >> This commit should not get backported until it soaks in master for a
> >> while.
> >
> > You will have to monitor the emails from stable to achieve that - as you
> > have a Fixes tag, that will trigger it to be picked up fairly quicky.
>
> I know; this is a rather vain attempt :)
>
> If I had not added the fixes tag, someone would have asked me to add it.
Hi Sean
If you had put a comment under the --- that you deliberately did not
add a Fixes tag because you wanted it to soak for a while, you
probably would not be asked.
I think the bot also looks at the subject to decide if it is a fix. So
you need to word the subject so it sounds like continuing development,
not a fix.
Andrew
On 11/28/22 19:42, Russell King (Oracle) wrote:
> On Mon, Nov 28, 2022 at 07:21:56PM -0500, Sean Anderson wrote:
>> On 11/28/22 18:22, Russell King (Oracle) wrote:
>> > This doesn't make any sense. priv->supported_speeds is the set of speeds
>> > read from the PMAPMD. The only bits that are valid for this are the
>> > MDIO_PMA_SPEED_* definitions, but teh above switch makes use of the
>> > MDIO_PCS_SPEED_* definitions. To see why this is wrong, look at these
>> > two definitions:
>> >
>> > #define MDIO_PMA_SPEED_10 0x0040 /* 10M capable */
>> > #define MDIO_PCS_SPEED_2_5G 0x0040 /* 2.5G capable */
>> >
>> > Note that they are the same value, yet above, you're testing for bit 6
>> > being clear effectively for both 10M and 2.5G speeds. I suspect this
>> > is *not* what you want.
>> >
>> > MDIO_PMA_SPEED_* are only valid for the PMAPMD MMD (MMD 1).
>> > MDIO_PCS_SPEED_* are only valid for the PCS MMD (MMD 3).
>>
>> Ugh. I almost noticed this from the register naming...
>>
>> Part of the problem is that all the defines are right next to each other
>> with no indication of what you just described.
>
> That's because they all refer to the speed register which is at the same
> address, but for some reason the 802.3 committees decided to make the
> register bits mean different things depending on the MMD. That's why the
> definition states the MMD name in it.
Well, then it's really a different register per MMD (and therefore the
definitions should be better separated). Grouping them together implies
that they share bits, when they do not (except for the 10G bit).
> This is true of all definitions in mdio.h - the naming convention is of
> the format "MDIO_mmd_register_bit" where the bit is specific to a MMD,
> or "MDIO_register_bit" where it is non-specific (e.g. the status
> register 1 link status bit.)
>
>> Anyway, what I want are the PCS/PMA speeds from the 2018 revision, which
>> this phy seems to follow.
>
> I think we should add further entries for the PMA/PMD speed register.
> For example, 2.5G is bit 13 and 5G is bit 14. (vs bits 6 and 7 for the
> PCS MMD version of the speed register.)
>
Yes. I will do this for v3.
--Sean
On Tue, Nov 29, 2022 at 10:56:56AM -0500, Sean Anderson wrote:
> On 11/28/22 19:42, Russell King (Oracle) wrote:
> > On Mon, Nov 28, 2022 at 07:21:56PM -0500, Sean Anderson wrote:
> >> On 11/28/22 18:22, Russell King (Oracle) wrote:
> >> > This doesn't make any sense. priv->supported_speeds is the set of speeds
> >> > read from the PMAPMD. The only bits that are valid for this are the
> >> > MDIO_PMA_SPEED_* definitions, but teh above switch makes use of the
> >> > MDIO_PCS_SPEED_* definitions. To see why this is wrong, look at these
> >> > two definitions:
> >> >
> >> > #define MDIO_PMA_SPEED_10 0x0040 /* 10M capable */
> >> > #define MDIO_PCS_SPEED_2_5G 0x0040 /* 2.5G capable */
> >> >
> >> > Note that they are the same value, yet above, you're testing for bit 6
> >> > being clear effectively for both 10M and 2.5G speeds. I suspect this
> >> > is *not* what you want.
> >> >
> >> > MDIO_PMA_SPEED_* are only valid for the PMAPMD MMD (MMD 1).
> >> > MDIO_PCS_SPEED_* are only valid for the PCS MMD (MMD 3).
> >>
> >> Ugh. I almost noticed this from the register naming...
> >>
> >> Part of the problem is that all the defines are right next to each other
> >> with no indication of what you just described.
> >
> > That's because they all refer to the speed register which is at the same
> > address, but for some reason the 802.3 committees decided to make the
> > register bits mean different things depending on the MMD. That's why the
> > definition states the MMD name in it.
>
> Well, then it's really a different register per MMD (and therefore the
> definitions should be better separated). Grouping them together implies
> that they share bits, when they do not (except for the 10G bit).
What about bits that are shared amongst the different registers.
Should we have multiple definitions for the link status bit in _all_
the different MMDs, despite it being the same across all status 1
registers?
Clause 45 is quite a trainwreck when it comes to these register
definitions.
As I've stated, there is a pattern to the naming. Understand it,
and it isn't confusing.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On 11/29/22 11:17, Russell King (Oracle) wrote:
> On Tue, Nov 29, 2022 at 10:56:56AM -0500, Sean Anderson wrote:
>> On 11/28/22 19:42, Russell King (Oracle) wrote:
>> > On Mon, Nov 28, 2022 at 07:21:56PM -0500, Sean Anderson wrote:
>> >> On 11/28/22 18:22, Russell King (Oracle) wrote:
>> >> > This doesn't make any sense. priv->supported_speeds is the set of speeds
>> >> > read from the PMAPMD. The only bits that are valid for this are the
>> >> > MDIO_PMA_SPEED_* definitions, but teh above switch makes use of the
>> >> > MDIO_PCS_SPEED_* definitions. To see why this is wrong, look at these
>> >> > two definitions:
>> >> >
>> >> > #define MDIO_PMA_SPEED_10 0x0040 /* 10M capable */
>> >> > #define MDIO_PCS_SPEED_2_5G 0x0040 /* 2.5G capable */
>> >> >
>> >> > Note that they are the same value, yet above, you're testing for bit 6
>> >> > being clear effectively for both 10M and 2.5G speeds. I suspect this
>> >> > is *not* what you want.
>> >> >
>> >> > MDIO_PMA_SPEED_* are only valid for the PMAPMD MMD (MMD 1).
>> >> > MDIO_PCS_SPEED_* are only valid for the PCS MMD (MMD 3).
>> >>
>> >> Ugh. I almost noticed this from the register naming...
>> >>
>> >> Part of the problem is that all the defines are right next to each other
>> >> with no indication of what you just described.
>> >
>> > That's because they all refer to the speed register which is at the same
>> > address, but for some reason the 802.3 committees decided to make the
>> > register bits mean different things depending on the MMD. That's why the
>> > definition states the MMD name in it.
>>
>> Well, then it's really a different register per MMD (and therefore the
>> definitions should be better separated). Grouping them together implies
>> that they share bits, when they do not (except for the 10G bit).
>
> What about bits that are shared amongst the different registers.
> Should we have multiple definitions for the link status bit in _all_
> the different MMDs, despite it being the same across all status 1
> registers?
No, but for registers which are 95% difference we should at least separate
them and add a comment.
> Clause 45 is quite a trainwreck when it comes to these register
> definitions.
Maybe they should have randomized the bit orders in the first place to discourage this sort of thing :)
> As I've stated, there is a pattern to the naming. Understand it,
> and it isn't confusing.
>
I don't have a problem with the naming, just the organization of the
source file.
--Sean
--Sean
On 11/29/22 11:46, Russell King (Oracle) wrote:
> On Tue, Nov 29, 2022 at 11:29:39AM -0500, Sean Anderson wrote:
>> On 11/29/22 11:17, Russell King (Oracle) wrote:
>> > On Tue, Nov 29, 2022 at 10:56:56AM -0500, Sean Anderson wrote:
>> >> On 11/28/22 19:42, Russell King (Oracle) wrote:
>> >> > On Mon, Nov 28, 2022 at 07:21:56PM -0500, Sean Anderson wrote:
>> >> >> On 11/28/22 18:22, Russell King (Oracle) wrote:
>> >> >> > This doesn't make any sense. priv->supported_speeds is the set of speeds
>> >> >> > read from the PMAPMD. The only bits that are valid for this are the
>> >> >> > MDIO_PMA_SPEED_* definitions, but teh above switch makes use of the
>> >> >> > MDIO_PCS_SPEED_* definitions. To see why this is wrong, look at these
>> >> >> > two definitions:
>> >> >> >
>> >> >> > #define MDIO_PMA_SPEED_10 0x0040 /* 10M capable */
>> >> >> > #define MDIO_PCS_SPEED_2_5G 0x0040 /* 2.5G capable */
>> >> >> >
>> >> >> > Note that they are the same value, yet above, you're testing for bit 6
>> >> >> > being clear effectively for both 10M and 2.5G speeds. I suspect this
>> >> >> > is *not* what you want.
>> >> >> >
>> >> >> > MDIO_PMA_SPEED_* are only valid for the PMAPMD MMD (MMD 1).
>> >> >> > MDIO_PCS_SPEED_* are only valid for the PCS MMD (MMD 3).
>> >> >>
>> >> >> Ugh. I almost noticed this from the register naming...
>> >> >>
>> >> >> Part of the problem is that all the defines are right next to each other
>> >> >> with no indication of what you just described.
>> >> >
>> >> > That's because they all refer to the speed register which is at the same
>> >> > address, but for some reason the 802.3 committees decided to make the
>> >> > register bits mean different things depending on the MMD. That's why the
>> >> > definition states the MMD name in it.
>> >>
>> >> Well, then it's really a different register per MMD (and therefore the
>> >> definitions should be better separated). Grouping them together implies
>> >> that they share bits, when they do not (except for the 10G bit).
>> >
>> > What about bits that are shared amongst the different registers.
>> > Should we have multiple definitions for the link status bit in _all_
>> > the different MMDs, despite it being the same across all status 1
>> > registers?
>>
>> No, but for registers which are 95% difference we should at least separate
>> them and add a comment.
>>
>> > Clause 45 is quite a trainwreck when it comes to these register
>> > definitions.
>>
>> Maybe they should have randomized the bit orders in the first place to discourage this sort of thing :)
>>
>> > As I've stated, there is a pattern to the naming. Understand it,
>> > and it isn't confusing.
>> >
>>
>> I don't have a problem with the naming, just the organization of the
>> source file.
>
> The organisation is sane. There are some shared bits in the SPEED
> register between different MMDs.
>
> If we separate the PMA and PCS with a blink line, do we then seperate
> the register groups with two blank lines? No, people will complain
> about that (they already do if you think about doing that in source
> files.)
>
> Sorry, but... one has to pay attention to the whole of the macro name,
> not just the last few characters... and think "is something that
> contains "_PCS_" in its name really suitable for use with a PMA/PMD
> MMD register when there's a PCS MMD? Now let me think... umm. no.
>
Well, what I had in mind was
diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
index 75b7257a51e1..d700e9e886b9 100644
--- a/include/uapi/linux/mdio.h
+++ b/include/uapi/linux/mdio.h
@@ -127,16 +127,36 @@
#define MDIO_AN_STAT1_PAGE 0x0040 /* Page received */
#define MDIO_AN_STAT1_XNP 0x0080 /* Extended next page status */
-/* Speed register. */
+/* Speed register common. */
#define MDIO_SPEED_10G 0x0001 /* 10G capable */
+
+/* PMA/PMD Speed register. */
+#define MDIO_PMA_SPEED_10G MDIO_SPEED_10G
#define MDIO_PMA_SPEED_2B 0x0002 /* 2BASE-TL capable */
#define MDIO_PMA_SPEED_10P 0x0004 /* 10PASS-TS capable */
#define MDIO_PMA_SPEED_1000 0x0010 /* 1000M capable */
#define MDIO_PMA_SPEED_100 0x0020 /* 100M capable */
#define MDIO_PMA_SPEED_10 0x0040 /* 10M capable */
+#define MDIO_PMA_SPEED_10G1G 0x0080 /* 10/1G capable */
+#define MDIO_PMA_SPEED_40G 0x0100 /* 40G capable */
+#define MDIO_PMA_SPEED_100G 0x0200 /* 100G capable */
+#define MDIO_PMA_SPEED_10GP 0x0400 /* 10GPASS-XR capable */
+#define MDIO_PMA_SPEED_25G 0x0800 /* 25G capable */
+#define MDIO_PMA_SPEED_200G 0x1000 /* 200G capable */
+#define MDIO_PMA_SPEED_2_5G 0x2000 /* 2.5G capable */
+#define MDIO_PMA_SPEED_5G 0x4000 /* 5G capable */
+#define MDIO_PMA_SPEED_400G 0x8000 /* 400G capable */
+
+/* PCS et al. Speed register. */
+#define MDIO_PCS_SPEED_10G MDIO_SPEED_10G
#define MDIO_PCS_SPEED_10P2B 0x0002 /* 10PASS-TS/2BASE-TL capable */
+#define MDIO_PCS_SPEED_40G 0x0004 /* 450G capable */
+#define MDIO_PCS_SPEED_100G 0x0008 /* 100G capable */
+#define MDIO_PCS_SPEED_25G 0x0010 /* 25G capable */
#define MDIO_PCS_SPEED_2_5G 0x0040 /* 2.5G capable */
#define MDIO_PCS_SPEED_5G 0x0080 /* 5G capable */
+#define MDIO_PCS_SPEED_200G 0x0100 /* 200G capable */
+#define MDIO_PCS_SPEED_400G 0x0200 /* 400G capable */
/* Device present registers. */
#define MDIO_DEVS_PRESENT(devad) (1 << (devad))
Really, these registers have almost nothing in common except their concept
and sub-address.
On another note: is BIT() allowed in uapi headers?
--Sean
On Tue, Nov 29, 2022 at 11:29:39AM -0500, Sean Anderson wrote:
> On 11/29/22 11:17, Russell King (Oracle) wrote:
> > On Tue, Nov 29, 2022 at 10:56:56AM -0500, Sean Anderson wrote:
> >> On 11/28/22 19:42, Russell King (Oracle) wrote:
> >> > On Mon, Nov 28, 2022 at 07:21:56PM -0500, Sean Anderson wrote:
> >> >> On 11/28/22 18:22, Russell King (Oracle) wrote:
> >> >> > This doesn't make any sense. priv->supported_speeds is the set of speeds
> >> >> > read from the PMAPMD. The only bits that are valid for this are the
> >> >> > MDIO_PMA_SPEED_* definitions, but teh above switch makes use of the
> >> >> > MDIO_PCS_SPEED_* definitions. To see why this is wrong, look at these
> >> >> > two definitions:
> >> >> >
> >> >> > #define MDIO_PMA_SPEED_10 0x0040 /* 10M capable */
> >> >> > #define MDIO_PCS_SPEED_2_5G 0x0040 /* 2.5G capable */
> >> >> >
> >> >> > Note that they are the same value, yet above, you're testing for bit 6
> >> >> > being clear effectively for both 10M and 2.5G speeds. I suspect this
> >> >> > is *not* what you want.
> >> >> >
> >> >> > MDIO_PMA_SPEED_* are only valid for the PMAPMD MMD (MMD 1).
> >> >> > MDIO_PCS_SPEED_* are only valid for the PCS MMD (MMD 3).
> >> >>
> >> >> Ugh. I almost noticed this from the register naming...
> >> >>
> >> >> Part of the problem is that all the defines are right next to each other
> >> >> with no indication of what you just described.
> >> >
> >> > That's because they all refer to the speed register which is at the same
> >> > address, but for some reason the 802.3 committees decided to make the
> >> > register bits mean different things depending on the MMD. That's why the
> >> > definition states the MMD name in it.
> >>
> >> Well, then it's really a different register per MMD (and therefore the
> >> definitions should be better separated). Grouping them together implies
> >> that they share bits, when they do not (except for the 10G bit).
> >
> > What about bits that are shared amongst the different registers.
> > Should we have multiple definitions for the link status bit in _all_
> > the different MMDs, despite it being the same across all status 1
> > registers?
>
> No, but for registers which are 95% difference we should at least separate
> them and add a comment.
>
> > Clause 45 is quite a trainwreck when it comes to these register
> > definitions.
>
> Maybe they should have randomized the bit orders in the first place to discourage this sort of thing :)
>
> > As I've stated, there is a pattern to the naming. Understand it,
> > and it isn't confusing.
> >
>
> I don't have a problem with the naming, just the organization of the
> source file.
The organisation is sane. There are some shared bits in the SPEED
register between different MMDs.
If we separate the PMA and PCS with a blink line, do we then seperate
the register groups with two blank lines? No, people will complain
about that (they already do if you think about doing that in source
files.)
Sorry, but... one has to pay attention to the whole of the macro name,
not just the last few characters... and think "is something that
contains "_PCS_" in its name really suitable for use with a PMA/PMD
MMD register when there's a PCS MMD? Now let me think... umm. no.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!