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