[PATCH v4 0/8] net: can: Use syscon regmap for TI specific RAMINIT register

[PATCH v4 0/8] net: can: Use syscon regmap for TI specific RAMINIT register

[Roger Quadros]

Hi,

Some hardware (TI am43xx) has a buggy RAMINIT DONE mechanism and it might
not always set the DONE bit. This will result in a lockup in c_can_hw_raminit_wait_ti(),
so patch 1 adds a timeout mechanism there.

There is a non compliancy within TI platforms with respect to the
layout of the RAMINIT register. The patches 3 and 4 address this issue
and make a flexible but standard way of defining the RAMINIT hardware register
layout in the device tree. The RAMINIT register is accessed using the syscon
regmap framework.

Patches available at
git@github.com:rogerq/linux.git	[for-v3.19/can]

Patches are tested on am335x-evm, am437x-gp-evm and dra7-evm.
Board support files to allow CAN testing on these boards are available at
git@github.com:rogerq/linux.git	[for-v3.19/omap-dts-dcan]

Changelog:

v4:
- updated "syscon-raminit" binding to contain the CAN instance id. We no longer
use different compatible ids for multiple CAN IPs on the same SoC. The instance
ID is used instead to locate the start/stop bit positions from driver data.

v3:
- allow driver data to be more than just CAN_ID
- RAMINIT register data moved to driver data instead of device tree file.

v2:
- added "ti" vendor prefix to TI specific raminit properties.
- split DTS changes into a separate series

cheers,
-roger

---
Roger Quadros (8):
  net: can: c_can: Add timeout to c_can_hw_raminit_ti()
  net: can: c_can: Introduce c_can_driver_data structure
  net: can: c_can: Add RAMINIT register information to driver data
  net: can: c_can: Add syscon/regmap RAMINIT mechanism
  net: can: c_can: Add support for START pulse in RAMINIT sequence
  net: can: c_can: Disable pins when CAN interface is down
  net: can: c_can: Add support for TI DRA7 DCAN
  net: can: c_can: Add support for TI am3352 DCAN

 .../devicetree/bindings/net/can/c_can.txt          |   4 +
 drivers/net/can/c_can/c_can.c                      |  20 ++
 drivers/net/can/c_can/c_can.h                      |  23 ++-
 drivers/net/can/c_can/c_can_platform.c             | 217 +++++++++++++++------
 4 files changed, 203 insertions(+), 61 deletions(-)

-- 
1.8.3.2

[PATCH v4 1/8] net: can: c_can: Add timeout to c_can_hw_raminit_ti()

[Roger Quadros]

TI's RAMINIT DONE mechanism is buggy on AM43xx SoC and may not always
be set after the START bit is set. Although it seems to work fine even
in that case. So add a timeout mechanism to c_can_hw_raminit_wait_ti().
Don't bail out in that failure case but just print an error message.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/net/can/c_can/c_can_platform.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
index fb279d6..b144e71 100644
--- a/drivers/net/can/c_can/c_can_platform.c
+++ b/drivers/net/can/c_can/c_can_platform.c
@@ -75,10 +75,18 @@ static void c_can_plat_write_reg_aligned_to_32bit(const struct c_can_priv *priv,
 static void c_can_hw_raminit_wait_ti(const struct c_can_priv *priv, u32 mask,
 				  u32 val)
 {
+	int timeout = 0;
 	/* We look only at the bits of our instance. */
 	val &= mask;
-	while ((readl(priv->raminit_ctrlreg) & mask) != val)
+	while ((readl(priv->raminit_ctrlreg) & mask) != val) {
 		udelay(1);
+		timeout++;
+
+		if (timeout == 1000) {
+			dev_err(&priv->dev->dev, "%s: time out\n", __func__);
+			break;
+		}
+	}
 }
 
 static void c_can_hw_raminit_ti(const struct c_can_priv *priv, bool enable)
-- 
1.8.3.2

[PATCH v4 2/8] net: can: c_can: Introduce c_can_driver_data structure

[Roger Quadros]

We want to have more data than just can_dev_id to be present
in the driver data e.g. TI platforms need RAMINIT register
description. Introduce the c_can_driver_data structure and move
the can_dev_id into it.

Tidy up the way it is used on probe().

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/net/can/c_can/c_can.h          |  4 +++
 drivers/net/can/c_can/c_can_platform.c | 52 +++++++++++++++++++---------------
 2 files changed, 33 insertions(+), 23 deletions(-)

diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index 99ad1aa..26c975d 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -169,6 +169,10 @@ enum c_can_dev_id {
 	BOSCH_D_CAN,
 };
 
+struct c_can_driver_data {
+	enum c_can_dev_id id;
+};
+
 /* c_can private data structure */
 struct c_can_priv {
 	struct can_priv can;	/* must be the first member */
diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
index b144e71..1546c2b 100644
--- a/drivers/net/can/c_can/c_can_platform.c
+++ b/drivers/net/can/c_can/c_can_platform.c
@@ -167,26 +167,34 @@ static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable)
 	}
 }
 
