Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hal: microchip: Missing Wake bit definitions #34325

Closed
albertofloyd opened this issue Apr 16, 2021 · 2 comments · Fixed by #34588
Closed

hal: microchip: Missing Wake bit definitions #34325

albertofloyd opened this issue Apr 16, 2021 · 2 comments · Fixed by #34588
Assignees
Labels
bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug

Comments

@albertofloyd
Copy link
Collaborator

Describe the bug
None of the wake event (GIRQ21 and GIRQ22) are exposed in the HAL

KSC_INT, SMB-I2C _WAKE_ONLY, ESPI_WAKE_ONLY, PS2_0A_WK, etc

Expected behavior
Bit definitions to be added in HAL.

Impact
Magic numbers to be used inside SoC power module making this error prone or relying in old documentation.

@albertofloyd albertofloyd added the bug The issue is a bug, or the PR is fixing a bug label Apr 16, 2021
@albertofloyd albertofloyd changed the title HAL microchip: Missing Wake hal: microchip: Missing Wake bit defintions Apr 16, 2021
@scottwcpg
Copy link
Collaborator

The modules/hal/microchip/mec/mec1501/component/keyscan.h has defines for keyscan GIRQ21 NVIC value 135 and GIRQ21 bit position 21, and a define for the GIRQ mask value. There's a type in the NVIC, MCHP_KSAN_NVIC instead of MCHP_KSCAN_NVIC. I will fix. Is this sufficient for key scan definitions?
PS/2 component header ps2_ctrl.h is missing the GIRQ21 wake bits. Will add.
I will add the GIRQ22 bit definitions to the the ecia.h component header so they are in one place.

@albertofloyd
Copy link
Collaborator Author

Sounds like a good plan.
Note most interrupt values and aggregator bits are directly used in device tree (no macros required)
I'm referring to LPM wake up-only GIRQs values (SMB, PS2, KSCINT & eSPI wake)

Current code
void soc_deep_sleep_non_wake_en(void)
{
#ifdef CONFIG_ESPI_XEC
GIRQ22_REGS->SRC = 0xfffffffful;
GIRQ22_REGS->EN_SET = (1ul << 9); /* Better to have a macro */
#endif
}

Code to be added better having a HAL macro than self-defined in the module itself.
/* When MEC15xx drivers are power-aware this should be move there /
#define MCHP_PS20B_WAKE_ONLY_BIT BIT(19)
void soc_deep_sleep_wake_en(void)
{
#ifdef CONFIG_PS2_XEC_0
/
Enable PS2_0B_WK /
GIRQ21_REGS->SRC = MCHP_PS20B_WAKE_ONLY_BIT ;
GIRQ21_REGS->EN_SET = MCHP_PS20B_WAKE_ONLY_BIT ;
#endif
#ifdef CONFIG_PS2_XEC_1
/
Enable PS2_1B_WK */
GIRQ21_REGS->SRC = (1ul << 21);
GIRQ21_REGS->EN_SET = (1ul << 21);
#endif
}

@albertofloyd albertofloyd changed the title hal: microchip: Missing Wake bit defintions hal: microchip: Missing Wake bit definitions Apr 19, 2021
@galak galak added the priority: low Low impact/importance bug label Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants