[PATCH v4 0/3] x86: clean ups and feature enhancement in pmc_atom

[PATCH v4 0/3] x86: clean ups and feature enhancement in pmc_atom

[Andy Shevchenko]

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(-)

-- 
2.1.3

[PATCH v4 1/3] x86: pmc_atom: clean up init function

[Andy Shevchenko]

There is no need to use err variable.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Aubrey Li <aubrey.li@linux.intel.com>
---
 arch/x86/kernel/pmc_atom.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/pmc_atom.c b/arch/x86/kernel/pmc_atom.c
index 0ee5025e..19b8efa 100644
--- a/arch/x86/kernel/pmc_atom.c
+++ b/arch/x86/kernel/pmc_atom.c
@@ -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);
-- 
2.1.3

[PATCH v4 2/3] x86: pmc_atom: don't check for NULL twice

[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 <andriy.shevchenko@linux.intel.com>
Acked-by: Aubrey Li <aubrey.li@linux.intel.com>
---
 arch/x86/kernel/pmc_atom.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/x86/kernel/pmc_atom.c b/arch/x86/kernel/pmc_atom.c
index 19b8efa..bcc91ea 100644
--- a/arch/x86/kernel/pmc_atom.c
+++ b/arch/x86/kernel/pmc_atom.c
@@ -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)
-- 
2.1.3

[PATCH v4 3/3] x86: pmc_atom: expose contents of PSS

[Andy Shevchenko]

The 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.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Aubrey Li <aubrey.li@linux.intel.com>
---
 arch/x86/include/asm/pmc_atom.h | 22 +++++++++++++++
 arch/x86/kernel/pmc_atom.c      | 61 +++++++++++++++++++++++++++++++++++++++--
 2 files changed, 80 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/pmc_atom.h b/arch/x86/include/asm/pmc_atom.h
index fc7a17c..bc0fc08 100644
--- a/arch/x86/include/asm/pmc_atom.h
+++ b/arch/x86/include/asm/pmc_atom.h
@@ -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
 
