diff mbox series

[v5,4/8] i2c: amd-asf: Add ACPI support for AMD ASF Controller

Message ID 20240913121110.1611340-5-Shyam-sundar.S-k@amd.com
State Superseded
Headers show
Series Introduce initial AMD ASF Controller driver support | expand

Commit Message

Shyam Sundar S K Sept. 13, 2024, 12:11 p.m. UTC 
The AMD ASF controller is presented to the operating system as an ACPI
device. The AMD ASF driver can use ACPI to obtain 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 should not be configured.

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            | 16 ++++++
 drivers/i2c/busses/Makefile           |  1 +
 drivers/i2c/busses/i2c-amd-asf-plat.c | 73 +++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-piix4.c        | 12 ++++-
 4 files changed, 101 insertions(+), 1 deletion(-)
 create mode 100644 drivers/i2c/busses/i2c-amd-asf-plat.c

Comments

Andy Shevchenko Sept. 13, 2024, 7:18 p.m. UTC | #1 
On Fri, Sep 13, 2024 at 05:41:06PM +0530, Shyam Sundar S K wrote:
> The AMD ASF controller is presented to the operating system as an ACPI
> device. The AMD ASF driver can use ACPI to obtain 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 should not be configured.

...

> +#include <linux/device.h>
> +#include <linux/errno.h>

+ gfp_types.h

> +#include <linux/io.h>
> +#include <linux/i2c.h>

Keep them ordered.

+ ioport.h // you use definitions from there

> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include <linux/sprintf.h>

...

> +static const char *amd_asf_port_name = " port 1";

It's a bit counter intuitive to have a leading space in the constants like
this...

...

> +struct amd_asf_dev {
> +	struct i2c_adapter adap;

> +	struct device *dev;

So, this is a dup of adap.dev.parent. Do you really need this shortcut?

> +	struct sb800_mmio_cfg mmio_cfg;
> +	struct resource *port_addr;
> +};

