[PATCH v4 0/6] Introduce initial AMD I3C HCI driver support

[PATCH v4 0/6] Introduce initial AMD I3C HCI driver support

[Shyam Sundar S K]

The AMD SoC includes an I3C IP block as part of the Fusion Controller Hub
(FCH). This series introduces the initial driver support to enable the I3C
IP block on AMD's latest processors.

Currently, the code is closely tied to dt-bindings. This initial set aims
to decouple some of these bindings by adding the MIPI ID, allowing the
current driver to support ACPI-enabled x86 systems.

It was discovered that the AMD I3C controller has several hardware issues,
including:
- Non-functional DMA mode (defaulting to PIO mode)
- Issues with Open-Drain (OD) and Push-Pull (PP) timing parameters
- Command response buffer threshold values

All of these issues have been addressed in this series.

v3->v4:
-------
 - use AMDI5017 as the _HID
 - use quirks bits within the .driver_data()
 - Add Reviewed-by tag

v2->v3:
-------
 - use MODULE_DEVICE_TABLE()
 - address comments from Jarkko
 - split version check and quirks into separate patches.
 
v1->v2:
-------
 - Address LKP reported problems
 - Guard boot_cpu_data usage with CONFIG_X86

Shyam Sundar S K (6):
  i3c: mipi-i3c-hci: Add AMDI5017 ACPI ID to the I3C Support List
  i3c: mipi-i3c-hci: Read HC_CONTROL_PIO_MODE only after i3c hci v1.1
  i3c: mipi-i3c-hci: Add a quirk to set PIO mode
  i3c: mipi-i3c-hci: Relocate helper macros to HCI header file
  i3c: mipi-i3c-hci: Add a quirk to set timing parameters
  i3c: mipi-i3c-hci: Add a quirk to set Response buffer threshold

 drivers/i3c/master/mipi-i3c-hci/Makefile     |  3 +-
 drivers/i3c/master/mipi-i3c-hci/core.c       | 33 +++++++++++----
 drivers/i3c/master/mipi-i3c-hci/hci.h        | 10 +++++
 drivers/i3c/master/mipi-i3c-hci/hci_quirks.c | 44 ++++++++++++++++++++
 4 files changed, 81 insertions(+), 9 deletions(-)
 create mode 100644 drivers/i3c/master/mipi-i3c-hci/hci_quirks.c

-- 
2.25.1

[PATCH v4 1/6] i3c: mipi-i3c-hci: Add AMDI5017 ACPI ID to the I3C Support List

[Shyam Sundar S K]

The current driver code lacks the necessary plumbing for ACPI IDs,
preventing the mipi-i3c-hci driver from being loaded on x86
platforms that advertise I3C ACPI support.

This update adds the AMDI5017 ACPI ID to the list of supported IDs.

Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i3c/master/mipi-i3c-hci/core.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index 4e7d6a43ee9b..b02fbd7882f8 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -834,12 +834,19 @@ static const __maybe_unused struct of_device_id i3c_hci_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, i3c_hci_of_match);
 
+static const struct acpi_device_id i3c_hci_acpi_match[] = {
+	{"AMDI5017"},
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, i3c_hci_acpi_match);
+
 static struct platform_driver i3c_hci_driver = {
 	.probe = i3c_hci_probe,
 	.remove_new = i3c_hci_remove,
 	.driver = {
 		.name = "mipi-i3c-hci",
 		.of_match_table = of_match_ptr(i3c_hci_of_match),
+		.acpi_match_table = i3c_hci_acpi_match,
 	},
 };
 module_platform_driver(i3c_hci_driver);
-- 
2.25.1

[PATCH v4 2/6] i3c: mipi-i3c-hci: Read HC_CONTROL_PIO_MODE only after i3c hci v1.1

[Shyam Sundar S K]

The HC_CONTROL_PIO_MODE bit was introduced in the HC_CONTROL register
starting from version 1.1. Therefore, checking the HC_CONTROL_PIO_MODE bit
on hardware that adheres to older specification revisions (i.e., versions
earlier than 1.1) is incorrect. To address this, add an additional check
to read the HCI version before attempting to read the HC_CONTROL_PIO_MODE
status.

Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i3c/master/mipi-i3c-hci/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index b02fbd7882f8..d1952a5619d4 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -33,6 +33,7 @@
 #define reg_clear(r, v)		reg_write(r, reg_read(r) & ~(v))
 
 #define HCI_VERSION			0x00	/* HCI Version (in BCD) */
+#define HCI_VERSION_V1			0x100   /* MIPI HCI Version number v1.0 */
 
 #define HC_CONTROL			0x04
 #define HC_CONTROL_BUS_ENABLE		BIT(31)
