[PATCH 0/5] Add ASF Controller Support to the i2c-piix4 driver

[PATCH 0/5] Add ASF Controller Support to the i2c-piix4 driver

[Shyam Sundar S K]

The 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.

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/i2c-piix4.c | 390 ++++++++++++++++++++++++++++++++-
 1 file changed, 381 insertions(+), 9 deletions(-)

-- 
2.25.1

[PATCH 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 | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 4e32d57ae0bf..2babe9a2291c 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 {
+	SMBUS_SB800,
+	SMBUS_LEGACY,
+};
+
 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,
+			     u8 piix4_algo, u8 port, bool notify_imc,
 			     u8 hw_port_nr, const char *name,
 			     struct i2c_adapter **padap)
 {
@@ -945,8 +951,15 @@ 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 (piix4_algo) {
+	case SMBUS_LEGACY:
+		adap->algo = &smbus_algorithm;
+		break;
+	case SMBUS_SB800:
+		adap->algo = &piix4_smbus_algorithm_sb800;
+		break;
+	}
 
 	adapdata = kzalloc(sizeof(*adapdata), GFP_KERNEL);
 	if (adapdata == NULL) {
@@ -957,7 +970,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 = piix4_algo;
 	adapdata->port = port << piix4_port_shift_sb800;
 	adapdata->notify_imc = notify_imc;
 
@@ -1013,7 +1026,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, SMBUS_SB800, port, notify_imc,
 					   hw_port_nr,
 					   piix4_main_port_names_sb800[port],
 					   &piix4_main_adapters[port]);
@@ -1085,7 +1098,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, SMBUS_LEGACY, 0, false, 0,
 					   "", &piix4_main_adapters[0]);
 		if (retval < 0)
 			return retval;
@@ -1114,7 +1127,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, SMBUS_LEGACY, 0, false, 1,
 				  is_sb800 ? piix4_aux_port_name_sb800 : "",
 				  &piix4_aux_adapter);
 	}
-- 
2.25.1

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

[Andi Shyti]

Hi Shyam,

...

> +enum piix4_algo {
> +	SMBUS_SB800,
> +	SMBUS_LEGACY,

please don't use the SMBUS_ prefix, this driver has its own
prefix which is PIIX4. I suggest calling these enums PIIX4_SMBUS
and PIIX4_SB800.

> +};
> +
>  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,
> +			     u8 piix4_algo, u8 port, bool notify_imc,
                             ^^
why "u8" and not "enum piix4_algo"?

Thanks,
Andi

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

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 | 187 +++++++++++++++++++++++++++++++++
 1 file changed, 187 insertions(+)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 2babe9a2291c..a44b53dd4dd7 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -50,6 +50,25 @@
 #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 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)
+
 /* count for request_region */
 #define SMBIOSIZE	9
 
