| Message ID | 20180330074751.25987-2-boris.brezillon@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add the I3C subsystem | expand |
SGkgQm9yaXMNCg0KSXQgbG9va3MgZ3JlYXQsIGp1c3Qgb25lIGNvbW1lbnQgdG8gREVGU0xWUyBj b21tYW5kOg0KDQrvu79PbiA2LzQvMTgsIDk6MzIgQU0sICJCb3JpcyBCcmV6aWxsb24iIDxib3Jp cy5icmV6aWxsb25AYm9vdGxpbi5jb20+IHdyb3RlOg0KDQogICAgK2ludCBpM2NfbWFzdGVyX2Rl ZnNsdnNfbG9ja2VkKHN0cnVjdCBpM2NfbWFzdGVyX2NvbnRyb2xsZXIgKm1hc3RlcikNCiAgICAr ew0KICAgICsJc3RydWN0IGkzY19jY2NfY21kX2Rlc3QgZGVzdCA9IHsNCiAgICArCQkuYWRkciA9 IEkzQ19CUk9BRENBU1RfQUREUiwNCiAgICArCX07DQogICAgKwlzdHJ1Y3QgaTNjX2NjY19jbWQg Y21kID0gew0KICAgICsJCS5pZCA9IEkzQ19DQ0NfREVGU0xWUywNCiAgICArCQkuZGVzdHMgPSAm ZGVzdCwNCiAgICArCQkubmRlc3RzID0gMSwNCiAgICArCX07DQogICAgKwlzdHJ1Y3QgaTNjX2Nj Y19kZWZzbHZzICpkZWZzbHZzOw0KICAgICsJc3RydWN0IGkzY19jY2NfZGV2X2Rlc2MgKmRlc2M7 DQogICAgKwlzdHJ1Y3QgaTNjX2RldmljZSAqaTNjZGV2Ow0KICAgICsJc3RydWN0IGkyY19kZXZp Y2UgKmkyY2RldjsNCiAgICArCXN0cnVjdCBpM2NfYnVzICpidXM7DQogICAgKwlib29sIHNlbmQg PSBmYWxzZTsNCiAgICArCWludCBuZGV2cyA9IDAsIHJldDsNCiAgICArDQogICAgKwlpZiAoIW1h c3RlcikNCiAgICArCQlyZXR1cm4gLUVJTlZBTDsNCiAgICArDQogICAgKwlidXMgPSBpM2NfbWFz dGVyX2dldF9idXMobWFzdGVyKTsNCiAgICArCWkzY19idXNfZm9yX2VhY2hfaTNjZGV2KGJ1cywg aTNjZGV2KSB7DQogICAgKwkJbmRldnMrKzsNCiAgICArCQlpZiAoSTNDX0JDUl9ERVZJQ0VfUk9M RShpM2NkZXYtPmluZm8uYmNyKSA9PSBJM0NfQkNSX0kzQ19NQVNURVIpDQogICAgKwkJCXNlbmQg PSB0cnVlOw0KICAgICsJfQ0KICAgICsNCiAgICArCS8qIE5vIG90aGVyIG1hc3RlciBvbiB0aGUg YnVzLCBza2lwIERFRlNMVlMuICovDQogICAgKwlpZiAoIXNlbmQpDQogICAgKwkJcmV0dXJuIDA7 DQogICAgKw0KICAgICsJaTNjX2J1c19mb3JfZWFjaF9pMmNkZXYoYnVzLCBpMmNkZXYpDQogICAg KwkJbmRldnMrKzsNCiAgICArDQogICAgKwlkZXN0LnBheWxvYWQubGVuID0gc2l6ZW9mKCpkZWZz bHZzKSArDQogICAgKwkJCSAgICgobmRldnMgLSAxKSAqIHNpemVvZihzdHJ1Y3QgaTNjX2NjY19k ZXZfZGVzYykpOw0KICAgICsJZGVmc2x2cyA9IGt6YWxsb2MoZGVzdC5wYXlsb2FkLmxlbiwgR0ZQ X0tFUk5FTCk7DQogICAgKwlpZiAoIWRlZnNsdnMpDQogICAgKwkJcmV0dXJuIC1FTk9NRU07DQog ICAgKw0KICAgICsJZGVzdC5wYXlsb2FkLmRhdGEgPSBkZWZzbHZzOw0KICAgICsNCiAgICArCWRl ZnNsdnMtPmNvdW50ID0gbmRldnM7DQogICAgKwlkZWZzbHZzLT5tYXN0ZXIuYmNyID0gbWFzdGVy LT50aGlzLT5pbmZvLmJjcjsNCiAgICArCWRlZnNsdnMtPm1hc3Rlci5kY3IgPSBtYXN0ZXItPnRo aXMtPmluZm8uZGNyOw0KICAgICsJZGVmc2x2cy0+bWFzdGVyLmR5bl9hZGRyID0gbWFzdGVyLT50 aGlzLT5pbmZvLmR5bl9hZGRyOw0KICAgICsJZGVmc2x2cy0+bWFzdGVyLnN0YXRpY19hZGRyID0g STNDX0JST0FEQ0FTVF9BRERSOw0KICAgICsNCiAgICArCWRlc2MgPSBkZWZzbHZzLT5zbGF2ZXM7 DQogICAgKwlpM2NfYnVzX2Zvcl9lYWNoX2kyY2RldihidXMsIGkyY2Rldikgew0KICAgICsJCWRl c2MtPmx2ciA9IGkyY2Rldi0+bHZyOw0KICAgICsJCWRlc2MtPnN0YXRpY19hZGRyID0gaTJjZGV2 LT5pbmZvLmFkZHI7DQogICAgKwkJZGVzYysrOw0KICAgICsJfQ0KICAgICsNCiAgICArCWkzY19i dXNfZm9yX2VhY2hfaTNjZGV2KGJ1cywgaTNjZGV2KSB7DQogICAgKwkJLyogU2tpcCB0aGUgSTND IGRldiByZXByZXNlbnRpbmcgdGhpcyBtYXN0ZXIuICovDQogICAgKwkJaWYgKGkzY2RldiA9PSBt YXN0ZXItPnRoaXMpDQogICAgKwkJCWNvbnRpbnVlOw0KICAgICsNCiAgICArCQlkZXNjLT5iY3Ig PSBpM2NkZXYtPmluZm8uYmNyOw0KICAgICsJCWRlc2MtPmRjciA9IGkzY2Rldi0+aW5mby5kY3I7 DQogICAgKwkJZGVzYy0+ZHluX2FkZHIgPSBpM2NkZXYtPmluZm8uZHluX2FkZHI7DQogICAgKwkJ ZGVzYy0+c3RhdGljX2FkZHIgPSBpM2NkZXYtPmluZm8uc3RhdGljX2FkZHI7DQogICAgKwkJZGVz YysrOw0KICAgICsJfQ0KICAgICsNCiAgICArCXJldCA9IGkzY19tYXN0ZXJfc2VuZF9jY2NfY21k X2xvY2tlZChtYXN0ZXIsICZjbWQpOw0KICAgICsJa2ZyZWUoZGVmc2x2cyk7DQogICAgKw0KICAg ICsJcmV0dXJuIHJldDsNCiAgICArfQ0KDQpZb3Ugc2hvdWxkIHNoaWZ0IGFsbCB0aGUgYWRkcmVz c2VzIChkeW5hbWljIGFuZCBzdGF0aWMpIG9uZSBiaXQgbGVmdC4gDQpBZGRyZXNzZXMgYXJlIHN0 b3JlZCBvbiBiaXRzIFs3OjFdLCB0aGlzIGlzIGRlc2NyaWJlZCBpbiBNSVBJIHNwZWMsIA0Kc2Vj dGlvbiA1LjEuOS4zLjcgRGVmaW5lIExpc3Qgb2YgU2xhdmVzIChERUZTTFZTKQ0KDQpSZWdhcmRz LA0KUHJ6ZW15c2xhdyBHYWoNCg0K -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 4 Jun 2018 09:11:25 +0000 Przemyslaw Gaj <pgaj@cadence.com> wrote: > Hi Boris > > It looks great, just one comment to DEFSLVS command: > > On 6/4/18, 9:32 AM, "Boris Brezillon" <boris.brezillon@bootlin.com> wrote: > > +int i3c_master_defslvs_locked(struct i3c_master_controller *master) > +{ > + struct i3c_ccc_cmd_dest dest = { > + .addr = I3C_BROADCAST_ADDR, > + }; > + struct i3c_ccc_cmd cmd = { > + .id = I3C_CCC_DEFSLVS, > + .dests = &dest, > + .ndests = 1, > + }; > + struct i3c_ccc_defslvs *defslvs; > + struct i3c_ccc_dev_desc *desc; > + struct i3c_device *i3cdev; > + struct i2c_device *i2cdev; > + struct i3c_bus *bus; > + bool send = false; > + int ndevs = 0, ret; > + > + if (!master) > + return -EINVAL; > + > + bus = i3c_master_get_bus(master); > + i3c_bus_for_each_i3cdev(bus, i3cdev) { > + ndevs++; > + if (I3C_BCR_DEVICE_ROLE(i3cdev->info.bcr) == I3C_BCR_I3C_MASTER) > + send = true; > + } > + > + /* No other master on the bus, skip DEFSLVS. */ > + if (!send) > + return 0; > + > + i3c_bus_for_each_i2cdev(bus, i2cdev) > + ndevs++; > + > + dest.payload.len = sizeof(*defslvs) + > + ((ndevs - 1) * sizeof(struct i3c_ccc_dev_desc)); > + defslvs = kzalloc(dest.payload.len, GFP_KERNEL); > + if (!defslvs) > + return -ENOMEM; > + > + dest.payload.data = defslvs; > + > + defslvs->count = ndevs; > + defslvs->master.bcr = master->this->info.bcr; > + defslvs->master.dcr = master->this->info.dcr; > + defslvs->master.dyn_addr = master->this->info.dyn_addr; > + defslvs->master.static_addr = I3C_BROADCAST_ADDR; > + > + desc = defslvs->slaves; > + i3c_bus_for_each_i2cdev(bus, i2cdev) { > + desc->lvr = i2cdev->lvr; > + desc->static_addr = i2cdev->info.addr; > + desc++; > + } > + > + i3c_bus_for_each_i3cdev(bus, i3cdev) { > + /* Skip the I3C dev representing this master. */ > + if (i3cdev == master->this) > + continue; > + > + desc->bcr = i3cdev->info.bcr; > + desc->dcr = i3cdev->info.dcr; > + desc->dyn_addr = i3cdev->info.dyn_addr; > + desc->static_addr = i3cdev->info.static_addr; > + desc++; > + } > + > + ret = i3c_master_send_ccc_cmd_locked(master, &cmd); > + kfree(defslvs); > + > + return ret; > +} > > You should shift all the addresses (dynamic and static) one bit left. > Addresses are stored on bits [7:1], this is described in MIPI spec, > section 5.1.9.3.7 Define List of Slaves (DEFSLVS) Oops, indeed. I will fix that. Thanks, Boris -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Boris, > +/** > + * struct i3c_priv_xfer - I3C SDR private transfer > + * @rnw: encodes the transfer direction. true for a read, false for a write > + * @len: transfer length in bytes of the transfer > + * @data: input/output buffer > + */ > +struct i3c_priv_xfer { > + bool rnw; > + u16 len; > + union { > + void *in; > + const void *out; > + } data; So, this is probably where most payloads end up? I didn't notice any sign of DMA in these patches, but given my experiences, DMA will come sooner or later. And in I2C, this was problematic because then a lot of drivers were in the wild getting their buffers from everywhere (stack!). We now have an opt-in approach to mark buffers as DMA-safe. I don't know if typical I3C transfers will be similar to I2C with usually small payloads where DMA usually makes not much sense. Yet, I think, that it might be a good idea to think about how this shall be handled with I3C right away. Maybe simply enforcing buffers to be DMA-safe. Or whatever. A clear rule on that might save you hazzle later. Regards, Wolfram
On Thu, 14 Jun 2018 13:19:42 +0900 Wolfram Sang <wsa@the-dreams.de> wrote: > Hi Boris, > > > +/** > > + * struct i3c_priv_xfer - I3C SDR private transfer > > + * @rnw: encodes the transfer direction. true for a read, false for a write > > + * @len: transfer length in bytes of the transfer > > + * @data: input/output buffer > > + */ > > +struct i3c_priv_xfer { > > + bool rnw; > > + u16 len; > > + union { > > + void *in; > > + const void *out; > > + } data; > > So, this is probably where most payloads end up? > > I didn't notice any sign of DMA in these patches, but given my > experiences, DMA will come sooner or later. And in I2C, this was > problematic because then a lot of drivers were in the wild getting their > buffers from everywhere (stack!). We now have an opt-in approach to mark > buffers as DMA-safe. > > I don't know if typical I3C transfers will be similar to I2C with > usually small payloads where DMA usually makes not much sense. Yet, I > think, that it might be a good idea to think about how this shall be > handled with I3C right away. Maybe simply enforcing buffers to be > DMA-safe. Or whatever. > > A clear rule on that might save you hazzle later. I completely agree. I'll clarify that and for people to pass DMA-able buffers to this struct (just as the SPI framework does). Note that we don't really have a way to ensure that the buffer is actually DMA-safe from the core, because this notion is architecture/SoC dependent. > > Regards, > > Wolfram > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > I don't know if typical I3C transfers will be similar to I2C with > > usually small payloads where DMA usually makes not much sense. Yet, I > > think, that it might be a good idea to think about how this shall be > > handled with I3C right away. Maybe simply enforcing buffers to be > > DMA-safe. Or whatever. > > > > A clear rule on that might save you hazzle later. > > I completely agree. I'll clarify that and for people to pass DMA-able > buffers to this struct (just as the SPI framework does). Note that we Ok, cool! > don't really have a way to ensure that the buffer is actually > DMA-safe from the core, because this notion is architecture/SoC > dependent. True, we can't ensure it at the core. Still, a documented rule will make it clear that everything else is considered a bug. Much better than a gray area you have to deal with later. Kind regards, Wolfram
Hi Boris, On Friday 30 March 2018 01:17 PM, Boris Brezillon wrote: > Add core infrastructure to support I3C in Linux and document it. > > This infrastructure is not complete yet and will be extended over > time. > > There are a few design choices that are worth mentioning because they > impact the way I3C device drivers can interact with their devices: > > - all functions used to send I3C/I2C frames must be called in > non-atomic context. Mainly done this way to ease implementation, but > this is still open to discussion. Please let me know if you think > it's worth considering an asynchronous model here > - the bus element is a separate object and is not implicitly described > by the master (as done in I2C). The reason is that I want to be able > to handle multiple master connected to the same bus and visible to > Linux. > In this situation, we should only have one instance of the device and > not one per master, and sharing the bus object would be part of the > solution to gracefully handle this case. > I'm not sure we will ever need to deal with multiple masters > controlling the same bus and exposed under Linux, but separating the > bus and master concept is pretty easy, hence the decision to do it > like that. There can only be one current master in I3C, so not sure of this scenario. But agree with bus and master separation. > The other benefit of separating the bus and master concepts is that > master devices appear under the bus directory in sysfs. > - I2C backward compatibility has been designed to be transparent to I2C > drivers and the I2C subsystem. The I3C master just registers an I2C > adapter which creates a new I2C bus. I'd say that, from a > representation PoV it's not ideal because what should appear as a > single I3C bus exposing I3C and I2C devices here appears as 2 > different busses connected to each other through the parenting (the > I3C master is the parent of the I2C and I3C busses). > On the other hand, I don't see a better solution if we want something > that is not invasive. > > Missing features in this preliminary version: > - I3C HDR modes are not supported > - no support for multi-master and the associated concepts (mastership > handover, support for secondary masters, ...) > - I2C devices can only be described using DT because this is the only > use case I have. However, the framework can easily be extended with > ACPI and board info support > - I3C slave framework. This has been completely omitted, but shouldn't > have a huge impact on the I3C framework because I3C slaves don't see > the whole bus, it's only about handling master requests and generating > IBIs. Some of the struct, constant and enum definitions could be > shared, but most of the I3C slave framework logic will be different > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > diff --git a/drivers/Makefile b/drivers/Makefile > index 24cd47014657..999239dc29d4 100644 > --- a/drivers/Makefile > +++ b/drivers/Makefile > @@ -111,7 +111,7 @@ obj-$(CONFIG_SERIO) += input/serio/ > obj-$(CONFIG_GAMEPORT) += input/gameport/ > obj-$(CONFIG_INPUT) += input/ > obj-$(CONFIG_RTC_LIB) += rtc/ > -obj-y += i2c/ media/ > +obj-y += i2c/ i3c/ media/ > obj-$(CONFIG_PPS) += pps/ > obj-y += ptp/ > obj-$(CONFIG_W1) += w1/ > diff --git a/drivers/i3c/Kconfig b/drivers/i3c/Kconfig > new file mode 100644 > index 000000000000..cf3752412ae9 > --- /dev/null > +++ b/drivers/i3c/Kconfig > @@ -0,0 +1,24 @@ > +# SPDX-License-Identifier: GPL-2.0 > + > +menuconfig I3C > + tristate "I3C support" > + select I2C > + help > + I3C is a serial protocol standardized by the MIPI alliance. > + > + It's supposed to be backward compatible with I2C while providing > + support for high speed transfers and native interrupt support > + without the need for extra pins. > + > + The I3C protocol also standardizes the slave device types and is > + mainly design to communicate with sensors. designed > + > + If you want I3C support, you should say Y here and also to the > + specific driver for your bus adapter(s) below. > + > + This I3C support can also be built as a module. If so, the module > + will be called i3c. > + > diff --git a/drivers/i3c/core.c b/drivers/i3c/core.c > new file mode 100644 > index 000000000000..d6d938a785a9 > --- /dev/null > +++ b/drivers/i3c/core.c > +static ssize_t bcr_show(struct device *dev, > + struct device_attribute *da, > + char *buf) > +{ > + struct i3c_device *i3cdev = dev_to_i3cdev(dev); > + struct i3c_bus *bus = i3c_device_get_bus(i3cdev); > + ssize_t ret; > + > + i3c_bus_normaluse_lock(bus); > + ret = sprintf(buf, "%x\n", i3cdev->info.bcr); > + i3c_bus_normaluse_unlock(bus); > + > + return ret; > +} > +static DEVICE_ATTR_RO(bcr); > + > +static ssize_t dcr_show(struct device *dev, > + struct device_attribute *da, > + char *buf) > +{ > + struct i3c_device *i3cdev = dev_to_i3cdev(dev); > + struct i3c_bus *bus = i3c_device_get_bus(i3cdev); > + ssize_t ret; > + > + i3c_bus_normaluse_lock(bus); > + ret = sprintf(buf, "%x\n", i3cdev->info.dcr); > + i3c_bus_normaluse_unlock(bus); > + > + return ret; > +} > +static DEVICE_ATTR_RO(dcr); > + > +static ssize_t pid_show(struct device *dev, > + struct device_attribute *da, > + char *buf) > +{ > + struct i3c_device *i3cdev = dev_to_i3cdev(dev); > + struct i3c_bus *bus = i3c_device_get_bus(i3cdev); > + ssize_t ret; > + > + i3c_bus_normaluse_lock(bus); > + ret = sprintf(buf, "%llx\n", i3cdev->info.pid); > + i3c_bus_normaluse_unlock(bus); > + > + return ret; > +} > +static DEVICE_ATTR_RO(pid); > + > +static ssize_t address_show(struct device *dev, > + struct device_attribute *da, > + char *buf) > +{ > + struct i3c_device *i3cdev = dev_to_i3cdev(dev); > + struct i3c_bus *bus = i3c_device_get_bus(i3cdev); > + ssize_t ret; > + > + i3c_bus_normaluse_lock(bus); > + ret = sprintf(buf, "%02x\n", i3cdev->info.dyn_addr); > + i3c_bus_normaluse_unlock(bus); > + > + return ret; > +} > +static DEVICE_ATTR_RO(address); should there be separate entries for dynamic and static address? If yes, you could also reduce the boilerplate by using a macro taking attribute name and format string. > +static int i3c_device_uevent(struct device *dev, struct kobj_uevent_env *env) > +{ > + struct i3c_device *i3cdev = dev_to_i3cdev(dev); > + u16 manuf = I3C_PID_MANUF_ID(i3cdev->info.pid); > + u16 part = I3C_PID_PART_ID(i3cdev->info.pid); > + u16 ext = I3C_PID_EXTRA_INFO(i3cdev->info.pid); > + > + if (I3C_PID_RND_LOWER_32BITS(i3cdev->info.pid)) > + return add_uevent_var(env, "MODALIAS=i3c:dcr%02Xmanuf%04X", > + i3cdev->info.dcr, manuf); > + > + return add_uevent_var(env, > + "MODALIAS=i3c:dcr%02Xmanuf%04Xpart%04xext%04x", > + i3cdev->info.dcr, manuf, part, ext); instance id should also be passed in the non-random case? > +} > +static const struct i3c_device_id * > +i3c_device_match_id(struct i3c_device *i3cdev, > + const struct i3c_device_id *id_table) > +{ > + const struct i3c_device_id *id; > + > + /* > + * The lower 32bits of the provisional ID is just filled with a random > + * value, try to match using DCR info. > + */ > + if (!I3C_PID_RND_LOWER_32BITS(i3cdev->info.pid)) { > + u16 manuf = I3C_PID_MANUF_ID(i3cdev->info.pid); > + u16 part = I3C_PID_PART_ID(i3cdev->info.pid); > + u16 ext_info = I3C_PID_EXTRA_INFO(i3cdev->info.pid); > + > + /* First try to match by manufacturer/part ID. */ > + for (id = id_table; id->match_flags != 0; id++) { > + if ((id->match_flags & I3C_MATCH_MANUF_AND_PART) != > + I3C_MATCH_MANUF_AND_PART) > + continue; > + > + if (manuf != id->manuf_id || part != id->part_id) > + continue; > + > + if ((id->match_flags & I3C_MATCH_EXTRA_INFO) && > + ext_info != id->extra_info) > + continue; > + > + return id; Here too, instance id is ignored. Seems like done on purpose? > + } > + } > + > + /* Fallback to DCR match. */ > + for (id = id_table; id->match_flags != 0; id++) { > + if ((id->match_flags & I3C_MATCH_DCR) && > + id->dcr == i3cdev->info.dcr) > + return id; > + } > + > + return NULL; > +} > +static int i3c_device_probe(struct device *dev) > +{ > + struct i3c_device *i3cdev = dev_to_i3cdev(dev); > + struct i3c_driver *driver = drv_to_i3cdrv(dev->driver); > + > + return driver->probe(i3cdev); Should you pm_runtime enable the device before probe? Like done for PCI in local_pci_probe() for example. Or I guess thats a problem because I2C devices don't expect it? > diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c > new file mode 100644 > index 000000000000..8948d9bdec82 > --- /dev/null > +++ b/drivers/i3c/device.c > @@ -0,0 +1,294 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2017 Cadence Design Systems Inc. 2018 now :) > + * > + * Author: Boris Brezillon <boris.brezillon@bootlin.com> > + */ > + > +#include <linux/atomic.h> > +#include <linux/bug.h> > +#include <linux/completion.h> > +#include <linux/device.h> > +#include <linux/mutex.h> > +#include <linux/slab.h> > + > +#include "internals.h" > + > +/** > + * i3c_device_do_priv_xfers() - do I3C SDR private transfers directed to a > + * specific device > + * > + * @dev: device with which the transfers should be done > + * @xfers: array of transfers > + * @nxfers: number of transfers > + * > + * Initiate one or several private SDR transfers with @dev. > + * > + * This function can sleep and thus cannot be called in atomic context. > + * > + * Return: 0 in case of success, a negative error core otherwise. > + */ Curious why you specifically call out SDR here. It could be HDR too, in future. Right? > +int i3c_device_do_priv_xfers(struct i3c_device *dev, > + struct i3c_priv_xfer *xfers, > + int nxfers) > +{ > + struct i3c_master_controller *master; > + int ret, i; > + > + if (nxfers < 1) > + return 0; > + > + master = i3c_device_get_master(dev); > + if (!master || !xfers) > + return -EINVAL; > + > + if (!master->ops->priv_xfers) > + return -ENOTSUPP; > + > + for (i = 0; i < nxfers; i++) { > + if (!xfers[i].len || !xfers[i].data.in) > + return -EINVAL; > + } > + > + i3c_bus_normaluse_lock(master->bus); > + ret = master->ops->priv_xfers(dev, xfers, nxfers); > + i3c_bus_normaluse_unlock(master->bus); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers); > +/** > + * struct i3c_device_info - I3C device information > + * @pid: Provisional ID > + * @bcr: Bus Characteristic Register > + * @dcr: Device Characteristic Register > + * @static_addr: static/I2C address > + * @dyn_addr: dynamic address > + * @hdr_cap: supported HDR modes This is just for querying and display device capability. We dont actually enter HDR mode at the moment, right? > + * @max_read_ds: max read speed information > + * @max_write_ds: max write speed information > + * @max_ibi_len: max IBI payload length > + * @max_read_turnaround: max read turn-around time in micro-seconds > + * @max_read_len: max private SDR read length in bytes > + * @max_write_len: max private SDR write length in bytes > + * > + * These are all basic information that should be advertised by an I3C device. > + * Some of them are optional depending on the device type and device > + * capabilities. > + * For each I3C slave attached to a master with > + * i3c_master_add_i3c_dev_locked(), the core will send the relevant CCC command > + * to retrieve these data. > + */ > +struct i3c_device_info { > + u64 pid; > + u8 bcr; > + u8 dcr; > + u8 static_addr; > + u8 dyn_addr; > + u8 hdr_cap; > + u8 max_read_ds; > + u8 max_write_ds; > + u8 max_ibi_len; > + u32 max_read_turnaround; > + u16 max_read_len; > + u16 max_write_len; > +}; Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sekhar, On Wed, 20 Jun 2018 17:07:53 +0530 Sekhar Nori <nsekhar@ti.com> wrote: > Hi Boris, > > On Friday 30 March 2018 01:17 PM, Boris Brezillon wrote: > > Add core infrastructure to support I3C in Linux and document it. > > > > This infrastructure is not complete yet and will be extended over > > time. > > > > There are a few design choices that are worth mentioning because they > > impact the way I3C device drivers can interact with their devices: > > > > - all functions used to send I3C/I2C frames must be called in > > non-atomic context. Mainly done this way to ease implementation, but > > this is still open to discussion. Please let me know if you think > > it's worth considering an asynchronous model here > > - the bus element is a separate object and is not implicitly described > > by the master (as done in I2C). The reason is that I want to be able > > to handle multiple master connected to the same bus and visible to > > Linux. > > In this situation, we should only have one instance of the device and > > not one per master, and sharing the bus object would be part of the > > solution to gracefully handle this case. > > I'm not sure we will ever need to deal with multiple masters > > controlling the same bus and exposed under Linux, but separating the > > bus and master concept is pretty easy, hence the decision to do it > > like that. > > There can only be one current master in I3C, so not sure of this > scenario. Yes, there's only one active master at any time, but still, you can have several masters (one primary and several secondary masters) connected to the same bus, and you might even have several of them controlled by the same Linux instance (don't really see a use case for that right now, but I'm pretty sure this will happen). The point of having a single bus instance pointed by various I3C masters in this case is to avoid exposing several times the same I3C device. If we don't do that we would have one I3C device instance per I3C master exposed under Linux even though they all control the same physical device. > But agree with bus and master separation. > > > The other benefit of separating the bus and master concepts is that > > master devices appear under the bus directory in sysfs. > > - I2C backward compatibility has been designed to be transparent to I2C > > drivers and the I2C subsystem. The I3C master just registers an I2C > > adapter which creates a new I2C bus. I'd say that, from a > > representation PoV it's not ideal because what should appear as a > > single I3C bus exposing I3C and I2C devices here appears as 2 > > different busses connected to each other through the parenting (the > > I3C master is the parent of the I2C and I3C busses). > > On the other hand, I don't see a better solution if we want something > > that is not invasive. > > > > Missing features in this preliminary version: > > - I3C HDR modes are not supported > > - no support for multi-master and the associated concepts (mastership > > handover, support for secondary masters, ...) > > - I2C devices can only be described using DT because this is the only > > use case I have. However, the framework can easily be extended with > > ACPI and board info support > > - I3C slave framework. This has been completely omitted, but shouldn't > > have a huge impact on the I3C framework because I3C slaves don't see > > the whole bus, it's only about handling master requests and generating > > IBIs. Some of the struct, constant and enum definitions could be > > shared, but most of the I3C slave framework logic will be different > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > > diff --git a/drivers/Makefile b/drivers/Makefile > > index 24cd47014657..999239dc29d4 100644 > > --- a/drivers/Makefile > > +++ b/drivers/Makefile > > @@ -111,7 +111,7 @@ obj-$(CONFIG_SERIO) += input/serio/ > > obj-$(CONFIG_GAMEPORT) += input/gameport/ > > obj-$(CONFIG_INPUT) += input/ > > obj-$(CONFIG_RTC_LIB) += rtc/ > > -obj-y += i2c/ media/ > > +obj-y += i2c/ i3c/ media/ > > obj-$(CONFIG_PPS) += pps/ > > obj-y += ptp/ > > obj-$(CONFIG_W1) += w1/ > > diff --git a/drivers/i3c/Kconfig b/drivers/i3c/Kconfig > > new file mode 100644 > > index 000000000000..cf3752412ae9 > > --- /dev/null > > +++ b/drivers/i3c/Kconfig > > @@ -0,0 +1,24 @@ > > +# SPDX-License-Identifier: GPL-2.0 > > + > > +menuconfig I3C > > + tristate "I3C support" > > + select I2C > > + help > > + I3C is a serial protocol standardized by the MIPI alliance. > > + > > + It's supposed to be backward compatible with I2C while providing > > + support for high speed transfers and native interrupt support > > + without the need for extra pins. > > + > > + The I3C protocol also standardizes the slave device types and is > > + mainly design to communicate with sensors. > > designed Will fix that. > > > + > > + If you want I3C support, you should say Y here and also to the > > + specific driver for your bus adapter(s) below. > > + > > + This I3C support can also be built as a module. If so, the module > > + will be called i3c. > > + > > diff --git a/drivers/i3c/core.c b/drivers/i3c/core.c > > new file mode 100644 > > index 000000000000..d6d938a785a9 > > --- /dev/null > > +++ b/drivers/i3c/core.c > > > +static ssize_t bcr_show(struct device *dev, > > + struct device_attribute *da, > > + char *buf) > > +{ > > + struct i3c_device *i3cdev = dev_to_i3cdev(dev); > > + struct i3c_bus *bus = i3c_device_get_bus(i3cdev); > > + ssize_t ret; > > + > > + i3c_bus_normaluse_lock(bus); > > + ret = sprintf(buf, "%x\n", i3cdev->info.bcr); > > + i3c_bus_normaluse_unlock(bus); > > + > > + return ret; > > +} > > +static DEVICE_ATTR_RO(bcr); > > + > > +static ssize_t dcr_show(struct device *dev, > > + struct device_attribute *da, > > + char *buf) > > +{ > > + struct i3c_device *i3cdev = dev_to_i3cdev(dev); > > + struct i3c_bus *bus = i3c_device_get_bus(i3cdev); > > + ssize_t ret; > > + > > + i3c_bus_normaluse_lock(bus); > > + ret = sprintf(buf, "%x\n", i3cdev->info.dcr); > > + i3c_bus_normaluse_unlock(bus); > > + > > + return ret; > > +} > > +static DEVICE_ATTR_RO(dcr); > > + > > +static ssize_t pid_show(struct device *dev, > > + struct device_attribute *da, > > + char *buf) > > +{ > > + struct i3c_device *i3cdev = dev_to_i3cdev(dev); > > + struct i3c_bus *bus = i3c_device_get_bus(i3cdev); > > + ssize_t ret; > > + > > + i3c_bus_normaluse_lock(bus); > > + ret = sprintf(buf, "%llx\n", i3cdev->info.pid); > > + i3c_bus_normaluse_unlock(bus); > > + > > + return ret; > > +} > > +static DEVICE_ATTR_RO(pid); > > + > > +static ssize_t address_show(struct device *dev, > > + struct device_attribute *da, > > + char *buf) > > +{ > > + struct i3c_device *i3cdev = dev_to_i3cdev(dev); > > + struct i3c_bus *bus = i3c_device_get_bus(i3cdev); > > + ssize_t ret; > > + > > + i3c_bus_normaluse_lock(bus); > > + ret = sprintf(buf, "%02x\n", i3cdev->info.dyn_addr); > > + i3c_bus_normaluse_unlock(bus); > > + > > + return ret; > > +} > > +static DEVICE_ATTR_RO(address); > > should there be separate entries for dynamic and static address? I didn't think exposing the static address was needed since it's never used except at initialization time. > > If yes, you could also reduce the boilerplate by using a macro taking > attribute name and format string. Hm, don't see the need for that yet, even if we expose both static and dynamic addresses. It's not like you'll save hundreds lines of code by doing that, we're talking about 10 lines. > > > +static int i3c_device_uevent(struct device *dev, struct kobj_uevent_env *env) > > +{ > > + struct i3c_device *i3cdev = dev_to_i3cdev(dev); > > + u16 manuf = I3C_PID_MANUF_ID(i3cdev->info.pid); > > + u16 part = I3C_PID_PART_ID(i3cdev->info.pid); > > + u16 ext = I3C_PID_EXTRA_INFO(i3cdev->info.pid); > > + > > + if (I3C_PID_RND_LOWER_32BITS(i3cdev->info.pid)) > > + return add_uevent_var(env, "MODALIAS=i3c:dcr%02Xmanuf%04X", > > + i3cdev->info.dcr, manuf); > > + > > + return add_uevent_var(env, > > + "MODALIAS=i3c:dcr%02Xmanuf%04Xpart%04xext%04x", > > + i3cdev->info.dcr, manuf, part, ext); > > instance id should also be passed in the non-random case? MODALIAS is used by user space to know which module should be loaded when a device is connected on the bus. Why would we need to know the instance ID? In comparison, the USB subsystem does not pass the ->iSerialNumber value in MODALIAS. > > > +} > > > +static const struct i3c_device_id * > > +i3c_device_match_id(struct i3c_device *i3cdev, > > + const struct i3c_device_id *id_table) > > +{ > > + const struct i3c_device_id *id; > > + > > + /* > > + * The lower 32bits of the provisional ID is just filled with a random > > + * value, try to match using DCR info. > > + */ > > + if (!I3C_PID_RND_LOWER_32BITS(i3cdev->info.pid)) { > > + u16 manuf = I3C_PID_MANUF_ID(i3cdev->info.pid); > > + u16 part = I3C_PID_PART_ID(i3cdev->info.pid); > > + u16 ext_info = I3C_PID_EXTRA_INFO(i3cdev->info.pid); > > + > > + /* First try to match by manufacturer/part ID. */ > > + for (id = id_table; id->match_flags != 0; id++) { > > + if ((id->match_flags & I3C_MATCH_MANUF_AND_PART) != > > + I3C_MATCH_MANUF_AND_PART) > > + continue; > > + > > + if (manuf != id->manuf_id || part != id->part_id) > > + continue; > > + > > + if ((id->match_flags & I3C_MATCH_EXTRA_INFO) && > > + ext_info != id->extra_info) > > + continue; > > + > > + return id; > > Here too, instance id is ignored. Seems like done on purpose? Yes, it's done on purpose, the instance ID does not impact the driver selection logic, it's just a way to uniquely identify devices of the same type on an I3C bus. > > > + } > > + } > > + > > + /* Fallback to DCR match. */ > > + for (id = id_table; id->match_flags != 0; id++) { > > + if ((id->match_flags & I3C_MATCH_DCR) && > > + id->dcr == i3cdev->info.dcr) > > + return id; > > + } > > + > > + return NULL; > > +} > > > +static int i3c_device_probe(struct device *dev) > > +{ > > + struct i3c_device *i3cdev = dev_to_i3cdev(dev); > > + struct i3c_driver *driver = drv_to_i3cdrv(dev->driver); > > + > > + return driver->probe(i3cdev); > > Should you pm_runtime enable the device before probe? Like done for PCI > in local_pci_probe() for example. Hm, I'm not sure, but I'd say that it's the device driver responsibility to do that. > Or I guess thats a problem because I2C > devices don't expect it? I2C device probing is not handled here, so that shouldn't be a problem. Still, I think we should wait for a real need before deciding whether calling pm_runtime enable() from the core is a wise thing to do or not. > > > diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c > > new file mode 100644 > > index 000000000000..8948d9bdec82 > > --- /dev/null > > +++ b/drivers/i3c/device.c > > @@ -0,0 +1,294 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (C) 2017 Cadence Design Systems Inc. > > 2018 now :) Will change all dates. > > > + * > > + * Author: Boris Brezillon <boris.brezillon@bootlin.com> > > + */ > > + > > +#include <linux/atomic.h> > > +#include <linux/bug.h> > > +#include <linux/completion.h> > > +#include <linux/device.h> > > +#include <linux/mutex.h> > > +#include <linux/slab.h> > > + > > +#include "internals.h" > > + > > +/** > > + * i3c_device_do_priv_xfers() - do I3C SDR private transfers directed to a > > + * specific device > > + * > > + * @dev: device with which the transfers should be done > > + * @xfers: array of transfers > > + * @nxfers: number of transfers > > + * > > + * Initiate one or several private SDR transfers with @dev. > > + * > > + * This function can sleep and thus cannot be called in atomic context. > > + * > > + * Return: 0 in case of success, a negative error core otherwise. > > + */ > > Curious why you specifically call out SDR here. It could be HDR too, in > future. Right? HDR transfers will be handled through a different function (see the previous version of this patch series where HDR modes were supported). Regarding the name itself, I just followed the naming used in the I3C spec, but I fine changing _priv_ by _sdr_ if you prefer. > > > +int i3c_device_do_priv_xfers(struct i3c_device *dev, > > + struct i3c_priv_xfer *xfers, > > + int nxfers) > > +{ > > + struct i3c_master_controller *master; > > + int ret, i; > > + > > + if (nxfers < 1) > > + return 0; > > + > > + master = i3c_device_get_master(dev); > > + if (!master || !xfers) > > + return -EINVAL; > > + > > + if (!master->ops->priv_xfers) > > + return -ENOTSUPP; > > + > > + for (i = 0; i < nxfers; i++) { > > + if (!xfers[i].len || !xfers[i].data.in) > > + return -EINVAL; > > + } > > + > > + i3c_bus_normaluse_lock(master->bus); > > + ret = master->ops->priv_xfers(dev, xfers, nxfers); > > + i3c_bus_normaluse_unlock(master->bus); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers); > > > +/** > > + * struct i3c_device_info - I3C device information > > + * @pid: Provisional ID > > + * @bcr: Bus Characteristic Register > > + * @dcr: Device Characteristic Register > > + * @static_addr: static/I2C address > > + * @dyn_addr: dynamic address > > + * @hdr_cap: supported HDR modes > > This is just for querying and display device capability. We dont > actually enter HDR mode at the moment, right? Right now it is, but is will be used when we'll later add HDR support (see v3 of this series if you want to have an idea of what the HDR API looks like) ;-). > > > + * @max_read_ds: max read speed information > > + * @max_write_ds: max write speed information > > + * @max_ibi_len: max IBI payload length > > + * @max_read_turnaround: max read turn-around time in micro-seconds > > + * @max_read_len: max private SDR read length in bytes > > + * @max_write_len: max private SDR write length in bytes > > + * > > + * These are all basic information that should be advertised by an I3C device. > > + * Some of them are optional depending on the device type and device > > + * capabilities. > > + * For each I3C slave attached to a master with > > + * i3c_master_add_i3c_dev_locked(), the core will send the relevant CCC command > > + * to retrieve these data. > > + */ > > +struct i3c_device_info { > > + u64 pid; > > + u8 bcr; > > + u8 dcr; > > + u8 static_addr; > > + u8 dyn_addr; > > + u8 hdr_cap; > > + u8 max_read_ds; > > + u8 max_write_ds; > > + u8 max_ibi_len; > > + u32 max_read_turnaround; > > + u16 max_read_len; > > + u16 max_write_len; > > +}; > Thanks for your review. Boris -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 30, 2018 at 9:47 AM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: > Add core infrastructure to support I3C in Linux and document it. > > This infrastructure is not complete yet and will be extended over > time. > > There are a few design choices that are worth mentioning because they > impact the way I3C device drivers can interact with their devices: > > - all functions used to send I3C/I2C frames must be called in > non-atomic context. Mainly done this way to ease implementation, but > this is still open to discussion. Please let me know if you think > it's worth considering an asynchronous model here This sounds like it can be changed later if it turns out necessary, so that's fine to me. > - the bus element is a separate object and is not implicitly described > by the master (as done in I2C). The reason is that I want to be able > to handle multiple master connected to the same bus and visible to > Linux. > In this situation, we should only have one instance of the device and > not one per master, and sharing the bus object would be part of the > solution to gracefully handle this case. > I'm not sure we will ever need to deal with multiple masters > controlling the same bus and exposed under Linux, but separating the > bus and master concept is pretty easy, hence the decision to do it > like that. > The other benefit of separating the bus and master concepts is that > master devices appear under the bus directory in sysfs. I'm not following here at all, sorry for missing prior discussion if this was already explained. What is the "multiple master" case? Do you mean multiple devices that are controlled by Linux and that each talk to other devices on the same bus, multiple operating systems that have talk to are able to own the bus with the kernel being one of them, a controller that controls multiple independent buses, or something else? > - I2C backward compatibility has been designed to be transparent to I2C > drivers and the I2C subsystem. The I3C master just registers an I2C > adapter which creates a new I2C bus. I'd say that, from a > representation PoV it's not ideal because what should appear as a > single I3C bus exposing I3C and I2C devices here appears as 2 > different busses connected to each other through the parenting (the > I3C master is the parent of the I2C and I3C busses). > On the other hand, I don't see a better solution if we want something > that is not invasive. Right, this seems like a reasonable compromise. > Missing features in this preliminary version: > - I3C HDR modes are not supported > - no support for multi-master and the associated concepts (mastership > handover, support for secondary masters, ...) > - I2C devices can only be described using DT because this is the only > use case I have. However, the framework can easily be extended with > ACPI and board info support > - I3C slave framework. This has been completely omitted, but shouldn't > have a huge impact on the I3C framework because I3C slaves don't see > the whole bus, it's only about handling master requests and generating > IBIs. Some of the struct, constant and enum definitions could be > shared, but most of the I3C slave framework logic will be different All of these seem ok to me for an initial version. It's already too big to review properly, so leaving out stuff helps. > +/** > + * i3cdev_to_dev() - Returns the device embedded in @i3cdev > + * @i3cdev: I3C device > + * > + * Return: a pointer to a device object. > + */ > +struct device *i3cdev_to_dev(struct i3c_device *i3cdev) > +{ > + return &i3cdev->dev; > +} > +EXPORT_SYMBOL_GPL(i3cdev_to_dev); > + > +/** > + * dev_to_i3cdev() - Returns the I3C device containing @dev > + * @dev: device object > + * > + * Return: a pointer to an I3C device object. > + */ > +struct i3c_device *dev_to_i3cdev(struct device *dev) > +{ > + return container_of(dev, struct i3c_device, dev); > +} > +EXPORT_SYMBOL_GPL(dev_to_i3cdev); Many other subsystems just make the device structure available to all client drivers so this can be an inline operation. Is there a strong reason to hide it here? > +static struct i3c_device * > +i3c_master_alloc_i3c_dev(struct i3c_master_controller *master, > + const struct i3c_device_info *info, > + const struct device_type *devtype) > +{ > + struct i3c_device *dev; > + > + dev = kzalloc(sizeof(*dev), GFP_KERNEL); > + if (!dev) > + return ERR_PTR(-ENOMEM); > + > + dev->common.bus = master->bus; > + dev->dev.parent = &master->bus->dev; > + dev->dev.type = devtype; > + dev->dev.bus = &i3c_bus_type; This feels a bit odd: so you have bus_type that can contain devices of three (?) different device types: i3c_device_type, i3c_master_type and i3c_busdev_type. Generally speaking, we don't have a lot of subsystems that even use device_types. I assume that the i3c_device_type for a device that corresponds to an endpoint on the bus, but I'm still confused about the other two, and why they are part of the same bus_type. Can you describe whether it's possible to have these arbitrarily mixed, or is there a strict hierarchy such as host_bus (e.g. platform_device) i3c_busdev i3c_master i3c_device If that is the actual hierarchy, why isn't it enough to have multiple masters as children of the platform_device, and then multiple devices under each of them? Can you also have a platform_device that controls multiple busdev instances, which each have multiple masters? > +/** > + * i3c_generic_ibi_alloc_pool() - Create a generic IBI pool > + * @dev: the device this pool will be used for > + * @req: IBI setup request describing what the device driver expects > + * > + * Create a generic IBI pool based on the information provided in @req. > + * > + * Return: a valid IBI pool in case of success, an ERR_PTR() otherwise. > + */ > +struct i3c_generic_ibi_pool * > +i3c_generic_ibi_alloc_pool(struct i3c_device *dev, > + const struct i3c_ibi_setup *req) > +{ > + struct i3c_generic_ibi_pool *pool; > + struct i3c_generic_ibi_slot *slot; > + unsigned int i; > + int ret; > + > + pool = kzalloc(sizeof(*pool), GFP_KERNEL); > + if (!pool) > + return ERR_PTR(-ENOMEM); > + > + spin_lock_init(&pool->lock); > + INIT_LIST_HEAD(&pool->free_slots); > + INIT_LIST_HEAD(&pool->pending); > + > + for (i = 0; i < req->num_slots; i++) { > + slot = kzalloc(sizeof(*slot), GFP_KERNEL); > + if (!slot) > + return ERR_PTR(-ENOMEM); > + > + i3c_master_init_ibi_slot(dev, &slot->base); > + > + if (req->max_payload_len) { > + slot->base.data = kzalloc(req->max_payload_len, > + GFP_KERNEL); You do a lot of allocations here, each with the same GFP_KERNEL flag. Do the objects have different lifetimes, or could you combine them into a larger allocation? Is this called frequently, or only while initializing a device? > +/** > + * struct i3c_ccc_mwl - payload passed to SETMWL/GETMWL CCC > + * > + * @len: maximum write length in bytes > + * > + * The maximum write length is only applicable to SDR private messages or > + * extended Write CCCs (like SETXTIME). > + */ > +struct i3c_ccc_mwl { > + __be16 len; > +} __packed; I would suggest only marking structures as __packed that are not already naturally packed. Note that a side-effect of __packed is that here alignof(struct i3c_ccc_mwl) will be '1', and architectures without efficient unaligned access will have to access the field one byte at a time because they assume that it may be misaligned. > +/** > + * struct i3c_ccc_mrl - payload passed to SETMRL/GETMRL CCC > + * > + * @len: maximum read length in bytes > + * @ibi_len: maximum IBI payload length > + * > + * The maximum read length is only applicable to SDR private messages or > + * extended Read CCCs (like GETXTIME). > + * The IBI length is only valid if the I3C slave is IBI capable > + * (%I3C_BCR_IBI_REQ_CAP is set). > + */ > +struct i3c_ccc_mrl { > + __be16 read_len; > + u8 ibi_len; > +} __packed; This one clearly needs the __packed to get sizeof(struct i3c_ccc_mrl)==3 > +/** > + * struct i3c_ccc_cmd_payload - CCC payload > + * > + * @len: payload length > + * @data: payload data > + */ > +struct i3c_ccc_cmd_payload { > + u16 len; > + void *data; > +}; > + > +/** > + * struct i3c_ccc_cmd_dest - CCC command destination > + * > + * @addr: can be an I3C device address or the broadcast address if this is a > + * broadcast CCC > + * @payload: payload to be sent to this device or broadcasted > + */ > +struct i3c_ccc_cmd_dest { > + u8 addr; > + struct i3c_ccc_cmd_payload payload; > +}; There seems to be a lot of padding in this structure: on a 64-bit machine, you have 11 bytes of data and 13 bytes of padding. Not sure if that's intentional or important at all. > +/** > + * struct i3c_ccc_cmd - CCC command > + * > + * @rnw: true if the CCC should retrieve data from the device. Only valid for > + * unicast commands > + * @id: CCC command id > + * @dests: array of destinations and associated payload for this CCC. Most of > + * the time, only one destination is provided > + * @ndests: number of destinations. Should always be one for broadcast commands > + */ > +struct i3c_ccc_cmd { > + bool rnw; > + u8 id; > + struct i3c_ccc_cmd_dest *dests; > + int ndests; > +}; Moving the 'ndests' above the pointer will make this structure 16 bytes instead of 24 on 64-bit architectures. > +/** > + * struct i3c_i2c_dev - I3C/I2C common information > + * @node: node element used to insert the device into the I2C or I3C device > + * list > + * @bus: I3C bus this device is connected to > + * @master: I3C master that instantiated this device. Will be used to send > + * I2C/I3C frames on the bus > + * @master_priv: master private data assigned to the device. Can be used to > + * add master specific information > + * > + * This structure is describing common I3C/I2C dev information. > + */ > +struct i3c_i2c_dev { > + struct list_head node; > + struct i3c_bus *bus; > + struct i3c_master_controller *master; > + void *master_priv; > +}; I find this hard to follow, which means either this has to be complicated and I just didn't take enough time to think it through, or maybe it can be simplified. The 'node' field seems particularly odd, can you explain what it's for? Normally all children of a bus device can be enumerated by walking the device model structures. Are you doing this just so you can walk a single list rather than walking the i3c and i2c devices separately? > +/** > + * struct i3c_bus - I3C bus object > + * @dev: device to be registered to the device-model > + * @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 > + * know whether it needs to request bus ownership before sending > + * a frame or not > + * @id: bus ID. Assigned by the framework when register the bus > + * @addrslots: a bitmap with 2-bits per-slot to encode the address status and > + * ease the DAA (Dynamic Address Assignment) procedure (see > + * &enum i3c_addr_slot_status) > + * @mode: bus mode (see &enum i3c_bus_mode) > + * @scl_rate: SCL signal rate for I3C and I2C mode > + * @devs: 2 lists containing all I3C/I2C devices connected to the bus > + * @lock: read/write lock on the bus. This is needed to protect against > + * operations that have an impact on the whole bus and the devices > + * connected to it. For example, when asking slaves to drop their > + * dynamic address (RSTDAA CCC), we need to make sure no one is trying > + * to send I3C frames to these devices. > + * Note that this lock does not protect against concurrency between > + * devices: several drivers can send different I3C/I2C frames through > + * the same master in parallel. This is the responsibility of the > + * master to guarantee that frames are actually sent sequentially and > + * not interlaced > + * > + * The I3C bus is represented with its own object and not implicitly described > + * by the I3C master to cope with the multi-master functionality, where one bus > + * can be shared amongst several masters, each of them requesting bus ownership > + * when they need to. > + */ > +struct i3c_bus { > + struct device dev; > + struct i3c_device *cur_master; > + int id; > + unsigned long addrslots[((I2C_MAX_ADDR + 1) * 2) / BITS_PER_LONG]; > + enum i3c_bus_mode mode; > + struct { > + unsigned long i3c; > + unsigned long i2c; > + } scl_rate; > + struct { > + struct list_head i3c; > + struct list_head i2c; > + } devs; > + struct rw_semaphore lock; > +}; Now here you have two separate lists. How is the i3c list different from i3c_bus->dev.p->klist_children? How do you get to the i2c_adapter from an i3c_bus? Does each master have one? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd, On Wed, 11 Jul 2018 16:01:56 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > > > - the bus element is a separate object and is not implicitly described > > by the master (as done in I2C). The reason is that I want to be able > > to handle multiple master connected to the same bus and visible to > > Linux. > > In this situation, we should only have one instance of the device and > > not one per master, and sharing the bus object would be part of the > > solution to gracefully handle this case. > > I'm not sure we will ever need to deal with multiple masters > > controlling the same bus and exposed under Linux, but separating the > > bus and master concept is pretty easy, hence the decision to do it > > like that. > > The other benefit of separating the bus and master concepts is that > > master devices appear under the bus directory in sysfs. > > I'm not following here at all, sorry for missing prior discussion if this > was already explained. What is the "multiple master" case? Do you > mean multiple devices that are controlled by Linux and that each talk > to other devices on the same bus, multiple operating systems that > have talk to are able to own the bus with the kernel being one of > them, a controller that controls multiple independent buses, > or something else? I mean several masters connected to the same bus and all exposed to the same Linux instance. In this case, the question is, should we have X I3C buses exposed (X being the number of masters) or should we only have one? Having a bus represented as a separate object allows us to switch to the "one bus : X masters" representation if we need too. > > > +/** > > + * i3cdev_to_dev() - Returns the device embedded in @i3cdev > > + * @i3cdev: I3C device > > + * > > + * Return: a pointer to a device object. > > + */ > > +struct device *i3cdev_to_dev(struct i3c_device *i3cdev) > > +{ > > + return &i3cdev->dev; > > +} > > +EXPORT_SYMBOL_GPL(i3cdev_to_dev); > > + > > +/** > > + * dev_to_i3cdev() - Returns the I3C device containing @dev > > + * @dev: device object > > + * > > + * Return: a pointer to an I3C device object. > > + */ > > +struct i3c_device *dev_to_i3cdev(struct device *dev) > > +{ > > + return container_of(dev, struct i3c_device, dev); > > +} > > +EXPORT_SYMBOL_GPL(dev_to_i3cdev); > > Many other subsystems just make the device structure available > to all client drivers so this can be an inline operation. Is there > a strong reason to hide it here? No, but I think most subsystem do provide dev_to_xxxdev() at least (to_platform_device() for instance) > > > +static struct i3c_device * > > +i3c_master_alloc_i3c_dev(struct i3c_master_controller *master, > > + const struct i3c_device_info *info, > > + const struct device_type *devtype) > > +{ > > + struct i3c_device *dev; > > + > > + dev = kzalloc(sizeof(*dev), GFP_KERNEL); > > + if (!dev) > > + return ERR_PTR(-ENOMEM); > > + > > + dev->common.bus = master->bus; > > + dev->dev.parent = &master->bus->dev; > > + dev->dev.type = devtype; > > + dev->dev.bus = &i3c_bus_type; > > This feels a bit odd: so you have bus_type that can contain devices > of three (?) different device types: i3c_device_type, i3c_master_type > and i3c_busdev_type. > > Generally speaking, we don't have a lot of subsystems that even > use device_types. I assume that the i3c_device_type for a > device that corresponds to an endpoint on the bus, but I'm > still confused about the other two, and why they are part of > the same bus_type. i3c_busdev is just a virtual device representing the bus itself. i3c_master is representing the I3C master driving the bus. The reason for having a different type here is to avoid attaching this device to a driver but still being able to see the master controller as a device on the bus. And finally, i3c_device are all remote devices that can be accessed through a given i3c_master. This all comes from the design choice I made to represent the bus as a separate object in order to be able to share it between different master controllers exposed through the same Linux instance. Since master controllers are also remote devices for other controllers, we need to represent them. > > Can you describe whether it's possible to have these arbitrarily > mixed, or is there a strict hierarchy such as > > host_bus (e.g. platform_device) > i3c_busdev > i3c_master > i3c_device It's more: platdev i3c_busdev i3c_master i3c_device > > If that is the actual hierarchy, why isn't it enough to have multiple > masters as children of the platform_device, and then multiple > devices under each of them? Can you also have a platform_device > that controls multiple busdev instances, which each have multiple > masters? Not yet. But at some point, we may have the same i3c_busdev instance shared by several platdev, each of these platdev having an i3c_master instance on the bus. > > > +/** > > + * i3c_generic_ibi_alloc_pool() - Create a generic IBI pool > > + * @dev: the device this pool will be used for > > + * @req: IBI setup request describing what the device driver expects > > + * > > + * Create a generic IBI pool based on the information provided in @req. > > + * > > + * Return: a valid IBI pool in case of success, an ERR_PTR() otherwise. > > + */ > > +struct i3c_generic_ibi_pool * > > +i3c_generic_ibi_alloc_pool(struct i3c_device *dev, > > + const struct i3c_ibi_setup *req) > > +{ > > + struct i3c_generic_ibi_pool *pool; > > + struct i3c_generic_ibi_slot *slot; > > + unsigned int i; > > + int ret; > > + > > + pool = kzalloc(sizeof(*pool), GFP_KERNEL); > > + if (!pool) > > + return ERR_PTR(-ENOMEM); > > + > > + spin_lock_init(&pool->lock); > > + INIT_LIST_HEAD(&pool->free_slots); > > + INIT_LIST_HEAD(&pool->pending); > > + > > + for (i = 0; i < req->num_slots; i++) { > > + slot = kzalloc(sizeof(*slot), GFP_KERNEL); > > + if (!slot) > > + return ERR_PTR(-ENOMEM); > > + > > + i3c_master_init_ibi_slot(dev, &slot->base); > > + > > + if (req->max_payload_len) { > > + slot->base.data = kzalloc(req->max_payload_len, > > + GFP_KERNEL); > > You do a lot of allocations here, each with the same GFP_KERNEL > flag. Do the objects have different lifetimes, or could you combine > them into a larger allocation? I guess I could pack them to a single kcalloc(). > > Is this called frequently, or only while initializing a device? Only at IBI handler registration time, so normally only when probing an I3C device. > > > +/** > > + * struct i3c_ccc_mwl - payload passed to SETMWL/GETMWL CCC > > + * > > + * @len: maximum write length in bytes > > + * > > + * The maximum write length is only applicable to SDR private messages or > > + * extended Write CCCs (like SETXTIME). > > + */ > > +struct i3c_ccc_mwl { > > + __be16 len; > > +} __packed; > > I would suggest only marking structures as __packed that are not already > naturally packed. Note that a side-effect of __packed is that here > alignof(struct i3c_ccc_mwl) will be '1', and architectures without efficient > unaligned access will have to access the field one byte at a time because > they assume that it may be misaligned. These are structure used to create packets to be sent on the wire. Making sure that everything is packed correctly is important, so I'm not sure I can remove the __packed everywhere. > > > +/** > > + * struct i3c_ccc_mrl - payload passed to SETMRL/GETMRL CCC > > + * > > + * @len: maximum read length in bytes > > + * @ibi_len: maximum IBI payload length > > + * > > + * The maximum read length is only applicable to SDR private messages or > > + * extended Read CCCs (like GETXTIME). > > + * The IBI length is only valid if the I3C slave is IBI capable > > + * (%I3C_BCR_IBI_REQ_CAP is set). > > + */ > > +struct i3c_ccc_mrl { > > + __be16 read_len; > > + u8 ibi_len; > > +} __packed; > > This one clearly needs the __packed to get sizeof(struct i3c_ccc_mrl)==3 Yep, that's what I meant. > > > +/** > > + * struct i3c_ccc_cmd_payload - CCC payload > > + * > > + * @len: payload length > > + * @data: payload data > > + */ > > +struct i3c_ccc_cmd_payload { > > + u16 len; > > + void *data; > > +}; > > + > > +/** > > + * struct i3c_ccc_cmd_dest - CCC command destination > > + * > > + * @addr: can be an I3C device address or the broadcast address if this is a > > + * broadcast CCC > > + * @payload: payload to be sent to this device or broadcasted > > + */ > > +struct i3c_ccc_cmd_dest { > > + u8 addr; > > + struct i3c_ccc_cmd_payload payload; > > +}; > > There seems to be a lot of padding in this structure: on a 64-bit > machine, you have 11 bytes of data and 13 bytes of padding. > Not sure if that's intentional or important at all. I don't think that a big issue. > > > +/** > > + * struct i3c_ccc_cmd - CCC command > > + * > > + * @rnw: true if the CCC should retrieve data from the device. Only valid for > > + * unicast commands > > + * @id: CCC command id > > + * @dests: array of destinations and associated payload for this CCC. Most of > > + * the time, only one destination is provided > > + * @ndests: number of destinations. Should always be one for broadcast commands > > + */ > > +struct i3c_ccc_cmd { > > + bool rnw; > > + u8 id; > > + struct i3c_ccc_cmd_dest *dests; > > + int ndests; > > +}; > > Moving the 'ndests' above the pointer will make this structure 16 bytes instead > of 24 on 64-bit architectures. Okay. Will do. > > > +/** > > + * struct i3c_i2c_dev - I3C/I2C common information > > + * @node: node element used to insert the device into the I2C or I3C device > > + * list > > + * @bus: I3C bus this device is connected to > > + * @master: I3C master that instantiated this device. Will be used to send > > + * I2C/I3C frames on the bus > > + * @master_priv: master private data assigned to the device. Can be used to > > + * add master specific information > > + * > > + * This structure is describing common I3C/I2C dev information. > > + */ > > +struct i3c_i2c_dev { > > + struct list_head node; > > + struct i3c_bus *bus; > > + struct i3c_master_controller *master; > > + void *master_priv; > > +}; > > I find this hard to follow, which means either this has to be complicated > and I just didn't take enough time to think it through, or maybe it can > be simplified. > > The 'node' field seems particularly odd, can you explain what it's > for? Normally all children of a bus device can be enumerated by > walking the device model structures. Are you doing this just so > you can walk a single list rather than walking the i3c and i2c > devices separately? The devices discovered on the bus are not directly registered to the device model, and I need to store them in a list to do some operations before exposing them. Once everything is ready to be used, I then iterate the list and register all not-yet-registered I3C devs. > > > +/** > > + * struct i3c_bus - I3C bus object > > + * @dev: device to be registered to the device-model > > + * @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 > > + * know whether it needs to request bus ownership before sending > > + * a frame or not > > + * @id: bus ID. Assigned by the framework when register the bus > > + * @addrslots: a bitmap with 2-bits per-slot to encode the address status and > > + * ease the DAA (Dynamic Address Assignment) procedure (see > > + * &enum i3c_addr_slot_status) > > + * @mode: bus mode (see &enum i3c_bus_mode) > > + * @scl_rate: SCL signal rate for I3C and I2C mode > > + * @devs: 2 lists containing all I3C/I2C devices connected to the bus > > + * @lock: read/write lock on the bus. This is needed to protect against > > + * operations that have an impact on the whole bus and the devices > > + * connected to it. For example, when asking slaves to drop their > > + * dynamic address (RSTDAA CCC), we need to make sure no one is trying > > + * to send I3C frames to these devices. > > + * Note that this lock does not protect against concurrency between > > + * devices: several drivers can send different I3C/I2C frames through > > + * the same master in parallel. This is the responsibility of the > > + * master to guarantee that frames are actually sent sequentially and > > + * not interlaced > > + * > > + * The I3C bus is represented with its own object and not implicitly described > > + * by the I3C master to cope with the multi-master functionality, where one bus > > + * can be shared amongst several masters, each of them requesting bus ownership > > + * when they need to. > > + */ > > +struct i3c_bus { > > + struct device dev; > > + struct i3c_device *cur_master; > > + int id; > > + unsigned long addrslots[((I2C_MAX_ADDR + 1) * 2) / BITS_PER_LONG]; > > + enum i3c_bus_mode mode; > > + struct { > > + unsigned long i3c; > > + unsigned long i2c; > > + } scl_rate; > > + struct { > > + struct list_head i3c; > > + struct list_head i2c; > > + } devs; > > + struct rw_semaphore lock; > > +}; > > Now here you have two separate lists. How is the i3c list > different from i3c_bus->dev.p->klist_children? As said above, the I3C devs are not registered right away. We need to make sure the controller is configured properly before exposing them to the device model. > > How do you get to the i2c_adapter from an i3c_bus? Does > each master have one? Each master has an i2c_adapter, and I need an internal wrapper to keep track of I3C-bus-only information attached to an I2C device (like LVR). Yet another reason to have a separate list of I2C devices. The other reason being that I2C devices need to be attached to the I3C master controller before being exposed to the I2C framework, and more importantly, the I3C master controller needs to know about all I2C devices connected on the bus, at bus initialization time (so before the bus becomes active). I hope my explanations are clear enough. Don't hesitate to tell me if that's not the case, and thanks a lot for taking the time to review this patchset. Regards, Boris -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 11 Jul 2018 16:41:20 +0200 Boris Brezillon <boris.brezillon@bootlin.com> wrote: > > > +/** > > > + * i3cdev_to_dev() - Returns the device embedded in @i3cdev > > > + * @i3cdev: I3C device > > > + * > > > + * Return: a pointer to a device object. > > > + */ > > > +struct device *i3cdev_to_dev(struct i3c_device *i3cdev) > > > +{ > > > + return &i3cdev->dev; > > > +} > > > +EXPORT_SYMBOL_GPL(i3cdev_to_dev); > > > + > > > +/** > > > + * dev_to_i3cdev() - Returns the I3C device containing @dev > > > + * @dev: device object > > > + * > > > + * Return: a pointer to an I3C device object. > > > + */ > > > +struct i3c_device *dev_to_i3cdev(struct device *dev) > > > +{ > > > + return container_of(dev, struct i3c_device, dev); > > > +} > > > +EXPORT_SYMBOL_GPL(dev_to_i3cdev); > > > > Many other subsystems just make the device structure available > > to all client drivers so this can be an inline operation. Is there > > a strong reason to hide it here? > > No, but I think most subsystem do provide dev_to_xxxdev() at least > (to_platform_device() for instance) > My bad. I misunderstood you question. The main reason I did that was because I didn't want to expose i3c_device internals to the I3C device drivers. Anyway, this part will be reworked in my v6 to address one problem we had when re-attaching a pre-existing device that had lost its dynamic address and acquired a new one. Since we want that operation to be transparent to I3C device drivers, I had to decouple the I3C device driver representation from the I3C master controller one. I thus end up with struct i3C_dev_desc on the controller API side, and struct i3c_device on the driver side with a link between the 2 object that can be updated at runtime. And as you can imagine, i3c_device does not contain a lot of information now. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 11, 2018 at 4:41 PM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: > On Wed, 11 Jul 2018 16:01:56 +0200 Arnd Bergmann <arnd@arndb.de> wrote: >> > - the bus element is a separate object and is not implicitly described >> > by the master (as done in I2C). The reason is that I want to be able >> > to handle multiple master connected to the same bus and visible to >> > Linux. >> > In this situation, we should only have one instance of the device and >> > not one per master, and sharing the bus object would be part of the >> > solution to gracefully handle this case. >> > I'm not sure we will ever need to deal with multiple masters >> > controlling the same bus and exposed under Linux, but separating the >> > bus and master concept is pretty easy, hence the decision to do it >> > like that. >> > The other benefit of separating the bus and master concepts is that >> > master devices appear under the bus directory in sysfs. >> >> I'm not following here at all, sorry for missing prior discussion if this >> was already explained. What is the "multiple master" case? Do you >> mean multiple devices that are controlled by Linux and that each talk >> to other devices on the same bus, multiple operating systems that >> have talk to are able to own the bus with the kernel being one of >> them, a controller that controls multiple independent buses, >> or something else? > > I mean several masters connected to the same bus and all exposed to the > same Linux instance. In this case, the question is, should we have X > I3C buses exposed (X being the number of masters) or should we only > have one? > > Having a bus represented as a separate object allows us to switch to > the "one bus : X masters" representation if we need too. ... >> >> This feels a bit odd: so you have bus_type that can contain devices >> of three (?) different device types: i3c_device_type, i3c_master_type >> and i3c_busdev_type. >> >> Generally speaking, we don't have a lot of subsystems that even >> use device_types. I assume that the i3c_device_type for a >> device that corresponds to an endpoint on the bus, but I'm >> still confused about the other two, and why they are part of >> the same bus_type. > > i3c_busdev is just a virtual device representing the bus itself. > i3c_master is representing the I3C master driving the bus. The reason > for having a different type here is to avoid attaching this device to a > driver but still being able to see the master controller as a device on > the bus. And finally, i3c_device are all remote devices that can be > accessed through a given i3c_master. > > This all comes from the design choice I made to represent the bus as a > separate object in order to be able to share it between different > master controllers exposed through the same Linux instance. Since > master controllers are also remote devices for other controllers, we > need to represent them. Ok, so I think this is the most important question to resolve: do we actually need to control multiple masters on a single bus from one OS or not? The problem that I see is that it breaks the tree abstraction that we use in the dtb interface, in the driver model and in sysfs. If we need to deal with a hardware bus structure like cpu / \ / \ platdev platdev | | i3c-master i3c-master \ / \ / i3c-bus / \ device device then that abstraction no longer holds. Clearly you could build a system like that, and if we have to support it, the i3c infrastructure should be prepared for it, since we wouldn't be able to retrofit it later. What would be the point of building such a system though? Is this for performance, failover, or something else? IOW, what feature would we lose if we were to declare that setup above invalid (and ensure you cannot represent it in DT)? >> > +/** >> > + * struct i3c_ccc_mwl - payload passed to SETMWL/GETMWL CCC >> > + * >> > + * @len: maximum write length in bytes >> > + * >> > + * The maximum write length is only applicable to SDR private messages or >> > + * extended Write CCCs (like SETXTIME). >> > + */ >> > +struct i3c_ccc_mwl { >> > + __be16 len; >> > +} __packed; >> >> I would suggest only marking structures as __packed that are not already >> naturally packed. Note that a side-effect of __packed is that here >> alignof(struct i3c_ccc_mwl) will be '1', and architectures without efficient >> unaligned access will have to access the field one byte at a time because >> they assume that it may be misaligned. > > These are structure used to create packets to be sent on the wire. > Making sure that everything is packed correctly is important, so I'm > not sure I can remove the __packed everywhere. I mean just the ones for which the __packed attribute only changes the alignment of the outer structure but not the layout inside of the structure. Alternatively, set both __packed and __aligned(). >> > +/** >> > + * struct i3c_i2c_dev - I3C/I2C common information >> > + * @node: node element used to insert the device into the I2C or I3C device >> > + * list >> > + * @bus: I3C bus this device is connected to >> > + * @master: I3C master that instantiated this device. Will be used to send >> > + * I2C/I3C frames on the bus >> > + * @master_priv: master private data assigned to the device. Can be used to >> > + * add master specific information >> > + * >> > + * This structure is describing common I3C/I2C dev information. >> > + */ >> > +struct i3c_i2c_dev { >> > + struct list_head node; >> > + struct i3c_bus *bus; >> > + struct i3c_master_controller *master; >> > + void *master_priv; >> > +}; >> >> I find this hard to follow, which means either this has to be complicated >> and I just didn't take enough time to think it through, or maybe it can >> be simplified. >> >> The 'node' field seems particularly odd, can you explain what it's >> for? Normally all children of a bus device can be enumerated by >> walking the device model structures. Are you doing this just so >> you can walk a single list rather than walking the i3c and i2c >> devices separately? > > The devices discovered on the bus are not directly registered to the > device model, and I need to store them in a list to do some operations > before exposing them. Once everything is ready to be used, I then > iterate the list and register all not-yet-registered I3C devs. Can you explain what those operations are and why we can't register everything directly? This seems rather unconventional, so I want to make sure it's done for a good reason. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 11 Jul 2018 17:39:56 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > On Wed, Jul 11, 2018 at 4:41 PM, Boris Brezillon > <boris.brezillon@bootlin.com> wrote: > > On Wed, 11 Jul 2018 16:01:56 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > >> > - the bus element is a separate object and is not implicitly described > >> > by the master (as done in I2C). The reason is that I want to be able > >> > to handle multiple master connected to the same bus and visible to > >> > Linux. > >> > In this situation, we should only have one instance of the device and > >> > not one per master, and sharing the bus object would be part of the > >> > solution to gracefully handle this case. > >> > I'm not sure we will ever need to deal with multiple masters > >> > controlling the same bus and exposed under Linux, but separating the > >> > bus and master concept is pretty easy, hence the decision to do it > >> > like that. > >> > The other benefit of separating the bus and master concepts is that > >> > master devices appear under the bus directory in sysfs. > >> > >> I'm not following here at all, sorry for missing prior discussion if this > >> was already explained. What is the "multiple master" case? Do you > >> mean multiple devices that are controlled by Linux and that each talk > >> to other devices on the same bus, multiple operating systems that > >> have talk to are able to own the bus with the kernel being one of > >> them, a controller that controls multiple independent buses, > >> or something else? > > > > I mean several masters connected to the same bus and all exposed to the > > same Linux instance. In this case, the question is, should we have X > > I3C buses exposed (X being the number of masters) or should we only > > have one? > > > > Having a bus represented as a separate object allows us to switch to > > the "one bus : X masters" representation if we need too. > ... > >> > >> This feels a bit odd: so you have bus_type that can contain devices > >> of three (?) different device types: i3c_device_type, i3c_master_type > >> and i3c_busdev_type. > >> > >> Generally speaking, we don't have a lot of subsystems that even > >> use device_types. I assume that the i3c_device_type for a > >> device that corresponds to an endpoint on the bus, but I'm > >> still confused about the other two, and why they are part of > >> the same bus_type. > > > > i3c_busdev is just a virtual device representing the bus itself. > > i3c_master is representing the I3C master driving the bus. The reason > > for having a different type here is to avoid attaching this device to a > > driver but still being able to see the master controller as a device on > > the bus. And finally, i3c_device are all remote devices that can be > > accessed through a given i3c_master. > > > > This all comes from the design choice I made to represent the bus as a > > separate object in order to be able to share it between different > > master controllers exposed through the same Linux instance. Since > > master controllers are also remote devices for other controllers, we > > need to represent them. > > Ok, so I think this is the most important question to resolve: do we > actually need to control multiple masters on a single bus from one OS > or not? > > The problem that I see is that it breaks the tree abstraction that > we use in the dtb interface, in the driver model and in sysfs. > If we need to deal with a hardware bus structure like > > cpu > / \ > / \ > platdev platdev > | | > i3c-master i3c-master > \ / > \ / > i3c-bus > / \ > device device > > then that abstraction no longer holds. Clearly you could build > a system like that, and if we have to support it, the i3c infrastructure > should be prepared for it, since we wouldn't be able to retrofit > it later. Exactly. For the DT representation I thought we could have the primary master hold the device nodes, and then have secondary masters reference the main master with a phandle (i3c-bus = <&main_i3c_master>;). For the sysfs representation, it would be the same. Only one of the master would create the i3c_bus object and the other masters would just reference it. > > What would be the point of building such a system though? This, I don't know. But as you said, if we go for a "one bus per master" representation, going back will be difficult. > Is this for performance, failover, or something else? No, I don't think so, especially since the mastership handover operation is not free. So keeping the same master in control is probably better in term of perfs. One case I can think of is when the primary master does not have enough resources to address all devices on the bus, and let the secondary master handle all transactions targeting those devices. > IOW, what feature would we lose if we were to declare that > setup above invalid (and ensure you cannot represent it in DT)? That's exactly the sort of discussion I wanted to trigger. Maybe we shouldn't care and expose this use case as if it was X different I3C buses (with all devices present on the bus being exposed X times to the system). > > >> > +/** > >> > + * struct i3c_ccc_mwl - payload passed to SETMWL/GETMWL CCC > >> > + * > >> > + * @len: maximum write length in bytes > >> > + * > >> > + * The maximum write length is only applicable to SDR private messages or > >> > + * extended Write CCCs (like SETXTIME). > >> > + */ > >> > +struct i3c_ccc_mwl { > >> > + __be16 len; > >> > +} __packed; > >> > >> I would suggest only marking structures as __packed that are not already > >> naturally packed. Note that a side-effect of __packed is that here > >> alignof(struct i3c_ccc_mwl) will be '1', and architectures without efficient > >> unaligned access will have to access the field one byte at a time because > >> they assume that it may be misaligned. > > > > These are structure used to create packets to be sent on the wire. > > Making sure that everything is packed correctly is important, so I'm > > not sure I can remove the __packed everywhere. > > I mean just the ones for which the __packed attribute only changes > the alignment of the outer structure but not the layout inside of the > structure. Alternatively, set both __packed and __aligned(). Ok. > > >> > +/** > >> > + * struct i3c_i2c_dev - I3C/I2C common information > >> > + * @node: node element used to insert the device into the I2C or I3C device > >> > + * list > >> > + * @bus: I3C bus this device is connected to > >> > + * @master: I3C master that instantiated this device. Will be used to send > >> > + * I2C/I3C frames on the bus > >> > + * @master_priv: master private data assigned to the device. Can be used to > >> > + * add master specific information > >> > + * > >> > + * This structure is describing common I3C/I2C dev information. > >> > + */ > >> > +struct i3c_i2c_dev { > >> > + struct list_head node; > >> > + struct i3c_bus *bus; > >> > + struct i3c_master_controller *master; > >> > + void *master_priv; > >> > +}; > >> > >> I find this hard to follow, which means either this has to be complicated > >> and I just didn't take enough time to think it through, or maybe it can > >> be simplified. > >> > >> The 'node' field seems particularly odd, can you explain what it's > >> for? Normally all children of a bus device can be enumerated by > >> walking the device model structures. Are you doing this just so > >> you can walk a single list rather than walking the i3c and i2c > >> devices separately? > > > > The devices discovered on the bus are not directly registered to the > > device model, and I need to store them in a list to do some operations > > before exposing them. Once everything is ready to be used, I then > > iterate the list and register all not-yet-registered I3C devs. > > Can you explain what those operations are and why we can't > register everything directly? This seems rather unconventional, > so I want to make sure it's done for a good reason. When we start a DAA operation (used to discover all devices on the bus), we have the bus lock held in maintenance mode (AKA exclusive mode). During this DAA the controller will add all the devices it has discovered on the bus and let the core query information about those devices (PID, DCR, HDR capabilies, SDR speed limitations, ...). It might also happen that a device that had been discovered previously is re-discovered because it had lost its dynamic address (i.e. when the device had been reset but not by Linux). In this case the I3C framework does not expose a new device but instead updates the dynamic address of the device already registered to the device model, so that new transactions initiated by the I3C device driver work correctly. This is the very reason we hold the lock in exclusive mode (we want all transactions to be stopped until we have updated dynamic addresses if needed). Now, let's imagine you register the device when the bus lock is held in exclusive mode, and the ->probe() function of the I3C driver needs to do an I3C transfer => you end up with a deadlock. So what we do instead is add new devices to the i3c bus list, release the bus lock and then register all new devices. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
n Wed, Jul 11, 2018 at 7:12 PM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: > On Wed, 11 Jul 2018 17:39:56 +0200 Arnd Bergmann <arnd@arndb.de> wrote: >> On Wed, Jul 11, 2018 at 4:41 PM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: >> >> The problem that I see is that it breaks the tree abstraction that >> we use in the dtb interface, in the driver model and in sysfs. >> If we need to deal with a hardware bus structure like >> >> cpu >> / \ >> / \ >> platdev platdev >> | | >> i3c-master i3c-master >> \ / >> \ / >> i3c-bus >> / \ >> device device >> >> then that abstraction no longer holds. Clearly you could build >> a system like that, and if we have to support it, the i3c infrastructure >> should be prepared for it, since we wouldn't be able to retrofit >> it later. > > Exactly. For the DT representation I thought we could have the primary > master hold the device nodes, and then have secondary masters reference > the main master with a phandle (i3c-bus = <&main_i3c_master>;). For the > sysfs representation, it would be the same. Only one of the master > would create the i3c_bus object and the other masters would just > reference it. Ok. >> What would be the point of building such a system though? > > This, I don't know. But as you said, if we go for a "one bus per > master" representation, going back will be difficult. > >> Is this for performance, failover, or something else? > > No, I don't think so, especially since the mastership handover > operation is not free. So keeping the same master in control is > probably better in term of perfs. Right. > One case I can think of is when the primary master does not have enough > resources to address all devices on the bus, and let the secondary > master handle all transactions targeting those devices. > I've read the specification a bit more, and from what I found there, it seems extremely unlikely that there was an intended use case where one OS instance would control more than one master on a single bus and flip them between primary and secondary mode. In particular, the protocol for the handover is defined in a way that intentionally avoids requiring a side channel to communicate data about the slave devices, and instead the secondary master(s) get informed about any changes of the topology as they happen through explicit messages. The description of the secondary master labels the introduction section of says "I3C Smart Sensor(s) / Hub(s) / Engine(s)" which sounds like some microcontroller that can act as a master of some sort, rather than being part of the OS itself. I can see several use cases for this, e.g. * A baseboard management controller booting first, reading all the sensors and possibly loading its own OS from an i3c flash before handing off control to the main CPU of a server and no longer being a master * A fan controller that occasionally wants to read temperature sensors and control fan speed, while normally being a secondary master. It periodically asks the OS to become a master to read the sensors and then immediately hands back control to the host OS as the master * Two controllers inside of the same SoC, but one of them owned by ARM Trustzone firmware or the Intel equivalent, the other owned by the OS, both accessing the same slaves. All of these require implementing handover between primary and secondary master, but Linux would still see a hierarchical bus structure, with the secondary masters looking like slave devices that might request being masters during some time. We may also run into the requirement that a master we probe is currently the secondary master and has to probe the bus by asking the current master for the available devices, and then taking over. >> IOW, what feature would we lose if we were to declare that >> setup above invalid (and ensure you cannot represent it in DT)? > > That's exactly the sort of discussion I wanted to trigger. Maybe we > shouldn't care and expose this use case as if it was X different I3C > buses (with all devices present on the bus being exposed X times > to the system). That's probably fine for many slave devices (you could read a single sensor multiple times if you iterate through all i2c sensors on all buses) but might not work for others (a slave device sending an interrupt to the current master for a request that was started from the previous master). My impression however is that this is actually a corner case that we can leave to be undefined, and not prepare to handle well, as long as we can deal with the interesting examples above. >> > The devices discovered on the bus are not directly registered to the >> > device model, and I need to store them in a list to do some operations >> > before exposing them. Once everything is ready to be used, I then >> > iterate the list and register all not-yet-registered I3C devs. >> >> Can you explain what those operations are and why we can't >> register everything directly? This seems rather unconventional, >> so I want to make sure it's done for a good reason. > > When we start a DAA operation (used to discover all devices on the > bus), we have the bus lock held in maintenance mode (AKA exclusive > mode). During this DAA the controller will add all the devices it has > discovered on the bus and let the core query information about those > devices (PID, DCR, HDR capabilies, SDR speed limitations, ...). It > might also happen that a device that had been discovered previously is > re-discovered because it had lost its dynamic address (i.e. when > the device had been reset but not by Linux). In this case the I3C > framework does not expose a new device but instead updates the dynamic > address of the device already registered to the device model, so that > new transactions initiated by the I3C device driver work correctly. > This is the very reason we hold the lock in exclusive mode (we want all > transactions to be stopped until we have updated dynamic addresses if > needed). > > Now, let's imagine you register the device when the bus lock is held in > exclusive mode, and the ->probe() function of the I3C driver needs to > do an I3C transfer => you end up with a deadlock. > > So what we do instead is add new devices to the i3c bus list, release > the bus lock and then register all new devices. Ok, but maybe you could you put the information about those devices on a local list on the stack rather than the controller? I suppose this would not change the logic much, but it would slightly simplify the data structures for the bus and stop others from wondering about them. ;-) This is a really minor point though, let's work out the problem of the multiple masters first. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 11 Jul 2018 22:10:26 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > n Wed, Jul 11, 2018 at 7:12 PM, Boris Brezillon > <boris.brezillon@bootlin.com> wrote: > > On Wed, 11 Jul 2018 17:39:56 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > >> On Wed, Jul 11, 2018 at 4:41 PM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: > >> > >> The problem that I see is that it breaks the tree abstraction that > >> we use in the dtb interface, in the driver model and in sysfs. > >> If we need to deal with a hardware bus structure like > >> > >> cpu > >> / \ > >> / \ > >> platdev platdev > >> | | > >> i3c-master i3c-master > >> \ / > >> \ / > >> i3c-bus > >> / \ > >> device device > >> > >> then that abstraction no longer holds. Clearly you could build > >> a system like that, and if we have to support it, the i3c infrastructure > >> should be prepared for it, since we wouldn't be able to retrofit > >> it later. > > > > Exactly. For the DT representation I thought we could have the primary > > master hold the device nodes, and then have secondary masters reference > > the main master with a phandle (i3c-bus = <&main_i3c_master>;). For the > > sysfs representation, it would be the same. Only one of the master > > would create the i3c_bus object and the other masters would just > > reference it. > > Ok. > > >> What would be the point of building such a system though? > > > > This, I don't know. But as you said, if we go for a "one bus per > > master" representation, going back will be difficult. > > > >> Is this for performance, failover, or something else? > > > > No, I don't think so, especially since the mastership handover > > operation is not free. So keeping the same master in control is > > probably better in term of perfs. > > Right. > > > One case I can think of is when the primary master does not have enough > > resources to address all devices on the bus, and let the secondary > > master handle all transactions targeting those devices. > > > > > I've read the specification a bit more, and from what I found there, > it seems extremely unlikely that there was an intended use case where > one OS instance would control more than one master on a single bus > and flip them between primary and secondary mode. > > In particular, the protocol for the handover is defined in a way that > intentionally avoids requiring a side channel to communicate data > about the slave devices, and instead the secondary master(s) get > informed about any changes of the topology as they happen > through explicit messages. > > The description of the secondary master labels the introduction section > of says > > "I3C Smart Sensor(s) / Hub(s) / Engine(s)" > > which sounds like some microcontroller that can act as a master of > some sort, rather than being part of the OS itself. > > I can see several use cases for this, e.g. > > * A baseboard management controller booting first, reading all > the sensors and possibly loading its own OS from an i3c flash > before handing off control to the main CPU of a server and > no longer being a master > > * A fan controller that occasionally wants to read temperature > sensors and control fan speed, while normally being a > secondary master. It periodically asks the OS to become > a master to read the sensors and then immediately hands back > control to the host OS as the master > > * Two controllers inside of the same SoC, but one of them owned > by ARM Trustzone firmware or the Intel equivalent, the other > owned by the OS, both accessing the same slaves. > > All of these require implementing handover between primary > and secondary master, but Linux would still see a hierarchical > bus structure, with the secondary masters looking like slave > devices that might request being masters during some time. > > We may also run into the requirement that a master we probe > is currently the secondary master and has to probe the bus > by asking the current master for the available devices, and > then taking over. > > >> IOW, what feature would we lose if we were to declare that > >> setup above invalid (and ensure you cannot represent it in DT)? > > > > That's exactly the sort of discussion I wanted to trigger. Maybe we > > shouldn't care and expose this use case as if it was X different I3C > > buses (with all devices present on the bus being exposed X times > > to the system). > > That's probably fine for many slave devices (you could read a > single sensor multiple times if you iterate through all i2c sensors > on all buses) but might not work for others (a slave device sending > an interrupt to the current master for a request that was started > from the previous master). > My impression however is that this is actually a corner case that > we can leave to be undefined, and not prepare to handle well, > as long as we can deal with the interesting examples above. Mastership handover/takeover is something I already thought about. Actually, that's the reason for the i3c_bus->cur_master field (so that the master being asked to do a transfer on the bus can request bus ownership it bus->cur_master != master->this->info.pid). I guess this would be replaced by something simpler if we get rid of the i3c_bus object (just a bool i3c_master->owns_bus). > > >> > The devices discovered on the bus are not directly registered to the > >> > device model, and I need to store them in a list to do some operations > >> > before exposing them. Once everything is ready to be used, I then > >> > iterate the list and register all not-yet-registered I3C devs. > >> > >> Can you explain what those operations are and why we can't > >> register everything directly? This seems rather unconventional, > >> so I want to make sure it's done for a good reason. > > > > When we start a DAA operation (used to discover all devices on the > > bus), we have the bus lock held in maintenance mode (AKA exclusive > > mode). During this DAA the controller will add all the devices it has > > discovered on the bus and let the core query information about those > > devices (PID, DCR, HDR capabilies, SDR speed limitations, ...). It > > might also happen that a device that had been discovered previously is > > re-discovered because it had lost its dynamic address (i.e. when > > the device had been reset but not by Linux). In this case the I3C > > framework does not expose a new device but instead updates the dynamic > > address of the device already registered to the device model, so that > > new transactions initiated by the I3C device driver work correctly. > > This is the very reason we hold the lock in exclusive mode (we want all > > transactions to be stopped until we have updated dynamic addresses if > > needed). > > > > Now, let's imagine you register the device when the bus lock is held in > > exclusive mode, and the ->probe() function of the I3C driver needs to > > do an I3C transfer => you end up with a deadlock. > > > > So what we do instead is add new devices to the i3c bus list, release > > the bus lock and then register all new devices. > > Ok, but maybe you could you put the information about those devices > on a local list on the stack rather than the controller? I suppose this > would not change the logic much, but it would slightly simplify the > data structures for the bus and stop others from wondering about > them. ;-) This has changed a bit in the v6 I'm about to send (probably next week). I now have 2 different objects which do not necessarily have the same lifetime: * i3c_dev_desc: an I3C device descriptor. This is the representation exposed to I3C master controller drivers which usually reserve one slot in the HW device table per-device on the bus. Since the same device can be re-discovered with a different address, we have a short period in time during which the controller will have 2 slots/descriptors used to control the same device, except one would be inactive (any transfer to the old dynamic address would fail) and the other one would be active. The core then takes care of releasing the old descriptor/slot and attaching the new one to the i3c_device object exposed to I3C device drivers * i3c_device: this is the object exposed to I3C device drivers. It's lifetime is usually the same as the i3c_dev_desc it's attached to, except when the device lose its dynamic address and get a new one assigned. In this case we keep the same i3c_device object, but dynamically re-attach it to a new i3c_dev_desc before releasing the old dev desc. This way the I3C does not even see that the device address has changed and can keep doing transfers to it. With these 2 distinct representations, the handling on the controller side is greatly simplified: the core takes care of the resource migration steps, while it was up to the controller to do that my previous versions (look at the ->reattach_i3c_dev() hook in the Cadence driver, and I think it's even more complicated with other controllers, like the HCI one). > This is a really minor point though, let's work out the problem of the > multiple masters first. I agree. This being said, moving to a representation where the bus is implicitly represented by the master_controller instance shouldn't be too difficult. So, if you think we should try this approach I can do the modifications in my v6. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[tried to send something like this yesterday, but it appears to have been lost, sorry for any duplicate] On 2018-07-11 19:12, Boris Brezillon wrote: > On Wed, 11 Jul 2018 17:39:56 +0200 > Arnd Bergmann <arnd@arndb.de> wrote: > >> On Wed, Jul 11, 2018 at 4:41 PM, Boris Brezillon >> <boris.brezillon@bootlin.com> wrote: >>> On Wed, 11 Jul 2018 16:01:56 +0200 Arnd Bergmann <arnd@arndb.de> wrote: >>>>> - the bus element is a separate object and is not implicitly described >>>>> by the master (as done in I2C). The reason is that I want to be able >>>>> to handle multiple master connected to the same bus and visible to >>>>> Linux. >>>>> In this situation, we should only have one instance of the device and >>>>> not one per master, and sharing the bus object would be part of the >>>>> solution to gracefully handle this case. >>>>> I'm not sure we will ever need to deal with multiple masters >>>>> controlling the same bus and exposed under Linux, but separating the >>>>> bus and master concept is pretty easy, hence the decision to do it >>>>> like that. >>>>> The other benefit of separating the bus and master concepts is that >>>>> master devices appear under the bus directory in sysfs. >>>> >>>> I'm not following here at all, sorry for missing prior discussion if this >>>> was already explained. What is the "multiple master" case? Do you >>>> mean multiple devices that are controlled by Linux and that each talk >>>> to other devices on the same bus, multiple operating systems that >>>> have talk to are able to own the bus with the kernel being one of >>>> them, a controller that controls multiple independent buses, >>>> or something else? >>> >>> I mean several masters connected to the same bus and all exposed to the >>> same Linux instance. In this case, the question is, should we have X >>> I3C buses exposed (X being the number of masters) or should we only >>> have one? >>> >>> Having a bus represented as a separate object allows us to switch to >>> the "one bus : X masters" representation if we need too. >> ... >>>> >>>> This feels a bit odd: so you have bus_type that can contain devices >>>> of three (?) different device types: i3c_device_type, i3c_master_type >>>> and i3c_busdev_type. >>>> >>>> Generally speaking, we don't have a lot of subsystems that even >>>> use device_types. I assume that the i3c_device_type for a >>>> device that corresponds to an endpoint on the bus, but I'm >>>> still confused about the other two, and why they are part of >>>> the same bus_type. >>> >>> i3c_busdev is just a virtual device representing the bus itself. >>> i3c_master is representing the I3C master driving the bus. The reason >>> for having a different type here is to avoid attaching this device to a >>> driver but still being able to see the master controller as a device on >>> the bus. And finally, i3c_device are all remote devices that can be >>> accessed through a given i3c_master. >>> >>> This all comes from the design choice I made to represent the bus as a >>> separate object in order to be able to share it between different >>> master controllers exposed through the same Linux instance. Since >>> master controllers are also remote devices for other controllers, we >>> need to represent them. >> >> Ok, so I think this is the most important question to resolve: do we >> actually need to control multiple masters on a single bus from one OS >> or not? >> >> The problem that I see is that it breaks the tree abstraction that >> we use in the dtb interface, in the driver model and in sysfs. >> If we need to deal with a hardware bus structure like >> >> cpu >> / \ >> / \ >> platdev platdev >> | | >> i3c-master i3c-master >> \ / >> \ / >> i3c-bus >> / \ >> device device >> >> then that abstraction no longer holds. Clearly you could build >> a system like that, and if we have to support it, the i3c infrastructure >> should be prepared for it, since we wouldn't be able to retrofit >> it later. > > Exactly. For the DT representation I thought we could have the primary > master hold the device nodes, and then have secondary masters reference > the main master with a phandle (i3c-bus = <&main_i3c_master>;). For the > sysfs representation, it would be the same. Only one of the master > would create the i3c_bus object and the other masters would just > reference it. > >> >> What would be the point of building such a system though? > > This, I don't know. But as you said, if we go for a "one bus per > master" representation, going back will be difficult. > >> Is this for performance, failover, or something else? > > No, I don't think so, especially since the mastership handover > operation is not free. So keeping the same master in control is > probably better in term of perfs. > > One case I can think of is when the primary master does not have enough > resources to address all devices on the bus, and let the secondary > master handle all transactions targeting those devices. > >> IOW, what feature would we lose if we were to declare that >> setup above invalid (and ensure you cannot represent it in DT)? > > That's exactly the sort of discussion I wanted to trigger. Maybe we > shouldn't care and expose this use case as if it was X different I3C > buses (with all devices present on the bus being exposed X times to the > system). For I2C, this multiple masters for one bus case was retrofitted in the i2c-demux-pinctrl driver. It's a huge kludge with a number of undesirable quirks. I don't know if the circumstances for adding this I2C driver also applies for I3C, but it might be an argument in favor of the proposed extra bus object... Cheers, Peter -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 12 Jul 2018 06:41:15 +0200 Peter Rosin <peda@axentia.se> wrote: > [tried to send something like this yesterday, but it appears to have been > lost, sorry for any duplicate] > > On 2018-07-11 19:12, Boris Brezillon wrote: > > On Wed, 11 Jul 2018 17:39:56 +0200 > > Arnd Bergmann <arnd@arndb.de> wrote: > > > >> On Wed, Jul 11, 2018 at 4:41 PM, Boris Brezillon > >> <boris.brezillon@bootlin.com> wrote: > >>> On Wed, 11 Jul 2018 16:01:56 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > >>>>> - the bus element is a separate object and is not implicitly described > >>>>> by the master (as done in I2C). The reason is that I want to be able > >>>>> to handle multiple master connected to the same bus and visible to > >>>>> Linux. > >>>>> In this situation, we should only have one instance of the device and > >>>>> not one per master, and sharing the bus object would be part of the > >>>>> solution to gracefully handle this case. > >>>>> I'm not sure we will ever need to deal with multiple masters > >>>>> controlling the same bus and exposed under Linux, but separating the > >>>>> bus and master concept is pretty easy, hence the decision to do it > >>>>> like that. > >>>>> The other benefit of separating the bus and master concepts is that > >>>>> master devices appear under the bus directory in sysfs. > >>>> > >>>> I'm not following here at all, sorry for missing prior discussion if this > >>>> was already explained. What is the "multiple master" case? Do you > >>>> mean multiple devices that are controlled by Linux and that each talk > >>>> to other devices on the same bus, multiple operating systems that > >>>> have talk to are able to own the bus with the kernel being one of > >>>> them, a controller that controls multiple independent buses, > >>>> or something else? > >>> > >>> I mean several masters connected to the same bus and all exposed to the > >>> same Linux instance. In this case, the question is, should we have X > >>> I3C buses exposed (X being the number of masters) or should we only > >>> have one? > >>> > >>> Having a bus represented as a separate object allows us to switch to > >>> the "one bus : X masters" representation if we need too. > >> ... > >>>> > >>>> This feels a bit odd: so you have bus_type that can contain devices > >>>> of three (?) different device types: i3c_device_type, i3c_master_type > >>>> and i3c_busdev_type. > >>>> > >>>> Generally speaking, we don't have a lot of subsystems that even > >>>> use device_types. I assume that the i3c_device_type for a > >>>> device that corresponds to an endpoint on the bus, but I'm > >>>> still confused about the other two, and why they are part of > >>>> the same bus_type. > >>> > >>> i3c_busdev is just a virtual device representing the bus itself. > >>> i3c_master is representing the I3C master driving the bus. The reason > >>> for having a different type here is to avoid attaching this device to a > >>> driver but still being able to see the master controller as a device on > >>> the bus. And finally, i3c_device are all remote devices that can be > >>> accessed through a given i3c_master. > >>> > >>> This all comes from the design choice I made to represent the bus as a > >>> separate object in order to be able to share it between different > >>> master controllers exposed through the same Linux instance. Since > >>> master controllers are also remote devices for other controllers, we > >>> need to represent them. > >> > >> Ok, so I think this is the most important question to resolve: do we > >> actually need to control multiple masters on a single bus from one OS > >> or not? > >> > >> The problem that I see is that it breaks the tree abstraction that > >> we use in the dtb interface, in the driver model and in sysfs. > >> If we need to deal with a hardware bus structure like > >> > >> cpu > >> / \ > >> / \ > >> platdev platdev > >> | | > >> i3c-master i3c-master > >> \ / > >> \ / > >> i3c-bus > >> / \ > >> device device > >> > >> then that abstraction no longer holds. Clearly you could build > >> a system like that, and if we have to support it, the i3c infrastructure > >> should be prepared for it, since we wouldn't be able to retrofit > >> it later. > > > > Exactly. For the DT representation I thought we could have the primary > > master hold the device nodes, and then have secondary masters reference > > the main master with a phandle (i3c-bus = <&main_i3c_master>;). For the > > sysfs representation, it would be the same. Only one of the master > > would create the i3c_bus object and the other masters would just > > reference it. > > > >> > >> What would be the point of building such a system though? > > > > This, I don't know. But as you said, if we go for a "one bus per > > master" representation, going back will be difficult. > > > >> Is this for performance, failover, or something else? > > > > No, I don't think so, especially since the mastership handover > > operation is not free. So keeping the same master in control is > > probably better in term of perfs. > > > > One case I can think of is when the primary master does not have enough > > resources to address all devices on the bus, and let the secondary > > master handle all transactions targeting those devices. > > > >> IOW, what feature would we lose if we were to declare that > >> setup above invalid (and ensure you cannot represent it in DT)? > > > > That's exactly the sort of discussion I wanted to trigger. Maybe we > > shouldn't care and expose this use case as if it was X different I3C > > buses (with all devices present on the bus being exposed X times to the > > system). > > For I2C, this multiple masters for one bus case was retrofitted in > the i2c-demux-pinctrl driver. It's a huge kludge with a number of > undesirable quirks. I don't know if the circumstances for adding > this I2C driver also applies for I3C, It's hard to guess now. > but it might be an argument > in favor of the proposed extra bus object... I know that Wolfram was in favor of this separate bus <-> master representation, probably because of his experience with this particular driver. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 12, 2018 at 6:41 AM, Peter Rosin <peda@axentia.se> wrote: > On 2018-07-11 19:12, Boris Brezillon wrote: >> On Wed, 11 Jul 2018 17:39:56 +0200 Arnd Bergmann <arnd@arndb.de> wrote: >> >> That's exactly the sort of discussion I wanted to trigger. Maybe we >> shouldn't care and expose this use case as if it was X different I3C >> buses (with all devices present on the bus being exposed X times to the >> system). > > For I2C, this multiple masters for one bus case was retrofitted in > the i2c-demux-pinctrl driver. It's a huge kludge with a number of > undesirable quirks. I don't know if the circumstances for adding > this I2C driver also applies for I3C, but it might be an argument > in favor of the proposed extra bus object... From reading the documentation and git history on that driver, it seems to be used only for static configuration, i.e. you use one driver or the other, but don't flip between them at runtime, right? I'm guessing that even with i3c we may have to support something like that, as a likely scenario might be that the i3c controller is multiplexed with a traditional i2c controller and/or gpios, but you would not be able to perform the i3c standard secondary master transition with the latter two because they are (by definition) not i3c compatible. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 12, 2018 at 12:09 AM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: > On Wed, 11 Jul 2018 22:10:26 +0200 Arnd Bergmann <arnd@arndb.de> wrote: >> On Wed, Jul 11, 2018 at 7:12 PM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: >> > On Wed, 11 Jul 2018 17:39:56 +0200 Arnd Bergmann <arnd@arndb.de> wrote: >> >> On Wed, Jul 11, 2018 at 4:41 PM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: >> >> IOW, what feature would we lose if we were to declare that >> >> setup above invalid (and ensure you cannot represent it in DT)? >> > >> > That's exactly the sort of discussion I wanted to trigger. Maybe we >> > shouldn't care and expose this use case as if it was X different I3C >> > buses (with all devices present on the bus being exposed X times >> > to the system). >> >> That's probably fine for many slave devices (you could read a >> single sensor multiple times if you iterate through all i2c sensors >> on all buses) but might not work for others (a slave device sending >> an interrupt to the current master for a request that was started >> from the previous master). >> My impression however is that this is actually a corner case that >> we can leave to be undefined, and not prepare to handle well, >> as long as we can deal with the interesting examples above. > > Mastership handover/takeover is something I already thought about. > Actually, that's the reason for the i3c_bus->cur_master field (so that > the master being asked to do a transfer on the bus can request bus > ownership it bus->cur_master != master->this->info.pid). > > I guess this would be replaced by something simpler if we get rid of > the i3c_bus object (just a bool i3c_master->owns_bus). If we can ignore the case of handing over between two masters that we both own, we end up being in just one of two states: a) currently we are the master b) we are not currently the master but have asked the current master to transfer ownership back to us. For this, we have to know who the current master is of course. I think that's the simplest case that would work with the specification, but it relies on the assumption that the master controlled by Linux is always more important than any other master, and that we just hand over ownership for as long as the others want it. If that is not the case, we also need a third state c) we have handed ownership to another master that is equally important, but no i2c device driver is currently trying to talk to a device, so we don't need ownership back until a slave driver changes state. The main difference between b) and c) that I see would be what happens with in-band interrupts. If I understand the specification correctly, only the current master receives them, so if you have any i2c device that uses interrupts to talk to a Linux driver, we want to be in b) rather than c). An example of this would be an input device on a PC: If the user operateds the keyboard or pointer and we have handed off ownership to a sensor hub, we never get an input event, right? >> Ok, but maybe you could you put the information about those devices >> on a local list on the stack rather than the controller? I suppose this >> would not change the logic much, but it would slightly simplify the >> data structures for the bus and stop others from wondering about >> them. ;-) > > This has changed a bit in the v6 I'm about to send (probably next week). > I now have 2 different objects which do not necessarily have the same > lifetime: > > * i3c_dev_desc: an I3C device descriptor. This is the representation > exposed to I3C master controller drivers which usually reserve one > slot in the HW device table per-device on the bus. Since the same > device can be re-discovered with a different address, we have a short > period in time during which the controller will have 2 > slots/descriptors used to control the same device, except one would > be inactive (any transfer to the old dynamic address would fail) and > the other one would be active. The core then takes care of releasing > the old descriptor/slot and attaching the new one to the i3c_device > object exposed to I3C device drivers > > * i3c_device: this is the object exposed to I3C device drivers. It's > lifetime is usually the same as the i3c_dev_desc it's attached to, > except when the device lose its dynamic address and get a new one > assigned. In this case we keep the same i3c_device object, but > dynamically re-attach it to a new i3c_dev_desc before releasing the > old dev desc. This way the I3C does not even see that the device > address has changed and can keep doing transfers to it. > > With these 2 distinct representations, the handling on the controller > side is greatly simplified: the core takes care of the resource > migration steps, while it was up to the controller to do that my > previous versions (look at the ->reattach_i3c_dev() hook in the Cadence > driver, and I think it's even more complicated with other controllers, > like the HCI one). > >> This is a really minor point though, let's work out the problem of the >> multiple masters first. > > I agree. This being said, moving to a representation where the bus is > implicitly represented by the master_controller instance shouldn't be > too difficult. So, if you think we should try this approach I can do > the modifications in my v6. I'd say let's wait before you do that change, possibly add a comment in there now to remind us of what an alternative would be. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018-07-12 10:08, Arnd Bergmann wrote: > On Thu, Jul 12, 2018 at 6:41 AM, Peter Rosin <peda@axentia.se> wrote: >> On 2018-07-11 19:12, Boris Brezillon wrote: >>> On Wed, 11 Jul 2018 17:39:56 +0200 Arnd Bergmann <arnd@arndb.de> wrote: >>> >>> That's exactly the sort of discussion I wanted to trigger. Maybe we >>> shouldn't care and expose this use case as if it was X different I3C >>> buses (with all devices present on the bus being exposed X times to the >>> system). >> >> For I2C, this multiple masters for one bus case was retrofitted in >> the i2c-demux-pinctrl driver. It's a huge kludge with a number of >> undesirable quirks. I don't know if the circumstances for adding >> this I2C driver also applies for I3C, but it might be an argument >> in favor of the proposed extra bus object... > > From reading the documentation and git history on that driver, > it seems to be used only for static configuration, i.e. you use > one driver or the other, but don't flip between them at runtime, > right? There is a sysfs file that can be used to change master at runtime (current_master). This causes all client drivers to be reprobed, which may not be the best thing to do for every client out there... > I'm guessing that even with i3c we may have to support something > like that, as a likely scenario might be that the i3c controller is > multiplexed with a traditional i2c controller and/or gpios, but you > would not be able to perform the i3c standard secondary master > transition with the latter two because they are (by definition) not > i3c compatible. i2c-demux-pinctrl should probably not be used as template for something else, but it is a good argument for some other design IMHO... Cheers, Peter -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 12 Jul 2018 10:21:40 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > On Thu, Jul 12, 2018 at 12:09 AM, Boris Brezillon > <boris.brezillon@bootlin.com> wrote: > > On Wed, 11 Jul 2018 22:10:26 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > >> On Wed, Jul 11, 2018 at 7:12 PM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: > >> > On Wed, 11 Jul 2018 17:39:56 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > >> >> On Wed, Jul 11, 2018 at 4:41 PM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: > > >> >> IOW, what feature would we lose if we were to declare that > >> >> setup above invalid (and ensure you cannot represent it in DT)? > >> > > >> > That's exactly the sort of discussion I wanted to trigger. Maybe we > >> > shouldn't care and expose this use case as if it was X different I3C > >> > buses (with all devices present on the bus being exposed X times > >> > to the system). > >> > >> That's probably fine for many slave devices (you could read a > >> single sensor multiple times if you iterate through all i2c sensors > >> on all buses) but might not work for others (a slave device sending > >> an interrupt to the current master for a request that was started > >> from the previous master). > >> My impression however is that this is actually a corner case that > >> we can leave to be undefined, and not prepare to handle well, > >> as long as we can deal with the interesting examples above. > > > > Mastership handover/takeover is something I already thought about. > > Actually, that's the reason for the i3c_bus->cur_master field (so that > > the master being asked to do a transfer on the bus can request bus > > ownership it bus->cur_master != master->this->info.pid). > > > > I guess this would be replaced by something simpler if we get rid of > > the i3c_bus object (just a bool i3c_master->owns_bus). > > If we can ignore the case of handing over between two masters that > we both own, we end up being in just one of two states: > > a) currently we are the master > b) we are not currently the master but have asked the current > master to transfer ownership back to us. For this, we have to > know who the current master is of course. > > I think that's the simplest case that would work with the specification, > but it relies on the assumption that the master controlled by Linux > is always more important than any other master, and that we just > hand over ownership for as long as the others want it. > > If that is not the case, we also need a third state > > c) we have handed ownership to another master that is equally > important, but no i2c device driver is currently trying to talk > to a device, so we don't need ownership back until a slave driver > changes state. That was the solution I was opting for. > > The main difference between b) and c) that I see would be what > happens with in-band interrupts. If I understand the specification > correctly, only the current master receives them, so if you have > any i2c device that uses interrupts to talk to a Linux driver, we ^ you mean i3c here, right? > want to be in b) rather than c). An example of this would be > an input device on a PC: If the user operateds the keyboard > or pointer and we have handed off ownership to a sensor hub, > we never get an input event, right? Correct. I guess we could try to regain bus ownership in case we have IBIs enabled. Or we let the secondary master give the bus back to us when it sees IBIs it can't handle, as described in section 5.1.7: " Once granted control of the Bus, the Secondary Master maintains control until another Master is granted Bus control. After the Secondary Master transitions to the Current Master role it could encounter Bus management activities besides the data transfers that it itself initiates. Some examples are the In-Band Interrupt, or the Hot-Join request. One optional possibility, shown at Section 5.1.7.2, is that the Secondary Master performs the Current Master’s actions with the full capabilities of the Main Master. Another optional possibility is that the Secondary Master, while serving in the Current Master role, could defer some actions to a more capable Master, as described in Section 5.1.7.3. " > > >> Ok, but maybe you could you put the information about those devices > >> on a local list on the stack rather than the controller? I suppose this > >> would not change the logic much, but it would slightly simplify the > >> data structures for the bus and stop others from wondering about > >> them. ;-) > > > > This has changed a bit in the v6 I'm about to send (probably next week). > > I now have 2 different objects which do not necessarily have the same > > lifetime: > > > > * i3c_dev_desc: an I3C device descriptor. This is the representation > > exposed to I3C master controller drivers which usually reserve one > > slot in the HW device table per-device on the bus. Since the same > > device can be re-discovered with a different address, we have a short > > period in time during which the controller will have 2 > > slots/descriptors used to control the same device, except one would > > be inactive (any transfer to the old dynamic address would fail) and > > the other one would be active. The core then takes care of releasing > > the old descriptor/slot and attaching the new one to the i3c_device > > object exposed to I3C device drivers > > > > * i3c_device: this is the object exposed to I3C device drivers. It's > > lifetime is usually the same as the i3c_dev_desc it's attached to, > > except when the device lose its dynamic address and get a new one > > assigned. In this case we keep the same i3c_device object, but > > dynamically re-attach it to a new i3c_dev_desc before releasing the > > old dev desc. This way the I3C does not even see that the device > > address has changed and can keep doing transfers to it. > > > > With these 2 distinct representations, the handling on the controller > > side is greatly simplified: the core takes care of the resource > > migration steps, while it was up to the controller to do that my > > previous versions (look at the ->reattach_i3c_dev() hook in the Cadence > > driver, and I think it's even more complicated with other controllers, > > like the HCI one). > > > >> This is a really minor point though, let's work out the problem of the > >> multiple masters first. > > > > I agree. This being said, moving to a representation where the bus is > > implicitly represented by the master_controller instance shouldn't be > > too difficult. So, if you think we should try this approach I can do > > the modifications in my v6. > > I'd say let's wait before you do that change, possibly add a comment > in there now to remind us of what an alternative would be. You mean I should keep the i3c_bus object? -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 12, 2018 at 10:46 AM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: > On Thu, 12 Jul 2018 10:21:40 +0200 Arnd Bergmann <arnd@arndb.de> wrote: >> On Thu, Jul 12, 2018 at 12:09 AM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: >> > On Wed, 11 Jul 2018 22:10:26 +0200 Arnd Bergmann <arnd@arndb.de> wrote: >> >> On Wed, Jul 11, 2018 at 7:12 PM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: >> >> > On Wed, 11 Jul 2018 17:39:56 +0200 Arnd Bergmann <arnd@arndb.de> wrote: >> >> >> On Wed, Jul 11, 2018 at 4:41 PM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: >> >> If we can ignore the case of handing over between two masters that >> we both own, we end up being in just one of two states: >> >> a) currently we are the master >> b) we are not currently the master but have asked the current >> master to transfer ownership back to us. For this, we have to >> know who the current master is of course. >> >> I think that's the simplest case that would work with the specification, >> but it relies on the assumption that the master controlled by Linux >> is always more important than any other master, and that we just >> hand over ownership for as long as the others want it. >> >> If that is not the case, we also need a third state >> >> c) we have handed ownership to another master that is equally >> important, but no i2c device driver is currently trying to talk >> to a device, so we don't need ownership back until a slave driver >> changes state. > > That was the solution I was opting for. > >> >> The main difference between b) and c) that I see would be what >> happens with in-band interrupts. If I understand the specification >> correctly, only the current master receives them, so if you have >> any i2c device that uses interrupts to talk to a Linux driver, we > > ^ you mean i3c here, right? sure >> want to be in b) rather than c). An example of this would be >> an input device on a PC: If the user operateds the keyboard >> or pointer and we have handed off ownership to a sensor hub, >> we never get an input event, right? > > Correct. I guess we could try to regain bus ownership in case we have > IBIs enabled. Or we let the secondary master give the bus back to us > when it sees IBIs it can't handle, as described in section 5.1.7: > > " > Once granted control of the Bus, the Secondary Master maintains > control until another Master is granted Bus control. After the > Secondary Master transitions to the Current Master role it could > encounter Bus management activities besides the data transfers that it > itself initiates. Some examples are the In-Band Interrupt, or the > Hot-Join request. One optional possibility, shown at Section 5.1.7.2, > is that the Secondary Master performs the Current Master’s actions with > the full capabilities of the Main Master. Another optional possibility > is that the Secondary Master, while serving in the Current Master role, > could defer some actions to a more capable Master, as described in > Section 5.1.7.3. > " Ah, so the current master can ask a secondary master to take over again even if the secondary master has not requested to be come the current master? >> > I agree. This being said, moving to a representation where the bus is >> > implicitly represented by the master_controller instance shouldn't be >> > too difficult. So, if you think we should try this approach I can do >> > the modifications in my v6. >> >> I'd say let's wait before you do that change, possibly add a comment >> in there now to remind us of what an alternative would be. > > You mean I should keep the i3c_bus object? I mean the ongoing discussion shouldn't stop you from posting a v6. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 12 Jul 2018 12:03:05 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > >> want to be in b) rather than c). An example of this would be > >> an input device on a PC: If the user operateds the keyboard > >> or pointer and we have handed off ownership to a sensor hub, > >> we never get an input event, right? > > > > Correct. I guess we could try to regain bus ownership in case we have > > IBIs enabled. Or we let the secondary master give the bus back to us > > when it sees IBIs it can't handle, as described in section 5.1.7: > > > > " > > Once granted control of the Bus, the Secondary Master maintains > > control until another Master is granted Bus control. After the > > Secondary Master transitions to the Current Master role it could > > encounter Bus management activities besides the data transfers that it > > itself initiates. Some examples are the In-Band Interrupt, or the > > Hot-Join request. One optional possibility, shown at Section 5.1.7.2, > > is that the Secondary Master performs the Current Master’s actions with > > the full capabilities of the Main Master. Another optional possibility > > is that the Secondary Master, while serving in the Current Master role, > > could defer some actions to a more capable Master, as described in > > Section 5.1.7.3. > > " > > Ah, so the current master can ask a secondary master to take over > again even if the secondary master has not requested to be come the > current master? Yes. Then the inactive master can refuse of course, but it is working both ways: - an inactive master can ask for bus ownership - an active master can ask an inactive one to take over > > >> > I agree. This being said, moving to a representation where the bus is > >> > implicitly represented by the master_controller instance shouldn't be > >> > too difficult. So, if you think we should try this approach I can do > >> > the modifications in my v6. > >> > >> I'd say let's wait before you do that change, possibly add a comment > >> in there now to remind us of what an alternative would be. > > > > You mean I should keep the i3c_bus object? > > I mean the ongoing discussion shouldn't stop you from posting a v6. Ok. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/Kconfig b/drivers/Kconfig index 879dc0604cba..f0563ce52fab 100644 --- a/drivers/Kconfig +++ b/drivers/Kconfig @@ -55,6 +55,8 @@ source "drivers/char/Kconfig" source "drivers/i2c/Kconfig" +source "drivers/i3c/Kconfig" + source "drivers/spi/Kconfig" source "drivers/spmi/Kconfig" diff --git a/drivers/Makefile b/drivers/Makefile index 24cd47014657..999239dc29d4 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -111,7 +111,7 @@ obj-$(CONFIG_SERIO) += input/serio/ obj-$(CONFIG_GAMEPORT) += input/gameport/ obj-$(CONFIG_INPUT) += input/ obj-$(CONFIG_RTC_LIB) += rtc/ -obj-y += i2c/ media/ +obj-y += i2c/ i3c/ media/ obj-$(CONFIG_PPS) += pps/ obj-y += ptp/ obj-$(CONFIG_W1) += w1/ diff --git a/drivers/i3c/Kconfig b/drivers/i3c/Kconfig new file mode 100644 index 000000000000..cf3752412ae9 --- /dev/null +++ b/drivers/i3c/Kconfig @@ -0,0 +1,24 @@ +# SPDX-License-Identifier: GPL-2.0 + +menuconfig I3C + tristate "I3C support" + select I2C + help + I3C is a serial protocol standardized by the MIPI alliance. + + It's supposed to be backward compatible with I2C while providing + support for high speed transfers and native interrupt support + without the need for extra pins. + + The I3C protocol also standardizes the slave device types and is + mainly design to communicate with sensors. + + If you want I3C support, you should say Y here and also to the + specific driver for your bus adapter(s) below. + + This I3C support can also be built as a module. If so, the module + will be called i3c. + +if I3C +source "drivers/i3c/master/Kconfig" +endif # I3C diff --git a/drivers/i3c/Makefile b/drivers/i3c/Makefile new file mode 100644 index 000000000000..3b6d1502d6e6 --- /dev/null +++ b/drivers/i3c/Makefile @@ -0,0 +1,4 @@ +# SPDX-License-Identifier: GPL-2.0 +i3c-y := core.o device.o master.o +obj-$(CONFIG_I3C) += i3c.o +obj-$(CONFIG_I3C) += master/ diff --git a/drivers/i3c/core.c b/drivers/i3c/core.c new file mode 100644 index 000000000000..d6d938a785a9 --- /dev/null +++ b/drivers/i3c/core.c @@ -0,0 +1,620 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2017 Cadence Design Systems Inc. + * + * Author: Boris Brezillon <boris.brezillon@bootlin.com> + */ + +#include <linux/device.h> +#include <linux/idr.h> +#include <linux/init.h> +#include <linux/list.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/of_device.h> +#include <linux/rwsem.h> +#include <linux/slab.h> + +#include "internals.h" + +static DEFINE_IDR(i3c_bus_idr); +static DEFINE_MUTEX(i3c_core_lock); + +/** + * i3c_bus_maintenance_lock - Lock the bus for a maintenance operation + * @bus: I3C bus to take the lock on + * + * This function takes the bus lock so that no other operations can occur on + * the bus. This is needed for all kind of bus maintenance operation, like + * - enabling/disabling slave events + * - re-triggering DAA + * - changing the dynamic address of a device + * - relinquishing mastership + * - ... + * + * The reason for this kind of locking is that we don't want drivers and core + * logic to rely on I3C device information that could be changed behind their + * back. + */ +void i3c_bus_maintenance_lock(struct i3c_bus *bus) +{ + down_write(&bus->lock); +} +EXPORT_SYMBOL_GPL(i3c_bus_maintenance_lock); + +/** + * i3c_bus_maintenance_unlock - Release the bus lock after a maintenance + * operation + * @bus: I3C bus to release the lock on + * + * Should be called when the bus maintenance operation is done. See + * i3c_bus_maintenance_lock() for more details on what these maintenance + * operations are. + */ +void i3c_bus_maintenance_unlock(struct i3c_bus *bus) +{ + up_write(&bus->lock); +} +EXPORT_SYMBOL_GPL(i3c_bus_maintenance_unlock); + +/** + * i3c_bus_normaluse_lock - Lock the bus for a normal operation + * @bus: I3C bus to take the lock on + * + * This function takes the bus lock for any operation that is not a maintenance + * operation (see i3c_bus_maintenance_lock() for a non-exhaustive list of + * maintenance operations). Basically all communications with I3C devices are + * normal operations (HDR, SDR transfers or CCC commands that do not change bus + * state or I3C dynamic address). + * + * Note that this lock is not guaranteeing serialization of normal operations. + * In other words, transfer requests passed to the I3C master can be submitted + * in parallel and I3C master drivers have to use their own locking to make + * sure two different communications are not inter-mixed, or access to the + * output/input queue is not done while the engine is busy. + */ +void i3c_bus_normaluse_lock(struct i3c_bus *bus) +{ + down_read(&bus->lock); +} +EXPORT_SYMBOL_GPL(i3c_bus_normaluse_lock); + +/** + * i3c_bus_normaluse_unlock - Release the bus lock after a normal operation + * @bus: I3C bus to release the lock on + * + * Should be called when a normal operation is done. See + * i3c_bus_normaluse_lock() for more details on what these normal operations + * are. + */ +void i3c_bus_normaluse_unlock(struct i3c_bus *bus) +{ + up_read(&bus->lock); +} +EXPORT_SYMBOL_GPL(i3c_bus_normaluse_unlock); + +static ssize_t bcr_show(struct device *dev, + struct device_attribute *da, + char *buf) +{ + struct i3c_device *i3cdev = dev_to_i3cdev(dev); + struct i3c_bus *bus = i3c_device_get_bus(i3cdev); + ssize_t ret; + + i3c_bus_normaluse_lock(bus); + ret = sprintf(buf, "%x\n", i3cdev->info.bcr); + i3c_bus_normaluse_unlock(bus); + + return ret; +} +static DEVICE_ATTR_RO(bcr); + +static ssize_t dcr_show(struct device *dev, + struct device_attribute *da, + char *buf) +{ + struct i3c_device *i3cdev = dev_to_i3cdev(dev); + struct i3c_bus *bus = i3c_device_get_bus(i3cdev); + ssize_t ret; + + i3c_bus_normaluse_lock(bus); + ret = sprintf(buf, "%x\n", i3cdev->info.dcr); + i3c_bus_normaluse_unlock(bus); + + return ret; +} +static DEVICE_ATTR_RO(dcr); + +static ssize_t pid_show(struct device *dev, + struct device_attribute *da, + char *buf) +{ + struct i3c_device *i3cdev = dev_to_i3cdev(dev); + struct i3c_bus *bus = i3c_device_get_bus(i3cdev); + ssize_t ret; + + i3c_bus_normaluse_lock(bus); + ret = sprintf(buf, "%llx\n", i3cdev->info.pid); + i3c_bus_normaluse_unlock(bus); + + return ret; +} +static DEVICE_ATTR_RO(pid); + +static ssize_t address_show(struct device *dev, + struct device_attribute *da, + char *buf) +{ + struct i3c_device *i3cdev = dev_to_i3cdev(dev); + struct i3c_bus *bus = i3c_device_get_bus(i3cdev); + ssize_t ret; + + i3c_bus_normaluse_lock(bus); + ret = sprintf(buf, "%02x\n", i3cdev->info.dyn_addr); + i3c_bus_normaluse_unlock(bus); + + return ret; +} +static DEVICE_ATTR_RO(address); + +static const char * const hdrcap_strings[] = { + "hdr-ddr", "hdr-tsp", "hdr-tsl", +}; + +static ssize_t hdrcap_show(struct device *dev, + struct device_attribute *da, + char *buf) +{ + struct i3c_device *i3cdev = dev_to_i3cdev(dev); + struct i3c_bus *bus = i3c_device_get_bus(i3cdev); + unsigned long caps = i3cdev->info.hdr_cap; + ssize_t offset = 0, ret; + int mode; + + i3c_bus_normaluse_lock(bus); + for_each_set_bit(mode, &caps, 8) { + if (mode >= ARRAY_SIZE(hdrcap_strings)) + break; + + if (!hdrcap_strings[mode]) + continue; + + ret = sprintf(buf + offset, offset ? " %s" : "%s", + hdrcap_strings[mode]); + if (ret < 0) + goto out; + + offset += ret; + } + + ret = sprintf(buf + offset, "\n"); + if (ret < 0) + goto out; + + ret = offset + ret; + +out: + i3c_bus_normaluse_unlock(bus); + + return ret; +} +static DEVICE_ATTR_RO(hdrcap); + +static struct attribute *i3c_device_attrs[] = { + &dev_attr_bcr.attr, + &dev_attr_dcr.attr, + &dev_attr_pid.attr, + &dev_attr_address.attr, + &dev_attr_hdrcap.attr, + NULL, +}; +ATTRIBUTE_GROUPS(i3c_device); + +static int i3c_device_uevent(struct device *dev, struct kobj_uevent_env *env) +{ + struct i3c_device *i3cdev = dev_to_i3cdev(dev); + u16 manuf = I3C_PID_MANUF_ID(i3cdev->info.pid); + u16 part = I3C_PID_PART_ID(i3cdev->info.pid); + u16 ext = I3C_PID_EXTRA_INFO(i3cdev->info.pid); + + if (I3C_PID_RND_LOWER_32BITS(i3cdev->info.pid)) + return add_uevent_var(env, "MODALIAS=i3c:dcr%02Xmanuf%04X", + i3cdev->info.dcr, manuf); + + return add_uevent_var(env, + "MODALIAS=i3c:dcr%02Xmanuf%04Xpart%04xext%04x", + i3cdev->info.dcr, manuf, part, ext); +} + +const struct device_type i3c_device_type = { + .groups = i3c_device_groups, + .uevent = i3c_device_uevent, +}; + +const struct device_type i3c_master_type = { + .groups = i3c_device_groups, +}; + +static const struct i3c_device_id * +i3c_device_match_id(struct i3c_device *i3cdev, + const struct i3c_device_id *id_table) +{ + const struct i3c_device_id *id; + + /* + * The lower 32bits of the provisional ID is just filled with a random + * value, try to match using DCR info. + */ + if (!I3C_PID_RND_LOWER_32BITS(i3cdev->info.pid)) { + u16 manuf = I3C_PID_MANUF_ID(i3cdev->info.pid); + u16 part = I3C_PID_PART_ID(i3cdev->info.pid); + u16 ext_info = I3C_PID_EXTRA_INFO(i3cdev->info.pid); + + /* First try to match by manufacturer/part ID. */ + for (id = id_table; id->match_flags != 0; id++) { + if ((id->match_flags & I3C_MATCH_MANUF_AND_PART) != + I3C_MATCH_MANUF_AND_PART) + continue; + + if (manuf != id->manuf_id || part != id->part_id) + continue; + + if ((id->match_flags & I3C_MATCH_EXTRA_INFO) && + ext_info != id->extra_info) + continue; + + return id; + } + } + + /* Fallback to DCR match. */ + for (id = id_table; id->match_flags != 0; id++) { + if ((id->match_flags & I3C_MATCH_DCR) && + id->dcr == i3cdev->info.dcr) + return id; + } + + return NULL; +} + +static int i3c_device_match(struct device *dev, struct device_driver *drv) +{ + struct i3c_device *i3cdev; + struct i3c_driver *i3cdrv; + + if (dev->type != &i3c_device_type) + return 0; + + i3cdev = dev_to_i3cdev(dev); + i3cdrv = drv_to_i3cdrv(drv); + if (i3c_device_match_id(i3cdev, i3cdrv->id_table)) + return 1; + + return 0; +} + +static int i3c_device_probe(struct device *dev) +{ + struct i3c_device *i3cdev = dev_to_i3cdev(dev); + struct i3c_driver *driver = drv_to_i3cdrv(dev->driver); + + return driver->probe(i3cdev); +} + +static int i3c_device_remove(struct device *dev) +{ + struct i3c_device *i3cdev = dev_to_i3cdev(dev); + struct i3c_driver *driver = drv_to_i3cdrv(dev->driver); + int ret; + + ret = driver->remove(i3cdev); + if (ret) + return ret; + + if (WARN_ON(i3cdev->ibi)) + i3c_device_free_ibi(i3cdev); + + return ret; +} + +struct bus_type i3c_bus_type = { + .name = "i3c", + .match = i3c_device_match, + .probe = i3c_device_probe, + .remove = i3c_device_remove, +}; + +enum i3c_addr_slot_status i3c_bus_get_addr_slot_status(struct i3c_bus *bus, + u16 addr) +{ + int status, bitpos = addr * 2; + + if (addr > I2C_MAX_ADDR) + return I3C_ADDR_SLOT_RSVD; + + status = bus->addrslots[bitpos / BITS_PER_LONG]; + status >>= bitpos % BITS_PER_LONG; + + return status & I3C_ADDR_SLOT_STATUS_MASK; +} + +void i3c_bus_set_addr_slot_status(struct i3c_bus *bus, u16 addr, + enum i3c_addr_slot_status status) +{ + int bitpos = addr * 2; + unsigned long *ptr; + + if (addr > I2C_MAX_ADDR) + return; + + ptr = bus->addrslots + (bitpos / BITS_PER_LONG); + *ptr &= ~(I3C_ADDR_SLOT_STATUS_MASK << (bitpos % BITS_PER_LONG)); + *ptr |= status << (bitpos % BITS_PER_LONG); +} + +bool i3c_bus_dev_addr_is_avail(struct i3c_bus *bus, u8 addr) +{ + enum i3c_addr_slot_status status; + + status = i3c_bus_get_addr_slot_status(bus, addr); + + return status == I3C_ADDR_SLOT_FREE; +} + +int i3c_bus_get_free_addr(struct i3c_bus *bus, u8 start_addr) +{ + enum i3c_addr_slot_status status; + u8 addr; + + for (addr = start_addr; addr < I3C_MAX_ADDR; addr++) { + status = i3c_bus_get_addr_slot_status(bus, addr); + if (status == I3C_ADDR_SLOT_FREE) + return addr; + } + + return -ENOMEM; +} + +static void i3c_bus_init_addrslots(struct i3c_bus *bus) +{ + int i; + + /* Addresses 0 to 7 are reserved. */ + for (i = 0; i < 8; i++) + i3c_bus_set_addr_slot_status(bus, i, I3C_ADDR_SLOT_RSVD); + + /* + * Reserve broadcast address and all addresses that might collide + * with the broadcast address when facing a single bit error. + */ + i3c_bus_set_addr_slot_status(bus, I3C_BROADCAST_ADDR, + I3C_ADDR_SLOT_RSVD); + for (i = 0; i < 7; i++) + i3c_bus_set_addr_slot_status(bus, I3C_BROADCAST_ADDR ^ BIT(i), + I3C_ADDR_SLOT_RSVD); +} + +static const char * const i3c_bus_mode_strings[] = { + [I3C_BUS_MODE_PURE] = "pure", + [I3C_BUS_MODE_MIXED_FAST] = "mixed-fast", + [I3C_BUS_MODE_MIXED_SLOW] = "mixed-slow", +}; + +static ssize_t mode_show(struct device *dev, + struct device_attribute *da, + char *buf) +{ + struct i3c_bus *i3cbus = container_of(dev, struct i3c_bus, dev); + ssize_t ret; + + i3c_bus_normaluse_lock(i3cbus); + if (i3cbus->mode < 0 || + i3cbus->mode > ARRAY_SIZE(i3c_bus_mode_strings) || + !i3c_bus_mode_strings[i3cbus->mode]) + ret = sprintf(buf, "unknown\n"); + else + ret = sprintf(buf, "%s\n", i3c_bus_mode_strings[i3cbus->mode]); + i3c_bus_normaluse_unlock(i3cbus); + + return ret; +} +static DEVICE_ATTR_RO(mode); + +static ssize_t current_master_show(struct device *dev, + struct device_attribute *da, + char *buf) +{ + struct i3c_bus *i3cbus = container_of(dev, struct i3c_bus, dev); + ssize_t ret; + + i3c_bus_normaluse_lock(i3cbus); + ret = sprintf(buf, "%s\n", dev_name(&i3cbus->cur_master->dev)); + i3c_bus_normaluse_unlock(i3cbus); + + return ret; +} +static DEVICE_ATTR_RO(current_master); + +static ssize_t i3c_scl_frequency_show(struct device *dev, + struct device_attribute *da, + char *buf) +{ + struct i3c_bus *i3cbus = container_of(dev, struct i3c_bus, dev); + ssize_t ret; + + i3c_bus_normaluse_lock(i3cbus); + ret = sprintf(buf, "%ld\n", i3cbus->scl_rate.i3c); + i3c_bus_normaluse_unlock(i3cbus); + + return ret; +} +static DEVICE_ATTR_RO(i3c_scl_frequency); + +static ssize_t i2c_scl_frequency_show(struct device *dev, + struct device_attribute *da, + char *buf) +{ + struct i3c_bus *i3cbus = container_of(dev, struct i3c_bus, dev); + ssize_t ret; + + i3c_bus_normaluse_lock(i3cbus); + ret = sprintf(buf, "%ld\n", i3cbus->scl_rate.i2c); + i3c_bus_normaluse_unlock(i3cbus); + + return ret; +} +static DEVICE_ATTR_RO(i2c_scl_frequency); + +static struct attribute *i3c_busdev_attrs[] = { + &dev_attr_mode.attr, + &dev_attr_current_master.attr, + &dev_attr_i3c_scl_frequency.attr, + &dev_attr_i2c_scl_frequency.attr, + NULL, +}; +ATTRIBUTE_GROUPS(i3c_busdev); + +static void i3c_busdev_release(struct device *dev) +{ + struct i3c_bus *bus = container_of(dev, struct i3c_bus, dev); + + while (!list_empty(&bus->devs.i2c)) { + struct i2c_device *i2cdev; + + i2cdev = list_first_entry(&bus->devs.i2c, struct i2c_device, + common.node); + list_del(&i2cdev->common.node); + of_node_put(i2cdev->info.of_node); + kfree(i2cdev); + } + + while (!list_empty(&bus->devs.i3c)) { + struct i3c_device *i3cdev; + + i3cdev = list_first_entry(&bus->devs.i3c, struct i3c_device, + common.node); + list_del(&i3cdev->common.node); + put_device(&i3cdev->dev); + } + + mutex_lock(&i3c_core_lock); + idr_remove(&i3c_bus_idr, bus->id); + mutex_unlock(&i3c_core_lock); + + of_node_put(bus->dev.of_node); + kfree(bus); +} + +static const struct device_type i3c_busdev_type = { + .groups = i3c_busdev_groups, +}; + +void i3c_bus_unref(struct i3c_bus *bus) +{ + put_device(&bus->dev); +} + +struct i3c_bus *i3c_bus_create(struct device *parent) +{ + struct i3c_bus *i3cbus; + int ret; + + i3cbus = kzalloc(sizeof(*i3cbus), GFP_KERNEL); + if (!i3cbus) + return ERR_PTR(-ENOMEM); + + init_rwsem(&i3cbus->lock); + INIT_LIST_HEAD(&i3cbus->devs.i2c); + INIT_LIST_HEAD(&i3cbus->devs.i3c); + i3c_bus_init_addrslots(i3cbus); + i3cbus->mode = I3C_BUS_MODE_PURE; + i3cbus->dev.parent = parent; + i3cbus->dev.of_node = of_node_get(parent->of_node); + i3cbus->dev.bus = &i3c_bus_type; + i3cbus->dev.type = &i3c_busdev_type; + i3cbus->dev.release = i3c_busdev_release; + + mutex_lock(&i3c_core_lock); + ret = idr_alloc(&i3c_bus_idr, i3cbus, 0, 0, GFP_KERNEL); + mutex_unlock(&i3c_core_lock); + if (ret < 0) + goto err_free_bus; + + i3cbus->id = ret; + device_initialize(&i3cbus->dev); + + return i3cbus; + +err_free_bus: + kfree(i3cbus); + + return ERR_PTR(ret); +} + +void i3c_bus_unregister(struct i3c_bus *bus) +{ + device_unregister(&bus->dev); +} + +int i3c_bus_register(struct i3c_bus *i3cbus) +{ + struct i2c_device *i2cdev; + + i3c_bus_for_each_i2cdev(i3cbus, i2cdev) { + switch (i2cdev->lvr & I3C_LVR_I2C_INDEX_MASK) { + case I3C_LVR_I2C_INDEX(0): + if (i3cbus->mode < I3C_BUS_MODE_MIXED_FAST) + i3cbus->mode = I3C_BUS_MODE_MIXED_FAST; + break; + + case I3C_LVR_I2C_INDEX(1): + case I3C_LVR_I2C_INDEX(2): + if (i3cbus->mode < I3C_BUS_MODE_MIXED_SLOW) + i3cbus->mode = I3C_BUS_MODE_MIXED_SLOW; + break; + + default: + return -EINVAL; + } + } + + if (!i3cbus->scl_rate.i3c) + i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; + + if (!i3cbus->scl_rate.i2c) { + if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW) + i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE; + else + i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_PLUS_SCL_RATE; + } + + /* + * I3C/I2C frequency may have been overridden, check that user-provided + * values are not exceeding max possible frequency. + */ + if (i3cbus->scl_rate.i3c > I3C_BUS_MAX_I3C_SCL_RATE || + i3cbus->scl_rate.i2c > I3C_BUS_I2C_FM_PLUS_SCL_RATE) { + return -EINVAL; + } + + dev_set_name(&i3cbus->dev, "i3c-%d", i3cbus->id); + + return device_add(&i3cbus->dev); +} + +static int __init i3c_init(void) +{ + return bus_register(&i3c_bus_type); +} +subsys_initcall(i3c_init); + +static void __exit i3c_exit(void) +{ + idr_destroy(&i3c_bus_idr); + bus_unregister(&i3c_bus_type); +} +module_exit(i3c_exit); + +MODULE_AUTHOR("Boris Brezillon <boris.brezillon@bootlin.com>"); +MODULE_DESCRIPTION("I3C core"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c new file mode 100644 index 000000000000..8948d9bdec82 --- /dev/null +++ b/drivers/i3c/device.c @@ -0,0 +1,294 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2017 Cadence Design Systems Inc. + * + * Author: Boris Brezillon <boris.brezillon@bootlin.com> + */ + +#include <linux/atomic.h> +#include <linux/bug.h> +#include <linux/completion.h> +#include <linux/device.h> +#include <linux/mutex.h> +#include <linux/slab.h> + +#include "internals.h" + +/** + * i3c_device_do_priv_xfers() - do I3C SDR private transfers directed to a + * specific device + * + * @dev: device with which the transfers should be done + * @xfers: array of transfers + * @nxfers: number of transfers + * + * Initiate one or several private SDR transfers with @dev. + * + * This function can sleep and thus cannot be called in atomic context. + * + * Return: 0 in case of success, a negative error core otherwise. + */ +int i3c_device_do_priv_xfers(struct i3c_device *dev, + struct i3c_priv_xfer *xfers, + int nxfers) +{ + struct i3c_master_controller *master; + int ret, i; + + if (nxfers < 1) + return 0; + + master = i3c_device_get_master(dev); + if (!master || !xfers) + return -EINVAL; + + if (!master->ops->priv_xfers) + return -ENOTSUPP; + + for (i = 0; i < nxfers; i++) { + if (!xfers[i].len || !xfers[i].data.in) + return -EINVAL; + } + + i3c_bus_normaluse_lock(master->bus); + ret = master->ops->priv_xfers(dev, xfers, nxfers); + i3c_bus_normaluse_unlock(master->bus); + + return ret; +} +EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers); + +/** + * i3c_device_get_info() - get I3C device information + * + * @dev: device we want information on + * @info: the information object to fill in + * + * Retrieve I3C dev info. + */ +void i3c_device_get_info(struct i3c_device *dev, + struct i3c_device_info *info) +{ + if (info) + *info = dev->info; +} +EXPORT_SYMBOL_GPL(i3c_device_get_info); + +/** + * i3c_device_disable_ibi() - Disable IBIs coming from a specific device + * @dev: device on which IBIs should be disabled + * + * This function disable IBIs coming from a specific device and wait for + * all pending IBIs to be processed. + * + * Return: 0 in case of success, a negative error core otherwise. + */ +int i3c_device_disable_ibi(struct i3c_device *dev) +{ + struct i3c_master_controller *master = i3c_device_get_master(dev); + int ret; + + i3c_bus_normaluse_lock(master->bus); + mutex_lock(&dev->ibi_lock); + if (!dev->ibi) { + ret = -EINVAL; + goto out; + } + + ret = master->ops->disable_ibi(dev); + if (ret) + goto out; + + reinit_completion(&dev->ibi->all_ibis_handled); + if (atomic_read(&dev->ibi->pending_ibis)) + wait_for_completion(&dev->ibi->all_ibis_handled); + + dev->ibi->enabled = false; + +out: + mutex_unlock(&dev->ibi_lock); + i3c_bus_normaluse_unlock(master->bus); + + return 0; +} +EXPORT_SYMBOL_GPL(i3c_device_disable_ibi); + +/** + * i3c_device_enable_ibi() - Enable IBIs coming from a specific device + * @dev: device on which IBIs should be enabled + * + * This function enable IBIs coming from a specific device and wait for + * all pending IBIs to be processed. This should be called on a device + * where i3c_device_request_ibi() has succeeded. + * + * Note that IBIs from this device might be received before this function + * returns to its caller. + * + * Return: 0 in case of success, a negative error core otherwise. + */ +int i3c_device_enable_ibi(struct i3c_device *dev) +{ + struct i3c_master_controller *master = i3c_device_get_master(dev); + int ret; + + i3c_bus_normaluse_lock(master->bus); + mutex_lock(&dev->ibi_lock); + if (!dev->ibi) { + ret = -EINVAL; + goto out; + } + + ret = master->ops->enable_ibi(dev); + if (!ret) + dev->ibi->enabled = true; + +out: + mutex_unlock(&dev->ibi_lock); + i3c_bus_normaluse_unlock(master->bus); + + return ret; +} +EXPORT_SYMBOL_GPL(i3c_device_enable_ibi); + +/** + * i3c_device_request_ibi() - Request an IBI + * @dev: device for which we should enable IBIs + * @req: setup requested for this IBI + * + * This function is responsible for pre-allocating all resources needed to + * process IBIs coming from @dev. When this function returns, the IBI is not + * enabled until i3c_device_enable_ibi() is called. + * + * Return: 0 in case of success, a negative error core otherwise. + */ +int i3c_device_request_ibi(struct i3c_device *dev, + const struct i3c_ibi_setup *req) +{ + struct i3c_master_controller *master = i3c_device_get_master(dev); + struct i3c_device_ibi_info *ibi; + int ret; + + if (!master->ops->request_ibi) + return -ENOTSUPP; + + if (!req->handler || !req->num_slots) + return -EINVAL; + + i3c_bus_normaluse_lock(master->bus); + mutex_lock(&dev->ibi_lock); + if (dev->ibi) { + ret = -EBUSY; + goto out; + } + + ibi = kzalloc(sizeof(*ibi), GFP_KERNEL); + if (!ibi) { + ret = -ENOMEM; + goto out; + } + + atomic_set(&ibi->pending_ibis, 0); + init_completion(&ibi->all_ibis_handled); + ibi->handler = req->handler; + ibi->max_payload_len = req->max_payload_len; + + dev->ibi = ibi; + ret = master->ops->request_ibi(dev, req); + if (ret) { + kfree(ibi); + dev->ibi = NULL; + } + +out: + mutex_unlock(&dev->ibi_lock); + i3c_bus_normaluse_unlock(master->bus); + + return ret; +} +EXPORT_SYMBOL_GPL(i3c_device_request_ibi); + +/** + * i3c_device_free_ibi() - Free all resources needed for IBI handling + * @dev: device on which you want to release IBI resources + * + * This function is responsible for de-allocating resources previously + * allocated by i3c_device_request_ibi(). It should be called after disabling + * IBIs with i3c_device_disable_ibi(). + */ +void i3c_device_free_ibi(struct i3c_device *dev) +{ + struct i3c_master_controller *master = i3c_device_get_master(dev); + + i3c_bus_normaluse_lock(master->bus); + mutex_lock(&dev->ibi_lock); + if (!dev->ibi) + goto out; + + if (WARN_ON(dev->ibi->enabled)) + BUG_ON(i3c_device_disable_ibi(dev)); + + master->ops->free_ibi(dev); + kfree(dev->ibi); + dev->ibi = NULL; + +out: + mutex_unlock(&dev->ibi_lock); + i3c_bus_normaluse_unlock(master->bus); +} +EXPORT_SYMBOL_GPL(i3c_device_free_ibi); + +/** + * i3cdev_to_dev() - Returns the device embedded in @i3cdev + * @i3cdev: I3C device + * + * Return: a pointer to a device object. + */ +struct device *i3cdev_to_dev(struct i3c_device *i3cdev) +{ + return &i3cdev->dev; +} +EXPORT_SYMBOL_GPL(i3cdev_to_dev); + +/** + * dev_to_i3cdev() - Returns the I3C device containing @dev + * @dev: device object + * + * Return: a pointer to an I3C device object. + */ +struct i3c_device *dev_to_i3cdev(struct device *dev) +{ + return container_of(dev, struct i3c_device, dev); +} +EXPORT_SYMBOL_GPL(dev_to_i3cdev); + +/** + * i3c_driver_register_with_owner() - register an I3C device driver + * + * @drv: driver to register + * @owner: module that owns this driver + * + * Register @drv to the core. + * + * Return: 0 in case of success, a negative error core otherwise. + */ +int i3c_driver_register_with_owner(struct i3c_driver *drv, struct module *owner) +{ + drv->driver.owner = owner; + drv->driver.bus = &i3c_bus_type; + + return driver_register(&drv->driver); +} +EXPORT_SYMBOL_GPL(i3c_driver_register_with_owner); + +/** + * i3c_driver_unregister() - unregister an I3C device driver + * + * @drv: driver to unregister + * + * Unregister @drv. + */ +void i3c_driver_unregister(struct i3c_driver *drv) +{ + driver_unregister(&drv->driver); +} +EXPORT_SYMBOL_GPL(i3c_driver_unregister); diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h new file mode 100644 index 000000000000..155e1145357a --- /dev/null +++ b/drivers/i3c/internals.h @@ -0,0 +1,28 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2017 Cadence Design Systems Inc. + * + * Author: Boris Brezillon <boris.brezillon@bootlin.com> + */ + +#ifndef I3C_INTERNALS_H +#define I3C_INTERNALS_H + +#include <linux/i3c/master.h> + +extern struct bus_type i3c_bus_type; +extern const struct device_type i3c_master_type; +extern const struct device_type i3c_device_type; + +void i3c_bus_unref(struct i3c_bus *bus); +struct i3c_bus *i3c_bus_create(struct device *parent); +void i3c_bus_unregister(struct i3c_bus *bus); +int i3c_bus_register(struct i3c_bus *i3cbus); +int i3c_bus_get_free_addr(struct i3c_bus *bus, u8 start_addr); +bool i3c_bus_dev_addr_is_avail(struct i3c_bus *bus, u8 addr); +void i3c_bus_set_addr_slot_status(struct i3c_bus *bus, u16 addr, + enum i3c_addr_slot_status status); +enum i3c_addr_slot_status i3c_bus_get_addr_slot_status(struct i3c_bus *bus, + u16 addr); + +#endif /* I3C_INTERNAL_H */ diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c new file mode 100644 index 000000000000..81a91b1cf98c --- /dev/null +++ b/drivers/i3c/master.c @@ -0,0 +1,1723 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2017 Cadence Design Systems Inc. + * + * Author: Boris Brezillon <boris.brezillon@bootlin.com> + */ + +#include <linux/atomic.h> +#include <linux/bug.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/export.h> +#include <linux/kernel.h> +#include <linux/list.h> +#include <linux/of.h> +#include <linux/slab.h> +#include <linux/spinlock.h> +#include <linux/workqueue.h> + +#include "internals.h" + +static struct i3c_master_controller * +i2c_adapter_to_i3c_master(struct i2c_adapter *adap) +{ + return container_of(adap, struct i3c_master_controller, i2c); +} + +static struct i2c_adapter * +i3c_master_to_i2c_adapter(struct i3c_master_controller *master) +{ + return &master->i2c; +} + +static struct i2c_device * +i3c_master_alloc_i2c_dev(struct i3c_master_controller *master, + const struct i2c_board_info *info, u8 lvr) +{ + struct i2c_device *dev; + + dev = kzalloc(sizeof(*dev), GFP_KERNEL); + if (!dev) + return ERR_PTR(-ENOMEM); + + dev->common.bus = master->bus; + dev->info = *info; + dev->lvr = lvr; + dev->info.of_node = of_node_get(info->of_node); + i3c_bus_set_addr_slot_status(master->bus, info->addr, + I3C_ADDR_SLOT_I2C_DEV); + + return dev; +} + +static int i3c_master_send_ccc_cmd_locked(struct i3c_master_controller *master, + struct i3c_ccc_cmd *cmd) +{ + if (!cmd || !master) + return -EINVAL; + + if (WARN_ON(master->init_done && + !rwsem_is_locked(&master->bus->lock))) + return -EINVAL; + + if (!master->ops->send_ccc_cmd) + return -ENOTSUPP; + + if ((cmd->id & I3C_CCC_DIRECT) && (!cmd->dests || !cmd->ndests)) + return -EINVAL; + + if (master->ops->supports_ccc_cmd && + !master->ops->supports_ccc_cmd(master, cmd)) + return -ENOTSUPP; + + return master->ops->send_ccc_cmd(master, cmd); +} + +static struct i2c_device * +i3c_master_find_i2c_dev_by_addr(const struct i3c_master_controller *master, + u16 addr) +{ + struct i2c_device *dev; + + i3c_bus_for_each_i2cdev(master->bus, dev) { + if (dev->client->addr == addr) + return dev; + } + + return NULL; +} + +/** + * i3c_master_get_free_addr() - get a free address on the bus + * @master: I3C master object + * @start_addr: where to start searching + * + * This function must be called with the bus lock held in write mode. + * + * Return: the first free address starting at @start_addr (included) or -ENOMEM + * if there's no more address available. + */ +int i3c_master_get_free_addr(struct i3c_master_controller *master, + u8 start_addr) +{ + return i3c_bus_get_free_addr(master->bus, start_addr); +} +EXPORT_SYMBOL_GPL(i3c_master_get_free_addr); + +static void i3c_device_release(struct device *dev) +{ + struct i3c_device *i3cdev = dev_to_i3cdev(dev); + + if (i3cdev->info.static_addr) + i3c_bus_set_addr_slot_status(i3cdev->common.bus, + i3cdev->info.static_addr, + I3C_ADDR_SLOT_FREE); + + if (i3cdev->info.dyn_addr) + i3c_bus_set_addr_slot_status(i3cdev->common.bus, + i3cdev->info.dyn_addr, + I3C_ADDR_SLOT_FREE); + + of_node_put(dev->of_node); + kfree(i3cdev); +} + +static struct i3c_device * +i3c_master_alloc_i3c_dev(struct i3c_master_controller *master, + const struct i3c_device_info *info, + const struct device_type *devtype) +{ + struct i3c_device *dev; + + dev = kzalloc(sizeof(*dev), GFP_KERNEL); + if (!dev) + return ERR_PTR(-ENOMEM); + + dev->common.bus = master->bus; + dev->dev.parent = &master->bus->dev; + dev->dev.type = devtype; + dev->dev.bus = &i3c_bus_type; + dev->dev.release = i3c_device_release; + dev->info = *info; + mutex_init(&dev->ibi_lock); + dev_set_name(&dev->dev, "%d-%llx", master->bus->id, info->pid); + + device_initialize(&dev->dev); + + if (info->static_addr) + i3c_bus_set_addr_slot_status(master->bus, info->static_addr, + I3C_ADDR_SLOT_I3C_DEV); + + if (info->dyn_addr) + i3c_bus_set_addr_slot_status(master->bus, info->dyn_addr, + I3C_ADDR_SLOT_I3C_DEV); + + return dev; +} + +/** + * i3c_master_set_info() - set master device information + * @master: master used to send frames on the bus + * @info: I3C device information + * + * Set master device info. This should be called from + * &i3c_master_controller_ops->bus_init(). + * + * Not all &i3c_device_info fields are meaningful for a master device. + * Here is a list of fields that should be properly filled: + * + * - &i3c_device_info->dyn_addr + * - &i3c_device_info->bcr + * - &i3c_device_info->dcr + * - &i3c_device_info->pid + * - &i3c_device_info->hdr_cap if %I3C_BCR_HDR_CAP bit is set in + * &i3c_device_info->bcr + * + * This function must be called with the bus lock held in maintenance mode. + * + * Return: 0 if @info contains valid information (not every piece of + * information can be checked, but we can at least make sure @info->dyn_addr + * and @info->bcr are correct), -EINVAL otherwise. + */ +int i3c_master_set_info(struct i3c_master_controller *master, + const struct i3c_device_info *info) +{ + struct i3c_device *i3cdev; + + 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; + + i3cdev = i3c_master_alloc_i3c_dev(master, info, &i3c_master_type); + if (IS_ERR(i3cdev)) + return PTR_ERR(i3cdev); + + master->this = i3cdev; + master->bus->cur_master = master->this; + i3cdev->common.bus = master->bus; + i3cdev->common.master = master; + list_add_tail(&i3cdev->common.node, &master->bus->devs.i3c); + + return 0; +} +EXPORT_SYMBOL_GPL(i3c_master_set_info); + +static int i3c_master_rstdaa_locked(struct i3c_master_controller *master, + u8 addr) +{ + struct i3c_ccc_cmd_dest dest = { }; + struct i3c_ccc_cmd cmd = { }; + enum i3c_addr_slot_status addrstat; + int ret; + + if (!master) + return -EINVAL; + + addrstat = i3c_bus_get_addr_slot_status(master->bus, addr); + if (addr != I3C_BROADCAST_ADDR && addrstat != I3C_ADDR_SLOT_I3C_DEV) + return -EINVAL; + + dest.addr = addr; + cmd.dests = &dest; + cmd.ndests = 1; + cmd.rnw = false; + cmd.id = I3C_CCC_RSTDAA(addr == I3C_BROADCAST_ADDR); + + ret = i3c_master_send_ccc_cmd_locked(master, &cmd); + if (ret) + return ret; + + return 0; +} + +/** + * i3c_master_entdaa_locked() - start a DAA (Dynamic Address Assignment) + * procedure + * @master: master used to send frames on the bus + * + * Send a ENTDAA CCC command to start a DAA procedure. + * + * Note that this function only sends the ENTDAA CCC command, all the logic + * behind dynamic address assignment has to be handled in the I3C master + * driver. + * + * This function must be called with the bus lock held in write mode. + * + * Return: 0 in case of success, a negative error code otherwise. + */ +int i3c_master_entdaa_locked(struct i3c_master_controller *master) +{ + struct i3c_ccc_cmd_dest dest = { }; + struct i3c_ccc_cmd cmd = { }; + int ret; + + dest.addr = I3C_BROADCAST_ADDR; + cmd.dests = &dest; + cmd.ndests = 1; + cmd.rnw = false; + cmd.id = I3C_CCC_ENTDAA; + + ret = i3c_master_send_ccc_cmd_locked(master, &cmd); + if (ret) + return ret; + + return 0; +} +EXPORT_SYMBOL_GPL(i3c_master_entdaa_locked); + +/** + * i3c_master_disec_locked() - send a DISEC CCC command + * @master: master used to send frames on the bus + * @addr: a valid I3C slave address or %I3C_BROADCAST_ADDR + * @evts: events to disable + * + * Send a DISEC CCC command to disable some or all events coming from a + * specific slave, or all devices if @addr is %I3C_BROADCAST_ADDR. + * + * This function must be called with the bus lock held in write mode. + * + * Return: 0 in case of success, a negative error code otherwise. + */ +int i3c_master_disec_locked(struct i3c_master_controller *master, u8 addr, + u8 evts) +{ + struct i3c_ccc_events events = { + .events = evts, + }; + struct i3c_ccc_cmd_dest dest = { + .addr = addr, + .payload.len = sizeof(events), + .payload.data = &events, + }; + struct i3c_ccc_cmd cmd = { + .id = I3C_CCC_DISEC(addr == I3C_BROADCAST_ADDR), + .dests = &dest, + .ndests = 1, + }; + + return i3c_master_send_ccc_cmd_locked(master, &cmd); +} +EXPORT_SYMBOL_GPL(i3c_master_disec_locked); + +/** + * i3c_master_enec_locked() - send an ENEC CCC command + * @master: master used to send frames on the bus + * @addr: a valid I3C slave address or %I3C_BROADCAST_ADDR + * @evts: events to disable + * + * Sends an ENEC CCC command to enable some or all events coming from a + * specific slave, or all devices if @addr is %I3C_BROADCAST_ADDR. + * + * This function must be called with the bus lock held in write mode. + * + * Return: 0 in case of success, a negative error code otherwise. + */ +int i3c_master_enec_locked(struct i3c_master_controller *master, u8 addr, + u8 evts) +{ + struct i3c_ccc_events events = { + .events = evts, + }; + struct i3c_ccc_cmd_dest dest = { + .addr = addr, + .payload.len = sizeof(events), + .payload.data = &events, + }; + struct i3c_ccc_cmd cmd = { + .id = I3C_CCC_ENEC(addr == I3C_BROADCAST_ADDR), + .dests = &dest, + .ndests = 1, + }; + + return i3c_master_send_ccc_cmd_locked(master, &cmd); +} +EXPORT_SYMBOL_GPL(i3c_master_enec_locked); + +/** + * i3c_master_defslvs_locked() - send a DEFSLVS CCC command + * @master: master used to send frames on the bus + * + * Send a DEFSLVS CCC command containing all the devices known to the @master. + * This is useful when you have secondary masters on the bus to propagate + * device information. + * + * This should be called after all I3C devices have been discovered (in other + * words, after the DAA procedure has finished) and instantiated in + * i3c_master_controller_ops->bus_init(). + * It should also be called if a master ACKed an Hot-Join request and assigned + * a dynamic address to the device joining the bus. + * + * This function must be called with the bus lock held in write mode. + * + * Return: 0 in case of success, a negative error code otherwise. + */ +int i3c_master_defslvs_locked(struct i3c_master_controller *master) +{ + struct i3c_ccc_cmd_dest dest = { + .addr = I3C_BROADCAST_ADDR, + }; + struct i3c_ccc_cmd cmd = { + .id = I3C_CCC_DEFSLVS, + .dests = &dest, + .ndests = 1, + }; + struct i3c_ccc_defslvs *defslvs; + struct i3c_ccc_dev_desc *desc; + struct i3c_device *i3cdev; + struct i2c_device *i2cdev; + struct i3c_bus *bus; + bool send = false; + int ndevs = 0, ret; + + if (!master) + return -EINVAL; + + bus = i3c_master_get_bus(master); + i3c_bus_for_each_i3cdev(bus, i3cdev) { + ndevs++; + if (I3C_BCR_DEVICE_ROLE(i3cdev->info.bcr) == I3C_BCR_I3C_MASTER) + send = true; + } + + /* No other master on the bus, skip DEFSLVS. */ + if (!send) + return 0; + + i3c_bus_for_each_i2cdev(bus, i2cdev) + ndevs++; + + dest.payload.len = sizeof(*defslvs) + + ((ndevs - 1) * sizeof(struct i3c_ccc_dev_desc)); + defslvs = kzalloc(dest.payload.len, GFP_KERNEL); + if (!defslvs) + return -ENOMEM; + + dest.payload.data = defslvs; + + defslvs->count = ndevs; + defslvs->master.bcr = master->this->info.bcr; + defslvs->master.dcr = master->this->info.dcr; + defslvs->master.dyn_addr = master->this->info.dyn_addr; + defslvs->master.static_addr = I3C_BROADCAST_ADDR; + + desc = defslvs->slaves; + i3c_bus_for_each_i2cdev(bus, i2cdev) { + desc->lvr = i2cdev->lvr; + desc->static_addr = i2cdev->info.addr; + desc++; + } + + i3c_bus_for_each_i3cdev(bus, i3cdev) { + /* Skip the I3C dev representing this master. */ + if (i3cdev == master->this) + continue; + + desc->bcr = i3cdev->info.bcr; + desc->dcr = i3cdev->info.dcr; + desc->dyn_addr = i3cdev->info.dyn_addr; + desc->static_addr = i3cdev->info.static_addr; + desc++; + } + + ret = i3c_master_send_ccc_cmd_locked(master, &cmd); + kfree(defslvs); + + return ret; +} +EXPORT_SYMBOL_GPL(i3c_master_defslvs_locked); + +static int i3c_master_setdasa_locked(struct i3c_master_controller *master, + u8 static_addr, u8 dyn_addr) +{ + struct i3c_ccc_setda setda = { + .addr = dyn_addr << 1, + }; + struct i3c_ccc_cmd_dest dest = { + .addr = static_addr, + .payload.len = sizeof(setda), + .payload.data = &setda, + }; + struct i3c_ccc_cmd cmd = { + .rnw = false, + .id = I3C_CCC_SETDASA, + .dests = &dest, + .ndests = 1, + }; + + if (!dyn_addr || !static_addr) + return -EINVAL; + + return i3c_master_send_ccc_cmd_locked(master, &cmd); +} + +static int i3c_master_setnewda_locked(struct i3c_master_controller *master, + u8 oldaddr, u8 newaddr) +{ + struct i3c_ccc_setda setda = { + .addr = newaddr << 1, + }; + struct i3c_ccc_cmd_dest dest = { + .addr = oldaddr, + .payload.len = sizeof(setda), + .payload.data = &setda, + }; + struct i3c_ccc_cmd cmd = { + .rnw = false, + .id = I3C_CCC_SETNEWDA, + .dests = &dest, + .ndests = 1, + }; + + if (!oldaddr || !newaddr) + return -EINVAL; + + return i3c_master_send_ccc_cmd_locked(master, &cmd); +} + +static int i3c_master_getmrl_locked(struct i3c_master_controller *master, + struct i3c_device_info *info) +{ + struct i3c_ccc_mrl mrl; + struct i3c_ccc_cmd_dest dest = { + .addr = info->dyn_addr, + .payload.len = sizeof(mrl), + .payload.data = &mrl, + }; + struct i3c_ccc_cmd cmd = { + .rnw = true, + .id = I3C_CCC_GETMRL, + .dests = &dest, + .ndests = 1, + }; + int ret; + + /* + * When the device does not have IBI payload GETMRL only returns 2 + * bytes of data. + */ + if (!(info->bcr & I3C_BCR_IBI_PAYLOAD)) + dest.payload.len -= 1; + + ret = i3c_master_send_ccc_cmd_locked(master, &cmd); + if (ret) + return ret; + + if (dest.payload.len != sizeof(mrl)) + return -EIO; + + info->max_read_len = be16_to_cpu(mrl.read_len); + + if (info->bcr & I3C_BCR_IBI_PAYLOAD) + info->max_ibi_len = mrl.ibi_len; + + return 0; +} + +static int i3c_master_getmwl_locked(struct i3c_master_controller *master, + struct i3c_device_info *info) +{ + struct i3c_ccc_mwl mwl; + struct i3c_ccc_cmd_dest dest = { + .addr = info->dyn_addr, + .payload.len = sizeof(mwl), + .payload.data = &mwl, + }; + struct i3c_ccc_cmd cmd = { + .rnw = true, + .id = I3C_CCC_GETMWL, + .dests = &dest, + .ndests = 1, + }; + int ret; + + ret = i3c_master_send_ccc_cmd_locked(master, &cmd); + if (ret) + return ret; + + if (dest.payload.len != sizeof(mwl)) + return -EIO; + + info->max_write_len = be16_to_cpu(mwl.len); + + return 0; +} + +static int i3c_master_getmxds_locked(struct i3c_master_controller *master, + struct i3c_device_info *info) +{ + struct i3c_ccc_getmxds getmaxds; + struct i3c_ccc_cmd_dest dest = { + .addr = info->dyn_addr, + .payload.len = sizeof(getmaxds), + .payload.data = &getmaxds, + }; + struct i3c_ccc_cmd cmd = { + .rnw = true, + .id = I3C_CCC_GETMXDS, + .dests = &dest, + .ndests = 1, + }; + int ret; + + ret = i3c_master_send_ccc_cmd_locked(master, &cmd); + if (ret) + return ret; + + if (dest.payload.len != 2 && dest.payload.len != 5) + return -EIO; + + info->max_read_ds = getmaxds.maxrd; + info->max_read_ds = getmaxds.maxwr; + if (dest.payload.len == 5) + info->max_read_turnaround = getmaxds.maxrdturn[0] | + ((u32)getmaxds.maxrdturn[1] << 8) | + ((u32)getmaxds.maxrdturn[2] << 16); + + return 0; +} + +static int i3c_master_gethdrcap_locked(struct i3c_master_controller *master, + struct i3c_device_info *info) +{ + struct i3c_ccc_gethdrcap gethdrcap; + struct i3c_ccc_cmd_dest dest = { + .addr = info->dyn_addr, + .payload.len = sizeof(gethdrcap), + .payload.data = &gethdrcap, + }; + struct i3c_ccc_cmd cmd = { + .rnw = true, + .id = I3C_CCC_GETHDRCAP, + .dests = &dest, + .ndests = 1, + }; + int ret; + + ret = i3c_master_send_ccc_cmd_locked(master, &cmd); + if (ret) + return ret; + + if (dest.payload.len != 1) + return -EIO; + + info->hdr_cap = gethdrcap.modes; + + return 0; +} + +static int i3c_master_getpid_locked(struct i3c_master_controller *master, + struct i3c_device_info *info) +{ + struct i3c_ccc_getpid getpid; + struct i3c_ccc_cmd_dest dest = { + .addr = info->dyn_addr, + .payload.len = sizeof(struct i3c_ccc_getpid), + .payload.data = &getpid, + }; + struct i3c_ccc_cmd cmd = { + .rnw = true, + .id = I3C_CCC_GETPID, + .dests = &dest, + .ndests = 1, + }; + int ret, i; + + ret = i3c_master_send_ccc_cmd_locked(master, &cmd); + if (ret) + return ret; + + info->pid = 0; + for (i = 0; i < sizeof(getpid.pid); i++) { + int sft = (sizeof(getpid.pid) - i - 1) * 8; + + info->pid |= (u64)getpid.pid[i] << sft; + } + + return 0; +} + +static int i3c_master_getbcr_locked(struct i3c_master_controller *master, + struct i3c_device_info *info) +{ + struct i3c_ccc_getbcr getbcr; + struct i3c_ccc_cmd_dest dest = { + .addr = info->dyn_addr, + .payload.len = sizeof(struct i3c_ccc_getbcr), + .payload.data = &getbcr, + }; + struct i3c_ccc_cmd cmd = { + .rnw = true, + .id = I3C_CCC_GETBCR, + .dests = &dest, + .ndests = 1, + }; + int ret; + + ret = i3c_master_send_ccc_cmd_locked(master, &cmd); + if (ret) + return ret; + + info->bcr = getbcr.bcr; + + return 0; +} + +static int i3c_master_getdcr_locked(struct i3c_master_controller *master, + struct i3c_device_info *info) +{ + struct i3c_ccc_getdcr getdcr; + struct i3c_ccc_cmd_dest dest = { + .addr = info->dyn_addr, + .payload.len = sizeof(struct i3c_ccc_getdcr), + .payload.data = &getdcr, + }; + struct i3c_ccc_cmd cmd = { + .rnw = true, + .id = I3C_CCC_GETDCR, + .dests = &dest, + .ndests = 1, + }; + int ret; + + ret = i3c_master_send_ccc_cmd_locked(master, &cmd); + if (ret) + return ret; + + info->dcr = getdcr.dcr; + + return 0; +} + +static int i3c_master_retrieve_dev_info(struct i3c_master_controller *master, + struct i3c_device_info *info, u8 addr) +{ + enum i3c_addr_slot_status slot_status; + int ret; + + if (!master || !info) + return -EINVAL; + + memset(info, 0, sizeof(*info)); + info->dyn_addr = addr; + + slot_status = i3c_bus_get_addr_slot_status(master->bus, + info->dyn_addr); + if (slot_status == I3C_ADDR_SLOT_RSVD || + slot_status == I3C_ADDR_SLOT_I2C_DEV) + return -EINVAL; + + ret = i3c_master_getpid_locked(master, info); + if (ret) + return ret; + + ret = i3c_master_getbcr_locked(master, info); + if (ret) + return ret; + + ret = i3c_master_getdcr_locked(master, info); + if (ret) + return ret; + + if (info->bcr & I3C_BCR_MAX_DATA_SPEED_LIM) { + ret = i3c_master_getmxds_locked(master, info); + if (ret) + return ret; + } + + if (info->bcr & I3C_BCR_IBI_PAYLOAD) + info->max_ibi_len = 1; + + i3c_master_getmrl_locked(master, info); + i3c_master_getmwl_locked(master, info); + + if (info->bcr & I3C_BCR_HDR_CAP) { + ret = i3c_master_gethdrcap_locked(master, info); + if (ret) + return ret; + } + + return 0; +} + +static int i3c_master_attach_i3c_dev(struct i3c_master_controller *master, + struct i3c_device *dev) +{ + int ret; + + /* + * We don't attach devices to the controller until they are + * addressable on the bus. + */ + if (!dev->info.static_addr && !dev->info.dyn_addr) + return 0; + + dev->common.master = master; + + /* Do not attach the master device itself. */ + if (master->this == dev) + return 0; + + if (!master->ops->attach_i3c_dev) + return 0; + + ret = master->ops->attach_i3c_dev(dev); + if (ret) + dev->common.master = NULL; + + return ret; +} + +static void i3c_master_reattach_i3c_dev(struct i3c_device *dev, + u8 old_dyn_addr) +{ + struct i3c_master_controller *master = i3c_device_get_master(dev); + + if (master->ops->reattach_i3c_dev) + master->ops->reattach_i3c_dev(dev, old_dyn_addr); + + if (old_dyn_addr) + i3c_bus_set_addr_slot_status(master->bus, old_dyn_addr, + I3C_ADDR_SLOT_FREE); +} + +static void i3c_master_detach_i3c_dev(struct i3c_device *dev) +{ + struct i3c_master_controller *master = i3c_device_get_master(dev); + + if (!master) + return; + + /* Do not detach the master device itself. */ + if (master->this == dev) + return; + + if (master->ops->detach_i3c_dev) + master->ops->detach_i3c_dev(dev); + + dev->common.master = NULL; +} + +static int i3c_master_attach_i2c_dev(struct i3c_master_controller *master, + struct i2c_device *dev) +{ + int ret; + + dev->common.master = master; + + if (!master->ops->attach_i2c_dev) + return 0; + + ret = master->ops->attach_i2c_dev(dev); + if (ret) + dev->common.master = NULL; + + return ret; +} + +static void i3c_master_detach_i2c_dev(struct i2c_device *dev) +{ + struct i3c_master_controller *master = i2c_device_get_master(dev); + + if (!master) + return; + + if (master->ops->detach_i2c_dev) + master->ops->detach_i2c_dev(dev); + + dev->common.master = NULL; +} + +static void i3c_master_pre_assign_dyn_addr(struct i3c_device *dev) +{ + struct i3c_master_controller *master = i3c_device_get_master(dev); + struct i3c_device_info info; + int ret; + + if (!dev->init_dyn_addr || !dev->info.static_addr || + dev->info.dyn_addr) + return; + + ret = i3c_master_setdasa_locked(master, dev->info.static_addr, + dev->init_dyn_addr); + if (ret) + return; + + ret = i3c_master_retrieve_dev_info(master, &info, dev->init_dyn_addr); + if (ret) + goto err_rstdaa; + + dev->info = info; + + i3c_master_reattach_i3c_dev(dev, 0); + + return; + +err_rstdaa: + i3c_master_rstdaa_locked(master, dev->init_dyn_addr); +} + +static void +i3c_master_register_new_i3c_devs(struct i3c_master_controller *master) +{ + struct i3c_device *i3cdev; + int ret; + + if (!master->init_done) + return; + + i3c_bus_for_each_i3cdev(master->bus, i3cdev) { + if (i3cdev->regfailed || device_is_registered(&i3cdev->dev) || + !i3cdev->info.dyn_addr) + continue; + + ret = device_add(&i3cdev->dev); + if (ret) { + dev_err(master->parent, + "Failed to add I3C device (err = %d)\n", ret); + i3cdev->regfailed = true; + } + } +} + +/** + * i3c_master_do_daa() - do a DAA (Dynamic Address Assignment) + * @master: master doing the DAA + * + * 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_device_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_do_daa(struct i3c_master_controller *master) +{ + int ret; + + i3c_bus_maintenance_lock(master->bus); + ret = master->ops->do_daa(master); + i3c_bus_maintenance_unlock(master->bus); + + if (ret) + return ret; + + i3c_bus_normaluse_lock(master->bus); + i3c_master_register_new_i3c_devs(master); + i3c_bus_normaluse_unlock(master->bus); + + return 0; +} +EXPORT_SYMBOL_GPL(i3c_master_do_daa); + +/** + * 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 master's ->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_device *i3cdev; + struct i2c_device *i2cdev; + int ret; + + /* + * First attach all devices with static definitions provided by the + * FW. + */ + i3c_bus_for_each_i2cdev(master->bus, i2cdev) { + ret = i3c_master_attach_i2c_dev(master, i2cdev); + if (ret) + goto err_detach_devs; + } + + i3c_bus_for_each_i3cdev(master->bus, i3cdev) { + ret = i3c_master_attach_i3c_dev(master, i3cdev); + 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. + */ + 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->parent, + "master_set_info() was not called in ->bus_init()\n"); + ret = -EINVAL; + goto err_bus_cleanup; + } + + /* + * Reset all dynamic address that may have been assigned before + * (assigned by the bootloader for example). + */ + ret = i3c_master_rstdaa_locked(master, I3C_BROADCAST_ADDR); + if (ret) + goto err_bus_cleanup; + + /* Disable all slave events before starting DAA. */ + ret = i3c_master_disec_locked(master, I3C_BROADCAST_ADDR, + I3C_CCC_EVENT_SIR | I3C_CCC_EVENT_MR | + I3C_CCC_EVENT_HJ); + if (ret) + goto err_bus_cleanup; + + /* + * Pre-assign dynamic address and retrieve device information if + * needed. + */ + i3c_bus_for_each_i3cdev(master->bus, i3cdev) + i3c_master_pre_assign_dyn_addr(i3cdev); + + ret = i3c_master_do_daa(master); + if (ret) + goto err_rstdaa; + + return 0; + +err_rstdaa: + i3c_master_rstdaa_locked(master, I3C_BROADCAST_ADDR); + +err_bus_cleanup: + if (master->ops->bus_cleanup) + master->ops->bus_cleanup(master); + +err_detach_devs: + i3c_bus_for_each_i3cdev(master->bus, i3cdev) + i3c_master_detach_i3c_dev(i3cdev); + + i3c_bus_for_each_i2cdev(master->bus, i2cdev) + i3c_master_detach_i2c_dev(i2cdev); + + return ret; +} + +static void i3c_master_bus_cleanup(struct i3c_master_controller *master) +{ + struct i3c_device *i3cdev; + struct i2c_device *i2cdev; + + if (master->ops->bus_cleanup) + master->ops->bus_cleanup(master); + + i3c_bus_for_each_i3cdev(master->bus, i3cdev) + i3c_master_detach_i3c_dev(i3cdev); + + i3c_bus_for_each_i2cdev(master->bus, i2cdev) + i3c_master_detach_i2c_dev(i2cdev); +} + +static struct i3c_device * +i3c_master_search_i3c_dev_by_pid(struct i3c_master_controller *master, u64 pid) +{ + struct i3c_device *i3cdev; + + i3c_bus_for_each_i3cdev(master->bus, i3cdev) { + if (i3cdev->info.pid == pid) + return i3cdev; + } + + return NULL; +} + +/** + * 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_device_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) +{ + u8 old_dyn_addr, expected_dyn_addr; + enum i3c_addr_slot_status status; + struct i3c_device *i3cdev; + struct i3c_device_info info; + int ret; + + if (!master) + return -EINVAL; + + status = i3c_bus_get_addr_slot_status(master->bus, addr); + if (status != I3C_ADDR_SLOT_FREE) + return -EINVAL; + + ret = i3c_master_retrieve_dev_info(master, &info, addr); + if (ret) + return ret; + + i3cdev = i3c_master_search_i3c_dev_by_pid(master, info.pid); + if (!i3cdev) { + i3cdev = i3c_master_alloc_i3c_dev(master, &info, + &i3c_device_type); + if (IS_ERR(i3cdev)) + return PTR_ERR(i3cdev); + + list_add_tail(&i3cdev->common.node, &master->bus->devs.i3c); + } + + old_dyn_addr = i3cdev->info.dyn_addr; + i3cdev->info.dyn_addr = addr; + + if (!i3cdev->common.master) { + ret = i3c_master_attach_i3c_dev(master, i3cdev); + if (ret) + goto err_put_dev; + } else { + i3c_master_reattach_i3c_dev(i3cdev, old_dyn_addr); + } + + /* + * Depending on our previous state, the expected dynamic address might + * differ: + * - if the device already had a dynamic address assigned, let's try to + * re-apply this one + * - if the device did not have a dynamic address and the firmware + * requested a specific address, pick this one + * - in any other case, keep the address automatically assigned by the + * master + */ + if (old_dyn_addr) + expected_dyn_addr = old_dyn_addr; + else if (i3cdev->init_dyn_addr) + expected_dyn_addr = i3cdev->init_dyn_addr; + else + expected_dyn_addr = i3cdev->info.dyn_addr; + + if (i3cdev->info.dyn_addr != expected_dyn_addr) { + /* + * Try to apply the expected dynamic address. If it fails, keep + * the address assigned by the master. + */ + ret = i3c_master_setnewda_locked(master, + i3cdev->info.dyn_addr, + expected_dyn_addr); + if (!ret) { + old_dyn_addr = i3cdev->info.dyn_addr; + i3cdev->info.dyn_addr = expected_dyn_addr; + i3c_master_reattach_i3c_dev(i3cdev, old_dyn_addr); + } + } + + return 0; + +err_put_dev: + list_del(&i3cdev->common.node); + put_device(&i3cdev->dev); + + return ret; +} +EXPORT_SYMBOL_GPL(i3c_master_add_i3c_dev_locked); + +#define OF_I3C_REG1_IS_I2C_DEV BIT(31) + +static int of_i3c_master_add_i2c_dev(struct i3c_master_controller *master, + struct device_node *node, u32 *reg) +{ + struct device *dev = master->parent; + struct i2c_board_info info = { }; + struct i2c_device *i2cdev; + /* LVR is encoded in the lowest byte of reg[1]. */ + u8 lvr = reg[1]; + int ret; + + ret = of_i2c_get_board_info(master->parent, node, &info); + if (ret) + return ret; + + /* + * We do not register the I2C device here, because the bus is not + * necessarily ready to transmit I2C frames, and the I2C adapter has + * not been registered yet. + * This is done in i3c_master_i2c_adapter_init() once everything is + * ready. + */ + i2cdev = i3c_master_alloc_i2c_dev(master, &info, lvr); + if (IS_ERR(i2cdev)) { + dev_err(dev, "Failed to allocate device %04x\n", info.addr); + return ret; + } + + if (lvr & I3C_LVR_I2C_FM_MODE) + master->bus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE; + + list_add_tail(&i2cdev->common.node, &master->bus->devs.i2c); + + return 0; +} + +static int of_i3c_master_add_i3c_dev(struct i3c_master_controller *master, + struct device_node *node, u32 *reg) +{ + struct i3c_device_info info = { }; + enum i3c_addr_slot_status addrstatus; + struct i3c_device *i3cdev; + u32 init_dyn_addr = 0; + + if (reg[0]) { + if (reg[0] > I3C_MAX_ADDR) + return -EINVAL; + + addrstatus = i3c_bus_get_addr_slot_status(master->bus, + reg[0]); + if (addrstatus != I3C_ADDR_SLOT_FREE) + return -EINVAL; + } + + info.static_addr = reg[0]; + + if (!of_property_read_u32(node, "assigned-address", &init_dyn_addr)) { + if (init_dyn_addr > I3C_MAX_ADDR) + return -EINVAL; + + addrstatus = i3c_bus_get_addr_slot_status(master->bus, + init_dyn_addr); + if (addrstatus != I3C_ADDR_SLOT_FREE) + return -EINVAL; + } + + info.pid = ((u64)reg[1] << 32) | reg[2]; + + if ((info.pid & GENMASK_ULL(63, 48)) || + I3C_PID_RND_LOWER_32BITS(info.pid)) + return -EINVAL; + + i3cdev = i3c_master_alloc_i3c_dev(master, &info, &i3c_device_type); + if (IS_ERR(i3cdev)) + return PTR_ERR(i3cdev); + + i3cdev->init_dyn_addr = init_dyn_addr; + i3cdev->dev.of_node = node; + list_add_tail(&i3cdev->common.node, &master->bus->devs.i3c); + + return 0; +} + +static int of_i3c_master_add_dev(struct i3c_master_controller *master, + struct device_node *node) +{ + u32 reg[3]; + int ret; + + if (!master || !node) + return -EINVAL; + + ret = of_property_read_u32_array(node, "reg", reg, ARRAY_SIZE(reg)); + if (ret) + return ret; + + if (reg[1] & OF_I3C_REG1_IS_I2C_DEV) + ret = of_i3c_master_add_i2c_dev(master, node, reg); + else + ret = of_i3c_master_add_i3c_dev(master, node, reg); + + return ret; +} + +static int of_populate_i3c_bus(struct i3c_master_controller *master) +{ + struct device *dev = &master->bus->dev; + struct device_node *i3cbus_np = dev->of_node; + struct device_node *node; + int ret; + u32 val; + + if (!i3cbus_np) + return 0; + + for_each_available_child_of_node(i3cbus_np, node) { + ret = of_i3c_master_add_dev(master, node); + if (ret) + return ret; + } + + /* + * The user might want to limit I2C and I3C speed in case some devices + * on the bus are not supporting typical rates, or if the bus topology + * prevents it from using max possible rate. + */ + if (!of_property_read_u32(i3cbus_np, "i2c-scl-hz", &val)) + master->bus->scl_rate.i2c = val; + + if (!of_property_read_u32(i3cbus_np, "i3c-scl-hz", &val)) + master->bus->scl_rate.i3c = val; + + return 0; +} + +static int i3c_master_i2c_adapter_xfer(struct i2c_adapter *adap, + struct i2c_msg *xfers, int nxfers) +{ + struct i3c_master_controller *master = i2c_adapter_to_i3c_master(adap); + struct i2c_device *dev; + int i, ret; + u16 addr; + + if (!xfers || !master || nxfers <= 0) + return -EINVAL; + + if (!master->ops->i2c_xfers) + return -ENOTSUPP; + + /* Doing transfers to different devices is not supported. */ + addr = xfers[0].addr; + for (i = 1; i < nxfers; i++) { + if (addr != xfers[i].addr) + return -ENOTSUPP; + } + + i3c_bus_normaluse_lock(master->bus); + dev = i3c_master_find_i2c_dev_by_addr(master, addr); + if (!dev) + ret = -ENOENT; + else + ret = master->ops->i2c_xfers(dev, xfers, nxfers); + i3c_bus_normaluse_unlock(master->bus); + + return ret ? ret : nxfers; +} + +static u32 i3c_master_i2c_functionalities(struct i2c_adapter *adap) +{ + struct i3c_master_controller *master = i2c_adapter_to_i3c_master(adap); + + return master->ops->i2c_funcs(master); +} + +static const struct i2c_algorithm i3c_master_i2c_algo = { + .master_xfer = i3c_master_i2c_adapter_xfer, + .functionality = i3c_master_i2c_functionalities, +}; + +static int i3c_master_i2c_adapter_init(struct i3c_master_controller *master) +{ + struct i2c_adapter *adap = i3c_master_to_i2c_adapter(master); + struct i2c_device *i2cdev; + int ret; + + adap->dev.parent = master->parent; + adap->owner = master->parent->driver->owner; + adap->algo = &i3c_master_i2c_algo; + strncpy(adap->name, dev_name(master->parent), sizeof(adap->name)); + + /* FIXME: Should we allow i3c masters to override these values? */ + adap->timeout = 1000; + adap->retries = 3; + + ret = i2c_add_adapter(adap); + if (ret) + return ret; + + /* + * 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) + i2cdev->client = i2c_new_device(adap, &i2cdev->info); + + return 0; +} + +static void i3c_master_i2c_adapter_cleanup(struct i3c_master_controller *master) +{ + i2c_del_adapter(&master->i2c); +} + +static void i3c_master_unregister_i3c_devs(struct i3c_master_controller *master) +{ + struct i3c_device *i3cdev; + + i3c_bus_for_each_i3cdev(master->bus, i3cdev) { + if (device_is_registered(&i3cdev->dev)) + device_del(&i3cdev->dev); + } +} + +/** + * i3c_master_queue_ibi() - Queue an IBI + * @dev: the device this IBI is coming from + * @slot: the IBI slot used to store the payload + * + * Queue an IBI to the controller workqueue. The IBI handler attached to + * the dev will be called from a workqueue context. + */ +void i3c_master_queue_ibi(struct i3c_device *dev, struct i3c_ibi_slot *slot) +{ + atomic_inc(&dev->ibi->pending_ibis); + queue_work(dev->common.master->wq, &slot->work); +} +EXPORT_SYMBOL_GPL(i3c_master_queue_ibi); + +static void i3c_master_handle_ibi(struct work_struct *work) +{ + struct i3c_ibi_slot *slot = container_of(work, struct i3c_ibi_slot, + work); + struct i3c_device *dev = slot->dev; + struct i3c_master_controller *master = i3c_device_get_master(dev); + struct i3c_ibi_payload payload; + + payload.data = slot->data; + payload.len = slot->len; + + dev->ibi->handler(dev, &payload); + master->ops->recycle_ibi_slot(dev, slot); + if (atomic_dec_and_test(&dev->ibi->pending_ibis)) + complete(&dev->ibi->all_ibis_handled); +} + +static void i3c_master_init_ibi_slot(struct i3c_device *dev, + struct i3c_ibi_slot *slot) +{ + slot->dev = dev; + INIT_WORK(&slot->work, i3c_master_handle_ibi); +} + +struct i3c_generic_ibi_slot { + struct list_head node; + struct i3c_ibi_slot base; +}; + +struct i3c_generic_ibi_pool { + spinlock_t lock; + unsigned int num_slots; + struct list_head free_slots; + struct list_head pending; +}; + +/** + * i3c_generic_ibi_free_pool() - Free a generic IBI pool + * @pool: the IBI pool to free + * + * Free all IBI slots allated by a generic IBI pool. + */ +void i3c_generic_ibi_free_pool(struct i3c_generic_ibi_pool *pool) +{ + struct i3c_generic_ibi_slot *slot; + unsigned int nslots = 0; + + while (!list_empty(&pool->free_slots)) { + slot = list_first_entry(&pool->free_slots, + struct i3c_generic_ibi_slot, node); + list_del(&slot->node); + kfree(slot->base.data); + kfree(slot); + nslots++; + } + + /* + * If the number of freed slots is not equal to the number of allocated + * slots we have a leak somewhere. + */ + WARN_ON(nslots != pool->num_slots); +} +EXPORT_SYMBOL_GPL(i3c_generic_ibi_free_pool); + +/** + * i3c_generic_ibi_alloc_pool() - Create a generic IBI pool + * @dev: the device this pool will be used for + * @req: IBI setup request describing what the device driver expects + * + * Create a generic IBI pool based on the information provided in @req. + * + * Return: a valid IBI pool in case of success, an ERR_PTR() otherwise. + */ +struct i3c_generic_ibi_pool * +i3c_generic_ibi_alloc_pool(struct i3c_device *dev, + const struct i3c_ibi_setup *req) +{ + struct i3c_generic_ibi_pool *pool; + struct i3c_generic_ibi_slot *slot; + unsigned int i; + int ret; + + pool = kzalloc(sizeof(*pool), GFP_KERNEL); + if (!pool) + return ERR_PTR(-ENOMEM); + + spin_lock_init(&pool->lock); + INIT_LIST_HEAD(&pool->free_slots); + INIT_LIST_HEAD(&pool->pending); + + for (i = 0; i < req->num_slots; i++) { + slot = kzalloc(sizeof(*slot), GFP_KERNEL); + if (!slot) + return ERR_PTR(-ENOMEM); + + i3c_master_init_ibi_slot(dev, &slot->base); + + if (req->max_payload_len) { + slot->base.data = kzalloc(req->max_payload_len, + GFP_KERNEL); + if (!slot->base.data) { + kfree(slot); + ret = -ENOMEM; + goto err_free_pool; + } + } + + list_add_tail(&slot->node, &pool->free_slots); + pool->num_slots++; + } + + return pool; + +err_free_pool: + i3c_generic_ibi_free_pool(pool); + return ERR_PTR(ret); +} +EXPORT_SYMBOL_GPL(i3c_generic_ibi_alloc_pool); + +/** + * i3c_generic_ibi_get_free_slot() - Get a free slot from a generic IBI pool + * @pool: the pool to query an IBI slot on + * + * Search for a free slot in a generic IBI pool. + * The slot should be returned to the pool using i3c_generic_ibi_recycle_slot() + * when it's no longer needed. + * + * Return: a pointer to a free slot, or NULL if there's no free slot available. + */ +struct i3c_ibi_slot * +i3c_generic_ibi_get_free_slot(struct i3c_generic_ibi_pool *pool) +{ + struct i3c_generic_ibi_slot *slot; + unsigned long flags; + + spin_lock_irqsave(&pool->lock, flags); + slot = list_first_entry_or_null(&pool->free_slots, + struct i3c_generic_ibi_slot, node); + if (slot) + list_del(&slot->node); + spin_unlock_irqrestore(&pool->lock, flags); + + return slot ? &slot->base : NULL; +} +EXPORT_SYMBOL_GPL(i3c_generic_ibi_get_free_slot); + +/** + * i3c_generic_ibi_recycle_slot() - Return a slot to a generic IBI pool + * @pool: the pool to return the IBI slot to + * @s: IBI slot to recycle + * + * Add an IBI slot back to its generic IBI pool. Should be called from the + * master driver struct_master_controller_ops->recycle_ibi() method. + */ +void i3c_generic_ibi_recycle_slot(struct i3c_generic_ibi_pool *pool, + struct i3c_ibi_slot *s) +{ + struct i3c_generic_ibi_slot *slot; + unsigned long flags; + + if (!s) + return; + + slot = container_of(s, struct i3c_generic_ibi_slot, base); + spin_lock_irqsave(&pool->lock, flags); + list_add_tail(&slot->node, &pool->free_slots); + spin_unlock_irqrestore(&pool->lock, flags); +} +EXPORT_SYMBOL_GPL(i3c_generic_ibi_recycle_slot); + +static void i3c_master_destroy_bus(struct i3c_master_controller *master) +{ + i3c_bus_unregister(master->bus); +} + +static int i3c_master_create_bus(struct i3c_master_controller *master) +{ + struct i3c_bus *i3cbus; + int ret; + + i3cbus = i3c_bus_create(master->parent); + if (IS_ERR(i3cbus)) + return PTR_ERR(i3cbus); + + master->bus = i3cbus; + + if (i3cbus->dev.of_node) { + ret = of_populate_i3c_bus(master); + if (ret) + goto err_destroy_bus; + } + + ret = i3c_bus_register(i3cbus); + if (ret) + goto err_destroy_bus; + + return 0; + +err_destroy_bus: + i3c_bus_unref(i3cbus); + + return ret; +} + +static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops) +{ + if (!ops || !ops->bus_init || !ops->priv_xfers || + !ops->send_ccc_cmd || !ops->do_daa || !ops->i2c_xfers || + !ops->i2c_funcs) + return -EINVAL; + + if (ops->request_ibi && + (!ops->enable_ibi || !ops->disable_ibi || !ops->free_ibi || + !ops->recycle_ibi_slot)) + return -EINVAL; + + return 0; +} + +/** + * 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 + * + * This function takes care of everything for you: + * + * - creates and initializes the I3C bus + * - populates the bus with static I2C devs if @parent->of_node is not + * NULL + * - registers all I3C devices added by the controller during bus + * initialization + * - registers the I2C adapter and all I2C devices + * + * Return: 0 in case of success, a negative error code otherwise. + */ +int i3c_master_register(struct i3c_master_controller *master, + struct device *parent, + const struct i3c_master_controller_ops *ops, + bool secondary) +{ + int ret; + + /* We do not support secondary masters yet. */ + if (secondary) + return -ENOTSUPP; + + ret = i3c_master_check_ops(ops); + if (ret) + return ret; + + master->parent = parent; + master->ops = ops; + master->secondary = secondary; + + ret = i3c_master_create_bus(master); + if (ret) + return ret; + + master->wq = alloc_workqueue("%s", 0, 0, dev_name(parent)); + if (!master->wq) { + ret = -ENOMEM; + goto err_destroy_bus; + } + + ret = i3c_master_bus_init(master); + if (ret) + goto err_destroy_wq; + + /* + * 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_cleanup_bus; + + /* + * We're done initializing the bus and the controller, we can now + * register I3C devices dicovered during the initial DAA. + */ + master->init_done = true; + i3c_bus_normaluse_lock(master->bus); + i3c_master_register_new_i3c_devs(master); + i3c_bus_normaluse_unlock(master->bus); + + return 0; + +err_cleanup_bus: + i3c_master_bus_cleanup(master); + +err_destroy_wq: + destroy_workqueue(master->wq); + +err_destroy_bus: + i3c_master_destroy_bus(master); + + return ret; +} +EXPORT_SYMBOL_GPL(i3c_master_register); + +/** + * i3c_master_unregister() - unregister an I3C master + * @master: master used to send frames on the bus + * + * Basically undo everything done in i3c_master_register(). + * + * Return: 0 in case of success, a negative error code otherwise. + */ +int i3c_master_unregister(struct i3c_master_controller *master) +{ + i3c_master_i2c_adapter_cleanup(master); + i3c_master_unregister_i3c_devs(master); + i3c_master_bus_cleanup(master); + destroy_workqueue(master->wq); + i3c_master_destroy_bus(master); + + return 0; +} +EXPORT_SYMBOL_GPL(i3c_master_unregister); diff --git a/drivers/i3c/master/Kconfig b/drivers/i3c/master/Kconfig new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/drivers/i3c/master/Makefile b/drivers/i3c/master/Makefile new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/include/linux/i3c/ccc.h b/include/linux/i3c/ccc.h new file mode 100644 index 000000000000..4d0712979ade --- /dev/null +++ b/include/linux/i3c/ccc.h @@ -0,0 +1,382 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (C) 2017 Cadence Design Systems Inc. + * + * Author: Boris Brezillon <boris.brezillon@bootlin.com> + */ + +#ifndef I3C_CCC_H +#define I3C_CCC_H + +#include <linux/bitops.h> + +/* I3C CCC (Common Command Codes) related definitions */ +#define I3C_CCC_DIRECT BIT(7) + +#define I3C_CCC_ID(id, broadcast) \ + ((id) | ((broadcast) ? 0 : I3C_CCC_DIRECT)) + +/* Commands valid in both broadcast and unicast modes */ +#define I3C_CCC_ENEC(broadcast) I3C_CCC_ID(0x0, broadcast) +#define I3C_CCC_DISEC(broadcast) I3C_CCC_ID(0x1, broadcast) +#define I3C_CCC_ENTAS(as, broadcast) I3C_CCC_ID(0x2 + (as), broadcast) +#define I3C_CCC_RSTDAA(broadcast) I3C_CCC_ID(0x6, broadcast) +#define I3C_CCC_SETMWL(broadcast) I3C_CCC_ID(0x9, broadcast) +#define I3C_CCC_SETMRL(broadcast) I3C_CCC_ID(0xa, broadcast) +#define I3C_CCC_SETXTIME(broadcast) ((broadcast) ? 0x28 : 0x98) +#define I3C_CCC_VENDOR(id, broadcast) ((id) + ((broadcast) ? 0x61 : 0xe0)) + +/* Broadcast-only commands */ +#define I3C_CCC_ENTDAA I3C_CCC_ID(0x7, true) +#define I3C_CCC_DEFSLVS I3C_CCC_ID(0x8, true) +#define I3C_CCC_ENTTM I3C_CCC_ID(0xb, true) +#define I3C_CCC_ENTHDR(x) I3C_CCC_ID(0x20 + (x), true) + +/* Unicast-only commands */ +#define I3C_CCC_SETDASA I3C_CCC_ID(0x7, false) +#define I3C_CCC_SETNEWDA I3C_CCC_ID(0x8, false) +#define I3C_CCC_GETMWL I3C_CCC_ID(0xb, false) +#define I3C_CCC_GETMRL I3C_CCC_ID(0xc, false) +#define I3C_CCC_GETPID I3C_CCC_ID(0xd, false) +#define I3C_CCC_GETBCR I3C_CCC_ID(0xe, false) +#define I3C_CCC_GETDCR I3C_CCC_ID(0xf, false) +#define I3C_CCC_GETSTATUS I3C_CCC_ID(0x10, false) +#define I3C_CCC_GETACCMST I3C_CCC_ID(0x11, false) +#define I3C_CCC_SETBRGTGT I3C_CCC_ID(0x13, false) +#define I3C_CCC_GETMXDS I3C_CCC_ID(0x14, false) +#define I3C_CCC_GETHDRCAP I3C_CCC_ID(0x15, false) +#define I3C_CCC_GETXTIME I3C_CCC_ID(0x19, false) + +#define I3C_CCC_EVENT_SIR BIT(0) +#define I3C_CCC_EVENT_MR BIT(1) +#define I3C_CCC_EVENT_HJ BIT(3) + +/** + * struct i3c_ccc_events - payload passed to ENEC/DISEC CCC + * + * @events: bitmask of I3C_CCC_EVENT_xxx events. + * + * Depending on the CCC command, the specific events coming from all devices + * (broadcast version) or a specific device (unicast version) will be + * enabled (ENEC) or disabled (DISEC). + */ +struct i3c_ccc_events { + u8 events; +} __packed; + +/** + * struct i3c_ccc_mwl - payload passed to SETMWL/GETMWL CCC + * + * @len: maximum write length in bytes + * + * The maximum write length is only applicable to SDR private messages or + * extended Write CCCs (like SETXTIME). + */ +struct i3c_ccc_mwl { + __be16 len; +} __packed; + +/** + * struct i3c_ccc_mrl - payload passed to SETMRL/GETMRL CCC + * + * @len: maximum read length in bytes + * @ibi_len: maximum IBI payload length + * + * The maximum read length is only applicable to SDR private messages or + * extended Read CCCs (like GETXTIME). + * The IBI length is only valid if the I3C slave is IBI capable + * (%I3C_BCR_IBI_REQ_CAP is set). + */ +struct i3c_ccc_mrl { + __be16 read_len; + u8 ibi_len; +} __packed; + +/** + * struct i3c_ccc_dev_desc - I3C/I2C device descriptor used for DEFSLVS + * + * @dyn_addr: dynamic address assigned to the I3C slave or 0 if the entry is + * describing an I2C slave. + * @dcr: DCR value (not applicable to entries describing I2C devices) + * @lvr: LVR value (not applicable to entries describing I3C devices) + * @bcr: BCR value or 0 if this entry is describing an I2C slave + * @static_addr: static address or 0 if the device does not have a static + * address + * + * The DEFSLVS command should be passed an array of i3c_ccc_dev_desc + * descriptors (one entry per I3C/I2C dev controlled by the master). + */ +struct i3c_ccc_dev_desc { + u8 dyn_addr; + union { + u8 dcr; + u8 lvr; + }; + u8 bcr; + u8 static_addr; +} __packed; + +/** + * struct i3c_ccc_defslvs - payload passed to DEFSLVS CCC + * + * @count: number of dev descriptors + * @master: descriptor describing the current master + * @slaves: array of descriptors describing slaves controlled by the + * current master + * + * Information passed to the broadcast DEFSLVS to propagate device + * information to all masters currently acting as slaves on the bus. + * This is only meaningful if you have more than one master. + */ +struct i3c_ccc_defslvs { + u8 count; + struct i3c_ccc_dev_desc master; + struct i3c_ccc_dev_desc slaves[0]; +} __packed; + +/** + * enum i3c_ccc_test_mode - enum listing all available test modes + * + * @I3C_CCC_EXIT_TEST_MODE: exit test mode + * @I3C_CCC_VENDOR_TEST_MODE: enter vendor test mode + */ +enum i3c_ccc_test_mode { + I3C_CCC_EXIT_TEST_MODE, + I3C_CCC_VENDOR_TEST_MODE, +}; + +/** + * struct i3c_ccc_enttm - payload passed to ENTTM CCC + * + * @mode: one of the &enum i3c_ccc_test_mode modes + * + * Information passed to the ENTTM CCC to instruct an I3C device to enter a + * specific test mode. + */ +struct i3c_ccc_enttm { + u8 mode; +} __packed; + +/** + * struct i3c_ccc_setda - payload passed to SETNEWDA and SETDASA CCCs + * + * @addr: dynamic address to assign to an I3C device + * + * Information passed to the SETNEWDA and SETDASA CCCs to assign/change the + * dynamic address of an I3C device. + */ +struct i3c_ccc_setda { + u8 addr; +} __packed; + +/** + * struct i3c_ccc_getpid - payload passed to GETPID CCC + * + * @pid: 48 bits PID in big endian + */ +struct i3c_ccc_getpid { + u8 pid[6]; +} __packed; + +/** + * struct i3c_ccc_getbcr - payload passed to GETBCR CCC + * + * @bcr: BCR (Bus Characteristic Register) value + */ +struct i3c_ccc_getbcr { + u8 bcr; +} __packed; + +/** + * struct i3c_ccc_getdcr - payload passed to GETDCR CCC + * + * @dcr: DCR (Device Characteristic Register) value + */ +struct i3c_ccc_getdcr { + u8 dcr; +} __packed; + +#define I3C_CCC_STATUS_PENDING_INT(status) ((status) & GENMASK(3, 0)) +#define I3C_CCC_STATUS_PROTOCOL_ERROR BIT(5) +#define I3C_CCC_STATUS_ACTIVITY_MODE(status) \ + (((status) & GENMASK(7, 6)) >> 6) + +/** + * struct i3c_ccc_getstatus - payload passed to GETSTATUS CCC + * + * @status: status of the I3C slave (see I3C_CCC_STATUS_xxx macros for more + * information). + */ +struct i3c_ccc_getstatus { + __be16 status; +} __packed; + +/** + * struct i3c_ccc_getaccmst - payload passed to GETACCMST CCC + * + * @newmaster: address of the master taking bus ownership + */ +struct i3c_ccc_getaccmst { + u8 newmaster; +} __packed; + +/** + * struct i3c_ccc_bridged_slave_desc - bridged slave descriptor + * + * @addr: dynamic address of the bridged device + * @id: ID of the slave device behind the bridge + */ +struct i3c_ccc_bridged_slave_desc { + u8 addr; + __be16 id; +} __packed; + +/** + * struct i3c_ccc_setbrgtgt - payload passed to SETBRGTGT CCC + * + * @count: number of bridged slaves + * @bslaves: bridged slave descriptors + */ +struct i3c_ccc_setbrgtgt { + u8 count; + struct i3c_ccc_bridged_slave_desc bslaves[0]; +} __packed; + +/** + * enum i3c_sdr_max_data_rate - max data rate values for private SDR transfers + */ +enum i3c_sdr_max_data_rate { + I3C_SDR0_FSCL_MAX, + I3C_SDR1_FSCL_8MHZ, + I3C_SDR2_FSCL_6MHZ, + I3C_SDR3_FSCL_4MHZ, + I3C_SDR4_FSCL_2MHZ, +}; + +/** + * enum i3c_tsco - clock to data turn-around + */ +enum i3c_tsco { + I3C_TSCO_8NS, + I3C_TSCO_9NS, + I3C_TSCO_10NS, + I3C_TSCO_11NS, + I3C_TSCO_12NS, +}; + +#define I3C_CCC_MAX_SDR_FSCL_MASK GENMASK(2, 0) +#define I3C_CCC_MAX_SDR_FSCL(x) ((x) & I3C_CCC_MAX_SDR_FSCL_MASK) + +/** + * struct i3c_ccc_getmxds - payload passed to GETMXDS CCC + * + * @maxwr: write limitations + * @maxrd: read limitations + * @maxrdturn: maximum read turn-around expressed micro-seconds and + * little-endian formatted + */ +struct i3c_ccc_getmxds { + u8 maxwr; + u8 maxrd; + u8 maxrdturn[3]; +} __packed; + +#define I3C_CCC_HDR_MODE(mode) BIT(mode) + +/** + * struct i3c_ccc_gethdrcap - payload passed to GETHDRCAP CCC + * + * @modes: bitmap of supported HDR modes + */ +struct i3c_ccc_gethdrcap { + u8 modes; +} __packed; + +/** + * enum i3c_ccc_setxtime_subcmd - SETXTIME sub-commands + */ +enum i3c_ccc_setxtime_subcmd { + I3C_CCC_SETXTIME_ST = 0x7f, + I3C_CCC_SETXTIME_DT = 0xbf, + I3C_CCC_SETXTIME_ENTER_ASYNC_MODE0 = 0xdf, + I3C_CCC_SETXTIME_ENTER_ASYNC_MODE1 = 0xef, + I3C_CCC_SETXTIME_ENTER_ASYNC_MODE2 = 0xf7, + I3C_CCC_SETXTIME_ENTER_ASYNC_MODE3 = 0xfb, + I3C_CCC_SETXTIME_ASYNC_TRIGGER = 0xfd, + I3C_CCC_SETXTIME_TPH = 0x3f, + I3C_CCC_SETXTIME_TU = 0x9f, + I3C_CCC_SETXTIME_ODR = 0x8f, +}; + +/** + * struct i3c_ccc_setxtime - payload passed to SETXTIME CCC + * + * @subcmd: one of the sub-commands ddefined in &enum i3c_ccc_setxtime_subcmd + * @data: sub-command payload. Amount of data is determined by + * &i3c_ccc_setxtime->subcmd + */ +struct i3c_ccc_setxtime { + u8 subcmd; + u8 data[0]; +} __packed; + +#define I3C_CCC_GETXTIME_SYNC_MODE BIT(0) +#define I3C_CCC_GETXTIME_ASYNC_MODE(x) BIT((x) + 1) +#define I3C_CCC_GETXTIME_OVERFLOW BIT(7) + +/** + * struct i3c_ccc_getxtime - payload retrieved from GETXTIME CCC + * + * @supported_modes: bitmap describing supported XTIME modes + * @state: current status (enabled mode and overflow status) + * @frequency: slave's internal oscillator frequency in 500KHz steps + * @inaccuracy: slave's internal oscillator inaccuracy in 0.1% steps + */ +struct i3c_ccc_getxtime { + u8 supported_modes; + u8 state; + u8 frequency; + u8 inaccuracy; +} __packed; + +/** + * struct i3c_ccc_cmd_payload - CCC payload + * + * @len: payload length + * @data: payload data + */ +struct i3c_ccc_cmd_payload { + u16 len; + void *data; +}; + +/** + * struct i3c_ccc_cmd_dest - CCC command destination + * + * @addr: can be an I3C device address or the broadcast address if this is a + * broadcast CCC + * @payload: payload to be sent to this device or broadcasted + */ +struct i3c_ccc_cmd_dest { + u8 addr; + struct i3c_ccc_cmd_payload payload; +}; + +/** + * struct i3c_ccc_cmd - CCC command + * + * @rnw: true if the CCC should retrieve data from the device. Only valid for + * unicast commands + * @id: CCC command id + * @dests: array of destinations and associated payload for this CCC. Most of + * the time, only one destination is provided + * @ndests: number of destinations. Should always be one for broadcast commands + */ +struct i3c_ccc_cmd { + bool rnw; + u8 id; + struct i3c_ccc_cmd_dest *dests; + int ndests; +}; + +#endif /* I3C_CCC_H */ diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h new file mode 100644 index 000000000000..411b13ceda9f --- /dev/null +++ b/include/linux/i3c/device.h @@ -0,0 +1,305 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (C) 2017 Cadence Design Systems Inc. + * + * Author: Boris Brezillon <boris.brezillon@bootlin.com> + */ + +#ifndef I3C_DEV_H +#define I3C_DEV_H + +#include <linux/bitops.h> +#include <linux/device.h> +#include <linux/i2c.h> +#include <linux/kconfig.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> + +/** + * enum i3c_hdr_mode - HDR mode ids + * @I3C_HDR_DDR: DDR mode + * @I3C_HDR_TSP: TSP mode + * @I3C_HDR_TSL: TSL mode + */ +enum i3c_hdr_mode { + I3C_HDR_DDR, + I3C_HDR_TSP, + I3C_HDR_TSL, +}; + +/** + * struct i3c_priv_xfer - I3C SDR private transfer + * @rnw: encodes the transfer direction. true for a read, false for a write + * @len: transfer length in bytes of the transfer + * @data: input/output buffer + */ +struct i3c_priv_xfer { + bool rnw; + u16 len; + union { + void *in; + const void *out; + } data; +}; + +/** + * enum i3c_dcr - I3C DCR values + * @I3C_DCR_GENERIC_DEVICE: generic I3C device + */ +enum i3c_dcr { + I3C_DCR_GENERIC_DEVICE = 0, +}; + +#define I3C_PID_MANUF_ID(pid) (((pid) & GENMASK_ULL(47, 33)) >> 33) +#define I3C_PID_RND_LOWER_32BITS(pid) (!!((pid) & BIT_ULL(32))) +#define I3C_PID_RND_VAL(pid) ((pid) & GENMASK_ULL(31, 0)) +#define I3C_PID_PART_ID(pid) (((pid) & GENMASK_ULL(31, 16)) >> 16) +#define I3C_PID_INSTANCE_ID(pid) (((pid) & GENMASK_ULL(15, 12)) >> 12) +#define I3C_PID_EXTRA_INFO(pid) ((pid) & GENMASK_ULL(11, 0)) + +#define I3C_BCR_DEVICE_ROLE(bcr) ((bcr) & GENMASK(7, 6)) +#define I3C_BCR_I3C_SLAVE (0 << 6) +#define I3C_BCR_I3C_MASTER (1 << 6) +#define I3C_BCR_HDR_CAP BIT(5) +#define I3C_BCR_BRIDGE BIT(4) +#define I3C_BCR_OFFLINE_CAP BIT(3) +#define I3C_BCR_IBI_PAYLOAD BIT(2) +#define I3C_BCR_IBI_REQ_CAP BIT(1) +#define I3C_BCR_MAX_DATA_SPEED_LIM BIT(0) + +/** + * struct i3c_device_info - I3C device information + * @pid: Provisional ID + * @bcr: Bus Characteristic Register + * @dcr: Device Characteristic Register + * @static_addr: static/I2C address + * @dyn_addr: dynamic address + * @hdr_cap: supported HDR modes + * @max_read_ds: max read speed information + * @max_write_ds: max write speed information + * @max_ibi_len: max IBI payload length + * @max_read_turnaround: max read turn-around time in micro-seconds + * @max_read_len: max private SDR read length in bytes + * @max_write_len: max private SDR write length in bytes + * + * These are all basic information that should be advertised by an I3C device. + * Some of them are optional depending on the device type and device + * capabilities. + * For each I3C slave attached to a master with + * i3c_master_add_i3c_dev_locked(), the core will send the relevant CCC command + * to retrieve these data. + */ +struct i3c_device_info { + u64 pid; + u8 bcr; + u8 dcr; + u8 static_addr; + u8 dyn_addr; + u8 hdr_cap; + u8 max_read_ds; + u8 max_write_ds; + u8 max_ibi_len; + u32 max_read_turnaround; + u16 max_read_len; + u16 max_write_len; +}; + +/* + * I3C device internals are kept hidden from I3C device users. It's just + * simpler to refactor things when everything goes through getter/setters, and + * I3C device drivers should not have to worry about internal representation + * anyway. + */ +struct i3c_device; + +/* These macros should be used to i3c_device_id entries. */ +#define I3C_MATCH_MANUF_AND_PART (I3C_MATCH_MANUF | I3C_MATCH_PART) + +#define I3C_DEVICE(_manufid, _partid, _drvdata) \ + { \ + .match_flags = I3C_MATCH_MANUF_AND_PART, \ + .manuf_id = _manufid, \ + .part_id = _partid, \ + .data = _drvdata, \ + } + +#define I3C_DEVICE_EXTRA_INFO(_manufid, _partid, _info, _drvdata) \ + { \ + .match_flags = I3C_MATCH_MANUF_AND_PART | \ + I3C_MATCH_EXTRA_INFO, \ + .manuf_id = _manufid, \ + .part_id = _partid, \ + .extra_info = _info, \ + .data = _drvdata, \ + } + +#define I3C_CLASS(_dcr, _drvdata) \ + { \ + .match_flags = I3C_MATCH_DCR, \ + .dcr = _dcr, \ + } + +/** + * struct i3c_driver - I3C device driver + * @driver: inherit from device_driver + * @probe: I3C device probe method + * @remove: I3C device remove method + * @id_table: I3C device match table. Will be used by the framework to decide + * which device to bind to this driver + */ +struct i3c_driver { + struct device_driver driver; + int (*probe)(struct i3c_device *dev); + int (*remove)(struct i3c_device *dev); + const struct i3c_device_id *id_table; +}; + +static inline struct i3c_driver *drv_to_i3cdrv(struct device_driver *drv) +{ + return container_of(drv, struct i3c_driver, driver); +} + +struct device *i3cdev_to_dev(struct i3c_device *i3cdev); +struct i3c_device *dev_to_i3cdev(struct device *dev); + +static inline void i3cdev_set_drvdata(struct i3c_device *i3cdev, + void *data) +{ + struct device *dev = i3cdev_to_dev(i3cdev); + + dev_set_drvdata(dev, data); +} + +static inline void *i3cdev_get_drvdata(struct i3c_device *i3cdev) +{ + struct device *dev = i3cdev_to_dev(i3cdev); + + return dev_get_drvdata(dev); +} + +int i3c_driver_register_with_owner(struct i3c_driver *drv, + struct module *owner); +void i3c_driver_unregister(struct i3c_driver *drv); + +#define i3c_driver_register(__drv) \ + i3c_driver_register_with_owner(__drv, THIS_MODULE) + +/** + * module_i3c_driver() - Register a module providing an I3C driver + * @__drv: the I3C driver to register + * + * Provide generic init/exit functions that simply register/unregister an I3C + * driver. + * Should be used by any driver that does not require extra init/cleanup steps. + */ +#define module_i3c_driver(__drv) \ + module_driver(__drv, i3c_driver_register, i3c_driver_unregister) + +/** + * i3c_i2c_driver_register() - Register an i2c and an i3c driver + * @i3cdrv: the I3C driver to register + * @i2cdrv: the I2C driver to register + * + * This function registers both @i2cdev and @i3cdev, and fails if one of these + * registrations fails. This is mainly useful for devices that support both I2C + * and I3C modes. + * Note that when CONFIG_I3C is not enabled, this function only registers the + * I2C driver. + * + * Return: 0 if both registrations succeeds, a negative error code otherwise. + */ +static inline int i3c_i2c_driver_register(struct i3c_driver *i3cdrv, + struct i2c_driver *i2cdrv) +{ + int ret; + + ret = i2c_add_driver(i2cdrv); + if (ret || !IS_ENABLED(CONFIG_I3C)) + return ret; + + ret = i3c_driver_register(i3cdrv); + if (ret) + i2c_del_driver(i2cdrv); + + return ret; +} + +/** + * i3c_i2c_driver_unregister() - Unregister an i2c and an i3c driver + * @i3cdrv: the I3C driver to register + * @i2cdrv: the I2C driver to register + * + * This function unregisters both @i3cdrv and @i2cdrv. + * Note that when CONFIG_I3C is not enabled, this function only unregisters the + * @i2cdrv. + */ +static inline void i3c_i2c_driver_unregister(struct i3c_driver *i3cdrv, + struct i2c_driver *i2cdrv) +{ + if (IS_ENABLED(CONFIG_I3C)) + i3c_driver_unregister(i3cdrv); + + i2c_del_driver(i2cdrv); +} + +/** + * module_i3c_i2c_driver() - Register a module providing an I3C and an I2C + * driver + * @__i3cdrv: the I3C driver to register + * @__i2cdrv: the I3C driver to register + * + * Provide generic init/exit functions that simply register/unregister an I3C + * and an I2C driver. + * This macro can be used even if CONFIG_I3C is disabled, in this case, only + * the I2C driver will be registered. + * Should be used by any driver that does not require extra init/cleanup steps. + */ +#define module_i3c_i2c_driver(__i3cdrv, __i2cdrv) \ + module_driver(__i3cdrv, \ + i3c_i2c_driver_register, \ + i3c_i2c_driver_unregister) + +int i3c_device_do_priv_xfers(struct i3c_device *dev, + struct i3c_priv_xfer *xfers, + int nxfers); + +void i3c_device_get_info(struct i3c_device *dev, struct i3c_device_info *info); + +struct i3c_ibi_payload { + unsigned int len; + const void *data; +}; + +/** + * struct i3c_ibi_setup - IBI setup object + * @max_payload_len: maximum length of the payload associated to an IBI. If one + * IBI appears to have a payload that is bigger than this + * number, the IBI will be rejected. + * @num_slots: number of pre-allocated IBI slots. This should be chosen so that + * the system never runs out of IBI slots, otherwise you'll lose + * IBIs. + * @handler: IBI handler, every time an IBI is received. This handler is called + * in a workqueue context. It is allowed to sleep and send new + * messages on the bus, though it's recommended to keep the + * processing done there as fast as possible to avoid delaying + * processing of other queued on the same workqueue. + * + * Temporary structure used to pass information to i3c_device_request_ibi(). + * This object can be allocated on the stack since i3c_device_request_ibi() + * copies every bit of information and do not use it after + * i3c_device_request_ibi() has returned. + */ +struct i3c_ibi_setup { + unsigned int max_payload_len; + unsigned int num_slots; + void (*handler)(struct i3c_device *dev, + const struct i3c_ibi_payload *payload); +}; + +int i3c_device_request_ibi(struct i3c_device *dev, + const struct i3c_ibi_setup *setup); +void i3c_device_free_ibi(struct i3c_device *dev); +int i3c_device_enable_ibi(struct i3c_device *dev); +int i3c_device_disable_ibi(struct i3c_device *dev); + +#endif /* I3C_DEV_H */ diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h new file mode 100644 index 000000000000..e1a5ce18780f --- /dev/null +++ b/include/linux/i3c/master.h @@ -0,0 +1,587 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (C) 2017 Cadence Design Systems Inc. + * + * Author: Boris Brezillon <boris.brezillon@bootlin.com> + */ + +#ifndef I3C_MASTER_H +#define I3C_MASTER_H + +#include <asm/bitsperlong.h> + +#include <linux/bitops.h> +#include <linux/i2c.h> +#include <linux/i3c/ccc.h> +#include <linux/i3c/device.h> +#include <linux/rwsem.h> +#include <linux/spinlock.h> +#include <linux/workqueue.h> + +#define I3C_HOT_JOIN_ADDR 0x2 +#define I3C_BROADCAST_ADDR 0x7e +#define I3C_MAX_ADDR GENMASK(6, 0) + +struct i3c_master_controller; +struct i3c_bus; + +/** + * struct i3c_i2c_dev - I3C/I2C common information + * @node: node element used to insert the device into the I2C or I3C device + * list + * @bus: I3C bus this device is connected to + * @master: I3C master that instantiated this device. Will be used to send + * I2C/I3C frames on the bus + * @master_priv: master private data assigned to the device. Can be used to + * add master specific information + * + * This structure is describing common I3C/I2C dev information. + */ +struct i3c_i2c_dev { + struct list_head node; + struct i3c_bus *bus; + struct i3c_master_controller *master; + void *master_priv; +}; + +#define I3C_LVR_I2C_INDEX_MASK GENMASK(7, 5) +#define I3C_LVR_I2C_INDEX(x) ((x) << 5) +#define I3C_LVR_I2C_FM_MODE BIT(4) + +#define I2C_MAX_ADDR GENMASK(9, 0) + +/** + * struct i2c_device - I2C device object + * @common: inherit common I3C/I2C description + * @info: I2C board info used to instantiate the I2C device. If you are + * using DT to describe your hardware, this will be filled for you + * @client: I2C client object created by the I2C framework. This will only + * be valid after i3c_master_register() returns + * @lvr: Legacy Virtual Register value as described in the I3C specification + * + * I2C device object. Note that the real I2C device is represented by + * i2c_device->client, but we need extra information to handle the device when + * it's connected to an I3C bus, hence the &struct i2c_device wrapper. + * + * The I2C framework is not impacted by this new representation. + */ +struct i2c_device { + struct i3c_i2c_dev common; + struct i2c_board_info info; + struct i2c_client *client; + u8 lvr; +}; + +/** + * struct i3c_ibi_slot - I3C IBI (In-Band Interrupt) slot + * @work: work associated to this slot. The IBI handler will be called from + * there + * @dev: the I3C device that has generated this IBI + * @len: length of the payload associated to this IBI + * @data: payload buffer + * + * An IBI slot is an object pre-allocated by the controller and used when an + * IBI comes in. + * Every time an IBI comes in, the I3C master driver should find a free IBI + * slot in its IBI slot pool, retrieve the IBI payload and queue the IBI using + * i3c_master_queue_ibi(). + * + * How IBI slots are allocated is left to the I3C master driver, though, for + * simple kmalloc-based allocation, the generic IBI slot pool can be used. + */ +struct i3c_ibi_slot { + struct work_struct work; + struct i3c_device *dev; + unsigned int len; + void *data; +}; + +/** + * struct i3c_device_ibi_info - IBI information attached to a specific device + * @all_ibis_handled: used to be informed when no more IBIs are waiting to be + * processed. Used by i3c_device_disable_ibi() to wait for + * all IBIs to be dequeued + * @pending_ibis: count the number of pending IBIs. Each pending IBI has its + * work element queued to the controller workqueue + * @max_payload_len: maximum payload length for an IBI coming from this device. + * this value is specified when calling + * i3c_device_request_ibi() and should not change at run + * time. All messages IBIs exceeding this limit should be + * rejected by the master + * @enabled: reflect the IBI status + * @handler: IBI handler specified at i3c_device_request_ibi() call time. This + * handler will be called from the controller workqueue, and as such + * is allowed to sleep (though it is recommended to process the IBI + * as fast as possible to not stall processing of other IBIs queued + * on the same workqueue). + * New I3C messages can be sent from the IBI handler + * + * The &struct_i3c_device_ibi_info object is allocated when + * i3c_device_request_ibi() is called and attached to a specific device. This + * object is here to manage IBIs coming from a specific I3C device. + * + * Note that this structure is the generic view of the IBI management + * infrastructure. I3C master drivers may have their own internal + * representation which they can associate to the device using + * controller-private data. + */ +struct i3c_device_ibi_info { + struct completion all_ibis_handled; + atomic_t pending_ibis; + unsigned int max_payload_len; + bool enabled; + void (*handler)(struct i3c_device *dev, + const struct i3c_ibi_payload *payload); +}; + +/** + * struct i3c_device - I3C device object + * @common: inherit common I3C/I2C description + * @dev: device object to register the I3C dev to the device model + * @regfailed: true if the device has already been registered to the device + * model and this operation failed. The goal it to not try + * register the device everytime a new device is discovered on + * the bus + * @info: I3C device information. Will be automatically filled when you create + * your device with i3c_master_add_i3c_dev_locked() + * @init_dyn_addr: initial dynamic address requested by the FW + * @ibi_lock: lock used to protect the &struct_i3c_device->ibi + * @ibi: IBI info attached to a device. Should be NULL until + * i3c_device_request_ibi() is called + * + * I3C device object. Every I3C devs on the I3C bus are represented, including + * I3C masters. For each of them, we have an instance of &struct i3c_device. + */ +struct i3c_device { + struct i3c_i2c_dev common; + struct device dev; + bool regfailed; + struct i3c_device_info info; + u8 init_dyn_addr; + struct mutex ibi_lock; + struct i3c_device_ibi_info *ibi; +}; + +/* + * The I3C specification says the maximum number of devices connected on the + * bus is 11, but this number depends on external parameters like trace length, + * capacitive load per Device, and the types of Devices present on the Bus. + * I3C master can also have limitations, so this number is just here as a + * reference and should be adjusted on a per-controller/per-board basis. + */ +#define I3C_BUS_MAX_DEVS 11 + +#define I3C_BUS_MAX_I3C_SCL_RATE 12900000 +#define I3C_BUS_TYP_I3C_SCL_RATE 12500000 +#define I3C_BUS_I2C_FM_PLUS_SCL_RATE 1000000 +#define I3C_BUS_I2C_FM_SCL_RATE 400000 +#define I3C_BUS_TLOW_OD_MIN_NS 200 + +/** + * enum i3c_bus_mode - I3C bus mode + * @I3C_BUS_MODE_PURE: only I3C devices are connected to the bus. No limitation + * expected + * @I3C_BUS_MODE_MIXED_FAST: I2C devices with 50ns spike filter are present on + * the bus. The only impact in this mode is that the + * high SCL pulse has to stay below 50ns to trick I2C + * devices when transmitting I3C frames + * @I3C_BUS_MODE_MIXED_SLOW: I2C devices without 50ns spike filter are present + * on the bus + */ +enum i3c_bus_mode { + I3C_BUS_MODE_PURE, + I3C_BUS_MODE_MIXED_FAST, + I3C_BUS_MODE_MIXED_SLOW, +}; + +/** + * enum i3c_addr_slot_status - I3C address slot status + * @I3C_ADDR_SLOT_FREE: address is free + * @I3C_ADDR_SLOT_RSVD: address is reserved + * @I3C_ADDR_SLOT_I2C_DEV: address is assigned to an I2C device + * @I3C_ADDR_SLOT_I3C_DEV: address is assigned to an I3C device + * @I3C_ADDR_SLOT_STATUS_MASK: address slot mask + * + * On an I3C bus, addresses are assigned dynamically, and we need to know which + * addresses are free to use and which ones are already assigned. + * + * Addresses marked as reserved are those reserved by the I3C protocol + * (broadcast address, ...). + */ +enum i3c_addr_slot_status { + I3C_ADDR_SLOT_FREE, + I3C_ADDR_SLOT_RSVD, + I3C_ADDR_SLOT_I2C_DEV, + I3C_ADDR_SLOT_I3C_DEV, + I3C_ADDR_SLOT_STATUS_MASK = 3, +}; + +/** + * struct i3c_bus - I3C bus object + * @dev: device to be registered to the device-model + * @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 + * know whether it needs to request bus ownership before sending + * a frame or not + * @id: bus ID. Assigned by the framework when register the bus + * @addrslots: a bitmap with 2-bits per-slot to encode the address status and + * ease the DAA (Dynamic Address Assignment) procedure (see + * &enum i3c_addr_slot_status) + * @mode: bus mode (see &enum i3c_bus_mode) + * @scl_rate: SCL signal rate for I3C and I2C mode + * @devs: 2 lists containing all I3C/I2C devices connected to the bus + * @lock: read/write lock on the bus. This is needed to protect against + * operations that have an impact on the whole bus and the devices + * connected to it. For example, when asking slaves to drop their + * dynamic address (RSTDAA CCC), we need to make sure no one is trying + * to send I3C frames to these devices. + * Note that this lock does not protect against concurrency between + * devices: several drivers can send different I3C/I2C frames through + * the same master in parallel. This is the responsibility of the + * master to guarantee that frames are actually sent sequentially and + * not interlaced + * + * The I3C bus is represented with its own object and not implicitly described + * by the I3C master to cope with the multi-master functionality, where one bus + * can be shared amongst several masters, each of them requesting bus ownership + * when they need to. + */ +struct i3c_bus { + struct device dev; + struct i3c_device *cur_master; + int id; + unsigned long addrslots[((I2C_MAX_ADDR + 1) * 2) / BITS_PER_LONG]; + enum i3c_bus_mode mode; + struct { + unsigned long i3c; + unsigned long i2c; + } scl_rate; + struct { + struct list_head i3c; + struct list_head i2c; + } devs; + struct rw_semaphore lock; +}; + +struct i3c_master_controller; + +/** + * struct i3c_master_controller_ops - I3C master methods + * @bus_init: hook responsible for the I3C bus initialization. You should at + * least call master_set_info() from there and set the bus mode. + * You can also put controller specific initialization in there. + * This method is mandatory. + * @bus_cleanup: cleanup everything done in + * &i3c_master_controller_ops->bus_init(). + * This method is optional. + * @attach_i3c_dev: called every time an I3C device is attached to the bus. It + * can be after a DAA or when a device is statically declared + * by the FW, in which case it will only have a static address + * and the dynamic address will be 0. + * This is a good place to attach master controller specific + * data to I3C devices. + * This method is optional. + * @reattach_i3c_dev: called every time an I3C device has its addressed + * changed. It can be because the device has been powered + * down and has lost its address, or it can happen when a + * device had a static address and has been assigned a + * dynamic address with SETDASA. + * This method is optional. + * @detach_i3c_dev: called when an I3C device is detached from the bus. Usually + * happens when the master device is unregistered. + * This method is optional. + * @do_daa: do a DAA (Dynamic Address Assignment) procedure. This is procedure + * should send an ENTDAA CCC command and then add all devices + * discovered sure the DAA using i3c_master_add_i3c_dev_locked(). + * Add devices added with i3c_master_add_i3c_dev_locked() will then be + * attached or re-attached to the controller. + * This method is mandatory. + * @supports_ccc_cmd: should return true if the CCC command is supported, false + * otherwise. + * This method is optional, if not provided the core assumes + * all CCC commands are supported. + * @send_ccc_cmd: send a CCC command + * This method is mandatory. + * @priv_xfers: do one or several private I3C SDR transfers + * This method is mandatory. + * @attach_i2c_dev: called every time an I2C device is attached to the bus. + * This is a good place to attach master controller specific + * data to I2C devices. + * This method is optional. + * @detach_i2c_dev: called when an I2C device is detached from the bus. Usually + * happens when the master device is unregistered. + * This method is optional. + * @i2c_xfers: do one or several I2C transfers. + * This method is mandatory. + * @i2c_funcs: expose the supported I2C functionalities. + * This method is mandatory. + * @request_ibi: attach an IBI handler to an I3C device. This implies defining + * an IBI handler and the constraints of the IBI (maximum payload + * length and number of pre-allocated slots). + * Some controllers support less IBI-capable devices than regular + * devices, so this method might return -%EBUSY if there's no + * more space for an extra IBI registration + * This method is optional. + * @free_ibi: free an IBI previously requested with ->request_ibi(). The IBI + * should have been disabled with ->disable_irq() prior to that + * This method is mandatory only if ->request_ibi is not NULL. + * @enable_ibi: enable the IBI. Only valid if ->request_ibi() has been called + * prior to ->enable_ibi(). The controller should first enable + * the IBI on the controller end (for example, unmask the hardware + * IRQ) and then send the ENEC CCC command (with the IBI flag set) + * to the I3C device. + * This method is mandatory only if ->request_ibi is not NULL. + * @disable_ibi: disable an IBI. First send the DISEC CCC command with the IBI + * flag set and then deactivate the hardware IRQ on the + * controller end. + * This method is mandatory only if ->request_ibi is not NULL. + * @recycle_ibi_slot: recycle an IBI slot. Called every time an IBI has been + * processed by its handler. The IBI slot should be put back + * in the IBI slot pool so that the controller can re-use it + * for a future IBI + * This method is mandatory only if ->request_ibi is not + * NULL. + */ +struct i3c_master_controller_ops { + int (*bus_init)(struct i3c_master_controller *master); + void (*bus_cleanup)(struct i3c_master_controller *master); + int (*attach_i3c_dev)(struct i3c_device *dev); + void (*reattach_i3c_dev)(struct i3c_device *dev, u8 old_dyn_addr); + void (*detach_i3c_dev)(struct i3c_device *dev); + int (*do_daa)(struct i3c_master_controller *master); + bool (*supports_ccc_cmd)(struct i3c_master_controller *master, + const struct i3c_ccc_cmd *cmd); + int (*send_ccc_cmd)(struct i3c_master_controller *master, + struct i3c_ccc_cmd *cmd); + int (*priv_xfers)(struct i3c_device *dev, + const struct i3c_priv_xfer *xfers, + int nxfers); + int (*attach_i2c_dev)(struct i2c_device *dev); + void (*detach_i2c_dev)(struct i2c_device *dev); + int (*i2c_xfers)(struct i2c_device *dev, + const struct i2c_msg *xfers, int nxfers); + u32 (*i2c_funcs)(struct i3c_master_controller *master); + int (*request_ibi)(struct i3c_device *dev, + const struct i3c_ibi_setup *req); + void (*free_ibi)(struct i3c_device *dev); + int (*enable_ibi)(struct i3c_device *dev); + int (*disable_ibi)(struct i3c_device *dev); + void (*recycle_ibi_slot)(struct i3c_device *dev, + struct i3c_ibi_slot *slot); +}; + +/** + * struct i3c_master_controller - I3C master controller object + * @parent: parent device that instantiated this master + * @this: an I3C device object representing this master. This device will be + * added to the list of I3C devs available on the bus + * @i2c: I2C adapter used for backward compatibility. This adapter is + * registered to the I2C subsystem to be as transparent as possible to + * existing I2C drivers + * @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 + * @bus: I3C bus object created by this master + * @wq: workqueue used to execute IBI handlers. Can also be used by master + * drivers if they need to postpone operations that need to take place + * 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 + * + * A &struct i3c_master_controller has to be registered to the I3C subsystem + * through i3c_master_register(). None of &struct i3c_master_controller fields + * should be set manually, just pass appropriate values to + * i3c_master_register(). + */ +struct i3c_master_controller { + struct device *parent; + struct i3c_device *this; + struct i2c_adapter i2c; + const struct i3c_master_controller_ops *ops; + bool secondary; + bool init_done; + struct i3c_bus *bus; + struct workqueue_struct *wq; +}; + +/** + * i3c_bus_for_each_i2cdev() - iterate over all I2C devices present on the bus + * @bus: the I3C bus + * @i2cdev: an I2C device updated to point to the current device at each loop + * iteration + * + * Iterate over all I2C devs present on the bus. + */ +#define i3c_bus_for_each_i2cdev(bus, i2cdev) \ + list_for_each_entry(i2cdev, &(bus)->devs.i2c, common.node) + +/** + * i3c_bus_for_each_i3cdev() - iterate over all I3C devices present on the bus + * @bus: the I3C bus + * @i3cdev: an I3C device updated to point to the current device at each loop + * iteration + * + * Iterate over all I3C devs present on the bus. + */ +#define i3c_bus_for_each_i3cdev(bus, i3cdev) \ + list_for_each_entry(i3cdev, &(bus)->devs.i3c, common.node) + +void i3c_bus_maintenance_lock(struct i3c_bus *bus); +void i3c_bus_maintenance_unlock(struct i3c_bus *bus); +void i3c_bus_normaluse_lock(struct i3c_bus *bus); +void i3c_bus_normaluse_unlock(struct i3c_bus *bus); + +int i3c_master_do_i2c_xfers(struct i3c_master_controller *master, + const struct i2c_msg *xfers, + int nxfers); + +int i3c_master_disec_locked(struct i3c_master_controller *master, u8 addr, + u8 evts); +int i3c_master_enec_locked(struct i3c_master_controller *master, u8 addr, + u8 evts); +int i3c_master_entdaa_locked(struct i3c_master_controller *master); +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_i3c_dev_locked(struct i3c_master_controller *master, + u8 addr); +int i3c_master_do_daa(struct i3c_master_controller *master); + +int i3c_master_set_info(struct i3c_master_controller *master, + const struct i3c_device_info *info); + +int i3c_master_register(struct i3c_master_controller *master, + struct device *parent, + const struct i3c_master_controller_ops *ops, + bool secondary); +int i3c_master_unregister(struct i3c_master_controller *master); + +/** + * i3c_device_get_master_data() - get master private data attached to an I3C + * device + * @dev: the I3C dev to attach private data to + * + * Return: the private data previously attached with + * i3c_device_set_master_data() or NULL if no data has been attached + * to the device. + */ +static inline void *i3c_device_get_master_data(const struct i3c_device *dev) +{ + return dev->common.master_priv; +} + +/** + * i3c_device_set_master_data() - attach master private data to an I3C device + * @dev: the I3C dev to attach private data to + * @data: private data + * + * This functions allows a master controller to attach per-device private data + * which can then be retrieved with i3c_device_get_master_data(). + * + * Attaching private data to a device is usually done just after calling + * i3c_master_add_i3c_dev_locked(). + */ +static inline void i3c_device_set_master_data(struct i3c_device *dev, + void *data) +{ + dev->common.master_priv = data; +} + +/** + * i2c_device_get_master_data() - get master private data attached to an I2C + * device + * @dev: the I2C dev to attach private data to + * + * Return: the private data previously attached with + * i2c_device_set_master_data() or NULL if no data has been attached + * to the device. + */ +static inline void *i2c_device_get_master_data(const struct i2c_device *dev) +{ + return dev->common.master_priv; +} + +/** + * i2c_device_set_master_data() - attach master private data to an I2C device + * @dev: the I2C dev to attach private data to + * @data: private data + * + * This functions allows a master controller to attach per-device private data + * which can then be retrieved with i2c_device_get_master_data(). + * + * Attaching private data to a device is usually done during + * &master_controller_ops->bus_init(), by iterating over all I2C devices + * instantiated by the core (using i3c_bus_for_each_i2cdev()). + */ +static inline void i2c_device_set_master_data(struct i2c_device *dev, + void *data) +{ + dev->common.master_priv = data; +} + +/** + * i3c_device_get_master() - get master used to communicate with a device + * @dev: I3C dev + * + * Return: the master controller driving @dev + */ +static inline struct i3c_master_controller * +i3c_device_get_master(struct i3c_device *dev) +{ + return dev->common.master; +} + +/** + * i2c_device_get_master() - get master used to communicate with a device + * @dev: I2C dev + * + * Return: the master controller driving @dev + */ +static inline struct i3c_master_controller * +i2c_device_get_master(struct i2c_device *dev) +{ + return dev->common.master; +} + +/** + * i3c_master_get_bus() - get the bus attached to a master + * @master: master object + * + * Return: the I3C bus @master is connected to + */ +static inline struct i3c_bus * +i3c_master_get_bus(struct i3c_master_controller *master) +{ + return master->bus; +} + +/** + * i3c_device_get_bus() - get the bus attached to a device + * @dev: an I3C device + * + * Return: the I3C bus @dev is connected to + */ +static inline struct i3c_bus *i3c_device_get_bus(struct i3c_device *dev) +{ + return dev->common.bus; +} + +struct i3c_generic_ibi_pool; + +struct i3c_generic_ibi_pool * +i3c_generic_ibi_alloc_pool(struct i3c_device *dev, + const struct i3c_ibi_setup *req); +void i3c_generic_ibi_free_pool(struct i3c_generic_ibi_pool *pool); + +struct i3c_ibi_slot * +i3c_generic_ibi_get_free_slot(struct i3c_generic_ibi_pool *pool); +void i3c_generic_ibi_recycle_slot(struct i3c_generic_ibi_pool *pool, + struct i3c_ibi_slot *slot); + +void i3c_master_queue_ibi(struct i3c_device *dev, struct i3c_ibi_slot *slot); + +struct i3c_ibi_slot *i3c_master_get_free_ibi_slot(struct i3c_device *dev); + +#endif /* I3C_MASTER_H */ diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h index 48fb2b43c35a..ba4a7a263bf5 100644 --- a/include/linux/mod_devicetable.h +++ b/include/linux/mod_devicetable.h @@ -448,6 +448,23 @@ struct pci_epf_device_id { kernel_ulong_t driver_data; }; +/* i3c */ + +#define I3C_MATCH_DCR 0x1 +#define I3C_MATCH_MANUF 0x2 +#define I3C_MATCH_PART 0x4 +#define I3C_MATCH_EXTRA_INFO 0x8 + +struct i3c_device_id { + __u8 match_flags; + __u8 dcr; + __u16 manuf_id; + __u16 part_id; + __u16 extra_info; + + const void *data; +}; + /* spi */ #define SPI_NAME_SIZE 32
Add core infrastructure to support I3C in Linux and document it. This infrastructure is not complete yet and will be extended over time. There are a few design choices that are worth mentioning because they impact the way I3C device drivers can interact with their devices: - all functions used to send I3C/I2C frames must be called in non-atomic context. Mainly done this way to ease implementation, but this is still open to discussion. Please let me know if you think it's worth considering an asynchronous model here - the bus element is a separate object and is not implicitly described by the master (as done in I2C). The reason is that I want to be able to handle multiple master connected to the same bus and visible to Linux. In this situation, we should only have one instance of the device and not one per master, and sharing the bus object would be part of the solution to gracefully handle this case. I'm not sure we will ever need to deal with multiple masters controlling the same bus and exposed under Linux, but separating the bus and master concept is pretty easy, hence the decision to do it like that. The other benefit of separating the bus and master concepts is that master devices appear under the bus directory in sysfs. - I2C backward compatibility has been designed to be transparent to I2C drivers and the I2C subsystem. The I3C master just registers an I2C adapter which creates a new I2C bus. I'd say that, from a representation PoV it's not ideal because what should appear as a single I3C bus exposing I3C and I2C devices here appears as 2 different busses connected to each other through the parenting (the I3C master is the parent of the I2C and I3C busses). On the other hand, I don't see a better solution if we want something that is not invasive. Missing features in this preliminary version: - I3C HDR modes are not supported - no support for multi-master and the associated concepts (mastership handover, support for secondary masters, ...) - I2C devices can only be described using DT because this is the only use case I have. However, the framework can easily be extended with ACPI and board info support - I3C slave framework. This has been completely omitted, but shouldn't have a huge impact on the I3C framework because I3C slaves don't see the whole bus, it's only about handling master requests and generating IBIs. Some of the struct, constant and enum definitions could be shared, but most of the I3C slave framework logic will be different Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> --- Changes in v3: - Fix locking issues - Explicitly include a bunch of headers (reported by Randy Dunlap) - Rename {i2c,i3c}-scl-frequency DT prop into {i2c,i3c}-scl-hz - Do not use BIT() macro in mod_devicetable.h - Fix typos - Fix/enhance some kernel doc headers - Rework the bus initialization code to simplify master drivers - Assign dynamic address with SETDASA if the device has a static address and the DT has a valid assigned-address property - Rework the LVR extraction in DT parsing code - Add code to detect when a device is re-attached to the bus after losing its dynamic address. In this case we know try to re-assign the old address, and most importantly, the I3C device driver sees the same device instance, not a new one - Add an ->i2c_funcs() hook to let the master declare which I2C features it supports - Unexport a few functions - Remove support for HDR mode since we have no real user yet Changes in v2: - Fix a bunch of mistake I made with the device model (pointed by GKH) - Move the documentation out of this commit (pointed by GKH) - only source drivers/i3c/master/Kconfig when CONFIG_I3C is enabled (pointed by GKH) - Add IBI infrastructure - Add helpers to ease support for Hot Join (most of the logic is delegated to I3C controller drivers) - move the doc out of this commit to improve readability - Fix a few bugs in device probing/remove (detected after trying to load/unload modules in various orders) - Add a module_i3c_i2c_driver() macro to ease integration of drivers for devices that support both I3C and I2C mode --- drivers/Kconfig | 2 + drivers/Makefile | 2 +- drivers/i3c/Kconfig | 24 + drivers/i3c/Makefile | 4 + drivers/i3c/core.c | 620 ++++++++++++++ drivers/i3c/device.c | 294 +++++++ drivers/i3c/internals.h | 28 + drivers/i3c/master.c | 1723 +++++++++++++++++++++++++++++++++++++++ drivers/i3c/master/Kconfig | 0 drivers/i3c/master/Makefile | 0 include/linux/i3c/ccc.h | 382 +++++++++ include/linux/i3c/device.h | 305 +++++++ include/linux/i3c/master.h | 587 +++++++++++++ include/linux/mod_devicetable.h | 17 + 14 files changed, 3987 insertions(+), 1 deletion(-) create mode 100644 drivers/i3c/Kconfig create mode 100644 drivers/i3c/Makefile create mode 100644 drivers/i3c/core.c create mode 100644 drivers/i3c/device.c create mode 100644 drivers/i3c/internals.h create mode 100644 drivers/i3c/master.c create mode 100644 drivers/i3c/master/Kconfig create mode 100644 drivers/i3c/master/Makefile create mode 100644 include/linux/i3c/ccc.h create mode 100644 include/linux/i3c/device.h create mode 100644 include/linux/i3c/master.h