+static struct c_can_driver_data c_can_drvdata = {
+	.id = BOSCH_C_CAN,
+};
+
+static struct c_can_driver_data d_can_drvdata = {
+	.id = BOSCH_D_CAN,
+};
+
 static struct platform_device_id c_can_id_table[] = {
-	[BOSCH_C_CAN_PLATFORM] = {
+	{
 		.name = KBUILD_MODNAME,
-		.driver_data = BOSCH_C_CAN,
+		.driver_data = (kernel_ulong_t)&c_can_drvdata,
 	},
-	[BOSCH_C_CAN] = {
+	{
 		.name = "c_can",
-		.driver_data = BOSCH_C_CAN,
+		.driver_data = (kernel_ulong_t)&c_can_drvdata,
 	},
-	[BOSCH_D_CAN] = {
+	{
 		.name = "d_can",
-		.driver_data = BOSCH_D_CAN,
-	}, {
-	}
+		.driver_data = (kernel_ulong_t)&d_can_drvdata,
+	},
+	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(platform, c_can_id_table);
 
 static const struct of_device_id c_can_of_table[] = {
-	{ .compatible = "bosch,c_can", .data = &c_can_id_table[BOSCH_C_CAN] },
-	{ .compatible = "bosch,d_can", .data = &c_can_id_table[BOSCH_D_CAN] },
+	{ .compatible = "bosch,c_can", .data = &c_can_drvdata },
+	{ .compatible = "bosch,d_can", .data = &d_can_drvdata },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, c_can_of_table);
@@ -198,21 +206,19 @@ static int c_can_plat_probe(struct platform_device *pdev)
 	struct net_device *dev;
 	struct c_can_priv *priv;
 	const struct of_device_id *match;
-	const struct platform_device_id *id;
 	struct resource *mem, *res;
 	int irq;
 	struct clk *clk;
-
-	if (pdev->dev.of_node) {
-		match = of_match_device(c_can_of_table, &pdev->dev);
-		if (!match) {
-			dev_err(&pdev->dev, "Failed to find matching dt id\n");
-			ret = -EINVAL;
-			goto exit;
-		}
-		id = match->data;
+	const struct c_can_driver_data *drvdata;
+
+	match = of_match_device(c_can_of_table, &pdev->dev);
+	if (match) {
+		drvdata = match->data;
+	} else if (pdev->id_entry->driver_data) {
+		drvdata = (struct c_can_driver_data *)
+			   pdev->id_entry->driver_data;
 	} else {
-		id = platform_get_device_id(pdev);
+		return -ENODEV;
 	}
 
 	/* get the appropriate clk */
@@ -244,7 +250,7 @@ static int c_can_plat_probe(struct platform_device *pdev)
 	}
 
 	priv = netdev_priv(dev);
-	switch (id->driver_data) {
+	switch (drvdata->id) {
 	case BOSCH_C_CAN:
 		priv->regs = reg_map_c_can;
 		switch (mem->flags & IORESOURCE_MEM_TYPE_MASK) {
@@ -303,7 +309,7 @@ static int c_can_plat_probe(struct platform_device *pdev)
 	priv->device = &pdev->dev;
 	priv->can.clock.freq = clk_get_rate(clk);
 	priv->priv = clk;
-	priv->type = id->driver_data;
+	priv->type = drvdata->id;
 
 	platform_set_drvdata(pdev, dev);
 	SET_NETDEV_DEV(dev, &pdev->dev);
-- 
1.8.3.2

[PATCH v4 3/8] net: can: c_can: Add RAMINIT register information to driver data

[Roger Quadros]

Some platforms (e.g. TI) need special RAMINIT register handling.
Provide a way to store RAMINIT register description in driver data.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/net/can/c_can/c_can.h          | 6 ++++++
 drivers/net/can/c_can/c_can_platform.c | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index 26c975d..3c305a1 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -171,6 +171,12 @@ enum c_can_dev_id {
 
 struct c_can_driver_data {
 	enum c_can_dev_id id;
+
+	/* RAMINIT register description. Optional. */
+	u8 num_can;		/* Number of CAN instances on the SoC */
+	u8 *raminit_start_bits;	/* Array of START bit positions */
+	u8 *raminit_done_bits;	/* Array of DONE bit positions */
+	bool raminit_pulse;	/* If set, sets and clears START bit (pulse) */
 };
 
 /* c_can private data structure */
diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
index 1546c2b..20deb67 100644
--- a/drivers/net/can/c_can/c_can_platform.c
+++ b/drivers/net/can/c_can/c_can_platform.c
@@ -250,6 +250,7 @@ static int c_can_plat_probe(struct platform_device *pdev)
 	}
 
 	priv = netdev_priv(dev);
+
 	switch (drvdata->id) {
 	case BOSCH_C_CAN:
 		priv->regs = reg_map_c_can;
-- 
1.8.3.2

[PATCH v4 4/8] net: can: c_can: Add syscon/regmap RAMINIT mechanism

[Roger Quadros]

Some TI SoCs like DRA7 have a RAMINIT register specification
different from the other AMxx SoCs and as expected by the
existing driver.

To add more insanity, this register is shared with other
IPs like DSS, PCIe and PWM.

Provides a more generic mechanism to specify the RAMINIT
register location and START/DONE bit position and use the
syscon/regmap framework to access the register.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 .../devicetree/bindings/net/can/c_can.txt          |   3 +
 drivers/net/can/c_can/c_can.h                      |  11 +-
 drivers/net/can/c_can/c_can_platform.c             | 112 ++++++++++++++-------
 3 files changed, 86 insertions(+), 40 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/can/c_can.txt b/Documentation/devicetree/bindings/net/can/c_can.txt
index 8f1ae81..a3ca3ee 100644
--- a/Documentation/devicetree/bindings/net/can/c_can.txt
+++ b/Documentation/devicetree/bindings/net/can/c_can.txt
@@ -12,6 +12,9 @@ Required properties:
 Optional properties:
 - ti,hwmods		: Must be "d_can<n>" or "c_can<n>", n being the
 			  instance number
+- syscon-raminit	: Handle to system control region that contains the
+			  RAMINIT register, register offset to the RAMINIT
+			  register and the CAN instance number (0 offset).
 
 Note: "ti,hwmods" field is used to fetch the base address and irq
 resources from TI, omap hwmod data base during device registration.
diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index 3c305a1..0e17c7b 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -179,6 +179,14 @@ struct c_can_driver_data {
 	bool raminit_pulse;	/* If set, sets and clears START bit (pulse) */
 };
 
+/* Out of band RAMINIT register access via syscon regmap */
+struct c_can_raminit {
+	struct regmap *syscon;	/* for raminit ctrl. reg. access */
+	unsigned int reg;	/* register index within syscon */
+	u8 start_bit;
+	u8 done_bit;
+};
+
 /* c_can private data structure */
 struct c_can_priv {
 	struct can_priv can;	/* must be the first member */
@@ -196,8 +204,7 @@ struct c_can_priv {
 	const u16 *regs;
 	void *priv;		/* for board-specific data */
 	enum c_can_dev_id type;
-	u32 __iomem *raminit_ctrlreg;
-	int instance;
+	struct c_can_raminit raminit_sys;	/* RAMINIT via syscon regmap */
 	void (*raminit) (const struct c_can_priv *priv, bool enable);
 	u32 comm_rcv_high;
 	u32 rxmasked;
diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
index 20deb67..3776483 100644
--- a/drivers/net/can/c_can/c_can_platform.c
+++ b/drivers/net/can/c_can/c_can_platform.c
@@ -32,14 +32,13 @@
 #include <linux/clk.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
 
 #include <linux/can/dev.h>
 
 #include "c_can.h"
 
-#define CAN_RAMINIT_START_MASK(i)	(0x001 << (i))
-#define CAN_RAMINIT_DONE_MASK(i)	(0x100 << (i))
-#define CAN_RAMINIT_ALL_MASK(i)		(0x101 << (i))
 #define DCAN_RAM_INIT_BIT		(1 << 3)
 static DEFINE_SPINLOCK(raminit_lock);
 /*
@@ -72,47 +71,61 @@ static void c_can_plat_write_reg_aligned_to_32bit(const struct c_can_priv *priv,
 	writew(val, priv->base + 2 * priv->regs[index]);
 }
 
-static void c_can_hw_raminit_wait_ti(const struct c_can_priv *priv, u32 mask,
-				  u32 val)
+static void c_can_hw_raminit_wait_syscon(const struct c_can_priv *priv,
+					 u32 mask, u32 val)
 {
 	int timeout = 0;
+	const struct c_can_raminit *raminit = &priv->raminit_sys;
+	u32 ctrl;
+
 	/* We look only at the bits of our instance. */
 	val &= mask;
-	while ((readl(priv->raminit_ctrlreg) & mask) != val) {
+	do {
 		udelay(1);
 		timeout++;
 
+		regmap_read(raminit->syscon, raminit->reg, &ctrl);
 		if (timeout == 1000) {
 			dev_err(&priv->dev->dev, "%s: time out\n", __func__);
 			break;
 		}
-	}
+	} while ((ctrl & mask) != val);
 }
 
-static void c_can_hw_raminit_ti(const struct c_can_priv *priv, bool enable)
+static void c_can_hw_raminit_syscon(const struct c_can_priv *priv, bool enable)
 {
-	u32 mask = CAN_RAMINIT_ALL_MASK(priv->instance);
+	u32 mask;
 	u32 ctrl;
+	const struct c_can_raminit *raminit = &priv->raminit_sys;
+	u8 start_bit, done_bit;
+
+	start_bit = raminit->start_bit;
+	done_bit = raminit->done_bit;
 
 	spin_lock(&raminit_lock);
 
-	ctrl = readl(priv->raminit_ctrlreg);
+	mask = 1 << start_bit | 1 << done_bit;
+	regmap_read(raminit->syscon, raminit->reg, &ctrl);
+
 	/* We clear the done and start bit first. The start bit is
 	 * looking at the 0 -> transition, but is not self clearing;
 	 * And we clear the init done bit as well.
+	 * NOTE: DONE must be written with 1 to clear it.
 	 */
-	ctrl &= ~CAN_RAMINIT_START_MASK(priv->instance);
-	ctrl |= CAN_RAMINIT_DONE_MASK(priv->instance);
-	writel(ctrl, priv->raminit_ctrlreg);
-	ctrl &= ~CAN_RAMINIT_DONE_MASK(priv->instance);
-	c_can_hw_raminit_wait_ti(priv, mask, ctrl);
+	ctrl &= ~(1 << start_bit);
+	ctrl |= 1 << done_bit;
+	regmap_write(raminit->syscon, raminit->reg, ctrl);
+
+	ctrl &= ~(1 << done_bit);
+	c_can_hw_raminit_wait_syscon(priv, mask, ctrl);
 
 	if (enable) {
 		/* Set start bit and wait for the done bit. */
-		ctrl |= CAN_RAMINIT_START_MASK(priv->instance);
-		writel(ctrl, priv->raminit_ctrlreg);
-		ctrl |= CAN_RAMINIT_DONE_MASK(priv->instance);
-		c_can_hw_raminit_wait_ti(priv, mask, ctrl);
+		ctrl |= 1 << start_bit;
+		regmap_write(raminit->syscon, raminit->reg, ctrl);
+
+		ctrl |= 1 << done_bit;
+		c_can_hw_raminit_wait_syscon(priv, mask, ctrl);
 	}
 	spin_unlock(&raminit_lock);
 }
@@ -206,10 +219,11 @@ static int c_can_plat_probe(struct platform_device *pdev)
 	struct net_device *dev;
 	struct c_can_priv *priv;
 	const struct of_device_id *match;
-	struct resource *mem, *res;
+	struct resource *mem;
 	int irq;
 	struct clk *clk;
 	const struct c_can_driver_data *drvdata;
+	struct device_node *np = pdev->dev.of_node;
 
 	match = of_match_device(c_can_of_table, &pdev->dev);
 	if (match) {
@@ -278,27 +292,49 @@ static int c_can_plat_probe(struct platform_device *pdev)
 		priv->read_reg32 = d_can_plat_read_reg32;
 		priv->write_reg32 = d_can_plat_write_reg32;
 
-		if (pdev->dev.of_node)
-			priv->instance = of_alias_get_id(pdev->dev.of_node, "d_can");
-		else
-			priv->instance = pdev->id;
-
-		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-		/* Not all D_CAN modules have a separate register for the D_CAN
-		 * RAM initialization. Use default RAM init bit in D_CAN module
-		 * if not specified in DT.
+		/* Check if we need custom RAMINIT via syscon. Mostly for TI
+		 * platforms. Only supported with DT boot.
 		 */
-		if (!res) {
+		if (np && of_property_read_bool(np, "syscon-raminit")) {
+			u32 id;
+			struct c_can_raminit *raminit = &priv->raminit_sys;
+
+			ret = -EINVAL;
+			raminit->syscon = syscon_regmap_lookup_by_phandle(np,
+									  "syscon-raminit");
+			if (IS_ERR(raminit->syscon)) {
+				dev_err(&pdev->dev,
+					"couldn't get syscon regmap for RAMINIT\n");
+				goto exit_free_device;
+			}
+
+			if (of_property_read_u32_index(np, "syscon-raminit", 1,
+						       &raminit->reg)) {
+				dev_err(&pdev->dev,
+					"couldn't get the RAMINIT reg. offset!\n");
+				goto exit_free_device;
+			}
+
+			if (of_property_read_u32_index(np, "syscon-raminit", 2,
+						       &id)) {
+				dev_err(&pdev->dev,
+					"couldn't get the CAN instance ID\n");
+				goto exit_free_device;
+			}
+
+			if (id >= drvdata->num_can) {
+				dev_err(&pdev->dev,
+					"Invalid CAN instance ID\n");
+				goto exit_free_device;
+			}
+
+			raminit->start_bit = drvdata->raminit_start_bits[id];
+			raminit->done_bit = drvdata->raminit_done_bits[id];
+
+			priv->raminit = c_can_hw_raminit_syscon;
+		} else {
 			priv->raminit = c_can_hw_raminit;
-			break;
 		}
-
-		priv->raminit_ctrlreg = devm_ioremap(&pdev->dev, res->start,
-						     resource_size(res));
-		if (!priv->raminit_ctrlreg || priv->instance < 0)
-			dev_info(&pdev->dev, "control memory is not used for raminit\n");
-		else
-			priv->raminit = c_can_hw_raminit_ti;
 		break;
 	default:
 		ret = -EINVAL;
-- 
1.8.3.2

[PATCH v4 5/8] net: can: c_can: Add support for START pulse in RAMINIT sequence

[Roger Quadros]

Some SoCs e.g. (TI DRA7xx) need a START pulse to start the
RAMINIT sequence i.e. START bit must be set and cleared before
checking for the DONE bit status.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/net/can/c_can/c_can.h          | 1 +
 drivers/net/can/c_can/c_can_platform.c | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index 0e17c7b..c6715ca 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -185,6 +185,7 @@ struct c_can_raminit {
 	unsigned int reg;	/* register index within syscon */
 	u8 start_bit;
 	u8 done_bit;
+	bool needs_pulse;
 };
 
 /* c_can private data structure */
diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
index 3776483..b838c6b 100644
--- a/drivers/net/can/c_can/c_can_platform.c
+++ b/drivers/net/can/c_can/c_can_platform.c
@@ -124,6 +124,12 @@ static void c_can_hw_raminit_syscon(const struct c_can_priv *priv, bool enable)
 		ctrl |= 1 << start_bit;
 		regmap_write(raminit->syscon, raminit->reg, ctrl);
 
+		/* clear START bit if start pulse is needed */
+		if (raminit->needs_pulse) {
+			ctrl &= ~(1 << start_bit);
+			regmap_write(raminit->syscon, raminit->reg, ctrl);
+		}
+
 		ctrl |= 1 << done_bit;
 		c_can_hw_raminit_wait_syscon(priv, mask, ctrl);
 	}
@@ -330,6 +336,7 @@ static int c_can_plat_probe(struct platform_device *pdev)
 
 			raminit->start_bit = drvdata->raminit_start_bits[id];
 			raminit->done_bit = drvdata->raminit_done_bits[id];
+			raminit->needs_pulse = drvdata->raminit_pulse;
 
 			priv->raminit = c_can_hw_raminit_syscon;
 		} else {
-- 
1.8.3.2

[PATCH v4 6/8] net: can: c_can: Disable pins when CAN interface is down

[Roger Quadros]

DRA7 CAN IP suffers from a problem which causes it to be prevented
from fully turning OFF (i.e. stuck in transition) if the module was
disabled while there was traffic on the CAN_RX line.

To work around this issue we select the SLEEP pin state by default
on probe and use the DEFAULT pin state on CAN up and back to the
SLEEP pin state on CAN down.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/net/can/c_can/c_can.c          | 20 ++++++++++++++++++++
 drivers/net/can/c_can/c_can.h          |  1 +
 drivers/net/can/c_can/c_can_platform.c | 20 ++++++++++++++++++++
 3 files changed, 41 insertions(+)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 8e78bb4..4dfc3ce 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -35,6 +35,7 @@
 #include <linux/list.h>
 #include <linux/io.h>
 #include <linux/pm_runtime.h>
+#include <linux/pinctrl/consumer.h>
 
 #include <linux/can.h>
 #include <linux/can/dev.h>
@@ -603,6 +604,15 @@ static int c_can_start(struct net_device *dev)
 
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
+	/* activate pins */
+	if (!IS_ERR(priv->pinctrl)) {
+		struct pinctrl_state *s;
+
+		s = pinctrl_lookup_state(priv->pinctrl, PINCTRL_STATE_DEFAULT);
+		if (!IS_ERR(s))
+			pinctrl_select_state(priv->pinctrl, s);
+	}
+
 	return 0;
 }
 
@@ -611,6 +621,16 @@ static void c_can_stop(struct net_device *dev)
 	struct c_can_priv *priv = netdev_priv(dev);
 
 	c_can_irq_control(priv, false);
+
+	/* deactivate pins */
+	if (!IS_ERR(priv->pinctrl)) {
+		struct pinctrl_state *s;
+
+		s = pinctrl_lookup_state(priv->pinctrl, PINCTRL_STATE_SLEEP);
+		if (!IS_ERR(s))
+			pinctrl_select_state(priv->pinctrl, s);
+	}
+
 	priv->can.state = CAN_STATE_STOPPED;
 }
 
diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index c6715ca..3cedf48 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -210,6 +210,7 @@ struct c_can_priv {
 	u32 comm_rcv_high;
 	u32 rxmasked;
 	u32 dlc[C_CAN_MSG_OBJ_TX_NUM];
+	struct pinctrl *pinctrl;
 };
 
 struct net_device *alloc_c_can_dev(void);
diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
index b838c6b..71b9063 100644
--- a/drivers/net/can/c_can/c_can_platform.c
+++ b/drivers/net/can/c_can/c_can_platform.c
@@ -34,6 +34,7 @@
 #include <linux/of_device.h>
 #include <linux/mfd/syscon.h>
 #include <linux/regmap.h>
+#include <linux/pinctrl/consumer.h>
 
 #include <linux/can/dev.h>
 
@@ -230,6 +231,7 @@ static int c_can_plat_probe(struct platform_device *pdev)
 	struct clk *clk;
 	const struct c_can_driver_data *drvdata;
 	struct device_node *np = pdev->dev.of_node;
+	struct pinctrl *pinctrl;
 
 	match = of_match_device(c_can_of_table, &pdev->dev);
 	if (match) {
@@ -241,6 +243,23 @@ static int c_can_plat_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (!IS_ERR(pinctrl)) {
+		struct pinctrl_state *s;
+
+		/* Deactivate pins to prevent DRA7 DCAN IP from being
+		 * stuck in transition when module is disabled.
+		 * Pins are activated in c_can_start() and deactivated
+		 * in c_can_stop()
+		 */
+		s = pinctrl_lookup_state(pinctrl, PINCTRL_STATE_SLEEP);
+		if (!IS_ERR(s))
+			pinctrl_select_state(pinctrl, s);
+	} else {
+		dev_warn(&pdev->dev,
+			 "failed to get pinctrl\n");
+	}
+
 	/* get the appropriate clk */
 	clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(clk)) {
@@ -270,6 +289,7 @@ static int c_can_plat_probe(struct platform_device *pdev)
 	}
 
 	priv = netdev_priv(dev);
+	priv->pinctrl = pinctrl;
 
 	switch (drvdata->id) {
 	case BOSCH_C_CAN:
-- 
1.8.3.2

[PATCH v4 7/8] net: can: c_can: Add support for TI DRA7 DCAN

[Roger Quadros]

DRA7 SoC has 2 CAN IPs. Provide compatible IDs and RAMINIT
register data for both.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 Documentation/devicetree/bindings/net/can/c_can.txt |  1 +
 drivers/net/can/c_can/c_can_platform.c              | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/can/c_can.txt b/Documentation/devicetree/bindings/net/can/c_can.txt
index a3ca3ee..f682fdb 100644
--- a/Documentation/devicetree/bindings/net/can/c_can.txt
+++ b/Documentation/devicetree/bindings/net/can/c_can.txt
@@ -4,6 +4,7 @@ Bosch C_CAN/D_CAN controller Device Tree Bindings
 Required properties:
 - compatible		: Should be "bosch,c_can" for C_CAN controllers and
 			  "bosch,d_can" for D_CAN controllers.
+			  Can be "ti,dra7-d_can".
 - reg			: physical base address and size of the C_CAN/D_CAN
 			  registers map
 - interrupts		: property with a value describing the interrupt
diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
index 71b9063..7a81db4 100644
--- a/drivers/net/can/c_can/c_can_platform.c
+++ b/drivers/net/can/c_can/c_can_platform.c
@@ -195,6 +195,16 @@ static struct c_can_driver_data d_can_drvdata = {
 	.id = BOSCH_D_CAN,
 };
 
+static u8 dra7_raminit_start_bits[] = {3, 5};
+static u8 dra7_raminit_done_bits[] = {1, 2};
+static struct c_can_driver_data dra7_dcan_drvdata = {
+	.id = BOSCH_D_CAN,
+	.num_can = 2,
+	.raminit_start_bits = dra7_raminit_start_bits,
+	.raminit_done_bits = dra7_raminit_done_bits,
+	.raminit_pulse = true,
+};
+
 static struct platform_device_id c_can_id_table[] = {
 	{
 		.name = KBUILD_MODNAME,
@@ -215,6 +225,7 @@ MODULE_DEVICE_TABLE(platform, c_can_id_table);
 static const struct of_device_id c_can_of_table[] = {
 	{ .compatible = "bosch,c_can", .data = &c_can_drvdata },
 	{ .compatible = "bosch,d_can", .data = &d_can_drvdata },
+	{ .compatible = "ti,dra7-d_can", .data = &dra7_dcan_drvdata },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, c_can_of_table);
-- 
1.8.3.2

[PATCH v4 8/8] net: can: c_can: Add support for TI am3352 DCAN

[Roger Quadros]

AM3352 SoC has 2 DCAN modules. Add compatible id and
raminit driver data for am3352 DCAN.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 Documentation/devicetree/bindings/net/can/c_can.txt |  2 +-
 drivers/net/can/c_can/c_can_platform.c              | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/can/c_can.txt b/Documentation/devicetree/bindings/net/can/c_can.txt
index f682fdb..6731730 100644
--- a/Documentation/devicetree/bindings/net/can/c_can.txt
+++ b/Documentation/devicetree/bindings/net/can/c_can.txt
@@ -4,7 +4,7 @@ Bosch C_CAN/D_CAN controller Device Tree Bindings
 Required properties:
 - compatible		: Should be "bosch,c_can" for C_CAN controllers and
 			  "bosch,d_can" for D_CAN controllers.
-			  Can be "ti,dra7-d_can".
+			  Can be "ti,dra7-d_can" or "ti,am3352-d_can".
 - reg			: physical base address and size of the C_CAN/D_CAN
 			  registers map
 - interrupts		: property with a value describing the interrupt
diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
index 7a81db4..eb09068 100644
--- a/drivers/net/can/c_can/c_can_platform.c
+++ b/drivers/net/can/c_can/c_can_platform.c
@@ -205,6 +205,15 @@ static struct c_can_driver_data dra7_dcan_drvdata = {
 	.raminit_pulse = true,
 };
 
+static u8 am3352_raminit_start_bits[] = {0, 1};
+static u8 am3352_raminit_done_bits[] = {8, 9};
+static struct c_can_driver_data am3352_dcan_drvdata = {
+	.id = BOSCH_D_CAN,
+	.num_can = 2,
+	.raminit_start_bits = am3352_raminit_start_bits,
+	.raminit_done_bits = am3352_raminit_done_bits,
+};
+
 static struct platform_device_id c_can_id_table[] = {
 	{
 		.name = KBUILD_MODNAME,
@@ -226,6 +235,7 @@ static const struct of_device_id c_can_of_table[] = {
 	{ .compatible = "bosch,c_can", .data = &c_can_drvdata },
 	{ .compatible = "bosch,d_can", .data = &d_can_drvdata },
 	{ .compatible = "ti,dra7-d_can", .data = &dra7_dcan_drvdata },
+	{ .compatible = "ti,am3352-d_can", .data = &am3352_dcan_drvdata },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, c_can_of_table);
-- 
1.8.3.2

Re: [PATCH v4 6/8] net: can: c_can: Disable pins when CAN interface is down

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1817 bytes --]

On 11/07/2014 03:49 PM, Roger Quadros wrote:
> DRA7 CAN IP suffers from a problem which causes it to be prevented
> from fully turning OFF (i.e. stuck in transition) if the module was
> disabled while there was traffic on the CAN_RX line.
> 
> To work around this issue we select the SLEEP pin state by default
> on probe and use the DEFAULT pin state on CAN up and back to the
> SLEEP pin state on CAN down.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/net/can/c_can/c_can.c          | 20 ++++++++++++++++++++
>  drivers/net/can/c_can/c_can.h          |  1 +
>  drivers/net/can/c_can/c_can_platform.c | 20 ++++++++++++++++++++
>  3 files changed, 41 insertions(+)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 8e78bb4..4dfc3ce 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -35,6 +35,7 @@
>  #include <linux/list.h>
>  #include <linux/io.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/pinctrl/consumer.h>
>  
>  #include <linux/can.h>
>  #include <linux/can/dev.h>
> @@ -603,6 +604,15 @@ static int c_can_start(struct net_device *dev)
>  
>  	priv->can.state = CAN_STATE_ERROR_ACTIVE;
>  
> +	/* activate pins */
> +	if (!IS_ERR(priv->pinctrl)) {
> +		struct pinctrl_state *s;
> +
> +		s = pinctrl_lookup_state(priv->pinctrl, PINCTRL_STATE_DEFAULT);
> +		if (!IS_ERR(s))
> +			pinctrl_select_state(priv->pinctrl, s);
> +	}

Please put this common code into a seperate function.

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 6/8] net: can: c_can: Disable pins when CAN interface is down

[Roger Quadros]

On 11/07/2014 04:54 PM, Marc Kleine-Budde wrote:
> On 11/07/2014 03:49 PM, Roger Quadros wrote:
>> DRA7 CAN IP suffers from a problem which causes it to be prevented
>> from fully turning OFF (i.e. stuck in transition) if the module was
>> disabled while there was traffic on the CAN_RX line.
>>
>> To work around this issue we select the SLEEP pin state by default
>> on probe and use the DEFAULT pin state on CAN up and back to the
>> SLEEP pin state on CAN down.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>  drivers/net/can/c_can/c_can.c          | 20 ++++++++++++++++++++
>>  drivers/net/can/c_can/c_can.h          |  1 +
>>  drivers/net/can/c_can/c_can_platform.c | 20 ++++++++++++++++++++
>>  3 files changed, 41 insertions(+)
>>
>> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
>> index 8e78bb4..4dfc3ce 100644
>> --- a/drivers/net/can/c_can/c_can.c
>> +++ b/drivers/net/can/c_can/c_can.c
>> @@ -35,6 +35,7 @@
>>  #include <linux/list.h>
>>  #include <linux/io.h>
>>  #include <linux/pm_runtime.h>
>> +#include <linux/pinctrl/consumer.h>
>>  
>>  #include <linux/can.h>
>>  #include <linux/can/dev.h>
>> @@ -603,6 +604,15 @@ static int c_can_start(struct net_device *dev)
>>  
>>  	priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>  
>> +	/* activate pins */
>> +	if (!IS_ERR(priv->pinctrl)) {
>> +		struct pinctrl_state *s;
>> +
>> +		s = pinctrl_lookup_state(priv->pinctrl, PINCTRL_STATE_DEFAULT);
>> +		if (!IS_ERR(s))
>> +			pinctrl_select_state(priv->pinctrl, s);
>> +	}
> 
> Please put this common code into a seperate function.

Oops, forgot this one in a hurry.

cheers,
-roger

[PATCH v5 6/8] net: can: c_can: Disable pins when CAN interface is down

[Roger Quadros]

DRA7 CAN IP suffers from a problem which causes it to be prevented
from fully turning OFF (i.e. stuck in transition) if the module was
disabled while there was traffic on the CAN_RX line.

To work around this issue we select the SLEEP pin state by default
on probe and use the DEFAULT pin state on CAN up and back to the
SLEEP pin state on CAN down.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/net/can/c_can/c_can.c          | 21 +++++++++++++++++++++
 drivers/net/can/c_can/c_can.h          |  1 +
 drivers/net/can/c_can/c_can_platform.c | 20 ++++++++++++++++++++
 3 files changed, 42 insertions(+)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 8e78bb4..a950eea 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -35,6 +35,7 @@
 #include <linux/list.h>
 #include <linux/io.h>
 #include <linux/pm_runtime.h>
+#include <linux/pinctrl/consumer.h>
 
 #include <linux/can.h>
 #include <linux/can/dev.h>
@@ -587,6 +588,21 @@ static int c_can_chip_config(struct net_device *dev)
 	return c_can_set_bittiming(dev);
 }
 
+/*
+ * Selects the pinctrl state specified in the name.
+ */
+static void c_can_pinctrl_select_state(struct c_can_priv *priv,
+				      const char *name)
+{
+	if (!IS_ERR(priv->pinctrl)) {
+		struct pinctrl_state *s;
+
+		s = pinctrl_lookup_state(priv->pinctrl, name);
+		if (!IS_ERR(s))
+			pinctrl_select_state(priv->pinctrl, s);
+	}
+}
+
 static int c_can_start(struct net_device *dev)
 {
 	struct c_can_priv *priv = netdev_priv(dev);
@@ -603,6 +619,8 @@ static int c_can_start(struct net_device *dev)
 
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
+	/* activate pins */
+	c_can_pinctrl_select_state(priv, PINCTRL_STATE_DEFAULT);
 	return 0;
 }
 
@@ -611,6 +629,9 @@ static void c_can_stop(struct net_device *dev)
 	struct c_can_priv *priv = netdev_priv(dev);
 
 	c_can_irq_control(priv, false);
+
+	/* deactivate pins */
+	c_can_pinctrl_select_state(priv, PINCTRL_STATE_SLEEP);
 	priv->can.state = CAN_STATE_STOPPED;
 }
 
diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index c6715ca..3cedf48 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -210,6 +210,7 @@ struct c_can_priv {
 	u32 comm_rcv_high;
 	u32 rxmasked;
 	u32 dlc[C_CAN_MSG_OBJ_TX_NUM];
+	struct pinctrl *pinctrl;
 };
 
 struct net_device *alloc_c_can_dev(void);
diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
index b838c6b..71b9063 100644
--- a/drivers/net/can/c_can/c_can_platform.c
+++ b/drivers/net/can/c_can/c_can_platform.c
@@ -34,6 +34,7 @@
 #include <linux/of_device.h>
 #include <linux/mfd/syscon.h>
 #include <linux/regmap.h>
+#include <linux/pinctrl/consumer.h>
 
 #include <linux/can/dev.h>
 
@@ -230,6 +231,7 @@ static int c_can_plat_probe(struct platform_device *pdev)
 	struct clk *clk;
 	const struct c_can_driver_data *drvdata;
 	struct device_node *np = pdev->dev.of_node;
+	struct pinctrl *pinctrl;
 
 	match = of_match_device(c_can_of_table, &pdev->dev);
 	if (match) {
@@ -241,6 +243,23 @@ static int c_can_plat_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (!IS_ERR(pinctrl)) {
+		struct pinctrl_state *s;
+
+		/* Deactivate pins to prevent DRA7 DCAN IP from being
+		 * stuck in transition when module is disabled.
+		 * Pins are activated in c_can_start() and deactivated
+		 * in c_can_stop()
+		 */
+		s = pinctrl_lookup_state(pinctrl, PINCTRL_STATE_SLEEP);
+		if (!IS_ERR(s))
+			pinctrl_select_state(pinctrl, s);
+	} else {
+		dev_warn(&pdev->dev,
+			 "failed to get pinctrl\n");
+	}
+
 	/* get the appropriate clk */
 	clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(clk)) {
@@ -270,6 +289,7 @@ static int c_can_plat_probe(struct platform_device *pdev)
 	}
 
 	priv = netdev_priv(dev);
+	priv->pinctrl = pinctrl;
 
 	switch (drvdata->id) {
 	case BOSCH_C_CAN:
-- 
1.8.3.2

Re: [PATCH v4 2/8] net: can: c_can: Introduce c_can_driver_data structure

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1593 bytes --]

On 11/07/2014 03:49 PM, Roger Quadros wrote:
> We want to have more data than just can_dev_id to be present
> in the driver data e.g. TI platforms need RAMINIT register
> description. Introduce the c_can_driver_data structure and move
> the can_dev_id into it.
> 
> Tidy up the way it is used on probe().
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>

[...]

> @@ -198,21 +206,19 @@ static int c_can_plat_probe(struct platform_device *pdev)
>  	struct net_device *dev;
>  	struct c_can_priv *priv;
>  	const struct of_device_id *match;
> -	const struct platform_device_id *id;
>  	struct resource *mem, *res;
>  	int irq;
>  	struct clk *clk;
> -
> -	if (pdev->dev.of_node) {
> -		match = of_match_device(c_can_of_table, &pdev->dev);
> -		if (!match) {
> -			dev_err(&pdev->dev, "Failed to find matching dt id\n");
> -			ret = -EINVAL;
> -			goto exit;
> -		}
> -		id = match->data;
> +	const struct c_can_driver_data *drvdata;
> +
> +	match = of_match_device(c_can_of_table, &pdev->dev);
> +	if (match) {
> +		drvdata = match->data;
> +	} else if (pdev->id_entry->driver_data) {
> +		drvdata = (struct c_can_driver_data *)
> +			   pdev->id_entry->driver_data;
                           ^^^^^^^^^^^^^^
I've changes this to platform_get_device_id() while aplying.

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 3/8] net: can: c_can: Add RAMINIT register information to driver data

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1724 bytes --]

On 11/07/2014 03:49 PM, Roger Quadros wrote:
> Some platforms (e.g. TI) need special RAMINIT register handling.
> Provide a way to store RAMINIT register description in driver data.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/net/can/c_can/c_can.h          | 6 ++++++
>  drivers/net/can/c_can/c_can_platform.c | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> index 26c975d..3c305a1 100644
> --- a/drivers/net/can/c_can/c_can.h
> +++ b/drivers/net/can/c_can/c_can.h
> @@ -171,6 +171,12 @@ enum c_can_dev_id {
>  
>  struct c_can_driver_data {
>  	enum c_can_dev_id id;
> +
> +	/* RAMINIT register description. Optional. */
> +	u8 num_can;		/* Number of CAN instances on the SoC */
> +	u8 *raminit_start_bits;	/* Array of START bit positions */
> +	u8 *raminit_done_bits;	/* Array of DONE bit positions */
> +	bool raminit_pulse;	/* If set, sets and clears START bit (pulse) */
>  };
>  
>  /* c_can private data structure */
> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
> index 1546c2b..20deb67 100644
> --- a/drivers/net/can/c_can/c_can_platform.c
> +++ b/drivers/net/can/c_can/c_can_platform.c
> @@ -250,6 +250,7 @@ static int c_can_plat_probe(struct platform_device *pdev)
>  	}
>  
>  	priv = netdev_priv(dev);
> +

Dropped this hunk while applying.

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 4/8] net: can: c_can: Add syscon/regmap RAMINIT mechanism

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 940 bytes --]

On 11/07/2014 03:49 PM, Roger Quadros wrote:
> Some TI SoCs like DRA7 have a RAMINIT register specification
> different from the other AMxx SoCs and as expected by the
> existing driver.
> 
> To add more insanity, this register is shared with other
> IPs like DSS, PCIe and PWM.
> 
> Provides a more generic mechanism to specify the RAMINIT
> register location and START/DONE bit position and use the
> syscon/regmap framework to access the register.

What about the existing device trees that don't have the syscon-raminit
phandle? We can either keep the existing init routines or create regmap
in the platform driver an use the new ones.

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 4/8] net: can: c_can: Add syscon/regmap RAMINIT mechanism

[Roger Quadros]

On 11/13/2014 01:09 PM, Marc Kleine-Budde wrote:
> On 11/07/2014 03:49 PM, Roger Quadros wrote:
>> Some TI SoCs like DRA7 have a RAMINIT register specification
>> different from the other AMxx SoCs and as expected by the
>> existing driver.
>>
>> To add more insanity, this register is shared with other
>> IPs like DSS, PCIe and PWM.
>>
>> Provides a more generic mechanism to specify the RAMINIT
>> register location and START/DONE bit position and use the
>> syscon/regmap framework to access the register.
> 
> What about the existing device trees that don't have the syscon-raminit
> phandle? We can either keep the existing init routines or create regmap
> in the platform driver an use the new ones.

There is only one user
arch/arm/boot/dts/am33xx.dtsi

The can nodes are disabled there and no other board file is enabling that node.
So there is no breakage as such and not worth the hassle to maintain the old routine.

I will be sending the corresponding dts changes today which Tony will take as we don't see
any DT binding changes.

cheers,
-roger

Re: [PATCH v4 4/8] net: can: c_can: Add syscon/regmap RAMINIT mechanism

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 8054 bytes --]

On 11/07/2014 03:49 PM, Roger Quadros wrote:
> Some TI SoCs like DRA7 have a RAMINIT register specification
> different from the other AMxx SoCs and as expected by the
> existing driver.
> 
> To add more insanity, this register is shared with other
> IPs like DSS, PCIe and PWM.
> 
> Provides a more generic mechanism to specify the RAMINIT
> register location and START/DONE bit position and use the
> syscon/regmap framework to access the register.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  .../devicetree/bindings/net/can/c_can.txt          |   3 +
>  drivers/net/can/c_can/c_can.h                      |  11 +-
>  drivers/net/can/c_can/c_can_platform.c             | 112 ++++++++++++++-------
>  3 files changed, 86 insertions(+), 40 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/c_can.txt b/Documentation/devicetree/bindings/net/can/c_can.txt
> index 8f1ae81..a3ca3ee 100644
> --- a/Documentation/devicetree/bindings/net/can/c_can.txt
> +++ b/Documentation/devicetree/bindings/net/can/c_can.txt
> @@ -12,6 +12,9 @@ Required properties:
>  Optional properties:
>  - ti,hwmods		: Must be "d_can<n>" or "c_can<n>", n being the
>  			  instance number
> +- syscon-raminit	: Handle to system control region that contains the
> +			  RAMINIT register, register offset to the RAMINIT
> +			  register and the CAN instance number (0 offset).
>  
>  Note: "ti,hwmods" field is used to fetch the base address and irq
>  resources from TI, omap hwmod data base during device registration.
> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> index 3c305a1..0e17c7b 100644
> --- a/drivers/net/can/c_can/c_can.h
> +++ b/drivers/net/can/c_can/c_can.h
> @@ -179,6 +179,14 @@ struct c_can_driver_data {
>  	bool raminit_pulse;	/* If set, sets and clears START bit (pulse) */
>  };
>  
> +/* Out of band RAMINIT register access via syscon regmap */
> +struct c_can_raminit {
> +	struct regmap *syscon;	/* for raminit ctrl. reg. access */
> +	unsigned int reg;	/* register index within syscon */
> +	u8 start_bit;
> +	u8 done_bit;
> +};
> +
>  /* c_can private data structure */
>  struct c_can_priv {
>  	struct can_priv can;	/* must be the first member */
> @@ -196,8 +204,7 @@ struct c_can_priv {
>  	const u16 *regs;
>  	void *priv;		/* for board-specific data */
>  	enum c_can_dev_id type;
> -	u32 __iomem *raminit_ctrlreg;
> -	int instance;
> +	struct c_can_raminit raminit_sys;	/* RAMINIT via syscon regmap */
>  	void (*raminit) (const struct c_can_priv *priv, bool enable);
>  	u32 comm_rcv_high;
>  	u32 rxmasked;
> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
> index 20deb67..3776483 100644
> --- a/drivers/net/can/c_can/c_can_platform.c
> +++ b/drivers/net/can/c_can/c_can_platform.c
> @@ -32,14 +32,13 @@
>  #include <linux/clk.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
>  
>  #include <linux/can/dev.h>
>  
>  #include "c_can.h"
>  
> -#define CAN_RAMINIT_START_MASK(i)	(0x001 << (i))
> -#define CAN_RAMINIT_DONE_MASK(i)	(0x100 << (i))
> -#define CAN_RAMINIT_ALL_MASK(i)		(0x101 << (i))
>  #define DCAN_RAM_INIT_BIT		(1 << 3)
>  static DEFINE_SPINLOCK(raminit_lock);
>  /*
> @@ -72,47 +71,61 @@ static void c_can_plat_write_reg_aligned_to_32bit(const struct c_can_priv *priv,
>  	writew(val, priv->base + 2 * priv->regs[index]);
>  }
>  
> -static void c_can_hw_raminit_wait_ti(const struct c_can_priv *priv, u32 mask,
> -				  u32 val)
> +static void c_can_hw_raminit_wait_syscon(const struct c_can_priv *priv,
> +					 u32 mask, u32 val)
>  {
>  	int timeout = 0;
> +	const struct c_can_raminit *raminit = &priv->raminit_sys;
> +	u32 ctrl;
> +
>  	/* We look only at the bits of our instance. */
>  	val &= mask;
> -	while ((readl(priv->raminit_ctrlreg) & mask) != val) {
> +	do {
>  		udelay(1);
>  		timeout++;
>  
> +		regmap_read(raminit->syscon, raminit->reg, &ctrl);
>  		if (timeout == 1000) {
>  			dev_err(&priv->dev->dev, "%s: time out\n", __func__);
>  			break;
>  		}
> -	}
> +	} while ((ctrl & mask) != val);
>  }
>  
> -static void c_can_hw_raminit_ti(const struct c_can_priv *priv, bool enable)
> +static void c_can_hw_raminit_syscon(const struct c_can_priv *priv, bool enable)
>  {
> -	u32 mask = CAN_RAMINIT_ALL_MASK(priv->instance);
> +	u32 mask;
>  	u32 ctrl;
> +	const struct c_can_raminit *raminit = &priv->raminit_sys;
> +	u8 start_bit, done_bit;
> +
> +	start_bit = raminit->start_bit;
> +	done_bit = raminit->done_bit;
>  
>  	spin_lock(&raminit_lock);
>  
> -	ctrl = readl(priv->raminit_ctrlreg);
> +	mask = 1 << start_bit | 1 << done_bit;
> +	regmap_read(raminit->syscon, raminit->reg, &ctrl);
> +
>  	/* We clear the done and start bit first. The start bit is
>  	 * looking at the 0 -> transition, but is not self clearing;
>  	 * And we clear the init done bit as well.
> +	 * NOTE: DONE must be written with 1 to clear it.
>  	 */
> -	ctrl &= ~CAN_RAMINIT_START_MASK(priv->instance);
> -	ctrl |= CAN_RAMINIT_DONE_MASK(priv->instance);
> -	writel(ctrl, priv->raminit_ctrlreg);
> -	ctrl &= ~CAN_RAMINIT_DONE_MASK(priv->instance);
> -	c_can_hw_raminit_wait_ti(priv, mask, ctrl);
> +	ctrl &= ~(1 << start_bit);
> +	ctrl |= 1 << done_bit;
> +	regmap_write(raminit->syscon, raminit->reg, ctrl);
> +
> +	ctrl &= ~(1 << done_bit);
> +	c_can_hw_raminit_wait_syscon(priv, mask, ctrl);
>  
>  	if (enable) {
>  		/* Set start bit and wait for the done bit. */
> -		ctrl |= CAN_RAMINIT_START_MASK(priv->instance);
> -		writel(ctrl, priv->raminit_ctrlreg);
> -		ctrl |= CAN_RAMINIT_DONE_MASK(priv->instance);
> -		c_can_hw_raminit_wait_ti(priv, mask, ctrl);
> +		ctrl |= 1 << start_bit;
> +		regmap_write(raminit->syscon, raminit->reg, ctrl);
> +
> +		ctrl |= 1 << done_bit;
> +		c_can_hw_raminit_wait_syscon(priv, mask, ctrl);
>  	}
>  	spin_unlock(&raminit_lock);
>  }
> @@ -206,10 +219,11 @@ static int c_can_plat_probe(struct platform_device *pdev)
>  	struct net_device *dev;
>  	struct c_can_priv *priv;
>  	const struct of_device_id *match;
> -	struct resource *mem, *res;
> +	struct resource *mem;
>  	int irq;
>  	struct clk *clk;
>  	const struct c_can_driver_data *drvdata;
> +	struct device_node *np = pdev->dev.of_node;
>  
>  	match = of_match_device(c_can_of_table, &pdev->dev);
>  	if (match) {
> @@ -278,27 +292,49 @@ static int c_can_plat_probe(struct platform_device *pdev)
>  		priv->read_reg32 = d_can_plat_read_reg32;
>  		priv->write_reg32 = d_can_plat_write_reg32;
>  
> -		if (pdev->dev.of_node)
> -			priv->instance = of_alias_get_id(pdev->dev.of_node, "d_can");
> -		else
> -			priv->instance = pdev->id;
> -
> -		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> -		/* Not all D_CAN modules have a separate register for the D_CAN
> -		 * RAM initialization. Use default RAM init bit in D_CAN module
> -		 * if not specified in DT.
> +		/* Check if we need custom RAMINIT via syscon. Mostly for TI
> +		 * platforms. Only supported with DT boot.
>  		 */
> -		if (!res) {
> +		if (np && of_property_read_bool(np, "syscon-raminit")) {
> +			u32 id;
> +			struct c_can_raminit *raminit = &priv->raminit_sys;
> +
> +			ret = -EINVAL;
> +			raminit->syscon = syscon_regmap_lookup_by_phandle(np,
> +									  "syscon-raminit");

You should return PTR_ERR() here, as it it might be -EPROBE_DEFER

> +			if (IS_ERR(raminit->syscon)) {
> +				dev_err(&pdev->dev,
> +					"couldn't get syscon regmap for RAMINIT\n");
> +				goto exit_free_device;
> +			}

...and maybe remove this error message completely.

regards,
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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 6/8] net: can: c_can: Disable pins when CAN interface is down

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 4856 bytes --]

On 11/12/2014 03:16 PM, Roger Quadros wrote:
> DRA7 CAN IP suffers from a problem which causes it to be prevented
> from fully turning OFF (i.e. stuck in transition) if the module was
> disabled while there was traffic on the CAN_RX line.
> 
> To work around this issue we select the SLEEP pin state by default
> on probe and use the DEFAULT pin state on CAN up and back to the
> SLEEP pin state on CAN down.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/net/can/c_can/c_can.c          | 21 +++++++++++++++++++++
>  drivers/net/can/c_can/c_can.h          |  1 +
>  drivers/net/can/c_can/c_can_platform.c | 20 ++++++++++++++++++++
>  3 files changed, 42 insertions(+)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 8e78bb4..a950eea 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -35,6 +35,7 @@
>  #include <linux/list.h>
>  #include <linux/io.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/pinctrl/consumer.h>
>  
>  #include <linux/can.h>
>  #include <linux/can/dev.h>
> @@ -587,6 +588,21 @@ static int c_can_chip_config(struct net_device *dev)
>  	return c_can_set_bittiming(dev);
>  }
>  
> +/*
> + * Selects the pinctrl state specified in the name.
> + */
> +static void c_can_pinctrl_select_state(struct c_can_priv *priv,
> +				      const char *name)
> +{
> +	if (!IS_ERR(priv->pinctrl)) {

What happens if priv->pinctrl is NULL? This is probably the case with
the c_can_pci driver.

> +		struct pinctrl_state *s;
> +
> +		s = pinctrl_lookup_state(priv->pinctrl, name);
> +		if (!IS_ERR(s))
> +			pinctrl_select_state(priv->pinctrl, s);
> +	}
> +}
> +
>  static int c_can_start(struct net_device *dev)
>  {
>  	struct c_can_priv *priv = netdev_priv(dev);
> @@ -603,6 +619,8 @@ static int c_can_start(struct net_device *dev)
>  
>  	priv->can.state = CAN_STATE_ERROR_ACTIVE;
>  
> +	/* activate pins */
> +	c_can_pinctrl_select_state(priv, PINCTRL_STATE_DEFAULT);
>  	return 0;
>  }
>  
> @@ -611,6 +629,9 @@ static void c_can_stop(struct net_device *dev)
>  	struct c_can_priv *priv = netdev_priv(dev);
>  
>  	c_can_irq_control(priv, false);
> +
> +	/* deactivate pins */
> +	c_can_pinctrl_select_state(priv, PINCTRL_STATE_SLEEP);
>  	priv->can.state = CAN_STATE_STOPPED;
>  }
>  
> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> index c6715ca..3cedf48 100644
> --- a/drivers/net/can/c_can/c_can.h
> +++ b/drivers/net/can/c_can/c_can.h
> @@ -210,6 +210,7 @@ struct c_can_priv {
>  	u32 comm_rcv_high;
>  	u32 rxmasked;
>  	u32 dlc[C_CAN_MSG_OBJ_TX_NUM];
> +	struct pinctrl *pinctrl;
>  };
>  
>  struct net_device *alloc_c_can_dev(void);
> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
> index b838c6b..71b9063 100644
> --- a/drivers/net/can/c_can/c_can_platform.c
> +++ b/drivers/net/can/c_can/c_can_platform.c
> @@ -34,6 +34,7 @@
>  #include <linux/of_device.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/regmap.h>
> +#include <linux/pinctrl/consumer.h>
>  
>  #include <linux/can/dev.h>
>  
> @@ -230,6 +231,7 @@ static int c_can_plat_probe(struct platform_device *pdev)
>  	struct clk *clk;
>  	const struct c_can_driver_data *drvdata;
>  	struct device_node *np = pdev->dev.of_node;
> +	struct pinctrl *pinctrl;
>  
>  	match = of_match_device(c_can_of_table, &pdev->dev);
>  	if (match) {
> @@ -241,6 +243,23 @@ static int c_can_plat_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> +	pinctrl = devm_pinctrl_get(&pdev->dev);
> +	if (!IS_ERR(pinctrl)) {
> +		struct pinctrl_state *s;
> +
> +		/* Deactivate pins to prevent DRA7 DCAN IP from being
> +		 * stuck in transition when module is disabled.
> +		 * Pins are activated in c_can_start() and deactivated
> +		 * in c_can_stop()
> +		 */
> +		s = pinctrl_lookup_state(pinctrl, PINCTRL_STATE_SLEEP);
> +		if (!IS_ERR(s))
> +			pinctrl_select_state(pinctrl, s);
> +	} else {
> +		dev_warn(&pdev->dev,
> +			 "failed to get pinctrl\n");
> +	}

Can you move the initial setting into c_can.c, register_c_can_dev()
should be a good candidate.

> +
>  	/* get the appropriate clk */
>  	clk = devm_clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(clk)) {
> @@ -270,6 +289,7 @@ static int c_can_plat_probe(struct platform_device *pdev)
>  	}
>  
>  	priv = netdev_priv(dev);
> +	priv->pinctrl = pinctrl;
>  
>  	switch (drvdata->id) {
>  	case BOSCH_C_CAN:
> 

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 4/8] net: can: c_can: Add syscon/regmap RAMINIT mechanism

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1003 bytes --]

On 11/13/2014 01:09 PM, Roger Quadros wrote:
>> What about the existing device trees that don't have the syscon-raminit
>> phandle? We can either keep the existing init routines or create regmap
>> in the platform driver an use the new ones.
> 
> There is only one user
> arch/arm/boot/dts/am33xx.dtsi
> 
> The can nodes are disabled there and no other board file is enabling that node.
> So there is no breakage as such and not worth the hassle to maintain the old routine.
> 
> I will be sending the corresponding dts changes today which Tony will take as we don't see
> any DT binding changes.

Alright then (I'll give your email to $CUSTOMERS complaining ;) ), this
whole series will go into 3.19.

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 6/8] net: can: c_can: Disable pins when CAN interface is down

[Roger Quadros]

On 11/13/2014 02:56 PM, Marc Kleine-Budde wrote:
> On 11/12/2014 03:16 PM, Roger Quadros wrote:
>> DRA7 CAN IP suffers from a problem which causes it to be prevented
>> from fully turning OFF (i.e. stuck in transition) if the module was
>> disabled while there was traffic on the CAN_RX line.
>>
>> To work around this issue we select the SLEEP pin state by default
>> on probe and use the DEFAULT pin state on CAN up and back to the
>> SLEEP pin state on CAN down.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>  drivers/net/can/c_can/c_can.c          | 21 +++++++++++++++++++++
>>  drivers/net/can/c_can/c_can.h          |  1 +
>>  drivers/net/can/c_can/c_can_platform.c | 20 ++++++++++++++++++++
>>  3 files changed, 42 insertions(+)
>>
>> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
>> index 8e78bb4..a950eea 100644
>> --- a/drivers/net/can/c_can/c_can.c
>> +++ b/drivers/net/can/c_can/c_can.c
>> @@ -35,6 +35,7 @@
>>  #include <linux/list.h>
>>  #include <linux/io.h>
>>  #include <linux/pm_runtime.h>
>> +#include <linux/pinctrl/consumer.h>
>>  
>>  #include <linux/can.h>
>>  #include <linux/can/dev.h>
>> @@ -587,6 +588,21 @@ static int c_can_chip_config(struct net_device *dev)
>>  	return c_can_set_bittiming(dev);
>>  }
>>  
>> +/*
>> + * Selects the pinctrl state specified in the name.
>> + */
>> +static void c_can_pinctrl_select_state(struct c_can_priv *priv,
>> +				      const char *name)
>> +{
>> +	if (!IS_ERR(priv->pinctrl)) {
> 
> What happens if priv->pinctrl is NULL? This is probably the case with
> the c_can_pci driver.

There will be a NULL pointer dereference error. :(

> 
>> +		struct pinctrl_state *s;
>> +
>> +		s = pinctrl_lookup_state(priv->pinctrl, name);
>> +		if (!IS_ERR(s))
>> +			pinctrl_select_state(priv->pinctrl, s);
>> +	}
>> +}
>> +
>>  static int c_can_start(struct net_device *dev)
>>  {
>>  	struct c_can_priv *priv = netdev_priv(dev);
>> @@ -603,6 +619,8 @@ static int c_can_start(struct net_device *dev)
>>  
>>  	priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>  
>> +	/* activate pins */
>> +	c_can_pinctrl_select_state(priv, PINCTRL_STATE_DEFAULT);
>>  	return 0;
>>  }
>>  
>> @@ -611,6 +629,9 @@ static void c_can_stop(struct net_device *dev)
>>  	struct c_can_priv *priv = netdev_priv(dev);
>>  
>>  	c_can_irq_control(priv, false);
>> +
>> +	/* deactivate pins */
>> +	c_can_pinctrl_select_state(priv, PINCTRL_STATE_SLEEP);
>>  	priv->can.state = CAN_STATE_STOPPED;
>>  }
>>  
>> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
>> index c6715ca..3cedf48 100644
>> --- a/drivers/net/can/c_can/c_can.h
>> +++ b/drivers/net/can/c_can/c_can.h
>> @@ -210,6 +210,7 @@ struct c_can_priv {
>>  	u32 comm_rcv_high;
>>  	u32 rxmasked;
>>  	u32 dlc[C_CAN_MSG_OBJ_TX_NUM];
>> +	struct pinctrl *pinctrl;
>>  };
>>  
>>  struct net_device *alloc_c_can_dev(void);
>> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
>> index b838c6b..71b9063 100644
>> --- a/drivers/net/can/c_can/c_can_platform.c
>> +++ b/drivers/net/can/c_can/c_can_platform.c
>> @@ -34,6 +34,7 @@
>>  #include <linux/of_device.h>
>>  #include <linux/mfd/syscon.h>
>>  #include <linux/regmap.h>
>> +#include <linux/pinctrl/consumer.h>
>>  
>>  #include <linux/can/dev.h>
>>  
>> @@ -230,6 +231,7 @@ static int c_can_plat_probe(struct platform_device *pdev)
>>  	struct clk *clk;
>>  	const struct c_can_driver_data *drvdata;
>>  	struct device_node *np = pdev->dev.of_node;
>> +	struct pinctrl *pinctrl;
>>  
>>  	match = of_match_device(c_can_of_table, &pdev->dev);
>>  	if (match) {
>> @@ -241,6 +243,23 @@ static int c_can_plat_probe(struct platform_device *pdev)
>>  		return -ENODEV;
>>  	}
>>  
>> +	pinctrl = devm_pinctrl_get(&pdev->dev);
>> +	if (!IS_ERR(pinctrl)) {
>> +		struct pinctrl_state *s;
>> +
>> +		/* Deactivate pins to prevent DRA7 DCAN IP from being
>> +		 * stuck in transition when module is disabled.
>> +		 * Pins are activated in c_can_start() and deactivated
>> +		 * in c_can_stop()
>> +		 */
>> +		s = pinctrl_lookup_state(pinctrl, PINCTRL_STATE_SLEEP);
>> +		if (!IS_ERR(s))
>> +			pinctrl_select_state(pinctrl, s);
>> +	} else {
>> +		dev_warn(&pdev->dev,
>> +			 "failed to get pinctrl\n");
>> +	}
> 
> Can you move the initial setting into c_can.c, register_c_can_dev()
> should be a good candidate.

OK. This should also address the NULL pointer issue you pointed out earlier with c_can_pci.

cheers,
-roger


> 
>> +
>>  	/* get the appropriate clk */
>>  	clk = devm_clk_get(&pdev->dev, NULL);
>>  	if (IS_ERR(clk)) {
>> @@ -270,6 +289,7 @@ static int c_can_plat_probe(struct platform_device *pdev)
>>  	}
>>  
>>  	priv = netdev_priv(dev);
>> +	priv->pinctrl = pinctrl;
>>  
>>  	switch (drvdata->id) {
>>  	case BOSCH_C_CAN:
>>
> 
> Marc
> 

Re: [PATCH v4 4/8] net: can: c_can: Add syscon/regmap RAMINIT mechanism

[Roger Quadros]

On 11/13/2014 02:44 PM, Marc Kleine-Budde wrote:
> On 11/07/2014 03:49 PM, Roger Quadros wrote:
>> Some TI SoCs like DRA7 have a RAMINIT register specification
>> different from the other AMxx SoCs and as expected by the
>> existing driver.
>>
>> To add more insanity, this register is shared with other
>> IPs like DSS, PCIe and PWM.
>>
>> Provides a more generic mechanism to specify the RAMINIT
>> register location and START/DONE bit position and use the
>> syscon/regmap framework to access the register.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>  .../devicetree/bindings/net/can/c_can.txt          |   3 +
>>  drivers/net/can/c_can/c_can.h                      |  11 +-
>>  drivers/net/can/c_can/c_can_platform.c             | 112 ++++++++++++++-------
>>  3 files changed, 86 insertions(+), 40 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/can/c_can.txt b/Documentation/devicetree/bindings/net/can/c_can.txt
>> index 8f1ae81..a3ca3ee 100644
>> --- a/Documentation/devicetree/bindings/net/can/c_can.txt
>> +++ b/Documentation/devicetree/bindings/net/can/c_can.txt
>> @@ -12,6 +12,9 @@ Required properties:
>>  Optional properties:
>>  - ti,hwmods		: Must be "d_can<n>" or "c_can<n>", n being the
>>  			  instance number
>> +- syscon-raminit	: Handle to system control region that contains the
>> +			  RAMINIT register, register offset to the RAMINIT
>> +			  register and the CAN instance number (0 offset).
>>  
>>  Note: "ti,hwmods" field is used to fetch the base address and irq
>>  resources from TI, omap hwmod data base during device registration.
>> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
>> index 3c305a1..0e17c7b 100644
>> --- a/drivers/net/can/c_can/c_can.h
>> +++ b/drivers/net/can/c_can/c_can.h
>> @@ -179,6 +179,14 @@ struct c_can_driver_data {
>>  	bool raminit_pulse;	/* If set, sets and clears START bit (pulse) */
>>  };
>>  
>> +/* Out of band RAMINIT register access via syscon regmap */
>> +struct c_can_raminit {
>> +	struct regmap *syscon;	/* for raminit ctrl. reg. access */
>> +	unsigned int reg;	/* register index within syscon */
>> +	u8 start_bit;
>> +	u8 done_bit;
>> +};
>> +
>>  /* c_can private data structure */
>>  struct c_can_priv {
>>  	struct can_priv can;	/* must be the first member */
>> @@ -196,8 +204,7 @@ struct c_can_priv {
>>  	const u16 *regs;
>>  	void *priv;		/* for board-specific data */
>>  	enum c_can_dev_id type;
>> -	u32 __iomem *raminit_ctrlreg;
>> -	int instance;
>> +	struct c_can_raminit raminit_sys;	/* RAMINIT via syscon regmap */
>>  	void (*raminit) (const struct c_can_priv *priv, bool enable);
>>  	u32 comm_rcv_high;
>>  	u32 rxmasked;
>> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
>> index 20deb67..3776483 100644
>> --- a/drivers/net/can/c_can/c_can_platform.c
>> +++ b/drivers/net/can/c_can/c_can_platform.c
>> @@ -32,14 +32,13 @@
>>  #include <linux/clk.h>
>>  #include <linux/of.h>
>>  #include <linux/of_device.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/regmap.h>
>>  
>>  #include <linux/can/dev.h>
>>  
>>  #include "c_can.h"
>>  
>> -#define CAN_RAMINIT_START_MASK(i)	(0x001 << (i))
>> -#define CAN_RAMINIT_DONE_MASK(i)	(0x100 << (i))
>> -#define CAN_RAMINIT_ALL_MASK(i)		(0x101 << (i))
>>  #define DCAN_RAM_INIT_BIT		(1 << 3)
>>  static DEFINE_SPINLOCK(raminit_lock);
>>  /*
>> @@ -72,47 +71,61 @@ static void c_can_plat_write_reg_aligned_to_32bit(const struct c_can_priv *priv,
>>  	writew(val, priv->base + 2 * priv->regs[index]);
>>  }
>>  
>> -static void c_can_hw_raminit_wait_ti(const struct c_can_priv *priv, u32 mask,
>> -				  u32 val)
>> +static void c_can_hw_raminit_wait_syscon(const struct c_can_priv *priv,
>> +					 u32 mask, u32 val)
>>  {
>>  	int timeout = 0;
>> +	const struct c_can_raminit *raminit = &priv->raminit_sys;
>> +	u32 ctrl;
>> +
>>  	/* We look only at the bits of our instance. */
>>  	val &= mask;
>> -	while ((readl(priv->raminit_ctrlreg) & mask) != val) {
>> +	do {
>>  		udelay(1);
>>  		timeout++;
>>  
>> +		regmap_read(raminit->syscon, raminit->reg, &ctrl);
>>  		if (timeout == 1000) {
>>  			dev_err(&priv->dev->dev, "%s: time out\n", __func__);
>>  			break;
>>  		}
>> -	}
>> +	} while ((ctrl & mask) != val);
>>  }
>>  
>> -static void c_can_hw_raminit_ti(const struct c_can_priv *priv, bool enable)
>> +static void c_can_hw_raminit_syscon(const struct c_can_priv *priv, bool enable)
>>  {
>> -	u32 mask = CAN_RAMINIT_ALL_MASK(priv->instance);
>> +	u32 mask;
>>  	u32 ctrl;
>> +	const struct c_can_raminit *raminit = &priv->raminit_sys;
>> +	u8 start_bit, done_bit;
>> +
>> +	start_bit = raminit->start_bit;
>> +	done_bit = raminit->done_bit;
>>  
>>  	spin_lock(&raminit_lock);
>>  
>> -	ctrl = readl(priv->raminit_ctrlreg);
>> +	mask = 1 << start_bit | 1 << done_bit;
>> +	regmap_read(raminit->syscon, raminit->reg, &ctrl);
>> +
>>  	/* We clear the done and start bit first. The start bit is
>>  	 * looking at the 0 -> transition, but is not self clearing;
>>  	 * And we clear the init done bit as well.
>> +	 * NOTE: DONE must be written with 1 to clear it.
>>  	 */
>> -	ctrl &= ~CAN_RAMINIT_START_MASK(priv->instance);
>> -	ctrl |= CAN_RAMINIT_DONE_MASK(priv->instance);
>> -	writel(ctrl, priv->raminit_ctrlreg);
>> -	ctrl &= ~CAN_RAMINIT_DONE_MASK(priv->instance);
>> -	c_can_hw_raminit_wait_ti(priv, mask, ctrl);
>> +	ctrl &= ~(1 << start_bit);
>> +	ctrl |= 1 << done_bit;
>> +	regmap_write(raminit->syscon, raminit->reg, ctrl);
>> +
>> +	ctrl &= ~(1 << done_bit);
>> +	c_can_hw_raminit_wait_syscon(priv, mask, ctrl);
>>  
>>  	if (enable) {
>>  		/* Set start bit and wait for the done bit. */
>> -		ctrl |= CAN_RAMINIT_START_MASK(priv->instance);
>> -		writel(ctrl, priv->raminit_ctrlreg);
>> -		ctrl |= CAN_RAMINIT_DONE_MASK(priv->instance);
>> -		c_can_hw_raminit_wait_ti(priv, mask, ctrl);
>> +		ctrl |= 1 << start_bit;
>> +		regmap_write(raminit->syscon, raminit->reg, ctrl);
>> +
>> +		ctrl |= 1 << done_bit;
>> +		c_can_hw_raminit_wait_syscon(priv, mask, ctrl);
>>  	}
>>  	spin_unlock(&raminit_lock);
>>  }
>> @@ -206,10 +219,11 @@ static int c_can_plat_probe(struct platform_device *pdev)
>>  	struct net_device *dev;
>>  	struct c_can_priv *priv;
>>  	const struct of_device_id *match;
>> -	struct resource *mem, *res;
>> +	struct resource *mem;
>>  	int irq;
>>  	struct clk *clk;
>>  	const struct c_can_driver_data *drvdata;
>> +	struct device_node *np = pdev->dev.of_node;
>>  
>>  	match = of_match_device(c_can_of_table, &pdev->dev);
>>  	if (match) {
>> @@ -278,27 +292,49 @@ static int c_can_plat_probe(struct platform_device *pdev)
>>  		priv->read_reg32 = d_can_plat_read_reg32;
>>  		priv->write_reg32 = d_can_plat_write_reg32;
>>  
>> -		if (pdev->dev.of_node)
>> -			priv->instance = of_alias_get_id(pdev->dev.of_node, "d_can");
>> -		else
>> -			priv->instance = pdev->id;
>> -
>> -		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> -		/* Not all D_CAN modules have a separate register for the D_CAN
>> -		 * RAM initialization. Use default RAM init bit in D_CAN module
>> -		 * if not specified in DT.
>> +		/* Check if we need custom RAMINIT via syscon. Mostly for TI
>> +		 * platforms. Only supported with DT boot.
>>  		 */
>> -		if (!res) {
>> +		if (np && of_property_read_bool(np, "syscon-raminit")) {
>> +			u32 id;
>> +			struct c_can_raminit *raminit = &priv->raminit_sys;
>> +
>> +			ret = -EINVAL;
>> +			raminit->syscon = syscon_regmap_lookup_by_phandle(np,
>> +									  "syscon-raminit");
> 
> You should return PTR_ERR() here, as it it might be -EPROBE_DEFER

Good catch.

> 
>> +			if (IS_ERR(raminit->syscon)) {
>> +				dev_err(&pdev->dev,
>> +					"couldn't get syscon regmap for RAMINIT\n");
>> +				goto exit_free_device;
>> +			}
> 
> ...and maybe remove this error message completely.

OK.

cheers,
-roger

[PATCH v5 6/8] net: can: c_can: Disable pins when CAN interface is down

[Roger Quadros]

DRA7 CAN IP suffers from a problem which causes it to be prevented
from fully turning OFF (i.e. stuck in transition) if the module was
disabled while there was traffic on the CAN_RX line.

To work around this issue we select the SLEEP pin state by default
on probe and use the DEFAULT pin state on CAN up and back to the
SLEEP pin state on CAN down.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/net/can/c_can/c_can.c | 37 +++++++++++++++++++++++++++++++++++++
 drivers/net/can/c_can/c_can.h |  1 +
 2 files changed, 38 insertions(+)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 8e78bb4..c80cb3d 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -35,6 +35,7 @@
 #include <linux/list.h>
 #include <linux/io.h>
 #include <linux/pm_runtime.h>
+#include <linux/pinctrl/consumer.h>
 
 #include <linux/can.h>
 #include <linux/can/dev.h>
@@ -587,6 +588,21 @@ static int c_can_chip_config(struct net_device *dev)
 	return c_can_set_bittiming(dev);
 }
 
+/*
+ * Selects the pinctrl state specified in the name.
+ */
+static void c_can_pinctrl_select_state(struct c_can_priv *priv,
+				      const char *name)
+{
+	if (!IS_ERR(priv->pinctrl)) {
+		struct pinctrl_state *s;
+
+		s = pinctrl_lookup_state(priv->pinctrl, name);
+		if (!IS_ERR(s))
+			pinctrl_select_state(priv->pinctrl, s);
+	}
+}
+
 static int c_can_start(struct net_device *dev)
 {
 	struct c_can_priv *priv = netdev_priv(dev);
@@ -603,6 +619,8 @@ static int c_can_start(struct net_device *dev)
 
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
+	/* activate pins */
+	c_can_pinctrl_select_state(priv, PINCTRL_STATE_DEFAULT);
 	return 0;
 }
 
@@ -611,6 +629,9 @@ static void c_can_stop(struct net_device *dev)
 	struct c_can_priv *priv = netdev_priv(dev);
 
 	c_can_irq_control(priv, false);
+
+	/* deactivate pins */
+	c_can_pinctrl_select_state(priv, PINCTRL_STATE_SLEEP);
 	priv->can.state = CAN_STATE_STOPPED;
 }
 
@@ -1244,6 +1265,22 @@ int register_c_can_dev(struct net_device *dev)
 	struct c_can_priv *priv = netdev_priv(dev);
 	int err;
 
+	priv->pinctrl = devm_pinctrl_get(dev->dev.parent);
+	if (!IS_ERR(priv->pinctrl)) {
+		struct pinctrl_state *s;
+
+		/* Deactivate pins to prevent DRA7 DCAN IP from being
+		 * stuck in transition when module is disabled.
+		 * Pins are activated in c_can_start() and deactivated
+		 * in c_can_stop()
+		 */
+		s = pinctrl_lookup_state(priv->pinctrl, PINCTRL_STATE_SLEEP);
+		if (!IS_ERR(s))
+			pinctrl_select_state(priv->pinctrl, s);
+	} else {
+		netdev_dbg(dev, "failed to get pinctrl\n");
+	}
+
 	c_can_pm_runtime_enable(priv);
 
 	dev->flags |= IFF_ECHO;	/* we support local echo */
diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index c6715ca..3cedf48 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -210,6 +210,7 @@ struct c_can_priv {
 	u32 comm_rcv_high;
 	u32 rxmasked;
 	u32 dlc[C_CAN_MSG_OBJ_TX_NUM];
+	struct pinctrl *pinctrl;
 };
 
 struct net_device *alloc_c_can_dev(void);
-- 
1.8.3.2

Re: [PATCH v5 6/8] net: can: c_can: Disable pins when CAN interface is down

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 4251 bytes --]

On 11/13/2014 04:23 PM, Roger Quadros wrote:
> DRA7 CAN IP suffers from a problem which causes it to be prevented
> from fully turning OFF (i.e. stuck in transition) if the module was
> disabled while there was traffic on the CAN_RX line.
> 
> To work around this issue we select the SLEEP pin state by default
> on probe and use the DEFAULT pin state on CAN up and back to the
> SLEEP pin state on CAN down.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/net/can/c_can/c_can.c | 37 +++++++++++++++++++++++++++++++++++++
>  drivers/net/can/c_can/c_can.h |  1 +
>  2 files changed, 38 insertions(+)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 8e78bb4..c80cb3d 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -35,6 +35,7 @@
>  #include <linux/list.h>
>  #include <linux/io.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/pinctrl/consumer.h>
>  
>  #include <linux/can.h>
>  #include <linux/can/dev.h>
> @@ -587,6 +588,21 @@ static int c_can_chip_config(struct net_device *dev)
>  	return c_can_set_bittiming(dev);
>  }
>  
> +/*
> + * Selects the pinctrl state specified in the name.
> + */
> +static void c_can_pinctrl_select_state(struct c_can_priv *priv,
> +				      const char *name)
> +{
> +	if (!IS_ERR(priv->pinctrl)) {
> +		struct pinctrl_state *s;
> +
> +		s = pinctrl_lookup_state(priv->pinctrl, name);
> +		if (!IS_ERR(s))
> +			pinctrl_select_state(priv->pinctrl, s);
> +	}
> +}
> +
>  static int c_can_start(struct net_device *dev)
>  {
>  	struct c_can_priv *priv = netdev_priv(dev);
> @@ -603,6 +619,8 @@ static int c_can_start(struct net_device *dev)
>  
>  	priv->can.state = CAN_STATE_ERROR_ACTIVE;
>  
> +	/* activate pins */
> +	c_can_pinctrl_select_state(priv, PINCTRL_STATE_DEFAULT);
>  	return 0;
>  }
>  
> @@ -611,6 +629,9 @@ static void c_can_stop(struct net_device *dev)
>  	struct c_can_priv *priv = netdev_priv(dev);
>  
>  	c_can_irq_control(priv, false);
> +
> +	/* deactivate pins */
> +	c_can_pinctrl_select_state(priv, PINCTRL_STATE_SLEEP);
>  	priv->can.state = CAN_STATE_STOPPED;
>  }
>  
> @@ -1244,6 +1265,22 @@ int register_c_can_dev(struct net_device *dev)
>  	struct c_can_priv *priv = netdev_priv(dev);
>  	int err;
>  
> +	priv->pinctrl = devm_pinctrl_get(dev->dev.parent);

What's dev->dev.parent?
Ah, this is set by SET_NETDEV_DEV(dev, &pdev->dev); Good work!

I thought we had to set priv->pinctrl in the platform.c.

> +	if (!IS_ERR(priv->pinctrl)) {
> +		struct pinctrl_state *s;
> +
> +		/* Deactivate pins to prevent DRA7 DCAN IP from being
> +		 * stuck in transition when module is disabled.
> +		 * Pins are activated in c_can_start() and deactivated
> +		 * in c_can_stop()
> +		 */
> +		s = pinctrl_lookup_state(priv->pinctrl, PINCTRL_STATE_SLEEP);
> +		if (!IS_ERR(s))
> +			pinctrl_select_state(priv->pinctrl, s);
> +	} else {
> +		netdev_dbg(dev, "failed to get pinctrl\n");
> +	}
> +
The above can be replace by this?

	c_can_pinctrl_select_state(priv, PINCTRL_STATE_SLEEP)

Then we don't have the worrying looking error message if there isn't any
pinctrl, e.g. for the PCI driver with pinctrl enabled in the Kernel.

I just stumbled over pinctrl_pm_select_sleep_state(), is it possible to
integrate this into runtime pm?

http://lxr.free-electrons.com/source/drivers/pinctrl/core.c#L1282

>  	c_can_pm_runtime_enable(priv);
>  
>  	dev->flags |= IFF_ECHO;	/* we support local echo */
> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> index c6715ca..3cedf48 100644
> --- a/drivers/net/can/c_can/c_can.h
> +++ b/drivers/net/can/c_can/c_can.h
> @@ -210,6 +210,7 @@ struct c_can_priv {
>  	u32 comm_rcv_high;
>  	u32 rxmasked;
>  	u32 dlc[C_CAN_MSG_OBJ_TX_NUM];
> +	struct pinctrl *pinctrl;
>  };
>  
>  struct net_device *alloc_c_can_dev(void);
> 

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 8/8] net: can: c_can: Add support for TI am3352 DCAN

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 260 bytes --]

