Re: [PATCH 02/19] soundwire: amd: Add support for AMD Master driver

On 11/01/23 20:07, Pierre-Louis Bossart wrote:

On 1/11/23 03:02, Vijendar Mukunda wrote:

...

AMD ACP IP block has two soundwire controller devices.

s/controller/manager?

will rephrase the commit message.

...

Add support for

• Master driver probe & remove sequence
• Helper functions to enable/disable interrupts, Initialize sdw controller, enable sdw pads
• Master driver sdw_master_ops & port_ops callbacks

Signed-off-by: Vijendar Mukunda Vijendar.Mukunda@amd.com

drivers/soundwire/amd_master.c | 1075 +++++++++++++++++++++++++++++ drivers/soundwire/amd_master.h | 279 ++++++++ include/linux/soundwire/sdw_amd.h | 21 + 3 files changed, 1375 insertions(+) create mode 100644 drivers/soundwire/amd_master.c create mode 100644 drivers/soundwire/amd_master.h

diff --git a/drivers/soundwire/amd_master.c b/drivers/soundwire/amd_master.c new file mode 100644 index 000000000000..7e1f618254ac --- /dev/null +++ b/drivers/soundwire/amd_master.c @@ -0,0 +1,1075 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • SoundWire AMD Master 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/soundwire/sdw_amd.h> +#include <linux/wait.h> +#include <sound/pcm_params.h> +#include <sound/soc.h> +#include "bus.h" +#include "amd_master.h"

+#define DRV_NAME "amd_sdw_controller"

+#define to_amd_sdw(b) container_of(b, struct amd_sdwc_ctrl, bus)

+static int amd_enable_sdw_pads(struct amd_sdwc_ctrl *ctrl) +{

• u32 sw_pad_enable_mask;
• u32 sw_pad_pulldown_mask;
• u32 sw_pad_pulldown_val;
• u32 val = 0;
• switch (ctrl->instance) {

Goodness no. A controller has one or more masters. It cannot have pins as described in the SoundWire master specification.

we are referring to manager instance only. will modify the private data structure name.

...

• case ACP_SDW0:

• sw_pad_enable_mask = AMD_SDW0_PAD_KEEPER_EN_MASK;
  

• sw_pad_pulldown_mask = AMD_SDW0_PAD_PULLDOWN_CTRL_ENABLE_MASK;
  

• break;
  

• case ACP_SDW1:

• sw_pad_enable_mask = AMD_SDW1_PAD_KEEPER_EN_MASK;
  

• sw_pad_pulldown_mask = AMD_SDW1_PAD_PULLDOWN_CTRL_ENABLE_MASK;
  

• break;
  

• default:

• return -EINVAL;
  

• }
• mutex_lock(ctrl->sdw_lock);
• val = acp_reg_readl(ctrl->mmio + ACP_SW_PAD_KEEPER_EN);
• val |= sw_pad_enable_mask;
• acp_reg_writel(val, ctrl->mmio + ACP_SW_PAD_KEEPER_EN);
• mutex_unlock(ctrl->sdw_lock);
• usleep_range(1000, 1500);
• mutex_lock(ctrl->sdw_lock);
• sw_pad_pulldown_val = acp_reg_readl(ctrl->mmio + ACP_PAD_PULLDOWN_CTRL);
• sw_pad_pulldown_val &= sw_pad_pulldown_mask;
• acp_reg_writel(sw_pad_pulldown_val, ctrl->mmio + ACP_PAD_PULLDOWN_CTRL);
• mutex_unlock(ctrl->sdw_lock);
• return 0;

+}

