| Message ID | 20181113024503.28886-2-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] drm/i915/icl: replace check for combo phy | expand |
On Mon, Nov 12, 2018 at 06:45:03PM -0800, Lucas De Marchi 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. > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > --- > drivers/gpu/drm/i915/intel_combo_phy.c | 8 +++++++- > 1 file changed, 7 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..9c06be45b84e 100644 > --- a/drivers/gpu/drm/i915/intel_combo_phy.c > +++ b/drivers/gpu/drm/i915/intel_combo_phy.c > @@ -9,6 +9,12 @@ > 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 - 1; \ > + (__port) >= PORT_A && (__port) < I915_MAX_PORTS; \ > + (__port)--) \ Heh, so 'enum port' is unsigned. Surely I would have get this right only after seeing it fail with only the >= condition :) An alternative: for ((__port) = I915_MAX_PORTS; (__port)-- > 0;) or add a negative value, like INVALID_PORT=-1 to 'enum port', but we'd need some user for that too. Either way: Reviewed-by: Imre Deak <imre.deak@intel.com> > + for_each_if(intel_port_is_combophy(__dev_priv, __port)) > + > enum { > PROCMON_0_85V_DOT_0, > PROCMON_0_95V_DOT_0, > @@ -232,7 +238,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)) > -- > 2.19.1.1.g56c4683e68 >
On Tue, Nov 13, 2018 at 03:19:06PM +0200, Imre Deak wrote: > On Mon, Nov 12, 2018 at 06:45:03PM -0800, Lucas De Marchi 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. > > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > > --- > > drivers/gpu/drm/i915/intel_combo_phy.c | 8 +++++++- > > 1 file changed, 7 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..9c06be45b84e 100644 > > --- a/drivers/gpu/drm/i915/intel_combo_phy.c > > +++ b/drivers/gpu/drm/i915/intel_combo_phy.c > > @@ -9,6 +9,12 @@ > > 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 - 1; \ > > + (__port) >= PORT_A && (__port) < I915_MAX_PORTS; \ > > + (__port)--) \ > > Heh, so 'enum port' is unsigned. Surely I would have get this right > only after seeing it fail with only the >= condition :) An > alternative: > > for ((__port) = I915_MAX_PORTS; (__port)-- > 0;) > > or add a negative value, like INVALID_PORT=-1 to 'enum port', but we'd Hm, actually we do have PORT_NONE=-1, in which case 'enum port' is a signed int. So why do we need (__port) < I915_MAX_PORTS ? > need some user for that too. Either way: > > Reviewed-by: Imre Deak <imre.deak@intel.com> > > > + for_each_if(intel_port_is_combophy(__dev_priv, __port)) > > + > > enum { > > PROCMON_0_85V_DOT_0, > > PROCMON_0_95V_DOT_0, > > @@ -232,7 +238,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)) > > -- > > 2.19.1.1.g56c4683e68 > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Nov 13, 2018 at 03:23:07PM +0200, Imre Deak wrote: > On Tue, Nov 13, 2018 at 03:19:06PM +0200, Imre Deak wrote: > > On Mon, Nov 12, 2018 at 06:45:03PM -0800, Lucas De Marchi 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. > > > > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_combo_phy.c | 8 +++++++- > > > 1 file changed, 7 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..9c06be45b84e 100644 > > > --- a/drivers/gpu/drm/i915/intel_combo_phy.c > > > +++ b/drivers/gpu/drm/i915/intel_combo_phy.c > > > @@ -9,6 +9,12 @@ > > > 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 - 1; \ > > > + (__port) >= PORT_A && (__port) < I915_MAX_PORTS; \ > > > + (__port)--) \ > > > > Heh, so 'enum port' is unsigned. Surely I would have get this right > > only after seeing it fail with only the >= condition :) An > > alternative: > > > > for ((__port) = I915_MAX_PORTS; (__port)-- > 0;) > > > > or add a negative value, like INVALID_PORT=-1 to 'enum port', but we'd > > Hm, actually we do have PORT_NONE=-1, in which case 'enum port' is a > signed int. So why do we need (__port) < I915_MAX_PORTS ? We don't actually *need*. I only thought the fact that the type is not explicit but rather depends on the enum value, it would be to fragile and it would start to fail silently if we ever change that. I like the alternative you gave since it's shorter and works on both cases. thanks Lucas De Marchi > > > need some user for that too. Either way: > > > > Reviewed-by: Imre Deak <imre.deak@intel.com> > > > > > + for_each_if(intel_port_is_combophy(__dev_priv, __port)) > > > + > > > enum { > > > PROCMON_0_85V_DOT_0, > > > PROCMON_0_95V_DOT_0, > > > @@ -232,7 +238,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)) > > > -- > > > 2.19.1.1.g56c4683e68 > > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_combo_phy.c b/drivers/gpu/drm/i915/intel_combo_phy.c index 49f3a533860d..9c06be45b84e 100644 --- a/drivers/gpu/drm/i915/intel_combo_phy.c +++ b/drivers/gpu/drm/i915/intel_combo_phy.c @@ -9,6 +9,12 @@ 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 - 1; \ + (__port) >= PORT_A && (__port) < I915_MAX_PORTS; \ + (__port)--) \ + for_each_if(intel_port_is_combophy(__dev_priv, __port)) + enum { PROCMON_0_85V_DOT_0, PROCMON_0_95V_DOT_0, @@ -232,7 +238,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. Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> --- drivers/gpu/drm/i915/intel_combo_phy.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)