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

[PATCH RESEND v3 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.

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 MIPI0100 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        | 11 ++++
 drivers/i3c/master/mipi-i3c-hci/hci_quirks.c | 55 ++++++++++++++++++++
 4 files changed, 93 insertions(+), 9 deletions(-)
 create mode 100644 drivers/i3c/master/mipi-i3c-hci/hci_quirks.c

-- 
2.25.1

[PATCH RESEND v3 1/6] i3c: mipi-i3c-hci: Add MIPI0100 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 MIPI0100 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..24dd4603d6c6 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[] = {
+	{"MIPI0100"},
+	{}
+};
+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 RESEND v3 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 24dd4603d6c6..a16da70bdfe1 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 RESEND v3 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/Makefile     |  3 ++-
 drivers/i3c/master/mipi-i3c-hci/core.c       |  7 +++++++
 drivers/i3c/master/mipi-i3c-hci/hci.h        |  2 ++
 drivers/i3c/master/mipi-i3c-hci/hci_quirks.c | 20 ++++++++++++++++++++
 4 files changed, 31 insertions(+), 1 deletion(-)
 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 a16da70bdfe1..4926fde6087d 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -754,6 +754,13 @@ static int i3c_hci_init(struct i3c_hci *hci)
 		return -EINVAL;
 	}
 
+	/* Initialize quirks for AMD platforms */
+	amd_i3c_hci_quirks_init(hci);
+
+	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);
diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
index f94d95e024be..a7ea37f8f8e0 100644
--- a/drivers/i3c/master/mipi-i3c-hci/hci.h
+++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
@@ -135,11 +135,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 */
 
 
 /* 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_i3c_hci_quirks_init(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..897c0231f585
--- /dev/null
+++ b/drivers/i3c/master/mipi-i3c-hci/hci_quirks.c
@@ -0,0 +1,20 @@
+// 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"
+
+void amd_i3c_hci_quirks_init(struct i3c_hci *hci)
+{
+#if defined(CONFIG_X86)
+	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
+		hci->quirks |= HCI_QUIRK_PIO_MODE;
+#endif
+}
-- 
2.25.1

[PATCH RESEND v3 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.

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 4926fde6087d..0dbaa1333a0c 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 a7ea37f8f8e0..d94e49be3091 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 RESEND v3 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/core.c       |  4 ++++
 drivers/i3c/master/mipi-i3c-hci/hci.h        |  2 ++
 drivers/i3c/master/mipi-i3c-hci/hci_quirks.c | 25 +++++++++++++++++++-
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index 0dbaa1333a0c..23cfdf0059ae 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -786,6 +786,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;
 }
 
diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
index d94e49be3091..5a3b4a014d62 100644
--- a/drivers/i3c/master/mipi-i3c-hci/hci.h
+++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
@@ -141,6 +141,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 */
+#define HCI_QUIRK_OD_PP_TIMING		BIT(3)  /* Set OD and PP timings for AMD platforms */
 
 
 /* global functions */
@@ -148,5 +149,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_i3c_hci_quirks_init(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
index 897c0231f585..6530b9b6128f 100644
--- a/drivers/i3c/master/mipi-i3c-hci/hci_quirks.c
+++ b/drivers/i3c/master/mipi-i3c-hci/hci_quirks.c
@@ -11,10 +11,33 @@
 #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_i3c_hci_quirks_init(struct i3c_hci *hci)
 {
 #if defined(CONFIG_X86)
-	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
+	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
 		hci->quirks |= HCI_QUIRK_PIO_MODE;
+		hci->quirks |= HCI_QUIRK_OD_PP_TIMING;
+	}
 #endif
 }
+
+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 RESEND v3 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       |  4 ++++
 drivers/i3c/master/mipi-i3c-hci/hci.h        |  2 ++
 drivers/i3c/master/mipi-i3c-hci/hci_quirks.c | 12 ++++++++++++
 3 files changed, 18 insertions(+)

diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index 23cfdf0059ae..00c085a1187f 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));
 
diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
index 5a3b4a014d62..6bda266516a1 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 */
@@ -150,5 +151,6 @@ void mipi_i3c_hci_pio_reset(struct i3c_hci *hci);
 void mipi_i3c_hci_dct_index_reset(struct i3c_hci *hci);
 void amd_i3c_hci_quirks_init(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 6530b9b6128f..f906e6476abd 100644
--- a/drivers/i3c/master/mipi-i3c-hci/hci_quirks.c
+++ b/drivers/i3c/master/mipi-i3c-hci/hci_quirks.c
@@ -20,12 +20,15 @@
 #define AMD_SCL_I3C_OD_TIMING          0x00cf00cf
 #define AMD_SCL_I3C_PP_TIMING          0x00160016
 
+#define QUEUE_THLD_CTRL		0xD0
+
 void amd_i3c_hci_quirks_init(struct i3c_hci *hci)
 {
 #if defined(CONFIG_X86)
 	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
 		hci->quirks |= HCI_QUIRK_PIO_MODE;
 		hci->quirks |= HCI_QUIRK_OD_PP_TIMING;
+		hci->quirks |= HCI_QUIRK_RESP_BUF_THLD;
 	}
 #endif
 }
@@ -41,3 +44,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 RESEND v3 2/6] i3c: mipi-i3c-hci: Read HC_CONTROL_PIO_MODE only after i3c hci v1.1

[Jarkko Nikula]

Hi

On 8/7/24 8:23 AM, 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 24dd4603d6c6..a16da70bdfe1 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 {

Here's typo and logic is reversed.

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

[Jarkko Nikula]

On 8/7/24 8:23 AM, Shyam Sundar S K wrote:
> 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.
> 
> 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(-)
> 
Reviewed-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

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

[Jarkko Nikula]

Hi

I Cc'ed Andy and Rafael because of ACPI ID allocation question that came 
to my mind below which I'm not expert enough to answer.

On 8/7/24 8:23 AM, 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 MIPI0100 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..24dd4603d6c6 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[] = {
> +	{"MIPI0100"},
> +	{}
> +};

I started thinking that because of quirks would AMD need to allocate an 
own ACPI ID for each of your HW version and not use generic MIPI ID?

Then passing AMD specific quirks would be easy via driver_data here.

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

[Jarkko Nikula]

Hi

On 8/7/24 8:23 AM, 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>
> ---

...

> +void amd_i3c_hci_quirks_init(struct i3c_hci *hci)
> +{
> +#if defined(CONFIG_X86)
> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
> +		hci->quirks |= HCI_QUIRK_PIO_MODE;
> +#endif
> +}

I was thinking these quirks can be passed as driver_data more cleanly 
and be specific only to affected HW if AMD HW would have an unique ACPI 
ID for each HW version.

Above X86_VENDOR_AMD might be too generic if and when quirks are fixed 
in the future HW :-)

So something like:

static const struct acpi_device_id i3c_hci_acpi_match[] = {
	{"AMDI1234", HCI_QUIRK_PIO_MODE | HCI_QUIRK_OD_PP_TIMING | 
HCI_QUIRK_RESP_BUF_THLD},
	{}
};

and set them in the i3c_hci_probe() as:

hci->quirks = (unsigned long)device_get_match_data(&pdev->dev);

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

[Andy Shevchenko]

On Fri, Aug 09, 2024 at 04:54:18PM +0300, Jarkko Nikula wrote:
> Hi
> 
> I Cc'ed Andy and Rafael because of ACPI ID allocation question that came to
> my mind below which I'm not expert enough to answer.
> 
> On 8/7/24 8:23 AM, 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 MIPI0100 ACPI ID to the list of supported IDs.

When adding a new ACPI ID, always provide the following information:

1) link (in some form) to the official confirmation / documentation for
the allocated ID by the vendor (MIPI in this case) _OR_ (very exceptional!)
why the bad ID had been allocated;

2) are there devices in the wild (on the market) that use the being added ID(s)?

3) excerpt from the device (independently if it's public already, see above,
or not) DSDT ACPI table.

With the given patch it looks to me that you most likely need a local, AMD
specific ID as well.

So, in my ideal world the DSDT should be like

	Device (I3CC)
	{
		Name (_HID, "...") // AMD specific _HID
		Name (_CID, "MIPI0100") // Compatible ID for generic I3C controller
		...
	}

Is this the case? Why not?

P.S. Make sure you Cc me on ACPI ID matters in the future.

-- 
With Best Regards,
Andy Shevchenko

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

[Shyam Sundar S K]

On 8/9/2024 19:48, Andy Shevchenko wrote:
> On Fri, Aug 09, 2024 at 04:54:18PM +0300, Jarkko Nikula wrote:
>> Hi
>>
>> I Cc'ed Andy and Rafael because of ACPI ID allocation question that came to
>> my mind below which I'm not expert enough to answer.
>>
>> On 8/7/24 8:23 AM, 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 MIPI0100 ACPI ID to the list of supported IDs.
> 
> When adding a new ACPI ID, always provide the following information:
> 
> 1) link (in some form) to the official confirmation / documentation for
> the allocated ID by the vendor (MIPI in this case) _OR_ (very exceptional!)
> why the bad ID had been allocated;

Member version:
https://members.mipi.org/wg/All-Members/document/previewpdf/89465

Public version: https://www.mipi.org/mipi-disco-for-i3c-download (this
requires a signup).

Since there is no direct link available for preview, I did not include
them in the commit-msg. But left a note that the MIPI ID is the one as
specified in the MIPI DisCo spec.

> 
> 2) are there devices in the wild (on the market) that use the being added ID(s)?
> 

Not in the wild. But the latest platform will have this support
included. So, these device IDs are crucial for the i3c-hci to be
supported on AMD platforms.


> 3) excerpt from the device (independently if it's public already, see above,
> or not) DSDT ACPI table.
> 
> With the given patch it looks to me that you most likely need a local, AMD
> specific ID as well.
> 
> So, in my ideal world the DSDT should be like
> 
> 	Device (I3CC)
> 	{
> 		Name (_HID, "...") // AMD specific _HID
> 		Name (_CID, "MIPI0100") // Compatible ID for generic I3C controller
> 		...
> 	}
> 
> Is this the case? Why not?

Please refer to the MIPI HCI I3C DisCo specification
(https://members.mipi.org/wg/All-Members/document/previewpdf/89465)
section 5.4. The ASL looks the same in case of AMD.

MSFT says that they want to use MIPI0100 as mentioned in the
specification.

What would you advise?

> 
> P.S. Make sure you Cc me on ACPI ID matters in the future.
> 

Sure, will do.

Thanks,
Shyam

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

[Shyam Sundar S K]

On 8/9/2024 19:25, Jarkko Nikula wrote:
> Hi
> 
> On 8/7/24 8:23 AM, 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>
>> ---
> 
> ...
> 
>> +void amd_i3c_hci_quirks_init(struct i3c_hci *hci)
>> +{
>> +#if defined(CONFIG_X86)
>> +    if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
>> +        hci->quirks |= HCI_QUIRK_PIO_MODE;
>> +#endif
>> +}
> 
> I was thinking these quirks can be passed as driver_data more cleanly
> and be specific only to affected HW if AMD HW would have an unique
> ACPI ID for each HW version.
> 
> Above X86_VENDOR_AMD might be too generic if and when quirks are fixed
> in the future HW :-)
> 
> So something like:
> 
> static const struct acpi_device_id i3c_hci_acpi_match[] = {
>     {"AMDI1234", HCI_QUIRK_PIO_MODE | HCI_QUIRK_OD_PP_TIMING |
> HCI_QUIRK_RESP_BUF_THLD},
>     {}
> };
> 
> and set them in the i3c_hci_probe() as:
> 
> hci->quirks = (unsigned long)device_get_match_data(&pdev->dev);

Nice idea. But only problem is that MSFT wants to have the same ACPI
ID present in the specification.

I have replied to Andy on patch 1/6. Can you please put your remarks
there?

Yeah, agreed that having X86_VENDOR_AMD is too generic, but felt its
good to have additional checks only after the HW is fixed, rather than
being speculative now.. :-)

