Skip to contentSkip to conversation

flexcan: Use a regulator to control the CAN transceiver

PATCH In review Fabio Estevam 3 participants 14 messages Jun 5, 2013 – Jun 12, 2013

Subsystems (5)
Device tree bindings Arm (32-bit) Arm sub-architectures Arm i.MX and Layerscape +1 more subsystems
lore
Share & tools
lore

Fabio Estevam

author reviewer
#0 lore raw

PATCH 1/2 flexcan: Use a regulator to control the CAN transceiver

From: Fabio Estevam <[email protected]>

Instead of using a GPIO to turn on/off the CAN transceiver, it is better to use a regulator as some systems may use a PMIC to power the CAN transceiver.

Signed-off-by Fabio Estevam <[email protected]>
---
 .../devicetree/bindings/net/can/fsl-flexcan.txt    |  2 +
 arch/arm/boot/dts/imx28-evk.dts                    | 11 +++++
 arch/arm/mach-mxs/mach-mxs.c                       | 54 ++--------------------
 drivers/net/can/flexcan.c                          | 24 +++++-----
 4 files changed, 29 insertions(+), 62 deletions(-)
Documentation/devicetree/bindings/net/can/fsl-flexcan.txt +2 -0
index 8ff324e..56d6cc3 100644
@@ -16,6 +16,8 @@ Optional properties:
 
 - clock-frequency : The oscillator frequency driving the flexcan device
 
+- xceiver-supply: Regulator that powers the CAN transceiver
+
 Example:
 
 	can@1c000 {
arch/arm/boot/dts/imx28-evk.dts +11 -0
index 3637bf3..dff42b3 100644
@@ -155,6 +155,7 @@
 			can0: can@80032000 {
 				pinctrl-names = "default";
 				pinctrl-0 = <&can0_pins_a>;
+				xceiver-supply = <&reg_can_3v3>;
 				status = "okay";
 			};
 

@@ -319,6 +320,16 @@
 			gpio = <&gpio3 30 0>;
 			enable-active-high;
 		};
