[PATCH 0/7] soundwire: add BTP/BRA prerequisites

[PATCH 0/7] soundwire: add BTP/BRA prerequisites

[Bard Liao]

Some prerequisites for BTP/BRA.

Pierre-Louis Bossart (7):
  soundwire: cadence: fix invalid PDI offset
  soundwire: cadence: remove PDI offset completely
  soundwire: remove unused sdw_bus_conf structure
  soundwire: reconcile dp0_prop and dpn_prop
  soundwire: clarify maximum allowed address
  soundwire: debugfs: add interface to read/write commands
  soundwire: bus: add stream refcount

 drivers/soundwire/cadence_master.c      |  30 ++---
 drivers/soundwire/debugfs.c             | 150 ++++++++++++++++++++++++
 drivers/soundwire/stream.c              |   5 +
 include/linux/soundwire/sdw.h           |  21 +---
 include/linux/soundwire/sdw_registers.h |   2 +-
 5 files changed, 170 insertions(+), 38 deletions(-)

-- 
2.34.1

[PATCH 1/7] soundwire: cadence: fix invalid PDI offset

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

For some reason, we add an offset to the PDI, presumably to skip the
PDI0 and PDI1 which are reserved for BPT.

This code is however completely wrong and leads to an out-of-bounds
access. We were just lucky so far since we used only a couple of PDIs
and remained within the PDI array bounds.

A Fixes: tag is not provided since there are no known platforms where
the out-of-bounds would be accessed, and the initial code had problems
as well.

