[PATCH] can: flexcan: add 2nd clock to support imx53 and newer

From: Steffen Trumtrar <redacted>

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 <redacted>
Cc: Hui Wang <redacted>
Signed-off-by: Steffen Trumtrar <redacted>
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 {
 	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);
 

-- 
1.7.10

Marc Kleine-Budde wrote:

[...]

 	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?

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

On 07/18/2012 04:12 AM, Hui Wang wrote:

Marc Kleine-Budde wrote:

quoted

From: Steffen Trumtrar <redacted>

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 <redacted>
Cc: Hui Wang <redacted>
Signed-off-by: Steffen Trumtrar <redacted>
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 {

  

[...]

quoted

     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?

quoted

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

quoted

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

quoted

+        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 :)

quoted

 
     free_candev(dev);
 
  

-- 
To unsubscribe from this list: send the line "unsubscribe linux-can" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120718/413fb0a7/attachment.sig>

Marc Kleine-Budde wrote:

On 07/18/2012 04:12 AM, Hui Wang wrote:
  

quoted

Marc Kleine-Budde wrote:
    

quoted

From: Steffen Trumtrar <redacted>

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 <redacted>
Cc: Hui Wang <redacted>
Signed-off-by: Steffen Trumtrar <redacted>
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 {

  
      

[...]
    

quoted

     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

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Hi,

Marc Kleine-Budde writes:

On 07/18/2012 04:12 AM, Hui Wang wrote:

quoted

Marc Kleine-Budde wrote:

quoted

From: Steffen Trumtrar <redacted>

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

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
___________________________________________________________

On 07/18/2012 11:05 AM, Lothar Wa?mann wrote:

Hi,

Marc Kleine-Budde writes:

quoted

On 07/18/2012 04:12 AM, Hui Wang wrote:

quoted

Marc Kleine-Budde wrote:

quoted

From: Steffen Trumtrar <redacted>

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

-- 
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: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120718/34408bf6/attachment.sig>