[RFC,v3,4/4] can: bxcan: add support for ST bxCAN controller

2317 diff mbox series

Add support for the basic extended CAN controller (bxCAN) found in many
low- to middle-end STM32 SoCs. It supports the Basic Extended CAN
protocol versions 2.0A and B with a maximum bit rate of 1 Mbit/s.

The controller supports two channels (CAN1 as master and CAN2 as slave)
and the driver can enable either or both of the channels. They share
some of the required logic (e. g. clocks and filters), and that means
you cannot use the slave CAN without enabling some hardware resources
managed by the master CAN.

Each channel has 3 transmit mailboxes, 2 receive FIFOs with 3 stages and
28 scalable filter banks.
It also manages 4 dedicated interrupt vectors:
- transmit interrupt
- FIFO 0 receive interrupt
- FIFO 1 receive interrupt
- status change error interrupt

Driver uses all 3 available mailboxes for transmission and FIFO 0 for
reception. Rx filter rules are configured to the minimum. They accept
all messages and assign filter 0 to CAN1 and filter 14 to CAN2 in
identifier mask mode with 32 bits width. It enables and uses transmit,
receive buffers for FIFO 0 and error and status change interrupts.

Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

---

Changes in v3:
- Remove 'Dario Binacchi <dariobin@libero.it>' SOB.
- Fix the documentation file path in the MAINTAINERS entry.
- Do not increment the "stats->rx_bytes" if the frame is remote.
- Remove pr_debug() call from bxcan_rmw().

Changes in v2:
- Fix sparse errors.
- Create a MAINTAINERS entry.
- Remove the print of the registers address.
- Remove the volatile keyword from bxcan_rmw().
- Use tx ring algorithm to manage tx mailboxes.
- Use can_{get|put}_echo_skb().
- Update DT properties.

 MAINTAINERS                        |    7 +
 drivers/net/can/Kconfig            |    1 +
 drivers/net/can/Makefile           |    1 +
 drivers/net/can/bxcan/Kconfig      |   34 +
 drivers/net/can/bxcan/Makefile     |    4 +
 drivers/net/can/bxcan/bxcan-core.c |  200 ++++++
 drivers/net/can/bxcan/bxcan-core.h |   31 +
 drivers/net/can/bxcan/bxcan-drv.c  | 1045 ++++++++++++++++++++++++++++
 8 files changed, 1323 insertions(+)
 create mode 100644 drivers/net/can/bxcan/Kconfig
 create mode 100644 drivers/net/can/bxcan/Makefile
 create mode 100644 drivers/net/can/bxcan/bxcan-core.c
 create mode 100644 drivers/net/can/bxcan/bxcan-core.h
 create mode 100644 drivers/net/can/bxcan/bxcan-drv.c

On 28.08.2022 15:33:29, Dario Binacchi wrote:
> Add support for the basic extended CAN controller (bxCAN) found in many
> low- to middle-end STM32 SoCs. It supports the Basic Extended CAN
> protocol versions 2.0A and B with a maximum bit rate of 1 Mbit/s.
> 
> The controller supports two channels (CAN1 as master and CAN2 as slave)
> and the driver can enable either or both of the channels. They share
> some of the required logic (e. g. clocks and filters), and that means
> you cannot use the slave CAN without enabling some hardware resources
> managed by the master CAN.

I don't like the concept of the core driver. E.g. the r-m-w of shared
registers bxcan_enable_filters() is racy by design.

How is the memory layout?
- 0x000 - 0x1ff: CAN1
- 0x200 - 0x3ff: shared mem
- 0x400 - 0x5ff: CAN2

Better model the shared mem as a "syscon" node:

| https://elixir.bootlin.com/linux/v6.0-rc4/source/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c#L1445

Then you can use regmap_*() functions to access the register space. The
regmap functions are always locked against each other.

[...]

> diff --git a/drivers/net/can/bxcan/bxcan-core.c b/drivers/net/can/bxcan/bxcan-core.c
> new file mode 100644
> index 000000000000..3644449095e9
> --- /dev/null
> +++ b/drivers/net/can/bxcan/bxcan-core.c
> @@ -0,0 +1,200 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* bxcan-core.c - STM32 Basic Extended CAN core controller driver
> + *
> + * This file is part of STM32 bxcan driver
> + *
> + * Copyright (c) 2022 Dario Binacchi <dario.binacchi@amarulasolutions.com>
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +
> +#include "bxcan-core.h"
> +
> +#define BXCAN_FILTER_ID(master)       (master ? 0 : 14)
> +
> +/* Filter master register (FMR) bits */
> +#define BXCAN_FMR_CANSB_MASK	GENMASK(13, 8)
> +#define BXCAN_FMR_CANSB(x)	(((x) & 0x3f) << 8)
> +#define BXCAN_FMR_FINIT		BIT(0)

nitpick:
please use 1 space instead of tabs

> +
> +/* Structure of the filter bank */
> +struct bxcan_fb {
> +	u32 fr1;		/* filter 1 */
> +	u32 fr2;		/* filter 2 */
> +};
> +
> +/* Structure of the hardware filter registers */
> +struct bxcan_fregs {
> +	u32 fmr;		/* 0x00 - filter master */
> +	u32 fm1r;		/* 0x04 - filter mode */
> +	u32 reserved2;		/* 0x08 */
> +	u32 fs1r;		/* 0x0c - filter scale */
> +	u32 reserved3;		/* 0x10 */
> +	u32 ffa1r;		/* 0x14 - filter FIFO assignment */
> +	u32 reserved4;		/* 0x18 */
> +	u32 fa1r;		/* 0x1c - filter activation */
> +	u32 reserved5[8];	/* 0x20 */
> +	struct bxcan_fb fb[28];	/* 0x40 - filter banks */
> +};
> +
> +struct bxcan_core_priv {
> +	void __iomem *base;
> +	struct bxcan_fregs __iomem *fregs;
> +	struct clk *clk_master;
> +	unsigned int clk_master_ref;
> +};
> +
> +void bxcan_disable_filters(struct device *dev, bool master)
> +{
> +	struct bxcan_core_priv *priv = dev_get_drvdata(dev);
> +	unsigned int fid = BXCAN_FILTER_ID(master);
> +	u32 fmask = BIT(fid);
> +
> +	bxcan_rmw(&priv->fregs->fa1r, fmask, 0);

This is racy, same for bxcan_rmw() on other shared registers.

> +}
> +
> +void bxcan_enable_filters(struct device *dev, bool master)
> +{
> +	struct bxcan_core_priv *priv = dev_get_drvdata(dev);
> +	unsigned int fid = BXCAN_FILTER_ID(master);
> +	u32 fmask = BIT(fid);
> +
> +	/* Filter settings:
> +	 *
> +	 * Accept all messages.
> +	 * Assign filter 0 to CAN1 and filter 14 to CAN2 in identifier
> +	 * mask mode with 32 bits width.
> +	 */
> +
> +	/* Enter filter initialization mode and assing filters to CAN
> +	 * controllers.
> +	 */
> +	bxcan_rmw(&priv->fregs->fmr, BXCAN_FMR_CANSB_MASK,
> +		  BXCAN_FMR_CANSB(14) | BXCAN_FMR_FINIT);
> +
> +	/* Deactivate filter */
> +	bxcan_rmw(&priv->fregs->fa1r, fmask, 0);
> +
> +	/* Two 32-bit registers in identifier mask mode */
> +	bxcan_rmw(&priv->fregs->fm1r, fmask, 0);
> +
> +	/* Single 32-bit scale configuration */
> +	bxcan_rmw(&priv->fregs->fs1r, 0, fmask);
> +
> +	/* Assign filter to FIFO 0 */
> +	bxcan_rmw(&priv->fregs->ffa1r, fmask, 0);
> +
> +	/* Accept all messages */
> +	writel(0, &priv->fregs->fb[fid].fr1);
> +	writel(0, &priv->fregs->fb[fid].fr2);
> +
> +	/* Activate filter */
> +	bxcan_rmw(&priv->fregs->fa1r, 0, fmask);
> +
> +	/* Exit filter initialization mode */
> +	bxcan_rmw(&priv->fregs->fmr, BXCAN_FMR_FINIT, 0);
> +}
> +
> +int bxcan_enable_master_clk(struct device *dev)
> +{
> +	struct bxcan_core_priv *priv = dev_get_drvdata(dev);
> +	int err;
> +
> +	if (priv->clk_master_ref == 0) {
> +		err = clk_prepare_enable(priv->clk_master);
> +		if (err)
> +			return err;
> +	}
> +
> +	priv->clk_master_ref++;

In general, don't do clock ref-counting. You can enable and disable the
same clock several times.

If you convert to syscon, you can add a clock node to the syscon node
and the syscon driver will take care of the clocks.

> +	return 0;
> +}
> +
> +void bxcan_disable_master_clk(struct device *dev)
> +{
> +	struct bxcan_core_priv *priv = dev_get_drvdata(dev);
> +
> +	if (priv->clk_master_ref == 0)
> +		return;
> +
> +	if (priv->clk_master_ref == 1)
> +		clk_disable_unprepare(priv->clk_master);
> +
> +	priv->clk_master_ref--;
> +}
> +
> +unsigned long bxcan_get_master_clk_rate(struct device *dev)
> +{
> +	struct bxcan_core_priv *priv = dev_get_drvdata(dev);
> +
> +	return clk_get_rate(priv->clk_master);
> +}
> +
> +void __iomem *bxcan_get_base_addr(struct device *dev)
> +{
> +	struct bxcan_core_priv *priv = dev_get_drvdata(dev);
> +
> +	return priv->base;
> +}
> +
> +static const struct of_device_id bxcan_core_of_match[] = {
> +	{.compatible = "st,stm32f4-bxcan-core"},
> +	{ /* sentinel */ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, bxcan_core_of_match);
> +
> +static int bxcan_core_probe(struct platform_device *pdev)
> +{
> +	struct bxcan_core_priv *priv;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = pdev->dev.of_node;
> +	void __iomem *regs;
> +	struct clk *clk;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +	regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
> +
> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk)) {
> +		dev_err(dev, "failed to get clock\n");
> +		return PTR_ERR(clk);
> +	}
> +
> +	priv->base = regs;
> +	priv->fregs = regs + 0x200;
> +	priv->clk_master = clk;
> +
> +	ret = of_platform_populate(np, NULL, NULL, dev);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to populate DT children\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct platform_driver bxcan_core_driver = {
> +	.driver = {
> +		.name = KBUILD_MODNAME,
> +		.of_match_table = bxcan_core_of_match,
> +	},
> +	.probe = bxcan_core_probe,
> +};
> +
> +module_platform_driver(bxcan_core_driver);
> +
> +MODULE_AUTHOR("Dario Binacchi <dario.binacchi@amarulasolutions.com>");
> +MODULE_DESCRIPTION("STMicroelectronics Basic Extended CAN core driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/net/can/bxcan/bxcan-core.h b/drivers/net/can/bxcan/bxcan-core.h
> new file mode 100644
> index 000000000000..925449cbc3bd
> --- /dev/null
> +++ b/drivers/net/can/bxcan/bxcan-core.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * bxcan-core - STM32 Basic Extended CAN core controller driver
> + *
> + * Copyright (c) 2022 Dario Binacchi <dario.binacchi@amarulasolutions.com>
> + */
> +
> +#ifndef __BXCAN_CORE_H
> +#define __BXCAN_CORE_H
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +
> +int bxcan_enable_master_clk(struct device *dev);
> +void bxcan_disable_master_clk(struct device *dev);
> +unsigned long bxcan_get_master_clk_rate(struct device *dev);
> +void __iomem *bxcan_get_base_addr(struct device *dev);
> +void bxcan_enable_filters(struct device *dev, bool master);
> +void bxcan_disable_filters(struct device *dev, bool master);
> +
> +static inline void bxcan_rmw(void __iomem *addr, u32 clear, u32 set)
> +{
> +	u32 old, val;
> +
> +	old = readl(addr);
> +	val = (old & ~clear) | set;
> +	if (val != old)
> +		writel(val, addr);
> +}
> +
> +#endif
> diff --git a/drivers/net/can/bxcan/bxcan-drv.c b/drivers/net/can/bxcan/bxcan-drv.c
> new file mode 100644
> index 000000000000..7ac1ee7a4269
> --- /dev/null
> +++ b/drivers/net/can/bxcan/bxcan-drv.c
> @@ -0,0 +1,1045 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// bxcan-drv.c - STM32 Basic Extended CAN controller driver
> +//
> +// Copyright (c) 2022 Dario Binacchi <dario.binacchi@amarulasolutions.com>
> +//
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/bitfield.h>
> +#include <linux/can.h>
> +#include <linux/can/dev.h>
> +#include <linux/can/error.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +
> +#include "bxcan-core.h"
> +
> +#define BXCAN_NAPI_WEIGHT		3
> +#define BXCAN_TIMEOUT_US		10000
> +
> +#define BXCAN_TX_MB_NUM			3
> +
> +/* Master control register (MCR) bits */
> +#define BXCAN_MCR_DBF			BIT(16)
> +#define BXCAN_MCR_RESET			BIT(15)
> +#define BXCAN_MCR_TTCM			BIT(7)
> +#define BXCAN_MCR_ABOM			BIT(6)
> +#define BXCAN_MCR_AWUM			BIT(5)
> +#define BXCAN_MCR_NART			BIT(4)
> +#define BXCAN_MCR_RFLM			BIT(3)
> +#define BXCAN_MCR_TXFP			BIT(2)
> +#define BXCAN_MCR_SLEEP			BIT(1)
> +#define BXCAN_MCR_INRQ			BIT(0)
> +
> +/* Master status register (MSR) bits */
> +#define BXCAN_MSR_RX			BIT(11)
> +#define BXCAN_MSR_SAMP			BIT(10)
> +#define BXCAN_MSR_RXM			BIT(9)
> +#define BXCAN_MSR_TXM			BIT(8)
> +#define BXCAN_MSR_SLAKI			BIT(4)
> +#define BXCAN_MSR_WKUI			BIT(3)
> +#define BXCAN_MSR_ERRI			BIT(2)
> +#define BXCAN_MSR_SLAK			BIT(1)
> +#define BXCAN_MSR_INAK			BIT(0)
> +
> +/* Transmit status register (TSR) bits */
> +#define BXCAN_TSR_LOW2			BIT(31)
> +#define BXCAN_TSR_LOW1			BIT(30)
> +#define BXCAN_TSR_LOW0			BIT(29)
> +#define BXCAN_TSR_TME			GENMASK(28, 26)
> +#define BXCAN_TSR_TME_SHIFT		(26)

unused

> +#define BXCAN_TSR_TME2			BIT(28)
> +#define BXCAN_TSR_TME1			BIT(27)
> +#define BXCAN_TSR_TME0			BIT(26)
> +#define BXCAN_TSR_CODE			GENMASK(25, 24)
> +#define BXCAN_TSR_ABRQ2			BIT(23)
> +#define BXCAN_TSR_TERR2			BIT(19)
> +#define BXCAN_TSR_ALST2			BIT(18)
> +#define BXCAN_TSR_TXOK2			BIT(17)
> +#define BXCAN_TSR_RQCP2			BIT(16)
> +#define BXCAN_TSR_ABRQ1			BIT(15)
> +#define BXCAN_TSR_TERR1			BIT(11)
> +#define BXCAN_TSR_ALST1			BIT(10)
> +#define BXCAN_TSR_TXOK1			BIT(9)
> +#define BXCAN_TSR_RQCP1			BIT(8)
> +#define BXCAN_TSR_ABRQ0			BIT(7)
> +#define BXCAN_TSR_TERR0			BIT(3)
> +#define BXCAN_TSR_ALST0			BIT(2)
> +#define BXCAN_TSR_TXOK0			BIT(1)
> +#define BXCAN_TSR_RQCP0			BIT(0)
> +
> +/* Receive FIFO 0 register (RF0R) bits */
> +#define BXCAN_RF0R_RFOM0		BIT(5)
> +#define BXCAN_RF0R_FOVR0		BIT(4)
> +#define BXCAN_RF0R_FULL0		BIT(3)
> +#define BXCAN_RF0R_FMP0			GENMASK(1, 0)
> +
> +/* Interrupt enable register (IER) bits */
> +#define BXCAN_IER_SLKIE			BIT(17)
> +#define BXCAN_IER_WKUIE			BIT(16)
> +#define BXCAN_IER_ERRIE			BIT(15)
> +#define BXCAN_IER_LECIE			BIT(11)
> +#define BXCAN_IER_BOFIE			BIT(10)
> +#define BXCAN_IER_EPVIE			BIT(9)
> +#define BXCAN_IER_EWGIE			BIT(8)
> +#define BXCAN_IER_FOVIE1		BIT(6)
> +#define BXCAN_IER_FFIE1			BIT(5)
> +#define BXCAN_IER_FMPIE1		BIT(4)
> +#define BXCAN_IER_FOVIE0		BIT(3)
> +#define BXCAN_IER_FFIE0			BIT(2)
> +#define BXCAN_IER_FMPIE0		BIT(1)
> +#define BXCAN_IER_TMEIE			BIT(0)
> +
> +/* Error status register (ESR) bits */
> +#define BXCAN_ESR_REC_SHIFT		(24)
> +#define BXCAN_ESR_REC			GENMASK(31, 24)
> +#define BXCAN_ESR_TEC_SHIFT		(16)
> +#define BXCAN_ESR_TEC			GENMASK(23, 16)
> +#define BXCAN_ESR_LEC_SHIFT		(4)

please remove the _SHIFT macros, no need for them...

> +#define BXCAN_ESR_LEC			GENMASK(6, 4)
> +#define BXCAN_ESR_BOFF			BIT(1)
> +#define BXCAN_ESR_EPVF			BIT(1)
> +#define BXCAN_ESR_EWGF			BIT(0)
> +#define BXCAN_TEC(esr)			(((esr) & BXCAN_ESR_TEC) >> \
> +					 BXCAN_ESR_TEC_SHIFT)
> +#define BXCAN_REC(esr)			(((esr) & BXCAN_ESR_REC) >> \
> +					 BXCAN_ESR_REC_SHIFT)

