2024-02-13 18:24:54

by Christian Marangi

[permalink] [raw]
Subject: [net-next RFC PATCH 0/2] net: phy: aquantia: fix system interface provision

Posting this as RFC as I think this require some discussion on the topic.

There is currently a problem. OEM multiple time provision Aquantia FW
with random and wrong data that may apply for one board but doesn't for
another. And at the same time OEM use the same broken FW for multiple
board and apply fixup at runtime.

This is the common case for AQR112 where downstream (uboot, OEM sdk,
openwrt to have the port correctly working) hack patch are used to fixup
broken system interface provision from the FW.

The downstream patch do one simple thing, they setup the SERDES startup
rate (that the FW may wrongly not init) and overwrite the
global system config for each rate to default values for the rwquested PHY
interface.

Now setting the SERDES startup value is SAFE, and this can be implemented
right away.

Overwriting the SERDES modes for each rate tho might pose some question
on how this is correct or wrong.

Reality is that probably every user an Aquantia PHY in one way or another
makes use of the SDK and have this patch in use making any kind of
provision on the FW ignored, (since the default values are always applied
at runtime) making the introduction of this change safe and restoring
correct functionality of AQR112 in the case of a broken FW loaded.

As said in the commit description, one thing this handle is the problem
where the FW is provision with 10BASE-R while the MAC supports and expect
UXSGMII.

The AQR PHY can correctly switch from one mode to another and I think it's
the most common case where one FW is broken.
This might be the safest change but again would not give us 100% idea that
the thing provision by the FW are correct.

Another idea might be adding a property like
"aquantia,broken-system-interface-provision" and with that enable we would
overwrite values with the default one.

Christian Marangi (2):
net: phy: aquantia: setup interface protocols for AQR112
net: phy: aquantia: add AQR112C and AQR112R PHY ID

drivers/net/phy/aquantia/aquantia.h | 17 +++
drivers/net/phy/aquantia/aquantia_main.c | 152 +++++++++++++++++++++++
2 files changed, 169 insertions(+)

--
2.43.0



2024-02-13 18:25:19

by Christian Marangi

[permalink] [raw]
Subject: [net-next RFC PATCH 1/2] net: phy: aquantia: setup interface protocols for AQR112

Aquantia Gen3 PHYs require additional regs to be set to correctly work
and communicate with the MAC.

For each rate, the startup rate needs to be set and the required global
cfg needs to be set.

The global cfg are per Rate and require different configuration based on
the requested interface mode.

Some FW might apply the correct values by default but some OEM might
provide generic FW that require additional fixup at runtime as the FW
doesn't correctly provison the PHY for the attached MAC interface mode.

The default values are taken from various source and SDK and all use the
same values for SGMII, 2500BASEX, 10GBASER and USXGMII.

One common problem that this handle is case where the FW provision for
10BASER but USXGMII is actually requested, hance runtime fixup is
required.

Signed-off-by: Christian Marangi <[email protected]>
---
drivers/net/phy/aquantia/aquantia.h | 17 ++++
drivers/net/phy/aquantia/aquantia_main.c | 110 +++++++++++++++++++++++
2 files changed, 127 insertions(+)

diff --git a/drivers/net/phy/aquantia/aquantia.h b/drivers/net/phy/aquantia/aquantia.h
index 1c19ae74ad2b..019528c536c7 100644
--- a/drivers/net/phy/aquantia/aquantia.h
+++ b/drivers/net/phy/aquantia/aquantia.h
@@ -39,6 +39,15 @@
#define VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA_MASK GENMASK(15, 0)
#define VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA(x) FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA_MASK, (u16)(x))

