--- Begin Message ---
- To: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>, vkoul@xxxxxxxxxx
- Subject: Re: [PATCH V6 2/8] soundwire: amd: Add support for AMD Manager driver
- From: "Mukunda,Vijendar" <vijendar.mukunda@xxxxxxx>
- Date: Wed, 8 Mar 2023 02:06:07 +0530
- Cc: alsa-devel@xxxxxxxxxxxxxxxx, Basavaraj.Hiregoudar@xxxxxxx, Sunil-kumar.Dommati@xxxxxxx, Mario.Limonciello@xxxxxxx, amadeuszx.slawinski@xxxxxxxxxxxxxxx, Mastan.Katragadda@xxxxxxx, Arungopal.kondaveeti@xxxxxxx, claudiu.beznea@xxxxxxxxxxxxx, Bard Liao <yung-chuan.liao@xxxxxxxxxxxxxxx>, Sanyog Kale <sanyog.r.kale@xxxxxxxxx>, open list <linux-kernel@xxxxxxxxxxxxxxx>
- In-reply-to: <09453549-73b3-bedb-89f6-61d482cabdf9@linux.intel.com>
- References: <20230307133135.545952-1-Vijendar.Mukunda@amd.com> <20230307133135.545952-3-Vijendar.Mukunda@amd.com> <09453549-73b3-bedb-89f6-61d482cabdf9@linux.intel.com>
- User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1
On 07/03/23 20:55, Pierre-Louis Bossart wrote: >> +static int amd_init_sdw_manager(struct amd_sdw_manager *amd_manager) >> +{ >> + u32 val; >> + int ret; >> + >> + acp_reg_writel(AMD_SDW_ENABLE, amd_manager->mmio + ACP_SW_EN); >> + ret = read_poll_timeout(acp_reg_readl, val, val, ACP_DELAY_US, AMD_SDW_TIMEOUT, false, >> + amd_manager->mmio + ACP_SW_EN_STATUS); >> + if (ret) >> + return ret; >> + >> + /* SoundWire manager bus reset */ >> + acp_reg_writel(AMD_SDW_BUS_RESET_REQ, amd_manager->mmio + ACP_SW_BUS_RESET_CTRL); >> + ret = read_poll_timeout(acp_reg_readl, val, (val & AMD_SDW_BUS_RESET_DONE), ACP_DELAY_US, >> + AMD_SDW_TIMEOUT, false, amd_manager->mmio + ACP_SW_BUS_RESET_CTRL); >> + if (ret) >> + return ret; >> + >> + acp_reg_writel(AMD_SDW_BUS_RESET_CLEAR_REQ, amd_manager->mmio + ACP_SW_BUS_RESET_CTRL); >> + ret = read_poll_timeout(acp_reg_readl, val, !val, ACP_DELAY_US, AMD_SDW_TIMEOUT, false, >> + amd_manager->mmio + ACP_SW_BUS_RESET_CTRL); >> + if (ret) { >> + dev_err(amd_manager->dev, "Failed to reset SoundWire manager instance%d\n", >> + amd_manager->instance); >> + return ret; >> + } >> + >> + acp_reg_writel(AMD_SDW_DISABLE, amd_manager->mmio + ACP_SW_EN); >> + return read_poll_timeout(acp_reg_readl, val, !val, ACP_DELAY_US, AMD_SDW_TIMEOUT, false, >> + amd_manager->mmio + ACP_SW_EN_STATUS); >> +} > ironically the change to use read_poll_timeout makes the code less clear > IMHO, specifically because the success criteria are > > 'val', 'val & AMD_SDW_BUS_RESET_DONE', '!val', '!val' > > It's hard to review and hard to spot potential issues. You may want to > add comments on what you are trying to check. Same comment for all the > rest of the code. I don't think it's really required to add comments for read_poll_timeout() API everywhere in the code. 'val', 'val & AMD_SDW_BUS_RESET_DONE', '!val', '!val' all these are break condition checks for register read operations. Rather than, I am happy to explain if any read_poll_timeout() logic in our code, hard to review. >> + >> +static int amd_enable_sdw_manager(struct amd_sdw_manager *amd_manager) >> +{ >> + u32 val; >> + >> + acp_reg_writel(AMD_SDW_ENABLE, amd_manager->mmio + ACP_SW_EN); >> + return read_poll_timeout(acp_reg_readl, val, val, ACP_DELAY_US, AMD_SDW_TIMEOUT, false, >> + amd_manager->mmio + ACP_SW_EN_STATUS); >> +} >> + >> +static int amd_disable_sdw_manager(struct amd_sdw_manager *amd_manager) >> +{ >> + u32 val; >> + >> + acp_reg_writel(AMD_SDW_DISABLE, amd_manager->mmio + ACP_SW_EN); >> + /* >> + * After invoking manager disable sequence, check whether >> + * manager has executed clock stop sequence. In this case, >> + * manager should ignore checking enable status register. >> + */ >> + val = acp_reg_readl(amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL); >> + if (val) >> + return 0; >> + return read_poll_timeout(acp_reg_readl, val, !val, ACP_DELAY_US, AMD_SDW_TIMEOUT, false, >> + amd_manager->mmio + ACP_SW_EN_STATUS); >> +} >> + >> +static void amd_enable_sdw_interrupts(struct amd_sdw_manager *amd_manager) >> +{ >> + struct sdw_manager_reg_mask *reg_mask = amd_manager->reg_mask; >> + u32 val; >> + >> + mutex_lock(amd_manager->acp_sdw_lock); >> + val = acp_reg_readl(amd_manager->acp_mmio + ACP_EXTERNAL_INTR_CNTL(amd_manager->instance)); >> + val |= reg_mask->acp_sdw_intr_mask; >> + acp_reg_writel(val, amd_manager->acp_mmio + ACP_EXTERNAL_INTR_CNTL(amd_manager->instance)); >> + mutex_unlock(amd_manager->acp_sdw_lock); >> + >> + acp_reg_writel(AMD_SDW_IRQ_MASK_0TO7, amd_manager->mmio + >> + ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7); >> + acp_reg_writel(AMD_SDW_IRQ_MASK_8TO11, amd_manager->mmio + >> + ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11); >> + acp_reg_writel(AMD_SDW_IRQ_ERROR_MASK, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK); >> +} >> + >> +static void amd_disable_sdw_interrupts(struct amd_sdw_manager *amd_manager) >> +{ >> + struct sdw_manager_reg_mask *reg_mask = amd_manager->reg_mask; >> + u32 val; >> + >> + mutex_lock(amd_manager->acp_sdw_lock); >> + val = acp_reg_readl(amd_manager->acp_mmio + ACP_EXTERNAL_INTR_CNTL(amd_manager->instance)); >> + val &= ~reg_mask->acp_sdw_intr_mask; >> + acp_reg_writel(val, amd_manager->acp_mmio + ACP_EXTERNAL_INTR_CNTL(amd_manager->instance)); >> + mutex_unlock(amd_manager->acp_sdw_lock); >> + >> + acp_reg_writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7); >> + acp_reg_writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11); >> + acp_reg_writel(0x00, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK); >> +} >> + >> +static void amd_sdw_set_frameshape(struct amd_sdw_manager *amd_manager) >> +{ >> + u32 frame_size; >> + >> + frame_size = (amd_manager->rows_index << 3) | amd_manager->cols_index; >> + acp_reg_writel(frame_size, amd_manager->mmio + ACP_SW_FRAMESIZE); >> +} >> + >> +static void amd_sdw_ctl_word_prep(u32 *lower_word, u32 *upper_word, u32 cmd_type, >> + struct sdw_msg *msg, int cmd_offset) >> +{ >> + u32 upper_data; >> + u32 lower_data = 0; >> + u16 addr; >> + u8 addr_upper, addr_lower; > nit-pick: use the same convention for data and addr, e.g. upper_data, > upper_addr. Same comment for the rest of the code. As it's not breaking anything, will fix it in supplement patch. >> + u8 data = 0; >> + >> + addr = msg->addr + cmd_offset; >> + addr_upper = (addr & 0xFF00) >> 8; >> + addr_lower = addr & 0xFF; >> + >> + if (cmd_type == AMD_SDW_CMD_WRITE) >> + data = msg->buf[cmd_offset]; >> + >> + upper_data = FIELD_PREP(AMD_SDW_MCP_CMD_DEV_ADDR, msg->dev_num); >> + upper_data |= FIELD_PREP(AMD_SDW_MCP_CMD_COMMAND, cmd_type); >> + upper_data |= FIELD_PREP(AMD_SDW_MCP_CMD_REG_ADDR_HIGH, addr_upper); >> + lower_data |= FIELD_PREP(AMD_SDW_MCP_CMD_REG_ADDR_LOW, addr_lower); >> + lower_data |= FIELD_PREP(AMD_SDW_MCP_CMD_REG_DATA, data); >> + >> + *upper_word = upper_data; >> + *lower_word = lower_data; >> +}
--- End Message ---
