[PATCH V7 0/4] Add AMD I3C master controller driver and bindings

[PATCH V7 0/4] Add AMD I3C master controller driver and bindings

[Manikanta Guntupalli]

This patch series introduces support for the AMD I3C master controller,
including the device tree binding and driver implementation.

In addition, the series adds big-endian MMIO accessors and extends
i3c_readl_fifo() and i3c_writel_fifo() helpers with endianness support.

Changes for V2:
Updated commit subject and description.
Moved allOf to after required.
Removed xlnx,num-targets property.
Added mixed mode support with clock configuration.
Converted smaller functions into inline functions.
Used FIELD_GET() in xi3c_get_response().
Updated xi3c_master_rd_from_rx_fifo() to use cmd->rx_buf.
Used parity8() for address parity calculation.
Added guards for locks.
Dropped num_targets and updated xi3c_master_do_daa().
Used __free(kfree) in xi3c_master_send_bdcast_ccc_cmd().
Dropped PM runtime support.
Updated xi3c_master_read() and xi3c_master_write() with
xi3c_is_resp_available() check.
Created separate functions: xi3c_master_init() and xi3c_master_reinit().
Used xi3c_master_init() in bus initialization and xi3c_master_reinit()
in error paths.
Added DAA structure to xi3c_master structure.

Changes for V3:
Updated commit description.
Corrected the order of properties and removed resets property.
Added compatible to required list.
Added interrupts to example.
Resolved merge conflicts.

Changes for V4:
Added h/w documentation details.
Updated timeout macros.
Removed type casting for xi3c_is_resp_available() macro.
Used ioread32() and iowrite32() instead of readl() and writel()
to keep consistency.
Read XI3C_RESET_OFFSET reg before udelay().
Removed xi3c_master_free_xfer() and directly used kfree().
Skipped checking return value of i3c_master_add_i3c_dev_locked().
Used devm_mutex_init() instead of mutex_init().

Changes for V5:
Renamed the xlnx,axi-i3c.yaml file into xlnx,axi-i3c-1.0.yaml.
Used GENMASK_ULL for PID mask as it's 64bit mask.

Changes for V6:
Corrected the $id in the YAML file to match the filename and fix
the dtschema warning.
Removed typecast for xi3c_getrevisionnumber(), xi3c_wrfifolevel(),
and xi3c_rdfifolevel().
Replaced dynamic allocation with a static variable for pid_bcr_dcr.
Fixed sparse warning in do_daa by typecasting the address parity value
to u8.
Fixed sparse warning in xi3c_master_bus_init by typecasting the pid value
to u64 in info.pid calculation.

Changes for V7:
Added i3c controller version details to commit description.
Added Reviewed-by tag to binding patch [1/4].
Added big-endian MMIO accessors [2/4].
Added endianness support for i3c_readl_fifo() and i3c_writel_fifo() [3/4].
Updated timeout macro name.
Updated xi3c_master_wr_to_tx_fifo() and xi3c_master_rd_from_rx_fifo()
to use i3c_writel_fifo() and i3c_readl_fifo().

Manikanta Guntupalli (4):
  dt-bindings: i3c: Add AMD I3C master controller support
  asm-generic/io.h: Add big-endian MMIO accessors
  i3c: master: Add endianness support for i3c_readl_fifo() and
    i3c_writel_fifo()
  i3c: master: Add AMD I3C bus controller driver

 .../bindings/i3c/xlnx,axi-i3c-1.0.yaml        |   55 +
 MAINTAINERS                                   |    7 +
 drivers/i3c/internals.h                       |   35 +-
 drivers/i3c/master/Kconfig                    |   16 +
 drivers/i3c/master/Makefile                   |    1 +
 drivers/i3c/master/amd-i3c-master.c           | 1012 +++++++++++++++++
 drivers/i3c/master/dw-i3c-master.c            |    9 +-
 drivers/i3c/master/i3c-master-cdns.c          |    9 +-
 drivers/i3c/master/renesas-i3c.c              |   12 +-
 include/asm-generic/io.h                      |  202 ++++
 10 files changed, 1342 insertions(+), 16 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i3c/xlnx,axi-i3c-1.0.yaml
 create mode 100644 drivers/i3c/master/amd-i3c-master.c

-- 
2.34.1


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH V7 1/4] dt-bindings: i3c: Add AMD I3C master controller support

[Manikanta Guntupalli]

Add device tree binding documentation for the AMD I3C master
controller version 1.0.

Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
Changes for V2:
Updated commit subject and description.
Moved allOf to after required.
Removed xlnx,num-targets property.

Changes for V3:
Updated commit description.
Corrected the order of properties and removed resets property.
Added compatible to required list.
Added interrupts to example.

Changes for V4:
Added h/w documentation details.

Changes for V5:
Renamed the xlnx,axi-i3c.yaml file into xlnx,axi-i3c-1.0.yaml.

Changes for V6:
Corrected the file name for $id in yaml to fix the dtschema warning.

Changes for V7:
Added i3c controller version details to commit description.
Added Reviewed-by tag.
---
 .../bindings/i3c/xlnx,axi-i3c-1.0.yaml        | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i3c/xlnx,axi-i3c-1.0.yaml

diff --git a/Documentation/devicetree/bindings/i3c/xlnx,axi-i3c-1.0.yaml b/Documentation/devicetree/bindings/i3c/xlnx,axi-i3c-1.0.yaml
new file mode 100644
index 000000000000..17c63807dbcf
--- /dev/null
+++ b/Documentation/devicetree/bindings/i3c/xlnx,axi-i3c-1.0.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i3c/xlnx,axi-i3c-1.0.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AMD I3C master
+
+maintainers:
+  - Manikanta Guntupalli <manikanta.guntupalli@amd.com>
+
+description:
+  The AXI-I3C IP is an I3C Controller with an AXI4-Lite interface, compatible
+  with the MIPI I3C Specification v1.1.1. The design includes bidirectional I/O
+  buffers that implement open collector drivers for the SDA and SCL signals.
+  External pull-up resistors are required to properly hold the bus at a Logic-1
+  level when the drivers are released.
+
+  For more details, please see https://docs.amd.com/r/en-US/pg439-axi-i3c
+
+properties:
+  compatible:
+    const: xlnx,axi-i3c-1.0
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+
+allOf:
+  - $ref: i3c.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i3c@80000000 {
+        compatible = "xlnx,axi-i3c-1.0";
+        reg = <0x80000000 0x10000>;
+        clocks = <&zynqmp_clk 71>;
+        interrupt-parent = <&imux>;
+        interrupts = <0 89 4>;
+        #address-cells = <3>;
+        #size-cells = <0>;
+    };
+...
-- 
2.34.1


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH V7 2/4] asm-generic/io.h: Add big-endian MMIO accessors

[Manikanta Guntupalli]

Add MMIO accessors to support big-endian memory operations. These helpers
include {read, write}{w, l, q}_be() and {read, write}s{w, l, q}_be(),
which allows to access big-endian memory regions while returning
the results in the CPU’s native endianness.

This provides a consistent interface to interact with hardware using
big-endian register layouts.

Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
---
Changes since V7:
This patch introduced in V7.
---
 include/asm-generic/io.h | 202 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 202 insertions(+)

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 11abad6c87e1..d18a8ca6c06c 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -295,6 +295,96 @@ static inline void writeq(u64 value, volatile void __iomem *addr)
 #endif
 #endif /* CONFIG_64BIT */
 
+/*
+ * {read,write}{w,l,q}_be() access big endian memory and return result
+ * in native endianness.
+ */
+
+#ifndef readw_be
+#define readw_be readw_be
+static inline u16 readw_be(const volatile void __iomem *addr)
+{
+	u16 val;
+
+	log_read_mmio(16, addr, _THIS_IP_, _RET_IP_);
+	__io_br();
+	val = __be16_to_cpu((__be16 __force)__raw_readw(addr));
+	__io_ar(val);
+	log_post_read_mmio(val, 16, addr, _THIS_IP_, _RET_IP_);
+	return val;
+}
+#endif
+
+#ifndef readl_be
+#define readl_be readl_be
+static inline u32 readl_be(const volatile void __iomem *addr)
+{
+	u32 val;
+
+	log_read_mmio(32, addr, _THIS_IP_, _RET_IP_);
+	__io_br();
+	val = __be32_to_cpu((__be32 __force)__raw_readl(addr));
+	__io_ar(val);
+	log_post_read_mmio(val, 32, addr, _THIS_IP_, _RET_IP_);
+	return val;
+}
+#endif
+
+#ifdef CONFIG_64BIT
+#ifndef readq_be
+#define readq_be readq_be
+static inline u64 readq_be(const volatile void __iomem *addr)
+{
+	u64 val;
+
+	log_read_mmio(64, addr, _THIS_IP_, _RET_IP_);
+	__io_br();
+	val = __be64_to_cpu((__be64 __force)__raw_readq(addr));
+	__io_ar(val);
+	log_post_read_mmio(val, 64, addr, _THIS_IP_, _RET_IP_);
+	return val;
+}
+#endif
+#endif /* CONFIG_64BIT */
+
+#ifndef writew_be
+#define writew_be writew_be
+static inline void writew_be(u16 value, volatile void __iomem *addr)
+{
+	log_write_mmio(value, 16, addr, _THIS_IP_, _RET_IP_);
+	__io_bw();
+	__raw_writew((u16 __force)__cpu_to_be16(value), addr);
+	__io_aw();
+	log_post_write_mmio(value, 16, addr, _THIS_IP_, _RET_IP_);
+}
+#endif
+
+#ifndef writel_be
+#define writel_be writel_be
+static inline void writel_be(u32 value, volatile void __iomem *addr)
+{
+	log_write_mmio(value, 32, addr, _THIS_IP_, _RET_IP_);
+	__io_bw();
+	__raw_writel((u32 __force)__cpu_to_be32(value), addr);
+	__io_aw();
+	log_post_write_mmio(value, 32, addr, _THIS_IP_, _RET_IP_);
+}
+#endif
+
+#ifdef CONFIG_64BIT
+#ifndef writeq_be
+#define writeq_be writeq_be
+static inline void writeq_be(u64 value, volatile void __iomem *addr)
+{
+	log_write_mmio(value, 64, addr, _THIS_IP_, _RET_IP_);
+	__io_bw();
+	__raw_writeq((u64 __force)__cpu_to_be64(value), addr);
+	__io_aw();
+	log_post_write_mmio(value, 64, addr, _THIS_IP_, _RET_IP_);
+}
+#endif
+#endif /* CONFIG_64BIT */
+
 /*
  * {read,write}{b,w,l,q}_relaxed() are like the regular version, but
  * are not guaranteed to provide ordering against spinlocks or memory
@@ -524,6 +614,118 @@ static inline void writesq(volatile void __iomem *addr, const void *buffer,
 #endif
 #endif /* CONFIG_64BIT */
 
+/*
+ * {read,write}s{w,l,q}_be() repeatedly access the same memory address
+ * in big endianness in 16-, 32- or 64-bit chunks (@count times) and
+ * return result in native endianness.
+ */
+
+#ifndef readsw_be
+#define readsw_be readsw_be
+static inline void readsw_be(const volatile void __iomem *addr,
+			     void *buffer,
+			     unsigned int count)
+{
+	if (count) {
+		u16 *buf = buffer;
+
+		do {
+			u16 x = __be16_to_cpu((__be16 __force)__raw_readw(addr));
+			*buf++ = x;
+		} while (--count);
+	}
+}
+#endif
+
+#ifndef readsl_be
+#define readsl_be readsl_be
+static inline void readsl_be(const volatile void __iomem *addr,
+			     void *buffer,
+			     unsigned int count)
+{
+	if (count) {
+		u32 *buf = buffer;
+
+		do {
+			u32 x = __be32_to_cpu((__be32 __force)__raw_readl(addr));
+			*buf++ = x;
+		} while (--count);
+	}
+}
+#endif
+
+#ifdef CONFIG_64BIT
+#ifndef readsq_be
+#define readsq_be readsq_be
+static inline void readsq_be(const volatile void __iomem *addr,
+			     void *buffer,
+			     unsigned int count)
+{
+	if (count) {
+		u64 *buf = buffer;
+
+		do {
+			u64 x = __be64_to_cpu((__be64 __force)__raw_readq(addr));
+			*buf++ = x;
+		} while (--count);
+	}
+}
+#endif
+#endif /* CONFIG_64BIT */
+
+#ifndef writesw_be
+#define writesw_be writesw_be
+static inline void writesw_be(volatile void __iomem *addr,
+			      const void *buffer,
+			      unsigned int count)
+{
+	if (count) {
+		const u16 *buf = buffer;
+
+		do {
+			__raw_writew((u16 __force)__cpu_to_be16(*buf), addr);
+			buf++;
+		} while (--count);
+	}
+}
+#endif
+
+#ifndef writesl_be
+#define writesl_be writesl_be
+static inline void writesl_be(volatile void __iomem *addr,
+			      const void *buffer,
+			      unsigned int count)
+{
+	if (count) {
+		const u32 *buf = buffer;
+
+		do {
+			__raw_writel((u32 __force)__cpu_to_be32(*buf), addr);
+			buf++;
+		} while (--count);
+	}
+}
+#endif
+
+#ifdef CONFIG_64BIT
+#ifndef writesq_be
+#define writesq_be writesq_be
+static inline void writesq_be(volatile void __iomem *addr,
+			      const void *buffer,
+			      unsigned int count)
+{
+	if (count) {
+		const u64 *buf = buffer;
+
+		do {
+			__raw_writeq((u64 __force)__cpu_to_be64(*buf), addr);
+			buf++;
+		} while (--count);
+	}
+}
+#endif
+#endif /* CONFIG_64BIT */
+
 #ifndef PCI_IOBASE
 #define PCI_IOBASE ((void __iomem *)0)
 #endif
-- 
2.34.1


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH V7 2/4] asm-generic/io.h: Add big-endian MMIO accessors

[Arnd Bergmann]

On Tue, Sep 23, 2025, at 17:45, Manikanta Guntupalli wrote:
> Add MMIO accessors to support big-endian memory operations. These helpers
> include {read, write}{w, l, q}_be() and {read, write}s{w, l, q}_be(),
> which allows to access big-endian memory regions while returning
> the results in the CPU’s native endianness.
>
> This provides a consistent interface to interact with hardware using
> big-endian register layouts.
>
> Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>

I feel like we already have too many accessor functions like these,
what's wrong with just using io{read,write}{8,16,32,64}be() in
your driver?

On most architectures (including arm, riscv, powerpc and microblaze,
but not x86), the ioread/write helpers are identical to the
readl/writel style helpers, the only difference being that on x86
they add an extra indirection for the port I/O check.

At the moment, there are only six drivers that use the
io{read,write}{8,16,32,64}be() style helpers. They
are all powerpc specific and can probably be changed
to io{read,write}be.

      Arnd

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [PATCH V7 2/4] asm-generic/io.h: Add big-endian MMIO accessors

[Guntupalli, Manikanta]

[Public]

Hi,

> -----Original Message-----
> From: Arnd Bergmann <arnd@arndb.de>
> Sent: Wednesday, September 24, 2025 12:08 AM
> To: Guntupalli, Manikanta <manikanta.guntupalli@amd.com>; git (AMD-Xilinx)
> <git@amd.com>; Simek, Michal <michal.simek@amd.com>; Alexandre Belloni
> <alexandre.belloni@bootlin.com>; Frank Li <Frank.Li@nxp.com>; Rob Herring
> <robh@kernel.org>; krzk+dt@kernel.org; Conor Dooley <conor+dt@kernel.org>;
> Przemysław Gaj <pgaj@cadence.com>; Wolfram Sang <wsa+renesas@sang-
> engineering.com>; tommaso.merciai.xr@bp.renesas.com;
> quic_msavaliy@quicinc.com; S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>;
> Sakari Ailus <sakari.ailus@linux.intel.com>; 'billy_tsai@aspeedtech.com'
> <billy_tsai@aspeedtech.com>; Kees Cook <kees@kernel.org>; Gustavo A. R. Silva
> <gustavoars@kernel.org>; Jarkko Nikula <jarkko.nikula@linux.intel.com>; Jorge
> Marques <jorge.marques@analog.com>; linux-i3c@lists.infradead.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Linux-Arch <linux-
> arch@vger.kernel.org>; linux-hardening@vger.kernel.org
> Cc: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; Goud, Srinivas
> <srinivas.goud@amd.com>; Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>;
> manion05gk@gmail.com
> Subject: Re: [PATCH V7 2/4] asm-generic/io.h: Add big-endian MMIO accessors
>
> On Tue, Sep 23, 2025, at 17:45, Manikanta Guntupalli wrote:
> > Add MMIO accessors to support big-endian memory operations. These
> > helpers include {read, write}{w, l, q}_be() and {read, write}s{w, l,
> > q}_be(), which allows to access big-endian memory regions while
> > returning the results in the CPU’s native endianness.
> >
> > This provides a consistent interface to interact with hardware using
> > big-endian register layouts.
> >
> > Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
>
> I feel like we already have too many accessor functions like these, what's wrong with
> just using io{read,write}{8,16,32,64}be() in your driver?
>
> On most architectures (including arm, riscv, powerpc and microblaze, but not x86),
> the ioread/write helpers are identical to the readl/writel style helpers, the only
> difference being that on x86 they add an extra indirection for the port I/O check.
>
> At the moment, there are only six drivers that use the
> io{read,write}{8,16,32,64}be() style helpers. They are all powerpc specific and can
> probably be changed to io{read,write}be.

The existing helpers io{read,write}{8,16,32,64}be() internally rely on {read,write}{b,w,l,q}().

From both description and implementation, {read,write}{b,w,l,q}() access little-endian memory and return the result in native endianness:

/*
 * {read,write}{b,w,l,q}() access little endian memory
 * and return result in native endianness.
 */
static inline u32 readl(const volatile void __iomem *addr)
{
        u32 val;

        log_read_mmio(32, addr, _THIS_IP_, _RET_IP_);
        __io_br();
        val = __le32_to_cpu((__le32 __force)__raw_readl(addr));
        __io_ar(val);
        log_post_read_mmio(val, 32, addr, _THIS_IP_, _RET_IP_);
        return val;
}

The io{read,write}*be() helpers then perform a byte swap, e.g.:

static inline u32 ioread32be(const volatile void __iomem *addr)
{
        return swab32(readl(addr));
}

This works on little-endian CPUs, but on big-endian CPUs the {read,write}{b,w,l,q}() helpers already return data in big-endian format. The extra byte swap in io{read,write}*be() therefore corrupts the data, producing little-endian values when big-endian is expected.

The newly introduced {read,write}{w,l,q}_be() helpers directly access big-endian IO memory and return results in native endianness, avoiding this mismatch.


Thanks,
Manikanta.
-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH V7 2/4] asm-generic/io.h: Add big-endian MMIO accessors

[Arnd Bergmann]

On Wed, Sep 24, 2025, at 10:59, Guntupalli, Manikanta wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>> Sent: Wednesday, September 24, 2025 12:08 AM
>> To: Guntupalli, Manikanta <manikanta.guntupalli@amd.com>; git (AMD-Xilinx)
>
> From both description and implementation, {read,write}{b,w,l,q}() 
> access little-endian memory and return the result in native endianness:
...
> This works on little-endian CPUs, but on big-endian CPUs the 
> {read,write}{b,w,l,q}() helpers already return data in big-endian 
> format.

> The extra byte swap in io{read,write}*be() therefore corrupts 
> the data, producing little-endian values when big-endian is expected.

> The newly introduced {read,write}{w,l,q}_be() helpers directly access 
> big-endian IO memory and return results in native endianness, avoiding 
> this mismatch.

No, in both your {read,write}{w,l,q}_be() and the existing io{read,write}*be()
helpers, big-endian platforms end up with an even number of swaps
(0 or 2), while little-endian platforms have exactly one swap.

The only exception is your {read,write}s{w,l,q}_be() string
helpers that are wrong because they have an extra swap and
are broken on FIFO registers in little-endian kernels.

     Arnd

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [PATCH V7 2/4] asm-generic/io.h: Add big-endian MMIO accessors

[Guntupalli, Manikanta]

[Public]

Hi,

> -----Original Message-----
> From: Arnd Bergmann <arnd@arndb.de>
> Sent: Wednesday, September 24, 2025 3:16 PM
> To: Guntupalli, Manikanta <manikanta.guntupalli@amd.com>; git (AMD-Xilinx)
> <git@amd.com>; Simek, Michal <michal.simek@amd.com>; Alexandre Belloni
> <alexandre.belloni@bootlin.com>; Frank Li <Frank.Li@nxp.com>; Rob Herring
> <robh@kernel.org>; krzk+dt@kernel.org; Conor Dooley <conor+dt@kernel.org>;
> Przemysław Gaj <pgaj@cadence.com>; Wolfram Sang <wsa+renesas@sang-
> engineering.com>; tommaso.merciai.xr@bp.renesas.com;
> quic_msavaliy@quicinc.com; S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>;
> Sakari Ailus <sakari.ailus@linux.intel.com>; 'billy_tsai@aspeedtech.com'
> <billy_tsai@aspeedtech.com>; Kees Cook <kees@kernel.org>; Gustavo A. R. Silva
> <gustavoars@kernel.org>; Jarkko Nikula <jarkko.nikula@linux.intel.com>; Jorge
> Marques <jorge.marques@analog.com>; linux-i3c@lists.infradead.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Linux-Arch <linux-
> arch@vger.kernel.org>; linux-hardening@vger.kernel.org
> Cc: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; Goud, Srinivas
> <srinivas.goud@amd.com>; Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>;
> manion05gk@gmail.com
> Subject: Re: [PATCH V7 2/4] asm-generic/io.h: Add big-endian MMIO accessors
>
> On Wed, Sep 24, 2025, at 10:59, Guntupalli, Manikanta wrote:
> >> From: Arnd Bergmann <arnd@arndb.de>
> >> Sent: Wednesday, September 24, 2025 12:08 AM
> >> To: Guntupalli, Manikanta <manikanta.guntupalli@amd.com>; git
> >> (AMD-Xilinx)
> >
> > From both description and implementation, {read,write}{b,w,l,q}()
> > access little-endian memory and return the result in native endianness:
> ...
> > This works on little-endian CPUs, but on big-endian CPUs the
> > {read,write}{b,w,l,q}() helpers already return data in big-endian
> > format.
>
> > The extra byte swap in io{read,write}*be() therefore corrupts the
> > data, producing little-endian values when big-endian is expected.
>
> > The newly introduced {read,write}{w,l,q}_be() helpers directly access
> > big-endian IO memory and return results in native endianness, avoiding
> > this mismatch.
>
> No, in both your {read,write}{w,l,q}_be() and the existing io{read,write}*be() helpers,
> big-endian platforms end up with an even number of swaps
> (0 or 2), while little-endian platforms have exactly one swap.
>
> The only exception is your {read,write}s{w,l,q}_be() string helpers that are wrong
> because they have an extra swap and are broken on FIFO registers in little-endian
> kernels.
We have performed our validation on little-endian kernels, and in our testing, the helpers worked correctly without any issues.

