On 3/16/23 10:57, Charles Keepax wrote:
> Currently issuing a sdw_nread/nwrite_no_pm across a page boundary
> will silently fail to write correctly as nothing updates the page
> registers, meaning the same page of the chip will get rewritten
> with each successive page of data.
>
> As the sdw_msg structure contains page information it seems
> reasonable that a single sdw_msg should always be within one
> page. It is also mostly simpler to handle the paging at the
> bus level rather than each master having to handle it in their
> xfer_msg callback.
>
> As such add handling to the bus code to split up a transfer into
> multiple sdw_msg's when they go across page boundaries.
This sounds good but we need to clarify that the multiple sdw_msg's will
not necessarily be sent one after the other, the msg_lock is held in the
sdw_transfer() function, so there should be no expectation that e.g. one
big chunk of firmware code can be sent without interruption.
I also wonder if we should have a lower bar than the page to avoid
hogging the bus with large read/write transactions. If there are
multiple devices on the same link and one of them signals an alert
status while a large transfer is on-going, the alert handling will
mechanically be delayed by up to a page - that's 32k reads/writes, isn't it?
> Signed-off-by: Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>
> ---
> drivers/soundwire/bus.c | 47 +++++++++++++++++++++++------------------
> 1 file changed, 26 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index 3c67266f94834..bdd251e871694 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -386,37 +386,42 @@ int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,
> * Read/Write IO functions.
> */
>
> -int sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
> +static int sdw_ntransfer_no_pm(struct sdw_slave *slave, u32 addr, u8 flags,
> + size_t count, u8 *val)
> {
> struct sdw_msg msg;
> + size_t size;
> int ret;
>
> - ret = sdw_fill_msg(&msg, slave, addr, count,
> - slave->dev_num, SDW_MSG_FLAG_READ, val);
> - if (ret < 0)
> - return ret;
> + while (count) {
> + // Only handle bytes up to next page boundary
> + size = min(count, (SDW_REGADDR + 1) - (addr & SDW_REGADDR));
>
> - ret = sdw_transfer(slave->bus, &msg);
> - if (slave->is_mockup_device)
> - ret = 0;
> - return ret;
> + ret = sdw_fill_msg(&msg, slave, addr, size, slave->dev_num, flags, val);
> + if (ret < 0)
> + return ret;
> +
> + ret = sdw_transfer(slave->bus, &msg);
> + if (ret < 0 && !slave->is_mockup_device)
> + return ret;
> +
> + addr += size;
> + val += size;
> + count -= size;
> + }
> +
> + return 0;
> +}
> +
> +int sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
> +{
> + return sdw_ntransfer_no_pm(slave, addr, SDW_MSG_FLAG_READ, count, val);
> }
> EXPORT_SYMBOL(sdw_nread_no_pm);
>
> int sdw_nwrite_no_pm(struct sdw_slave *slave, u32 addr, size_t count, const u8 *val)
> {
> - struct sdw_msg msg;
> - int ret;
> -
> - ret = sdw_fill_msg(&msg, slave, addr, count,
> - slave->dev_num, SDW_MSG_FLAG_WRITE, (u8 *)val);
> - if (ret < 0)
> - return ret;
> -
> - ret = sdw_transfer(slave->bus, &msg);
> - if (slave->is_mockup_device)
> - ret = 0;
> - return ret;
> + return sdw_ntransfer_no_pm(slave, addr, SDW_MSG_FLAG_WRITE, count, (u8 *)val);
> }
> EXPORT_SYMBOL(sdw_nwrite_no_pm);
>
