| Message ID | 1589202833-6908-1-git-send-email-pthombar@cadence.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | I3C mastership handover support | expand |
On Mon, 11 May 2020 15:13:53 +0200 Parshuram Thombare <pthombar@cadence.com> wrote: > add i3c_secondary_master_register which is used > to register secondary masters. > > Signed-off-by: Parshuram Thombare <pthombar@cadence.com> > --- > drivers/i3c/master.c | 154 ++++++++++++++++++++++++++++++++++++- > include/linux/i3c/master.h | 3 + > 2 files changed, 156 insertions(+), 1 deletion(-) > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > index ba07a7d49633..669bd7e45810 100644 > --- a/drivers/i3c/master.c > +++ b/drivers/i3c/master.c > @@ -1768,6 +1768,90 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) > return ret; > } > > +/** > + * i3c_secondary_master_bus_init() - initialize an I3C bus for secondary > + * master > + * @master: secondary master initializing the bus > + * > + * This function does > + * > + * 1. Attach I2C devs to the master > + * > + * 2. Call &i3c_master_controller_ops->bus_init() method to initialize > + * the master controller. That's usually where the bus mode is selected > + * (pure bus or mixed fast/slow bus) Can you really select the bus mode without knowing the I3C devices you have on the bus? Or maybe that's a preliminary initialization which is then updated when you receive DEFSLVS events. > + * > + * Once this is done, I2C devices should be usable. I suspect you'll have to request bus ownership first, which means they're not really usable, just registered to the I2C subsystem. This might lead to a whole bunch of problems when drivers will try to send messages to the I2C devices and receive ETIMEDOUT/EBUSY errors. We'll probably need some updates to the I2C framework if we want I2C to play nicely with bus handover, but I think we can keep that for later. I'd suggest to forget about I2C for now and mark that as a limitation (no I2C devs on secondary masters). > + * > + * Return: a 0 in case of success, an negative error code otherwise. > + */ > +static int i3c_secondary_master_bus_init(struct i3c_master_controller *master) > +{ > + enum i3c_addr_slot_status status; > + struct i2c_dev_boardinfo *i2cboardinfo; > + struct i2c_dev_desc *i2cdev; > + int ret; > + > + /* > + * First attach all devices with static definitions provided by the > + * FW. > + */ > + list_for_each_entry(i2cboardinfo, &master->boardinfo.i2c, node) { > + status = i3c_bus_get_addr_slot_status(&master->bus, > + i2cboardinfo->base.addr); > + if (status != I3C_ADDR_SLOT_FREE) { > + ret = -EBUSY; > + goto err_detach_devs; > + } > + > + i3c_bus_set_addr_slot_status(&master->bus, > + i2cboardinfo->base.addr, > + I3C_ADDR_SLOT_I2C_DEV); > + > + i2cdev = i3c_master_alloc_i2c_dev(master, i2cboardinfo); > + if (IS_ERR(i2cdev)) { > + ret = PTR_ERR(i2cdev); > + goto err_detach_devs; > + } > + > + ret = i3c_master_attach_i2c_dev(master, i2cdev); > + if (ret) { > + i3c_master_free_i2c_dev(i2cdev); > + goto err_detach_devs; > + } > + } > + > + /* > + * Now execute the controller specific ->bus_init() routine, which > + * might configure its internal logic to match the bus limitations. > + */ > + ret = master->ops->bus_init(master); > + if (ret) > + goto err_detach_devs; > + > + /* > + * The master device should have been instantiated in ->bus_init(), > + * complain if this was not the case. > + */ > + if (!master->this) { > + dev_err(&master->dev, > + "master_set_info() was not called in ->bus_init()\n"); > + ret = -EINVAL; > + goto err_bus_cleanup; > + } > + > + return 0; > + > +err_bus_cleanup: > + if (master->ops->bus_cleanup) > + master->ops->bus_cleanup(master); > + > +err_detach_devs: > + i3c_master_detach_free_devs(master); > + > + return ret; > +} > + > static void i3c_master_bus_cleanup(struct i3c_master_controller *master) > { > if (master->ops->bus_cleanup) > @@ -2457,7 +2541,10 @@ static int i3c_master_init(struct i3c_master_controller *master, > goto err_put_dev; > } > > - ret = i3c_master_bus_init(master); > + if (secondary) > + ret = i3c_secondary_master_bus_init(master); > + else > + ret = i3c_master_bus_init(master); > if (ret) > goto err_put_dev; > > @@ -2535,6 +2622,71 @@ int i3c_master_register(struct i3c_master_controller *master, > } > EXPORT_SYMBOL_GPL(i3c_master_register); > > +/** > + * i3c_secondary_master_register() - register an I3C secondary 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 > + * > + * This function does minimal required initialization for secondary > + * master, rest functionality like creating and registering I2C > + * and I3C devices is done in defslvs processing. > + * > + * i3c_secondary_master_register() does following things - > + * - creates and initializes the I3C bus > + * - populates the bus with static I2C devs if @parent->of_node is not > + * NULL > + * initialization > + * - allocate memory for defslvs_data.devs, which is used to receive > + * defslvs list > + * - create I3C device representing this master > + * - registers the I2C adapter and all I2C devices > + * > + * Return: 0 in case of success, a negative error code otherwise. > + */ > +int i3c_secondary_master_register(struct i3c_master_controller *master, > + struct device *parent, > + const struct i3c_master_controller_ops *ops) > +{ > + int ret; > + > + ret = i3c_master_init(master, parent, ops, true); > + if (ret) > + return ret; > + > + ret = device_add(&master->dev); > + if (ret) > + goto err_cleanup_bus; > + > + /* > + * Expose our I3C bus as an I2C adapter so that I2C devices are exposed > + * through the I2C subsystem. > + */ > + ret = i3c_master_i2c_adapter_init(master); > + if (ret) > + goto err_del_dev; > + > + /* > + * We're done initializing the bus and the controller, we can now > + * register I3C devices from defslvs list. > + */ > + master->init_done = true; > + > + return 0; > + > +err_del_dev: > + device_del(&master->dev); > + > +err_cleanup_bus: > + i3c_master_bus_cleanup(master); > + > + put_device(&master->dev); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(i3c_secondary_master_register); > + > /** > * i3c_master_unregister() - unregister an I3C master > * @master: master used to send frames on the bus > diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h > index f5ba82c390bc..5124ff4831eb 100644 > --- a/include/linux/i3c/master.h > +++ b/include/linux/i3c/master.h > @@ -534,6 +534,9 @@ int i3c_master_set_info(struct i3c_master_controller *master, > int i3c_master_register(struct i3c_master_controller *master, > struct device *parent, > const struct i3c_master_controller_ops *ops); > +int i3c_secondary_master_register(struct i3c_master_controller *master, > + struct device *parent, > + const struct i3c_master_controller_ops *ops); > int i3c_master_unregister(struct i3c_master_controller *master); > > /**
>Can you really select the bus mode without knowing the I3C devices you >have on the bus? Or maybe that's a preliminary initialization which is >then updated when you receive DEFSLVS events. I think we can select bus mode based on knowledge of I2C devices on the bus. I was expecting to support different I2C devices information on main master and secondary masters. But that seems problematic for mastership requests. As IMO for mastership acquire and yield to work, all master capable devices should be operating in same bus mode. In previous patch set I tried approach of all devices using same (Pure) bus mode during initialization and updating it on DEFSLVS reception. But this can cause issue for hot joining devices as current master would have already switched it's bus mode to something other than pure mode, and hot join would not work. To me it's seems for mastership handover to work, we need all masters to have complete I2C device list on the bus. >I suspect you'll have to request bus ownership first, which means >they're not really usable, just registered to the I2C subsystem. This >might lead to a whole bunch of problems when drivers will try to send >messages to the I2C devices and receive ETIMEDOUT/EBUSY errors. We'll >probably need some updates to the I2C framework if we want I2C to play >nicely with bus handover, but I think we can keep that for later. I'd >suggest to forget about I2C for now and mark that as a limitation (no >I2C devs on secondary masters). Correct, here I2C devices are just registered. Irrespective of the device type (I2C/I3C) to which master want to communicate with, bus has to be acquired. Regards, Parshuram Thombare
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c index ba07a7d49633..669bd7e45810 100644 --- a/drivers/i3c/master.c +++ b/drivers/i3c/master.c @@ -1768,6 +1768,90 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) return ret; } +/** + * i3c_secondary_master_bus_init() - initialize an I3C bus for secondary + * master + * @master: secondary master initializing the bus + * + * This function does + * + * 1. Attach I2C devs to the master + * + * 2. Call &i3c_master_controller_ops->bus_init() method to initialize + * the master controller. That's usually where the bus mode is selected + * (pure bus or mixed fast/slow bus) + * + * Once this is done, I2C devices should be usable. + * + * Return: a 0 in case of success, an negative error code otherwise. + */ +static int i3c_secondary_master_bus_init(struct i3c_master_controller *master) +{ + enum i3c_addr_slot_status status; + struct i2c_dev_boardinfo *i2cboardinfo; + struct i2c_dev_desc *i2cdev; + int ret; + + /* + * First attach all devices with static definitions provided by the + * FW. + */ + list_for_each_entry(i2cboardinfo, &master->boardinfo.i2c, node) { + status = i3c_bus_get_addr_slot_status(&master->bus, + i2cboardinfo->base.addr); + if (status != I3C_ADDR_SLOT_FREE) { + ret = -EBUSY; + goto err_detach_devs; + } + + i3c_bus_set_addr_slot_status(&master->bus, + i2cboardinfo->base.addr, + I3C_ADDR_SLOT_I2C_DEV); + + i2cdev = i3c_master_alloc_i2c_dev(master, i2cboardinfo); + if (IS_ERR(i2cdev)) { + ret = PTR_ERR(i2cdev); + goto err_detach_devs; + } + + ret = i3c_master_attach_i2c_dev(master, i2cdev); + if (ret) { + i3c_master_free_i2c_dev(i2cdev); + goto err_detach_devs; + } + } + + /* + * Now execute the controller specific ->bus_init() routine, which + * might configure its internal logic to match the bus limitations. + */ + ret = master->ops->bus_init(master); + if (ret) + goto err_detach_devs; + + /* + * The master device should have been instantiated in ->bus_init(), + * complain if this was not the case. + */ + if (!master->this) { + dev_err(&master->dev, + "master_set_info() was not called in ->bus_init()\n"); + ret = -EINVAL; + goto err_bus_cleanup; + } + + return 0; + +err_bus_cleanup: + if (master->ops->bus_cleanup) + master->ops->bus_cleanup(master); + +err_detach_devs: + i3c_master_detach_free_devs(master); + + return ret; +} + static void i3c_master_bus_cleanup(struct i3c_master_controller *master) { if (master->ops->bus_cleanup) @@ -2457,7 +2541,10 @@ static int i3c_master_init(struct i3c_master_controller *master, goto err_put_dev; } - ret = i3c_master_bus_init(master); + if (secondary) + ret = i3c_secondary_master_bus_init(master); + else + ret = i3c_master_bus_init(master); if (ret) goto err_put_dev; @@ -2535,6 +2622,71 @@ int i3c_master_register(struct i3c_master_controller *master, } EXPORT_SYMBOL_GPL(i3c_master_register); +/** + * i3c_secondary_master_register() - register an I3C secondary 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 + * + * This function does minimal required initialization for secondary + * master, rest functionality like creating and registering I2C + * and I3C devices is done in defslvs processing. + * + * i3c_secondary_master_register() does following things - + * - creates and initializes the I3C bus + * - populates the bus with static I2C devs if @parent->of_node is not + * NULL + * initialization + * - allocate memory for defslvs_data.devs, which is used to receive + * defslvs list + * - create I3C device representing this master + * - registers the I2C adapter and all I2C devices + * + * Return: 0 in case of success, a negative error code otherwise. + */ +int i3c_secondary_master_register(struct i3c_master_controller *master, + struct device *parent, + const struct i3c_master_controller_ops *ops) +{ + int ret; + + ret = i3c_master_init(master, parent, ops, true); + if (ret) + return ret; + + ret = device_add(&master->dev); + if (ret) + goto err_cleanup_bus; + + /* + * Expose our I3C bus as an I2C adapter so that I2C devices are exposed + * through the I2C subsystem. + */ + ret = i3c_master_i2c_adapter_init(master); + if (ret) + goto err_del_dev; + + /* + * We're done initializing the bus and the controller, we can now + * register I3C devices from defslvs list. + */ + master->init_done = true; + + return 0; + +err_del_dev: + device_del(&master->dev); + +err_cleanup_bus: + i3c_master_bus_cleanup(master); + + put_device(&master->dev); + + return ret; +} +EXPORT_SYMBOL_GPL(i3c_secondary_master_register); + /** * i3c_master_unregister() - unregister an I3C master * @master: master used to send frames on the bus diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h index f5ba82c390bc..5124ff4831eb 100644 --- a/include/linux/i3c/master.h +++ b/include/linux/i3c/master.h @@ -534,6 +534,9 @@ int i3c_master_set_info(struct i3c_master_controller *master, int i3c_master_register(struct i3c_master_controller *master, struct device *parent, const struct i3c_master_controller_ops *ops); +int i3c_secondary_master_register(struct i3c_master_controller *master, + struct device *parent, + const struct i3c_master_controller_ops *ops); int i3c_master_unregister(struct i3c_master_controller *master); /**
add i3c_secondary_master_register which is used to register secondary masters. Signed-off-by: Parshuram Thombare <pthombar@cadence.com> --- drivers/i3c/master.c | 154 ++++++++++++++++++++++++++++++++++++- include/linux/i3c/master.h | 3 + 2 files changed, 156 insertions(+), 1 deletion(-)