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

[Vitor Soares <vitor.soares@synopsys.com>]

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

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c