| Message ID | 20220524074007.2792445-3-Basavaraj.Natikar@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Enhancements to AMD pinctrl and implementation of AMD pinmux | expand |
Hi Basavaraj, thanks for your patch! On Tue, May 24, 2022 at 9:40 AM Basavaraj Natikar <Basavaraj.Natikar@amd.com> wrote: > Presently there is no way to change pinmux configuration runtime. > Hence add IOMUX details which can be used to configure IOMUX > gpio pins runtime to different functionalities. > > Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com> Interesting! > +static acpi_status acpi_get_iomux_region(acpi_handle handle, u32 level, > + void *ctx, void **return_value) > +{ > + struct acpi_namespace_node *node = handle; > + union acpi_operand_object *region_obj; > + struct amd_gpio *gpio_dev = ctx; > + > + /* Already mapped the IOMUX base */ > + if (gpio_dev->iomux_base) > + return AE_OK; > + > + /* Valid object */ > + if (!node || !node->object) > + return AE_OK; > + > + /* Valid operand or namespace node*/ > + if ((ACPI_GET_DESCRIPTOR_TYPE(node->object) != ACPI_DESC_TYPE_OPERAND) && > + (ACPI_GET_DESCRIPTOR_TYPE(node->object) != ACPI_DESC_TYPE_NAMED)) > + return AE_OK; > + > + /* Valid object type*/ > + if (node->object->common.type == ACPI_TYPE_LOCAL_DATA) > + return AE_OK; > + > + region_obj = node->object; > + if (!region_obj->region.handler) > + return AE_OK; > + > + if (region_obj->region.space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) > + return AE_OK; > + > + if (strncmp("IOMX", region_obj->region.node->name.ascii, strlen("IOMX"))) > + return AE_OK; > + > + gpio_dev->iomux_base = devm_ioremap(&gpio_dev->pdev->dev, > + region_obj->region.address, > + region_obj->region.length); > + if (!gpio_dev->iomux_base) > + dev_err(&gpio_dev->pdev->dev, "failed to devm_ioremap() iomux_base\n"); > + > + return AE_OK; > +} > + > +static void amd_update_iomux_info(struct amd_gpio *gpio_dev) > +{ > + acpi_handle sys_bus_handle; > + int status = acpi_get_handle(NULL, "\\_SB", &sys_bus_handle); > + > + if (ACPI_FAILURE(status)) { > + dev_err(&gpio_dev->pdev->dev, "Failed to get SB handle\n"); > + return; > + } > + > + status = acpi_walk_namespace(ACPI_TYPE_REGION, sys_bus_handle, ACPI_UINT32_MAX, > + acpi_get_iomux_region, NULL, gpio_dev, NULL); > + > + if (ACPI_FAILURE(status)) { > + dev_err(&gpio_dev->pdev->dev, "Failed to get acpi_get_iomux_region\n"); > + return; > + } > +} Oh this looks scary to me, make sure you get the review from the GPIO ACPI experts, Andy Shevchenko and/or Mika Westerberg. (Added on the To: line) Yours, Linus Walleij
>> + >> + status = acpi_walk_namespace(ACPI_TYPE_REGION, sys_bus_handle, ACPI_UINT32_MAX, >> + acpi_get_iomux_region, NULL, gpio_dev, NULL); >> + >> + if (ACPI_FAILURE(status)) { >> + dev_err(&gpio_dev->pdev->dev, "Failed to get acpi_get_iomux_region\n"); >> + return; >> + } >> +} > Oh this looks scary to me, make sure you get the review from the GPIO > ACPI experts, Andy Shevchenko and/or Mika Westerberg. (Added on the > To: line) Thanks Linus for the feedback. Hi Andy/Mika, Please provide your suggestions for this patch. Thanks, Basavaraj > > Yours, > Linus Walleij
Hi, On Tue, May 24, 2022 at 04:48:03PM +0530, Basavaraj Natikar wrote: > > >> + > >> + status = acpi_walk_namespace(ACPI_TYPE_REGION, sys_bus_handle, ACPI_UINT32_MAX, > >> + acpi_get_iomux_region, NULL, gpio_dev, NULL); > >> + > >> + if (ACPI_FAILURE(status)) { > >> + dev_err(&gpio_dev->pdev->dev, "Failed to get acpi_get_iomux_region\n"); > >> + return; > >> + } > >> +} > > Oh this looks scary to me, make sure you get the review from the GPIO > > ACPI experts, Andy Shevchenko and/or Mika Westerberg. (Added on the > > To: line) > > Thanks Linus for the feedback. > > Hi Andy/Mika, > > Please provide your suggestions for this patch. If this is about muxing pins, have you looked at the ACPI PinFunction () and related descriptors?
On 5/24/2022 5:07 PM, Mika Westerberg wrote: > Hi, > > On Tue, May 24, 2022 at 04:48:03PM +0530, Basavaraj Natikar wrote: >>>> + >>>> + status = acpi_walk_namespace(ACPI_TYPE_REGION, sys_bus_handle, ACPI_UINT32_MAX, >>>> + acpi_get_iomux_region, NULL, gpio_dev, NULL); >>>> + >>>> + if (ACPI_FAILURE(status)) { >>>> + dev_err(&gpio_dev->pdev->dev, "Failed to get acpi_get_iomux_region\n"); >>>> + return; >>>> + } >>>> +} >>> Oh this looks scary to me, make sure you get the review from the GPIO >>> ACPI experts, Andy Shevchenko and/or Mika Westerberg. (Added on the >>> To: line) >> Thanks Linus for the feedback. >> >> Hi Andy/Mika, >> >> Please provide your suggestions for this patch. > If this is about muxing pins, have you looked at the ACPI PinFunction () > and related descriptors? This is about finding below IOMX (pinmux region) from ACPI namespace. OperationRegion (IOMX, SystemMemory, 0xFED80D00, 0x0100) I did not find any other methods to get IOMUX system memory region.
On Tue, May 24, 2022 at 05:22:47PM +0530, Basavaraj Natikar wrote: > > On 5/24/2022 5:07 PM, Mika Westerberg wrote: > > > Hi, > > > > On Tue, May 24, 2022 at 04:48:03PM +0530, Basavaraj Natikar wrote: > >>>> + > >>>> + status = acpi_walk_namespace(ACPI_TYPE_REGION, sys_bus_handle, ACPI_UINT32_MAX, > >>>> + acpi_get_iomux_region, NULL, gpio_dev, NULL); > >>>> + > >>>> + if (ACPI_FAILURE(status)) { > >>>> + dev_err(&gpio_dev->pdev->dev, "Failed to get acpi_get_iomux_region\n"); > >>>> + return; > >>>> + } > >>>> +} > >>> Oh this looks scary to me, make sure you get the review from the GPIO > >>> ACPI experts, Andy Shevchenko and/or Mika Westerberg. (Added on the > >>> To: line) > >> Thanks Linus for the feedback. > >> > >> Hi Andy/Mika, > >> > >> Please provide your suggestions for this patch. > > If this is about muxing pins, have you looked at the ACPI PinFunction () > > and related descriptors? > > This is about finding below IOMX (pinmux region) from ACPI namespace. Yes, I know but you use it for muxing pins, no? > OperationRegion (IOMX, SystemMemory, 0xFED80D00, 0x0100) > > I did not find any other methods to get IOMUX system memory region. It seems to be standard BAR so simple memory descriptor in _CRS () probably works too and does not need any additional lines of code.
On 5/24/2022 5:37 PM, Mika Westerberg wrote: > On Tue, May 24, 2022 at 05:22:47PM +0530, Basavaraj Natikar wrote: >> On 5/24/2022 5:07 PM, Mika Westerberg wrote: >> >>> Hi, >>> >>> On Tue, May 24, 2022 at 04:48:03PM +0530, Basavaraj Natikar wrote: >>>>>> + >>>>>> + status = acpi_walk_namespace(ACPI_TYPE_REGION, sys_bus_handle, ACPI_UINT32_MAX, >>>>>> + acpi_get_iomux_region, NULL, gpio_dev, NULL); >>>>>> + >>>>>> + if (ACPI_FAILURE(status)) { >>>>>> + dev_err(&gpio_dev->pdev->dev, "Failed to get acpi_get_iomux_region\n"); >>>>>> + return; >>>>>> + } >>>>>> +} >>>>> Oh this looks scary to me, make sure you get the review from the GPIO >>>>> ACPI experts, Andy Shevchenko and/or Mika Westerberg. (Added on the >>>>> To: line) >>>> Thanks Linus for the feedback. >>>> >>>> Hi Andy/Mika, >>>> >>>> Please provide your suggestions for this patch. >>> If this is about muxing pins, have you looked at the ACPI PinFunction () >>> and related descriptors? >> This is about finding below IOMX (pinmux region) from ACPI namespace. > Yes, I know but you use it for muxing pins, no? > >> OperationRegion (IOMX, SystemMemory, 0xFED80D00, 0x0100) >> >> I did not find any other methods to get IOMUX system memory region. > It seems to be standard BAR so simple memory descriptor in _CRS () > probably works too and does not need any additional lines of code. 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 ) }) Return (RBUF) /* \_SB_.GPIO._CRS.RBUF */ }
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)
On Tue, May 24, 2022 at 10:01 AM Basavaraj Natikar <Basavaraj.Natikar@amd.com> wrote: > > Presently there is no way to change pinmux configuration runtime. > Hence add IOMUX details which can be used to configure IOMUX > gpio pins runtime to different functionalities. GPIO run time ... > +#include "../acpi/acpica/accommon.h" One shoudn't ever include this except ACPICA code. ... > +static acpi_status acpi_get_iomux_region(acpi_handle handle, u32 level, > + void *ctx, void **return_value) > +{ > + struct acpi_namespace_node *node = handle; > + union acpi_operand_object *region_obj; > + struct amd_gpio *gpio_dev = ctx; > + > + /* Already mapped the IOMUX base */ > + if (gpio_dev->iomux_base) > + return AE_OK; > + > + /* Valid object */ > + if (!node || !node->object) > + return AE_OK; > + > + /* Valid operand or namespace node*/ > + if ((ACPI_GET_DESCRIPTOR_TYPE(node->object) != ACPI_DESC_TYPE_OPERAND) && > + (ACPI_GET_DESCRIPTOR_TYPE(node->object) != ACPI_DESC_TYPE_NAMED)) > + return AE_OK; > + > + /* Valid object type*/ > + if (node->object->common.type == ACPI_TYPE_LOCAL_DATA) > + return AE_OK; > + > + region_obj = node->object; > + if (!region_obj->region.handler) > + return AE_OK; > + > + if (region_obj->region.space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) > + return AE_OK; > + > + if (strncmp("IOMX", region_obj->region.node->name.ascii, strlen("IOMX"))) > + return AE_OK; > + > + gpio_dev->iomux_base = devm_ioremap(&gpio_dev->pdev->dev, > + region_obj->region.address, > + region_obj->region.length); > + if (!gpio_dev->iomux_base) > + dev_err(&gpio_dev->pdev->dev, "failed to devm_ioremap() iomux_base\n"); > + > + return AE_OK; > +} Your commit message is quite poor, can you add a lot of missed details, i.e. how it's done in ACPI (DSDT), what the version of ACPI specification have you in mind when implementing this, etc. ... > + acpi_handle sys_bus_handle; > + int status = acpi_get_handle(NULL, "\\_SB", &sys_bus_handle); > + > + if (ACPI_FAILURE(status)) { > + dev_err(&gpio_dev->pdev->dev, "Failed to get SB handle\n"); > + return; > + } Usually you don't need an \\_SB handle. Why is it here?
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?
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".
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)
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.
On 5/25/2022 10:15 PM, Andy Shevchenko wrote: > 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. Thanks. Sounds good, will make this change. Thanks, Basavaraj
diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c index 1a7d686494ff..3058b6d35e47 100644 --- a/drivers/pinctrl/pinctrl-amd.c +++ b/drivers/pinctrl/pinctrl-amd.c @@ -32,6 +32,8 @@ #include <linux/pinctrl/pinconf.h> #include <linux/pinctrl/pinconf-generic.h> +#include "../acpi/acpica/accommon.h" + #include "core.h" #include "pinctrl-utils.h" #include "pinctrl-amd.h" @@ -958,6 +960,68 @@ static struct pinctrl_desc amd_pinctrl_desc = { .owner = THIS_MODULE, }; +static acpi_status acpi_get_iomux_region(acpi_handle handle, u32 level, + void *ctx, void **return_value) +{ + struct acpi_namespace_node *node = handle; + union acpi_operand_object *region_obj; + struct amd_gpio *gpio_dev = ctx; + + /* Already mapped the IOMUX base */ + if (gpio_dev->iomux_base) + return AE_OK; + + /* Valid object */ + if (!node || !node->object) + return AE_OK; + + /* Valid operand or namespace node*/ + if ((ACPI_GET_DESCRIPTOR_TYPE(node->object) != ACPI_DESC_TYPE_OPERAND) && + (ACPI_GET_DESCRIPTOR_TYPE(node->object) != ACPI_DESC_TYPE_NAMED)) + return AE_OK; + + /* Valid object type*/ + if (node->object->common.type == ACPI_TYPE_LOCAL_DATA) + return AE_OK; + + region_obj = node->object; + if (!region_obj->region.handler) + return AE_OK; + + if (region_obj->region.space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) + return AE_OK; + + if (strncmp("IOMX", region_obj->region.node->name.ascii, strlen("IOMX"))) + return AE_OK; + + gpio_dev->iomux_base = devm_ioremap(&gpio_dev->pdev->dev, + region_obj->region.address, + region_obj->region.length); + if (!gpio_dev->iomux_base) + dev_err(&gpio_dev->pdev->dev, "failed to devm_ioremap() iomux_base\n"); + + return AE_OK; +} + +static void amd_update_iomux_info(struct amd_gpio *gpio_dev) +{ + acpi_handle sys_bus_handle; + int status = acpi_get_handle(NULL, "\\_SB", &sys_bus_handle); + + if (ACPI_FAILURE(status)) { + dev_err(&gpio_dev->pdev->dev, "Failed to get SB handle\n"); + return; + } + + status = acpi_walk_namespace(ACPI_TYPE_REGION, sys_bus_handle, ACPI_UINT32_MAX, + acpi_get_iomux_region, NULL, gpio_dev, NULL); + + if (ACPI_FAILURE(status)) { + dev_err(&gpio_dev->pdev->dev, "Failed to get acpi_get_iomux_region\n"); + return; + } +} + static int amd_gpio_probe(struct platform_device *pdev) { int ret = 0; @@ -1052,6 +1116,7 @@ static int amd_gpio_probe(struct platform_device *pdev) platform_set_drvdata(pdev, gpio_dev); acpi_register_wakeup_handler(gpio_dev->irq, amd_gpio_check_wake, gpio_dev); + amd_update_iomux_info(gpio_dev); dev_dbg(&pdev->dev, "amd gpio driver loaded\n"); return ret; diff --git a/drivers/pinctrl/pinctrl-amd.h b/drivers/pinctrl/pinctrl-amd.h index de2bc9dddc9c..8296426f4c81 100644 --- a/drivers/pinctrl/pinctrl-amd.h +++ b/drivers/pinctrl/pinctrl-amd.h @@ -89,6 +89,7 @@ struct amd_function { struct amd_gpio { raw_spinlock_t lock; void __iomem *base; + void __iomem *iomux_base; const struct amd_pingroup *groups; u32 ngroups;
Presently there is no way to change pinmux configuration runtime. Hence add IOMUX details which can be used to configure IOMUX gpio pins runtime to different functionalities. Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com> --- drivers/pinctrl/pinctrl-amd.c | 65 +++++++++++++++++++++++++++++++++++ drivers/pinctrl/pinctrl-amd.h | 1 + 2 files changed, 66 insertions(+)