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

Commit Message

Vijendar Mukunda Jan. 11, 2023, 9:02 a.m. UTC

AMD ACP IP block has two soundwire controller devices.
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 <[e-mail]>
---
 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

Comments

Amadeusz Sławiński Jan. 11, 2023, 1:59 p.m. UTC | #1

On 1/11/2023 10:02 AM, Vijendar Mukunda wrote:
> AMD ACP IP block has two soundwire controller devices.
> 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 <[e-mail]>
> ---

...

> +
> +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;

Double space before '='.

> +	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;
> +}
> +

...

> +
> +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;
> +		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);

Double space after '='.

> +	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 void amd_sdwc_probe_work(struct work_struct *work)
> +{
> +	struct amd_sdwc_ctrl *ctrl  = container_of(work, struct amd_sdwc_ctrl, probe_work);

Double space before '='.

> +	struct sdw_master_prop *prop;
> +	int ret;
> +
> +	prop = &ctrl->bus.prop;
> +	if (!prop->hw_disabled) {
> +		ret = amd_enable_sdw_pads(ctrl);
> +		if (ret)
> +			return;
> +		ret = amd_init_sdw_controller(ctrl);
> +		if (ret)
> +			return;
> +		amd_enable_sdw_interrupts(ctrl);
> +		ret = amd_enable_sdw_controller(ctrl);
> +		if (ret)
> +			return;
> +		ret = amd_sdwc_set_frameshape(ctrl, 50, 10);
> +		if (!ret)
> +			ctrl->startup_done = true;
> +	}
> +}
> +
> +static int amd_sdwc_probe(struct platform_device *pdev)
> +{
> +	const struct acp_sdw_pdata *pdata = pdev->dev.platform_data;
> +	struct resource *res;
> +	struct device *dev = &pdev->dev;

Same as in previous patch, you assign dev here, but keep using 
&pdev->dev below?

> +	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;

Double space before '='.

> +	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;

Double space after '='.

> +		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",
> +			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;
> +

You may need to cancel work if someone tries to unload driver before 
probe work completes. Something like

cancel_work_sync(&ctrl->probe_work);

should probably work here.

> +	amd_disable_sdw_interrupts(ctrl);
> +	sdw_bus_master_delete(&ctrl->bus);
> +	ret = amd_disable_sdw_controller(ctrl);
> +	return ret;
> +}
> +

Vijendar Mukunda Jan. 11, 2023, 2:16 p.m. UTC | #2

On 11/01/23 19:29, Amadeusz Sławiński wrote:
> On 1/11/2023 10:02 AM, Vijendar Mukunda wrote:
>> AMD ACP IP block has two soundwire controller devices.
>> 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 <[e-mail]>
>> ---
>
> ...
>
>> +
>> +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;
>
> Double space before '='.
will fix it.
>
>> +    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;
>> +}
>> +
>
> ...
>
>> +
>> +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;
>> +        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);
>
> Double space after '='.
>
will fix it.
>> +    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 void amd_sdwc_probe_work(struct work_struct *work)
>> +{
>> +    struct amd_sdwc_ctrl *ctrl  = container_of(work, struct amd_sdwc_ctrl, probe_work);
>
> Double space before '='.
Will fix it.
>
>> +    struct sdw_master_prop *prop;
>> +    int ret;
>> +
>> +    prop = &ctrl->bus.prop;
>> +    if (!prop->hw_disabled) {
>> +        ret = amd_enable_sdw_pads(ctrl);
>> +        if (ret)
>> +            return;
>> +        ret = amd_init_sdw_controller(ctrl);
>> +        if (ret)
>> +            return;
>> +        amd_enable_sdw_interrupts(ctrl);
>> +        ret = amd_enable_sdw_controller(ctrl);
>> +        if (ret)
>> +            return;
>> +        ret = amd_sdwc_set_frameshape(ctrl, 50, 10);
>> +        if (!ret)
>> +            ctrl->startup_done = true;
>> +    }
>> +}
>> +
>> +static int amd_sdwc_probe(struct platform_device *pdev)
>> +{
>> +    const struct acp_sdw_pdata *pdata = pdev->dev.platform_data;
>> +    struct resource *res;
>> +    struct device *dev = &pdev->dev;
>
> Same as in previous patch, you assign dev here, but keep using &pdev->dev below?
> will remove 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;
>
> Double space before '='.
> will fix it.
>> +    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;
>
> Double space after '='.
>
will fix it.
>> +        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",
>> +            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;
>> +
>
> You may need to cancel work if someone tries to unload driver before probe work completes. Something like
>
> cancel_work_sync(&ctrl->probe_work);
>
> should probably work here.
will fix it and post the v2 patch.
>
>> +    amd_disable_sdw_interrupts(ctrl);
>> +    sdw_bus_master_delete(&ctrl->bus);
>> +    ret = amd_disable_sdw_controller(ctrl);
>> +    return ret;
>> +}
>> +
>
>