On Fri, Nov 07, 2014 at 04:49:22PM +0200, Roger Quadros wrote:
> AM3352 SoC has 2 DCAN modules. Add compatible id and
> raminit driver data for am3352 DCAN.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>

Acked-by: Wolfram Sang <wsa@the-dreams.de>


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 8/8] net: can: c_can: Add support for TI am3352 DCAN

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 639 bytes --]

On 11/13/2014 05:06 PM, Wolfram Sang wrote:
> On Fri, Nov 07, 2014 at 04:49:22PM +0200, Roger Quadros wrote:
>> AM3352 SoC has 2 DCAN modules. Add compatible id and
>> raminit driver data for am3352 DCAN.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> 
> Acked-by: Wolfram Sang <wsa@the-dreams.de>

For the whole series or just this patch?

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 8/8] net: can: c_can: Add support for TI am3352 DCAN

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 464 bytes --]

On Fri, Nov 14, 2014 at 10:25:55AM +0100, Marc Kleine-Budde wrote:
> On 11/13/2014 05:06 PM, Wolfram Sang wrote:
> > On Fri, Nov 07, 2014 at 04:49:22PM +0200, Roger Quadros wrote:
> >> AM3352 SoC has 2 DCAN modules. Add compatible id and
> >> raminit driver data for am3352 DCAN.
> >>
> >> Signed-off-by: Roger Quadros <rogerq@ti.com>
> > 
> > Acked-by: Wolfram Sang <wsa@the-dreams.de>
> 
> For the whole series or just this patch?

