Message-ID: <2ca6aa55-c367-8d5e-702a-9fb1a518310a@linux.intel.com>
Date:   Fri, 17 Mar 2023 09:29:53 -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>
Cc:     vkoul@kernel.org, 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>
 <447cac77-4cc7-b2a3-23e7-978e1641a401@linux.intel.com>
 <20230317140807.GI68926@ediswmail.ad.cirrus.com>
From:   Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
In-Reply-To: <20230317140807.GI68926@ediswmail.ad.cirrus.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


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.