[v5] can: xilinx CAN controller support.

This patch adds xilinx CAN controller support.
This driver supports both ZYNQ CANPS and Soft IP
AXI CAN controller.

Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
---
This patch is rebased on the 3.14 rc5 kernel.
Changes for v5:
- Updated the driver with the review comments.
- Remove the check for the tx fifo full interrupt condition
  form Tx interrupt routine as we are checking it in the _xmit
  routine.
- Clearing the txok interrupt in the tx interrupt routine for
  every Tx can frame.
Changes for v4:
- Added check for the tx fifo full interrupt condition in
  Tx interrupt routine.
- Added be iohelper functions.
- Moved the clock enable/disable to probe/remove because of
  Added big endian support for AXI CAN controller case(reading
  a register during probe for that we need to enable clock).
Changes for v3:
- Updated the driver with the review comments.
- Modified the tranmit logic as per Marc suggestion.
- Enabling the clock when the interface is up to reduce the
  Power consumption.
Changes for v2:
- Updated with the review comments.
- Removed the unnecessary debug prints.
- include tx,rx fifo depths in ZYNQ CANPS case also.
---
 .../devicetree/bindings/net/can/xilinx_can.txt     |   45 +
 drivers/net/can/Kconfig                            |    7 +
 drivers/net/can/Makefile                           |    1 +
 drivers/net/can/xilinx_can.c                       | 1195 ++++++++++++++++++++
 4 files changed, 1248 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/can/xilinx_can.txt
 create mode 100644 drivers/net/can/xilinx_can.c

Hi Kedar,

On Tue, 2014-03-04 at 06:50PM +0530, Kedareswara rao Appana wrote:
> This patch adds xilinx CAN controller support.
> This driver supports both ZYNQ CANPS and Soft IP
> AXI CAN controller.
> 
[...]
> 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..0e57103
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/xilinx_can.txt
> @@ -0,0 +1,45 @@
> +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"

Let's reconsider these names. These are rather Zynq specific names. Does
the IP documentation use these as well? The names should match the
naming used for the IP, rather than the SOC. Is this the correct data sheet:
http://www.xilinx.com/support/documentation/ip_documentation/axi_can/v1_03_a/ds791_axi_can.pdf
? According to that the names should rather be 's_axi_aclk' and
'can_clk', IMHO.

	Sören


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 05.03.2014 00:51, Sören Brinkmann wrote:
> Hi Kedar,
> 
> On Tue, 2014-03-04 at 06:50PM +0530, Kedareswara rao Appana wrote:
>> This patch adds xilinx CAN controller support.
>> This driver supports both ZYNQ CANPS and Soft IP
>> AXI CAN controller.
>>
> [...]
>> 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..0e57103
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/can/xilinx_can.txt
>> @@ -0,0 +1,45 @@
>> +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"
> 
> Let's reconsider these names. These are rather Zynq specific names. Does
> the IP documentation use these as well? The names should match the
> naming used for the IP, rather than the SOC. Is this the correct data sheet:
> http://www.xilinx.com/support/documentation/ip_documentation/axi_can/v1_03_a/ds791_axi_can.pdf
> ? According to that the names should rather be 's_axi_aclk' and
> 'can_clk', IMHO.
> 
> 	Sören
> 

And when renaming stuff anyway:

+
+#define DRIVER_NAME	"XILINX_CAN"
+

should be changed to

+
+#define DRIVER_NAME	"xilinx_can"
+

Regards,
Oliver

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Soren,

> -----Original Message-----

> From: Sören Brinkmann [mailto:soren.brinkmann@xilinx.com]

> Sent: Wednesday, March 05, 2014 5:21 AM

> To: Appana Durga Kedareswara Rao

> Cc: wg@grandegger.com; mkl@pengutronix.de; Michal Simek;

> grant.likely@linaro.org; robh+dt@kernel.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; Appana Durga

> Kedareswara Rao

> Subject: Re: [PATCH v5] can: xilinx CAN controller support.

>

> Hi Kedar,

>

> On Tue, 2014-03-04 at 06:50PM +0530, Kedareswara rao Appana wrote:

> > This patch adds xilinx CAN controller support.

> > This driver supports both ZYNQ CANPS and Soft IP AXI CAN controller.

> >

> [...]

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

> > --- /dev/null

> > +++ b/Documentation/devicetree/bindings/net/can/xilinx_can.txt

> > @@ -0,0 +1,45 @@

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

>

> Let's reconsider these names. These are rather Zynq specific names. Does

> the IP documentation use these as well? The names should match the

> naming used for the IP, rather than the SOC. Is this the correct data sheet:

> http://www.xilinx.com/support/documentation/ip_documentation/axi_can

> /v1_03_a/ds791_axi_can.pdf

> ? According to that the names should rather be 's_axi_aclk' and 'can_clk',

> IMHO.


Axi CAN driver uses only one clock i.e ref_clk the name in the documentation for this clock is can_clk.
Ok will change the clock name to can_clk.


Regards,
Kedar.

>

>       Sören




This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

Hi Oliver,

> -----Original Message-----

> From: Oliver Hartkopp [mailto:socketcan@hartkopp.net]

> Sent: Wednesday, March 05, 2014 12:29 PM

> To: Appana Durga Kedareswara Rao

> Cc: Soren Brinkmann; wg@grandegger.com; mkl@pengutronix.de; Michal

> Simek; grant.likely@linaro.org; robh+dt@kernel.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; Appana Durga Kedareswara Rao

> Subject: Re: [PATCH v5] can: xilinx CAN controller support.

>

>

>

> On 05.03.2014 00:51, Sören Brinkmann wrote:

> > Hi Kedar,

> >

> > On Tue, 2014-03-04 at 06:50PM +0530, Kedareswara rao Appana wrote:

> >> This patch adds xilinx CAN controller support.

> >> This driver supports both ZYNQ CANPS and Soft IP AXI CAN controller.

> >>

> > [...]

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

> >> --- /dev/null

> >> +++ b/Documentation/devicetree/bindings/net/can/xilinx_can.txt

> >> @@ -0,0 +1,45 @@

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

> >

> > Let's reconsider these names. These are rather Zynq specific names.

> > Does the IP documentation use these as well? The names should match

> > the naming used for the IP, rather than the SOC. Is this the correct data

> sheet:

> >

> http://www.xilinx.com/support/documentation/ip_documentation/axi_can

> /v

> > 1_03_a/ds791_axi_can.pdf ? According to that the names should rather

> > be 's_axi_aclk' and 'can_clk', IMHO.

> >

> >     Sören

> >

>

> And when renaming stuff anyway:

>

> +

> +#define DRIVER_NAME  "XILINX_CAN"

> +

>

> should be changed to


Ok Will do

Regards,
Kedar.

>

> +

> +#define DRIVER_NAME  "xilinx_can"

> +

>

> Regards,

> Oliver

>




This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

On 03/04/2014 02:20 PM, Kedareswara rao Appana wrote:
> This patch adds xilinx CAN controller support.
> This driver supports both ZYNQ CANPS and Soft IP
> AXI CAN controller.
> 
> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> ---
> This patch is rebased on the 3.14 rc5 kernel.
> Changes for v5:
> - Updated the driver with the review comments.
> - Remove the check for the tx fifo full interrupt condition
>   form Tx interrupt routine as we are checking it in the _xmit
>   routine.
> - Clearing the txok interrupt in the tx interrupt routine for
>   every Tx can frame.
> Changes for v4:
> - Added check for the tx fifo full interrupt condition in
>   Tx interrupt routine.
> - Added be iohelper functions.
> - Moved the clock enable/disable to probe/remove because of
>   Added big endian support for AXI CAN controller case(reading
>   a register during probe for that we need to enable clock).
> Changes for v3:
> - Updated the driver with the review comments.
> - Modified the tranmit logic as per Marc suggestion.
> - Enabling the clock when the interface is up to reduce the
>   Power consumption.
> Changes for v2:
> - Updated with the review comments.
> - Removed the unnecessary debug prints.
> - include tx,rx fifo depths in ZYNQ CANPS case also.
> ---
>  .../devicetree/bindings/net/can/xilinx_can.txt     |   45 +
>  drivers/net/can/Kconfig                            |    7 +
>  drivers/net/can/Makefile                           |    1 +
>  drivers/net/can/xilinx_can.c                       | 1195 ++++++++++++++++++++
>  4 files changed, 1248 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..0e57103
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/xilinx_can.txt
> @@ -0,0 +1,45 @@
> +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).
> +- tx-fifo-depth		: Can Tx fifo depth.
> +- rx-fifo-depth		: Can Rx fifo depth.
> +
> +
> +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>;
> +			tx-fifo-depth = <0x40>;
> +			rx-fifo-depth = <0x40>;
> +		};
> +For Axi CAN Dts file:
> +	axi_can_0: axi-can@40000000 {
> +			compatible = "xlnx,axi-can-1.00.a";
> +			clocks = <&clkc 0>;
> +			clock-names = "ref_clk" ;
> +			reg = <0x40000000 0x10000>;
> +			interrupt-parent = <&intc>;
> +			interrupts = <0 59 1>;
> +			tx-fifo-depth = <0x40>;
> +			rx-fifo-depth = <0x40>;
> +		};
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index 9e7d95d..b180239 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -125,6 +125,13 @@ config CAN_GRCAN
>  	  endian syntheses of the cores would need some modifications on
>  	  the hardware level to work.
>  
> +config CAN_XILINXCAN
> +	tristate "Xilinx CAN"
> +	depends on ARCH_ZYNQ || MICROBLAZE

Is Zynq multiarch already?

> +	---help---
> +	  Xilinx CAN driver. This driver supports both soft AXI CAN IP and
> +	  Zynq CANPS IP.
> +
>  source "drivers/net/can/mscan/Kconfig"
>  
>  source "drivers/net/can/sja1000/Kconfig"
> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> index c744039..0b8e11e 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -25,5 +25,6 @@ obj-$(CONFIG_CAN_JANZ_ICAN3)	+= janz-ican3.o
>  obj-$(CONFIG_CAN_FLEXCAN)	+= flexcan.o
>  obj-$(CONFIG_PCH_CAN)		+= pch_can.o
>  obj-$(CONFIG_CAN_GRCAN)		+= grcan.o
> +obj-$(CONFIG_CAN_XILINXCAN)	+= xilinx_can.o
>  
>  ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> new file mode 100644
> index 0000000..a8552a2
> --- /dev/null
> +++ b/drivers/net/can/xilinx_can.c
> @@ -0,0 +1,1195 @@
> +/* Xilinx CAN device driver
> + *
> + * Copyright (C) 2012 - 2014 Xilinx, Inc.
> + * Copyright (C) 2009 PetaLogix. All rights reserved.
> + *
> + * Description:
> + * This driver is developed for Axi CAN IP and for Zynq CANPS Controller.
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/netdevice.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/skbuff.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/can/dev.h>
> +#include <linux/can/error.h>
> +#include <linux/can/led.h>
> +
> +#define DRIVER_NAME	"XILINX_CAN"
> +
> +/* CAN registers set */
> +enum xcan_reg {
> +	XCAN_SRR_OFFSET		= 0x00, /* Software reset */
> +	XCAN_MSR_OFFSET		= 0x04, /* Mode select */
> +	XCAN_BRPR_OFFSET	= 0x08, /* Baud rate prescaler */
> +	XCAN_BTR_OFFSET		= 0x0C, /* Bit timing */
> +	XCAN_ECR_OFFSET		= 0x10, /* Error counter */
> +	XCAN_ESR_OFFSET		= 0x14, /* Error status */
> +	XCAN_SR_OFFSET		= 0x18, /* Status */
> +	XCAN_ISR_OFFSET		= 0x1C, /* Interrupt status */
> +	XCAN_IER_OFFSET		= 0x20, /* Interrupt enable */
> +	XCAN_ICR_OFFSET		= 0x24, /* Interrupt clear */
> +	XCAN_TXFIFO_ID_OFFSET	= 0x30,/* TX FIFO ID */
> +	XCAN_TXFIFO_DLC_OFFSET	= 0x34, /* TX FIFO DLC */
> +	XCAN_TXFIFO_DW1_OFFSET	= 0x38, /* TX FIFO Data Word 1 */
> +	XCAN_TXFIFO_DW2_OFFSET	= 0x3C, /* TX FIFO Data Word 2 */
> +	XCAN_RXFIFO_ID_OFFSET	= 0x50, /* RX FIFO ID */
> +	XCAN_RXFIFO_DLC_OFFSET	= 0x54, /* RX FIFO DLC */
> +	XCAN_RXFIFO_DW1_OFFSET	= 0x58, /* RX FIFO Data Word 1 */
> +	XCAN_RXFIFO_DW2_OFFSET	= 0x5C, /* RX FIFO Data Word 2 */
> +};
> +
> +/* CAN register bit masks - XCAN_<REG>_<BIT>_MASK */
> +#define XCAN_SRR_CEN_MASK		0x00000002 /* CAN enable */
> +#define XCAN_SRR_RESET_MASK		0x00000001 /* Soft Reset the CAN core */
> +#define XCAN_MSR_LBACK_MASK		0x00000002 /* Loop back mode select */
> +#define XCAN_MSR_SLEEP_MASK		0x00000001 /* Sleep mode select */
> +#define XCAN_BRPR_BRP_MASK		0x000000FF /* Baud rate prescaler */
> +#define XCAN_BTR_SJW_MASK		0x00000180 /* Synchronous jump width */
> +#define XCAN_BTR_TS2_MASK		0x00000070 /* Time segment 2 */
> +#define XCAN_BTR_TS1_MASK		0x0000000F /* Time segment 1 */
> +#define XCAN_ECR_REC_MASK		0x0000FF00 /* Receive error counter */
> +#define XCAN_ECR_TEC_MASK		0x000000FF /* Transmit error counter */
> +#define XCAN_ESR_ACKER_MASK		0x00000010 /* ACK error */
> +#define XCAN_ESR_BERR_MASK		0x00000008 /* Bit error */
> +#define XCAN_ESR_STER_MASK		0x00000004 /* Stuff error */
> +#define XCAN_ESR_FMER_MASK		0x00000002 /* Form error */
> +#define XCAN_ESR_CRCER_MASK		0x00000001 /* CRC error */
> +#define XCAN_SR_TXFLL_MASK		0x00000400 /* TX FIFO is full */
> +#define XCAN_SR_ESTAT_MASK		0x00000180 /* Error status */
> +#define XCAN_SR_ERRWRN_MASK		0x00000040 /* Error warning */
> +#define XCAN_SR_NORMAL_MASK		0x00000008 /* Normal mode */
> +#define XCAN_SR_LBACK_MASK		0x00000002 /* Loop back mode */
> +#define XCAN_SR_CONFIG_MASK		0x00000001 /* Configuration mode */
> +#define XCAN_IXR_TXFEMP_MASK		0x00004000 /* TX FIFO Empty */
> +#define XCAN_IXR_WKUP_MASK		0x00000800 /* Wake up interrupt */
> +#define XCAN_IXR_SLP_MASK		0x00000400 /* Sleep interrupt */
> +#define XCAN_IXR_BSOFF_MASK		0x00000200 /* Bus off interrupt */
> +#define XCAN_IXR_ERROR_MASK		0x00000100 /* Error interrupt */
> +#define XCAN_IXR_RXNEMP_MASK		0x00000080 /* RX FIFO NotEmpty intr */
> +#define XCAN_IXR_RXOFLW_MASK		0x00000040 /* RX FIFO Overflow intr */
> +#define XCAN_IXR_RXOK_MASK		0x00000010 /* Message received intr */
> +#define XCAN_IXR_TXFLL_MASK		0x00000004 /* Tx FIFO Full intr */
> +#define XCAN_IXR_TXOK_MASK		0x00000002 /* TX successful intr */
> +#define XCAN_IXR_ARBLST_MASK		0x00000001 /* Arbitration lost intr */
> +#define XCAN_IDR_ID1_MASK		0xFFE00000 /* Standard msg identifier */
> +#define XCAN_IDR_SRR_MASK		0x00100000 /* Substitute remote TXreq */
> +#define XCAN_IDR_IDE_MASK		0x00080000 /* Identifier extension */
> +#define XCAN_IDR_ID2_MASK		0x0007FFFE /* Extended message ident */
> +#define XCAN_IDR_RTR_MASK		0x00000001 /* Remote TX request */
> +#define XCAN_DLCR_DLC_MASK		0xF0000000 /* Data length code */
> +
> +#define XCAN_INTR_ALL		(XCAN_IXR_TXOK_MASK | XCAN_IXR_BSOFF_MASK |\
> +				 XCAN_IXR_WKUP_MASK | XCAN_IXR_SLP_MASK | \
> +				 XCAN_IXR_RXNEMP_MASK | XCAN_IXR_ERROR_MASK | \
> +				 XCAN_IXR_ARBLST_MASK | XCAN_IXR_RXOK_MASK)
> +
> +/* CAN register bit shift - XCAN_<REG>_<BIT>_SHIFT */
> +#define XCAN_BTR_SJW_SHIFT		7  /* Synchronous jump width */
> +#define XCAN_BTR_TS2_SHIFT		4  /* Time segment 2 */
> +#define XCAN_IDR_ID1_SHIFT		21 /* Standard Messg Identifier */
> +#define XCAN_IDR_ID2_SHIFT		1  /* Extended Message Identifier */
> +#define XCAN_DLCR_DLC_SHIFT		28 /* Data length code */
> +#define XCAN_ESR_REC_SHIFT		8  /* Rx Error Count */
> +
> +/* CAN frame length constants */
> +#define XCAN_ECHO_SKB_MAX		64
> +#define XCAN_FRAME_MAX_DATA_LEN		8
> +#define XCAN_TIMEOUT			(1 * HZ)
> +
> +/**
> + * struct xcan_priv - This definition define CAN driver instance
> + * @can:			CAN private data structure.
> + * @tx_head:			Tx CAN packets ready to send on the queue
> + * @tx_tail:			Tx CAN packets successfully sended on the queue
> + * @tx_max:			Maximum number packets the driver can send
> + * @napi:			NAPI structure
> + * @read_reg:			For reading data from CAN registers
> + * @write_reg:			For writing data to CAN registers
> + * @dev:			Network device data structure
> + * @reg_base:			Ioremapped address to registers
> + * @irq_flags:			For request_irq()
> + * @aperclk:			Pointer to struct clk
> + * @devclk:			Pointer to struct clk
> + */
> +struct xcan_priv {
> +	struct can_priv can;
> +	unsigned int tx_head;
> +	unsigned int tx_tail;
> +	u32 tx_max;
Please make it an unsigned int, too.

> +	struct napi_struct napi;
> +	u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
> +	void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
> +			u32 val);
> +	struct net_device *dev;
> +	void __iomem *reg_base;
> +	unsigned long irq_flags;
> +	struct clk *aperclk;
> +	struct clk *devclk;

Please rename the clock variables to match the names in the DT.

