Hi! > From: Martin Jerabek <martin.jerabek01@xxxxxxxxx> > > This driver adds support for the CTU CAN FD open-source IP core. > More documentation and core sources at project page > (https://gitlab.fel.cvut.cz/canbus/ctucanfd_ip_core). > The core integration to Xilinx Zynq system as platform driver > is available (https://gitlab.fel.cvut.cz/canbus/zynq/zynq-can-sja1000-top). > Implementation on Intel FGA based PCI Express board is available > from project (https://gitlab.fel.cvut.cz/canbus/pcie-ctu_can_fd). Is "FGA" a typo? Yes, it is. Anyway, following link tells me: Project 'canbus/pcie-ctu_can_fd' was moved to 'canbus/pcie-ctucanfd'. Please update any links and bookmarks that may still have the old path. Fixing it in Kconfig is more important. > +++ b/drivers/net/can/ctucanfd/Kconfig > @@ -0,0 +1,15 @@ > +if CAN_CTUCANFD > + > +endif Empty -> drop? > +++ b/drivers/net/can/ctucanfd/Makefile > @@ -0,0 +1,7 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +++ b/drivers/net/can/ctucanfd/ctu_can_fd.c > @@ -0,0 +1,1105 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later Having Makefile and sources with different licenses is rather unusual. > +static const char * const ctucan_state_strings[] = { > + "CAN_STATE_ERROR_ACTIVE", > + "CAN_STATE_ERROR_WARNING", > + "CAN_STATE_ERROR_PASSIVE", > + "CAN_STATE_BUS_OFF", > + "CAN_STATE_STOPPED", > + "CAN_STATE_SLEEPING" > +}; Put this near function that uses this? > +/** > + * ctucan_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 > + */ > +/** > + * ctucan_chip_start - This routine starts the driver > + * @ndev: Pointer to net_device structure > + * > + * This is the drivers start routine. > + * > + * Return: 0 on success and failure value on error > + */ Good documentation is nice, but repeating "This routine starts the driver" in "This is the drivers start routine." is not too helpful. > +/** > + * ctucan_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 txbuf and populates their fields to > + * start the transmission. > + * > + * Return: %NETDEV_TX_OK on success and failure value on error > + */ Based on other documentation, I'd expect this to return -ESOMETHING on error, but it returns NETDEV_TX_BUSY. > + /* Check if the TX buffer is full */ > + if (unlikely(!CTU_CAN_FD_TXTNF(ctu_can_get_status(&priv->p)))) { > + netif_stop_queue(ndev); > + netdev_err(ndev, "BUG!, no TXB free when queue awake!\n"); > + return NETDEV_TX_BUSY; > + } You call stop_queue() without spinlock... > + spin_lock_irqsave(&priv->tx_lock, flags); > + > + ctucan_hw_txt_set_rdy(&priv->p, txb_id); > + > + priv->txb_head++; > + > + /* Check if all TX buffers are full */ > + if (!CTU_CAN_FD_TXTNF(ctu_can_get_status(&priv->p))) > + netif_stop_queue(ndev); > + > + spin_unlock_irqrestore(&priv->tx_lock, flags); ...and then with spinlock held. One of them is buggy. > +/** > + * 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: 1 on success and 0 on failure. > + */ Adapt to usual 0 / -EFOO? > + /* Check for Arbitration Lost interrupt */ > + if (isr.s.ali) { > + if (dologerr) > + netdev_info(ndev, " arbitration lost"); > + priv->can.can_stats.arbitration_lost++; > + if (skb) { > + cf->can_id |= CAN_ERR_LOSTARB; > + cf->data[0] = CAN_ERR_LOSTARB_UNSPEC; > + } > + } > + > + /* Check for Bus Error interrupt */ > + if (isr.s.bei) { > + netdev_info(ndev, " bus error"); Missing "if (dologerr)" here? > +static int ctucan_rx_poll(struct napi_struct *napi, int quota) > +{ > + struct net_device *ndev = napi->dev; > + struct ctucan_priv *priv = netdev_priv(ndev); > + int work_done = 0; > + union ctu_can_fd_status status; > + u32 framecnt; > + > + framecnt = ctucan_hw_get_rx_frame_count(&priv->p); > + netdev_dbg(ndev, "rx_poll: %d frames in RX FIFO", framecnt); This will be rather noisy, right? > + /* Check for RX FIFO Overflow */ > + status = ctu_can_get_status(&priv->p); > + if (status.s.dor) { > + struct net_device_stats *stats = &ndev->stats; > + struct can_frame *cf; > + struct sk_buff *skb; > + > + netdev_info(ndev, " rx fifo overflow"); And this goes at different loglevel, which will be confusing? > +/** > + * xcan_tx_interrupt - Tx Done Isr > + * @ndev: net_device pointer > + */ > +static void ctucan_tx_interrupt(struct net_device *ndev) Mismatch between code and docs. > + netdev_dbg(ndev, "%s", __func__); This is inconsistent with other debugging.... and perhaps it is time to remove this kind of debugging for merge. > +/** > + * xcan_interrupt - CAN Isr > + */ > +static irqreturn_t ctucan_interrupt(int irq, void *dev_id) Inconsistent. > + /* Error interrupts */ > + if (isr.s.ewli || isr.s.fcsi || isr.s.ali) { > + union ctu_can_fd_int_stat ierrmask = { .s = { > + .ewli = 1, .fcsi = 1, .ali = 1, .bei = 1 } }; > + icr.u32 = isr.u32 & ierrmask.u32; We normally do bit arithmetics instead of this. > + { > + union ctu_can_fd_int_stat imask; > + > + imask.u32 = 0xffffffff; > + ctucan_hw_int_ena_clr(&priv->p, imask); > + ctucan_hw_int_mask_set(&priv->p, imask); > + } More like this. Plus avoid block here...? > +/** > + * ctucan_close - Driver close routine > + * @ndev: Pointer to net_device structure > + * > + * Return: 0 always > + */ You see, this is better. No need to say "Driver close routine" twice. Now, make the rest consistent :-). > +EXPORT_SYMBOL(ctucan_suspend); > +EXPORT_SYMBOL(ctucan_resume); _GPL? And what kind of multi-module stuff are you doing that you need symbols exported? > +int ctucan_probe_common(struct device *dev, void __iomem *addr, int irq, unsigned int ntxbufs, > + unsigned long can_clk_rate, int pm_enable_call, > + void (*set_drvdata_fnc)(struct device *dev, struct net_device *ndev)) > +{ Splitting/simplifying this somehow would be good. > +/* Register descriptions: */ > +union ctu_can_fd_frame_form_w { > + uint32_t u32; u32, as you write elsewhere. > + struct ctu_can_fd_frame_form_w_s { > +#ifdef __LITTLE_ENDIAN_BITFIELD > + /* FRAME_FORM_W */ > + uint32_t dlc : 4; > + uint32_t reserved_4 : 1; > + uint32_t rtr : 1; > + uint32_t ide : 1; > + uint32_t fdf : 1; > + uint32_t reserved_8 : 1; > + uint32_t brs : 1; > + uint32_t esi_rsv : 1; > + uint32_t rwcnt : 5; > + uint32_t reserved_31_16 : 16; > +#else I believe you should simply avoid using bitfields. > +union ctu_can_fd_timestamp_l_w { > + uint32_t u32; > + struct ctu_can_fd_timestamp_l_w_s { > + /* TIMESTAMP_L_W */ > + uint32_t time_stamp_31_0 : 32; > + } s; > +}; This is crazy. > +union ctu_can_fd_data_5_8_w { > + uint32_t u32; > + struct ctu_can_fd_data_5_8_w_s { > +#ifdef __LITTLE_ENDIAN_BITFIELD > + /* DATA_5_8_W */ > + uint32_t data_5 : 8; > + uint32_t data_6 : 8; > + uint32_t data_7 : 8; > + uint32_t data_8 : 8; > +#else > + uint32_t data_8 : 8; > + uint32_t data_7 : 8; > + uint32_t data_6 : 8; > + uint32_t data_5 : 8; > +#endif > + } s; > +}; even more crazy. > +#ifdef __KERNEL__ > +# include <linux/can/dev.h> > +#else > +/* The hardware registers mapping and low level layer should build > + * in userspace to allow development and verification of CTU CAN IP > + * core VHDL design when loaded into hardware. Debugging hardware > + * from kernel driver is really difficult, leads to system stucks > + * by error reporting etc. Testing of exactly the same code > + * in userspace together with headers generated automatically > + * generated from from IP-XACT/cactus helps to driver to hardware > + * and QEMU emulation model consistency keeping. > + */ > +# include "ctu_can_fd_linux_defs.h" > +#endif Please remove non-kernel code for merge. > +void ctucan_hw_write32(struct ctucan_hw_priv *priv, > + enum ctu_can_fd_can_registers reg, u32 val) > +{ > + iowrite32(val, priv->mem_base + reg); > +} And get rid of this kind of abstraction layer. > +// TODO: rename or do not depend on previous value of id TODO: grep for TODO and C++ comments before attempting merge. > +static bool ctucan_hw_len_to_dlc(u8 len, u8 *dlc) > +{ > + *dlc = can_len2dlc(len); > + return true; > +} Compared to how well other code is documented... This one is voodoo. > +bool ctucan_hw_set_ret_limit(struct ctucan_hw_priv *priv, bool enable, u8 limit) > +{ > + union ctu_can_fd_mode_settings reg; > + > + if (limit > CTU_CAN_FD_RETR_MAX) > + return false; > + > + reg.u32 = priv->read_reg(priv, CTU_CAN_FD_MODE); > + reg.s.rtrle = enable ? RTRLE_ENABLED : RTRLE_DISABLED; > + reg.s.rtrth = limit & 0xF; > + priv->write_reg(priv, CTU_CAN_FD_MODE, reg.u32); > + return true; > +} As elsewhere, I'd suggest 0/-ERRNO. > +void ctucan_hw_set_mode_reg(struct ctucan_hw_priv *priv, > + const struct can_ctrlmode *mode) > +{ > + u32 flags = mode->flags; > + union ctu_can_fd_mode_settings reg; > + > + reg.u32 = priv->read_reg(priv, CTU_CAN_FD_MODE); > + if (mode->mask & CAN_CTRLMODE_LOOPBACK) > + reg.s.ilbp = flags & CAN_CTRLMODE_LOOPBACK ? > + INT_LOOP_ENABLED : INT_LOOP_DISABLED; Not sure what is going on here, but having mode->flags in same format as hardware register would help...? > + switch (fnum) { > + case CTU_CAN_FD_FILTER_A: > + if (reg.s.sfa) > + return true; > + break; > + case CTU_CAN_FD_FILTER_B: > + if (reg.s.sfb) > + return true; > + break; > + case CTU_CAN_FD_FILTER_C: > + if (reg.s.sfc) > + return true; > + break; > + } Check indentation of break statemnts, also elsewhere in this file > +bool ctucan_hw_get_range_filter_support(struct ctucan_hw_priv *priv) > +{ > + union ctu_can_fd_filter_control_filter_status reg; > + > + reg.u32 = priv->read_reg(priv, CTU_CAN_FD_FILTER_CONTROL); > + > + if (reg.s.sfr) > + return true; return !!reg.s.sfr; ? > +enum ctu_can_fd_tx_status_tx1s ctucan_hw_get_tx_status(struct ctucan_hw_priv > + *priv, u8 buf) ... > + default: > + status = ~0; > + } > + return (enum ctu_can_fd_tx_status_tx1s)status; > +} Is ~0 in the enum? > + // TODO: use named constants for the command TODO... > +// TODO: AL_CAPTURE and ERROR_CAPTURE ... > +#if defined(__LITTLE_ENDIAN_BITFIELD) == defined(__BIG_ENDIAN_BITFIELD) > +# error __BIG_ENDIAN_BITFIELD or __LITTLE_ENDIAN_BITFIELD must be defined. > +#endif :-). > +// True if Core is transceiver of current frame > +#define CTU_CAN_FD_IS_TRANSMITTER(stat) (!!(stat).ts) > + > +// True if Core is receiver of current frame > +#define CTU_CAN_FD_IS_RECEIVER(stat) (!!(stat).s.rxs) Why not, documentation is nice. But it is in big contrast to other parts of code where there's no docs at all. > +struct ctucan_hw_priv; > +#ifndef ctucan_hw_priv > +struct ctucan_hw_priv { > + void __iomem *mem_base; > + u32 (*read_reg)(struct ctucan_hw_priv *priv, > + enum ctu_can_fd_can_registers reg); > + void (*write_reg)(struct ctucan_hw_priv *priv, > + enum ctu_can_fd_can_registers reg, u32 val); > +}; > +#endif Should not be needed in kernel. > +/** > + * ctucan_hw_read_rx_word - Reads one word of CAN Frame from RX FIFO Buffer. > + * > + * @priv: Private info > + * > + * Return: One wword of received frame Typo 'word'. > +++ b/drivers/net/can/ctucanfd/ctu_can_fd_regs.h > @@ -0,0 +1,971 @@ > + > +/* This file is autogenerated, DO NOT EDIT! */ > + Yay. How is that supposed to work after merge? Best regards, Pavel -- http://www.livejournal.com/~pavelmachek
Attachment:
signature.asc
Description: PGP signature
