Re: [PATCH v3 2/3] platform/x86:intel/pmc: Create generic_core_init() for all platforms

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5104 bytes --]

On Fri, 20 Dec 2024, Xi Pardee wrote:
> On 12/20/2024 3:52 AM, Ilpo Järvinen wrote:
> > On Thu, 19 Dec 2024, Xi Pardee wrote:
> > 
> > > Create a generic_core_init() function for all architecture to reduce
> > > duplicate code in each architecture file. Create an info structure
> > > to catch the variations between each architecture and pass it to the
> > > generic init function.
> > > 
> > > Convert all architectures to call the generic core init function.
> > > 
> > > Signed-off-by: Xi Pardee <xi.pardee@linux.intel.com>
> > This looks much better!
> > 
> > > ---
> > >   drivers/platform/x86/intel/pmc/adl.c  | 21 ++++--------
> > >   drivers/platform/x86/intel/pmc/arl.c  | 47 ++++++++-------------------
> > >   drivers/platform/x86/intel/pmc/cnp.c  | 21 ++++--------
> > >   drivers/platform/x86/intel/pmc/core.c | 45 +++++++++++++++++++++++++
> > >   drivers/platform/x86/intel/pmc/core.h | 25 ++++++++++++++
> > >   drivers/platform/x86/intel/pmc/icl.c  | 18 ++++------
> > >   drivers/platform/x86/intel/pmc/lnl.c  | 24 +++++---------
> > >   drivers/platform/x86/intel/pmc/mtl.c  | 45 +++++++------------------
> > >   drivers/platform/x86/intel/pmc/spt.c  | 18 ++++------
> > >   drivers/platform/x86/intel/pmc/tgl.c  | 31 +++++++++---------
> > >   10 files changed, 145 insertions(+), 150 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/intel/pmc/adl.c
> > > b/drivers/platform/x86/intel/pmc/adl.c
> > > index e7878558fd909..ac37f4ece9c70 100644
> > > --- a/drivers/platform/x86/intel/pmc/adl.c
> > > +++ b/drivers/platform/x86/intel/pmc/adl.c

> > > diff --git a/drivers/platform/x86/intel/pmc/arl.c
> > > b/drivers/platform/x86/intel/pmc/arl.c
> > > index 05dec4f5019f3..137a1fdfee715 100644
> > > --- a/drivers/platform/x86/intel/pmc/arl.c
> > > +++ b/drivers/platform/x86/intel/pmc/arl.c
> > > @@ -691,40 +691,19 @@ static int arl_resume(struct pmc_dev *pmcdev)
> > >   	return cnl_resume(pmcdev);
> > >   }
> > >   +static struct pmc_dev_info arl_pmc_dev = {
> > > +	.func = 0,
> > > +	.ssram = true,
> > > +	.dmu_guid = ARL_PMT_DMU_GUID,
> > > +	.regmap_list = arl_pmc_info_list,
> > > +	.map = &arl_socs_reg_map,
> > > +	.fixup = arl_d3_fixup,
> > I think the fixups should be left to be called from the per architecture
> > init funcs.
> 
> Will rename the fixup field to platform_specifc (more explanation at the end).

> > > diff --git a/drivers/platform/x86/intel/pmc/core.h
> > > b/drivers/platform/x86/intel/pmc/core.h
> > > index a1886d8e1ef3e..82be953db9463 100644
> > > --- a/drivers/platform/x86/intel/pmc/core.h
> > > +++ b/drivers/platform/x86/intel/pmc/core.h
> > > @@ -436,6 +436,30 @@ enum pmc_index {
> > >   	PMC_IDX_MAX
> > >   };
> > >   +/**
> > > + * struct pmc_dev_info - Structure to keep pmc device info
> > > + * @func:		Function number of the primary pmc
> > Capitalize "PMC" in the comments.
> Will change it.
> > 
> > > + * @ssram:		Bool shows if platform has ssram support
> > > + * @dmu_guid:		DMU GUID
> > > + * @regmap_list:	Pointer to a list of pmc_info structure that could be
> > > + *			available for the platform
> > > + * @map:		Pointer to a pmc_reg_map struct that contains platform
> > > + *			specific attributes of the primary pmc
> > > + * @fixup:		Function to perform platform specific fixup
> > > + * @suspend:		Function to perform platform specific suspend
> > > + * @resume:		Function to perform platform specific resume
> > > + */
> > > +struct pmc_dev_info {
> > > +	u8 func;
> > > +	bool ssram;
> > > +	u32 dmu_guid;
> > > +	struct pmc_info *regmap_list;
> > > +	const struct pmc_reg_map *map;
> > > +	void (*fixup)(void);
> > > +	void (*suspend)(struct pmc_dev *pmcdev);
> > > +	int (*resume)(struct pmc_dev *pmcdev);
> > > +};

> > > -	return tgl_core_generic_init(pmcdev, PCH_H);
> > > +	return tgl_core_generic_init(pmcdev, &tgl_pmc_dev);
> > >   }
> > > 
> > It might be also worth to consider what is stored into those
> > X86_MATCH_VFM()s so that the simple init functions could be removed
> > entirely but it could be done in another patch on top of this one.
>
> Right now we store the init function pointer for each architecture in
> X86_MATCH_VFM()
> structure. We could change it to be a pointer to the pmc_dev_info structure
> instead. Most
> of the per architecture init function call the generic_init function directly
> except for TGL
> init function. The TGL case can be handled by adding a new callback function
> pointer field named
> platform_specific and this field can also be used to replace the fixup field.

To preserve generality, the function pointer should be a real replacement 
of the generic init function (if non-NULL), not some platform specific 
hook is called from one point of the generic init function.

If the init function is NULL, just call the generic init function 
directly.

If not NULL, that per platform replacement function in can then do the 
fixup like it currently does (and whatever extra is needed) and call the 
generic init function.

-- 
 i.