> +};
> +
> +/* CAN Bittiming constants as per Xilinx CAN specs */
> +static const struct can_bittiming_const xcan_bittiming_const = {
> +	.name = DRIVER_NAME,
> +	.tseg1_min = 1,
> +	.tseg1_max = 16,
> +	.tseg2_min = 1,
> +	.tseg2_max = 8,
> +	.sjw_max = 4,
> +	.brp_min = 1,
> +	.brp_max = 256,
> +	.brp_inc = 1,
> +};
> +
> +/**
> + * xcan_write_reg_le - Write a value to the device register little endian
> + * @priv:	Driver private data structure
> + * @reg:	Register offset
> + * @val:	Value to write at the Register offset
> + *
> + * Write data to the paricular CAN register
> + */
> +static void xcan_write_reg_le(const struct xcan_priv *priv, enum xcan_reg reg,
> +			u32 val)
> +{
> +	iowrite32(val, priv->reg_base + reg);
> +}
> +
> +/**
> + * xcan_read_reg_le - Read a value from the device register little endian
> + * @priv:	Driver private data structure
> + * @reg:	Register offset
> + *
> + * Read data from the particular CAN register
> + * Return: value read from the CAN register
> + */
> +static u32 xcan_read_reg_le(const struct xcan_priv *priv, enum xcan_reg reg)
> +{
> +	return ioread32(priv->reg_base + reg);
> +}
> +
> +/**
> + * xcan_write_reg_be - Write a value to the device register big endian
> + * @priv:	Driver private data structure
> + * @reg:	Register offset
> + * @val:	Value to write at the Register offset
> + *
> + * Write data to the paricular CAN register
> + */
> +static void xcan_write_reg_be(const struct xcan_priv *priv, enum xcan_reg reg,
> +			u32 val)
> +{
> +	iowrite32be(val, priv->reg_base + reg);
> +}
> +
> +/**
> + * xcan_read_reg_be - Read a value from the device register big endian
> + * @priv:	Driver private data structure
> + * @reg:	Register offset
> + *
> + * Read data from the particular CAN register
> + * Return: value read from the CAN register
> + */
> +static u32 xcan_read_reg_be(const struct xcan_priv *priv, enum xcan_reg reg)
> +{
> +	return ioread32be(priv->reg_base + reg);
> +}
> +
> +/**
> + * set_reset_mode - Resets the CAN device mode
> + * @ndev:	Pointer to net_device structure
> + *
> + * This is the driver reset mode routine.The driver
> + * enters into configuration mode.
> + *
> + * Return: 0 on success and failure value on error
> + */
> +static int set_reset_mode(struct net_device *ndev)
> +{
> +	struct xcan_priv *priv = netdev_priv(ndev);
> +	unsigned long timeout;
> +
> +	priv->can.state = CAN_STATE_STOPPED;

I don't think changeing the state makes sense here. You evaluate the
priv->can.state in xcan_chip_start(), but you return if this function
returns a error.

> +
> +	timeout = jiffies + XCAN_TIMEOUT;
> +	while (!(priv->read_reg(priv, XCAN_SR_OFFSET) & XCAN_SR_CONFIG_MASK)) {
> +		if (time_after(jiffies, timeout)) {
> +			netdev_warn(ndev, "timedout waiting for config mode\n");
> +			return -ETIMEDOUT;
> +		}
> +		usleep_range(500, 10000);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * xcan_set_bittiming - CAN set bit timing routine
> + * @ndev:	Pointer to net_device structure
> + *
> + * This is the driver set bittiming  routine.
> + * Return: 0 on success and failure value on error
> + */
> +static int xcan_set_bittiming(struct net_device *ndev)
> +{
> +	struct xcan_priv *priv = netdev_priv(ndev);
> +	struct can_bittiming *bt = &priv->can.bittiming;
> +	u32 btr0, btr1;
> +	u32 is_config_mode;
> +
> +	/* Check whether Xilinx CAN is in configuration mode.
> +	 * It cannot set bit timing if Xilinx CAN is not in configuration mode.
> +	 */
> +	is_config_mode = priv->read_reg(priv, XCAN_SR_OFFSET) &
> +				XCAN_SR_CONFIG_MASK;
> +	if (!is_config_mode) {
> +		netdev_alert(ndev,
> +			"Cannot set bittiming can is not in config mode\n");
> +		return -EPERM;
> +	}

You only call set_bittiming from xcan_chip_start() and go into config
mode before. So if this triggers there must be a bug in the driver, you
could add "BUG!" to the output.

> +
> +	/* Setting Baud Rate prescalar value in BRPR Register */
> +	btr0 = (bt->brp - 1);
> +
> +	/* Setting Time Segment 1 in BTR Register */
> +	btr1 = (bt->prop_seg + bt->phase_seg1 - 1);
> +
> +	/* Setting Time Segment 2 in BTR Register */
> +	btr1 |= (bt->phase_seg2 - 1) << XCAN_BTR_TS2_SHIFT;
> +
> +	/* Setting Synchronous jump width in BTR Register */
> +	btr1 |= (bt->sjw - 1) << XCAN_BTR_SJW_SHIFT;
> +
> +	priv->write_reg(priv, XCAN_BRPR_OFFSET, btr0);
> +	priv->write_reg(priv, XCAN_BTR_OFFSET, btr1);
> +
> +	netdev_dbg(ndev, "BRPR=0x%08x, BTR=0x%08x\n",
> +			priv->read_reg(priv, XCAN_BRPR_OFFSET),
> +			priv->read_reg(priv, XCAN_BTR_OFFSET));
> +
> +	return 0;
> +}
> +
> +/**
> + * xcan_chip_start - This the drivers start routine
> + * @ndev:	Pointer to net_device structure
> + *
> + * This is the drivers start routine.
> + * Based on the State of the CAN device it puts
> + * the CAN device into a proper mode.
> + *
> + * Return: 0 on success and failure value on error
> + */
> +static int xcan_chip_start(struct net_device *ndev)
> +{
> +	struct xcan_priv *priv = netdev_priv(ndev);
> +	u32 err;
> +	unsigned long timeout;
> +
> +	/* Check if it is in reset mode */
> +	err = set_reset_mode(ndev);
> +	if (err < 0)
> +		return err;
> +
> +	err = xcan_set_bittiming(ndev);
> +	if (err < 0)
> +		return err;
> +
> +	/* Enable interrupts */
> +	priv->write_reg(priv, XCAN_IER_OFFSET, XCAN_INTR_ALL);
> +
> +	/* Check whether it is loopback mode or normal mode  */
> +	if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
> +		/* Put device into loopback mode */
> +		priv->write_reg(priv, XCAN_MSR_OFFSET, XCAN_MSR_LBACK_MASK);
> +	else
> +		/* The device is in normal mode */
> +		priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> +
> +	if (priv->can.state == CAN_STATE_STOPPED) {

Your device is in stopped mode, add you go though set_reset_mode() already.

> +		/* Enable Xilinx CAN */
> +		priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> +		priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +		timeout = jiffies + XCAN_TIMEOUT;
> +		if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
> +			while ((priv->read_reg(priv, XCAN_SR_OFFSET)
> +					& XCAN_SR_LBACK_MASK) == 0) {
> +				if (time_after(jiffies, timeout)) {
> +					netdev_warn(ndev,
> +						"timedout for loopback mode\n");
> +					return -ETIMEDOUT;
> +				}
> +				usleep_range(500, 10000);
> +			}
> +		} else {
> +			while ((priv->read_reg(priv, XCAN_SR_OFFSET)
> +					& XCAN_SR_NORMAL_MASK) == 0) {
> +				if (time_after(jiffies, timeout)) {
> +					netdev_warn(ndev,
> +						"timedout for normal mode\n");
> +					return -ETIMEDOUT;
> +				}
> +				usleep_range(500, 10000);
> +			}
> +		}
> +		netdev_dbg(ndev, "status:#x%08x\n",
> +				priv->read_reg(priv, XCAN_SR_OFFSET));
> +	}
> +	priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +	return 0;
> +}
> +
> +/**
> + * xcan_do_set_mode - This sets the mode of the driver
> + * @ndev:	Pointer to net_device structure
> + * @mode:	Tells the mode of the driver
> + *
> + * This check the drivers state and calls the
> + * the corresponding modes to set.
> + *
> + * Return: 0 on success and failure value on error
> + */
> +static int xcan_do_set_mode(struct net_device *ndev, enum can_mode mode)
> +{
> +	int ret;
> +
> +	switch (mode) {
> +	case CAN_MODE_START:
> +		ret = xcan_chip_start(ndev);
> +		if (ret < 0)
> +			netdev_err(ndev, "xcan_chip_start failed!\n");
> +		netif_wake_queue(ndev);

Don't wake the queue if, chip_start() fails.

> +		break;
> +	default:
> +		ret = -EOPNOTSUPP;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +/**
> + * xcan_start_xmit - Starts the transmission
> + * @skb:	sk_buff pointer that contains data to be Txed
> + * @ndev:	Pointer to net_device structure
> + *
> + * This function is invoked from upper layers to initiate transmission. This
> + * function uses the next available free txbuff and populates their fields to
> + * start the transmission.
> + *
> + * Return: 0 on success and failure value on error
> + */
> +static int xcan_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> +{
> +	struct xcan_priv *priv = netdev_priv(ndev);
> +	struct net_device_stats *stats = &ndev->stats;
> +	struct can_frame *cf = (struct can_frame *)skb->data;
> +	u32 id, dlc, data[2] = {0, 0};
> +
> +	if (can_dropped_invalid_skb(ndev, skb))
> +		return NETDEV_TX_OK;
> +
> +	/* Check if the TX buffer is full */
> +	if (unlikely(priv->read_reg(priv, XCAN_SR_OFFSET) &
> +			XCAN_SR_TXFLL_MASK)) {
> +		netif_stop_queue(ndev);
> +		netdev_err(ndev, "BUG!, TX FIFO full when queue awake!\n");
> +		return NETDEV_TX_BUSY;
> +	}
> +
> +	/* Watch carefully on the bit sequence */
> +	if (cf->can_id & CAN_EFF_FLAG) {
> +		/* Extended CAN ID format */
> +		id = ((cf->can_id & CAN_EFF_MASK) << XCAN_IDR_ID2_SHIFT) &
> +			XCAN_IDR_ID2_MASK;
> +		id |= (((cf->can_id & CAN_EFF_MASK) >>
> +			(CAN_EFF_ID_BITS-CAN_SFF_ID_BITS)) <<
> +			XCAN_IDR_ID1_SHIFT) & XCAN_IDR_ID1_MASK;
> +
> +		/* The substibute remote TX request bit should be "1"
> +		 * for extended frames as in the Xilinx CAN datasheet
> +		 */
> +		id |= XCAN_IDR_IDE_MASK | XCAN_IDR_SRR_MASK;
> +
> +		if (cf->can_id & CAN_RTR_FLAG)
> +			/* Extended frames remote TX request */
> +			id |= XCAN_IDR_RTR_MASK;
> +	} else {
> +		/* Standard CAN ID format */
> +		id = ((cf->can_id & CAN_SFF_MASK) << XCAN_IDR_ID1_SHIFT) &
> +			XCAN_IDR_ID1_MASK;
> +
> +		if (cf->can_id & CAN_RTR_FLAG)
> +			/* Standard frames remote TX request */
> +			id |= XCAN_IDR_SRR_MASK;
> +	}
> +
> +	dlc = cf->can_dlc << XCAN_DLCR_DLC_SHIFT;
> +
> +	if (cf->can_dlc > 0)
> +		data[0] = be32_to_cpup((__be32 *)(cf->data + 0));
> +	if (cf->can_dlc > 4)
> +		data[1] = be32_to_cpup((__be32 *)(cf->data + 4));
> +
> +	can_put_echo_skb(skb, ndev, priv->tx_head % priv->tx_max);
> +	priv->tx_head++;
> +
> +	/* Write the Frame to Xilinx CAN TX FIFO */
> +	priv->write_reg(priv, XCAN_TXFIFO_ID_OFFSET, id);
> +	/* If the CAN frame is RTR frame this write triggers tranmission */
> +	priv->write_reg(priv, XCAN_TXFIFO_DLC_OFFSET, dlc);
> +	if (!(cf->can_id & CAN_RTR_FLAG)) {
> +		priv->write_reg(priv, XCAN_TXFIFO_DW1_OFFSET, data[0]);
> +		/* If the CAN frame is Standard/Extended frame this
> +		 * write triggers tranmission
> +		 */
> +		priv->write_reg(priv, XCAN_TXFIFO_DW2_OFFSET, data[1]);
> +		stats->tx_bytes += cf->can_dlc;
> +	}
> +
> +	/* Check if the TX buffer is full */
> +	if ((priv->tx_head - priv->tx_tail) == priv->tx_max)
> +		netif_stop_queue(ndev);
> +
> +	return NETDEV_TX_OK;
> +}
> +
> +/**
> + * xcan_rx -  Is called from CAN isr to complete the received
> + *		frame  processing
> + * @ndev:	Pointer to net_device structure
> + *
> + * This function is invoked from the CAN isr(poll) to process the Rx frames. It
> + * does minimal processing and invokes "netif_receive_skb" to complete further
> + * processing.
> + * Return: 0 on success and negative error value on error
> + */
> +static int xcan_rx(struct net_device *ndev)
> +{
> +	struct xcan_priv *priv = netdev_priv(ndev);
> +	struct net_device_stats *stats = &ndev->stats;
> +	struct can_frame *cf;
> +	struct sk_buff *skb;
> +	u32 id_xcan, dlc, data[2] = {0, 0};
> +
> +	skb = alloc_can_skb(ndev, &cf);
> +	if (!skb)
> +		return -ENOMEM;

This would only happen under heavy memory preassure, so it's probably
better to drop the frame and increase the rx_dropped counter:

		stats->rx_dropped++;

You can make this function return the number of received CAN frames, see
flexcan.c.

> +
> +	/* Read a frame from Xilinx zynq CANPS */
> +	id_xcan = priv->read_reg(priv, XCAN_RXFIFO_ID_OFFSET);
> +	dlc = priv->read_reg(priv, XCAN_RXFIFO_DLC_OFFSET) >>
> +				XCAN_DLCR_DLC_SHIFT;
> +
> +	/* Change Xilinx CAN data length format to socketCAN data format */
> +	cf->can_dlc = get_can_dlc(dlc);
> +
> +	/* Change Xilinx CAN ID format to socketCAN ID format */
> +	if (id_xcan & XCAN_IDR_IDE_MASK) {
> +		/* The received frame is an Extended format frame */
> +		cf->can_id = (id_xcan & XCAN_IDR_ID1_MASK) >> 3;
> +		cf->can_id |= (id_xcan & XCAN_IDR_ID2_MASK) >>
> +				XCAN_IDR_ID2_SHIFT;
> +		cf->can_id |= CAN_EFF_FLAG;
> +		if (id_xcan & XCAN_IDR_RTR_MASK)
> +			cf->can_id |= CAN_RTR_FLAG;
> +	} else {
> +		/* The received frame is a standard format frame */
> +		cf->can_id = (id_xcan & XCAN_IDR_ID1_MASK) >>
> +				XCAN_IDR_ID1_SHIFT;
> +		if (id_xcan & XCAN_IDR_RTR_MASK)
> +			cf->can_id |= CAN_RTR_FLAG;
> +	}
> +
> +	if (!(id_xcan & XCAN_IDR_RTR_MASK)) {
> +		data[0] = priv->read_reg(priv, XCAN_RXFIFO_DW1_OFFSET);
> +		data[1] = priv->read_reg(priv, XCAN_RXFIFO_DW2_OFFSET);
> +
> +		/* Change Xilinx CAN data format to socketCAN data format */
		if (cf->can_dlc > 0)
> +		*(__be32 *)(cf->data) = cpu_to_be32(data[0]);
> +		if (cf->can_dlc > 4)
> +			*(__be32 *)(cf->data + 4) = cpu_to_be32(data[1]);
> +	}
> +	can_led_event(ndev, CAN_LED_EVENT_RX);
> +
> +	netif_receive_skb(skb);
> +
> +	stats->rx_bytes += cf->can_dlc;
> +	stats->rx_packets++;
> +	return 0;
> +}
> +
> +/**
> + * xcan_err_interrupt - error frame Isr
> + * @ndev:	net_device pointer
> + * @isr:	interrupt status register value
> + *
> + * This is the CAN error interrupt and it will
> + * check the the type of error and forward the error
> + * frame to upper layers.
> + */
> +static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
> +{
> +	struct xcan_priv *priv = netdev_priv(ndev);
> +	struct net_device_stats *stats = &ndev->stats;
> +	struct can_frame *cf;
> +	struct sk_buff *skb;
> +	u32 err_status, status;
> +
> +	skb = alloc_can_err_skb(ndev, &cf);
> +	if (!skb) {
> +		netdev_err(ndev, "alloc_can_err_skb() failed!\n");
> +		return;
> +	}
> +
> +	err_status = priv->read_reg(priv, XCAN_ESR_OFFSET);
> +	priv->write_reg(priv, XCAN_ESR_OFFSET, err_status);
> +	status = priv->read_reg(priv, XCAN_SR_OFFSET);
> +
> +	if (isr & XCAN_IXR_BSOFF_MASK) {
> +		priv->can.state = CAN_STATE_BUS_OFF;
> +		cf->can_id |= CAN_ERR_BUSOFF;
> +		priv->can.can_stats.bus_off++;
> +		/* Leave device in Config Mode in bus-off state */
> +		priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_RESET_MASK);
> +		can_bus_off(ndev);
> +	} else if ((status & XCAN_SR_ESTAT_MASK) == XCAN_SR_ESTAT_MASK) {
> +		cf->can_id |= CAN_ERR_CRTL;
> +		priv->can.state = CAN_STATE_ERROR_PASSIVE;
> +		priv->can.can_stats.error_passive++;
> +		cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE |
> +					CAN_ERR_CRTL_TX_PASSIVE;
> +	} else if (status & XCAN_SR_ERRWRN_MASK) {
> +		cf->can_id |= CAN_ERR_CRTL;
> +		priv->can.state = CAN_STATE_ERROR_WARNING;
> +		priv->can.can_stats.error_warning++;
> +		cf->data[1] |= CAN_ERR_CRTL_RX_WARNING |
> +					CAN_ERR_CRTL_TX_WARNING;
> +	}
> +
> +	/* Check for Arbitration lost interrupt */
> +	if (isr & XCAN_IXR_ARBLST_MASK) {
> +		cf->can_id |= CAN_ERR_LOSTARB;
> +		cf->data[0] = CAN_ERR_LOSTARB_UNSPEC;
> +		priv->can.can_stats.arbitration_lost++;
> +	}
> +
> +	/* Check for RX FIFO Overflow interrupt */
> +	if (isr & XCAN_IXR_RXOFLW_MASK) {
> +		cf->can_id |= CAN_ERR_CRTL;
> +		cf->data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
> +		stats->rx_over_errors++;
> +		stats->rx_errors++;
> +		priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_RESET_MASK);
> +	}
> +
> +	/* Check for error interrupt */
> +	if (isr & XCAN_IXR_ERROR_MASK) {
> +		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> +		cf->data[2] |= CAN_ERR_PROT_UNSPEC;
> +
> +		/* Check for Ack error interrupt */
> +		if (err_status & XCAN_ESR_ACKER_MASK) {
> +			cf->can_id |= CAN_ERR_ACK;
> +			cf->data[3] |= CAN_ERR_PROT_LOC_ACK;
> +			stats->tx_errors++;
> +		}
> +
> +		/* Check for Bit error interrupt */
> +		if (err_status & XCAN_ESR_BERR_MASK) {
> +			cf->can_id |= CAN_ERR_PROT;
> +			cf->data[2] = CAN_ERR_PROT_BIT;
> +			stats->tx_errors++;
> +		}
> +
> +		/* Check for Stuff error interrupt */
> +		if (err_status & XCAN_ESR_STER_MASK) {
> +			cf->can_id |= CAN_ERR_PROT;
> +			cf->data[2] = CAN_ERR_PROT_STUFF;
> +			stats->rx_errors++;
> +		}
> +
> +		/* Check for Form error interrupt */
> +		if (err_status & XCAN_ESR_FMER_MASK) {
> +			cf->can_id |= CAN_ERR_PROT;
> +			cf->data[2] = CAN_ERR_PROT_FORM;
> +			stats->rx_errors++;
> +		}
> +
> +		/* Check for CRC error interrupt */
> +		if (err_status & XCAN_ESR_CRCER_MASK) {
> +			cf->can_id |= CAN_ERR_PROT;
> +			cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ |
> +					CAN_ERR_PROT_LOC_CRC_DEL;
> +			stats->rx_errors++;
> +		}
> +			priv->can.can_stats.bus_error++;
> +	}
> +
> +	netif_rx(skb);
> +	stats->rx_packets++;
> +	stats->rx_bytes += cf->can_dlc;
> +
> +	netdev_dbg(ndev, "%s: error status register:0x%x\n",
> +			__func__, priv->read_reg(priv, XCAN_ESR_OFFSET));
> +}
> +
> +/**
> + * xcan_state_interrupt - It will check the state of the CAN device
> + * @ndev:	net_device pointer
> + * @isr:	interrupt status register value
> + *
> + * This will checks the state of the CAN device
> + * and puts the device into appropriate state.
> + */
> +static void xcan_state_interrupt(struct net_device *ndev, u32 isr)
> +{
> +	struct xcan_priv *priv = netdev_priv(ndev);
> +
> +	/* Check for Sleep interrupt if set put CAN device in sleep state */
> +	if (isr & XCAN_IXR_SLP_MASK)
> +		priv->can.state = CAN_STATE_SLEEPING;
> +
> +	/* Check for Wake up interrupt if set put CAN device in Active state */
> +	if (isr & XCAN_IXR_WKUP_MASK)
> +		priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +}
> +
> +/**
> + * xcan_rx_poll - Poll routine for rx packets (NAPI)
> + * @napi:	napi structure pointer
> + * @quota:	Max number of rx packets to be processed.
> + *
> + * This is the poll routine for rx part.
> + * It will process the packets maximux quota value.
> + *
> + * Return: number of packets received
> + */
> +static int xcan_rx_poll(struct napi_struct *napi, int quota)
> +{
> +	struct net_device *ndev = napi->dev;
> +	struct xcan_priv *priv = netdev_priv(ndev);
> +	u32 isr, ier;
> +	int work_done = 0;
> +
> +	isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
> +	while ((isr & XCAN_IXR_RXNEMP_MASK) && (work_done < quota)) {
> +		if (isr & XCAN_IXR_RXOK_MASK) {
> +			priv->write_reg(priv, XCAN_ICR_OFFSET,
> +				XCAN_IXR_RXOK_MASK);
> +			if (xcan_rx(ndev) < 0)
> +				return work_done;

You should not leave the napi function like this, you are not allowed to
skip the if (work_done < quota) {...} part.

> +			work_done++;

I think the flexcan does it better:

			work_done += xcan_rx(ndev);

> +		} else {
> +			priv->write_reg(priv, XCAN_ICR_OFFSET,
> +				XCAN_IXR_RXNEMP_MASK);
> +			break;
> +		}
> +		priv->write_reg(priv, XCAN_ICR_OFFSET, XCAN_IXR_RXNEMP_MASK);
> +		isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
> +	}
> +
> +	if (work_done < quota) {
> +		napi_complete(napi);
> +		ier = priv->read_reg(priv, XCAN_IER_OFFSET);
> +		ier |= (XCAN_IXR_RXOK_MASK | XCAN_IXR_RXNEMP_MASK);
> +		priv->write_reg(priv, XCAN_IER_OFFSET, ier);
> +	}
> +	return work_done;
> +}
> +
> +/**
> + * xcan_tx_interrupt - Tx Done Isr
> + * @ndev:	net_device pointer
> + * @isr:	Interrupt status register value
> + */
> +static void xcan_tx_interrupt(struct net_device *ndev, u32 isr)
> +{
> +	struct xcan_priv *priv = netdev_priv(ndev);
> +	struct net_device_stats *stats = &ndev->stats;
> +
> +	while (priv->tx_head - priv->tx_tail > 0) {
> +		priv->write_reg(priv, XCAN_ICR_OFFSET, XCAN_IXR_TXOK_MASK);
> +		if (!(isr & XCAN_IXR_TXOK_MASK))
> +			break;
> +		can_get_echo_skb(ndev, priv->tx_tail %
> +					priv->tx_max);
> +		priv->tx_tail++;
> +		stats->tx_packets++;
> +		can_led_event(ndev, CAN_LED_EVENT_TX);
> +		isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
> +	}
> +	netif_wake_queue(ndev);
> +}
> +
> +/**
> + * xcan_interrupt - CAN Isr
> + * @irq:	irq number
> + * @dev_id:	device id poniter
> + *
> + * This is the xilinx CAN Isr. It checks for the type of interrupt
> + * and invokes the corresponding ISR.
> + *
> + * Return:
> + * IRQ_NONE - If CAN device is in sleep mode, IRQ_HANDLED otherwise
> + */
> +static irqreturn_t xcan_interrupt(int irq, void *dev_id)
> +{
> +	struct net_device *ndev = (struct net_device *)dev_id;
> +	struct xcan_priv *priv = netdev_priv(ndev);
> +	u32 isr, ier;
> +
> +	/* Get the interrupt status from Xilinx CAN */
> +	isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
> +	if (!isr)
> +		return IRQ_NONE;
> +
> +	netdev_dbg(ndev, "%s: isr:#x%08x, err:#x%08x\n", __func__,
> +			isr, priv->read_reg(priv, XCAN_ESR_OFFSET));

Please remove this debug output.

> +	/* Check for the type of interrupt and Processing it */
> +	if (isr & (XCAN_IXR_SLP_MASK | XCAN_IXR_WKUP_MASK)) {
> +		priv->write_reg(priv, XCAN_ICR_OFFSET, (XCAN_IXR_SLP_MASK |
> +				XCAN_IXR_WKUP_MASK));
> +		xcan_state_interrupt(ndev, isr);
> +	}
> +
> +	/* Check for Tx interrupt and Processing it */
> +	if (isr & XCAN_IXR_TXOK_MASK) {
> +		priv->write_reg(priv, XCAN_ICR_OFFSET, XCAN_IXR_TXOK_MASK);

I think this write is now completely inside tx_interrupt()?

> +		xcan_tx_interrupt(ndev, isr);
> +	}
> +
> +	/* Check for the type of error interrupt and Processing it */
> +	if (isr & (XCAN_IXR_ERROR_MASK | XCAN_IXR_RXOFLW_MASK |
> +			XCAN_IXR_BSOFF_MASK | XCAN_IXR_ARBLST_MASK)) {
> +		priv->write_reg(priv, XCAN_ICR_OFFSET, (XCAN_IXR_ERROR_MASK |
> +				XCAN_IXR_RXOFLW_MASK | XCAN_IXR_BSOFF_MASK |
> +				XCAN_IXR_ARBLST_MASK));
> +		xcan_err_interrupt(ndev, isr);
> +	}
> +
> +	/* Check for the type of receive interrupt and Processing it */
> +	if (isr & (XCAN_IXR_RXNEMP_MASK | XCAN_IXR_RXOK_MASK)) {
> +		ier = priv->read_reg(priv, XCAN_IER_OFFSET);
> +		ier &= ~(XCAN_IXR_RXNEMP_MASK | XCAN_IXR_RXOK_MASK);
> +		priv->write_reg(priv, XCAN_IER_OFFSET, ier);
> +		napi_schedule(&priv->napi);
> +	}
> +	return IRQ_HANDLED;
> +}
> +
> +/**
> + * xcan_chip_stop - Driver stop routine
> + * @ndev:	Pointer to net_device structure
> + *
> + * This is the drivers stop routine. It will disable the
> + * interrupts and put the device into configuration mode.
> + */
> +static void xcan_chip_stop(struct net_device *ndev)
> +{
> +	struct xcan_priv *priv = netdev_priv(ndev);
> +	u32 ier;
> +
> +	/* Disable interrupts and leave the can in configuration mode */
> +	ier = priv->read_reg(priv, XCAN_IER_OFFSET);
> +	ier &= ~XCAN_INTR_ALL;
> +	priv->write_reg(priv, XCAN_IER_OFFSET, ier);
> +	priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_RESET_MASK);
> +	priv->can.state = CAN_STATE_STOPPED;
> +}
> +
> +/**
> + * xcan_open - Driver open routine
> + * @ndev:	Pointer to net_device structure
> + *
> + * This is the driver open routine.
> + * Return: 0 on success and failure value on error
> + */
> +static int xcan_open(struct net_device *ndev)
> +{
> +	struct xcan_priv *priv = netdev_priv(ndev);
> +	int ret;
> +
> +	ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
> +			ndev->name, ndev);
> +	if (ret < 0) {
> +		netdev_err(ndev, "Irq allocation for CAN failed\n");
> +		goto err;
> +	}
> +
> +	ret = clk_prepare_enable(priv->devclk);
> +	if (ret) {
> +		netdev_err(ndev, "unable to enable device clock\n");
> +		goto err;
> +	}
> +
> +	ret = clk_prepare_enable(priv->aperclk);
> +	if (ret) {
> +		netdev_err(ndev, "unable to enable aper clock\n");
> +		goto err_aperclk;
> +	}
> +
> +	/* Set chip into reset mode */
> +	ret = set_reset_mode(ndev);
> +	if (ret < 0) {
> +		netdev_err(ndev, "mode resetting failed failed!\n");
> +		goto err_devclk;
> +	}
> +
> +	/* Common open */
> +	ret = open_candev(ndev);
> +	if (ret)
> +		goto err_devclk;
> +
> +	ret = xcan_chip_start(ndev);
> +	if (ret < 0) {
> +		netdev_err(ndev, "xcan_chip_start failed!\n");
> +		goto err_devclk;
> +	}
> +
> +	can_led_event(ndev, CAN_LED_EVENT_OPEN);
> +	napi_enable(&priv->napi);
> +	netif_start_queue(ndev);
> +
> +	return 0;
> +
> +err_aperclk:
> +	free_irq(ndev->irq, ndev);
> +	clk_disable_unprepare(priv->devclk);
> +err_devclk:
> +	free_irq(ndev->irq, ndev);
> +	clk_disable_unprepare(priv->devclk);
> +	clk_disable_unprepare(priv->aperclk);

