[PATCH v3 0/5] Introduce AMD ASF Controller Support to the i2c-piix4 driver

[PATCH v3 0/5] Introduce AMD ASF Controller Support to the i2c-piix4 driver

[Shyam Sundar S K]

The AMD ASF (Alert Standard Format) function block is essentially an SMBus
controller with built-in ASF functionality. It features two pins SCL1 and
SDA1 that facilitate communication with other SMBus devices. This dual
capability allows the ASF controller to issue generic SMBus packets and
communicate with the DASH controller using MCTP over ASF. Additionally,
the ASF controller supports remote commands defined by the ASF
specification, such as shutdown, reset, power-up, and power-down, without
requiring any software interaction.

The concept is to enable a remote system to communicate with the target
system over the network. The local network controller, such as an Ethernet
MAC, receives remote packets and relays the commands to the FCH
(Fusion Controller Hub) through the ASF. Examples of these commands
include shutdown and reset. Since ASF uses the SMBus protocol, this
controller can be configured as a secondary SMBus controller.

This series of updates is focused on integrating the capabilities of AMD's
ASF (Alert Standard Format) controller into the i2c-piix4 driver.

v3:
----
 - Fix LKP reported issue by adding 'depends on X86'
 - Drop callback when using acpi_dev_get_resources()
 - Address other remarks from Andy on v2.

v2:
----
 - Change function signature from u8 to enum
 - Use default case in switch
 - Use acpi_dev_get_resources() and drop devm_kzalloc() usage
 - Fix LKP reported issues
 - Address other minor remarks from Andy and Andi Shyti

Shyam Sundar S K (5):
  i2c: piix4: Allow more than two algo selection for SMBus
  i2c: piix4: Add i2c_algorithm operations to support AMD ASF with SMBus
  i2c: piix4: Add ACPI support for ASF SMBus device
  i2c: piix4: Adjust the SMBus debug message
  i2c: piix4: Clear remote IRR bit to get successive interrupt

 drivers/i2c/busses/Kconfig     |   3 +-
 drivers/i2c/busses/i2c-piix4.c | 396 ++++++++++++++++++++++++++++++++-
 2 files changed, 389 insertions(+), 10 deletions(-)

-- 
2.25.1

[PATCH v3 1/5] i2c: piix4: Allow more than two algo selection for SMBus

[Shyam Sundar S K]

The current implementation of the piix4 driver has a limitation in that it
only supports two algorithms for the I2C adapter:

- SB800 Algorithm: This is used for newer AMD chipsets.
- Legacy PIIX4 Algorithm: This is used for older systems.

The selection between these two algorithms is controlled by a boolean
parameter in the piix4_add_adapter() function. This means that the driver
can only toggle between these two options, which limits its flexibility.

AMD's SMBus (System Management Bus) implementation supports additional
functionalities, such as ASF (Alert Standard Format). ASF is a protocol
used for system management and monitoring, which can be part of the SoC
(System on Chip). To support ASF or any other future algorithms, the
driver needs to be more flexible in its algorithm selection.

The proposed change involves modifying the piix4_add_adapter() function to
accommodate more than just two algorithm selections. Instead of using a
boolean parameter to select between two algorithms, the function signature
will be updated to allow for multiple algorithm options.

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i2c/busses/i2c-piix4.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 4e32d57ae0bf..d56083e58a2d 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -165,6 +165,11 @@ struct sb800_mmio_cfg {
 	bool use_mmio;
 };
 
