From: Boris Brezillon <boris.brezillon@collabora.com>
To: Przemyslaw Gaj <pgaj@cadence.com>
Cc: linux-i3c@lists.infradead.org, vitor.soares@synopsys.com,
rafalc@cadence.com, agolec@cadence.com, bbrezillon@kernel.org
Subject: Re: [PATCH v5 4/7] i3c: Add support for mastership request to I3C subsystem
Date: Sat, 6 Jul 2019 11:00:06 +0200 [thread overview]
Message-ID: <20190706110006.767571b8@collabora.com> (raw)
In-Reply-To: <1561236905-8901-5-git-send-email-pgaj@cadence.com>
On Sat, 22 Jun 2019 21:55:02 +0100
Przemyslaw Gaj <pgaj@cadence.com> wrote:
> diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
> index 86b7b44..cdfc5bf 100644
> --- a/drivers/i3c/internals.h
> +++ b/drivers/i3c/internals.h
> @@ -14,6 +14,10 @@ extern struct bus_type i3c_bus_type;
>
> void i3c_bus_normaluse_lock(struct i3c_bus *bus);
> void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
> +void i3c_bus_maintenance_lock(struct i3c_bus *bus);
> +void i3c_bus_maintenance_unlock(struct i3c_bus *bus);
I don't think you need to expose the maintenance lock helpers, looks
like they're only used in master.c.
> +int i3c_master_acquire_bus_ownership(struct i3c_master_controller *master);
> +
>
> int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
> struct i3c_priv_xfer *xfers,
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index cbace14..3b44e66 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -93,6 +93,18 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus)
> up_read(&bus->lock);
> }
>
> +/*
> + * i3c_bus_downgrade_maintenance_lock - Downgrade the bus lock to normal
> + * operation
> + *
> + * Should be called when a maintenance operation is done and normal
> + * operation is planned. See i3c_bus_maintenance_lock() and
> + * i3c_bus_normaluse_lock() for more details.
> + */
> +static void i3c_bus_downgrade_maintenance_lock(struct i3c_bus *bus)
> +{
> + downgrade_write(&bus->lock);
> +}
Missing blank line.
> static struct i3c_master_controller *dev_to_i3cmaster(struct device *dev)
> {
> return container_of(dev, struct i3c_master_controller, dev);
> @@ -341,6 +353,22 @@ static int i3c_device_probe(struct device *dev)
> return driver->probe(i3cdev);
> }
>
> +static int
> +i3c_master_enable_mr_events_locked(struct i3c_master_controller *master)
> +{
> + if (!master->ops->enable_mr_events)
> + return -ENOTSUPP;
> +
> + return master->ops->enable_mr_events(master);
> +}
> +
> +static void i3c_master_disable_mr_events(struct i3c_master_controller *master)
> +{
> + if (!master->ops->disable_mr_events)
> + return;
> +
> + master->ops->disable_mr_events(master);
> +}
Missing blank line.
> static int i3c_device_remove(struct device *dev)
> {
> struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> @@ -462,6 +490,42 @@ static int i3c_bus_init(struct i3c_bus *i3cbus)
> return 0;
> }
>
> +static int
> +i3c_master_request_mastership_locked(struct i3c_master_controller *master)
> +{
> + if (WARN_ON(master->init_done &&
> + !rwsem_is_locked(&master->bus.lock)))
Hm, that looks suspicious. The lock should be held even if ->init_done
is false. Can you explain the difference between init_done
and !init_done?
> + return -EINVAL;
> +
> + if (!master->ops->request_mastership)
> + return -ENOTSUPP;
> +
> + return master->ops->request_mastership(master);
> +}
> +
> +static int i3c_master_owns_bus(struct i3c_master_controller *master)
> +{
> + return (master->bus.cur_master == master->this);
> +}
> +
> +int i3c_master_acquire_bus_ownership(struct i3c_master_controller *master)
> +{
> + int ret;
> +
> + if (!i3c_master_owns_bus(master)) {
> + i3c_bus_normaluse_unlock(&master->bus);
> + i3c_bus_maintenance_lock(&master->bus);
> +
> + ret = i3c_master_request_mastership_locked(master);
> + if (ret) {
> + i3c_bus_maintenance_unlock(&master->bus);
> + return ret;
> + }
> + i3c_bus_downgrade_maintenance_lock(&master->bus);
> + }
I think this block deserves a comment: the lock/unlock dance is far
from obvious.
> +
> + return 0;
> +}
> static const char * const i3c_bus_mode_strings[] = {
> [I3C_BUS_MODE_PURE] = "pure",
> [I3C_BUS_MODE_MIXED_FAST] = "mixed-fast",
> @@ -636,6 +700,22 @@ i3c_master_alloc_i2c_dev(struct i3c_master_controller *master,
> return dev;
> }
>
> +static struct i2c_dev_desc *
> +i3c_master_alloc_i2c_dev_no_boardinfo(struct i3c_master_controller *master,
> + u16 addr, u8 lvr)
> +{
> + struct i2c_dev_desc *dev;
> +
> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> + if (!dev)
> + return ERR_PTR(-ENOMEM);
> +
> + dev->common.master = master;
> + dev->addr = addr;
> + dev->lvr = lvr;
> +
> + return dev;
> +}
> static void *i3c_ccc_cmd_dest_init(struct i3c_ccc_cmd_dest *dest, u8 addr,
> u16 payloadlen)
> {
> @@ -705,6 +785,8 @@ i3c_master_find_i2c_dev_by_addr(const struct i3c_master_controller *master,
> struct i2c_dev_desc *dev;
>
> i3c_bus_for_each_i2cdev(&master->bus, dev) {
> + if (!dev->boardinfo)
> + continue;
Blank line here, please.
> if (dev->boardinfo->base.addr == addr)
> return dev;
> }
> @@ -1478,7 +1560,8 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
> return;
>
> i3c_bus_for_each_i3cdev(&master->bus, desc) {
> - if (desc->dev || !desc->info.dyn_addr || desc == master->this)
> + if (desc->dev || !desc->info.dyn_addr ||
> + desc == master->this || !desc->info.pid)
Does the new logic trigger a case where pid == 0? The check looks good
anyway, but I'd rather have it in a separate commit unless you have a
good explanation on why this couldn't happen before this patch.
> continue;
>
> desc->dev = kzalloc(sizeof(*desc->dev), GFP_KERNEL);
> @@ -1504,6 +1587,69 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
> }
> }
>
> +static struct i2c_dev_boardinfo *
> +i3c_master_find_i2c_boardinfo(const struct i3c_master_controller *master,
> + u16 addr, u8 lvr)
> +{
> + struct i2c_dev_boardinfo *i2cboardinfo;
> +
> + list_for_each_entry(i2cboardinfo, &master->boardinfo.i2c, node) {
> + if (i2cboardinfo->base.addr == addr &&
> + i2cboardinfo->lvr == lvr)
> + return i2cboardinfo;
> + }
> +
> + return NULL;
> +}
> +
> +static void
> +i3c_master_register_new_i2c_devs(struct i3c_master_controller *master)
> +{
> + struct i2c_adapter *adap = i3c_master_to_i2c_adapter(master);
> + struct i2c_dev_desc *i2cdev;
> +
> + if (!master->init_done)
> + return;
> +
> + i3c_bus_for_each_i2cdev(&master->bus, i2cdev) {
> +
You can drop this blank line.
> + if (i2cdev->dev)
> + continue;
> +
Add a comment explaining why.
> + if (!i2cdev->boardinfo)
> + continue;
> +
> + i2cdev->dev = i2c_new_device(adap, &i2cdev->boardinfo->base);
> + }
> +}
> +
> +static int i3c_master_get_accmst_locked(struct i3c_master_controller *master,
> + u8 addr)
> +{
> + 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, 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)
> + goto out;
> +
> + if (dest.payload.len != sizeof(*accmst))
> + ret = -EIO;
> +
> +out:
> + i3c_ccc_cmd_dest_cleanup(&dest);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_get_accmst_locked);
Add a blank line here (please run checkpatch --strict, it should
complain about such coding style issues).
> /**
> * i3c_master_do_daa() - do a DAA (Dynamic Address Assignment)
> * @master: master doing the DAA
> @@ -1548,10 +1694,6 @@ static int i3c_master_set_info(struct i3c_master_controller *master,
> 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)
> - return -EINVAL;
> -
> if (master->this)
> return -EINVAL;
>
> @@ -1560,7 +1702,8 @@ static int i3c_master_set_info(struct i3c_master_controller *master,
> return PTR_ERR(i3cdev);
>
> master->this = i3cdev;
> - master->bus.cur_master = master->this;
> + if (!secondary)
> + master->bus.cur_master = master->this;
>
> ret = i3c_master_attach_i3c_dev(master, i3cdev);
> if (ret)
> @@ -1601,37 +1744,7 @@ static void i3c_master_detach_free_devs(struct i3c_master_controller *master)
> }
> }
>
[...]
> +/**
> + * i3c_master_bus_init() - initialize an I3C bus
> + * @master: main master initializing the bus
> + *
> + * This function is following all initialisation steps described in the I3C
> + * specification:
> + *
> + * 1. Attach I2C and statically defined I3C devs to the master so that the
> + * master can fill its internal device table appropriately
> + *
> + * 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)
> + *
> + * 3. Instruct all devices on the bus to drop their dynamic address. This is
> + * particularly important when the bus was previously configured by someone
> + * else (for example the bootloader)
> + *
> + * 4. Disable all slave events.
> + *
> + * 5. Pre-assign dynamic addresses requested by the FW with SETDASA for I3C
> + * devices that have a static address
> + *
> + * 6. Do a DAA (Dynamic Address Assignment) to assign dynamic addresses to all
> + * remaining I3C devices
> + *
> + * Once this is done, all I3C and I2C devices should be usable.
> + *
> + * Return: a 0 in case of success, an negative error code otherwise.
> + */
> +static int i3c_master_bus_init(struct i3c_master_controller *master)
> +{
> + struct i3c_dev_desc *i3cdev;
> + int ret;
> +
> + /*
> + * First attach all devices with static definitions provided by the
> + * FW.
> + */
> + ret = i3c_master_attach_static_devs(master);
> + if (ret)
> + goto err_detach_devs;
> /*
> * Now execute the controller specific ->bus_init() routine, which
> * might configure its internal logic to match the bus limitations.
> @@ -1780,45 +1926,76 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
> }
The i3c_master_attach_static_devs() split can be done in a separate
commit. Please try to split things as much as possible to ease review.
>
> /**
> - * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus
> - * @master: master used to send frames on the bus
> - * @addr: I3C slave dynamic address assigned to the device
> + * i3c_master_add_i2c_dev_locked() - add an I2C slave to the bus
> + * @master: master used to register I2C device
> + * @addr: I2C device address
> + * @lvr: legacy virtual register value
> *
> - * This function is instantiating an I3C device object and adding it to the
> - * I3C device list. All device information are automatically retrieved using
> - * standard CCC commands.
> - *
> - * The I3C device object is returned in case the master wants to attach
> - * private data to it using i3c_dev_set_master_data().
> + * This function is instantiating an I2C device object and adding it to the
> + * I2C device list.
> *
> * This function must be called with the bus lock held in write mode.
> *
> * Return: a 0 in case of success, an negative error code otherwise.
> */
> -int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
> - u8 addr)
> +int i3c_master_add_i2c_dev_locked(struct i3c_master_controller *master,
> + u16 addr, u8 lvr)
> {
> - struct i3c_device_info info = { .dyn_addr = addr };
> - struct i3c_dev_desc *newdev, *olddev;
> - u8 old_dyn_addr = addr, expected_dyn_addr;
> - struct i3c_ibi_setup ibireq = { };
> - bool enable_ibi = false;
> + enum i3c_addr_slot_status status;
> + struct i2c_dev_desc *i2cdev;
> int ret;
>
> if (!master)
> return -EINVAL;
>
> - newdev = i3c_master_alloc_i3c_dev(master, &info);
> - if (IS_ERR(newdev))
> - return PTR_ERR(newdev);
> + status = i3c_bus_get_addr_slot_status(&master->bus,
> + addr);
> + if (status != I3C_ADDR_SLOT_FREE)
> + return -EBUSY;
>
> - ret = i3c_master_attach_i3c_dev(master, newdev);
> - if (ret)
> + i3c_bus_set_addr_slot_status(&master->bus, addr,
> + I3C_ADDR_SLOT_I2C_DEV);
> +
> + i2cdev = i3c_master_alloc_i2c_dev_no_boardinfo(master, addr, lvr);
> +
> + if (IS_ERR(i2cdev)) {
> + ret = PTR_ERR(i2cdev);
> + goto err_free_dev;
> + }
> +
> + i2cdev->boardinfo = i3c_master_find_i2c_boardinfo(master, addr, lvr);
> +
> + ret = i3c_master_attach_i2c_dev(master, i2cdev);
> +
> + if (ret) {
> + ret = PTR_ERR(i2cdev);
> goto err_free_dev;
> + }
> +
> + return 0;
> +
> +err_free_dev:
> + i3c_bus_set_addr_slot_status(&master->bus, addr,
> + I3C_ADDR_SLOT_FREE);
> + i3c_master_free_i2c_dev(i2cdev);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_add_i2c_dev_locked);
Same here.
> +
> +static int
> +i3c_master_retrieve_info_and_reuse(struct i3c_master_controller *master,
> + struct i3c_dev_desc *newdev)
> +{
> + struct i3c_dev_desc *olddev;
> + u8 old_dyn_addr = newdev->info.dyn_addr, expected_dyn_addr;
> + struct i3c_ibi_setup ibireq = { };
> + bool enable_ibi = false;
> + int ret;
>
> ret = i3c_master_retrieve_dev_info(newdev);
> if (ret)
> - goto err_detach_dev;
> + return ret;
>
> olddev = i3c_master_search_i3c_dev_duplicate(newdev);
> if (olddev) {
> @@ -1857,7 +2034,7 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
>
> ret = i3c_master_reattach_i3c_dev(newdev, old_dyn_addr);
> if (ret)
> - goto err_detach_dev;
> + return ret;
>
> /*
> * Depending on our previous state, the expected dynamic address might
> @@ -1920,6 +2097,50 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
> }
>
> return 0;
> +}
> +
> +/**
> + * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus
> + * @master: master used to send frames on the bus
> + * @addr: I3C slave dynamic address assigned to the device
> + *
> + * This function is instantiating an I3C device object and adding it to the
> + * I3C device list. All device information are automatically retrieved using
> + * standard CCC commands.
> + *
> + * The I3C device object is returned in case the master wants to attach
> + * private data to it using i3c_dev_set_master_data().
> + *
> + * This function must be called with the bus lock held in write mode.
> + *
> + * Return: a 0 in case of success, an negative error code otherwise.
> + */
> +int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
> + u8 addr)
> +{
> + struct i3c_device_info info = { .dyn_addr = addr };
> + struct i3c_dev_desc *newdev;
> + int ret;
> +
> + if (!master)
> + return -EINVAL;
> +
> + newdev = i3c_master_alloc_i3c_dev(master, &info);
> + if (IS_ERR(newdev))
> + return PTR_ERR(newdev);
> +
> + ret = i3c_master_attach_i3c_dev(master, newdev);
> + if (ret)
> + goto err_free_dev;
> +
> + if (i3c_master_owns_bus(master)) {
> + ret = i3c_master_retrieve_info_and_reuse(master, newdev);
> + if (ret)
> + goto err_detach_dev;
> + } else
> + master->want_to_acquire_bus = true;
Please add curly braces.
> +
> + return 0;
>
> err_detach_dev:
> if (newdev->dev && newdev->dev->desc)
> @@ -2101,11 +2322,15 @@ static int i3c_master_i2c_adapter_xfer(struct i2c_adapter *adap,
> }
>
> i3c_bus_normaluse_lock(&master->bus);
> + ret = i3c_master_acquire_bus_ownership(master);
> + if (ret)
> + goto err_unlock_bus;
> dev = i3c_master_find_i2c_dev_by_addr(master, addr);
> if (!dev)
> ret = -ENOENT;
> else
> ret = master->ops->i2c_xfers(dev, xfers, nxfers);
> +err_unlock_bus:
> i3c_bus_normaluse_unlock(&master->bus);
>
> return ret ? ret : nxfers;
> @@ -2144,9 +2369,12 @@ static int i3c_master_i2c_adapter_init(struct i3c_master_controller *master)
> * We silently ignore failures here. The bus should keep working
> * correctly even if one or more i2c devices are not registered.
> */
> - i3c_bus_for_each_i2cdev(&master->bus, i2cdev)
> + i3c_bus_for_each_i2cdev(&master->bus, i2cdev) {
> + if (!i2cdev->boardinfo)
> + continue;
> i2cdev->dev = i2c_new_device(adap, &i2cdev->boardinfo->base);
>
> + }
> return 0;
> }
[...]
> /**
> * i3c_master_init() - initializes all the structures required by I3C master
> * @master: master used to send frames on the bus
> @@ -2417,9 +2712,6 @@ int i3c_master_init(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)
> @@ -2432,6 +2724,7 @@ int i3c_master_init(struct i3c_master_controller *master,
> master->dev.release = i3c_masterdev_release;
> master->ops = ops;
> master->secondary = secondary;
> + master->want_to_acquire_bus = secondary;
Why do we need to set it to true here? Actually, I'm not even sure you
need that variable. Looks like it's only ever set, nothing reads it.
> INIT_LIST_HEAD(&master->boardinfo.i2c);
> INIT_LIST_HEAD(&master->boardinfo.i3c);
>
> @@ -2488,6 +2781,92 @@ void i3c_master_cleanup(struct i3c_master_controller *master)
> EXPORT_SYMBOL_GPL(i3c_master_cleanup);
>
Overall, I like this version much better than the previous ones. Still
need some refinements though (and more documentation/comments to explain
how that works).
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
next prev parent reply other threads:[~2019-07-06 9:00 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-22 20:54 [PATCH v5 0/7] Add the I3C mastership request Przemyslaw Gaj
2019-06-22 20:54 ` [PATCH v5 1/7] i3c: add addr and lvr to i2c_dev_desc structure Przemyslaw Gaj
2019-06-22 20:55 ` [PATCH v5 2/7] i3c: split i3c_master_register into init - register pair Przemyslaw Gaj
2019-07-06 8:48 ` Boris Brezillon
2019-12-02 10:31 ` Przemyslaw Gaj
2019-06-22 20:55 ` [PATCH v5 3/7] i3c: export i3c_bus_set_mode function Przemyslaw Gaj
2019-07-06 7:39 ` Boris Brezillon
2019-06-22 20:55 ` [PATCH v5 4/7] i3c: Add support for mastership request to I3C subsystem Przemyslaw Gaj
2019-07-06 9:00 ` Boris Brezillon [this message]
2019-07-10 18:04 ` Vitor Soares
2019-07-11 5:28 ` Przemyslaw Gaj
2019-07-11 10:11 ` Vitor Soares
2019-07-12 10:10 ` Przemyslaw Gaj
2019-07-12 11:28 ` Vitor Soares
2019-08-11 10:17 ` Boris Brezillon
2019-08-12 13:55 ` Vitor Soares
2019-08-12 14:55 ` Boris Brezillon
2019-08-22 11:08 ` Vitor Soares
2019-06-22 20:55 ` [PATCH v5 5/7] i3c: master: cdns: add support for mastership request to Cadence I3C master driver Przemyslaw Gaj
2019-06-22 20:55 ` [PATCH v5 6/7] i3c: master: Add module author Przemyslaw Gaj
2019-06-22 20:55 ` [PATCH v5 7/7] MAINTAINERS: add myself as co-maintainer of i3c subsystem Przemyslaw Gaj
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190706110006.767571b8@collabora.com \
--to=boris.brezillon@collabora.com \
--cc=agolec@cadence.com \
--cc=bbrezillon@kernel.org \
--cc=linux-i3c@lists.infradead.org \
--cc=pgaj@cadence.com \
--cc=rafalc@cadence.com \
--cc=vitor.soares@synopsys.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).