Re: [PATCH 1/2] i3c: Add support for mastership request to I3C subsystem

[Boris Brezillon <bbrezillon@kernel.org>]

Hi Przemek,

On Fri, 14 Dec 2018 07:14:44 +0000
Przemyslaw Gaj <pgaj@cadence.com> wrote:

>     > Mastership request occurs in the following cases:
>     > - Mastership is requested automatically after secondary master
>     > receives mastership ENEC event. This allows secondary masters to
>     > initialize their bus  
>     
>     I guess this only happens when the secondary master received a DEFSLVS
>     packet, right?
> 
> Actually, this happens when current master enables MR event. 
> He should take care of providing devices list.
> Of course, we can consider adding a flag which indicates if slaves are already defined or not.

i3c_master_add_i3c_dev_locked() already handles this "device is already
known/registered" case, so that shouldn't be a problem.

>     >  static const char * const i3c_bus_mode_strings[] = {
>     >  	[I3C_BUS_MODE_PURE] = "pure",
>     >  	[I3C_BUS_MODE_MIXED_FAST] = "mixed-fast",
>     > @@ -491,8 +500,15 @@ static ssize_t current_master_show(struct device *dev,
>     >  				   char *buf)
>     >  {
>     >  	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
>     > +	struct i3c_master_controller *master;
>     >  	ssize_t ret;
>     >  
>     > +	if (!i3cbus->cur_master) {
>     > +		master = container_of(i3cbus, struct i3c_master_controller, bus);
>     > +		if (i3c_master_request_mastership(master))
>     > +			return sprintf(buf, "unknown\n");
>     > +	}  
>     
>     Hm, why is that needed? When you are a secondary master ->cur_master
>     should be set to the i3c_dev_desc representing the main master at init
>     time.
> 
> I do not add devices at initialization time, so secondary master does not have i3c_dev_desc object describing main master.
> DEFSLVS is providing us incomplete information, we don’t have PID yet. PID is retrieved from device when secondary master adds device. 
> This is possible only if current master is ready to relinquish bus control.
> 
> I decided to set cur_master after secondary master completely initialized his bus. What do you think?

Maybe we should not initialize the bus until we have DEFSLVS
information then.

>     
>     >  		return -EINVAL;
>     >  
>     >  	if (master->this)
>     > @@ -1569,7 +1628,8 @@ int i3c_master_set_info(struct i3c_master_controller *master,
>     >  		return PTR_ERR(i3cdev);
>     >  
>     >  	master->this = i3cdev;
>     > -	master->bus.cur_master = master->this;
>     > +	if (master->op_mode == I3C_MASTER_MODE)
>     > +		master->bus.cur_master = master->this;  
>     
>     When you hare a secondary master, you should make ->cur_master point to
>     an i3c_dev_desc describing the current/main master. Should be possible
>     thanks to the DEFSLVS info.
> 
> As I said before it's not so easy with DEFSLVS. 
> We can consider creating device list without full information. Do you agree?

We can just leave ->cur_master to NULL at first and have the following
function:

static bool i3c_master_owns_bus(struct i3c_master_controller *master)
{
	return master->bus->cur_master == master->this;
}

Regarding the proposal to have a partially discovered device list, why
not, depends what we want to do with that.

>     
>     >  
>     >  	ret = i3c_master_attach_i3c_dev(master, i3cdev);
>     >  	if (ret)
>     > @@ -1727,6 +1787,12 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
>     >  	}
>     >  
>     >  	/*
>     > +	 * Don't reset addresses if this is secondary master.
>     > +	 * Secondary masters can't do DAA.
>     > +	 */  
>     
>     Hm, I'm not sure how that's supposed to work. What if the secondary
>     master was initialized by the bootloader. Don't you need to reset
>     something and maybe trigger a MSTREQ+DAA or a HotJoin?
> 
> Even if secondary master does MSTREQ, he can't do DAA. He can't manage the bus, only main master can do that.
> I'll point this information in spec below.

Thanks.

> 
> I'm wondering about HotJoin. I can't find this use case in my mind.
>     > +	/*
>     > +	 * We can register I3C devices received from master by DEFSLVS.
>     > +	 */
>     > +	master->init_done = true;  
>     
>     I think this should be set before that, in the i3c_master_register()
>     function. When a secondary master is initialized, it should populate
>     the dev list based on a previous DEFSLVS frame it has received before
>     the ->probe() call, or just start with an empty list.
> 
> Same here. I do not populate list of devices until I'm able to get full information from devices.

Correct. But ->init_done should be set before that IMO, unless we
decide to defer bus initialization after DEFSLVS/ENEC(MR).

>     
>     > +	i3c_bus_normaluse_lock(&master->bus);
>     > +	i3c_master_register_new_i3c_devs(master);
>     > +	i3c_bus_normaluse_unlock(&master->bus);
>     > +}
>     > +
>     >  /**
>     >   * 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:
>     >   *
>     > @@ -2424,10 +2559,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;
>     > @@ -2439,6 +2570,7 @@ int i3c_master_register(struct i3c_master_controller *master,
>     >  	master->dev.release = i3c_masterdev_release;
>     >  	master->ops = ops;
>     >  	master->secondary = secondary;
>     > +	master->op_mode = secondary ? I3C_SLAVE_MODE : I3C_MASTER_MODE;
>     >  	INIT_LIST_HEAD(&master->boardinfo.i2c);
>     >  	INIT_LIST_HEAD(&master->boardinfo.i3c);
>     >  
>     > @@ -2497,6 +2629,13 @@ int i3c_master_register(struct i3c_master_controller *master,
>     >  		goto err_del_dev;
>     >  
>     >  	/*
>     > +	 * We can stop here. Devices will be attached after bus takeover.
>     > +	 * Secondary masters can't do DAA.
>     > +	 */
>     > +	if (master->secondary)
>     > +		return 0;  
>     
>     See, I don't think we should stop here. We should provide a hook to let
>     secondary slaves populate the dev list based on data they might have
>     received before that.
> 
> Do we want to populate list with data received by DEFSLVS? 

I guess we do. I mean, we shouldn't populate the list directly, but
we should request bus ownership, and then add I3C devices one by one
using i3c_master_add_i3c_dev_locked(). This function will do the rest
(query PID, BCR, DCR, ... and then add the device to the list).

> If yes, I can populate list but we are not able to register devices because we don’t have 
> PID yet and device drivers are matched by PID.

Now I understand why you want it to be driven by ENEC(MR). Can't we
have a situation where i3c_master_register() is called after it has
received both DEFSLV and ENEC(MR). Shouldn't we request bus ownership
directly at init time in this case.

Looks like all of this will be I3C master controller driven though, so
maybe it's just better to let master drivers handle all of that on
their own.

>     > +	i3c_bus_maintenance_lock(&master->bus);
>     > +	ret = i3c_master_get_accmst_locked(master, info);
>     > +	i3c_bus_maintenance_unlock(&master->bus);
>     > +	if (ret)
>     > +		return ret;  
>     
>     Hm, will this really work? I thought the mastership handover procedure
>     had to be done atomically (using Sr between each transfer to avoid
>     external interruptions). I might be wrong though (need to read the
>     spec to refresh my memory).
> 
> There wouldn't be interruption. Secondary master may ack or nack this command.
> If he acks, it's done, he became current master.
> If he nacks or transmits incorrect address, he will not acquire mastership.

Yep, I realized that afterwards, when reading the spec again.

>     
>     > +
>     > +	return 0;
>     > +}
>     > +EXPORT_SYMBOL_GPL(i3c_master_mastership_ack);
>     > +
>     > +static void i3c_secondary_master_bus_init(struct work_struct *work)
>     > +{
>     > +	struct i3c_master_controller *master;
>     > +
>     > +	master = container_of(work, struct i3c_master_controller, mastership);
>     > +
>     > +	if (i3c_master_request_mastership(master))
>     > +		dev_err(&master->dev, "Mastership failed.");
>     > +}
>     > +
>     > +/**
>     > + * i3c_secondary_master_events_enabled() - event from current master
>     > + * @master: master used to send frames on the bus
>     > + * @evts: enabled events
>     > + *
>     > + * This function allows to perform required operations after current
>     > + * master enables particular events on the bus.
>     > + */
>     > +void i3c_secondary_master_events_enabled(struct i3c_master_controller *master,
>     > +					 u8 evts)
>     > +{
>     > +	if ((evts & I3C_CCC_EVENT_MR) &&
>     > +	    !master->init_done) {
>     > +		INIT_WORK(&master->mastership, i3c_secondary_master_bus_init);
>     > +		queue_work(master->wq, &master->mastership);
>     > +	}
>     > +}
>     > +EXPORT_SYMBOL_GPL(i3c_secondary_master_events_enabled);  
>     
>     Hm, so you're trying to standardize events handling. I had initially
>     left that to the driver as it's likely to be controller specific.
> 
> I think that I3C spec describes common set of events.