What would you advise?

Thanks,
Shyam

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

[Shyam Sundar S K]

On 8/9/2024 18:51, Jarkko Nikula wrote:
> Hi
> 
> On 8/7/24 8:23 AM, 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 24dd4603d6c6..a16da70bdfe1 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 {
> 
> Here's typo and logic is reversed.

ah! good catch! thanks, will change this in the next version.

Thanks,
Shyam

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

[Andy Shevchenko]

On Fri, Aug 09, 2024 at 09:02:35PM +0530, Shyam Sundar S K wrote:
> On 8/9/2024 19:48, Andy Shevchenko wrote:
> > On Fri, Aug 09, 2024 at 04:54:18PM +0300, Jarkko Nikula wrote:
> >> On 8/7/24 8:23 AM, Shyam Sundar S K wrote:

...

> > When adding a new ACPI ID, always provide the following information:
> > 
> > 1) link (in some form) to the official confirmation / documentation for
> > the allocated ID by the vendor (MIPI in this case) _OR_ (very exceptional!)
> > why the bad ID had been allocated;
> 
> Member version:
> https://members.mipi.org/wg/All-Members/document/previewpdf/89465
> 
> Public version: https://www.mipi.org/mipi-disco-for-i3c-download (this
> requires a signup).
> 
> Since there is no direct link available for preview, I did not include
> them in the commit-msg. But left a note that the MIPI ID is the one as
> specified in the MIPI DisCo spec.

It's fine.

> > 2) are there devices in the wild (on the market) that use the being added ID(s)?
> 
> Not in the wild. But the latest platform will have this support
> included. So, these device IDs are crucial for the i3c-hci to be
> supported on AMD platforms.

Good, let's do it right then!

