[PATCH net-next RESEND 0/5] dwmac-sun8i cleanup and shutdown hook

These patches clean up some things I noticed while fixing suspend/resume
behavior. The first four are minor code improvements. The last one adds
a shutdown hook to minimize power consumption on boards without a PMIC.

Now that the fixes series is merged, I'm resending this series rebased
on top of net-next and with Chen-Yu's Reviewed-by tags.

Samuel Holland (5):
  net: stmmac: dwmac-sun8i: Return void from PHY unpower
  net: stmmac: dwmac-sun8i: Remove unnecessary PHY power check
  net: stmmac: dwmac-sun8i: Use reset_control_reset
  net: stmmac: dwmac-sun8i: Minor probe function cleanup
  net: stmmac: dwmac-sun8i: Add a shutdown callback

 .../net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 31 ++++++++++++-------
 1 file changed, 19 insertions(+), 12 deletions(-)

-- 
2.26.2

In commit e5c02cf54154 ("i2c: mv64xxx: Add runtime PM support"), error
pointers to optional clocks were replaced by NULL to simplify the resume
callback implementation. However, that commit missed that the IS_ERR
check in mv64xxx_of_config should be replaced with a NULL check. As a
result, the check always passes, even for an invalid device tree.

Fixes: e5c02cf54154 ("i2c: mv64xxx: Add runtime PM support")
Reported-by: Dan Carpenter <redacted>
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index b03c344323d1..c590d36b5fd1 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c

@@ -813,7 +813,7 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
 	 * need to know tclk in order to calculate bus clock
 	 * factors.
 	 */
-	if (IS_ERR(drv_data->clk)) {
+	if (!drv_data->clk) {
 		rc = -ENODEV;
 		goto out;
 	}

-- 
2.26.2

Use the appropriate function instead of reimplementing it,
and update the error message to match the code.

Reviewed-by: Chen-Yu Tsai <redacted>
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index 3c3d0b99d3e8..0e8d88417251 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c

@@ -806,11 +806,9 @@ static int sun8i_dwmac_power_internal_phy(struct stmmac_priv *priv)
 	/* Make sure the EPHY is properly reseted, as U-Boot may leave
 	 * it at deasserted state, and thus it may fail to reset EMAC.
 	 */
-	reset_control_assert(gmac->rst_ephy);
-
-	ret = reset_control_deassert(gmac->rst_ephy);
+	ret = reset_control_reset(gmac->rst_ephy);
 	if (ret) {
-		dev_err(priv->device, "Cannot deassert internal phy\n");
+		dev_err(priv->device, "Cannot reset internal PHY\n");
 		clk_disable_unprepare(gmac->ephy_clk);
 		return ret;
 	}

-- 
2.26.2

This is a deinitialization function that always returned zero, and that
return value was always ignored. Have it return void instead.

Reviewed-by: Chen-Yu Tsai <redacted>
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index a5e0eff4a387..8e505019adf8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c

@@ -820,15 +820,14 @@ static int sun8i_dwmac_power_internal_phy(struct stmmac_priv *priv)
 	return 0;
 }
 