Have a look at your cleanup code, double free irq()? double disable the
dev_clk, this looks broken?

> +err:
> +	return ret;
> +}
> +
> +/**
> + * xcan_close - Driver close routine
> + * @ndev:	Pointer to net_device structure
> + *
> + * Return: 0 always
> + */
> +static int xcan_close(struct net_device *ndev)
> +{
> +	struct xcan_priv *priv = netdev_priv(ndev);
> +
> +	netif_stop_queue(ndev);
> +	napi_disable(&priv->napi);
> +	xcan_chip_stop(ndev);
> +	clk_disable_unprepare(priv->aperclk);
> +	clk_disable_unprepare(priv->devclk);
> +	free_irq(ndev->irq, ndev);
> +	close_candev(ndev);
> +
> +	can_led_event(ndev, CAN_LED_EVENT_STOP);
> +
> +	return 0;
> +}
> +
> +/**
> + * xcan_get_berr_counter - error counter routine
> + * @ndev:	Pointer to net_device structure
> + * @bec:	Pointer to can_berr_counter structure
> + *
> + * This is the driver error counter routine.
> + * Return: 0 always
> + */
> +static int xcan_get_berr_counter(const struct net_device *ndev,
> +					struct can_berr_counter *bec)
> +{
> +	struct xcan_priv *priv = netdev_priv(ndev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(priv->devclk);
> +	if (ret)
> +		goto err;
> +
> +	ret = clk_prepare_enable(priv->aperclk);
> +	if (ret)
> +		goto err_clk;
> +
> +	bec->txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) & XCAN_ECR_TEC_MASK;
> +	bec->rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) &
> +			XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT);
> +
> +	clk_disable_unprepare(priv->aperclk);
> +	clk_disable_unprepare(priv->devclk);
> +
> +	return 0;
> +
> +err_clk:
> +	clk_disable_unprepare(priv->devclk);
> +err:
> +	return ret;
> +}
> +
> +static const struct net_device_ops xcan_netdev_ops = {
> +	.ndo_open	= xcan_open,
> +	.ndo_stop	= xcan_close,
> +	.ndo_start_xmit	= xcan_start_xmit,
> +};
> +
> +#ifdef CONFIG_PM_SLEEP

please remove the ifdef...

> +/**
> + * xcan_suspend - Suspend method for the driver
> + * @_dev:	Address of the platform_device structure
> + *
> + * Put the driver into low power mode.
> + * Return: 0 always
> + */
> +static int xcan_suspend(struct device *_dev)

...add __maybe_unused instead.

> +{
> +	struct platform_device *pdev = container_of(_dev,
> +			struct platform_device, dev);

dev_get_drvdata(device);

> +	struct net_device *ndev = platform_get_drvdata(pdev);
> +	struct xcan_priv *priv = netdev_priv(ndev);
> +
> +	if (netif_running(ndev)) {
> +		netif_stop_queue(ndev);
> +		netif_device_detach(ndev);
> +	}
> +
> +	priv->write_reg(priv, XCAN_MSR_OFFSET, XCAN_MSR_SLEEP_MASK);
> +	priv->can.state = CAN_STATE_SLEEPING;
> +
> +	clk_disable(priv->aperclk);
> +	clk_disable(priv->devclk);
> +
> +	return 0;
> +}
> +
> +/**
> + * xcan_resume - Resume from suspend
> + * @dev:	Address of the platformdevice structure
> + *
> + * Resume operation after suspend.
> + * Return: 0 on success and failure value on error
> + */
> +static int xcan_resume(struct device *dev)

dito

> +{
> +	struct platform_device *pdev = container_of(dev,
> +			struct platform_device, dev);

dito

> +	struct net_device *ndev = platform_get_drvdata(pdev);
> +	struct xcan_priv *priv = netdev_priv(ndev);
> +	int ret;
> +
> +	ret = clk_enable(priv->aperclk);
> +	if (ret) {
> +		dev_err(dev, "Cannot enable clock.\n");
> +		return ret;
> +	}
> +	ret = clk_enable(priv->devclk);
> +	if (ret) {
> +		dev_err(dev, "Cannot enable clock.\n");
> +		clk_disable_unprepare(priv->aperclk);
> +		return ret;
> +	}
> +
> +	priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> +	priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> +	priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +
> +	if (netif_running(ndev)) {
> +		netif_device_attach(ndev);
> +		netif_start_queue(ndev);
> +	}
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend, xcan_resume);
> +
> +/**
> + * xcan_probe - Platform registration call
> + * @pdev:	Handle to the platform device structure
> + *
> + * This function does all the memory allocation and registration for the CAN
> + * device.
> + *
> + * Return: 0 on success and failure value on error
> + */
> +static int xcan_probe(struct platform_device *pdev)
> +{
> +	struct resource *res; /* IO mem resources */
> +	struct net_device *ndev;
> +	struct xcan_priv *priv;
> +	int ret, rx_max;
> +
> +	/* Create a CAN device instance */
> +	ndev = alloc_candev(sizeof(struct xcan_priv), XCAN_ECHO_SKB_MAX);

Can you rearange your code, so that you use tx-fifo-depth instead of a
hard coded value?

> +	if (!ndev)
> +		return -ENOMEM;
> +
> +	priv = netdev_priv(ndev);
> +	priv->dev = ndev;
> +	priv->can.bittiming_const = &xcan_bittiming_const;
> +	priv->can.do_set_mode = xcan_do_set_mode;
> +	priv->can.do_get_berr_counter = xcan_get_berr_counter;
> +	priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
> +					CAN_CTRLMODE_BERR_REPORTING;
> +
> +	/* Get IRQ for the device */
> +	ndev->irq = platform_get_irq(pdev, 0);
> +	ndev->flags |= IFF_ECHO;	/* We support local echo */
> +
> +	platform_set_drvdata(pdev, ndev);
> +	SET_NETDEV_DEV(ndev, &pdev->dev);
> +	ndev->netdev_ops = &xcan_netdev_ops;
> +
> +	/* Get the virtual base address for the device */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->reg_base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv->reg_base)) {
> +		ret = PTR_ERR(priv->reg_base);
> +		goto err_free;
> +	}
> +
> +	ret = of_property_read_u32(pdev->dev.of_node, "tx-fifo-depth",
> +				&priv->tx_max);
> +	if (ret < 0)
> +		goto err_free;
> +
> +	ret = of_property_read_u32(pdev->dev.of_node, "rx-fifo-depth", &rx_max);
> +	if (ret < 0)
> +		goto err_free;
> +
> +	/* Getting the CAN devclk info */
> +	priv->devclk = devm_clk_get(&pdev->dev, "ref_clk");
> +	if (IS_ERR(priv->devclk)) {
> +		dev_err(&pdev->dev, "Device clock not found.\n");
> +		ret = PTR_ERR(priv->devclk);
> +		goto err_free;
> +	}
> +
> +	/* Check for type of CAN device */
> +	if (of_device_is_compatible(pdev->dev.of_node,
> +				    "xlnx,zynq-can-1.00.a")) {
> +		priv->aperclk = devm_clk_get(&pdev->dev, "aper_clk");
> +		if (IS_ERR(priv->aperclk)) {
> +			dev_err(&pdev->dev, "aper clock not found\n");
> +			ret = PTR_ERR(priv->aperclk);
> +			goto err_free;
> +		}
> +	} else {
> +		priv->aperclk = priv->devclk;
> +	}
> +
> +	ret = clk_prepare_enable(priv->devclk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "unable to enable device clock\n");
> +		goto err_free;
> +	}
> +
> +	ret = clk_prepare_enable(priv->aperclk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "unable to enable aper clock\n");
> +		goto err_unprepar_disabledev;
> +	}
> +
> +	priv->write_reg = xcan_write_reg_le;
> +	priv->read_reg = xcan_read_reg_le;
> +
> +	if (priv->read_reg(priv, XCAN_SR_OFFSET) != XCAN_SR_CONFIG_MASK) {
> +		priv->write_reg = xcan_write_reg_be;
> +		priv->read_reg = xcan_read_reg_be;
> +	}
> +
> +	priv->can.clock.freq = clk_get_rate(priv->devclk);
> +
> +	netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
> +
> +	ret = register_candev(ndev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "fail to register failed (err=%d)\n", ret);
> +		goto err_unprepar_disableaper;
> +	}
> +
> +	devm_can_led_init(ndev);
> +	clk_disable_unprepare(priv->aperclk);
> +	clk_disable_unprepare(priv->devclk);
> +	netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo depth:%d\n",
> +			priv->reg_base, ndev->irq, priv->can.clock.freq,
> +			priv->tx_max);
> +
> +	return 0;
> +
> +err_unprepar_disableaper:
       typo ^^^^      ^^^^ can you add a _

err_unprepare_disable_aper:

> +	clk_disable_unprepare(priv->aperclk);
> +err_unprepar_disabledev:
> +	clk_disable_unprepare(priv->devclk);
> +err_free:
> +	free_candev(ndev);
> +
> +	return ret;
> +}
> +
> +/**
> + * xcan_remove - Unregister the device after releasing the resources
> + * @pdev:	Handle to the platform device structure
> + *
> + * This function frees all the resources allocated to the device.
> + * Return: 0 always
> + */
> +static int xcan_remove(struct platform_device *pdev)
> +{
> +	struct net_device *ndev = platform_get_drvdata(pdev);
> +	struct xcan_priv *priv = netdev_priv(ndev);
> +
> +	if (set_reset_mode(ndev) < 0)
> +		netdev_err(ndev, "mode resetting failed!\n");
> +
> +	unregister_candev(ndev);
> +	netif_napi_del(&priv->napi);
> +	free_candev(ndev);
> +
> +	return 0;
> +}
> +
> +/* Match table for OF platform binding */
> +static struct of_device_id xcan_of_match[] = {
> +	{ .compatible = "xlnx,zynq-can-1.00.a", },
> +	{ .compatible = "xlnx,axi-can-1.00.a", },
> +	{ /* end of list */ },
> +};
> +MODULE_DEVICE_TABLE(of, xcan_of_match);
> +
> +static struct platform_driver xcan_driver = {
> +	.probe = xcan_probe,
> +	.remove	= xcan_remove,
> +	.driver	= {
> +		.owner = THIS_MODULE,
> +		.name = DRIVER_NAME,
> +		.pm = &xcan_dev_pm_ops,
> +		.of_match_table	= xcan_of_match,
> +	},
> +};
> +
> +module_platform_driver(xcan_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Xilinx Inc");
> +MODULE_DESCRIPTION("Xilinx CAN interface");
> 

Marc

Hi Marc,

On 03/10/2014 03:57 PM, Marc Kleine-Budde wrote:
> On 03/04/2014 02:20 PM, Kedareswara rao Appana wrote:
>> This patch adds xilinx CAN controller support.
>> This driver supports both ZYNQ CANPS and Soft IP
>> AXI CAN controller.
>>
>> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
>> ---
>> This patch is rebased on the 3.14 rc5 kernel.
>> Changes for v5:
>> - Updated the driver with the review comments.
>> - Remove the check for the tx fifo full interrupt condition
>>   form Tx interrupt routine as we are checking it in the _xmit
>>   routine.
>> - Clearing the txok interrupt in the tx interrupt routine for
>>   every Tx can frame.
>> Changes for v4:
>> - Added check for the tx fifo full interrupt condition in
>>   Tx interrupt routine.
>> - Added be iohelper functions.
>> - Moved the clock enable/disable to probe/remove because of
>>   Added big endian support for AXI CAN controller case(reading
>>   a register during probe for that we need to enable clock).
>> Changes for v3:
>> - Updated the driver with the review comments.
>> - Modified the tranmit logic as per Marc suggestion.
>> - Enabling the clock when the interface is up to reduce the
>>   Power consumption.
>> Changes for v2:
>> - Updated with the review comments.
>> - Removed the unnecessary debug prints.
>> - include tx,rx fifo depths in ZYNQ CANPS case also.
>> ---
>>  .../devicetree/bindings/net/can/xilinx_can.txt     |   45 +
>>  drivers/net/can/Kconfig                            |    7 +
>>  drivers/net/can/Makefile                           |    1 +
>>  drivers/net/can/xilinx_can.c                       | 1195 ++++++++++++++++++++
>>  4 files changed, 1248 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..0e57103
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/can/xilinx_can.txt
>> @@ -0,0 +1,45 @@
>> +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).
>> +- tx-fifo-depth		: Can Tx fifo depth.
>> +- rx-fifo-depth		: Can Rx fifo depth.
>> +
>> +
>> +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>;
>> +			tx-fifo-depth = <0x40>;
>> +			rx-fifo-depth = <0x40>;
>> +		};
>> +For Axi CAN Dts file:
>> +	axi_can_0: axi-can@40000000 {
>> +			compatible = "xlnx,axi-can-1.00.a";
>> +			clocks = <&clkc 0>;
>> +			clock-names = "ref_clk" ;
>> +			reg = <0x40000000 0x10000>;
>> +			interrupt-parent = <&intc>;
>> +			interrupts = <0 59 1>;
>> +			tx-fifo-depth = <0x40>;
>> +			rx-fifo-depth = <0x40>;
>> +		};
>> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
>> index 9e7d95d..b180239 100644
>> --- a/drivers/net/can/Kconfig
>> +++ b/drivers/net/can/Kconfig
>> @@ -125,6 +125,13 @@ config CAN_GRCAN
>>  	  endian syntheses of the cores would need some modifications on
>>  	  the hardware level to work.
>>  
>> +config CAN_XILINXCAN
>> +	tristate "Xilinx CAN"
>> +	depends on ARCH_ZYNQ || MICROBLAZE
> 
> Is Zynq multiarch already?

let me just answer this. Zynq is device with hardcoded dual arm cortex-a9 + some IPs
and programmable logic.
It means if you add Microblaze to PL or another CPU then you get multi architecture setup.

Thanks,
Michal

On 03/10/2014 04:04 PM, Michal Simek wrote:
> Hi Marc,
> 
> On 03/10/2014 03:57 PM, Marc Kleine-Budde wrote:
>> On 03/04/2014 02:20 PM, Kedareswara rao Appana wrote:
>>> This patch adds xilinx CAN controller support.
>>> This driver supports both ZYNQ CANPS and Soft IP
>>> AXI CAN controller.
>>>
>>> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
>>> ---
>>> This patch is rebased on the 3.14 rc5 kernel.
>>> Changes for v5:
>>> - Updated the driver with the review comments.
>>> - Remove the check for the tx fifo full interrupt condition
>>>   form Tx interrupt routine as we are checking it in the _xmit
>>>   routine.
>>> - Clearing the txok interrupt in the tx interrupt routine for
>>>   every Tx can frame.
>>> Changes for v4:
>>> - Added check for the tx fifo full interrupt condition in
>>>   Tx interrupt routine.
>>> - Added be iohelper functions.
>>> - Moved the clock enable/disable to probe/remove because of
>>>   Added big endian support for AXI CAN controller case(reading
>>>   a register during probe for that we need to enable clock).
>>> Changes for v3:
>>> - Updated the driver with the review comments.
>>> - Modified the tranmit logic as per Marc suggestion.
>>> - Enabling the clock when the interface is up to reduce the
>>>   Power consumption.
>>> Changes for v2:
>>> - Updated with the review comments.
>>> - Removed the unnecessary debug prints.
>>> - include tx,rx fifo depths in ZYNQ CANPS case also.
>>> ---
>>>  .../devicetree/bindings/net/can/xilinx_can.txt     |   45 +
>>>  drivers/net/can/Kconfig                            |    7 +
>>>  drivers/net/can/Makefile                           |    1 +
>>>  drivers/net/can/xilinx_can.c                       | 1195 ++++++++++++++++++++
>>>  4 files changed, 1248 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..0e57103
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/can/xilinx_can.txt
>>> @@ -0,0 +1,45 @@
>>> +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).
>>> +- tx-fifo-depth		: Can Tx fifo depth.
>>> +- rx-fifo-depth		: Can Rx fifo depth.
>>> +
>>> +
>>> +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>;
>>> +			tx-fifo-depth = <0x40>;
>>> +			rx-fifo-depth = <0x40>;
>>> +		};
>>> +For Axi CAN Dts file:
>>> +	axi_can_0: axi-can@40000000 {
>>> +			compatible = "xlnx,axi-can-1.00.a";
>>> +			clocks = <&clkc 0>;
>>> +			clock-names = "ref_clk" ;
>>> +			reg = <0x40000000 0x10000>;
>>> +			interrupt-parent = <&intc>;
>>> +			interrupts = <0 59 1>;
>>> +			tx-fifo-depth = <0x40>;
>>> +			rx-fifo-depth = <0x40>;
>>> +		};
>>> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
>>> index 9e7d95d..b180239 100644
>>> --- a/drivers/net/can/Kconfig
>>> +++ b/drivers/net/can/Kconfig
>>> @@ -125,6 +125,13 @@ config CAN_GRCAN
>>>  	  endian syntheses of the cores would need some modifications on
>>>  	  the hardware level to work.
>>>  
>>> +config CAN_XILINXCAN
>>> +	tristate "Xilinx CAN"
>>> +	depends on ARCH_ZYNQ || MICROBLAZE
>>
>> Is Zynq multiarch already?
> 
> let me just answer this. Zynq is device with hardcoded dual arm cortex-a9 + some IPs
> and programmable logic.
> It means if you add Microblaze to PL or another CPU then you get multi architecture setup.

Sorry for the confusion, I'm not talking about
more-than-one-architecture-in-a-FPGA, but the feature of the ARM-Linux
Kernel to run on different ARM architecture. For example, it's possible
to start the same kernel on a freescale imx6 and a the new marvel ebu
platform. If the Zynq Kernel supports this feature, too, we should not
limit to ARCH_ZYNQ, but use ARM instead.

Marc

