Re: [PATCH] platform/x86/intel/pmc: Add Arrow Lake U/H support to intel_pmc_core driver

[rajvi.jingar]

Hi Ilpo,

Thanks for reviewing the patch.

Restructuring the code to handle platform variations won't be that 
straightforward, so we will be sending the next version in next cycle rc1.

Thanks,

Rajvi

On 9/10/2024 2:32 AM, Ilpo Järvinen wrote:

> On Mon, 9 Sep 2024, Rajvi Jingar wrote:
>
> > Add Arrow Lake U and Arro Lake H support in intel_pmc_core driver
> Arrow
>
> Add . to the end of sentence.
>
> > Signed-off-by: Rajvi Jingar <rajvi.jingar@xxxxxxxxxxxxxxx>
> > ---
> >   drivers/platform/x86/intel/pmc/arl.c  | 65 ++++++++++++++++++++++-----
> >   drivers/platform/x86/intel/pmc/core.c |  2 +
> >   drivers/platform/x86/intel/pmc/core.h |  7 +++
> >   3 files changed, 64 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/platform/x86/intel/pmc/arl.c b/drivers/platform/x86/intel/pmc/arl.c
> > index e10527c4e3e0..964f5f040dd9 100644
> > --- a/drivers/platform/x86/intel/pmc/arl.c
> > +++ b/drivers/platform/x86/intel/pmc/arl.c
> > @@ -16,6 +16,7 @@
> >   #define IOEP_LPM_REQ_GUID	0x5077612
> >   #define SOCS_LPM_REQ_GUID	0x8478657
> >   #define PCHS_LPM_REQ_GUID	0x9684572
> > +#define SOCM_LPM_REQ_GUID	0x2625030
> >   
> >   static const u8 ARL_LPM_REG_INDEX[] = {0, 4, 5, 6, 8, 9, 10, 11, 12, 13, 14, 15, 16, 20};
> >   
> > @@ -650,6 +651,7 @@ const struct pmc_reg_map arl_pchs_reg_map = {
> >   	.etr3_offset = ETR3_OFFSET,
> >   };
> >   
> > +#define PMC_DEVID_SOCM 0x777f
> >   #define PMC_DEVID_SOCS 0xae7f
> >   #define PMC_DEVID_IOEP 0x7ecf
> >   #define PMC_DEVID_PCHS 0x7f27
> > @@ -669,11 +671,17 @@ static struct pmc_info arl_pmc_info_list[] = {
> >   		.devid	= PMC_DEVID_PCHS,
> >   		.map	= &arl_pchs_reg_map,
> >   	},
> > +	{
> > +		.guid	= SOCM_LPM_REQ_GUID,
> > +		.devid	= PMC_DEVID_SOCM,
> > +		.map	= &mtl_socm_reg_map,
> > +	},
> >   	{}
> >   };
> >   
> >   #define ARL_NPU_PCI_DEV			0xad1d
> >   #define ARL_GNA_PCI_DEV			0xae4c
> > +#define ARL_H_GNA_PCI_DEV 		0x774c
> >   /*
> >    * Set power state of select devices that do not have drivers to D3
> >    * so that they do not block Package C entry.
> > @@ -684,6 +692,12 @@ static void arl_d3_fixup(void)
> >   	pmc_core_set_device_d3(ARL_GNA_PCI_DEV);
> >   }
> >   
> > +static void arl_h_d3_fixup(void)
> > +{
> > +	pmc_core_set_device_d3(ARL_NPU_PCI_DEV);
> > +	pmc_core_set_device_d3(ARL_H_GNA_PCI_DEV);
> > +}
> > +
> >   static int arl_resume(struct pmc_dev *pmcdev)
> >   {
> >   	arl_d3_fixup();
> > @@ -692,16 +706,47 @@ static int arl_resume(struct pmc_dev *pmcdev)
> >   	return pmc_core_resume_common(pmcdev);
> >   }
> >   
> > +static int arl_h_resume(struct pmc_dev *pmcdev)
> > +{
> > +	arl_h_d3_fixup();
> > +	pmc_core_send_ltr_ignore(pmcdev, 3, 0);
> > +
> > +	return pmc_core_resume_common(pmcdev);
> > +}
> > +
> > +int arl_h_core_init(struct pmc_dev *pmcdev)
> > +{
> > +	arl_h_d3_fixup();
> > +
> > +	return arl_core_generic_init(pmcdev, SOC_M);
> > +}
> > +
> >   int arl_core_init(struct pmc_dev *pmcdev)
> >   {
> > -	struct pmc *pmc = pmcdev->pmcs[PMC_IDX_SOC];
> > +	arl_d3_fixup();
> > +
> > +	return arl_core_generic_init(pmcdev, SOC_S);
> > +}
> > +
> > +int arl_core_generic_init(struct pmc_dev *pmcdev, int soc_tp)
> This function has no callers outside or arl.c and should be made static
> and the code should be reordered such that no prototype is needed.
>
> > +{
> >   	int ret;
> > -	int func = 0;
> > +	int func;
> >   	bool ssram_init = true;
> > +	struct pmc *pmc = pmcdev->pmcs[PMC_IDX_SOC];
> >   
> > -	arl_d3_fixup();
> >   	pmcdev->suspend = cnl_suspend;
> > -	pmcdev->resume = arl_resume;
> > +
> > +	if (soc_tp == SOC_M) {
> > +		func = 2;
> > +		pmcdev->resume = arl_h_resume;
> > +	} else if (soc_tp == SOC_S) {
> > +		func = 0;
> > +		pmcdev->resume = arl_resume;
> It would be preferrable to make an info structure to describe this kind
> of platform variations so that if () forests like this are avoided.
>
> > +	} else {
> > +		return -EINVAL;
> > +	}
> > +
> >   	pmcdev->regmap_list = arl_pmc_info_list;
> >   
> >   	/*
> > @@ -711,7 +756,10 @@ int arl_core_init(struct pmc_dev *pmcdev)
> >   	ret = pmc_core_ssram_init(pmcdev, func);
> >   	if (ret) {
> >   		ssram_init = false;
> > -		pmc->map = &arl_socs_reg_map;
> > +		if (soc_tp == SOC_M)
> > +			pmc->map = &mtl_socm_reg_map;
> > +		else
> > +			pmc->map = &arl_socs_reg_map;
> As with above, use an info struct.