| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <216b288d-16d3-5806-86fb-fc6ba83b757a@amd.com>
Date: Thu, 12 Jan 2023 16:05:29 +0530
From: "Mukunda,Vijendar" <vijendar.mukunda@....com>
To: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
broonie@...nel.org, vkoul@...nel.org, alsa-devel@...a-project.org
Cc: Basavaraj.Hiregoudar@....com, Sunil-kumar.Dommati@....com,
Mario.Limonciello@....com, Mastan.Katragadda@....com,
arungopal.kondaveeti@....com,
Bard Liao <yung-chuan.liao@...ux.intel.com>,
Sanyog Kale <sanyog.r.kale@...el.com>,
open list <linux-kernel@...r.kernel.org>
Subject: 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@....com>
>> Signed-off-by: Mastan Katragadda <Mastan.Katragadda@....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;
>> +}
Powered by blists - more mailing lists