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

From: Pierre-Louis Bossart
Date: Fri Dec 01 2017 - 18:27:40 EST

Next message: Florian Fainelli: "Re: [PATCH v5 net-next,mips 1/7] dt-bindings: Add Cavium Octeon Common Ethernet Interface."
Previous message: David Daney: "[PATCH v5 net-next,mips 5/7] MIPS: Octeon: Automatically provision CVMSEG space."
In reply to: Vinod Koul: "[PATCH v4 06/15] soundwire: Add IO transfer"
Next in thread: Vinod Koul: "Re: [alsa-devel] [PATCH v4 06/15] soundwire: Add IO transfer"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxx>
Signed-off-by: Vinod Koul <vinod.koul@xxxxxxxxx>
---
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 */

Next message: Florian Fainelli: "Re: [PATCH v5 net-next,mips 1/7] dt-bindings: Add Cavium Octeon Common Ethernet Interface."
Previous message: David Daney: "[PATCH v5 net-next,mips 5/7] MIPS: Octeon: Automatically provision CVMSEG space."
In reply to: Vinod Koul: "[PATCH v4 06/15] soundwire: Add IO transfer"
Next in thread: Vinod Koul: "Re: [alsa-devel] [PATCH v4 06/15] soundwire: Add IO transfer"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]