Re: [Intel-gfx] [PATCH] drm/i915/display: Use unions per platform in intel_dpll_hw_state

["Souza, Jose" <jose.souza@intel.com>]

On Wed, 2022-02-23 at 21:58 +0200, Imre Deak wrote:
> On Tue, Feb 22, 2022 at 06:20:45AM -0800, José Roberto de Souza wrote:
> [...]
> > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> > index ba2fdfce15792..4360e1c9266d8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> > @@ -184,52 +184,74 @@ enum icl_port_dpll_id {
> >  };
> >  
> >  struct intel_dpll_hw_state {
> > -	/* i9xx, pch plls */
> > -	u32 dpll;
> > -	u32 dpll_md;
> > -	u32 fp0;
> > -	u32 fp1;
> > -
> > -	/* hsw, bdw */
> > -	u32 wrpll;
> > -	u32 spll;
> > -
> > -	/* skl */
> > -	/*
> > -	 * DPLL_CTRL1 has 6 bits for each each this DPLL. We store those in
> > -	 * lower part of ctrl1 and they get shifted into position when writing
> > -	 * the register.  This allows us to easily compare the state to share
> > -	 * the DPLL.
> > -	 */
> > -	u32 ctrl1;
> > -	/* HDMI only, 0 when used for DP */
> > -	u32 cfgcr1, cfgcr2;
> > -
> > -	/* icl */
> > -	u32 cfgcr0;
> > -
> > -	/* tgl */
> > -	u32 div0;
> > -
> > -	/* bxt */
> > -	u32 ebb0, ebb4, pll0, pll1, pll2, pll3, pll6, pll8, pll9, pll10, pcsdw12;
> > -
> > -	/*
> > -	 * ICL uses the following, already defined:
> > -	 * u32 cfgcr0, cfgcr1;
> > -	 */
> > -	u32 mg_refclkin_ctl;
> > -	u32 mg_clktop2_coreclkctl1;
> > -	u32 mg_clktop2_hsclkctl;
> > -	u32 mg_pll_div0;
> > -	u32 mg_pll_div1;
> > -	u32 mg_pll_lf;
> > -	u32 mg_pll_frac_lock;
> > -	u32 mg_pll_ssc;
> > -	u32 mg_pll_bias;
> > -	u32 mg_pll_tdc_coldst_bias;
> > -	u32 mg_pll_bias_mask;
> > -	u32 mg_pll_tdc_coldst_bias_mask;
> > +	union {
> > +		/* icl+ combo */
> > +		struct {
> > +			u32 icl_cfgcr0;
> > +			u32 icl_cfgcr1;
> > +			u32 icl_div0;
> 
> At least icl_ddi_combo_pll_get_freq() and icl_dump_hw_state() missed
> converting cfgcr1 to icl_cfgcr1. Would it be less error-prone/simpler to

Damn, yep missed that.

> store here all the skl+ combo state? That would also reduce the diff
> size.

Looks a good idea and will not make intel_dpll_hw_state increase in size.

Will send v2, thanks for the review. 

> 
> > +		};
> > +
> > +		/* icl+ TC */
> > +		struct {
> > +			u32 mg_refclkin_ctl;
> > +			u32 mg_clktop2_coreclkctl1;
> > +			u32 mg_clktop2_hsclkctl;
> > +			u32 mg_pll_div0;
> > +			u32 mg_pll_div1;
> > +			u32 mg_pll_lf;
> > +			u32 mg_pll_frac_lock;
> > +			u32 mg_pll_ssc;
> > +			u32 mg_pll_bias;
> > +			u32 mg_pll_tdc_coldst_bias;
> > +			u32 mg_pll_bias_mask;
> > +			u32 mg_pll_tdc_coldst_bias_mask;
> > +		};
> > +
> > +		/* bxt */
> > +		struct {
> > +			/* bxt */
> > +			u32 ebb0;
> > +			u32 ebb4;
> > +			u32 pll0;
> > +			u32 pll1;
> > +			u32 pll2;
> > +			u32 pll3;
> > +			u32 pll6;
> > +			u32 pll8;
> > +			u32 pll9;
> > +			u32 pll10;
> > +			u32 pcsdw12;
> > +		};
> > +
> > +		/* skl */
> > +		struct {
> > +			/*
> > +			 * DPLL_CTRL1 has 6 bits for each this DPLL. We store those in
> > +			 * lower part of ctrl1 and they get shifted into position when writing
> > +			 * the register.  This allows us to easily compare the state to share
> > +			 * the DPLL.
> > +			 */
> > +			u32 ctrl1;
> > +			u32 cfgcr1;
> > +			/* HDMI only, 0 when used for DP */
> > +			u32 cfgcr2;
> > +		};
> > +
> > +		/* hsw, bdw */
> > +		struct {
> > +			u32 wrpll;
> > +			u32 spll;
> > +		};
> > +
> > +		/* i9xx, pch plls */
> > +		struct {
> > +			u32 dpll;
> > +			u32 dpll_md;
> > +			u32 fp0;
> > +			u32 fp1;
> > +		};
> > +	};
> >  };
> >  
> >  /**
> > -- 
> > 2.35.1
> >