-static int sun8i_dwmac_unpower_internal_phy(struct sunxi_priv_data *gmac)
+static void sun8i_dwmac_unpower_internal_phy(struct sunxi_priv_data *gmac)
 {
 	if (!gmac->internal_phy_powered)
-		return 0;
+		return;
 
 	clk_disable_unprepare(gmac->ephy_clk);
 	reset_control_assert(gmac->rst_ephy);
 	gmac->internal_phy_powered = false;
-	return 0;
 }
 
 /* MDIO multiplexing switch function

-- 
2.26.2

sun8i_dwmac_unpower_internal_phy already checks if the PHY is powered,
so there is no need to do it again here.

Reviewed-by: Chen-Yu Tsai <redacted>
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index 8e505019adf8..3c3d0b99d3e8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c

@@ -1018,10 +1018,8 @@ static void sun8i_dwmac_exit(struct platform_device *pdev, void *priv)
 {
 	struct sunxi_priv_data *gmac = priv;
 
-	if (gmac->variant->soc_has_internal_phy) {
-		if (gmac->internal_phy_powered)
-			sun8i_dwmac_unpower_internal_phy(gmac);
-	}
+	if (gmac->variant->soc_has_internal_phy)
+		sun8i_dwmac_unpower_internal_phy(gmac);
 
 	clk_disable_unprepare(gmac->tx_clk);
 

-- 
2.26.2

Adjust the spacing and use an explicit "return 0" in the success path
to make the function easier to parse.

Reviewed-by: Chen-Yu Tsai <redacted>
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index 0e8d88417251..4638d4203af5 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c

@@ -1227,6 +1227,7 @@ static int sun8i_dwmac_probe(struct platform_device *pdev)
 
 	ndev = dev_get_drvdata(&pdev->dev);
 	priv = netdev_priv(ndev);
+
 	/* The mux must be registered after parent MDIO
 	 * so after stmmac_dvr_probe()
 	 */

@@ -1245,7 +1246,8 @@ static int sun8i_dwmac_probe(struct platform_device *pdev)
 			goto dwmac_remove;
 	}
 
-	return ret;
+	return 0;
+
 dwmac_mux:
 	reset_control_put(gmac->rst_ephy);
 	clk_put(gmac->ephy_clk);

-- 
2.26.2

The Ethernet MAC and PHY are usually major consumers of power on boards
which may not be able to fully power off (those with no PMIC). Powering
down the MAC and internal PHY saves power while these boards are "off".

Reviewed-by: Chen-Yu Tsai <redacted>
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index 4638d4203af5..926e8d5e8963 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c