@@ -101,6 +120,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 +188,7 @@ struct sb800_mmio_cfg {
 enum piix4_algo {
 	SMBUS_SB800,
 	SMBUS_LEGACY,
+	SMBUS_ASF,
 };
 
 struct i2c_piix4_adapdata {
@@ -179,6 +200,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 +909,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 +1143,9 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 	case SMBUS_SB800:
 		adap->algo = &piix4_smbus_algorithm_sb800;
 		break;
+	case SMBUS_ASF:
+		adap->algo = &sb800_asf_smbus_algorithm;
+		break;
 	}
 
 	adapdata = kzalloc(sizeof(*adapdata), GFP_KERNEL);
-- 
2.25.1

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

[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-rc5 next-20240823]
[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/20240826-113028
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20240822142200.686842-3-Shyam-sundar.S-k%40amd.com
patch subject: [PATCH 2/5] i2c: piix4: Add i2c_algorithm operations to support AMD ASF with SMBus
config: i386-randconfig-016-20240826 (https://download.01.org/0day-ci/archive/20240826/202408261818.T8eXqGIz-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/20240826/202408261818.T8eXqGIz-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/202408261818.T8eXqGIz-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

>> drivers/i2c/busses/i2c-piix4.c:1069:10: error: 'const struct i2c_algorithm' has no member named 'reg_slave'
    1069 |         .reg_slave = sb800_asf_reg_slave,
         |          ^~~~~~~~~
>> drivers/i2c/busses/i2c-piix4.c:1069:22: error: initialization of 'int (*)(struct i2c_adapter *, struct i2c_msg *, int)' from incompatible pointer type 'int (*)(struct i2c_client *)' [-Werror=incompatible-pointer-types]
    1069 |         .reg_slave = sb800_asf_reg_slave,
         |                      ^~~~~~~~~~~~~~~~~~~
   drivers/i2c/busses/i2c-piix4.c:1069:22: note: (near initialization for 'sb800_asf_smbus_algorithm.<anonymous>.xfer_atomic')
>> drivers/i2c/busses/i2c-piix4.c:1070:10: error: 'const struct i2c_algorithm' has no member named 'unreg_slave'
    1070 |         .unreg_slave = sb800_asf_unreg_slave,
         |          ^~~~~~~~~~~
>> drivers/i2c/busses/i2c-piix4.c:1070:24: error: initialization of 'int (*)(struct i2c_adapter *, u16,  short unsigned int,  char,  u8,  int,  union i2c_smbus_data *)' {aka 'int (*)(struct i2c_adapter *, short unsigned int,  short unsigned int,  char,  unsigned char,  int,  union i2c_smbus_data *)'} from incompatible pointer type 'int (*)(struct i2c_client *)' [-Werror=incompatible-pointer-types]
    1070 |         .unreg_slave = sb800_asf_unreg_slave,
         |                        ^~~~~~~~~~~~~~~~~~~~~
   drivers/i2c/busses/i2c-piix4.c:1070:24: note: (near initialization for 'sb800_asf_smbus_algorithm.smbus_xfer')
>> drivers/i2c/busses/i2c-piix4.c:1067:63: warning: missing braces around initializer [-Wmissing-braces]
    1067 | static const struct i2c_algorithm sb800_asf_smbus_algorithm = {
         |                                                               ^
    1068 |         .master_xfer = sb800_asf_xfer,
         |                                      }
    1069 |         .reg_slave = sb800_asf_reg_slave,
         |                      {                  }
   cc1: some warnings being treated as errors


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

  1066	
> 1067	static const struct i2c_algorithm sb800_asf_smbus_algorithm = {
  1068		.master_xfer = sb800_asf_xfer,
> 1069		.reg_slave = sb800_asf_reg_slave,
> 1070		.unreg_slave = sb800_asf_unreg_slave,
  1071		.functionality = sb800_asf_func,
  1072	};
  1073	

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

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

[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-rc5 next-20240823]
[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/20240826-113028
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20240822142200.686842-3-Shyam-sundar.S-k%40amd.com
patch subject: [PATCH 2/5] i2c: piix4: Add i2c_algorithm operations to support AMD ASF with SMBus
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20240826/202408261803.Zxa8c8JJ-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240826/202408261803.Zxa8c8JJ-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/202408261803.Zxa8c8JJ-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/i2c/busses/i2c-piix4.c:1069:3: error: field designator 'reg_slave' does not refer to any field in type 'const struct i2c_algorithm'
    1069 |         .reg_slave = sb800_asf_reg_slave,
         |         ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/i2c/busses/i2c-piix4.c:1070:3: error: field designator 'unreg_slave' does not refer to any field in type 'const struct i2c_algorithm'
    1070 |         .unreg_slave = sb800_asf_unreg_slave,
         |         ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   2 errors generated.


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

  1066	
  1067	static const struct i2c_algorithm sb800_asf_smbus_algorithm = {
  1068		.master_xfer = sb800_asf_xfer,
> 1069		.reg_slave = sb800_asf_reg_slave,
> 1070		.unreg_slave = sb800_asf_unreg_slave,
  1071		.functionality = sb800_asf_func,
  1072	};
  1073	

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

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

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 | 161 ++++++++++++++++++++++++++++++++-
 1 file changed, 160 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index a44b53dd4dd7..00fc641e6277 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -121,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 */
 
@@ -185,6 +187,11 @@ struct sb800_mmio_cfg {
 	bool use_mmio;
 };
 
+struct sb800_asf_data {
+	unsigned short addr;
+	int irq;
+};
+
 enum piix4_algo {
 	SMBUS_SB800,
 	SMBUS_LEGACY,
@@ -201,6 +208,8 @@ struct i2c_piix4_adapdata {
 	struct sb800_mmio_cfg mmio_cfg;
 	u8 algo_select;
 	struct i2c_client *slave;
+	bool is_asf;
+	struct delayed_work work_buf;
 };
 
 static int piix4_sb800_region_request(struct device *dev,
@@ -909,6 +918,66 @@ 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;
+	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)) {
+		/* 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);
+		}
+	} else {
+		/* Set bank as full */
+		reg = reg | (BIT(3) | BIT(2));
+		outb_p(reg, ASFDATABNKSEL);
+	}
+
+	outb_p(0, ASFSETDATARDPTR);
+	if (!(cmd & BIT(0))) {
+		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 +1264,86 @@ 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 = (struct i2c_piix4_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 acpi_status sb800_asf_acpi_resource_cb(struct acpi_resource *resource, void *context)
+{
+	struct sb800_asf_data *data = context;
+
+	switch (resource->type) {
+	case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
+		data->irq = resource->data.extended_irq.interrupts[0];
+		break;
+	case ACPI_RESOURCE_TYPE_IO:
+		data->addr = resource->data.io.minimum;
+		break;
+	}
+
+	return AE_OK;
+}
+
+static int sb800_asf_add_adap(struct pci_dev *dev)
+{
+	struct i2c_piix4_adapdata *adapdata;
+	struct sb800_asf_data *data;
+	acpi_status status;
+	acpi_handle handle;
+	int ret;
+
+	status = acpi_get_handle(NULL, SB800_ASF_ACPI_PATH, &handle);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	data = devm_kzalloc(&dev->dev, sizeof(struct sb800_asf_data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	status = acpi_walk_resources(handle, METHOD_NAME__CRS, sb800_asf_acpi_resource_cb, data);
+	if (ACPI_FAILURE(status))
+		return -EINVAL;
+
+	if (!data->addr)
+		return -EINVAL;
+
+	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 += 1;
+	return 1;
+}
+
 static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba,
 				    bool notify_imc)
 {
@@ -1243,6 +1392,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 +1429,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 +1462,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 +1482,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 3/5] i2c: piix4: Add ACPI support for ASF SMBus device

[kernel test robot]

Hi Shyam,

kernel test robot noticed the following build warnings:

[auto build test WARNING on andi-shyti/i2c/i2c-host]
[also build test WARNING on linus/master v6.11-rc5 next-20240823]
[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/20240826-113028
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20240822142200.686842-4-Shyam-sundar.S-k%40amd.com
patch subject: [PATCH 3/5] i2c: piix4: Add ACPI support for ASF SMBus device
config: i386-buildonly-randconfig-003-20240826 (https://download.01.org/0day-ci/archive/20240826/202408261816.iYeJHXHU-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240826/202408261816.iYeJHXHU-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/202408261816.iYeJHXHU-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/i2c/busses/i2c-piix4.c:935:6: warning: variable 'cmd' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
     935 |         if (!(reg & SB800_ASF_ERROR_STATUS)) {
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/i2c/busses/i2c-piix4.c:971:8: note: uninitialized use occurs here
     971 |         if (!(cmd & BIT(0))) {
         |               ^~~
   drivers/i2c/busses/i2c-piix4.c:935:2: note: remove the 'if' if its condition is always true
     935 |         if (!(reg & SB800_ASF_ERROR_STATUS)) {
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/i2c/busses/i2c-piix4.c:927:19: note: initialize the variable 'cmd' to silence this warning
     927 |         u8 bank, reg, cmd;
         |                          ^
         |                           = '\0'
   1 warning generated.


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

   920	
   921	static void sb800_asf_process_slave(struct work_struct *work)
   922	{
   923		struct i2c_piix4_adapdata *adapdata = container_of(work, struct i2c_piix4_adapdata,
   924								   work_buf.work);
   925		unsigned short piix4_smba = adapdata->smba;
   926		u8 data[SB800_ASF_BLOCK_MAX_BYTES];
   927		u8 bank, reg, cmd;
   928		u8 len, val = 0;
   929		int i;
   930	
   931		/* Read slave status register */
   932		reg = inb_p(ASFSLVSTA);
   933	
   934		/* Check if no error bits are set in slave status register */
 > 935		if (!(reg & SB800_ASF_ERROR_STATUS)) {
   936			/* Read data bank */
   937			reg = inb_p(ASFDATABNKSEL);
   938			bank = (reg & BIT(3)) >> 3;
   939	
   940			/* Set read data bank */
   941			if (bank) {
   942				reg = reg | BIT(4);
   943				reg = reg & (~BIT(3));
   944			} else {
   945				reg = reg & (~BIT(4));
   946				reg = reg & (~BIT(2));
   947			}
   948	
   949			/* Read command register */
   950			outb_p(reg, ASFDATABNKSEL);
   951			cmd = inb_p(ASFINDEX);
   952			len = inb_p(ASFDATARWPTR);
   953			for (i = 0; i < len; i++)
   954				data[i] = inb_p(ASFINDEX);
   955	
   956			/* Clear data bank status */
   957			if (bank) {
   958				reg = reg | BIT(3);
   959				outb_p(reg, ASFDATABNKSEL);
   960			} else {
   961				reg = reg | BIT(2);
   962				outb_p(reg, ASFDATABNKSEL);
   963			}
   964		} else {
   965			/* Set bank as full */
   966			reg = reg | (BIT(3) | BIT(2));
   967			outb_p(reg, ASFDATABNKSEL);
   968		}
   969	
   970		outb_p(0, ASFSETDATARDPTR);
   971		if (!(cmd & BIT(0))) {
   972			i2c_slave_event(adapdata->slave, I2C_SLAVE_WRITE_REQUESTED, &val);
   973			for (i = 0; i < len; i++) {
   974				val = data[i];
   975				i2c_slave_event(adapdata->slave, I2C_SLAVE_WRITE_RECEIVED, &val);
   976			}
   977			i2c_slave_event(adapdata->slave, I2C_SLAVE_STOP, &val);
   978		}
   979	}
   980	

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

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

[Shyam Sundar S K]

+Andy (this has some ACPI handling that adds AMD ASF support to the
existing piix4 driver for SMBus)

On 8/22/2024 19:51, 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.
> 
> 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.
> 
> 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 | 161 ++++++++++++++++++++++++++++++++-
>  1 file changed, 160 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index a44b53dd4dd7..00fc641e6277 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -121,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 */
>  
> @@ -185,6 +187,11 @@ struct sb800_mmio_cfg {
>  	bool use_mmio;
>  };
>  
> +struct sb800_asf_data {
> +	unsigned short addr;
> +	int irq;
> +};
> +
>  enum piix4_algo {
>  	SMBUS_SB800,
>  	SMBUS_LEGACY,
> @@ -201,6 +208,8 @@ struct i2c_piix4_adapdata {
>  	struct sb800_mmio_cfg mmio_cfg;
>  	u8 algo_select;
>  	struct i2c_client *slave;
> +	bool is_asf;
> +	struct delayed_work work_buf;
>  };
>  
>  static int piix4_sb800_region_request(struct device *dev,
> @@ -909,6 +918,66 @@ 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;
> +	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)) {
> +		/* 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);
> +		}
> +	} else {
> +		/* Set bank as full */
> +		reg = reg | (BIT(3) | BIT(2));
> +		outb_p(reg, ASFDATABNKSEL);
> +	}
> +
> +	outb_p(0, ASFSETDATARDPTR);
> +	if (!(cmd & BIT(0))) {
> +		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 +1264,86 @@ 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 = (struct i2c_piix4_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 acpi_status sb800_asf_acpi_resource_cb(struct acpi_resource *resource, void *context)
> +{
> +	struct sb800_asf_data *data = context;
> +
> +	switch (resource->type) {
> +	case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
> +		data->irq = resource->data.extended_irq.interrupts[0];
> +		break;
> +	case ACPI_RESOURCE_TYPE_IO:
> +		data->addr = resource->data.io.minimum;
> +		break;
> +	}
> +
> +	return AE_OK;
> +}
> +
> +static int sb800_asf_add_adap(struct pci_dev *dev)
> +{
> +	struct i2c_piix4_adapdata *adapdata;
> +	struct sb800_asf_data *data;
> +	acpi_status status;
> +	acpi_handle handle;
> +	int ret;
> +
> +	status = acpi_get_handle(NULL, SB800_ASF_ACPI_PATH, &handle);
> +	if (ACPI_FAILURE(status))
> +		return -ENODEV;
> +
> +	data = devm_kzalloc(&dev->dev, sizeof(struct sb800_asf_data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	status = acpi_walk_resources(handle, METHOD_NAME__CRS, sb800_asf_acpi_resource_cb, data);
> +	if (ACPI_FAILURE(status))
> +		return -EINVAL;
> +
> +	if (!data->addr)
> +		return -EINVAL;
> +
> +	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 += 1;
> +	return 1;
> +}
> +
>  static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba,
>  				    bool notify_imc)
>  {
> @@ -1243,6 +1392,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 +1429,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 +1462,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 +1482,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))

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

[Andy Shevchenko]

On Wed, Sep 04, 2024 at 01:06:16AM +0530, Shyam Sundar S K wrote:
> +Andy (this has some ACPI handling that adds AMD ASF support to the
> existing piix4 driver for SMBus)

Thanks.

> On 8/22/2024 19:51, 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.
> > 
> > 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.

...

> > +static acpi_status sb800_asf_acpi_resource_cb(struct acpi_resource *resource, void *context)
> > +{
> > +	struct sb800_asf_data *data = context;
> > +
> > +	switch (resource->type) {
> > +	case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
> > +		data->irq = resource->data.extended_irq.interrupts[0];
> > +		break;
> > +	case ACPI_RESOURCE_TYPE_IO:
> > +		data->addr = resource->data.io.minimum;
> > +		break;
> > +	}
> > +
> > +	return AE_OK;
> > +}
> > +
> > +static int sb800_asf_add_adap(struct pci_dev *dev)
> > +{
> > +	struct i2c_piix4_adapdata *adapdata;
> > +	struct sb800_asf_data *data;
> > +	acpi_status status;
> > +	acpi_handle handle;
> > +	int ret;

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

> > +	data = devm_kzalloc(&dev->dev, sizeof(struct sb800_asf_data), GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;

Why can't it be on stack?

> > +	status = acpi_walk_resources(handle, METHOD_NAME__CRS, sb800_asf_acpi_resource_cb, data);
> > +	if (ACPI_FAILURE(status))
> > +		return -EINVAL;
> > +
> > +	if (!data->addr)
> > +		return -EINVAL;

This is reinvention of acpi_dev_get_resources(). Many drivers are using it, you
may found a lot of examples.

> > +	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 += 1;
> > +	return 1;
> > +}

-- 
With Best Regards,
Andy Shevchenko

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

[Shyam Sundar S K]

On 9/4/2024 01:36, Andy Shevchenko wrote:
> On Wed, Sep 04, 2024 at 01:06:16AM +0530, Shyam Sundar S K wrote:
>> +Andy (this has some ACPI handling that adds AMD ASF support to the
>> existing piix4 driver for SMBus)
> 
> Thanks.
> 
>> On 8/22/2024 19:51, 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.
>>>
>>> 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.
> 
> ...
> 
>>> +static acpi_status sb800_asf_acpi_resource_cb(struct acpi_resource *resource, void *context)
>>> +{
>>> +	struct sb800_asf_data *data = context;
>>> +
>>> +	switch (resource->type) {
>>> +	case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
>>> +		data->irq = resource->data.extended_irq.interrupts[0];
>>> +		break;
>>> +	case ACPI_RESOURCE_TYPE_IO:
>>> +		data->addr = resource->data.io.minimum;
>>> +		break;
>>> +	}
>>> +
>>> +	return AE_OK;
>>> +}
>>> +
>>> +static int sb800_asf_add_adap(struct pci_dev *dev)
>>> +{
>>> +	struct i2c_piix4_adapdata *adapdata;
>>> +	struct sb800_asf_data *data;
>>> +	acpi_status status;
>>> +	acpi_handle handle;
>>> +	int ret;
> 
>>> +	status = acpi_get_handle(NULL, SB800_ASF_ACPI_PATH, &handle);
>>> +	if (ACPI_FAILURE(status))
>>> +		return -ENODEV;
> 
>>> +	data = devm_kzalloc(&dev->dev, sizeof(struct sb800_asf_data), GFP_KERNEL);
>>> +	if (!data)
>>> +		return -ENOMEM;
> 
> Why can't it be on stack?
> 
>>> +	status = acpi_walk_resources(handle, METHOD_NAME__CRS, sb800_asf_acpi_resource_cb, data);
>>> +	if (ACPI_FAILURE(status))
>>> +		return -EINVAL;
>>> +
>>> +	if (!data->addr)
>>> +		return -EINVAL;
> 
> This is reinvention of acpi_dev_get_resources(). Many drivers are using it, you
> may found a lot of examples.

Thank you for the quick feedback. I have submitted v2 addressing your
remarks. Kindly take a look.

Thanks,
Shyam

[PATCH 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 00fc641e6277..446e9235f900 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -1194,6 +1194,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 {
 	struct i2c_adapter *adap;
 	struct i2c_piix4_adapdata *adapdata;
+	char *node = "PIIX4";
 	int retval;
 
 	adap = kzalloc(sizeof(*adap), GFP_KERNEL);
@@ -1213,6 +1214,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;
 	}
@@ -1240,7 +1242,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

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

[Andi Shyti]

Hi Shyam,

> @@ -1194,6 +1194,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
>  {
>  	struct i2c_adapter *adap;
>  	struct i2c_piix4_adapdata *adapdata;
> +	char *node = "PIIX4";

please, make this const and initialize it...

>  	int retval;
>  
>  	adap = kzalloc(sizeof(*adap), GFP_KERNEL);
> @@ -1213,6 +1214,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
>  		adap->algo = &piix4_smbus_algorithm_sb800;
>  		break;

... here.

>  	case SMBUS_ASF:
> +		node = "ASF";
>  		adap->algo = &sb800_asf_smbus_algorithm;
>  		break;

shall we have a default case here? I thought checkpatch complains
when no default is specified.

Thanks,
Andi

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

[Shyam Sundar S K]

Hi Andi,

On 9/4/2024 03:21, Andi Shyti wrote:
> Hi Shyam,
> 
>> @@ -1194,6 +1194,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
>>  {
>>  	struct i2c_adapter *adap;
>>  	struct i2c_piix4_adapdata *adapdata;
>> +	char *node = "PIIX4";
> 
> please, make this const and initialize it...
> 
>>  	int retval;
>>  
>>  	adap = kzalloc(sizeof(*adap), GFP_KERNEL);
>> @@ -1213,6 +1214,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
>>  		adap->algo = &piix4_smbus_algorithm_sb800;
>>  		break;
> 
> ... here.
> 
>>  	case SMBUS_ASF:
>> +		node = "ASF";
>>  		adap->algo = &sb800_asf_smbus_algorithm;
>>  		break;
> 
> shall we have a default case here? I thought checkpatch complains
> when no default is specified.

checkpatch does not complain about it. I have added a default case
based on your suggestion in v2. Kindly have a look.

Thanks,
Shyam

> 
> Thanks,
> Andi

[PATCH 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 446e9235f900..146c0f31e26b 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 */
@@ -1281,6 +1284,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;
 }
 
@@ -1295,6 +1299,10 @@ static acpi_status sb800_asf_acpi_resource_cb(struct acpi_resource *resource, vo
 	case ACPI_RESOURCE_TYPE_IO:
 		data->addr = resource->data.io.minimum;
 		break;
+	case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
+		data->eoi_addr = resource->data.fixed_memory32.address;
+		data->eoi_sz = resource->data.fixed_memory32.address_length;
+		break;
 	}
 
 	return AE_OK;
@@ -1340,6 +1348,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 += 1;
-- 
2.25.1