Hi Marc,

On 03/07/2016 09:02 AM, Ramesh Shanmugasundaram wrote:

quoted

Hi Oliver,

Thanks for the review comments.

quoted

On 03/03/2016 04:38 PM, Ramesh Shanmugasundaram wrote:

quoted

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

quoted

 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.

quoted

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.

quoted

quoted

 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

(..)

quoted

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

quoted

quoted

+	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 */
+};

(..)

quoted

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

Yes, that's the user's intention but the manual states DTSEG still need to be configured. In the above configuration, it will not be.
Besides, this will not be a "pure" CAN 2.0 node (i.e.) if a frame with length > 8 bytes is received the controller will "ACK" because in FD mode it can receive up to 64 bytes frame.

The controller does support a "pure" classical CAN mode with a different set of register map itself. Do you think a pure CAN 2.0 mode support would be beneficial? I can submit this in coming days on top of current submission.

The current submission status is:
 - Controller operates in CAN FD mode only.
 - If needed to interoperate with CAN 2.0 nodes, data bitrate still need to be configured and it will work perfectly. However, it is not a "pure" CAN 2.0 node as mentioned above.

Thanks,
Ramesh