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