@@ -756,7 +757,7 @@ static int i3c_hci_init(struct i3c_hci *hci)
 	/* Try activating DMA operations first */
 	if (hci->RHS_regs) {
 		reg_clear(HC_CONTROL, HC_CONTROL_PIO_MODE);
-		if (reg_read(HC_CONTROL) & HC_CONTROL_PIO_MODE) {
+		if (regval > HCI_VERSION_V1 && (reg_read(HC_CONTROL) & HC_CONTROL_PIO_MODE)) {
 			dev_err(&hci->master.dev, "PIO mode is stuck\n");
 			ret = -EIO;
 		} else {
@@ -768,7 +769,7 @@ static int i3c_hci_init(struct i3c_hci *hci)
 	/* If no DMA, try PIO */
 	if (!hci->io && hci->PIO_regs) {
 		reg_set(HC_CONTROL, HC_CONTROL_PIO_MODE);
-		if (!(reg_read(HC_CONTROL) & HC_CONTROL_PIO_MODE)) {
+		if (regval > HCI_VERSION_V1 && !(reg_read(HC_CONTROL) & HC_CONTROL_PIO_MODE)) {
 			dev_err(&hci->master.dev, "DMA mode is stuck\n");
 			ret = -EIO;
 		} else {
-- 
2.25.1

[PATCH v4 3/6] i3c: mipi-i3c-hci: Add a quirk to set PIO mode

[Shyam Sundar S K]

The AMD HCI controller currently only supports PIO mode but exposes DMA
rings to the OS, which leads to the controller being configured in DMA
mode. To address this, add a quirk to avoid configuring the controller in
DMA mode and default to PIO mode.

Additionally, introduce a generic quirk infrastructure to the mipi-i3c-hci
driver to facilitate seamless future quirk additions.

Co-developed-by: Krishnamoorthi M <krishnamoorthi.m@amd.com>
Signed-off-by: Krishnamoorthi M <krishnamoorthi.m@amd.com>
Co-developed-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com>
Signed-off-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i3c/master/mipi-i3c-hci/core.c | 9 ++++++++-
 drivers/i3c/master/mipi-i3c-hci/hci.h  | 1 +
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index d1952a5619d4..3aa4aa9deb56 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -754,6 +754,11 @@ static int i3c_hci_init(struct i3c_hci *hci)
 		return -EINVAL;
 	}
 
+	/* Quirk for HCI_QUIRK_PIO_MODE on AMD platforms */
+	regval = reg_read(HCI_VERSION);
+	if (hci->quirks & HCI_QUIRK_PIO_MODE)
+		hci->RHS_regs = NULL;
+
 	/* Try activating DMA operations first */
 	if (hci->RHS_regs) {
 		reg_clear(HC_CONTROL, HC_CONTROL_PIO_MODE);
@@ -804,6 +809,8 @@ static int i3c_hci_probe(struct platform_device *pdev)
 	/* temporary for dev_printk's, to be replaced in i3c_master_register */
 	hci->master.dev.init_name = dev_name(&pdev->dev);
 
+	hci->quirks = (unsigned long)device_get_match_data(&pdev->dev);
+
 	ret = i3c_hci_init(hci);
 	if (ret)
 		return ret;
@@ -836,7 +843,7 @@ static const __maybe_unused struct of_device_id i3c_hci_of_match[] = {
 MODULE_DEVICE_TABLE(of, i3c_hci_of_match);
 
 static const struct acpi_device_id i3c_hci_acpi_match[] = {
-	{"AMDI5017"},
+	{"AMDI5017", HCI_QUIRK_PIO_MODE},
 	{}
 };
 MODULE_DEVICE_TABLE(acpi, i3c_hci_acpi_match);
diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
index f94d95e024be..c56b838fb431 100644
--- a/drivers/i3c/master/mipi-i3c-hci/hci.h
+++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
@@ -135,6 +135,7 @@ struct i3c_hci_dev_data {
 
 /* list of quirks */
 #define HCI_QUIRK_RAW_CCC	BIT(1)	/* CCC framing must be explicit */
+#define HCI_QUIRK_PIO_MODE	BIT(2)  /* Set PIO mode for AMD platforms */
 
 
 /* global functions */
-- 
2.25.1

[PATCH v4 4/6] i3c: mipi-i3c-hci: Relocate helper macros to HCI header file

[Shyam Sundar S K]

The reg_* helper macros are currently limited to core.c. Moving them to
hci.h will allow their functionality to be utilized in other files outside
of core.c.

Reviewed-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Co-developed-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com>
Signed-off-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i3c/master/mipi-i3c-hci/core.c | 6 ------
 drivers/i3c/master/mipi-i3c-hci/hci.h  | 5 +++++
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index 3aa4aa9deb56..c0f3c0ce9f6a 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -12,7 +12,6 @@
 #include <linux/errno.h>
 #include <linux/i3c/master.h>
 #include <linux/interrupt.h>
-#include <linux/io.h>
 #include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -27,11 +26,6 @@
  * Host Controller Capabilities and Operation Registers
  */
 
-#define reg_read(r)		readl(hci->base_regs + (r))
-#define reg_write(r, v)		writel(v, hci->base_regs + (r))
-#define reg_set(r, v)		reg_write(r, reg_read(r) | (v))
-#define reg_clear(r, v)		reg_write(r, reg_read(r) & ~(v))
-
 #define HCI_VERSION			0x00	/* HCI Version (in BCD) */
 #define HCI_VERSION_V1			0x100   /* MIPI HCI Version number v1.0 */
 
diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
index c56b838fb431..76658789b018 100644
--- a/drivers/i3c/master/mipi-i3c-hci/hci.h
+++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
@@ -10,6 +10,7 @@
 #ifndef HCI_H
 #define HCI_H
 
+#include <linux/io.h>
 
 /* Handy logging macro to save on line length */
 #define DBG(x, ...) pr_devel("%s: " x "\n", __func__, ##__VA_ARGS__)
@@ -26,6 +27,10 @@
 #define W2_BIT_(x)  BIT((x) - 64)
 #define W3_BIT_(x)  BIT((x) - 96)
 
+#define reg_read(r)		readl(hci->base_regs + (r))
+#define reg_write(r, v)		writel(v, hci->base_regs + (r))
+#define reg_set(r, v)		reg_write(r, reg_read(r) | (v))
+#define reg_clear(r, v)		reg_write(r, reg_read(r) & ~(v))
 
 struct hci_cmd_ops;
 
-- 
2.25.1

[PATCH v4 5/6] i3c: mipi-i3c-hci: Add a quirk to set timing parameters

[Shyam Sundar S K]

The AMD HCI controller is currently unstable at 12.5 MHz. To address this,
a quirk is added to configure the clock rate to 9 MHz as a workaround,
with proportional adjustments to the Open-Drain (OD) and Push-Pull (PP)
values.

Co-developed-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com>
Signed-off-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i3c/master/mipi-i3c-hci/Makefile     |  3 +-
 drivers/i3c/master/mipi-i3c-hci/core.c       |  6 +++-
 drivers/i3c/master/mipi-i3c-hci/hci.h        |  2 ++
 drivers/i3c/master/mipi-i3c-hci/hci_quirks.c | 33 ++++++++++++++++++++
 4 files changed, 42 insertions(+), 2 deletions(-)
 create mode 100644 drivers/i3c/master/mipi-i3c-hci/hci_quirks.c

diff --git a/drivers/i3c/master/mipi-i3c-hci/Makefile b/drivers/i3c/master/mipi-i3c-hci/Makefile
index a658e7b8262c..1f8cd5c48fde 100644
--- a/drivers/i3c/master/mipi-i3c-hci/Makefile
+++ b/drivers/i3c/master/mipi-i3c-hci/Makefile
@@ -3,4 +3,5 @@
 obj-$(CONFIG_MIPI_I3C_HCI)		+= mipi-i3c-hci.o
 mipi-i3c-hci-y				:= core.o ext_caps.o pio.o dma.o \
 					   cmd_v1.o cmd_v2.o \
-					   dat_v1.o dct_v1.o
+					   dat_v1.o dct_v1.o \
+					   hci_quirks.o
diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index c0f3c0ce9f6a..fe49765fc2da 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -784,6 +784,10 @@ static int i3c_hci_init(struct i3c_hci *hci)
 		return ret;
 	}
 
+	/* Configure OD and PP timings for AMD platforms */
+	if (hci->quirks & HCI_QUIRK_OD_PP_TIMING)
+		amd_set_od_pp_timing(hci);
+
 	return 0;
 }
 
@@ -837,7 +841,7 @@ static const __maybe_unused struct of_device_id i3c_hci_of_match[] = {
 MODULE_DEVICE_TABLE(of, i3c_hci_of_match);
 
 static const struct acpi_device_id i3c_hci_acpi_match[] = {
-	{"AMDI5017", HCI_QUIRK_PIO_MODE},
+	{"AMDI5017", HCI_QUIRK_PIO_MODE | HCI_QUIRK_OD_PP_TIMING},
 	{}
 };
 MODULE_DEVICE_TABLE(acpi, i3c_hci_acpi_match);
diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
index 76658789b018..361e1366fe38 100644
--- a/drivers/i3c/master/mipi-i3c-hci/hci.h
+++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
@@ -141,11 +141,13 @@ struct i3c_hci_dev_data {
 /* list of quirks */
 #define HCI_QUIRK_RAW_CCC	BIT(1)	/* CCC framing must be explicit */
 #define HCI_QUIRK_PIO_MODE	BIT(2)  /* Set PIO mode for AMD platforms */
+#define HCI_QUIRK_OD_PP_TIMING		BIT(3)  /* Set OD and PP timings for AMD platforms */
 
 
 /* global functions */
 void mipi_i3c_hci_resume(struct i3c_hci *hci);
 void mipi_i3c_hci_pio_reset(struct i3c_hci *hci);
 void mipi_i3c_hci_dct_index_reset(struct i3c_hci *hci);
+void amd_set_od_pp_timing(struct i3c_hci *hci);
 
 #endif
diff --git a/drivers/i3c/master/mipi-i3c-hci/hci_quirks.c b/drivers/i3c/master/mipi-i3c-hci/hci_quirks.c
new file mode 100644
index 000000000000..e8ea4d101f66
--- /dev/null
+++ b/drivers/i3c/master/mipi-i3c-hci/hci_quirks.c
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * I3C HCI Quirks
+ *
+ * Copyright 2024 Advanced Micro Devices, Inc.
+ *
+ * Authors: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
+ *	    Guruvendra Punugupati <Guruvendra.Punugupati@amd.com>
+ */
+
+#include <linux/i3c/master.h>
+#include "hci.h"
+
+/* Timing registers */
+#define HCI_SCL_I3C_OD_TIMING          0x214
+#define HCI_SCL_I3C_PP_TIMING          0x218
+#define HCI_SDA_HOLD_SWITCH_DLY_TIMING 0x230
+
+/* Timing values to configure 9MHz frequency */
+#define AMD_SCL_I3C_OD_TIMING          0x00cf00cf
+#define AMD_SCL_I3C_PP_TIMING          0x00160016
+
+void amd_set_od_pp_timing(struct i3c_hci *hci)
+{
+	u32 data;
+
+	reg_write(HCI_SCL_I3C_OD_TIMING, AMD_SCL_I3C_OD_TIMING);
+	reg_write(HCI_SCL_I3C_PP_TIMING, AMD_SCL_I3C_PP_TIMING);
+	data = reg_read(HCI_SDA_HOLD_SWITCH_DLY_TIMING);
+	/* Configure maximum TX hold time */
+	data |= W0_MASK(18, 16);
+	reg_write(HCI_SDA_HOLD_SWITCH_DLY_TIMING, data);
+}
-- 
2.25.1

[PATCH v4 6/6] i3c: mipi-i3c-hci: Add a quirk to set Response buffer threshold

[Shyam Sundar S K]

The current driver sets the response buffer threshold value to 1
(N+1, 2 DWORDS) in the QUEUE THRESHOLD register. However, the AMD
I3C controller only generates interrupts when the response buffer
threshold value is set to 0 (1 DWORD).

Therefore, a quirk is added to set the response buffer threshold value
to 0.

Co-developed-by: Krishnamoorthi M <krishnamoorthi.m@amd.com>
Signed-off-by: Krishnamoorthi M <krishnamoorthi.m@amd.com>
Co-developed-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com>
Signed-off-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i3c/master/mipi-i3c-hci/core.c       |  6 +++++-
 drivers/i3c/master/mipi-i3c-hci/hci.h        |  2 ++
 drivers/i3c/master/mipi-i3c-hci/hci_quirks.c | 11 +++++++++++
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index fe49765fc2da..094a93353280 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -147,6 +147,10 @@ static int i3c_hci_bus_init(struct i3c_master_controller *m)
 	if (ret)
 		return ret;
 
+	/* Set RESP_BUF_THLD to 0(n) to get 1(n+1) response */
+	if (hci->quirks & HCI_QUIRK_RESP_BUF_THLD)
+		amd_set_resp_buf_thld(hci);
+
 	reg_set(HC_CONTROL, HC_CONTROL_BUS_ENABLE);
 	DBG("HC_CONTROL = %#x", reg_read(HC_CONTROL));
 
@@ -841,7 +845,7 @@ static const __maybe_unused struct of_device_id i3c_hci_of_match[] = {
 MODULE_DEVICE_TABLE(of, i3c_hci_of_match);
 
 static const struct acpi_device_id i3c_hci_acpi_match[] = {
-	{"AMDI5017", HCI_QUIRK_PIO_MODE | HCI_QUIRK_OD_PP_TIMING},
+	{"AMDI5017", HCI_QUIRK_PIO_MODE | HCI_QUIRK_OD_PP_TIMING | HCI_QUIRK_RESP_BUF_THLD},
 	{}
 };
 MODULE_DEVICE_TABLE(acpi, i3c_hci_acpi_match);
diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
index 361e1366fe38..aaa47ac47381 100644
--- a/drivers/i3c/master/mipi-i3c-hci/hci.h
+++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
@@ -142,6 +142,7 @@ struct i3c_hci_dev_data {
 #define HCI_QUIRK_RAW_CCC	BIT(1)	/* CCC framing must be explicit */
 #define HCI_QUIRK_PIO_MODE	BIT(2)  /* Set PIO mode for AMD platforms */
 #define HCI_QUIRK_OD_PP_TIMING		BIT(3)  /* Set OD and PP timings for AMD platforms */
+#define HCI_QUIRK_RESP_BUF_THLD		BIT(4)  /* Set resp buf thld to 0 for AMD platforms */
 
 
 /* global functions */
@@ -149,5 +150,6 @@ void mipi_i3c_hci_resume(struct i3c_hci *hci);
 void mipi_i3c_hci_pio_reset(struct i3c_hci *hci);
 void mipi_i3c_hci_dct_index_reset(struct i3c_hci *hci);
 void amd_set_od_pp_timing(struct i3c_hci *hci);
+void amd_set_resp_buf_thld(struct i3c_hci *hci);
 
 #endif
diff --git a/drivers/i3c/master/mipi-i3c-hci/hci_quirks.c b/drivers/i3c/master/mipi-i3c-hci/hci_quirks.c
index e8ea4d101f66..3b9c6e76c536 100644
--- a/drivers/i3c/master/mipi-i3c-hci/hci_quirks.c
+++ b/drivers/i3c/master/mipi-i3c-hci/hci_quirks.c
@@ -20,6 +20,8 @@
 #define AMD_SCL_I3C_OD_TIMING          0x00cf00cf
 #define AMD_SCL_I3C_PP_TIMING          0x00160016
 
+#define QUEUE_THLD_CTRL                0xD0
+
 void amd_set_od_pp_timing(struct i3c_hci *hci)
 {
 	u32 data;
@@ -31,3 +33,12 @@ void amd_set_od_pp_timing(struct i3c_hci *hci)
 	data |= W0_MASK(18, 16);
 	reg_write(HCI_SDA_HOLD_SWITCH_DLY_TIMING, data);
 }
+
+void amd_set_resp_buf_thld(struct i3c_hci *hci)
+{
+	u32 data;
+
+	data = reg_read(QUEUE_THLD_CTRL);
+	data = data & ~W0_MASK(15, 8);
+	reg_write(QUEUE_THLD_CTRL, data);
+}
-- 
2.25.1

Re: [PATCH v4 1/6] i3c: mipi-i3c-hci: Add AMDI5017 ACPI ID to the I3C Support List

[Shyam Sundar S K]

+ Andy

On 8/21/2024 19:05, Shyam Sundar S K wrote:
> The current driver code lacks the necessary plumbing for ACPI IDs,
> preventing the mipi-i3c-hci driver from being loaded on x86
> platforms that advertise I3C ACPI support.
> 
> This update adds the AMDI5017 ACPI ID to the list of supported IDs.
> 
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/i3c/master/mipi-i3c-hci/core.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> index 4e7d6a43ee9b..b02fbd7882f8 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> @@ -834,12 +834,19 @@ static const __maybe_unused struct of_device_id i3c_hci_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, i3c_hci_of_match);
>  
> +static const struct acpi_device_id i3c_hci_acpi_match[] = {
> +	{"AMDI5017"},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(acpi, i3c_hci_acpi_match);
> +
>  static struct platform_driver i3c_hci_driver = {
>  	.probe = i3c_hci_probe,
>  	.remove_new = i3c_hci_remove,
>  	.driver = {
>  		.name = "mipi-i3c-hci",
>  		.of_match_table = of_match_ptr(i3c_hci_of_match),
> +		.acpi_match_table = i3c_hci_acpi_match,
>  	},
>  };
>  module_platform_driver(i3c_hci_driver);

Re: [PATCH v4 1/6] i3c: mipi-i3c-hci: Add AMDI5017 ACPI ID to the I3C Support List

[Andy Shevchenko]

On Wed, Aug 21, 2024 at 07:07:45PM +0530, Shyam Sundar S K wrote:
> + Andy

Thank you!

> On 8/21/2024 19:05, Shyam Sundar S K wrote:
> > The current driver code lacks the necessary plumbing for ACPI IDs,
> > preventing the mipi-i3c-hci driver from being loaded on x86
> > platforms that advertise I3C ACPI support.
> > 
> > This update adds the AMDI5017 ACPI ID to the list of supported IDs.

Please, provide a DSDT excerpt to show how it will look like in the ACPI
tables.

...

> > +static const struct acpi_device_id i3c_hci_acpi_match[] = {
> > +	{"AMDI5017"},

Spaces are missing

	{ "AMDI5017" },

> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(acpi, i3c_hci_acpi_match);

...

Otherwise LGTM, thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/6] i3c: mipi-i3c-hci: Add AMDI5017 ACPI ID to the I3C Support List

[Shyam Sundar S K]

On 8/21/2024 19:26, Andy Shevchenko wrote:
> On Wed, Aug 21, 2024 at 07:07:45PM +0530, Shyam Sundar S K wrote:
>> + Andy
> 
> Thank you!
> 
>> On 8/21/2024 19:05, Shyam Sundar S K wrote:
>>> The current driver code lacks the necessary plumbing for ACPI IDs,
>>> preventing the mipi-i3c-hci driver from being loaded on x86
>>> platforms that advertise I3C ACPI support.
>>>
>>> This update adds the AMDI5017 ACPI ID to the list of supported IDs.
> 
> Please, provide a DSDT excerpt to show how it will look like in the ACPI
> tables.


    Scope (_SB)
    {
	...

        Name (HCID, "AMDI5017")
        Device (I3CA)
        {
            Method (_HID, 0, Serialized)  // _HID: Hardware ID
            {
                If ((I30M == Zero))
                {
                    If (CondRefOf (HCIB))
                    {
                        Return (HCID) /* \_SB_.HCID */
                    }
                    Else
                    {
                        Return (I3ID) /* \_SB_.I3ID */
                    }
                }
                Else
                {
                    Return (I2ID) /* \_SB_.I2ID */
                }
            }
	
	...
    }

> 
> ...
> 
>>> +static const struct acpi_device_id i3c_hci_acpi_match[] = {
>>> +	{"AMDI5017"},
> 
> Spaces are missing
> 
> 	{ "AMDI5017" },

OK. Will change it.

> 
>>> +	{}
>>> +};
>>> +MODULE_DEVICE_TABLE(acpi, i3c_hci_acpi_match);
> 
> ...
> 
> Otherwise LGTM, thanks!
> 

Thanks,
Shyam

Re: [PATCH v4 1/6] i3c: mipi-i3c-hci: Add AMDI5017 ACPI ID to the I3C Support List

[Andy Shevchenko]

On Wed, Aug 21, 2024 at 08:42:12PM +0530, Shyam Sundar S K wrote:
> On 8/21/2024 19:26, Andy Shevchenko wrote:
> > On Wed, Aug 21, 2024 at 07:07:45PM +0530, Shyam Sundar S K wrote:
> >> On 8/21/2024 19:05, Shyam Sundar S K wrote:

...

> >>> This update adds the AMDI5017 ACPI ID to the list of supported IDs.

s/This update adds/Add/

> > Please, provide a DSDT excerpt to show how it will look like in the ACPI
> > tables.
> 
>     Scope (_SB)
>     {
> 	...
> 
>         Name (HCID, "AMDI5017")
>         Device (I3CA)
>         {
>             Method (_HID, 0, Serialized)  // _HID: Hardware ID
>             {
>                 If ((I30M == Zero))
>                 {
>                     If (CondRefOf (HCIB))
>                     {
>                         Return (HCID) /* \_SB_.HCID */
>                     }
>                     Else
>                     {
>                         Return (I3ID) /* \_SB_.I3ID */

Do I understand correctly that I3ID is the old one (as per another path I have
seen last week or so), i.o.w. *not* a MIPI allocated one?

If it's the case, feel free to add
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
from ACPI ID perspective.

>                     }
>                 }
>                 Else
>                 {
>                     Return (I2ID) /* \_SB_.I2ID */
>                 }
>             }
> 	
> 	...
>     }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/6] i3c: mipi-i3c-hci: Add AMDI5017 ACPI ID to the I3C Support List

[Andy Shevchenko]

On Wed, Aug 21, 2024 at 06:39:04PM +0300, Andy Shevchenko wrote:
> On Wed, Aug 21, 2024 at 08:42:12PM +0530, Shyam Sundar S K wrote:
> > On 8/21/2024 19:26, Andy Shevchenko wrote:
> > > On Wed, Aug 21, 2024 at 07:07:45PM +0530, Shyam Sundar S K wrote:
> > >> On 8/21/2024 19:05, Shyam Sundar S K wrote:

...

> > >>> This update adds the AMDI5017 ACPI ID to the list of supported IDs.
> 
> s/This update adds/Add/
> 
> > > Please, provide a DSDT excerpt to show how it will look like in the ACPI
> > > tables.
> > 
> >     Scope (_SB)
> >     {
> > 	...
> > 
> >         Name (HCID, "AMDI5017")
> >         Device (I3CA)
> >         {
> >             Method (_HID, 0, Serialized)  // _HID: Hardware ID
> >             {
> >                 If ((I30M == Zero))
> >                 {
> >                     If (CondRefOf (HCIB))
> >                     {
> >                         Return (HCID) /* \_SB_.HCID */
> >                     }
> >                     Else
> >                     {
> >                         Return (I3ID) /* \_SB_.I3ID */
> 
> Do I understand correctly that I3ID is the old one (as per another path I have
> seen last week or so), i.o.w. *not* a MIPI allocated one?
> 
> If it's the case, feel free to add
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> from ACPI ID perspective.

Regarding MIPI ID and using it as a _CID is kinda unsolved now, in any case
_CID *requires* _HID to be present, and hence _HID has a priority in
enumeration. It doesn't matter if it's absent now (it's even more flexible in
case MIPI decides to use _another_ ID for _CID) as long as software uses
_HID for enumeration.

> >                     }
> >                 }
> >                 Else
> >                 {
> >                     Return (I2ID) /* \_SB_.I2ID */
> >                 }
> >             }
> > 	
> > 	...
> >     }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/6] i3c: mipi-i3c-hci: Add AMDI5017 ACPI ID to the I3C Support List

[Andy Shevchenko]

On Wed, Aug 21, 2024 at 06:42:13PM +0300, Andy Shevchenko wrote:
> On Wed, Aug 21, 2024 at 06:39:04PM +0300, Andy Shevchenko wrote:
> > On Wed, Aug 21, 2024 at 08:42:12PM +0530, Shyam Sundar S K wrote:
> > > On 8/21/2024 19:26, Andy Shevchenko wrote:
> > > > On Wed, Aug 21, 2024 at 07:07:45PM +0530, Shyam Sundar S K wrote:
> > > >> On 8/21/2024 19:05, Shyam Sundar S K wrote:

...

> > > >>> This update adds the AMDI5017 ACPI ID to the list of supported IDs.
> > 
> > s/This update adds/Add/
> > 
> > > > Please, provide a DSDT excerpt to show how it will look like in the ACPI
> > > > tables.
> > > 
> > >     Scope (_SB)
> > >     {
> > > 	...
> > > 
> > >         Name (HCID, "AMDI5017")
> > >         Device (I3CA)
> > >         {
> > >             Method (_HID, 0, Serialized)  // _HID: Hardware ID
> > >             {
> > >                 If ((I30M == Zero))
> > >                 {
> > >                     If (CondRefOf (HCIB))
> > >                     {
> > >                         Return (HCID) /* \_SB_.HCID */
> > >                     }
> > >                     Else
> > >                     {
> > >                         Return (I3ID) /* \_SB_.I3ID */
> > 
> > Do I understand correctly that I3ID is the old one (as per another path I have
> > seen last week or so), i.o.w. *not* a MIPI allocated one?
> > 
> > If it's the case, feel free to add
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > from ACPI ID perspective.
> 
> Regarding MIPI ID and using it as a _CID is kinda unsolved now, in any case
> _CID *requires* _HID to be present, and hence _HID has a priority in
> enumeration. It doesn't matter if it's absent now (it's even more flexible in

Just to be crystal clear

 "It doesn't matter if _CID is absent now..."

> case MIPI decides to use _another_ ID for _CID) as long as software uses
> _HID for enumeration.
> 
> > >                     }
> > >                 }
> > >                 Else
> > >                 {
> > >                     Return (I2ID) /* \_SB_.I2ID */
> > >                 }
> > >             }
> > > 	
> > > 	...
> > >     }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/6] i3c: mipi-i3c-hci: Add AMDI5017 ACPI ID to the I3C Support List

[Shyam Sundar S K]

On 8/21/2024 21:09, Andy Shevchenko wrote:
> On Wed, Aug 21, 2024 at 08:42:12PM +0530, Shyam Sundar S K wrote:
>> On 8/21/2024 19:26, Andy Shevchenko wrote:
>>> On Wed, Aug 21, 2024 at 07:07:45PM +0530, Shyam Sundar S K wrote:
>>>> On 8/21/2024 19:05, Shyam Sundar S K wrote:
> 
> ...
> 
>>>>> This update adds the AMDI5017 ACPI ID to the list of supported IDs.
> 
> s/This update adds/Add/
> 
>>> Please, provide a DSDT excerpt to show how it will look like in the ACPI
>>> tables.
>>
>>     Scope (_SB)
>>     {
>> 	...
>>
>>         Name (HCID, "AMDI5017")
>>         Device (I3CA)
>>         {
>>             Method (_HID, 0, Serialized)  // _HID: Hardware ID
>>             {
>>                 If ((I30M == Zero))
>>                 {
>>                     If (CondRefOf (HCIB))
>>                     {
>>                         Return (HCID) /* \_SB_.HCID */
>>                     }
>>                     Else
>>                     {
>>                         Return (I3ID) /* \_SB_.I3ID */
> 
> Do I understand correctly that I3ID is the old one (as per another path I have
> seen last week or so), i.o.w. *not* a MIPI allocated one?

Yes. That's right.

> 
> If it's the case, feel free to add
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> from ACPI ID perspective.
> 
>>                     }
>>                 }
>>                 Else
>>                 {
>>                     Return (I2ID) /* \_SB_.I2ID */
>>                 }
>>             }
>> 	
>> 	...
>>     }
> 

Re: [PATCH v4 1/6] i3c: mipi-i3c-hci: Add AMDI5017 ACPI ID to the I3C Support List

[Shyam Sundar S K]

On 8/21/2024 21:13, Andy Shevchenko wrote:
> On Wed, Aug 21, 2024 at 06:42:13PM +0300, Andy Shevchenko wrote:
>> On Wed, Aug 21, 2024 at 06:39:04PM +0300, Andy Shevchenko wrote:
>>> On Wed, Aug 21, 2024 at 08:42:12PM +0530, Shyam Sundar S K wrote:
>>>> On 8/21/2024 19:26, Andy Shevchenko wrote:
>>>>> On Wed, Aug 21, 2024 at 07:07:45PM +0530, Shyam Sundar S K wrote:
>>>>>> On 8/21/2024 19:05, Shyam Sundar S K wrote:
> 
> ...
> 
>>>>>>> This update adds the AMDI5017 ACPI ID to the list of supported IDs.
>>>
>>> s/This update adds/Add/
>>>
>>>>> Please, provide a DSDT excerpt to show how it will look like in the ACPI
>>>>> tables.
>>>>
>>>>     Scope (_SB)
>>>>     {
>>>> 	...
>>>>
>>>>         Name (HCID, "AMDI5017")
>>>>         Device (I3CA)
>>>>         {
>>>>             Method (_HID, 0, Serialized)  // _HID: Hardware ID
>>>>             {
>>>>                 If ((I30M == Zero))
>>>>                 {
>>>>                     If (CondRefOf (HCIB))
>>>>                     {
>>>>                         Return (HCID) /* \_SB_.HCID */
>>>>                     }
>>>>                     Else
>>>>                     {
>>>>                         Return (I3ID) /* \_SB_.I3ID */
>>>
>>> Do I understand correctly that I3ID is the old one (as per another path I have
>>> seen last week or so), i.o.w. *not* a MIPI allocated one?
>>>
>>> If it's the case, feel free to add
>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> from ACPI ID perspective.
>>
>> Regarding MIPI ID and using it as a _CID is kinda unsolved now, in any case
>> _CID *requires* _HID to be present, and hence _HID has a priority in
>> enumeration. It doesn't matter if it's absent now (it's even more flexible in
> 
> Just to be crystal clear
> 
>  "It doesn't matter if _CID is absent now..."

Thanks! yeah, and that's right. I have left this decision to my BIOS
peers.

btw, I have notified MIPI about this error in the MIPI I3C DisCo
Specification.

Thanks,
Shyam

> 
>> case MIPI decides to use _another_ ID for _CID) as long as software uses
>> _HID for enumeration.
>>
>>>>                     }
>>>>                 }
>>>>                 Else
>>>>                 {
>>>>                     Return (I2ID) /* \_SB_.I2ID */
>>>>                 }
>>>>             }
>>>> 	
>>>> 	...
>>>>     }
> 

Re: [PATCH v4 2/6] i3c: mipi-i3c-hci: Read HC_CONTROL_PIO_MODE only after i3c hci v1.1

[Jarkko Nikula]

Hi

On 8/21/24 4:35 PM, Shyam Sundar S K wrote:
> The HC_CONTROL_PIO_MODE bit was introduced in the HC_CONTROL register
> starting from version 1.1. Therefore, checking the HC_CONTROL_PIO_MODE bit
> on hardware that adheres to older specification revisions (i.e., versions
> earlier than 1.1) is incorrect. To address this, add an additional check
> to read the HCI version before attempting to read the HC_CONTROL_PIO_MODE
> status.
> 
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>   drivers/i3c/master/mipi-i3c-hci/core.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> index b02fbd7882f8..d1952a5619d4 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> @@ -33,6 +33,7 @@
>   #define reg_clear(r, v)		reg_write(r, reg_read(r) & ~(v))
>   
>   #define HCI_VERSION			0x00	/* HCI Version (in BCD) */
> +#define HCI_VERSION_V1			0x100   /* MIPI HCI Version number v1.0 */
>   
>   #define HC_CONTROL			0x04
>   #define HC_CONTROL_BUS_ENABLE		BIT(31)
> @@ -756,7 +757,7 @@ static int i3c_hci_init(struct i3c_hci *hci)
>   	/* Try activating DMA operations first */
>   	if (hci->RHS_regs) {
>   		reg_clear(HC_CONTROL, HC_CONTROL_PIO_MODE);
> -		if (reg_read(HC_CONTROL) & HC_CONTROL_PIO_MODE) {
> +		if (regval > HCI_VERSION_V1 && (reg_read(HC_CONTROL) & HC_CONTROL_PIO_MODE)) {
>   			dev_err(&hci->master.dev, "PIO mode is stuck\n");
>   			ret = -EIO;
>   		} else {

Actually here after this patch regval doesn't contain the version but 
HC_CONTROL which is read some line above. HCI_VERSION read is added by 
the patch 3/6 so this patch alone may cause a regression.

I'd say better to use already set hci->version_major and 
hci->version_minor since then code is not vulnerable if some other 
regval = reg_read(OTHER_REG) is added later before this code block.

Re: [PATCH v4 3/6] i3c: mipi-i3c-hci: Add a quirk to set PIO mode

[Jarkko Nikula]

On 8/21/24 4:35 PM, Shyam Sundar S K wrote:
> The AMD HCI controller currently only supports PIO mode but exposes DMA
> rings to the OS, which leads to the controller being configured in DMA
> mode. To address this, add a quirk to avoid configuring the controller in
> DMA mode and default to PIO mode.
> 
> Additionally, introduce a generic quirk infrastructure to the mipi-i3c-hci
> driver to facilitate seamless future quirk additions.
> 
> Co-developed-by: Krishnamoorthi M <krishnamoorthi.m@amd.com>
> Signed-off-by: Krishnamoorthi M <krishnamoorthi.m@amd.com>
> Co-developed-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com>
> Signed-off-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>   drivers/i3c/master/mipi-i3c-hci/core.c | 9 ++++++++-
>   drivers/i3c/master/mipi-i3c-hci/hci.h  | 1 +
>   2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> index d1952a5619d4..3aa4aa9deb56 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> @@ -754,6 +754,11 @@ static int i3c_hci_init(struct i3c_hci *hci)
>   		return -EINVAL;
>   	}
>   
> +	/* Quirk for HCI_QUIRK_PIO_MODE on AMD platforms */
> +	regval = reg_read(HCI_VERSION);
> +	if (hci->quirks & HCI_QUIRK_PIO_MODE)
> +		hci->RHS_regs = NULL;
> +
>   	/* Try activating DMA operations first */
>   	if (hci->RHS_regs) {
>   		reg_clear(HC_CONTROL, HC_CONTROL_PIO_MODE);

This looks otherwise ok to me but with "regval = reg_read(HCI_VERSION);" 
removed. I commented that in the patch 2/6.

Re: [PATCH v4 5/6] i3c: mipi-i3c-hci: Add a quirk to set timing parameters

[Jarkko Nikula]

On 8/21/24 4:35 PM, Shyam Sundar S K wrote:
> The AMD HCI controller is currently unstable at 12.5 MHz. To address this,
> a quirk is added to configure the clock rate to 9 MHz as a workaround,
> with proportional adjustments to the Open-Drain (OD) and Push-Pull (PP)
> values.
> 
> Co-developed-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com>
> Signed-off-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>   drivers/i3c/master/mipi-i3c-hci/Makefile     |  3 +-
>   drivers/i3c/master/mipi-i3c-hci/core.c       |  6 +++-
>   drivers/i3c/master/mipi-i3c-hci/hci.h        |  2 ++
>   drivers/i3c/master/mipi-i3c-hci/hci_quirks.c | 33 ++++++++++++++++++++
>   4 files changed, 42 insertions(+), 2 deletions(-)
>   create mode 100644 drivers/i3c/master/mipi-i3c-hci/hci_quirks.c
> 
Reviewed-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH v4 6/6] i3c: mipi-i3c-hci: Add a quirk to set Response buffer threshold

[Jarkko Nikula]

On 8/21/24 4:35 PM, Shyam Sundar S K wrote:
> The current driver sets the response buffer threshold value to 1
> (N+1, 2 DWORDS) in the QUEUE THRESHOLD register. However, the AMD
> I3C controller only generates interrupts when the response buffer
> threshold value is set to 0 (1 DWORD).
> 
> Therefore, a quirk is added to set the response buffer threshold value
> to 0.
> 
> Co-developed-by: Krishnamoorthi M <krishnamoorthi.m@amd.com>
> Signed-off-by: Krishnamoorthi M <krishnamoorthi.m@amd.com>
> Co-developed-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com>
> Signed-off-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>   drivers/i3c/master/mipi-i3c-hci/core.c       |  6 +++++-
>   drivers/i3c/master/mipi-i3c-hci/hci.h        |  2 ++
>   drivers/i3c/master/mipi-i3c-hci/hci_quirks.c | 11 +++++++++++
>   3 files changed, 18 insertions(+), 1 deletion(-)
> 
Reviewed-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>