[PATCH 0/3] c_can: Update can support

[PATCH 0/3] c_can: Update can support

[Amit Virdi]

This patchset aims to improve the Bosch c_can driver support. The changes are
as a result of problems encountered using the c_can driver on SPEAr platform.

Although significant care has been taken to ensure that the changes don't break
the existing functionality, however there might be some hiccups on other platforms.

The patch to provide generic interface is provided to add more flexibility for
the end applications.

The patches have been tested on SPEAr1310 EVB and the patchset rebased on master
of http://git.gitorious.org/linux-can/linux-can-next.git

Amit Virdi (3):
  can: c_can: Correct register alignment info passing through DT
  can: c_can: Provide generic interface to configure c-can message
    objects
  can: c_can: Enable clock before first use

 .../devicetree/bindings/net/can/c_can.txt          |  59 ++++++++++--
 drivers/net/can/c_can/c_can.c                      | 100 ++++++++++++---------
 drivers/net/can/c_can/c_can.h                      |  16 +++-
 drivers/net/can/c_can/c_can_platform.c             |  66 +++++++++++++-
 4 files changed, 190 insertions(+), 51 deletions(-)

-- 
1.8.0

[PATCH 1/3] can: c_can: Correct register alignment info passing through DT

[Amit Virdi]

Depending upon the integration of the C_CAN controller in the SoC, the
registers may be aligned to 16-bit or 32-bit boundary. The device tree
support added to the driver assumes this information can be passed
through 'flags' in the platform resources, however current DT
implementation provides no such provision.

So passing the information explicitly through the optional parameter
'reg_alignment' in the device tree.

Signed-off-by: Amit Virdi <amit.virdi@st.com>
Reviewed-by: Shiraz Hashim <shiraz.hashim@st.com>
---
 Documentation/devicetree/bindings/net/can/c_can.txt |  4 ++++
 drivers/net/can/c_can/c_can_platform.c              | 11 ++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/can/c_can.txt b/Documentation/devicetree/bindings/net/can/c_can.txt
index 8f1ae81..7d645cb 100644
--- a/Documentation/devicetree/bindings/net/can/c_can.txt
+++ b/Documentation/devicetree/bindings/net/can/c_can.txt
@@ -12,6 +12,10 @@ Required properties:
 Optional properties:
 - ti,hwmods		: Must be "d_can<n>" or "c_can<n>", n being the
 			  instance number
+- reg_alignment : Signifies register alignment. Must be either of
+                        1. 0x8 or 0x10 - If registers are 16-bit aligned
+                        2. 0x18 - If registers are 32-bit aligned
+                Default is 32-bit alignment.
 
 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_platform.c b/drivers/net/can/c_can/c_can_platform.c
index 0044fd8..451e81f 100644
--- a/drivers/net/can/c_can/c_can_platform.c
+++ b/drivers/net/can/c_can/c_can_platform.c
@@ -117,6 +117,7 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
 	struct pinctrl *pinctrl;
 	struct resource *mem, *res;
 	int irq;
+	u32 reg_alignment = IORESOURCE_MEM_32BIT;
 	struct clk *clk;
 
 	if (pdev->dev.of_node) {
@@ -127,6 +128,11 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
 			goto exit;
 		}
 		id = match->data;
+
+		if (of_property_read_u32(pdev->dev.of_node,
+				"reg_alignment", &reg_alignment))
+			dev_warn(&pdev->dev, "Register alignment not provided, \
+					using 32-bit as default\n");
 	} else {
 		id = platform_get_device_id(pdev);
 	}
@@ -166,6 +172,9 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
 		goto exit_release_mem;
 	}
 
+	if (!pdev->dev.of_node)
+		reg_alignment = mem->flags;
+
 	/* allocate the c_can device */
 	dev = alloc_c_can_dev();
 	if (!dev) {
@@ -177,7 +186,7 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
 	switch (id->driver_data) {
 	case BOSCH_C_CAN:
 		priv->regs = reg_map_c_can;
-		switch (mem->flags & IORESOURCE_MEM_TYPE_MASK) {
+		switch (reg_alignment & IORESOURCE_MEM_TYPE_MASK) {
 		case IORESOURCE_MEM_32BIT:
 			priv->read_reg = c_can_plat_read_reg_aligned_to_32bit;
 			priv->write_reg = c_can_plat_write_reg_aligned_to_32bit;
-- 
1.8.0

Re: [PATCH 1/3] can: c_can: Correct register alignment info passing through DT

[Marc Kleine-Budde]

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

On 12/20/2012 11:05 AM, Amit Virdi wrote:
> Depending upon the integration of the C_CAN controller in the SoC, the
> registers may be aligned to 16-bit or 32-bit boundary. The device tree
> support added to the driver assumes this information can be passed
> through 'flags' in the platform resources, however current DT
> implementation provides no such provision.
> 
> So passing the information explicitly through the optional parameter
> 'reg_alignment' in the device tree.

Please put devicetree-discuss for dt related topics on Cc. Have a look
at Documentation/devicetree/bindings for existing bindings. E.g. there
already is reg-io-width.

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: 261 bytes --]

Re: [PATCH 1/3] can: c_can: Correct register alignment info passing through DT

[Amit Virdi]

On 12/20/2012 7:29 PM, Marc Kleine-Budde wrote:
> On 12/20/2012 11:05 AM, Amit Virdi wrote:
>> Depending upon the integration of the C_CAN controller in the SoC, the
>> registers may be aligned to 16-bit or 32-bit boundary. The device tree
>> support added to the driver assumes this information can be passed
>> through 'flags' in the platform resources, however current DT
>> implementation provides no such provision.
>>
>> So passing the information explicitly through the optional parameter
>> 'reg_alignment' in the device tree.
>
> Please put devicetree-discuss for dt related topics on Cc. Have a look
> at Documentation/devicetree/bindings for existing bindings. E.g. there
> already is reg-io-width.
>

Yeah, thanks! I would modify the code for this.

Regards
Amit Virdi

[PATCH 2/3] can: c_can: Provide generic interface to configure c-can message objects

[Amit Virdi]

Depending on the underlying platform, the configuration of the c-can
message objects can change. For e.g. in some systems, it makes more
sense to receive many message objects and transmit very few. In any
case, providing flexibility in configuring the message objects is highly
desirable.

The total number of message objects for C_CAN controller is fixed at 32.
The receive message objects are assigned higher priority so they begin
with message object#1. So, in order to configure the message object the
driver just needs two parameters - rx_split (for differentiating between
lower bucket and higher bucket) and tx_num.

However, if the user doesn't specify these parameters, then the message
objects are configured with default parameters with equal distribution
of receive and transmit message objects (16 each).

Signed-off-by: Amit Virdi <amit.virdi@st.com>
Cc: Bhupesh Sharma <bhupesh.sharma@st.com>
Reviewed-by: Shiraz Hashim <shiraz.hashim@st.com>
---
 .../devicetree/bindings/net/can/c_can.txt          |  55 ++++++++++--
 drivers/net/can/c_can/c_can.c                      | 100 ++++++++++++---------
 drivers/net/can/c_can/c_can.h                      |  16 +++-
 drivers/net/can/c_can/c_can_platform.c             |  47 +++++++++-
 4 files changed, 168 insertions(+), 50 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/can/c_can.txt b/Documentation/devicetree/bindings/net/can/c_can.txt
index 7d645cb..4ddd127 100644
--- a/Documentation/devicetree/bindings/net/can/c_can.txt
+++ b/Documentation/devicetree/bindings/net/can/c_can.txt
@@ -8,20 +8,25 @@ Required properties:
 			  registers map
 - interrupts		: property with a value describing the interrupt
 			  number
-
 Optional properties:
+
 - ti,hwmods		: Must be "d_can<n>" or "c_can<n>", n being the
 			  instance number
+- rx_split      : Receive message objects lying in the lower bucket
+- tx_num        : Total number of transmit message objects
 - reg_alignment : Signifies register alignment. Must be either of
                         1. 0x8 or 0x10 - If registers are 16-bit aligned
                         2. 0x18 - If registers are 32-bit aligned
                 Default is 32-bit alignment.
 
-Note: "ti,hwmods" field is used to fetch the base address and irq
-resources from TI, omap hwmod data base during device registration.
-Future plan is to migrate hwmod data base contents into device tree
-blob so that, all the required data will be used from device tree dts
-file.
+Note:
+1. "ti,hwmods" field is used to fetch the base address and irq resources from
+   TI, omap hwmod data base during device registration.  Future plan is to
+   migrate hwmod data base contents into device tree blob so that, all the
+   required data will be used from device tree dts file.
+2. Bosch C_CAN controller has 32 total message objects. RX first message object
+   starts from 1. Depending upon the application, rx_split should lie somewhere
+   between rx_first and rx_last (32 - tx_num)
 
 Example:
 
@@ -46,8 +51,46 @@ Step1: SoC common .dtsi file
 		status = "disabled";
 	};
 
+(or)
+
+	can0@a1000000 {
+		compatible = "bosch,c_can";
+		ti,hwmods = "c_can0";
+		reg = <0xa1000000 0x1000>;
+		interrupts = <55>;
+		interrupt-parent = <&intc>;
+		reg_alignment = <0x10>;
+		status = "disabled";
+	};
+
+(or)
+
+	can1@a2000000 {
+		compatible = "bosch,c_can";
+		ti,hwmods = "c_can1";
+		reg = <0xa2000000 0x1000>;
+		interrupts = <55>;
+		interrupt-parent = <&intc>;
+		reg_alignment = <0x18>;
+		status = "disabled";
+	};
+
 Step 2: board specific .dts file
 
+	can0@a1000000 {
+		rx_split = <16>;
+		tx_num = <6>;
+		status = "okay";
+	};
+
+(or)
+
+	can1@a2000000 {
+		status = "okay";
+	};
+
+(or)
+
 	&dcan1 {
 		status = "okay";
 	};
diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 5233b8f..723bcba 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -141,22 +141,12 @@
 
 /* message object split */
 #define C_CAN_NO_OF_OBJECTS	32
-#define C_CAN_MSG_OBJ_RX_NUM	16
-#define C_CAN_MSG_OBJ_TX_NUM	16
-
 #define C_CAN_MSG_OBJ_RX_FIRST	1
-#define C_CAN_MSG_OBJ_RX_LAST	(C_CAN_MSG_OBJ_RX_FIRST + \
-				C_CAN_MSG_OBJ_RX_NUM - 1)
-
-#define C_CAN_MSG_OBJ_TX_FIRST	(C_CAN_MSG_OBJ_RX_LAST + 1)
-#define C_CAN_MSG_OBJ_TX_LAST	(C_CAN_MSG_OBJ_TX_FIRST + \
-				C_CAN_MSG_OBJ_TX_NUM - 1)
 
