[PATCH RFC 00/13] dmaengine: Smart Data Accelerator Interface (SDXI) basic support

[PATCH RFC 00/13] dmaengine: Smart Data Accelerator Interface (SDXI) basic support

[Nathan Lynch via B4 Relay]

The Smart Data Accelerator Interface (SDXI) is a vendor-neutral
architecture for memory-to-memory data movement offload designed for
kernel bypass and virtualization.

General information on SDXI may be found at:
https://www.snia.org/sdxi

This submission adds a driver with basic support for PCIe-hosted SDXI 1.0
implementations and includes a DMA engine provider.

It is very much a work in progress. Among other issues, the DMA
provider code only supports single-threaded polled mode, and context
management should use better data structures.

While we're addressing those shortcomings, we'd appreciate any
feedback on:

* Where the code should live. SDXI entails a fair amount of code for
  context and descriptor management, and we expect to eventually add a
  character device ABI for user space access. Should all of this go in
  drivers/dma/sdxi?

* Whether the DMA engine provider should use virt-dma/vchan. SDXI
  submission queues can be almost arbitrarily large, and I'm not sure
  putting a software queue in front of that makes sense.

Planned future SDXI work (out of scope for this series):

* Character device for user space access. We are evaluating the uacce
  framework for this.

* Support for operation types to be added in future SDXI revisions.

* Greater configurability for control structures, e.g. descriptor ring
  size.

The latest released version of the SDXI specification is 1.0:
https://www.snia.org/sites/default/files/technical-work/sdxi/release/SNIA-SDXI-Specification-v1.0a.pdf

Draft versions of future SDXI specifications in development may be found at:
https://www.snia.org/tech_activities/publicreview#sdxi

---
Nathan Lynch (13):
      PCI: Add SNIA SDXI accelerator sub-class
      dmaengine: sdxi: Add control structure definitions
      dmaengine: sdxi: Add descriptor encoding and unit tests
      dmaengine: sdxi: Add MMIO register definitions
      dmaengine: sdxi: Add software data structures
      dmaengine: sdxi: Add error reporting support
      dmaengine: sdxi: Import descriptor enqueue code from spec
      dmaengine: sdxi: Context creation/removal, descriptor submission
      dmaengine: sdxi: Add core device management code
      dmaengine: sdxi: Add PCI driver support
      dmaengine: sdxi: Add DMA engine provider
      dmaengine: sdxi: Add Kconfig and Makefile
      MAINTAINERS: Add entry for SDXI driver

 MAINTAINERS                         |   7 +
 drivers/dma/Kconfig                 |   2 +
 drivers/dma/Makefile                |   1 +
 drivers/dma/sdxi/.kunitconfig       |   4 +
 drivers/dma/sdxi/Kconfig            |  23 ++
 drivers/dma/sdxi/Makefile           |  17 ++
 drivers/dma/sdxi/context.c          | 547 ++++++++++++++++++++++++++++++++++++
 drivers/dma/sdxi/context.h          |  28 ++
 drivers/dma/sdxi/descriptor.c       | 197 +++++++++++++
 drivers/dma/sdxi/descriptor.h       | 107 +++++++
 drivers/dma/sdxi/descriptor_kunit.c | 181 ++++++++++++
 drivers/dma/sdxi/device.c           | 401 ++++++++++++++++++++++++++
 drivers/dma/sdxi/dma.c              | 409 +++++++++++++++++++++++++++
 drivers/dma/sdxi/dma.h              |  12 +
 drivers/dma/sdxi/enqueue.c          | 136 +++++++++
 drivers/dma/sdxi/enqueue.h          |  16 ++
 drivers/dma/sdxi/error.c            | 340 ++++++++++++++++++++++
 drivers/dma/sdxi/error.h            |  16 ++
 drivers/dma/sdxi/hw.h               | 249 ++++++++++++++++
 drivers/dma/sdxi/mmio.h             |  92 ++++++
 drivers/dma/sdxi/pci.c              | 216 ++++++++++++++
 drivers/dma/sdxi/sdxi.h             | 206 ++++++++++++++
 include/linux/pci_ids.h             |   1 +
 23 files changed, 3208 insertions(+)
---
base-commit: c17b750b3ad9f45f2b6f7e6f7f4679844244f0b9
change-id: 20250813-sdxi-base-73d7c9fdce57

Best regards,
-- 
Nathan Lynch <nathan.lynch@amd.com>

[PATCH RFC 01/13] PCI: Add SNIA SDXI accelerator sub-class

[Nathan Lynch via B4 Relay]

From: Nathan Lynch <nathan.lynch@amd.com>

This was added to the PCI Code and ID Assignment Specification in
revision 1.14 (2021). Refer to 1.19. "Base Class 12h" of that document
as well as the "SDXI PCI-Express Device Architecture" chapter of the
SDXI specification:

"""
  SDXI functions are expected to be identified through the SDXI class
  code.
  * SNIA Smart Data Accelerator Interface (SDXI) controller:
    * Base Class = 0x12
    * Sub Class = 0x01
    * Programming Interface = 0x0
"""

Information about SDXI may be found at the SNIA website:

  https://www.snia.org/sdxi

Co-developed-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
---
 include/linux/pci_ids.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 92ffc4373f6de3dcf82226a50d0e36af366e888e..ac9bb3d64949919019d40d1f86cd3658bfb1c661 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -154,6 +154,7 @@
 
 #define PCI_BASE_CLASS_ACCELERATOR	0x12
 #define PCI_CLASS_ACCELERATOR_PROCESSING	0x1200
+#define PCI_CLASS_ACCELERATOR_SDXI		0x120100
 
 #define PCI_CLASS_OTHERS		0xff
 

-- 
2.39.5

Re: [PATCH RFC 01/13] PCI: Add SNIA SDXI accelerator sub-class

[Bjorn Helgaas]

On Fri, Sep 05, 2025 at 01:48:24PM -0500, Nathan Lynch via B4 Relay wrote:
> From: Nathan Lynch <nathan.lynch@amd.com>
> 
> This was added to the PCI Code and ID Assignment Specification in
> revision 1.14 (2021). Refer to 1.19. "Base Class 12h" of that document
> as well as the "SDXI PCI-Express Device Architecture" chapter of the
> SDXI specification:

Would prefer if this said:

  Add sub-class code for SNIA Smart Data Accelerator Interface (SDXI).
  See PCI Code and ID Assignment spec r1.14, sec 1.19.

so the antecedent of "this" is here instead of in the subject and
"1.19" doesn't get confused with a spec revision.

> """
>   SDXI functions are expected to be identified through the SDXI class
>   code.
>   * SNIA Smart Data Accelerator Interface (SDXI) controller:
>     * Base Class = 0x12
>     * Sub Class = 0x01
>     * Programming Interface = 0x0
> """
> 
> Information about SDXI may be found at the SNIA website:
> 
>   https://www.snia.org/sdxi

I don't think the SDXI spec material is really necessary.  The PCI
Code and ID spec is definitive for class codes.

> Co-developed-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  include/linux/pci_ids.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 92ffc4373f6de3dcf82226a50d0e36af366e888e..ac9bb3d64949919019d40d1f86cd3658bfb1c661 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -154,6 +154,7 @@
>  
>  #define PCI_BASE_CLASS_ACCELERATOR	0x12
>  #define PCI_CLASS_ACCELERATOR_PROCESSING	0x1200
> +#define PCI_CLASS_ACCELERATOR_SDXI		0x120100
>  
>  #define PCI_CLASS_OTHERS		0xff
>  
> 
> -- 
> 2.39.5
> 
> 

Re: [PATCH RFC 01/13] PCI: Add SNIA SDXI accelerator sub-class

[Nathan Lynch]

Bjorn Helgaas <helgaas@kernel.org> writes:
> On Fri, Sep 05, 2025 at 01:48:24PM -0500, Nathan Lynch via B4 Relay wrote:
>> From: Nathan Lynch <nathan.lynch@amd.com>
>> 
>> This was added to the PCI Code and ID Assignment Specification in
>> revision 1.14 (2021). Refer to 1.19. "Base Class 12h" of that document
>> as well as the "SDXI PCI-Express Device Architecture" chapter of the
>> SDXI specification:
>
> Would prefer if this said:
>
>   Add sub-class code for SNIA Smart Data Accelerator Interface (SDXI).
>   See PCI Code and ID Assignment spec r1.14, sec 1.19.
>
> so the antecedent of "this" is here instead of in the subject and
> "1.19" doesn't get confused with a spec revision.
>
>> """
>>   SDXI functions are expected to be identified through the SDXI class
>>   code.
>>   * SNIA Smart Data Accelerator Interface (SDXI) controller:
>>     * Base Class = 0x12
>>     * Sub Class = 0x01
>>     * Programming Interface = 0x0
>> """
>> 
>> Information about SDXI may be found at the SNIA website:
>> 
>>   https://www.snia.org/sdxi
>
> I don't think the SDXI spec material is really necessary.  The PCI
> Code and ID spec is definitive for class codes.
>
>> Co-developed-by: Wei Huang <wei.huang2@amd.com>
>> Signed-off-by: Wei Huang <wei.huang2@amd.com>
>> Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Thanks Bjorn, I have made the commit message changes you've suggested
and incorporated your ack for this patch.

[PATCH RFC 02/13] dmaengine: sdxi: Add control structure definitions

[Nathan Lynch via B4 Relay]

From: Nathan Lynch <nathan.lynch@amd.com>

SDXI defines a multitude of control structures that reside in system
memory and are shared between software and the implementation.

Add:

* C struct definitions for the SDXI control structures to be used by
  the driver;
* Bitmask constants for accessing fields and subfields of the control
  structures;
* Symbolic constants corresponding to significant values such as
  context states and commands.

Co-developed-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
---
 drivers/dma/sdxi/hw.h | 249 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 249 insertions(+)

diff --git a/drivers/dma/sdxi/hw.h b/drivers/dma/sdxi/hw.h
new file mode 100644
index 0000000000000000000000000000000000000000..4ac0e200773b0646e84a65794e02cdf9e583db6d
--- /dev/null
+++ b/drivers/dma/sdxi/hw.h
@@ -0,0 +1,249 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Copyright (C) 2025 Advanced Micro Devices, Inc. */
+
+/*
+ * Control structures and constants defined in the SDXI specification,
+ * with low-level accessors. The ordering of the structures here
+ * follows the order of their definitions in the SDXI spec.
+ *
+ * Names of structures, members, and subfields (bit ranges within
+ * members) are written to match the spec, generally. E.g. struct
+ * sdxi_cxt_l2_ent corresponds to CXT_L2_ENT in the spec.
+ *
+ * Note: a member can have a subfield whose name is identical to the
+ * member's name. E.g. CXT_L2_ENT's lv01_ptr.
+ *
+ * All reserved fields and bits (usually named "rsvd" or some
+ * variation) must be set to zero by the driver unless otherwise
+ * specified.
+ */
+
+#ifndef LINUX_SDXI_HW_H
+#define LINUX_SDXI_HW_H
+
+#include <asm/byteorder.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/build_bug.h>
+#include <linux/log2.h>
+#include <linux/stddef.h>
+#include <linux/types.h>
+
+/* Context Level 2 Table Entry (CXT_L2_ENT) */
+struct sdxi_cxt_l2_ent {
+	__le64 lv01_ptr;
+#define SDXI_CXT_L2_ENT_LV01_PTR GENMASK_ULL(63, 12)
+#define SDXI_CXT_L2_ENT_VL       BIT_ULL(0)
+} __packed;
+static_assert(sizeof(struct sdxi_cxt_l2_ent) == 8);
+
+/*
+ * The level 2 table is 4KB and has 512 level 1 pointer entries.
+ */
+#define SDXI_L2_TABLE_ENTRIES 512
+struct sdxi_cxt_l2_table {
+	struct sdxi_cxt_l2_ent entry[SDXI_L2_TABLE_ENTRIES];
+};
+static_assert(sizeof(struct sdxi_cxt_l2_table) == 4096);
+
+/* Context level 1 table entry (CXT_L1_ENT) */
+struct sdxi_cxt_l1_ent {
+	__le64 cxt_ctl_ptr;
+#define SDXI_CXT_L1_ENT_VL             BIT_ULL(0)
+#define SDXI_CXT_L1_ENT_KA             BIT_ULL(1)
+#define SDXI_CXT_L1_ENT_PV             BIT_ULL(2)
+#define SDXI_CXT_L1_ENT_CXT_CTL_PTR    GENMASK_ULL(63, 6)
+	__le64 akey_ptr;
+#define SDXI_CXT_L1_ENT_AKEY_SZ        GENMASK_ULL(3, 0)
+#define SDXI_CXT_L1_ENT_AKEY_PTR       GENMASK_ULL(63, 12)
+	__le32 misc0;
+#define SDXI_CXT_L1_ENT_PASID          GENMASK(19, 0)
+#define SDXI_CXT_L1_ENT_MAX_BUFFER     GENMASK(23, 20)
+	__le32 opb_000_enb;
+	__u8 rsvd_0[8];
+} __packed;
+static_assert(sizeof(struct sdxi_cxt_l1_ent) == 32);
+
+#define SDXI_L1_TABLE_ENTRIES 128
+struct sdxi_cxt_l1_table {
+	struct sdxi_cxt_l1_ent entry[SDXI_L1_TABLE_ENTRIES];
+};
+static_assert(sizeof(struct sdxi_cxt_l1_table) == 4096);
+
+/* Context control block (CXT_CTL) */
+struct sdxi_cxt_ctl {
+	__le64 ds_ring_ptr;
+#define SDXI_CXT_CTL_VL             BIT_ULL(0)
+#define SDXI_CXT_CTL_QOS            GENMASK_ULL(3, 2)
+#define SDXI_CXT_CTL_SE             BIT_ULL(4)
+#define SDXI_CXT_CTL_CSA            BIT_ULL(5)
+#define SDXI_CXT_CTL_DS_RING_PTR    GENMASK_ULL(63, 6)
+	__le32 ds_ring_sz;
+	__u8 rsvd_0[4];
+	__le64 cxt_sts_ptr;
+#define SDXI_CXT_CTL_CXT_STS_PTR    GENMASK_ULL(63, 4)
+	__le64 write_index_ptr;
+#define SDXI_CXT_CTL_WRITE_INDEX_PTR GENMASK_ULL(63, 3)
+	__u8 rsvd_1[32];
+} __packed;
+static_assert(sizeof(struct sdxi_cxt_ctl) == 64);
+
+/* Context Status (CXT_STS) */
+struct sdxi_cxt_sts {
+	__u8 state;
+#define SDXI_CXT_STS_STATE GENMASK(3, 0)
+	__u8 misc0;
+	__u8 rsvd_0[6];
+	__le64 read_index;
+} __packed;
+static_assert(sizeof(struct sdxi_cxt_sts) == 16);
+
+/* Valid values for FIELD_GET(SDXI_CXT_STS_STATE, sdxi_cxt_sts.state) */
+enum cxt_sts_state {
+	CXTV_STOP_SW  = 0x0,
+	CXTV_RUN      = 0x1,
+	CXTV_STOPG_SW = 0x2,
+	CXTV_STOP_FN  = 0x4,
+	CXTV_STOPG_FN = 0x6,
+	CXTV_ERR_FN   = 0xf,
+};
+
+static inline enum cxt_sts_state sdxi_cxt_sts_state(const struct sdxi_cxt_sts *sts)
+{
+	return FIELD_GET(SDXI_CXT_STS_STATE, READ_ONCE(sts->state));
+}
+
+/* Access key entry (AKEY_ENT) */
+struct sdxi_akey_ent {
+	__le16 intr_num;
+#define SDXI_AKEY_ENT_VL BIT(0)
+#define SDXI_AKEY_ENT_PV BIT(2)
+	__le16 tgt_sfunc;
+	__le32 pasid;
+#define SDXI_AKEY_ENT_PASID GENMASK(19, 0)
+	__le16 stag;
+	__u8   rsvd_0[2];
+	__le16 rkey;
+	__u8   rsvd_1[2];
+} __packed;
+static_assert(sizeof(struct sdxi_akey_ent) == 16);
+
+/* Error Log Header Entry (ERRLOG_HD_ENT) */
+struct sdxi_errlog_hd_ent {
+	__le32 opcode;
+	__le16 misc0;
+	__le16 cxt_num;
+	__le64 dsc_index;
+	__u8   rsvd_0[28];
+	__le16 err_class;
+	__u8   rsvd_1[2];
+	__le32 vendor[4];
+} __packed;
+static_assert(sizeof(struct sdxi_errlog_hd_ent) == 64);
+
+/* Completion status block (CST_BLK) */
+struct sdxi_cst_blk {
+	__le64 signal;
+	__le32 flags;
+#define SDXI_CST_BLK_ER_BIT BIT(31)
+	__u8 rsvd_0[20];
+} __packed;
+static_assert(sizeof(struct sdxi_cst_blk) == 32);
+
+/*
+ * Size of the "body" of each descriptor between the common opcode and
+ * csb_ptr fields.
+ */
+#define DSC_OPERATION_BYTES 52
+
+#define define_sdxi_dsc(tag_, name_, op_body_)				\
+	struct tag_ {							\
+		__le32 opcode;						\
+		op_body_						\
+		__le64 csb_ptr;						\
+	} name_;							\
+	static_assert(sizeof(struct tag_) ==				\
+		      sizeof(struct sdxi_dsc_generic));			\
+	static_assert(offsetof(struct tag_, csb_ptr) ==			\
+		      offsetof(struct sdxi_dsc_generic, csb_ptr))
+
+struct sdxi_desc {
+	union {
+		__le64 qw[8];
+
+		/* DSC_GENERIC - common header and footer */
+		struct_group_tagged(sdxi_dsc_generic, generic,
+			__le32 opcode;
+#define SDXI_DSC_VL  BIT(0)
+#define SDXI_DSC_SE  BIT(1)
+#define SDXI_DSC_FE  BIT(2)
+#define SDXI_DSC_CH  BIT(3)
+#define SDXI_DSC_CSR BIT(4)
+#define SDXI_DSC_RB  BIT(5)
+#define SDXI_DSC_FLAGS   GENMASK(5, 0)
+#define SDXI_DSC_SUBTYPE GENMASK(15, 8)
+#define SDXI_DSC_TYPE    GENMASK(26, 16)
+			__u8 operation[DSC_OPERATION_BYTES];
+			__le64 csb_ptr;
+#define SDXI_DSC_NP BIT_ULL(0)
+#define SDXI_DSC_CSB_PTR GENMASK_ULL(63, 5)
+		);
+
+		/* DmaBaseGrp: DSC_DMAB_NOP */
+		define_sdxi_dsc(sdxi_dsc_dmab_nop, nop,
+			__u8 rsvd_0[DSC_OPERATION_BYTES];
+		);
+
+#define SDXI_DSC_OP_TYPE_DMAB 0x001
+#define SDXI_DSC_OP_SUBTYPE_COPY 0x03
+		/* DmaBaseGrp: DSC_DMAB_COPY */
+		define_sdxi_dsc(sdxi_dsc_dmab_copy, copy,
+				__le32 size;
+				__u8 attr;
+				__u8 rsvd_0[3];
+				__le16 akey0;
+				__le16 akey1;
+				__le64 addr0;
+				__le64 addr1;
+				__u8 rsvd_1[24];
+				);
+
+#define SDXI_DSC_OP_TYPE_INTR 0x004
+#define SDXI_DSC_OP_SUBTYPE_INTR 0x00
+		/* IntrGrp: DSC_INTR */
+		define_sdxi_dsc(sdxi_dsc_intr, intr,
+			__u8 rsvd_0[8];
+			__le16 akey;
+			__u8 rsvd_1[42];
+		);
+
+#define SDXI_DSC_OP_TYPE_ADMIN 0x002
+#define SDXI_DSC_OP_SUBTYPE_CXT_START_NM 0x03
+#define SDXI_DSC_OP_SUBTYPE_CXT_START_RS 0x08
+		/* AdminGrp: DSC_CXT_START */
+		define_sdxi_dsc(sdxi_dsc_cxt_start, cxt_start,
+				__u8 rsvd_0;
+				__u8 vflags;
+				__le16 vf_num;
+				__le16 cxt_start;
+				__le16 cxt_end;
+				__u8 rsvd_1[4];
+				__le64 db_value;
+				__u8 rsvd_2[32];
+				);
+
+#define SDXI_DSC_OP_SUBTYPE_CXT_STOP     0x04
+		/* AdminGrp: DSC_CXT_STOP */
+		define_sdxi_dsc(sdxi_dsc_cxt_stop, cxt_stop,
+			__u8 rsvd_0;
+			__u8 vflags;
+			__le16 vf_num;
+			__le16 cxt_start;
+			__le16 cxt_end;
+			__u8 rsvd_1[44];
+		);
+	};
+};
+static_assert(sizeof(struct sdxi_desc) == 64);
+
+#endif /* LINUX_SDXI_HW_H */

-- 
2.39.5

[PATCH RFC 03/13] dmaengine: sdxi: Add descriptor encoding and unit tests

[Nathan Lynch via B4 Relay]

From: Nathan Lynch <nathan.lynch@amd.com>

Add support for encoding several types of SDXI descriptors:

* Copy
* Interrupt
* Context start
* Context stop

Each type of descriptor has a corresponding parameter struct which is
an input to its encoder function. E.g. to encode a copy descriptor,
the client initializes a struct sdxi_copy object with the source,
destination, size, etc and passes that to sdxi_encode_copy().

Include unit tests that verify that encoded descriptors have the
expected values and that fallible encode functions fail on invalid
inputs.

Co-developed-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
---
 drivers/dma/sdxi/.kunitconfig       |   4 +
 drivers/dma/sdxi/descriptor.c       | 197 ++++++++++++++++++++++++++++++++++++
 drivers/dma/sdxi/descriptor.h       | 107 ++++++++++++++++++++
 drivers/dma/sdxi/descriptor_kunit.c | 181 +++++++++++++++++++++++++++++++++
 4 files changed, 489 insertions(+)

diff --git a/drivers/dma/sdxi/.kunitconfig b/drivers/dma/sdxi/.kunitconfig
new file mode 100644
index 0000000000000000000000000000000000000000..a98cf19770f03bce82ef86d378d2a2e34da5154a
--- /dev/null
+++ b/drivers/dma/sdxi/.kunitconfig
@@ -0,0 +1,4 @@
+CONFIG_KUNIT=y
+CONFIG_DMADEVICES=y
+CONFIG_SDXI=y
+CONFIG_SDXI_KUNIT_TEST=y
diff --git a/drivers/dma/sdxi/descriptor.c b/drivers/dma/sdxi/descriptor.c
new file mode 100644
index 0000000000000000000000000000000000000000..6ea5247bf8cdaac19131ca5326ba1640c0b557f8
--- /dev/null
+++ b/drivers/dma/sdxi/descriptor.c
@@ -0,0 +1,197 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * SDXI descriptor encoding.
+ *
+ * Copyright (C) 2025 Advanced Micro Devices, Inc.
+ */
+
+#include <kunit/test.h>
+#include <kunit/test-bug.h>
+#include <kunit/visibility.h>
+#include <linux/align.h>
+#include <linux/bitfield.h>
+#include <linux/bitmap.h>
+#include <linux/dma-mapping.h>
+#include <linux/log2.h>
+#include <linux/packing.h>
+#include <linux/string.h>
+#include <linux/types.h>
+#include <asm/byteorder.h>
+
+#include "hw.h"
+#include "descriptor.h"
+
+enum {
+	SDXI_PACKING_QUIRKS = QUIRK_LITTLE_ENDIAN | QUIRK_LSW32_IS_FIRST,
+};
+
+#define sdxi_desc_field(_high, _low, _member) \
+	PACKED_FIELD(_high, _low, struct sdxi_desc_unpacked, _member)
+#define sdxi_desc_flag(_bit, _member) \
+	sdxi_desc_field(_bit, _bit, _member)
+
+static const struct packed_field_u16 common_descriptor_fields[] = {
+	sdxi_desc_flag(0, vl),
+	sdxi_desc_flag(1, se),
+	sdxi_desc_flag(2, fe),
+	sdxi_desc_flag(3, ch),
+	sdxi_desc_flag(4, csr),
+	sdxi_desc_flag(5, rb),
+	sdxi_desc_field(15, 8, subtype),
+	sdxi_desc_field(26, 16, type),
+	sdxi_desc_flag(448, np),
+	sdxi_desc_field(511, 453, csb_ptr),
+};
+
+void sdxi_desc_unpack(struct sdxi_desc_unpacked *to,
+		      const struct sdxi_desc *from)
+{
+	*to = (struct sdxi_desc_unpacked){};
+	unpack_fields(from, sizeof(*from), to, common_descriptor_fields,
+		      SDXI_PACKING_QUIRKS);
+}
+EXPORT_SYMBOL_IF_KUNIT(sdxi_desc_unpack);
+
+static void desc_clear(struct sdxi_desc *desc)
+{
+	memset(desc, 0, sizeof(*desc));
+}
+
+static __must_check int sdxi_encode_size32(u64 size, __le32 *dest)
+{
+	/*
+	 * sizes are encoded as value - 1:
+	 * value    encoding
+	 *     1           0
+	 *     2           1
+	 *   ...
+	 *    4G  0xffffffff
+	 */
+	if (WARN_ON_ONCE(size > SZ_4G) ||
+	    WARN_ON_ONCE(size == 0))
+		return -EINVAL;
+	size = clamp_val(size, 1, SZ_4G);
+	*dest = cpu_to_le32((u32)(size - 1));
+	return 0;
+}
+
+int sdxi_encode_copy(struct sdxi_desc *desc, const struct sdxi_copy *params)
+{
+	u64 csb_ptr;
+	u32 opcode;
+	__le32 size;
+	int err;
+
+	err = sdxi_encode_size32(params->len, &size);
+	if (err)
+		return err;
+	/*
+	 * TODO: reject overlapping src and dst. Quoting "Memory
+	 * Consistency Model": "Software shall not ... overlap the
+	 * source buffer, destination buffer, Atomic Return Data, or
+	 * completion status block."
+	 */
+
+	opcode = (FIELD_PREP(SDXI_DSC_VL, 1) |
+		  FIELD_PREP(SDXI_DSC_SUBTYPE, SDXI_DSC_OP_SUBTYPE_COPY) |
+		  FIELD_PREP(SDXI_DSC_TYPE, SDXI_DSC_OP_TYPE_DMAB));
+
+	csb_ptr = FIELD_PREP(SDXI_DSC_NP, 1);
+
+	desc_clear(desc);
+	desc->copy = (struct sdxi_dsc_dmab_copy) {
+		.opcode = cpu_to_le32(opcode),
+		.size = size,
+		.akey0 = cpu_to_le16(params->src_akey),
+		.akey1 = cpu_to_le16(params->dst_akey),
+		.addr0 = cpu_to_le64(params->src),
+		.addr1 = cpu_to_le64(params->dst),
+		.csb_ptr = cpu_to_le64(csb_ptr),
+	};
+
+	return 0;
+}
+EXPORT_SYMBOL_IF_KUNIT(sdxi_encode_copy);
+
+int sdxi_encode_intr(struct sdxi_desc *desc,
+		     const struct sdxi_intr *params)
+{
+	u64 csb_ptr;
+	u32 opcode;
+
+	opcode = (FIELD_PREP(SDXI_DSC_VL, 1) |
+		  FIELD_PREP(SDXI_DSC_SUBTYPE, SDXI_DSC_OP_SUBTYPE_INTR) |
+		  FIELD_PREP(SDXI_DSC_TYPE, SDXI_DSC_OP_TYPE_INTR));
+
+	csb_ptr = FIELD_PREP(SDXI_DSC_NP, 1);
+
+	desc_clear(desc);
+	desc->intr = (struct sdxi_dsc_intr) {
+		.opcode = cpu_to_le32(opcode),
+		.akey = cpu_to_le16(params->akey),
+		.csb_ptr = cpu_to_le64(csb_ptr),
+	};
+
+	return 0;
+}
+EXPORT_SYMBOL_IF_KUNIT(sdxi_encode_intr);
+
+int sdxi_encode_cxt_start(struct sdxi_desc *desc,
+			  const struct sdxi_cxt_start *params)
+{
+	u16 cxt_start;
+	u16 cxt_end;
+	u64 csb_ptr;
+	u32 opcode;
+
+	opcode = (FIELD_PREP(SDXI_DSC_VL, 1) |
+		  FIELD_PREP(SDXI_DSC_FE, 1) |
+		  FIELD_PREP(SDXI_DSC_SUBTYPE, SDXI_DSC_OP_SUBTYPE_CXT_START_NM) |
+		  FIELD_PREP(SDXI_DSC_TYPE, SDXI_DSC_OP_TYPE_ADMIN));
+
+	cxt_start = params->range.cxt_start;
+	cxt_end = params->range.cxt_end;
+
+	csb_ptr = FIELD_PREP(SDXI_DSC_NP, 1);
+
+	desc_clear(desc);
+	desc->cxt_start = (struct sdxi_dsc_cxt_start) {
+		.opcode = cpu_to_le32(opcode),
+		.cxt_start = cpu_to_le16(cxt_start),
+		.cxt_end = cpu_to_le16(cxt_end),
+		.csb_ptr = cpu_to_le64(csb_ptr),
+	};
+
+	return 0;
+}
+EXPORT_SYMBOL_IF_KUNIT(sdxi_encode_cxt_start);
+
+int sdxi_encode_cxt_stop(struct sdxi_desc *desc,
+			  const struct sdxi_cxt_stop *params)
+{
+	u16 cxt_start;
+	u16 cxt_end;
+	u64 csb_ptr;
+	u32 opcode;
+
+	opcode = (FIELD_PREP(SDXI_DSC_VL, 1) |
+		  FIELD_PREP(SDXI_DSC_FE, 1) |
+		  FIELD_PREP(SDXI_DSC_SUBTYPE, SDXI_DSC_OP_SUBTYPE_CXT_STOP) |
+		  FIELD_PREP(SDXI_DSC_TYPE, SDXI_DSC_OP_TYPE_ADMIN));
+
+	cxt_start = params->range.cxt_start;
+	cxt_end = params->range.cxt_end;
+
+	csb_ptr = FIELD_PREP(SDXI_DSC_NP, 1);
+
+	desc_clear(desc);
+	desc->cxt_stop = (struct sdxi_dsc_cxt_stop) {
+		.opcode = cpu_to_le32(opcode),
+		.cxt_start = cpu_to_le16(cxt_start),
+		.cxt_end = cpu_to_le16(cxt_end),
+		.csb_ptr = cpu_to_le64(csb_ptr),
+	};
+
+	return 0;
+}
+EXPORT_SYMBOL_IF_KUNIT(sdxi_encode_cxt_stop);
diff --git a/drivers/dma/sdxi/descriptor.h b/drivers/dma/sdxi/descriptor.h
new file mode 100644
index 0000000000000000000000000000000000000000..141463dfd56bd4a88b4b3c9d45b13cc8101e1961
--- /dev/null
+++ b/drivers/dma/sdxi/descriptor.h
@@ -0,0 +1,107 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef DMA_SDXI_DESCRIPTOR_H
+#define DMA_SDXI_DESCRIPTOR_H
+
+/*
+ * Facilities for encoding SDXI descriptors.
+ *
+ * Copyright (C) 2025 Advanced Micro Devices, Inc.
+ */
+
+#include <asm/byteorder.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/errno.h>
+#include <linux/minmax.h>
+#include <linux/sizes.h>
+#include <linux/stddef.h>
+#include <linux/types.h>
+
+#include "hw.h"
+
+static inline void sdxi_desc_set_csb(struct sdxi_desc *desc,
+				     dma_addr_t addr)
+{
+	desc->csb_ptr = cpu_to_le64(FIELD_PREP(SDXI_DSC_CSB_PTR, addr >> 5));
+}
+
+struct sdxi_cxt_range {
+	u16 cxt_start;
+	u16 cxt_end;
+};
+
+static inline struct sdxi_cxt_range __sdxi_cxt_range(u16 a, u16 b)
+{
+	return (struct sdxi_cxt_range) {
+		.cxt_start = min(a, b),
+		.cxt_end   = max(a, b),
+	};
+}
+
+#define sdxi_cxt_range_1(_id)			\
+	({					\
+		u16 id = (_id);			\
+		__sdxi_cxt_range(id, id);	\
+	})
+
+#define sdxi_cxt_range_2(_id1, _id2) __sdxi_cxt_range(_id1, _id2)
+
+#define _sdxi_cxt_range(_1, _2, _fn, ...) _fn
+
+#define sdxi_cxt_range(...)						\
+	_sdxi_cxt_range(__VA_ARGS__,					\
+			sdxi_cxt_range_2, sdxi_cxt_range_1)(__VA_ARGS__)
+
+struct sdxi_copy {
+	dma_addr_t src;
+	dma_addr_t dst;
+	size_t len;
+	u16 src_akey;
+	u16 dst_akey;
+};
+
+int sdxi_encode_copy(struct sdxi_desc *desc,
+		     const struct sdxi_copy *params);
+
+struct sdxi_intr {
+	u16 akey;
+};
+
+int sdxi_encode_intr(struct sdxi_desc *desc,
+		     const struct sdxi_intr *params);
+
+struct sdxi_cxt_start {
+	struct sdxi_cxt_range range;
+};
+
+int sdxi_encode_cxt_start(struct sdxi_desc *desc,
+			  const struct sdxi_cxt_start *params);
+
+struct sdxi_cxt_stop {
+	struct sdxi_cxt_range range;
+};
+
+int sdxi_encode_cxt_stop(struct sdxi_desc *desc,
+			  const struct sdxi_cxt_stop *params);
+
+/*
+ * Fields common to all SDXI descriptors in "unpacked" form, for use
+ * with pack_fields() and unpack_fields().
+ */
+struct sdxi_desc_unpacked {
+	u64 csb_ptr;
+	u16 type;
+	u8 subtype;
+	bool vl;
+	bool se;
+	bool fe;
+	bool ch;
+	bool csr;
+	bool rb;
+	bool np;
+};
+
+void sdxi_desc_unpack(struct sdxi_desc_unpacked *to,
+		      const struct sdxi_desc *from);
+
+#endif /* DMA_SDXI_DESCRIPTOR_H */
diff --git a/drivers/dma/sdxi/descriptor_kunit.c b/drivers/dma/sdxi/descriptor_kunit.c
new file mode 100644
index 0000000000000000000000000000000000000000..eb89d5a152cd789fb8cfa66b78bf30e583a1680d
--- /dev/null
+++ b/drivers/dma/sdxi/descriptor_kunit.c
@@ -0,0 +1,181 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * SDXI descriptor encoding tests.
+ *
+ * Copyright (C) 2025 Advanced Micro Devices, Inc.
+ */
+#include <kunit/device.h>
+#include <kunit/test-bug.h>
+#include <kunit/test.h>
+#include <linux/container_of.h>
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+#include <linux/string.h>
+
+#include "descriptor.h"
+
+MODULE_IMPORT_NS("EXPORTED_FOR_KUNIT_TESTING");
+
+static void desc_poison(struct sdxi_desc *d)
+{
+	memset(d, 0xff, sizeof(*d));
+}
+
+static void copy(struct kunit *t)
+{
+	struct sdxi_desc_unpacked unpacked;
+	struct sdxi_copy copy = {};
+	struct sdxi_desc desc = {};
+
+	desc_poison(&desc);
+	KUNIT_EXPECT_EQ(t, -EINVAL, sdxi_encode_copy(&desc, &copy));
+
+	desc_poison(&desc);
+	copy.len = SZ_4G + 1;
+	KUNIT_EXPECT_EQ(t, -EINVAL, sdxi_encode_copy(&desc, &copy));
+
+	desc_poison(&desc);
+	copy.len = 1;
+	KUNIT_EXPECT_EQ(t, 0, sdxi_encode_copy(&desc, &copy));
+
+	desc_poison(&desc);
+	copy.len = SZ_4G;
+	KUNIT_EXPECT_EQ(t, 0, sdxi_encode_copy(&desc, &copy));
+	KUNIT_EXPECT_EQ(t, SZ_4G - 1, le32_to_cpu(desc.copy.size));
+
+	desc_poison(&desc);
+	KUNIT_EXPECT_EQ(t, 0,
+			sdxi_encode_copy(&desc,
+					 &(struct sdxi_copy) {
+						 .src = 0x1000,
+						 .dst = 0x2000,
+						 .len = 0x100,
+						 .src_akey = 1,
+						 .dst_akey = 2,
+					 }));
+	KUNIT_EXPECT_EQ(t, 0x1000, le64_to_cpu(desc.copy.addr0));
+	KUNIT_EXPECT_EQ(t, 0x2000, le64_to_cpu(desc.copy.addr1));
+	KUNIT_EXPECT_EQ(t, 0x100, 1 + le32_to_cpu(desc.copy.size));
+	KUNIT_EXPECT_EQ(t, 1, le16_to_cpu(desc.copy.akey0));
+	KUNIT_EXPECT_EQ(t, 2, le16_to_cpu(desc.copy.akey1));
+
+	sdxi_desc_unpack(&unpacked, &desc);
+	KUNIT_EXPECT_EQ(t, unpacked.vl, 1);
+	KUNIT_EXPECT_EQ(t, unpacked.ch, 0);
+	KUNIT_EXPECT_EQ(t, unpacked.subtype, SDXI_DSC_OP_SUBTYPE_COPY);
+	KUNIT_EXPECT_EQ(t, unpacked.type, SDXI_DSC_OP_TYPE_DMAB);
+	KUNIT_EXPECT_EQ(t, unpacked.csb_ptr, 0);
+	KUNIT_EXPECT_EQ(t, unpacked.np, 1);
+}
+
+static void intr(struct kunit *t)
+{
+	struct sdxi_desc_unpacked unpacked;
+	struct sdxi_intr intr = {
+		.akey = 1234,
+	};
+	struct sdxi_desc desc;
+
+	desc_poison(&desc);
+	KUNIT_EXPECT_EQ(t, 0, sdxi_encode_intr(&desc, &intr));
+	KUNIT_EXPECT_EQ(t, 1234, le16_to_cpu(desc.intr.akey));
+
+	sdxi_desc_unpack(&unpacked, &desc);
+	KUNIT_EXPECT_EQ(t, unpacked.vl, 1);
+	KUNIT_EXPECT_EQ(t, unpacked.ch, 0);
+	KUNIT_EXPECT_EQ(t, unpacked.subtype, SDXI_DSC_OP_SUBTYPE_INTR);
+	KUNIT_EXPECT_EQ(t, unpacked.type, SDXI_DSC_OP_TYPE_INTR);
+	KUNIT_EXPECT_EQ(t, unpacked.csb_ptr, 0);
+	KUNIT_EXPECT_EQ(t, unpacked.np, 1);
+}
+
+static void cxt_start(struct kunit *t)
+{
+	struct sdxi_cxt_start start = {
+		.range = sdxi_cxt_range(1, U16_MAX)
+	};
+	struct sdxi_desc desc = {};
+	struct sdxi_desc_unpacked unpacked;
+
+	KUNIT_EXPECT_EQ(t, 0, sdxi_encode_cxt_start(&desc, &start));
+
+	/* Check op-specific fields. */
+	KUNIT_EXPECT_EQ(t, 0, desc.cxt_start.vflags);
+	KUNIT_EXPECT_EQ(t, 0, desc.cxt_start.vf_num);
+	KUNIT_EXPECT_EQ(t, 1, desc.cxt_start.cxt_start);
+	KUNIT_EXPECT_EQ(t, U16_MAX, desc.cxt_start.cxt_end);
+	KUNIT_EXPECT_EQ(t, 0, desc.cxt_start.db_value);
+
+	/*
+	 * Check generic fields. Some flags have mandatory values
+	 * according to the operation type.
+	 */
+	sdxi_desc_unpack(&unpacked, &desc);
+	KUNIT_EXPECT_EQ(t, unpacked.vl, 1);
+	KUNIT_EXPECT_EQ(t, unpacked.se, 0);
+	KUNIT_EXPECT_EQ(t, unpacked.fe, 1);
+	KUNIT_EXPECT_EQ(t, unpacked.ch, 0);
+	KUNIT_EXPECT_EQ(t, unpacked.subtype, SDXI_DSC_OP_SUBTYPE_CXT_START_NM);
+	KUNIT_EXPECT_EQ(t, unpacked.type, SDXI_DSC_OP_TYPE_ADMIN);
+	KUNIT_EXPECT_EQ(t, unpacked.csb_ptr, 0);
+	KUNIT_EXPECT_EQ(t, unpacked.np, 1);
+}
+
+static void cxt_stop(struct kunit *t)
+{
+	struct sdxi_cxt_stop stop = {
+		.range = sdxi_cxt_range(1, U16_MAX)
+	};
+	struct sdxi_desc desc = {};
+	struct sdxi_desc_unpacked unpacked;
+
+	KUNIT_EXPECT_EQ(t, 0, sdxi_encode_cxt_stop(&desc, &stop));
+
+	/* Check op-specific fields */
+	KUNIT_EXPECT_EQ(t, 0, desc.cxt_stop.vflags);
+	KUNIT_EXPECT_EQ(t, 0, desc.cxt_stop.vf_num);
+	KUNIT_EXPECT_EQ(t, 1, desc.cxt_stop.cxt_start);
+	KUNIT_EXPECT_EQ(t, U16_MAX, desc.cxt_stop.cxt_end);
+
+	/*
+	 * Check generic fields. Some flags have mandatory values
+	 * according to the operation type.
+	 */
+	sdxi_desc_unpack(&unpacked, &desc);
+	KUNIT_EXPECT_EQ(t, unpacked.vl, 1);
+	KUNIT_EXPECT_EQ(t, unpacked.se, 0);
+	KUNIT_EXPECT_EQ(t, unpacked.fe, 1);
+	KUNIT_EXPECT_EQ(t, unpacked.ch, 0);
+	KUNIT_EXPECT_EQ(t, unpacked.subtype, SDXI_DSC_OP_SUBTYPE_CXT_STOP);
+	KUNIT_EXPECT_EQ(t, unpacked.type, SDXI_DSC_OP_TYPE_ADMIN);
+	KUNIT_EXPECT_EQ(t, unpacked.csb_ptr, 0);
+	KUNIT_EXPECT_EQ(t, unpacked.np, 1);
+}
+
+static struct kunit_case generic_desc_tcs[] = {
+	KUNIT_CASE(copy),
+	KUNIT_CASE(intr),
+	KUNIT_CASE(cxt_start),
+	KUNIT_CASE(cxt_stop),
+	{},
+};
+
+static int generic_desc_setup_device(struct kunit *t)
+{
+	struct device *dev = kunit_device_register(t, "sdxi-mock-device");
+
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(t, dev);
+	t->priv = dev;
+	return 0;
+}
+
+static struct kunit_suite generic_desc_ts = {
+	.name = "Generic SDXI descriptor encoding",
+	.test_cases = generic_desc_tcs,
+	.init = generic_desc_setup_device,
+};
+kunit_test_suite(generic_desc_ts);
+
+MODULE_DESCRIPTION("SDXI descriptor encoding tests");
+MODULE_AUTHOR("Nathan Lynch");
+MODULE_LICENSE("GPL");

