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