Messages in this thread | | | Date | Mon, 27 Feb 2023 12:08:44 -0600 | Subject | Re: [PATCH V4 2/8] soundwire: amd: Add support for AMD Manager driver | From | Mario Limonciello <> |
| |
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@amd.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); >>> +} >
| |