Message-ID: <447cac77-4cc7-b2a3-23e7-978e1641a401@linux.intel.com>
Date:   Thu, 16 Mar 2023 13:46:57 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.8.0
Subject: Re: [PATCH 2/2] soundwire: bus: Update sdw_nread/nwrite_no_pm to
 handle page boundaries
Content-Language: en-US
To:     Charles Keepax <ckeepax@opensource.cirrus.com>, vkoul@kernel.org
Cc:     yung-chuan.liao@linux.intel.com, sanyog.r.kale@intel.com,
        alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
        patches@opensource.cirrus.com
References: <20230316155734.3191577-1-ckeepax@opensource.cirrus.com>
 <20230316155734.3191577-2-ckeepax@opensource.cirrus.com>
From:   Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
In-Reply-To: <20230316155734.3191577-2-ckeepax@opensource.cirrus.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


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@opensource.cirrus.com>
> ---
>  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);
>