RE: [PATCH v5] can: c_can: Add runtime PM support to Bosch C_CAN/D_CAN controller
Hi Marc,
Thanks for the review
On Tue, Aug 07, 2012 at 17:27:05, Marc Kleine-Budde wrote:
On 08/07/2012 01:20 PM, AnilKumar Ch wrote:
Add Runtime PM support to C_CAN/D_CAN controller. The runtime PM
APIs control clocks for C_CAN/D_CAN IP and prevent access to the
register of C_CAN/D_CAN IP when clock is turned off.
Signed-off-by: AnilKumar Ch anilku...@ti.com
---
This patch has been tested on AM335X EVM. Due to lack of hardware
I am not able to test c_can functionality. I appreciate if anyone
can test c_can functionality with this patch.
The priv-pdev-dev will not work for c_can/d_can which are not using
the platform driver. Options:
a) priv-dev
add a struct device pointer to the priv, and
1) guard pm_runtime_get_sync() for priv-dev == NULL
(or check if pm_runtime_get_sync copes with NULL pointer)
- or -
This is a better option but to implement this current priv-dev
needs to move to priv-ndev. Then we can add dev pointer to priv
struct.
2) fill in the priv-dev in all other drivers (i.e. PCI)
and check if it makes sense/works to call it on PCI hardware
b) convert all other drivers, i.e. the PCI driver to create a
platform device
Without thinking long, I suggest to a) with flavour 1)
Maybe there are better solutions.
Marc
More comments inline:
This patch is based on can-next/master
Changes from v4:
- Incorporated Vaibhav H review comments on v4.
* Moved pm_runtime put/get_sync calls to appropriate positions.
- This patch is from Add DT support to C_CAN/D_CAN controller
patch series. Rest of the patches in this series were applied
so this v5 contains only this patch.
drivers/net/can/c_can/c_can.c |8
drivers/net/can/c_can/c_can.h |1 +
drivers/net/can/c_can/c_can_platform.c | 11 +++
3 files changed, 20 insertions(+)
diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index eea6608..ca1151d 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -34,6 +34,8 @@
#include linux/if_ether.h
#include linux/list.h
#include linux/io.h
+#include linux/pm_runtime.h
+#include linux/platform_device.h
#include linux/can.h
#include linux/can/dev.h
@@ -673,6 +675,8 @@ static int c_can_get_berr_counter(const struct
net_device *dev,
unsigned int reg_err_counter;
struct c_can_priv *priv = netdev_priv(dev);
+ pm_runtime_get_sync(priv-pdev-dev);
+
Why do you make a get_sync() here, where's the correspondning put_sync()?
While setting the bitrate, top layers checks the status of
C_CAN/D_CAN BERR register to know the bit status. I will take
care properly.
reg_err_counter = priv-read_reg(priv, C_CAN_ERR_CNT_REG);
bec-rxerr = (reg_err_counter ERR_CNT_REC_MASK)
ERR_CNT_REC_SHIFT;
@@ -1053,6 +1057,8 @@ static int c_can_open(struct net_device *dev)
int err;
struct c_can_priv *priv = netdev_priv(dev);
+ pm_runtime_get_sync(priv-pdev-dev);
+
/* open the can device */
err = open_candev(dev);
if (err) {
What if open_candev() fails? Please add a jump label
exit_open_candev_fail and add thje put_sync() there.
Missed, I will take care in return path
Regards
AnilKumar
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5] can: c_can: Add runtime PM support to Bosch C_CAN/D_CAN controller
Add Runtime PM support to C_CAN/D_CAN controller. The runtime PM
APIs control clocks for C_CAN/D_CAN IP and prevent access to the
register of C_CAN/D_CAN IP when clock is turned off.
Signed-off-by: AnilKumar Ch anilku...@ti.com
---
This patch has been tested on AM335X EVM. Due to lack of hardware
I am not able to test c_can functionality. I appreciate if anyone
can test c_can functionality with this patch.
This patch is based on can-next/master
Changes from v4:
- Incorporated Vaibhav H review comments on v4.
* Moved pm_runtime put/get_sync calls to appropriate positions.
- This patch is from Add DT support to C_CAN/D_CAN controller
patch series. Rest of the patches in this series were applied
so this v5 contains only this patch.
drivers/net/can/c_can/c_can.c |8
drivers/net/can/c_can/c_can.h |1 +
drivers/net/can/c_can/c_can_platform.c | 11 +++
3 files changed, 20 insertions(+)
diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index eea6608..ca1151d 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -34,6 +34,8 @@
#include linux/if_ether.h
#include linux/list.h
#include linux/io.h
+#include linux/pm_runtime.h
+#include linux/platform_device.h
#include linux/can.h
#include linux/can/dev.h
@@ -673,6 +675,8 @@ static int c_can_get_berr_counter(const struct net_device
*dev,
unsigned int reg_err_counter;
struct c_can_priv *priv = netdev_priv(dev);
+ pm_runtime_get_sync(priv-pdev-dev);
+
reg_err_counter = priv-read_reg(priv, C_CAN_ERR_CNT_REG);
bec-rxerr = (reg_err_counter ERR_CNT_REC_MASK)
ERR_CNT_REC_SHIFT;
@@ -1053,6 +1057,8 @@ static int c_can_open(struct net_device *dev)
int err;
struct c_can_priv *priv = netdev_priv(dev);
+ pm_runtime_get_sync(priv-pdev-dev);
+
/* open the can device */
err = open_candev(dev);
if (err) {
@@ -1079,6 +1085,7 @@ static int c_can_open(struct net_device *dev)
exit_irq_fail:
close_candev(dev);
+ pm_runtime_put_sync(priv-pdev-dev);
return err;
}
@@ -1091,6 +1098,7 @@ static int c_can_close(struct net_device *dev)
c_can_stop(dev);
free_irq(dev-irq, dev);
close_candev(dev);
+ pm_runtime_put_sync(priv-pdev-dev);
return 0;
}
diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index 4e56baa..ef84439 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -153,6 +153,7 @@ struct c_can_priv {
struct can_priv can;/* must be the first member */
struct napi_struct napi;
struct net_device *dev;
+ struct platform_device *pdev;
int tx_object;
int current_status;
int last_status;
diff --git a/drivers/net/can/c_can/c_can_platform.c
b/drivers/net/can/c_can/c_can_platform.c
index d0a66cf..cbb1184 100644
--- a/drivers/net/can/c_can/c_can_platform.c
+++ b/drivers/net/can/c_can/c_can_platform.c
@@ -32,6 +32,7 @@
#include linux/clk.h
#include linux/of.h
#include linux/of_device.h
+#include linux/pm_runtime.h
#include linux/can/dev.h
@@ -177,7 +178,11 @@ static int __devinit c_can_plat_probe(struct
platform_device *pdev)
goto exit_free_device;
}
+ pm_runtime_enable(pdev-dev);
+ pm_runtime_get_sync(pdev-dev);
+
dev-irq = irq;
+ priv-pdev = pdev;
priv-base = addr;
priv-can.clock.freq = clk_get_rate(clk);
priv-priv = clk;
@@ -192,12 +197,16 @@ static int __devinit c_can_plat_probe(struct
platform_device *pdev)
goto exit_free_device;
}
+ pm_runtime_put_sync(pdev-dev);
+
dev_info(pdev-dev, %s device registered (regs=%p, irq=%d)\n,
KBUILD_MODNAME, priv-base, dev-irq);
return 0;
exit_free_device:
platform_set_drvdata(pdev, NULL);
+ pm_runtime_put_sync(pdev-dev);
+ pm_runtime_disable(pdev-dev);
free_c_can_dev(dev);
exit_iounmap:
iounmap(addr);
@@ -226,6 +235,8 @@ static int __devexit c_can_plat_remove(struct
platform_device *pdev)
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
release_mem_region(mem-start, resource_size(mem));
+ pm_runtime_put_sync(pdev-dev);
+ pm_runtime_disable(pdev-dev);
clk_put(priv-priv);
return 0;
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] can: c_can: Add runtime PM support to Bosch C_CAN/D_CAN controller
On 08/07/2012 01:20 PM, AnilKumar Ch wrote:
Add Runtime PM support to C_CAN/D_CAN controller. The runtime PM
APIs control clocks for C_CAN/D_CAN IP and prevent access to the
register of C_CAN/D_CAN IP when clock is turned off.
Signed-off-by: AnilKumar Ch anilku...@ti.com
---
This patch has been tested on AM335X EVM. Due to lack of hardware
I am not able to test c_can functionality. I appreciate if anyone
can test c_can functionality with this patch.
The priv-pdev-dev will not work for c_can/d_can which are not using
the platform driver. Options:
a) priv-dev
add a struct device pointer to the priv, and
1) guard pm_runtime_get_sync() for priv-dev == NULL
(or check if pm_runtime_get_sync copes with NULL pointer)
- or -
2) fill in the priv-dev in all other drivers (i.e. PCI)
and check if it makes sense/works to call it on PCI hardware
b) convert all other drivers, i.e. the PCI driver to create a
platform device
Without thinking long, I suggest to a) with flavour 1)
Maybe there are better solutions.
Marc
More comments inline:
This patch is based on can-next/master
Changes from v4:
- Incorporated Vaibhav H review comments on v4.
* Moved pm_runtime put/get_sync calls to appropriate positions.
- This patch is from Add DT support to C_CAN/D_CAN controller
patch series. Rest of the patches in this series were applied
so this v5 contains only this patch.
drivers/net/can/c_can/c_can.c |8
drivers/net/can/c_can/c_can.h |1 +
drivers/net/can/c_can/c_can_platform.c | 11 +++
3 files changed, 20 insertions(+)
diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index eea6608..ca1151d 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -34,6 +34,8 @@
#include linux/if_ether.h
#include linux/list.h
#include linux/io.h
+#include linux/pm_runtime.h
+#include linux/platform_device.h
#include linux/can.h
#include linux/can/dev.h
@@ -673,6 +675,8 @@ static int c_can_get_berr_counter(const struct net_device
*dev,
unsigned int reg_err_counter;
struct c_can_priv *priv = netdev_priv(dev);
+ pm_runtime_get_sync(priv-pdev-dev);
+
Why do you make a get_sync() here, where's the correspondning put_sync()?
reg_err_counter = priv-read_reg(priv, C_CAN_ERR_CNT_REG);
bec-rxerr = (reg_err_counter ERR_CNT_REC_MASK)
ERR_CNT_REC_SHIFT;
@@ -1053,6 +1057,8 @@ static int c_can_open(struct net_device *dev)
int err;
struct c_can_priv *priv = netdev_priv(dev);
+ pm_runtime_get_sync(priv-pdev-dev);
+
/* open the can device */
err = open_candev(dev);
if (err) {
What if open_candev() fails? Please add a jump label
exit_open_candev_fail and add thje put_sync() there.
goto exit_open_candev_fail;
@@ -1079,6 +1085,7 @@ static int c_can_open(struct net_device *dev)
exit_irq_fail:
close_candev(dev);
exit_open_candev_fail:
+ pm_runtime_put_sync(priv-pdev-dev);
return err;
}
@@ -1091,6 +1098,7 @@ static int c_can_close(struct net_device *dev)
c_can_stop(dev);
free_irq(dev-irq, dev);
close_candev(dev);
+ pm_runtime_put_sync(priv-pdev-dev);
return 0;
}
diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index 4e56baa..ef84439 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -153,6 +153,7 @@ struct c_can_priv {
struct can_priv can;/* must be the first member */
struct napi_struct napi;
struct net_device *dev;
+ struct platform_device *pdev;
int tx_object;
int current_status;
int last_status;
diff --git a/drivers/net/can/c_can/c_can_platform.c
b/drivers/net/can/c_can/c_can_platform.c
index d0a66cf..cbb1184 100644
--- a/drivers/net/can/c_can/c_can_platform.c
+++ b/drivers/net/can/c_can/c_can_platform.c
@@ -32,6 +32,7 @@
#include linux/clk.h
#include linux/of.h
#include linux/of_device.h
+#include linux/pm_runtime.h
#include linux/can/dev.h
@@ -177,7 +178,11 @@ static int __devinit c_can_plat_probe(struct
platform_device *pdev)
goto exit_free_device;
}
+ pm_runtime_enable(pdev-dev);
+ pm_runtime_get_sync(pdev-dev);
+
dev-irq = irq;
+ priv-pdev = pdev;
priv-base = addr;
priv-can.clock.freq = clk_get_rate(clk);
priv-priv = clk;
@@ -192,12 +197,16 @@ static int __devinit c_can_plat_probe(struct
platform_device *pdev)
goto exit_free_device;
}
+ pm_runtime_put_sync(pdev-dev);
+
dev_info(pdev-dev, %s device registered (regs=%p, irq=%d)\n,
KBUILD_MODNAME, priv-base, dev-irq);
return 0;
exit_free_device:
platform_set_drvdata(pdev, NULL);
+ pm_runtime_put_sync(pdev-dev);
+
