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.
