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: Friday, September 01, 2017 11:15 PM
> 
> [...]
> >> 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.
> 
> You patches are also white-space mangled.
Perhaps because of the outlook. Although I've tested send to myself and
copy the text then use "git am" to apply, it works in my local, but
seems can not work when it send out. :(

Can we send the updated patches in separate e-mails, with attachments
instead of inline text? Because we cannot easily switch to other
mail client...

Or, do you have other proposal?

> 
> [...]
> > 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"
> 
> Typo, sorry, I mean "(reg_esr & FLEXCAN_ESR_ERR_BUS)".
Sorry I made another typo too. :)

> 
> > 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)
> 
> The "FLEXCAN_QUIRK_BROKEN_PERR_STATE" is for the i.MX6. Only thinking
> about the core here.
> 
> > 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)
> 
> I was oping that the error warning interrupt occurs when the error
> counter falls again below error passive. Then we could enable the error
> interrupt on "active -> waning" and disable it on "passive -> warning"
> state changes. If that does work, than we can disable the error
> interrupt not before the "warning -> active" change.
Disable error interrupt on "passive -> warning" cannot handle the case:
1. Disconnect the cable, until error passive reported
2. Connect the cable, and wait error warning reported
3. Disconnect the cable while it is still error warning
Due to the error interrupt is disabled, so it cannot report error
passive again.

> 
> > 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.
> 
> Yes, there is just a little overhead.
> 
> > 2. If flexcan_irq_state() is called when the state is changed,
> >     then, that's what help us to report correct state transitions.
> 
> The only difference between "new core" and "old core":
> 
> new core: error interrupt enabled between error warning and passive.
> old core: error interrupt enabled between error active and passive.
> 
> Not sure it it's worth the extra code.
Yes, it is worth. :) Please see the test result in 
https://marc.info/?l=linux-can&m=150424626206586&w=2, where we can
optimize for the new core further.

Best regards
Yi