Just this one.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 6/8] net: can: c_can: Disable pins when CAN interface is down

[Roger Quadros]

On 11/13/2014 06:03 PM, Marc Kleine-Budde wrote:
> On 11/13/2014 04:23 PM, Roger Quadros wrote:
>> DRA7 CAN IP suffers from a problem which causes it to be prevented
>> from fully turning OFF (i.e. stuck in transition) if the module was
>> disabled while there was traffic on the CAN_RX line.
>>
>> To work around this issue we select the SLEEP pin state by default
>> on probe and use the DEFAULT pin state on CAN up and back to the
>> SLEEP pin state on CAN down.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>  drivers/net/can/c_can/c_can.c | 37 +++++++++++++++++++++++++++++++++++++
>>  drivers/net/can/c_can/c_can.h |  1 +
>>  2 files changed, 38 insertions(+)
>>
>> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
>> index 8e78bb4..c80cb3d 100644
>> --- a/drivers/net/can/c_can/c_can.c
>> +++ b/drivers/net/can/c_can/c_can.c
>> @@ -35,6 +35,7 @@
>>  #include <linux/list.h>
>>  #include <linux/io.h>
>>  #include <linux/pm_runtime.h>
>> +#include <linux/pinctrl/consumer.h>
>>  
>>  #include <linux/can.h>
>>  #include <linux/can/dev.h>
>> @@ -587,6 +588,21 @@ static int c_can_chip_config(struct net_device *dev)
>>  	return c_can_set_bittiming(dev);
>>  }
>>  
>> +/*
>> + * Selects the pinctrl state specified in the name.
>> + */
>> +static void c_can_pinctrl_select_state(struct c_can_priv *priv,
>> +				      const char *name)
>> +{
>> +	if (!IS_ERR(priv->pinctrl)) {
>> +		struct pinctrl_state *s;
>> +
>> +		s = pinctrl_lookup_state(priv->pinctrl, name);
>> +		if (!IS_ERR(s))
>> +			pinctrl_select_state(priv->pinctrl, s);
>> +	}
>> +}
>> +
>>  static int c_can_start(struct net_device *dev)
>>  {
>>  	struct c_can_priv *priv = netdev_priv(dev);
>> @@ -603,6 +619,8 @@ static int c_can_start(struct net_device *dev)
>>  
>>  	priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>  
>> +	/* activate pins */
>> +	c_can_pinctrl_select_state(priv, PINCTRL_STATE_DEFAULT);
>>  	return 0;
>>  }
>>  
>> @@ -611,6 +629,9 @@ static void c_can_stop(struct net_device *dev)
>>  	struct c_can_priv *priv = netdev_priv(dev);
>>  
>>  	c_can_irq_control(priv, false);
>> +
>> +	/* deactivate pins */
>> +	c_can_pinctrl_select_state(priv, PINCTRL_STATE_SLEEP);
>>  	priv->can.state = CAN_STATE_STOPPED;
>>  }
>>  
>> @@ -1244,6 +1265,22 @@ int register_c_can_dev(struct net_device *dev)
>>  	struct c_can_priv *priv = netdev_priv(dev);
>>  	int err;
>>  
>> +	priv->pinctrl = devm_pinctrl_get(dev->dev.parent);
> 
> What's dev->dev.parent?
> Ah, this is set by SET_NETDEV_DEV(dev, &pdev->dev); Good work!
> 
> I thought we had to set priv->pinctrl in the platform.c.
> 
>> +	if (!IS_ERR(priv->pinctrl)) {
>> +		struct pinctrl_state *s;
>> +
>> +		/* Deactivate pins to prevent DRA7 DCAN IP from being
>> +		 * stuck in transition when module is disabled.
>> +		 * Pins are activated in c_can_start() and deactivated
>> +		 * in c_can_stop()
>> +		 */
>> +		s = pinctrl_lookup_state(priv->pinctrl, PINCTRL_STATE_SLEEP);
>> +		if (!IS_ERR(s))
>> +			pinctrl_select_state(priv->pinctrl, s);
>> +	} else {
>> +		netdev_dbg(dev, "failed to get pinctrl\n");
>> +	}
>> +
> The above can be replace by this?
> 
> 	c_can_pinctrl_select_state(priv, PINCTRL_STATE_SLEEP)

