Hi Bjorn,
On 10/1/2024 8:09 AM, Bjorn Andersson wrote:
On Mon, Sep 30, 2024 at 11:21:23AM GMT, Dan Carpenter wrote:
On Sun, Sep 29, 2024 at 10:46:37PM -0500, Bjorn Andersson wrote:
@@ -602,6 +603,7 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
peripheral.clk_div = itr->clk_div;
peripheral.set_config = 1;
peripheral.multi_msg = false;
+ peripheral.shared_se = gi2c->se.shared_geni_se;
for (i = 0; i < num; i++) {
gi2c->cur = &msgs[i];
@@ -612,6 +614,8 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
if (i < num - 1)
peripheral.stretch = 1;
+ peripheral.first_msg = (i == 0);
+ peripheral.last_msg = (i == num - 1);
There are multiple error paths in this loop, which would result in us
never issuing the unlock TRE - effectively blocking other subsystems
from accessing the serial engine until we perform our next access
(assuming that APSS issuing a lock TRE when APSS already has the channel
locked isn't a problem?)
Hi Bjorn,
I saw the words "error paths" and "unlock" and I thought there was maybe
something we could do here with static analysis.
Appreciate you picking up on those topics :)
But I don't know what TRE or APSS mean.
The "APSS" is "the application processor sub system", which is where
we typically find Linux running. TRE is, if I understand correctly, a
request on the DMA engine queue.
Yes, Thats right. Transfer ring element, it's like a command to the
engine. Can be configuration TRE, DMA xfer request TRE etc.
The one thing I do see is that this uses "one err" style error handling where
there is one err label and it calls dmaengine_terminate_sync(gi2c->rx_c)
regardless of whether or not geni_i2c_gpi() was called or failed/succeeded.
The scheme presented in this series injects a "LOCK" request before the
DMA request marked first_msg, and an "UNLOCK" after the ones marked
last_msg. This is needed because the controller is also concurrently by
some DSP (or similar), so the LOCK/UNLOCK scheme forms mutual exclusion
of the operations between the Linux and DSP systems.
I'm not sure if it's possible to tie the unlock operation to
dmaengine_terminate_sync() in some way.
I think terminate_sync() should clean up all xfers and will continue for
the next request as a fresh start.
Giving this some more thought, it feels like the current scheme serves
the purpose of providing mutual exclusion both for the controller and
to some degree for the device. But I'd like to understand why we can't
inject the lock/unlock implicitly for each transfer...
Explicitly adding lock/unlock per transfer adds execution load. Lock is
meant for taking an ownership of the bus which is better to manage per
session.
Regards,
Bjorn
regards,
dan carpenter
