Re: [PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt lines

[Ciprian Marian Costea <ciprianmarian.costea@xxxxxxxxxxx>]

On 11/20/2024 2:20 PM, Marc Kleine-Budde wrote:
> On 19.11.2024 10:10:53, Ciprian Costea wrote:
> > From: Ciprian Marian Costea <ciprianmarian.costea@xxxxxxxxxxx>
> >
> > On S32G2/S32G3 SoC, there are separate interrupts
> > for state change, bus errors, MBs 0-7 and MBs 8-127 respectively.
> >
> > In order to handle this FlexCAN hardware particularity, reuse
> > the 'FLEXCAN_QUIRK_NR_IRQ_3' quirk provided by mcf5441x's irq
> > handling support.
> >
> > Additionally, introduce 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ' quirk,
> > which can be used in case there are two separate mailbox ranges
> > controlled by independent hardware interrupt lines, as it is
> > the case on S32G2/S32G3 SoC.
>
> Please move the quirk and quirk handling to the 2nd patch. The 3rd patch
> should only add the nxp,s32g2-flexcan compatible and the struct
> flexcan_devtype_data nxp_s32g2_devtype_data.

I will refactor accordingly in V2.

> > Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@xxxxxxxxxxx>
> > ---
> >   drivers/net/can/flexcan/flexcan-core.c | 25 +++++++++++++++++++++++--
> >   drivers/net/can/flexcan/flexcan.h      |  3 +++
> >   2 files changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
> > index f0dee04800d3..dc56d4a7d30b 100644
> > --- a/drivers/net/can/flexcan/flexcan-core.c
> > +++ b/drivers/net/can/flexcan/flexcan-core.c
> > @@ -390,9 +390,10 @@ static const struct flexcan_devtype_data nxp_s32g2_devtype_data = {
> >   	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
> >   		FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_BROKEN_PERR_STATE |
> >   		FLEXCAN_QUIRK_USE_RX_MAILBOX | FLEXCAN_QUIRK_SUPPORT_FD |
> > -		FLEXCAN_QUIRK_SUPPORT_ECC |
> > +		FLEXCAN_QUIRK_SUPPORT_ECC | FLEXCAN_QUIRK_NR_IRQ_3 |
> >   		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX |
> > -		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR,
> > +		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR |
> > +		FLEXCAN_QUIRK_SECONDARY_MB_IRQ,
> >   };
> >   
> >   static const struct can_bittiming_const flexcan_bittiming_const = {
> > @@ -1771,12 +1772,21 @@ static int flexcan_open(struct net_device *dev)
> >   			goto out_free_irq_boff;
> >   	}
> >   
> > +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
> > +		err = request_irq(priv->irq_secondary_mb,
> > +				  flexcan_irq, IRQF_SHARED, dev->name, dev);
> > +		if (err)
> > +			goto out_free_irq_err;
> > +	}
> > +
> >   	flexcan_chip_interrupts_enable(dev);
> >   
> >   	netif_start_queue(dev);
> >   
> >   	return 0;
> >   
> > + out_free_irq_err:
> > +	free_irq(priv->irq_err, dev);
> >    out_free_irq_boff:
> >   	free_irq(priv->irq_boff, dev);
> >    out_free_irq:
> > @@ -1808,6 +1818,9 @@ static int flexcan_close(struct net_device *dev)
> >   		free_irq(priv->irq_boff, dev);
> >   	}
> >   
> > +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ)
> > +		free_irq(priv->irq_secondary_mb, dev);
> > +
> >   	free_irq(dev->irq, dev);
> >   	can_rx_offload_disable(&priv->offload);
> >   	flexcan_chip_stop_disable_on_error(dev);
> > @@ -2197,6 +2210,14 @@ static int flexcan_probe(struct platform_device *pdev)
> >   		}
> >   	}
> >   
> > +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
> > +		priv->irq_secondary_mb = platform_get_irq(pdev, 3);
> > +		if (priv->irq_secondary_mb < 0) {
> > +			err = priv->irq_secondary_mb;
> > +			goto failed_platform_get_irq;
> > +		}
> > +	}
> > +
> >   	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SUPPORT_FD) {
> >   		priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD |
> >   			CAN_CTRLMODE_FD_NON_ISO;
> > diff --git a/drivers/net/can/flexcan/flexcan.h b/drivers/net/can/flexcan/flexcan.h
> > index 4933d8c7439e..d4b1a954c538 100644
> > --- a/drivers/net/can/flexcan/flexcan.h
> > +++ b/drivers/net/can/flexcan/flexcan.h
> > @@ -70,6 +70,8 @@
> >   #define FLEXCAN_QUIRK_SUPPORT_RX_FIFO BIT(16)
> >   /* Setup stop mode with ATF SCMI protocol to support wakeup */
> >   #define FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI BIT(17)
> > +/* Setup secondary mailbox interrupt */
>
> Describe why this quirk is needed. If you have a proper description in
> the commit message, you can copy it here.

I will add more motivation in the associated comment in V2.

> > +#define FLEXCAN_QUIRK_SECONDARY_MB_IRQ	BIT(18)
> >   
> >   struct flexcan_devtype_data {
> >   	u32 quirks;		/* quirks needed for different IP cores */
> > @@ -105,6 +107,7 @@ struct flexcan_priv {
> >   	struct regulator *reg_xceiver;
> >   	struct flexcan_stop_mode stm;
> >   
> > +	int irq_secondary_mb;
>
> Please place it after the irq_err, this way it's in order with the
> spread sheet.

Will refactor.


Best Regards,
Ciprian

> >   	int irq_boff;
> >   	int irq_err;
> >   
>
> regards,
> Marc