| 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: Mon, 27 Feb 2023 12:08:44 -0600
From: Mario Limonciello <mario.limonciello@....com>
To: "Mukunda,Vijendar" <vijendar.mukunda@....com>,
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, amadeuszx.slawinski@...ux.intel.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 V4 2/8] soundwire: amd: Add support for AMD Manager driver
On 2/27/23 12:08, Mukunda,Vijendar wrote:
> On 27/02/23 22:05, Pierre-Louis Bossart wrote:
>>
>> On 2/27/23 10:47, Vijendar Mukunda wrote:
>>> AMD ACP(v6.x) IP block has two SoundWire manager devices.
>>> Add support for
>>> - Manager driver probe & remove sequence
>>> - Helper functions to enable/disable interrupts,
>>> Initialize sdw manager, enable sdw pads
>>> - Manager driver sdw_master_ops & port_ops callbacks
>>>
>>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@....com>
>>> ---
>>> drivers/soundwire/amd_manager.c | 749 ++++++++++++++++++++++++++++++
>>> drivers/soundwire/amd_manager.h | 252 ++++++++++
>>> include/linux/soundwire/sdw_amd.h | 67 +++
>>> 3 files changed, 1068 insertions(+)
>>> create mode 100644 drivers/soundwire/amd_manager.c
>>> create mode 100644 drivers/soundwire/amd_manager.h
>>> create mode 100644 include/linux/soundwire/sdw_amd.h
>>>
>>> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
>>> new file mode 100644
>>> index 000000000000..63cd8d066602
>>> --- /dev/null
>>> +++ b/drivers/soundwire/amd_manager.c
>>> @@ -0,0 +1,749 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * SoundWire AMD Manager driver
>>> + *
>>> + * Copyright 2023 Advanced Micro Devices, Inc.
>>> + */
>>> +
>>> +#include <linux/completion.h>
>>> +#include <linux/device.h>
>>> +#include <linux/io.h>
>>> +#include <linux/jiffies.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/soundwire/sdw.h>
>>> +#include <linux/soundwire/sdw_registers.h>
>>> +#include <linux/wait.h>
>>> +#include <sound/pcm_params.h>
>>> +#include <sound/soc.h>
>>> +#include "bus.h"
>>> +#include "amd_manager.h"
>>> +
>>> +#define DRV_NAME "amd_sdw_manager"
>>> +
>>> +#define to_amd_sdw(b) container_of(b, struct amd_sdw_manager, bus)
>>> +
>>> +static void amd_enable_sdw_pads(struct amd_sdw_manager *amd_manager)
>>> +{
>>> + u32 sw_pad_pulldown_val;
>>> + u32 val;
>>> +
>>> + mutex_lock(amd_manager->acp_sdw_lock);
>>> + val = acp_reg_readl(amd_manager->acp_mmio + ACP_SW_PAD_KEEPER_EN);
>>> + val |= amd_manager->reg_mask->sw_pad_enable_mask;
>>> + acp_reg_writel(val, amd_manager->acp_mmio + ACP_SW_PAD_KEEPER_EN);
>>> + usleep_range(1000, 1500);
>>> +
>>> + sw_pad_pulldown_val = acp_reg_readl(amd_manager->acp_mmio + ACP_PAD_PULLDOWN_CTRL);
>>> + sw_pad_pulldown_val &= amd_manager->reg_mask->sw_pad_pulldown_mask;
>>> + acp_reg_writel(sw_pad_pulldown_val, amd_manager->acp_mmio + ACP_PAD_PULLDOWN_CTRL);
>>> + mutex_unlock(amd_manager->acp_sdw_lock);
>>> +}
>>> +
>>> +static int amd_init_sdw_manager(struct amd_sdw_manager *amd_manager)
>>> +{
>>> + u32 val;
>>> + u32 timeout = 0;
>>> + u32 retry_count = 0;
>>> +
>>> + acp_reg_writel(AMD_SDW_ENABLE, amd_manager->mmio + ACP_SW_EN);
>>> + do {
>>> + val = acp_reg_readl(amd_manager->mmio + ACP_SW_EN_STATUS);
>>> + if (val)
>>> + break;
>>> + usleep_range(10, 20);
>>> + } while (retry_count++ < AMD_SDW_STAT_MAX_RETRY_COUNT);
>>> +
>>> + if (retry_count > AMD_SDW_STAT_MAX_RETRY_COUNT)
>>> + return -ETIMEDOUT;
>>> +
>>> + /* SoundWire manager bus reset */
>>> + acp_reg_writel(AMD_SDW_BUS_RESET_REQ, amd_manager->mmio + ACP_SW_BUS_RESET_CTRL);
>>> + val = acp_reg_readl(amd_manager->mmio + ACP_SW_BUS_RESET_CTRL);
>>> + while (!(val & AMD_SDW_BUS_RESET_DONE)) {
>>> + val = acp_reg_readl(amd_manager->mmio + ACP_SW_BUS_RESET_CTRL);
>>> + if (timeout > AMD_DELAY_LOOP_ITERATION)
>>> + break;
>>> + usleep_range(1, 5);
>>> + timeout++;
>>> + }
>>> + if (timeout > AMD_DELAY_LOOP_ITERATION)
>>> + return -ETIMEDOUT;
>> nit-pick: add a new line here since you're starting a different sequence.
>>
> will fix it.
>>> + timeout = 0;
>>> + acp_reg_writel(AMD_SDW_BUS_RESET_CLEAR_REQ, amd_manager->mmio + ACP_SW_BUS_RESET_CTRL);
>>> + val = acp_reg_readl(amd_manager->mmio + ACP_SW_BUS_RESET_CTRL);
>>> + while (val) {
>>> + val = acp_reg_readl(amd_manager->mmio + ACP_SW_BUS_RESET_CTRL);
>>> + if (timeout > AMD_DELAY_LOOP_ITERATION)
>>> + break;
>>> + usleep_range(1, 5);
>>> + timeout++;
>>> + }
>>> + if (timeout > AMD_DELAY_LOOP_ITERATION) {
>>> + dev_err(amd_manager->dev, "Failed to reset SoundWire manager instance%d\n",
>>> + amd_manager->instance);
>>> + return -ETIMEDOUT;
>>> + }
>> and same here, this is a new sequence starting here so a newline would help
> will fix it.
>>> + retry_count = 0;
>>> + acp_reg_writel(AMD_SDW_DISABLE, amd_manager->mmio + ACP_SW_EN);
>>> + do {
>>> + val = acp_reg_readl(amd_manager->mmio + ACP_SW_EN_STATUS);
>>> + if (!val)
>>> + break;
>>> + usleep_range(10, 20);
>>> + } while (retry_count++ < AMD_SDW_STAT_MAX_RETRY_COUNT);
>>> +
>>> + if (retry_count > AMD_SDW_STAT_MAX_RETRY_COUNT)
>>> + return -ETIMEDOUT;
>>> + return 0;
>>> +}
>>> +
>>> +static int amd_enable_sdw_manager(struct amd_sdw_manager *amd_manager)
>>> +{
>>> + u32 val;
>>> + u32 retry_count = 0;
>>> +
>>> + acp_reg_writel(AMD_SDW_ENABLE, amd_manager->mmio + ACP_SW_EN);
>>> + do {
>>> + val = acp_reg_readl(amd_manager->mmio + ACP_SW_EN_STATUS);
>>> + if (val)
>>> + break;
>>> + usleep_range(10, 20);
>>> + } while (retry_count++ < AMD_SDW_STAT_MAX_RETRY_COUNT);
>>> +
>>> + if (retry_count > AMD_SDW_STAT_MAX_RETRY_COUNT)
>>> + return -ETIMEDOUT;
>>> + return 0;
>>> +}
>>> +static void amd_sdw_ctl_word_prep(u32 *low_word, u32 *high_word, u32 cmd_type,
>>> + struct sdw_msg *msg, int cmd_offset)
>>> +{
>>> + u32 low_data = 0, high_data;
>>> + u16 addr;
>>> + u8 addr_high, addr_low;
>>> + u8 data = 0;
>> nit-pick: this looks a bit messy, usually it's simpler to have
>> initialized variables on separate lines. If you keep the low and high on
>> the same line, then at least use the same order.
> will fix it.
>>> +
>>> + addr = msg->addr + cmd_offset;
>>> + addr_high = (addr & 0xFF00) >> 8;
>>> + addr_low = addr & 0xFF;
>>> +
>>> + if (cmd_type == AMD_SDW_CMD_WRITE)
>>> + data = msg->buf[cmd_offset];
>>> +
>>> + high_data = FIELD_PREP(AMD_SDW_MCP_CMD_DEV_ADDR, msg->dev_num);
>>> + high_data |= FIELD_PREP(AMD_SDW_MCP_CMD_COMMAND, cmd_type);
>>> + high_data |= FIELD_PREP(AMD_SDW_MCP_CMD_REG_ADDR_HIGH, addr_high);
>>> + low_data |= FIELD_PREP(AMD_SDW_MCP_CMD_REG_ADDR_LOW, addr_low);
>>> + low_data |= FIELD_PREP(AMD_SDW_MCP_CMD_REG_DATA, data);
>>> +
>>> + *high_word = high_data;
>>> + *low_word = low_data;
>>> +}
>>> +
>>> +static u64 amd_sdw_send_cmd_get_resp(struct amd_sdw_manager *amd_manager, u32 lword, u32 uword)
>>> +{
>>> + u64 resp;
>>> + u32 resp_lower, resp_high;
>> nit-pick: resp_low? you seem to use this low/high pattern in a number of
>> places.
> will fix it.
>>> + u32 sts;
>>> + u32 timeout = 0;
>>> +
>>> + sts = acp_reg_readl(amd_manager->mmio + ACP_SW_IMM_CMD_STS);
>>> + while (sts & AMD_SDW_IMM_CMD_BUSY) {
>>> + if (timeout++ > AMD_SDW_RETRY_COUNT) {
>>> + dev_err(amd_manager->dev, "SDW%x previous cmd status clear failed\n",
>>> + amd_manager->instance);
>>> + return -ETIMEDOUT;
>>> + }
>>> + usleep_range(5, 10);
>>> + sts = acp_reg_readl(amd_manager->mmio + ACP_SW_IMM_CMD_STS);
>>> + }
>>> +
>>> + timeout = 0;
>>> + if (sts & AMD_SDW_IMM_RES_VALID) {
>>> + dev_err(amd_manager->dev, "SDW%x manager is in bad state\n", amd_manager->instance);
>>> + acp_reg_writel(0x00, amd_manager->mmio + ACP_SW_IMM_CMD_STS);
>>> + }
>>> + acp_reg_writel(uword, amd_manager->mmio + ACP_SW_IMM_CMD_UPPER_WORD);
>>> + acp_reg_writel(lword, amd_manager->mmio + ACP_SW_IMM_CMD_LOWER_QWORD);
>>> +
>>> + sts = acp_reg_readl(amd_manager->mmio + ACP_SW_IMM_CMD_STS);
>>> + while (!(sts & AMD_SDW_IMM_RES_VALID)) {
>>> + if (timeout++ > AMD_SDW_RETRY_COUNT) {
>>> + dev_err(amd_manager->dev, "SDW%x cmd response timeout occurred\n",
>>> + amd_manager->instance);
>>> + return -ETIMEDOUT;
>>> + }
>>> + usleep_range(5, 10);
>>> + sts = acp_reg_readl(amd_manager->mmio + ACP_SW_IMM_CMD_STS);
>>> + }
>>> + resp_high = acp_reg_readl(amd_manager->mmio + ACP_SW_IMM_RESP_UPPER_WORD);
>>> + resp_lower = acp_reg_readl(amd_manager->mmio + ACP_SW_IMM_RESP_LOWER_QWORD);
>> nit-pick: newline here.
> will fix it.
>>> + timeout = 0;
>>> + acp_reg_writel(AMD_SDW_IMM_RES_VALID, amd_manager->mmio + ACP_SW_IMM_CMD_STS);
>>> + sts = acp_reg_readl(amd_manager->mmio + ACP_SW_IMM_CMD_STS);
>>> + while ((sts & AMD_SDW_IMM_RES_VALID)) {
>>> + if (timeout++ > AMD_SDW_RETRY_COUNT) {
>>> + dev_err(amd_manager->dev, "SDW%x cmd status retry failed\n",
>>> + amd_manager->instance);
>>> + return -ETIMEDOUT;
>>> + }
>>> + usleep_range(5, 10);
>>> + sts = acp_reg_readl(amd_manager->mmio + ACP_SW_IMM_CMD_STS);
>>> + }
>>> + resp = resp_high;
>>> + resp = (resp << 32) | resp_lower;
>>> + return resp;
>>> +}
>>> +static unsigned int _amd_sdw_xfer_msg(struct amd_sdw_manager *amd_manager, struct sdw_msg *msg,
>>> + int cmd, int cmd_offset)
>>> +{
>>> + u64 response;
>>> + u32 uword = 0, lword = 0;
>> couldn't figure out what uword and lword refer to. Is this high/low as
>> above or something else?
> its high and low. Register naming convention uses
> upper word and lower word. will use naming convention
> throughout the code.
While fixing it, it would be good to start off the code with reverse
xmas tree everywhere too.
>>> +
>>> + amd_sdw_ctl_word_prep(&lword, &uword, cmd, msg, cmd_offset);
>>> + response = amd_sdw_send_cmd_get_resp(amd_manager, lword, uword);
>>> + return amd_sdw_fill_msg_resp(amd_manager, msg, response, cmd_offset);
>>> +}
>
Powered by blists - more mailing lists