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

[Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 4409 bytes --]

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(&regs->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(&regs->iflag1);
>>> +	}
>>> +
>>> +	/*
>>> +	 * The error bits are clear on read,
>>> +	 * so use saved value from irq handler.
>>> +	 */
>>> +	reg_esr = readl(&regs->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, &regs->imask1);
>>> +		writel(priv->reg_ctrl_default, &regs->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   |


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core