Re: [PATCH] drm/i915/skl: Init cdclk in the driver rather than relying on pre-os

["Kumar, Shobhit" <shobhit.kumar@linux.intel.com>]

On 10/06/2015 04:11 PM, Imre Deak wrote:
> On ti, 2015-10-06 at 15:26 +0530, Kumar, Shobhit wrote:
>> On 10/05/2015 09:05 PM, Imre Deak wrote:
>>> On ma, 2015-10-05 at 20:52 +0530, Shobhit Kumar wrote:
>>>> Mostly reuse what is programmed by pre-os, but in case there is no
>>>> pre-os initialization, init the cdclk with the default value.
>>>>
>>>> Cc: Imre Deak <imre.deak@intel.com>
>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_ddi.c | 6 ++----
>>>>    1 file changed, 2 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>>> index 2d3cc82..675c60d 100644
>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>> @@ -2947,10 +2947,8 @@ void intel_ddi_pll_init(struct drm_device *dev)
>>>>
>>>>    		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
>>>>    		dev_priv->skl_boot_cdclk = cdclk_freq;
>>>> -		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
>>>> -			DRM_ERROR("LCPLL1 is disabled\n");
>>>> -		else
>>>> -			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>>>> +
>>>> +		skl_init_cdclk(dev_priv);
>>>
>>> How does this prevent changing the clock if BIOS did enable some output?
>>> We shouldn't change the clock in that case.
>>
>> In that case it will try to re-apply the same clock that BIOS enabled.
>> Not sure if this is allowed, but I checked the cdclock change sequence
>> and it is mostly followed in skl_init_cdclk.
>> In my tests where BIOS does enable this, I faced no issues in
>> initializing again in driver.
>
> The first step in that sequence:
> "Disable all display engine functions using the full mode set disable
> sequence on all pipes, ports, and planes."

Oh, yeah, I again made mistake of assuming that display is not enabled 
in the first place. You are right, though it works if I change the clock 
again.

>
> So the problem is not that the PLL itself may be enabled here (as BIOS
> left it), but that some output is also enabled.

Yes.

>
>> I have noticed on some pre-os this value is programmed correctly except
>> for the decimal part. That causes AUX transactions to fail on SKl. That
>> is what triggered this patch actually. So other way is to completely
>> validate the value in get_display_clock_speed instead of bit[28:26] and
>> then if wrong then only do the cdclk init.
>
> I think we'd need to detect at this point if outputs are enabled and
> only attempt to work around the above BIOS problem if this is not the
> case. Alternatively you could also disable the active outputs as a first
> step.

Ok, let me detect if any output is enabled by BIOS and accordingly 
initialize cdclk.

Regards
Shobhit

>
>>
>> Regards
>> Shobhit
>>
>>>
>>>>    	} else if (IS_BROXTON(dev)) {
>>>>    		broxton_init_cdclk(dev);
>>>>    		broxton_ddi_phy_init(dev);
>>>
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx