Re: [alsa-devel] [PATCH v4 06/15] soundwire: Add IO transfer

On 12/1/17 3:56 AM, Vinod Koul wrote:

SoundWire bus supports read or write register(s) for SoundWire Slave device. sdw_read() and sdw_write() APIs are provided for single register read/write. sdw_nread() and sdw_nwrite() for operations on contiguous registers.

Signed-off-by: Sanyog Kale sanyog.r.kale@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com

---

drivers/soundwire/bus.c | 275 ++++++++++++++++++++++++++++++++++++++++++ drivers/soundwire/bus.h | 37 ++++++ include/linux/soundwire/sdw.h | 57 +++++++++ 3 files changed, 369 insertions(+)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 507ae85ad58e..2f108f162905 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -3,6 +3,8 @@

#include <linux/acpi.h> #include <linux/mod_devicetable.h> +#include <linux/pm_runtime.h> +#include <linux/soundwire/sdw_registers.h> #include <linux/soundwire/sdw.h> #include "bus.h"

@@ -22,6 +24,12 @@ int sdw_add_bus_master(struct sdw_bus *bus) return -ENODEV; }

• if (!bus->ops) {

• dev_err(bus->dev, "SoundWire Bus ops are not set");
  

• return -EINVAL;
  

• }
• mutex_init(&bus->msg_lock); mutex_init(&bus->bus_lock); INIT_LIST_HEAD(&bus->slaves);

@@ -97,6 +105,273 @@ void sdw_delete_bus_master(struct sdw_bus *bus) } EXPORT_SYMBOL(sdw_delete_bus_master);

+/*

• • SDW IO Calls
• */

+static inline int find_response_code(enum sdw_command_response resp) +{

• switch (resp) {
• case SDW_CMD_OK:

• return 0;
  

• case SDW_CMD_IGNORED:

• return -ENODATA;
  

• case SDW_CMD_TIMEOUT:

• return -ETIMEDOUT;
  

• default:

• return -EIO;
  

the 'default' case will handle both SDW_CMD_FAIL (which is a bus event usually due to bus clash or parity issues) and SDW_CMD_FAIL_OTHER (which is an imp-def IP event).

Do they really belong in the same basket? From a debug perspective there is quite a bit of information lost.

• }

+}

+static inline int do_transfer(struct sdw_bus *bus, struct sdw_msg *msg) +{

• int retry = bus->prop.err_threshold;
• enum sdw_command_response resp;
• int ret = 0, i;
• for (i = 0; i <= retry; i++) {

• resp = bus->ops->xfer_msg(bus, msg);
  

• ret = find_response_code(resp);
  

• /* if cmd is ok or ignored return */
  

• if (ret == 0 || ret == -ENODATA)
  

Can you document why you don't retry on a CMD_IGNORED? I know there was a reason, I just can't remember it.

Now that I think of it, the retry on TIMEOUT makes no sense to me. The retry was intended for bus-level issues, where maybe a single bit error causes an issue without consequences, but the TIMEOUT is a completely different beast, it's the master IP that doesn't answer really, a completely different case.

• 	return ret;
  

• }
• return ret;

+}

+static inline int do_transfer_defer(struct sdw_bus *bus,

• 	struct sdw_msg *msg, struct sdw_defer *defer)
  

+{

• int retry = bus->prop.err_threshold;
• enum sdw_command_response resp;
• int ret = 0, i;
• defer->msg = msg;
• defer->length = msg->len;
• for (i = 0; i <= retry; i++) {

• resp = bus->ops->xfer_msg_defer(bus, msg, defer);
  

• ret = find_response_code(resp);
  

• /* if cmd is ok or ignored return */
  

• if (ret == 0 || ret == -ENODATA)
  

• 	return ret;
  

• }
• return ret;

+}

+static int sdw_reset_page(struct sdw_bus *bus, u16 dev_num) +{

• int retry = bus->prop.err_threshold;
• enum sdw_command_response resp;
• int ret = 0, i;
• for (i = 0; i <= retry; i++) {

• resp = bus->ops->reset_page_addr(bus, dev_num);
  

• ret = find_response_code(resp);
  

• /* if cmd is ok or ignored return */
  

• if (ret == 0 || ret == -ENODATA)
  

• 	return ret;
  

• }
• return ret;

+}

+/**

• • sdw_transfer() - Synchronous transfer message to a SDW Slave device
• • @bus: SDW bus
• • @slave: SDW Slave

is this just me or this argument is not used?

• • @msg: SDW message to be xfered
• */

+int sdw_transfer(struct sdw_bus *bus,

• struct sdw_slave *slave, struct sdw_msg *msg)
  

+{

• int ret;
• mutex_lock(&bus->msg_lock);
• ret = do_transfer(bus, msg);
• if (ret != 0 && ret != -ENODATA)

• dev_err(bus->dev, "trf on Slave %d failed:%d\n",
  

• 		msg->dev_num, ret);
  

• if (msg->page)

• sdw_reset_page(bus, msg->dev_num);
  

• mutex_unlock(&bus->msg_lock);
• return ret;

+}

+/**

• • sdw_transfer_defer() - Asynchronously transfer message to a SDW Slave device
• • @bus: SDW bus
• • @slave: SDW Slave

same, this argument is not used in the code below.

• • @msg: SDW message to be xfered
• • @defer: Defer block for signal completion
• • Caller needs to hold the msg_lock lock while calling this
• */

+int sdw_transfer_defer(struct sdw_bus *bus, struct sdw_slave *slave,

• 	struct sdw_msg *msg, struct sdw_defer *defer)
  

+{

• int ret;
• if (!bus->ops->xfer_msg_defer)

• return -ENOTSUPP;
  

• ret = do_transfer_defer(bus, msg, defer);
• if (ret != 0 && ret != -ENODATA)

• dev_err(bus->dev, "Defer trf on Slave %d failed:%d\n",
  

• 		msg->dev_num, ret);
  

• if (msg->page)

• sdw_reset_page(bus, msg->dev_num);
  

• return ret;

+}

+int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,

• u32 addr, size_t count, u16 dev_num, u8 flags, u8 *buf)
  

+{

• memset(msg, 0, sizeof(*msg));
• msg->addr = addr;

add comment on implicit truncation to 16-bit address

• msg->len = count;
• msg->dev_num = dev_num;
• msg->flags = flags;
• msg->buf = buf;
• msg->ssp_sync = false;
• msg->page = false;
• if (addr < SDW_REG_NO_PAGE) { /* no paging area */

• return 0;
  

• } else if (addr >= SDW_REG_MAX) { /* illegal addr */

• pr_err("SDW: Invalid address %x passed\n", addr);
  

• return -EINVAL;
  

• }
• if (addr < SDW_REG_OPTIONAL_PAGE) { /* 32k but no page */

• if (slave && !slave->prop.paging_support)
  

• 	return 0;
  

• /* no need for else as that will fall thru to paging */
  

• }
• /* paging madatory */

mandatory

• if (dev_num == SDW_ENUM_DEV_NUM || dev_num == SDW_BROADCAST_DEV_NUM) {

• pr_err("SDW: Invalid device for paging :%d\n", dev_num);
  

• return -EINVAL;
  

• }
• if (!slave) {

• pr_err("SDW: No slave for paging addr\n");
  

• return -EINVAL;
  

I would move this test up, since if you have a NULL slave you should return an error in all case, otherwise there will be an oops in the code below ...

• } else if (!slave->prop.paging_support) {

• dev_err(&slave->dev,
  

• 	"address %x needs paging but no support", addr);
  

• return -EINVAL;
  

• }
• msg->addr_page1 = (addr >> SDW_REG_SHIFT(SDW_SCP_ADDRPAGE1_MASK));
• msg->addr_page2 = (addr >> SDW_REG_SHIFT(SDW_SCP_ADDRPAGE2_MASK));
• msg->addr |= BIT(15);
• msg->page = true;

looks ok :-)

• return 0;

+}