Thanks,
Manikanta.

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH V7 2/4] asm-generic/io.h: Add big-endian MMIO accessors

[Frank Li]

On Tue, Sep 23, 2025 at 09:15:49PM +0530, Manikanta Guntupalli wrote:
> Add MMIO accessors to support big-endian memory operations. These helpers
> include {read, write}{w, l, q}_be() and {read, write}s{w, l, q}_be(),
> which allows to access big-endian memory regions while returning
> the results in the CPU’s native endianness.
>
> This provides a consistent interface to interact with hardware using
> big-endian register layouts.
>
> Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> Changes since V7:
> This patch introduced in V7.
> ---
>  include/asm-generic/io.h | 202 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 202 insertions(+)
>
> diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> index 11abad6c87e1..d18a8ca6c06c 100644
> --- a/include/asm-generic/io.h
> +++ b/include/asm-generic/io.h
> @@ -295,6 +295,96 @@ static inline void writeq(u64 value, volatile void __iomem *addr)
>  #endif
>  #endif /* CONFIG_64BIT */
>
> +/*
> + * {read,write}{w,l,q}_be() access big endian memory and return result
> + * in native endianness.
> + */
> +
> +#ifndef readw_be
> +#define readw_be readw_be
> +static inline u16 readw_be(const volatile void __iomem *addr)
> +{
> +	u16 val;
> +
> +	log_read_mmio(16, addr, _THIS_IP_, _RET_IP_);
> +	__io_br();
> +	val = __be16_to_cpu((__be16 __force)__raw_readw(addr));
> +	__io_ar(val);
> +	log_post_read_mmio(val, 16, addr, _THIS_IP_, _RET_IP_);
> +	return val;
> +}
> +#endif
> +
> +#ifndef readl_be
> +#define readl_be readl_be
> +static inline u32 readl_be(const volatile void __iomem *addr)
> +{
> +	u32 val;
> +
> +	log_read_mmio(32, addr, _THIS_IP_, _RET_IP_);
> +	__io_br();
> +	val = __be32_to_cpu((__be32 __force)__raw_readl(addr));
> +	__io_ar(val);
> +	log_post_read_mmio(val, 32, addr, _THIS_IP_, _RET_IP_);
> +	return val;
> +}
> +#endif
> +
> +#ifdef CONFIG_64BIT
> +#ifndef readq_be
> +#define readq_be readq_be
> +static inline u64 readq_be(const volatile void __iomem *addr)
> +{
> +	u64 val;
> +
> +	log_read_mmio(64, addr, _THIS_IP_, _RET_IP_);
> +	__io_br();
> +	val = __be64_to_cpu((__be64 __force)__raw_readq(addr));
> +	__io_ar(val);
> +	log_post_read_mmio(val, 64, addr, _THIS_IP_, _RET_IP_);
> +	return val;
> +}
> +#endif
> +#endif /* CONFIG_64BIT */
> +
> +#ifndef writew_be
> +#define writew_be writew_be
> +static inline void writew_be(u16 value, volatile void __iomem *addr)
> +{
> +	log_write_mmio(value, 16, addr, _THIS_IP_, _RET_IP_);
> +	__io_bw();
> +	__raw_writew((u16 __force)__cpu_to_be16(value), addr);
> +	__io_aw();
> +	log_post_write_mmio(value, 16, addr, _THIS_IP_, _RET_IP_);
> +}
> +#endif
> +
> +#ifndef writel_be
> +#define writel_be writel_be
> +static inline void writel_be(u32 value, volatile void __iomem *addr)
> +{
> +	log_write_mmio(value, 32, addr, _THIS_IP_, _RET_IP_);
> +	__io_bw();
> +	__raw_writel((u32 __force)__cpu_to_be32(value), addr);
> +	__io_aw();
> +	log_post_write_mmio(value, 32, addr, _THIS_IP_, _RET_IP_);
> +}
> +#endif
> +
> +#ifdef CONFIG_64BIT
> +#ifndef writeq_be
> +#define writeq_be writeq_be
> +static inline void writeq_be(u64 value, volatile void __iomem *addr)
> +{
> +	log_write_mmio(value, 64, addr, _THIS_IP_, _RET_IP_);
> +	__io_bw();
> +	__raw_writeq((u64 __force)__cpu_to_be64(value), addr);
> +	__io_aw();
> +	log_post_write_mmio(value, 64, addr, _THIS_IP_, _RET_IP_);
> +}
> +#endif
> +#endif /* CONFIG_64BIT */
> +
>  /*
>   * {read,write}{b,w,l,q}_relaxed() are like the regular version, but
>   * are not guaranteed to provide ordering against spinlocks or memory
> @@ -524,6 +614,118 @@ static inline void writesq(volatile void __iomem *addr, const void *buffer,
>  #endif
>  #endif /* CONFIG_64BIT */
>
> +/*
> + * {read,write}s{w,l,q}_be() repeatedly access the same memory address
> + * in big endianness in 16-, 32- or 64-bit chunks (@count times) and
> + * return result in native endianness.
> + */
> +
> +#ifndef readsw_be
> +#define readsw_be readsw_be
> +static inline void readsw_be(const volatile void __iomem *addr,
> +			     void *buffer,
> +			     unsigned int count)
> +{
> +	if (count) {
> +		u16 *buf = buffer;
> +
> +		do {
> +			u16 x = __be16_to_cpu((__be16 __force)__raw_readw(addr));
> +			*buf++ = x;
> +		} while (--count);
> +	}
> +}
> +#endif
> +
> +#ifndef readsl_be
> +#define readsl_be readsl_be
> +static inline void readsl_be(const volatile void __iomem *addr,
> +			     void *buffer,
> +			     unsigned int count)
> +{
> +	if (count) {
> +		u32 *buf = buffer;
> +
> +		do {
> +			u32 x = __be32_to_cpu((__be32 __force)__raw_readl(addr));
> +			*buf++ = x;
> +		} while (--count);
> +	}
> +}
> +#endif
> +
> +#ifdef CONFIG_64BIT
> +#ifndef readsq_be
> +#define readsq_be readsq_be
> +static inline void readsq_be(const volatile void __iomem *addr,
> +			     void *buffer,
> +			     unsigned int count)
> +{
> +	if (count) {
> +		u64 *buf = buffer;
> +
> +		do {
> +			u64 x = __be64_to_cpu((__be64 __force)__raw_readq(addr));
> +			*buf++ = x;
> +		} while (--count);
> +	}
> +}
> +#endif
> +#endif /* CONFIG_64BIT */
> +
> +#ifndef writesw_be
> +#define writesw_be writesw_be
> +static inline void writesw_be(volatile void __iomem *addr,
> +			      const void *buffer,
> +			      unsigned int count)
> +{
> +	if (count) {
> +		const u16 *buf = buffer;
> +
> +		do {
> +			__raw_writew((u16 __force)__cpu_to_be16(*buf), addr);
> +			buf++;
> +		} while (--count);
> +	}
> +}
> +#endif
> +
> +#ifndef writesl_be
> +#define writesl_be writesl_be
> +static inline void writesl_be(volatile void __iomem *addr,
> +			      const void *buffer,
> +			      unsigned int count)
> +{
> +	if (count) {
> +		const u32 *buf = buffer;
> +
> +		do {
> +			__raw_writel((u32 __force)__cpu_to_be32(*buf), addr);
> +			buf++;
> +		} while (--count);
> +	}
> +}
> +#endif
> +
> +#ifdef CONFIG_64BIT
> +#ifndef writesq_be
> +#define writesq_be writesq_be
> +static inline void writesq_be(volatile void __iomem *addr,
> +			      const void *buffer,
> +			      unsigned int count)
> +{
> +	if (count) {
> +		const u64 *buf = buffer;
> +
> +		do {
> +			__raw_writeq((u64 __force)__cpu_to_be64(*buf), addr);
> +			buf++;
> +		} while (--count);
> +	}
> +}
> +#endif
> +#endif /* CONFIG_64BIT */
> +
>  #ifndef PCI_IOBASE
>  #define PCI_IOBASE ((void __iomem *)0)
>  #endif
> --
> 2.34.1
>

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH V7 2/4] asm-generic/io.h: Add big-endian MMIO accessors

[kernel test robot]

Hi Manikanta,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master arnd-asm-generic/master v6.17-rc7 next-20250924]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Manikanta-Guntupalli/dt-bindings-i3c-Add-AMD-I3C-master-controller-support/20250923-234944
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20250923154551.2112388-3-manikanta.guntupalli%40amd.com
patch subject: [PATCH V7 2/4] asm-generic/io.h: Add big-endian MMIO accessors
config: sparc-allnoconfig (https://download.01.org/0day-ci/archive/20250925/202509250413.sOTeU37m-lkp@intel.com/config)
compiler: sparc-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250925/202509250413.sOTeU37m-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202509250413.sOTeU37m-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from include/linux/io.h:12,
                    from include/linux/irq.h:20,
                    from include/asm-generic/hardirq.h:17,
                    from arch/sparc/include/asm/hardirq_32.h:11,
                    from arch/sparc/include/asm/hardirq.h:7,
                    from include/linux/hardirq.h:11,
                    from include/linux/interrupt.h:11,
                    from include/linux/trace_recursion.h:5,
                    from include/linux/ftrace.h:10,
                    from include/linux/perf_event.h:43,
                    from arch/sparc/mm/fault_32.c:22:
>> arch/sparc/include/asm/io.h:16:9: warning: 'readw_be' redefined
      16 | #define readw_be(__addr)        __raw_readw(__addr)
         |         ^~~~~~~~
   In file included from arch/sparc/include/asm/io_32.h:21,
                    from arch/sparc/include/asm/io.h:7:
   include/asm-generic/io.h:304:9: note: this is the location of the previous definition
     304 | #define readw_be readw_be
         |         ^~~~~~~~
>> arch/sparc/include/asm/io.h:17:9: warning: 'readl_be' redefined
      17 | #define readl_be(__addr)        __raw_readl(__addr)
         |         ^~~~~~~~
   include/asm-generic/io.h:319:9: note: this is the location of the previous definition
     319 | #define readl_be readl_be
         |         ^~~~~~~~
>> arch/sparc/include/asm/io.h:19:9: warning: 'writel_be' redefined
      19 | #define writel_be(__w, __addr)  __raw_writel(__w, __addr)
         |         ^~~~~~~~~
   include/asm-generic/io.h:363:9: note: this is the location of the previous definition
     363 | #define writel_be writel_be
         |         ^~~~~~~~~
>> arch/sparc/include/asm/io.h:20:9: warning: 'writew_be' redefined
      20 | #define writew_be(__l, __addr)  __raw_writew(__l, __addr)
         |         ^~~~~~~~~
   include/asm-generic/io.h:351:9: note: this is the location of the previous definition
     351 | #define writew_be writew_be
         |         ^~~~~~~~~
--
   In file included from include/linux/io.h:12,
                    from include/linux/irq.h:20,
                    from include/asm-generic/hardirq.h:17,
                    from arch/sparc/include/asm/hardirq_32.h:11,
                    from arch/sparc/include/asm/hardirq.h:7,
                    from include/linux/hardirq.h:11,
                    from include/linux/highmem.h:12,
                    from include/linux/pagemap.h:11,
                    from arch/sparc/mm/srmmu.c:15:
>> arch/sparc/include/asm/io.h:16:9: warning: 'readw_be' redefined
      16 | #define readw_be(__addr)        __raw_readw(__addr)
         |         ^~~~~~~~
   In file included from arch/sparc/include/asm/io_32.h:21,
                    from arch/sparc/include/asm/io.h:7:
   include/asm-generic/io.h:304:9: note: this is the location of the previous definition
     304 | #define readw_be readw_be
         |         ^~~~~~~~
>> arch/sparc/include/asm/io.h:17:9: warning: 'readl_be' redefined
      17 | #define readl_be(__addr)        __raw_readl(__addr)
         |         ^~~~~~~~
   include/asm-generic/io.h:319:9: note: this is the location of the previous definition
     319 | #define readl_be readl_be
         |         ^~~~~~~~
>> arch/sparc/include/asm/io.h:19:9: warning: 'writel_be' redefined
      19 | #define writel_be(__w, __addr)  __raw_writel(__w, __addr)
         |         ^~~~~~~~~
   include/asm-generic/io.h:363:9: note: this is the location of the previous definition
     363 | #define writel_be writel_be
         |         ^~~~~~~~~
>> arch/sparc/include/asm/io.h:20:9: warning: 'writew_be' redefined
      20 | #define writew_be(__l, __addr)  __raw_writew(__l, __addr)
         |         ^~~~~~~~~
   include/asm-generic/io.h:351:9: note: this is the location of the previous definition
     351 | #define writew_be writew_be
         |         ^~~~~~~~~
   arch/sparc/mm/srmmu.c: In function 'poke_hypersparc':
   arch/sparc/mm/srmmu.c:1074:32: warning: variable 'clear' set but not used [-Wunused-but-set-variable]
    1074 |         volatile unsigned long clear;
         |                                ^~~~~


vim +/readw_be +16 arch/sparc/include/asm/io.h

21dccddf45aae2 Jan Andersson 2011-05-10   9  
21dccddf45aae2 Jan Andersson 2011-05-10  10  /*
21dccddf45aae2 Jan Andersson 2011-05-10  11   * Defines used for both SPARC32 and SPARC64
21dccddf45aae2 Jan Andersson 2011-05-10  12   */
21dccddf45aae2 Jan Andersson 2011-05-10  13  
21dccddf45aae2 Jan Andersson 2011-05-10  14  /* Big endian versions of memory read/write routines */
21dccddf45aae2 Jan Andersson 2011-05-10  15  #define readb_be(__addr)	__raw_readb(__addr)
21dccddf45aae2 Jan Andersson 2011-05-10 @16  #define readw_be(__addr)	__raw_readw(__addr)
21dccddf45aae2 Jan Andersson 2011-05-10 @17  #define readl_be(__addr)	__raw_readl(__addr)
21dccddf45aae2 Jan Andersson 2011-05-10  18  #define writeb_be(__b, __addr)	__raw_writeb(__b, __addr)
21dccddf45aae2 Jan Andersson 2011-05-10 @19  #define writel_be(__w, __addr)	__raw_writel(__w, __addr)
21dccddf45aae2 Jan Andersson 2011-05-10 @20  #define writew_be(__l, __addr)	__raw_writew(__l, __addr)
21dccddf45aae2 Jan Andersson 2011-05-10  21  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH V7 2/4] asm-generic/io.h: Add big-endian MMIO accessors

[kernel test robot]

Hi Manikanta,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master arnd-asm-generic/master v6.17-rc7 next-20250924]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Manikanta-Guntupalli/dt-bindings-i3c-Add-AMD-I3C-master-controller-support/20250923-234944
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20250923154551.2112388-3-manikanta.guntupalli%40amd.com
patch subject: [PATCH V7 2/4] asm-generic/io.h: Add big-endian MMIO accessors
config: powerpc-allnoconfig (https://download.01.org/0day-ci/archive/20250925/202509251347.sb9SGhab-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250925/202509251347.sb9SGhab-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202509251347.sb9SGhab-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from arch/powerpc/include/asm/io.h:962,
                    from include/linux/io.h:12,
                    from include/linux/irq.h:20,
                    from arch/powerpc/include/asm/hardirq.h:6,
                    from include/linux/hardirq.h:11,
                    from include/linux/interrupt.h:11,
                    from include/linux/kernel_stat.h:8,
                    from include/linux/cgroup.h:27,
                    from include/linux/memcontrol.h:13,
                    from include/linux/swap.h:9,
                    from include/linux/suspend.h:5,
                    from arch/powerpc/kernel/asm-offsets.c:21:
>> include/asm-generic/io.h:304:18: error: redefinition of 'readw_be'
     304 | #define readw_be readw_be
         |                  ^~~~~~~~
   include/asm-generic/io.h:305:19: note: in expansion of macro 'readw_be'
     305 | static inline u16 readw_be(const volatile void __iomem *addr)
         |                   ^~~~~~~~
   arch/powerpc/include/asm/io.h:517:19: note: previous definition of 'readw_be' with type 'u16(const volatile void *)' {aka 'short unsigned int(const volatile void *)'}
     517 | static inline u16 readw_be(const volatile void __iomem *addr)
         |                   ^~~~~~~~
>> include/asm-generic/io.h:319:18: error: redefinition of 'readl_be'
     319 | #define readl_be readl_be
         |                  ^~~~~~~~
   include/asm-generic/io.h:320:19: note: in expansion of macro 'readl_be'
     320 | static inline u32 readl_be(const volatile void __iomem *addr)
         |                   ^~~~~~~~
   arch/powerpc/include/asm/io.h:522:19: note: previous definition of 'readl_be' with type 'u32(const volatile void *)' {aka 'unsigned int(const volatile void *)'}
     522 | static inline u32 readl_be(const volatile void __iomem *addr)
         |                   ^~~~~~~~
>> include/asm-generic/io.h:351:19: error: redefinition of 'writew_be'
     351 | #define writew_be writew_be
         |                   ^~~~~~~~~
   include/asm-generic/io.h:352:20: note: in expansion of macro 'writew_be'
     352 | static inline void writew_be(u16 value, volatile void __iomem *addr)
         |                    ^~~~~~~~~
   arch/powerpc/include/asm/io.h:545:20: note: previous definition of 'writew_be' with type 'void(u16,  volatile void *)' {aka 'void(short unsigned int,  volatile void *)'}
     545 | static inline void writew_be(u16 val, volatile void __iomem *addr)
         |                    ^~~~~~~~~
>> include/asm-generic/io.h:363:19: error: redefinition of 'writel_be'
     363 | #define writel_be writel_be
         |                   ^~~~~~~~~
   include/asm-generic/io.h:364:20: note: in expansion of macro 'writel_be'
     364 | static inline void writel_be(u32 value, volatile void __iomem *addr)
         |                    ^~~~~~~~~
   arch/powerpc/include/asm/io.h:550:20: note: previous definition of 'writel_be' with type 'void(u32,  volatile void *)' {aka 'void(unsigned int,  volatile void *)'}
     550 | static inline void writel_be(u32 val, volatile void __iomem *addr)
         |                    ^~~~~~~~~
   make[3]: *** [scripts/Makefile.build:182: arch/powerpc/kernel/asm-offsets.s] Error 1
   make[3]: Target 'prepare' not remade because of errors.
   make[2]: *** [Makefile:1282: prepare0] Error 2
   make[2]: Target 'prepare' not remade because of errors.
   make[1]: *** [Makefile:248: __sub-make] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [Makefile:248: __sub-make] Error 2
   make: Target 'prepare' not remade because of errors.


vim +/readw_be +304 include/asm-generic/io.h

   297	
   298	/*
   299	 * {read,write}{w,l,q}_be() access big endian memory and return result
   300	 * in native endianness.
   301	 */
   302	
   303	#ifndef readw_be
 > 304	#define readw_be readw_be
   305	static inline u16 readw_be(const volatile void __iomem *addr)
   306	{
   307		u16 val;
   308	
   309		log_read_mmio(16, addr, _THIS_IP_, _RET_IP_);
   310		__io_br();
   311		val = __be16_to_cpu((__be16 __force)__raw_readw(addr));
   312		__io_ar(val);
   313		log_post_read_mmio(val, 16, addr, _THIS_IP_, _RET_IP_);
   314		return val;
   315	}
   316	#endif
   317	
   318	#ifndef readl_be
 > 319	#define readl_be readl_be
   320	static inline u32 readl_be(const volatile void __iomem *addr)
   321	{
   322		u32 val;
   323	
   324		log_read_mmio(32, addr, _THIS_IP_, _RET_IP_);
   325		__io_br();
   326		val = __be32_to_cpu((__be32 __force)__raw_readl(addr));
   327		__io_ar(val);
   328		log_post_read_mmio(val, 32, addr, _THIS_IP_, _RET_IP_);
   329		return val;
   330	}
   331	#endif
   332	
   333	#ifdef CONFIG_64BIT
   334	#ifndef readq_be
   335	#define readq_be readq_be
   336	static inline u64 readq_be(const volatile void __iomem *addr)
   337	{
   338		u64 val;
   339	
   340		log_read_mmio(64, addr, _THIS_IP_, _RET_IP_);
   341		__io_br();
   342		val = __be64_to_cpu((__be64 __force)__raw_readq(addr));
   343		__io_ar(val);
   344		log_post_read_mmio(val, 64, addr, _THIS_IP_, _RET_IP_);
   345		return val;
   346	}
   347	#endif
   348	#endif /* CONFIG_64BIT */
   349	
   350	#ifndef writew_be
 > 351	#define writew_be writew_be
   352	static inline void writew_be(u16 value, volatile void __iomem *addr)
   353	{
   354		log_write_mmio(value, 16, addr, _THIS_IP_, _RET_IP_);
   355		__io_bw();
   356		__raw_writew((u16 __force)__cpu_to_be16(value), addr);
   357		__io_aw();
   358		log_post_write_mmio(value, 16, addr, _THIS_IP_, _RET_IP_);
   359	}
   360	#endif
   361	
   362	#ifndef writel_be
 > 363	#define writel_be writel_be
   364	static inline void writel_be(u32 value, volatile void __iomem *addr)
   365	{
   366		log_write_mmio(value, 32, addr, _THIS_IP_, _RET_IP_);
   367		__io_bw();
   368		__raw_writel((u32 __force)__cpu_to_be32(value), addr);
   369		__io_aw();
   370		log_post_write_mmio(value, 32, addr, _THIS_IP_, _RET_IP_);
   371	}
   372	#endif
   373	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo() and i3c_writel_fifo()

[Manikanta Guntupalli]

Add endianness handling to the FIFO access helpers i3c_readl_fifo() and
i3c_writel_fifo(). This ensures correct data transfers on platforms where
the FIFO registers are expected to be accessed in either big-endian or
little-endian format.

Update the Synopsys, Cadence, and Renesas I3C master controller drivers to
pass the appropriate endianness argument to these helpers.

Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
---
Changes since V7:
This patch introduced in V7.
---
 drivers/i3c/internals.h              | 35 +++++++++++++++++++++++-----
 drivers/i3c/master/dw-i3c-master.c   |  9 ++++---
 drivers/i3c/master/i3c-master-cdns.c |  9 ++++---
 drivers/i3c/master/renesas-i3c.c     | 12 ++++++----
 4 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
index 0d857cc68cc5..399bbf006dcd 100644
--- a/drivers/i3c/internals.h
+++ b/drivers/i3c/internals.h
@@ -24,21 +24,35 @@ int i3c_dev_request_ibi_locked(struct i3c_dev_desc *dev,
 			       const struct i3c_ibi_setup *req);
 void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev);
 