-- 
2.39.5

Re: [PATCH RFC 03/13] dmaengine: sdxi: Add descriptor encoding and unit tests

[Jonathan Cameron]

On Fri, 05 Sep 2025 13:48:26 -0500
Nathan Lynch via B4 Relay <devnull+nathan.lynch.amd.com@kernel.org> wrote:

> From: Nathan Lynch <nathan.lynch@amd.com>
> 
> Add support for encoding several types of SDXI descriptors:
> 
> * Copy
> * Interrupt
> * Context start
> * Context stop
> 
> Each type of descriptor has a corresponding parameter struct which is
> an input to its encoder function. E.g. to encode a copy descriptor,
> the client initializes a struct sdxi_copy object with the source,
> destination, size, etc and passes that to sdxi_encode_copy().
> 
> Include unit tests that verify that encoded descriptors have the
> expected values and that fallible encode functions fail on invalid
> inputs.
> 
> Co-developed-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>

Hi,
A few comments inline. Mostly it is a case of personal taste
vs what I'm seeing as complexity that is needed.

Jonathan


> ---
>  drivers/dma/sdxi/.kunitconfig       |   4 +
>  drivers/dma/sdxi/descriptor.c       | 197 ++++++++++++++++++++++++++++++++++++
>  drivers/dma/sdxi/descriptor.h       | 107 ++++++++++++++++++++
>  drivers/dma/sdxi/descriptor_kunit.c | 181 +++++++++++++++++++++++++++++++++
>  4 files changed, 489 insertions(+)
> 
> diff --git a/drivers/dma/sdxi/.kunitconfig b/drivers/dma/sdxi/.kunitconfig
> new file mode 100644
> index 0000000000000000000000000000000000000000..a98cf19770f03bce82ef86d378d2a2e34da5154a
> --- /dev/null
> +++ b/drivers/dma/sdxi/.kunitconfig
> @@ -0,0 +1,4 @@
> +CONFIG_KUNIT=y
> +CONFIG_DMADEVICES=y
> +CONFIG_SDXI=y
> +CONFIG_SDXI_KUNIT_TEST=y
> diff --git a/drivers/dma/sdxi/descriptor.c b/drivers/dma/sdxi/descriptor.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..6ea5247bf8cdaac19131ca5326ba1640c0b557f8
> --- /dev/null
> +++ b/drivers/dma/sdxi/descriptor.c

> +enum {
> +	SDXI_PACKING_QUIRKS = QUIRK_LITTLE_ENDIAN | QUIRK_LSW32_IS_FIRST,
> +};
> +
> +#define sdxi_desc_field(_high, _low, _member) \
> +	PACKED_FIELD(_high, _low, struct sdxi_desc_unpacked, _member)
> +#define sdxi_desc_flag(_bit, _member) \
> +	sdxi_desc_field(_bit, _bit, _member)
> +
> +static const struct packed_field_u16 common_descriptor_fields[] = {
> +	sdxi_desc_flag(0, vl),
> +	sdxi_desc_flag(1, se),
> +	sdxi_desc_flag(2, fe),
> +	sdxi_desc_flag(3, ch),
> +	sdxi_desc_flag(4, csr),
> +	sdxi_desc_flag(5, rb),
> +	sdxi_desc_field(15, 8, subtype),
> +	sdxi_desc_field(26, 16, type),
> +	sdxi_desc_flag(448, np),
> +	sdxi_desc_field(511, 453, csb_ptr),

I'm not immediately seeing the advantage of dealing with unpacking in here
when patch 2 introduced a bunch of field defines that can be used directly
in the tests. 

> +};
> +
> +void sdxi_desc_unpack(struct sdxi_desc_unpacked *to,
> +		      const struct sdxi_desc *from)
> +{
> +	*to = (struct sdxi_desc_unpacked){};
> +	unpack_fields(from, sizeof(*from), to, common_descriptor_fields,
> +		      SDXI_PACKING_QUIRKS);
> +}
> +EXPORT_SYMBOL_IF_KUNIT(sdxi_desc_unpack);

> +int sdxi_encode_cxt_stop(struct sdxi_desc *desc,
> +			  const struct sdxi_cxt_stop *params)
> +{
> +	u16 cxt_start;
> +	u16 cxt_end;

I'd either combine like types, or assign at point of declaration to
cut down on a few lines of code.

> +	u64 csb_ptr;
> +	u32 opcode;
> +
> +	opcode = (FIELD_PREP(SDXI_DSC_VL, 1) |
> +		  FIELD_PREP(SDXI_DSC_FE, 1) |
> +		  FIELD_PREP(SDXI_DSC_SUBTYPE, SDXI_DSC_OP_SUBTYPE_CXT_STOP) |
> +		  FIELD_PREP(SDXI_DSC_TYPE, SDXI_DSC_OP_TYPE_ADMIN));
> +
> +	cxt_start = params->range.cxt_start;
> +	cxt_end = params->range.cxt_end;
> +
> +	csb_ptr = FIELD_PREP(SDXI_DSC_NP, 1);
> +
> +	desc_clear(desc);

Not particularly important, but I'd be tempted to combine these with

	*desc = (struct sdxi_desc) {
		.ctx_stop = {
			.opcode = cpu_to_le32(opcode),
			.cxt_start = cpu_to_le16(cxt_start),
			.cxt_end = cpu_to_le16(cxt_end),
			.csb_ptr = cpu_to_le64(csb_ptr),
		},
	};

To me that more clearly shows what is set and that the
rest is zeroed.

> +	desc->cxt_stop = (struct sdxi_dsc_cxt_stop) {
> +		.opcode = cpu_to_le32(opcode),
> +		.cxt_start = cpu_to_le16(cxt_start),
> +		.cxt_end = cpu_to_le16(cxt_end),
> +		.csb_ptr = cpu_to_le64(csb_ptr),
> +	};
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_IF_KUNIT(sdxi_encode_cxt_stop);
> diff --git a/drivers/dma/sdxi/descriptor.h b/drivers/dma/sdxi/descriptor.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..141463dfd56bd4a88b4b3c9d45b13cc8101e1961
> --- /dev/null
> +++ b/drivers/dma/sdxi/descriptor.h
> @@ -0,0 +1,107 @@

> +/*
> + * Fields common to all SDXI descriptors in "unpacked" form, for use
> + * with pack_fields() and unpack_fields().
> + */
> +struct sdxi_desc_unpacked {
> +	u64 csb_ptr;
> +	u16 type;
> +	u8 subtype;
> +	bool vl;
> +	bool se;
> +	bool fe;
> +	bool ch;
> +	bool csr;
> +	bool rb;
> +	bool np;
> +};
> +
> +void sdxi_desc_unpack(struct sdxi_desc_unpacked *to,
> +		      const struct sdxi_desc *from);
> +
> +#endif /* DMA_SDXI_DESCRIPTOR_H */
> diff --git a/drivers/dma/sdxi/descriptor_kunit.c b/drivers/dma/sdxi/descriptor_kunit.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..eb89d5a152cd789fb8cfa66b78bf30e583a1680d
> --- /dev/null
> +++ b/drivers/dma/sdxi/descriptor_kunit.c
> @@ -0,0 +1,181 @@

> +static void cxt_stop(struct kunit *t)
> +{
> +	struct sdxi_cxt_stop stop = {
> +		.range = sdxi_cxt_range(1, U16_MAX)
> +	};
> +	struct sdxi_desc desc = {};
> +	struct sdxi_desc_unpacked unpacked;
> +
> +	KUNIT_EXPECT_EQ(t, 0, sdxi_encode_cxt_stop(&desc, &stop));
> +
> +	/* Check op-specific fields */
> +	KUNIT_EXPECT_EQ(t, 0, desc.cxt_stop.vflags);
> +	KUNIT_EXPECT_EQ(t, 0, desc.cxt_stop.vf_num);
> +	KUNIT_EXPECT_EQ(t, 1, desc.cxt_stop.cxt_start);
> +	KUNIT_EXPECT_EQ(t, U16_MAX, desc.cxt_stop.cxt_end);
> +
> +	/*
> +	 * Check generic fields. Some flags have mandatory values
> +	 * according to the operation type.
> +	 */
> +	sdxi_desc_unpack(&unpacked, &desc);

Follow up on the comments on unpacking above, to me just pulling the
values directly is simpler to follow.

	KUNIT_EXPECT_EQ(t, 0, FIELD_GET(desc.generic.opcode, SDXI_DSC_VL));

or something along those lines.

> +	KUNIT_EXPECT_EQ(t, unpacked.vl, 1);
> +	KUNIT_EXPECT_EQ(t, unpacked.se, 0);
> +	KUNIT_EXPECT_EQ(t, unpacked.fe, 1);
> +	KUNIT_EXPECT_EQ(t, unpacked.ch, 0);
> +	KUNIT_EXPECT_EQ(t, unpacked.subtype, SDXI_DSC_OP_SUBTYPE_CXT_STOP);
> +	KUNIT_EXPECT_EQ(t, unpacked.type, SDXI_DSC_OP_TYPE_ADMIN);
> +	KUNIT_EXPECT_EQ(t, unpacked.csb_ptr, 0);
> +	KUNIT_EXPECT_EQ(t, unpacked.np, 1);
> +}
> +
> +static struct kunit_case generic_desc_tcs[] = {
> +	KUNIT_CASE(copy),
> +	KUNIT_CASE(intr),
> +	KUNIT_CASE(cxt_start),
> +	KUNIT_CASE(cxt_stop),
> +	{},

Trivial but I'd drop that comma as nothing can come after this.

> +};

Re: [PATCH RFC 03/13] dmaengine: sdxi: Add descriptor encoding and unit tests

[Nathan Lynch]

Jonathan Cameron <jonathan.cameron@huawei.com> writes:
> On Fri, 05 Sep 2025 13:48:26 -0500
> Nathan Lynch via B4 Relay <devnull+nathan.lynch.amd.com@kernel.org> wrote:
>> +++ b/drivers/dma/sdxi/descriptor.c
>
>> +enum {
>> +	SDXI_PACKING_QUIRKS = QUIRK_LITTLE_ENDIAN | QUIRK_LSW32_IS_FIRST,
>> +};
>> +
>> +#define sdxi_desc_field(_high, _low, _member) \
>> +	PACKED_FIELD(_high, _low, struct sdxi_desc_unpacked, _member)
>> +#define sdxi_desc_flag(_bit, _member) \
>> +	sdxi_desc_field(_bit, _bit, _member)
>> +
>> +static const struct packed_field_u16 common_descriptor_fields[] = {
>> +	sdxi_desc_flag(0, vl),
>> +	sdxi_desc_flag(1, se),
>> +	sdxi_desc_flag(2, fe),
>> +	sdxi_desc_flag(3, ch),
>> +	sdxi_desc_flag(4, csr),
>> +	sdxi_desc_flag(5, rb),
>> +	sdxi_desc_field(15, 8, subtype),
>> +	sdxi_desc_field(26, 16, type),
>> +	sdxi_desc_flag(448, np),
>> +	sdxi_desc_field(511, 453, csb_ptr),
>
> I'm not immediately seeing the advantage of dealing with unpacking in here
> when patch 2 introduced a bunch of field defines that can be used directly
> in the tests.

My idea is to use the bitfield macros (GENMASK etc) for the real code
that encodes descriptors while using the packing API in the tests for
those functions.

By limiting what's shared between the real code and the tests I get more
confidence in both. If both the driver code and the tests rely on the
bitfield macros, and then upon adding a new descriptor field I
mistranslate the bit numbering from the spec, that error is more likely
to propagate to the tests undetected than if the test code relies on a
separate mechanism for decoding descriptors.

I find the packing API quite convenient to use for the SDXI descriptor
tests since the spec defines the fields in terms of bit offsets that can
be directly copied to a packed_field_ array.


>> +};
>> +
>> +void sdxi_desc_unpack(struct sdxi_desc_unpacked *to,
>> +		      const struct sdxi_desc *from)
>> +{
>> +	*to = (struct sdxi_desc_unpacked){};
>> +	unpack_fields(from, sizeof(*from), to, common_descriptor_fields,
>> +		      SDXI_PACKING_QUIRKS);
>> +}
>> +EXPORT_SYMBOL_IF_KUNIT(sdxi_desc_unpack);
>
>> +int sdxi_encode_cxt_stop(struct sdxi_desc *desc,
>> +			  const struct sdxi_cxt_stop *params)
>> +{
>> +	u16 cxt_start;
>> +	u16 cxt_end;
>
> I'd either combine like types, or assign at point of declaration to
> cut down on a few lines of code.

OK.


>> +	u64 csb_ptr;
>> +	u32 opcode;
>> +
>> +	opcode = (FIELD_PREP(SDXI_DSC_VL, 1) |
>> +		  FIELD_PREP(SDXI_DSC_FE, 1) |
>> +		  FIELD_PREP(SDXI_DSC_SUBTYPE, SDXI_DSC_OP_SUBTYPE_CXT_STOP) |
>> +		  FIELD_PREP(SDXI_DSC_TYPE, SDXI_DSC_OP_TYPE_ADMIN));
>> +
>> +	cxt_start = params->range.cxt_start;
>> +	cxt_end = params->range.cxt_end;
>> +
>> +	csb_ptr = FIELD_PREP(SDXI_DSC_NP, 1);
>> +
>> +	desc_clear(desc);
>
> Not particularly important, but I'd be tempted to combine these with
>
> 	*desc = (struct sdxi_desc) {
> 		.ctx_stop = {
> 			.opcode = cpu_to_le32(opcode),
> 			.cxt_start = cpu_to_le16(cxt_start),
> 			.cxt_end = cpu_to_le16(cxt_end),
> 			.csb_ptr = cpu_to_le64(csb_ptr),
> 		},
> 	};
>
> To me that more clearly shows what is set and that the
> rest is zeroed.

Maybe I prefer your version too. Just mentioning in case it's not clear:
cxt_stop is a union member with the same size as the enclosing struct
sdxi_desc. Each member of struct sdxi_desc's interior anonymous union is
intended to completely overlay the entire object.

The reason for the preceding desc_clear() is that the designated
initializer construct does not necessarily zero padding bytes in the
object. Now, there *shouldn't* be any padding bytes in SDXI descriptors
as I've defined them, so I'm hoping the redundant stores are discarded
in the generated code. But I haven't checked this.

And it looks like I neglected to mark all the descriptor structs __packed,
oops.

I think I can add the __packed to struct sdxi_desc et al, use your
suggested initializer, and discard desc_clear().


>> +	desc->cxt_stop = (struct sdxi_dsc_cxt_stop) {
>> +		.opcode = cpu_to_le32(opcode),
>> +		.cxt_start = cpu_to_le16(cxt_start),
>> +		.cxt_end = cpu_to_le16(cxt_end),
>> +		.csb_ptr = cpu_to_le64(csb_ptr),
>> +	};
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_IF_KUNIT(sdxi_encode_cxt_stop);
>> diff --git a/drivers/dma/sdxi/descriptor.h b/drivers/dma/sdxi/descriptor.h
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..141463dfd56bd4a88b4b3c9d45b13cc8101e1961
>> --- /dev/null
>> +++ b/drivers/dma/sdxi/descriptor.h
>> @@ -0,0 +1,107 @@
>
>> +/*
>> + * Fields common to all SDXI descriptors in "unpacked" form, for use
>> + * with pack_fields() and unpack_fields().
>> + */
>> +struct sdxi_desc_unpacked {
>> +	u64 csb_ptr;
>> +	u16 type;
>> +	u8 subtype;
>> +	bool vl;
>> +	bool se;
>> +	bool fe;
>> +	bool ch;
>> +	bool csr;
>> +	bool rb;
>> +	bool np;
>> +};
>> +
>> +void sdxi_desc_unpack(struct sdxi_desc_unpacked *to,
>> +		      const struct sdxi_desc *from);
>> +
>> +#endif /* DMA_SDXI_DESCRIPTOR_H */
>> diff --git a/drivers/dma/sdxi/descriptor_kunit.c b/drivers/dma/sdxi/descriptor_kunit.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..eb89d5a152cd789fb8cfa66b78bf30e583a1680d
>> --- /dev/null
>> +++ b/drivers/dma/sdxi/descriptor_kunit.c
>> @@ -0,0 +1,181 @@
>
>> +static void cxt_stop(struct kunit *t)
>> +{
>> +	struct sdxi_cxt_stop stop = {
>> +		.range = sdxi_cxt_range(1, U16_MAX)
>> +	};
>> +	struct sdxi_desc desc = {};
>> +	struct sdxi_desc_unpacked unpacked;
>> +
>> +	KUNIT_EXPECT_EQ(t, 0, sdxi_encode_cxt_stop(&desc, &stop));
>> +
>> +	/* Check op-specific fields */
>> +	KUNIT_EXPECT_EQ(t, 0, desc.cxt_stop.vflags);
>> +	KUNIT_EXPECT_EQ(t, 0, desc.cxt_stop.vf_num);
>> +	KUNIT_EXPECT_EQ(t, 1, desc.cxt_stop.cxt_start);
>> +	KUNIT_EXPECT_EQ(t, U16_MAX, desc.cxt_stop.cxt_end);
>> +
>> +	/*
>> +	 * Check generic fields. Some flags have mandatory values
>> +	 * according to the operation type.
>> +	 */
>> +	sdxi_desc_unpack(&unpacked, &desc);
>
> Follow up on the comments on unpacking above, to me just pulling the
> values directly is simpler to follow.
>
> 	KUNIT_EXPECT_EQ(t, 0, FIELD_GET(desc.generic.opcode, SDXI_DSC_VL));
>
> or something along those lines.
>
>> +	KUNIT_EXPECT_EQ(t, unpacked.vl, 1);
>> +	KUNIT_EXPECT_EQ(t, unpacked.se, 0);
>> +	KUNIT_EXPECT_EQ(t, unpacked.fe, 1);
>> +	KUNIT_EXPECT_EQ(t, unpacked.ch, 0);
>> +	KUNIT_EXPECT_EQ(t, unpacked.subtype, SDXI_DSC_OP_SUBTYPE_CXT_STOP);
>> +	KUNIT_EXPECT_EQ(t, unpacked.type, SDXI_DSC_OP_TYPE_ADMIN);
>> +	KUNIT_EXPECT_EQ(t, unpacked.csb_ptr, 0);
>> +	KUNIT_EXPECT_EQ(t, unpacked.np, 1);
>> +}
>> +
>> +static struct kunit_case generic_desc_tcs[] = {
>> +	KUNIT_CASE(copy),
>> +	KUNIT_CASE(intr),
>> +	KUNIT_CASE(cxt_start),
>> +	KUNIT_CASE(cxt_stop),
>> +	{},
>
> Trivial but I'd drop that comma as nothing can come after this.

Sure.

Re: [PATCH RFC 03/13] dmaengine: sdxi: Add descriptor encoding and unit tests

[Jonathan Cameron]

On Mon, 15 Sep 2025 14:30:23 -0500
Nathan Lynch <nathan.lynch@amd.com> wrote:

+CC Kees given I refer to a prior discussion Kees helped out with
and this is a different related case.

> Jonathan Cameron <jonathan.cameron@huawei.com> writes:
> > On Fri, 05 Sep 2025 13:48:26 -0500
> > Nathan Lynch via B4 Relay <devnull+nathan.lynch.amd.com@kernel.org> wrote:  
> >> +++ b/drivers/dma/sdxi/descriptor.c  
> >  
> >> +enum {
> >> +	SDXI_PACKING_QUIRKS = QUIRK_LITTLE_ENDIAN | QUIRK_LSW32_IS_FIRST,
> >> +};
> >> +
> >> +#define sdxi_desc_field(_high, _low, _member) \
> >> +	PACKED_FIELD(_high, _low, struct sdxi_desc_unpacked, _member)
> >> +#define sdxi_desc_flag(_bit, _member) \
> >> +	sdxi_desc_field(_bit, _bit, _member)
> >> +
> >> +static const struct packed_field_u16 common_descriptor_fields[] = {
> >> +	sdxi_desc_flag(0, vl),
> >> +	sdxi_desc_flag(1, se),
> >> +	sdxi_desc_flag(2, fe),
> >> +	sdxi_desc_flag(3, ch),
> >> +	sdxi_desc_flag(4, csr),
> >> +	sdxi_desc_flag(5, rb),
> >> +	sdxi_desc_field(15, 8, subtype),
> >> +	sdxi_desc_field(26, 16, type),
> >> +	sdxi_desc_flag(448, np),
> >> +	sdxi_desc_field(511, 453, csb_ptr),  
> >
> > I'm not immediately seeing the advantage of dealing with unpacking in here
> > when patch 2 introduced a bunch of field defines that can be used directly
> > in the tests.  
> 
> My idea is to use the bitfield macros (GENMASK etc) for the real code
> that encodes descriptors while using the packing API in the tests for
> those functions.
> 
> By limiting what's shared between the real code and the tests I get more
> confidence in both. If both the driver code and the tests rely on the
> bitfield macros, and then upon adding a new descriptor field I
> mistranslate the bit numbering from the spec, that error is more likely
> to propagate to the tests undetected than if the test code relies on a
> separate mechanism for decoding descriptors.

That's a fair reason.  Perhaps add a comment just above the first
instance of this or top of file to express that?
> 
> I find the packing API quite convenient to use for the SDXI descriptor
> tests since the spec defines the fields in terms of bit offsets that can
> be directly copied to a packed_field_ array.
> 
> 
> >> +};

> >> +	u64 csb_ptr;
> >> +	u32 opcode;
> >> +
> >> +	opcode = (FIELD_PREP(SDXI_DSC_VL, 1) |
> >> +		  FIELD_PREP(SDXI_DSC_FE, 1) |
> >> +		  FIELD_PREP(SDXI_DSC_SUBTYPE, SDXI_DSC_OP_SUBTYPE_CXT_STOP) |
> >> +		  FIELD_PREP(SDXI_DSC_TYPE, SDXI_DSC_OP_TYPE_ADMIN));
> >> +
> >> +	cxt_start = params->range.cxt_start;
> >> +	cxt_end = params->range.cxt_end;
> >> +
> >> +	csb_ptr = FIELD_PREP(SDXI_DSC_NP, 1);
> >> +
> >> +	desc_clear(desc);  
> >
> > Not particularly important, but I'd be tempted to combine these with
> >
> > 	*desc = (struct sdxi_desc) {
> > 		.ctx_stop = {
> > 			.opcode = cpu_to_le32(opcode),
> > 			.cxt_start = cpu_to_le16(cxt_start),
> > 			.cxt_end = cpu_to_le16(cxt_end),
> > 			.csb_ptr = cpu_to_le64(csb_ptr),
> > 		},
> > 	};
> >
> > To me that more clearly shows what is set and that the
> > rest is zeroed.  
> 
> Maybe I prefer your version too. Just mentioning in case it's not clear:
> cxt_stop is a union member with the same size as the enclosing struct
> sdxi_desc. Each member of struct sdxi_desc's interior anonymous union is
> intended to completely overlay the entire object.
> 
> The reason for the preceding desc_clear() is that the designated
> initializer construct does not necessarily zero padding bytes in the
> object. Now, there *shouldn't* be any padding bytes in SDXI descriptors
> as I've defined them, so I'm hoping the redundant stores are discarded
> in the generated code. But I haven't checked this.

So, this one is 'fun' (and I can hopefully find the references)
The C spec has had some updates that might cover this though
I'm not sure and too lazy to figure it out today.  Anyhow,
that doesn't help anyway as we care about older compilers.

So we cheat and just check the compiler does fill them ;)

Via a reply Kees sent on a discussion of the somewhat related {}
https://lore.kernel.org/linux-iio/202505090942.48EBF01B@keescook/

https://elixir.bootlin.com/linux/v6.17-rc6/source/lib/tests/stackinit_kunit.c

I think the relevant one is __dynamic_all which is used with various hole sizes
and with both bare structures and unions.

+CC Kees who might have time to shout if I have this particular case wrong ;)



> 
> And it looks like I neglected to mark all the descriptor structs __packed,
> oops.
> 
> I think I can add the __packed to struct sdxi_desc et al, use your
> suggested initializer, and discard desc_clear().

That would indeed work.

> 
> 
> >> +	desc->cxt_stop = (struct sdxi_dsc_cxt_stop) {
> >> +		.opcode = cpu_to_le32(opcode),
> >> +		.cxt_start = cpu_to_le16(cxt_start),
> >> +		.cxt_end = cpu_to_le16(cxt_end),
> >> +		.csb_ptr = cpu_to_le64(csb_ptr),
> >> +	};

Re: [PATCH RFC 03/13] dmaengine: sdxi: Add descriptor encoding and unit tests

[Nathan Lynch]

Jonathan Cameron <jonathan.cameron@huawei.com> writes:
> On Mon, 15 Sep 2025 14:30:23 -0500
> Nathan Lynch <nathan.lynch@amd.com> wrote:
>
> +CC Kees given I refer to a prior discussion Kees helped out with
> and this is a different related case.
>
>> Jonathan Cameron <jonathan.cameron@huawei.com> writes:
>> > On Fri, 05 Sep 2025 13:48:26 -0500
>> > Nathan Lynch via B4 Relay <devnull+nathan.lynch.amd.com@kernel.org> wrote:  
>> >> +++ b/drivers/dma/sdxi/descriptor.c  
>> >  
>> >> +enum {
>> >> +	SDXI_PACKING_QUIRKS = QUIRK_LITTLE_ENDIAN | QUIRK_LSW32_IS_FIRST,
>> >> +};
>> >> +
>> >> +#define sdxi_desc_field(_high, _low, _member) \
>> >> +	PACKED_FIELD(_high, _low, struct sdxi_desc_unpacked, _member)
>> >> +#define sdxi_desc_flag(_bit, _member) \
>> >> +	sdxi_desc_field(_bit, _bit, _member)
>> >> +
>> >> +static const struct packed_field_u16 common_descriptor_fields[] = {
>> >> +	sdxi_desc_flag(0, vl),
>> >> +	sdxi_desc_flag(1, se),
>> >> +	sdxi_desc_flag(2, fe),
>> >> +	sdxi_desc_flag(3, ch),
>> >> +	sdxi_desc_flag(4, csr),
>> >> +	sdxi_desc_flag(5, rb),
>> >> +	sdxi_desc_field(15, 8, subtype),
>> >> +	sdxi_desc_field(26, 16, type),
>> >> +	sdxi_desc_flag(448, np),
>> >> +	sdxi_desc_field(511, 453, csb_ptr),  
>> >
>> > I'm not immediately seeing the advantage of dealing with unpacking in here
>> > when patch 2 introduced a bunch of field defines that can be used directly
>> > in the tests.  
>> 
>> My idea is to use the bitfield macros (GENMASK etc) for the real code
>> that encodes descriptors while using the packing API in the tests for
>> those functions.
>> 
>> By limiting what's shared between the real code and the tests I get more
>> confidence in both. If both the driver code and the tests rely on the
>> bitfield macros, and then upon adding a new descriptor field I
>> mistranslate the bit numbering from the spec, that error is more likely
>> to propagate to the tests undetected than if the test code relies on a
>> separate mechanism for decoding descriptors.
>
> That's a fair reason.  Perhaps add a comment just above the first
> instance of this or top of file to express that?

OK. Looks like sdxi_desc_unpack() and the related field description
structure could be moved to the test code too.


>> I find the packing API quite convenient to use for the SDXI descriptor
>> tests since the spec defines the fields in terms of bit offsets that can
>> be directly copied to a packed_field_ array.
>> 
>> 
>> >> +};
>
>> >> +	u64 csb_ptr;
>> >> +	u32 opcode;
>> >> +
>> >> +	opcode = (FIELD_PREP(SDXI_DSC_VL, 1) |
>> >> +		  FIELD_PREP(SDXI_DSC_FE, 1) |
>> >> +		  FIELD_PREP(SDXI_DSC_SUBTYPE, SDXI_DSC_OP_SUBTYPE_CXT_STOP) |
>> >> +		  FIELD_PREP(SDXI_DSC_TYPE, SDXI_DSC_OP_TYPE_ADMIN));
>> >> +
>> >> +	cxt_start = params->range.cxt_start;
>> >> +	cxt_end = params->range.cxt_end;
>> >> +
>> >> +	csb_ptr = FIELD_PREP(SDXI_DSC_NP, 1);
>> >> +
>> >> +	desc_clear(desc);  
>> >
>> > Not particularly important, but I'd be tempted to combine these with
>> >
>> > 	*desc = (struct sdxi_desc) {
>> > 		.ctx_stop = {
>> > 			.opcode = cpu_to_le32(opcode),
>> > 			.cxt_start = cpu_to_le16(cxt_start),
>> > 			.cxt_end = cpu_to_le16(cxt_end),
>> > 			.csb_ptr = cpu_to_le64(csb_ptr),
>> > 		},
>> > 	};
>> >
>> > To me that more clearly shows what is set and that the
>> > rest is zeroed.  
>> 
>> Maybe I prefer your version too. Just mentioning in case it's not clear:
>> cxt_stop is a union member with the same size as the enclosing struct
>> sdxi_desc. Each member of struct sdxi_desc's interior anonymous union is
>> intended to completely overlay the entire object.
>> 
>> The reason for the preceding desc_clear() is that the designated
>> initializer construct does not necessarily zero padding bytes in the
>> object. Now, there *shouldn't* be any padding bytes in SDXI descriptors
>> as I've defined them, so I'm hoping the redundant stores are discarded
>> in the generated code. But I haven't checked this.
>
> So, this one is 'fun' (and I can hopefully find the references)
> The C spec has had some updates that might cover this though
> I'm not sure and too lazy to figure it out today.  Anyhow,
> that doesn't help anyway as we care about older compilers.
>
> So we cheat and just check the compiler does fill them ;)
>
> Via a reply Kees sent on a discussion of the somewhat related {}
> https://lore.kernel.org/linux-iio/202505090942.48EBF01B@keescook/
>
> https://elixir.bootlin.com/linux/v6.17-rc6/source/lib/tests/stackinit_kunit.c
>
> I think the relevant one is __dynamic_all which is used with various hole sizes
> and with both bare structures and unions.
>
> +CC Kees who might have time to shout if I have this particular case
> wrong ;)

