diff mbox series

[17/19] soundwire: amd: add pm_prepare callback and pm ops support

Message ID 20230111090222.2016499-18-Vijendar.Mukunda@amd.com
State New
Headers
Series [01/19] ASoC: amd: ps: create platform devices based on acp config |

Commit Message

Mukunda,Vijendar Jan. 11, 2023, 9:02 a.m. UTC 
  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(+)

Comments

Pierre-Louis Bossart Jan. 11, 2023, 3:58 p.m. UTC | #1 
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)
>  };
>
Mukunda,Vijendar Jan. 12, 2023, 10:14 a.m. UTC | #2 
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)
>>  };
>>
Pierre-Louis Bossart Jan. 12, 2023, 2:50 p.m. UTC | #3 
>>> +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 mbox series

Patch

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)
 };