Message-ID: <20200804095717.irnchkz2imw7tdf7@duo.ucw.cz>
Date:   Tue, 4 Aug 2020 11:57:17 +0200
From:   Pavel Machek <pavel@....cz>
To:     pisa@....felk.cvut.cz
Cc:     linux-can@...r.kernel.org, devicetree@...r.kernel.org,
        mkl@...gutronix.de, socketcan@...tkopp.net, wg@...ndegger.com,
        davem@...emloft.net, robh+dt@...nel.org, mark.rutland@....com,
        c.emde@...dl.org, armbru@...hat.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, martin.jerabek01@...il.com,
        ondrej.ille@...il.com, jnovak@....cvut.cz, jara.beran@...il.com,
        porazil@...ron.com
Subject: Re: [PATCH v4 3/6] can: ctucanfd: add support for CTU CAN FD
 open-source IP core - bus independent part.

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@...il.com>
> + *     Martin Jerabek <martin.jerabek01@...il.com>
> + *     Jaroslav Beran <jara.beran@...il.com>
> + *
> + * Project advisors:
> + *     Jiri Novak <jnovak@....cvut.cz>
> + *     Pavel Pisa <pisa@....felk.cvut.cz>
> + *
> + * 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

Download attachment "signature.asc" of type "application/pgp-signature" (196 bytes)

Powered by blists - more mailing lists