Thanks for the references, when making this decision I consulted:

https://gustedt.wordpress.com/2012/10/24/c11-defects-initialization-of-padding/

and

https://interrupt.memfault.com/blog/c-struct-padding-initialization

But we seem to agree that it's a moot point for this code if I make the
changes discussed below.


>> And it looks like I neglected to mark all the descriptor structs __packed,
>> oops.
>> 
>> I think I can add the __packed to struct sdxi_desc et al, use your
>> suggested initializer, and discard desc_clear().
>
> That would indeed work.
>
>> 
>> 
>> >> +	desc->cxt_stop = (struct sdxi_dsc_cxt_stop) {
>> >> +		.opcode = cpu_to_le32(opcode),
>> >> +		.cxt_start = cpu_to_le16(cxt_start),
>> >> +		.cxt_end = cpu_to_le16(cxt_end),
>> >> +		.csb_ptr = cpu_to_le64(csb_ptr),
>> >> +	};

[PATCH RFC 04/13] dmaengine: sdxi: Add MMIO register definitions

[Nathan Lynch via B4 Relay]

From: Nathan Lynch <nathan.lynch@amd.com>

Add offsets and bitmasks for:

* General control and status registers (MMIO_CTL0, MMIO_CTL2,
  MMIO_STS0)
* Capability registers (MMIO_CAP0, MMIO_CAP1)
* Context table pointer register (MMIO_CXT_L2)
* Error logging control and status registers (MMIO_ERR_CTL,
  MMIO_ERR_STS, MMIO_ERR_CFG, MMIO_ERR_WRT, MMIO_ERR_RD)

This is a useful subset of the MMIO registers and fields defined in
the spec. The driver currently does not use MMIO_VERSION,
MMIO_GRP_ENUM, or the mailbox registers.

Co-developed-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
---
 drivers/dma/sdxi/mmio.h | 92 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/drivers/dma/sdxi/mmio.h b/drivers/dma/sdxi/mmio.h
new file mode 100644
index 0000000000000000000000000000000000000000..36d174a1f8859055f7808d520de1ff193c49ae26
--- /dev/null
+++ b/drivers/dma/sdxi/mmio.h
@@ -0,0 +1,92 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+/*
+ * SDXI MMIO register offsets and layouts.
+ *
+ * Copyright (C) 2025 Advanced Micro Devices, Inc.
+ */
+
+#ifndef DMA_SDXI_MMIO_H
+#define DMA_SDXI_MMIO_H
+
+#include <linux/bits.h>
+#include <linux/compiler_attributes.h>
+#include <linux/compiler_types.h>
+#include <linux/types.h>
+
+/* Refer to "MMIO Control Registers". */
+enum sdxi_reg {
+	SDXI_MMIO_CTL0       = 0x00000,
+	SDXI_MMIO_CTL2       = 0x00010,
+	SDXI_MMIO_STS0       = 0x00100,
+	SDXI_MMIO_CAP0       = 0x00200,
+	SDXI_MMIO_CAP1       = 0x00208,
+	SDXI_MMIO_VERSION    = 0x00210,
+	SDXI_MMIO_CXT_L2     = 0x10000,
+	SDXI_MMIO_RKEY       = 0x10100,
+	SDXI_MMIO_ERR_CTL    = 0x20000,
+	SDXI_MMIO_ERR_STS    = 0x20008,
+	SDXI_MMIO_ERR_CFG    = 0x20010,
+	SDXI_MMIO_ERR_WRT    = 0x20020,
+	SDXI_MMIO_ERR_RD     = 0x20028,
+};
+
+enum {
+	/* SDXI_MMIO_CTL0 fields */
+	SDXI_MMIO_CTL0_FN_GSR         = GENMASK_ULL(1, 0),
+	SDXI_MMIO_CTL0_FN_PASID_VL    = BIT_ULL(2),
+	SDXI_MMIO_CTL0_FN_ERR_INTR_EN = BIT_ULL(4),
+	SDXI_MMIO_CTL0_FN_PASID       = GENMASK_ULL(27, 8),
+	SDXI_MMIO_CTL0_FN_GRP_ID      = GENMASK_ULL(63, 32),
+
+	/* SDXI_MMIO_CTL2 fields */
+	SDXI_MMIO_CTL2_MAX_BUFFER  = GENMASK_ULL(3, 0),
+	SDXI_MMIO_CTL2_MAX_AKEY_SZ = GENMASK_ULL(15, 12),
+	SDXI_MMIO_CTL2_MAX_CXT     = GENMASK_ULL(31, 16),
+	SDXI_MMIO_CTL2_OPB_000_AVL = GENMASK_ULL(63, 32),
+
+	/* SDXI_MMIO_STS0 bit definitions */
+	SDXI_MMIO_STS0_FN_GSV = GENMASK_ULL(2, 0),
+
+	/* SDXI_MMIO_CAP0 bit definitions */
+	SDXI_MMIO_CAP0_SFUNC = GENMASK_ULL(15, 0),
+	SDXI_MMIO_CAP0_DB_STRIDE = GENMASK_ULL(22, 20),
+	SDXI_MMIO_CAP0_MAX_DS_RING_SZ = GENMASK_ULL(28, 24),
+
+	/* SDXI_MMIO_CAP1 fields */
+	SDXI_MMIO_CAP1_MAX_BUFFER    = GENMASK_ULL(3, 0),
+	SDXI_MMIO_CAP1_RKEY_CAP      = BIT_ULL(4),
+	SDXI_MMIO_CAP1_RM            = BIT_ULL(5),
+	SDXI_MMIO_CAP1_MMIO64        = BIT_ULL(6),
+	SDXI_MMIO_CAP1_MAX_ERRLOG_SZ = GENMASK_ULL(11, 8),
+	SDXI_MMIO_CAP1_MAX_AKEY_SZ   = GENMASK_ULL(15, 12),
+	SDXI_MMIO_CAP1_MAX_CXT       = GENMASK_ULL(31, 16),
+	SDXI_MMIO_CAP1_OPB_000_CAP   = GENMASK_ULL(63, 32),
+
+	/* SDXI_MMIO_VERSION fields */
+	SDXI_MMIO_VERSION_MINOR = GENMASK_ULL(7, 0),
+	SDXI_MMIO_VERSION_MAJOR = GENMASK_ULL(23, 16),
+
+	/* SDXI_MMIO_CXT_L2 fields */
+	SDXI_MMIO_CXT_L2_PTR = GENMASK_ULL(63, 12),
+
+	/* SDXI_MMIO_ERR_CFG bit definitions */
+	SDXI_MMIO_ERR_CFG_PTR = GENMASK_ULL(63, 12),
+	SDXI_MMIO_ERR_CFG_SZ  = GENMASK_ULL(5, 1),
+	SDXI_MMIO_ERR_CFG_EN  = BIT_ULL(0),
+
+	/* SDXI_MMIO_RKEY bit definitions */
+	SDXI_MMIO_RKEY_PTR = GENMASK_ULL(63, 12),
+	SDXI_MMIO_RKEY_SZ = GENMASK_ULL(4, 1),
+	SDXI_MMIO_RKEY_EN = BIT_ULL(0),
+
+	/* SDXI_MMIO_ERR_CTL bit definitions */
+	SDXI_MMIO_ERR_CTL_EN = BIT_ULL(0),
+
+	/* SDXI_MMIO_ERR_STS bit definitions. */
+	SDXI_MMIO_ERR_STS_STS_BIT = BIT_ULL(0),
+	SDXI_MMIO_ERR_STS_OVF_BIT = BIT_ULL(1),
+	SDXI_MMIO_ERR_STS_ERR_BIT = BIT_ULL(3),
+};
+
+#endif  /* DMA_SDXI_MMIO_H */

-- 
2.39.5

[PATCH RFC 05/13] dmaengine: sdxi: Add software data structures

[Nathan Lynch via B4 Relay]

From: Nathan Lynch <nathan.lynch@amd.com>

Add the driver's central header sdxi.h, which brings in the major
software abstractions used throughout the driver -- mainly the SDXI
device or function (sdxi_dev) and context (sdxi_cxt).

Co-developed-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
---
 drivers/dma/sdxi/sdxi.h | 206 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 206 insertions(+)

diff --git a/drivers/dma/sdxi/sdxi.h b/drivers/dma/sdxi/sdxi.h
new file mode 100644
index 0000000000000000000000000000000000000000..13e02f0541e0d60412c99b0b75bd37155a531e1d
--- /dev/null
+++ b/drivers/dma/sdxi/sdxi.h
@@ -0,0 +1,206 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * SDXI device driver header
+ *
+ * Copyright (C) 2025 Advanced Micro Devices, Inc.
+ */
+
+#ifndef __SDXI_H
+#define __SDXI_H
+
+#include <linux/dev_printk.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmapool.h>
+#include <linux/dmaengine.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/types.h>
+
+#include "../virt-dma.h"
+#include "hw.h"
+#include "mmio.h"
+
+#define SDXI_DRV_NAME		"sdxi"
+
+#define ID_TO_L2_INDEX(id)	(((id) >> 9) & 0x1FF)
+#define ID_TO_L1_INDEX(id)	((id) & 0x7F)
+#define IS_VF_DEVICE(sdxi)	((sdxi)->is_vf)
+
+#define L2_TABLE_ENTRIES	(1 << 9)
+#define L1_TABLE_ENTRIES	(1 << 7)
+#define L2_TABLE_SIZE		4096
+#define L1_TABLE_SIZE		4096
+
+#define OP_TYPE_ERRLOG          0x7f7
+
+#define DESC_RING_BASE_PTR_SHIFT	6
+#define CXT_STATUS_PTR_SHIFT		4
+#define WRT_INDEX_PTR_SHIFT		3
+
+#define L1_CXT_CTRL_PTR_SHIFT		6
+#define L1_CXT_AKEY_PTR_SHIFT		12
+
+#define MAX_DMA_COPY_BYTES		(1ULL << 32)
+
+/* Submission Queue */
+struct sdxi_sq {
+	struct sdxi_cxt *cxt;		/* owner */
+
+	u32 ring_entries;
+	u32 ring_size;
+	struct sdxi_desc *desc_ring;
+	dma_addr_t ring_dma;
+
+	__le64 *write_index;
+	dma_addr_t write_index_dma;
+
+	struct sdxi_cxt_sts *cxt_sts;
+	dma_addr_t cxt_sts_dma;
+
+	/* NB: define doorbell here */
+};
+
+struct sdxi_tasklet_data {
+	struct sdxi_cmd *cmd;
+};
+
+struct sdxi_cmd {
+	struct work_struct work;
+	struct sdxi_cxt *cxt;
+	struct sdxi_cst_blk *cst_blk;
+	dma_addr_t cst_blk_dma;
+	int ret;
+	size_t len;
+	u64 src_addr;
+	u64 dst_addr;
+	/* completion callback support */
+	void (*sdxi_cmd_callback)(void *data, int err);
+	void *data;
+};
+
+struct sdxi_dma_chan {
+	struct virt_dma_chan vc;
+	struct sdxi_cxt *cxt;
+};
+
+/*
+ * The size of the AKey table is flexible, from 4KB to 1MB. Always use
+ * the minimum size for now.
+ */
+struct sdxi_akey_table {
+	struct sdxi_akey_ent entry[SZ_4K / sizeof(struct sdxi_akey_ent)];
+};
+
+/* For encoding the akey table size in CXT_L1_ENT's akey_sz. */
+static inline u8 akey_table_order(const struct sdxi_akey_table *tbl)
+{
+	static_assert(sizeof(struct sdxi_akey_table) == SZ_4K);
+	return 0;
+}
+
+/* Context */
+struct sdxi_cxt {
+	struct sdxi_dev *sdxi;	/* owner */
+	unsigned int id;
+
+	resource_size_t db_base;	/* doorbell MMIO base addr */
+	__le64 __iomem *db;		/* doorbell virt addr */
+
+	struct sdxi_cxt_ctl *cxt_ctl;
+	dma_addr_t cxt_ctl_dma;
+
+	struct sdxi_akey_table *akey_table;
+	dma_addr_t akey_table_dma;
+
+	struct sdxi_sq *sq;
+
+	/* NB: might need to move to sdxi_device? */
+	struct sdxi_dma_chan sdxi_dma_chan;
+
+	struct sdxi_process *process;	/* process reprsentation */
+};
+
+/**
+ * struct sdxi_dev_ops - Bus-specific methods for SDXI devices.
+ *
+ * @irq_init: Allocate MSIs.
+ * @irq_exit: Release MSIs.
+ */
+struct sdxi_dev_ops {
+	int (*irq_init)(struct sdxi_dev *sdxi);
+	void (*irq_exit)(struct sdxi_dev *sdxi);
+};
+
+struct sdxi_dev {
+	struct device *dev;
+	resource_size_t ctrl_regs_bar;	/* ctrl registers base (BAR0) */
+	resource_size_t dbs_bar;	/* doorbells base (BAR2) */
+	void __iomem *ctrl_regs;	/* virt addr of ctrl registers */
+	void __iomem *dbs;		/* virt addr of doorbells */
+
+	/* hardware capabilities (from cap0 & cap1) */
+	u16 sfunc;			/* function's requester id */
+	u32 db_stride;			/* doorbell stride in bytes */
+	u64 max_ring_entries;		/* max # of ring entries supported */
+
+	u32 max_akeys;			/* max akey # supported */
+	u32 max_cxts;			/* max contexts # supported */
+	u32 op_grp_cap;			/* supported operatation group cap */
+
+	/* context management */
+	struct mutex cxt_lock;		/* context protection */
+	int cxt_count;
+	struct sdxi_cxt_l2_table *l2_table;
+	dma_addr_t l2_dma;
+	/* list of context l1 tables, on-demand, access with [l2_idx] */
+	struct sdxi_cxt_l1_table *l1_table_array[L2_TABLE_ENTRIES];
+	/* all contexts, on-demand, access with [l2_idx][l1_idx] */
+	struct sdxi_cxt **cxt_array[L2_TABLE_ENTRIES];
+
+	struct dma_pool *write_index_pool;
+	struct dma_pool *cxt_sts_pool;
+	struct dma_pool *cxt_ctl_pool;
+
+	/* error log */
+	int error_irq;
+	struct sdxi_errlog_hd_ent *err_log;
+	dma_addr_t err_log_dma;
+
+	/* DMA engine */
+	struct dma_device dma_dev;
+	struct sdxi_dma_chan *sdxi_dma_chan;
+	struct sdxi_tasklet_data tdata;
+
+	/* special contexts */
+	struct sdxi_cxt *admin_cxt;	/* admin context */
+	struct sdxi_cxt *dma_cxt;	/* DMA engine context */
+
+	const struct sdxi_dev_ops *dev_ops;
+};
+
+static inline struct device *sdxi_to_dev(const struct sdxi_dev *sdxi)
+{
+	return sdxi->dev;
+}
+
+#define sdxi_dbg(s, fmt, ...) dev_dbg(sdxi_to_dev(s), fmt, ## __VA_ARGS__)
+#define sdxi_info(s, fmt, ...) dev_info(sdxi_to_dev(s), fmt, ## __VA_ARGS__)
+#define sdxi_err(s, fmt, ...) dev_err(sdxi_to_dev(s), fmt, ## __VA_ARGS__)
+
+/* Device Control */
+int sdxi_device_init(struct sdxi_dev *sdxi, const struct sdxi_dev_ops *ops);
+void sdxi_device_exit(struct sdxi_dev *sdxi);
+
+static inline u64 sdxi_read64(const struct sdxi_dev *sdxi, enum sdxi_reg reg)
+{
+	return ioread64(sdxi->ctrl_regs + reg);
+}
+
+static inline void sdxi_write64(struct sdxi_dev *sdxi, enum sdxi_reg reg, u64 val)
+{
+	iowrite64(val, sdxi->ctrl_regs + reg);
+}
+
+#endif /* __SDXI_H */

-- 
2.39.5

Re: [PATCH RFC 05/13] dmaengine: sdxi: Add software data structures

[Jonathan Cameron]

On Fri, 05 Sep 2025 13:48:28 -0500
Nathan Lynch via B4 Relay <devnull+nathan.lynch.amd.com@kernel.org> wrote:

> From: Nathan Lynch <nathan.lynch@amd.com>
> 
> Add the driver's central header sdxi.h, which brings in the major
> software abstractions used throughout the driver -- mainly the SDXI
> device or function (sdxi_dev) and context (sdxi_cxt).
> 
> Co-developed-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>

I'm not personally a fan of 'header' patches.  It's find of reasonable if it's
just stuff of the datasheet, but once we get function definitions, we should
have the function implementations in the same patch.

> ---
>  drivers/dma/sdxi/sdxi.h | 206 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 206 insertions(+)
> 
> diff --git a/drivers/dma/sdxi/sdxi.h b/drivers/dma/sdxi/sdxi.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..13e02f0541e0d60412c99b0b75bd37155a531e1d
> --- /dev/null
> +++ b/drivers/dma/sdxi/sdxi.h
> @@ -0,0 +1,206 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * SDXI device driver header
> + *
> + * Copyright (C) 2025 Advanced Micro Devices, Inc.
> + */
> +
> +#ifndef __SDXI_H
> +#define __SDXI_H
> +
> +#include <linux/dev_printk.h>
> +#include <linux/device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmapool.h>

Some of these could I think be removed in favor of one or two forwards
definitions.  In general good to keep to minimal includes following principles of
include what you use din each file.

> +#include <linux/dmaengine.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/types.h>



> +/* Device Control */

Superficially these don't seem have anything to do with controlling
the device. So this comment is confusing to me rather than helpful.

> +int sdxi_device_init(struct sdxi_dev *sdxi, const struct sdxi_dev_ops *ops);
> +void sdxi_device_exit(struct sdxi_dev *sdxi);

Bring these in with the code, not in an earlier patch.
Ideally set things up so the code is build able after each patch.

Re: [PATCH RFC 05/13] dmaengine: sdxi: Add software data structures

[Nathan Lynch]

Jonathan Cameron <jonathan.cameron@huawei.com> writes:
> On Fri, 05 Sep 2025 13:48:28 -0500
> Nathan Lynch via B4 Relay <devnull+nathan.lynch.amd.com@kernel.org> wrote:
>
>> From: Nathan Lynch <nathan.lynch@amd.com>
>> 
>> Add the driver's central header sdxi.h, which brings in the major
>> software abstractions used throughout the driver -- mainly the SDXI
>> device or function (sdxi_dev) and context (sdxi_cxt).
>> 
>> Co-developed-by: Wei Huang <wei.huang2@amd.com>
>> Signed-off-by: Wei Huang <wei.huang2@amd.com>
>> Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
>
> I'm not personally a fan of 'header' patches.  It's find of reasonable if it's
> just stuff of the datasheet, but once we get function definitions, we should
> have the function implementations in the same patch.
>
>> ---
>>  drivers/dma/sdxi/sdxi.h | 206 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 206 insertions(+)
>> 
>> diff --git a/drivers/dma/sdxi/sdxi.h b/drivers/dma/sdxi/sdxi.h
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..13e02f0541e0d60412c99b0b75bd37155a531e1d
>> --- /dev/null
>> +++ b/drivers/dma/sdxi/sdxi.h
>> @@ -0,0 +1,206 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * SDXI device driver header
>> + *
>> + * Copyright (C) 2025 Advanced Micro Devices, Inc.
>> + */
>> +
>> +#ifndef __SDXI_H
>> +#define __SDXI_H
>> +
>> +#include <linux/dev_printk.h>
>> +#include <linux/device.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/dmapool.h>
>
> Some of these could I think be removed in favor of one or two forwards
> definitions.  In general good to keep to minimal includes following principles of
> include what you use din each file.
>
>> +#include <linux/dmaengine.h>
>> +#include <linux/io-64-nonatomic-lo-hi.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/types.h>
>
>
>
>> +/* Device Control */
>
> Superficially these don't seem have anything to do with controlling
> the device. So this comment is confusing to me rather than helpful.
>
>> +int sdxi_device_init(struct sdxi_dev *sdxi, const struct sdxi_dev_ops *ops);
>> +void sdxi_device_exit(struct sdxi_dev *sdxi);
>
> Bring these in with the code, not in an earlier patch.
> Ideally set things up so the code is build able after each patch.

Agreed on all points here and I'll modify the series accordingly,
thanks.

Re: [PATCH RFC 05/13] dmaengine: sdxi: Add software data structures

[Markus Elfring]

…
> +++ b/drivers/dma/sdxi/sdxi.h
> @@ -0,0 +1,206 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * SDXI device driver header
> + *
> + * Copyright (C) 2025 Advanced Micro Devices, Inc.
> + */
> +
> +#ifndef __SDXI_H
> +#define __SDXI_H
…

Do you find the usage of leading underscores still appropriate for such an identifier?
https://wiki.sei.cmu.edu/confluence/display/c/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier
https://en.wikipedia.org/wiki/Include_guard#Difficulties

Regards,
Markus

[PATCH RFC 06/13] dmaengine: sdxi: Add error reporting support

[Nathan Lynch via B4 Relay]

From: Nathan Lynch <nathan.lynch@amd.com>

SDXI implementations provide software with detailed information about
error conditions using a per-device ring buffer in system memory. When
an error condition is signaled via interrupt, the driver retrieves any
pending error log entries and reports them to the kernel log.

Co-developed-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
---
 drivers/dma/sdxi/error.c | 340 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/dma/sdxi/error.h |  16 +++
 2 files changed, 356 insertions(+)

diff --git a/drivers/dma/sdxi/error.c b/drivers/dma/sdxi/error.c
new file mode 100644
index 0000000000000000000000000000000000000000..c5e33f5989250352f6b081a3049b3b1f972c85a6
--- /dev/null
+++ b/drivers/dma/sdxi/error.c
@@ -0,0 +1,340 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * SDXI error reporting.
+ *
+ * Copyright (C) 2025 Advanced Micro Devices, Inc.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/irqreturn.h>
+#include <linux/packing.h>
+#include <linux/types.h>
+
+#include "error.h"
+#include "mmio.h"
+#include "sdxi.h"
+
+/*
+ * The error log ring buffer size is configurable, but for now we fix
+ * it to 64 entries (which is the spec minimum).
+ */
+#define ERROR_LOG_ENTRIES 64
+#define ERROR_LOG_SZ (ERROR_LOG_ENTRIES * sizeof(struct sdxi_errlog_hd_ent))
+
+/* The "unpacked" counterpart to ERRLOG_HD_ENT. */
+struct errlog_entry {
+	u64 dsc_index;
+	u16 cxt_num;
+	u16 err_class;
+	u16 type;
+	u8 step;
+	u8 buf;
+	u8 sub_step;
+	u8 re;
+	bool vl;
+	bool cv;
+	bool div;
+	bool bv;
+};
+
+#define ERRLOG_ENTRY_FIELD(hi_, lo_, name_)				\
+	PACKED_FIELD(hi_, lo_, struct errlog_entry, name_)
+#define ERRLOG_ENTRY_FLAG(nr_, name_) \
+	ERRLOG_ENTRY_FIELD(nr_, nr_, name_)
+
+/* Refer to "Error Log Header Entry (ERRLOG_HD_ENT)" */
+static const struct packed_field_u16 errlog_hd_ent_fields[] = {
+	ERRLOG_ENTRY_FLAG(0, vl),
+	ERRLOG_ENTRY_FIELD(13, 8, step),
+	ERRLOG_ENTRY_FIELD(26, 16, type),
+	ERRLOG_ENTRY_FLAG(32, cv),
+	ERRLOG_ENTRY_FLAG(33, div),
+	ERRLOG_ENTRY_FLAG(34, bv),
+	ERRLOG_ENTRY_FIELD(38, 36, buf),
+	ERRLOG_ENTRY_FIELD(43, 40, sub_step),
+	ERRLOG_ENTRY_FIELD(46, 44, re),
+	ERRLOG_ENTRY_FIELD(63, 48, cxt_num),
+	ERRLOG_ENTRY_FIELD(127, 64, dsc_index),
+	ERRLOG_ENTRY_FIELD(367, 352, err_class),
+};
+
+enum {
+	SDXI_PACKING_QUIRKS = QUIRK_LITTLE_ENDIAN | QUIRK_LSW32_IS_FIRST,
+};
+
+/*
+ * Refer to "(Flagged) Processing Step" and
+ * "Error Log Header Entry (ERRLOG_HD_ENT)", subfield "step"
+ */
+enum errv_step {
+	ERRV_INT         = 1,
+	ERRV_CXT_L2      = 2,
+	ERRV_CXT_L1      = 3,
+	ERRV_CXT_CTL     = 4,
+	ERRV_CXT_STS     = 5,
+	ERRV_WRT_IDX     = 6,
+	ERRV_DSC_GEN     = 7,
+	ERRV_DSC_CSB     = 8,
+	ERRV_ATOMIC      = 9,
+	ERRV_DSC_BUF     = 10,
+	ERRV_DSC_AKEY    = 11,
+	ERRV_FN_RKEY     = 12,
+};
+
+static const char *const processing_steps[] = {
+	[ERRV_INT]        = "Internal Error",
+	[ERRV_CXT_L2]     = "Context Level 2 Table Entry - Translate, Read, Validate",
+	[ERRV_CXT_L1]     = "Context Level 1 Table Entry - Translate, Read, Validate",
+	[ERRV_CXT_CTL]    = "Context Control - Translate, Read, Validate",
+	[ERRV_CXT_STS]    = "Context Status - Translate, Access, Validate",
+	[ERRV_WRT_IDX]    = "Write_Index - Translate, Read, Validate",
+	[ERRV_DSC_GEN]    = "Descriptor Entry - Translate, Access, Validate",
+	[ERRV_DSC_CSB]    = "Descriptor CST_BLK - Translate, Access, Validate",
+	[ERRV_ATOMIC]     = "Atomic Return Data - Translate, Access",
+	[ERRV_DSC_BUF]    = "Descriptor: Data Buffer - Translate, Access",
+	[ERRV_DSC_AKEY]   = "Descriptor AKey Lookup - Translate, Access, Validate",
+	[ERRV_FN_RKEY]    = "Function RKey Lookup - Translate, Read, Validate",
+};
+
+static const char *step_str(enum errv_step step)
+{
+	const char *str = "reserved";
+
+	switch (step) {
+	case ERRV_INT ... ERRV_FN_RKEY:
+		str = processing_steps[step];
+		break;
+	}
+
+	return str;
+}
+
+/* Refer to "Error Log Header Entry (ERRLOG_HD_ENT)", subfield "sub_step" */
+enum errv_sub_step {
+	SUB_STEP_OTHER    = 0,
+	SUB_STEP_ATF      = 1,
+	SUB_STEP_DAF      = 2,
+	SUB_STEP_DVF      = 3,
+};
+
+static const char * const processing_sub_steps[] = {
+	[SUB_STEP_OTHER]    = "Other/unknown",
+	[SUB_STEP_ATF]      = "Address Translation Failure",
+	[SUB_STEP_DAF]      = "Data Access Failure",
+	[SUB_STEP_DVF]      = "Data Validation Failure",
+};
+
+static const char *sub_step_str(enum errv_sub_step sub_step)
+{
+	const char *str = "reserved";
+
+	switch (sub_step) {
+	case SUB_STEP_OTHER ... SUB_STEP_DVF:
+		str = processing_sub_steps[sub_step];
+		break;
+	}
+
+	return str;
+}
+
+/* Refer to "Error Log Header Entry (ERRLOG_HD_ENT)", subfield "re" */
+enum fn_reaction {
+	FN_REACT_INFORM      = 0,
+	FN_REACT_CXT_STOP    = 1,
+	FN_REACT_FN_STOP     = 2,
+};
+
+static const char * const fn_reactions[] = {
+	[FN_REACT_INFORM]      = "Informative, nothing stopped",
+	[FN_REACT_CXT_STOP]    = "Context stopped",
+	[FN_REACT_FN_STOP]     = "Function stopped",
+};
+
+static const char *reaction_str(enum fn_reaction reaction)
+{
+	const char *str = "reserved";
+
+	switch (reaction) {
+	case FN_REACT_INFORM ... FN_REACT_FN_STOP:
+		str = fn_reactions[reaction];
+		break;
+	}
+
+	return str;
+}
+
+static void sdxi_print_err(struct sdxi_dev *sdxi, u64 err_rd)
+{
+	struct errlog_entry ent;
+	size_t index;
+
+	index = err_rd % ERROR_LOG_ENTRIES;
+
+	unpack_fields(&sdxi->err_log[index], sizeof(sdxi->err_log[0]),
+		      &ent, errlog_hd_ent_fields, SDXI_PACKING_QUIRKS);
+
+	if (!ent.vl) {
+		dev_err_ratelimited(sdxi_to_dev(sdxi),
+				    "Ignoring error log entry with vl=0\n");
+		return;
+	}
+
+	if (ent.type != OP_TYPE_ERRLOG) {
+		dev_err_ratelimited(sdxi_to_dev(sdxi),
+				    "Ignoring error log entry with type=%#x\n",
+				    ent.type);
+		return;
+	}
+
+	sdxi_err(sdxi, "error log entry[%zu], MMIO_ERR_RD=%#llx:\n",
+		 index, err_rd);
+	sdxi_err(sdxi, "  re: %#x (%s)\n", ent.re, reaction_str(ent.re));
+	sdxi_err(sdxi, "  step: %#x (%s)\n", ent.step, step_str(ent.step));
+	sdxi_err(sdxi, "  sub_step: %#x (%s)\n",
+		 ent.sub_step, sub_step_str(ent.sub_step));
+	sdxi_err(sdxi, "  cv: %u div: %u bv: %u\n", ent.cv, ent.div, ent.bv);
+	if (ent.bv)
+		sdxi_err(sdxi, "  buf: %u\n", ent.buf);
+	if (ent.cv)
+		sdxi_err(sdxi, "  cxt_num: %#x\n", ent.cxt_num);
+	if (ent.div)
+		sdxi_err(sdxi, "  dsc_index: %#llx\n", ent.dsc_index);
+	sdxi_err(sdxi, "  err_class: %#x\n", ent.err_class);
+}
+
+/* Refer to "Error Log Processing by Software" */
+static irqreturn_t sdxi_irq_thread(int irq, void *data)
+{
+	struct sdxi_dev *sdxi = data;
+	u64 write_index;
+	u64 read_index;
+	u64 err_sts;
+
+	/* 1. Check MMIO_ERR_STS and perform any required remediation. */
+	err_sts = sdxi_read64(sdxi, SDXI_MMIO_ERR_STS);
+	if (!(err_sts & SDXI_MMIO_ERR_STS_STS_BIT))
+		return IRQ_HANDLED;
+
+	if (err_sts & SDXI_MMIO_ERR_STS_ERR_BIT) {
+		/*
+		 * Assume this isn't recoverable; e.g. the error log
+		 * isn't configured correctly. Don't clear
+		 * SDXI_MMIO_ERR_STS before returning.
+		 */
+		sdxi_err(sdxi, "attempted but failed to log errors\n");
+		sdxi_err(sdxi, "error log not functional\n");
+		return IRQ_HANDLED;
+	}
+
+	if (err_sts & SDXI_MMIO_ERR_STS_OVF_BIT)
+		sdxi_err(sdxi, "error log overflow, some entries lost\n");
+
+	/* 2. If MMIO_ERR_STS.sts is 1, then compute read_index. */
+	read_index = sdxi_read64(sdxi, SDXI_MMIO_ERR_RD);
+
+	/* 3. Clear MMIO_ERR_STS. The flags in this register are RW1C. */
+	sdxi_write64(sdxi, SDXI_MMIO_ERR_STS,
+		     SDXI_MMIO_ERR_STS_STS_BIT |
+		     SDXI_MMIO_ERR_STS_OVF_BIT |
+		     SDXI_MMIO_ERR_STS_ERR_BIT);
+
+	/* 4. Compute write_index. */
+	write_index = sdxi_read64(sdxi, SDXI_MMIO_ERR_WRT);
+
+	/* 5. If the indexes are equal then exit. */
+	if (read_index == write_index)
+		return IRQ_HANDLED;
+
+	/* 6. While read_index < write_index... */
+	while (read_index < write_index) {
+
+		/*
+		 * 7. and 8. Compute the real ring buffer index from
+		 * read_index and process the entry.
+		 */
+		sdxi_print_err(sdxi, read_index);
+
+		/* 9. Advance read_index. */
+		++read_index;
+
+		/* 10. Return to step 6. */
+	}
+
+	/* 11. Write read_index to MMIO_ERR_RD. */
+	sdxi_write64(sdxi, SDXI_MMIO_ERR_RD, read_index);
+
+	return IRQ_HANDLED;
+}
+
+/* Refer to "Error Log Initialization" */
+int sdxi_error_init(struct sdxi_dev *sdxi)
+{
+	u64 reg;
+	int err;
+
+	/* 1. Clear MMIO_ERR_CFG. Error interrupts are inhibited until step 6. */
+	sdxi_write64(sdxi, SDXI_MMIO_ERR_CFG, 0);
+
+	/* 2. Clear MMIO_ERR_STS. The flags in this register are RW1C. */
+	reg = FIELD_PREP(SDXI_MMIO_ERR_STS_STS_BIT, 1) |
+	      FIELD_PREP(SDXI_MMIO_ERR_STS_OVF_BIT, 1) |
+	      FIELD_PREP(SDXI_MMIO_ERR_STS_ERR_BIT, 1);
+	sdxi_write64(sdxi, SDXI_MMIO_ERR_STS, reg);
+
+	/* 3. Allocate memory for the error log ring buffer, initialize to zero. */
+	sdxi->err_log = dma_alloc_coherent(sdxi_to_dev(sdxi), ERROR_LOG_SZ,
+					   &sdxi->err_log_dma, GFP_KERNEL);
+	if (!sdxi->err_log)
+		return -ENOMEM;
+
+	/*
+	 * 4. Set MMIO_ERR_CTL.intr_en to 1 if interrupts on
+	 * context-level errors are desired.
+	 */
+	reg = sdxi_read64(sdxi, SDXI_MMIO_ERR_CTL);
+	FIELD_MODIFY(SDXI_MMIO_ERR_CTL_EN, &reg, 1);
+	sdxi_write64(sdxi, SDXI_MMIO_ERR_CTL, reg);
+
+	/*
+	 * The spec is not explicit about when to do this, but this
+	 * seems like the right time: enable interrupt on
+	 * function-level transition to error state.
+	 */
+	reg = sdxi_read64(sdxi, SDXI_MMIO_CTL0);
+	FIELD_MODIFY(SDXI_MMIO_CTL0_FN_ERR_INTR_EN, &reg, 1);
+	sdxi_write64(sdxi, SDXI_MMIO_CTL0, reg);
+
+	/* 5. Clear MMIO_ERR_WRT and MMIO_ERR_RD. */
+	sdxi_write64(sdxi, SDXI_MMIO_ERR_WRT, 0);
+	sdxi_write64(sdxi, SDXI_MMIO_ERR_RD, 0);
+
+	/*
+	 * Error interrupts can be generated once MMIO_ERR_CFG.en is
+	 * set in step 6, so set up the handler now.
+	 */
+	err = request_threaded_irq(sdxi->error_irq, NULL, sdxi_irq_thread,
+				   IRQF_TRIGGER_NONE, "SDXI error", sdxi);
+	if (err)
+		goto free_errlog;
+
+	/* 6. Program MMIO_ERR_CFG. */
+	reg = FIELD_PREP(SDXI_MMIO_ERR_CFG_PTR, sdxi->err_log_dma >> 12) |
+	      FIELD_PREP(SDXI_MMIO_ERR_CFG_SZ, ERROR_LOG_ENTRIES >> 6) |
+	      FIELD_PREP(SDXI_MMIO_ERR_CFG_EN, 1);
+	sdxi_write64(sdxi, SDXI_MMIO_ERR_CFG, reg);
+
+	return 0;
+
+free_errlog:
+	dma_free_coherent(sdxi_to_dev(sdxi), ERROR_LOG_SZ,
+			  sdxi->err_log, sdxi->err_log_dma);
+	return err;
+}
+
+void sdxi_error_exit(struct sdxi_dev *sdxi)
+{
+	sdxi_write64(sdxi, SDXI_MMIO_ERR_CFG, 0);
+	free_irq(sdxi->error_irq, sdxi);
+	dma_free_coherent(sdxi_to_dev(sdxi), ERROR_LOG_SZ,
+			  sdxi->err_log, sdxi->err_log_dma);
+}
diff --git a/drivers/dma/sdxi/error.h b/drivers/dma/sdxi/error.h
new file mode 100644
index 0000000000000000000000000000000000000000..50019d9811184464227ae13baa509101a2a3aacc
--- /dev/null
+++ b/drivers/dma/sdxi/error.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * SDXI error handling entry points.
+ *
+ * Copyright (C) 2025 Advanced Micro Devices, Inc.
+ */
+
+#ifndef DMA_SDXI_ERROR_H
+#define DMA_SDXI_ERROR_H
+
+struct sdxi_dev;
+
+int sdxi_error_init(struct sdxi_dev *sdxi);
+void sdxi_error_exit(struct sdxi_dev *sdxi);
+
+#endif  /* DMA_SDXI_ERROR_H */