+enum i3c_fifo_endian {
+	I3C_FIFO_LITTLE_ENDIAN,
+	I3C_FIFO_BIG_ENDIAN,
+};
+
 /**
  * i3c_writel_fifo - Write data buffer to 32bit FIFO
  * @addr: FIFO Address to write to
  * @buf: Pointer to the data bytes to write
  * @nbytes: Number of bytes to write
+ * @endian: Endianness of FIFO write
  */
 static inline void i3c_writel_fifo(void __iomem *addr, const void *buf,
-				   int nbytes)
+				   int nbytes, enum i3c_fifo_endian endian)
 {
-	writesl(addr, buf, nbytes / 4);
+	if (endian)
+		writesl_be(addr, buf, nbytes / 4);
+	else
+		writesl(addr, buf, nbytes / 4);
+
 	if (nbytes & 3) {
 		u32 tmp = 0;
 
 		memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);
-		writel(tmp, addr);
+
+		if (endian)
+			writel_be(tmp, addr);
+		else
+			writel(tmp, addr);
 	}
 }
 
@@ -47,15 +61,24 @@ static inline void i3c_writel_fifo(void __iomem *addr, const void *buf,
  * @addr: FIFO Address to read from
  * @buf: Pointer to the buffer to store read bytes
  * @nbytes: Number of bytes to read
+ * @endian: Endianness of FIFO read
  */
 static inline void i3c_readl_fifo(const void __iomem *addr, void *buf,
-				  int nbytes)
+				  int nbytes, enum i3c_fifo_endian endian)
 {
-	readsl(addr, buf, nbytes / 4);
+	if (endian)
+		readsl_be(addr, buf, nbytes / 4);
+	else
+		readsl(addr, buf, nbytes / 4);
+
 	if (nbytes & 3) {
 		u32 tmp;
 
-		tmp = readl(addr);
+		if (endian)
+			tmp = readl_be(addr);
+		else
+			tmp = readl(addr);
+
 		memcpy(buf + (nbytes & ~3), &tmp, nbytes & 3);
 	}
 }
diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 974122b2d20e..5d723ac041c2 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -337,19 +337,22 @@ static int dw_i3c_master_get_free_pos(struct dw_i3c_master *master)
 static void dw_i3c_master_wr_tx_fifo(struct dw_i3c_master *master,
 				     const u8 *bytes, int nbytes)
 {
-	i3c_writel_fifo(master->regs + RX_TX_DATA_PORT, bytes, nbytes);
+	i3c_writel_fifo(master->regs + RX_TX_DATA_PORT, bytes, nbytes,
+			I3C_FIFO_LITTLE_ENDIAN);
 }
 
 static void dw_i3c_master_read_rx_fifo(struct dw_i3c_master *master,
 				       u8 *bytes, int nbytes)
 {
-	i3c_readl_fifo(master->regs + RX_TX_DATA_PORT, bytes, nbytes);
+	i3c_readl_fifo(master->regs + RX_TX_DATA_PORT, bytes, nbytes,
+		       I3C_FIFO_LITTLE_ENDIAN);
 }
 
 static void dw_i3c_master_read_ibi_fifo(struct dw_i3c_master *master,
 					u8 *bytes, int nbytes)
 {
-	i3c_readl_fifo(master->regs + IBI_QUEUE_STATUS, bytes, nbytes);
+	i3c_readl_fifo(master->regs + IBI_QUEUE_STATUS, bytes, nbytes,
+		       I3C_FIFO_LITTLE_ENDIAN);
 }
 
 static struct dw_i3c_xfer *
diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c
index 97b151564d3d..de3b5e894b4b 100644
--- a/drivers/i3c/master/i3c-master-cdns.c
+++ b/drivers/i3c/master/i3c-master-cdns.c
@@ -428,13 +428,15 @@ to_cdns_i3c_master(struct i3c_master_controller *master)
 static void cdns_i3c_master_wr_to_tx_fifo(struct cdns_i3c_master *master,
 					  const u8 *bytes, int nbytes)
 {
-	i3c_writel_fifo(master->regs + TX_FIFO, bytes, nbytes);
+	i3c_writel_fifo(master->regs + TX_FIFO, bytes, nbytes,
+			I3C_FIFO_LITTLE_ENDIAN);
 }
 
 static void cdns_i3c_master_rd_from_rx_fifo(struct cdns_i3c_master *master,
 					    u8 *bytes, int nbytes)
 {
-	i3c_readl_fifo(master->regs + RX_FIFO, bytes, nbytes);
+	i3c_readl_fifo(master->regs + RX_FIFO, bytes, nbytes,
+		       I3C_FIFO_LITTLE_ENDIAN);
 }
 
 static bool cdns_i3c_master_supports_ccc_cmd(struct i3c_master_controller *m,
@@ -1319,7 +1321,8 @@ static void cdns_i3c_master_handle_ibi(struct cdns_i3c_master *master,
 	buf = slot->data;
 
 	nbytes = IBIR_XFER_BYTES(ibir);
-	i3c_readl_fifo(master->regs + IBI_DATA_FIFO, buf, nbytes);
+	i3c_readl_fifo(master->regs + IBI_DATA_FIFO, buf, nbytes,
+		       I3C_FIFO_LITTLE_ENDIAN);
 
 	slot->len = min_t(unsigned int, IBIR_XFER_BYTES(ibir),
 			  dev->ibi->max_payload_len);
diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 174d3dc5d276..5610cf71740e 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -835,7 +835,8 @@ static int renesas_i3c_priv_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer
 		}
 
 		if (!i3c_xfers[i].rnw && i3c_xfers[i].len > 4) {
-			i3c_writel_fifo(i3c->regs + NTDTBP0, cmd->tx_buf, cmd->len);
+			i3c_writel_fifo(i3c->regs + NTDTBP0, cmd->tx_buf, cmd->len,
+					I3C_FIFO_LITTLE_ENDIAN);
 			if (cmd->len > NTDTBP0_DEPTH * sizeof(u32))
 				renesas_set_bit(i3c->regs, NTIE, NTIE_TDBEIE0);
 		}
@@ -1021,7 +1022,8 @@ static irqreturn_t renesas_i3c_tx_isr(int irq, void *data)
 			/* Clear the Transmit Buffer Empty status flag. */
 			renesas_clear_bit(i3c->regs, NTST, NTST_TDBEF0);
 		} else {
-			i3c_writel_fifo(i3c->regs + NTDTBP0, cmd->tx_buf, cmd->len);
+			i3c_writel_fifo(i3c->regs + NTDTBP0, cmd->tx_buf, cmd->len,
+					I3C_FIFO_LITTLE_ENDIAN);
 		}
 	}
 
@@ -1061,7 +1063,8 @@ static irqreturn_t renesas_i3c_resp_isr(int irq, void *data)
 			if (NDBSTLV0_RDBLV(renesas_readl(i3c->regs, NDBSTLV0)) && !cmd->err)
 				bytes_remaining = data_len - cmd->rx_count;
 
-			i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, bytes_remaining);
+			i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, bytes_remaining,
+				       I3C_FIFO_LITTLE_ENDIAN);
 			renesas_clear_bit(i3c->regs, NTIE, NTIE_RDBFIE0);
 			break;
 		default:
@@ -1203,7 +1206,8 @@ static irqreturn_t renesas_i3c_rx_isr(int irq, void *data)
 			cmd->i2c_bytes_left--;
 		} else {
 			read_bytes = NDBSTLV0_RDBLV(renesas_readl(i3c->regs, NDBSTLV0)) * sizeof(u32);
-			i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, read_bytes);
+			i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, read_bytes,
+				       I3C_FIFO_LITTLE_ENDIAN);
 			cmd->rx_count = read_bytes;
 		}
 
-- 
2.34.1


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo() and i3c_writel_fifo()

[Frank Li]

On Tue, Sep 23, 2025 at 09:15:50PM +0530, Manikanta Guntupalli wrote:
> Add endianness handling to the FIFO access helpers i3c_readl_fifo() and
> i3c_writel_fifo(). This ensures correct data transfers on platforms where
> the FIFO registers are expected to be accessed in either big-endian or
> little-endian format.
>
> Update the Synopsys, Cadence, and Renesas I3C master controller drivers to
> pass the appropriate endianness argument to these helpers.
>
> Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> Changes since V7:
> This patch introduced in V7.
> ---
>  drivers/i3c/internals.h              | 35 +++++++++++++++++++++++-----
>  drivers/i3c/master/dw-i3c-master.c   |  9 ++++---
>  drivers/i3c/master/i3c-master-cdns.c |  9 ++++---
>  drivers/i3c/master/renesas-i3c.c     | 12 ++++++----
>  4 files changed, 49 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
> index 0d857cc68cc5..399bbf006dcd 100644
> --- a/drivers/i3c/internals.h
> +++ b/drivers/i3c/internals.h
> @@ -24,21 +24,35 @@ int i3c_dev_request_ibi_locked(struct i3c_dev_desc *dev,
>  			       const struct i3c_ibi_setup *req);
>  void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev);
>
> +enum i3c_fifo_endian {
> +	I3C_FIFO_LITTLE_ENDIAN,
> +	I3C_FIFO_BIG_ENDIAN,
> +};
> +
>  /**
>   * i3c_writel_fifo - Write data buffer to 32bit FIFO
>   * @addr: FIFO Address to write to
>   * @buf: Pointer to the data bytes to write
>   * @nbytes: Number of bytes to write
> + * @endian: Endianness of FIFO write
>   */
>  static inline void i3c_writel_fifo(void __iomem *addr, const void *buf,
> -				   int nbytes)
> +				   int nbytes, enum i3c_fifo_endian endian)
>  {
> -	writesl(addr, buf, nbytes / 4);
> +	if (endian)
> +		writesl_be(addr, buf, nbytes / 4);
> +	else
> +		writesl(addr, buf, nbytes / 4);
> +
>  	if (nbytes & 3) {
>  		u32 tmp = 0;
>
>  		memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);
> -		writel(tmp, addr);
> +
> +		if (endian)
> +			writel_be(tmp, addr);
> +		else
> +			writel(tmp, addr);
>  	}
>  }
>
> @@ -47,15 +61,24 @@ static inline void i3c_writel_fifo(void __iomem *addr, const void *buf,
>   * @addr: FIFO Address to read from
>   * @buf: Pointer to the buffer to store read bytes
>   * @nbytes: Number of bytes to read
> + * @endian: Endianness of FIFO read
>   */
>  static inline void i3c_readl_fifo(const void __iomem *addr, void *buf,
> -				  int nbytes)
> +				  int nbytes, enum i3c_fifo_endian endian)
>  {
> -	readsl(addr, buf, nbytes / 4);
> +	if (endian)
> +		readsl_be(addr, buf, nbytes / 4);
> +	else
> +		readsl(addr, buf, nbytes / 4);
> +
>  	if (nbytes & 3) {
>  		u32 tmp;
>
> -		tmp = readl(addr);
> +		if (endian)
> +			tmp = readl_be(addr);
> +		else
> +			tmp = readl(addr);
> +
>  		memcpy(buf + (nbytes & ~3), &tmp, nbytes & 3);
>  	}
>  }
> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index 974122b2d20e..5d723ac041c2 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -337,19 +337,22 @@ static int dw_i3c_master_get_free_pos(struct dw_i3c_master *master)
>  static void dw_i3c_master_wr_tx_fifo(struct dw_i3c_master *master,
>  				     const u8 *bytes, int nbytes)
>  {
> -	i3c_writel_fifo(master->regs + RX_TX_DATA_PORT, bytes, nbytes);
> +	i3c_writel_fifo(master->regs + RX_TX_DATA_PORT, bytes, nbytes,
> +			I3C_FIFO_LITTLE_ENDIAN);
>  }
>
>  static void dw_i3c_master_read_rx_fifo(struct dw_i3c_master *master,
>  				       u8 *bytes, int nbytes)
>  {
> -	i3c_readl_fifo(master->regs + RX_TX_DATA_PORT, bytes, nbytes);
> +	i3c_readl_fifo(master->regs + RX_TX_DATA_PORT, bytes, nbytes,
> +		       I3C_FIFO_LITTLE_ENDIAN);
>  }
>
>  static void dw_i3c_master_read_ibi_fifo(struct dw_i3c_master *master,
>  					u8 *bytes, int nbytes)
>  {
> -	i3c_readl_fifo(master->regs + IBI_QUEUE_STATUS, bytes, nbytes);
> +	i3c_readl_fifo(master->regs + IBI_QUEUE_STATUS, bytes, nbytes,
> +		       I3C_FIFO_LITTLE_ENDIAN);
>  }
>
>  static struct dw_i3c_xfer *
> diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c
> index 97b151564d3d..de3b5e894b4b 100644
> --- a/drivers/i3c/master/i3c-master-cdns.c
> +++ b/drivers/i3c/master/i3c-master-cdns.c
> @@ -428,13 +428,15 @@ to_cdns_i3c_master(struct i3c_master_controller *master)
>  static void cdns_i3c_master_wr_to_tx_fifo(struct cdns_i3c_master *master,
>  					  const u8 *bytes, int nbytes)
>  {
> -	i3c_writel_fifo(master->regs + TX_FIFO, bytes, nbytes);
> +	i3c_writel_fifo(master->regs + TX_FIFO, bytes, nbytes,
> +			I3C_FIFO_LITTLE_ENDIAN);
>  }
>
>  static void cdns_i3c_master_rd_from_rx_fifo(struct cdns_i3c_master *master,
>  					    u8 *bytes, int nbytes)
>  {
> -	i3c_readl_fifo(master->regs + RX_FIFO, bytes, nbytes);
> +	i3c_readl_fifo(master->regs + RX_FIFO, bytes, nbytes,
> +		       I3C_FIFO_LITTLE_ENDIAN);
>  }
>
>  static bool cdns_i3c_master_supports_ccc_cmd(struct i3c_master_controller *m,
> @@ -1319,7 +1321,8 @@ static void cdns_i3c_master_handle_ibi(struct cdns_i3c_master *master,
>  	buf = slot->data;
>
>  	nbytes = IBIR_XFER_BYTES(ibir);
> -	i3c_readl_fifo(master->regs + IBI_DATA_FIFO, buf, nbytes);
> +	i3c_readl_fifo(master->regs + IBI_DATA_FIFO, buf, nbytes,
> +		       I3C_FIFO_LITTLE_ENDIAN);
>
>  	slot->len = min_t(unsigned int, IBIR_XFER_BYTES(ibir),
>  			  dev->ibi->max_payload_len);
> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 174d3dc5d276..5610cf71740e 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -835,7 +835,8 @@ static int renesas_i3c_priv_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer
>  		}
>
>  		if (!i3c_xfers[i].rnw && i3c_xfers[i].len > 4) {
> -			i3c_writel_fifo(i3c->regs + NTDTBP0, cmd->tx_buf, cmd->len);
> +			i3c_writel_fifo(i3c->regs + NTDTBP0, cmd->tx_buf, cmd->len,
> +					I3C_FIFO_LITTLE_ENDIAN);
>  			if (cmd->len > NTDTBP0_DEPTH * sizeof(u32))
>  				renesas_set_bit(i3c->regs, NTIE, NTIE_TDBEIE0);
>  		}
> @@ -1021,7 +1022,8 @@ static irqreturn_t renesas_i3c_tx_isr(int irq, void *data)
>  			/* Clear the Transmit Buffer Empty status flag. */
>  			renesas_clear_bit(i3c->regs, NTST, NTST_TDBEF0);
>  		} else {
> -			i3c_writel_fifo(i3c->regs + NTDTBP0, cmd->tx_buf, cmd->len);
> +			i3c_writel_fifo(i3c->regs + NTDTBP0, cmd->tx_buf, cmd->len,
> +					I3C_FIFO_LITTLE_ENDIAN);
>  		}
>  	}
>
> @@ -1061,7 +1063,8 @@ static irqreturn_t renesas_i3c_resp_isr(int irq, void *data)
>  			if (NDBSTLV0_RDBLV(renesas_readl(i3c->regs, NDBSTLV0)) && !cmd->err)
>  				bytes_remaining = data_len - cmd->rx_count;
>
> -			i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, bytes_remaining);
> +			i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, bytes_remaining,
> +				       I3C_FIFO_LITTLE_ENDIAN);
>  			renesas_clear_bit(i3c->regs, NTIE, NTIE_RDBFIE0);
>  			break;
>  		default:
> @@ -1203,7 +1206,8 @@ static irqreturn_t renesas_i3c_rx_isr(int irq, void *data)
>  			cmd->i2c_bytes_left--;
>  		} else {
>  			read_bytes = NDBSTLV0_RDBLV(renesas_readl(i3c->regs, NDBSTLV0)) * sizeof(u32);
> -			i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, read_bytes);
> +			i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, read_bytes,
> +				       I3C_FIFO_LITTLE_ENDIAN);
>  			cmd->rx_count = read_bytes;
>  		}
>
> --
> 2.34.1
>

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo() and i3c_writel_fifo()

[Arnd Bergmann]