+#define VEND1_GLOBAL_STARTUP_RATE 0x031a
+#define VEND1_GLOBAL_STARTUP_RATE_MASK GENMASK(3, 0)
+#define VEND1_GLOBAL_STARTUP_RATE_LOW_POWER 0
+#define VEND1_GLOBAL_STARTUP_RATE_100M 1
+#define VEND1_GLOBAL_STARTUP_RATE_1G 2
+#define VEND1_GLOBAL_STARTUP_RATE_10G 3
+#define VEND1_GLOBAL_STARTUP_RATE_2_5G 4
+#define VEND1_GLOBAL_STARTUP_RATE_5G 5
+
/* The following registers all have similar layouts; first the registers... */
#define VEND1_GLOBAL_CFG_10M 0x0310
#define VEND1_GLOBAL_CFG_100M 0x031b
@@ -52,6 +61,14 @@
#define VEND1_GLOBAL_CFG_SERDES_MODE_SGMII 3
#define VEND1_GLOBAL_CFG_SERDES_MODE_OCSGMII 4
#define VEND1_GLOBAL_CFG_SERDES_MODE_XFI5G 6
+/* Enable System Interface Autonegotiation */
+#define VEND1_GLOBAL_CFG_AUTONEG_EN BIT(3)
+/* Enable System Interface Training */
+#define VEND1_GLOBAL_CFG_TRAINING_EN BIT(4)
+/* Reset System Interface For Every Transition */
+#define VEND1_GLOBAL_CFG_RESET_ON_TRANSITION_EN BIT(5)
+/* Keep SERDES Silent During Transition */
+#define VEND1_GLOBAL_CFG_SERDES_SILENCE_EN BIT(6)
#define VEND1_GLOBAL_CFG_RATE_ADAPT GENMASK(8, 7)
#define VEND1_GLOBAL_CFG_RATE_ADAPT_NONE 0
#define VEND1_GLOBAL_CFG_RATE_ADAPT_USX 1
diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
index 97a2fafa15ca..6ee1a134bc60 100644
--- a/drivers/net/phy/aquantia/aquantia_main.c
+++ b/drivers/net/phy/aquantia/aquantia_main.c
@@ -746,6 +746,114 @@ static int aqr107_probe(struct phy_device *phydev)
return aqr_hwmon_probe(phydev);
}

