diff mbox series

[V6,8/8] soundwire: amd: add pm_prepare callback and pm ops support

Message ID 20230307133135.545952-9-Vijendar.Mukunda@amd.com
State New
Headers
Series [V6,1/8] soundwire: export sdw_compute_slave_ports() function |

Commit Message

Mukunda,Vijendar March 7, 2023, 1:31 p.m. UTC 
  Add pm_prepare callback and System level pm ops support for
AMD SoundWire manager driver.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
Signed-off-by: Mastan Katragadda <Mastan.Katragadda@amd.com>
---
 drivers/soundwire/amd_manager.c | 85 +++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

Comments

Pierre-Louis Bossart March 7, 2023, 3:28 p.m. UTC | #1 
> +static int amd_resume_child_device(struct device *dev, void *data)
> +{
> +	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +	int ret;
> +
> +	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);

I still don't get why the test above was needed. It's racy and brings
limited benefits.

> +	if (ret < 0)
> +		dev_err(dev, "pm_request_resume failed: %d\n", ret);
> +
> +	return ret;
> +}
Mukunda,Vijendar March 7, 2023, 8:25 p.m. UTC | #2 
On 07/03/23 20:58, Pierre-Louis Bossart wrote:
>> +static int amd_resume_child_device(struct device *dev, void *data)
>> +{
>> +	struct sdw_slave *slave = dev_to_sdw_dev(dev);
>> +	int ret;
>> +
>> +	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);
> I still don't get why the test above was needed. It's racy and brings
> limited benefits.
As explained below thread,

https://lore.kernel.org/lkml/acd3a560-1218-9f1d-06ec-19e4d3d4e2c9@amd.com

Our scenario is multiple peripheral devices are connected
over the same link.

In our implementation, device_for_each_child() function invokes
amd_resume_child_device callback for each child.
When any one of the child device is active, It will break the
iteration, which results in failure resuming all child devices.

If we skip , pm_suspended check , it will not resume all
peripheral devices when any one of the peripheral device is active.
>
>> +	if (ret < 0)
>> +		dev_err(dev, "pm_request_resume failed: %d\n", ret);
>> +
>> +	return ret;
>> +}
Pierre-Louis Bossart March 7, 2023, 9:08 p.m. UTC | #3 
On 3/7/23 14:25, Mukunda,Vijendar wrote:
> On 07/03/23 20:58, Pierre-Louis Bossart wrote:
>>> +static int amd_resume_child_device(struct device *dev, void *data)
>>> +{
>>> +	struct sdw_slave *slave = dev_to_sdw_dev(dev);
>>> +	int ret;
>>> +
>>> +	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);
>> I still don't get why the test above was needed. It's racy and brings
>> limited benefits.
> As explained below thread,
> 
> https://lore.kernel.org/lkml/acd3a560-1218-9f1d-06ec-19e4d3d4e2c9@amd.com
> 
> Our scenario is multiple peripheral devices are connected
> over the same link.
> 
> In our implementation, device_for_each_child() function invokes
> amd_resume_child_device callback for each child.
> When any one of the child device is active, It will break the
> iteration, which results in failure resuming all child devices.

Can you clarify the 'it will break the iteration' statement?

Are you saying pm_request_resume() will return a negative error code if
the device is already active?

We've used an unconditional pm_request_resume() in the Intel code for
quite some time, including with multiple amplifiers per link, and have
never observed the issue you report, so I'd like to get to the root
cause pretty please. You took the Intel code and added a test for AMD
platforms, and I'd really like to understand if the Intel code was wrong
in the first place, or if the test is not needed. Something does not add
up here.

> 
> If we skip , pm_suspended check , it will not resume all
> peripheral devices when any one of the peripheral device is active.
>>
>>> +	if (ret < 0)
>>> +		dev_err(dev, "pm_request_resume failed: %d\n", ret);
>>> +
>>> +	return ret;
>>> +}
>
Mukunda,Vijendar March 8, 2023, 4:32 a.m. UTC | #4 
On 08/03/23 02:38, Pierre-Louis Bossart wrote:
>
> On 3/7/23 14:25, Mukunda,Vijendar wrote:
>> On 07/03/23 20:58, Pierre-Louis Bossart wrote:
>>>> +static int amd_resume_child_device(struct device *dev, void *data)
>>>> +{
>>>> +	struct sdw_slave *slave = dev_to_sdw_dev(dev);
>>>> +	int ret;
>>>> +
>>>> +	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);
>>> I still don't get why the test above was needed. It's racy and brings
>>> limited benefits.
>> As explained below thread,
>>
>> https://lore.kernel.org/lkml/acd3a560-1218-9f1d-06ec-19e4d3d4e2c9@amd.com
>>
>> Our scenario is multiple peripheral devices are connected
>> over the same link.
>>
>> In our implementation, device_for_each_child() function invokes
>> amd_resume_child_device callback for each child.
>> When any one of the child device is active, It will break the
>> iteration, which results in failure resuming all child devices.
> Can you clarify the 'it will break the iteration' statement?
>
> Are you saying pm_request_resume() will return a negative error code if
> the device is already active?
>
> We've used an unconditional pm_request_resume() in the Intel code for
> quite some time, including with multiple amplifiers per link, and have
> never observed the issue you report, so I'd like to get to the root
> cause pretty please. You took the Intel code and added a test for AMD
> platforms, and I'd really like to understand if the Intel code was wrong
> in the first place, or if the test is not needed. Something does not add
> up here.
AMP Codec (In aggregate mode) + Jack Codec connected over the same
link on our platform.
Consider below, scenario.
Active stream is running on AMP codec and Jack codec is already in runtime
suspend state.
If system level suspend is invoked, in prepare callback, we need to resume
both the codec devices.

device_for_each_child() will invoke amd_resume_child_device() function callback
for each device which will try to resume the child device in this case.
By definition, device_for_each_child() Iterate over @parent's child devices,
and invokes the callback for each. We check the return of amd_resume_child_device()
each time.
If it returns anything other than 0, we break out and return that value.

In current scenario, As AMP codec is not in runtime suspend state,
pm_request_resume() will return a value as 1. This will break the
sequence for resuming rest of the child devices(JACK codec in our case).

As mentioned in an earlier thread, there are two possible solutions.
1. check pm runtime suspend state and return 0 if it is not suspended
2. simply always return 0 for amd_resume_child_device() function callback.

We opted first one as solution.
>
>> If we skip , pm_suspended check , it will not resume all
>> peripheral devices when any one of the peripheral device is active.
>>>> +	if (ret < 0)
>>>> +		dev_err(dev, "pm_request_resume failed: %d\n", ret);
>>>> +
>>>> +	return ret;
>>>> +}
Pierre-Louis Bossart March 8, 2023, 1:58 p.m. UTC | #5 
On 3/7/23 22:32, Mukunda,Vijendar wrote:
> On 08/03/23 02:38, Pierre-Louis Bossart wrote:
>>
>> On 3/7/23 14:25, Mukunda,Vijendar wrote:
>>> On 07/03/23 20:58, Pierre-Louis Bossart wrote:
>>>>> +static int amd_resume_child_device(struct device *dev, void *data)
>>>>> +{
>>>>> +	struct sdw_slave *slave = dev_to_sdw_dev(dev);
>>>>> +	int ret;
>>>>> +
>>>>> +	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);
>>>> I still don't get why the test above was needed. It's racy and brings
>>>> limited benefits.
>>> As explained below thread,
>>>
>>> https://lore.kernel.org/lkml/acd3a560-1218-9f1d-06ec-19e4d3d4e2c9@amd.com
>>>
>>> Our scenario is multiple peripheral devices are connected
>>> over the same link.
>>>
>>> In our implementation, device_for_each_child() function invokes
>>> amd_resume_child_device callback for each child.
>>> When any one of the child device is active, It will break the
>>> iteration, which results in failure resuming all child devices.
>> Can you clarify the 'it will break the iteration' statement?
>>
>> Are you saying pm_request_resume() will return a negative error code if
>> the device is already active?
>>
>> We've used an unconditional pm_request_resume() in the Intel code for
>> quite some time, including with multiple amplifiers per link, and have
>> never observed the issue you report, so I'd like to get to the root
>> cause pretty please. You took the Intel code and added a test for AMD
>> platforms, and I'd really like to understand if the Intel code was wrong
>> in the first place, or if the test is not needed. Something does not add
>> up here.
> AMP Codec (In aggregate mode) + Jack Codec connected over the same
> link on our platform.
> Consider below, scenario.
> Active stream is running on AMP codec and Jack codec is already in runtime
> suspend state.
> If system level suspend is invoked, in prepare callback, we need to resume
> both the codec devices.
> 
> device_for_each_child() will invoke amd_resume_child_device() function callback
> for each device which will try to resume the child device in this case.
> By definition, device_for_each_child() Iterate over @parent's child devices,
> and invokes the callback for each. We check the return of amd_resume_child_device()
> each time.
> If it returns anything other than 0, we break out and return that value.
> 
> In current scenario, As AMP codec is not in runtime suspend state,
> pm_request_resume() will return a value as 1. This will break the
> sequence for resuming rest of the child devices(JACK codec in our case).

