Skip to content

[26.04_linux-nvidia] pinctrl: mediatek: MT8901 ACPI wake-event EINT support and bus-hold bias - #564

Open
kmaddaraki wants to merge 2 commits into
NVIDIA:26.04_linux-nvidiafrom
kmaddaraki:fastos-pinctrl
Open

[26.04_linux-nvidia] pinctrl: mediatek: MT8901 ACPI wake-event EINT support and bus-hold bias#564
kmaddaraki wants to merge 2 commits into
NVIDIA:26.04_linux-nvidiafrom
kmaddaraki:fastos-pinctrl

Conversation

@kmaddaraki

Copy link
Copy Markdown

Two SAUCE patches for the MediaTek MT8901 pinctrl driver under ACPI, ported from the FastOS kernel.

pinctrl: mt8901: Enable ACPI wake-event EINT support

On MT8901 ACPI platforms some EINT wake sources belong to pins that are owned by firmware and have no Linux consumer driver, so nothing calls enable_irq_wake() for them and the platform cannot be woken from system sleep through those pins. The ACPI GPIO interrupts declared for the controller (_AEI) are also never requested after the pinctrl driver probes, so the ACPI event handlers for those pins do not run. Add a wake-event pin list to mtk_eint that the SoC driver populates, program it on init and resume, force wake_mask for the declared pins so the state survives suspend/resume without a consumer driver, declare the MT8901 event pins in the SoC data, and call acpi_gpiochip_request_interrupts() once the GPIO chip has been added.

pinctrl: mediatek: Add bus-hold bias and program-by-GPIO helper

The MediaTek bias code only knows pull-up and pull-down. The pad bus-keeper mode (PU and PD both enabled) cannot be requested or read back, yet the MT8901 SoundWire pads need it: the SoundWire controller's _CRS carries PinFunction() vendor encodings asking for bus-keeper on the data lines, and without it codec enumeration fails unless the pad registers are written by hand. Add MTK_BUS_HOLD as a third pull mode in the PU/PD path, wire PIN_CONFIG_BIAS_BUS_HOLD through pinctrl-paris get/set, and reject it on the RSEL and R1R0 paths that cannot express it. Register each mtk_pinctrl instance at probe and export mtk_pinctrl_program_bias_by_gpio() through <linux/soc/mediatek/mtk-pinctrl.h> so a client driver can apply pad bias by absolute SoC GPIO number. Nothing in this tree calls the helper yet; the SoundWire manager that consumes it will follow in a separate PR.

Existing DT and pull-up/pull-down behaviour is unchanged.

@nirmoy nirmoy added the help wanted Extra attention is needed label Sep 2, 2026
@nirmoy

nirmoy commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

BaseOS Kernel Review

Warning

⚠️ Review needs attention

The new MediaTek GPIO bias helper bypasses the pinctrl core configuration lock, allowing PU/PD updates to race with normal pinconf operations and report success with incorrect hardware state.

Findings: Critical 0 · High 0 · Medium 1 · Low 0

🔍 Review artifacts

📦 Kernel deb builds — 🟢 2/2 passed

Note

Build reports and debs are retained for 10 days after the PR closes.

Review metadata
  • Reviewed head: 1f8500beee46
  • Overall status: attention needed
  • Architectures: 2/2 successful

This comment is maintained by BaseOS Reviewer and updated when the GitHub watcher publishes a newer review.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

PR Validation Report

Patchscan ✅ No Missing Fixes

All cherry-picked commits checked — no missing upstream fixes found.

PR Lint ❌ Errors found

Details
Checking 2 commits...

Cherry-pick digest:
┌──────────────┬──────────────────────────────────────────────────────────────────┬────────────┬─────────┬───────────────────────────┐
│ Local        │ Referenced upstream / Patch subject                              │ Patch-ID   │ Subject │ SoB chain                 │
├──────────────┼──────────────────────────────────────────────────────────────────┼────────────┼─────────┼───────────────────────────┤
│ 1f8500beee46 │ [SAUCE] pinctrl: mediatek: add bus-hold bias and program-by-gpio │ N/A        │ N/A     │ paralika, kmaddara        │
├──────────────┼──────────────────────────────────────────────────────────────────┼────────────┼─────────┼───────────────────────────┤
│ 761ead28432d │ [SAUCE] pinctrl: mt8901: enable acpi wake-event eint support     │ N/A        │ N/A     │ pant, kmaddara            │
└──────────────┴──────────────────────────────────────────────────────────────────┴────────────┴─────────┴───────────────────────────┘

