Re: [PATCH v2 3/4] net: macb: Add pm runtime support

From: Claudiu.Beznea
Date: Mon Nov 26 2018 - 09:47:22 EST

Next message: Peter Zijlstra: "Re: [PATCH perf,bpf 0/5] reveal invisible bpf programs"
Previous message: Andrew Lunn: "Re: [PATCH v2 3/4] net: macb: Add pm runtime support"
In reply to: Andrew Lunn: "Re: [PATCH v2 3/4] net: macb: Add pm runtime support"
Next in thread: Harini Katakam: "[PATCH v2 4/4] net: macb: Add support for suspend/resume with full power down"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 26.11.2018 09:07, Harini Katakam wrote:
> From: Harini Katakam <harinik@xxxxxxxxxx>
>
> Add runtime pm functions and move clock handling there.
> Add runtime PM calls to mdio functions to allow for active mdio bus.
>
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xxxxxxxxxx>
> Signed-off-by: Harini Katakam <harinik@xxxxxxxxxx>
> ---
> v2 changes:
> Allow for mdio bus to be active
>
> Changes from RFC:
> Updated pm get sync/put sync calls.
> Removed unecessary clk up in mdio helpers.
>
> drivers/net/ethernet/cadence/macb_main.c | 121 ++++++++++++++++++++++++++-----
> 1 file changed, 101 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 32453d4..4b85ad7 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -37,6 +37,7 @@
> #include <linux/udp.h>
> #include <linux/tcp.h>
> #include <linux/iopoll.h>
> +#include <linux/pm_runtime.h>
> #include "macb.h"
>
> #define MACB_RX_BUFFER_SIZE 128
> @@ -80,6 +81,8 @@
> */
> #define MACB_HALT_TIMEOUT 1230
>
> +#define MACB_PM_TIMEOUT 100 /* ms */
> +
> #define MACB_MDIO_TIMEOUT 1000000 /* in usecs */
>
> /* DMA buffer descriptor might be different size
> @@ -335,6 +338,10 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
> int value;
> int err;
>
> + err = pm_runtime_get_sync(&bp->pdev->dev);
> + if (err < 0)
> + return err;
> +
> err = macb_mdio_wait_for_idle(bp);
> if (err < 0)
> return err;
> @@ -346,11 +353,17 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
> | MACB_BF(CODE, MACB_MAN_CODE)));
>
> err = macb_mdio_wait_for_idle(bp);
> - if (err < 0)
> + if (err < 0) {
> + pm_runtime_mark_last_busy(&bp->pdev->dev);
> + pm_runtime_put_autosuspend(&bp->pdev->dev);

This may look nicer with some sort of goto, if this will remain here.

> return err;
> + }
>
> value = MACB_BFEXT(DATA, macb_readl(bp, MAN));
>
> + pm_runtime_mark_last_busy(&bp->pdev->dev);
> + pm_runtime_put_autosuspend(&bp->pdev->dev);
> +
> return value;
> }
>
> @@ -360,10 +373,17 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
> struct macb *bp = bus->priv;
> int err;
>
> - err = macb_mdio_wait_for_idle(bp);
> + err = pm_runtime_get_sync(&bp->pdev->dev);
> if (err < 0)
> return err;
>
> + err = macb_mdio_wait_for_idle(bp);
> + if (err < 0) {
> + pm_runtime_mark_last_busy(&bp->pdev->dev);
> + pm_runtime_put_autosuspend(&bp->pdev->dev);

Ditto

> + return err;
> + }
> +
> macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_SOF)
> | MACB_BF(RW, MACB_MAN_WRITE)
> | MACB_BF(PHYA, mii_id)
> @@ -375,6 +395,9 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
> if (err < 0)
> return err;
>
> + pm_runtime_mark_last_busy(&bp->pdev->dev);
> + pm_runtime_put_autosuspend(&bp->pdev->dev);
> +
> return 0;
> }
>
> @@ -2386,12 +2409,18 @@ static int macb_open(struct net_device *dev)
>
> netdev_dbg(bp->dev, "open\n");
>
> + err = pm_runtime_get_sync(&bp->pdev->dev);
> + if (err < 0)
> + goto pm_exit;
> +
> /* carrier starts down */
> netif_carrier_off(dev);
>
> /* if the phy is not yet register, retry later*/
> - if (!dev->phydev)
> - return -EAGAIN;
> + if (!dev->phydev) {
> + err = -EAGAIN;
> + goto pm_exit;
> + }
>
> /* RX buffers initialization */
> macb_init_rx_buffer_size(bp, bufsz);
> @@ -2400,7 +2429,7 @@ static int macb_open(struct net_device *dev)
> if (err) {
> netdev_err(dev, "Unable to allocate DMA memory (error %d)\n",
> err);
> - return err;
> + goto pm_exit;
> }
>
> bp->macbgem_ops.mog_init_rings(bp);
> @@ -2417,6 +2446,11 @@ static int macb_open(struct net_device *dev)
> if (bp->ptp_info)
> bp->ptp_info->ptp_init(dev);
>
> +pm_exit:
> + if (err) {
> + pm_runtime_put_sync(&bp->pdev->dev);
> + return err;

You could get this of this "return err" and used it bellow, instead of
"return 0"

> + }
> return 0;

Here:
return err;

