Re: [PATCH v3 4/4] can: flexcan: Consolidate and unify state change handling

[Andri Yngvason <andri.yngvason@marel.com>]

Quoting Wolfgang Grandegger (2014-11-25 21:03:24)
> On 09/26/2014 07:31 PM, Andri Yngvason wrote:
> > Replacing error state change handling with the new mechanism.
> > 
> > Changes made since last proposal:
> > can: flexcan: improve rx/tx state recognition.
> 
> Move below "---", please.
> 
> > Note: Flexcan is problematic because there is no interrupt generated when
> > transitioning between error-active and error-passive. If the bus is
> > disconnected, there is no event that will trigger a napi poll, so the new state
> > will not be discovered until the bus is connected. This problem goes away if
> > berr-reporting is enabled, because then the ack-error takes care of it.
> 
> Well, this is a known issue and should be addressed via platform data
> (using the FLEXCAN_HAS_BROKEN_ERR_STATE flags).
>
I thought that was about something else.
> 
> > 
> > Another problem is that if nothing is happening on the bus, when it's connected
> > again, we won't see the new state until someone transmits something on the bus.
> > 
> > I'd say that these are minor issues because a "healthy" bus has traffic and the
> > "disconnected bus" issue has a work-around.
> 
> Would be nice to do some tests on a more recent Flexcan core as well.
> 
Isn't imx6 fairly recent?
>
> > 
> > Signed-off-by: Andri Yngvason <andri.yngvason@marel.com>
> > ---
> >  drivers/net/can/flexcan.c | 101 +++++++++-------------------------------------
> >  1 file changed, 18 insertions(+), 83 deletions(-)
> > 
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index 60f86bd..e7dea64 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> > @@ -577,98 +577,30 @@ static int flexcan_poll_bus_err(struct net_device *dev, u32 reg_esr)
> >       return 1;
> >  }
> >  
> > -static void do_state(struct net_device *dev,
> > -                  struct can_frame *cf, enum can_state new_state)
> > -{
> > -     struct flexcan_priv *priv = netdev_priv(dev);
> > -     struct can_berr_counter bec;
> > -
> > -     __flexcan_get_berr_counter(dev, &bec);
> > -
> > -     switch (priv->can.state) {
> > -     case CAN_STATE_ERROR_ACTIVE:
> > -             /*
> > -              * from: ERROR_ACTIVE
> > -              * to  : ERROR_WARNING, ERROR_PASSIVE, BUS_OFF
> > -              * =>  : there was a warning int
> > -              */
> > -             if (new_state >= CAN_STATE_ERROR_WARNING &&
> > -                 new_state <= CAN_STATE_BUS_OFF) {
> > -                     netdev_dbg(dev, "Error Warning IRQ\n");
> > -                     priv->can.can_stats.error_warning++;
> > -
> > -                     cf->can_id |= CAN_ERR_CRTL;
> > -                     cf->data[1] = (bec.txerr > bec.rxerr) ?
> > -                             CAN_ERR_CRTL_TX_WARNING :
> > -                             CAN_ERR_CRTL_RX_WARNING;
> > -             }
> > -     case CAN_STATE_ERROR_WARNING:   /* fallthrough */
> > -             /*
> > -              * from: ERROR_ACTIVE, ERROR_WARNING
> > -              * to  : ERROR_PASSIVE, BUS_OFF
> > -              * =>  : error passive int
> > -              */
> > -             if (new_state >= CAN_STATE_ERROR_PASSIVE &&
> > -                 new_state <= CAN_STATE_BUS_OFF) {
> > -                     netdev_dbg(dev, "Error Passive IRQ\n");
> > -                     priv->can.can_stats.error_passive++;
> > -
> > -                     cf->can_id |= CAN_ERR_CRTL;
> > -                     cf->data[1] = (bec.txerr > bec.rxerr) ?
> > -                             CAN_ERR_CRTL_TX_PASSIVE :
> > -                             CAN_ERR_CRTL_RX_PASSIVE;
> > -             }
> > -             break;
> > -     case CAN_STATE_BUS_OFF:
> > -             netdev_err(dev, "BUG! "
> > -                        "hardware recovered automatically from BUS_OFF\n");
> > -             break;
> > -     default:
> > -             break;
> > -     }
> > -
> > -     /* process state changes depending on the new state */
> > -     switch (new_state) {
> > -     case CAN_STATE_ERROR_WARNING:
> > -             netdev_dbg(dev, "Error Warning\n");
> > -             cf->can_id |= CAN_ERR_CRTL;
> > -             cf->data[1] = (bec.txerr > bec.rxerr) ?
> > -                     CAN_ERR_CRTL_TX_WARNING :
> > -                     CAN_ERR_CRTL_RX_WARNING;
> > -             break;
> > -     case CAN_STATE_ERROR_ACTIVE:
> > -             netdev_dbg(dev, "Error Active\n");
> > -             cf->can_id |= CAN_ERR_PROT;
> > -             cf->data[2] = CAN_ERR_PROT_ACTIVE;
> > -             break;
> > -     case CAN_STATE_BUS_OFF:
> > -             cf->can_id |= CAN_ERR_BUSOFF;
> > -             can_bus_off(dev);
> > -             break;
> > -     default:
> > -             break;
> > -     }
> > -}
> > -
> >  static int flexcan_poll_state(struct net_device *dev, u32 reg_esr)
> >  {
> >       struct flexcan_priv *priv = netdev_priv(dev);
> >       struct sk_buff *skb;
> >       struct can_frame *cf;
> > -     enum can_state new_state;
> > +     enum can_state new_state = 0, rx_state = 0, tx_state = 0;
> >       int flt;
> > +     struct can_berr_counter bec;
> >  
> >       flt = reg_esr & FLEXCAN_ESR_FLT_CONF_MASK;
> >       if (likely(flt == FLEXCAN_ESR_FLT_CONF_ACTIVE)) {
> > -             if (likely(!(reg_esr & (FLEXCAN_ESR_TX_WRN |
> > -                                     FLEXCAN_ESR_RX_WRN))))
> > -                     new_state = CAN_STATE_ERROR_ACTIVE;
> > -             else
> > -                     new_state = CAN_STATE_ERROR_WARNING;
> > -     } else if (unlikely(flt == FLEXCAN_ESR_FLT_CONF_PASSIVE))
> > +             tx_state = unlikely(reg_esr & FLEXCAN_ESR_TX_WRN) ?
> > +                        CAN_STATE_ERROR_WARNING : CAN_STATE_ERROR_ACTIVE;
> > +             rx_state = unlikely(reg_esr & FLEXCAN_ESR_RX_WRN) ?
> > +                        CAN_STATE_ERROR_WARNING : CAN_STATE_ERROR_ACTIVE;
> > +             new_state = max(tx_state, rx_state);
> > +     } else if (unlikely(flt == FLEXCAN_ESR_FLT_CONF_PASSIVE)) {
> > +             __flexcan_get_berr_counter(dev, &bec);
> >               new_state = CAN_STATE_ERROR_PASSIVE;
> > -     else
> > +             rx_state = bec.rxerr >= bec.txerr ? new_state : 0;
> > +             tx_state = bec.rxerr <= bec.txerr ? new_state : 0;
> > +     } else {
> >               new_state = CAN_STATE_BUS_OFF;
> > +     }
> >  
> >       /* state hasn't changed */
> >       if (likely(new_state == priv->can.state))
> > @@ -678,8 +610,11 @@ static int flexcan_poll_state(struct net_device *dev, u32 reg_esr)
> >       if (unlikely(!skb))
> >               return 0;
> >  
> > -     do_state(dev, cf, new_state);
> > -     priv->can.state = new_state;
> 
> On SJA1000 and MSCAN you use:
> 
>         if (new_state != priv->can.state) ...
>
That's because they had that before. I was trying not to make more changes than
were needed.

Also, this makes sense with regard to not feeding the frame if there was no
change.
>
> > +     can_change_state(dev, cf, new_state, tx_state, rx_state);
> > +
> > +     if (unlikely(new_state == CAN_STATE_BUS_OFF))
> > +             can_bus_off(dev);
> > +
> >       netif_receive_skb(skb);
> >  
> >       dev->stats.rx_packets++;
> >