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

2016-03-02 Thread Marc Kleine-Budde 
On 03/02/2016 11:08 AM, Ramesh Shanmugasundaram wrote:
 I see no locking for the tx-path.
>>>
>>> I am not sure why locking is needed in tx-path?
>>
>> If the tx-path is shared between both channels you need locking as the
>> networking subsystem may send one frame to each controller at the same
>> time.
> 
> Yes. Tx completion interrupt is shared by both channels but the Tx
> FIFO, echo skbs, head, tail are maintained on a per channel basis. 
> Yes, net subsystem can send one frame to each channel at the same
> time and the tx completion interrupt handler will traverse each
> channel & update per channel context accordingly.

Ok. Which instruction starts the transmit? Please add a comment to the
code. You stop the tx_queue after this, so your code is racy, as the
interrupt might happen in between.

>>> However, looking at it again, I should move the incrementing of head
>>> after the "sts" handing to be apt. What do you think?
>>
>> With one producer (one SW instance) and one consumer (the HW) you can
>> write lockless code (if the HW allows it), but with two producers it's not
>> possible.
> 
> For a channel represented as netdev, there is still one producer (one
> SW instance) isn't it? net acquires lock before calling xmit. The
> consumer is tx completion interrupt handler which updates per channel
> attributes only.

Ok - I haven't looked at the code in depth yet. Now it's clear, that
just the tx interrupt is shared.

> Sorry if I am annoying. I have tested this with two channels
> transmitting & receiving at the same time and it works fine. If I am
> missing lock, I would like to understand it thoroughly before making
> the change.

Marc

-- 
Pengutronix e.K.  | Marc Kleine-Budde   |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund  | Fax:   +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |



signature.asc
Description: OpenPGP digital signature

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

2016-03-02 Thread Ramesh Shanmugasundaram 
Hi Marc,

> On 03/02/2016 09:41 AM, Ramesh Shanmugasundaram wrote:
> >> Is the channel loopback mode configurable per channel? If so, please
> >> remove the module parameter and make use of CAN_CTRLMODE_LOOPBACK to
> >> configure it.
> >
> > The loopback setting is not truly a per channel attribute. It requires
> > touching Rx rules which can be done only when the controller's global
> > state is reset (during probe).
> > CAN_CTRLMODE_LOOPBACK config option is possible only through .ndo_open
> > of that channel but the global controller state needs to be
> > operational by this time. As it is a global attribute & for the above
> > reason, I choose the module option.
> 
> I see, ok then.
> 
> >>> 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.
> >>
> >> I see no locking for the tx-path.
> >
> > I am not sure why locking is needed in tx-path?
> 
> If the tx-path is shared between both channels you need locking as the
> networking subsystem may send one frame to each controller at the same
> time.

Yes. Tx completion interrupt is shared by both channels but the Tx FIFO, echo 
skbs, head, tail are maintained on a per channel basis. 
Yes, net subsystem can send one frame to each channel at the same time and the 
tx completion interrupt handler will traverse each channel & update per channel 
context accordingly.

> > However, looking at it again, I should move the incrementing of head
> > after the "sts" handing to be apt. What do you think?
> 
> With one producer (one SW instance) and one consumer (the HW) you can
> write lockless code (if the HW allows it), but with two producers it's not
> possible.

For a channel represented as netdev, there is still one producer (one SW 
instance) isn't it? net acquires lock before calling xmit. The consumer is tx 
completion interrupt handler which updates per channel attributes only.

Sorry if I am annoying. I have tested this with two channels transmitting & 
receiving at the same time and it works fine. If I am missing lock, I would 
like to understand it thoroughly before making the change.

