Messages in this thread | | | Subject | Re: [alsa-devel] [RFC PATCH 6/6] soundwire: qcom: add support for SoundWire controller | From | Srinivas Kandagatla <> | Date | Sun, 9 Jun 2019 13:15:55 +0100 |
| |
Thanks for taking time to review,
On 07/06/2019 14:36, Pierre-Louis Bossart wrote: > >> +config SOUNDWIRE_QCOM >> + tristate "Qualcomm SoundWire Master driver" >> + select SOUNDWIRE_BUS >> + depends on SND_SOC > > depends on SLIMBUS if you need the SlimBus link to talk to your > SoundWire Master? > > Also depends on device tree since you use of_ functions? > Thats true, will fix this in next version. >> +#define SWRM_COMP_HW_VERSION 0x00 > > Can we please use SDW_ or QCOM_SDW_ as prefix? >
SWRM prefix is as per the data sheet register names, If it help am happy to add QCOM_ prefix it.
>> +#define SWRM_INTERRUPT_STATUS_NEW_SLAVE_AUTO_ENUM_FINISHED BIT(11) >> +#define SWRM_INTERRUPT_STATUS_AUTO_ENUM_FAILED BIT(12) >> +#define SWRM_INTERRUPT_STATUS_AUTO_ENUM_TABLE_IS_FULL BIT(13) > > This hints at hardware support to assign Device Numbers automagically so > will likely have impacts on the bus driver code, no?
yes, this controller has autoenumeration support, however I had disabled this code due to the fact that we need some more support in bus driver to accommodate this feature.
I will explore this side once again, may be before sending next version.
> > >> +#define SWRM_MAX_ROW_VAL 0 /* Rows = 48 */ >> +#define SWRM_DEFAULT_ROWS 48 >> +#define SWRM_MIN_COL_VAL 0 /* Cols = 2 */ >> +#define SWRM_DEFAULT_COL 16 >> +#define SWRM_SPECIAL_CMD_ID 0xF >> +#define MAX_FREQ_NUM 1 >> +#define TIMEOUT_MS 1000 >> +#define QCOM_SWRM_MAX_RD_LEN 0xf >> +#define DEFAULT_CLK_FREQ 9600000 > > The clocks and frame shape don't match usual expectations for PDM. > For a 9.6 MHz support, you would typically use 8 columns and 50 rows to > transport PDM with a 50x oversampling. I've never seen anyone use 48x > for PDM. > >> +#define SWRM_MAX_DAIS 0xF >> + >> +struct qcom_swrm_port_config { >> + u8 si; >> + u8 off1; >> + u8 off2; >> +}; >> + >> +struct qcom_swrm_ctrl { >> + struct sdw_bus bus; >> + struct device *dev; >> + struct regmap *regmap; >> + struct completion sp_cmd_comp; >> + struct work_struct slave_work; >> + /* read/write lock */ >> + struct mutex lock; >> + /* Port alloc/free lock */ >> + struct mutex port_lock; >> + struct clk *hclk; >> + int fifo_status; >> + void __iomem *base; >> + u8 wr_cmd_id; >> + u8 rd_cmd_id; >> + int irq; >> + unsigned int version; >> + int num_din_ports; >> + int num_dout_ports; >> + unsigned long dout_port_mask; >> + unsigned long din_port_mask; >> + struct qcom_swrm_port_config pconfig[SDW_MAX_PORTS]; > > this is not necessarily correct. the initial definitions for > SDW_MAX_PORTS was for Slave devices. There is no definitions for Masters > in the SoundWire spec, so you could use whatever constant you want for > your hardware.
Yes, I can move this define to this driver specific.
> >> + struct sdw_stream_runtime *sruntime[SWRM_MAX_DAIS]; >> + enum sdw_slave_status status[SDW_MAX_DEVICES]; >> + u32 (*reg_read)(struct qcom_swrm_ctrl *ctrl, int reg); >> + int (*reg_write)(struct qcom_swrm_ctrl *ctrl, int reg, int val); >> +}; >> + >> +#define to_qcom_sdw(b) container_of(b, struct qcom_swrm_ctrl, bus) >> + >> +struct usecase { >> + u8 num_port; >> + u8 num_ch; >> + u32 chrate; >> +}; > > this structure doesn't seem to be used? > looks like left over, I will remove this.
>> +static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *ctrl, u8 >> cmd_data, >> + u8 dev_addr, u16 reg_addr) >> +{ >> + int ret = 0; >> + u8 cmd_id; >> + u32 val; >> + >> + mutex_lock(&ctrl->lock); >> + if (dev_addr == SDW_BROADCAST_DEV_NUM) { >> + cmd_id = SWRM_SPECIAL_CMD_ID; >> + } else { >> + if (++ctrl->wr_cmd_id == SWRM_SPECIAL_CMD_ID) >> + ctrl->wr_cmd_id = 0; >> + >> + cmd_id = ctrl->wr_cmd_id; >> + } > > might be worth having a helper/macro since you are doing the same thing > below. >
I will do that!
>> + >> + val = SWRM_REG_VAL_PACK(cmd_data, dev_addr, cmd_id, reg_addr); >> + ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_WR_CMD, val); >> + if (ret < 0) { >> + dev_err(ctrl->dev, "%s: reg 0x%x write failed, err:%d\n", >> + __func__, val, ret); >> + goto err; >> + } >> + >> + if (dev_addr == SDW_BROADCAST_DEV_NUM) { >> + ctrl->fifo_status = 0; >> + ret = wait_for_completion_timeout(&ctrl->sp_cmd_comp, >> + msecs_to_jiffies(TIMEOUT_MS)); > > This is odd. The SoundWire spec does not handle writes to a single > device or broadcast writes differently. I don't see a clear reason why > you would only timeout for a broadcast write. >
There is danger of blocking here without timeout.
>> + >> + if (!ret || ctrl->fifo_status) { >> + dev_err(ctrl->dev, "reg 0x%x write failed\n", val); >> + ret = -ENODATA; >> + goto err; >> + } >> + } >> +err: >> + mutex_unlock(&ctrl->lock); >> + return ret; >> +} >> + >> +static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *ctrl, >> + u8 dev_addr, u16 reg_addr, >> + u32 len, u8 *rval) >> +{ >> + int i, ret = 0; >> + u8 cmd_id = 0; >> + u32 val; >> + >> + mutex_lock(&ctrl->lock); >> + if (dev_addr == SDW_ENUM_DEV_NUM) { >> + cmd_id = SWRM_SPECIAL_CMD_ID; >> + } else { >> + if (++ctrl->rd_cmd_id == SWRM_SPECIAL_CMD_ID) >> + ctrl->rd_cmd_id = 0; >> + >> + cmd_id = ctrl->rd_cmd_id; >> + } > > helper? yes will add that.
> >> + val = SWRM_REG_VAL_PACK(len, dev_addr, cmd_id, reg_addr); >> + ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_RD_CMD, val); >> + if (ret < 0) { >> + dev_err(ctrl->dev, "reg 0x%x write failed err:%d\n", val, ret); >> + goto err; >> + } >> + >> + if (dev_addr == SDW_ENUM_DEV_NUM) { >> + ctrl->fifo_status = 0; >> + ret = wait_for_completion_timeout(&ctrl->sp_cmd_comp, >> + msecs_to_jiffies(TIMEOUT_MS)); >> > same comment here, there isn't a clear reason to only timeout for a read > from device0. > >> + if (!ret || ctrl->fifo_status) { >> + dev_err(ctrl->dev, "reg 0x%x read failed\n", val); >> + ret = -ENODATA; >> + goto err; >> + } >> + } >> + >> + for (i = 0; i < len; i++) { >> + rval[i] = ctrl->reg_read(ctrl, SWRM_CMD_FIFO_RD_FIFO_ADDR); >> + rval[i] &= 0xFF; >> + } >> + >> +err: >> + mutex_unlock(&ctrl->lock); >> + return ret; >> +} >> + >> +static void qcom_swrm_get_device_status(struct qcom_swrm_ctrl *ctrl) >> +{ >> + u32 val = ctrl->reg_read(ctrl, SWRM_MCP_SLV_STATUS); >> + int i; >> + >> + for (i = 1; i < SDW_MAX_DEVICES; i++) { >> + u32 s; >> + >> + s = (val >> (i * 2)); >> + s &= SWRM_MCP_SLV_STATUS_MASK; >> + ctrl->status[i] = s; >> + } >> +} >> + >> +static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id) >> +{ >> + struct qcom_swrm_ctrl *ctrl = dev_id; >> + u32 sts, value; >> + >> + sts = ctrl->reg_read(ctrl, SWRM_INTERRUPT_STATUS); >> + >> + if (sts & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED) >> + complete(&ctrl->sp_cmd_comp); >> + >> + if (sts & SWRM_INTERRUPT_STATUS_CMD_ERROR) { >> + value = ctrl->reg_read(ctrl, SWRM_CMD_FIFO_STATUS); >> + dev_err_ratelimited(ctrl->dev, >> + "CMD error, fifo status 0x%x\n", >> + value); >> + ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CMD, 0x1); >> + if ((value & 0xF) == 0xF) { >> + ctrl->fifo_status = -ENODATA; >> + complete(&ctrl->sp_cmd_comp); >> + } >> + } >> + >> + if ((sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED) || >> + sts & SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS) { >> + if (sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED) >> + ctrl->status[0] = SDW_SLAVE_ATTACHED; >> + >> + schedule_work(&ctrl->slave_work); >> + } >> + >> + if (sts & SWRM_INTERRUPT_STATUS_SLAVE_PEND_IRQ) >> + dev_dbg(ctrl->dev, "Slave pend irq\n"); >> + >> + if (sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED) >> + dev_dbg(ctrl->dev, "New slave attached\n"); >> + >> + if (sts & SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET) >> + dev_err_ratelimited(ctrl->dev, "Bus clash detected\n"); >> + >> + if (sts & SWRM_INTERRUPT_STATUS_RD_FIFO_OVERFLOW) >> + dev_err(ctrl->dev, "Read FIFO overflow\n"); >> + >> + if (sts & SWRM_INTERRUPT_STATUS_RD_FIFO_UNDERFLOW) >> + dev_err(ctrl->dev, "Read FIFO underflow\n"); >> + >> + if (sts & SWRM_INTERRUPT_STATUS_WR_CMD_FIFO_OVERFLOW) >> + dev_err(ctrl->dev, "Write FIFO overflow\n"); >> + >> + if (sts & SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION) >> + dev_err(ctrl->dev, "Port collision detected\n"); >> + >> + if (sts & SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH) >> + dev_err(ctrl->dev, "Read enable valid mismatch\n"); >> + >> + if (sts & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED) >> + dev_err(ctrl->dev, "Cmd id finished\n"); >> + >> + if (sts & SWRM_INTERRUPT_STATUS_BUS_RESET_FINISHED) >> + dev_err(ctrl->dev, "Bus reset finished\n"); > > This list is odd as well. It makes sense to only log error cases if you > don't really know how to handle them, but a 'NEW SLAVE ATTACHED' should > lead to an action, no?
NEW SLAVE ATTACHED interrupt already schedules action by reporting this to soudwire bus layer. Some of them are leftover as part of debug, i will try to clean them up and see if some of them can be handled properly. > >> + >> + ctrl->reg_write(ctrl, SWRM_INTERRUPT_CLEAR, sts); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl) >> +{ >> + u32 val; >> + u8 row_ctrl = SWRM_MAX_ROW_VAL; >> + u8 col_ctrl = SWRM_MIN_COL_VAL; >> + u8 ssp_period = 1; >> + u8 retry_cmd_num = 3; > > probably want a define for those magic values, they are quite important.
Yep, will do that!
> >> + >> + /* Clear Rows and Cols */ >> + val = ((row_ctrl << SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT) | >> + (col_ctrl << SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT) | >> + (ssp_period << SWRM_MCP_FRAME_CTRL_BANK_SSP_PERIOD_SHFT)); >> + >> + ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val); >> + >> + /* Disable Auto enumeration */ >> + ctrl->reg_write(ctrl, SWRM_ENUMERATOR_CFG_ADDR, 0); > > This goes back to my earlier comment. Do you disable this > auto-enumeration to avoid conflicts with the existing bus management? > That's not necessarily smart, we may want to change that bus layer to > reduce the enumeration time if hardware can do it.
I will explore add this support in bus befor next version.
> >> + >> + /* Mask soundwire interrupts */ >> + ctrl->reg_write(ctrl, SWRM_INTERRUPT_MASK_ADDR, >> + SWRM_INTERRUPT_STATUS_RMSK); >> + >> + /* Configure No pings */ >> + val = ctrl->reg_read(ctrl, SWRM_MCP_CFG_ADDR); > > If there is any sort of PREQ signaling for Slave-initiated interrupts, > disabling PINGs is likely a non-conformant implementation since the > master is required to issue a PING command within 32 frames. That's also > the only way to know if a device is attached, so additional comments are > likely required. This is the value of Maximum number of consiecutive read/write commands without ping command in between. I will try to collect more details and add some comments here.
> >> + >> + val &= ~SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK; >> + val |= (0x1f << SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_SHFT); >> + ctrl->reg_write(ctrl, SWRM_MCP_CFG_ADDR, val); >> + >> + /* Configure number of retries of a read/write cmd */ >> + val = (retry_cmd_num << SWRM_CMD_FIFO_CFG_NUM_OF_CMD_RETRY_SHFT); >> + ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CFG_ADDR, val); >> + >> + /* Set IRQ to PULSE */ >> + ctrl->reg_write(ctrl, SWRM_COMP_CFG_ADDR, >> + SWRM_COMP_CFG_IRQ_LEVEL_OR_PULSE_MSK | >> + SWRM_COMP_CFG_ENABLE_MSK); > > indentation seems off in this code? > Yes! I will fix this.
>> + return 0; >> +} >> + >> +static enum sdw_command_response qcom_swrm_xfer_msg(struct sdw_bus *bus, >> + struct sdw_msg *msg) >> +{ >> + struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus); >> + int ret, i, len; >> + >> + if (msg->flags == SDW_MSG_FLAG_READ) { >> + for (i = 0; i < msg->len;) { >> + if ((msg->len - i) < QCOM_SWRM_MAX_RD_LEN) >> + len = msg->len - i; >> + else >> + len = QCOM_SWRM_MAX_RD_LEN; >> + >> + ret = qcom_swrm_cmd_fifo_rd_cmd(ctrl, msg->dev_num, >> + msg->addr + i, len, >> + &msg->buf[i]); >> + if (ret < 0) { >> + if (ret == -ENODATA) >> + return SDW_CMD_IGNORED; >> + >> + return ret; >> + } >> + >> + i = i + len; >> + } >> + } else if (msg->flags == SDW_MSG_FLAG_WRITE) { >> + for (i = 0; i < msg->len; i++) { >> + ret = qcom_swrm_cmd_fifo_wr_cmd(ctrl, msg->buf[i], >> + msg->dev_num, >> + msg->addr + i); >> + if (ret < 0) { >> + if (ret == -ENODATA) >> + return SDW_CMD_IGNORED; >> + >> + return ret; >> + } >> + } >> + } >> + >> + return SDW_CMD_OK; >> +} >> + >> +static int qcom_swrm_pre_bank_switch(struct sdw_bus *bus) >> +{ >> + u32 reg = SWRM_MCP_FRAME_CTRL_BANK_ADDR(bus->params.next_bank); >> + struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus); >> + u32 val; >> + >> + val = ctrl->reg_read(ctrl, reg); >> + val |= ((0 << SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT) | >> + (7l << SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT)); > > magic values, probably need a macro here? > Okay, will add a proper define for this values.
>> + ctrl->reg_write(ctrl, reg, val); >> + >> + return 0; >> +} >> + > >> +static int qcom_swrm_register_dais(struct qcom_swrm_ctrl *ctrl) >> +{ >> + int num_dais = ctrl->num_dout_ports + ctrl->num_din_ports; >> + struct snd_soc_dai_driver *dais; >> + int i; >> + >> + /* PDM dais are only tested for now */ >> + dais = devm_kcalloc(ctrl->dev, num_dais, sizeof(*dais), GFP_KERNEL); >> + if (!dais) >> + return -ENOMEM; >> + >> + for (i = 0; i < num_dais; i++) { >> + dais[i].name = kasprintf(GFP_KERNEL, "SDW Pin%d", i); > > if (!dais[i].name) > >> + if (i < ctrl->num_dout_ports) { >> + dais[i].playback.stream_name = kasprintf(GFP_KERNEL, >> + "SDW Tx%d", i); >> + if (!dais[i].playback.stream_name) { >> + kfree(dais[i].name); >> + return -ENOMEM; >> + } > > also need to free previously allocated memory in earlier iterations, or > use devm_ > Yes, thats true, I will fix this in next version.
Thanks, srini
| |