On Tue, Sep 23, 2025, at 17:45, Manikanta Guntupalli wrote:
>  /**
>   * i3c_writel_fifo - Write data buffer to 32bit FIFO
>   * @addr: FIFO Address to write to
>   * @buf: Pointer to the data bytes to write
>   * @nbytes: Number of bytes to write
> + * @endian: Endianness of FIFO write
>   */
>  static inline void i3c_writel_fifo(void __iomem *addr, const void *buf,
> -				   int nbytes)
> +				   int nbytes, enum i3c_fifo_endian endian)
>  {
> -	writesl(addr, buf, nbytes / 4);
> +	if (endian)
> +		writesl_be(addr, buf, nbytes / 4);
> +	else
> +		writesl(addr, buf, nbytes / 4);
> +

This seems counter-intuitive: a FIFO doesn't really have
an endianness, it is instead used to transfer a stream of
bytes, so if the device has a fixed endianess, the
FIFO still needs to be read using a plain writesl().

I see that your writesl_be() has an incorrect definition, which
would lead to the i3c_writel_fifo() function accidentally still
working if both the device and CPU use big-endian registers:

static inline void writesl_be(volatile void __iomem *addr,
			      const void *buffer,
			      unsigned int count)
{
	if (count) {
		const u32 *buf = buffer;
		do {
			__raw_writel((u32 __force)__cpu_to_be32(*buf), addr);
			buf++;
		} while (--count);
	}
}

The __cpu_to_be32() call that you add here means that the
FIFO data is swapped on little-endian CPUs but not swapped
on big-endian ones. Compare this to the normal writesl()
function that never swaps because it writes a byte stream.

>  	if (nbytes & 3) {
>  		u32 tmp = 0;
> 
>  		memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);
> -		writel(tmp, addr);
> +
> +		if (endian)
> +			writel_be(tmp, addr);
> +		else
> +			writel(tmp, addr);

This bit however seems to fix a bug, but does so in a
confusing way. The way the FIFO registers usually deal
with excess bytes is to put them into the first bytes
of the FIFO register, so this should just be a

       writesl(addr, &tmp, 1);

to write one set of four bytes into the FIFO without
endian-swapping.

Could it be that you are just trying to use a normal
i3c adapter with little-endian registers on a normal
big-endian machine but ran into this bug?

     Arnd

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo() and i3c_writel_fifo()

[Guntupalli, Manikanta]

[Public]

Hi,

> -----Original Message-----
> From: Arnd Bergmann <arnd@arndb.de>
> Sent: Wednesday, September 24, 2025 12:21 AM
> To: Guntupalli, Manikanta <manikanta.guntupalli@amd.com>; git (AMD-Xilinx)
> <git@amd.com>; Simek, Michal <michal.simek@amd.com>; Alexandre Belloni
> <alexandre.belloni@bootlin.com>; Frank Li <Frank.Li@nxp.com>; Rob Herring
> <robh@kernel.org>; krzk+dt@kernel.org; Conor Dooley <conor+dt@kernel.org>;
> Przemysław Gaj <pgaj@cadence.com>; Wolfram Sang <wsa+renesas@sang-
> engineering.com>; tommaso.merciai.xr@bp.renesas.com;
> quic_msavaliy@quicinc.com; S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>;
> Sakari Ailus <sakari.ailus@linux.intel.com>; 'billy_tsai@aspeedtech.com'
> <billy_tsai@aspeedtech.com>; Kees Cook <kees@kernel.org>; Gustavo A. R. Silva
> <gustavoars@kernel.org>; Jarkko Nikula <jarkko.nikula@linux.intel.com>; Jorge
> Marques <jorge.marques@analog.com>; linux-i3c@lists.infradead.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Linux-Arch <linux-
> arch@vger.kernel.org>; linux-hardening@vger.kernel.org
> Cc: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; Goud, Srinivas
> <srinivas.goud@amd.com>; Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>;
> manion05gk@gmail.com
> Subject: Re: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo()
> and i3c_writel_fifo()
>
> On Tue, Sep 23, 2025, at 17:45, Manikanta Guntupalli wrote:
> >  /**
> >   * i3c_writel_fifo - Write data buffer to 32bit FIFO
> >   * @addr: FIFO Address to write to
> >   * @buf: Pointer to the data bytes to write
> >   * @nbytes: Number of bytes to write
> > + * @endian: Endianness of FIFO write
> >   */
> >  static inline void i3c_writel_fifo(void __iomem *addr, const void *buf,
> > -                              int nbytes)
> > +                              int nbytes, enum i3c_fifo_endian endian)
> >  {
> > -   writesl(addr, buf, nbytes / 4);
> > +   if (endian)
> > +           writesl_be(addr, buf, nbytes / 4);
> > +   else
> > +           writesl(addr, buf, nbytes / 4);
> > +
>
> This seems counter-intuitive: a FIFO doesn't really have an endianness, it is instead
> used to transfer a stream of bytes, so if the device has a fixed endianess, the FIFO
> still needs to be read using a plain writesl().
>
> I see that your writesl_be() has an incorrect definition, which would lead to the
> i3c_writel_fifo() function accidentally still working if both the device and CPU use
> big-endian registers:
>
> static inline void writesl_be(volatile void __iomem *addr,
>                             const void *buffer,
>                             unsigned int count)
> {
>       if (count) {
>               const u32 *buf = buffer;
>               do {
>                       __raw_writel((u32 __force)__cpu_to_be32(*buf), addr);
>                       buf++;
>               } while (--count);
>       }
> }
>
> The __cpu_to_be32() call that you add here means that the FIFO data is swapped
> on little-endian CPUs but not swapped on big-endian ones. Compare this to the
> normal writesl() function that never swaps because it writes a byte stream.
The use of __cpu_to_be32() in writesl_be() is intentional. The goal here is to ensure that the FIFO is always accessed in big-endian format, regardless of whether the CPU is little-endian or big-endian. On little-endian CPUs, this introduces the required byte swap before writing; on big-endian CPUs, the data is already in big-endian order, so no additional swap occurs.
This guarantees that the FIFO sees consistent big-endian data, matching the hardware's expectation.

>
> >     if (nbytes & 3) {
> >             u32 tmp = 0;
> >
> >             memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);
> > -           writel(tmp, addr);
> > +
> > +           if (endian)
> > +                   writel_be(tmp, addr);
> > +           else
> > +                   writel(tmp, addr);
>
> This bit however seems to fix a bug, but does so in a confusing way. The way the
> FIFO registers usually deal with excess bytes is to put them into the first bytes of the
> FIFO register, so this should just be a
>
>        writesl(addr, &tmp, 1);
>
> to write one set of four bytes into the FIFO without endian-swapping.
>
> Could it be that you are just trying to use a normal i3c adapter with little-endian
> registers on a normal big-endian machine but ran into this bug?
The intention here is to enforce big-endian ordering for the trailing bytes as well. By using __cpu_to_be32() for full words and writel_be() for the leftover word, the FIFO is always accessed in big-endian format, regardless of the CPU's native endianness. This ensures consistent and correct data ordering from the device's perspective.

Thanks,
Manikanta.

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo() and i3c_writel_fifo()

[Arnd Bergmann]

On Wed, Sep 24, 2025, at 11:00, Guntupalli, Manikanta wrote:
>> >     if (nbytes & 3) {
>> >             u32 tmp = 0;
>> >
>> >             memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);
>> > -           writel(tmp, addr);
>> > +
>> > +           if (endian)
>> > +                   writel_be(tmp, addr);
>> > +           else
>> > +                   writel(tmp, addr);
>>
>> This bit however seems to fix a bug, but does so in a confusing way. The way the
>> FIFO registers usually deal with excess bytes is to put them into the first bytes of the
>> FIFO register, so this should just be a
>>
>>        writesl(addr, &tmp, 1);
>>
>> to write one set of four bytes into the FIFO without endian-swapping.
>>
>> Could it be that you are just trying to use a normal i3c adapter with little-endian
>> registers on a normal big-endian machine but ran into this bug?
> The intention here is to enforce big-endian ordering for the trailing 
> bytes as well. By using __cpu_to_be32() for full words and writel_be() 
> for the leftover word, the FIFO is always accessed in big-endian 
> format, regardless of the CPU's native endianness. This ensures 
> consistent and correct data ordering from the device's perspective.

No, this makes no sense: The 'else' portion is incorrect in the
function, and prevents it from working on all big-endian CPUs because
'writel()' only works for little-endian 32-bit registers. If you just
fix the bug as I described above, this will work correctly on any
driver calling the current function. At that point, your hack becomes
a nop for big-endian systems, while calling the function with
'endian = true' on little-endian kernels is still wrong.

Please start by fixing the existing bug and test that again.

I know that endianess with FIFO registers is hard to understand,
and everyone has to spend the time once to actually wrap their
head around this. Even if you don't believe me, please try the
bugfix below (without your added argument) and think about how
FIFO registers that transfer byte streams are different of
fixed-endian 32-bit registers. Note that your driver uses
little-endian readl() for normal registers, and this is portable
to both LE and BE kernels.

See also the explanation in 41739ee355ab ("asm-generic: io:
don't perform swab during {in,out} string functions").

     Arnd

diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
index 0d857cc68cc5..0f8a25cb71e7 100644
--- a/drivers/i3c/internals.h
+++ b/drivers/i3c/internals.h
@@ -38,7 +38,7 @@ static inline void i3c_writel_fifo(void __iomem *addr, const void *buf,
 		u32 tmp = 0;
 
 		memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);
-		writel(tmp, addr);
+		writesl(addr, &buf, 1);
 	}
 }
 
@@ -55,7 +55,7 @@ static inline void i3c_readl_fifo(const void __iomem *addr, void *buf,
 	if (nbytes & 3) {
 		u32 tmp;
 
-		tmp = readl(addr);
+		readsl(addr, &tmp, 1);
 		memcpy(buf + (nbytes & ~3), &tmp, nbytes & 3);
 	}
 }

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo() and i3c_writel_fifo()

[Guntupalli, Manikanta]

[Public]

Hi,

> -----Original Message-----
> From: Arnd Bergmann <arnd@arndb.de>
> Sent: Wednesday, September 24, 2025 3:30 PM
> To: Guntupalli, Manikanta <manikanta.guntupalli@amd.com>; git (AMD-Xilinx)
> <git@amd.com>; Simek, Michal <michal.simek@amd.com>; Alexandre Belloni
> <alexandre.belloni@bootlin.com>; Frank Li <Frank.Li@nxp.com>; Rob Herring
> <robh@kernel.org>; krzk+dt@kernel.org; Conor Dooley <conor+dt@kernel.org>;
> Przemysław Gaj <pgaj@cadence.com>; Wolfram Sang <wsa+renesas@sang-
> engineering.com>; tommaso.merciai.xr@bp.renesas.com;
> quic_msavaliy@quicinc.com; S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>;
> Sakari Ailus <sakari.ailus@linux.intel.com>; 'billy_tsai@aspeedtech.com'
> <billy_tsai@aspeedtech.com>; Kees Cook <kees@kernel.org>; Gustavo A. R. Silva
> <gustavoars@kernel.org>; Jarkko Nikula <jarkko.nikula@linux.intel.com>; Jorge
> Marques <jorge.marques@analog.com>; linux-i3c@lists.infradead.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Linux-Arch <linux-
> arch@vger.kernel.org>; linux-hardening@vger.kernel.org
> Cc: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; Goud, Srinivas
> <srinivas.goud@amd.com>; Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>;
> manion05gk@gmail.com
> Subject: Re: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo()
> and i3c_writel_fifo()
>
> On Wed, Sep 24, 2025, at 11:00, Guntupalli, Manikanta wrote:
> >> >     if (nbytes & 3) {
> >> >             u32 tmp = 0;
> >> >
> >> >             memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);
> >> > -           writel(tmp, addr);
> >> > +
> >> > +           if (endian)
> >> > +                   writel_be(tmp, addr);
> >> > +           else
> >> > +                   writel(tmp, addr);
> >>
> >> This bit however seems to fix a bug, but does so in a confusing way.
> >> The way the FIFO registers usually deal with excess bytes is to put
> >> them into the first bytes of the FIFO register, so this should just
> >> be a
> >>
> >>        writesl(addr, &tmp, 1);
> >>
> >> to write one set of four bytes into the FIFO without endian-swapping.
> >>
> >> Could it be that you are just trying to use a normal i3c adapter with
> >> little-endian registers on a normal big-endian machine but ran into this bug?
> > The intention here is to enforce big-endian ordering for the trailing
> > bytes as well. By using __cpu_to_be32() for full words and writel_be()
> > for the leftover word, the FIFO is always accessed in big-endian
> > format, regardless of the CPU's native endianness. This ensures
> > consistent and correct data ordering from the device's perspective.
>
> No, this makes no sense: The 'else' portion is incorrect in the function, and prevents
> it from working on all big-endian CPUs because 'writel()' only works for little-endian
> 32-bit registers. If you just fix the bug as I described above, this will work correctly on
> any driver calling the current function. At that point, your hack becomes a nop for big-
> endian systems, while calling the function with 'endian = true' on little-endian kernels
> is still wrong.
>
> Please start by fixing the existing bug and test that again.
>
> I know that endianess with FIFO registers is hard to understand, and everyone has
> to spend the time once to actually wrap their head around this. Even if you don't
> believe me, please try the bugfix below (without your added argument) and think
> about how FIFO registers that transfer byte streams are different of fixed-endian 32-
> bit registers. Note that your driver uses little-endian readl() for normal registers, and
> this is portable to both LE and BE kernels.
>
> See also the explanation in 41739ee355ab ("asm-generic: io:
> don't perform swab during {in,out} string functions").
>
>      Arnd
>
> diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h index
> 0d857cc68cc5..0f8a25cb71e7 100644
> --- a/drivers/i3c/internals.h
> +++ b/drivers/i3c/internals.h
> @@ -38,7 +38,7 @@ static inline void i3c_writel_fifo(void __iomem *addr, const
> void *buf,
>               u32 tmp = 0;
>
>               memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);
> -             writel(tmp, addr);
> +             writesl(addr, &buf, 1);
>       }
>  }
>
> @@ -55,7 +55,7 @@ static inline void i3c_readl_fifo(const void __iomem *addr, void
> *buf,
>       if (nbytes & 3) {
>               u32 tmp;
>
> -             tmp = readl(addr);
> +             readsl(addr, &tmp, 1);
>               memcpy(buf + (nbytes & ~3), &tmp, nbytes & 3);
>       }
>  }

We have not observed any issue on little-endian systems in our testing so far (as I mentioned earlier in asm-generic/io.h: Add big-endian MMIO accessors).

That said, I understand your point about FIFO semantics being different from fixed-endian registers. To cover both cases, we considered using writesl() for little-endian and introducing a writesl_be() helper for big-endian, as shown below:

static inline void i3c_writel_fifo(void __iomem *addr, const void *buf,
                                   int nbytes, enum i3c_fifo_endian endian)
{
        if (endian)
                writesl_be(addr, buf, nbytes / 4);
        else
                writesl(addr, buf, nbytes / 4);

        if (nbytes & 3) {
                u32 tmp = 0;

                memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);

                if (endian)
                        writesl_be(addr, &tmp, 1);
                else
                        writesl(addr, &tmp, 1);
        }
}

With this approach, both little-endian and big-endian cases works as expected.

Thanks,
Manikanta.

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo() and i3c_writel_fifo()

[Arnd Bergmann]

On Wed, Sep 24, 2025, at 14:22, Guntupalli, Manikanta wrote:

>> @@ -55,7 +55,7 @@ static inline void i3c_readl_fifo(const void __iomem *addr, void
>> *buf,
>>       if (nbytes & 3) {
>>               u32 tmp;
>>
>> -             tmp = readl(addr);
>> +             readsl(addr, &tmp, 1);
>>               memcpy(buf + (nbytes & ~3), &tmp, nbytes & 3);
>>       }
>>  }
>
> We have not observed any issue on little-endian systems in our testing 
> so far (as I mentioned earlier in asm-generic/io.h: Add big-endian MMIO 
> accessors).

Did you test the little-endian system with the 'endian' flag set
to I3C_FIFO_BIG_ENDIAN though? Your v7 code will still work on
little-endian kernels if that flag is set to I3C_FIFO_LITTLE_ENDIAN,
and it will also work on big-endian kernels if the flag is
set to I3C_FIFO_BIG_ENDIAN. But is broken for the other two:

- on little-endian kernels with I3C_FIFO_BIG_ENDIAN, the entire
  data buffer is byteswapped in 32-bit chunks

- on big-endian kernels with I3C_FIFO_LITTLE_ENDIAN, you run into
  the existing bug of the swapped tail word.

> That said, I understand your point about FIFO semantics being different 
> from fixed-endian registers. To cover both cases, we considered using 
> writesl() for little-endian and introducing a writesl_be() helper for 
> big-endian, as shown below:
>
> static inline void i3c_writel_fifo(void __iomem *addr, const void *buf,
>                                    int nbytes, enum i3c_fifo_endian endian)
> {
>         if (endian)
>                 writesl_be(addr, buf, nbytes / 4);
>         else
>                 writesl(addr, buf, nbytes / 4);
>
>         if (nbytes & 3) {
>                 u32 tmp = 0;
>
>                 memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);
>
>                 if (endian)
>                         writesl_be(addr, &tmp, 1);
>                 else
>                         writesl(addr, &tmp, 1);
>         }
> }
>
> With this approach, both little-endian and big-endian cases works as expected.

This version should fix the cases where you have a big-endian
kernel with either I3C_FIFO_BIG_ENDIAN or I3C_FIFO_LITTLE_ENDIAN,
as neither combination does any byte swaps.

However I'm fairly sure it's still broken for little-endian
kernels when a driver asks for a I3C_FIFO_BIG_ENDIAN conversion,
same as v7.

     Arnd

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo() and i3c_writel_fifo()

[Guntupalli, Manikanta]

[Public]

Hi,

> -----Original Message-----
> From: Arnd Bergmann <arnd@arndb.de>
> Sent: Wednesday, September 24, 2025 7:35 PM
> To: Guntupalli, Manikanta <manikanta.guntupalli@amd.com>; git (AMD-Xilinx)
> <git@amd.com>; Simek, Michal <michal.simek@amd.com>; Alexandre Belloni
> <alexandre.belloni@bootlin.com>; Frank Li <Frank.Li@nxp.com>; Rob Herring
> <robh@kernel.org>; krzk+dt@kernel.org; Conor Dooley <conor+dt@kernel.org>;
> Przemysław Gaj <pgaj@cadence.com>; Wolfram Sang <wsa+renesas@sang-
> engineering.com>; tommaso.merciai.xr@bp.renesas.com;
> quic_msavaliy@quicinc.com; S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>;
> Sakari Ailus <sakari.ailus@linux.intel.com>; 'billy_tsai@aspeedtech.com'
> <billy_tsai@aspeedtech.com>; Kees Cook <kees@kernel.org>; Gustavo A. R. Silva
> <gustavoars@kernel.org>; Jarkko Nikula <jarkko.nikula@linux.intel.com>; Jorge
> Marques <jorge.marques@analog.com>; linux-i3c@lists.infradead.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Linux-Arch <linux-
> arch@vger.kernel.org>; linux-hardening@vger.kernel.org
> Cc: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; Goud, Srinivas
> <srinivas.goud@amd.com>; Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>;
> manion05gk@gmail.com
> Subject: Re: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo()
> and i3c_writel_fifo()
>
> On Wed, Sep 24, 2025, at 14:22, Guntupalli, Manikanta wrote:
>
> >> @@ -55,7 +55,7 @@ static inline void i3c_readl_fifo(const void
> >> __iomem *addr, void *buf,
> >>       if (nbytes & 3) {
> >>               u32 tmp;
> >>
> >> -             tmp = readl(addr);
> >> +             readsl(addr, &tmp, 1);
> >>               memcpy(buf + (nbytes & ~3), &tmp, nbytes & 3);
> >>       }
> >>  }
> >
> > We have not observed any issue on little-endian systems in our testing
> > so far (as I mentioned earlier in asm-generic/io.h: Add big-endian
> > MMIO accessors).
>
> Did you test the little-endian system with the 'endian' flag set to
> I3C_FIFO_BIG_ENDIAN though?
Yes.

Your v7 code will still work on little-endian kernels
> if that flag is set to I3C_FIFO_LITTLE_ENDIAN, and it will also work on big-endian
> kernels if the flag is set to I3C_FIFO_BIG_ENDIAN. But is broken for the other two:
>
> - on little-endian kernels with I3C_FIFO_BIG_ENDIAN, the entire
>   data buffer is byteswapped in 32-bit chunks
>
> - on big-endian kernels with I3C_FIFO_LITTLE_ENDIAN, you run into
>   the existing bug of the swapped tail word.
>
> > That said, I understand your point about FIFO semantics being
> > different from fixed-endian registers. To cover both cases, we
> > considered using
> > writesl() for little-endian and introducing a writesl_be() helper for
> > big-endian, as shown below:
> >
> > static inline void i3c_writel_fifo(void __iomem *addr, const void *buf,
> >                                    int nbytes, enum i3c_fifo_endian
> > endian) {
> >         if (endian)
> >                 writesl_be(addr, buf, nbytes / 4);
> >         else
> >                 writesl(addr, buf, nbytes / 4);
> >
> >         if (nbytes & 3) {
> >                 u32 tmp = 0;
> >
> >                 memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);
> >
> >                 if (endian)
> >                         writesl_be(addr, &tmp, 1);
> >                 else
> >                         writesl(addr, &tmp, 1);
> >         }
> > }
> >
> > With this approach, both little-endian and big-endian cases works as expected.
>
> This version should fix the cases where you have a big-endian kernel with either
> I3C_FIFO_BIG_ENDIAN or I3C_FIFO_LITTLE_ENDIAN, as neither combination
> does any byte swaps.
>
> However I'm fairly sure it's still broken for little-endian kernels when a driver asks for
> a I3C_FIFO_BIG_ENDIAN conversion, same as v7.
We tested using the I3C_FIFO_BIG_ENDIAN flag from the driver on little-endian kernels, and it works as expected.

Thanks,
Manikanta.

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo() and i3c_writel_fifo()

[Arnd Bergmann]

On Wed, Sep 24, 2025, at 17:23, Guntupalli, Manikanta wrote:
>> Subject: Re: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo() and i3c_writel_fifo()
>> > }
>> >
>> > With this approach, both little-endian and big-endian cases works as expected.
>>
>> This version should fix the cases where you have a big-endian kernel with either
>> I3C_FIFO_BIG_ENDIAN or I3C_FIFO_LITTLE_ENDIAN, as neither combination
>> does any byte swaps.
>>
>> However I'm fairly sure it's still broken for little-endian kernels when a driver asks for
>> a I3C_FIFO_BIG_ENDIAN conversion, same as v7.
> We tested using the I3C_FIFO_BIG_ENDIAN flag from the driver on 
> little-endian kernels, and it works as expected.

Can you explain how that works? What I see is that your
readsl_be()/writesl_be() functions do a byteswap on
every four bytes, so the bytestream that gets copied
to/from the FIFO gets garbled, in particular the
final (unaligned) bytes of the kernel buffer end up
in the higher bytes of the FIFO register rather than
the first bytes as they do on a big-endian kernel.

Are both the big-endian and little-endian kernels in
your tests on microblaze, using the upstream version
of asm/io.h? Is there a hardware byteswap between the
CPU local bus and the i3c controller? If there is
one, is it set the same way for both kernels?

    Arnd

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo() and i3c_writel_fifo()

[Guntupalli, Manikanta]

[Public]

Hi,