Pierre-Louis Bossart Jan. 11, 2023, 2:37 p.m. UTC | #3

On 1/11/23 03:02, Vijendar Mukunda wrote:
> AMD ACP IP block has two soundwire controller devices.

s/controller/manager?

> 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 <[e-mail]>
> ---
>  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.

> +	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?

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.

> +	 */
> +	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

And it's manager anyways, not controller.

> +		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.

> +		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?

> +	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...

> +		}
> +	}
> +
> +	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.

> +}
> +
> +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://elixir.bootlin.com/linux/latest/source/drivers/soundwire/generic_bandwidth_allocation.c#L38

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.

> +
> +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.

> +
> +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.

> +		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?

> +		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?

> +		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.

> +		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?

> +	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.

> +			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("[e-mail]");
> +MODULE_DESCRIPTION("AMD soundwire driver");
> +MODULE_LICENSE("GPL v2");

"GPL" is enough

> +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.


> 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?
>  
>  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.

> +	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?

> +	u32 power_mode_mask;
> +};
>  #endif

Vijendar Mukunda Jan. 13, 2023, 6:21 p.m. UTC | #4

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 <[e-mail]>
>> ---
>>  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.bootlin.com%2Flinux%2Flatest%2Fsource%2Fdrivers%2Fsoundwire%2Fgeneric_bandwidth_allocation.c%23L38&data=05%7C01%7CVijendar.Mukunda%40amd.com%7Ccac3e7985a9347a69be908daf3f1ea8a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638090517594233520%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=MhN0eJYtggAnkUyb6mDIWW8LvwGeS8JJ%2F2UBHkHHOYo%3D&reserved=0
>
> 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("[e-mail]");
>> +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

Pierre-Louis Bossart Jan. 13, 2023, 6:41 p.m. UTC | #5

>>> +	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.

I don't get the reply. The Cadence IP also has the ability to get the
response immediately. There's limited scope for creativity, the commands
are defined in the spec and the responses as well.

>>> +		}
>>> +	}
>>> +
>>> +	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.

how? there's a buffer of responses in both cases. please clarify.

>>> +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.bootlin.com%2Flinux%2Flatest%2Fsource%2Fdrivers%2Fsoundwire%2Fgeneric_bandwidth_allocation.c%23L38&data=05%7C01%7CVijendar.Mukunda%40amd.com%7Ccac3e7985a9347a69be908daf3f1ea8a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638090517594233520%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=MhN0eJYtggAnkUyb6mDIWW8LvwGeS8JJ%2F2UBHkHHOYo%3D&reserved=0
>>
>> 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.

ok.

>>> +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.

That's fine, it's perfectly ok to have different strategies on the host
side. Exporting additional functions from generic_bandwidth_allocation.c
would help, you can pick what you need.
  
>>> +
>>> +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.

Ah, that's an important piece of information that should probably be
captured to help reviewers. On the Intel side the assignment from stream
types to ports is handled at the machine driver + topology level.


>>> +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.

if they are independent, then why does the channel type for SDW1 depends
on SDW0_MAX_DAI?

>>> +		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.

ok, please clarify with a comment. It's fine but different from other
implementations on device and host sides.

>>> +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.

ah, that's good to some extent that it wasn't the Intel IP behaving :-)

Vijendar Mukunda Jan. 16, 2023, 7:53 a.m. UTC | #6

On 14/01/23 00:11, Pierre-Louis Bossart wrote:
>>>> +	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.
> I don't get the reply. The Cadence IP also has the ability to get the
> response immediately. There's limited scope for creativity, the commands
> are defined in the spec and the responses as well.
As per our understanding in Intel code, responses are processed
after sending all commands.
In our case, we send the command and process the response
immediately before invoking the next command.
>>>> +		}
>>>> +	}
>>>> +
>>>> +	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.
> how? there's a buffer of responses in both cases. please clarify.
Ours implementation is not interrupt driven like Intel.
When we send command over the link, we will wait for command's
response in polling method and process the response immediately
before issuing the new command.