> }
>
> @@ -2445,6 +2479,8 @@ static int macb_close(struct net_device *dev)
> if (bp->ptp_info)
> bp->ptp_info->ptp_remove(dev);
>
> + pm_runtime_put(&bp->pdev->dev);
> +
> return 0;
> }
>
> @@ -4008,6 +4044,11 @@ static int macb_probe(struct platform_device *pdev)
> if (err)
> return err;
>
> + pm_runtime_set_autosuspend_delay(&pdev->dev, MACB_PM_TIMEOUT);
> + pm_runtime_use_autosuspend(&pdev->dev);
> + pm_runtime_get_noresume(&pdev->dev);
> + pm_runtime_set_active(&pdev->dev);
> + pm_runtime_enable(&pdev->dev);
> native_io = hw_is_native_io(mem);
>
> macb_probe_queues(mem, native_io, &queue_mask, &num_queues);
> @@ -4139,6 +4180,9 @@ static int macb_probe(struct platform_device *pdev)
> macb_is_gem(bp) ? "GEM" : "MACB", macb_readl(bp, MID),
> dev->base_addr, dev->irq, dev->dev_addr);
>
> + pm_runtime_mark_last_busy(&bp->pdev->dev);
> + pm_runtime_put_autosuspend(&bp->pdev->dev);
> +
> return 0;
>
> err_out_unregister_mdio:
> @@ -4158,6 +4202,9 @@ static int macb_probe(struct platform_device *pdev)
> clk_disable_unprepare(pclk);
> clk_disable_unprepare(rx_clk);
> clk_disable_unprepare(tsu_clk);
> + pm_runtime_disable(&pdev->dev);
> + pm_runtime_set_suspended(&pdev->dev);
> + pm_runtime_dont_use_autosuspend(&pdev->dev);
>
> return err;
> }
> @@ -4181,11 +4228,16 @@ static int macb_remove(struct platform_device *pdev)
> mdiobus_free(bp->mii_bus);
>
> unregister_netdev(dev);
> - clk_disable_unprepare(bp->tx_clk);
> - clk_disable_unprepare(bp->hclk);
> - clk_disable_unprepare(bp->pclk);
> - clk_disable_unprepare(bp->rx_clk);
> - clk_disable_unprepare(bp->tsu_clk);
> + pm_runtime_disable(&pdev->dev);
> + pm_runtime_dont_use_autosuspend(&pdev->dev);
> + if (!pm_runtime_suspended(&pdev->dev)) {
> + clk_disable_unprepare(bp->tx_clk);
> + clk_disable_unprepare(bp->hclk);
> + clk_disable_unprepare(bp->pclk);
> + clk_disable_unprepare(bp->rx_clk);
> + clk_disable_unprepare(bp->tsu_clk);
> + pm_runtime_set_suspended(&pdev->dev);
> + }
> of_node_put(bp->phy_node);
> free_netdev(dev);
> }
> @@ -4205,13 +4257,9 @@ static int __maybe_unused macb_suspend(struct device *dev)
> macb_writel(bp, IER, MACB_BIT(WOL));
> macb_writel(bp, WOL, MACB_BIT(MAG));
> enable_irq_wake(bp->queues[0].irq);
> - } else {
> - clk_disable_unprepare(bp->tx_clk);
> - clk_disable_unprepare(bp->hclk);
> - clk_disable_unprepare(bp->pclk);
> - clk_disable_unprepare(bp->rx_clk);
> }
> - clk_disable_unprepare(bp->tsu_clk);
> +
> + pm_runtime_force_suspend(dev);
>
> return 0;
> }
> @@ -4221,11 +4269,43 @@ static int __maybe_unused macb_resume(struct device *dev)
> struct net_device *netdev = dev_get_drvdata(dev);
> struct macb *bp = netdev_priv(netdev);
>
> + pm_runtime_force_resume(dev);
> +
> if (bp->wol & MACB_WOL_ENABLED) {
> macb_writel(bp, IDR, MACB_BIT(WOL));
> macb_writel(bp, WOL, 0);
> disable_irq_wake(bp->queues[0].irq);
> - } else {
> + }
> +
> + netif_device_attach(netdev);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused macb_runtime_suspend(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct net_device *netdev = platform_get_drvdata(pdev);
> + struct macb *bp = netdev_priv(netdev);
> +
> + if (!(device_may_wakeup(&bp->dev->dev))) {
> + clk_disable_unprepare(bp->tx_clk);
> + clk_disable_unprepare(bp->hclk);
> + clk_disable_unprepare(bp->pclk);
> + clk_disable_unprepare(bp->rx_clk);
> + }
> + clk_disable_unprepare(bp->tsu_clk);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused macb_runtime_resume(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct net_device *netdev = platform_get_drvdata(pdev);
> + struct macb *bp = netdev_priv(netdev);
> +
> + if (!(device_may_wakeup(&bp->dev->dev))) {
> clk_prepare_enable(bp->pclk);
> clk_prepare_enable(bp->hclk);
> clk_prepare_enable(bp->tx_clk);
> @@ -4233,12 +4313,13 @@ static int __maybe_unused macb_resume(struct device *dev)
> }
> clk_prepare_enable(bp->tsu_clk);
>
> - netif_device_attach(netdev);
> -
> return 0;
> }
>
> -static SIMPLE_DEV_PM_OPS(macb_pm_ops, macb_suspend, macb_resume);
> +static const struct dev_pm_ops macb_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(macb_suspend, macb_resume)
> + SET_RUNTIME_PM_OPS(macb_runtime_suspend, macb_runtime_resume, NULL)
> +};
>
> static struct platform_driver macb_driver = {
> .probe = macb_probe,
>

Next message: Peter Zijlstra: "Re: [PATCH perf,bpf 0/5] reveal invisible bpf programs"
Previous message: Andrew Lunn: "Re: [PATCH v2 3/4] net: macb: Add pm runtime support"
In reply to: Andrew Lunn: "Re: [PATCH v2 3/4] net: macb: Add pm runtime support"
Next in thread: Harini Katakam: "[PATCH v2 4/4] net: macb: Add support for suspend/resume with full power down"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]