On 13/02/23 23:50, Pierre-Louis Bossart wrote: > > On 2/13/23 03:40, Vijendar Mukunda wrote: >> Add support for runtime pm ops for AMD soundwire manager driver. >> >> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@xxxxxxx> >> Signed-off-by: Mastan Katragadda <Mastan.Katragadda@xxxxxxx> >> --- >> drivers/soundwire/amd_manager.c | 163 ++++++++++++++++++++++++++++++ >> drivers/soundwire/amd_manager.h | 3 + >> include/linux/soundwire/sdw_amd.h | 16 +++ >> 3 files changed, 182 insertions(+) >> >> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c >> index 87f9a987d93a..eced189ba6e0 100644 >> --- a/drivers/soundwire/amd_manager.c >> +++ b/drivers/soundwire/amd_manager.c >> @@ -14,6 +14,7 @@ >> #include <linux/slab.h> >> #include <linux/soundwire/sdw.h> >> #include <linux/soundwire/sdw_registers.h> >> +#include <linux/pm_runtime.h> >> #include <linux/wait.h> >> #include <sound/pcm_params.h> >> #include <sound/soc.h> >> @@ -185,6 +186,15 @@ static void amd_disable_sdw_interrupts(struct amd_sdw_manager *amd_manager) >> acp_reg_writel(0x00, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK); >> } >> >> +static int amd_deinit_sdw_manager(struct amd_sdw_manager *amd_manager) >> +{ >> + int ret; >> + >> + amd_disable_sdw_interrupts(amd_manager); >> + ret = amd_disable_sdw_manager(amd_manager); >> + return ret; >> +} >> + >> static void amd_sdw_set_frameshape(struct amd_sdw_manager *amd_manager) >> { >> u32 frame_size; >> @@ -1043,6 +1053,12 @@ static int amd_sdw_manager_probe(struct platform_device *pdev) >> INIT_WORK(&amd_manager->amd_sdw_work, amd_sdw_update_slave_status_work); >> INIT_WORK(&amd_manager->probe_work, amd_sdw_probe_work); >> schedule_work(&amd_manager->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); > that doesn't sound good to me, why do this here and not in the work > function? That creates a racy case where the device might suspend before > being initialized. This can be moved to work function. > >> return 0; >> } >> >> @@ -1057,14 +1073,161 @@ static int amd_sdw_manager_remove(struct platform_device *pdev) >> amd_disable_sdw_interrupts(amd_manager); >> sdw_bus_master_delete(&amd_manager->bus); >> ret = amd_disable_sdw_manager(amd_manager); >> + pm_runtime_disable(&pdev->dev); > shouldn't you do the pm_runtime_disable first? I agree. Will fix it. > >> return ret; >> } >> +/* AMD pm_runtime quirk definitions */ >> + >> +/* >> + * Force the clock to stop(ClockStopMode0) when suspend callback >> + * is invoked. >> + */ >> +#define AMD_SDW_CLK_STOP_MODE 1 >> + >> +/* >> + * Stop the bus when runtime suspend/system level suspend callback >> + * is invoked. If set, a complete bus reset and re-enumeration will >> + * be performed when the bus restarts. >> + */ >> +#define AMD_SDW_POWER_OFF_MODE 2 > You need to clarify this mode, can you deal with device in-band wakes if > the power is off? On the Current platform, in-band wakes are not supported if the power is off. > >> #define ACP_SDW0 0 >> #define ACP_SDW1 1 >> >> @@ -57,6 +71,7 @@ struct sdw_amd_dai_runtime { >> * @instance: soundwire manager instance >> * @quirks: soundwire manager quirks >> * @wake_en_mask: wake enable mask per soundwire manager >> + * @clk_stopped: flag set to true when clock is stopped >> * @power_mode_mask: flag interprets amd soundwire manager power mode >> * @dai_runtime_array: dai runtime array >> */ >> @@ -86,6 +101,7 @@ struct amd_sdw_manager { >> u32 quirks; >> u32 wake_en_mask; >> u32 power_mode_mask; >> + bool clk_stopped; >> >> struct sdw_amd_dai_runtime **dai_runtime_array; >> };