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/7/2024 00:21, Shyam Sundar S K wrote:
> 
> 
> 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.

Andy, I posted a new version. Can you please take a look. This has a
separate _HID driver for ASF now with piix4 as library.

Thanks,
Shyam

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