Yes, indeed.

> 
> Then we don't have the worrying looking error message if there isn't any
> pinctrl, e.g. for the PCI driver with pinctrl enabled in the Kernel.
> 
> I just stumbled over pinctrl_pm_select_sleep_state(), is it possible to
> integrate this into runtime pm?
> 
> http://lxr.free-electrons.com/source/drivers/pinctrl/core.c#L1282

I think those functions are there for the same reason but not sure why aren't 
they used in runtime pm core.

Linus W. any hints?

cheers,
-roger

> 
>>  	c_can_pm_runtime_enable(priv);
>>  
>>  	dev->flags |= IFF_ECHO;	/* we support local echo */
>> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
>> index c6715ca..3cedf48 100644
>> --- a/drivers/net/can/c_can/c_can.h
>> +++ b/drivers/net/can/c_can/c_can.h
>> @@ -210,6 +210,7 @@ struct c_can_priv {
>>  	u32 comm_rcv_high;
>>  	u32 rxmasked;
>>  	u32 dlc[C_CAN_MSG_OBJ_TX_NUM];
>> +	struct pinctrl *pinctrl;
>>  };
>>  
>>  struct net_device *alloc_c_can_dev(void);
>>
> 
> Marc
> 

[PATCH v5 4/8] net: can: c_can: Add syscon/regmap RAMINIT mechanism