> > 3) excerpt from the device (independently if it's public already, see above,
> > or not) DSDT ACPI table.
> > 
> > With the given patch it looks to me that you most likely need a local, AMD
> > specific ID as well.
> > 
> > So, in my ideal world the DSDT should be like
> > 
> > 	Device (I3CC)
> > 	{
> > 		Name (_HID, "...") // AMD specific _HID
> > 		Name (_CID, "MIPI0100") // Compatible ID for generic I3C controller
> > 		...
> > 	}
> > 
> > Is this the case? Why not?
> 
> Please refer to the MIPI HCI I3C DisCo specification
> (https://members.mipi.org/wg/All-Members/document/previewpdf/89465)
> section 5.4. The ASL looks the same in case of AMD.
> 
> MSFT says that they want to use MIPI0100 as mentioned in the
> specification.

MIPI doesn't know how to assign the ACPI ID correctly. But again, what I put in
the above is the correct way of approaching.

> What would you advise?

Since my intuition and experience tells me that the two devices even based on
the same IP are not the same (see word 'quirk' or '.driver_data' or alike in
the kernel sources) the generic ID may not be used for the specific vendor
unless it's _the only_ vendor for the certain IP.

So, please do as I suggested above. And file a error report (and correction
proposal) to the MIPI, so in "5.1 I3C Host Controller ACPI Hardware ID (_HID)"
they should use _CID instead of _HID and add some text like
"Each vendor should dedicate it's own _HID for the platform in question. The
same _HID as _CID may be used if and only if vendor guarantees that there 100%
compatibility with MIPI as described in this and other related documents."

I.o.w. do you 100% guarantee that MIPI HCI I3C DisCo covers all necessary
properties that you need for _your_ hardware? If not, use my approach, if yes,
use the same _HID *and* _CID.

Microsoft should know this as well and much better than MIPI.

-- 
With Best Regards,
Andy Shevchenko

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

[Shyam Sundar S K]

On 8/9/2024 21:27, Andy Shevchenko wrote:
> On Fri, Aug 09, 2024 at 09:02:35PM +0530, Shyam Sundar S K wrote:
>> On 8/9/2024 19:48, Andy Shevchenko wrote:
>>> On Fri, Aug 09, 2024 at 04:54:18PM +0300, Jarkko Nikula wrote:
>>>> On 8/7/24 8:23 AM, Shyam Sundar S K wrote:
> 
> ...
> 
>>> When adding a new ACPI ID, always provide the following information:
>>>
>>> 1) link (in some form) to the official confirmation / documentation for
>>> the allocated ID by the vendor (MIPI in this case) _OR_ (very exceptional!)
>>> why the bad ID had been allocated;
>>
>> Member version:
>> https://members.mipi.org/wg/All-Members/document/previewpdf/89465
>>
>> Public version: https://www.mipi.org/mipi-disco-for-i3c-download (this
>> requires a signup).
>>
>> Since there is no direct link available for preview, I did not include
>> them in the commit-msg. But left a note that the MIPI ID is the one as
>> specified in the MIPI DisCo spec.
> 
> It's fine.
> 
>>> 2) are there devices in the wild (on the market) that use the being added ID(s)?
>>
>> Not in the wild. But the latest platform will have this support
>> included. So, these device IDs are crucial for the i3c-hci to be
>> supported on AMD platforms.
> 
> Good, let's do it right then!
> 
>>> 3) excerpt from the device (independently if it's public already, see above,
>>> or not) DSDT ACPI table.
>>>
>>> With the given patch it looks to me that you most likely need a local, AMD
>>> specific ID as well.
>>>
>>> So, in my ideal world the DSDT should be like
>>>
>>> 	Device (I3CC)
>>> 	{
>>> 		Name (_HID, "...") // AMD specific _HID
>>> 		Name (_CID, "MIPI0100") // Compatible ID for generic I3C controller
>>> 		...
>>> 	}
>>>
>>> Is this the case? Why not?
>>
>> Please refer to the MIPI HCI I3C DisCo specification
>> (https://members.mipi.org/wg/All-Members/document/previewpdf/89465)
>> section 5.4. The ASL looks the same in case of AMD.
>>
>> MSFT says that they want to use MIPI0100 as mentioned in the
>> specification.
> 
> MIPI doesn't know how to assign the ACPI ID correctly. But again, what I put in
> the above is the correct way of approaching.
> 
>> What would you advise?
> 
> Since my intuition and experience tells me that the two devices even based on
> the same IP are not the same (see word 'quirk' or '.driver_data' or alike in
> the kernel sources) the generic ID may not be used for the specific vendor
> unless it's _the only_ vendor for the certain IP.
> 
> So, please do as I suggested above. And file a error report (and correction
> proposal) to the MIPI, so in "5.1 I3C Host Controller ACPI Hardware ID (_HID)"

Thank you. I shall file a error report soon to get it corrected.

> they should use _CID instead of _HID and add some text like
> "Each vendor should dedicate it's own _HID for the platform in question. The
> same _HID as _CID may be used if and only if vendor guarantees that there 100%
> compatibility with MIPI as described in this and other related documents."
> 
> I.o.w. do you 100% guarantee that MIPI HCI I3C DisCo covers all necessary
> properties that you need for _your_ hardware? If not, use my approach, if yes,
> use the same _HID *and* _CID.
> 
> Microsoft should know this as well and much better than MIPI.
> 

Agree with your thoughts. I will respin based on your remarks after
internal discussion. If it requires _HID to be MIPI0100, I will come
back with answers.

Thanks,
Shyam

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

[Alexandre Belloni]

On 09/08/2024 18:57:04+0300, Andy Shevchenko wrote:
> > Please refer to the MIPI HCI I3C DisCo specification
> > (https://members.mipi.org/wg/All-Members/document/previewpdf/89465)
> > section 5.4. The ASL looks the same in case of AMD.
> > 
> > MSFT says that they want to use MIPI0100 as mentioned in the
> > specification.
> 
> MIPI doesn't know how to assign the ACPI ID correctly. But again, what I put in
> the above is the correct way of approaching.
> 
> > What would you advise?
> 
> Since my intuition and experience tells me that the two devices even based on
> the same IP are not the same (see word 'quirk' or '.driver_data' or alike in
> the kernel sources) the generic ID may not be used for the specific vendor
> unless it's _the only_ vendor for the certain IP.

Just to be clear, the HCI defines the register interface to the IP but
not the IP itself, this is just like the various USB and SD HCIs. So we
will definitively see quirks as implementers will interpret the
interface differently (and so I agree with everything that was said ;) )

> 
> So, please do as I suggested above. And file a error report (and correction
> proposal) to the MIPI, so in "5.1 I3C Host Controller ACPI Hardware ID (_HID)"
> they should use _CID instead of _HID and add some text like
> "Each vendor should dedicate it's own _HID for the platform in question. The
> same _HID as _CID may be used if and only if vendor guarantees that there 100%
> compatibility with MIPI as described in this and other related documents."
> 
> I.o.w. do you 100% guarantee that MIPI HCI I3C DisCo covers all necessary
> properties that you need for _your_ hardware? If not, use my approach, if yes,
> use the same _HID *and* _CID.
> 
> Microsoft should know this as well and much better than MIPI.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Jarkko Nikula]

