Marc Kleine-Budde wrote: > Wolfgang Grandegger wrote: >> I realized a few issues. You can add my "acked-by" when they are fixed. > > thanks for the review.
[...]
>>> +static void flexcan_poll_err_frame(struct net_device *dev,
>>> + struct can_frame *cf, u32 reg_esr)
>>> +{
>>> + struct flexcan_priv *priv = netdev_priv(dev);
>>> + int error_warning = 0, rx_errors = 0, tx_errors = 0;
>>> +
>>> + if (reg_esr & FLEXCAN_ESR_BIT1_ERR) {
>>> + rx_errors = 1;
>>> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
>>> + cf->data[2] |= CAN_ERR_PROT_BIT1;
>>> + }
>>> +
>>> + if (reg_esr & FLEXCAN_ESR_BIT0_ERR) {
>>> + rx_errors = 1;
>>> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
>>> + cf->data[2] |= CAN_ERR_PROT_BIT0;
>>> + }
>>> +
>>> + if (reg_esr & FLEXCAN_ESR_FRM_ERR) {
>>> + rx_errors = 1;
>>> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
>>> + cf->data[2] |= CAN_ERR_PROT_FORM;
>>> + }
>>> +
>>> + if (reg_esr & FLEXCAN_ESR_STF_ERR) {
>>> + rx_errors = 1;
>>> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
>>> + cf->data[2] |= CAN_ERR_PROT_STUFF;
>>> + }
>>> +
>>> +
>>> + if (reg_esr & FLEXCAN_ESR_ACK_ERR) {
>>> + tx_errors = 1;
>>> + cf->can_id |= CAN_ERR_ACK;
>> This is a bus-error as well. Therefore I think it should be:
>>
>> if (reg_esr & FLEXCAN_ESR_ACK_ERR) {
>> tx_errors = 1;
>> cf->can_id |= CAN_ERR_ACK;
>> cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
>> cf->data[3] |= CAN_ERR_PROT_LOC_ACK; /* ACK slot */
>> }
>>
>> I need to check what CAN_ERR_ACK is intended for. Then, cf->can_id could
>> be preset with "CAN_ERR_PROT | CAN_ERR_BUSERROR" at the beginning.
This controller issues an ACK error if there are no other nodes on the
CAN bus to send a ACK that the message has been received. Or all
remaining Nodes are in bus off state.
From the datasheet:
"This bit indicates that an acknowledge (ACK) error has been detected by
the transmitter node; that is, a dominant bit has not been detected
during the ACK SLOT."
>>
>>> + }
>>> +
>>> + if (error_warning)
>>> + priv->can.can_stats.error_warning++;
>> Hm, error_warning is always 0 !?
>
> this must go into the state handling function, will fix.
>>> + if (rx_errors)
>>> + dev->stats.rx_errors++;
>>> + if (tx_errors)
>>> + dev->stats.tx_errors++;
>>> +
>>> +}
[...]
>>> +static int flexcan_poll(struct napi_struct *napi, int quota)
>>> +{
>>> + struct net_device *dev = napi->dev;
>>> + const struct flexcan_priv *priv = netdev_priv(dev);
>>> + struct flexcan_regs __iomem *regs = priv->base;
>>> + u32 reg_iflag1, reg_esr;
>>> + int work_done = 0;
>>> +
>>> + reg_iflag1 = readl(®s->iflag1);
>>> +
>>> + /* first handle RX-FIFO */
>>> + while (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE &&
>>> + work_done < quota) {
>>> + flexcan_read_frame(dev);
>>> +
>>> + work_done++;
>>> + reg_iflag1 = readl(®s->iflag1);
>>> + }
>>> +
>>> + /*
>>> + * The error bits are clear on read,
>>> + * so use saved value from irq handler.
>>> + */
>>> + reg_esr = readl(®s->esr) | priv->reg_esr;
>> Re-reading reg_esr may cause lost of state changes.
>
> To my understanding of the datasheet and my observation, only the error
> bits are cleared on read. The bit defining the status
> (FLEXCAN_ESR_FLT_CONF_MASK) == error active, error passive and bus off
> are not cleared on read.
>
> However there are two bits defining RX and TX warning level, I'll check
> these.
I just checked with real hardware , only the error bits are cleared on read.
>>> + if (work_done < quota) {
>>> + flexcan_poll_err(dev, reg_esr);
>> An error frame is created here for each call of flexcan_poll(), not only
>> in case of errors.
>
> Doh, will fix this.
>
>>> + work_done++;
>>> + }
>>> +
>>> + if (work_done < quota) {
>>> + napi_complete(napi);
>>> + /* enable IRQs */
>>> + writel(FLEXCAN_IFLAG_DEFAULT, ®s->imask1);
>>> + writel(priv->reg_ctrl_default, ®s->ctrl);
>>> + }
>>> +
>>> + return work_done;
>>> +}
a reworked patch will follow soon.
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 |
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
