| Message ID | 20240119013326.3405484-1-jisheng.teoh@starfivetech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,RESEND] i2c: cadence: Add system suspend and resume PM support | expand |
Hi Ji Sheng, I'm not fully conviced here. On Fri, Jan 19, 2024 at 09:33:26AM +0800, Ji Sheng Teoh wrote: > Enable device system suspend and resume PM support, and mark the device > state as suspended during system suspend to reject any data transfer. > > Signed-off-by: Ji Sheng Teoh <jisheng.teoh@starfivetech.com> > --- > Initial v2 was archived, previous discussion can be found in [1]. > [1]: https://lore.kernel.org/all/20231209131516.1916550-1-jisheng.teoh@starfivetech.com/ > > Changes since v1: > - Add missing err assignment in cdns_i2c_resume(). > --- > drivers/i2c/busses/i2c-cadence.c | 33 ++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c > index de3f58b60dce..4bb7d6756947 100644 > --- a/drivers/i2c/busses/i2c-cadence.c > +++ b/drivers/i2c/busses/i2c-cadence.c > @@ -1176,6 +1176,18 @@ static int __maybe_unused cdns_i2c_runtime_suspend(struct device *dev) > return 0; > } > > +static int __maybe_unused cdns_i2c_suspend(struct device *dev) > +{ > + struct cdns_i2c *xi2c = dev_get_drvdata(dev); > + > + i2c_mark_adapter_suspended(&xi2c->adap); this should go before the return '0' after checking that cdns_i2c_runtime_suspend(), even though we know it always returns '0', we still can use likely or unlikely. > + if (!pm_runtime_status_suspended(dev)) > + return cdns_i2c_runtime_suspend(dev); > + > + return 0; > +} > + > /** > * cdns_i2c_init - Controller initialisation > * @id: Device private data structure > @@ -1219,7 +1231,28 @@ static int __maybe_unused cdns_i2c_runtime_resume(struct device *dev) > return 0; > } > > +static int __maybe_unused cdns_i2c_resume(struct device *dev) > +{ > + struct cdns_i2c *xi2c = dev_get_drvdata(dev); > + int err; > + > + err = cdns_i2c_runtime_resume(dev); > + if (err) > + return err; > + > + if (pm_runtime_status_suspended(dev)) { > + err = cdns_i2c_runtime_suspend(dev); > + if (err) > + return err; We call the cdns_i2c_resume() functions to come up from a suspended state. But, if we fail to resume, we call the suspend and return '0' (because this always returns '0'). In other words, if we take this path, we call resume, but we still end up suspended and return success. Andi > + } > + > + i2c_mark_adapter_resumed(&xi2c->adap); > + > + return 0; > +} > + > static const struct dev_pm_ops cdns_i2c_dev_pm_ops = { > + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(cdns_i2c_suspend, cdns_i2c_resume) > SET_RUNTIME_PM_OPS(cdns_i2c_runtime_suspend, > cdns_i2c_runtime_resume, NULL) > }; > -- > 2.43.0 >
Hi Andi, > > Hi Ji Sheng, > > I'm not fully conviced here. > > On Fri, Jan 19, 2024 at 09:33:26AM +0800, Ji Sheng Teoh wrote: > > Enable device system suspend and resume PM support, and mark the > > device state as suspended during system suspend to reject any data transfer. > > > > Signed-off-by: Ji Sheng Teoh <jisheng.teoh@starfivetech.com> > > --- > > Initial v2 was archived, previous discussion can be found in [1]. > > [1]: > > https://lore.kernel.org/all/20231209131516.1916550-1-jisheng.teoh@star > > fivetech.com/ > > > > Changes since v1: > > - Add missing err assignment in cdns_i2c_resume(). > > --- > > drivers/i2c/busses/i2c-cadence.c | 33 > > ++++++++++++++++++++++++++++++++ > > 1 file changed, 33 insertions(+) > > > > diff --git a/drivers/i2c/busses/i2c-cadence.c > > b/drivers/i2c/busses/i2c-cadence.c > > index de3f58b60dce..4bb7d6756947 100644 > > --- a/drivers/i2c/busses/i2c-cadence.c > > +++ b/drivers/i2c/busses/i2c-cadence.c > > @@ -1176,6 +1176,18 @@ static int __maybe_unused cdns_i2c_runtime_suspend(struct device *dev) > > return 0; > > } > > > > +static int __maybe_unused cdns_i2c_suspend(struct device *dev) { > > + struct cdns_i2c *xi2c = dev_get_drvdata(dev); > > + > > + i2c_mark_adapter_suspended(&xi2c->adap); > > this should go before the return '0' after checking that cdns_i2c_runtime_suspend(), even though we know it always returns '0', we > still can use likely or unlikely. > Ok, that works for me. Will move it right before return '0', and append 'unlikely' to the runtime suspend check. eg: +if (unlikely(!pm_runtime_status_suspended(dev))) + cdns_i2c_runtime_suspend(dev); + +i2c_mark_adapter_suspended(&xi2c->adap); + +return 0; > > + if (!pm_runtime_status_suspended(dev)) > > + return cdns_i2c_runtime_suspend(dev); > > + > > + return 0; > > +} > > + > > /** > > * cdns_i2c_init - Controller initialisation > > * @id: Device private data structure > > @@ -1219,7 +1231,28 @@ static int __maybe_unused cdns_i2c_runtime_resume(struct device *dev) > > return 0; > > } > > > > +static int __maybe_unused cdns_i2c_resume(struct device *dev) { > > + struct cdns_i2c *xi2c = dev_get_drvdata(dev); > > + int err; > > + > > + err = cdns_i2c_runtime_resume(dev); > > + if (err) > > + return err; > > + > > + if (pm_runtime_status_suspended(dev)) { > > + err = cdns_i2c_runtime_suspend(dev); > > + if (err) > > + return err; > > We call the cdns_i2c_resume() functions to come up from a suspended state. But, if we fail to resume, we call the suspend and return > '0' (because this always returns '0'). > > In other words, if we take this path, we call resume, but we still end up suspended and return success. > > Andi > My understanding is that during system level resume 'cdns_i2c_resume()', the i2c device itself can still be held in runtime suspend regardless of the change in system level PM. Looking back at this, we invoke cdns_i2c_runtime_resume() to enable clock and init the i2c device, the runtime PM state is still unchanged and kept suspended. pm_runtime_status_suspended() will be evaluated as true, and runtime suspend 'cdns_i2c_runtime_suspend()' is invoked to disable the clock. This balances the clock count enabled earlier. The runtime PM state is only resumed during cdns_i2c_master_xfer() through pm_runtime_resume_and_get(), and subsequently kept suspended through pm_runtime_put_autosuspend(). Since the cdns_i2c_runtime_suspend() always return '0', I will simplify them as follow: +if (pm_runtime_status_suspended(dev)) + cdns_i2c_runtime_suspend(dev); > > + } > > + > > + i2c_mark_adapter_resumed(&xi2c->adap); > > + > > + return 0; > > +} > > + > > static const struct dev_pm_ops cdns_i2c_dev_pm_ops = { > > + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(cdns_i2c_suspend, cdns_i2c_resume) > > SET_RUNTIME_PM_OPS(cdns_i2c_runtime_suspend, > > cdns_i2c_runtime_resume, NULL) > > }; > > -- > > 2.43.0 > >
Hi Ji Sheng, ... > > > +static int __maybe_unused cdns_i2c_resume(struct device *dev) { > > > + struct cdns_i2c *xi2c = dev_get_drvdata(dev); > > > + int err; > > > + > > > + err = cdns_i2c_runtime_resume(dev); > > > + if (err) > > > + return err; > > > + > > > + if (pm_runtime_status_suspended(dev)) { > > > + err = cdns_i2c_runtime_suspend(dev); > > > + if (err) > > > + return err; > > > > We call the cdns_i2c_resume() functions to come up from a suspended state. But, if we fail to resume, we call the suspend and return > > '0' (because this always returns '0'). > > > > In other words, if we take this path, we call resume, but we still end up suspended and return success. > > > > Andi > > > > My understanding is that during system level resume 'cdns_i2c_resume()', the i2c device itself can still be held in runtime suspend regardless of the change in system level PM. > Looking back at this, we invoke cdns_i2c_runtime_resume() to enable clock and init the i2c device, the runtime PM state is still unchanged and kept suspended. > pm_runtime_status_suspended() will be evaluated as true, and runtime suspend 'cdns_i2c_runtime_suspend()' is invoked to disable the clock. This balances the clock count enabled earlier. If this is your issue, what if we do not enable the clock during resume? and we just mark the device as resumed? > The runtime PM state is only resumed during cdns_i2c_master_xfer() through pm_runtime_resume_and_get(), and subsequently kept suspended through pm_runtime_put_autosuspend(). > Since the cdns_i2c_runtime_suspend() always return '0', I will simplify them as follow: > +if (pm_runtime_status_suspended(dev)) > + cdns_i2c_runtime_suspend(dev); I'd prefer checking the error value, even though we are sure on the expected return. It's more future proof. Andi > > > + } > > > + > > > + i2c_mark_adapter_resumed(&xi2c->adap); > > > + > > > + return 0; > > > +}
Hi Andi, > > ... > > > > > +static int __maybe_unused cdns_i2c_resume(struct device *dev) { > > > > + struct cdns_i2c *xi2c = dev_get_drvdata(dev); > > > > + int err; > > > > + > > > > + err = cdns_i2c_runtime_resume(dev); > > > > + if (err) > > > > + return err; > > > > + > > > > + if (pm_runtime_status_suspended(dev)) { > > > > + err = cdns_i2c_runtime_suspend(dev); > > > > + if (err) > > > > + return err; > > > > > > We call the cdns_i2c_resume() functions to come up from a suspended > > > state. But, if we fail to resume, we call the suspend and return '0' (because this always returns '0'). > > > > > > In other words, if we take this path, we call resume, but we still end up suspended and return success. > > > > > > Andi > > > > > > > My understanding is that during system level resume 'cdns_i2c_resume()', the i2c device itself can still be held in runtime suspend > regardless of the change in system level PM. > > Looking back at this, we invoke cdns_i2c_runtime_resume() to enable clock and init the i2c device, the runtime PM state is still > unchanged and kept suspended. > > pm_runtime_status_suspended() will be evaluated as true, and runtime suspend 'cdns_i2c_runtime_suspend()' is invoked to > disable the clock. This balances the clock count enabled earlier. > > If this is your issue, what if we do not enable the clock during resume? and we just mark the device as resumed? > That will work as well. The i2c device will be runtime resumed again during cdns_i2c_master_xfer() anyway, but thought that it would be a good idea to check if the i2c device is able runtime resume during a system level resume. > > The runtime PM state is only resumed during cdns_i2c_master_xfer() through pm_runtime_resume_and_get(), and subsequently > kept suspended through pm_runtime_put_autosuspend(). > > Since the cdns_i2c_runtime_suspend() always return '0', I will simplify them as follow: > > +if (pm_runtime_status_suspended(dev)) > > + cdns_i2c_runtime_suspend(dev); > > I'd prefer checking the error value, even though we are sure on the expected return. It's more future proof. > > Andi > Ok, I will keep the original changes. > > > > + } > > > > + > > > > + i2c_mark_adapter_resumed(&xi2c->adap); > > > > + > > > > + return 0; > > > > +}
Hi Jisheng, On Wed, Jan 24, 2024 at 04:46:43AM +0000, JiSheng Teoh wrote: > Hi Andi, > > > > ... > > > > > > > +static int __maybe_unused cdns_i2c_resume(struct device *dev) { > > > > > + struct cdns_i2c *xi2c = dev_get_drvdata(dev); > > > > > + int err; > > > > > + > > > > > + err = cdns_i2c_runtime_resume(dev); > > > > > + if (err) > > > > > + return err; > > > > > + > > > > > + if (pm_runtime_status_suspended(dev)) { > > > > > + err = cdns_i2c_runtime_suspend(dev); > > > > > + if (err) > > > > > + return err; > > > > > > > > We call the cdns_i2c_resume() functions to come up from a suspended > > > > state. But, if we fail to resume, we call the suspend and return '0' (because this always returns '0'). > > > > > > > > In other words, if we take this path, we call resume, but we still end up suspended and return success. > > > > > > > > Andi > > > > > > > > > > My understanding is that during system level resume 'cdns_i2c_resume()', the i2c device itself can still be held in runtime suspend > > regardless of the change in system level PM. > > > Looking back at this, we invoke cdns_i2c_runtime_resume() to enable clock and init the i2c device, the runtime PM state is still > > unchanged and kept suspended. > > > pm_runtime_status_suspended() will be evaluated as true, and runtime suspend 'cdns_i2c_runtime_suspend()' is invoked to > > disable the clock. This balances the clock count enabled earlier. > > > > If this is your issue, what if we do not enable the clock during resume? and we just mark the device as resumed? > > > That will work as well. The i2c device will be runtime resumed again during cdns_i2c_master_xfer() anyway, but thought that it > would be a good idea to check if the i2c device is able runtime resume during a system level resume. That's fine, I think it might work this way, as well, so let's keept it at your original implementation. If we save here, we add complication somewhere else. Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Before applying the patch I will read it again to make sure all is balanced. Thanks, Andi > > > The runtime PM state is only resumed during cdns_i2c_master_xfer() through pm_runtime_resume_and_get(), and subsequently > > kept suspended through pm_runtime_put_autosuspend(). > > > Since the cdns_i2c_runtime_suspend() always return '0', I will simplify them as follow: > > > +if (pm_runtime_status_suspended(dev)) > > > + cdns_i2c_runtime_suspend(dev); > > > > I'd prefer checking the error value, even though we are sure on the expected return. It's more future proof. > > > > Andi > > > Ok, I will keep the original changes. > > > > > > + } > > > > > + > > > > > + i2c_mark_adapter_resumed(&xi2c->adap); > > > > > + > > > > > + return 0; > > > > > +}
Hi Andi, > > > > > > ... > > > > > > > > > +static int __maybe_unused cdns_i2c_resume(struct device *dev) { > > > > > > + struct cdns_i2c *xi2c = dev_get_drvdata(dev); > > > > > > + int err; > > > > > > + > > > > > > + err = cdns_i2c_runtime_resume(dev); > > > > > > + if (err) > > > > > > + return err; > > > > > > + > > > > > > + if (pm_runtime_status_suspended(dev)) { > > > > > > + err = cdns_i2c_runtime_suspend(dev); > > > > > > + if (err) > > > > > > + return err; > > > > > > > > > > We call the cdns_i2c_resume() functions to come up from a > > > > > suspended state. But, if we fail to resume, we call the suspend and return '0' (because this always returns '0'). > > > > > > > > > > In other words, if we take this path, we call resume, but we still end up suspended and return success. > > > > > > > > > > Andi > > > > > > > > > > > > > My understanding is that during system level resume > > > > 'cdns_i2c_resume()', the i2c device itself can still be held in > > > > runtime suspend > > > regardless of the change in system level PM. > > > > Looking back at this, we invoke cdns_i2c_runtime_resume() to > > > > enable clock and init the i2c device, the runtime PM state is > > > > still > > > unchanged and kept suspended. > > > > pm_runtime_status_suspended() will be evaluated as true, and > > > > runtime suspend 'cdns_i2c_runtime_suspend()' is invoked to > > > disable the clock. This balances the clock count enabled earlier. > > > > > > If this is your issue, what if we do not enable the clock during resume? and we just mark the device as resumed? > > > > > That will work as well. The i2c device will be runtime resumed again > > during cdns_i2c_master_xfer() anyway, but thought that it would be a good idea to check if the i2c device is able runtime resume > during a system level resume. > > That's fine, I think it might work this way, as well, so let's keept it at your original implementation. > > If we save here, we add complication somewhere else. > > Reviewed-by: Andi Shyti <andi.shyti@kernel.org> > > Before applying the patch I will read it again to make sure all is balanced. > > Thanks, > Andi > Alright, thanks for your review. > > > > The runtime PM state is only resumed during cdns_i2c_master_xfer() > > > > through pm_runtime_resume_and_get(), and subsequently > > > kept suspended through pm_runtime_put_autosuspend(). > > > > Since the cdns_i2c_runtime_suspend() always return '0', I will simplify them as follow: > > > > +if (pm_runtime_status_suspended(dev)) > > > > + cdns_i2c_runtime_suspend(dev); > > > > > > I'd prefer checking the error value, even though we are sure on the expected return. It's more future proof. > > > > > > Andi > > > > > Ok, I will keep the original changes. > > > > > > > > + } > > > > > > + > > > > > > + i2c_mark_adapter_resumed(&xi2c->adap); > > > > > > + > > > > > > + return 0; > > > > > > +}
Hi On Fri, 19 Jan 2024 09:33:26 +0800, Ji Sheng Teoh wrote: > Enable device system suspend and resume PM support, and mark the device > state as suspended during system suspend to reject any data transfer. > > Applied to i2c/i2c-host on git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git Thank you, Andi Patches applied =============== [1/1] i2c: cadence: Add system suspend and resume PM support commit: 747bdf912e22732e8de9bd04a2d3e387055604a8
diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c index de3f58b60dce..4bb7d6756947 100644 --- a/drivers/i2c/busses/i2c-cadence.c +++ b/drivers/i2c/busses/i2c-cadence.c @@ -1176,6 +1176,18 @@ static int __maybe_unused cdns_i2c_runtime_suspend(struct device *dev) return 0; } +static int __maybe_unused cdns_i2c_suspend(struct device *dev) +{ + struct cdns_i2c *xi2c = dev_get_drvdata(dev); + + i2c_mark_adapter_suspended(&xi2c->adap); + + if (!pm_runtime_status_suspended(dev)) + return cdns_i2c_runtime_suspend(dev); + + return 0; +} + /** * cdns_i2c_init - Controller initialisation * @id: Device private data structure @@ -1219,7 +1231,28 @@ static int __maybe_unused cdns_i2c_runtime_resume(struct device *dev) return 0; } +static int __maybe_unused cdns_i2c_resume(struct device *dev) +{ + struct cdns_i2c *xi2c = dev_get_drvdata(dev); + int err; + + err = cdns_i2c_runtime_resume(dev); + if (err) + return err; + + if (pm_runtime_status_suspended(dev)) { + err = cdns_i2c_runtime_suspend(dev); + if (err) + return err; + } + + i2c_mark_adapter_resumed(&xi2c->adap); + + return 0; +} + static const struct dev_pm_ops cdns_i2c_dev_pm_ops = { + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(cdns_i2c_suspend, cdns_i2c_resume) SET_RUNTIME_PM_OPS(cdns_i2c_runtime_suspend, cdns_i2c_runtime_resume, NULL) };
Enable device system suspend and resume PM support, and mark the device state as suspended during system suspend to reject any data transfer. Signed-off-by: Ji Sheng Teoh <jisheng.teoh@starfivetech.com> --- Initial v2 was archived, previous discussion can be found in [1]. [1]: https://lore.kernel.org/all/20231209131516.1916550-1-jisheng.teoh@starfivetech.com/ Changes since v1: - Add missing err assignment in cdns_i2c_resume(). --- drivers/i2c/busses/i2c-cadence.c | 33 ++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)