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.
> 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.
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...
Regards,
Bjorn
> regards,
> dan carpenter
>