... remove these and use FIELD_GET() directly.

> +
> +/* Bit timing register (BTR) bits */
> +#define BXCAN_BTR_SILM			BIT(31)
> +#define BXCAN_BTR_LBKM			BIT(30)
> +#define BXCAN_BTR_SJW_MASK		GENMASK(25, 24)
> +#define BXCAN_BTR_SJW(x)		(((x) & 0x03) << 24)
> +#define BXCAN_BTR_TS2_MASK		GENMASK(22, 20)
> +#define BXCAN_BTR_TS2(x)		(((x) & 0x07) << 20)
> +#define BXCAN_BTR_TS1_MASK		GENMASK(19, 16)
> +#define BXCAN_BTR_TS1(x)		(((x) & 0x0f) << 16)
> +#define BXCAN_BTR_BRP_MASK		GENMASK(9, 0)
> +#define BXCAN_BTR_BRP(x)		((x) & 0x3ff)

same for these

> +
> +/* TX mailbox identifier register (TIxR, x = 0..2) bits */
> +#define BXCAN_TIxR_STID(x)		(((x) & 0x7ff) << 21)
> +#define BXCAN_TIxR_EXID(x)		((x) << 3)

same here

> +#define BXCAN_TIxR_IDE			BIT(2)
> +#define BXCAN_TIxR_RTR			BIT(1)
> +#define BXCAN_TIxR_TXRQ			BIT(0)
> +
> +/* TX mailbox data length and time stamp register (TDTxR, x = 0..2 bits */
> +#define BXCAN_TDTxR_TIME(x)		(((x) & 0x0f) << 16)

same

> +#define BXCAN_TDTxR_TGT			BIT(8)
> +#define BXCAN_TDTxR_DLC_MASK		GENMASK(3, 0)
> +#define BXCAN_TDTxR_DLC(x)		((x) & 0x0f)

same

