Re: [PATCH v4 3/6] can: ctucanfd: add support for CTU CAN FD open-source IP core - bus independent part.

From: Pavel Machek
Date: Tue Aug 04 2020 - 05:57:25 EST

Next message: Sakari Ailus: "Re: [PATCH] nvmem: core: add sanity check in nvmem_device_read()"
Previous message: David Hildenbrand: "Re: [PATCH v3 6/6] mm: document semantics of ZONE_MOVABLE"
In reply to: pisa: "[PATCH v4 3/6] can: ctucanfd: add support for CTU CAN FD open-source IP core - bus independent part."
Next in thread: pisa: "[PATCH v4 4/6] can: ctucanfd: CTU CAN FD open-source IP core - PCI bus support."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi!

> More about CAN related projects used and developed at the Faculty
> of the Electrical Engineering (http://www.fel.cvut.cz/en/)
> of Czech Technical University (https://www.cvut.cz/en)
> in at Prague http://canbus.pages.fel.cvut.cz/ .

Should this go to Documentation, not a changelog?

> + help
> + 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).
> + More about CAN related projects used and developed at the Faculty
> + of the Electrical Engineering (http://www.fel.cvut.cz/en/)
> + of Czech Technical University in Prague (https://www.cvut.cz/en)
> + at http://canbus.pages.fel.cvut.cz/ .

Again, links to university main mage here are little bit excessive.

> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*******************************************************************************
> + *
> + * CTU CAN FD IP Core
> + * Copyright (C) 2015-2018
> + *
> + * Authors:
> + * Ondrej Ille <ondrej.ille@xxxxxxxxx>
> + * Martin Jerabek <martin.jerabek01@xxxxxxxxx>
> + * Jaroslav Beran <jara.beran@xxxxxxxxx>
> + *
> + * Project advisors:
> + * Jiri Novak <jnovak@xxxxxxxxxxx>
> + * Pavel Pisa <pisa@xxxxxxxxxxxxxxxx>
> + *
> + * Department of Measurement (http://meas.fel.cvut.cz/)
> + * Faculty of Electrical Engineering (http://www.fel.cvut.cz)
> + * Czech Technical University (http://www.cvut.cz/)

Again, a bit too many links... and important information is missing:
who is the copyright holder?

> + * 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.

With SPDX, this should be removed.

> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Martin Jerabek");
> +MODULE_DESCRIPTION("CTU CAN FD interface");

This normally goes to end of the file, MODULE_AUTHOR usually has <>
section with email address.

> +/* TX buffer rotation:
> + * - when a buffer transitions to empty state, rotate order and priorities
> + * - if more buffers seem to transition at the same time, rotate
> + * by the number of buffers
> + * - it may be assumed that buffers transition to empty state in FIFO order
> + * (because we manage priorities that way)
> + * - at frame filling, do not rotate anything, just increment buffer modulo
> + * counter
> + */
> +
> +#define CTUCAN_FLAG_RX_FFW_BUFFERED 1
> +
> +static int ctucan_reset(struct net_device *ndev)
> +{
> + int i;
> + struct ctucan_priv *priv = netdev_priv(ndev);
> +
> + netdev_dbg(ndev, "ctucan_reset");
> +
> + ctucan_hw_reset(&priv->p);
> + for (i = 0; i < 100; ++i) {

i++ would be usual kernel style.

> + if (ctucan_hw_check_access(&priv->p))
> + return 0;
> + usleep_range(100, 200);
> + }
> +
> + netdev_warn(ndev, "device did not leave reset\n");
> + return -ETIMEDOUT;
> +}

This does extra sleep after last check_access... but I doubt that
matters.

> +static int ctucan_set_bittiming(struct net_device *ndev)
> +{
> + struct ctucan_priv *priv = netdev_priv(ndev);
> + struct can_bittiming *bt = &priv->can.bittiming;
> +
> + netdev_dbg(ndev, "ctucan_set_bittiming");
> +
> + if (ctucan_hw_is_enabled(&priv->p)) {
> + netdev_alert(ndev,
> + "BUG! Cannot set bittiming - CAN is enabled\n");
> + return -EPERM;
> + }

alert is normally reserved for .. way higher severity.

> + /* Note that bt may be modified here */
> + ctucan_hw_set_nom_bittiming(&priv->p, bt);
> +
> + return 0;
> +}
> +
> +/**
> + * ctucan_set_data_bittiming - CAN set data bit timing routine
> + * @ndev: Pointer to net_device structure
> + *
> + * This is the driver set data bittiming routine.
> + * Return: 0 on success and failure value on error
> + */
> +static int ctucan_set_data_bittiming(struct net_device *ndev)
> +{
> + struct ctucan_priv *priv = netdev_priv(ndev);
> + struct can_bittiming *dbt = &priv->can.data_bittiming;
> +
> + netdev_dbg(ndev, "ctucan_set_data_bittiming");
> +
> + if (ctucan_hw_is_enabled(&priv->p)) {
> + netdev_alert(ndev,
> + "BUG! Cannot set bittiming - CAN is enabled\n");
> + return -EPERM;
> + }
> +
> + /* Note that dbt may be modified here */
> + ctucan_hw_set_data_bittiming(&priv->p, dbt);
> +
> + return 0;
> +}
> +
> +/**
> + * ctucan_set_secondary_sample_point - CAN set secondary sample point routine
> + * @ndev: Pointer to net_device structure
> + *
> + * Return: 0 on success and failure value on error
> + */
> +static int ctucan_set_secondary_sample_point(struct net_device *ndev)
> +{
> + struct ctucan_priv *priv = netdev_priv(ndev);
> + struct can_bittiming *dbt = &priv->can.data_bittiming;
> + int ssp_offset = 0;
> + bool ssp_ena;
> +
> + netdev_dbg(ndev, "ctucan_set_secondary_sample_point");
> +
> + if (ctucan_hw_is_enabled(&priv->p)) {
> + netdev_alert(ndev,
> + "BUG! Cannot set SSP - CAN is enabled\n");
> + return -EPERM;
> + }
> +
> + // Use for bit-rates above 1 Mbits/s

/* */ style comment would be more common here.

> + if (dbt->bitrate > 1000000) {
> + ssp_ena = true;
> +
> + // Calculate SSP in minimal time quanta
> + ssp_offset = (priv->can.clock.freq / 1000) *
> + dbt->sample_point / dbt->bitrate;
> +
> + if (ssp_offset > 127) {
> + netdev_warn(ndev, "SSP offset saturated to 127\n");
> + ssp_offset = 127;
> + }
> + } else {
> + ssp_ena = false;
> + }

Init ssp_ena to false, and you can get rid of else branch?

> +/**
> + * ctucan_chip_start - This the drivers start routine
> + * @ndev: Pointer to net_device structure
> + *
> + * This is the drivers start routine.

driver's?

> +/**
> + * ctucan_start_xmit - Starts the transmission
...
> + * Return: 0 on success and failure value on error

Umm, no, AFAICT.

> +static int ctucan_start_xmit(struct sk_buff *skb, struct net_device *ndev)

Should it return netdev_tx_t ?

> +/**
> + * xcan_rx - Is called from CAN isr to complete the received
> + * frame processing

Double space.

> +static const char *ctucan_state_to_str(enum can_state state)
> +{
> + if (state >= CAN_STATE_MAX)
> + return "UNKNOWN";
> + return ctucan_state_strings[state];
> +}

Is enum always unsigned?

> +/**
> + * ctucan_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.
> + */

You are free to use 80 columns...

> + skb = alloc_can_err_skb(ndev, &cf);
> +
> + /* EWLI: error warning limit condition met
> + * FCSI: Fault confinement State changed
> + * ALI: arbitration lost (just informative)
> + * BEI: bus error interrupt
> + */

Extra space before "error"... and something is wrong with big letters there.

> + if (state == CAN_STATE_BUS_OFF) {
> + priv->can.can_stats.bus_off++;
> + can_bus_off(ndev);
> + if (skb)
> + cf->can_id |= CAN_ERR_BUSOFF;
> + } else if (state == CAN_STATE_ERROR_PASSIVE) {
> + priv->can.can_stats.error_passive++;
> + if (skb) {
> + cf->can_id |= CAN_ERR_CRTL;
> + cf->data[1] = (berr.rxerr > 127) ?
> + CAN_ERR_CRTL_RX_PASSIVE :
> + CAN_ERR_CRTL_TX_PASSIVE;
> + cf->data[6] = berr.txerr;
> + cf->data[7] = berr.rxerr;
> + }
> + } else if (state == CAN_STATE_ERROR_WARNING) {
> + priv->can.can_stats.error_warning++;
> + if (skb) {
> + cf->can_id |= CAN_ERR_CRTL;
> + cf->data[1] |= (berr.txerr > berr.rxerr) ?
> + CAN_ERR_CRTL_TX_WARNING :
> + CAN_ERR_CRTL_RX_WARNING;
> + cf->data[6] = berr.txerr;
> + cf->data[7] = berr.rxerr;
> + }
> + } else if (state == CAN_STATE_ERROR_ACTIVE) {
> + cf->data[1] = CAN_ERR_CRTL_ACTIVE;
> + cf->data[6] = berr.txerr;
> + cf->data[7] = berr.rxerr;
> + } else {
> + netdev_warn(ndev, " unhandled error state (%d:%s)!",
> + state, ctucan_state_to_str(state));
> + }

This sounds like switch (state) to me?

> + /* Check for Bus Error interrupt */
> + if (isr.s.bei) {
> + netdev_info(ndev, " bus error");
> + priv->can.can_stats.bus_error++;
> + stats->tx_errors++; // TODO: really?

really? :-).

> + some_buffers_processed = false;
> + while ((int)(priv->txb_head - priv->txb_tail) > 0) {
> + u32 txb_idx = priv->txb_tail & priv->txb_mask;
> + u32 status = ctucan_hw_get_tx_status(&priv->p, txb_idx);
> +
> + netdev_dbg(ndev, "TXI: TXB#%u: status 0x%x",
> + txb_idx, status);
> +
> + switch (status) {
> + case TXT_TOK:
> + netdev_dbg(ndev, "TXT_OK");
> + can_get_echo_skb(ndev, txb_idx);
> + stats->tx_packets++;
> + break;
> + case TXT_ERR:
> + /* This indicated that retransmit limit has been
> + * reached. Obviously we should not echo the
> + * frame, but also not indicate any kind
> + * of error. If desired, it was already reported
> + * (possible multiple times) on each arbitration
> + * lost.
> + */
> + netdev_warn(ndev, "TXB in Error state");
> + can_free_echo_skb(ndev, txb_idx);
> + stats->tx_dropped++;
> + break;
> + case TXT_ABT:
> + /* Same as for TXT_ERR, only with different
> + * cause. We *could* re-queue the frame, but
> + * multiqueue/abort is not supported yet anyway.
> + */
> + netdev_warn(ndev, "TXB in Aborted state");
> + can_free_echo_skb(ndev, txb_idx);
> + stats->tx_dropped++;
> + break;
> + default:
> + /* Bug only if the first buffer is not finished,
> + * otherwise it is pretty much expected
> + */
> + if (first) {
> + netdev_err(ndev, "BUG: TXB#%u not in a finished state (0x%x)!",
> + txb_idx, status);
> + spin_unlock_irqrestore(&priv->tx_lock,
> + flags);
> + /* do not clear nor wake */
> + return;
> + }
> + goto clear;
> + }
> + priv->txb_tail++;
> + first = false;
> + some_buffers_processed = true;
> + /* Adjust priorities *before* marking the buffer
> + * as empty.
> + */
> + ctucan_rotate_txb_prio(ndev);
> + ctucan_hw_txt_set_empty(&priv->p, txb_idx);
> + }
> +clear:
> + spin_unlock_irqrestore(&priv->tx_lock, flags);

Could some part of this be split into separate function?

> + /* If no buffers were processed this time, wa cannot

we

> + * clear - that would introduce a race condition.
> + */
> + if (some_buffers_processed) {
> + /* Clear the interrupt again as not to receive it again
> + * for a buffer we already handled (possibly causing
> + * the bug log)
> + */

Not sure this is correct english.

> +static irqreturn_t ctucan_interrupt(int irq, void *dev_id)
> +{
> + struct net_device *ndev = (struct net_device *)dev_id;
> + struct ctucan_priv *priv = netdev_priv(ndev);
> + union ctu_can_fd_int_stat isr, icr;
> + int irq_loops = 0;
> +
> + netdev_dbg(ndev, "ctucan_interrupt");
> +
> + do {
> + /* Get the interrupt status */
> + isr = ctu_can_fd_int_sts(&priv->p);

> + }
> + /* Ignore RI, TI, LFI, RFI, BSI */
> + } while (irq_loops++ < 10000);

For loop would be more usual here.

> +static void ctucan_chip_stop(struct net_device *ndev)
> +{
> + struct ctucan_priv *priv = netdev_priv(ndev);
> + union ctu_can_fd_int_stat mask;
> +
> + netdev_dbg(ndev, "ctucan_chip_stop");
> +
> + mask.u32 = 0xffffffff;
> +
> + /* Disable interrupts and disable can */

can->CAN?

> + netdev_dbg(ndev, "ctucan_open");
> +
> + ret = pm_runtime_get_sync(priv->dev);
> + if (ret < 0) {
> + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
> + __func__, ret);
> + return ret;
> + }

IIRC pm_runtime_get... is special, and you need to put even in the
error case.

> + ret = pm_runtime_get_sync(priv->dev);
> + if (ret < 0) {
> + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
> + __func__, ret);
> + return ret;

Same here.

> +int ctucan_suspend(struct device *dev)
> +EXPORT_SYMBOL(ctucan_suspend);
> +int ctucan_resume(struct device *dev)
> +EXPORT_SYMBOL(ctucan_resume);

Exporting these is quite unusual.

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

At least format it so that set_drvdata_fnc is on single line?

> +{
...
> + if (set_drvdata_fnc != NULL)
> + set_drvdata_fnc(dev, ndev);

No need for != NULL.

> + SET_NETDEV_DEV(ndev, dev);
> + ndev->netdev_ops = &ctucan_netdev_ops;
> +
> + /* Getting the CAN can_clk info */
> + if (can_clk_rate == 0) {

!can_clk_rate would work, too.

> + priv->can_clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(priv->can_clk)) {
> + dev_err(dev, "Device clock not found.\n");
> + ret = PTR_ERR(priv->can_clk);
> + goto err_free;
> + }
> + can_clk_rate = clk_get_rate(priv->can_clk);
> + }
> +
> + priv->p.write_reg = ctucan_hw_write32;
> + priv->p.read_reg = ctucan_hw_read32;
> +
> + if (pm_enable_call)
> + pm_runtime_enable(dev);
> + ret = pm_runtime_get_sync(dev);
> + if (ret < 0) {

put?

> + if ((priv->p.read_reg(&priv->p, CTU_CAN_FD_DEVICE_ID) &
> + 0xFFFF) != CTU_CAN_FD_ID) {
> + priv->p.write_reg = ctucan_hw_write32_be;
> + priv->p.read_reg = ctucan_hw_read32_be;
> + if ((priv->p.read_reg(&priv->p, CTU_CAN_FD_DEVICE_ID) &
> + 0xFFFF) != CTU_CAN_FD_ID) {
> + netdev_err(ndev, "CTU_CAN_FD signature not found\n");
> + ret = -ENODEV;
> + goto err_disableclks;
> + }
> + }
> +
> + ret = ctucan_reset(ndev);
> + if (ret < 0)
> + goto err_pmdisable;

Should be goto err_disableclks, AFAICT. Plus that label is quite confusing.

> +static __init int ctucan_init(void)
> +{
> + pr_info("%s CAN netdevice driver\n", DRV_NAME);
> +
> + return 0;
> +}
> +
> +module_init(ctucan_init);

> +static __exit void ctucan_exit(void)
> +{
> + pr_info("%s: driver removed\n", DRV_NAME);
> +}
> +
> +module_exit(ctucan_exit);

Remove?

> +#ifndef __KERNEL__
> +# include "ctu_can_fd_linux_defs.h"
> +#else
> +# include <linux/can/dev.h>
> +#endif

Should always assume kernel?

Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachment: signature.asc
Description: PGP signature

Next message: Sakari Ailus: "Re: [PATCH] nvmem: core: add sanity check in nvmem_device_read()"
Previous message: David Hildenbrand: "Re: [PATCH v3 6/6] mm: document semantics of ZONE_MOVABLE"
In reply to: pisa: "[PATCH v4 3/6] can: ctucanfd: add support for CTU CAN FD open-source IP core - bus independent part."
Next in thread: pisa: "[PATCH v4 4/6] can: ctucanfd: CTU CAN FD open-source IP core - PCI bus support."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]