| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <q2tpu6nf2b336xumasxeqgfgfspsoy7t2x2hfeqtlqrjae2hov@5t6igf4fvzg6>
Date: Thu, 19 Jun 2025 12:32:33 +0200
From: Jorge Marques <gastmaier@...il.com>
To: Frank Li <Frank.li@....com>
Cc: Jorge Marques <jorge.marques@...log.com>,
Alexandre Belloni <alexandre.belloni@...tlin.com>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
linux-i3c@...ts.infradead.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] i3c: master: Add driver for Analog Devices I3C
Controller IP
On Wed, Jun 18, 2025 at 10:18:48PM -0400, Frank Li wrote:
> On Thu, Jun 19, 2025 at 12:17:26AM +0200, Jorge Marques wrote:
> > On Wed, Jun 18, 2025 at 03:40:58PM -0400, Frank Li wrote:
> > > On Wed, Jun 18, 2025 at 09:16:44AM +0200, Jorge Marques wrote:
> > > > Add support for Analog Devices I3C Controller IP, an AXI-interfaced IP
> > > > core that supports I3C and I2C devices, multiple speed-grades and
> > > > I3C IBIs.
> > > >
> > > > Signed-off-by: Jorge Marques <jorge.marques@...log.com>
> > > > ---
> > > > MAINTAINERS | 1 +
> > > > drivers/i3c/master/Kconfig | 11 +
> > > > drivers/i3c/master/Makefile | 1 +
> > > > drivers/i3c/master/adi-i3c-master.c | 1026 +++++++++++++++++++++++++++++++++++
> > > > 4 files changed, 1039 insertions(+)
> > > >
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 6f56e17dcecf902c6812827c1ec3e067c65e9894..9eb5b6c327590725d1641fd4b73e48fc1d1a3954 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -11247,6 +11247,7 @@ I3C DRIVER FOR ANALOG DEVICES I3C CONTROLLER IP
> > > > M: Jorge Marques <jorge.marques@...log.com>
> > > > S: Maintained
> > > > F: Documentation/devicetree/bindings/i3c/adi,i3c-master.yaml
> > > > +F: drivers/i3c/master/adi-i3c-master.c
> > > >
> > > > I3C DRIVER FOR CADENCE I3C MASTER IP
> > > > M: Przemysław Gaj <pgaj@...ence.com>
> > > > diff --git a/drivers/i3c/master/Kconfig b/drivers/i3c/master/Kconfig
> > > > index 7b30db3253af9d5c6aee6544c060e491bfbeb643..328b7145cdefa20e708ebfa3383e849ce51c5a71 100644
> > > > --- a/drivers/i3c/master/Kconfig
> > > > +++ b/drivers/i3c/master/Kconfig
> > > > @@ -1,4 +1,15 @@
> > > > # SPDX-License-Identifier: GPL-2.0-only
> > > > +config ADI_I3C_MASTER
> > > > + tristate "Analog Devices I3C master driver"
> > > > + depends on HAS_IOMEM
> > > > + help
> > > > + Support for Analog Devices I3C Controller IP, an AXI-interfaced IP
> > > > + core that supports I3C and I2C devices, multiple speed-grades and
> > > > + I3C IBIs.
> > > > +
> > > > + This driver can also be built as a module. If so, the module
> > > > + will be called adi-i3c-master.
> > > > +
> > > > config CDNS_I3C_MASTER
> > > > tristate "Cadence I3C master driver"
> > > > depends on HAS_IOMEM
> > > > diff --git a/drivers/i3c/master/Makefile b/drivers/i3c/master/Makefile
> > > > index 3e97960160bc85e5eaf2966ec0c3fae458c2711e..6cc4f4b73e7bdc206b68c750390f9c3cc2ccb199 100644
> > > > --- a/drivers/i3c/master/Makefile
> > > > +++ b/drivers/i3c/master/Makefile
> > > > @@ -1,4 +1,5 @@
> > > > # SPDX-License-Identifier: GPL-2.0-only
> > > > +obj-$(CONFIG_ADI_I3C_MASTER) += adi-i3c-master.o
> > > > obj-$(CONFIG_CDNS_I3C_MASTER) += i3c-master-cdns.o
> > > > obj-$(CONFIG_DW_I3C_MASTER) += dw-i3c-master.o
> > > > obj-$(CONFIG_AST2600_I3C_MASTER) += ast2600-i3c-master.o
> > > > diff --git a/drivers/i3c/master/adi-i3c-master.c b/drivers/i3c/master/adi-i3c-master.c
> > > > new file mode 100644
> > > > index 0000000000000000000000000000000000000000..d4e0756efec9588383ac55ca9ea9228368ce767d
> > > > --- /dev/null
> > > > +++ b/drivers/i3c/master/adi-i3c-master.c
> > > > @@ -0,0 +1,1026 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +/*
> > > > + * I3C Controller driver
> > > > + * Copyright 2025 Analog Devices Inc.
> > > > + * Author: Jorge Marques <jorge.marques@...log.com>
> > > > + */
> > > > +
> > > > +#include <linux/bitops.h>
> > > > +#include <linux/bitfield.h>
> > > > +#include <linux/clk.h>
> > > > +#include <linux/err.h>
> > > > +#include <linux/errno.h>
> > > > +#include <linux/i3c/master.h>
> > > > +#include <linux/interrupt.h>
> > > > +#include <linux/io.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/platform_device.h>
> > > > +
> > > > +#define VERSION_MAJOR(x) ((u32)FIELD_GET(GENMASK(23, 16), (x)))
> > > > +#define VERSION_MINOR(x) ((u32)FIELD_GET(GENMASK(15, 8), (x)))
> > > > +#define VERSION_PATCH(x) ((u32)FIELD_GET(GENMASK(7, 0), (x)))
> > Hi Frank,
> > >
> > > I suppose that needn't (u32), any warnings report without u32?
> > >
> > Here I will actually remove and use the macros from
> > linux/fpga/adi-axi-common.h
> >
> > > > +
> > > > +#define MAX_DEVS 16
> > > > +
> > > > +#define REG_VERSION 0x000
> > to be replaced removed, and ADI_AXI_REG_VERSION used instead.
> > > > +#define REG_ENABLE 0x040
> > > > +#define REG_IRQ_MASK 0x080
> > > > +#define REG_IRQ_PENDING 0x084
> > > > +#define REG_IRQ_PENDING_CMDR BIT(5)
> > > > +#define REG_IRQ_PENDING_IBI ((u32)BIT(6))
> > >
> > > needn't u32
> > In earlier CI runs, I used to get with gcc_x86_64, due to the inversion
> > ~REG_IRQ_PENDING_IBI:
> >
> > > conversion from 'long unsigned int' to 'unsigned int' changes value
> > > from '18446744073709551615' to '4294967295' [-Werror=overflow]
> >
> > Couldn't replicate in this patch version, so I will remove.
> >
> > > > +#define REG_IRQ_PENDING_DAA BIT(7)
> > >
> > > Add empty line between registers
> > >
> > Ok
> > > > +#define REG_CMD_FIFO 0x0d4
> > > > +#define REG_CMD_FIFO_0_IS_CCC BIT(22)
> > > > +#define REG_CMD_FIFO_0_BCAST BIT(21)
> > > > +#define REG_CMD_FIFO_0_SR BIT(20)
> > > > +#define REG_CMD_FIFO_0_LEN(l) FIELD_PREP(GENMASK(19, 8), (l))
> > > > +#define REG_CMD_FIFO_0_DEV_ADDR(a) FIELD_PREP(GENMASK(7, 1), a)
> > > > +#define REG_CMD_FIFO_0_RNW BIT(0)
> > > > +#define REG_CMD_FIFO_1_CCC(id) FIELD_PREP(GENMASK(7, 0), (id))
> > > > +#define REG_CMD_FIFO_ROOM 0x0c0
> > > > +#define REG_CMDR_FIFO 0x0d8
> > > > +#define REG_CMDR_FIFO_NO_ERROR 0
> > > > +#define REG_CMDR_FIFO_CE0_ERROR 1
> > > > +#define REG_CMDR_FIFO_CE2_ERROR 4
> > > > +#define REG_CMDR_FIFO_NACK_RESP 6
> > > > +#define REG_CMDR_FIFO_UDA_ERROR 8
> > > > +#define REG_CMDR_FIFO_ERROR(x) FIELD_GET(GENMASK(23, 20), (x))
> > > > +#define REG_CMDR_FIFO_XFER_BYTES(x) FIELD_GET(GENMASK(19, 8), (x))
> > >
> > > Sort according bit pos. REG_CMDR_FIFO_XFER_BYTES should before
> > > REG_CMDR_FIFO_ERROR
> > >
> > Ahh true
> > > > +#define REG_SDO_FIFO 0x0dc
> > > > +#define REG_SDO_FIFO_ROOM 0x0c8
> > > > +#define REG_SDI_FIFO 0x0e0
> > > > +#define REG_IBI_FIFO 0x0e4
> > > > +#define REG_FIFO_STATUS 0x0e8
> > > > +#define REG_FIFO_STATUS_CMDR_EMPTY BIT(0)
> > > > +#define REG_FIFO_STATUS_IBI_EMPTY BIT(1)
> > > > +#define REG_OPS 0x100
> > > > +#define REG_OPS_SET_SG(x) FIELD_PREP(GENMASK(6, 5), (x))
> > > > +#define REG_OPS_PP_SG_MASK GENMASK(6, 5)
> > >
> > > REG_OPS_SET_SG(x) FIELD_PREP(REG_OPS_PP_SG_MASK, (x))
> > Ack.
> > >
> > > > +#define REG_IBI_CONFIG 0x140
> > > > +#define REG_IBI_CONFIG_LISTEN BIT(1)
> > > > +#define REG_IBI_CONFIG_ENABLE BIT(0)
> > >
> > > sort it. keep consisent in whole file, from 0->31 or 31->0
> > >
> > Ack, decrementing
> > > > +#define REG_DEV_CHAR 0x180
> > > > +#define REG_DEV_CHAR_IS_I2C BIT(0)
> > > > +#define REG_DEV_CHAR_IS_ATTACHED BIT(1)
> > > > +#define REG_DEV_CHAR_BCR_IBI(x) (((x) & GENMASK(2, 1)) << 1)
> > >
> > > FIELD_PREP()
> > Ok, I will remove the simplification and make it clearer by setting
> >
> > #define REG_DEV_CHAR_BCR_IBI(x) FIELD_PREP(GENMASK(3, 2), (x))
> >
> > Then usage:
> >
> > #include <linux/i3c/device.h>
> > ...
> > bcr_ibi = FIELD_GET(I3C_BCR_IBI_PAYLOAD | I3C_BCR_IBI_REQ_CAP, (i3cdev->info.bcr));
> > ...REG_DEV_CHAR_BCR_IBI(bcr_ibi)...;
> >
> > >
> > > > +#define REG_DEV_CHAR_WEN BIT(8)
> > > > +#define REG_DEV_CHAR_ADDR(x) FIELD_PREP(GENMASK(15, 9), (x))
> > > > +
> > > > +enum speed_grade {PP_SG_UNSET, PP_SG_1MHZ, PP_SG_3MHZ, PP_SG_6MHZ, PP_SG_12MHZ};
> > > > +struct adi_i3c_cmd {
> > > > + u32 cmd0;
> > > > + u32 cmd1;
> > > > + u32 tx_len;
> > > > + const void *tx_buf;
> > > > + u32 rx_len;
> > > > + void *rx_buf;
> > > > + u32 error;
> > > > +};
> > > > +
> > > > +struct adi_i3c_xfer {
> > > > + struct list_head node;
> > > > + struct completion comp;
> > > > + int ret;
> > > > + unsigned int ncmds;
> > > > + unsigned int ncmds_comp;
> > > > + struct adi_i3c_cmd cmds[];
> > > > +};
> > > > +
> > > > +struct adi_i3c_master {
> > > > + struct i3c_master_controller base;
> > > > + u32 free_rr_slots;
> > > > + unsigned int maxdevs;
> > > > + struct {
> > > > + unsigned int num_slots;
> > > > + struct i3c_dev_desc **slots;
> > > > + spinlock_t lock; /* Protect IBI slot access */
> > > > + } ibi;
> > > > + struct {
> > > > + struct list_head list;
> > > > + struct adi_i3c_xfer *cur;
> > > > + spinlock_t lock; /* Protect transfer */
> > > > + } xferqueue;
> > > > + void __iomem *regs;
> > > > + struct clk *clk;
> > > > + unsigned long i3c_scl_lim;
> > > > + struct {
> > > > + u8 addrs[MAX_DEVS];
> > > > + u8 index;
> > > > + } daa;
> > > > +};
> > > > +
> > > > +static inline struct adi_i3c_master *to_adi_i3c_master(struct i3c_master_controller *master)
> > > > +{
> > > > + return container_of(master, struct adi_i3c_master, base);
> > > > +}
> > > > +
> > > > +static void adi_i3c_master_wr_to_tx_fifo(struct adi_i3c_master *master,
> > > > + const u8 *bytes, unsigned int nbytes)
> > > > +{
> > > > + unsigned int n, m;
> > > > + u32 tmp;
> > > > +
> > > > + n = readl(master->regs + REG_SDO_FIFO_ROOM);
> > > > + m = min(n, nbytes);
> > > > + writesl(master->regs + REG_SDO_FIFO, bytes, m / 4);
> > > > +
> > > > + if (m & 3) {
> > > > + memcpy(&tmp, bytes + (m & ~3), m & 3);
> > > > + writel(tmp, master->regs + REG_SDO_FIFO);
> > > > + }
> > > > +}
> > > > +
> > > > +static void adi_i3c_master_rd_from_rx_fifo(struct adi_i3c_master *master,
> > > > + u8 *bytes, unsigned int nbytes)
> > > > +{
> > > > + readsl(master->regs + REG_SDI_FIFO, bytes, nbytes / 4);
> > > > + if (nbytes & 3) {
> > > > + u32 tmp;
> > > > +
> > > > + tmp = readl(master->regs + REG_SDI_FIFO);
> > > > + memcpy(bytes + (nbytes & ~3), &tmp, nbytes & 3);
> > > > + }
> > > > +}
> > >
> > > can you move above two helper function as inline into drivers/i3c/internals.h
> > >
> > > or create new one drivers/i3c/master/helper.h or other naming.
> > >
> > > Renesas IP have similar above two functions to access FIFO.
> > > https://lore.kernel.org/linux-i3c/174964724485.330045.2181706921272138816.robh@kernel.org/T/#t
> > >
> > Ok, yes that's common because the passes type u8 doesn't ensure
> > multiples of 32 at least. Added to my TODO list.
> > > > +
> > > > +static bool adi_i3c_master_supports_ccc_cmd(struct i3c_master_controller *m,
> > > > + const struct i3c_ccc_cmd *cmd)
> > > > +{
> > > > + if (cmd->ndests > 1)
> > > > + return false;
> > > > +
> > > > + switch (cmd->id) {
> > > > + case I3C_CCC_ENEC(true):
> > > > + case I3C_CCC_ENEC(false):
> > > > + case I3C_CCC_DISEC(true):
> > > > + case I3C_CCC_DISEC(false):
> > > > + case I3C_CCC_RSTDAA(true):
> > > > + case I3C_CCC_RSTDAA(false):
> > > > + case I3C_CCC_ENTDAA:
> > > > + case I3C_CCC_SETDASA:
> > > > + case I3C_CCC_SETNEWDA:
> > > > + case I3C_CCC_GETMWL:
> > > > + case I3C_CCC_GETMRL:
> > > > + case I3C_CCC_GETPID:
> > > > + case I3C_CCC_GETBCR:
> > > > + case I3C_CCC_GETDCR:
> > > > + case I3C_CCC_GETSTATUS:
> > > > + case I3C_CCC_GETHDRCAP:
> > > > + return true;
> > > > + default:
> > > > + break;
> > > > + }
> > > > +
> > > > + return false;
> > > > +}
> > > > +
> > > > +static int adi_i3c_master_disable(struct adi_i3c_master *master)
> > > > +{
> > > > + writel(0, master->regs + REG_IBI_CONFIG);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static struct adi_i3c_xfer *adi_i3c_master_alloc_xfer(struct adi_i3c_master *master,
> > > > + unsigned int ncmds)
> > > > +{
> > > > + struct adi_i3c_xfer *xfer;
> > > > +
> > > > + xfer = kzalloc(struct_size(xfer, cmds, ncmds), GFP_KERNEL);
> > > > + if (!xfer)
> > > > + return NULL;
> > > > +
> > > > + INIT_LIST_HEAD(&xfer->node);
> > > > + xfer->ncmds = ncmds;
> > > > + xfer->ret = -ETIMEDOUT;
> > > > +
> > > > + return xfer;
> > > > +}
> > > > +
> > > > +static void adi_i3c_master_start_xfer_locked(struct adi_i3c_master *master)
> > > > +{
> > > > + struct adi_i3c_xfer *xfer = master->xferqueue.cur;
> > > > + unsigned int i, n, m;
> > > > +
> > > > + if (!xfer)
> > > > + return;
> > > > +
> > > > + for (i = 0; i < xfer->ncmds; i++) {
> > > > + struct adi_i3c_cmd *cmd = &xfer->cmds[i];
> > > > +
> > > > + if (!(cmd->cmd0 & REG_CMD_FIFO_0_RNW))
> > > > + adi_i3c_master_wr_to_tx_fifo(master, cmd->tx_buf, cmd->tx_len);
> > > > + }
> > > > +
> > > > + n = readl(master->regs + REG_CMD_FIFO_ROOM);
> > > > + for (i = 0; i < xfer->ncmds; i++) {
> > > > + struct adi_i3c_cmd *cmd = &xfer->cmds[i];
> > > > +
> > > > + m = cmd->cmd0 & REG_CMD_FIFO_0_IS_CCC ? 2 : 1;
> > > > + if (m > n)
> > > > + break;
> > > > + writel(cmd->cmd0, master->regs + REG_CMD_FIFO);
> > > > + if (cmd->cmd0 & REG_CMD_FIFO_0_IS_CCC)
> > > > + writel(cmd->cmd1, master->regs + REG_CMD_FIFO);
> > > > + n -= m;
> > > > + }
> > > > +}
> > > > +
> > > > +static void adi_i3c_master_end_xfer_locked(struct adi_i3c_master *master,
> > > > + u32 pending)
> > > > +{
> > > > + struct adi_i3c_xfer *xfer = master->xferqueue.cur;
> > > > + int i, ret = 0;
> > > > + u32 status0;
> > > > +
> > > > + if (!xfer)
> > > > + return;
> > > > +
> > > > + for (status0 = readl(master->regs + REG_FIFO_STATUS);
> > > > + !(status0 & REG_FIFO_STATUS_CMDR_EMPTY);
> > > > + status0 = readl(master->regs + REG_FIFO_STATUS)) {
> > >
> > > how about
> > >
> > > while (!(readl(master->regs + REG_FIFO_STATUS) & REG_FIFO_STATUS_CMDR_EMPTY))) {
> > >
> > Ack, thanks!
> > Same for 'REG_FIFO_STATUS_IBI_EMPTY for'.
> > > > + struct adi_i3c_cmd *cmd;
> > > > + u32 cmdr, rx_len;
> > > > +
> > > > + cmdr = readl(master->regs + REG_CMDR_FIFO);
> > > > +
> > > > + cmd = &xfer->cmds[xfer->ncmds_comp++];
> > > > + if (cmd->cmd0 & REG_CMD_FIFO_0_RNW) {
> > > > + rx_len = min_t(u32, REG_CMDR_FIFO_XFER_BYTES(cmdr), cmd->rx_len);
> > > > + adi_i3c_master_rd_from_rx_fifo(master, cmd->rx_buf, rx_len);
> > > > + }
> > > > + cmd->error = REG_CMDR_FIFO_ERROR(cmdr);
> > > > + }
> > > > +
> > > > + for (i = 0; i < xfer->ncmds; i++) {
> > >
> > > I think you iterate to ncmds_comp?
> > >
> > You are correct thanks! Each command yields a command receipt interrupt.
> > > > + switch (xfer->cmds[i].error) {
> > > > + case REG_CMDR_FIFO_NO_ERROR:
> > > > + break;
> > > > +
> > > > + case REG_CMDR_FIFO_CE0_ERROR:
> > > > + case REG_CMDR_FIFO_CE2_ERROR:
> > > > + case REG_CMDR_FIFO_NACK_RESP:
> > > > + case REG_CMDR_FIFO_UDA_ERROR:
> > > > + ret = -EIO;
> > > > + break;
> > > > +
> > > > + default:
> > > > + ret = -EINVAL;
> > > > + break;
> > > > + }
> > > > + }
> > > > +
> > > > + xfer->ret = ret;
> > > > +
> > > > + if (xfer->ncmds_comp != xfer->ncmds)
> > > > + return;
> > > > +
> > > > + complete(&xfer->comp);
> > > > +
> > > > + xfer = list_first_entry_or_null(&master->xferqueue.list,
> > > > + struct adi_i3c_xfer, node);
> > > > + if (xfer)
> > > > + list_del_init(&xfer->node);
> > > > +
> > > > + master->xferqueue.cur = xfer;
> > > > + adi_i3c_master_start_xfer_locked(master);
> > > > +}
> > > > +
> > > > +static void adi_i3c_master_queue_xfer(struct adi_i3c_master *master,
> > > > + struct adi_i3c_xfer *xfer)
> > > > +{
> > > > + init_completion(&xfer->comp);
> > > > + guard(spinlock_irqsave)(&master->xferqueue.lock);
> > > > + if (master->xferqueue.cur) {
> > > > + list_add_tail(&xfer->node, &master->xferqueue.list);
> > > > + } else {
> > > > + master->xferqueue.cur = xfer;
> > > > + adi_i3c_master_start_xfer_locked(master);
> > > > + }
> > > > +}
> > > > +
> > > > +static void adi_i3c_master_unqueue_xfer(struct adi_i3c_master *master,
> > > > + struct adi_i3c_xfer *xfer)
> > > > +{
> > > > + guard(spinlock_irqsave)(&master->xferqueue.lock);
> > > > + if (master->xferqueue.cur == xfer)
> > > > + master->xferqueue.cur = NULL;
> > > > + else
> > > > + list_del_init(&xfer->node);
> > > > +
> > > > + writel(0x01, master->regs + REG_ENABLE);
> > > > + writel(0x00, master->regs + REG_ENABLE);
> > > > + writel(REG_IRQ_PENDING_CMDR, master->regs + REG_IRQ_MASK);
> > > > +}
> > > > +
> > > > +static enum i3c_error_code adi_i3c_cmd_get_err(struct adi_i3c_cmd *cmd)
> > > > +{
> > > > + switch (cmd->error) {
> > > > + case REG_CMDR_FIFO_CE0_ERROR:
> > > > + return I3C_ERROR_M0;
> > > > +
> > > > + case REG_CMDR_FIFO_CE2_ERROR:
> > > > + case REG_CMDR_FIFO_NACK_RESP:
> > > > + return I3C_ERROR_M2;
> > > > +
> > > > + default:
> > > > + break;
> > > > + }
> > > > +
> > > > + return I3C_ERROR_UNKNOWN;
> > > > +}
> > > > +
> > > > +static int adi_i3c_master_send_ccc_cmd(struct i3c_master_controller *m,
> > > > + struct i3c_ccc_cmd *cmd)
> > > > +{
> > > > + struct adi_i3c_master *master = to_adi_i3c_master(m);
> > > > + struct adi_i3c_xfer *xfer;
> > > > + struct adi_i3c_cmd *ccmd;
> > > > +
> > > > + xfer = adi_i3c_master_alloc_xfer(master, 1);
> > > > + if (!xfer)
> > > > + return -ENOMEM;
> > > > +
> > > > + ccmd = xfer->cmds;
> > > > + ccmd->cmd1 = REG_CMD_FIFO_1_CCC(cmd->id);
> > > > + ccmd->cmd0 = REG_CMD_FIFO_0_IS_CCC |
> > > > + REG_CMD_FIFO_0_LEN(cmd->dests[0].payload.len);
> > > > +
> > > > + if (cmd->id & I3C_CCC_DIRECT)
> > > > + ccmd->cmd0 |= REG_CMD_FIFO_0_DEV_ADDR(cmd->dests[0].addr);
> > > > +
> > > > + if (cmd->rnw) {
> > > > + ccmd->cmd0 |= REG_CMD_FIFO_0_RNW;
> > > > + ccmd->rx_buf = cmd->dests[0].payload.data;
> > > > + ccmd->rx_len = cmd->dests[0].payload.len;
> > > > + } else {
> > > > + ccmd->tx_buf = cmd->dests[0].payload.data;
> > > > + ccmd->tx_len = cmd->dests[0].payload.len;
> > > > + }
> > > > +
> > > > + adi_i3c_master_queue_xfer(master, xfer);
> > > > + if (!wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000)))
> > > > + adi_i3c_master_unqueue_xfer(master, xfer);
> > > > +
> > > > + cmd->err = adi_i3c_cmd_get_err(&xfer->cmds[0]);
> > > > + kfree(xfer);
> > >
> > > you can use
> > > struct adi_i3c_xfer *xfer __free(kfree) = NULL;
> > >
> > > to simple code.
> > >
> > Ah cool, will use in all xfer instances.
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int adi_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
> > > > + struct i3c_priv_xfer *xfers,
> > > > + int nxfers)
> > > > +{
> > > > + struct i3c_master_controller *m = i3c_dev_get_master(dev);
> > > > + struct adi_i3c_master *master = to_adi_i3c_master(m);
> > > > + struct adi_i3c_xfer *xfer;
> > > > + int i, ret;
> > > > +
> > > > + if (!nxfers)
> > > > + return 0;
> > >
> > > add an empty line.
> > >
> > Ack.
> > > > + xfer = adi_i3c_master_alloc_xfer(master, nxfers);
> > > > + if (!xfer)
> > > > + return -ENOMEM;
> > > > +
> > > > + for (i = 0; i < nxfers; i++) {
> > > > + struct adi_i3c_cmd *ccmd = &xfer->cmds[i];
> > > > +
> > > > + ccmd->cmd0 = REG_CMD_FIFO_0_DEV_ADDR(dev->info.dyn_addr);
> > > > +
> > > > + if (xfers[i].rnw) {
> > > > + ccmd->cmd0 |= REG_CMD_FIFO_0_RNW;
> > > > + ccmd->rx_buf = xfers[i].data.in;
> > > > + ccmd->rx_len = xfers[i].len;
> > > > + } else {
> > > > + ccmd->tx_buf = xfers[i].data.out;
> > > > + ccmd->tx_len = xfers[i].len;
> > > > + }
> > > > +
> > > > + ccmd->cmd0 |= REG_CMD_FIFO_0_LEN(xfers[i].len);
> > > > +
> > > > + if (i < nxfers - 1)
> > > > + ccmd->cmd0 |= REG_CMD_FIFO_0_SR;
> > > > +
> > > > + if (!i)
> > > > + ccmd->cmd0 |= REG_CMD_FIFO_0_BCAST;
> > > > + }
> > > > +
> > > > + adi_i3c_master_queue_xfer(master, xfer);
> > > > + if (!wait_for_completion_timeout(&xfer->comp,
> > > > + msecs_to_jiffies(1000)))
> > > > + adi_i3c_master_unqueue_xfer(master, xfer);
> > > > +
> > > > + ret = xfer->ret;
> > > > +
> > > > + for (i = 0; i < nxfers; i++)
> > > > + xfers[i].err = adi_i3c_cmd_get_err(&xfer->cmds[i]);
> > > > +
> > > > + kfree(xfer);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +struct adi_i3c_i2c_dev_data {
> > > > + u16 id;
> > > > + s16 ibi;
> > > > + struct i3c_generic_ibi_pool *ibi_pool;
> > > > +};
> > > > +
> > > > +static int adi_i3c_master_get_rr_slot(struct adi_i3c_master *master,
> > > > + u8 dyn_addr)
> > > > +{
> > > > + if (!master->free_rr_slots)
> > > > + return -ENOSPC;
> > > > +
> > > > + return ffs(master->free_rr_slots) - 1;
> > > > +}
> > > > +
> > > > +static int adi_i3c_master_reattach_i3c_dev(struct i3c_dev_desc *dev, u8 dyn_addr)
> > > > +{
> > > > + struct i3c_master_controller *m = i3c_dev_get_master(dev);
> > > > + struct adi_i3c_master *master = to_adi_i3c_master(m);
> > > > + u8 addr;
> > > > +
> > > > + addr = dev->info.dyn_addr ? dev->info.dyn_addr : dev->info.static_addr;
> > > > +
> > > > + writel(REG_DEV_CHAR_ADDR(dyn_addr), master->regs + REG_DEV_CHAR);
> > > > + writel((readl(master->regs + REG_DEV_CHAR) &
> > > > + ~REG_DEV_CHAR_IS_ATTACHED) | REG_DEV_CHAR_WEN,
> > > > + master->regs + REG_DEV_CHAR);
> > > > +
> > > > + writel(REG_DEV_CHAR_ADDR(addr), master->regs + REG_DEV_CHAR);
> > > > + writel(readl(master->regs + REG_DEV_CHAR) |
> > > > + REG_DEV_CHAR_IS_ATTACHED | REG_DEV_CHAR_WEN,
> > > > + master->regs + REG_DEV_CHAR);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int adi_i3c_master_attach_i3c_dev(struct i3c_dev_desc *dev)
> > > > +{
> > > > + struct i3c_master_controller *m = i3c_dev_get_master(dev);
> > > > + struct adi_i3c_master *master = to_adi_i3c_master(m);
> > > > + struct adi_i3c_i2c_dev_data *data;
> > > > + int slot;
> > > > + u8 addr;
> > > > +
> > > > + data = kzalloc(sizeof(*data), GFP_KERNEL);
> > > > + if (!data)
> > > > + return -ENOMEM;
> > > > +
> > > > + slot = adi_i3c_master_get_rr_slot(master, dev->info.dyn_addr);
> > > > + if (slot < 0) {
> > > > + kfree(data);
> > > > + return slot;
> > > > + }
> > > > +
> > > > + data->ibi = -1;
> > >
> > > why -1 here?
> > >
> > Doesn't mean much, just stores the ibi slot occupied by the device.
> > NULL would be position 0 of the array.
> > It can be left uninitialized and not cleaned-up when removed.
> > > > + data->id = slot;
> > > > + i3c_dev_set_master_data(dev, data);
> > > > + master->free_rr_slots &= ~BIT(slot);
> > > > +
> > > > + addr = dev->info.dyn_addr ? dev->info.dyn_addr : dev->info.static_addr;
> > > > +
> > > > + writel(REG_DEV_CHAR_ADDR(addr), master->regs + REG_DEV_CHAR);
> > > > + writel(readl(master->regs + REG_DEV_CHAR) |
> > > > + REG_DEV_CHAR_IS_ATTACHED | REG_DEV_CHAR_WEN,
> > > > + master->regs + REG_DEV_CHAR);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void adi_i3c_master_sync_dev_char(struct i3c_master_controller *m)
> > > > +{
> > > > + struct adi_i3c_master *master = to_adi_i3c_master(m);
> > > > + struct i3c_dev_desc *i3cdev;
> > > > + u8 addr;
> > > > +
> > > > + i3c_bus_for_each_i3cdev(&m->bus, i3cdev) {
> > > > + addr = i3cdev->info.dyn_addr ?
> > > > + i3cdev->info.dyn_addr : i3cdev->info.static_addr;
> > > > + writel(REG_DEV_CHAR_ADDR(addr), master->regs + REG_DEV_CHAR);
> > > > + writel(readl(master->regs + REG_DEV_CHAR) |
> > > > + REG_DEV_CHAR_BCR_IBI(i3cdev->info.bcr) | REG_DEV_CHAR_WEN,
> > > > + master->regs + REG_DEV_CHAR);
> > > > + }
> > > > +}
> > > > +
> > > > +static void adi_i3c_master_detach_i3c_dev(struct i3c_dev_desc *dev)
> > > > +{
> > > > + struct i3c_master_controller *m = i3c_dev_get_master(dev);
> > > > + struct adi_i3c_master *master = to_adi_i3c_master(m);
> > > > + struct adi_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
> > > > + u8 addr;
> > > > +
> > > > + addr = dev->info.dyn_addr ? dev->info.dyn_addr : dev->info.static_addr;
> > > > +
> > > > + writel(REG_DEV_CHAR_ADDR(addr), master->regs + REG_DEV_CHAR);
> > > > + writel((readl(master->regs + REG_DEV_CHAR) &
> > > > + ~REG_DEV_CHAR_IS_ATTACHED) | REG_DEV_CHAR_WEN,
> > > > + master->regs + REG_DEV_CHAR);
> > > > +
> > > > + i3c_dev_set_master_data(dev, NULL);
> > > > + master->free_rr_slots |= BIT(data->id);
> > > > + kfree(data);
> > > > +}
> > > > +
> > > > +static int adi_i3c_master_attach_i2c_dev(struct i2c_dev_desc *dev)
> > > > +{
> > > > + struct i3c_master_controller *m = i2c_dev_get_master(dev);
> > > > + struct adi_i3c_master *master = to_adi_i3c_master(m);
> > > > + struct adi_i3c_i2c_dev_data *data;
> > > > + int slot;
> > > > +
> > > > + slot = adi_i3c_master_get_rr_slot(master, 0);
> > > > + if (slot < 0)
> > > > + return slot;
> > > > +
> > > > + data = kzalloc(sizeof(*data), GFP_KERNEL);
> > > > + if (!data)
> > > > + return -ENOMEM;
> > > > +
> > > > + data->id = slot;
> > > > + master->free_rr_slots &= ~BIT(slot);
> > > > + i2c_dev_set_master_data(dev, data);
> > > > +
> > > > + writel(REG_DEV_CHAR_ADDR(dev->addr) |
> > > > + REG_DEV_CHAR_IS_I2C | REG_DEV_CHAR_IS_ATTACHED | REG_DEV_CHAR_WEN,
> > > > + master->regs + REG_DEV_CHAR);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void adi_i3c_master_detach_i2c_dev(struct i2c_dev_desc *dev)
> > > > +{
> > > > + struct i3c_master_controller *m = i2c_dev_get_master(dev);
> > > > + struct adi_i3c_master *master = to_adi_i3c_master(m);
> > > > + struct adi_i3c_i2c_dev_data *data = i2c_dev_get_master_data(dev);
> > > > +
> > > > + writel(REG_DEV_CHAR_ADDR(dev->addr) |
> > > > + REG_DEV_CHAR_IS_I2C | REG_DEV_CHAR_WEN,
> > > > + master->regs + REG_DEV_CHAR);
> > > > +
> > > > + i2c_dev_set_master_data(dev, NULL);
> > > > + master->free_rr_slots |= BIT(data->id);
> > > > + kfree(data);
> > > > +}
> > > > +
> > > > +static void adi_i3c_master_bus_cleanup(struct i3c_master_controller *m)
> > > > +{
> > > > + struct adi_i3c_master *master = to_adi_i3c_master(m);
> > > > +
> > > > + adi_i3c_master_disable(master);
> > > > +}
> > > > +
> > > > +static void adi_i3c_master_upd_i3c_scl_lim(struct adi_i3c_master *master)
> > > > +{
> > > > + struct i3c_master_controller *m = &master->base;
> > > > + struct i3c_bus *bus = i3c_master_get_bus(m);
> > > > + u8 i3c_scl_lim = 0;
> > > > + struct i3c_dev_desc *dev;
> > > > + u8 pp_sg;
> > > > +
> > > > + i3c_bus_for_each_i3cdev(bus, dev) {
> > > > + u8 max_fscl;
> > > > +
> > > > + max_fscl = max(I3C_CCC_MAX_SDR_FSCL(dev->info.max_read_ds),
> > > > + I3C_CCC_MAX_SDR_FSCL(dev->info.max_write_ds));
> > > > +
> > > > + switch (max_fscl) {
> > > > + case I3C_SDR1_FSCL_8MHZ:
> > > > + max_fscl = PP_SG_6MHZ;
> > > > + break;
> > > > + case I3C_SDR2_FSCL_6MHZ:
> > > > + max_fscl = PP_SG_3MHZ;
> > > > + break;
> > > > + case I3C_SDR3_FSCL_4MHZ:
> > > > + max_fscl = PP_SG_3MHZ;
> > > > + break;
> > > > + case I3C_SDR4_FSCL_2MHZ:
> > > > + max_fscl = PP_SG_1MHZ;
> > > > + break;
> > > > + case I3C_SDR0_FSCL_MAX:
> > > > + default:
> > > > + max_fscl = PP_SG_12MHZ;
> > > > + break;
> > > > + }
> > > > +
> > > > + if (max_fscl &&
> > > > + (i3c_scl_lim > max_fscl || !i3c_scl_lim))
> > > > + i3c_scl_lim = max_fscl;
> > > > + }
> > > > +
> > > > + if (!i3c_scl_lim)
> > > > + return;
> > > > +
> > > > + master->i3c_scl_lim = i3c_scl_lim - 1;
> > > > +
> > > > + pp_sg = readl(master->regs + REG_OPS) & ~REG_OPS_PP_SG_MASK;
> > > > + pp_sg |= REG_OPS_SET_SG(master->i3c_scl_lim);
> > > > +
> > > > + writel(pp_sg, master->regs + REG_OPS);
> > > > +}
> > > > +
> > > > +static void adi_i3c_master_get_features(struct adi_i3c_master *master,
> > > > + unsigned int slot,
> > > > + struct i3c_device_info *info)
> > > > +{
> > > > + memset(info, 0, sizeof(*info));
> > > > +
> > > > + info->dyn_addr = 0x31;
> > >
> > > master address is hardcode?
> > >
> > Yes.
> > There is/was no register to read it from because the controller does not
> > support secondary mastership or changing role, so this address is not
> > used for anything (as is). Since it is odd, I will move add these
> > hardcoded values to the register map (read-only), still won't do
> > anything.
>
> need comments for 0x31
>
Ack, will likely become a register read but will also add a comment.
> > > > + info->dcr = 0x00;
> > > > + info->bcr = 0x40;
> > > > + info->pid = 0;
> > > > +}
> > > > +
> > > > +static int adi_i3c_master_do_daa(struct i3c_master_controller *m)
> > > > +{
> > > > + struct adi_i3c_master *master = to_adi_i3c_master(m);
> > > > + u32 irq_mask;
> > > > + int ret, addr;
> > > > +
> > > > + addr = 0x8;
> > > > + for (u8 i = 0; i < MAX_DEVS; i++) {
> > > > + addr = i3c_master_get_free_addr(m, addr);
> > > > + if (addr < 0)
> > > > + return addr;
> > >
> > > assume MAX_DEVS 128, if only get 4 address, suppose you should enumerate
> > > for such 4 devices, instead of return error and give up do_daa.
> > >
> > The IP Core supports just 16 devices (max # on the bus), so it should
> > not run-out of addresses, but I understand the assignment. As is, the IP
> > core has 16 slots and will only terminate the DAA from its side if it
> > run out of slots (if no device request an address (NACK bit), the DAA is
> > also terminated). Not writing a DA during the DA IRQ will just hang the
> > bus.
> >
> > For the suggestion to work I need to re-read the spec and rework the RTL
> > to allow this early termination. However, I am currently a little
> > opposed of changing the RTL beyond basic read-only registers
> > modifications.
> > > > + master->daa.addrs[i] = addr;
> > > > + }
> > > > +
> > > > + irq_mask = readl(master->regs + REG_IRQ_MASK);
> > > > + writel(irq_mask | REG_IRQ_PENDING_DAA,
> > > > + master->regs + REG_IRQ_MASK);
> > > > +
> > > > + master->daa.index = 0;
> > > > + ret = i3c_master_entdaa_locked(&master->base);
> > > > +
> > > > + writel(irq_mask, master->regs + REG_IRQ_MASK);
> > > > +
> > > > + /* DAA always finishes with CE2_ERROR or NACK_RESP */
> > > > + if (ret && ret != I3C_ERROR_M2)
> > > > + return ret;
> > > > +
> > > > + /* Add I3C devices discovered */
> > > > + for (u8 i = 0; i < master->daa.index; i++)
> > > > + i3c_master_add_i3c_dev_locked(m, master->daa.addrs[i]);
> > > > + /* Sync retrieved devs info with the IP */
> > > > + adi_i3c_master_sync_dev_char(m);
> > > > +
> > > > + i3c_master_defslvs_locked(&master->base);
> > > > +
> > > > + adi_i3c_master_upd_i3c_scl_lim(master);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int adi_i3c_master_bus_init(struct i3c_master_controller *m)
> > > > +{
> > > > + struct adi_i3c_master *master = to_adi_i3c_master(m);
> > > > + struct i3c_device_info info = { };
> > > > + int ret;
> > > > +
> > > > + ret = i3c_master_get_free_addr(m, 0);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + adi_i3c_master_get_features(master, 0, &info);
> > > > + ret = i3c_master_set_info(&master->base, &info);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + writel(REG_IBI_CONFIG_LISTEN,
> > > > + master->regs + REG_IBI_CONFIG);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void adi_i3c_master_handle_ibi(struct adi_i3c_master *master,
> > > > + u32 ibi)
> > > > +{
> > > > + struct adi_i3c_i2c_dev_data *data;
> > > > + struct i3c_ibi_slot *slot;
> > > > + struct i3c_dev_desc *dev;
> > > > + u8 da, id;
> > > > + u8 *mdb;
> > > > +
> > > > + da = (ibi >> 17) & GENMASK(6, 0);
> > > > + for (id = 0; id < master->ibi.num_slots; id++) {
> > > > + if (master->ibi.slots[id] &&
> > > > + master->ibi.slots[id]->info.dyn_addr == da)
> > > > + break;
> > > > + }
> > > > +
> > > > + if (id == master->ibi.num_slots)
> > > > + return;
> > > > +
> > > > + dev = master->ibi.slots[id];
> > > > + guard(spinlock)(&master->ibi.lock);
> > > > +
> > > > + data = i3c_dev_get_master_data(dev);
> > > > + slot = i3c_generic_ibi_get_free_slot(data->ibi_pool);
> > > > + if (!slot)
> > > > + return;
> > > > +
> > > > + mdb = slot->data;
> > > > + mdb[0] = (ibi >> 8) & GENMASK(7, 0);
> > > > +
> > > > + slot->len = 1;
> > > > + i3c_master_queue_ibi(dev, slot);
> > > > +}
> > > > +
> > > > +static void adi_i3c_master_demux_ibis(struct adi_i3c_master *master)
> > > > +{
> > > > + u32 status0;
> > > > +
> > > > + for (status0 = readl(master->regs + REG_FIFO_STATUS);
> > > > + !(status0 & REG_FIFO_STATUS_IBI_EMPTY);
> > > > + status0 = readl(master->regs + REG_FIFO_STATUS)) {
> > >
> > > use while()
> > >
> > Ack.
> > > > + u32 ibi = readl(master->regs + REG_IBI_FIFO);
> > > > +
> > > > + adi_i3c_master_handle_ibi(master, ibi);
> > > > + }
> > > > +}
> > > > +
> > > > +static void adi_i3c_master_handle_da_req(struct adi_i3c_master *master)
> > > > +{
> > > > + u8 payload0[8];
> > > > + u32 addr;
> > > > +
> > > > + /* Clear device characteristics */
> > > > + adi_i3c_master_rd_from_rx_fifo(master, payload0, 6);
> > > > + addr = master->daa.addrs[master->daa.index++];
> > > > + addr = (addr << 1) | (parity8(addr) ? 0 : 1);
> > > > +
> > > > + writel(addr, master->regs + REG_SDO_FIFO);
> > > > +}
> > > > +
> > > > +static irqreturn_t adi_i3c_master_irq(int irq, void *data)
> > > > +{
> > > > + struct adi_i3c_master *master = data;
> > > > + u32 pending;
> > > > +
> > > > + pending = readl(master->regs + REG_IRQ_PENDING);
> > > > + writel(pending, master->regs + REG_IRQ_PENDING);
> > >
> > > Add an empty line here
> > >
> > Ack.
> > > > + if (pending & REG_IRQ_PENDING_CMDR) {
> > > > + spin_lock(&master->xferqueue.lock);
> > > > + adi_i3c_master_end_xfer_locked(master, pending);
> > > > + spin_unlock(&master->xferqueue.lock);
> > >
> > > you use guard, keep consisent, use guard or scoped_guard here
> > >
> > scoped_guard
> > > > + }
> > > > + if (pending & REG_IRQ_PENDING_IBI)
> > > > + adi_i3c_master_demux_ibis(master);
> > > > + if (pending & REG_IRQ_PENDING_DAA)
> > > > + adi_i3c_master_handle_da_req(master);
> > > > +
> > > > + return IRQ_HANDLED;
> > > > +}
> > > > +
> > > > +static int adi_i3c_master_i2c_xfers(struct i2c_dev_desc *dev,
> > > > + struct i2c_msg *xfers,
> > > > + int nxfers)
> > > > +{
> > > > + struct i3c_master_controller *m = i2c_dev_get_master(dev);
> > > > + struct adi_i3c_master *master = to_adi_i3c_master(m);
> > > > + struct adi_i3c_xfer *xfer;
> > > > + int i, ret;
> > > > +
> > > > + if (!nxfers)
> > > > + return 0;
> > > > + for (i = 0; i < nxfers; i++) {
> > > > + if (xfers[i].flags & I2C_M_TEN)
> > > > + return -EOPNOTSUPP;
> > > > + }
> > > > + xfer = adi_i3c_master_alloc_xfer(master, nxfers);
> > > > + if (!xfer)
> > > > + return -ENOMEM;
> > > > +
> > > > + for (i = 0; i < nxfers; i++) {
> > > > + struct adi_i3c_cmd *ccmd = &xfer->cmds[i];
> > > > +
> > > > + ccmd->cmd0 = REG_CMD_FIFO_0_DEV_ADDR(xfers[i].addr);
> > > > +
> > > > + if (xfers[i].flags & I2C_M_RD) {
> > > > + ccmd->cmd0 |= REG_CMD_FIFO_0_RNW;
> > > > + ccmd->rx_buf = xfers[i].buf;
> > > > + ccmd->rx_len = xfers[i].len;
> > > > + } else {
> > > > + ccmd->tx_buf = xfers[i].buf;
> > > > + ccmd->tx_len = xfers[i].len;
> > > > + }
> > > > +
> > > > + ccmd->cmd0 |= REG_CMD_FIFO_0_LEN(xfers[i].len);
> > >
> > > Just curiouse
> > > What's happen if IBI happen (address arbitrate) at first command?
> > >
> > The controller does the state machine for the IBI and yield a IBIR
> > interrupt, then proceeds with the xfer, yielding the CMDR for each CMD.
>
> Does controller retry current command again after handle IBI?
>
> Frank Li
>
Yes, it resumes/retries the current command. After the IBI has been
resolved, the controller issues either a stop, to return to the Bus
Free/Idle condition if the command stack is empty, or repeated start,
to proceed with current command.
Looking into the spec i3c basic 1.1.1, fig 169, this controller never
Disable Target Interrupts after a NACK followed by a repeated start. The
driver would issue a command to do so in other opportunity.
Best regards,
Jorge
> > > > + }
> > > > +
> > > > + adi_i3c_master_queue_xfer(master, xfer);
> > > > + if (!wait_for_completion_timeout(&xfer->comp,
> > > > + m->i2c.timeout))
> > > > + adi_i3c_master_unqueue_xfer(master, xfer);
> > > > +
> > > > + ret = xfer->ret;
> > > > + kfree(xfer);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int adi_i3c_master_disable_ibi(struct i3c_dev_desc *dev)
> > > > +{
> > > > + struct i3c_master_controller *m = i3c_dev_get_master(dev);
> > > > + struct adi_i3c_master *master = to_adi_i3c_master(m);
> > > > + struct i3c_dev_desc *i3cdev;
> > > > + u32 enabled = 0;
> > > > + int ret;
> > > > +
> > > > + ret = i3c_master_disec_locked(m, dev->info.dyn_addr,
> > > > + I3C_CCC_EVENT_SIR);
> > > > +
> > > > + i3c_bus_for_each_i3cdev(&m->bus, i3cdev) {
> > > > + if (dev != i3cdev && i3cdev->ibi)
> > > > + enabled |= i3cdev->ibi->enabled;
> > > > + }
> > > > + if (!enabled) {
> > > > + writel(REG_IBI_CONFIG_LISTEN,
> > > > + master->regs + REG_IBI_CONFIG);
> > > > + writel(readl(master->regs + REG_IRQ_MASK) & ~REG_IRQ_PENDING_IBI,
> > > > + master->regs + REG_IRQ_MASK);
> > > > + }
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int adi_i3c_master_enable_ibi(struct i3c_dev_desc *dev)
> > > > +{
> > > > + struct i3c_master_controller *m = i3c_dev_get_master(dev);
> > > > + struct adi_i3c_master *master = to_adi_i3c_master(m);
> > > > +
> > > > + writel(REG_IBI_CONFIG_LISTEN | REG_IBI_CONFIG_ENABLE,
> > > > + master->regs + REG_IBI_CONFIG);
> > > > +
> > > > + writel(readl(master->regs + REG_IRQ_MASK) | REG_IRQ_PENDING_IBI,
> > > > + master->regs + REG_IRQ_MASK);
> > > > +
> > > > + return i3c_master_enec_locked(m, dev->info.dyn_addr,
> > > > + I3C_CCC_EVENT_SIR);
> > > > +}
> > > > +
> > > > +static int adi_i3c_master_request_ibi(struct i3c_dev_desc *dev,
> > > > + const struct i3c_ibi_setup *req)
> > > > +{
> > > > + struct i3c_master_controller *m = i3c_dev_get_master(dev);
> > > > + struct adi_i3c_master *master = to_adi_i3c_master(m);
> > > > + struct adi_i3c_i2c_dev_data *data;
> > > > + unsigned long flags;
> > > > + unsigned int i;
> > > > +
> > > > + data = i3c_dev_get_master_data(dev);
> > > > + data->ibi_pool = i3c_generic_ibi_alloc_pool(dev, req);
> > > > + if (IS_ERR(data->ibi_pool))
> > > > + return PTR_ERR(data->ibi_pool);
> > > > +
> > > > + spin_lock_irqsave(&master->ibi.lock, flags);
> > > > + for (i = 0; i < master->ibi.num_slots; i++) {
> > > > + if (!master->ibi.slots[i]) {
> > > > + data->ibi = i;
> > > > + master->ibi.slots[i] = dev;
> > > > + break;
> > > > + }
> > > > + }
> > > > + spin_unlock_irqrestore(&master->ibi.lock, flags);
> > >
> > > same here, use scope_guard to keep consistent.
> > >
> > Ack.
> > > > +
> > > > + if (i < master->ibi.num_slots)
> > > > + return 0;
> > > > +
> > > > + i3c_generic_ibi_free_pool(data->ibi_pool);
> > > > + data->ibi_pool = NULL;
> > > > +
> > > > + return -ENOSPC;
> > > > +}
> > > > +
> > > > +static void adi_i3c_master_free_ibi(struct i3c_dev_desc *dev)
> > > > +{
> > > > + struct i3c_master_controller *m = i3c_dev_get_master(dev);
> > > > + struct adi_i3c_master *master = to_adi_i3c_master(m);
> > > > + struct adi_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
> > > > + unsigned long flags;
> > > > +
> > > > + spin_lock_irqsave(&master->ibi.lock, flags);
> > > > + master->ibi.slots[data->ibi] = NULL;
> > > > + data->ibi = -1;
> > > > + spin_unlock_irqrestore(&master->ibi.lock, flags);
> > > > +
> > > > + i3c_generic_ibi_free_pool(data->ibi_pool);
> > > > +}
> > > > +
> > > > +static void adi_i3c_master_recycle_ibi_slot(struct i3c_dev_desc *dev,
> > > > + struct i3c_ibi_slot *slot)
> > > > +{
> > > > + struct adi_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
> > > > +
> > > > + i3c_generic_ibi_recycle_slot(data->ibi_pool, slot);
> > > > +}
> > > > +
> > > > +static const struct i3c_master_controller_ops adi_i3c_master_ops = {
> > > > + .bus_init = adi_i3c_master_bus_init,
> > > > + .bus_cleanup = adi_i3c_master_bus_cleanup,
> > > > + .attach_i3c_dev = adi_i3c_master_attach_i3c_dev,
> > > > + .reattach_i3c_dev = adi_i3c_master_reattach_i3c_dev,
> > > > + .detach_i3c_dev = adi_i3c_master_detach_i3c_dev,
> > > > + .attach_i2c_dev = adi_i3c_master_attach_i2c_dev,
> > > > + .detach_i2c_dev = adi_i3c_master_detach_i2c_dev,
> > > > + .do_daa = adi_i3c_master_do_daa,
> > > > + .supports_ccc_cmd = adi_i3c_master_supports_ccc_cmd,
> > > > + .send_ccc_cmd = adi_i3c_master_send_ccc_cmd,
> > > > + .priv_xfers = adi_i3c_master_priv_xfers,
> > > > + .i2c_xfers = adi_i3c_master_i2c_xfers,
> > > > + .request_ibi = adi_i3c_master_request_ibi,
> > > > + .enable_ibi = adi_i3c_master_enable_ibi,
> > > > + .disable_ibi = adi_i3c_master_disable_ibi,
> > > > + .free_ibi = adi_i3c_master_free_ibi,
> > > > + .recycle_ibi_slot = adi_i3c_master_recycle_ibi_slot,
> > > > +};
> > > > +
> > > > +static const struct of_device_id adi_i3c_master_of_match[] = {
> > > > + { .compatible = "adi,i3c-master" },
> > > > + {}
> > > > +};
> > > > +
> > > > +static int adi_i3c_master_probe(struct platform_device *pdev)
> > > > +{
> > > > + struct adi_i3c_master *master;
> > > > + unsigned int version;
> > > > + int ret, irq;
> > > > +
> > > > + master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL);
> > > > + if (!master)
> > > > + return -ENOMEM;
> > > > +
> > > > + master->regs = devm_platform_ioremap_resource(pdev, 0);
> > > > + if (IS_ERR(master->regs))
> > > > + return PTR_ERR(master->regs);
> > > > +
> > > > + master->clk = devm_clk_get_enabled(&pdev->dev, "axi");
> > > > + if (IS_ERR(master->clk))
> > > > + return PTR_ERR(master->clk);
> > >
> > > you have optional clock i3c, not use it in driver?
> > >
> > It is not used in the driver indeed, it describes how the IP Core is
> > compiled and wired, but doesn't affect the logic in the driver itself.
> > The RTL logic is always driven by a 100MHz clock, and the source is
> > either axi or i3c, if the latter present.
>
> can you use devm_clk_bulk_get_all_enabled() ?
>
I believe so, I would keep the required assertion like this then:
ret = devm_clk_bulk_get_all_enabled(&pdev->dev, &clk);
if (ret < 0)
return dev_err_probe(&pdev->dev, ret,
"Failed to get clocks\n");
while (ret--)
if (clk[ret].id && !strcmp(clk[ret].id, "axi"))
break;
if (ret == -1)
return dev_err_probe(&pdev->dev, -ENODEV,
"Failed to get clock axi\n");
Best regards,
Jorge
> > > > +
> > > > + irq = platform_get_irq(pdev, 0);
> > > > + if (irq < 0)
> > > > + return irq;
> > > > +
> > > > + version = readl(master->regs + REG_VERSION);
> > > > + if (VERSION_MAJOR(version) != 0) {
> > > > + dev_err(&pdev->dev, "Unsupported IP version %u.%u.%c\n",
> > > > + VERSION_MAJOR(version),
> > > > + VERSION_MINOR(version),
> > > > + VERSION_PATCH(version));
> > > > + return -EINVAL;
> > >
> > > return dev_err_probe();
> > Ack.
> > >
> > > Frank
> > Best regards,
> > Jorge
> > > > + }
> > > > +
> > > > + writel(0x00, master->regs + REG_ENABLE);
> > > > + writel(0x00, master->regs + REG_IRQ_MASK);
> > > > +
> > > > + ret = devm_request_irq(&pdev->dev, irq, adi_i3c_master_irq, 0,
> > > > + dev_name(&pdev->dev), master);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + platform_set_drvdata(pdev, master);
> > > > +
> > > > + master->maxdevs = MAX_DEVS;
> > > > + master->free_rr_slots = GENMASK(master->maxdevs, 1);
> > > > +
> > > > + writel(REG_IRQ_PENDING_CMDR, master->regs + REG_IRQ_MASK);
> > > > +
> > > > + spin_lock_init(&master->ibi.lock);
> > > > + master->ibi.num_slots = 15;
> > > > + master->ibi.slots = devm_kcalloc(&pdev->dev, master->ibi.num_slots,
> > > > + sizeof(*master->ibi.slots),
> > > > + GFP_KERNEL);
> > > > + if (!master->ibi.slots)
> > > > + return -ENOMEM;
> > > > +
> > > > + return i3c_master_register(&master->base, &pdev->dev,
> > > > + &adi_i3c_master_ops, false);
> > > > +}
> > > > +
> > > > +static void adi_i3c_master_remove(struct platform_device *pdev)
> > > > +{
> > > > + struct adi_i3c_master *master = platform_get_drvdata(pdev);
> > > > +
> > > > + writel(0xff, master->regs + REG_IRQ_PENDING);
> > > > + writel(0x00, master->regs + REG_IRQ_MASK);
> > > > + writel(0x01, master->regs + REG_ENABLE);
> > > > +
> > > > + i3c_master_unregister(&master->base);
> > > > +}
> > > > +
> > > > +static struct platform_driver adi_i3c_master = {
> > > > + .probe = adi_i3c_master_probe,
> > > > + .remove = adi_i3c_master_remove,
> > > > + .driver = {
> > > > + .name = "adi-i3c-master",
> > > > + .of_match_table = adi_i3c_master_of_match,
> > > > + },
> > > > +};
> > > > +module_platform_driver(adi_i3c_master);
> > > > +
> > > > +MODULE_AUTHOR("Jorge Marques <jorge.marques@...log.com>");
> > > > +MODULE_DESCRIPTION("Analog Devices I3C master driver");
> > > > +MODULE_LICENSE("GPL");
> > > >
> > > > --
> > > > 2.49.0
> > > >
Powered by blists - more mailing lists