Lint: all checks passed.

PR metadata:
E: PR targets 26.04_linux-nvidia but body has no https://bugs.launchpad.net/... link

@clsotog

clsotog commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

I got this finding with Codex:
[P1] bias-bus-hold currently disables bias instead of enabling bus hold. drivers/pinctrl/mediatek/pinctrl-paris.c:291 forwards the generic config argument, but generic bias-bus-hold is defined with argument 0 and documented as “argument is ignored” (drivers/pinctrl/pinconf-generic.c:174, /tmp/NV-Kernels-pr564/include/linux/pinctrl/pinconf-
generic.h:25).
The new MediaTek PU/PD setter checks arg == MTK_DISABLE first, so that 0 clears both PU and PD instead of setting both for keeper mode (drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c:627).
Fix by passing MTK_ENABLE for PIN_CONFIG_BIAS_BUS_HOLD, or by making the MTK_BUS_HOLD path ignore arg and enable PU+PD.

On MT8901-based ACPI platforms some EINT wake sources belong to pins
that are owned by firmware and have no Linux consumer driver. Two
things are missing for such a pin to wake the SoC from system sleep.

First, the EINT block only forwards a wake event to the SPM when the
pin's bit in the EINT event mask is cleared. Nothing in the driver
programs that register, so the event never reaches the SPM.

Second, the ACPI GPIO interrupts declared in the controller's _AEI
are never requested after the pinctrl driver probes. Without that
request there is no IRQ consumer for the pin, so nothing calls
enable_irq_wake() on it, its bit in wake_mask stays clear, and
mtk_eint_do_suspend() masks the interrupt for the duration of
suspend. The SPM cannot see the event from a masked EINT.

Add a wake-event pin list to mtk_eint so the SoC driver can declare
the pins whose event mask must be cleared, and clear it on init and
on resume so the setting survives suspend/resume. Declare EINT 20 for
MT8901. Call acpi_gpiochip_request_interrupts() once the GPIO chip has
been added so gpiolib-acpi requests the _AEI interrupts and, for the
wake-capable ones, calls enable_irq_wake(). That sets the pin's bit
in wake_mask through the normal irq_set_wake path, so the EINT stays
unmasked across suspend without any driver touching wake_mask directly.

Signed-off-by: Ankit Pant <ankit.pant@mediatek.com>
Signed-off-by: Kiran Maddaraki <kmaddaraki@nvidia.com>
@kmaddaraki

Copy link
Copy Markdown
Author

Thanks Carol, confirmed. The generic parser passes argument 0 for bias-bus-hold and the PU/PD setter read that as MTK_DISABLE. Fixed in the updated head by passing MTK_ENABLE for PIN_CONFIG_BIAS_BUS_HOLD, since presence of the property means enable. Boro flagged the same issue.

@kmaddaraki

Copy link
Copy Markdown
Author

Updated head addresses the BaseOS review:

  • bias-bus-hold: PIN_CONFIG_BIAS_BUS_HOLD now always requests MTK_ENABLE; the generic property carries no argument.
  • program-by-GPIO helper: refuses with -EINVAL and warns if more than one registered pinctrl instance could own the pin, instead of programming the first match. Every supported platform registers a single instance, so behaviour is unchanged there. Public signature unchanged, so the consumer in [26.04_linux-nvidia-bos] N1x audio: MediaTek mt8901 SoundWire/ASoC/SOF stack #567 is unaffected.
  • Comment and kernel-doc nits fixed.
  • wake_mask finding: no code change. MediaTek confirmed the EINT wake path needs two things: the event mask cleared so the event reaches the SPM, and the EINT unmasked during suspend. The patch clears the event mask directly, and the unmask comes from gpiolib-acpi calling enable_irq_wake() on the wake-capable _AEI GpioInt once acpi_gpiochip_request_interrupts() is called, which sets the pin's wake_mask bit through the normal irq_set_wake path. The commit message previously claimed the patch forced wake_mask itself; it now describes the actual mechanism.

…IO helper