A follow-up patch completely removes this useless offset.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/cadence_master.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
index 0efc1c3bee5f..3e7cf04aaf2a 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -1880,7 +1880,7 @@ struct sdw_cdns_pdi *sdw_cdns_alloc_pdi(struct sdw_cdns *cdns,
 
 	/* check if we found a PDI, else find in bi-directional */
 	if (!pdi)
-		pdi = cdns_find_pdi(cdns, 2, stream->num_bd, stream->bd,
+		pdi = cdns_find_pdi(cdns, 0, stream->num_bd, stream->bd,
 				    dai_id);
 
 	if (pdi) {
-- 
2.34.1

[PATCH 2/7] soundwire: cadence: remove PDI offset completely

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

This offset is set to exactly zero and serves no purpose. Remove.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/cadence_master.c | 30 +++++++++---------------------
 1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
index 3e7cf04aaf2a..c2c1463a5c53 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -1236,7 +1236,7 @@ EXPORT_SYMBOL(sdw_cdns_enable_interrupt);
 
 static int cdns_allocate_pdi(struct sdw_cdns *cdns,
 			     struct sdw_cdns_pdi **stream,
-			     u32 num, u32 pdi_offset)
+			     u32 num)
 {
 	struct sdw_cdns_pdi *pdi;
 	int i;
@@ -1249,7 +1249,7 @@ static int cdns_allocate_pdi(struct sdw_cdns *cdns,
 		return -ENOMEM;
 
 	for (i = 0; i < num; i++) {
-		pdi[i].num = i + pdi_offset;
+		pdi[i].num = i;
 	}
 
 	*stream = pdi;
@@ -1266,7 +1266,6 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
 		      struct sdw_cdns_stream_config config)
 {
 	struct sdw_cdns_streams *stream;
-	int offset;
 	int ret;
 
 	cdns->pcm.num_bd = config.pcm_bd;
@@ -1277,24 +1276,15 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
 	stream = &cdns->pcm;
 
 	/* we allocate PDI0 and PDI1 which are used for Bulk */
-	offset = 0;
-
-	ret = cdns_allocate_pdi(cdns, &stream->bd,
-				stream->num_bd, offset);
+	ret = cdns_allocate_pdi(cdns, &stream->bd, stream->num_bd);
 	if (ret)
 		return ret;
 
-	offset += stream->num_bd;
-
-	ret = cdns_allocate_pdi(cdns, &stream->in,
-				stream->num_in, offset);
+	ret = cdns_allocate_pdi(cdns, &stream->in, stream->num_in);
 	if (ret)
 		return ret;
 
-	offset += stream->num_in;
-
-	ret = cdns_allocate_pdi(cdns, &stream->out,
-				stream->num_out, offset);
+	ret = cdns_allocate_pdi(cdns, &stream->out, stream->num_out);
 	if (ret)
 		return ret;
 
@@ -1802,7 +1792,6 @@ EXPORT_SYMBOL(cdns_set_sdw_stream);
  * cdns_find_pdi() - Find a free PDI
  *
  * @cdns: Cadence instance
- * @offset: Starting offset
  * @num: Number of PDIs
  * @pdi: PDI instances
  * @dai_id: DAI id
@@ -1811,14 +1800,13 @@ EXPORT_SYMBOL(cdns_set_sdw_stream);
  * expected to match, return NULL otherwise.
  */
 static struct sdw_cdns_pdi *cdns_find_pdi(struct sdw_cdns *cdns,
-					  unsigned int offset,
 					  unsigned int num,
 					  struct sdw_cdns_pdi *pdi,
 					  int dai_id)
 {
 	int i;
 
-	for (i = offset; i < offset + num; i++)
+	for (i = 0; i < num; i++)
 		if (pdi[i].num == dai_id)
 			return &pdi[i];
 
@@ -1872,15 +1860,15 @@ struct sdw_cdns_pdi *sdw_cdns_alloc_pdi(struct sdw_cdns *cdns,
 	struct sdw_cdns_pdi *pdi = NULL;
 
 	if (dir == SDW_DATA_DIR_RX)
-		pdi = cdns_find_pdi(cdns, 0, stream->num_in, stream->in,
+		pdi = cdns_find_pdi(cdns, stream->num_in, stream->in,
 				    dai_id);
 	else
-		pdi = cdns_find_pdi(cdns, 0, stream->num_out, stream->out,
+		pdi = cdns_find_pdi(cdns, stream->num_out, stream->out,
 				    dai_id);
 
 	/* check if we found a PDI, else find in bi-directional */
 	if (!pdi)
-		pdi = cdns_find_pdi(cdns, 0, stream->num_bd, stream->bd,
+		pdi = cdns_find_pdi(cdns, stream->num_bd, stream->bd,
 				    dai_id);
 
 	if (pdi) {
-- 
2.34.1

[PATCH 3/7] soundwire: remove unused sdw_bus_conf structure

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

This is redundant with sdw_bus_params, and was never used.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 include/linux/soundwire/sdw.h | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 66f814b63a43..e5d0aa58e7f6 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -542,21 +542,6 @@ enum sdw_reg_bank {
 	SDW_BANK1,
 };
 
-/**
- * struct sdw_bus_conf: Bus configuration
- *
- * @clk_freq: Clock frequency, in Hz
- * @num_rows: Number of rows in frame
- * @num_cols: Number of columns in frame
- * @bank: Next register bank
- */
-struct sdw_bus_conf {
-	unsigned int clk_freq;
-	unsigned int num_rows;
-	unsigned int num_cols;
-	unsigned int bank;
-};
-
 /**
  * struct sdw_prepare_ch: Prepare/De-prepare Data Port channel
  *
-- 
2.34.1

[PATCH 4/7] soundwire: reconcile dp0_prop and dpn_prop

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The definitions for DP0 are missing a set of fields that are required
to reuse the same configuration code as DPn.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 include/linux/soundwire/sdw.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index e5d0aa58e7f6..e3a4bccc2a7e 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -235,7 +235,8 @@ enum sdw_clk_stop_mode {
  * @BRA_flow_controlled: Slave implementation results in an OK_NotReady
  * response
  * @simple_ch_prep_sm: If channel prepare sequence is required
- * @imp_def_interrupts: If set, each bit corresponds to support for
+  * @ch_prep_timeout: Port-specific timeout value, in milliseconds
+  * @imp_def_interrupts: If set, each bit corresponds to support for
  * implementation-defined interrupts
  *
  * The wordlengths are specified by Spec as max, min AND number of
@@ -249,6 +250,7 @@ struct sdw_dp0_prop {
 	u32 *words;
 	bool BRA_flow_controlled;
 	bool simple_ch_prep_sm;
+	u32 ch_prep_timeout;
 	bool imp_def_interrupts;
 };
 
-- 
2.34.1

[PATCH 5/7] soundwire: clarify maximum allowed address

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The existing code sets the maximum address at 0x80000000, which is not
completely accurate. The last 2 Gbytes are indeed reserved, but so are
the 896 Mbytes just before. The maximum address which can be used with
paging or BRA is 0x47FFFFFF per Table 131 of the SoundWire 1.2.1
specification.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 include/linux/soundwire/sdw_registers.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/soundwire/sdw_registers.h b/include/linux/soundwire/sdw_registers.h
index 138bec908c40..658b10fa5b20 100644
--- a/include/linux/soundwire/sdw_registers.h
+++ b/include/linux/soundwire/sdw_registers.h
@@ -13,7 +13,7 @@
 
 #define SDW_REG_NO_PAGE				0x00008000
 #define SDW_REG_OPTIONAL_PAGE			0x00010000
-#define SDW_REG_MAX				0x80000000
+#define SDW_REG_MAX				0x48000000
 
 #define SDW_DPN_SIZE				0x100
 #define SDW_BANK1_OFFSET			0x10
-- 
2.34.1

[PATCH 6/7] soundwire: debugfs: add interface to read/write commands

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

We have an existing debugfs files to read standard registers
(DP0/SCP/DPn).

This patch provides a more generic interface to ANY set of read/write
contiguous registers in a peripheral device. In follow-up patches,
this interface will be extended to use BRA transfers.

The sequence is to use the following files added under the existing
debugsfs directory for each peripheral device:

command (write 0, read 1)
num_bytes
start_address
firmware_file (only for writes)
read_buffer (only for reads)

Example for a read command - this checks the 6 bytes used for
enumeration.

cd /sys/kernel/debug/soundwire/master-0-0/sdw\:0\:025d\:0711\:01/
echo 1 > command
echo 6 > num_bytes
echo 0x50 > start_address
echo 1 > go
cat read_buffer
address 0x50 val 0x30
address 0x51 val 0x02
address 0x52 val 0x5d
address 0x53 val 0x07
address 0x54 val 0x11
address 0x55 val 0x01

Example with a 2-byte firmware file written in DP0 address 0x22

od -x /lib/firmware/test_firmware
0000000 0a37
0000002

cd /sys/kernel/debug/soundwire/master-0-0/sdw\:0\:025d\:0711\:01/
echo 0 > command
echo 2 > num_bytes
echo 0x22 > start_address
echo "test_firmware" > firmware_file
echo 1 > go

cd /sys/kernel/debug/soundwire/master-0-0/sdw\:0\:025d\:0711\:01/
echo 1 > command
echo 2 > num_bytes
echo 0x22 > start_address
echo 1 > go
cat read_buffer

address 0x22 val 0x37
address 0x23 val 0x0a

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/debugfs.c | 150 ++++++++++++++++++++++++++++++++++++
 1 file changed, 150 insertions(+)

diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
index 67abd7e52f09..6d253d69871d 100644
--- a/drivers/soundwire/debugfs.c
+++ b/drivers/soundwire/debugfs.c
@@ -3,6 +3,7 @@
 
 #include <linux/device.h>
 #include <linux/debugfs.h>
+#include <linux/firmware.h>
 #include <linux/mod_devicetable.h>
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
@@ -137,6 +138,145 @@ static int sdw_slave_reg_show(struct seq_file *s_file, void *data)
 }
 DEFINE_SHOW_ATTRIBUTE(sdw_slave_reg);
 
+#define MAX_CMD_BYTES 256
+
+static int cmd;
+static u32 start_addr;
+static size_t num_bytes;
+static u8 read_buffer[MAX_CMD_BYTES];
+static char *firmware_file;
+
+static int set_command(void *data, u64 value)
+{
+	struct sdw_slave *slave = data;
+
+	if (value > 1)
+		return -EINVAL;
+
+	/* Userspace changed the hardware state behind the kernel's back */
+	add_taint(TAINT_USER, LOCKDEP_STILL_OK);
+
+	dev_dbg(&slave->dev, "command: %s\n", value ? "read" : "write");
+	cmd = value;
+
+	return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(set_command_fops, NULL,
+			 set_command, "%llu\n");
+
+static int set_start_address(void *data, u64 value)
+{
+	struct sdw_slave *slave = data;
+
+	/* Userspace changed the hardware state behind the kernel's back */
+	add_taint(TAINT_USER, LOCKDEP_STILL_OK);
+
+	dev_dbg(&slave->dev, "start address %#llx\n", value);
+
+	start_addr = value;
+
+	return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(set_start_address_fops, NULL,
+			 set_start_address, "%llu\n");
+
+static int set_num_bytes(void *data, u64 value)
+{
+	struct sdw_slave *slave = data;
+
+	if (value == 0 || value > MAX_CMD_BYTES)
+		return -EINVAL;
+
+	/* Userspace changed the hardware state behind the kernel's back */
+	add_taint(TAINT_USER, LOCKDEP_STILL_OK);
+
+	dev_dbg(&slave->dev, "number of bytes %lld\n", value);
+
+	num_bytes = value;
+
+	return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(set_num_bytes_fops, NULL,
+			 set_num_bytes, "%llu\n");
+
+static int cmd_go(void *data, u64 value)
+{
+	struct sdw_slave *slave = data;
+	int ret;
+
+	if (value != 1)
+		return -EINVAL;
+
+	/* one last check */
+	if (start_addr > SDW_REG_MAX ||
+	    num_bytes == 0 || num_bytes > MAX_CMD_BYTES)
+		return -EINVAL;
+
+	ret = pm_runtime_get_sync(&slave->dev);
+	if (ret < 0 && ret != -EACCES) {
+		pm_runtime_put_noidle(&slave->dev);
+		return ret;
+	}
+
+	/* Userspace changed the hardware state behind the kernel's back */
+	add_taint(TAINT_USER, LOCKDEP_STILL_OK);
+
+	dev_dbg(&slave->dev, "starting command\n");
+
+	if (cmd == 0) {
+		const struct firmware *fw;
+
+		ret = request_firmware(&fw, firmware_file, &slave->dev);
+		if (ret < 0) {
+			dev_err(&slave->dev, "firmware %s not found\n", firmware_file);
+			goto out;
+		}
+
+		if (fw->size != num_bytes) {
+			dev_err(&slave->dev,
+				"firmware %s: unexpected size %zd, desired %zd\n",
+				firmware_file, fw->size, num_bytes);
+			release_firmware(fw);
+			goto out;
+		}
+
+		ret = sdw_nwrite_no_pm(slave, start_addr, num_bytes, fw->data);
+		release_firmware(fw);
+	} else {
+		ret = sdw_nread_no_pm(slave, start_addr, num_bytes, read_buffer);
+	}
+
+	dev_dbg(&slave->dev, "command completed %d\n", ret);
+
+out:
+	pm_runtime_mark_last_busy(&slave->dev);
+	pm_runtime_put(&slave->dev);
+
+	return ret;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(cmd_go_fops, NULL,
+			 cmd_go, "%llu\n");
+
+#define MAX_LINE_LEN 128
+
+static int read_buffer_show(struct seq_file *s_file, void *data)
+{
+	char buf[MAX_LINE_LEN];
+	int i;
+
+	if (num_bytes == 0 || num_bytes > MAX_CMD_BYTES)
+		return -EINVAL;
+
+	for (i = 0; i < num_bytes; i++) {
+		scnprintf(buf, MAX_LINE_LEN, "address %#x val 0x%02x\n",
+			  start_addr + i, read_buffer[i]);
+		seq_printf(s_file, "%s", buf);
+	}
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(read_buffer);
+
 void sdw_slave_debugfs_init(struct sdw_slave *slave)
 {
 	struct dentry *master;
@@ -151,6 +291,16 @@ void sdw_slave_debugfs_init(struct sdw_slave *slave)
 
 	debugfs_create_file("registers", 0400, d, slave, &sdw_slave_reg_fops);
 
+	/* interface to send arbitrary commands */
+	debugfs_create_file("command", 0200, d, slave, &set_command_fops);
+	debugfs_create_file("start_address", 0200, d, slave, &set_start_address_fops);
+	debugfs_create_file("num_bytes", 0200, d, slave, &set_num_bytes_fops);
+	debugfs_create_file("go", 0200, d, slave, &cmd_go_fops);
+
+	debugfs_create_file("read_buffer", 0400, d, slave, &read_buffer_fops);
+	firmware_file = NULL;
+	debugfs_create_str("firmware_file", 0200, d, &firmware_file);
+
 	slave->debugfs = d;
 }
 
-- 
2.34.1

[PATCH 7/7] soundwire: bus: add stream refcount

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The notion of stream is by construction based on a multi-bus
capability, to allow for aggregation of Peripheral devices or
functions located on different segments. We currently count how many
master_rt contexts are used by a stream, but we don't have the dual
refcount of how many streams are allocated on a given bus. This
refcount will be useful to check if BTP/BRA streams can be allocated.

Note that the stream_refcount is modified in sdw_master_rt_alloc() and
sdw_master_rt_free() which are both called with the bus_lock mutex
held, so there's no need for refcount_ primitives for additional
protection.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/stream.c    | 5 +++++
 include/linux/soundwire/sdw.h | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index 4e9e7d2a942d..7aa4900dcf31 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -1181,6 +1181,8 @@ static struct sdw_master_runtime
 	m_rt->bus = bus;
 	m_rt->stream = stream;
 
+	bus->stream_refcount++;
+
 	return m_rt;
 }
 
@@ -1217,6 +1219,7 @@ static void sdw_master_rt_free(struct sdw_master_runtime *m_rt,
 			       struct sdw_stream_runtime *stream)
 {
 	struct sdw_slave_runtime *s_rt, *_s_rt;
+	struct sdw_bus *bus = m_rt->bus;
 
 	list_for_each_entry_safe(s_rt, _s_rt, &m_rt->slave_rt_list, m_rt_node) {
 		sdw_slave_port_free(s_rt->slave, stream);
@@ -1226,6 +1229,8 @@ static void sdw_master_rt_free(struct sdw_master_runtime *m_rt,
 	list_del(&m_rt->stream_node);
 	list_del(&m_rt->bus_node);
 	kfree(m_rt);
+
+	bus->stream_refcount--;
 }
 
 /**
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index e3a4bccc2a7e..71a7031f7b3a 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -902,6 +902,7 @@ struct sdw_master_ops {
  * meaningful if multi_link is set. If set to 1, hardware-based
  * synchronization will be used even if a stream only uses a single
  * SoundWire segment.
+ * @stream_refcount: number of streams currently using this bus
  */
 struct sdw_bus {
 	struct device *dev;
@@ -931,6 +932,7 @@ struct sdw_bus {
 	u32 bank_switch_timeout;
 	bool multi_link;
 	int hw_sync_min_links;
+	int stream_refcount;
 };
 
 int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
-- 
2.34.1

Re: [PATCH 4/7] soundwire: reconcile dp0_prop and dpn_prop

[Vinod Koul]

On 26-03-24, 09:01, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> The definitions for DP0 are missing a set of fields that are required
> to reuse the same configuration code as DPn.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Rander Wang <rander.wang@intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---
>  include/linux/soundwire/sdw.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index e5d0aa58e7f6..e3a4bccc2a7e 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -235,7 +235,8 @@ enum sdw_clk_stop_mode {
>   * @BRA_flow_controlled: Slave implementation results in an OK_NotReady
>   * response
>   * @simple_ch_prep_sm: If channel prepare sequence is required
> - * @imp_def_interrupts: If set, each bit corresponds to support for
> +  * @ch_prep_timeout: Port-specific timeout value, in milliseconds
> +  * @imp_def_interrupts: If set, each bit corresponds to support for

why is this line modified? seems to be same as previous and leading
space added which might not be correct


>   * implementation-defined interrupts
>   *
>   * The wordlengths are specified by Spec as max, min AND number of
> @@ -249,6 +250,7 @@ struct sdw_dp0_prop {
>  	u32 *words;
>  	bool BRA_flow_controlled;
>  	bool simple_ch_prep_sm;
> +	u32 ch_prep_timeout;
>  	bool imp_def_interrupts;
>  };
>  
> -- 
> 2.34.1

-- 
~Vinod

Re: [PATCH 6/7] soundwire: debugfs: add interface to read/write commands

[Vinod Koul]

On 26-03-24, 09:01, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> We have an existing debugfs files to read standard registers
> (DP0/SCP/DPn).
> 
> This patch provides a more generic interface to ANY set of read/write
> contiguous registers in a peripheral device. In follow-up patches,
> this interface will be extended to use BRA transfers.
> 
> The sequence is to use the following files added under the existing
> debugsfs directory for each peripheral device:
> 
> command (write 0, read 1)
> num_bytes
> start_address
> firmware_file (only for writes)
> read_buffer (only for reads)
> 
> Example for a read command - this checks the 6 bytes used for
> enumeration.
> 
> cd /sys/kernel/debug/soundwire/master-0-0/sdw\:0\:025d\:0711\:01/
> echo 1 > command
> echo 6 > num_bytes
> echo 0x50 > start_address
> echo 1 > go

can we have a simpler interface? i am not a big fan of this kind of
structure for debugging.

How about two files read_bytes and write_bytes where you read/write
bytes.

echo 0x50 6 > read_bytes
cat read_bytes

in this format I would like to see addr and values (not need to print
address /value words (regmap does that too)

For write

echo start_addr N byte0 byte 1 ... byte N > write_bytes

 
> cat read_buffer
> address 0x50 val 0x30
> address 0x51 val 0x02
> address 0x52 val 0x5d
> address 0x53 val 0x07
> address 0x54 val 0x11
> address 0x55 val 0x01
> 
> Example with a 2-byte firmware file written in DP0 address 0x22
> 
> od -x /lib/firmware/test_firmware
> 0000000 0a37
> 0000002
> 
> cd /sys/kernel/debug/soundwire/master-0-0/sdw\:0\:025d\:0711\:01/
> echo 0 > command
> echo 2 > num_bytes
> echo 0x22 > start_address
> echo "test_firmware" > firmware_file
> echo 1 > go
> 
> cd /sys/kernel/debug/soundwire/master-0-0/sdw\:0\:025d\:0711\:01/
> echo 1 > command
> echo 2 > num_bytes
> echo 0x22 > start_address
> echo 1 > go
> cat read_buffer
> 
> address 0x22 val 0x37
> address 0x23 val 0x0a
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Rander Wang <rander.wang@intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---
>  drivers/soundwire/debugfs.c | 150 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 150 insertions(+)
> 
> diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
> index 67abd7e52f09..6d253d69871d 100644
> --- a/drivers/soundwire/debugfs.c
> +++ b/drivers/soundwire/debugfs.c
> @@ -3,6 +3,7 @@
>  
>  #include <linux/device.h>
>  #include <linux/debugfs.h>
> +#include <linux/firmware.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/slab.h>
> @@ -137,6 +138,145 @@ static int sdw_slave_reg_show(struct seq_file *s_file, void *data)
>  }
>  DEFINE_SHOW_ATTRIBUTE(sdw_slave_reg);
>  
> +#define MAX_CMD_BYTES 256
> +
> +static int cmd;
> +static u32 start_addr;
> +static size_t num_bytes;
> +static u8 read_buffer[MAX_CMD_BYTES];
> +static char *firmware_file;
> +
> +static int set_command(void *data, u64 value)
> +{
> +	struct sdw_slave *slave = data;
> +
> +	if (value > 1)
> +		return -EINVAL;
> +
> +	/* Userspace changed the hardware state behind the kernel's back */
> +	add_taint(TAINT_USER, LOCKDEP_STILL_OK);
> +
> +	dev_dbg(&slave->dev, "command: %s\n", value ? "read" : "write");
> +	cmd = value;
> +
> +	return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(set_command_fops, NULL,
> +			 set_command, "%llu\n");
> +
> +static int set_start_address(void *data, u64 value)
> +{
> +	struct sdw_slave *slave = data;
> +
> +	/* Userspace changed the hardware state behind the kernel's back */
> +	add_taint(TAINT_USER, LOCKDEP_STILL_OK);
> +
> +	dev_dbg(&slave->dev, "start address %#llx\n", value);
> +
> +	start_addr = value;
> +
> +	return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(set_start_address_fops, NULL,
> +			 set_start_address, "%llu\n");
> +
> +static int set_num_bytes(void *data, u64 value)
> +{
> +	struct sdw_slave *slave = data;
> +
> +	if (value == 0 || value > MAX_CMD_BYTES)
> +		return -EINVAL;
> +
> +	/* Userspace changed the hardware state behind the kernel's back */
> +	add_taint(TAINT_USER, LOCKDEP_STILL_OK);
> +
> +	dev_dbg(&slave->dev, "number of bytes %lld\n", value);
> +
> +	num_bytes = value;
> +
> +	return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(set_num_bytes_fops, NULL,
> +			 set_num_bytes, "%llu\n");
> +
> +static int cmd_go(void *data, u64 value)
> +{
> +	struct sdw_slave *slave = data;
> +	int ret;
> +
> +	if (value != 1)
> +		return -EINVAL;
> +
> +	/* one last check */
> +	if (start_addr > SDW_REG_MAX ||
> +	    num_bytes == 0 || num_bytes > MAX_CMD_BYTES)
> +		return -EINVAL;
> +
> +	ret = pm_runtime_get_sync(&slave->dev);
> +	if (ret < 0 && ret != -EACCES) {
> +		pm_runtime_put_noidle(&slave->dev);
> +		return ret;
> +	}
> +
> +	/* Userspace changed the hardware state behind the kernel's back */
> +	add_taint(TAINT_USER, LOCKDEP_STILL_OK);
> +
> +	dev_dbg(&slave->dev, "starting command\n");
> +
> +	if (cmd == 0) {
> +		const struct firmware *fw;
> +
> +		ret = request_firmware(&fw, firmware_file, &slave->dev);
> +		if (ret < 0) {
> +			dev_err(&slave->dev, "firmware %s not found\n", firmware_file);
> +			goto out;
> +		}
> +
> +		if (fw->size != num_bytes) {
> +			dev_err(&slave->dev,
> +				"firmware %s: unexpected size %zd, desired %zd\n",
> +				firmware_file, fw->size, num_bytes);
> +			release_firmware(fw);
> +			goto out;
> +		}
> +
> +		ret = sdw_nwrite_no_pm(slave, start_addr, num_bytes, fw->data);
> +		release_firmware(fw);
> +	} else {
> +		ret = sdw_nread_no_pm(slave, start_addr, num_bytes, read_buffer);
> +	}
> +
> +	dev_dbg(&slave->dev, "command completed %d\n", ret);
> +
> +out:
> +	pm_runtime_mark_last_busy(&slave->dev);
> +	pm_runtime_put(&slave->dev);
> +
> +	return ret;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(cmd_go_fops, NULL,
> +			 cmd_go, "%llu\n");
> +
> +#define MAX_LINE_LEN 128
> +
> +static int read_buffer_show(struct seq_file *s_file, void *data)
> +{
> +	char buf[MAX_LINE_LEN];
> +	int i;
> +
> +	if (num_bytes == 0 || num_bytes > MAX_CMD_BYTES)
> +		return -EINVAL;
> +
> +	for (i = 0; i < num_bytes; i++) {
> +		scnprintf(buf, MAX_LINE_LEN, "address %#x val 0x%02x\n",
> +			  start_addr + i, read_buffer[i]);
> +		seq_printf(s_file, "%s", buf);
> +	}
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(read_buffer);
> +
>  void sdw_slave_debugfs_init(struct sdw_slave *slave)
>  {
>  	struct dentry *master;
> @@ -151,6 +291,16 @@ void sdw_slave_debugfs_init(struct sdw_slave *slave)
>  
>  	debugfs_create_file("registers", 0400, d, slave, &sdw_slave_reg_fops);
>  
> +	/* interface to send arbitrary commands */
> +	debugfs_create_file("command", 0200, d, slave, &set_command_fops);
> +	debugfs_create_file("start_address", 0200, d, slave, &set_start_address_fops);
> +	debugfs_create_file("num_bytes", 0200, d, slave, &set_num_bytes_fops);
> +	debugfs_create_file("go", 0200, d, slave, &cmd_go_fops);
> +
> +	debugfs_create_file("read_buffer", 0400, d, slave, &read_buffer_fops);
> +	firmware_file = NULL;
> +	debugfs_create_str("firmware_file", 0200, d, &firmware_file);
> +
>  	slave->debugfs = d;
>  }
>  
> -- 
> 2.34.1

-- 
~Vinod

Re: [PATCH 7/7] soundwire: bus: add stream refcount

[Vinod Koul]

On 26-03-24, 09:01, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> The notion of stream is by construction based on a multi-bus
> capability, to allow for aggregation of Peripheral devices or
> functions located on different segments. We currently count how many
> master_rt contexts are used by a stream, but we don't have the dual
> refcount of how many streams are allocated on a given bus. This
> refcount will be useful to check if BTP/BRA streams can be allocated.
> 
> Note that the stream_refcount is modified in sdw_master_rt_alloc() and
> sdw_master_rt_free() which are both called with the bus_lock mutex
> held, so there's no need for refcount_ primitives for additional
> protection.

This lgtm, I would like to see this patch with its user when you
allocate BTP/BRA streams

> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Rander Wang <rander.wang@intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---
>  drivers/soundwire/stream.c    | 5 +++++
>  include/linux/soundwire/sdw.h | 2 ++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index 4e9e7d2a942d..7aa4900dcf31 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -1181,6 +1181,8 @@ static struct sdw_master_runtime
>  	m_rt->bus = bus;
>  	m_rt->stream = stream;
>  
> +	bus->stream_refcount++;
> +
>  	return m_rt;
>  }
>  
> @@ -1217,6 +1219,7 @@ static void sdw_master_rt_free(struct sdw_master_runtime *m_rt,
>  			       struct sdw_stream_runtime *stream)
>  {
>  	struct sdw_slave_runtime *s_rt, *_s_rt;
> +	struct sdw_bus *bus = m_rt->bus;
>  
>  	list_for_each_entry_safe(s_rt, _s_rt, &m_rt->slave_rt_list, m_rt_node) {
>  		sdw_slave_port_free(s_rt->slave, stream);
> @@ -1226,6 +1229,8 @@ static void sdw_master_rt_free(struct sdw_master_runtime *m_rt,
>  	list_del(&m_rt->stream_node);
>  	list_del(&m_rt->bus_node);
>  	kfree(m_rt);
> +
> +	bus->stream_refcount--;
>  }
>  
>  /**
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index e3a4bccc2a7e..71a7031f7b3a 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -902,6 +902,7 @@ struct sdw_master_ops {
>   * meaningful if multi_link is set. If set to 1, hardware-based
>   * synchronization will be used even if a stream only uses a single
>   * SoundWire segment.
> + * @stream_refcount: number of streams currently using this bus
>   */
>  struct sdw_bus {
>  	struct device *dev;
> @@ -931,6 +932,7 @@ struct sdw_bus {
>  	u32 bank_switch_timeout;
>  	bool multi_link;
>  	int hw_sync_min_links;
> +	int stream_refcount;
>  };
>  
>  int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
> -- 
> 2.34.1

-- 
~Vinod

Re: (subset) [PATCH 0/7] soundwire: add BTP/BRA prerequisites

[Vinod Koul]

On Tue, 26 Mar 2024 09:01:15 +0000, Bard Liao wrote:
> Some prerequisites for BTP/BRA.
> 
> Pierre-Louis Bossart (7):
>   soundwire: cadence: fix invalid PDI offset
>   soundwire: cadence: remove PDI offset completely
>   soundwire: remove unused sdw_bus_conf structure
>   soundwire: reconcile dp0_prop and dpn_prop
>   soundwire: clarify maximum allowed address
>   soundwire: debugfs: add interface to read/write commands
>   soundwire: bus: add stream refcount
> 
> [...]

Applied, thanks!

[1/7] soundwire: cadence: fix invalid PDI offset
      commit: 8ee1b439b1540ae543149b15a2a61b9dff937d91
[2/7] soundwire: cadence: remove PDI offset completely
      commit: 1845165fbd6e746c60e8f2e4fc88febd6a195143
[3/7] soundwire: remove unused sdw_bus_conf structure
      commit: 59401c3c08e1a306e29a8d6c826685e2c5c6c794
[5/7] soundwire: clarify maximum allowed address
      commit: bc13cf3f6e63dd708ccd160a28e6bb696af7e9f6

Best regards,
-- 
~Vinod

Re: [PATCH 6/7] soundwire: debugfs: add interface to read/write commands

[Pierre-Louis Bossart]

On 4/5/24 06:45, Vinod Koul wrote:
> On 26-03-24, 09:01, Bard Liao wrote:
>> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>
>> We have an existing debugfs files to read standard registers
>> (DP0/SCP/DPn).
>>
>> This patch provides a more generic interface to ANY set of read/write
>> contiguous registers in a peripheral device. In follow-up patches,
>> this interface will be extended to use BRA transfers.
>>
>> The sequence is to use the following files added under the existing
>> debugsfs directory for each peripheral device:
>>
>> command (write 0, read 1)
>> num_bytes
>> start_address
>> firmware_file (only for writes)
>> read_buffer (only for reads)
>>
>> Example for a read command - this checks the 6 bytes used for
>> enumeration.
>>
>> cd /sys/kernel/debug/soundwire/master-0-0/sdw\:0\:025d\:0711\:01/
>> echo 1 > command
>> echo 6 > num_bytes
>> echo 0x50 > start_address
>> echo 1 > go
> 
> can we have a simpler interface? i am not a big fan of this kind of
> structure for debugging.
> 
> How about two files read_bytes and write_bytes where you read/write
> bytes.
> 
> echo 0x50 6 > read_bytes
> cat read_bytes
> 
> in this format I would like to see addr and values (not need to print
> address /value words (regmap does that too)
> 
> For write
> 
> echo start_addr N byte0 byte 1 ... byte N > write_bytes

I think you missed the required extension where we will add a new
'command_type' to specify which of the regular or BTP/BRA accesses is used.

Also the bytes can come from a firmware file, it would be very odd to
have a command line with 32k value, wouldn't it?

I share your concern about making the interface as simple as possible,
but I don't see how it can be made simpler really. We have to specify
- read/write
- access type (BRA or regular)
- start address
- number of bytes
- a firmware file for writes
- a means to see the read data.

And I personally prefer one 1:1 mapping between setting and debugfs
file, rather than a M:1 mapping that require users to think about the
syntax and which value maps to what setting. At my age I have to define
things that I will remember on the next Monday.

RE: [PATCH 4/7] soundwire: reconcile dp0_prop and dpn_prop

[Liao, Bard]

> -----Original Message-----
> From: Vinod Koul <vkoul@kernel.org>
> Sent: Friday, April 5, 2024 7:33 PM
> To: Bard Liao <yung-chuan.liao@linux.intel.com>
> Cc: linux-sound@vger.kernel.org; linux-kernel@vger.kernel.org; pierre-
> louis.bossart@linux.intel.com; Liao, Bard <bard.liao@intel.com>
> Subject: Re: [PATCH 4/7] soundwire: reconcile dp0_prop and dpn_prop
> 
> On 26-03-24, 09:01, Bard Liao wrote:
> > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> >
> > The definitions for DP0 are missing a set of fields that are required
> > to reuse the same configuration code as DPn.
> >
> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > Reviewed-by: Rander Wang <rander.wang@intel.com>
> > Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> > ---
> >  include/linux/soundwire/sdw.h | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> > index e5d0aa58e7f6..e3a4bccc2a7e 100644
> > --- a/include/linux/soundwire/sdw.h
> > +++ b/include/linux/soundwire/sdw.h
> > @@ -235,7 +235,8 @@ enum sdw_clk_stop_mode {
> >   * @BRA_flow_controlled: Slave implementation results in an OK_NotReady
> >   * response
> >   * @simple_ch_prep_sm: If channel prepare sequence is required
> > - * @imp_def_interrupts: If set, each bit corresponds to support for
> > +  * @ch_prep_timeout: Port-specific timeout value, in milliseconds
> > +  * @imp_def_interrupts: If set, each bit corresponds to support for
> 
> why is this line modified? seems to be same as previous and leading
> space added which might not be correct

Thanks for pointing this out. Sent v2 to fix it.

> 
> 
> >   * implementation-defined interrupts
> >   *
> >   * The wordlengths are specified by Spec as max, min AND number of
> > @@ -249,6 +250,7 @@ struct sdw_dp0_prop {
> >  	u32 *words;
> >  	bool BRA_flow_controlled;
> >  	bool simple_ch_prep_sm;
> > +	u32 ch_prep_timeout;
> >  	bool imp_def_interrupts;
> >  };
> >
> > --
> > 2.34.1
> 
> --
> ~Vinod

Re: [PATCH 6/7] soundwire: debugfs: add interface to read/write commands

[Vinod Koul]

On 05-04-24, 10:12, Pierre-Louis Bossart wrote:
> On 4/5/24 06:45, Vinod Koul wrote:
> > On 26-03-24, 09:01, Bard Liao wrote:
> >> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> >>
> >> We have an existing debugfs files to read standard registers
> >> (DP0/SCP/DPn).
> >>
> >> This patch provides a more generic interface to ANY set of read/write
> >> contiguous registers in a peripheral device. In follow-up patches,
> >> this interface will be extended to use BRA transfers.
> >>
> >> The sequence is to use the following files added under the existing
> >> debugsfs directory for each peripheral device:
> >>
> >> command (write 0, read 1)
> >> num_bytes
> >> start_address
> >> firmware_file (only for writes)
> >> read_buffer (only for reads)
> >>
> >> Example for a read command - this checks the 6 bytes used for
> >> enumeration.
> >>
> >> cd /sys/kernel/debug/soundwire/master-0-0/sdw\:0\:025d\:0711\:01/
> >> echo 1 > command
> >> echo 6 > num_bytes
> >> echo 0x50 > start_address
> >> echo 1 > go
> > 
> > can we have a simpler interface? i am not a big fan of this kind of
> > structure for debugging.
> > 
> > How about two files read_bytes and write_bytes where you read/write
> > bytes.
> > 
> > echo 0x50 6 > read_bytes
> > cat read_bytes
> > 
> > in this format I would like to see addr and values (not need to print
> > address /value words (regmap does that too)
> > 
> > For write
> > 
> > echo start_addr N byte0 byte 1 ... byte N > write_bytes
> 
> I think you missed the required extension where we will add a new
> 'command_type' to specify which of the regular or BTP/BRA accesses is used.
> 
> Also the bytes can come from a firmware file, it would be very odd to
> have a command line with 32k value, wouldn't it?

ofc no one should expect that... it should be written directly from the firmware file

> I share your concern about making the interface as simple as possible,
> but I don't see how it can be made simpler really. We have to specify
> - read/write
> - access type (BRA or regular)
> - start address
> - number of bytes
> - a firmware file for writes
> - a means to see the read data.
> 
> And I personally prefer one 1:1 mapping between setting and debugfs
> file, rather than a M:1 mapping that require users to think about the
> syntax and which value maps to what setting. At my age I have to define
> things that I will remember on the next Monday.

Exactly, you won't remember all the files to write to, my idea was a
simple format or addr, N and data.. a TLV kind of structure..

What would happen if you issue go, but missed writing one of the files.
Also I would expect you to do error checking of inputs...

Thanks
-- 
~Vinod

Re: [PATCH 6/7] soundwire: debugfs: add interface to read/write commands

[Pierre-Louis Bossart]

On 4/11/24 04:28, Vinod Koul wrote:
> On 05-04-24, 10:12, Pierre-Louis Bossart wrote:
>> On 4/5/24 06:45, Vinod Koul wrote:
>>> On 26-03-24, 09:01, Bard Liao wrote:
>>>> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>>>
>>>> We have an existing debugfs files to read standard registers
>>>> (DP0/SCP/DPn).
>>>>
>>>> This patch provides a more generic interface to ANY set of read/write
>>>> contiguous registers in a peripheral device. In follow-up patches,
>>>> this interface will be extended to use BRA transfers.
>>>>
>>>> The sequence is to use the following files added under the existing
>>>> debugsfs directory for each peripheral device:
>>>>
>>>> command (write 0, read 1)
>>>> num_bytes
>>>> start_address
>>>> firmware_file (only for writes)
>>>> read_buffer (only for reads)
>>>>
>>>> Example for a read command - this checks the 6 bytes used for
>>>> enumeration.
>>>>
>>>> cd /sys/kernel/debug/soundwire/master-0-0/sdw\:0\:025d\:0711\:01/
>>>> echo 1 > command
>>>> echo 6 > num_bytes
>>>> echo 0x50 > start_address
>>>> echo 1 > go
>>>
>>> can we have a simpler interface? i am not a big fan of this kind of
>>> structure for debugging.
>>>
>>> How about two files read_bytes and write_bytes where you read/write
>>> bytes.
>>>
>>> echo 0x50 6 > read_bytes
>>> cat read_bytes
>>>
>>> in this format I would like to see addr and values (not need to print
>>> address /value words (regmap does that too)
>>>
>>> For write
>>>
>>> echo start_addr N byte0 byte 1 ... byte N > write_bytes
>>
>> I think you missed the required extension where we will add a new
>> 'command_type' to specify which of the regular or BTP/BRA accesses is used.
>>
>> Also the bytes can come from a firmware file, it would be very odd to
>> have a command line with 32k value, wouldn't it?
> 
> ofc no one should expect that... it should be written directly from the firmware file
> 
>> I share your concern about making the interface as simple as possible,
>> but I don't see how it can be made simpler really. We have to specify
>> - read/write
>> - access type (BRA or regular)
>> - start address
>> - number of bytes
>> - a firmware file for writes
>> - a means to see the read data.
>>
>> And I personally prefer one 1:1 mapping between setting and debugfs
>> file, rather than a M:1 mapping that require users to think about the
>> syntax and which value maps to what setting. At my age I have to define
>> things that I will remember on the next Monday.
> 
> Exactly, you won't remember all the files to write to, my idea was a
> simple format or addr, N and data.. a TLV kind of structure..

So your updated proposal for a write is

echo 1 0 0x50 6 test.bin > write_bytes

Mine was

echo 1 > command
echo 0 > access
echo 0x50 > start_addr
echo 6 > num_bytes
echo test.bin > firmware
echo 1 > go

I find the last version a lot more explicit and self-explanatory. There
is no need to look-up the documentation of the list and order of arguments.
You can argue that there are just three files needed (write command,
read command, read results), but there's more work to parse arguments
and remember the arguments order.

There's also the scripting part. In my tests, I relied on scripts where
I modified one line, it's just easier to visualize than modifying one
argument in the middle of a line.

The last point is extensibility. In the existing BPT/BRA tests, the data
is sent by chunks in each frame. We know that some peripherals cannot
tolerate back-to-back transfers in contiguous frames, that would require
additional arguments to control the flow. If we have to do this with a
list of arguments, it's not straightforward to do without a versioning
scheme.

The risk of getting things wrong is not really a concern, if the
destination or number of bytes is incorrect the command will fail. It
will not cause any issues. It's a debugfs interface to inject commands,
it's expected that people will actually try to make things fail...

Again, this is NOT how the BPT/BRA protocol would be used in a real
product. The integration would rely on the codec driver initiating those
transfers. What this debugfs interface helps with is only for initial
integration tests between a host and peripheral to simulate that the
codec driver will do in the final iteration.

> What would happen if you issue go, but missed writing one of the files.
> Also I would expect you to do error checking of inputs...

Please see the patch, the inputs are checked when possible to avoid
buffer overflows, etc, but as mentioned above it's important to allow
for bad commands to be issued to test the robustness of the driver.
There is e.g. no check on the start_addr, number of bytes, the interface
allows access to any part of the peripheral memory space. That's a
feature, not a bug.