x86: clean ups and feature enhancement in pmc_atom
v4 cover x86: clean ups and feature enhancement in pmc_atom
Couple of clean ups and one feature enhancement (expose PSS register in the debug fs).
Changes since v3:
- rebase on top of recent linux-next
- resend to better place for review and push
Changes since v2:
- rebase on top of recent linux-next
- applied Acks
- introduce patch 3/3
Andy Shevchenko (3):
x86: pmc_atom: clean up init function
x86: pmc_atom: don't check for NULL twice
x86: pmc_atom: expose contents of PSS
arch/x86/include/asm/pmc_atom.h | 22 ++++++++++++ arch/x86/kernel/pmc_atom.c | 75 +++++++++++++++++++++++++++++++++-------- 2 files changed, 83 insertions(+), 14 deletions(-)
Signature · 1 line
2.1.3
PATCH v4 1/3 x86: pmc_atom: clean up init function
There is no need to use err variable.
--- arch/x86/kernel/pmc_atom.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
arch/x86/kernel/pmc_atom.c +3 -7
@@ -292,7 +292,6 @@ MODULE_DEVICE_TABLE(pci, pmc_pci_ids);static int __init pmc_atom_init (void ) { -int err = -ENODEV ;struct pci_dev *pdev =NULL ;const struct pci_device_id *ent;
@@ -306,14 +305,11 @@ static int __init pmc_atom_init(void)*/ for_each_pci_dev (pdev) { ent =pci_match_id (pmc_pci_ids, pdev); -if (ent) { - err =pmc_setup_dev (pdev); - goto out; - } +if (ent) +return pmc_setup_dev (pdev); }/* Device not found. */ -out: -return err; +return -ENODEV ; }module_init (pmc_atom_init);
Signature · 1 line
2.1.3
PATCH v4 2/3 x86: pmc_atom: don't check for NULL twice
In reply to Andy Shevchenkodebugfs_remove_recursive() is NULL-aware, thus, we may safely remove the check here. There is no need to assing NULL to variable since it will be not used anywhere.
--- arch/x86/kernel/pmc_atom.c | 4 ---- 1 file changed, 4 deletions(-)
arch/x86/kernel/pmc_atom.c +0 -4
@@ -202,11 +202,7 @@ static const struct file_operations pmc_sleep_tmr_ops = {static void pmc_dbgfs_unregister (struct pmc_dev *pmc) { -if (!pmc->dbgfs_dir) -return ; -debugfs_remove_recursive (pmc->dbgfs_dir); - pmc->dbgfs_dir =NULL ; }static int pmc_dbgfs_register (struct pmc_dev *pmc,struct pci_dev *pdev)
Signature · 1 line
2.1.3
Re: [PATCH v4 2/3] x86: pmc_atom: don't check for NULL twice
Quoted 15 lines · Andy Shevchenko
debugfs_remove_recursive()is NULL-aware, thus, we may safely remove the check here. There is no need to assing NULL to variable since it will be not used anywhere.Signed-off-by Andy Shevchenko <[email protected]>Acked-by Aubrey Li <[email protected]>--- arch/x86/kernel/pmc_atom.c | 4 ---- 1 file changed, 4 deletions(-)arch/x86/kernel/pmc_atom.c +0
-0
arch/x86/kernel/pmc_atom.c static const struct file_operations pmc_sleep_tmr_ops = { · line 206
Quoted 9 lines
arch/x86/kernel/pmc_atom.c +0
-4@@ -202,11 +202,7 @@ static const struct file_operations pmc_sleep_tmr_ops = {static void pmc_dbgfs_unregister (struct pmc_dev *pmc) { -if (!pmc->dbgfs_dir) -return ; -debugfs_remove_recursive (pmc->dbgfs_dir); - pmc->dbgfs_dir =NULL ; }
So while the patch is correct per se, the whole function is useless.
pmc_dbgfs_register()
dir = debugfs_create_dir("pmc_atom", NULL);
....
if (!f)
goto err;
pmc->dbgfs_dir = dir;
return 0;
err:
pmc_dbgfs_unregister(pmc);So pmc->dbgfs_dir is always NULL when this is called....
Aside of that, if DEBUGFS=n the code keeps pmc->regmap ioremapped for no reason.
Looking deeper:
The pmc_power_off function is installed _BEFORE_ pmc_hw_reg_setup() is called, which might not be called if the ioremap fails ....
Thanks,
tglx
Re: [PATCH v4 2/3] x86: pmc_atom: don't check for NULL twice
Quoted 5 lines · Thomas Gleixner
Quoted 3 lines · Andy Shevchenko
debugfs_remove_recursive()is NULL-aware, thus, we may safely remove the check here. There is no need to assing NULL to variable since it will be not used anywhere.
Thanks for review, my comments below.
Quoted 37 lines
Quoted 12 lines
Signed-off-by Andy Shevchenko <[email protected]>Acked-by Aubrey Li <[email protected]>--- arch/x86/kernel/pmc_atom.c | 4 ---- 1 file changed, 4 deletions(-)arch/x86/kernel/pmc_atom.c +0
-0arch/x86/kernel/pmc_atom.c static const struct file_operations pmc_sleep_tmr_ops = { · line 206
Quoted 9 lines
arch/x86/kernel/pmc_atom.c +0
-4@@ -202,11 +202,7 @@ static const struct file_operations pmc_sleep_tmr_ops = {static void pmc_dbgfs_unregister (struct pmc_dev *pmc) { -if (!pmc->dbgfs_dir) -return ; -debugfs_remove_recursive (pmc->dbgfs_dir); - pmc->dbgfs_dir =NULL ; }So while the patch is correct per se, the whole function is useless.
pmc_dbgfs_register()dir = debugfs_create_dir("pmc_atom", NULL); .... if (!f) goto err; pmc->dbgfs_dir = dir; return 0; err: pmc_dbgfs_unregister(pmc);
Quoted 1 line
So
pmc->dbgfs_diris always NULL when this is called....
Good catch, I add a new patch to address this.
Quoted 2 lines
Aside of that, if DEBUGFS=n the code keeps
pmc->regmapioremapped for no reason.
pmc_hw_reg_setup() uses it. Aubrey, is it intended behaviour?
Quoted 4 lines
Looking deeper:
The
pmc_power_offfunction is installed _BEFORE_pmc_hw_reg_setup()is called, which might not be called if the ioremap fails ....
I don't see any problem here (pmc_power_off uses acpi_base_addr which is independent to IO mapped space), though Aubrey may shed a light on this.
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
PATCH v4 3/3 x86: pmc_atom: expose contents of PSS
In reply to Andy ShevchenkoThe PSS register reflects the power state of each island on SoC. It would be useful to know which of the islands is on or off at the momemnt.
--- arch/x86/include/asm/pmc_atom.h | 22 +++++++++++++++ arch/x86/kernel/pmc_atom.c | 61 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 80 insertions(+), 3 deletions(-)
arch/x86/include/asm/pmc_atom.h +22 -0
@@ -53,6 +53,28 @@/* Sleep state counter is in units of of 32us */ #define PMC_TMR_SHIFT 5 +/* Power status of power islands */ +#define PMC_PSS 0x98 + +#define PMC_PSS_BIT_GBE BIT(0) +#define PMC_PSS_BIT_SATA BIT(1) +#define PMC_PSS_BIT_HDA BIT(2) +#define PMC_PSS_BIT_SEC BIT(3) +#define PMC_PSS_BIT_PCIE BIT(4) +#define PMC_PSS_BIT_LPSS BIT(5) +#define PMC_PSS_BIT_LPE BIT(6) +#define PMC_PSS_BIT_DFX BIT(7) +#define PMC_PSS_BIT_USH_CTRL BIT(8) +#define PMC_PSS_BIT_USH_SUS BIT(9) +#define PMC_PSS_BIT_USH_VCCS BIT(10) +#define PMC_PSS_BIT_USH_VCCA BIT(11) +#define PMC_PSS_BIT_OTG_CTRL BIT(12) +#define PMC_PSS_BIT_OTG_VCCS BIT(13) +#define PMC_PSS_BIT_OTG_VCCA_CLK BIT(14) +#define PMC_PSS_BIT_OTG_VCCA BIT(15) +#define PMC_PSS_BIT_USB BIT(16) +#define PMC_PSS_BIT_USB_SUS BIT(17) +/* These registers reflect D3 status of functions */ #define PMC_D3_STS_0 0xA0
arch/x86/kernel/pmc_atom.c +58 -3
91 lines not on this page · raw message
Signature · 1 line
2.1.3