Message ID | 4F0D4FBA.1080108@essax.com |
---|---|
State | Awaiting Upstream, archived |
Delegated to: | David Miller |
Headers | show |
> @@ -689,12 +680,6 @@ static void cc770_tx_interrupt(struct > net_device *dev, unsigned int o) > /* Nothing more to send, switch off interrupts */ > cc770_write_reg(priv, msgobj[mo].ctrl0, > MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES); > - /* > - * We had some cases of repeated IRQ so make sure the > - * INT is acknowledged > - */ > - cc770_write_reg(priv, msgobj[mo].ctrl0, > - MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES); A wild guess is that this is needed to clear an interrupt that was asserted during the ISR processing. So you need to ack the IRQ as well as mask it. Not sure if the difference between the xxx_UNC and xxx_RES bits though. David -- 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
Hello, On 01/11/2012 10:00 AM, Wolfgang Zarre wrote: >>> Please provide an extra patch for these unrelated changes. If we really >>> want to remove it. >>> >> >> Sure, this I can do. >> > > Ok, here the patch to remove: Looks good, please add a patch description and put in a patch series together with the spinlock patch (see other mail). Marc > -------------------------------------------------------- > diff --git a/drivers/net/can/cc770/cc770.c b/drivers/net/can/cc770/cc770.c > index 2d12f89..dad6707 100644 > --- a/drivers/net/can/cc770/cc770.c > +++ b/drivers/net/can/cc770/cc770.c > @@ -460,15 +460,6 @@ static netdev_tx_t cc770_start_xmit(struct sk_buff > *skb, struct net_device *dev) > > stats->tx_bytes += dlc; > > - > - /* > - * HM: We had some cases of repeated IRQs so make sure the > - * INT is acknowledged I know it's already further up, but > - * doing again fixed the issue > - */ > - cc770_write_reg(priv, msgobj[mo].ctrl0, > - MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES); > - > return NETDEV_TX_OK; > } > > @@ -689,12 +680,6 @@ static void cc770_tx_interrupt(struct net_device > *dev, unsigned int o) > /* Nothing more to send, switch off interrupts */ > cc770_write_reg(priv, msgobj[mo].ctrl0, > MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES); > - /* > - * We had some cases of repeated IRQ so make sure the > - * INT is acknowledged > - */ > - cc770_write_reg(priv, msgobj[mo].ctrl0, > - MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES); > > stats->tx_packets++; > can_get_echo_skb(dev, 0); > ---------------------------------------------------------- Marc
Hello David, > >> @@ -689,12 +680,6 @@ static void cc770_tx_interrupt(struct >> net_device *dev, unsigned int o) >> /* Nothing more to send, switch off interrupts */ >> cc770_write_reg(priv, msgobj[mo].ctrl0, >> MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES); >> - /* >> - * We had some cases of repeated IRQ so make sure the >> - * INT is acknowledged >> - */ >> - cc770_write_reg(priv, msgobj[mo].ctrl0, >> - MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES); > > A wild guess is that this is needed to clear an interrupt > that was asserted during the ISR processing. > So you need to ack the IRQ as well as mask it. I was digging a bit and as far as I understood by studying both manuals of i82527 and cc770 the procedure is in any case most probably not completely clean. From my point of view there would be no need to set and reset TXIE/RXIE after every request because according to the description as below (e.g for TXIE): MSB LSB Meaning Write 0 0 Not allowed (indeterminate) 0 1 reset 1 0 set 1 1 unchanged TXIE Transmit Interrupt Enable one An interrupt will be generated after a successful transmission of a frame zero No interrupt will be generated after a successful transmission of a frame The Transmit Interrupt Enable bit enables the 82527 to initiate an interrupt after the successful transmission by the corresponding message object This bit is written by the CPU In fact with that we are telling the chip if we want to have interrupts or not for a transmission and therefore it would be enough, so far I understood, to set it at _init and switch it of at _deinit and between just using 'unchanged'. Actually to ack the IRQ we would have to set TXOK of the status register to zero after reading a one for a successful transmission but is in fact the chip doesn't care if we reset TXOK or not. However, if time permits I'll try to dig deeper and doing some tests because it would need some changes more. Anyway, I might be wrong as well because sometimes I'm checking under heavy workload the wrong bit's as You know already ;-) More or less the reason for the request of removal was just because assuming that the repeated interrupts were caused as well not having spinlock's and on the other hand if obsolete to save I/O resources which affects mostly smaller systems with heavy load. However, maybe Wolfgang can give an advice. > > Not sure if the difference between the xxx_UNC and xxx_RES > bits though. > > David > > 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
Hello Marc, > Hello, > > On 01/11/2012 10:00 AM, Wolfgang Zarre wrote: >>>> Please provide an extra patch for these unrelated changes. If we really >>>> want to remove it. >>>> >>> >>> Sure, this I can do. >>> >> >> Ok, here the patch to remove: > > Looks good, please add a patch description and put in a patch series > together with the spinlock patch (see other mail). Thanks, also for the additional hints. It might be that I let this patch for now until clarified if we should remove or not. > > Marc > >> -------------------------------------------------------- >> diff --git a/drivers/net/can/cc770/cc770.c b/drivers/net/can/cc770/cc770.c >> index 2d12f89..dad6707 100644 >> --- a/drivers/net/can/cc770/cc770.c >> +++ b/drivers/net/can/cc770/cc770.c >> @@ -460,15 +460,6 @@ static netdev_tx_t cc770_start_xmit(struct sk_buff >> *skb, struct net_device *dev) >> >> stats->tx_bytes += dlc; >> >> - >> - /* >> - * HM: We had some cases of repeated IRQs so make sure the >> - * INT is acknowledged I know it's already further up, but >> - * doing again fixed the issue >> - */ >> - cc770_write_reg(priv, msgobj[mo].ctrl0, >> - MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES); >> - >> return NETDEV_TX_OK; >> } >> >> @@ -689,12 +680,6 @@ static void cc770_tx_interrupt(struct net_device >> *dev, unsigned int o) >> /* Nothing more to send, switch off interrupts */ >> cc770_write_reg(priv, msgobj[mo].ctrl0, >> MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES); >> - /* >> - * We had some cases of repeated IRQ so make sure the >> - * INT is acknowledged >> - */ >> - cc770_write_reg(priv, msgobj[mo].ctrl0, >> - MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES); >> >> stats->tx_packets++; >> can_get_echo_skb(dev, 0); >> ---------------------------------------------------------- > > Marc > 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
On 01/11/2012 03:42 PM, Wolfgang Zarre wrote: > Thanks, also for the additional hints. You're welcome. > It might be that I let this patch for now until clarified if we > should remove or not. Yes, the just prepare the spinlock patch. Marc
diff --git a/drivers/net/can/cc770/cc770.c b/drivers/net/can/cc770/cc770.c index 2d12f89..dad6707 100644 --- a/drivers/net/can/cc770/cc770.c +++ b/drivers/net/can/cc770/cc770.c @@ -460,15 +460,6 @@ static netdev_tx_t cc770_start_xmit(struct sk_buff *skb, struct net_device *dev) stats->tx_bytes += dlc; - - /* - * HM: We had some cases of repeated IRQs so make sure the - * INT is acknowledged I know it's already further up, but - * doing again fixed the issue - */ - cc770_write_reg(priv, msgobj[mo].ctrl0, - MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES); - return NETDEV_TX_OK; } @@ -689,12 +680,6 @@ static void cc770_tx_interrupt(struct net_device *dev, unsigned int o) /* Nothing more to send, switch off interrupts */ cc770_write_reg(priv, msgobj[mo].ctrl0, MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES); - /* - * We had some cases of repeated IRQ so make sure the - * INT is acknowledged - */ - cc770_write_reg(priv, msgobj[mo].ctrl0, - MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES); stats->tx_packets++; can_get_echo_skb(dev, 0);