Message-ID: <20230324154037.xpqh65ehhdryagaf@pengutronix.de>
Date:   Fri, 24 Mar 2023 16:40:37 +0100
From:   Marc Kleine-Budde <mkl@...gutronix.de>
To:     Dario Binacchi <dario.binacchi@...rulasolutions.com>
Cc:     linux-kernel@...r.kernel.org,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Amarula patchwork <linux-amarula@...rulasolutions.com>,
        Vincent Mailhol <mailhol.vincent@...adoo.fr>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        michael@...rulasolutions.com, Rob Herring <robh@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        linux-can@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [RESEND PATCH v7 5/5] can: bxcan: add support for ST bxCAN
 controller

On 15.03.2023 22:10:40, 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.
> 
> 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@...rulasolutions.com>
> Reviewed-by: Vincent Mailhol <mailhol.vincent@...adoo.fr>


[...]

> diff --git a/drivers/net/can/bxcan.c b/drivers/net/can/bxcan.c
> new file mode 100644
> index 000000000000..daf4d7ef00e7
> --- /dev/null
> +++ b/drivers/net/can/bxcan.c

[...]

> +
> +static inline void bxcan_rmw(struct bxcan_priv *priv, void __iomem *addr,
> +			     u32 clear, u32 set)
> +{
> +	unsigned long flags;
> +	u32 old, val;
> +
> +	spin_lock_irqsave(&priv->rmw_lock, flags);
> +	old = readl(addr);
> +	val = (old & ~clear) | set;
> +	if (val != old)
> +		writel(val, addr);
> +
> +	spin_unlock_irqrestore(&priv->rmw_lock, flags);
> +}

I think you don't need the spin_lock anymore, but it's not in the hot
path, so leave it this way.

> +

[...]

> +static irqreturn_t bxcan_rx_isr(int irq, void *dev_id)
> +{
> +	struct net_device *ndev = dev_id;
> +	struct bxcan_priv *priv = netdev_priv(ndev);
> +
> +	can_rx_offload_irq_offload_fifo(&priv->offload);
> +	can_rx_offload_irq_finish(&priv->offload);
> +
> +	return IRQ_HANDLED;

This handler is not 100% shared IRQ safe, you have to return IRQ_NONE if
no IRQ is active.

> +}
> +
> +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;
> +	int idx;
> +
> +	tsr = readl(&regs->tsr);
> +	if (!(tsr & (BXCAN_TSR_RQCP0 | BXCAN_TSR_RQCP1 | BXCAN_TSR_RQCP2)))
> +		return IRQ_HANDLED;

Is this a check for no IRQ? Then return IRQ_NONE.

> +
> +	while (priv->tx_head - priv->tx_tail > 0) {
> +		idx = bxcan_get_tx_tail(priv);
> +		rqcp_bit = BXCAN_TSR_RQCP0 << (idx << 3);
> +		if (!(tsr & rqcp_bit))
> +			break;
> +
> +		stats->tx_packets++;
> +		stats->tx_bytes += can_get_echo_skb(ndev, idx, NULL);
> +		priv->tx_tail++;
> +	}
> +
> +	writel(tsr, &regs->tsr);
> +
> +	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);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void bxcan_handle_state_change(struct net_device *ndev, u32 esr)
> +{
> +	struct bxcan_priv *priv = netdev_priv(ndev);
> +	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 = FIELD_GET(BXCAN_ESR_TEC_MASK, esr);
> +	bec.rxerr = FIELD_GET(BXCAN_ESR_REC_MASK, 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);
> +
> +	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);
> +
> +	if (new_state == CAN_STATE_BUS_OFF) {
> +		can_bus_off(ndev);
> +	} else if (skb) {
> +		cf->can_id |= CAN_ERR_CNT;
> +		cf->data[6] = bec.txerr;
> +		cf->data[7] = bec.rxerr;
> +	}
> +
> +	if (skb) {
> +		int err;
> +
> +		err = can_rx_offload_queue_timestamp(&priv->offload, skb,
> +						     priv->timestamp);
> +		if (err)
> +			ndev->stats.rx_fifo_errors++;
> +	}
> +}
> +
> +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 = FIELD_GET(BXCAN_ESR_LEC_MASK, esr);
> +
> +	/* Early exit if no lec update or no error.
> +	 * No lec update means that no CAN bus event has been detected
> +	 * since CPU wrote BXCAN_LEC_UNUSED value to status reg.
> +	 */
> +	if (lec_code == BXCAN_LEC_UNUSED || lec_code == BXCAN_LEC_NO_ERROR)
> +		return;
> +
> +	/* 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 (skb)
> +		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> +
> +	switch (lec_code) {
> +	case BXCAN_LEC_STUFF_ERROR:
> +		netdev_dbg(ndev, "Stuff error\n");
> +		ndev->stats.rx_errors++;
> +		if (skb)
> +			cf->data[2] |= CAN_ERR_PROT_STUFF;
> +		break;
> +
> +	case BXCAN_LEC_FORM_ERROR:
> +		netdev_dbg(ndev, "Form error\n");
> +		ndev->stats.rx_errors++;
> +		if (skb)
> +			cf->data[2] |= CAN_ERR_PROT_FORM;
> +		break;
> +
> +	case BXCAN_LEC_ACK_ERROR:
> +		netdev_dbg(ndev, "Ack error\n");
> +		ndev->stats.tx_errors++;
> +		if (skb) {
> +			cf->can_id |= CAN_ERR_ACK;
> +			cf->data[3] = CAN_ERR_PROT_LOC_ACK;
> +		}
> +		break;
> +
> +	case BXCAN_LEC_BIT1_ERROR:
> +		netdev_dbg(ndev, "Bit error (recessive)\n");
> +		ndev->stats.tx_errors++;
> +		if (skb)
> +			cf->data[2] |= CAN_ERR_PROT_BIT1;
> +		break;
> +
> +	case BXCAN_LEC_BIT0_ERROR:
> +		netdev_dbg(ndev, "Bit error (dominant)\n");
> +		ndev->stats.tx_errors++;
> +		if (skb)
> +			cf->data[2] |= CAN_ERR_PROT_BIT0;
> +		break;
> +
> +	case BXCAN_LEC_CRC_ERROR:
> +		netdev_dbg(ndev, "CRC error\n");
> +		ndev->stats.rx_errors++;
> +		if (skb) {
> +			cf->data[2] |= CAN_ERR_PROT_BIT;
> +			cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ;
> +		}
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	if (skb) {
> +		int err;
> +
> +		err = can_rx_offload_queue_timestamp(&priv->offload, skb,
> +						     priv->timestamp);
> +		if (err)
> +			ndev->stats.rx_fifo_errors++;
> +	}
> +}
> +
> +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;

No IRQ, the driver returns IRQ_NONE here? Looks good!

> +
> +	esr = readl(&regs->esr);
> +	bxcan_handle_state_change(ndev, esr);
> +
> +	if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
> +		bxcan_handle_bus_err(ndev, esr);
> +
> +	msr |= BXCAN_MSR_ERRI;
> +	writel(msr, &regs->msr);
> +	can_rx_offload_irq_finish(&priv->offload);
> +
> +	return IRQ_HANDLED;
> +}

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung Nürnberg              | Phone: +49-5121-206917-129  |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

Powered by blists - more mailing lists