lkml.org 
[lkml]   [2020]   [Oct]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v6 3/6] can: ctucanfd: add support for CTU CAN FD open-source IP core - bus independent part.
Hi!

> From: Martin Jerabek <martin.jerabek01@gmail.com>
>
> 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
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2020-10-22 13:03    [W:0.112 / U:2.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site