+
+		reg_can_3v3: can-3v3 {
+			compatible = "regulator-fixed";
+			regulator-name = "can-3v3";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+			gpio = <&gpio2 13 0>;
+			enable-active-high;
+		};
+
 	};
 
 	sound {
arch/arm/mach-mxs/mach-mxs.c +3 -51

81 lines not on this page · raw message

drivers/net/can/flexcan.c +13 -11
index f873b9f..3de02b1 100644
@@ -37,6 +37,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
 
 #define DRV_NAME			"flexcan"
 

@@ -211,6 +212,7 @@ struct flexcan_priv {
 	struct clk *clk_per;
 	struct flexcan_platform_data *pdata;
 	const struct flexcan_devtype_data *devtype_data;
+	struct regulator *reg_xceiver;
 };
 
 static struct flexcan_devtype_data fsl_p1010_devtype_data = {

@@ -258,15 +260,6 @@ static inline void flexcan_write(u32 val, void __iomem *addr)
 }
 #endif
 
-/*
- * Swtich transceiver on or off
- */
-static void flexcan_transceiver_switch(const struct flexcan_priv *priv, int on)
-{
-	if (priv->pdata && priv->pdata->transceiver_switch)
-		priv->pdata->transceiver_switch(on);
-}
-
 static inline int flexcan_has_and_handle_berr(const struct flexcan_priv *priv,
 					      u32 reg_esr)
 {

@@ -799,7 +792,11 @@ static int flexcan_chip_start(struct net_device *dev)
 	if (priv->devtype_data->features & FLEXCAN_HAS_V10_FEATURES)
 		flexcan_write(0x0, &regs->rxfgmask);
 
-	flexcan_transceiver_switch(priv, 1);
+	if (priv->reg_xceiver)	{
+		err = regulator_enable(priv->reg_xceiver);
+		if (err)
+			goto out;
+	}
 
 	/* synchronize with the can bus */
 	reg_mcr = flexcan_read(&regs->mcr);

@@ -842,7 +839,8 @@ static void flexcan_chip_stop(struct net_device *dev)
 	reg |= FLEXCAN_MCR_MDIS | FLEXCAN_MCR_HALT;
 	flexcan_write(reg, &regs->mcr);
 
-	flexcan_transceiver_switch(priv, 0);
+	if (priv->reg_xceiver)
+		regulator_disable(priv->reg_xceiver);
 	priv->can.state = CAN_STATE_STOPPED;
 
 	return;

@@ -1084,6 +1082,10 @@ static int flexcan_probe(struct platform_device *pdev)
 	priv->pdata = pdev->dev.platform_data;
 	priv->devtype_data = devtype_data;
 
+	priv->reg_xceiver = devm_regulator_get(&pdev->dev, "xceiver");
+	if (IS_ERR(priv->reg_xceiver))
+		priv->reg_xceiver = NULL;
+
 	netif_napi_add(dev, &priv->napi, flexcan_poll, FLEXCAN_NAPI_WEIGHT);
 
 	dev_set_drvdata(&pdev->dev, dev);
Signature · 1 line
1.8.1.2
author reviewer
#1 lore raw

PATCH 2/2 flexcan: Enable/Disable the CAN regulator in probe/remove

From: Fabio Estevam <[email protected]>

mx28evk has two CAN ports and they are controlled by the same GPIO.

If can0 runs flexcan_chip_stop(), then the regulator will be disabled, which means that can1 cannot run anymore.

To overcome such scenario, let's enable the CAN regulator in flexcan_probe() and disabled it in flexcan_remove().

Signed-off-by Fabio Estevam <[email protected]>
---
 drivers/net/can/flexcan.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)
drivers/net/can/flexcan.c +9 -8
index 3de02b1..456a549 100644
@@ -792,12 +792,6 @@ static int flexcan_chip_start(struct net_device *dev)
 	if (priv->devtype_data->features & FLEXCAN_HAS_V10_FEATURES)
 		flexcan_write(0x0, &regs->rxfgmask);
 
-	if (priv->reg_xceiver)	{
-		err = regulator_enable(priv->reg_xceiver);
-		if (err)
-			goto out;
-	}
-
 	/* synchronize with the can bus */
 	reg_mcr = flexcan_read(&regs->mcr);
 	reg_mcr &= ~FLEXCAN_MCR_HALT;

@@ -839,8 +833,6 @@ static void flexcan_chip_stop(struct net_device *dev)
 	reg |= FLEXCAN_MCR_MDIS | FLEXCAN_MCR_HALT;
 	flexcan_write(reg, &regs->mcr);
 
-	if (priv->reg_xceiver)
-		regulator_disable(priv->reg_xceiver);
 	priv->can.state = CAN_STATE_STOPPED;
 
 	return;

@@ -1099,6 +1091,12 @@ static int flexcan_probe(struct platform_device *pdev)
 
 	devm_can_led_init(dev);
 
+	if (priv->reg_xceiver) {
+		err = regulator_enable(priv->reg_xceiver);
+		if (err)
+			goto failed_register;
+	}
+
 	dev_info(&pdev->dev, "device registered (reg_base=%p, irq=%d)\n",
 		 priv->base, dev->irq);
 

@@ -1122,6 +1120,9 @@ static int flexcan_remove(struct platform_device *pdev)
 	struct flexcan_priv *priv = netdev_priv(dev);
 	struct resource *mem;
 
+	if (priv->reg_xceiver)
+		regulator_disable(priv->reg_xceiver);
+
 	unregister_flexcandev(dev);
 	iounmap(priv->base);
Signature · 1 line
1.8.1.2
maintainer
#2 lore raw

Re: [PATCH 2/2] flexcan: Enable/Disable the CAN regulator in probe/remove

Quoted 4 lines · Fabio Estevam

mx28evk has two CAN ports and they are controlled by the same GPIO.

If can0 runs flexcan_chip_stop(), then the regulator will be disabled, which means that can1 cannot run anymore.

Are you sure? I think the regulator framework has a proper refcount.

Quoted 2 lines

To overcome such scenario, let's enable the CAN regulator in flexcan_probe() and disabled it in flexcan_remove().

Marc

Signature · 4 lines
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   |
author reviewer
#3 lore raw

Re: [PATCH 2/2] flexcan: Enable/Disable the CAN regulator in probe/remove

Hi Marc,

Quoted 7 lines · Marc Kleine-Budde
Quoted 4 lines · Fabio Estevam

mx28evk has two CAN ports and they are controlled by the same GPIO.

If can0 runs flexcan_chip_stop(), then the regulator will be disabled, which means that can1 cannot run anymore.

Are you sure? I think the regulator framework has a proper refcount.

Yes, the refcount is OK. The problem is that on mx28evk we have two CAN ports controlled by the same regulator.

Let's say that can0 executes flexcan_chip_stop:

static void flexcan_chip_stop(struct net_device *dev)

{
	struct flexcan_priv *priv = netdev_priv(dev);
	struct flexcan_regs __iomem *regs = priv->base;
	u32 reg;

	/* Disable all interrupts */
	flexcan_write(0, &regs->imask1);

	/* Disable + halt module */
	reg = flexcan_read(&regs->mcr);
	reg |= FLEXCAN_MCR_MDIS | FLEXCAN_MCR_HALT;
	flexcan_write(reg, &regs->mcr);

        if (priv->reg_xceiver)
	        regulator_disabled(priv->reg_xceiver);

...

It will turn off the transceiver here, which means that can1 will not be able to operate.

Currently this situation is properly handled inside mach-mxs by only disabling the CAN transceiver when both CAN ports are not used.

With the conversion to regulator I thought on enabling the regulator at the probe function and disabling it at remove.

I understand that the power consumption will be higher in this case, but I could not find a better alternative so far.

Regards,

Fabio Estevam

maintainer
#4 lore raw

Re: [PATCH 2/2] flexcan: Enable/Disable the CAN regulator in probe/remove

Quoted 32 lines · Fabio Estevam

Hi Marc,

Quoted 7 lines · Marc Kleine-Budde
Quoted 4 lines · Fabio Estevam

mx28evk has two CAN ports and they are controlled by the same GPIO.

If can0 runs flexcan_chip_stop(), then the regulator will be disabled, which means that can1 cannot run anymore.

Are you sure? I think the regulator framework has a proper refcount.

Yes, the refcount is OK. The problem is that on mx28evk we have two CAN ports controlled by the same regulator.

Let's say that can0 executes flexcan_chip_stop:

static void flexcan_chip_stop(struct net_device *dev)

{
	struct flexcan_priv *priv = netdev_priv(dev);
	struct flexcan_regs __iomem *regs = priv->base;
	u32 reg;

	/* Disable all interrupts */
	flexcan_write(0, &regs->imask1);

	/* Disable + halt module */
	reg = flexcan_read(&regs->mcr);
	reg |= FLEXCAN_MCR_MDIS | FLEXCAN_MCR_HALT;
	flexcan_write(reg, &regs->mcr);

        if (priv->reg_xceiver)
	        regulator_disabled(priv->reg_xceiver);
Quoted 7 lines

...

It will turn off the transceiver here, which means that can1 will not be able to operate.

Currently this situation is properly handled inside mach-mxs by only disabling the CAN transceiver when both CAN ports are not used.

Why don't you enable the regulator in can1, too?

Marc

Signature · 4 lines
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   |
author reviewer
#5 lore raw

Re: [PATCH 2/2] flexcan: Enable/Disable the CAN regulator in probe/remove

Quoted 7 lines · Marc Kleine-Budde
Quoted 5 lines

It will turn off the transceiver here, which means that can1 will not be able to operate.

Currently this situation is properly handled inside mach-mxs by only disabling the CAN transceiver when both CAN ports are not used.

Why don't you enable the regulator in can1, too?

Since they use the same regulator (the same GPIO pin control both CAN ports on mx28evk), I can not have regulator disabled for can0 and regulator enabled for can1.

maintainer
#6 lore raw

Re: [PATCH 2/2] flexcan: Enable/Disable the CAN regulator in probe/remove

Quoted 5 lines · Fabio Estevam
Quoted 3 lines · Marc Kleine-Budde
Quoted 3 lines

It will turn off the transceiver here, which means that can1 will not be able to operate.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

Currently this situation is properly handled inside mach-mxs by only disabling the CAN transceiver when both CAN ports are not used.

Why don't you enable the regulator in can1, too?

Since they use the same regulator (the same GPIO pin control both CAN ports on mx28evk), I can not have regulator disabled for can0 and regulator enabled for can1.

Why not? I think regulators have proper refcounts and will stay enabled in the "disabled for CAN0 and enabled for CAN1" case.

Marc

Signature · 4 lines
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   |
maintainer
#7 lore raw
In reply to Fabio Estevam
Quoted 4 lines · Fabio Estevam

Instead of using a GPIO to turn on/off the CAN transceiver, it is better to use a regulator as some systems may use a PMIC to power the CAN transceiver.

Signed-off-by Fabio Estevam <[email protected]>

Just a short note, as I'm not in the office today.

I like the idea of using an established subsystem for the transceiver/phy. But I don't like the solution you suggested in patch 2.

Marc

Signature · 4 lines
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   |
maintainer
#8 lore raw
In reply to Fabio Estevam
Quoted 6 lines · Fabio Estevam

From: Fabio Estevam <[email protected]>

Instead of using a GPIO to turn on/off the CAN transceiver, it is better to use a regulator as some systems may use a PMIC to power the CAN transceiver.

Signed-off-by Fabio Estevam <[email protected]>

I'm preparing a patch to remove the usage of flexcan_platform_data from the imx tree. Can you add that patch to your series and remove "linux/can/platform/flexcan.h" from the source tree in this patch?

Quoted 33 lines
---
 .../devicetree/bindings/net/can/fsl-flexcan.txt    |  2 +
 arch/arm/boot/dts/imx28-evk.dts                    | 11 +++++
 arch/arm/mach-mxs/mach-mxs.c                       | 54 ++--------------------
 drivers/net/can/flexcan.c                          | 24 +++++-----
 4 files changed, 29 insertions(+), 62 deletions(-)
Documentation/devicetree/bindings/net/can/fsl-flexcan.txt +2 -0
index 8ff324e..56d6cc3 100644
@@ -16,6 +16,8 @@ Optional properties:
 
 - clock-frequency : The oscillator frequency driving the flexcan device
 
+- xceiver-supply: Regulator that powers the CAN transceiver
+
 Example:
 
 	can@1c000 {
arch/arm/boot/dts/imx28-evk.dts +1 -0
index 3637bf3..dff42b3 100644
@@ -155,6 +155,7 @@
 			can0: can@80032000 {
 				pinctrl-names = "default";
 				pinctrl-0 = <&can0_pins_a>;
+				xceiver-supply = <&reg_can_3v3>;
 				status = "okay";
 			};

arch/arm/boot/dts/imx28-evk.dts line 329

Quoted 10 lines
arch/arm/boot/dts/imx28-evk.dts +7 -0
@@ -319,6 +320,16 @@
 			gpio = <&gpio3 30 0>;
 			enable-active-high;
 		};
+
+		reg_can_3v3: can-3v3 {
+			compatible = "regulator-fixed";
+			regulator-name = "can-3v3";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+			gpio = <&gpio2 13 0>;

Can you convert GPIO2_13 from a hog gpio to a normal pinctl gpio?

Looks good otherwise.

Marc

Signature · 4 lines
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   |
author reviewer
#9 lore raw
Quoted 11 lines · Marc Kleine-Budde
Quoted 6 lines · Fabio Estevam

From: Fabio Estevam <[email protected]>

Instead of using a GPIO to turn on/off the CAN transceiver, it is better to use a regulator as some systems may use a PMIC to power the CAN transceiver.

Signed-off-by Fabio Estevam <[email protected]>

I'm preparing a patch to remove the usage of flexcan_platform_data from the imx tree. Can you add that patch to your series and remove "linux/can/platform/flexcan.h" from the source tree in this patch?

Sure, no problem. I can send your patch as part of my series and work through your other comments.

Regards,

Fabio Estevam

author reviewer
#10 lore raw
In reply to Marc Kleine-Budde

Hi Marc,

On Mon, Jun 10, 2013 at 1:59 PM, Marc Kleine-Budde <[email protected]> wrote:
};

Quoted 9 lines

arch/arm/boot/dts/imx28-evk.dts line 329

Quoted 7 lines
arch/arm/boot/dts/imx28-evk.dts +7 -0
+
+             reg_can_3v3: can-3v3 {
+                     compatible = "regulator-fixed";
+                     regulator-name = "can-3v3";
+                     regulator-min-microvolt = <3300000>;
+                     regulator-max-microvolt = <3300000>;
+                     gpio = <&gpio2 13 0>;

Can you convert GPIO2_13 from a hog gpio to a normal pinctl gpio?

I kept gpio2_13 as a hog gpio because on imx28-evk.dts we use this approach for the fec and usb regulators.

Hope this is OK.

maintainer
#11 lore raw
Quoted 10 lines · Fabio Estevam

On Mon, Jun 10, 2013 at 1:59 PM, Marc Kleine-Budde <[email protected]> wrote:
};

Quoted 8 lines
Quoted 7 lines
arch/arm/boot/dts/imx28-evk.dts +7 -0
+
+             reg_can_3v3: can-3v3 {
+                     compatible = "regulator-fixed";
+                     regulator-name = "can-3v3";
+                     regulator-min-microvolt = <3300000>;
+                     regulator-max-microvolt = <3300000>;
+                     gpio = <&gpio2 13 0>;
Quoted 4 lines
Quoted 1 line

Can you convert GPIO2_13 from a hog gpio to a normal pinctl gpio?

I kept gpio2_13 as a hog gpio because on imx28-evk.dts we use this approach for the fec and usb regulators.

Using the hog gpio pinmux for the fec and usb might be due to historical reasons. I personally prefer the pinmux properly added to the regulator. But I'm not the mxs maintainer, so I don't insist on changing the pinmux setup. However a Acked-by from Shawn Guo would be fine.

Marc

Signature · 4 lines
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   |
Acked-by on v1 1/2
#12 lore raw
In reply to Fabio Estevam
Quoted 10 lines · Fabio Estevam

From: Fabio Estevam <[email protected]>

Instead of using a GPIO to turn on/off the CAN transceiver, it is better to use a regulator as some systems may use a PMIC to power the CAN transceiver.

Signed-off-by Fabio Estevam <[email protected]>
---
 .../devicetree/bindings/net/can/fsl-flexcan.txt    |  2 +
 arch/arm/boot/dts/imx28-evk.dts                    | 11 +++++
 arch/arm/mach-mxs/mach-mxs.c                       | 54 ++--------------------
Acked-by Shawn Guo <[email protected]>
Quoted 2 lines
 drivers/net/can/flexcan.c                          | 24 +++++-----
 4 files changed, 29 insertions(+), 62 deletions(-)
maintainer
#13 lore raw
Quoted 6 lines · Shawn Guo
Quoted 5 lines · Fabio Estevam

From: Fabio Estevam <[email protected]>

Instead of using a GPIO to turn on/off the CAN transceiver, it is better to use a regulator as some systems may use a PMIC to power the CAN transceiver.

Quoted 7 lines
Quoted 5 lines
Signed-off-by Fabio Estevam <[email protected]>
---
 .../devicetree/bindings/net/can/fsl-flexcan.txt    |  2 +
 arch/arm/boot/dts/imx28-evk.dts                    | 11 +++++
 arch/arm/mach-mxs/mach-mxs.c                       | 54 ++--------------------
Acked-by Shawn Guo <[email protected]>

Tnx, I (back) ported the patch to current net-next/master, resulting in this diff between the patches.

of_platform_populate(NULL, of_default_bus_match_table,

--                           mxs_auxdata_lookup, parent);
-+                           NULL, parent);
+-                           mxs_auxdata_lookup, NULL);
++                           NULL, NULL);

Hope it's alright to keep your Acked-by. I will post the resulting patch set soon, but I want to test it on real hardware next Monday.

Marc

Signature · 4 lines
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   |