On Thu, Dec 07, 2023 at 04:29:35PM -0600, Pierre-Louis Bossart wrote:
This patch suggests a minimum bound of 64 bytes, for smaller transfers
128 in the code.
+int sdw_bpt_close_stream(struct sdw_bus *bus,
struct sdw_slave *slave,enum sdw_bpt_type mode,struct sdw_bpt_msg *msg)+{
Seems weird to pass the message to close?
+int sdw_bpt_open_stream(struct sdw_bus *bus,
struct sdw_slave *slave,enum sdw_bpt_type mode,struct sdw_bpt_msg *msg)
Ditto, here does it make sense to pass the msg to open? I guess the idea is that one only sends a single message in one open->send->wait->close cycle? Would be much nicer if multiple send/waits could be done in a single open/close, or if the limitation is unavoidable why split out open/send into separate calls at all? Just have send and wait and wrap open/close into them.
- if (msg->len % SDW_BPT_MSG_BYTE_ALIGNMENT) {
dev_err(bus->dev, "BPT message length %d is not a multiple of %d bytes\n",msg->len, SDW_BPT_MSG_BYTE_ALIGNMENT);return -EINVAL;- }
Should this really be here? My understanding is this alignment is a limitation of specific hardware so should this check not be in the Cadence master code.
+int sdw_bpt_send_async(struct sdw_bus *bus,
struct sdw_slave *slave,struct sdw_bpt_msg *msg)+{
- if (msg->len > SDW_BPT_MSG_MAX_BYTES)
return -EINVAL;
Does this check make sense since this was already checked in open? I guess the user could pass in a different message at this stage, but that I guess is part of what feels weird about passing the message into open.
+/**
- struct sdw_btp_msg - Message structure
- @addr: Start Register address accessed in the Slave
- @len: number of bytes to transfer. More than 64Kb can be transferred
- but a practical limit of SDW_BPT_MSG_MAX_BYTES is enforced.
Where is the 64kb coming from here?
+/*
- Add a minimum number of bytes for BPT transfer sizes. BPT has a lot of
- overhead, enabling/disabling a stream costs 6 write commands, plus the bank
- switch that could be delayed in time. In addition, transferring very small
- data sets over DMA is known to be problematic on multiple platforms.
- */
+#define SDW_BPT_MSG_MIN_BYTES 128
Is it really necessary for the core to enforce a minimum transfer size (well except for the required alignment thing)? Yes small transfers don't obviously make sense, but there is nothing inherently wrong with doing one, which makes me feel it is excessive for the core to block more than it has to here.
I would be of the opinion its up to driver writers if they have some reason to do small BRA transfers. Firstly, since we are so keen on allowing BRA and normal writes to overlap, one could see use-cases when you want to do something through BRA such that it doesn't block the normal command stream. Also there is already 1 feature on cs42l43 that can only be accessed through BRA, yes that is a heroically questionable hardware design choice. Whilst we are mostly ignoring that for now, I can see this being something some other hardware teams decide to heroically do at some point as well.
Thanks, Charles