Re: [PATCH v4 2/2] can: m_can: add Bosch M_CAN controller support

[Dong Aisheng <b29396@freescale.com>]

On Tue, Jul 15, 2014 at 10:46:32AM +0200, Marc Kleine-Budde wrote:
> On 07/15/2014 10:26 AM, Dong Aisheng wrote:
> >>>>> +static void m_can_read_fifo(const struct net_device *dev, struct can_frame *cf,
> >>>>> +			    u32 rxfs)
> >>>>> +{
> >>>>> +	struct m_can_priv *priv = netdev_priv(dev);
> >>>>> +	u32 flags, fgi;
> >>>>> +
> >>>>> +	/* calculate the fifo get index for where to read data */
> >>>>> +	fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF;
> >>>>> +	flags = m_can_fifo_read(priv, fgi, 0x0);
> >>>>                                            ^^^
> >>>>
> >>>> Can you introduce an enum for the offsets, please adjust the signature
> >>>> of m_can_fifo_read() accordingly.
> >>>>
> >>>
> >>> I wonder enum may not be suitable.
> >>> The Rx Buffer and FIFO Element is as follows:
> >>>    31 24 23 16 15 8 7 0
> >>> R0 ESI XTD RTR ID[28:0]
> >>
> >> M_CAN_FIFO_ID
> >>
> >>> R1 ANMF FIDX[6:0] res EDL BRS DLC[3:0] RXTS[15:0]
> >>
> >> M_CAN_FIFO_DLC
> >>
> >>> R2 DB3[7:0] DB2[7:0] DB1[7:0] DB0[7:0]
> >>> R3 DB7[7:0] DB6[7:0] DB5[7:0] DB4[7:0]
> >>
> >> M_CAN_FIFO_DATA0
> >> M_CAN_FIFO_DATA1
> >>
> > 
> > You mean as follows?
> > enum m_can_fifo {
> >         M_CAN_FIFO_ID = 0,
> >         M_CAN_FIFO_DLC,
> = 0x4,
> >         M_CAN_FIFO_DATA0,
> = 0x8,
> >         M_CAN_FIFO_DATA1,
> = 0xc,
> > };
> > 
> > static inline u32 m_can_fifo_read(const struct m_can_priv *priv,
> >                                   u32 fgi, enum m_can_fifo fifo)
> > {
> >         return readl(priv->mram_base + priv->mcfg[MRAM_RXF0].off +
> >                      fgi * RXF0_ELEMENT_SIZE + fifo * 0x4);
> > }
> 
> without the * 0x4
> 
> > id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_ID);
> > 
> > The problem is when adding long frames support, it becomes:
> > enum m_can_fifo {
> >         M_CAN_FIFO_ID = 0,
> >         M_CAN_FIFO_DLC,
> >         M_CAN_FIFO_DATA0,
> >         M_CAN_FIFO_DATA1,
> >         ....
> >         M_CAN_FIFO_DATA15,
> > };
> 
> #define M_CAN_FIFO_DATA(n)
> 	(enum m_can_fifo)(M_CAN_FIFO_DATA_0 + (n) << 2)
> 

This is a bit strange using and we may still have to define other M_CAN_FIFO_DATAx
to avoid the enum value exceeds the defined range.
enum m_can_fifo {
        M_CAN_FIFO_ID = 0,
        M_CAN_FIFO_DLC = 0x4,
        M_CAN_FIFO_DATA0 = 0x8,
        M_CAN_FIFO_DATA1 = 0xc,
	....
        M_CAN_FIFO_DATA15 = 0xc,
};

However, actually we will not use them at all after introducing M_CAN_FIFO_DATA(n).
If that, why we still need define them in enum?

Comparing to this way, why not simply just do as follows:
#define M_CAN_FIFO_ID		0x0
#define M_CAN_FIFO_DLC		0x4
#define M_CAN_FIFO_DATA(n)	(0x8 + (n) << 2)

What do you think?

> > But it's useless because we may not use enum to read fifo data anymore.
> > It's not suitable to read fifo one by one:
> > m_can_fifo_read(priv, fgi, M_CAN_FIFO_DATA0);
> > m_can_fifo_read(priv, fgi, M_CAN_FIFO_DATA1);
> > ..
> > m_can_fifo_read(priv, fgi, M_CAN_FIFO_DATA15);
> > 
> > 
> > Instead, we may read data according to real dlc value within a for loop like:
> > #define M_CAN_FIFO(n)   (n * 0x4)
> > id = m_can_fifo_read(priv, fgi, M_CAN_FIFO(0));
> > dlc = m_can_fifo_read(priv, fgi, M_CAN_FIFO(1));
> > for (i = 0; dlc > 0; dlc -= 0x4, i++) {
> >         ....
> >         data[i] = m_can_fifo_read(priv, fgi, M_CAN_FIFO(i + 2));
> > }
> 
> id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_ID);
> dlc = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC);
> for (i = 0; i <= dlc; i++)
> 	data[i] = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DATA(i));

Does it work?
The dlc is in bytes while m_can_fifo_read is read in words.

Regards
Dong Aisheng

> 
> > So i'm not sure define that enum now is really needed.
> 
> [...]
> 
> >>>>> +static int m_can_handle_lec_err(struct net_device *dev,
> >>>>> +				enum m_can_lec_type lec_type)
> >>>>> +{
> >>>>> +	struct m_can_priv *priv = netdev_priv(dev);
> >>>>> +	struct net_device_stats *stats = &dev->stats;
> >>>>> +	struct can_frame *cf;
> >>>>> +	struct sk_buff *skb;
> >>>>> +
> >>>>> +	/* early exit if no lec update */
> >>>>> +	if (lec_type == LEC_UNUSED)
> >>>>> +		return 0;
> >>>>
> >>>> I think this is not needed, as checked by the only caller.
> >>>
> >>> You mean move it to caller as follows?
> >>>         /* handle lec errors on the bus */
> >>>         if ((psr & LEC_UNUSED) && ((psr & LEC_UNUSED)!= LEC_UNUSED) &&
> >>
> >> yes - or something like this:
> >>
> >> static inline bool is_lec(u32 psr)
> >> {
> >> 	u32 psr &= LEC_UNUSED
> >>
> >> 	return psr && (psr != LEC_UNUSED)
> >> }
> >>
> >> 	if ((priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) &&
> >> 		is_lec(psr)) {
> >> 	}
> >>
> > 
> > 
> > Looks fine.
> > Maybe is_lec_err(u32 psr) better? :-)
> 
> Yes, is_lec() was just a random placeholder :) Descriptive function
> names are always preferred.
> 
> 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   |
>