+enum piix4_algo {
+	PIIX4_SB800,
+	PIIX4_SMBUS,
+};
+
 struct i2c_piix4_adapdata {
 	unsigned short smba;
 
@@ -173,6 +178,7 @@ struct i2c_piix4_adapdata {
 	bool notify_imc;
 	u8 port;		/* Port number, shifted */
 	struct sb800_mmio_cfg mmio_cfg;
+	u8 algo_select;
 };
 
 static int piix4_sb800_region_request(struct device *dev,
@@ -929,7 +935,7 @@ static struct i2c_adapter *piix4_aux_adapter;
 static int piix4_adapter_count;
 
 static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
-			     bool sb800_main, u8 port, bool notify_imc,
+			     enum piix4_algo algo, u8 port, bool notify_imc,
 			     u8 hw_port_nr, const char *name,
 			     struct i2c_adapter **padap)
 {
@@ -945,8 +951,18 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 
 	adap->owner = THIS_MODULE;
 	adap->class = I2C_CLASS_HWMON;
-	adap->algo = sb800_main ? &piix4_smbus_algorithm_sb800
-				: &smbus_algorithm;
+
+	switch (algo) {
+	case PIIX4_SMBUS:
+		adap->algo = &smbus_algorithm;
+		break;
+	case PIIX4_SB800:
+		adap->algo = &piix4_smbus_algorithm_sb800;
+		break;
+	default:
+		dev_err(&dev->dev, "Unsupported SMBus algorithm\n");
+		return -EINVAL;
+	}
 
 	adapdata = kzalloc(sizeof(*adapdata), GFP_KERNEL);
 	if (adapdata == NULL) {
@@ -957,7 +973,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 
 	adapdata->mmio_cfg.use_mmio = piix4_sb800_use_mmio(dev);
 	adapdata->smba = smba;
-	adapdata->sb800_main = sb800_main;
+	adapdata->algo_select = algo;
 	adapdata->port = port << piix4_port_shift_sb800;
 	adapdata->notify_imc = notify_imc;
 
@@ -1013,7 +1029,7 @@ static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba,
 	for (port = 0; port < piix4_adapter_count; port++) {
 		u8 hw_port_nr = port == 0 ? 0 : port + 1;
 
-		retval = piix4_add_adapter(dev, smba, true, port, notify_imc,
+		retval = piix4_add_adapter(dev, smba, PIIX4_SB800, port, notify_imc,
 					   hw_port_nr,
 					   piix4_main_port_names_sb800[port],
 					   &piix4_main_adapters[port]);
@@ -1085,7 +1101,7 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 			return retval;
 
 		/* Try to register main SMBus adapter, give up if we can't */
-		retval = piix4_add_adapter(dev, retval, false, 0, false, 0,
+		retval = piix4_add_adapter(dev, retval, PIIX4_SMBUS, 0, false, 0,
 					   "", &piix4_main_adapters[0]);
 		if (retval < 0)
 			return retval;
@@ -1114,7 +1130,7 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	if (retval > 0) {
 		/* Try to add the aux adapter if it exists,
 		 * piix4_add_adapter will clean up if this fails */
-		piix4_add_adapter(dev, retval, false, 0, false, 1,
+		piix4_add_adapter(dev, retval, PIIX4_SMBUS, 0, false, 1,
 				  is_sb800 ? piix4_aux_port_name_sb800 : "",
 				  &piix4_aux_adapter);
 	}
-- 
2.25.1

[PATCH v3 2/5] i2c: piix4: Add i2c_algorithm operations to support AMD ASF with SMBus

[Shyam Sundar S K]

Implement the i2c_algorithm operations to enable support for AMD ASF
(Alert Standard Format) with SMBus. This enhancement includes:

- Adding functionality to identify and select the supported ASF functions.
- Implementing mechanisms for registering and deregistering I2C slave
  devices.
- Providing support for data transfer operations over ASF.

These changes will extend the piix4 driver to accommodate the additional
capabilities provided by AMD's ASF Controller.

Additionally, include a 'select' Kconfig entry for CONFIG_I2C_PIIX4, as
the current patch utilizes reg_slave and unreg_slave callbacks, which are
controlled by IS_ENABLED(CONFIG_I2C_SLAVE).

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i2c/busses/Kconfig     |   1 +
 drivers/i2c/busses/i2c-piix4.c | 184 +++++++++++++++++++++++++++++++++
 2 files changed, 185 insertions(+)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index a22f9125322a..10ad839bf4a2 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -197,6 +197,7 @@ config I2C_PIIX4
 	tristate "Intel PIIX4 and compatible (ATI/AMD/Serverworks/Broadcom/SMSC)"
 	depends on PCI && HAS_IOPORT
 	select I2C_SMBUS
+	select I2C_SLAVE
 	help
 	  If you say yes to this option, support will be included for the Intel
 	  PIIX4 family of mainboard I2C interfaces.  Specifically, the following
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index d56083e58a2d..003cb04312cf 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -50,6 +50,22 @@
 #define SMBSLVEVT	(0xA + piix4_smba)
 #define SMBSLVDAT	(0xC + piix4_smba)
 
+/* SB800 ASF register bits */
+#define SB800_ASF_SLV_LISTN	0
+#define SB800_ASF_SLV_INTR	1
+#define SB800_ASF_SLV_RST	4
+#define SB800_ASF_PEC_SP	5
+#define SB800_ASF_DATA_EN	7
+#define SB800_ASF_MSTR_EN	16
+#define SB800_ASF_CLK_EN	17
+
+/* SB800 ASF address offsets */
+#define ASFLISADDR		(9 + piix4_smba)
+#define ASFSTA			(0xA + piix4_smba)
+#define ASFSLVSTA		(0xD + piix4_smba)
+#define ASFDATABNKSEL		(0x13 + piix4_smba)
+#define ASFSLVEN		(0x15 + piix4_smba)
+
 /* count for request_region */
 #define SMBIOSIZE	9
 
@@ -101,6 +117,7 @@
 
 #define SB800_PIIX4_FCH_PM_ADDR			0xFED80300
 #define SB800_PIIX4_FCH_PM_SIZE			8
+#define SB800_ASF_BLOCK_MAX_BYTES		72
 
 /* insmod parameters */
 
@@ -168,6 +185,7 @@ struct sb800_mmio_cfg {
 enum piix4_algo {
 	PIIX4_SB800,
 	PIIX4_SMBUS,
+	SMBUS_ASF,
 };
 
 struct i2c_piix4_adapdata {
@@ -179,6 +197,7 @@ struct i2c_piix4_adapdata {
 	u8 port;		/* Port number, shifted */
 	struct sb800_mmio_cfg mmio_cfg;
 	u8 algo_select;
+	struct i2c_client *slave;
 };
 
 static int piix4_sb800_region_request(struct device *dev,
@@ -887,6 +906,168 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 	return retval;
 }
 
+static void sb800_asf_update_bits(unsigned short piix4_smba, u8 bit, unsigned long offset, bool set)
+{
+	unsigned long reg;
+
+	reg = inb_p(offset);
+	if (set)
+		set_bit(bit, &reg);
+	else
+		clear_bit(bit, &reg);
+	outb_p(reg, offset);
+}
+
+static void sb800_asf_update_bytes(struct i2c_piix4_adapdata *adap, u8 bit, bool set)
+{
+	unsigned long reg;
+
+	reg = ioread32(adap->mmio_cfg.addr);
+	if (set)
+		set_bit(bit, &reg);
+	else
+		clear_bit(bit, &reg);
+	iowrite32(reg, adap->mmio_cfg.addr);
+}
+
+static void sb800_asf_setup_slave(struct i2c_piix4_adapdata *adap)
+{
+	unsigned short piix4_smba = adap->smba;
+
+	/* Reset both host and slave before setting up */
+	outb_p(0, SMBHSTSTS);
+	outb_p(0, ASFSLVSTA);
+	outb_p(0, ASFSTA);
+
+	/* Update slave address */
+	sb800_asf_update_bits(piix4_smba, SB800_ASF_SLV_LISTN, ASFLISADDR, true);
+	/* Enable slave and set the clock */
+	sb800_asf_update_bytes(adap, SB800_ASF_MSTR_EN, false);
+	sb800_asf_update_bytes(adap, SB800_ASF_CLK_EN, true);
+	/* Enable slave interrupt */
+	sb800_asf_update_bits(piix4_smba, SB800_ASF_SLV_INTR, ASFSLVEN, true);
+	sb800_asf_update_bits(piix4_smba, SB800_ASF_SLV_RST, ASFSLVEN, false);
+	/* Enable PEC and PEC append */
+	sb800_asf_update_bits(piix4_smba, SB800_ASF_DATA_EN, SMBHSTCNT, true);
+	sb800_asf_update_bits(piix4_smba, SB800_ASF_PEC_SP, SMBHSTCNT, true);
+}
+
+static s32 sb800_asf_access(struct i2c_adapter *adap, u16 addr, u8 command, u8 *data)
+{
+	struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(adap);
+	unsigned short piix4_smba = adapdata->smba;
+	u8 len;
+	int i;
+
+	outb_p((addr << 1), SMBHSTADD);
+	outb_p(command, SMBHSTCMD);
+	len = data[0];
+	if (len == 0 || len > SB800_ASF_BLOCK_MAX_BYTES)
+		return -EINVAL;
+
+	outb_p(len, SMBHSTDAT0);
+	inb_p(SMBHSTCNT); /* Reset SMBBLKDAT */
+	for (i = 1; i <= len; i++)
+		outb_p(data[i], SMBBLKDAT);
+
+	outb_p(PIIX4_BLOCK_DATA, SMBHSTCNT);
+	/* Enable PEC and PEC append */
+	sb800_asf_update_bits(piix4_smba, SB800_ASF_DATA_EN, SMBHSTCNT, true);
+	sb800_asf_update_bits(piix4_smba, SB800_ASF_PEC_SP, SMBHSTCNT, true);
+
+	return piix4_transaction(adap);
+}
+
+static int sb800_asf_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+{
+	struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(adap);
+	unsigned short piix4_smba = adapdata->smba;
+	u8 asf_data[SB800_ASF_BLOCK_MAX_BYTES];
+	struct i2c_msg *dev_msgs = msgs;
+	u8 prev_port;
+	int ret;
+
+	if (msgs->flags & I2C_M_RD) {
+		dev_err(&adap->dev, "Read not supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	/* Exclude the receive header and PEC */
+	if (msgs->len > SB800_ASF_BLOCK_MAX_BYTES - 3) {
+		dev_err(&adap->dev, "ASF max message length exceeded\n");
+		return -EOPNOTSUPP;
+	}
+
+	asf_data[0] = dev_msgs->len;
+	memcpy(asf_data + 1, dev_msgs[0].buf, dev_msgs->len);
+
+	ret = piix4_sb800_region_request(&adap->dev, &adapdata->mmio_cfg);
+	if (ret)
+		return ret;
+
+	sb800_asf_update_bits(piix4_smba, SB800_ASF_SLV_RST, ASFSLVEN, true);
+	sb800_asf_update_bits(piix4_smba, SB800_ASF_SLV_LISTN, ASFLISADDR, false);
+	/* Clear ASF slave status */
+	outb_p(0, ASFSLVSTA);
+
+	/* Enable ASF SMBus master function */
+	sb800_asf_update_bytes(adapdata, SB800_ASF_MSTR_EN, true);
+	prev_port = piix4_sb800_port_sel(adapdata->port, &adapdata->mmio_cfg);
+	ret = sb800_asf_access(adap, msgs->addr, msgs[0].buf[0], asf_data);
+	piix4_sb800_port_sel(prev_port, &adapdata->mmio_cfg);
+	sb800_asf_setup_slave(adapdata);
+	piix4_sb800_region_release(&adap->dev, &adapdata->mmio_cfg);
+	return ret;
+}
+
+static int sb800_asf_reg_slave(struct i2c_client *slave)
+{
+	struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(slave->adapter);
+	unsigned short piix4_smba = adapdata->smba;
+	int ret;
+	u8 reg;
+
+	if (adapdata->slave)
+		return -EBUSY;
+
+	ret = piix4_sb800_region_request(&slave->dev, &adapdata->mmio_cfg);
+	if (ret)
+		return ret;
+
+	reg = (slave->addr << 1) | BIT(0);
+	outb_p(reg, ASFLISADDR);
+
+	sb800_asf_setup_slave(adapdata);
+	adapdata->slave = slave;
+	sb800_asf_update_bits(piix4_smba, SB800_ASF_DATA_EN, ASFDATABNKSEL, false);
+	piix4_sb800_region_release(&slave->dev, &adapdata->mmio_cfg);
+	return 0;
+}
+
+static int sb800_asf_unreg_slave(struct i2c_client *slave)
+{
+	struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(slave->adapter);
+	unsigned short piix4_smba = adapdata->smba;
+
+	sb800_asf_update_bits(piix4_smba, SB800_ASF_SLV_INTR, ASFSLVEN, false);
+	sb800_asf_update_bits(piix4_smba, SB800_ASF_SLV_RST, ASFSLVEN, true);
+	adapdata->slave = NULL;
+	return 0;
+}
+
+static u32 sb800_asf_func(struct i2c_adapter *adapter)
+{
+	return I2C_FUNC_SMBUS_BLOCK_DATA | I2C_FUNC_SMBUS_BYTE |
+		I2C_FUNC_SLAVE | I2C_FUNC_SMBUS_WRITE_BLOCK_DATA | I2C_FUNC_SMBUS_PEC;
+}
+
+static const struct i2c_algorithm sb800_asf_smbus_algorithm = {
+	.master_xfer = sb800_asf_xfer,
+	.reg_slave = sb800_asf_reg_slave,
+	.unreg_slave = sb800_asf_unreg_slave,
+	.functionality = sb800_asf_func,
+};
+
 static u32 piix4_func(struct i2c_adapter *adapter)
 {
 	return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
@@ -959,6 +1140,9 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 	case PIIX4_SB800:
 		adap->algo = &piix4_smbus_algorithm_sb800;
 		break;
+	case SMBUS_ASF:
+		adap->algo = &sb800_asf_smbus_algorithm;
+		break;
 	default:
 		dev_err(&dev->dev, "Unsupported SMBus algorithm\n");
 		return -EINVAL;
-- 
2.25.1

[PATCH v3 3/5] i2c: piix4: Add ACPI support for ASF SMBus device

[Shyam Sundar S K]

The AMD ASF controller is presented to the operating system as an ACPI
device. The piix4 driver can obtain the ASF handle through ACPI to
retrieve information about the ASF controller's attributes, such as the
ASF address space and interrupt number, and to handle ASF interrupts.

Currently, the piix4 driver assumes that a specific port address is
designated for AUX operations. However, with the introduction of ASF, the
same port address may also be used by the ASF controller. Therefore, a
check needs to be added to ensure that if ASF is advertised and enabled in
ACPI, the AUX port is not set up.

Additionally, include a 'depends on X86' Kconfig entry for
CONFIG_I2C_PIIX4, as the current patch utilizes acpi_dev_get_resources(),
which is compiled only when CONFIG_ACPI is enabled, and CONFIG_ACPI
depends on CONFIG_X86.

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

 drivers/i2c/busses/Kconfig     |   2 +-
 drivers/i2c/busses/i2c-piix4.c | 167 ++++++++++++++++++++++++++++++++-
 2 files changed, 167 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 10ad839bf4a2..7d080a009ee3 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -195,7 +195,7 @@ config I2C_ISMT
 
 config I2C_PIIX4
 	tristate "Intel PIIX4 and compatible (ATI/AMD/Serverworks/Broadcom/SMSC)"
-	depends on PCI && HAS_IOPORT
+	depends on PCI && HAS_IOPORT && X86
 	select I2C_SMBUS
 	select I2C_SLAVE
 	help
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 003cb04312cf..2bf9611d864a 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -60,9 +60,12 @@
 #define SB800_ASF_CLK_EN	17
 
 /* SB800 ASF address offsets */
+#define ASFINDEX		(7 + piix4_smba)
 #define ASFLISADDR		(9 + piix4_smba)
 #define ASFSTA			(0xA + piix4_smba)
 #define ASFSLVSTA		(0xD + piix4_smba)
+#define ASFDATARWPTR		(0x11 + piix4_smba)
+#define ASFSETDATARDPTR		(0x12 + piix4_smba)
 #define ASFDATABNKSEL		(0x13 + piix4_smba)
 #define ASFSLVEN		(0x15 + piix4_smba)
 
@@ -118,6 +121,8 @@
 #define SB800_PIIX4_FCH_PM_ADDR			0xFED80300
 #define SB800_PIIX4_FCH_PM_SIZE			8
 #define SB800_ASF_BLOCK_MAX_BYTES		72
+#define SB800_ASF_ERROR_STATUS			0xE
+#define SB800_ASF_ACPI_PATH			"\\_SB.ASFC"
 
 /* insmod parameters */
 
@@ -182,6 +187,11 @@ struct sb800_mmio_cfg {
 	bool use_mmio;
 };
 
+struct sb800_asf_data {
+	unsigned short addr;
+	int irq;
+};
+
 enum piix4_algo {
 	PIIX4_SB800,
 	PIIX4_SMBUS,
@@ -194,10 +204,12 @@ struct i2c_piix4_adapdata {
 	/* SB800 */
 	bool sb800_main;
 	bool notify_imc;
+	bool is_asf;
 	u8 port;		/* Port number, shifted */
 	struct sb800_mmio_cfg mmio_cfg;
 	u8 algo_select;
 	struct i2c_client *slave;
+	struct delayed_work work_buf;
 };
 
 static int piix4_sb800_region_request(struct device *dev,
@@ -906,6 +918,67 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 	return retval;
 }
 
+static void sb800_asf_process_slave(struct work_struct *work)
+{
+	struct i2c_piix4_adapdata *adapdata =
+		container_of(work, struct i2c_piix4_adapdata, work_buf.work);
+	unsigned short piix4_smba = adapdata->smba;
+	u8 data[SB800_ASF_BLOCK_MAX_BYTES];
+	u8 bank, reg, cmd = 0;
+	u8 len, val = 0;
+	int i;
+
+	/* Read slave status register */
+	reg = inb_p(ASFSLVSTA);
+
+	/* Check if no error bits are set in slave status register */
+	if (reg & SB800_ASF_ERROR_STATUS) {
+		/* Set bank as full */
+		reg = reg | GENMASK(3, 2);
+		outb_p(reg, ASFDATABNKSEL);
+	} else {
+		/* Read data bank */
+		reg = inb_p(ASFDATABNKSEL);
+		bank = (reg & BIT(3)) >> 3;
+
+		/* Set read data bank */
+		if (bank) {
+			reg = reg | BIT(4);
+			reg = reg & ~BIT(3);
+		} else {
+			reg = reg & ~BIT(4);
+			reg = reg & ~BIT(2);
+		}
+
+		/* Read command register */
+		outb_p(reg, ASFDATABNKSEL);
+		cmd = inb_p(ASFINDEX);
+		len = inb_p(ASFDATARWPTR);
+		for (i = 0; i < len; i++)
+			data[i] = inb_p(ASFINDEX);
+
+		/* Clear data bank status */
+		if (bank) {
+			reg = reg | BIT(3);
+			outb_p(reg, ASFDATABNKSEL);
+		} else {
+			reg = reg | BIT(2);
+			outb_p(reg, ASFDATABNKSEL);
+		}
+	}
+
+	outb_p(0, ASFSETDATARDPTR);
+	if (cmd & BIT(0))
+		return;
+
+	i2c_slave_event(adapdata->slave, I2C_SLAVE_WRITE_REQUESTED, &val);
+	for (i = 0; i < len; i++) {
+		val = data[i];
+		i2c_slave_event(adapdata->slave, I2C_SLAVE_WRITE_RECEIVED, &val);
+	}
+	i2c_slave_event(adapdata->slave, I2C_SLAVE_STOP, &val);
+}
+
 static void sb800_asf_update_bits(unsigned short piix4_smba, u8 bit, unsigned long offset, bool set)
 {
 	unsigned long reg;
@@ -1195,6 +1268,88 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 	return 0;
 }
 
+static irqreturn_t sb800_asf_irq_handler(int irq, void *ptr)
+{
+	struct i2c_piix4_adapdata *adapdata = ptr;
+	unsigned short piix4_smba = adapdata->smba;
+	u8 slave_int = inb_p(ASFSTA);
+
+	if (slave_int & BIT(6)) {
+		/* Slave Interrupt */
+		outb_p(slave_int | BIT(6), ASFSTA);
+		schedule_delayed_work(&adapdata->work_buf, HZ);
+	} else {
+		/* Master Interrupt */
+		sb800_asf_update_bits(piix4_smba, SB800_ASF_SLV_INTR, SMBHSTSTS, true);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int sb800_asf_add_adap(struct pci_dev *dev)
+{
+	struct i2c_piix4_adapdata *adapdata;
+	struct resource_entry *rentry;
+	struct sb800_asf_data data;
+	struct list_head res_list;
+	struct acpi_device *adev;
+	acpi_status status;
+	acpi_handle handle;
+	int ret;
+
+	status = acpi_get_handle(NULL, SB800_ASF_ACPI_PATH, &handle);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	adev = acpi_fetch_acpi_dev(handle);
+	if (!adev)
+		return -ENODEV;
+
+	INIT_LIST_HEAD(&res_list);
+	ret = acpi_dev_get_resources(adev, &res_list, NULL, NULL);
+	if (ret < 0) {
+		dev_err(&dev->dev, "Error getting ASF ACPI resource: %d\n", ret);
+		return ret;
+	}
+
+	list_for_each_entry(rentry, &res_list, node) {
+		switch (resource_type(rentry->res)) {
+		case IORESOURCE_IO:
+			data.addr = rentry->res->start;
+			break;
+		case IORESOURCE_IRQ:
+			data.irq = rentry->res->start;
+			break;
+		default:
+			dev_warn(&adev->dev, "Invalid ASF resource\n");
+			break;
+		}
+	}
+
+	acpi_dev_free_resource_list(&res_list);
+	ret = piix4_add_adapter(dev, data.addr, SMBUS_ASF, piix4_adapter_count, false, 0,
+				piix4_main_port_names_sb800[piix4_adapter_count],
+				&piix4_main_adapters[piix4_adapter_count]);
+	if (ret) {
+		dev_err(&dev->dev, "Failed to add ASF adapter: %d\n", ret);
+		return -ENODEV;
+	}
+
+	adapdata = i2c_get_adapdata(piix4_main_adapters[piix4_adapter_count]);
+	ret = devm_request_irq(&dev->dev, data.irq, sb800_asf_irq_handler, IRQF_SHARED,
+			       "sb800_smbus_asf", adapdata);
+	if (ret) {
+		dev_err(&dev->dev, "Unable to request irq: %d for use\n", data.irq);
+		return ret;
+	}
+
+	INIT_DELAYED_WORK(&adapdata->work_buf, sb800_asf_process_slave);
+	adapdata->is_asf = true;
+	/* Increment the adapter count by 1 as ASF is added to the list */
+	piix4_adapter_count++;
+	return 1;
+}
+
 static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba,
 				    bool notify_imc)
 {
@@ -1243,6 +1398,7 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	int retval;
 	bool is_sb800 = false;
+	bool is_asf = false;
 
 	if ((dev->vendor == PCI_VENDOR_ID_ATI &&
 	     dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
@@ -1279,6 +1435,10 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		retval = piix4_add_adapters_sb800(dev, retval, notify_imc);
 		if (retval < 0)
 			return retval;
+
+		/* Check if ASF is enabled in SB800 */
+		if (sb800_asf_add_adap(dev))
+			is_asf = true;
 	} else {
 		retval = piix4_setup(dev, id);
 		if (retval < 0)
@@ -1308,7 +1468,9 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	if (dev->vendor == PCI_VENDOR_ID_AMD &&
 	    (dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS ||
 	     dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS)) {
-		retval = piix4_setup_sb800(dev, id, 1);
+		/* Do not setup AUX port if ASF is enabled */
+		if (!is_asf)
+			retval = piix4_setup_sb800(dev, id, 1);
 	}
 
 	if (retval > 0) {
@@ -1326,6 +1488,9 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
 {
 	struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(adap);
 
+	if (adapdata->is_asf)
+		cancel_delayed_work_sync(&adapdata->work_buf);
+
 	if (adapdata->smba) {
 		i2c_del_adapter(adap);
 		if (adapdata->port == (0 << piix4_port_shift_sb800))
-- 
2.25.1

Re: [PATCH v3 3/5] i2c: piix4: Add ACPI support for ASF SMBus device

[Andy Shevchenko]

On Fri, Sep 06, 2024 at 12:41:59PM +0530, Shyam Sundar S K wrote:
> The AMD ASF controller is presented to the operating system as an ACPI
> device. The piix4 driver can obtain the ASF handle through ACPI to
> retrieve information about the ASF controller's attributes, such as the
> ASF address space and interrupt number, and to handle ASF interrupts.

Can you share an excerpt of DSDT to see how it looks like?

> Currently, the piix4 driver assumes that a specific port address is
> designated for AUX operations. However, with the introduction of ASF, the
> same port address may also be used by the ASF controller. Therefore, a
> check needs to be added to ensure that if ASF is advertised and enabled in
> ACPI, the AUX port is not set up.

> Additionally, include a 'depends on X86' Kconfig entry for
> CONFIG_I2C_PIIX4, as the current patch utilizes acpi_dev_get_resources(),
> which is compiled only when CONFIG_ACPI is enabled, and CONFIG_ACPI
> depends on CONFIG_X86.

Yeah, please don't do that. If it requires ACPI, make it clear, there is
no x86 compile-time dependency.

Second issue with this is that now you require entire ACPI machinery for
the previous cases where it wasn't needed. Imagine an embedded system with
limited amount of memory for which you require +1Mbyte just for nothing.

Look how the other do (hint: ifdeffery in the code with stubs).

> +#define SB800_ASF_ACPI_PATH			"\\_SB.ASFC"

...

> +static void sb800_asf_process_slave(struct work_struct *work)
> +{
> +	struct i2c_piix4_adapdata *adapdata =
> +		container_of(work, struct i2c_piix4_adapdata, work_buf.work);
> +	unsigned short piix4_smba = adapdata->smba;
> +	u8 data[SB800_ASF_BLOCK_MAX_BYTES];

> +	u8 bank, reg, cmd = 0;

Move cmd assignment into the respective branch of the conditional below, in
that case it will be closer and more symmetrical.

> +	u8 len, val = 0;

> +	int i;

Why signed?

> +	/* Read slave status register */
> +	reg = inb_p(ASFSLVSTA);
> +
> +	/* Check if no error bits are set in slave status register */
> +	if (reg & SB800_ASF_ERROR_STATUS) {
> +		/* Set bank as full */
> +		reg = reg | GENMASK(3, 2);
> +		outb_p(reg, ASFDATABNKSEL);
> +	} else {
> +		/* Read data bank */
> +		reg = inb_p(ASFDATABNKSEL);

> +		bank = (reg & BIT(3)) >> 3;

Try
		bank = (reg & BIT(3)) ? 1 : 0;

Probably it doesn't affect the code generation, but at least seems cleaner
to read.

> +		/* Set read data bank */
> +		if (bank) {
> +			reg = reg | BIT(4);
> +			reg = reg & ~BIT(3);
> +		} else {
> +			reg = reg & ~BIT(4);
> +			reg = reg & ~BIT(2);
> +		}
> +
> +		/* Read command register */
> +		outb_p(reg, ASFDATABNKSEL);
> +		cmd = inb_p(ASFINDEX);
> +		len = inb_p(ASFDATARWPTR);
> +		for (i = 0; i < len; i++)
> +			data[i] = inb_p(ASFINDEX);
> +
> +		/* Clear data bank status */
> +		if (bank) {
> +			reg = reg | BIT(3);
> +			outb_p(reg, ASFDATABNKSEL);
> +		} else {
> +			reg = reg | BIT(2);
> +			outb_p(reg, ASFDATABNKSEL);
> +		}
> +	}
> +
> +	outb_p(0, ASFSETDATARDPTR);
> +	if (cmd & BIT(0))
> +		return;
> +
> +	i2c_slave_event(adapdata->slave, I2C_SLAVE_WRITE_REQUESTED, &val);
> +	for (i = 0; i < len; i++) {
> +		val = data[i];
> +		i2c_slave_event(adapdata->slave, I2C_SLAVE_WRITE_RECEIVED, &val);
> +	}
> +	i2c_slave_event(adapdata->slave, I2C_SLAVE_STOP, &val);
> +}

...

> +static irqreturn_t sb800_asf_irq_handler(int irq, void *ptr)
> +{
> +	struct i2c_piix4_adapdata *adapdata = ptr;
> +	unsigned short piix4_smba = adapdata->smba;
> +	u8 slave_int = inb_p(ASFSTA);
> +
> +	if (slave_int & BIT(6)) {
> +		/* Slave Interrupt */
> +		outb_p(slave_int | BIT(6), ASFSTA);
> +		schedule_delayed_work(&adapdata->work_buf, HZ);
> +	} else {
> +		/* Master Interrupt */

Please, start using inclusive non-offensive terms instead of old 'master/slave'
terminology. Nowadays it's a part of the standard AFAIU.

Note, I'm talking only about comments and messages, the APIs is another story
that should be addressed separately.

> +		sb800_asf_update_bits(piix4_smba, SB800_ASF_SLV_INTR, SMBHSTSTS, true);
> +	}
> +
> +	return IRQ_HANDLED;
> +}

...

> +static int sb800_asf_add_adap(struct pci_dev *dev)
> +{
> +	struct i2c_piix4_adapdata *adapdata;
> +	struct resource_entry *rentry;
> +	struct sb800_asf_data data;

> +	struct list_head res_list;

Why not LIST_HEAD(); as it was in the previous version?

> +	struct acpi_device *adev;
> +	acpi_status status;
> +	acpi_handle handle;
> +	int ret;

> +	status = acpi_get_handle(NULL, SB800_ASF_ACPI_PATH, &handle);
> +	if (ACPI_FAILURE(status))
> +		return -ENODEV;
> +
> +	adev = acpi_fetch_acpi_dev(handle);
> +	if (!adev)
> +		return -ENODEV;

This approach I don't like. I would like to see DSDT for that
as I mentioned above.

> +	INIT_LIST_HEAD(&res_list);

See above.

> +	ret = acpi_dev_get_resources(adev, &res_list, NULL, NULL);
> +	if (ret < 0) {

> +		dev_err(&dev->dev, "Error getting ASF ACPI resource: %d\n", ret);
> +		return ret;

		return dev_err_probe(...);

> +	}
> +
> +	list_for_each_entry(rentry, &res_list, node) {
> +		switch (resource_type(rentry->res)) {
> +		case IORESOURCE_IO:
> +			data.addr = rentry->res->start;
> +			break;
> +		case IORESOURCE_IRQ:
> +			data.irq = rentry->res->start;
> +			break;
> +		default:
> +			dev_warn(&adev->dev, "Invalid ASF resource\n");
> +			break;
> +		}
> +	}
> +
> +	acpi_dev_free_resource_list(&res_list);
> +	ret = piix4_add_adapter(dev, data.addr, SMBUS_ASF, piix4_adapter_count, false, 0,
> +				piix4_main_port_names_sb800[piix4_adapter_count],
> +				&piix4_main_adapters[piix4_adapter_count]);
> +	if (ret) {
> +		dev_err(&dev->dev, "Failed to add ASF adapter: %d\n", ret);
> +		return -ENODEV;

		return dev_err_probe(...);

> +	}
> +
> +	adapdata = i2c_get_adapdata(piix4_main_adapters[piix4_adapter_count]);
> +	ret = devm_request_irq(&dev->dev, data.irq, sb800_asf_irq_handler, IRQF_SHARED,
> +			       "sb800_smbus_asf", adapdata);
> +	if (ret) {
> +		dev_err(&dev->dev, "Unable to request irq: %d for use\n", data.irq);
> +		return ret;

		return dev_err_probe(...);

> +	}
> +
> +	INIT_DELAYED_WORK(&adapdata->work_buf, sb800_asf_process_slave);
> +	adapdata->is_asf = true;
> +	/* Increment the adapter count by 1 as ASF is added to the list */
> +	piix4_adapter_count++;
> +	return 1;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/5] i2c: piix4: Add ACPI support for ASF SMBus device

[Shyam Sundar S K]

On 9/6/2024 17:54, Andy Shevchenko wrote:
> On Fri, Sep 06, 2024 at 12:41:59PM +0530, Shyam Sundar S K wrote:
>> The AMD ASF controller is presented to the operating system as an ACPI
>> device. The piix4 driver can obtain the ASF handle through ACPI to
>> retrieve information about the ASF controller's attributes, such as the
>> ASF address space and interrupt number, and to handle ASF interrupts.
> 
> Can you share an excerpt of DSDT to see how it looks like?

Device (ASFC)
{
	...
	
    Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
    {
        Name (ASBB, ResourceTemplate ()
        {
            Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, )
            {
                0x00000014,
            }
            IO (Decode16,
                0x0B20,             // Range Minimum
                0x0B20,             // Range Maximum
                0x00,               // Alignment
                0x20,               // Length
                )
            Memory32Fixed (ReadWrite,
                0xFEC00040,         // Address Base
                0x00000100,         // Address Length
                )
        })
        Return (ASBB) /* \_SB_.ASFC._CRS.ASBB */
    }

	...
}

> 
>> Currently, the piix4 driver assumes that a specific port address is
>> designated for AUX operations. However, with the introduction of ASF, the
>> same port address may also be used by the ASF controller. Therefore, a
>> check needs to be added to ensure that if ASF is advertised and enabled in
>> ACPI, the AUX port is not set up.
> 
>> Additionally, include a 'depends on X86' Kconfig entry for
>> CONFIG_I2C_PIIX4, as the current patch utilizes acpi_dev_get_resources(),
>> which is compiled only when CONFIG_ACPI is enabled, and CONFIG_ACPI
>> depends on CONFIG_X86.
> 
> Yeah, please don't do that. If it requires ACPI, make it clear, there is
> no x86 compile-time dependency.

You mean to say make the dependencies as:

depends on PCI && HAS_IOPORT && ACPI

instead of:

depends on PCI && HAS_IOPORT && X86

> 
> Second issue with this is that now you require entire ACPI machinery for
> the previous cases where it wasn't needed. Imagine an embedded system with
> limited amount of memory for which you require +1Mbyte just for nothing.
> 
> Look how the other do (hint: ifdeffery in the code with stubs).
> 
>> +#define SB800_ASF_ACPI_PATH			"\\_SB.ASFC"
> 
> ...
> 
>> +static void sb800_asf_process_slave(struct work_struct *work)
>> +{
>> +	struct i2c_piix4_adapdata *adapdata =
>> +		container_of(work, struct i2c_piix4_adapdata, work_buf.work);
>> +	unsigned short piix4_smba = adapdata->smba;
>> +	u8 data[SB800_ASF_BLOCK_MAX_BYTES];
> 
>> +	u8 bank, reg, cmd = 0;
> 
> Move cmd assignment into the respective branch of the conditional below, in
> that case it will be closer and more symmetrical.

meaning, make the cmd assignment only in the if() case. Not sure if I
understand your remark completely.

> 
>> +	u8 len, val = 0;
> 
>> +	int i;
> 
> Why signed?
> 
>> +	/* Read slave status register */
>> +	reg = inb_p(ASFSLVSTA);
>> +
>> +	/* Check if no error bits are set in slave status register */
>> +	if (reg & SB800_ASF_ERROR_STATUS) {
>> +		/* Set bank as full */
>> +		reg = reg | GENMASK(3, 2);
>> +		outb_p(reg, ASFDATABNKSEL);
>> +	} else {
>> +		/* Read data bank */
>> +		reg = inb_p(ASFDATABNKSEL);
> 
>> +		bank = (reg & BIT(3)) >> 3;
> 
> Try
> 		bank = (reg & BIT(3)) ? 1 : 0;
> 
> Probably it doesn't affect the code generation, but at least seems cleaner
> to read.
> 
>> +		/* Set read data bank */
>> +		if (bank) {
>> +			reg = reg | BIT(4);
>> +			reg = reg & ~BIT(3);
>> +		} else {
>> +			reg = reg & ~BIT(4);
>> +			reg = reg & ~BIT(2);
>> +		}
>> +
>> +		/* Read command register */
>> +		outb_p(reg, ASFDATABNKSEL);
>> +		cmd = inb_p(ASFINDEX);
>> +		len = inb_p(ASFDATARWPTR);
>> +		for (i = 0; i < len; i++)
>> +			data[i] = inb_p(ASFINDEX);
>> +
>> +		/* Clear data bank status */
>> +		if (bank) {
>> +			reg = reg | BIT(3);
>> +			outb_p(reg, ASFDATABNKSEL);
>> +		} else {
>> +			reg = reg | BIT(2);
>> +			outb_p(reg, ASFDATABNKSEL);
>> +		}
>> +	}
>> +
>> +	outb_p(0, ASFSETDATARDPTR);
>> +	if (cmd & BIT(0))
>> +		return;
>> +
>> +	i2c_slave_event(adapdata->slave, I2C_SLAVE_WRITE_REQUESTED, &val);
>> +	for (i = 0; i < len; i++) {
>> +		val = data[i];
>> +		i2c_slave_event(adapdata->slave, I2C_SLAVE_WRITE_RECEIVED, &val);
>> +	}
>> +	i2c_slave_event(adapdata->slave, I2C_SLAVE_STOP, &val);
>> +}
> 
> ...
> 
>> +static irqreturn_t sb800_asf_irq_handler(int irq, void *ptr)
>> +{
>> +	struct i2c_piix4_adapdata *adapdata = ptr;
>> +	unsigned short piix4_smba = adapdata->smba;
>> +	u8 slave_int = inb_p(ASFSTA);
>> +
>> +	if (slave_int & BIT(6)) {
>> +		/* Slave Interrupt */
>> +		outb_p(slave_int | BIT(6), ASFSTA);
>> +		schedule_delayed_work(&adapdata->work_buf, HZ);
>> +	} else {
>> +		/* Master Interrupt */
> 
> Please, start using inclusive non-offensive terms instead of old 'master/slave'
> terminology. Nowadays it's a part of the standard AFAIU.
> 

OK. I get it ( tried to retain the names as mentioned in the AMD ASF
databook).

Which one would you advise (instead of master/slave)?

Primary/secondary
Controller/Worker
Requester/Responder

> Note, I'm talking only about comments and messages, the APIs is another story
> that should be addressed separately.
> 
>> +		sb800_asf_update_bits(piix4_smba, SB800_ASF_SLV_INTR, SMBHSTSTS, true);
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
> 
> ...
> 
>> +static int sb800_asf_add_adap(struct pci_dev *dev)
>> +{
>> +	struct i2c_piix4_adapdata *adapdata;
>> +	struct resource_entry *rentry;
>> +	struct sb800_asf_data data;
> 
>> +	struct list_head res_list;
> 
> Why not LIST_HEAD(); as it was in the previous version?
> 
>> +	struct acpi_device *adev;
>> +	acpi_status status;
>> +	acpi_handle handle;
>> +	int ret;
> 
>> +	status = acpi_get_handle(NULL, SB800_ASF_ACPI_PATH, &handle);
>> +	if (ACPI_FAILURE(status))
>> +		return -ENODEV;
>> +
>> +	adev = acpi_fetch_acpi_dev(handle);
>> +	if (!adev)
>> +		return -ENODEV;
> 
> This approach I don't like. I would like to see DSDT for that
> as I mentioned above.

I have posted the DSDT. Can you please elaborate your remarks?

> 
>> +	INIT_LIST_HEAD(&res_list);
> 
> See above.
> 
>> +	ret = acpi_dev_get_resources(adev, &res_list, NULL, NULL);
>> +	if (ret < 0) {
> 
>> +		dev_err(&dev->dev, "Error getting ASF ACPI resource: %d\n", ret);
>> +		return ret;
> 
> 		return dev_err_probe(...);

I thought dev_err_probe(...); is called only from the .probe
functions. Is that not the case?

In the current proposed change, sb800_asf_add_adap() gets called from
*_probe().

Or you mean to say, no need for a error print and just do a error return?

if (ret < 0)
	return ret;

Likewise for below remarks on dev_err_probe(...);

Thanks,
Shyam

> 
>> +	}
>> +
>> +	list_for_each_entry(rentry, &res_list, node) {
>> +		switch (resource_type(rentry->res)) {
>> +		case IORESOURCE_IO:
>> +			data.addr = rentry->res->start;
>> +			break;
>> +		case IORESOURCE_IRQ:
>> +			data.irq = rentry->res->start;
>> +			break;
>> +		default:
>> +			dev_warn(&adev->dev, "Invalid ASF resource\n");
>> +			break;
>> +		}
>> +	}
>> +
>> +	acpi_dev_free_resource_list(&res_list);
>> +	ret = piix4_add_adapter(dev, data.addr, SMBUS_ASF, piix4_adapter_count, false, 0,
>> +				piix4_main_port_names_sb800[piix4_adapter_count],
>> +				&piix4_main_adapters[piix4_adapter_count]);
>> +	if (ret) {
>> +		dev_err(&dev->dev, "Failed to add ASF adapter: %d\n", ret);
>> +		return -ENODEV;
> 
> 		return dev_err_probe(...);
> 
>> +	}
>> +
>> +	adapdata = i2c_get_adapdata(piix4_main_adapters[piix4_adapter_count]);
>> +	ret = devm_request_irq(&dev->dev, data.irq, sb800_asf_irq_handler, IRQF_SHARED,
>> +			       "sb800_smbus_asf", adapdata);
>> +	if (ret) {
>> +		dev_err(&dev->dev, "Unable to request irq: %d for use\n", data.irq);
>> +		return ret;
> 
> 		return dev_err_probe(...);
> 
>> +	}
>> +
>> +	INIT_DELAYED_WORK(&adapdata->work_buf, sb800_asf_process_slave);
>> +	adapdata->is_asf = true;
>> +	/* Increment the adapter count by 1 as ASF is added to the list */
>> +	piix4_adapter_count++;
>> +	return 1;
>> +}
> 

Re: [PATCH v3 3/5] i2c: piix4: Add ACPI support for ASF SMBus device

[Andy Shevchenko]

On Fri, Sep 06, 2024 at 06:50:48PM +0530, Shyam Sundar S K wrote:
> On 9/6/2024 17:54, Andy Shevchenko wrote:
> > On Fri, Sep 06, 2024 at 12:41:59PM +0530, Shyam Sundar S K wrote:

First of all, you haven't replied to some of my comments, I assume that you
agree on them and are going to fix as suggested?

...

> >> The AMD ASF controller is presented to the operating system as an ACPI
> >> device. The piix4 driver can obtain the ASF handle through ACPI to
> >> retrieve information about the ASF controller's attributes, such as the
> >> ASF address space and interrupt number, and to handle ASF interrupts.
> > 
> > Can you share an excerpt of DSDT to see how it looks like?
> 
> Device (ASFC)
> {
> 	...

Can you put the necessary bits for the enumeration (you may replace some IDs if
they are not public yet to something like XX..XX or xx..xx)?

>     Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
>     {
>         Name (ASBB, ResourceTemplate ()
>         {
>             Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, )
>             {
>                 0x00000014,
>             }
>             IO (Decode16,
>                 0x0B20,             // Range Minimum
>                 0x0B20,             // Range Maximum

Typo in value? Shouldn't this be 0x0b3f?

>                 0x00,               // Alignment
>                 0x20,               // Length
>                 )
>             Memory32Fixed (ReadWrite,
>                 0xFEC00040,         // Address Base
>                 0x00000100,         // Address Length
>                 )
>         })
>         Return (ASBB) /* \_SB_.ASFC._CRS.ASBB */
>     }
> 	...
> }

...

> >> Additionally, include a 'depends on X86' Kconfig entry for
> >> CONFIG_I2C_PIIX4, as the current patch utilizes acpi_dev_get_resources(),
> >> which is compiled only when CONFIG_ACPI is enabled, and CONFIG_ACPI
> >> depends on CONFIG_X86.
> > 
> > Yeah, please don't do that. If it requires ACPI, make it clear, there is
> > no x86 compile-time dependency.
> 
> You mean to say make the dependencies as:
> 
> depends on PCI && HAS_IOPORT && ACPI
> 
> instead of:
> 
> depends on PCI && HAS_IOPORT && X86

Yes, but see below as well about the stubs

~~~vvv
> > Second issue with this is that now you require entire ACPI machinery for
> > the previous cases where it wasn't needed. Imagine an embedded system with
> > limited amount of memory for which you require +1Mbyte just for nothing.
> > 
> > Look how the other do (hint: ifdeffery in the code with stubs).

___^^^

...

> >> +	u8 bank, reg, cmd = 0;
> > 
> > Move cmd assignment into the respective branch of the conditional below, in
> > that case it will be closer and more symmetrical.
> 
> meaning, make the cmd assignment only in the if() case.

Yes.

> Not sure if I understand your remark completely.

	if (...) {
		cmd = 0;
	} else {
		cmd = ...
	}

...

> >> +	if (slave_int & BIT(6)) {
> >> +		/* Slave Interrupt */
> >> +		outb_p(slave_int | BIT(6), ASFSTA);
> >> +		schedule_delayed_work(&adapdata->work_buf, HZ);
> >> +	} else {
> >> +		/* Master Interrupt */
> > 
> > Please, start using inclusive non-offensive terms instead of old 'master/slave'
> > terminology. Nowadays it's a part of the standard AFAIU.
> 
> OK. I get it ( tried to retain the names as mentioned in the AMD ASF
> databook).
> 
> Which one would you advise (instead of master/slave)?

As per official I2C specification. :-)

> Primary/secondary
> Controller/Worker
> Requester/Responder

See, e.g., a93c2e5fe766 ("i2c: reword i2c_algorithm according to newest specification").

> > Note, I'm talking only about comments and messages, the APIs is another story
> > that should be addressed separately.
> > 
> >> +		sb800_asf_update_bits(piix4_smba, SB800_ASF_SLV_INTR, SMBHSTSTS, true);
> >> +	}

...

> >> +	status = acpi_get_handle(NULL, SB800_ASF_ACPI_PATH, &handle);
> >> +	if (ACPI_FAILURE(status))
> >> +		return -ENODEV;
> >> +
> >> +	adev = acpi_fetch_acpi_dev(handle);
> >> +	if (!adev)
> >> +		return -ENODEV;
> > 
> > This approach I don't like. I would like to see DSDT for that
> > as I mentioned above.
> 
> I have posted the DSDT. Can you please elaborate your remarks?

Not that parts that affect this...

...

> >> +	ret = acpi_dev_get_resources(adev, &res_list, NULL, NULL);
> >> +	if (ret < 0) {
> > 
> >> +		dev_err(&dev->dev, "Error getting ASF ACPI resource: %d\n", ret);
> >> +		return ret;
> > 
> > 		return dev_err_probe(...);
> 
> I thought dev_err_probe(...); is called only from the .probe
> functions. Is that not the case?

I assume you call this due to use of devm_*(). Either devm_*() should be
replaced to non-devm_*() analogues, or these moved to dev_err_probe().

> In the current proposed change, sb800_asf_add_adap() gets called from
> *_probe().
> 
> Or you mean to say, no need for a error print and just do a error return?

No. It's also possible, but this is up to you.

> if (ret < 0)
> 	return ret;
> 
> Likewise for below remarks on dev_err_probe(...);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/5] i2c: piix4: Add ACPI support for ASF SMBus device

[Shyam Sundar S K]

On 9/6/2024 20:10, Andy Shevchenko wrote:
> On Fri, Sep 06, 2024 at 06:50:48PM +0530, Shyam Sundar S K wrote:
>> On 9/6/2024 17:54, Andy Shevchenko wrote:
>>> On Fri, Sep 06, 2024 at 12:41:59PM +0530, Shyam Sundar S K wrote:
> 
> First of all, you haven't replied to some of my comments, I assume that you
> agree on them and are going to fix as suggested?

I agree with your comments. I have only requested further
clarification on a few points where I need more understanding.

> 
> ...
> 
>>>> The AMD ASF controller is presented to the operating system as an ACPI
>>>> device. The piix4 driver can obtain the ASF handle through ACPI to
>>>> retrieve information about the ASF controller's attributes, such as the
>>>> ASF address space and interrupt number, and to handle ASF interrupts.
>>>
>>> Can you share an excerpt of DSDT to see how it looks like?
>>
>> Device (ASFC)
>> {
>> 	...
> 
> Can you put the necessary bits for the enumeration (you may replace some IDs if
> they are not public yet to something like XX..XX or xx..xx)?
> 

Name (_HID, "AMDIXXXX")  // _HID: Hardware ID
Name (_UID, Zero)  // _UID: Unique ID

>>     Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
>>     {
>>         Name (ASBB, ResourceTemplate ()
>>         {
>>             Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, )
>>             {
>>                 0x00000014,
>>             }
>>             IO (Decode16,
>>                 0x0B20,             // Range Minimum
>>                 0x0B20,             // Range Maximum
> 
> Typo in value? Shouldn't this be 0x0b3f?

Its is 0xb20, that is meant for ASF.

> 
>>                 0x00,               // Alignment
>>                 0x20,               // Length
>>                 )
>>             Memory32Fixed (ReadWrite,
>>                 0xFEC00040,         // Address Base
>>                 0x00000100,         // Address Length
>>                 )
>>         })
>>         Return (ASBB) /* \_SB_.ASFC._CRS.ASBB */
>>     }
>> 	...
>> }
> 
> ...
> 
>>>> Additionally, include a 'depends on X86' Kconfig entry for
>>>> CONFIG_I2C_PIIX4, as the current patch utilizes acpi_dev_get_resources(),
>>>> which is compiled only when CONFIG_ACPI is enabled, and CONFIG_ACPI
>>>> depends on CONFIG_X86.
>>>
>>> Yeah, please don't do that. If it requires ACPI, make it clear, there is
>>> no x86 compile-time dependency.
>>
>> You mean to say make the dependencies as:
>>
>> depends on PCI && HAS_IOPORT && ACPI
>>
>> instead of:
>>
>> depends on PCI && HAS_IOPORT && X86
> 
> Yes, but see below as well about the stubs
> 
> ~~~vvv
>>> Second issue with this is that now you require entire ACPI machinery for
>>> the previous cases where it wasn't needed. Imagine an embedded system with
>>> limited amount of memory for which you require +1Mbyte just for nothing.
>>>
>>> Look how the other do (hint: ifdeffery in the code with stubs).
> 
> ___^^^
> 
> ...
> 
>>>> +	u8 bank, reg, cmd = 0;
>>>
>>> Move cmd assignment into the respective branch of the conditional below, in
>>> that case it will be closer and more symmetrical.
>>
>> meaning, make the cmd assignment only in the if() case.
> 
> Yes.
> 
>> Not sure if I understand your remark completely.
> 
> 	if (...) {
> 		cmd = 0;
> 	} else {
> 		cmd = ...
> 	}
> 

Got it.

> ...
> 
>>>> +	if (slave_int & BIT(6)) {
>>>> +		/* Slave Interrupt */
>>>> +		outb_p(slave_int | BIT(6), ASFSTA);
>>>> +		schedule_delayed_work(&adapdata->work_buf, HZ);
>>>> +	} else {
>>>> +		/* Master Interrupt */
>>>
>>> Please, start using inclusive non-offensive terms instead of old 'master/slave'
>>> terminology. Nowadays it's a part of the standard AFAIU.
>>
>> OK. I get it ( tried to retain the names as mentioned in the AMD ASF
>> databook).
>>
>> Which one would you advise (instead of master/slave)?
> 
> As per official I2C specification. :-)

Thanks! I will change to controller/target instead of master/slave.

> 
>> Primary/secondary
>> Controller/Worker
>> Requester/Responder
> 
> See, e.g., a93c2e5fe766 ("i2c: reword i2c_algorithm according to newest specification").
> 
>>> Note, I'm talking only about comments and messages, the APIs is another story
>>> that should be addressed separately.
>>>
>>>> +		sb800_asf_update_bits(piix4_smba, SB800_ASF_SLV_INTR, SMBHSTSTS, true);
>>>> +	}
> 
> ...
> 
>>>> +	status = acpi_get_handle(NULL, SB800_ASF_ACPI_PATH, &handle);
>>>> +	if (ACPI_FAILURE(status))
>>>> +		return -ENODEV;
>>>> +
>>>> +	adev = acpi_fetch_acpi_dev(handle);
>>>> +	if (!adev)
>>>> +		return -ENODEV;
>>>
>>> This approach I don't like. I would like to see DSDT for that
>>> as I mentioned above.
>>
>> I have posted the DSDT. Can you please elaborate your remarks?
> 
> Not that parts that affect this...

Alright, I have posted the _HID enumeration details above. Please let
me know if using acpi_fetch_acpi_dev() is acceptable or if there's a
better alternative.

I am open to making changes based on these clarifications.

Thanks,
Shyam

> 
> ...
> 
>>>> +	ret = acpi_dev_get_resources(adev, &res_list, NULL, NULL);
>>>> +	if (ret < 0) {
>>>
>>>> +		dev_err(&dev->dev, "Error getting ASF ACPI resource: %d\n", ret);
>>>> +		return ret;
>>>
>>> 		return dev_err_probe(...);
>>
>> I thought dev_err_probe(...); is called only from the .probe
>> functions. Is that not the case?
> 
> I assume you call this due to use of devm_*(). Either devm_*() should be
> replaced to non-devm_*() analogues, or these moved to dev_err_probe().
> 
>> In the current proposed change, sb800_asf_add_adap() gets called from
>> *_probe().
>>
>> Or you mean to say, no need for a error print and just do a error return?
> 
> No. It's also possible, but this is up to you.
> 
>> if (ret < 0)
>> 	return ret;
>>
>> Likewise for below remarks on dev_err_probe(...);
> 

Re: [PATCH v3 3/5] i2c: piix4: Add ACPI support for ASF SMBus device

[Andy Shevchenko]

On Fri, Sep 06, 2024 at 08:41:19PM +0530, Shyam Sundar S K wrote:
> On 9/6/2024 20:10, Andy Shevchenko wrote:
> > On Fri, Sep 06, 2024 at 06:50:48PM +0530, Shyam Sundar S K wrote:
> >> On 9/6/2024 17:54, Andy Shevchenko wrote:
> >>> On Fri, Sep 06, 2024 at 12:41:59PM +0530, Shyam Sundar S K wrote:

...

> >>>> The AMD ASF controller is presented to the operating system as an ACPI
> >>>> device. The piix4 driver can obtain the ASF handle through ACPI to
> >>>> retrieve information about the ASF controller's attributes, such as the
> >>>> ASF address space and interrupt number, and to handle ASF interrupts.
> >>>
> >>> Can you share an excerpt of DSDT to see how it looks like?
> >>
> >> Device (ASFC)
> >> {
> >> 	...
> > 
> > Can you put the necessary bits for the enumeration (you may replace some IDs if
> > they are not public yet to something like XX..XX or xx..xx)?
> 
> Name (_HID, "AMDIXXXX")  // _HID: Hardware ID
> Name (_UID, Zero)  // _UID: Unique ID

Thank you!

Now a question, why your case can't have a separate (platform) device driver?

> >>     Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
> >>     {
> >>         Name (ASBB, ResourceTemplate ()
> >>         {
> >>             Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, )
> >>             {
> >>                 0x00000014,
> >>             }
> >>             IO (Decode16,
> >>                 0x0B20,             // Range Minimum
> >>                 0x0B20,             // Range Maximum
> > 
> > Typo in value? Shouldn't this be 0x0b3f?
> 
> Its is 0xb20, that is meant for ASF.

Yes, I mixed up IO() vs. Memory*() resource. The IO() has two values for
the start address and you fixed that to the above mentioned value.

TL;DR: this looks okay.

> >>                 0x00,               // Alignment
> >>                 0x20,               // Length
> >>                 )
> >>             Memory32Fixed (ReadWrite,
> >>                 0xFEC00040,         // Address Base
> >>                 0x00000100,         // Address Length
> >>                 )
> >>         })
> >>         Return (ASBB) /* \_SB_.ASFC._CRS.ASBB */
> >>     }
> >> 	...
> >> }

...

> >>>> +	status = acpi_get_handle(NULL, SB800_ASF_ACPI_PATH, &handle);
> >>>> +	if (ACPI_FAILURE(status))
> >>>> +		return -ENODEV;
> >>>> +
> >>>> +	adev = acpi_fetch_acpi_dev(handle);
> >>>> +	if (!adev)
> >>>> +		return -ENODEV;
> >>>
> >>> This approach I don't like. I would like to see DSDT for that
> >>> as I mentioned above.
> >>
> >> I have posted the DSDT. Can you please elaborate your remarks?
> > 
> > Not that parts that affect this...
> 
> Alright, I have posted the _HID enumeration details above. Please let
> me know if using acpi_fetch_acpi_dev() is acceptable or if there's a
> better alternative.

> I am open to making changes based on these clarifications.

Since you have a proper Device object in ACPI, it seems to me that you should
do other way around, i.e. having a platform device driver for this ACPI device
(based on _HID) and use piix4 as a library for it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/5] i2c: piix4: Add ACPI support for ASF SMBus device

[Shyam Sundar S K]

On 9/6/2024 21:34, Andy Shevchenko wrote:
> On Fri, Sep 06, 2024 at 08:41:19PM +0530, Shyam Sundar S K wrote:
>> On 9/6/2024 20:10, Andy Shevchenko wrote:
>>> On Fri, Sep 06, 2024 at 06:50:48PM +0530, Shyam Sundar S K wrote:
>>>> On 9/6/2024 17:54, Andy Shevchenko wrote:
>>>>> On Fri, Sep 06, 2024 at 12:41:59PM +0530, Shyam Sundar S K wrote:
> 
> ...
> 
>>>>>> The AMD ASF controller is presented to the operating system as an ACPI
>>>>>> device. The piix4 driver can obtain the ASF handle through ACPI to
>>>>>> retrieve information about the ASF controller's attributes, such as the
>>>>>> ASF address space and interrupt number, and to handle ASF interrupts.
>>>>>
>>>>> Can you share an excerpt of DSDT to see how it looks like?
>>>>
>>>> Device (ASFC)
>>>> {
>>>> 	...
>>>
>>> Can you put the necessary bits for the enumeration (you may replace some IDs if
>>> they are not public yet to something like XX..XX or xx..xx)?
>>
>> Name (_HID, "AMDIXXXX")  // _HID: Hardware ID
>> Name (_UID, Zero)  // _UID: Unique ID
> 
> Thank you!
> 
> Now a question, why your case can't have a separate (platform) device driver?

I evaluated this approach before proposing the change, considering the
option of creating a separate platform driver, which is relatively
easier to implement.

However, there are a couple of important points to note:

- ASF is a subset of SMBus. If a system has 3 SMBus ports, this change
would allow one of the ports to handle ASF operations.

- In the current i2c_piix4 driver, the assumption is that the port
address 0xb20 is designated for auxiliary operations, but this same
port can also be used for ASF. This could lead to a scenario of port
collision. I tried to highlight this in the commit message, and you
can see some dance in piix4_probe().

- As a result, users might encounter an error on platforms that
support ASF: "SMBus region 0x%x already in use!"

This is why I believe it would be more meaningful to integrate the ASF
changes into the SMBus driver.

Thoughts..?

Thanks,
Shyam

> 
>>>>     Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
>>>>     {
>>>>         Name (ASBB, ResourceTemplate ()
>>>>         {
>>>>             Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, )
>>>>             {
>>>>                 0x00000014,
>>>>             }
>>>>             IO (Decode16,
>>>>                 0x0B20,             // Range Minimum
>>>>                 0x0B20,             // Range Maximum
>>>
>>> Typo in value? Shouldn't this be 0x0b3f?
>>
>> Its is 0xb20, that is meant for ASF.
> 
> Yes, I mixed up IO() vs. Memory*() resource. The IO() has two values for
> the start address and you fixed that to the above mentioned value.
> 
> TL;DR: this looks okay.
> 
>>>>                 0x00,               // Alignment
>>>>                 0x20,               // Length
>>>>                 )
>>>>             Memory32Fixed (ReadWrite,
>>>>                 0xFEC00040,         // Address Base
>>>>                 0x00000100,         // Address Length
>>>>                 )
>>>>         })
>>>>         Return (ASBB) /* \_SB_.ASFC._CRS.ASBB */
>>>>     }
>>>> 	...
>>>> }
> 
> ...
> 
>>>>>> +	status = acpi_get_handle(NULL, SB800_ASF_ACPI_PATH, &handle);
>>>>>> +	if (ACPI_FAILURE(status))
>>>>>> +		return -ENODEV;
>>>>>> +
>>>>>> +	adev = acpi_fetch_acpi_dev(handle);
>>>>>> +	if (!adev)
>>>>>> +		return -ENODEV;
>>>>>
>>>>> This approach I don't like. I would like to see DSDT for that
>>>>> as I mentioned above.
>>>>
>>>> I have posted the DSDT. Can you please elaborate your remarks?
>>>
>>> Not that parts that affect this...
>>
>> Alright, I have posted the _HID enumeration details above. Please let
>> me know if using acpi_fetch_acpi_dev() is acceptable or if there's a
>> better alternative.
> 
>> I am open to making changes based on these clarifications.
> 
> Since you have a proper Device object in ACPI, it seems to me that you should
> do other way around, i.e. having a platform device driver for this ACPI device
> (based on _HID) and use piix4 as a library for it.
> 

Re: [PATCH v3 3/5] i2c: piix4: Add ACPI support for ASF SMBus device

[Shyam Sundar S K]

On 9/7/2024 00:21, Shyam Sundar S K wrote:
> 
> 
> On 9/6/2024 21:34, Andy Shevchenko wrote:
>> On Fri, Sep 06, 2024 at 08:41:19PM +0530, Shyam Sundar S K wrote:
>>> On 9/6/2024 20:10, Andy Shevchenko wrote:
>>>> On Fri, Sep 06, 2024 at 06:50:48PM +0530, Shyam Sundar S K wrote:
>>>>> On 9/6/2024 17:54, Andy Shevchenko wrote:
>>>>>> On Fri, Sep 06, 2024 at 12:41:59PM +0530, Shyam Sundar S K wrote:
>>
>> ...
>>
>>>>>>> The AMD ASF controller is presented to the operating system as an ACPI
>>>>>>> device. The piix4 driver can obtain the ASF handle through ACPI to
>>>>>>> retrieve information about the ASF controller's attributes, such as the
>>>>>>> ASF address space and interrupt number, and to handle ASF interrupts.
>>>>>>
>>>>>> Can you share an excerpt of DSDT to see how it looks like?
>>>>>
>>>>> Device (ASFC)
>>>>> {
>>>>> 	...
>>>>
>>>> Can you put the necessary bits for the enumeration (you may replace some IDs if
>>>> they are not public yet to something like XX..XX or xx..xx)?
>>>
>>> Name (_HID, "AMDIXXXX")  // _HID: Hardware ID
>>> Name (_UID, Zero)  // _UID: Unique ID
>>
>> Thank you!
>>
>> Now a question, why your case can't have a separate (platform) device driver?
> 
> I evaluated this approach before proposing the change, considering the
> option of creating a separate platform driver, which is relatively
> easier to implement.
> 
> However, there are a couple of important points to note:
> 
> - ASF is a subset of SMBus. If a system has 3 SMBus ports, this change
> would allow one of the ports to handle ASF operations.
> 
> - In the current i2c_piix4 driver, the assumption is that the port
> address 0xb20 is designated for auxiliary operations, but this same
> port can also be used for ASF. This could lead to a scenario of port
> collision. I tried to highlight this in the commit message, and you
> can see some dance in piix4_probe().
> 
> - As a result, users might encounter an error on platforms that
> support ASF: "SMBus region 0x%x already in use!"
> 
> This is why I believe it would be more meaningful to integrate the ASF
> changes into the SMBus driver.

Andy, I posted a new version. Can you please take a look. This has a
separate _HID driver for ASF now with piix4 as library.

Thanks,
Shyam

> 
> Thoughts..?
> 
> Thanks,
> Shyam
> 
>>
>>>>>     Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
>>>>>     {
>>>>>         Name (ASBB, ResourceTemplate ()
>>>>>         {
>>>>>             Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, )
>>>>>             {
>>>>>                 0x00000014,
>>>>>             }
>>>>>             IO (Decode16,
>>>>>                 0x0B20,             // Range Minimum
>>>>>                 0x0B20,             // Range Maximum
>>>>
>>>> Typo in value? Shouldn't this be 0x0b3f?
>>>
>>> Its is 0xb20, that is meant for ASF.
>>
>> Yes, I mixed up IO() vs. Memory*() resource. The IO() has two values for
>> the start address and you fixed that to the above mentioned value.
>>
>> TL;DR: this looks okay.
>>
>>>>>                 0x00,               // Alignment
>>>>>                 0x20,               // Length
>>>>>                 )
>>>>>             Memory32Fixed (ReadWrite,
>>>>>                 0xFEC00040,         // Address Base
>>>>>                 0x00000100,         // Address Length
>>>>>                 )
>>>>>         })
>>>>>         Return (ASBB) /* \_SB_.ASFC._CRS.ASBB */
>>>>>     }
>>>>> 	...
>>>>> }
>>
>> ...
>>
>>>>>>> +	status = acpi_get_handle(NULL, SB800_ASF_ACPI_PATH, &handle);
>>>>>>> +	if (ACPI_FAILURE(status))
>>>>>>> +		return -ENODEV;
>>>>>>> +
>>>>>>> +	adev = acpi_fetch_acpi_dev(handle);
>>>>>>> +	if (!adev)
>>>>>>> +		return -ENODEV;
>>>>>>
>>>>>> This approach I don't like. I would like to see DSDT for that
>>>>>> as I mentioned above.
>>>>>
>>>>> I have posted the DSDT. Can you please elaborate your remarks?
>>>>
>>>> Not that parts that affect this...
>>>
>>> Alright, I have posted the _HID enumeration details above. Please let
>>> me know if using acpi_fetch_acpi_dev() is acceptable or if there's a
>>> better alternative.
>>
>>> I am open to making changes based on these clarifications.
>>
>> Since you have a proper Device object in ACPI, it seems to me that you should
>> do other way around, i.e. having a platform device driver for this ACPI device
>> (based on _HID) and use piix4 as a library for it.
>>

Re: [PATCH v3 3/5] i2c: piix4: Add ACPI support for ASF SMBus device

[kernel test robot]

Hi Shyam,

kernel test robot noticed the following build errors:

[auto build test ERROR on andi-shyti/i2c/i2c-host]
[also build test ERROR on linus/master v6.11-rc6 next-20240906]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Shyam-Sundar-S-K/i2c-piix4-Allow-more-than-two-algo-selection-for-SMBus/20240906-152430
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20240906071201.2254354-4-Shyam-sundar.S-k%40amd.com
patch subject: [PATCH v3 3/5] i2c: piix4: Add ACPI support for ASF SMBus device
config: x86_64-buildonly-randconfig-006-20240907 (https://download.01.org/0day-ci/archive/20240907/202409071045.2aUSULRN-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240907/202409071045.2aUSULRN-lkp@intel.com/reproduce)

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

All errors (new ones prefixed by >>):

   drivers/i2c/busses/i2c-piix4.c: In function 'sb800_asf_add_adap':
   drivers/i2c/busses/i2c-piix4.c:1304:16: error: implicit declaration of function 'acpi_fetch_acpi_dev'; did you mean 'acpi_match_acpi_device'? [-Werror=implicit-function-declaration]
    1304 |         adev = acpi_fetch_acpi_dev(handle);
         |                ^~~~~~~~~~~~~~~~~~~
         |                acpi_match_acpi_device
   drivers/i2c/busses/i2c-piix4.c:1304:14: warning: assignment to 'struct acpi_device *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
    1304 |         adev = acpi_fetch_acpi_dev(handle);
         |              ^
   drivers/i2c/busses/i2c-piix4.c:1309:15: error: implicit declaration of function 'acpi_dev_get_resources'; did you mean 'acpi_get_event_resources'? [-Werror=implicit-function-declaration]
    1309 |         ret = acpi_dev_get_resources(adev, &res_list, NULL, NULL);
         |               ^~~~~~~~~~~~~~~~~~~~~~
         |               acpi_get_event_resources
   In file included from include/linux/device.h:15,
                    from include/linux/pci.h:37,
                    from drivers/i2c/busses/i2c-piix4.c:26:
>> drivers/i2c/busses/i2c-piix4.c:1324:39: error: invalid use of undefined type 'struct acpi_device'
    1324 |                         dev_warn(&adev->dev, "Invalid ASF resource\n");
         |                                       ^~
   include/linux/dev_printk.h:110:25: note: in definition of macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                         ^~~
   drivers/i2c/busses/i2c-piix4.c:1324:25: note: in expansion of macro 'dev_warn'
    1324 |                         dev_warn(&adev->dev, "Invalid ASF resource\n");
         |                         ^~~~~~~~
   drivers/i2c/busses/i2c-piix4.c:1329:9: error: implicit declaration of function 'acpi_dev_free_resource_list'; did you mean 'pci_free_resource_list'? [-Werror=implicit-function-declaration]
    1329 |         acpi_dev_free_resource_list(&res_list);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~
         |         pci_free_resource_list
   cc1: some warnings being treated as errors


vim +1324 drivers/i2c/busses/i2c-piix4.c

  1288	
  1289	static int sb800_asf_add_adap(struct pci_dev *dev)
  1290	{
  1291		struct i2c_piix4_adapdata *adapdata;
  1292		struct resource_entry *rentry;
  1293		struct sb800_asf_data data;
  1294		struct list_head res_list;
  1295		struct acpi_device *adev;
  1296		acpi_status status;
  1297		acpi_handle handle;
  1298		int ret;
  1299	
  1300		status = acpi_get_handle(NULL, SB800_ASF_ACPI_PATH, &handle);
  1301		if (ACPI_FAILURE(status))
  1302			return -ENODEV;
  1303	
  1304		adev = acpi_fetch_acpi_dev(handle);
  1305		if (!adev)
  1306			return -ENODEV;
  1307	
  1308		INIT_LIST_HEAD(&res_list);
  1309		ret = acpi_dev_get_resources(adev, &res_list, NULL, NULL);
  1310		if (ret < 0) {
  1311			dev_err(&dev->dev, "Error getting ASF ACPI resource: %d\n", ret);
  1312			return ret;
  1313		}
  1314	
  1315		list_for_each_entry(rentry, &res_list, node) {
  1316			switch (resource_type(rentry->res)) {
  1317			case IORESOURCE_IO:
  1318				data.addr = rentry->res->start;
  1319				break;
  1320			case IORESOURCE_IRQ:
  1321				data.irq = rentry->res->start;
  1322				break;
  1323			default:
> 1324				dev_warn(&adev->dev, "Invalid ASF resource\n");
  1325				break;
  1326			}
  1327		}
  1328	
  1329		acpi_dev_free_resource_list(&res_list);
  1330		ret = piix4_add_adapter(dev, data.addr, SMBUS_ASF, piix4_adapter_count, false, 0,
  1331					piix4_main_port_names_sb800[piix4_adapter_count],
  1332					&piix4_main_adapters[piix4_adapter_count]);
  1333		if (ret) {
  1334			dev_err(&dev->dev, "Failed to add ASF adapter: %d\n", ret);
  1335			return -ENODEV;
  1336		}
  1337	
  1338		adapdata = i2c_get_adapdata(piix4_main_adapters[piix4_adapter_count]);
  1339		ret = devm_request_irq(&dev->dev, data.irq, sb800_asf_irq_handler, IRQF_SHARED,
  1340				       "sb800_smbus_asf", adapdata);
  1341		if (ret) {
  1342			dev_err(&dev->dev, "Unable to request irq: %d for use\n", data.irq);
  1343			return ret;
  1344		}
  1345	
  1346		INIT_DELAYED_WORK(&adapdata->work_buf, sb800_asf_process_slave);
  1347		adapdata->is_asf = true;
  1348		/* Increment the adapter count by 1 as ASF is added to the list */
  1349		piix4_adapter_count++;
  1350		return 1;
  1351	}
  1352	

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

[PATCH v3 4/5] i2c: piix4: Adjust the SMBus debug message

[Shyam Sundar S K]

With the addition of ASF, the current adapter information must now
correctly print the SMBus node details, whether it pertains to PIIX4 or
ASF. Update the driver to reflect this change accordingly.

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i2c/busses/i2c-piix4.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 2bf9611d864a..6abbaeaf2810 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -1195,6 +1195,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 {
 	struct i2c_adapter *adap;
 	struct i2c_piix4_adapdata *adapdata;
+	const char *node = "PIIX4";
 	int retval;
 
 	adap = kzalloc(sizeof(*adap), GFP_KERNEL);
@@ -1214,6 +1215,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 		adap->algo = &piix4_smbus_algorithm_sb800;
 		break;
 	case SMBUS_ASF:
+		node = "ASF";
 		adap->algo = &sb800_asf_smbus_algorithm;
 		break;
 	default:
@@ -1244,7 +1246,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 	}
 
 	snprintf(adap->name, sizeof(adap->name),
-		"SMBus PIIX4 adapter%s at %04x", name, smba);
+		"SMBus %s adapter%s at %04x", node, name, smba);
 
 	i2c_set_adapdata(adap, adapdata);
 
-- 
2.25.1

[PATCH v3 5/5] i2c: piix4: Clear remote IRR bit to get successive interrupt

[Shyam Sundar S K]

To ensure successive interrupts upon packet reception, it is necessary to
clear the remote IRR bit by writing the interrupt number to the EOI
register. The base address for this operation is provided by the BIOS and
retrieved by the driver by traversing the ASF object's namespace.

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i2c/busses/i2c-piix4.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 6abbaeaf2810..bf79b2280613 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -188,6 +188,8 @@ struct sb800_mmio_cfg {
 };
 
 struct sb800_asf_data {
+	resource_size_t eoi_addr;
+	resource_size_t eoi_sz;
 	unsigned short addr;
 	int irq;
 };
@@ -199,6 +201,7 @@ enum piix4_algo {
 };
 
 struct i2c_piix4_adapdata {
+	void __iomem *eoi_base;
 	unsigned short smba;
 
 	/* SB800 */
@@ -1285,6 +1288,7 @@ static irqreturn_t sb800_asf_irq_handler(int irq, void *ptr)
 		sb800_asf_update_bits(piix4_smba, SB800_ASF_SLV_INTR, SMBHSTSTS, true);
 	}
 
+	iowrite32(irq, adapdata->eoi_base);
 	return IRQ_HANDLED;
 }
 
@@ -1322,6 +1326,10 @@ static int sb800_asf_add_adap(struct pci_dev *dev)
 		case IORESOURCE_IRQ:
 			data.irq = rentry->res->start;
 			break;
+		case IORESOURCE_MEM:
+			data.eoi_addr = rentry->res->start;
+			data.eoi_sz = resource_size(rentry->res);
+			break;
 		default:
 			dev_warn(&adev->dev, "Invalid ASF resource\n");
 			break;
@@ -1346,6 +1354,9 @@ static int sb800_asf_add_adap(struct pci_dev *dev)
 	}
 
 	INIT_DELAYED_WORK(&adapdata->work_buf, sb800_asf_process_slave);
+	adapdata->eoi_base = devm_ioremap(&dev->dev, data.eoi_addr, data.eoi_sz);
+	if (!adapdata->eoi_base)
+		return -ENOMEM;
 	adapdata->is_asf = true;
 	/* Increment the adapter count by 1 as ASF is added to the list */
 	piix4_adapter_count++;
-- 
2.25.1