| Message ID | 1439394260-15137-6-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2015-08-12 12:44 GMT-03:00 <ville.syrjala@linux.intel.com>: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > The PORTA HPD defines are not BXT specific. They also exist on SPT, > and partially already on LPT:LP. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_irq.c | 2 +- > drivers/gpu/drm/i915/i915_reg.h | 10 +++++----- > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 8a1e35e..d12106c 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -1248,7 +1248,7 @@ static bool bxt_port_hotplug_long_detect(enum port port, u32 val) > { > switch (port) { > case PORT_A: > - return val & BXT_PORTA_HOTPLUG_LONG_DETECT; > + return val & PORTA_HOTPLUG_LONG_DETECT; > case PORT_B: > return val & PORTB_HOTPLUG_LONG_DETECT; > case PORT_C: > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index ed2d150..0e9990b 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -6002,11 +6002,11 @@ enum skl_disp_power_wells { > > /* digital port hotplug */ > #define PCH_PORT_HOTPLUG 0xc4030 /* SHOTPLUG_CTL */ > -#define BXT_PORTA_HOTPLUG_ENABLE (1 << 28) > -#define BXT_PORTA_HOTPLUG_STATUS_MASK (3 << 24) > -#define BXT_PORTA_HOTPLUG_NO_DETECT (0 << 24) > -#define BXT_PORTA_HOTPLUG_SHORT_DETECT (1 << 24) > -#define BXT_PORTA_HOTPLUG_LONG_DETECT (2 << 24) > +#define PORTA_HOTPLUG_ENABLE (1 << 28) /* LPT:LP+ & BXT */ Although the doc for LPT _suggests_ this is only for LPT:LP, it doesn't mark this bit as LPT:LP-specific just like it marks all the other LPT:LP-specific bits in every register, so I wonder if this is really LPT:LP or if there's another way to find this out, like some strap or VBT bit. Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > +#define PORTA_HOTPLUG_STATUS_MASK (3 << 24) /* SPT+ & BXT */ > +#define PORTA_HOTPLUG_NO_DETECT (0 << 24) /* SPT+ & BXT */ > +#define PORTA_HOTPLUG_SHORT_DETECT (1 << 24) /* SPT+ & BXT */ > +#define PORTA_HOTPLUG_LONG_DETECT (2 << 24) /* SPT+ & BXT */ > #define PORTD_HOTPLUG_ENABLE (1 << 20) > #define PORTD_PULSE_DURATION_2ms (0 << 18) > #define PORTD_PULSE_DURATION_4_5ms (1 << 18) > -- > 2.4.6 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Aug 26, 2015 at 04:13:52PM -0300, Paulo Zanoni wrote: > 2015-08-12 12:44 GMT-03:00 <ville.syrjala@linux.intel.com>: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > The PORTA HPD defines are not BXT specific. They also exist on SPT, > > and partially already on LPT:LP. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_irq.c | 2 +- > > drivers/gpu/drm/i915/i915_reg.h | 10 +++++----- > > 2 files changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > index 8a1e35e..d12106c 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -1248,7 +1248,7 @@ static bool bxt_port_hotplug_long_detect(enum port port, u32 val) > > { > > switch (port) { > > case PORT_A: > > - return val & BXT_PORTA_HOTPLUG_LONG_DETECT; > > + return val & PORTA_HOTPLUG_LONG_DETECT; > > case PORT_B: > > return val & PORTB_HOTPLUG_LONG_DETECT; > > case PORT_C: > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index ed2d150..0e9990b 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -6002,11 +6002,11 @@ enum skl_disp_power_wells { > > > > /* digital port hotplug */ > > #define PCH_PORT_HOTPLUG 0xc4030 /* SHOTPLUG_CTL */ > > -#define BXT_PORTA_HOTPLUG_ENABLE (1 << 28) > > -#define BXT_PORTA_HOTPLUG_STATUS_MASK (3 << 24) > > -#define BXT_PORTA_HOTPLUG_NO_DETECT (0 << 24) > > -#define BXT_PORTA_HOTPLUG_SHORT_DETECT (1 << 24) > > -#define BXT_PORTA_HOTPLUG_LONG_DETECT (2 << 24) > > +#define PORTA_HOTPLUG_ENABLE (1 << 28) /* LPT:LP+ & BXT */ > > Although the doc for LPT _suggests_ this is only for LPT:LP, it > doesn't mark this bit as LPT:LP-specific just like it marks all the > other LPT:LP-specific bits in every register, so I wonder if this is > really LPT:LP or if there's another way to find this out, like some > strap or VBT bit. Hmm. Indeed. There is that note about the enable being in the north on DevLPT:H. I guess that's what gave me the idea for this patch. But the rest of the text just talks about PCH being on the same package or not. Not sure if the two conditions are entirely the same thing. For HSW the north register says we need this on DevHSW:ULT. For BDW it refers us to a package type indication in FUSE_STRAP3, but the only relevant looking bit is the ULT mode bit, which it also says is currently unused and should be ignored :( Art, could you help us out here? How should we actually determine (on HSW/BDW) whether to enable the DDI A HPD in the north, south, or both? > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > +#define PORTA_HOTPLUG_STATUS_MASK (3 << 24) /* SPT+ & BXT */ > > +#define PORTA_HOTPLUG_NO_DETECT (0 << 24) /* SPT+ & BXT */ > > +#define PORTA_HOTPLUG_SHORT_DETECT (1 << 24) /* SPT+ & BXT */ > > +#define PORTA_HOTPLUG_LONG_DETECT (2 << 24) /* SPT+ & BXT */ > > #define PORTD_HOTPLUG_ENABLE (1 << 20) > > #define PORTD_PULSE_DURATION_2ms (0 << 18) > > #define PORTD_PULSE_DURATION_4_5ms (1 << 18) > > -- > > 2.4.6 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >> On Wed, Aug 26, 2015 at 04:13:52PM -0300, Paulo Zanoni wrote: ... >> Although the doc for LPT _suggests_ this is only for LPT:LP, it >> doesn't mark this bit as LPT:LP-specific just like it marks all the >> other LPT:LP-specific bits in every register, so I wonder if this is >> really LPT:LP or if there's another way to find this out, like some >> strap or VBT bit. > >Hmm. Indeed. There is that note about the enable being in the north >on DevLPT:H. I guess that's what gave me the idea for this patch. But >the rest of the text just talks about PCH being on the same package or >not. Not sure if the two conditions are entirely the same thing. > >For HSW the north register says we need this on DevHSW:ULT. For BDW it >refers us to a package type indication in FUSE_STRAP3, but the only >relevant looking bit is the ULT mode bit, which it also says is >currently unused and should be ignored :( > >Art, could you help us out here? How should we actually determine >(on HSW/BDW) whether to enable the DDI A HPD in the north, south, >or both? The north and south hotplug control registers have text to try to explain this in the DDI A HPD enable fields, which I pasted below. You always enable north. You additionally enable south if CPU and PCH are in the same package. "This only applies to systems that have the CPU and PCH in the same package, where the DDI A HPD input is connected to the PCH and the HPD must be enabled in both the North Display Engine Registers HOTPLUG_CTL and the South Display Engine Registers SHOTPLUG_CTL. The HPD status is found in North Display Engine Registers HOTPLUG_CTL. On systems that have the CPU and PCH in separate packages, the DDI A HPD input is connected to the CPU, and the DDI A HPD input must be enabled in only the North Display Engine Registers HOTPLUG_CTL." That fuse may not be correct on all SKUs, but I assume you have other ways to recognize what kind of package it is. I originally listed out ULT and ULX, but it became more complicated with BDW.
On Wed, Aug 26, 2015 at 09:59:13PM +0000, Runyan, Arthur J wrote: > > >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > >> On Wed, Aug 26, 2015 at 04:13:52PM -0300, Paulo Zanoni wrote: > ... > >> Although the doc for LPT _suggests_ this is only for LPT:LP, it > >> doesn't mark this bit as LPT:LP-specific just like it marks all the > >> other LPT:LP-specific bits in every register, so I wonder if this is > >> really LPT:LP or if there's another way to find this out, like some > >> strap or VBT bit. > > > >Hmm. Indeed. There is that note about the enable being in the north > >on DevLPT:H. I guess that's what gave me the idea for this patch. But > >the rest of the text just talks about PCH being on the same package or > >not. Not sure if the two conditions are entirely the same thing. > > > >For HSW the north register says we need this on DevHSW:ULT. For BDW it > >refers us to a package type indication in FUSE_STRAP3, but the only > >relevant looking bit is the ULT mode bit, which it also says is > >currently unused and should be ignored :( > > > >Art, could you help us out here? How should we actually determine > >(on HSW/BDW) whether to enable the DDI A HPD in the north, south, > >or both? > > The north and south hotplug control registers have text to try to explain this in the DDI A HPD enable fields, which I pasted below. You always enable north. You additionally enable south if CPU and PCH are in the same package. OK thanks. That more or less matches my original understanding. > > "This only applies to systems that have the CPU and PCH in the same package, where the DDI A HPD input is connected to the PCH and the HPD must be enabled in both the North Display Engine Registers HOTPLUG_CTL and the South Display Engine Registers SHOTPLUG_CTL. The HPD status is found in North Display Engine Registers HOTPLUG_CTL. > > On systems that have the CPU and PCH in separate packages, the DDI A HPD input is connected to the CPU, and the DDI A HPD input must be enabled in only the North Display Engine Registers HOTPLUG_CTL." > > That fuse may not be correct on all SKUs, but I assume you have other ways to recognize what kind of package it is. I originally listed out ULT and ULX, but it became more complicated with BDW. I'm not aware of any way of identifying the package type. My original assumption was LPT-LP -> MCP, LPT-H -> separate package, and most of the material I've managed to dig up would seem to support that. But it's hard to be sure.
On Wed, Aug 26, 2015 at 04:13:52PM -0300, Paulo Zanoni wrote: > 2015-08-12 12:44 GMT-03:00 <ville.syrjala@linux.intel.com>: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > The PORTA HPD defines are not BXT specific. They also exist on SPT, > > and partially already on LPT:LP. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_irq.c | 2 +- > > drivers/gpu/drm/i915/i915_reg.h | 10 +++++----- > > 2 files changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > index 8a1e35e..d12106c 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -1248,7 +1248,7 @@ static bool bxt_port_hotplug_long_detect(enum port port, u32 val) > > { > > switch (port) { > > case PORT_A: > > - return val & BXT_PORTA_HOTPLUG_LONG_DETECT; > > + return val & PORTA_HOTPLUG_LONG_DETECT; > > case PORT_B: > > return val & PORTB_HOTPLUG_LONG_DETECT; > > case PORT_C: > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index ed2d150..0e9990b 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -6002,11 +6002,11 @@ enum skl_disp_power_wells { > > > > /* digital port hotplug */ > > #define PCH_PORT_HOTPLUG 0xc4030 /* SHOTPLUG_CTL */ > > -#define BXT_PORTA_HOTPLUG_ENABLE (1 << 28) > > -#define BXT_PORTA_HOTPLUG_STATUS_MASK (3 << 24) > > -#define BXT_PORTA_HOTPLUG_NO_DETECT (0 << 24) > > -#define BXT_PORTA_HOTPLUG_SHORT_DETECT (1 << 24) > > -#define BXT_PORTA_HOTPLUG_LONG_DETECT (2 << 24) > > +#define PORTA_HOTPLUG_ENABLE (1 << 28) /* LPT:LP+ & BXT */ > > Although the doc for LPT _suggests_ this is only for LPT:LP, it > doesn't mark this bit as LPT:LP-specific just like it marks all the > other LPT:LP-specific bits in every register, so I wonder if this is > really LPT:LP or if there's another way to find this out, like some > strap or VBT bit. Just did a quick experiment and the bit won't stick on my desktop machine with LPT-H, but it will on a ULT with LPT-LP. So looks like that part of the patch is at least correct, unless some strap would affect whether the bit even sticks or not. > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > +#define PORTA_HOTPLUG_STATUS_MASK (3 << 24) /* SPT+ & BXT */ > > +#define PORTA_HOTPLUG_NO_DETECT (0 << 24) /* SPT+ & BXT */ > > +#define PORTA_HOTPLUG_SHORT_DETECT (1 << 24) /* SPT+ & BXT */ > > +#define PORTA_HOTPLUG_LONG_DETECT (2 << 24) /* SPT+ & BXT */ > > #define PORTD_HOTPLUG_ENABLE (1 << 20) > > #define PORTD_PULSE_DURATION_2ms (0 << 18) > > #define PORTD_PULSE_DURATION_4_5ms (1 << 18) > > -- > > 2.4.6 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >> That fuse may not be correct on all SKUs, but I assume you have other ways to >recognize what kind of package it is. I originally listed out ULT and ULX, but it >became more complicated with BDW. > >I'm not aware of any way of identifying the package type. My original >assumption was LPT-LP -> MCP, LPT-H -> separate package, and most of >the material I've managed to dig up would seem to support that. But >it's hard to be sure. That is correct.
On Thu, Aug 27, 2015 at 07:34:41PM +0000, Runyan, Arthur J wrote: > >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > >> That fuse may not be correct on all SKUs, but I assume you have other ways to > >recognize what kind of package it is. I originally listed out ULT and ULX, but it > >became more complicated with BDW. > > > >I'm not aware of any way of identifying the package type. My original > >assumption was LPT-LP -> MCP, LPT-H -> separate package, and most of > >the material I've managed to dig up would seem to support that. But > >it's hard to be sure. > > That is correct. Excellent. Thanks Art.
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 8a1e35e..d12106c 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -1248,7 +1248,7 @@ static bool bxt_port_hotplug_long_detect(enum port port, u32 val) { switch (port) { case PORT_A: - return val & BXT_PORTA_HOTPLUG_LONG_DETECT; + return val & PORTA_HOTPLUG_LONG_DETECT; case PORT_B: return val & PORTB_HOTPLUG_LONG_DETECT; case PORT_C: diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index ed2d150..0e9990b 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -6002,11 +6002,11 @@ enum skl_disp_power_wells { /* digital port hotplug */ #define PCH_PORT_HOTPLUG 0xc4030 /* SHOTPLUG_CTL */ -#define BXT_PORTA_HOTPLUG_ENABLE (1 << 28) -#define BXT_PORTA_HOTPLUG_STATUS_MASK (3 << 24) -#define BXT_PORTA_HOTPLUG_NO_DETECT (0 << 24) -#define BXT_PORTA_HOTPLUG_SHORT_DETECT (1 << 24) -#define BXT_PORTA_HOTPLUG_LONG_DETECT (2 << 24) +#define PORTA_HOTPLUG_ENABLE (1 << 28) /* LPT:LP+ & BXT */ +#define PORTA_HOTPLUG_STATUS_MASK (3 << 24) /* SPT+ & BXT */ +#define PORTA_HOTPLUG_NO_DETECT (0 << 24) /* SPT+ & BXT */ +#define PORTA_HOTPLUG_SHORT_DETECT (1 << 24) /* SPT+ & BXT */ +#define PORTA_HOTPLUG_LONG_DETECT (2 << 24) /* SPT+ & BXT */ #define PORTD_HOTPLUG_ENABLE (1 << 20) #define PORTD_PULSE_DURATION_2ms (0 << 18) #define PORTD_PULSE_DURATION_4_5ms (1 << 18)