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

[Michal Simek <monstr@monstr.eu>]

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

On 02/11/2014 01:35 PM, Marc Kleine-Budde wrote:
> 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.

We are using xlnx prefix for all generated properties that's why
Kedar just kept it there.


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

ok.


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

No problem with that. Let Kedar to fix it according this and he will send v2 with this.

Thanks,
Michal



-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



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