Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755633Ab0KIM06 (ORCPT ); Tue, 9 Nov 2010 07:26:58 -0500 Received: from sm-d311v.smileserver.ne.jp ([203.211.202.206]:2277 "EHLO sm-d311v.smileserver.ne.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755546Ab0KIM0x (ORCPT ); Tue, 9 Nov 2010 07:26:53 -0500 Message-ID: <00fe01cb8009$62e11410$66f8800a@maildom.okisemi.com> From: "Tomoya MORINAGA" To: "Marc Kleine-Budde" Cc: "Wolfgang Grandegger" , "David S. Miller" , "Wolfram Sang" , "Christian Pellegrin" , "Barry Song" <21cnbao@gmail.com>, "Samuel Ortiz" , , , "LKML" , , , , , "Masayuki Ohtake" , , References: <005301cb7ffa$5b63cd90$66f8800a@maildom.okisemi.com> Subject: Re: [PATCH net-next-2.6 v2] can: Topcliff: PCH_CAN driver: Fix build warnings Date: Tue, 9 Nov 2010 21:26:50 +0900 MIME-Version: 1.0 Content-Type: text/plain; charset="iso-2022-jp" Content-Transfer-Encoding: 7bit X-Priority: 3 X-MSMail-Priority: Normal X-Mailer: Microsoft Outlook Express 6.00.2800.1983 X-MimeOLE: Produced By Microsoft MimeOLE V6.00.2800.1983 X-Hosting-Pf: 0 X-NAI-Spam-Score: 2.5 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 61192 Lines: 1967 Sorry, for late response. ----- Original Message ----- From: "Tomoya MORINAGA" To: "Tomoya MORINAGA" Sent: Tuesday, November 09, 2010 7:39 PM Subject: Re: [PATCH net-next-2.6 v2] can: Topcliff: PCH_CAN driver: Fix build warnings > On 11/02/2010 11:27 AM, Tomoya MORINAGA wrote: > > On Saturday, October 30, 2010 1:23 AM, Marc Kleine-Budde wrote: > > > >>> The driver has already been merged. Please send incremental patches > >>> against david's net-2.6 branch. > > > > I agree. > > > >> > >> Here a review, find comments inline. Lets talk about my remarks, please > >> answer inline and don't delete the code. > >> > >> Can you please explain me your locking sheme? If I understand the > >> documenation correctly the two message interfaces can be used mutual. > >> And you use one for rx the other one for tx. > > > > I show our locking scheme. > > When CPU accesses MessageRAM via IF1, CPU protect until read-modify-write > > so that IF2 access not occurred, vice versa. > > Why is that needed? For MessageRAM data consistency. > > > > >> > >> Please use netdev_ instead of dev_ for debug. > > > > I agree. > > > >> > >>> --- /dev/null > >>> +++ b/drivers/net/can/pch_can.c > >>> @@ -0,0 +1,1436 @@ > >>> +/* > >>> + * Copyright (C) 1999 - 2010 Intel Corporation. > >>> + * Copyright (C) 2010 OKI SEMICONDUCTOR CO., LTD. > >>> + * > >>> + * 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; version 2 of the License. > >>> + * > >>> + * 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, write to the Free Software > >>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307, USA. > >>> + */ > >>> + > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> + > >>> +#define MAX_MSG_OBJ 32 > >>> +#define MSG_OBJ_RX 0 /* The receive message object flag. */ > >>> +#define MSG_OBJ_TX 1 /* The transmit message object flag. */ > >>> + > >>> +#define CAN_CTRL_INIT 0x0001 /* The INIT bit of CANCONT register. */ > >>> +#define CAN_CTRL_IE 0x0002 /* The IE bit of CAN control register */ > >>> +#define CAN_CTRL_IE_SIE_EIE 0x000e > >>> +#define CAN_CTRL_CCE 0x0040 > >>> +#define CAN_CTRL_OPT 0x0080 /* The OPT bit of CANCONT register. */ > >>> +#define CAN_OPT_SILENT 0x0008 /* The Silent bit of CANOPT reg. */ > >>> +#define CAN_OPT_LBACK 0x0010 /* The LoopBack bit of CANOPT reg. */ > >>> +#define CAN_CMASK_RX_TX_SET 0x00f3 > >>> +#define CAN_CMASK_RX_TX_GET 0x0073 > >>> +#define CAN_CMASK_ALL 0xff > >>> +#define CAN_CMASK_RDWR 0x80 > >>> +#define CAN_CMASK_ARB 0x20 > >>> +#define CAN_CMASK_CTRL 0x10 > >>> +#define CAN_CMASK_MASK 0x40 > >>> +#define CAN_CMASK_NEWDAT 0x04 > >>> +#define CAN_CMASK_CLRINTPND 0x08 > >>> + > >>> +#define CAN_IF_MCONT_NEWDAT 0x8000 > >>> +#define CAN_IF_MCONT_INTPND 0x2000 > >>> +#define CAN_IF_MCONT_UMASK 0x1000 > >>> +#define CAN_IF_MCONT_TXIE 0x0800 > >>> +#define CAN_IF_MCONT_RXIE 0x0400 > >>> +#define CAN_IF_MCONT_RMTEN 0x0200 > >>> +#define CAN_IF_MCONT_TXRQXT 0x0100 > >>> +#define CAN_IF_MCONT_EOB 0x0080 > >>> +#define CAN_IF_MCONT_DLC 0x000f > >>> +#define CAN_IF_MCONT_MSGLOST 0x4000 > >>> +#define CAN_MASK2_MDIR_MXTD 0xc000 > >>> +#define CAN_ID2_DIR 0x2000 > >>> +#define CAN_ID_MSGVAL 0x8000 > >>> + > >>> +#define CAN_STATUS_INT 0x8000 > >>> +#define CAN_IF_CREQ_BUSY 0x8000 > >>> +#define CAN_ID2_XTD 0x4000 > >>> + > >>> +#define CAN_REC 0x00007f00 > >>> +#define CAN_TEC 0x000000ff > >> > >> A prefix for like PCH_ instead of CAN_ for all those define above would > >> be fine to avoid namespace clashes and/or confusion with the defines from the socketcan framework. > >> > > > > I agree. > > > >>> + > >>> +#define PCH_RX_OK 0x00000010 > >>> +#define PCH_TX_OK 0x00000008 > >>> +#define PCH_BUS_OFF 0x00000080 > >>> +#define PCH_EWARN 0x00000040 > >>> +#define PCH_EPASSIV 0x00000020 > >>> +#define PCH_LEC0 0x00000001 > >>> +#define PCH_LEC1 0x00000002 > >>> +#define PCH_LEC2 0x00000004 > >> > >> These are just single set bit, please use BIT() > >> Consider adding the name of the corresponding register to the define's > >> name. > > > > I agree. > > > >> > >>> +#define PCH_LEC_ALL (PCH_LEC0 | PCH_LEC1 | PCH_LEC2) > >>> +#define PCH_STUF_ERR PCH_LEC0 > >>> +#define PCH_FORM_ERR PCH_LEC1 > >>> +#define PCH_ACK_ERR (PCH_LEC0 | PCH_LEC1) > >>> +#define PCH_BIT1_ERR PCH_LEC2 > >>> +#define PCH_BIT0_ERR (PCH_LEC0 | PCH_LEC2) > >>> +#define PCH_CRC_ERR (PCH_LEC1 | PCH_LEC2) > >>> + > >>> +/* bit position of certain controller bits. */ > >>> +#define BIT_BITT_BRP 0 > >>> +#define BIT_BITT_SJW 6 > >>> +#define BIT_BITT_TSEG1 8 > >>> +#define BIT_BITT_TSEG2 12 > >>> +#define BIT_IF1_MCONT_RXIE 10 > >>> +#define BIT_IF2_MCONT_TXIE 11 > >>> +#define BIT_BRPE_BRPE 6 > >>> +#define BIT_ES_TXERRCNT 0 > >>> +#define BIT_ES_RXERRCNT 8 > >> > >> these are usually called SHIFT > > > > I agree. Is the below TRUE ? > > e.g.#define PCH_SHIFT_BITT_BRP 0 > > I would put the SHIFT at the end, YMMV > > #define PCH_BIT_BRP_SHIFT I agree. > > > > >> > >>> +#define MSK_BITT_BRP 0x3f > >>> +#define MSK_BITT_SJW 0xc0 > >>> +#define MSK_BITT_TSEG1 0xf00 > >>> +#define MSK_BITT_TSEG2 0x7000 > >>> +#define MSK_BRPE_BRPE 0x3c0 > >>> +#define MSK_BRPE_GET 0x0f > >>> +#define MSK_CTRL_IE_SIE_EIE 0x07 > >>> +#define MSK_MCONT_TXIE 0x08 > >>> +#define MSK_MCONT_RXIE 0x10 > >> > >> MSK or MASK is okay, however the last two are just single bits. > >> > >> Please add a PCH_ prefix here, too. > > > > I agree. > > > >> > >>> +#define PCH_CAN_NO_TX_BUFF 1 > >>> +#define COUNTER_LIMIT 10 > >> > >> dito > > > > I agree. > > > >> > >>> + > >>> +#define PCH_CAN_CLK 50000000 /* 50MHz */ > >>> + > >>> +/* > >>> + * Define the number of message object. > >>> + * PCH CAN communications are done via Message RAM. > >>> + * The Message RAM consists of 32 message objects. > >>> + */ > >>> +#define PCH_RX_OBJ_NUM 26 /* 1~ PCH_RX_OBJ_NUM is Rx*/ > >>> +#define PCH_TX_OBJ_NUM 6 /* PCH_RX_OBJ_NUM is RX ~ Tx*/ > >>> +#define PCH_OBJ_NUM (PCH_TX_OBJ_NUM + PCH_RX_OBJ_NUM) > >> > >> You define MAX_MSG_OBJ earlier, seems like two names for the same value. > > > > In case, a use uses all message objects(=32), you are right. > > But user does not alway use all message object. > > No one will change these values if the driver isn't buggy. And it > doesn't make any sense to not use all objects. I see. I will delete PCH_OBJ_NUM or MAX_MSG_OBJ. > > > > > > >> > >>> + > >>> +#define PCH_FIFO_THRESH 16 > >>> + > >>> +enum pch_can_mode { > >>> + PCH_CAN_ENABLE, > >>> + PCH_CAN_DISABLE, > >>> + PCH_CAN_ALL, > >>> + PCH_CAN_NONE, > >>> + PCH_CAN_STOP, > >>> + PCH_CAN_RUN, > >>> +}; > >>> + > >>> +struct pch_can_regs { > >>> + u32 cont; > >>> + u32 stat; > >>> + u32 errc; > >>> + u32 bitt; > >>> + u32 intr; > >>> + u32 opt; > >>> + u32 brpe; > >>> + u32 reserve1; > >> > >> VVVV > >>> + u32 if1_creq; > >>> + u32 if1_cmask; > >>> + u32 if1_mask1; > >>> + u32 if1_mask2; > >>> + u32 if1_id1; > >>> + u32 if1_id2; > >>> + u32 if1_mcont; > >>> + u32 if1_dataa1; > >>> + u32 if1_dataa2; > >>> + u32 if1_datab1; > >>> + u32 if1_datab2; > >> ^^^^ > >> > >> these registers and.... > >> > >>> + u32 reserve2; > >>> + u32 reserve3[12]; > >> > >> ...and these > >> > >> VVVV > >>> + u32 if2_creq; > >>> + u32 if2_cmask; > >>> + u32 if2_mask1; > >>> + u32 if2_mask2; > >>> + u32 if2_id1; > >>> + u32 if2_id2; > >>> + u32 if2_mcont; > >>> + u32 if2_dataa1; > >>> + u32 if2_dataa2; > >>> + u32 if2_datab1; > >>> + u32 if2_datab2; > >> > >> ^^^^ > >> > >> ...are identical. I suggest to make a struct defining a complete > >> "Message Interface Register Set". If you include the correct number of > >> reserved bytes in the struct, you can have an array of two of these > >> structs in the struct pch_can_regs. > > > > To me, IMHOHO, it looks insignificant point. > > Please show the merit ? > > See Wolfgangs comments. You can get rid of duplicated code.... Using this method, I can't image to be able to reduce code size, now. However I will try it. > > > > >> > >>> + u32 reserve4; > >>> + u32 reserve5[20]; > >>> + u32 treq1; > >>> + u32 treq2; > >>> + u32 reserve6[2]; > >>> + u32 reserve7[56]; > >>> + u32 reserve8[3]; > >>> + u32 srst; > >>> +}; > >>> + > >>> +struct pch_can_priv { > >>> + struct can_priv can; > >>> + struct pci_dev *dev; > >>> + unsigned int tx_enable[MAX_MSG_OBJ]; > >>> + unsigned int rx_enable[MAX_MSG_OBJ]; > >>> + unsigned int rx_link[MAX_MSG_OBJ]; > >>> + unsigned int int_enables; > >>> + unsigned int int_stat; > >>> + struct net_device *ndev; > >>> + spinlock_t msgif_reg_lock; /* Message Interface Registers Access Lock*/ > >> ^^^ > >> please add a whitespace > > > > I agree. > > > >> > >>> + unsigned int msg_obj[MAX_MSG_OBJ]; > >>> + struct pch_can_regs __iomem *regs; > >>> + struct napi_struct napi; > >>> + unsigned int tx_obj; /* Point next Tx Obj index */ > >>> + unsigned int use_msi; > >>> +}; > >>> + > >>> +static struct can_bittiming_const pch_can_bittiming_const = { > >>> + .name = "pch_can", > >>> + .tseg1_min = 1, > >>> + .tseg1_max = 16, > >>> + .tseg2_min = 1, > >>> + .tseg2_max = 8, > >>> + .sjw_max = 4, > >>> + .brp_min = 1, > >>> + .brp_max = 1024, /* 6bit + extended 4bit */ > >>> + .brp_inc = 1, > >>> +}; > >>> + > >>> +static DEFINE_PCI_DEVICE_TABLE(pch_pci_tbl) = { > >>> + {PCI_VENDOR_ID_INTEL, 0x8818, PCI_ANY_ID, PCI_ANY_ID,}, > >>> + {0,} > >>> +}; > >>> +MODULE_DEVICE_TABLE(pci, pch_pci_tbl); > >>> + > >>> +static inline void pch_can_bit_set(u32 *addr, u32 mask) > >> ^^^^^ > >> > >> that should be an void __iomem *, see mail I've send the other day. > >> Please use sparse to check for this kinds of errors. > >> (Compile the driver with C=2, i.e.: make drivers/net/can/pch_can.ko C=2) > >> > > > > I agree. > > > >>> +{ > >>> + iowrite32(ioread32(addr) | mask, addr); > >>> +} > >>> + > >>> +static inline void pch_can_bit_clear(u32 *addr, u32 mask) > >> ^^^^^ > >> > >> dito > > > > I agree. > > > >> > >>> +{ > >>> + iowrite32(ioread32(addr) & ~mask, addr); > >>> +} > >>> + > >>> +static void pch_can_set_run_mode(struct pch_can_priv *priv, > >>> + enum pch_can_mode mode) > >>> +{ > >>> + switch (mode) { > >>> + case PCH_CAN_RUN: > >>> + pch_can_bit_clear(&priv->regs->cont, CAN_CTRL_INIT); > >>> + break; > >>> + > >>> + case PCH_CAN_STOP: > >>> + pch_can_bit_set(&priv->regs->cont, CAN_CTRL_INIT); > >>> + break; > >>> + > >>> + default: > >>> + dev_err(&priv->ndev->dev, "%s -> Invalid Mode.\n", __func__); > >>> + break; > >>> + } > >>> +} > >>> + > >>> +static void pch_can_set_optmode(struct pch_can_priv *priv) > >>> +{ > >>> + u32 reg_val = ioread32(&priv->regs->opt); > >>> + > >>> + if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) > >>> + reg_val |= CAN_OPT_SILENT; > >>> + > >>> + if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) > >>> + reg_val |= CAN_OPT_LBACK; > >>> + > >>> + pch_can_bit_set(&priv->regs->cont, CAN_CTRL_OPT); > >>> + iowrite32(reg_val, &priv->regs->opt); > >>> +} > >>> + > >> > >> IMHO the function name is missleading, if I understand the code > >> correctly, this functions triggers the transmission of the message. > >> After this it checks for busy, > > > > Yes, your understanding is TRUE. > > > >> but > >> > >>> +static void pch_can_check_if_busy(u32 __iomem *creq_addr, u32 num) > >> ^^^^ > >> > > > > Yes, me too. > > I will rename the function name. > > > > How about "pch_can_rw_msg_obj" > > > >> that should probaby be a void > > What't the above mean ? > > pch_can_check_if_busy is already "void" function. > > >>> +static void pch_can_check_if_busy(u32 __iomem *creq_addr, u32 num) > >> ^^^^ > > That u32 should be a void. I agree. > > BTW: Does the Intel chip support x64? If so, have you tested the driver > on a 64 bit kernel. > > > > >> > >>> +{ > >>> + u32 counter = COUNTER_LIMIT; > >>> + u32 ifx_creq; > >>> + > >>> + iowrite32(num, creq_addr); > >>> + while (counter) { > >>> + ifx_creq = ioread32(creq_addr) & CAN_IF_CREQ_BUSY; > >>> + if (!ifx_creq) > >>> + break; > >>> + counter--; > >>> + udelay(1); > >>> + } > >>> + if (!counter) > >>> + pr_err("%s:IF1 BUSY Flag is set forever.\n", __func__); > >>> +} > >>> + > >>> +static void pch_can_set_int_enables(struct pch_can_priv *priv, > >>> + enum pch_can_mode interrupt_no) > >>> +{ > >>> + switch (interrupt_no) { > >>> + case PCH_CAN_ENABLE: > >>> + pch_can_bit_set(&priv->regs->cont, CAN_CTRL_IE); > >> > >> noone uses this case. > > > > I agree. > > > >> > >>> + break; > >>> + > >>> + case PCH_CAN_DISABLE: > >>> + pch_can_bit_clear(&priv->regs->cont, CAN_CTRL_IE); > >>> + break; > >>> + > >>> + case PCH_CAN_ALL: > >>> + pch_can_bit_set(&priv->regs->cont, CAN_CTRL_IE_SIE_EIE); > >>> + break; > >>> + > >>> + case PCH_CAN_NONE: > >>> + pch_can_bit_clear(&priv->regs->cont, CAN_CTRL_IE_SIE_EIE); > >>> + break; > >>> + > >>> + default: > >>> + dev_err(&priv->ndev->dev, "Invalid interrupt number.\n"); > >>> + break; > >>> + } > >>> +} > >>> + > >>> +static void pch_can_set_rx_enable(struct pch_can_priv *priv, u32 buff_num, > >>> + int set) > >>> +{ > >>> + unsigned long flags; > >>> + > >>> + spin_lock_irqsave(&priv->msgif_reg_lock, flags); > >>> + /* Reading the receive buffer data from RAM to Interface1 registers */ > >>> + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if1_cmask); > >>> + pch_can_check_if_busy(&priv->regs->if1_creq, buff_num); > >>> + > >>> + /* Setting the IF1MASK1 register to access MsgVal and RxIE bits */ > >>> + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_ARB | CAN_CMASK_CTRL, > >>> + &priv->regs->if1_cmask); > >>> + > >>> + if (set == 1) { > >>> + /* Setting the MsgVal and RxIE bits */ > >>> + pch_can_bit_set(&priv->regs->if1_mcont, CAN_IF_MCONT_RXIE); > >>> + pch_can_bit_set(&priv->regs->if1_id2, CAN_ID_MSGVAL); > >>> + > >>> + } else if (set == 0) { > >>> + /* Resetting the MsgVal and RxIE bits */ > >>> + pch_can_bit_clear(&priv->regs->if1_mcont, CAN_IF_MCONT_RXIE); > >>> + pch_can_bit_clear(&priv->regs->if1_id2, CAN_ID_MSGVAL); > >>> + } > >>> + > >>> + pch_can_check_if_busy(&priv->regs->if1_creq, buff_num); > >>> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags); > >>> +} > >>> + > >>> +static void pch_can_rx_enable_all(struct pch_can_priv *priv) > >>> +{ > >>> + int i; > >>> + > >>> + /* Traversing to obtain the object configured as receivers. */ > >>> + for (i = 1; i <= PCH_RX_OBJ_NUM; i++) > >>> + pch_can_set_rx_enable(priv, i, 1); > >>> +} > >>> + > >>> +static void pch_can_rx_disable_all(struct pch_can_priv *priv) > >>> +{ > >>> + int i; > >>> + > >>> + /* Traversing to obtain the object configured as receivers. */ > >>> + for (i = 1; i <= PCH_RX_OBJ_NUM; i++) > >>> + pch_can_set_rx_enable(priv, i, 0); > >>> +} > >>> + > >>> +static void pch_can_set_tx_enable(struct pch_can_priv *priv, u32 buff_num, > >>> + u32 set) > >>> +{ > >>> + unsigned long flags; > >>> + > >>> + spin_lock_irqsave(&priv->msgif_reg_lock, flags); > >>> + /* Reading the Msg buffer from Message RAM to Interface2 registers. */ > >>> + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if2_cmask); > >>> + pch_can_check_if_busy(&priv->regs->if2_creq, buff_num); > >>> + > >>> + /* Setting the IF2CMASK register for accessing the > >>> + MsgVal and TxIE bits */ > >>> + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_ARB | CAN_CMASK_CTRL, > >>> + &priv->regs->if2_cmask); > >>> + > >>> + if (set == 1) { > >>> + /* Setting the MsgVal and TxIE bits */ > >>> + pch_can_bit_set(&priv->regs->if2_mcont, CAN_IF_MCONT_TXIE); > >>> + pch_can_bit_set(&priv->regs->if2_id2, CAN_ID_MSGVAL); > >>> + } else if (set == 0) { > >>> + /* Resetting the MsgVal and TxIE bits. */ > >>> + pch_can_bit_clear(&priv->regs->if2_mcont, CAN_IF_MCONT_TXIE); > >>> + pch_can_bit_clear(&priv->regs->if2_id2, CAN_ID_MSGVAL); > >>> + } > >>> + > >>> + pch_can_check_if_busy(&priv->regs->if2_creq, buff_num); > >>> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags); > >>> +} > >>> + > >>> +static void pch_can_tx_enable_all(struct pch_can_priv *priv) > >>> +{ > >>> + int i; > >>> + > >>> + /* Traversing to obtain the object configured as transmit object. */ > >>> + for (i = PCH_RX_OBJ_NUM + 1; i <= PCH_OBJ_NUM; i++) > >>> + pch_can_set_tx_enable(priv, i, 1); > >>> +} > >>> + > >>> +static void pch_can_tx_disable_all(struct pch_can_priv *priv) > >>> +{ > >>> + int i; > >>> + > >>> + /* Traversing to obtain the object configured as transmit object. */ > >>> + for (i = PCH_RX_OBJ_NUM + 1; i <= PCH_OBJ_NUM; i++) > >>> + pch_can_set_tx_enable(priv, i, 0); > >>> +} > >>> + > >>> +static int pch_can_int_pending(struct pch_can_priv *priv) > >> ^^^ > >> > >> make it u32 as it returns a register value, or a u16 as you only use > >> the 16 lower bits. > > > > I agree. I will modify to u32. > > > >> > >>> +{ > >>> + return ioread32(&priv->regs->intr) & 0xffff; > >>> +} > >>> + > >>> +static void pch_can_clear_buffers(struct pch_can_priv *priv) > >>> +{ > >>> + int i; /* Msg Obj ID (1~32) */ > >>> + > >>> + for (i = 1; i <= PCH_RX_OBJ_NUM; i++) { > >> > >> IMHO the readability would be improved if you define something like > >> PCH_RX_OBJ_START and PCH_RX_OBJ_END. > > > > I agree. > > > >> > >>> + iowrite32(CAN_CMASK_RX_TX_SET, &priv->regs->if1_cmask); > >>> + iowrite32(0xffff, &priv->regs->if1_mask1); > >>> + iowrite32(0xffff, &priv->regs->if1_mask2); > >>> + iowrite32(0x0, &priv->regs->if1_id1); > >>> + iowrite32(0x0, &priv->regs->if1_id2); > >>> + iowrite32(0x0, &priv->regs->if1_mcont); > >>> + iowrite32(0x0, &priv->regs->if1_dataa1); > >>> + iowrite32(0x0, &priv->regs->if1_dataa2); > >>> + iowrite32(0x0, &priv->regs->if1_datab1); > >>> + iowrite32(0x0, &priv->regs->if1_datab2); > >>> + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_MASK | > >>> + CAN_CMASK_ARB | CAN_CMASK_CTRL, > >>> + &priv->regs->if1_cmask); > >>> + pch_can_check_if_busy(&priv->regs->if1_creq, i); > >>> + } > >>> + > >>> + for (i = PCH_RX_OBJ_NUM + 1; i <= PCH_OBJ_NUM; i++) { > >> ^^^^^^^^^^^^^^^^^^ > >> dito for TX objects > > > > I agree. > > > >> > >>> + iowrite32(CAN_CMASK_RX_TX_SET, &priv->regs->if2_cmask); > >>> + iowrite32(0xffff, &priv->regs->if2_mask1); > >>> + iowrite32(0xffff, &priv->regs->if2_mask2); > >>> + iowrite32(0x0, &priv->regs->if2_id1); > >>> + iowrite32(0x0, &priv->regs->if2_id2); > >>> + iowrite32(0x0, &priv->regs->if2_mcont); > >>> + iowrite32(0x0, &priv->regs->if2_dataa1); > >>> + iowrite32(0x0, &priv->regs->if2_dataa2); > >>> + iowrite32(0x0, &priv->regs->if2_datab1); > >>> + iowrite32(0x0, &priv->regs->if2_datab2); > >>> + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_MASK | CAN_CMASK_ARB | > >>> + CAN_CMASK_CTRL, &priv->regs->if2_cmask); > >>> + pch_can_check_if_busy(&priv->regs->if2_creq, i); > >>> + } > >>> +} > >>> + > >>> +static void pch_can_config_rx_tx_buffers(struct pch_can_priv *priv) > >>> +{ > >>> + int i; > >>> + unsigned long flags; > >>> + > >>> + spin_lock_irqsave(&priv->msgif_reg_lock, flags); > >>> + > >>> + for (i = 1; i <= PCH_RX_OBJ_NUM; i++) { > >>> + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if1_cmask); > >>> + pch_can_check_if_busy(&priv->regs->if1_creq, i); > >> > >> If I understand the code correctly, the about function triggers a > >> transfer. Why do you first trigger a transfer, then set the message contents.... > > > > For doing Read-Modify-Write. > > As to fixed parameter of message object, it doesn't be modified every access. > > I see. > > > We will modify to write only. > > > >>> + > >>> + iowrite32(0x0, &priv->regs->if1_id1); > >>> + iowrite32(0x0, &priv->regs->if1_id2); > >>> + > >>> + pch_can_bit_set(&priv->regs->if1_mcont, CAN_IF_MCONT_UMASK); > >> > >> Why do you set the "Use acceptance mask" bit? We want to receive > >> all can messages. > > > > Without "Use acceptance mask" means received packet matched ID[28:0] only. > > As a result, filter is enabled. > > > > With "Use acceptance mask" and setting Msk[0:28]=all 1, all packets can be received(=No filter state) > > Thanks for the explenation. > > > > >> > >>> + > >>> + /* Set FIFO mode set to 0 except last Rx Obj*/ > >>> + pch_can_bit_clear(&priv->regs->if1_mcont, CAN_IF_MCONT_EOB); > >>> + /* In case FIFO mode, Last EoB of Rx Obj must be 1 */ > >>> + if (i == (PCH_RX_OBJ_NUM - 1)) > >>> + pch_can_bit_set(&priv->regs->if1_mcont, > >>> + CAN_IF_MCONT_EOB); > >> > >> Make it if () { } else { }, please. > > > > Sorry, I can't understand. > > else {} is not necessary. > > Please look at the code block above, again. You frist clean the bit > unconditionally, then you set the bit in the if. Please make it: > > if (last) > set_bit > else > clear_bit I understand. I will modify like above. Thanks. > > > > >> > >>> + > >>> + iowrite32(0, &priv->regs->if1_mask1); > >>> + pch_can_bit_clear(&priv->regs->if1_mask2, > >>> + 0x1fff | CAN_MASK2_MDIR_MXTD); > >>> + > >>> + /* Setting CMASK for writing */ > >>> + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_MASK | CAN_CMASK_ARB | > >>> + CAN_CMASK_CTRL, &priv->regs->if1_cmask); > >>> + > >>> + pch_can_check_if_busy(&priv->regs->if1_creq, i); > >> > >> ...and then trigger the transfer again? > > > > This means Read-Modify-Write. > > ic > > > > >> > >>> + } > >>> + > >>> + for (i = PCH_RX_OBJ_NUM + 1; i <= PCH_OBJ_NUM; i++) { > >>> + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if2_cmask); > >>> + pch_can_check_if_busy(&priv->regs->if2_creq, i); > >> > >> same question about triggering the transfer 2 times applied here, too > > > > ditto. > > > >>> + > >>> + /* Resetting DIR bit for reception */ > >>> + iowrite32(0x0, &priv->regs->if2_id1); > >>> + iowrite32(0x0, &priv->regs->if2_id2); > >>> + pch_can_bit_set(&priv->regs->if2_id2, CAN_ID2_DIR); > >> > >> Can you combine the two accesses to >if2_id2 into one? > > > > I agree. > > > >> > >>> + > >>> + /* Setting EOB bit for transmitter */ > >>> + iowrite32(CAN_IF_MCONT_EOB, &priv->regs->if2_mcont); > >>> + > >>> + pch_can_bit_set(&priv->regs->if2_mcont, > >>> + CAN_IF_MCONT_UMASK); > >> > >> dito for if2_mcont > > > > ditto. > > > >> > >>> + > >>> + iowrite32(0, &priv->regs->if2_mask1); > >>> + pch_can_bit_clear(&priv->regs->if2_mask2, 0x1fff); > >>> + > >>> + /* Setting CMASK for writing */ > >>> + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_MASK | CAN_CMASK_ARB | > >>> + CAN_CMASK_CTRL, &priv->regs->if2_cmask); > >>> + > >>> + pch_can_check_if_busy(&priv->regs->if2_creq, i); > >>> + } > >>> + > >>> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags); > >>> +} > >>> + > >>> +static void pch_can_init(struct pch_can_priv *priv) > >>> +{ > >>> + /* Stopping the Can device. */ > >>> + pch_can_set_run_mode(priv, PCH_CAN_STOP); > >>> + > >>> + /* Clearing all the message object buffers. */ > >>> + pch_can_clear_buffers(priv); > >>> + > >>> + /* Configuring the respective message object as either rx/tx object. */ > >>> + pch_can_config_rx_tx_buffers(priv); > >>> + > >>> + /* Enabling the interrupts. */ > >>> + pch_can_set_int_enables(priv, PCH_CAN_ALL); > >>> +} > >>> + > >>> +static void pch_can_release(struct pch_can_priv *priv) > >>> +{ > >>> + /* Stooping the CAN device. */ > >>> + pch_can_set_run_mode(priv, PCH_CAN_STOP); > >>> + > >>> + /* Disabling the interrupts. */ > >>> + pch_can_set_int_enables(priv, PCH_CAN_NONE); > >>> + > >>> + /* Disabling all the receive object. */ > >>> + pch_can_rx_disable_all(priv); > >>> + > >>> + /* Disabling all the transmit object. */ > >>> + pch_can_tx_disable_all(priv); > >>> +} > >>> + > >>> +/* This function clears interrupt(s) from the CAN device. */ > >>> +static void pch_can_int_clr(struct pch_can_priv *priv, u32 mask) > >>> +{ > >>> + if (mask == CAN_STATUS_INT) { > >> > >> is this a valid case? > > > > This "if" is always false. > > I will delete this condition. > > > >> > >>> + ioread32(&priv->regs->stat); > >>> + return; > >>> + } > >>> + > >>> + /* Clear interrupt for transmit object */ > >>> + if ((mask >= 1) && (mask <= PCH_RX_OBJ_NUM)) { > >>> + /* Setting CMASK for clearing the reception interrupts. */ > >>> + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_CTRL | CAN_CMASK_ARB, > >>> + &priv->regs->if1_cmask); > >>> + > >>> + /* Clearing the Dir bit. */ > >>> + pch_can_bit_clear(&priv->regs->if1_id2, CAN_ID2_DIR); > >>> + > >>> + /* Clearing NewDat & IntPnd */ > >>> + pch_can_bit_clear(&priv->regs->if1_mcont, > >>> + CAN_IF_MCONT_NEWDAT | CAN_IF_MCONT_INTPND); > >>> + > >>> + pch_can_check_if_busy(&priv->regs->if1_creq, mask); > >>> + } else if ((mask > PCH_RX_OBJ_NUM) && (mask <= PCH_OBJ_NUM)) { > >>> + /* Setting CMASK for clearing interrupts for > >>> + frame transmission. */ > >> > >> /* > >> * this is the prefered style of multi line comments, > >> * please adjust you comments > >> */ > > > > I understand. > > > >> > >>> + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_CTRL | CAN_CMASK_ARB, > >>> + &priv->regs->if2_cmask); > >>> + > >>> + /* Resetting the ID registers. */ > >>> + pch_can_bit_set(&priv->regs->if2_id2, > >>> + CAN_ID2_DIR | (0x7ff << 2)); > >>> + iowrite32(0x0, &priv->regs->if2_id1); > >>> + > >>> + /* Claring NewDat, TxRqst & IntPnd */ > >>> + pch_can_bit_clear(&priv->regs->if2_mcont, > >>> + CAN_IF_MCONT_NEWDAT | CAN_IF_MCONT_INTPND | > >>> + CAN_IF_MCONT_TXRQXT); > >>> + pch_can_check_if_busy(&priv->regs->if2_creq, mask); > >>> + } > >>> +} > >>> + > >>> +static u32 pch_can_get_buffer_status(struct pch_can_priv *priv) > >>> +{ > >>> + return (ioread32(&priv->regs->treq1) & 0xffff) | > >>> + ((ioread32(&priv->regs->treq2) & 0xffff) << 16); > >> > >> the second 0xffff is not needed, as the return value is u32 and you shift by 16. > > > > I agree. > > > >> > >>> +} > >>> + > >>> +static void pch_can_reset(struct pch_can_priv *priv) > >>> +{ > >>> + /* write to sw reset register */ > >>> + iowrite32(1, &priv->regs->srst); > >>> + iowrite32(0, &priv->regs->srst); > >>> +} > >>> + > >>> +static void pch_can_error(struct net_device *ndev, u32 status) > >>> +{ > >>> + struct sk_buff *skb; > >>> + struct pch_can_priv *priv = netdev_priv(ndev); > >>> + struct can_frame *cf; > >>> + u32 errc; > >>> + struct net_device_stats *stats = &(priv->ndev->stats); > >>> + enum can_state state = priv->can.state; > >>> + > >>> + skb = alloc_can_err_skb(ndev, &cf); > >>> + if (!skb) > >>> + return; > >>> + > >>> + if (status & PCH_BUS_OFF) { > >>> + pch_can_tx_disable_all(priv); > >>> + pch_can_rx_disable_all(priv); > >>> + state = CAN_STATE_BUS_OFF; > >>> + cf->can_id |= CAN_ERR_BUSOFF; > >>> + can_bus_off(ndev); > >>> + } > >>> + > >>> + /* Warning interrupt. */ > >>> + if (status & PCH_EWARN) { > >>> + state = CAN_STATE_ERROR_WARNING; > >>> + priv->can.can_stats.error_warning++; > >>> + cf->can_id |= CAN_ERR_CRTL; > >>> + errc = ioread32(&priv->regs->errc); > >>> + if (((errc & CAN_REC) >> 8) > 96) > >>> + cf->data[1] |= CAN_ERR_CRTL_RX_WARNING; > >>> + if ((errc & CAN_TEC) > 96) > >>> + cf->data[1] |= CAN_ERR_CRTL_TX_WARNING; > >>> + dev_warn(&ndev->dev, > >>> + "%s -> Error Counter is more than 96.\n", __func__); > >> > >> Please use just "debug" level not warning here. Consider to use > >> netdev_dbg() instead. IMHO the __func__ can be dropped and the > >> "official" name for the error is "Error Warning". > > > > I want to know the reason. > > Why is it not dev_warn but netdev_dbg ? > > If you use warning level it would end up on the console or and in the > syslog. It's quite complicated (for programs) to get information from > there. This is why we send CAN error frames. They hold the same > information but int a binary form, thus it's easier to process. I understand the reason. BTW, Why do you say not dev_dbg but netdev_dbg ? > > > > >> > >>> + } > >>> + /* Error passive interrupt. */ > >>> + if (status & PCH_EPASSIV) { > >>> + priv->can.can_stats.error_passive++; > >>> + state = CAN_STATE_ERROR_PASSIVE; > >>> + cf->can_id |= CAN_ERR_CRTL; > >>> + errc = ioread32(&priv->regs->errc); > >>> + if (((errc & CAN_REC) >> 8) > 127) > >>> + cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE; > >>> + if ((errc & CAN_TEC) > 127) > >>> + cf->data[1] |= CAN_ERR_CRTL_TX_PASSIVE; > >>> + dev_err(&ndev->dev, > >>> + "%s -> CAN controller is ERROR PASSIVE .\n", __func__); > >> > >> dito > > > > ditto > > > >> > >>> + } > >>> + > >>> + if (status & PCH_LEC_ALL) { > >>> + priv->can.can_stats.bus_error++; > >>> + stats->rx_errors++; > >>> + switch (status & PCH_LEC_ALL) { > >> > >> I suggest to convert to a if-bit-set because there might be more than > >> one bit set. > > > > I agree. > > > >> > >>> + case PCH_STUF_ERR: > >>> + cf->data[2] |= CAN_ERR_PROT_STUFF; > >>> + break; > >>> + case PCH_FORM_ERR: > >>> + cf->data[2] |= CAN_ERR_PROT_FORM; > >>> + break; > >>> + case PCH_ACK_ERR: > >>> + cf->data[2] |= CAN_ERR_PROT_LOC_ACK | > >>> + CAN_ERR_PROT_LOC_ACK_DEL; > >>> + break; > >>> + case PCH_BIT1_ERR: > >>> + case PCH_BIT0_ERR: > >>> + cf->data[2] |= CAN_ERR_PROT_BIT; > >>> + break; > >>> + case PCH_CRC_ERR: > >>> + cf->data[2] |= CAN_ERR_PROT_LOC_CRC_SEQ | > >>> + CAN_ERR_PROT_LOC_CRC_DEL; > >>> + break; > >>> + default: > >>> + iowrite32(status | PCH_LEC_ALL, &priv->regs->stat); > >>> + break; > >>> + } > >>> + > >>> + } > >>> + > >>> + priv->can.state = state; > >>> + netif_receive_skb(skb); > >>> + > >>> + stats->rx_packets++; > >>> + stats->rx_bytes += cf->can_dlc; > >>> +} > >>> + > >>> +static irqreturn_t pch_can_interrupt(int irq, void *dev_id) > >>> +{ > >>> + struct net_device *ndev = (struct net_device *)dev_id; > >>> + struct pch_can_priv *priv = netdev_priv(ndev); > >>> + > >>> + pch_can_set_int_enables(priv, PCH_CAN_NONE); > >>> + napi_schedule(&priv->napi); > >>> + > >>> + return IRQ_HANDLED; > >>> +} > >>> + > >>> +static void pch_fifo_thresh(struct pch_can_priv *priv, int obj_id) > >>> +{ > >>> + if (obj_id < PCH_FIFO_THRESH) { > >>> + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_CTRL | > >>> + CAN_CMASK_ARB, &priv->regs->if1_cmask); > >>> + > >>> + /* Clearing the Dir bit. */ > >>> + pch_can_bit_clear(&priv->regs->if1_id2, CAN_ID2_DIR); > >>> + > >>> + /* Clearing NewDat & IntPnd */ > >>> + pch_can_bit_clear(&priv->regs->if1_mcont, > >>> + CAN_IF_MCONT_INTPND); > >>> + pch_can_check_if_busy(&priv->regs->if1_creq, obj_id); > >>> + } else if (obj_id > PCH_FIFO_THRESH) { > >>> + pch_can_int_clr(priv, obj_id); > >>> + } else if (obj_id == PCH_FIFO_THRESH) { > >>> + int cnt; > >>> + for (cnt = 0; cnt < PCH_FIFO_THRESH; cnt++) > >>> + pch_can_int_clr(priv, cnt+1); > >>> + } > >>> +} > >>> + > >>> +static int pch_can_rx_msg_lost(struct net_device *ndev, int obj_id) > >>> +{ > >>> + struct pch_can_priv *priv = netdev_priv(ndev); > >>> + struct net_device_stats *stats = &(priv->ndev->stats); > >>> + struct sk_buff *skb; > >>> + struct can_frame *cf; > >>> + > >>> + dev_err(&priv->ndev->dev, "Msg Obj is overwritten.\n"); > >>> + pch_can_bit_clear(&priv->regs->if1_mcont, > >>> + CAN_IF_MCONT_MSGLOST); > >>> + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_CTRL, > >>> + &priv->regs->if1_cmask); > >>> + pch_can_check_if_busy(&priv->regs->if1_creq, obj_id); > >>> + > >>> + skb = alloc_can_err_skb(ndev, &cf); > >>> + if (!skb) > >>> + return -ENOMEM; > >>> + > >>> + priv->can.can_stats.error_passive++; > >>> + priv->can.state = CAN_STATE_ERROR_PASSIVE; > >>> + cf->can_id |= CAN_ERR_CRTL; > >>> + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW; > >>> + stats->rx_over_errors++; > >>> + stats->rx_errors++; > >>> + > >>> + netif_receive_skb(skb); > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static int pch_can_rx_normal(struct net_device *ndev, u32 obj_num, int quota) > >>> +{ > >>> + u32 reg; > >>> + canid_t id; > >>> + u32 ide; > >>> + u32 rtr; > >>> + int rcv_pkts = 0; > >>> + int rtn; > >>> + int next_flag = 0; > >>> + struct sk_buff *skb; > >>> + struct can_frame *cf; > >>> + struct pch_can_priv *priv = netdev_priv(ndev); > >>> + struct net_device_stats *stats = &(priv->ndev->stats); > >>> + > >>> + /* Reading the messsage object from the Message RAM */ > >>> + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if1_cmask); > >>> + pch_can_check_if_busy(&priv->regs->if1_creq, obj_num); > >>> + > >>> + /* Reading the MCONT register. */ > >>> + reg = ioread32(&priv->regs->if1_mcont); > >>> + reg &= 0xffff; > >>> + > >>> + for (; (!(reg & CAN_IF_MCONT_EOB)) && (quota > 0); > >>> + obj_num++, next_flag = 0) { > >>> + /* If MsgLost bit set. */ > >>> + if (reg & CAN_IF_MCONT_MSGLOST) { > >>> + rtn = pch_can_rx_msg_lost(ndev, obj_num); > >>> + if (!rtn) > >>> + return rtn; > >>> + rcv_pkts++; > >>> + quota--; > >>> + next_flag = 1; > >>> + } else if (!(reg & CAN_IF_MCONT_NEWDAT)) > >>> + next_flag = 1; > >>> + > >> > >> after rearanging the code (see below..) you should be able to use a continue here. > >> > >>> + if (!next_flag) { > >>> + skb = alloc_can_skb(priv->ndev, &cf); > >>> + if (!skb) > >>> + return -ENOMEM; > >>> + > >>> + /* Get Received data */ > >>> + ide = ((ioread32(&priv->regs->if1_id2)) & CAN_ID2_XTD); > >>> + if (ide) { > >>> + id = (ioread32(&priv->regs->if1_id1) & 0xffff); > >>> + id |= (((ioread32(&priv->regs->if1_id2)) & > >>> + 0x1fff) << 16); > >>> + cf->can_id = (id & CAN_EFF_MASK) | CAN_EFF_FLAG; > >> ^^^^^^^^^^^^^^^^^ > >> > >> is the mask needed, you mask the if1_id{1,2} already > > > > I will delete > > > >> > >>> + } else { > >>> + id = (((ioread32(&priv->regs->if1_id2)) & > >>> + (CAN_SFF_MASK << 2)) >> 2); > >>> + cf->can_id = (id & CAN_SFF_MASK); > >> > >> one mask can go away > > > > I agree. > > > >> > >>> + } > >>> + > >>> + rtr = ioread32(&priv->regs->if1_id2) & CAN_ID2_DIR; > >> ^^ > >> > >> remove one space > > > > I agree. > > > >> > >>> + > >>> + if (rtr) > >>> + cf->can_id |= CAN_RTR_FLAG; > >>> + > >>> + cf->can_dlc = get_can_dlc((ioread32(&priv->regs-> > >>> + if1_mcont)) & 0xF); > >>> + *(u16 *)(cf->data + 0) = ioread16(&priv->regs-> > >>> + if1_dataa1); > >>> + *(u16 *)(cf->data + 2) = ioread16(&priv->regs-> > >>> + if1_dataa2); > >>> + *(u16 *)(cf->data + 4) = ioread16(&priv->regs-> > >>> + if1_datab1); > >>> + *(u16 *)(cf->data + 6) = ioread16(&priv->regs-> > >>> + if1_datab2); > >> > >> are you sure, the bytes in the can package a in the correct order. > >> Please test your pch_can against a non pch_can system. > > > > Unfortunately, we don't have non pch_can system. > > Have a look a the driver/net/can/usb subdir and buy one of those. It > really hard to find bugs if you test against your own driver. We can't buy this right now for few budget. But I heard we have "CANalyzer". Using this, we can test this endian concern. But we may need much time for studying how to use. > > > > >> > >>> + > >>> + netif_receive_skb(skb); > >>> + rcv_pkts++; > >>> + stats->rx_packets++; > >>> + quota--; > >>> + stats->rx_bytes += cf->can_dlc; > >>> + > >>> + pch_fifo_thresh(priv, obj_num); > >>> + } > >>> + > >>> + /* Reading the messsage object from the Message RAM */ > >>> + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if1_cmask); > >>> + pch_can_check_if_busy(&priv->regs->if1_creq, obj_num + 1); > >>> + reg = ioread32(&priv->regs->if1_mcont); > >> > >> this is almost the same code as before the the loop, can you rearange > >> the code to avoid duplication? > > > > I agree. > > > >> > >>> + } > >>> + > >>> + return rcv_pkts; > >>> +} > >>> + > >>> +static void pch_can_tx_complete(struct net_device *ndev, u32 int_stat) > >>> +{ > >>> + struct pch_can_priv *priv = netdev_priv(ndev); > >>> + struct net_device_stats *stats = &(priv->ndev->stats); > >>> + unsigned long flags; > >>> + u32 dlc; > >>> + > >>> + can_get_echo_skb(ndev, int_stat - PCH_RX_OBJ_NUM - 1); > >>> + spin_lock_irqsave(&priv->msgif_reg_lock, flags); > >>> + iowrite32(CAN_CMASK_RX_TX_GET | CAN_CMASK_CLRINTPND, > >>> + &priv->regs->if2_cmask); > >>> + dlc = ioread32(&priv->regs->if2_mcont) & CAN_IF_MCONT_DLC; > >>> + pch_can_check_if_busy(&priv->regs->if2_creq, int_stat); > >>> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags); > >>> + if (dlc > 8) > >>> + dlc = 8; > >> > >> use get_can_dlc > > > > I agree. > > > >> > >>> + stats->tx_bytes += dlc; > >>> + stats->tx_packets++; > >>> +} > >>> + > >>> +static int pch_can_rx_poll(struct napi_struct *napi, int quota) > >>> +{ > >>> + struct net_device *ndev = napi->dev; > >>> + struct pch_can_priv *priv = netdev_priv(ndev); > >>> + u32 int_stat; > >>> + int rcv_pkts = 0; > >>> + u32 reg_stat; > >>> + unsigned long flags; > >>> + > >>> + int_stat = pch_can_int_pending(priv); > >>> + if (!int_stat) > >>> + goto END; > >>> + > >>> + if ((int_stat == CAN_STATUS_INT) && (quota > 0)) { > >>> + reg_stat = ioread32(&priv->regs->stat); > >>> + if (reg_stat & (PCH_BUS_OFF | PCH_LEC_ALL)) { > >>> + if ((reg_stat & PCH_LEC_ALL) != PCH_LEC_ALL) { > >>> + pch_can_error(ndev, reg_stat); > >>> + quota--; > >>> + } > >>> + } > >>> + > >>> + if (reg_stat & PCH_TX_OK) { > >>> + spin_lock_irqsave(&priv->msgif_reg_lock, flags); > >>> + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if2_cmask); > >>> + pch_can_check_if_busy(&priv->regs->if2_creq, > >>> + ioread32(&priv->regs->intr)); > >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^ > >> > >> Isn't this "int_stat". Might it be possilbe that regs->intr changes > >> between the pch_can_int_pending and here? > > > > This code was mistake. > > This condition, message object is not acccessed. > > Thus, pch_can_check_if_busy can be deleted. > > > >> > >> What should this transfer do? > >> > >>> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags); > >>> + pch_can_bit_clear(&priv->regs->stat, PCH_TX_OK); > >>> + } > >>> + > >>> + if (reg_stat & PCH_RX_OK) > >>> + pch_can_bit_clear(&priv->regs->stat, PCH_RX_OK); > >>> + > >>> + int_stat = pch_can_int_pending(priv); > >>> + } > >>> + > >>> + if (quota == 0) > >>> + goto END; > >>> + > >>> + if ((int_stat >= 1) && (int_stat <= PCH_RX_OBJ_NUM)) { > >>> + spin_lock_irqsave(&priv->msgif_reg_lock, flags); > >>> + rcv_pkts += pch_can_rx_normal(ndev, int_stat, quota); > >>> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags); > >>> + quota -= rcv_pkts; > >>> + if (rcv_pkts < 0) > >> > >> how can this happen? > > > > My mistake. > > if (quota < 0) is TRUE. > > > > > >> > >>> + goto END; > >>> + } else if ((int_stat > PCH_RX_OBJ_NUM) && (int_stat <= PCH_OBJ_NUM)) { > >>> + /* Handle transmission interrupt */ > >>> + pch_can_tx_complete(ndev, int_stat); > >>> + } > >>> + > >>> +END: > >>> + napi_complete(napi); > >>> + pch_can_set_int_enables(priv, PCH_CAN_ALL); > >>> + > >>> + return rcv_pkts; > >>> +} > >>> + > >>> +static int pch_set_bittiming(struct net_device *ndev) > >>> +{ > >>> + struct pch_can_priv *priv = netdev_priv(ndev); > >>> + const struct can_bittiming *bt = &priv->can.bittiming; > >>> + u32 canbit; > >>> + u32 bepe; > >>> + > >>> + /* Setting the CCE bit for accessing the Can Timing register. */ > >>> + pch_can_bit_set(&priv->regs->cont, CAN_CTRL_CCE); > >>> + > >>> + canbit = (bt->brp - 1) & MSK_BITT_BRP; > >>> + canbit |= (bt->sjw - 1) << BIT_BITT_SJW; > >>> + canbit |= (bt->phase_seg1 + bt->prop_seg - 1) << BIT_BITT_TSEG1; > >>> + canbit |= (bt->phase_seg2 - 1) << BIT_BITT_TSEG2; > >>> + bepe = ((bt->brp - 1) & MSK_BRPE_BRPE) >> BIT_BRPE_BRPE; > >>> + iowrite32(canbit, &priv->regs->bitt); > >>> + iowrite32(bepe, &priv->regs->brpe); > >>> + pch_can_bit_clear(&priv->regs->cont, CAN_CTRL_CCE); > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static void pch_can_start(struct net_device *ndev) > >>> +{ > >>> + struct pch_can_priv *priv = netdev_priv(ndev); > >>> + > >>> + if (priv->can.state != CAN_STATE_STOPPED) > >>> + pch_can_reset(priv); > >>> + > >>> + pch_set_bittiming(ndev); > >>> + pch_can_set_optmode(priv); > >>> + > >>> + pch_can_tx_enable_all(priv); > >>> + pch_can_rx_enable_all(priv); > >>> + > >>> + /* Setting the CAN to run mode. */ > >>> + pch_can_set_run_mode(priv, PCH_CAN_RUN); > >>> + > >>> + priv->can.state = CAN_STATE_ERROR_ACTIVE; > >>> + > >>> + return; > >>> +} > >>> + > >>> +static int pch_can_do_set_mode(struct net_device *ndev, enum can_mode mode) > >>> +{ > >>> + int ret = 0; > >>> + > >>> + switch (mode) { > >>> + case CAN_MODE_START: > >>> + pch_can_start(ndev); > >>> + netif_wake_queue(ndev); > >>> + break; > >>> + default: > >>> + ret = -EOPNOTSUPP; > >>> + break; > >>> + } > >>> + > >>> + return ret; > >>> +} > >>> + > >>> +static int pch_can_open(struct net_device *ndev) > >>> +{ > >>> + struct pch_can_priv *priv = netdev_priv(ndev); > >>> + int retval; > >>> + > >>> + /* Regsitering the interrupt. */ > >>> + retval = request_irq(priv->dev->irq, pch_can_interrupt, IRQF_SHARED, > >>> + ndev->name, ndev); > >>> + if (retval) { > >>> + dev_err(&ndev->dev, "request_irq failed.\n"); > >>> + goto req_irq_err; > >>> + } > >>> + > >>> + /* Open common can device */ > >>> + retval = open_candev(ndev); > >>> + if (retval) { > >>> + dev_err(ndev->dev.parent, "open_candev() failed %d\n", retval); > >>> + goto err_open_candev; > >>> + } > >>> + > >>> + pch_can_init(priv); > >>> + pch_can_start(ndev); > >>> + napi_enable(&priv->napi); > >>> + netif_start_queue(ndev); > >>> + > >>> + return 0; > >>> + > >>> +err_open_candev: > >>> + free_irq(priv->dev->irq, ndev); > >>> +req_irq_err: > >>> + pch_can_release(priv); > >>> + > >>> + return retval; > >>> +} > >>> + > >>> +static int pch_close(struct net_device *ndev) > >>> +{ > >>> + struct pch_can_priv *priv = netdev_priv(ndev); > >>> + > >>> + netif_stop_queue(ndev); > >>> + napi_disable(&priv->napi); > >>> + pch_can_release(priv); > >>> + free_irq(priv->dev->irq, ndev); > >>> + close_candev(ndev); > >>> + priv->can.state = CAN_STATE_STOPPED; > >>> + return 0; > >>> +} > >>> + > >>> +static netdev_tx_t pch_xmit(struct sk_buff *skb, struct net_device *ndev) > >>> +{ > >>> + unsigned long flags; > >>> + struct pch_can_priv *priv = netdev_priv(ndev); > >>> + struct can_frame *cf = (struct can_frame *)skb->data; > >>> + int tx_buffer_avail = 0; > >> > >> What I'm totally missing is the TX flow controll. Your driver has to > >> ensure that the package leave the controller in the order that come > >> into the xmit function. Further you have to stop your xmit queue if > >> you're out of tx objects and reenable if you have a object free. > >> > >> Use netif_stop_queue() and netif_wake_queue() for this. > > > > In this code, I think "out of tx objects" cannot be occurred. > > It's not a matter of code it's the hardware. You cannot put more than a > certain number of CAN frames into the hardware. If you have a CAN bus at > a certain speed, you can only send a certain number of CAN frames in a > second. So you cannot push more than this amount of frames/s into the > hardware. > > > Nevertheless, are netif_stop_queue() and netif_wake_queue() is necessary ? > > Yes. I can' understand your issue. Please can you hear my opinion? Please see the head of pch_xmit. > > + if (priv->tx_obj == (PCH_OBJ_NUM + 1)) { /* Point tail Obj + 1 */ > > + while (ioread32(&priv->regs->treq2) & 0xfc00) > > + udelay(1); When points tail of Tx message object, this driver waits until completion of all tx messaeg objects. Thus, application/driver ought not to be able to put Tx object exceed the number of tx message object. Thus I think these code(netif_stop_queue/netif_wake_queue) are completely redundant. > > > > >> > >>> + > >>> + if (can_dropped_invalid_skb(ndev, skb)) > >>> + return NETDEV_TX_OK; > >>> + > >>> + if (priv->tx_obj == (PCH_OBJ_NUM + 1)) { /* Point tail Obj + 1 */ > >>> + while (ioread32(&priv->regs->treq2) & 0xfc00) > >>> + udelay(1); > >> > >> please no (possible) infinite delays! > > > > I will add break processing. > > > >> > >>> + priv->tx_obj = PCH_RX_OBJ_NUM + 1; /* Point head of Tx Obj ID */ > >>> + } > >> > >>> + > >>> + tx_buffer_avail = priv->tx_obj; > >> > >> why has the "object" become a "buffer" now? :) > > > > You are right. > > I will modify the name. > > > >> > >>> + priv->tx_obj++; > >>> + > >>> + /* Attaining the lock. */ > >>> + spin_lock_irqsave(&priv->msgif_reg_lock, flags); > >>> + > >>> + /* Setting the CMASK register to set value*/ > >> ^^^ > >> > >> pleas add a whitespace > > > > I agree. > > > >> > >>> + iowrite32(CAN_CMASK_RX_TX_SET, &priv->regs->if2_cmask); > >>> + > >>> + /* If ID extended is set. */ > >>> + if (cf->can_id & CAN_EFF_FLAG) { > >>> + iowrite32(cf->can_id & 0xffff, &priv->regs->if2_id1); > >>> + iowrite32(((cf->can_id >> 16) & 0x1fff) | CAN_ID2_XTD, > >>> + &priv->regs->if2_id2); > >>> + } else { > >>> + iowrite32(0, &priv->regs->if2_id1); > >>> + iowrite32((cf->can_id & CAN_SFF_MASK) << 2, > >>> + &priv->regs->if2_id2); > >>> + } > >>> + > >>> + pch_can_bit_set(&priv->regs->if2_id2, CAN_ID_MSGVAL); > >> > >> Do you need to do a read-modify-write of the hardware register? Please > >> prepare the values you want to write to hardware, then do it. > > > > Current design policy for read/write message object, > > the driver is designed with Read-Modify-Write. > > > > I will modify to Write only for reducing accessing Message RAM. > > > >> > >>> + > >>> + /* If remote frame has to be transmitted.. */ > >>> + if (!(cf->can_id & CAN_RTR_FLAG)) > >>> + pch_can_bit_set(&priv->regs->if2_id2, CAN_ID2_DIR); > >> dito > >>> + /* If remote frame has to be transmitted.. */ > >>> + if (cf->can_id & CAN_RTR_FLAG) > >>> + pch_can_bit_clear(&priv->regs->if2_id2, CAN_ID2_DIR); > >> dito > >>> + > >>> + /* Copy data to register */ > >>> + if (cf->can_dlc > 0) { > >>> + u32 data1 = *((u16 *)&cf->data[0]); > >>> + iowrite32(data1, &priv->regs->if2_dataa1); > >> > >> do you think you send the bytes in correct order? > > > > Let me study this endianess. > > > >> > >>> + } > >>> + if (cf->can_dlc > 2) { > >>> + u32 data1 = *((u16 *)&cf->data[2]); > >>> + iowrite32(data1, &priv->regs->if2_dataa2); > >>> + } > >>> + if (cf->can_dlc > 4) { > >>> + u32 data1 = *((u16 *)&cf->data[4]); > >>> + iowrite32(data1, &priv->regs->if2_datab1); > >>> + } > >>> + if (cf->can_dlc > 6) { > >>> + u32 data1 = *((u16 *)&cf->data[6]); > >>> + iowrite32(data1, &priv->regs->if2_datab2); > >>> + } > >>> + > >>> + can_put_echo_skb(skb, ndev, tx_buffer_avail - PCH_RX_OBJ_NUM - 1); > >>> + > >>> + /* Set the size of the data. */ > >>> + iowrite32(cf->can_dlc, &priv->regs->if2_mcont); > >>> + > >>> + /* Update if2_mcont */ > >>> + pch_can_bit_set(&priv->regs->if2_mcont, > >>> + CAN_IF_MCONT_NEWDAT | CAN_IF_MCONT_TXRQXT | > >>> + CAN_IF_MCONT_TXIE); > >> > >> pleae first perpare your value, then write to hardware. > > > > ditto. > > > >> > >>> + > >>> + if (tx_buffer_avail == PCH_RX_OBJ_NUM) /* If points tail of FIFO */ > >>> + pch_can_bit_set(&priv->regs->if2_mcont, CAN_IF_MCONT_EOB); > >> > >> dito > >> > >> Is EOB relevant for TX objects? > > > > This is mistake. No meaning for tx. > > I will modify. > > > >> > >>> + pch_can_check_if_busy(&priv->regs->if2_creq, tx_buffer_avail); > >>> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags); > >>> + > >>> + return NETDEV_TX_OK; > >>> +} > >>> + > >>> +static const struct net_device_ops pch_can_netdev_ops = { > >>> + .ndo_open = pch_can_open, > >>> + .ndo_stop = pch_close, > >>> + .ndo_start_xmit = pch_xmit, > >>> +}; > >>> + > >>> +static void __devexit pch_can_remove(struct pci_dev *pdev) > >>> +{ > >>> + struct net_device *ndev = pci_get_drvdata(pdev); > >>> + struct pch_can_priv *priv = netdev_priv(ndev); > >>> + > >>> + unregister_candev(priv->ndev); > >>> + pci_iounmap(pdev, priv->regs); > >>> + if (priv->use_msi) > >>> + pci_disable_msi(priv->dev); > >>> + pci_release_regions(pdev); > >>> + pci_disable_device(pdev); > >>> + pci_set_drvdata(pdev, NULL); > >>> + free_candev(priv->ndev); > >>> +} > >>> + > >>> +#ifdef CONFIG_PM > >>> +static void pch_can_set_int_custom(struct pch_can_priv *priv) > >>> +{ > >>> + /* Clearing the IE, SIE and EIE bits of Can control register. */ > >>> + pch_can_bit_clear(&priv->regs->cont, CAN_CTRL_IE_SIE_EIE); > >>> + > >>> + /* Appropriately setting them. */ > >>> + pch_can_bit_set(&priv->regs->cont, > >>> + ((priv->int_enables & MSK_CTRL_IE_SIE_EIE) << 1)); > >>> +} > >>> + > >>> +/* This function retrieves interrupt enabled for the CAN device. */ > >>> +static u32 pch_can_get_int_enables(struct pch_can_priv *priv) > >>> +{ > >>> + /* Obtaining the status of IE, SIE and EIE interrupt bits. */ > >>> + return (ioread32(&priv->regs->cont) & CAN_CTRL_IE_SIE_EIE) >> 1; > >>> +} > >>> + > >>> +static u32 pch_can_get_rx_enable(struct pch_can_priv *priv, u32 buff_num) > >>> +{ > >>> + unsigned long flags; > >>> + u32 enable; > >>> + > >>> + spin_lock_irqsave(&priv->msgif_reg_lock, flags); > >>> + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if1_cmask); > >>> + pch_can_check_if_busy(&priv->regs->if1_creq, buff_num); > >>> + > >>> + if (((ioread32(&priv->regs->if1_id2)) & CAN_ID_MSGVAL) && > >>> + ((ioread32(&priv->regs->if1_mcont)) & > >>> + CAN_IF_MCONT_RXIE)) > >>> + enable = 1; > >>> + else > >>> + enable = 0; > >>> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags); > >>> + return enable; > >>> +} > >>> + > >>> +static u32 pch_can_get_tx_enable(struct pch_can_priv *priv, u32 buff_num) > >>> +{ > >>> + unsigned long flags; > >>> + u32 enable; > >>> + > >>> + spin_lock_irqsave(&priv->msgif_reg_lock, flags); > >>> + > >>> + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if2_cmask); > >>> + pch_can_check_if_busy(&priv->regs->if2_creq, buff_num); > >>> + if (((ioread32(&priv->regs->if2_id2)) & CAN_ID_MSGVAL) && > >>> + ((ioread32(&priv->regs->if2_mcont)) & > >>> + CAN_IF_MCONT_TXIE)) { > >>> + enable = 1; > >>> + } else { > >>> + enable = 0; > >>> + } > >>> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags); > >>> + > >>> + return enable; > >>> +} > >>> + > >>> +static void pch_can_set_rx_buffer_link(struct pch_can_priv *priv, > >>> + u32 buffer_num, u32 set) > >>> +{ > >>> + unsigned long flags; > >>> + > >>> + spin_lock_irqsave(&priv->msgif_reg_lock, flags); > >>> + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if1_cmask); > >>> + pch_can_check_if_busy(&priv->regs->if1_creq, buffer_num); > >>> + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_CTRL, &priv->regs->if1_cmask); > >>> + if (set == 1) > >>> + pch_can_bit_clear(&priv->regs->if1_mcont, CAN_IF_MCONT_EOB); > >>> + else > >>> + pch_can_bit_set(&priv->regs->if1_mcont, CAN_IF_MCONT_EOB); > >>> + > >>> + pch_can_check_if_busy(&priv->regs->if1_creq, buffer_num); > >>> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags); > >>> +} > >>> + > >>> +static u32 pch_can_get_rx_buffer_link(struct pch_can_priv *priv, u32 buffer_num) > >>> +{ > >>> + unsigned long flags; > >>> + u32 link; > >>> + > >>> + spin_lock_irqsave(&priv->msgif_reg_lock, flags); > >>> + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if1_cmask); > >>> + pch_can_check_if_busy(&priv->regs->if1_creq, buffer_num); > >>> + > >>> + if (ioread32(&priv->regs->if1_mcont) & CAN_IF_MCONT_EOB) > >>> + link = 0; > >>> + else > >>> + link = 1; > >>> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags); > >>> + return link; > >>> +} > >>> + > >>> +static int pch_can_suspend(struct pci_dev *pdev, pm_message_t state) > >>> +{ > >>> + int i; > >>> + int retval; > >>> + u32 buf_stat; /* Variable for reading the transmit buffer status. */ > >>> + u32 counter = COUNTER_LIMIT; > >>> + > >>> + struct net_device *dev = pci_get_drvdata(pdev); > >>> + struct pch_can_priv *priv = netdev_priv(dev); > >>> + > >>> + /* Stop the CAN controller */ > >>> + pch_can_set_run_mode(priv, PCH_CAN_STOP); > >>> + > >>> + /* Indicate that we are aboutto/in suspend */ > >>> + priv->can.state = CAN_STATE_STOPPED; > >>> + > >>> + /* Waiting for all transmission to complete. */ > >>> + while (counter) { > >>> + buf_stat = pch_can_get_buffer_status(priv); > >>> + if (!buf_stat) > >>> + break; > >>> + counter--; > >>> + udelay(1); > >>> + } > >>> + if (!counter) > >>> + dev_err(&pdev->dev, "%s -> Transmission time out.\n", __func__); > >>> + > >>> + /* Save interrupt configuration and then disable them */ > >>> + priv->int_enables = pch_can_get_int_enables(priv); > >>> + pch_can_set_int_enables(priv, PCH_CAN_DISABLE); > >>> + > >>> + /* Save Tx buffer enable state */ > >>> + for (i = PCH_RX_OBJ_NUM + 1; i <= PCH_OBJ_NUM; i++) > >>> + priv->tx_enable[i] = pch_can_get_tx_enable(priv, i); > >>> + > >>> + /* Disable all Transmit buffers */ > >>> + pch_can_tx_disable_all(priv); > >>> + > >>> + /* Save Rx buffer enable state */ > >>> + for (i = 1; i <= PCH_RX_OBJ_NUM; i++) { > >>> + priv->rx_enable[i] = pch_can_get_rx_enable(priv, i); > >>> + priv->rx_link[i] = pch_can_get_rx_buffer_link(priv, i); > >>> + } > >>> + > >>> + /* Disable all Receive buffers */ > >>> + pch_can_rx_disable_all(priv); > >>> + retval = pci_save_state(pdev); > >>> + if (retval) { > >>> + dev_err(&pdev->dev, "pci_save_state failed.\n"); > >>> + } else { > >>> + pci_enable_wake(pdev, PCI_D3hot, 0); > >>> + pci_disable_device(pdev); > >>> + pci_set_power_state(pdev, pci_choose_state(pdev, state)); > >>> + } > >>> + > >>> + return retval; > >>> +} > >>> + > >>> +static int pch_can_resume(struct pci_dev *pdev) > >>> +{ > >>> + int i; > >>> + int retval; > >>> + struct net_device *dev = pci_get_drvdata(pdev); > >>> + struct pch_can_priv *priv = netdev_priv(dev); > >>> + > >>> + pci_set_power_state(pdev, PCI_D0); > >>> + pci_restore_state(pdev); > >>> + retval = pci_enable_device(pdev); > >>> + if (retval) { > >>> + dev_err(&pdev->dev, "pci_enable_device failed.\n"); > >>> + return retval; > >>> + } > >>> + > >>> + pci_enable_wake(pdev, PCI_D3hot, 0); > >>> + > >>> + priv->can.state = CAN_STATE_ERROR_ACTIVE; > >>> + > >>> + /* Disabling all interrupts. */ > >>> + pch_can_set_int_enables(priv, PCH_CAN_DISABLE); > >>> + > >>> + /* Setting the CAN device in Stop Mode. */ > >>> + pch_can_set_run_mode(priv, PCH_CAN_STOP); > >>> + > >>> + /* Configuring the transmit and receive buffers. */ > >>> + pch_can_config_rx_tx_buffers(priv); > >>> + > >>> + /* Restore the CAN state */ > >>> + pch_set_bittiming(dev); > >>> + > >>> + /* Listen/Active */ > >>> + pch_can_set_optmode(priv); > >>> + > >>> + /* Enabling the transmit buffer. */ > >>> + for (i = 1; i <= PCH_RX_OBJ_NUM; i++) > >>> + pch_can_set_tx_enable(priv, i, priv->tx_enable[i]); > >>> + > >>> + /* Configuring the receive buffer and enabling them. */ > >>> + for (i = PCH_RX_OBJ_NUM + 1; i <= PCH_OBJ_NUM; i++) { > >>> + /* Restore buffer link */ > >>> + pch_can_set_rx_buffer_link(priv, i, priv->rx_link[i]); > >>> + > >>> + /* Restore buffer enables */ > >>> + pch_can_set_rx_enable(priv, i, priv->rx_enable[i]); > >>> + } > >>> + > >>> + /* Enable CAN Interrupts */ > >>> + pch_can_set_int_custom(priv); > >>> + > >>> + /* Restore Run Mode */ > >>> + pch_can_set_run_mode(priv, PCH_CAN_RUN); > >>> + > >>> + return retval; > >>> +} > >>> +#else > >>> +#define pch_can_suspend NULL > >>> +#define pch_can_resume NULL > >>> +#endif > >>> + > >>> +static int pch_can_get_berr_counter(const struct net_device *dev, > >>> + struct can_berr_counter *bec) > >>> +{ > >>> + struct pch_can_priv *priv = netdev_priv(dev); > >>> + > >>> + bec->txerr = ioread32(&priv->regs->errc) & CAN_TEC; > >>> + bec->rxerr = (ioread32(&priv->regs->errc) & CAN_REC) >> 8; > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static int __devinit pch_can_probe(struct pci_dev *pdev, > >>> + const struct pci_device_id *id) > >>> +{ > >>> + struct net_device *ndev; > >>> + struct pch_can_priv *priv; > >>> + int rc; > >>> + void __iomem *addr; > >>> + > >>> + rc = pci_enable_device(pdev); > >>> + if (rc) { > >>> + dev_err(&pdev->dev, "Failed pci_enable_device %d\n", rc); > >>> + goto probe_exit_endev; > >>> + } > >>> + > >>> + rc = pci_request_regions(pdev, KBUILD_MODNAME); > >>> + if (rc) { > >>> + dev_err(&pdev->dev, "Failed pci_request_regions %d\n", rc); > >>> + goto probe_exit_pcireq; > >>> + } > >>> + > >>> + addr = pci_iomap(pdev, 1, 0); > >>> + if (!addr) { > >>> + rc = -EIO; > >>> + dev_err(&pdev->dev, "Failed pci_iomap\n"); > >>> + goto probe_exit_ipmap; > >>> + } > >>> + > >>> + ndev = alloc_candev(sizeof(struct pch_can_priv), PCH_TX_OBJ_NUM); > >>> + if (!ndev) { > >>> + rc = -ENOMEM; > >>> + dev_err(&pdev->dev, "Failed alloc_candev\n"); > >>> + goto probe_exit_alloc_candev; > >>> + } > >>> + > >>> + priv = netdev_priv(ndev); > >>> + priv->ndev = ndev; > >>> + priv->regs = addr; > >>> + priv->dev = pdev; > >>> + priv->can.bittiming_const = &pch_can_bittiming_const; > >>> + priv->can.do_set_mode = pch_can_do_set_mode; > >>> + priv->can.do_get_berr_counter = pch_can_get_berr_counter; > >>> + priv->can.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY | > >>> + CAN_CTRLMODE_LOOPBACK; > >>> + priv->tx_obj = PCH_RX_OBJ_NUM + 1; /* Point head of Tx Obj */ > >>> + > >>> + ndev->irq = pdev->irq; > >>> + ndev->flags |= IFF_ECHO; > >>> + > >>> + pci_set_drvdata(pdev, ndev); > >>> + SET_NETDEV_DEV(ndev, &pdev->dev); > >>> + ndev->netdev_ops = &pch_can_netdev_ops; > >>> + priv->can.clock.freq = PCH_CAN_CLK; /* Hz */ > >>> + > >>> + netif_napi_add(ndev, &priv->napi, pch_can_rx_poll, PCH_RX_OBJ_NUM); > >>> + > >>> + rc = pci_enable_msi(priv->dev); > >>> + if (rc) { > >>> + dev_info(&ndev->dev, "PCH CAN opened without MSI\n"); > >>> + priv->use_msi = 0; > >>> + } else { > >>> + dev_info(&ndev->dev, "PCH CAN opened with MSI\n"); > >>> + priv->use_msi = 1; > >>> + } > >>> + > >>> + rc = register_candev(ndev); > >>> + if (rc) { > >>> + dev_err(&pdev->dev, "Failed register_candev %d\n", rc); > >>> + goto probe_exit_reg_candev; > >>> + } > >>> + > >>> + return 0; > >>> + > >>> +probe_exit_reg_candev: > >>> + free_candev(ndev); > >>> +probe_exit_alloc_candev: > >>> + pci_iounmap(pdev, addr); > >>> +probe_exit_ipmap: > >>> + pci_release_regions(pdev); > >>> +probe_exit_pcireq: > >>> + pci_disable_device(pdev); > >>> +probe_exit_endev: > >>> + return rc; > >>> +} > >>> + > >>> +static struct pci_driver pch_can_pcidev = { > >>> + .name = "pch_can", > >>> + .id_table = pch_pci_tbl, > >>> + .probe = pch_can_probe, > >>> + .remove = __devexit_p(pch_can_remove), > >>> + .suspend = pch_can_suspend, > >>> + .resume = pch_can_resume, > >>> +}; > >>> + > >>> +static int __init pch_can_pci_init(void) > >>> +{ > >>> + return pci_register_driver(&pch_can_pcidev); > >>> +} > >>> +module_init(pch_can_pci_init); > >>> + > >>> +static void __exit pch_can_pci_exit(void) > >>> +{ > >>> + pci_unregister_driver(&pch_can_pcidev); > >>> +} > >>> +module_exit(pch_can_pci_exit); > >>> + > >>> +MODULE_DESCRIPTION("Intel EG20T PCH CAN(Controller Area Network) Driver"); > >>> +MODULE_LICENSE("GPL v2"); > >>> +MODULE_VERSION("0.94"); > >> > >> cheers, 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 | > >> > >> > > > > Thanks, Tomoya(OKI SEMICONDUCTOR CO., LTD.) > > cheers, 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 | > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/