Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3791 bytes --]

On 07/02/2014 08:20 AM, Dong Aisheng wrote:
[...]

>>> +static int m_can_do_rx_poll(struct net_device *dev, int quota)
>>> +{
>>> +	struct m_can_priv *priv = netdev_priv(dev);
>>> +	struct net_device_stats *stats = &dev->stats;
>>> +	struct sk_buff *skb;
>>> +	struct can_frame *frame;
>>> +	u32 rxfs, flags, fgi;
>>> +	u32 num_rx_pkts = 0;
>>> +
>>> +	rxfs = m_can_read(priv, M_CAN_RXF0S);
>>> +	if (!(rxfs & RXFS_FFL_MASK)) {
>>> +		netdev_dbg(dev, "no messages in fifo0\n");
>>> +		return 0;
>>> +	}
>>> +
>>> +	while ((rxfs & RXFS_FFL_MASK) && (quota > 0)) {
>>> +		netdev_dbg(dev, "fifo0 status 0x%x\n", rxfs);
>>
>> Please remove the netdev_dbg(), once the driver is stable it should be
>> of no use.
>>
> 
> Got it.
> 
>>> +		if (rxfs & RXFS_RFL)
>>> +			netdev_warn(dev, "Rx FIFO 0 Message Lost\n");
>>
>> What does that mean? Can you still rx the message if it's lost?
>>
> 
> It just warns that there's a message lost, but there are still other
> message in fifo to receive.
> 
>>> +
>>> +		skb = alloc_can_skb(dev, &frame);
>>> +		if (!skb) {
>>> +			stats->rx_dropped++;
>>> +			return -ENOMEM;
>>
>> Have a look at the user of m_can_do_rx_poll() and how it makes use of
>> the return value.
>>
> 
> Right, thanks for spotting it.
> 
>>> +		}
>>> +
>>> +		fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF;
>>
>> BTW: Is this a _real_ fifo? Or evolution of the c_can/d_can interface
>> where it's not a fifo at all.
>>
> 
> Yes, it is real fifo in the message ram.
> 
>>> +		flags = readl(priv->mram_base + priv->rxf0_off + fgi * 16);
>>> +		if (flags & RX_BUF_XTD)
>>> +			frame->can_id = (flags & CAN_EFF_MASK) | CAN_EFF_FLAG;
>>> +		else
>>> +			frame->can_id = (flags >> 18) & CAN_SFF_MASK;
>>> +		netdev_dbg(dev, "R0 0x%x\n", flags);
>>
>> please remove dbg
>>> +
>>> +		if (flags & RX_BUF_RTR) {
>>> +			frame->can_id |= CAN_RTR_FLAG;
>>> +		} else {
>>> +			flags = readl(priv->mram_base +
>>> +					priv->rxf0_off + fgi * 16 + 0x4);
>>> +			frame->can_dlc = get_can_dlc((flags >> 16) & 0x0F);
>>> +			netdev_dbg(dev, "R1 0x%x\n", flags);
>>
>> please remove
>>
>>> +
>>> +			*(u32 *)(frame->data + 0) = readl(priv->mram_base +
>>> +					priv->rxf0_off + fgi * 16 + 0x8);
>>> +			*(u32 *)(frame->data + 4) = readl(priv->mram_base +
>>> +					priv->rxf0_off + fgi * 16 + 0xC);
>>
>>
>> can you create a wrapper function to hide the pointer arithmetics here?
>> Somethig like m_can_read_fifo()
>>
> 
> Yes, i could make a wrapper function for it.
> 
>>> +			netdev_dbg(dev, "R2 0x%x\n", *(u32 *)(frame->data + 0));
>>> +			netdev_dbg(dev, "R3 0x%x\n", *(u32 *)(frame->data + 4));
>>> +		}
>>> +
>>> +		/* acknowledge rx fifo 0 */
>>> +		m_can_write(priv, M_CAN_RXF0A, fgi);
>>> +
>>> +		netif_receive_skb(skb);
>>> +		netdev_dbg(dev, "new packet received\n");
>>> +
>>> +		stats->rx_packets++;
>>> +		stats->rx_bytes += frame->can_dlc;
>>
>> Please move the stats handling in front of netif_receive_skb() as the
>> skb and thus frame is not a valid pointer anymore.
>>
> 
> Good catch!
> Will change it.
> 
>>> +
>>> +		can_led_event(dev, CAN_LED_EVENT_RX);
>>
>> Please move out of the loop so that it is just called once (if a CAN
>> frame is rx'ed) per  m_can_do_rx_poll().

> Why that?
> The purpose is calling it for each new packet received.

It will only trigger LED blinking, and tglx pointed out, that we don't
need the overhead of calling it for every CAN frame.

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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]