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

["Li, Aubrey" <aubrey.li@linux.intel.com>]

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