| 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
| ||
|
Date: Wed, 8 Mar 2023 02:06:07 +0530
From: "Mukunda,Vijendar" <vijendar.mukunda@....com>
To: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
vkoul@...nel.org
Cc: alsa-devel@...a-project.org, Basavaraj.Hiregoudar@....com,
Sunil-kumar.Dommati@....com, Mario.Limonciello@....com,
amadeuszx.slawinski@...ux.intel.com, Mastan.Katragadda@....com,
Arungopal.kondaveeti@....com, claudiu.beznea@...rochip.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 V6 2/8] soundwire: amd: Add support for AMD Manager driver
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;
>> +}
Powered by blists - more mailing lists