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

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

Hi Przemek,

From: Przemyslaw Gaj <pgaj@cadence.com>
Date: Wed, Apr 10, 2019 at 07:53:40

> Hi Vitor,
> 
> The 04/09/2019 15:46, Vitor Soares wrote:
> > 
> > Hi Przemek,
> > 
> > From: Przemyslaw Gaj <pgaj@cadence.com>
> > Date: Tue, Apr 09, 2019 at 16:20:30
> > 
> > > Hi Vitor,
> > > 
> > > The 04/09/2019 14:31, Vitor Soares wrote:
> > > > 
> > > > Hi Przemek,
> > > > 
> > > > From my analyses you do i3c_master_register for secondary master in follow 
> > > cases:
> > > > - HC has already an dynamic address and it is the owner of the bus.
> > > 
> > > Correct. It requests bus ownership before registration, if DA is assigned.
> > > 
> > > > Or 
> > > > - when it receive MR event enabled
> > > 
> > > Actually, when MR event is enabled and secondary master is not initialized 
> > > yet.
> > 
> > I didn't find where you do this verification, can you point me?
> 
> In the driver, cdns_i3c_sec_master_event_up(). I'm requesting mastership and
> registering the master when event is enabled and master is not registered yet.
> 

Sorry, I really don't see that. I assume it is some logic in the 
controller.

> > 
> > > 
> > > > 
> > > > Is this correct?
> > > > 
> > > > From: vitor <soares@synopsys.com>
> > > > Date: Mon, Apr 01, 2019 at 19:41:39
> > > > 
> > > > > Hi Przemek,
> > > > > 
> > > > > > +/**
> > > > > >   * i3c_master_register() - register an I3C master
> > > > > >   * @master: master used to send frames on the bus
> > > > > >   * @parent: the parent device (the one that provides this I3C master
> > > > > >   *	    controller)
> > > > > >   * @ops: the master controller operations
> > > > > > - * @secondary: true if you are registering a secondary master. Will 
> > > return
> > > > > > - *	       -ENOTSUPP if set to true since secondary masters are not yet
> > > > > > - *	       supported
> > > > > > + * @secondary: true if you are registering a secondary master.
> > > > > >   *
> > > > > >   * This function takes care of everything for you:
> > > > > >   *
> > > > > > @@ -2427,10 +2672,6 @@ int i3c_master_register(struct 
> > > i3c_master_controller 
> > > > > *master,
> > > > > >  	struct i2c_dev_boardinfo *i2cbi;
> > > > > >  	int ret;
> > > > > >  
> > > > > > -	/* We do not support secondary masters yet. */
> > > > > > -	if (secondary)
> > > > > > -		return -ENOTSUPP;
> > > > > > -
> > > > > >  	ret = i3c_master_check_ops(ops);
> > > > > >  	if (ret)
> > > > > >  		return ret;
> > > > 
> > > > The bus mode is set before the sec master does 
> > > i3c_master_add_i3c_dev_locked() and
> > > > i3c_master_add_i2c_dev() and after that it isn't updated. This can cause 
> > > troubles for
> > > > HDR-TS modes and when you have i2c devices INDEX(2) on the bus.
> > > 
> > > Yes, I see this now. It may happen before HC driver updates device list. I
> > > should update device list right before i3c_master_register().
> > > 
> > > > 
> > > > > > @@ -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.

> 
> > 
> > > 
> > > > 
> > > > > > +	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.

What if the slave is a secondary master? In your opinion what should be 
the flow?

> 
> > > 
> > > > 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.

Another point is what happen if the current master received MR request 
during this registration?

Best regards,
Vitor Soares

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