Hi

On 8/9/24 6:44 PM, Shyam Sundar S K wrote:
>> I was thinking these quirks can be passed as driver_data more cleanly
>> and be specific only to affected HW if AMD HW would have an unique
>> ACPI ID for each HW version.
>>
>> Above X86_VENDOR_AMD might be too generic if and when quirks are fixed
>> in the future HW :-)
>>
>> So something like:
>>
>> static const struct acpi_device_id i3c_hci_acpi_match[] = {
>>      {"AMDI1234", HCI_QUIRK_PIO_MODE | HCI_QUIRK_OD_PP_TIMING |
>> HCI_QUIRK_RESP_BUF_THLD},
>>      {}
>> };
>>
>> and set them in the i3c_hci_probe() as:
>>
>> hci->quirks = (unsigned long)device_get_match_data(&pdev->dev);
> 
> Nice idea. But only problem is that MSFT wants to have the same ACPI
> ID present in the specification.
> 
> I have replied to Andy on patch 1/6. Can you please put your remarks
> there?
> 
Well this is implementation detail later in the series and I found it 
better to focus ACPI ID discussion in 1/6.

> Yeah, agreed that having X86_VENDOR_AMD is too generic, but felt its
> good to have additional checks only after the HW is fixed, rather than
> being speculative now.. :-)
> 
> What would you advise?
> Most probably there will be future HW with either exactly same set of 
quirks, reduced quirks or new quirks and X86_VENDOR_AMD test will work 
only with the first case :-)

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

[Shyam Sundar S K]

Hi,

On 8/12/2024 14:47, Jarkko Nikula wrote:
> Hi
> 
> On 8/9/24 6:44 PM, Shyam Sundar S K wrote:
>>> I was thinking these quirks can be passed as driver_data more cleanly
>>> and be specific only to affected HW if AMD HW would have an unique
>>> ACPI ID for each HW version.
>>>
>>> Above X86_VENDOR_AMD might be too generic if and when quirks are fixed
>>> in the future HW :-)
>>>
>>> So something like:
>>>
>>> static const struct acpi_device_id i3c_hci_acpi_match[] = {
>>>      {"AMDI1234", HCI_QUIRK_PIO_MODE | HCI_QUIRK_OD_PP_TIMING |
>>> HCI_QUIRK_RESP_BUF_THLD},
>>>      {}
>>> };
>>>
>>> and set them in the i3c_hci_probe() as:
>>>
>>> hci->quirks = (unsigned long)device_get_match_data(&pdev->dev);
>>
>> Nice idea. But only problem is that MSFT wants to have the same ACPI
>> ID present in the specification.
>>
>> I have replied to Andy on patch 1/6. Can you please put your remarks
>> there?
>>
> Well this is implementation detail later in the series and I found it
> better to focus ACPI ID discussion in 1/6.
> 
>> Yeah, agreed that having X86_VENDOR_AMD is too generic, but felt its
>> good to have additional checks only after the HW is fixed, rather than
>> being speculative now.. :-)
>>
>> What would you advise?
>> Most probably there will be future HW with either exactly same set of 
> quirks, reduced quirks or new quirks and X86_VENDOR_AMD test will work
> only with the first case :-)

