Patchwork [v2,1/3] can: at91_can: clean up usage of AT91_MB_RX_FIRST and AT91_MB_RX_NUM

login
register
mail settings
Submitter Marc Kleine-Budde
Date Jan. 11, 2011, 1:21 p.m.
Message ID <1294752085-30151-2-git-send-email-mkl@pengutronix.de>
Download mbox | patch
Permalink /patch/78382/
State Superseded
Delegated to: David Miller
Headers show

Comments

Marc Kleine-Budde - Jan. 11, 2011, 1:21 p.m.
This patch cleans up the usage of two macros which specify the mailbox
usage. AT91_MB_RX_FIRST and AT91_MB_RX_NUM define the first and the
number of RX mailboxes. The current driver uses these variables in an
unclean way; assuming that AT91_MB_RX_FIRST is 0;

This patch cleans up the usage of these macros, no longer assuming
AT91_MB_RX_FIRST == 0.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/at91_can.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)
Marc Kleine-Budde - Jan. 17, 2011, 7:50 p.m.
On 01/11/2011 02:21 PM, Marc Kleine-Budde wrote:
> This patch cleans up the usage of two macros which specify the mailbox
> usage. AT91_MB_RX_FIRST and AT91_MB_RX_NUM define the first and the
> number of RX mailboxes. The current driver uses these variables in an
> unclean way; assuming that AT91_MB_RX_FIRST is 0;
> 
> This patch cleans up the usage of these macros, no longer assuming
> AT91_MB_RX_FIRST == 0.
> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

Any comments on this?

Marc
Wolfgang Grandegger - Jan. 18, 2011, 9:26 a.m.
On 01/17/2011 08:50 PM, Marc Kleine-Budde wrote:
> On 01/11/2011 02:21 PM, Marc Kleine-Budde wrote:
>> This patch cleans up the usage of two macros which specify the mailbox
>> usage. AT91_MB_RX_FIRST and AT91_MB_RX_NUM define the first and the
>> number of RX mailboxes. The current driver uses these variables in an
>> unclean way; assuming that AT91_MB_RX_FIRST is 0;
>>
>> This patch cleans up the usage of these macros, no longer assuming
>> AT91_MB_RX_FIRST == 0.
>>
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> 
> Any comments on this?

Looks fine too me. You can add my:

Acked-by: Wolfgang Grandegger <wg@grandegger.com>

Thanks,

Wolfgang.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Jan. 22, 2011, 12:54 a.m.
From: Marc Kleine-Budde <mkl@pengutronix.de>
Date: Mon, 17 Jan 2011 20:50:36 +0100

> On 01/11/2011 02:21 PM, Marc Kleine-Budde wrote:
>> This patch cleans up the usage of two macros which specify the mailbox
>> usage. AT91_MB_RX_FIRST and AT91_MB_RX_NUM define the first and the
>> number of RX mailboxes. The current driver uses these variables in an
>> unclean way; assuming that AT91_MB_RX_FIRST is 0;
>> 
>> This patch cleans up the usage of these macros, no longer assuming
>> AT91_MB_RX_FIRST == 0.
>> 
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> 
> Any comments on this?

I would also seriously like to see these changes get some feedback,
they've been rotting in patchwork for more than a week.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfgang Grandegger - Jan. 22, 2011, 7:14 a.m.
On 01/22/2011 01:54 AM, David Miller wrote:
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Date: Mon, 17 Jan 2011 20:50:36 +0100
> 
>> On 01/11/2011 02:21 PM, Marc Kleine-Budde wrote:
>>> This patch cleans up the usage of two macros which specify the mailbox
>>> usage. AT91_MB_RX_FIRST and AT91_MB_RX_NUM define the first and the
>>> number of RX mailboxes. The current driver uses these variables in an
>>> unclean way; assuming that AT91_MB_RX_FIRST is 0;
>>>
>>> This patch cleans up the usage of these macros, no longer assuming
>>> AT91_MB_RX_FIRST == 0.
>>>
>>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>>
>> Any comments on this?
> 
> I would also seriously like to see these changes get some feedback,
> they've been rotting in patchwork for more than a week.

V2 was OK and it got my "Acked-by" here:

http://marc.info/?l=linux-netdev&m=129534267002747&w=2

Wolfgang.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kurt Van Dijck - Jan. 22, 2011, 7:50 a.m.
On Fri, Jan 21, 2011 at 04:54:09PM -0800, David Miller wrote:
> > On 01/11/2011 02:21 PM, Marc Kleine-Budde wrote:
> >> This patch cleans up the usage of two macros which specify the mailbox
> >> usage. AT91_MB_RX_FIRST and AT91_MB_RX_NUM define the first and the
> >> number of RX mailboxes. The current driver uses these variables in an
> >> unclean way; assuming that AT91_MB_RX_FIRST is 0;
> >> 
> >> This patch cleans up the usage of these macros, no longer assuming
> >> AT91_MB_RX_FIRST == 0.
> >> 
> >> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > 
> > Any comments on this?
> 
> I would also seriously like to see these changes get some feedback,
> they've been rotting in patchwork for more than a week.

I have no experience with this specific chip.
IMO, this chip errata (as explained in the post) got an elegant solution.
That part definitely gets my
Acked-by: Kurt Van Dijck <kurt.van.dijck@eia.be>

Regards,
Kurt
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marc Kleine-Budde - Jan. 24, 2011, 12:44 p.m.
On 01/22/2011 01:54 AM, David Miller wrote:
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Date: Mon, 17 Jan 2011 20:50:36 +0100
> 
>> On 01/11/2011 02:21 PM, Marc Kleine-Budde wrote:
>>> This patch cleans up the usage of two macros which specify the mailbox
>>> usage. AT91_MB_RX_FIRST and AT91_MB_RX_NUM define the first and the
>>> number of RX mailboxes. The current driver uses these variables in an
>>> unclean way; assuming that AT91_MB_RX_FIRST is 0;
>>>
>>> This patch cleans up the usage of these macros, no longer assuming
>>> AT91_MB_RX_FIRST == 0.
>>>
>>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>>
>> Any comments on this?
> 
> I would also seriously like to see these changes get some feedback,
> they've been rotting in patchwork for more than a week.

I've just talked to our customer (the one who noticed the problem in the
first place). They have the patched driver running over the weekend
without problems.

I'm going to resend the series the the Acks.

regards, Marc

Patch

diff --git a/drivers/net/can/at91_can.c b/drivers/net/can/at91_can.c
index 7ef83d0..892c3d8 100644
--- a/drivers/net/can/at91_can.c
+++ b/drivers/net/can/at91_can.c
@@ -2,7 +2,7 @@ 
  * at91_can.c - CAN network driver for AT91 SoC CAN controller
  *
  * (C) 2007 by Hans J. Koch <hjk@hansjkoch.de>
- * (C) 2008, 2009, 2010 by Marc Kleine-Budde <kernel@pengutronix.de>
+ * (C) 2008, 2009, 2010, 2011 by Marc Kleine-Budde <kernel@pengutronix.de>
  *
  * This software may be distributed under the terms of the GNU General
  * Public License ("GPL") version 2 as distributed in the 'COPYING'
@@ -55,7 +55,8 @@ 
 #define AT91_MB_RX_MASK(i)	((1 << (i)) - 1)
 #define AT91_MB_RX_SPLIT	8
 #define AT91_MB_RX_LOW_LAST	(AT91_MB_RX_SPLIT - 1)
-#define AT91_MB_RX_LOW_MASK	(AT91_MB_RX_MASK(AT91_MB_RX_SPLIT))
+#define AT91_MB_RX_LOW_MASK	(AT91_MB_RX_MASK(AT91_MB_RX_SPLIT) & \
+				 ~AT91_MB_RX_MASK(AT91_MB_RX_FIRST))
 
 #define AT91_MB_TX_NUM		(1 << AT91_MB_TX_SHIFT)
 #define AT91_MB_TX_FIRST	(AT91_MB_RX_LAST + 1)
@@ -254,7 +255,8 @@  static void at91_setup_mailboxes(struct net_device *dev)
 		set_mb_mode_prio(priv, i, AT91_MB_MODE_TX, 0);
 
 	/* Reset tx and rx helper pointers */
-	priv->tx_next = priv->tx_echo = priv->rx_next = 0;
+	priv->tx_next = priv->tx_echo = 0;
+	priv->rx_next = AT91_MB_RX_FIRST;
 }
 
 static int at91_set_bittiming(struct net_device *dev)
@@ -590,10 +592,10 @@  static int at91_poll_rx(struct net_device *dev, int quota)
 			"order of incoming frames cannot be guaranteed\n");
 
  again:
-	for (mb = find_next_bit(addr, AT91_MB_RX_NUM, priv->rx_next);
-	     mb < AT91_MB_RX_NUM && quota > 0;
+	for (mb = find_next_bit(addr, AT91_MB_RX_LAST + 1, priv->rx_next);
+	     mb < AT91_MB_RX_LAST + 1 && quota > 0;
 	     reg_sr = at91_read(priv, AT91_SR),
-	     mb = find_next_bit(addr, AT91_MB_RX_NUM, ++priv->rx_next)) {
+	     mb = find_next_bit(addr, AT91_MB_RX_LAST + 1, ++priv->rx_next)) {
 		at91_read_msg(dev, mb);
 
 		/* reactivate mailboxes */
@@ -610,8 +612,8 @@  static int at91_poll_rx(struct net_device *dev, int quota)
 
 	/* upper group completed, look again in lower */
 	if (priv->rx_next > AT91_MB_RX_LOW_LAST &&
-	    quota > 0 && mb >= AT91_MB_RX_NUM) {
-		priv->rx_next = 0;
+	    quota > 0 && mb > AT91_MB_RX_LAST) {
+		priv->rx_next = AT91_MB_RX_FIRST;
 		goto again;
 	}