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

[Przemyslaw Gaj <pgaj@cadence.com>]

Hi Boris,

> On Dec 14, 2018, at 9:24 AM, Boris Brezillon <bbrezillon@kernel.org> wrote:
> 
> EXTERNAL MAIL
> 
> 
> 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.

I meant defined by DEFSLVS command when I wrote "slaves are already defined".
I know i3c_master_add_i3c_dev_locked() handles such cases. This flag could 
indicate that we already got DEFSLVS and then (after ENEC(MR)) we could check 
if slaves were defined and finish bus initialization.

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

It is good idea. We can use DEFSLVS instead of ENEC(MR) event as a trigger
for secondary master bus initialization or both. I assumed that main master will 
not send ENEC(MR) without sending DEFSLVS and ENEC(MR) will let secondary 
master know that not only slave list is defined, but also he can request mastership.

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

Yes, we need decision to know which path to choose :)

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

I think it's possible and I think we should request mastership and finish
initialization in this case. And what if MR is disabled during initialization?
Do we want to wait for DEFSLVS or ENEC(MR) event and then finish
initialization? I just wanted to have generic solution for both cases.

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

So, I can try adding devices received by DEFSLVS on driver side and
then call i3c_master_register(). Is that what you meant? I'm sorry if I
misunderstood something.

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

Ok, I'll think about it.

> 
>>> +/**
>>>  * 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

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