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

[Shyam Sundar S K <Shyam-sundar.S-k@amd.com>]

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

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

	...
}

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

You mean to say make the dependencies as:

depends on PCI && HAS_IOPORT && ACPI

instead of:

depends on PCI && HAS_IOPORT && X86

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

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

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

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

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

Primary/secondary
Controller/Worker
Requester/Responder

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

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

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

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

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

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

if (ret < 0)
	return ret;

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

Thanks,
Shyam

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