Re: [PATCH v4 2/2] can: m_can: add ti,am62-m_can compatible with out-of-band wakeup support

[Kendall Willis]

On 10:55-20260903, Markus Schneider-Pargmann wrote:
> On Wed Sep 2, 2026 at 11:03 PM CEST, Kendall Willis wrote:
> > In TI AM62X, AM62A, and AM62P SoCs, the M_CAN controller is a part of
> > the MCU power domain which is OFF in the deepest low power states.
> > However, the m_can pins are able to wakeup the system from states where
> > the MCU domain is OFF through its out-of-band wakeup functionality.
> 
> There are m_can devices in the main domain as well. Does this apply to
> these as well?
>

This doesn't apply to the m_can devices in the main domain, so I'll make
the compatible to be more specific. Maybe something like
"ti,am62-mcu-mcan".

> >
> > Introduce the ti,am62-m_can compatible with match data to identify the
> > out-of-band wakeup capability for TI AM62 SoCs. During suspend, set the
> > out-of-band wakeup flag when the device is configured as a wakeup
> > source, the "wakeup" pinctrl state is present, and the match data
> > indicates out-of-band wakeup support.
> >
> > Signed-off-by: Kendall Willis <k-willis@xxxxxx>
> > ---
> >  drivers/net/can/m_can/m_can.c          | 7 ++++++-
> >  drivers/net/can/m_can/m_can.h          | 4 ++++
> >  drivers/net/can/m_can/m_can_platform.c | 5 +++++
> >  3 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> > index 16f80607e150e2ca2950616ff2c39b7aa13710b4..0788816d881ef8ba84ad5a84a6d2d3e60517ea7a 100644
> > --- a/drivers/net/can/m_can/m_can.c
> > +++ b/drivers/net/can/m_can/m_can.c
> > @@ -2598,6 +2598,7 @@ int m_can_class_suspend(struct device *dev)
> >  {
> >  	struct m_can_classdev *cdev = dev_get_drvdata(dev);
> >  	struct net_device *ndev = cdev->net;
> > +	const struct m_can_pdata *pdata = of_device_get_match_data(dev);
> 
> The core m_can driver works for all m_can forms, tcan, platform and pci.
> I think it would be cleaner if you parse this in the platform driver and
> set it in m_can_classdev instead. Otherwise of_match_data has to be the
> same for all drivers which is not obvious when you are editing tcan for
> example.
> 

That makes sense. I'll add it to m_can_classdev in the platform driver
in the next version.

> >  	int ret = 0;
> >  
> >  	if (netif_running(ndev)) {
> > @@ -2622,8 +2623,12 @@ int m_can_class_suspend(struct device *dev)
> >  		cdev->can.state = CAN_STATE_SLEEPING;
> >  	}
> >  
> > -	if (!m_can_class_wakeup_pinctrl_enabled(cdev))
> > +	if (m_can_class_wakeup_pinctrl_enabled(cdev)) {
> > +		if (pdata && pdata->out_band_wakeup)
> > +			device_set_out_band_wakeup(dev);
> > +	} else {
> >  		pinctrl_pm_select_sleep_state(dev);
> > +	}
> 
> I just noticed these are called even if ret is not 0, so suspend failed.
> This shouldn't happen, but not a problem of your patch. But you could
> add a fix for that.
> 

Yep, I could add a fix for that.

> >  
> >  	return ret;
> >  }
> > diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> > index 4743342b2fba3823819b9e2bc5b085cc0fe6d1e2..486d808bc677482acf446b1450c3a9b6b7405178 100644
> > --- a/drivers/net/can/m_can/m_can.h
> > +++ b/drivers/net/can/m_can/m_can.h
> > @@ -134,6 +134,10 @@ struct m_can_classdev {
> >  	struct pinctrl_state *pinctrl_state_wakeup;
> >  };
> >  
> > +struct m_can_pdata {
> > +	bool out_band_wakeup;
> > +};
> > +
> >  struct m_can_classdev *m_can_class_allocate_dev(struct device *dev, int sizeof_priv);
> >  void m_can_class_free_dev(struct net_device *net);
> >  int m_can_class_register(struct m_can_classdev *cdev);
> > diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c
> > index 56da411878af0185ff9cc512e325137428a68255..ae2d89b65fe8da65b1ca73cbf80cd8ee8483e2e8 100644
> > --- a/drivers/net/can/m_can/m_can_platform.c
> > +++ b/drivers/net/can/m_can/m_can_platform.c
> > @@ -218,8 +218,13 @@ static const struct dev_pm_ops m_can_pmops = {
> >  	SET_SYSTEM_SLEEP_PM_OPS(m_can_suspend, m_can_resume)
> >  };
> >  
> > +static const struct m_can_pdata m_can_plat_am62 = {
> 
> I don't like the pdata name here, this suggests to be platform data, but
> this is of match data instead. Maybe of_data?

I can change it to of_data. I've also seen soc_data as well, but that
doesn't really work since the compatible is per power domain.

Best,
Kendall