Re: [PATCH] can: xilinx CAN controller support.

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 5805 bytes --]

On 02/11/2014 12:45 PM, Michal Simek wrote:
> Hi Marc,
> 
> On 02/07/2014 10:37 AM, Marc Kleine-Budde wrote:
>> On 02/07/2014 09:42 AM, Appana Durga Kedareswara Rao wrote:
>>>>> ---
>>>>> 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
>>>>> +                     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"
>>>>> +                     (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@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.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]