diff mbox series

[net-next,5/8] net: phy: aquantia: wait for FW reset before checking the vendor ID

Message ID 20240619184550.34524-6-brgl@bgdev.pl (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: support 2.5G ethernet in dwmac-qcom-ethqos | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/apply fail Patch does not apply to net-next-0

Commit Message

Bartosz Golaszewski June 19, 2024, 6:45 p.m. UTC 
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Checking the firmware register before it boots makes no sense, it will
report 0 even if FW is loaded. Always wait for FW to boot before
continuing.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/net/phy/aquantia/aquantia.h          | 1 +
 drivers/net/phy/aquantia/aquantia_firmware.c | 4 ++++
 drivers/net/phy/aquantia/aquantia_main.c     | 6 +++---
 3 files changed, 8 insertions(+), 3 deletions(-)

Comments

Andrew Lunn June 19, 2024, 7:27 p.m. UTC | #1 
On Wed, Jun 19, 2024 at 08:45:46PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Checking the firmware register before it boots makes no sense, it will
> report 0 even if FW is loaded. Always wait for FW to boot before
> continuing.

Please split this patch up. One patch which renames the method to the
more generic aqr_ since it is used by more than aqr107. Then add the
new use of it.

Is this actually a fix? What happens to the firmware if you try to
download it while it is still booting? Or do you end up downloading
firmware when it is not actually needed? Please expand the commit
message.

    Andrew

---
pw-bot: cr
Bartosz Golaszewski June 20, 2024, 7:24 a.m. UTC | #2 
On Wed, Jun 19, 2024 at 9:27 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Wed, Jun 19, 2024 at 08:45:46PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Checking the firmware register before it boots makes no sense, it will
> > report 0 even if FW is loaded. Always wait for FW to boot before
> > continuing.
>
> Please split this patch up. One patch which renames the method to the
> more generic aqr_ since it is used by more than aqr107. Then add the
> new use of it.
>

Will do.

> Is this actually a fix? What happens to the firmware if you try to
> download it while it is still booting? Or do you end up downloading
> firmware when it is not actually needed? Please expand the commit
> message.
>

It says '0' and the driver tries to load it from nvmem, then the
filesystem and bails-out after these two fail. I'll extend the commit
message.

Bart

>     Andrew
>
> ---
> pw-bot: cr
diff mbox series

Patch

diff --git a/drivers/net/phy/aquantia/aquantia.h b/drivers/net/phy/aquantia/aquantia.h
index b8502793962e..2465345081f8 100644
--- a/drivers/net/phy/aquantia/aquantia.h
+++ b/drivers/net/phy/aquantia/aquantia.h
@@ -201,5 +201,6 @@  int aqr_phy_led_hw_control_set(struct phy_device *phydev, u8 index,
 int aqr_phy_led_active_low_set(struct phy_device *phydev, int index, bool enable);
 int aqr_phy_led_polarity_set(struct phy_device *phydev, int index,
 			     unsigned long modes);
+int aqr_wait_reset_complete(struct phy_device *phydev);
 
 #endif /* AQUANTIA_H */
diff --git a/drivers/net/phy/aquantia/aquantia_firmware.c b/drivers/net/phy/aquantia/aquantia_firmware.c
index 0c9640ef153b..524627a36c6f 100644
--- a/drivers/net/phy/aquantia/aquantia_firmware.c
+++ b/drivers/net/phy/aquantia/aquantia_firmware.c
@@ -353,6 +353,10 @@  int aqr_firmware_load(struct phy_device *phydev)
 {
 	int ret;
 
+	ret = aqr_wait_reset_complete(phydev);
+	if (ret)
+		return ret;
+
 	/* Check if the firmware is not already loaded by pooling
 	 * the current version returned by the PHY. If 0 is returned,
 	 * no firmware is loaded.
diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
index 11da460698b0..eab779db225c 100644
--- a/drivers/net/phy/aquantia/aquantia_main.c
+++ b/drivers/net/phy/aquantia/aquantia_main.c
@@ -441,7 +441,7 @@  static int aqr107_set_tunable(struct phy_device *phydev,
  * The chip also provides a "reset completed" bit, but it's cleared after
  * read. Therefore function would time out if called again.
  */
-static int aqr107_wait_reset_complete(struct phy_device *phydev)
+int aqr_wait_reset_complete(struct phy_device *phydev)
 {
 	int val;
 
@@ -494,7 +494,7 @@  static int aqr107_config_init(struct phy_device *phydev)
 	WARN(phydev->interface == PHY_INTERFACE_MODE_XGMII,
 	     "Your devicetree is out of date, please update it. The AQR107 family doesn't support XGMII, maybe you mean USXGMII.\n");
 
-	ret = aqr107_wait_reset_complete(phydev);
+	ret = aqr_wait_reset_complete(phydev);
 	if (!ret)
 		aqr107_chip_info(phydev);
 
@@ -522,7 +522,7 @@  static int aqcs109_config_init(struct phy_device *phydev)
 	    phydev->interface != PHY_INTERFACE_MODE_2500BASEX)
 		return -ENODEV;
 
-	ret = aqr107_wait_reset_complete(phydev);
+	ret = aqr_wait_reset_complete(phydev);
 	if (!ret)
 		aqr107_chip_info(phydev);