| Message ID | baf987c11c0242c2bf87b81f9396b09d@BN1BFFO11FD018.protection.gbl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2015-01-12 at 08:34PM +0530, Kedareswara rao Appana wrote: > Instead of enabling/disabling clocks at several locations in the driver, > Use the runtime_pm framework. This consolidates the actions for runtime PM > In the appropriate callbacks and makes the driver more readable and mantainable. > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com> > --- > Changes for v5: > - Updated with the review comments. > Updated the remove fuction to use runtime_pm. > Chnages for v4: > - Updated with the review comments. > Changes for v3: > - Converted the driver to use runtime_pm. > Changes for v2: > - Removed the struct platform_device* from suspend/resume > as suggest by Lothar. > > drivers/net/can/xilinx_can.c | 157 ++++++++++++++++++++++++++++------------- > 1 files changed, 107 insertions(+), 50 deletions(-) [..] > +static int __maybe_unused xcan_runtime_resume(struct device *dev) > { > - struct platform_device *pdev = dev_get_drvdata(dev); > - struct net_device *ndev = platform_get_drvdata(pdev); > + struct net_device *ndev = dev_get_drvdata(dev); > struct xcan_priv *priv = netdev_priv(ndev); > int ret; > + u32 isr, status; > > ret = clk_enable(priv->bus_clk); > if (ret) { > @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev) > ret = clk_enable(priv->can_clk); > if (ret) { > dev_err(dev, "Cannot enable clock.\n"); > - clk_disable_unprepare(priv->bus_clk); > + clk_disable(priv->bus_clk); [...] > @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev) > { > struct net_device *ndev = platform_get_drvdata(pdev); > struct xcan_priv *priv = netdev_priv(ndev); > + int ret; > + > + ret = pm_runtime_get_sync(&pdev->dev); > + if (ret < 0) { > + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n", > + __func__, ret); > + return ret; > + } > > if (set_reset_mode(ndev) < 0) > netdev_err(ndev, "mode resetting failed!\n"); > > unregister_candev(ndev); > + pm_runtime_disable(&pdev->dev); > netif_napi_del(&priv->napi); > + clk_disable_unprepare(priv->bus_clk); > + clk_disable_unprepare(priv->can_clk); Shouldn't pretty much all these occurrences of clk_disable/enable disappear? This should all be handled by the runtime_pm framework now. Sören
On 01/12/2015 04:04 PM, Kedareswara rao Appana wrote: > Instead of enabling/disabling clocks at several locations in the driver, > Use the runtime_pm framework. This consolidates the actions for runtime PM > In the appropriate callbacks and makes the driver more readable and mantainable. > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com> FTBFS: > drivers/net/can/xilinx_can.c:1064:9: error: undefined identifier 'SET_PM_RUNTIME_PM_OPS' > CC [M] drivers/net/can/xilinx_can.o > drivers/net/can/xilinx_can.c:1064:2: error: implicit declaration of function ‘SET_PM_RUNTIME_PM_OPS’ [-Werror=implicit-function-declaration] > SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend, xcan_runtime_resume, NULL) > ^ > drivers/net/can/xilinx_can.c:1065:1: error: initializer element is not constant > }; > ^ > drivers/net/can/xilinx_can.c:1065:1: error: (near initialization for ‘xcan_dev_pm_ops.prepare’) Have a look at commit 40bd62c6194bdee1bc6652b3b0aa28e34883f603: More comments inline. Looks quite good now. > PM: Remove the SET_PM_RUNTIME_PM_OPS() macro > > There're now no users left of the SET_PM_RUNTIME_PM_OPS() macro, since > all have converted to use the SET_RUNTIME_PM_OPS() macro instead, so > let's remove it. > --- > Changes for v5: > - Updated with the review comments. > Updated the remove fuction to use runtime_pm. > Chnages for v4: > - Updated with the review comments. > Changes for v3: > - Converted the driver to use runtime_pm. > Changes for v2: > - Removed the struct platform_device* from suspend/resume > as suggest by Lothar. > > drivers/net/can/xilinx_can.c | 157 ++++++++++++++++++++++++++++------------- > 1 files changed, 107 insertions(+), 50 deletions(-) > > diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c > index 6c67643..67aef00 100644 > --- a/drivers/net/can/xilinx_can.c > +++ b/drivers/net/can/xilinx_can.c > @@ -32,6 +32,7 @@ > #include <linux/can/dev.h> > #include <linux/can/error.h> > #include <linux/can/led.h> > +#include <linux/pm_runtime.h> > > #define DRIVER_NAME "xilinx_can" > > @@ -138,7 +139,7 @@ struct xcan_priv { > u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg); > void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg, > u32 val); > - struct net_device *dev; > + struct device *dev; > void __iomem *reg_base; > unsigned long irq_flags; > struct clk *bus_clk; > @@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev) > struct xcan_priv *priv = netdev_priv(ndev); > int ret; > > + ret = pm_runtime_get_sync(priv->dev); > + if (ret < 0) { > + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n", > + __func__, ret); > + return ret; > + } > + > ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags, > ndev->name, ndev); > if (ret < 0) { > @@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev) > goto err; > } > > - ret = clk_prepare_enable(priv->can_clk); > - if (ret) { > - netdev_err(ndev, "unable to enable device clock\n"); > - goto err_irq; > - } > - > - ret = clk_prepare_enable(priv->bus_clk); > - if (ret) { > - netdev_err(ndev, "unable to enable bus clock\n"); > - goto err_can_clk; > - } > - > /* Set chip into reset mode */ > ret = set_reset_mode(ndev); > if (ret < 0) { > netdev_err(ndev, "mode resetting failed!\n"); > - goto err_bus_clk; > + goto err_irq; > } > > /* Common open */ > ret = open_candev(ndev); > if (ret) > - goto err_bus_clk; > + goto err_irq; > > ret = xcan_chip_start(ndev); > if (ret < 0) { > @@ -887,13 +883,11 @@ static int xcan_open(struct net_device *ndev) > > err_candev: > close_candev(ndev); > -err_bus_clk: > - clk_disable_unprepare(priv->bus_clk); > -err_can_clk: > - clk_disable_unprepare(priv->can_clk); > err_irq: > free_irq(ndev->irq, ndev); > err: > + pm_runtime_put(priv->dev); > + > return ret; > } > > @@ -910,12 +904,11 @@ static int xcan_close(struct net_device *ndev) > netif_stop_queue(ndev); > napi_disable(&priv->napi); > xcan_chip_stop(ndev); > - clk_disable_unprepare(priv->bus_clk); > - clk_disable_unprepare(priv->can_clk); > free_irq(ndev->irq, ndev); > close_candev(ndev); > > can_led_event(ndev, CAN_LED_EVENT_STOP); > + pm_runtime_put(priv->dev); > > return 0; > } > @@ -934,27 +927,20 @@ static int xcan_get_berr_counter(const struct net_device *ndev, > struct xcan_priv *priv = netdev_priv(ndev); > int ret; > > - ret = clk_prepare_enable(priv->can_clk); > - if (ret) > - goto err; > - > - ret = clk_prepare_enable(priv->bus_clk); > - if (ret) > - goto err_clk; > + ret = pm_runtime_get_sync(priv->dev); > + if (ret < 0) { > + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n", > + __func__, ret); > + return ret; > + } > > bec->txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) & XCAN_ECR_TEC_MASK; > bec->rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) & > XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT); > > - clk_disable_unprepare(priv->bus_clk); > - clk_disable_unprepare(priv->can_clk); > + pm_runtime_put(priv->dev); > > return 0; > - > -err_clk: > - clk_disable_unprepare(priv->can_clk); > -err: > - return ret; > } > > > @@ -967,15 +953,45 @@ static const struct net_device_ops xcan_netdev_ops = { > > /** > * xcan_suspend - Suspend method for the driver > - * @dev: Address of the platform_device structure > + * @dev: Address of the device structure > * > * Put the driver into low power mode. > - * Return: 0 always > + * Return: 0 on success and failure value on error > */ > static int __maybe_unused xcan_suspend(struct device *dev) > { > - struct platform_device *pdev = dev_get_drvdata(dev); > - struct net_device *ndev = platform_get_drvdata(pdev); > + if (!device_may_wakeup(dev)) > + return pm_runtime_force_suspend(dev); > + > + return 0; > +} > + > +/** > + * xcan_resume - Resume from suspend > + * @dev: Address of the device structure > + * > + * Resume operation after suspend. > + * Return: 0 on success and failure value on error > + */ > +static int __maybe_unused xcan_resume(struct device *dev) > +{ > + if (!device_may_wakeup(dev)) > + return pm_runtime_force_resume(dev); > + > + return 0; > + > +} > + > +/** > + * xcan_runtime_suspend - Runtime suspend method for the driver > + * @dev: Address of the device structure > + * > + * Put the driver into low power mode. > + * Return: 0 always > + */ > +static int __maybe_unused xcan_runtime_suspend(struct device *dev) > +{ > + struct net_device *ndev = dev_get_drvdata(dev); > struct xcan_priv *priv = netdev_priv(ndev); > > if (netif_running(ndev)) { > @@ -993,18 +1009,18 @@ static int __maybe_unused xcan_suspend(struct device *dev) > } > > /** > - * xcan_resume - Resume from suspend > - * @dev: Address of the platformdevice structure > + * xcan_runtime_resume - Runtime resume from suspend > + * @dev: Address of the device structure > * > * Resume operation after suspend. > * Return: 0 on success and failure value on error > */ > -static int __maybe_unused xcan_resume(struct device *dev) > +static int __maybe_unused xcan_runtime_resume(struct device *dev) > { > - struct platform_device *pdev = dev_get_drvdata(dev); > - struct net_device *ndev = platform_get_drvdata(pdev); > + struct net_device *ndev = dev_get_drvdata(dev); > struct xcan_priv *priv = netdev_priv(ndev); > int ret; > + u32 isr, status; > > ret = clk_enable(priv->bus_clk); > if (ret) { > @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev) > ret = clk_enable(priv->can_clk); > if (ret) { > dev_err(dev, "Cannot enable clock.\n"); > - clk_disable_unprepare(priv->bus_clk); > + clk_disable(priv->bus_clk); > return ret; > } > > priv->write_reg(priv, XCAN_MSR_OFFSET, 0); > priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK); > - priv->can.state = CAN_STATE_ERROR_ACTIVE; > + isr = priv->read_reg(priv, XCAN_ISR_OFFSET); > + status = priv->read_reg(priv, XCAN_SR_OFFSET); > > if (netif_running(ndev)) { > + if (isr & XCAN_IXR_BSOFF_MASK) { > + priv->can.state = CAN_STATE_BUS_OFF; > + priv->write_reg(priv, XCAN_SRR_OFFSET, > + XCAN_SRR_RESET_MASK); > + } else if ((status & XCAN_SR_ESTAT_MASK) == > + XCAN_SR_ESTAT_MASK) { > + priv->can.state = CAN_STATE_ERROR_PASSIVE; > + } else if (status & XCAN_SR_ERRWRN_MASK) { > + priv->can.state = CAN_STATE_ERROR_WARNING; > + } else { > + priv->can.state = CAN_STATE_ERROR_ACTIVE; > + } > netif_device_attach(ndev); > netif_start_queue(ndev); > } > @@ -1030,7 +1059,10 @@ static int __maybe_unused xcan_resume(struct device *dev) > return 0; > } > > -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend, xcan_resume); > +static const struct dev_pm_ops xcan_dev_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume) > + SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend, xcan_runtime_resume, NULL) > +}; > > /** > * xcan_probe - Platform registration call > @@ -1071,7 +1103,7 @@ static int xcan_probe(struct platform_device *pdev) > return -ENOMEM; > > priv = netdev_priv(ndev); > - priv->dev = ndev; > + priv->dev = &pdev->dev; > priv->can.bittiming_const = &xcan_bittiming_const; > priv->can.do_set_mode = xcan_do_set_mode; > priv->can.do_get_berr_counter = xcan_get_berr_counter; > @@ -1137,6 +1169,16 @@ static int xcan_probe(struct platform_device *pdev) > > netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max); > > + pm_runtime_set_active(&pdev->dev); > + pm_runtime_irq_safe(&pdev->dev); > + pm_runtime_enable(&pdev->dev); > + ret = pm_runtime_get_sync(&pdev->dev); > + if (ret < 0) { > + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n", > + __func__, ret); > + goto err_pmdisable; after err_pmdisable is a runtime_put(), your cleanup is broken. > + } > + > ret = register_candev(ndev); > if (ret) { > dev_err(&pdev->dev, "fail to register failed (err=%d)\n", ret); > goto err_unprepare_disable_busclk; I think you have to adjust the jump label. > } > > devm_can_led_init(ndev); > > pm_runtime_put(&pdev->dev); > @@ -1144,15 +1186,19 @@ static int xcan_probe(struct platform_device *pdev) > } > > devm_can_led_init(ndev); > - clk_disable_unprepare(priv->bus_clk); > - clk_disable_unprepare(priv->can_clk); > + > + pm_runtime_put(&pdev->dev); > + > netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo depth:%d\n", > priv->reg_base, ndev->irq, priv->can.clock.freq, > priv->tx_max); > > return 0; > > +err_pmdisable: > + pm_runtime_disable(&pdev->dev); > err_unprepare_disable_busclk: > + pm_runtime_put(priv->dev); This cleanup is not correct. > clk_disable_unprepare(priv->bus_clk); > err_unprepare_disable_dev: > clk_disable_unprepare(priv->can_clk); > @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev) > { > struct net_device *ndev = platform_get_drvdata(pdev); > struct xcan_priv *priv = netdev_priv(ndev); > + int ret; > + > + ret = pm_runtime_get_sync(&pdev->dev); > + if (ret < 0) { > + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n", > + __func__, ret); > + return ret; > + } > > if (set_reset_mode(ndev) < 0) > netdev_err(ndev, "mode resetting failed!\n"); > > unregister_candev(ndev); > + pm_runtime_disable(&pdev->dev); > netif_napi_del(&priv->napi); > + clk_disable_unprepare(priv->bus_clk); > + clk_disable_unprepare(priv->can_clk); > free_candev(ndev); > > return 0; > Marc
On 01/12/2015 07:45 PM, Sören Brinkmann wrote: > On Mon, 2015-01-12 at 08:34PM +0530, Kedareswara rao Appana wrote: >> Instead of enabling/disabling clocks at several locations in the driver, >> Use the runtime_pm framework. This consolidates the actions for runtime PM >> In the appropriate callbacks and makes the driver more readable and mantainable. >> >> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> >> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com> >> --- >> Changes for v5: >> - Updated with the review comments. >> Updated the remove fuction to use runtime_pm. >> Chnages for v4: >> - Updated with the review comments. >> Changes for v3: >> - Converted the driver to use runtime_pm. >> Changes for v2: >> - Removed the struct platform_device* from suspend/resume >> as suggest by Lothar. >> >> drivers/net/can/xilinx_can.c | 157 ++++++++++++++++++++++++++++------------- >> 1 files changed, 107 insertions(+), 50 deletions(-) > [..] >> +static int __maybe_unused xcan_runtime_resume(struct device *dev) >> { >> - struct platform_device *pdev = dev_get_drvdata(dev); >> - struct net_device *ndev = platform_get_drvdata(pdev); >> + struct net_device *ndev = dev_get_drvdata(dev); >> struct xcan_priv *priv = netdev_priv(ndev); >> int ret; >> + u32 isr, status; >> >> ret = clk_enable(priv->bus_clk); >> if (ret) { >> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev) >> ret = clk_enable(priv->can_clk); >> if (ret) { >> dev_err(dev, "Cannot enable clock.\n"); >> - clk_disable_unprepare(priv->bus_clk); >> + clk_disable(priv->bus_clk); > [...] >> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev) >> { >> struct net_device *ndev = platform_get_drvdata(pdev); >> struct xcan_priv *priv = netdev_priv(ndev); >> + int ret; >> + >> + ret = pm_runtime_get_sync(&pdev->dev); >> + if (ret < 0) { >> + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n", >> + __func__, ret); >> + return ret; >> + } >> >> if (set_reset_mode(ndev) < 0) >> netdev_err(ndev, "mode resetting failed!\n"); >> >> unregister_candev(ndev); >> + pm_runtime_disable(&pdev->dev); >> netif_napi_del(&priv->napi); >> + clk_disable_unprepare(priv->bus_clk); >> + clk_disable_unprepare(priv->can_clk); > > Shouldn't pretty much all these occurrences of clk_disable/enable > disappear? This should all be handled by the runtime_pm framework now. We have: - clk_prepare_enable() in probe - clk_disable_unprepare() in remove - clk_enable() in runtime_resume - clk_disable() in runtime_suspend Which is, as far as I understand the right way to do it. Maybe Kedareswara can post the clock debug output again with this patch iteration. Have I missed something? regards, Marc
On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote: > On 01/12/2015 07:45 PM, Sören Brinkmann wrote: > > On Mon, 2015-01-12 at 08:34PM +0530, Kedareswara rao Appana wrote: > >> Instead of enabling/disabling clocks at several locations in the driver, > >> Use the runtime_pm framework. This consolidates the actions for runtime PM > >> In the appropriate callbacks and makes the driver more readable and mantainable. > >> > >> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > >> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com> > >> --- > >> Changes for v5: > >> - Updated with the review comments. > >> Updated the remove fuction to use runtime_pm. > >> Chnages for v4: > >> - Updated with the review comments. > >> Changes for v3: > >> - Converted the driver to use runtime_pm. > >> Changes for v2: > >> - Removed the struct platform_device* from suspend/resume > >> as suggest by Lothar. > >> > >> drivers/net/can/xilinx_can.c | 157 ++++++++++++++++++++++++++++------------- > >> 1 files changed, 107 insertions(+), 50 deletions(-) > > [..] > >> +static int __maybe_unused xcan_runtime_resume(struct device *dev) > >> { > >> - struct platform_device *pdev = dev_get_drvdata(dev); > >> - struct net_device *ndev = platform_get_drvdata(pdev); > >> + struct net_device *ndev = dev_get_drvdata(dev); > >> struct xcan_priv *priv = netdev_priv(ndev); > >> int ret; > >> + u32 isr, status; > >> > >> ret = clk_enable(priv->bus_clk); > >> if (ret) { > >> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev) > >> ret = clk_enable(priv->can_clk); > >> if (ret) { > >> dev_err(dev, "Cannot enable clock.\n"); > >> - clk_disable_unprepare(priv->bus_clk); > >> + clk_disable(priv->bus_clk); > > [...] > >> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev) > >> { > >> struct net_device *ndev = platform_get_drvdata(pdev); > >> struct xcan_priv *priv = netdev_priv(ndev); > >> + int ret; > >> + > >> + ret = pm_runtime_get_sync(&pdev->dev); > >> + if (ret < 0) { > >> + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n", > >> + __func__, ret); > >> + return ret; > >> + } > >> > >> if (set_reset_mode(ndev) < 0) > >> netdev_err(ndev, "mode resetting failed!\n"); > >> > >> unregister_candev(ndev); > >> + pm_runtime_disable(&pdev->dev); > >> netif_napi_del(&priv->napi); > >> + clk_disable_unprepare(priv->bus_clk); > >> + clk_disable_unprepare(priv->can_clk); > > > > Shouldn't pretty much all these occurrences of clk_disable/enable > > disappear? This should all be handled by the runtime_pm framework now. > > We have: > - clk_prepare_enable() in probe This should become something like pm_runtime_get_sync(), shouldn't it? > - clk_disable_unprepare() in remove pm_runtime_put() > - clk_enable() in runtime_resume > - clk_disable() in runtime_suspend These are the ones needed. The above makes me suspect that the clocks are always on, regardless of the runtime suspend state since they are enabled in probe and disabled in remove, is that right? Ideally, the usage in probe and remove should be migrated to runtime_pm and clocks should really only be running when needed and not throughout the whole lifetime of the driver. Sören
On 01/13/2015 06:08 PM, Sören Brinkmann wrote: > On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote: >> On 01/12/2015 07:45 PM, Sören Brinkmann wrote: >>> On Mon, 2015-01-12 at 08:34PM +0530, Kedareswara rao Appana wrote: >>>> Instead of enabling/disabling clocks at several locations in the driver, >>>> Use the runtime_pm framework. This consolidates the actions for runtime PM >>>> In the appropriate callbacks and makes the driver more readable and mantainable. >>>> >>>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> >>>> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com> >>>> --- >>>> Changes for v5: >>>> - Updated with the review comments. >>>> Updated the remove fuction to use runtime_pm. >>>> Chnages for v4: >>>> - Updated with the review comments. >>>> Changes for v3: >>>> - Converted the driver to use runtime_pm. >>>> Changes for v2: >>>> - Removed the struct platform_device* from suspend/resume >>>> as suggest by Lothar. >>>> >>>> drivers/net/can/xilinx_can.c | 157 ++++++++++++++++++++++++++++------------- >>>> 1 files changed, 107 insertions(+), 50 deletions(-) >>> [..] >>>> +static int __maybe_unused xcan_runtime_resume(struct device *dev) >>>> { >>>> - struct platform_device *pdev = dev_get_drvdata(dev); >>>> - struct net_device *ndev = platform_get_drvdata(pdev); >>>> + struct net_device *ndev = dev_get_drvdata(dev); >>>> struct xcan_priv *priv = netdev_priv(ndev); >>>> int ret; >>>> + u32 isr, status; >>>> >>>> ret = clk_enable(priv->bus_clk); >>>> if (ret) { >>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev) >>>> ret = clk_enable(priv->can_clk); >>>> if (ret) { >>>> dev_err(dev, "Cannot enable clock.\n"); >>>> - clk_disable_unprepare(priv->bus_clk); >>>> + clk_disable(priv->bus_clk); >>> [...] >>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev) >>>> { >>>> struct net_device *ndev = platform_get_drvdata(pdev); >>>> struct xcan_priv *priv = netdev_priv(ndev); >>>> + int ret; >>>> + >>>> + ret = pm_runtime_get_sync(&pdev->dev); >>>> + if (ret < 0) { >>>> + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n", >>>> + __func__, ret); >>>> + return ret; >>>> + } >>>> >>>> if (set_reset_mode(ndev) < 0) >>>> netdev_err(ndev, "mode resetting failed!\n"); >>>> >>>> unregister_candev(ndev); >>>> + pm_runtime_disable(&pdev->dev); >>>> netif_napi_del(&priv->napi); >>>> + clk_disable_unprepare(priv->bus_clk); >>>> + clk_disable_unprepare(priv->can_clk); >>> >>> Shouldn't pretty much all these occurrences of clk_disable/enable >>> disappear? This should all be handled by the runtime_pm framework now. >> >> We have: >> - clk_prepare_enable() in probe > > This should become something like pm_runtime_get_sync(), shouldn't it? > >> - clk_disable_unprepare() in remove > > pm_runtime_put() > >> - clk_enable() in runtime_resume >> - clk_disable() in runtime_suspend > > These are the ones needed. > > The above makes me suspect that the clocks are always on, regardless of Define "on" :) The clocks are prepared after probe() exists, but not enabled. The first pm_runtime_get_sync() will enable the clocks. > the runtime suspend state since they are enabled in probe and disabled > in remove, is that right? Ideally, the usage in probe and remove should > be migrated to runtime_pm and clocks should really only be running when > needed and not throughout the whole lifetime of the driver. The clocks are not en/disabled via pm_runtime, because pm_runtime_get_sync() is called from atomic contect. We can have another look into the driver and try to change this. Marc
On Tue, 2015-01-13 at 06:17PM +0100, Marc Kleine-Budde wrote: > On 01/13/2015 06:08 PM, Sören Brinkmann wrote: > > On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote: > >> On 01/12/2015 07:45 PM, Sören Brinkmann wrote: > >>> On Mon, 2015-01-12 at 08:34PM +0530, Kedareswara rao Appana wrote: > >>>> Instead of enabling/disabling clocks at several locations in the driver, > >>>> Use the runtime_pm framework. This consolidates the actions for runtime PM > >>>> In the appropriate callbacks and makes the driver more readable and mantainable. > >>>> > >>>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > >>>> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com> > >>>> --- > >>>> Changes for v5: > >>>> - Updated with the review comments. > >>>> Updated the remove fuction to use runtime_pm. > >>>> Chnages for v4: > >>>> - Updated with the review comments. > >>>> Changes for v3: > >>>> - Converted the driver to use runtime_pm. > >>>> Changes for v2: > >>>> - Removed the struct platform_device* from suspend/resume > >>>> as suggest by Lothar. > >>>> > >>>> drivers/net/can/xilinx_can.c | 157 ++++++++++++++++++++++++++++------------- > >>>> 1 files changed, 107 insertions(+), 50 deletions(-) > >>> [..] > >>>> +static int __maybe_unused xcan_runtime_resume(struct device *dev) > >>>> { > >>>> - struct platform_device *pdev = dev_get_drvdata(dev); > >>>> - struct net_device *ndev = platform_get_drvdata(pdev); > >>>> + struct net_device *ndev = dev_get_drvdata(dev); > >>>> struct xcan_priv *priv = netdev_priv(ndev); > >>>> int ret; > >>>> + u32 isr, status; > >>>> > >>>> ret = clk_enable(priv->bus_clk); > >>>> if (ret) { > >>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev) > >>>> ret = clk_enable(priv->can_clk); > >>>> if (ret) { > >>>> dev_err(dev, "Cannot enable clock.\n"); > >>>> - clk_disable_unprepare(priv->bus_clk); > >>>> + clk_disable(priv->bus_clk); > >>> [...] > >>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev) > >>>> { > >>>> struct net_device *ndev = platform_get_drvdata(pdev); > >>>> struct xcan_priv *priv = netdev_priv(ndev); > >>>> + int ret; > >>>> + > >>>> + ret = pm_runtime_get_sync(&pdev->dev); > >>>> + if (ret < 0) { > >>>> + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n", > >>>> + __func__, ret); > >>>> + return ret; > >>>> + } > >>>> > >>>> if (set_reset_mode(ndev) < 0) > >>>> netdev_err(ndev, "mode resetting failed!\n"); > >>>> > >>>> unregister_candev(ndev); > >>>> + pm_runtime_disable(&pdev->dev); > >>>> netif_napi_del(&priv->napi); > >>>> + clk_disable_unprepare(priv->bus_clk); > >>>> + clk_disable_unprepare(priv->can_clk); > >>> > >>> Shouldn't pretty much all these occurrences of clk_disable/enable > >>> disappear? This should all be handled by the runtime_pm framework now. > >> > >> We have: > >> - clk_prepare_enable() in probe > > > > This should become something like pm_runtime_get_sync(), shouldn't it? > > > >> - clk_disable_unprepare() in remove > > > > pm_runtime_put() > > > >> - clk_enable() in runtime_resume > >> - clk_disable() in runtime_suspend > > > > These are the ones needed. > > > > The above makes me suspect that the clocks are always on, regardless of > > Define "on" :) > The clocks are prepared after probe() exists, but not enabled. The first > pm_runtime_get_sync() will enable the clocks. > > > the runtime suspend state since they are enabled in probe and disabled > > in remove, is that right? Ideally, the usage in probe and remove should > > be migrated to runtime_pm and clocks should really only be running when > > needed and not throughout the whole lifetime of the driver. > > The clocks are not en/disabled via pm_runtime, because > pm_runtime_get_sync() is called from atomic contect. We can have another > look into the driver and try to change this. Wasn't that why the call to pm_runtime_irq_safe() was added? Also, clk_enable/disable should be okay to be run from atomic context. And if the clock are already prepared after the exit of probe that should be enough. Then remove() should just have to do the unprepare. But I don't see why runtime_pm shouldn't be able to do the enable/disable. Sören
On 01/13/2015 06:24 PM, Sören Brinkmann wrote: > On Tue, 2015-01-13 at 06:17PM +0100, Marc Kleine-Budde wrote: >> On 01/13/2015 06:08 PM, Sören Brinkmann wrote: >>> On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote: >>>> On 01/12/2015 07:45 PM, Sören Brinkmann wrote: >>>>> On Mon, 2015-01-12 at 08:34PM +0530, Kedareswara rao Appana wrote: >>>>>> Instead of enabling/disabling clocks at several locations in the driver, >>>>>> Use the runtime_pm framework. This consolidates the actions for runtime PM >>>>>> In the appropriate callbacks and makes the driver more readable and mantainable. >>>>>> >>>>>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> >>>>>> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com> >>>>>> --- >>>>>> Changes for v5: >>>>>> - Updated with the review comments. >>>>>> Updated the remove fuction to use runtime_pm. >>>>>> Chnages for v4: >>>>>> - Updated with the review comments. >>>>>> Changes for v3: >>>>>> - Converted the driver to use runtime_pm. >>>>>> Changes for v2: >>>>>> - Removed the struct platform_device* from suspend/resume >>>>>> as suggest by Lothar. >>>>>> >>>>>> drivers/net/can/xilinx_can.c | 157 ++++++++++++++++++++++++++++------------- >>>>>> 1 files changed, 107 insertions(+), 50 deletions(-) >>>>> [..] >>>>>> +static int __maybe_unused xcan_runtime_resume(struct device *dev) >>>>>> { >>>>>> - struct platform_device *pdev = dev_get_drvdata(dev); >>>>>> - struct net_device *ndev = platform_get_drvdata(pdev); >>>>>> + struct net_device *ndev = dev_get_drvdata(dev); >>>>>> struct xcan_priv *priv = netdev_priv(ndev); >>>>>> int ret; >>>>>> + u32 isr, status; >>>>>> >>>>>> ret = clk_enable(priv->bus_clk); >>>>>> if (ret) { >>>>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev) >>>>>> ret = clk_enable(priv->can_clk); >>>>>> if (ret) { >>>>>> dev_err(dev, "Cannot enable clock.\n"); >>>>>> - clk_disable_unprepare(priv->bus_clk); >>>>>> + clk_disable(priv->bus_clk); >>>>> [...] >>>>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev) >>>>>> { >>>>>> struct net_device *ndev = platform_get_drvdata(pdev); >>>>>> struct xcan_priv *priv = netdev_priv(ndev); >>>>>> + int ret; >>>>>> + >>>>>> + ret = pm_runtime_get_sync(&pdev->dev); >>>>>> + if (ret < 0) { >>>>>> + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n", >>>>>> + __func__, ret); >>>>>> + return ret; >>>>>> + } >>>>>> >>>>>> if (set_reset_mode(ndev) < 0) >>>>>> netdev_err(ndev, "mode resetting failed!\n"); >>>>>> >>>>>> unregister_candev(ndev); >>>>>> + pm_runtime_disable(&pdev->dev); >>>>>> netif_napi_del(&priv->napi); >>>>>> + clk_disable_unprepare(priv->bus_clk); >>>>>> + clk_disable_unprepare(priv->can_clk); >>>>> >>>>> Shouldn't pretty much all these occurrences of clk_disable/enable >>>>> disappear? This should all be handled by the runtime_pm framework now. >>>> >>>> We have: >>>> - clk_prepare_enable() in probe >>> >>> This should become something like pm_runtime_get_sync(), shouldn't it? >>> >>>> - clk_disable_unprepare() in remove >>> >>> pm_runtime_put() >>> >>>> - clk_enable() in runtime_resume >>>> - clk_disable() in runtime_suspend >>> >>> These are the ones needed. >>> >>> The above makes me suspect that the clocks are always on, regardless of >> >> Define "on" :) >> The clocks are prepared after probe() exists, but not enabled. The first >> pm_runtime_get_sync() will enable the clocks. >> >>> the runtime suspend state since they are enabled in probe and disabled >>> in remove, is that right? Ideally, the usage in probe and remove should >>> be migrated to runtime_pm and clocks should really only be running when >>> needed and not throughout the whole lifetime of the driver. >> >> The clocks are not en/disabled via pm_runtime, because >> pm_runtime_get_sync() is called from atomic contect. We can have another >> look into the driver and try to change this. > Wasn't that why the call to pm_runtime_irq_safe() was added? Good question. That should be investigated. > Also, clk_enable/disable should be okay to be run from atomic context. > And if the clock are already prepared after the exit of probe that > should be enough. Then remove() should just have to do the unprepare. > But I don't see why runtime_pm shouldn't be able to do the > enable/disable. runtime_pm does call the clk_{enable,disable} function. But you mean clk_prepare() + pm_runtime_get_sync() should be used in probe() instead of calling clk_prepare_enable(). Good idea! I think the "pm_runtime_set_active(&pdev->dev);" has to be removed from the patch. Coming back whether blocking calls are allowed or not. If you make a call to pm_runtime_irq_safe(), you state that it's okay to call pm_runtime_get_sync() from atomic context. But it's only called in open, probe, remove and in xcan_get_berr_counter, which is not called from atomic either. So let's try to remove the pm_runtime_irq_safe() and use clk_prepare_enable() clk_disable_unprepare() in the runtime_resume() runtime_suspend() functions. Marc
On Tue, 2015-01-13 at 06:44PM +0100, Marc Kleine-Budde wrote: > On 01/13/2015 06:24 PM, Sören Brinkmann wrote: > > On Tue, 2015-01-13 at 06:17PM +0100, Marc Kleine-Budde wrote: > >> On 01/13/2015 06:08 PM, Sören Brinkmann wrote: > >>> On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote: > >>>> On 01/12/2015 07:45 PM, Sören Brinkmann wrote: > >>>>> On Mon, 2015-01-12 at 08:34PM +0530, Kedareswara rao Appana wrote: > >>>>>> Instead of enabling/disabling clocks at several locations in the driver, > >>>>>> Use the runtime_pm framework. This consolidates the actions for runtime PM > >>>>>> In the appropriate callbacks and makes the driver more readable and mantainable. > >>>>>> > >>>>>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > >>>>>> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com> > >>>>>> --- > >>>>>> Changes for v5: > >>>>>> - Updated with the review comments. > >>>>>> Updated the remove fuction to use runtime_pm. > >>>>>> Chnages for v4: > >>>>>> - Updated with the review comments. > >>>>>> Changes for v3: > >>>>>> - Converted the driver to use runtime_pm. > >>>>>> Changes for v2: > >>>>>> - Removed the struct platform_device* from suspend/resume > >>>>>> as suggest by Lothar. > >>>>>> > >>>>>> drivers/net/can/xilinx_can.c | 157 ++++++++++++++++++++++++++++------------- > >>>>>> 1 files changed, 107 insertions(+), 50 deletions(-) > >>>>> [..] > >>>>>> +static int __maybe_unused xcan_runtime_resume(struct device *dev) > >>>>>> { > >>>>>> - struct platform_device *pdev = dev_get_drvdata(dev); > >>>>>> - struct net_device *ndev = platform_get_drvdata(pdev); > >>>>>> + struct net_device *ndev = dev_get_drvdata(dev); > >>>>>> struct xcan_priv *priv = netdev_priv(ndev); > >>>>>> int ret; > >>>>>> + u32 isr, status; > >>>>>> > >>>>>> ret = clk_enable(priv->bus_clk); > >>>>>> if (ret) { > >>>>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev) > >>>>>> ret = clk_enable(priv->can_clk); > >>>>>> if (ret) { > >>>>>> dev_err(dev, "Cannot enable clock.\n"); > >>>>>> - clk_disable_unprepare(priv->bus_clk); > >>>>>> + clk_disable(priv->bus_clk); > >>>>> [...] > >>>>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev) > >>>>>> { > >>>>>> struct net_device *ndev = platform_get_drvdata(pdev); > >>>>>> struct xcan_priv *priv = netdev_priv(ndev); > >>>>>> + int ret; > >>>>>> + > >>>>>> + ret = pm_runtime_get_sync(&pdev->dev); > >>>>>> + if (ret < 0) { > >>>>>> + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n", > >>>>>> + __func__, ret); > >>>>>> + return ret; > >>>>>> + } > >>>>>> > >>>>>> if (set_reset_mode(ndev) < 0) > >>>>>> netdev_err(ndev, "mode resetting failed!\n"); > >>>>>> > >>>>>> unregister_candev(ndev); > >>>>>> + pm_runtime_disable(&pdev->dev); > >>>>>> netif_napi_del(&priv->napi); > >>>>>> + clk_disable_unprepare(priv->bus_clk); > >>>>>> + clk_disable_unprepare(priv->can_clk); > >>>>> > >>>>> Shouldn't pretty much all these occurrences of clk_disable/enable > >>>>> disappear? This should all be handled by the runtime_pm framework now. > >>>> > >>>> We have: > >>>> - clk_prepare_enable() in probe > >>> > >>> This should become something like pm_runtime_get_sync(), shouldn't it? > >>> > >>>> - clk_disable_unprepare() in remove > >>> > >>> pm_runtime_put() > >>> > >>>> - clk_enable() in runtime_resume > >>>> - clk_disable() in runtime_suspend > >>> > >>> These are the ones needed. > >>> > >>> The above makes me suspect that the clocks are always on, regardless of > >> > >> Define "on" :) > >> The clocks are prepared after probe() exists, but not enabled. The first > >> pm_runtime_get_sync() will enable the clocks. > >> > >>> the runtime suspend state since they are enabled in probe and disabled > >>> in remove, is that right? Ideally, the usage in probe and remove should > >>> be migrated to runtime_pm and clocks should really only be running when > >>> needed and not throughout the whole lifetime of the driver. > >> > >> The clocks are not en/disabled via pm_runtime, because > >> pm_runtime_get_sync() is called from atomic contect. We can have another > >> look into the driver and try to change this. > > > Wasn't that why the call to pm_runtime_irq_safe() was added? > > Good question. That should be investigated. > > > Also, clk_enable/disable should be okay to be run from atomic context. > > And if the clock are already prepared after the exit of probe that > > should be enough. Then remove() should just have to do the unprepare. > > But I don't see why runtime_pm shouldn't be able to do the > > enable/disable. > > runtime_pm does call the clk_{enable,disable} function. But you mean > clk_prepare() + pm_runtime_get_sync() should be used in probe() instead > of calling clk_prepare_enable(). Good idea! I think the > "pm_runtime_set_active(&pdev->dev);" has to be removed from the patch. Right, that's what I was thinking. The proposed changes make sense, IMHO. > > Coming back whether blocking calls are allowed or not. > If you make a call to pm_runtime_irq_safe(), you state that it's okay to > call pm_runtime_get_sync() from atomic context. But it's only called in > open, probe, remove and in xcan_get_berr_counter, which is not called > from atomic either. So let's try to remove the pm_runtime_irq_safe() and > use clk_prepare_enable() clk_disable_unprepare() in the runtime_resume() > runtime_suspend() functions. IIRC, xcan_get_berr_counter() is called from atomic context. I think that was how this got started. Sören
On 01/13/2015 06:49 PM, Sören Brinkmann wrote: > On Tue, 2015-01-13 at 06:44PM +0100, Marc Kleine-Budde wrote: >> On 01/13/2015 06:24 PM, Sören Brinkmann wrote: >>> On Tue, 2015-01-13 at 06:17PM +0100, Marc Kleine-Budde wrote: >>>> On 01/13/2015 06:08 PM, Sören Brinkmann wrote: >>>>> On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote: >>>>>> On 01/12/2015 07:45 PM, Sören Brinkmann wrote: >>>>>>> On Mon, 2015-01-12 at 08:34PM +0530, Kedareswara rao Appana wrote: >>>>>>>> Instead of enabling/disabling clocks at several locations in the driver, >>>>>>>> Use the runtime_pm framework. This consolidates the actions for runtime PM >>>>>>>> In the appropriate callbacks and makes the driver more readable and mantainable. >>>>>>>> >>>>>>>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> >>>>>>>> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com> >>>>>>>> --- >>>>>>>> Changes for v5: >>>>>>>> - Updated with the review comments. >>>>>>>> Updated the remove fuction to use runtime_pm. >>>>>>>> Chnages for v4: >>>>>>>> - Updated with the review comments. >>>>>>>> Changes for v3: >>>>>>>> - Converted the driver to use runtime_pm. >>>>>>>> Changes for v2: >>>>>>>> - Removed the struct platform_device* from suspend/resume >>>>>>>> as suggest by Lothar. >>>>>>>> >>>>>>>> drivers/net/can/xilinx_can.c | 157 ++++++++++++++++++++++++++++------------- >>>>>>>> 1 files changed, 107 insertions(+), 50 deletions(-) >>>>>>> [..] >>>>>>>> +static int __maybe_unused xcan_runtime_resume(struct device *dev) >>>>>>>> { >>>>>>>> - struct platform_device *pdev = dev_get_drvdata(dev); >>>>>>>> - struct net_device *ndev = platform_get_drvdata(pdev); >>>>>>>> + struct net_device *ndev = dev_get_drvdata(dev); >>>>>>>> struct xcan_priv *priv = netdev_priv(ndev); >>>>>>>> int ret; >>>>>>>> + u32 isr, status; >>>>>>>> >>>>>>>> ret = clk_enable(priv->bus_clk); >>>>>>>> if (ret) { >>>>>>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev) >>>>>>>> ret = clk_enable(priv->can_clk); >>>>>>>> if (ret) { >>>>>>>> dev_err(dev, "Cannot enable clock.\n"); >>>>>>>> - clk_disable_unprepare(priv->bus_clk); >>>>>>>> + clk_disable(priv->bus_clk); >>>>>>> [...] >>>>>>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev) >>>>>>>> { >>>>>>>> struct net_device *ndev = platform_get_drvdata(pdev); >>>>>>>> struct xcan_priv *priv = netdev_priv(ndev); >>>>>>>> + int ret; >>>>>>>> + >>>>>>>> + ret = pm_runtime_get_sync(&pdev->dev); >>>>>>>> + if (ret < 0) { >>>>>>>> + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n", >>>>>>>> + __func__, ret); >>>>>>>> + return ret; >>>>>>>> + } >>>>>>>> >>>>>>>> if (set_reset_mode(ndev) < 0) >>>>>>>> netdev_err(ndev, "mode resetting failed!\n"); >>>>>>>> >>>>>>>> unregister_candev(ndev); >>>>>>>> + pm_runtime_disable(&pdev->dev); >>>>>>>> netif_napi_del(&priv->napi); >>>>>>>> + clk_disable_unprepare(priv->bus_clk); >>>>>>>> + clk_disable_unprepare(priv->can_clk); >>>>>>> >>>>>>> Shouldn't pretty much all these occurrences of clk_disable/enable >>>>>>> disappear? This should all be handled by the runtime_pm framework now. >>>>>> >>>>>> We have: >>>>>> - clk_prepare_enable() in probe >>>>> >>>>> This should become something like pm_runtime_get_sync(), shouldn't it? >>>>> >>>>>> - clk_disable_unprepare() in remove >>>>> >>>>> pm_runtime_put() >>>>> >>>>>> - clk_enable() in runtime_resume >>>>>> - clk_disable() in runtime_suspend >>>>> >>>>> These are the ones needed. >>>>> >>>>> The above makes me suspect that the clocks are always on, regardless of >>>> >>>> Define "on" :) >>>> The clocks are prepared after probe() exists, but not enabled. The first >>>> pm_runtime_get_sync() will enable the clocks. >>>> >>>>> the runtime suspend state since they are enabled in probe and disabled >>>>> in remove, is that right? Ideally, the usage in probe and remove should >>>>> be migrated to runtime_pm and clocks should really only be running when >>>>> needed and not throughout the whole lifetime of the driver. >>>> >>>> The clocks are not en/disabled via pm_runtime, because >>>> pm_runtime_get_sync() is called from atomic contect. We can have another >>>> look into the driver and try to change this. >> >>> Wasn't that why the call to pm_runtime_irq_safe() was added? >> >> Good question. That should be investigated. >> >>> Also, clk_enable/disable should be okay to be run from atomic context. >>> And if the clock are already prepared after the exit of probe that >>> should be enough. Then remove() should just have to do the unprepare. >>> But I don't see why runtime_pm shouldn't be able to do the >>> enable/disable. >> >> runtime_pm does call the clk_{enable,disable} function. But you mean >> clk_prepare() + pm_runtime_get_sync() should be used in probe() instead >> of calling clk_prepare_enable(). Good idea! I think the >> "pm_runtime_set_active(&pdev->dev);" has to be removed from the patch. > > Right, that's what I was thinking. The proposed changes make sense, IMHO. > >> >> Coming back whether blocking calls are allowed or not. >> If you make a call to pm_runtime_irq_safe(), you state that it's okay to >> call pm_runtime_get_sync() from atomic context. But it's only called in >> open, probe, remove and in xcan_get_berr_counter, which is not called >> from atomic either. So let's try to remove the pm_runtime_irq_safe() and >> use clk_prepare_enable() clk_disable_unprepare() in the runtime_resume() >> runtime_suspend() functions. > > IIRC, xcan_get_berr_counter() is called from atomic context. I think > that was how this got started. In some drivers the get_berr_counter() function is used in the irq handler, but here it's only called from outside, an thus from non atomic context. From an older mail of yours: > I have the feeling I'm missing something. If I remove the 'must not > sleep' requirement from the runtime suspend/resume functions, I get > this: > > BUG: sleeping function called from invalid context at drivers/base/power/runtime.c:954 http://lxr.free-electrons.com/source/drivers/base/power/runtime.c#L954 I think it's failing because of the pm_runtime_irq_safe() call. > in_atomic(): 0, irqs_disabled(): 0, pid: 161, name: ip > INFO: lockdep is turned off. > CPU: 0 PID: 161 Comm: ip Not tainted 3.18.0-rc1-xilinx-00059-g21da26693b61-dirty #104 > [<c00186a8>] (unwind_backtrace) from [<c00139f4>] (show_stack+0x20/0x24) > [<c00139f4>] (show_stack) from [<c055a41c>] (dump_stack+0x8c/0xd0) > [<c055a41c>] (dump_stack) from [<c0054808>] (__might_sleep+0x1ac/0x1e4) > [<c0054808>] (__might_sleep) from [<c034f8f0>] (__pm_runtime_resume+0x40/0x9c) > [<c034f8f0>] (__pm_runtime_resume) from [<c03b48d8>] (xcan_get_berr_counter+0x2c/0x9c) > [<c03b48d8>] (xcan_get_berr_counter) from [<c03b2ecc>] (can_fill_info+0x160/0x1f4) > [<c03b2ecc>] (can_fill_info) from [<c049f3b0>] (rtnl_fill_ifinfo+0x794/0x970) > [<c049f3b0>] (rtnl_fill_ifinfo) from [<c04a0048>] (rtnl_dump_ifinfo+0x1b4/0x2fc) > [<c04a0048>] (rtnl_dump_ifinfo) from [<c04af9c8>] (netlink_dump+0xe4/0x270) > [<c04af9c8>] (netlink_dump) from [<c04b0764>] (__netlink_dump_start+0xdc/0x170) > [<c04b0764>] (__netlink_dump_start) from [<c04a1fc4>] (rtnetlink_rcv_msg+0x154/0x1e0) > [<c04a1fc4>] (rtnetlink_rcv_msg) from [<c04b1e88>] (netlink_rcv_skb+0x68/0xc4) > [<c04b1e88>] (netlink_rcv_skb) from [<c04a045c>] (rtnetlink_rcv+0x28/0x34) > [<c04a045c>] (rtnetlink_rcv) from [<c04b1770>] (netlink_unicast+0x144/0x210) > [<c04b1770>] (netlink_unicast) from [<c04b1c9c>] (netlink_sendmsg+0x394/0x414) > [<c04b1c9c>] (netlink_sendmsg) from [<c046ffcc>] (sock_sendmsg+0x8c/0xc0) > [<c046ffcc>] (sock_sendmsg) from [<c04726bc>] (SyS_sendto+0xd8/0x114) > [<c04726bc>] (SyS_sendto) from [<c000f3e0>] (ret_fast_syscall+0x0/0x48) > > I.e. the core calls this function from atomic context. And in an earlier > thread you said the core can also call this before/after calling the > open/close callbacks (which applies here too, I think). Marc
On Tue, 2015-01-13 at 07:03PM +0100, Marc Kleine-Budde wrote: > On 01/13/2015 06:49 PM, Sören Brinkmann wrote: > > On Tue, 2015-01-13 at 06:44PM +0100, Marc Kleine-Budde wrote: > >> On 01/13/2015 06:24 PM, Sören Brinkmann wrote: > >>> On Tue, 2015-01-13 at 06:17PM +0100, Marc Kleine-Budde wrote: > >>>> On 01/13/2015 06:08 PM, Sören Brinkmann wrote: > >>>>> On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote: > >>>>>> On 01/12/2015 07:45 PM, Sören Brinkmann wrote: > >>>>>>> On Mon, 2015-01-12 at 08:34PM +0530, Kedareswara rao Appana wrote: > >>>>>>>> Instead of enabling/disabling clocks at several locations in the driver, > >>>>>>>> Use the runtime_pm framework. This consolidates the actions for runtime PM > >>>>>>>> In the appropriate callbacks and makes the driver more readable and mantainable. > >>>>>>>> > >>>>>>>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > >>>>>>>> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com> > >>>>>>>> --- > >>>>>>>> Changes for v5: > >>>>>>>> - Updated with the review comments. > >>>>>>>> Updated the remove fuction to use runtime_pm. > >>>>>>>> Chnages for v4: > >>>>>>>> - Updated with the review comments. > >>>>>>>> Changes for v3: > >>>>>>>> - Converted the driver to use runtime_pm. > >>>>>>>> Changes for v2: > >>>>>>>> - Removed the struct platform_device* from suspend/resume > >>>>>>>> as suggest by Lothar. > >>>>>>>> > >>>>>>>> drivers/net/can/xilinx_can.c | 157 ++++++++++++++++++++++++++++------------- > >>>>>>>> 1 files changed, 107 insertions(+), 50 deletions(-) > >>>>>>> [..] > >>>>>>>> +static int __maybe_unused xcan_runtime_resume(struct device *dev) > >>>>>>>> { > >>>>>>>> - struct platform_device *pdev = dev_get_drvdata(dev); > >>>>>>>> - struct net_device *ndev = platform_get_drvdata(pdev); > >>>>>>>> + struct net_device *ndev = dev_get_drvdata(dev); > >>>>>>>> struct xcan_priv *priv = netdev_priv(ndev); > >>>>>>>> int ret; > >>>>>>>> + u32 isr, status; > >>>>>>>> > >>>>>>>> ret = clk_enable(priv->bus_clk); > >>>>>>>> if (ret) { > >>>>>>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev) > >>>>>>>> ret = clk_enable(priv->can_clk); > >>>>>>>> if (ret) { > >>>>>>>> dev_err(dev, "Cannot enable clock.\n"); > >>>>>>>> - clk_disable_unprepare(priv->bus_clk); > >>>>>>>> + clk_disable(priv->bus_clk); > >>>>>>> [...] > >>>>>>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev) > >>>>>>>> { > >>>>>>>> struct net_device *ndev = platform_get_drvdata(pdev); > >>>>>>>> struct xcan_priv *priv = netdev_priv(ndev); > >>>>>>>> + int ret; > >>>>>>>> + > >>>>>>>> + ret = pm_runtime_get_sync(&pdev->dev); > >>>>>>>> + if (ret < 0) { > >>>>>>>> + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n", > >>>>>>>> + __func__, ret); > >>>>>>>> + return ret; > >>>>>>>> + } > >>>>>>>> > >>>>>>>> if (set_reset_mode(ndev) < 0) > >>>>>>>> netdev_err(ndev, "mode resetting failed!\n"); > >>>>>>>> > >>>>>>>> unregister_candev(ndev); > >>>>>>>> + pm_runtime_disable(&pdev->dev); > >>>>>>>> netif_napi_del(&priv->napi); > >>>>>>>> + clk_disable_unprepare(priv->bus_clk); > >>>>>>>> + clk_disable_unprepare(priv->can_clk); > >>>>>>> > >>>>>>> Shouldn't pretty much all these occurrences of clk_disable/enable > >>>>>>> disappear? This should all be handled by the runtime_pm framework now. > >>>>>> > >>>>>> We have: > >>>>>> - clk_prepare_enable() in probe > >>>>> > >>>>> This should become something like pm_runtime_get_sync(), shouldn't it? > >>>>> > >>>>>> - clk_disable_unprepare() in remove > >>>>> > >>>>> pm_runtime_put() > >>>>> > >>>>>> - clk_enable() in runtime_resume > >>>>>> - clk_disable() in runtime_suspend > >>>>> > >>>>> These are the ones needed. > >>>>> > >>>>> The above makes me suspect that the clocks are always on, regardless of > >>>> > >>>> Define "on" :) > >>>> The clocks are prepared after probe() exists, but not enabled. The first > >>>> pm_runtime_get_sync() will enable the clocks. > >>>> > >>>>> the runtime suspend state since they are enabled in probe and disabled > >>>>> in remove, is that right? Ideally, the usage in probe and remove should > >>>>> be migrated to runtime_pm and clocks should really only be running when > >>>>> needed and not throughout the whole lifetime of the driver. > >>>> > >>>> The clocks are not en/disabled via pm_runtime, because > >>>> pm_runtime_get_sync() is called from atomic contect. We can have another > >>>> look into the driver and try to change this. > >> > >>> Wasn't that why the call to pm_runtime_irq_safe() was added? > >> > >> Good question. That should be investigated. > >> > >>> Also, clk_enable/disable should be okay to be run from atomic context. > >>> And if the clock are already prepared after the exit of probe that > >>> should be enough. Then remove() should just have to do the unprepare. > >>> But I don't see why runtime_pm shouldn't be able to do the > >>> enable/disable. > >> > >> runtime_pm does call the clk_{enable,disable} function. But you mean > >> clk_prepare() + pm_runtime_get_sync() should be used in probe() instead > >> of calling clk_prepare_enable(). Good idea! I think the > >> "pm_runtime_set_active(&pdev->dev);" has to be removed from the patch. > > > > Right, that's what I was thinking. The proposed changes make sense, IMHO. > > > >> > >> Coming back whether blocking calls are allowed or not. > >> If you make a call to pm_runtime_irq_safe(), you state that it's okay to > >> call pm_runtime_get_sync() from atomic context. But it's only called in > >> open, probe, remove and in xcan_get_berr_counter, which is not called > >> from atomic either. So let's try to remove the pm_runtime_irq_safe() and > >> use clk_prepare_enable() clk_disable_unprepare() in the runtime_resume() > >> runtime_suspend() functions. > > > > IIRC, xcan_get_berr_counter() is called from atomic context. I think > > that was how this got started. > > In some drivers the get_berr_counter() function is used in the irq > handler, but here it's only called from outside, an thus from non atomic > context. > > From an older mail of yours: > > > I have the feeling I'm missing something. If I remove the 'must not > > sleep' requirement from the runtime suspend/resume functions, I get > > this: > > > > BUG: sleeping function called from invalid context at drivers/base/power/runtime.c:954 > > http://lxr.free-electrons.com/source/drivers/base/power/runtime.c#L954 > > I think it's failing because of the pm_runtime_irq_safe() call. Adding that call fixed this issue. Sören
On 01/12/2015 04:04 PM, Kedareswara rao Appana wrote: > Instead of enabling/disabling clocks at several locations in the driver, > Use the runtime_pm framework. This consolidates the actions for runtime PM > In the appropriate callbacks and makes the driver more readable and mantainable. > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com> > --- > Changes for v5: > - Updated with the review comments. > Updated the remove fuction to use runtime_pm. > Chnages for v4: > - Updated with the review comments. > Changes for v3: > - Converted the driver to use runtime_pm. > Changes for v2: > - Removed the struct platform_device* from suspend/resume > as suggest by Lothar. Any plans to submit a v6? Marc
Hi Marc, > -----Original Message----- > From: Marc Kleine-Budde [mailto:mkl@pengutronix.de] > Sent: Wednesday, January 28, 2015 8:16 PM > To: Appana Durga Kedareswara Rao; wg@grandegger.com; Michal Simek; > Soren Brinkmann; grant.likely@linaro.org; robh+dt@kernel.org > Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org; Appana Durga Kedareswara Rao > Subject: Re: [PATCH v5] can: Convert to runtime_pm > > On 01/12/2015 04:04 PM, Kedareswara rao Appana wrote: > > Instead of enabling/disabling clocks at several locations in the > > driver, Use the runtime_pm framework. This consolidates the actions > > for runtime PM In the appropriate callbacks and makes the driver more > readable and mantainable. > > > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com> > > --- > > Changes for v5: > > - Updated with the review comments. > > Updated the remove fuction to use runtime_pm. > > Chnages for v4: > > - Updated with the review comments. > > Changes for v3: > > - Converted the driver to use runtime_pm. > > Changes for v2: > > - Removed the struct platform_device* from suspend/resume > > as suggest by Lothar. > > Any plans to submit a v6? I was on vacation till yesterday just came to office today only. Will look into it and will send v6 at the earliest. Regards, Kedar. > > Marc > -- > Pengutronix e.K. | Marc Kleine-Budde | > Industrial Linux Solutions | Phone: +49-231-2826-924 | > Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | > Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c index 6c67643..67aef00 100644 --- a/drivers/net/can/xilinx_can.c +++ b/drivers/net/can/xilinx_can.c @@ -32,6 +32,7 @@ #include <linux/can/dev.h> #include <linux/can/error.h> #include <linux/can/led.h> +#include <linux/pm_runtime.h> #define DRIVER_NAME "xilinx_can" @@ -138,7 +139,7 @@ struct xcan_priv { u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg); void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg, u32 val); - struct net_device *dev; + struct device *dev; void __iomem *reg_base; unsigned long irq_flags; struct clk *bus_clk; @@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev) struct xcan_priv *priv = netdev_priv(ndev); int ret; + ret = pm_runtime_get_sync(priv->dev); + if (ret < 0) { + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n", + __func__, ret); + return ret; + } + ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags, ndev->name, ndev); if (ret < 0) { @@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev) goto err; } - ret = clk_prepare_enable(priv->can_clk); - if (ret) { - netdev_err(ndev, "unable to enable device clock\n"); - goto err_irq; - } - - ret = clk_prepare_enable(priv->bus_clk); - if (ret) { - netdev_err(ndev, "unable to enable bus clock\n"); - goto err_can_clk; - } - /* Set chip into reset mode */ ret = set_reset_mode(ndev); if (ret < 0) { netdev_err(ndev, "mode resetting failed!\n"); - goto err_bus_clk; + goto err_irq; } /* Common open */ ret = open_candev(ndev); if (ret) - goto err_bus_clk; + goto err_irq; ret = xcan_chip_start(ndev); if (ret < 0) { @@ -887,13 +883,11 @@ static int xcan_open(struct net_device *ndev) err_candev: close_candev(ndev); -err_bus_clk: - clk_disable_unprepare(priv->bus_clk); -err_can_clk: - clk_disable_unprepare(priv->can_clk); err_irq: free_irq(ndev->irq, ndev); err: + pm_runtime_put(priv->dev); + return ret; } @@ -910,12 +904,11 @@ static int xcan_close(struct net_device *ndev) netif_stop_queue(ndev); napi_disable(&priv->napi); xcan_chip_stop(ndev); - clk_disable_unprepare(priv->bus_clk); - clk_disable_unprepare(priv->can_clk); free_irq(ndev->irq, ndev); close_candev(ndev); can_led_event(ndev, CAN_LED_EVENT_STOP); + pm_runtime_put(priv->dev); return 0; } @@ -934,27 +927,20 @@ static int xcan_get_berr_counter(const struct net_device *ndev, struct xcan_priv *priv = netdev_priv(ndev); int ret; - ret = clk_prepare_enable(priv->can_clk); - if (ret) - goto err; - - ret = clk_prepare_enable(priv->bus_clk); - if (ret) - goto err_clk; + ret = pm_runtime_get_sync(priv->dev); + if (ret < 0) { + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n", + __func__, ret); + return ret; + } bec->txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) & XCAN_ECR_TEC_MASK; bec->rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) & XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT); - clk_disable_unprepare(priv->bus_clk); - clk_disable_unprepare(priv->can_clk); + pm_runtime_put(priv->dev); return 0; - -err_clk: - clk_disable_unprepare(priv->can_clk); -err: - return ret; } @@ -967,15 +953,45 @@ static const struct net_device_ops xcan_netdev_ops = { /** * xcan_suspend - Suspend method for the driver - * @dev: Address of the platform_device structure + * @dev: Address of the device structure * * Put the driver into low power mode. - * Return: 0 always + * Return: 0 on success and failure value on error */ static int __maybe_unused xcan_suspend(struct device *dev) { - struct platform_device *pdev = dev_get_drvdata(dev); - struct net_device *ndev = platform_get_drvdata(pdev); + if (!device_may_wakeup(dev)) + return pm_runtime_force_suspend(dev); + + return 0; +} + +/** + * xcan_resume - Resume from suspend + * @dev: Address of the device structure + * + * Resume operation after suspend. + * Return: 0 on success and failure value on error + */ +static int __maybe_unused xcan_resume(struct device *dev) +{ + if (!device_may_wakeup(dev)) + return pm_runtime_force_resume(dev); + + return 0; + +} + +/** + * xcan_runtime_suspend - Runtime suspend method for the driver + * @dev: Address of the device structure + * + * Put the driver into low power mode. + * Return: 0 always + */ +static int __maybe_unused xcan_runtime_suspend(struct device *dev) +{ + struct net_device *ndev = dev_get_drvdata(dev); struct xcan_priv *priv = netdev_priv(ndev); if (netif_running(ndev)) { @@ -993,18 +1009,18 @@ static int __maybe_unused xcan_suspend(struct device *dev) } /** - * xcan_resume - Resume from suspend - * @dev: Address of the platformdevice structure + * xcan_runtime_resume - Runtime resume from suspend + * @dev: Address of the device structure * * Resume operation after suspend. * Return: 0 on success and failure value on error */ -static int __maybe_unused xcan_resume(struct device *dev) +static int __maybe_unused xcan_runtime_resume(struct device *dev) { - struct platform_device *pdev = dev_get_drvdata(dev); - struct net_device *ndev = platform_get_drvdata(pdev); + struct net_device *ndev = dev_get_drvdata(dev); struct xcan_priv *priv = netdev_priv(ndev); int ret; + u32 isr, status; ret = clk_enable(priv->bus_clk); if (ret) { @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev) ret = clk_enable(priv->can_clk); if (ret) { dev_err(dev, "Cannot enable clock.\n"); - clk_disable_unprepare(priv->bus_clk); + clk_disable(priv->bus_clk); return ret; } priv->write_reg(priv, XCAN_MSR_OFFSET, 0); priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK); - priv->can.state = CAN_STATE_ERROR_ACTIVE; + isr = priv->read_reg(priv, XCAN_ISR_OFFSET); + status = priv->read_reg(priv, XCAN_SR_OFFSET); if (netif_running(ndev)) { + if (isr & XCAN_IXR_BSOFF_MASK) { + priv->can.state = CAN_STATE_BUS_OFF; + priv->write_reg(priv, XCAN_SRR_OFFSET, + XCAN_SRR_RESET_MASK); + } else if ((status & XCAN_SR_ESTAT_MASK) == + XCAN_SR_ESTAT_MASK) { + priv->can.state = CAN_STATE_ERROR_PASSIVE; + } else if (status & XCAN_SR_ERRWRN_MASK) { + priv->can.state = CAN_STATE_ERROR_WARNING; + } else { + priv->can.state = CAN_STATE_ERROR_ACTIVE; + } netif_device_attach(ndev); netif_start_queue(ndev); } @@ -1030,7 +1059,10 @@ static int __maybe_unused xcan_resume(struct device *dev) return 0; } -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend, xcan_resume); +static const struct dev_pm_ops xcan_dev_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume) + SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend, xcan_runtime_resume, NULL) +}; /** * xcan_probe - Platform registration call @@ -1071,7 +1103,7 @@ static int xcan_probe(struct platform_device *pdev) return -ENOMEM; priv = netdev_priv(ndev); - priv->dev = ndev; + priv->dev = &pdev->dev; priv->can.bittiming_const = &xcan_bittiming_const; priv->can.do_set_mode = xcan_do_set_mode; priv->can.do_get_berr_counter = xcan_get_berr_counter; @@ -1137,6 +1169,16 @@ static int xcan_probe(struct platform_device *pdev) netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max); + pm_runtime_set_active(&pdev->dev); + pm_runtime_irq_safe(&pdev->dev); + pm_runtime_enable(&pdev->dev); + ret = pm_runtime_get_sync(&pdev->dev); + if (ret < 0) { + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n", + __func__, ret); + goto err_pmdisable; + } + ret = register_candev(ndev); if (ret) { dev_err(&pdev->dev, "fail to register failed (err=%d)\n", ret); @@ -1144,15 +1186,19 @@ static int xcan_probe(struct platform_device *pdev) } devm_can_led_init(ndev); - clk_disable_unprepare(priv->bus_clk); - clk_disable_unprepare(priv->can_clk); + + pm_runtime_put(&pdev->dev); + netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo depth:%d\n", priv->reg_base, ndev->irq, priv->can.clock.freq, priv->tx_max); return 0; +err_pmdisable: + pm_runtime_disable(&pdev->dev); err_unprepare_disable_busclk: + pm_runtime_put(priv->dev); clk_disable_unprepare(priv->bus_clk); err_unprepare_disable_dev: clk_disable_unprepare(priv->can_clk); @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev) { struct net_device *ndev = platform_get_drvdata(pdev); struct xcan_priv *priv = netdev_priv(ndev); + int ret; + + ret = pm_runtime_get_sync(&pdev->dev); + if (ret < 0) { + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n", + __func__, ret); + return ret; + } if (set_reset_mode(ndev) < 0) netdev_err(ndev, "mode resetting failed!\n"); unregister_candev(ndev); + pm_runtime_disable(&pdev->dev); netif_napi_del(&priv->napi); + clk_disable_unprepare(priv->bus_clk); + clk_disable_unprepare(priv->can_clk); free_candev(ndev); return 0;