The MediaTek pinctrl bias code only knows pull-up and pull-down. The
pad "bus keeper" mode, with both PU and PD enabled, cannot be
requested or read back, yet the MT8901 SoundWire pads need it: the
ACPI _CRS for the SoundWire controller carries PinFunction() vendor
encodings that ask for bus-keeper on the data lines, and without it
codec enumeration fails unless the pad registers are written by hand.

Add MTK_BUS_HOLD as a third pull mode in the PU/PD bias path and wire
PIN_CONFIG_BIAS_BUS_HOLD through the pinctrl-paris get/set handlers.
The generic bias-bus-hold property carries no argument, so the set
handler always requests MTK_ENABLE for it. The RSEL and R1R0 bias
paths cannot express bus-hold and reject it explicitly. The bias
getters report PU=1, PD=1 back as MTK_BUS_HOLD.

Register every mtk_pinctrl instance in a global list at probe and
export mtk_pinctrl_program_bias_by_gpio() through
<linux/soc/mediatek/mtk-pinctrl.h>, so a client driver that parses
PinFunction() vendor encodings from its own _CRS can apply pad bias
by absolute SoC GPIO number without going through pinctrl states.
The instance is unregistered via a devm action so it leaves the list
before devm frees it. MediaTek pin numbers are zero-based per
controller, so the helper refuses with -EINVAL if more than one
registered controller could own the requested pin instead of
programming whichever probed first. Every supported platform
registers a single pinctrl instance, so this cannot trigger today.

Signed-off-by: shubham.paralikar <shubham.paralikar@mediatek.com>
Signed-off-by: Kiran Maddaraki <kmaddaraki@nvidia.com>
@kmaddaraki

Copy link
Copy Markdown
Author

On the remaining Medium, the helper returning -EINVAL when more than one controller could own a pin:

This cannot happen on any platform this driver supports. MediaTek describes all pads of an SoC under a single pinctrl device, and MT8901 exposes exactly one (NVDA9221). The upstream MediaTek device trees follow the same shape: mt8186, mt8188, mt8192 and mt8195 each have one pinctrl node with several register banks inside it, never two nodes. So the instance list always has one entry and the helper is unambiguous. The guard exists so that a hypothetical future two-controller platform fails loudly instead of programming whichever controller probed first, which was the original High.

@clsotog

clsotog commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the changes. I got another finding let me know:
drivers/pinctrl/mediatek/pinctrl-paris.c:1153 returns directly if the new devm_add_action_or_reset(... mtk_paris_unregister_instance ...) allocation fails. At that point gpiochip_add_data() and pinctrl_add_gpio_range() have already succeeded, so probe failure leaves a registered gpiochip/range backed by devm-freed hw. Mirror the cleanup used at /drivers/pinctrl/mediatek/pinctrl-paris.c:1134: remove the range and gpiochip_remove(&hw->chip) before returning.

@jamieNguyenNVIDIA

Copy link
Copy Markdown
Collaborator

I independently reached the same gpiochip lifetime finding Carol reported on
PR 564. Since the two PRs carry patch-identical series, it also applies to
PR 565 if that head remains unchanged.

In addition to Carol's finding, I have a few non-blocking comments:

  • [minor] The exported helper accepts only a bare per-controller pin
    number, although the ACPI PinFunction() resource identifies its
    controller. The author has confirmed that supported platforms have one
    pinctrl instance, so this is not a current functional blocker, but retaining
    the controller identity would make the public API less restrictive.
  • [minor] The second commit says the RSEL path rejects bus-hold. Composite
    RSEL+PU/PD pins instead skip RSEL and succeed through PU/PD, retaining the
    existing RSEL value. Please reword the commit message accordingly.
  • [nit] The new dev_info() is in the shared mtk_eint_do_init() path,
    so it adds an info-level boot message on every MediaTek platform using that
    path. Would dev_dbg() be more appropriate?
  • [nit] eint_event and total_wake_eints describe an event-mask
    exception list, not all wake-capable EINTs. Names reflecting that purpose
    would be clearer.
  • [nit] MTK_SDW_PIN_CFG_* describes SoundWire-specific ACPI encodings
    that pinctrl does not consume. These constants would fit better with the
    SoundWire consumer than in the public MediaTek pinctrl header.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

help wanted Extra attention is needed pending_review_comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants