mbox series

[v3,0/5] Introduce AMD ASF Controller Support to the i2c-piix4 driver

Message ID 20240906071201.2254354-1-Shyam-sundar.S-k@amd.com
Headers show
Series Introduce AMD ASF Controller Support to the i2c-piix4 driver | expand

Message

Shyam Sundar S K Sept. 6, 2024, 7:11 a.m. UTC
The AMD 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.

v3:
----
 - Fix LKP reported issue by adding 'depends on X86'
 - Drop callback when using acpi_dev_get_resources()
 - Address other remarks from Andy on v2.

v2:
----
 - Change function signature from u8 to enum
 - Use default case in switch
 - Use acpi_dev_get_resources() and drop devm_kzalloc() usage
 - Fix LKP reported issues
 - Address other minor remarks from Andy and Andi Shyti

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/Kconfig     |   3 +-
 drivers/i2c/busses/i2c-piix4.c | 396 ++++++++++++++++++++++++++++++++-
 2 files changed, 389 insertions(+), 10 deletions(-)

Comments

Andy Shevchenko Sept. 6, 2024, 12:24 p.m. UTC | #1
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?

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

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.

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

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.

> +	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(...);

> +	}
> +
> +	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;
> +}
Shyam Sundar S K Sept. 6, 2024, 1:20 p.m. UTC | #2
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;
>> +}
>
Shyam Sundar S K Sept. 6, 2024, 3:11 p.m. UTC | #3
On 9/6/2024 20:10, Andy Shevchenko wrote:
> On Fri, Sep 06, 2024 at 06:50:48PM +0530, Shyam Sundar S K wrote:
>> On 9/6/2024 17:54, Andy Shevchenko wrote:
>>> On Fri, Sep 06, 2024 at 12:41:59PM +0530, Shyam Sundar S K wrote:
> 
> First of all, you haven't replied to some of my comments, I assume that you
> agree on them and are going to fix as suggested?

I agree with your comments. I have only requested further
clarification on a few points where I need more understanding.

> 
> ...
> 
>>>> 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)
>> {
>> 	...
> 
> Can you put the necessary bits for the enumeration (you may replace some IDs if
> they are not public yet to something like XX..XX or xx..xx)?
> 

Name (_HID, "AMDIXXXX")  // _HID: Hardware ID
Name (_UID, Zero)  // _UID: Unique ID

>>     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
> 
> Typo in value? Shouldn't this be 0x0b3f?

Its is 0xb20, that is meant for ASF.

> 
>>                 0x00,               // Alignment
>>                 0x20,               // Length
>>                 )
>>             Memory32Fixed (ReadWrite,
>>                 0xFEC00040,         // Address Base
>>                 0x00000100,         // Address Length
>>                 )
>>         })
>>         Return (ASBB) /* \_SB_.ASFC._CRS.ASBB */
>>     }
>> 	...
>> }
> 
> ...
> 
>>>> 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
> 
> Yes, but see below as well about the stubs
> 
> ~~~vvv
>>> 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).
> 
> ___^^^
> 
> ...
> 
>>>> +	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.
> 
> Yes.
> 
>> Not sure if I understand your remark completely.
> 
> 	if (...) {
> 		cmd = 0;
> 	} else {
> 		cmd = ...
> 	}
> 

Got it.

> ...
> 
>>>> +	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)?
> 
> As per official I2C specification. :-)

Thanks! I will change to controller/target instead of master/slave.

> 
>> Primary/secondary
>> Controller/Worker
>> Requester/Responder
> 
> See, e.g., a93c2e5fe766 ("i2c: reword i2c_algorithm according to newest specification").
> 
>>> 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);
>>>> +	}
> 
> ...
> 
>>>> +	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?
> 
> Not that parts that affect this...

Alright, I have posted the _HID enumeration details above. Please let
me know if using acpi_fetch_acpi_dev() is acceptable or if there's a
better alternative.

I am open to making changes based on these clarifications.

Thanks,
Shyam

> 
> ...
> 
>>>> +	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?
> 
> I assume you call this due to use of devm_*(). Either devm_*() should be
> replaced to non-devm_*() analogues, or these moved to dev_err_probe().
> 
>> 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?
> 
> No. It's also possible, but this is up to you.
> 
>> if (ret < 0)
>> 	return ret;
>>
>> Likewise for below remarks on dev_err_probe(...);
>
Shyam Sundar S K Sept. 6, 2024, 6:51 p.m. UTC | #4
On 9/6/2024 21:34, Andy Shevchenko wrote:
> On Fri, Sep 06, 2024 at 08:41:19PM +0530, Shyam Sundar S K wrote:
>> On 9/6/2024 20:10, Andy Shevchenko wrote:
>>> On Fri, Sep 06, 2024 at 06:50:48PM +0530, Shyam Sundar S K wrote:
>>>> 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)
>>>> {
>>>> 	...
>>>
>>> Can you put the necessary bits for the enumeration (you may replace some IDs if
>>> they are not public yet to something like XX..XX or xx..xx)?
>>
>> Name (_HID, "AMDIXXXX")  // _HID: Hardware ID
>> Name (_UID, Zero)  // _UID: Unique ID
> 
> Thank you!
> 
> Now a question, why your case can't have a separate (platform) device driver?

I evaluated this approach before proposing the change, considering the
option of creating a separate platform driver, which is relatively
easier to implement.

However, there are a couple of important points to note:

- ASF is a subset of SMBus. If a system has 3 SMBus ports, this change
would allow one of the ports to handle ASF operations.

- In the current i2c_piix4 driver, the assumption is that the port
address 0xb20 is designated for auxiliary operations, but this same
port can also be used for ASF. This could lead to a scenario of port
collision. I tried to highlight this in the commit message, and you
can see some dance in piix4_probe().

- As a result, users might encounter an error on platforms that
support ASF: "SMBus region 0x%x already in use!"

This is why I believe it would be more meaningful to integrate the ASF
changes into the SMBus driver.

Thoughts..?

Thanks,
Shyam

> 
>>>>     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
>>>
>>> Typo in value? Shouldn't this be 0x0b3f?
>>
>> Its is 0xb20, that is meant for ASF.
> 
> Yes, I mixed up IO() vs. Memory*() resource. The IO() has two values for
> the start address and you fixed that to the above mentioned value.
> 
> TL;DR: this looks okay.
> 
>>>>                 0x00,               // Alignment
>>>>                 0x20,               // Length
>>>>                 )
>>>>             Memory32Fixed (ReadWrite,
>>>>                 0xFEC00040,         // Address Base
>>>>                 0x00000100,         // Address Length
>>>>                 )
>>>>         })
>>>>         Return (ASBB) /* \_SB_.ASFC._CRS.ASBB */
>>>>     }
>>>> 	...
>>>> }
> 
> ...
> 
>>>>>> +	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?
>>>
>>> Not that parts that affect this...
>>
>> Alright, I have posted the _HID enumeration details above. Please let
>> me know if using acpi_fetch_acpi_dev() is acceptable or if there's a
>> better alternative.
> 
>> I am open to making changes based on these clarifications.
> 
> Since you have a proper Device object in ACPI, it seems to me that you should
> do other way around, i.e. having a platform device driver for this ACPI device
> (based on _HID) and use piix4 as a library for it.
>