RE: [PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem

Vitor Soares
2019-04-12T14:28:37Z

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Wed, Apr 10, 2019 at 11:36:03

> On Wed, 10 Apr 2019 10:05:33 +0000
> Vitor Soares <vitor.soares@synopsys.com> wrote:
>
>
> > > > > >
> > > > > > > > @@ -2504,9 +2745,11 @@ int i3c_master_register(struct
> > > > > i3c_master_controller
> > > > > > > *master,
> > > > > > > > * register I3C devices dicovered during the initial DAA.
> > > > > > > > */
> > > > > > > > master->init_done = true;
> > > > > > > > - i3c_bus_normaluse_lock(&master->bus);
> > > > > > > > - i3c_master_register_new_i3c_devs(master);
> > > > > > > > - i3c_bus_normaluse_unlock(&master->bus);
> > > > > > > > + i3c_master_register_new_devs(master);
> > > > > > > > +
> > > > > > > > + i3c_bus_maintenance_lock(&master->bus);
> > > > > > > > + i3c_master_enable_mr_events(master);
> > > > > >
> > > > > > Why are you enabling MR events here? As a standard function this might case
> > > > > issues
> > > > > > because the master can support ENEC/DISEC commands but not multi-master
> > > > > typologies.
> > > > >
> > > > > I enable it at the end of master registration to be sure that current master
> > > > > is
> > > > > ready for bus mastership relinquish. If controller does not support secondary
> > > > > master role it can just do nothing with it.
> > > >
> > > > I think it isn't good idea to have this here because you are forcing HC driver to
> > > > verify it. If it is to be done in subsystem side probably it is better to have a
> > > > master/slave functionalities structure.
> > >
> > > If multi-master topology is not supported, request_mastership() hook won't be
> > > implemented and also you will not implement enable/disable_mr_events() hooks.
> > > You don't have to verify it in the driver.
> >
> > In that case we will need a driver for each role/set of available
> > features and duplicate the code otherwise this need to be checked in HC
> > side.
>
> Not really, just 2 set of ops, one with the hooks set and another one
> with the hooks left unassigned (NULL). But I guess we can also add
> a caps field where we'd list i3c master caps (secondary master,
> supports MR, ...).

Yes we can do this, but what I see is to have several ops structures
based on HC capabilities.

>
> >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > > > + i3c_bus_maintenance_unlock(&master->bus);
> > > > > > > >
> > > > > > > > return 0;
> > > > > > > >
> > > > > > > > @@ -2524,6 +2767,30 @@ int i3c_master_register(struct
> > > > > i3c_master_controller
> > > > > > > *master,
> > > > > > > > EXPORT_SYMBOL_GPL(i3c_master_register);
> > > > > > > >
> > > > > >
> > > > > > I'm thinking if isn't better to instantiate the bus apart and them register
> > > > > the secondary master.
> > > > >
> > > > > It looked like that before but we decided to register everything or nothing.
> > > >
> > > > I see your point, but for the future, slave implementation, we can have a
> > > > function to instantiate just the bus and another for master or slave.
> > > >
> > >
> > > We can go the same way with slave, and register bus and slave at once. If this
> > > is the case.
>
> The slave framework isn't there yet, and I don't think we should expose
> the bus concept to slave drivers anyway. If a master can act both as a
> slave (I mean a slave that can do more than just send MR requests) and
> a master (secondary), the driver should register to both framework.
>
> >
> > What if the slave is a secondary master? In your opinion what should be
> > the flow?
>
> i3c_slave_regiter(&ctrl->slave);
> i3c_master_regiter(&ctrl->master);
>
> The order is arbitrary, and those might actually be called from
> different path if it makes more sense. I'm just pointing out that
> registering a slave and registering a master are 2 different things,
> and the master side of the framework should probably not automate slave
> registration, at least not until we have a better idea of what the
> slave API will look like.

We need to understand better your point.

The secondary master can have both roles and even not implementing all
slave function it is a slave when is not a master.
For me still keep the idea:

bus
/ \
/ \
slave master

and just one role is active at time.

I didn't get why the bus shouldn't be instantiated for slave. Can you
explain?

In any case we will need a common point to switch the roles.

>
> >
> > >
> > > > >
> > > > > > In this way you are able to add DEFSLVS even before the HC has enable MR
> > > > > events like it is done
> > > > > > with dt devices.
> > > > >
> > > > > I don't get it. What do you mean "add DEFSLVS"? DEFSLVS should be received
> > > > > from
> > > > > current master right after ENTDAA. Could you please explain?
> > > >
> > > > So the current master receive an hot join and send the DEFSLVS, when do you
> > > > add them to the bus? Will you go for the all process to get all dev info and so on?
> > > >
> > >
> > > When current master receives an hot-join, do ENTDAA to enumerate device which
> > > joined the bus and then sends DEFSLVS. This data is stored in secondary master
> > > controller and if secondary master can request mastership, collects all
> > > required informations and adds the devices. It has to collect PID, DEFSLVS does
> > > not provide this information.
> >
> > I see now. You need to take care here because when the secondary master
> > add the i3c_dev it might change the address.
> > This is one of the reasons I would prefer a dedicated function to add the
> > DEFSLVS.
>
> IIRC, Cadence IP is parsing DEFSLVS in HW, and the device table is
> automatically filled based on that. We should only add the DEFSLVS
> parsing helper once we start seeing a need for it (probably when adding
> secondary slave support since you seem to insist on this aspect :-)).

For me the subsystem should hold and handle all this information. Anyway,
this information is received and needed before the mastership takeover.
Basically I want to avoid to put this type of logistics in HC driver
layer and call the maintenance_lock.

I'm insisting because it seems that I have a different use case to
address and I don't see how this fit on it.

>
> >
> > Another point is what happen if the current master received MR request
> > during this registration?
>
> You mean when registering the primary I3C master? The driver should
> take care of disabling MR interrupts and if possible reject all
> incoming MR. MR should only be re-enabled when ->enable_mr_events() is
> called by the core.

I'm aware of that I just didn't see any disable events.

Best regards,
Vitor Soares