| Message ID | 20191127055334.1476-5-qiangqing.zhang@nxp.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | can: flexcan: fixes for stop mode | expand |
Hi Marc, You may have missed this patch, any comments about it? Best Regards, Joakim Zhang > -----Original Message----- > From: Joakim Zhang <qiangqing.zhang@nxp.com> > Sent: 2019年11月27日 13:57 > To: mkl@pengutronix.de; sean@geanix.com; linux-can@vger.kernel.org > Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org; Joakim Zhang > <qiangqing.zhang@nxp.com> > Subject: [PATCH V2 4/4] can: flexcan: add LPSR mode support > > For i.MX7D LPSR mode, the controller will lost power and got the configuration > state lost after system resume back. (coming i.MX8QM/QXP will also > completely power off the domain, the controller state will be lost and needs > restore). > So we need to set pinctrl state again and re-start chip to do re-configuration > after resume. > > For wakeup case, it should not set pinctrl to sleep state by > pinctrl_pm_select_sleep_state. > For interface is not up before suspend case, we don't need re-configure as it > will be configured by user later by interface up. > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > ------ > ChangeLog: > V1->V2: no change. > --- > drivers/net/can/flexcan.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c index > d178146b3da5..d1509cffdd24 100644 > --- a/drivers/net/can/flexcan.c > +++ b/drivers/net/can/flexcan.c > @@ -26,6 +26,7 @@ > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > #include <linux/regulator/consumer.h> > +#include <linux/pinctrl/consumer.h> > #include <linux/regmap.h> > > #define DRV_NAME "flexcan" > @@ -1707,7 +1708,7 @@ static int __maybe_unused flexcan_suspend(struct > device *device) { > struct net_device *dev = dev_get_drvdata(device); > struct flexcan_priv *priv = netdev_priv(dev); > - int err = 0; > + int err; > > if (netif_running(dev)) { > /* if wakeup is enabled, enter stop mode @@ -1719,25 +1720,27 > @@ static int __maybe_unused flexcan_suspend(struct device *device) > if (err) > return err; > } else { > - err = flexcan_chip_disable(priv); > + flexcan_chip_stop(dev); > + > + err = pm_runtime_force_suspend(device); > if (err) > return err; > > - err = pm_runtime_force_suspend(device); > + pinctrl_pm_select_sleep_state(device); > } > netif_stop_queue(dev); > netif_device_detach(dev); > } > priv->can.state = CAN_STATE_SLEEPING; > > - return err; > + return 0; > } > > static int __maybe_unused flexcan_resume(struct device *device) { > struct net_device *dev = dev_get_drvdata(device); > struct flexcan_priv *priv = netdev_priv(dev); > - int err = 0; > + int err; > > priv->can.state = CAN_STATE_ERROR_ACTIVE; > if (netif_running(dev)) { > @@ -1749,15 +1752,19 @@ static int __maybe_unused > flexcan_resume(struct device *device) > if (err) > return err; > } else { > + pinctrl_pm_select_default_state(device); > + > err = pm_runtime_force_resume(device); > if (err) > return err; > > - err = flexcan_chip_enable(priv); > + err = flexcan_chip_start(dev); > + if (err) > + return err; > } > } > > - return err; > + return 0; > } > > static int __maybe_unused flexcan_runtime_suspend(struct device *device) > -- > 2.17.1
On 11/27/19 6:56 AM, Joakim Zhang wrote: > For i.MX7D LPSR mode, the controller will lost power and got the > configuration state lost after system resume back. (coming i.MX8QM/QXP > will also completely power off the domain, the controller state will be > lost and needs restore). > So we need to set pinctrl state again and re-start chip to do > re-configuration after resume. > > For wakeup case, it should not set pinctrl to sleep state by > pinctrl_pm_select_sleep_state. > For interface is not up before suspend case, we don't need > re-configure as it will be configured by user later by interface up. > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > ------ > ChangeLog: > V1->V2: no change. > --- > drivers/net/can/flexcan.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c > index d178146b3da5..d1509cffdd24 100644 > --- a/drivers/net/can/flexcan.c > +++ b/drivers/net/can/flexcan.c > @@ -26,6 +26,7 @@ > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > #include <linux/regulator/consumer.h> > +#include <linux/pinctrl/consumer.h> > #include <linux/regmap.h> > > #define DRV_NAME "flexcan" > @@ -1707,7 +1708,7 @@ static int __maybe_unused flexcan_suspend(struct device *device) > { > struct net_device *dev = dev_get_drvdata(device); > struct flexcan_priv *priv = netdev_priv(dev); > - int err = 0; > + int err; > > if (netif_running(dev)) { > /* if wakeup is enabled, enter stop mode > @@ -1719,25 +1720,27 @@ static int __maybe_unused flexcan_suspend(struct device *device) > if (err) > return err; > } else { > - err = flexcan_chip_disable(priv); > + flexcan_chip_stop(dev); > + > + err = pm_runtime_force_suspend(device); > if (err) > return err; > > - err = pm_runtime_force_suspend(device); > + pinctrl_pm_select_sleep_state(device); Please add error handling for pinctrl_pm_select_sleep_state(). > } > netif_stop_queue(dev); > netif_device_detach(dev); > } > priv->can.state = CAN_STATE_SLEEPING; > > - return err; > + return 0; > } > > static int __maybe_unused flexcan_resume(struct device *device) > { > struct net_device *dev = dev_get_drvdata(device); > struct flexcan_priv *priv = netdev_priv(dev); > - int err = 0; > + int err; > > priv->can.state = CAN_STATE_ERROR_ACTIVE; > if (netif_running(dev)) { > @@ -1749,15 +1752,19 @@ static int __maybe_unused flexcan_resume(struct device *device) > if (err) > return err; > } else { > + pinctrl_pm_select_default_state(device); same here > + > err = pm_runtime_force_resume(device); > if (err) > return err; > > - err = flexcan_chip_enable(priv); > + err = flexcan_chip_start(dev); > + if (err) > + return err; > } > } > > - return err; > + return 0; > } > > static int __maybe_unused flexcan_runtime_suspend(struct device *device) > Marc
On 11/27/19 6:56 AM, Joakim Zhang wrote: > For i.MX7D LPSR mode, the controller will lost power and got the > configuration state lost after system resume back. (coming i.MX8QM/QXP > will also completely power off the domain, the controller state will be > lost and needs restore). > So we need to set pinctrl state again and re-start chip to do > re-configuration after resume. > > For wakeup case, it should not set pinctrl to sleep state by > pinctrl_pm_select_sleep_state. > For interface is not up before suspend case, we don't need > re-configure as it will be configured by user later by interface up. > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > ------ > ChangeLog: > V1->V2: no change. > --- > drivers/net/can/flexcan.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c > index d178146b3da5..d1509cffdd24 100644 > --- a/drivers/net/can/flexcan.c > +++ b/drivers/net/can/flexcan.c > @@ -26,6 +26,7 @@ > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > #include <linux/regulator/consumer.h> > +#include <linux/pinctrl/consumer.h> > #include <linux/regmap.h> > > #define DRV_NAME "flexcan" > @@ -1707,7 +1708,7 @@ static int __maybe_unused flexcan_suspend(struct device *device) > { > struct net_device *dev = dev_get_drvdata(device); > struct flexcan_priv *priv = netdev_priv(dev); > - int err = 0; > + int err; > > if (netif_running(dev)) { > /* if wakeup is enabled, enter stop mode > @@ -1719,25 +1720,27 @@ static int __maybe_unused flexcan_suspend(struct device *device) > if (err) > return err; > } else { > - err = flexcan_chip_disable(priv); > + flexcan_chip_stop(dev); chip_stop calls chip_disable, but doesn't propagate the error value. Please create a seperate patch to propagate the error value of chip_stop(). Marc
> -----Original Message----- > From: Marc Kleine-Budde <mkl@pengutronix.de> > Sent: 2019年12月4日 17:00 > To: Joakim Zhang <qiangqing.zhang@nxp.com>; sean@geanix.com; > linux-can@vger.kernel.org > Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org > Subject: Re: [PATCH V2 4/4] can: flexcan: add LPSR mode support > > On 11/27/19 6:56 AM, Joakim Zhang wrote: > > For i.MX7D LPSR mode, the controller will lost power and got the > > configuration state lost after system resume back. (coming i.MX8QM/QXP > > will also completely power off the domain, the controller state will > > be lost and needs restore). > > So we need to set pinctrl state again and re-start chip to do > > re-configuration after resume. > > > > For wakeup case, it should not set pinctrl to sleep state by > > pinctrl_pm_select_sleep_state. > > For interface is not up before suspend case, we don't need > > re-configure as it will be configured by user later by interface up. > > > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > > ------ > > ChangeLog: > > V1->V2: no change. > > --- > > drivers/net/can/flexcan.c | 21 ++++++++++++++------- > > 1 file changed, 14 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c > > index d178146b3da5..d1509cffdd24 100644 > > --- a/drivers/net/can/flexcan.c > > +++ b/drivers/net/can/flexcan.c > > @@ -26,6 +26,7 @@ > > #include <linux/platform_device.h> > > #include <linux/pm_runtime.h> > > #include <linux/regulator/consumer.h> > > +#include <linux/pinctrl/consumer.h> > > #include <linux/regmap.h> > > > > #define DRV_NAME "flexcan" > > @@ -1707,7 +1708,7 @@ static int __maybe_unused > flexcan_suspend(struct > > device *device) { > > struct net_device *dev = dev_get_drvdata(device); > > struct flexcan_priv *priv = netdev_priv(dev); > > - int err = 0; > > + int err; > > > > if (netif_running(dev)) { > > /* if wakeup is enabled, enter stop mode @@ -1719,25 +1720,27 > @@ > > static int __maybe_unused flexcan_suspend(struct device *device) > > if (err) > > return err; > > } else { > > - err = flexcan_chip_disable(priv); > > + flexcan_chip_stop(dev); > > + > > + err = pm_runtime_force_suspend(device); > > if (err) > > return err; > > > > - err = pm_runtime_force_suspend(device); > > + pinctrl_pm_select_sleep_state(device); > > Please add error handling for pinctrl_pm_select_sleep_state(). Got it. > > } > > netif_stop_queue(dev); > > netif_device_detach(dev); > > } > > priv->can.state = CAN_STATE_SLEEPING; > > > > - return err; > > + return 0; > > } > > > > static int __maybe_unused flexcan_resume(struct device *device) { > > struct net_device *dev = dev_get_drvdata(device); > > struct flexcan_priv *priv = netdev_priv(dev); > > - int err = 0; > > + int err; > > > > priv->can.state = CAN_STATE_ERROR_ACTIVE; > > if (netif_running(dev)) { > > @@ -1749,15 +1752,19 @@ static int __maybe_unused > flexcan_resume(struct device *device) > > if (err) > > return err; > > } else { > > + pinctrl_pm_select_default_state(device); > > same here Got it. > > + > > err = pm_runtime_force_resume(device); > > if (err) > > return err; > > > > - err = flexcan_chip_enable(priv); > > + err = flexcan_chip_start(dev); > > + if (err) > > + return err; > > } > > } > > > > - return err; > > + return 0; > > } > > > > static int __maybe_unused flexcan_runtime_suspend(struct device > > *device) > > Best Regards, Joakim Zhang > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung West/Dortmund | Phone: +49-231-2826-924 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
> -----Original Message----- > From: Marc Kleine-Budde <mkl@pengutronix.de> > Sent: 2019年12月4日 17:21 > To: Joakim Zhang <qiangqing.zhang@nxp.com>; sean@geanix.com; > linux-can@vger.kernel.org > Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org > Subject: Re: [PATCH V2 4/4] can: flexcan: add LPSR mode support > > On 11/27/19 6:56 AM, Joakim Zhang wrote: > > For i.MX7D LPSR mode, the controller will lost power and got the > > configuration state lost after system resume back. (coming i.MX8QM/QXP > > will also completely power off the domain, the controller state will > > be lost and needs restore). > > So we need to set pinctrl state again and re-start chip to do > > re-configuration after resume. > > > > For wakeup case, it should not set pinctrl to sleep state by > > pinctrl_pm_select_sleep_state. > > For interface is not up before suspend case, we don't need > > re-configure as it will be configured by user later by interface up. > > > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > > ------ > > ChangeLog: > > V1->V2: no change. > > --- > > drivers/net/can/flexcan.c | 21 ++++++++++++++------- > > 1 file changed, 14 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c > > index d178146b3da5..d1509cffdd24 100644 > > --- a/drivers/net/can/flexcan.c > > +++ b/drivers/net/can/flexcan.c > > @@ -26,6 +26,7 @@ > > #include <linux/platform_device.h> > > #include <linux/pm_runtime.h> > > #include <linux/regulator/consumer.h> > > +#include <linux/pinctrl/consumer.h> > > #include <linux/regmap.h> > > > > #define DRV_NAME "flexcan" > > @@ -1707,7 +1708,7 @@ static int __maybe_unused > flexcan_suspend(struct > > device *device) { > > struct net_device *dev = dev_get_drvdata(device); > > struct flexcan_priv *priv = netdev_priv(dev); > > - int err = 0; > > + int err; > > > > if (netif_running(dev)) { > > /* if wakeup is enabled, enter stop mode @@ -1719,25 +1720,27 > @@ > > static int __maybe_unused flexcan_suspend(struct device *device) > > if (err) > > return err; > > } else { > > - err = flexcan_chip_disable(priv); > > + flexcan_chip_stop(dev); > > chip_stop calls chip_disable, but doesn't propagate the error value. > Please create a seperate patch to propagate the error value of chip_stop(). Okay, I will do it. Best Regards, Joakim Zhang > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung West/Dortmund | Phone: +49-231-2826-924 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
> -----Original Message----- > From: Marc Kleine-Budde <mkl@pengutronix.de> > Sent: 2019年12月4日 17:21 > To: Joakim Zhang <qiangqing.zhang@nxp.com>; sean@geanix.com; > linux-can@vger.kernel.org > Cc: dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org > Subject: Re: [PATCH V2 4/4] can: flexcan: add LPSR mode support > > On 11/27/19 6:56 AM, Joakim Zhang wrote: > > For i.MX7D LPSR mode, the controller will lost power and got the > > configuration state lost after system resume back. (coming i.MX8QM/QXP > > will also completely power off the domain, the controller state will > > be lost and needs restore). > > So we need to set pinctrl state again and re-start chip to do > > re-configuration after resume. > > > > For wakeup case, it should not set pinctrl to sleep state by > > pinctrl_pm_select_sleep_state. > > For interface is not up before suspend case, we don't need > > re-configure as it will be configured by user later by interface up. > > > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > > ------ > > ChangeLog: > > V1->V2: no change. > > --- > > drivers/net/can/flexcan.c | 21 ++++++++++++++------- > > 1 file changed, 14 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c > > index d178146b3da5..d1509cffdd24 100644 > > --- a/drivers/net/can/flexcan.c > > +++ b/drivers/net/can/flexcan.c > > @@ -26,6 +26,7 @@ > > #include <linux/platform_device.h> > > #include <linux/pm_runtime.h> > > #include <linux/regulator/consumer.h> > > +#include <linux/pinctrl/consumer.h> > > #include <linux/regmap.h> > > > > #define DRV_NAME "flexcan" > > @@ -1707,7 +1708,7 @@ static int __maybe_unused > flexcan_suspend(struct > > device *device) { > > struct net_device *dev = dev_get_drvdata(device); > > struct flexcan_priv *priv = netdev_priv(dev); > > - int err = 0; > > + int err; > > > > if (netif_running(dev)) { > > /* if wakeup is enabled, enter stop mode @@ -1719,25 +1720,27 > @@ > > static int __maybe_unused flexcan_suspend(struct device *device) > > if (err) > > return err; > > } else { > > - err = flexcan_chip_disable(priv); > > + flexcan_chip_stop(dev); > > chip_stop calls chip_disable, but doesn't propagate the error value. > Please create a seperate patch to propagate the error value of chip_stop(). Hi Marc, I found not only chip_disable should propagate the error value, others chip_freeze, transceiver_disable also need.' I cook a patch below, is it fine for you? diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c index 19602b77907f..c5e4b6928dee 100644 --- a/drivers/net/can/flexcan.c +++ b/drivers/net/can/flexcan.c @@ -1263,14 +1263,19 @@ static int flexcan_chip_start(struct net_device *dev) * * this functions is entered with clocks enabled */ -static void flexcan_chip_stop(struct net_device *dev) +static int flexcan_chip_stop(struct net_device *dev) { struct flexcan_priv *priv = netdev_priv(dev); struct flexcan_regs __iomem *regs = priv->regs; + int err; /* freeze + disable module */ - flexcan_chip_freeze(priv); - flexcan_chip_disable(priv); + err = flexcan_chip_freeze(priv); + if (err) + return err; + err = flexcan_chip_disable(priv); + if (err) + goto out_chip_unfreeze; /* Disable all interrupts */ priv->write(0, ®s->imask2); @@ -1278,8 +1283,19 @@ static void flexcan_chip_stop(struct net_device *dev) priv->write(priv->reg_ctrl_default & ~FLEXCAN_CTRL_ERR_ALL, ®s->ctrl); - flexcan_transceiver_disable(priv); + err = flexcan_transceiver_disable(priv); + if (err) + goto out_chip_enable; + priv->can.state = CAN_STATE_STOPPED; + + return 0; + +out_chip_enable: + flexcan_chip_enable(priv); +out_chip_unfreeze: + flexcan_chip_unfreeze(priv); + return err; } static int flexcan_open(struct net_device *dev) Best Regards, Joakim Zhang > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung West/Dortmund | Phone: +49-231-2826-924 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c index d178146b3da5..d1509cffdd24 100644 --- a/drivers/net/can/flexcan.c +++ b/drivers/net/can/flexcan.c @@ -26,6 +26,7 @@ #include <linux/platform_device.h> #include <linux/pm_runtime.h> #include <linux/regulator/consumer.h> +#include <linux/pinctrl/consumer.h> #include <linux/regmap.h> #define DRV_NAME "flexcan" @@ -1707,7 +1708,7 @@ static int __maybe_unused flexcan_suspend(struct device *device) { struct net_device *dev = dev_get_drvdata(device); struct flexcan_priv *priv = netdev_priv(dev); - int err = 0; + int err; if (netif_running(dev)) { /* if wakeup is enabled, enter stop mode @@ -1719,25 +1720,27 @@ static int __maybe_unused flexcan_suspend(struct device *device) if (err) return err; } else { - err = flexcan_chip_disable(priv); + flexcan_chip_stop(dev); + + err = pm_runtime_force_suspend(device); if (err) return err; - err = pm_runtime_force_suspend(device); + pinctrl_pm_select_sleep_state(device); } netif_stop_queue(dev); netif_device_detach(dev); } priv->can.state = CAN_STATE_SLEEPING; - return err; + return 0; } static int __maybe_unused flexcan_resume(struct device *device) { struct net_device *dev = dev_get_drvdata(device); struct flexcan_priv *priv = netdev_priv(dev); - int err = 0; + int err; priv->can.state = CAN_STATE_ERROR_ACTIVE; if (netif_running(dev)) { @@ -1749,15 +1752,19 @@ static int __maybe_unused flexcan_resume(struct device *device) if (err) return err; } else { + pinctrl_pm_select_default_state(device); + err = pm_runtime_force_resume(device); if (err) return err; - err = flexcan_chip_enable(priv); + err = flexcan_chip_start(dev); + if (err) + return err; } } - return err; + return 0; } static int __maybe_unused flexcan_runtime_suspend(struct device *device)
For i.MX7D LPSR mode, the controller will lost power and got the configuration state lost after system resume back. (coming i.MX8QM/QXP will also completely power off the domain, the controller state will be lost and needs restore). So we need to set pinctrl state again and re-start chip to do re-configuration after resume. For wakeup case, it should not set pinctrl to sleep state by pinctrl_pm_select_sleep_state. For interface is not up before suspend case, we don't need re-configure as it will be configured by user later by interface up. Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> ------ ChangeLog: V1->V2: no change. --- drivers/net/can/flexcan.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)