Well, yes, now that makes sense, thanks for the details.

I think the reason why we didn't see the problem with the Intel code is
that both amplifiers are on the same dailink, so they are by
construction either both suspended or both active. We never had
different types of devices on the same link.

I would however suggest this simpler alternative, where only negative
return values are returned:

ret = pm_request_resume(dev);
if (ret < 0) {
	dev_err(dev, "pm_request_resume failed: %d\n", ret);
        return ret;
}
return 0;

this would work just fine, no?


> 
> As mentioned in an earlier thread, there are two possible solutions.
> 1. check pm runtime suspend state and return 0 if it is not suspended
> 2. simply always return 0 for amd_resume_child_device() function callback.
> 
> We opted first one as solution.

My suggestion looks like your option 2. It's cleaner IMHO.
Mukunda,Vijendar March 8, 2023, 2:19 p.m. UTC | #6 
On 08/03/23 19:28, Pierre-Louis Bossart wrote:
>
> On 3/7/23 22:32, Mukunda,Vijendar wrote:
>> On 08/03/23 02:38, Pierre-Louis Bossart wrote:
>>> On 3/7/23 14:25, Mukunda,Vijendar wrote:
>>>> On 07/03/23 20:58, Pierre-Louis Bossart wrote:
>>>>>> +static int amd_resume_child_device(struct device *dev, void *data)
>>>>>> +{
>>>>>> +	struct sdw_slave *slave = dev_to_sdw_dev(dev);
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	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);
>>>>> I still don't get why the test above was needed. It's racy and brings
>>>>> limited benefits.
>>>> As explained below thread,
>>>>
>>>> https://lore.kernel.org/lkml/acd3a560-1218-9f1d-06ec-19e4d3d4e2c9@amd.com
>>>>
>>>> Our scenario is multiple peripheral devices are connected
>>>> over the same link.
>>>>
>>>> In our implementation, device_for_each_child() function invokes
>>>> amd_resume_child_device callback for each child.
>>>> When any one of the child device is active, It will break the
>>>> iteration, which results in failure resuming all child devices.
>>> Can you clarify the 'it will break the iteration' statement?
>>>
>>> Are you saying pm_request_resume() will return a negative error code if
>>> the device is already active?
>>>
>>> We've used an unconditional pm_request_resume() in the Intel code for
>>> quite some time, including with multiple amplifiers per link, and have
>>> never observed the issue you report, so I'd like to get to the root
>>> cause pretty please. You took the Intel code and added a test for AMD
>>> platforms, and I'd really like to understand if the Intel code was wrong
>>> in the first place, or if the test is not needed. Something does not add
>>> up here.
>> AMP Codec (In aggregate mode) + Jack Codec connected over the same
>> link on our platform.
>> Consider below, scenario.
>> Active stream is running on AMP codec and Jack codec is already in runtime
>> suspend state.
>> If system level suspend is invoked, in prepare callback, we need to resume
>> both the codec devices.
>>
>> device_for_each_child() will invoke amd_resume_child_device() function callback
>> for each device which will try to resume the child device in this case.
>> By definition, device_for_each_child() Iterate over @parent's child devices,
>> and invokes the callback for each. We check the return of amd_resume_child_device()
>> each time.
>> If it returns anything other than 0, we break out and return that value.
>>
>> In current scenario, As AMP codec is not in runtime suspend state,
>> pm_request_resume() will return a value as 1. This will break the
>> sequence for resuming rest of the child devices(JACK codec in our case).
> Well, yes, now that makes sense, thanks for the details.
>
> I think the reason why we didn't see the problem with the Intel code is
> that both amplifiers are on the same dailink, so they are by
> construction either both suspended or both active. We never had
> different types of devices on the same link.
>
> I would however suggest this simpler alternative, where only negative
> return values are returned:
>
> ret = pm_request_resume(dev);
> if (ret < 0) {
> 	dev_err(dev, "pm_request_resume failed: %d\n", ret);
>         return ret;
> }
> return 0;
>
> this would work just fine, no?
> No, As explained, pm_request_resume() return value is 1 for active device.
>> As mentioned in an earlier thread, there are two possible solutions.
>> 1. check pm runtime suspend state and return 0 if it is not suspended
>> 2. simply always return 0 for amd_resume_child_device() function callback.
>>
>> We opted first one as solution.
> My suggestion looks like your option 2. It's cleaner IMHO.
To use option 2, we need to respin the patch series.
Is it okay if we fix it as supplement patch?
Pierre-Louis Bossart March 8, 2023, 2:23 p.m. UTC | #7 
>>> device_for_each_child() will invoke amd_resume_child_device() function callback
>>> for each device which will try to resume the child device in this case.
>>> By definition, device_for_each_child() Iterate over @parent's child devices,
>>> and invokes the callback for each. We check the return of amd_resume_child_device()
>>> each time.
>>> If it returns anything other than 0, we break out and return that value.
>>>
>>> In current scenario, As AMP codec is not in runtime suspend state,
>>> pm_request_resume() will return a value as 1. This will break the
>>> sequence for resuming rest of the child devices(JACK codec in our case).
>> Well, yes, now that makes sense, thanks for the details.
>>
>> I think the reason why we didn't see the problem with the Intel code is
>> that both amplifiers are on the same dailink, so they are by
>> construction either both suspended or both active. We never had
>> different types of devices on the same link.
>>
>> I would however suggest this simpler alternative, where only negative
>> return values are returned:
>>
>> ret = pm_request_resume(dev);
>> if (ret < 0) {
>> 	dev_err(dev, "pm_request_resume failed: %d\n", ret);
>>         return ret;
>> }
>> return 0;
>>
>> this would work just fine, no?
>> No, As explained, pm_request_resume() return value is 1 for active device.
>>> As mentioned in an earlier thread, there are two possible solutions.
>>> 1. check pm runtime suspend state and return 0 if it is not suspended
>>> 2. simply always return 0 for amd_resume_child_device() function callback.
>>>
>>> We opted first one as solution.
>> My suggestion looks like your option 2. It's cleaner IMHO.
> To use option 2, we need to respin the patch series.
> Is it okay if we fix it as supplement patch?

