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

Ramesh Shanmugasundaram
2016-03-07T09:33:28Z

Hi Rob,

Thanks for the review comments.

> On Thu, Mar 03, 2016 at 03:38:35PM +0000, Ramesh Shanmugasundaram wrote:
> > This patch adds support for the CAN FD controller found in Renesas
> > R-Car SoCs. The controller operates in CAN FD mode by default.
> >
> > CAN FD mode supports both Classical CAN & CAN FD frame formats. The
> > controller supports ISO 11898-1:2015 CAN FD format only.
> >
> > This controller supports two channels and the driver can enable either
> > or both of the channels.
> >
> > Driver uses Rx FIFOs (one per channel) for reception & Common FIFOs
> > (one per channel) for transmission. Rx filter rules are configured to
> > the minimum (one per channel) and it accepts Standard, Extended, Data
> > & Remote Frame combinations.
> >
> > Note: There are few documentation errors in R-Car Gen3 Hardware User
> > Manual v0.5E with respect to CAN FD controller. They are listed below:
> >
> > 1. CAN FD interrupt numbers 29 & 30 are listed as per channel
> > interrupts. However, they are common to both channels (i.e.) they are
> > global and channel interrupts respectively.
> >
> > 2. CANFD clock is derived from PLL1. This is not documented.
> >
> > 3. CANFD clock is further divided by (1/2) within the CAN FD controller.
> > This is not documented.
> >
> > 4. The minimum value of NTSEG1 in RSCFDnCFDCmNCFG register is 2 Tq. It
> > is mentioned 4 Tq in the manual.
> >
> > 5. The maximum number of message RAM area the controller can use is
> > 3584 bytes. It is specified 10752 bytes in the manual.
> >
> > Signed-off-by: Ramesh Shanmugasundaram
> > <ramesh.shanmugasundaram@bp.renesas.com>
> > ---
> > 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
> >
> > Thanks,
> > Ramesh
> > ---
> > .../devicetree/bindings/net/can/rcar_canfd.txt | 86 ++
> > drivers/net/can/Kconfig | 10 +
> > drivers/net/can/Makefile | 1 +
> > drivers/net/can/rcar_canfd.c | 1624
> ++++++++++++++++++++
> > 4 files changed, 1721 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/net/can/rcar_canfd.txt
> > create mode 100644 drivers/net/can/rcar_canfd.c
> >
> > diff --git a/Documentation/devicetree/bindings/net/can/rcar_canfd.txt
> > b/Documentation/devicetree/bindings/net/can/rcar_canfd.txt
> > new file mode 100644
> > index 0000000..4299bd8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/can/rcar_canfd.txt
> > @@ -0,0 +1,86 @@
> > +Renesas R-Car CAN FD controller Device Tree Bindings
> > +----------------------------------------------------
> > +
> > +Required properties:
> > +- compatible: Must contain one or more of the following:
> > + - "renesas,rcar-gen3-canfd" for R-Car Gen3 compatible controller.
> > + - "renesas,r8a7795-canfd" for R8A7795 (R-Car H3) compatible
> controller.
> > +
> > + When compatible with the generic version, nodes must list the
> > + SoC-specific version corresponding to the platform first, followed
> > + by the family-specific and/or generic versions.
> > +
> > +- reg: physical base address and size of the R-Car CAN FD register map.
> > +- interrupts: interrupt specifier for the Global & Channel interrupts
> > +- clocks: phandles and clock specifiers for 3 clock inputs.
> > +- clock-names: 3 clock input name strings: "fck", "canfd", "can_clk".
> > +- pinctrl-0: pin control group to be used for this controller.
> > +- pinctrl-names: must be "default".
> > +
> > +Required properties for "renesas,r8a7795-canfd" compatible:
> > +In R8A7795 SoC, canfd clock is a div6 clock and can be used by both
> > +CAN and CAN FD controller at the same time. It needs to be scaled to
> > +maximum frequency if any of these controllers use it. This is done
> > +using the below properties.
> > +
> > +- assigned-clocks: phandle of canfd clock.
> > +- assigned-clock-rates: maximum frequency of this clock.
> > +
> > +Each channel is represented as a child node. They can be
> > +enabled/disabled using "status" property.
>
> How many channels? Required or optional?

Two channels and it is a required property. I will update as below in the next revision.

Required child node:
The controller supports two channels and each is represented as a child
node. The names of the child nodes are "channel0" and "channel1" respectively.
Each child node supports the "status" property only, which is used to
enable/disable the respective channel.

>
> > +
> > +Example
> > +-------
> > +
> > +SoC common .dtsi file:
> > +
> > + canfd: canfd@e66c0000 {
>
> can@e66c0000

Thanks. I will update.

Thanks,
Ramesh