Re: [alsa-devel] [RFC PATCH 6/6] soundwire: qcom: add support for SoundWire controller

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux