Re: [PATCH v2 2/3] pinctrl: amd: Get and update IOMUX details

[Basavaraj Natikar <bnatikar@xxxxxxx>]

On 5/24/2022 10:09 PM, Andy Shevchenko wrote:

> On Tue, May 24, 2022 at 07:20:34PM +0530, Basavaraj Natikar wrote:
>> On 5/24/2022 6:04 PM, Mika Westerberg wrote:
>>> On Tue, May 24, 2022 at 05:54:47PM +0530, Basavaraj Natikar wrote:
>>>> There is no CRS method defined for IOMX/0xFED80D00 in ACPI namespace // IOMX Address Base 
>>>> Hence I added additional code to get IOMX memory region.
>>>>
>>>> since _CRS method is used to get GPIO pin base for AMDI0030 in
>>>> pinctrl-amd as below, same is not available for IOMX
>>>>  
>>>>   Device (GPIO)
>>>>         {
>>>>             Name (_HID, "AMDI0030")  // _HID: Hardware ID
>>>>             Name (_CID, "AMDI0030")  // _CID: Compatible ID
>>>>             Name (_UID, Zero)  // _UID: Unique ID
>>>>             Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
>>>>             {
>>>>                 Name (RBUF, ResourceTemplate ()
>>>>                 {
>>>>                     Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, )
>>>>                     {
>>>>                         0x00000007,
>>>>                     }
>>>>                     Memory32Fixed (ReadWrite,
>>>>                         0xFED81500,         // Address Base
>>>>                         0x00000400,         // Address Length
>>>>                         )
>>> Is there something preventing you to add it here like below?
>>>
>>>                      Memory32Fixed (ReadWrite, 0xFED80D00 0x00000400)
>> yes few system has different entries already defined like below      
>>   Device (GPIO)
>>         {
>>             Name (_HID, "AMDI0030")  // _HID: Hardware ID
>>             Name (_CID, "AMDI0030")  // _CID: Compatible ID
>>             Name (_UID, Zero)  // _UID: Unique ID
>>             Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
>>             {
>>                 Name (RBUF, ResourceTemplate ()
>>                 {
>>                     Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, )
>>                     {
>>                         0x00000007,
>>                     }
>>                     Memory32Fixed (ReadWrite,
>>                         0xFED81500,         // Address Base
>>                         0x00000400,         // Address Length
>>                         )
>>                 })
>>                 Return (RBUF) /* \_SB_.GPIO._CRS.RBUF */
>>             }
>>
>>
>>         Device (GPIO)
>>         {
>>             Name (_HID, "AMDI0030")  // _HID: Hardware ID
>>             Name (_CID, "AMDI0030")  // _CID: Compatible ID
>>             Name (_UID, Zero)  // _UID: Unique ID
>>             Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
>>             {
>>                 Name (RBUF, ResourceTemplate ()
>>                 {
>>                     Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, )
>>                     {
>>                         0x00000007,
>>                     }
>>                     Memory32Fixed (ReadWrite,
>>                         0xFED81500,         // Address Base
>>                         0x00000400,         // Address Length
>>                         )
>>                     Memory32Fixed (ReadWrite,
>>                         0xFED81200,         // Address Base
>>                         0x00000100,         // Address Length
>>                         )
>>                 })
>>                 Return (RBUF) /* \_SB_.GPIO._CRS.RBUF */
>>             }
>>
>> if we add or in future some entries added. 
>> is there way to map particular entry for IOMUX? 
> Straightforward way is to add it always to the end and add _DSD boolean
> property ("amd,pinctrl-iomux-present") and act accordingly. More flexible and
> less error prone is to name all resources with DSD property and find resource
> by name: "amd,pinctrl-resource-names" = "bank0", "bank1", "iomux".
>
Idea of adding _DSD property looks good, we can add easily _DSD
"pinctrl-iomux-present" u8 property to return index or instance
value directly. 

But systems already in use or old platforms needs BIOS update to avail 
this feature and it may also impact existing windows OS functionality.

one more flexible way is to directly use _DSD property to get memory
region as below also works, which also needs BIOS update to
avail feature.

                    Package (0x02)
                    {
                        "pinctrl-iomux",
                        Package (0x2)
                        {
                          0xFED80D00,
                          0x00000100
                        }
                    }

How about adding generic new function in drivers/acpi/utils.h to avoid
using "../acpi/acpica/accommon.h" in pinctrl-amd.c like below.

acpi_status acpi_get_sys_mem_region(acpi_handle handle, 
                           const char *nname, struct acpi_mem_space_context *res); 

can be used directly in acpi_get_iomux_region by calling like 
acpi_get_sys_mem_region(handle, "IOMX", &res)
This can be generically used to get details like below Operation regions 
which is already present in dsdt table.
OperationRegion (IOMX, SystemMemory, 0xFED80D00, 0x0100)