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:
signature.asc
Description: OpenPGP digital signature