On Tue, Mar 4, 2014 at 7:20 AM, Kedareswara rao Appana
<appana.durga.rao@xilinx.com> wrote:
> This patch adds xilinx CAN controller support.
> This driver supports both ZYNQ CANPS and Soft IP
> AXI CAN controller.
>
> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> ---
> This patch is rebased on the 3.14 rc5 kernel.
> Changes for v5:
> - Updated the driver with the review comments.
> - Remove the check for the tx fifo full interrupt condition
>   form Tx interrupt routine as we are checking it in the _xmit
>   routine.
> - Clearing the txok interrupt in the tx interrupt routine for
>   every Tx can frame.
> Changes for v4:
> - Added check for the tx fifo full interrupt condition in
>   Tx interrupt routine.
> - Added be iohelper functions.
> - Moved the clock enable/disable to probe/remove because of
>   Added big endian support for AXI CAN controller case(reading
>   a register during probe for that we need to enable clock).
> Changes for v3:
> - Updated the driver with the review comments.
> - Modified the tranmit logic as per Marc suggestion.
> - Enabling the clock when the interface is up to reduce the
>   Power consumption.
> Changes for v2:
> - Updated with the review comments.
> - Removed the unnecessary debug prints.
> - include tx,rx fifo depths in ZYNQ CANPS case also.
> ---
>  .../devicetree/bindings/net/can/xilinx_can.txt     |   45 +
>  drivers/net/can/Kconfig                            |    7 +
>  drivers/net/can/Makefile                           |    1 +
>  drivers/net/can/xilinx_can.c                       | 1195 ++++++++++++++++++++
>  4 files changed, 1248 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..0e57103
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/xilinx_can.txt
> @@ -0,0 +1,45 @@
> +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).
> +- tx-fifo-depth                : Can Tx fifo depth.
> +- rx-fifo-depth                : Can Rx fifo depth.
> +
> +
> +Example:
> +
> +For Zynq CANPS Dts file:
> +       zynq_can_0: zynq-can@e0008000 {

The preferred node name convention is to use the type of device. There
is no standard in this case, but I would use just "can@...".

Otherwise, for the binding:

Acked-by: Rob Herring <robh@kernel.org>

> +                       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>;
> +                       tx-fifo-depth = <0x40>;
> +                       rx-fifo-depth = <0x40>;
> +               };
> +For Axi CAN Dts file:
> +       axi_can_0: axi-can@40000000 {
> +                       compatible = "xlnx,axi-can-1.00.a";
> +                       clocks = <&clkc 0>;
> +                       clock-names = "ref_clk" ;
> +                       reg = <0x40000000 0x10000>;
> +                       interrupt-parent = <&intc>;
> +                       interrupts = <0 59 1>;
> +                       tx-fifo-depth = <0x40>;
> +                       rx-fifo-depth = <0x40>;
> +               };
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index 9e7d95d..b180239 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -125,6 +125,13 @@ config CAN_GRCAN
>           endian syntheses of the cores would need some modifications on
>           the hardware level to work.
>
> +config CAN_XILINXCAN
> +       tristate "Xilinx CAN"
> +       depends on ARCH_ZYNQ || MICROBLAZE
> +       ---help---
> +         Xilinx CAN driver. This driver supports both soft AXI CAN IP and
> +         Zynq CANPS IP.
> +
>  source "drivers/net/can/mscan/Kconfig"
>
>  source "drivers/net/can/sja1000/Kconfig"
> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> index c744039..0b8e11e 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -25,5 +25,6 @@ obj-$(CONFIG_CAN_JANZ_ICAN3)  += janz-ican3.o
>  obj-$(CONFIG_CAN_FLEXCAN)      += flexcan.o
>  obj-$(CONFIG_PCH_CAN)          += pch_can.o
>  obj-$(CONFIG_CAN_GRCAN)                += grcan.o
> +obj-$(CONFIG_CAN_XILINXCAN)    += xilinx_can.o
>
>  ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> new file mode 100644
> index 0000000..a8552a2
> --- /dev/null
> +++ b/drivers/net/can/xilinx_can.c
> @@ -0,0 +1,1195 @@
> +/* Xilinx CAN device driver
> + *
> + * Copyright (C) 2012 - 2014 Xilinx, Inc.
> + * Copyright (C) 2009 PetaLogix. All rights reserved.
> + *
> + * Description:
> + * This driver is developed for Axi CAN IP and for Zynq CANPS Controller.
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/netdevice.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/skbuff.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/can/dev.h>
> +#include <linux/can/error.h>
> +#include <linux/can/led.h>
> +
> +#define DRIVER_NAME    "XILINX_CAN"
> +
> +/* CAN registers set */
> +enum xcan_reg {
> +       XCAN_SRR_OFFSET         = 0x00, /* Software reset */
> +       XCAN_MSR_OFFSET         = 0x04, /* Mode select */
> +       XCAN_BRPR_OFFSET        = 0x08, /* Baud rate prescaler */
> +       XCAN_BTR_OFFSET         = 0x0C, /* Bit timing */
> +       XCAN_ECR_OFFSET         = 0x10, /* Error counter */
> +       XCAN_ESR_OFFSET         = 0x14, /* Error status */
> +       XCAN_SR_OFFSET          = 0x18, /* Status */
> +       XCAN_ISR_OFFSET         = 0x1C, /* Interrupt status */
> +       XCAN_IER_OFFSET         = 0x20, /* Interrupt enable */
> +       XCAN_ICR_OFFSET         = 0x24, /* Interrupt clear */
> +       XCAN_TXFIFO_ID_OFFSET   = 0x30,/* TX FIFO ID */
> +       XCAN_TXFIFO_DLC_OFFSET  = 0x34, /* TX FIFO DLC */
> +       XCAN_TXFIFO_DW1_OFFSET  = 0x38, /* TX FIFO Data Word 1 */
> +       XCAN_TXFIFO_DW2_OFFSET  = 0x3C, /* TX FIFO Data Word 2 */
> +       XCAN_RXFIFO_ID_OFFSET   = 0x50, /* RX FIFO ID */
> +       XCAN_RXFIFO_DLC_OFFSET  = 0x54, /* RX FIFO DLC */
> +       XCAN_RXFIFO_DW1_OFFSET  = 0x58, /* RX FIFO Data Word 1 */
> +       XCAN_RXFIFO_DW2_OFFSET  = 0x5C, /* RX FIFO Data Word 2 */
> +};
> +
> +/* CAN register bit masks - XCAN_<REG>_<BIT>_MASK */
> +#define XCAN_SRR_CEN_MASK              0x00000002 /* CAN enable */
> +#define XCAN_SRR_RESET_MASK            0x00000001 /* Soft Reset the CAN core */
> +#define XCAN_MSR_LBACK_MASK            0x00000002 /* Loop back mode select */
> +#define XCAN_MSR_SLEEP_MASK            0x00000001 /* Sleep mode select */
> +#define XCAN_BRPR_BRP_MASK             0x000000FF /* Baud rate prescaler */
> +#define XCAN_BTR_SJW_MASK              0x00000180 /* Synchronous jump width */
> +#define XCAN_BTR_TS2_MASK              0x00000070 /* Time segment 2 */
> +#define XCAN_BTR_TS1_MASK              0x0000000F /* Time segment 1 */
> +#define XCAN_ECR_REC_MASK              0x0000FF00 /* Receive error counter */
> +#define XCAN_ECR_TEC_MASK              0x000000FF /* Transmit error counter */
> +#define XCAN_ESR_ACKER_MASK            0x00000010 /* ACK error */
> +#define XCAN_ESR_BERR_MASK             0x00000008 /* Bit error */
> +#define XCAN_ESR_STER_MASK             0x00000004 /* Stuff error */
> +#define XCAN_ESR_FMER_MASK             0x00000002 /* Form error */
> +#define XCAN_ESR_CRCER_MASK            0x00000001 /* CRC error */
> +#define XCAN_SR_TXFLL_MASK             0x00000400 /* TX FIFO is full */
> +#define XCAN_SR_ESTAT_MASK             0x00000180 /* Error status */
> +#define XCAN_SR_ERRWRN_MASK            0x00000040 /* Error warning */
> +#define XCAN_SR_NORMAL_MASK            0x00000008 /* Normal mode */
> +#define XCAN_SR_LBACK_MASK             0x00000002 /* Loop back mode */
> +#define XCAN_SR_CONFIG_MASK            0x00000001 /* Configuration mode */
> +#define XCAN_IXR_TXFEMP_MASK           0x00004000 /* TX FIFO Empty */
> +#define XCAN_IXR_WKUP_MASK             0x00000800 /* Wake up interrupt */
> +#define XCAN_IXR_SLP_MASK              0x00000400 /* Sleep interrupt */
> +#define XCAN_IXR_BSOFF_MASK            0x00000200 /* Bus off interrupt */
> +#define XCAN_IXR_ERROR_MASK            0x00000100 /* Error interrupt */
> +#define XCAN_IXR_RXNEMP_MASK           0x00000080 /* RX FIFO NotEmpty intr */
> +#define XCAN_IXR_RXOFLW_MASK           0x00000040 /* RX FIFO Overflow intr */
> +#define XCAN_IXR_RXOK_MASK             0x00000010 /* Message received intr */
> +#define XCAN_IXR_TXFLL_MASK            0x00000004 /* Tx FIFO Full intr */
> +#define XCAN_IXR_TXOK_MASK             0x00000002 /* TX successful intr */
> +#define XCAN_IXR_ARBLST_MASK           0x00000001 /* Arbitration lost intr */
> +#define XCAN_IDR_ID1_MASK              0xFFE00000 /* Standard msg identifier */
> +#define XCAN_IDR_SRR_MASK              0x00100000 /* Substitute remote TXreq */
> +#define XCAN_IDR_IDE_MASK              0x00080000 /* Identifier extension */
> +#define XCAN_IDR_ID2_MASK              0x0007FFFE /* Extended message ident */
> +#define XCAN_IDR_RTR_MASK              0x00000001 /* Remote TX request */
> +#define XCAN_DLCR_DLC_MASK             0xF0000000 /* Data length code */
> +
> +#define XCAN_INTR_ALL          (XCAN_IXR_TXOK_MASK | XCAN_IXR_BSOFF_MASK |\
> +                                XCAN_IXR_WKUP_MASK | XCAN_IXR_SLP_MASK | \
> +                                XCAN_IXR_RXNEMP_MASK | XCAN_IXR_ERROR_MASK | \
> +                                XCAN_IXR_ARBLST_MASK | XCAN_IXR_RXOK_MASK)
> +
> +/* CAN register bit shift - XCAN_<REG>_<BIT>_SHIFT */
> +#define XCAN_BTR_SJW_SHIFT             7  /* Synchronous jump width */
> +#define XCAN_BTR_TS2_SHIFT             4  /* Time segment 2 */
> +#define XCAN_IDR_ID1_SHIFT             21 /* Standard Messg Identifier */
> +#define XCAN_IDR_ID2_SHIFT             1  /* Extended Message Identifier */
> +#define XCAN_DLCR_DLC_SHIFT            28 /* Data length code */
> +#define XCAN_ESR_REC_SHIFT             8  /* Rx Error Count */
> +
> +/* CAN frame length constants */
> +#define XCAN_ECHO_SKB_MAX              64
> +#define XCAN_FRAME_MAX_DATA_LEN                8
> +#define XCAN_TIMEOUT                   (1 * HZ)
> +
> +/**
> + * struct xcan_priv - This definition define CAN driver instance
> + * @can:                       CAN private data structure.
> + * @tx_head:                   Tx CAN packets ready to send on the queue
> + * @tx_tail:                   Tx CAN packets successfully sended on the queue
> + * @tx_max:                    Maximum number packets the driver can send
> + * @napi:                      NAPI structure
> + * @read_reg:                  For reading data from CAN registers
> + * @write_reg:                 For writing data to CAN registers
> + * @dev:                       Network device data structure
> + * @reg_base:                  Ioremapped address to registers
> + * @irq_flags:                 For request_irq()
> + * @aperclk:                   Pointer to struct clk
> + * @devclk:                    Pointer to struct clk
> + */
> +struct xcan_priv {
> +       struct can_priv can;
> +       unsigned int tx_head;
> +       unsigned int tx_tail;
> +       u32 tx_max;
> +       struct napi_struct napi;
> +       u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
> +       void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
> +                       u32 val);
> +       struct net_device *dev;
> +       void __iomem *reg_base;
> +       unsigned long irq_flags;
> +       struct clk *aperclk;
> +       struct clk *devclk;
> +};
> +
> +/* CAN Bittiming constants as per Xilinx CAN specs */
> +static const struct can_bittiming_const xcan_bittiming_const = {
> +       .name = DRIVER_NAME,
> +       .tseg1_min = 1,
> +       .tseg1_max = 16,
> +       .tseg2_min = 1,
> +       .tseg2_max = 8,
> +       .sjw_max = 4,
> +       .brp_min = 1,
> +       .brp_max = 256,
> +       .brp_inc = 1,
> +};
> +
> +/**
> + * xcan_write_reg_le - Write a value to the device register little endian
> + * @priv:      Driver private data structure
> + * @reg:       Register offset
> + * @val:       Value to write at the Register offset
> + *
> + * Write data to the paricular CAN register
> + */
> +static void xcan_write_reg_le(const struct xcan_priv *priv, enum xcan_reg reg,
> +                       u32 val)
> +{
> +       iowrite32(val, priv->reg_base + reg);
> +}
> +
> +/**
> + * xcan_read_reg_le - Read a value from the device register little endian
> + * @priv:      Driver private data structure
> + * @reg:       Register offset
> + *
> + * Read data from the particular CAN register
> + * Return: value read from the CAN register
> + */
> +static u32 xcan_read_reg_le(const struct xcan_priv *priv, enum xcan_reg reg)
> +{
> +       return ioread32(priv->reg_base + reg);
> +}
> +
> +/**
> + * xcan_write_reg_be - Write a value to the device register big endian
> + * @priv:      Driver private data structure
> + * @reg:       Register offset
> + * @val:       Value to write at the Register offset
> + *
> + * Write data to the paricular CAN register
> + */
> +static void xcan_write_reg_be(const struct xcan_priv *priv, enum xcan_reg reg,
> +                       u32 val)
> +{
> +       iowrite32be(val, priv->reg_base + reg);
> +}
> +
> +/**
> + * xcan_read_reg_be - Read a value from the device register big endian
> + * @priv:      Driver private data structure
> + * @reg:       Register offset
> + *
> + * Read data from the particular CAN register
> + * Return: value read from the CAN register
> + */
> +static u32 xcan_read_reg_be(const struct xcan_priv *priv, enum xcan_reg reg)
> +{
> +       return ioread32be(priv->reg_base + reg);
> +}
> +
> +/**
> + * set_reset_mode - Resets the CAN device mode
> + * @ndev:      Pointer to net_device structure
> + *
> + * This is the driver reset mode routine.The driver
> + * enters into configuration mode.
> + *
> + * Return: 0 on success and failure value on error
> + */
> +static int set_reset_mode(struct net_device *ndev)
> +{
> +       struct xcan_priv *priv = netdev_priv(ndev);
> +       unsigned long timeout;
> +
> +       priv->can.state = CAN_STATE_STOPPED;
> +
> +       timeout = jiffies + XCAN_TIMEOUT;
> +       while (!(priv->read_reg(priv, XCAN_SR_OFFSET) & XCAN_SR_CONFIG_MASK)) {
> +               if (time_after(jiffies, timeout)) {
> +                       netdev_warn(ndev, "timedout waiting for config mode\n");
> +                       return -ETIMEDOUT;
> +               }
> +               usleep_range(500, 10000);
> +       }
> +
> +       return 0;
> +}
> +
> +/**
> + * xcan_set_bittiming - CAN set bit timing routine
> + * @ndev:      Pointer to net_device structure
> + *
> + * This is the driver set bittiming  routine.
> + * Return: 0 on success and failure value on error
> + */
> +static int xcan_set_bittiming(struct net_device *ndev)
> +{
> +       struct xcan_priv *priv = netdev_priv(ndev);
> +       struct can_bittiming *bt = &priv->can.bittiming;
> +       u32 btr0, btr1;
> +       u32 is_config_mode;
> +
> +       /* Check whether Xilinx CAN is in configuration mode.
> +        * It cannot set bit timing if Xilinx CAN is not in configuration mode.
> +        */
> +       is_config_mode = priv->read_reg(priv, XCAN_SR_OFFSET) &
> +                               XCAN_SR_CONFIG_MASK;
> +       if (!is_config_mode) {
> +               netdev_alert(ndev,
> +                       "Cannot set bittiming can is not in config mode\n");
> +               return -EPERM;
> +       }
> +
> +       /* Setting Baud Rate prescalar value in BRPR Register */
> +       btr0 = (bt->brp - 1);
> +
> +       /* Setting Time Segment 1 in BTR Register */
> +       btr1 = (bt->prop_seg + bt->phase_seg1 - 1);
> +
> +       /* Setting Time Segment 2 in BTR Register */
> +       btr1 |= (bt->phase_seg2 - 1) << XCAN_BTR_TS2_SHIFT;
> +
> +       /* Setting Synchronous jump width in BTR Register */
> +       btr1 |= (bt->sjw - 1) << XCAN_BTR_SJW_SHIFT;
> +
> +       priv->write_reg(priv, XCAN_BRPR_OFFSET, btr0);
> +       priv->write_reg(priv, XCAN_BTR_OFFSET, btr1);
> +
> +       netdev_dbg(ndev, "BRPR=0x%08x, BTR=0x%08x\n",
> +                       priv->read_reg(priv, XCAN_BRPR_OFFSET),
> +                       priv->read_reg(priv, XCAN_BTR_OFFSET));
> +
> +       return 0;
> +}
> +
> +/**
> + * xcan_chip_start - This the drivers start routine
> + * @ndev:      Pointer to net_device structure
> + *
> + * This is the drivers start routine.
> + * Based on the State of the CAN device it puts
> + * the CAN device into a proper mode.
> + *
> + * Return: 0 on success and failure value on error
> + */
> +static int xcan_chip_start(struct net_device *ndev)
> +{
> +       struct xcan_priv *priv = netdev_priv(ndev);
> +       u32 err;
> +       unsigned long timeout;
> +
> +       /* Check if it is in reset mode */
> +       err = set_reset_mode(ndev);
> +       if (err < 0)
> +               return err;
> +
> +       err = xcan_set_bittiming(ndev);
> +       if (err < 0)
> +               return err;
> +
> +       /* Enable interrupts */
> +       priv->write_reg(priv, XCAN_IER_OFFSET, XCAN_INTR_ALL);
> +
> +       /* Check whether it is loopback mode or normal mode  */
> +       if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
> +               /* Put device into loopback mode */
> +               priv->write_reg(priv, XCAN_MSR_OFFSET, XCAN_MSR_LBACK_MASK);
> +       else
> +               /* The device is in normal mode */
> +               priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> +
> +       if (priv->can.state == CAN_STATE_STOPPED) {
> +               /* Enable Xilinx CAN */
> +               priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> +               priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +               timeout = jiffies + XCAN_TIMEOUT;
> +               if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
> +                       while ((priv->read_reg(priv, XCAN_SR_OFFSET)
> +                                       & XCAN_SR_LBACK_MASK) == 0) {
> +                               if (time_after(jiffies, timeout)) {
> +                                       netdev_warn(ndev,
> +                                               "timedout for loopback mode\n");
> +                                       return -ETIMEDOUT;
> +                               }
> +                               usleep_range(500, 10000);
> +                       }
> +               } else {
> +                       while ((priv->read_reg(priv, XCAN_SR_OFFSET)
> +                                       & XCAN_SR_NORMAL_MASK) == 0) {
> +                               if (time_after(jiffies, timeout)) {
> +                                       netdev_warn(ndev,
> +                                               "timedout for normal mode\n");
> +                                       return -ETIMEDOUT;
> +                               }
> +                               usleep_range(500, 10000);
> +                       }
> +               }
> +               netdev_dbg(ndev, "status:#x%08x\n",
> +                               priv->read_reg(priv, XCAN_SR_OFFSET));
> +       }
> +       priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +       return 0;
> +}
> +
> +/**
> + * xcan_do_set_mode - This sets the mode of the driver
> + * @ndev:      Pointer to net_device structure
> + * @mode:      Tells the mode of the driver
> + *
> + * This check the drivers state and calls the
> + * the corresponding modes to set.
> + *
> + * Return: 0 on success and failure value on error
> + */
> +static int xcan_do_set_mode(struct net_device *ndev, enum can_mode mode)
> +{
> +       int ret;
> +
> +       switch (mode) {
> +       case CAN_MODE_START:
> +               ret = xcan_chip_start(ndev);
> +               if (ret < 0)
> +                       netdev_err(ndev, "xcan_chip_start failed!\n");
> +               netif_wake_queue(ndev);
> +               break;
> +       default:
> +               ret = -EOPNOTSUPP;
> +               break;
> +       }
> +
> +       return ret;
> +}
> +
> +/**
> + * xcan_start_xmit - Starts the transmission
> + * @skb:       sk_buff pointer that contains data to be Txed
> + * @ndev:      Pointer to net_device structure
> + *
> + * This function is invoked from upper layers to initiate transmission. This
> + * function uses the next available free txbuff and populates their fields to
> + * start the transmission.
> + *
> + * Return: 0 on success and failure value on error
> + */
> +static int xcan_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> +{
> +       struct xcan_priv *priv = netdev_priv(ndev);
> +       struct net_device_stats *stats = &ndev->stats;
> +       struct can_frame *cf = (struct can_frame *)skb->data;
> +       u32 id, dlc, data[2] = {0, 0};
> +
> +       if (can_dropped_invalid_skb(ndev, skb))
> +               return NETDEV_TX_OK;
> +
> +       /* Check if the TX buffer is full */
> +       if (unlikely(priv->read_reg(priv, XCAN_SR_OFFSET) &
> +                       XCAN_SR_TXFLL_MASK)) {
> +               netif_stop_queue(ndev);
> +               netdev_err(ndev, "BUG!, TX FIFO full when queue awake!\n");
> +               return NETDEV_TX_BUSY;
> +       }
> +
> +       /* Watch carefully on the bit sequence */
> +       if (cf->can_id & CAN_EFF_FLAG) {
> +               /* Extended CAN ID format */
> +               id = ((cf->can_id & CAN_EFF_MASK) << XCAN_IDR_ID2_SHIFT) &
> +                       XCAN_IDR_ID2_MASK;
> +               id |= (((cf->can_id & CAN_EFF_MASK) >>
> +                       (CAN_EFF_ID_BITS-CAN_SFF_ID_BITS)) <<
> +                       XCAN_IDR_ID1_SHIFT) & XCAN_IDR_ID1_MASK;
> +
> +               /* The substibute remote TX request bit should be "1"
> +                * for extended frames as in the Xilinx CAN datasheet
> +                */
> +               id |= XCAN_IDR_IDE_MASK | XCAN_IDR_SRR_MASK;
> +
> +               if (cf->can_id & CAN_RTR_FLAG)
> +                       /* Extended frames remote TX request */
> +                       id |= XCAN_IDR_RTR_MASK;
> +       } else {
> +               /* Standard CAN ID format */
> +               id = ((cf->can_id & CAN_SFF_MASK) << XCAN_IDR_ID1_SHIFT) &
> +                       XCAN_IDR_ID1_MASK;
> +
> +               if (cf->can_id & CAN_RTR_FLAG)
> +                       /* Standard frames remote TX request */
> +                       id |= XCAN_IDR_SRR_MASK;
> +       }
> +
> +       dlc = cf->can_dlc << XCAN_DLCR_DLC_SHIFT;
> +
> +       if (cf->can_dlc > 0)
> +               data[0] = be32_to_cpup((__be32 *)(cf->data + 0));
> +       if (cf->can_dlc > 4)
> +               data[1] = be32_to_cpup((__be32 *)(cf->data + 4));
> +
> +       can_put_echo_skb(skb, ndev, priv->tx_head % priv->tx_max);
> +       priv->tx_head++;
> +
> +       /* Write the Frame to Xilinx CAN TX FIFO */
> +       priv->write_reg(priv, XCAN_TXFIFO_ID_OFFSET, id);
> +       /* If the CAN frame is RTR frame this write triggers tranmission */
> +       priv->write_reg(priv, XCAN_TXFIFO_DLC_OFFSET, dlc);
> +       if (!(cf->can_id & CAN_RTR_FLAG)) {
> +               priv->write_reg(priv, XCAN_TXFIFO_DW1_OFFSET, data[0]);
> +               /* If the CAN frame is Standard/Extended frame this
> +                * write triggers tranmission
> +                */
> +               priv->write_reg(priv, XCAN_TXFIFO_DW2_OFFSET, data[1]);
> +               stats->tx_bytes += cf->can_dlc;
> +       }
> +
> +       /* Check if the TX buffer is full */
> +       if ((priv->tx_head - priv->tx_tail) == priv->tx_max)
> +               netif_stop_queue(ndev);
> +
> +       return NETDEV_TX_OK;
> +}
> +
> +/**
> + * xcan_rx -  Is called from CAN isr to complete the received
> + *             frame  processing
> + * @ndev:      Pointer to net_device structure
> + *
> + * This function is invoked from the CAN isr(poll) to process the Rx frames. It
> + * does minimal processing and invokes "netif_receive_skb" to complete further
> + * processing.
> + * Return: 0 on success and negative error value on error
> + */
> +static int xcan_rx(struct net_device *ndev)
> +{
> +       struct xcan_priv *priv = netdev_priv(ndev);
> +       struct net_device_stats *stats = &ndev->stats;
> +       struct can_frame *cf;
> +       struct sk_buff *skb;
> +       u32 id_xcan, dlc, data[2] = {0, 0};
> +
> +       skb = alloc_can_skb(ndev, &cf);
> +       if (!skb)
> +               return -ENOMEM;
> +
> +       /* Read a frame from Xilinx zynq CANPS */
> +       id_xcan = priv->read_reg(priv, XCAN_RXFIFO_ID_OFFSET);
> +       dlc = priv->read_reg(priv, XCAN_RXFIFO_DLC_OFFSET) >>
> +                               XCAN_DLCR_DLC_SHIFT;
> +
> +       /* Change Xilinx CAN data length format to socketCAN data format */
> +       cf->can_dlc = get_can_dlc(dlc);
> +
> +       /* Change Xilinx CAN ID format to socketCAN ID format */
> +       if (id_xcan & XCAN_IDR_IDE_MASK) {
> +               /* The received frame is an Extended format frame */
> +               cf->can_id = (id_xcan & XCAN_IDR_ID1_MASK) >> 3;
> +               cf->can_id |= (id_xcan & XCAN_IDR_ID2_MASK) >>
> +                               XCAN_IDR_ID2_SHIFT;
> +               cf->can_id |= CAN_EFF_FLAG;
> +               if (id_xcan & XCAN_IDR_RTR_MASK)
> +                       cf->can_id |= CAN_RTR_FLAG;
> +       } else {
> +               /* The received frame is a standard format frame */
> +               cf->can_id = (id_xcan & XCAN_IDR_ID1_MASK) >>
> +                               XCAN_IDR_ID1_SHIFT;
> +               if (id_xcan & XCAN_IDR_RTR_MASK)
> +                       cf->can_id |= CAN_RTR_FLAG;
> +       }
> +
> +       if (!(id_xcan & XCAN_IDR_RTR_MASK)) {
> +               data[0] = priv->read_reg(priv, XCAN_RXFIFO_DW1_OFFSET);
> +               data[1] = priv->read_reg(priv, XCAN_RXFIFO_DW2_OFFSET);
> +
> +               /* Change Xilinx CAN data format to socketCAN data format */
> +               *(__be32 *)(cf->data) = cpu_to_be32(data[0]);
> +               if (cf->can_dlc > 4)
> +                       *(__be32 *)(cf->data + 4) = cpu_to_be32(data[1]);
> +       }
> +       can_led_event(ndev, CAN_LED_EVENT_RX);
> +
> +       netif_receive_skb(skb);
> +
> +       stats->rx_bytes += cf->can_dlc;
> +       stats->rx_packets++;
> +       return 0;
> +}
> +
> +/**
> + * xcan_err_interrupt - error frame Isr
> + * @ndev:      net_device pointer
> + * @isr:       interrupt status register value
> + *
> + * This is the CAN error interrupt and it will
> + * check the the type of error and forward the error
> + * frame to upper layers.
> + */
> +static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
> +{
> +       struct xcan_priv *priv = netdev_priv(ndev);
> +       struct net_device_stats *stats = &ndev->stats;
> +       struct can_frame *cf;
> +       struct sk_buff *skb;
> +       u32 err_status, status;
> +
> +       skb = alloc_can_err_skb(ndev, &cf);
> +       if (!skb) {
> +               netdev_err(ndev, "alloc_can_err_skb() failed!\n");
> +               return;
> +       }
> +
> +       err_status = priv->read_reg(priv, XCAN_ESR_OFFSET);
> +       priv->write_reg(priv, XCAN_ESR_OFFSET, err_status);
> +       status = priv->read_reg(priv, XCAN_SR_OFFSET);
> +
> +       if (isr & XCAN_IXR_BSOFF_MASK) {
> +               priv->can.state = CAN_STATE_BUS_OFF;
> +               cf->can_id |= CAN_ERR_BUSOFF;
> +               priv->can.can_stats.bus_off++;
> +               /* Leave device in Config Mode in bus-off state */
> +               priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_RESET_MASK);
> +               can_bus_off(ndev);
> +       } else if ((status & XCAN_SR_ESTAT_MASK) == XCAN_SR_ESTAT_MASK) {
> +               cf->can_id |= CAN_ERR_CRTL;
> +               priv->can.state = CAN_STATE_ERROR_PASSIVE;
> +               priv->can.can_stats.error_passive++;
> +               cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE |
> +                                       CAN_ERR_CRTL_TX_PASSIVE;
> +       } else if (status & XCAN_SR_ERRWRN_MASK) {
> +               cf->can_id |= CAN_ERR_CRTL;
> +               priv->can.state = CAN_STATE_ERROR_WARNING;
> +               priv->can.can_stats.error_warning++;
> +               cf->data[1] |= CAN_ERR_CRTL_RX_WARNING |
> +                                       CAN_ERR_CRTL_TX_WARNING;
> +       }
> +
> +       /* Check for Arbitration lost interrupt */
> +       if (isr & XCAN_IXR_ARBLST_MASK) {
> +               cf->can_id |= CAN_ERR_LOSTARB;
> +               cf->data[0] = CAN_ERR_LOSTARB_UNSPEC;
> +               priv->can.can_stats.arbitration_lost++;
> +       }
> +
> +       /* Check for RX FIFO Overflow interrupt */
> +       if (isr & XCAN_IXR_RXOFLW_MASK) {
> +               cf->can_id |= CAN_ERR_CRTL;
> +               cf->data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
> +               stats->rx_over_errors++;
> +               stats->rx_errors++;
> +               priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_RESET_MASK);
> +       }
> +
> +       /* Check for error interrupt */
> +       if (isr & XCAN_IXR_ERROR_MASK) {
> +               cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> +               cf->data[2] |= CAN_ERR_PROT_UNSPEC;
> +
> +               /* Check for Ack error interrupt */
> +               if (err_status & XCAN_ESR_ACKER_MASK) {
> +                       cf->can_id |= CAN_ERR_ACK;
> +                       cf->data[3] |= CAN_ERR_PROT_LOC_ACK;
> +                       stats->tx_errors++;
> +               }
> +
> +               /* Check for Bit error interrupt */
> +               if (err_status & XCAN_ESR_BERR_MASK) {
> +                       cf->can_id |= CAN_ERR_PROT;
> +                       cf->data[2] = CAN_ERR_PROT_BIT;
> +                       stats->tx_errors++;
> +               }
> +
> +               /* Check for Stuff error interrupt */
> +               if (err_status & XCAN_ESR_STER_MASK) {
> +                       cf->can_id |= CAN_ERR_PROT;
> +                       cf->data[2] = CAN_ERR_PROT_STUFF;
> +                       stats->rx_errors++;
> +               }
> +
> +               /* Check for Form error interrupt */
> +               if (err_status & XCAN_ESR_FMER_MASK) {
> +                       cf->can_id |= CAN_ERR_PROT;
> +                       cf->data[2] = CAN_ERR_PROT_FORM;
> +                       stats->rx_errors++;
> +               }
> +
> +               /* Check for CRC error interrupt */
> +               if (err_status & XCAN_ESR_CRCER_MASK) {
> +                       cf->can_id |= CAN_ERR_PROT;
> +                       cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ |
> +                                       CAN_ERR_PROT_LOC_CRC_DEL;
> +                       stats->rx_errors++;
> +               }
> +                       priv->can.can_stats.bus_error++;
> +       }
> +
> +       netif_rx(skb);
> +       stats->rx_packets++;
> +       stats->rx_bytes += cf->can_dlc;
> +
> +       netdev_dbg(ndev, "%s: error status register:0x%x\n",
> +                       __func__, priv->read_reg(priv, XCAN_ESR_OFFSET));
> +}
> +
> +/**
> + * xcan_state_interrupt - It will check the state of the CAN device
> + * @ndev:      net_device pointer
> + * @isr:       interrupt status register value
> + *
> + * This will checks the state of the CAN device
> + * and puts the device into appropriate state.
> + */
> +static void xcan_state_interrupt(struct net_device *ndev, u32 isr)
> +{
> +       struct xcan_priv *priv = netdev_priv(ndev);
> +
> +       /* Check for Sleep interrupt if set put CAN device in sleep state */
> +       if (isr & XCAN_IXR_SLP_MASK)
> +               priv->can.state = CAN_STATE_SLEEPING;
> +
> +       /* Check for Wake up interrupt if set put CAN device in Active state */
> +       if (isr & XCAN_IXR_WKUP_MASK)
> +               priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +}
> +
> +/**
> + * xcan_rx_poll - Poll routine for rx packets (NAPI)
> + * @napi:      napi structure pointer
> + * @quota:     Max number of rx packets to be processed.
> + *
> + * This is the poll routine for rx part.
> + * It will process the packets maximux quota value.
> + *
> + * Return: number of packets received
> + */
> +static int xcan_rx_poll(struct napi_struct *napi, int quota)
> +{
> +       struct net_device *ndev = napi->dev;
> +       struct xcan_priv *priv = netdev_priv(ndev);
> +       u32 isr, ier;
> +       int work_done = 0;
> +
> +       isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
> +       while ((isr & XCAN_IXR_RXNEMP_MASK) && (work_done < quota)) {
> +               if (isr & XCAN_IXR_RXOK_MASK) {
> +                       priv->write_reg(priv, XCAN_ICR_OFFSET,
> +                               XCAN_IXR_RXOK_MASK);
> +                       if (xcan_rx(ndev) < 0)
> +                               return work_done;
> +                       work_done++;
> +               } else {
> +                       priv->write_reg(priv, XCAN_ICR_OFFSET,
> +                               XCAN_IXR_RXNEMP_MASK);
> +                       break;
> +               }
> +               priv->write_reg(priv, XCAN_ICR_OFFSET, XCAN_IXR_RXNEMP_MASK);
> +               isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
> +       }
> +
> +       if (work_done < quota) {
> +               napi_complete(napi);
> +               ier = priv->read_reg(priv, XCAN_IER_OFFSET);
> +               ier |= (XCAN_IXR_RXOK_MASK | XCAN_IXR_RXNEMP_MASK);
> +               priv->write_reg(priv, XCAN_IER_OFFSET, ier);
> +       }
> +       return work_done;
> +}
> +
> +/**
> + * xcan_tx_interrupt - Tx Done Isr
> + * @ndev:      net_device pointer
> + * @isr:       Interrupt status register value
> + */
> +static void xcan_tx_interrupt(struct net_device *ndev, u32 isr)
> +{
> +       struct xcan_priv *priv = netdev_priv(ndev);
> +       struct net_device_stats *stats = &ndev->stats;
> +
> +       while (priv->tx_head - priv->tx_tail > 0) {
> +               priv->write_reg(priv, XCAN_ICR_OFFSET, XCAN_IXR_TXOK_MASK);
> +               if (!(isr & XCAN_IXR_TXOK_MASK))
> +                       break;
> +               can_get_echo_skb(ndev, priv->tx_tail %
> +                                       priv->tx_max);
> +               priv->tx_tail++;
> +               stats->tx_packets++;
> +               can_led_event(ndev, CAN_LED_EVENT_TX);
> +               isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
> +       }
> +       netif_wake_queue(ndev);
> +}
> +
> +/**
> + * xcan_interrupt - CAN Isr
> + * @irq:       irq number
> + * @dev_id:    device id poniter
> + *
> + * This is the xilinx CAN Isr. It checks for the type of interrupt
> + * and invokes the corresponding ISR.
> + *
> + * Return:
> + * IRQ_NONE - If CAN device is in sleep mode, IRQ_HANDLED otherwise
> + */
> +static irqreturn_t xcan_interrupt(int irq, void *dev_id)
> +{
> +       struct net_device *ndev = (struct net_device *)dev_id;
> +       struct xcan_priv *priv = netdev_priv(ndev);
> +       u32 isr, ier;
> +
> +       /* Get the interrupt status from Xilinx CAN */
> +       isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
> +       if (!isr)
> +               return IRQ_NONE;
> +
> +       netdev_dbg(ndev, "%s: isr:#x%08x, err:#x%08x\n", __func__,
> +                       isr, priv->read_reg(priv, XCAN_ESR_OFFSET));
> +
> +       /* Check for the type of interrupt and Processing it */
> +       if (isr & (XCAN_IXR_SLP_MASK | XCAN_IXR_WKUP_MASK)) {
> +               priv->write_reg(priv, XCAN_ICR_OFFSET, (XCAN_IXR_SLP_MASK |
> +                               XCAN_IXR_WKUP_MASK));
> +               xcan_state_interrupt(ndev, isr);
> +       }
> +
> +       /* Check for Tx interrupt and Processing it */
> +       if (isr & XCAN_IXR_TXOK_MASK) {
> +               priv->write_reg(priv, XCAN_ICR_OFFSET, XCAN_IXR_TXOK_MASK);
> +               xcan_tx_interrupt(ndev, isr);
> +       }
> +
> +       /* Check for the type of error interrupt and Processing it */
> +       if (isr & (XCAN_IXR_ERROR_MASK | XCAN_IXR_RXOFLW_MASK |
> +                       XCAN_IXR_BSOFF_MASK | XCAN_IXR_ARBLST_MASK)) {
> +               priv->write_reg(priv, XCAN_ICR_OFFSET, (XCAN_IXR_ERROR_MASK |
> +                               XCAN_IXR_RXOFLW_MASK | XCAN_IXR_BSOFF_MASK |
> +                               XCAN_IXR_ARBLST_MASK));
> +               xcan_err_interrupt(ndev, isr);
> +       }
> +
> +       /* Check for the type of receive interrupt and Processing it */
> +       if (isr & (XCAN_IXR_RXNEMP_MASK | XCAN_IXR_RXOK_MASK)) {
> +               ier = priv->read_reg(priv, XCAN_IER_OFFSET);
> +               ier &= ~(XCAN_IXR_RXNEMP_MASK | XCAN_IXR_RXOK_MASK);
> +               priv->write_reg(priv, XCAN_IER_OFFSET, ier);
> +               napi_schedule(&priv->napi);
> +       }
> +       return IRQ_HANDLED;
> +}
> +
> +/**
> + * xcan_chip_stop - Driver stop routine
> + * @ndev:      Pointer to net_device structure
> + *
> + * This is the drivers stop routine. It will disable the
> + * interrupts and put the device into configuration mode.
> + */
> +static void xcan_chip_stop(struct net_device *ndev)
> +{
> +       struct xcan_priv *priv = netdev_priv(ndev);
> +       u32 ier;
> +
> +       /* Disable interrupts and leave the can in configuration mode */
> +       ier = priv->read_reg(priv, XCAN_IER_OFFSET);
> +       ier &= ~XCAN_INTR_ALL;
> +       priv->write_reg(priv, XCAN_IER_OFFSET, ier);
> +       priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_RESET_MASK);
> +       priv->can.state = CAN_STATE_STOPPED;
> +}
> +
> +/**
> + * xcan_open - Driver open routine
> + * @ndev:      Pointer to net_device structure
> + *
> + * This is the driver open routine.
> + * Return: 0 on success and failure value on error
> + */
> +static int xcan_open(struct net_device *ndev)
> +{
> +       struct xcan_priv *priv = netdev_priv(ndev);
> +       int ret;
> +
> +       ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
> +                       ndev->name, ndev);
> +       if (ret < 0) {
> +               netdev_err(ndev, "Irq allocation for CAN failed\n");
> +               goto err;
> +       }
> +
> +       ret = clk_prepare_enable(priv->devclk);
> +       if (ret) {
> +               netdev_err(ndev, "unable to enable device clock\n");
> +               goto err;
> +       }
> +
> +       ret = clk_prepare_enable(priv->aperclk);
> +       if (ret) {
> +               netdev_err(ndev, "unable to enable aper clock\n");
> +               goto err_aperclk;
> +       }
> +
> +       /* Set chip into reset mode */
> +       ret = set_reset_mode(ndev);
> +       if (ret < 0) {
> +               netdev_err(ndev, "mode resetting failed failed!\n");
> +               goto err_devclk;
> +       }
> +
> +       /* Common open */
> +       ret = open_candev(ndev);
> +       if (ret)
> +               goto err_devclk;
> +
> +       ret = xcan_chip_start(ndev);
> +       if (ret < 0) {
> +               netdev_err(ndev, "xcan_chip_start failed!\n");
> +               goto err_devclk;
> +       }
> +
> +       can_led_event(ndev, CAN_LED_EVENT_OPEN);
> +       napi_enable(&priv->napi);
> +       netif_start_queue(ndev);
> +
> +       return 0;
> +
> +err_aperclk:
> +       free_irq(ndev->irq, ndev);
> +       clk_disable_unprepare(priv->devclk);
> +err_devclk:
> +       free_irq(ndev->irq, ndev);
> +       clk_disable_unprepare(priv->devclk);
> +       clk_disable_unprepare(priv->aperclk);
> +err:
> +       return ret;
> +}
> +
> +/**
> + * xcan_close - Driver close routine
> + * @ndev:      Pointer to net_device structure
> + *
> + * Return: 0 always
> + */
> +static int xcan_close(struct net_device *ndev)
> +{
> +       struct xcan_priv *priv = netdev_priv(ndev);
> +
> +       netif_stop_queue(ndev);
> +       napi_disable(&priv->napi);
> +       xcan_chip_stop(ndev);
> +       clk_disable_unprepare(priv->aperclk);
> +       clk_disable_unprepare(priv->devclk);
> +       free_irq(ndev->irq, ndev);
> +       close_candev(ndev);
> +
> +       can_led_event(ndev, CAN_LED_EVENT_STOP);
> +
> +       return 0;
> +}
> +
> +/**
> + * xcan_get_berr_counter - error counter routine
> + * @ndev:      Pointer to net_device structure
> + * @bec:       Pointer to can_berr_counter structure
> + *
> + * This is the driver error counter routine.
> + * Return: 0 always
> + */
> +static int xcan_get_berr_counter(const struct net_device *ndev,
> +                                       struct can_berr_counter *bec)
> +{
> +       struct xcan_priv *priv = netdev_priv(ndev);
> +       int ret;
> +
> +       ret = clk_prepare_enable(priv->devclk);
> +       if (ret)
> +               goto err;
> +
> +       ret = clk_prepare_enable(priv->aperclk);
> +       if (ret)
> +               goto err_clk;
> +
> +       bec->txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) & XCAN_ECR_TEC_MASK;
> +       bec->rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) &
> +                       XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT);
> +
> +       clk_disable_unprepare(priv->aperclk);
> +       clk_disable_unprepare(priv->devclk);
> +
> +       return 0;
> +
> +err_clk:
> +       clk_disable_unprepare(priv->devclk);
> +err:
> +       return ret;
> +}
> +
> +static const struct net_device_ops xcan_netdev_ops = {
> +       .ndo_open       = xcan_open,
> +       .ndo_stop       = xcan_close,
> +       .ndo_start_xmit = xcan_start_xmit,
> +};
> +
> +#ifdef CONFIG_PM_SLEEP
> +/**
> + * xcan_suspend - Suspend method for the driver
> + * @_dev:      Address of the platform_device structure
> + *
> + * Put the driver into low power mode.
> + * Return: 0 always
> + */
> +static int xcan_suspend(struct device *_dev)
> +{
> +       struct platform_device *pdev = container_of(_dev,
> +                       struct platform_device, dev);
> +       struct net_device *ndev = platform_get_drvdata(pdev);
> +       struct xcan_priv *priv = netdev_priv(ndev);
> +
> +       if (netif_running(ndev)) {
> +               netif_stop_queue(ndev);
> +               netif_device_detach(ndev);
> +       }
> +
> +       priv->write_reg(priv, XCAN_MSR_OFFSET, XCAN_MSR_SLEEP_MASK);
> +       priv->can.state = CAN_STATE_SLEEPING;
> +
> +       clk_disable(priv->aperclk);
> +       clk_disable(priv->devclk);
> +
> +       return 0;
> +}
> +
> +/**
> + * xcan_resume - Resume from suspend
> + * @dev:       Address of the platformdevice structure
> + *
> + * Resume operation after suspend.
> + * Return: 0 on success and failure value on error
> + */
> +static int xcan_resume(struct device *dev)
> +{
> +       struct platform_device *pdev = container_of(dev,
> +                       struct platform_device, dev);
> +       struct net_device *ndev = platform_get_drvdata(pdev);
> +       struct xcan_priv *priv = netdev_priv(ndev);
> +       int ret;
> +
> +       ret = clk_enable(priv->aperclk);
> +       if (ret) {
> +               dev_err(dev, "Cannot enable clock.\n");
> +               return ret;
> +       }
> +       ret = clk_enable(priv->devclk);
> +       if (ret) {
> +               dev_err(dev, "Cannot enable clock.\n");
> +               clk_disable_unprepare(priv->aperclk);
> +               return ret;
> +       }
> +
> +       priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> +       priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> +       priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +
> +       if (netif_running(ndev)) {
> +               netif_device_attach(ndev);
> +               netif_start_queue(ndev);
> +       }
> +
> +       return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend, xcan_resume);
> +
> +/**
> + * xcan_probe - Platform registration call
> + * @pdev:      Handle to the platform device structure
> + *
> + * This function does all the memory allocation and registration for the CAN
> + * device.
> + *
> + * Return: 0 on success and failure value on error
> + */
> +static int xcan_probe(struct platform_device *pdev)
> +{
> +       struct resource *res; /* IO mem resources */
> +       struct net_device *ndev;
> +       struct xcan_priv *priv;
> +       int ret, rx_max;
> +
> +       /* Create a CAN device instance */
> +       ndev = alloc_candev(sizeof(struct xcan_priv), XCAN_ECHO_SKB_MAX);
> +       if (!ndev)
> +               return -ENOMEM;
> +
> +       priv = netdev_priv(ndev);
> +       priv->dev = ndev;
> +       priv->can.bittiming_const = &xcan_bittiming_const;
> +       priv->can.do_set_mode = xcan_do_set_mode;
> +       priv->can.do_get_berr_counter = xcan_get_berr_counter;
> +       priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
> +                                       CAN_CTRLMODE_BERR_REPORTING;
> +
> +       /* Get IRQ for the device */
> +       ndev->irq = platform_get_irq(pdev, 0);
> +       ndev->flags |= IFF_ECHO;        /* We support local echo */
> +
> +       platform_set_drvdata(pdev, ndev);
> +       SET_NETDEV_DEV(ndev, &pdev->dev);
> +       ndev->netdev_ops = &xcan_netdev_ops;
> +
> +       /* Get the virtual base address for the device */
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       priv->reg_base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(priv->reg_base)) {
> +               ret = PTR_ERR(priv->reg_base);
> +               goto err_free;
> +       }
> +
> +       ret = of_property_read_u32(pdev->dev.of_node, "tx-fifo-depth",
> +                               &priv->tx_max);
> +       if (ret < 0)
> +               goto err_free;
> +
> +       ret = of_property_read_u32(pdev->dev.of_node, "rx-fifo-depth", &rx_max);
> +       if (ret < 0)
> +               goto err_free;
> +
> +       /* Getting the CAN devclk info */
> +       priv->devclk = devm_clk_get(&pdev->dev, "ref_clk");
> +       if (IS_ERR(priv->devclk)) {
> +               dev_err(&pdev->dev, "Device clock not found.\n");
> +               ret = PTR_ERR(priv->devclk);
> +               goto err_free;
> +       }
> +
> +       /* Check for type of CAN device */
> +       if (of_device_is_compatible(pdev->dev.of_node,
> +                                   "xlnx,zynq-can-1.00.a")) {
> +               priv->aperclk = devm_clk_get(&pdev->dev, "aper_clk");
> +               if (IS_ERR(priv->aperclk)) {
> +                       dev_err(&pdev->dev, "aper clock not found\n");
> +                       ret = PTR_ERR(priv->aperclk);
> +                       goto err_free;
> +               }
> +       } else {
> +               priv->aperclk = priv->devclk;
> +       }
> +
> +       ret = clk_prepare_enable(priv->devclk);
> +       if (ret) {
> +               dev_err(&pdev->dev, "unable to enable device clock\n");
> +               goto err_free;
> +       }
> +
> +       ret = clk_prepare_enable(priv->aperclk);
> +       if (ret) {
> +               dev_err(&pdev->dev, "unable to enable aper clock\n");
> +               goto err_unprepar_disabledev;
> +       }
> +
> +       priv->write_reg = xcan_write_reg_le;
> +       priv->read_reg = xcan_read_reg_le;
> +
> +       if (priv->read_reg(priv, XCAN_SR_OFFSET) != XCAN_SR_CONFIG_MASK) {
> +               priv->write_reg = xcan_write_reg_be;
> +               priv->read_reg = xcan_read_reg_be;
> +       }
> +
> +       priv->can.clock.freq = clk_get_rate(priv->devclk);
> +
> +       netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
> +
> +       ret = register_candev(ndev);
> +       if (ret) {
> +               dev_err(&pdev->dev, "fail to register failed (err=%d)\n", ret);
> +               goto err_unprepar_disableaper;
> +       }
> +
> +       devm_can_led_init(ndev);
> +       clk_disable_unprepare(priv->aperclk);
> +       clk_disable_unprepare(priv->devclk);
> +       netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo depth:%d\n",
> +                       priv->reg_base, ndev->irq, priv->can.clock.freq,
> +                       priv->tx_max);
> +
> +       return 0;
> +
> +err_unprepar_disableaper:
> +       clk_disable_unprepare(priv->aperclk);
> +err_unprepar_disabledev:
> +       clk_disable_unprepare(priv->devclk);
> +err_free:
> +       free_candev(ndev);
> +
> +       return ret;
> +}
> +
> +/**
> + * xcan_remove - Unregister the device after releasing the resources
> + * @pdev:      Handle to the platform device structure
> + *
> + * This function frees all the resources allocated to the device.
> + * Return: 0 always
> + */
> +static int xcan_remove(struct platform_device *pdev)
> +{
> +       struct net_device *ndev = platform_get_drvdata(pdev);
> +       struct xcan_priv *priv = netdev_priv(ndev);
> +
> +       if (set_reset_mode(ndev) < 0)
> +               netdev_err(ndev, "mode resetting failed!\n");
> +
> +       unregister_candev(ndev);
> +       netif_napi_del(&priv->napi);
> +       free_candev(ndev);
> +
> +       return 0;
> +}
> +
> +/* Match table for OF platform binding */
> +static struct of_device_id xcan_of_match[] = {
> +       { .compatible = "xlnx,zynq-can-1.00.a", },
> +       { .compatible = "xlnx,axi-can-1.00.a", },
> +       { /* end of list */ },
> +};
> +MODULE_DEVICE_TABLE(of, xcan_of_match);
> +
> +static struct platform_driver xcan_driver = {
> +       .probe = xcan_probe,
> +       .remove = xcan_remove,
> +       .driver = {
> +               .owner = THIS_MODULE,
> +               .name = DRIVER_NAME,
> +               .pm = &xcan_dev_pm_ops,
> +               .of_match_table = xcan_of_match,
> +       },
> +};
> +
> +module_platform_driver(xcan_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Xilinx Inc");
> +MODULE_DESCRIPTION("Xilinx CAN interface");
> --
> 1.7.4
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Monday 10 March 2014 16:10:31 Marc Kleine-Budde wrote:
> >>> +config CAN_XILINXCAN
> >>> +   tristate "Xilinx CAN"
> >>> +   depends on ARCH_ZYNQ || MICROBLAZE
> >>
> >> Is Zynq multiarch already?
> > 
> > let me just answer this. Zynq is device with hardcoded dual arm cortex-a9 + some IPs
> > and programmable logic.
> > It means if you add Microblaze to PL or another CPU then you get multi architecture setup.
> 
> Sorry for the confusion, I'm not talking about
> more-than-one-architecture-in-a-FPGA, but the feature of the ARM-Linux
> Kernel to run on different ARM architecture. For example, it's possible
> to start the same kernel on a freescale imx6 and a the new marvel ebu
> platform. If the Zynq Kernel supports this feature, too, we should not
> limit to ARCH_ZYNQ, but use ARM instead.

Zynq has been multiplatform capable from the start. Ideally we'd let
users build the driver for any architecture and specify the exact
dependencies, it doesn't even have to be limited to ARM.

You probably want something like

	config CAN_XILINX
	tristate "Xilinx CAN"
	depends on ARCH_ZYNQ || MICROBLAZE || COMPILE_TEST
	depends on COMMON_CLK && HAS_MMIO  # whatever you need for other architectures

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 03/10/2014 04:15 PM, Rob Herring wrote:
> On Tue, Mar 4, 2014 at 7:20 AM, Kedareswara rao Appana
> <appana.durga.rao@xilinx.com> wrote:
>> This patch adds xilinx CAN controller support.
>> This driver supports both ZYNQ CANPS and Soft IP
>> AXI CAN controller.
>>
>> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
>> ---
>> This patch is rebased on the 3.14 rc5 kernel.
>> Changes for v5:
>> - Updated the driver with the review comments.
>> - Remove the check for the tx fifo full interrupt condition
>>   form Tx interrupt routine as we are checking it in the _xmit
>>   routine.
>> - Clearing the txok interrupt in the tx interrupt routine for
>>   every Tx can frame.
>> Changes for v4:
>> - Added check for the tx fifo full interrupt condition in
>>   Tx interrupt routine.
>> - Added be iohelper functions.
>> - Moved the clock enable/disable to probe/remove because of
>>   Added big endian support for AXI CAN controller case(reading
>>   a register during probe for that we need to enable clock).
>> Changes for v3:
>> - Updated the driver with the review comments.
>> - Modified the tranmit logic as per Marc suggestion.
>> - Enabling the clock when the interface is up to reduce the
>>   Power consumption.
>> Changes for v2:
>> - Updated with the review comments.
>> - Removed the unnecessary debug prints.
>> - include tx,rx fifo depths in ZYNQ CANPS case also.
>> ---
>>  .../devicetree/bindings/net/can/xilinx_can.txt     |   45 +
>>  drivers/net/can/Kconfig                            |    7 +
>>  drivers/net/can/Makefile                           |    1 +
>>  drivers/net/can/xilinx_can.c                       | 1195 ++++++++++++++++++++
>>  4 files changed, 1248 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..0e57103
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/can/xilinx_can.txt
>> @@ -0,0 +1,45 @@
>> +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).
>> +- tx-fifo-depth                : Can Tx fifo depth.
>> +- rx-fifo-depth                : Can Rx fifo depth.
>> +
>> +
>> +Example:
>> +
>> +For Zynq CANPS Dts file:
>> +       zynq_can_0: zynq-can@e0008000 {
> 
> The preferred node name convention is to use the type of device. There
> is no standard in this case, but I would use just "can@...".
> 
> Otherwise, for the binding:
> 
> Acked-by: Rob Herring <robh@kernel.org>

Thanks Rob for your ACK.

Kedar: You have got more things to change from Marc that's why please change this too.

Thanks,
Michal


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 03/10/2014 04:15 PM, Arnd Bergmann wrote:
> On Monday 10 March 2014 16:10:31 Marc Kleine-Budde wrote:
>>>>> +config CAN_XILINXCAN
>>>>> +   tristate "Xilinx CAN"
>>>>> +   depends on ARCH_ZYNQ || MICROBLAZE
>>>>
>>>> Is Zynq multiarch already?
>>>
>>> let me just answer this. Zynq is device with hardcoded dual arm cortex-a9 + some IPs
>>> and programmable logic.
>>> It means if you add Microblaze to PL or another CPU then you get multi architecture setup.
>>
>> Sorry for the confusion, I'm not talking about
>> more-than-one-architecture-in-a-FPGA, but the feature of the ARM-Linux
>> Kernel to run on different ARM architecture. For example, it's possible
>> to start the same kernel on a freescale imx6 and a the new marvel ebu
>> platform. If the Zynq Kernel supports this feature, too, we should not
>> limit to ARCH_ZYNQ, but use ARM instead.
> 
> Zynq has been multiplatform capable from the start. Ideally we'd let
> users build the driver for any architecture and specify the exact
> dependencies, it doesn't even have to be limited to ARM.
> 
> You probably want something like
> 
> 	config CAN_XILINX
> 	tristate "Xilinx CAN"
> 	depends on ARCH_ZYNQ || MICROBLAZE || COMPILE_TEST
> 	depends on COMMON_CLK && HAS_MMIO  # whatever you need for other architectures

Fengguang: Are you use COMPILE_TEST in zero day testing system?

Thanks,
Michal


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> > You probably want something like
> > 
> > 	config CAN_XILINX
> > 	tristate "Xilinx CAN"
> > 	depends on ARCH_ZYNQ || MICROBLAZE || COMPILE_TEST
> > 	depends on COMMON_CLK && HAS_MMIO  # whatever you need for other architectures
> 
> Fengguang: Are you use COMPILE_TEST in zero day testing system?

Yeah, the randconfig build/boot tests should cover both COMPILE_TEST
and !COMPILE_TEST cases.

Thanks,
Fengguang
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 03/11/2014 05:45 AM, Fengguang Wu wrote:
>>> You probably want something like
>>>
>>> 	config CAN_XILINX
>>> 	tristate "Xilinx CAN"
>>> 	depends on ARCH_ZYNQ || MICROBLAZE || COMPILE_TEST
>>> 	depends on COMMON_CLK && HAS_MMIO  # whatever you need for other architectures
>>
>> Fengguang: Are you use COMPILE_TEST in zero day testing system?
> 
> Yeah, the randconfig build/boot tests should cover both COMPILE_TEST
> and !COMPILE_TEST cases.

But the point is that randconfig is really random it means it definitely can
cover but not there is no certainty that for that build, you are running,
has both option for all architectures enabled.
Or do I read it wrongly?

Thanks,
Michal


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Michal,

> -----Original Message-----
> From: Michal Simek [mailto:michal.simek@xilinx.com]
> Sent: Monday, March 10, 2014 8:53 PM
> To: Rob Herring
> Cc: Appana Durga Kedareswara Rao; Wolfgang Grandegger; Marc Kleine-
> Budde; Michal Simek; Grant Likely; Rob Herring; linux-can@vger.kernel.org;
> netdev; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org; Appana Durga
> Kedareswara Rao
> Subject: Re: [PATCH v5] can: xilinx CAN controller support.
>
> On 03/10/2014 04:15 PM, Rob Herring wrote:
> > On Tue, Mar 4, 2014 at 7:20 AM, Kedareswara rao Appana
> > <appana.durga.rao@xilinx.com> wrote:
> >> This patch adds xilinx CAN controller support.
> >> This driver supports both ZYNQ CANPS and Soft IP AXI CAN controller.
> >>
> >> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> >> ---
> >> This patch is rebased on the 3.14 rc5 kernel.
> >> Changes for v5:
> >> - Updated the driver with the review comments.
> >> - Remove the check for the tx fifo full interrupt condition
> >>   form Tx interrupt routine as we are checking it in the _xmit
> >>   routine.
> >> - Clearing the txok interrupt in the tx interrupt routine for
> >>   every Tx can frame.
> >> Changes for v4:
> >> - Added check for the tx fifo full interrupt condition in
> >>   Tx interrupt routine.
> >> - Added be iohelper functions.
> >> - Moved the clock enable/disable to probe/remove because of
> >>   Added big endian support for AXI CAN controller case(reading
> >>   a register during probe for that we need to enable clock).
> >> Changes for v3:
> >> - Updated the driver with the review comments.
> >> - Modified the tranmit logic as per Marc suggestion.
> >> - Enabling the clock when the interface is up to reduce the
> >>   Power consumption.
> >> Changes for v2:
> >> - Updated with the review comments.
> >> - Removed the unnecessary debug prints.
> >> - include tx,rx fifo depths in ZYNQ CANPS case also.
> >> ---
> >>  .../devicetree/bindings/net/can/xilinx_can.txt     |   45 +
> >>  drivers/net/can/Kconfig                            |    7 +
> >>  drivers/net/can/Makefile                           |    1 +
> >>  drivers/net/can/xilinx_can.c                       | 1195 ++++++++++++++++++++
> >>  4 files changed, 1248 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..0e57103
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/net/can/xilinx_can.txt
> >> @@ -0,0 +1,45 @@
> >> +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).
> >> +- tx-fifo-depth                : Can Tx fifo depth.
> >> +- rx-fifo-depth                : Can Rx fifo depth.
> >> +
> >> +
> >> +Example:
> >> +
> >> +For Zynq CANPS Dts file:
> >> +       zynq_can_0: zynq-can@e0008000 {
> >
> > The preferred node name convention is to use the type of device. There
> > is no standard in this case, but I would use just "can@...".
> >
> > Otherwise, for the binding:
> >
> > Acked-by: Rob Herring <robh@kernel.org>
>
> Thanks Rob for your ACK.
>
> Kedar: You have got more things to change from Marc that's why please
> change this too.
>
Thanks Rob for the ACk
Michal@  I  will do

Regards,
Kedar.


> Thanks,
> Michal
>
>



This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 03/11/2014 01:34 PM, Appana Durga Kedareswara Rao wrote:

>>> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig index
>>> 9e7d95d..b180239 100644
>>> --- a/drivers/net/can/Kconfig
>>> +++ b/drivers/net/can/Kconfig
>>> @@ -125,6 +125,13 @@ config CAN_GRCAN
>>>       endian syntheses of the cores would need some modifications on
>>>       the hardware level to work.
>>>
>>> +config CAN_XILINXCAN
>>> +   tristate "Xilinx CAN"
>>> +   depends on ARCH_ZYNQ || MICROBLAZE
>>
>> Is Zynq multiarch already?
> Discussions are going on this
> So the final thing that Fengguang ( fengguang.wu@intel.com)
> Proposed is
>         config CAN_XILINX
>         tristate "Xilinx CAN"
>         depends on ARCH_ZYNQ || MICROBLAZE || COMPILE_TEST
>         depends on COMMON_CLK && HAS_MMIO  # whatever you need for other architectures

> Are you Ok for this?

You have to fill the 2nd depends on with some sane values, though.

[...]

>>> +/**
>>> + * struct xcan_priv - This definition define CAN driver instance
>>> + * @can:                   CAN private data structure.
>>> + * @tx_head:                       Tx CAN packets ready to send on the
>> queue
>>> + * @tx_tail:                       Tx CAN packets successfully sended on the
>> queue
>>> + * @tx_max:                        Maximum number packets the driver can
>> send
>>> + * @napi:                  NAPI structure
>>> + * @read_reg:                      For reading data from CAN registers
>>> + * @write_reg:                     For writing data to CAN registers
>>> + * @dev:                   Network device data structure
>>> + * @reg_base:                      Ioremapped address to registers
>>> + * @irq_flags:                     For request_irq()
>>> + * @aperclk:                       Pointer to struct clk
>>> + * @devclk:                        Pointer to struct clk
>>> + */
>>> +struct xcan_priv {
>>> +   struct can_priv can;
>>> +   unsigned int tx_head;
>>> +   unsigned int tx_tail;
>>> +   u32 tx_max;
>> Please make it an unsigned int, too.
>>
> Ok
> 
> 
>>> +   struct napi_struct napi;
>>> +   u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
>>> +   void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
>>> +                   u32 val);
>>> +   struct net_device *dev;
>>> +   void __iomem *reg_base;
>>> +   unsigned long irq_flags;
>>> +   struct clk *aperclk;
>>> +   struct clk *devclk;
>>
>> Please rename the clock variables to match the names in the DT.
>>
> The clock names are different for axi CAN and CANPS case.
> So will make them as busclk and devclk
> Are you ok with this?

Why not "ref_clk" and "aper_clk" as used in the DT?

>>> +};
>>> +

