Remove the use of phy_set_max_speed in phy driver as the
function is mainly used in MAC driver to set the max
speed.
Instead use get_features to fix up Phy PMA capabilities for
AQR111, AQR111B0, AQR114C and AQCS109
Fixes: 038ba1dc4e54 ("net: phy: aquantia: add AQR111 and AQR111B0 PHY ID")
Fixes: 0974f1f03b07 ("net: phy: aquantia: remove false 5G and 10G speed ability for AQCS109")
Fixes: c278ec644377 ("net: phy: aquantia: add support for AQR114C PHY ID")
Link: https://lore.kernel.org/all/20240913011635.1286027-1-quic_abchauha@quicinc.com/T/
Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>
---
Changes since v3
1. remove setting of 2500baseX bit introduced as
part of previous patches.
2. follow reverse xmas declaration of variables.
3. remove local mask introduced as part of
previous patch and optimize the logic.
Changes since v2
1. seperate out the changes into two different patches.
2. use phy_gbit_features instead of initializing each and
every link mode bits.
3. write seperate functions for 2.5Gbps supported phy.
4. remove FIBRE bit which was introduced in patch 1.
Changes since v1
1. remove usage of phy_set_max_speed in the aquantia driver code.
2. Introduce aqr_custom_get_feature which checks for the phy id and
takes necessary actions based on max_speed supported by the phy.
3. remove aqr111_config_init as it is just a wrapper function.
drivers/net/phy/aquantia/aquantia_main.c | 51 +++++++++++++++---------
1 file changed, 32 insertions(+), 19 deletions(-)
diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
index af6784b118d2..73f4e67e14b6 100644
--- a/drivers/net/phy/aquantia/aquantia_main.c
+++ b/drivers/net/phy/aquantia/aquantia_main.c
@@ -527,12 +527,6 @@ static int aqcs109_config_init(struct phy_device *phydev)
if (!ret)
aqr107_chip_info(phydev);
- /* AQCS109 belongs to a chip family partially supporting 10G and 5G.
- * PMA speed ability bits are the same for all members of the family,
- * AQCS109 however supports speeds up to 2.5G only.
- */
- phy_set_max_speed(phydev, SPEED_2500);
-
return aqr107_set_downshift(phydev, MDIO_AN_VEND_PROV_DOWNSHIFT_DFLT);
}
@@ -741,6 +735,31 @@ static int aqr113c_config_init(struct phy_device *phydev)
return aqr113c_fill_interface_modes(phydev);
}
+static int aqr111_get_features(struct phy_device *phydev)
+{
+ unsigned long *supported = phydev->supported;
+ int ret;
+
+ /* Normal feature discovery */
+ ret = genphy_c45_pma_read_abilities(phydev);
+ if (ret)
+ return ret;
+
+ /* PHY FIXUP */
+ /* Although the PHY sets bit 12.18.19, it does not support 10G modes */
+ linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
+ linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT, supported);
+ linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, supported);
+
+ /* Phy supports Speeds up to 5G with Autoneg though the phy PMA says otherwise */
+ linkmode_or(supported, supported, phy_gbit_features);
+ /* Set the 5G speed if it wasn't set as part of the PMA feature discovery */
+ linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
+ linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
+
+ return 0;
+}
+
static int aqr107_probe(struct phy_device *phydev)
{
int ret;
@@ -757,16 +776,6 @@ static int aqr107_probe(struct phy_device *phydev)
return aqr_hwmon_probe(phydev);
}
-static int aqr111_config_init(struct phy_device *phydev)
-{
- /* AQR111 reports supporting speed up to 10G,
- * however only speeds up to 5G are supported.
- */
- phy_set_max_speed(phydev, SPEED_5000);
-
- return aqr107_config_init(phydev);
-}
-
static int aqr115c_get_features(struct phy_device *phydev)
{
unsigned long *supported = phydev->supported;
@@ -868,6 +877,7 @@ static struct phy_driver aqr_driver[] = {
.get_sset_count = aqr107_get_sset_count,
.get_strings = aqr107_get_strings,
.get_stats = aqr107_get_stats,
+ .get_features = aqr115c_get_features,
.link_change_notify = aqr107_link_change_notify,
.led_brightness_set = aqr_phy_led_brightness_set,
.led_hw_is_supported = aqr_phy_led_hw_is_supported,
@@ -880,7 +890,7 @@ static struct phy_driver aqr_driver[] = {
.name = "Aquantia AQR111",
.probe = aqr107_probe,
.get_rate_matching = aqr107_get_rate_matching,
- .config_init = aqr111_config_init,
+ .config_init = aqr107_config_init,
.config_aneg = aqr_config_aneg,
.config_intr = aqr_config_intr,
.handle_interrupt = aqr_handle_interrupt,
@@ -892,6 +902,7 @@ static struct phy_driver aqr_driver[] = {
.get_sset_count = aqr107_get_sset_count,
.get_strings = aqr107_get_strings,
.get_stats = aqr107_get_stats,
+ .get_features = aqr111_get_features,
.link_change_notify = aqr107_link_change_notify,
.led_brightness_set = aqr_phy_led_brightness_set,
.led_hw_is_supported = aqr_phy_led_hw_is_supported,
@@ -904,7 +915,7 @@ static struct phy_driver aqr_driver[] = {
.name = "Aquantia AQR111B0",
.probe = aqr107_probe,
.get_rate_matching = aqr107_get_rate_matching,
- .config_init = aqr111_config_init,
+ .config_init = aqr107_config_init,
.config_aneg = aqr_config_aneg,
.config_intr = aqr_config_intr,
.handle_interrupt = aqr_handle_interrupt,
@@ -916,6 +927,7 @@ static struct phy_driver aqr_driver[] = {
.get_sset_count = aqr107_get_sset_count,
.get_strings = aqr107_get_strings,
.get_stats = aqr107_get_stats,
+ .get_features = aqr111_get_features,
.link_change_notify = aqr107_link_change_notify,
.led_brightness_set = aqr_phy_led_brightness_set,
.led_hw_is_supported = aqr_phy_led_hw_is_supported,
@@ -1025,7 +1037,7 @@ static struct phy_driver aqr_driver[] = {
.name = "Aquantia AQR114C",
.probe = aqr107_probe,
.get_rate_matching = aqr107_get_rate_matching,
- .config_init = aqr111_config_init,
+ .config_init = aqr107_config_init,
.config_aneg = aqr_config_aneg,
.config_intr = aqr_config_intr,
.handle_interrupt = aqr_handle_interrupt,
@@ -1037,6 +1049,7 @@ static struct phy_driver aqr_driver[] = {
.get_sset_count = aqr107_get_sset_count,
.get_strings = aqr107_get_strings,
.get_stats = aqr107_get_stats,
+ .get_features = aqr111_get_features,
.link_change_notify = aqr107_link_change_notify,
.led_brightness_set = aqr_phy_led_brightness_set,
.led_hw_is_supported = aqr_phy_led_hw_is_supported,
--
2.25.1Hi,
On Thu, 26 Sep 2024 18:05:53 -0700
Abhishek Chauhan <quic_abchauha@quicinc.com> wrote:
> Remove the use of phy_set_max_speed in phy driver as the
> function is mainly used in MAC driver to set the max
> speed.
>
> Instead use get_features to fix up Phy PMA capabilities for
> AQR111, AQR111B0, AQR114C and AQCS109
>
> Fixes: 038ba1dc4e54 ("net: phy: aquantia: add AQR111 and AQR111B0 PHY ID")
> Fixes: 0974f1f03b07 ("net: phy: aquantia: remove false 5G and 10G speed ability for AQCS109")
> Fixes: c278ec644377 ("net: phy: aquantia: add support for AQR114C PHY ID")
> Link: https://lore.kernel.org/all/20240913011635.1286027-1-quic_abchauha@quicinc.com/T/
> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>
[...]
> +static int aqr111_get_features(struct phy_device *phydev)
> +{
> + unsigned long *supported = phydev->supported;
> + int ret;
> +
> + /* Normal feature discovery */
> + ret = genphy_c45_pma_read_abilities(phydev);
> + if (ret)
> + return ret;
> +
> + /* PHY FIXUP */
> + /* Although the PHY sets bit 12.18.19, it does not support 10G modes */
> + linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
> + linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT, supported);
> + linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, supported);
> +
> + /* Phy supports Speeds up to 5G with Autoneg though the phy PMA says otherwise */
> + linkmode_or(supported, supported, phy_gbit_features);
> + /* Set the 5G speed if it wasn't set as part of the PMA feature discovery */
> + linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
> + linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
As you are moving away from phy_set_max_speed(phydev, 5000), it should mean
that what used to be in the supported bits already contained the
5GBaseT bit, as phy_set_max_speed simply clears the highest speeds.
In such case, calling the newly introduced function from
patch 1 should be enough ?
Thanks,
Maxime
On 9/27/2024 9:37 AM, Maxime Chevallier wrote:
> Hi,
>
> On Thu, 26 Sep 2024 18:05:53 -0700
> Abhishek Chauhan <quic_abchauha@quicinc.com> wrote:
>
>> Remove the use of phy_set_max_speed in phy driver as the
>> function is mainly used in MAC driver to set the max
>> speed.
>>
>> Instead use get_features to fix up Phy PMA capabilities for
>> AQR111, AQR111B0, AQR114C and AQCS109
>>
>> Fixes: 038ba1dc4e54 ("net: phy: aquantia: add AQR111 and AQR111B0 PHY ID")
>> Fixes: 0974f1f03b07 ("net: phy: aquantia: remove false 5G and 10G speed ability for AQCS109")
>> Fixes: c278ec644377 ("net: phy: aquantia: add support for AQR114C PHY ID")
>> Link: https://lore.kernel.org/all/20240913011635.1286027-1-quic_abchauha@quicinc.com/T/
>> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>
>
> [...]
>
>> +static int aqr111_get_features(struct phy_device *phydev)
>> +{
>> + unsigned long *supported = phydev->supported;
>> + int ret;
>> +
>> + /* Normal feature discovery */
>> + ret = genphy_c45_pma_read_abilities(phydev);
>> + if (ret)
>> + return ret;
>> +
>> + /* PHY FIXUP */
>> + /* Although the PHY sets bit 12.18.19, it does not support 10G modes */
>> + linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
>> + linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT, supported);
>> + linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, supported);
>> +
>> + /* Phy supports Speeds up to 5G with Autoneg though the phy PMA says otherwise */
>> + linkmode_or(supported, supported, phy_gbit_features);
>> + /* Set the 5G speed if it wasn't set as part of the PMA feature discovery */
>> + linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
>> + linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
>
> As you are moving away from phy_set_max_speed(phydev, 5000), it should mean
> that what used to be in the supported bits already contained the
> 5GBaseT bit, as phy_set_max_speed simply clears the highest speeds.
>
> In such case, calling the newly introduced function from
> patch 1 should be enough ?
>
Well i am not sure about how other phy(AQR111, AQR111B0, AQR114C and AQCS109) behaved,
but based on my testing and observation with AQR115c, it was pretty clear that
the phy did not advertise Autoneg capabilities, did not set lower speed such as 10M/100M/1000BaseT
,it did set capabilities beyond what is recommended in the data book.
So the below mentioned phys such as
AQR111, AQR111B0, AQR114C = supports speed up to 5Gbps which means i cannot use the function
defined in the previous patch as that sets speeds up to 2.5Gbps and all lower speeds.
AQCS109 = supports speed up to 2.5Gbps and hence i have reused the same function aqr115c_get_features
as part of this patch.
Also my plan was to make 1 patch but i got review comments from Andrew to separate it out
and hence two patches with 2 different functions and removing the usage of phy_set_max_speed
> Thanks,
>
> Maxime
On Fri, 27 Sep 2024 12:42:36 -0700
"Abhishek Chauhan (ABC)" <quic_abchauha@quicinc.com> wrote:
> On 9/27/2024 9:37 AM, Maxime Chevallier wrote:
> > Hi,
> >
> > On Thu, 26 Sep 2024 18:05:53 -0700
> > Abhishek Chauhan <quic_abchauha@quicinc.com> wrote:
> >
> >> Remove the use of phy_set_max_speed in phy driver as the
> >> function is mainly used in MAC driver to set the max
> >> speed.
> >>
> >> Instead use get_features to fix up Phy PMA capabilities for
> >> AQR111, AQR111B0, AQR114C and AQCS109
> >>
> >> Fixes: 038ba1dc4e54 ("net: phy: aquantia: add AQR111 and AQR111B0 PHY ID")
> >> Fixes: 0974f1f03b07 ("net: phy: aquantia: remove false 5G and 10G speed ability for AQCS109")
> >> Fixes: c278ec644377 ("net: phy: aquantia: add support for AQR114C PHY ID")
> >> Link: https://lore.kernel.org/all/20240913011635.1286027-1-quic_abchauha@quicinc.com/T/
> >> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>
> >
> > [...]
> >
> >> +static int aqr111_get_features(struct phy_device *phydev)
> >> +{
> >> + unsigned long *supported = phydev->supported;
> >> + int ret;
> >> +
> >> + /* Normal feature discovery */
> >> + ret = genphy_c45_pma_read_abilities(phydev);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + /* PHY FIXUP */
> >> + /* Although the PHY sets bit 12.18.19, it does not support 10G modes */
> >> + linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
> >> + linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT, supported);
> >> + linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, supported);
> >> +
> >> + /* Phy supports Speeds up to 5G with Autoneg though the phy PMA says otherwise */
> >> + linkmode_or(supported, supported, phy_gbit_features);
> >> + /* Set the 5G speed if it wasn't set as part of the PMA feature discovery */
> >> + linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
> >> + linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
> >
> > As you are moving away from phy_set_max_speed(phydev, 5000), it should mean
> > that what used to be in the supported bits already contained the
> > 5GBaseT bit, as phy_set_max_speed simply clears the highest speeds.
> >
> > In such case, calling the newly introduced function from
> > patch 1 should be enough ?
> >
>
> Well i am not sure about how other phy(AQR111, AQR111B0, AQR114C and AQCS109) behaved,
> but based on my testing and observation with AQR115c, it was pretty clear that
> the phy did not advertise Autoneg capabilities, did not set lower speed such as 10M/100M/1000BaseT
> ,it did set capabilities beyond what is recommended in the data book.
>
> So the below mentioned phys such as
>
> AQR111, AQR111B0, AQR114C = supports speed up to 5Gbps which means i cannot use the function
> defined in the previous patch as that sets speeds up to 2.5Gbps and all lower speeds.
>
> AQCS109 = supports speed up to 2.5Gbps and hence i have reused the same function aqr115c_get_features
> as part of this patch.
I understand your point, and it's hard indeed to be sure that no
regression was introduced. It does feel like you're reading the PHY
features, then reconstructing them almost from scratch again, but given
that the PMA report looks totally incorrect, there not much choice
indeed. So that's fine for me then.
Maxime
On Sat, Sep 28, 2024 at 10:52:42AM +0200, Maxime Chevallier wrote:
> On Fri, 27 Sep 2024 12:42:36 -0700
> "Abhishek Chauhan (ABC)" <quic_abchauha@quicinc.com> wrote:
>
> > On 9/27/2024 9:37 AM, Maxime Chevallier wrote:
> > > Hi,
> > >
> > > On Thu, 26 Sep 2024 18:05:53 -0700
> > > Abhishek Chauhan <quic_abchauha@quicinc.com> wrote:
> > >
> > >> Remove the use of phy_set_max_speed in phy driver as the
> > >> function is mainly used in MAC driver to set the max
> > >> speed.
> > >>
> > >> Instead use get_features to fix up Phy PMA capabilities for
> > >> AQR111, AQR111B0, AQR114C and AQCS109
> > >>
> > >> Fixes: 038ba1dc4e54 ("net: phy: aquantia: add AQR111 and AQR111B0 PHY ID")
> > >> Fixes: 0974f1f03b07 ("net: phy: aquantia: remove false 5G and 10G speed ability for AQCS109")
> > >> Fixes: c278ec644377 ("net: phy: aquantia: add support for AQR114C PHY ID")
> > >> Link: https://lore.kernel.org/all/20240913011635.1286027-1-quic_abchauha@quicinc.com/T/
> > >> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>
> > >
> > > [...]
> > >
> > >> +static int aqr111_get_features(struct phy_device *phydev)
> > >> +{
> > >> + unsigned long *supported = phydev->supported;
> > >> + int ret;
> > >> +
> > >> + /* Normal feature discovery */
> > >> + ret = genphy_c45_pma_read_abilities(phydev);
> > >> + if (ret)
> > >> + return ret;
> > >> +
> > >> + /* PHY FIXUP */
> > >> + /* Although the PHY sets bit 12.18.19, it does not support 10G modes */
> > >> + linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
> > >> + linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT, supported);
> > >> + linkmode_clear_bit(ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, supported);
> > >> +
> > >> + /* Phy supports Speeds up to 5G with Autoneg though the phy PMA says otherwise */
> > >> + linkmode_or(supported, supported, phy_gbit_features);
> > >> + /* Set the 5G speed if it wasn't set as part of the PMA feature discovery */
> > >> + linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
> > >> + linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
> > >
> > > As you are moving away from phy_set_max_speed(phydev, 5000), it should mean
> > > that what used to be in the supported bits already contained the
> > > 5GBaseT bit, as phy_set_max_speed simply clears the highest speeds.
> > >
> > > In such case, calling the newly introduced function from
> > > patch 1 should be enough ?
> > >
> >
> > Well i am not sure about how other phy(AQR111, AQR111B0, AQR114C and AQCS109) behaved,
> > but based on my testing and observation with AQR115c, it was pretty clear that
> > the phy did not advertise Autoneg capabilities, did not set lower speed such as 10M/100M/1000BaseT
> > ,it did set capabilities beyond what is recommended in the data book.
> >
> > So the below mentioned phys such as
> >
> > AQR111, AQR111B0, AQR114C = supports speed up to 5Gbps which means i cannot use the function
> > defined in the previous patch as that sets speeds up to 2.5Gbps and all lower speeds.
> >
> > AQCS109 = supports speed up to 2.5Gbps and hence i have reused the same function aqr115c_get_features
> > as part of this patch.
>
> I understand your point, and it's hard indeed to be sure that no
> regression was introduced. It does feel like you're reading the PHY
> features, then reconstructing them almost from scratch again, but given
> that the PMA report looks totally incorrect, there not much choice
> indeed. So that's fine for me then.
I think this is getting overly complex, so let's rewind a bit.
I believe Abhishek mentioned in a previous review what the differences
are between what the PHY reports when read, and what it actually
supports, and the result was that there was not a single bit in the
supported mask that was correct. I was hopeful that maybe Andrew would
respond to that, but seems not to, so I'm putting this statement here.
More on this below.
Therefore, I believe that using genphy_c45_pma_read_abilities() here
is a mistake.
Now, looking at these two patches, patch 1 clears:
ETHTOOL_LINK_MODE_10000baseT_Full_BIT
ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT
ETHTOOL_LINK_MODE_10000baseKR_Full_BIT
ETHTOOL_LINK_MODE_5000baseT_Full_BIT
and sets:
phy_gbit_features
ETHTOOL_LINK_MODE_2500baseT_Full_BIT
Patch 2 clears:
ETHTOOL_LINK_MODE_10000baseT_Full_BIT
ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT
ETHTOOL_LINK_MODE_10000baseKR_Full_BIT
and sets:
phy_gbit_features
ETHTOOL_LINK_MODE_5000baseT_Full_BIT
ETHTOOL_LINK_MODE_2500baseT_Full_BIT
So, the only difference between the code in patch 1 and patch 2 is
whether ETHTOOL_LINK_MODE_5000baseT_Full_BIT is set. Everything else
is the same.
So, the function in patch 2 can call the function in patch 1, and then
do:
linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
phydev->supported);
However, I go back to my original point. Is it worth all this
complexity clearing and setting these bits, when what is read from
the PHY is so wrong? This is what was said in previous emails:
| //Print added by me in the AQR driver
| [ 5.583440] AQR supported mask=00,00000000,00018000,000e102c
|
| key points :-
|
| AQR115c supports 10Mbps(F/H) but feature discovery says no
| AQR115C supports 1Gbps(F/H) but feature discovery says no
| AQR115C supports 2500BaseX but feature discovery says no
| AQR115C supports Autoneg but feature discovery says Hell no!
| AQR115C does not support 10GBaseT/KX4/KR but feature discovery says yes
| AQR115c does not support 5GbaseT but feature discovery says yes
Providing a fuller picture:
- bit 0,1 not set (10baseT_{Half,Full}) - but should be set.
- bit 2,3 are set (100baseT_{Half,Full}) - correct!
- bit 4 not set (1000baseT_Half) - but should be set.
- bit 5 is set (1000baseT_Full) - correct!
- bit 6 not set (Autoneg) - but should be set.
- bit 12 is set (10000baseT_Full) - no idea!
- bits 17-19 set (1000baseKX_Full, 10000baseKX4_Full, 10000baseKR_Full) -
but should be clear.
- bit 47 is set (2500baseT_Full) - correct!
- bit 48 is set (5000baseT_Full) - but should be clear.
Sorting this by status (correct, should be clear, should be set):
Should be clear:
- bits 17-19 set (1000baseKX_Full, 10000baseKX4_Full, 10000baseKR_Full)
- bit 48 is set (5000baseT_Full)
Should be set:
- bit 0,1 not set (10baseT_{Half,Full})
- bit 4 not set (1000baseT_Half)
- bit 6 not set (Autoneg)
Correct:
- bit 2,3 are set (100baseT_{Half,Full})
- bit 5 is set (1000baseT_Full)
- bit 47 is set (2500baseT_Full)
Unknown:
- bit 12 is set (10000baseT_Full)
So that's four bits to be cleared, four bits to be set, four bits are
correct, and one we don't know. Is it really worth reading the PHY
abilities, or would just having a list of the ethtool link modes that
the PHY actually supports be a saner, more maintainable solution as
originally proposed?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> I think this is getting overly complex, so let's rewind a bit. > > I believe Abhishek mentioned in a previous review what the differences > are between what the PHY reports when read, and what it actually > supports, and the result was that there was not a single bit in the > supported mask that was correct. I was hopeful that maybe Andrew would > respond to that, but seems not to, so I'm putting this statement here. > More on this below. Yes, i did not really realise how wrong Marvell got this. As you point out, it is more wrong than right. My thinking with calling the usual feature discovery mechanism and then fixing them up, is that we keep extending them. BaseT1 has been added etc. If a PHY is mostly getting it right, we might in the future get new features implemented for free, if the hardware correctly declares them. But in this case, if it cannot get even the basics mostly correct, there is little hope it will get more exotic features correct. So, i agree in Russell. Forget about asking the hardware, just hard code the correct features. Sorry for making you do extra work which you now need to discard. However, please do keep it as two patches. It makes it easier to deal with regressions on the device you cannot test if we can just revert one patch. Andrew
On 9/30/2024 5:18 AM, Andrew Lunn wrote: >> I think this is getting overly complex, so let's rewind a bit. >> >> I believe Abhishek mentioned in a previous review what the differences >> are between what the PHY reports when read, and what it actually >> supports, and the result was that there was not a single bit in the >> supported mask that was correct. I was hopeful that maybe Andrew would >> respond to that, but seems not to, so I'm putting this statement here. >> More on this below. > > Yes, i did not really realise how wrong Marvell got this. As you point > out, it is more wrong than right. > > My thinking with calling the usual feature discovery mechanism and > then fixing them up, is that we keep extending them. BaseT1 has been > added etc. If a PHY is mostly getting it right, we might in the future > get new features implemented for free, if the hardware correctly > declares them. But in this case, if it cannot get even the basics > mostly correct, there is little hope it will get more exotic features > correct. > > So, i agree in Russell. Forget about asking the hardware, just hard > code the correct features. > > Sorry for making you do extra work which you now need to discard. > No worries, Its better to discuss now than to regret later. I will make the changes accordingly and raise v5 today after testing. Thanks Russell/Maxime/Andrew. > However, please do keep it as two patches. It makes it easier to deal > with regressions on the device you cannot test if we can just revert > one patch. > > Andrew
© 2016 - 2024 Red Hat, Inc.