> -----Original Message-----
> From: Arnd Bergmann <arnd@arndb.de>
> Sent: Wednesday, September 24, 2025 9:13 PM
> To: Guntupalli, Manikanta <manikanta.guntupalli@amd.com>; git (AMD-Xilinx)
> <git@amd.com>; Simek, Michal <michal.simek@amd.com>; Alexandre Belloni
> <alexandre.belloni@bootlin.com>; Frank Li <Frank.Li@nxp.com>; Rob Herring
> <robh@kernel.org>; krzk+dt@kernel.org; Conor Dooley <conor+dt@kernel.org>;
> Przemysław Gaj <pgaj@cadence.com>; Wolfram Sang <wsa+renesas@sang-
> engineering.com>; tommaso.merciai.xr@bp.renesas.com;
> quic_msavaliy@quicinc.com; S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>;
> Sakari Ailus <sakari.ailus@linux.intel.com>; 'billy_tsai@aspeedtech.com'
> <billy_tsai@aspeedtech.com>; Kees Cook <kees@kernel.org>; Gustavo A. R. Silva
> <gustavoars@kernel.org>; Jarkko Nikula <jarkko.nikula@linux.intel.com>; Jorge
> Marques <jorge.marques@analog.com>; linux-i3c@lists.infradead.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Linux-Arch <linux-
> arch@vger.kernel.org>; linux-hardening@vger.kernel.org
> Cc: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; Goud, Srinivas
> <srinivas.goud@amd.com>; Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>;
> manion05gk@gmail.com
> Subject: Re: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo()
> and i3c_writel_fifo()
>
> On Wed, Sep 24, 2025, at 17:23, Guntupalli, Manikanta wrote:
> >> Subject: Re: [PATCH V7 3/4] i3c: master: Add endianness support for
> >> i3c_readl_fifo() and i3c_writel_fifo()
> >> > }
> >> >
> >> > With this approach, both little-endian and big-endian cases works as expected.
> >>
> >> This version should fix the cases where you have a big-endian kernel
> >> with either I3C_FIFO_BIG_ENDIAN or I3C_FIFO_LITTLE_ENDIAN, as neither
> >> combination does any byte swaps.
> >>
> >> However I'm fairly sure it's still broken for little-endian kernels
> >> when a driver asks for a I3C_FIFO_BIG_ENDIAN conversion, same as v7.
> > We tested using the I3C_FIFO_BIG_ENDIAN flag from the driver on
> > little-endian kernels, and it works as expected.
>
> Can you explain how that works? What I see is that your
> readsl_be()/writesl_be() functions do a byteswap on every four bytes, so the
> bytestream that gets copied to/from the FIFO gets garbled, in particular the final
> (unaligned) bytes of the kernel buffer end up in the higher bytes of the FIFO register
> rather than the first bytes as they do on a big-endian kernel.
>
> Are both the big-endian and little-endian kernels in your tests on microblaze, using
> the upstream version of asm/io.h? Is there a hardware byteswap between the CPU
> local bus and the i3c controller? If there is one, is it set the same way for both
> kernels?
>
To clarify, my testing was performed on the latest upstream kernel on a ZCU102 (Zynq UltraScale+ MPSoC, Cortex-A53, little-endian) with big-endian FIFOs and no bus-level byteswap. For more details, please refer to my reply in Re: [PATCH] [v2] i3c: fix big-endian FIFO transfers.

Please don't take this as negative or aggressive-my intention is purely to learn and ensure it works correctly in all cases.

Thanks,
Manikanta.

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo() and i3c_writel_fifo()

[Arnd Bergmann]

On Thu, Sep 25, 2025, at 11:26, Guntupalli, Manikanta wrote:

>> Can you explain how that works? What I see is that your
>> readsl_be()/writesl_be() functions do a byteswap on every four bytes, so the
>> bytestream that gets copied to/from the FIFO gets garbled, in particular the final
>> (unaligned) bytes of the kernel buffer end up in the higher bytes of the FIFO register
>> rather than the first bytes as they do on a big-endian kernel.
>>
>> Are both the big-endian and little-endian kernels in your tests on microblaze, using
>> the upstream version of asm/io.h? Is there a hardware byteswap between the CPU
>> local bus and the i3c controller? If there is one, is it set the same way for both
>> kernels?
>>
> To clarify, my testing was performed on the latest upstream kernel on a 
> ZCU102 (Zynq UltraScale+ MPSoC, Cortex-A53, little-endian) with 
> big-endian FIFOs and no bus-level byteswap. For more details, please 
> refer to my reply in Re: [PATCH] [v2] i3c: fix big-endian FIFO 
> transfers.

Ok, thanks fro the clarification. I got confused by your description
mentioning big-endian in [PATCH V7 3/4] and assumed this would be on
a big-endian microblaze CPU, after I saw that the original i3c_readl_fifo()
had a bug in that configuration that your patch addressed and this
being a driver for a xilinx design. That fix just turned out unrelated
to what you were actually trying to do ;-)

> Please don't take this as negative or aggressive-my intention is purely 
> to learn and ensure it works correctly in all cases.

No worries, I should not have jumped to conclusions myself based
on what I saw in your implementation and assumed that fixing the
one bug would address your problem as well.

I do understand that your driver clearly needs a special case,
we just need to come to a conclusion what exactly the hardware
does and how to best deal with it. This is partly about whether
you may be able to use an external DMA engine such as
xlnx,zynqmp-dma-1.0 or xlnx,zynqmp-dpdma, and whether that would
need the same byteswap.

If you already plan to add that support, you likely need to
allocate a bounce buffer and byteswap the data in place
to have it copied in and out of the FIFO, and when you have
that, you can use the regular i3c_readl_fifo() unchanged.
If you are sure that the i3c controller is not going to be
used with DMA, or if the FIFO register as seen by the DMA
master does not require a byteswap, having a local helper
for the transfer is likely easier.

     Arnd

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo() and i3c_writel_fifo()

[Guntupalli, Manikanta]

[Public]

Hi,

> -----Original Message-----
> From: Arnd Bergmann <arnd@arndb.de>
> Sent: Thursday, September 25, 2025 5:45 PM
> To: Guntupalli, Manikanta <manikanta.guntupalli@amd.com>; git (AMD-Xilinx)
> <git@amd.com>; Simek, Michal <michal.simek@amd.com>; Alexandre Belloni
> <alexandre.belloni@bootlin.com>; Frank Li <Frank.Li@nxp.com>; Rob Herring
> <robh@kernel.org>; krzk+dt@kernel.org; Conor Dooley <conor+dt@kernel.org>;
> Przemysław Gaj <pgaj@cadence.com>; Wolfram Sang <wsa+renesas@sang-
> engineering.com>; tommaso.merciai.xr@bp.renesas.com;
> quic_msavaliy@quicinc.com; S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>;
> Sakari Ailus <sakari.ailus@linux.intel.com>; 'billy_tsai@aspeedtech.com'
> <billy_tsai@aspeedtech.com>; Kees Cook <kees@kernel.org>; Gustavo A. R. Silva
> <gustavoars@kernel.org>; Jarkko Nikula <jarkko.nikula@linux.intel.com>; Jorge
> Marques <jorge.marques@analog.com>; linux-i3c@lists.infradead.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Linux-Arch <linux-
> arch@vger.kernel.org>; linux-hardening@vger.kernel.org
> Cc: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; Goud, Srinivas
> <srinivas.goud@amd.com>; Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>;
> manion05gk@gmail.com
> Subject: Re: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo()
> and i3c_writel_fifo()
>
> On Thu, Sep 25, 2025, at 11:26, Guntupalli, Manikanta wrote:
>
> >> Can you explain how that works? What I see is that your
> >> readsl_be()/writesl_be() functions do a byteswap on every four bytes,
> >> so the bytestream that gets copied to/from the FIFO gets garbled, in
> >> particular the final
> >> (unaligned) bytes of the kernel buffer end up in the higher bytes of
> >> the FIFO register rather than the first bytes as they do on a big-endian kernel.
> >>
> >> Are both the big-endian and little-endian kernels in your tests on
> >> microblaze, using the upstream version of asm/io.h? Is there a
> >> hardware byteswap between the CPU local bus and the i3c controller?
> >> If there is one, is it set the same way for both kernels?
> >>
> > To clarify, my testing was performed on the latest upstream kernel on
> > a
> > ZCU102 (Zynq UltraScale+ MPSoC, Cortex-A53, little-endian) with
> > big-endian FIFOs and no bus-level byteswap. For more details, please
> > refer to my reply in Re: [PATCH] [v2] i3c: fix big-endian FIFO
> > transfers.
>
> Ok, thanks fro the clarification. I got confused by your description mentioning big-
> endian in [PATCH V7 3/4] and assumed this would be on a big-endian microblaze
> CPU, after I saw that the original i3c_readl_fifo() had a bug in that configuration that
> your patch addressed and this being a driver for a xilinx design. That fix just turned
> out unrelated to what you were actually trying to do ;-)
>
> > Please don't take this as negative or aggressive-my intention is
> > purely to learn and ensure it works correctly in all cases.
>
> No worries, I should not have jumped to conclusions myself based on what I saw in
> your implementation and assumed that fixing the one bug would address your
> problem as well.
>
> I do understand that your driver clearly needs a special case, we just need to come
> to a conclusion what exactly the hardware does and how to best deal with it. This is
> partly about whether you may be able to use an external DMA engine such as
> xlnx,zynqmp-dma-1.0 or xlnx,zynqmp-dpdma, and whether that would need the
> same byteswap.
>
> If you already plan to add that support, you likely need to allocate a bounce buffer
> and byteswap the data in place to have it copied in and out of the FIFO, and when
> you have that, you can use the regular i3c_readl_fifo() unchanged.
> If you are sure that the i3c controller is not going to be used with DMA, or if the FIFO
> register as seen by the DMA master does not require a byteswap, having a local
> helper for the transfer is likely easier.
>
Thanks for understanding.

The I3C controller is not going to be used with DMA in our case.

We had initially implemented local helpers, but based on Frank Li's suggestion, we added the support in i3c_readl_fifo() and i3c_writel_fifo() to make it more generic and beneficial for others as well. That's why we included it here.

Thanks,
Manikanta.

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo() and i3c_writel_fifo()

[Frank Li]

On Thu, Sep 25, 2025 at 04:37:54PM +0000, Guntupalli, Manikanta wrote:
> [Public]
>
> Hi,
>
> > -----Original Message-----
> > From: Arnd Bergmann <arnd@arndb.de>
> > Sent: Thursday, September 25, 2025 5:45 PM
> > To: Guntupalli, Manikanta <manikanta.guntupalli@amd.com>; git (AMD-Xilinx)
> > <git@amd.com>; Simek, Michal <michal.simek@amd.com>; Alexandre Belloni
> > <alexandre.belloni@bootlin.com>; Frank Li <Frank.Li@nxp.com>; Rob Herring
> > <robh@kernel.org>; krzk+dt@kernel.org; Conor Dooley <conor+dt@kernel.org>;
> > Przemysław Gaj <pgaj@cadence.com>; Wolfram Sang <wsa+renesas@sang-
> > engineering.com>; tommaso.merciai.xr@bp.renesas.com;
> > quic_msavaliy@quicinc.com; S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>;
> > Sakari Ailus <sakari.ailus@linux.intel.com>; 'billy_tsai@aspeedtech.com'
> > <billy_tsai@aspeedtech.com>; Kees Cook <kees@kernel.org>; Gustavo A. R. Silva
> > <gustavoars@kernel.org>; Jarkko Nikula <jarkko.nikula@linux.intel.com>; Jorge
> > Marques <jorge.marques@analog.com>; linux-i3c@lists.infradead.org;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Linux-Arch <linux-
> > arch@vger.kernel.org>; linux-hardening@vger.kernel.org
> > Cc: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; Goud, Srinivas
> > <srinivas.goud@amd.com>; Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>;
> > manion05gk@gmail.com
> > Subject: Re: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo()
> > and i3c_writel_fifo()
> >
> > On Thu, Sep 25, 2025, at 11:26, Guntupalli, Manikanta wrote:
> >
> > >> Can you explain how that works? What I see is that your
> > >> readsl_be()/writesl_be() functions do a byteswap on every four bytes,
> > >> so the bytestream that gets copied to/from the FIFO gets garbled, in
> > >> particular the final
> > >> (unaligned) bytes of the kernel buffer end up in the higher bytes of
> > >> the FIFO register rather than the first bytes as they do on a big-endian kernel.
> > >>
> > >> Are both the big-endian and little-endian kernels in your tests on
> > >> microblaze, using the upstream version of asm/io.h? Is there a
> > >> hardware byteswap between the CPU local bus and the i3c controller?
> > >> If there is one, is it set the same way for both kernels?
> > >>
> > > To clarify, my testing was performed on the latest upstream kernel on
> > > a
> > > ZCU102 (Zynq UltraScale+ MPSoC, Cortex-A53, little-endian) with
> > > big-endian FIFOs and no bus-level byteswap. For more details, please
> > > refer to my reply in Re: [PATCH] [v2] i3c: fix big-endian FIFO
> > > transfers.
> >
> > Ok, thanks fro the clarification. I got confused by your description mentioning big-
> > endian in [PATCH V7 3/4] and assumed this would be on a big-endian microblaze
> > CPU, after I saw that the original i3c_readl_fifo() had a bug in that configuration that
> > your patch addressed and this being a driver for a xilinx design. That fix just turned
> > out unrelated to what you were actually trying to do ;-)
> >
> > > Please don't take this as negative or aggressive-my intention is
> > > purely to learn and ensure it works correctly in all cases.
> >
> > No worries, I should not have jumped to conclusions myself based on what I saw in
> > your implementation and assumed that fixing the one bug would address your
> > problem as well.
> >
> > I do understand that your driver clearly needs a special case, we just need to come
> > to a conclusion what exactly the hardware does and how to best deal with it. This is
> > partly about whether you may be able to use an external DMA engine such as
> > xlnx,zynqmp-dma-1.0 or xlnx,zynqmp-dpdma, and whether that would need the
> > same byteswap.
> >
> > If you already plan to add that support, you likely need to allocate a bounce buffer
> > and byteswap the data in place to have it copied in and out of the FIFO, and when
> > you have that, you can use the regular i3c_readl_fifo() unchanged.
> > If you are sure that the i3c controller is not going to be used with DMA, or if the FIFO
> > register as seen by the DMA master does not require a byteswap, having a local
> > helper for the transfer is likely easier.
> >
> Thanks for understanding.
>
> The I3C controller is not going to be used with DMA in our case.
>
> We had initially implemented local helpers, but based on Frank Li's suggestion, we added the support in i3c_readl_fifo() and i3c_writel_fifo() to make it more generic and beneficial for others as well. That's why we included it here.

Yes, 32 bit FIFO generally two orders

BIT 31..24, 23..16, 15..8, 7..0
    B3      B2      B1     B0
    A0      A1      A2     A3

Need write/read to memory as

0x00     B0       A0
0x01     B1       A1
0x02     B2       A2
0x03     B3       A3

We don't expect every driver need duplicate to handle these two kind order
and trail data. Just need clear API defination to avoid confusiton at
difference cpu endian system.

Frank

>
> Thanks,
> Manikanta.

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo() and i3c_writel_fifo()

[kernel test robot]

Hi Manikanta,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master arnd-asm-generic/master v6.17-rc7 next-20250924]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Manikanta-Guntupalli/dt-bindings-i3c-Add-AMD-I3C-master-controller-support/20250923-234944
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20250923154551.2112388-4-manikanta.guntupalli%40amd.com
patch subject: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo() and i3c_writel_fifo()
config: mips-randconfig-r123-20250925 (https://download.01.org/0day-ci/archive/20250925/202509252022.QvbNmJil-lkp@intel.com/config)
compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project cafc064fc7a96b3979a023ddae1da2b499d6c954)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250925/202509252022.QvbNmJil-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202509252022.QvbNmJil-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
   drivers/i3c/master/i3c-master-cdns.c: note: in included file:
>> drivers/i3c/master/../internals.h:53:25: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected unsigned int [usertype] val @@     got restricted __be32 [usertype] @@
   drivers/i3c/master/../internals.h:53:25: sparse:     expected unsigned int [usertype] val
   drivers/i3c/master/../internals.h:53:25: sparse:     got restricted __be32 [usertype]
>> drivers/i3c/master/../internals.h:53:25: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void volatile [noderef] __iomem *mem @@     got unsigned int * @@
   drivers/i3c/master/../internals.h:53:25: sparse:     expected void volatile [noderef] __iomem *mem
   drivers/i3c/master/../internals.h:53:25: sparse:     got unsigned int *
>> drivers/i3c/master/../internals.h:78:31: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] __iomem *mem @@     got unsigned int * @@
   drivers/i3c/master/../internals.h:78:31: sparse:     expected void const volatile [noderef] __iomem *mem
   drivers/i3c/master/../internals.h:78:31: sparse:     got unsigned int *
>> drivers/i3c/master/../internals.h:78:31: sparse: sparse: cast to restricted __be32
>> drivers/i3c/master/../internals.h:78:31: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] __iomem *mem @@     got unsigned int * @@
   drivers/i3c/master/../internals.h:78:31: sparse:     expected void const volatile [noderef] __iomem *mem
   drivers/i3c/master/../internals.h:78:31: sparse:     got unsigned int *
>> drivers/i3c/master/../internals.h:78:31: sparse: sparse: cast to restricted __be32

vim +53 drivers/i3c/master/../internals.h

    31	
    32	/**
    33	 * i3c_writel_fifo - Write data buffer to 32bit FIFO
    34	 * @addr: FIFO Address to write to
    35	 * @buf: Pointer to the data bytes to write
    36	 * @nbytes: Number of bytes to write
    37	 * @endian: Endianness of FIFO write
    38	 */
    39	static inline void i3c_writel_fifo(void __iomem *addr, const void *buf,
    40					   int nbytes, enum i3c_fifo_endian endian)
    41	{
    42		if (endian)
    43			writesl_be(addr, buf, nbytes / 4);
    44		else
    45			writesl(addr, buf, nbytes / 4);
    46	
    47		if (nbytes & 3) {
    48			u32 tmp = 0;
    49	
    50			memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);
    51	
    52			if (endian)
  > 53				writel_be(tmp, addr);
    54			else
    55				writel(tmp, addr);
    56		}
    57	}
    58	
    59	/**
    60	 * i3c_readl_fifo - Read data buffer from 32bit FIFO
    61	 * @addr: FIFO Address to read from
    62	 * @buf: Pointer to the buffer to store read bytes
    63	 * @nbytes: Number of bytes to read
    64	 * @endian: Endianness of FIFO read
    65	 */
    66	static inline void i3c_readl_fifo(const void __iomem *addr, void *buf,
    67					  int nbytes, enum i3c_fifo_endian endian)
    68	{
    69		if (endian)
    70			readsl_be(addr, buf, nbytes / 4);
    71		else
    72			readsl(addr, buf, nbytes / 4);
    73	
    74		if (nbytes & 3) {
    75			u32 tmp;
    76	
    77			if (endian)
  > 78				tmp = readl_be(addr);
    79			else
    80				tmp = readl(addr);
    81	
    82			memcpy(buf + (nbytes & ~3), &tmp, nbytes & 3);
    83		}
    84	}
    85	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH V7 4/4] i3c: master: Add AMD I3C bus controller driver

[Manikanta Guntupalli]

Add an I3C master driver and maintainers fragment for the AMD I3C bus
controller.

The driver currently supports the I3C bus operating in SDR i3c mode,
with features including Dynamic Address Assignment, private data transfers,
and CCC transfers in both broadcast and direct modes. It also supports
operation in I2C mode.

Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
---
Changes for V2:
Updated commit description.
Added mixed mode support with clock configuration.
Converted smaller functions into inline functions.
Used FIELD_GET() in xi3c_get_response().
Updated xi3c_master_rd_from_rx_fifo() to use cmd->rx_buf.
Used parity8() for address parity calculation.
Added guards for locks.
Dropped num_targets and updated xi3c_master_do_daa().
Used __free(kfree) in xi3c_master_send_bdcast_ccc_cmd().
Dropped PM runtime support.
Updated xi3c_master_read() and xi3c_master_write() with
xi3c_is_resp_available() check.
Created separate functions: xi3c_master_init() and xi3c_master_reinit().
Used xi3c_master_init() in bus initialization and xi3c_master_reinit()
in error paths.
Added DAA structure to xi3c_master structure.

Changes for V3:
Resolved merge conflicts.

Changes for V4:
Updated timeout macros.
Removed type casting for xi3c_is_resp_available() macro.
Used ioread32() and iowrite32() instead of readl() and writel()
to keep consistency.
Read XI3C_RESET_OFFSET reg before udelay().
Removed xi3c_master_free_xfer() and directly used kfree().
Skipped checking return value of i3c_master_add_i3c_dev_locked().
Used devm_mutex_init() instead of mutex_init().

Changes for V5:
Used GENMASK_ULL for PID mask as it's 64bit mask.

Changes for V6:
Removed typecast for xi3c_getrevisionnumber(), xi3c_wrfifolevel(),
and xi3c_rdfifolevel().
Replaced dynamic allocation with a static variable for pid_bcr_dcr.
Fixed sparse warning in do_daa by typecasting the address parity value
to u8.
Fixed sparse warning in xi3c_master_bus_init by typecasting the pid value
to u64 in info.pid calculation.

Changes for V7:
Updated timeout macro name.
Updated xi3c_master_wr_to_tx_fifo() and xi3c_master_rd_from_rx_fifo()
to use i3c_writel_fifo() and i3c_readl_fifo().
---
 MAINTAINERS                         |    7 +
 drivers/i3c/master/Kconfig          |   16 +
 drivers/i3c/master/Makefile         |    1 +
 drivers/i3c/master/amd-i3c-master.c | 1012 +++++++++++++++++++++++++++
 4 files changed, 1036 insertions(+)
 create mode 100644 drivers/i3c/master/amd-i3c-master.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 8886d66bd824..fe88efb41f5b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11782,6 +11782,13 @@ L:	linux-i2c@vger.kernel.org
 S:	Maintained
 F:	drivers/i2c/i2c-stub.c
 