-- 
2.39.5

Re: [PATCH RFC 06/13] dmaengine: sdxi: Add error reporting support

[Jonathan Cameron]

On Fri, 05 Sep 2025 13:48:29 -0500
Nathan Lynch via B4 Relay <devnull+nathan.lynch.amd.com@kernel.org> wrote:

> From: Nathan Lynch <nathan.lynch@amd.com>
> 
> SDXI implementations provide software with detailed information about
> error conditions using a per-device ring buffer in system memory. When
> an error condition is signaled via interrupt, the driver retrieves any
> pending error log entries and reports them to the kernel log.
> 
> Co-developed-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
Hi,
A few more comments inline. Kind of similar stuff around
having both register definitions for unpacking and the structure
definitions in patch 2.

Thanks,

Jonathan
> ---
>  drivers/dma/sdxi/error.c | 340 +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/dma/sdxi/error.h |  16 +++
>  2 files changed, 356 insertions(+)
> 
> diff --git a/drivers/dma/sdxi/error.c b/drivers/dma/sdxi/error.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..c5e33f5989250352f6b081a3049b3b1f972c85a6
> --- /dev/null
> +++ b/drivers/dma/sdxi/error.c

> +/* The "unpacked" counterpart to ERRLOG_HD_ENT. */
> +struct errlog_entry {
> +	u64 dsc_index;
> +	u16 cxt_num;
> +	u16 err_class;
> +	u16 type;
> +	u8 step;
> +	u8 buf;
> +	u8 sub_step;
> +	u8 re;
> +	bool vl;
> +	bool cv;
> +	bool div;
> +	bool bv;
> +};
> +
> +#define ERRLOG_ENTRY_FIELD(hi_, lo_, name_)				\
> +	PACKED_FIELD(hi_, lo_, struct errlog_entry, name_)
> +#define ERRLOG_ENTRY_FLAG(nr_, name_) \
> +	ERRLOG_ENTRY_FIELD(nr_, nr_, name_)
> +
> +/* Refer to "Error Log Header Entry (ERRLOG_HD_ENT)" */
> +static const struct packed_field_u16 errlog_hd_ent_fields[] = {
> +	ERRLOG_ENTRY_FLAG(0, vl),
> +	ERRLOG_ENTRY_FIELD(13, 8, step),
> +	ERRLOG_ENTRY_FIELD(26, 16, type),
> +	ERRLOG_ENTRY_FLAG(32, cv),
> +	ERRLOG_ENTRY_FLAG(33, div),
> +	ERRLOG_ENTRY_FLAG(34, bv),
> +	ERRLOG_ENTRY_FIELD(38, 36, buf),
> +	ERRLOG_ENTRY_FIELD(43, 40, sub_step),
> +	ERRLOG_ENTRY_FIELD(46, 44, re),
> +	ERRLOG_ENTRY_FIELD(63, 48, cxt_num),
> +	ERRLOG_ENTRY_FIELD(127, 64, dsc_index),
> +	ERRLOG_ENTRY_FIELD(367, 352, err_class),

The association between the fields here and struct sdxi_err_log_hd_ent
to me should be via some defines in patch 2 for the various fields
embedded in misc0 etc.

> +};

> +static void sdxi_print_err(struct sdxi_dev *sdxi, u64 err_rd)
> +{
> +	struct errlog_entry ent;
> +	size_t index;
> +
> +	index = err_rd % ERROR_LOG_ENTRIES;
> +
> +	unpack_fields(&sdxi->err_log[index], sizeof(sdxi->err_log[0]),
> +		      &ent, errlog_hd_ent_fields, SDXI_PACKING_QUIRKS);
> +
> +	if (!ent.vl) {
> +		dev_err_ratelimited(sdxi_to_dev(sdxi),
> +				    "Ignoring error log entry with vl=0\n");
> +		return;
> +	}
> +
> +	if (ent.type != OP_TYPE_ERRLOG) {
> +		dev_err_ratelimited(sdxi_to_dev(sdxi),
> +				    "Ignoring error log entry with type=%#x\n",
> +				    ent.type);
> +		return;
> +	}
> +
> +	sdxi_err(sdxi, "error log entry[%zu], MMIO_ERR_RD=%#llx:\n",
> +		 index, err_rd);
> +	sdxi_err(sdxi, "  re: %#x (%s)\n", ent.re, reaction_str(ent.re));
> +	sdxi_err(sdxi, "  step: %#x (%s)\n", ent.step, step_str(ent.step));
> +	sdxi_err(sdxi, "  sub_step: %#x (%s)\n",
> +		 ent.sub_step, sub_step_str(ent.sub_step));
> +	sdxi_err(sdxi, "  cv: %u div: %u bv: %u\n", ent.cv, ent.div, ent.bv);
> +	if (ent.bv)
> +		sdxi_err(sdxi, "  buf: %u\n", ent.buf);
> +	if (ent.cv)
> +		sdxi_err(sdxi, "  cxt_num: %#x\n", ent.cxt_num);
> +	if (ent.div)
> +		sdxi_err(sdxi, "  dsc_index: %#llx\n", ent.dsc_index);
> +	sdxi_err(sdxi, "  err_class: %#x\n", ent.err_class);
Consider using tracepoints for error logging rather than large splats in the
log. Maybe you add those in later patches!

I'd then just fill the tracepoint in directly rather than have an unpacking
step.

> +}

> +/* Refer to "Error Log Initialization" */
> +int sdxi_error_init(struct sdxi_dev *sdxi)
> +{
> +	u64 reg;
> +	int err;
> +
> +	/* 1. Clear MMIO_ERR_CFG. Error interrupts are inhibited until step 6. */
> +	sdxi_write64(sdxi, SDXI_MMIO_ERR_CFG, 0);
> +
> +	/* 2. Clear MMIO_ERR_STS. The flags in this register are RW1C. */
> +	reg = FIELD_PREP(SDXI_MMIO_ERR_STS_STS_BIT, 1) |
> +	      FIELD_PREP(SDXI_MMIO_ERR_STS_OVF_BIT, 1) |
> +	      FIELD_PREP(SDXI_MMIO_ERR_STS_ERR_BIT, 1);
> +	sdxi_write64(sdxi, SDXI_MMIO_ERR_STS, reg);
> +
> +	/* 3. Allocate memory for the error log ring buffer, initialize to zero. */
> +	sdxi->err_log = dma_alloc_coherent(sdxi_to_dev(sdxi), ERROR_LOG_SZ,
> +					   &sdxi->err_log_dma, GFP_KERNEL);
> +	if (!sdxi->err_log)
> +		return -ENOMEM;
> +
> +	/*
> +	 * 4. Set MMIO_ERR_CTL.intr_en to 1 if interrupts on
> +	 * context-level errors are desired.
> +	 */
> +	reg = sdxi_read64(sdxi, SDXI_MMIO_ERR_CTL);
> +	FIELD_MODIFY(SDXI_MMIO_ERR_CTL_EN, &reg, 1);
> +	sdxi_write64(sdxi, SDXI_MMIO_ERR_CTL, reg);
> +
> +	/*
> +	 * The spec is not explicit about when to do this, but this
> +	 * seems like the right time: enable interrupt on
> +	 * function-level transition to error state.
> +	 */
> +	reg = sdxi_read64(sdxi, SDXI_MMIO_CTL0);
> +	FIELD_MODIFY(SDXI_MMIO_CTL0_FN_ERR_INTR_EN, &reg, 1);
> +	sdxi_write64(sdxi, SDXI_MMIO_CTL0, reg);
> +
> +	/* 5. Clear MMIO_ERR_WRT and MMIO_ERR_RD. */
> +	sdxi_write64(sdxi, SDXI_MMIO_ERR_WRT, 0);
> +	sdxi_write64(sdxi, SDXI_MMIO_ERR_RD, 0);
> +
> +	/*
> +	 * Error interrupts can be generated once MMIO_ERR_CFG.en is
> +	 * set in step 6, so set up the handler now.
> +	 */
> +	err = request_threaded_irq(sdxi->error_irq, NULL, sdxi_irq_thread,
> +				   IRQF_TRIGGER_NONE, "SDXI error", sdxi);
> +	if (err)
> +		goto free_errlog;
> +
> +	/* 6. Program MMIO_ERR_CFG. */

I'm guessing these are numbers steps in some bit of the spec?
If not some of these comments like this one provide no value.  We can
see what is being written from the code!  Perhaps add a very specific
spec reference if you want to show why the numbering is here.


> +	reg = FIELD_PREP(SDXI_MMIO_ERR_CFG_PTR, sdxi->err_log_dma >> 12) |
> +	      FIELD_PREP(SDXI_MMIO_ERR_CFG_SZ, ERROR_LOG_ENTRIES >> 6) |
> +	      FIELD_PREP(SDXI_MMIO_ERR_CFG_EN, 1);
> +	sdxi_write64(sdxi, SDXI_MMIO_ERR_CFG, reg);
> +
> +	return 0;
> +
> +free_errlog:
> +	dma_free_coherent(sdxi_to_dev(sdxi), ERROR_LOG_SZ,
> +			  sdxi->err_log, sdxi->err_log_dma);
> +	return err;
> +}
> +
> +void sdxi_error_exit(struct sdxi_dev *sdxi)
> +{
> +	sdxi_write64(sdxi, SDXI_MMIO_ERR_CFG, 0);
> +	free_irq(sdxi->error_irq, sdxi);
> +	dma_free_coherent(sdxi_to_dev(sdxi), ERROR_LOG_SZ,
> +			  sdxi->err_log, sdxi->err_log_dma);
> +}

> 

Re: [PATCH RFC 06/13] dmaengine: sdxi: Add error reporting support

[Nathan Lynch]

Jonathan Cameron <jonathan.cameron@huawei.com> writes:

> On Fri, 05 Sep 2025 13:48:29 -0500
> Nathan Lynch via B4 Relay <devnull+nathan.lynch.amd.com@kernel.org> wrote:
>
>> From: Nathan Lynch <nathan.lynch@amd.com>
>> 
>> SDXI implementations provide software with detailed information about
>> error conditions using a per-device ring buffer in system memory. When
>> an error condition is signaled via interrupt, the driver retrieves any
>> pending error log entries and reports them to the kernel log.
>> 
>> Co-developed-by: Wei Huang <wei.huang2@amd.com>
>> Signed-off-by: Wei Huang <wei.huang2@amd.com>
>> Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
> Hi,
> A few more comments inline. Kind of similar stuff around
> having both register definitions for unpacking and the structure
> definitions in patch 2.
>
> Thanks,
>
> Jonathan
>> ---
>>  drivers/dma/sdxi/error.c | 340 +++++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/dma/sdxi/error.h |  16 +++
>>  2 files changed, 356 insertions(+)
>> 
>> diff --git a/drivers/dma/sdxi/error.c b/drivers/dma/sdxi/error.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..c5e33f5989250352f6b081a3049b3b1f972c85a6
>> --- /dev/null
>> +++ b/drivers/dma/sdxi/error.c
>
>> +/* The "unpacked" counterpart to ERRLOG_HD_ENT. */
>> +struct errlog_entry {
>> +	u64 dsc_index;
>> +	u16 cxt_num;
>> +	u16 err_class;
>> +	u16 type;
>> +	u8 step;
>> +	u8 buf;
>> +	u8 sub_step;
>> +	u8 re;
>> +	bool vl;
>> +	bool cv;
>> +	bool div;
>> +	bool bv;
>> +};
>> +
>> +#define ERRLOG_ENTRY_FIELD(hi_, lo_, name_)				\
>> +	PACKED_FIELD(hi_, lo_, struct errlog_entry, name_)
>> +#define ERRLOG_ENTRY_FLAG(nr_, name_) \
>> +	ERRLOG_ENTRY_FIELD(nr_, nr_, name_)
>> +
>> +/* Refer to "Error Log Header Entry (ERRLOG_HD_ENT)" */
>> +static const struct packed_field_u16 errlog_hd_ent_fields[] = {
>> +	ERRLOG_ENTRY_FLAG(0, vl),
>> +	ERRLOG_ENTRY_FIELD(13, 8, step),
>> +	ERRLOG_ENTRY_FIELD(26, 16, type),
>> +	ERRLOG_ENTRY_FLAG(32, cv),
>> +	ERRLOG_ENTRY_FLAG(33, div),
>> +	ERRLOG_ENTRY_FLAG(34, bv),
>> +	ERRLOG_ENTRY_FIELD(38, 36, buf),
>> +	ERRLOG_ENTRY_FIELD(43, 40, sub_step),
>> +	ERRLOG_ENTRY_FIELD(46, 44, re),
>> +	ERRLOG_ENTRY_FIELD(63, 48, cxt_num),
>> +	ERRLOG_ENTRY_FIELD(127, 64, dsc_index),
>> +	ERRLOG_ENTRY_FIELD(367, 352, err_class),
>
> The association between the fields here and struct sdxi_err_log_hd_ent
> to me should be via some defines in patch 2 for the various fields
> embedded in misc0 etc.
>
>> +};
>
>> +static void sdxi_print_err(struct sdxi_dev *sdxi, u64 err_rd)
>> +{
>> +	struct errlog_entry ent;
>> +	size_t index;
>> +
>> +	index = err_rd % ERROR_LOG_ENTRIES;
>> +
>> +	unpack_fields(&sdxi->err_log[index], sizeof(sdxi->err_log[0]),
>> +		      &ent, errlog_hd_ent_fields, SDXI_PACKING_QUIRKS);
>> +
>> +	if (!ent.vl) {
>> +		dev_err_ratelimited(sdxi_to_dev(sdxi),
>> +				    "Ignoring error log entry with vl=0\n");
>> +		return;
>> +	}
>> +
>> +	if (ent.type != OP_TYPE_ERRLOG) {
>> +		dev_err_ratelimited(sdxi_to_dev(sdxi),
>> +				    "Ignoring error log entry with type=%#x\n",
>> +				    ent.type);
>> +		return;
>> +	}
>> +
>> +	sdxi_err(sdxi, "error log entry[%zu], MMIO_ERR_RD=%#llx:\n",
>> +		 index, err_rd);
>> +	sdxi_err(sdxi, "  re: %#x (%s)\n", ent.re, reaction_str(ent.re));
>> +	sdxi_err(sdxi, "  step: %#x (%s)\n", ent.step, step_str(ent.step));
>> +	sdxi_err(sdxi, "  sub_step: %#x (%s)\n",
>> +		 ent.sub_step, sub_step_str(ent.sub_step));
>> +	sdxi_err(sdxi, "  cv: %u div: %u bv: %u\n", ent.cv, ent.div, ent.bv);
>> +	if (ent.bv)
>> +		sdxi_err(sdxi, "  buf: %u\n", ent.buf);
>> +	if (ent.cv)
>> +		sdxi_err(sdxi, "  cxt_num: %#x\n", ent.cxt_num);
>> +	if (ent.div)
>> +		sdxi_err(sdxi, "  dsc_index: %#llx\n", ent.dsc_index);
>> +	sdxi_err(sdxi, "  err_class: %#x\n", ent.err_class);
> Consider using tracepoints for error logging rather than large splats
> in the log.

Agreed, context-level errors (which will be user-triggerable once there
is an ABI exposed to user space) should not be dumped to the kernel log
by default.

Some function-level errors may be appropriate to print.

> I'd then just fill the tracepoint in directly rather than have an
> unpacking step.

Yes, I can do that.


>
>> +}
>
>> +/* Refer to "Error Log Initialization" */
>> +int sdxi_error_init(struct sdxi_dev *sdxi)
>> +{
>> +	u64 reg;
>> +	int err;
>> +
>> +	/* 1. Clear MMIO_ERR_CFG. Error interrupts are inhibited until step 6. */
>> +	sdxi_write64(sdxi, SDXI_MMIO_ERR_CFG, 0);
>> +
>> +	/* 2. Clear MMIO_ERR_STS. The flags in this register are RW1C. */
>> +	reg = FIELD_PREP(SDXI_MMIO_ERR_STS_STS_BIT, 1) |
>> +	      FIELD_PREP(SDXI_MMIO_ERR_STS_OVF_BIT, 1) |
>> +	      FIELD_PREP(SDXI_MMIO_ERR_STS_ERR_BIT, 1);
>> +	sdxi_write64(sdxi, SDXI_MMIO_ERR_STS, reg);
>> +
>> +	/* 3. Allocate memory for the error log ring buffer, initialize to zero. */
>> +	sdxi->err_log = dma_alloc_coherent(sdxi_to_dev(sdxi), ERROR_LOG_SZ,
>> +					   &sdxi->err_log_dma, GFP_KERNEL);
>> +	if (!sdxi->err_log)
>> +		return -ENOMEM;
>> +
>> +	/*
>> +	 * 4. Set MMIO_ERR_CTL.intr_en to 1 if interrupts on
>> +	 * context-level errors are desired.
>> +	 */
>> +	reg = sdxi_read64(sdxi, SDXI_MMIO_ERR_CTL);
>> +	FIELD_MODIFY(SDXI_MMIO_ERR_CTL_EN, &reg, 1);
>> +	sdxi_write64(sdxi, SDXI_MMIO_ERR_CTL, reg);
>> +
>> +	/*
>> +	 * The spec is not explicit about when to do this, but this
>> +	 * seems like the right time: enable interrupt on
>> +	 * function-level transition to error state.
>> +	 */
>> +	reg = sdxi_read64(sdxi, SDXI_MMIO_CTL0);
>> +	FIELD_MODIFY(SDXI_MMIO_CTL0_FN_ERR_INTR_EN, &reg, 1);
>> +	sdxi_write64(sdxi, SDXI_MMIO_CTL0, reg);
>> +
>> +	/* 5. Clear MMIO_ERR_WRT and MMIO_ERR_RD. */
>> +	sdxi_write64(sdxi, SDXI_MMIO_ERR_WRT, 0);
>> +	sdxi_write64(sdxi, SDXI_MMIO_ERR_RD, 0);
>> +
>> +	/*
>> +	 * Error interrupts can be generated once MMIO_ERR_CFG.en is
>> +	 * set in step 6, so set up the handler now.
>> +	 */
>> +	err = request_threaded_irq(sdxi->error_irq, NULL, sdxi_irq_thread,
>> +				   IRQF_TRIGGER_NONE, "SDXI error", sdxi);
>> +	if (err)
>> +		goto free_errlog;
>> +
>> +	/* 6. Program MMIO_ERR_CFG. */
>
> I'm guessing these are numbers steps in some bit of the spec?
> If not some of these comments like this one provide no value.  We can
> see what is being written from the code!  Perhaps add a very specific
> spec reference if you want to show why the numbering is here.

Perhaps it's understated, but at the beginning of this function:

  /* Refer to "Error Log Initialization" */
  int sdxi_error_init(struct sdxi_dev *sdxi)

The numbered steps in the function correspond to the numbered steps in
that part of the spec.

I could make the comment something like:

/*
 * The numbered steps below correspond to the sequence outlined in 3.4.2
 * "Error Log Initialization".
 */

though I'm unsure how stable the section numbering in the SDXI spec will
be over time.

Re: [PATCH RFC 06/13] dmaengine: sdxi: Add error reporting support

[Jonathan Cameron]

> >> +
> >> +	/* 6. Program MMIO_ERR_CFG. */  
> >
> > I'm guessing these are numbers steps in some bit of the spec?
> > If not some of these comments like this one provide no value.  We can
> > see what is being written from the code!  Perhaps add a very specific
> > spec reference if you want to show why the numbering is here.  
> 
> Perhaps it's understated, but at the beginning of this function:
> 
>   /* Refer to "Error Log Initialization" */
>   int sdxi_error_init(struct sdxi_dev *sdxi)
> 
> The numbered steps in the function correspond to the numbered steps in
> that part of the spec.
> 
> I could make the comment something like:
> 
> /*
>  * The numbered steps below correspond to the sequence outlined in 3.4.2
>  * "Error Log Initialization".
>  */
> 
> though I'm unsure how stable the section numbering in the SDXI spec will
> be over time.

Always reference sections by name (which you do!) and version of the spec
for alongside the section number.  They are rarely stable for long.

Jonathan

[PATCH RFC 07/13] dmaengine: sdxi: Import descriptor enqueue code from spec

[Nathan Lynch via B4 Relay]

From: Nathan Lynch <nathan.lynch@amd.com>

Import the example code from the "SDXI Descriptor Ring Operation"
chapter of the SDXI 1.0 spec[1], which demonstrates lockless
descriptor submission to the ring. Lightly alter the code
to (somewhat) comply with Linux coding style, and use byte order-aware
types as well as kernel atomic and barrier APIs.

Ultimately we may not really need a lockless submission path, and it
would be better for it to more closely integrate with the rest of the
driver.

[1] https://www.snia.org/sites/default/files/technical-work/sdxi/release/SNIA-SDXI-Specification-v1.0a.pdf

Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
---
 drivers/dma/sdxi/enqueue.c | 136 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/dma/sdxi/enqueue.h |  16 ++++++
 2 files changed, 152 insertions(+)

diff --git a/drivers/dma/sdxi/enqueue.c b/drivers/dma/sdxi/enqueue.c
new file mode 100644
index 0000000000000000000000000000000000000000..822d9b890fa3538dcc09e99ef562a6d8419290f0
--- /dev/null
+++ b/drivers/dma/sdxi/enqueue.c
@@ -0,0 +1,136 @@
+// SPDX-License-Identifier: BSD-3-Clause
+/*
+ *
+ * Copyright (c) 2024, The Storage Networking Industry Association.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the
+ * distribution.
+ *
+ * * Neither the name of The Storage Networking Industry Association
+ * (SNIA) nor the names of its contributors may be used to endorse or
+ * promote products derived from this software without specific prior
+ * written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
+ * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
+ * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
+ * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+ * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
+ * OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <asm/barrier.h>
+#include <asm/byteorder.h>
+#include <asm/rwonce.h>
+#include <linux/atomic.h>
+#include <linux/errno.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/processor.h>
+#include <linux/types.h>
+
+#include "enqueue.h"
+
+/*
+ * Code adapted from the "SDXI Descriptor Ring Operation" chapter of
+ * the SDXI spec, specifically the example code in "Enqueuing one or
+ * more Descriptors."
+ */
+
+#define SDXI_DESCR_SIZE 64
+#define SDXI_DS_NUM_QW (SDXI_DESCR_SIZE / sizeof(__le64))
+#define SDXI_MULTI_PRODUCER 1  /* Define to 0 if single-producer. */
+
+static int update_ring(const __le64 *enq_entries,   /* Ptr to entries to enqueue */
+		       u64 enq_num,                 /* Number of entries to enqueue */
+		       __le64 *ring_base,           /* Ptr to ring location */
+		       u64 ring_size,               /* (Ring Size in bytes)/64 */
+		       u64 index)                   /* Starting ring index to update */
+{
+	for (u64 i = 0; i < enq_num; i++) {
+		__le64 *ringp = ring_base + ((index + i) % ring_size) * SDXI_DS_NUM_QW;
+		const __le64 *entryp = enq_entries + (i * SDXI_DS_NUM_QW);
+
+		for (u64 j = 1; j < SDXI_DS_NUM_QW; j++)
+			*(ringp + j) = *(entryp + j);
+	}
+
+	/* Now write the first QW of the new entries to the ring. */
+	dma_wmb();
+	for (u64 i = 0; i < enq_num; i++) {
+		__le64 *ringp = ring_base + ((index + i) % ring_size) * SDXI_DS_NUM_QW;
+		const __le64 *entryp = enq_entries + (i * SDXI_DS_NUM_QW);
+
+		*ringp = *entryp;
+	}
+
+	return 0;
+}
+
+int sdxi_enqueue(const __le64 *enq_entries,                 /* Ptr to entries to enqueue */
+		 u64 enq_num,                               /* Number of entries to enqueue */
+		 __le64 *ring_base,                         /* Ptr to ring location */
+		 u64 ring_size,                             /* (Ring Size in bytes)/64 */
+		 __le64 const volatile * const Read_Index,  /* Ptr to Read_Index location */
+		 __le64 volatile * const Write_Index,       /* Ptr to Write_Index location */
+		 __le64 __iomem *Door_Bell)                 /* Ptr to Ring Doorbell location */
+{
+	u64 old_write_idx;
+	u64 new_idx;
+
+	while (true) {
+		u64 read_idx;
+
+		read_idx = le64_to_cpu(READ_ONCE(*Read_Index));
+		dma_rmb();  /* Get Read_Index before Write_Index to always get consistent values */
+		old_write_idx = le64_to_cpu(READ_ONCE(*Write_Index));
+
+		if (read_idx > old_write_idx) {
+			/* Only happens if Write_Index wraps or ring has bad setup */
+			return -EIO;
+		}
+
+		new_idx = old_write_idx + enq_num;
+		if (new_idx - read_idx > ring_size) {
+			cpu_relax();
+			continue;  /* Not enough free entries, try again */
+		}
+
+		if (SDXI_MULTI_PRODUCER) {
+			/* Try to atomically update Write_Index. */
+			bool success = cmpxchg(Write_Index,
+					       cpu_to_le64(old_write_idx),
+					       cpu_to_le64(new_idx)) == cpu_to_le64(old_write_idx);
+			if (success)
+				break;  /* Updated Write_Index, no need to try again. */
+		} else {
+			/* Single-Producer case */
+			WRITE_ONCE(*Write_Index, cpu_to_le64(new_idx));
+			dma_wmb();  /* Make the Write_Index update visible before the Door_Bell update. */
+			break;  /* Always successful for single-producer */
+		}
+		/* Couldn"t update Write_Index, try again. */
+	}
+
+	/* Write_Index is now advanced. Let's write out entries to the ring. */
+	update_ring(enq_entries, enq_num, ring_base, ring_size, old_write_idx);
+
+	/* Door_Bell write required; only needs ordering wrt update of Write_Index. */
+	iowrite64(new_idx, Door_Bell);
+
+	return 0;
+}
diff --git a/drivers/dma/sdxi/enqueue.h b/drivers/dma/sdxi/enqueue.h
new file mode 100644
index 0000000000000000000000000000000000000000..28c1493779db1119ff0d682fa6623b016998042a
--- /dev/null
+++ b/drivers/dma/sdxi/enqueue.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+/* Copyright (c) 2024, The Storage Networking Industry Association. */
+#ifndef DMA_SDXI_ENQUEUE_H
+#define DMA_SDXI_ENQUEUE_H
+
+#include <linux/types.h>
+
+int sdxi_enqueue(const __le64 *enq_entries,  /* Ptr to entries to enqueue */
+		 u64 enq_num,  /* Number of entries to enqueue */
+		 __le64 *ring_base,  /* Ptr to ring location */
+		 u64 ring_size,  /* (Ring Size in bytes)/64 */
+		 __le64 const volatile * const Read_Index,  /* Ptr to Read_Index location */
+		 __le64 volatile * const Write_Index,  /* Ptr to Write_Index location */
+		 __le64 __iomem *Door_Bell);  /* Ptr to Ring Doorbell location */
+
+#endif /* DMA_SDXI_ENQUEUE_H */

-- 
2.39.5

Re: [PATCH RFC 07/13] dmaengine: sdxi: Import descriptor enqueue code from spec

[Jonathan Cameron]

On Fri, 05 Sep 2025 13:48:30 -0500
Nathan Lynch via B4 Relay <devnull+nathan.lynch.amd.com@kernel.org> wrote:

> From: Nathan Lynch <nathan.lynch@amd.com>
> 
> Import the example code from the "SDXI Descriptor Ring Operation"
> chapter of the SDXI 1.0 spec[1], which demonstrates lockless
> descriptor submission to the ring. Lightly alter the code
> to (somewhat) comply with Linux coding style, and use byte order-aware
> types as well as kernel atomic and barrier APIs.
> 
> Ultimately we may not really need a lockless submission path, and it
> would be better for it to more closely integrate with the rest of the
> driver.
> 
> [1] https://www.snia.org/sites/default/files/technical-work/sdxi/release/SNIA-SDXI-Specification-v1.0a.pdf
> 
> Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
Hi Nathan,

I suspect you have a good idea of what needs to happen to get this ready for a merge
but I'll comment briefly anyway!

> ---
>  drivers/dma/sdxi/enqueue.c | 136 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/dma/sdxi/enqueue.h |  16 ++++++
>  2 files changed, 152 insertions(+)
> 
> diff --git a/drivers/dma/sdxi/enqueue.c b/drivers/dma/sdxi/enqueue.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..822d9b890fa3538dcc09e99ef562a6d8419290f0
> --- /dev/null
> +++ b/drivers/dma/sdxi/enqueue.c
> @@ -0,0 +1,136 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + *
> + * Copyright (c) 2024, The Storage Networking Industry Association.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *
> + * * Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the
> + * distribution.
> + *
> + * * Neither the name of The Storage Networking Industry Association
> + * (SNIA) nor the names of its contributors may be used to endorse or
> + * promote products derived from this software without specific prior
> + * written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
> + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
> + * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
> + * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
> + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
> + * OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include <asm/barrier.h>
> +#include <asm/byteorder.h>
> +#include <asm/rwonce.h>
> +#include <linux/atomic.h>
> +#include <linux/errno.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/processor.h>
> +#include <linux/types.h>
> +
> +#include "enqueue.h"
> +
> +/*
> + * Code adapted from the "SDXI Descriptor Ring Operation" chapter of
> + * the SDXI spec, specifically the example code in "Enqueuing one or
> + * more Descriptors."
> + */
> +
> +#define SDXI_DESCR_SIZE 64
I think that's already effectively encoded in the asserts in the header.
Hence don't repeat it here. Use sizeof() whatever makes sense.

> +#define SDXI_DS_NUM_QW (SDXI_DESCR_SIZE / sizeof(__le64))

> +#define SDXI_MULTI_PRODUCER 1  /* Define to 0 if single-producer. */

Get rid of other path and drop this.

> +
> +static int update_ring(const __le64 *enq_entries,   /* Ptr to entries to enqueue */
> +		       u64 enq_num,                 /* Number of entries to enqueue */
> +		       __le64 *ring_base,           /* Ptr to ring location */
> +		       u64 ring_size,               /* (Ring Size in bytes)/64 */
> +		       u64 index)                   /* Starting ring index to update */

Whilst I get the minimal changes bit, make this kernel-doc.

> +{
> +	for (u64 i = 0; i < enq_num; i++) {
> +		__le64 *ringp = ring_base + ((index + i) % ring_size) * SDXI_DS_NUM_QW;
> +		const __le64 *entryp = enq_entries + (i * SDXI_DS_NUM_QW);
> +
> +		for (u64 j = 1; j < SDXI_DS_NUM_QW; j++)
> +			*(ringp + j) = *(entryp + j);

memcpy?

> +	}
> +
> +	/* Now write the first QW of the new entries to the ring. */
> +	dma_wmb();
> +	for (u64 i = 0; i < enq_num; i++) {
> +		__le64 *ringp = ring_base + ((index + i) % ring_size) * SDXI_DS_NUM_QW;
> +		const __le64 *entryp = enq_entries + (i * SDXI_DS_NUM_QW);
> +
> +		*ringp = *entryp;
> +	}
> +
> +	return 0;
> +}
> +
> +int sdxi_enqueue(const __le64 *enq_entries,                 /* Ptr to entries to enqueue */
> +		 u64 enq_num,                               /* Number of entries to enqueue */
> +		 __le64 *ring_base,                         /* Ptr to ring location */
> +		 u64 ring_size,                             /* (Ring Size in bytes)/64 */
> +		 __le64 const volatile * const Read_Index,  /* Ptr to Read_Index location */
> +		 __le64 volatile * const Write_Index,       /* Ptr to Write_Index location */
> +		 __le64 __iomem *Door_Bell)                 /* Ptr to Ring Doorbell location */
> +{
> +	u64 old_write_idx;
> +	u64 new_idx;
> +
> +	while (true) {
> +		u64 read_idx;
> +
> +		read_idx = le64_to_cpu(READ_ONCE(*Read_Index));
> +		dma_rmb();  /* Get Read_Index before Write_Index to always get consistent values */
> +		old_write_idx = le64_to_cpu(READ_ONCE(*Write_Index));
> +
> +		if (read_idx > old_write_idx) {
> +			/* Only happens if Write_Index wraps or ring has bad setup */
> +			return -EIO;
> +		}
> +
> +		new_idx = old_write_idx + enq_num;
> +		if (new_idx - read_idx > ring_size) {
> +			cpu_relax();
> +			continue;  /* Not enough free entries, try again */
> +		}
> +
> +		if (SDXI_MULTI_PRODUCER) {
> +			/* Try to atomically update Write_Index. */
> +			bool success = cmpxchg(Write_Index,
> +					       cpu_to_le64(old_write_idx),
> +					       cpu_to_le64(new_idx)) == cpu_to_le64(old_write_idx);
> +			if (success)
> +				break;  /* Updated Write_Index, no need to try again. */
> +		} else {
> +			/* Single-Producer case */
> +			WRITE_ONCE(*Write_Index, cpu_to_le64(new_idx));
> +			dma_wmb();  /* Make the Write_Index update visible before the Door_Bell update. */
> +			break;  /* Always successful for single-producer */
> +		}
> +		/* Couldn"t update Write_Index, try again. */
> +	}
> +
> +	/* Write_Index is now advanced. Let's write out entries to the ring. */
> +	update_ring(enq_entries, enq_num, ring_base, ring_size, old_write_idx);
> +
> +	/* Door_Bell write required; only needs ordering wrt update of Write_Index. */
> +	iowrite64(new_idx, Door_Bell);
> +
> +	return 0;
> +}
> diff --git a/drivers/dma/sdxi/enqueue.h b/drivers/dma/sdxi/enqueue.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..28c1493779db1119ff0d682fa6623b016998042a
> --- /dev/null
> +++ b/drivers/dma/sdxi/enqueue.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: BSD-3-Clause */
> +/* Copyright (c) 2024, The Storage Networking Industry Association. */
> +#ifndef DMA_SDXI_ENQUEUE_H
> +#define DMA_SDXI_ENQUEUE_H
> +
> +#include <linux/types.h>
> +
> +int sdxi_enqueue(const __le64 *enq_entries,  /* Ptr to entries to enqueue */
> +		 u64 enq_num,  /* Number of entries to enqueue */
> +		 __le64 *ring_base,  /* Ptr to ring location */
> +		 u64 ring_size,  /* (Ring Size in bytes)/64 */
> +		 __le64 const volatile * const Read_Index,  /* Ptr to Read_Index location */
> +		 __le64 volatile * const Write_Index,  /* Ptr to Write_Index location */
> +		 __le64 __iomem *Door_Bell);  /* Ptr to Ring Doorbell location */
> +
> +#endif /* DMA_SDXI_ENQUEUE_H */
> 

Re: [External] : [PATCH RFC 07/13] dmaengine: sdxi: Import descriptor enqueue code from spec

[ALOK TIWARI]

On 9/6/2025 12:18 AM, Nathan Lynch via B4 Relay wrote:
> +		} else {
> +			/* Single-Producer case */
> +			WRITE_ONCE(*Write_Index, cpu_to_le64(new_idx));
> +			dma_wmb();  /* Make the Write_Index update visible before the Door_Bell update. */
> +			break;  /* Always successful for single-producer */
> +		}
> +		/* Couldn"t update Write_Index, try again. */
> +	}

