Re: [PATCH 2/2] soundwire: bus: Update sdw_nread/nwrite_no_pm to handle page boundaries

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

On 3/17/23 09:08, Charles Keepax wrote:
> On Thu, Mar 16, 2023 at 01:46:57PM -0500, Pierre-Louis Bossart wrote:
>>
>>
>> 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 will update the kdoc for nread/nwrite to specify that
> transactions that cross a page boundry are not expected to be
> atomic.

Sounds good.

>> 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?
>>
> 
> I think its 16k, but I would be inclined to say this is a
> separate fix. The current code will tie up the bus for longer
> than my fix does, since it only calls sdw_transfer once, and it
> will write the wrong registers whilst doing it. Also to be clear
> this wasn't found with super large transfers just medium sized
> ones that happened to cross a page boundry.
> 
> If we want to add some transaction size capping that is really
> a new feature, this patch a bug fix. I would also be inclined
> to say if we are going to add transaction size capping, we
> probably want some property to specify what we are capping to.

Yes indeed, this would be a new feature. I think this should be a
manager property, depending on which peripherals are integrated and what
latencies are expected.