| Message ID | 20240822142200.686842-4-Shyam-sundar.S-k@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add ASF Controller Support to the i2c-piix4 driver | expand |
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
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
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))