| Message ID | 1344338457-595-1-git-send-email-anilkumar@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <anilkumar@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); > + 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; >
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 <anilkumar@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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 <anilkumar@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(+)