+I3C DRIVER FOR AMD AXI I3C MASTER
+M:	Manikanta Guntupalli <manikanta.guntupalli@amd.com>
+R:	Michal Simek <michal.simek@amd.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/i3c/xlnx,axi-i3c.yaml
+F:	drivers/i3c/master/amd-i3c-master.c
+
 I3C DRIVER FOR ASPEED AST2600
 M:	Jeremy Kerr <jk@codeconstruct.com.au>
 S:	Maintained
diff --git a/drivers/i3c/master/Kconfig b/drivers/i3c/master/Kconfig
index 13df2944f2ec..4b884a678893 100644
--- a/drivers/i3c/master/Kconfig
+++ b/drivers/i3c/master/Kconfig
@@ -1,4 +1,20 @@
 # SPDX-License-Identifier: GPL-2.0-only
+
+config AMD_I3C_MASTER
+	tristate "AMD I3C Master driver"
+	depends on I3C
+	depends on HAS_IOMEM
+	help
+	  Support for AMD I3C Master Controller.
+
+	  This driver allows communication with I3C devices using the AMD
+	  I3C master, currently supporting Standard Data Rate (SDR) mode.
+	  Features include Dynamic Address Assignment, private transfers,
+	  and CCC transfers in both broadcast and direct modes.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called amd-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 aac74f3e3851..109bd48cb159 100644
--- a/drivers/i3c/master/Makefile
+++ b/drivers/i3c/master/Makefile
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_AMD_I3C_MASTER)		+= amd-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/amd-i3c-master.c b/drivers/i3c/master/amd-i3c-master.c
new file mode 100644
index 000000000000..c89f7de85635
--- /dev/null
+++ b/drivers/i3c/master/amd-i3c-master.c
@@ -0,0 +1,1012 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * I3C master driver for the AMD I3C controller.
+ *
+ * Copyright (C) 2025, Advanced Micro Devices, Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/i3c/master.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/ioport.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/unaligned.h>
+
+#include "../internals.h"
+
+#define XI3C_VERSION_OFFSET			0x00	/* Version Register */
+#define XI3C_RESET_OFFSET			0x04	/* Soft Reset Register */
+#define XI3C_CR_OFFSET				0x08	/* Control Register */
+#define XI3C_ADDRESS_OFFSET			0x0C	/* Target Address Register */
+#define XI3C_SR_OFFSET				0x10	/* Status Register */
+#define XI3C_CMD_FIFO_OFFSET			0x20	/* I3C Command FIFO Register */
+#define XI3C_WR_FIFO_OFFSET			0x24	/* I3C Write Data FIFO Register */
+#define XI3C_RD_FIFO_OFFSET			0x28	/* I3C Read Data FIFO Register */
+#define XI3C_RESP_STATUS_FIFO_OFFSET		0x2C	/* I3C Response status FIFO Register */
+#define XI3C_FIFO_LVL_STATUS_OFFSET		0x30	/* I3C CMD & WR FIFO LVL Register */
+#define XI3C_FIFO_LVL_STATUS_1_OFFSET		0x34	/* I3C RESP & RD FIFO LVL Register */
+#define XI3C_SCL_HIGH_TIME_OFFSET		0x38	/* I3C SCL HIGH Register */
+#define XI3C_SCL_LOW_TIME_OFFSET		0x3C	/* I3C SCL LOW  Register */
+#define XI3C_SDA_HOLD_TIME_OFFSET		0x40	/* I3C SDA HOLD Register */
+#define XI3C_TSU_START_OFFSET			0x48	/* I3C START SETUP Register  */
+#define XI3C_THD_START_OFFSET			0x4C	/* I3C START HOLD Register */
+#define XI3C_TSU_STOP_OFFSET			0x50	/* I3C STOP Setup Register  */
+#define XI3C_OD_SCL_HIGH_TIME_OFFSET		0x54	/* I3C OD SCL HIGH Register */
+#define XI3C_OD_SCL_LOW_TIME_OFFSET		0x58	/* I3C OD SCL LOW  Register */
+#define XI3C_PID0_OFFSET			0x6C	/* LSB 4 bytes of the PID */
+#define XI3C_PID1_BCR_DCR			0x70	/* MSB 2 bytes of the PID, BCR and DCR */
+
+#define XI3C_CR_EN_MASK				BIT(0)	/* Core Enable */
+#define XI3C_CR_RESUME_MASK			BIT(2)	/* Core Resume */
+#define XI3C_SR_RESP_NOT_EMPTY_MASK		BIT(4)	/* Resp Fifo not empty status mask */
+#define XI3C_RD_FIFO_NOT_EMPTY_MASK		BIT(15)	/* Read Fifo not empty status mask */
+
+#define XI3C_BCR_MASK				GENMASK(23, 16)
+#define XI3C_DCR_MASK				GENMASK(31, 24)
+#define XI3C_PID_MASK				GENMASK_ULL(63, 16)
+#define XI3C_SCL_HIGH_TIME_MASK			GENMASK(17, 0)
+#define XI3C_SCL_LOW_TIME_MASK			GENMASK(17, 0)
+#define XI3C_SDA_HOLD_TIME_MASK			GENMASK(17, 0)
+#define XI3C_TSU_START_MASK			GENMASK(17, 0)
+#define XI3C_THD_START_MASK			GENMASK(17, 0)
+#define XI3C_TSU_STOP_MASK			GENMASK(17, 0)
+#define XI3C_REV_NUM_MASK			GENMASK(15, 8)
+#define XI3C_PID1_MASK				GENMASK(15, 0)
+#define XI3C_WR_FIFO_LEVEL_MASK			GENMASK(15, 0)
+#define XI3C_CMD_LEN_MASK			GENMASK(11, 0)
+#define XI3C_RESP_CODE_MASK			GENMASK(8, 5)
+#define XI3C_ADDR_MASK				GENMASK(6, 0)
+#define XI3C_CMD_TYPE_MASK			GENMASK(3, 0)
+#define XI3C_CMD_TID_MASK			GENMASK(3, 0)
+#define XI3C_FIFOS_RST_MASK			GENMASK(4, 1)
+
+#define XI3C_OD_TLOW_NS				500000
+#define XI3C_OD_THIGH_NS			41000
+#define XI3C_I2C_TCASMIN_NS			600000
+#define XI3C_TCASMIN_NS				260000
+#define XI3C_MAXDATA_LENGTH			4095
+#define XI3C_MAX_DEVS				32
+#define XI3C_DAA_SLAVEINFO_READ_BYTECOUNT	8
+
+#define XI3C_I2C_MODE				0
+#define XI3C_I2C_TID				0
+#define XI3C_SDR_MODE				1
+#define XI3C_SDR_TID				1
+
+#define XI3C_WORD_LEN				4
+
+/* timeout waiting for the controller finish transfers */
+#define XI3C_XFER_TIMEOUT_MS			100000
+#define XI3C_XFER_TIMEOUT_JIFFIES		(msecs_to_jiffies(XI3C_XFER_TIMEOUT_MS))
+
+#define xi3c_getrevisionnumber(master)						\
+	(FIELD_GET(XI3C_REV_NUM_MASK,						\
+		   ioread32((master)->membase + XI3C_VERSION_OFFSET)))
+
+#define xi3c_wrfifolevel(master)						\
+	(ioread32((master)->membase + XI3C_FIFO_LVL_STATUS_OFFSET) &		\
+	 XI3C_WR_FIFO_LEVEL_MASK)
+
+#define xi3c_rdfifolevel(master)						\
+	(ioread32((master)->membase + XI3C_FIFO_LVL_STATUS_1_OFFSET) &		\
+	 XI3C_WR_FIFO_LEVEL_MASK)
+
+#define xi3c_is_resp_available(master)						\
+	(FIELD_GET(XI3C_SR_RESP_NOT_EMPTY_MASK,					\
+		   ioread32((master)->membase + XI3C_SR_OFFSET)))
+
+struct xi3c_cmd {
+	void *tx_buf;
+	void *rx_buf;
+	u16 tx_len;
+	u16 rx_len;
+	u8 addr;
+	u8 type;
+	u8 tid;
+	bool rnw;
+	bool is_daa;
+	bool continued;
+};
+
+struct xi3c_xfer {
+	struct list_head node;
+	struct completion comp;
+	int ret;
+	unsigned int ncmds;
+	struct xi3c_cmd cmds[] __counted_by(ncmds);
+};
+
+/**
+ * struct xi3c_master - I3C Master structure
+ * @base: I3C master controller
+ * @dev: Pointer to device structure
+ * @xferqueue: Transfer queue structure
+ * @xferqueue.list: List member
+ * @xferqueue.cur: Current ongoing transfer
+ * @xferqueue.lock: Queue lock
+ * @membase: Memory base of the HW registers
+ * @pclk: Input clock
+ * @lock: Transfer lock
+ * @daa: daa structure
+ * @daa.addrs: Slave addresses array
+ * @daa.index: Slave device index
+ */
+struct xi3c_master {
+	struct i3c_master_controller base;
+	struct device *dev;
+	struct {
+		struct list_head list;
+		struct xi3c_xfer *cur;
+		/* Queue lock */
+		spinlock_t lock;
+	} xferqueue;
+	void __iomem *membase;
+	struct clk *pclk;
+	/* Transfer lock */
+	struct mutex lock;
+	struct {
+		u8 addrs[XI3C_MAX_DEVS];
+		u8 index;
+	} daa;
+};
+
+static inline struct xi3c_master *
+to_xi3c_master(struct i3c_master_controller *master)
+{
+	return container_of(master, struct xi3c_master, base);
+}
+
+static int xi3c_get_response(struct xi3c_master *master)
+{
+	u32 resp_reg, response_data;
+	int ret;
+
+	ret = readl_poll_timeout(master->membase + XI3C_SR_OFFSET,
+				 resp_reg,
+				 resp_reg & XI3C_SR_RESP_NOT_EMPTY_MASK,
+				 0, XI3C_XFER_TIMEOUT_MS);
+	if (ret) {
+		dev_err(master->dev, "XI3C response timeout\n");
+		return ret;
+	}
+
+	response_data = ioread32(master->membase + XI3C_RESP_STATUS_FIFO_OFFSET);
+
+	/* Return response code */
+	return FIELD_GET(XI3C_RESP_CODE_MASK, response_data);
+}
+
+static void xi3c_master_write_to_cmdfifo(struct xi3c_master *master,
+					 struct xi3c_cmd *cmd, u16 len)
+{
+	u32 transfer_cmd = 0;
+	u8 addr;
+
+	addr = ((cmd->addr & XI3C_ADDR_MASK) << 1) | (cmd->rnw & BIT(0));
+
+	transfer_cmd = cmd->type & XI3C_CMD_TYPE_MASK;
+	transfer_cmd |= (u32)(!cmd->continued) << 4;
+	transfer_cmd |= (u32)(addr) << 8;
+	transfer_cmd |= (u32)(cmd->tid & XI3C_CMD_TID_MASK) << 28;
+
+	/*
+	 * For dynamic addressing, an additional 1-byte length must be added
+	 * to the command FIFO to account for the address present in the TX FIFO
+	 */
+	if (cmd->is_daa) {
+		i3c_writel_fifo(master->membase + XI3C_WR_FIFO_OFFSET,
+				(u8 *)cmd->tx_buf, cmd->tx_len, I3C_FIFO_BIG_ENDIAN);
+
+		len++;
+		master->daa.index++;
+	}
+
+	transfer_cmd |= (u32)(len & XI3C_CMD_LEN_MASK) << 16;
+	iowrite32(transfer_cmd, master->membase + XI3C_CMD_FIFO_OFFSET);
+}
+
+static inline void xi3c_master_enable(struct xi3c_master *master)
+{
+	iowrite32(ioread32(master->membase + XI3C_CR_OFFSET) | XI3C_CR_EN_MASK,
+		  master->membase + XI3C_CR_OFFSET);
+}
+
+static inline void xi3c_master_disable(struct xi3c_master *master)
+{
+	iowrite32(ioread32(master->membase + XI3C_CR_OFFSET) & (~XI3C_CR_EN_MASK),
+		  master->membase + XI3C_CR_OFFSET);
+}
+
+static inline void xi3c_master_resume(struct xi3c_master *master)
+{
+	iowrite32(ioread32(master->membase + XI3C_CR_OFFSET) |
+		  XI3C_CR_RESUME_MASK, master->membase + XI3C_CR_OFFSET);
+}
+
+static void xi3c_master_reset_fifos(struct xi3c_master *master)
+{
+	u32 data;
+
+	/* Reset fifos */
+	data = ioread32(master->membase + XI3C_RESET_OFFSET);
+	data |= XI3C_FIFOS_RST_MASK;
+	iowrite32(data, master->membase + XI3C_RESET_OFFSET);
+	ioread32(master->membase + XI3C_RESET_OFFSET);
+	udelay(10);
+	data &= ~XI3C_FIFOS_RST_MASK;
+	iowrite32(data, master->membase + XI3C_RESET_OFFSET);
+	ioread32(master->membase + XI3C_RESET_OFFSET);
+	udelay(10);
+}
+
+static inline void xi3c_master_init(struct xi3c_master *master)
+{
+	/* Reset fifos */
+	xi3c_master_reset_fifos(master);
+
+	/* Enable controller */
+	xi3c_master_enable(master);
+}
+
+static inline void xi3c_master_reinit(struct xi3c_master *master)
+{
+	/* Reset fifos */
+	xi3c_master_reset_fifos(master);
+
+	/* Resume controller */
+	xi3c_master_resume(master);
+}
+
+static struct xi3c_xfer *
+xi3c_master_alloc_xfer(struct xi3c_master *master, unsigned int ncmds)
+{
+	struct xi3c_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 xi3c_master_rd_from_rx_fifo(struct xi3c_master *master,
+					struct xi3c_cmd *cmd)
+{
+	u16 rx_data_available;
+	u16 len;
+
+	rx_data_available = xi3c_rdfifolevel(master);
+	len = rx_data_available * XI3C_WORD_LEN;
+
+	if (len) {
+		i3c_readl_fifo(master->membase + XI3C_RD_FIFO_OFFSET, (u8 *)cmd->rx_buf,
+			       len, I3C_FIFO_BIG_ENDIAN);
+
+		cmd->rx_buf = (u8 *)cmd->rx_buf + len;
+		cmd->rx_len -= len;
+	}
+}
+
+static int xi3c_master_read(struct xi3c_master *master, struct xi3c_cmd *cmd)
+{
+	unsigned long timeout;
+	u32 status_reg;
+	int ret;
+
+	if (!cmd->rx_buf || cmd->rx_len > XI3C_MAXDATA_LENGTH)
+		return -EINVAL;
+
+	/* Fill command fifo */
+	xi3c_master_write_to_cmdfifo(master, cmd, cmd->rx_len);
+
+	ret = readl_poll_timeout(master->membase + XI3C_SR_OFFSET,
+				 status_reg,
+				 status_reg & XI3C_RD_FIFO_NOT_EMPTY_MASK,
+				 0, XI3C_XFER_TIMEOUT_MS);
+	if (ret) {
+		if (cmd->is_daa) {
+			cmd->is_daa = false;
+			ret = I3C_ERROR_M2;
+		} else {
+			dev_err(master->dev, "XI3C read timeout\n");
+		}
+		return ret;
+	}
+
+	timeout = jiffies + XI3C_XFER_TIMEOUT_JIFFIES;
+
+	/* Read data from rx fifo */
+	while (cmd->rx_len > 0 && !xi3c_is_resp_available(master)) {
+		if (time_after(jiffies, timeout)) {
+			dev_err(master->dev, "XI3C read timeout\n");
+			return -EIO;
+		}
+		xi3c_master_rd_from_rx_fifo(master, cmd);
+	}
+
+	/* Read remaining data */
+	xi3c_master_rd_from_rx_fifo(master, cmd);
+
+	return 0;
+}
+
+static void xi3c_master_wr_to_tx_fifo(struct xi3c_master *master,
+				      struct xi3c_cmd *cmd)
+{
+	u16 wrfifo_space;
+	u16 len;
+
+	wrfifo_space = xi3c_wrfifolevel(master);
+	if (cmd->tx_len > wrfifo_space * XI3C_WORD_LEN)
+		len = wrfifo_space * XI3C_WORD_LEN;
+	else
+		len = cmd->tx_len;
+
+	if (len) {
+		i3c_writel_fifo(master->membase + XI3C_WR_FIFO_OFFSET, (u8 *)cmd->tx_buf,
+				len, I3C_FIFO_BIG_ENDIAN);
+
+		cmd->tx_buf = (u8 *)cmd->tx_buf + len;
+		cmd->tx_len -= len;
+	}
+}
+
+static int xi3c_master_write(struct xi3c_master *master, struct xi3c_cmd *cmd)
+{
+	unsigned long timeout;
+	u16 cmd_len;
+
+	cmd_len = cmd->tx_len;
+	if (!cmd->tx_buf || cmd->tx_len > XI3C_MAXDATA_LENGTH)
+		return -EINVAL;
+
+	/* Fill Tx fifo */
+	xi3c_master_wr_to_tx_fifo(master, cmd);
+
+	/* Write to command fifo */
+	xi3c_master_write_to_cmdfifo(master, cmd, cmd_len);
+
+	timeout = jiffies + XI3C_XFER_TIMEOUT_JIFFIES;
+	/* Fill if any remaining data to tx fifo */
+	while (cmd->tx_len > 0 && !xi3c_is_resp_available(master)) {
+		if (time_after(jiffies, timeout)) {
+			dev_err(master->dev, "XI3C write timeout\n");
+			return -EIO;
+		}
+
+		xi3c_master_wr_to_tx_fifo(master, cmd);
+	}
+
+	return 0;
+}
+
+static int xi3c_master_xfer(struct xi3c_master *master, struct xi3c_cmd *cmd)
+{
+	int ret;
+
+	if (cmd->rnw)
+		ret = xi3c_master_read(master, cmd);
+	else
+		ret = xi3c_master_write(master, cmd);
+
+	if (ret < 0)
+		goto err_xfer_out;
+
+	ret = xi3c_get_response(master);
+	if (ret)
+		goto err_xfer_out;
+
+	return 0;
+
+err_xfer_out:
+	xi3c_master_reinit(master);
+	return ret;
+}
+
+static void xi3c_master_dequeue_xfer_locked(struct xi3c_master *master,
+					    struct xi3c_xfer *xfer)
+{
+	if (master->xferqueue.cur == xfer)
+		master->xferqueue.cur = NULL;
+	else
+		list_del_init(&xfer->node);
+}
+
+static void xi3c_master_dequeue_xfer(struct xi3c_master *master,
+				     struct xi3c_xfer *xfer)
+{
+	guard(spinlock_irqsave)(&master->xferqueue.lock);
+
+	xi3c_master_dequeue_xfer_locked(master, xfer);
+}
+
+static void xi3c_master_start_xfer_locked(struct xi3c_master *master)
+{
+	struct xi3c_xfer *xfer = master->xferqueue.cur;
+	int ret = 0, i;
+
+	if (!xfer)
+		return;
+
+	for (i = 0; i < xfer->ncmds; i++) {
+		struct xi3c_cmd *cmd = &xfer->cmds[i];
+
+		ret = xi3c_master_xfer(master, cmd);
+		if (ret)
+			break;
+	}
+
+	xfer->ret = ret;
+	complete(&xfer->comp);
+
+	xfer = list_first_entry_or_null(&master->xferqueue.list,
+					struct xi3c_xfer,
+					node);
+	if (xfer)
+		list_del_init(&xfer->node);
+
+	master->xferqueue.cur = xfer;
+	xi3c_master_start_xfer_locked(master);
+}
+
+static inline void xi3c_master_enqueue_xfer(struct xi3c_master *master,
+					    struct xi3c_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;
+		xi3c_master_start_xfer_locked(master);
+	}
+}
+
+static inline int xi3c_master_common_xfer(struct xi3c_master *master,
+					  struct xi3c_xfer *xfer)
+{
+	int ret, timeout;
+
+	guard(mutex)(&master->lock);
+
+	xi3c_master_enqueue_xfer(master, xfer);
+	timeout = wait_for_completion_timeout(&xfer->comp,
+					      XI3C_XFER_TIMEOUT_JIFFIES);
+	if (!timeout)
+		ret = -ETIMEDOUT;
+	else
+		ret = xfer->ret;
+
+	if (ret)
+		xi3c_master_dequeue_xfer(master, xfer);
+
+	return ret;
+}
+
+static int xi3c_master_do_daa(struct i3c_master_controller *m)
+{
+	struct xi3c_master *master = to_xi3c_master(m);
+	struct xi3c_cmd *daa_cmd;
+	struct xi3c_xfer *xfer;
+	u8 pid_bufs[XI3C_MAX_DEVS][8];
+	u8 data, last_addr = 0;
+	int addr, ret, i;
+	u8 *pid_buf;
+
+	xfer = xi3c_master_alloc_xfer(master, 1);
+	if (!xfer) {
+		ret = -ENOMEM;
+		goto err_daa_mem;
+	}
+
+	for (i = 0; i < XI3C_MAX_DEVS; i++) {
+		addr = i3c_master_get_free_addr(m, last_addr + 1);
+		if (addr < 0) {
+			ret = -ENOSPC;
+			goto err_daa;
+		}
+		master->daa.addrs[i] = (u8)addr;
+		last_addr = (u8)addr;
+	}
+
+	/* Fill ENTDAA CCC */
+	data = I3C_CCC_ENTDAA;
+	daa_cmd = &xfer->cmds[0];
+	daa_cmd->addr = I3C_BROADCAST_ADDR;
+	daa_cmd->rnw = 0;
+	daa_cmd->tx_buf = &data;
+	daa_cmd->tx_len = 1;
+	daa_cmd->type = XI3C_SDR_MODE;
+	daa_cmd->tid = XI3C_SDR_TID;
+	daa_cmd->continued = true;
+
+	ret = xi3c_master_common_xfer(master, xfer);
+	/* DAA always finishes with CE2_ERROR or NACK_RESP */
+	if (ret && ret != I3C_ERROR_M2) {
+		goto err_daa;
+	} else {
+		if (ret && ret == I3C_ERROR_M2) {
+			ret = 0;
+			goto err_daa;
+		}
+	}
+
+	master->daa.index = 0;
+
+	while (true) {
+		struct xi3c_cmd *cmd = &xfer->cmds[0];
+
+		pid_buf = pid_bufs[master->daa.index];
+		addr = (master->daa.addrs[master->daa.index] << 1) |
+		       (u8)(!parity8(master->daa.addrs[master->daa.index]));
+
+		cmd->tx_buf = (u8 *)&addr;
+		cmd->tx_len = 1;
+		cmd->addr = I3C_BROADCAST_ADDR;
+		cmd->rnw = 1;
+		cmd->rx_buf = pid_buf;
+		cmd->rx_len = XI3C_DAA_SLAVEINFO_READ_BYTECOUNT;
+		cmd->is_daa = true;
+		cmd->type = XI3C_SDR_MODE;
+		cmd->tid = XI3C_SDR_TID;
+		cmd->continued = true;
+
+		ret = xi3c_master_common_xfer(master, xfer);
+
+		/* DAA always finishes with CE2_ERROR or NACK_RESP */
+		if (ret && ret != I3C_ERROR_M2) {
+			goto err_daa;
+		} else {
+			if (ret && ret == I3C_ERROR_M2) {
+				xi3c_master_resume(master);
+				master->daa.index--;
+				ret = 0;
+				break;
+			}
+		}
+	}
+
+	kfree(xfer);
+
+	for (i = 0; i < master->daa.index; i++) {
+		i3c_master_add_i3c_dev_locked(m, master->daa.addrs[i]);
+
+		u64 data = FIELD_GET(XI3C_PID_MASK, get_unaligned_be64(pid_bufs[i]));
+
+		dev_info(master->dev, "Client %d: PID: 0x%llx\n", i, data);
+	}
+
+	return 0;
+
+err_daa:
+	kfree(xfer);
+err_daa_mem:
+	xi3c_master_reinit(master);
+	return ret;
+}
+
+static bool
+xi3c_master_supports_ccc_cmd(struct i3c_master_controller *master,
+			     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_ENTAS(0, true):
+	case I3C_CCC_ENTAS(0, false):
+	case I3C_CCC_RSTDAA(true):
+	case I3C_CCC_RSTDAA(false):
+	case I3C_CCC_ENTDAA:
+	case I3C_CCC_SETMWL(true):
+	case I3C_CCC_SETMWL(false):
+	case I3C_CCC_SETMRL(true):
+	case I3C_CCC_SETMRL(false):
+	case I3C_CCC_ENTHDR(0):
+	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_GETMXDS:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static int xi3c_master_send_bdcast_ccc_cmd(struct xi3c_master *master,
+					   struct i3c_ccc_cmd *ccc)
+{
+	u16 xfer_len = ccc->dests[0].payload.len + 1;
+	struct xi3c_xfer *xfer;
+	struct xi3c_cmd *cmd;
+	int ret;
+
+	xfer = xi3c_master_alloc_xfer(master, 1);
+	if (!xfer)
+		return -ENOMEM;
+
+	u8 *buf __free(kfree) = kmalloc(xfer_len, GFP_KERNEL);
+	if (!buf) {
+		kfree(xfer);
+		return -ENOMEM;
+	}
+
+	buf[0] = ccc->id;
+	memcpy(&buf[1], ccc->dests[0].payload.data, ccc->dests[0].payload.len);
+
+	cmd = &xfer->cmds[0];
+	cmd->addr = ccc->dests[0].addr;
+	cmd->rnw = ccc->rnw;
+	cmd->tx_buf = buf;
+	cmd->tx_len = xfer_len;
+	cmd->type = XI3C_SDR_MODE;
+	cmd->tid = XI3C_SDR_TID;
+	cmd->continued = false;
+
+	ret = xi3c_master_common_xfer(master, xfer);
+	kfree(xfer);
+
+	return ret;
+}
+
+static int xi3c_master_send_direct_ccc_cmd(struct xi3c_master *master,
+					   struct i3c_ccc_cmd *ccc)
+{
+	struct xi3c_xfer *xfer;
+	struct xi3c_cmd *cmd;
+	int ret;
+
+	xfer = xi3c_master_alloc_xfer(master, 2);
+	if (!xfer)
+		return -ENOMEM;
+
+	/* Broadcasted message */
+	cmd = &xfer->cmds[0];
+	cmd->addr = I3C_BROADCAST_ADDR;
+	cmd->rnw = 0;
+	cmd->tx_buf = &ccc->id;
+	cmd->tx_len = 1;
+	cmd->type = XI3C_SDR_MODE;
+	cmd->tid = XI3C_SDR_TID;
+	cmd->continued = true;
+
+	/* Directed message */
+	cmd = &xfer->cmds[1];
+	cmd->addr = ccc->dests[0].addr;
+	cmd->rnw = ccc->rnw;
+	if (cmd->rnw) {
+		cmd->rx_buf = ccc->dests[0].payload.data;
+		cmd->rx_len = ccc->dests[0].payload.len;
+	} else {
+		cmd->tx_buf = ccc->dests[0].payload.data;
+		cmd->tx_len = ccc->dests[0].payload.len;
+	}
+	cmd->type = XI3C_SDR_MODE;
+	cmd->tid = XI3C_SDR_TID;
+	cmd->continued = false;
+
+	ret = xi3c_master_common_xfer(master, xfer);
+	kfree(xfer);
+	return ret;
+}
+
+static int xi3c_master_send_ccc_cmd(struct i3c_master_controller *m,
+				    struct i3c_ccc_cmd *cmd)
+{
+	struct xi3c_master *master = to_xi3c_master(m);
+	bool broadcast = cmd->id < 0x80;
+
+	if (broadcast)
+		return xi3c_master_send_bdcast_ccc_cmd(master, cmd);
+
+	return xi3c_master_send_direct_ccc_cmd(master, cmd);
+}
+
+static int xi3c_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 xi3c_master *master = to_xi3c_master(m);
+	struct xi3c_xfer *xfer;
+	int i, ret;
+
+	if (!nxfers)
+		return 0;
+
+	xfer = xi3c_master_alloc_xfer(master, nxfers);
+	if (!xfer)
+		return -ENOMEM;
+
+	for (i = 0; i < nxfers; i++) {
+		struct xi3c_cmd *cmd = &xfer->cmds[i];
+
+		cmd->addr = dev->info.dyn_addr;
+		cmd->rnw = xfers[i].rnw;
+
+		if (cmd->rnw) {
+			cmd->rx_buf = xfers[i].data.in;
+			cmd->rx_len = xfers[i].len;
+		} else {
+			cmd->tx_buf = (void *)xfers[i].data.out;
+			cmd->tx_len = xfers[i].len;
+		}
+
+		cmd->type = XI3C_SDR_MODE;
+		cmd->tid = XI3C_SDR_TID;
+		cmd->continued = (i + 1) < nxfers;
+	}
+
+	ret = xi3c_master_common_xfer(master, xfer);
+	kfree(xfer);
+	return ret;
+}
+
+static int xi3c_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 xi3c_master *master = to_xi3c_master(m);
+	struct xi3c_xfer *xfer;
+	int i, ret;
+
+	if (!nxfers)
+		return 0;
+
+	xfer = xi3c_master_alloc_xfer(master, nxfers);
+	if (!xfer)
+		return -ENOMEM;
+
+	for (i = 0; i < nxfers; i++) {
+		struct xi3c_cmd *cmd = &xfer->cmds[i];
+
+		cmd->addr = xfers[i].addr & XI3C_ADDR_MASK;
+		cmd->rnw = xfers[i].flags & I2C_M_RD;
+
+		if (cmd->rnw) {
+			cmd->rx_buf = xfers[i].buf;
+			cmd->rx_len = xfers[i].len;
+		} else {
+			cmd->tx_buf = (void *)xfers[i].buf;
+			cmd->tx_len = xfers[i].len;
+		}
+
+		cmd->type = XI3C_I2C_MODE;
+		cmd->tid = XI3C_I2C_TID;
+		cmd->continued = (i + 1) < nxfers;
+	}
+
+	ret = xi3c_master_common_xfer(master, xfer);
+	kfree(xfer);
+	return ret;
+}
+
+static int xi3c_clk_cfg(struct xi3c_master *master, unsigned long sclhz, u8 mode)
+{
+	unsigned long core_rate, core_periodns;
+	u32 thigh, tlow, thold, odthigh, odtlow, tcasmin, tsustart, tsustop, thdstart;
+
+	core_rate = clk_get_rate(master->pclk);
+	if (!core_rate)
+		return -EINVAL;
+
+	core_periodns = DIV_ROUND_UP(1000000000, core_rate);
+
+	thigh = DIV_ROUND_UP(core_rate, sclhz) >> 1;
+	tlow = thigh;
+
+	/* Hold time : 40% of tlow time */
+	thold = (tlow * 4) / 10;
+
+	/*
+	 * For initial IP (revision number == 0), minimum data hold time is 5.
+	 * For updated IP (revision number > 0), minimum data hold time is 6.
+	 * Updated IP supports achieving high data rate with low reference
+	 * frequency.
+	 */
+	if (xi3c_getrevisionnumber(master) == 0)
+		thold = (thold < 5) ? 5 : thold;
+	else
+		thold = (thold < 6) ? 6 : thold;
+
+	iowrite32((thigh - 2) & XI3C_SCL_HIGH_TIME_MASK,
+		  master->membase + XI3C_SCL_HIGH_TIME_OFFSET);
+	iowrite32((tlow - 2) & XI3C_SCL_LOW_TIME_MASK,
+		  master->membase + XI3C_SCL_LOW_TIME_OFFSET);
+	iowrite32((thold - 2) & XI3C_SDA_HOLD_TIME_MASK,
+		  master->membase + XI3C_SDA_HOLD_TIME_OFFSET);
+
+	if (!mode) {
+		/* I2C */
+		iowrite32((thigh - 2) & XI3C_SCL_HIGH_TIME_MASK,
+			  master->membase + XI3C_OD_SCL_HIGH_TIME_OFFSET);
+		iowrite32((tlow - 2) & XI3C_SCL_LOW_TIME_MASK,
+			  master->membase + XI3C_OD_SCL_LOW_TIME_OFFSET);
+
+		tcasmin = DIV_ROUND_UP(XI3C_I2C_TCASMIN_NS, core_periodns);
+	} else {
+		/* I3C */
+		odtlow = DIV_ROUND_UP(XI3C_OD_TLOW_NS, core_periodns);
+		odthigh = DIV_ROUND_UP(XI3C_OD_THIGH_NS, core_periodns);
+
+		odtlow = (tlow < odtlow) ? odtlow : tlow;
+		odthigh = (thigh > odthigh) ? odthigh : thigh;
+
+		iowrite32((odthigh - 2) & XI3C_SCL_HIGH_TIME_MASK,
+			  master->membase + XI3C_OD_SCL_HIGH_TIME_OFFSET);
+		iowrite32((odtlow - 2) & XI3C_SCL_LOW_TIME_MASK,
+			  master->membase + XI3C_OD_SCL_LOW_TIME_OFFSET);
+
+		tcasmin = DIV_ROUND_UP(XI3C_TCASMIN_NS, core_periodns);
+	}
+
+	thdstart = (thigh > tcasmin) ? thigh : tcasmin;
+	tsustart = (tlow > tcasmin) ? tlow : tcasmin;
+	tsustop = (tlow > tcasmin) ? tlow : tcasmin;
+
+	iowrite32((tsustart - 2) & XI3C_TSU_START_MASK,
+		  master->membase + XI3C_TSU_START_OFFSET);
+	iowrite32((thdstart - 2) & XI3C_THD_START_MASK,
+		  master->membase + XI3C_THD_START_OFFSET);
+	iowrite32((tsustop - 2) & XI3C_TSU_STOP_MASK,
+		  master->membase + XI3C_TSU_STOP_OFFSET);
+
+	return 0;
+}
+
+static int xi3c_master_bus_init(struct i3c_master_controller *m)
+{
+	struct xi3c_master *master = to_xi3c_master(m);
+	struct i3c_bus *bus = i3c_master_get_bus(m);
+	struct i3c_device_info info = { };
+	unsigned long sclhz;
+	u64 pid1_bcr_dcr;
+	u8 mode;
+	int ret;
+
+	switch (bus->mode) {
+	case I3C_BUS_MODE_MIXED_FAST:
+	case I3C_BUS_MODE_MIXED_LIMITED:
+		mode = XI3C_I2C_MODE;
+		sclhz = bus->scl_rate.i2c;
+		break;
+	case I3C_BUS_MODE_PURE:
+		mode = XI3C_SDR_MODE;
+		sclhz = bus->scl_rate.i3c;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = xi3c_clk_cfg(master, sclhz, mode);
+	if (ret)
+		return ret;
+
+	/* Get an address for the master. */
+	ret = i3c_master_get_free_addr(m, 0);
+	if (ret < 0)
+		return ret;
+
+	info.dyn_addr = (u8)ret;
+
+	/* Write the dynamic address value to the address register. */
+	iowrite32(info.dyn_addr, master->membase + XI3C_ADDRESS_OFFSET);
+
+	/* Read PID, BCR and DCR values, and assign to i3c device info. */
+	pid1_bcr_dcr = ioread32(master->membase + XI3C_PID1_BCR_DCR);
+	info.pid = (((u64)(FIELD_GET(XI3C_PID1_MASK, pid1_bcr_dcr)) << 32) |
+		    ioread32(master->membase + XI3C_PID0_OFFSET));
+	info.bcr = (u8)FIELD_GET(XI3C_BCR_MASK, pid1_bcr_dcr);
+	info.dcr = (u8)FIELD_GET(XI3C_DCR_MASK, pid1_bcr_dcr);
+
+	ret = i3c_master_set_info(&master->base, &info);
+	if (ret)
+		return ret;
+
+	xi3c_master_init(master);
+
+	return ret;
+}
+
+static void xi3c_master_bus_cleanup(struct i3c_master_controller *m)
+{
+	struct xi3c_master *master = to_xi3c_master(m);
+
+	xi3c_master_disable(master);
+}
+
+static const struct i3c_master_controller_ops xi3c_master_ops = {
+	.bus_init = xi3c_master_bus_init,
+	.bus_cleanup = xi3c_master_bus_cleanup,
+	.do_daa = xi3c_master_do_daa,
+	.supports_ccc_cmd = xi3c_master_supports_ccc_cmd,
+	.send_ccc_cmd = xi3c_master_send_ccc_cmd,
+	.priv_xfers = xi3c_master_priv_xfers,
+	.i2c_xfers = xi3c_master_i2c_xfers,
+};
+
+static int xi3c_master_probe(struct platform_device *pdev)
+{
+	struct xi3c_master *master;
+	int ret;
+
+	master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL);
+	if (!master)
+		return -ENOMEM;
+
+	master->membase = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(master->membase))
+		return PTR_ERR(master->membase);
+
+	master->pclk = devm_clk_get_enabled(&pdev->dev, NULL);
+	if (IS_ERR(master->pclk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(master->pclk),
+				     "Failed to get and enable clock\n");
+
+	master->dev = &pdev->dev;
+
+	ret = devm_mutex_init(master->dev, &master->lock);
+	if (ret)
+		return ret;
+
+	spin_lock_init(&master->xferqueue.lock);
+	INIT_LIST_HEAD(&master->xferqueue.list);
+
+	platform_set_drvdata(pdev, master);
+
+	return i3c_master_register(&master->base, &pdev->dev,
+				   &xi3c_master_ops, false);
+}
+
+static void xi3c_master_remove(struct platform_device *pdev)
+{
+	struct xi3c_master *master = platform_get_drvdata(pdev);
+
+	i3c_master_unregister(&master->base);
+}
+
+static const struct of_device_id xi3c_master_of_ids[] = {
+	{ .compatible = "xlnx,axi-i3c-1.0" },
+	{ },
+};
+
+static struct platform_driver xi3c_master_driver = {
+	.probe = xi3c_master_probe,
+	.remove = xi3c_master_remove,
+	.driver = {
+		.name = "axi-i3c-master",
+		.of_match_table = xi3c_master_of_ids,
+	},
+};
+module_platform_driver(xi3c_master_driver);
+
+MODULE_AUTHOR("Manikanta Guntupalli <manikanta.guntupalli@amd.com>");
+MODULE_DESCRIPTION("AXI I3C master driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH V7 4/4] i3c: master: Add AMD I3C bus controller driver

[Frank Li]

On Tue, Sep 23, 2025 at 09:15:51PM +0530, Manikanta Guntupalli wrote:
> Add an I3C master driver and maintainers fragment for the AMD I3C bus
> controller.
>
> The driver currently supports the I3C bus operating in SDR i3c mode,
> with features including Dynamic Address Assignment, private data transfers,
> and CCC transfers in both broadcast and direct modes. It also supports
> operation in I2C mode.
>
> Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
> ---
> Changes for V2:
> Updated commit description.
> Added mixed mode support with clock configuration.
> Converted smaller functions into inline functions.
> Used FIELD_GET() in xi3c_get_response().
> Updated xi3c_master_rd_from_rx_fifo() to use cmd->rx_buf.
> Used parity8() for address parity calculation.
> Added guards for locks.
> Dropped num_targets and updated xi3c_master_do_daa().
> Used __free(kfree) in xi3c_master_send_bdcast_ccc_cmd().
> Dropped PM runtime support.
> Updated xi3c_master_read() and xi3c_master_write() with
> xi3c_is_resp_available() check.
> Created separate functions: xi3c_master_init() and xi3c_master_reinit().
> Used xi3c_master_init() in bus initialization and xi3c_master_reinit()
> in error paths.
> Added DAA structure to xi3c_master structure.
>
> Changes for V3:
> Resolved merge conflicts.
>
> Changes for V4:
> Updated timeout macros.
> Removed type casting for xi3c_is_resp_available() macro.
> Used ioread32() and iowrite32() instead of readl() and writel()
> to keep consistency.
> Read XI3C_RESET_OFFSET reg before udelay().
> Removed xi3c_master_free_xfer() and directly used kfree().
> Skipped checking return value of i3c_master_add_i3c_dev_locked().
> Used devm_mutex_init() instead of mutex_init().
>
> Changes for V5:
> Used GENMASK_ULL for PID mask as it's 64bit mask.
>
> Changes for V6:
> Removed typecast for xi3c_getrevisionnumber(), xi3c_wrfifolevel(),
> and xi3c_rdfifolevel().
> Replaced dynamic allocation with a static variable for pid_bcr_dcr.
> Fixed sparse warning in do_daa by typecasting the address parity value
> to u8.
> Fixed sparse warning in xi3c_master_bus_init by typecasting the pid value
> to u64 in info.pid calculation.
>
> Changes for V7:
> Updated timeout macro name.
> Updated xi3c_master_wr_to_tx_fifo() and xi3c_master_rd_from_rx_fifo()
> to use i3c_writel_fifo() and i3c_readl_fifo().
> ---
>  MAINTAINERS                         |    7 +
>  drivers/i3c/master/Kconfig          |   16 +
>  drivers/i3c/master/Makefile         |    1 +
>  drivers/i3c/master/amd-i3c-master.c | 1012 +++++++++++++++++++++++++++
>  4 files changed, 1036 insertions(+)
>  create mode 100644 drivers/i3c/master/amd-i3c-master.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8886d66bd824..fe88efb41f5b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11782,6 +11782,13 @@ L:	linux-i2c@vger.kernel.org
>  S:	Maintained
>  F:	drivers/i2c/i2c-stub.c
>
> +I3C DRIVER FOR AMD AXI I3C MASTER
> +M:	Manikanta Guntupalli <manikanta.guntupalli@amd.com>
> +R:	Michal Simek <michal.simek@amd.com>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/i3c/xlnx,axi-i3c.yaml
> +F:	drivers/i3c/master/amd-i3c-master.c
> +
>  I3C DRIVER FOR ASPEED AST2600
>  M:	Jeremy Kerr <jk@codeconstruct.com.au>
>  S:	Maintained
> diff --git a/drivers/i3c/master/Kconfig b/drivers/i3c/master/Kconfig
> index 13df2944f2ec..4b884a678893 100644
> --- a/drivers/i3c/master/Kconfig
> +++ b/drivers/i3c/master/Kconfig
> @@ -1,4 +1,20 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> +
> +config AMD_I3C_MASTER
> +	tristate "AMD I3C Master driver"
> +	depends on I3C
> +	depends on HAS_IOMEM
> +	help
> +	  Support for AMD I3C Master Controller.
> +
> +	  This driver allows communication with I3C devices using the AMD
> +	  I3C master, currently supporting Standard Data Rate (SDR) mode.
> +	  Features include Dynamic Address Assignment, private transfers,
> +	  and CCC transfers in both broadcast and direct modes.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called amd-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 aac74f3e3851..109bd48cb159 100644
> --- a/drivers/i3c/master/Makefile
> +++ b/drivers/i3c/master/Makefile
> @@ -1,4 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_AMD_I3C_MASTER)		+= amd-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/amd-i3c-master.c b/drivers/i3c/master/amd-i3c-master.c
> new file mode 100644
> index 000000000000..c89f7de85635
> --- /dev/null
> +++ b/drivers/i3c/master/amd-i3c-master.c
> @@ -0,0 +1,1012 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * I3C master driver for the AMD I3C controller.
> + *
> + * Copyright (C) 2025, Advanced Micro Devices, Inc.
> + */
> +

