[PATCH v2] can: Fix bug in suspend/resume

[PATCH v2] can: Fix bug in suspend/resume

[Kedareswara rao Appana]

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(-)

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;
 
-- 
1.7.4

Re: [PATCH v2] can: Fix bug in suspend/resume

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1517 bytes --]

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
-- 
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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] can: Fix bug in suspend/resume

[Sören Brinkmann]

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

Re: [PATCH v2] can: Fix bug in suspend/resume

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1958 bytes --]

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


-- 
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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] can: Fix bug in suspend/resume

[Sören Brinkmann]

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

Re: [PATCH v2] can: Fix bug in suspend/resume

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1445 bytes --]

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

-- 
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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]