| Message ID | 20240326090122.1051806-7-yung-chuan.liao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: add BTP/BRA prerequisites | expand |
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
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.
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
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.
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; }