| Message ID | 20230220100418.76754-1-Vijendar.Mukunda@amd.com |
|---|---|
| Headers | show |
| Series | Add SoundWire support for AMD platforms | expand |
> +static int amd_init_sdw_manager(struct amd_sdw_manager *amd_manager) > +{ > + u32 val; > + u32 timeout = 0; > + u32 retry_count = 0; > + > + acp_reg_writel(AMD_SDW_ENABLE, amd_manager->mmio + ACP_SW_EN); > + do { > + val = acp_reg_readl(amd_manager->mmio + ACP_SW_EN_STATUS); > + if (val) > + break; > + usleep_range(10, 50); that's a 5x range used for all usleep_range() in this file, is this intentional? > + } while (retry_count++ < AMD_SDW_STAT_MAX_RETRY_COUNT); > + > + if (retry_count > AMD_SDW_STAT_MAX_RETRY_COUNT) > + return -ETIMEDOUT; > + > + /* SoundWire manager bus reset */ > + acp_reg_writel(AMD_SDW_BUS_RESET_REQ, amd_manager->mmio + ACP_SW_BUS_RESET_CTRL); > + val = acp_reg_readl(amd_manager->mmio + ACP_SW_BUS_RESET_CTRL); > + while (!(val & AMD_SDW_BUS_RESET_DONE)) { > + val = acp_reg_readl(amd_manager->mmio + ACP_SW_BUS_RESET_CTRL); > + if (timeout > AMD_DELAY_LOOP_ITERATION) > + break; so if you break here... > + usleep_range(1, 5); > + timeout++; > + } > + if (timeout == AMD_DELAY_LOOP_ITERATION) > + return -ETIMEDOUT; ... this test will never be used. the timeout management looks wrong? > + timeout = 0; > + acp_reg_writel(AMD_SDW_BUS_RESET_CLEAR_REQ, amd_manager->mmio + ACP_SW_BUS_RESET_CTRL); > + val = acp_reg_readl(amd_manager->mmio + ACP_SW_BUS_RESET_CTRL); > + while (val) { > + val = acp_reg_readl(amd_manager->mmio + ACP_SW_BUS_RESET_CTRL); > + if (timeout > AMD_DELAY_LOOP_ITERATION) > + break; same here... > + usleep_range(1, 5); > + timeout++; > + } > + if (timeout == AMD_DELAY_LOOP_ITERATION) { ... and here. > + dev_err(amd_manager->dev, "Failed to reset SoundWire manager instance%d\n", > + amd_manager->instance); > + return -ETIMEDOUT; > + } > + retry_count = 0; > + acp_reg_writel(AMD_SDW_DISABLE, amd_manager->mmio + ACP_SW_EN); > + do { > + val = acp_reg_readl(amd_manager->mmio + ACP_SW_EN_STATUS); > + if (!val) > + break; > + usleep_range(10, 50); > + } while (retry_count++ < AMD_SDW_STAT_MAX_RETRY_COUNT); > + > + if (retry_count > AMD_SDW_STAT_MAX_RETRY_COUNT) > + return -ETIMEDOUT; that one looks correct though. > + return 0; > +} > +static void amd_enable_sdw_interrupts(struct amd_sdw_manager *amd_manager) > +{ > + struct sdw_manager_reg_mask *reg_mask = amd_manager->reg_mask; > + u32 val; > + > + mutex_lock(amd_manager->acp_sdw_lock); > + val = acp_reg_readl(amd_manager->acp_mmio + ACP_EXTERNAL_INTR_CNTL(amd_manager->instance)); > + val |= reg_mask->acp_sdw_intr_mask; > + acp_reg_writel(val, amd_manager->acp_mmio + ACP_EXTERNAL_INTR_CNTL(amd_manager->instance)); here you handle a read-modify-write sequence on the INTL_CNTL register... > + val = acp_reg_readl(amd_manager->acp_mmio + ACP_EXTERNAL_INTR_CNTL(amd_manager->instance)); ... but here you only read from the same register. what's the purpose of this read? > + mutex_unlock(amd_manager->acp_sdw_lock); > + dev_dbg(amd_manager->dev, "acp_ext_intr_ctrl[0x%x]:0x%x\n", > + ACP_EXTERNAL_INTR_CNTL(amd_manager->instance), val); > + val = acp_reg_readl(amd_manager->acp_mmio + ACP_EXTERNAL_INTR_STAT(amd_manager->instance)); > + if (val) > + acp_reg_writel(val, amd_manager->acp_mmio + > + ACP_EXTERNAL_INTR_STAT(amd_manager->instance)); > + > + acp_reg_writel(AMD_SDW_IRQ_MASK_0TO7, amd_manager->mmio + > + ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7); > + acp_reg_writel(AMD_SDW_IRQ_MASK_8TO11, amd_manager->mmio + > + ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11); > + acp_reg_writel(AMD_SDW_IRQ_ERROR_MASK, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK); can you explain why the writes are not protected by the mutex? > +} > + > +static void amd_disable_sdw_interrupts(struct amd_sdw_manager *amd_manager) > +{ > + struct sdw_manager_reg_mask *reg_mask = amd_manager->reg_mask; > + u32 val; > + > + mutex_lock(amd_manager->acp_sdw_lock); > + val = acp_reg_readl(amd_manager->acp_mmio + ACP_EXTERNAL_INTR_CNTL(amd_manager->instance)); > + val &= ~reg_mask->acp_sdw_intr_mask; > + acp_reg_writel(val, amd_manager->acp_mmio + ACP_EXTERNAL_INTR_CNTL(amd_manager->instance)); you don't have the read here as in the enable sequence to presumably that wasn't needed? > + mutex_unlock(amd_manager->acp_sdw_lock); > + > + acp_reg_writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7); > + acp_reg_writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11); > + acp_reg_writel(0x00, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK); same, not clear why the writes are not protected? > +} ... > +static u64 amd_sdw_send_cmd_get_resp(struct amd_sdw_manager *amd_manager, u32 lword, u32 uword) > +{ > + u64 resp; > + u32 resp_lower, resp_high; > + u32 sts; > + u32 timeout = 0; > + > + sts = acp_reg_readl(amd_manager->mmio + ACP_SW_IMM_CMD_STS); > + while (sts & AMD_SDW_IMM_CMD_BUSY) { > + sts = acp_reg_readl(amd_manager->mmio + ACP_SW_IMM_CMD_STS); > + if (timeout > AMD_SDW_RETRY_COUNT) { > + dev_err(amd_manager->dev, "SDW%x previous cmd status clear failed\n", > + amd_manager->instance); > + return -ETIMEDOUT; > + } > + timeout++; no usleep_range() here? Also is there any merit in using the same retry count across the board, this is about command handling, not enabling the hardware - presumably this should be tied to the SoundWire bus frame timing, not internal delays. > + } > + > + timeout = 0; > + if (sts & AMD_SDW_IMM_RES_VALID) { > + dev_err(amd_manager->dev, "SDW%x manager is in bad state\n", amd_manager->instance); > + acp_reg_writel(0x00, amd_manager->mmio + ACP_SW_IMM_CMD_STS); > + } > + acp_reg_writel(uword, amd_manager->mmio + ACP_SW_IMM_CMD_UPPER_WORD); > + acp_reg_writel(lword, amd_manager->mmio + ACP_SW_IMM_CMD_LOWER_QWORD); > + > + sts = acp_reg_readl(amd_manager->mmio + ACP_SW_IMM_CMD_STS); > + while (!(sts & AMD_SDW_IMM_RES_VALID)) { > + sts = acp_reg_readl(amd_manager->mmio + ACP_SW_IMM_CMD_STS); > + if (timeout > AMD_SDW_RETRY_COUNT) { > + dev_err(amd_manager->dev, "SDW%x cmd response timeout occurred\n", > + amd_manager->instance); > + return -ETIMEDOUT; usleep_range? > + } > + timeout++; > + } > + resp_high = acp_reg_readl(amd_manager->mmio + ACP_SW_IMM_RESP_UPPER_WORD); > + resp_lower = acp_reg_readl(amd_manager->mmio + ACP_SW_IMM_RESP_LOWER_QWORD); > + timeout = 0; > + acp_reg_writel(AMD_SDW_IMM_RES_VALID, amd_manager->mmio + ACP_SW_IMM_CMD_STS); > + while ((sts & AMD_SDW_IMM_RES_VALID)) { > + sts = acp_reg_readl(amd_manager->mmio + ACP_SW_IMM_CMD_STS); > + if (timeout > AMD_SDW_RETRY_COUNT) { > + dev_err(amd_manager->dev, "SDW%x cmd status retry failed\n", > + amd_manager->instance); > + return -ETIMEDOUT; > + } > + timeout++; usleep_range() ? > + } > + resp = resp_high; > + resp = (resp << 32) | resp_lower; > + return resp; > +} > + > +static enum sdw_command_response > +amd_program_scp_addr(struct amd_sdw_manager *amd_manager, 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_sdw_ctl_word_prep(&lword, &uword, AMD_SDW_CMD_WRITE, &scp_msg, 0); > + response_buf[0] = amd_sdw_send_cmd_get_resp(amd_manager, lword, uword); > + scp_msg.addr = SDW_SCP_ADDRPAGE2; > + scp_msg.buf = &msg->addr_page2; > + amd_sdw_ctl_word_prep(&lword, &uword, AMD_SDW_CMD_WRITE, &scp_msg, 0); > + response_buf[1] = amd_sdw_send_cmd_get_resp(amd_manager, lword, uword); > + > + /* check response the writes */ revisit comment, grammar does not add up - missing to/if/after? > + for (index = 0; index < 2; index++) { what is the 2 here? > + if (response_buf[index] == -ETIMEDOUT) { > + dev_err(amd_manager->dev, "Program SCP cmd timeout\n"); that's one log here, possibly 2 ... > + 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(amd_manager->dev, "Program SCP NACK received\n"); > + } > + } > + } > + > + if (timeout) { > + dev_err_ratelimited(amd_manager->dev, > + "SCP_addrpage command timeout for Slave %d\n", msg->dev_num); ... and one more here. Is this needed/useful? > + return SDW_CMD_TIMEOUT; > + } > + > + if (nack) { > + dev_err_ratelimited(amd_manager->dev, > + "SCP_addrpage NACKed for Slave %d\n", msg->dev_num); > + return SDW_CMD_FAIL; > + } > + > + if (no_ack) { > + dev_dbg_ratelimited(amd_manager->dev, > + "SCP_addrpage ignored for Slave %d\n", msg->dev_num); > + return SDW_CMD_IGNORED; > + } > + return SDW_CMD_OK; > +} > +static int amd_sdw_manager_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_sdw_manager *amd_manager; > + int ret; > + > + if (!pdev->dev.platform_data) { > + dev_err(dev, "platform_data not retrieved\n"); can this happen? > + return -ENODEV; > + } > + amd_manager = devm_kzalloc(dev, sizeof(struct amd_sdw_manager), GFP_KERNEL); > + if (!amd_manager) > + return -ENOMEM; > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) > + return -ENOMEM; > + amd_manager->acp_mmio = devm_ioremap(dev, res->start, resource_size(res)); > + if (IS_ERR(amd_manager->mmio)) { > + dev_err(dev, "mmio not found\n"); > + return PTR_ERR(amd_manager->mmio); > + } > + amd_manager->instance = pdata->instance; > + amd_manager->mmio = amd_manager->acp_mmio + > + (amd_manager->instance * SDW_MANAGER_REG_OFFSET); > + amd_manager->acp_sdw_lock = pdata->acp_sdw_lock; > + amd_manager->cols_index = sdw_find_col_index(AMD_SDW_DEFAULT_COLUMNS); > + amd_manager->rows_index = sdw_find_row_index(AMD_SDW_DEFAULT_ROWS); > + amd_manager->dev = dev; > + amd_manager->bus.ops = &amd_sdw_ops; > + amd_manager->bus.port_ops = &amd_sdw_port_ops; > + amd_manager->bus.compute_params = &amd_sdw_compute_params; > + amd_manager->bus.clk_stop_timeout = 200; > + amd_manager->bus.link_id = amd_manager->instance; > + switch (amd_manager->instance) { > + case ACP_SDW0: > + amd_manager->num_dout_ports = AMD_SDW0_MAX_TX_PORTS; > + amd_manager->num_din_ports = AMD_SDW0_MAX_RX_PORTS; > + break; > + case ACP_SDW1: > + amd_manager->num_dout_ports = AMD_SDW1_MAX_TX_PORTS; > + amd_manager->num_din_ports = AMD_SDW1_MAX_RX_PORTS; > + break; > + default: > + return -EINVAL; > + } > + amd_manager->reg_mask = &sdw_manager_reg_mask_array[amd_manager->instance]; > + params = &amd_manager->bus.params; > + params->max_dr_freq = AMD_SDW_DEFAULT_CLK_FREQ * 2; > + params->curr_dr_freq = AMD_SDW_DEFAULT_CLK_FREQ * 2; > + params->col = AMD_SDW_DEFAULT_COLUMNS; > + params->row = AMD_SDW_DEFAULT_ROWS; > + prop = &amd_manager->bus.prop; > + prop->clk_freq = &amd_sdw_freq_tbl[0]; > + prop->mclk_freq = AMD_SDW_BUS_BASE_FREQ; > + > + ret = sdw_bus_master_add(&amd_manager->bus, dev, dev->fwnode); > + if (ret) { > + dev_err(dev, "Failed to register SoundWire manager(%d)\n", ret); > + return ret; > + } > + dev_set_drvdata(dev, amd_manager); > + INIT_WORK(&amd_manager->probe_work, amd_sdw_probe_work); > + /* > + * Instead of having lengthy probe sequence, spilt probe in two and typo: split. > + * use work queue for SoundWire manager startup sequence. The wording 'startup' is confusing in that you do not have a startup sequence as for Intel. It's just deferred probe to avoid taking too much time. > + */ > + schedule_work(&amd_manager->probe_work); > + return 0; > +}
On 2/20/23 05:04, Vijendar Mukunda wrote: > Enable build for SoundWire manager driver for AMD platforms. > > Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> +static void amd_sdw_read_and_process_ping_status(struct amd_sdw_manager *amd_manager) > +{ > + u64 response; > + > + mutex_lock(&amd_manager->bus.msg_lock); > + response = amd_sdw_send_cmd_get_resp(amd_manager, 0, 0); > + mutex_unlock(&amd_manager->bus.msg_lock); > + amd_sdw_process_ping_status(response, amd_manager); do you have a case where a new command could be sent after the mutex_unlock(), which could change the response fields? In other words, should the last amd_sdw_process_ping_status() function be protected as well? > +}
On 2/20/23 05:04, Vijendar Mukunda wrote: > Add wake enable interrupt support for both the SoundWire manager > instances. > > Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com> > Signed-off-by: Mastan Katragadda <Mastan.Katragadda@amd.com> > --- > drivers/soundwire/amd_manager.c | 10 ++++++++++ > drivers/soundwire/amd_manager.h | 1 + > 2 files changed, 11 insertions(+) > > diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c > index 3322adeca0d8..a7182aa78652 100644 > --- a/drivers/soundwire/amd_manager.c > +++ b/drivers/soundwire/amd_manager.c > @@ -932,6 +932,13 @@ static void amd_sdw_update_slave_status(u32 status_change_0to7, u32 status_chang > } > } > > +static void amd_sdw_process_wake_event(struct amd_sdw_manager *amd_manager) > +{ > + pm_request_resume(amd_manager->dev); is this needed? In the Intel case, the wakes do not necessarily come as in-band wakes, but they can also be notified by the PCI subsystem, so we do have to use pm_request_resume. In the AMD case, what happens if you don't do this? Doesn't the interrupt trigger a pm_runtime_resume already? > + acp_reg_writel(0x00, amd_manager->acp_mmio + ACP_SW_WAKE_EN(amd_manager->instance)); > + acp_reg_writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_8TO11); > +} \
On 21/02/23 21:43, Pierre-Louis Bossart wrote: > > On 2/20/23 05:04, Vijendar Mukunda wrote: >> Add wake enable interrupt support for both the SoundWire manager >> instances. >> >> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com> >> Signed-off-by: Mastan Katragadda <Mastan.Katragadda@amd.com> >> --- >> drivers/soundwire/amd_manager.c | 10 ++++++++++ >> drivers/soundwire/amd_manager.h | 1 + >> 2 files changed, 11 insertions(+) >> >> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c >> index 3322adeca0d8..a7182aa78652 100644 >> --- a/drivers/soundwire/amd_manager.c >> +++ b/drivers/soundwire/amd_manager.c >> @@ -932,6 +932,13 @@ static void amd_sdw_update_slave_status(u32 status_change_0to7, u32 status_chang >> } >> } >> >> +static void amd_sdw_process_wake_event(struct amd_sdw_manager *amd_manager) >> +{ >> + pm_request_resume(amd_manager->dev); > is this needed? > > In the Intel case, the wakes do not necessarily come as in-band wakes, > but they can also be notified by the PCI subsystem, so we do have to use > pm_request_resume. > > In the AMD case, what happens if you don't do this? Doesn't the > interrupt trigger a pm_runtime_resume already? ACP PCI driver receives soundwire interrupt and soundwire manager interrupt work queue will be scheduled. In this work queue, wake interrupt status bit is checked. As still soundwire manager in D3 state, it required to invoke pm_request_resume(). Without pm_request_resume() call, pm_runtime_resume won't be triggered in this scenario. > >> + acp_reg_writel(0x00, amd_manager->acp_mmio + ACP_SW_WAKE_EN(amd_manager->instance)); >> + acp_reg_writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_8TO11); >> +} > \
On 21/02/23 21:27, Pierre-Louis Bossart wrote: >> +static int amd_init_sdw_manager(struct amd_sdw_manager *amd_manager) >> +{ >> + u32 val; >> + u32 timeout = 0; >> + u32 retry_count = 0; >> + >> + acp_reg_writel(AMD_SDW_ENABLE, amd_manager->mmio + ACP_SW_EN); >> + do { >> + val = acp_reg_readl(amd_manager->mmio + ACP_SW_EN_STATUS); >> + if (val) >> + break; >> + usleep_range(10, 50); > that's a 5x range used for all usleep_range() in this file, is this > intentional? > usleep_range(10, 20) will be enough. Will fix it. >> + } while (retry_count++ < AMD_SDW_STAT_MAX_RETRY_COUNT); >> + >> + if (retry_count > AMD_SDW_STAT_MAX_RETRY_COUNT) >> + return -ETIMEDOUT; >> + >> + /* SoundWire manager bus reset */ >> + acp_reg_writel(AMD_SDW_BUS_RESET_REQ, amd_manager->mmio + ACP_SW_BUS_RESET_CTRL); >> + val = acp_reg_readl(amd_manager->mmio + ACP_SW_BUS_RESET_CTRL); >> + while (!(val & AMD_SDW_BUS_RESET_DONE)) { >> + val = acp_reg_readl(amd_manager->mmio + ACP_SW_BUS_RESET_CTRL); >> + if (timeout > AMD_DELAY_LOOP_ITERATION) >> + break; > so if you break here... > >> + usleep_range(1, 5); >> + timeout++; >> + } >> + if (timeout == AMD_DELAY_LOOP_ITERATION) >> + return -ETIMEDOUT; > ... this test will never be used. the timeout management looks wrong? Yes it overlooked. Will fix it. >> + timeout = 0; >> + acp_reg_writel(AMD_SDW_BUS_RESET_CLEAR_REQ, amd_manager->mmio + ACP_SW_BUS_RESET_CTRL); >> + val = acp_reg_readl(amd_manager->mmio + ACP_SW_BUS_RESET_CTRL); >> + while (val) { >> + val = acp_reg_readl(amd_manager->mmio + ACP_SW_BUS_RESET_CTRL); >> + if (timeout > AMD_DELAY_LOOP_ITERATION) >> + break; > same here... >> + usleep_range(1, 5); >> + timeout++; >> + } >> + if (timeout == AMD_DELAY_LOOP_ITERATION) { > ... and here. will fix it. >> + dev_err(amd_manager->dev, "Failed to reset SoundWire manager instance%d\n", >> + amd_manager->instance); >> + return -ETIMEDOUT; >> + } >> + retry_count = 0; >> + acp_reg_writel(AMD_SDW_DISABLE, amd_manager->mmio + ACP_SW_EN); >> + do { >> + val = acp_reg_readl(amd_manager->mmio + ACP_SW_EN_STATUS); >> + if (!val) >> + break; >> + usleep_range(10, 50); >> + } while (retry_count++ < AMD_SDW_STAT_MAX_RETRY_COUNT); >> + >> + if (retry_count > AMD_SDW_STAT_MAX_RETRY_COUNT) >> + return -ETIMEDOUT; > that one looks correct though. > >> + return 0; >> +} >> +static void amd_enable_sdw_interrupts(struct amd_sdw_manager *amd_manager) >> +{ >> + struct sdw_manager_reg_mask *reg_mask = amd_manager->reg_mask; >> + u32 val; >> + >> + mutex_lock(amd_manager->acp_sdw_lock); >> + val = acp_reg_readl(amd_manager->acp_mmio + ACP_EXTERNAL_INTR_CNTL(amd_manager->instance)); >> + val |= reg_mask->acp_sdw_intr_mask; >> + acp_reg_writel(val, amd_manager->acp_mmio + ACP_EXTERNAL_INTR_CNTL(amd_manager->instance)); > here you handle a read-modify-write sequence on the INTL_CNTL register... > >> + val = acp_reg_readl(amd_manager->acp_mmio + ACP_EXTERNAL_INTR_CNTL(amd_manager->instance)); > ... but here you only read from the same register. what's the purpose of > this read? This read to print the register value. This register read is really not required. we can drop dev_dbg() statement. >> + mutex_unlock(amd_manager->acp_sdw_lock); >> + dev_dbg(amd_manager->dev, "acp_ext_intr_ctrl[0x%x]:0x%x\n", >> + ACP_EXTERNAL_INTR_CNTL(amd_manager->instance), val); >> + val = acp_reg_readl(amd_manager->acp_mmio + ACP_EXTERNAL_INTR_STAT(amd_manager->instance)); >> + if (val) >> + acp_reg_writel(val, amd_manager->acp_mmio + >> + ACP_EXTERNAL_INTR_STAT(amd_manager->instance)); we will also remove ACP_EXTERNAL_INTR_STAT register update code. This change has side effects. ACP_EXTERNAL_INTR_STAT register should be updated in ACP PCI driver(parent driver) IRQ handler. >> + >> + acp_reg_writel(AMD_SDW_IRQ_MASK_0TO7, amd_manager->mmio + >> + ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7); >> + acp_reg_writel(AMD_SDW_IRQ_MASK_8TO11, amd_manager->mmio + >> + ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11); >> + acp_reg_writel(AMD_SDW_IRQ_ERROR_MASK, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK); > can you explain why the writes are not protected by the mutex? ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7, ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11, ACP_SW_ERROR_INTR_MASK registers are soundwire manager instance specific registers whereas ACP_EXTERNAL_INTR_CNTL register is part of ACP common space registers will be accessed by ACP PCI driver and other DMA driver modules, which needs to be protected by mutex lock. >> +} >> + >> +static void amd_disable_sdw_interrupts(struct amd_sdw_manager *amd_manager) >> +{ >> + struct sdw_manager_reg_mask *reg_mask = amd_manager->reg_mask; >> + u32 val; >> + >> + mutex_lock(amd_manager->acp_sdw_lock); >> + val = acp_reg_readl(amd_manager->acp_mmio + ACP_EXTERNAL_INTR_CNTL(amd_manager->instance)); >> + val &= ~reg_mask->acp_sdw_intr_mask; >> + acp_reg_writel(val, amd_manager->acp_mmio + ACP_EXTERNAL_INTR_CNTL(amd_manager->instance)); > you don't have the read here as in the enable sequence to presumably > that wasn't needed? please refer above reply. >> + mutex_unlock(amd_manager->acp_sdw_lock); >> + >> + acp_reg_writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7); >> + acp_reg_writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11); >> + acp_reg_writel(0x00, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK); > same, not clear why the writes are not protected? please refer above reply. >> +} > ... > >> +static u64 amd_sdw_send_cmd_get_resp(struct amd_sdw_manager *amd_manager, u32 lword, u32 uword) >> +{ >> + u64 resp; >> + u32 resp_lower, resp_high; >> + u32 sts; >> + u32 timeout = 0; >> + >> + sts = acp_reg_readl(amd_manager->mmio + ACP_SW_IMM_CMD_STS); >> + while (sts & AMD_SDW_IMM_CMD_BUSY) { >> + sts = acp_reg_readl(amd_manager->mmio + ACP_SW_IMM_CMD_STS); >> + if (timeout > AMD_SDW_RETRY_COUNT) { >> + dev_err(amd_manager->dev, "SDW%x previous cmd status clear failed\n", >> + amd_manager->instance); >> + return -ETIMEDOUT; >> + } >> + timeout++; > no usleep_range() here? will fix it by using usleep_range(5, 10). > Also is there any merit in using the same retry count across the board, > this is about command handling, not enabling the hardware - presumably > this should be tied to the SoundWire bus frame timing, not internal delays. > >> + } >> + >> + timeout = 0; >> + if (sts & AMD_SDW_IMM_RES_VALID) { >> + dev_err(amd_manager->dev, "SDW%x manager is in bad state\n", amd_manager->instance); >> + acp_reg_writel(0x00, amd_manager->mmio + ACP_SW_IMM_CMD_STS); >> + } >> + acp_reg_writel(uword, amd_manager->mmio + ACP_SW_IMM_CMD_UPPER_WORD); >> + acp_reg_writel(lword, amd_manager->mmio + ACP_SW_IMM_CMD_LOWER_QWORD); >> + >> + sts = acp_reg_readl(amd_manager->mmio + ACP_SW_IMM_CMD_STS); >> + while (!(sts & AMD_SDW_IMM_RES_VALID)) { >> + sts = acp_reg_readl(amd_manager->mmio + ACP_SW_IMM_CMD_STS); >> + if (timeout > AMD_SDW_RETRY_COUNT) { >> + dev_err(amd_manager->dev, "SDW%x cmd response timeout occurred\n", >> + amd_manager->instance); >> + return -ETIMEDOUT; > usleep_range? will fix it by using usleep_range(5, 10). >> + } >> + timeout++; >> + } >> + resp_high = acp_reg_readl(amd_manager->mmio + ACP_SW_IMM_RESP_UPPER_WORD); >> + resp_lower = acp_reg_readl(amd_manager->mmio + ACP_SW_IMM_RESP_LOWER_QWORD); >> + timeout = 0; >> + acp_reg_writel(AMD_SDW_IMM_RES_VALID, amd_manager->mmio + ACP_SW_IMM_CMD_STS); >> + while ((sts & AMD_SDW_IMM_RES_VALID)) { >> + sts = acp_reg_readl(amd_manager->mmio + ACP_SW_IMM_CMD_STS); >> + if (timeout > AMD_SDW_RETRY_COUNT) { >> + dev_err(amd_manager->dev, "SDW%x cmd status retry failed\n", >> + amd_manager->instance); >> + return -ETIMEDOUT; >> + } >> + timeout++; > usleep_range() ? will fix it by using usleep_range(5, 10). >> + } >> + resp = resp_high; >> + resp = (resp << 32) | resp_lower; >> + return resp; >> +} >> + >> +static enum sdw_command_response >> +amd_program_scp_addr(struct amd_sdw_manager *amd_manager, 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_sdw_ctl_word_prep(&lword, &uword, AMD_SDW_CMD_WRITE, &scp_msg, 0); >> + response_buf[0] = amd_sdw_send_cmd_get_resp(amd_manager, lword, uword); >> + scp_msg.addr = SDW_SCP_ADDRPAGE2; >> + scp_msg.buf = &msg->addr_page2; >> + amd_sdw_ctl_word_prep(&lword, &uword, AMD_SDW_CMD_WRITE, &scp_msg, 0); >> + response_buf[1] = amd_sdw_send_cmd_get_resp(amd_manager, lword, uword); >> + >> + /* check response the writes */ > revisit comment, grammar does not add up - missing to/if/after? will fix it. >> + for (index = 0; index < 2; index++) { > what is the 2 here? 2 denotes SCP commands response buffer count. >> + if (response_buf[index] == -ETIMEDOUT) { >> + dev_err(amd_manager->dev, "Program SCP cmd timeout\n"); > that's one log here, possibly 2 ... we are checking inside loop. >> + 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(amd_manager->dev, "Program SCP NACK received\n"); >> + } >> + } >> + } >> + >> + if (timeout) { >> + dev_err_ratelimited(amd_manager->dev, >> + "SCP_addrpage command timeout for Slave %d\n", msg->dev_num); > ... and one more here. Is this needed/useful? Even if any one of the scp command response reports timeout , we need to return timeout error. >> + return SDW_CMD_TIMEOUT; >> + } >> + >> + if (nack) { >> + dev_err_ratelimited(amd_manager->dev, >> + "SCP_addrpage NACKed for Slave %d\n", msg->dev_num); >> + return SDW_CMD_FAIL; >> + } >> + >> + if (no_ack) { >> + dev_dbg_ratelimited(amd_manager->dev, >> + "SCP_addrpage ignored for Slave %d\n", msg->dev_num); >> + return SDW_CMD_IGNORED; >> + } >> + return SDW_CMD_OK; >> +} >> +static int amd_sdw_manager_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_sdw_manager *amd_manager; >> + int ret; >> + >> + if (!pdev->dev.platform_data) { >> + dev_err(dev, "platform_data not retrieved\n"); > can this happen? not needed. we can drop this check. >> + return -ENODEV; >> + } >> + amd_manager = devm_kzalloc(dev, sizeof(struct amd_sdw_manager), GFP_KERNEL); >> + if (!amd_manager) >> + return -ENOMEM; >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (!res) >> + return -ENOMEM; >> + amd_manager->acp_mmio = devm_ioremap(dev, res->start, resource_size(res)); >> + if (IS_ERR(amd_manager->mmio)) { >> + dev_err(dev, "mmio not found\n"); >> + return PTR_ERR(amd_manager->mmio); >> + } >> + amd_manager->instance = pdata->instance; >> + amd_manager->mmio = amd_manager->acp_mmio + >> + (amd_manager->instance * SDW_MANAGER_REG_OFFSET); >> + amd_manager->acp_sdw_lock = pdata->acp_sdw_lock; >> + amd_manager->cols_index = sdw_find_col_index(AMD_SDW_DEFAULT_COLUMNS); >> + amd_manager->rows_index = sdw_find_row_index(AMD_SDW_DEFAULT_ROWS); >> + amd_manager->dev = dev; >> + amd_manager->bus.ops = &amd_sdw_ops; >> + amd_manager->bus.port_ops = &amd_sdw_port_ops; >> + amd_manager->bus.compute_params = &amd_sdw_compute_params; >> + amd_manager->bus.clk_stop_timeout = 200; >> + amd_manager->bus.link_id = amd_manager->instance; >> + switch (amd_manager->instance) { >> + case ACP_SDW0: >> + amd_manager->num_dout_ports = AMD_SDW0_MAX_TX_PORTS; >> + amd_manager->num_din_ports = AMD_SDW0_MAX_RX_PORTS; >> + break; >> + case ACP_SDW1: >> + amd_manager->num_dout_ports = AMD_SDW1_MAX_TX_PORTS; >> + amd_manager->num_din_ports = AMD_SDW1_MAX_RX_PORTS; >> + break; >> + default: >> + return -EINVAL; >> + } >> + amd_manager->reg_mask = &sdw_manager_reg_mask_array[amd_manager->instance]; >> + params = &amd_manager->bus.params; >> + params->max_dr_freq = AMD_SDW_DEFAULT_CLK_FREQ * 2; >> + params->curr_dr_freq = AMD_SDW_DEFAULT_CLK_FREQ * 2; >> + params->col = AMD_SDW_DEFAULT_COLUMNS; >> + params->row = AMD_SDW_DEFAULT_ROWS; >> + prop = &amd_manager->bus.prop; >> + prop->clk_freq = &amd_sdw_freq_tbl[0]; >> + prop->mclk_freq = AMD_SDW_BUS_BASE_FREQ; >> + >> + ret = sdw_bus_master_add(&amd_manager->bus, dev, dev->fwnode); >> + if (ret) { >> + dev_err(dev, "Failed to register SoundWire manager(%d)\n", ret); >> + return ret; >> + } >> + dev_set_drvdata(dev, amd_manager); >> + INIT_WORK(&amd_manager->probe_work, amd_sdw_probe_work); >> + /* >> + * Instead of having lengthy probe sequence, spilt probe in two and > typo: split. > >> + * use work queue for SoundWire manager startup sequence. > The wording 'startup' is confusing in that you do not have a startup > sequence as for Intel. It's just deferred probe to avoid taking too much > time. will modify the comment. >> + */ >> + schedule_work(&amd_manager->probe_work); >> + return 0; >> +}
On 21/02/23 21:35, Pierre-Louis Bossart wrote: >> +static void amd_sdw_read_and_process_ping_status(struct amd_sdw_manager *amd_manager) >> +{ >> + u64 response; >> + >> + mutex_lock(&amd_manager->bus.msg_lock); >> + response = amd_sdw_send_cmd_get_resp(amd_manager, 0, 0); >> + mutex_unlock(&amd_manager->bus.msg_lock); >> + amd_sdw_process_ping_status(response, amd_manager); > do you have a case where a new command could be sent after the > mutex_unlock(), which could change the response fields? No. There won't be a new command which will change response fields. We are using lock to prevent peripheral registers read/writes during sending ping command. This implementation is used to send ping command and read and process peripheral status when PREQ is asserted and this function is also invoked after peripheral enumeration sequence is completed to collect updated peripheral status. > In other words, should the last amd_sdw_process_ping_status() function > be protected as well? IMO, it's not needed. >> +}
ACP IP(v6.x) block has two SoundWire manager instance support. This patchset adds support for AMD SoundWire manager driver. Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com> Changes since v2: - Remove useless variable initializations. - Add helper function to interpret peripheral status. - Move runtime pm sequence to probe_work workqueue. - Use string "SoundWire" instead of "soundwire" in code. - Update comments in interrupt handler and probe sequence. - Rename "sdw_lock" as "acp_sdw_lock". - Remove __func__ from dev_dbg statements. Changes since v1: - Drop asoc tree based patches. will send asoc patches as a separate series. - Fixed double space errors. - Use dev instead of pci->dev. - Use SoundWire manager terminology. - Remove amd_sdw_compute_slave_ports() function and use exported sdw_compute_slave_ports() function. - Remove unused variable "num_ports" from amd_manager structure. - Drop startup and shutdown dai callbacks. - Drop reset_page_addr callback. - Use relative address offset to program SoundWire manager registers throughout the code. - Separate wake enable interrupt handling from slave status handling logic. - Use acp_mmio to program ACP common registers. - Use dai_runtime_array implementation in dai_ops. - Refactor port_ops callbacks. - Add comments in port_ops callbacks. - Add retry count logic in irq thread to address faulty case. - Add helper function to interpret command response. - Add generic bandwidth allocation dependency in Kconfig options. - Add comments for AMD SoundWire power modes. - Add missing timeout check in amd_init_sdw_manager callback. - Declare frameshape parameters in probe call. - Handle error case in clock stop sequence. - Add comments in pm_prepare and pm_ops callbacks. Vijendar Mukunda (8): soundwire: export sdw_compute_slave_ports() function soundwire: amd: Add support for AMD Manager driver soundwire: amd: register SoundWire manager dai ops soundwire: amd: enable build for AMD SoundWire manager driver soundwire: amd: add SoundWire manager interrupt handling soundwire: amd: add runtime pm ops for AMD SoundWire manager driver soundwire: amd: handle SoundWire wake enable interrupt soundwire: amd: add pm_prepare callback and pm ops support drivers/soundwire/Kconfig | 10 + drivers/soundwire/Makefile | 4 + drivers/soundwire/amd_manager.c | 1335 +++++++++++++++++ drivers/soundwire/amd_manager.h | 275 ++++ drivers/soundwire/bus.h | 9 + .../soundwire/generic_bandwidth_allocation.c | 12 +- include/linux/soundwire/sdw_amd.h | 108 ++ 7 files changed, 1744 insertions(+), 9 deletions(-) create mode 100644 drivers/soundwire/amd_manager.c create mode 100644 drivers/soundwire/amd_manager.h create mode 100644 include/linux/soundwire/sdw_amd.h