On 12/12/23 06:19, Charles Keepax wrote:
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.
indeed, that's a left-over from an earlier trial. Will fix.
+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?
It's not necessary in my current solution, but I opted to keep the arguments aligned.
+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.
it's needed for the Intel solution, the open() will allocate the required DMA buffers and copy the data from the messsage into the DMA buffers with the required formatting expected by the IP.
An alternative would be to simplify open/close but then we have to add a hw_params/prepare and hw_free steps. I didn't see a need for such a split, unlike for audio the arguments are not going to be variable.
But yes it's a good question, what exactly is an open/startup callback supposed to do in ALSA/ASoC?
- 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.
As discussed earlier, we *could* move this to host-specific parts, but then the codec driver would need to know about host alignment requirements. One of those 'be careful what you ask for' things where you may have more work to do on the codec driver side...
+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.
The error check is a big open, we could assume that all the previous steps were done the right way and skip tests, or we would add a set of paranoia checks.
The primary intent of those checks is to help the integration of codec drivers, it's better to get an error code and a clear error message than a kernel oops because the expected sequence is not followed. Once the integration is done, those tests are probably not very useful indeed.
+/**
- 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?
Ah, this is a reference to the 16 bit address limitation in the regular read/write commands.
+/*
- 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 think it's really better to have a clear design intent that BRA is not meant for small transfers. It would be opening a can of worms if we start seeing uses of this protocol beyond the intended firmware/table download and history buffer retrieval.
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.
To be clear, I would like to allow BRA in parallel with normal writes *to a different set of registers* to deal with alerts, etc.
A set of registers only accessible through BRA is a very questionable design indeed. That's not what the SoundWire spec intended and it's not what this patchset had in mind. You'll have to tell us more on what exactly the hardware is and does, and how strongly you want this capability supported...