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

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@amd.com Signed-off-by: Mastan Katragadda Mastan.Katragadda@amd.com

---

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;

+}