| Message ID | 1404201809-7010-1-git-send-email-B45475@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 07/01/2014 10:03 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 > Changes for v4: > - call flexcan_irq > > drivers/net/can/flexcan.c | 38 +++++++++++++++++++++++++++++++++++++- > 1 file changed, 37 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c > index f425ec2..098fcac 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; > + int err_irq; > > struct clk *clk_ipg; > struct clk *clk_per; > @@ -744,6 +745,27 @@ 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_ALL_INT) { > + if (reg_esr & FLEXCAN_ESR_ERR_INT) > + flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK, > + ®s->ctrl); > + flexcan_irq(irq, dev); I still don't understand why you need a special flexcan_err_irq() function. Why don't you just call: request_irq(priv->err_irq, flexcan_irq, IRQF_SHARED, dev->name, dev); instead? > + 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 +966,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); Why do you want to free the err_irq, if requesting the err_irq fails? However you should adjust all error cleanup path following this request irq, so that it will be freed. BTW: where is the corresponding free_irq() in flexcan_close()? > + goto out_free_irq; > + } > + } > + > /* start chip and queuing */ > err = flexcan_chip_start(dev); > if (err) > @@ -1099,7 +1130,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 +1157,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 +1184,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; > Marc
On 07/01/2014 06:04 PM, Marc Kleine-Budde wrote: > -----Original Message----- > From: Marc Kleine-Budde [mailto:mkl@pengutronix.de] > Sent: Tuesday, July 01, 2014 6:04 PM > To: Zhao Qiang-B45475; linuxppc-dev@lists.ozlabs.org; wg@grandegger.com; > linux-can@vger.kernel.org; Wood Scott-B07421 > Subject: Re: [PATCH v4] flexcan: add err_irq handler for flexcan > > On 07/01/2014 10:03 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 Changes > for > > v4: > > - call flexcan_irq > > > > drivers/net/can/flexcan.c | 38 +++++++++++++++++++++++++++++++++++++- > > 1 file changed, 37 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c > > index f425ec2..098fcac 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; > > + int err_irq; > > > > struct clk *clk_ipg; > > struct clk *clk_per; > > @@ -744,6 +745,27 @@ 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_ALL_INT) { > > + if (reg_esr & FLEXCAN_ESR_ERR_INT) > > + flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK, > > + ®s->ctrl); > > + flexcan_irq(irq, dev); > > I still don't understand why you need a special flexcan_err_irq() > function. Why don't you just call: > > request_irq(priv->err_irq, flexcan_irq, IRQF_SHARED, dev->name, dev); > > instead? Flexcan_irq is for flexcan normal interrupt(such as Message Buffer, Wake up and so on). And it will return IRQ_HANDLED if flexcan_irq is triggered. But err_irq is shared with other devices, it should return IRQ_HANDLED when the interrupt is triggered by flexcan device, if not return IRQ_NONE. > > > + 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 > > +966,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); > > Why do you want to free the err_irq, if requesting the err_irq fails? > However you should adjust all error cleanup path following this request > irq, so that it will be freed. > > BTW: where is the corresponding free_irq() in flexcan_close()? Here, I think you are right. Thanks for your reminder. I will modify it. > > > + goto out_free_irq; > > + } > > + } > > + > > /* start chip and queuing */ > > err = flexcan_chip_start(dev); > > if (err) > > @@ -1099,7 +1130,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 +1157,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 +1184,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; > > > > 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 07/02/2014 04:00 AM, qiang.zhao@freescale.com wrote: >>> +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_ALL_INT) { >>> + if (reg_esr & FLEXCAN_ESR_ERR_INT) >>> + flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK, >>> + ®s->ctrl); >>> + flexcan_irq(irq, dev); >> >> I still don't understand why you need a special flexcan_err_irq() >> function. Why don't you just call: >> >> request_irq(priv->err_irq, flexcan_irq, IRQF_SHARED, dev->name, dev); >> >> instead? > > Flexcan_irq is for flexcan normal interrupt(such as Message Buffer, Wake up and so on). > And it will return IRQ_HANDLED if flexcan_irq is triggered. > But err_irq is shared with other devices, it should return IRQ_HANDLED when the interrupt > is triggered by flexcan device, if not return IRQ_NONE. What about fixing flexcan_irq() first and the make use of it? Marc
On 07/02/2014 03:04 PM, Marc Kleine-Budde wrote: > -----Original Message----- > From: Marc Kleine-Budde [mailto:mkl@pengutronix.de] > Sent: Wednesday, July 02, 2014 3:04 PM > To: Zhao Qiang-B45475; linuxppc-dev@lists.ozlabs.org; wg@grandegger.com; > linux-can@vger.kernel.org; Wood Scott-B07421 > Subject: Re: [PATCH v4] flexcan: add err_irq handler for flexcan > > On 07/02/2014 04:00 AM, qiang.zhao@freescale.com wrote: > >>> +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_ALL_INT) { > >>> + if (reg_esr & FLEXCAN_ESR_ERR_INT) > >>> + flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK, > >>> + ®s->ctrl); > >>> + flexcan_irq(irq, dev); > >> > >> I still don't understand why you need a special flexcan_err_irq() > >> function. Why don't you just call: > >> > >> request_irq(priv->err_irq, flexcan_irq, IRQF_SHARED, dev->name, dev); > >> > >> instead? > > > > Flexcan_irq is for flexcan normal interrupt(such as Message Buffer, > Wake up and so on). > > And it will return IRQ_HANDLED if flexcan_irq is triggered. > > But err_irq is shared with other devices, it should return IRQ_HANDLED > > when the interrupt is triggered by flexcan device, if not return > IRQ_NONE. > > What about fixing flexcan_irq() first and the make use of it? Err_irq is a shared interrupt with other device, I hope that its handler is independent. However, if you persist in your opinion, I will do it as you said. > > 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 07/02/2014 10:32 AM, qiang.zhao@freescale.com wrote: > On 07/02/2014 03:04 PM, Marc Kleine-Budde wrote: >> -----Original Message----- >> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de] >> Sent: Wednesday, July 02, 2014 3:04 PM >> To: Zhao Qiang-B45475; linuxppc-dev@lists.ozlabs.org; wg@grandegger.com; >> linux-can@vger.kernel.org; Wood Scott-B07421 >> Subject: Re: [PATCH v4] flexcan: add err_irq handler for flexcan >> >> On 07/02/2014 04:00 AM, qiang.zhao@freescale.com wrote: >>>>> +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_ALL_INT) { >>>>> + if (reg_esr & FLEXCAN_ESR_ERR_INT) >>>>> + flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK, >>>>> + ®s->ctrl); >>>>> + flexcan_irq(irq, dev); >>>> >>>> I still don't understand why you need a special flexcan_err_irq() >>>> function. Why don't you just call: >>>> >>>> request_irq(priv->err_irq, flexcan_irq, IRQF_SHARED, dev->name, dev); >>>> >>>> instead? >>> >>> Flexcan_irq is for flexcan normal interrupt(such as Message Buffer, >> Wake up and so on). >>> And it will return IRQ_HANDLED if flexcan_irq is triggered. >>> But err_irq is shared with other devices, it should return IRQ_HANDLED >>> when the interrupt is triggered by flexcan device, if not return >> IRQ_NONE. >> >> What about fixing flexcan_irq() first and the make use of it? > > Err_irq is a shared interrupt with other device, > I hope that its handler is independent. > However, if you persist in your opinion, I will do it as you said. There is another option, you can move all of the error interrupt handling code from flexcan_irq() to flexcan_irq_err(). To keep the ARM SoCs supported, you need to call flexcan_irq_err() form the flexcan_irq() handler. What I don't want is a) code duplication and b) no fishy wrapper functions around flexcan_irq() that work around flexcan_irq() returning IRQ_HANDLED unconditionally. Marc
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c index f425ec2..098fcac 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; + int err_irq; struct clk *clk_ipg; struct clk *clk_per; @@ -744,6 +745,27 @@ 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_ALL_INT) { + if (reg_esr & FLEXCAN_ESR_ERR_INT) + flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK, + ®s->ctrl); + flexcan_irq(irq, 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 +966,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 +1130,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 +1157,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 +1184,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 Changes for v4: - call flexcan_irq drivers/net/can/flexcan.c | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-)