Re: [RFC PATCH 07/16] soundwire: bus: add API for BPT protocol

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

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...