+/**

• • sdw_nread() - Read "n" contiguous SDW Slave registers
• • @slave: SDW Slave
• • @addr: Register address
• • @count: length
• • @val: Buffer for values to be read
• */

+int sdw_nread(struct sdw_slave *slave, u32 addr, size_t count, u8 *val) +{

• struct sdw_msg msg;
• int ret;
• ret = sdw_fill_msg(&msg, slave, addr, count,

• 	slave->dev_num, SDW_MSG_FLAG_READ, val);
  

• if (ret < 0)

• return ret;
  

... if you don't test for the slave argument in the sdw_fill_msg but the address is correct then the rest of the code will bomb out.

• ret = pm_runtime_get_sync(slave->bus->dev);
• if (!ret)

• return ret;
  

• ret = sdw_transfer(slave->bus, slave, &msg);
• pm_runtime_put(slave->bus->dev);
• return ret;

+} +EXPORT_SYMBOL(sdw_nread);

+/**

• • sdw_nwrite() - Write "n" contiguous SDW Slave registers
• • @slave: SDW Slave
• • @addr: Register address
• • @count: length
• • @val: Buffer for values to be read
• */

+int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, u8 *val) +{

• struct sdw_msg msg;
• int ret;
• ret = sdw_fill_msg(&msg, slave, addr, count,

• 	slave->dev_num, SDW_MSG_FLAG_WRITE, val);
  

• if (ret < 0)

• return ret;
  

• ret = pm_runtime_get_sync(slave->bus->dev);
• if (!ret)

• return ret;
  

• ret = sdw_transfer(slave->bus, slave, &msg);
• pm_runtime_put(slave->bus->dev);
• return ret;

+} +EXPORT_SYMBOL(sdw_nwrite);

+/**

• • sdw_read() - Read a SDW Slave register
• • @slave: SDW Slave
• • @addr: Register address
• */

+int sdw_read(struct sdw_slave *slave, u32 addr) +{

• u8 buf;
• int ret;
• ret = sdw_nread(slave, addr, 1, &buf);
• if (ret < 0)

• return ret;
  

• else

• return buf;
  

+} +EXPORT_SYMBOL(sdw_read);

+/**

• • sdw_write() - Write a SDW Slave register
• • @slave: SDW Slave
• • @addr: Register address
• • @value: Register value
• */

+int sdw_write(struct sdw_slave *slave, u32 addr, u8 value) +{

• return sdw_nwrite(slave, addr, 1, &value);

+} +EXPORT_SYMBOL(sdw_write);

• void sdw_extract_slave_id(struct sdw_bus *bus, u64 addr, struct sdw_slave_id *id) {

diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h index a54921825ce0..bde5f840ca96 100644 --- a/drivers/soundwire/bus.h +++ b/drivers/soundwire/bus.h @@ -16,4 +16,41 @@ static inline int sdw_acpi_find_slaves(struct sdw_bus *bus) void sdw_extract_slave_id(struct sdw_bus *bus, u64 addr, struct sdw_slave_id *id);

+enum {

• SDW_MSG_FLAG_READ = 0,
• SDW_MSG_FLAG_WRITE,

+};

+/**

• • struct sdw_msg - Message structure
• • @addr: Register address accessed in the Slave
• • @len: number of messages
• • @dev_num: Slave device number
• • @addr_page1: SCP address page 1 Slave register
• • @addr_page2: SCP address page 2 Slave register
• • @flags: transfer flags, indicate if xfer is read or write
• • @buf: message data buffer
• • @ssp_sync: Send message at SSP (Stream Synchronization Point)
• • @page: address requires paging
• */

+struct sdw_msg {

• u16 addr;
• u16 len;
• u16 dev_num;

was there a reason for dev_num with 16 bits - you have 16 values max...

• u8 addr_page1;
• u8 addr_page2;
• u8 flags;
• u8 *buf;
• bool ssp_sync;
• bool page;

+};

+int sdw_transfer(struct sdw_bus *bus, struct sdw_slave *slave,

• 	struct sdw_msg *msg);
  

+int sdw_transfer_defer(struct sdw_bus *bus, struct sdw_slave *slave,

• 	struct sdw_msg *msg, struct sdw_defer *defer);
  

+int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,

• u32 addr, size_t count, u16 dev_num, u8 flags, u8 *buf);
  

