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

Commit Message

Przemysław Gaj Dec. 13, 2018, 12:28 p.m. UTC

This patch adds support for mastership request to I3C subsystem.

Mastership event is enabled for all the masters on the I3C bus
by default.

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
- If above procedure fails for some reason, user can force
mastership request through sysfs. Of course, mastership event
still has to be enabled on current master side
- Mastership is also requested automatically when device driver
tries to transfer data using master controller in slave mode.

There is still some limitation:
- I2C devices are not registered on secondary master side. I2C
devices received in DEFSLVS CCC command are added to device list
just to send back this information to the other master controllers
in case of bus handoff. We can add support for this in the future
if there is such use case.

Signed-off-by: Przemyslaw Gaj <[e-mail]>
---
 drivers/i3c/master.c       | 260 ++++++++++++++++++++++++++++++++++++++++++---
 include/linux/i3c/master.h |  43 +++++++-
 2 files changed, 289 insertions(+), 14 deletions(-)

Comments

Boris Brezillon Dec. 13, 2018, 1:50 p.m. UTC | #1

On Thu, 13 Dec 2018 12:28:00 +0000
Przemyslaw Gaj <[e-mail]> wrote:

> This patch adds support for mastership request to I3C subsystem.
> 
> Mastership event is enabled for all the masters on the I3C bus
> by default.
> 
> 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?

> - If above procedure fails for some reason, user can force
> mastership request through sysfs. Of course, mastership event
> still has to be enabled on current master side

Sounds like a good idea.

> - Mastership is also requested automatically when device driver
> tries to transfer data using master controller in slave mode.

And that too.

> 
> There is still some limitation:
> - I2C devices are not registered on secondary master side. I2C
> devices received in DEFSLVS CCC command are added to device list
> just to send back this information to the other master controllers
> in case of bus handoff. We can add support for this in the future
> if there is such use case.

I2C devices must be described under the bus represented by the
secondary master for that to work properly (we need a way to bind
devices to drivers). So, it's probably a good thing to discard (or
leave them inactive) any devices that do not have a i2c_board_info entry
on the secondary master side.

> 
> Signed-off-by: Przemyslaw Gaj <[e-mail]>
> ---
>  drivers/i3c/master.c       | 260 ++++++++++++++++++++++++++++++++++++++++++---
>  include/linux/i3c/master.h |  43 +++++++-
>  2 files changed, 289 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 68d6553..e98b600 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -460,6 +460,15 @@ static int i3c_bus_init(struct i3c_bus *i3cbus)
>  	return 0;
>  }
>  
> +static int i3c_master_request_mastership(struct i3c_master_controller *master)
> +{
> +	if (!master->ops->request_mastership)
> +		return -ENOTSUPP;

Please add a blank line here

> +	if (master->ops->request_mastership(master))
> +		return -EIO;

and here.

> +	return 0;
> +}
> +
>  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.

> +
>  	i3c_bus_normaluse_lock(i3cbus);
>  	ret = sprintf(buf, "%d-%llx\n", i3cbus->id,
>  		      i3cbus->cur_master->info.pid);
> @@ -659,6 +675,11 @@ static int i3c_master_send_ccc_cmd_locked(struct i3c_master_controller *master,
>  	if (!cmd || !master)
>  		return -EINVAL;
>  
> +	if (master->op_mode == I3C_SLAVE_MODE) {
> +		if (i3c_master_request_mastership(master))
> +			return -EIO;
> +	}
> +

This shouldn't be needed either. if i3cbus->cur_master != master->this,
that means the master is operating in slave mode.

>  	if (WARN_ON(master->init_done &&
>  		    !rwsem_is_locked(&master->bus.lock)))
>  		return -EINVAL;
> @@ -1371,6 +1392,7 @@ static int i3c_master_reattach_i3c_dev(struct i3c_dev_desc *dev,
>  						      dev->info.dyn_addr);
>  		if (status != I3C_ADDR_SLOT_FREE)
>  			return -EBUSY;
> +

Drop this change (or make it part of a separate patch fixing coding
style issues).

>  		i3c_bus_set_addr_slot_status(&master->bus,
>  					     dev->info.dyn_addr,
>  					     I3C_ADDR_SLOT_I3C_DEV);
> @@ -1491,6 +1513,46 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
>  }
>  
>  /**
> + * i3c_master_get_accmst_locked() - send a GETACCMST CCC command
> + * @master: master used to send frames on the bus
> + * @info: I3C device information

Maybe you can simply pass the dynamic address instead of the full
device_info struct.

> + *
> + * Send a GETACCMST CCC command.
> + *
> + * This should be called if the curent master acknowledges bus takeover.

I really thought this would be all automated by the master IP, but
apparently it's not :-).

> + *
> + * This function must be called with the bus lock held in write mode.
> + *
> + * Return: 0 in case of success, a positive I3C error code if the error is
> + * one of the official Mx error codes, and a negative error code otherwise.
> + */
> +int i3c_master_get_accmst_locked(struct i3c_master_controller *master,
> +				 const struct i3c_device_info *info)
> +{
> +	struct i3c_ccc_getaccmst *accmst;
> +	struct i3c_ccc_cmd_dest dest;
> +	struct i3c_ccc_cmd cmd;
> +	int ret;
> +
> +	accmst = i3c_ccc_cmd_dest_init(&dest, info->dyn_addr, sizeof(*accmst));
> +	if (!accmst)
> +		return -ENOMEM;
> +
> +	i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETACCMST, &dest, 1);
> +
> +	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
> +
> +	if (ret)
> +		return ret;
> +
> +	if (dest.payload.len != sizeof(*accmst))
> +		return -EIO;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_get_accmst_locked);
> +
> +/**
>   * i3c_master_do_daa() - do a DAA (Dynamic Address Assignment)
>   * @master: master doing the DAA
>   *
> @@ -1554,11 +1616,8 @@ int i3c_master_set_info(struct i3c_master_controller *master,
>  	struct i3c_dev_desc *i3cdev;
>  	int ret;
>  
> -	if (!i3c_bus_dev_addr_is_avail(&master->bus, info->dyn_addr))
> -		return -EINVAL;
> -
> -	if (I3C_BCR_DEVICE_ROLE(info->bcr) == I3C_BCR_I3C_MASTER &&
> -	    master->secondary)
> +	if (master->op_mode == I3C_MASTER_MODE &&
> +	    !i3c_bus_dev_addr_is_avail(&master->bus, info->dyn_addr))

Please place this test in a separate if branch, as they are not really
related.

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

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

> +	if (master->secondary)
> +		return 0;

Add a blank like here.

> +	/*
>  	 * Reset all dynamic address that may have been assigned before
>  	 * (assigned by the bootloader for example).
>  	 */
> @@ -1738,6 +1804,7 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
>  	ret = i3c_master_disec_locked(master, I3C_BROADCAST_ADDR,
>  				      I3C_CCC_EVENT_SIR | I3C_CCC_EVENT_MR |
>  				      I3C_CCC_EVENT_HJ);
> +

Drop this blank line.

