| Message ID | 20240102-j7200-pcie-s2r-v1-4-84e55da52400@bootlin.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add suspend to ram support for PCIe on J7200 | expand |
On Mon, Jan 15, 2024 at 6:16 PM Thomas Richard <thomas.richard@bootlin.com> wrote: > > From: Théo Lebrun <theo.lebrun@bootlin.com> > > Implement resume support Why? ... > +#ifdef CONFIG_PM No ifdeffery, use proper macros. ... > + dev_err(dev, "control %u: error restoring mux: %d\n", i, ret); Won't anything go wrong and spam the log buffer?
Hi! 2024-01-15 at 17:14, Thomas Richard wrote: > From: Théo Lebrun <theo.lebrun@bootlin.com> > > Implement resume support What Andy said, and please don't omit punctuation. Try to make it a pleasure to read your patches! > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > Signed-off-by: Thomas Richard <thomas.richard@bootlin.com> > --- > drivers/mux/mmio.c | 34 ++++++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c > index fd1d121a584b..ab4ef195fc0d 100644 > --- a/drivers/mux/mmio.c > +++ b/drivers/mux/mmio.c > @@ -125,13 +125,47 @@ static int mux_mmio_probe(struct platform_device *pdev) > > mux_chip->ops = &mux_mmio_ops; > > + dev_set_drvdata(dev, mux_chip); > + > return devm_mux_chip_register(dev, mux_chip); > } > > +#ifdef CONFIG_PM > +static int mux_mmio_resume_noirq(struct device *dev) > +{ > + struct mux_chip *mux_chip = dev_get_drvdata(dev); > + int global_ret = 0; > + unsigned int i; > + > + for (i = 0; i < mux_chip->controllers; i++) { > + struct mux_control *mux = &mux_chip->mux[i]; > + int val = mux->cached_state; You are not supposed to look at (or change) cached_state outside the mux core. > + int ret; > + > + if (val == MUX_IDLE_AS_IS) The cached_state can never be MUX_IDLE_AS_IS. Sure, it happens to have the same actual value as the correct MUX_CACHE_UNKNOWN, but abusing that is all kinds of wrong. > + continue; > + > + ret = mux_mmio_set(mux, val); > + if (ret) { If mux_mmio_set() fails, cached_state ends up wrong as it should be set to MUX_CACHE_UNKNOWN on failure. Low-level stuff like this needs to be done by the mux core, or things becomes a maintenance hazard... So, the meat of this function belongs in the mux core since none of it looks mmio specific. It could probably be named mux_chip_resume() or something such. That makes it simple to use the correct constant, and the mux_control_set() helper makes it easy to get the handling of cached_state right. Cheers, Peter > + dev_err(dev, "control %u: error restoring mux: %d\n", i, ret); > + if (!global_ret) > + global_ret = ret; > + } > + } > + > + return global_ret; > +} > +#endif > + > +static const struct dev_pm_ops mux_mmio_pm_ops = { > + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, mux_mmio_resume_noirq) > +}; > + > static struct platform_driver mux_mmio_driver = { > .driver = { > .name = "mmio-mux", > .of_match_table = mux_mmio_dt_ids, > + .pm = &mux_mmio_pm_ops, > }, > .probe = mux_mmio_probe, > }; >
Hello Peter, Thanks for the review. On 1/15/24 23:31, Peter Rosin wrote: > Hi! > > 2024-01-15 at 17:14, Thomas Richard wrote: >> From: Théo Lebrun <theo.lebrun@bootlin.com> >> >> Implement resume support > > What Andy said, and please don't omit punctuation. Try to make it a > pleasure to read your patches! Yes my commit message needs to be more verbose, sorry. > >> >> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> >> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com> >> --- >> drivers/mux/mmio.c | 34 ++++++++++++++++++++++++++++++++++ >> 1 file changed, 34 insertions(+) >> >> diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c >> index fd1d121a584b..ab4ef195fc0d 100644 >> --- a/drivers/mux/mmio.c >> +++ b/drivers/mux/mmio.c >> @@ -125,13 +125,47 @@ static int mux_mmio_probe(struct platform_device *pdev) >> >> mux_chip->ops = &mux_mmio_ops; >> >> + dev_set_drvdata(dev, mux_chip); >> + >> return devm_mux_chip_register(dev, mux_chip); >> } >> >> +#ifdef CONFIG_PM >> +static int mux_mmio_resume_noirq(struct device *dev) >> +{ >> + struct mux_chip *mux_chip = dev_get_drvdata(dev); >> + int global_ret = 0; >> + unsigned int i; >> + >> + for (i = 0; i < mux_chip->controllers; i++) { >> + struct mux_control *mux = &mux_chip->mux[i]; >> + int val = mux->cached_state; > > You are not supposed to look at (or change) cached_state outside the > mux core. > >> + int ret; >> + >> + if (val == MUX_IDLE_AS_IS) > > The cached_state can never be MUX_IDLE_AS_IS. Sure, it happens to have > the same actual value as the correct MUX_CACHE_UNKNOWN, but abusing > that is all kinds of wrong. > >> + continue; >> + >> + ret = mux_mmio_set(mux, val); >> + if (ret) { > > If mux_mmio_set() fails, cached_state ends up wrong as it should be set > to MUX_CACHE_UNKNOWN on failure. Low-level stuff like this needs to be > done by the mux core, or things becomes a maintenance hazard... > > So, the meat of this function belongs in the mux core since none of > it looks mmio specific. It could probably be named mux_chip_resume() > or something such. That makes it simple to use the correct constant, > and the mux_control_set() helper makes it easy to get the handling of > cached_state right. > Thanks for the explanations. So I created a mux_chip_resume function in the mux core. This function restores each mux using mux_control_set. The restored state is the cached state. The muxes with a MUX_CACHE_UNKNOWN cache state are ignored. So this patch will be splitted, one patch for the core, one for the mmio driver. Regards,
diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c index fd1d121a584b..ab4ef195fc0d 100644 --- a/drivers/mux/mmio.c +++ b/drivers/mux/mmio.c @@ -125,13 +125,47 @@ static int mux_mmio_probe(struct platform_device *pdev) mux_chip->ops = &mux_mmio_ops; + dev_set_drvdata(dev, mux_chip); + return devm_mux_chip_register(dev, mux_chip); } +#ifdef CONFIG_PM +static int mux_mmio_resume_noirq(struct device *dev) +{ + struct mux_chip *mux_chip = dev_get_drvdata(dev); + int global_ret = 0; + unsigned int i; + + for (i = 0; i < mux_chip->controllers; i++) { + struct mux_control *mux = &mux_chip->mux[i]; + int val = mux->cached_state; + int ret; + + if (val == MUX_IDLE_AS_IS) + continue; + + ret = mux_mmio_set(mux, val); + if (ret) { + dev_err(dev, "control %u: error restoring mux: %d\n", i, ret); + if (!global_ret) + global_ret = ret; + } + } + + return global_ret; +} +#endif + +static const struct dev_pm_ops mux_mmio_pm_ops = { + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, mux_mmio_resume_noirq) +}; + static struct platform_driver mux_mmio_driver = { .driver = { .name = "mmio-mux", .of_match_table = mux_mmio_dt_ids, + .pm = &mux_mmio_pm_ops, }, .probe = mux_mmio_probe, };