| Message ID | 20180330074751.25987-1-boris.brezillon@bootlin.com |
|---|---|
| Headers | show |
| Series | Add the I3C subsystem | expand |
Hi, On Fri, 30 Mar 2018 09:47:41 +0200 Boris Brezillon <boris.brezillon@bootlin.com> wrote: > This patch series is a proposal for a new I3C subsystem. This v4 has been sent almost a month ago and I didn't get any feedback so far apart from Rob's R-b. Greg, is there any chance we can get these patches merged in 4.18? If not, could you tell me what should be addressed/improved/reworked? Thanks, Boris > > 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 if 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 > now, just in case we need it some day. > 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: > - support for HDR modes (has been removed because of lack of real users) > - 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 > > Main changes between v2 and v3 are: > - Reworked the DT bindings as suggested by Rob > - Reworked the bus initialization step as suggested by Vitor > - Added a driver for an I3C GPIO expander > > Main changes between the initial RFC and this v2 are: > - Add a generic infrastructure to support IBIs. It's worth mentioning > that I tried exposing IBIs as a regular IRQs, but after several > attempts and a discussion with Mark Zyngier, it appeared that it was > not really fitting in the Linux IRQ model (the fact that you have > payload attached to IBIs, the fact that most of the time an IBI will > generate a transfer on the bus which has to be done in an atomic > context, ...) > The counterpart of this decision is the latency induced by the > workqueue approach, but since I don't have real use cases, I don't > know if this can be a problem or not. > - Add helpers to support Hot Join > - Add support for IBIs and Hot Join in Cadence I3C master driver > - Address several issues in how I was using the device model > > Note that I2C changes have been sent separately [1] this time. Other > than that, no big changes in this version, I just addressed the > comments I received from Thomas, Peter, Geert and Rob. > > Thanks, > > Boris > > [1]https://patchwork.ozlabs.org/project/linux-i2c/list/?series=35687 > > Boris Brezillon (10): > i3c: Add core I3C infrastructure > docs: driver-api: Add I3C documentation > i3c: Add sysfs ABI spec > dt-bindings: i3c: Document core bindings > dt-bindings: i3c: Add macros to help fill I3C/I2C device's reg > property > MAINTAINERS: Add myself as the I3C subsystem maintainer > i3c: master: Add driver for Cadence IP > dt-bindings: i3c: Document Cadence I3C master bindings > gpio: Add a driver for Cadence I3C GPIO expander > dt-bindings: gpio: Add bindings for Cadence I3C gpio expander > > Documentation/ABI/testing/sysfs-bus-i3c | 95 ++ > .../devicetree/bindings/gpio/gpio-cdns-i3c.txt | 39 + > .../devicetree/bindings/i3c/cdns,i3c-master.txt | 44 + > Documentation/devicetree/bindings/i3c/i3c.txt | 140 ++ > Documentation/driver-api/i3c/conf.py | 10 + > Documentation/driver-api/i3c/device-driver-api.rst | 7 + > Documentation/driver-api/i3c/index.rst | 9 + > Documentation/driver-api/i3c/master-driver-api.rst | 8 + > Documentation/driver-api/i3c/protocol.rst | 201 +++ > Documentation/driver-api/index.rst | 1 + > MAINTAINERS | 9 + > drivers/Kconfig | 2 + > drivers/Makefile | 2 +- > drivers/gpio/Kconfig | 11 + > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-cdns-i3c.c | 380 +++++ > 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 | 5 + > drivers/i3c/master/Makefile | 1 + > drivers/i3c/master/i3c-master-cdns.c | 1650 +++++++++++++++++++ > include/dt-bindings/i3c/i3c.h | 28 + > include/linux/i3c/ccc.h | 382 +++++ > include/linux/i3c/device.h | 305 ++++ > include/linux/i3c/master.h | 587 +++++++ > include/linux/mod_devicetable.h | 17 + > 30 files changed, 6626 insertions(+), 1 deletion(-) > create mode 100644 Documentation/ABI/testing/sysfs-bus-i3c > create mode 100644 Documentation/devicetree/bindings/gpio/gpio-cdns-i3c.txt > create mode 100644 Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt > create mode 100644 Documentation/devicetree/bindings/i3c/i3c.txt > create mode 100644 Documentation/driver-api/i3c/conf.py > create mode 100644 Documentation/driver-api/i3c/device-driver-api.rst > create mode 100644 Documentation/driver-api/i3c/index.rst > create mode 100644 Documentation/driver-api/i3c/master-driver-api.rst > create mode 100644 Documentation/driver-api/i3c/protocol.rst > create mode 100644 drivers/gpio/gpio-cdns-i3c.c > 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 drivers/i3c/master/i3c-master-cdns.c > create mode 100644 include/dt-bindings/i3c/i3c.h > create mode 100644 include/linux/i3c/ccc.h > create mode 100644 include/linux/i3c/device.h > create mode 100644 include/linux/i3c/master.h > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 23, 2018 at 07:38:14PM +0200, Boris Brezillon wrote: > Hi, > > On Fri, 30 Mar 2018 09:47:41 +0200 > Boris Brezillon <boris.brezillon@bootlin.com> wrote: > > > This patch series is a proposal for a new I3C subsystem. > > This v4 has been sent almost a month ago and I didn't get any feedback > so far apart from Rob's R-b. Greg, is there any chance we can get these > patches merged in 4.18? If not, could you tell me what should be > addressed/improved/reworked? I'll look over it later this week, thanks. greg k-h -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 a driver for Cadence I3C GPIO expander. > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> This is pretty much OK, and I don't want to raise the bar even higher for you to get this code into the kernel, so: Acked-by: Linus Walleij <linus.walleij@linaro.org> The following is an observation for future improvement: > +static int cdns_i3c_gpio_read_reg(struct cdns_i3c_gpio *gpioc, u8 reg, > + u8 *val) > +{ > + struct i3c_priv_xfer xfers[] = { > + { > + .len = sizeof(reg), > + .data.out = ®, > + }, > + { > + .rnw = true, > + .len = sizeof(*val), > + .data.in = val, > + }, > + }; > + > + return i3c_device_do_priv_xfers(gpioc->i3cdev, xfers, > + ARRAY_SIZE(xfers)); > +} > + > +static int cdns_i3c_gpio_write_reg(struct cdns_i3c_gpio *gpioc, u8 reg, > + u8 val) > +{ > + struct i3c_priv_xfer xfers[] = { > + { > + .len = sizeof(reg), > + .data.out = ®, > + }, > + { > + .len = sizeof(val), > + .data.out = &val, > + }, > + }; > + > + return i3c_device_do_priv_xfers(gpioc->i3cdev, xfers, > + ARRAY_SIZE(xfers)); > +} This is starting to resemble drivers/base/regmap/regmap-i2c.c Maybe we should very quickly add regmap-i3c.c as this infrastructre has had a great positive effect on may kernel subsystems. > +static int cdns_i3c_gpio_get_direction(struct gpio_chip *g, unsigned offset) > +{ > + struct cdns_i3c_gpio *gpioc = gpioc_to_cdns_gpioc(g); > + > + return gpioc->dir & BIT(offset); I would: return !!(gpioc->dir & BIT(offset)); So you clamp it to bit 0. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 23, 2018 at 07:56:46PM +0200, Greg Kroah-Hartman wrote: > On Mon, Apr 23, 2018 at 07:38:14PM +0200, Boris Brezillon wrote: > > Hi, > > > > On Fri, 30 Mar 2018 09:47:41 +0200 > > Boris Brezillon <boris.brezillon@bootlin.com> wrote: > > > > > This patch series is a proposal for a new I3C subsystem. > > > > This v4 has been sent almost a month ago and I didn't get any feedback > > so far apart from Rob's R-b. Greg, is there any chance we can get these > > patches merged in 4.18? If not, could you tell me what should be > > addressed/improved/reworked? > > I'll look over it later this week, thanks. At first glance, it would be great to have at least one other reviewed-by or signed-off-by on the main code here. I don't want to be the only one to have to review it :) thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 09:47:44AM +0200, Boris Brezillon wrote: > Document sysfs files/directories/symlinks exposed by the I3C subsystem. > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > --- > Changes in v2: > - new patch > --- > Documentation/ABI/testing/sysfs-bus-i3c | 95 +++++++++++++++++++++++++++++++++ > 1 file changed, 95 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-bus-i3c > > diff --git a/Documentation/ABI/testing/sysfs-bus-i3c b/Documentation/ABI/testing/sysfs-bus-i3c > new file mode 100644 > index 000000000000..5e88cc093e0e > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-bus-i3c > @@ -0,0 +1,95 @@ > +What: /sys/bus/i3c/devices/i3c-<bus-id> > +KernelVersion: 4.16 Wrong kernel versions :) -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Greg, On Sun, 29 Apr 2018 15:37:00 +0200 Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Fri, Mar 30, 2018 at 09:47:44AM +0200, Boris Brezillon wrote: > > Document sysfs files/directories/symlinks exposed by the I3C subsystem. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > --- > > Changes in v2: > > - new patch > > --- > > Documentation/ABI/testing/sysfs-bus-i3c | 95 +++++++++++++++++++++++++++++++++ > > 1 file changed, 95 insertions(+) > > create mode 100644 Documentation/ABI/testing/sysfs-bus-i3c > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-i3c b/Documentation/ABI/testing/sysfs-bus-i3c > > new file mode 100644 > > index 000000000000..5e88cc093e0e > > --- /dev/null > > +++ b/Documentation/ABI/testing/sysfs-bus-i3c > > @@ -0,0 +1,95 @@ > > +What: /sys/bus/i3c/devices/i3c-<bus-id> > > +KernelVersion: 4.16 > > Wrong kernel versions :) I'll fix that. Thanks, Boris -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Greg, On Sun, 29 Apr 2018 15:36:42 +0200 Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Mon, Apr 23, 2018 at 07:56:46PM +0200, Greg Kroah-Hartman wrote: > > On Mon, Apr 23, 2018 at 07:38:14PM +0200, Boris Brezillon wrote: > > > Hi, > > > > > > On Fri, 30 Mar 2018 09:47:41 +0200 > > > Boris Brezillon <boris.brezillon@bootlin.com> wrote: > > > > > > > This patch series is a proposal for a new I3C subsystem. > > > > > > This v4 has been sent almost a month ago and I didn't get any feedback > > > so far apart from Rob's R-b. Greg, is there any chance we can get these > > > patches merged in 4.18? If not, could you tell me what should be > > > addressed/improved/reworked? > > > > I'll look over it later this week, thanks. > > At first glance, it would be great to have at least one other > reviewed-by or signed-off-by on the main code here. I don't want to be > the only one to have to review it :) I understand. Arnd, Wolfram, any chance you could spend some time reviewing this patch series? I know Arnd is in vacation for the next few weeks, so I don't expect him to be able to do that immediately. Also, Wolfram stated that he didn't have time to review the series in details when I submitted the v1, I don't know if things have changed since then. Anyway, I'll keep searching for people to review the code. Regards, Boris -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Greg, On Sun, Apr 29, 2018 at 3:37 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Fri, Mar 30, 2018 at 09:47:44AM +0200, Boris Brezillon wrote: >> Document sysfs files/directories/symlinks exposed by the I3C subsystem. >> >> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> >> --- >> Changes in v2: >> - new patch >> --- >> Documentation/ABI/testing/sysfs-bus-i3c | 95 +++++++++++++++++++++++++++++++++ >> 1 file changed, 95 insertions(+) >> create mode 100644 Documentation/ABI/testing/sysfs-bus-i3c >> >> diff --git a/Documentation/ABI/testing/sysfs-bus-i3c b/Documentation/ABI/testing/sysfs-bus-i3c >> new file mode 100644 >> index 000000000000..5e88cc093e0e >> --- /dev/null >> +++ b/Documentation/ABI/testing/sysfs-bus-i3c >> @@ -0,0 +1,95 @@ >> +What: /sys/bus/i3c/devices/i3c-<bus-id> >> +KernelVersion: 4.16 > > Wrong kernel versions :) Do you update these when backporting to stable? ;-) Gr{oetje,eeting}s, Geert
On Wed, May 02, 2018 at 11:47:49AM +0200, Geert Uytterhoeven wrote: > Hi Greg, > > On Sun, Apr 29, 2018 at 3:37 PM, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > On Fri, Mar 30, 2018 at 09:47:44AM +0200, Boris Brezillon wrote: > >> Document sysfs files/directories/symlinks exposed by the I3C subsystem. > >> > >> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > >> --- > >> Changes in v2: > >> - new patch > >> --- > >> Documentation/ABI/testing/sysfs-bus-i3c | 95 +++++++++++++++++++++++++++++++++ > >> 1 file changed, 95 insertions(+) > >> create mode 100644 Documentation/ABI/testing/sysfs-bus-i3c > >> > >> diff --git a/Documentation/ABI/testing/sysfs-bus-i3c b/Documentation/ABI/testing/sysfs-bus-i3c > >> new file mode 100644 > >> index 000000000000..5e88cc093e0e > >> --- /dev/null > >> +++ b/Documentation/ABI/testing/sysfs-bus-i3c > >> @@ -0,0 +1,95 @@ > >> +What: /sys/bus/i3c/devices/i3c-<bus-id> > >> +KernelVersion: 4.16 > > > > Wrong kernel versions :) > > Do you update these when backporting to stable? ;-) I'm not adding new features/subsystems to stable :) -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Greg, On Wed, May 2, 2018 at 1:10 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Wed, May 02, 2018 at 11:47:49AM +0200, Geert Uytterhoeven wrote: >> On Sun, Apr 29, 2018 at 3:37 PM, Greg Kroah-Hartman >> <gregkh@linuxfoundation.org> wrote: >> > On Fri, Mar 30, 2018 at 09:47:44AM +0200, Boris Brezillon wrote: >> >> Document sysfs files/directories/symlinks exposed by the I3C subsystem. >> >> >> >> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> >> >> --- >> >> Changes in v2: >> >> - new patch >> >> --- >> >> Documentation/ABI/testing/sysfs-bus-i3c | 95 +++++++++++++++++++++++++++++++++ >> >> 1 file changed, 95 insertions(+) >> >> create mode 100644 Documentation/ABI/testing/sysfs-bus-i3c >> >> >> >> diff --git a/Documentation/ABI/testing/sysfs-bus-i3c b/Documentation/ABI/testing/sysfs-bus-i3c >> >> new file mode 100644 >> >> index 000000000000..5e88cc093e0e >> >> --- /dev/null >> >> +++ b/Documentation/ABI/testing/sysfs-bus-i3c >> >> @@ -0,0 +1,95 @@ >> >> +What: /sys/bus/i3c/devices/i3c-<bus-id> >> >> +KernelVersion: 4.16 >> > >> > Wrong kernel versions :) >> >> Do you update these when backporting to stable? ;-) > > I'm not adding new features/subsystems to stable :) True. So let's leave that for the LTSI patch monkey ;-) Gr{oetje,eeting}s, Geert
Hi Boris,
Few things regarding Cadence IP driver:
On 6/4/18, 9:31 AM, "Boris Brezillon" <boris.brezillon@bootlin.com> wrote:
+static void cdns_i3c_master_handle_ibi(struct cdns_i3c_master *master,
+ u32 ibir)
+{
+ struct cdns_i3c_i2c_dev_data *data;
+ bool data_consumed = false;
+ struct i3c_ibi_slot *slot;
+ u32 id = IBIR_SLVID(ibir);
+ struct i3c_device *dev;
+ int len, i, j;
+ u8 *buf;
+
+ /*
+ * FIXME: maybe we should report the FIFO OVF errors to the upper
+ * layer.
+ */
+ if (id >= master->ibi.num_slots || (ibir & IBIR_ERROR))
+ goto out;
+
+ dev = master->ibi.slots[id];
+ spin_lock(&master->ibi.lock);
+
+ data = i3c_device_get_master_data(dev);
+ slot = i3c_generic_ibi_get_free_slot(data->ibi_pool);
+ if (!slot)
+ goto out_unlock;
+
+ buf = slot->data;
+
+ len = IBIR_XFER_BYTES(ibir);
+ for (i = 0; i < IBIR_XFER_BYTES(ibir); i += 4) {
+ u32 tmp = readl(master->regs + IBI_DATA_FIFO);
+
+ for (j = 0; j < 4 && i + j < dev->ibi->max_payload_len; j++)
+ buf[i + j] = tmp >> (j * 8);
+
+ }
+ slot->len = min_t(unsigned int, IBIR_XFER_BYTES(ibir),
+ dev->ibi->max_payload_len);
+ i3c_master_queue_ibi(dev, slot);
+ data_consumed = true;
+
+out_unlock:
+ spin_unlock(&master->ibi.lock);
+
+out:
+ /* Consume data from the FIFO if it's not been done already. */
+ if (!data_consumed) {
+ for (i = 0; i < IBIR_XFER_BYTES(ibir); i += 4)
+ readl(master->regs + IBI_DATA_FIFO);
+ }
+}
len variable is unneeded.
+static int cdns_i3c_master_probe(struct platform_device *pdev)
+{
+ struct cdns_i3c_master *master;
+ struct resource *res;
+ int ret, irq;
+ u32 val;
+
+ master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL);
+ if (!master)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ master->regs = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(master->regs))
+ return PTR_ERR(master->regs);
+
+ master->pclk = devm_clk_get(&pdev->dev, "pclk");
+ if (IS_ERR(master->pclk))
+ return PTR_ERR(master->pclk);
+
+ master->sysclk = devm_clk_get(&pdev->dev, "sysclk");
+ if (IS_ERR(master->pclk))
+ return PTR_ERR(master->pclk);
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0)
+ return irq;
+
+ ret = clk_prepare_enable(master->pclk);
+ if (ret)
+ return ret;
+
+ ret = clk_prepare_enable(master->sysclk);
+ if (ret)
+ goto err_disable_pclk;
+
+ if (readl(master->regs + DEV_ID) != DEV_ID_I3C_MASTER) {
+ ret = -EINVAL;
+ goto err_disable_sysclk;
+ }
+
+ spin_lock_init(&master->xferqueue.lock);
+ INIT_LIST_HEAD(&master->xferqueue.list);
+
+ INIT_WORK(&master->hj_work, cdns_i3c_master_hj);
+ writel(0xffffffff, master->regs + MST_IDR);
+ writel(0xffffffff, master->regs + SLV_IDR);
+ ret = devm_request_irq(&pdev->dev, irq, cdns_i3c_master_interrupt, 0,
+ dev_name(&pdev->dev), master);
+ if (ret)
+ goto err_disable_sysclk;
+
+ platform_set_drvdata(pdev, master);
+
+ val = readl(master->regs + CONF_STATUS0);
+
+ /* Device ID0 is reserved to describe this master. */
+ master->maxdevs = CONF_STATUS0_DEVS_NUM(val);
+ master->free_rr_slots = GENMASK(master->maxdevs, 1);
+
+ val = readl(master->regs + CONF_STATUS1);
+ master->caps.cmdfifodepth = CONF_STATUS1_CMD_DEPTH(val);
+ master->caps.rxfifodepth = CONF_STATUS1_RX_DEPTH(val);
+ master->caps.txfifodepth = CONF_STATUS1_TX_DEPTH(val);
+ master->caps.ibirfifodepth = 16;
IBI fifo depth is hardcoded. You can read this value from CONF_STATUS0 register.
+ master->caps.cmdrfifodepth = 16;
CMDR fifo depth is hardcoded. You can read this value from CONF_STATUS0 register also.
+
+ spin_lock_init(&master->ibi.lock);
+ master->ibi.num_slots = CONF_STATUS1_IBI_HW_RES(val);
+ master->ibi.slots = devm_kzalloc(&pdev->dev,
+ sizeof(*master->ibi.slots) *
+ master->ibi.num_slots,
+ GFP_KERNEL);
+ if (!master->ibi.slots)
+ goto err_disable_sysclk;
+
+ writel(IBIR_THR(1), master->regs + CMD_IBI_THR_CTRL);
+ writel(MST_INT_IBIR_THR, master->regs + MST_IER);
+ writel(DEVS_CTRL_DEV_CLR_ALL, master->regs + DEVS_CTRL);
+
+ ret = i3c_master_register(&master->base, &pdev->dev,
+ &cdns_i3c_master_ops, false);
+ if (ret)
+ goto err_disable_sysclk;
+
+ return 0;
+
+err_disable_sysclk:
+ clk_disable_unprepare(master->sysclk);
+
+err_disable_pclk:
+ clk_disable_unprepare(master->pclk);
+
+ return ret;
+}
Regards,
Przemyslaw Gaj
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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Przemek, On Mon, 4 Jun 2018 09:24:51 +0000 Przemyslaw Gaj <pgaj@cadence.com> wrote: > Hi Boris, > > Few things regarding Cadence IP driver: > > On 6/4/18, 9:31 AM, "Boris Brezillon" <boris.brezillon@bootlin.com> wrote: > > +static void cdns_i3c_master_handle_ibi(struct cdns_i3c_master *master, > + u32 ibir) > +{ > + struct cdns_i3c_i2c_dev_data *data; > + bool data_consumed = false; > + struct i3c_ibi_slot *slot; > + u32 id = IBIR_SLVID(ibir); > + struct i3c_device *dev; > + int len, i, j; > + u8 *buf; > + > + /* > + * FIXME: maybe we should report the FIFO OVF errors to the upper > + * layer. > + */ > + if (id >= master->ibi.num_slots || (ibir & IBIR_ERROR)) > + goto out; > + > + dev = master->ibi.slots[id]; > + spin_lock(&master->ibi.lock); > + > + data = i3c_device_get_master_data(dev); > + slot = i3c_generic_ibi_get_free_slot(data->ibi_pool); > + if (!slot) > + goto out_unlock; > + > + buf = slot->data; > + > + len = IBIR_XFER_BYTES(ibir); > + for (i = 0; i < IBIR_XFER_BYTES(ibir); i += 4) { > + u32 tmp = readl(master->regs + IBI_DATA_FIFO); > + > + for (j = 0; j < 4 && i + j < dev->ibi->max_payload_len; j++) > + buf[i + j] = tmp >> (j * 8); > + > + } > + slot->len = min_t(unsigned int, IBIR_XFER_BYTES(ibir), > + dev->ibi->max_payload_len); > + i3c_master_queue_ibi(dev, slot); > + data_consumed = true; > + > +out_unlock: > + spin_unlock(&master->ibi.lock); > + > +out: > + /* Consume data from the FIFO if it's not been done already. */ > + if (!data_consumed) { > + for (i = 0; i < IBIR_XFER_BYTES(ibir); i += 4) > + readl(master->regs + IBI_DATA_FIFO); > + } > +} > > len variable is unneeded. Will get rid of len. > + > + /* Device ID0 is reserved to describe this master. */ > + master->maxdevs = CONF_STATUS0_DEVS_NUM(val); > + master->free_rr_slots = GENMASK(master->maxdevs, 1); > + > + val = readl(master->regs + CONF_STATUS1); > + master->caps.cmdfifodepth = CONF_STATUS1_CMD_DEPTH(val); > + master->caps.rxfifodepth = CONF_STATUS1_RX_DEPTH(val); > + master->caps.txfifodepth = CONF_STATUS1_TX_DEPTH(val); > + master->caps.ibirfifodepth = 16; > > IBI fifo depth is hardcoded. You can read this value from CONF_STATUS0 register. > > + master->caps.cmdrfifodepth = 16; > > CMDR fifo depth is hardcoded. You can read this value from CONF_STATUS0 register also. Sure, I'll use the FIFO depth exposed in CONF_STATUS0. Thanks, Boris -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 devicetree" 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 devicetree" 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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, I realize I never replied to this review. On Thu, 26 Apr 2018 10:44:26 +0200 Linus Walleij <linus.walleij@linaro.org> wrote: > On Fri, Mar 30, 2018 at 9:47 AM, Boris Brezillon > <boris.brezillon@bootlin.com> wrote: > > > Add a driver for Cadence I3C GPIO expander. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > This is pretty much OK, and I don't want to raise the bar > even higher for you to get this code into the kernel, so: > Acked-by: Linus Walleij <linus.walleij@linaro.org> Thanks. > > The following is an observation for future improvement: > > > +static int cdns_i3c_gpio_read_reg(struct cdns_i3c_gpio *gpioc, u8 reg, > > + u8 *val) > > +{ > > + struct i3c_priv_xfer xfers[] = { > > + { > > + .len = sizeof(reg), > > + .data.out = ®, > > + }, > > + { > > + .rnw = true, > > + .len = sizeof(*val), > > + .data.in = val, > > + }, > > + }; > > + > > + return i3c_device_do_priv_xfers(gpioc->i3cdev, xfers, > > + ARRAY_SIZE(xfers)); > > +} > > + > > +static int cdns_i3c_gpio_write_reg(struct cdns_i3c_gpio *gpioc, u8 reg, > > + u8 val) > > +{ > > + struct i3c_priv_xfer xfers[] = { > > + { > > + .len = sizeof(reg), > > + .data.out = ®, > > + }, > > + { > > + .len = sizeof(val), > > + .data.out = &val, > > + }, > > + }; > > + > > + return i3c_device_do_priv_xfers(gpioc->i3cdev, xfers, > > + ARRAY_SIZE(xfers)); > > +} > > This is starting to resemble > drivers/base/regmap/regmap-i2c.c > > Maybe we should very quickly add regmap-i3c.c as this > infrastructre has had a great positive effect on may kernel > subsystems. Yes I considered that too, I was just waiting for at least one other user before adding this regmap-i3c implementation. > > > +static int cdns_i3c_gpio_get_direction(struct gpio_chip *g, unsigned offset) > > +{ > > + struct cdns_i3c_gpio *gpioc = gpioc_to_cdns_gpioc(g); > > + > > + return gpioc->dir & BIT(offset); > > I would: > > return !!(gpioc->dir & BIT(offset)); > > So you clamp it to bit 0. Will fix that in my v5. Thanks for your review. Boris -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 devicetree" 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 devicetree" 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 devicetree" 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 devicetree" 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 devicetree" 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 devicetree" 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 devicetree" 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 devicetree" 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 devicetree" 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 devicetree" 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 devicetree" 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 devicetree" 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 devicetree" 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 devicetree" 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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
This patch series is a proposal for a new I3C subsystem. 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 if 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 now, just in case we need it some day. 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: - support for HDR modes (has been removed because of lack of real users) - 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 Main changes between v2 and v3 are: - Reworked the DT bindings as suggested by Rob - Reworked the bus initialization step as suggested by Vitor - Added a driver for an I3C GPIO expander Main changes between the initial RFC and this v2 are: - Add a generic infrastructure to support IBIs. It's worth mentioning that I tried exposing IBIs as a regular IRQs, but after several attempts and a discussion with Mark Zyngier, it appeared that it was not really fitting in the Linux IRQ model (the fact that you have payload attached to IBIs, the fact that most of the time an IBI will generate a transfer on the bus which has to be done in an atomic context, ...) The counterpart of this decision is the latency induced by the workqueue approach, but since I don't have real use cases, I don't know if this can be a problem or not. - Add helpers to support Hot Join - Add support for IBIs and Hot Join in Cadence I3C master driver - Address several issues in how I was using the device model Note that I2C changes have been sent separately [1] this time. Other than that, no big changes in this version, I just addressed the comments I received from Thomas, Peter, Geert and Rob. Thanks, Boris [1]https://patchwork.ozlabs.org/project/linux-i2c/list/?series=35687 Boris Brezillon (10): i3c: Add core I3C infrastructure docs: driver-api: Add I3C documentation i3c: Add sysfs ABI spec dt-bindings: i3c: Document core bindings dt-bindings: i3c: Add macros to help fill I3C/I2C device's reg property MAINTAINERS: Add myself as the I3C subsystem maintainer i3c: master: Add driver for Cadence IP dt-bindings: i3c: Document Cadence I3C master bindings gpio: Add a driver for Cadence I3C GPIO expander dt-bindings: gpio: Add bindings for Cadence I3C gpio expander Documentation/ABI/testing/sysfs-bus-i3c | 95 ++ .../devicetree/bindings/gpio/gpio-cdns-i3c.txt | 39 + .../devicetree/bindings/i3c/cdns,i3c-master.txt | 44 + Documentation/devicetree/bindings/i3c/i3c.txt | 140 ++ Documentation/driver-api/i3c/conf.py | 10 + Documentation/driver-api/i3c/device-driver-api.rst | 7 + Documentation/driver-api/i3c/index.rst | 9 + Documentation/driver-api/i3c/master-driver-api.rst | 8 + Documentation/driver-api/i3c/protocol.rst | 201 +++ Documentation/driver-api/index.rst | 1 + MAINTAINERS | 9 + drivers/Kconfig | 2 + drivers/Makefile | 2 +- drivers/gpio/Kconfig | 11 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-cdns-i3c.c | 380 +++++ 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 | 5 + drivers/i3c/master/Makefile | 1 + drivers/i3c/master/i3c-master-cdns.c | 1650 +++++++++++++++++++ include/dt-bindings/i3c/i3c.h | 28 + include/linux/i3c/ccc.h | 382 +++++ include/linux/i3c/device.h | 305 ++++ include/linux/i3c/master.h | 587 +++++++ include/linux/mod_devicetable.h | 17 + 30 files changed, 6626 insertions(+), 1 deletion(-) create mode 100644 Documentation/ABI/testing/sysfs-bus-i3c create mode 100644 Documentation/devicetree/bindings/gpio/gpio-cdns-i3c.txt create mode 100644 Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt create mode 100644 Documentation/devicetree/bindings/i3c/i3c.txt create mode 100644 Documentation/driver-api/i3c/conf.py create mode 100644 Documentation/driver-api/i3c/device-driver-api.rst create mode 100644 Documentation/driver-api/i3c/index.rst create mode 100644 Documentation/driver-api/i3c/master-driver-api.rst create mode 100644 Documentation/driver-api/i3c/protocol.rst create mode 100644 drivers/gpio/gpio-cdns-i3c.c 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 drivers/i3c/master/i3c-master-cdns.c create mode 100644 include/dt-bindings/i3c/i3c.h create mode 100644 include/linux/i3c/ccc.h create mode 100644 include/linux/i3c/device.h create mode 100644 include/linux/i3c/master.h