diff --git a/arch/x86/kernel/pmc_atom.c b/arch/x86/kernel/pmc_atom.c
index bcc91ea..0eb469b 100644
--- a/arch/x86/kernel/pmc_atom.c
+++ b/arch/x86/kernel/pmc_atom.c
@@ -38,12 +38,12 @@ struct pmc_dev {
 static struct pmc_dev pmc_device;
 static u32 acpi_base_addr;
 
-struct pmc_dev_map {
+struct pmc_bit_map {
 	const char *name;
 	u32 bit_mask;
 };
 
-static const struct pmc_dev_map dev_map[] = {
+static const struct pmc_bit_map dev_map[] = {
 	{"0  - LPSS1_F0_DMA",		BIT_LPSS1_F0_DMA},
 	{"1  - LPSS1_F1_PWM1",		BIT_LPSS1_F1_PWM1},
 	{"2  - LPSS1_F2_PWM2",		BIT_LPSS1_F2_PWM2},
@@ -82,6 +82,27 @@ static const struct pmc_dev_map dev_map[] = {
 	{"35 - DFX",			BIT_DFX},
 };
 
+static const struct pmc_bit_map pss_map[] = {
+	{"0  - GBE",			PMC_PSS_BIT_GBE},
+	{"1  - SATA",			PMC_PSS_BIT_SATA},
+	{"2  - HDA",			PMC_PSS_BIT_HDA},
+	{"3  - SEC",			PMC_PSS_BIT_SEC},
+	{"4  - PCIE",			PMC_PSS_BIT_PCIE},
+	{"5  - LPSS",			PMC_PSS_BIT_LPSS},
+	{"6  - LPE",			PMC_PSS_BIT_LPE},
+	{"7  - DFX",			PMC_PSS_BIT_DFX},
+	{"8  - USH_CTRL",		PMC_PSS_BIT_USH_CTRL},
+	{"9  - USH_SUS",		PMC_PSS_BIT_USH_SUS},
+	{"10 - USH_VCCS",		PMC_PSS_BIT_USH_VCCS},
+	{"11 - USH_VCCA",		PMC_PSS_BIT_USH_VCCA},
+	{"12 - OTG_CTRL",		PMC_PSS_BIT_OTG_CTRL},
+	{"13 - OTG_VCCS",		PMC_PSS_BIT_OTG_VCCS},
+	{"14 - OTG_VCCA_CLK",		PMC_PSS_BIT_OTG_VCCA_CLK},
+	{"15 - OTG_VCCA",		PMC_PSS_BIT_OTG_VCCA},
+	{"16 - USB",			PMC_PSS_BIT_USB},
+	{"17 - USB_SUS",		PMC_PSS_BIT_USB_SUS},
+};
+
 static inline u32 pmc_reg_read(struct pmc_dev *pmc, int reg_offset)
 {
 	return readl(pmc->regmap + reg_offset);
@@ -169,6 +190,32 @@ static const struct file_operations pmc_dev_state_ops = {
 	.release	= single_release,
 };
 
+static int pmc_pss_state_show(struct seq_file *s, void *unused)
+{
+	struct pmc_dev *pmc = s->private;
+	u32 pss = pmc_reg_read(pmc, PMC_PSS);
+	int pss_index;
+
+	for (pss_index = 0; pss_index < ARRAY_SIZE(pss_map); pss_index++) {
+		seq_printf(s, "Island: %-32s\tState: %s\n",
+			pss_map[pss_index].name,
+			pss_map[pss_index].bit_mask & pss ? "Off" : "On");
+	}
+	return 0;
+}
+
+static int pmc_pss_state_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, pmc_pss_state_show, inode->i_private);
+}
+
+static const struct file_operations pmc_pss_state_ops = {
+	.open		= pmc_pss_state_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
 static int pmc_sleep_tmr_show(struct seq_file *s, void *unused)
 {
 	struct pmc_dev *pmc = s->private;
@@ -216,9 +263,17 @@ static int pmc_dbgfs_register(struct pmc_dev *pmc, struct pci_dev *pdev)
 	f = debugfs_create_file("dev_state", S_IFREG | S_IRUGO,
 				dir, pmc, &pmc_dev_state_ops);
 	if (!f) {
-		dev_err(&pdev->dev, "dev_states register failed\n");
+		dev_err(&pdev->dev, "dev_state register failed\n");
 		goto err;
 	}
+
+	f = debugfs_create_file("pss_state", S_IFREG | S_IRUGO,
+				dir, pmc, &pmc_pss_state_ops);
+	if (!f) {
+		dev_err(&pdev->dev, "pss_state register failed\n");
+		goto err;
+	}
+
 	f = debugfs_create_file("sleep_state", S_IFREG | S_IRUGO,
 				dir, pmc, &pmc_sleep_tmr_ops);
 	if (!f) {
-- 
2.1.3

Re: [PATCH v4 2/3] x86: pmc_atom: don't check for NULL twice

[Thomas Gleixner]

On Wed, 12 Nov 2014, Andy Shevchenko wrote:

> 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 <andriy.shevchenko@linux.intel.com>
> Acked-by: Aubrey Li <aubrey.li@linux.intel.com>
> ---
>  arch/x86/kernel/pmc_atom.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/pmc_atom.c b/arch/x86/kernel/pmc_atom.c
> index 19b8efa..bcc91ea 100644
> --- a/arch/x86/kernel/pmc_atom.c
> +++ b/arch/x86/kernel/pmc_atom.c
> @@ -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

[Andy Shevchenko]

On Wed, 2014-11-19 at 12:36 +0100, Thomas Gleixner wrote:
> On Wed, 12 Nov 2014, Andy Shevchenko wrote:
> 
> > 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.

> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Acked-by: Aubrey Li <aubrey.li@linux.intel.com>
> > ---
> >  arch/x86/kernel/pmc_atom.c | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/pmc_atom.c b/arch/x86/kernel/pmc_atom.c
> > index 19b8efa..bcc91ea 100644
> > --- a/arch/x86/kernel/pmc_atom.c
> > +++ b/arch/x86/kernel/pmc_atom.c
> > @@ -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....

Good catch, I add a new patch to address this.


> Aside of that, if DEBUGFS=n the code keeps pmc->regmap ioremapped for
> no reason.

pmc_hw_reg_setup() uses it. Aubrey, is it intended behaviour?

> 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 ....

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.

-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy