* [PATCH] can: Fix bug in suspend/resume
@ 2014-11-13 6:58 ` Kedareswara rao Appana
0 siblings, 0 replies; 18+ messages in thread
From: Kedareswara rao Appana @ 2014-11-13 6:58 UTC (permalink / raw)
To: wg, mkl, michal.simek, soren.brinkmann, grant.likely, robh+dt
Cc: devicetree, netdev, linux-kernel, linux-can,
Kedareswara rao Appana, linux-arm-kernel
When accessing the priv structure use container_of instead of dev_get_drvdata.
Enable the clocks in the suspend before accessing the registers of the CAN.
Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
---
drivers/net/can/xilinx_can.c | 20 ++++++++++++++++++--
1 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 5e8b560..63ef645 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -972,15 +972,30 @@ 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 platform_device *pdev = container_of(dev,
+ struct platform_device, dev);
struct net_device *ndev = platform_get_drvdata(pdev);
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,7 +1014,8 @@ 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 platform_device *pdev = container_of(dev,
+ struct platform_device, dev);
struct net_device *ndev = platform_get_drvdata(pdev);
struct xcan_priv *priv = netdev_priv(ndev);
int ret;
--
1.7.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH] can: Fix bug in suspend/resume
@ 2014-11-13 6:58 ` Kedareswara rao Appana
0 siblings, 0 replies; 18+ messages in thread
From: Kedareswara rao Appana @ 2014-11-13 6:58 UTC (permalink / raw)
To: wg, mkl, michal.simek, soren.brinkmann, grant.likely, robh+dt
Cc: linux-can, netdev, linux-arm-kernel, linux-kernel, devicetree,
Kedareswara rao Appana
When accessing the priv structure use container_of instead of dev_get_drvdata.
Enable the clocks in the suspend before accessing the registers of the CAN.
Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
---
drivers/net/can/xilinx_can.c | 20 ++++++++++++++++++--
1 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 5e8b560..63ef645 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -972,15 +972,30 @@ 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 platform_device *pdev = container_of(dev,
+ struct platform_device, dev);
struct net_device *ndev = platform_get_drvdata(pdev);
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,7 +1014,8 @@ 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 platform_device *pdev = container_of(dev,
+ struct platform_device, dev);
struct net_device *ndev = platform_get_drvdata(pdev);
struct xcan_priv *priv = netdev_priv(ndev);
int ret;
--
1.7.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH] can: Fix bug in suspend/resume
@ 2014-11-13 6:58 ` Kedareswara rao Appana
0 siblings, 0 replies; 18+ messages in thread
From: Kedareswara rao Appana @ 2014-11-13 6:58 UTC (permalink / raw)
To: linux-arm-kernel
When accessing the priv structure use container_of instead of dev_get_drvdata.
Enable the clocks in the suspend before accessing the registers of the CAN.
Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
---
drivers/net/can/xilinx_can.c | 20 ++++++++++++++++++--
1 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 5e8b560..63ef645 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -972,15 +972,30 @@ 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 platform_device *pdev = container_of(dev,
+ struct platform_device, dev);
struct net_device *ndev = platform_get_drvdata(pdev);
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,7 +1014,8 @@ 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 platform_device *pdev = container_of(dev,
+ struct platform_device, dev);
struct net_device *ndev = platform_get_drvdata(pdev);
struct xcan_priv *priv = netdev_priv(ndev);
int ret;
--
1.7.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH] can: Fix bug in suspend/resume
2014-11-13 6:58 ` Kedareswara rao Appana
@ 2014-11-13 10:15 ` Marc Kleine-Budde
-1 siblings, 0 replies; 18+ messages in thread
From: Marc Kleine-Budde @ 2014-11-13 10:15 UTC (permalink / raw)
To: Kedareswara rao Appana, wg, michal.simek, soren.brinkmann,
grant.likely, robh+dt
Cc: linux-can, netdev, linux-arm-kernel, linux-kernel, devicetree,
Kedareswara rao Appana
[-- Attachment #1: Type: text/plain, Size: 2424 bytes --]
On 11/13/2014 07:58 AM, Kedareswara rao Appana wrote:
> When accessing the priv structure use container_of instead of dev_get_drvdata.
Why?
> Enable the clocks in the suspend before accessing the registers of the CAN.
>
> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> ---
> drivers/net/can/xilinx_can.c | 20 ++++++++++++++++++--
> 1 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 5e8b560..63ef645 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -972,15 +972,30 @@ 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 platform_device *pdev = container_of(dev,
> + struct platform_device, dev);
> struct net_device *ndev = platform_get_drvdata(pdev);
> 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;
> + }
Now you have clock imbalance. Per suspend/resume cycle the clocks are
enabled twice, but disabled only once.
> +
> priv->write_reg(priv, XCAN_MSR_OFFSET, XCAN_MSR_SLEEP_MASK);
> priv->can.state = CAN_STATE_SLEEPING;
>
> @@ -999,7 +1014,8 @@ 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 platform_device *pdev = container_of(dev,
> + struct platform_device, dev);
> struct net_device *ndev = platform_get_drvdata(pdev);
> struct xcan_priv *priv = netdev_priv(ndev);
> int ret;
>
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 --]
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH] can: Fix bug in suspend/resume
@ 2014-11-13 10:15 ` Marc Kleine-Budde
0 siblings, 0 replies; 18+ messages in thread
From: Marc Kleine-Budde @ 2014-11-13 10:15 UTC (permalink / raw)
To: linux-arm-kernel
On 11/13/2014 07:58 AM, Kedareswara rao Appana wrote:
> When accessing the priv structure use container_of instead of dev_get_drvdata.
Why?
> Enable the clocks in the suspend before accessing the registers of the CAN.
>
> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> ---
> drivers/net/can/xilinx_can.c | 20 ++++++++++++++++++--
> 1 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 5e8b560..63ef645 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -972,15 +972,30 @@ 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 platform_device *pdev = container_of(dev,
> + struct platform_device, dev);
> struct net_device *ndev = platform_get_drvdata(pdev);
> 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;
> + }
Now you have clock imbalance. Per suspend/resume cycle the clocks are
enabled twice, but disabled only once.
> +
> priv->write_reg(priv, XCAN_MSR_OFFSET, XCAN_MSR_SLEEP_MASK);
> priv->can.state = CAN_STATE_SLEEPING;
>
> @@ -999,7 +1014,8 @@ 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 platform_device *pdev = container_of(dev,
> + struct platform_device, dev);
> struct net_device *ndev = platform_get_drvdata(pdev);
> struct xcan_priv *priv = netdev_priv(ndev);
> int ret;
>
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 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141113/ce32ab24/attachment.sig>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] can: Fix bug in suspend/resume
2014-11-13 10:15 ` Marc Kleine-Budde
@ 2014-11-13 10:46 ` Marc Kleine-Budde
-1 siblings, 0 replies; 18+ messages in thread
From: Marc Kleine-Budde @ 2014-11-13 10:46 UTC (permalink / raw)
To: Kedareswara rao Appana, wg, michal.simek, soren.brinkmann,
grant.likely, robh+dt
Cc: linux-can, netdev, linux-arm-kernel, linux-kernel, devicetree,
Kedareswara rao Appana
[-- Attachment #1: Type: text/plain, Size: 2840 bytes --]
On 11/13/2014 11:15 AM, Marc Kleine-Budde wrote:
> On 11/13/2014 07:58 AM, Kedareswara rao Appana wrote:
>> When accessing the priv structure use container_of instead of dev_get_drvdata.
>
> Why?
The drvdata here is the struct net_device, not the platform device.
Please state this in the commit message.
If I understand the code correct, you can make use of the existing
helper function to_platform_device():
http://lxr.free-electrons.com/source/include/linux/platform_device.h#L42
>
>> Enable the clocks in the suspend before accessing the registers of the CAN.
>>
>> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
>> ---
>> drivers/net/can/xilinx_can.c | 20 ++++++++++++++++++--
>> 1 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
>> index 5e8b560..63ef645 100644
>> --- a/drivers/net/can/xilinx_can.c
>> +++ b/drivers/net/can/xilinx_can.c
>> @@ -972,15 +972,30 @@ 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 platform_device *pdev = container_of(dev,
>> + struct platform_device, dev);
>> struct net_device *ndev = platform_get_drvdata(pdev);
>> 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;
>> + }
>
> Now you have clock imbalance. Per suspend/resume cycle the clocks are
> enabled twice, but disabled only once.
>
>> +
>> priv->write_reg(priv, XCAN_MSR_OFFSET, XCAN_MSR_SLEEP_MASK);
>> priv->can.state = CAN_STATE_SLEEPING;
>>
>> @@ -999,7 +1014,8 @@ 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 platform_device *pdev = container_of(dev,
>> + struct platform_device, dev);
>> struct net_device *ndev = platform_get_drvdata(pdev);
>> struct xcan_priv *priv = netdev_priv(ndev);
>> int ret;
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 --]
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH] can: Fix bug in suspend/resume
@ 2014-11-13 10:46 ` Marc Kleine-Budde
0 siblings, 0 replies; 18+ messages in thread
From: Marc Kleine-Budde @ 2014-11-13 10:46 UTC (permalink / raw)
To: linux-arm-kernel
On 11/13/2014 11:15 AM, Marc Kleine-Budde wrote:
> On 11/13/2014 07:58 AM, Kedareswara rao Appana wrote:
>> When accessing the priv structure use container_of instead of dev_get_drvdata.
>
> Why?
The drvdata here is the struct net_device, not the platform device.
Please state this in the commit message.
If I understand the code correct, you can make use of the existing
helper function to_platform_device():
http://lxr.free-electrons.com/source/include/linux/platform_device.h#L42
>
>> Enable the clocks in the suspend before accessing the registers of the CAN.
>>
>> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
>> ---
>> drivers/net/can/xilinx_can.c | 20 ++++++++++++++++++--
>> 1 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
>> index 5e8b560..63ef645 100644
>> --- a/drivers/net/can/xilinx_can.c
>> +++ b/drivers/net/can/xilinx_can.c
>> @@ -972,15 +972,30 @@ 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 platform_device *pdev = container_of(dev,
>> + struct platform_device, dev);
>> struct net_device *ndev = platform_get_drvdata(pdev);
>> 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;
>> + }
>
> Now you have clock imbalance. Per suspend/resume cycle the clocks are
> enabled twice, but disabled only once.
>
>> +
>> priv->write_reg(priv, XCAN_MSR_OFFSET, XCAN_MSR_SLEEP_MASK);
>> priv->can.state = CAN_STATE_SLEEPING;
>>
>> @@ -999,7 +1014,8 @@ 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 platform_device *pdev = container_of(dev,
>> + struct platform_device, dev);
>> struct net_device *ndev = platform_get_drvdata(pdev);
>> struct xcan_priv *priv = netdev_priv(ndev);
>> int ret;
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 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141113/fb52e0ee/attachment.sig>
^ permalink raw reply [flat|nested] 18+ messages in thread[parent not found: <54648BF4.60007-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]
* RE: [PATCH] can: Fix bug in suspend/resume
2014-11-13 10:46 ` Marc Kleine-Budde
(?)
@ 2014-11-13 10:58 ` Appana Durga Kedareswara Rao
-1 siblings, 0 replies; 18+ messages in thread
From: Appana Durga Kedareswara Rao @ 2014-11-13 10:58 UTC (permalink / raw)
To: Marc Kleine-Budde, wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org,
Michal Simek, Soren Brinkmann,
grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Cc: linux-can-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Hi Marc,
-----Original Message-----
From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
Sent: Thursday, November 13, 2014 4: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] can: Fix bug in suspend/resume
On 11/13/2014 11:15 AM, Marc Kleine-Budde wrote:
> On 11/13/2014 07:58 AM, Kedareswara rao Appana wrote:
>> When accessing the priv structure use container_of instead of dev_get_drvdata.
>
> Why?
The drvdata here is the struct net_device, not the platform device.
Please state this in the commit message.
If I understand the code correct, you can make use of the existing helper function to_platform_device():
http://lxr.free-electrons.com/source/include/linux/platform_device.h#L42
Thanks for the suggestion.
Will use this macro(to_platform_device) .
>
>> Enable the clocks in the suspend before accessing the registers of the CAN.
>>
>> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
>> ---
>> drivers/net/can/xilinx_can.c | 20 ++++++++++++++++++--
>> 1 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/xilinx_can.c
>> b/drivers/net/can/xilinx_can.c index 5e8b560..63ef645 100644
>> --- a/drivers/net/can/xilinx_can.c
>> +++ b/drivers/net/can/xilinx_can.c
>> @@ -972,15 +972,30 @@ 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 platform_device *pdev = container_of(dev,
>> + struct platform_device, dev);
>> struct net_device *ndev = platform_get_drvdata(pdev);
>> 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;
>> + }
>
> Now you have clock imbalance. Per suspend/resume cycle the clocks are
> enabled twice, but disabled only once.
>
The clocks are getting disabled and un prepared at the end of the probe.
In the resume the driver is doing register write.
In order to do that register write I have to again enable and prepare the clocks.
Regards,
Kedar.
>> +
>> priv->write_reg(priv, XCAN_MSR_OFFSET, XCAN_MSR_SLEEP_MASK);
>> priv->can.state = CAN_STATE_SLEEPING;
>>
>> @@ -999,7 +1014,8 @@ 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 platform_device *pdev = container_of(dev,
>> + struct platform_device, dev);
>> struct net_device *ndev = platform_get_drvdata(pdev);
>> struct xcan_priv *priv = netdev_priv(ndev);
>> int ret;
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.
^ permalink raw reply [flat|nested] 18+ messages in thread* RE: [PATCH] can: Fix bug in suspend/resume
@ 2014-11-13 10:58 ` Appana Durga Kedareswara Rao
0 siblings, 0 replies; 18+ messages in thread
From: Appana Durga Kedareswara Rao @ 2014-11-13 10:58 UTC (permalink / raw)
To: Marc Kleine-Budde, 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
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4224 bytes --]
Hi Marc,
-----Original Message-----
From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
Sent: Thursday, November 13, 2014 4: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] can: Fix bug in suspend/resume
On 11/13/2014 11:15 AM, Marc Kleine-Budde wrote:
> On 11/13/2014 07:58 AM, Kedareswara rao Appana wrote:
>> When accessing the priv structure use container_of instead of dev_get_drvdata.
>
> Why?
The drvdata here is the struct net_device, not the platform device.
Please state this in the commit message.
If I understand the code correct, you can make use of the existing helper function to_platform_device():
http://lxr.free-electrons.com/source/include/linux/platform_device.h#L42
Thanks for the suggestion.
Will use this macro(to_platform_device) .
>
>> Enable the clocks in the suspend before accessing the registers of the CAN.
>>
>> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
>> ---
>> drivers/net/can/xilinx_can.c | 20 ++++++++++++++++++--
>> 1 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/xilinx_can.c
>> b/drivers/net/can/xilinx_can.c index 5e8b560..63ef645 100644
>> --- a/drivers/net/can/xilinx_can.c
>> +++ b/drivers/net/can/xilinx_can.c
>> @@ -972,15 +972,30 @@ 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 platform_device *pdev = container_of(dev,
>> + struct platform_device, dev);
>> struct net_device *ndev = platform_get_drvdata(pdev);
>> 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;
>> + }
>
> Now you have clock imbalance. Per suspend/resume cycle the clocks are
> enabled twice, but disabled only once.
>
The clocks are getting disabled and un prepared at the end of the probe.
In the resume the driver is doing register write.
In order to do that register write I have to again enable and prepare the clocks.
Regards,
Kedar.
>> +
>> priv->write_reg(priv, XCAN_MSR_OFFSET, XCAN_MSR_SLEEP_MASK);
>> priv->can.state = CAN_STATE_SLEEPING;
>>
>> @@ -999,7 +1014,8 @@ 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 platform_device *pdev = container_of(dev,
>> + struct platform_device, dev);
>> struct net_device *ndev = platform_get_drvdata(pdev);
>> struct xcan_priv *priv = netdev_priv(ndev);
>> int ret;
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.
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH] can: Fix bug in suspend/resume
@ 2014-11-13 10:58 ` Appana Durga Kedareswara Rao
0 siblings, 0 replies; 18+ messages in thread
From: Appana Durga Kedareswara Rao @ 2014-11-13 10:58 UTC (permalink / raw)
To: linux-arm-kernel
Hi Marc,
-----Original Message-----
From: Marc Kleine-Budde [mailto:mkl at pengutronix.de]
Sent: Thursday, November 13, 2014 4:16 PM
To: Appana Durga Kedareswara Rao; wg at grandegger.com; Michal Simek; Soren Brinkmann; grant.likely at linaro.org; robh+dt at kernel.org
Cc: linux-can at vger.kernel.org; netdev at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; devicetree at vger.kernel.org; Appana Durga Kedareswara Rao
Subject: Re: [PATCH] can: Fix bug in suspend/resume
On 11/13/2014 11:15 AM, Marc Kleine-Budde wrote:
> On 11/13/2014 07:58 AM, Kedareswara rao Appana wrote:
>> When accessing the priv structure use container_of instead of dev_get_drvdata.
>
> Why?
The drvdata here is the struct net_device, not the platform device.
Please state this in the commit message.
If I understand the code correct, you can make use of the existing helper function to_platform_device():
http://lxr.free-electrons.com/source/include/linux/platform_device.h#L42
Thanks for the suggestion.
Will use this macro(to_platform_device) .
>
>> Enable the clocks in the suspend before accessing the registers of the CAN.
>>
>> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
>> ---
>> drivers/net/can/xilinx_can.c | 20 ++++++++++++++++++--
>> 1 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/xilinx_can.c
>> b/drivers/net/can/xilinx_can.c index 5e8b560..63ef645 100644
>> --- a/drivers/net/can/xilinx_can.c
>> +++ b/drivers/net/can/xilinx_can.c
>> @@ -972,15 +972,30 @@ 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 platform_device *pdev = container_of(dev,
>> + struct platform_device, dev);
>> struct net_device *ndev = platform_get_drvdata(pdev);
>> 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;
>> + }
>
> Now you have clock imbalance. Per suspend/resume cycle the clocks are
> enabled twice, but disabled only once.
>
The clocks are getting disabled and un prepared at the end of the probe.
In the resume the driver is doing register write.
In order to do that register write I have to again enable and prepare the clocks.
Regards,
Kedar.
>> +
>> priv->write_reg(priv, XCAN_MSR_OFFSET, XCAN_MSR_SLEEP_MASK);
>> priv->can.state = CAN_STATE_SLEEPING;
>>
>> @@ -999,7 +1014,8 @@ 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 platform_device *pdev = container_of(dev,
>> + struct platform_device, dev);
>> struct net_device *ndev = platform_get_drvdata(pdev);
>> struct xcan_priv *priv = netdev_priv(ndev);
>> int ret;
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.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] can: Fix bug in suspend/resume
2014-11-13 10:58 ` Appana Durga Kedareswara Rao
@ 2014-11-13 11:32 ` Lothar Waßmann
-1 siblings, 0 replies; 18+ messages in thread
From: Lothar Waßmann @ 2014-11-13 11:32 UTC (permalink / raw)
To: Appana Durga Kedareswara Rao
Cc: Marc Kleine-Budde, wg@grandegger.com, Michal Simek,
Soren Brinkmann, grant.likely@linaro.org, robh+dt@kernel.org,
netdev@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, linux-can@vger.kernel.org
Hi,
Appana Durga Kedareswara Rao wrote:
> Hi Marc,
>
> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Thursday, November 13, 2014 4: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] can: Fix bug in suspend/resume
>
> On 11/13/2014 11:15 AM, Marc Kleine-Budde wrote:
> > On 11/13/2014 07:58 AM, Kedareswara rao Appana wrote:
> >> When accessing the priv structure use container_of instead of dev_get_drvdata.
> >
> > Why?
>
> The drvdata here is the struct net_device, not the platform device.
> Please state this in the commit message.
>
> If I understand the code correct, you can make use of the existing helper function to_platform_device():
>
> http://lxr.free-electrons.com/source/include/linux/platform_device.h#L42
>
> Thanks for the suggestion.
> Will use this macro(to_platform_device) .
>
> >
> >> Enable the clocks in the suspend before accessing the registers of the CAN.
> >>
> >> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> >> ---
> >> drivers/net/can/xilinx_can.c | 20 ++++++++++++++++++--
> >> 1 files changed, 18 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/can/xilinx_can.c
> >> b/drivers/net/can/xilinx_can.c index 5e8b560..63ef645 100644
> >> --- a/drivers/net/can/xilinx_can.c
> >> +++ b/drivers/net/can/xilinx_can.c
> >> @@ -972,15 +972,30 @@ 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 platform_device *pdev = container_of(dev,
> >> + struct platform_device, dev);
> >> struct net_device *ndev = platform_get_drvdata(pdev);
> >> struct xcan_priv *priv = netdev_priv(ndev);
>
Why not simply:
struct net_device *ndev = dev_get_drvdata(dev);
There is no need for a struct platform_device* at all.
Lothar Waßmann
--
___________________________________________________________
Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH] can: Fix bug in suspend/resume
@ 2014-11-13 11:32 ` Lothar Waßmann
0 siblings, 0 replies; 18+ messages in thread
From: Lothar Waßmann @ 2014-11-13 11:32 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
Appana Durga Kedareswara Rao wrote:
> Hi Marc,
>
> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl at pengutronix.de]
> Sent: Thursday, November 13, 2014 4:16 PM
> To: Appana Durga Kedareswara Rao; wg at grandegger.com; Michal Simek; Soren Brinkmann; grant.likely at linaro.org; robh+dt at kernel.org
> Cc: linux-can at vger.kernel.org; netdev at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; devicetree at vger.kernel.org; Appana Durga Kedareswara Rao
> Subject: Re: [PATCH] can: Fix bug in suspend/resume
>
> On 11/13/2014 11:15 AM, Marc Kleine-Budde wrote:
> > On 11/13/2014 07:58 AM, Kedareswara rao Appana wrote:
> >> When accessing the priv structure use container_of instead of dev_get_drvdata.
> >
> > Why?
>
> The drvdata here is the struct net_device, not the platform device.
> Please state this in the commit message.
>
> If I understand the code correct, you can make use of the existing helper function to_platform_device():
>
> http://lxr.free-electrons.com/source/include/linux/platform_device.h#L42
>
> Thanks for the suggestion.
> Will use this macro(to_platform_device) .
>
> >
> >> Enable the clocks in the suspend before accessing the registers of the CAN.
> >>
> >> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> >> ---
> >> drivers/net/can/xilinx_can.c | 20 ++++++++++++++++++--
> >> 1 files changed, 18 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/can/xilinx_can.c
> >> b/drivers/net/can/xilinx_can.c index 5e8b560..63ef645 100644
> >> --- a/drivers/net/can/xilinx_can.c
> >> +++ b/drivers/net/can/xilinx_can.c
> >> @@ -972,15 +972,30 @@ 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 platform_device *pdev = container_of(dev,
> >> + struct platform_device, dev);
> >> struct net_device *ndev = platform_get_drvdata(pdev);
> >> struct xcan_priv *priv = netdev_priv(ndev);
>
Why not simply:
struct net_device *ndev = dev_get_drvdata(dev);
There is no need for a struct platform_device* at all.
Lothar Wa?mann
--
___________________________________________________________
Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] can: Fix bug in suspend/resume
2014-11-13 11:32 ` Lothar Waßmann
@ 2014-11-13 12:03 ` Marc Kleine-Budde
-1 siblings, 0 replies; 18+ messages in thread
From: Marc Kleine-Budde @ 2014-11-13 12:03 UTC (permalink / raw)
To: Lothar Waßmann, Appana Durga Kedareswara Rao
Cc: wg@grandegger.com, Michal Simek, Soren Brinkmann,
grant.likely@linaro.org, robh+dt@kernel.org,
netdev@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, linux-can@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 847 bytes --]
On 11/13/2014 12:32 PM, Lothar Waßmann wrote:
>>>> static int __maybe_unused xcan_suspend(struct device *dev) {
>>>> - struct platform_device *pdev = dev_get_drvdata(dev);
>>>> + struct platform_device *pdev = container_of(dev,
>>>> + struct platform_device, dev);
>>>> struct net_device *ndev = platform_get_drvdata(pdev);
>>>> struct xcan_priv *priv = netdev_priv(ndev);
>>
> Why not simply:
> struct net_device *ndev = dev_get_drvdata(dev);
>
> There is no need for a struct platform_device* at all.
ACK
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 --]
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH] can: Fix bug in suspend/resume
@ 2014-11-13 12:03 ` Marc Kleine-Budde
0 siblings, 0 replies; 18+ messages in thread
From: Marc Kleine-Budde @ 2014-11-13 12:03 UTC (permalink / raw)
To: linux-arm-kernel
On 11/13/2014 12:32 PM, Lothar Wa?mann wrote:
>>>> static int __maybe_unused xcan_suspend(struct device *dev) {
>>>> - struct platform_device *pdev = dev_get_drvdata(dev);
>>>> + struct platform_device *pdev = container_of(dev,
>>>> + struct platform_device, dev);
>>>> struct net_device *ndev = platform_get_drvdata(pdev);
>>>> struct xcan_priv *priv = netdev_priv(ndev);
>>
> Why not simply:
> struct net_device *ndev = dev_get_drvdata(dev);
>
> There is no need for a struct platform_device* at all.
ACK
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 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141113/7020cd38/attachment.sig>
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH] can: Fix bug in suspend/resume
2014-11-13 10:46 ` Marc Kleine-Budde
(?)
@ 2014-11-13 11:01 ` Appana Durga Kedareswara Rao
-1 siblings, 0 replies; 18+ messages in thread
From: Appana Durga Kedareswara Rao @ 2014-11-13 11:01 UTC (permalink / raw)
To: Marc Kleine-Budde, 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
Hi Marc,
-----Original Message-----
From: Appana Durga Kedareswara Rao
Sent: Thursday, November 13, 2014 4:28 PM
To: 'Marc Kleine-Budde'; 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
Subject: RE: [PATCH] can: Fix bug in suspend/resume
Hi Marc,
-----Original Message-----
From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
Sent: Thursday, November 13, 2014 4: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] can: Fix bug in suspend/resume
On 11/13/2014 11:15 AM, Marc Kleine-Budde wrote:
> On 11/13/2014 07:58 AM, Kedareswara rao Appana wrote:
>> When accessing the priv structure use container_of instead of dev_get_drvdata.
>
> Why?
The drvdata here is the struct net_device, not the platform device.
Please state this in the commit message.
If I understand the code correct, you can make use of the existing helper function to_platform_device():
http://lxr.free-electrons.com/source/include/linux/platform_device.h#L42
Thanks for the suggestion.
Will use this macro(to_platform_device) .
>
>> Enable the clocks in the suspend before accessing the registers of the CAN.
>>
>> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
>> ---
>> drivers/net/can/xilinx_can.c | 20 ++++++++++++++++++--
>> 1 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/xilinx_can.c
>> b/drivers/net/can/xilinx_can.c index 5e8b560..63ef645 100644
>> --- a/drivers/net/can/xilinx_can.c
>> +++ b/drivers/net/can/xilinx_can.c
>> @@ -972,15 +972,30 @@ 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 platform_device *pdev = container_of(dev,
>> + struct platform_device, dev);
>> struct net_device *ndev = platform_get_drvdata(pdev);
>> 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;
>> + }
>
> Now you have clock imbalance. Per suspend/resume cycle the clocks are
> enabled twice, but disabled only once.
>
The clocks are getting disabled and un prepared at the end of the probe.
In the resume the driver is doing register write.
Sorry spell mistake not in resume in the suspend I am doing a register write.
In order to do that register write I have to again enable and prepare the clocks.
Regards,
Kedar.
Regards,
Kedar.
>> +
>> priv->write_reg(priv, XCAN_MSR_OFFSET, XCAN_MSR_SLEEP_MASK);
>> priv->can.state = CAN_STATE_SLEEPING;
>>
>> @@ -999,7 +1014,8 @@ 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 platform_device *pdev = container_of(dev,
>> + struct platform_device, dev);
>> struct net_device *ndev = platform_get_drvdata(pdev);
>> struct xcan_priv *priv = netdev_priv(ndev);
>> int ret;
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.
^ permalink raw reply [flat|nested] 18+ messages in thread* RE: [PATCH] can: Fix bug in suspend/resume
@ 2014-11-13 11:01 ` Appana Durga Kedareswara Rao
0 siblings, 0 replies; 18+ messages in thread
From: Appana Durga Kedareswara Rao @ 2014-11-13 11:01 UTC (permalink / raw)
To: Marc Kleine-Budde, 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
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4759 bytes --]
Hi Marc,
-----Original Message-----
From: Appana Durga Kedareswara Rao
Sent: Thursday, November 13, 2014 4:28 PM
To: 'Marc Kleine-Budde'; 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
Subject: RE: [PATCH] can: Fix bug in suspend/resume
Hi Marc,
-----Original Message-----
From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
Sent: Thursday, November 13, 2014 4: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] can: Fix bug in suspend/resume
On 11/13/2014 11:15 AM, Marc Kleine-Budde wrote:
> On 11/13/2014 07:58 AM, Kedareswara rao Appana wrote:
>> When accessing the priv structure use container_of instead of dev_get_drvdata.
>
> Why?
The drvdata here is the struct net_device, not the platform device.
Please state this in the commit message.
If I understand the code correct, you can make use of the existing helper function to_platform_device():
http://lxr.free-electrons.com/source/include/linux/platform_device.h#L42
Thanks for the suggestion.
Will use this macro(to_platform_device) .
>
>> Enable the clocks in the suspend before accessing the registers of the CAN.
>>
>> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
>> ---
>> drivers/net/can/xilinx_can.c | 20 ++++++++++++++++++--
>> 1 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/xilinx_can.c
>> b/drivers/net/can/xilinx_can.c index 5e8b560..63ef645 100644
>> --- a/drivers/net/can/xilinx_can.c
>> +++ b/drivers/net/can/xilinx_can.c
>> @@ -972,15 +972,30 @@ 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 platform_device *pdev = container_of(dev,
>> + struct platform_device, dev);
>> struct net_device *ndev = platform_get_drvdata(pdev);
>> 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;
>> + }
>
> Now you have clock imbalance. Per suspend/resume cycle the clocks are
> enabled twice, but disabled only once.
>
The clocks are getting disabled and un prepared at the end of the probe.
In the resume the driver is doing register write.
Sorry spell mistake not in resume in the suspend I am doing a register write.
In order to do that register write I have to again enable and prepare the clocks.
Regards,
Kedar.
Regards,
Kedar.
>> +
>> priv->write_reg(priv, XCAN_MSR_OFFSET, XCAN_MSR_SLEEP_MASK);
>> priv->can.state = CAN_STATE_SLEEPING;
>>
>> @@ -999,7 +1014,8 @@ 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 platform_device *pdev = container_of(dev,
>> + struct platform_device, dev);
>> struct net_device *ndev = platform_get_drvdata(pdev);
>> struct xcan_priv *priv = netdev_priv(ndev);
>> int ret;
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.
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH] can: Fix bug in suspend/resume
@ 2014-11-13 11:01 ` Appana Durga Kedareswara Rao
0 siblings, 0 replies; 18+ messages in thread
From: Appana Durga Kedareswara Rao @ 2014-11-13 11:01 UTC (permalink / raw)
To: linux-arm-kernel
Hi Marc,
-----Original Message-----
From: Appana Durga Kedareswara Rao
Sent: Thursday, November 13, 2014 4:28 PM
To: 'Marc Kleine-Budde'; wg at grandegger.com; Michal Simek; Soren Brinkmann; grant.likely at linaro.org; robh+dt at kernel.org
Cc: linux-can at vger.kernel.org; netdev at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; devicetree at vger.kernel.org
Subject: RE: [PATCH] can: Fix bug in suspend/resume
Hi Marc,
-----Original Message-----
From: Marc Kleine-Budde [mailto:mkl at pengutronix.de]
Sent: Thursday, November 13, 2014 4:16 PM
To: Appana Durga Kedareswara Rao; wg at grandegger.com; Michal Simek; Soren Brinkmann; grant.likely at linaro.org; robh+dt at kernel.org
Cc: linux-can at vger.kernel.org; netdev at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; devicetree at vger.kernel.org; Appana Durga Kedareswara Rao
Subject: Re: [PATCH] can: Fix bug in suspend/resume
On 11/13/2014 11:15 AM, Marc Kleine-Budde wrote:
> On 11/13/2014 07:58 AM, Kedareswara rao Appana wrote:
>> When accessing the priv structure use container_of instead of dev_get_drvdata.
>
> Why?
The drvdata here is the struct net_device, not the platform device.
Please state this in the commit message.
If I understand the code correct, you can make use of the existing helper function to_platform_device():
http://lxr.free-electrons.com/source/include/linux/platform_device.h#L42
Thanks for the suggestion.
Will use this macro(to_platform_device) .
>
>> Enable the clocks in the suspend before accessing the registers of the CAN.
>>
>> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
>> ---
>> drivers/net/can/xilinx_can.c | 20 ++++++++++++++++++--
>> 1 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/xilinx_can.c
>> b/drivers/net/can/xilinx_can.c index 5e8b560..63ef645 100644
>> --- a/drivers/net/can/xilinx_can.c
>> +++ b/drivers/net/can/xilinx_can.c
>> @@ -972,15 +972,30 @@ 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 platform_device *pdev = container_of(dev,
>> + struct platform_device, dev);
>> struct net_device *ndev = platform_get_drvdata(pdev);
>> 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;
>> + }
>
> Now you have clock imbalance. Per suspend/resume cycle the clocks are
> enabled twice, but disabled only once.
>
The clocks are getting disabled and un prepared at the end of the probe.
In the resume the driver is doing register write.
Sorry spell mistake not in resume in the suspend I am doing a register write.
In order to do that register write I have to again enable and prepare the clocks.
Regards,
Kedar.
Regards,
Kedar.
>> +
>> priv->write_reg(priv, XCAN_MSR_OFFSET, XCAN_MSR_SLEEP_MASK);
>> priv->can.state = CAN_STATE_SLEEPING;
>>
>> @@ -999,7 +1014,8 @@ 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 platform_device *pdev = container_of(dev,
>> + struct platform_device, dev);
>> struct net_device *ndev = platform_get_drvdata(pdev);
>> struct xcan_priv *priv = netdev_priv(ndev);
>> int ret;
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.
^ permalink raw reply [flat|nested] 18+ messages in thread