[Roger Quadros]

Some TI SoCs like DRA7 have a RAMINIT register specification
different from the other AMxx SoCs and as expected by the
existing driver.

To add more insanity, this register is shared with other
IPs like DSS, PCIe and PWM.

Provides a more generic mechanism to specify the RAMINIT
register location and START/DONE bit position and use the
syscon/regmap framework to access the register.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 .../devicetree/bindings/net/can/c_can.txt          |   3 +
 drivers/net/can/c_can/c_can.h                      |  11 +-
 drivers/net/can/c_can/c_can_platform.c             | 113 ++++++++++++++-------
 3 files changed, 87 insertions(+), 40 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/can/c_can.txt b/Documentation/devicetree/bindings/net/can/c_can.txt
index 8f1ae81..a3ca3ee 100644
--- a/Documentation/devicetree/bindings/net/can/c_can.txt
+++ b/Documentation/devicetree/bindings/net/can/c_can.txt
@@ -12,6 +12,9 @@ Required properties:
 Optional properties:
 - ti,hwmods		: Must be "d_can<n>" or "c_can<n>", n being the
 			  instance number
+- syscon-raminit	: Handle to system control region that contains the
+			  RAMINIT register, register offset to the RAMINIT
+			  register and the CAN instance number (0 offset).
 
 Note: "ti,hwmods" field is used to fetch the base address and irq
 resources from TI, omap hwmod data base during device registration.
diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index 3c305a1..0e17c7b 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -179,6 +179,14 @@ struct c_can_driver_data {
 	bool raminit_pulse;	/* If set, sets and clears START bit (pulse) */
 };
 
+/* Out of band RAMINIT register access via syscon regmap */
+struct c_can_raminit {
+	struct regmap *syscon;	/* for raminit ctrl. reg. access */
+	unsigned int reg;	/* register index within syscon */
+	u8 start_bit;
+	u8 done_bit;
+};
+
 /* c_can private data structure */
 struct c_can_priv {
 	struct can_priv can;	/* must be the first member */
@@ -196,8 +204,7 @@ struct c_can_priv {
 	const u16 *regs;
 	void *priv;		/* for board-specific data */
 	enum c_can_dev_id type;
-	u32 __iomem *raminit_ctrlreg;
-	int instance;
+	struct c_can_raminit raminit_sys;	/* RAMINIT via syscon regmap */
 	void (*raminit) (const struct c_can_priv *priv, bool enable);
 	u32 comm_rcv_high;
 	u32 rxmasked;
diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
index 1546c2b..89739a1 100644
--- a/drivers/net/can/c_can/c_can_platform.c
+++ b/drivers/net/can/c_can/c_can_platform.c
@@ -32,14 +32,13 @@
 #include <linux/clk.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
 
 #include <linux/can/dev.h>
 
 #include "c_can.h"
 
-#define CAN_RAMINIT_START_MASK(i)	(0x001 << (i))
-#define CAN_RAMINIT_DONE_MASK(i)	(0x100 << (i))
-#define CAN_RAMINIT_ALL_MASK(i)		(0x101 << (i))
 #define DCAN_RAM_INIT_BIT		(1 << 3)
 static DEFINE_SPINLOCK(raminit_lock);
 /*
@@ -72,47 +71,61 @@ static void c_can_plat_write_reg_aligned_to_32bit(const struct c_can_priv *priv,
 	writew(val, priv->base + 2 * priv->regs[index]);
 }
 
-static void c_can_hw_raminit_wait_ti(const struct c_can_priv *priv, u32 mask,
-				  u32 val)
+static void c_can_hw_raminit_wait_syscon(const struct c_can_priv *priv,
+					 u32 mask, u32 val)
 {
 	int timeout = 0;
+	const struct c_can_raminit *raminit = &priv->raminit_sys;
+	u32 ctrl;
+
 	/* We look only at the bits of our instance. */
 	val &= mask;
-	while ((readl(priv->raminit_ctrlreg) & mask) != val) {
+	do {
 		udelay(1);
 		timeout++;
 
+		regmap_read(raminit->syscon, raminit->reg, &ctrl);
 		if (timeout == 1000) {
 			dev_err(&priv->dev->dev, "%s: time out\n", __func__);
 			break;
 		}
-	}
+	} while ((ctrl & mask) != val);
 }
 
-static void c_can_hw_raminit_ti(const struct c_can_priv *priv, bool enable)
+static void c_can_hw_raminit_syscon(const struct c_can_priv *priv, bool enable)
 {
-	u32 mask = CAN_RAMINIT_ALL_MASK(priv->instance);
+	u32 mask;
 	u32 ctrl;
+	const struct c_can_raminit *raminit = &priv->raminit_sys;
+	u8 start_bit, done_bit;
+
+	start_bit = raminit->start_bit;
+	done_bit = raminit->done_bit;
 
 	spin_lock(&raminit_lock);
 
-	ctrl = readl(priv->raminit_ctrlreg);
+	mask = 1 << start_bit | 1 << done_bit;
+	regmap_read(raminit->syscon, raminit->reg, &ctrl);
+
 	/* We clear the done and start bit first. The start bit is
 	 * looking at the 0 -> transition, but is not self clearing;
 	 * And we clear the init done bit as well.
+	 * NOTE: DONE must be written with 1 to clear it.
 	 */
-	ctrl &= ~CAN_RAMINIT_START_MASK(priv->instance);
-	ctrl |= CAN_RAMINIT_DONE_MASK(priv->instance);
-	writel(ctrl, priv->raminit_ctrlreg);
-	ctrl &= ~CAN_RAMINIT_DONE_MASK(priv->instance);
-	c_can_hw_raminit_wait_ti(priv, mask, ctrl);
+	ctrl &= ~(1 << start_bit);
+	ctrl |= 1 << done_bit;
+	regmap_write(raminit->syscon, raminit->reg, ctrl);
+
+	ctrl &= ~(1 << done_bit);
+	c_can_hw_raminit_wait_syscon(priv, mask, ctrl);
 
 	if (enable) {
 		/* Set start bit and wait for the done bit. */
-		ctrl |= CAN_RAMINIT_START_MASK(priv->instance);
-		writel(ctrl, priv->raminit_ctrlreg);
-		ctrl |= CAN_RAMINIT_DONE_MASK(priv->instance);
-		c_can_hw_raminit_wait_ti(priv, mask, ctrl);
+		ctrl |= 1 << start_bit;
+		regmap_write(raminit->syscon, raminit->reg, ctrl);
+
+		ctrl |= 1 << done_bit;
+		c_can_hw_raminit_wait_syscon(priv, mask, ctrl);
 	}
 	spin_unlock(&raminit_lock);
 }
@@ -206,10 +219,11 @@ static int c_can_plat_probe(struct platform_device *pdev)
 	struct net_device *dev;
 	struct c_can_priv *priv;
 	const struct of_device_id *match;
-	struct resource *mem, *res;
+	struct resource *mem;
 	int irq;
 	struct clk *clk;
 	const struct c_can_driver_data *drvdata;
+	struct device_node *np = pdev->dev.of_node;
 
 	match = of_match_device(c_can_of_table, &pdev->dev);
 	if (match) {
@@ -277,27 +291,50 @@ static int c_can_plat_probe(struct platform_device *pdev)
 		priv->read_reg32 = d_can_plat_read_reg32;
 		priv->write_reg32 = d_can_plat_write_reg32;
 
-		if (pdev->dev.of_node)
-			priv->instance = of_alias_get_id(pdev->dev.of_node, "d_can");
-		else
-			priv->instance = pdev->id;
-
-		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-		/* Not all D_CAN modules have a separate register for the D_CAN
-		 * RAM initialization. Use default RAM init bit in D_CAN module
-		 * if not specified in DT.
+		/* Check if we need custom RAMINIT via syscon. Mostly for TI
+		 * platforms. Only supported with DT boot.
 		 */
-		if (!res) {
+		if (np && of_property_read_bool(np, "syscon-raminit")) {
+			u32 id;
+			struct c_can_raminit *raminit = &priv->raminit_sys;
+
+			ret = -EINVAL;
+			raminit->syscon = syscon_regmap_lookup_by_phandle(np,
+									  "syscon-raminit");
+			if (IS_ERR(raminit->syscon)) {
+				/* can fail with -EPROBE_DEFER */
+				ret = PTR_ERR(raminit->syscon);
+				free_c_can_dev(dev);
+				return ret;
+			}
+
+			if (of_property_read_u32_index(np, "syscon-raminit", 1,
+						       &raminit->reg)) {
+				dev_err(&pdev->dev,
+					"couldn't get the RAMINIT reg. offset!\n");
+				goto exit_free_device;
+			}
+
+			if (of_property_read_u32_index(np, "syscon-raminit", 2,
+						       &id)) {
+				dev_err(&pdev->dev,
+					"couldn't get the CAN instance ID\n");
+				goto exit_free_device;
+			}
+
+			if (id >= drvdata->num_can) {
+				dev_err(&pdev->dev,
+					"Invalid CAN instance ID\n");
+				goto exit_free_device;
+			}
+
+			raminit->start_bit = drvdata->raminit_start_bits[id];
+			raminit->done_bit = drvdata->raminit_done_bits[id];
+
+			priv->raminit = c_can_hw_raminit_syscon;
+		} else {
 			priv->raminit = c_can_hw_raminit;
-			break;
 		}
-
-		priv->raminit_ctrlreg = devm_ioremap(&pdev->dev, res->start,
-						     resource_size(res));
-		if (!priv->raminit_ctrlreg || priv->instance < 0)
-			dev_info(&pdev->dev, "control memory is not used for raminit\n");
-		else
-			priv->raminit = c_can_hw_raminit_ti;
 		break;
 	default:
 		ret = -EINVAL;
-- 
1.8.3.2

[PATCH v7 6/8] net: can: c_can: Disable pins when CAN interface is down

[Roger Quadros]

DRA7 CAN IP suffers from a problem which causes it to be prevented
from fully turning OFF (i.e. stuck in transition) if the module was
disabled while there was traffic on the CAN_RX line.

To work around this issue we select the SLEEP pin state by default
on probe and use the DEFAULT pin state on CAN up and back to the
SLEEP pin state on CAN down.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/net/can/c_can/c_can.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 8e78bb4..f94a9fa 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -35,6 +35,7 @@
 #include <linux/list.h>
 #include <linux/io.h>
 #include <linux/pm_runtime.h>
+#include <linux/pinctrl/consumer.h>
 
 #include <linux/can.h>
 #include <linux/can/dev.h>
@@ -603,6 +604,8 @@ static int c_can_start(struct net_device *dev)
 
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
+	/* activate pins */
+	pinctrl_pm_select_default_state(dev->dev.parent);
 	return 0;
 }
 
@@ -611,6 +614,9 @@ static void c_can_stop(struct net_device *dev)
 	struct c_can_priv *priv = netdev_priv(dev);
 
 	c_can_irq_control(priv, false);
+
+	/* deactivate pins */
+	pinctrl_pm_select_sleep_state(dev->dev.parent);
 	priv->can.state = CAN_STATE_STOPPED;
 }
 
@@ -1244,6 +1250,13 @@ int register_c_can_dev(struct net_device *dev)
 	struct c_can_priv *priv = netdev_priv(dev);
 	int err;
 
+	/* Deactivate pins to prevent DRA7 DCAN IP from being
+	 * stuck in transition when module is disabled.
+	 * Pins are activated in c_can_start() and deactivated
+	 * in c_can_stop()
+	 */
+	pinctrl_pm_select_sleep_state(dev->dev.parent);
+
 	c_can_pm_runtime_enable(priv);
 
 	dev->flags |= IFF_ECHO;	/* we support local echo */
-- 
1.8.3.2

Re: [PATCH v7 6/8] net: can: c_can: Disable pins when CAN interface is down

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 866 bytes --]

