| Message ID | e7070431858e49e29cd03d76fd7a66c3@BN1AFFO11FD055.protection.gbl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/14/2014 09:16 AM, Kedareswara rao Appana wrote: > The drvdata in the suspend/resume is of type struct net_device, > not the platform device.Enable the clocks in the suspend before > accessing the registers of the CAN. > > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com> > --- > Changes for v2: > - Removed the struct platform_device* from suspend/resume > as suggest by Lothar. > - The clocks are getting disabled and un prepared at the end of the probe. > In the suspend the driver is doing a register write.In order > To do that register write we have to again enable and prepare the clocks. Please look the at suspend/resume code and count the clock_enable/disable manually. After a suspend/resume cycle, you have enabled the clock twice, but disabled it once. I think you have to abstract the clock handling behind runtime PM. I haven't done this myself yet, but the strong feeling that this is a possible solution to your problem. These links might help: http://lwn.net/Articles/505683/ http://www.slideshare.net/linaroorg/runtime-pm http://www.slideshare.net/linaroorg/lca14-407-deployingruntimepmonarmsocs http://www.slideshare.net/SamsungOSG/shuah-khan-lpcpmops Marc
On Fri, 2014-11-14 at 09:54AM +0100, Marc Kleine-Budde wrote: > On 11/14/2014 09:16 AM, Kedareswara rao Appana wrote: > > The drvdata in the suspend/resume is of type struct net_device, > > not the platform device.Enable the clocks in the suspend before > > accessing the registers of the CAN. > > > > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com> > > --- > > Changes for v2: > > - Removed the struct platform_device* from suspend/resume > > as suggest by Lothar. > > - The clocks are getting disabled and un prepared at the end of the probe. > > In the suspend the driver is doing a register write.In order > > To do that register write we have to again enable and prepare the clocks. > > Please look the at suspend/resume code and count the > clock_enable/disable manually. After a suspend/resume cycle, you have > enabled the clock twice, but disabled it once. > > I think you have to abstract the clock handling behind runtime PM. I > haven't done this myself yet, but the strong feeling that this is a > possible solution to your problem. These links might help: I agree, the clock handling looks weird. Also the clk_disable calls in xcan_get_berr_counter() look suspicious to me, but I might be wrong. I think you can take a look at gpio-zynq for an example for runtime_pm usage. I think the usage model in that driver is similar to here. Thanks, Sören
On 11/14/2014 04:05 PM, Sören Brinkmann wrote: > On Fri, 2014-11-14 at 09:54AM +0100, Marc Kleine-Budde wrote: >> On 11/14/2014 09:16 AM, Kedareswara rao Appana wrote: >>> The drvdata in the suspend/resume is of type struct net_device, >>> not the platform device.Enable the clocks in the suspend before >>> accessing the registers of the CAN. >>> >>> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com> >>> --- >>> Changes for v2: >>> - Removed the struct platform_device* from suspend/resume >>> as suggest by Lothar. >>> - The clocks are getting disabled and un prepared at the end of the probe. >>> In the suspend the driver is doing a register write.In order >>> To do that register write we have to again enable and prepare the clocks. >> >> Please look the at suspend/resume code and count the >> clock_enable/disable manually. After a suspend/resume cycle, you have >> enabled the clock twice, but disabled it once. >> >> I think you have to abstract the clock handling behind runtime PM. I >> haven't done this myself yet, but the strong feeling that this is a >> possible solution to your problem. These links might help: > > I agree, the clock handling looks weird. Also the clk_disable calls in > xcan_get_berr_counter() look suspicious to me, but I might be wrong. > I think you can take a look at gpio-zynq for an example for runtime_pm > usage. I think the usage model in that driver is similar to here. The xcan_get_berr_counter() function is correct, when doing manual (i.e. non runtime-pm) clock handling. This function might be called if the interface is down, this means clocks are disabled. Marc
On Fri, 2014-11-14 at 04:09PM +0100, Marc Kleine-Budde wrote: > On 11/14/2014 04:05 PM, Sören Brinkmann wrote: > > On Fri, 2014-11-14 at 09:54AM +0100, Marc Kleine-Budde wrote: > >> On 11/14/2014 09:16 AM, Kedareswara rao Appana wrote: > >>> The drvdata in the suspend/resume is of type struct net_device, > >>> not the platform device.Enable the clocks in the suspend before > >>> accessing the registers of the CAN. > >>> > >>> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com> > >>> --- > >>> Changes for v2: > >>> - Removed the struct platform_device* from suspend/resume > >>> as suggest by Lothar. > >>> - The clocks are getting disabled and un prepared at the end of the probe. > >>> In the suspend the driver is doing a register write.In order > >>> To do that register write we have to again enable and prepare the clocks. > >> > >> Please look the at suspend/resume code and count the > >> clock_enable/disable manually. After a suspend/resume cycle, you have > >> enabled the clock twice, but disabled it once. > >> > >> I think you have to abstract the clock handling behind runtime PM. I > >> haven't done this myself yet, but the strong feeling that this is a > >> possible solution to your problem. These links might help: > > > > I agree, the clock handling looks weird. Also the clk_disable calls in > > xcan_get_berr_counter() look suspicious to me, but I might be wrong. > > I think you can take a look at gpio-zynq for an example for runtime_pm > > usage. I think the usage model in that driver is similar to here. > > The xcan_get_berr_counter() function is correct, when doing manual (i.e. > non runtime-pm) clock handling. This function might be called if the > interface is down, this means clocks are disabled. I see, thanks for the clarification. Guess that should become pm_runtime_get_sync() and pm_runtime_put() when converting to runtime_pm. Sören
On 11/14/2014 04:20 PM, Sören Brinkmann wrote: >>>> Please look the at suspend/resume code and count the >>>> clock_enable/disable manually. After a suspend/resume cycle, you have >>>> enabled the clock twice, but disabled it once. >>>> >>>> I think you have to abstract the clock handling behind runtime PM. I >>>> haven't done this myself yet, but the strong feeling that this is a >>>> possible solution to your problem. These links might help: >>> >>> I agree, the clock handling looks weird. Also the clk_disable calls in >>> xcan_get_berr_counter() look suspicious to me, but I might be wrong. >>> I think you can take a look at gpio-zynq for an example for runtime_pm >>> usage. I think the usage model in that driver is similar to here. >> >> The xcan_get_berr_counter() function is correct, when doing manual (i.e. >> non runtime-pm) clock handling. This function might be called if the >> interface is down, this means clocks are disabled. > > I see, thanks for the clarification. Guess that should become > pm_runtime_get_sync() and pm_runtime_put() when converting to > runtime_pm. Yes, as far as I understand runtime pm. Marc
diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c index 5e8b560..485262f 100644 --- a/drivers/net/can/xilinx_can.c +++ b/drivers/net/can/xilinx_can.c @@ -972,15 +972,28 @@ static const struct net_device_ops xcan_netdev_ops = { */ 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); + struct net_device *ndev = dev_get_drvdata(dev); struct xcan_priv *priv = netdev_priv(ndev); + int ret; if (netif_running(ndev)) { netif_stop_queue(ndev); netif_device_detach(ndev); } + ret = clk_prepare_enable(priv->can_clk); + if (ret) { + dev_err(dev, "unable to enable device clock\n"); + return ret; + } + + ret = clk_prepare_enable(priv->bus_clk); + if (ret) { + dev_err(dev, "unable to enable bus clock\n"); + clk_disable_unprepare(priv->can_clk); + return ret; + } + priv->write_reg(priv, XCAN_MSR_OFFSET, XCAN_MSR_SLEEP_MASK); priv->can.state = CAN_STATE_SLEEPING; @@ -999,8 +1012,7 @@ static int __maybe_unused xcan_suspend(struct device *dev) */ static int __maybe_unused xcan_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;
The drvdata in the suspend/resume is of type struct net_device, not the platform device.Enable the clocks in the suspend before accessing the registers of the CAN. Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com> --- Changes for v2: - Removed the struct platform_device* from suspend/resume as suggest by Lothar. - The clocks are getting disabled and un prepared at the end of the probe. In the suspend the driver is doing a register write.In order To do that register write we have to again enable and prepare the clocks. drivers/net/can/xilinx_can.c | 20 ++++++++++++++++---- 1 files changed, 16 insertions(+), 4 deletions(-)