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
