Re: [PATCH] CAN: Add Flexcan CAN controller driver

[Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>]

On 07/21/2010 10:42 PM, Marc Kleine-Budde wrote:
> 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.

Mainly this bus error can cause the infamous IRQ and message flooding
when no cable is connected.

> 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."

That's what the above error describes, like on the SJA1000, apart from
setting CAN_ERR_ACK. Setting CAN_ERR_ACK is somehow bogus, but just
leave it for the time being. I will fix it globally when time permits.

Wolfgang.