| Message ID | 1342562748-24701-1-git-send-email-mkl@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Marc Kleine-Budde wrote: > From: Steffen Trumtrar <s.trumtrar@pengutronix.de> > > This patch add support for a second clock to the flexcan driver. On modern > freescale ARM cores like the imx53 and imx6q two clocks ("ipg" and "per") > must be enabled in order to access the CAN core. > > In the original driver, the clock was requested without specifying the > connection id, further all mainline ARM archs with flexcan support > (imx28, imx25, imx35) register their flexcan clock without a connection id, > too. > > This patch first renames the existing clk variable to clk_ipg and adds the > connection id "ipg" to the clk_get() call. Then a second clock "per" is > requested. As all archs don't specify a connection id, both clk_get return > the same clock. This ensures compatibility to existing flexcan support > and adds support for imx53 at the same time. > > After this patch hits mainline, the archs may give their existing flexcan > clock the "ipg" connection id and implement a dummy "per" clock. > > This patch has been tested on imx28 (unmodified clk tree) and on imx53 > with a seperate "ipg" and "per" clock. > > Cc: Sascha Hauer <s.hauer@pengutronix.de> > Cc: Shawn Guo <shawn.guo@linaro.org> > Cc: Hui Wang <jason77.wang@gmail.com> > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de> > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> > --- > drivers/net/can/flexcan.c | 52 ++++++++++++++++++++++++++++++--------------- > 1 file changed, 35 insertions(+), 17 deletions(-) > > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c > index c8a6fc7..281d51f 100644 > --- a/drivers/net/can/flexcan.c > +++ b/drivers/net/can/flexcan.c > @@ -190,7 +190,8 @@ struct flexcan_priv { > > [...] > if (!clock_freq) { > - clk = clk_get(&pdev->dev, NULL); > - if (IS_ERR(clk)) { > - dev_err(&pdev->dev, "no clock defined\n"); > - err = PTR_ERR(clk); > + clk_ipg = clk_get(&pdev->dev, "ipg"); > + if (IS_ERR(clk_ipg)) { > + dev_err(&pdev->dev, "no ipg clock defined\n"); > + err = PTR_ERR(clk_ipg); > + goto failed_clock; > + } > + clock_freq = clk_get_rate(clk_ipg); > + > + clk_per = clk_get(&pdev->dev, "per"); > + if (IS_ERR(clk_per)) { > + dev_err(&pdev->dev, "no per clock defined\n"); > + err = PTR_ERR(clk_per); > goto failed_clock; > } > For those only register one clk and without con_id (mx35), clk_per will equal to clk_ipg, how to handle this situation, modify mx35 clk tree? > - clock_freq = clk_get_rate(clk); > } > > mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > @@ -1039,7 +1052,8 @@ static int __devinit flexcan_probe(struct platform_device *pdev) > CAN_CTRLMODE_BERR_REPORTING; > priv->base = base; > priv->dev = dev; > - priv->clk = clk; > + priv->clk_ipg = clk_ipg; > + priv->clk_per = clk_per; > priv->pdata = pdev->dev.platform_data; > priv->devtype_data = devtype_data; > > @@ -1067,9 +1081,11 @@ static int __devinit flexcan_probe(struct platform_device *pdev) > failed_map: > release_mem_region(mem->start, mem_size); > failed_get: > - if (clk) > - clk_put(clk); > failed_clock: > + if (clk_per) > Use if (!IS_ERR(clk_per)) > + clk_put(clk_per); > + if (clk_ipg) > Ditto. If we use devm_clk_get(), we can save to call clk_put(). > + clk_put(clk_ipg); > return err; > } > > @@ -1086,8 +1102,10 @@ static int __devexit flexcan_remove(struct platform_device *pdev) > mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > release_mem_region(mem->start, resource_size(mem)); > > - if (priv->clk) > - clk_put(priv->clk); > + if (priv->clk_per) > + clk_put(priv->clk_per); > + if (priv->clk_ipg) > + clk_put(priv->clk_ipg); > > free_candev(dev); > >
On 07/18/2012 04:12 AM, Hui Wang wrote: > Marc Kleine-Budde wrote: >> From: Steffen Trumtrar <s.trumtrar@pengutronix.de> >> >> This patch add support for a second clock to the flexcan driver. On >> modern >> freescale ARM cores like the imx53 and imx6q two clocks ("ipg" and "per") >> must be enabled in order to access the CAN core. >> >> In the original driver, the clock was requested without specifying the >> connection id, further all mainline ARM archs with flexcan support >> (imx28, imx25, imx35) register their flexcan clock without a >> connection id, >> too. >> >> This patch first renames the existing clk variable to clk_ipg and adds >> the >> connection id "ipg" to the clk_get() call. Then a second clock "per" is >> requested. As all archs don't specify a connection id, both clk_get >> return >> the same clock. This ensures compatibility to existing flexcan support >> and adds support for imx53 at the same time. >> >> After this patch hits mainline, the archs may give their existing flexcan >> clock the "ipg" connection id and implement a dummy "per" clock. >> >> This patch has been tested on imx28 (unmodified clk tree) and on imx53 >> with a seperate "ipg" and "per" clock. >> >> Cc: Sascha Hauer <s.hauer@pengutronix.de> >> Cc: Shawn Guo <shawn.guo@linaro.org> >> Cc: Hui Wang <jason77.wang@gmail.com> >> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de> >> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> >> --- >> drivers/net/can/flexcan.c | 52 >> ++++++++++++++++++++++++++++++--------------- >> 1 file changed, 35 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c >> index c8a6fc7..281d51f 100644 >> --- a/drivers/net/can/flexcan.c >> +++ b/drivers/net/can/flexcan.c >> @@ -190,7 +190,8 @@ struct flexcan_priv { >> >> > [...] >> if (!clock_freq) { >> - clk = clk_get(&pdev->dev, NULL); >> - if (IS_ERR(clk)) { >> - dev_err(&pdev->dev, "no clock defined\n"); >> - err = PTR_ERR(clk); >> + clk_ipg = clk_get(&pdev->dev, "ipg"); >> + if (IS_ERR(clk_ipg)) { >> + dev_err(&pdev->dev, "no ipg clock defined\n"); >> + err = PTR_ERR(clk_ipg); >> + goto failed_clock; >> + } >> + clock_freq = clk_get_rate(clk_ipg); >> + >> + clk_per = clk_get(&pdev->dev, "per"); >> + if (IS_ERR(clk_per)) { >> + dev_err(&pdev->dev, "no per clock defined\n"); >> + err = PTR_ERR(clk_per); >> goto failed_clock; >> } >> > For those only register one clk and without con_id (mx35), clk_per will > equal to clk_ipg, how to handle this situation, modify mx35 clk tree? This isn't a problem, the clock is just enabled twice. As soon as this patch is mainline the clock tree on the one-clock-for-flexcan archs can modify their clock tree. I've commented on this in the commit message, have you read it? >> - clock_freq = clk_get_rate(clk); >> } >> >> mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> @@ -1039,7 +1052,8 @@ static int __devinit flexcan_probe(struct >> platform_device *pdev) >> CAN_CTRLMODE_BERR_REPORTING; >> priv->base = base; >> priv->dev = dev; >> - priv->clk = clk; >> + priv->clk_ipg = clk_ipg; >> + priv->clk_per = clk_per; >> priv->pdata = pdev->dev.platform_data; >> priv->devtype_data = devtype_data; >> >> @@ -1067,9 +1081,11 @@ static int __devinit flexcan_probe(struct >> platform_device *pdev) >> failed_map: >> release_mem_region(mem->start, mem_size); >> failed_get: >> - if (clk) >> - clk_put(clk); >> failed_clock: >> + if (clk_per) >> > Use if (!IS_ERR(clk_per)) Yes, good catch. Is it allowed to call clk_put with a NULL pointer? Both clocks can be NULL, if the frequency is defined via the device tree, this is case for powerpc. >> + clk_put(clk_per); >> + if (clk_ipg) >> > Ditto. > > If we use devm_clk_get(), we can save to call clk_put(). Even better. I think I'll send a separate patch to convert the whole driver to devm. >> + clk_put(clk_ipg); >> return err; >> } >> >> @@ -1086,8 +1102,10 @@ static int __devexit flexcan_remove(struct >> platform_device *pdev) >> mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> release_mem_region(mem->start, resource_size(mem)); >> >> - if (priv->clk) >> - clk_put(priv->clk); >> + if (priv->clk_per) >> + clk_put(priv->clk_per); >> + if (priv->clk_ipg) >> + clk_put(priv->clk_ipg); This will go away with devm then :) >> >> free_candev(dev); >> >> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-can" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Marc
Marc Kleine-Budde wrote: > On 07/18/2012 04:12 AM, Hui Wang wrote: > >> Marc Kleine-Budde wrote: >> >>> From: Steffen Trumtrar <s.trumtrar@pengutronix.de> >>> >>> This patch add support for a second clock to the flexcan driver. On >>> modern >>> freescale ARM cores like the imx53 and imx6q two clocks ("ipg" and "per") >>> must be enabled in order to access the CAN core. >>> >>> In the original driver, the clock was requested without specifying the >>> connection id, further all mainline ARM archs with flexcan support >>> (imx28, imx25, imx35) register their flexcan clock without a >>> connection id, >>> too. >>> >>> This patch first renames the existing clk variable to clk_ipg and adds >>> the >>> connection id "ipg" to the clk_get() call. Then a second clock "per" is >>> requested. As all archs don't specify a connection id, both clk_get >>> return >>> the same clock. This ensures compatibility to existing flexcan support >>> and adds support for imx53 at the same time. >>> >>> After this patch hits mainline, the archs may give their existing flexcan >>> clock the "ipg" connection id and implement a dummy "per" clock. >>> >>> This patch has been tested on imx28 (unmodified clk tree) and on imx53 >>> with a seperate "ipg" and "per" clock. >>> >>> Cc: Sascha Hauer <s.hauer@pengutronix.de> >>> Cc: Shawn Guo <shawn.guo@linaro.org> >>> Cc: Hui Wang <jason77.wang@gmail.com> >>> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de> >>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> >>> --- >>> drivers/net/can/flexcan.c | 52 >>> ++++++++++++++++++++++++++++++--------------- >>> 1 file changed, 35 insertions(+), 17 deletions(-) >>> >>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c >>> index c8a6fc7..281d51f 100644 >>> --- a/drivers/net/can/flexcan.c >>> +++ b/drivers/net/can/flexcan.c >>> @@ -190,7 +190,8 @@ struct flexcan_priv { >>> >>> >>> >> [...] >> >>> if (!clock_freq) { >>> - clk = clk_get(&pdev->dev, NULL); >>> - if (IS_ERR(clk)) { >>> - dev_err(&pdev->dev, "no clock defined\n"); >>> - err = PTR_ERR(clk); >>> + clk_ipg = clk_get(&pdev->dev, "ipg"); >>> + if (IS_ERR(clk_ipg)) { >>> + dev_err(&pdev->dev, "no ipg clock defined\n"); >>> + err = PTR_ERR(clk_ipg); >>> + goto failed_clock; >>> + } >>> + clock_freq = clk_get_rate(clk_ipg); >>> + >>> + clk_per = clk_get(&pdev->dev, "per"); >>> + if (IS_ERR(clk_per)) { >>> + dev_err(&pdev->dev, "no per clock defined\n"); >>> + err = PTR_ERR(clk_per); >>> goto failed_clock; >>> } >>> >>> >> For those only register one clk and without con_id (mx35), clk_per will >> equal to clk_ipg, how to handle this situation, modify mx35 clk tree? >> > > This isn't a problem, the clock is just enabled twice. As soon as this > patch is mainline the clock tree on the one-clock-for-flexcan archs can > modify their clock tree. I've commented on this in the commit message, > have you read it? > Got it now. :-)
On Wed, Jul 18, 2012 at 10:37:22AM +0200, Marc Kleine-Budde wrote: > > Yes, good catch. > > Is it allowed to call clk_put with a NULL pointer? As NULL is a valid clock by definition it is allowed to do this. However, calling clk_put with a NULL pointer when a corresponding call to clk_get never returned NULL is probably not a good idea. Sascha
Hi, Marc Kleine-Budde writes: > On 07/18/2012 04:12 AM, Hui Wang wrote: > > Marc Kleine-Budde wrote: > >> From: Steffen Trumtrar <s.trumtrar@pengutronix.de> > >> > >> This patch add support for a second clock to the flexcan driver. On > >> modern > >> freescale ARM cores like the imx53 and imx6q two clocks ("ipg" and "per") > >> must be enabled in order to access the CAN core. > >> - clock_freq = clk_get_rate(clk); > >> } > >> > >> mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >> @@ -1039,7 +1052,8 @@ static int __devinit flexcan_probe(struct > >> platform_device *pdev) > >> CAN_CTRLMODE_BERR_REPORTING; > >> priv->base = base; > >> priv->dev = dev; > >> - priv->clk = clk; > >> + priv->clk_ipg = clk_ipg; > >> + priv->clk_per = clk_per; > >> priv->pdata = pdev->dev.platform_data; > >> priv->devtype_data = devtype_data; > >> > >> @@ -1067,9 +1081,11 @@ static int __devinit flexcan_probe(struct > >> platform_device *pdev) > >> failed_map: > >> release_mem_region(mem->start, mem_size); > >> failed_get: > >> - if (clk) > >> - clk_put(clk); > >> failed_clock: > >> + if (clk_per) > >> > > Use if (!IS_ERR(clk_per)) > > Yes, good catch. > > Is it allowed to call clk_put with a NULL pointer? Both clocks can be > NULL, if the frequency is defined via the device tree, this is case for > powerpc. > This has been discussed here already several times. Everything returned by clk_get() that does not satisfy the IS_ERR() check, is a valid clk cookie and can be used for the clk API calls. Lothar Waßmann
On 07/18/2012 11:05 AM, Lothar Waßmann wrote: > Hi, > > Marc Kleine-Budde writes: >> On 07/18/2012 04:12 AM, Hui Wang wrote: >>> Marc Kleine-Budde wrote: >>>> From: Steffen Trumtrar <s.trumtrar@pengutronix.de> >>>> >>>> This patch add support for a second clock to the flexcan driver. On >>>> modern >>>> freescale ARM cores like the imx53 and imx6q two clocks ("ipg" and "per") >>>> must be enabled in order to access the CAN core. >>>> - clock_freq = clk_get_rate(clk); >>>> } >>>> >>>> mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>> @@ -1039,7 +1052,8 @@ static int __devinit flexcan_probe(struct >>>> platform_device *pdev) >>>> CAN_CTRLMODE_BERR_REPORTING; >>>> priv->base = base; >>>> priv->dev = dev; >>>> - priv->clk = clk; >>>> + priv->clk_ipg = clk_ipg; >>>> + priv->clk_per = clk_per; >>>> priv->pdata = pdev->dev.platform_data; >>>> priv->devtype_data = devtype_data; >>>> >>>> @@ -1067,9 +1081,11 @@ static int __devinit flexcan_probe(struct >>>> platform_device *pdev) >>>> failed_map: >>>> release_mem_region(mem->start, mem_size); >>>> failed_get: >>>> - if (clk) >>>> - clk_put(clk); >>>> failed_clock: >>>> + if (clk_per) >>>> >>> Use if (!IS_ERR(clk_per)) >> >> Yes, good catch. >> >> Is it allowed to call clk_put with a NULL pointer? Both clocks can be >> NULL, if the frequency is defined via the device tree, this is case for >> powerpc. >> > This has been discussed here already several times. Everything > returned by clk_get() that does not satisfy the IS_ERR() check, is a > valid clk cookie and can be used for the clk API calls. I've switched to devm completely (see Patch v2), so the clk_put is obsolete. In this particular driver the NULL pointer in question is not returned via the clk_get(). On powerpc we don't use clk_get(), because it's not working (AFIAK) on the p1010 (the powerpc arch that has a flexcan). Marc
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c index c8a6fc7..281d51f 100644 --- a/drivers/net/can/flexcan.c +++ b/drivers/net/can/flexcan.c @@ -190,7 +190,8 @@ struct flexcan_priv { u32 reg_esr; u32 reg_ctrl_default; - struct clk *clk; + struct clk *clk_ipg; + struct clk *clk_per; struct flexcan_platform_data *pdata; const struct flexcan_devtype_data *devtype_data; }; @@ -828,7 +829,8 @@ static int flexcan_open(struct net_device *dev) struct flexcan_priv *priv = netdev_priv(dev); int err; - clk_prepare_enable(priv->clk); + clk_prepare_enable(priv->clk_ipg); + clk_prepare_enable(priv->clk_per); err = open_candev(dev); if (err) @@ -850,7 +852,8 @@ static int flexcan_open(struct net_device *dev) out_close: close_candev(dev); out: - clk_disable_unprepare(priv->clk); + clk_disable_unprepare(priv->clk_per); + clk_disable_unprepare(priv->clk_ipg); return err; } @@ -864,7 +867,8 @@ static int flexcan_close(struct net_device *dev) flexcan_chip_stop(dev); free_irq(dev->irq, dev); - clk_disable_unprepare(priv->clk); + clk_disable_unprepare(priv->clk_per); + clk_disable_unprepare(priv->clk_ipg); close_candev(dev); @@ -903,7 +907,8 @@ static int __devinit register_flexcandev(struct net_device *dev) struct flexcan_regs __iomem *regs = priv->base; u32 reg, err; - clk_prepare_enable(priv->clk); + clk_prepare_enable(priv->clk_ipg); + clk_prepare_enable(priv->clk_per); /* select "bus clock", chip must be disabled */ flexcan_chip_disable(priv); @@ -936,7 +941,8 @@ static int __devinit register_flexcandev(struct net_device *dev) out: /* disable core and turn off clocks */ flexcan_chip_disable(priv); - clk_disable_unprepare(priv->clk); + clk_disable_unprepare(priv->clk_per); + clk_disable_unprepare(priv->clk_ipg); return err; } @@ -964,7 +970,7 @@ static int __devinit flexcan_probe(struct platform_device *pdev) struct net_device *dev; struct flexcan_priv *priv; struct resource *mem; - struct clk *clk = NULL; + struct clk *clk_ipg = NULL, *clk_per = NULL; struct pinctrl *pinctrl; void __iomem *base; resource_size_t mem_size; @@ -980,13 +986,20 @@ static int __devinit flexcan_probe(struct platform_device *pdev) "clock-frequency", &clock_freq); if (!clock_freq) { - clk = clk_get(&pdev->dev, NULL); - if (IS_ERR(clk)) { - dev_err(&pdev->dev, "no clock defined\n"); - err = PTR_ERR(clk); + clk_ipg = clk_get(&pdev->dev, "ipg"); + if (IS_ERR(clk_ipg)) { + dev_err(&pdev->dev, "no ipg clock defined\n"); + err = PTR_ERR(clk_ipg); + goto failed_clock; + } + clock_freq = clk_get_rate(clk_ipg); + + clk_per = clk_get(&pdev->dev, "per"); + if (IS_ERR(clk_per)) { + dev_err(&pdev->dev, "no per clock defined\n"); + err = PTR_ERR(clk_per); goto failed_clock; } - clock_freq = clk_get_rate(clk); } mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); @@ -1039,7 +1052,8 @@ static int __devinit flexcan_probe(struct platform_device *pdev) CAN_CTRLMODE_BERR_REPORTING; priv->base = base; priv->dev = dev; - priv->clk = clk; + priv->clk_ipg = clk_ipg; + priv->clk_per = clk_per; priv->pdata = pdev->dev.platform_data; priv->devtype_data = devtype_data; @@ -1067,9 +1081,11 @@ static int __devinit flexcan_probe(struct platform_device *pdev) failed_map: release_mem_region(mem->start, mem_size); failed_get: - if (clk) - clk_put(clk); failed_clock: + if (clk_per) + clk_put(clk_per); + if (clk_ipg) + clk_put(clk_ipg); return err; } @@ -1086,8 +1102,10 @@ static int __devexit flexcan_remove(struct platform_device *pdev) mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); release_mem_region(mem->start, resource_size(mem)); - if (priv->clk) - clk_put(priv->clk); + if (priv->clk_per) + clk_put(priv->clk_per); + if (priv->clk_ipg) + clk_put(priv->clk_ipg); free_candev(dev);