| Message ID | 20240911115407.1090046-5-Shyam-sundar.S-k@amd.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Andi Shyti |
| Headers | show |
| Series | Introduce initial AMD ASF Controller driver support | expand |
On Wed, Sep 11, 2024 at 05:24:03PM +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. With brief look this is much better than the previous version(s). Thank you for rewriting it this way! Some comments below. ... > +#include <linux/acpi.h> No need (see below) + device.h + errno.h + gfp_types.h > +#include <linux/i2c-smbus.h> This should be i2c.h + mod_devicetable.h + module.h > +#include <linux/platform_device.h> > +#include <linux/slab.h> Not in use. + sprintf.h > +#include "i2c-piix4.h" > + > +static const char *sb800_asf_port_name = " port 1"; > + > +struct amd_asf_dev { > + struct device *dev; > + struct i2c_adapter adap; Make it first member, it might help if we ever do a container_of() against this. > + struct sb800_mmio_cfg mmio_cfg; > + unsigned short port_addr; What you probably want is to have void __iomem *addr; and use devm_ioport_map() somewhere (see drivers/pinctl/intel/pinctrl-lynxpoint.c, for example) > +}; > +static int amd_asf_probe(struct platform_device *pdev) > +{ > + struct resource_entry *rentry; > + struct amd_asf_dev *asf_dev; > + struct acpi_device *adev; > + LIST_HEAD(res_list); > + int ret; > + adev = ACPI_COMPANION(&pdev->dev); > + if (!adev) > + return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get ASF device\n"); No need. You will get here only if enumerated via ACPI (or if it's out-of-tree board file which we do not care about at all). > + 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; > + platform_set_drvdata(pdev, asf_dev); Is it used? > + asf_dev->adap.owner = THIS_MODULE; > + asf_dev->mmio_cfg.use_mmio = true; > + asf_dev->adap.class = I2C_CLASS_HWMON; > + ret = acpi_dev_get_resources(adev, &res_list, NULL, NULL); > + if (ret < 0) > + return dev_err_probe(&pdev->dev, ret, "Error getting ASF ACPI resource: %d\n", ret); > + > + list_for_each_entry(rentry, &res_list, node) { > + switch (resource_type(rentry->res)) { > + case IORESOURCE_IO: > + asf_dev->port_addr = rentry->res->start; > + break; > + default: > + dev_warn(&adev->dev, "Invalid ASF resource\n"); > + break; > + } > + } > + > + acpi_dev_free_resource_list(&res_list); Now this is a duplicate of what ACPI glue layer does. You have these already available as platform device resources. > + /* Set up the sysfs linkage to our parent device */ > + asf_dev->adap.dev.parent = &pdev->dev; > + > + snprintf(asf_dev->adap.name, sizeof(asf_dev->adap.name), > + "SMBus ASF adapter%s at %04x", sb800_asf_port_name, asf_dev->port_addr); > + i2c_set_adapdata(&asf_dev->adap, asf_dev); Is it used? > + ret = i2c_add_adapter(&asf_dev->adap); Use devm variant of this casll. > + if (ret) { > + release_region(asf_dev->port_addr, SMBIOSIZE); Why? > + return ret; > + } > + > + return 0; return devm_i2c_add_adapter(...); > +} > + > +static void amd_asf_remove(struct platform_device *pdev) > +{ > + struct amd_asf_dev *dev = platform_get_drvdata(pdev); > + if (dev->port_addr) { Redundant. > + i2c_del_adapter(&dev->adap); With devm this should be removed. > + release_region(dev->port_addr, SMBIOSIZE); Why? > + } > +} > + > +static const struct acpi_device_id amd_asf_acpi_ids[] = { > + {"AMDI001A", 0}, { "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, > + .remove_new = amd_asf_remove, > +}; > +module_platform_driver(amd_asf_driver); ... > + status = acpi_get_handle(NULL, (acpi_string)SB800_ASF_ACPI_PATH, &handle); Does it compile with CONFIG_ACPI=n? Also don't you need to include acpi.h for this? Or is it already there? (I haven't checked). > + if (ACPI_SUCCESS(status)) > + is_asf = true;
On 9/11/2024 20:46, Andy Shevchenko wrote: > On Wed, Sep 11, 2024 at 05:24:03PM +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. > > With brief look this is much better than the previous version(s). > Thank you for rewriting it this way! > > Some comments below. > > ... > >> +#include <linux/acpi.h> > > No need (see below) > > + device.h > + errno.h > + gfp_types.h > >> +#include <linux/i2c-smbus.h> > > This should be i2c.h > > + mod_devicetable.h > + module.h > >> +#include <linux/platform_device.h> > >> +#include <linux/slab.h> > > Not in use. > > + sprintf.h > >> +#include "i2c-piix4.h" >> + >> +static const char *sb800_asf_port_name = " port 1"; >> + >> +struct amd_asf_dev { >> + struct device *dev; >> + struct i2c_adapter adap; > > Make it first member, it might help if we ever do a container_of() against > this. > >> + struct sb800_mmio_cfg mmio_cfg; >> + unsigned short port_addr; > > What you probably want is to have > > void __iomem *addr; > I will address the above remarks in the next patch. I believe this should remain "unsigned short" because - it is defined a unsigned short in i2c_piix4 - this is just a port address (like 0xb00, 0xb20) and not a real iomem address. > and use devm_ioport_map() somewhere (see > drivers/pinctl/intel/pinctrl-lynxpoint.c, for example) > >> +}; > >> +static int amd_asf_probe(struct platform_device *pdev) >> +{ >> + struct resource_entry *rentry; >> + struct amd_asf_dev *asf_dev; >> + struct acpi_device *adev; >> + LIST_HEAD(res_list); >> + int ret; > >> + adev = ACPI_COMPANION(&pdev->dev); >> + if (!adev) >> + return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get ASF device\n"); > > No need. You will get here only if enumerated via ACPI (or if it's out-of-tree > board file which we do not care about at all). Not sure if I understand your comment correctly. But I used ACPI_COMPANION to retrieve the acpi device that needs to be passed to acpi_dev_get_resources(struct acpi_device *, ...) to address your previous remarks. > >> + 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; > >> + platform_set_drvdata(pdev, asf_dev); > > Is it used? > >> + asf_dev->adap.owner = THIS_MODULE; >> + asf_dev->mmio_cfg.use_mmio = true; >> + asf_dev->adap.class = I2C_CLASS_HWMON; > >> + ret = acpi_dev_get_resources(adev, &res_list, NULL, NULL); >> + if (ret < 0) >> + return dev_err_probe(&pdev->dev, ret, "Error getting ASF ACPI resource: %d\n", ret); >> + >> + list_for_each_entry(rentry, &res_list, node) { >> + switch (resource_type(rentry->res)) { >> + case IORESOURCE_IO: >> + asf_dev->port_addr = rentry->res->start; >> + break; >> + default: >> + dev_warn(&adev->dev, "Invalid ASF resource\n"); >> + break; >> + } >> + } >> + >> + acpi_dev_free_resource_list(&res_list); > > Now this is a duplicate of what ACPI glue layer does. You have these already > available as platform device resources. looking at drivers/acpi/resource.c acpi_dev_get_resources() mentions that the caller should call acpi_dev_free_resource_list(). Is that not the case? > >> + /* Set up the sysfs linkage to our parent device */ >> + asf_dev->adap.dev.parent = &pdev->dev; >> + >> + snprintf(asf_dev->adap.name, sizeof(asf_dev->adap.name), >> + "SMBus ASF adapter%s at %04x", sb800_asf_port_name, asf_dev->port_addr); > >> + i2c_set_adapdata(&asf_dev->adap, asf_dev); > > Is it used? Yes, in the subsequent patches. > >> + ret = i2c_add_adapter(&asf_dev->adap); > > Use devm variant of this casll. > >> + if (ret) { > >> + release_region(asf_dev->port_addr, SMBIOSIZE); > > Why? > >> + return ret; >> + } >> + >> + return 0; > > return devm_i2c_add_adapter(...); > >> +} >> + >> +static void amd_asf_remove(struct platform_device *pdev) >> +{ >> + struct amd_asf_dev *dev = platform_get_drvdata(pdev); > >> + if (dev->port_addr) { > > Redundant. > >> + i2c_del_adapter(&dev->adap); > > With devm this should be removed. > >> + release_region(dev->port_addr, SMBIOSIZE); > > Why? My bad :-( Will remove it. > >> + } >> +} >> + >> +static const struct acpi_device_id amd_asf_acpi_ids[] = { >> + {"AMDI001A", 0}, > > { "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, >> + .remove_new = amd_asf_remove, >> +}; >> +module_platform_driver(amd_asf_driver); > > ... > >> + status = acpi_get_handle(NULL, (acpi_string)SB800_ASF_ACPI_PATH, &handle); > > Does it compile with CONFIG_ACPI=n? I have used a explicit 'depends on' ACPI to this driver soon that LKP does not complain with a randconfig. > > Also don't you need to include acpi.h for this? Or is it already there? > (I haven't checked). acpi_get_handle() is defined in acpi.h. please assume the rest of the unanswered remarks as "I agree" :-) Thanks, Shyam > >> + if (ACPI_SUCCESS(status)) >> + is_asf = true; >
On Wed, Sep 11, 2024 at 10:07:42PM +0530, Shyam Sundar S K wrote: > On 9/11/2024 20:46, Andy Shevchenko wrote: > > On Wed, Sep 11, 2024 at 05:24:03PM +0530, Shyam Sundar S K wrote: ... > >> + struct i2c_adapter adap; > > > > Make it first member, it might help if we ever do a container_of() against > > this. > > > >> + struct sb800_mmio_cfg mmio_cfg; > >> + unsigned short port_addr; > > > > What you probably want is to have > > > > void __iomem *addr; > > I will address the above remarks in the next patch. > > I believe this should remain "unsigned short" because > > - it is defined a unsigned short in i2c_piix4 > - this is just a port address (like 0xb00, 0xb20) and not a real iomem > address. It all depends on how you use that. The devm_ioport_map() is just a trick (in combination with ioreadXX()/iowriteXX() APIs) to have it in "mapped" address space. > > and use devm_ioport_map() somewhere (see > > drivers/pinctl/intel/pinctrl-lynxpoint.c, for example) ... > >> + LIST_HEAD(res_list); > >> + adev = ACPI_COMPANION(&pdev->dev); > >> + if (!adev) > >> + return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get ASF device\n"); > > > > No need. You will get here only if enumerated via ACPI (or if it's out-of-tree > > board file which we do not care about at all). > > Not sure if I understand your comment correctly. But I used > ACPI_COMPANION to retrieve the acpi device that needs to be passed to > acpi_dev_get_resources(struct acpi_device *, ...) to address your > previous remarks. With platform device driver you don't need all this as you are repeating what drivers/acpi/acpi_platform.c already does it for all ACPI devices. > >> + ret = acpi_dev_get_resources(adev, &res_list, NULL, NULL); > >> + if (ret < 0) > >> + return dev_err_probe(&pdev->dev, ret, "Error getting ASF ACPI resource: %d\n", ret); > >> + > >> + list_for_each_entry(rentry, &res_list, node) { > >> + switch (resource_type(rentry->res)) { > >> + case IORESOURCE_IO: > >> + asf_dev->port_addr = rentry->res->start; > >> + break; > >> + default: > >> + dev_warn(&adev->dev, "Invalid ASF resource\n"); > >> + break; > >> + } > >> + } > >> + > >> + acpi_dev_free_resource_list(&res_list); > > > > Now this is a duplicate of what ACPI glue layer does. You have these already > > available as platform device resources. > > looking at drivers/acpi/resource.c acpi_dev_get_resources() mentions > that the caller should call acpi_dev_free_resource_list(). Is that not > the case? I meant that entire block is just a dup of the existing code. See above. Instead use simple platform_get_resource() and similar to retrieve this information. ... > >> + i2c_set_adapdata(&asf_dev->adap, asf_dev); > > > > Is it used? > > Yes, in the subsequent patches. I wasn't Cc'ed on the series. Please, make sure you Cc me in the entire series. ... > >> + status = acpi_get_handle(NULL, (acpi_string)SB800_ASF_ACPI_PATH, &handle); > > > > Does it compile with CONFIG_ACPI=n? > > I have used a explicit 'depends on' ACPI to this driver soon that LKP > does not complain with a randconfig. > > > Also don't you need to include acpi.h for this? Or is it already there? > > (I haven't checked). > > acpi_get_handle() is defined in acpi.h. Yes, and I meant piix4 driver where you call this API. > please assume the rest of the unanswered remarks as "I agree" :-) Noted! > >> + if (ACPI_SUCCESS(status)) > >> + is_asf = true;
On 9/11/2024 22:07, Shyam Sundar S K wrote: > > > On 9/11/2024 20:46, Andy Shevchenko wrote: >> On Wed, Sep 11, 2024 at 05:24:03PM +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. >> >> With brief look this is much better than the previous version(s). >> Thank you for rewriting it this way! >> >> Some comments below. >> >> ... >> >>> +#include <linux/acpi.h> >> >> No need (see below) >> >> + device.h >> + errno.h >> + gfp_types.h >> >>> +#include <linux/i2c-smbus.h> >> >> This should be i2c.h >> >> + mod_devicetable.h >> + module.h >> >>> +#include <linux/platform_device.h> >> >>> +#include <linux/slab.h> >> >> Not in use. >> >> + sprintf.h >> >>> +#include "i2c-piix4.h" >>> + >>> +static const char *sb800_asf_port_name = " port 1"; >>> + >>> +struct amd_asf_dev { >>> + struct device *dev; >>> + struct i2c_adapter adap; >> >> Make it first member, it might help if we ever do a container_of() against >> this. >> >>> + struct sb800_mmio_cfg mmio_cfg; >>> + unsigned short port_addr; >> >> What you probably want is to have >> >> void __iomem *addr; >> > > I will address the above remarks in the next patch. > > I believe this should remain "unsigned short" because > > - it is defined a unsigned short in i2c_piix4 > - this is just a port address (like 0xb00, 0xb20) and not a real iomem > address. > > >> and use devm_ioport_map() somewhere (see >> drivers/pinctl/intel/pinctrl-lynxpoint.c, for example) >> >>> +}; >> >>> +static int amd_asf_probe(struct platform_device *pdev) >>> +{ >>> + struct resource_entry *rentry; >>> + struct amd_asf_dev *asf_dev; >>> + struct acpi_device *adev; >>> + LIST_HEAD(res_list); >>> + int ret; >> >>> + adev = ACPI_COMPANION(&pdev->dev); >>> + if (!adev) >>> + return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get ASF device\n"); >> >> No need. You will get here only if enumerated via ACPI (or if it's out-of-tree >> board file which we do not care about at all). > > Not sure if I understand your comment correctly. But I used > ACPI_COMPANION to retrieve the acpi device that needs to be passed to > acpi_dev_get_resources(struct acpi_device *, ...) to address your > previous remarks. > >> >>> + 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; >> >>> + platform_set_drvdata(pdev, asf_dev); >> >> Is it used? >> >>> + asf_dev->adap.owner = THIS_MODULE; >>> + asf_dev->mmio_cfg.use_mmio = true; >>> + asf_dev->adap.class = I2C_CLASS_HWMON; >> >>> + ret = acpi_dev_get_resources(adev, &res_list, NULL, NULL); >>> + if (ret < 0) >>> + return dev_err_probe(&pdev->dev, ret, "Error getting ASF ACPI resource: %d\n", ret); >>> + >>> + list_for_each_entry(rentry, &res_list, node) { >>> + switch (resource_type(rentry->res)) { >>> + case IORESOURCE_IO: >>> + asf_dev->port_addr = rentry->res->start; >>> + break; >>> + default: >>> + dev_warn(&adev->dev, "Invalid ASF resource\n"); >>> + break; >>> + } >>> + } >>> + >>> + acpi_dev_free_resource_list(&res_list); >> >> Now this is a duplicate of what ACPI glue layer does. You have these already >> available as platform device resources. > > looking at drivers/acpi/resource.c acpi_dev_get_resources() mentions > that the caller should call acpi_dev_free_resource_list(). Is that not > the case? Ignore this. I understand what you mean now.. Thanks, Shyam > >> >>> + /* Set up the sysfs linkage to our parent device */ >>> + asf_dev->adap.dev.parent = &pdev->dev; >>> + >>> + snprintf(asf_dev->adap.name, sizeof(asf_dev->adap.name), >>> + "SMBus ASF adapter%s at %04x", sb800_asf_port_name, asf_dev->port_addr); >> >>> + i2c_set_adapdata(&asf_dev->adap, asf_dev); >> >> Is it used? > > Yes, in the subsequent patches. > >> >>> + ret = i2c_add_adapter(&asf_dev->adap); >> >> Use devm variant of this casll. >> >>> + if (ret) { >> >>> + release_region(asf_dev->port_addr, SMBIOSIZE); >> >> Why? >> >>> + return ret; >>> + } >>> + >>> + return 0; >> >> return devm_i2c_add_adapter(...); >> >>> +} >>> + >>> +static void amd_asf_remove(struct platform_device *pdev) >>> +{ >>> + struct amd_asf_dev *dev = platform_get_drvdata(pdev); >> >>> + if (dev->port_addr) { >> >> Redundant. >> >>> + i2c_del_adapter(&dev->adap); >> >> With devm this should be removed. >> >>> + release_region(dev->port_addr, SMBIOSIZE); >> >> Why? > > My bad :-( Will remove it. > >> >>> + } >>> +} >>> + >>> +static const struct acpi_device_id amd_asf_acpi_ids[] = { >>> + {"AMDI001A", 0}, >> >> { "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, >>> + .remove_new = amd_asf_remove, >>> +}; >>> +module_platform_driver(amd_asf_driver); >> >> ... >> >>> + status = acpi_get_handle(NULL, (acpi_string)SB800_ASF_ACPI_PATH, &handle); >> >> Does it compile with CONFIG_ACPI=n? > > I have used a explicit 'depends on' ACPI to this driver soon that LKP > does not complain with a randconfig. > >> >> Also don't you need to include acpi.h for this? Or is it already there? >> (I haven't checked). > > acpi_get_handle() is defined in acpi.h. > > please assume the rest of the unanswered remarks as "I agree" :-) > > Thanks, > Shyam > >> >>> + if (ACPI_SUCCESS(status)) >>> + is_asf = true; >>
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index a22f9125322a..262a8193c0bc 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 ACPI && 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..eb3cd166850c --- /dev/null +++ b/drivers/i2c/busses/i2c-amd-asf-plat.c @@ -0,0 +1,109 @@ +// 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/acpi.h> +#include <linux/i2c-smbus.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include "i2c-piix4.h" + +static const char *sb800_asf_port_name = " port 1"; + +struct amd_asf_dev { + struct device *dev; + struct i2c_adapter adap; + struct sb800_mmio_cfg mmio_cfg; + unsigned short port_addr; +}; + +static int amd_asf_probe(struct platform_device *pdev) +{ + struct resource_entry *rentry; + struct amd_asf_dev *asf_dev; + struct acpi_device *adev; + LIST_HEAD(res_list); + int ret; + + adev = ACPI_COMPANION(&pdev->dev); + if (!adev) + return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get ASF device\n"); + + 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; + platform_set_drvdata(pdev, asf_dev); + + asf_dev->adap.owner = THIS_MODULE; + asf_dev->mmio_cfg.use_mmio = true; + asf_dev->adap.class = I2C_CLASS_HWMON; + + ret = acpi_dev_get_resources(adev, &res_list, NULL, NULL); + if (ret < 0) + return dev_err_probe(&pdev->dev, ret, "Error getting ASF ACPI resource: %d\n", ret); + + list_for_each_entry(rentry, &res_list, node) { + switch (resource_type(rentry->res)) { + case IORESOURCE_IO: + asf_dev->port_addr = rentry->res->start; + break; + default: + dev_warn(&adev->dev, "Invalid ASF resource\n"); + break; + } + } + + acpi_dev_free_resource_list(&res_list); + /* Set up the sysfs linkage to our parent device */ + asf_dev->adap.dev.parent = &pdev->dev; + + snprintf(asf_dev->adap.name, sizeof(asf_dev->adap.name), + "SMBus ASF adapter%s at %04x", sb800_asf_port_name, asf_dev->port_addr); + + i2c_set_adapdata(&asf_dev->adap, asf_dev); + ret = i2c_add_adapter(&asf_dev->adap); + if (ret) { + release_region(asf_dev->port_addr, SMBIOSIZE); + return ret; + } + + return 0; +} + +static void amd_asf_remove(struct platform_device *pdev) +{ + struct amd_asf_dev *dev = platform_get_drvdata(pdev); + + if (dev->port_addr) { + i2c_del_adapter(&dev->adap); + release_region(dev->port_addr, SMBIOSIZE); + } +} + +static const struct acpi_device_id amd_asf_acpi_ids[] = { + {"AMDI001A", 0}, + { } +}; +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, + .remove_new = amd_asf_remove, +}; +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 174cce254e96..ff1e378a7198 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 */ @@ -1021,6 +1022,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 && @@ -1083,10 +1087,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) {