> +static int amd_asf_probe(struct platform_device *pdev)
> +{

With

	struct device *dev = &pdev->dev;

the following lines can be made shorter.

> +	struct amd_asf_dev *asf_dev;
> +
> +	asf_dev = devm_kzalloc(&pdev->dev, sizeof(*asf_dev), GFP_KERNEL);
> +	if (!asf_dev)
> +		return dev_err_probe(&pdev->dev, -ENOMEM, "Failed to allocate memory\n");
> +
> +	asf_dev->dev = &pdev->dev;
> +	asf_dev->mmio_cfg.use_mmio = true;

> +	platform_set_drvdata(pdev, asf_dev);

I believe this won't be needed, see the respective email reply in the series.

> +	asf_dev->port_addr = platform_get_resource(pdev, IORESOURCE_IO, 0);
> +	if (!asf_dev->port_addr)
> +		return dev_err_probe(&pdev->dev, -EINVAL, "missing IO resources\n");
> +
> +	asf_dev->adap.owner = THIS_MODULE;
> +	asf_dev->adap.dev.parent = &pdev->dev;
> +
> +	i2c_set_adapdata(&asf_dev->adap, asf_dev);
> +	snprintf(asf_dev->adap.name, sizeof(asf_dev->adap.name),
> +		 "SMBus ASF adapter%s at 0x%llx", amd_asf_port_name, asf_dev->port_addr->start);
> +
> +	return devm_i2c_add_adapter(&pdev->dev, &asf_dev->adap);
> +}
kernel test robot Sept. 13, 2024, 10:45 p.m. UTC | #2 
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-rc7 next-20240913]
[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-Change-the-parameter-list-of-piix4_transaction-function/20240913-201353
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20240913121110.1611340-5-Shyam-sundar.S-k%40amd.com
patch subject: [PATCH v5 4/8] i2c: amd-asf: Add ACPI support for AMD ASF Controller
config: arm-allmodconfig (https://download.01.org/0day-ci/archive/20240914/202409140624.TOshFT39-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/202409140624.TOshFT39-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/202409140624.TOshFT39-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/i2c/busses/i2c-amd-asf-plat.c: In function 'amd_asf_probe':
>> drivers/i2c/busses/i2c-amd-asf-plat.c:52:47: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 5 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
      52 |                  "SMBus ASF adapter%s at 0x%llx", amd_asf_port_name, asf_dev->port_addr->start);
         |                                            ~~~^                      ~~~~~~~~~~~~~~~~~~~~~~~~~
         |                                               |                                        |
         |                                               long long unsigned int                   resource_size_t {aka unsigned int}
         |                                            %x


vim +52 drivers/i2c/busses/i2c-amd-asf-plat.c

    30	
    31	static int amd_asf_probe(struct platform_device *pdev)
    32	{
    33		struct amd_asf_dev *asf_dev;
    34	
    35		asf_dev = devm_kzalloc(&pdev->dev, sizeof(*asf_dev), GFP_KERNEL);
    36		if (!asf_dev)
    37			return dev_err_probe(&pdev->dev, -ENOMEM, "Failed to allocate memory\n");
    38	
    39		asf_dev->dev = &pdev->dev;
    40		asf_dev->mmio_cfg.use_mmio = true;
    41		platform_set_drvdata(pdev, asf_dev);
    42	
    43		asf_dev->port_addr = platform_get_resource(pdev, IORESOURCE_IO, 0);
    44		if (!asf_dev->port_addr)
    45			return dev_err_probe(&pdev->dev, -EINVAL, "missing IO resources\n");
    46	
    47		asf_dev->adap.owner = THIS_MODULE;
    48		asf_dev->adap.dev.parent = &pdev->dev;
    49	
    50		i2c_set_adapdata(&asf_dev->adap, asf_dev);
    51		snprintf(asf_dev->adap.name, sizeof(asf_dev->adap.name),
  > 52			 "SMBus ASF adapter%s at 0x%llx", amd_asf_port_name, asf_dev->port_addr->start);
    53	
    54		return devm_i2c_add_adapter(&pdev->dev, &asf_dev->adap);
    55	}
    56
kernel test robot Sept. 19, 2024, 7:44 p.m. UTC | #3 
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 next-20240919]
[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-Change-the-parameter-list-of-piix4_transaction-function/20240913-201353
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20240913121110.1611340-5-Shyam-sundar.S-k%40amd.com
patch subject: [PATCH v5 4/8] i2c: amd-asf: Add ACPI support for AMD ASF Controller
config: arm-randconfig-003-20240919 (https://download.01.org/0day-ci/archive/20240919/202409192333.P0Nve307-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 8663a75fa2f31299ab8d1d90288d9df92aadee88)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240919/202409192333.P0Nve307-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/202409192333.P0Nve307-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/i2c/busses/i2c-amd-asf-plat.c:15:
   In file included from include/linux/i2c.h:19:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:21:
   In file included from include/linux/mm.h:2232:
   include/linux/vmstat.h:517:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     517 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> drivers/i2c/busses/i2c-amd-asf-plat.c:52:56: warning: format specifies type 'unsigned long long' but the argument has type 'resource_size_t' (aka 'unsigned int') [-Wformat]
      52 |                  "SMBus ASF adapter%s at 0x%llx", amd_asf_port_name, asf_dev->port_addr->start);
         |                                            ~~~~                      ^~~~~~~~~~~~~~~~~~~~~~~~~
         |                                            %x
   2 warnings generated.


vim +52 drivers/i2c/busses/i2c-amd-asf-plat.c

    30	
    31	static int amd_asf_probe(struct platform_device *pdev)
    32	{
    33		struct amd_asf_dev *asf_dev;
    34	
    35		asf_dev = devm_kzalloc(&pdev->dev, sizeof(*asf_dev), GFP_KERNEL);
    36		if (!asf_dev)
    37			return dev_err_probe(&pdev->dev, -ENOMEM, "Failed to allocate memory\n");
    38	
    39		asf_dev->dev = &pdev->dev;
    40		asf_dev->mmio_cfg.use_mmio = true;
    41		platform_set_drvdata(pdev, asf_dev);
    42	
    43		asf_dev->port_addr = platform_get_resource(pdev, IORESOURCE_IO, 0);
    44		if (!asf_dev->port_addr)
    45			return dev_err_probe(&pdev->dev, -EINVAL, "missing IO resources\n");
    46	
    47		asf_dev->adap.owner = THIS_MODULE;
    48		asf_dev->adap.dev.parent = &pdev->dev;
    49	
    50		i2c_set_adapdata(&asf_dev->adap, asf_dev);
    51		snprintf(asf_dev->adap.name, sizeof(asf_dev->adap.name),
  > 52			 "SMBus ASF adapter%s at 0x%llx", amd_asf_port_name, asf_dev->port_addr->start);
    53	
    54		return devm_i2c_add_adapter(&pdev->dev, &asf_dev->adap);
    55	}
    56
diff mbox series

Patch

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index a22f9125322a..03afcdbff209 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -95,6 +95,22 @@  config I2C_AMD_MP2
 	  This driver can also be built as modules.  If so, the modules will
 	  be called i2c-amd-mp2-pci and i2c-amd-mp2-plat.
 
+config I2C_AMD_ASF
+	tristate "AMD ASF I2C Controller Support"
+	depends on I2C_PIIX4
+	help
+	  This option enables support for the AMD ASF (Alert Standard Format)
+	  I2C controller. The AMD ASF controller is an SMBus controller with
+	  built-in ASF functionality, allowing it to issue generic SMBus
+	  packets and communicate with the DASH controller using MCTP over
+	  ASF.
+
+	  If you have an AMD system with ASF support and want to enable this
+	  functionality, say Y or M here. If unsure, say N.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called i2c_amd_asf_plat.
+
 config I2C_HIX5HD2
 	tristate "Hix5hd2 high-speed I2C driver"
 	depends on ARCH_HISI || ARCH_HIX5HD2 || COMPILE_TEST
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 78d0561339e5..74920380a337 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -38,6 +38,7 @@  obj-$(CONFIG_I2C_POWERMAC)	+= i2c-powermac.o
 # Embedded system I2C/SMBus host controller drivers
 obj-$(CONFIG_I2C_ALTERA)	+= i2c-altera.o
 obj-$(CONFIG_I2C_AMD_MP2)	+= i2c-amd-mp2-pci.o i2c-amd-mp2-plat.o
+obj-$(CONFIG_I2C_AMD_ASF)	+= i2c-amd-asf-plat.o
 obj-$(CONFIG_I2C_ASPEED)	+= i2c-aspeed.o
 obj-$(CONFIG_I2C_AT91)		+= i2c-at91.o
 i2c-at91-objs			:= i2c-at91-core.o i2c-at91-master.o
diff --git a/drivers/i2c/busses/i2c-amd-asf-plat.c b/drivers/i2c/busses/i2c-amd-asf-plat.c
new file mode 100644
index 000000000000..3d80805a7a3e
--- /dev/null
+++ b/drivers/i2c/busses/i2c-amd-asf-plat.c
@@ -0,0 +1,73 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * AMD Alert Standard Format Platform Driver
+ *
+ * Copyright (c) 2024, Advanced Micro Devices, Inc.
+ * All Rights Reserved.
+ *
+ * Authors: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
+ *	    Sanket Goswami <Sanket.Goswami@amd.com>
+ */
+
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/io.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/sprintf.h>
+#include "i2c-piix4.h"
+
+static const char *amd_asf_port_name = " port 1";
+
+struct amd_asf_dev {
+	struct i2c_adapter adap;
+	struct device *dev;
+	struct sb800_mmio_cfg mmio_cfg;
+	struct resource *port_addr;
+};
+
+static int amd_asf_probe(struct platform_device *pdev)
+{
+	struct amd_asf_dev *asf_dev;
+
+	asf_dev = devm_kzalloc(&pdev->dev, sizeof(*asf_dev), GFP_KERNEL);
+	if (!asf_dev)
+		return dev_err_probe(&pdev->dev, -ENOMEM, "Failed to allocate memory\n");
+
+	asf_dev->dev = &pdev->dev;
+	asf_dev->mmio_cfg.use_mmio = true;
+	platform_set_drvdata(pdev, asf_dev);
+
+	asf_dev->port_addr = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	if (!asf_dev->port_addr)
+		return dev_err_probe(&pdev->dev, -EINVAL, "missing IO resources\n");
+
+	asf_dev->adap.owner = THIS_MODULE;
+	asf_dev->adap.dev.parent = &pdev->dev;
+
+	i2c_set_adapdata(&asf_dev->adap, asf_dev);
+	snprintf(asf_dev->adap.name, sizeof(asf_dev->adap.name),
+		 "SMBus ASF adapter%s at 0x%llx", amd_asf_port_name, asf_dev->port_addr->start);
+
+	return devm_i2c_add_adapter(&pdev->dev, &asf_dev->adap);
+}
+
+static const struct acpi_device_id amd_asf_acpi_ids[] = {
+	{ "AMDI001A" },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, amd_asf_acpi_ids);
+
+static struct platform_driver amd_asf_driver = {
+	.driver = {
+		.name = "i2c-amd-asf",
+		.acpi_match_table = amd_asf_acpi_ids,
+	},
+	.probe = amd_asf_probe,
+};
+module_platform_driver(amd_asf_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("AMD Alert Standard Format Driver");
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 7e0fb51ce532..f4a65344b481 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -84,6 +84,7 @@ 
 
 #define SB800_PIIX4_FCH_PM_ADDR			0xFED80300
 #define SB800_PIIX4_FCH_PM_SIZE			8
+#define SB800_ASF_ACPI_PATH			"\\_SB.ASFC"
 
 /* insmod parameters */
 
@@ -1023,6 +1024,9 @@  static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	int retval;
 	bool is_sb800 = false;
+	bool is_asf = false;
+	acpi_status status;
+	acpi_handle handle;
 
 	if ((dev->vendor == PCI_VENDOR_ID_ATI &&
 	     dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
@@ -1085,10 +1089,16 @@  static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		}
 	}
 
+	status = acpi_get_handle(NULL, (acpi_string)SB800_ASF_ACPI_PATH, &handle);
+	if (ACPI_SUCCESS(status))
+		is_asf = true;
+
 	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) {