>>>> +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.bootlin.com%2Flinux%2Flatest%2Fsource%2Fdrivers%2Fsoundwire%2Fgeneric_bandwidth_allocation.c%23L38&data=05%7C01%7CVijendar.Mukunda%40amd.com%7Ccac3e7985a9347a69be908daf3f1ea8a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638090517594233520%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=MhN0eJYtggAnkUyb6mDIWW8LvwGeS8JJ%2F2UBHkHHOYo%3D&reserved=0
>>>
>>> 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.
> ok.
>
>>>> +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.
> That's fine, it's perfectly ok to have different strategies on the host
> side. Exporting additional functions from generic_bandwidth_allocation.c
> would help, you can pick what you need.
>   
>>>> +
>>>> +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.
> Ah, that's an important piece of information that should probably be
> captured to help reviewers. On the Intel side the assignment from stream
> types to ports is handled at the machine driver + topology level.
We will add comments in the code.
>
>
>>>> +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.
> if they are independent, then why does the channel type for SDW1 depends
> on SDW0_MAX_DAI?
There is no hard dependency for SDW1 on SDW0_MAX_DAI.
We will modify the code.
>>>> +		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.
> ok, please clarify with a comment. It's fine but different from other
> implementations on device and host sides.
Will add comment.
>
>>>> +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.
> ah, that's good to some extent that it wasn't the Intel IP behaving :-)
>

Pierre-Louis Bossart Jan. 16, 2023, 2:57 p.m. UTC | #7

On 1/16/23 01:53, Mukunda,Vijendar wrote:
> On 14/01/23 00:11, Pierre-Louis Bossart wrote:
>>>>> +	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.
>> I don't get the reply. The Cadence IP also has the ability to get the
>> response immediately. There's limited scope for creativity, the commands
>> are defined in the spec and the responses as well.
> As per our understanding in Intel code, responses are processed
> after sending all commands.
> In our case, we send the command and process the response
> immediately before invoking the next command.

The Cadence IP can queue a number of commands, I think 8 off the top of
my head. But the response is provided immediately after each command.

Maybe the disconnect is that there's an ability to define a watermark on
the response buffer, so that the software can decide to process the
command responses in one shot.

>>>>> +		}
>>>>> +	}
>>>>> +
>>>>> +	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.
>> how? there's a buffer of responses in both cases. please clarify.
> Ours implementation is not interrupt driven like Intel.
> When we send command over the link, we will wait for command's
> response in polling method and process the response immediately
> before issuing the new command.

On the Intel side we use an interrupt to avoid polling, and in case of N
commands the watermark will be set to N to reduce the overhead. That
said, most users only use 1 command at a time, it's only recently that
Cirrus Logic experimented with multiple commands to speed-up transfers.

Even if there are differences in the way the responses are processed,
whether one-at-a-time or in a batch, the point remains that each command
response can be individually analyzed and that could be using a helper -
moving code from cadence_master.c into the bus layer.

Vijendar Mukunda Jan. 17, 2023, 11:37 a.m. UTC | #8

On 16/01/23 20:27, Pierre-Louis Bossart wrote:
>
> On 1/16/23 01:53, Mukunda,Vijendar wrote:
>> On 14/01/23 00:11, Pierre-Louis Bossart wrote:
>>>>>> +	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.
>>> I don't get the reply. The Cadence IP also has the ability to get the
>>> response immediately. There's limited scope for creativity, the commands
>>> are defined in the spec and the responses as well.
>> As per our understanding in Intel code, responses are processed
>> after sending all commands.
>> In our case, we send the command and process the response
>> immediately before invoking the next command.
> The Cadence IP can queue a number of commands, I think 8 off the top of
> my head. But the response is provided immediately after each command.
>
> Maybe the disconnect is that there's an ability to define a watermark on
> the response buffer, so that the software can decide to process the
> command responses in one shot.
>
>>>>>> +		}
>>>>>> +	}
>>>>>> +
>>>>>> +	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.
>>> how? there's a buffer of responses in both cases. please clarify.
>> Ours implementation is not interrupt driven like Intel.
>> When we send command over the link, we will wait for command's
>> response in polling method and process the response immediately
>> before issuing the new command.
> On the Intel side we use an interrupt to avoid polling, and in case of N
> commands the watermark will be set to N to reduce the overhead. That
> said, most users only use 1 command at a time, it's only recently that
> Cirrus Logic experimented with multiple commands to speed-up transfers.
>
> Even if there are differences in the way the responses are processed,
> whether one-at-a-time or in a batch, the point remains that each command
> response can be individually analyzed and that could be using a helper -
> moving code from cadence_master.c into the bus layer.
>
> will implement a helper function to analyze the response.

Patch

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) {
+	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++;
+	}
+	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.
+	 */
+	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;
+		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 int amd_disable_sdw_interrupts(struct amd_sdwc_ctrl *ctrl)
+{
+	u32 val;
+	u32 acp_ext_intr_cntl;
+	u32 acp_sdw_intr_mask;
+	u32 sw_stat_mask_0to7;
+	u32 sw_stat_mask_8to11;
+	u32 sw_err_intr_mask;
+
+	switch (ctrl->instance) {
+	case ACP_SDW0:
+		acp_ext_intr_cntl = ACP_EXTERNAL_INTR_CNTL;
+		acp_sdw_intr_mask = AMD_SDW0_EXT_INTR_MASK;
+		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_cntl = ACP_EXTERNAL_INTR_CNTL1;
+		acp_sdw_intr_mask = AMD_SDW1_EXT_INTR_MASK;
+		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_cntl);
+	val &= ~acp_sdw_intr_mask;
+	acp_reg_writel(val, ctrl->mmio + acp_ext_intr_cntl);
+	mutex_unlock(ctrl->sdw_lock);
+
+	acp_reg_writel(0x00, ctrl->mmio + sw_stat_mask_0to7);
+	acp_reg_writel(0x00, ctrl->mmio + sw_stat_mask_8to11);
+	acp_reg_writel(0x00, ctrl->mmio + sw_err_intr_mask);
+	return 0;
+}
+
+static int amd_sdwc_set_frameshape(struct amd_sdwc_ctrl *ctrl, u32 rows, u32 cols)
+{
+	u32 sdw_rows, sdw_cols, frame_size;
+	u32 acp_sw_frame_reg;
+
+	switch (ctrl->instance) {
+	case ACP_SDW0:
+		acp_sw_frame_reg = ACP_SW_FRAMESIZE;
+		break;
+	case ACP_SDW1:
+		acp_sw_frame_reg = ACP_P1_SW_FRAMESIZE;
+		break;
+	default:
+		return -EINVAL;
+	}
+	sdw_cols = sdw_find_col_index(cols);
+	sdw_rows = sdw_find_row_index(rows);
+	frame_size = (sdw_rows << 3) | sdw_cols;
+	acp_reg_writel(frame_size, ctrl->mmio + acp_sw_frame_reg);
+	return 0;
+}
+
+static void amd_sdwc_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 = 0;
+	u16 addr;
+	u8 addr_high, addr_low;
+	u8 data = 0;
+
+	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_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;
+		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 */
+	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");
+			}
+		}
+	}
+
+	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;
+}
+
+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;
+}
+
+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;
+		}
+	}
+}
+
+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;
+}
+
+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:
+		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;
+		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;
+		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;
+		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;
+	prop->quirks = SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH |
+		       SDW_MASTER_QUIRKS_CLEAR_INITIAL_PARITY;
+
+	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_prop_read(struct sdw_bus *bus)
+{
+	sdw_master_read_prop(bus);
+	sdw_master_read_amd_prop(bus);
+	return 0;
+}
+
+static const struct sdw_master_port_ops amd_sdwc_port_ops = {
+	.dpn_set_port_params = amd_sdwc_port_params,
+	.dpn_set_port_transport_params = amd_sdwc_transport_params,
+	.dpn_port_enable_ch = amd_sdwc_port_enable,
+};
+
+static const struct sdw_master_ops amd_sdwc_ops = {
+	.read_prop = amd_prop_read,
+	.xfer_msg = amd_sdwc_xfer_msg,
+	.reset_page_addr = amd_reset_page_addr,
+	.read_ping_status = amd_sdwc_read_ping_status,
+};
+
+static void amd_sdwc_probe_work(struct work_struct *work)
+{
+	struct amd_sdwc_ctrl *ctrl  = container_of(work, struct amd_sdwc_ctrl, probe_work);
+	struct sdw_master_prop *prop;
+	int ret;
+
+	prop = &ctrl->bus.prop;
+	if (!prop->hw_disabled) {
+		ret = amd_enable_sdw_pads(ctrl);
+		if (ret)
+			return;
+		ret = amd_init_sdw_controller(ctrl);
+		if (ret)
+			return;
+		amd_enable_sdw_interrupts(ctrl);
+		ret = amd_enable_sdw_controller(ctrl);
+		if (ret)
+			return;
+		ret = amd_sdwc_set_frameshape(ctrl, 50, 10);
+		if (!ret)
+			ctrl->startup_done = true;
+	}
+}
+
+static int amd_sdwc_probe(struct platform_device *pdev)
+{
+	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",
+			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("[e-mail]");
+MODULE_DESCRIPTION("AMD soundwire driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" DRV_NAME);
+
diff --git a/drivers/soundwire/amd_master.h b/drivers/soundwire/amd_master.h
new file mode 100644
index 000000000000..42f32ca0c7a8
--- /dev/null
+++ b/drivers/soundwire/amd_master.h
@@ -0,0 +1,279 @@ 
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2023 Advanced Micro Devices, Inc. All rights reserved.
+ */
+
+#ifndef __AMD_MASTER_H
+#define __AMD_MASTER_H
+
+#define ACP_PAD_PULLDOWN_CTRL                         0x0001448
+#define ACP_SW_PAD_KEEPER_EN                          0x0001454
+#define ACP_SW_WAKE_EN                                0x0001458
+#define ACP_SW1_WAKE_EN                               0x0001460
+#define ACP_SW_I2S_ERROR_REASON                       0x00018B4
+
+#define ACP_EXTERNAL_INTR_ENB                         0x0001A00
+#define ACP_EXTERNAL_INTR_CNTL                        0x0001A04
+#define ACP_EXTERNAL_INTR_CNTL1                       0x0001A08
+#define ACP_EXTERNAL_INTR_STAT                        0x0001A0C
+#define ACP_EXTERNAL_INTR_STAT1                       0x0001A10
+#define ACP_ERROR_STATUS                              0x0001A4C
+#define ACP_P1_SW_I2S_ERROR_REASON                    0x0001A50
+
+#define ACP_SW_EN                                     0x0003000
+#define ACP_SW_EN_STATUS                              0x0003004
+#define ACP_SW_FRAMESIZE                              0x0003008
+#define ACP_SW_SSP_COUNTER                            0x000300C
+#define ACP_SW_AUDIO_TX_EN                            0x0003010
+#define ACP_SW_AUDIO_TX_EN_STATUS                     0x0003014
+#define ACP_SW_AUDIO_TX_FRAME_FORMAT                  0x0003018
+#define ACP_SW_AUDIO_TX_SAMPLEINTERVAL                0x000301C
+#define ACP_SW_AUDIO_TX_HCTRL_DP0                     0x0003020
+#define ACP_SW_AUDIO_TX_HCTRL_DP1                     0x0003024
+#define ACP_SW_AUDIO_TX_HCTRL_DP2                     0x0003028
+#define ACP_SW_AUDIO_TX_HCTRL_DP3                     0x000302C
+#define ACP_SW_AUDIO_TX_OFFSET_DP0                    0x0003030
+#define ACP_SW_AUDIO_TX_OFFSET_DP1                    0x0003034
+#define ACP_SW_AUDIO_TX_OFFSET_DP2                    0x0003038
+#define ACP_SW_AUDIO_TX_OFFSET_DP3                    0x000303C
+#define ACP_SW_AUDIO_TX_CHANNEL_ENABLE_DP0            0x0003040
+#define ACP_SW_AUDIO_TX_CHANNEL_ENABLE_DP1            0x0003044
+#define ACP_SW_AUDIO_TX_CHANNEL_ENABLE_DP2            0x0003048
+#define ACP_SW_AUDIO_TX_CHANNEL_ENABLE_DP3            0x000304C
+#define ACP_SW_BT_TX_EN                               0x0003050
+#define ACP_SW_BT_TX_EN_STATUS                        0x0003054
+#define ACP_SW_BT_TX_FRAME_FORMAT                     0x0003058
+#define ACP_SW_BT_TX_SAMPLEINTERVAL                   0x000305C
+#define ACP_SW_BT_TX_HCTRL                            0x0003060
+#define ACP_SW_BT_TX_OFFSET                           0x0003064
+#define ACP_SW_BT_TX_CHANNEL_ENABLE_DP0               0x0003068
+#define ACP_SW_HEADSET_TX_EN                          0x000306C
+#define ACP_SW_HEADSET_TX_EN_STATUS                   0x0003070
+#define ACP_SW_HEADSET_TX_FRAME_FORMAT                0x0003074
+#define ACP_SW_HEADSET_TX_SAMPLEINTERVAL              0x0003078
+#define ACP_SW_HEADSET_TX_HCTRL                       0x000307C
+#define ACP_SW_HEADSET_TX_OFFSET                      0x0003080
+#define ACP_SW_HEADSET_TX_CHANNEL_ENABLE_DP0          0x0003084
+#define ACP_SW_AUDIO_RX_EN                            0x0003088
+#define ACP_SW_AUDIO_RX_EN_STATUS                     0x000308C
+#define ACP_SW_AUDIO_RX_FRAME_FORMAT                  0x0003090
+#define ACP_SW_AUDIO_RX_SAMPLEINTERVAL                0x0003094
+#define ACP_SW_AUDIO_RX_HCTRL_DP0                     0x0003098
+#define ACP_SW_AUDIO_RX_HCTRL_DP1                     0x000309C
+#define ACP_SW_AUDIO_RX_HCTRL_DP2                     0x0003100
+#define ACP_SW_AUDIO_RX_HCTRL_DP3                     0x0003104
+#define ACP_SW_AUDIO_RX_OFFSET_DP0                    0x0003108
+#define ACP_SW_AUDIO_RX_OFFSET_DP1                    0x000310C
+#define ACP_SW_AUDIO_RX_OFFSET_DP2                    0x0003110
+#define ACP_SW_AUDIO_RX_OFFSET_DP3                    0x0003114
+#define ACP_SW_AUDIO_RX_CHANNEL_ENABLE_DP0            0x0003118
+#define ACP_SW_AUDIO_RX_CHANNEL_ENABLE_DP1            0x000311C
+#define ACP_SW_AUDIO_RX_CHANNEL_ENABLE_DP2            0x0003120
+#define ACP_SW_AUDIO_RX_CHANNEL_ENABLE_DP3            0x0003124
+#define ACP_SW_BT_RX_EN                               0x0003128
+#define ACP_SW_BT_RX_EN_STATUS                        0x000312C
+#define ACP_SW_BT_RX_FRAME_FORMAT                     0x0003130
+#define ACP_SW_BT_RX_SAMPLEINTERVAL                   0x0003134
+#define ACP_SW_BT_RX_HCTRL                            0x0003138
+#define ACP_SW_BT_RX_OFFSET                           0x000313C
+#define ACP_SW_BT_RX_CHANNEL_ENABLE_DP0               0x0003140
+#define ACP_SW_HEADSET_RX_EN                          0x0003144
+#define ACP_SW_HEADSET_RX_EN_STATUS                   0x0003148
+#define ACP_SW_HEADSET_RX_FRAME_FORMAT                0x000314C
+#define ACP_SW_HEADSET_RX_SAMPLEINTERVAL              0x0003150
+#define ACP_SW_HEADSET_RX_HCTRL                       0x0003154
+#define ACP_SW_HEADSET_RX_OFFSET                      0x0003158
+#define ACP_SW_HEADSET_RX_CHANNEL_ENABLE_DP0          0x000315C
+#define ACP_SW_BPT_PORT_EN                            0x0003160
+#define ACP_SW_BPT_PORT_EN_STATUS                     0x0003164
+#define ACP_SW_BPT_PORT_FRAME_FORMAT                  0x0003168
+#define ACP_SW_BPT_PORT_SAMPLEINTERVAL                0x000316C
+#define ACP_SW_BPT_PORT_HCTRL                         0x0003170
+#define ACP_SW_BPT_PORT_OFFSET                        0x0003174
+#define ACP_SW_BPT_PORT_CHANNEL_ENABLE                0x0003178
+#define ACP_SW_BPT_PORT_FIRST_BYTE_ADDR               0x000317C
+#define ACP_SW_CLK_RESUME_CTRL                        0x0003180
+#define ACP_SW_CLK_RESUME_DELAY_CNTR                  0x0003184
+#define ACP_SW_BUS_RESET_CTRL                         0x0003188
+#define ACP_SW_PRBS_ERR_STATUS                        0x000318C
+#define SW_IMM_CMD_UPPER_WORD                         0x0003230
+#define SW_IMM_CMD_LOWER_QWORD                        0x0003234
+#define SW_IMM_RESP_UPPER_WORD                        0x0003238
+#define SW_IMM_RESP_LOWER_QWORD                       0x000323C
+#define SW_IMM_CMD_STS                                0x0003240
+#define SW_BRA_BASE_ADDRESS                           0x0003244
+#define SW_BRA_TRANSFER_SIZE                          0x0003248
+#define SW_BRA_DMA_BUSY                               0x000324C
+#define SW_BRA_RESP                                   0x0003250
+#define SW_BRA_RESP_FRAME_ADDR                        0x0003254
+#define SW_BRA_CURRENT_TRANSFER_SIZE                  0x0003258
+#define SW_STATE_CHANGE_STATUS_0TO7                   0x000325C
+#define SW_STATE_CHANGE_STATUS_8TO11                  0x0003260
+#define SW_STATE_CHANGE_STATUS_MASK_0TO7              0x0003264
+#define SW_STATE_CHANGE_STATUS_MASK_8TO11             0x0003268
+#define SW_CLK_FREQUENCY_CTRL                         0x000326C
+#define SW_ERROR_INTR_MASK                            0x0003270
+#define SW_PHY_TEST_MODE_DATA_OFF                     0x0003274
+
+#define ACP_P1_SW_EN                                  0x0003C00
+#define ACP_P1_SW_EN_STATUS                           0x0003C04
+#define ACP_P1_SW_FRAMESIZE                           0x0003C08
+#define ACP_P1_SW_SSP_COUNTER                         0x0003C0C
+#define ACP_P1_SW_BT_TX_EN                            0x0003C50
+#define ACP_P1_SW_BT_TX_EN_STATUS                     0x0003C54
+#define ACP_P1_SW_BT_TX_FRAME_FORMAT                  0x0003C58
+#define ACP_P1_SW_BT_TX_SAMPLEINTERVAL                0x0003C5C
+#define ACP_P1_SW_BT_TX_HCTRL                         0x0003C60
+#define ACP_P1_SW_BT_TX_OFFSET                        0x0003C64
+#define ACP_P1_SW_BT_TX_CHANNEL_ENABLE_DP0            0x0003C68
+#define ACP_P1_SW_BT_RX_EN                            0x0003D28
+#define ACP_P1_SW_BT_RX_EN_STATUS                     0x0003D2C
+#define ACP_P1_SW_BT_RX_FRAME_FORMAT                  0x0003D30
+#define ACP_P1_SW_BT_RX_SAMPLEINTERVAL                0x0003D34
+#define ACP_P1_SW_BT_RX_HCTRL                         0x0003D38
+#define ACP_P1_SW_BT_RX_OFFSET                        0x0003D3C
+#define ACP_P1_SW_BT_RX_CHANNEL_ENABLE_DP0            0x0003D40
+#define ACP_P1_SW_BPT_PORT_EN                         0x0003D60
+#define ACP_P1_SW_BPT_PORT_EN_STATUS                  0x0003D64
+#define ACP_P1_SW_BPT_PORT_FRAME_FORMAT               0x0003D68
+#define ACP_P1_SW_BPT_PORT_SAMPLEINTERVAL             0x0003D6C
+#define ACP_P1_SW_BPT_PORT_HCTRL                      0x0003D70
+#define ACP_P1_SW_BPT_PORT_OFFSET                     0x0003D74
+#define ACP_P1_SW_BPT_PORT_CHANNEL_ENABLE             0x0003D78
+#define ACP_P1_SW_BPT_PORT_FIRST_BYTE_ADDR            0x0003D7C
+#define ACP_P1_SW_CLK_RESUME_CTRL                     0x0003D80
+#define ACP_P1_SW_CLK_RESUME_DELAY_CNTR               0x0003D84
+#define ACP_P1_SW_BUS_RESET_CTRL                      0x0003D88
+#define ACP_P1_SW_PRBS_ERR_STATUS                     0x0003D8C
+
+#define P1_SW_IMM_CMD_UPPER_WORD                      0x0003E30
+#define P1_SW_IMM_CMD_LOWER_QWORD                     0x0003E34
+#define P1_SW_IMM_RESP_UPPER_WORD                     0x0003E38
+#define P1_SW_IMM_RESP_LOWER_QWORD                    0x0003E3C
+#define P1_SW_IMM_CMD_STS                             0x0003E40
+#define P1_SW_BRA_BASE_ADDRESS                        0x0003E44
+#define P1_SW_BRA_TRANSFER_SIZE                       0x0003E48
+#define P1_SW_BRA_DMA_BUSY                            0x0003E4C
+#define P1_SW_BRA_RESP                                0x0003E50
+#define P1_SW_BRA_RESP_FRAME_ADDR                     0x0003E54
+#define P1_SW_BRA_CURRENT_TRANSFER_SIZE               0x0003E58
+#define P1_SW_STATE_CHANGE_STATUS_0TO7                0x0003E5C
+#define P1_SW_STATE_CHANGE_STATUS_8TO11               0x0003E60
+#define P1_SW_STATE_CHANGE_STATUS_MASK_0TO7           0x0003E64
+#define P1_SW_STATE_CHANGE_STATUS_MASK_8TO11          0x0003E68
+#define P1_SW_CLK_FREQUENCY_CTRL                      0x0003E6C
+#define P1_SW_ERROR_INTR_MASK                         0x0003E70
+#define P1_SW_PHY_TEST_MODE_DATA_OFF                  0x0003E74
+
+#define ACP_PHY_BASE_ADDRESS		0x1240000
+#define AMD_DELAY_LOOP_ITERATION	1000
+#define AMD_SDW_DEFAULT_CLK_FREQ	12000000
+#define AMD_SDW_RETRY_COUNT		1000
+
+#define AMD_SDW_MCP_RESP_ACK		BIT(0)
+#define AMD_SDW_MCP_RESP_NACK		BIT(1)
+#define AMD_SDW_MCP_RESP_RDATA		GENMASK(14, 7)
+
+#define AMD_SDW_MCP_CMD_SSP_TAG			BIT(31)
+#define AMD_SDW_MCP_CMD_COMMAND			GENMASK(14, 12)
+#define AMD_SDW_MCP_CMD_DEV_ADDR		GENMASK(11, 8)
+#define AMD_SDW_MCP_CMD_REG_ADDR_HIGH		GENMASK(7, 0)
+#define AMD_SDW_MCP_CMD_REG_ADDR_LOW		GENMASK(31, 24)
+#define AMD_SDW_MCP_CMD_REG_DATA		GENMASK(14, 7)
+#define AMD_SDW_MCP_SLAVE_STAT_0_3		GENMASK(14, 7)
+#define AMD_SDW_MCP_SLAVE_STAT_4_11		GENMASK(39, 24)
+#define AMD_SDW_MCP_SLAVE_STATUS_MASK		GENMASK(1, 0)
+#define AMD_SDW_MCP_SLAVE_STATUS_BITS		GENMASK(3, 2)
+#define AMD_SDW_MCP_SLAVE_STATUS_8TO_11		GENMASK(15, 0)
+#define AMD_SDW_MCP_SLAVE_STATUS_VALID_MASK(x)  BIT(((x) * 4))
+#define AMD_SDW_MCP_SLAVE_STAT_SHIFT_MASK(x)	(((x) * 4) + 1)
+
+#define AMD_SDW_MASTER_SUSPEND_DELAY_MS		3000
+#define AMD_SDW_CLK_STOP_MAX_RETRY_COUNT	100
+#define AMD_SDW_QUIRK_MASK_BUS_ENABLE		BIT(0)
+
+#define AMD_SDW_IMM_RES_VALID		1
+#define AMD_SDW_IMM_CMD_BUSY		2
+#define AMD_SDW_ENABLE			1
+#define AMD_SDW_DISABLE			0
+#define AMD_SDW_BUS_RESET_CLEAR_REQ	0
+#define AMD_SDW_BUS_RESET_REQ		1
+#define AMD_SDW_BUS_RESET_DONE		2
+#define AMD_SDW_BUS_BASE_FREQ		24000000
+
+#define AMD_SDW0_EXT_INTR_MASK		0x200000
+#define AMD_SDW1_EXT_INTR_MASK		4
+#define AMD_SDW_IRQ_MASK_0TO7		0x77777777
+#define AMD_SDW_IRQ_MASK_8TO11		0x000D7777
+#define AMD_SDW_IRQ_ERROR_MASK		0xFF
+#define AMD_SDW_MAX_FREQ_NUM		1
+#define AMD_SDW0_MAX_TX_PORTS		3
+#define AMD_SDW0_MAX_RX_PORTS		3
+#define AMD_SDW1_MAX_TX_PORTS		1
+#define AMD_SDW1_MAX_RX_PORTS		1
+
+#define AMD_SDW_SLAVE_0_ATTACHED	5
+#define AMD_SDW_SSP_COUNTER_VAL	3
+
+#define AMD_DPN_FRAME_FMT_PFM			GENMASK(1, 0)
+#define AMD_DPN_FRAME_FMT_PDM			GENMASK(3, 2)
+#define AMD_DPN_FRAME_FMT_BLK_PKG_MODE		BIT(4)
+#define AMD_DPN_FRAME_FMT_BLK_GRP_CTRL		GENMASK(6, 5)
+#define AMD_DPN_FRAME_FMT_WORD_LEN		GENMASK(12, 7)
+#define AMD_DPN_FRAME_FMT_PCM_OR_PDM		BIT(13)
+#define AMD_DPN_HCTRL_HSTOP			GENMASK(3, 0)
+#define AMD_DPN_HCTRL_HSTART			GENMASK(7, 4)
+#define AMD_DPN_OFFSET_CTRL_1			GENMASK(7, 0)
+#define AMD_DPN_OFFSET_CTRL_2			GENMASK(15, 8)
+#define AMD_DPN_CH_EN_LCTRL			GENMASK(2, 0)
+#define AMD_DPN_CH_EN_CHMASK			GENMASK(10, 3)
+#define AMD_SDW_STAT_MAX_RETRY_COUNT		100
+#define AMD_SDW0_PAD_PULLDOWN_CTRL_ENABLE_MASK	0x7F9F
+#define AMD_SDW1_PAD_PULLDOWN_CTRL_ENABLE_MASK	0x7FFA
+#define AMD_SDW0_PAD_PULLDOWN_CTRL_DISABLE_MASK	0x60
+#define AMD_SDW1_PAD_PULLDOWN_CTRL_DISABLE_MASK	5
+#define AMD_SDW0_PAD_KEEPER_EN_MASK	1
+#define AMD_SDW1_PAD_KEEPER_EN_MASK	0x10
+#define AMD_SDW0_PAD_KEEPER_DISABLE_MASK        0x1E
+#define AMD_SDW1_PAD_KEEPER_DISABLE_MASK	0xF
+
+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,
+};
+
+enum amd_sdw_cmd_type {
+	AMD_SDW_CMD_PING = 0,
+	AMD_SDW_CMD_READ = 2,
+	AMD_SDW_CMD_WRITE = 3,
+};
+
+static u32 amd_sdwc_freq_tbl[AMD_SDW_MAX_FREQ_NUM] = {
+	AMD_SDW_DEFAULT_CLK_FREQ,
+};
+
+struct sdw_transport_data {
+	int hstart;
+	int hstop;
+	int block_offset;
+	int sub_block_offset;
+};
+
+static inline u32 acp_reg_readl(void __iomem *base_addr)
+{
+	return readl(base_addr);
+}
+
+static inline void acp_reg_writel(u32 val, void __iomem *base_addr)
+{
+	writel(val, base_addr);
+}
+#endif
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
 
 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;
+	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;
+	u32 power_mode_mask;
+};
 #endif