Re: [PATCH 14/19] soundwire: amd: add runtime pm ops for AMD master driver

[Mukunda,Vijendar]

On 11/01/23 21:17, Pierre-Louis Bossart wrote:
>
> On 1/11/23 03:02, Vijendar Mukunda wrote:
>> Add support for runtime pm ops for AMD master driver.
>>
>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@xxxxxxx>
>> Signed-off-by: Mastan Katragadda <Mastan.Katragadda@xxxxxxx>
>> ---
>>  drivers/soundwire/amd_master.c    | 205 ++++++++++++++++++++++++++++++
>>  drivers/soundwire/amd_master.h    |   3 +
>>  include/linux/soundwire/sdw_amd.h |   1 +
>>  3 files changed, 209 insertions(+)
>>
>> diff --git a/drivers/soundwire/amd_master.c b/drivers/soundwire/amd_master.c
>> index c7063b8bdd7b..d2d7f07de202 100644
>> --- a/drivers/soundwire/amd_master.c
>> +++ b/drivers/soundwire/amd_master.c
>> @@ -15,6 +15,7 @@
>>  #include <linux/soundwire/sdw.h>
>>  #include <linux/soundwire/sdw_registers.h>
>>  #include <linux/soundwire/sdw_amd.h>
>> +#include <linux/pm_runtime.h>
>>  #include <linux/wait.h>
>>  #include <sound/pcm_params.h>
>>  #include <sound/soc.h>
>> @@ -290,6 +291,17 @@ static int amd_disable_sdw_interrupts(struct amd_sdwc_ctrl *ctrl)
>>  	return 0;
>>  }
>>  
>> +static int amd_deinit_sdw_controller(struct amd_sdwc_ctrl *ctrl)
>> +{
>> +	int ret;
>> +
>> +	ret = amd_disable_sdw_interrupts(ctrl);
>> +	if (ret)
>> +		return ret;
>> +	ret = amd_disable_sdw_controller(ctrl);
>> +	return ret;
>> +}
>> +
>>  static int amd_sdwc_set_frameshape(struct amd_sdwc_ctrl *ctrl, u32 rows, u32 cols)
>>  {
>>  	u32 sdw_rows, sdw_cols, frame_size;
>> @@ -1387,6 +1399,12 @@ static int amd_sdwc_probe(struct platform_device *pdev)
>>  	INIT_WORK(&ctrl->amd_sdw_work, amd_sdwc_update_slave_status_work);
>>  	INIT_WORK(&ctrl->probe_work, amd_sdwc_probe_work);
>>  	schedule_work(&ctrl->probe_work);
>> +	/* Enable runtime PM */
>> +	pm_runtime_set_autosuspend_delay(dev, AMD_SDW_MASTER_SUSPEND_DELAY_MS);
>> +	pm_runtime_use_autosuspend(dev);
>> +	pm_runtime_mark_last_busy(dev);
>> +	pm_runtime_set_active(dev);
>> +	pm_runtime_enable(dev);
>>  	return 0;
>>  }
>>  
>> @@ -1398,14 +1416,201 @@ static int amd_sdwc_remove(struct platform_device *pdev)
>>  	amd_disable_sdw_interrupts(ctrl);
>>  	sdw_bus_master_delete(&ctrl->bus);
>>  	ret = amd_disable_sdw_controller(ctrl);
>> +	pm_runtime_disable(&pdev->dev);
>>  	return ret;
>>  }
>>  
>> +static int amd_sdwc_clock_stop(struct amd_sdwc_ctrl *ctrl)
>> +{
>> +	u32 clk_resume_ctrl_reg;
>> +	u32 wake_en_reg;
>> +	u32 val;
>> +	u32 retry_count = 0;
>> +	int ret;
>> +
>> +	ret = sdw_bus_prep_clk_stop(&ctrl->bus);
>> +	if (ret < 0 && ret != -ENODATA) {
>> +		dev_err(ctrl->dev, "prepare clock stop failed %d", ret);
>> +		return ret;
>> +	}
>> +	ret = sdw_bus_clk_stop(&ctrl->bus);
>> +	if (ret < 0 && ret != -ENODATA) {
>> +		dev_err(ctrl->dev, "bus clock stop failed %d", ret);
>> +		return ret;
> You need to be very careful here, because returning an error may prevent
> the device from suspending.
>
> If it's safe and possible to recover during the resume step, you
> probably want to log the error but let the suspend continue.
will fix it.
>> +	}
>> +	switch (ctrl->instance) {
>> +	case ACP_SDW0:
>> +		clk_resume_ctrl_reg = ACP_SW_CLK_RESUME_CTRL;
>> +		wake_en_reg = ACP_SW_WAKE_EN;
>> +		break;
>> +	case ACP_SDW1:
>> +		clk_resume_ctrl_reg = ACP_P1_SW_CLK_RESUME_CTRL;
>> +		wake_en_reg = ACP_SW1_WAKE_EN;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
> why not store these offsets during the probe and use them directly here?
> You know at probe time which master you're using.
will fix it.
>> +
>> +	do {
>> +		val = acp_reg_readl(ctrl->mmio + clk_resume_ctrl_reg);
>> +		if (val & AMD_SDW_CLK_STOP_DONE) {
>> +			ctrl->clk_stopped = true;
>> +			break;
>> +		}
>> +	} while (retry_count++ < AMD_SDW_CLK_STOP_MAX_RETRY_COUNT);
>> +
>> +	if (!ctrl->clk_stopped) {
>> +		dev_err(ctrl->dev, "SDW%x clock stop failed\n", ctrl->instance);
>> +		return -ETIMEDOUT;
>> +	}
>> +
>> +	if (ctrl->wake_en_mask)
>> +		acp_reg_writel(0x01, ctrl->mmio + wake_en_reg);
>> +
>> +	dev_dbg(ctrl->dev, "SDW%x clock stop successful\n", ctrl->instance);
>> +	return 0;
>> +}
>> +
>> +static int amd_sdwc_clock_stop_exit(struct amd_sdwc_ctrl *ctrl)
>> +{
>> +	int ret;
>> +	u32 clk_resume_ctrl_reg;
>> +	u32 val = 0;
>> +	u32 retry_count = 0;
>> +
>> +	switch (ctrl->instance) {
>> +	case ACP_SDW0:
>> +		clk_resume_ctrl_reg = ACP_SW_CLK_RESUME_CTRL;
>> +		break;
>> +	case ACP_SDW1:
>> +		clk_resume_ctrl_reg = ACP_P1_SW_CLK_RESUME_CTRL;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +	if (ctrl->clk_stopped) {
>> +		val = acp_reg_readl(ctrl->mmio + clk_resume_ctrl_reg);
>> +		val |= AMD_SDW_CLK_RESUME_REQ;
>> +		acp_reg_writel(val, ctrl->mmio + clk_resume_ctrl_reg);
>> +		do {
>> +			val = acp_reg_readl(ctrl->mmio + clk_resume_ctrl_reg);
>> +			if (val & AMD_SDW_CLK_RESUME_DONE)
>> +				break;
>> +			usleep_range(10, 100);
>> +		} while (retry_count++ < AMD_SDW_CLK_STOP_MAX_RETRY_COUNT);
>> +		if (val & AMD_SDW_CLK_RESUME_DONE) {
>> +			acp_reg_writel(0, ctrl->mmio + clk_resume_ctrl_reg);
>> +			ret = sdw_bus_exit_clk_stop(&ctrl->bus);
>> +			if (ret < 0)
>> +				dev_err(ctrl->dev, "bus failed to exit clock stop %d\n", ret);
>> +			ctrl->clk_stopped = false;
>> +		}
>> +	}
>> +	if (ctrl->clk_stopped) {
>> +		dev_err(ctrl->dev, "SDW%x clock stop exit failed\n", ctrl->instance);
>> +		return -ETIMEDOUT;
>> +	}
>> +
>> +	dev_dbg(ctrl->dev, "SDW%x clock stop exit successful\n", ctrl->instance);
>> +
>> +	return 0;
>> +}
>> +
>> +static int __maybe_unused amd_suspend_runtime(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) {
> do you have a case where the startup is not done? This was an
> Intel-specific thing.
We have included startup_done flag in probe_work to check whether Manager
has started. In case if manager init sequence fails, then there is no need
to apply any PM ops.

>> +		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_resume_runtime(struct device *dev)
>> +{
>> +	struct amd_sdwc_ctrl *ctrl = dev_get_drvdata(dev);
>> +	struct sdw_bus *bus = &ctrl->bus;
>> +	int ret;
>> +	u32 clk_resume_ctrl_reg;
>> +	u32 val = 0;
>> +	u32 retry_count = 0;
>> +
>> +	if (bus->prop.hw_disabled || !ctrl->startup_done) {
> same here
>
>> +		dev_dbg(bus->dev, "SoundWire master %d is disabled or not-started, ignoring\n",
>> +			bus->link_id);
>> +		return 0;
>> +	}
>> +
>> +	switch (ctrl->instance) {
>> +	case ACP_SDW0:
>> +		clk_resume_ctrl_reg = ACP_SW_CLK_RESUME_CTRL;
>> +		break;
>> +	case ACP_SDW1:
>> +		clk_resume_ctrl_reg = ACP_P1_SW_CLK_RESUME_CTRL;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
> select registers in the probe.
will fix it.
>> +
>> +	if (ctrl->power_mode_mask & AMD_SDW_CLK_STOP_MODE) {
>> +		ret = amd_sdwc_clock_stop_exit(ctrl);
>> +		if (ret)
>> +			return ret;
>> +	} else if (ctrl->power_mode_mask & AMD_SDW_POWER_OFF_MODE) {
>> +		val = acp_reg_readl(ctrl->mmio + clk_resume_ctrl_reg);
>> +		if (val) {
>> +			val |= AMD_SDW_CLK_RESUME_REQ;
>> +			acp_reg_writel(val, ctrl->mmio + clk_resume_ctrl_reg);
>> +			do {
>> +				val = acp_reg_readl(ctrl->mmio + clk_resume_ctrl_reg);
>> +				if (val & AMD_SDW_CLK_RESUME_DONE)
>> +					break;
>> +				usleep_range(10, 100);
>> +			} while (retry_count++ < AMD_SDW_CLK_STOP_MAX_RETRY_COUNT);
>> +			if (val & AMD_SDW_CLK_RESUME_DONE) {
>> +				acp_reg_writel(0, ctrl->mmio + clk_resume_ctrl_reg);
>> +				ctrl->clk_stopped = false;
>> +			}
>> +		}
>> +		sdw_clear_slave_status(bus, SDW_UNATTACH_REQUEST_MASTER_RESET);
>> +		amd_init_sdw_controller(ctrl);
>> +		amd_enable_sdw_interrupts(ctrl);
>> +		ret = amd_enable_sdw_controller(ctrl);
>> +		if (ret)
>> +			return ret;
>> +		ret = amd_sdwc_set_frameshape(ctrl, 50, 10);
> this should be defined at probe time, using magic numbers like this will
> not work in all cases and totally depends on the frame rate and
> bandwidth needs.
Will fix it.
>> +		if (ret)
>> +			return ret;
>> +	}
>> +	return 0;
>> +}