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;
>> +}
>
