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

[Imre Deak]

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
store here all the skl+ combo state? That would also reduce the diff
size.

> +		};
> +
> +		/* 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
>