Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751689AbaBKNWo (ORCPT ); Tue, 11 Feb 2014 08:22:44 -0500 Received: from mail-ee0-f54.google.com ([74.125.83.54]:58374 "EHLO mail-ee0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751014AbaBKNWm (ORCPT ); Tue, 11 Feb 2014 08:22:42 -0500 Message-ID: <52FA2419.70102@monstr.eu> Date: Tue, 11 Feb 2014 14:22:33 +0100 From: Michal Simek Reply-To: monstr@monstr.eu User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130330 Thunderbird/17.0.5 MIME-Version: 1.0 To: Marc Kleine-Budde CC: Michal Simek , "robh+dt@kernel.org" , Arnd Bergmann , Appana Durga Kedareswara Rao , "wg@grandegger.com" , "grant.likely@linaro.org" , "linux-can@vger.kernel.org" , "netdev@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" Subject: Re: [PATCH] can: xilinx CAN controller support. References: <6c2bcce0-9897-4d1d-a8b9-47924e40f73c@VA3EHSMHS008.ehs.local> <52F382D5.6090706@pengutronix.de> <8b4dad82-c72a-4e1f-b1af-b8c7964bbf24@TX2EHSMHS043.ehs.local> <52F4A960.10809@pengutronix.de> <52FA1910.2060101@pengutronix.de> In-Reply-To: <52FA1910.2060101@pengutronix.de> X-Enigmail-Version: 1.6 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="hjpfTfliEGdB02KKQBhcv4U3qXDB4SDcb" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --hjpfTfliEGdB02KKQBhcv4U3qXDB4SDcb Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 Ax= i CAN >>>>>> + controllers. >>>>>> +- reg : Physical base address and size of th= e 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 detai= ls). >>>>>> +- 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 =3D "xlnx,zynq-can-1.00.a"; >>>>>> + clocks =3D <&clkc 19>, <&clkc 36>; >>>>>> + clock-names =3D "ref_clk", "aper_clk"; >>>>>> + reg =3D <0xe0008000 0x1000>; >>>>>> + interrupts =3D <0 28 4>; >>>>>> + interrupt-parent =3D <&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 fi= xed for tx and rx fifo's(64) >>>> Xlnx,can-{rx,tx}-dpth is required only for AXI CAN case it is not re= quired 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 jus= t change it >> for different software behaviour. >=20 > 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. >=20 > 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 lik= e >>> to make it a generic CAN driver option. But we have to look around, e= =2Eg. >>> what the Ethernet driver use to configure their hardware. >> >> I think the real question is not if this is acceptable or not. It is j= ust >> reality that we can setup hardware fifo depth and driver has to reflec= t 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 generi= c feature. >> But if this is just specific feature for us then current properties sh= ould >> 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 de= scription >> with all variants and they are automatically generated from tools. >> >> Please let me know what you think. >=20 > I like: >=20 > 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 --=20 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 --hjpfTfliEGdB02KKQBhcv4U3qXDB4SDcb Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlL6JBkACgkQykllyylKDCFgPACfR2Jzw7gwJ3lLQ5y/OWVSSJNy xzEAn2Z4wvwW7Xxy+9bVcMrXn8YoMXzp =VtUH -----END PGP SIGNATURE----- --hjpfTfliEGdB02KKQBhcv4U3qXDB4SDcb-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/