| Message ID | 20240213133558.1836-1-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 038ba1dc4e54d51d953f5618d8eb5dd39bd9de25 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phy: aquantia: add AQR111 and AQR111B0 PHY ID | expand |
On Tue, Feb 13, 2024 at 02:35:51PM +0100, Christian Marangi wrote: > Add Aquantia AQR111 and AQR111B0 PHY ID. These PHY advertise 10G speed > but actually supports up to 5G speed, hence some manual fixup is needed. Any chance this is a "golden screwdriver" situation? The chip really can do 10G, but the firmware is supposed to limit it to 5G? This is just a firmware "bug"? Andrew
On Tue, Feb 13, 2024 at 03:09:57PM +0100, Andrew Lunn wrote: > On Tue, Feb 13, 2024 at 02:35:51PM +0100, Christian Marangi wrote: > > Add Aquantia AQR111 and AQR111B0 PHY ID. These PHY advertise 10G speed > > but actually supports up to 5G speed, hence some manual fixup is needed. > > Any chance this is a "golden screwdriver" situation? The chip really > can do 10G, but the firmware is supposed to limit it to 5G? This is > just a firmware "bug"? > From [1] the PHY can support up to 5G so yes it is a firmware bug. I can try searching for some regs to fix the wrong provision values if really needed. [1] https://www.marvell.com/content/dam/marvell/en/public-collateral/transceivers/marvell-phys-transceivers-aqrate-gen3-product-brief-2019-09.pdf
On Tue, Feb 13, 2024 at 03:18:09PM +0100, Christian Marangi wrote: > On Tue, Feb 13, 2024 at 03:09:57PM +0100, Andrew Lunn wrote: > > On Tue, Feb 13, 2024 at 02:35:51PM +0100, Christian Marangi wrote: > > > Add Aquantia AQR111 and AQR111B0 PHY ID. These PHY advertise 10G speed > > > but actually supports up to 5G speed, hence some manual fixup is needed. > > > > Any chance this is a "golden screwdriver" situation? The chip really > > can do 10G, but the firmware is supposed to limit it to 5G? This is > > just a firmware "bug"? > > > > >From [1] the PHY can support up to 5G so yes it is a firmware bug. I can > try searching for some regs to fix the wrong provision values if really > needed. > > [1] https://www.marvell.com/content/dam/marvell/en/public-collateral/transceivers/marvell-phys-transceivers-aqrate-gen3-product-brief-2019-09.pdf I think you missed the meaning of golden screwdriver. https://www.urbandictionary.com/define.php?term=Golden%20Screwdriver It could be that the silicon can do 10G, but marvell are selling it as a 5G device, with firmware limiting it to 5G. And that firmware limitation has a bug, so some of the 10G functionality is leaking through. Anyway, you change looks O.K. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Tue, Feb 13, 2024 at 04:10:01PM +0100, Andrew Lunn wrote: > On Tue, Feb 13, 2024 at 03:18:09PM +0100, Christian Marangi wrote: > > On Tue, Feb 13, 2024 at 03:09:57PM +0100, Andrew Lunn wrote: > > > On Tue, Feb 13, 2024 at 02:35:51PM +0100, Christian Marangi wrote: > > > > Add Aquantia AQR111 and AQR111B0 PHY ID. These PHY advertise 10G speed > > > > but actually supports up to 5G speed, hence some manual fixup is needed. > > > > > > Any chance this is a "golden screwdriver" situation? The chip really > > > can do 10G, but the firmware is supposed to limit it to 5G? This is > > > just a firmware "bug"? > > > > > > > >From [1] the PHY can support up to 5G so yes it is a firmware bug. I can > > try searching for some regs to fix the wrong provision values if really > > needed. > > > > [1] https://www.marvell.com/content/dam/marvell/en/public-collateral/transceivers/marvell-phys-transceivers-aqrate-gen3-product-brief-2019-09.pdf > > I think you missed the meaning of golden screwdriver. > > https://www.urbandictionary.com/define.php?term=Golden%20Screwdriver > > It could be that the silicon can do 10G, but marvell are selling it as > a 5G device, with firmware limiting it to 5G. And that firmware > limitation has a bug, so some of the 10G functionality is leaking > through. Oh! Yep I didn't know the meaning of Golden Screwdriver. With the amount of things we are noticing on these PHY it can be anything from Marvell itself, from OEM messing up with the Provision to a buf in the FW itself... (example that thing that Asus made on the other patch where they HW disable the port by default, that is against any default spec of the documentation) (or also a patch that I still have to submit where some manual fixup are needed on aqr112 since the FW mess with the SERDES startup regs (again probably OEM not correctly provisioning the FW)) > > Anyway, you change looks O.K. > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > Thanks for the Reviewed-by tag!
> With the amount of things we are noticing on these PHY it can be > anything from Marvell itself, from OEM messing up with the Provision to > a buf in the FW itself... Ideally, we want to get Marvell to release one universal firmware for each PHY. Get the OEM out of the picture. This is something i said to then Aquantia years ago, that provisioning is going to make driver support a real problem. Seems like i predicted it correctly :-( Andrew
On Tue, Feb 13, 2024 at 04:46:47PM +0100, Andrew Lunn wrote: > > With the amount of things we are noticing on these PHY it can be > > anything from Marvell itself, from OEM messing up with the Provision to > > a buf in the FW itself... > > Ideally, we want to get Marvell to release one universal firmware for > each PHY. Get the OEM out of the picture. This is something i said to > then Aquantia years ago, that provisioning is going to make driver > support a real problem. Seems like i predicted it correctly :-( > I can totally see them answer to this request with: OEM should just attach a SPI to the PHY if major modification are needed, making us (Marvell) releasing an universal firmware irrelevant since it should be handled internally by the Aquantia PHY. Reality is that on Router or more Consumer devices where OEM try to cut cost everywhere, FW are getting placed on NAND partition or even part of the OEM FW and then userspace tools are used to setup and load the Aquantia PHY FW. This cause the side effect of OEM building one Aquantia PHY FW and then fixing the Provision data at runtime making all the idea of Marvell to include these configuration values in the FW broken. (example OEM build one Aquantia PHY FW for all kind of PHY ID and then apply specific fixup to setup 10base-r or uxsgmii based on what is actually present on the board on the MAC side) (yes the PHY can support both and there is a way to tweak this... this is actually required on AQR112 and other version that we currently don't support, ideally it should be handled by the FW internally but reality is that we have case where the FW provision one mode and the other is actually present in the system)
On Tue, Feb 13, 2024 at 02:35:51PM +0100, Christian Marangi wrote: > Add Aquantia AQR111 and AQR111B0 PHY ID. These PHY advertise 10G speed > but actually supports up to 5G speed, hence some manual fixup is needed. > > The Aquantia AQR111B0 PHY is just a variant of the AQR111 with smaller > chip size. > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> The discussion around provisioning should not prevent this being merged. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
Hello: This patch was applied to netdev/net-next.git (main) by Paolo Abeni <pabeni@redhat.com>: On Tue, 13 Feb 2024 14:35:51 +0100 you wrote: > Add Aquantia AQR111 and AQR111B0 PHY ID. These PHY advertise 10G speed > but actually supports up to 5G speed, hence some manual fixup is needed. > > The Aquantia AQR111B0 PHY is just a variant of the AQR111 with smaller > chip size. > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > [...] Here is the summary with links: - [net-next] net: phy: aquantia: add AQR111 and AQR111B0 PHY ID https://git.kernel.org/netdev/net-next/c/038ba1dc4e54 You are awesome, thank you!
diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c index 97a2fafa15ca..656ea77265a2 100644 --- a/drivers/net/phy/aquantia/aquantia_main.c +++ b/drivers/net/phy/aquantia/aquantia_main.c @@ -22,6 +22,8 @@ #define PHY_ID_AQR107 0x03a1b4e0 #define PHY_ID_AQCS109 0x03a1b5c2 #define PHY_ID_AQR405 0x03a1b4b0 +#define PHY_ID_AQR111 0x03a1b610 +#define PHY_ID_AQR111B0 0x03a1b612 #define PHY_ID_AQR112 0x03a1b662 #define PHY_ID_AQR412 0x03a1b712 #define PHY_ID_AQR113C 0x31c31c12 @@ -746,6 +748,16 @@ 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 struct phy_driver aqr_driver[] = { { PHY_ID_MATCH_MODEL(PHY_ID_AQ1202), @@ -819,6 +831,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_AQR111), + .name = "Aquantia AQR111", + .probe = aqr107_probe, + .get_rate_matching = aqr107_get_rate_matching, + .config_init = aqr111_config_init, + .config_aneg = aqr_config_aneg, + .config_intr = aqr_config_intr, + .handle_interrupt = aqr_handle_interrupt, + .read_status = aqr107_read_status, + .get_tunable = aqr107_get_tunable, + .set_tunable = aqr107_set_tunable, + .suspend = aqr107_suspend, + .resume = aqr107_resume, + .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_AQR111B0), + .name = "Aquantia AQR111B0", + .probe = aqr107_probe, + .get_rate_matching = aqr107_get_rate_matching, + .config_init = aqr111_config_init, + .config_aneg = aqr_config_aneg, + .config_intr = aqr_config_intr, + .handle_interrupt = aqr_handle_interrupt, + .read_status = aqr107_read_status, + .get_tunable = aqr107_get_tunable, + .set_tunable = aqr107_set_tunable, + .suspend = aqr107_suspend, + .resume = aqr107_resume, + .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_AQR405), .name = "Aquantia AQR405", @@ -894,6 +944,8 @@ static struct mdio_device_id __maybe_unused aqr_tbl[] = { { PHY_ID_MATCH_MODEL(PHY_ID_AQR107) }, { PHY_ID_MATCH_MODEL(PHY_ID_AQCS109) }, { PHY_ID_MATCH_MODEL(PHY_ID_AQR405) }, + { PHY_ID_MATCH_MODEL(PHY_ID_AQR111) }, + { PHY_ID_MATCH_MODEL(PHY_ID_AQR111B0) }, { PHY_ID_MATCH_MODEL(PHY_ID_AQR112) }, { PHY_ID_MATCH_MODEL(PHY_ID_AQR412) }, { PHY_ID_MATCH_MODEL(PHY_ID_AQR113C) },
Add Aquantia AQR111 and AQR111B0 PHY ID. These PHY advertise 10G speed but actually supports up to 5G speed, hence some manual fixup is needed. The Aquantia AQR111B0 PHY is just a variant of the AQR111 with smaller chip size. Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- drivers/net/phy/aquantia/aquantia_main.c | 52 ++++++++++++++++++++++++ 1 file changed, 52 insertions(+)