| Message ID | 20231209131516.1916550-1-jisheng.teoh@starfivetech.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] i2c: cadence: Add system suspend and resume PM support | expand |
Hi Ji Sheng, On Sat, Dec 09, 2023 at 09:15:16PM +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> > --- > Changes since v1: > - Add missing err assignment in cdns_i2c_resume(). thanks for the quick version update. However, while it's nice to see such prompt proactivity, we also need to allow more time for others to review your change. Next time, please give it a bit more time before sending out version 2. :-) > --- > 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); > + > + 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) > +{ I am not really understanding what you are trying to do here: > + struct cdns_i2c *xi2c = dev_get_drvdata(dev); > + int err; > + > + err = cdns_i2c_runtime_resume(dev); First you try to resume... > + if (err) > + return err; > + > + if (pm_runtime_status_suspended(dev)) { ... then you check if you are suspended ... > + err = cdns_i2c_runtime_suspend(dev); ... and suspend again? Shouldn't this be _resume()? Thanks, Andi > + 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) > }; > -- > 2.25.1 >
On Sat, 9 Dec 2023 21:57:44 +0100 Andi Shyti <andi.shyti@kernel.org> wrote: > Hi Ji Sheng, > > On Sat, Dec 09, 2023 at 09:15:16PM +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> > > --- > > Changes since v1: > > - Add missing err assignment in cdns_i2c_resume(). > > thanks for the quick version update. However, while it's nice to > see such prompt proactivity, we also need to allow more time for > others to review your change. > > Next time, please give it a bit more time before sending out > version 2. :-) > Thanks, will take note of that. > > --- > > 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); > > + > > + 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) > > +{ > > I am not really understanding what you are trying to do here: > > > + struct cdns_i2c *xi2c = dev_get_drvdata(dev); > > + int err; > > + > > + err = cdns_i2c_runtime_resume(dev); > > First you try to resume... > > > + if (err) > > + return err; > > + > > + if (pm_runtime_status_suspended(dev)) { > > ... then you check if you are suspended ... This serves as a check and balance to ensure that when the system resumes with device in runtime suspend state, we disable the clock enabled in earlier cdns_i2c_runtime_resume() to ensure a balanced clock reference count for subsequent runtime resume transition. Similar implementation can be found in this commit: https://github.com/torvalds/linux/commit/44c99904cf61f945d02ac9976ab10dd5ccaea393 > > > + err = cdns_i2c_runtime_suspend(dev); > > ... and suspend again? Shouldn't this be _resume()? > > Thanks, [O> Andi > > > + 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) > > }; > > -- > > 2.25.1 > >
Hi Ji Sheng, [...] > > > +static int __maybe_unused cdns_i2c_resume(struct device *dev) > > > +{ > > > > I am not really understanding what you are trying to do here: > > > > > + struct cdns_i2c *xi2c = dev_get_drvdata(dev); > > > + int err; > > > + > > > + err = cdns_i2c_runtime_resume(dev); > > > > First you try to resume... > > > > > + if (err) > > > + return err; > > > + > > > + if (pm_runtime_status_suspended(dev)) { > > > > ... then you check if you are suspended ... > > This serves as a check and balance to ensure that when the system > resumes with device in runtime suspend state, we disable the clock > enabled in earlier cdns_i2c_runtime_resume() to ensure a balanced clock > reference count for subsequent runtime resume transition. > Similar implementation can be found in this commit: > https://github.com/torvalds/linux/commit/44c99904cf61f945d02ac9976ab10dd5ccaea393 OK, this is done purely for clock balancing, but then, I still don't understand the case. I expect the clock counter to be unbalanced when you suspend (because is moving towards '0'). While, if you check if the clock is unbalanced when resuming, it means that the clock had a negative counter (which is impossible because the clock counter is unsigned). If there is any unbalancing at this stage, then I recommend you to check what has happened previously. ... Or is there anything I am missing? Thanks, Andi > > > + err = cdns_i2c_runtime_suspend(dev); > > > > ... and suspend again? Shouldn't this be _resume()? > > > > Thanks, > [O> Andi > > > > > + if (err) > > > + return err; > > > + } > > > + > > > + i2c_mark_adapter_resumed(&xi2c->adap); > > > + > > > + return 0; > > > +}
On Sun, 10 Dec 2023 12:54:12 +0100 Andi Shyti <andi.shyti@kernel.org> wrote: > Hi Ji Sheng, > > [...] > > > > > +static int __maybe_unused cdns_i2c_resume(struct device *dev) > > > > +{ > > > > > > I am not really understanding what you are trying to do here: > > > > > > > + struct cdns_i2c *xi2c = dev_get_drvdata(dev); > > > > + int err; > > > > + > > > > + err = cdns_i2c_runtime_resume(dev); > > > > > > First you try to resume... > > > > > > > + if (err) > > > > + return err; > > > > + > > > > + if (pm_runtime_status_suspended(dev)) { > > > > > > ... then you check if you are suspended ... > > > > This serves as a check and balance to ensure that when the system > > resumes with device in runtime suspend state, we disable the clock > > enabled in earlier cdns_i2c_runtime_resume() to ensure a balanced > > clock reference count for subsequent runtime resume transition. > > Similar implementation can be found in this commit: > > https://github.com/torvalds/linux/commit/44c99904cf61f945d02ac9976ab10dd5ccaea393 > > > > OK, this is done purely for clock balancing, but then, I still > don't understand the case. I expect the clock counter to be > unbalanced when you suspend (because is moving towards '0'). > > While, if you check if the clock is unbalanced when resuming, it > means that the clock had a negative counter (which is impossible > because the clock counter is unsigned). > > If there is any unbalancing at this stage, then I recommend you > to check what has happened previously. > > ... Or is there anything I am missing? > > Thanks, > Andi You are right, the clock counter will move towards 0 during system suspend. Conversely during system resume, the clock counter is incremented to 1 early on in cdns_i2c_runtime_resume(). So the clock counter is not negative to start with. At this point of time, clock counter is 1. If the device is in runtime suspend, we decrement the clock counter back to 0, so the subsequent runtime resume could increment it back to 1. In a sense, balancing the clock counter. Please help correct me if I've got it wrong. > > > > > + err = cdns_i2c_runtime_suspend(dev); > > > > > > ... and suspend again? Shouldn't this be _resume()? > > > > > > Thanks, > > > Andi > > > > > > > + if (err) > > > > + return err; > > > > + } > > > > + > > > > + i2c_mark_adapter_resumed(&xi2c->adap); > > > > + > > > > + return 0; > > > > +}
Hi Andi, > On Sun, 10 Dec 2023 12:54:12 +0100 > Andi Shyti <andi.shyti@kernel.org> wrote: > > > Hi Ji Sheng, > > > > [...] > > > > > > > +static int __maybe_unused cdns_i2c_resume(struct device *dev) { > > > > > > > > I am not really understanding what you are trying to do here: > > > > > > > > > + struct cdns_i2c *xi2c = dev_get_drvdata(dev); > > > > > + int err; > > > > > + > > > > > + err = cdns_i2c_runtime_resume(dev); > > > > > > > > First you try to resume... > > > > > > > > > + if (err) > > > > > + return err; > > > > > + > > > > > + if (pm_runtime_status_suspended(dev)) { > > > > > > > > ... then you check if you are suspended ... > > > > > > This serves as a check and balance to ensure that when the system > > > resumes with device in runtime suspend state, we disable the clock > > > enabled in earlier cdns_i2c_runtime_resume() to ensure a balanced > > > clock reference count for subsequent runtime resume transition. > > > Similar implementation can be found in this commit: > > > https://github.com/torvalds/linux/commit/44c99904cf61f945d02ac9976ab > > > 10dd5ccaea393 > > > > > > > OK, this is done purely for clock balancing, but then, I still don't > > understand the case. I expect the clock counter to be unbalanced when > > you suspend (because is moving towards '0'). > > > > While, if you check if the clock is unbalanced when resuming, it means > > that the clock had a negative counter (which is impossible because the > > clock counter is unsigned). > > > > If there is any unbalancing at this stage, then I recommend you to > > check what has happened previously. > > > > ... Or is there anything I am missing? > > > > Thanks, > > Andi > > You are right, the clock counter will move towards 0 during system suspend. > Conversely during system resume, the clock counter is incremented to 1 early on in cdns_i2c_runtime_resume(). So the clock counter > is not negative to start with. > At this point of time, clock counter is 1. If the device is in runtime suspend, we decrement the clock counter back to 0, so the > subsequent runtime resume could increment it back to 1. In a sense, balancing the clock counter. > Please help correct me if I've got it wrong. > Just to check on the status of this patch. Let me know if the above statement makes sense. > > > > > > > + err = cdns_i2c_runtime_suspend(dev); > > > > > > > > ... and suspend again? Shouldn't this be _resume()? > > > > > > > > Thanks, > > > > Andi > > > > > > > > > + if (err) > > > > > + return err; > > > > > + } > > > > > + > > > > > + i2c_mark_adapter_resumed(&xi2c->adap); > > > > > + > > > > > + return 0; > > > > > +} >
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> --- Changes since v1: - Add missing err assignment in cdns_i2c_resume(). --- drivers/i2c/busses/i2c-cadence.c | 33 ++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)