I would vote for re-spinning a new version and ask others to review.
Mukunda,Vijendar March 8, 2023, 3:05 p.m. UTC | #8 
On 08/03/23 19:53, Pierre-Louis Bossart wrote:
>>>> device_for_each_child() will invoke amd_resume_child_device() function callback
>>>> for each device which will try to resume the child device in this case.
>>>> By definition, device_for_each_child() Iterate over @parent's child devices,
>>>> and invokes the callback for each. We check the return of amd_resume_child_device()
>>>> each time.
>>>> If it returns anything other than 0, we break out and return that value.
>>>>
>>>> In current scenario, As AMP codec is not in runtime suspend state,
>>>> pm_request_resume() will return a value as 1. This will break the
>>>> sequence for resuming rest of the child devices(JACK codec in our case).
>>> Well, yes, now that makes sense, thanks for the details.
>>>
>>> I think the reason why we didn't see the problem with the Intel code is
>>> that both amplifiers are on the same dailink, so they are by
>>> construction either both suspended or both active. We never had
>>> different types of devices on the same link.
>>>
>>> I would however suggest this simpler alternative, where only negative
>>> return values are returned:
>>>
>>> ret = pm_request_resume(dev);
>>> if (ret < 0) {
>>> 	dev_err(dev, "pm_request_resume failed: %d\n", ret);
>>>         return ret;
>>> }
>>> return 0;
>>>
>>> this would work just fine, no?
Sorry its my bad. This would work fine.
We will fix it and respin the patch series.
>>> No, As explained, pm_request_resume() return value is 1 for active device.
>>>> As mentioned in an earlier thread, there are two possible solutions.
>>>> 1. check pm runtime suspend state and return 0 if it is not suspended
>>>> 2. simply always return 0 for amd_resume_child_device() function callback.
>>>>
>>>> We opted first one as solution.
>>> My suggestion looks like your option 2. It's cleaner IMHO.
>> To use option 2, we need to respin the patch series.
>> Is it okay if we fix it as supplement patch?
> I would vote for re-spinning a new version and ask others to review.
diff mbox series

