| Message ID | 20181113185640.8128-1-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915/icl: reverse uninit order | expand |
On Tue, 13 Nov 2018, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > Bspec 21257 says "DDIA PHY is the comp master, so it must > not be un-initialized if other combo PHYs are in use". Here > we are shutting down all phys, so it's not strictly required. > However let's be consistent on deinitializing things in the > reversed order we initialized them. > > v2: simplify protection for enum port being unsigned in future > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > --- > drivers/gpu/drm/i915/intel_combo_phy.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_combo_phy.c b/drivers/gpu/drm/i915/intel_combo_phy.c > index 49f3a533860d..62939df048d7 100644 > --- a/drivers/gpu/drm/i915/intel_combo_phy.c > +++ b/drivers/gpu/drm/i915/intel_combo_phy.c > @@ -9,6 +9,10 @@ > for ((__port) = PORT_A; (__port) < I915_MAX_PORTS; (__port)++) \ > for_each_if(intel_port_is_combophy(__dev_priv, __port)) > > +#define for_each_combo_port_rev(__dev_priv, __port) \ The list.h list macros use _reverse suffix for reverse traversal. I think we can afford to spend the extra letters here too. BR, Jani. > + for ((__port) = I915_MAX_PORTS; (__port)-- > PORT_A;) \ > + for_each_if(intel_port_is_combophy(__dev_priv, __port)) > + > enum { > PROCMON_0_85V_DOT_0, > PROCMON_0_95V_DOT_0, > @@ -232,7 +236,7 @@ void icl_combo_phys_uninit(struct drm_i915_private *dev_priv) > { > enum port port; > > - for_each_combo_port(dev_priv, port) { > + for_each_combo_port_rev(dev_priv, port) { > u32 val; > > if (!icl_combo_phy_verify_state(dev_priv, port))
On Tue, Nov 13, 2018 at 10:31:25PM +0200, Jani Nikula wrote: > On Tue, 13 Nov 2018, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > > Bspec 21257 says "DDIA PHY is the comp master, so it must > > not be un-initialized if other combo PHYs are in use". Here > > we are shutting down all phys, so it's not strictly required. > > However let's be consistent on deinitializing things in the > > reversed order we initialized them. > > > > v2: simplify protection for enum port being unsigned in future > > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > > --- > > drivers/gpu/drm/i915/intel_combo_phy.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_combo_phy.c b/drivers/gpu/drm/i915/intel_combo_phy.c > > index 49f3a533860d..62939df048d7 100644 > > --- a/drivers/gpu/drm/i915/intel_combo_phy.c > > +++ b/drivers/gpu/drm/i915/intel_combo_phy.c > > @@ -9,6 +9,10 @@ > > for ((__port) = PORT_A; (__port) < I915_MAX_PORTS; (__port)++) \ > > for_each_if(intel_port_is_combophy(__dev_priv, __port)) > > > > +#define for_each_combo_port_rev(__dev_priv, __port) \ > > The list.h list macros use _reverse suffix for reverse traversal. I > think we can afford to spend the extra letters here too. I followed what we do on other for_each macro: for_each_power_well_rev for_each_power_domain_well_rev Should those be reverse as well? Lucas De Marchi > > BR, > Jani. > > > + for ((__port) = I915_MAX_PORTS; (__port)-- > PORT_A;) \ > > + for_each_if(intel_port_is_combophy(__dev_priv, __port)) > > + > > enum { > > PROCMON_0_85V_DOT_0, > > PROCMON_0_95V_DOT_0, > > @@ -232,7 +236,7 @@ void icl_combo_phys_uninit(struct drm_i915_private *dev_priv) > > { > > enum port port; > > > > - for_each_combo_port(dev_priv, port) { > > + for_each_combo_port_rev(dev_priv, port) { > > u32 val; > > > > if (!icl_combo_phy_verify_state(dev_priv, port)) > > -- > Jani Nikula, Intel Open Source Graphics Center
On Tue, 13 Nov 2018, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > On Tue, Nov 13, 2018 at 10:31:25PM +0200, Jani Nikula wrote: >> On Tue, 13 Nov 2018, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >> > Bspec 21257 says "DDIA PHY is the comp master, so it must >> > not be un-initialized if other combo PHYs are in use". Here >> > we are shutting down all phys, so it's not strictly required. >> > However let's be consistent on deinitializing things in the >> > reversed order we initialized them. >> > >> > v2: simplify protection for enum port being unsigned in future >> > >> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> >> > --- >> > drivers/gpu/drm/i915/intel_combo_phy.c | 6 +++++- >> > 1 file changed, 5 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_combo_phy.c b/drivers/gpu/drm/i915/intel_combo_phy.c >> > index 49f3a533860d..62939df048d7 100644 >> > --- a/drivers/gpu/drm/i915/intel_combo_phy.c >> > +++ b/drivers/gpu/drm/i915/intel_combo_phy.c >> > @@ -9,6 +9,10 @@ >> > for ((__port) = PORT_A; (__port) < I915_MAX_PORTS; (__port)++) \ >> > for_each_if(intel_port_is_combophy(__dev_priv, __port)) >> > >> > +#define for_each_combo_port_rev(__dev_priv, __port) \ >> >> The list.h list macros use _reverse suffix for reverse traversal. I >> think we can afford to spend the extra letters here too. > > I followed what we do on other for_each macro: > for_each_power_well_rev > for_each_power_domain_well_rev > > Should those be reverse as well? I think so yes. BR, Jani. > > Lucas De Marchi > >> >> BR, >> Jani. >> >> > + for ((__port) = I915_MAX_PORTS; (__port)-- > PORT_A;) \ >> > + for_each_if(intel_port_is_combophy(__dev_priv, __port)) >> > + >> > enum { >> > PROCMON_0_85V_DOT_0, >> > PROCMON_0_95V_DOT_0, >> > @@ -232,7 +236,7 @@ void icl_combo_phys_uninit(struct drm_i915_private *dev_priv) >> > { >> > enum port port; >> > >> > - for_each_combo_port(dev_priv, port) { >> > + for_each_combo_port_rev(dev_priv, port) { >> > u32 val; >> > >> > if (!icl_combo_phy_verify_state(dev_priv, port)) >> >> -- >> Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/intel_combo_phy.c b/drivers/gpu/drm/i915/intel_combo_phy.c index 49f3a533860d..62939df048d7 100644 --- a/drivers/gpu/drm/i915/intel_combo_phy.c +++ b/drivers/gpu/drm/i915/intel_combo_phy.c @@ -9,6 +9,10 @@ for ((__port) = PORT_A; (__port) < I915_MAX_PORTS; (__port)++) \ for_each_if(intel_port_is_combophy(__dev_priv, __port)) +#define for_each_combo_port_rev(__dev_priv, __port) \ + for ((__port) = I915_MAX_PORTS; (__port)-- > PORT_A;) \ + for_each_if(intel_port_is_combophy(__dev_priv, __port)) + enum { PROCMON_0_85V_DOT_0, PROCMON_0_95V_DOT_0, @@ -232,7 +236,7 @@ void icl_combo_phys_uninit(struct drm_i915_private *dev_priv) { enum port port; - for_each_combo_port(dev_priv, port) { + for_each_combo_port_rev(dev_priv, port) { u32 val; if (!icl_combo_phy_verify_state(dev_priv, port))
Bspec 21257 says "DDIA PHY is the comp master, so it must not be un-initialized if other combo PHYs are in use". Here we are shutting down all phys, so it's not strictly required. However let's be consistent on deinitializing things in the reversed order we initialized them. v2: simplify protection for enum port being unsigned in future Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> --- drivers/gpu/drm/i915/intel_combo_phy.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)