[...]

>>> +/**
>>> + * xcan_chip_start - This the drivers start routine
>>> + * @ndev:  Pointer to net_device structure
>>> + *
>>> + * This is the drivers start routine.
>>> + * Based on the State of the CAN device it puts
>>> + * the CAN device into a proper mode.
>>> + *
>>> + * Return: 0 on success and failure value on error  */ static int
>>> +xcan_chip_start(struct net_device *ndev) {
>>> +   struct xcan_priv *priv = netdev_priv(ndev);
>>> +   u32 err;
>>> +   unsigned long timeout;
>>> +
>>> +   /* Check if it is in reset mode */
>>> +   err = set_reset_mode(ndev);
>>> +   if (err < 0)
>>> +           return err;
>>> +
>>> +   err = xcan_set_bittiming(ndev);
>>> +   if (err < 0)
>>> +           return err;
>>> +
>>> +   /* Enable interrupts */
>>> +   priv->write_reg(priv, XCAN_IER_OFFSET, XCAN_INTR_ALL);
>>> +
>>> +   /* Check whether it is loopback mode or normal mode  */
>>> +   if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
>>> +           /* Put device into loopback mode */
>>> +           priv->write_reg(priv, XCAN_MSR_OFFSET,
>> XCAN_MSR_LBACK_MASK);
>>> +   else
>>> +           /* The device is in normal mode */
>>> +           priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
>>> +
>>> +   if (priv->can.state == CAN_STATE_STOPPED) {
>>
>> Your device is in stopped mode, add you go though set_reset_mode()
>> already.
> 
> Will remove this if condition I putted this condition here because in set_reset_mode we are putting the
> Device state in Stopped state.

So the device is in CAN_STATE_STOPPED, as this code just went through
set_reset_mode() some lines ago. so it makes no sense to check if it
(still) is (the code runs serialized here).

>>
>>> +           /* Enable Xilinx CAN */
>>> +           priv->write_reg(priv, XCAN_SRR_OFFSET,
>> XCAN_SRR_CEN_MASK);
>>> +           priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>> +           timeout = jiffies + XCAN_TIMEOUT;
>>> +           if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
>>> +                   while ((priv->read_reg(priv, XCAN_SR_OFFSET)
>>> +                                   & XCAN_SR_LBACK_MASK) == 0) {
>>> +                           if (time_after(jiffies, timeout)) {
>>> +                                   netdev_warn(ndev,
>>> +                                           "timedout for loopback
>> mode\n");
>>> +                                   return -ETIMEDOUT;
>>> +                           }
>>> +                           usleep_range(500, 10000);
>>> +                   }
>>> +           } else {
>>> +                   while ((priv->read_reg(priv, XCAN_SR_OFFSET)
>>> +                                   & XCAN_SR_NORMAL_MASK) == 0) {
>>> +                           if (time_after(jiffies, timeout)) {
>>> +                                   netdev_warn(ndev,
>>> +                                           "timedout for normal
>> mode\n");
>>> +                                   return -ETIMEDOUT;
>>> +                           }
>>> +                           usleep_range(500, 10000);
>>> +                   }
>>> +           }
>>> +           netdev_dbg(ndev, "status:#x%08x\n",
>>> +                           priv->read_reg(priv, XCAN_SR_OFFSET));
>>> +   }
>>> +   priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>> +   return 0;
>>> +}

Marc

Hi Marc,


> -----Original Message-----

> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]