@@ -1282,6 +1282,15 @@ static int sun8i_dwmac_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static void sun8i_dwmac_shutdown(struct platform_device *pdev)
+{
+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct stmmac_priv *priv = netdev_priv(ndev);
+	struct sunxi_priv_data *gmac = priv->plat->bsp_priv;
+
+	sun8i_dwmac_exit(pdev, gmac);
+}
+
 static const struct of_device_id sun8i_dwmac_match[] = {
 	{ .compatible = "allwinner,sun8i-h3-emac",
 		.data = &emac_variant_h3 },

@@ -1302,6 +1311,7 @@ MODULE_DEVICE_TABLE(of, sun8i_dwmac_match);
 static struct platform_driver sun8i_dwmac_driver = {
 	.probe  = sun8i_dwmac_probe,
 	.remove = sun8i_dwmac_remove,
+	.shutdown = sun8i_dwmac_shutdown,
 	.driver = {
 		.name           = "dwmac-sun8i",
 		.pm		= &stmmac_pltfr_pm_ops,

-- 
2.26.2

On 2/8/21 12:28 AM, Samuel Holland wrote:

In commit e5c02cf54154 ("i2c: mv64xxx: Add runtime PM support"), error
pointers to optional clocks were replaced by NULL to simplify the resume
callback implementation. However, that commit missed that the IS_ERR
check in mv64xxx_of_config should be replaced with a NULL check. As a
result, the check always passes, even for an invalid device tree.

Sorry, please ignore this unrelated patch. I accidentally copied it to
the wrong directory before sending this series.

Samuel

On Mon, Feb 08, 2021 at 12:31:34AM -0600, Samuel Holland wrote:

On 2/8/21 12:28 AM, Samuel Holland wrote:

quoted

In commit e5c02cf54154 ("i2c: mv64xxx: Add runtime PM support"), error
pointers to optional clocks were replaced by NULL to simplify the resume
callback implementation. However, that commit missed that the IS_ERR
check in mv64xxx_of_config should be replaced with a NULL check. As a
result, the check always passes, even for an invalid device tree.

Sorry, please ignore this unrelated patch. I accidentally copied it to
the wrong directory before sending this series.

Hi Samuel

This patch looks correct. But i don't see it in i2c/for-next, where as
e5c02cf54154 is. I just want to make sure it does not get lost...

	     Andrew

On Sun, Feb 7, 2021 at 10:32 PM Samuel Holland [off-list ref] wrote:

I'm assuming you have exclusive access to the phy and this isn't a
shared line? Just wanting to confirm since the function call has the
following comment in the header for the documentation.

 * Consumers must not use reset_control_(de)assert on shared reset lines when
 * reset_control_reset has been used.
 *

If that is the case it might not hurt to add some documentation to
your call to reset_control_reset here explaining that it is safe to do
so since you have exclusive access.

On Mon, 8 Feb 2021 00:31:34 -0600 Samuel Holland wrote:

On 2/8/21 12:28 AM, Samuel Holland wrote:

quoted

In commit e5c02cf54154 ("i2c: mv64xxx: Add runtime PM support"), error
pointers to optional clocks were replaced by NULL to simplify the resume
callback implementation. However, that commit missed that the IS_ERR
check in mv64xxx_of_config should be replaced with a NULL check. As a
result, the check always passes, even for an invalid device tree.  

Sorry, please ignore this unrelated patch. I accidentally copied it to
the wrong directory before sending this series.

Unfortunately patchwork decided to take this patch in instead of the
real 1/5 patch. Please make a clean repost even if there are no review
comments to address.

On 2/8/21 7:20 AM, Andrew Lunn wrote:

On Mon, Feb 08, 2021 at 12:31:34AM -0600, Samuel Holland wrote:

quoted

On 2/8/21 12:28 AM, Samuel Holland wrote:

quoted

In commit e5c02cf54154 ("i2c: mv64xxx: Add runtime PM support"), error
pointers to optional clocks were replaced by NULL to simplify the resume
callback implementation. However, that commit missed that the IS_ERR
check in mv64xxx_of_config should be replaced with a NULL check. As a
result, the check always passes, even for an invalid device tree.

Sorry, please ignore this unrelated patch. I accidentally copied it to
the wrong directory before sending this series.

Hi Samuel

This patch looks correct. But i don't see it in i2c/for-next, where as
e5c02cf54154 is. I just want to make sure it does not get lost...

Thanks for the concern. I had already sent it separately[0], to the
appropriate maintainers, so this submission was a duplicate.

Cheers,
Samuel

[0]:
https://lore.kernel.org/lkml/20210208061922.10073-1-samuel@sholland.org/

	     Andrew

On 2/8/21 10:29 AM, Alexander Duyck wrote:

On Sun, Feb 7, 2021 at 10:32 PM Samuel Holland [off-list ref] wrote:

quoted

Use the appropriate function instead of reimplementing it,
and update the error message to match the code.

Reviewed-by: Chen-Yu Tsai <redacted>
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index 3c3d0b99d3e8..0e8d88417251 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c

@@ -806,11 +806,9 @@ static int sun8i_dwmac_power_internal_phy(struct stmmac_priv *priv)
        /* Make sure the EPHY is properly reseted, as U-Boot may leave
         * it at deasserted state, and thus it may fail to reset EMAC.
         */
-       reset_control_assert(gmac->rst_ephy);
-
-       ret = reset_control_deassert(gmac->rst_ephy);
+       ret = reset_control_reset(gmac->rst_ephy);
        if (ret) {
-               dev_err(priv->device, "Cannot deassert internal phy\n");
+               dev_err(priv->device, "Cannot reset internal PHY\n");
                clk_disable_unprepare(gmac->ephy_clk);
                return ret;
        }

I'm assuming you have exclusive access to the phy and this isn't a
shared line? Just wanting to confirm since the function call has the
following comment in the header for the documentation.

Yes, this driver has exclusive access:

	gmac->rst_ephy = of_reset_control_get_exclusive(iphynode, NULL);

And this is a reset line for the Ethernet PHY inside the SoC, that as
far as I can tell is not shared with anything else.

 * Consumers must not use reset_control_(de)assert on shared reset lines when
 * reset_control_reset has been used.
 *

If that is the case it might not hurt to add some documentation to
your call to reset_control_reset here explaining that it is safe to do
so since you have exclusive access.

I can expand the comment above this line for v2.

Cheers,
Samuel