[v4,06/15] soundwire: Add IO transfer

Commit Message

Vinod Koul Dec. 1, 2017, 9:56 a.m. UTC

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 <[e-mail]>
Signed-off-by: Vinod Koul <[e-mail]>
---
 drivers/soundwire/bus.c       | 275 ++++++++++++++++++++++++++++++++++++++++++
 drivers/soundwire/bus.h       |  37 ++++++
 include/linux/soundwire/sdw.h |  57 +++++++++
 3 files changed, 369 insertions(+)

Comments

Pierre-Louis Bossart Dec. 1, 2017, 11:27 p.m. UTC | #1

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 <[e-mail]>
> Signed-off-by: Vinod Koul <[e-mail]>
> ---
>   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 */
>

Vinod Koul Dec. 3, 2017, 5:04 p.m. UTC | #2

On Fri, Dec 01, 2017 at 05:27:31PM -0600, Pierre-Louis Bossart wrote:

> >+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.

at higher level the error handling is same. the information is not lost as
it is expected that you would log it at error source.

> >+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.

CMD_IGNORED can be okay on broadcast. User of this API can retry all they
want!

> 
> 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.

well in those cases where you have blue wires, it actually helps :)

> >+/**
> >+ * 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?

That's what happens where API gets reworked umpteen times, thanks for
pointing. Earlier slave was required to get the page address calculation,
now that it is removed, it is no longer required !

> >+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

Sure..

> >+	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

thanks for spotting

> 
> >+	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 ...

naah, this fn is called for all IO, like broadcast where we have no slave.
So it is really optional for API, but for paging it is mandatory!

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

this wont oops as slave null would never come here

> >+		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 :-)

finally !!! yeah the paging and IO code has given me most headache till now!


> >+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.

I dont think so..

> >+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...

cant remember, we should use lesser bits though.

> >+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?

mask, not following!

Taking a wild guess that you are asking about last error, which is for SW
errors like malloc fail etc...

Pierre-Louis Bossart Dec. 4, 2017, 3:01 a.m. UTC | #3

On 12/3/17 11:04 AM, Vinod Koul wrote:
> On Fri, Dec 01, 2017 at 05:27:31PM -0600, Pierre-Louis Bossart wrote:
> 
>>> +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.
> 
> at higher level the error handling is same. the information is not lost as
> it is expected that you would log it at error source.

I don't understand this. It's certainly not the same for me if you 
detect an electric problem or if the IP is in the weeds. Logging at the 
source is fine but this filtering prevents higher levels from doing 
anything different.

> 
>>> +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.
> 
> CMD_IGNORED can be okay on broadcast. User of this API can retry all they
> want!

So you retry if this is a CMD_FAILED but let higher levels retry for 
CMD_IGNORED, sorry I don't see the logic.


> 
>>
>> 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.
> 
> well in those cases where you have blue wires, it actually helps :)

Blue wires are not supposed to change electrical behavior. TIMEOUT is 
only an internal SOC level issue, so no I don't get how this helps.

You have a retry count that is provided in the BIOS/firmware through 
disco properties and it's meant to bus errors. You are abusing the 
definitions. A command failed is supposed to be detected at the frame 
rate, which is typically 20us. a timeout is likely a 100s of ms value, 
so if you retry on top it's going to lock up the bus.

> 
>>> +/**
>>> + * 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?
> 
> That's what happens where API gets reworked umpteen times, thanks for
> pointing. Earlier slave was required to get the page address calculation,
> now that it is removed, it is no longer required !
> 
>>> +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
> 
> Sure..
> 
>>> +	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
> 
> thanks for spotting
> 
>>
>>> +	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 ...
> 
> naah, this fn is called for all IO, like broadcast where we have no slave.
> So it is really optional for API, but for paging it is mandatory!
> 
>>
>>> +	} else if (!slave->prop.paging_support) {
> 
> this wont oops as slave null would never come here
> 
>>> +		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 :-)
> 
> finally !!! yeah the paging and IO code has given me most headache till now!
> 
> 
>>> +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.
> 
> I dont think so..

Actually you are right, this makes no sense  to test for a null slave 
because you are already dead.

+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);

the dev_num indirection is already killing you.

+	if (ret < 0)
+		return ret;
+
+	ret = pm_runtime_get_sync(slave->bus->dev);
+	if (!ret)
+		return ret;

> 
>>> +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...
> 
> cant remember, we should use lesser bits though.
> 
>>> +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?
> 
> mask, not following!
> 
> Taking a wild guess that you are asking about last error, which is for SW
> errors like malloc fail etc...

no, I was asking why this is declared as if it was used for a bitmask, 
why not 0,1,2,3,4?

Vinod Koul Dec. 5, 2017, 6:31 a.m. UTC | #4

On Sun, Dec 03, 2017 at 09:01:41PM -0600, Pierre-Louis Bossart wrote:
> On 12/3/17 11:04 AM, Vinod Koul wrote:
> >On Fri, Dec 01, 2017 at 05:27:31PM -0600, Pierre-Louis Bossart wrote:

Sorry looks like I missed replying to this one earlier.

> >>>+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.
> >
> >at higher level the error handling is same. the information is not lost as
> >it is expected that you would log it at error source.
> 
> I don't understand this. It's certainly not the same for me if you detect an
> electric problem or if the IP is in the weeds. Logging at the source is fine
> but this filtering prevents higher levels from doing anything different.

The point is higher levels like here cant do much than bail out and complain.

Can you point out what would be different behaviour in each of these cases?

> >>>+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.
> >
> >CMD_IGNORED can be okay on broadcast. User of this API can retry all they
> >want!
> 
> So you retry if this is a CMD_FAILED but let higher levels retry for
> CMD_IGNORED, sorry I don't see the logic.

Yes that is right.

If I am doing a broadcast read, lets say for Device Id registers, why in the
world would I want to retry? CMD_IGNORED is a valid response and required to
stop enumeration cycle in that case.

But if I am not expecting a CMD_IGNORED response, I can very well go ahead
and retry from caller. The context is with caller and they can choose to do
appropriate handling.

And I have clarified this couple of times to you already, not sure how many
more times I would have to do that.

> >>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.
> >
> >well in those cases where you have blue wires, it actually helps :)
> 
> Blue wires are not supposed to change electrical behavior. TIMEOUT is only
> an internal SOC level issue, so no I don't get how this helps.
> 
> You have a retry count that is provided in the BIOS/firmware through disco
> properties and it's meant to bus errors. You are abusing the definitions. A
> command failed is supposed to be detected at the frame rate, which is
> typically 20us. a timeout is likely a 100s of ms value, so if you retry on
> top it's going to lock up the bus.

The world is not perfect! A guy debugging setups needs all the help. I do
not see any reason for not to retry. Bus is anyway locked up while a
transfer is ongoing (we serialize transfers).

Now if you feel this should be abhorred, I can change this for timeout.

> >>>+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?
> >
> >mask, not following!
> >
> >Taking a wild guess that you are asking about last error, which is for SW
> >errors like malloc fail etc...
> 
> no, I was asking why this is declared as if it was used for a bitmask, why
> not 0,1,2,3,4?

Oh okay, I think it was something to do with bits for errors, but don see it
helping so I can change it either way...

Pierre-Louis Bossart Dec. 5, 2017, 1:43 p.m. UTC | #5

On 12/5/17 12:31 AM, Vinod Koul wrote:
> On Sun, Dec 03, 2017 at 09:01:41PM -0600, Pierre-Louis Bossart wrote:
>> On 12/3/17 11:04 AM, Vinod Koul wrote:
>>> On Fri, Dec 01, 2017 at 05:27:31PM -0600, Pierre-Louis Bossart wrote:
> 
> Sorry looks like I missed replying to this one earlier.
> 
>>>>> +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.
>>>
>>> at higher level the error handling is same. the information is not lost as
>>> it is expected that you would log it at error source.
>>
>> I don't understand this. It's certainly not the same for me if you detect an
>> electric problem or if the IP is in the weeds. Logging at the source is fine
>> but this filtering prevents higher levels from doing anything different.
> 
> The point is higher levels like here cant do much than bail out and complain.
> 
> Can you point out what would be different behaviour in each of these cases?
> 
>>>>> +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.
>>>
>>> CMD_IGNORED can be okay on broadcast. User of this API can retry all they
>>> want!
>>
>> So you retry if this is a CMD_FAILED but let higher levels retry for
>> CMD_IGNORED, sorry I don't see the logic.
> 
> Yes that is right.
> 
> If I am doing a broadcast read, lets say for Device Id registers, why in the
> world would I want to retry? CMD_IGNORED is a valid response and required to
> stop enumeration cycle in that case.
> 
> But if I am not expecting a CMD_IGNORED response, I can very well go ahead
> and retry from caller. The context is with caller and they can choose to do
> appropriate handling.
> 
> And I have clarified this couple of times to you already, not sure how many
> more times I would have to do that.

Until you clarify what you are doing.
There is ONE case where IGNORED is a valid answer (reading the Prepare 
not finished bits), and it should not only be documented but analyzed in 
more details.
For a write an IGNORED is never OK.

> 
>>>> 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.
>>>
>>> well in those cases where you have blue wires, it actually helps :)
>>
>> Blue wires are not supposed to change electrical behavior. TIMEOUT is only
>> an internal SOC level issue, so no I don't get how this helps.
>>
>> You have a retry count that is provided in the BIOS/firmware through disco
>> properties and it's meant to bus errors. You are abusing the definitions. A
>> command failed is supposed to be detected at the frame rate, which is
>> typically 20us. a timeout is likely a 100s of ms value, so if you retry on
>> top it's going to lock up the bus.
> 
> The world is not perfect! A guy debugging setups needs all the help. I do
> not see any reason for not to retry. Bus is anyway locked up while a
> transfer is ongoing (we serialize transfers).
> 
> Now if you feel this should be abhorred, I can change this for timeout.

This TIMEOUT thing is your own definition, it's not part of the spec, so 
I don't see how it can be lumped together with spec-related parts.

It's fine to keep a retry but please document what the expectations are 
for the TIMEOUT case.

> 
>>>>> +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?
>>>
>>> mask, not following!
>>>
>>> Taking a wild guess that you are asking about last error, which is for SW
>>> errors like malloc fail etc...
>>
>> no, I was asking why this is declared as if it was used for a bitmask, why
>> not 0,1,2,3,4?
> 
> Oh okay, I think it was something to do with bits for errors, but don see it
> helping so I can change it either way...

Unless you use bit-wise operators and combined responses there is no 
reason to keep the current definitions.

Pierre-Louis Bossart Dec. 5, 2017, 2:48 p.m. UTC | #6

On 12/5/17 7:43 AM, Pierre-Louis Bossart wrote:
> On 12/5/17 12:31 AM, Vinod Koul wrote:
>> On Sun, Dec 03, 2017 at 09:01:41PM -0600, Pierre-Louis Bossart wrote:
>>> On 12/3/17 11:04 AM, Vinod Koul wrote:
>>>> On Fri, Dec 01, 2017 at 05:27:31PM -0600, Pierre-Louis Bossart wrote:
>>
>> Sorry looks like I missed replying to this one earlier.
>>
>>>>>> +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.
>>>>
>>>> at higher level the error handling is same. the information is not 
>>>> lost as
>>>> it is expected that you would log it at error source.
>>>
>>> I don't understand this. It's certainly not the same for me if you 
>>> detect an
>>> electric problem or if the IP is in the weeds. Logging at the source 
>>> is fine
>>> but this filtering prevents higher levels from doing anything different.
>>
>> The point is higher levels like here cant do much than bail out and 
>> complain.
>>
>> Can you point out what would be different behaviour in each of these 
>> cases?
>>
>>>>>> +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.
>>>>
>>>> CMD_IGNORED can be okay on broadcast. User of this API can retry all 
>>>> they
>>>> want!
>>>
>>> So you retry if this is a CMD_FAILED but let higher levels retry for
>>> CMD_IGNORED, sorry I don't see the logic.
>>
>> Yes that is right.
>>
>> If I am doing a broadcast read, lets say for Device Id registers, why 
>> in the
>> world would I want to retry? CMD_IGNORED is a valid response and 
>> required to
>> stop enumeration cycle in that case.
>>
>> But if I am not expecting a CMD_IGNORED response, I can very well go 
>> ahead
>> and retry from caller. The context is with caller and they can choose 
>> to do
>> appropriate handling.
>>
>> And I have clarified this couple of times to you already, not sure how 
>> many
>> more times I would have to do that.
> 
> Until you clarify what you are doing.
> There is ONE case where IGNORED is a valid answer (reading the Prepare 
> not finished bits), and it should not only be documented but analyzed in 
> more details.
I meant Read SCP_DevID registers from Device0... prepare bits should 
never return a CMD_IGNORED

> For a write an IGNORED is never OK.
> 
>>
>>>>> 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.
>>>>
>>>> well in those cases where you have blue wires, it actually helps :)
>>>
>>> Blue wires are not supposed to change electrical behavior. TIMEOUT is 
>>> only
>>> an internal SOC level issue, so no I don't get how this helps.
>>>
>>> You have a retry count that is provided in the BIOS/firmware through 
>>> disco
>>> properties and it's meant to bus errors. You are abusing the 
>>> definitions. A
>>> command failed is supposed to be detected at the frame rate, which is
>>> typically 20us. a timeout is likely a 100s of ms value, so if you 
>>> retry on
>>> top it's going to lock up the bus.
>>
>> The world is not perfect! A guy debugging setups needs all the help. I do
>> not see any reason for not to retry. Bus is anyway locked up while a
>> transfer is ongoing (we serialize transfers).
>>
>> Now if you feel this should be abhorred, I can change this for timeout.
> 
> This TIMEOUT thing is your own definition, it's not part of the spec, so 
> I don't see how it can be lumped together with spec-related parts.
> 
> It's fine to keep a retry but please document what the expectations are 
> for the TIMEOUT case.
> 
>>
>>>>>> +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?
>>>>
>>>> mask, not following!
>>>>
>>>> Taking a wild guess that you are asking about last error, which is 
>>>> for SW
>>>> errors like malloc fail etc...
>>>
>>> no, I was asking why this is declared as if it was used for a 
>>> bitmask, why
>>> not 0,1,2,3,4?
>>
>> Oh okay, I think it was something to do with bits for errors, but don 
>> see it
>> helping so I can change it either way...
> 
> Unless you use bit-wise operators and combined responses there is no 
> reason to keep the current definitions.
> 
> _______________________________________________
> Alsa-devel mailing list
> [e-mail]
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Vinod Koul Dec. 6, 2017, 5:58 a.m. UTC | #7

On Tue, Dec 05, 2017 at 08:48:03AM -0600, Pierre-Louis Bossart wrote:
> On 12/5/17 7:43 AM, Pierre-Louis Bossart wrote:
> >On 12/5/17 12:31 AM, Vinod Koul wrote:
> >>On Sun, Dec 03, 2017 at 09:01:41PM -0600, Pierre-Louis Bossart wrote:

> >>>>>>+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.
> >>>>
> >>>>CMD_IGNORED can be okay on broadcast. User of this API can retry all
> >>>>they
> >>>>want!
> >>>
> >>>So you retry if this is a CMD_FAILED but let higher levels retry for
> >>>CMD_IGNORED, sorry I don't see the logic.
> >>
> >>Yes that is right.
> >>
> >>If I am doing a broadcast read, lets say for Device Id registers, why in
> >>the
> >>world would I want to retry? CMD_IGNORED is a valid response and
> >>required to
> >>stop enumeration cycle in that case.

Above is the clarfication

> >>But if I am not expecting a CMD_IGNORED response, I can very well go
> >>ahead
> >>and retry from caller. The context is with caller and they can choose to
> >>do
> >>appropriate handling.
> >>
> >>And I have clarified this couple of times to you already, not sure how
> >>many
> >>more times I would have to do that.
> >
> >Until you clarify what you are doing.

Let me try again, I think u missed that part of my reply above

If I am doing a broadcast read, lets say for Device Id registers,
why in the world would I want to retry? CMD_IGNORED is a valid response
and required to stop enumeration cycle in that case.

> >There is ONE case where IGNORED is a valid answer (reading the Prepare not
> >finished bits), and it should not only be documented but analyzed in more
> >details.
> I meant Read SCP_DevID registers from Device0... prepare bits should never
> return a CMD_IGNORED

Precisely as I pointed out above.

> >For a write an IGNORED is never OK.

Agreed, but then transfer does both read and write. Write call can treat it
as error and read call leaves it upto caller.

Does that sound okay for you?

Pierre-Louis Bossart Dec. 6, 2017, 1:32 p.m. UTC | #8

On 12/5/17 11:58 PM, Vinod Koul wrote:
> On Tue, Dec 05, 2017 at 08:48:03AM -0600, Pierre-Louis Bossart wrote:
>> On 12/5/17 7:43 AM, Pierre-Louis Bossart wrote:
>>> On 12/5/17 12:31 AM, Vinod Koul wrote:
>>>> On Sun, Dec 03, 2017 at 09:01:41PM -0600, Pierre-Louis Bossart wrote:
> 
>>>>>>>> +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.
>>>>>>
>>>>>> CMD_IGNORED can be okay on broadcast. User of this API can retry all
>>>>>> they
>>>>>> want!
>>>>>
>>>>> So you retry if this is a CMD_FAILED but let higher levels retry for
>>>>> CMD_IGNORED, sorry I don't see the logic.
>>>>
>>>> Yes that is right.
>>>>
>>>> If I am doing a broadcast read, lets say for Device Id registers, why in
>>>> the
>>>> world would I want to retry? CMD_IGNORED is a valid response and
>>>> required to
>>>> stop enumeration cycle in that case.
> 
> Above is the clarfication
> 
>>>> But if I am not expecting a CMD_IGNORED response, I can very well go
>>>> ahead
>>>> and retry from caller. The context is with caller and they can choose to
>>>> do
>>>> appropriate handling.
>>>>
>>>> And I have clarified this couple of times to you already, not sure how
>>>> many
>>>> more times I would have to do that.
>>>
>>> Until you clarify what you are doing.
> 
> Let me try again, I think u missed that part of my reply above
> 
> If I am doing a broadcast read, lets say for Device Id registers,
> why in the world would I want to retry? CMD_IGNORED is a valid response
> and required to stop enumeration cycle in that case.
> 
>>> There is ONE case where IGNORED is a valid answer (reading the Prepare not
>>> finished bits), and it should not only be documented but analyzed in more
>>> details.
>> I meant Read SCP_DevID registers from Device0... prepare bits should never
>> return a CMD_IGNORED
> 
> Precisely as I pointed out above.
> 
>>> For a write an IGNORED is never OK.
> 
> Agreed, but then transfer does both read and write. Write call can treat it
> as error and read call leaves it upto caller.
> 
> Does that sound okay for you?

Not really.
You have one case where it's ok and all others are not ok, to me this 
sounds like you should avoid the retry at the lowest level rather than 
pushing this to the caller
And now that I think of it, the definitions for the DisCo spec were 
really meant for hardware-based retries available in some master IPs. If 
this is enabled, then the loops in software are not really needed at 
all. Can you please check this point?

Vinod Koul Dec. 6, 2017, 2:44 p.m. UTC | #9

On Wed, Dec 06, 2017 at 07:32:43AM -0600, Pierre-Louis Bossart wrote:
> On 12/5/17 11:58 PM, Vinod Koul wrote:
> >On Tue, Dec 05, 2017 at 08:48:03AM -0600, Pierre-Louis Bossart wrote:
> >>On 12/5/17 7:43 AM, Pierre-Louis Bossart wrote:
> >>>On 12/5/17 12:31 AM, Vinod Koul wrote:
> >>>>On Sun, Dec 03, 2017 at 09:01:41PM -0600, Pierre-Louis Bossart wrote:
> >
> >>>>>>>>+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.
> >>>>>>
> >>>>>>CMD_IGNORED can be okay on broadcast. User of this API can retry all
> >>>>>>they
> >>>>>>want!
> >>>>>
> >>>>>So you retry if this is a CMD_FAILED but let higher levels retry for
> >>>>>CMD_IGNORED, sorry I don't see the logic.
> >>>>
> >>>>Yes that is right.
> >>>>
> >>>>If I am doing a broadcast read, lets say for Device Id registers, why in
> >>>>the
> >>>>world would I want to retry? CMD_IGNORED is a valid response and
> >>>>required to
> >>>>stop enumeration cycle in that case.
> >
> >Above is the clarfication
> >
> >>>>But if I am not expecting a CMD_IGNORED response, I can very well go
> >>>>ahead
> >>>>and retry from caller. The context is with caller and they can choose to
> >>>>do
> >>>>appropriate handling.
> >>>>
> >>>>And I have clarified this couple of times to you already, not sure how
> >>>>many
> >>>>more times I would have to do that.
> >>>
> >>>Until you clarify what you are doing.
> >
> >Let me try again, I think u missed that part of my reply above
> >
> >If I am doing a broadcast read, lets say for Device Id registers,
> >why in the world would I want to retry? CMD_IGNORED is a valid response
> >and required to stop enumeration cycle in that case.
> >
> >>>There is ONE case where IGNORED is a valid answer (reading the Prepare not
> >>>finished bits), and it should not only be documented but analyzed in more
> >>>details.
> >>I meant Read SCP_DevID registers from Device0... prepare bits should never
> >>return a CMD_IGNORED
> >
> >Precisely as I pointed out above.
> >
> >>>For a write an IGNORED is never OK.
> >
> >Agreed, but then transfer does both read and write. Write call can treat it
> >as error and read call leaves it upto caller.
> >
> >Does that sound okay for you?
> 
> Not really.
> You have one case where it's ok and all others are not ok, to me this sounds
> like you should avoid the retry at the lowest level rather than pushing this
> to the caller
> And now that I think of it, the definitions for the DisCo spec were really
> meant for hardware-based retries available in some master IPs. If this is
> enabled, then the loops in software are not really needed at all. Can you
> please check this point?

Sure I will check that

Patch

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;
+	}
+}
+
+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)
+			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
+ * @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
+ * @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;
+	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 */
+	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;
+	} 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;
+
+	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;
+
+	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;
+	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,
+};
+
 /*
  * 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 */