...

> +
> +static inline int xi3c_master_common_xfer(struct xi3c_master *master,
> +					  struct xi3c_xfer *xfer)
> +{
> +	int ret, timeout;
> +
> +	guard(mutex)(&master->lock);
> +
> +	xi3c_master_enqueue_xfer(master, xfer);
> +	timeout = wait_for_completion_timeout(&xfer->comp,
> +					      XI3C_XFER_TIMEOUT_JIFFIES);

Recent patch use time_left instead of timeout.

> +	if (!timeout)
> +		ret = -ETIMEDOUT;
> +	else
> +		ret = xfer->ret;
> +
> +	if (ret)
> +		xi3c_master_dequeue_xfer(master, xfer);
> +
> +	return ret;
> +}
> +
> +static int xi3c_master_do_daa(struct i3c_master_controller *m)
> +{
> +	struct xi3c_master *master = to_xi3c_master(m);
> +	struct xi3c_cmd *daa_cmd;
> +	struct xi3c_xfer *xfer;

struct xi3c_xfer __free(kfree) *xfer;
will simple err path.

> +	u8 pid_bufs[XI3C_MAX_DEVS][8];
> +	u8 data, last_addr = 0;
> +	int addr, ret, i;
> +	u8 *pid_buf;

try keep reverise christmas tree order

> +
> +	xfer = xi3c_master_alloc_xfer(master, 1);

only 1 xfer, needn't alloc from heap. Just use stack varible should be
enough.

> +	if (!xfer) {
> +		ret = -ENOMEM;
> +		goto err_daa_mem;
> +	}
> +
> +	for (i = 0; i < XI3C_MAX_DEVS; i++) {
> +		addr = i3c_master_get_free_addr(m, last_addr + 1);
> +		if (addr < 0) {
> +			ret = -ENOSPC;
> +			goto err_daa;
> +		}
> +		master->daa.addrs[i] = (u8)addr;
> +		last_addr = (u8)addr;
> +	}
> +
> +	/* Fill ENTDAA CCC */
> +	data = I3C_CCC_ENTDAA;
> +	daa_cmd = &xfer->cmds[0];
> +	daa_cmd->addr = I3C_BROADCAST_ADDR;
> +	daa_cmd->rnw = 0;
> +	daa_cmd->tx_buf = &data;
> +	daa_cmd->tx_len = 1;
> +	daa_cmd->type = XI3C_SDR_MODE;
> +	daa_cmd->tid = XI3C_SDR_TID;
> +	daa_cmd->continued = true;
> +
> +	ret = xi3c_master_common_xfer(master, xfer);
> +	/* DAA always finishes with CE2_ERROR or NACK_RESP */
> +	if (ret && ret != I3C_ERROR_M2) {
> +		goto err_daa;
> +	} else {
> +		if (ret && ret == I3C_ERROR_M2) {
> +			ret = 0;
> +			goto err_daa;
> +		}
> +	}
> +
> +	master->daa.index = 0;
> +
> +	while (true) {
> +		struct xi3c_cmd *cmd = &xfer->cmds[0];
> +
> +		pid_buf = pid_bufs[master->daa.index];
> +		addr = (master->daa.addrs[master->daa.index] << 1) |
> +		       (u8)(!parity8(master->daa.addrs[master->daa.index]));
> +
> +		cmd->tx_buf = (u8 *)&addr;
> +		cmd->tx_len = 1;
> +		cmd->addr = I3C_BROADCAST_ADDR;
> +		cmd->rnw = 1;
> +		cmd->rx_buf = pid_buf;
> +		cmd->rx_len = XI3C_DAA_SLAVEINFO_READ_BYTECOUNT;
> +		cmd->is_daa = true;
> +		cmd->type = XI3C_SDR_MODE;
> +		cmd->tid = XI3C_SDR_TID;
> +		cmd->continued = true;
> +
> +		ret = xi3c_master_common_xfer(master, xfer);
> +
> +		/* DAA always finishes with CE2_ERROR or NACK_RESP */
> +		if (ret && ret != I3C_ERROR_M2) {
> +			goto err_daa;
> +		} else {
> +			if (ret && ret == I3C_ERROR_M2) {
> +				xi3c_master_resume(master);
> +				master->daa.index--;
> +				ret = 0;
> +				break;
> +			}
> +		}
> +	}
> +
> +	kfree(xfer);
> +
> +	for (i = 0; i < master->daa.index; i++) {
> +		i3c_master_add_i3c_dev_locked(m, master->daa.addrs[i]);
> +
> +		u64 data = FIELD_GET(XI3C_PID_MASK, get_unaligned_be64(pid_bufs[i]));
> +
> +		dev_info(master->dev, "Client %d: PID: 0x%llx\n", i, data);
> +	}
> +
> +	return 0;
> +
> +err_daa:
> +	kfree(xfer);
> +err_daa_mem:
> +	xi3c_master_reinit(master);
> +	return ret;
> +}
> +

> +static int xi3c_master_send_bdcast_ccc_cmd(struct xi3c_master *master,
> +					   struct i3c_ccc_cmd *ccc)
> +{
> +	u16 xfer_len = ccc->dests[0].payload.len + 1;
> +	struct xi3c_xfer *xfer;
> +	struct xi3c_cmd *cmd;
> +	int ret;
> +
> +	xfer = xi3c_master_alloc_xfer(master, 1);
> +	if (!xfer)
> +		return -ENOMEM;

the same here. use struct xi3c_xfer xfer should be more simple.

Frank
> +
> +	u8 *buf __free(kfree) = kmalloc(xfer_len, GFP_KERNEL);
> +	if (!buf) {
> +		kfree(xfer);
> +		return -ENOMEM;
> +	}
> +
> +	buf[0] = ccc->id;
> +	memcpy(&buf[1], ccc->dests[0].payload.data, ccc->dests[0].payload.len);
> +
> +	cmd = &xfer->cmds[0];
> +	cmd->addr = ccc->dests[0].addr;
> +	cmd->rnw = ccc->rnw;
> +	cmd->tx_buf = buf;
> +	cmd->tx_len = xfer_len;
> +	cmd->type = XI3C_SDR_MODE;
> +	cmd->tid = XI3C_SDR_TID;
> +	cmd->continued = false;
> +
> +	ret = xi3c_master_common_xfer(master, xfer);
> +	kfree(xfer);
> +
> +	return ret;
> +}
> +
...
> +static struct platform_driver xi3c_master_driver = {
> +	.probe = xi3c_master_probe,
> +	.remove = xi3c_master_remove,
> +	.driver = {
> +		.name = "axi-i3c-master",
> +		.of_match_table = xi3c_master_of_ids,
> +	},
> +};
> +module_platform_driver(xi3c_master_driver);
> +
> +MODULE_AUTHOR("Manikanta Guntupalli <manikanta.guntupalli@amd.com>");
> +MODULE_DESCRIPTION("AXI I3C master driver");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [PATCH V7 4/4] i3c: master: Add AMD I3C bus controller driver

[Guntupalli, Manikanta]

[Public]

Hi,

> -----Original Message-----
> From: Frank Li <Frank.li@nxp.com>
> Sent: Wednesday, September 24, 2025 12:53 AM
> To: Guntupalli, Manikanta <manikanta.guntupalli@amd.com>
> Cc: git (AMD-Xilinx) <git@amd.com>; Simek, Michal <michal.simek@amd.com>;
> alexandre.belloni@bootlin.com; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; pgaj@cadence.com; wsa+renesas@sang-engineering.com;
> tommaso.merciai.xr@bp.renesas.com; arnd@arndb.de;
> quic_msavaliy@quicinc.com; S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>;
> sakari.ailus@linux.intel.com; billy_tsai@aspeedtech.com; kees@kernel.org;
> gustavoars@kernel.org; jarkko.nikula@linux.intel.com; jorge.marques@analog.com;
> linux-i3c@lists.infradead.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-arch@vger.kernel.org; linux-
> hardening@vger.kernel.org; Pandey, Radhey Shyam
> <radhey.shyam.pandey@amd.com>; Goud, Srinivas <srinivas.goud@amd.com>;
> Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>; manion05gk@gmail.com
> Subject: Re: [PATCH V7 4/4] i3c: master: Add AMD I3C bus controller driver
>
> On Tue, Sep 23, 2025 at 09:15:51PM +0530, Manikanta Guntupalli wrote:
> > Add an I3C master driver and maintainers fragment for the AMD I3C bus
> > controller.
> >
> > The driver currently supports the I3C bus operating in SDR i3c mode,
> > with features including Dynamic Address Assignment, private data
> > transfers, and CCC transfers in both broadcast and direct modes. It
> > also supports operation in I2C mode.
> >
> > Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
> > ---
> > Changes for V2:
> > Updated commit description.
> > Added mixed mode support with clock configuration.
> > Converted smaller functions into inline functions.
> > Used FIELD_GET() in xi3c_get_response().
> > Updated xi3c_master_rd_from_rx_fifo() to use cmd->rx_buf.
> > Used parity8() for address parity calculation.
> > Added guards for locks.
> > Dropped num_targets and updated xi3c_master_do_daa().
> > Used __free(kfree) in xi3c_master_send_bdcast_ccc_cmd().
> > Dropped PM runtime support.
> > Updated xi3c_master_read() and xi3c_master_write() with
> > xi3c_is_resp_available() check.
> > Created separate functions: xi3c_master_init() and xi3c_master_reinit().
> > Used xi3c_master_init() in bus initialization and xi3c_master_reinit()
> > in error paths.
> > Added DAA structure to xi3c_master structure.
> >
> > Changes for V3:
> > Resolved merge conflicts.
> >
> > Changes for V4:
> > Updated timeout macros.
> > Removed type casting for xi3c_is_resp_available() macro.
> > Used ioread32() and iowrite32() instead of readl() and writel() to
> > keep consistency.
> > Read XI3C_RESET_OFFSET reg before udelay().
> > Removed xi3c_master_free_xfer() and directly used kfree().
> > Skipped checking return value of i3c_master_add_i3c_dev_locked().
> > Used devm_mutex_init() instead of mutex_init().
> >
> > Changes for V5:
> > Used GENMASK_ULL for PID mask as it's 64bit mask.
> >
> > Changes for V6:
> > Removed typecast for xi3c_getrevisionnumber(), xi3c_wrfifolevel(), and
> > xi3c_rdfifolevel().
> > Replaced dynamic allocation with a static variable for pid_bcr_dcr.
> > Fixed sparse warning in do_daa by typecasting the address parity value
> > to u8.
> > Fixed sparse warning in xi3c_master_bus_init by typecasting the pid
> > value to u64 in info.pid calculation.
> >
> > Changes for V7:
> > Updated timeout macro name.
> > Updated xi3c_master_wr_to_tx_fifo() and xi3c_master_rd_from_rx_fifo()
> > to use i3c_writel_fifo() and i3c_readl_fifo().
> > ---
> >  MAINTAINERS                         |    7 +
> >  drivers/i3c/master/Kconfig          |   16 +
> >  drivers/i3c/master/Makefile         |    1 +
> >  drivers/i3c/master/amd-i3c-master.c | 1012
> > +++++++++++++++++++++++++++
> >  4 files changed, 1036 insertions(+)
> >  create mode 100644 drivers/i3c/master/amd-i3c-master.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > 8886d66bd824..fe88efb41f5b 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -11782,6 +11782,13 @@ L: linux-i2c@vger.kernel.org
> >  S: Maintained
> >  F: drivers/i2c/i2c-stub.c
> >
> > +I3C DRIVER FOR AMD AXI I3C MASTER
> > +M: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
> > +R: Michal Simek <michal.simek@amd.com>
> > +S: Maintained
> > +F: Documentation/devicetree/bindings/i3c/xlnx,axi-i3c.yaml
> > +F: drivers/i3c/master/amd-i3c-master.c
> > +
> >  I3C DRIVER FOR ASPEED AST2600
> >  M: Jeremy Kerr <jk@codeconstruct.com.au>
> >  S: Maintained
> > diff --git a/drivers/i3c/master/Kconfig b/drivers/i3c/master/Kconfig
> > index 13df2944f2ec..4b884a678893 100644
> > --- a/drivers/i3c/master/Kconfig
> > +++ b/drivers/i3c/master/Kconfig
> > @@ -1,4 +1,20 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> > +
> > +config AMD_I3C_MASTER
> > +   tristate "AMD I3C Master driver"
> > +   depends on I3C
> > +   depends on HAS_IOMEM
> > +   help
> > +     Support for AMD I3C Master Controller.
> > +
> > +     This driver allows communication with I3C devices using the AMD
> > +     I3C master, currently supporting Standard Data Rate (SDR) mode.
> > +     Features include Dynamic Address Assignment, private transfers,
> > +     and CCC transfers in both broadcast and direct modes.
> > +
> > +     This driver can also be built as a module.  If so, the module
> > +     will be called amd-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 aac74f3e3851..109bd48cb159 100644
> > --- a/drivers/i3c/master/Makefile
> > +++ b/drivers/i3c/master/Makefile
> > @@ -1,4 +1,5 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> > +obj-$(CONFIG_AMD_I3C_MASTER)               += amd-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/amd-i3c-master.c
> > b/drivers/i3c/master/amd-i3c-master.c
> > new file mode 100644
> > index 000000000000..c89f7de85635
> > --- /dev/null
> > +++ b/drivers/i3c/master/amd-i3c-master.c
> > @@ -0,0 +1,1012 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * I3C master driver for the AMD I3C controller.
> > + *
> > + * Copyright (C) 2025, Advanced Micro Devices, Inc.
> > + */
> > +
>
> ...
>
> > +
> > +static inline int xi3c_master_common_xfer(struct xi3c_master *master,
> > +                                     struct xi3c_xfer *xfer)
> > +{
> > +   int ret, timeout;
> > +
> > +   guard(mutex)(&master->lock);
> > +
> > +   xi3c_master_enqueue_xfer(master, xfer);
> > +   timeout = wait_for_completion_timeout(&xfer->comp,
> > +                                         XI3C_XFER_TIMEOUT_JIFFIES);
>
> Recent patch use time_left instead of timeout.
We will update.
>
> > +   if (!timeout)
> > +           ret = -ETIMEDOUT;
> > +   else
> > +           ret = xfer->ret;
> > +
> > +   if (ret)
> > +           xi3c_master_dequeue_xfer(master, xfer);
> > +
> > +   return ret;
> > +}
> > +
> > +static int xi3c_master_do_daa(struct i3c_master_controller *m) {
> > +   struct xi3c_master *master = to_xi3c_master(m);
> > +   struct xi3c_cmd *daa_cmd;
> > +   struct xi3c_xfer *xfer;
>
> struct xi3c_xfer __free(kfree) *xfer;
> will simple err path.
We will fix.
>
> > +   u8 pid_bufs[XI3C_MAX_DEVS][8];
> > +   u8 data, last_addr = 0;
> > +   int addr, ret, i;
> > +   u8 *pid_buf;
>
> try keep reverise christmas tree order
We will fix.
> > +
> > +   xfer = xi3c_master_alloc_xfer(master, 1);
>
> only 1 xfer, needn't alloc from heap. Just use stack varible should be enough.
xi3c_master_alloc_xfer() handles both memory allocation and structure initialization. If we replace it with a stack variable in this case, we would need to duplicate the initialization logic in several places. Using the helper keeps the code consistent and avoids repetition.
>
> > +   if (!xfer) {
> > +           ret = -ENOMEM;
> > +           goto err_daa_mem;
> > +   }
> > +
> > +   for (i = 0; i < XI3C_MAX_DEVS; i++) {
> > +           addr = i3c_master_get_free_addr(m, last_addr + 1);
> > +           if (addr < 0) {
> > +                   ret = -ENOSPC;
> > +                   goto err_daa;
> > +           }
> > +           master->daa.addrs[i] = (u8)addr;
> > +           last_addr = (u8)addr;
> > +   }
> > +
> > +   /* Fill ENTDAA CCC */
> > +   data = I3C_CCC_ENTDAA;
> > +   daa_cmd = &xfer->cmds[0];
> > +   daa_cmd->addr = I3C_BROADCAST_ADDR;
> > +   daa_cmd->rnw = 0;
> > +   daa_cmd->tx_buf = &data;
> > +   daa_cmd->tx_len = 1;
> > +   daa_cmd->type = XI3C_SDR_MODE;
> > +   daa_cmd->tid = XI3C_SDR_TID;
> > +   daa_cmd->continued = true;
> > +
> > +   ret = xi3c_master_common_xfer(master, xfer);
> > +   /* DAA always finishes with CE2_ERROR or NACK_RESP */
> > +   if (ret && ret != I3C_ERROR_M2) {
> > +           goto err_daa;
> > +   } else {
> > +           if (ret && ret == I3C_ERROR_M2) {
> > +                   ret = 0;
> > +                   goto err_daa;
> > +           }
> > +   }
> > +
> > +   master->daa.index = 0;
> > +
> > +   while (true) {
> > +           struct xi3c_cmd *cmd = &xfer->cmds[0];
> > +
> > +           pid_buf = pid_bufs[master->daa.index];
> > +           addr = (master->daa.addrs[master->daa.index] << 1) |
> > +                  (u8)(!parity8(master->daa.addrs[master->daa.index]));
> > +
> > +           cmd->tx_buf = (u8 *)&addr;
> > +           cmd->tx_len = 1;
> > +           cmd->addr = I3C_BROADCAST_ADDR;
> > +           cmd->rnw = 1;
> > +           cmd->rx_buf = pid_buf;
> > +           cmd->rx_len = XI3C_DAA_SLAVEINFO_READ_BYTECOUNT;
> > +           cmd->is_daa = true;
> > +           cmd->type = XI3C_SDR_MODE;
> > +           cmd->tid = XI3C_SDR_TID;
> > +           cmd->continued = true;
> > +
> > +           ret = xi3c_master_common_xfer(master, xfer);
> > +
> > +           /* DAA always finishes with CE2_ERROR or NACK_RESP */
> > +           if (ret && ret != I3C_ERROR_M2) {
> > +                   goto err_daa;
> > +           } else {
> > +                   if (ret && ret == I3C_ERROR_M2) {
> > +                           xi3c_master_resume(master);
> > +                           master->daa.index--;
> > +                           ret = 0;
> > +                           break;
> > +                   }
> > +           }
> > +   }
> > +
> > +   kfree(xfer);
> > +
> > +   for (i = 0; i < master->daa.index; i++) {
> > +           i3c_master_add_i3c_dev_locked(m, master->daa.addrs[i]);
> > +
> > +           u64 data = FIELD_GET(XI3C_PID_MASK,
> > +get_unaligned_be64(pid_bufs[i]));
> > +
> > +           dev_info(master->dev, "Client %d: PID: 0x%llx\n", i, data);
> > +   }
> > +
> > +   return 0;
> > +
> > +err_daa:
> > +   kfree(xfer);
> > +err_daa_mem:
> > +   xi3c_master_reinit(master);
> > +   return ret;
> > +}
> > +
>
> > +static int xi3c_master_send_bdcast_ccc_cmd(struct xi3c_master *master,
> > +                                      struct i3c_ccc_cmd *ccc)
> > +{
> > +   u16 xfer_len = ccc->dests[0].payload.len + 1;
> > +   struct xi3c_xfer *xfer;
> > +   struct xi3c_cmd *cmd;
> > +   int ret;
> > +
> > +   xfer = xi3c_master_alloc_xfer(master, 1);
> > +   if (!xfer)
> > +           return -ENOMEM;
>
> the same here. use struct xi3c_xfer xfer should be more simple.
Same reasoning as mentioned above - xi3c_master_alloc_xfer() handles both allocation and initialization, avoiding code duplication.

> > +
> > +   u8 *buf __free(kfree) = kmalloc(xfer_len, GFP_KERNEL);
> > +   if (!buf) {
> > +           kfree(xfer);
> > +           return -ENOMEM;
> > +   }
> > +
> > +   buf[0] = ccc->id;
> > +   memcpy(&buf[1], ccc->dests[0].payload.data,
> > +ccc->dests[0].payload.len);
> > +
> > +   cmd = &xfer->cmds[0];
> > +   cmd->addr = ccc->dests[0].addr;
> > +   cmd->rnw = ccc->rnw;
> > +   cmd->tx_buf = buf;
> > +   cmd->tx_len = xfer_len;
> > +   cmd->type = XI3C_SDR_MODE;
> > +   cmd->tid = XI3C_SDR_TID;
> > +   cmd->continued = false;
> > +
> > +   ret = xi3c_master_common_xfer(master, xfer);
> > +   kfree(xfer);
> > +
> > +   return ret;
> > +}
> > +
> ...
> > +static struct platform_driver xi3c_master_driver = {
> > +   .probe = xi3c_master_probe,
> > +   .remove = xi3c_master_remove,
> > +   .driver = {
> > +           .name = "axi-i3c-master",
> > +           .of_match_table = xi3c_master_of_ids,
> > +   },
> > +};
> > +module_platform_driver(xi3c_master_driver);
> > +
> > +MODULE_AUTHOR("Manikanta Guntupalli <manikanta.guntupalli@amd.com>");
> > +MODULE_DESCRIPTION("AXI I3C master driver");
> MODULE_LICENSE("GPL");
> > --
> > 2.34.1
> >

Thanks,
Manikanta.

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c