[PATCH v4] flexcan: add err_irq handler for flexcan

[PATCH v4] flexcan: add err_irq handler for flexcan

[Zhao Qiang]

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(&regs->esr);
+	reg_ctrl = flexcan_read(&regs->ctrl);
+
+	if (reg_esr & FLEXCAN_ESR_ALL_INT) {
+		if (reg_esr & FLEXCAN_ESR_ERR_INT)
+			flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK,
+				      &regs->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;
-- 
1.8.5

Re: [PATCH v4] flexcan: add err_irq handler for flexcan

[Marc Kleine-Budde]

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

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(&regs->esr);
> +	reg_ctrl = flexcan_read(&regs->ctrl);
> +
> +	if (reg_esr & FLEXCAN_ESR_ALL_INT) {
> +		if (reg_esr & FLEXCAN_ESR_ERR_INT)
> +			flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK,
> +				      &regs->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

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

RE: [PATCH v4] flexcan: add err_irq handler for flexcan

[qiang.zhao]

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(&regs->esr);
> > +	reg_ctrl = flexcan_read(&regs->ctrl);
> > +
> > +	if (reg_esr & FLEXCAN_ESR_ALL_INT) {
> > +		if (reg_esr & FLEXCAN_ESR_ERR_INT)
> > +			flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK,
> > +				      &regs->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   |

Re: [PATCH v4] flexcan: add err_irq handler for flexcan

[Marc Kleine-Budde]

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

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(&regs->esr);
>>> +	reg_ctrl = flexcan_read(&regs->ctrl);
>>> +
>>> +	if (reg_esr & FLEXCAN_ESR_ALL_INT) {
>>> +		if (reg_esr & FLEXCAN_ESR_ERR_INT)
>>> +			flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK,
>>> +				      &regs->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

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

RE: [PATCH v4] flexcan: add err_irq handler for flexcan

[qiang.zhao]

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(&regs->esr);
> >>> +	reg_ctrl = flexcan_read(&regs->ctrl);
> >>> +
> >>> +	if (reg_esr & FLEXCAN_ESR_ALL_INT) {
> >>> +		if (reg_esr & FLEXCAN_ESR_ERR_INT)
> >>> +			flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK,
> >>> +				      &regs->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   |

Re: [PATCH v4] flexcan: add err_irq handler for flexcan

[Marc Kleine-Budde]

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

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(&regs->esr);
>>>>> +	reg_ctrl = flexcan_read(&regs->ctrl);
>>>>> +
>>>>> +	if (reg_esr & FLEXCAN_ESR_ALL_INT) {
>>>>> +		if (reg_esr & FLEXCAN_ESR_ERR_INT)
>>>>> +			flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK,
>>>>> +				      &regs->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
-- 
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: 242 bytes --]