>  	if (ret && ret != I3C_ERROR_M2)
>  		goto err_bus_cleanup;
>  
> @@ -1789,6 +1856,44 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
>  	return NULL;
>  }
>  
> +int i3c_master_add_i2c_dev(struct i3c_master_controller *master,
> +			   struct i2c_dev_boardinfo *info)
> +{
> +	enum i3c_addr_slot_status status;
> +	struct device *dev = &master->dev;
> +	struct i2c_dev_boardinfo *boardinfo;
> +	struct i2c_dev_desc *i2cdev;
> +	int ret;
> +
> +	status = i3c_bus_get_addr_slot_status(&master->bus,
> +					      info->base.addr);
> +	if (status != I3C_ADDR_SLOT_FREE)
> +		return -EBUSY;
> +
> +	boardinfo = devm_kzalloc(dev, sizeof(*boardinfo), GFP_KERNEL);
> +	if (!boardinfo)
> +		return -ENOMEM;
> +
> +	i3c_bus_set_addr_slot_status(&master->bus,
> +				     info->base.addr,
> +				     I3C_ADDR_SLOT_I2C_DEV);
> +
> +	boardinfo->base.addr = info->base.addr;
> +	boardinfo->lvr = info->lvr;
> +
> +	i2cdev = i3c_master_alloc_i2c_dev(master, boardinfo);
> +	if (IS_ERR(i2cdev))
> +		return PTR_ERR(i2cdev);
> +
> +	ret = i3c_master_attach_i2c_dev(master, i2cdev);
> +	if (ret) {
> +		i3c_master_free_i2c_dev(i2cdev);
> +		return ret;
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_add_i2c_dev);
> +
>  /**
>   * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus
>   * @master: master used to send frames on the bus
> @@ -1832,6 +1937,15 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
>  	if (ret)
>  		goto err_free_dev;
>  
> +	if (I3C_BCR_DEVICE_ROLE(newdev->info.bcr) == I3C_BCR_I3C_MASTER &&
> +	    master->ops->enable_mastership) {
> +		if (master->ops->enable_mastership(newdev)) {
> +			dev_warn(&master->dev,
> +				 "Failed to enable mastership for device: %d%llx",
> +				 master->bus.id, newdev->info.pid);
> +		}
> +	}
> +
>  	olddev = i3c_master_search_i3c_dev_duplicate(newdev);
>  	if (olddev) {
>  		newdev->boardinfo = olddev->boardinfo;
> @@ -2102,6 +2216,11 @@ static int i3c_master_i2c_adapter_xfer(struct i2c_adapter *adap,
>  			return -ENOTSUPP;
>  	}
>  
> +	if (master->op_mode == I3C_SLAVE_MODE) {
> +		if (i3c_master_request_mastership(master))

Shouldn't this be called with the lock held?

Oh, and we might have a race here if the master gains bus ownership and
looses it just after that and before it was able to the transfer. I
think we should:

1/ send a GETACCMST
2/ disable MR EVENTS
3/ do the I3C/I2C transfers
4/ enable MR EVENTS

1 and 2 should be sent in the same transaction to avoid cases where
another master tries to acquire the bus, or if that's not possible, the
master that just gained ownership of the bus should reject any incoming
MR until DISEC(MR) has been sent.



> +			return -EIO;
> +	}
> +
>  	i3c_bus_normaluse_lock(&master->bus);
>  	dev = i3c_master_find_i2c_dev_by_addr(master, addr);
>  	if (!dev)
> @@ -2393,15 +2512,31 @@ static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops)
>  	return 0;
>  }
>  
> +static void i3c_master_bus_takeover(struct work_struct *work)
> +{
> +	struct i3c_master_controller *master;
> +
> +	master = container_of(work, struct i3c_master_controller, mastership);
> +
> +	i3c_bus_maintenance_lock(&master->bus);
> +	master->ops->update_devs(master);
> +	i3c_bus_maintenance_unlock(&master->bus);

Blank like here please.

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

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

> +
> +	/*
>  	 * We're done initializing the bus and the controller, we can now
>  	 * register I3C devices dicovered during the initial DAA.
>  	 */
> @@ -2521,6 +2660,95 @@ int i3c_master_register(struct i3c_master_controller *master,
>  EXPORT_SYMBOL_GPL(i3c_master_register);
>  
>  /**
> + * i3c_master_switch_operation_mode() - changes operation mode of the controller
> + * @master: master used to send frames on the bus
> + * @new_master: the device that takes control of the bus
> + * @op_mode: the master new operation mode
> + *
> + * Return: 0 in case of success, a negative error code otherwise.
> + */
> +int i3c_master_switch_operation_mode(struct i3c_master_controller *master,
> +				     struct i3c_dev_desc *new_master,
> +				     enum i3c_op_mode new_op_mode)
> +{
> +	if (master->op_mode == new_op_mode)
> +		return -EEXIST;
> +
> +	master->op_mode = new_op_mode;
> +
> +	if (new_op_mode == I3C_SLAVE_MODE) {
> +		master->bus.cur_master = new_master;
> +	} else {
> +		master->bus.cur_master = master->this;

This is done too early. You should update cur->master only after the
GETACCMST operation succeeds.

> +		INIT_WORK(&master->mastership, i3c_master_bus_takeover);

INIT_WORK() should be called once at init time, not everytime you
switch from one mode to another.

> +		queue_work(master->wq, &master->mastership);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_switch_operation_mode);
> +
> +/**
> + * i3c_master_mastership_ack() - performs operations before bus handover.
> + * @master: master used to send frames on the bus
> + * @info: I3C device information
> + *
> + * Basically, it sends DEFSLVS command to ensure new master is taking
> + * the bus with complete list of devices and then acknowledges bus takeover.
> + *
> + * Return: 0 in case of success, a negative error code otherwise.
> + */
> +int i3c_master_mastership_ack(struct i3c_master_controller *master,
> +			      const struct i3c_device_info *info)
> +{
> +	int ret;
> +
> +	i3c_bus_maintenance_lock(&master->bus);
> +	ret = i3c_master_defslvs_locked(master);
> +	i3c_bus_maintenance_unlock(&master->bus);
> +	if (ret)
> +		return ret;
> +

Hm, this should have been sent during DAA, no need to do it again here,
right?

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

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

> +
> +/**
>   * i3c_master_unregister() - unregister an I3C master
>   * @master: master used to send frames on the bus
>   *
> @@ -2552,6 +2780,11 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
>  	if (!master || !xfers)
>  		return -EINVAL;
>  
> +	if (master->op_mode == I3C_SLAVE_MODE) {
> +		if (i3c_master_request_mastership(master))
> +			return -EIO;
> +	}
> +
>  	if (!master->ops->priv_xfers)
>  		return -ENOTSUPP;
>  
> @@ -2657,5 +2890,6 @@ static void __exit i3c_exit(void)
>  module_exit(i3c_exit);
>  
>  MODULE_AUTHOR("Boris Brezillon <[e-mail]>");
> +MODULE_AUTHOR("Przemyslaw Gaj <[e-mail]>");

Please do that in a separate patch.

>  MODULE_DESCRIPTION("I3C core");
>  MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> index f13fd8b..ada956a 100644
> --- a/include/linux/i3c/master.h
> +++ b/include/linux/i3c/master.h
> @@ -282,6 +282,16 @@ enum i3c_addr_slot_status {
>  };
>  
>  /**
> + * enum i3c_op_mode - I3C controller operation mode
> + * @I3C_SLAVE_MODE: I3C controller operates in slave mode
> + * @I3C_MASTER_MODE: I3C controller operates in master mode
> + */
> +enum i3c_op_mode {
> +	I3C_SLAVE_MODE,
> +	I3C_MASTER_MODE
> +};
> +
> +/**
>   * 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?

>		     This function allows to
> + *               update devices received from previous bus owner in DEFSLVS
> + *               command. Useful also when new device joins the bus controlled
> + *               by secondary master, main master will be able to add
> + *               this device after mastership takeover.
> + * @request_mastership: requests bus mastership. By default called by secondary
> + *                      master after ENEC CCC is received and when devices were
> + *                      not fully initialized yet. Mastership is also requested
> + *                      when device driver wants to transfer data using master
> + *                      in slave mode.
> + * @enable_mastership: enable the Mastership for specified device. Mastership
> + *                     does not require handler. Mastership is enabled for all
> + *                     masters by default.

Maybe ->enable_mr_events() (->enable_mastership() is just unclear to
me). Oh, and if you have an enable function you should have a disable
one too.

>   */
>  struct i3c_master_controller_ops {
>  	int (*bus_init)(struct i3c_master_controller *master);
> @@ -445,6 +469,9 @@ struct i3c_master_controller_ops {
>  	int (*disable_ibi)(struct i3c_dev_desc *dev);
>  	void (*recycle_ibi_slot)(struct i3c_dev_desc *dev,
>  				 struct i3c_ibi_slot *slot);
> +	void (*update_devs)(struct i3c_master_controller *master);
> +	int (*request_mastership)(struct i3c_master_controller *master);
> +	int (*enable_mastership)(struct i3c_dev_desc *dev);
>  };
>  
>  /**
> @@ -458,6 +485,7 @@ struct i3c_master_controller_ops {
>   * @ops: master operations. See &struct i3c_master_controller_ops
>   * @secondary: true if the master is a secondary master
>   * @init_done: true when the bus initialization is done
> + * @op_mode: controller operation mode

As said earlier, I don't think this is needed. Just check ->cur_master
to know whether the master owns the bus or not.

>   * @boardinfo.i3c: list of I3C  boardinfo objects
>   * @boardinfo.i2c: list of I2C boardinfo objects
>   * @boardinfo: board-level information attached to devices connected on the bus
> @@ -467,6 +495,7 @@ struct i3c_master_controller_ops {
>   *	in a thread context. Typical examples are Hot Join processing which
>   *	requires taking the bus lock in maintenance, which in turn, can only
>   *	be done from a sleep-able context
> + * @mastership: work for switching operation mode after bus takeover

How about mr_work or something like that. mastership is a bit
ambiguous, and it's not really clear that it's representing a work
object.

>   *
>   * A &struct i3c_master_controller has to be registered to the I3C subsystem
>   * through i3c_master_register(). None of &struct i3c_master_controller fields
> @@ -480,12 +509,14 @@ struct i3c_master_controller {
>  	const struct i3c_master_controller_ops *ops;
>  	unsigned int secondary : 1;
>  	unsigned int init_done : 1;
> +	enum i3c_op_mode op_mode;
>  	struct {
>  		struct list_head i3c;
>  		struct list_head i2c;
>  	} boardinfo;
>  	struct i3c_bus bus;
>  	struct workqueue_struct *wq;
> +	struct work_struct mastership;
>  };
>  
>  /**
> @@ -523,7 +554,8 @@ int i3c_master_defslvs_locked(struct i3c_master_controller *master);
>  
>  int i3c_master_get_free_addr(struct i3c_master_controller *master,
>  			     u8 start_addr);
> -
> +int i3c_master_add_i2c_dev(struct i3c_master_controller *master,
> +			   struct i2c_dev_boardinfo *info);
>  int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
>  				  u8 addr);
>  int i3c_master_do_daa(struct i3c_master_controller *master);
> @@ -536,6 +568,15 @@ int i3c_master_register(struct i3c_master_controller *master,
>  			const struct i3c_master_controller_ops *ops,
>  			bool secondary);
>  int i3c_master_unregister(struct i3c_master_controller *master);
> +int i3c_master_get_accmst_locked(struct i3c_master_controller *master,
> +				 const struct i3c_device_info *info);
> +int i3c_master_switch_operation_mode(struct i3c_master_controller *master,
> +				     struct i3c_dev_desc *new_master,
> +				     enum i3c_op_mode new_op_mode);
> +int i3c_master_mastership_ack(struct i3c_master_controller *master,
> +			      const struct i3c_device_info *info);
> +void i3c_secondary_master_events_enabled(struct i3c_master_controller *master,
> +					 u8 evts);
>  
>  /**
>   * i3c_dev_get_master_data() - get master private data attached to an I3C

Przemysław Gaj Dec. 14, 2018, 7:14 a.m. UTC | #2

Hi Boris,

Thank you for reviewing this so quickly.

On 12/13/18, 2:50 PM, "Boris Brezillon" <[e-mail]> wrote:

    EXTERNAL MAIL
    
    
    On Thu, 13 Dec 2018 12:28:00 +0000
    Przemyslaw Gaj <[e-mail]> wrote:
    
    > This patch adds support for mastership request to I3C subsystem.
    > 
    > Mastership event is enabled for all the masters on the I3C bus
    > by default.
    > 
    > 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.
    
    > - If above procedure fails for some reason, user can force
    > mastership request through sysfs. Of course, mastership event
    > still has to be enabled on current master side
    
    Sounds like a good idea.
    
    > - Mastership is also requested automatically when device driver
    > tries to transfer data using master controller in slave mode.
    
    And that too.
    
    > 
    > There is still some limitation:
    > - I2C devices are not registered on secondary master side. I2C
    > devices received in DEFSLVS CCC command are added to device list
    > just to send back this information to the other master controllers
    > in case of bus handoff. We can add support for this in the future
    > if there is such use case.
    
    I2C devices must be described under the bus represented by the
    secondary master for that to work properly (we need a way to bind
    devices to drivers). So, it's probably a good thing to discard (or
    leave them inactive) any devices that do not have a i2c_board_info entry
    on the secondary master side.
    
    > 
    > Signed-off-by: Przemyslaw Gaj <[e-mail]>
    > ---
    >  drivers/i3c/master.c       | 260 ++++++++++++++++++++++++++++++++++++++++++---
    >  include/linux/i3c/master.h |  43 +++++++-
    >  2 files changed, 289 insertions(+), 14 deletions(-)
    > 
    > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
    > index 68d6553..e98b600 100644
    > --- a/drivers/i3c/master.c
    > +++ b/drivers/i3c/master.c
    > @@ -460,6 +460,15 @@ static int i3c_bus_init(struct i3c_bus *i3cbus)
    >  	return 0;
    >  }
    >  
    > +static int i3c_master_request_mastership(struct i3c_master_controller *master)
    > +{
    > +	if (!master->ops->request_mastership)
    > +		return -ENOTSUPP;
    
    Please add a blank line here
    
    > +	if (master->ops->request_mastership(master))
    > +		return -EIO;
    
    and here.

Ok for both.
    
    > +	return 0;
    > +}
    > +
    >  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?
    
    > +
    >  	i3c_bus_normaluse_lock(i3cbus);
    >  	ret = sprintf(buf, "%d-%llx\n", i3cbus->id,
    >  		      i3cbus->cur_master->info.pid);
    > @@ -659,6 +675,11 @@ static int i3c_master_send_ccc_cmd_locked(struct i3c_master_controller *master,
    >  	if (!cmd || !master)
    >  		return -EINVAL;
    >  
    > +	if (master->op_mode == I3C_SLAVE_MODE) {
    > +		if (i3c_master_request_mastership(master))
    > +			return -EIO;
    > +	}
    > +
    
    This shouldn't be needed either. if i3cbus->cur_master != master->this,
    that means the master is operating in slave mode.

I'll try that.
    
    >  	if (WARN_ON(master->init_done &&
    >  		    !rwsem_is_locked(&master->bus.lock)))
    >  		return -EINVAL;
    > @@ -1371,6 +1392,7 @@ static int i3c_master_reattach_i3c_dev(struct i3c_dev_desc *dev,
    >  						      dev->info.dyn_addr);
    >  		if (status != I3C_ADDR_SLOT_FREE)
    >  			return -EBUSY;
    > +
    
    Drop this change (or make it part of a separate patch fixing coding
    style issues).

Ok.
    
    >  		i3c_bus_set_addr_slot_status(&master->bus,
    >  					     dev->info.dyn_addr,
    >  					     I3C_ADDR_SLOT_I3C_DEV);
    > @@ -1491,6 +1513,46 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
    >  }
    >  
    >  /**
    > + * i3c_master_get_accmst_locked() - send a GETACCMST CCC command
    > + * @master: master used to send frames on the bus
    > + * @info: I3C device information
    
    Maybe you can simply pass the dynamic address instead of the full
    device_info struct.

No problem. Now I see it's not needed.
    
    > + *
    > + * Send a GETACCMST CCC command.
    > + *
    > + * This should be called if the curent master acknowledges bus takeover.
    
    I really thought this would be all automated by the master IP, but
    apparently it's not :-).
    
    > + *
    > + * This function must be called with the bus lock held in write mode.
    > + *
    > + * Return: 0 in case of success, a positive I3C error code if the error is
    > + * one of the official Mx error codes, and a negative error code otherwise.
    > + */
    > +int i3c_master_get_accmst_locked(struct i3c_master_controller *master,
    > +				 const struct i3c_device_info *info)
    > +{
    > +	struct i3c_ccc_getaccmst *accmst;
    > +	struct i3c_ccc_cmd_dest dest;
    > +	struct i3c_ccc_cmd cmd;
    > +	int ret;
    > +
    > +	accmst = i3c_ccc_cmd_dest_init(&dest, info->dyn_addr, sizeof(*accmst));
    > +	if (!accmst)
    > +		return -ENOMEM;
    > +
    > +	i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETACCMST, &dest, 1);
    > +
    > +	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
    > +
    > +	if (ret)
    > +		return ret;
    > +
    > +	if (dest.payload.len != sizeof(*accmst))
    > +		return -EIO;
    > +
    > +	return 0;
    > +}
    > +EXPORT_SYMBOL_GPL(i3c_master_get_accmst_locked);
    > +
    > +/**
    >   * i3c_master_do_daa() - do a DAA (Dynamic Address Assignment)
    >   * @master: master doing the DAA
    >   *
    > @@ -1554,11 +1616,8 @@ int i3c_master_set_info(struct i3c_master_controller *master,
    >  	struct i3c_dev_desc *i3cdev;
    >  	int ret;
    >  
    > -	if (!i3c_bus_dev_addr_is_avail(&master->bus, info->dyn_addr))
    > -		return -EINVAL;
    > -
    > -	if (I3C_BCR_DEVICE_ROLE(info->bcr) == I3C_BCR_I3C_MASTER &&
    > -	    master->secondary)
    > +	if (master->op_mode == I3C_MASTER_MODE &&
    > +	    !i3c_bus_dev_addr_is_avail(&master->bus, info->dyn_addr))
    
    Please place this test in a separate if branch, as they are not really
    related.

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

I'm wondering about HotJoin. I can't find this use case in my mind.
    
    > +	if (master->secondary)
    > +		return 0;
    
    Add a blank like here.

Ok.
    
    > +	/*
    >  	 * Reset all dynamic address that may have been assigned before
    >  	 * (assigned by the bootloader for example).
    >  	 */
    > @@ -1738,6 +1804,7 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
    >  	ret = i3c_master_disec_locked(master, I3C_BROADCAST_ADDR,
    >  				      I3C_CCC_EVENT_SIR | I3C_CCC_EVENT_MR |
    >  				      I3C_CCC_EVENT_HJ);
    > +
    
    Drop this blank line.

Ok.
    
    >  	if (ret && ret != I3C_ERROR_M2)
    >  		goto err_bus_cleanup;
    >  
    > @@ -1789,6 +1856,44 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
    >  	return NULL;
    >  }
    >  
    > +int i3c_master_add_i2c_dev(struct i3c_master_controller *master,
    > +			   struct i2c_dev_boardinfo *info)
    > +{
    > +	enum i3c_addr_slot_status status;
    > +	struct device *dev = &master->dev;
    > +	struct i2c_dev_boardinfo *boardinfo;
    > +	struct i2c_dev_desc *i2cdev;
    > +	int ret;
    > +
    > +	status = i3c_bus_get_addr_slot_status(&master->bus,
    > +					      info->base.addr);
    > +	if (status != I3C_ADDR_SLOT_FREE)
    > +		return -EBUSY;
    > +
    > +	boardinfo = devm_kzalloc(dev, sizeof(*boardinfo), GFP_KERNEL);
    > +	if (!boardinfo)
    > +		return -ENOMEM;
    > +
    > +	i3c_bus_set_addr_slot_status(&master->bus,
    > +				     info->base.addr,
    > +				     I3C_ADDR_SLOT_I2C_DEV);
    > +
    > +	boardinfo->base.addr = info->base.addr;
    > +	boardinfo->lvr = info->lvr;
    > +
    > +	i2cdev = i3c_master_alloc_i2c_dev(master, boardinfo);
    > +	if (IS_ERR(i2cdev))
    > +		return PTR_ERR(i2cdev);
    > +
    > +	ret = i3c_master_attach_i2c_dev(master, i2cdev);
    > +	if (ret) {
    > +		i3c_master_free_i2c_dev(i2cdev);
    > +		return ret;
    > +	}
    > +	return 0;
    > +}
    > +EXPORT_SYMBOL_GPL(i3c_master_add_i2c_dev);
    > +
    >  /**
    >   * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus
    >   * @master: master used to send frames on the bus
    > @@ -1832,6 +1937,15 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
    >  	if (ret)
    >  		goto err_free_dev;
    >  
    > +	if (I3C_BCR_DEVICE_ROLE(newdev->info.bcr) == I3C_BCR_I3C_MASTER &&
    > +	    master->ops->enable_mastership) {
    > +		if (master->ops->enable_mastership(newdev)) {
    > +			dev_warn(&master->dev,
    > +				 "Failed to enable mastership for device: %d%llx",
    > +				 master->bus.id, newdev->info.pid);
    > +		}
    > +	}
    > +
    >  	olddev = i3c_master_search_i3c_dev_duplicate(newdev);
    >  	if (olddev) {
    >  		newdev->boardinfo = olddev->boardinfo;
    > @@ -2102,6 +2216,11 @@ static int i3c_master_i2c_adapter_xfer(struct i2c_adapter *adap,
    >  			return -ENOTSUPP;
    >  	}
    >  
    > +	if (master->op_mode == I3C_SLAVE_MODE) {
    > +		if (i3c_master_request_mastership(master))
    
    Shouldn't this be called with the lock held?
    
    Oh, and we might have a race here if the master gains bus ownership and
    looses it just after that and before it was able to the transfer. I
    think we should:
    
    1/ send a GETACCMST
    2/ disable MR EVENTS
    3/ do the I3C/I2C transfers
    4/ enable MR EVENTS
    
    1 and 2 should be sent in the same transaction to avoid cases where
    another master tries to acquire the bus, or if that's not possible, the
    master that just gained ownership of the bus should reject any incoming
    MR until DISEC(MR) has been sent.

I strongly agree with your opinion. I'll implement it this way.
    
    > +			return -EIO;
    > +	}
    > +
    >  	i3c_bus_normaluse_lock(&master->bus);
    >  	dev = i3c_master_find_i2c_dev_by_addr(master, addr);
    >  	if (!dev)
    > @@ -2393,15 +2512,31 @@ static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops)
    >  	return 0;
    >  }
    >  
    > +static void i3c_master_bus_takeover(struct work_struct *work)
    > +{
    > +	struct i3c_master_controller *master;
    > +
    > +	master = container_of(work, struct i3c_master_controller, mastership);
    > +
    > +	i3c_bus_maintenance_lock(&master->bus);
    > +	master->ops->update_devs(master);
    > +	i3c_bus_maintenance_unlock(&master->bus);
    
    Blank like here please.

Ok
    
    > +	/*
    > +	 * 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.
    
    > +	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? 
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.
    
    > +
    > +	/*
    >  	 * We're done initializing the bus and the controller, we can now
    >  	 * register I3C devices dicovered during the initial DAA.
    >  	 */
    > @@ -2521,6 +2660,95 @@ int i3c_master_register(struct i3c_master_controller *master,
    >  EXPORT_SYMBOL_GPL(i3c_master_register);
    >  
    >  /**
    > + * i3c_master_switch_operation_mode() - changes operation mode of the controller
    > + * @master: master used to send frames on the bus
    > + * @new_master: the device that takes control of the bus
    > + * @op_mode: the master new operation mode
    > + *
    > + * Return: 0 in case of success, a negative error code otherwise.
    > + */
    > +int i3c_master_switch_operation_mode(struct i3c_master_controller *master,
    > +				     struct i3c_dev_desc *new_master,
    > +				     enum i3c_op_mode new_op_mode)
    > +{
    > +	if (master->op_mode == new_op_mode)
    > +		return -EEXIST;
    > +
    > +	master->op_mode = new_op_mode;
    > +
    > +	if (new_op_mode == I3C_SLAVE_MODE) {
    > +		master->bus.cur_master = new_master;
    > +	} else {
    > +		master->bus.cur_master = master->this;
    
    This is done too early. You should update cur->master only after the
    GETACCMST operation succeeds.

Indeed.
    
    > +		INIT_WORK(&master->mastership, i3c_master_bus_takeover);
    
    INIT_WORK() should be called once at init time, not everytime you
    switch from one mode to another.

My mistake.
    
    > +		queue_work(master->wq, &master->mastership);
    > +	}
    > +
    > +	return 0;
    > +}
    > +EXPORT_SYMBOL_GPL(i3c_master_switch_operation_mode);
    > +
    > +/**
    > + * i3c_master_mastership_ack() - performs operations before bus handover.
    > + * @master: master used to send frames on the bus
    > + * @info: I3C device information
    > + *
    > + * Basically, it sends DEFSLVS command to ensure new master is taking
    > + * the bus with complete list of devices and then acknowledges bus takeover.
    > + *
    > + * Return: 0 in case of success, a negative error code otherwise.
    > + */
    > +int i3c_master_mastership_ack(struct i3c_master_controller *master,
    > +			      const struct i3c_device_info *info)
    > +{
    > +	int ret;
    > +
    > +	i3c_bus_maintenance_lock(&master->bus);
    > +	ret = i3c_master_defslvs_locked(master);
    > +	i3c_bus_maintenance_unlock(&master->bus);
    > +	if (ret)
    > +		return ret;
    > +
    
    Hm, this should have been sent during DAA, no need to do it again here,
    right?

I thought that there could be the case when something has changed on the bus but now I think
device list on secondary master side is up-to-date without that. I'll drop this.
    
    > +	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.
    
    > +
    > +	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.
    
    > +
    > +/**
    >   * i3c_master_unregister() - unregister an I3C master
    >   * @master: master used to send frames on the bus
    >   *
    > @@ -2552,6 +2780,11 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
    >  	if (!master || !xfers)
    >  		return -EINVAL;
    >  
    > +	if (master->op_mode == I3C_SLAVE_MODE) {
    > +		if (i3c_master_request_mastership(master))
    > +			return -EIO;
    > +	}
    > +
    >  	if (!master->ops->priv_xfers)
    >  		return -ENOTSUPP;
    >  
    > @@ -2657,5 +2890,6 @@ static void __exit i3c_exit(void)
    >  module_exit(i3c_exit);
    >  
    >  MODULE_AUTHOR("Boris Brezillon <[e-mail]>");
    > +MODULE_AUTHOR("Przemyslaw Gaj <[e-mail]>");
    
    Please do that in a separate patch.

Ok.
    
    >  MODULE_DESCRIPTION("I3C core");
    >  MODULE_LICENSE("GPL v2");
    > diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
    > index f13fd8b..ada956a 100644
    > --- a/include/linux/i3c/master.h
    > +++ b/include/linux/i3c/master.h
    > @@ -282,6 +282,16 @@ enum i3c_addr_slot_status {
    >  };
    >  
    >  /**
    > + * enum i3c_op_mode - I3C controller operation mode
    > + * @I3C_SLAVE_MODE: I3C controller operates in slave mode
    > + * @I3C_MASTER_MODE: I3C controller operates in master mode
    > + */
    > +enum i3c_op_mode {
    > +	I3C_SLAVE_MODE,
    > +	I3C_MASTER_MODE
    > +};
    > +
    > +/**
    >   * 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.

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?
    
    >		     This function allows to
    > + *               update devices received from previous bus owner in DEFSLVS
    > + *               command. Useful also when new device joins the bus controlled
    > + *               by secondary master, main master will be able to add
    > + *               this device after mastership takeover.
    > + * @request_mastership: requests bus mastership. By default called by secondary
    > + *                      master after ENEC CCC is received and when devices were
    > + *                      not fully initialized yet. Mastership is also requested
    > + *                      when device driver wants to transfer data using master
    > + *                      in slave mode.
    > + * @enable_mastership: enable the Mastership for specified device. Mastership
    > + *                     does not require handler. Mastership is enabled for all
    > + *                     masters by default.
    
    Maybe ->enable_mr_events() (->enable_mastership() is just unclear to
    me). Oh, and if you have an enable function you should have a disable
    one too.

I agree.
    
    >   */
    >  struct i3c_master_controller_ops {
    >  	int (*bus_init)(struct i3c_master_controller *master);
    > @@ -445,6 +469,9 @@ struct i3c_master_controller_ops {
    >  	int (*disable_ibi)(struct i3c_dev_desc *dev);
    >  	void (*recycle_ibi_slot)(struct i3c_dev_desc *dev,
    >  				 struct i3c_ibi_slot *slot);
    > +	void (*update_devs)(struct i3c_master_controller *master);
    > +	int (*request_mastership)(struct i3c_master_controller *master);
    > +	int (*enable_mastership)(struct i3c_dev_desc *dev);
    >  };
    >  
    >  /**
    > @@ -458,6 +485,7 @@ struct i3c_master_controller_ops {
    >   * @ops: master operations. See &struct i3c_master_controller_ops
    >   * @secondary: true if the master is a secondary master
    >   * @init_done: true when the bus initialization is done
    > + * @op_mode: controller operation mode
    
    As said earlier, I don't think this is needed. Just check ->cur_master
    to know whether the master owns the bus or not.

I'll try.
    
    >   * @boardinfo.i3c: list of I3C  boardinfo objects
    >   * @boardinfo.i2c: list of I2C boardinfo objects
    >   * @boardinfo: board-level information attached to devices connected on the bus
    > @@ -467,6 +495,7 @@ struct i3c_master_controller_ops {
    >   *	in a thread context. Typical examples are Hot Join processing which
    >   *	requires taking the bus lock in maintenance, which in turn, can only
    >   *	be done from a sleep-able context
    > + * @mastership: work for switching operation mode after bus takeover
    
    How about mr_work or something like that. mastership is a bit
    ambiguous, and it's not really clear that it's representing a work
    object.

I agree.
    
    >   *
    >   * A &struct i3c_master_controller has to be registered to the I3C subsystem
    >   * through i3c_master_register(). None of &struct i3c_master_controller fields
    > @@ -480,12 +509,14 @@ struct i3c_master_controller {
    >  	const struct i3c_master_controller_ops *ops;
    >  	unsigned int secondary : 1;
    >  	unsigned int init_done : 1;
    > +	enum i3c_op_mode op_mode;
    >  	struct {
    >  		struct list_head i3c;
    >  		struct list_head i2c;
    >  	} boardinfo;
    >  	struct i3c_bus bus;
    >  	struct workqueue_struct *wq;
    > +	struct work_struct mastership;
    >  };
    >  
    >  /**
    > @@ -523,7 +554,8 @@ int i3c_master_defslvs_locked(struct i3c_master_controller *master);
    >  
    >  int i3c_master_get_free_addr(struct i3c_master_controller *master,
    >  			     u8 start_addr);
    > -
    > +int i3c_master_add_i2c_dev(struct i3c_master_controller *master,
    > +			   struct i2c_dev_boardinfo *info);
    >  int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
    >  				  u8 addr);
    >  int i3c_master_do_daa(struct i3c_master_controller *master);
    > @@ -536,6 +568,15 @@ int i3c_master_register(struct i3c_master_controller *master,
    >  			const struct i3c_master_controller_ops *ops,
    >  			bool secondary);
    >  int i3c_master_unregister(struct i3c_master_controller *master);
    > +int i3c_master_get_accmst_locked(struct i3c_master_controller *master,
    > +				 const struct i3c_device_info *info);
    > +int i3c_master_switch_operation_mode(struct i3c_master_controller *master,
    > +				     struct i3c_dev_desc *new_master,
    > +				     enum i3c_op_mode new_op_mode);
    > +int i3c_master_mastership_ack(struct i3c_master_controller *master,
    > +			      const struct i3c_device_info *info);
    > +void i3c_secondary_master_events_enabled(struct i3c_master_controller *master,
    > +					 u8 evts);
    >  
    >  /**
    >   * i3c_dev_get_master_data() - get master private data attached to an I3C
    

Thanks,
Przemek

Boris Brezillon Dec. 14, 2018, 8:24 a.m. UTC | #3

Hi Przemek,

On Fri, 14 Dec 2018 07:14:44 +0000
Przemyslaw Gaj <[e-mail]> 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

Przemysław Gaj Dec. 14, 2018, 1:47 p.m. UTC | #4

Hi Boris,

> On Dec 14, 2018, at 9:24 AM, Boris Brezillon <[e-mail]> wrote:
> 
> EXTERNAL MAIL
> 
> 
> Hi Przemek,
> 
> On Fri, 14 Dec 2018 07:14:44 +0000
> Przemyslaw Gaj <[e-mail]> 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

Boris Brezillon Dec. 19, 2018, 2:16 p.m. UTC | #5

On Fri, 14 Dec 2018 13:47:39 +0000
Przemyslaw Gaj <[e-mail]> wrote:

> >>> @@ -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?

Then you'd have to delay this part until ENEC(MR) is received, so maybe
it's just easier to let master controller drivers handle that part on
their own.

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

It really depends what we mean by "bus initialization". If we decide
that secondary master bus init is just about initializing the
i3c_master_controller and i3c_bus structs (and maybe enable the
controller and send an HJ event on the bus) and nothing more, then we
should let I3C master controller drivers register I3C dev on their own.

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

I don't know. I was just thinking out loud. Right now, I can't tell
which option is best:

1/ provide a minimal secondary master init routine and let drivers do
   all the hard work
2/ provide an automated procedure to simplify driver's life and take
   the risk of making it too restrictive

If I had to work on that, I'd probably go for option #1 and try to
refine things when we start having more controllers, but I might be
wrong.

Patch

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 68d6553..e98b600 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -460,6 +460,15 @@  static int i3c_bus_init(struct i3c_bus *i3cbus)
 	return 0;
 }
 
+static int i3c_master_request_mastership(struct i3c_master_controller *master)
+{
+	if (!master->ops->request_mastership)
+		return -ENOTSUPP;
+	if (master->ops->request_mastership(master))
+		return -EIO;
+	return 0;
+}
+
 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");
+	}
+
 	i3c_bus_normaluse_lock(i3cbus);
 	ret = sprintf(buf, "%d-%llx\n", i3cbus->id,
 		      i3cbus->cur_master->info.pid);
@@ -659,6 +675,11 @@  static int i3c_master_send_ccc_cmd_locked(struct i3c_master_controller *master,
 	if (!cmd || !master)
 		return -EINVAL;
 
+	if (master->op_mode == I3C_SLAVE_MODE) {
+		if (i3c_master_request_mastership(master))
+			return -EIO;
+	}
+
 	if (WARN_ON(master->init_done &&
 		    !rwsem_is_locked(&master->bus.lock)))
 		return -EINVAL;
@@ -1371,6 +1392,7 @@  static int i3c_master_reattach_i3c_dev(struct i3c_dev_desc *dev,
 						      dev->info.dyn_addr);
 		if (status != I3C_ADDR_SLOT_FREE)
 			return -EBUSY;
+
 		i3c_bus_set_addr_slot_status(&master->bus,
 					     dev->info.dyn_addr,
 					     I3C_ADDR_SLOT_I3C_DEV);
@@ -1491,6 +1513,46 @@  i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
 }
 
 /**
+ * i3c_master_get_accmst_locked() - send a GETACCMST CCC command
+ * @master: master used to send frames on the bus
+ * @info: I3C device information
+ *
+ * Send a GETACCMST CCC command.
+ *
+ * This should be called if the curent master acknowledges bus takeover.
+ *
+ * This function must be called with the bus lock held in write mode.
+ *
+ * Return: 0 in case of success, a positive I3C error code if the error is
+ * one of the official Mx error codes, and a negative error code otherwise.
+ */
+int i3c_master_get_accmst_locked(struct i3c_master_controller *master,
+				 const struct i3c_device_info *info)
+{
+	struct i3c_ccc_getaccmst *accmst;
+	struct i3c_ccc_cmd_dest dest;
+	struct i3c_ccc_cmd cmd;
+	int ret;
+
+	accmst = i3c_ccc_cmd_dest_init(&dest, info->dyn_addr, sizeof(*accmst));
+	if (!accmst)
+		return -ENOMEM;
+
+	i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETACCMST, &dest, 1);
+
+	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
+
+	if (ret)
+		return ret;
+
+	if (dest.payload.len != sizeof(*accmst))
+		return -EIO;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(i3c_master_get_accmst_locked);
+
+/**
  * i3c_master_do_daa() - do a DAA (Dynamic Address Assignment)
  * @master: master doing the DAA
  *
@@ -1554,11 +1616,8 @@  int i3c_master_set_info(struct i3c_master_controller *master,
 	struct i3c_dev_desc *i3cdev;
 	int ret;
 
-	if (!i3c_bus_dev_addr_is_avail(&master->bus, info->dyn_addr))
-		return -EINVAL;
-
-	if (I3C_BCR_DEVICE_ROLE(info->bcr) == I3C_BCR_I3C_MASTER &&
-	    master->secondary)
+	if (master->op_mode == I3C_MASTER_MODE &&
+	    !i3c_bus_dev_addr_is_avail(&master->bus, info->dyn_addr))
 		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;
 
 	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.
+	 */
+	if (master->secondary)
+		return 0;
+	/*
 	 * Reset all dynamic address that may have been assigned before
 	 * (assigned by the bootloader for example).
 	 */
@@ -1738,6 +1804,7 @@  static int i3c_master_bus_init(struct i3c_master_controller *master)
 	ret = i3c_master_disec_locked(master, I3C_BROADCAST_ADDR,
 				      I3C_CCC_EVENT_SIR | I3C_CCC_EVENT_MR |
 				      I3C_CCC_EVENT_HJ);
+
 	if (ret && ret != I3C_ERROR_M2)
 		goto err_bus_cleanup;
 
@@ -1789,6 +1856,44 @@  i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
 	return NULL;
 }
 
+int i3c_master_add_i2c_dev(struct i3c_master_controller *master,
+			   struct i2c_dev_boardinfo *info)
+{
+	enum i3c_addr_slot_status status;
+	struct device *dev = &master->dev;
+	struct i2c_dev_boardinfo *boardinfo;
+	struct i2c_dev_desc *i2cdev;
+	int ret;
+
+	status = i3c_bus_get_addr_slot_status(&master->bus,
+					      info->base.addr);
+	if (status != I3C_ADDR_SLOT_FREE)
+		return -EBUSY;
+
+	boardinfo = devm_kzalloc(dev, sizeof(*boardinfo), GFP_KERNEL);
+	if (!boardinfo)
+		return -ENOMEM;
+
+	i3c_bus_set_addr_slot_status(&master->bus,
+				     info->base.addr,
+				     I3C_ADDR_SLOT_I2C_DEV);
+
+	boardinfo->base.addr = info->base.addr;
+	boardinfo->lvr = info->lvr;
+
+	i2cdev = i3c_master_alloc_i2c_dev(master, boardinfo);
+	if (IS_ERR(i2cdev))
+		return PTR_ERR(i2cdev);
+
+	ret = i3c_master_attach_i2c_dev(master, i2cdev);
+	if (ret) {
+		i3c_master_free_i2c_dev(i2cdev);
+		return ret;
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(i3c_master_add_i2c_dev);
+
 /**
  * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus
  * @master: master used to send frames on the bus
@@ -1832,6 +1937,15 @@  int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
 	if (ret)
 		goto err_free_dev;
 
+	if (I3C_BCR_DEVICE_ROLE(newdev->info.bcr) == I3C_BCR_I3C_MASTER &&
+	    master->ops->enable_mastership) {
+		if (master->ops->enable_mastership(newdev)) {
+			dev_warn(&master->dev,
+				 "Failed to enable mastership for device: %d%llx",
+				 master->bus.id, newdev->info.pid);
+		}
+	}
+
 	olddev = i3c_master_search_i3c_dev_duplicate(newdev);
 	if (olddev) {
 		newdev->boardinfo = olddev->boardinfo;
@@ -2102,6 +2216,11 @@  static int i3c_master_i2c_adapter_xfer(struct i2c_adapter *adap,
 			return -ENOTSUPP;
 	}
 
+	if (master->op_mode == I3C_SLAVE_MODE) {
+		if (i3c_master_request_mastership(master))
+			return -EIO;
+	}
+
 	i3c_bus_normaluse_lock(&master->bus);
 	dev = i3c_master_find_i2c_dev_by_addr(master, addr);
 	if (!dev)
@@ -2393,15 +2512,31 @@  static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops)
 	return 0;
 }
 
+static void i3c_master_bus_takeover(struct work_struct *work)
+{
+	struct i3c_master_controller *master;
+
+	master = container_of(work, struct i3c_master_controller, mastership);
+
+	i3c_bus_maintenance_lock(&master->bus);
+	master->ops->update_devs(master);
+	i3c_bus_maintenance_unlock(&master->bus);
+	/*
+	 * We can register I3C devices received from master by DEFSLVS.
+	 */
+	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() - 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;
+
+	/*
 	 * We're done initializing the bus and the controller, we can now
 	 * register I3C devices dicovered during the initial DAA.
 	 */
@@ -2521,6 +2660,95 @@  int i3c_master_register(struct i3c_master_controller *master,
 EXPORT_SYMBOL_GPL(i3c_master_register);
 
 /**
+ * i3c_master_switch_operation_mode() - changes operation mode of the controller
+ * @master: master used to send frames on the bus
+ * @new_master: the device that takes control of the bus
+ * @op_mode: the master new operation mode
+ *
+ * Return: 0 in case of success, a negative error code otherwise.
+ */
+int i3c_master_switch_operation_mode(struct i3c_master_controller *master,
+				     struct i3c_dev_desc *new_master,
+				     enum i3c_op_mode new_op_mode)
+{
+	if (master->op_mode == new_op_mode)
+		return -EEXIST;
+
+	master->op_mode = new_op_mode;
+
+	if (new_op_mode == I3C_SLAVE_MODE) {
+		master->bus.cur_master = new_master;
+	} else {
+		master->bus.cur_master = master->this;
+		INIT_WORK(&master->mastership, i3c_master_bus_takeover);
+		queue_work(master->wq, &master->mastership);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(i3c_master_switch_operation_mode);
+
+/**
+ * i3c_master_mastership_ack() - performs operations before bus handover.
+ * @master: master used to send frames on the bus
+ * @info: I3C device information
+ *
+ * Basically, it sends DEFSLVS command to ensure new master is taking
+ * the bus with complete list of devices and then acknowledges bus takeover.
+ *
+ * Return: 0 in case of success, a negative error code otherwise.
+ */
+int i3c_master_mastership_ack(struct i3c_master_controller *master,
+			      const struct i3c_device_info *info)
+{
+	int ret;
+
+	i3c_bus_maintenance_lock(&master->bus);
+	ret = i3c_master_defslvs_locked(master);
+	i3c_bus_maintenance_unlock(&master->bus);
+	if (ret)
+		return ret;
+
+	i3c_bus_maintenance_lock(&master->bus);
+	ret = i3c_master_get_accmst_locked(master, info);
+	i3c_bus_maintenance_unlock(&master->bus);
+	if (ret)
+		return ret;
+
+	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);
+
+/**
  * i3c_master_unregister() - unregister an I3C master
  * @master: master used to send frames on the bus
  *
@@ -2552,6 +2780,11 @@  int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
 	if (!master || !xfers)
 		return -EINVAL;
 
+	if (master->op_mode == I3C_SLAVE_MODE) {
+		if (i3c_master_request_mastership(master))
+			return -EIO;
+	}
+
 	if (!master->ops->priv_xfers)
 		return -ENOTSUPP;
 
@@ -2657,5 +2890,6 @@  static void __exit i3c_exit(void)
 module_exit(i3c_exit);
 
 MODULE_AUTHOR("Boris Brezillon <[e-mail]>");
+MODULE_AUTHOR("Przemyslaw Gaj <[e-mail]>");
 MODULE_DESCRIPTION("I3C core");
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index f13fd8b..ada956a 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -282,6 +282,16 @@  enum i3c_addr_slot_status {
 };
 
 /**
+ * enum i3c_op_mode - I3C controller operation mode
+ * @I3C_SLAVE_MODE: I3C controller operates in slave mode
+ * @I3C_MASTER_MODE: I3C controller operates in master mode
+ */
+enum i3c_op_mode {
+	I3C_SLAVE_MODE,
+	I3C_MASTER_MODE
+};
+
+/**
  * 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. This function allows to
+ *               update devices received from previous bus owner in DEFSLVS
+ *               command. Useful also when new device joins the bus controlled
+ *               by secondary master, main master will be able to add
+ *               this device after mastership takeover.
+ * @request_mastership: requests bus mastership. By default called by secondary
+ *                      master after ENEC CCC is received and when devices were
+ *                      not fully initialized yet. Mastership is also requested
+ *                      when device driver wants to transfer data using master
+ *                      in slave mode.
+ * @enable_mastership: enable the Mastership for specified device. Mastership
+ *                     does not require handler. Mastership is enabled for all
+ *                     masters by default.
  */
 struct i3c_master_controller_ops {
 	int (*bus_init)(struct i3c_master_controller *master);
@@ -445,6 +469,9 @@  struct i3c_master_controller_ops {
 	int (*disable_ibi)(struct i3c_dev_desc *dev);
 	void (*recycle_ibi_slot)(struct i3c_dev_desc *dev,
 				 struct i3c_ibi_slot *slot);
+	void (*update_devs)(struct i3c_master_controller *master);
+	int (*request_mastership)(struct i3c_master_controller *master);
+	int (*enable_mastership)(struct i3c_dev_desc *dev);
 };
 
 /**
@@ -458,6 +485,7 @@  struct i3c_master_controller_ops {
  * @ops: master operations. See &struct i3c_master_controller_ops
  * @secondary: true if the master is a secondary master
  * @init_done: true when the bus initialization is done
+ * @op_mode: controller operation mode
  * @boardinfo.i3c: list of I3C  boardinfo objects
  * @boardinfo.i2c: list of I2C boardinfo objects
  * @boardinfo: board-level information attached to devices connected on the bus
@@ -467,6 +495,7 @@  struct i3c_master_controller_ops {
  *	in a thread context. Typical examples are Hot Join processing which
  *	requires taking the bus lock in maintenance, which in turn, can only
  *	be done from a sleep-able context
+ * @mastership: work for switching operation mode after bus takeover
  *
  * A &struct i3c_master_controller has to be registered to the I3C subsystem
  * through i3c_master_register(). None of &struct i3c_master_controller fields
@@ -480,12 +509,14 @@  struct i3c_master_controller {
 	const struct i3c_master_controller_ops *ops;
 	unsigned int secondary : 1;
 	unsigned int init_done : 1;
+	enum i3c_op_mode op_mode;
 	struct {
 		struct list_head i3c;
 		struct list_head i2c;
 	} boardinfo;
 	struct i3c_bus bus;
 	struct workqueue_struct *wq;
+	struct work_struct mastership;
 };
 
 /**
@@ -523,7 +554,8 @@  int i3c_master_defslvs_locked(struct i3c_master_controller *master);
 
 int i3c_master_get_free_addr(struct i3c_master_controller *master,
 			     u8 start_addr);
-
+int i3c_master_add_i2c_dev(struct i3c_master_controller *master,
+			   struct i2c_dev_boardinfo *info);
 int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
 				  u8 addr);
 int i3c_master_do_daa(struct i3c_master_controller *master);
@@ -536,6 +568,15 @@  int i3c_master_register(struct i3c_master_controller *master,
 			const struct i3c_master_controller_ops *ops,
 			bool secondary);
 int i3c_master_unregister(struct i3c_master_controller *master);
+int i3c_master_get_accmst_locked(struct i3c_master_controller *master,
+				 const struct i3c_device_info *info);
+int i3c_master_switch_operation_mode(struct i3c_master_controller *master,
+				     struct i3c_dev_desc *new_master,
+				     enum i3c_op_mode new_op_mode);
+int i3c_master_mastership_ack(struct i3c_master_controller *master,
+			      const struct i3c_device_info *info);
+void i3c_secondary_master_events_enabled(struct i3c_master_controller *master,
+					 u8 evts);
 
 /**
  * i3c_dev_get_master_data() - get master private data attached to an I3C