I can add an additional check with the CPU ID and distinguish them(so
the quirk gets applied to the affected HW versions) and just not
restrict to X86_VENDOR_AMD, would that be fine with you?

OTOH, Since these are quirks (where its a broken hardware problems)
and the idea you suggested is related to driver data (where driver
data is meant to store private information about the device)

static const struct acpi_device_id i3c_hci_acpi_match[] = {
     {"AMDI1234", HCI_QUIRK_PIO_MODE | HCI_QUIRK_OD_PP_TIMING |
HCI_QUIRK_RESP_BUF_THLD},
     {}
};

does that not conflict? quirk vs driver data?

I am OK to implement it the way you prefer :-)

Thanks,
Shyam

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

[Andy Shevchenko]

On Fri, Aug 09, 2024 at 08:39:40PM +0200, Alexandre Belloni wrote:
> On 09/08/2024 18:57:04+0300, Andy Shevchenko wrote:
> > > Please refer to the MIPI HCI I3C DisCo specification
> > > (https://members.mipi.org/wg/All-Members/document/previewpdf/89465)
> > > section 5.4. The ASL looks the same in case of AMD.
> > > 
> > > MSFT says that they want to use MIPI0100 as mentioned in the
> > > specification.
> > 
> > MIPI doesn't know how to assign the ACPI ID correctly. But again, what I put in
> > the above is the correct way of approaching.
> > 
> > > What would you advise?
> > 
> > Since my intuition and experience tells me that the two devices even based on
> > the same IP are not the same (see word 'quirk' or '.driver_data' or alike in
> > the kernel sources) the generic ID may not be used for the specific vendor
> > unless it's _the only_ vendor for the certain IP.
> 
> Just to be clear, the HCI defines the register interface to the IP but
> not the IP itself, this is just like the various USB and SD HCIs.

Thank you for this elaboration, it's a crucial aspect!

> So we will definitively see quirks as implementers will interpret the
> interface differently

With the above remark this makes a lot of sense.

> (and so I agree with everything that was said ;) )

> > So, please do as I suggested above. And file a error report (and correction
> > proposal) to the MIPI, so in "5.1 I3C Host Controller ACPI Hardware ID (_HID)"
> > they should use _CID instead of _HID and add some text like
> > "Each vendor should dedicate it's own _HID for the platform in question. The
> > same _HID as _CID may be used if and only if vendor guarantees that there 100%
> > compatibility with MIPI as described in this and other related documents."
> > 
> > I.o.w. do you 100% guarantee that MIPI HCI I3C DisCo covers all necessary
> > properties that you need for _your_ hardware? If not, use my approach, if yes,
> > use the same _HID *and* _CID.
> > 
> > Microsoft should know this as well and much better than MIPI.

-- 
With Best Regards,
Andy Shevchenko

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

[Shyam Sundar S K]

