* [PATCH v4 0/2] Add ADI I3C Controller
@ 2025-06-26 10:07 Jorge Marques
2025-06-26 10:07 ` [PATCH v4 1/2] dt-bindings: i3c: Add adi-i3c-master Jorge Marques
2025-06-26 10:07 ` [PATCH v4 2/2] i3c: master: Add driver for Analog Devices I3C Controller IP Jorge Marques
0 siblings, 2 replies; 17+ messages in thread
From: Jorge Marques @ 2025-06-26 10:07 UTC (permalink / raw)
To: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: linux-i3c, devicetree, linux-kernel, gastmaier, Jorge Marques
I3C Controller is subset of the I3C-basic specification to interface
peripherals through I3C and I2C. The controller RTL is FPGA
synthesizable and documentation is provided at
https://analogdevicesinc.github.io/hdl/library/i3c_controller
The main target for the I3C Controller IP is low-cost FPGAs.
In this version the driver supports IBI (only the MDB), I3C and I2C
transfers.
Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
Changes in v4:
Documentation/devicetree/bindings/i3c/adi,i3c-master.yaml:
- Add -1.00.a suffix where missing
- Extend clocks descriptions
- Add minItems to clock-names, to match clocks
- Use header in example
-
adi-i3c-master.c:
- Regmap:
- Add new controller info registers (dyn_addr, dcr, bcr, pid)
- Always decreasing fields
- Add line break between registers
- Reformat REG_DEV_CHAR_BSCR_IBI to use easier to read FIELD_GET,
FIELD_PREP
- Read controller info from regmap with explanation comment
- Use linux/fpga/adi-axi-common.h macros
- Use __counter_by macro on ncmds
- Use __free macro
- Use new i3c_writel_fifo and i3c_readl_fifo macros
- Rename bytes to buf when nbytes is present
- Use scoped_guard instead of spin_lock, spin_unlock
- Reformat loops to read fifo status, use while single line alternative
- Drop adi_i3c_master.max_devs, use MAX_DEVS directly
- Use devm_clk_bulk_get_all_enabled, dropping clock name match (CHECK_DTB does it)
- Init spin_lock
- Init list head
- Link to v3: https://lore.kernel.org/r/20250618-adi-i3c-master-v3-0-e66170a6cb95@analog.com
Changes in v3:
Documentation/devicetree/bindings/i3c/adi,i3c-master.yaml:
- Small reworking of the description
- Add -1.00.a suffix to compatible
adi-i3c-master.c:
- Misspelling
- Remove REG_CMD_FIFO_0_LEN_MAX since it is a HDL parameter
- Use adapter timeout value for I2C transfers, as in
https://lore.kernel.org/linux-i3c/aEBd%2FFIKADYr%2F631@lizhi-Precision-Tower-5810/T/#t
- Link to v2: https://lore.kernel.org/r/20250606-adi-i3c-master-v2-0-e68b9aad2630@analog.com
Changes in v2:
Documentation/devicetree/bindings/i3c/adi,i3c-master.yaml:
- Move allof
- Rename clocks to axi, i3c
adi-i3c-master.c:
- Update license year
- Rework regmap to use FIELD_GET, FIELD_PREP
- Reformat regmap to have FIELDS after REG, prefixed by reg name.
- Add overflow safeguards to cmd, tx fifos
- Fix macro related macros (mostly erroneous `| ~BITMASK`
- Use guard macros, remove goto.
- Simplify daa logic
- Replace devm_clk_get with devm_clk_get_enabled
- Solve 64bit->32bit warnings on x86_64 systems by casting to u32
- Immediate clear irq request flags, then handle it.
- Link to v1: https://lore.kernel.org/r/20250604-adi-i3c-master-v1-0-0488e80dafcb@analog.com
---
Jorge Marques (2):
dt-bindings: i3c: Add adi-i3c-master
i3c: master: Add driver for Analog Devices I3C Controller IP
.../bindings/i3c/adi,i3c-master-1.00.a.yaml | 66 ++
MAINTAINERS | 6 +
drivers/i3c/master/Kconfig | 11 +
drivers/i3c/master/Makefile | 1 +
drivers/i3c/master/adi-i3c-master.c | 1017 ++++++++++++++++++++
5 files changed, 1101 insertions(+)
---
base-commit: 51963783b876a2f493a3eac0ea9eba271cb6809a
change-id: 20250604-adi-i3c-master-2a5148c58c47
prerequisite-message-id: <20250622-i3c-writesl-readsl-v2-0-2afd34ec6306@analog.com>
prerequisite-patch-id: 5443f14ca82fc08593960fafdb43488cce56f7d9
prerequisite-patch-id: 647084f5fe09f4887f633b0b02b306912a156672
prerequisite-patch-id: 6f582bb2ef1aafb66f26c515a19d5efa06ab8968
Best regards,
--
Jorge Marques <jorge.marques@analog.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v4 1/2] dt-bindings: i3c: Add adi-i3c-master
2025-06-26 10:07 [PATCH v4 0/2] Add ADI I3C Controller Jorge Marques
@ 2025-06-26 10:07 ` Jorge Marques
2025-06-26 16:13 ` Frank Li
2025-06-27 6:56 ` Krzysztof Kozlowski
2025-06-26 10:07 ` [PATCH v4 2/2] i3c: master: Add driver for Analog Devices I3C Controller IP Jorge Marques
1 sibling, 2 replies; 17+ messages in thread
From: Jorge Marques @ 2025-06-26 10:07 UTC (permalink / raw)
To: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: linux-i3c, devicetree, linux-kernel, gastmaier, Jorge Marques
Add bindings doc for ADI I3C Controller IP core, a FPGA synthesizable IP
core that implements the MIPI I3C Basic controller specification.
Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
.../bindings/i3c/adi,i3c-master-1.00.a.yaml | 66 ++++++++++++++++++++++
MAINTAINERS | 5 ++
2 files changed, 71 insertions(+)
diff --git a/Documentation/devicetree/bindings/i3c/adi,i3c-master-1.00.a.yaml b/Documentation/devicetree/bindings/i3c/adi,i3c-master-1.00.a.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..9bad24187800cb47090f440b70c519b3f710adf2
--- /dev/null
+++ b/Documentation/devicetree/bindings/i3c/adi,i3c-master-1.00.a.yaml
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i3c/adi,i3c-master-1.00.a.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices I3C Controller
+
+description: |
+ FPGA-based I3C controller designed to interface with I3C and I2C peripherals,
+ implementing a subset of the I3C-basic specification.
+
+ https://analogdevicesinc.github.io/hdl/library/i3c_controller
+
+maintainers:
+ - Jorge Marques <jorge.marques@analog.com>
+
+properties:
+ compatible:
+ const: adi,i3c-master-1.00.a
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ minItems: 1
+ items:
+ - description: The AXI interconnect clock, drives the register map.
+ - description: The I3C controller clock. AXI clock drives all logic if not provided.
+
+ clock-names:
+ minItems: 1
+ items:
+ - const: axi
+ - const: i3c
+
+ interrupts:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+ - interrupts
+
+allOf:
+ - $ref: i3c.yaml#
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ i3c@44a00000 {
+ compatible = "adi,i3c-master-1.00.a";
+ reg = <0x44a00000 0x1000>;
+ interrupts = <3 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clkc 15>, <&clkc 15>;
+ clock-names = "axi", "i3c";
+ #address-cells = <3>;
+ #size-cells = <0>;
+
+ /* I3C and I2C devices */
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 96b82704950184bd71623ff41fc4df31e4c7fe87..6f56e17dcecf902c6812827c1ec3e067c65e9894 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11243,6 +11243,11 @@ S: Maintained
F: Documentation/devicetree/bindings/i3c/aspeed,ast2600-i3c.yaml
F: drivers/i3c/master/ast2600-i3c-master.c
+I3C DRIVER FOR ANALOG DEVICES I3C CONTROLLER IP
+M: Jorge Marques <jorge.marques@analog.com>
+S: Maintained
+F: Documentation/devicetree/bindings/i3c/adi,i3c-master.yaml
+
I3C DRIVER FOR CADENCE I3C MASTER IP
M: Przemysław Gaj <pgaj@cadence.com>
S: Maintained
--
2.49.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 2/2] i3c: master: Add driver for Analog Devices I3C Controller IP
2025-06-26 10:07 [PATCH v4 0/2] Add ADI I3C Controller Jorge Marques
2025-06-26 10:07 ` [PATCH v4 1/2] dt-bindings: i3c: Add adi-i3c-master Jorge Marques
@ 2025-06-26 10:07 ` Jorge Marques
2025-06-26 16:51 ` Frank Li
1 sibling, 1 reply; 17+ messages in thread
From: Jorge Marques @ 2025-06-26 10:07 UTC (permalink / raw)
To: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: linux-i3c, devicetree, linux-kernel, gastmaier, Jorge Marques
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@analog.com>
---
MAINTAINERS | 1 +
drivers/i3c/master/Kconfig | 11 +
drivers/i3c/master/Makefile | 1 +
drivers/i3c/master/adi-i3c-master.c | 1017 +++++++++++++++++++++++++++++++++++
4 files changed, 1030 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@analog.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@cadence.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..7e4dac4fb16553b0cbb667d503c65cc9187b92be
--- /dev/null
+++ b/drivers/i3c/master/adi-i3c-master.c
@@ -0,0 +1,1017 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * I3C Controller driver
+ * Copyright 2025 Analog Devices Inc.
+ * Author: Jorge Marques <jorge.marques@analog.com>
+ */
+
+#include <linux/bitops.h>
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/fpga/adi-axi-common.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>
+
+#include "../internals.h"
+
+#define MAX_DEVS 16
+
+#define REG_ENABLE 0x040
+
+#define REG_PID_L 0x054
+#define REG_PID_H 0x058
+#define REG_DCR_BCR_DA 0x05C
+#define REG_DCR_BCR_DA_GET_DA(x) FIELD_GET(GENMASK(22, 16), (x))
+#define REG_DCR_BCR_DA_GET_BCR(x) FIELD_GET(GENMASK(15, 8), (x))
+#define REG_DCR_BCR_DA_GET_DCR(x) FIELD_GET(GENMASK(7, 0), (x))
+
+#define REG_IRQ_MASK 0x080
+#define REG_IRQ_PENDING 0x084
+#define REG_IRQ_PENDING_DAA BIT(7)
+#define REG_IRQ_PENDING_IBI BIT(6)
+#define REG_IRQ_PENDING_CMDR BIT(5)
+
+#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_UDA_ERROR 8
+#define REG_CMDR_FIFO_NACK_RESP 6
+#define REG_CMDR_FIFO_CE2_ERROR 4
+#define REG_CMDR_FIFO_CE0_ERROR 1
+#define REG_CMDR_FIFO_NO_ERROR 0
+#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))
+
+#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_PP_SG_MASK GENMASK(6, 5)
+#define REG_OPS_SET_SG(x) FIELD_PREP(REG_OPS_PP_SG_MASK, (x))
+
+#define REG_IBI_CONFIG 0x140
+#define REG_IBI_CONFIG_ENABLE BIT(0)
+#define REG_IBI_CONFIG_LISTEN BIT(1)
+
+#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) FIELD_PREP(GENMASK(3, 2), (x))
+#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[] __counted_by(ncmds);
+};
+
+struct adi_i3c_master {
+ struct i3c_master_controller base;
+ u32 free_rr_slots;
+ 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 *buf, unsigned int nbytes)
+{
+ unsigned int n, m;
+
+ n = readl(master->regs + REG_SDO_FIFO_ROOM);
+ m = min(n, nbytes);
+ i3c_writel_fifo(master->regs + REG_SDO_FIFO, buf, nbytes);
+}
+
+static void adi_i3c_master_rd_from_rx_fifo(struct adi_i3c_master *master,
+ u8 *buf, unsigned int nbytes)
+{
+ i3c_readl_fifo(master->regs + REG_SDI_FIFO, buf, nbytes);
+}
+
+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;
+
+ if (!xfer)
+ return;
+
+ while (!(readl(master->regs + REG_FIFO_STATUS) & REG_FIFO_STATUS_CMDR_EMPTY)) {
+ 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_comp; i++) {
+ 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 __free(kfree) = NULL;
+ 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]);
+
+ 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 __free(kfree) = NULL;
+ int i, ret;
+
+ if (!nxfers)
+ return 0;
+
+ 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]);
+
+ 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->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;
+ u32 bcr_ibi;
+ 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);
+ bcr_ibi = FIELD_GET(I3C_BCR_IBI_PAYLOAD | I3C_BCR_IBI_REQ_CAP, (i3cdev->info.bcr));
+ writel(readl(master->regs + REG_DEV_CHAR) |
+ REG_DEV_CHAR_BCR_IBI(bcr_ibi) | 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)
+{
+ u32 buf;
+
+ /* Dynamic address and PID are for identification only */
+ memset(info, 0, sizeof(*info));
+ buf = readl(master->regs + REG_DCR_BCR_DA);
+ info->dyn_addr = REG_DCR_BCR_DA_GET_DA(buf);
+ info->dcr = REG_DCR_BCR_DA_GET_DCR(buf);
+ info->bcr = REG_DCR_BCR_DA_GET_BCR(buf);
+ info->pid = readl(master->regs + REG_PID_L);
+ info->pid |= (u64)readl(master->regs + REG_PID_H) << 32;
+}
+
+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;
+ 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)
+{
+ while (!(readl(master->regs + REG_FIFO_STATUS) & REG_FIFO_STATUS_IBI_EMPTY)) {
+ 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;
+
+ 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);
+ if (pending & REG_IRQ_PENDING_CMDR) {
+ scoped_guard(spinlock_irqsave, &master->xferqueue.lock) {
+ adi_i3c_master_end_xfer_locked(master, pending);
+ }
+ }
+ 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 __free(kfree) = NULL;
+ int i;
+
+ 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);
+ }
+
+ adi_i3c_master_queue_xfer(master, xfer);
+ if (!wait_for_completion_timeout(&xfer->comp,
+ m->i2c.timeout))
+ adi_i3c_master_unqueue_xfer(master, xfer);
+
+ return xfer->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 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);
+
+ scoped_guard(spinlock_irqsave, &master->ibi.lock) {
+ for (i = 0; i < master->ibi.num_slots; i++) {
+ if (!master->ibi.slots[i]) {
+ data->ibi = i;
+ master->ibi.slots[i] = dev;
+ break;
+ }
+ }
+ }
+
+ 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);
+
+ scoped_guard(spinlock_irqsave, &master->ibi.lock) {
+ master->ibi.slots[data->ibi] = NULL;
+ }
+
+ 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;
+ struct clk_bulk_data *clk;
+ 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);
+
+ 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");
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0)
+ return irq;
+
+ version = readl(master->regs + ADI_AXI_REG_VERSION);
+ if (ADI_AXI_PCORE_VER_MAJOR(version) != 0)
+ dev_err_probe(&pdev->dev, -ENODEV, "Unsupported peripheral version %u.%u.%u\n",
+ ADI_AXI_PCORE_VER_MAJOR(version),
+ ADI_AXI_PCORE_VER_MINOR(version),
+ ADI_AXI_PCORE_VER_PATCH(version));
+
+ 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->free_rr_slots = GENMASK(MAX_DEVS, 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;
+
+ spin_lock_init(&master->xferqueue.lock);
+ INIT_LIST_HEAD(&master->xferqueue.list);
+
+ 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@analog.com>");
+MODULE_DESCRIPTION("Analog Devices I3C master driver");
+MODULE_LICENSE("GPL");
--
2.49.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: i3c: Add adi-i3c-master
2025-06-26 10:07 ` [PATCH v4 1/2] dt-bindings: i3c: Add adi-i3c-master Jorge Marques
@ 2025-06-26 16:13 ` Frank Li
2025-06-27 6:56 ` Krzysztof Kozlowski
1 sibling, 0 replies; 17+ messages in thread
From: Frank Li @ 2025-06-26 16:13 UTC (permalink / raw)
To: Jorge Marques
Cc: Alexandre Belloni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-i3c, devicetree, linux-kernel, gastmaier
On Thu, Jun 26, 2025 at 12:07:36PM +0200, Jorge Marques wrote:
> Add bindings doc for ADI I3C Controller IP core, a FPGA synthesizable IP
> core that implements the MIPI I3C Basic controller specification.
>
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> ---
Reviewed-by: Frank Li <Frank.Li@nxp.com>
> .../bindings/i3c/adi,i3c-master-1.00.a.yaml | 66 ++++++++++++++++++++++
> MAINTAINERS | 5 ++
> 2 files changed, 71 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/i3c/adi,i3c-master-1.00.a.yaml b/Documentation/devicetree/bindings/i3c/adi,i3c-master-1.00.a.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..9bad24187800cb47090f440b70c519b3f710adf2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i3c/adi,i3c-master-1.00.a.yaml
> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i3c/adi,i3c-master-1.00.a.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices I3C Controller
> +
> +description: |
> + FPGA-based I3C controller designed to interface with I3C and I2C peripherals,
> + implementing a subset of the I3C-basic specification.
> +
> + https://analogdevicesinc.github.io/hdl/library/i3c_controller
> +
> +maintainers:
> + - Jorge Marques <jorge.marques@analog.com>
> +
> +properties:
> + compatible:
> + const: adi,i3c-master-1.00.a
> +
> + reg:
> + maxItems: 1
> +
> + clocks:
> + minItems: 1
> + items:
> + - description: The AXI interconnect clock, drives the register map.
> + - description: The I3C controller clock. AXI clock drives all logic if not provided.
> +
> + clock-names:
> + minItems: 1
> + items:
> + - const: axi
> + - const: i3c
> +
> + interrupts:
> + maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - clock-names
> + - interrupts
> +
> +allOf:
> + - $ref: i3c.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> +
> + i3c@44a00000 {
> + compatible = "adi,i3c-master-1.00.a";
> + reg = <0x44a00000 0x1000>;
> + interrupts = <3 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&clkc 15>, <&clkc 15>;
> + clock-names = "axi", "i3c";
> + #address-cells = <3>;
> + #size-cells = <0>;
> +
> + /* I3C and I2C devices */
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 96b82704950184bd71623ff41fc4df31e4c7fe87..6f56e17dcecf902c6812827c1ec3e067c65e9894 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11243,6 +11243,11 @@ S: Maintained
> F: Documentation/devicetree/bindings/i3c/aspeed,ast2600-i3c.yaml
> F: drivers/i3c/master/ast2600-i3c-master.c
>
> +I3C DRIVER FOR ANALOG DEVICES I3C CONTROLLER IP
> +M: Jorge Marques <jorge.marques@analog.com>
> +S: Maintained
> +F: Documentation/devicetree/bindings/i3c/adi,i3c-master.yaml
> +
> I3C DRIVER FOR CADENCE I3C MASTER IP
> M: Przemysław Gaj <pgaj@cadence.com>
> S: Maintained
>
> --
> 2.49.0
>
>
> --
> linux-i3c mailing list
> linux-i3c@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/2] i3c: master: Add driver for Analog Devices I3C Controller IP
2025-06-26 10:07 ` [PATCH v4 2/2] i3c: master: Add driver for Analog Devices I3C Controller IP Jorge Marques
@ 2025-06-26 16:51 ` Frank Li
2025-06-27 14:10 ` Jorge Marques
0 siblings, 1 reply; 17+ messages in thread
From: Frank Li @ 2025-06-26 16:51 UTC (permalink / raw)
To: Jorge Marques
Cc: Alexandre Belloni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-i3c, devicetree, linux-kernel, gastmaier
On Thu, Jun 26, 2025 at 12:07:37PM +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@analog.com>
> ---
> MAINTAINERS | 1 +
> drivers/i3c/master/Kconfig | 11 +
> drivers/i3c/master/Makefile | 1 +
> drivers/i3c/master/adi-i3c-master.c | 1017 +++++++++++++++++++++++++++++++++++
> 4 files changed, 1030 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@analog.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@cadence.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..7e4dac4fb16553b0cbb667d503c65cc9187b92be
> --- /dev/null
> +++ b/drivers/i3c/master/adi-i3c-master.c
> @@ -0,0 +1,1017 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * I3C Controller driver
> + * Copyright 2025 Analog Devices Inc.
> + * Author: Jorge Marques <jorge.marques@analog.com>
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/fpga/adi-axi-common.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>
> +
> +#include "../internals.h"
> +
> +#define MAX_DEVS 16
name is too short and quite common, I worry about conflict with other
define in future. Can you use ADI_MAX_DEVS?
> +
> +#define REG_ENABLE 0x040
> +
> +#define REG_PID_L 0x054
> +#define REG_PID_H 0x058
> +#define REG_DCR_BCR_DA 0x05C
> +#define REG_DCR_BCR_DA_GET_DA(x) FIELD_GET(GENMASK(22, 16), (x))
> +#define REG_DCR_BCR_DA_GET_BCR(x) FIELD_GET(GENMASK(15, 8), (x))
> +#define REG_DCR_BCR_DA_GET_DCR(x) FIELD_GET(GENMASK(7, 0), (x))
> +
> +#define REG_IRQ_MASK 0x080
> +#define REG_IRQ_PENDING 0x084
> +#define REG_IRQ_PENDING_DAA BIT(7)
> +#define REG_IRQ_PENDING_IBI BIT(6)
> +#define REG_IRQ_PENDING_CMDR BIT(5)
> +
> +#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)
Does check patch complain miss () at '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_UDA_ERROR 8
> +#define REG_CMDR_FIFO_NACK_RESP 6
> +#define REG_CMDR_FIFO_CE2_ERROR 4
> +#define REG_CMDR_FIFO_CE0_ERROR 1
> +#define REG_CMDR_FIFO_NO_ERROR 0
> +#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))
> +
> +#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_PP_SG_MASK GENMASK(6, 5)
> +#define REG_OPS_SET_SG(x) FIELD_PREP(REG_OPS_PP_SG_MASK, (x))
> +
> +#define REG_IBI_CONFIG 0x140
> +#define REG_IBI_CONFIG_ENABLE BIT(0)
> +#define REG_IBI_CONFIG_LISTEN BIT(1)
> +
> +#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) FIELD_PREP(GENMASK(3, 2), (x))
> +#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};
add empty line here
> +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[] __counted_by(ncmds);
> +};
> +
> +struct adi_i3c_master {
> + struct i3c_master_controller base;
> + u32 free_rr_slots;
> + 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 *buf, unsigned int nbytes)
> +{
> + unsigned int n, m;
> +
> + n = readl(master->regs + REG_SDO_FIFO_ROOM);
> + m = min(n, nbytes);
> + i3c_writel_fifo(master->regs + REG_SDO_FIFO, buf, nbytes);
You need mention at cover letter or some place, dependent on
https://lore.kernel.org/linux-i3c/20250622-i3c-writesl-readsl-v2-0-2afd34ec6306@analog.com/T/#t
> +}
> +
> +static void adi_i3c_master_rd_from_rx_fifo(struct adi_i3c_master *master,
> + u8 *buf, unsigned int nbytes)
> +{
> + i3c_readl_fifo(master->regs + REG_SDI_FIFO, buf, nbytes);
> +}
I think you can direct use i3c_readl_fifo() without wrap
adi_i3c_master_rd_from_rx_fifo().
> +
> +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;
> +
> + if (!xfer)
> + return;
> +
> + while (!(readl(master->regs + REG_FIFO_STATUS) & REG_FIFO_STATUS_CMDR_EMPTY)) {
> + 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_comp; i++) {
> + 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 __free(kfree) = NULL;
> + 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]);
> +
> + 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 __free(kfree) = NULL;
> + int i, ret;
> +
> + if (!nxfers)
> + return 0;
> +
> + 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]);
> +
> + return ret;
> +}
> +
> +struct adi_i3c_i2c_dev_data {
> + u16 id;
> + s16 ibi;
> + struct i3c_generic_ibi_pool *ibi_pool;
can you move ibi_pool to fist one? in 64bit system, it is align to 8 byte
so there will be 4 bytes hole in adi_i3c_i2c_dev_data.
> +};
> +
> +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->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;
> + u32 bcr_ibi;
> + 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);
> + bcr_ibi = FIELD_GET(I3C_BCR_IBI_PAYLOAD | I3C_BCR_IBI_REQ_CAP, (i3cdev->info.bcr));
> + writel(readl(master->regs + REG_DEV_CHAR) |
> + REG_DEV_CHAR_BCR_IBI(bcr_ibi) | 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)
> +{
> + u32 buf;
> +
> + /* Dynamic address and PID are for identification only */
> + memset(info, 0, sizeof(*info));
> + buf = readl(master->regs + REG_DCR_BCR_DA);
> + info->dyn_addr = REG_DCR_BCR_DA_GET_DA(buf);
> + info->dcr = REG_DCR_BCR_DA_GET_DCR(buf);
> + info->bcr = REG_DCR_BCR_DA_GET_BCR(buf);
> + info->pid = readl(master->regs + REG_PID_L);
> + info->pid |= (u64)readl(master->regs + REG_PID_H) << 32;
> +}
> +
> +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;
the first 8 address already reserved at i3c_bus_init_addrslots(). You can
start from 0, i3c_master_get_free_addr() will return from 8 auto.
> + for (u8 i = 0; i < MAX_DEVS; i++) {
> + addr = i3c_master_get_free_addr(m, addr);
> + if (addr < 0)
> + return addr;
> + 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);
This version should be fine, but I suggest use .set_speed callback. If it
can't match your requirment, we can improve it.
> +
> + 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);
this funciton is little bit long, I suggest use scoped_guard to make
scoped_guard(spinlock, &master->ibi.lock) {
because it is spin_lock, so lock time should be as less as possible.
> +
> + 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)
> +{
> + while (!(readl(master->regs + REG_FIFO_STATUS) & REG_FIFO_STATUS_IBI_EMPTY)) {
> + 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;
> +
> + 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);
> + if (pending & REG_IRQ_PENDING_CMDR) {
> + scoped_guard(spinlock_irqsave, &master->xferqueue.lock) {
> + adi_i3c_master_end_xfer_locked(master, pending);
> + }
> + }
> + 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 __free(kfree) = NULL;
> + int i;
> +
> + 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);
> + }
> +
> + adi_i3c_master_queue_xfer(master, xfer);
> + if (!wait_for_completion_timeout(&xfer->comp,
> + m->i2c.timeout))
> + adi_i3c_master_unqueue_xfer(master, xfer);
> +
> + return xfer->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 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);
> +
> + scoped_guard(spinlock_irqsave, &master->ibi.lock) {
> + for (i = 0; i < master->ibi.num_slots; i++) {
> + if (!master->ibi.slots[i]) {
> + data->ibi = i;
> + master->ibi.slots[i] = dev;
> + break;
> + }
> + }
> + }
> +
> + 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);
> +
> + scoped_guard(spinlock_irqsave, &master->ibi.lock) {
> + master->ibi.slots[data->ibi] = NULL;
> + }
> +
> + 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" },
binding compatible string changed, you need update here also.
Frank
> + {}
> +};
> +
> +static int adi_i3c_master_probe(struct platform_device *pdev)
> +{
> + struct adi_i3c_master *master;
> + struct clk_bulk_data *clk;
> + 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);
> +
> + 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");
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0)
> + return irq;
> +
> + version = readl(master->regs + ADI_AXI_REG_VERSION);
> + if (ADI_AXI_PCORE_VER_MAJOR(version) != 0)
> + dev_err_probe(&pdev->dev, -ENODEV, "Unsupported peripheral version %u.%u.%u\n",
> + ADI_AXI_PCORE_VER_MAJOR(version),
> + ADI_AXI_PCORE_VER_MINOR(version),
> + ADI_AXI_PCORE_VER_PATCH(version));
> +
> + 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->free_rr_slots = GENMASK(MAX_DEVS, 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;
> +
> + spin_lock_init(&master->xferqueue.lock);
> + INIT_LIST_HEAD(&master->xferqueue.list);
> +
> + 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@analog.com>");
> +MODULE_DESCRIPTION("Analog Devices I3C master driver");
> +MODULE_LICENSE("GPL");
>
> --
> 2.49.0
>
>
> --
> linux-i3c mailing list
> linux-i3c@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: i3c: Add adi-i3c-master
2025-06-26 10:07 ` [PATCH v4 1/2] dt-bindings: i3c: Add adi-i3c-master Jorge Marques
2025-06-26 16:13 ` Frank Li
@ 2025-06-27 6:56 ` Krzysztof Kozlowski
2025-06-27 14:38 ` Jorge Marques
1 sibling, 1 reply; 17+ messages in thread
From: Krzysztof Kozlowski @ 2025-06-27 6:56 UTC (permalink / raw)
To: Jorge Marques
Cc: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-i3c, devicetree, linux-kernel, gastmaier
On Thu, Jun 26, 2025 at 12:07:36PM +0200, Jorge Marques wrote:
> Add bindings doc for ADI I3C Controller IP core, a FPGA synthesizable IP
> core that implements the MIPI I3C Basic controller specification.
How did you resolve my last comment? I don't see any explanation -
neither here nor in the binding description. Binding description is
actually better place, I think now.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/2] i3c: master: Add driver for Analog Devices I3C Controller IP
2025-06-26 16:51 ` Frank Li
@ 2025-06-27 14:10 ` Jorge Marques
0 siblings, 0 replies; 17+ messages in thread
From: Jorge Marques @ 2025-06-27 14:10 UTC (permalink / raw)
To: Frank Li
Cc: Jorge Marques, Alexandre Belloni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-i3c, devicetree,
linux-kernel
Hi Frank,
There is some commentary from my side on set_speed, and one or other
NACK.
On Thu, Jun 26, 2025 at 12:51:28PM -0400, Frank Li wrote:
> On Thu, Jun 26, 2025 at 12:07:37PM +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@analog.com>
> > ---
> > MAINTAINERS | 1 +
> > drivers/i3c/master/Kconfig | 11 +
> > drivers/i3c/master/Makefile | 1 +
> > drivers/i3c/master/adi-i3c-master.c | 1017 +++++++++++++++++++++++++++++++++++
> > 4 files changed, 1030 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@analog.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@cadence.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..7e4dac4fb16553b0cbb667d503c65cc9187b92be
> > --- /dev/null
> > +++ b/drivers/i3c/master/adi-i3c-master.c
> > @@ -0,0 +1,1017 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * I3C Controller driver
> > + * Copyright 2025 Analog Devices Inc.
> > + * Author: Jorge Marques <jorge.marques@analog.com>
> > + */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/fpga/adi-axi-common.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>
> > +
> > +#include "../internals.h"
> > +
> > +#define MAX_DEVS 16
>
> name is too short and quite common, I worry about conflict with other
> define in future. Can you use ADI_MAX_DEVS?
>
Sure
> > +
> > +#define REG_ENABLE 0x040
> > +
> > +#define REG_PID_L 0x054
> > +#define REG_PID_H 0x058
> > +#define REG_DCR_BCR_DA 0x05C
> > +#define REG_DCR_BCR_DA_GET_DA(x) FIELD_GET(GENMASK(22, 16), (x))
> > +#define REG_DCR_BCR_DA_GET_BCR(x) FIELD_GET(GENMASK(15, 8), (x))
> > +#define REG_DCR_BCR_DA_GET_DCR(x) FIELD_GET(GENMASK(7, 0), (x))
> > +
> > +#define REG_IRQ_MASK 0x080
> > +#define REG_IRQ_PENDING 0x084
> > +#define REG_IRQ_PENDING_DAA BIT(7)
> > +#define REG_IRQ_PENDING_IBI BIT(6)
> > +#define REG_IRQ_PENDING_CMDR BIT(5)
> > +
> > +#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)
>
> Does check patch complain miss () at 'a'
>
Good question why it didn't.
Will add.
> > +#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_UDA_ERROR 8
> > +#define REG_CMDR_FIFO_NACK_RESP 6
> > +#define REG_CMDR_FIFO_CE2_ERROR 4
> > +#define REG_CMDR_FIFO_CE0_ERROR 1
> > +#define REG_CMDR_FIFO_NO_ERROR 0
> > +#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))
> > +
> > +#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_PP_SG_MASK GENMASK(6, 5)
> > +#define REG_OPS_SET_SG(x) FIELD_PREP(REG_OPS_PP_SG_MASK, (x))
> > +
> > +#define REG_IBI_CONFIG 0x140
> > +#define REG_IBI_CONFIG_ENABLE BIT(0)
> > +#define REG_IBI_CONFIG_LISTEN BIT(1)
> > +
> > +#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) FIELD_PREP(GENMASK(3, 2), (x))
> > +#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};
>
> add empty line here
>
Ack
> > +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[] __counted_by(ncmds);
> > +};
> > +
> > +struct adi_i3c_master {
> > + struct i3c_master_controller base;
> > + u32 free_rr_slots;
> > + 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 *buf, unsigned int nbytes)
> > +{
> > + unsigned int n, m;
> > +
> > + n = readl(master->regs + REG_SDO_FIFO_ROOM);
> > + m = min(n, nbytes);
> > + i3c_writel_fifo(master->regs + REG_SDO_FIFO, buf, nbytes);
>
> You need mention at cover letter or some place, dependent on
> https://lore.kernel.org/linux-i3c/20250622-i3c-writesl-readsl-v2-0-2afd34ec6306@analog.com/T/#t
>
Will add to the top of the cover also.
I used `b4 prep --edit-deps` and `--check-deps` which appends at the end
of the cover as
prerequisite-message-id: <20250622-i3c-writesl-readsl-v2-0-2afd34ec6306@analog.com>
> > +}
> > +
> > +static void adi_i3c_master_rd_from_rx_fifo(struct adi_i3c_master *master,
> > + u8 *buf, unsigned int nbytes)
> > +{
> > + i3c_readl_fifo(master->regs + REG_SDI_FIFO, buf, nbytes);
> > +}
>
> I think you can direct use i3c_readl_fifo() without wrap
> adi_i3c_master_rd_from_rx_fifo().
>
Hum, it is used twice in the code, involve a register address..
I would keep as is, feels odd to have only the write wrapper method (even
though has the FIFO_ROOM assert.
> > +
> > +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;
> > +
> > + if (!xfer)
> > + return;
> > +
> > + while (!(readl(master->regs + REG_FIFO_STATUS) & REG_FIFO_STATUS_CMDR_EMPTY)) {
> > + 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_comp; i++) {
> > + 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 __free(kfree) = NULL;
> > + 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]);
> > +
> > + 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 __free(kfree) = NULL;
> > + int i, ret;
> > +
> > + if (!nxfers)
> > + return 0;
> > +
> > + 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]);
> > +
> > + return ret;
> > +}
> > +
> > +struct adi_i3c_i2c_dev_data {
> > + u16 id;
> > + s16 ibi;
> > + struct i3c_generic_ibi_pool *ibi_pool;
>
> can you move ibi_pool to fist one? in 64bit system, it is align to 8 byte
> so there will be 4 bytes hole in adi_i3c_i2c_dev_data.
>
Yes
> > +};
> > +
> > +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->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;
> > + u32 bcr_ibi;
> > + 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);
> > + bcr_ibi = FIELD_GET(I3C_BCR_IBI_PAYLOAD | I3C_BCR_IBI_REQ_CAP, (i3cdev->info.bcr));
> > + writel(readl(master->regs + REG_DEV_CHAR) |
> > + REG_DEV_CHAR_BCR_IBI(bcr_ibi) | 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)
> > +{
> > + u32 buf;
> > +
> > + /* Dynamic address and PID are for identification only */
> > + memset(info, 0, sizeof(*info));
> > + buf = readl(master->regs + REG_DCR_BCR_DA);
> > + info->dyn_addr = REG_DCR_BCR_DA_GET_DA(buf);
> > + info->dcr = REG_DCR_BCR_DA_GET_DCR(buf);
> > + info->bcr = REG_DCR_BCR_DA_GET_BCR(buf);
> > + info->pid = readl(master->regs + REG_PID_L);
> > + info->pid |= (u64)readl(master->regs + REG_PID_H) << 32;
> > +}
> > +
> > +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;
>
> the first 8 address already reserved at i3c_bus_init_addrslots(). You can
> start from 0, i3c_master_get_free_addr() will return from 8 auto.
>
OK.
> > + for (u8 i = 0; i < MAX_DEVS; i++) {
> > + addr = i3c_master_get_free_addr(m, addr);
> > + if (addr < 0)
> > + return addr;
> > + 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);
>
> This version should be fine, but I suggest use .set_speed callback. If it
> can't match your requirment, we can improve it.
>
adi_i3c_master_upd_i3c_scl_lim configures the push pull speed according
the spec. Ideally no one would manufacture a peripheral that doesn't
support 12.5MHz, but the controller support slower push pull speed
regardless to my opinion and according the spec.
Looking into the set_speed callback, that's actually interesting, and
the name is rather misleading, something like "set_open_drain_speed"
would be rather clearer. Still, what it is actually doing, as seen in
the svc-i3c-master, is setting a 50% duty-cycle, and ...
... solely for the first broadcast address during bus
initialization, because, quote (6.2, Note 9):
The Controller uses this timing to send the first Broadcast Address
after Bus initialization, in order to disable the I2C Spike Filter for
applicable I3C Target Devices.
But also, quote (5.1.2.2.2):
If the I3C Controller intends to transmit solely to other I3C Devices
(i.e., not to any I2C Devices), then it may optionally maintain the
SCL pulse width below 50 ns, with the result that any I2C Devices
present on the Bus will see only a 0 value. This shorter pulse width
produces a higher data rate, because for Open Drain Arbitration only
the Low time is extended. See Figure 13, middle waveform.
So it handles a corner-case of when I3C devices are in I2C mode.
Let's look what is on the market with I2C/I3C dual support.
ST LSM6DSOX, INT1 pin:
- not connected: dual mode, the ENTDA/DAA must be performed in I2C
fast mode. When the slave is addressed, the I²C slave is disabled
and the timing is compatible with I3C specifications (1)
- connected: only i3c
TDK ICM-42670-P:
The device automatically switches to I3C after receiving 0x7E+W (2).
(1) That means the whole DAA must be done in open drain, which is not
enforced by the abstraction, the controller may or may not do the whole
DAA in open drain.
(2) No mention of the 0x7E+W high time, so I assume 200ns according to
(6.2 Note 9).
If I were to add support to it (6.2 Note 9), I wouldn't do it as runtime
configurable, but as a devicetree node, or even solely at the RTL logic,
as the default header waveform for the ENTDAA transfer.
> > +
> > + 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);
>
> this funciton is little bit long, I suggest use scoped_guard to make
> scoped_guard(spinlock, &master->ibi.lock) {
>
> because it is spin_lock, so lock time should be as less as possible.
>
> > +
> > + data = i3c_dev_get_master_data(dev);
I can only move `data` before the lock.
And reformat the MDB var like this (the compiler should squash it).
mdb = (((raw) >> 8) & GENMASK(7, 0));
slot->data[0] = mdb;
> > + 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)
> > +{
> > + while (!(readl(master->regs + REG_FIFO_STATUS) & REG_FIFO_STATUS_IBI_EMPTY)) {
> > + 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;
> > +
> > + 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);
> > + if (pending & REG_IRQ_PENDING_CMDR) {
> > + scoped_guard(spinlock_irqsave, &master->xferqueue.lock) {
> > + adi_i3c_master_end_xfer_locked(master, pending);
> > + }
> > + }
> > + 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 __free(kfree) = NULL;
> > + int i;
> > +
> > + 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);
> > + }
> > +
> > + adi_i3c_master_queue_xfer(master, xfer);
> > + if (!wait_for_completion_timeout(&xfer->comp,
> > + m->i2c.timeout))
> > + adi_i3c_master_unqueue_xfer(master, xfer);
> > +
> > + return xfer->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 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);
> > +
> > + scoped_guard(spinlock_irqsave, &master->ibi.lock) {
> > + for (i = 0; i < master->ibi.num_slots; i++) {
> > + if (!master->ibi.slots[i]) {
> > + data->ibi = i;
> > + master->ibi.slots[i] = dev;
> > + break;
> > + }
> > + }
> > + }
> > +
> > + 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);
> > +
> > + scoped_guard(spinlock_irqsave, &master->ibi.lock) {
> > + master->ibi.slots[data->ibi] = NULL;
> > + }
> > +
> > + 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" },
>
> binding compatible string changed, you need update here also.
>
> Frank
>
Yes. Thank you.
Best regards,
Jorge
> > + {}
> > +};
> > +
> > +static int adi_i3c_master_probe(struct platform_device *pdev)
> > +{
> > + struct adi_i3c_master *master;
> > + struct clk_bulk_data *clk;
> > + 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);
> > +
> > + 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");
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0)
> > + return irq;
> > +
> > + version = readl(master->regs + ADI_AXI_REG_VERSION);
> > + if (ADI_AXI_PCORE_VER_MAJOR(version) != 0)
> > + dev_err_probe(&pdev->dev, -ENODEV, "Unsupported peripheral version %u.%u.%u\n",
> > + ADI_AXI_PCORE_VER_MAJOR(version),
> > + ADI_AXI_PCORE_VER_MINOR(version),
> > + ADI_AXI_PCORE_VER_PATCH(version));
> > +
> > + 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->free_rr_slots = GENMASK(MAX_DEVS, 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;
> > +
> > + spin_lock_init(&master->xferqueue.lock);
> > + INIT_LIST_HEAD(&master->xferqueue.list);
> > +
> > + 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@analog.com>");
> > +MODULE_DESCRIPTION("Analog Devices I3C master driver");
> > +MODULE_LICENSE("GPL");
> >
> > --
> > 2.49.0
> >
> >
> > --
> > linux-i3c mailing list
> > linux-i3c@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: i3c: Add adi-i3c-master
2025-06-27 6:56 ` Krzysztof Kozlowski
@ 2025-06-27 14:38 ` Jorge Marques
2025-06-27 14:49 ` Krzysztof Kozlowski
0 siblings, 1 reply; 17+ messages in thread
From: Jorge Marques @ 2025-06-27 14:38 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Jorge Marques, Alexandre Belloni, Frank Li, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-i3c, devicetree,
linux-kernel
On Fri, Jun 27, 2025 at 08:56:55AM +0200, Krzysztof Kozlowski wrote:
> On Thu, Jun 26, 2025 at 12:07:36PM +0200, Jorge Marques wrote:
> > Add bindings doc for ADI I3C Controller IP core, a FPGA synthesizable IP
> > core that implements the MIPI I3C Basic controller specification.
>
> How did you resolve my last comment? I don't see any explanation -
> neither here nor in the binding description. Binding description is
> actually better place, I think now.
>
> Best regards,
> Krzysztof
>
Hi Krzysztof,
I forgot to condense out discussion on v4.
What about this binding description:
description: |
FPGA-based I3C controller designed to interface with I3C and I2C
peripherals, implementing a subset of the I3C-basic specification.
The IP core is tested on arm, microblaze, and arm64 architectures.
It takes one or two clocks, axi and i3c. If only axi is provided,
then there is no clock signal to the i3c input clock pin and axi
clock drives the whole IP. The compatible is suffixed by 1.00.a
foreseeing future controllers by Analog Devices Inc. and breaking
changes.
https://analogdevicesinc.github.io/hdl/library/i3c_controller
Further, deeper descriptions are available at the linked doc.
Best regards,
Jorge
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: i3c: Add adi-i3c-master
2025-06-27 14:38 ` Jorge Marques
@ 2025-06-27 14:49 ` Krzysztof Kozlowski
2025-06-27 16:02 ` Jorge Marques
0 siblings, 1 reply; 17+ messages in thread
From: Krzysztof Kozlowski @ 2025-06-27 14:49 UTC (permalink / raw)
To: Jorge Marques
Cc: Jorge Marques, Alexandre Belloni, Frank Li, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-i3c, devicetree,
linux-kernel
On 27/06/2025 16:38, Jorge Marques wrote:
> On Fri, Jun 27, 2025 at 08:56:55AM +0200, Krzysztof Kozlowski wrote:
>> On Thu, Jun 26, 2025 at 12:07:36PM +0200, Jorge Marques wrote:
>>> Add bindings doc for ADI I3C Controller IP core, a FPGA synthesizable IP
>>> core that implements the MIPI I3C Basic controller specification.
>>
>> How did you resolve my last comment? I don't see any explanation -
>> neither here nor in the binding description. Binding description is
>> actually better place, I think now.
>>
>> Best regards,
>> Krzysztof
>>
>
> Hi Krzysztof,
>
> I forgot to condense out discussion on v4.
> What about this binding description:
>
> description: |
> FPGA-based I3C controller designed to interface with I3C and I2C
> peripherals, implementing a subset of the I3C-basic specification.
> The IP core is tested on arm, microblaze, and arm64 architectures.
> It takes one or two clocks, axi and i3c. If only axi is provided,
> then there is no clock signal to the i3c input clock pin and axi
This is obvious from the schema, drop.
> clock drives the whole IP. The compatible is suffixed by 1.00.a
> foreseeing future controllers by Analog Devices Inc. and breaking
> changes.
I don't understand that. How are you breaking any changes? And how
1.00.a predicts future? I don't think this reflects previous discussion.
Why you were asked to go with v1.00.a?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: i3c: Add adi-i3c-master
2025-06-27 14:49 ` Krzysztof Kozlowski
@ 2025-06-27 16:02 ` Jorge Marques
2025-07-02 10:58 ` Jorge Marques
0 siblings, 1 reply; 17+ messages in thread
From: Jorge Marques @ 2025-06-27 16:02 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Jorge Marques, Alexandre Belloni, Frank Li, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-i3c, devicetree,
linux-kernel
On Fri, Jun 27, 2025 at 04:49:19PM +0200, Krzysztof Kozlowski wrote:
> On 27/06/2025 16:38, Jorge Marques wrote:
> > On Fri, Jun 27, 2025 at 08:56:55AM +0200, Krzysztof Kozlowski wrote:
> >> On Thu, Jun 26, 2025 at 12:07:36PM +0200, Jorge Marques wrote:
> >>> Add bindings doc for ADI I3C Controller IP core, a FPGA synthesizable IP
> >>> core that implements the MIPI I3C Basic controller specification.
> >>
> >> How did you resolve my last comment? I don't see any explanation -
> >> neither here nor in the binding description. Binding description is
> >> actually better place, I think now.
> >>
> >> Best regards,
> >> Krzysztof
> >>
> >
> > Hi Krzysztof,
> >
> > I forgot to condense out discussion on v4.
> > What about this binding description:
> >
> > description: |
> > FPGA-based I3C controller designed to interface with I3C and I2C
> > peripherals, implementing a subset of the I3C-basic specification.
> > The IP core is tested on arm, microblaze, and arm64 architectures.
> > It takes one or two clocks, axi and i3c. If only axi is provided,
> > then there is no clock signal to the i3c input clock pin and axi
>
> This is obvious from the schema, drop.
Ack.
>
> > clock drives the whole IP. The compatible is suffixed by 1.00.a
> > foreseeing future controllers by Analog Devices Inc. and breaking
> > changes.
>
> I don't understand that. How are you breaking any changes? And how
> 1.00.a predicts future? I don't think this reflects previous discussion.
> Why you were asked to go with v1.00.a?
The -1.00.a suffix came from this discussion:
https://lore.kernel.org/linux-i3c/ildi2pup2zkyv4stuknkrjysex3yzsbrrsrwbgcc4xgvdhwrdd@7qh4y6mutgy2/
Other adi bindings use this suffix. I personally wouldn't add any suffix
unless told otherwise, as I expressed on the thread. Should I drop it?
or suffix it with something else?
>
> Best regards,
> Krzysztof
Best regards,
Jorge
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: i3c: Add adi-i3c-master
2025-06-27 16:02 ` Jorge Marques
@ 2025-07-02 10:58 ` Jorge Marques
2025-07-02 11:38 ` Conor Dooley
2025-07-02 14:23 ` Marcelo Schmitt
0 siblings, 2 replies; 17+ messages in thread
From: Jorge Marques @ 2025-07-02 10:58 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Jorge Marques, Alexandre Belloni, Frank Li, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-i3c, devicetree,
linux-kernel
On Fri, Jun 27, 2025 at 06:02:26PM +0200, Jorge Marques wrote:
> On Fri, Jun 27, 2025 at 04:49:19PM +0200, Krzysztof Kozlowski wrote:
> > On 27/06/2025 16:38, Jorge Marques wrote:
> > > On Fri, Jun 27, 2025 at 08:56:55AM +0200, Krzysztof Kozlowski wrote:
> > >> On Thu, Jun 26, 2025 at 12:07:36PM +0200, Jorge Marques wrote:
> > >>> Add bindings doc for ADI I3C Controller IP core, a FPGA synthesizable IP
> > >>> core that implements the MIPI I3C Basic controller specification.
> > >>
> > >> How did you resolve my last comment? I don't see any explanation -
> > >> neither here nor in the binding description. Binding description is
> > >> actually better place, I think now.
> > >>
> > >> Best regards,
> > >> Krzysztof
> > >>
> > >
> > > Hi Krzysztof,
> > >
> > > I forgot to condense out discussion on v4.
> > > What about this binding description:
> > >
> > > description: |
> > > FPGA-based I3C controller designed to interface with I3C and I2C
> > > peripherals, implementing a subset of the I3C-basic specification.
> > > The IP core is tested on arm, microblaze, and arm64 architectures.
> > > It takes one or two clocks, axi and i3c. If only axi is provided,
> > > then there is no clock signal to the i3c input clock pin and axi
> >
> > This is obvious from the schema, drop.
> Ack.
>
> >
> > > clock drives the whole IP. The compatible is suffixed by 1.00.a
> > > foreseeing future controllers by Analog Devices Inc. and breaking
> > > changes.
> >
> > I don't understand that. How are you breaking any changes? And how
> > 1.00.a predicts future? I don't think this reflects previous discussion.
> > Why you were asked to go with v1.00.a?
> The -1.00.a suffix came from this discussion:
>
> https://lore.kernel.org/linux-i3c/ildi2pup2zkyv4stuknkrjysex3yzsbrrsrwbgcc4xgvdhwrdd@7qh4y6mutgy2/
>
> Other adi bindings use this suffix. I personally wouldn't add any suffix
> unless told otherwise, as I expressed on the thread. Should I drop it?
> or suffix it with something else?
> >
> > Best regards,
> > Krzysztof
>
> Best regards,
> Jorge
Hi Krzysztof and Conor,
I went after the reason of the historically -1.00.a suffix and
discovered that they came into existence due to AMD Xilinx auto
generation devicetree tool SDTGen
https://github.com/Xilinx/system-device-tree-xlnx
that would automatically suffix with the IP version, defaulting to 1.00.a,
and for a seamless experience, the were copied over to the dt-bindings.
The adi,axi-pwmgen dt-binding went more creative and suffixed with
-2.00.a, while never enforcing Vivado to yield the devicetree with such
value (Major version is asserted in the driver through reg access, the
current core version is v2.1.1)
Testing on my side (AMD Xilinx Vivado 2024.2), it seems Vivado now
defaults to 1.0, so the previous bindings from the other IPs are not
accurate anymore, either, (axi-pwmgen auto gens
`compatible = "xlnx,axi-pwm-gen-1.0";` (`xlnx` instead of `adi`, also)).
For fun, the current Vivado version thinks the devicetree node for the
i3c master should be as follows:
i3c_host_interface: i3c_controller_host_interface@44a00000 {
compatible = "xlnx,i3c-controller-host-interface-1.0";
reg = <0x44a00000 0x10000>;
clocks = <&clkc 15>;
clock-names = "s_axi_aclk";
};
Let me know if we can drop the suffix, or replace with something else.
The current register defined core version is v0.1.0.
Best regards,
Jorge
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: i3c: Add adi-i3c-master
2025-07-02 10:58 ` Jorge Marques
@ 2025-07-02 11:38 ` Conor Dooley
2025-07-11 8:14 ` Jorge Marques
2025-07-02 14:23 ` Marcelo Schmitt
1 sibling, 1 reply; 17+ messages in thread
From: Conor Dooley @ 2025-07-02 11:38 UTC (permalink / raw)
To: Jorge Marques
Cc: Krzysztof Kozlowski, Jorge Marques, Alexandre Belloni, Frank Li,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-i3c,
devicetree, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 4195 bytes --]
On Wed, Jul 02, 2025 at 12:58:00PM +0200, Jorge Marques wrote:
> On Fri, Jun 27, 2025 at 06:02:26PM +0200, Jorge Marques wrote:
> > On Fri, Jun 27, 2025 at 04:49:19PM +0200, Krzysztof Kozlowski wrote:
> > > On 27/06/2025 16:38, Jorge Marques wrote:
> > > > On Fri, Jun 27, 2025 at 08:56:55AM +0200, Krzysztof Kozlowski wrote:
> > > >> On Thu, Jun 26, 2025 at 12:07:36PM +0200, Jorge Marques wrote:
> > > >>> Add bindings doc for ADI I3C Controller IP core, a FPGA synthesizable IP
> > > >>> core that implements the MIPI I3C Basic controller specification.
> > > >>
> > > >> How did you resolve my last comment? I don't see any explanation -
> > > >> neither here nor in the binding description. Binding description is
> > > >> actually better place, I think now.
> > > >>
> > > >> Best regards,
> > > >> Krzysztof
> > > >>
> > > >
> > > > Hi Krzysztof,
> > > >
> > > > I forgot to condense out discussion on v4.
> > > > What about this binding description:
> > > >
> > > > description: |
> > > > FPGA-based I3C controller designed to interface with I3C and I2C
> > > > peripherals, implementing a subset of the I3C-basic specification.
> > > > The IP core is tested on arm, microblaze, and arm64 architectures.
> > > > It takes one or two clocks, axi and i3c. If only axi is provided,
> > > > then there is no clock signal to the i3c input clock pin and axi
> > >
> > > This is obvious from the schema, drop.
> > Ack.
> >
> > >
> > > > clock drives the whole IP. The compatible is suffixed by 1.00.a
> > > > foreseeing future controllers by Analog Devices Inc. and breaking
> > > > changes.
> > >
> > > I don't understand that. How are you breaking any changes? And how
> > > 1.00.a predicts future? I don't think this reflects previous discussion.
> > > Why you were asked to go with v1.00.a?
> > The -1.00.a suffix came from this discussion:
> >
> > https://lore.kernel.org/linux-i3c/ildi2pup2zkyv4stuknkrjysex3yzsbrrsrwbgcc4xgvdhwrdd@7qh4y6mutgy2/
> >
> > Other adi bindings use this suffix. I personally wouldn't add any suffix
> > unless told otherwise, as I expressed on the thread. Should I drop it?
> > or suffix it with something else?
>
> I went after the reason of the historically -1.00.a suffix and
> discovered that they came into existence due to AMD Xilinx auto
> generation devicetree tool SDTGen
> https://github.com/Xilinx/system-device-tree-xlnx
> that would automatically suffix with the IP version, defaulting to 1.00.a,
> and for a seamless experience, the were copied over to the dt-bindings.
>
> The adi,axi-pwmgen dt-binding went more creative and suffixed with
> -2.00.a, while never enforcing Vivado to yield the devicetree with such
> value (Major version is asserted in the driver through reg access, the
> current core version is v2.1.1)
>
> Testing on my side (AMD Xilinx Vivado 2024.2), it seems Vivado now
> defaults to 1.0, so the previous bindings from the other IPs are not
> accurate anymore, either, (axi-pwmgen auto gens
> `compatible = "xlnx,axi-pwm-gen-1.0";` (`xlnx` instead of `adi`, also)).
>
> For fun, the current Vivado version thinks the devicetree node for the
> i3c master should be as follows:
>
> i3c_host_interface: i3c_controller_host_interface@44a00000 {
> compatible = "xlnx,i3c-controller-host-interface-1.0";
> reg = <0x44a00000 0x10000>;
> clocks = <&clkc 15>;
> clock-names = "s_axi_aclk";
> };
>
> Let me know if we can drop the suffix, or replace with something else.
> The current register defined core version is v0.1.0.
Please, as I requested in the original thread, add versioning
information. I'm clearly not asking you to make up some garbage version
number, or to add "1.00a" if that's not an accurate version (and never
did AFAICT). If 0.1.0 is the version of the IP core, I'd like you to use
that.
If other ADI IP cores are using crap version numbers that came from
some Xilinx tooling that don't represent the actual versions of the IPs,
that's probably something you should mention to your colleagues that
maintain those bindings.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: i3c: Add adi-i3c-master
2025-07-02 10:58 ` Jorge Marques
2025-07-02 11:38 ` Conor Dooley
@ 2025-07-02 14:23 ` Marcelo Schmitt
2025-07-02 15:04 ` Conor Dooley
1 sibling, 1 reply; 17+ messages in thread
From: Marcelo Schmitt @ 2025-07-02 14:23 UTC (permalink / raw)
To: Jorge Marques
Cc: Krzysztof Kozlowski, Jorge Marques, Alexandre Belloni, Frank Li,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-i3c,
devicetree, linux-kernel
Hello,
On 07/02, Jorge Marques wrote:
> On Fri, Jun 27, 2025 at 06:02:26PM +0200, Jorge Marques wrote:
> > On Fri, Jun 27, 2025 at 04:49:19PM +0200, Krzysztof Kozlowski wrote:
> > > On 27/06/2025 16:38, Jorge Marques wrote:
> > > > On Fri, Jun 27, 2025 at 08:56:55AM +0200, Krzysztof Kozlowski wrote:
> > > >> On Thu, Jun 26, 2025 at 12:07:36PM +0200, Jorge Marques wrote:
> > > >>> Add bindings doc for ADI I3C Controller IP core, a FPGA synthesizable IP
> > > >>> core that implements the MIPI I3C Basic controller specification.
> > > >>
...
>
> I went after the reason of the historically -1.00.a suffix and
> discovered that they came into existence due to AMD Xilinx auto
> generation devicetree tool SDTGen
> https://github.com/Xilinx/system-device-tree-xlnx
> that would automatically suffix with the IP version, defaulting to 1.00.a,
> and for a seamless experience, the were copied over to the dt-bindings.
>
> The adi,axi-pwmgen dt-binding went more creative and suffixed with
> -2.00.a, while never enforcing Vivado to yield the devicetree with such
> value (Major version is asserted in the driver through reg access, the
> current core version is v2.1.1)
>
> Testing on my side (AMD Xilinx Vivado 2024.2), it seems Vivado now
> defaults to 1.0, so the previous bindings from the other IPs are not
> accurate anymore, either, (axi-pwmgen auto gens
> `compatible = "xlnx,axi-pwm-gen-1.0";` (`xlnx` instead of `adi`, also)).
>
> For fun, the current Vivado version thinks the devicetree node for the
> i3c master should be as follows:
>
> i3c_host_interface: i3c_controller_host_interface@44a00000 {
> compatible = "xlnx,i3c-controller-host-interface-1.0";
> reg = <0x44a00000 0x10000>;
> clocks = <&clkc 15>;
> clock-names = "s_axi_aclk";
> };
>
> Let me know if we can drop the suffix, or replace with something else.
> The current register defined core version is v0.1.0.
I have no say in how dt-bindings should be, but having the IP version number
automatically generated doesn't sound like a sane thing to do IMHO. Maybe try to
standardize with -1.00.a suffix? Though one could also argue about dropping such
suffixes. Since we were told that the convention is to have the file name to
follow the compatible, I would go with 'xlnx,i3c-controller-host-interface'
(without any suffix) so the file name is smaller and clearer IMHO. dt maintainers
might prefer that in a different way.
Best regards,
Marcelo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: i3c: Add adi-i3c-master
2025-07-02 14:23 ` Marcelo Schmitt
@ 2025-07-02 15:04 ` Conor Dooley
0 siblings, 0 replies; 17+ messages in thread
From: Conor Dooley @ 2025-07-02 15:04 UTC (permalink / raw)
To: Marcelo Schmitt
Cc: Jorge Marques, Krzysztof Kozlowski, Jorge Marques,
Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-i3c, devicetree, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2979 bytes --]
On Wed, Jul 02, 2025 at 11:23:43AM -0300, Marcelo Schmitt wrote:
> Hello,
>
> On 07/02, Jorge Marques wrote:
> > On Fri, Jun 27, 2025 at 06:02:26PM +0200, Jorge Marques wrote:
> > > On Fri, Jun 27, 2025 at 04:49:19PM +0200, Krzysztof Kozlowski wrote:
> > > > On 27/06/2025 16:38, Jorge Marques wrote:
> > > > > On Fri, Jun 27, 2025 at 08:56:55AM +0200, Krzysztof Kozlowski wrote:
> > > > >> On Thu, Jun 26, 2025 at 12:07:36PM +0200, Jorge Marques wrote:
> > > > >>> Add bindings doc for ADI I3C Controller IP core, a FPGA synthesizable IP
> > > > >>> core that implements the MIPI I3C Basic controller specification.
> > > > >>
> ...
> >
> > I went after the reason of the historically -1.00.a suffix and
> > discovered that they came into existence due to AMD Xilinx auto
> > generation devicetree tool SDTGen
> > https://github.com/Xilinx/system-device-tree-xlnx
> > that would automatically suffix with the IP version, defaulting to 1.00.a,
> > and for a seamless experience, the were copied over to the dt-bindings.
> >
> > The adi,axi-pwmgen dt-binding went more creative and suffixed with
> > -2.00.a, while never enforcing Vivado to yield the devicetree with such
> > value (Major version is asserted in the driver through reg access, the
> > current core version is v2.1.1)
> >
> > Testing on my side (AMD Xilinx Vivado 2024.2), it seems Vivado now
> > defaults to 1.0, so the previous bindings from the other IPs are not
> > accurate anymore, either, (axi-pwmgen auto gens
> > `compatible = "xlnx,axi-pwm-gen-1.0";` (`xlnx` instead of `adi`, also)).
> >
> > For fun, the current Vivado version thinks the devicetree node for the
> > i3c master should be as follows:
> >
> > i3c_host_interface: i3c_controller_host_interface@44a00000 {
> > compatible = "xlnx,i3c-controller-host-interface-1.0";
> > reg = <0x44a00000 0x10000>;
> > clocks = <&clkc 15>;
> > clock-names = "s_axi_aclk";
> > };
> >
> > Let me know if we can drop the suffix, or replace with something else.
> > The current register defined core version is v0.1.0.
>
> I have no say in how dt-bindings should be, but having the IP version number
> automatically generated doesn't sound like a sane thing to do IMHO. Maybe try to
> standardize with -1.00.a suffix? Though one could also argue about dropping such
> suffixes. Since we were told that the convention is to have the file name to
> follow the compatible, I would go with 'xlnx,i3c-controller-host-interface'
> (without any suffix) so the file name is smaller and clearer IMHO. dt maintainers
> might prefer that in a different way.
To my knowledge this is an ADI IP from their HDL catalogue:
https://github.com/analogdevicesinc/hdl/tree/main/library/i3c_controller
I see no reason why it should have an "xlnx" vendor prefix - and I don't
think Jorge was seriously suggesting it either, given they mentioned it
was being shared "for fun".
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: i3c: Add adi-i3c-master
2025-07-02 11:38 ` Conor Dooley
@ 2025-07-11 8:14 ` Jorge Marques
2025-07-11 16:28 ` Conor Dooley
0 siblings, 1 reply; 17+ messages in thread
From: Jorge Marques @ 2025-07-11 8:14 UTC (permalink / raw)
To: Conor Dooley
Cc: Krzysztof Kozlowski, Devid Lechner, Adrian Costina, Jorge Marques,
Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-i3c, devicetree, linux-kernel
On Wed, Jul 02, 2025 at 12:38:41PM +0100, Conor Dooley wrote:
> On Wed, Jul 02, 2025 at 12:58:00PM +0200, Jorge Marques wrote:
> > On Fri, Jun 27, 2025 at 06:02:26PM +0200, Jorge Marques wrote:
> > > On Fri, Jun 27, 2025 at 04:49:19PM +0200, Krzysztof Kozlowski wrote:
> > > > On 27/06/2025 16:38, Jorge Marques wrote:
> > > > > On Fri, Jun 27, 2025 at 08:56:55AM +0200, Krzysztof Kozlowski wrote:
> > > > >> On Thu, Jun 26, 2025 at 12:07:36PM +0200, Jorge Marques wrote:
> > > > >>> Add bindings doc for ADI I3C Controller IP core, a FPGA synthesizable IP
> > > > >>> core that implements the MIPI I3C Basic controller specification.
> > > > >>
> > > > >> How did you resolve my last comment? I don't see any explanation -
> > > > >> neither here nor in the binding description. Binding description is
> > > > >> actually better place, I think now.
> > > > >>
> > > > >> Best regards,
> > > > >> Krzysztof
> > > > >>
> > > > >
> > > > > Hi Krzysztof,
> > > > >
> > > > > I forgot to condense out discussion on v4.
> > > > > What about this binding description:
> > > > >
> > > > > description: |
> > > > > FPGA-based I3C controller designed to interface with I3C and I2C
> > > > > peripherals, implementing a subset of the I3C-basic specification.
> > > > > The IP core is tested on arm, microblaze, and arm64 architectures.
> > > > > It takes one or two clocks, axi and i3c. If only axi is provided,
> > > > > then there is no clock signal to the i3c input clock pin and axi
> > > >
> > > > This is obvious from the schema, drop.
> > > Ack.
> > >
> > > >
> > > > > clock drives the whole IP. The compatible is suffixed by 1.00.a
> > > > > foreseeing future controllers by Analog Devices Inc. and breaking
> > > > > changes.
> > > >
> > > > I don't understand that. How are you breaking any changes? And how
> > > > 1.00.a predicts future? I don't think this reflects previous discussion.
> > > > Why you were asked to go with v1.00.a?
> > > The -1.00.a suffix came from this discussion:
> > >
> > > https://lore.kernel.org/linux-i3c/ildi2pup2zkyv4stuknkrjysex3yzsbrrsrwbgcc4xgvdhwrdd@7qh4y6mutgy2/
> > >
> > > Other adi bindings use this suffix. I personally wouldn't add any suffix
> > > unless told otherwise, as I expressed on the thread. Should I drop it?
> > > or suffix it with something else?
>
> >
> > I went after the reason of the historically -1.00.a suffix and
> > discovered that they came into existence due to AMD Xilinx auto
> > generation devicetree tool SDTGen
> > https://github.com/Xilinx/system-device-tree-xlnx
> > that would automatically suffix with the IP version, defaulting to 1.00.a,
> > and for a seamless experience, the were copied over to the dt-bindings.
> >
> > The adi,axi-pwmgen dt-binding went more creative and suffixed with
> > -2.00.a, while never enforcing Vivado to yield the devicetree with such
> > value (Major version is asserted in the driver through reg access, the
> > current core version is v2.1.1)
> >
> > Testing on my side (AMD Xilinx Vivado 2024.2), it seems Vivado now
> > defaults to 1.0, so the previous bindings from the other IPs are not
> > accurate anymore, either, (axi-pwmgen auto gens
> > `compatible = "xlnx,axi-pwm-gen-1.0";` (`xlnx` instead of `adi`, also)).
> >
> > For fun, the current Vivado version thinks the devicetree node for the
> > i3c master should be as follows:
> >
> > i3c_host_interface: i3c_controller_host_interface@44a00000 {
> > compatible = "xlnx,i3c-controller-host-interface-1.0";
> > reg = <0x44a00000 0x10000>;
> > clocks = <&clkc 15>;
> > clock-names = "s_axi_aclk";
> > };
> >
> > Let me know if we can drop the suffix, or replace with something else.
> > The current register defined core version is v0.1.0.
>
> Please, as I requested in the original thread, add versioning
> information. I'm clearly not asking you to make up some garbage version
> number, or to add "1.00a" if that's not an accurate version (and never
> did AFAICT). If 0.1.0 is the version of the IP core, I'd like you to use
> that.
>
> If other ADI IP cores are using crap version numbers that came from
> some Xilinx tooling that don't represent the actual versions of the IPs,
> that's probably something you should mention to your colleagues that
> maintain those bindings.
Hi Conor,
My proposal (1) for the Analog Devices Inc. open source IP Cores is to
migrate to semantic versioning.
(1) https://github.com/analogdevicesinc/hdl/pull/1831
The dt-binding must end with the major value, with the first supported
version starting at 1. Therefore, the binding will be:
* adi,i3c-master-1
At driver level, the debug version value is `v%u.%u.%u`, and the driver
shall check the minor version value for handling feature changes. While
the major bump is restricted for breaking changes, which, in some cases,
also generates a new dt-binding.
For backwards compatibility, existing ADI IP Cores that prints `v%u.%u.%c`
should print `v%u.%u.%u` instead, but keep their current value (e.g.,
`a`->`97`), until the minor is bumped, when the patch version resets to 0.
Rework across register accessed IPs is being handled here:
https://github.com/analogdevicesinc/hdl/pull/1829
There are more than 50 IPs with register access, so I need to be cautious.
What is your take on this? Does it resolve the bindings concern?
Adding David to the thread, since the axi-spi-engine already moved to
`v%u.%u.%u` format.
Best regards,
Jorge
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: i3c: Add adi-i3c-master
2025-07-11 8:14 ` Jorge Marques
@ 2025-07-11 16:28 ` Conor Dooley
2025-07-14 9:53 ` Jorge Marques
0 siblings, 1 reply; 17+ messages in thread
From: Conor Dooley @ 2025-07-11 16:28 UTC (permalink / raw)
To: Jorge Marques
Cc: Krzysztof Kozlowski, Devid Lechner, Adrian Costina, Jorge Marques,
Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-i3c, devicetree, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 7065 bytes --]
On Fri, Jul 11, 2025 at 10:14:45AM +0200, Jorge Marques wrote:
> On Wed, Jul 02, 2025 at 12:38:41PM +0100, Conor Dooley wrote:
> > On Wed, Jul 02, 2025 at 12:58:00PM +0200, Jorge Marques wrote:
> > > On Fri, Jun 27, 2025 at 06:02:26PM +0200, Jorge Marques wrote:
> > > > On Fri, Jun 27, 2025 at 04:49:19PM +0200, Krzysztof Kozlowski wrote:
> > > > > On 27/06/2025 16:38, Jorge Marques wrote:
> > > > > > On Fri, Jun 27, 2025 at 08:56:55AM +0200, Krzysztof Kozlowski wrote:
> > > > > >> On Thu, Jun 26, 2025 at 12:07:36PM +0200, Jorge Marques wrote:
> > > > > >>> Add bindings doc for ADI I3C Controller IP core, a FPGA synthesizable IP
> > > > > >>> core that implements the MIPI I3C Basic controller specification.
> > > > > >>
> > > > > >> How did you resolve my last comment? I don't see any explanation -
> > > > > >> neither here nor in the binding description. Binding description is
> > > > > >> actually better place, I think now.
> > > > > >>
> > > > > >> Best regards,
> > > > > >> Krzysztof
> > > > > >>
> > > > > >
> > > > > > Hi Krzysztof,
> > > > > >
> > > > > > I forgot to condense out discussion on v4.
> > > > > > What about this binding description:
> > > > > >
> > > > > > description: |
> > > > > > FPGA-based I3C controller designed to interface with I3C and I2C
> > > > > > peripherals, implementing a subset of the I3C-basic specification.
> > > > > > The IP core is tested on arm, microblaze, and arm64 architectures.
> > > > > > It takes one or two clocks, axi and i3c. If only axi is provided,
> > > > > > then there is no clock signal to the i3c input clock pin and axi
> > > > >
> > > > > This is obvious from the schema, drop.
> > > > Ack.
> > > >
> > > > >
> > > > > > clock drives the whole IP. The compatible is suffixed by 1.00.a
> > > > > > foreseeing future controllers by Analog Devices Inc. and breaking
> > > > > > changes.
> > > > >
> > > > > I don't understand that. How are you breaking any changes? And how
> > > > > 1.00.a predicts future? I don't think this reflects previous discussion.
> > > > > Why you were asked to go with v1.00.a?
> > > > The -1.00.a suffix came from this discussion:
> > > >
> > > > https://lore.kernel.org/linux-i3c/ildi2pup2zkyv4stuknkrjysex3yzsbrrsrwbgcc4xgvdhwrdd@7qh4y6mutgy2/
> > > >
> > > > Other adi bindings use this suffix. I personally wouldn't add any suffix
> > > > unless told otherwise, as I expressed on the thread. Should I drop it?
> > > > or suffix it with something else?
> >
> > >
> > > I went after the reason of the historically -1.00.a suffix and
> > > discovered that they came into existence due to AMD Xilinx auto
> > > generation devicetree tool SDTGen
> > > https://github.com/Xilinx/system-device-tree-xlnx
> > > that would automatically suffix with the IP version, defaulting to 1.00.a,
> > > and for a seamless experience, the were copied over to the dt-bindings.
> > >
> > > The adi,axi-pwmgen dt-binding went more creative and suffixed with
> > > -2.00.a, while never enforcing Vivado to yield the devicetree with such
> > > value (Major version is asserted in the driver through reg access, the
> > > current core version is v2.1.1)
> > >
> > > Testing on my side (AMD Xilinx Vivado 2024.2), it seems Vivado now
> > > defaults to 1.0, so the previous bindings from the other IPs are not
> > > accurate anymore, either, (axi-pwmgen auto gens
> > > `compatible = "xlnx,axi-pwm-gen-1.0";` (`xlnx` instead of `adi`, also)).
> > >
> > > For fun, the current Vivado version thinks the devicetree node for the
> > > i3c master should be as follows:
> > >
> > > i3c_host_interface: i3c_controller_host_interface@44a00000 {
> > > compatible = "xlnx,i3c-controller-host-interface-1.0";
> > > reg = <0x44a00000 0x10000>;
> > > clocks = <&clkc 15>;
> > > clock-names = "s_axi_aclk";
> > > };
> > >
> > > Let me know if we can drop the suffix, or replace with something else.
> > > The current register defined core version is v0.1.0.
> >
> > Please, as I requested in the original thread, add versioning
> > information. I'm clearly not asking you to make up some garbage version
> > number, or to add "1.00a" if that's not an accurate version (and never
> > did AFAICT). If 0.1.0 is the version of the IP core, I'd like you to use
> > that.
> >
> > If other ADI IP cores are using crap version numbers that came from
> > some Xilinx tooling that don't represent the actual versions of the IPs,
> > that's probably something you should mention to your colleagues that
> > maintain those bindings.
>
>
> My proposal (1) for the Analog Devices Inc. open source IP Cores is to
> migrate to semantic versioning.
> (1) https://github.com/analogdevicesinc/hdl/pull/1831
>
> The dt-binding must end with the major value, with the first supported
> version starting at 1. Therefore, the binding will be:
>
> * adi,i3c-master-1
>
> At driver level, the debug version value is `v%u.%u.%u`, and the driver
> shall check the minor version value for handling feature changes. While
> the major bump is restricted for breaking changes, which, in some cases,
> also generates a new dt-binding.
>
> For backwards compatibility, existing ADI IP Cores that prints `v%u.%u.%c`
> should print `v%u.%u.%u` instead, but keep their current value (e.g.,
> `a`->`97`), until the minor is bumped, when the patch version resets to 0.
>
> Rework across register accessed IPs is being handled here:
> https://github.com/analogdevicesinc/hdl/pull/1829
> There are more than 50 IPs with register access, so I need to be cautious.
>
> What is your take on this? Does it resolve the bindings concern?
I think it is a good idea to establish version numbers that actually
have a meaning, rather than using something garbage. I think you should
probably put the "v" into the number in the compatible - 1.00 or 1.00.a
is fairly clearly a version number but "-1" is less clear.
Also "the binding must end with the major value" is a bit of an odd
requirement I think. The sort of situations that may cause different
compatibles to be introduced are not likely to be restricted to majors.
That may be something like properties only valid for some versions, or
at worst a bug that requires checking the compatible (even if these
are much rarer when there's a version register, they're still possible).
IDM what number you opt for in the end either, it could well just be the
lowest number where the current register offset/format for the version
number register exists.
It's a bit hypocritical of me to ask this, since I have not yet written
one for the Microchip stuff - but I think you should document whatever
numbering you end up on, like in xilinx.txt and
sifive-blocks-ip-versioning.txt, so that we can point people at it.
>
> Adding David to the thread, since the axi-spi-engine already moved to
> `v%u.%u.%u` format.
>
> Best regards,
> Jorge
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: i3c: Add adi-i3c-master
2025-07-11 16:28 ` Conor Dooley
@ 2025-07-14 9:53 ` Jorge Marques
0 siblings, 0 replies; 17+ messages in thread
From: Jorge Marques @ 2025-07-14 9:53 UTC (permalink / raw)
To: Conor Dooley
Cc: Krzysztof Kozlowski, Devid Lechner, Adrian Costina, Jorge Marques,
Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-i3c, devicetree, linux-kernel
On Fri, Jul 11, 2025 at 05:28:57PM +0100, Conor Dooley wrote:
> On Fri, Jul 11, 2025 at 10:14:45AM +0200, Jorge Marques wrote:
> > On Wed, Jul 02, 2025 at 12:38:41PM +0100, Conor Dooley wrote:
> > > On Wed, Jul 02, 2025 at 12:58:00PM +0200, Jorge Marques wrote:
> > > > On Fri, Jun 27, 2025 at 06:02:26PM +0200, Jorge Marques wrote:
> > > > > On Fri, Jun 27, 2025 at 04:49:19PM +0200, Krzysztof Kozlowski wrote:
> > > > > > On 27/06/2025 16:38, Jorge Marques wrote:
> > > > > > > On Fri, Jun 27, 2025 at 08:56:55AM +0200, Krzysztof Kozlowski wrote:
> > > > > > >> On Thu, Jun 26, 2025 at 12:07:36PM +0200, Jorge Marques wrote:
> > > > > > >>> Add bindings doc for ADI I3C Controller IP core, a FPGA synthesizable IP
> > > > > > >>> core that implements the MIPI I3C Basic controller specification.
> > > > > > >>
> > > > > > >> How did you resolve my last comment? I don't see any explanation -
> > > > > > >> neither here nor in the binding description. Binding description is
> > > > > > >> actually better place, I think now.
> > > > > > >>
> > > > > > >> Best regards,
> > > > > > >> Krzysztof
> > > > > > >>
> > > > > > >
> > > > > > > Hi Krzysztof,
> > > > > > >
> > > > > > > I forgot to condense out discussion on v4.
> > > > > > > What about this binding description:
> > > > > > >
> > > > > > > description: |
> > > > > > > FPGA-based I3C controller designed to interface with I3C and I2C
> > > > > > > peripherals, implementing a subset of the I3C-basic specification.
> > > > > > > The IP core is tested on arm, microblaze, and arm64 architectures.
> > > > > > > It takes one or two clocks, axi and i3c. If only axi is provided,
> > > > > > > then there is no clock signal to the i3c input clock pin and axi
> > > > > >
> > > > > > This is obvious from the schema, drop.
> > > > > Ack.
> > > > >
> > > > > >
> > > > > > > clock drives the whole IP. The compatible is suffixed by 1.00.a
> > > > > > > foreseeing future controllers by Analog Devices Inc. and breaking
> > > > > > > changes.
> > > > > >
> > > > > > I don't understand that. How are you breaking any changes? And how
> > > > > > 1.00.a predicts future? I don't think this reflects previous discussion.
> > > > > > Why you were asked to go with v1.00.a?
> > > > > The -1.00.a suffix came from this discussion:
> > > > >
> > > > > https://lore.kernel.org/linux-i3c/ildi2pup2zkyv4stuknkrjysex3yzsbrrsrwbgcc4xgvdhwrdd@7qh4y6mutgy2/
> > > > >
> > > > > Other adi bindings use this suffix. I personally wouldn't add any suffix
> > > > > unless told otherwise, as I expressed on the thread. Should I drop it?
> > > > > or suffix it with something else?
> > >
> > > >
> > > > I went after the reason of the historically -1.00.a suffix and
> > > > discovered that they came into existence due to AMD Xilinx auto
> > > > generation devicetree tool SDTGen
> > > > https://github.com/Xilinx/system-device-tree-xlnx
> > > > that would automatically suffix with the IP version, defaulting to 1.00.a,
> > > > and for a seamless experience, the were copied over to the dt-bindings.
> > > >
> > > > The adi,axi-pwmgen dt-binding went more creative and suffixed with
> > > > -2.00.a, while never enforcing Vivado to yield the devicetree with such
> > > > value (Major version is asserted in the driver through reg access, the
> > > > current core version is v2.1.1)
> > > >
> > > > Testing on my side (AMD Xilinx Vivado 2024.2), it seems Vivado now
> > > > defaults to 1.0, so the previous bindings from the other IPs are not
> > > > accurate anymore, either, (axi-pwmgen auto gens
> > > > `compatible = "xlnx,axi-pwm-gen-1.0";` (`xlnx` instead of `adi`, also)).
> > > >
> > > > For fun, the current Vivado version thinks the devicetree node for the
> > > > i3c master should be as follows:
> > > >
> > > > i3c_host_interface: i3c_controller_host_interface@44a00000 {
> > > > compatible = "xlnx,i3c-controller-host-interface-1.0";
> > > > reg = <0x44a00000 0x10000>;
> > > > clocks = <&clkc 15>;
> > > > clock-names = "s_axi_aclk";
> > > > };
> > > >
> > > > Let me know if we can drop the suffix, or replace with something else.
> > > > The current register defined core version is v0.1.0.
> > >
> > > Please, as I requested in the original thread, add versioning
> > > information. I'm clearly not asking you to make up some garbage version
> > > number, or to add "1.00a" if that's not an accurate version (and never
> > > did AFAICT). If 0.1.0 is the version of the IP core, I'd like you to use
> > > that.
> > >
> > > If other ADI IP cores are using crap version numbers that came from
> > > some Xilinx tooling that don't represent the actual versions of the IPs,
> > > that's probably something you should mention to your colleagues that
> > > maintain those bindings.
> >
> >
> > My proposal (1) for the Analog Devices Inc. open source IP Cores is to
> > migrate to semantic versioning.
> > (1) https://github.com/analogdevicesinc/hdl/pull/1831
> >
> > The dt-binding must end with the major value, with the first supported
> > version starting at 1. Therefore, the binding will be:
> >
> > * adi,i3c-master-1
> >
> > At driver level, the debug version value is `v%u.%u.%u`, and the driver
> > shall check the minor version value for handling feature changes. While
> > the major bump is restricted for breaking changes, which, in some cases,
> > also generates a new dt-binding.
> >
> > For backwards compatibility, existing ADI IP Cores that prints `v%u.%u.%c`
> > should print `v%u.%u.%u` instead, but keep their current value (e.g.,
> > `a`->`97`), until the minor is bumped, when the patch version resets to 0.
> >
> > Rework across register accessed IPs is being handled here:
> > https://github.com/analogdevicesinc/hdl/pull/1829
> > There are more than 50 IPs with register access, so I need to be cautious.
> >
> > What is your take on this? Does it resolve the bindings concern?
>
> I think it is a good idea to establish version numbers that actually
> have a meaning, rather than using something garbage. I think you should
> probably put the "v" into the number in the compatible - 1.00 or 1.00.a
> is fairly clearly a version number but "-1" is less clear.
>
Hi Conor,
The format that is currently winning my internal discussion is
`adi,<ip-name>-v<major>`.
And the next patch will be submitted as `adi,i3c-master-v1`.
> Also "the binding must end with the major value" is a bit of an odd
> requirement I think. The sort of situations that may cause different
> compatibles to be introduced are not likely to be restricted to majors.
sifive-blocks-ip-versioning.txt uses
"sifive,<ip-name><integer-version-number>", While the xilinx.txt uses
the last version that uses the same interface. Both implicitly hide away
features (minor) and fixes (patches). What I am enforcing is that, if
the interface changes, the major field must be bumped. In most cases,
everything else is minor matching at the driver level, and if exceptions
occur, add to the commit message the justification and that's it.
> That may be something like properties only valid for some versions, or
> at worst a bug that requires checking the compatible (even if these
> are much rarer when there's a version register, they're still possible).
>
> IDM what number you opt for in the end either, it could well just be the
> lowest number where the current register offset/format for the version
> number register exists.
>
> It's a bit hypocritical of me to ask this, since I have not yet written
> one for the Microchip stuff - but I think you should document whatever
> numbering you end up on, like in xilinx.txt and
> sifive-blocks-ip-versioning.txt, so that we can point people at it.
Best regards,
Jorge
> >
> > Adding David to the thread, since the axi-spi-engine already moved to
> > `v%u.%u.%u` format.
> >
> > Best regards,
> > Jorge
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2025-07-14 9:53 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-26 10:07 [PATCH v4 0/2] Add ADI I3C Controller Jorge Marques
2025-06-26 10:07 ` [PATCH v4 1/2] dt-bindings: i3c: Add adi-i3c-master Jorge Marques
2025-06-26 16:13 ` Frank Li
2025-06-27 6:56 ` Krzysztof Kozlowski
2025-06-27 14:38 ` Jorge Marques
2025-06-27 14:49 ` Krzysztof Kozlowski
2025-06-27 16:02 ` Jorge Marques
2025-07-02 10:58 ` Jorge Marques
2025-07-02 11:38 ` Conor Dooley
2025-07-11 8:14 ` Jorge Marques
2025-07-11 16:28 ` Conor Dooley
2025-07-14 9:53 ` Jorge Marques
2025-07-02 14:23 ` Marcelo Schmitt
2025-07-02 15:04 ` Conor Dooley
2025-06-26 10:07 ` [PATCH v4 2/2] i3c: master: Add driver for Analog Devices I3C Controller IP Jorge Marques
2025-06-26 16:51 ` Frank Li
2025-06-27 14:10 ` Jorge Marques
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).