-#define C_CAN_MSG_OBJ_RX_SPLIT	9
-#define C_CAN_MSG_RX_LOW_LAST	(C_CAN_MSG_OBJ_RX_SPLIT - 1)
+#define C_CAN_MSG_OBJ_RX_LAST(x)	(C_CAN_NO_OF_OBJECTS -\
+						get_msg_obj_tx_num(x))
 
-#define C_CAN_NEXT_MSG_OBJ_MASK	(C_CAN_MSG_OBJ_TX_NUM - 1)
-#define RECEIVE_OBJECT_BITS	0x0000ffff
+#define C_CAN_MSG_OBJ_TX_LAST	C_CAN_NO_OF_OBJECTS
 
 /* status interrupt */
 #define STATUS_INTERRUPT	0x8000
@@ -171,9 +161,6 @@
 /* Wait for ~1 sec for INIT bit */
 #define INIT_WAIT_MS		1000
 
-/* napi related */
-#define C_CAN_NAPI_WEIGHT	C_CAN_MSG_OBJ_RX_NUM
-
 /* c_can lec values */
 enum c_can_lec_type {
 	LEC_NO_ERROR = 0,
@@ -239,16 +226,42 @@ static inline void c_can_reset_ram(const struct c_can_priv *priv, bool enable)
 		priv->raminit(priv, enable);
 }
 
+static inline unsigned int get_msg_obj_rx_split(const struct c_can_priv *priv)
+{
+	return priv->pdata->rx_split;
+}
+
+static inline unsigned int get_msg_obj_tx_num(const struct c_can_priv *priv)
+{
+	return priv->pdata->tx_num;
+}
+
+static inline unsigned int get_msg_obj_rx_low_last(
+			const struct c_can_priv *priv)
+{
+	return get_msg_obj_rx_split(priv) - 1;
+}
+
+static inline unsigned int get_msg_obj_tx_first(const struct c_can_priv *priv)
+{
+	return C_CAN_NO_OF_OBJECTS - get_msg_obj_tx_num(priv) + 1;
+}
+
+static inline unsigned int get_next_msg_obj_mask(const struct c_can_priv *priv)
+{
+	return get_msg_obj_tx_num(priv) - 1;
+}
+
 static inline int get_tx_next_msg_obj(const struct c_can_priv *priv)
 {
-	return (priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) +
-			C_CAN_MSG_OBJ_TX_FIRST;
+	return (priv->tx_next & get_next_msg_obj_mask(priv)) +
+			get_msg_obj_tx_first(priv);
 }
 
 static inline int get_tx_echo_msg_obj(const struct c_can_priv *priv)
 {
-	return (priv->tx_echo & C_CAN_NEXT_MSG_OBJ_MASK) +
-			C_CAN_MSG_OBJ_TX_FIRST;
+	return (priv->tx_echo & get_next_msg_obj_mask(priv)) +
+			get_msg_obj_tx_first(priv);
 }
 
 static u32 c_can_read_reg32(struct c_can_priv *priv, enum reg index)