Thanks for the help,
Ramesh.
N�r��yb�X��ǧv�^�)޺{.n�+{�v�"��^n�r���z���h�&���G���h�(�階�ݢj"���m��z�ޖ���f���h���~�m�

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

2016-03-02 Thread Ramesh Shanmugasundaram 
Hi Marc,

Thanks for the review.

> On 03/01/2016 10:34 AM, 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. Two
> > test modes are available and can be enabled by the
> > "rcar_canfd.testmode" module parameter. Refer to Documentation/kernel-
> parameters.txt.
> 
> Is the channel loopback mode configurable per channel? If so, please
> remove the module parameter and make use of CAN_CTRLMODE_LOOPBACK to
> configure it.

The loopback setting is not truly a per channel attribute. It requires touching 
Rx rules which can be done only when the controller's global state is reset 
(during probe). CAN_CTRLMODE_LOOPBACK config option is possible only through 
.ndo_open of that channel but the global controller state needs to be 
operational by this time. As it is a global attribute & for the above reason, I 
choose the module option.

> 
> > 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.
> 
> I see no locking for the tx-path.
 
I am not sure why locking is needed in tx-path? 
However, looking at it again, I should move the incrementing of head after the 
"sts" handing to be apt. What do you think?

Thanks,
Ramesh
N�r��yb�X��ǧv�^�)޺{.n�+{�v�"��^n�r���z���h�&���G���h�(�階�ݢj"���m��z�ޖ���f���h���~�m�

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

2016-03-01 Thread Marc Kleine-Budde 
On 03/01/2016 10:34 AM, 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. Two test modes
> are available and can be enabled by the "rcar_canfd.testmode" module
> parameter. Refer to Documentation/kernel-parameters.txt.

Is the channel loopback mode configurable per channel? If so, please
remove the module parameter and make use of CAN_CTRLMODE_LOOPBACK to
configure it.

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

I see no locking for the tx-path.

> 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 mentioned 10752 bytes in the manual.
> 
> Signed-off-by: Ramesh Shanmugasundaram 
> 

Marc

-- 
Pengutronix e.K.  | Marc Kleine-Budde   |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund  | Fax:   +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |



signature.asc
Description: OpenPGP digital signature

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

2016-03-01 Thread Ramesh Shanmugasundaram 
This patch adds support for the CAN FD controller found in Renesas R-Car
SoCs. The controller operates in CAN FD mode by default. Two test modes
are available and can be enabled by the "rcar_canfd.testmode" module
parameter. Refer to Documentation/kernel-parameters.txt.

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 mentioned 10752 bytes in the manual.

Signed-off-by: Ramesh Shanmugasundaram 
---
Hi All,

   This work is based on linux-can-next (tag:linux-can-next-for-4.6-20160226) 
with
   the following patch applied on top

   [PATCH] can: rcar_can: Add r8a7795 support

   Pinctrl & Clock related to the controller are already submitted as below:

   https://lkml.org/lkml/2016/2/25/546
   https://lkml.org/lkml/2016/2/26/452
   http://www.spinics.net/lists/arm-kernel/msg487238.html

   DT submission will be done separately based on linux-next

Thanks,
Ramesh
---
 .../devicetree/bindings/net/can/rcar_canfd.txt |   86 +
 Documentation/kernel-parameters.txt|8 +
 drivers/net/can/Kconfig|   10 +
 drivers/net/can/Makefile   |1 +
 drivers/net/can/rcar_canfd.c   | 1905 
 5 files changed, 2010 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 000..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.
+
+Example
+---
+
+SoC common .dtsi file:
+
+   canfd: canfd@e66c {
+   compatible = "renesas,r8a7795-canfd",
+"renesas,rcar-gen3-canfd";
+   reg = <0 0xe66c 0 0x8000>;
+   interrupts = ,
+  ;
+   clocks = < CPG_MOD 914>,
+  < CPG_CORE R8A7795_CLK_CANFD>,
+  <_clk>;
+   clock-names = "fck", "canfd", "can_clk";
+   assigned-clocks = < CPG_CORE R8A7795_CLK_CANFD>;
+   assigned-clock-rates = <4000>;
+   power-domains = <>;
+   status = "disabled";
+
+   channel0 {
+