+static int amd_init_sdw_controller(struct amd_sdwc_ctrl *ctrl) +{

• u32 acp_sw_en_reg, acp_sw_en_stat_reg, sw_bus_reset_reg;
• u32 val = 0;
• u32 timeout = 0;
• u32 retry_count = 0;
• switch (ctrl->instance) {
• case ACP_SDW0:

• acp_sw_en_reg = ACP_SW_EN;
  

• acp_sw_en_stat_reg = ACP_SW_EN_STATUS;
  

• sw_bus_reset_reg = ACP_SW_BUS_RESET_CTRL;
  

• break;
  

• case ACP_SDW1:

• acp_sw_en_reg = ACP_P1_SW_EN;
  

• acp_sw_en_stat_reg = ACP_P1_SW_EN_STATUS;
  

• sw_bus_reset_reg = ACP_P1_SW_BUS_RESET_CTRL;
  

• break;
  

• default:

• return -EINVAL;
  

• }
• acp_reg_writel(AMD_SDW_ENABLE, ctrl->mmio + acp_sw_en_reg);
• do {

• val = acp_reg_readl(ctrl->mmio + acp_sw_en_stat_reg);
  

• if (val)
  

• 	break;
  

• usleep_range(10, 50);
  

• } while (retry_count++ < AMD_SDW_STAT_MAX_RETRY_COUNT);
• if (retry_count > AMD_SDW_STAT_MAX_RETRY_COUNT)

• return -ETIMEDOUT;
  

• /* Sdw Controller reset */
• acp_reg_writel(AMD_SDW_BUS_RESET_REQ, ctrl->mmio + sw_bus_reset_reg);
• val = acp_reg_readl(ctrl->mmio + sw_bus_reset_reg);
• while (!(val & AMD_SDW_BUS_RESET_DONE)) {

• val = acp_reg_readl(ctrl->mmio + sw_bus_reset_reg);
  

• if (timeout > AMD_DELAY_LOOP_ITERATION)
  

• 	break;
  

• usleep_range(1, 5);
  

• timeout++;
  

• }

no test on timeout here to check if the bus was indeed reset?

It's a miss. we will add the code.

If you are talking about bus_reset you are referring to a master btw. The terms bus/master/link are interchangeable. A controller is not defined in the SoundWire specification, this is part of the DisCo spec to deal with enumeration when multiple bus/master/link are supported in the platform.

...

• timeout = 0;
• acp_reg_writel(AMD_SDW_BUS_RESET_CLEAR_REQ, ctrl->mmio + sw_bus_reset_reg);
• val = acp_reg_readl(ctrl->mmio + sw_bus_reset_reg);
• while (val) {

• val = acp_reg_readl(ctrl->mmio + sw_bus_reset_reg);
  

• if (timeout > AMD_DELAY_LOOP_ITERATION)
  

• 	break;
  

• usleep_range(1, 5);
  

• timeout++;
  

• }
• if (timeout == AMD_DELAY_LOOP_ITERATION) {

• dev_err(ctrl->dev, "Failed to reset SW%x Soundwire Controller\n", ctrl->instance);
  

• return -ETIMEDOUT;
  

• }
• retry_count = 0;
• acp_reg_writel(AMD_SDW_DISABLE, ctrl->mmio + acp_sw_en_reg);
• do {

• val = acp_reg_readl(ctrl->mmio + acp_sw_en_stat_reg);
  

• if (!val)
  

• 	break;
  

• usleep_range(10, 50);
  

• } 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_controller(struct amd_sdwc_ctrl *ctrl) +{

• u32 acp_sw_en_reg;
• u32 acp_sw_en_stat_reg;
• u32 val = 0;
• u32 retry_count = 0;
• switch (ctrl->instance) {
• case ACP_SDW0:

• acp_sw_en_reg = ACP_SW_EN;
  

• acp_sw_en_stat_reg = ACP_SW_EN_STATUS;
  

• break;
  

• case ACP_SDW1:

• acp_sw_en_reg = ACP_P1_SW_EN;
  

• acp_sw_en_stat_reg = ACP_P1_SW_EN_STATUS;
  

• break;
  

• default:

• return -EINVAL;
  

• }
• acp_reg_writel(AMD_SDW_ENABLE, ctrl->mmio + acp_sw_en_reg);
• do {

• val = acp_reg_readl(ctrl->mmio + acp_sw_en_stat_reg);
  

• if (val)
  

• 	break;
  

• usleep_range(10, 50);
  

• } 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_disable_sdw_controller(struct amd_sdwc_ctrl *ctrl) +{

• u32 clk_resume_ctrl_reg;
• u32 acp_sw_en_reg;
• u32 acp_sw_en_stat_reg;
• u32 val = 0;
• u32 retry_count = 0;
• switch (ctrl->instance) {
• case ACP_SDW0:

• acp_sw_en_reg = ACP_SW_EN;
  

• acp_sw_en_stat_reg = ACP_SW_EN_STATUS;
  

• clk_resume_ctrl_reg = ACP_SW_CLK_RESUME_CTRL;
  

• break;
  

• case ACP_SDW1:

• acp_sw_en_reg = ACP_P1_SW_EN;
  

• acp_sw_en_stat_reg = ACP_P1_SW_EN_STATUS;
  

• clk_resume_ctrl_reg = ACP_P1_SW_CLK_RESUME_CTRL;
  

• break;
  

• default:

• return -EINVAL;
  

• }
• acp_reg_writel(AMD_SDW_DISABLE, ctrl->mmio + acp_sw_en_reg);
• /*

• * After invoking controller disable sequence, check whether
  

• * controller has executed clock stop sequence. In this case,
  

• * controller should ignore checking enable status register.
  

again clock stop is a sequence at the master/link/bus level, not the controller.

Throughout the code we are referring manager instance only.

...

• */
  

• val = acp_reg_readl(ctrl->mmio + clk_resume_ctrl_reg);
• if (val)

• return 0;
  

• do {

• val = acp_reg_readl(ctrl->mmio + acp_sw_en_stat_reg);
  

• if (!val)
  

• 	break;
  

• usleep_range(10, 50);
  

• } 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_interrupts(struct amd_sdwc_ctrl *ctrl) +{

• u32 val;
• u32 acp_ext_intr_stat, acp_ext_intr_ctrl, acp_sdw_intr_mask;
• u32 sw_stat_mask_0to7, sw_stat_mask_8to11, sw_err_intr_mask;
• switch (ctrl->instance) {
• case ACP_SDW0:

• acp_ext_intr_ctrl = ACP_EXTERNAL_INTR_CNTL;
  

should be renamed and end in CNTL0 if the other is CNTL1

Its register header file macro. It's still good to go.

And it's manager anyways, not controller.

It's manager only.

...

• acp_sdw_intr_mask = AMD_SDW0_EXT_INTR_MASK;
  

• acp_ext_intr_stat = ACP_EXTERNAL_INTR_STAT;
  

• sw_stat_mask_0to7 = SW_STATE_CHANGE_STATUS_MASK_0TO7;
  

• sw_stat_mask_8to11 = SW_STATE_CHANGE_STATUS_MASK_8TO11;
  

• sw_err_intr_mask = SW_ERROR_INTR_MASK;
  

• break;
  

• case ACP_SDW1:

• acp_ext_intr_ctrl = ACP_EXTERNAL_INTR_CNTL1;
  

• acp_sdw_intr_mask = AMD_SDW1_EXT_INTR_MASK;
  

• acp_ext_intr_stat = ACP_EXTERNAL_INTR_STAT1;
  

• sw_stat_mask_0to7 = P1_SW_STATE_CHANGE_STATUS_MASK_0TO7;
  

• sw_stat_mask_8to11 = P1_SW_STATE_CHANGE_STATUS_MASK_8TO11;
  

• sw_err_intr_mask = P1_SW_ERROR_INTR_MASK;
  

• break;
  

• default:

• return -EINVAL;
  

• }
• mutex_lock(ctrl->sdw_lock);
• val = acp_reg_readl(ctrl->mmio + acp_ext_intr_ctrl);
• val |= acp_sdw_intr_mask;
• acp_reg_writel(val, ctrl->mmio + acp_ext_intr_ctrl);
• val = acp_reg_readl(ctrl->mmio + acp_ext_intr_ctrl);
• mutex_unlock(ctrl->sdw_lock);
• dev_dbg(ctrl->dev, "%s: acp_ext_intr_ctrl[0x%x]:0x%x\n", __func__, acp_ext_intr_ctrl, val);
• val = acp_reg_readl(ctrl->mmio + acp_ext_intr_stat);
• if (val)

• acp_reg_writel(val, ctrl->mmio + acp_ext_intr_stat);
  

• acp_reg_writel(AMD_SDW_IRQ_MASK_0TO7, ctrl->mmio + sw_stat_mask_0to7);
• acp_reg_writel(AMD_SDW_IRQ_MASK_8TO11, ctrl->mmio + sw_stat_mask_8to11);
• acp_reg_writel(AMD_SDW_IRQ_ERROR_MASK, ctrl->mmio + sw_err_intr_mask);
• return 0;

+}

+static u64 amd_sdwc_send_cmd_get_resp(struct amd_sdwc_ctrl *ctrl, u32 lword, u32 uword) +{

• u64 resp = 0;
• u32 imm_cmd_stat_reg, imm_cmd_uword_reg, imm_cmd_lword_reg;
• u32 imm_resp_uword_reg, imm_resp_lword_reg;
• u32 resp_lower, resp_high;
• u32 sts = 0;
• u32 timeout = 0;
• switch (ctrl->instance) {
• case ACP_SDW0:

• imm_cmd_stat_reg = SW_IMM_CMD_STS;
  

• imm_cmd_uword_reg = SW_IMM_CMD_UPPER_WORD;
  

• imm_cmd_lword_reg = SW_IMM_CMD_LOWER_QWORD;
  

• imm_resp_uword_reg = SW_IMM_RESP_UPPER_WORD;
  

• imm_resp_lword_reg = SW_IMM_RESP_LOWER_QWORD;
  

• break;
  

• case ACP_SDW1:

• imm_cmd_stat_reg = P1_SW_IMM_CMD_STS;
  

• imm_cmd_uword_reg = P1_SW_IMM_CMD_UPPER_WORD;
  

• imm_cmd_lword_reg = P1_SW_IMM_CMD_LOWER_QWORD;
  

• imm_resp_uword_reg = P1_SW_IMM_RESP_UPPER_WORD;
  

• imm_resp_lword_reg = P1_SW_IMM_RESP_LOWER_QWORD;
  

naming consistency would be good, the P1 is sometimes a prefix, sometimes a suffix, sometimes in the middle. Pick one.

These are taken from our AMD register header file. It's still good to go.

...

• break;
  

• default:

• return -EINVAL;
  

• }
• sts = acp_reg_readl(ctrl->mmio + imm_cmd_stat_reg);
• while (sts & AMD_SDW_IMM_CMD_BUSY) {

• sts = acp_reg_readl(ctrl->mmio + imm_cmd_stat_reg);
  

• if (timeout > AMD_SDW_RETRY_COUNT) {
  

• 	dev_err(ctrl->dev, "SDW%x previous cmd status clear failed\n",
  

• 		ctrl->instance);
  

• 	return -ETIMEDOUT;
  

• }
  

• timeout++;
  

• }
• timeout = 0;
• if (sts & AMD_SDW_IMM_RES_VALID) {

• dev_err(ctrl->dev, "SDW%x controller is in bad state\n", ctrl->instance);
  

• acp_reg_writel(0x00, ctrl->mmio + imm_cmd_stat_reg);
  

• }
• acp_reg_writel(uword, ctrl->mmio + imm_cmd_uword_reg);
• acp_reg_writel(lword, ctrl->mmio + imm_cmd_lword_reg);
• sts = acp_reg_readl(ctrl->mmio + imm_cmd_stat_reg);
• while (!(sts & AMD_SDW_IMM_RES_VALID)) {

• sts = acp_reg_readl(ctrl->mmio + imm_cmd_stat_reg);
  

• if (timeout > AMD_SDW_RETRY_COUNT) {
  

• 	dev_err(ctrl->dev, "SDW%x cmd response timeout occurred\n", ctrl->instance);
  

• 	return -ETIMEDOUT;
  

• }
  

• timeout++;
  

• }
• resp_high = acp_reg_readl(ctrl->mmio + imm_resp_uword_reg);
• resp_lower = acp_reg_readl(ctrl->mmio + imm_resp_lword_reg);
• timeout = 0;
• acp_reg_writel(AMD_SDW_IMM_RES_VALID, ctrl->mmio + imm_cmd_stat_reg);
• while ((sts & AMD_SDW_IMM_RES_VALID)) {

• sts = acp_reg_readl(ctrl->mmio + imm_cmd_stat_reg);
  

• if (timeout > AMD_SDW_RETRY_COUNT) {
  

• 	dev_err(ctrl->dev, "SDW%x cmd status retry failed\n", ctrl->instance);
  

• 	return -ETIMEDOUT;
  

• }
  

• timeout++;
  

• }
• resp = resp_high;
• resp = (resp << 32) | resp_lower;
• return resp;

+}

+static enum sdw_command_response +amd_program_scp_addr(struct amd_sdwc_ctrl *ctrl, struct sdw_msg *msg) +{

• struct sdw_msg scp_msg = {0};
• u64 response_buf[2] = {0};
• u32 uword = 0, lword = 0;
• int nack = 0, no_ack = 0;
• int index, timeout = 0;
• scp_msg.dev_num = msg->dev_num;
• scp_msg.addr = SDW_SCP_ADDRPAGE1;
• scp_msg.buf = &msg->addr_page1;
• amd_sdwc_ctl_word_prep(&lword, &uword, AMD_SDW_CMD_WRITE, &scp_msg, 0);
• response_buf[0] = amd_sdwc_send_cmd_get_resp(ctrl, lword, uword);
• scp_msg.addr = SDW_SCP_ADDRPAGE2;
• scp_msg.buf = &msg->addr_page2;
• amd_sdwc_ctl_word_prep(&lword, &uword, AMD_SDW_CMD_WRITE, &scp_msg, 0);
• response_buf[1] = amd_sdwc_send_cmd_get_resp(ctrl, lword, uword);
• /* check response the writes */

response to the writes? after the writes?

will correct the comment.

...

• for (index = 0; index < 2; index++) {

• if (response_buf[index] == -ETIMEDOUT) {
  

• 	dev_err(ctrl->dev, "Program SCP cmd timeout\n");
  

• 	timeout = 1;
  

• } else if (!(response_buf[index] & AMD_SDW_MCP_RESP_ACK)) {
  

• 	no_ack = 1;
  

• 	if (response_buf[index] & AMD_SDW_MCP_RESP_NACK) {
  

• 		nack = 1;
  

• 		dev_err(ctrl->dev, "Program SCP NACK received\n");
  

• 	}
  

this is a copy of the cadence_master.c code... With the error added that this is not for a controller but for a master...

Its manager instance only. Our immediate command and response mechanism allows sending commands over the link and get the response for every command immediately, unlike as mentioned in candence_master.c.

...

• }
  

• }
• if (timeout) {

• dev_err_ratelimited(ctrl->dev,
  

• 		    "SCP_addrpage command timeout for Slave %d\n", msg->dev_num);
  

• return SDW_CMD_TIMEOUT;
  

• }
• if (nack) {

• dev_err_ratelimited(ctrl->dev,
  

• 		    "SCP_addrpage NACKed for Slave %d\n", msg->dev_num);
  

• return SDW_CMD_FAIL;
  

• }
• if (no_ack) {

• dev_dbg_ratelimited(ctrl->dev,
  

• 		    "SCP_addrpage ignored for Slave %d\n", msg->dev_num);
  

• return SDW_CMD_IGNORED;
  

• }
• return SDW_CMD_OK;

this should probably become a helper since the response is really the same as in cadence_master.c

There's really room for optimization and reuse here.

not really needed. Please refer above comment as command/response mechanism differs from Intel's implementation.

...

+}

+static int amd_prep_msg(struct amd_sdwc_ctrl *ctrl, struct sdw_msg *msg, int *cmd) +{

• int ret;
• if (msg->page) {

• ret = amd_program_scp_addr(ctrl, msg);
  

• if (ret) {
  

• 	msg->len = 0;
  

• 	return ret;
  

• }
  

• }
• switch (msg->flags) {
• case SDW_MSG_FLAG_READ:

• *cmd = AMD_SDW_CMD_READ;
  

• break;
  

• case SDW_MSG_FLAG_WRITE:

• *cmd = AMD_SDW_CMD_WRITE;
  

• break;
  

• default:

• dev_err(ctrl->dev, "Invalid msg cmd: %d\n", msg->flags);
  

• return -EINVAL;
  

• }
• return 0;

+}

this is the same code as in cadence_master.c

you just replaced sdw_cnds by amd_sdw_ctrl (which is a mistake) and cdns->dev by ctrl->dev.

...

+static unsigned int _amd_sdwc_xfer_msg(struct amd_sdwc_ctrl *ctrl, struct sdw_msg *msg,

• 		       int cmd, int cmd_offset)
  

+{

• u64 response = 0;
• u32 uword = 0, lword = 0;
• int nack = 0, no_ack = 0;
• int timeout = 0;
• amd_sdwc_ctl_word_prep(&lword, &uword, cmd, msg, cmd_offset);
• response = amd_sdwc_send_cmd_get_resp(ctrl, lword, uword);
• if (response & AMD_SDW_MCP_RESP_ACK) {

• if (cmd == AMD_SDW_CMD_READ)
  

• 	msg->buf[cmd_offset] = FIELD_GET(AMD_SDW_MCP_RESP_RDATA, response);
  

• } else {

• no_ack = 1;
  

• if (response == -ETIMEDOUT) {
  

• 	timeout = 1;
  

• } else if (response & AMD_SDW_MCP_RESP_NACK) {
  

• 	nack = 1;
  

• 	dev_err(ctrl->dev, "Program SCP NACK received\n");
  

• }
  

• }
• if (timeout) {

• dev_err_ratelimited(ctrl->dev, "command timeout for Slave %d\n", msg->dev_num);
  

• return SDW_CMD_TIMEOUT;
  

• }
• if (nack) {

• dev_err_ratelimited(ctrl->dev,
  

• 		    "command response NACK received for Slave %d\n", msg->dev_num);
  

• return SDW_CMD_FAIL;
  

• }
• if (no_ack) {

• dev_err_ratelimited(ctrl->dev, "command is ignored for Slave %d\n", msg->dev_num);
  

• return SDW_CMD_IGNORED;
  

• }
• return SDW_CMD_OK;

+}

+static enum sdw_command_response amd_sdwc_xfer_msg(struct sdw_bus *bus, struct sdw_msg *msg) +{

• struct amd_sdwc_ctrl *ctrl = to_amd_sdw(bus);
• int ret, i;
• int cmd = 0;
• ret = amd_prep_msg(ctrl, msg, &cmd);
• if (ret)

• return SDW_CMD_FAIL_OTHER;
  

• for (i = 0; i < msg->len; i++) {

• ret = _amd_sdwc_xfer_msg(ctrl, msg, cmd, i);
  

• if (ret)
  

• 	return ret;
  

• }
• return SDW_CMD_OK;

+}

+static enum sdw_command_response +amd_reset_page_addr(struct sdw_bus *bus, unsigned int dev_num) +{

• struct amd_sdwc_ctrl *ctrl = to_amd_sdw(bus);
• struct sdw_msg msg;
• /* Create dummy message with valid device number */
• memset(&msg, 0, sizeof(msg));
• msg.dev_num = dev_num;
• return amd_program_scp_addr(ctrl, &msg);

+}

+static u32 amd_sdwc_read_ping_status(struct sdw_bus *bus) +{

• struct amd_sdwc_ctrl *ctrl = to_amd_sdw(bus);
• u64 response;
• u32 slave_stat = 0;
• response = amd_sdwc_send_cmd_get_resp(ctrl, 0, 0);
• /* slave status from ping response*/
• slave_stat = FIELD_GET(AMD_SDW_MCP_SLAVE_STAT_0_3, response);
• slave_stat |= FIELD_GET(AMD_SDW_MCP_SLAVE_STAT_4_11, response) << 8;
• dev_dbg(ctrl->dev, "%s: slave_stat:0x%x\n", __func__, slave_stat);
• return slave_stat;

+}

+static void amd_sdwc_compute_slave_ports(struct sdw_master_runtime *m_rt,

• 			 struct sdw_transport_data *t_data)
  

+{

• struct sdw_slave_runtime *s_rt = NULL;
• struct sdw_port_runtime *p_rt;
• int port_bo, sample_int;
• unsigned int rate, bps, ch = 0;
• unsigned int slave_total_ch;
• struct sdw_bus_params *b_params = &m_rt->bus->params;
• port_bo = t_data->block_offset;
• list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {

• rate = m_rt->stream->params.rate;
  

• bps = m_rt->stream->params.bps;
  

• sample_int = (m_rt->bus->params.curr_dr_freq / rate);
  

• slave_total_ch = 0;
  

• list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
  

• 	ch = sdw_ch_mask_to_ch(p_rt->ch_mask);
  

• 	sdw_fill_xport_params(&p_rt->transport_params,
  

• 			      p_rt->num, false,
  

• 			      SDW_BLK_GRP_CNT_1,
  

• 			      sample_int, port_bo, port_bo >> 8,
  

• 			      t_data->hstart,
  

• 			      t_data->hstop,
  

• 			      SDW_BLK_PKG_PER_PORT, 0x0);
  

• 	sdw_fill_port_params(&p_rt->port_params,
  

• 			     p_rt->num, bps,
  

• 			     SDW_PORT_FLOW_MODE_ISOCH,
  

• 			     b_params->s_data_mode);
  

• 	port_bo += bps * ch;
  

• 	slave_total_ch += ch;
  

• }
  

• if (m_rt->direction == SDW_DATA_DIR_TX &&
  

•     m_rt->ch_count == slave_total_ch) {
  

• 	port_bo = t_data->block_offset;
  

• }
  

• }

+}

ok, this is really bad.

This is a verbatim copy of the same function in generic_bandwidth_allocation.c

see https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.boo...

You only removed the comments and renamed the function.

Seriously? Why would you do that?

And in addition, this has *NOTHING* to do with the master support.

Programming the ports on peripheral side is something that happens at the stream level.

I am afraid it's a double NAK, or rather NAK^2 from me here.

Our intention is to implement our own compute params callback. Sorry, instead of making a copied one , we could have exported this API.

...

+static int amd_sdwc_compute_params(struct sdw_bus *bus) +{

• struct sdw_transport_data t_data = {0};
• struct sdw_master_runtime *m_rt;
• struct sdw_port_runtime *p_rt;
• struct sdw_bus_params *b_params = &bus->params;
• int port_bo, hstart, hstop, sample_int;
• unsigned int rate, bps;
• port_bo = 0;
• hstart = 1;
• hstop = bus->params.col - 1;
• t_data.hstop = hstop;
• t_data.hstart = hstart;
• list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {

• rate = m_rt->stream->params.rate;
  

• bps = m_rt->stream->params.bps;
  

• sample_int = (bus->params.curr_dr_freq / rate);
  

• list_for_each_entry(p_rt, &m_rt->port_list, port_node) {
  

• 	port_bo = (p_rt->num * 64) + 1;
  

• 	dev_dbg(bus->dev, "p_rt->num=%d hstart=%d hstop=%d port_bo=%d\n",
  

• 		p_rt->num, hstart, hstop, port_bo);
  

• 	sdw_fill_xport_params(&p_rt->transport_params, p_rt->num,
  

• 			      false, SDW_BLK_GRP_CNT_1, sample_int,
  

• 			      port_bo, port_bo >> 8, hstart, hstop,
  

• 			      SDW_BLK_PKG_PER_PORT, 0x0);
  

• 	sdw_fill_port_params(&p_rt->port_params,
  

• 			     p_rt->num, bps,
  

• 			     SDW_PORT_FLOW_MODE_ISOCH,
  

• 			     b_params->m_data_mode);
  

• 	t_data.hstart = hstart;
  

• 	t_data.hstop = hstop;
  

• 	t_data.block_offset = port_bo;
  

• 	t_data.sub_block_offset = 0;
  

• }
  

• amd_sdwc_compute_slave_ports(m_rt, &t_data);
  

• }
• return 0;

+}

this is a variation on sdw_compute_master_ports() in generic_allocation.c

You would need a lot more comments to convince me that this is intentional and needed.

This is intentional. We have a HW bug, if we go it generic bdw allocation API, when we launch multiple streams, we are observing noise for shorter duration for active stream. To avoid that, we have slightly modified the sdw_compute_master_ports() API. As of now we are enabling solution for 48khz, 2Ch, 16bit. We will expand the coverage in the future.

...

+static int amd_sdwc_port_params(struct sdw_bus *bus, struct sdw_port_params *p_params,

• 		unsigned int bank)
  

+{

• struct amd_sdwc_ctrl *ctrl = to_amd_sdw(bus);
• u32 channel_type, frame_fmt_reg, dpn_frame_fmt;
• dev_dbg(ctrl->dev, "%s: p_params->num:0x%x\n", __func__, p_params->num);
• switch (ctrl->instance) {
• case ACP_SDW0:

• channel_type = p_params->num;
  

• break;
  

• case ACP_SDW1:

• channel_type = p_params->num + ACP_SDW0_MAX_DAI;
  

• break;
  

• default:

• return -EINVAL;
  

• }
• switch (channel_type) {
• case ACP_SDW0_AUDIO_TX:

you'll have to explain what you mean by 'channel_type'

This looks like the streams that can be supported by this master implementation, with dailinks for each.

SDW0 Manager instance registers  6 CPU DAI (3 TX & 3 RX Ports) whereas SDW1  Manager Instance registers 2 CPU DAI (one TX & one RX port)

Each port number on Manager side is mapped to a channel number. i.e SDW0 Pin0 -> port number 0 -> Audio TX     SDW0 Pin1 -> Port number 1 -> Headset TX     SDW0 Pin2 -> Port number 2 -> BT TX     SDW0 Pin3 -> port number 3 -> Audio RX     SDW0 Pin4 -> Port number 4 -> Headset RX     SDW0 Pin5 -> Port number 5 -> BT RX

Whereas for SDW1 instance

SDW1 Pin0 -> port number 0 -> P1 BT TX     SDW1 Pin1 -> Port number 1 -> P1 BT RX      We use this channel value to program register set for transport params, port params and Channel enable for each manager instance. We need to use same channel mapping for programming DMA controller registers in Soundwire DMA driver. i.e if AUDIO TX channel is selected then we need to use Audio TX registers for DMA programming in Soundwire DMA driver.

...

• frame_fmt_reg = ACP_SW_AUDIO_TX_FRAME_FORMAT;
  

• break;
  

• case ACP_SDW0_HS_TX:

• frame_fmt_reg = ACP_SW_HEADSET_TX_FRAME_FORMAT;
  

• break;
  

• case ACP_SDW0_BT_TX:

• frame_fmt_reg = ACP_SW_BT_TX_FRAME_FORMAT;
  

• break;
  

• case ACP_SDW1_BT_TX:

• frame_fmt_reg = ACP_P1_SW_BT_TX_FRAME_FORMAT;
  

• break;
  

• case ACP_SDW0_AUDIO_RX:

• frame_fmt_reg = ACP_SW_AUDIO_RX_FRAME_FORMAT;
  

• break;
  

• case ACP_SDW0_HS_RX:

• frame_fmt_reg = ACP_SW_HEADSET_RX_FRAME_FORMAT;
  

• break;
  

• case ACP_SDW0_BT_RX:

• frame_fmt_reg = ACP_SW_BT_RX_FRAME_FORMAT;
  

• break;
  

• case ACP_SDW1_BT_RX:

• frame_fmt_reg = ACP_P1_SW_BT_RX_FRAME_FORMAT;
  

• break;
  

• default:

• dev_err(bus->dev, "%s:Invalid channel:%d\n", __func__, channel_type);
  

• return -EINVAL;
  

• }
• dpn_frame_fmt = acp_reg_readl(ctrl->mmio + frame_fmt_reg);
• u32p_replace_bits(&dpn_frame_fmt, p_params->flow_mode, AMD_DPN_FRAME_FMT_PFM);
• u32p_replace_bits(&dpn_frame_fmt, p_params->data_mode, AMD_DPN_FRAME_FMT_PDM);
• u32p_replace_bits(&dpn_frame_fmt, p_params->bps - 1, AMD_DPN_FRAME_FMT_WORD_LEN);
• acp_reg_writel(dpn_frame_fmt, ctrl->mmio + frame_fmt_reg);
• return 0;

+}

+static int amd_sdwc_transport_params(struct sdw_bus *bus,

• 		     struct sdw_transport_params *params,
  

• 		     enum sdw_reg_bank bank)
  

+{

• struct amd_sdwc_ctrl *ctrl = to_amd_sdw(bus);
• u32 ssp_counter_reg;
• u32 dpn_frame_fmt;
• u32 dpn_sampleinterval;
• u32 dpn_hctrl;
• u32 dpn_offsetctrl;
• u32 dpn_lanectrl;
• u32 channel_type;
• u32 frame_fmt_reg, sample_int_reg, hctrl_dp0_reg;
• u32 offset_reg, lane_ctrl_reg;
• switch (ctrl->instance) {
• case ACP_SDW0:

• ssp_counter_reg = ACP_SW_SSP_COUNTER;
  

• channel_type = params->port_num;
  

• break;
  

• case ACP_SDW1:

• ssp_counter_reg = ACP_P1_SW_SSP_COUNTER;
  

• channel_type = params->port_num + ACP_SDW0_MAX_DAI;
  

There's obviously a dependency between SDW0 and SDW1 managers that you haven't described?

No, both are independent manager instances which are connected in different power domains.

...

• break;
  

• default:

• return -EINVAL;
  

• }
• acp_reg_writel(AMD_SDW_SSP_COUNTER_VAL, ctrl->mmio + ssp_counter_reg);
• dev_dbg(bus->dev, "%s: p_params->num:0x%x entry channel_type:0x%x\n",

• __func__, params->port_num, channel_type);
  

• switch (channel_type) {
• case ACP_SDW0_AUDIO_TX:
• {

• frame_fmt_reg = ACP_SW_AUDIO_TX_FRAME_FORMAT;
  

• sample_int_reg = ACP_SW_AUDIO_TX_SAMPLEINTERVAL;
  

• hctrl_dp0_reg = ACP_SW_AUDIO_TX_HCTRL_DP0;
  

• offset_reg = ACP_SW_AUDIO_TX_OFFSET_DP0;
  

• lane_ctrl_reg = ACP_SW_AUDIO_TX_CHANNEL_ENABLE_DP0;
  

This is confusing. Is this about enabling a stream or selecting the lane for this port? Same for all cases.

is this saying that the two cases are handled by the same register - unlike what is normative for the peripherals where the two concepts are handeld in DPN_ChannelEn and DPN_LaneCtrl registers?

we have to refer the same register to program channel enable and lane ctrl as per our soundwire register definition.

...

• break;
  

• }
• case ACP_SDW0_HS_TX:
• {

• frame_fmt_reg = ACP_SW_HEADSET_TX_FRAME_FORMAT;
  

• sample_int_reg = ACP_SW_HEADSET_TX_SAMPLEINTERVAL;
  

• hctrl_dp0_reg = ACP_SW_HEADSET_TX_HCTRL;
  

• offset_reg = ACP_SW_HEADSET_TX_OFFSET;
  

• lane_ctrl_reg = ACP_SW_HEADSET_TX_CHANNEL_ENABLE_DP0;
  

• break;
  

• }
• case ACP_SDW0_BT_TX:
• {

• frame_fmt_reg = ACP_SW_BT_TX_FRAME_FORMAT;
  

• sample_int_reg = ACP_SW_BT_TX_SAMPLEINTERVAL;
  

• hctrl_dp0_reg = ACP_SW_BT_TX_HCTRL;
  

• offset_reg = ACP_SW_BT_TX_OFFSET;
  

• lane_ctrl_reg = ACP_SW_BT_TX_CHANNEL_ENABLE_DP0;
  

• break;
  

• }
• case ACP_SDW1_BT_TX:
• {

• frame_fmt_reg = ACP_P1_SW_BT_TX_FRAME_FORMAT;
  

• sample_int_reg = ACP_P1_SW_BT_TX_SAMPLEINTERVAL;
  

• hctrl_dp0_reg = ACP_P1_SW_BT_TX_HCTRL;
  

• offset_reg = ACP_P1_SW_BT_TX_OFFSET;
  

• lane_ctrl_reg = ACP_P1_SW_BT_TX_CHANNEL_ENABLE_DP0;
  

• break;
  

• }
• case ACP_SDW0_AUDIO_RX:
• {

• frame_fmt_reg = ACP_SW_AUDIO_RX_FRAME_FORMAT;
  

• sample_int_reg = ACP_SW_AUDIO_RX_SAMPLEINTERVAL;
  

• hctrl_dp0_reg = ACP_SW_AUDIO_RX_HCTRL_DP0;
  

• offset_reg = ACP_SW_AUDIO_RX_OFFSET_DP0;
  

• lane_ctrl_reg = ACP_SW_AUDIO_RX_CHANNEL_ENABLE_DP0;
  

• break;
  

• }
• case ACP_SDW0_HS_RX:
• {

• frame_fmt_reg = ACP_SW_HEADSET_RX_FRAME_FORMAT;
  

• sample_int_reg = ACP_SW_HEADSET_RX_SAMPLEINTERVAL;
  

• hctrl_dp0_reg = ACP_SW_HEADSET_RX_HCTRL;
  

• offset_reg = ACP_SW_HEADSET_RX_OFFSET;
  

• lane_ctrl_reg = ACP_SW_HEADSET_RX_CHANNEL_ENABLE_DP0;
  

• break;
  

• }
• case ACP_SDW0_BT_RX:
• {

• frame_fmt_reg = ACP_SW_BT_RX_FRAME_FORMAT;
  

• sample_int_reg = ACP_SW_BT_RX_SAMPLEINTERVAL;
  

• hctrl_dp0_reg = ACP_SW_BT_RX_HCTRL;
  

• offset_reg = ACP_SW_BT_RX_OFFSET;
  

• lane_ctrl_reg = ACP_SW_BT_RX_CHANNEL_ENABLE_DP0;
  

• break;
  

• }
• case ACP_SDW1_BT_RX:
• {

• frame_fmt_reg = ACP_P1_SW_BT_RX_FRAME_FORMAT;
  

• sample_int_reg = ACP_P1_SW_BT_RX_SAMPLEINTERVAL;
  

• hctrl_dp0_reg = ACP_P1_SW_BT_RX_HCTRL;
  

• offset_reg = ACP_P1_SW_BT_RX_OFFSET;
  

• lane_ctrl_reg = ACP_P1_SW_BT_RX_CHANNEL_ENABLE_DP0;
  

• break;
  

• }
• default:

• dev_err(bus->dev, "%s:Invalid channel:%d\n", __func__, channel_type);
  

• return -EINVAL;
  

• }
• dpn_frame_fmt = acp_reg_readl(ctrl->mmio + frame_fmt_reg);
• u32p_replace_bits(&dpn_frame_fmt, params->blk_pkg_mode, AMD_DPN_FRAME_FMT_BLK_PKG_MODE);
• u32p_replace_bits(&dpn_frame_fmt, params->blk_grp_ctrl, AMD_DPN_FRAME_FMT_BLK_GRP_CTRL);
• u32p_replace_bits(&dpn_frame_fmt, SDW_STREAM_PCM, AMD_DPN_FRAME_FMT_PCM_OR_PDM);
• acp_reg_writel(dpn_frame_fmt, ctrl->mmio + frame_fmt_reg);
• dpn_sampleinterval = params->sample_interval - 1;
• acp_reg_writel(dpn_sampleinterval, ctrl->mmio + sample_int_reg);
• dpn_hctrl = FIELD_PREP(AMD_DPN_HCTRL_HSTOP, params->hstop);
• dpn_hctrl |= FIELD_PREP(AMD_DPN_HCTRL_HSTART, params->hstart);
• acp_reg_writel(dpn_hctrl, ctrl->mmio + hctrl_dp0_reg);
• dpn_offsetctrl = FIELD_PREP(AMD_DPN_OFFSET_CTRL_1, params->offset1);
• dpn_offsetctrl |= FIELD_PREP(AMD_DPN_OFFSET_CTRL_2, params->offset2);
• acp_reg_writel(dpn_offsetctrl, ctrl->mmio + offset_reg);
• dpn_lanectrl = acp_reg_readl(ctrl->mmio + lane_ctrl_reg);
• u32p_replace_bits(&dpn_lanectrl, params->lane_ctrl, AMD_DPN_CH_EN_LCTRL);
• acp_reg_writel(dpn_lanectrl, ctrl->mmio + lane_ctrl_reg);
• return 0;

+}

+static int amd_sdwc_port_enable(struct sdw_bus *bus,

• 		struct sdw_enable_ch *enable_ch,
  

• 		unsigned int bank)
  

+{

• struct amd_sdwc_ctrl *ctrl = to_amd_sdw(bus);
• u32 dpn_ch_enable;
• u32 ch_enable_reg, channel_type;
• switch (ctrl->instance) {
• case ACP_SDW0:

• channel_type = enable_ch->port_num;
  

• break;
  

• case ACP_SDW1:

• channel_type = enable_ch->port_num + ACP_SDW0_MAX_DAI;
  

• break;
  

• default:

• return -EINVAL;
  

• }
• switch (channel_type) {
• case ACP_SDW0_AUDIO_TX:

• ch_enable_reg = ACP_SW_AUDIO_TX_CHANNEL_ENABLE_DP0;
  

in the function above, I commented on

lane_ctrl_reg = ACP_SW_AUDIO_TX_CHANNEL_ENABLE_DP0;

This looks really weird. You need to add comments is this is really intentional.

Please refer above comment reply. We will add comments.

...

• break;
  

• case ACP_SDW0_HS_TX:

• ch_enable_reg = ACP_SW_HEADSET_TX_CHANNEL_ENABLE_DP0;
  

• break;
  

• case ACP_SDW0_BT_TX:

• ch_enable_reg = ACP_SW_BT_TX_CHANNEL_ENABLE_DP0;
  

• break;
  

• case ACP_SDW1_BT_TX:

• ch_enable_reg = ACP_P1_SW_BT_TX_CHANNEL_ENABLE_DP0;
  

• break;
  

• case ACP_SDW0_AUDIO_RX:

• ch_enable_reg = ACP_SW_AUDIO_RX_CHANNEL_ENABLE_DP0;
  

• break;
  

• case ACP_SDW0_HS_RX:

• ch_enable_reg = ACP_SW_HEADSET_RX_CHANNEL_ENABLE_DP0;
  

• break;
  

• case ACP_SDW0_BT_RX:

• ch_enable_reg = ACP_SW_BT_RX_CHANNEL_ENABLE_DP0;
  

• break;
  

• case ACP_SDW1_BT_RX:

• ch_enable_reg = ACP_P1_SW_BT_RX_CHANNEL_ENABLE_DP0;
  

• break;
  

• default:

• dev_err(bus->dev, "%s:Invalid channel:%d\n", __func__, channel_type);
  

• return -EINVAL;
  

• }
• dpn_ch_enable = acp_reg_readl(ctrl->mmio + ch_enable_reg);
• u32p_replace_bits(&dpn_ch_enable, enable_ch->ch_mask, AMD_DPN_CH_EN_CHMASK);
• if (enable_ch->enable)

• acp_reg_writel(dpn_ch_enable, ctrl->mmio + ch_enable_reg);
  

• else

• acp_reg_writel(0, ctrl->mmio + ch_enable_reg);
  

• return 0;

+}

+static int sdw_master_read_amd_prop(struct sdw_bus *bus) +{

• struct amd_sdwc_ctrl *ctrl = to_amd_sdw(bus);
• struct fwnode_handle *link;
• struct sdw_master_prop *prop;
• u32 quirk_mask = 0;
• u32 wake_en_mask = 0;
• u32 power_mode_mask = 0;
• char name[32];
• prop = &bus->prop;
• /* Find master handle */
• snprintf(name, sizeof(name), "mipi-sdw-link-%d-subproperties", bus->link_id);
• link = device_get_named_child_node(bus->dev, name);
• if (!link) {

• dev_err(bus->dev, "Master node %s not found\n", name);
  

• return -EIO;
  

• }
• fwnode_property_read_u32(link, "amd-sdw-enable", &quirk_mask);
• if (!(quirk_mask & AMD_SDW_QUIRK_MASK_BUS_ENABLE))

• prop->hw_disabled = true;
  

same quirk as Intel, nice :-)

...

• prop->quirks = SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH |

•        SDW_MASTER_QUIRKS_CLEAR_INITIAL_PARITY;
  

And here too. Is this really needed or just-copy-pasted?

No, It's not a copy and paste. We have seen issues bus clash/parity errors during peripheral enumeration/initialization across the multiple links without this quirk. We have also seen device alerts missed during peripheral initialization sequence.

...

• fwnode_property_read_u32(link, "amd-sdw-wake-enable", &wake_en_mask);
• ctrl->wake_en_mask = wake_en_mask;
• fwnode_property_read_u32(link, "amd-sdw-power-mode", &power_mode_mask);
• ctrl->power_mode_mask = power_mode_mask;
• return 0;

+}

+static int amd_sdwc_probe(struct platform_device *pdev)

why not use an auxiliary device? we've moved away from platform devices maybe two years ago.

+{

...

• const struct acp_sdw_pdata *pdata = pdev->dev.platform_data;
• struct resource *res;
• struct device *dev = &pdev->dev;
• struct sdw_master_prop *prop;
• struct sdw_bus_params *params;
• struct amd_sdwc_ctrl *ctrl;
• int ret;
• if (!pdev->dev.platform_data) {

• dev_err(&pdev->dev, "platform_data not retrieved\n");
  

• return -ENODEV;
  

• }
• ctrl = devm_kzalloc(&pdev->dev, sizeof(struct amd_sdwc_ctrl), GFP_KERNEL);
• if (!ctrl)

• return -ENOMEM;
  

• res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
• if (!res) {

• dev_err(&pdev->dev, "IORESOURCE_MEM FAILED\n");
  

• return -ENOMEM;
  

• }
• ctrl->mmio = devm_ioremap(&pdev->dev, res->start, resource_size(res));
• if (IS_ERR(ctrl->mmio)) {

• dev_err(&pdev->dev, "mmio not found\n");
  

• return PTR_ERR(ctrl->mmio);
  

• }
• ctrl->instance = pdata->instance;
• ctrl->sdw_lock = pdata->sdw_lock;
• ctrl->rows_index = sdw_find_row_index(50);
• ctrl->cols_index = sdw_find_col_index(10);
• ctrl->dev = dev;
• dev_set_drvdata(&pdev->dev, ctrl);
• ctrl->bus.ops = &amd_sdwc_ops;
• ctrl->bus.port_ops = &amd_sdwc_port_ops;
• ctrl->bus.compute_params = &amd_sdwc_compute_params;
• ctrl->bus.clk_stop_timeout = 1;
• switch (ctrl->instance) {
• case ACP_SDW0:

• ctrl->num_dout_ports =  AMD_SDW0_MAX_TX_PORTS;
  

• ctrl->num_din_ports = AMD_SDW0_MAX_RX_PORTS;
  

• break;
  

• case ACP_SDW1:

• ctrl->num_dout_ports = AMD_SDW1_MAX_TX_PORTS;
  

• ctrl->num_din_ports = AMD_SDW1_MAX_RX_PORTS;
  

• break;
  

• default:

• return -EINVAL;
  

• }
• params = &ctrl->bus.params;
• params->max_dr_freq = AMD_SDW_DEFAULT_CLK_FREQ * 2;
• params->curr_dr_freq = AMD_SDW_DEFAULT_CLK_FREQ * 2;
• params->col = 10;
• params->row = 50;
• prop = &ctrl->bus.prop;
• prop->clk_freq = &amd_sdwc_freq_tbl[0];
• prop->mclk_freq = AMD_SDW_BUS_BASE_FREQ;
• ctrl->bus.link_id = ctrl->instance;
• ret = sdw_bus_master_add(&ctrl->bus, dev, dev->fwnode);
• if (ret) {

• dev_err(dev, "Failed to register Soundwire controller (%d)\n",
  

master. the confusion continues.

It's manager instance.

...

• 	ret);
  

• return ret;
  

• }
• INIT_WORK(&ctrl->probe_work, amd_sdwc_probe_work);
• schedule_work(&ctrl->probe_work);
• return 0;

+}

+static int amd_sdwc_remove(struct platform_device *pdev) +{

• struct amd_sdwc_ctrl *ctrl = dev_get_drvdata(&pdev->dev);
• int ret;
• amd_disable_sdw_interrupts(ctrl);
• sdw_bus_master_delete(&ctrl->bus);
• ret = amd_disable_sdw_controller(ctrl);
• return ret;

+}

+static struct platform_driver amd_sdwc_driver = {

• .probe = &amd_sdwc_probe,
• .remove = &amd_sdwc_remove,
• .driver = {

• .name	= "amd_sdw_controller",
  

• }

+}; +module_platform_driver(amd_sdwc_driver);

+MODULE_AUTHOR("Vijendar.Mukunda@amd.com"); +MODULE_DESCRIPTION("AMD soundwire driver"); +MODULE_LICENSE("GPL v2");

"GPL" is enough

will modify the license.

...

+enum amd_sdw_channel {

• /* SDW0 */
• ACP_SDW0_AUDIO_TX = 0,
• ACP_SDW0_BT_TX,
• ACP_SDW0_HS_TX,
• ACP_SDW0_AUDIO_RX,
• ACP_SDW0_BT_RX,
• ACP_SDW0_HS_RX,
• /* SDW1 */
• ACP_SDW1_BT_TX,
• ACP_SDW1_BT_RX,

+};

you really need to comment on this. It looks like you've special-cased manager ports for specific usages? This is perfectly fine in closed applications, but it's not clear how it might work with headset, amplifier and mic codec devices.

will add comments.

...

diff --git a/include/linux/soundwire/sdw_amd.h b/include/linux/soundwire/sdw_amd.h index f0123815af46..5ec39f8c2f2e 100644 --- a/include/linux/soundwire/sdw_amd.h +++ b/include/linux/soundwire/sdw_amd.h @@ -10,9 +10,30 @@

#define AMD_SDW_CLK_STOP_MODE 1 #define AMD_SDW_POWER_OFF_MODE 2 +#define ACP_SDW0 0 +#define ACP_SDW1 1 +#define ACP_SDW0_MAX_DAI 6

is this related to the definition of amd_sdw_channel or the number of ports available?

port number and channel count is same for SDW0 instance. Please go through channel mapping explanation mentioned in one of the above content.

...

struct acp_sdw_pdata { u16 instance; struct mutex *sdw_lock; };

+struct amd_sdwc_ctrl {

• struct sdw_bus bus;
• struct device *dev;
• void __iomem *mmio;
• struct work_struct probe_work;
• struct mutex *sdw_lock;

comment please.

will add comment.

...

• int num_din_ports;
• int num_dout_ports;
• int cols_index;
• int rows_index;
• u32 instance;
• u32 quirks;
• u32 wake_en_mask;
• int num_ports;
• bool startup_done;

ah this was an Intel definition. Due to power dependencies we had to split the probe and startup step. Does AMD have a need for this? Is the SoundWire master IP dependent on DSP boot or something?

if you are referring to startup_done flag, we have already explained in another patch reply.

...

• u32 power_mode_mask;

+}; #endif