RE: [PATCH v2 4/4] can: flexcan: add wakeup support for imx95

[Bough Chen]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> Sent: 2024年7月16日 22:46
> To: Frank Li <frank.li@xxxxxxx>
> Cc: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>; David S. Miller
> <davem@xxxxxxxxxxxxx>; Eric Dumazet <edumazet@xxxxxxxxxx>; Jakub
> Kicinski <kuba@xxxxxxxxxx>; Paolo Abeni <pabeni@xxxxxxxxxx>; Rob Herring
> <robh@xxxxxxxxxx>; Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>; linux-can@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Bough Chen
> <haibo.chen@xxxxxxx>; imx@xxxxxxxxxxxxxxx; Han Xu <han.xu@xxxxxxx>
> Subject: Re: [PATCH v2 4/4] can: flexcan: add wakeup support for imx95
> 
> On 16.07.2024 10:40:31, Frank Li wrote:
> > > > @@ -2330,9 +2366,12 @@ static int __maybe_unused
> flexcan_noirq_resume(struct device *device)
> > > >  	if (netif_running(dev)) {
> > > >  		int err;
> > > >
> > > > -		err = pm_runtime_force_resume(device);
> > > > -		if (err)
> > > > -			return err;
> > > > +		if (!(device_may_wakeup(device) &&
> > >                       ^^^^^^^^^^^^^^^^^^^^^^^^
> > >
> > > Where does this come from?
> >
> > include/linux/pm_wakeup.h
> >
> > static inline bool device_may_wakeup(struct device *dev)
> > {
> >         return dev->power.can_wakeup && !!dev->power.wakeup;
> > }
> 
> Sorry for the confusion. I wanted to point out, that the original driver doesn't
> have the check to device_may_wakeup(). Why was this added?

Hi Marc,

Here add this to make sure for CAN with flag FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI and really used as wakeup source, do not need to call pm_runtime_force_resume(), keep it align with
what we do in flexcan_noirq_suspend.
As the comment in flexcan_noirq_suspend, CAN with flag FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI, when used as wakeup source, need to keep CAN clock on when system suspend, let ATF part logic works, detail steps please refer to this patch commit log. Whether gate off the CAN clock or not depends on the Hardware design. So for this case, in flexcan_noirq_suspend, directly return0, do not call the pm_runtime_force_suspend(), then in flexcan_noirq_resume(), use the same logic to skip the pm_runtime_force_resume().

Best Regards
Haibo Chen 
> 
> > >
> > > > +		      priv->devtype_data.quirks &
> FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI)) {
> > > > +			err = pm_runtime_force_resume(device);
> > > > +			if (err)
> > > > +				return err;
> > > > +		}
> > > >
> > > >  		if (device_may_wakeup(device))
> > > >  			flexcan_enable_wakeup_irq(priv, false); diff --git
> > > > a/drivers/net/can/flexcan/flexcan.h
> > > > b/drivers/net/can/flexcan/flexcan.h
> > > > index 025c3417031f4..4933d8c7439e6 100644
> > > > --- a/drivers/net/can/flexcan/flexcan.h
> > > > +++ b/drivers/net/can/flexcan/flexcan.h
> > > > @@ -68,6 +68,8 @@
> > > >  #define FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR BIT(15)
> > > >  /* Device supports RX via FIFO */  #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)
> > > >
> > > >  struct flexcan_devtype_data {
> > > >  	u32 quirks;		/* quirks needed for different IP cores */
> 
> 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-9   |