[Intel-gfx] [PATCH] drm/i915/vlv: assert and de-assert sideband reset at boot and resume v3

[Ville Syrjälä]

On Thu, May 22, 2014 at 09:51:22AM +0300, Imre Deak wrote:
> On Wed, 2014-05-21 at 21:43 +0300, Ville Syrjälä wrote:
> > On Wed, May 21, 2014 at 11:11:15AM -0700, Jesse Barnes wrote:
> > > And to answer more specifically...
> > > 
> > > On Wed, 21 May 2014 20:54:03 +0300
> > > Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> > > 
> > > > > +		__vlv_set_power_well(dev_priv, PUNIT_POWER_WELL_DPIO_CMN_BC,
> > > > > +				     false);
> > > > > +		__vlv_set_power_well(dev_priv, PUNIT_POWER_WELL_DPIO_CMN_BC,
> > > > > +				     true);
> > > > 
> > > > Hmm. I wonder how the power well hack in intel_uncore_sanitize() ties in
> > > > with this. We should definitely rip that out regardless.
> > > 
> > > Yeah we can rip that out.  That's just an ungate, and it assumes the
> > > BIOS has already done the reset toggle for us.
> > 
> > Well I'm assumign the system may boot/resume with the wells powered
> > down. And we definitely don't have the cri/ref clocks set up at this
> > point. So if they're needed to complete the resets the PHY may get stuck
> > or something. Also it just ungates everything at once, if the wells need
> > some ordering in bringup we have just messed things up here.
> 
> I agree, there doesn't seem to be anything that needs those power wells
> until the power domain init code runs. I can send a patch for this after
> some testing.
>  
> > > > Another thing I'm wodering is did the BIOS/hw really power on the
> > > > common lane, or did we do that outselves? If the latter, then I wonder
> > > > if we simply do that too early. Or more precisely do we need to make
> > > > sure the cri clock and/or refclock are enabled before we power on the
> > > > common lane?
> > > 
> > > Depends on the platform.  It looks like the right thing happens at boot
> > > time on most machines (i.e. the BIOS does a full toggle which causes a
> > > reset), but on suspend/resume it's up to us.  And of course on machines
> > > without video init at boot time, we need to do it ourselves as well.
> > > 
> > > I don't think the cri or refclk is required at this point, at least not
> > > if the docs we have are correct (the PHY reset is supposed to be the
> > > very first thing).
> > 
> > The timing diagrams do show some kind of relationship between the
> > cri/ref clocks and the reset signals getting deasserted.
> 
> Under "4.3 Clock Signal List" there is a hint about this saying that the
> cri and pll1 ref clocks are needed for the init time calibration of the
> whole PHY. The calibration is started by CMN reset, which should occur
> after all the needed lanes are powered on. But this matches what we do
> already now, except that toggling of the CMN reset signal is not
> guaranteed if it was already deasserted by the BIOS. This would be fixed
> by Jesse's v1 patch (but not v2).

My main issue with Jesse's patch is that it may fix the resume time
problem. But things should still fail in the exact same way if we
power gate the TX lanes and then try to bring them back. So it should
fail by simply disabling and re-enabling one port while keeping the
common lane powered up eg. by having the other port enabled all the
time.

And we don't assert cmnreset when powergating the cmnlane, so that
too may mess up the cmn lane power up since cmnreset is now
deasserted before sidereset. So even if we power down the common
lane as well, things may go bad.

> 
> > > > And third is that do we need to enable the TX wells before the CMN
> > > > well? Currently we do the opposite which could also explain why this
> > > > CMN well toggle fixes things.
> > > 
> > > I don't think that matters, but we should ask the PHY guys.  The lack
> > > of symmetry between the gate and ungate bothers me too.
> > 
> > My theory here is that the cmn and/or side reset needs to propagate to
> > the data lanes to propery initialize them. If the lanes are powered down
> > when the reset occurs things go bad.
> 
> I agree that the CMN reset should happen after all the lanes are powered
> on, but this is what we do already now, except guaranteeing that it
> toggles. It's not clear what triggers the side reset signal, I assume
> Punit deasserts it after powering on the CMN lane. Also not clear if
> this really needs to happen after powering on the TX lanes. 
> 
> > And looks like the "Dynamic Power Gating" section in the phy cspec
> > pretty much says as much. That is you need to fully reset the entire
> > phy if you want to ungate even one of the blocks.
> 
> What does full reset mean? In my opinion toggling the CMN reset line is
> enough, but I guess you suggest that we also need to power off/on the
> CMN lane first. It would be good if Jesse could try if toggling CMN
> reset alone is enough to get rid of the problem he saw.

IIRC the docs said sidereset needs to be reasserted too. And AFAICS the
only way to do that is by toggling the power to the common lane.

> 
> > So based on that I think pretty much everything I said is needed to make
> > stuff work correctly.
> 
> I'd be also happy to get some clarification from the HW people about our
> assumptions, but your changes look sensible anyway, since handling the
> CMN reset and the cri/ref clocks do belong logically to the power well
> code.
> 
> > Also I think currently we power gate the TX blocks
> > whenever the port gets disabled. If we want to keep doing that, we need
> > to adjust valleyview_modeset_global_resources() to fully reset the phy
> > whenever previously power down wells need to be brought up. The
> > alternative would be to model the entire PHY as a single power well,
> > which would definitely be the simpler option.
> 
> Yes this is correct and it needs fixing. I took it for granted that we
> can turn on/off parts of the PHY independently from the rest of it, but
> now it looks like it's not possible on VLV. (As I understand it's
> possible on CHV.) I planned to come up with some fix after figuring out
> how things would work with fastboot, where based on this constraint we
> also have to avoid disabling/enabling any PHY lanes during booting.
> Currently we enable all the lanes at that time. I guess we could just
> leave the PHY configuration as-is until we read out the HW configuration
> and then leave the PHY config unchanged if there are active DP/HDMI/VGA
> outputs and disable the whole PHY otherwise.
> 
> --Imre

-- 
Ville Syrjälä
Intel OTC