Yes, and those common events are already defined in ccc.h. What I meant
is that handling MR or HotJoin events is likely to be controller
specific, not sure we need to define an mr_work at the
i3c_master_controller level, we can just let I3C drivers define their
own work if they need one (like I did to support HJ in the Cadence
driver).

>     > +/**
>     >   * struct i3c_bus - I3C bus object
>     >   * @cur_master: I3C master currently driving the bus. Since I3C is multi-master
>     >   *		this can change over the time. Will be used to let a master
>     > @@ -418,6 +428,20 @@ struct i3c_bus {
>     >   *		      for a future IBI
>     >   *		      This method is mandatory only if ->request_ibi is not
>     >   *		      NULL.
>     > + * @update_devs: updates device list. Called after bus takeover. Secondary
>     > + *               master can't perform DAA procedure.  
>     
>     Is that really true? Can you point me to the relevant section in the
>     spec describing this constraint?
> 
> Sure, I3C Devices Roles vs Responsibilities, Table in spec. Dynamic Address
> Assignment, Secondary master is not capable to do that.

Thanks for pointing this out, I didn't notice.

> 
> Please also take a look at Hot-Join Dynamic Address Assignment. Some of controllers may
> have HJ-DAA implemented in the hardware, maybe it's better to send DEFSLVS right before
> GETACCMST?

It's the responsibility of the master doing a DAA to send a DEFSLVS
frame if there are new secondary masters on the bus. So DEFSLVS should
be sent just after the HJ-DAA in this case, not before GETACCMST.

Regards,

Boris

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