On 02/11/2014 12:45 PM, Michal Simek wrote:

Hi Marc,

On 02/07/2014 10:37 AM, Marc Kleine-Budde wrote:

quoted

On 02/07/2014 09:42 AM, Appana Durga Kedareswara Rao wrote:

quoted

quoted

quoted

---
This patch is rebased on the 3.14 rc1 kernel.
---
 .../devicetree/bindings/net/can/xilinx_can.txt     |   43 +
 drivers/net/can/Kconfig                            |    8 +
 drivers/net/can/Makefile                           |    1 +
 drivers/net/can/xilinx_can.c                       | 1150 ++++++++++++++++++++
 4 files changed, 1202 insertions(+), 0 deletions(-)  create mode
100644 Documentation/devicetree/bindings/net/can/xilinx_can.txt
 create mode 100644 drivers/net/can/xilinx_can.c

diff --git a/Documentation/devicetree/bindings/net/can/xilinx_can.txt

b/Documentation/devicetree/bindings/net/can/xilinx_can.txt
new file mode 100644
index 0000000..34f9643

--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/xilinx_can.txt

@@ -0,0 +1,43 @@
+Xilinx Axi CAN/Zynq CANPS controller Device Tree Bindings
+---------------------------------------------------------
+
+Required properties:
+- compatible               : Should be "xlnx,zynq-can-1.00.a" for Zynq

CAN

quoted

+                     controllers and "xlnx,axi-can-1.00.a" for Axi CAN
+                     controllers.
+- reg                      : Physical base address and size of the Axi CAN/Zynq
+                     CANPS registers map.
+- interrupts               : Property with a value describing the interrupt
+                     number.
+- interrupt-parent : Must be core interrupt controller
+- clock-names              : List of input clock names - "ref_clk",

"aper_clk"

quoted

+                     (See clock bindings for details. Two clocks are
+                      required for Zynq CAN. For Axi CAN
+                      case it is one(ref_clk)).
+- clocks           : Clock phandles (see clock bindings for details).
+- xlnx,can-tx-dpth : Can Tx fifo depth (Required for Axi CAN).
+- xlnx,can-rx-dpth : Can Rx fifo depth (Required for Axi CAN).
+
+
+Example:
+
+For Zynq CANPS Dts file:
+   zynq_can_0: zynq-can at e0008000 {
+                   compatible = "xlnx,zynq-can-1.00.a";
+                   clocks = <&clkc 19>, <&clkc 36>;
+                   clock-names = "ref_clk", "aper_clk";
+                   reg = <0xe0008000 0x1000>;
+                   interrupts = <0 28 4>;
+                   interrupt-parent = <&intc>;

Above xlnx,can-{rx,tx}-dpth is mentioned as required, but it's not in the
Zynq example.

One of the Difference b/w the AXI CAN and zynq CAN is in AXI CAN the fifo depth(tx,rx)
Is user configurable. But in case of ZYNQ CAN  the fifo depth  is fixed for tx and rx fifo's(64)
Xlnx,can-{rx,tx}-dpth is required only for AXI CAN case it is not required for zynq CAN.
That's why didn't putted that property in device tree.

The device tree should be a hardware only description and should not
hold any user configurable data. Please split your patch into two
patches. The first one should add the driver with a fixed fifo size
(e.g. 0x40) for the AXI, too. The second patch should make the fifo
configurable via device tree.

can-rx/tx-dpth is not user configurable data as you think.
This is FPGA where you can configure this parameter in design tools.
It means these 2 values just describe real hardware and user can't just change it
for different software behaviour.

I see, thanks for the clarification. I had a short grep over the
arm/boot/dts folder and it seems that fifo-depth is a more or less
common property. I think it should be called {rx,tx}-fifo-depth. I'm
unsure whether we need the xlnx or not.

Also I don't think it is worth to create 2 patches for the same driver
where the first one is useless for axi can device. But if you think
that it is worth to do we can create 2 patches as you suggested.

Also what we can do is to define that this property is required also
for zynq which is 0x40 and change code according too.

Good idea, I think this would make the driver more uniform.

quoted

If it's acceptable to describe the fifo usage by device tree, I'd like
to make it a generic CAN driver option. But we have to look around, e.g.
what the Ethernet driver use to configure their hardware.

I think the real question is not if this is acceptable or not. It is just
reality that we can setup hardware fifo depth and driver has to reflect this
because without it driver just doesn't work for axi can.

The only remaining question is if we should create generic DT binding
for fifo depth. Arnd, Rob: Any opinion about it?
Definitely will be worth to have one generic binding if this is generic feature.
But if this is just specific feature for us then current properties should
be fine.

In general all these xlnx,XXX properties just reflect all configurable options
which you can setup in design tool which means that provide full hw description
with all variants and they are automatically generated from tools.

Please let me know what you think.

I like:

    rx-fifo-depth
    tx-fifo-depth

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   |

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 242 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140211/b15e7ecc/attachment.sig>