| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2e5dbd97-b4ea-3846-b9a4-67b0064ad4e4@amd.com>
Date: Wed, 11 Jan 2023 19:46:54 +0530
From: "Mukunda,Vijendar" <vijendar.mukunda@....com>
To: Amadeusz Sławiński
<amadeuszx.slawinski@...ux.intel.com>, broonie@...nel.org,
vkoul@...nel.org, alsa-devel@...a-project.org
Cc: Mastan.Katragadda@....com, Sunil-kumar.Dommati@....com,
Basavaraj.Hiregoudar@....com,
Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
open list <linux-kernel@...r.kernel.org>,
Mario.Limonciello@....com, arungopal.kondaveeti@....com,
Sanyog Kale <sanyog.r.kale@...el.com>,
Bard Liao <yung-chuan.liao@...ux.intel.com>
Subject: Re: [PATCH 02/19] soundwire: amd: Add support for AMD Master driver
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 <Vijendar.Mukunda@....com>
>> ---
>
> ...
>
>> +
>> +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;
>> +}
>> +
>
>
Powered by blists - more mailing lists