On 8/12/2024 15:02, Shyam Sundar S K wrote:
> Hi,
> 
> On 8/12/2024 14:47, Jarkko Nikula wrote:
>> Hi
>>
>> On 8/9/24 6:44 PM, Shyam Sundar S K wrote:
>>>> I was thinking these quirks can be passed as driver_data more cleanly
>>>> and be specific only to affected HW if AMD HW would have an unique
>>>> ACPI ID for each HW version.
>>>>
>>>> Above X86_VENDOR_AMD might be too generic if and when quirks are fixed
>>>> in the future HW :-)
>>>>
>>>> So something like:
>>>>
>>>> static const struct acpi_device_id i3c_hci_acpi_match[] = {
>>>>      {"AMDI1234", HCI_QUIRK_PIO_MODE | HCI_QUIRK_OD_PP_TIMING |
>>>> HCI_QUIRK_RESP_BUF_THLD},
>>>>      {}
>>>> };
>>>>
>>>> and set them in the i3c_hci_probe() as:
>>>>
>>>> hci->quirks = (unsigned long)device_get_match_data(&pdev->dev);
>>>
>>> Nice idea. But only problem is that MSFT wants to have the same ACPI
>>> ID present in the specification.
>>>
>>> I have replied to Andy on patch 1/6. Can you please put your remarks
>>> there?
>>>
>> Well this is implementation detail later in the series and I found it
>> better to focus ACPI ID discussion in 1/6.
>>
>>> Yeah, agreed that having X86_VENDOR_AMD is too generic, but felt its
>>> good to have additional checks only after the HW is fixed, rather than
>>> being speculative now.. :-)
>>>
>>> What would you advise?
>>> Most probably there will be future HW with either exactly same set of 
>> quirks, reduced quirks or new quirks and X86_VENDOR_AMD test will work
>> only with the first case :-)
> 
> I can add an additional check with the CPU ID and distinguish them(so
> the quirk gets applied to the affected HW versions) and just not
> restrict to X86_VENDOR_AMD, would that be fine with you?
> 
> OTOH, Since these are quirks (where its a broken hardware problems)
> and the idea you suggested is related to driver data (where driver
> data is meant to store private information about the device)
> 
> static const struct acpi_device_id i3c_hci_acpi_match[] = {
>      {"AMDI1234", HCI_QUIRK_PIO_MODE | HCI_QUIRK_OD_PP_TIMING |
> HCI_QUIRK_RESP_BUF_THLD},
>      {}
> };
> 
> does that not conflict? quirk vs driver data?
> 
> I am OK to implement it the way you prefer :-)

Jarkko, any feedback on this?

Thanks,
Shyam

> 
> Thanks,
> Shyam
> 
> 
> 

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

[Jarkko Nikula]

On 8/19/24 9:41 AM, Shyam Sundar S K wrote:
>> I can add an additional check with the CPU ID and distinguish them(so
>> the quirk gets applied to the affected HW versions) and just not
>> restrict to X86_VENDOR_AMD, would that be fine with you?
>>
>> OTOH, Since these are quirks (where its a broken hardware problems)
>> and the idea you suggested is related to driver data (where driver
>> data is meant to store private information about the device)
>>
>> static const struct acpi_device_id i3c_hci_acpi_match[] = {
>>       {"AMDI1234", HCI_QUIRK_PIO_MODE | HCI_QUIRK_OD_PP_TIMING |
>> HCI_QUIRK_RESP_BUF_THLD},
>>       {}
>> };
>>
>> does that not conflict? quirk vs driver data?
>>
>> I am OK to implement it the way you prefer :-)
> 
> Jarkko, any feedback on this?
> 
Sorry, forgot to reply... What do you mean about conflict? So if driver 
data would pass quirk bits as above and set only to unique ACPI ID 
specific to that HW then there is no reason to check CPU ID later in the 
code.

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

[Shyam Sundar S K]

On 8/19/2024 16:40, Jarkko Nikula wrote:
> On 8/19/24 9:41 AM, Shyam Sundar S K wrote:
>>> I can add an additional check with the CPU ID and distinguish them(so
>>> the quirk gets applied to the affected HW versions) and just not
>>> restrict to X86_VENDOR_AMD, would that be fine with you?
>>>
>>> OTOH, Since these are quirks (where its a broken hardware problems)
>>> and the idea you suggested is related to driver data (where driver
>>> data is meant to store private information about the device)
>>>
>>> static const struct acpi_device_id i3c_hci_acpi_match[] = {
>>>       {"AMDI1234", HCI_QUIRK_PIO_MODE | HCI_QUIRK_OD_PP_TIMING |
>>> HCI_QUIRK_RESP_BUF_THLD},
>>>       {}
>>> };
>>>
>>> does that not conflict? quirk vs driver data?
>>>
>>> I am OK to implement it the way you prefer :-)
>>
>> Jarkko, any feedback on this?
>>
> Sorry, forgot to reply... What do you mean about conflict? So if
> driver data would pass quirk bits as above and set only to unique ACPI
> ID specific to that HW then there is no reason to check CPU ID later
> in the code.

OK. Let me respin a new version.

Thanks,
Shyam