Re: [PATCH v2] can: rcar_canfd: Add Renesas R-Car CAN FD driver

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

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

On 03/07/2016 09:02 AM, Ramesh Shanmugasundaram wrote:
> Hi Oliver,
> 
> Thanks for the review comments.
> 
>> On 03/03/2016 04:38 PM, Ramesh Shanmugasundaram wrote:
>>
>>> Changes since v1:
>>> 	* Removed testmodes & debugfs code (suggested by Oliver H)
>>> 	* Fixed tx path race issue by introducing lock (suggested by Marc K)
>>> 	* Removed __maybe_unused attribute of rcar_canfd_of_table
>>>
>>
>>
>>>  obj-$(CONFIG_CAN_M_CAN)		+= m_can/
>>>  obj-$(CONFIG_CAN_RCAR)		+= rcar_can.o
>>> +obj-$(CONFIG_CANFD_RCAR)	+= rcar_canfd.o
>>
>> Just for the sake of an ordered directory structure:
>>
>> Would it make sense to create a rcar directory where rcar_can.c and
>> rcar_canfd.c are placed?
>>
> Yes. I'll place them in rcar dir.
> 
>> Additionally (besides of one accident with the obsolete PCH_CAN) all CAN
>> drivers start with CONFIG_CAN_...
>>
>> Following that scheme the config option should be named
>>
>> 	CONFIG_CAN_RCAR_CANFD
>>
>> as we had in CONFIG_CAN_IFI_CANFD.
> 
> Agreed.
> 
>>
>>>  obj-$(CONFIG_CAN_SJA1000)	+= sja1000/
>>>  obj-$(CONFIG_CAN_SUN4I)		+= sun4i_can.o
>>>  obj-$(CONFIG_CAN_TI_HECC)	+= ti_hecc.o
>>> diff --git a/drivers/net/can/rcar_canfd.c
>>> b/drivers/net/can/rcar_canfd.c
>>
>> (..)
>>
>>> +/* Channel priv data */
>>> +struct rcar_canfd_channel {
>>> +	struct can_priv can;			/* Must be the first member */
>>> +	struct net_device *ndev;
>>> +	struct rcar_canfd_global *gpriv;	/* Controller reference */
>>> +	void __iomem *base;			/* Register base address */
>>> +#ifdef CONFIG_DEBUG_FS
>>> +	struct dentry *dev_root;
>>> +#endif /* CONFIG_DEBUG_FS */
>>
>> Some missed stuff from debugfs removal?
> 
> :-(. Sorry. Will clean up.
> 
>>
>>> +	struct napi_struct napi;
>>> +	u8  tx_len[RCANFD_FIFO_DEPTH];		/* For net stats */
>>> +	u32 tx_head;				/* Incremented on xmit */
>>> +	u32 tx_tail;				/* Incremented on xmit done */
>>> +	u32 channel;				/* Channel number */
>>> +	spinlock_t tx_lock;			/* To protect tx path */
>>> +};
>>
>> (..)
>>
>>> +static int rcar_canfd_start(struct net_device *ndev) {
>>> +	struct rcar_canfd_channel *priv = netdev_priv(ndev);
>>> +	int err = -EOPNOTSUPP;
>>> +	u32 sts, ch = priv->channel;
>>> +	u32 ridx = ch + RCANFD_RFFIFO_IDX;
>>> +
>>> +	/* Ensure channel starts in FD mode */
>>> +	if (!(priv->can.ctrlmode & CAN_CTRLMODE_FD)) {
>>> +		netdev_err(ndev, "enable can fd mode for channel %d\n", ch);
>>> +		goto fail_mode;
>>> +	}
>>
>> What's the reason behind this check?
>>
>> A CAN FD capable CAN controller can be either configured to run as CAN 2.0
>> (Classic CAN) or as CAN FD controller.
>>
>> So why are to throwing an error here and produce an initialization
>> failure?
> 
> When this controller is configured in FD mode and used only with CAN 2.0 nodes, it still expects a DTSEG (data bitrate) configuration same as NTSEG (nominal bitrate). This check, specifically in ndo_open, ensures both are configured and will work fine with CAN 2.0 nodes (e.g.)
> 
> "ip link set can0 up type can bitrate 1000000 dbitrate 1000000 fd on"
> 
> If I don't have this check, a configuration like this
> 
> "ip link set can0 up type can bitrate 1000000"
> 
> will bring up the controller without DTSEG configured. 

That should bring up the controller in CAN 2.0 mode.

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: 455 bytes --]