Re: [PATCH V3 6/8] soundwire: amd: add runtime pm ops for AMD SoundWire manager driver

On 21/02/23 21:40, Pierre-Louis Bossart wrote:

...

+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;

return amd_disable_sdw_manager(amd_manager); ?

will fix it.

...

+} +static int amd_sdw_clock_stop(struct amd_sdw_manager *amd_manager) +{

• u32 val;
• u32 retry_count = 0;
• int ret;
• ret = sdw_bus_prep_clk_stop(&amd_manager->bus);
• if (ret < 0 && ret != -ENODATA) {

• dev_err(amd_manager->dev, "prepare clock stop failed %d", ret);
  

• return 0;
  

• }
• ret = sdw_bus_clk_stop(&amd_manager->bus);
• if (ret < 0 && ret != -ENODATA) {

• dev_err(amd_manager->dev, "bus clock stop failed %d", ret);
  

• return 0;
  

• }
• do {

• val = acp_reg_readl(amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL);
  

don't you need a minimal usleep_range to avoid re-reading the same register over and over?

This is tied to the frame duration, isn't it? this is 20us typically.

I agree. will update the code with usleep_range(10,20)

...

• if (val & AMD_SDW_CLK_STOP_DONE) {
  

• 	amd_manager->clk_stopped = true;
  

• 	break;
  

• }
  

• } while (retry_count++ < AMD_SDW_CLK_STOP_MAX_RETRY_COUNT);
• if (!amd_manager->clk_stopped) {

• dev_err(amd_manager->dev, "SDW%x clock stop failed\n", amd_manager->instance);
  

• return 0;
  

• }
• if (amd_manager->wake_en_mask)

• acp_reg_writel(0x01, amd_manager->acp_mmio + ACP_SW_WAKE_EN(amd_manager->instance));
  

• dev_dbg(amd_manager->dev, "SDW%x clock stop successful\n", amd_manager->instance);
• return 0;

+}

+static int amd_sdw_clock_stop_exit(struct amd_sdw_manager *amd_manager) +{

• int ret;
• u32 val;
• u32 retry_count = 0;
• if (amd_manager->clk_stopped) {

• val = acp_reg_readl(amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL);
  

• val |= AMD_SDW_CLK_RESUME_REQ;
  

• acp_reg_writel(val, amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL);
  

• do {
  

• 	val = acp_reg_readl(amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL);
  

• 	if (val & AMD_SDW_CLK_RESUME_DONE)
  

• 		break;
  

• 	usleep_range(10, 100);
  

that's 10x range for sleep, that sounds a vague and suspicious, no?

Agreed . usleep_range(10, 20) is enough in this case. will fix it.

...

• } while (retry_count++ < AMD_SDW_CLK_STOP_MAX_RETRY_COUNT);
  

• if (val & AMD_SDW_CLK_RESUME_DONE) {
  

• 	acp_reg_writel(0, amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL);
  

• 	ret = sdw_bus_exit_clk_stop(&amd_manager->bus);
  

• 	if (ret < 0)
  

• 		dev_err(amd_manager->dev, "bus failed to exit clock stop %d\n",
  

• 			ret);
  

• 	amd_manager->clk_stopped = false;
  

• }
  

• }
• if (amd_manager->clk_stopped) {

• dev_err(amd_manager->dev, "SDW%x clock stop exit failed\n", amd_manager->instance);
  

• return 0;
  

• }
• dev_dbg(amd_manager->dev, "SDW%x clock stop exit successful\n", amd_manager->instance);
• return 0;