> +
> +/* RX FIFO mailbox identifier register (RIxR, x = 0..1 */
> +#define BXCAN_RIxR_STID_SHIFT		(21)
> +#define BXCAN_RIxR_EXID_SHIFT		(3)
> +#define BXCAN_RIxR_IDE			BIT(2)
> +#define BXCAN_RIxR_RTR			BIT(1)
> +
> +/* RX FIFO mailbox data length and timestamp register (RDTxR, x = 0..1) bits */
> +#define BXCAN_RDTxR_TIME		GENMASK(31, 16)
> +#define BXCAN_RDTxR_FMI			GENMASK(15, 8)
> +#define BXCAN_RDTxR_DLC			GENMASK(3, 0)
> +
> +enum bxcan_lec_code {
> +	LEC_NO_ERROR = 0,
> +	LEC_STUFF_ERROR,
> +	LEC_FORM_ERROR,
> +	LEC_ACK_ERROR,
> +	LEC_BIT1_ERROR,
> +	LEC_BIT0_ERROR,
> +	LEC_CRC_ERROR,
> +	LEC_UNUSED

Please add BXCAN_ as prefix

> +};
> +
> +/* Structure of the message buffer */
> +struct bxcan_mb {
> +	u32 id;			/* can identifier */
> +	u32 dlc;		/* data length control and timestamp */
> +	u32 data[2];		/* data */
> +};
> +
> +/* Structure of the hardware registers */
> +struct bxcan_regs {
> +	u32 mcr;			/* 0x00 - master control */
> +	u32 msr;			/* 0x04 - master status */
> +	u32 tsr;			/* 0x08 - transmit status */
> +	u32 rf0r;			/* 0x0c - FIFO 0 */
> +	u32 rf1r;			/* 0x10 - FIFO 1 */
> +	u32 ier;			/* 0x14 - interrupt enable */
> +	u32 esr;			/* 0x18 - error status */
> +	u32 btr;			/* 0x1c - bit timing*/
> +	u32 reserved0[88];		/* 0x20 */
> +	struct bxcan_mb tx_mb[BXCAN_TX_MB_NUM];	/* 0x180 - tx mailbox */
> +	struct bxcan_mb rx_mb[2];	/* 0x1b0 - rx mailbox */

Please add a define for the "2", as well.

> +};
> +
> +struct bxcan_priv {
> +	struct can_priv can;
> +	struct device *dev;
> +	struct net_device *ndev;
> +	struct napi_struct napi;

consider using can_rx_offload().

The idea is to read from the mailbox in the IRQ handler and push the
frame into the networking stack in the napi.

> +
> +	struct bxcan_regs __iomem *regs;
> +	int tx_irq;
> +	int sce_irq;
> +	u8 tx_dlc[BXCAN_TX_MB_NUM];

unused

> +	bool master;
> +	struct clk *clk;
> +	unsigned int tx_head;
> +	unsigned int tx_tail;
> +};
> +
> +static const struct can_bittiming_const bxcan_bittiming_const = {
> +	.name = KBUILD_MODNAME,
> +	.tseg1_min = 1,
> +	.tseg1_max = 16,
> +	.tseg2_min = 1,
> +	.tseg2_max = 8,
> +	.sjw_max = 4,
> +	.brp_min = 1,
> +	.brp_max = 1024,
> +	.brp_inc = 1,
> +};
> +
> +static inline u8 bxcan_get_tx_head(const struct bxcan_priv *priv)
> +{
> +	return priv->tx_head % BXCAN_TX_MB_NUM;
> +}
> +
> +static inline u8 bxcan_get_tx_tail(const struct bxcan_priv *priv)
> +{
> +	return priv->tx_tail % BXCAN_TX_MB_NUM;
> +}
> +
> +static inline u8 bxcan_get_tx_free(const struct bxcan_priv *priv)
> +{
> +	return BXCAN_TX_MB_NUM - (priv->tx_head - priv->tx_tail);
> +}
> +
> +static bool bxcan_tx_busy(const struct bxcan_priv *priv)
> +{
> +	if (bxcan_get_tx_free(priv) > 0)
> +		return false;
> +
> +	netif_stop_queue(priv->ndev);
> +
> +	/* Memory barrier before checking tx_free (head and tail) */
> +	smp_mb();
> +
> +	if (bxcan_get_tx_free(priv) == 0) {
> +		netdev_dbg(priv->ndev,
> +			   "Stopping tx-queue (tx_head=0x%08x, tx_tail=0x%08x, len=%d).\n",
> +			   priv->tx_head, priv->tx_tail,
> +			   priv->tx_head - priv->tx_tail);
> +
> +		return true;
> +	}
> +
> +	netif_start_queue(priv->ndev);
> +
> +	return false;
> +}
> +
> +static int bxcan_chip_softreset(struct bxcan_priv *priv)
> +{
> +	struct bxcan_regs __iomem *regs = priv->regs;
> +	unsigned int timeout = BXCAN_TIMEOUT_US / 10;
> +
> +	bxcan_rmw(&regs->mcr, 0, BXCAN_MCR_RESET);
> +	while (timeout-- && !(readl(&regs->msr) & BXCAN_MSR_SLAK))
> +		udelay(10);

make use of readx_poll_timeout(), see:

| https://elixir.bootlin.com/linux/v6.0-rc4/source/drivers/net/can/spi/mcp251x.c#L410

> +
> +	if (!(readl(&regs->msr) & BXCAN_MSR_SLAK))
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}
> +
> +static int bxcan_enter_init_mode(struct bxcan_priv *priv)
> +{
> +	struct bxcan_regs __iomem *regs = priv->regs;
> +	unsigned int timeout = BXCAN_TIMEOUT_US / 10;
> +
> +	bxcan_rmw(&regs->mcr, 0, BXCAN_MCR_INRQ);
> +	while (timeout-- && !(readl(&regs->msr) & BXCAN_MSR_INAK))
> +		udelay(100);

same here

> +
> +	if (!(readl(&regs->msr) & BXCAN_MSR_INAK))
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}
> +
> +static int bxcan_leave_init_mode(struct bxcan_priv *priv)
> +{
> +	struct bxcan_regs __iomem *regs = priv->regs;
> +	unsigned int timeout = BXCAN_TIMEOUT_US / 10;
> +
> +	bxcan_rmw(&regs->mcr, BXCAN_MCR_INRQ, 0);
> +	while (timeout-- && (readl(&regs->msr) & BXCAN_MSR_INAK))
> +		udelay(100);

same

> +
> +	if (readl(&regs->msr) & BXCAN_MSR_INAK)
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}
> +
> +static int bxcan_leave_sleep_mode(struct bxcan_priv *priv)
> +{
> +	struct bxcan_regs __iomem *regs = priv->regs;
> +	unsigned int timeout = BXCAN_TIMEOUT_US / 10;
> +
> +	bxcan_rmw(&regs->mcr, BXCAN_MCR_SLEEP, 0);
> +	while (timeout-- && (readl(&regs->msr) & BXCAN_MSR_SLAK))
> +		udelay(100);

same

> +
> +	if (readl(&regs->msr) & BXCAN_MSR_SLAK)
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}
> +
> +static int bxcan_enter_sleep_mode(struct bxcan_priv *priv)
> +{
> +	struct bxcan_regs __iomem *regs = priv->regs;
> +	unsigned int timeout = BXCAN_TIMEOUT_US / 10;
> +
> +	bxcan_rmw(&regs->mcr, 0, BXCAN_MCR_SLEEP);
> +	while (timeout-- && !(readl(&regs->msr) & BXCAN_MSR_SLAK))
> +		udelay(100);

same

> +
> +	if (!(readl(&regs->msr) & BXCAN_MSR_SLAK))
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}
> +
> +static irqreturn_t bxcan_rx_isr(int irq, void *dev_id)
> +{
> +	struct net_device *ndev = dev_id;
> +	struct bxcan_priv *priv = netdev_priv(ndev);
> +	struct bxcan_regs __iomem *regs = priv->regs;
> +
> +	if (napi_schedule_prep(&priv->napi)) {
> +		/* Disable Rx FIFO message pending interrupt */
> +		bxcan_rmw(&regs->ier, BXCAN_IER_FMPIE0, 0);
> +		__napi_schedule(&priv->napi);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t bxcan_tx_isr(int irq, void *dev_id)
> +{
> +	struct net_device *ndev = dev_id;
> +	struct bxcan_priv *priv = netdev_priv(ndev);
> +	struct bxcan_regs __iomem *regs = priv->regs;
> +	struct net_device_stats *stats = &ndev->stats;
> +	u32 tsr, rqcp_bit, bytes = 0, pkts = 0;
> +	int n, idx;
> +
> +	tsr = readl(&regs->tsr);
> +	for (n = 0, idx = bxcan_get_tx_tail(priv); n < BXCAN_TX_MB_NUM; n++) {

loop from tail to head:

| while (priv->tx_head - priv->tx_tail > 0) {
| ...
|       idx = bxcan_get_tx_tail()
| ...


> +		rqcp_bit = BXCAN_TSR_RQCP0 << (idx << 3);
> +		if (tsr & rqcp_bit) {
> +			pkts++;
> +			bytes += can_get_echo_skb(ndev, idx, NULL);
> +		}
> +
> +		idx += 1;
> +		if (idx == BXCAN_TX_MB_NUM)
> +			idx = 0;
> +	}
> +
> +	if (!pkts)
> +		return IRQ_HANDLED;

This looks wrong. Better check if any relevant bits in tsr are set, if
not return IRQ_NONE.

> +
> +	writel(tsr, &regs->tsr);
> +
> +	priv->tx_tail += pkts;
> +	if (bxcan_get_tx_free(priv)) {
> +		/* Make sure that anybody stopping the queue after
> +		 * this sees the new tx_ring->tail.
> +		 */
> +		smp_mb();
> +		netif_wake_queue(ndev);
> +	}
> +
> +	stats->tx_bytes += bytes;
> +	stats->tx_packets += pkts;
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void bxcan_handle_state_change(struct net_device *ndev, u32 esr)
> +{
> +	struct bxcan_priv *priv = netdev_priv(ndev);
> +	struct net_device_stats *stats = &ndev->stats;
> +	enum can_state new_state = priv->can.state;
> +	struct can_berr_counter bec;
> +	enum can_state rx_state, tx_state;
> +	struct sk_buff *skb;
> +	struct can_frame *cf;
> +
> +	/* Early exit if no error flag is set */
> +	if (!(esr & (BXCAN_ESR_EWGF | BXCAN_ESR_EPVF | BXCAN_ESR_BOFF)))
> +		return;
> +
> +	bec.txerr = BXCAN_TEC(esr);
> +	bec.rxerr = BXCAN_REC(esr);
> +
> +	if (esr & BXCAN_ESR_BOFF)
> +		new_state = CAN_STATE_BUS_OFF;
> +	else if (esr & BXCAN_ESR_EPVF)
> +		new_state = CAN_STATE_ERROR_PASSIVE;
> +	else if (esr & BXCAN_ESR_EWGF)
> +		new_state = CAN_STATE_ERROR_WARNING;
> +
> +	/* state hasn't changed */
> +	if (unlikely(new_state == priv->can.state))
> +		return;
> +
> +	skb = alloc_can_err_skb(ndev, &cf);
> +	if (unlikely(!skb))
> +		return;

Continue, even if you got no skb.

> +
> +	tx_state = bec.txerr >= bec.rxerr ? new_state : 0;
> +	rx_state = bec.txerr <= bec.rxerr ? new_state : 0;
> +	can_change_state(ndev, cf, tx_state, rx_state);

can_change_state() handles cf == NULL....

> +
> +	if (new_state == CAN_STATE_BUS_OFF)
> +		can_bus_off(ndev);
> +
> +	stats->rx_bytes += cf->len;
> +	stats->rx_packets++;

no statistics for error frames.

> +	netif_rx(skb);
> +}
> +
> +static void bxcan_handle_bus_err(struct net_device *ndev, u32 esr)
> +{
> +	struct bxcan_priv *priv = netdev_priv(ndev);
> +	enum bxcan_lec_code lec_code;
> +	struct can_frame *cf;
> +	struct sk_buff *skb;
> +
> +	lec_code = (esr & BXCAN_ESR_LEC_SHIFT) >> BXCAN_ESR_LEC_SHIFT;

Use FIELD_GET()

> +
> +	/* Early exit if no lec update or no error.
> +	 * No lec update means that no CAN bus event has been detected
> +	 * since CPU wrote LEC_UNUSED value to status reg.
> +	 */
> +	if (lec_code == LEC_UNUSED || lec_code == LEC_NO_ERROR)
> +		return;
> +
> +	if (!(priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING))
> +		return;

Can you disable the generation of the bus error interrupt?

> +
> +	/* Common for all type of bus errors */
> +	priv->can.can_stats.bus_error++;
> +
> +	/* Propagate the error condition to the CAN stack */
> +	skb = alloc_can_err_skb(ndev, &cf);
> +	if (unlikely(!skb))
> +		return;

continue, even without skb

> +
> +	ndev->stats.rx_bytes += cf->len;

no stats

> +
> +	/* Check for 'last error code' which tells us the
> +	 * type of the last error to occur on the CAN bus
> +	 */
> +	cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> +
> +	switch (lec_code) {
> +	case LEC_STUFF_ERROR:
> +		netdev_dbg(ndev, "Stuff error\n");
> +		ndev->stats.rx_errors++;
> +		cf->data[2] |= CAN_ERR_PROT_STUFF;
> +		break;
> +	case LEC_FORM_ERROR:
> +		netdev_dbg(ndev, "Form error\n");
> +		ndev->stats.rx_errors++;
> +		cf->data[2] |= CAN_ERR_PROT_FORM;
> +		break;
> +	case LEC_ACK_ERROR:
> +		netdev_dbg(ndev, "Ack error\n");
> +		ndev->stats.tx_errors++;
> +		cf->data[3] = CAN_ERR_PROT_LOC_ACK;
> +		break;
> +	case LEC_BIT1_ERROR:
> +		netdev_dbg(ndev, "Bit error (recessive)\n");
> +		ndev->stats.tx_errors++;
> +		cf->data[2] |= CAN_ERR_PROT_BIT1;
> +		break;
> +	case LEC_BIT0_ERROR:
> +		netdev_dbg(ndev, "Bit error (dominant)\n");
> +		ndev->stats.tx_errors++;
> +		cf->data[2] |= CAN_ERR_PROT_BIT0;
> +		break;
> +	case LEC_CRC_ERROR:
> +		netdev_dbg(ndev, "CRC error\n");
> +		ndev->stats.rx_errors++;
> +		cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	netif_rx(skb);
> +}
> +
> +static irqreturn_t bxcan_state_change_isr(int irq, void *dev_id)
> +{
> +	struct net_device *ndev = dev_id;
> +	struct bxcan_priv *priv = netdev_priv(ndev);
> +	struct bxcan_regs __iomem *regs = priv->regs;
> +	u32 msr, esr;
> +
> +	msr = readl(&regs->msr);
> +	if (!(msr & BXCAN_MSR_ERRI))
> +		return IRQ_NONE;
> +
> +	esr = readl(&regs->esr);
> +	bxcan_handle_state_change(ndev, esr);
> +	bxcan_handle_bus_err(ndev, esr);
> +
> +	msr |= BXCAN_MSR_ERRI;
> +	writel(msr, &regs->msr);

nitpick: please add an empty line in front of return

> +	return IRQ_HANDLED;
> +}
> +
> +static int bxcan_start(struct net_device *ndev)

please rename to bxcan_chip_start()

> +{
> +	struct bxcan_priv *priv = netdev_priv(ndev);
> +	struct bxcan_regs __iomem *regs = priv->regs;
> +	struct can_bittiming *bt = &priv->can.bittiming;
> +	u32 set;
> +	int err;
> +
> +	err = bxcan_chip_softreset(priv);
> +	if (err) {
> +		netdev_err(ndev, "failed to reset chip, error %d\n", err);
> +		return err;
> +	}
> +
> +	err = bxcan_leave_sleep_mode(priv);
> +	if (err) {
> +		netdev_err(ndev, "failed to leave sleep mode, error %d\n", err);
> +		goto failed_leave_sleep;
> +	}
> +
> +	err = bxcan_enter_init_mode(priv);
> +	if (err) {
> +		netdev_err(ndev, "failed to enter init mode, error %d\n", err);
> +		goto failed_enter_init;
> +	}
> +
> +	/* MCR
> +	 *
> +	 * select request order priority
> +	 * disable time triggered mode
> +	 * bus-off state left on sw request
> +	 * sleep mode left on sw request
> +	 * retransmit automatically on error
> +	 * do not lock RX FIFO on overrun
> +	 */
> +	bxcan_rmw(&regs->mcr, BXCAN_MCR_TTCM | BXCAN_MCR_ABOM | BXCAN_MCR_AWUM |
> +		  BXCAN_MCR_NART | BXCAN_MCR_RFLM, BXCAN_MCR_TXFP);
> +
> +	/* Bit timing register settings */
> +	set = BXCAN_BTR_BRP(bt->brp - 1) |
> +		BXCAN_BTR_TS1(bt->phase_seg1 + bt->prop_seg - 1) |
> +		BXCAN_BTR_TS2(bt->phase_seg2 - 1) | BXCAN_BTR_SJW(bt->sjw - 1);
> +
> +	/* loopback + silent mode put the controller in test mode,
> +	 * useful for hot self-test
> +	 */
> +	if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
> +		set |= BXCAN_BTR_LBKM;
> +
> +	if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> +		set |= BXCAN_BTR_SILM;
> +
> +	netdev_dbg(ndev,
> +		   "TQ[ns]: %d, PrS: %d, PhS1: %d, PhS2: %d, SJW: %d, BRP: %d, CAN_BTR: 0x%08x\n",
> +		   bt->tq, bt->prop_seg, bt->phase_seg1, bt->phase_seg2,
> +		   bt->sjw, bt->brp, set);
> +	bxcan_rmw(&regs->btr, BXCAN_BTR_SILM | BXCAN_BTR_LBKM |
> +		  BXCAN_BTR_BRP_MASK | BXCAN_BTR_TS1_MASK | BXCAN_BTR_TS2_MASK |
> +		  BXCAN_BTR_SJW_MASK, set);
> +
> +	bxcan_enable_filters(priv->dev->parent, priv->master);
> +
> +	/* Clear all internal status */
> +	priv->tx_head = 0;
> +	priv->tx_tail = 0;
> +
> +	err = bxcan_leave_init_mode(priv);
> +	if (err) {
> +		netdev_err(ndev, "failed to leave init mode, error %d\n", err);
> +		goto failed_leave_init;
> +	}
> +
> +	/* Set a `lec` value so that we can check for updates later */
> +	bxcan_rmw(&regs->esr, BXCAN_ESR_LEC, LEC_UNUSED << BXCAN_ESR_LEC_SHIFT);
> +
> +	/* IER
> +	 *
> +	 * Enable interrupt for:
> +	 * bus-off
> +	 * passive error
> +	 * warning error
> +	 * last error code
> +	 * RX FIFO pending message
> +	 * TX mailbox empty
> +	 */
> +	bxcan_rmw(&regs->ier, BXCAN_IER_WKUIE | BXCAN_IER_SLKIE |
> +		  BXCAN_IER_FOVIE1 | BXCAN_IER_FFIE1 | BXCAN_IER_FMPIE1 |
> +		  BXCAN_IER_FOVIE0 | BXCAN_IER_FFIE0,
> +		  BXCAN_IER_ERRIE | BXCAN_IER_LECIE | BXCAN_IER_BOFIE |
> +		  BXCAN_IER_EPVIE | BXCAN_IER_EWGIE | BXCAN_IER_FMPIE0 |
> +		  BXCAN_IER_TMEIE);
> +
> +	priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +	return 0;
> +
> +failed_leave_init:
> +failed_enter_init:
> +failed_leave_sleep:
> +	bxcan_chip_softreset(priv);
> +	return err;
> +}
> +
> +static int bxcan_open(struct net_device *ndev)
> +{
> +	struct bxcan_priv *priv = netdev_priv(ndev);
> +	int err;
> +
> +	err = open_candev(ndev);
> +	if (err) {
> +		netdev_err(ndev, "open_candev() failed, error %d\n", err);
> +		goto failed_open;
> +	}
> +
> +	napi_enable(&priv->napi);
> +	err = request_irq(ndev->irq, bxcan_rx_isr, IRQF_SHARED, ndev->name,
> +			  ndev);
> +	if (err) {
> +		netdev_err(ndev, "failed to register rx irq(%d), error %d\n",
> +			   ndev->irq, err);
> +		goto failed_rx_irq_request;
> +	}
> +
> +	err = request_irq(priv->tx_irq, bxcan_tx_isr, IRQF_SHARED, ndev->name,
> +			  ndev);
> +	if (err) {
> +		netdev_err(ndev, "failed to register tx irq(%d), error %d\n",
> +			   priv->tx_irq, err);
> +		goto failed_tx_irq_request;
> +	}
> +
> +	err = request_irq(priv->sce_irq, bxcan_state_change_isr, IRQF_SHARED,
> +			  ndev->name, ndev);
> +	if (err) {
> +		netdev_err(ndev, "failed to register sce irq(%d), error %d\n",
> +			   priv->sce_irq, err);
> +		goto failed_sce_irq_request;
> +	}
> +
> +	err = bxcan_start(ndev);
> +	if (err)
> +		goto failed_start;
> +
> +	netif_start_queue(ndev);
> +	return 0;
> +
> +failed_start:
> +failed_sce_irq_request:
> +	free_irq(priv->tx_irq, ndev);
> +failed_tx_irq_request:
> +	free_irq(ndev->irq, ndev);
> +failed_rx_irq_request:
> +	napi_disable(&priv->napi);
> +	close_candev(ndev);
> +failed_open:
> +	return err;
> +}
> +
> +static void bxcan_stop(struct net_device *ndev)

please rename to bxcan_chip_stop()
> +{
> +	struct bxcan_priv *priv = netdev_priv(ndev);
> +
> +	bxcan_disable_filters(priv->dev->parent, priv->master);

do you have to disable the IRQs?

> +	bxcan_enter_sleep_mode(priv);
> +	priv->can.state = CAN_STATE_STOPPED;
> +}
> +
> +static int bxcan_close(struct net_device *ndev)

and this bxcan_stop() to match the .ndo_stop.

> +{
> +	struct bxcan_priv *priv = netdev_priv(ndev);
> +
> +	netif_stop_queue(ndev);
> +	bxcan_stop(ndev);
> +	free_irq(ndev->irq, ndev);
> +	free_irq(priv->tx_irq, ndev);
> +	free_irq(priv->sce_irq, ndev);
> +	napi_disable(&priv->napi);
> +	close_candev(ndev);
> +	return 0;
> +}
> +
> +static netdev_tx_t bxcan_start_xmit(struct sk_buff *skb,
> +				    struct net_device *ndev)
> +{
> +	struct bxcan_priv *priv = netdev_priv(ndev);
> +	struct can_frame *cf = (struct can_frame *)skb->data;
> +	struct bxcan_regs __iomem *regs = priv->regs;
> +	struct bxcan_mb __iomem *mb_regs;
> +	unsigned int idx;
> +	u32 id;
> +	int i, j;
> +
> +	if (can_dropped_invalid_skb(ndev, skb))
> +		return NETDEV_TX_OK;
> +
> +	if (bxcan_tx_busy(priv))
> +		return NETDEV_TX_BUSY;
> +
> +	idx = bxcan_get_tx_head(priv);
> +	priv->tx_head++;
> +	if (bxcan_get_tx_free(priv) == 0)
> +		netif_stop_queue(ndev);
> +
> +	mb_regs = &regs->tx_mb[idx];
> +	if (cf->can_id & CAN_EFF_FLAG)
> +		id = BXCAN_TIxR_EXID(cf->can_id & CAN_EFF_MASK) |
> +			BXCAN_TIxR_IDE;
> +	else
> +		id = BXCAN_TIxR_STID(cf->can_id & CAN_SFF_MASK);
> +
> +	if (cf->can_id & CAN_RTR_FLAG)
> +		id |= BXCAN_TIxR_RTR;
> +
> +	bxcan_rmw(&mb_regs->dlc, BXCAN_TDTxR_DLC_MASK,
> +		  BXCAN_TDTxR_DLC(cf->len));

Is rmw needed here, or does a pure write work, too?

> +	can_put_echo_skb(skb, ndev, idx, 0);
> +
> +	for (i = 0, j = 0; i < cf->len; i += 4, j++)
> +		writel(*(u32 *)(cf->data + i), &mb_regs->data[j]);
> +
> +	/* Start transmission */
> +	writel(id | BXCAN_TIxR_TXRQ, &mb_regs->id);
> +
> +	return NETDEV_TX_OK;
> +}
> +
> +static const struct net_device_ops bxcan_netdev_ops = {
> +	.ndo_open = bxcan_open,
> +	.ndo_stop = bxcan_close,
> +	.ndo_start_xmit = bxcan_start_xmit,
> +	.ndo_change_mtu = can_change_mtu,
> +};
> +
> +static void bxcan_rx_pkt(struct net_device *ndev,
> +			 struct bxcan_mb __iomem *mb_regs)
> +{
> +	struct net_device_stats *stats = &ndev->stats;
> +	struct can_frame *cf;
> +	struct sk_buff *skb;
> +	u32 id, dlc;
> +
> +	skb = alloc_can_skb(ndev, &cf);
> +	if (!skb) {
> +		stats->rx_dropped++;
> +		return;
> +	}
> +
> +	id = readl(&mb_regs->id);
> +	if (id & BXCAN_RIxR_IDE)
> +		cf->can_id = (id >> BXCAN_RIxR_EXID_SHIFT) | CAN_EFF_FLAG;
> +	else
> +		cf->can_id = (id >> BXCAN_RIxR_STID_SHIFT) & CAN_SFF_MASK;
> +
> +	dlc = readl(&mb_regs->dlc) & BXCAN_RDTxR_DLC;
> +	cf->len = can_cc_dlc2len(dlc);
> +
> +	if (id & BXCAN_RIxR_RTR) {
> +		cf->can_id |= CAN_RTR_FLAG;
> +	} else {
> +		int i, j;
> +
> +		for (i = 0, j = 0; i < cf->len; i += 4, j++)
> +			*(u32 *)(cf->data + i) = readl(&mb_regs->data[j]);
> +
> +		stats->rx_bytes += cf->len;
> +	}
> +
> +	stats->rx_packets++;
> +	netif_receive_skb(skb);
> +}
> +
> +static int bxcan_rx_poll(struct napi_struct *napi, int quota)
> +{
> +	struct net_device *ndev = napi->dev;
> +	struct bxcan_priv *priv = netdev_priv(ndev);
> +	struct bxcan_regs __iomem *regs = priv->regs;
> +	int num_pkts;
> +	u32 rf0r;
> +
> +	for (num_pkts = 0; num_pkts < quota; num_pkts++) {
> +		rf0r = readl(&regs->rf0r);
> +		if (!(rf0r & BXCAN_RF0R_FMP0))
> +			break;
> +
> +		bxcan_rx_pkt(ndev, &regs->rx_mb[0]);
> +
> +		rf0r |= BXCAN_RF0R_RFOM0;
> +		writel(rf0r, &regs->rf0r);
> +	}
> +
> +	if (num_pkts < quota) {
> +		napi_complete_done(napi, num_pkts);
> +		bxcan_rmw(&regs->ier, 0, BXCAN_IER_FMPIE0);
> +	}
> +
> +	return num_pkts;
> +}
> +
> +static int bxcan_do_set_mode(struct net_device *ndev, enum can_mode mode)
> +{
> +	int err;
> +
> +	switch (mode) {
> +	case CAN_MODE_START:
> +		err = bxcan_start(ndev);
> +		if (err)
> +			return err;
> +
> +		netif_wake_queue(ndev);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static int bxcan_disable_clks(struct bxcan_priv *priv)
> +{
> +	if (priv->clk)
> +		clk_disable_unprepare(priv->clk);
> +
> +	bxcan_disable_master_clk(priv->dev->parent);
> +	return 0;
> +}
> +
> +static int bxcan_enable_clks(struct bxcan_priv *priv)
> +{
> +	int err;
> +
> +	err = bxcan_enable_master_clk(priv->dev->parent);
> +	if (err)
> +		return err;
> +
> +	if (priv->clk) {
> +		err = clk_prepare_enable(priv->clk);

clk_prepare_enable() works on NULL pointer, too.

> +		if (err)
> +			bxcan_disable_master_clk(priv->dev->parent);
> +	}
> +
> +	return err;
> +}
> +
> +static int bxcan_get_berr_counter(const struct net_device *ndev,
> +				  struct can_berr_counter *bec)
> +{
> +	struct bxcan_priv *priv = netdev_priv(ndev);
> +	struct bxcan_regs __iomem *regs = priv->regs;
> +	u32 esr;
> +	int err;
> +
> +	err = bxcan_enable_clks(priv);
> +	if (err)
> +		return err;
> +
> +	esr = readl(&regs->esr);
> +	bec->txerr = BXCAN_TEC(esr);
> +	bec->rxerr = BXCAN_REC(esr);
> +	err = bxcan_disable_clks(priv);
> +	return 0;
> +}
> +
> +static int bxcan_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct device *dev = &pdev->dev;
> +	struct net_device *ndev;
> +	struct bxcan_priv *priv;
> +	struct clk *clk = NULL;
> +	bool master;
> +	u32 offset;
> +	int err, rx_irq, tx_irq, sce_irq;
> +
> +	master = of_property_read_bool(np, "st,can-master");
> +	if (!master) {
> +		clk = devm_clk_get(dev, NULL);
> +		if (IS_ERR(clk)) {
> +			dev_err(dev, "failed to get clock\n");
> +			return PTR_ERR(clk);
> +		}
> +	}
> +
> +	rx_irq = platform_get_irq_byname(pdev, "rx0");
> +	if (rx_irq < 0) {
> +		dev_err(dev, "failed to get rx0 irq\n");
> +		return rx_irq;
> +	}
> +
> +	tx_irq = platform_get_irq_byname(pdev, "tx");
> +	if (tx_irq < 0) {
> +		dev_err(dev, "failed to get tx irq\n");
> +		return tx_irq;
> +	}
> +
> +	sce_irq = platform_get_irq_byname(pdev, "sce");
> +	if (sce_irq < 0) {
> +		dev_err(dev, "failed to get sce irq\n");
> +		return sce_irq;
> +	}
> +
> +	err = of_property_read_u32(np, "reg", &offset);
> +	if (err) {
> +		dev_err(dev, "failed to get reg property\n");
> +		return err;
> +	}
> +
> +	ndev = alloc_candev(sizeof(struct bxcan_priv), BXCAN_TX_MB_NUM);
> +	if (!ndev) {
> +		dev_err(dev, "alloc_candev() failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	priv = netdev_priv(ndev);
> +	platform_set_drvdata(pdev, ndev);
> +	SET_NETDEV_DEV(ndev, dev);
> +	ndev->netdev_ops = &bxcan_netdev_ops;
> +	ndev->irq = rx_irq;
> +	ndev->flags |= IFF_ECHO;
> +
> +	priv->dev = dev;
> +	priv->ndev = ndev;
> +	priv->regs = bxcan_get_base_addr(dev->parent) + offset;
> +	priv->clk = clk;
> +	priv->tx_irq = tx_irq;
> +	priv->sce_irq = sce_irq;
> +	priv->master = master;
> +	priv->can.clock.freq =
> +		master ? bxcan_get_master_clk_rate(dev->parent) :
> +		clk_get_rate(clk);
> +	priv->tx_head = 0;
> +	priv->tx_tail = 0;
> +	priv->can.bittiming_const = &bxcan_bittiming_const;
> +	priv->can.do_set_mode = bxcan_do_set_mode;
> +	priv->can.do_get_berr_counter = bxcan_get_berr_counter;
> +	priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
> +		CAN_CTRLMODE_LISTENONLY	| CAN_CTRLMODE_BERR_REPORTING;
> +	netif_napi_add(ndev, &priv->napi, bxcan_rx_poll, BXCAN_NAPI_WEIGHT);
> +
> +	err = bxcan_enable_clks(priv);
> +	if (err) {
> +		dev_err(dev, "failed to enable clocks\n");
> +		return err;
> +	}
> +
> +	err = register_candev(ndev);
> +	if (err) {
> +		dev_err(dev, "failed to register netdev\n");
> +		goto failed_register;
> +	}
> +
> +	dev_info(dev, "clk: %d Hz, IRQs: %d, %d, %d\n", priv->can.clock.freq,
> +		 tx_irq, rx_irq, sce_irq);
> +	return 0;
> +
> +failed_register:
> +	netif_napi_del(&priv->napi);
> +	free_candev(ndev);
> +	return err;
> +}
> +
> +static int bxcan_remove(struct platform_device *pdev)
> +{
> +	struct net_device *ndev = platform_get_drvdata(pdev);
> +	struct bxcan_priv *priv = netdev_priv(ndev);
> +
> +	unregister_candev(ndev);
> +	bxcan_disable_clks(priv);
> +	netif_napi_del(&priv->napi);
> +	free_candev(ndev);
> +	return 0;
> +}
> +
> +static int __maybe_unused bxcan_suspend(struct device *dev)
> +{
> +	struct net_device *ndev = dev_get_drvdata(dev);
> +	struct bxcan_priv *priv = netdev_priv(ndev);
> +
> +	if (!netif_running(ndev))
> +		return 0;
> +
> +	netif_stop_queue(ndev);
> +	netif_device_detach(ndev);
> +
> +	bxcan_enter_sleep_mode(priv);
> +	priv->can.state = CAN_STATE_SLEEPING;
> +	bxcan_disable_clks(priv);
> +	return 0;
> +}
> +
> +static int __maybe_unused bxcan_resume(struct device *dev)
> +{
> +	struct net_device *ndev = dev_get_drvdata(dev);
> +	struct bxcan_priv *priv = netdev_priv(ndev);
> +
> +	if (!netif_running(ndev))
> +		return 0;
> +
> +	bxcan_enable_clks(priv);
> +	bxcan_leave_sleep_mode(priv);
> +	priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +
> +	netif_device_attach(ndev);
> +	netif_start_queue(ndev);
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(bxcan_pm_ops, bxcan_suspend, bxcan_resume);
> +
> +static const struct of_device_id bxcan_of_match[] = {
> +	{.compatible = "st,stm32f4-bxcan"},
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, bxcan_of_match);
> +
> +static struct platform_driver bxcan_driver = {
> +	.driver = {
> +		.name = KBUILD_MODNAME,
> +		.pm = &bxcan_pm_ops,
> +		.of_match_table = bxcan_of_match,
> +	},
> +	.probe = bxcan_probe,
> +	.remove = bxcan_remove,
> +};
> +
> +module_platform_driver(bxcan_driver);
> +
> +MODULE_AUTHOR("Dario Binacchi <dario.binacchi@amarulasolutions.com>");
> +MODULE_DESCRIPTION("STMicroelectronics Basic Extended CAN controller driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.32.0
> 
> 

Marc

On Tue, Sep 6, 2022 at 12:40 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 28.08.2022 15:33:29, Dario Binacchi wrote:
> > Add support for the basic extended CAN controller (bxCAN) found in many
> > low- to middle-end STM32 SoCs. It supports the Basic Extended CAN
> > protocol versions 2.0A and B with a maximum bit rate of 1 Mbit/s.
> >
> > The controller supports two channels (CAN1 as master and CAN2 as slave)
> > and the driver can enable either or both of the channels. They share
> > some of the required logic (e. g. clocks and filters), and that means
> > you cannot use the slave CAN without enabling some hardware resources
> > managed by the master CAN.
>
> I don't like the concept of the core driver. E.g. the r-m-w of shared
> registers bxcan_enable_filters() is racy by design.
>
> How is the memory layout?
> - 0x000 - 0x1ff: CAN1
> - 0x200 - 0x3ff: shared mem
> - 0x400 - 0x5ff: CAN2
>

Yes, but shared mem can be accessed by CAN2 only if CAN1 is enabled.
For example, if only CAN2 is used, the clock of CAN1 must still be enabled.

> Better model the shared mem as a "syscon" node:
>
> | https://elixir.bootlin.com/linux/v6.0-rc4/source/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c#L1445
>
> Then you can use regmap_*() functions to access the register space. The
> regmap functions are always locked against each other.

Ok, I will use "syscon".
The device tree description also raised more than one question for the
shared memory description
by the reviewers.
Following your suggestions I think the device tree will also be improved.

>
> [...]
>
> > diff --git a/drivers/net/can/bxcan/bxcan-core.c b/drivers/net/can/bxcan/bxcan-core.c
> > new file mode 100644
> > index 000000000000..3644449095e9
> > --- /dev/null
> > +++ b/drivers/net/can/bxcan/bxcan-core.c
> > @@ -0,0 +1,200 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* bxcan-core.c - STM32 Basic Extended CAN core controller driver
> > + *
> > + * This file is part of STM32 bxcan driver
> > + *
> > + * Copyright (c) 2022 Dario Binacchi <dario.binacchi@amarulasolutions.com>
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/clk.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +
> > +#include "bxcan-core.h"
> > +
> > +#define BXCAN_FILTER_ID(master)       (master ? 0 : 14)
> > +
> > +/* Filter master register (FMR) bits */
> > +#define BXCAN_FMR_CANSB_MASK GENMASK(13, 8)
> > +#define BXCAN_FMR_CANSB(x)   (((x) & 0x3f) << 8)
> > +#define BXCAN_FMR_FINIT              BIT(0)
>
> nitpick:
> please use 1 space instead of tabs
>
> > +
> > +/* Structure of the filter bank */
> > +struct bxcan_fb {
> > +     u32 fr1;                /* filter 1 */
> > +     u32 fr2;                /* filter 2 */
> > +};
> > +
> > +/* Structure of the hardware filter registers */
> > +struct bxcan_fregs {
> > +     u32 fmr;                /* 0x00 - filter master */
> > +     u32 fm1r;               /* 0x04 - filter mode */
> > +     u32 reserved2;          /* 0x08 */
> > +     u32 fs1r;               /* 0x0c - filter scale */
> > +     u32 reserved3;          /* 0x10 */
> > +     u32 ffa1r;              /* 0x14 - filter FIFO assignment */
> > +     u32 reserved4;          /* 0x18 */
> > +     u32 fa1r;               /* 0x1c - filter activation */
> > +     u32 reserved5[8];       /* 0x20 */
> > +     struct bxcan_fb fb[28]; /* 0x40 - filter banks */
> > +};
> > +
> > +struct bxcan_core_priv {
> > +     void __iomem *base;
> > +     struct bxcan_fregs __iomem *fregs;
> > +     struct clk *clk_master;
> > +     unsigned int clk_master_ref;
> > +};
> > +
> > +void bxcan_disable_filters(struct device *dev, bool master)
> > +{
> > +     struct bxcan_core_priv *priv = dev_get_drvdata(dev);
> > +     unsigned int fid = BXCAN_FILTER_ID(master);
> > +     u32 fmask = BIT(fid);
> > +
> > +     bxcan_rmw(&priv->fregs->fa1r, fmask, 0);
>
> This is racy, same for bxcan_rmw() on other shared registers.
>
> > +}
> > +
> > +void bxcan_enable_filters(struct device *dev, bool master)
> > +{
> > +     struct bxcan_core_priv *priv = dev_get_drvdata(dev);
> > +     unsigned int fid = BXCAN_FILTER_ID(master);
> > +     u32 fmask = BIT(fid);
> > +
> > +     /* Filter settings:
> > +      *
> > +      * Accept all messages.
> > +      * Assign filter 0 to CAN1 and filter 14 to CAN2 in identifier
> > +      * mask mode with 32 bits width.
> > +      */
> > +
> > +     /* Enter filter initialization mode and assing filters to CAN
> > +      * controllers.
> > +      */
> > +     bxcan_rmw(&priv->fregs->fmr, BXCAN_FMR_CANSB_MASK,
> > +               BXCAN_FMR_CANSB(14) | BXCAN_FMR_FINIT);
> > +
> > +     /* Deactivate filter */
> > +     bxcan_rmw(&priv->fregs->fa1r, fmask, 0);
> > +
> > +     /* Two 32-bit registers in identifier mask mode */
> > +     bxcan_rmw(&priv->fregs->fm1r, fmask, 0);
> > +
> > +     /* Single 32-bit scale configuration */
> > +     bxcan_rmw(&priv->fregs->fs1r, 0, fmask);
> > +
> > +     /* Assign filter to FIFO 0 */
> > +     bxcan_rmw(&priv->fregs->ffa1r, fmask, 0);
> > +
> > +     /* Accept all messages */
> > +     writel(0, &priv->fregs->fb[fid].fr1);
> > +     writel(0, &priv->fregs->fb[fid].fr2);
> > +
> > +     /* Activate filter */
> > +     bxcan_rmw(&priv->fregs->fa1r, 0, fmask);
> > +
> > +     /* Exit filter initialization mode */
> > +     bxcan_rmw(&priv->fregs->fmr, BXCAN_FMR_FINIT, 0);
> > +}
> > +
> > +int bxcan_enable_master_clk(struct device *dev)
> > +{
> > +     struct bxcan_core_priv *priv = dev_get_drvdata(dev);
> > +     int err;
> > +
> > +     if (priv->clk_master_ref == 0) {
> > +             err = clk_prepare_enable(priv->clk_master);
> > +             if (err)
> > +                     return err;
> > +     }
> > +
> > +     priv->clk_master_ref++;
>
> In general, don't do clock ref-counting. You can enable and disable the
> same clock several times.

I had used ref-counting to avoid disabling the clock of CAN1 with CAN2 enabled
and thus losing access to the shared memory.

>
> If you convert to syscon, you can add a clock node to the syscon node
> and the syscon driver will take care of the clocks.

I hope that syscon will allow me to guarantee the enabling of the CAN1
clock if CAN1 is disabled
and CAN2 enabled.

>
> > +     return 0;
> > +}
> > +
> > +void bxcan_disable_master_clk(struct device *dev)
> > +{
> > +     struct bxcan_core_priv *priv = dev_get_drvdata(dev);
> > +
> > +     if (priv->clk_master_ref == 0)
> > +             return;
> > +
> > +     if (priv->clk_master_ref == 1)
> > +             clk_disable_unprepare(priv->clk_master);
> > +
> > +     priv->clk_master_ref--;
> > +}
> > +
> > +unsigned long bxcan_get_master_clk_rate(struct device *dev)
> > +{
> > +     struct bxcan_core_priv *priv = dev_get_drvdata(dev);
> > +
> > +     return clk_get_rate(priv->clk_master);
> > +}
> > +
> > +void __iomem *bxcan_get_base_addr(struct device *dev)
> > +{
> > +     struct bxcan_core_priv *priv = dev_get_drvdata(dev);
> > +
> > +     return priv->base;
> > +}
> > +
> > +static const struct of_device_id bxcan_core_of_match[] = {
> > +     {.compatible = "st,stm32f4-bxcan-core"},
> > +     { /* sentinel */ },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, bxcan_core_of_match);
> > +
> > +static int bxcan_core_probe(struct platform_device *pdev)
> > +{
> > +     struct bxcan_core_priv *priv;
> > +     struct device *dev = &pdev->dev;
> > +     struct device_node *np = pdev->dev.of_node;
> > +     void __iomem *regs;
> > +     struct clk *clk;
> > +     int ret;
> > +
> > +     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +     if (!priv)
> > +             return -ENOMEM;
> > +
> > +     platform_set_drvdata(pdev, priv);
> > +     regs = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(regs))
> > +             return PTR_ERR(regs);
> > +
> > +     clk = devm_clk_get(&pdev->dev, NULL);
> > +     if (IS_ERR(clk)) {
> > +             dev_err(dev, "failed to get clock\n");
> > +             return PTR_ERR(clk);
> > +     }
> > +
> > +     priv->base = regs;
> > +     priv->fregs = regs + 0x200;
> > +     priv->clk_master = clk;
> > +
> > +     ret = of_platform_populate(np, NULL, NULL, dev);
> > +     if (ret < 0) {
> > +             dev_err(dev, "failed to populate DT children\n");
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static struct platform_driver bxcan_core_driver = {
> > +     .driver = {
> > +             .name = KBUILD_MODNAME,
> > +             .of_match_table = bxcan_core_of_match,
> > +     },
> > +     .probe = bxcan_core_probe,
> > +};
> > +
> > +module_platform_driver(bxcan_core_driver);
> > +
> > +MODULE_AUTHOR("Dario Binacchi <dario.binacchi@amarulasolutions.com>");
> > +MODULE_DESCRIPTION("STMicroelectronics Basic Extended CAN core driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/net/can/bxcan/bxcan-core.h b/drivers/net/can/bxcan/bxcan-core.h
> > new file mode 100644
> > index 000000000000..925449cbc3bd
> > --- /dev/null
> > +++ b/drivers/net/can/bxcan/bxcan-core.h
> > @@ -0,0 +1,31 @@
> > +/* SPDX-License-Identifier: GPL-2.0
> > + *
> > + * bxcan-core - STM32 Basic Extended CAN core controller driver
> > + *
> > + * Copyright (c) 2022 Dario Binacchi <dario.binacchi@amarulasolutions.com>
> > + */
> > +
> > +#ifndef __BXCAN_CORE_H
> > +#define __BXCAN_CORE_H
> > +
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +
> > +int bxcan_enable_master_clk(struct device *dev);
> > +void bxcan_disable_master_clk(struct device *dev);
> > +unsigned long bxcan_get_master_clk_rate(struct device *dev);
> > +void __iomem *bxcan_get_base_addr(struct device *dev);
> > +void bxcan_enable_filters(struct device *dev, bool master);
> > +void bxcan_disable_filters(struct device *dev, bool master);
> > +
> > +static inline void bxcan_rmw(void __iomem *addr, u32 clear, u32 set)
> > +{
> > +     u32 old, val;
> > +
> > +     old = readl(addr);
> > +     val = (old & ~clear) | set;
> > +     if (val != old)
> > +             writel(val, addr);
> > +}
> > +
> > +#endif
> > diff --git a/drivers/net/can/bxcan/bxcan-drv.c b/drivers/net/can/bxcan/bxcan-drv.c
> > new file mode 100644
> > index 000000000000..7ac1ee7a4269
> > --- /dev/null
> > +++ b/drivers/net/can/bxcan/bxcan-drv.c
> > @@ -0,0 +1,1045 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// bxcan-drv.c - STM32 Basic Extended CAN controller driver
> > +//
> > +// Copyright (c) 2022 Dario Binacchi <dario.binacchi@amarulasolutions.com>
> > +//
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/can.h>
> > +#include <linux/can/dev.h>
> > +#include <linux/can/error.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include "bxcan-core.h"
> > +
> > +#define BXCAN_NAPI_WEIGHT            3
> > +#define BXCAN_TIMEOUT_US             10000
> > +
> > +#define BXCAN_TX_MB_NUM                      3
> > +
> > +/* Master control register (MCR) bits */
> > +#define BXCAN_MCR_DBF                        BIT(16)
> > +#define BXCAN_MCR_RESET                      BIT(15)
> > +#define BXCAN_MCR_TTCM                       BIT(7)
> > +#define BXCAN_MCR_ABOM                       BIT(6)
> > +#define BXCAN_MCR_AWUM                       BIT(5)
> > +#define BXCAN_MCR_NART                       BIT(4)
> > +#define BXCAN_MCR_RFLM                       BIT(3)
> > +#define BXCAN_MCR_TXFP                       BIT(2)
> > +#define BXCAN_MCR_SLEEP                      BIT(1)
> > +#define BXCAN_MCR_INRQ                       BIT(0)
> > +
> > +/* Master status register (MSR) bits */
> > +#define BXCAN_MSR_RX                 BIT(11)
> > +#define BXCAN_MSR_SAMP                       BIT(10)
> > +#define BXCAN_MSR_RXM                        BIT(9)
> > +#define BXCAN_MSR_TXM                        BIT(8)
> > +#define BXCAN_MSR_SLAKI                      BIT(4)
> > +#define BXCAN_MSR_WKUI                       BIT(3)
> > +#define BXCAN_MSR_ERRI                       BIT(2)
> > +#define BXCAN_MSR_SLAK                       BIT(1)
> > +#define BXCAN_MSR_INAK                       BIT(0)
> > +
> > +/* Transmit status register (TSR) bits */
> > +#define BXCAN_TSR_LOW2                       BIT(31)
> > +#define BXCAN_TSR_LOW1                       BIT(30)
> > +#define BXCAN_TSR_LOW0                       BIT(29)
> > +#define BXCAN_TSR_TME                        GENMASK(28, 26)
> > +#define BXCAN_TSR_TME_SHIFT          (26)
>
> unused
>
> > +#define BXCAN_TSR_TME2                       BIT(28)
> > +#define BXCAN_TSR_TME1                       BIT(27)
> > +#define BXCAN_TSR_TME0                       BIT(26)
> > +#define BXCAN_TSR_CODE                       GENMASK(25, 24)
> > +#define BXCAN_TSR_ABRQ2                      BIT(23)
> > +#define BXCAN_TSR_TERR2                      BIT(19)
> > +#define BXCAN_TSR_ALST2                      BIT(18)
> > +#define BXCAN_TSR_TXOK2                      BIT(17)
> > +#define BXCAN_TSR_RQCP2                      BIT(16)
> > +#define BXCAN_TSR_ABRQ1                      BIT(15)
> > +#define BXCAN_TSR_TERR1                      BIT(11)
> > +#define BXCAN_TSR_ALST1                      BIT(10)
> > +#define BXCAN_TSR_TXOK1                      BIT(9)
> > +#define BXCAN_TSR_RQCP1                      BIT(8)
> > +#define BXCAN_TSR_ABRQ0                      BIT(7)
> > +#define BXCAN_TSR_TERR0                      BIT(3)
> > +#define BXCAN_TSR_ALST0                      BIT(2)
> > +#define BXCAN_TSR_TXOK0                      BIT(1)
> > +#define BXCAN_TSR_RQCP0                      BIT(0)
> > +
> > +/* Receive FIFO 0 register (RF0R) bits */
> > +#define BXCAN_RF0R_RFOM0             BIT(5)
> > +#define BXCAN_RF0R_FOVR0             BIT(4)
> > +#define BXCAN_RF0R_FULL0             BIT(3)
> > +#define BXCAN_RF0R_FMP0                      GENMASK(1, 0)
> > +
> > +/* Interrupt enable register (IER) bits */
> > +#define BXCAN_IER_SLKIE                      BIT(17)
> > +#define BXCAN_IER_WKUIE                      BIT(16)
> > +#define BXCAN_IER_ERRIE                      BIT(15)
> > +#define BXCAN_IER_LECIE                      BIT(11)
> > +#define BXCAN_IER_BOFIE                      BIT(10)
> > +#define BXCAN_IER_EPVIE                      BIT(9)
> > +#define BXCAN_IER_EWGIE                      BIT(8)
> > +#define BXCAN_IER_FOVIE1             BIT(6)
> > +#define BXCAN_IER_FFIE1                      BIT(5)
> > +#define BXCAN_IER_FMPIE1             BIT(4)
> > +#define BXCAN_IER_FOVIE0             BIT(3)
> > +#define BXCAN_IER_FFIE0                      BIT(2)
> > +#define BXCAN_IER_FMPIE0             BIT(1)
> > +#define BXCAN_IER_TMEIE                      BIT(0)
> > +
> > +/* Error status register (ESR) bits */
> > +#define BXCAN_ESR_REC_SHIFT          (24)
> > +#define BXCAN_ESR_REC                        GENMASK(31, 24)
> > +#define BXCAN_ESR_TEC_SHIFT          (16)
> > +#define BXCAN_ESR_TEC                        GENMASK(23, 16)
> > +#define BXCAN_ESR_LEC_SHIFT          (4)
>
> please remove the _SHIFT macros, no need for them...
>
> > +#define BXCAN_ESR_LEC                        GENMASK(6, 4)
> > +#define BXCAN_ESR_BOFF                       BIT(1)
> > +#define BXCAN_ESR_EPVF                       BIT(1)
> > +#define BXCAN_ESR_EWGF                       BIT(0)
> > +#define BXCAN_TEC(esr)                       (((esr) & BXCAN_ESR_TEC) >> \
> > +                                      BXCAN_ESR_TEC_SHIFT)
> > +#define BXCAN_REC(esr)                       (((esr) & BXCAN_ESR_REC) >> \
> > +                                      BXCAN_ESR_REC_SHIFT)
>
> ... remove these and use FIELD_GET() directly.

ok, definitely better

>
> > +
> > +/* Bit timing register (BTR) bits */
> > +#define BXCAN_BTR_SILM                       BIT(31)
> > +#define BXCAN_BTR_LBKM                       BIT(30)
> > +#define BXCAN_BTR_SJW_MASK           GENMASK(25, 24)
> > +#define BXCAN_BTR_SJW(x)             (((x) & 0x03) << 24)
> > +#define BXCAN_BTR_TS2_MASK           GENMASK(22, 20)
> > +#define BXCAN_BTR_TS2(x)             (((x) & 0x07) << 20)
> > +#define BXCAN_BTR_TS1_MASK           GENMASK(19, 16)
> > +#define BXCAN_BTR_TS1(x)             (((x) & 0x0f) << 16)
> > +#define BXCAN_BTR_BRP_MASK           GENMASK(9, 0)
> > +#define BXCAN_BTR_BRP(x)             ((x) & 0x3ff)
>
> same for these
>
> > +
> > +/* TX mailbox identifier register (TIxR, x = 0..2) bits */
> > +#define BXCAN_TIxR_STID(x)           (((x) & 0x7ff) << 21)
> > +#define BXCAN_TIxR_EXID(x)           ((x) << 3)
>
> same here
>
> > +#define BXCAN_TIxR_IDE                       BIT(2)
> > +#define BXCAN_TIxR_RTR                       BIT(1)
> > +#define BXCAN_TIxR_TXRQ                      BIT(0)
> > +
> > +/* TX mailbox data length and time stamp register (TDTxR, x = 0..2 bits */
> > +#define BXCAN_TDTxR_TIME(x)          (((x) & 0x0f) << 16)
>
> same
>
> > +#define BXCAN_TDTxR_TGT                      BIT(8)
> > +#define BXCAN_TDTxR_DLC_MASK         GENMASK(3, 0)
> > +#define BXCAN_TDTxR_DLC(x)           ((x) & 0x0f)
>
> same
>
> > +
> > +/* RX FIFO mailbox identifier register (RIxR, x = 0..1 */
> > +#define BXCAN_RIxR_STID_SHIFT                (21)
> > +#define BXCAN_RIxR_EXID_SHIFT                (3)
> > +#define BXCAN_RIxR_IDE                       BIT(2)
> > +#define BXCAN_RIxR_RTR                       BIT(1)
> > +
> > +/* RX FIFO mailbox data length and timestamp register (RDTxR, x = 0..1) bits */
> > +#define BXCAN_RDTxR_TIME             GENMASK(31, 16)
> > +#define BXCAN_RDTxR_FMI                      GENMASK(15, 8)
> > +#define BXCAN_RDTxR_DLC                      GENMASK(3, 0)
> > +
> > +enum bxcan_lec_code {
> > +     LEC_NO_ERROR = 0,
> > +     LEC_STUFF_ERROR,
> > +     LEC_FORM_ERROR,
> > +     LEC_ACK_ERROR,
> > +     LEC_BIT1_ERROR,
> > +     LEC_BIT0_ERROR,
> > +     LEC_CRC_ERROR,
> > +     LEC_UNUSED
>
> Please add BXCAN_ as prefix
>
> > +};
> > +
> > +/* Structure of the message buffer */
> > +struct bxcan_mb {
> > +     u32 id;                 /* can identifier */
> > +     u32 dlc;                /* data length control and timestamp */
> > +     u32 data[2];            /* data */
> > +};
> > +
> > +/* Structure of the hardware registers */
> > +struct bxcan_regs {
> > +     u32 mcr;                        /* 0x00 - master control */
> > +     u32 msr;                        /* 0x04 - master status */
> > +     u32 tsr;                        /* 0x08 - transmit status */
> > +     u32 rf0r;                       /* 0x0c - FIFO 0 */
> > +     u32 rf1r;                       /* 0x10 - FIFO 1 */
> > +     u32 ier;                        /* 0x14 - interrupt enable */
> > +     u32 esr;                        /* 0x18 - error status */
> > +     u32 btr;                        /* 0x1c - bit timing*/
> > +     u32 reserved0[88];              /* 0x20 */
> > +     struct bxcan_mb tx_mb[BXCAN_TX_MB_NUM]; /* 0x180 - tx mailbox */
> > +     struct bxcan_mb rx_mb[2];       /* 0x1b0 - rx mailbox */
>
> Please add a define for the "2", as well.
>
> > +};
> > +
> > +struct bxcan_priv {
> > +     struct can_priv can;
> > +     struct device *dev;
> > +     struct net_device *ndev;
> > +     struct napi_struct napi;
>
> consider using can_rx_offload().
>
> The idea is to read from the mailbox in the IRQ handler and push the
> frame into the networking stack in the napi.

Ok, thanks for the suggestion and the explanation. I will use it.

>
> > +
> > +     struct bxcan_regs __iomem *regs;
> > +     int tx_irq;
> > +     int sce_irq;
> > +     u8 tx_dlc[BXCAN_TX_MB_NUM];
>
> unused
>
> > +     bool master;
> > +     struct clk *clk;
> > +     unsigned int tx_head;
> > +     unsigned int tx_tail;
> > +};
> > +
> > +static const struct can_bittiming_const bxcan_bittiming_const = {
> > +     .name = KBUILD_MODNAME,
> > +     .tseg1_min = 1,
> > +     .tseg1_max = 16,
> > +     .tseg2_min = 1,
> > +     .tseg2_max = 8,
> > +     .sjw_max = 4,
> > +     .brp_min = 1,
> > +     .brp_max = 1024,
> > +     .brp_inc = 1,
> > +};
> > +
> > +static inline u8 bxcan_get_tx_head(const struct bxcan_priv *priv)
> > +{
> > +     return priv->tx_head % BXCAN_TX_MB_NUM;
> > +}
> > +
> > +static inline u8 bxcan_get_tx_tail(const struct bxcan_priv *priv)
> > +{
> > +     return priv->tx_tail % BXCAN_TX_MB_NUM;
> > +}
> > +
> > +static inline u8 bxcan_get_tx_free(const struct bxcan_priv *priv)
> > +{
> > +     return BXCAN_TX_MB_NUM - (priv->tx_head - priv->tx_tail);
> > +}
> > +
> > +static bool bxcan_tx_busy(const struct bxcan_priv *priv)
> > +{
> > +     if (bxcan_get_tx_free(priv) > 0)
> > +             return false;
> > +
> > +     netif_stop_queue(priv->ndev);
> > +
> > +     /* Memory barrier before checking tx_free (head and tail) */
> > +     smp_mb();
> > +
> > +     if (bxcan_get_tx_free(priv) == 0) {
> > +             netdev_dbg(priv->ndev,
> > +                        "Stopping tx-queue (tx_head=0x%08x, tx_tail=0x%08x, len=%d).\n",
> > +                        priv->tx_head, priv->tx_tail,
> > +                        priv->tx_head - priv->tx_tail);
> > +
> > +             return true;
> > +     }
> > +
> > +     netif_start_queue(priv->ndev);
> > +
> > +     return false;
> > +}
> > +
> > +static int bxcan_chip_softreset(struct bxcan_priv *priv)
> > +{
> > +     struct bxcan_regs __iomem *regs = priv->regs;
> > +     unsigned int timeout = BXCAN_TIMEOUT_US / 10;
> > +
> > +     bxcan_rmw(&regs->mcr, 0, BXCAN_MCR_RESET);
> > +     while (timeout-- && !(readl(&regs->msr) & BXCAN_MSR_SLAK))
> > +             udelay(10);
>
> make use of readx_poll_timeout(), see:
>
> | https://elixir.bootlin.com/linux/v6.0-rc4/source/drivers/net/can/spi/mcp251x.c#L410

definitely better

> > +
> > +     if (!(readl(&regs->msr) & BXCAN_MSR_SLAK))
> > +             return -ETIMEDOUT;
> > +
> > +     return 0;
> > +}
> > +
> > +static int bxcan_enter_init_mode(struct bxcan_priv *priv)
> > +{
> > +     struct bxcan_regs __iomem *regs = priv->regs;
> > +     unsigned int timeout = BXCAN_TIMEOUT_US / 10;
> > +
> > +     bxcan_rmw(&regs->mcr, 0, BXCAN_MCR_INRQ);
> > +     while (timeout-- && !(readl(&regs->msr) & BXCAN_MSR_INAK))
> > +             udelay(100);
>
> same here
>
> > +
> > +     if (!(readl(&regs->msr) & BXCAN_MSR_INAK))
> > +             return -ETIMEDOUT;
> > +
> > +     return 0;
> > +}
> > +
> > +static int bxcan_leave_init_mode(struct bxcan_priv *priv)
> > +{
> > +     struct bxcan_regs __iomem *regs = priv->regs;
> > +     unsigned int timeout = BXCAN_TIMEOUT_US / 10;
> > +
> > +     bxcan_rmw(&regs->mcr, BXCAN_MCR_INRQ, 0);
> > +     while (timeout-- && (readl(&regs->msr) & BXCAN_MSR_INAK))
> > +             udelay(100);
>
> same
>
> > +
> > +     if (readl(&regs->msr) & BXCAN_MSR_INAK)
> > +             return -ETIMEDOUT;
> > +
> > +     return 0;
> > +}
> > +
> > +static int bxcan_leave_sleep_mode(struct bxcan_priv *priv)
> > +{
> > +     struct bxcan_regs __iomem *regs = priv->regs;
> > +     unsigned int timeout = BXCAN_TIMEOUT_US / 10;
> > +
> > +     bxcan_rmw(&regs->mcr, BXCAN_MCR_SLEEP, 0);
> > +     while (timeout-- && (readl(&regs->msr) & BXCAN_MSR_SLAK))
> > +             udelay(100);
>
> same
>
> > +
> > +     if (readl(&regs->msr) & BXCAN_MSR_SLAK)
> > +             return -ETIMEDOUT;
> > +
> > +     return 0;
> > +}
> > +
> > +static int bxcan_enter_sleep_mode(struct bxcan_priv *priv)
> > +{
> > +     struct bxcan_regs __iomem *regs = priv->regs;
> > +     unsigned int timeout = BXCAN_TIMEOUT_US / 10;
> > +
> > +     bxcan_rmw(&regs->mcr, 0, BXCAN_MCR_SLEEP);
> > +     while (timeout-- && !(readl(&regs->msr) & BXCAN_MSR_SLAK))
> > +             udelay(100);
>
> same
>
> > +
> > +     if (!(readl(&regs->msr) & BXCAN_MSR_SLAK))
> > +             return -ETIMEDOUT;
> > +
> > +     return 0;
> > +}
> > +
> > +static irqreturn_t bxcan_rx_isr(int irq, void *dev_id)
> > +{
> > +     struct net_device *ndev = dev_id;
> > +     struct bxcan_priv *priv = netdev_priv(ndev);
> > +     struct bxcan_regs __iomem *regs = priv->regs;
> > +
> > +     if (napi_schedule_prep(&priv->napi)) {
> > +             /* Disable Rx FIFO message pending interrupt */
> > +             bxcan_rmw(&regs->ier, BXCAN_IER_FMPIE0, 0);
> > +             __napi_schedule(&priv->napi);
> > +     }
> > +
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t bxcan_tx_isr(int irq, void *dev_id)
> > +{
> > +     struct net_device *ndev = dev_id;
> > +     struct bxcan_priv *priv = netdev_priv(ndev);
> > +     struct bxcan_regs __iomem *regs = priv->regs;
> > +     struct net_device_stats *stats = &ndev->stats;
> > +     u32 tsr, rqcp_bit, bytes = 0, pkts = 0;
> > +     int n, idx;
> > +
> > +     tsr = readl(&regs->tsr);
> > +     for (n = 0, idx = bxcan_get_tx_tail(priv); n < BXCAN_TX_MB_NUM; n++) {
>
> loop from tail to head:
>
> | while (priv->tx_head - priv->tx_tail > 0) {
> | ...
> |       idx = bxcan_get_tx_tail()
> | ...
>

Ok, thanks

>
> > +             rqcp_bit = BXCAN_TSR_RQCP0 << (idx << 3);
> > +             if (tsr & rqcp_bit) {
> > +                     pkts++;
> > +                     bytes += can_get_echo_skb(ndev, idx, NULL);
> > +             }
> > +
> > +             idx += 1;
> > +             if (idx == BXCAN_TX_MB_NUM)
> > +                     idx = 0;
> > +     }
> > +
> > +     if (!pkts)
> > +             return IRQ_HANDLED;
>
> This looks wrong. Better check if any relevant bits in tsr are set, if
> not return IRQ_NONE.

Ok

>
> > +
> > +     writel(tsr, &regs->tsr);
> > +
> > +     priv->tx_tail += pkts;
> > +     if (bxcan_get_tx_free(priv)) {
> > +             /* Make sure that anybody stopping the queue after
> > +              * this sees the new tx_ring->tail.
> > +              */
> > +             smp_mb();
> > +             netif_wake_queue(ndev);
> > +     }
> > +
> > +     stats->tx_bytes += bytes;
> > +     stats->tx_packets += pkts;
> > +
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +static void bxcan_handle_state_change(struct net_device *ndev, u32 esr)
> > +{
> > +     struct bxcan_priv *priv = netdev_priv(ndev);
> > +     struct net_device_stats *stats = &ndev->stats;
> > +     enum can_state new_state = priv->can.state;
> > +     struct can_berr_counter bec;
> > +     enum can_state rx_state, tx_state;
> > +     struct sk_buff *skb;
> > +     struct can_frame *cf;
> > +
> > +     /* Early exit if no error flag is set */
> > +     if (!(esr & (BXCAN_ESR_EWGF | BXCAN_ESR_EPVF | BXCAN_ESR_BOFF)))
> > +             return;
> > +
> > +     bec.txerr = BXCAN_TEC(esr);
> > +     bec.rxerr = BXCAN_REC(esr);
> > +
> > +     if (esr & BXCAN_ESR_BOFF)
> > +             new_state = CAN_STATE_BUS_OFF;
> > +     else if (esr & BXCAN_ESR_EPVF)
> > +             new_state = CAN_STATE_ERROR_PASSIVE;
> > +     else if (esr & BXCAN_ESR_EWGF)
> > +             new_state = CAN_STATE_ERROR_WARNING;
> > +
> > +     /* state hasn't changed */
> > +     if (unlikely(new_state == priv->can.state))
> > +             return;
> > +
> > +     skb = alloc_can_err_skb(ndev, &cf);
> > +     if (unlikely(!skb))
> > +             return;
>
> Continue, even if you got no skb.
>
> > +
> > +     tx_state = bec.txerr >= bec.rxerr ? new_state : 0;
> > +     rx_state = bec.txerr <= bec.rxerr ? new_state : 0;
> > +     can_change_state(ndev, cf, tx_state, rx_state);
>
> can_change_state() handles cf == NULL....
>
> > +
> > +     if (new_state == CAN_STATE_BUS_OFF)
> > +             can_bus_off(ndev);
> > +
> > +     stats->rx_bytes += cf->len;
> > +     stats->rx_packets++;
>
> no statistics for error frames.
>
> > +     netif_rx(skb);
> > +}
> > +
> > +static void bxcan_handle_bus_err(struct net_device *ndev, u32 esr)
> > +{
> > +     struct bxcan_priv *priv = netdev_priv(ndev);
> > +     enum bxcan_lec_code lec_code;
> > +     struct can_frame *cf;
> > +     struct sk_buff *skb;
> > +
> > +     lec_code = (esr & BXCAN_ESR_LEC_SHIFT) >> BXCAN_ESR_LEC_SHIFT;
>
> Use FIELD_GET()
>
> > +
> > +     /* Early exit if no lec update or no error.
> > +      * No lec update means that no CAN bus event has been detected
> > +      * since CPU wrote LEC_UNUSED value to status reg.
> > +      */
> > +     if (lec_code == LEC_UNUSED || lec_code == LEC_NO_ERROR)
> > +             return;
> > +
> > +     if (!(priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING))
> > +             return;
>
> Can you disable the generation of the bus error interrupt?

I thought to make the setting of the BXCAN_IER_LECIE bit in the ier register
depend on CAN_CTRLMODE_BERR_REPORTING.
What do you think?

>
> > +
> > +     /* Common for all type of bus errors */
> > +     priv->can.can_stats.bus_error++;
> > +
> > +     /* Propagate the error condition to the CAN stack */
> > +     skb = alloc_can_err_skb(ndev, &cf);
> > +     if (unlikely(!skb))
> > +             return;
>
> continue, even without skb
>
> > +
> > +     ndev->stats.rx_bytes += cf->len;
>
> no stats
>
> > +
> > +     /* Check for 'last error code' which tells us the
> > +      * type of the last error to occur on the CAN bus
> > +      */
> > +     cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> > +
> > +     switch (lec_code) {
> > +     case LEC_STUFF_ERROR:
> > +             netdev_dbg(ndev, "Stuff error\n");
> > +             ndev->stats.rx_errors++;
> > +             cf->data[2] |= CAN_ERR_PROT_STUFF;
> > +             break;
> > +     case LEC_FORM_ERROR:
> > +             netdev_dbg(ndev, "Form error\n");
> > +             ndev->stats.rx_errors++;
> > +             cf->data[2] |= CAN_ERR_PROT_FORM;
> > +             break;
> > +     case LEC_ACK_ERROR:
> > +             netdev_dbg(ndev, "Ack error\n");
> > +             ndev->stats.tx_errors++;
> > +             cf->data[3] = CAN_ERR_PROT_LOC_ACK;
> > +             break;
> > +     case LEC_BIT1_ERROR:
> > +             netdev_dbg(ndev, "Bit error (recessive)\n");
> > +             ndev->stats.tx_errors++;
> > +             cf->data[2] |= CAN_ERR_PROT_BIT1;
> > +             break;
> > +     case LEC_BIT0_ERROR:
> > +             netdev_dbg(ndev, "Bit error (dominant)\n");
> > +             ndev->stats.tx_errors++;
> > +             cf->data[2] |= CAN_ERR_PROT_BIT0;
> > +             break;
> > +     case LEC_CRC_ERROR:
> > +             netdev_dbg(ndev, "CRC error\n");
> > +             ndev->stats.rx_errors++;
> > +             cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ;
> > +             break;
> > +     default:
> > +             break;
> > +     }
> > +
> > +     netif_rx(skb);
> > +}
> > +
> > +static irqreturn_t bxcan_state_change_isr(int irq, void *dev_id)
> > +{
> > +     struct net_device *ndev = dev_id;
> > +     struct bxcan_priv *priv = netdev_priv(ndev);
> > +     struct bxcan_regs __iomem *regs = priv->regs;
> > +     u32 msr, esr;
> > +
> > +     msr = readl(&regs->msr);
> > +     if (!(msr & BXCAN_MSR_ERRI))
> > +             return IRQ_NONE;
> > +
> > +     esr = readl(&regs->esr);
> > +     bxcan_handle_state_change(ndev, esr);
> > +     bxcan_handle_bus_err(ndev, esr);
> > +
> > +     msr |= BXCAN_MSR_ERRI;
> > +     writel(msr, &regs->msr);
>
> nitpick: please add an empty line in front of return
>
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +static int bxcan_start(struct net_device *ndev)
>
> please rename to bxcan_chip_start()
>
> > +{
> > +     struct bxcan_priv *priv = netdev_priv(ndev);
> > +     struct bxcan_regs __iomem *regs = priv->regs;
> > +     struct can_bittiming *bt = &priv->can.bittiming;
> > +     u32 set;
> > +     int err;
> > +
> > +     err = bxcan_chip_softreset(priv);
> > +     if (err) {
> > +             netdev_err(ndev, "failed to reset chip, error %d\n", err);
> > +             return err;
> > +     }
> > +
> > +     err = bxcan_leave_sleep_mode(priv);
> > +     if (err) {
> > +             netdev_err(ndev, "failed to leave sleep mode, error %d\n", err);
> > +             goto failed_leave_sleep;
> > +     }
> > +
> > +     err = bxcan_enter_init_mode(priv);
> > +     if (err) {
> > +             netdev_err(ndev, "failed to enter init mode, error %d\n", err);
> > +             goto failed_enter_init;
> > +     }
> > +
> > +     /* MCR
> > +      *
> > +      * select request order priority
> > +      * disable time triggered mode
> > +      * bus-off state left on sw request
> > +      * sleep mode left on sw request
> > +      * retransmit automatically on error
> > +      * do not lock RX FIFO on overrun
> > +      */
> > +     bxcan_rmw(&regs->mcr, BXCAN_MCR_TTCM | BXCAN_MCR_ABOM | BXCAN_MCR_AWUM |
> > +               BXCAN_MCR_NART | BXCAN_MCR_RFLM, BXCAN_MCR_TXFP);
> > +
> > +     /* Bit timing register settings */
> > +     set = BXCAN_BTR_BRP(bt->brp - 1) |
> > +             BXCAN_BTR_TS1(bt->phase_seg1 + bt->prop_seg - 1) |
> > +             BXCAN_BTR_TS2(bt->phase_seg2 - 1) | BXCAN_BTR_SJW(bt->sjw - 1);
> > +
> > +     /* loopback + silent mode put the controller in test mode,
> > +      * useful for hot self-test
> > +      */
> > +     if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
> > +             set |= BXCAN_BTR_LBKM;
> > +
> > +     if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> > +             set |= BXCAN_BTR_SILM;
> > +
> > +     netdev_dbg(ndev,
> > +                "TQ[ns]: %d, PrS: %d, PhS1: %d, PhS2: %d, SJW: %d, BRP: %d, CAN_BTR: 0x%08x\n",
> > +                bt->tq, bt->prop_seg, bt->phase_seg1, bt->phase_seg2,
> > +                bt->sjw, bt->brp, set);
> > +     bxcan_rmw(&regs->btr, BXCAN_BTR_SILM | BXCAN_BTR_LBKM |
> > +               BXCAN_BTR_BRP_MASK | BXCAN_BTR_TS1_MASK | BXCAN_BTR_TS2_MASK |
> > +               BXCAN_BTR_SJW_MASK, set);
> > +
> > +     bxcan_enable_filters(priv->dev->parent, priv->master);
> > +
> > +     /* Clear all internal status */
> > +     priv->tx_head = 0;
> > +     priv->tx_tail = 0;
> > +
> > +     err = bxcan_leave_init_mode(priv);
> > +     if (err) {
> > +             netdev_err(ndev, "failed to leave init mode, error %d\n", err);
> > +             goto failed_leave_init;
> > +     }
> > +
> > +     /* Set a `lec` value so that we can check for updates later */
> > +     bxcan_rmw(&regs->esr, BXCAN_ESR_LEC, LEC_UNUSED << BXCAN_ESR_LEC_SHIFT);
> > +
> > +     /* IER
> > +      *
> > +      * Enable interrupt for:
> > +      * bus-off
> > +      * passive error
> > +      * warning error
> > +      * last error code
> > +      * RX FIFO pending message
> > +      * TX mailbox empty
> > +      */
> > +     bxcan_rmw(&regs->ier, BXCAN_IER_WKUIE | BXCAN_IER_SLKIE |
> > +               BXCAN_IER_FOVIE1 | BXCAN_IER_FFIE1 | BXCAN_IER_FMPIE1 |
> > +               BXCAN_IER_FOVIE0 | BXCAN_IER_FFIE0,
> > +               BXCAN_IER_ERRIE | BXCAN_IER_LECIE | BXCAN_IER_BOFIE |
> > +               BXCAN_IER_EPVIE | BXCAN_IER_EWGIE | BXCAN_IER_FMPIE0 |
> > +               BXCAN_IER_TMEIE);
> > +
> > +     priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > +     return 0;
> > +
> > +failed_leave_init:
> > +failed_enter_init:
> > +failed_leave_sleep:
> > +     bxcan_chip_softreset(priv);
> > +     return err;
> > +}
> > +
> > +static int bxcan_open(struct net_device *ndev)
> > +{
> > +     struct bxcan_priv *priv = netdev_priv(ndev);
> > +     int err;
> > +
> > +     err = open_candev(ndev);
> > +     if (err) {
> > +             netdev_err(ndev, "open_candev() failed, error %d\n", err);
> > +             goto failed_open;
> > +     }
> > +
> > +     napi_enable(&priv->napi);
> > +     err = request_irq(ndev->irq, bxcan_rx_isr, IRQF_SHARED, ndev->name,
> > +                       ndev);
> > +     if (err) {
> > +             netdev_err(ndev, "failed to register rx irq(%d), error %d\n",
> > +                        ndev->irq, err);
> > +             goto failed_rx_irq_request;
> > +     }
> > +
> > +     err = request_irq(priv->tx_irq, bxcan_tx_isr, IRQF_SHARED, ndev->name,
> > +                       ndev);
> > +     if (err) {
> > +             netdev_err(ndev, "failed to register tx irq(%d), error %d\n",
> > +                        priv->tx_irq, err);
> > +             goto failed_tx_irq_request;
> > +     }
> > +
> > +     err = request_irq(priv->sce_irq, bxcan_state_change_isr, IRQF_SHARED,
> > +                       ndev->name, ndev);
> > +     if (err) {
> > +             netdev_err(ndev, "failed to register sce irq(%d), error %d\n",
> > +                        priv->sce_irq, err);
> > +             goto failed_sce_irq_request;
> > +     }
> > +
> > +     err = bxcan_start(ndev);
> > +     if (err)
> > +             goto failed_start;
> > +
> > +     netif_start_queue(ndev);
> > +     return 0;
> > +
> > +failed_start:
> > +failed_sce_irq_request:
> > +     free_irq(priv->tx_irq, ndev);
> > +failed_tx_irq_request:
> > +     free_irq(ndev->irq, ndev);
> > +failed_rx_irq_request:
> > +     napi_disable(&priv->napi);
> > +     close_candev(ndev);
> > +failed_open:
> > +     return err;
> > +}
> > +
> > +static void bxcan_stop(struct net_device *ndev)
>
> please rename to bxcan_chip_stop()
> > +{
> > +     struct bxcan_priv *priv = netdev_priv(ndev);
> > +
> > +     bxcan_disable_filters(priv->dev->parent, priv->master);
>
> do you have to disable the IRQs?

I do not think so. But I will re-read the reference manual carefully.

>
> > +     bxcan_enter_sleep_mode(priv);
> > +     priv->can.state = CAN_STATE_STOPPED;
> > +}
> > +
> > +static int bxcan_close(struct net_device *ndev)
>
> and this bxcan_stop() to match the .ndo_stop.
>
> > +{
> > +     struct bxcan_priv *priv = netdev_priv(ndev);
> > +
> > +     netif_stop_queue(ndev);
> > +     bxcan_stop(ndev);
> > +     free_irq(ndev->irq, ndev);
> > +     free_irq(priv->tx_irq, ndev);
> > +     free_irq(priv->sce_irq, ndev);
> > +     napi_disable(&priv->napi);
> > +     close_candev(ndev);
> > +     return 0;
> > +}
> > +
> > +static netdev_tx_t bxcan_start_xmit(struct sk_buff *skb,
> > +                                 struct net_device *ndev)
> > +{
> > +     struct bxcan_priv *priv = netdev_priv(ndev);
> > +     struct can_frame *cf = (struct can_frame *)skb->data;
> > +     struct bxcan_regs __iomem *regs = priv->regs;
> > +     struct bxcan_mb __iomem *mb_regs;
> > +     unsigned int idx;
> > +     u32 id;
> > +     int i, j;
> > +
> > +     if (can_dropped_invalid_skb(ndev, skb))
> > +             return NETDEV_TX_OK;
> > +
> > +     if (bxcan_tx_busy(priv))
> > +             return NETDEV_TX_BUSY;
> > +
> > +     idx = bxcan_get_tx_head(priv);
> > +     priv->tx_head++;
> > +     if (bxcan_get_tx_free(priv) == 0)
> > +             netif_stop_queue(ndev);
> > +
> > +     mb_regs = &regs->tx_mb[idx];
> > +     if (cf->can_id & CAN_EFF_FLAG)
> > +             id = BXCAN_TIxR_EXID(cf->can_id & CAN_EFF_MASK) |
> > +                     BXCAN_TIxR_IDE;
> > +     else
> > +             id = BXCAN_TIxR_STID(cf->can_id & CAN_SFF_MASK);
> > +
> > +     if (cf->can_id & CAN_RTR_FLAG)
> > +             id |= BXCAN_TIxR_RTR;
> > +
> > +     bxcan_rmw(&mb_regs->dlc, BXCAN_TDTxR_DLC_MASK,
> > +               BXCAN_TDTxR_DLC(cf->len));
>
> Is rmw needed here, or does a pure write work, too?

I'll use a pure write.

>
> > +     can_put_echo_skb(skb, ndev, idx, 0);
> > +
> > +     for (i = 0, j = 0; i < cf->len; i += 4, j++)
> > +             writel(*(u32 *)(cf->data + i), &mb_regs->data[j]);
> > +
> > +     /* Start transmission */
> > +     writel(id | BXCAN_TIxR_TXRQ, &mb_regs->id);
> > +
> > +     return NETDEV_TX_OK;
> > +}
> > +
> > +static const struct net_device_ops bxcan_netdev_ops = {
> > +     .ndo_open = bxcan_open,
> > +     .ndo_stop = bxcan_close,
> > +     .ndo_start_xmit = bxcan_start_xmit,
> > +     .ndo_change_mtu = can_change_mtu,
> > +};
> > +
> > +static void bxcan_rx_pkt(struct net_device *ndev,
> > +                      struct bxcan_mb __iomem *mb_regs)
> > +{
> > +     struct net_device_stats *stats = &ndev->stats;
> > +     struct can_frame *cf;
> > +     struct sk_buff *skb;
> > +     u32 id, dlc;
> > +
> > +     skb = alloc_can_skb(ndev, &cf);
> > +     if (!skb) {
> > +             stats->rx_dropped++;
> > +             return;
> > +     }
> > +
> > +     id = readl(&mb_regs->id);
> > +     if (id & BXCAN_RIxR_IDE)
> > +             cf->can_id = (id >> BXCAN_RIxR_EXID_SHIFT) | CAN_EFF_FLAG;
> > +     else
> > +             cf->can_id = (id >> BXCAN_RIxR_STID_SHIFT) & CAN_SFF_MASK;
> > +
> > +     dlc = readl(&mb_regs->dlc) & BXCAN_RDTxR_DLC;
> > +     cf->len = can_cc_dlc2len(dlc);
> > +
> > +     if (id & BXCAN_RIxR_RTR) {
> > +             cf->can_id |= CAN_RTR_FLAG;
> > +     } else {
> > +             int i, j;
> > +
> > +             for (i = 0, j = 0; i < cf->len; i += 4, j++)
> > +                     *(u32 *)(cf->data + i) = readl(&mb_regs->data[j]);
> > +
> > +             stats->rx_bytes += cf->len;
> > +     }
> > +
> > +     stats->rx_packets++;
> > +     netif_receive_skb(skb);
> > +}
> > +
> > +static int bxcan_rx_poll(struct napi_struct *napi, int quota)
> > +{
> > +     struct net_device *ndev = napi->dev;
> > +     struct bxcan_priv *priv = netdev_priv(ndev);
> > +     struct bxcan_regs __iomem *regs = priv->regs;
> > +     int num_pkts;
> > +     u32 rf0r;
> > +
> > +     for (num_pkts = 0; num_pkts < quota; num_pkts++) {
> > +             rf0r = readl(&regs->rf0r);
> > +             if (!(rf0r & BXCAN_RF0R_FMP0))
> > +                     break;
> > +
> > +             bxcan_rx_pkt(ndev, &regs->rx_mb[0]);
> > +
> > +             rf0r |= BXCAN_RF0R_RFOM0;
> > +             writel(rf0r, &regs->rf0r);
> > +     }
> > +
> > +     if (num_pkts < quota) {
> > +             napi_complete_done(napi, num_pkts);
> > +             bxcan_rmw(&regs->ier, 0, BXCAN_IER_FMPIE0);
> > +     }
> > +
> > +     return num_pkts;
> > +}
> > +
> > +static int bxcan_do_set_mode(struct net_device *ndev, enum can_mode mode)
> > +{
> > +     int err;
> > +
> > +     switch (mode) {
> > +     case CAN_MODE_START:
> > +             err = bxcan_start(ndev);
> > +             if (err)
> > +                     return err;
> > +
> > +             netif_wake_queue(ndev);
> > +             break;
> > +     default:
> > +             return -EOPNOTSUPP;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int bxcan_disable_clks(struct bxcan_priv *priv)
> > +{
> > +     if (priv->clk)
> > +             clk_disable_unprepare(priv->clk);
> > +
> > +     bxcan_disable_master_clk(priv->dev->parent);
> > +     return 0;
> > +}
> > +
> > +static int bxcan_enable_clks(struct bxcan_priv *priv)
> > +{
> > +     int err;
> > +
> > +     err = bxcan_enable_master_clk(priv->dev->parent);
> > +     if (err)
> > +             return err;
> > +
> > +     if (priv->clk) {
> > +             err = clk_prepare_enable(priv->clk);
>
> clk_prepare_enable() works on NULL pointer, too.

The slave has its own clock, the master uses the one owned by the core driver.
In this way I wanted to avoid having to enable the master clock twice
(and therefore
improperly increase the ref-counting).

Thanks for all your suggestions and explanations.
I'll develop them asap.

Thanks and regards,
Dario

>
> > +             if (err)
> > +                     bxcan_disable_master_clk(priv->dev->parent);
> > +     }
> > +
> > +     return err;
> > +}
> > +
> > +static int bxcan_get_berr_counter(const struct net_device *ndev,
> > +                               struct can_berr_counter *bec)
> > +{
> > +     struct bxcan_priv *priv = netdev_priv(ndev);
> > +     struct bxcan_regs __iomem *regs = priv->regs;
> > +     u32 esr;
> > +     int err;
> > +
> > +     err = bxcan_enable_clks(priv);
> > +     if (err)
> > +             return err;
> > +
> > +     esr = readl(&regs->esr);
> > +     bec->txerr = BXCAN_TEC(esr);
> > +     bec->rxerr = BXCAN_REC(esr);
> > +     err = bxcan_disable_clks(priv);
> > +     return 0;
> > +}
> > +
> > +static int bxcan_probe(struct platform_device *pdev)
> > +{
> > +     struct device_node *np = pdev->dev.of_node;
> > +     struct device *dev = &pdev->dev;
> > +     struct net_device *ndev;
> > +     struct bxcan_priv *priv;
> > +     struct clk *clk = NULL;
> > +     bool master;
> > +     u32 offset;
> > +     int err, rx_irq, tx_irq, sce_irq;
> > +
> > +     master = of_property_read_bool(np, "st,can-master");
> > +     if (!master) {
> > +             clk = devm_clk_get(dev, NULL);
> > +             if (IS_ERR(clk)) {
> > +                     dev_err(dev, "failed to get clock\n");
> > +                     return PTR_ERR(clk);
> > +             }
> > +     }
> > +
> > +     rx_irq = platform_get_irq_byname(pdev, "rx0");
> > +     if (rx_irq < 0) {
> > +             dev_err(dev, "failed to get rx0 irq\n");
> > +             return rx_irq;
> > +     }
> > +
> > +     tx_irq = platform_get_irq_byname(pdev, "tx");
> > +     if (tx_irq < 0) {
> > +             dev_err(dev, "failed to get tx irq\n");
> > +             return tx_irq;
> > +     }
> > +
> > +     sce_irq = platform_get_irq_byname(pdev, "sce");
> > +     if (sce_irq < 0) {
> > +             dev_err(dev, "failed to get sce irq\n");
> > +             return sce_irq;
> > +     }
> > +
> > +     err = of_property_read_u32(np, "reg", &offset);
> > +     if (err) {
> > +             dev_err(dev, "failed to get reg property\n");
> > +             return err;
> > +     }
> > +
> > +     ndev = alloc_candev(sizeof(struct bxcan_priv), BXCAN_TX_MB_NUM);
> > +     if (!ndev) {
> > +             dev_err(dev, "alloc_candev() failed\n");
> > +             return -ENOMEM;
> > +     }
> > +
> > +     priv = netdev_priv(ndev);
> > +     platform_set_drvdata(pdev, ndev);
> > +     SET_NETDEV_DEV(ndev, dev);
> > +     ndev->netdev_ops = &bxcan_netdev_ops;
> > +     ndev->irq = rx_irq;
> > +     ndev->flags |= IFF_ECHO;
> > +
> > +     priv->dev = dev;
> > +     priv->ndev = ndev;
> > +     priv->regs = bxcan_get_base_addr(dev->parent) + offset;
> > +     priv->clk = clk;
> > +     priv->tx_irq = tx_irq;
> > +     priv->sce_irq = sce_irq;
> > +     priv->master = master;
> > +     priv->can.clock.freq =
> > +             master ? bxcan_get_master_clk_rate(dev->parent) :
> > +             clk_get_rate(clk);
> > +     priv->tx_head = 0;
> > +     priv->tx_tail = 0;
> > +     priv->can.bittiming_const = &bxcan_bittiming_const;
> > +     priv->can.do_set_mode = bxcan_do_set_mode;
> > +     priv->can.do_get_berr_counter = bxcan_get_berr_counter;
> > +     priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
> > +             CAN_CTRLMODE_LISTENONLY | CAN_CTRLMODE_BERR_REPORTING;
> > +     netif_napi_add(ndev, &priv->napi, bxcan_rx_poll, BXCAN_NAPI_WEIGHT);
> > +
> > +     err = bxcan_enable_clks(priv);
> > +     if (err) {
> > +             dev_err(dev, "failed to enable clocks\n");
> > +             return err;
> > +     }
> > +
> > +     err = register_candev(ndev);
> > +     if (err) {
> > +             dev_err(dev, "failed to register netdev\n");
> > +             goto failed_register;
> > +     }
> > +
> > +     dev_info(dev, "clk: %d Hz, IRQs: %d, %d, %d\n", priv->can.clock.freq,
> > +              tx_irq, rx_irq, sce_irq);
> > +     return 0;
> > +
> > +failed_register:
> > +     netif_napi_del(&priv->napi);
> > +     free_candev(ndev);
> > +     return err;
> > +}
> > +
> > +static int bxcan_remove(struct platform_device *pdev)
> > +{
> > +     struct net_device *ndev = platform_get_drvdata(pdev);
> > +     struct bxcan_priv *priv = netdev_priv(ndev);
> > +
> > +     unregister_candev(ndev);
> > +     bxcan_disable_clks(priv);
> > +     netif_napi_del(&priv->napi);
> > +     free_candev(ndev);
> > +     return 0;
> > +}
> > +
> > +static int __maybe_unused bxcan_suspend(struct device *dev)
> > +{
> > +     struct net_device *ndev = dev_get_drvdata(dev);
> > +     struct bxcan_priv *priv = netdev_priv(ndev);
> > +
> > +     if (!netif_running(ndev))
> > +             return 0;
> > +
> > +     netif_stop_queue(ndev);
> > +     netif_device_detach(ndev);
> > +
> > +     bxcan_enter_sleep_mode(priv);
> > +     priv->can.state = CAN_STATE_SLEEPING;
> > +     bxcan_disable_clks(priv);
> > +     return 0;
> > +}
> > +
> > +static int __maybe_unused bxcan_resume(struct device *dev)
> > +{
> > +     struct net_device *ndev = dev_get_drvdata(dev);
> > +     struct bxcan_priv *priv = netdev_priv(ndev);
> > +
> > +     if (!netif_running(ndev))
> > +             return 0;
> > +
> > +     bxcan_enable_clks(priv);
> > +     bxcan_leave_sleep_mode(priv);
> > +     priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > +
> > +     netif_device_attach(ndev);
> > +     netif_start_queue(ndev);
> > +     return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(bxcan_pm_ops, bxcan_suspend, bxcan_resume);
> > +
> > +static const struct of_device_id bxcan_of_match[] = {
> > +     {.compatible = "st,stm32f4-bxcan"},
> > +     { /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, bxcan_of_match);
> > +
> > +static struct platform_driver bxcan_driver = {
> > +     .driver = {
> > +             .name = KBUILD_MODNAME,
> > +             .pm = &bxcan_pm_ops,
> > +             .of_match_table = bxcan_of_match,
> > +     },
> > +     .probe = bxcan_probe,
> > +     .remove = bxcan_remove,
> > +};
> > +
> > +module_platform_driver(bxcan_driver);
> > +
> > +MODULE_AUTHOR("Dario Binacchi <dario.binacchi@amarulasolutions.com>");
> > +MODULE_DESCRIPTION("STMicroelectronics Basic Extended CAN controller driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.32.0
> >
> >
>
> Marc
>
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |



--

Dario Binacchi

Embedded Linux Developer

dario.binacchi@amarulasolutions.com