> Sent: Tuesday, March 11, 2014 6:19 PM

> To: Appana Durga Kedareswara Rao

> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-arm-

> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;

> devicetree@vger.kernel.org; robh+dt@kernel.org; grant.likely@linaro.org;

> Michal Simek; wg@grandegger.com; fengguang.wu@intel.com

> Subject: Re: [PATCH v5] can: xilinx CAN controller support.

>

> On 03/11/2014 01:34 PM, Appana Durga Kedareswara Rao wrote:

>

> >>> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig index

> >>> 9e7d95d..b180239 100644

> >>> --- a/drivers/net/can/Kconfig

> >>> +++ b/drivers/net/can/Kconfig

> >>> @@ -125,6 +125,13 @@ config CAN_GRCAN

> >>>       endian syntheses of the cores would need some modifications on

> >>>       the hardware level to work.

> >>>

> >>> +config CAN_XILINXCAN

> >>> +   tristate "Xilinx CAN"

> >>> +   depends on ARCH_ZYNQ || MICROBLAZE

> >>

> >> Is Zynq multiarch already?

> > Discussions are going on this

> > So the final thing that Fengguang ( fengguang.wu@intel.com) Proposed

> > is

> >         config CAN_XILINX

> >         tristate "Xilinx CAN"

> >         depends on ARCH_ZYNQ || MICROBLAZE || COMPILE_TEST

> >         depends on COMMON_CLK && HAS_MMIO  # whatever you need for

> > other architectures

>

> > Are you Ok for this?

>

> You have to fill the 2nd depends on with some sane values, though.

>

> [...]

>

Ok

> >>> +/**

> >>> + * struct xcan_priv - This definition define CAN driver instance

> >>> + * @can:                   CAN private data structure.

> >>> + * @tx_head:                       Tx CAN packets ready to send on the

> >> queue

> >>> + * @tx_tail:                       Tx CAN packets successfully sended on the

> >> queue

> >>> + * @tx_max:                        Maximum number packets the driver can

> >> send

> >>> + * @napi:                  NAPI structure

> >>> + * @read_reg:                      For reading data from CAN registers

> >>> + * @write_reg:                     For writing data to CAN registers

> >>> + * @dev:                   Network device data structure

> >>> + * @reg_base:                      Ioremapped address to registers

> >>> + * @irq_flags:                     For request_irq()

> >>> + * @aperclk:                       Pointer to struct clk

> >>> + * @devclk:                        Pointer to struct clk

> >>> + */

> >>> +struct xcan_priv {

> >>> +   struct can_priv can;

> >>> +   unsigned int tx_head;

> >>> +   unsigned int tx_tail;

> >>> +   u32 tx_max;

> >> Please make it an unsigned int, too.

> >>

> > Ok

> >

> >

> >>> +   struct napi_struct napi;

> >>> +   u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);

> >>> +   void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,

> >>> +                   u32 val);

> >>> +   struct net_device *dev;

> >>> +   void __iomem *reg_base;

> >>> +   unsigned long irq_flags;

> >>> +   struct clk *aperclk;

> >>> +   struct clk *devclk;

> >>

> >> Please rename the clock variables to match the names in the DT.

> >>

> > The clock names are different for axi CAN and CANPS case.

> > So will make them as busclk and devclk Are you ok with this?

>

> Why not "ref_clk" and "aper_clk" as used in the DT?

>

One of the comments I got from the Soren(sorenb@xilinx.com)
Is the  clock-names must match the data sheet.
If I Modify the clock names then it is different names for AXI CAN
and CANPS case.

> >>> +};

> >>> +

>

> [...]

>

> >>> +/**

> >>> + * xcan_chip_start - This the drivers start routine

> >>> + * @ndev:  Pointer to net_device structure

> >>> + *

> >>> + * This is the drivers start routine.

> >>> + * Based on the State of the CAN device it puts

> >>> + * the CAN device into a proper mode.

> >>> + *

> >>> + * Return: 0 on success and failure value on error  */ static int

> >>> +xcan_chip_start(struct net_device *ndev) {

> >>> +   struct xcan_priv *priv = netdev_priv(ndev);

> >>> +   u32 err;

> >>> +   unsigned long timeout;

> >>> +

> >>> +   /* Check if it is in reset mode */

> >>> +   err = set_reset_mode(ndev);

> >>> +   if (err < 0)

> >>> +           return err;

> >>> +

> >>> +   err = xcan_set_bittiming(ndev);

> >>> +   if (err < 0)

> >>> +           return err;

> >>> +

> >>> +   /* Enable interrupts */

> >>> +   priv->write_reg(priv, XCAN_IER_OFFSET, XCAN_INTR_ALL);

> >>> +

> >>> +   /* Check whether it is loopback mode or normal mode  */

> >>> +   if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)

> >>> +           /* Put device into loopback mode */

> >>> +           priv->write_reg(priv, XCAN_MSR_OFFSET,

> >> XCAN_MSR_LBACK_MASK);

> >>> +   else

> >>> +           /* The device is in normal mode */

> >>> +           priv->write_reg(priv, XCAN_MSR_OFFSET, 0);

> >>> +

