RE: [PATCH 1/6] can: flexcan: fix state transition regression

["ZHU Yi (ST-FIR/ENG1-Zhu)" <Yi.Zhu5@cn.bosch.com>]

Hello Wolfgang,

> From: Wolfgang Grandegger [mailto:wg@grandegger.com]
> Sent: Thursday, August 31, 2017 7:37 PM
> 
> Hello ZHU Yi,
> 
> usually a [PATCH 0/x] is coming first telling what the series does. We
> use "git format-patch --cover-letter ..." for that purpose.
OK. Thanks for your hint!
I'll prepare the updated patch series with cover letter later.

> 
> Am 31.08.2017 um 10:24 schrieb ZHU Yi (ST-FIR/ENG1-Zhu):
> [...]
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index 13f0f21..df4bfb8 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> > @@ -765,8 +765,9 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
> >   		flexcan_write(reg_esr & FLEXCAN_ESR_ALL_INT, &regs->esr);
> >   	}
> >
> > -	/* state change interrupt */
> > -	if (reg_esr & FLEXCAN_ESR_ERR_STATE)
> > +	/* state change interrupt or broken error state quirk fix is enabled */
> > +	if ((reg_esr & FLEXCAN_ESR_ERR_STATE) ||
> > +	    (priv->devtype_data->quirks &
> FLEXCAN_QUIRK_BROKEN_ERR_STATE))
> >   		flexcan_irq_state(dev, reg_esr);
> 
> This means that flexcan_irq_state() is called for any interrupt source.
> That's what we had in the pre rx-offload version. Thinking about it:
> do we need that (overhead for RX and TX)? Maybe the following works already:
> 
>  +	if ((reg_esr & FLEXCAN_ESR_ERR_STATE) ||
>  +	    ((priv->devtype_data->quirks & FLEXCAN_QUIRK_BROKEN_ERR_STATE) &&
>  +		(reg_esr & FLEXCAN_ESR_ERR_STATE)))
I'm not sure that I understood your proposal correctly, the guess is
the second "reg_esr & FLEXCAN_ESR_ERR_STATE" is a typo, do you mean
"reg_esr & FLEXCAN_ESR_BUS_ERR"?

If we only call flexcan_irq_state() upon state + error interrupt, then:
(new core: with    [TR]WRN_INT, e.g., i.MX6,
 old core: without [TR]WRN_INT, e.g., i.MX53)
1. error state increases, enough for both new and old cores.
    active ---> warning ---> passive ---> bus off
                   ^            ^            ^
                WRN_INT/     ERR_INT      BOFF_INT
	        ERR_INT(old core)

2. error state decreases, not enough for both new and old cores.
    active <--- warning <--- passive
       ^           ^
    [TR]X       WRN_INT/
	        [TR]X(old core)
The [TR]X interrupt is required for the state decreases, and the
overhead of them IMHO is not overkill:
1. If flexcan_irq_state() is called when the state is not change,
   then, the overhead is read FLT_CONF and/or read error counter,
   do O(1) calculation and compare the state, then quit.

2. If flexcan_irq_state() is called when the state is changed,
   then, that's what help us to report correct state transitions.

Of course, if flexcan core can generate appropriate state interrupts
in its next version (FlexCAN4?), then we don't need such overhead
at all, but when will this wish come true? :)

Best regards
Yi