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
