On 11/19/2024 11:26 AM, Vincent Mailhol wrote:
On 19/11/2024 at 17:10, 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.
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;
+ }
Is the logic here correct?
request_irq(priv->irq_err, flexcan_irq, IRQF_SHARED, dev->name, dev);
is called only if the device has the FLEXCAN_QUIRK_NR_IRQ_3 quirk.
So, if the device has the FLEXCAN_QUIRK_SECONDARY_MB_IRQ but not the
FLEXCAN_QUIRK_NR_IRQ_3, you may end up trying to free an irq which was
not initialized.
Did you confirm if it is safe to call free_irq() on an uninitialized irq?
(and I can see that currently there is no such device with
FLEXCAN_QUIRK_SECONDARY_MB_IRQ but without FLEXCAN_QUIRK_NR_IRQ_3, but
who knows if such device will be introduced in the future?)
Hello Vincent,
Thanks for your review. Indeed this seems to be an incorrect logic since
I do not want to create any dependency between 'FLEXCAN_QUIRK_NR_IRQ_3'
and 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ'.
I will change the impacted section to:
if (err) {
if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
goto out_free_irq_err;
else
goto out_free_irq;
}
Best Regards,
Ciprian
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 */
+#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;
int irq_boff;
int irq_err;
Yours sincerely,
Vincent Mailhol