> >>> +   if (priv->can.state == CAN_STATE_STOPPED) {

> >>

> >> Your device is in stopped mode, add you go though set_reset_mode()

> >> already.

> >

> > Will remove this if condition I putted this condition here because in

> > set_reset_mode we are putting the Device state in Stopped state.

>

> So the device is in CAN_STATE_STOPPED, as this code just went through

> set_reset_mode() some lines ago. so it makes no sense to check if it

> (still) is (the code runs serialized here).

>


After removing the if condition and the line(CAN_STATE_STOPPED from the set_reset_mode)
the code for chip_start is below It is better to check whether it is really Configured in the loopback or normal mode
That's why kept the loops as it is.

static int xcan_chip_start(struct net_device *ndev)
{
        struct xcan_priv *priv = netdev_priv(ndev);
        u32 err;
        unsigned long timeout;

        /* Check if it is in reset mode */
        err = set_reset_mode(ndev);
        if (err < 0)
                return err;

        err = xcan_set_bittiming(ndev);
        if (err < 0)
                return err;

        /* Enable interrupts */
        priv->write_reg(priv, XCAN_IER_OFFSET, XCAN_INTR_ALL);

        /* Check whether it is loopback mode or normal mode  */
        if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
                /* Put device into loopback mode */
                priv->write_reg(priv, XCAN_MSR_OFFSET, XCAN_MSR_LBACK_MASK);
        else
                /* The device is in normal mode */
                priv->write_reg(priv, XCAN_MSR_OFFSET, 0);

        priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
        timeout = jiffies + XCAN_TIMEOUT;
        if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
                while ((priv->read_reg(priv, XCAN_SR_OFFSET)
                                & XCAN_SR_LBACK_MASK) == 0) {
                        if (time_after(jiffies, timeout)) {
                                netdev_warn(ndev,
                                        "timedout for loopback mode\n");
                                return -ETIMEDOUT;
                        }
                        usleep_range(500, 10000);
                }
        } else {
                while ((priv->read_reg(priv, XCAN_SR_OFFSET)
                                & XCAN_SR_NORMAL_MASK) == 0) {
                        if (time_after(jiffies, timeout)) {
                                netdev_warn(ndev,
                                        "timedout for normal mode\n");
                                return -ETIMEDOUT;
                        }
                        usleep_range(500, 10000);
                }
        }
        netdev_dbg(ndev, "status:#x%08x\n",
                        priv->read_reg(priv, XCAN_SR_OFFSET));

        priv->can.state = CAN_STATE_ERROR_ACTIVE;
        return 0;
}


Regards,
Kedar.

> >>

> >>> +           /* Enable Xilinx CAN */

> >>> +           priv->write_reg(priv, XCAN_SRR_OFFSET,

> >> XCAN_SRR_CEN_MASK);

> >>> +           priv->can.state = CAN_STATE_ERROR_ACTIVE;

> >>> +           timeout = jiffies + XCAN_TIMEOUT;

> >>> +           if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {

> >>> +                   while ((priv->read_reg(priv, XCAN_SR_OFFSET)

> >>> +                                   & XCAN_SR_LBACK_MASK) == 0) {

> >>> +                           if (time_after(jiffies, timeout)) {

> >>> +                                   netdev_warn(ndev,

> >>> +                                           "timedout for loopback

> >> mode\n");

> >>> +                                   return -ETIMEDOUT;

> >>> +                           }

> >>> +                           usleep_range(500, 10000);

> >>> +                   }

> >>> +           } else {

> >>> +                   while ((priv->read_reg(priv, XCAN_SR_OFFSET)

> >>> +                                   & XCAN_SR_NORMAL_MASK) == 0) {

> >>> +                           if (time_after(jiffies, timeout)) {

> >>> +                                   netdev_warn(ndev,

> >>> +                                           "timedout for normal

> >> mode\n");

> >>> +                                   return -ETIMEDOUT;

> >>> +                           }

> >>> +                           usleep_range(500, 10000);

> >>> +                   }

> >>> +           }

> >>> +           netdev_dbg(ndev, "status:#x%08x\n",

> >>> +                           priv->read_reg(priv, XCAN_SR_OFFSET));

> >>> +   }

> >>> +   priv->can.state = CAN_STATE_ERROR_ACTIVE;

> >>> +   return 0;

> >>> +}

>

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




This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

On 03/11/2014 03:08 PM, Appana Durga Kedareswara Rao wrote:

>>>>> +   struct napi_struct napi;
>>>>> +   u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
>>>>> +   void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
>>>>> +                   u32 val);
>>>>> +   struct net_device *dev;
>>>>> +   void __iomem *reg_base;
>>>>> +   unsigned long irq_flags;
>>>>> +   struct clk *aperclk;
>>>>> +   struct clk *devclk;
>>>>
>>>> Please rename the clock variables to match the names in the DT.
>>>>
>>> The clock names are different for axi CAN and CANPS case.
>>> So will make them as busclk and devclk Are you ok with this?
>>
>> Why not "ref_clk" and "aper_clk" as used in the DT?
>>
> One of the comments I got from the Soren(sorenb@xilinx.com)
> Is the  clock-names must match the data sheet.
> If I Modify the clock names then it is different names for AXI CAN
> and CANPS case.

Sorry, my faul, I thought the names are already these from the
datasheet. As Sören pointed out please use 's_axi_aclk' and
'can_clk' for the DT and for the the variable names in the private
struct, too.

The 'official' name of the ip core seems to be axi_can, should we rename
the driver? I suspect, that Michal wants to keep xilinx in the name for
marketing reasons :P

[...]

>>>>> +/**
>>>>> + * xcan_chip_start - This the drivers start routine
>>>>> + * @ndev:  Pointer to net_device structure
>>>>> + *
>>>>> + * This is the drivers start routine.
>>>>> + * Based on the State of the CAN device it puts
>>>>> + * the CAN device into a proper mode.
>>>>> + *
>>>>> + * Return: 0 on success and failure value on error  */ static int
>>>>> +xcan_chip_start(struct net_device *ndev) {
>>>>> +   struct xcan_priv *priv = netdev_priv(ndev);
>>>>> +   u32 err;
>>>>> +   unsigned long timeout;
>>>>> +
>>>>> +   /* Check if it is in reset mode */
>>>>> +   err = set_reset_mode(ndev);
>>>>> +   if (err < 0)
>>>>> +           return err;
>>>>> +
>>>>> +   err = xcan_set_bittiming(ndev);
>>>>> +   if (err < 0)
>>>>> +           return err;
>>>>> +
>>>>> +   /* Enable interrupts */
>>>>> +   priv->write_reg(priv, XCAN_IER_OFFSET, XCAN_INTR_ALL);
>>>>> +
>>>>> +   /* Check whether it is loopback mode or normal mode  */
>>>>> +   if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
>>>>> +           /* Put device into loopback mode */
>>>>> +           priv->write_reg(priv, XCAN_MSR_OFFSET,
>>>> XCAN_MSR_LBACK_MASK);
>>>>> +   else
>>>>> +           /* The device is in normal mode */
>>>>> +           priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
>>>>> +
>>>>> +   if (priv->can.state == CAN_STATE_STOPPED) {
>>>>
>>>> Your device is in stopped mode, add you go though set_reset_mode()
>>>> already.
>>>
>>> Will remove this if condition I putted this condition here because in
>>> set_reset_mode we are putting the Device state in Stopped state.
>>
>> So the device is in CAN_STATE_STOPPED, as this code just went through
>> set_reset_mode() some lines ago. so it makes no sense to check if it
>> (still) is (the code runs serialized here).
>>
> 
> After removing the if condition and the line(CAN_STATE_STOPPED from
> the set_reset_mode) the code for chip_start is below It is better to
> check whether it is really Configured in the loopback or normal mode 
> That's why kept the loops as it is.
> 
> static int xcan_chip_start(struct net_device *ndev)
> {
>         struct xcan_priv *priv = netdev_priv(ndev);
>         u32 err;
>         unsigned long timeout;
> 
>         /* Check if it is in reset mode */
>         err = set_reset_mode(ndev);
>         if (err < 0)
>                 return err;
> 
>         err = xcan_set_bittiming(ndev);
>         if (err < 0)
>                 return err;
> 
>         /* Enable interrupts */
>         priv->write_reg(priv, XCAN_IER_OFFSET, XCAN_INTR_ALL);
> 
>         /* Check whether it is loopback mode or normal mode  */
>         if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
>                 /* Put device into loopback mode */
>                 priv->write_reg(priv, XCAN_MSR_OFFSET, XCAN_MSR_LBACK_MASK);
>         else
>                 /* The device is in normal mode */
>                 priv->write_reg(priv, XCAN_MSR_OFFSET, 0);

What about using two temp variables? Like this (untested, though):

	u32 reg_msg;
	u32 reg_sr_mask;

	if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
		reg_msr = XCAN_MSR_LBACK_MASK;
		reg_sr_mask = XCAN_SR_LBACK_MASK;
	} else {
		reg_msr = 0x0;
		reg_sr_mask = XCAN_SR_NORMAL_MASK;
	}

	priv->write_reg(priv, XCAN_MSR_OFFSET, reg_msr);
	priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);

	timeout = jiffies + XCAN_TIMEOUT;
	while (!(priv->read_reg(priv, XCAN_SR_OFFSET) & reg_sr_mask)) {
		if (time_after(jiffies, timeout)) {
			netdev_warn(ndev, "time out waiting for correct mode\n")
			return -ETIMEDOUT;
			}
		usleep_range(500, 10000);
	}

> 
>         priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
>         timeout = jiffies + XCAN_TIMEOUT;
>         if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
>                 while ((priv->read_reg(priv, XCAN_SR_OFFSET)
>                                 & XCAN_SR_LBACK_MASK) == 0) {
>                         if (time_after(jiffies, timeout)) {
>                                 netdev_warn(ndev,
>                                         "timedout for loopback mode\n");
>                                 return -ETIMEDOUT;
>                         }
>                         usleep_range(500, 10000);
>                 }
>         } else {
>                 while ((priv->read_reg(priv, XCAN_SR_OFFSET)
>                                 & XCAN_SR_NORMAL_MASK) == 0) {
>                         if (time_after(jiffies, timeout)) {
>                                 netdev_warn(ndev,
>                                         "timedout for normal mode\n");
>                                 return -ETIMEDOUT;
>                         }
>                         usleep_range(500, 10000);
>                 }
>         }
>         netdev_dbg(ndev, "status:#x%08x\n",
>                         priv->read_reg(priv, XCAN_SR_OFFSET));
> 
>         priv->can.state = CAN_STATE_ERROR_ACTIVE;
>         return 0;
> }

Marc

On 03/11/2014 01:48 PM, Marc Kleine-Budde wrote:
> On 03/11/2014 01:34 PM, Appana Durga Kedareswara Rao wrote:
> 
>>>> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig index
>>>> 9e7d95d..b180239 100644
>>>> --- a/drivers/net/can/Kconfig
>>>> +++ b/drivers/net/can/Kconfig
>>>> @@ -125,6 +125,13 @@ config CAN_GRCAN
>>>>       endian syntheses of the cores would need some modifications on
>>>>       the hardware level to work.
>>>>
>>>> +config CAN_XILINXCAN
>>>> +   tristate "Xilinx CAN"
>>>> +   depends on ARCH_ZYNQ || MICROBLAZE
>>>
>>> Is Zynq multiarch already?
>> Discussions are going on this
>> So the final thing that Fengguang ( fengguang.wu@intel.com)
>> Proposed is
>>         config CAN_XILINX
>>         tristate "Xilinx CAN"
>>         depends on ARCH_ZYNQ || MICROBLAZE || COMPILE_TEST
>>         depends on COMMON_CLK && HAS_MMIO  # whatever you need for other architectures
> 
>> Are you Ok for this?
> 
> You have to fill the 2nd depends on with some sane values, though.

This should be that one
config CAN_XILINX
         tristate "Xilinx CAN"
         depends on ARCH_ZYNQ || MICROBLAZE || COMPILE_TEST
         depends on COMMON_CLK && HAS_IOMEM


Is there any driver which is in CAN which not use MMIO accesses?
If not, maybe we should just add dependency to

Thanks,
Michal

Hi guys,


On 03/11/2014 03:31 PM, Marc Kleine-Budde wrote:
> On 03/11/2014 03:08 PM, Appana Durga Kedareswara Rao wrote:
> 
>>>>>> +   struct napi_struct napi;
>>>>>> +   u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
>>>>>> +   void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
>>>>>> +                   u32 val);
>>>>>> +   struct net_device *dev;
>>>>>> +   void __iomem *reg_base;
>>>>>> +   unsigned long irq_flags;
>>>>>> +   struct clk *aperclk;
>>>>>> +   struct clk *devclk;
>>>>>
>>>>> Please rename the clock variables to match the names in the DT.
>>>>>
>>>> The clock names are different for axi CAN and CANPS case.
>>>> So will make them as busclk and devclk Are you ok with this?
>>>
>>> Why not "ref_clk" and "aper_clk" as used in the DT?
>>>
>> One of the comments I got from the Soren(sorenb@xilinx.com)
>> Is the  clock-names must match the data sheet.
>> If I Modify the clock names then it is different names for AXI CAN
>> and CANPS case.
> 
> Sorry, my faul, I thought the names are already these from the
> datasheet. As Sören pointed out please use 's_axi_aclk' and
> 'can_clk' for the DT and for the the variable names in the private
> struct, too.
> 
> The 'official' name of the ip core seems to be axi_can, should we rename
> the driver? I suspect, that Michal wants to keep xilinx in the name for
> marketing reasons :P

I hope that I am not moving to marketing position.  :-)

opb_can, plb_can, axi_can, amba_can are all valid options for this IP.

Maybe in future Xilinx will decide to use different bus and then will just move
all current soft IPs to new bus and drivers will be compatible.
This is exactly what happened when Xilinx moved from OPB to PLB and then
from PLB to AXI.
That's why I think in general having bus name in name doesn't fit for our case.

The same is for clock name which has bus name in it.
For PLB it was called SPLB_Clk and I don't have OPB version but
at least standalone driver points to OPB version where I believe
SPLB_Clk name was not used.

That's why if you want to reflect that clock is coming from bus
you should use any generic name. At least for these soft IPs
and this one is special because it is one which also went to silicon.
Next example is XADC.

That's why I just don't think we can find out better name than
xilinx_can.

Thanks,
Michal

On Wed, 2014-03-12 at 11:18AM +0100, Michal Simek wrote:
> Hi guys,
> 
> 
> On 03/11/2014 03:31 PM, Marc Kleine-Budde wrote:
> > On 03/11/2014 03:08 PM, Appana Durga Kedareswara Rao wrote:
> > 
> >>>>>> +   struct napi_struct napi;
> >>>>>> +   u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
> >>>>>> +   void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
> >>>>>> +                   u32 val);
> >>>>>> +   struct net_device *dev;
> >>>>>> +   void __iomem *reg_base;
> >>>>>> +   unsigned long irq_flags;
> >>>>>> +   struct clk *aperclk;
> >>>>>> +   struct clk *devclk;
> >>>>>
> >>>>> Please rename the clock variables to match the names in the DT.
> >>>>>
> >>>> The clock names are different for axi CAN and CANPS case.
> >>>> So will make them as busclk and devclk Are you ok with this?
> >>>
> >>> Why not "ref_clk" and "aper_clk" as used in the DT?
> >>>
> >> One of the comments I got from the Soren(sorenb@xilinx.com)
> >> Is the  clock-names must match the data sheet.
> >> If I Modify the clock names then it is different names for AXI CAN
> >> and CANPS case.
> > 
> > Sorry, my faul, I thought the names are already these from the
> > datasheet. As Sören pointed out please use 's_axi_aclk' and
> > 'can_clk' for the DT and for the the variable names in the private
> > struct, too.
> > 
> > The 'official' name of the ip core seems to be axi_can, should we rename
> > the driver? I suspect, that Michal wants to keep xilinx in the name for
> > marketing reasons :P
> 
> I hope that I am not moving to marketing position.  :-)
> 
> opb_can, plb_can, axi_can, amba_can are all valid options for this IP.
> 
> Maybe in future Xilinx will decide to use different bus and then will just move
> all current soft IPs to new bus and drivers will be compatible.
> This is exactly what happened when Xilinx moved from OPB to PLB and then
> from PLB to AXI.
> That's why I think in general having bus name in name doesn't fit for our case.
> 
> The same is for clock name which has bus name in it.
> For PLB it was called SPLB_Clk and I don't have OPB version but
> at least standalone driver points to OPB version where I believe
> SPLB_Clk name was not used.

Okay, then 'bus_clk' would probably be fine. That is hopefully obvious
enough to be mapped to a clock input of that IP, while being generic
enough to allow other buses as well.

	Sören


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Michal,

On Tue, Mar 11, 2014 at 10:38:14AM +0100, Michal Simek wrote:
> On 03/11/2014 05:45 AM, Fengguang Wu wrote:
> >>> You probably want something like
> >>>
> >>> 	config CAN_XILINX
> >>> 	tristate "Xilinx CAN"
> >>> 	depends on ARCH_ZYNQ || MICROBLAZE || COMPILE_TEST
> >>> 	depends on COMMON_CLK && HAS_MMIO  # whatever you need for other architectures
> >>
> >> Fengguang: Are you use COMPILE_TEST in zero day testing system?
> > 
> > Yeah, the randconfig build/boot tests should cover both COMPILE_TEST
> > and !COMPILE_TEST cases.
> 
> But the point is that randconfig is really random it means it definitely can
> cover but not there is no certainty that for that build, you are running,
> has both option for all architectures enabled.
> Or do I read it wrongly?

Good point!  Since the 0day system is running randconfig tests
everyday, it has very good opportunity to cover the COMPILE_TEST case.
But to be sure, I'll explicitly enable COMPILE_TEST in some cases.

Thanks,
Fengguang
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Marc,

> -----Original Message-----

> From: Michal Simek [mailto:monstr@monstr.eu]

> Sent: Wednesday, March 12, 2014 3:32 PM

> To: Marc Kleine-Budde

> Cc: Appana Durga Kedareswara Rao; linux-can@vger.kernel.org;

> netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-

> kernel@vger.kernel.org; devicetree@vger.kernel.org; robh+dt@kernel.org;

> grant.likely@linaro.org; Michal Simek; wg@grandegger.com;

> fengguang.wu@intel.com

> Subject: Re: [PATCH v5] can: xilinx CAN controller support.

>

> On 03/11/2014 01:48 PM, Marc Kleine-Budde wrote:

> > On 03/11/2014 01:34 PM, Appana Durga Kedareswara Rao wrote:

> >

> >>>> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig

> >>>> index

> >>>> 9e7d95d..b180239 100644

> >>>> --- a/drivers/net/can/Kconfig

> >>>> +++ b/drivers/net/can/Kconfig

> >>>> @@ -125,6 +125,13 @@ config CAN_GRCAN

> >>>>       endian syntheses of the cores would need some modifications on

> >>>>       the hardware level to work.

> >>>>

> >>>> +config CAN_XILINXCAN

> >>>> +   tristate "Xilinx CAN"

> >>>> +   depends on ARCH_ZYNQ || MICROBLAZE

> >>>

> >>> Is Zynq multiarch already?

> >> Discussions are going on this

> >> So the final thing that Fengguang ( fengguang.wu@intel.com) Proposed

> >> is

> >>         config CAN_XILINX

> >>         tristate "Xilinx CAN"

> >>         depends on ARCH_ZYNQ || MICROBLAZE || COMPILE_TEST

> >>         depends on COMMON_CLK && HAS_MMIO  # whatever you need for

> >> other architectures

> >

> >> Are you Ok for this?

> >

> > You have to fill the 2nd depends on with some sane values, though.

>

> This should be that one

> config CAN_XILINX

>          tristate "Xilinx CAN"

>          depends on ARCH_ZYNQ || MICROBLAZE || COMPILE_TEST

>          depends on COMMON_CLK && HAS_IOMEM

>

>

> Is there any driver which is in CAN which not use MMIO accesses?

> If not, maybe we should just add dependency to




Can you please comment on this?
I had update driver with all the other comments I had a little confusion regarding
Which Kconfig options I need to use.

Regards,
Kedar.


> 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

>




This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.