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

On 07/21/2010 10:42 PM, Marc Kleine-Budde wrote:

Marc Kleine-Budde wrote:

quoted

Wolfgang Grandegger wrote:

quoted

I realized a few issues. You can add my "acked-by" when they are fixed.

thanks for the review.

[...]

quoted

quoted

quoted

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