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

From: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
To: Oliver Hartkopp <socketcan@hartkopp.net>,
	Marc Kleine-Budde <mkl@pengutronix.de>,
	"wg@grandegger.com" <wg@grandegger.com>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"pawel.moll@arm.com" <pawel.moll@arm.com>,
	"mark.rutland@arm.com" <mark.rutland@arm.com>,
	"ijc+devicetree@hellion.org.uk" <ijc+devicetree@hellion.org.uk>,
	"galak@codeaurora.org" <galak@codeaurora.org>,
	"corbet@lwn.net" <corbet@lwn.net>
Cc: "linux-renesas-soc@vger.kernel.org"
	<linux-renesas-soc@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-can@vger.kernel.org" <linux-can@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	"geert+renesas@glider.be" <geert+renesas@glider.be>,
	Chris Paterson <Chris.Paterson2@renesas.com>
Subject: RE: [PATCH v2] can: rcar_canfd: Add Renesas R-Car CAN FD driver
Date: Tue, 8 Mar 2016 12:48:50 +0000	[thread overview]
Message-ID: <SG2PR06MB10388F87746A1191E51B199FC3B20@SG2PR06MB1038.apcprd06.prod.outlook.com> (raw)
In-Reply-To: <56DEC49D.4@hartkopp.net>

Hi Oliver,

Thanks for the comments.

> On 08.03.2016 09:57, Ramesh Shanmugasundaram wrote:
> 
> >
> > As you mentioned further down, when a user does this
> >
> > "ip link set can0 up type can bitrate 1000000"
> >
> > the intention is to put the controller in CAN 2.0 mode. Even if we use a
> status flag or copy the data bitrate equal to the nominal bitrate, what
> would it achieve? It still cannot be a CAN 2.0 node - it is a CAN FD node
> configured with same nominal & data bitrate.
> >
> > This is why I have this check in ndo_open, so that the user is aware it
> is a CAN FD node always and avoid misconfiguration like above with
> EOPNOTSUPP.
> >
> 
> ok - got it now.
> 
> In fact you provided a CAN driver which is "CAN-FD-only".

Yes. That's the status of current submission.

> We did not had that before but there's a solution for this kind of setup.
> 
> There is a similar case with CAN_CTRLMODE_FD_NON_ISO we had on the M_CAN
> driver which only provides non-ISO configuration for the supported IP core
> and _no_ option to _change_ this value:
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id
> =6cfda7fbebe8a4fd33ea5722fa0212f98f643c35
> 
> If you would do it similar in rcar_canfd.c with
> 
> /* CAN_CTRLMODE_FD is fixed with R-Car CAN FD */
> priv->can.ctrlmode = CAN_CTRLMODE_FD;
> 
> and remove CAN_CTRLMODE_FD from the priv->can.ctrlmode_supported
> assignment then it should do the entire configuration process correctly
> for you.
> Including the proper tests for the two bitrates.
> (see open_candev() in linux/drivers/net/can/dev.c)
> 
> Right?

I did try this option earlier but there are two problems with this method.

1) Below configuration is not possible

ip link set can0 up type can bitrate 1000000 dbitrate 1000000 fd on

"fd on" -> This is not allowed because CAN_CTRLMODE_FD bit is not set in ctrlmode_supported.

2) If I ignore "fd on", my interface MTU stays as CAN_MTU only. If I have to change the MTU alone to CANFD_MTU using another netlink message, it again checks ctrlmode_supported where it would fail. I have the option of providing my own change_mtu function & ignore this check but two configuration messages are required for my driver alone :-(.

Both these anomalies are addressed with the current check I have. 

Thanks,
Ramesh