Add support for runtime pm ops for AMD master driver.
Signed-off-by: Vijendar Mukunda <[email protected]>
Signed-off-by: Mastan Katragadda <[email protected]>
---
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;
+ }
+ 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;
+ }
+
+ 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) {
+ 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) {
+ 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;
+ }
+
+ 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);
+ if (ret)
+ return ret;
+ }
+ return 0;
+}
+
+static const struct dev_pm_ops amd_pm = {
+ SET_RUNTIME_PM_OPS(amd_suspend_runtime, amd_resume_runtime, NULL)
+};
+
static struct platform_driver amd_sdwc_driver = {
.probe = &amd_sdwc_probe,
.remove = &amd_sdwc_remove,
.driver = {
.name = "amd_sdw_controller",
+ .pm = &amd_pm,
}
};
module_platform_driver(amd_sdwc_driver);
diff --git a/drivers/soundwire/amd_master.h b/drivers/soundwire/amd_master.h
index b43a5d6496cb..cc254255512b 100644
--- a/drivers/soundwire/amd_master.h
+++ b/drivers/soundwire/amd_master.h
@@ -237,6 +237,9 @@
#define AMD_SDW0_PAD_KEEPER_DISABLE_MASK 0x1E
#define AMD_SDW1_PAD_KEEPER_DISABLE_MASK 0xF
#define AMD_SDW_PREQ_INTR_STAT BIT(19)
+#define AMD_SDW_CLK_STOP_DONE 1
+#define AMD_SDW_CLK_RESUME_REQ 2
+#define AMD_SDW_CLK_RESUME_DONE 3
enum amd_sdw_channel {
/* SDW0 */
diff --git a/include/linux/soundwire/sdw_amd.h b/include/linux/soundwire/sdw_amd.h
index 2db03b2f0c3b..f362f195b887 100644
--- a/include/linux/soundwire/sdw_amd.h
+++ b/include/linux/soundwire/sdw_amd.h
@@ -38,6 +38,7 @@ struct amd_sdwc_ctrl {
u32 quirks;
u32 wake_en_mask;
int num_ports;
+ bool clk_stopped;
bool startup_done;
u32 power_mode_mask;
};
--
2.34.1
On 1/11/23 03:02, Vijendar Mukunda wrote:
> Add support for runtime pm ops for AMD master driver.
>
> Signed-off-by: Vijendar Mukunda <[email protected]>
> Signed-off-by: Mastan Katragadda <[email protected]>
> ---
> 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.
> + }
> + 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.
> +
> + 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.
> + 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.
> +
> + 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.
> + if (ret)
> + return ret;
> + }
> + return 0;
> +}
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 <[email protected]>
>> Signed-off-by: Mastan Katragadda <[email protected]>
>> ---
>> 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;
>> +}
>>> +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.
Not following, sorry.
We introduced the .startup callback for intel because of a power
dependency where we could not access and initialize the registers at the
.probe time for the master driver.
Do you have a similar dependency, and if not why not remove this flag?