On 11/14/2014 04:40 PM, Roger Quadros wrote:
> DRA7 CAN IP suffers from a problem which causes it to be prevented
> from fully turning OFF (i.e. stuck in transition) if the module was
> disabled while there was traffic on the CAN_RX line.
> 
> To work around this issue we select the SLEEP pin state by default
> on probe and use the DEFAULT pin state on CAN up and back to the
> SLEEP pin state on CAN down.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>

Wow! Some interations later this patch got quite nice and shiny :)
Applied all to can-next/master.

Thanks,
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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 7/8] net: can: c_can: Add support for TI DRA7 DCAN

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2131 bytes --]

On 11/07/2014 03:49 PM, Roger Quadros wrote:
> DRA7 SoC has 2 CAN IPs. Provide compatible IDs and RAMINIT
> register data for both.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  Documentation/devicetree/bindings/net/can/c_can.txt |  1 +
>  drivers/net/can/c_can/c_can_platform.c              | 11 +++++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/c_can.txt b/Documentation/devicetree/bindings/net/can/c_can.txt
> index a3ca3ee..f682fdb 100644
> --- a/Documentation/devicetree/bindings/net/can/c_can.txt
> +++ b/Documentation/devicetree/bindings/net/can/c_can.txt
> @@ -4,6 +4,7 @@ Bosch C_CAN/D_CAN controller Device Tree Bindings
>  Required properties:
>  - compatible		: Should be "bosch,c_can" for C_CAN controllers and
>  			  "bosch,d_can" for D_CAN controllers.
> +			  Can be "ti,dra7-d_can".
>  - reg			: physical base address and size of the C_CAN/D_CAN
>  			  registers map
>  - interrupts		: property with a value describing the interrupt
> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
> index 71b9063..7a81db4 100644
> --- a/drivers/net/can/c_can/c_can_platform.c
> +++ b/drivers/net/can/c_can/c_can_platform.c
> @@ -195,6 +195,16 @@ static struct c_can_driver_data d_can_drvdata = {
>  	.id = BOSCH_D_CAN,
>  };
>  
> +static u8 dra7_raminit_start_bits[] = {3, 5};
> +static u8 dra7_raminit_done_bits[] = {1, 2};
> +static struct c_can_driver_data dra7_dcan_drvdata = {
> +	.id = BOSCH_D_CAN,
> +	.num_can = 2,
                   ^
Replaced by ARRAY_SIZE(dra7_raminit_start_bits)

Same for the am3352_dcan_drvdata in the next patch.

> +	.raminit_start_bits = dra7_raminit_start_bits,
> +	.raminit_done_bits = dra7_raminit_done_bits,
> +	.raminit_pulse = true,
> +};

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v7 6/8] net: can: c_can: Disable pins when CAN interface is down

[Roger Quadros]

On 11/14/2014 05:49 PM, Marc Kleine-Budde wrote:
> On 11/14/2014 04:40 PM, Roger Quadros wrote:
>> DRA7 CAN IP suffers from a problem which causes it to be prevented
>> from fully turning OFF (i.e. stuck in transition) if the module was
>> disabled while there was traffic on the CAN_RX line.
>>
>> To work around this issue we select the SLEEP pin state by default
>> on probe and use the DEFAULT pin state on CAN up and back to the
>> SLEEP pin state on CAN down.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> 
> Wow! Some interations later this patch got quite nice and shiny :)
> Applied all to can-next/master.

Thanks Marc :)

cheers,
-roger

Re: [PATCH v5 4/8] net: can: c_can: Add syscon/regmap RAMINIT mechanism

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 6825 bytes --]

On 11/14/2014 04:37 PM, Roger Quadros wrote:
> Some TI SoCs like DRA7 have a RAMINIT register specification
> different from the other AMxx SoCs and as expected by the
> existing driver.
> 
> To add more insanity, this register is shared with other
> IPs like DSS, PCIe and PWM.
> 
> Provides a more generic mechanism to specify the RAMINIT
> register location and START/DONE bit position and use the
> syscon/regmap framework to access the register.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  .../devicetree/bindings/net/can/c_can.txt          |   3 +
>  drivers/net/can/c_can/c_can.h                      |  11 +-
>  drivers/net/can/c_can/c_can_platform.c             | 113 ++++++++++++++-------
>  3 files changed, 87 insertions(+), 40 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/c_can.txt b/Documentation/devicetree/bindings/net/can/c_can.txt
> index 8f1ae81..a3ca3ee 100644
> --- a/Documentation/devicetree/bindings/net/can/c_can.txt
> +++ b/Documentation/devicetree/bindings/net/can/c_can.txt
> @@ -12,6 +12,9 @@ Required properties:
>  Optional properties:
>  - ti,hwmods		: Must be "d_can<n>" or "c_can<n>", n being the
>  			  instance number
> +- syscon-raminit	: Handle to system control region that contains the
> +			  RAMINIT register, register offset to the RAMINIT
> +			  register and the CAN instance number (0 offset).
>  
>  Note: "ti,hwmods" field is used to fetch the base address and irq
>  resources from TI, omap hwmod data base during device registration.
> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> index 3c305a1..0e17c7b 100644
> --- a/drivers/net/can/c_can/c_can.h
> +++ b/drivers/net/can/c_can/c_can.h
> @@ -179,6 +179,14 @@ struct c_can_driver_data {
>  	bool raminit_pulse;	/* If set, sets and clears START bit (pulse) */
>  };
>  
> +/* Out of band RAMINIT register access via syscon regmap */
> +struct c_can_raminit {
> +	struct regmap *syscon;	/* for raminit ctrl. reg. access */
> +	unsigned int reg;	/* register index within syscon */
> +	u8 start_bit;
> +	u8 done_bit;
> +};
> +
>  /* c_can private data structure */
>  struct c_can_priv {
>  	struct can_priv can;	/* must be the first member */
> @@ -196,8 +204,7 @@ struct c_can_priv {
>  	const u16 *regs;
>  	void *priv;		/* for board-specific data */
>  	enum c_can_dev_id type;
> -	u32 __iomem *raminit_ctrlreg;
> -	int instance;
> +	struct c_can_raminit raminit_sys;	/* RAMINIT via syscon regmap */
>  	void (*raminit) (const struct c_can_priv *priv, bool enable);
>  	u32 comm_rcv_high;
>  	u32 rxmasked;
> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
> index 1546c2b..89739a1 100644
> --- a/drivers/net/can/c_can/c_can_platform.c
> +++ b/drivers/net/can/c_can/c_can_platform.c
> @@ -32,14 +32,13 @@
>  #include <linux/clk.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
>  
>  #include <linux/can/dev.h>
>  
>  #include "c_can.h"
>  
> -#define CAN_RAMINIT_START_MASK(i)	(0x001 << (i))
> -#define CAN_RAMINIT_DONE_MASK(i)	(0x100 << (i))
> -#define CAN_RAMINIT_ALL_MASK(i)		(0x101 << (i))
>  #define DCAN_RAM_INIT_BIT		(1 << 3)
>  static DEFINE_SPINLOCK(raminit_lock);
>  /*
> @@ -72,47 +71,61 @@ static void c_can_plat_write_reg_aligned_to_32bit(const struct c_can_priv *priv,
>  	writew(val, priv->base + 2 * priv->regs[index]);
>  }
>  
> -static void c_can_hw_raminit_wait_ti(const struct c_can_priv *priv, u32 mask,
> -				  u32 val)
> +static void c_can_hw_raminit_wait_syscon(const struct c_can_priv *priv,
> +					 u32 mask, u32 val)
>  {
>  	int timeout = 0;
> +	const struct c_can_raminit *raminit = &priv->raminit_sys;
> +	u32 ctrl;
> +
>  	/* We look only at the bits of our instance. */
>  	val &= mask;
> -	while ((readl(priv->raminit_ctrlreg) & mask) != val) {
> +	do {
>  		udelay(1);
>  		timeout++;
>  
> +		regmap_read(raminit->syscon, raminit->reg, &ctrl);
>  		if (timeout == 1000) {
>  			dev_err(&priv->dev->dev, "%s: time out\n", __func__);
>  			break;
>  		}
> -	}
> +	} while ((ctrl & mask) != val);
>  }
>  
> -static void c_can_hw_raminit_ti(const struct c_can_priv *priv, bool enable)
> +static void c_can_hw_raminit_syscon(const struct c_can_priv *priv, bool enable)
>  {
> -	u32 mask = CAN_RAMINIT_ALL_MASK(priv->instance);
> +	u32 mask;
>  	u32 ctrl;
> +	const struct c_can_raminit *raminit = &priv->raminit_sys;
> +	u8 start_bit, done_bit;
> +
> +	start_bit = raminit->start_bit;
> +	done_bit = raminit->done_bit;
>  
>  	spin_lock(&raminit_lock);
>  
> -	ctrl = readl(priv->raminit_ctrlreg);
> +	mask = 1 << start_bit | 1 << done_bit;
> +	regmap_read(raminit->syscon, raminit->reg, &ctrl);
> +
>  	/* We clear the done and start bit first. The start bit is
>  	 * looking at the 0 -> transition, but is not self clearing;
>  	 * And we clear the init done bit as well.
> +	 * NOTE: DONE must be written with 1 to clear it.
>  	 */
> -	ctrl &= ~CAN_RAMINIT_START_MASK(priv->instance);
> -	ctrl |= CAN_RAMINIT_DONE_MASK(priv->instance);
> -	writel(ctrl, priv->raminit_ctrlreg);
> -	ctrl &= ~CAN_RAMINIT_DONE_MASK(priv->instance);
> -	c_can_hw_raminit_wait_ti(priv, mask, ctrl);
> +	ctrl &= ~(1 << start_bit);
> +	ctrl |= 1 << done_bit;
> +	regmap_write(raminit->syscon, raminit->reg, ctrl);
> +
> +	ctrl &= ~(1 << done_bit);
> +	c_can_hw_raminit_wait_syscon(priv, mask, ctrl);
>  
>  	if (enable) {
>  		/* Set start bit and wait for the done bit. */
> -		ctrl |= CAN_RAMINIT_START_MASK(priv->instance);
> -		writel(ctrl, priv->raminit_ctrlreg);
> -		ctrl |= CAN_RAMINIT_DONE_MASK(priv->instance);
> -		c_can_hw_raminit_wait_ti(priv, mask, ctrl);
> +		ctrl |= 1 << start_bit;
> +		regmap_write(raminit->syscon, raminit->reg, ctrl);
> +
> +		ctrl |= 1 << done_bit;
> +		c_can_hw_raminit_wait_syscon(priv, mask, ctrl);
>  	}
>  	spin_unlock(&raminit_lock);
>  }

My arm gcc-4.7.2 spits this warnings, I'll initialize ctrl to 0.

> drivers/net/can/c_can/c_can_platform.c: In function 'c_can_hw_raminit_wait_syscon':
> drivers/net/can/c_can/c_can_platform.c:92:17: warning: 'ctrl' may be used uninitialized in this function [-Wuninitialized]
> drivers/net/can/c_can/c_can_platform.c: In function 'c_can_hw_raminit_syscon':
> drivers/net/can/c_can/c_can_platform.c:115:7: warning: 'ctrl' is used uninitialized in this function [-Wuninitialized]

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 4/8] net: can: c_can: Add syscon/regmap RAMINIT mechanism

[Roger Quadros]

On 11/14/2014 06:32 PM, Marc Kleine-Budde wrote:
> On 11/14/2014 04:37 PM, Roger Quadros wrote:
>> Some TI SoCs like DRA7 have a RAMINIT register specification
>> different from the other AMxx SoCs and as expected by the
>> existing driver.
>>
>> To add more insanity, this register is shared with other
>> IPs like DSS, PCIe and PWM.
>>
>> Provides a more generic mechanism to specify the RAMINIT
>> register location and START/DONE bit position and use the
>> syscon/regmap framework to access the register.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>  .../devicetree/bindings/net/can/c_can.txt          |   3 +
>>  drivers/net/can/c_can/c_can.h                      |  11 +-
>>  drivers/net/can/c_can/c_can_platform.c             | 113 ++++++++++++++-------
>>  3 files changed, 87 insertions(+), 40 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/can/c_can.txt b/Documentation/devicetree/bindings/net/can/c_can.txt
>> index 8f1ae81..a3ca3ee 100644
>> --- a/Documentation/devicetree/bindings/net/can/c_can.txt
>> +++ b/Documentation/devicetree/bindings/net/can/c_can.txt
>> @@ -12,6 +12,9 @@ Required properties:
>>  Optional properties:
>>  - ti,hwmods		: Must be "d_can<n>" or "c_can<n>", n being the
>>  			  instance number
>> +- syscon-raminit	: Handle to system control region that contains the
>> +			  RAMINIT register, register offset to the RAMINIT
>> +			  register and the CAN instance number (0 offset).
>>  
>>  Note: "ti,hwmods" field is used to fetch the base address and irq
>>  resources from TI, omap hwmod data base during device registration.
>> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
>> index 3c305a1..0e17c7b 100644
>> --- a/drivers/net/can/c_can/c_can.h
>> +++ b/drivers/net/can/c_can/c_can.h
>> @@ -179,6 +179,14 @@ struct c_can_driver_data {
>>  	bool raminit_pulse;	/* If set, sets and clears START bit (pulse) */
>>  };
>>  
>> +/* Out of band RAMINIT register access via syscon regmap */
>> +struct c_can_raminit {
>> +	struct regmap *syscon;	/* for raminit ctrl. reg. access */
>> +	unsigned int reg;	/* register index within syscon */
>> +	u8 start_bit;
>> +	u8 done_bit;
>> +};
>> +
>>  /* c_can private data structure */
>>  struct c_can_priv {
>>  	struct can_priv can;	/* must be the first member */
>> @@ -196,8 +204,7 @@ struct c_can_priv {
>>  	const u16 *regs;
>>  	void *priv;		/* for board-specific data */
>>  	enum c_can_dev_id type;
>> -	u32 __iomem *raminit_ctrlreg;
>> -	int instance;
>> +	struct c_can_raminit raminit_sys;	/* RAMINIT via syscon regmap */
>>  	void (*raminit) (const struct c_can_priv *priv, bool enable);
>>  	u32 comm_rcv_high;
>>  	u32 rxmasked;
>> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
>> index 1546c2b..89739a1 100644
>> --- a/drivers/net/can/c_can/c_can_platform.c
>> +++ b/drivers/net/can/c_can/c_can_platform.c
>> @@ -32,14 +32,13 @@
>>  #include <linux/clk.h>
>>  #include <linux/of.h>
>>  #include <linux/of_device.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/regmap.h>
>>  
>>  #include <linux/can/dev.h>
>>  
>>  #include "c_can.h"
>>  
>> -#define CAN_RAMINIT_START_MASK(i)	(0x001 << (i))
>> -#define CAN_RAMINIT_DONE_MASK(i)	(0x100 << (i))
>> -#define CAN_RAMINIT_ALL_MASK(i)		(0x101 << (i))
>>  #define DCAN_RAM_INIT_BIT		(1 << 3)
>>  static DEFINE_SPINLOCK(raminit_lock);
>>  /*
>> @@ -72,47 +71,61 @@ static void c_can_plat_write_reg_aligned_to_32bit(const struct c_can_priv *priv,
>>  	writew(val, priv->base + 2 * priv->regs[index]);
>>  }
>>  
>> -static void c_can_hw_raminit_wait_ti(const struct c_can_priv *priv, u32 mask,
>> -				  u32 val)
>> +static void c_can_hw_raminit_wait_syscon(const struct c_can_priv *priv,
>> +					 u32 mask, u32 val)
>>  {
>>  	int timeout = 0;
>> +	const struct c_can_raminit *raminit = &priv->raminit_sys;
>> +	u32 ctrl;
>> +
>>  	/* We look only at the bits of our instance. */
>>  	val &= mask;
>> -	while ((readl(priv->raminit_ctrlreg) & mask) != val) {
>> +	do {
>>  		udelay(1);
>>  		timeout++;
>>  
>> +		regmap_read(raminit->syscon, raminit->reg, &ctrl);
>>  		if (timeout == 1000) {
>>  			dev_err(&priv->dev->dev, "%s: time out\n", __func__);
>>  			break;
>>  		}
>> -	}
>> +	} while ((ctrl & mask) != val);
>>  }
>>  
>> -static void c_can_hw_raminit_ti(const struct c_can_priv *priv, bool enable)
>> +static void c_can_hw_raminit_syscon(const struct c_can_priv *priv, bool enable)
>>  {
>> -	u32 mask = CAN_RAMINIT_ALL_MASK(priv->instance);
>> +	u32 mask;
>>  	u32 ctrl;
>> +	const struct c_can_raminit *raminit = &priv->raminit_sys;
>> +	u8 start_bit, done_bit;
>> +
>> +	start_bit = raminit->start_bit;
>> +	done_bit = raminit->done_bit;
>>  
>>  	spin_lock(&raminit_lock);
>>  
>> -	ctrl = readl(priv->raminit_ctrlreg);
>> +	mask = 1 << start_bit | 1 << done_bit;
>> +	regmap_read(raminit->syscon, raminit->reg, &ctrl);
>> +
>>  	/* We clear the done and start bit first. The start bit is
>>  	 * looking at the 0 -> transition, but is not self clearing;
>>  	 * And we clear the init done bit as well.
>> +	 * NOTE: DONE must be written with 1 to clear it.
>>  	 */
>> -	ctrl &= ~CAN_RAMINIT_START_MASK(priv->instance);
>> -	ctrl |= CAN_RAMINIT_DONE_MASK(priv->instance);
>> -	writel(ctrl, priv->raminit_ctrlreg);
>> -	ctrl &= ~CAN_RAMINIT_DONE_MASK(priv->instance);
>> -	c_can_hw_raminit_wait_ti(priv, mask, ctrl);
>> +	ctrl &= ~(1 << start_bit);
>> +	ctrl |= 1 << done_bit;
>> +	regmap_write(raminit->syscon, raminit->reg, ctrl);
>> +
>> +	ctrl &= ~(1 << done_bit);
>> +	c_can_hw_raminit_wait_syscon(priv, mask, ctrl);
>>  
>>  	if (enable) {
>>  		/* Set start bit and wait for the done bit. */
>> -		ctrl |= CAN_RAMINIT_START_MASK(priv->instance);
>> -		writel(ctrl, priv->raminit_ctrlreg);
>> -		ctrl |= CAN_RAMINIT_DONE_MASK(priv->instance);
>> -		c_can_hw_raminit_wait_ti(priv, mask, ctrl);
>> +		ctrl |= 1 << start_bit;
>> +		regmap_write(raminit->syscon, raminit->reg, ctrl);
>> +
>> +		ctrl |= 1 << done_bit;
>> +		c_can_hw_raminit_wait_syscon(priv, mask, ctrl);
>>  	}
>>  	spin_unlock(&raminit_lock);
>>  }
> 
> My arm gcc-4.7.2 spits this warnings, I'll initialize ctrl to 0.

My 4.7.3 doesn't. Initializing to 0 is fine as well.

cheers,
-roger

> 
>> drivers/net/can/c_can/c_can_platform.c: In function 'c_can_hw_raminit_wait_syscon':
>> drivers/net/can/c_can/c_can_platform.c:92:17: warning: 'ctrl' may be used uninitialized in this function [-Wuninitialized]
>> drivers/net/can/c_can/c_can_platform.c: In function 'c_can_hw_raminit_syscon':
>> drivers/net/can/c_can/c_can_platform.c:115:7: warning: 'ctrl' is used uninitialized in this function [-Wuninitialized]
> 
> Marc
> 

Re: [PATCH v4 3/8] net: can: c_can: Add RAMINIT register information to driver data

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2146 bytes --]

On 11/07/2014 03:49 PM, Roger Quadros wrote:
> Some platforms (e.g. TI) need special RAMINIT register handling.
> Provide a way to store RAMINIT register description in driver data.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/net/can/c_can/c_can.h          | 6 ++++++
>  drivers/net/can/c_can/c_can_platform.c | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> index 26c975d..3c305a1 100644
> --- a/drivers/net/can/c_can/c_can.h
> +++ b/drivers/net/can/c_can/c_can.h
> @@ -171,6 +171,12 @@ enum c_can_dev_id {
>  
>  struct c_can_driver_data {
>  	enum c_can_dev_id id;
> +
> +	/* RAMINIT register description. Optional. */
> +	u8 num_can;		/* Number of CAN instances on the SoC */
> +	u8 *raminit_start_bits;	/* Array of START bit positions */
> +	u8 *raminit_done_bits;	/* Array of DONE bit positions */

What do you think about making this a struct:

+struct raminit_bits {
+       u8 start;
+       u8 done;
+};

 struct c_can_driver_data {
        enum c_can_dev_id id;
+
+       /* RAMINIT register description. Optional. */
+       const struct raminit_bits *raminit_bits; /* Array of START/DONE bit positions */
+       u8 raminit_num;         /* Number of CAN instances on the SoC */
+       bool raminit_pulse;     /* If set, sets and clears START bit (pulse) */
 };

The driver data looks like this:

+static const struct raminit_bits dra7_raminit_bits[] = {
+       [0] = { .start = 3, .done = 1, },
+       [1] = { .start = 5, .done = 2, },
+};
+
+static const struct c_can_driver_data dra7_dcan_drvdata = {
+       .id = BOSCH_D_CAN,
+       .raminit_num = ARRAY_SIZE(dra7_raminit_bits),
+       .raminit_bits = dra7_raminit_bits,
+       .raminit_pulse = true,
+};

I'll send an updated series.

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 3/8] net: can: c_can: Add RAMINIT register information to driver data

[Roger Quadros]

On 11/14/2014 07:55 PM, Marc Kleine-Budde wrote:
> On 11/07/2014 03:49 PM, Roger Quadros wrote:
>> Some platforms (e.g. TI) need special RAMINIT register handling.
>> Provide a way to store RAMINIT register description in driver data.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>  drivers/net/can/c_can/c_can.h          | 6 ++++++
>>  drivers/net/can/c_can/c_can_platform.c | 1 +
>>  2 files changed, 7 insertions(+)
>>
>> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
>> index 26c975d..3c305a1 100644
>> --- a/drivers/net/can/c_can/c_can.h
>> +++ b/drivers/net/can/c_can/c_can.h
>> @@ -171,6 +171,12 @@ enum c_can_dev_id {
>>  
>>  struct c_can_driver_data {
>>  	enum c_can_dev_id id;
>> +
>> +	/* RAMINIT register description. Optional. */
>> +	u8 num_can;		/* Number of CAN instances on the SoC */
>> +	u8 *raminit_start_bits;	/* Array of START bit positions */
>> +	u8 *raminit_done_bits;	/* Array of DONE bit positions */
> 
> What do you think about making this a struct:
> 
> +struct raminit_bits {
> +       u8 start;
> +       u8 done;
> +};
> 
>  struct c_can_driver_data {
>         enum c_can_dev_id id;
> +
> +       /* RAMINIT register description. Optional. */
> +       const struct raminit_bits *raminit_bits; /* Array of START/DONE bit positions */
> +       u8 raminit_num;         /* Number of CAN instances on the SoC */
> +       bool raminit_pulse;     /* If set, sets and clears START bit (pulse) */
>  };
> 
> The driver data looks like this:
> 
> +static const struct raminit_bits dra7_raminit_bits[] = {
> +       [0] = { .start = 3, .done = 1, },
> +       [1] = { .start = 5, .done = 2, },
> +};
> +
> +static const struct c_can_driver_data dra7_dcan_drvdata = {
> +       .id = BOSCH_D_CAN,
> +       .raminit_num = ARRAY_SIZE(dra7_raminit_bits),
> +       .raminit_bits = dra7_raminit_bits,
> +       .raminit_pulse = true,
> +};
> 
> I'll send an updated series.

Looks better. Thanks.

cheers,
-roger

Re: [PATCH v4 3/8] net: can: c_can: Add RAMINIT register information to driver data

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 469 bytes --]

On 11/17/2014 12:17 PM, Roger Quadros wrote:
>> I'll send an updated series.
> 
> Looks better. Thanks.

Can you make a quick test with the new series on real hardware.

Thanks,
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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 3/8] net: can: c_can: Add RAMINIT register information to driver data

[Roger Quadros]

On 11/17/2014 01:22 PM, Marc Kleine-Budde wrote:
> On 11/17/2014 12:17 PM, Roger Quadros wrote:
>>> I'll send an updated series.
>>
>> Looks better. Thanks.
> 
> Can you make a quick test with the new series on real hardware.

Yes. I'll let you know in a while and respond on the v8 thread.

cheers,
-roger

Re: [PATCH v7 6/8] net: can: c_can: Disable pins when CAN interface is down

[Linus Walleij]

On Fri, Nov 14, 2014 at 4:40 PM, Roger Quadros <rogerq@ti.com> wrote:

> DRA7 CAN IP suffers from a problem which causes it to be prevented
> from fully turning OFF (i.e. stuck in transition) if the module was
> disabled while there was traffic on the CAN_RX line.
>
> To work around this issue we select the SLEEP pin state by default
> on probe and use the DEFAULT pin state on CAN up and back to the
> SLEEP pin state on CAN down.
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

I see you figured it out all by yourselves :D

(Sorry for being absent.)

> +#include <linux/pinctrl/consumer.h>
> +       pinctrl_pm_select_default_state(dev->dev.parent);
> +       pinctrl_pm_select_sleep_state(dev->dev.parent);
> +       pinctrl_pm_select_sleep_state(dev->dev.parent);

NB: in drivers/base/pinctrl.c:

#ifdef CONFIG_PM
        /*
         * If power management is enabled, we also look for the optional
         * sleep and idle pin states, with semantics as defined in
         * <linux/pinctrl/pinctrl-state.h>
         */
        dev->pins->sleep_state = pinctrl_lookup_state(dev->pins->p,
                                        PINCTRL_STATE_SLEEP);

So if these states are necessary for the driver to work, put
depends on PM or select PM in the Kconfig.

Yours,
Linus Walleij

Re: [PATCH v5 6/8] net: can: c_can: Disable pins when CAN interface is down

[Linus Walleij]

On Fri, Nov 14, 2014 at 2:43 PM, Roger Quadros <rogerq@ti.com> wrote:
> On 11/13/2014 06:03 PM, Marc Kleine-Budde wrote:
>> On 11/13/2014 04:23 PM, Roger Quadros wrote:

>> I just stumbled over pinctrl_pm_select_sleep_state(), is it possible to
>> integrate this into runtime pm?
>>
>> http://lxr.free-electrons.com/source/drivers/pinctrl/core.c#L1282
>
> I think those functions are there for the same reason but not sure why aren't
> they used in runtime pm core.
>
> Linus W. any hints?

It is not used from PM core because there are cases where
you may want to put pins to sleep for completely PM-core
unrelated things.

Things like turning off a serial port from userspace,
for example. That should put the pins to sleep.

Yours,
Linus Walleij

Re: [PATCH v7 6/8] net: can: c_can: Disable pins when CAN interface is down

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1852 bytes --]

On 11/27/2014 02:26 PM, Linus Walleij wrote:
> On Fri, Nov 14, 2014 at 4:40 PM, Roger Quadros <rogerq@ti.com> wrote:
> 
>> DRA7 CAN IP suffers from a problem which causes it to be prevented
>> from fully turning OFF (i.e. stuck in transition) if the module was
>> disabled while there was traffic on the CAN_RX line.
>>
>> To work around this issue we select the SLEEP pin state by default
>> on probe and use the DEFAULT pin state on CAN up and back to the
>> SLEEP pin state on CAN down.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks, however the patch is already upstream.

> 
> I see you figured it out all by yourselves :D
> 
> (Sorry for being absent.)
> 
>> +#include <linux/pinctrl/consumer.h>
>> +       pinctrl_pm_select_default_state(dev->dev.parent);
>> +       pinctrl_pm_select_sleep_state(dev->dev.parent);
>> +       pinctrl_pm_select_sleep_state(dev->dev.parent);
> 
> NB: in drivers/base/pinctrl.c:
> 
> #ifdef CONFIG_PM
>         /*
>          * If power management is enabled, we also look for the optional
>          * sleep and idle pin states, with semantics as defined in
>          * <linux/pinctrl/pinctrl-state.h>
>          */
>         dev->pins->sleep_state = pinctrl_lookup_state(dev->pins->p,
>                                         PINCTRL_STATE_SLEEP);
> 
> So if these states are necessary for the driver to work, put
> depends on PM or select PM in the Kconfig.

Roger, you can prepare a patch, if needed.

Thanks,
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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v7 6/8] net: can: c_can: Disable pins when CAN interface is down

[Roger Quadros]

On 27/11/14 23:19, Marc Kleine-Budde wrote:
> On 11/27/2014 02:26 PM, Linus Walleij wrote:
>> On Fri, Nov 14, 2014 at 4:40 PM, Roger Quadros <rogerq@ti.com> wrote:
>>
>>> DRA7 CAN IP suffers from a problem which causes it to be prevented
>>> from fully turning OFF (i.e. stuck in transition) if the module was
>>> disabled while there was traffic on the CAN_RX line.
>>>
>>> To work around this issue we select the SLEEP pin state by default
>>> on probe and use the DEFAULT pin state on CAN up and back to the
>>> SLEEP pin state on CAN down.
>>>
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>
>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Thanks, however the patch is already upstream.
> 
>>
>> I see you figured it out all by yourselves :D
>>
>> (Sorry for being absent.)
>>
>>> +#include <linux/pinctrl/consumer.h>
>>> +       pinctrl_pm_select_default_state(dev->dev.parent);
>>> +       pinctrl_pm_select_sleep_state(dev->dev.parent);
>>> +       pinctrl_pm_select_sleep_state(dev->dev.parent);
>>
>> NB: in drivers/base/pinctrl.c:
>>
>> #ifdef CONFIG_PM
>>         /*
>>          * If power management is enabled, we also look for the optional
>>          * sleep and idle pin states, with semantics as defined in
>>          * <linux/pinctrl/pinctrl-state.h>
>>          */
>>         dev->pins->sleep_state = pinctrl_lookup_state(dev->pins->p,
>>                                         PINCTRL_STATE_SLEEP);
>>
>> So if these states are necessary for the driver to work, put
>> depends on PM or select PM in the Kconfig.
> 
> Roger, you can prepare a patch, if needed.

As pinctrl sleep is an optional driver feature we don't need to do any changes.
For the DRA7 specific case, the platform configs ensure that PM is enabled.

cheers,
-roger