| Message ID | 20230111090222.2016499-18-Vijendar.Mukunda@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add soundwire support for Pink Sardine platform | expand |
On 1/11/23 03:02, Vijendar Mukunda wrote: > Add pm_prepare callback and System level pm ops support for > AMD master driver. > > Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com> > Signed-off-by: Mastan Katragadda <Mastan.Katragadda@amd.com> > --- > drivers/soundwire/amd_master.c | 76 ++++++++++++++++++++++++++++++++++ > 1 file changed, 76 insertions(+) > > diff --git a/drivers/soundwire/amd_master.c b/drivers/soundwire/amd_master.c > index 2fd77a673c22..f4478cc17aac 100644 > --- a/drivers/soundwire/amd_master.c > +++ b/drivers/soundwire/amd_master.c > @@ -1552,6 +1552,80 @@ static int amd_sdwc_clock_stop_exit(struct amd_sdwc_ctrl *ctrl) > return 0; > } > > +static int amd_resume_child_device(struct device *dev, void *data) > +{ > + int ret; > + struct sdw_slave *slave = dev_to_sdw_dev(dev); > + > + if (!slave->probed) { > + dev_dbg(dev, "skipping device, no probed driver\n"); > + return 0; > + } > + if (!slave->dev_num_sticky) { > + dev_dbg(dev, "skipping device, never detected on bus\n"); > + return 0; > + } > + > + if (!pm_runtime_suspended(dev)) > + return 0; > + ret = pm_request_resume(dev); > + if (ret < 0) > + dev_err(dev, "%s: pm_request_resume failed: %d\n", __func__, ret); > + > + return ret; > +} > + > +static int __maybe_unused amd_pm_prepare(struct device *dev) > +{ > + struct amd_sdwc_ctrl *ctrl = dev_get_drvdata(dev); > + struct sdw_bus *bus = &ctrl->bus; > + int ret; > + > + if (bus->prop.hw_disabled || !ctrl->startup_done) { > + dev_dbg(bus->dev, "SoundWire master %d is disabled or not-started, ignoring\n", > + bus->link_id); > + return 0; > + } > + ret = device_for_each_child(bus->dev, NULL, amd_resume_child_device); > + if (ret < 0) > + dev_err(dev, "%s: amd_resume_child_device failed: %d\n", __func__, ret); > + if (pm_runtime_suspended(dev) && ctrl->power_mode_mask & AMD_SDW_CLK_STOP_MODE) { > + ret = pm_request_resume(dev); > + if (ret < 0) { > + dev_err(bus->dev, "pm_request_resume failed: %d\n", ret); > + return 0; > + } > + } > + return 0; > +} This seems to be inspired by the Intel code, but is this necessary here? For Intel, we saw cases where we had to pm_resume before doing a system suspend, otherwise the hardware was in a bad state. Do you actually need to do so, or is is possible to do a system suspend when the clock is stopped. And in the case where the bus is in 'power-off' mode, do you actually need to resume at all? > + > +static int __maybe_unused amd_suspend(struct device *dev) > +{ > + struct amd_sdwc_ctrl *ctrl = dev_get_drvdata(dev); > + struct sdw_bus *bus = &ctrl->bus; > + int ret; > + > + if (bus->prop.hw_disabled || !ctrl->startup_done) { > + dev_dbg(bus->dev, "SoundWire master %d is disabled or not-started, ignoring\n", > + bus->link_id); > + return 0; > + } > + > + if (ctrl->power_mode_mask & AMD_SDW_CLK_STOP_MODE) { > + ret = amd_sdwc_clock_stop(ctrl); > + if (ret) > + return ret; > + } else if (ctrl->power_mode_mask & AMD_SDW_POWER_OFF_MODE) { > + ret = amd_sdwc_clock_stop(ctrl); do you actually need to stop the clock before powering-off? This seems counter intuitive and not so useful? > + if (ret) > + return ret; > + ret = amd_deinit_sdw_controller(ctrl); > + if (ret) > + return ret; > + } > + return 0; > +} > + > static int __maybe_unused amd_suspend_runtime(struct device *dev) > { > struct amd_sdwc_ctrl *ctrl = dev_get_drvdata(dev); > @@ -1638,6 +1712,8 @@ static int __maybe_unused amd_resume_runtime(struct device *dev) > } > > static const struct dev_pm_ops amd_pm = { > + .prepare = amd_pm_prepare, > + SET_SYSTEM_SLEEP_PM_OPS(amd_suspend, amd_resume_runtime) > SET_RUNTIME_PM_OPS(amd_suspend_runtime, amd_resume_runtime, NULL) > }; >
On 11/01/23 21:28, Pierre-Louis Bossart wrote: > > On 1/11/23 03:02, Vijendar Mukunda wrote: >> Add pm_prepare callback and System level pm ops support for >> AMD master driver. >> >> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com> >> Signed-off-by: Mastan Katragadda <Mastan.Katragadda@amd.com> >> --- >> drivers/soundwire/amd_master.c | 76 ++++++++++++++++++++++++++++++++++ >> 1 file changed, 76 insertions(+) >> >> diff --git a/drivers/soundwire/amd_master.c b/drivers/soundwire/amd_master.c >> index 2fd77a673c22..f4478cc17aac 100644 >> --- a/drivers/soundwire/amd_master.c >> +++ b/drivers/soundwire/amd_master.c >> @@ -1552,6 +1552,80 @@ static int amd_sdwc_clock_stop_exit(struct amd_sdwc_ctrl *ctrl) >> return 0; >> } >> >> +static int amd_resume_child_device(struct device *dev, void *data) >> +{ >> + int ret; >> + struct sdw_slave *slave = dev_to_sdw_dev(dev); >> + >> + if (!slave->probed) { >> + dev_dbg(dev, "skipping device, no probed driver\n"); >> + return 0; >> + } >> + if (!slave->dev_num_sticky) { >> + dev_dbg(dev, "skipping device, never detected on bus\n"); >> + return 0; >> + } >> + >> + if (!pm_runtime_suspended(dev)) >> + return 0; >> + ret = pm_request_resume(dev); >> + if (ret < 0) >> + dev_err(dev, "%s: pm_request_resume failed: %d\n", __func__, ret); >> + >> + return ret; >> +} >> + >> +static int __maybe_unused amd_pm_prepare(struct device *dev) >> +{ >> + struct amd_sdwc_ctrl *ctrl = dev_get_drvdata(dev); >> + struct sdw_bus *bus = &ctrl->bus; >> + int ret; >> + >> + if (bus->prop.hw_disabled || !ctrl->startup_done) { >> + dev_dbg(bus->dev, "SoundWire master %d is disabled or not-started, ignoring\n", >> + bus->link_id); >> + return 0; >> + } >> + ret = device_for_each_child(bus->dev, NULL, amd_resume_child_device); >> + if (ret < 0) >> + dev_err(dev, "%s: amd_resume_child_device failed: %d\n", __func__, ret); >> + if (pm_runtime_suspended(dev) && ctrl->power_mode_mask & AMD_SDW_CLK_STOP_MODE) { >> + ret = pm_request_resume(dev); >> + if (ret < 0) { >> + dev_err(bus->dev, "pm_request_resume failed: %d\n", ret); >> + return 0; >> + } >> + } >> + return 0; >> +} > This seems to be inspired by the Intel code, but is this necessary here? No It's not inspired by intel code. Initially, we haven't included pm_prepare callback. We have observed issues without pm_prepare callback. > For Intel, we saw cases where we had to pm_resume before doing a system > suspend, otherwise the hardware was in a bad state. > > Do you actually need to do so, or is is possible to do a system suspend > when the clock is stopped. > > And in the case where the bus is in 'power-off' mode, do you actually > need to resume at all? Our platform supports different power modes. To support all combinations, we have included pm_prepare callback. >> + >> +static int __maybe_unused amd_suspend(struct device *dev) >> +{ >> + struct amd_sdwc_ctrl *ctrl = dev_get_drvdata(dev); >> + struct sdw_bus *bus = &ctrl->bus; >> + int ret; >> + >> + if (bus->prop.hw_disabled || !ctrl->startup_done) { >> + dev_dbg(bus->dev, "SoundWire master %d is disabled or not-started, ignoring\n", >> + bus->link_id); >> + return 0; >> + } >> + >> + if (ctrl->power_mode_mask & AMD_SDW_CLK_STOP_MODE) { >> + ret = amd_sdwc_clock_stop(ctrl); >> + if (ret) >> + return ret; >> + } else if (ctrl->power_mode_mask & AMD_SDW_POWER_OFF_MODE) { >> + ret = amd_sdwc_clock_stop(ctrl); > do you actually need to stop the clock before powering-off? This seems > counter intuitive and not so useful? Yes, as per our design, we need to stop the clock before powering off. >> + if (ret) >> + return ret; >> + ret = amd_deinit_sdw_controller(ctrl); >> + if (ret) >> + return ret; >> + } >> + return 0; >> +} >> + >> static int __maybe_unused amd_suspend_runtime(struct device *dev) >> { >> struct amd_sdwc_ctrl *ctrl = dev_get_drvdata(dev); >> @@ -1638,6 +1712,8 @@ static int __maybe_unused amd_resume_runtime(struct device *dev) >> } >> >> static const struct dev_pm_ops amd_pm = { >> + .prepare = amd_pm_prepare, >> + SET_SYSTEM_SLEEP_PM_OPS(amd_suspend, amd_resume_runtime) >> SET_RUNTIME_PM_OPS(amd_suspend_runtime, amd_resume_runtime, NULL) >> }; >>
>>> +static int __maybe_unused amd_pm_prepare(struct device *dev) >>> +{ >>> + struct amd_sdwc_ctrl *ctrl = dev_get_drvdata(dev); >>> + struct sdw_bus *bus = &ctrl->bus; >>> + int ret; >>> + >>> + if (bus->prop.hw_disabled || !ctrl->startup_done) { >>> + dev_dbg(bus->dev, "SoundWire master %d is disabled or not-started, ignoring\n", >>> + bus->link_id); >>> + return 0; >>> + } >>> + ret = device_for_each_child(bus->dev, NULL, amd_resume_child_device); >>> + if (ret < 0) >>> + dev_err(dev, "%s: amd_resume_child_device failed: %d\n", __func__, ret); >>> + if (pm_runtime_suspended(dev) && ctrl->power_mode_mask & AMD_SDW_CLK_STOP_MODE) { >>> + ret = pm_request_resume(dev); >>> + if (ret < 0) { >>> + dev_err(bus->dev, "pm_request_resume failed: %d\n", ret); >>> + return 0; >>> + } >>> + } >>> + return 0; >>> +} >> This seems to be inspired by the Intel code, but is this necessary here? > No It's not inspired by intel code. Initially, we haven't included > pm_prepare callback. We have observed issues without > pm_prepare callback. >> For Intel, we saw cases where we had to pm_resume before doing a system >> suspend, otherwise the hardware was in a bad state. >> >> Do you actually need to do so, or is is possible to do a system suspend >> when the clock is stopped. >> >> And in the case where the bus is in 'power-off' mode, do you actually >> need to resume at all? > Our platform supports different power modes. To support all > combinations, we have included pm_prepare callback. >> do you actually need to stop the clock before powering-off? This seems >> counter intuitive and not so useful? > Yes, as per our design, we need to stop the clock > before powering off. It'd be good to add comments capturing these points, that would be useful for new contributors and reviewers to know this is intentional and required by the hardware programming sequences.
diff --git a/drivers/soundwire/amd_master.c b/drivers/soundwire/amd_master.c index 2fd77a673c22..f4478cc17aac 100644 --- a/drivers/soundwire/amd_master.c +++ b/drivers/soundwire/amd_master.c @@ -1552,6 +1552,80 @@ static int amd_sdwc_clock_stop_exit(struct amd_sdwc_ctrl *ctrl) return 0; } +static int amd_resume_child_device(struct device *dev, void *data) +{ + int ret; + struct sdw_slave *slave = dev_to_sdw_dev(dev); + + if (!slave->probed) { + dev_dbg(dev, "skipping device, no probed driver\n"); + return 0; + } + if (!slave->dev_num_sticky) { + dev_dbg(dev, "skipping device, never detected on bus\n"); + return 0; + } + + if (!pm_runtime_suspended(dev)) + return 0; + ret = pm_request_resume(dev); + if (ret < 0) + dev_err(dev, "%s: pm_request_resume failed: %d\n", __func__, ret); + + return ret; +} + +static int __maybe_unused amd_pm_prepare(struct device *dev) +{ + struct amd_sdwc_ctrl *ctrl = dev_get_drvdata(dev); + struct sdw_bus *bus = &ctrl->bus; + int ret; + + if (bus->prop.hw_disabled || !ctrl->startup_done) { + dev_dbg(bus->dev, "SoundWire master %d is disabled or not-started, ignoring\n", + bus->link_id); + return 0; + } + ret = device_for_each_child(bus->dev, NULL, amd_resume_child_device); + if (ret < 0) + dev_err(dev, "%s: amd_resume_child_device failed: %d\n", __func__, ret); + if (pm_runtime_suspended(dev) && ctrl->power_mode_mask & AMD_SDW_CLK_STOP_MODE) { + ret = pm_request_resume(dev); + if (ret < 0) { + dev_err(bus->dev, "pm_request_resume failed: %d\n", ret); + return 0; + } + } + return 0; +} + +static int __maybe_unused amd_suspend(struct device *dev) +{ + struct amd_sdwc_ctrl *ctrl = dev_get_drvdata(dev); + struct sdw_bus *bus = &ctrl->bus; + int ret; + + if (bus->prop.hw_disabled || !ctrl->startup_done) { + dev_dbg(bus->dev, "SoundWire master %d is disabled or not-started, ignoring\n", + bus->link_id); + return 0; + } + + if (ctrl->power_mode_mask & AMD_SDW_CLK_STOP_MODE) { + ret = amd_sdwc_clock_stop(ctrl); + if (ret) + return ret; + } else if (ctrl->power_mode_mask & AMD_SDW_POWER_OFF_MODE) { + ret = amd_sdwc_clock_stop(ctrl); + if (ret) + return ret; + ret = amd_deinit_sdw_controller(ctrl); + if (ret) + return ret; + } + return 0; +} + static int __maybe_unused amd_suspend_runtime(struct device *dev) { struct amd_sdwc_ctrl *ctrl = dev_get_drvdata(dev); @@ -1638,6 +1712,8 @@ static int __maybe_unused amd_resume_runtime(struct device *dev) } static const struct dev_pm_ops amd_pm = { + .prepare = amd_pm_prepare, + SET_SYSTEM_SLEEP_PM_OPS(amd_suspend, amd_resume_runtime) SET_RUNTIME_PM_OPS(amd_suspend_runtime, amd_resume_runtime, NULL) };