Patch

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 70e4fcd7e1e0..8d4117f0dc3c 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -1079,6 +1079,89 @@  static int amd_sdw_clock_stop_exit(struct amd_sdw_manager *amd_manager)
 	return 0;
 }
 
+static int amd_resume_child_device(struct device *dev, void *data)
+{
+	struct sdw_slave *slave = dev_to_sdw_dev(dev);
+	int ret;
+
+	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, "pm_request_resume failed: %d\n", ret);
+
+	return ret;
+}
+
+static int __maybe_unused amd_pm_prepare(struct device *dev)
+{
+	struct amd_sdw_manager *amd_manager = dev_get_drvdata(dev);
+	struct sdw_bus *bus = &amd_manager->bus;
+	int ret;
+
+	if (bus->prop.hw_disabled) {
+		dev_dbg(bus->dev, "SoundWire manager %d is disabled, ignoring\n",
+			bus->link_id);
+		return 0;
+	}
+	/*
+	 * When multiple peripheral devices connected over the same link, if SoundWire manager
+	 * device is not in runtime suspend state, observed that device alerts are missing
+	 * without pm_prepare on AMD platforms in clockstop mode0.
+	 */
+	if (amd_manager->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;
+		}
+	}
+	/* To force peripheral devices to system level suspend state, resume the devices
+	 * from runtime suspend state first. Without that unable to dispatch the alert
+	 * status to peripheral driver during system level resume as they are in runtime
+	 * suspend state.
+	 */
+	ret = device_for_each_child(bus->dev, NULL, amd_resume_child_device);
+	if (ret < 0)
+		dev_err(dev, "amd_resume_child_device failed: %d\n", ret);
+	return 0;
+}
+
+static int __maybe_unused amd_suspend(struct device *dev)
+{
+	struct amd_sdw_manager *amd_manager = dev_get_drvdata(dev);
+	struct sdw_bus *bus = &amd_manager->bus;
+	int ret;
+
+	if (bus->prop.hw_disabled) {
+		dev_dbg(bus->dev, "SoundWire manager %d is disabled, ignoring\n",
+			bus->link_id);
+		return 0;
+	}
+
+	if (amd_manager->power_mode_mask & AMD_SDW_CLK_STOP_MODE) {
+		return amd_sdw_clock_stop(amd_manager);
+	} else if (amd_manager->power_mode_mask & AMD_SDW_POWER_OFF_MODE) {
+		/*
+		 * As per hardware programming sequence on AMD platforms,
+		 * clock stop should be invoked first before powering-off
+		 */
+		ret = amd_sdw_clock_stop(amd_manager);
+		if (ret)
+			return ret;
+		return amd_deinit_sdw_manager(amd_manager);
+	}
+	return 0;
+}
+
 static int __maybe_unused amd_suspend_runtime(struct device *dev)
 {
 	struct amd_sdw_manager *amd_manager = dev_get_drvdata(dev);
@@ -1141,6 +1224,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)
 };