+static int aqr112_setup_interface_protocols(struct phy_device *phydev)
+{
+ phy_interface_t iface = phydev->interface;
+ u16 startup_rate, global_cfg_val;
+ int i, ret;
+
+ /* Default global cfg are taken from Aquantia UBoot Source and various
+ * source and all makes use of the following reference values.
+ */
+ switch (iface) {
+ case PHY_INTERFACE_MODE_SGMII:
+ startup_rate = VEND1_GLOBAL_STARTUP_RATE_1G;
+ global_cfg_val = FIELD_PREP(VEND1_GLOBAL_CFG_SERDES_MODE,
+ VEND1_GLOBAL_CFG_SERDES_MODE_SGMII) |
+ VEND1_GLOBAL_CFG_AUTONEG_EN |
+ VEND1_GLOBAL_CFG_SERDES_SILENCE_EN;
+ break;
+ case PHY_INTERFACE_MODE_2500BASEX:
+ startup_rate = VEND1_GLOBAL_STARTUP_RATE_2_5G;
+ global_cfg_val = FIELD_PREP(VEND1_GLOBAL_CFG_SERDES_MODE,
+ VEND1_GLOBAL_CFG_SERDES_MODE_OCSGMII) |
+ VEND1_GLOBAL_CFG_SERDES_SILENCE_EN |
+ FIELD_PREP(VEND1_GLOBAL_CFG_RATE_ADAPT,
+ VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE);
+ break;
+ case PHY_INTERFACE_MODE_5GBASER:
+ startup_rate = VEND1_GLOBAL_STARTUP_RATE_5G;
+ global_cfg_val = FIELD_PREP(VEND1_GLOBAL_CFG_SERDES_MODE,
+ VEND1_GLOBAL_CFG_SERDES_MODE_XFI5G) |
+ FIELD_PREP(VEND1_GLOBAL_CFG_RATE_ADAPT,
+ VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE);
+ break;
+ case PHY_INTERFACE_MODE_10GBASER:
+ case PHY_INTERFACE_MODE_USXGMII:
+ startup_rate = VEND1_GLOBAL_STARTUP_RATE_10G;
+ global_cfg_val = FIELD_PREP(VEND1_GLOBAL_CFG_SERDES_MODE,
+ VEND1_GLOBAL_CFG_SERDES_MODE_XFI);
+ if (iface == PHY_INTERFACE_MODE_USXGMII)
+ global_cfg_val |= FIELD_PREP(VEND1_GLOBAL_CFG_RATE_ADAPT,
+ VEND1_GLOBAL_CFG_RATE_ADAPT_USX);
+ else
+ global_cfg_val |= FIELD_PREP(VEND1_GLOBAL_CFG_RATE_ADAPT,
+ VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ /* set PHY in low power mode so we can configure protocols */
+ ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_SC,
+ VEND1_GLOBAL_SC_LOW_POWER);
+ if (ret)
+ return ret;
+
+ /* Some dalay is needed to put the chip in low-power */
+ mdelay(10);
+
+ /* Setup the SERDES interface link startup rate */
+ ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_STARTUP_RATE,
+ FIELD_PREP(VEND1_GLOBAL_STARTUP_RATE_MASK, startup_rate));
+ if (ret)
+ return ret;
+
+ /* Walk the media-speed configuration registers to setup host-side
+ * serdes modes that may be used by the PHY depending on the
+ * negotiated media speed.
+ */
+ for (i = 0; i < ARRAY_SIZE(aqr_global_cfg_regs); i++) {
+ u16 reg = aqr_global_cfg_regs[i];
+ int val;
+
+ val = phy_read_mmd(phydev, MDIO_MMD_VEND1, reg);
+ if (val < 0)
+ return val;
+
+ /* If global_cfg is 0, rate is not supported by FW */
+ if (!val)
+ continue;
+
+ ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, reg,
+ global_cfg_val);
+ if (ret)
+ return ret;
+ }
+
+ /* set PHY out of low power mode */
+ ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_SC,
+ VEND1_GLOBAL_SC_LOW_POWER);
+ if (ret)
+ return ret;
+
+ /* Some dalay is needed to put the chip out of low-power */
+ mdelay(10);
+
+ return 0;
+}
+
+static int aqr112_config_init(struct phy_device *phydev)
+{
+ int ret;
+
+ ret = aqr112_setup_interface_protocols(phydev);
+ if (ret)
+ return ret;
+
+ return aqr107_fill_interface_modes(phydev);
+}
+
static struct phy_driver aqr_driver[] = {
{
PHY_ID_MATCH_MODEL(PHY_ID_AQ1202),
@@ -831,6 +939,7 @@ static struct phy_driver aqr_driver[] = {
PHY_ID_MATCH_MODEL(PHY_ID_AQR112),
.name = "Aquantia AQR112",
.probe = aqr107_probe,
+ .config_init = aqr112_config_init,
.config_aneg = aqr_config_aneg,
.config_intr = aqr_config_intr,
.handle_interrupt = aqr_handle_interrupt,
@@ -849,6 +958,7 @@ static struct phy_driver aqr_driver[] = {
PHY_ID_MATCH_MODEL(PHY_ID_AQR412),
.name = "Aquantia AQR412",
.probe = aqr107_probe,
+ .config_init = aqr112_config_init,
.config_aneg = aqr_config_aneg,
.config_intr = aqr_config_intr,
.handle_interrupt = aqr_handle_interrupt,
--
2.43.0


2024-02-13 18:25:33

by Christian Marangi

[permalink] [raw]
Subject: [net-next RFC PATCH 2/2] net: phy: aquantia: add AQR112C and AQR112R PHY ID

Add Aquantia AQR112C and AQR112R PHY ID. These additional PHY are just
variant of the AQR112 with a different chip size or a different target
market. The R variant is usually found on commercial product from PUZZEL
OEM.

Signed-off-by: Christian Marangi <[email protected]>
---
drivers/net/phy/aquantia/aquantia_main.c | 42 ++++++++++++++++++++++++
1 file changed, 42 insertions(+)

diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
index 6ee1a134bc60..fd7a79dbb4fb 100644
--- a/drivers/net/phy/aquantia/aquantia_main.c
+++ b/drivers/net/phy/aquantia/aquantia_main.c
@@ -23,6 +23,8 @@
#define PHY_ID_AQCS109 0x03a1b5c2
#define PHY_ID_AQR405 0x03a1b4b0
#define PHY_ID_AQR112 0x03a1b662
+#define PHY_ID_AQR112C 0x03a1b790
+#define PHY_ID_AQR112R 0x31c31d12
#define PHY_ID_AQR412 0x03a1b712
#define PHY_ID_AQR113C 0x31c31c12

@@ -954,6 +956,44 @@ static struct phy_driver aqr_driver[] = {
.get_stats = aqr107_get_stats,
.link_change_notify = aqr107_link_change_notify,
},
+{
+ PHY_ID_MATCH_MODEL(PHY_ID_AQR112C),
+ .name = "Aquantia AQR112C",
+ .probe = aqr107_probe,
+ .config_init = aqr112_config_init,
+ .config_aneg = aqr_config_aneg,
+ .config_intr = aqr_config_intr,
+ .handle_interrupt = aqr_handle_interrupt,
+ .get_tunable = aqr107_get_tunable,
+ .set_tunable = aqr107_set_tunable,
+ .suspend = aqr107_suspend,
+ .resume = aqr107_resume,
+ .read_status = aqr107_read_status,
+ .get_rate_matching = aqr107_get_rate_matching,
+ .get_sset_count = aqr107_get_sset_count,
+ .get_strings = aqr107_get_strings,
+ .get_stats = aqr107_get_stats,
+ .link_change_notify = aqr107_link_change_notify,
+},
+{
+ PHY_ID_MATCH_MODEL(PHY_ID_AQR112R),
+ .name = "Aquantia AQR112R",
+ .probe = aqr107_probe,
+ .config_init = aqr112_config_init,
+ .config_aneg = aqr_config_aneg,
+ .config_intr = aqr_config_intr,
+ .handle_interrupt = aqr_handle_interrupt,
+ .get_tunable = aqr107_get_tunable,
+ .set_tunable = aqr107_set_tunable,
+ .suspend = aqr107_suspend,
+ .resume = aqr107_resume,
+ .read_status = aqr107_read_status,
+ .get_rate_matching = aqr107_get_rate_matching,
+ .get_sset_count = aqr107_get_sset_count,
+ .get_strings = aqr107_get_strings,
+ .get_stats = aqr107_get_stats,
+ .link_change_notify = aqr107_link_change_notify,
+},
{
PHY_ID_MATCH_MODEL(PHY_ID_AQR412),
.name = "Aquantia AQR412",
@@ -1005,6 +1045,8 @@ static struct mdio_device_id __maybe_unused aqr_tbl[] = {
{ PHY_ID_MATCH_MODEL(PHY_ID_AQCS109) },
{ PHY_ID_MATCH_MODEL(PHY_ID_AQR405) },
{ PHY_ID_MATCH_MODEL(PHY_ID_AQR112) },
+ { PHY_ID_MATCH_MODEL(PHY_ID_AQR112C) },
+ { PHY_ID_MATCH_MODEL(PHY_ID_AQR112R) },
{ PHY_ID_MATCH_MODEL(PHY_ID_AQR412) },
{ PHY_ID_MATCH_MODEL(PHY_ID_AQR113C) },
{ }
--
2.43.0


2024-02-13 18:51:17

by Andrew Lunn

[permalink] [raw]
Subject: Re: [net-next RFC PATCH 0/2] net: phy: aquantia: fix system interface provision

On Tue, Feb 13, 2024 at 07:24:10PM +0100, Christian Marangi wrote:
> Posting this as RFC as I think this require some discussion on the topic.
>
> There is currently a problem. OEM multiple time provision Aquantia FW
> with random and wrong data that may apply for one board but doesn't for
> another. And at the same time OEM use the same broken FW for multiple
> board and apply fixup at runtime.
>
> This is the common case for AQR112 where downstream (uboot, OEM sdk,
> openwrt to have the port correctly working) hack patch are used to fixup
> broken system interface provision from the FW.
>
> The downstream patch do one simple thing, they setup the SERDES startup
> rate (that the FW may wrongly not init) and overwrite the
> global system config for each rate to default values for the rwquested PHY
> interface.
>
> Now setting the SERDES startup value is SAFE, and this can be implemented
> right away.
>
> Overwriting the SERDES modes for each rate tho might pose some question
> on how this is correct or wrong.
>
> Reality is that probably every user an Aquantia PHY in one way or another
> makes use of the SDK and have this patch in use making any kind of
> provision on the FW ignored, (since the default values are always applied
> at runtime) making the introduction of this change safe and restoring
> correct functionality of AQR112 in the case of a broken FW loaded.

This is part of the discussion i had with Aquantia about
provisioning. Basically, you cannot trust any register to contain a
known value, e.g the value the data sheet indicates the reset value
should be, or that the 802.3 standard says it should be.

So in effect, the driver needs to write every single register it
depends on.

> This might be the safest change but again would not give us 100% idea that
> the thing provision by the FW are correct.

I would say, we have to assume provision is 100% wrong. Write every
single register with the needed value.

Is the provisioning information available? Can it be read from the
flash? Can it be dumped from firmware we have on disk? Dumping it for
a number of devices could give a list of register values which are
highly suspect, ones that OEMs typically mess with. We could start by
always setting those registers.

Andrew

2024-02-13 19:19:02

by Christian Marangi

[permalink] [raw]
Subject: Re: [net-next RFC PATCH 0/2] net: phy: aquantia: fix system interface provision

On Tue, Feb 13, 2024 at 07:46:45PM +0100, Andrew Lunn wrote:
> On Tue, Feb 13, 2024 at 07:24:10PM +0100, Christian Marangi wrote:
> > Posting this as RFC as I think this require some discussion on the topic.
> >
> > There is currently a problem. OEM multiple time provision Aquantia FW
> > with random and wrong data that may apply for one board but doesn't for
> > another. And at the same time OEM use the same broken FW for multiple
> > board and apply fixup at runtime.
> >
> > This is the common case for AQR112 where downstream (uboot, OEM sdk,
> > openwrt to have the port correctly working) hack patch are used to fixup
> > broken system interface provision from the FW.
> >
> > The downstream patch do one simple thing, they setup the SERDES startup
> > rate (that the FW may wrongly not init) and overwrite the
> > global system config for each rate to default values for the rwquested PHY
> > interface.
> >
> > Now setting the SERDES startup value is SAFE, and this can be implemented
> > right away.
> >
> > Overwriting the SERDES modes for each rate tho might pose some question
> > on how this is correct or wrong.
> >
> > Reality is that probably every user an Aquantia PHY in one way or another
> > makes use of the SDK and have this patch in use making any kind of
> > provision on the FW ignored, (since the default values are always applied
> > at runtime) making the introduction of this change safe and restoring
> > correct functionality of AQR112 in the case of a broken FW loaded.
>
> This is part of the discussion i had with Aquantia about
> provisioning. Basically, you cannot trust any register to contain a
> known value, e.g the value the data sheet indicates the reset value
> should be, or that the 802.3 standard says it should be.
>
> So in effect, the driver needs to write every single register it
> depends on.
>

Well if that's the case then this RFC patch is a must. With a
misconfigured System Interface configuration, the PHY can't comunicate
with the MAC.

> > This might be the safest change but again would not give us 100% idea that
> > the thing provision by the FW are correct.
>
> I would say, we have to assume provision is 100% wrong. Write every
> single register with the needed value.
>
> Is the provisioning information available? Can it be read from the
> flash? Can it be dumped from firmware we have on disk? Dumping it for
> a number of devices could give a list of register values which are
> highly suspect, ones that OEMs typically mess with. We could start by
> always setting those registers.
>

We know where they are stored in the FW but it's not documented how the
provision values are stored in the FW. (the format, how they are
organized...) I can waste some time trying to reverse it and produce a
tool to parse them if needed.

Would love also some comments by Russell about this, there was a patch
adding support for WoL where another user was messing with these regs
and he was with the idea of being careful with overwriting the provision
values.

--
Ansuel

2024-02-13 20:59:08

by Andrew Lunn

[permalink] [raw]
Subject: Re: [net-next RFC PATCH 0/2] net: phy: aquantia: fix system interface provision

> > So in effect, the driver needs to write every single register it
> > depends on.
> >
>
> Well if that's the case then this RFC patch is a must. With a
> misconfigured System Interface configuration, the PHY can't comunicate
> with the MAC.
>
> > > This might be the safest change but again would not give us 100% idea that
> > > the thing provision by the FW are correct.
> >
> > I would say, we have to assume provision is 100% wrong. Write every
> > single register with the needed value.
> >
> > Is the provisioning information available? Can it be read from the
> > flash? Can it be dumped from firmware we have on disk? Dumping it for
> > a number of devices could give a list of register values which are
> > highly suspect, ones that OEMs typically mess with. We could start by
> > always setting those registers.
> >
>
> We know where they are stored in the FW but it's not documented how the
> provision values are stored in the FW. (the format, how they are
> organized...) I can waste some time trying to reverse it and produce a
> tool to parse them if needed.

It might be worth it. How complex could it be? The obvious format is a
C45 mmd.reg pair and a value.

> Would love also some comments by Russell about this, there was a patch
> adding support for WoL where another user was messing with these regs
> and he was with the idea of being careful with overwriting the provision
> values.

I expect the SERDES eye configuration is in there somewhere, and we
should not touch that. That was one of the arguments Aquantia made at
the time, that needs to be stored somewhere, and is board specific.

But knowing what standard 802.3 registers are commonly changed would
be useful, and could help track down silly problems like the
transmitter being disabled by default by provisioning.

Andrew

2024-02-13 21:03:22

by Christian Marangi

[permalink] [raw]
Subject: Re: [net-next RFC PATCH 0/2] net: phy: aquantia: fix system interface provision

On Tue, Feb 13, 2024 at 09:58:59PM +0100, Andrew Lunn wrote:
> > > So in effect, the driver needs to write every single register it
> > > depends on.
> > >
> >
> > Well if that's the case then this RFC patch is a must. With a
> > misconfigured System Interface configuration, the PHY can't comunicate
> > with the MAC.
> >
> > > > This might be the safest change but again would not give us 100% idea that
> > > > the thing provision by the FW are correct.
> > >
> > > I would say, we have to assume provision is 100% wrong. Write every
> > > single register with the needed value.
> > >
> > > Is the provisioning information available? Can it be read from the
> > > flash? Can it be dumped from firmware we have on disk? Dumping it for
> > > a number of devices could give a list of register values which are
> > > highly suspect, ones that OEMs typically mess with. We could start by
> > > always setting those registers.
> > >
> >
> > We know where they are stored in the FW but it's not documented how the
> > provision values are stored in the FW. (the format, how they are
> > organized...) I can waste some time trying to reverse it and produce a
> > tool to parse them if needed.
>
> It might be worth it. How complex could it be? The obvious format is a
> C45 mmd.reg pair and a value.
>

Working on it. I already confirmed the FW have actually a provision part
and is not empty.

The format looks to be u16 reg 16 value but I need to understand it
better as not everything about provision is in mmd 1e so there must be
some magic values to signal where the section has to be appled.

> > Would love also some comments by Russell about this, there was a patch
> > adding support for WoL where another user was messing with these regs
> > and he was with the idea of being careful with overwriting the provision
> > values.
>
> I expect the SERDES eye configuration is in there somewhere, and we
> should not touch that. That was one of the arguments Aquantia made at
> the time, that needs to be stored somewhere, and is board specific.
>
> But knowing what standard 802.3 registers are commonly changed would
> be useful, and could help track down silly problems like the
> transmitter being disabled by default by provisioning.
>

Yes having a tool to parse them would probably be useful and eventually
even apply fixup in the firmware loading (if we really want)

--
Ansuel

2024-02-16 23:26:38

by Christian Marangi

[permalink] [raw]
Subject: Re: [net-next RFC PATCH 0/2] net: phy: aquantia: fix system interface provision

On Tue, Feb 13, 2024 at 10:03:05PM +0100, Christian Marangi wrote:
> On Tue, Feb 13, 2024 at 09:58:59PM +0100, Andrew Lunn wrote:
> > > > So in effect, the driver needs to write every single register it
> > > > depends on.
> > > >
> > >
> > > Well if that's the case then this RFC patch is a must. With a
> > > misconfigured System Interface configuration, the PHY can't comunicate
> > > with the MAC.
> > >
> > > > > This might be the safest change but again would not give us 100% idea that
> > > > > the thing provision by the FW are correct.
> > > >
> > > > I would say, we have to assume provision is 100% wrong. Write every
> > > > single register with the needed value.
> > > >
> > > > Is the provisioning information available? Can it be read from the
> > > > flash? Can it be dumped from firmware we have on disk? Dumping it for
> > > > a number of devices could give a list of register values which are
> > > > highly suspect, ones that OEMs typically mess with. We could start by
> > > > always setting those registers.
> > > >
> > >
> > > We know where they are stored in the FW but it's not documented how the
> > > provision values are stored in the FW. (the format, how they are
> > > organized...) I can waste some time trying to reverse it and produce a
> > > tool to parse them if needed.
> >
> > It might be worth it. How complex could it be? The obvious format is a
> > C45 mmd.reg pair and a value.
> >
>
> Working on it. I already confirmed the FW have actually a provision part
> and is not empty.
>
> The format looks to be u16 reg 16 value but I need to understand it
> better as not everything about provision is in mmd 1e so there must be
> some magic values to signal where the section has to be appled.
>
> > > Would love also some comments by Russell about this, there was a patch
> > > adding support for WoL where another user was messing with these regs
> > > and he was with the idea of being careful with overwriting the provision
> > > values.
> >
> > I expect the SERDES eye configuration is in there somewhere, and we
> > should not touch that. That was one of the arguments Aquantia made at
> > the time, that needs to be stored somewhere, and is board specific.
> >
> > But knowing what standard 802.3 registers are commonly changed would
> > be useful, and could help track down silly problems like the
> > transmitter being disabled by default by provisioning.
> >
>
> Yes having a tool to parse them would probably be useful and eventually
> even apply fixup in the firmware loading (if we really want)
>

As promised, I reversed the format and created a script. It's still WIP
in the sense that I have to still to find a better way to show the
values. Here the script [1].

Feel free to suggest improvements to it. Various discovery were done
while reversing this, especially the thing with the BUG.

[1] https://github.com/Ansuel/aqr_prov_table_parser

--
Ansuel