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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Wed, May 25, 2022 at 03:12:05PM +0530, Basavaraj Natikar wrote:
> 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. 

Better to name them all, it will be more consistent and robust.

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

I don't believe it will anyhow alter Windows, except the new part not being
tested in Windows.

> one more flexible way is to directly use _DSD property to get memory
> region as below also works, 

Strongly no. We do not allow to repeat in _DSD the existing ACPI concepts.

...

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

It seems you missed the point of OpRegions in ACPI. We have a standart way of
subscribing to the OpRegion:s, if somebody in ACPI touches it, you will handle
writes and reads in the driver.

Using OpRegion in replace of _CRS is a huge abuse of ACPI. Fix your firmware,
because it sounds like it must be fixed.

-- 
With Best Regards,
Andy Shevchenko