Messages in this thread Patch in this message | | | Subject | Re: [alsa-devel] [PATCH v4 06/15] soundwire: Add IO transfer | From | Pierre-Louis Bossart <> | Date | Fri, 1 Dec 2017 17:27:31 -0600 |
| |
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 */ >
| |