Typo Couldn"t -> Couldn't

Thanks,
Alok

[PATCH RFC 08/13] dmaengine: sdxi: Context creation/removal, descriptor submission

[Nathan Lynch via B4 Relay]

From: Nathan Lynch <nathan.lynch@amd.com>

Add functions for creating and removing SDXI contexts and submitting
descriptors against them.

An SDXI function supports one or more contexts, each of which has its
own descriptor ring and associated state. Each context has a 16-bit
index. A special context is installed at index 0 and is used for
configuring other contexts and performing administrative actions.

The creation of each context entails the allocation of the following
control structure hierarchy:

* Context L1 Table slot
  * Access key (AKey) table
  * Context control block
    * Descriptor ring
    * Write index
    * Context status block

Co-developed-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
---
 drivers/dma/sdxi/context.c | 547 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/dma/sdxi/context.h |  28 +++
 2 files changed, 575 insertions(+)

diff --git a/drivers/dma/sdxi/context.c b/drivers/dma/sdxi/context.c
new file mode 100644
index 0000000000000000000000000000000000000000..50eae5b3b303d67891113377e2df209d199aa533
--- /dev/null
+++ b/drivers/dma/sdxi/context.c
@@ -0,0 +1,547 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * SDXI submission queue (sq) and descriptor management
+ *
+ * Copyright (C) 2025 Advanced Micro Devices, Inc.
+ */
+
+#define pr_fmt(fmt)     "SDXI: " fmt
+
+#include <linux/delay.h>
+#include <linux/dma-direction.h>
+#include <linux/dma-mapping.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/types.h>
+#include <linux/wordpart.h>
+
+#include "context.h"
+#include "descriptor.h"
+#include "enqueue.h"
+#include "hw.h"
+#include "sdxi.h"
+
+/* Alloc sdxi_sq in kernel space */
+static struct sdxi_sq *sdxi_sq_alloc(struct sdxi_cxt *cxt, int ring_entries)
+{
+	struct sdxi_dev *sdxi = cxt->sdxi;
+	struct device *dev = sdxi_to_dev(sdxi);
+	u64 write_index_ptr;
+	struct sdxi_sq *sq;
+	u64 ds_ring_ptr;
+	u64 cxt_sts_ptr;
+	u32 ds_ring_sz;
+
+	/* alloc desc_ring */
+	if (ring_entries > sdxi->max_ring_entries) {
+		sdxi_err(sdxi, "%d ring entries requested, max is %llu\n",
+			ring_entries, sdxi->max_ring_entries);
+		return NULL;
+	}
+
+	sq = kzalloc(sizeof(*sq), GFP_KERNEL);
+	if (!sq)
+		return NULL;
+
+	sq->ring_entries = ring_entries;
+	sq->ring_size = sizeof(sq->desc_ring[0]) * sq->ring_entries;
+	sq->desc_ring = dma_alloc_coherent(dev, sq->ring_size, &sq->ring_dma,
+					   GFP_KERNEL);
+	if (!sq->desc_ring)
+		goto free_sq;
+
+	sq->cxt_sts = dma_pool_zalloc(sdxi->cxt_sts_pool, GFP_KERNEL, &sq->cxt_sts_dma);
+	if (!sq->cxt_sts)
+		goto free_desc_ring;
+
+	sq->write_index = dma_pool_zalloc(sdxi->write_index_pool, GFP_KERNEL,
+					  &sq->write_index_dma);
+	if (!sq->write_index)
+		goto free_cxt_sts;
+
+	/* final setup */
+	if (cxt->id == SDXI_ADMIN_CXT_ID || cxt->id == SDXI_DMA_CXT_ID)
+		sq->cxt_sts->state = FIELD_PREP(SDXI_CXT_STS_STATE, CXTV_RUN);
+
+	write_index_ptr = FIELD_PREP(SDXI_CXT_CTL_WRITE_INDEX_PTR,
+				     sq->write_index_dma >> 3);
+	cxt_sts_ptr = FIELD_PREP(SDXI_CXT_CTL_CXT_STS_PTR,
+				 sq->cxt_sts_dma >> 4);
+	ds_ring_sz = sq->ring_size >> 6;
+
+	cxt->cxt_ctl->write_index_ptr = cpu_to_le64(write_index_ptr);
+	cxt->cxt_ctl->cxt_sts_ptr     = cpu_to_le64(cxt_sts_ptr);
+	cxt->cxt_ctl->ds_ring_sz      = cpu_to_le32(ds_ring_sz);
+
+	/* turn it on now */
+	sq->cxt = cxt;
+	cxt->sq = sq;
+	ds_ring_ptr = (FIELD_PREP(SDXI_CXT_CTL_DS_RING_PTR, sq->ring_dma >> 6) |
+		       FIELD_PREP(SDXI_CXT_CTL_VL, 1));
+	dma_wmb();
+	WRITE_ONCE(cxt->cxt_ctl->ds_ring_ptr, cpu_to_le64(ds_ring_ptr));
+
+	sdxi_dbg(sdxi, "sq created, id=%d, cxt_ctl=%p\n"
+		 "  desc ring addr:   v=0x%p:d=%pad\n"
+		 "  write index addr: v=0x%p:d=%pad\n"
+		 "  cxt status addr: v=0x%p:d=%pad\n",
+		 cxt->id, cxt->cxt_ctl,
+		 sq->desc_ring, &sq->ring_dma,
+		 sq->write_index, &sq->write_index_dma,
+		 sq->cxt_sts, &sq->cxt_sts_dma);
+
+	return sq;
+
+free_cxt_sts:
+	dma_pool_free(sdxi->cxt_sts_pool, sq->cxt_sts, sq->cxt_sts_dma);
+free_desc_ring:
+	dma_free_coherent(dev, sq->ring_size, sq->desc_ring, sq->ring_dma);
+free_sq:
+	kfree(sq);
+	return NULL;
+}
+
+static void sdxi_sq_free(struct sdxi_sq *sq)
+{
+	struct sdxi_cxt *cxt = sq->cxt;
+	struct sdxi_dev *sdxi = cxt->sdxi;
+	struct device *dev = sdxi_to_dev(sdxi);
+
+	if (!cxt)
+		return;
+
+	dma_pool_free(sdxi->write_index_pool, sq->write_index, sq->write_index_dma);
+	dma_pool_free(sdxi->cxt_sts_pool, sq->cxt_sts, sq->cxt_sts_dma);
+	dma_free_coherent(dev, sq->ring_size, sq->desc_ring, sq->ring_dma);
+
+	cxt->sq = NULL;
+	kfree(sq);
+}
+
+/* Default size 1024 ==> 64KB descriptor ring, guaranteed */
+#define DEFAULT_DESC_RING_ENTRIES	1024
+static struct sdxi_sq *sdxi_sq_alloc_default(struct sdxi_cxt *cxt)
+{
+	return sdxi_sq_alloc(cxt, DEFAULT_DESC_RING_ENTRIES);
+}
+
+static bool sdxi_cxt_l2_ent_vl(const struct sdxi_cxt_l2_ent *ent)
+{
+	return FIELD_GET(SDXI_CXT_L2_ENT_VL, le64_to_cpu(ent->lv01_ptr));
+}
+
+static dma_addr_t sdxi_cxt_l2_ent_lv01_ptr(const struct sdxi_cxt_l2_ent *ent)
+{
+	return FIELD_GET(SDXI_CXT_L2_ENT_LV01_PTR, le64_to_cpu(ent->lv01_ptr)) << ilog2(SZ_4K);
+}
+
+static void set_cxt_l2_entry(struct sdxi_cxt_l2_ent *l2_entry,
+			     dma_addr_t l1_table_dma)
+{
+	u64 lv01_ptr;
+
+	/* We shouldn't be updating a live entry. */
+	if (WARN_ON_ONCE(sdxi_cxt_l2_ent_vl(l2_entry)))
+		return;
+	/* L1 tables must be 4K-aligned. */
+	if (WARN_ON_ONCE(!IS_ALIGNED(l1_table_dma, SZ_4K)))
+		return;
+
+	lv01_ptr = (FIELD_PREP(SDXI_CXT_L2_ENT_LV01_PTR,
+				   l1_table_dma >> ilog2(SZ_4K)) |
+		    FIELD_PREP(SDXI_CXT_L2_ENT_VL, 1));
+
+	/*
+	 * Ensure the valid bit update follows prior updates to other
+	 * control structures.
+	 */
+	dma_wmb();
+	WRITE_ONCE(l2_entry->lv01_ptr, cpu_to_le64(lv01_ptr));
+}
+
+static void set_cxt_l1_entry(struct sdxi_dev *sdxi,
+			     struct sdxi_cxt_l1_ent *l1_entry,
+			     struct sdxi_cxt *cxt)
+{
+	u64 cxt_ctl_ptr;
+	u64 akey_ptr;
+	u16 intr_num;
+	u32 misc0;
+
+	if (!cxt) {
+		memset(l1_entry, 0, sizeof(*l1_entry));
+		return;
+	}
+
+	cxt_ctl_ptr = (FIELD_PREP(SDXI_CXT_L1_ENT_VL, 1) |
+		       FIELD_PREP(SDXI_CXT_L1_ENT_KA, 1) |
+		       FIELD_PREP(SDXI_CXT_L1_ENT_CXT_CTL_PTR,
+				  cxt->cxt_ctl_dma >> L1_CXT_CTRL_PTR_SHIFT));
+	akey_ptr = (FIELD_PREP(SDXI_CXT_L1_ENT_AKEY_SZ,
+			       akey_table_order(cxt->akey_table)) |
+		    FIELD_PREP(SDXI_CXT_L1_ENT_AKEY_PTR,
+			       cxt->akey_table_dma >> L1_CXT_AKEY_PTR_SHIFT));
+	misc0 = FIELD_PREP(SDXI_CXT_L1_ENT_MAX_BUFFER, 11);
+
+	*l1_entry = (struct sdxi_cxt_l1_ent) {
+		.cxt_ctl_ptr = cpu_to_le64(cxt_ctl_ptr),
+		.akey_ptr = cpu_to_le64(akey_ptr),
+		.misc0 = cpu_to_le32(misc0),
+		.opb_000_enb = cpu_to_le32(sdxi->op_grp_cap),
+	};
+
+	intr_num = le16_to_cpu(cxt->akey_table->entry[0].intr_num);
+	FIELD_MODIFY(SDXI_AKEY_ENT_VL, &intr_num, 1);
+	cxt->akey_table->entry[0].intr_num = cpu_to_le16(intr_num);
+}
+
+static int config_cxt_tables(struct sdxi_dev *sdxi,
+			     struct sdxi_cxt *cxt)
+{
+	struct sdxi_cxt_l1_table *l1_table;
+	struct sdxi_cxt_l1_ent *l1_entry;
+	u16 l2_idx;
+	u8 l1_idx;
+
+	l2_idx = ID_TO_L2_INDEX(cxt->id);
+	l1_idx = ID_TO_L1_INDEX(cxt->id);
+
+	/* Allocate L1 table if not present. */
+	l1_table = sdxi->l1_table_array[l2_idx];
+	if (!l1_table) {
+		struct sdxi_cxt_l2_ent *l2_entry;
+		dma_addr_t l1_table_dma;
+
+		l1_table = dma_alloc_coherent(sdxi_to_dev(sdxi),
+					      sizeof(*l1_table),
+					      &l1_table_dma, GFP_KERNEL);
+		if (!l1_table)
+			return -ENOMEM;
+
+		/* Track the L1 table vaddr. */
+		sdxi->l1_table_array[l2_idx] = l1_table;
+
+		/* Install the new entry in the L2 table. */
+		l2_entry = &sdxi->l2_table->entry[l2_idx];
+		set_cxt_l2_entry(l2_entry, l1_table_dma);
+	}
+
+	/* Populate the L1 entry. */
+	l1_entry = &l1_table->entry[l1_idx];
+	set_cxt_l1_entry(cxt->sdxi, l1_entry, cxt);
+
+	return 0;
+}
+
+static void clear_cxt_table_entries(struct sdxi_cxt_l2_table *l2_table,
+				    struct sdxi_cxt_l1_table *l1_table,
+				    struct sdxi_cxt *cxt)
+{
+	struct sdxi_cxt_l2_ent *l2_entry;
+	struct sdxi_cxt_l1_ent *l1_entry;
+	struct sdxi_dev *sdxi = cxt->sdxi;
+	dma_addr_t l1_dma;
+
+	l2_entry = &l2_table->entry[ID_TO_L2_INDEX(cxt->id)];
+	l1_entry = &l1_table->entry[ID_TO_L1_INDEX(cxt->id)];
+
+	memset(l1_entry, 0, sizeof(*l1_entry));
+
+	/* If this L1 table has been completely zeroed then free it. */
+	if (memchr_inv(l1_table, 0, L1_TABLE_SIZE))
+		return;
+
+	l1_dma = sdxi_cxt_l2_ent_lv01_ptr(l2_entry);
+
+	memset(l2_entry, 0, sizeof(*l2_entry));
+
+	dma_free_coherent(sdxi_to_dev(sdxi), sizeof(*l1_table),
+			  l1_table, l1_dma);
+}
+
+static void cleanup_cxt_tables(struct sdxi_dev *sdxi,
+			       struct sdxi_cxt *cxt)
+{
+	u16 l2_idx;
+	struct sdxi_cxt_l1_table *l1_table;
+
+	if (!cxt)
+		return;
+
+	l2_idx = ID_TO_L2_INDEX(cxt->id);
+
+	l1_table = sdxi->l1_table_array[l2_idx];
+	/* clear l1 entry */
+	/* FIXME combine clear_cxt_table_entries and this function */
+	clear_cxt_table_entries(sdxi->l2_table, l1_table, cxt);
+}
+
+static struct sdxi_cxt *alloc_cxt(struct sdxi_dev *sdxi)
+{
+	struct sdxi_cxt *cxt;
+	u16 id, l2_idx, l1_idx;
+
+	if (sdxi->cxt_count >= sdxi->max_cxts)
+		return NULL;
+
+	/* search for an empty context slot */
+	for (id = 0; id < sdxi->max_cxts; id++) {
+		l2_idx = ID_TO_L2_INDEX(id);
+		l1_idx = ID_TO_L1_INDEX(id);
+
+		if (sdxi->cxt_array[l2_idx] == NULL) {
+			int sz = sizeof(struct sdxi_cxt *) * L1_TABLE_ENTRIES;
+			struct sdxi_cxt **ptr = kzalloc(sz, GFP_KERNEL);
+
+			sdxi->cxt_array[l2_idx] = ptr;
+			if (!(sdxi->cxt_array[l2_idx]))
+				return NULL;
+		}
+
+		cxt = (sdxi->cxt_array)[l2_idx][l1_idx];
+		/* found one empty slot */
+		if (!cxt)
+			break;
+	}
+
+	/* nothing found, bail... */
+	if (id == sdxi->max_cxts)
+		return NULL;
+
+	/* alloc context and initialize it */
+	cxt = kzalloc(sizeof(struct sdxi_cxt), GFP_KERNEL);
+	if (!cxt)
+		return NULL;
+
+	cxt->akey_table = dma_alloc_coherent(sdxi_to_dev(sdxi),
+					     sizeof(*cxt->akey_table),
+					     &cxt->akey_table_dma, GFP_KERNEL);
+	if (!cxt->akey_table) {
+		kfree(cxt);
+		return NULL;
+	}
+
+	cxt->sdxi = sdxi;
+	cxt->id = id;
+	cxt->db_base = sdxi->dbs_bar + id * sdxi->db_stride;
+	cxt->db = sdxi->dbs + id * sdxi->db_stride;
+
+	sdxi->cxt_array[l2_idx][l1_idx] = cxt;
+	sdxi->cxt_count++;
+
+	return cxt;
+}
+
+static void free_cxt(struct sdxi_cxt *cxt)
+{
+	struct sdxi_dev *sdxi = cxt->sdxi;
+	u16 l2_idx, l1_idx;
+
+	l2_idx = ID_TO_L2_INDEX(cxt->id);
+	l1_idx = ID_TO_L1_INDEX(cxt->id);
+
+	sdxi->cxt_count--;
+	dma_free_coherent(sdxi_to_dev(sdxi), sizeof(*cxt->akey_table),
+			  cxt->akey_table, cxt->akey_table_dma);
+	kfree(cxt);
+
+	(sdxi->cxt_array)[l2_idx][l1_idx] = NULL;
+}
+
+/* alloc context resources and populate context table */
+static struct sdxi_cxt *sdxi_cxt_alloc(struct sdxi_dev *sdxi)
+{
+	struct sdxi_cxt *cxt;
+
+	mutex_lock(&sdxi->cxt_lock);
+
+	cxt = alloc_cxt(sdxi);
+	if (!cxt)
+		goto drop_cxt_lock;
+
+	cxt->cxt_ctl = dma_pool_zalloc(sdxi->cxt_ctl_pool, GFP_KERNEL,
+				       &cxt->cxt_ctl_dma);
+	if (!cxt->cxt_ctl)
+		goto release_cxt;
+
+	if (config_cxt_tables(sdxi, cxt))
+		goto release_cxt_ctl;
+
+	mutex_unlock(&sdxi->cxt_lock);
+	return cxt;
+
+release_cxt_ctl:
+	dma_pool_free(sdxi->cxt_ctl_pool, cxt->cxt_ctl, cxt->cxt_ctl_dma);
+release_cxt:
+	free_cxt(cxt);
+drop_cxt_lock:
+	mutex_unlock(&sdxi->cxt_lock);
+	return NULL;
+}
+
+/* clear context table and free context resources */
+static void sdxi_cxt_free(struct sdxi_cxt *cxt)
+{
+	struct sdxi_dev *sdxi = cxt->sdxi;
+
+	mutex_lock(&sdxi->cxt_lock);
+
+	cleanup_cxt_tables(sdxi, cxt);
+	dma_pool_free(sdxi->cxt_ctl_pool, cxt->cxt_ctl, cxt->cxt_ctl_dma);
+	free_cxt(cxt);
+
+	mutex_unlock(&sdxi->cxt_lock);
+}
+
+struct sdxi_cxt *sdxi_working_cxt_init(struct sdxi_dev *sdxi,
+				       enum sdxi_cxt_id id)
+{
+	struct sdxi_cxt *cxt;
+	struct sdxi_sq *sq;
+
+	cxt = sdxi_cxt_alloc(sdxi);
+	if (!cxt) {
+		sdxi_err(sdxi, "failed to alloc a new context\n");
+		return NULL;
+	}
+
+	/* check if context ID matches */
+	if (id < SDXI_ANY_CXT_ID && cxt->id != id) {
+		sdxi_err(sdxi, "failed to alloc a context with id=%d\n", id);
+		goto err_cxt_id;
+	}
+
+	sq = sdxi_sq_alloc_default(cxt);
+	if (!sq) {
+		sdxi_err(sdxi, "failed to alloc a submission queue (sq)\n");
+		goto err_sq_alloc;
+	}
+
+	return cxt;
+
+err_sq_alloc:
+err_cxt_id:
+	sdxi_cxt_free(cxt);
+
+	return NULL;
+}
+
+static const char *cxt_sts_state_str(enum cxt_sts_state state)
+{
+	static const char *const context_states[] = {
+		[CXTV_STOP_SW]  = "stopped (software)",
+		[CXTV_RUN]      = "running",
+		[CXTV_STOPG_SW] = "stopping (software)",
+		[CXTV_STOP_FN]  = "stopped (function)",
+		[CXTV_STOPG_FN] = "stopping (function)",
+		[CXTV_ERR_FN]   = "error",
+	};
+	const char *str = "unknown";
+
+	switch (state) {
+	case CXTV_STOP_SW:
+	case CXTV_RUN:
+	case CXTV_STOPG_SW:
+	case CXTV_STOP_FN:
+	case CXTV_STOPG_FN:
+	case CXTV_ERR_FN:
+		str = context_states[state];
+	}
+
+	return str;
+}
+
+int sdxi_submit_desc(struct sdxi_cxt *cxt, const struct sdxi_desc *desc)
+{
+	struct sdxi_sq *sq;
+	__le64 __iomem *db;
+	__le64 *ring_base;
+	u64 ring_entries;
+	__le64 *rd_idx;
+	__le64 *wr_idx;
+
+	sq = cxt->sq;
+	ring_entries = sq->ring_entries;
+	ring_base = sq->desc_ring[0].qw;
+	rd_idx = &sq->cxt_sts->read_index;
+	wr_idx = sq->write_index;
+	db = cxt->db;
+
+	return sdxi_enqueue(desc->qw, 1, ring_base, ring_entries,
+			    rd_idx, wr_idx, db);
+}
+
+static void sdxi_cxt_shutdown(struct sdxi_cxt *target_cxt)
+{
+	unsigned long deadline = jiffies + msecs_to_jiffies(1000);
+	struct sdxi_cxt *admin_cxt = target_cxt->sdxi->admin_cxt;
+	struct sdxi_dev *sdxi = target_cxt->sdxi;
+	struct sdxi_cxt_sts *sts = target_cxt->sq->cxt_sts;
+	struct sdxi_desc desc;
+	u16 cxtid = target_cxt->id;
+	struct sdxi_cxt_stop params = {
+		.range = sdxi_cxt_range(cxtid),
+	};
+	enum cxt_sts_state state = sdxi_cxt_sts_state(sts);
+	int err;
+
+	sdxi_dbg(sdxi, "%s entry: context state: %s",
+		 __func__, cxt_sts_state_str(state));
+
+	err = sdxi_encode_cxt_stop(&desc, &params);
+	if (err)
+		return;
+
+	err = sdxi_submit_desc(admin_cxt, &desc);
+	if (err)
+		return;
+
+	sdxi_dbg(sdxi, "shutting down context %u\n", cxtid);
+
+	do {
+		enum cxt_sts_state state = sdxi_cxt_sts_state(sts);
+
+		sdxi_dbg(sdxi, "context %u state: %s", cxtid,
+			 cxt_sts_state_str(state));
+
+		switch (state) {
+		case CXTV_ERR_FN:
+			sdxi_err(sdxi, "context %u went into error state while stopping\n",
+				cxtid);
+			fallthrough;
+		case CXTV_STOP_SW:
+		case CXTV_STOP_FN:
+			return;
+		case CXTV_RUN:
+		case CXTV_STOPG_SW:
+		case CXTV_STOPG_FN:
+			/* transitional states */
+			fsleep(1000);
+			break;
+		default:
+			sdxi_err(sdxi, "context %u in unknown state %u\n",
+				 cxtid, state);
+			return;
+		}
+	} while (time_before(jiffies, deadline));
+
+	sdxi_err(sdxi, "stopping context %u timed out (state = %u)\n",
+		cxtid, sdxi_cxt_sts_state(sts));
+}
+
+void sdxi_working_cxt_exit(struct sdxi_cxt *cxt)
+{
+	struct sdxi_sq *sq;
+
+	if (!cxt)
+		return;
+
+	sq = cxt->sq;
+	if (!sq)
+		return;
+
+	sdxi_cxt_shutdown(cxt);
+
+	sdxi_sq_free(sq);
+
+	sdxi_cxt_free(cxt);
+}
diff --git a/drivers/dma/sdxi/context.h b/drivers/dma/sdxi/context.h
new file mode 100644
index 0000000000000000000000000000000000000000..0c4ab90ff3bd41d585c8b05ac95d51d7e1c82aa3
--- /dev/null
+++ b/drivers/dma/sdxi/context.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Header for sq and descriptor management
+ *
+ * Copyright (C) 2025 Advanced Micro Devices, Inc.
+ */
+
+#ifndef __SDXI_SQ_H
+#define __SDXI_SQ_H
+
+struct sdxi_cxt;
+struct sdxi_dev;
+struct sdxi_desc;
+
+enum sdxi_cxt_id {
+	SDXI_ADMIN_CXT_ID = 0,
+	SDXI_DMA_CXT_ID = 1,
+	SDXI_ANY_CXT_ID,
+};
+
+/* Context Control */
+struct sdxi_cxt *sdxi_working_cxt_init(struct sdxi_dev *sdxi,
+				       enum sdxi_cxt_id);
+void sdxi_working_cxt_exit(struct sdxi_cxt *cxt);
+
+int sdxi_submit_desc(struct sdxi_cxt *cxt, const struct sdxi_desc *desc);
+
+#endif /* __SDXI_SQ_H */

-- 
2.39.5

Re: [PATCH RFC 08/13] dmaengine: sdxi: Context creation/removal, descriptor submission

[Jonathan Cameron]

On Fri, 05 Sep 2025 13:48:31 -0500
Nathan Lynch via B4 Relay <devnull+nathan.lynch.amd.com@kernel.org> wrote:

> From: Nathan Lynch <nathan.lynch@amd.com>
> 
> Add functions for creating and removing SDXI contexts and submitting
> descriptors against them.
> 
> An SDXI function supports one or more contexts, each of which has its
> own descriptor ring and associated state. Each context has a 16-bit
> index. A special context is installed at index 0 and is used for
> configuring other contexts and performing administrative actions.
> 
> The creation of each context entails the allocation of the following
> control structure hierarchy:
> 
> * Context L1 Table slot
>   * Access key (AKey) table
>   * Context control block
>     * Descriptor ring
>     * Write index
>     * Context status block
> 
> Co-developed-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
Some superficial stuff inline.

I haven't yet reread the spec against this (and it's been a while
since I looked at sdxi!) but overall seems reasonable.
> ---
>  drivers/dma/sdxi/context.c | 547 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/dma/sdxi/context.h |  28 +++
>  2 files changed, 575 insertions(+)
> 
> diff --git a/drivers/dma/sdxi/context.c b/drivers/dma/sdxi/context.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..50eae5b3b303d67891113377e2df209d199aa533
> --- /dev/null
> +++ b/drivers/dma/sdxi/context.c
> @@ -0,0 +1,547 @@


> +
> +static struct sdxi_cxt *alloc_cxt(struct sdxi_dev *sdxi)
> +{
> +	struct sdxi_cxt *cxt;
> +	u16 id, l2_idx, l1_idx;
> +
> +	if (sdxi->cxt_count >= sdxi->max_cxts)
> +		return NULL;
> +
> +	/* search for an empty context slot */
> +	for (id = 0; id < sdxi->max_cxts; id++) {
> +		l2_idx = ID_TO_L2_INDEX(id);
> +		l1_idx = ID_TO_L1_INDEX(id);
> +
> +		if (sdxi->cxt_array[l2_idx] == NULL) {
> +			int sz = sizeof(struct sdxi_cxt *) * L1_TABLE_ENTRIES;
> +			struct sdxi_cxt **ptr = kzalloc(sz, GFP_KERNEL);
> +
> +			sdxi->cxt_array[l2_idx] = ptr;
> +			if (!(sdxi->cxt_array[l2_idx]))
> +				return NULL;
> +		}
> +
> +		cxt = (sdxi->cxt_array)[l2_idx][l1_idx];
> +		/* found one empty slot */
> +		if (!cxt)
> +			break;
> +	}
> +
> +	/* nothing found, bail... */
> +	if (id == sdxi->max_cxts)
> +		return NULL;
> +
> +	/* alloc context and initialize it */
> +	cxt = kzalloc(sizeof(struct sdxi_cxt), GFP_KERNEL);

sizeof(*ctx) usually preferred as it saves anyone checking types match.

> +	if (!cxt)
> +		return NULL;
> +
> +	cxt->akey_table = dma_alloc_coherent(sdxi_to_dev(sdxi),
> +					     sizeof(*cxt->akey_table),
> +					     &cxt->akey_table_dma, GFP_KERNEL);
> +	if (!cxt->akey_table) {
> +		kfree(cxt);

Similar to below. You could use a DEFINE_FREE() to auto cleanup up ctx
on error.

> +		return NULL;
> +	}
> +
> +	cxt->sdxi = sdxi;
> +	cxt->id = id;
> +	cxt->db_base = sdxi->dbs_bar + id * sdxi->db_stride;
> +	cxt->db = sdxi->dbs + id * sdxi->db_stride;
> +
> +	sdxi->cxt_array[l2_idx][l1_idx] = cxt;
> +	sdxi->cxt_count++;
> +
> +	return cxt;
> +}

> +struct sdxi_cxt *sdxi_working_cxt_init(struct sdxi_dev *sdxi,
> +				       enum sdxi_cxt_id id)
> +{
> +	struct sdxi_cxt *cxt;
> +	struct sdxi_sq *sq;
> +
> +	cxt = sdxi_cxt_alloc(sdxi);
> +	if (!cxt) {
> +		sdxi_err(sdxi, "failed to alloc a new context\n");
> +		return NULL;
> +	}
> +
> +	/* check if context ID matches */
> +	if (id < SDXI_ANY_CXT_ID && cxt->id != id) {
> +		sdxi_err(sdxi, "failed to alloc a context with id=%d\n", id);
> +		goto err_cxt_id;
> +	}
> +
> +	sq = sdxi_sq_alloc_default(cxt);
> +	if (!sq) {
> +		sdxi_err(sdxi, "failed to alloc a submission queue (sq)\n");
> +		goto err_sq_alloc;
> +	}
> +
> +	return cxt;
> +
> +err_sq_alloc:
> +err_cxt_id:
> +	sdxi_cxt_free(cxt);

Might be worth doing a DEFINE_FREE() then you can use return_ptr(ctx); instead
of return ctx.  Will allow you to simply return on any errors.

> +
> +	return NULL;
> +}
> +
> +static const char *cxt_sts_state_str(enum cxt_sts_state state)
> +{
> +	static const char *const context_states[] = {
> +		[CXTV_STOP_SW]  = "stopped (software)",
> +		[CXTV_RUN]      = "running",
> +		[CXTV_STOPG_SW] = "stopping (software)",
> +		[CXTV_STOP_FN]  = "stopped (function)",
> +		[CXTV_STOPG_FN] = "stopping (function)",
> +		[CXTV_ERR_FN]   = "error",
> +	};
> +	const char *str = "unknown";
> +
> +	switch (state) {
> +	case CXTV_STOP_SW:
> +	case CXTV_RUN:
> +	case CXTV_STOPG_SW:
> +	case CXTV_STOP_FN:
> +	case CXTV_STOPG_FN:
> +	case CXTV_ERR_FN:
> +		str = context_states[state];

I'd do a default to make it explicit that there are other states.  If there
aren't then just return here and skip the return below.  A compiler should
be able to see if you handled them all and complain loudly if a new one is
added that you haven't handled.

> +	}
> +
> +	return str;
> +}
> +
> +int sdxi_submit_desc(struct sdxi_cxt *cxt, const struct sdxi_desc *desc)
> +{
> +	struct sdxi_sq *sq;
> +	__le64 __iomem *db;
> +	__le64 *ring_base;
> +	u64 ring_entries;
> +	__le64 *rd_idx;
> +	__le64 *wr_idx;
> +
> +	sq = cxt->sq;
> +	ring_entries = sq->ring_entries;
> +	ring_base = sq->desc_ring[0].qw;
> +	rd_idx = &sq->cxt_sts->read_index;
> +	wr_idx = sq->write_index;
> +	db = cxt->db;
I'm not sure the local variables add anything, but if you really want
to keep them, then at least combine with declaration.

	struct sdxi_sq *sq = ctx->sq;
	__le64 __iomem *db = ctx->db;


just to keep thing code more compact.

Personally I'd just have a local sq and do the rest in the call

	return sdxi_enqueue(desc->qw, 1, sq->desc_ring[0].wq,
etc


> +
> +	return sdxi_enqueue(desc->qw, 1, ring_base, ring_entries,
> +			    rd_idx, wr_idx, db);
				
> +}
> +
> +static void sdxi_cxt_shutdown(struct sdxi_cxt *target_cxt)
> +{
> +	unsigned long deadline = jiffies + msecs_to_jiffies(1000);
> +	struct sdxi_cxt *admin_cxt = target_cxt->sdxi->admin_cxt;
> +	struct sdxi_dev *sdxi = target_cxt->sdxi;
> +	struct sdxi_cxt_sts *sts = target_cxt->sq->cxt_sts;
> +	struct sdxi_desc desc;
> +	u16 cxtid = target_cxt->id;
> +	struct sdxi_cxt_stop params = {
> +		.range = sdxi_cxt_range(cxtid),
> +	};
> +	enum cxt_sts_state state = sdxi_cxt_sts_state(sts);
> +	int err;
> +
> +	sdxi_dbg(sdxi, "%s entry: context state: %s",
> +		 __func__, cxt_sts_state_str(state));
> +
> +	err = sdxi_encode_cxt_stop(&desc, &params);
> +	if (err)
> +		return;
> +
> +	err = sdxi_submit_desc(admin_cxt, &desc);
> +	if (err)
> +		return;
> +
> +	sdxi_dbg(sdxi, "shutting down context %u\n", cxtid);
> +
> +	do {
> +		enum cxt_sts_state state = sdxi_cxt_sts_state(sts);
> +
> +		sdxi_dbg(sdxi, "context %u state: %s", cxtid,
> +			 cxt_sts_state_str(state));
> +
> +		switch (state) {
> +		case CXTV_ERR_FN:
> +			sdxi_err(sdxi, "context %u went into error state while stopping\n",
> +				cxtid);
> +			fallthrough;

I'd just return unless a later patch adds something more interesting to the next
cases.

> +		case CXTV_STOP_SW:
> +		case CXTV_STOP_FN:
> +			return;
> +		case CXTV_RUN:
> +		case CXTV_STOPG_SW:
> +		case CXTV_STOPG_FN:
> +			/* transitional states */
> +			fsleep(1000);
> +			break;
> +		default:
> +			sdxi_err(sdxi, "context %u in unknown state %u\n",
> +				 cxtid, state);
> +			return;
> +		}
> +	} while (time_before(jiffies, deadline));
> +
> +	sdxi_err(sdxi, "stopping context %u timed out (state = %u)\n",
> +		cxtid, sdxi_cxt_sts_state(sts));
> +}
> +
> +void sdxi_working_cxt_exit(struct sdxi_cxt *cxt)
> +{
> +	struct sdxi_sq *sq;
> +
> +	if (!cxt)
Superficially this looks like defensive programming that we don't need
as it makes not sense to call this if ctx is NULL.  Add a comment if
there is a path where this actually happens.
> +		return;
> +
> +	sq = cxt->sq;
> +	if (!sq)
Add a comment on why this might happen, or drop teh cehck.

> +		return;
> +
> +	sdxi_cxt_shutdown(cxt);
> +
> +	sdxi_sq_free(sq);
> +
> +	sdxi_cxt_free(cxt);
> +}

Re: [PATCH RFC 08/13] dmaengine: sdxi: Context creation/removal, descriptor submission

[Nathan Lynch]

Jonathan Cameron <jonathan.cameron@huawei.com> writes:
> On Fri, 05 Sep 2025 13:48:31 -0500
> Nathan Lynch via B4 Relay <devnull+nathan.lynch.amd.com@kernel.org> wrote:
>
>> From: Nathan Lynch <nathan.lynch@amd.com>
>> 
>> Add functions for creating and removing SDXI contexts and submitting
>> descriptors against them.
>> 
>> An SDXI function supports one or more contexts, each of which has its
>> own descriptor ring and associated state. Each context has a 16-bit
>> index. A special context is installed at index 0 and is used for
>> configuring other contexts and performing administrative actions.
>> 
>> The creation of each context entails the allocation of the following
>> control structure hierarchy:
>> 
>> * Context L1 Table slot
>>   * Access key (AKey) table
>>   * Context control block
>>     * Descriptor ring
>>     * Write index
>>     * Context status block
>> 
>> Co-developed-by: Wei Huang <wei.huang2@amd.com>
>> Signed-off-by: Wei Huang <wei.huang2@amd.com>
>> Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
> Some superficial stuff inline.
>
> I haven't yet reread the spec against this (and it's been a while
> since I looked at sdxi!) but overall seems reasonable.
>> ---
>>  drivers/dma/sdxi/context.c | 547 +++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/dma/sdxi/context.h |  28 +++
>>  2 files changed, 575 insertions(+)
>> 
>> diff --git a/drivers/dma/sdxi/context.c b/drivers/dma/sdxi/context.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..50eae5b3b303d67891113377e2df209d199aa533
>> --- /dev/null
>> +++ b/drivers/dma/sdxi/context.c
>> @@ -0,0 +1,547 @@
>
>
>> +
>> +static struct sdxi_cxt *alloc_cxt(struct sdxi_dev *sdxi)
>> +{
>> +	struct sdxi_cxt *cxt;
>> +	u16 id, l2_idx, l1_idx;
>> +
>> +	if (sdxi->cxt_count >= sdxi->max_cxts)
>> +		return NULL;
>> +
>> +	/* search for an empty context slot */
>> +	for (id = 0; id < sdxi->max_cxts; id++) {
>> +		l2_idx = ID_TO_L2_INDEX(id);
>> +		l1_idx = ID_TO_L1_INDEX(id);
>> +
>> +		if (sdxi->cxt_array[l2_idx] == NULL) {
>> +			int sz = sizeof(struct sdxi_cxt *) * L1_TABLE_ENTRIES;
>> +			struct sdxi_cxt **ptr = kzalloc(sz, GFP_KERNEL);
>> +
>> +			sdxi->cxt_array[l2_idx] = ptr;
>> +			if (!(sdxi->cxt_array[l2_idx]))
>> +				return NULL;
>> +		}
>> +
>> +		cxt = (sdxi->cxt_array)[l2_idx][l1_idx];
>> +		/* found one empty slot */
>> +		if (!cxt)
>> +			break;
>> +	}
>> +
>> +	/* nothing found, bail... */
>> +	if (id == sdxi->max_cxts)
>> +		return NULL;
>> +
>> +	/* alloc context and initialize it */
>> +	cxt = kzalloc(sizeof(struct sdxi_cxt), GFP_KERNEL);
>
> sizeof(*ctx) usually preferred as it saves anyone checking types
> match.

yep, will fix.


>> +	if (!cxt)
>> +		return NULL;
>> +
>> +	cxt->akey_table = dma_alloc_coherent(sdxi_to_dev(sdxi),
>> +					     sizeof(*cxt->akey_table),
>> +					     &cxt->akey_table_dma, GFP_KERNEL);
>> +	if (!cxt->akey_table) {
>> +		kfree(cxt);
>
> Similar to below. You could use a DEFINE_FREE() to auto cleanup up ctx
> on error.

OK, I've been hesitant to try the cleanup stuff so far but I'll give it
a shot (here and other places).


>> +		return NULL;
>> +	}
>> +
>> +	cxt->sdxi = sdxi;
>> +	cxt->id = id;
>> +	cxt->db_base = sdxi->dbs_bar + id * sdxi->db_stride;
>> +	cxt->db = sdxi->dbs + id * sdxi->db_stride;
>> +
>> +	sdxi->cxt_array[l2_idx][l1_idx] = cxt;
>> +	sdxi->cxt_count++;
>> +
>> +	return cxt;
>> +}
>
>> +struct sdxi_cxt *sdxi_working_cxt_init(struct sdxi_dev *sdxi,
>> +				       enum sdxi_cxt_id id)
>> +{
>> +	struct sdxi_cxt *cxt;
>> +	struct sdxi_sq *sq;
>> +
>> +	cxt = sdxi_cxt_alloc(sdxi);
>> +	if (!cxt) {
>> +		sdxi_err(sdxi, "failed to alloc a new context\n");
>> +		return NULL;
>> +	}
>> +
>> +	/* check if context ID matches */
>> +	if (id < SDXI_ANY_CXT_ID && cxt->id != id) {
>> +		sdxi_err(sdxi, "failed to alloc a context with id=%d\n", id);
>> +		goto err_cxt_id;
>> +	}
>> +
>> +	sq = sdxi_sq_alloc_default(cxt);
>> +	if (!sq) {
>> +		sdxi_err(sdxi, "failed to alloc a submission queue (sq)\n");
>> +		goto err_sq_alloc;
>> +	}
>> +
>> +	return cxt;
>> +
>> +err_sq_alloc:
>> +err_cxt_id:
>> +	sdxi_cxt_free(cxt);
>
> Might be worth doing a DEFINE_FREE() then you can use return_ptr(ctx); instead
> of return ctx.  Will allow you to simply return on any errors.
>
>> +
>> +	return NULL;
>> +}
>> +
>> +static const char *cxt_sts_state_str(enum cxt_sts_state state)
>> +{
>> +	static const char *const context_states[] = {
>> +		[CXTV_STOP_SW]  = "stopped (software)",
>> +		[CXTV_RUN]      = "running",
>> +		[CXTV_STOPG_SW] = "stopping (software)",
>> +		[CXTV_STOP_FN]  = "stopped (function)",
>> +		[CXTV_STOPG_FN] = "stopping (function)",
>> +		[CXTV_ERR_FN]   = "error",
>> +	};
>> +	const char *str = "unknown";
>> +
>> +	switch (state) {
>> +	case CXTV_STOP_SW:
>> +	case CXTV_RUN:
>> +	case CXTV_STOPG_SW:
>> +	case CXTV_STOP_FN:
>> +	case CXTV_STOPG_FN:
>> +	case CXTV_ERR_FN:
>> +		str = context_states[state];
>
> I'd do a default to make it explicit that there are other states. If
> there aren't then just return here and skip the return below. A
> compiler should be able to see if you handled them all and complain
> loudly if a new one is added that you haven't handled.

The CXTV_... values are the only valid states that an SDXI device is
allowed to report for a context, but this function is intended to be
resilient against unspecified values in case of implementation bugs (in
the caller, or firmware, whatever). That's why it falls back to
returning "unknown".

But it's coded without a default label so that -Wswitch (which is
enabled by -Wall and so is generally active for kernel code) will warn
on an unhandled case. The presence of a default label will actually
defeat this unless the compiler is invoked with -Wswitch-enum, which
even W=1 doesn't enable.

I really do want warnings on unhandled cases of this sort, so I suppose
at the very least this code deserves a comment to deter well-meaning
people from trying to add a default label. Or I could add the default
label and see how painful it is to use -Wswitch-enum throughout the
driver. There are several similar functions in the error reporting code
so this isn't the only instance of this pattern in the driver.


>> +	}
>> +
>> +	return str;
>> +}
>> +
>> +int sdxi_submit_desc(struct sdxi_cxt *cxt, const struct sdxi_desc *desc)
>> +{
>> +	struct sdxi_sq *sq;
>> +	__le64 __iomem *db;
>> +	__le64 *ring_base;
>> +	u64 ring_entries;
>> +	__le64 *rd_idx;
>> +	__le64 *wr_idx;
>> +
>> +	sq = cxt->sq;
>> +	ring_entries = sq->ring_entries;
>> +	ring_base = sq->desc_ring[0].qw;
>> +	rd_idx = &sq->cxt_sts->read_index;
>> +	wr_idx = sq->write_index;
>> +	db = cxt->db;
> I'm not sure the local variables add anything, but if you really want
> to keep them, then at least combine with declaration.
>
> 	struct sdxi_sq *sq = ctx->sq;
> 	__le64 __iomem *db = ctx->db;
>
>
> just to keep thing code more compact.
>
> Personally I'd just have a local sq and do the rest in the call
>
> 	return sdxi_enqueue(desc->qw, 1, sq->desc_ring[0].wq,
> etc

Yeah, that makes sense.

>
>
>> +
>> +	return sdxi_enqueue(desc->qw, 1, ring_base, ring_entries,
>> +			    rd_idx, wr_idx, db);
> 				
>> +}
>> +
>> +static void sdxi_cxt_shutdown(struct sdxi_cxt *target_cxt)
>> +{
>> +	unsigned long deadline = jiffies + msecs_to_jiffies(1000);
>> +	struct sdxi_cxt *admin_cxt = target_cxt->sdxi->admin_cxt;
>> +	struct sdxi_dev *sdxi = target_cxt->sdxi;
>> +	struct sdxi_cxt_sts *sts = target_cxt->sq->cxt_sts;
>> +	struct sdxi_desc desc;
>> +	u16 cxtid = target_cxt->id;
>> +	struct sdxi_cxt_stop params = {
>> +		.range = sdxi_cxt_range(cxtid),
>> +	};
>> +	enum cxt_sts_state state = sdxi_cxt_sts_state(sts);
>> +	int err;
>> +
>> +	sdxi_dbg(sdxi, "%s entry: context state: %s",
>> +		 __func__, cxt_sts_state_str(state));
>> +
>> +	err = sdxi_encode_cxt_stop(&desc, &params);
>> +	if (err)
>> +		return;
>> +
>> +	err = sdxi_submit_desc(admin_cxt, &desc);
>> +	if (err)
>> +		return;
>> +
>> +	sdxi_dbg(sdxi, "shutting down context %u\n", cxtid);
>> +
>> +	do {
>> +		enum cxt_sts_state state = sdxi_cxt_sts_state(sts);
>> +
>> +		sdxi_dbg(sdxi, "context %u state: %s", cxtid,
>> +			 cxt_sts_state_str(state));
>> +
>> +		switch (state) {
>> +		case CXTV_ERR_FN:
>> +			sdxi_err(sdxi, "context %u went into error state while stopping\n",
>> +				cxtid);
>> +			fallthrough;
>
> I'd just return unless a later patch adds something more interesting to the next
> cases.

Agreed.


>> +		case CXTV_STOP_SW:
>> +		case CXTV_STOP_FN:
>> +			return;
>> +		case CXTV_RUN:
>> +		case CXTV_STOPG_SW:
>> +		case CXTV_STOPG_FN:
>> +			/* transitional states */
>> +			fsleep(1000);
>> +			break;
>> +		default:
>> +			sdxi_err(sdxi, "context %u in unknown state %u\n",
>> +				 cxtid, state);
>> +			return;
>> +		}
>> +	} while (time_before(jiffies, deadline));
>> +
>> +	sdxi_err(sdxi, "stopping context %u timed out (state = %u)\n",
>> +		cxtid, sdxi_cxt_sts_state(sts));
>> +}
>> +
>> +void sdxi_working_cxt_exit(struct sdxi_cxt *cxt)
>> +{
>> +	struct sdxi_sq *sq;
>> +
>> +	if (!cxt)
> Superficially this looks like defensive programming that we don't need
> as it makes not sense to call this if ctx is NULL.  Add a comment if
> there is a path where this actually happens.
>> +		return;
>> +
>> +	sq = cxt->sq;
>> +	if (!sq)
> Add a comment on why this might happen, or drop teh cehck.

Yeah we can probably remove these checks.

Re: [PATCH RFC 08/13] dmaengine: sdxi: Context creation/removal, descriptor submission

[Jonathan Cameron]

> >> +static const char *cxt_sts_state_str(enum cxt_sts_state state)
> >> +{
> >> +	static const char *const context_states[] = {
> >> +		[CXTV_STOP_SW]  = "stopped (software)",
> >> +		[CXTV_RUN]      = "running",
> >> +		[CXTV_STOPG_SW] = "stopping (software)",
> >> +		[CXTV_STOP_FN]  = "stopped (function)",
> >> +		[CXTV_STOPG_FN] = "stopping (function)",
> >> +		[CXTV_ERR_FN]   = "error",
> >> +	};
> >> +	const char *str = "unknown";
> >> +
> >> +	switch (state) {
> >> +	case CXTV_STOP_SW:
> >> +	case CXTV_RUN:
> >> +	case CXTV_STOPG_SW:
> >> +	case CXTV_STOP_FN:
> >> +	case CXTV_STOPG_FN:
> >> +	case CXTV_ERR_FN:
> >> +		str = context_states[state];  
> >
> > I'd do a default to make it explicit that there are other states. If
> > there aren't then just return here and skip the return below. A
> > compiler should be able to see if you handled them all and complain
> > loudly if a new one is added that you haven't handled.  
> 
> The CXTV_... values are the only valid states that an SDXI device is
> allowed to report for a context, but this function is intended to be
> resilient against unspecified values in case of implementation bugs (in
> the caller, or firmware, whatever). That's why it falls back to
> returning "unknown".
> 
> But it's coded without a default label so that -Wswitch (which is
> enabled by -Wall and so is generally active for kernel code) will warn
> on an unhandled case. The presence of a default label will actually
> defeat this unless the compiler is invoked with -Wswitch-enum, which
> even W=1 doesn't enable.
> 
> I really do want warnings on unhandled cases of this sort, so I suppose
> at the very least this code deserves a comment to deter well-meaning
> people from trying to add a default label. Or I could add the default
> label and see how painful it is to use -Wswitch-enum throughout the
> driver. There are several similar functions in the error reporting code
> so this isn't the only instance of this pattern in the driver.

Thanks for the response. Makes sense.

J

Re: [PATCH RFC 08/13] dmaengine: sdxi: Context creation/removal, descriptor submission

[Markus Elfring]

…
> +++ b/drivers/dma/sdxi/context.c
> @@ -0,0 +1,547 @@
…
> +static void sdxi_cxt_free(struct sdxi_cxt *cxt)
> +{
> +	struct sdxi_dev *sdxi = cxt->sdxi;
> +
> +	mutex_lock(&sdxi->cxt_lock);
> +
> +	cleanup_cxt_tables(sdxi, cxt);
> +	dma_pool_free(sdxi->cxt_ctl_pool, cxt->cxt_ctl, cxt->cxt_ctl_dma);
> +	free_cxt(cxt);
> +
> +	mutex_unlock(&sdxi->cxt_lock);
> +}
…

Under which circumstances would you become interested to apply a statement
like “guard(mutex)(&sdxi->cxt_lock);”?
https://elixir.bootlin.com/linux/v6.17-rc6/source/include/linux/mutex.h#L228

Regards,
Markus

[PATCH RFC 09/13] dmaengine: sdxi: Add core device management code

[Nathan Lynch via B4 Relay]

From: Nathan Lynch <nathan.lynch@amd.com>

Add code that manages device initialization and exit and provides
entry points for the PCI driver code to come.

Co-developed-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
---
 drivers/dma/sdxi/device.c | 397 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 397 insertions(+)

diff --git a/drivers/dma/sdxi/device.c b/drivers/dma/sdxi/device.c
new file mode 100644
index 0000000000000000000000000000000000000000..61123bc1d47b6547538b6e783ad96a9c2851494e
--- /dev/null
+++ b/drivers/dma/sdxi/device.c
@@ -0,0 +1,397 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * SDXI hardware device driver
+ *
+ * Copyright (C) 2025 Advanced Micro Devices, Inc.
+ */
+
+#include <asm/mmu.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/dma-direction.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmapool.h>
+#include <linux/log2.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pci.h>
+
+#include "context.h"
+#include "descriptor.h"
+#include "hw.h"
+#include "error.h"
+#include "sdxi.h"
+
+enum sdxi_fn_gsv {
+	SDXI_GSV_STOP,
+	SDXI_GSV_INIT,
+	SDXI_GSV_ACTIVE,
+	SDXI_GSV_STOPG_SF,
+	SDXI_GSV_STOPG_HD,
+	SDXI_GSV_ERROR,
+};
+
+static const char *const gsv_strings[] = {
+	[SDXI_GSV_STOP]     = "stopped",
+	[SDXI_GSV_INIT]     = "initializing",
+	[SDXI_GSV_ACTIVE]   = "active",
+	[SDXI_GSV_STOPG_SF] = "soft stopping",
+	[SDXI_GSV_STOPG_HD] = "hard stopping",
+	[SDXI_GSV_ERROR]    = "error",
+};
+
+static const char *gsv_str(enum sdxi_fn_gsv gsv)
+{
+	if ((size_t)gsv < ARRAY_SIZE(gsv_strings))
+		return gsv_strings[(size_t)gsv];
+
+	WARN_ONCE(1, "unexpected gsv %u\n", gsv);
+
+	return "unknown";
+}
+
+enum sdxi_fn_gsr {
+	SDXI_GSRV_RESET,
+	SDXI_GSRV_STOP_SF,
+	SDXI_GSRV_STOP_HD,
+	SDXI_GSRV_ACTIVE,
+};
+
+static enum sdxi_fn_gsv sdxi_dev_gsv(const struct sdxi_dev *sdxi)
+{
+	return (enum sdxi_fn_gsv)FIELD_GET(SDXI_MMIO_STS0_FN_GSV,
+					sdxi_read64(sdxi, SDXI_MMIO_STS0));
+}
+
+static void sdxi_write_fn_gsr(struct sdxi_dev *sdxi, enum sdxi_fn_gsr cmd)
+{
+	u64 ctl0 = sdxi_read64(sdxi, SDXI_MMIO_CTL0);
+
+	FIELD_MODIFY(SDXI_MMIO_CTL0_FN_GSR, &ctl0, cmd);
+	sdxi_write64(sdxi, SDXI_MMIO_CTL0, ctl0);
+}
+
+static int sdxi_dev_start(struct sdxi_dev *sdxi)
+{
+	unsigned long deadline;
+	enum sdxi_fn_gsv status;
+
+	status = sdxi_dev_gsv(sdxi);
+	if (status != SDXI_GSV_STOP) {
+		sdxi_err(sdxi,
+			 "can't activate busy device (unexpected gsv: %s)\n",
+			 gsv_str(status));
+		return -EIO;
+	}
+
+	sdxi_write_fn_gsr(sdxi, SDXI_GSRV_ACTIVE);
+
+	deadline = jiffies + msecs_to_jiffies(1000);
+	do {
+		status = sdxi_dev_gsv(sdxi);
+		sdxi_dbg(sdxi, "%s: function state: %s\n", __func__, gsv_str(status));
+
+		switch (status) {
+		case SDXI_GSV_ACTIVE:
+			sdxi_dbg(sdxi, "activated\n");
+			return 0;
+		case SDXI_GSV_ERROR:
+			sdxi_err(sdxi, "went to error state\n");
+			return -EIO;
+		case SDXI_GSV_INIT:
+		case SDXI_GSV_STOP:
+			/* transitional states, wait */
+			fsleep(1000);
+			break;
+		default:
+			sdxi_err(sdxi, "unexpected gsv %u, giving up\n", status);
+			return -EIO;
+		}
+	} while (time_before(jiffies, deadline));
+
+	sdxi_err(sdxi, "activation timed out, current status %u\n",
+		sdxi_dev_gsv(sdxi));
+	return -ETIMEDOUT;
+
+}
+
+/* Get the device to the GSV_STOP state. */
+static int sdxi_dev_stop(struct sdxi_dev *sdxi)
+{
+	unsigned long deadline = jiffies + msecs_to_jiffies(1000);
+	bool reset_issued = false;
+
+	do {
+		enum sdxi_fn_gsv status = sdxi_dev_gsv(sdxi);
+
+		sdxi_dbg(sdxi, "%s: function state: %s\n", __func__, gsv_str(status));
+
+		switch (status) {
+		case SDXI_GSV_ACTIVE:
+			sdxi_write_fn_gsr(sdxi, SDXI_GSRV_STOP_SF);
+			break;
+		case SDXI_GSV_ERROR:
+			if (!reset_issued) {
+				sdxi_info(sdxi,
+					  "function in error state, issuing reset\n");
+				sdxi_write_fn_gsr(sdxi, SDXI_GSRV_RESET);
+				reset_issued = true;
+			} else {
+				fsleep(1000);
+			}
+			break;
+		case SDXI_GSV_STOP:
+			return 0;
+		case SDXI_GSV_INIT:
+		case SDXI_GSV_STOPG_SF:
+		case SDXI_GSV_STOPG_HD:
+			/* transitional states, wait */
+			sdxi_dbg(sdxi, "waiting for stop (gsv = %u)\n",
+				 status);
+			fsleep(1000);
+			break;
+		default:
+			sdxi_err(sdxi, "unknown gsv %u, giving up\n", status);
+			return -EIO;
+		}
+	} while (time_before(jiffies, deadline));
+
+	sdxi_err(sdxi, "stop attempt timed out, current status %u\n",
+		sdxi_dev_gsv(sdxi));
+	return -ETIMEDOUT;
+}
+
+static void sdxi_stop(struct sdxi_dev *sdxi)
+{
+	sdxi_dev_stop(sdxi);
+}
+
+/* Refer to "Activation of the SDXI Function by Software". */
+static int sdxi_fn_activate(struct sdxi_dev *sdxi)
+{
+	const struct sdxi_dev_ops *ops = sdxi->dev_ops;
+	u64 cxt_l2;
+	u64 cap0;
+	u64 cap1;
+	u64 ctl2;
+	int err;
+
+	/*
+	 * Clear any existing configuration from MMIO_CTL0 and ensure
+	 * the function is in GSV_STOP state.
+	 */
+	sdxi_write64(sdxi, SDXI_MMIO_CTL0, 0);
+	err = sdxi_dev_stop(sdxi);
+	if (err)
+		return err;
+
+	/*
+	 * 1.a. Discover limits and implemented features via MMIO_CAP0
+	 * and MMIO_CAP1.
+	 */
+	cap0 = sdxi_read64(sdxi, SDXI_MMIO_CAP0);
+	sdxi->sfunc = FIELD_GET(SDXI_MMIO_CAP0_SFUNC, cap0);
+	sdxi->max_ring_entries = SZ_1K;
+	sdxi->max_ring_entries *= 1U << FIELD_GET(SDXI_MMIO_CAP0_MAX_DS_RING_SZ, cap0);
+	sdxi->db_stride = SZ_4K;
+	sdxi->db_stride *= 1U << FIELD_GET(SDXI_MMIO_CAP0_DB_STRIDE, cap0);
+
+	cap1 = sdxi_read64(sdxi, SDXI_MMIO_CAP1);
+	sdxi->max_akeys = SZ_256;
+	sdxi->max_akeys *= 1U << FIELD_GET(SDXI_MMIO_CAP1_MAX_AKEY_SZ, cap1);
+	sdxi->max_cxts = 1 + FIELD_GET(SDXI_MMIO_CAP1_MAX_CXT, cap1);
+	sdxi->op_grp_cap = FIELD_GET(SDXI_MMIO_CAP1_OPB_000_CAP, cap1);
+
+	/*
+	 * 1.b. Configure SDXI parameters via MMIO_CTL2. We don't have
+	 * any reason to impose more conservative limits than those
+	 * reported in the capability registers, so use those for now.
+	 */
+	ctl2 = 0;
+	ctl2 |= FIELD_PREP(SDXI_MMIO_CTL2_MAX_BUFFER,
+			   FIELD_GET(SDXI_MMIO_CAP1_MAX_BUFFER, cap1));
+	ctl2 |= FIELD_PREP(SDXI_MMIO_CTL2_MAX_AKEY_SZ,
+			   FIELD_GET(SDXI_MMIO_CAP1_MAX_AKEY_SZ, cap1));
+	ctl2 |= FIELD_PREP(SDXI_MMIO_CTL2_MAX_CXT,
+			   FIELD_GET(SDXI_MMIO_CAP1_MAX_CXT, cap1));
+	ctl2 |= FIELD_PREP(SDXI_MMIO_CTL2_OPB_000_AVL,
+			   FIELD_GET(SDXI_MMIO_CAP1_OPB_000_CAP, cap1));
+	sdxi_write64(sdxi, SDXI_MMIO_CTL2, ctl2);
+
+	sdxi_dbg(sdxi,
+		 "sfunc:%#x descmax:%llu dbstride:%#x akeymax:%u cxtmax:%u opgrps:%#x\n",
+		 sdxi->sfunc, sdxi->max_ring_entries, sdxi->db_stride,
+		 sdxi->max_akeys, sdxi->max_cxts, sdxi->op_grp_cap);
+
+	/* 2.a-2.b. Allocate and zero the 4KB Context Level 2 Table */
+	sdxi->l2_table = dmam_alloc_coherent(sdxi_to_dev(sdxi), L2_TABLE_SIZE,
+					     &sdxi->l2_dma, GFP_KERNEL);
+	if (!sdxi->l2_table)
+		return -ENOMEM;
+
+	/* 2.c. Program MMIO_CXT_L2 */
+	cxt_l2 = FIELD_PREP(SDXI_MMIO_CXT_L2_PTR, sdxi->l2_dma >> ilog2(SZ_4K));
+	sdxi_write64(sdxi, SDXI_MMIO_CXT_L2, cxt_l2);
+
+	/*
+	 * 2.c.i. TODO: Program MMIO_CTL0.fn_pasid and
+	 * MMIO_CTL0.fn_pasid if guest virtual addressing required.
+	 */
+
+	/*
+	 * This covers the following steps:
+	 *
+	 * 3. Context Level 1 Table Setup for contexts 0..127.
+	 * 4.a. Create the administrative context and associated control
+	 * structures.
+	 * 4.b. Set its CXT_STS.state to CXTV_RUN; see 10.b.
+	 *
+	 * The admin context will not consume descriptors until we
+	 * write its doorbell later.
+	 */
+	sdxi->admin_cxt = sdxi_working_cxt_init(sdxi, SDXI_ADMIN_CXT_ID);
+	if (!sdxi->admin_cxt)
+		return -ENOMEM;
+	/*
+	 * 5. Mailbox: we don't use this facility and we assume the
+	 * reset values are sane.
+	 *
+	 * 6. If restoring saved state, adjust as appropriate. (We're not.)
+	 */
+
+	/*
+	 * MSI allocation is informed by the function's maximum
+	 * supported contexts, which was discovered in 1.a. Need to do
+	 * this before step 7, which claims an IRQ.
+	 */
+	err = (ops && ops->irq_init) ? ops->irq_init(sdxi) : 0;
+	if (err)
+		goto admin_cxt_exit;
+
+	/* 7. Initialize error log according to "Error Log Initialization". */
+	err = sdxi_error_init(sdxi);
+	if (err)
+		goto irq_exit;
+
+	/*
+	 * 8. "Software may also need to configure and enable
+	 * additional [features]". We've already performed MSI setup,
+	 * nothing else for us to do here for now.
+	 */
+
+	/*
+	 * 9. Set MMIO_CTL0.fn_gsr to GSRV_ACTIVE and wait for
+	 * MMIO_STS0.fn_gsv to reach GSV_ACTIVE or GSV_ERROR.
+	 */
+	err = sdxi_dev_start(sdxi);
+	if (err)
+		goto error_exit;
+
+	/*
+	 * 10. Jump start the admin context. This step refers to
+	 * "Starting A context and Context Signaling," where method #3
+	 * recommends writing an "appropriate" value to the doorbell
+	 * register. We haven't queued any descriptors to the admin
+	 * context at this point, so the appropriate value would be 0.
+	 */
+	iowrite64(0, sdxi->admin_cxt->db);
+
+	return 0;
+
+error_exit:
+	sdxi_error_exit(sdxi);
+irq_exit:
+	if (ops && ops->irq_exit)
+		ops->irq_exit(sdxi);
+admin_cxt_exit:
+	sdxi_working_cxt_exit(sdxi->admin_cxt);
+	return err;
+}
+
+int sdxi_device_init(struct sdxi_dev *sdxi, const struct sdxi_dev_ops *ops)
+{
+	struct sdxi_cxt_start params;
+	struct sdxi_cxt *admin_cxt;
+	struct sdxi_desc desc;
+	struct sdxi_cxt *dma_cxt;
+	int err;
+
+	sdxi->dev_ops = ops;
+
+	sdxi->write_index_pool = dmam_pool_create("Write_Index", sdxi_to_dev(sdxi),
+						  sizeof(__le64), sizeof(__le64), 0);
+	sdxi->cxt_sts_pool = dmam_pool_create("CXT_STS", sdxi_to_dev(sdxi),
+					      sizeof(struct sdxi_cxt_sts),
+					      sizeof(struct sdxi_cxt_sts), 0);
+	sdxi->cxt_ctl_pool = dmam_pool_create("CXT_CTL", sdxi_to_dev(sdxi),
+					      sizeof(struct sdxi_cxt_ctl),
+					      sizeof(struct sdxi_cxt_ctl), 0);
+	if (!sdxi->write_index_pool || !sdxi->cxt_sts_pool || !sdxi->cxt_ctl_pool)
+		return -ENOMEM;
+
+	err = sdxi_fn_activate(sdxi);
+	if (err)
+		return err;
+
+	admin_cxt = sdxi->admin_cxt;
+
+	dma_cxt = sdxi_working_cxt_init(sdxi, SDXI_DMA_CXT_ID);
+	if (!dma_cxt) {
+		err = -EINVAL;
+		goto fn_stop;
+	}
+
+	sdxi->dma_cxt = dma_cxt;
+
+	params = (typeof(params)) {
+		.range = sdxi_cxt_range(dma_cxt->id),
+	};
+	err = sdxi_encode_cxt_start(&desc, &params);
+	if (err)
+		goto fn_stop;
+
+	err = sdxi_submit_desc(admin_cxt, &desc);
+	if (err)
+		goto fn_stop;
+
+	return 0;
+fn_stop:
+	sdxi_stop(sdxi);
+	return err;
+}
+
+void sdxi_device_exit(struct sdxi_dev *sdxi)
+{
+	sdxi_working_cxt_exit(sdxi->dma_cxt);
+
+	/* Walk sdxi->cxt_array freeing any allocated rows. */
+	for (size_t i = 0; i < L2_TABLE_ENTRIES; ++i) {
+		if (!sdxi->cxt_array[i])
+			continue;
+		/* When a context is released its entry in the table should be NULL. */
+		for (size_t j = 0; j < L1_TABLE_ENTRIES; ++j) {
+			struct sdxi_cxt *cxt = sdxi->cxt_array[i][j];
+
+			if (!cxt)
+				continue;
+			if (cxt->id != 0)  /* admin context shutdown is last */
+				sdxi_working_cxt_exit(cxt);
+			sdxi->cxt_array[i][j] = NULL;
+		}
+		if (i != 0)  /* another special case for admin cxt */
+			kfree(sdxi->cxt_array[i]);
+	}
+
+	sdxi_working_cxt_exit(sdxi->admin_cxt);
+	kfree(sdxi->cxt_array[0]);  /* ugh */
+
+	sdxi_stop(sdxi);
+	sdxi_error_exit(sdxi);
+	if (sdxi->dev_ops && sdxi->dev_ops->irq_exit)
+		sdxi->dev_ops->irq_exit(sdxi);
+}
+
+MODULE_DESCRIPTION("SDXI driver");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Wei Huang");
+MODULE_AUTHOR("Nathan Lynch");

-- 
2.39.5

Re: [PATCH RFC 09/13] dmaengine: sdxi: Add core device management code

[Jonathan Cameron]

On Fri, 05 Sep 2025 13:48:32 -0500
Nathan Lynch via B4 Relay <devnull+nathan.lynch.amd.com@kernel.org> wrote:

> From: Nathan Lynch <nathan.lynch@amd.com>
> 
> Add code that manages device initialization and exit and provides
> entry points for the PCI driver code to come.

I'd prefer a patch series that started with the PCI device and built up
functionality for the stuff found earlier + in this patch on top of it.
Doing that allows each patch to be fully tested and reviewed on it's own.

However not my driver or subsystem so up to others on whether
they care!

One request for more info inline.

> 
> Co-developed-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
> ---


> +
> +/* Refer to "Activation of the SDXI Function by Software". */
> +static int sdxi_fn_activate(struct sdxi_dev *sdxi)
> +{
> +	const struct sdxi_dev_ops *ops = sdxi->dev_ops;
> +	u64 cxt_l2;
> +	u64 cap0;
> +	u64 cap1;
> +	u64 ctl2;

Combine these u64 declarations on one line.

> +	int err;
> +
> +	/*
> +	 * Clear any existing configuration from MMIO_CTL0 and ensure
> +	 * the function is in GSV_STOP state.
> +	 */
> +	sdxi_write64(sdxi, SDXI_MMIO_CTL0, 0);
> +	err = sdxi_dev_stop(sdxi);
> +	if (err)
> +		return err;
> +
> +	/*
> +	 * 1.a. Discover limits and implemented features via MMIO_CAP0
> +	 * and MMIO_CAP1.
> +	 */
> +	cap0 = sdxi_read64(sdxi, SDXI_MMIO_CAP0);
> +	

> +
> +void sdxi_device_exit(struct sdxi_dev *sdxi)
> +{
> +	sdxi_working_cxt_exit(sdxi->dma_cxt);
> +
> +	/* Walk sdxi->cxt_array freeing any allocated rows. */
> +	for (size_t i = 0; i < L2_TABLE_ENTRIES; ++i) {
> +		if (!sdxi->cxt_array[i])
> +			continue;
> +		/* When a context is released its entry in the table should be NULL. */
> +		for (size_t j = 0; j < L1_TABLE_ENTRIES; ++j) {
> +			struct sdxi_cxt *cxt = sdxi->cxt_array[i][j];
> +
> +			if (!cxt)
> +				continue;
> +			if (cxt->id != 0)  /* admin context shutdown is last */
> +				sdxi_working_cxt_exit(cxt);
> +			sdxi->cxt_array[i][j] = NULL;
> +		}
> +		if (i != 0)  /* another special case for admin cxt */
> +			kfree(sdxi->cxt_array[i]);
> +	}
> +
> +	sdxi_working_cxt_exit(sdxi->admin_cxt);
> +	kfree(sdxi->cxt_array[0]);  /* ugh */

The constraints here need to be described a little more clearly.


> +
> +	sdxi_stop(sdxi);
> +	sdxi_error_exit(sdxi);
> +	if (sdxi->dev_ops && sdxi->dev_ops->irq_exit)
> +		sdxi->dev_ops->irq_exit(sdxi);
> +}

Re: [PATCH RFC 09/13] dmaengine: sdxi: Add core device management code

[Nathan Lynch]

Jonathan Cameron <jonathan.cameron@huawei.com> writes:
> On Fri, 05 Sep 2025 13:48:32 -0500
> Nathan Lynch via B4 Relay <devnull+nathan.lynch.amd.com@kernel.org> wrote:
>
>> From: Nathan Lynch <nathan.lynch@amd.com>
>> 
>> Add code that manages device initialization and exit and provides
>> entry points for the PCI driver code to come.
>
> I'd prefer a patch series that started with the PCI device and built up
> functionality for the stuff found earlier + in this patch on top of it.
> Doing that allows each patch to be fully tested and reviewed on it's
> own.

Right, I think you made this point elsewhere too -- I'll try organizing
the series as you suggest.


>> +/* Refer to "Activation of the SDXI Function by Software". */
>> +static int sdxi_fn_activate(struct sdxi_dev *sdxi)
>> +{
>> +	const struct sdxi_dev_ops *ops = sdxi->dev_ops;
>> +	u64 cxt_l2;
>> +	u64 cap0;
>> +	u64 cap1;
>> +	u64 ctl2;
>
> Combine these u64 declarations on one line.

Sure.


>> +void sdxi_device_exit(struct sdxi_dev *sdxi)
>> +{
>> +	sdxi_working_cxt_exit(sdxi->dma_cxt);
>> +
>> +	/* Walk sdxi->cxt_array freeing any allocated rows. */
>> +	for (size_t i = 0; i < L2_TABLE_ENTRIES; ++i) {
>> +		if (!sdxi->cxt_array[i])
>> +			continue;
>> +		/* When a context is released its entry in the table should be NULL. */
>> +		for (size_t j = 0; j < L1_TABLE_ENTRIES; ++j) {
>> +			struct sdxi_cxt *cxt = sdxi->cxt_array[i][j];
>> +
>> +			if (!cxt)
>> +				continue;
>> +			if (cxt->id != 0)  /* admin context shutdown is last */
>> +				sdxi_working_cxt_exit(cxt);
>> +			sdxi->cxt_array[i][j] = NULL;
>> +		}
>> +		if (i != 0)  /* another special case for admin cxt */
>> +			kfree(sdxi->cxt_array[i]);
>> +	}
>> +
>> +	sdxi_working_cxt_exit(sdxi->admin_cxt);
>> +	kfree(sdxi->cxt_array[0]);  /* ugh */
>
> The constraints here need to be described a little more clearly.

Heh, I think I was tired of finding and fixing memory leaks when I wrote
these comments. If this part of the code survives to the next version
I'll improve them, but I plan to rework how we track contexts using a
per-device xarray, so this code should become a single xa_for_each()
loop followed by the admin context cleanup.

[PATCH RFC 10/13] dmaengine: sdxi: Add PCI driver support

[Nathan Lynch via B4 Relay]

From: Nathan Lynch <nathan.lynch@amd.com>

Add support for binding to PCIe-hosted SDXI devices. SDXI requires
MSI(-X) for PCI implementations, so this code will be gated by
CONFIG_PCI_MSI in the Makefile.

Co-developed-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
---
 drivers/dma/sdxi/pci.c | 216 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 216 insertions(+)

diff --git a/drivers/dma/sdxi/pci.c b/drivers/dma/sdxi/pci.c
new file mode 100644
index 0000000000000000000000000000000000000000..b7f74555395c605c4affffb198ee359accac8521
--- /dev/null
+++ b/drivers/dma/sdxi/pci.c
@@ -0,0 +1,216 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * SDXI PCI device code
+ *
+ * Copyright (C) 2025 Advanced Micro Devices, Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/dev_printk.h>
+#include <linux/dma-direction.h>
+#include <linux/dma-mapping.h>
+#include <linux/errno.h>
+#include <linux/io.h>
+#include <linux/iomap.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pci-ats.h>
+#include <linux/pci.h>
+
+#include "mmio.h"
+#include "sdxi.h"
+
+/*
+ * SDXI devices signal message 0 on error conditions, see "Error
+ * Logging Control and Status Registers".
+ */
+#define ERROR_IRQ_MSG 0
+
+/* MMIO BARs */
+#define MMIO_CTL_REGS_BAR		0x0
+#define MMIO_DOORBELL_BAR		0x2
+
+static struct pci_dev *sdxi_to_pci_dev(const struct sdxi_dev *sdxi)
+{
+	return to_pci_dev(sdxi_to_dev(sdxi));
+}
+
+static int sdxi_pci_irq_init(struct sdxi_dev *sdxi)
+{
+	struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
+	int msi_count;
+	int ret;
+
+	/* 1st irq for error + 1 for each context */
+	msi_count = sdxi->max_cxts + 1;
+
+	ret = pci_alloc_irq_vectors(pdev, 1, msi_count,
+				    PCI_IRQ_MSI | PCI_IRQ_MSIX);
+	if (ret < 0) {
+		sdxi_err(sdxi, "alloc MSI/MSI-X vectors failed\n");
+		return ret;
+	}
+
+	sdxi->error_irq = pci_irq_vector(pdev, ERROR_IRQ_MSG);
+
+	sdxi_dbg(sdxi, "allocated %d irq vectors", ret);
+
+	return 0;
+}
+
+static void sdxi_pci_irq_exit(struct sdxi_dev *sdxi)
+{
+	pci_free_irq_vectors(sdxi_to_pci_dev(sdxi));
+}
+
+static int sdxi_pci_map(struct sdxi_dev *sdxi)
+{
+	struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
+	int bars, ret;
+
+	bars = 1 << MMIO_CTL_REGS_BAR | 1 << MMIO_DOORBELL_BAR;
+	ret = pcim_iomap_regions(pdev, bars, SDXI_DRV_NAME);
+	if (ret) {
+		sdxi_err(sdxi, "pcim_iomap_regions failed (%d)\n", ret);
+		return ret;
+	}
+
+	sdxi->dbs_bar = pci_resource_start(pdev, MMIO_DOORBELL_BAR);
+
+	/* FIXME: pcim_iomap_table may return NULL, and it's deprecated. */
+	sdxi->ctrl_regs = pcim_iomap_table(pdev)[MMIO_CTL_REGS_BAR];
+	sdxi->dbs = pcim_iomap_table(pdev)[MMIO_DOORBELL_BAR];
+	if (!sdxi->ctrl_regs || !sdxi->dbs) {
+		sdxi_err(sdxi, "pcim_iomap_table failed\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void sdxi_pci_unmap(struct sdxi_dev *sdxi)
+{
+	struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
+
+	pcim_iounmap(pdev, sdxi->ctrl_regs);
+	pcim_iounmap(pdev, sdxi->dbs);
+}
+
+static int sdxi_pci_init(struct sdxi_dev *sdxi)
+{
+	struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
+	struct device *dev = &pdev->dev;
+	int dma_bits = 64;
+	int ret;
+
+	ret = pcim_enable_device(pdev);
+	if (ret) {
+		sdxi_err(sdxi, "pcim_enbale_device failed\n");
+		return ret;
+	}
+
+	pci_set_master(pdev);
+	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(dma_bits));
+	if (ret) {
+		sdxi_err(sdxi, "failed to set DMA mask & coherent bits\n");
+		return ret;
+	}
+
+	ret = sdxi_pci_map(sdxi);
+	if (ret) {
+		sdxi_err(sdxi, "failed to map device IO resources\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static void sdxi_pci_exit(struct sdxi_dev *sdxi)
+{
+	sdxi_pci_unmap(sdxi);
+}
+
+static struct sdxi_dev *sdxi_device_alloc(struct device *dev)
+{
+	struct sdxi_dev *sdxi;
+
+	sdxi = kzalloc(sizeof(*sdxi), GFP_KERNEL);
+	if (!sdxi)
+		return NULL;
+
+	sdxi->dev = dev;
+
+	mutex_init(&sdxi->cxt_lock);
+
+	return sdxi;
+}
+
+static void sdxi_device_free(struct sdxi_dev *sdxi)
+{
+	kfree(sdxi);
+}
+
+static const struct sdxi_dev_ops sdxi_pci_dev_ops = {
+	.irq_init = sdxi_pci_irq_init,
+	.irq_exit = sdxi_pci_irq_exit,
+};
+
+static int sdxi_pci_probe(struct pci_dev *pdev,
+			  const struct pci_device_id *id)
+{
+	struct device *dev = &pdev->dev;
+	struct sdxi_dev *sdxi;
+	int err;
+
+	sdxi = sdxi_device_alloc(dev);
+	if (!sdxi)
+		return -ENOMEM;
+
+	pci_set_drvdata(pdev, sdxi);
+
+	err = sdxi_pci_init(sdxi);
+	if (err)
+		goto free_sdxi;
+
+	err = sdxi_device_init(sdxi, &sdxi_pci_dev_ops);
+	if (err)
+		goto pci_exit;
+
+	return 0;
+
+pci_exit:
+	sdxi_pci_exit(sdxi);
+free_sdxi:
+	sdxi_device_free(sdxi);
+
+	return err;
+}
+
+static void sdxi_pci_remove(struct pci_dev *pdev)
+{
+	struct sdxi_dev *sdxi = pci_get_drvdata(pdev);
+
+	sdxi_device_exit(sdxi);
+	sdxi_pci_exit(sdxi);
+	sdxi_device_free(sdxi);
+}
+
+static const struct pci_device_id sdxi_id_table[] = {
+	{ PCI_DEVICE_CLASS(PCI_CLASS_ACCELERATOR_SDXI, 0xffffff) },
+	{0, }
+};
+MODULE_DEVICE_TABLE(pci, sdxi_id_table);
+
+static struct pci_driver sdxi_driver = {
+	.name = "sdxi",
+	.id_table = sdxi_id_table,
+	.probe = sdxi_pci_probe,
+	.remove = sdxi_pci_remove,
+	.sriov_configure = pci_sriov_configure_simple,
+};
+
+module_pci_driver(sdxi_driver);

-- 
2.39.5

Re: [PATCH RFC 10/13] dmaengine: sdxi: Add PCI driver support

[Mario Limonciello]

On 9/5/2025 1:48 PM, Nathan Lynch via B4 Relay wrote:
> From: Nathan Lynch <nathan.lynch@amd.com>
> 
> Add support for binding to PCIe-hosted SDXI devices. SDXI requires
> MSI(-X) for PCI implementations, so this code will be gated by
> CONFIG_PCI_MSI in the Makefile.
> 
> Co-developed-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
> ---
>   drivers/dma/sdxi/pci.c | 216 +++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 216 insertions(+)
> 
> diff --git a/drivers/dma/sdxi/pci.c b/drivers/dma/sdxi/pci.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..b7f74555395c605c4affffb198ee359accac8521
> --- /dev/null
> +++ b/drivers/dma/sdxi/pci.c
> @@ -0,0 +1,216 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * SDXI PCI device code
> + *
> + * Copyright (C) 2025 Advanced Micro Devices, Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/dev_printk.h>
> +#include <linux/dma-direction.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/errno.h>
> +#include <linux/io.h>
> +#include <linux/iomap.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pci-ats.h>
> +#include <linux/pci.h>
> +
> +#include "mmio.h"
> +#include "sdxi.h"
> +
> +/*
> + * SDXI devices signal message 0 on error conditions, see "Error
> + * Logging Control and Status Registers".
> + */
> +#define ERROR_IRQ_MSG 0
> +
> +/* MMIO BARs */
> +#define MMIO_CTL_REGS_BAR		0x0
> +#define MMIO_DOORBELL_BAR		0x2
> +
> +static struct pci_dev *sdxi_to_pci_dev(const struct sdxi_dev *sdxi)
> +{
> +	return to_pci_dev(sdxi_to_dev(sdxi));
> +}
> +
> +static int sdxi_pci_irq_init(struct sdxi_dev *sdxi)
> +{
> +	struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
> +	int msi_count;
> +	int ret;
> +
> +	/* 1st irq for error + 1 for each context */
> +	msi_count = sdxi->max_cxts + 1;
> +
> +	ret = pci_alloc_irq_vectors(pdev, 1, msi_count,
> +				    PCI_IRQ_MSI | PCI_IRQ_MSIX);
> +	if (ret < 0) {
> +		sdxi_err(sdxi, "alloc MSI/MSI-X vectors failed\n");
> +		return ret;
> +	}
> +
> +	sdxi->error_irq = pci_irq_vector(pdev, ERROR_IRQ_MSG);
> +
> +	sdxi_dbg(sdxi, "allocated %d irq vectors", ret);
> +
> +	return 0;
> +}
> +
> +static void sdxi_pci_irq_exit(struct sdxi_dev *sdxi)
> +{
> +	pci_free_irq_vectors(sdxi_to_pci_dev(sdxi));
> +}
> +
> +static int sdxi_pci_map(struct sdxi_dev *sdxi)
> +{
> +	struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
> +	int bars, ret;
> +
> +	bars = 1 << MMIO_CTL_REGS_BAR | 1 << MMIO_DOORBELL_BAR;
> +	ret = pcim_iomap_regions(pdev, bars, SDXI_DRV_NAME);
> +	if (ret) {
> +		sdxi_err(sdxi, "pcim_iomap_regions failed (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	sdxi->dbs_bar = pci_resource_start(pdev, MMIO_DOORBELL_BAR);
> +
> +	/* FIXME: pcim_iomap_table may return NULL, and it's deprecated. */
> +	sdxi->ctrl_regs = pcim_iomap_table(pdev)[MMIO_CTL_REGS_BAR];
> +	sdxi->dbs = pcim_iomap_table(pdev)[MMIO_DOORBELL_BAR];
> +	if (!sdxi->ctrl_regs || !sdxi->dbs) {
> +		sdxi_err(sdxi, "pcim_iomap_table failed\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static void sdxi_pci_unmap(struct sdxi_dev *sdxi)
> +{
> +	struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
> +
> +	pcim_iounmap(pdev, sdxi->ctrl_regs);
> +	pcim_iounmap(pdev, sdxi->dbs);
> +}
> +
> +static int sdxi_pci_init(struct sdxi_dev *sdxi)
> +{
> +	struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
> +	struct device *dev = &pdev->dev;
> +	int dma_bits = 64;
> +	int ret;
> +
> +	ret = pcim_enable_device(pdev);
> +	if (ret) {
> +		sdxi_err(sdxi, "pcim_enbale_device failed\n");
> +		return ret;
> +	}
> +
> +	pci_set_master(pdev);

Does pci_set_master() need to come before dma_set_mask_and_coherent() 
and sdxi_pci_map()?

If so; there should be an error handling path that does pci_clear_master().

> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(dma_bits));
> +	if (ret) {
> +		sdxi_err(sdxi, "failed to set DMA mask & coherent bits\n");
> +		return ret;
> +	}
> +
> +	ret = sdxi_pci_map(sdxi);
> +	if (ret) {
> +		sdxi_err(sdxi, "failed to map device IO resources\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void sdxi_pci_exit(struct sdxi_dev *sdxi)
> +{
> +	sdxi_pci_unmap(sdxi);

I think you need a pci_clear_master() here.

> +}
> +
> +static struct sdxi_dev *sdxi_device_alloc(struct device *dev)
> +{
> +	struct sdxi_dev *sdxi;
> +
> +	sdxi = kzalloc(sizeof(*sdxi), GFP_KERNEL);
> +	if (!sdxi)
> +		return NULL;
> +
> +	sdxi->dev = dev;
> +
> +	mutex_init(&sdxi->cxt_lock);
> +
> +	return sdxi;
> +}
> +
> +static void sdxi_device_free(struct sdxi_dev *sdxi)
> +{
> +	kfree(sdxi);
> +}
> +
> +static const struct sdxi_dev_ops sdxi_pci_dev_ops = {
> +	.irq_init = sdxi_pci_irq_init,
> +	.irq_exit = sdxi_pci_irq_exit,
> +};
> +
> +static int sdxi_pci_probe(struct pci_dev *pdev,
> +			  const struct pci_device_id *id)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sdxi_dev *sdxi;
> +	int err;
> +
> +	sdxi = sdxi_device_alloc(dev);
> +	if (!sdxi)
> +		return -ENOMEM;
> +
> +	pci_set_drvdata(pdev, sdxi);
> +
> +	err = sdxi_pci_init(sdxi);
> +	if (err)
> +		goto free_sdxi;
> +
> +	err = sdxi_device_init(sdxi, &sdxi_pci_dev_ops);
> +	if (err)
> +		goto pci_exit;
> +
> +	return 0;
> +
> +pci_exit:
> +	sdxi_pci_exit(sdxi);
> +free_sdxi:
> +	sdxi_device_free(sdxi);
> +
> +	return err;
> +}
> +
> +static void sdxi_pci_remove(struct pci_dev *pdev)
> +{
> +	struct sdxi_dev *sdxi = pci_get_drvdata(pdev);
> +
> +	sdxi_device_exit(sdxi);
> +	sdxi_pci_exit(sdxi);
> +	sdxi_device_free(sdxi);
> +}
> +
> +static const struct pci_device_id sdxi_id_table[] = {
> +	{ PCI_DEVICE_CLASS(PCI_CLASS_ACCELERATOR_SDXI, 0xffffff) },
> +	{0, }
> +};
> +MODULE_DEVICE_TABLE(pci, sdxi_id_table);
> +
> +static struct pci_driver sdxi_driver = {
> +	.name = "sdxi",
> +	.id_table = sdxi_id_table,
> +	.probe = sdxi_pci_probe,
> +	.remove = sdxi_pci_remove,
> +	.sriov_configure = pci_sriov_configure_simple,
> +};
> +
> +module_pci_driver(sdxi_driver);
> 

Re: [PATCH RFC 10/13] dmaengine: sdxi: Add PCI driver support

[Nathan Lynch]

Mario Limonciello <mario.limonciello@amd.com> writes:
> On 9/5/2025 1:48 PM, Nathan Lynch via B4 Relay wrote:
>> +static int sdxi_pci_map(struct sdxi_dev *sdxi)
>> +{
>> +	struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
>> +	int bars, ret;
>> +
>> +	bars = 1 << MMIO_CTL_REGS_BAR | 1 << MMIO_DOORBELL_BAR;
>> +	ret = pcim_iomap_regions(pdev, bars, SDXI_DRV_NAME);
>> +	if (ret) {
>> +		sdxi_err(sdxi, "pcim_iomap_regions failed (%d)\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	sdxi->dbs_bar = pci_resource_start(pdev, MMIO_DOORBELL_BAR);
>> +
>> +	/* FIXME: pcim_iomap_table may return NULL, and it's deprecated. */
>> +	sdxi->ctrl_regs = pcim_iomap_table(pdev)[MMIO_CTL_REGS_BAR];
>> +	sdxi->dbs = pcim_iomap_table(pdev)[MMIO_DOORBELL_BAR];
>> +	if (!sdxi->ctrl_regs || !sdxi->dbs) {
>> +		sdxi_err(sdxi, "pcim_iomap_table failed\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void sdxi_pci_unmap(struct sdxi_dev *sdxi)
>> +{
>> +	struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
>> +
>> +	pcim_iounmap(pdev, sdxi->ctrl_regs);
>> +	pcim_iounmap(pdev, sdxi->dbs);
>> +}
>> +
>> +static int sdxi_pci_init(struct sdxi_dev *sdxi)
>> +{
>> +	struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
>> +	struct device *dev = &pdev->dev;
>> +	int dma_bits = 64;
>> +	int ret;
>> +
>> +	ret = pcim_enable_device(pdev);
>> +	if (ret) {
>> +		sdxi_err(sdxi, "pcim_enbale_device failed\n");
>> +		return ret;
>> +	}
>> +
>> +	pci_set_master(pdev);
>
> Does pci_set_master() need to come before dma_set_mask_and_coherent() 
> and sdxi_pci_map()?

Yes, I think so. I'll audit this code for conformance to the
initialization sequence described in Documentation/pci/pci.rst.

>
> If so; there should be an error handling path that does
> pci_clear_master().

Right.

>
>> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(dma_bits));
>> +	if (ret) {
>> +		sdxi_err(sdxi, "failed to set DMA mask & coherent bits\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = sdxi_pci_map(sdxi);
>> +	if (ret) {
>> +		sdxi_err(sdxi, "failed to map device IO resources\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void sdxi_pci_exit(struct sdxi_dev *sdxi)
>> +{
>> +	sdxi_pci_unmap(sdxi);
>
> I think you need a pci_clear_master() here.

OK, will fix.

Re: [PATCH RFC 10/13] dmaengine: sdxi: Add PCI driver support

[Bjorn Helgaas]

On Fri, Sep 05, 2025 at 01:48:33PM -0500, Nathan Lynch via B4 Relay wrote:
> From: Nathan Lynch <nathan.lynch@amd.com>
> 
> Add support for binding to PCIe-hosted SDXI devices. SDXI requires
> MSI(-X) for PCI implementations, so this code will be gated by
> CONFIG_PCI_MSI in the Makefile.

> +static int sdxi_pci_init(struct sdxi_dev *sdxi)
> +{
> +	struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
> +	struct device *dev = &pdev->dev;
> +	int dma_bits = 64;
> +	int ret;
> +
> +	ret = pcim_enable_device(pdev);
> +	if (ret) {
> +		sdxi_err(sdxi, "pcim_enbale_device failed\n");

s/pcim_enbale_device/pcim_enable_device/

> +		return ret;
> +	}
> +
> +	pci_set_master(pdev);
> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(dma_bits));

I don't see the point of "dma_bits" over using 64 here.

> +	if (ret) {
> +		sdxi_err(sdxi, "failed to set DMA mask & coherent bits\n");
> +		return ret;
> +	}
> +
> +	ret = sdxi_pci_map(sdxi);
> +	if (ret) {
> +		sdxi_err(sdxi, "failed to map device IO resources\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}

Re: [PATCH RFC 10/13] dmaengine: sdxi: Add PCI driver support

[Nathan Lynch]

Bjorn Helgaas <helgaas@kernel.org> writes:
> On Fri, Sep 05, 2025 at 01:48:33PM -0500, Nathan Lynch via B4 Relay wrote:
>> +static int sdxi_pci_init(struct sdxi_dev *sdxi)
>> +{
>> +	struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
>> +	struct device *dev = &pdev->dev;
>> +	int dma_bits = 64;
>> +	int ret;
>> +
>> +	ret = pcim_enable_device(pdev);
>> +	if (ret) {
>> +		sdxi_err(sdxi, "pcim_enbale_device failed\n");
>
> s/pcim_enbale_device/pcim_enable_device/

Will fix.

>
>> +		return ret;
>> +	}
>> +
>> +	pci_set_master(pdev);
>> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(dma_bits));
>
> I don't see the point of "dma_bits" over using 64 here.

Agreed.

Re: [PATCH RFC 10/13] dmaengine: sdxi: Add PCI driver support

[Jonathan Cameron]

On Fri, 05 Sep 2025 13:48:33 -0500
Nathan Lynch via B4 Relay <devnull+nathan.lynch.amd.com@kernel.org> wrote:

> From: Nathan Lynch <nathan.lynch@amd.com>
> 
> Add support for binding to PCIe-hosted SDXI devices. SDXI requires
> MSI(-X) for PCI implementations, so this code will be gated by
> CONFIG_PCI_MSI in the Makefile.
> 
> Co-developed-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>

I've tried not to overlap too much with other reviewers.
A few comments inline.

> ---
>  drivers/dma/sdxi/pci.c | 216 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 216 insertions(+)
> 
> diff --git a/drivers/dma/sdxi/pci.c b/drivers/dma/sdxi/pci.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..b7f74555395c605c4affffb198ee359accac8521
> --- /dev/null
> +++ b/drivers/dma/sdxi/pci.c

> + * SDXI devices signal message 0 on error conditions, see "Error
> + * Logging Control and Status Registers".
> + */
> +#define ERROR_IRQ_MSG 0
> +
> +/* MMIO BARs */
> +#define MMIO_CTL_REGS_BAR		0x0
> +#define MMIO_DOORBELL_BAR		0x2

> +
> +static void sdxi_pci_unmap(struct sdxi_dev *sdxi)
> +{
> +	struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
> +
> +	pcim_iounmap(pdev, sdxi->ctrl_regs);
> +	pcim_iounmap(pdev, sdxi->dbs);
> +}
> +
> +static int sdxi_pci_init(struct sdxi_dev *sdxi)
> +{
> +	struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
> +	struct device *dev = &pdev->dev;
> +	int dma_bits = 64;
> +	int ret;
> +
> +	ret = pcim_enable_device(pdev);
> +	if (ret) {
> +		sdxi_err(sdxi, "pcim_enbale_device failed\n");

enable.

> +		return ret;

For probe stuff I'd suggest not using the sdxi wrapper and instead
using return dev_err_probe(); I guess you could define and sdxi_err_probe()
if you want to.   

> +	}
> +
> +	pci_set_master(pdev);
> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(dma_bits));
> +	if (ret) {
> +		sdxi_err(sdxi, "failed to set DMA mask & coherent bits\n");
> +		return ret;
> +	}
> +
> +	ret = sdxi_pci_map(sdxi);
> +	if (ret) {
> +		sdxi_err(sdxi, "failed to map device IO resources\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void sdxi_pci_exit(struct sdxi_dev *sdxi)
> +{
> +	sdxi_pci_unmap(sdxi);
> +}
> +
> +static struct sdxi_dev *sdxi_device_alloc(struct device *dev)
> +{
> +	struct sdxi_dev *sdxi;
> +
> +	sdxi = kzalloc(sizeof(*sdxi), GFP_KERNEL);
	sdxi = devm_kzalloc(dev, sizeof(*sdxi), GFP_KERNEL);
seems like it would be sufficient here

> +	if (!sdxi)
> +		return NULL;
> +
> +	sdxi->dev = dev;
> +
> +	mutex_init(&sdxi->cxt_lock);
For new code, nice to use
	rc = devm_mutex_init(&sdxi->ctx_lock);
	if (rc)
		return ERR_PTR(rc);

Benefit for lock debugging is small, but it's also not particularly
bad wrt to code complexity here.  Up to you.

> +
> +	return sdxi;
> +}
> +
> +static void sdxi_device_free(struct sdxi_dev *sdxi)
> +{
> +	kfree(sdxi);
> +}
If this doesn't get more complex later I'd just take the view
it's obvious that kfree(sdxi) is undoing something done in sdxi_device_alloc()
and just call that inline.  No need for the trivial wrapper.

Or just use devm_kzalloc() and let the automatic stuff clean it up
for you on error or remove.


> +static const struct pci_device_id sdxi_id_table[] = {
> +	{ PCI_DEVICE_CLASS(PCI_CLASS_ACCELERATOR_SDXI, 0xffffff) },
> +	{0, }
	{ }

is fine

> +};
> +MODULE_DEVICE_TABLE(pci, sdxi_id_table);

> 

Re: [External] : [PATCH RFC 10/13] dmaengine: sdxi: Add PCI driver support

[ALOK TIWARI]

On 9/6/2025 12:18 AM, Nathan Lynch via B4 Relay wrote:
> +static int sdxi_pci_init(struct sdxi_dev *sdxi)
> +{
> +	struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
> +	struct device *dev = &pdev->dev;
> +	int dma_bits = 64;
> +	int ret;
> +
> +	ret = pcim_enable_device(pdev);
> +	if (ret) {
> +		sdxi_err(sdxi, "pcim_enbale_device failed\n");

typo pcim_enbale_device -> pcim_enable_device

> +		return ret;
> +	}
> +
> +	pci_set_master(pdev);
> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(dma_bits));
> +	if (ret) {
> +		sdxi_err(sdxi, "failed to set DMA mask & coherent bits\n");
> +		return ret;
> +	}
> +
> +	ret = sdxi_pci_map(sdxi);
> +	if (ret) {
> +		sdxi_err(sdxi, "failed to map device IO resources\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}


Thanks,
Alok

[PATCH RFC 11/13] dmaengine: sdxi: Add DMA engine provider

[Nathan Lynch via B4 Relay]

From: Nathan Lynch <nathan.lynch@amd.com>

Add support for memcpy and interrupt capabilities. Register one
channel per SDXI function discovered for now.

Co-developed-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
---
 drivers/dma/sdxi/device.c |   4 +
 drivers/dma/sdxi/dma.c    | 409 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/dma/sdxi/dma.h    |  12 ++
 3 files changed, 425 insertions(+)

diff --git a/drivers/dma/sdxi/device.c b/drivers/dma/sdxi/device.c
index 61123bc1d47b6547538b6e783ad96a9c2851494e..e5e1593189993717cae311906885ae268385b28d 100644
--- a/drivers/dma/sdxi/device.c
+++ b/drivers/dma/sdxi/device.c
@@ -18,6 +18,7 @@
 
 #include "context.h"
 #include "descriptor.h"
+#include "dma.h"
 #include "hw.h"
 #include "error.h"
 #include "sdxi.h"
@@ -354,6 +355,8 @@ int sdxi_device_init(struct sdxi_dev *sdxi, const struct sdxi_dev_ops *ops)
 	if (err)
 		goto fn_stop;
 
+	sdxi_dma_register(sdxi->dma_cxt);
+
 	return 0;
 fn_stop:
 	sdxi_stop(sdxi);
@@ -362,6 +365,7 @@ int sdxi_device_init(struct sdxi_dev *sdxi, const struct sdxi_dev_ops *ops)
 
 void sdxi_device_exit(struct sdxi_dev *sdxi)
 {
+	sdxi_dma_unregister(sdxi->dma_cxt);
 	sdxi_working_cxt_exit(sdxi->dma_cxt);
 
 	/* Walk sdxi->cxt_array freeing any allocated rows. */
diff --git a/drivers/dma/sdxi/dma.c b/drivers/dma/sdxi/dma.c
new file mode 100644
index 0000000000000000000000000000000000000000..ad8515deba53898b2b4ea0d38c40042b566abe1f
--- /dev/null
+++ b/drivers/dma/sdxi/dma.c
@@ -0,0 +1,409 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * SDXI DMA engine implementation
+ *   Derived from ptdma code
+ *
+ * Copyright (C) 2025 Advanced Micro Devices, Inc.
+ */
+
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+
+#include "../dmaengine.h"
+#include "context.h"
+#include "descriptor.h"
+#include "dma.h"
+#include "sdxi.h"
+
+struct sdxi_dma_desc {
+	struct virt_dma_desc vd;
+	struct sdxi_cxt *cxt;
+	enum dma_status status;
+	bool issued_to_hw;
+	struct sdxi_cmd sdxi_cmd;
+};
+
+static inline struct sdxi_dma_chan *to_sdxi_dma_chan(struct dma_chan *dma_chan)
+{
+	return container_of(dma_chan, struct sdxi_dma_chan, vc.chan);
+}
+
+static inline struct sdxi_dma_desc *to_sdxi_dma_desc(struct virt_dma_desc *vd)
+{
+	return container_of(vd, struct sdxi_dma_desc, vd);
+}
+
+static void sdxi_dma_free_chan_resources(struct dma_chan *dma_chan)
+{
+	struct sdxi_dma_chan *chan = to_sdxi_dma_chan(dma_chan);
+
+	vchan_free_chan_resources(&chan->vc);
+	/* NB: more configure with sdxi_cxt? */
+}
+
+static void sdxi_dma_synchronize(struct dma_chan *c)
+{
+	struct sdxi_dma_chan *chan = to_sdxi_dma_chan(c);
+
+	vchan_synchronize(&chan->vc);
+}
+
+static void sdxi_do_cleanup(struct virt_dma_desc *vd)
+{
+	struct sdxi_dma_desc *dma_desc = to_sdxi_dma_desc(vd);
+	struct sdxi_cmd *cmd = &dma_desc->sdxi_cmd;
+	struct device *dev = sdxi_to_dev(dma_desc->cxt->sdxi);
+
+	dma_free_coherent(dev, sizeof(*cmd->cst_blk),
+			  cmd->cst_blk, cmd->cst_blk_dma);
+	kfree(dma_desc);
+}
+
+static int sdxi_dma_start_desc(struct sdxi_dma_desc *dma_desc)
+{
+	struct sdxi_dev *sdxi;
+	struct sdxi_cmd *sdxi_cmd;
+	struct sdxi_cxt *cxt;
+	struct sdxi_desc desc;
+	struct sdxi_copy copy;
+	struct sdxi_cst_blk *cst_blk;
+	dma_addr_t cst_blk_dma;
+	int err;
+
+	sdxi_cmd = &dma_desc->sdxi_cmd;
+	sdxi = sdxi_cmd->cxt->sdxi;
+
+	cxt = dma_desc->cxt;
+
+	if (sdxi_cmd->len > MAX_DMA_COPY_BYTES)
+		return -EINVAL;
+
+	copy = (typeof(copy)) {
+		.src = sdxi_cmd->src_addr,
+		.dst = sdxi_cmd->dst_addr,
+		.src_akey = 0,
+		.dst_akey = 0,
+		.len = sdxi_cmd->len,
+	};
+
+	err = sdxi_encode_copy(&desc, &copy);
+	if (err)
+		return err;
+
+	err = sdxi_encode_copy(&desc, &copy);
+	if (err)
+		return err;
+
+	/* FIXME convert to pool */
+	cst_blk = dma_alloc_coherent(sdxi_to_dev(sdxi), sizeof(*cst_blk),
+				     &cst_blk_dma, GFP_NOWAIT);
+	if (!cst_blk)
+		return -ENOMEM;
+
+	cst_blk->signal = cpu_to_le64(0xff);
+
+	sdxi_cmd->cst_blk = cst_blk;
+	sdxi_cmd->cst_blk_dma = cst_blk_dma;
+	sdxi_cmd->ret = 0; /* TODO: get desc submit status & update ret value */
+
+	sdxi_desc_set_csb(&desc, cst_blk_dma);
+	err = sdxi_submit_desc(cxt, &desc);
+	if (err)
+		goto free_cst_blk;
+
+	sdxi->tdata.cmd = sdxi_cmd; /* FIXME: this is not compatible w/multiple clients */
+	dma_desc->issued_to_hw = 1;
+	return 0;
+free_cst_blk:
+	dma_free_coherent(sdxi_to_dev(sdxi), sizeof(*cst_blk),
+			  cst_blk, cst_blk_dma);
+	return err;
+}
+
+static struct sdxi_dma_desc *sdxi_next_dma_desc(struct sdxi_dma_chan *chan)
+{
+	/* Get the next DMA descriptor on the active list */
+	struct virt_dma_desc *vd = vchan_next_desc(&chan->vc);
+
+	return vd ? to_sdxi_dma_desc(vd) : NULL;
+}
+
+static struct sdxi_dma_desc *sdxi_handle_active_desc(struct sdxi_dma_chan *chan,
+						     struct sdxi_dma_desc *desc)
+{
+	struct dma_async_tx_descriptor *tx_desc;
+	struct virt_dma_desc *vd;
+	unsigned long flags;
+
+	/* Loop over descriptors until one is found with commands */
+	do {
+		if (desc) {
+			if (!desc->issued_to_hw) {
+				/* No errors, keep going */
+				if (desc->status != DMA_ERROR)
+					return desc;
+			}
+
+			tx_desc = &desc->vd.tx;
+			vd = &desc->vd;
+		} else {
+			tx_desc = NULL;
+		}
+
+		spin_lock_irqsave(&chan->vc.lock, flags);
+
+		if (desc) {
+
+			if (desc->status != DMA_COMPLETE) {
+				if (desc->status != DMA_ERROR)
+					desc->status = DMA_COMPLETE;
+
+				dma_cookie_complete(tx_desc);
+				dma_descriptor_unmap(tx_desc);
+				list_del(&desc->vd.node);
+			} else {
+				/* Don't handle it twice */
+				tx_desc = NULL;
+			}
+		}
+
+		desc = sdxi_next_dma_desc(chan);
+
+		spin_unlock_irqrestore(&chan->vc.lock, flags);
+
+		if (tx_desc) {
+			dmaengine_desc_get_callback_invoke(tx_desc, NULL);
+			dma_run_dependencies(tx_desc);
+			vchan_vdesc_fini(vd);
+		}
+	} while (desc);
+
+	return NULL;
+}
+
+static void sdxi_cmd_callback(void *data, int err)
+{
+	struct sdxi_dma_desc *desc = data;
+	struct dma_chan *dma_chan;
+	struct sdxi_dma_chan *chan;
+	int ret;
+
+	if (err == -EINPROGRESS)
+		return;
+
+	dma_chan = desc->vd.tx.chan;
+	chan = to_sdxi_dma_chan(dma_chan);
+
+	if (err)
+		desc->status = DMA_ERROR;
+
+	while (true) {
+		/* Check for DMA descriptor completion */
+		desc = sdxi_handle_active_desc(chan, desc);
+
+		/* Don't submit cmd if no descriptor or DMA is paused */
+		if (!desc)
+			break;
+
+		ret = sdxi_dma_start_desc(desc);
+		if (!ret)
+			break;
+
+		desc->status = DMA_ERROR;
+	}
+}
+
+static struct sdxi_dma_desc *sdxi_dma_alloc_dma_desc(struct sdxi_dma_chan *chan,
+						     unsigned long flags)
+{
+	struct sdxi_dma_desc *desc;
+
+	desc = kzalloc(sizeof(*desc), GFP_NOWAIT);
+	if (!desc)
+		return NULL;
+
+	desc->cxt = chan->cxt;
+
+	vchan_tx_prep(&chan->vc, &desc->vd, flags);
+
+	desc->cxt->sdxi = chan->cxt->sdxi;
+	desc->issued_to_hw = 0;
+	desc->status = DMA_IN_PROGRESS;
+
+	return desc;
+}
+
+static struct sdxi_dma_desc *sdxi_dma_create_desc(struct dma_chan *dma_chan,
+						  dma_addr_t dst,
+						  dma_addr_t src,
+						  unsigned int len,
+						  unsigned long flags)
+{
+	struct sdxi_dma_chan *chan = to_sdxi_dma_chan(dma_chan);
+	struct sdxi_dma_desc *desc;
+	struct sdxi_cmd *sdxi_cmd;
+
+	desc = sdxi_dma_alloc_dma_desc(chan, flags);
+	if (!desc)
+		return NULL;
+
+	sdxi_cmd = &desc->sdxi_cmd;
+	sdxi_cmd->cxt = chan->cxt;
+	sdxi_cmd->cxt->sdxi = chan->cxt->sdxi;
+	sdxi_cmd->src_addr = src;
+	sdxi_cmd->dst_addr = dst;
+	sdxi_cmd->len = len;
+	sdxi_cmd->sdxi_cmd_callback = sdxi_cmd_callback;
+	sdxi_cmd->data = desc;
+
+	return desc;
+}
+
+static struct dma_async_tx_descriptor *
+sdxi_dma_prep_memcpy(struct dma_chan *dma_chan, dma_addr_t dst,
+		     dma_addr_t src, size_t len, unsigned long flags)
+{
+	struct sdxi_dma_desc *desc;
+
+	desc = sdxi_dma_create_desc(dma_chan, dst, src, len, flags);
+	if (!desc)
+		return NULL;
+
+	return &desc->vd.tx;
+}
+
+static struct dma_async_tx_descriptor *
+sdxi_prep_dma_interrupt(struct dma_chan *dma_chan, unsigned long flags)
+{
+	struct sdxi_dma_chan *chan = to_sdxi_dma_chan(dma_chan);
+	struct sdxi_dma_desc *desc;
+
+	desc = sdxi_dma_alloc_dma_desc(chan, flags);
+	if (!desc)
+		return NULL;
+
+	return &desc->vd.tx;
+}
+
+static void sdxi_dma_issue_pending(struct dma_chan *dma_chan)
+{
+	struct sdxi_dma_chan *chan = to_sdxi_dma_chan(dma_chan);
+	struct sdxi_dma_desc *desc;
+	unsigned long flags;
+	bool engine_is_idle = true;
+
+	spin_lock_irqsave(&chan->vc.lock, flags);
+
+	desc = sdxi_next_dma_desc(chan);
+	if (desc)
+		engine_is_idle = false;
+
+	vchan_issue_pending(&chan->vc);
+
+	desc = sdxi_next_dma_desc(chan);
+
+	spin_unlock_irqrestore(&chan->vc.lock, flags);
+
+	/* If there was nothing active, start processing */
+	if (engine_is_idle)
+		sdxi_cmd_callback(desc, 0);
+}
+
+static void sdxi_check_trans_status(struct sdxi_dma_chan *chan)
+{
+	struct sdxi_cxt *cxt = chan->cxt;
+	struct sdxi_cmd *cmd;
+
+	if (!cxt)
+		return;
+
+	cmd = cxt->sdxi->tdata.cmd;
+
+	if (le64_to_cpu(cmd->cst_blk->signal) == 0xfe)
+		sdxi_cmd_callback(cmd->data, cmd->ret);
+}
+
+static enum dma_status sdxi_tx_status(struct dma_chan *dma_chan, dma_cookie_t cookie,
+				      struct dma_tx_state *tx_state)
+{
+	struct sdxi_dma_chan *chan = to_sdxi_dma_chan(dma_chan);
+
+	sdxi_check_trans_status(chan);
+
+	return dma_cookie_status(dma_chan, cookie, tx_state);
+}
+
+static int sdxi_dma_terminate_all(struct dma_chan *dma_chan)
+{
+	struct sdxi_dma_chan *chan = to_sdxi_dma_chan(dma_chan);
+	unsigned long flags;
+	LIST_HEAD(head);
+
+	spin_lock_irqsave(&chan->vc.lock, flags);
+	vchan_get_all_descriptors(&chan->vc, &head);
+	spin_unlock_irqrestore(&chan->vc.lock, flags);
+
+	vchan_dma_desc_free_list(&chan->vc, &head);
+	vchan_free_chan_resources(&chan->vc);
+
+	return 0;
+}
+
+int sdxi_dma_register(struct sdxi_cxt *dma_cxt)
+{
+	struct sdxi_dma_chan *chan;
+	struct sdxi_dev *sdxi = dma_cxt->sdxi;
+	struct device *dev = sdxi_to_dev(sdxi);
+	struct dma_device *dma_dev = &sdxi->dma_dev;
+	int ret = 0;
+
+	sdxi->sdxi_dma_chan = devm_kzalloc(dev, sizeof(*sdxi->sdxi_dma_chan),
+					   GFP_KERNEL);
+	if (!sdxi->sdxi_dma_chan)
+		return -ENOMEM;
+
+	sdxi->sdxi_dma_chan->cxt = dma_cxt;
+
+	dma_dev->dev = dev;
+	dma_dev->src_addr_widths = DMA_SLAVE_BUSWIDTH_64_BYTES;
+	dma_dev->dst_addr_widths = DMA_SLAVE_BUSWIDTH_64_BYTES;
+	dma_dev->directions = BIT(DMA_MEM_TO_MEM);
+	dma_dev->residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
+	dma_cap_set(DMA_MEMCPY, dma_dev->cap_mask);
+	dma_cap_set(DMA_INTERRUPT, dma_dev->cap_mask);
+
+	dma_cap_set(DMA_PRIVATE, dma_dev->cap_mask);
+
+	INIT_LIST_HEAD(&dma_dev->channels);
+
+	chan = sdxi->sdxi_dma_chan;
+	chan->cxt->sdxi = sdxi;
+
+	/* Set base and prep routines */
+	dma_dev->device_free_chan_resources = sdxi_dma_free_chan_resources;
+	dma_dev->device_prep_dma_memcpy = sdxi_dma_prep_memcpy;
+	dma_dev->device_prep_dma_interrupt = sdxi_prep_dma_interrupt;
+	dma_dev->device_issue_pending = sdxi_dma_issue_pending;
+	dma_dev->device_tx_status = sdxi_tx_status;
+	dma_dev->device_terminate_all = sdxi_dma_terminate_all;
+	dma_dev->device_synchronize = sdxi_dma_synchronize;
+
+	chan->vc.desc_free = sdxi_do_cleanup;
+	vchan_init(&chan->vc, dma_dev);
+
+	dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
+
+	ret = dma_async_device_register(dma_dev);
+	if (ret)
+		goto err_reg;
+
+	return 0;
+
+err_reg:
+	return ret;
+}
+
+void sdxi_dma_unregister(struct sdxi_cxt *dma_cxt)
+{
+	dma_async_device_unregister(&dma_cxt->sdxi->dma_dev);
+}
diff --git a/drivers/dma/sdxi/dma.h b/drivers/dma/sdxi/dma.h
new file mode 100644
index 0000000000000000000000000000000000000000..cdea77cd274043a76b658fa93716f6cea2867216
--- /dev/null
+++ b/drivers/dma/sdxi/dma.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Copyright (C) 2025 Advanced Micro Devices, Inc. */
+
+#ifndef DMA_SDXI_DMA_H
+#define DMA_SDXI_DMA_H
+
+struct sdxi_cxt;
+
+int sdxi_dma_register(struct sdxi_cxt *dma_cxt);
+void sdxi_dma_unregister(struct sdxi_cxt *dma_cxt);
+
+#endif /* DMA_SDXI_DMA_H */

-- 
2.39.5

Re: [PATCH RFC 11/13] dmaengine: sdxi: Add DMA engine provider

[Jonathan Cameron]

On Fri, 05 Sep 2025 13:48:34 -0500
Nathan Lynch via B4 Relay <devnull+nathan.lynch.amd.com@kernel.org> wrote:

> From: Nathan Lynch <nathan.lynch@amd.com>
> 
> Add support for memcpy and interrupt capabilities. Register one
> channel per SDXI function discovered for now.
> 
> Co-developed-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
A few superficial comments inline.

Good to see support for this standard device btw.

Thanks,

Jonathan

> ---
>  drivers/dma/sdxi/device.c |   4 +
>  drivers/dma/sdxi/dma.c    | 409 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/dma/sdxi/dma.h    |  12 ++
>  3 files changed, 425 insertions(+)

> diff --git a/drivers/dma/sdxi/dma.c b/drivers/dma/sdxi/dma.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..ad8515deba53898b2b4ea0d38c40042b566abe1f
> --- /dev/null
> +++ b/drivers/dma/sdxi/dma.c

> +static int sdxi_dma_start_desc(struct sdxi_dma_desc *dma_desc)
> +{
> +	struct sdxi_dev *sdxi;
> +	struct sdxi_cmd *sdxi_cmd;
> +	struct sdxi_cxt *cxt;
> +	struct sdxi_desc desc;
> +	struct sdxi_copy copy;
> +	struct sdxi_cst_blk *cst_blk;
> +	dma_addr_t cst_blk_dma;
> +	int err;
> +
> +	sdxi_cmd = &dma_desc->sdxi_cmd;
> +	sdxi = sdxi_cmd->cxt->sdxi;
> +
> +	cxt = dma_desc->cxt;
Probably makes sense to combine with the declarations above.

> +
> +	if (sdxi_cmd->len > MAX_DMA_COPY_BYTES)
> +		return -EINVAL;
> +
> +	copy = (typeof(copy)) {
> +		.src = sdxi_cmd->src_addr,
> +		.dst = sdxi_cmd->dst_addr,
> +		.src_akey = 0,
> +		.dst_akey = 0,
> +		.len = sdxi_cmd->len,
> +	};
> +
> +	err = sdxi_encode_copy(&desc, &copy);
> +	if (err)
> +		return err;
> +
> +	err = sdxi_encode_copy(&desc, &copy);
> +	if (err)
> +		return err;
> +
> +	/* FIXME convert to pool */
> +	cst_blk = dma_alloc_coherent(sdxi_to_dev(sdxi), sizeof(*cst_blk),
> +				     &cst_blk_dma, GFP_NOWAIT);
> +	if (!cst_blk)
> +		return -ENOMEM;
> +
> +	cst_blk->signal = cpu_to_le64(0xff);
> +
> +	sdxi_cmd->cst_blk = cst_blk;
> +	sdxi_cmd->cst_blk_dma = cst_blk_dma;
> +	sdxi_cmd->ret = 0; /* TODO: get desc submit status & update ret value */
> +
> +	sdxi_desc_set_csb(&desc, cst_blk_dma);
> +	err = sdxi_submit_desc(cxt, &desc);
> +	if (err)
> +		goto free_cst_blk;
> +
> +	sdxi->tdata.cmd = sdxi_cmd; /* FIXME: this is not compatible w/multiple clients */
> +	dma_desc->issued_to_hw = 1;
> +	return 0;
> +free_cst_blk:
> +	dma_free_coherent(sdxi_to_dev(sdxi), sizeof(*cst_blk),
> +			  cst_blk, cst_blk_dma);
> +	return err;
> +}

> +static struct sdxi_dma_desc *sdxi_handle_active_desc(struct sdxi_dma_chan *chan,
> +						     struct sdxi_dma_desc *desc)
> +{
> +	struct dma_async_tx_descriptor *tx_desc;
> +	struct virt_dma_desc *vd;
> +	unsigned long flags;
> +
> +	/* Loop over descriptors until one is found with commands */
> +	do {
> +		if (desc) {
> +			if (!desc->issued_to_hw) {
> +				/* No errors, keep going */
> +				if (desc->status != DMA_ERROR)
> +					return desc;
> +			}
> +
> +			tx_desc = &desc->vd.tx;
> +			vd = &desc->vd;
> +		} else {
> +			tx_desc = NULL;
> +		}
> +
> +		spin_lock_irqsave(&chan->vc.lock, flags);
> +
> +		if (desc) {
> +

No blank line here.

> +			if (desc->status != DMA_COMPLETE) {
> +				if (desc->status != DMA_ERROR)
> +					desc->status = DMA_COMPLETE;
> +
> +				dma_cookie_complete(tx_desc);
> +				dma_descriptor_unmap(tx_desc);
> +				list_del(&desc->vd.node);
> +			} else {
> +				/* Don't handle it twice */
> +				tx_desc = NULL;
> +			}
> +		}
> +
> +		desc = sdxi_next_dma_desc(chan);
> +
> +		spin_unlock_irqrestore(&chan->vc.lock, flags);
> +
> +		if (tx_desc) {
> +			dmaengine_desc_get_callback_invoke(tx_desc, NULL);
> +			dma_run_dependencies(tx_desc);
> +			vchan_vdesc_fini(vd);
> +		}
> +	} while (desc);
> +
> +	return NULL;
> +}
> +
> +static void sdxi_cmd_callback(void *data, int err)
> +{
> +	struct sdxi_dma_desc *desc = data;
> +	struct dma_chan *dma_chan;
> +	struct sdxi_dma_chan *chan;
> +	int ret;
> +
> +	if (err == -EINPROGRESS)
> +		return;
> +
> +	dma_chan = desc->vd.tx.chan;
> +	chan = to_sdxi_dma_chan(dma_chan);
> +
> +	if (err)
> +		desc->status = DMA_ERROR;
> +
> +	while (true) {
> +		/* Check for DMA descriptor completion */
> +		desc = sdxi_handle_active_desc(chan, desc);
> +
> +		/* Don't submit cmd if no descriptor or DMA is paused */
> +		if (!desc)
perhaps return?
> +			break;
> +
> +		ret = sdxi_dma_start_desc(desc);
> +		if (!ret)
> +			break;
Perhaps return to make it clear that there is nothing else to do.
> +
> +		desc->status = DMA_ERROR;
> +	}
> +}
> +

> +
> +static struct sdxi_dma_desc *sdxi_dma_create_desc(struct dma_chan *dma_chan,
> +						  dma_addr_t dst,
> +						  dma_addr_t src,
> +						  unsigned int len,
> +						  unsigned long flags)
> +{
> +	struct sdxi_dma_chan *chan = to_sdxi_dma_chan(dma_chan);
> +	struct sdxi_dma_desc *desc;
> +	struct sdxi_cmd *sdxi_cmd;
> +
> +	desc = sdxi_dma_alloc_dma_desc(chan, flags);
> +	if (!desc)
> +		return NULL;
> +
> +	sdxi_cmd = &desc->sdxi_cmd;
Maybe
	*sdxi_cmd = (struct sdxi_cmd) {
		.ctx = chan->ctx,
etc

	};
> +	sdxi_cmd->cxt = chan->cxt;
> +	sdxi_cmd->cxt->sdxi = chan->cxt->sdxi;
> +	sdxi_cmd->src_addr = src;
> +	sdxi_cmd->dst_addr = dst;
> +	sdxi_cmd->len = len;
> +	sdxi_cmd->sdxi_cmd_callback = sdxi_cmd_callback;
> +	sdxi_cmd->data = desc;
> +
> +	return desc;
> +}

> +
> +static void sdxi_check_trans_status(struct sdxi_dma_chan *chan)
> +{
> +	struct sdxi_cxt *cxt = chan->cxt;
> +	struct sdxi_cmd *cmd;
> +
> +	if (!cxt)
> +		return;
> +
> +	cmd = cxt->sdxi->tdata.cmd;
> +
> +	if (le64_to_cpu(cmd->cst_blk->signal) == 0xfe)

Given that's a magic looking value, I think this 0xfe needs a define.

> +		sdxi_cmd_callback(cmd->data, cmd->ret);
> +}

> +
> +int sdxi_dma_register(struct sdxi_cxt *dma_cxt)
> +{
> +	struct sdxi_dma_chan *chan;
> +	struct sdxi_dev *sdxi = dma_cxt->sdxi;
> +	struct device *dev = sdxi_to_dev(sdxi);
> +	struct dma_device *dma_dev = &sdxi->dma_dev;
> +	int ret = 0;
> +
> +	sdxi->sdxi_dma_chan = devm_kzalloc(dev, sizeof(*sdxi->sdxi_dma_chan),
> +					   GFP_KERNEL);
This results in a mix of manual cleanup and devm.  That's generally something
we want to avoid because it makes code hard to review for race conditions etc.
I'd consider using custom actions and devm_add_action_or_reset() to ensure
that everything up to the first thing you want to not manage is done with
devm and ensure everything after that is done by hand.

Or use devm for everything.

> +	if (!sdxi->sdxi_dma_chan)
> +		return -ENOMEM;
> +
> +	sdxi->sdxi_dma_chan->cxt = dma_cxt;
> +
> +	dma_dev->dev = dev;
> +	dma_dev->src_addr_widths = DMA_SLAVE_BUSWIDTH_64_BYTES;
> +	dma_dev->dst_addr_widths = DMA_SLAVE_BUSWIDTH_64_BYTES;
> +	dma_dev->directions = BIT(DMA_MEM_TO_MEM);
> +	dma_dev->residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
> +	dma_cap_set(DMA_MEMCPY, dma_dev->cap_mask);
> +	dma_cap_set(DMA_INTERRUPT, dma_dev->cap_mask);
> +
> +	dma_cap_set(DMA_PRIVATE, dma_dev->cap_mask);
> +
> +	INIT_LIST_HEAD(&dma_dev->channels);
> +
> +	chan = sdxi->sdxi_dma_chan;
> +	chan->cxt->sdxi = sdxi;
> +
> +	/* Set base and prep routines */
> +	dma_dev->device_free_chan_resources = sdxi_dma_free_chan_resources;
> +	dma_dev->device_prep_dma_memcpy = sdxi_dma_prep_memcpy;
> +	dma_dev->device_prep_dma_interrupt = sdxi_prep_dma_interrupt;
> +	dma_dev->device_issue_pending = sdxi_dma_issue_pending;
> +	dma_dev->device_tx_status = sdxi_tx_status;
> +	dma_dev->device_terminate_all = sdxi_dma_terminate_all;
> +	dma_dev->device_synchronize = sdxi_dma_synchronize;
> +
> +	chan->vc.desc_free = sdxi_do_cleanup;
> +	vchan_init(&chan->vc, dma_dev);
> +
> +	dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> +
> +	ret = dma_async_device_register(dma_dev);
> +	if (ret)
> +		goto err_reg;
> +
> +	return 0;
> +
> +err_reg:

Just return early unless there is something to do.

> +	return ret;
> +}
> +
> +void sdxi_dma_unregister(struct sdxi_cxt *dma_cxt)
> +{
> +	dma_async_device_unregister(&dma_cxt->sdxi->dma_dev);
> +}

[PATCH RFC 12/13] dmaengine: sdxi: Add Kconfig and Makefile

[Nathan Lynch via B4 Relay]

From: Nathan Lynch <nathan.lynch@amd.com>

Add SDXI Kconfig that includes debug and unit test options in addition
to the usual tristate. SDXI_DEBUG seems necessary because
DMADEVICES_DEBUG makes dmatest too verbose.

One goal is to keep the bus-agnostic portions of the driver buildable
without PCI(_MSI), in case non-PCI SDXI implementations come along
later.

Co-developed-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
---
 drivers/dma/Kconfig       |  2 ++
 drivers/dma/Makefile      |  1 +
 drivers/dma/sdxi/Kconfig  | 23 +++++++++++++++++++++++
 drivers/dma/sdxi/Makefile | 17 +++++++++++++++++
 4 files changed, 43 insertions(+)

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 05c7c7d9e5a4e52a8ad7ada8c8b9b1a6f9d875f6..cccf00b73e025944681b03cffe441c372526d3f3 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -774,6 +774,8 @@ source "drivers/dma/fsl-dpaa2-qdma/Kconfig"
 
 source "drivers/dma/lgm/Kconfig"
 
+source "drivers/dma/sdxi/Kconfig"
+
 source "drivers/dma/stm32/Kconfig"
 
 # clients
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index a54d7688392b1a0e956fa5d23633507f52f017d9..ae4154595e1a6250b441a90078e9df3607d3d1dd 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -85,6 +85,7 @@ obj-$(CONFIG_XGENE_DMA) += xgene-dma.o
 obj-$(CONFIG_ST_FDMA) += st_fdma.o
 obj-$(CONFIG_FSL_DPAA2_QDMA) += fsl-dpaa2-qdma/
 obj-$(CONFIG_INTEL_LDMA) += lgm/
+obj-$(CONFIG_SDXI) += sdxi/
 
 obj-y += amd/
 obj-y += mediatek/
diff --git a/drivers/dma/sdxi/Kconfig b/drivers/dma/sdxi/Kconfig
new file mode 100644
index 0000000000000000000000000000000000000000..c9757cffb5f64fbc175ded8d0c9d751f0a22b6df
--- /dev/null
+++ b/drivers/dma/sdxi/Kconfig
@@ -0,0 +1,23 @@
+config SDXI
+	tristate "SDXI support"
+	select DMA_ENGINE
+	select DMA_VIRTUAL_CHANNELS
+	select PACKING
+	help
+	  Enable support for Smart Data Accelerator Interface (SDXI)
+	  Platform Data Mover devices. SDXI is a vendor-neutral
+	  standard for a memory-to-memory data mover and acceleration
+	  interface.
+
+config SDXI_DEBUG
+	bool "SDXI driver debug"
+	default DMADEVICES_DEBUG
+	depends on SDXI != n
+	help
+	  Enable debug output from the SDXI driver. This is an option
+	  for use by developers and most users should say N here.
+
+config SDXI_KUNIT_TEST
+       tristate "SDXI unit tests" if !KUNIT_ALL_TESTS
+       depends on SDXI && KUNIT
+       default KUNIT_ALL_TESTS
diff --git a/drivers/dma/sdxi/Makefile b/drivers/dma/sdxi/Makefile
new file mode 100644
index 0000000000000000000000000000000000000000..c67e475689b45d6260fe970fb4afcc25f8f9ebc1
--- /dev/null
+++ b/drivers/dma/sdxi/Makefile
@@ -0,0 +1,17 @@
+# SPDX-License-Identifier: GPL-2.0
+
+ccflags-$(CONFIG_SDXI_DEBUG) += -DDEBUG
+
+obj-$(CONFIG_SDXI) += sdxi.o
+
+sdxi-objs += \
+	context.o     \
+	descriptor.o  \
+	device.o      \
+	dma.o         \
+	enqueue.o     \
+	error.o
+
+sdxi-$(CONFIG_PCI_MSI) += pci.o
+
+obj-$(CONFIG_SDXI_KUNIT_TEST) += descriptor_kunit.o

-- 
2.39.5

Re: [PATCH RFC 12/13] dmaengine: sdxi: Add Kconfig and Makefile

[Jonathan Cameron]

On Fri, 05 Sep 2025 13:48:35 -0500
Nathan Lynch via B4 Relay <devnull+nathan.lynch.amd.com@kernel.org> wrote:

> From: Nathan Lynch <nathan.lynch@amd.com>
> 
> Add SDXI Kconfig that includes debug and unit test options in addition
> to the usual tristate. SDXI_DEBUG seems necessary because
> DMADEVICES_DEBUG makes dmatest too verbose.
> 
> One goal is to keep the bus-agnostic portions of the driver buildable
> without PCI(_MSI), in case non-PCI SDXI implementations come along
> later.
> 
> Co-developed-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
It's up to the dma maintainer, but personally and for subsystems I
do maintain this approach of putting the build files in at the end
is not something I'd accept.

The reason being that it leads to issues in earlier patches being
hidden with stuff not well separated.  I'd much rather see the driver
built up so that it builds at each step with each new patch
adding additional functionality.  Also avoids things like comments
on the build dependencies in patch descriptions earlier in the series.
They become clear as the code is with the patch.  The one about MSIX
for example doesn't seem to be related to what is here.

Anyhow, no idea if dma maintainers prefer that or this approach.

> ---
>  drivers/dma/Kconfig       |  2 ++
>  drivers/dma/Makefile      |  1 +
>  drivers/dma/sdxi/Kconfig  | 23 +++++++++++++++++++++++
>  drivers/dma/sdxi/Makefile | 17 +++++++++++++++++
>  4 files changed, 43 insertions(+)
> 
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index 05c7c7d9e5a4e52a8ad7ada8c8b9b1a6f9d875f6..cccf00b73e025944681b03cffe441c372526d3f3 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -774,6 +774,8 @@ source "drivers/dma/fsl-dpaa2-qdma/Kconfig"
>  
>  source "drivers/dma/lgm/Kconfig"
>  
> +source "drivers/dma/sdxi/Kconfig"
> +
>  source "drivers/dma/stm32/Kconfig"
>  
>  # clients
> diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
> index a54d7688392b1a0e956fa5d23633507f52f017d9..ae4154595e1a6250b441a90078e9df3607d3d1dd 100644
> --- a/drivers/dma/Makefile
> +++ b/drivers/dma/Makefile
> @@ -85,6 +85,7 @@ obj-$(CONFIG_XGENE_DMA) += xgene-dma.o
>  obj-$(CONFIG_ST_FDMA) += st_fdma.o
>  obj-$(CONFIG_FSL_DPAA2_QDMA) += fsl-dpaa2-qdma/
>  obj-$(CONFIG_INTEL_LDMA) += lgm/
> +obj-$(CONFIG_SDXI) += sdxi/
>  
>  obj-y += amd/
>  obj-y += mediatek/
> diff --git a/drivers/dma/sdxi/Kconfig b/drivers/dma/sdxi/Kconfig
> new file mode 100644
> index 0000000000000000000000000000000000000000..c9757cffb5f64fbc175ded8d0c9d751f0a22b6df
> --- /dev/null
> +++ b/drivers/dma/sdxi/Kconfig
> @@ -0,0 +1,23 @@
> +config SDXI
> +	tristate "SDXI support"
> +	select DMA_ENGINE
> +	select DMA_VIRTUAL_CHANNELS
> +	select PACKING
> +	help
> +	  Enable support for Smart Data Accelerator Interface (SDXI)
> +	  Platform Data Mover devices. SDXI is a vendor-neutral
> +	  standard for a memory-to-memory data mover and acceleration
> +	  interface.
> +
> +config SDXI_DEBUG
> +	bool "SDXI driver debug"
> +	default DMADEVICES_DEBUG
> +	depends on SDXI != n
> +	help
> +	  Enable debug output from the SDXI driver. This is an option
> +	  for use by developers and most users should say N here.
> +
> +config SDXI_KUNIT_TEST
> +       tristate "SDXI unit tests" if !KUNIT_ALL_TESTS
> +       depends on SDXI && KUNIT
> +       default KUNIT_ALL_TESTS
> diff --git a/drivers/dma/sdxi/Makefile b/drivers/dma/sdxi/Makefile
> new file mode 100644
> index 0000000000000000000000000000000000000000..c67e475689b45d6260fe970fb4afcc25f8f9ebc1
> --- /dev/null
> +++ b/drivers/dma/sdxi/Makefile
> @@ -0,0 +1,17 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +ccflags-$(CONFIG_SDXI_DEBUG) += -DDEBUG

What does this actually do?  More modern drivers rarely
do this any more because we have nice facilities like dynamic debug.

> +
> +obj-$(CONFIG_SDXI) += sdxi.o
> +
> +sdxi-objs += \
> +	context.o     \
> +	descriptor.o  \
> +	device.o      \
> +	dma.o         \
> +	enqueue.o     \
> +	error.o
> +
> +sdxi-$(CONFIG_PCI_MSI) += pci.o
> +
> +obj-$(CONFIG_SDXI_KUNIT_TEST) += descriptor_kunit.o
> 

Re: [PATCH RFC 12/13] dmaengine: sdxi: Add Kconfig and Makefile

[Nathan Lynch]

Jonathan Cameron <jonathan.cameron@huawei.com> writes:

> On Fri, 05 Sep 2025 13:48:35 -0500
> Nathan Lynch via B4 Relay <devnull+nathan.lynch.amd.com@kernel.org> wrote:
>
>> From: Nathan Lynch <nathan.lynch@amd.com>
>> 
>> Add SDXI Kconfig that includes debug and unit test options in addition
>> to the usual tristate. SDXI_DEBUG seems necessary because
>> DMADEVICES_DEBUG makes dmatest too verbose.
>> 
>> One goal is to keep the bus-agnostic portions of the driver buildable
>> without PCI(_MSI), in case non-PCI SDXI implementations come along
>> later.
>> 
>> Co-developed-by: Wei Huang <wei.huang2@amd.com>
>> Signed-off-by: Wei Huang <wei.huang2@amd.com>
>> Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
> It's up to the dma maintainer, but personally and for subsystems I
> do maintain this approach of putting the build files in at the end
> is not something I'd accept.
>
> The reason being that it leads to issues in earlier patches being
> hidden with stuff not well separated.  I'd much rather see the driver
> built up so that it builds at each step with each new patch
> adding additional functionality.  Also avoids things like comments
> on the build dependencies in patch descriptions earlier in the series.
> They become clear as the code is with the patch.

Thanks for looking over the whole series. I'll plan on reorganizing it
according to your suggestions unless Vinod expresses a different
preference.


>> +ccflags-$(CONFIG_SDXI_DEBUG) += -DDEBUG
>
> What does this actually do?  More modern drivers rarely
> do this any more because we have nice facilities like dynamic debug.

Yeah after reviewing the dynamic debug doc I think we can get rid of
SDXI_DEBUG. Thanks.

[PATCH RFC 13/13] MAINTAINERS: Add entry for SDXI driver

[Nathan Lynch via B4 Relay]

From: Nathan Lynch <nathan.lynch@amd.com>

Add an entry for the SDXI driver to MAINTAINERS. Wei and I will
maintain the driver.

The SDXI specification and other materials may be found at:

  https://www.snia.org/sdxi

Co-developed-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index daf520a13bdf6a991c0160a96620f40308c29ee0..eb636ed1aa77aff30a871057efef81de8ff56cd7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22676,6 +22676,13 @@ L:	sdricohcs-devel@lists.sourceforge.net (subscribers-only)
 S:	Maintained
 F:	drivers/mmc/host/sdricoh_cs.c
 
+SDXI (Smart Data Accelerator Interface) DRIVER
+M:	Nathan Lynch <nathan.lynch@amd.com>
+M:	Wei Huang <wei.huang2@amd.com>
+L:	dmaengine@vger.kernel.org
+S:	Supported
+F:	drivers/dma/sdxi/
+
 SECO BOARDS CEC DRIVER
 M:	Ettore Chimenti <ek5.chimenti@gmail.com>
 S:	Maintained

-- 
2.39.5