gpio: phytium: update phytium gpio controller driver support#1539
Open
newearth-ss wants to merge 7 commits intodeepin-community:linux-6.6.yfrom
Open
gpio: phytium: update phytium gpio controller driver support#1539newearth-ss wants to merge 7 commits intodeepin-community:linux-6.6.yfrom
newearth-ss wants to merge 7 commits intodeepin-community:linux-6.6.yfrom
Conversation
Improve gpio set affinity logic to ensure the correctness of sharing interruptions. Mainline: Open-Source Signed-off-by: Cui Fulong <cuifulong2112@phytium.com.cn> Signed-off-by: Li Yuze <liyuze@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
When gpio irq is enabled by firmware, kernel will report soft lock and rcu, which will cause it to freeze. So unable and clear irq before registering irq in gpio driver. Mainline: Open-Source Signed-off-by: Cui Fulong <cuifulong2112@phytium.com.cn> Signed-off-by: Li Yuze <liyuze@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
This patch add driver version information, which will be used to synchronize and manage driver patches in the future. Mainline: Open-Source Signed-off-by: Cui Fulong <cuifulong2112@phytium.com.cn> Signed-off-by: Li Yuze <liyuze@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
This patch supports irq_set_wake for phytium GPIO driver. It allows GPIOs to be configured as wakeup sources and wake the system from suspend. Mainline: Open-Source Signed-off-by: Cui Fulong <cuifulong2112@phytium.com.cn> Signed-off-by: Li Yuze <liyuze@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
Add irq_set_affinity interface for gpio-pci driver to use set affinity function normally. Mainline: Open-Source Signed-off-by: Cui Fulong <cuifulong2112@phytium.com.cn> Signed-off-by: Li Yuze <liyuze@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
Add irq clear macro definitions as 0xffffffff to make the code more standardized. And delete the redundant code about "is_resuming" variable initialization. Mainline: Open-Source Signed-off-by: Cui Fulong <cuifulong2112@phytium.com.cn> Signed-off-by: Li Yuze <liyuze@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
Add lock for irq_set_wake function to prevent concurrency access register issue. Mainline: Open-Source Signed-off-by: Cui Fulong <cuifulong2112@phytium.com.cn> Signed-off-by: Li Yuze <liyuze@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
Reviewer's GuideExtends the Phytium GPIO core, PCI, platform, and SGPIO drivers with IRQ wake support, refined affinity handling for single-parent IRQs, improved suspend/resume interrupt masking, explicit IRQ clearing on init/resume, and driver version macros. Sequence diagram for GPIO IRQ wake configuration via phytium_gpio_irq_set_wakesequenceDiagram
actor PMCore
participant IRQCore as IRQ_subsystem
participant IRQChip as phytium_gpio_irq_chip
participant Core as phytium_gpio_core
participant HW as GPIO_hardware
PMCore->>IRQCore: enable/disable_irq_wake(virtual_irq, enable)
IRQCore->>IRQChip: irq_set_wake(irq_data, enable)
IRQChip->>Core: phytium_gpio_irq_set_wake(irq_data, enable)
Core->>Core: irq_data_get_irq_chip_data
Core->>Core: gpiochip_get_data
Core->>Core: irqd_to_hwirq -> bit
alt dedicated_parent_irq
Core->>IRQCore: irq_set_irq_wake(gpio->irq[bit], enable)
else single_parent_irq
Core->>IRQCore: irq_set_irq_wake(gpio->irq[0], enable)
end
Core->>Core: update ctx.wake_en with BIT(bit)
alt gpio_is_resuming
Core->>HW: writel(~ctx.wake_en, regs + GPIO_INTMASK)
Core->>HW: writel(ctx.wake_en, regs + GPIO_INTEN)
end
Core-->>IRQChip: return 0
IRQChip-->>IRQCore: return
IRQCore-->>PMCore: completion
Sequence diagram for updated GPIO suspend and resume interrupt handlingsequenceDiagram
participant PM as PM_core
participant PCI as phytium_gpio_pci_or_platform_driver
participant Core as phytium_gpio_core
participant HW as GPIO_hardware
PM->>PCI: phytium_gpio_pci_suspend or phytium_gpio_suspend
PCI->>Core: save ctx.inten, ctx.intmask, ctx.int_type, ctx.int_polarity, ctx.debounce
PCI->>HW: readl current interrupt registers
PCI->>HW: writel(~ctx.wake_en, regs + GPIO_INTMASK)
PCI->>HW: writel(ctx.wake_en, regs + GPIO_INTEN)
PCI-->>PM: suspend_complete
PM->>PCI: phytium_gpio_pci_resume or phytium_gpio_resume
PCI->>HW: writel(ctx.ls_sync, regs + GPIO_LS_SYNC)
PCI->>HW: writel(ctx.intmask, regs + GPIO_INTMASK)
PCI->>HW: writel(ctx.int_type, regs + GPIO_INTTYPE_LEVEL)
PCI->>HW: writel(ctx.int_polarity, regs + GPIO_INT_POLARITY)
PCI->>HW: writel(ctx.debounce, regs + GPIO_DEBOUNCE)
PCI->>HW: writel(GPIO_CLEAR_IRQ, regs + GPIO_PORTA_EOI)
PCI->>HW: writel(ctx.inten, regs + GPIO_INTEN)
PCI->>Core: gpio->is_resuming = 0
PCI-->>PM: resume_complete
Updated class diagram for Phytium GPIO core context and IRQ operationsclassDiagram
class phytium_gpio_ctx {
u32 inten
u32 intmask
u32 int_type
u32 int_polarity
u32 raw_intstatus
u32 ls_sync
u32 debounce
u32 wake_en
}
class phytium_gpio {
struct gpio_chip gc
void __iomem *regs
raw_spinlock_t lock
struct phytium_gpio_ctx ctx
int irq[NGPIO_MAX]
int is_resuming
}
class gpio_chip {
int base
function get_direction(gc, offset)
function direction_input(gc, offset)
function direction_output(gc, offset, value)
struct irq_chip_data irq
}
class irq_chip_data {
unsigned int num_parents
int *parents
function handler
unsigned int default_type
}
class irq_chip {
function irq_print_chip(irq_data, seq_file)
function irq_enable(irq_data)
function irq_disable(irq_data)
function irq_set_wake(irq_data, enable)
function irq_set_affinity(irq_data, mask_val, force)
unsigned long flags
}
class phytium_gpio_core_api {
int phytium_gpio_get_direction(gc, offset)
void phytium_gpio_irq_print_chip(irq_data, seq_file)
void phytium_gpio_irq_enable(irq_data)
void phytium_gpio_irq_disable(irq_data)
void phytium_gpio_irq_handler(irq_desc)
int phytium_gpio_irq_set_wake(irq_data, enable)
int phytium_gpio_irq_set_affinity(irq_data, cpumask, force)
}
class phytium_gpio_pci_driver {
function phytium_gpio_pci_probe(pci_dev, pci_device_id)
function phytium_gpio_pci_remove(pci_dev)
function phytium_gpio_pci_suspend(device)
function phytium_gpio_pci_resume(device)
const char *version = PHYTIUM_GPIO_DRIVER_VERSION
}
class phytium_gpio_platform_driver {
function phytium_gpio_probe(platform_device)
function phytium_gpio_remove(platform_device)
function phytium_gpio_suspend(device)
function phytium_gpio_resume(device)
const char *version = PHYTIUM_GPIO_DRIVER_VERSION
}
class phytium_sgpio_driver {
const char *version = GPIO_SGPIO_DRIVER_VERSION
}
phytium_gpio_ctx <.. phytium_gpio : contains
phytium_gpio o-- gpio_chip : embeds
gpio_chip o-- irq_chip_data : irq
irq_chip_data ..> irq_chip : uses
phytium_gpio_core_api <.. phytium_gpio : implemented_by
phytium_gpio_pci_driver ..> phytium_gpio_core_api : calls
phytium_gpio_platform_driver ..> phytium_gpio_core_api : calls
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @newearth-ss. Thanks for your PR. I'm waiting for a deepin-community member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In phytium_gpio_irq_set_wake(), the function always returns 0 even if irq_set_irq_wake() fails; consider propagating the actual error code (and possibly including the IRQ number in the log) so callers can handle wake configuration failures correctly.
- The new wake_en state in phytium_gpio_ctx is used in suspend/resume and set_wake paths; ensure it is explicitly initialized to 0 at probe (rather than relying on implicit zeroing) to avoid undefined wake mask state on first suspend.
- GPIO_CLEAR_IRQ is hard-coded as 0xffffffff; if the hardware IRQ status width can vary with NGPIO or between variants, consider deriving this mask from the actual number of lines or a register-specific constant to avoid inadvertently touching undefined bits.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In phytium_gpio_irq_set_wake(), the function always returns 0 even if irq_set_irq_wake() fails; consider propagating the actual error code (and possibly including the IRQ number in the log) so callers can handle wake configuration failures correctly.
- The new wake_en state in phytium_gpio_ctx is used in suspend/resume and set_wake paths; ensure it is explicitly initialized to 0 at probe (rather than relying on implicit zeroing) to avoid undefined wake mask state on first suspend.
- GPIO_CLEAR_IRQ is hard-coded as 0xffffffff; if the hardware IRQ status width can vary with NGPIO or between variants, consider deriving this mask from the actual number of lines or a register-specific constant to avoid inadvertently touching undefined bits.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
Adds version metadata and improves Phytium GPIO interrupt handling across platform/PCI variants, including wake-from-suspend support.
Changes:
- Add
MODULE_VERSION()reporting for SGPIO and GPIO platform/PCI drivers. - Introduce
irq_set_wakesupport and a newwake_encontext field to manage wake-enabled GPIOs during suspend/resume. - Clear/disable pending/enable interrupt registers during probe and standardize IRQ clear constant usage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| drivers/gpio/gpio-phytium-sgpio.c | Adds driver version macro and exposes it via MODULE_VERSION(). |
| drivers/gpio/gpio-phytium-platform.c | Hooks irq_set_wake, clears IRQ/INTEN on probe, and updates suspend/resume interrupt masking based on wake configuration. |
| drivers/gpio/gpio-phytium-pci.c | Hooks irq_set_wake/affinity, clears IRQ/INTEN on probe, and updates suspend/resume interrupt masking based on wake configuration. |
| drivers/gpio/gpio-phytium-core.h | Adds IRQ clear constant, driver version macro, and wake_en in suspend context; declares phytium_gpio_irq_set_wake(). |
| drivers/gpio/gpio-phytium-core.c | Implements phytium_gpio_irq_set_wake() and hardens single-parent affinity handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+391
to
+395
| if (gpio->irq[bit]) | ||
| ret = irq_set_irq_wake(gpio->irq[bit], enable); | ||
| else | ||
| ret = irq_set_irq_wake(gpio->irq[0], enable); | ||
|
|
There was a problem hiding this comment.
phytium_gpio_irq_set_wake() treats gpio->irq[bit] as a boolean and may pass an invalid IRQ number to irq_set_irq_wake() (e.g., negative values in gpio->irq[] or a valid IRQ number of 0). It should instead select the parent IRQ based on gc->irq.num_parents (e.g., use parents[0] when num_parents==1, otherwise parents[bit] after validating bit < num_parents) and only call irq_set_irq_wake() with a non-negative IRQ.
Comment on lines
+396
to
+413
| if (ret < 0) | ||
| dev_err(gc->parent, "set gpio irq wake failed!\n"); | ||
|
|
||
| raw_spin_lock_irqsave(&gpio->lock, flags); | ||
|
|
||
| if (enable) { | ||
| ctx->wake_en |= BIT(bit); | ||
| if (gpio->is_resuming == 1) { | ||
| writel(~ctx->wake_en, gpio->regs + GPIO_INTMASK); | ||
| writel(ctx->wake_en, gpio->regs + GPIO_INTEN); | ||
| } | ||
| } else | ||
| ctx->wake_en &= ~BIT(bit); | ||
|
|
||
| raw_spin_unlock_irqrestore(&gpio->lock, flags); | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
phytium_gpio_irq_set_wake() logs errors from irq_set_irq_wake() but always returns 0. This prevents callers from detecting wake configuration failures; please propagate the return value (or an appropriate error) back to the IRQ core/PM subsystem.
Comment on lines
+407
to
+408
| } else | ||
| ctx->wake_en &= ~BIT(bit); |
There was a problem hiding this comment.
When disabling wake (enable==0), ctx->wake_en is updated but GPIO_INTMASK/GPIO_INTEN are only reprogrammed in the enable path (and only when is_resuming==1). If irq_set_wake(..., 0) is called while the device is in the suspended/is_resuming state, the hardware wake mask may remain stale. Consider updating GPIO_INTMASK/GPIO_INTEN in both the enable and disable paths when is_resuming is set.
Suggested change
| } else | |
| ctx->wake_en &= ~BIT(bit); | |
| } else { | |
| ctx->wake_en &= ~BIT(bit); | |
| if (gpio->is_resuming == 1) { | |
| writel(~ctx->wake_en, gpio->regs + GPIO_INTMASK); | |
| writel(ctx->wake_en, gpio->regs + GPIO_INTEN); | |
| } | |
| } |
| .irq_enable = phytium_gpio_irq_enable, | ||
| .irq_disable = phytium_gpio_irq_disable, | ||
| .irq_set_wake = phytium_gpio_irq_set_wake, | ||
| .irq_set_affinity = phytium_gpio_irq_set_affinity, |
There was a problem hiding this comment.
Indentation/alignment on this line uses spaces instead of the usual tab alignment used throughout the file/kernel style. This is likely to trigger checkpatch warnings; please align the assignment consistently with the surrounding irq_chip fields.
Suggested change
| .irq_set_affinity = phytium_gpio_irq_set_affinity, | |
| .irq_set_affinity = phytium_gpio_irq_set_affinity, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This patch updates the support for phytium gpio controller driver.
Summary by Sourcery
Update Phytium GPIO core, PCI, platform, and SGPIO drivers to improve interrupt handling, suspend/resume behavior, and expose driver versioning.
New Features:
Enhancements: