[PATCH v2 0/4] x86: pmc_atom: Add Cherrytrail support

[PATCH v2 0/4] x86: pmc_atom: Add Cherrytrail support

[Andy Shevchenko]

This is the reworked patch series which had been sent earlier [1] to support
Intel CherryTrail SoC.

The patches were tested on both BayTrail and CherryTrail SoCs.

[1] https://patchwork.kernel.org/patch/5235891/

Changes to v1:
 - rebased on top of recent pmc_atom changes
 - cleaned up, reqorked and split to few smaller patches

Andy Shevchenko (4):
  x86: pmc_atom: save struct device pointer in pmc
  x86: pmc_atom: print index of device in loop
  x86: pmc_atom: supply register mappings via pmc object
  PMC driver: Add Cherrytrail PMC interface

 arch/x86/include/asm/pmc_atom.h |  25 ++++
 arch/x86/kernel/pmc_atom.c      | 270 +++++++++++++++++++++++++---------------
 2 files changed, 198 insertions(+), 97 deletions(-)

-- 
2.1.4

[PATCH v2 1/4] x86: pmc_atom: save struct device pointer in pmc

[Andy Shevchenko]

The change allows to use dev_printk() type of macros in the module functions.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/x86/kernel/pmc_atom.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/pmc_atom.c b/arch/x86/kernel/pmc_atom.c
index d66a4fe..d338222 100644
--- a/arch/x86/kernel/pmc_atom.c
+++ b/arch/x86/kernel/pmc_atom.c
@@ -26,6 +26,7 @@
 #include <asm/pmc_atom.h>
 
 struct pmc_dev {
+	struct device *dev;
 	u32 base_addr;
 	void __iomem *regmap;
 #ifdef CONFIG_DEBUG_FS
@@ -250,7 +251,7 @@ static void pmc_dbgfs_unregister(struct pmc_dev *pmc)
 	debugfs_remove_recursive(pmc->dbgfs_dir);
 }
 
-static int pmc_dbgfs_register(struct pmc_dev *pmc, struct pci_dev *pdev)
+static int pmc_dbgfs_register(struct pmc_dev *pmc)
 {
 	struct dentry *dir, *f;
 
@@ -263,21 +264,21 @@ 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_state register failed\n");
+		dev_err(pmc->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");
+		dev_err(pmc->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) {
-		dev_err(&pdev->dev, "sleep_state register failed\n");
+		dev_err(pmc->dev, "sleep_state register failed\n");
 		goto err;
 	}
 
@@ -287,7 +288,7 @@ err:
 	return -ENODEV;
 }
 #else
-static int pmc_dbgfs_register(struct pmc_dev *pmc, struct pci_dev *pdev)
+static int pmc_dbgfs_register(struct pmc_dev *pmc)
 {
 	return 0;
 }
@@ -315,10 +316,12 @@ static int pmc_setup_dev(struct pci_dev *pdev)
 		return -ENOMEM;
 	}
 
+	pmc->dev = &pdev->dev;
+
 	/* PMC hardware registers setup */
 	pmc_hw_reg_setup(pmc);
 
-	ret = pmc_dbgfs_register(pmc, pdev);
+	ret = pmc_dbgfs_register(pmc);
 	if (ret) {
 		iounmap(pmc->regmap);
 	}
-- 
2.1.4

[PATCH v2 2/4] x86: pmc_atom: print index of device in loop

[Andy Shevchenko]

The register mapping may change from one platform to another. Thus, indices
might be not the same on different platforms. The patch makes the code to print
the device index dynamically at run time.

The patch also changes the for loop to iterate over the map until a terminator
is found.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/x86/kernel/pmc_atom.c | 129 ++++++++++++++++++++++-----------------------
 1 file changed, 64 insertions(+), 65 deletions(-)

diff --git a/arch/x86/kernel/pmc_atom.c b/arch/x86/kernel/pmc_atom.c
index d338222..0bcfc9e 100644
--- a/arch/x86/kernel/pmc_atom.c
+++ b/arch/x86/kernel/pmc_atom.c
@@ -43,63 +43,65 @@ struct pmc_bit_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},
-	{"3  - LPSS1_F3_HSUART1",	BIT_LPSS1_F3_HSUART1},
-	{"4  - LPSS1_F4_HSUART2",	BIT_LPSS1_F4_HSUART2},
-	{"5  - LPSS1_F5_SPI",		BIT_LPSS1_F5_SPI},
-	{"6  - LPSS1_F6_Reserved",	BIT_LPSS1_F6_XXX},
-	{"7  - LPSS1_F7_Reserved",	BIT_LPSS1_F7_XXX},
-	{"8  - SCC_EMMC",		BIT_SCC_EMMC},
-	{"9  - SCC_SDIO",		BIT_SCC_SDIO},
-	{"10 - SCC_SDCARD",		BIT_SCC_SDCARD},
-	{"11 - SCC_MIPI",		BIT_SCC_MIPI},
-	{"12 - HDA",			BIT_HDA},
-	{"13 - LPE",			BIT_LPE},
-	{"14 - OTG",			BIT_OTG},
-	{"15 - USH",			BIT_USH},
-	{"16 - GBE",			BIT_GBE},
-	{"17 - SATA",			BIT_SATA},
-	{"18 - USB_EHCI",		BIT_USB_EHCI},
-	{"19 - SEC",			BIT_SEC},
-	{"20 - PCIE_PORT0",		BIT_PCIE_PORT0},
-	{"21 - PCIE_PORT1",		BIT_PCIE_PORT1},
-	{"22 - PCIE_PORT2",		BIT_PCIE_PORT2},
-	{"23 - PCIE_PORT3",		BIT_PCIE_PORT3},
-	{"24 - LPSS2_F0_DMA",		BIT_LPSS2_F0_DMA},
-	{"25 - LPSS2_F1_I2C1",		BIT_LPSS2_F1_I2C1},
-	{"26 - LPSS2_F2_I2C2",		BIT_LPSS2_F2_I2C2},
-	{"27 - LPSS2_F3_I2C3",		BIT_LPSS2_F3_I2C3},
-	{"28 - LPSS2_F3_I2C4",		BIT_LPSS2_F4_I2C4},
-	{"29 - LPSS2_F5_I2C5",		BIT_LPSS2_F5_I2C5},
-	{"30 - LPSS2_F6_I2C6",		BIT_LPSS2_F6_I2C6},
-	{"31 - LPSS2_F7_I2C7",		BIT_LPSS2_F7_I2C7},
-	{"32 - SMB",			BIT_SMB},
-	{"33 - OTG_SS_PHY",		BIT_OTG_SS_PHY},
-	{"34 - USH_SS_PHY",		BIT_USH_SS_PHY},
-	{"35 - DFX",			BIT_DFX},
+	{"LPSS1_F0_DMA",	BIT_LPSS1_F0_DMA},
+	{"LPSS1_F1_PWM1",	BIT_LPSS1_F1_PWM1},
+	{"LPSS1_F2_PWM2",	BIT_LPSS1_F2_PWM2},
+	{"LPSS1_F3_HSUART1",	BIT_LPSS1_F3_HSUART1},
+	{"LPSS1_F4_HSUART2",	BIT_LPSS1_F4_HSUART2},
+	{"LPSS1_F5_SPI",	BIT_LPSS1_F5_SPI},
+	{"LPSS1_F6_Reserved",	BIT_LPSS1_F6_XXX},
+	{"LPSS1_F7_Reserved",	BIT_LPSS1_F7_XXX},
+	{"SCC_EMMC",		BIT_SCC_EMMC},
+	{"SCC_SDIO",		BIT_SCC_SDIO},
+	{"SCC_SDCARD",		BIT_SCC_SDCARD},
+	{"SCC_MIPI",		BIT_SCC_MIPI},
+	{"HDA",			BIT_HDA},
+	{"LPE",			BIT_LPE},
+	{"OTG",			BIT_OTG},
+	{"USH",			BIT_USH},
+	{"GBE",			BIT_GBE},
+	{"SATA",		BIT_SATA},
+	{"USB_EHCI",		BIT_USB_EHCI},
+	{"SEC",			BIT_SEC},
+	{"PCIE_PORT0",		BIT_PCIE_PORT0},
+	{"PCIE_PORT1",		BIT_PCIE_PORT1},
+	{"PCIE_PORT2",		BIT_PCIE_PORT2},
+	{"PCIE_PORT3",		BIT_PCIE_PORT3},
+	{"LPSS2_F0_DMA",	BIT_LPSS2_F0_DMA},
+	{"LPSS2_F1_I2C1",	BIT_LPSS2_F1_I2C1},
+	{"LPSS2_F2_I2C2",	BIT_LPSS2_F2_I2C2},
+	{"LPSS2_F3_I2C3",	BIT_LPSS2_F3_I2C3},
+	{"LPSS2_F3_I2C4",	BIT_LPSS2_F4_I2C4},
+	{"LPSS2_F5_I2C5",	BIT_LPSS2_F5_I2C5},
+	{"LPSS2_F6_I2C6",	BIT_LPSS2_F6_I2C6},
+	{"LPSS2_F7_I2C7",	BIT_LPSS2_F7_I2C7},
+	{"SMB",			BIT_SMB},
+	{"OTG_SS_PHY",		BIT_OTG_SS_PHY},
+	{"USH_SS_PHY",		BIT_USH_SS_PHY},
+	{"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},
+	{"GBE",			PMC_PSS_BIT_GBE},
+	{"SATA",		PMC_PSS_BIT_SATA},
+	{"HDA",			PMC_PSS_BIT_HDA},
+	{"SEC",			PMC_PSS_BIT_SEC},
+	{"PCIE",		PMC_PSS_BIT_PCIE},
+	{"LPSS",		PMC_PSS_BIT_LPSS},
+	{"LPE",			PMC_PSS_BIT_LPE},
+	{"DFX",			PMC_PSS_BIT_DFX},
+	{"USH_CTRL",		PMC_PSS_BIT_USH_CTRL},
+	{"USH_SUS",		PMC_PSS_BIT_USH_SUS},
+	{"USH_VCCS",		PMC_PSS_BIT_USH_VCCS},
+	{"USH_VCCA",		PMC_PSS_BIT_USH_VCCA},
+	{"OTG_CTRL",		PMC_PSS_BIT_OTG_CTRL},
+	{"OTG_VCCS",		PMC_PSS_BIT_OTG_VCCS},
+	{"OTG_VCCA_CLK",	PMC_PSS_BIT_OTG_VCCA_CLK},
+	{"OTG_VCCA",		PMC_PSS_BIT_OTG_VCCA},
+	{"USB",			PMC_PSS_BIT_USB},
+	{"USB_SUS",		PMC_PSS_BIT_USB_SUS},
+	{},
 };
 
 static inline u32 pmc_reg_read(struct pmc_dev *pmc, int reg_offset)
