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
From: Pierre-Louis Bossart <[email protected]>
This offset is set to exactly zero and serves no purpose. Remove.
Signed-off-by: Pierre-Louis Bossart <[email protected]>
Reviewed-by: Rander Wang <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
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
From: Pierre-Louis Bossart <[email protected]>
This is redundant with sdw_bus_params, and was never used.
Signed-off-by: Pierre-Louis Bossart <[email protected]>
Reviewed-by: Rander Wang <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
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
From: Pierre-Louis Bossart <[email protected]>
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 <[email protected]>
Reviewed-by: Rander Wang <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
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
From: Pierre-Louis Bossart <[email protected]>
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 <[email protected]>
Reviewed-by: Rander Wang <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
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
From: Pierre-Louis Bossart <[email protected]>
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 <[email protected]>
Reviewed-by: Rander Wang <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
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
From: Pierre-Louis Bossart <[email protected]>
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 <[email protected]>
Reviewed-by: Rander Wang <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
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
From: Pierre-Louis Bossart <[email protected]>
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 <[email protected]>
Reviewed-by: Rander Wang <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
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
On 26-03-24, 09:01, Bard Liao wrote:
> From: Pierre-Louis Bossart <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: Rander Wang <[email protected]>
> Signed-off-by: Bard Liao <[email protected]>
> ---
> 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
On 26-03-24, 09:01, Bard Liao wrote:
> From: Pierre-Louis Bossart <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: Rander Wang <[email protected]>
> Signed-off-by: Bard Liao <[email protected]>
> ---
> 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
On 26-03-24, 09:01, Bard Liao wrote:
> From: Pierre-Louis Bossart <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: Rander Wang <[email protected]>
> Signed-off-by: Bard Liao <[email protected]>
> ---
> 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
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
On 4/5/24 06:45, Vinod Koul wrote:
> On 26-03-24, 09:01, Bard Liao wrote:
>> From: Pierre-Louis Bossart <[email protected]>
>>
>> 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.
> -----Original Message-----
> From: Vinod Koul <[email protected]>
> Sent: Friday, April 5, 2024 7:33 PM
> To: Bard Liao <[email protected]>
> Cc: [email protected]; [email protected]; pierre-
> [email protected]; Liao, Bard <[email protected]>
> 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 <[email protected]>
> >
> > 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 <[email protected]>
> > Reviewed-by: Rander Wang <[email protected]>
> > Signed-off-by: Bard Liao <[email protected]>
> > ---
> > 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
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 <[email protected]>
> >>
> >> 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
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 <[email protected]>
>>>>
>>>> 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.