| Message ID | 1403507484-46547-1-git-send-email-B45475@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 06/23/2014 09:11 AM, Zhao Qiang wrote: > when flexcan is not physically linked, command 'cantest' will > trigger an err_irq, add err_irq handler for it. > > Signed-off-by: Zhao Qiang <B45475@freescale.com> > --- > Changes for v2: > - use a space instead of tab > - use flexcan_poll_state instead of print > Changes for v3: > - return IRQ_HANDLED if err is triggered > - stop transmitted packets when there is an err_interrupt > > drivers/net/can/flexcan.c | 35 ++++++++++++++++++++++++++++++++++- > 1 file changed, 34 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c > index f425ec2..6802a25 100644 > --- a/drivers/net/can/flexcan.c > +++ b/drivers/net/can/flexcan.c > @@ -208,6 +208,7 @@ struct flexcan_priv { > void __iomem *base; > u32 reg_esr; > u32 reg_ctrl_default; > + unsigned int err_irq; > > struct clk *clk_ipg; > struct clk *clk_per; > @@ -744,6 +745,24 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id) > return IRQ_HANDLED; > } > > +static irqreturn_t flexcan_err_irq(int irq, void *dev_id) > +{ > + struct net_device *dev = dev_id; > + struct flexcan_priv *priv = netdev_priv(dev); > + struct flexcan_regs __iomem *regs = priv->base; > + u32 reg_ctrl, reg_esr; > + > + reg_esr = flexcan_read(®s->esr); > + reg_ctrl = flexcan_read(®s->ctrl); > + if (reg_esr & FLEXCAN_ESR_TX_WRN) { When does the hardware trigger the interrupt? > + flexcan_write(reg_esr & ~FLEXCAN_ESR_TX_WRN, ®s->esr); > + flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK, ®s->ctrl); > + netif_stop_queue(dev); Why are you stopping the txqueue? > + return IRQ_HANDLED; > + } > + return IRQ_NONE; > +} > + Marc
On 06/23/2014 03:18 PM, Marc Kleine-Budde wrote: > > On 06/23/2014 09:11 AM, Zhao Qiang wrote: > > when flexcan is not physically linked, command 'cantest' will trigger > > an err_irq, add err_irq handler for it. > > > > Signed-off-by: Zhao Qiang <B45475@freescale.com> > > --- > > Changes for v2: > > - use a space instead of tab > > - use flexcan_poll_state instead of print Changes for v3: > > - return IRQ_HANDLED if err is triggered > > - stop transmitted packets when there is an err_interrupt > > > > drivers/net/can/flexcan.c | 35 ++++++++++++++++++++++++++++++++++- > > 1 file changed, 34 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c > > index f425ec2..6802a25 100644 > > --- a/drivers/net/can/flexcan.c > > +++ b/drivers/net/can/flexcan.c > > @@ -208,6 +208,7 @@ struct flexcan_priv { > > void __iomem *base; > > u32 reg_esr; > > u32 reg_ctrl_default; > > + unsigned int err_irq; > > > > struct clk *clk_ipg; > > struct clk *clk_per; > > @@ -744,6 +745,24 @@ static irqreturn_t flexcan_irq(int irq, void > *dev_id) > > return IRQ_HANDLED; > > } > > > > +static irqreturn_t flexcan_err_irq(int irq, void *dev_id) { > > + struct net_device *dev = dev_id; > > + struct flexcan_priv *priv = netdev_priv(dev); > > + struct flexcan_regs __iomem *regs = priv->base; > > + u32 reg_ctrl, reg_esr; > > + > > + reg_esr = flexcan_read(®s->esr); > > + reg_ctrl = flexcan_read(®s->ctrl); > > + if (reg_esr & FLEXCAN_ESR_TX_WRN) { > > When does the hardware trigger the interrupt? When there is no wire link between tx and rx, tx start transfer and doesn’t get the ack. > > > + flexcan_write(reg_esr & ~FLEXCAN_ESR_TX_WRN, ®s->esr); > > + flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK, ®s->ctrl); > > + netif_stop_queue(dev); > > Why are you stopping the txqueue? There is no wire link, tx can't transfer successfully. > > > + return IRQ_HANDLED; > > + } > > + return IRQ_NONE; > > +} > > + > > 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 |
On 06/23/2014 09:26 AM, qiang.zhao@freescale.com wrote: > > On 06/23/2014 03:18 PM, Marc Kleine-Budde wrote: > >> >> On 06/23/2014 09:11 AM, Zhao Qiang wrote: >>> when flexcan is not physically linked, command 'cantest' will trigger >>> an err_irq, add err_irq handler for it. >>> >>> Signed-off-by: Zhao Qiang <B45475@freescale.com> >>> --- >>> Changes for v2: >>> - use a space instead of tab >>> - use flexcan_poll_state instead of print Changes for v3: >>> - return IRQ_HANDLED if err is triggered >>> - stop transmitted packets when there is an err_interrupt >>> >>> drivers/net/can/flexcan.c | 35 ++++++++++++++++++++++++++++++++++- >>> 1 file changed, 34 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c >>> index f425ec2..6802a25 100644 >>> --- a/drivers/net/can/flexcan.c >>> +++ b/drivers/net/can/flexcan.c >>> @@ -208,6 +208,7 @@ struct flexcan_priv { >>> void __iomem *base; >>> u32 reg_esr; >>> u32 reg_ctrl_default; >>> + unsigned int err_irq; >>> >>> struct clk *clk_ipg; >>> struct clk *clk_per; >>> @@ -744,6 +745,24 @@ static irqreturn_t flexcan_irq(int irq, void >> *dev_id) >>> return IRQ_HANDLED; >>> } >>> >>> +static irqreturn_t flexcan_err_irq(int irq, void *dev_id) { >>> + struct net_device *dev = dev_id; >>> + struct flexcan_priv *priv = netdev_priv(dev); >>> + struct flexcan_regs __iomem *regs = priv->base; >>> + u32 reg_ctrl, reg_esr; >>> + >>> + reg_esr = flexcan_read(®s->esr); >>> + reg_ctrl = flexcan_read(®s->ctrl); >>> + if (reg_esr & FLEXCAN_ESR_TX_WRN) { >> >> When does the hardware trigger the interrupt? > > When there is no wire link between tx and rx, tx start transfer and doesn’t get the ack. You are testing for the warning interrupt, not for the FLEXCAN_ESR_ACK_ERR (which is triggered there isn't any ACK). >>> + flexcan_write(reg_esr & ~FLEXCAN_ESR_TX_WRN, ®s->esr); >>> + flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK, ®s->ctrl); >>> + netif_stop_queue(dev); >> >> Why are you stopping the txqueue? > > There is no wire link, tx can't transfer successfully. You are testing for the warning interrupt, which is triggered if the error counter increases from 95 to 96. And the error counter can increase due to several reasons. No link is only one of them. If the CAN core cannot transmit new packages any more the flow control in the driver will take care. What about calling the normal interrupt if er err_irq occurs, as this function will take care of both normal and error interrupts anyway? Marc
On 06/23/2014 03:37 PM, Marc Kleine-Budde wrote: > -----Original Message----- > From: Marc Kleine-Budde [mailto:mkl@pengutronix.de] > Sent: Monday, June 23, 2014 3:37 PM > To: Zhao Qiang-B45475; linuxppc-dev@lists.ozlabs.org; linux- > can@vger.kernel.org; wg@grandegger.com; Wood Scott-B07421 > Subject: Re: [PATCH v3 1/2] flexcan: add err_irq handler for flexcan > > On 06/23/2014 09:26 AM, qiang.zhao@freescale.com wrote: > > > > On 06/23/2014 03:18 PM, Marc Kleine-Budde wrote: > > > >> > >> On 06/23/2014 09:11 AM, Zhao Qiang wrote: > >>> when flexcan is not physically linked, command 'cantest' will > >>> trigger an err_irq, add err_irq handler for it. > >>> > >>> Signed-off-by: Zhao Qiang <B45475@freescale.com> > >>> --- > >>> Changes for v2: > >>> - use a space instead of tab > >>> - use flexcan_poll_state instead of print Changes for v3: > >>> - return IRQ_HANDLED if err is triggered > >>> - stop transmitted packets when there is an err_interrupt > >>> > >>> drivers/net/can/flexcan.c | 35 ++++++++++++++++++++++++++++++++++- > >>> 1 file changed, 34 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c > >>> index f425ec2..6802a25 100644 > >>> --- a/drivers/net/can/flexcan.c > >>> +++ b/drivers/net/can/flexcan.c > >>> @@ -208,6 +208,7 @@ struct flexcan_priv { > >>> void __iomem *base; > >>> u32 reg_esr; > >>> u32 reg_ctrl_default; > >>> + unsigned int err_irq; > >>> > >>> struct clk *clk_ipg; > >>> struct clk *clk_per; > >>> @@ -744,6 +745,24 @@ static irqreturn_t flexcan_irq(int irq, void > >> *dev_id) > >>> return IRQ_HANDLED; > >>> } > >>> > >>> +static irqreturn_t flexcan_err_irq(int irq, void *dev_id) { > >>> + struct net_device *dev = dev_id; > >>> + struct flexcan_priv *priv = netdev_priv(dev); > >>> + struct flexcan_regs __iomem *regs = priv->base; > >>> + u32 reg_ctrl, reg_esr; > >>> + > >>> + reg_esr = flexcan_read(®s->esr); > >>> + reg_ctrl = flexcan_read(®s->ctrl); > >>> + if (reg_esr & FLEXCAN_ESR_TX_WRN) { > >> > >> When does the hardware trigger the interrupt? > > > > When there is no wire link between tx and rx, tx start transfer and > doesn’t get the ack. > > You are testing for the warning interrupt, not for the > FLEXCAN_ESR_ACK_ERR (which is triggered there isn't any ACK). > > >>> + flexcan_write(reg_esr & ~FLEXCAN_ESR_TX_WRN, ®s->esr); > >>> + flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK, ®s->ctrl); > >>> + netif_stop_queue(dev); > >> > >> Why are you stopping the txqueue? > > > > There is no wire link, tx can't transfer successfully. > > You are testing for the warning interrupt, which is triggered if the > error counter increases from 95 to 96. And the error counter can increase > due to several reasons. No link is only one of them. If the CAN core > cannot transmit new packages any more the flow control in the driver will > take care. When Tx error counter increases from 95 to 96, there must be issue for tx, So why can't I stop the txqueue? You said that there are several reasons, would you like to take some examples? > > What about calling the normal interrupt if er err_irq occurs, as this > function will take care of both normal and error interrupts anyway? Calling the normal interrupt doesn't work. > > 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 |
On 06/23/2014 10:15 AM, qiang.zhao@freescale.com wrote: [...] >>>>> + reg_esr = flexcan_read(®s->esr); >>>>> + reg_ctrl = flexcan_read(®s->ctrl); >>>>> + if (reg_esr & FLEXCAN_ESR_TX_WRN) { >>>> >>>> When does the hardware trigger the interrupt? >>> >>> When there is no wire link between tx and rx, tx start transfer and >> doesn’t get the ack. >> >> You are testing for the warning interrupt, not for the >> FLEXCAN_ESR_ACK_ERR (which is triggered there isn't any ACK). >> >>>>> + flexcan_write(reg_esr & ~FLEXCAN_ESR_TX_WRN, ®s->esr); >>>>> + flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK, ®s->ctrl); >>>>> + netif_stop_queue(dev); >>>> >>>> Why are you stopping the txqueue? >>> >>> There is no wire link, tx can't transfer successfully. >> >> You are testing for the warning interrupt, which is triggered if the >> error counter increases from 95 to 96. And the error counter can increase >> due to several reasons. No link is only one of them. If the CAN core >> cannot transmit new packages any more the flow control in the driver will >> take care. > > When Tx error counter increases from 95 to 96, there must be issue for tx, > So why can't I stop the txqueue? Why do you want to stop the queue? It's compliant with the CAN spec to keep sending CAN frames if the error counter increases to 96. If there isn't any problem with the CAN bus anymore, the TX error counters will decrease with every successfully transmitted CAN frame. > You said that there are several reasons, would you like to take some examples? See CAN Error Confinement Rules in http://www.can-wiki.info/doku.php?id=can_faq_erors >> What about calling the normal interrupt if er err_irq occurs, as this >> function will take care of both normal and error interrupts anyway? > > Calling the normal interrupt doesn't work. Why? flexcan_irq() if FLEXCAN_CTRL_TWRN_MSK is set and will schedule the NAPI routine: > if ((reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE) || > (reg_esr & FLEXCAN_ESR_ERR_STATE) || > flexcan_has_and_handle_berr(priv, reg_esr)) { [...] > napi_schedule(&priv->napi); > } Marc
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c index f425ec2..6802a25 100644 --- a/drivers/net/can/flexcan.c +++ b/drivers/net/can/flexcan.c @@ -208,6 +208,7 @@ struct flexcan_priv { void __iomem *base; u32 reg_esr; u32 reg_ctrl_default; + unsigned int err_irq; struct clk *clk_ipg; struct clk *clk_per; @@ -744,6 +745,24 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id) return IRQ_HANDLED; } +static irqreturn_t flexcan_err_irq(int irq, void *dev_id) +{ + struct net_device *dev = dev_id; + struct flexcan_priv *priv = netdev_priv(dev); + struct flexcan_regs __iomem *regs = priv->base; + u32 reg_ctrl, reg_esr; + + reg_esr = flexcan_read(®s->esr); + reg_ctrl = flexcan_read(®s->ctrl); + if (reg_esr & FLEXCAN_ESR_TX_WRN) { + flexcan_write(reg_esr & ~FLEXCAN_ESR_TX_WRN, ®s->esr); + flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK, ®s->ctrl); + netif_stop_queue(dev); + return IRQ_HANDLED; + } + return IRQ_NONE; +} + static void flexcan_set_bittiming(struct net_device *dev) { const struct flexcan_priv *priv = netdev_priv(dev); @@ -944,6 +963,15 @@ static int flexcan_open(struct net_device *dev) if (err) goto out_close; + if (priv->err_irq) { + err = request_irq(priv->err_irq, flexcan_err_irq, IRQF_SHARED, + dev->name, dev); + if (err) { + free_irq(priv->err_irq, dev); + goto out_free_irq; + } + } + /* start chip and queuing */ err = flexcan_chip_start(dev); if (err) @@ -1099,7 +1127,7 @@ static int flexcan_probe(struct platform_device *pdev) struct resource *mem; struct clk *clk_ipg = NULL, *clk_per = NULL; void __iomem *base; - int err, irq; + int err, irq, err_irq; u32 clock_freq = 0; if (pdev->dev.of_node) @@ -1126,6 +1154,10 @@ static int flexcan_probe(struct platform_device *pdev) if (irq <= 0) return -ENODEV; + err_irq = platform_get_irq(pdev, 1); + if (err_irq <= 0) + err_irq = 0; + base = devm_ioremap_resource(&pdev->dev, mem); if (IS_ERR(base)) return PTR_ERR(base); @@ -1149,6 +1181,7 @@ static int flexcan_probe(struct platform_device *pdev) dev->flags |= IFF_ECHO; priv = netdev_priv(dev); + priv->err_irq = err_irq; priv->can.clock.freq = clock_freq; priv->can.bittiming_const = &flexcan_bittiming_const; priv->can.do_set_mode = flexcan_set_mode;
when flexcan is not physically linked, command 'cantest' will trigger an err_irq, add err_irq handler for it. Signed-off-by: Zhao Qiang <B45475@freescale.com> --- Changes for v2: - use a space instead of tab - use flexcan_poll_state instead of print Changes for v3: - return IRQ_HANDLED if err is triggered - stop transmitted packets when there is an err_interrupt drivers/net/can/flexcan.c | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-)