@@ -148,16 +150,14 @@ static int pmc_dev_state_show(struct seq_file *s, void *unused)
 	struct pmc_dev *pmc = s->private;
 	u32 func_dis, func_dis_2, func_dis_index;
 	u32 d3_sts_0, d3_sts_1, d3_sts_index;
-	int dev_num, dev_index, reg_index;
+	int dev_index, reg_index;
 
 	func_dis = pmc_reg_read(pmc, PMC_FUNC_DIS);
 	func_dis_2 = pmc_reg_read(pmc, PMC_FUNC_DIS_2);
 	d3_sts_0 = pmc_reg_read(pmc, PMC_D3_STS_0);
 	d3_sts_1 = pmc_reg_read(pmc, PMC_D3_STS_1);
 
-	dev_num = ARRAY_SIZE(dev_map);
-
-	for (dev_index = 0; dev_index < dev_num; dev_index++) {
+	for (dev_index = 0; dev_map[dev_index].name; dev_index++) {
 		reg_index = dev_index / PMC_REG_BIT_WIDTH;
 		if (reg_index) {
 			func_dis_index = func_dis_2;
@@ -167,8 +167,8 @@ static int pmc_dev_state_show(struct seq_file *s, void *unused)
 			d3_sts_index = d3_sts_0;
 		}
 
-		seq_printf(s, "Dev: %-32s\tState: %s [%s]\n",
-			dev_map[dev_index].name,
+		seq_printf(s, "Dev: %-2d - %-32s\tState: %s [%s]\n",
+			dev_index, dev_map[dev_index].name,
 			dev_map[dev_index].bit_mask & func_dis_index ?
 			"Disabled" : "Enabled ",
 			dev_map[dev_index].bit_mask & d3_sts_index ?
@@ -195,9 +195,9 @@ static int pmc_pss_state_show(struct seq_file *s, void *unused)
 	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,
+	for (pss_index = 0; pss_map[pss_index].name; pss_index++) {
+		seq_printf(s, "Island: %-2d - %-32s\tState: %s\n",
+			pss_index, pss_map[pss_index].name,
 			pss_map[pss_index].bit_mask & pss ? "Off" : "On");
 	}
 	return 0;
@@ -322,9 +322,8 @@ static int pmc_setup_dev(struct pci_dev *pdev)
 	pmc_hw_reg_setup(pmc);
 
 	ret = pmc_dbgfs_register(pmc);
-	if (ret) {
+	if (ret)
 		iounmap(pmc->regmap);
-	}
 
 	return ret;
 }
-- 
2.1.4

[PATCH v2 3/4] x86: pmc_atom: supply register mappings via pmc object

[Andy Shevchenko]

The patch converts the functions to use the register mappings provided by pmc
object. It would help in case of mappings on different platforms.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/x86/kernel/pmc_atom.c | 48 ++++++++++++++++++++++++++++++----------------
 1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kernel/pmc_atom.c b/arch/x86/kernel/pmc_atom.c
index 0bcfc9e..0f24ef7 100644
--- a/arch/x86/kernel/pmc_atom.c
+++ b/arch/x86/kernel/pmc_atom.c
@@ -25,10 +25,21 @@
 
 #include <asm/pmc_atom.h>
 
+struct pmc_bit_map {
+	const char *name;
+	u32 bit_mask;
+};
+
+struct pmc_reg_map {
+	const struct pmc_bit_map *dev;
+	const struct pmc_bit_map *pss;
+};
+
 struct pmc_dev {
 	struct device *dev;
 	u32 base_addr;
 	void __iomem *regmap;
+	const struct pmc_reg_map *map;
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *dbgfs_dir;
 #endif /* CONFIG_DEBUG_FS */
@@ -37,11 +48,6 @@ struct pmc_dev {
 static struct pmc_dev pmc_device;
 static u32 acpi_base_addr;
 
-struct pmc_bit_map {
-	const char *name;
-	u32 bit_mask;
-};
-
 static const struct pmc_bit_map dev_map[] = {
 	{"LPSS1_F0_DMA",	BIT_LPSS1_F0_DMA},
 	{"LPSS1_F1_PWM1",	BIT_LPSS1_F1_PWM1},
@@ -104,6 +110,11 @@ static const struct pmc_bit_map pss_map[] = {
 	{},
 };
 
+static const struct pmc_reg_map reg_map = {
+	.dev		= dev_map,
+	.pss		= pss_map,
+};
+
 static inline u32 pmc_reg_read(struct pmc_dev *pmc, int reg_offset)
 {
 	return readl(pmc->regmap + reg_offset);
@@ -148,17 +159,18 @@ static void pmc_hw_reg_setup(struct pmc_dev *pmc)
 static int pmc_dev_state_show(struct seq_file *s, void *unused)
 {
 	struct pmc_dev *pmc = s->private;
+	const struct pmc_bit_map *map = pmc->map->dev;
 	u32 func_dis, func_dis_2, func_dis_index;
 	u32 d3_sts_0, d3_sts_1, d3_sts_index;
-	int dev_index, reg_index;
+	int index, reg_index;
 
 	func_dis = pmc_reg_read(pmc, PMC_FUNC_DIS);
 	func_dis_2 = pmc_reg_read(pmc, PMC_FUNC_DIS_2);
 	d3_sts_0 = pmc_reg_read(pmc, PMC_D3_STS_0);
 	d3_sts_1 = pmc_reg_read(pmc, PMC_D3_STS_1);
 
-	for (dev_index = 0; dev_map[dev_index].name; dev_index++) {
-		reg_index = dev_index / PMC_REG_BIT_WIDTH;
+	for (index = 0; map[index].name; index++) {
+		reg_index = index / PMC_REG_BIT_WIDTH;
 		if (reg_index) {
 			func_dis_index = func_dis_2;
 			d3_sts_index = d3_sts_1;
@@ -168,10 +180,10 @@ static int pmc_dev_state_show(struct seq_file *s, void *unused)
 		}
 
 		seq_printf(s, "Dev: %-2d - %-32s\tState: %s [%s]\n",
-			dev_index, dev_map[dev_index].name,
-			dev_map[dev_index].bit_mask & func_dis_index ?
+			index, map[index].name,
+			map[index].bit_mask & func_dis_index ?
 			"Disabled" : "Enabled ",
-			dev_map[dev_index].bit_mask & d3_sts_index ?
+			map[index].bit_mask & d3_sts_index ?
 			"D3" : "D0");
 	}
 	return 0;
@@ -192,13 +204,14 @@ static const struct file_operations pmc_dev_state_ops = {
 static int pmc_pss_state_show(struct seq_file *s, void *unused)
 {
 	struct pmc_dev *pmc = s->private;
+	const struct pmc_bit_map *map = pmc->map->pss;
 	u32 pss = pmc_reg_read(pmc, PMC_PSS);
-	int pss_index;
+	int index;
 
-	for (pss_index = 0; pss_map[pss_index].name; pss_index++) {
+	for (index = 0; map[index].name; index++) {
 		seq_printf(s, "Island: %-2d - %-32s\tState: %s\n",
-			pss_index, pss_map[pss_index].name,
-			pss_map[pss_index].bit_mask & pss ? "Off" : "On");
+			index, map[index].name,
+			map[index].bit_mask & pss ? "Off" : "On");
 	}
 	return 0;
 }
@@ -294,7 +307,7 @@ static int pmc_dbgfs_register(struct pmc_dev *pmc)
 }
 #endif /* CONFIG_DEBUG_FS */
 
-static int pmc_setup_dev(struct pci_dev *pdev)
+static int pmc_setup_dev(struct pci_dev *pdev, const struct pmc_reg_map *map)
 {
 	struct pmc_dev *pmc = &pmc_device;
 	int ret;
@@ -317,6 +330,7 @@ static int pmc_setup_dev(struct pci_dev *pdev)
 	}
 
 	pmc->dev = &pdev->dev;
+	pmc->map = map;
 
 	/* PMC hardware registers setup */
 	pmc_hw_reg_setup(pmc);
@@ -359,7 +373,7 @@ static int __init pmc_atom_init(void)
 	for_each_pci_dev(pdev) {
 		ent = pci_match_id(pmc_pci_ids, pdev);
 		if (ent)
-			return pmc_setup_dev(pdev);
+			return pmc_setup_dev(pdev, &reg_map);
 	}
 	/* Device not found. */
 	return -ENODEV;
-- 
2.1.4

[PATCH v2 4/4] PMC driver: Add Cherrytrail PMC interface

[Andy Shevchenko]

The patch adds CHT PMC interface. This exposes all the South IP device power
states and S0ix states for CHT. The bit map of FUNC_DIS and D3_STS_0 registers
for SoCs are consistent. The D3_STS_1 and FUNC_DIS_2 registers, however, are
not aligned. This is fixed by splitting a common mapping on per register basis.

Signed-off-by: Kumar P Mahesh <mahesh.kumar.p@intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/x86/include/asm/pmc_atom.h |  25 +++++++++
 arch/x86/kernel/pmc_atom.c      | 118 ++++++++++++++++++++++++++++++----------
 2 files changed, 114 insertions(+), 29 deletions(-)

diff --git a/arch/x86/include/asm/pmc_atom.h b/arch/x86/include/asm/pmc_atom.h
index bc0fc08..000a223 100644
--- a/arch/x86/include/asm/pmc_atom.h
+++ b/arch/x86/include/asm/pmc_atom.h
@@ -18,6 +18,8 @@
 
 /* ValleyView Power Control Unit PCI Device ID */
 #define	PCI_DEVICE_ID_VLV_PMC	0x0F1C
+/* CherryTrail Power Control Unit PCI Device ID */
+#define	PCI_DEVICE_ID_CHT_PMC	0x229C
 
 /* PMC Memory mapped IO registers */
 #define	PMC_BASE_ADDR_OFFSET	0x44
@@ -29,6 +31,10 @@
 #define	PMC_FUNC_DIS		0x34
 #define	PMC_FUNC_DIS_2		0x38
 
+/* CHT specific bits in FUNC_DIS2 register */
+#define	BIT_FD_GMM		BIT(3)
+#define	BIT_FD_ISH		BIT(4)
+
 /* S0ix wake event control */
 #define	PMC_S0IX_WAKE_EN	0x3C
 
@@ -75,6 +81,21 @@
 #define PMC_PSS_BIT_USB			BIT(16)
 #define PMC_PSS_BIT_USB_SUS		BIT(17)
 
+/* CHT specific bits in PSS register */
+#define	PMC_PSS_BIT_CHT_UFS		BIT(7)
+#define	PMC_PSS_BIT_CHT_UXD		BIT(11)
+#define	PMC_PSS_BIT_CHT_UXD_FD		BIT(12)
+#define	PMC_PSS_BIT_CHT_UX_ENG		BIT(15)
+#define	PMC_PSS_BIT_CHT_USB_SUS		BIT(16)
+#define	PMC_PSS_BIT_CHT_GMM		BIT(17)
+#define	PMC_PSS_BIT_CHT_ISH		BIT(18)
+#define	PMC_PSS_BIT_CHT_DFX_MASTER	BIT(26)
+#define	PMC_PSS_BIT_CHT_DFX_CLUSTER1	BIT(27)
+#define	PMC_PSS_BIT_CHT_DFX_CLUSTER2	BIT(28)
+#define	PMC_PSS_BIT_CHT_DFX_CLUSTER3	BIT(29)
+#define	PMC_PSS_BIT_CHT_DFX_CLUSTER4	BIT(30)
+#define	PMC_PSS_BIT_CHT_DFX_CLUSTER5	BIT(31)
+
 /* These registers reflect D3 status of functions */
 #define	PMC_D3_STS_0		0xA0
 
@@ -117,6 +138,10 @@
 #define	BIT_USH_SS_PHY		BIT(2)
 #define	BIT_DFX			BIT(3)
 
+/* CHT specific bits in PMC_D3_STS_1 register */
+#define	BIT_STS_GMM		BIT(1)
+#define	BIT_STS_ISH		BIT(2)
+
 /* PMC I/O Registers */
 #define	ACPI_BASE_ADDR_OFFSET	0x40
 #define	ACPI_BASE_ADDR_MASK	0xFFFFFE00
diff --git a/arch/x86/kernel/pmc_atom.c b/arch/x86/kernel/pmc_atom.c
index 0f24ef7..41f4a33 100644
--- a/arch/x86/kernel/pmc_atom.c
+++ b/arch/x86/kernel/pmc_atom.c
@@ -31,7 +31,10 @@ struct pmc_bit_map {
 };
 
 struct pmc_reg_map {
-	const struct pmc_bit_map *dev;
+	const struct pmc_bit_map *d3_sts_0;
+	const struct pmc_bit_map *d3_sts_1;
+	const struct pmc_bit_map *func_dis;
+	const struct pmc_bit_map *func_dis_2;
 	const struct pmc_bit_map *pss;
 };
 
@@ -48,7 +51,7 @@ struct pmc_dev {
 static struct pmc_dev pmc_device;
 static u32 acpi_base_addr;
 
-static const struct pmc_bit_map dev_map[] = {
+static const struct pmc_bit_map d3_sts_0_map[] = {
 	{"LPSS1_F0_DMA",	BIT_LPSS1_F0_DMA},
 	{"LPSS1_F1_PWM1",	BIT_LPSS1_F1_PWM1},
 	{"LPSS1_F2_PWM2",	BIT_LPSS1_F2_PWM2},
@@ -81,6 +84,10 @@ static const struct pmc_bit_map dev_map[] = {
 	{"LPSS2_F5_I2C5",	BIT_LPSS2_F5_I2C5},
 	{"LPSS2_F6_I2C6",	BIT_LPSS2_F6_I2C6},
 	{"LPSS2_F7_I2C7",	BIT_LPSS2_F7_I2C7},
+	{},
+};
+
+static struct pmc_bit_map byt_d3_sts_1_map[] = {
 	{"SMB",			BIT_SMB},
 	{"OTG_SS_PHY",		BIT_OTG_SS_PHY},
 	{"USH_SS_PHY",		BIT_USH_SS_PHY},
@@ -88,7 +95,21 @@ static const struct pmc_bit_map dev_map[] = {
 	{},
 };
 
-static const struct pmc_bit_map pss_map[] = {
+static struct pmc_bit_map cht_d3_sts_1_map[] = {
+	{"SMB",			BIT_SMB},
+	{"GMM",			BIT_STS_GMM},
+	{"ISH",			BIT_STS_ISH},
+	{},
+};
+
+static struct pmc_bit_map cht_func_dis_2_map[] = {
+	{"SMB",			BIT_SMB},
+	{"GMM",			BIT_FD_GMM},
+	{"ISH",			BIT_FD_ISH},
+	{},
+};
+
+static const struct pmc_bit_map byt_pss_map[] = {
 	{"GBE",			PMC_PSS_BIT_GBE},
 	{"SATA",		PMC_PSS_BIT_SATA},
 	{"HDA",			PMC_PSS_BIT_HDA},
@@ -110,9 +131,43 @@ static const struct pmc_bit_map pss_map[] = {
 	{},
 };
 
-static const struct pmc_reg_map reg_map = {
-	.dev		= dev_map,
-	.pss		= pss_map,
+static const struct pmc_bit_map cht_pss_map[] = {
+	{"SATA",		PMC_PSS_BIT_SATA},
+	{"HDA",			PMC_PSS_BIT_HDA},
+	{"SEC",			PMC_PSS_BIT_SEC},
+	{"PCIE",		PMC_PSS_BIT_PCIE},
+	{"LPSS",		PMC_PSS_BIT_LPSS},
+	{"LPE",			PMC_PSS_BIT_LPE},
+	{"UFS",			PMC_PSS_BIT_CHT_UFS},
+	{"UXD",			PMC_PSS_BIT_CHT_UXD},
+	{"UXD_FD",		PMC_PSS_BIT_CHT_UXD_FD},
+	{"UX_ENG",		PMC_PSS_BIT_CHT_UX_ENG},
+	{"USB_SUS",		PMC_PSS_BIT_CHT_USB_SUS},
+	{"GMM",			PMC_PSS_BIT_CHT_GMM},
+	{"ISH",			PMC_PSS_BIT_CHT_ISH},
+	{"DFX_MASTER",		PMC_PSS_BIT_CHT_DFX_MASTER},
+	{"DFX_CLUSTER1",	PMC_PSS_BIT_CHT_DFX_CLUSTER1},
+	{"DFX_CLUSTER2",	PMC_PSS_BIT_CHT_DFX_CLUSTER2},
+	{"DFX_CLUSTER3",	PMC_PSS_BIT_CHT_DFX_CLUSTER3},
+	{"DFX_CLUSTER4",	PMC_PSS_BIT_CHT_DFX_CLUSTER4},
+	{"DFX_CLUSTER5",	PMC_PSS_BIT_CHT_DFX_CLUSTER5},
+	{},
+};
+
+static const struct pmc_reg_map byt_reg_map = {
+	.d3_sts_0	= d3_sts_0_map,
+	.d3_sts_1	= byt_d3_sts_1_map,
+	.func_dis	= d3_sts_0_map,
+	.func_dis_2	= byt_d3_sts_1_map,
+	.pss		= byt_pss_map,
+};
+
+static const struct pmc_reg_map cht_reg_map = {
+	.d3_sts_0	= d3_sts_0_map,
+	.d3_sts_1	= cht_d3_sts_1_map,
+	.func_dis	= d3_sts_0_map,
+	.func_dis_2	= cht_func_dis_2_map,
+	.pss		= cht_pss_map,
 };
 
 static inline u32 pmc_reg_read(struct pmc_dev *pmc, int reg_offset)
@@ -156,36 +211,39 @@ static void pmc_hw_reg_setup(struct pmc_dev *pmc)
 }
 
 #ifdef CONFIG_DEBUG_FS
+static void pmc_dev_state_print(struct seq_file *s, int reg_index,
+				u32 sts, const struct pmc_bit_map *sts_map,
+				u32 fd, const struct pmc_bit_map *fd_map)
+{
+	int offset = PMC_REG_BIT_WIDTH * reg_index;
+	int index;
+
+	for (index = 0; sts_map[index].name; index++) {
+		seq_printf(s, "Dev: %-2d - %-32s\tState: %s [%s]\n",
+			offset + index, sts_map[index].name,
+			fd_map[index].bit_mask & fd ?  "Disabled" : "Enabled ",
+			sts_map[index].bit_mask & sts ?  "D3" : "D0");
+	}
+}
+
 static int pmc_dev_state_show(struct seq_file *s, void *unused)
 {
 	struct pmc_dev *pmc = s->private;
-	const struct pmc_bit_map *map = pmc->map->dev;
-	u32 func_dis, func_dis_2, func_dis_index;
-	u32 d3_sts_0, d3_sts_1, d3_sts_index;
-	int index, reg_index;
+	const struct pmc_reg_map *m = pmc->map;
+	u32 func_dis, func_dis_2;
+	u32 d3_sts_0, d3_sts_1;
 
 	func_dis = pmc_reg_read(pmc, PMC_FUNC_DIS);
 	func_dis_2 = pmc_reg_read(pmc, PMC_FUNC_DIS_2);
 	d3_sts_0 = pmc_reg_read(pmc, PMC_D3_STS_0);
 	d3_sts_1 = pmc_reg_read(pmc, PMC_D3_STS_1);
 
-	for (index = 0; map[index].name; index++) {
-		reg_index = index / PMC_REG_BIT_WIDTH;
-		if (reg_index) {
-			func_dis_index = func_dis_2;
-			d3_sts_index = d3_sts_1;
-		} else {
-			func_dis_index = func_dis;
-			d3_sts_index = d3_sts_0;
-		}
+	/* Low part */
+	pmc_dev_state_print(s, 0, d3_sts_0, m->d3_sts_0, func_dis, m->func_dis);
+
+	/* High part */
+	pmc_dev_state_print(s, 1, d3_sts_1, m->d3_sts_1, func_dis_2, m->func_dis_2);
 
-		seq_printf(s, "Dev: %-2d - %-32s\tState: %s [%s]\n",
-			index, map[index].name,
-			map[index].bit_mask & func_dis_index ?
-			"Disabled" : "Enabled ",
-			map[index].bit_mask & d3_sts_index ?
-			"D3" : "D0");
-	}
 	return 0;
 }
 
@@ -307,9 +365,10 @@ static int pmc_dbgfs_register(struct pmc_dev *pmc)
 }
 #endif /* CONFIG_DEBUG_FS */
 
-static int pmc_setup_dev(struct pci_dev *pdev, const struct pmc_reg_map *map)
+static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	struct pmc_dev *pmc = &pmc_device;
+	const struct pmc_reg_map *map = (struct pmc_reg_map *)ent->driver_data;
 	int ret;
 
 	/* Obtain ACPI base address */
@@ -351,7 +410,8 @@ static int pmc_setup_dev(struct pci_dev *pdev, const struct pmc_reg_map *map)
  * a driver on the same PCI id.
  */
 static const struct pci_device_id pmc_pci_ids[] = {
-	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_VLV_PMC) },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_VLV_PMC), (kernel_ulong_t)&byt_reg_map },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_CHT_PMC), (kernel_ulong_t)&cht_reg_map },
 	{ 0, },
 };
 
@@ -373,7 +433,7 @@ static int __init pmc_atom_init(void)
 	for_each_pci_dev(pdev) {
 		ent = pci_match_id(pmc_pci_ids, pdev);
 		if (ent)
-			return pmc_setup_dev(pdev, &reg_map);
+			return pmc_setup_dev(pdev, ent);
 	}
 	/* Device not found. */
 	return -ENODEV;
-- 
2.1.4

Re: [PATCH v2 1/4] x86: pmc_atom: save struct device pointer in pmc

[Li, Aubrey]

On 2015/1/21 5:50, Andy Shevchenko wrote:
> The change allows to use dev_printk() type of macros in the module functions.

The patch itself looks good to me, but not sure why do we need this
change, will we use dev_prink in the subsequent patches?

Thanks,
-Aubrey
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  arch/x86/kernel/pmc_atom.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/pmc_atom.c b/arch/x86/kernel/pmc_atom.c
> index d66a4fe..d338222 100644
> --- a/arch/x86/kernel/pmc_atom.c
> +++ b/arch/x86/kernel/pmc_atom.c
> @@ -26,6 +26,7 @@
>  #include <asm/pmc_atom.h>
>  
>  struct pmc_dev {
> +	struct device *dev;
>  	u32 base_addr;
>  	void __iomem *regmap;
>  #ifdef CONFIG_DEBUG_FS
> @@ -250,7 +251,7 @@ static void pmc_dbgfs_unregister(struct pmc_dev *pmc)
>  	debugfs_remove_recursive(pmc->dbgfs_dir);
>  }
>  
> -static int pmc_dbgfs_register(struct pmc_dev *pmc, struct pci_dev *pdev)
> +static int pmc_dbgfs_register(struct pmc_dev *pmc)
>  {
>  	struct dentry *dir, *f;
>  
> @@ -263,21 +264,21 @@ 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_state register failed\n");
> +		dev_err(pmc->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");
> +		dev_err(pmc->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) {
> -		dev_err(&pdev->dev, "sleep_state register failed\n");
> +		dev_err(pmc->dev, "sleep_state register failed\n");
>  		goto err;
>  	}
>  
> @@ -287,7 +288,7 @@ err:
>  	return -ENODEV;
>  }
>  #else
> -static int pmc_dbgfs_register(struct pmc_dev *pmc, struct pci_dev *pdev)
> +static int pmc_dbgfs_register(struct pmc_dev *pmc)
>  {
>  	return 0;
>  }
> @@ -315,10 +316,12 @@ static int pmc_setup_dev(struct pci_dev *pdev)
>  		return -ENOMEM;
>  	}
>  
> +	pmc->dev = &pdev->dev;
> +
>  	/* PMC hardware registers setup */
>  	pmc_hw_reg_setup(pmc);
>  
> -	ret = pmc_dbgfs_register(pmc, pdev);
> +	ret = pmc_dbgfs_register(pmc);
>  	if (ret) {
>  		iounmap(pmc->regmap);
>  	}
> 

Re: [PATCH v2 2/4] x86: pmc_atom: print index of device in loop

[Li, Aubrey]

On 2015/1/21 5:50, Andy Shevchenko wrote:
> The register mapping may change from one platform to another. Thus, indices
> might be not the same on different platforms. The patch makes the code to print
> the device index dynamically at run time.

Will another platform use the same table but different bit position? In
my opinion, different platform should use different mapping table.

> 
> The patch also changes the for loop to iterate over the map until a terminator
> is found.

Why do we need to do this? did you see any hurt from the existing
implementation?

Thanks,
-Aubrey
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  arch/x86/kernel/pmc_atom.c | 129 ++++++++++++++++++++++-----------------------
>  1 file changed, 64 insertions(+), 65 deletions(-)
> 
> diff --git a/arch/x86/kernel/pmc_atom.c b/arch/x86/kernel/pmc_atom.c
> index d338222..0bcfc9e 100644
> --- a/arch/x86/kernel/pmc_atom.c
> +++ b/arch/x86/kernel/pmc_atom.c
> @@ -43,63 +43,65 @@ struct pmc_bit_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},
> -	{"3  - LPSS1_F3_HSUART1",	BIT_LPSS1_F3_HSUART1},
> -	{"4  - LPSS1_F4_HSUART2",	BIT_LPSS1_F4_HSUART2},
> -	{"5  - LPSS1_F5_SPI",		BIT_LPSS1_F5_SPI},
> -	{"6  - LPSS1_F6_Reserved",	BIT_LPSS1_F6_XXX},
> -	{"7  - LPSS1_F7_Reserved",	BIT_LPSS1_F7_XXX},
> -	{"8  - SCC_EMMC",		BIT_SCC_EMMC},
> -	{"9  - SCC_SDIO",		BIT_SCC_SDIO},
> -	{"10 - SCC_SDCARD",		BIT_SCC_SDCARD},
> -	{"11 - SCC_MIPI",		BIT_SCC_MIPI},
> -	{"12 - HDA",			BIT_HDA},
> -	{"13 - LPE",			BIT_LPE},
> -	{"14 - OTG",			BIT_OTG},
> -	{"15 - USH",			BIT_USH},
> -	{"16 - GBE",			BIT_GBE},
> -	{"17 - SATA",			BIT_SATA},
> -	{"18 - USB_EHCI",		BIT_USB_EHCI},
> -	{"19 - SEC",			BIT_SEC},
> -	{"20 - PCIE_PORT0",		BIT_PCIE_PORT0},
> -	{"21 - PCIE_PORT1",		BIT_PCIE_PORT1},
> -	{"22 - PCIE_PORT2",		BIT_PCIE_PORT2},
> -	{"23 - PCIE_PORT3",		BIT_PCIE_PORT3},
> -	{"24 - LPSS2_F0_DMA",		BIT_LPSS2_F0_DMA},
> -	{"25 - LPSS2_F1_I2C1",		BIT_LPSS2_F1_I2C1},
> -	{"26 - LPSS2_F2_I2C2",		BIT_LPSS2_F2_I2C2},
> -	{"27 - LPSS2_F3_I2C3",		BIT_LPSS2_F3_I2C3},
> -	{"28 - LPSS2_F3_I2C4",		BIT_LPSS2_F4_I2C4},
> -	{"29 - LPSS2_F5_I2C5",		BIT_LPSS2_F5_I2C5},
> -	{"30 - LPSS2_F6_I2C6",		BIT_LPSS2_F6_I2C6},
> -	{"31 - LPSS2_F7_I2C7",		BIT_LPSS2_F7_I2C7},
> -	{"32 - SMB",			BIT_SMB},
> -	{"33 - OTG_SS_PHY",		BIT_OTG_SS_PHY},
> -	{"34 - USH_SS_PHY",		BIT_USH_SS_PHY},
> -	{"35 - DFX",			BIT_DFX},
> +	{"LPSS1_F0_DMA",	BIT_LPSS1_F0_DMA},
> +	{"LPSS1_F1_PWM1",	BIT_LPSS1_F1_PWM1},
> +	{"LPSS1_F2_PWM2",	BIT_LPSS1_F2_PWM2},
> +	{"LPSS1_F3_HSUART1",	BIT_LPSS1_F3_HSUART1},
> +	{"LPSS1_F4_HSUART2",	BIT_LPSS1_F4_HSUART2},
> +	{"LPSS1_F5_SPI",	BIT_LPSS1_F5_SPI},
> +	{"LPSS1_F6_Reserved",	BIT_LPSS1_F6_XXX},
> +	{"LPSS1_F7_Reserved",	BIT_LPSS1_F7_XXX},
> +	{"SCC_EMMC",		BIT_SCC_EMMC},
> +	{"SCC_SDIO",		BIT_SCC_SDIO},
> +	{"SCC_SDCARD",		BIT_SCC_SDCARD},
> +	{"SCC_MIPI",		BIT_SCC_MIPI},
> +	{"HDA",			BIT_HDA},
> +	{"LPE",			BIT_LPE},
> +	{"OTG",			BIT_OTG},
> +	{"USH",			BIT_USH},
> +	{"GBE",			BIT_GBE},
> +	{"SATA",		BIT_SATA},
> +	{"USB_EHCI",		BIT_USB_EHCI},
> +	{"SEC",			BIT_SEC},
> +	{"PCIE_PORT0",		BIT_PCIE_PORT0},
> +	{"PCIE_PORT1",		BIT_PCIE_PORT1},
> +	{"PCIE_PORT2",		BIT_PCIE_PORT2},
> +	{"PCIE_PORT3",		BIT_PCIE_PORT3},
> +	{"LPSS2_F0_DMA",	BIT_LPSS2_F0_DMA},
> +	{"LPSS2_F1_I2C1",	BIT_LPSS2_F1_I2C1},
> +	{"LPSS2_F2_I2C2",	BIT_LPSS2_F2_I2C2},
> +	{"LPSS2_F3_I2C3",	BIT_LPSS2_F3_I2C3},
> +	{"LPSS2_F3_I2C4",	BIT_LPSS2_F4_I2C4},
> +	{"LPSS2_F5_I2C5",	BIT_LPSS2_F5_I2C5},
> +	{"LPSS2_F6_I2C6",	BIT_LPSS2_F6_I2C6},
> +	{"LPSS2_F7_I2C7",	BIT_LPSS2_F7_I2C7},
> +	{"SMB",			BIT_SMB},
> +	{"OTG_SS_PHY",		BIT_OTG_SS_PHY},
> +	{"USH_SS_PHY",		BIT_USH_SS_PHY},
> +	{"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},
> +	{"GBE",			PMC_PSS_BIT_GBE},
> +	{"SATA",		PMC_PSS_BIT_SATA},
> +	{"HDA",			PMC_PSS_BIT_HDA},
> +	{"SEC",			PMC_PSS_BIT_SEC},
> +	{"PCIE",		PMC_PSS_BIT_PCIE},
> +	{"LPSS",		PMC_PSS_BIT_LPSS},
> +	{"LPE",			PMC_PSS_BIT_LPE},
> +	{"DFX",			PMC_PSS_BIT_DFX},
> +	{"USH_CTRL",		PMC_PSS_BIT_USH_CTRL},
> +	{"USH_SUS",		PMC_PSS_BIT_USH_SUS},
> +	{"USH_VCCS",		PMC_PSS_BIT_USH_VCCS},
> +	{"USH_VCCA",		PMC_PSS_BIT_USH_VCCA},
> +	{"OTG_CTRL",		PMC_PSS_BIT_OTG_CTRL},
> +	{"OTG_VCCS",		PMC_PSS_BIT_OTG_VCCS},
> +	{"OTG_VCCA_CLK",	PMC_PSS_BIT_OTG_VCCA_CLK},
> +	{"OTG_VCCA",		PMC_PSS_BIT_OTG_VCCA},
> +	{"USB",			PMC_PSS_BIT_USB},
> +	{"USB_SUS",		PMC_PSS_BIT_USB_SUS},
> +	{},
>  };
>  
>  static inline u32 pmc_reg_read(struct pmc_dev *pmc, int reg_offset)
> @@ -148,16 +150,14 @@ static int pmc_dev_state_show(struct seq_file *s, void *unused)
>  	struct pmc_dev *pmc = s->private;
>  	u32 func_dis, func_dis_2, func_dis_index;
>  	u32 d3_sts_0, d3_sts_1, d3_sts_index;
> -	int dev_num, dev_index, reg_index;
> +	int dev_index, reg_index;
>  
>  	func_dis = pmc_reg_read(pmc, PMC_FUNC_DIS);
>  	func_dis_2 = pmc_reg_read(pmc, PMC_FUNC_DIS_2);
>  	d3_sts_0 = pmc_reg_read(pmc, PMC_D3_STS_0);
>  	d3_sts_1 = pmc_reg_read(pmc, PMC_D3_STS_1);
>  
> -	dev_num = ARRAY_SIZE(dev_map);
> -
> -	for (dev_index = 0; dev_index < dev_num; dev_index++) {
> +	for (dev_index = 0; dev_map[dev_index].name; dev_index++) {
>  		reg_index = dev_index / PMC_REG_BIT_WIDTH;
>  		if (reg_index) {
>  			func_dis_index = func_dis_2;
> @@ -167,8 +167,8 @@ static int pmc_dev_state_show(struct seq_file *s, void *unused)
>  			d3_sts_index = d3_sts_0;
>  		}
>  
> -		seq_printf(s, "Dev: %-32s\tState: %s [%s]\n",
> -			dev_map[dev_index].name,
> +		seq_printf(s, "Dev: %-2d - %-32s\tState: %s [%s]\n",
> +			dev_index, dev_map[dev_index].name,
>  			dev_map[dev_index].bit_mask & func_dis_index ?
>  			"Disabled" : "Enabled ",
>  			dev_map[dev_index].bit_mask & d3_sts_index ?
> @@ -195,9 +195,9 @@ static int pmc_pss_state_show(struct seq_file *s, void *unused)
>  	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,
> +	for (pss_index = 0; pss_map[pss_index].name; pss_index++) {
> +		seq_printf(s, "Island: %-2d - %-32s\tState: %s\n",
> +			pss_index, pss_map[pss_index].name,
>  			pss_map[pss_index].bit_mask & pss ? "Off" : "On");
>  	}
>  	return 0;
> @@ -322,9 +322,8 @@ static int pmc_setup_dev(struct pci_dev *pdev)
>  	pmc_hw_reg_setup(pmc);
>  
>  	ret = pmc_dbgfs_register(pmc);
> -	if (ret) {
> +	if (ret)
>  		iounmap(pmc->regmap);
> -	}
>  
>  	return ret;
>  }
> 

Re: [PATCH v2 4/4] PMC driver: Add Cherrytrail PMC interface

[Li, Aubrey]

On 2015/1/21 5:50, Andy Shevchenko wrote:
> The patch adds CHT PMC interface. This exposes all the South IP device power
> states and S0ix states for CHT. The bit map of FUNC_DIS and D3_STS_0 registers
> for SoCs are consistent. The D3_STS_1 and FUNC_DIS_2 registers, however, are
> not aligned. This is fixed by splitting a common mapping on per register basis.
> 
Should we define the bit map table completely separate for different
platforms? My concern is, when D3_STS_0 and FUNC_DIS becomes not
consistent in a new SoC, the implementation in this patch has to be
rewritten completely.

Defining entire bit map table for different platform introduces
reduplicated bit definitions, but when we add a new platform in future,
we don't need to consider the existing platforms definition, and no need
to change code structure any longer.

Thoughts?

Thanks,
-Aubrey

> Signed-off-by: Kumar P Mahesh <mahesh.kumar.p@intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  arch/x86/include/asm/pmc_atom.h |  25 +++++++++
>  arch/x86/kernel/pmc_atom.c      | 118 ++++++++++++++++++++++++++++++----------
>  2 files changed, 114 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/x86/include/asm/pmc_atom.h b/arch/x86/include/asm/pmc_atom.h
> index bc0fc08..000a223 100644
> --- a/arch/x86/include/asm/pmc_atom.h
> +++ b/arch/x86/include/asm/pmc_atom.h
> @@ -18,6 +18,8 @@
>  
>  /* ValleyView Power Control Unit PCI Device ID */
>  #define	PCI_DEVICE_ID_VLV_PMC	0x0F1C
> +/* CherryTrail Power Control Unit PCI Device ID */
> +#define	PCI_DEVICE_ID_CHT_PMC	0x229C
>  
>  /* PMC Memory mapped IO registers */
>  #define	PMC_BASE_ADDR_OFFSET	0x44
> @@ -29,6 +31,10 @@
>  #define	PMC_FUNC_DIS		0x34
>  #define	PMC_FUNC_DIS_2		0x38
>  
> +/* CHT specific bits in FUNC_DIS2 register */
> +#define	BIT_FD_GMM		BIT(3)
> +#define	BIT_FD_ISH		BIT(4)
> +
>  /* S0ix wake event control */
>  #define	PMC_S0IX_WAKE_EN	0x3C
>  
> @@ -75,6 +81,21 @@
>  #define PMC_PSS_BIT_USB			BIT(16)
>  #define PMC_PSS_BIT_USB_SUS		BIT(17)
>  
> +/* CHT specific bits in PSS register */
> +#define	PMC_PSS_BIT_CHT_UFS		BIT(7)
> +#define	PMC_PSS_BIT_CHT_UXD		BIT(11)
> +#define	PMC_PSS_BIT_CHT_UXD_FD		BIT(12)
> +#define	PMC_PSS_BIT_CHT_UX_ENG		BIT(15)
> +#define	PMC_PSS_BIT_CHT_USB_SUS		BIT(16)
> +#define	PMC_PSS_BIT_CHT_GMM		BIT(17)
> +#define	PMC_PSS_BIT_CHT_ISH		BIT(18)
> +#define	PMC_PSS_BIT_CHT_DFX_MASTER	BIT(26)
> +#define	PMC_PSS_BIT_CHT_DFX_CLUSTER1	BIT(27)
> +#define	PMC_PSS_BIT_CHT_DFX_CLUSTER2	BIT(28)
> +#define	PMC_PSS_BIT_CHT_DFX_CLUSTER3	BIT(29)
> +#define	PMC_PSS_BIT_CHT_DFX_CLUSTER4	BIT(30)
> +#define	PMC_PSS_BIT_CHT_DFX_CLUSTER5	BIT(31)
> +
>  /* These registers reflect D3 status of functions */
>  #define	PMC_D3_STS_0		0xA0
>  
> @@ -117,6 +138,10 @@
>  #define	BIT_USH_SS_PHY		BIT(2)
>  #define	BIT_DFX			BIT(3)
>  
> +/* CHT specific bits in PMC_D3_STS_1 register */
> +#define	BIT_STS_GMM		BIT(1)
> +#define	BIT_STS_ISH		BIT(2)
> +
>  /* PMC I/O Registers */
>  #define	ACPI_BASE_ADDR_OFFSET	0x40
>  #define	ACPI_BASE_ADDR_MASK	0xFFFFFE00
> diff --git a/arch/x86/kernel/pmc_atom.c b/arch/x86/kernel/pmc_atom.c
> index 0f24ef7..41f4a33 100644
> --- a/arch/x86/kernel/pmc_atom.c
> +++ b/arch/x86/kernel/pmc_atom.c
> @@ -31,7 +31,10 @@ struct pmc_bit_map {
>  };
>  
>  struct pmc_reg_map {
> -	const struct pmc_bit_map *dev;
> +	const struct pmc_bit_map *d3_sts_0;
> +	const struct pmc_bit_map *d3_sts_1;
> +	const struct pmc_bit_map *func_dis;
> +	const struct pmc_bit_map *func_dis_2;
>  	const struct pmc_bit_map *pss;
>  };
>  
> @@ -48,7 +51,7 @@ struct pmc_dev {
>  static struct pmc_dev pmc_device;
>  static u32 acpi_base_addr;
>  
> -static const struct pmc_bit_map dev_map[] = {
> +static const struct pmc_bit_map d3_sts_0_map[] = {
>  	{"LPSS1_F0_DMA",	BIT_LPSS1_F0_DMA},
>  	{"LPSS1_F1_PWM1",	BIT_LPSS1_F1_PWM1},
>  	{"LPSS1_F2_PWM2",	BIT_LPSS1_F2_PWM2},
> @@ -81,6 +84,10 @@ static const struct pmc_bit_map dev_map[] = {
>  	{"LPSS2_F5_I2C5",	BIT_LPSS2_F5_I2C5},
>  	{"LPSS2_F6_I2C6",	BIT_LPSS2_F6_I2C6},
>  	{"LPSS2_F7_I2C7",	BIT_LPSS2_F7_I2C7},
> +	{},
> +};
> +
> +static struct pmc_bit_map byt_d3_sts_1_map[] = {
>  	{"SMB",			BIT_SMB},
>  	{"OTG_SS_PHY",		BIT_OTG_SS_PHY},
>  	{"USH_SS_PHY",		BIT_USH_SS_PHY},
> @@ -88,7 +95,21 @@ static const struct pmc_bit_map dev_map[] = {
>  	{},
>  };
>  
> -static const struct pmc_bit_map pss_map[] = {
> +static struct pmc_bit_map cht_d3_sts_1_map[] = {
> +	{"SMB",			BIT_SMB},
> +	{"GMM",			BIT_STS_GMM},
> +	{"ISH",			BIT_STS_ISH},
> +	{},
> +};
> +
> +static struct pmc_bit_map cht_func_dis_2_map[] = {
> +	{"SMB",			BIT_SMB},
> +	{"GMM",			BIT_FD_GMM},
> +	{"ISH",			BIT_FD_ISH},
> +	{},
> +};
> +
> +static const struct pmc_bit_map byt_pss_map[] = {
>  	{"GBE",			PMC_PSS_BIT_GBE},
>  	{"SATA",		PMC_PSS_BIT_SATA},
>  	{"HDA",			PMC_PSS_BIT_HDA},
> @@ -110,9 +131,43 @@ static const struct pmc_bit_map pss_map[] = {
>  	{},
>  };
>  
> -static const struct pmc_reg_map reg_map = {
> -	.dev		= dev_map,
> -	.pss		= pss_map,
> +static const struct pmc_bit_map cht_pss_map[] = {
> +	{"SATA",		PMC_PSS_BIT_SATA},
> +	{"HDA",			PMC_PSS_BIT_HDA},
> +	{"SEC",			PMC_PSS_BIT_SEC},
> +	{"PCIE",		PMC_PSS_BIT_PCIE},
> +	{"LPSS",		PMC_PSS_BIT_LPSS},
> +	{"LPE",			PMC_PSS_BIT_LPE},
> +	{"UFS",			PMC_PSS_BIT_CHT_UFS},
> +	{"UXD",			PMC_PSS_BIT_CHT_UXD},
> +	{"UXD_FD",		PMC_PSS_BIT_CHT_UXD_FD},
> +	{"UX_ENG",		PMC_PSS_BIT_CHT_UX_ENG},
> +	{"USB_SUS",		PMC_PSS_BIT_CHT_USB_SUS},
> +	{"GMM",			PMC_PSS_BIT_CHT_GMM},
> +	{"ISH",			PMC_PSS_BIT_CHT_ISH},
> +	{"DFX_MASTER",		PMC_PSS_BIT_CHT_DFX_MASTER},
> +	{"DFX_CLUSTER1",	PMC_PSS_BIT_CHT_DFX_CLUSTER1},
> +	{"DFX_CLUSTER2",	PMC_PSS_BIT_CHT_DFX_CLUSTER2},
> +	{"DFX_CLUSTER3",	PMC_PSS_BIT_CHT_DFX_CLUSTER3},
> +	{"DFX_CLUSTER4",	PMC_PSS_BIT_CHT_DFX_CLUSTER4},
> +	{"DFX_CLUSTER5",	PMC_PSS_BIT_CHT_DFX_CLUSTER5},
> +	{},
> +};
> +
> +static const struct pmc_reg_map byt_reg_map = {
> +	.d3_sts_0	= d3_sts_0_map,
> +	.d3_sts_1	= byt_d3_sts_1_map,
> +	.func_dis	= d3_sts_0_map,
> +	.func_dis_2	= byt_d3_sts_1_map,
> +	.pss		= byt_pss_map,
> +};
> +
> +static const struct pmc_reg_map cht_reg_map = {
> +	.d3_sts_0	= d3_sts_0_map,
> +	.d3_sts_1	= cht_d3_sts_1_map,
> +	.func_dis	= d3_sts_0_map,
> +	.func_dis_2	= cht_func_dis_2_map,
> +	.pss		= cht_pss_map,
>  };
>  
>  static inline u32 pmc_reg_read(struct pmc_dev *pmc, int reg_offset)
> @@ -156,36 +211,39 @@ static void pmc_hw_reg_setup(struct pmc_dev *pmc)
>  }
>  
>  #ifdef CONFIG_DEBUG_FS
> +static void pmc_dev_state_print(struct seq_file *s, int reg_index,
> +				u32 sts, const struct pmc_bit_map *sts_map,
> +				u32 fd, const struct pmc_bit_map *fd_map)
> +{
> +	int offset = PMC_REG_BIT_WIDTH * reg_index;
> +	int index;
> +
> +	for (index = 0; sts_map[index].name; index++) {
> +		seq_printf(s, "Dev: %-2d - %-32s\tState: %s [%s]\n",
> +			offset + index, sts_map[index].name,
> +			fd_map[index].bit_mask & fd ?  "Disabled" : "Enabled ",
> +			sts_map[index].bit_mask & sts ?  "D3" : "D0");
> +	}
> +}
> +
>  static int pmc_dev_state_show(struct seq_file *s, void *unused)
>  {
>  	struct pmc_dev *pmc = s->private;
> -	const struct pmc_bit_map *map = pmc->map->dev;
> -	u32 func_dis, func_dis_2, func_dis_index;
> -	u32 d3_sts_0, d3_sts_1, d3_sts_index;
> -	int index, reg_index;
> +	const struct pmc_reg_map *m = pmc->map;
> +	u32 func_dis, func_dis_2;
> +	u32 d3_sts_0, d3_sts_1;
>  
>  	func_dis = pmc_reg_read(pmc, PMC_FUNC_DIS);
>  	func_dis_2 = pmc_reg_read(pmc, PMC_FUNC_DIS_2);
>  	d3_sts_0 = pmc_reg_read(pmc, PMC_D3_STS_0);
>  	d3_sts_1 = pmc_reg_read(pmc, PMC_D3_STS_1);
>  
> -	for (index = 0; map[index].name; index++) {
> -		reg_index = index / PMC_REG_BIT_WIDTH;
> -		if (reg_index) {
> -			func_dis_index = func_dis_2;
> -			d3_sts_index = d3_sts_1;
> -		} else {
> -			func_dis_index = func_dis;
> -			d3_sts_index = d3_sts_0;
> -		}
> +	/* Low part */
> +	pmc_dev_state_print(s, 0, d3_sts_0, m->d3_sts_0, func_dis, m->func_dis);
> +
> +	/* High part */
> +	pmc_dev_state_print(s, 1, d3_sts_1, m->d3_sts_1, func_dis_2, m->func_dis_2);
>  
> -		seq_printf(s, "Dev: %-2d - %-32s\tState: %s [%s]\n",
> -			index, map[index].name,
> -			map[index].bit_mask & func_dis_index ?
> -			"Disabled" : "Enabled ",
> -			map[index].bit_mask & d3_sts_index ?
> -			"D3" : "D0");
> -	}
>  	return 0;
>  }
>  
> @@ -307,9 +365,10 @@ static int pmc_dbgfs_register(struct pmc_dev *pmc)
>  }
>  #endif /* CONFIG_DEBUG_FS */
>  
> -static int pmc_setup_dev(struct pci_dev *pdev, const struct pmc_reg_map *map)
> +static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent)
>  {
>  	struct pmc_dev *pmc = &pmc_device;
> +	const struct pmc_reg_map *map = (struct pmc_reg_map *)ent->driver_data;
>  	int ret;
>  
>  	/* Obtain ACPI base address */
> @@ -351,7 +410,8 @@ static int pmc_setup_dev(struct pci_dev *pdev, const struct pmc_reg_map *map)
>   * a driver on the same PCI id.
>   */
>  static const struct pci_device_id pmc_pci_ids[] = {
> -	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_VLV_PMC) },
> +	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_VLV_PMC), (kernel_ulong_t)&byt_reg_map },
> +	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_CHT_PMC), (kernel_ulong_t)&cht_reg_map },
>  	{ 0, },
>  };
>  
> @@ -373,7 +433,7 @@ static int __init pmc_atom_init(void)
>  	for_each_pci_dev(pdev) {
>  		ent = pci_match_id(pmc_pci_ids, pdev);
>  		if (ent)
> -			return pmc_setup_dev(pdev, &reg_map);
> +			return pmc_setup_dev(pdev, ent);
>  	}
>  	/* Device not found. */
>  	return -ENODEV;
> 

Re: [PATCH v2 4/4] PMC driver: Add Cherrytrail PMC interface

[Andy Shevchenko]

On Thu, 2015-01-22 at 12:02 +0800, Li, Aubrey wrote:
> On 2015/1/21 5:50, Andy Shevchenko wrote:
> > The patch adds CHT PMC interface. This exposes all the South IP device power
> > states and S0ix states for CHT. The bit map of FUNC_DIS and D3_STS_0 registers
> > for SoCs are consistent. The D3_STS_1 and FUNC_DIS_2 registers, however, are
> > not aligned. This is fixed by splitting a common mapping on per register basis.
> > 
> Should we define the bit map table completely separate for different
> platforms? My concern is, when D3_STS_0 and FUNC_DIS becomes not
> consistent in a new SoC, the implementation in this patch has to be
> rewritten completely.
> 
> Defining entire bit map table for different platform introduces
> reduplicated bit definitions, but when we add a new platform in future,
> we don't need to consider the existing platforms definition, and no need
> to change code structure any longer.
> 
> Thoughts?
> 

But this what I did by introducing pmc_reg_map structure per SoC.
You may or may not use previous definitions.


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

Re: [PATCH v2 1/4] x86: pmc_atom: save struct device pointer in pmc

[Andy Shevchenko]

On Thu, 2015-01-22 at 11:42 +0800, Li, Aubrey wrote:
> On 2015/1/21 5:50, Andy Shevchenko wrote:
> > The change allows to use dev_printk() type of macros in the module functions.
> 
> The patch itself looks good to me, but not sure why do we need this
> change, will we use dev_prink in the subsequent patches?

Has no relations to the rest, indeed. Just found that would be easier to
send them together.

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

Re: [PATCH v2 2/4] x86: pmc_atom: print index of device in loop

[Andy Shevchenko]

On Thu, 2015-01-22 at 11:45 +0800, Li, Aubrey wrote:
> On 2015/1/21 5:50, Andy Shevchenko wrote:
> > The register mapping may change from one platform to another. Thus, indices
> > might be not the same on different platforms. The patch makes the code to print
> > the device index dynamically at run time.
> 
> Will another platform use the same table but different bit position? In
> my opinion, different platform should use different mapping table.

Yes, indeed.

The only improvement I could suggest now is to use indices for bit field
name from one array of possible names.

Or use macro to fill the item like 
#define BIT_X(bitname) { .name = __stringify(bitname), .bit_mask = BIT_
## bitname, }


> > 
> > The patch also changes the for loop to iterate over the map until a terminator
> > is found.
> 
> Why do we need to do this? did you see any hurt from the existing
> implementation?

Just a micro optimization plus it allows in consequent patches to avoid
size members in the pmc_reg_map.

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

Re: [PATCH v2 4/4] PMC driver: Add Cherrytrail PMC interface

[Li, Aubrey]

On 2015/1/22 17:26, Andy Shevchenko wrote:
> On Thu, 2015-01-22 at 12:02 +0800, Li, Aubrey wrote:
>> On 2015/1/21 5:50, Andy Shevchenko wrote:
>>> The patch adds CHT PMC interface. This exposes all the South IP device power
>>> states and S0ix states for CHT. The bit map of FUNC_DIS and D3_STS_0 registers
>>> for SoCs are consistent. The D3_STS_1 and FUNC_DIS_2 registers, however, are
>>> not aligned. This is fixed by splitting a common mapping on per register basis.
>>>
>> Should we define the bit map table completely separate for different
>> platforms? My concern is, when D3_STS_0 and FUNC_DIS becomes not
>> consistent in a new SoC, the implementation in this patch has to be
>> rewritten completely.
>>
>> Defining entire bit map table for different platform introduces
>> reduplicated bit definitions, but when we add a new platform in future,
>> we don't need to consider the existing platforms definition, and no need
>> to change code structure any longer.
>>
>> Thoughts?
>>
> 
> But this what I did by introducing pmc_reg_map structure per SoC.
> You may or may not use previous definitions.
> 
okay, it makes sense to me.

Thanks,
-Aubrey

Re: [PATCH v2 0/4] x86: pmc_atom: Add Cherrytrail support

[Andy Shevchenko]

On Tue, 2015-01-20 at 23:49 +0200, Andy Shevchenko wrote:
> This is the reworked patch series which had been sent earlier [1] to support
> Intel CherryTrail SoC.
> 
> The patches were tested on both BayTrail and CherryTrail SoCs.
> 
> [1] https://patchwork.kernel.org/patch/5235891/

Aubrey, is everything is clear for you now? Can I send v3 with your
Ack's?

> 
> Changes to v1:
>  - rebased on top of recent pmc_atom changes
>  - cleaned up, reqorked and split to few smaller patches
> 
> Andy Shevchenko (4):
>   x86: pmc_atom: save struct device pointer in pmc
>   x86: pmc_atom: print index of device in loop
>   x86: pmc_atom: supply register mappings via pmc object
>   PMC driver: Add Cherrytrail PMC interface
> 
>  arch/x86/include/asm/pmc_atom.h |  25 ++++
>  arch/x86/kernel/pmc_atom.c      | 270 +++++++++++++++++++++++++---------------
>  2 files changed, 198 insertions(+), 97 deletions(-)
> 


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

Re: [PATCH v2 0/4] x86: pmc_atom: Add Cherrytrail support

[Li, Aubrey]

On 2015/2/23 20:45, Andy Shevchenko wrote:
> On Tue, 2015-01-20 at 23:49 +0200, Andy Shevchenko wrote:
>> This is the reworked patch series which had been sent earlier [1] to support
>> Intel CherryTrail SoC.
>>
>> The patches were tested on both BayTrail and CherryTrail SoCs.
>>
>> [1] https://patchwork.kernel.org/patch/5235891/
> 
> Aubrey, is everything is clear for you now? Can I send v3 with your
> Ack's?
> 
Sorry for the late response due to Chinese New Year holiday, I'll send
my comments or Ack soon.

Thanks,
-Aubrey

>>
>> Changes to v1:
>>  - rebased on top of recent pmc_atom changes
>>  - cleaned up, reqorked and split to few smaller patches
>>
>> Andy Shevchenko (4):
>>   x86: pmc_atom: save struct device pointer in pmc
>>   x86: pmc_atom: print index of device in loop
>>   x86: pmc_atom: supply register mappings via pmc object
>>   PMC driver: Add Cherrytrail PMC interface
>>
>>  arch/x86/include/asm/pmc_atom.h |  25 ++++
>>  arch/x86/kernel/pmc_atom.c      | 270 +++++++++++++++++++++++++---------------
>>  2 files changed, 198 insertions(+), 97 deletions(-)
>>
> 
> 

Re: [PATCH v2 0/4] x86: pmc_atom: Add Cherrytrail support

[Li, Aubrey]

On 2015/2/23 20:45, Andy Shevchenko wrote:
> On Tue, 2015-01-20 at 23:49 +0200, Andy Shevchenko wrote:
>> This is the reworked patch series which had been sent earlier [1] to support
>> Intel CherryTrail SoC.
>>
>> The patches were tested on both BayTrail and CherryTrail SoCs.
>>
>> [1] https://patchwork.kernel.org/patch/5235891/
> 
> Aubrey, is everything is clear for you now? Can I send v3 with your
> Ack's?
> 

The patches overall look good to me, except a few minor comments need to
be addressed in the last conversation. For example, I think we don't
need patch 1/4 if we won't use dev_print. some other changes might be
necessary only if they makes code cleaner and works better.

Certainly, it would be better if other x86 maintainers can take a look
at these patches.

Thanks,
-Aubrey
>>
>> Changes to v1:
>>  - rebased on top of recent pmc_atom changes
>>  - cleaned up, reqorked and split to few smaller patches
>>
>> Andy Shevchenko (4):
>>   x86: pmc_atom: save struct device pointer in pmc
>>   x86: pmc_atom: print index of device in loop
>>   x86: pmc_atom: supply register mappings via pmc object
>>   PMC driver: Add Cherrytrail PMC interface
>>
>>  arch/x86/include/asm/pmc_atom.h |  25 ++++
>>  arch/x86/kernel/pmc_atom.c      | 270 +++++++++++++++++++++++++---------------
>>  2 files changed, 198 insertions(+), 97 deletions(-)
>>
> 
> 

Re: [PATCH v2 0/4] x86: pmc_atom: Add Cherrytrail support

[Andy Shevchenko]

On Tue, 2015-03-03 at 11:37 +0800, Li, Aubrey wrote:
> On 2015/2/23 20:45, Andy Shevchenko wrote:
> > On Tue, 2015-01-20 at 23:49 +0200, Andy Shevchenko wrote:
> >> This is the reworked patch series which had been sent earlier [1] to support
> >> Intel CherryTrail SoC.
> >>
> >> The patches were tested on both BayTrail and CherryTrail SoCs.
> >>
> >> [1] https://patchwork.kernel.org/patch/5235891/
> > 
> > Aubrey, is everything is clear for you now? Can I send v3 with your
> > Ack's?

> The patches overall look good to me, except a few minor comments need to
> be addressed in the last conversation. For example, I think we don't
> need patch 1/4 if we won't use dev_print.

We still use them. Like I said the patch has no relations to the series,
though it simplifies already existing function.


>  some other changes might be
> necessary only if they makes code cleaner and works better.

Thus, I think the patch 1/4 is still useful.

> Certainly, it would be better if other x86 maintainers can take a look
> at these patches.

Agree.

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

Re: [PATCH v2 0/4] x86: pmc_atom: Add Cherrytrail support

[Shevchenko, Andriy]

On Mon, 2015-02-23 at 14:45 +0200, Andy Shevchenko wrote:
> On Tue, 2015-01-20 at 23:49 +0200, Andy Shevchenko wrote:
> > This is the reworked patch series which had been sent earlier [1] to support
> > Intel CherryTrail SoC.
> > 
> > The patches were tested on both BayTrail and CherryTrail SoCs.
> > 
> > [1] https://patchwork.kernel.org/patch/5235891/
> 
> Aubrey, is everything is clear for you now? Can I send v3 with your
> Ack's?

So, what is the status of the review then? Do I have anything to
improve / comment?

> 
> > 
> > Changes to v1:
> >  - rebased on top of recent pmc_atom changes
> >  - cleaned up, reqorked and split to few smaller patches
> > 
> > Andy Shevchenko (4):
> >   x86: pmc_atom: save struct device pointer in pmc
> >   x86: pmc_atom: print index of device in loop
> >   x86: pmc_atom: supply register mappings via pmc object
> >   PMC driver: Add Cherrytrail PMC interface
> > 
> >  arch/x86/include/asm/pmc_atom.h |  25 ++++
> >  arch/x86/kernel/pmc_atom.c      | 270 +++++++++++++++++++++++++---------------
> >  2 files changed, 198 insertions(+), 97 deletions(-)
> > 
> 
> 


-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH v2 0/4] x86: pmc_atom: Add Cherrytrail support

[Shevchenko, Andriy]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1793 bytes --]

On Mon, 2015-02-23 at 14:45 +0200, Andy Shevchenko wrote:
> On Tue, 2015-01-20 at 23:49 +0200, Andy Shevchenko wrote:
> > This is the reworked patch series which had been sent earlier [1] to support
> > Intel CherryTrail SoC.
> > 
> > The patches were tested on both BayTrail and CherryTrail SoCs.
> > 
> > [1] https://patchwork.kernel.org/patch/5235891/
> 
> Aubrey, is everything is clear for you now? Can I send v3 with your
> Ack's?

So, what is the status of the review then? Do I have anything to
improve / comment?

> 
> > 
> > Changes to v1:
> >  - rebased on top of recent pmc_atom changes
> >  - cleaned up, reqorked and split to few smaller patches
> > 
> > Andy Shevchenko (4):
> >   x86: pmc_atom: save struct device pointer in pmc
> >   x86: pmc_atom: print index of device in loop
> >   x86: pmc_atom: supply register mappings via pmc object
> >   PMC driver: Add Cherrytrail PMC interface
> > 
> >  arch/x86/include/asm/pmc_atom.h |  25 ++++
> >  arch/x86/kernel/pmc_atom.c      | 270 +++++++++++++++++++++++++---------------
> >  2 files changed, 198 insertions(+), 97 deletions(-)
> > 
> 
> 


-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2 0/4] x86: pmc_atom: Add Cherrytrail support

[Ingo Molnar]

* Shevchenko, Andriy <andriy.shevchenko@intel.com> wrote:

> On Mon, 2015-02-23 at 14:45 +0200, Andy Shevchenko wrote:
> > On Tue, 2015-01-20 at 23:49 +0200, Andy Shevchenko wrote:
> > > This is the reworked patch series which had been sent earlier [1] to support
> > > Intel CherryTrail SoC.
> > > 
> > > The patches were tested on both BayTrail and CherryTrail SoCs.
> > > 
> > > [1] https://patchwork.kernel.org/patch/5235891/
> > 
> > Aubrey, is everything is clear for you now? Can I send v3 with your
> > Ack's?
> 
> So, what is the status of the review then? Do I have anything to
> improve / comment?

So the last review pass pointed out a few minor problems (and also 
generated a few acks), please propagate those into the series and 
resend the latestest so I can have a fresh look.

Thanks,

	Ingo