• #endif /* __SDW_BUS_H */

diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index ddfae18f4306..d3337b5b882b 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -31,6 +31,27 @@ enum sdw_slave_status { SDW_SLAVE_RESERVED = 3, };

+/**

• • enum sdw_command_response - Command response as defined by SDW spec
• • @SDW_CMD_OK: cmd was successful
• • @SDW_CMD_IGNORED: cmd was ignored
• • @SDW_CMD_FAIL: cmd was NACKed
• • @SDW_CMD_TIMEOUT: cmd timedout
• • @SDW_CMD_FAIL_OTHER: cmd failed due to other reason than above
• • NOTE: The enum is different than actual Spec as response in the Spec is
• • combination of ACK/NAK bits
• • SDW_CMD_TIMEOUT/FAIL_OTHER is defined for SW use, not in spec
• */

+enum sdw_command_response {

• SDW_CMD_OK = 0,
• SDW_CMD_IGNORED = 1,
• SDW_CMD_FAIL = 2,
• SDW_CMD_TIMEOUT = 4,
• SDW_CMD_FAIL_OTHER = 8,

Humm, I can't recall if/why this is a mask? does it need to be?

+};

• /*
  • SDW properties, defined in MIPI DisCo spec v1.0
  */

@@ -359,12 +380,37 @@ int sdw_handle_slave_status(struct sdw_bus *bus,

• SDW master structures and APIs

*/

+struct sdw_msg;

+/**

• • struct sdw_defer - SDW deffered message
• • @length: message length
• • @complete: message completion
• • @msg: SDW message
• */

+struct sdw_defer {

• int length;
• struct completion complete;
• struct sdw_msg *msg;

+};

• /**

  • struct sdw_master_ops - Master driver ops
  • @read_prop: Read Master properties
• • @xfer_msg: Transfer message callback
• • @xfer_msg_defer: Defer version of transfer message callback
• • @reset_page_addr: Reset the SCP page address registers

  */ struct sdw_master_ops { int (*read_prop)(struct sdw_bus *bus);

• enum sdw_command_response (*xfer_msg)

• 	(struct sdw_bus *bus, struct sdw_msg *msg);
  

• enum sdw_command_response (*xfer_msg_defer)

• 	(struct sdw_bus *bus, struct sdw_msg *msg,
  

• 	struct sdw_defer *defer);
  

• enum sdw_command_response (*reset_page_addr)

• 	(struct sdw_bus *bus, unsigned int dev_num);
  

  };

  /**

@@ -375,8 +421,10 @@ struct sdw_master_ops {

• @assigned: Bitmap for Slave device numbers.
• Bit set implies used number, bit clear implies unused number.
• @bus_lock: bus lock

• • @msg_lock: message lock
  • @ops: Master callback ops
  • @prop: Master properties
• • @defer_msg: Defer message
  • @clk_stop_timeout: Clock stop timeout computed
  */ struct sdw_bus {

@@ -385,12 +433,21 @@ struct sdw_bus { struct list_head slaves; DECLARE_BITMAP(assigned, SDW_MAX_DEVICES); struct mutex bus_lock;

• struct mutex msg_lock; const struct sdw_master_ops *ops; struct sdw_master_prop prop;

• struct sdw_defer defer_msg; unsigned int clk_stop_timeout; };

  int sdw_add_bus_master(struct sdw_bus *bus); void sdw_delete_bus_master(struct sdw_bus *bus);

+/* messaging and data APIs */

+int sdw_read(struct sdw_slave *slave, u32 addr); +int sdw_write(struct sdw_slave *slave, u32 addr, u8 value); +int sdw_nread(struct sdw_slave *slave, u32 addr, size_t count, u8 *val); +int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, u8 *val);

• #endif /* __SOUNDWIRE_H */