@@ -384,7 +397,8 @@ static inline void c_can_activate_all_lower_rx_msg_obj(struct net_device *dev,
 	int i;
 	struct c_can_priv *priv = netdev_priv(dev);
 
-	for (i = C_CAN_MSG_OBJ_RX_FIRST; i <= C_CAN_MSG_RX_LOW_LAST; i++) {
+	for (i = C_CAN_MSG_OBJ_RX_FIRST; i <= get_msg_obj_rx_low_last(priv);
+			i++) {
 		priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface),
 				ctrl_mask & ~(IF_MCONT_MSGLST |
 					IF_MCONT_INTPND | IF_MCONT_NEWDAT));
@@ -545,7 +559,7 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
 
 	/* prepare message object for transmission */
 	c_can_write_msg_object(dev, 0, frame, msg_obj_no);
-	can_put_echo_skb(skb, dev, msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
+	can_put_echo_skb(skb, dev, msg_obj_no - get_msg_obj_tx_first(priv));
 
 	/*
 	 * we have to stop the queue in case of a wrap around or
@@ -553,7 +567,7 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
 	 */
 	priv->tx_next++;
 	if (c_can_is_next_tx_obj_busy(priv, get_tx_next_msg_obj(priv)) ||
-			(priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) == 0)
+			(priv->tx_next & get_next_msg_obj_mask(priv)) == 0)
 		netif_stop_queue(dev);
 
 	return NETDEV_TX_OK;
@@ -604,17 +618,18 @@ static int c_can_set_bittiming(struct net_device *dev)
 static void c_can_configure_msg_objects(struct net_device *dev)
 {
 	int i;
+	struct c_can_priv *priv = netdev_priv(dev);
 
 	/* first invalidate all message objects */
 	for (i = C_CAN_MSG_OBJ_RX_FIRST; i <= C_CAN_NO_OF_OBJECTS; i++)
 		c_can_inval_msg_object(dev, 0, i);
 
 	/* setup receive message objects */
-	for (i = C_CAN_MSG_OBJ_RX_FIRST; i < C_CAN_MSG_OBJ_RX_LAST; i++)
+	for (i = C_CAN_MSG_OBJ_RX_FIRST; i < C_CAN_MSG_OBJ_RX_LAST(priv); i++)
 		c_can_setup_receive_object(dev, 0, i, 0, 0,
 			(IF_MCONT_RXIE | IF_MCONT_UMASK) & ~IF_MCONT_EOB);
 
-	c_can_setup_receive_object(dev, 0, C_CAN_MSG_OBJ_RX_LAST, 0, 0,
+	c_can_setup_receive_object(dev, 0, C_CAN_MSG_OBJ_RX_LAST(priv), 0, 0,
 			IF_MCONT_EOB | IF_MCONT_RXIE | IF_MCONT_UMASK);
 }
 
@@ -745,8 +760,8 @@ static void c_can_do_tx(struct net_device *dev)
 		msg_obj_no = get_tx_echo_msg_obj(priv);
 		val = c_can_read_reg32(priv, C_CAN_TXRQST1_REG);
 		if (!(val & (1 << (msg_obj_no - 1)))) {
-			can_get_echo_skb(dev,
-					msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
+			can_get_echo_skb(dev, msg_obj_no - \
+					get_msg_obj_tx_first(priv));
 			stats->tx_bytes += priv->read_reg(priv,
 					C_CAN_IFACE(MSGCTRL_REG, 0))
 					& IF_MCONT_DLC_MASK;
@@ -758,8 +773,8 @@ static void c_can_do_tx(struct net_device *dev)
 	}
 
 	/* restart queue if wrap-up or if queue stalled on last pkt */
-	if (((priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) != 0) ||
-			((priv->tx_echo & C_CAN_NEXT_MSG_OBJ_MASK) == 0))
+	if (((priv->tx_next & get_next_msg_obj_mask(priv)) != 0) ||
+			((priv->tx_echo & get_next_msg_obj_mask(priv)) == 0))
 		netif_wake_queue(dev);
 }
 
@@ -770,19 +785,19 @@ static void c_can_do_tx(struct net_device *dev)
  * object it finds free (starting with the lowest). Bits NEWDAT and
  * INTPND are set for this message object indicating that a new message
  * has arrived. To work-around this issue, we keep two groups of message
- * objects whose partitioning is defined by C_CAN_MSG_OBJ_RX_SPLIT.
+ * objects whose partitioning is defined by get_msg_obj_rx_split(priv).
  *
  * To ensure in-order frame reception we use the following
  * approach while re-activating a message object to receive further
  * frames:
  * - if the current message object number is lower than
- *   C_CAN_MSG_RX_LOW_LAST, do not clear the NEWDAT bit while clearing
+ *   get_msg_obj_rx_low_last(priv), do not clear the NEWDAT bit while clearing
  *   the INTPND bit.
  * - if the current message object number is equal to
- *   C_CAN_MSG_RX_LOW_LAST then clear the NEWDAT bit of all lower
+ *   get_msg_obj_rx_low_last(priv) then clear the NEWDAT bit of all lower
  *   receive message objects.
  * - if the current message object number is greater than
- *   C_CAN_MSG_RX_LOW_LAST then clear the NEWDAT bit of
+ *   get_msg_obj_rx_low_last(priv) then clear the NEWDAT bit of
  *   only this message object.
  */
 static int c_can_do_rx_poll(struct net_device *dev, int quota)
@@ -793,7 +808,7 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
 	u32 val = c_can_read_reg32(priv, C_CAN_INTPND1_REG);
 
 	for (msg_obj = C_CAN_MSG_OBJ_RX_FIRST;
-			msg_obj <= C_CAN_MSG_OBJ_RX_LAST && quota > 0;
+			msg_obj <= C_CAN_MSG_OBJ_RX_LAST(priv) && quota > 0;
 			val = c_can_read_reg32(priv, C_CAN_INTPND1_REG),
 			msg_obj++) {
 		/*
@@ -822,14 +837,14 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
 			/* read the data from the message object */
 			c_can_read_msg_object(dev, 0, msg_ctrl_save);
 
-			if (msg_obj < C_CAN_MSG_RX_LOW_LAST)
+			if (msg_obj < get_msg_obj_rx_low_last(priv))
 				c_can_mark_rx_msg_obj(dev, 0,
 						msg_ctrl_save, msg_obj);
-			else if (msg_obj > C_CAN_MSG_RX_LOW_LAST)
+			else if (msg_obj > get_msg_obj_rx_low_last(priv))
 				/* activate this msg obj */
 				c_can_activate_rx_msg_obj(dev, 0,
 						msg_ctrl_save, msg_obj);
-			else if (msg_obj == C_CAN_MSG_RX_LOW_LAST)
+			else if (msg_obj == get_msg_obj_rx_low_last(priv))
 				/* activate all lower message objects */
 				c_can_activate_all_lower_rx_msg_obj(dev,
 						0, msg_ctrl_save);
@@ -1055,10 +1070,10 @@ static int c_can_poll(struct napi_struct *napi, int quota)
 		if (lec_type)
 			work_done += c_can_handle_bus_err(dev, lec_type);
 	} else if ((irqstatus >= C_CAN_MSG_OBJ_RX_FIRST) &&
-			(irqstatus <= C_CAN_MSG_OBJ_RX_LAST)) {
+			(irqstatus <= C_CAN_MSG_OBJ_RX_LAST(priv))) {
 		/* handle events corresponding to receive message objects */
 		work_done += c_can_do_rx_poll(dev, (quota - work_done));
-	} else if ((irqstatus >= C_CAN_MSG_OBJ_TX_FIRST) &&
+	} else if ((irqstatus >= get_msg_obj_tx_first(priv)) &&
 			(irqstatus <= C_CAN_MSG_OBJ_TX_LAST)) {
 		/* handle events corresponding to transmit message objects */
 		c_can_do_tx(dev);
@@ -1146,17 +1161,18 @@ static int c_can_close(struct net_device *dev)
 	return 0;
 }
 
-struct net_device *alloc_c_can_dev(void)
+struct net_device *alloc_c_can_dev(unsigned int tx_num)
 {
 	struct net_device *dev;
 	struct c_can_priv *priv;
 
-	dev = alloc_candev(sizeof(struct c_can_priv), C_CAN_MSG_OBJ_TX_NUM);
+	dev = alloc_candev(sizeof(struct c_can_priv), tx_num);
 	if (!dev)
 		return NULL;
 
 	priv = netdev_priv(dev);
-	netif_napi_add(dev, &priv->napi, c_can_poll, C_CAN_NAPI_WEIGHT);
+	netif_napi_add(dev, &priv->napi, c_can_poll, \
+			(C_CAN_NO_OF_OBJECTS - tx_num));
 
 	priv->dev = dev;
 	priv->can.bittiming_const = &c_can_bittiming_const;
diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index d2e1c21..d5cd5d2 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -150,12 +150,26 @@ enum c_can_dev_id {
 	BOSCH_D_CAN,
 };
 
+/**
+ * struct c_can_platform_data - Depending on the underlying platform,
+ *                             the message object configuration
+ *                             for c_can controller can change
+ * @rx_split: defines the split point for the lower and upper Rx msg
+ *            object buckets.
+ * @tx_num: number of objs kept aside for TX purposes
+ */
+struct c_can_platform_data {
+	unsigned int rx_split;
+	unsigned int tx_num;
+};
+
 /* c_can private data structure */
 struct c_can_priv {
 	struct can_priv can;	/* must be the first member */
 	struct napi_struct napi;
 	struct net_device *dev;
 	struct device *device;
+	struct c_can_platform_data *pdata;
 	int tx_object;
 	int current_status;
 	int last_status;
@@ -174,7 +188,7 @@ struct c_can_priv {
 	void (*raminit) (const struct c_can_priv *priv, bool enable);
 };
 
-struct net_device *alloc_c_can_dev(void);
+struct net_device *alloc_c_can_dev(unsigned int echo_count);
 void free_c_can_dev(struct net_device *dev);
 int register_c_can_dev(struct net_device *dev);
 void unregister_c_can_dev(struct net_device *dev);
diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
index 451e81f..6e8dc56 100644
--- a/drivers/net/can/c_can/c_can_platform.c
+++ b/drivers/net/can/c_can/c_can_platform.c
@@ -106,6 +106,25 @@ static const struct of_device_id c_can_of_table[] = {
 };
 MODULE_DEVICE_TABLE(of, c_can_of_table);
 
+#ifdef CONFIG_OF
+static int __devinit c_can_probe_config_dt(struct platform_device *pdev,
+					struct device_node *np)
+{
+	struct c_can_platform_data *pdata = dev_get_platdata(&pdev->dev);
+
+	of_property_read_u32(np, "rx_split", &pdata->rx_split);
+	of_property_read_u32(np, "tx_num", &pdata->tx_num);
+
+	return 0;
+}
+#else
+static int __devinit c_can_probe_config_dt(struct platform_device *pdev,
+		struct device_node *np)
+{
+	return -ENOSYS;
+}
+#endif
+
 static int __devinit c_can_plat_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -114,6 +133,7 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
 	struct c_can_priv *priv;
 	const struct of_device_id *match;
 	const struct platform_device_id *id;
+	struct c_can_platform_data *pdata = dev_get_platdata(&pdev->dev);
 	struct pinctrl *pinctrl;
 	struct resource *mem, *res;
 	int irq;
@@ -129,12 +149,36 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
 		}
 		id = match->data;
 
+		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+		pdev->dev.platform_data = pdata;
+		ret = c_can_probe_config_dt(pdev, pdev->dev.of_node);
+
+		if (ret) {
+			dev_err(&pdev->dev, "Missing platform data\n");
+			return -ENOSYS;
+		} else if (!(pdata->rx_split && pdata->tx_num)) {
+			dev_info(&pdev->dev,
+					"invalid plat data, using default\n");
+			pdata->rx_split = 9;
+			pdata->tx_num = 16;
+		}
+
 		if (of_property_read_u32(pdev->dev.of_node,
 				"reg_alignment", &reg_alignment))
 			dev_warn(&pdev->dev, "Register alignment not provided, \
 					using 32-bit as default\n");
 	} else {
 		id = platform_get_device_id(pdev);
+
+		/*
+		 * get the device specific details like Rx/Tx message object
+		 * configurations from the platform data.
+		 */
+		pdata = pdev->dev.platform_data;
+		if (!pdata) {
+			dev_err(&pdev->dev, "platform data is NULL\n");
+			return -EINVAL;
+		}
 	}
 
 	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
@@ -176,7 +220,7 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
 		reg_alignment = mem->flags;
 
 	/* allocate the c_can device */
-	dev = alloc_c_can_dev();
+	dev = alloc_c_can_dev(pdata->tx_num);
 	if (!dev) {
 		ret = -ENOMEM;
 		goto exit_iounmap;
@@ -186,6 +230,7 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
 	switch (id->driver_data) {
 	case BOSCH_C_CAN:
 		priv->regs = reg_map_c_can;
+		priv->pdata = pdata;
 		switch (reg_alignment & IORESOURCE_MEM_TYPE_MASK) {
 		case IORESOURCE_MEM_32BIT:
 			priv->read_reg = c_can_plat_read_reg_aligned_to_32bit;
-- 
1.8.0

Re: [PATCH 2/3] can: c_can: Provide generic interface to configure c-can message objects

[Wolfgang Grandegger]

On 12/20/2012 11:05 AM, Amit Virdi wrote:
> Depending on the underlying platform, the configuration of the c-can
> message objects can change. For e.g. in some systems, it makes more
> sense to receive many message objects and transmit very few. In any
> case, providing flexibility in configuring the message objects is highly
> desirable.
> 
> The total number of message objects for C_CAN controller is fixed at 32.
> The receive message objects are assigned higher priority so they begin
> with message object#1. So, in order to configure the message object the
> driver just needs two parameters - rx_split (for differentiating between
> lower bucket and higher bucket) and tx_num.

What is your motivation to make this configurable?
Why is the current default not good enough?

> However, if the user doesn't specify these parameters, then the message
> objects are configured with default parameters with equal distribution
> of receive and transmit message objects (16 each).
> 
> Signed-off-by: Amit Virdi <amit.virdi@st.com>
> Cc: Bhupesh Sharma <bhupesh.sharma@st.com>
> Reviewed-by: Shiraz Hashim <shiraz.hashim@st.com>
> ---
>  .../devicetree/bindings/net/can/c_can.txt          |  55 ++++++++++--
>  drivers/net/can/c_can/c_can.c                      | 100 ++++++++++++---------
>  drivers/net/can/c_can/c_can.h                      |  16 +++-
>  drivers/net/can/c_can/c_can_platform.c             |  47 +++++++++-
>  4 files changed, 168 insertions(+), 50 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/c_can.txt b/Documentation/devicetree/bindings/net/can/c_can.txt
> index 7d645cb..4ddd127 100644
> --- a/Documentation/devicetree/bindings/net/can/c_can.txt
> +++ b/Documentation/devicetree/bindings/net/can/c_can.txt
> @@ -8,20 +8,25 @@ Required properties:
>  			  registers map
>  - interrupts		: property with a value describing the interrupt
>  			  number
> -
>  Optional properties:
> +
>  - ti,hwmods		: Must be "d_can<n>" or "c_can<n>", n being the
>  			  instance number
> +- rx_split      : Receive message objects lying in the lower bucket
> +- tx_num        : Total number of transmit message objects
>  - reg_alignment : Signifies register alignment. Must be either of
>                          1. 0x8 or 0x10 - If registers are 16-bit aligned
>                          2. 0x18 - If registers are 32-bit aligned
>                  Default is 32-bit alignment.

I would prefer "reg_alignment" in bytes. 0x4 is much more readable than
0x18.

> -Note: "ti,hwmods" field is used to fetch the base address and irq
> -resources from TI, omap hwmod data base during device registration.
> -Future plan is to migrate hwmod data base contents into device tree
> -blob so that, all the required data will be used from device tree dts
> -file.
> +Note:
> +1. "ti,hwmods" field is used to fetch the base address and irq resources from
> +   TI, omap hwmod data base during device registration.  Future plan is to
> +   migrate hwmod data base contents into device tree blob so that, all the
> +   required data will be used from device tree dts file.
> +2. Bosch C_CAN controller has 32 total message objects. RX first message object
> +   starts from 1. Depending upon the application, rx_split should lie somewhere
> +   between rx_first and rx_last (32 - tx_num)
>  
>  Example:
>  
> @@ -46,8 +51,46 @@ Step1: SoC common .dtsi file
>  		status = "disabled";
>  	};
>  
> +(or)
> +
> +	can0@a1000000 {
> +		compatible = "bosch,c_can";
> +		ti,hwmods = "c_can0";
> +		reg = <0xa1000000 0x1000>;
> +		interrupts = <55>;
> +		interrupt-parent = <&intc>;
> +		reg_alignment = <0x10>;
> +		status = "disabled";
> +	};
> +
> +(or)
> +
> +	can1@a2000000 {
> +		compatible = "bosch,c_can";
> +		ti,hwmods = "c_can1";
> +		reg = <0xa2000000 0x1000>;
> +		interrupts = <55>;
> +		interrupt-parent = <&intc>;
> +		reg_alignment = <0x18>;
> +		status = "disabled";
> +	};
> +
>  Step 2: board specific .dts file
>  
> +	can0@a1000000 {
> +		rx_split = <16>;
> +		tx_num = <6>;
> +		status = "okay";
> +	};
> +
> +(or)
> +
> +	can1@a2000000 {
> +		status = "okay";
> +	};
> +
> +(or)
> +
>  	&dcan1 {
>  		status = "okay";
>  	};
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 5233b8f..723bcba 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -141,22 +141,12 @@
>  
>  /* message object split */
>  #define C_CAN_NO_OF_OBJECTS	32
> -#define C_CAN_MSG_OBJ_RX_NUM	16
> -#define C_CAN_MSG_OBJ_TX_NUM	16
> -
>  #define C_CAN_MSG_OBJ_RX_FIRST	1
> -#define C_CAN_MSG_OBJ_RX_LAST	(C_CAN_MSG_OBJ_RX_FIRST + \
> -				C_CAN_MSG_OBJ_RX_NUM - 1)
> -
> -#define C_CAN_MSG_OBJ_TX_FIRST	(C_CAN_MSG_OBJ_RX_LAST + 1)
> -#define C_CAN_MSG_OBJ_TX_LAST	(C_CAN_MSG_OBJ_TX_FIRST + \
> -				C_CAN_MSG_OBJ_TX_NUM - 1)
>  
> -#define C_CAN_MSG_OBJ_RX_SPLIT	9
> -#define C_CAN_MSG_RX_LOW_LAST	(C_CAN_MSG_OBJ_RX_SPLIT - 1)
> +#define C_CAN_MSG_OBJ_RX_LAST(x)	(C_CAN_NO_OF_OBJECTS -\
> +						get_msg_obj_tx_num(x))
>  
> -#define C_CAN_NEXT_MSG_OBJ_MASK	(C_CAN_MSG_OBJ_TX_NUM - 1)
> -#define RECEIVE_OBJECT_BITS	0x0000ffff
> +#define C_CAN_MSG_OBJ_TX_LAST	C_CAN_NO_OF_OBJECTS
>  
>  /* status interrupt */
>  #define STATUS_INTERRUPT	0x8000
> @@ -171,9 +161,6 @@
>  /* Wait for ~1 sec for INIT bit */
>  #define INIT_WAIT_MS		1000
>  
> -/* napi related */
> -#define C_CAN_NAPI_WEIGHT	C_CAN_MSG_OBJ_RX_NUM
> -
>  /* c_can lec values */
>  enum c_can_lec_type {
>  	LEC_NO_ERROR = 0,
> @@ -239,16 +226,42 @@ static inline void c_can_reset_ram(const struct c_can_priv *priv, bool enable)
>  		priv->raminit(priv, enable);
>  }
>  
> +static inline unsigned int get_msg_obj_rx_split(const struct c_can_priv *priv)
> +{
> +	return priv->pdata->rx_split;
> +}
> +
> +static inline unsigned int get_msg_obj_tx_num(const struct c_can_priv *priv)
> +{
> +	return priv->pdata->tx_num;
> +}
> +
> +static inline unsigned int get_msg_obj_rx_low_last(
> +			const struct c_can_priv *priv)
> +{
> +	return get_msg_obj_rx_split(priv) - 1;
> +}
> +
> +static inline unsigned int get_msg_obj_tx_first(const struct c_can_priv *priv)
> +{
> +	return C_CAN_NO_OF_OBJECTS - get_msg_obj_tx_num(priv) + 1;
> +}
> +
> +static inline unsigned int get_next_msg_obj_mask(const struct c_can_priv *priv)
> +{
> +	return get_msg_obj_tx_num(priv) - 1;
> +}
> +
>  static inline int get_tx_next_msg_obj(const struct c_can_priv *priv)
>  {
> -	return (priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) +
> -			C_CAN_MSG_OBJ_TX_FIRST;
> +	return (priv->tx_next & get_next_msg_obj_mask(priv)) +
> +			get_msg_obj_tx_first(priv);
>  }
>  
>  static inline int get_tx_echo_msg_obj(const struct c_can_priv *priv)
>  {
> -	return (priv->tx_echo & C_CAN_NEXT_MSG_OBJ_MASK) +
> -			C_CAN_MSG_OBJ_TX_FIRST;
> +	return (priv->tx_echo & get_next_msg_obj_mask(priv)) +
> +			get_msg_obj_tx_first(priv);
>  }
>  
>  static u32 c_can_read_reg32(struct c_can_priv *priv, enum reg index)
> @@ -384,7 +397,8 @@ static inline void c_can_activate_all_lower_rx_msg_obj(struct net_device *dev,
>  	int i;
>  	struct c_can_priv *priv = netdev_priv(dev);
>  
> -	for (i = C_CAN_MSG_OBJ_RX_FIRST; i <= C_CAN_MSG_RX_LOW_LAST; i++) {
> +	for (i = C_CAN_MSG_OBJ_RX_FIRST; i <= get_msg_obj_rx_low_last(priv);
> +			i++) {
>  		priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface),
>  				ctrl_mask & ~(IF_MCONT_MSGLST |
>  					IF_MCONT_INTPND | IF_MCONT_NEWDAT));
> @@ -545,7 +559,7 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
>  
>  	/* prepare message object for transmission */
>  	c_can_write_msg_object(dev, 0, frame, msg_obj_no);
> -	can_put_echo_skb(skb, dev, msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
> +	can_put_echo_skb(skb, dev, msg_obj_no - get_msg_obj_tx_first(priv));
>  
>  	/*
>  	 * we have to stop the queue in case of a wrap around or
> @@ -553,7 +567,7 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
>  	 */
>  	priv->tx_next++;
>  	if (c_can_is_next_tx_obj_busy(priv, get_tx_next_msg_obj(priv)) ||
> -			(priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) == 0)
> +			(priv->tx_next & get_next_msg_obj_mask(priv)) == 0)
>  		netif_stop_queue(dev);
>  
>  	return NETDEV_TX_OK;
> @@ -604,17 +618,18 @@ static int c_can_set_bittiming(struct net_device *dev)
>  static void c_can_configure_msg_objects(struct net_device *dev)
>  {
>  	int i;
> +	struct c_can_priv *priv = netdev_priv(dev);
>  
>  	/* first invalidate all message objects */
>  	for (i = C_CAN_MSG_OBJ_RX_FIRST; i <= C_CAN_NO_OF_OBJECTS; i++)
>  		c_can_inval_msg_object(dev, 0, i);
>  
>  	/* setup receive message objects */
> -	for (i = C_CAN_MSG_OBJ_RX_FIRST; i < C_CAN_MSG_OBJ_RX_LAST; i++)
> +	for (i = C_CAN_MSG_OBJ_RX_FIRST; i < C_CAN_MSG_OBJ_RX_LAST(priv); i++)
>  		c_can_setup_receive_object(dev, 0, i, 0, 0,
>  			(IF_MCONT_RXIE | IF_MCONT_UMASK) & ~IF_MCONT_EOB);
>  
> -	c_can_setup_receive_object(dev, 0, C_CAN_MSG_OBJ_RX_LAST, 0, 0,
> +	c_can_setup_receive_object(dev, 0, C_CAN_MSG_OBJ_RX_LAST(priv), 0, 0,
>  			IF_MCONT_EOB | IF_MCONT_RXIE | IF_MCONT_UMASK);
>  }
>  
> @@ -745,8 +760,8 @@ static void c_can_do_tx(struct net_device *dev)
>  		msg_obj_no = get_tx_echo_msg_obj(priv);
>  		val = c_can_read_reg32(priv, C_CAN_TXRQST1_REG);
>  		if (!(val & (1 << (msg_obj_no - 1)))) {
> -			can_get_echo_skb(dev,
> -					msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
> +			can_get_echo_skb(dev, msg_obj_no - \
> +					get_msg_obj_tx_first(priv));
>  			stats->tx_bytes += priv->read_reg(priv,
>  					C_CAN_IFACE(MSGCTRL_REG, 0))
>  					& IF_MCONT_DLC_MASK;
> @@ -758,8 +773,8 @@ static void c_can_do_tx(struct net_device *dev)
>  	}
>  
>  	/* restart queue if wrap-up or if queue stalled on last pkt */
> -	if (((priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) != 0) ||
> -			((priv->tx_echo & C_CAN_NEXT_MSG_OBJ_MASK) == 0))
> +	if (((priv->tx_next & get_next_msg_obj_mask(priv)) != 0) ||
> +			((priv->tx_echo & get_next_msg_obj_mask(priv)) == 0))
>  		netif_wake_queue(dev);
>  }
>  
> @@ -770,19 +785,19 @@ static void c_can_do_tx(struct net_device *dev)
>   * object it finds free (starting with the lowest). Bits NEWDAT and
>   * INTPND are set for this message object indicating that a new message
>   * has arrived. To work-around this issue, we keep two groups of message
> - * objects whose partitioning is defined by C_CAN_MSG_OBJ_RX_SPLIT.
> + * objects whose partitioning is defined by get_msg_obj_rx_split(priv).
>   *
>   * To ensure in-order frame reception we use the following
>   * approach while re-activating a message object to receive further
>   * frames:
>   * - if the current message object number is lower than
> - *   C_CAN_MSG_RX_LOW_LAST, do not clear the NEWDAT bit while clearing
> + *   get_msg_obj_rx_low_last(priv), do not clear the NEWDAT bit while clearing
>   *   the INTPND bit.
>   * - if the current message object number is equal to
> - *   C_CAN_MSG_RX_LOW_LAST then clear the NEWDAT bit of all lower
> + *   get_msg_obj_rx_low_last(priv) then clear the NEWDAT bit of all lower
>   *   receive message objects.
>   * - if the current message object number is greater than
> - *   C_CAN_MSG_RX_LOW_LAST then clear the NEWDAT bit of
> + *   get_msg_obj_rx_low_last(priv) then clear the NEWDAT bit of
>   *   only this message object.
>   */
>  static int c_can_do_rx_poll(struct net_device *dev, int quota)
> @@ -793,7 +808,7 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
>  	u32 val = c_can_read_reg32(priv, C_CAN_INTPND1_REG);
>  
>  	for (msg_obj = C_CAN_MSG_OBJ_RX_FIRST;
> -			msg_obj <= C_CAN_MSG_OBJ_RX_LAST && quota > 0;
> +			msg_obj <= C_CAN_MSG_OBJ_RX_LAST(priv) && quota > 0;
>  			val = c_can_read_reg32(priv, C_CAN_INTPND1_REG),
>  			msg_obj++) {
>  		/*
> @@ -822,14 +837,14 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
>  			/* read the data from the message object */
>  			c_can_read_msg_object(dev, 0, msg_ctrl_save);
>  
> -			if (msg_obj < C_CAN_MSG_RX_LOW_LAST)
> +			if (msg_obj < get_msg_obj_rx_low_last(priv))
>  				c_can_mark_rx_msg_obj(dev, 0,
>  						msg_ctrl_save, msg_obj);
> -			else if (msg_obj > C_CAN_MSG_RX_LOW_LAST)
> +			else if (msg_obj > get_msg_obj_rx_low_last(priv))
>  				/* activate this msg obj */
>  				c_can_activate_rx_msg_obj(dev, 0,
>  						msg_ctrl_save, msg_obj);
> -			else if (msg_obj == C_CAN_MSG_RX_LOW_LAST)
> +			else if (msg_obj == get_msg_obj_rx_low_last(priv))
>  				/* activate all lower message objects */
>  				c_can_activate_all_lower_rx_msg_obj(dev,
>  						0, msg_ctrl_save);
> @@ -1055,10 +1070,10 @@ static int c_can_poll(struct napi_struct *napi, int quota)
>  		if (lec_type)
>  			work_done += c_can_handle_bus_err(dev, lec_type);
>  	} else if ((irqstatus >= C_CAN_MSG_OBJ_RX_FIRST) &&
> -			(irqstatus <= C_CAN_MSG_OBJ_RX_LAST)) {
> +			(irqstatus <= C_CAN_MSG_OBJ_RX_LAST(priv))) {
>  		/* handle events corresponding to receive message objects */
>  		work_done += c_can_do_rx_poll(dev, (quota - work_done));
> -	} else if ((irqstatus >= C_CAN_MSG_OBJ_TX_FIRST) &&
> +	} else if ((irqstatus >= get_msg_obj_tx_first(priv)) &&
>  			(irqstatus <= C_CAN_MSG_OBJ_TX_LAST)) {
>  		/* handle events corresponding to transmit message objects */
>  		c_can_do_tx(dev);
> @@ -1146,17 +1161,18 @@ static int c_can_close(struct net_device *dev)
>  	return 0;
>  }
>  
> -struct net_device *alloc_c_can_dev(void)
> +struct net_device *alloc_c_can_dev(unsigned int tx_num)
>  {
>  	struct net_device *dev;
>  	struct c_can_priv *priv;
>  
> -	dev = alloc_candev(sizeof(struct c_can_priv), C_CAN_MSG_OBJ_TX_NUM);
> +	dev = alloc_candev(sizeof(struct c_can_priv), tx_num);
>  	if (!dev)
>  		return NULL;
>  
>  	priv = netdev_priv(dev);
> -	netif_napi_add(dev, &priv->napi, c_can_poll, C_CAN_NAPI_WEIGHT);
> +	netif_napi_add(dev, &priv->napi, c_can_poll, \
> +			(C_CAN_NO_OF_OBJECTS - tx_num));
>  
>  	priv->dev = dev;
>  	priv->can.bittiming_const = &c_can_bittiming_const;
> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> index d2e1c21..d5cd5d2 100644
> --- a/drivers/net/can/c_can/c_can.h
> +++ b/drivers/net/can/c_can/c_can.h
> @@ -150,12 +150,26 @@ enum c_can_dev_id {
>  	BOSCH_D_CAN,
>  };
>  
> +/**
> + * struct c_can_platform_data - Depending on the underlying platform,
> + *                             the message object configuration
> + *                             for c_can controller can change
> + * @rx_split: defines the split point for the lower and upper Rx msg
> + *            object buckets.
> + * @tx_num: number of objs kept aside for TX purposes
> + */
> +struct c_can_platform_data {
> +	unsigned int rx_split;
> +	unsigned int tx_num;
> +};
> +
>  /* c_can private data structure */
>  struct c_can_priv {
>  	struct can_priv can;	/* must be the first member */
>  	struct napi_struct napi;
>  	struct net_device *dev;
>  	struct device *device;
> +	struct c_can_platform_data *pdata;
>  	int tx_object;
>  	int current_status;
>  	int last_status;
> @@ -174,7 +188,7 @@ struct c_can_priv {
>  	void (*raminit) (const struct c_can_priv *priv, bool enable);
>  };
>  
> -struct net_device *alloc_c_can_dev(void);
> +struct net_device *alloc_c_can_dev(unsigned int echo_count);
>  void free_c_can_dev(struct net_device *dev);
>  int register_c_can_dev(struct net_device *dev);
>  void unregister_c_can_dev(struct net_device *dev);
> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
> index 451e81f..6e8dc56 100644
> --- a/drivers/net/can/c_can/c_can_platform.c
> +++ b/drivers/net/can/c_can/c_can_platform.c
> @@ -106,6 +106,25 @@ static const struct of_device_id c_can_of_table[] = {
>  };
>  MODULE_DEVICE_TABLE(of, c_can_of_table);
>  
> +#ifdef CONFIG_OF
> +static int __devinit c_can_probe_config_dt(struct platform_device *pdev,
> +					struct device_node *np)
> +{
> +	struct c_can_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +
> +	of_property_read_u32(np, "rx_split", &pdata->rx_split);
> +	of_property_read_u32(np, "tx_num", &pdata->tx_num);
> +
> +	return 0;
> +}
> +#else
> +static int __devinit c_can_probe_config_dt(struct platform_device *pdev,
> +		struct device_node *np)
> +{
> +	return -ENOSYS;
> +}
> +#endif
> +
>  static int __devinit c_can_plat_probe(struct platform_device *pdev)
>  {
>  	int ret;
> @@ -114,6 +133,7 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
>  	struct c_can_priv *priv;
>  	const struct of_device_id *match;
>  	const struct platform_device_id *id;
> +	struct c_can_platform_data *pdata = dev_get_platdata(&pdev->dev);
>  	struct pinctrl *pinctrl;
>  	struct resource *mem, *res;
>  	int irq;
> @@ -129,12 +149,36 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
>  		}
>  		id = match->data;
>  
> +		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +		pdev->dev.platform_data = pdata;
> +		ret = c_can_probe_config_dt(pdev, pdev->dev.of_node);
> +
> +		if (ret) {
> +			dev_err(&pdev->dev, "Missing platform data\n");
> +			return -ENOSYS;
> +		} else if (!(pdata->rx_split && pdata->tx_num)) {
> +			dev_info(&pdev->dev,
> +					"invalid plat data, using default\n");
> +			pdata->rx_split = 9;
> +			pdata->tx_num = 16;
> +		}
> +
>  		if (of_property_read_u32(pdev->dev.of_node,
>  				"reg_alignment", &reg_alignment))
>  			dev_warn(&pdev->dev, "Register alignment not provided, \
>  					using 32-bit as default\n");
>  	} else {
>  		id = platform_get_device_id(pdev);
> +
> +		/*
> +		 * get the device specific details like Rx/Tx message object
> +		 * configurations from the platform data.
> +		 */
> +		pdata = pdev->dev.platform_data;
> +		if (!pdata) {
> +			dev_err(&pdev->dev, "platform data is NULL\n");
> +			return -EINVAL;
> +		}
>  	}
>  
>  	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
> @@ -176,7 +220,7 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
>  		reg_alignment = mem->flags;
>  
>  	/* allocate the c_can device */
> -	dev = alloc_c_can_dev();
> +	dev = alloc_c_can_dev(pdata->tx_num);
>  	if (!dev) {
>  		ret = -ENOMEM;
>  		goto exit_iounmap;
> @@ -186,6 +230,7 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
>  	switch (id->driver_data) {
>  	case BOSCH_C_CAN:
>  		priv->regs = reg_map_c_can;
> +		priv->pdata = pdata;
>  		switch (reg_alignment & IORESOURCE_MEM_TYPE_MASK) {
>  		case IORESOURCE_MEM_32BIT:
>  			priv->read_reg = c_can_plat_read_reg_aligned_to_32bit;
> 

Re: [PATCH 2/3] can: c_can: Provide generic interface to configure c-can message objects

[Amit Virdi]

On 12/20/2012 3:56 PM, Wolfgang Grandegger wrote:
> On 12/20/2012 11:05 AM, Amit Virdi wrote:
>> Depending on the underlying platform, the configuration of the c-can
>> message objects can change. For e.g. in some systems, it makes more
>> sense to receive many message objects and transmit very few. In any
>> case, providing flexibility in configuring the message objects is highly
>> desirable.
>>
>> The total number of message objects for C_CAN controller is fixed at 32.
>> The receive message objects are assigned higher priority so they begin
>> with message object#1. So, in order to configure the message object the
>> driver just needs two parameters - rx_split (for differentiating between
>> lower bucket and higher bucket) and tx_num.
>
> What is your motivation to make this configurable?
> Why is the current default not good enough?

By default the number of receive message objects is kept equal to the 
number of transmit message objects (16). However, we had some practical 
use cases in which it was expected that the controller would receive 
more number of message objects than it would transmit. The current 
configuration didn't result in an efficient behavior. Maybe, Bhupesh can 
elaborate this point in more detail.

>
>> However, if the user doesn't specify these parameters, then the message
>> objects are configured with default parameters with equal distribution
>> of receive and transmit message objects (16 each).
>>
>> Signed-off-by: Amit Virdi<amit.virdi@st.com>
>> Cc: Bhupesh Sharma<bhupesh.sharma@st.com>
>> Reviewed-by: Shiraz Hashim<shiraz.hashim@st.com>
>> ---
>>   .../devicetree/bindings/net/can/c_can.txt          |  55 ++++++++++--
>>   drivers/net/can/c_can/c_can.c                      | 100 ++++++++++++---------
>>   drivers/net/can/c_can/c_can.h                      |  16 +++-
>>   drivers/net/can/c_can/c_can_platform.c             |  47 +++++++++-
>>   4 files changed, 168 insertions(+), 50 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/can/c_can.txt b/Documentation/devicetree/bindings/net/can/c_can.txt
>> index 7d645cb..4ddd127 100644
>> --- a/Documentation/devicetree/bindings/net/can/c_can.txt
>> +++ b/Documentation/devicetree/bindings/net/can/c_can.txt
>> @@ -8,20 +8,25 @@ Required properties:
>>                          registers map
>>   - interrupts         : property with a value describing the interrupt
>>                          number
>> -
>>   Optional properties:
>> +
>>   - ti,hwmods          : Must be "d_can<n>" or "c_can<n>", n being the
>>                          instance number
>> +- rx_split      : Receive message objects lying in the lower bucket
>> +- tx_num        : Total number of transmit message objects
>>   - reg_alignment : Signifies register alignment. Must be either of
>>                           1. 0x8 or 0x10 - If registers are 16-bit aligned
>>                           2. 0x18 - If registers are 32-bit aligned
>>                   Default is 32-bit alignment.
>
> I would prefer "reg_alignment" in bytes. 0x4 is much more readable than
> 0x18.
>

Ok, that's true. I'll modify it in V2.

[..]

Regards
Amit Virdi

RE: [PATCH 2/3] can: c_can: Provide generic interface to configure c-can message objects

[Bhupesh SHARMA]

> On 12/20/2012 3:56 PM, Wolfgang Grandegger wrote:
> > On 12/20/2012 11:05 AM, Amit Virdi wrote:
> >> Depending on the underlying platform, the configuration of the c-can
> >> message objects can change. For e.g. in some systems, it makes more
> >> sense to receive many message objects and transmit very few. In any
> >> case, providing flexibility in configuring the message objects is
> >> highly desirable.
> >>
> >> The total number of message objects for C_CAN controller is fixed at 32.
> >> The receive message objects are assigned higher priority so they
> >> begin with message object#1. So, in order to configure the message
> >> object the driver just needs two parameters - rx_split (for
> >> differentiating between lower bucket and higher bucket) and tx_num.
> >
> > What is your motivation to make this configurable?
> > Why is the current default not good enough?
> 
> By default the number of receive message objects is kept equal to the
> number of transmit message objects (16). However, we had some practical
> use cases in which it was expected that the controller would receive more
> number of message objects than it would transmit. The current configuration
> didn't result in an efficient behavior. Maybe, Bhupesh can elaborate this
> point in more detail.
> 

Yes. We actually had this requirement from one of our customer whose use-case involved
the SPEAr MPU to be a master controlling a processing line having a number of CAN devices. In such a case
there was a specific requirement to have more Rx objects than Tx ones.

I believe this is a good feature to keep, otherwise we always have the default option of 
having equal msg objects for Tx and Rx purposes (16 each).

Regards,
Bhupesh

Re: [PATCH 2/3] can: c_can: Provide generic interface to configure c-can message objects

[Wolfgang Grandegger]

On 12/20/2012 12:06 PM, Bhupesh SHARMA wrote:
>> On 12/20/2012 3:56 PM, Wolfgang Grandegger wrote:
>>> On 12/20/2012 11:05 AM, Amit Virdi wrote:
>>>> Depending on the underlying platform, the configuration of the c-can
>>>> message objects can change. For e.g. in some systems, it makes more
>>>> sense to receive many message objects and transmit very few. In any
>>>> case, providing flexibility in configuring the message objects is
>>>> highly desirable.
>>>>
>>>> The total number of message objects for C_CAN controller is fixed at 32.
>>>> The receive message objects are assigned higher priority so they
>>>> begin with message object#1. So, in order to configure the message
>>>> object the driver just needs two parameters - rx_split (for
>>>> differentiating between lower bucket and higher bucket) and tx_num.
>>>
>>> What is your motivation to make this configurable?
>>> Why is the current default not good enough?
>>
>> By default the number of receive message objects is kept equal to the
>> number of transmit message objects (16). However, we had some practical
>> use cases in which it was expected that the controller would receive more
>> number of message objects than it would transmit. The current configuration
>> didn't result in an efficient behavior. Maybe, Bhupesh can elaborate this
>> point in more detail.
>>
> 
> Yes. We actually had this requirement from one of our customer whose use-case involved
> the SPEAr MPU to be a master controlling a processing line having a number of CAN devices. In such a case
> there was a specific requirement to have more Rx objects than Tx ones.

OK.

> I believe this is a good feature to keep, otherwise we always have the default option of 
> having equal msg objects for Tx and Rx purposes (16 each).

We need to improve the RX handling anyway similar to Marc's at91_can
driver. There 24 objects are used for RX and 8 for TX.

Wolfgang.

Re: [PATCH 2/3] can: c_can: Provide generic interface to configure c-can message objects

[Amit Virdi]

On 12/20/2012 4:42 PM, Wolfgang Grandegger wrote:
>> I believe this is a good feature to keep, otherwise we always have the default option of
>> >  having equal msg objects for Tx and Rx purposes (16 each).
> We need to improve the RX handling anyway similar to Marc's at91_can
> driver. There 24 objects are used for RX and 8 for TX.

Ok but maybe for the time being we can go ahead with this 
implementation. When Marc's helper library is ready, we can send patches 
over the current implementation to adapt to it.

Regards
Amit Virdi

Re: [PATCH 2/3] can: c_can: Provide generic interface to configure c-can message objects

[Wolfgang Grandegger]

On 12/20/2012 12:18 PM, Amit Virdi wrote:
> On 12/20/2012 4:42 PM, Wolfgang Grandegger wrote:
>>> I believe this is a good feature to keep, otherwise we always have
>>> the default option of
>>> >  having equal msg objects for Tx and Rx purposes (16 each).
>> We need to improve the RX handling anyway similar to Marc's at91_can
>> driver. There 24 objects are used for RX and 8 for TX.
> 
> Ok but maybe for the time being we can go ahead with this
> implementation. When Marc's helper library is ready, we can send patches
> over the current implementation to adapt to it.

This TX handing improvement does not block this patch. The question is
also if the device-tree is the right place for such variables. If they
should be configurable by the user, the device-tree is too static (apart
from the fact that it's not a hardware property). A normal user cannot
change the device-tree.

Wolfgang.

Re: [PATCH 2/3] can: c_can: Provide generic interface to configure c-can message objects

[Marc Kleine-Budde]

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

On 12/20/2012 11:05 AM, Amit Virdi wrote:
> Depending on the underlying platform, the configuration of the c-can
> message objects can change. For e.g. in some systems, it makes more
> sense to receive many message objects and transmit very few. In any
> case, providing flexibility in configuring the message objects is highly
> desirable.
> 
> The total number of message objects for C_CAN controller is fixed at 32.
> The receive message objects are assigned higher priority so they begin
> with message object#1. So, in order to configure the message object the
> driver just needs two parameters - rx_split (for differentiating between
> lower bucket and higher bucket) and tx_num.
> 
> However, if the user doesn't specify these parameters, then the message
> objects are configured with default parameters with equal distribution
> of receive and transmit message objects (16 each).

As Wolfgang pointed out, the DT should only describe the hardware. In
the ethernet world I think there is ethtool to configure the hardware.
Maybe we need something similar for CAN (or add CAN support to ethtool).

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: 261 bytes --]

Re: [PATCH 2/3] can: c_can: Provide generic interface to configure c-can message objects

[Wolfgang Grandegger]

On 12/20/2012 03:04 PM, Marc Kleine-Budde wrote:
> On 12/20/2012 11:05 AM, Amit Virdi wrote:
>> Depending on the underlying platform, the configuration of the c-can
>> message objects can change. For e.g. in some systems, it makes more
>> sense to receive many message objects and transmit very few. In any
>> case, providing flexibility in configuring the message objects is highly
>> desirable.
>>
>> The total number of message objects for C_CAN controller is fixed at 32.
>> The receive message objects are assigned higher priority so they begin
>> with message object#1. So, in order to configure the message object the
>> driver just needs two parameters - rx_split (for differentiating between
>> lower bucket and higher bucket) and tx_num.
>>
>> However, if the user doesn't specify these parameters, then the message
>> objects are configured with default parameters with equal distribution
>> of receive and transmit message objects (16 each).
> 
> As Wolfgang pointed out, the DT should only describe the hardware. In
> the ethernet world I think there is ethtool to configure the hardware.
> Maybe we need something similar for CAN (or add CAN support to ethtool).

Yeah, a simple tool using simple ioctl request would be nice, indeed.
Well, as a per device setting seems overkill a simple module parameter
would just do it.

Wolfgang.

Re: [PATCH 2/3] can: c_can: Provide generic interface to configure c-can message objects

[Amit Virdi]

On 12/21/2012 1:43 AM, Wolfgang Grandegger wrote:
> On 12/20/2012 03:04 PM, Marc Kleine-Budde wrote:
>> On 12/20/2012 11:05 AM, Amit Virdi wrote:
>>> Depending on the underlying platform, the configuration of the c-can
>>> message objects can change. For e.g. in some systems, it makes more
>>> sense to receive many message objects and transmit very few. In any
>>> case, providing flexibility in configuring the message objects is highly
>>> desirable.
>>>
>>> The total number of message objects for C_CAN controller is fixed at 32.
>>> The receive message objects are assigned higher priority so they begin
>>> with message object#1. So, in order to configure the message object the
>>> driver just needs two parameters - rx_split (for differentiating between
>>> lower bucket and higher bucket) and tx_num.
>>>
>>> However, if the user doesn't specify these parameters, then the message
>>> objects are configured with default parameters with equal distribution
>>> of receive and transmit message objects (16 each).
>>
>> As Wolfgang pointed out, the DT should only describe the hardware. In
>> the ethernet world I think there is ethtool to configure the hardware.
>> Maybe we need something similar for CAN (or add CAN support to ethtool).
>
> Yeah, a simple tool using simple ioctl request would be nice, indeed.
> Well, as a per device setting seems overkill a simple module parameter
> would just do it.
>

So, you're saying to drop the device configuration from the DT and use 
some tool to do that. This would be nice if there are enough devices, 
apart from the Bosch C-CAN controller, which require this sort of 
configuration.

Regards
Amit Virdi

RE: [PATCH 2/3] can: c_can: Provide generic interface to configure c-can message objects

[Bhupesh SHARMA]

> -----Original Message-----
> From: Amit VIRDI
> Sent: Wednesday, January 16, 2013 1:15 PM
> To: Wolfgang Grandegger
> Cc: Marc Kleine-Budde; linux-can@vger.kernel.org; Bhupesh SHARMA;
> anilkumar@ti.com; spear-devel
> Subject: Re: [PATCH 2/3] can: c_can: Provide generic interface to configure c-
> can message objects
> 
> On 12/21/2012 1:43 AM, Wolfgang Grandegger wrote:
> > On 12/20/2012 03:04 PM, Marc Kleine-Budde wrote:
> >> On 12/20/2012 11:05 AM, Amit Virdi wrote:
> >>> Depending on the underlying platform, the configuration of the c-can
> >>> message objects can change. For e.g. in some systems, it makes more
> >>> sense to receive many message objects and transmit very few. In any
> >>> case, providing flexibility in configuring the message objects is
> >>> highly desirable.
> >>>
> >>> The total number of message objects for C_CAN controller is fixed at 32.
> >>> The receive message objects are assigned higher priority so they
> >>> begin with message object#1. So, in order to configure the message
> >>> object the driver just needs two parameters - rx_split (for
> >>> differentiating between lower bucket and higher bucket) and tx_num.
> >>>
> >>> However, if the user doesn't specify these parameters, then the
> >>> message objects are configured with default parameters with equal
> >>> distribution of receive and transmit message objects (16 each).
> >>
> >> As Wolfgang pointed out, the DT should only describe the hardware. In
> >> the ethernet world I think there is ethtool to configure the hardware.
> >> Maybe we need something similar for CAN (or add CAN support to
> ethtool).
> >
> > Yeah, a simple tool using simple ioctl request would be nice, indeed.
> > Well, as a per device setting seems overkill a simple module parameter
> > would just do it.
> >
> 
> So, you're saying to drop the device configuration from the DT and use some
> tool to do that. This would be nice if there are enough devices, apart from
> the Bosch C-CAN controller, which require this sort of configuration.
> 

Indeed. It would make sense to put this type of configuration in the IPROUTE2 tool so that such
changes can be performed by the user himself using the IP tool. But with the other CAN controllers having
different management techniques (some using Message Objects, while the others using FIFO based designs),
it would be difficult to maintain a generic interface in IP tool for the same.

Or am I missing something here? Please let us know your views on the same.

Regards,
Bhupesh

Re: [PATCH 2/3] can: c_can: Provide generic interface to configure c-can message objects

[Wolfgang Grandegger]

On 01/16/2013 09:25 AM, Bhupesh SHARMA wrote:
> 
> 
>> -----Original Message-----
>> From: Amit VIRDI
>> Sent: Wednesday, January 16, 2013 1:15 PM
>> To: Wolfgang Grandegger
>> Cc: Marc Kleine-Budde; linux-can@vger.kernel.org; Bhupesh SHARMA;
>> anilkumar@ti.com; spear-devel
>> Subject: Re: [PATCH 2/3] can: c_can: Provide generic interface to configure c-
>> can message objects
>>
>> On 12/21/2012 1:43 AM, Wolfgang Grandegger wrote:
>>> On 12/20/2012 03:04 PM, Marc Kleine-Budde wrote:
>>>> On 12/20/2012 11:05 AM, Amit Virdi wrote:
>>>>> Depending on the underlying platform, the configuration of the c-can
>>>>> message objects can change. For e.g. in some systems, it makes more
>>>>> sense to receive many message objects and transmit very few. In any
>>>>> case, providing flexibility in configuring the message objects is
>>>>> highly desirable.
>>>>>
>>>>> The total number of message objects for C_CAN controller is fixed at 32.
>>>>> The receive message objects are assigned higher priority so they
>>>>> begin with message object#1. So, in order to configure the message
>>>>> object the driver just needs two parameters - rx_split (for
>>>>> differentiating between lower bucket and higher bucket) and tx_num.
>>>>>
>>>>> However, if the user doesn't specify these parameters, then the
>>>>> message objects are configured with default parameters with equal
>>>>> distribution of receive and transmit message objects (16 each).
>>>>
>>>> As Wolfgang pointed out, the DT should only describe the hardware. In
>>>> the ethernet world I think there is ethtool to configure the hardware.
>>>> Maybe we need something similar for CAN (or add CAN support to
>> ethtool).
>>>
>>> Yeah, a simple tool using simple ioctl request would be nice, indeed.
>>> Well, as a per device setting seems overkill a simple module parameter
>>> would just do it.
>>>
>>
>> So, you're saying to drop the device configuration from the DT and use some
>> tool to do that. This would be nice if there are enough devices, apart from
>> the Bosch C-CAN controller, which require this sort of configuration.
>>
> 
> Indeed. It would make sense to put this type of configuration in the IPROUTE2 tool so that such
> changes can be performed by the user himself using the IP tool. But with the other CAN controllers having
> different management techniques (some using Message Objects, while the others using FIFO based designs),
> it would be difficult to maintain a generic interface in IP tool for the same.

Yep. Using iproute2 for this purpose is definitely overkill. It should
be used for generic *per-device* CAN configuration.

> 
> Or am I missing something here? Please let us know your views on the same.

Why not using a simple module parameter as I already suggested. A good
default would make 99% of the user happy. The remaining 1% is free to
fiddle with these module parameters.

Wolfgang.

Re: [PATCH 2/3] can: c_can: Provide generic interface to configure c-can message objects

[Wolfgang Grandegger]

On 01/16/2013 08:45 AM, Amit Virdi wrote:
> On 12/21/2012 1:43 AM, Wolfgang Grandegger wrote:
>> On 12/20/2012 03:04 PM, Marc Kleine-Budde wrote:
>>> On 12/20/2012 11:05 AM, Amit Virdi wrote:
>>>> Depending on the underlying platform, the configuration of the c-can
>>>> message objects can change. For e.g. in some systems, it makes more
>>>> sense to receive many message objects and transmit very few. In any
>>>> case, providing flexibility in configuring the message objects is
>>>> highly
>>>> desirable.
>>>>
>>>> The total number of message objects for C_CAN controller is fixed at
>>>> 32.
>>>> The receive message objects are assigned higher priority so they begin
>>>> with message object#1. So, in order to configure the message object the
>>>> driver just needs two parameters - rx_split (for differentiating
>>>> between
>>>> lower bucket and higher bucket) and tx_num.
>>>>
>>>> However, if the user doesn't specify these parameters, then the message
>>>> objects are configured with default parameters with equal distribution
>>>> of receive and transmit message objects (16 each).
>>>
>>> As Wolfgang pointed out, the DT should only describe the hardware. In
>>> the ethernet world I think there is ethtool to configure the hardware.
>>> Maybe we need something similar for CAN (or add CAN support to ethtool).
>>
>> Yeah, a simple tool using simple ioctl request would be nice, indeed.
>> Well, as a per device setting seems overkill a simple module parameter
>> would just do it.
>>
> 
> So, you're saying to drop the device configuration from the DT and use
> some tool to do that. This would be nice if there are enough devices,
> apart from the Bosch C-CAN controller, which require this sort of
> configuration.

No, I said: "a simple module parameter would just do it".

Wolfgang.

Re: [PATCH 2/3] can: c_can: Provide generic interface to configure c-can message objects

[Amit Virdi]

On 1/16/2013 2:33 PM, Wolfgang Grandegger wrote:
> On 01/16/2013 08:45 AM, Amit Virdi wrote:
>> On 12/21/2012 1:43 AM, Wolfgang Grandegger wrote:
>>> On 12/20/2012 03:04 PM, Marc Kleine-Budde wrote:
>>>> On 12/20/2012 11:05 AM, Amit Virdi wrote:
>>>>> Depending on the underlying platform, the configuration of the c-can
>>>>> message objects can change. For e.g. in some systems, it makes more
>>>>> sense to receive many message objects and transmit very few. In any
>>>>> case, providing flexibility in configuring the message objects is
>>>>> highly
>>>>> desirable.
>>>>>
>>>>> The total number of message objects for C_CAN controller is fixed at
>>>>> 32.
>>>>> The receive message objects are assigned higher priority so they begin
>>>>> with message object#1. So, in order to configure the message object the
>>>>> driver just needs two parameters - rx_split (for differentiating
>>>>> between
>>>>> lower bucket and higher bucket) and tx_num.
>>>>>
>>>>> However, if the user doesn't specify these parameters, then the message
>>>>> objects are configured with default parameters with equal distribution
>>>>> of receive and transmit message objects (16 each).
>>>>
>>>> As Wolfgang pointed out, the DT should only describe the hardware. In
>>>> the ethernet world I think there is ethtool to configure the hardware.
>>>> Maybe we need something similar for CAN (or add CAN support to ethtool).
>>>
>>> Yeah, a simple tool using simple ioctl request would be nice, indeed.
>>> Well, as a per device setting seems overkill a simple module parameter
>>> would just do it.
>>>
>>
>> So, you're saying to drop the device configuration from the DT and use
>> some tool to do that. This would be nice if there are enough devices,
>> apart from the Bosch C-CAN controller, which require this sort of
>> configuration.
>
> No, I said: "a simple module parameter would just do it".
>

Ok, now I understood!

Regards
Amit Virdi

[PATCH 3/3] can: c_can: Enable clock before first use

[Amit Virdi]

Current implementation assumes clock to be always enabled. Instead,
explicitly enable device clock before usage.

Signed-off-by: Amit Virdi <amit.virdi@st.com>
Reviewed-by: Shiraz Hashim <shiraz.hashim@st.com>
---
 drivers/net/can/c_can/c_can_platform.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
index 6e8dc56..d0bd6e6 100644
--- a/drivers/net/can/c_can/c_can_platform.c
+++ b/drivers/net/can/c_can/c_can_platform.c
@@ -194,6 +194,12 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
 		goto exit;
 	}
 
+	ret = clk_prepare_enable(clk);
+	if (ret) {
+		dev_err(&pdev->dev, "could not prepare CAN clock\n");
+		goto exit_no_clk_en;
+	}
+
 	/* get the platform data */
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	irq = platform_get_irq(pdev, 0);
@@ -295,6 +301,8 @@ exit_iounmap:
 exit_release_mem:
 	release_mem_region(mem->start, resource_size(mem));
 exit_free_clk:
+	clk_disable_unprepare(clk);
+exit_no_clk_en:
 	clk_put(clk);
 exit:
 	dev_err(&pdev->dev, "probe failed\n");
-- 
1.8.0

Re: [PATCH 3/3] can: c_can: Enable clock before first use

[Marc Kleine-Budde]

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

On 12/20/2012 11:05 AM, Amit Virdi wrote:
> Current implementation assumes clock to be always enabled. Instead,
> explicitly enable device clock before usage.
> 
> Signed-off-by: Amit Virdi <amit.virdi@st.com>
> Reviewed-by: Shiraz Hashim <shiraz.hashim@st.com>
> ---
>  drivers/net/can/c_can/c_can_platform.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
> index 6e8dc56..d0bd6e6 100644
> --- a/drivers/net/can/c_can/c_can_platform.c
> +++ b/drivers/net/can/c_can/c_can_platform.c
> @@ -194,6 +194,12 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
>  		goto exit;
>  	}
>  
> +	ret = clk_prepare_enable(clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "could not prepare CAN clock\n");
> +		goto exit_no_clk_en;
> +	}
> +
>  	/* get the platform data */
>  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	irq = platform_get_irq(pdev, 0);
> @@ -295,6 +301,8 @@ exit_iounmap:
>  exit_release_mem:
>  	release_mem_region(mem->start, resource_size(mem));
>  exit_free_clk:
> +	clk_disable_unprepare(clk);
> +exit_no_clk_en:
>  	clk_put(clk);
>  exit:
>  	dev_err(&pdev->dev, "probe failed\n");

Why do you have to enable the clock if the driver is loaded? What about
disabling the clock on driver unload. The clock should be enabled on
open and disabled on close if the network interface. If you need to
access the registers of your hardware, you should enable the clock
before accessing the regs and disable the clock when finished.

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: 261 bytes --]

Re: [PATCH 3/3] can: c_can: Enable clock before first use

[Amit Virdi]

On 12/20/2012 7:37 PM, Marc Kleine-Budde wrote:
> On 12/20/2012 11:05 AM, Amit Virdi wrote:
>> Current implementation assumes clock to be always enabled. Instead,
>> explicitly enable device clock before usage.
>>
>> Signed-off-by: Amit Virdi<amit.virdi@st.com>
>> Reviewed-by: Shiraz Hashim<shiraz.hashim@st.com>
>> ---
>>   drivers/net/can/c_can/c_can_platform.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
>> index 6e8dc56..d0bd6e6 100644
>> --- a/drivers/net/can/c_can/c_can_platform.c
>> +++ b/drivers/net/can/c_can/c_can_platform.c
>> @@ -194,6 +194,12 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
>>   		goto exit;
>>   	}
>>
>> +	ret = clk_prepare_enable(clk);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "could not prepare CAN clock\n");
>> +		goto exit_no_clk_en;
>> +	}
>> +
>>   	/* get the platform data */
>>   	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>   	irq = platform_get_irq(pdev, 0);
>> @@ -295,6 +301,8 @@ exit_iounmap:
>>   exit_release_mem:
>>   	release_mem_region(mem->start, resource_size(mem));
>>   exit_free_clk:
>> +	clk_disable_unprepare(clk);
>> +exit_no_clk_en:
>>   	clk_put(clk);
>>   exit:
>>   	dev_err(&pdev->dev, "probe failed\n");
>
> Why do you have to enable the clock if the driver is loaded? What about
> disabling the clock on driver unload. The clock should be enabled on
> open and disabled on close if the network interface. If you need to
> access the registers of your hardware, you should enable the clock
> before accessing the regs and disable the clock when finished.
>

Okay, I would take care of it in V2.

Regards
Amit Virdi

RE: [PATCH 3/3] can: c_can: Enable clock before first use

[AnilKumar, Chimata]

+Rafael +arm list

On Wed, Jan 16, 2013 at 13:33:27, Amit Virdi wrote:
> On 12/20/2012 7:37 PM, Marc Kleine-Budde wrote:
> > On 12/20/2012 11:05 AM, Amit Virdi wrote:
> >> Current implementation assumes clock to be always enabled. Instead,
> >> explicitly enable device clock before usage.
> >>
> >> Signed-off-by: Amit Virdi<amit.virdi@st.com>
> >> Reviewed-by: Shiraz Hashim<shiraz.hashim@st.com>
> >> ---
> >>   drivers/net/can/c_can/c_can_platform.c | 8 ++++++++
> >>   1 file changed, 8 insertions(+)
> >>
> >> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
> >> index 6e8dc56..d0bd6e6 100644
> >> --- a/drivers/net/can/c_can/c_can_platform.c
> >> +++ b/drivers/net/can/c_can/c_can_platform.c
> >> @@ -194,6 +194,12 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
> >>   		goto exit;
> >>   	}
> >>
> >> +	ret = clk_prepare_enable(clk);
> >> +	if (ret) {
> >> +		dev_err(&pdev->dev, "could not prepare CAN clock\n");
> >> +		goto exit_no_clk_en;
> >> +	}
> >> +
> >>   	/* get the platform data */
> >>   	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >>   	irq = platform_get_irq(pdev, 0);
> >> @@ -295,6 +301,8 @@ exit_iounmap:
> >>   exit_release_mem:
> >>   	release_mem_region(mem->start, resource_size(mem));
> >>   exit_free_clk:
> >> +	clk_disable_unprepare(clk);
> >> +exit_no_clk_en:
> >>   	clk_put(clk);
> >>   exit:
> >>   	dev_err(&pdev->dev, "probe failed\n");
> >
> > Why do you have to enable the clock if the driver is loaded? What about
> > disabling the clock on driver unload. The clock should be enabled on
> > open and disabled on close if the network interface. If you need to
> > access the registers of your hardware, you should enable the clock
> > before accessing the regs and disable the clock when finished.
> >
> 
> Okay, I would take care of it in V2.

Hi Amit,

As Marc said clock enable/disable should be in sync with open/
close functions. I think pm_runtime calls should take care of
clock control. In your case you have to create a wrapper which
exports runtime pm hooks, which should internally handle the
clock enable/disable.

If we add clock specific calls to the driver, then driver might
break on the platforms which has runtime pm implemented because
runtime pm support to the c_can driver is already added.

Rafael,

Do you have any suggestions here? Am I missing anything here?

Thanks & Regards
AnilKumar

Re: [PATCH 3/3] can: c_can: Enable clock before first use

[Rafael J. Wysocki]

On Wednesday, January 16, 2013 09:54:38 AM AnilKumar, Chimata wrote:
> +Rafael +arm list
> 
> On Wed, Jan 16, 2013 at 13:33:27, Amit Virdi wrote:
> > On 12/20/2012 7:37 PM, Marc Kleine-Budde wrote:
> > > On 12/20/2012 11:05 AM, Amit Virdi wrote:
> > >> Current implementation assumes clock to be always enabled. Instead,
> > >> explicitly enable device clock before usage.
> > >>
> > >> Signed-off-by: Amit Virdi<amit.virdi@st.com>
> > >> Reviewed-by: Shiraz Hashim<shiraz.hashim@st.com>
> > >> ---
> > >>   drivers/net/can/c_can/c_can_platform.c | 8 ++++++++
> > >>   1 file changed, 8 insertions(+)
> > >>
> > >> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
> > >> index 6e8dc56..d0bd6e6 100644
> > >> --- a/drivers/net/can/c_can/c_can_platform.c
> > >> +++ b/drivers/net/can/c_can/c_can_platform.c
> > >> @@ -194,6 +194,12 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
> > >>   		goto exit;
> > >>   	}
> > >>
> > >> +	ret = clk_prepare_enable(clk);
> > >> +	if (ret) {
> > >> +		dev_err(&pdev->dev, "could not prepare CAN clock\n");
> > >> +		goto exit_no_clk_en;
> > >> +	}
> > >> +
> > >>   	/* get the platform data */
> > >>   	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >>   	irq = platform_get_irq(pdev, 0);
> > >> @@ -295,6 +301,8 @@ exit_iounmap:
> > >>   exit_release_mem:
> > >>   	release_mem_region(mem->start, resource_size(mem));
> > >>   exit_free_clk:
> > >> +	clk_disable_unprepare(clk);
> > >> +exit_no_clk_en:
> > >>   	clk_put(clk);
> > >>   exit:
> > >>   	dev_err(&pdev->dev, "probe failed\n");
> > >
> > > Why do you have to enable the clock if the driver is loaded? What about
> > > disabling the clock on driver unload. The clock should be enabled on
> > > open and disabled on close if the network interface. If you need to
> > > access the registers of your hardware, you should enable the clock
> > > before accessing the regs and disable the clock when finished.
> > >
> > 
> > Okay, I would take care of it in V2.
> 
> Hi Amit,
> 
> As Marc said clock enable/disable should be in sync with open/
> close functions. I think pm_runtime calls should take care of
> clock control. In your case you have to create a wrapper which
> exports runtime pm hooks, which should internally handle the
> clock enable/disable.
> 
> If we add clock specific calls to the driver, then driver might
> break on the platforms which has runtime pm implemented because
> runtime pm support to the c_can driver is already added.
> 
> Rafael,
> 
> Do you have any suggestions here? Am I missing anything here?

No, you aren't.

Clock-specific hooks in the driver (outside of its PM callbacks) may not work
or worse yet may cause problems to happen on platforms where PM is handled
via ACPI, for example.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/3] c_can: Update can support

[Marc Kleine-Budde]

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

On 12/20/2012 11:05 AM, Amit Virdi wrote:
> This patchset aims to improve the Bosch c_can driver support. The changes are
> as a result of problems encountered using the c_can driver on SPEAr platform.
> 
> Although significant care has been taken to ensure that the changes don't break
> the existing functionality, however there might be some hiccups on other platforms.
> 
> The patch to provide generic interface is provided to add more flexibility for
> the end applications.
> 
> The patches have been tested on SPEAr1310 EVB and the patchset rebased on master
> of http://git.gitorious.org/linux-can/linux-can-next.git

I'm on vacation already. I'll have a look at the patches in about 2 weeks.

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: 261 bytes --]