Cc: Imre, I think you were involved in adding the checks.
BR,
Jani.
On Tue, 05 Oct 2021, "Nautiyal, Ankit K" <ankit.k.nautiyal@xxxxxxxxx> wrote:
> On 10/5/2021 1:34 PM, Jani Nikula wrote:
>> On Tue, 05 Oct 2021, Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx> wrote:
>>> The low voltage sku check can be ignored as OEMs need to consider that
>>> when designing the board and then put any limits in VBT.
>> "can" or "must"?
>>
>> VBT has been notoriously buggy over the years, and we need to safeguard
>> against that. Are there any cases where having these checks are wrong?
>
> Hi Jani,
>
> Bspec page for Combo PHY PLL frequencies now says "OEM must use VBT to
> specify a maximum that is tolerated by the board design" for the rates
> above 5.4G.
>
> Earlier it was mentioned that rates > 5.4G were supported on SKUs with
> Higher I/O Voltage.
>
> There was an instance where on an ADL-S board, where VBT was showing as
> HBR3 supporting for a combo phy port, but we were reading the IO
> voltage as 0.85V in is_low_voltage_sku()
>
> (Specifically, we were reading Register_PORT_COMP_DW3 bits 24-25 as 0)
> for a combo PHY port, and therefore we were limiting the BW to 5.4Gbps
>
> Due to this, 8k@60 mode was getting pruned on the board for that combo
> phy port. On removing the low_voltage_sku( ) the mode was able to be set
> properly.
>
> Incidentally, with Windows 8k@60 was also coming up on the same board on
> same port.
>
> So I had checked with HW team and GOP/VBT team if driver should consider
> the low voltage sku check. As per their response we 'can' ignore the
> check and rely on the VBT, as OEM should limit the rate as per board
> design. The Bspec was also updated to reflect the same.
>
> So IMHO we need not limit the rate as per is_low_voltage_sku check, as
> this limiting of the rate through VBT is a must for the OEMs.
>
> I should perhaps change the wording of the commit message to convey the
> same.
>
>
> Thanks & Regards,
>
> Ankit
>
>
>>
>> BR,
>> Jani.
>>
>>> Same is now changed in Bspec (53720).
>>>
>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
>>> ---
>>> drivers/gpu/drm/i915/display/intel_dp.c | 32 +++----------------------
>>> 1 file changed, 3 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>>> index 74a657ae131a..75c364c3c88e 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>>> @@ -297,23 +297,13 @@ static int dg2_max_source_rate(struct intel_dp *intel_dp)
>>> return intel_dp_is_edp(intel_dp) ? 810000 : 1350000;
>>> }
>>>
>>> -static bool is_low_voltage_sku(struct drm_i915_private *i915, enum phy phy)
>>> -{
>>> - u32 voltage;
>>> -
>>> - voltage = intel_de_read(i915, ICL_PORT_COMP_DW3(phy)) & VOLTAGE_INFO_MASK;
>>> -
>>> - return voltage == VOLTAGE_INFO_0_85V;
>>> -}
>>> -
>>> static int icl_max_source_rate(struct intel_dp *intel_dp)
>>> {
>>> struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>>> struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
>>> enum phy phy = intel_port_to_phy(dev_priv, dig_port->base.port);
>>>
>>> - if (intel_phy_is_combo(dev_priv, phy) &&
>>> - (is_low_voltage_sku(dev_priv, phy) || !intel_dp_is_edp(intel_dp)))
>>> + if (intel_phy_is_combo(dev_priv, phy) && !intel_dp_is_edp(intel_dp))
>>> return 540000;
>>>
>>> return 810000;
>>> @@ -321,23 +311,7 @@ static int icl_max_source_rate(struct intel_dp *intel_dp)
>>>
>>> static int ehl_max_source_rate(struct intel_dp *intel_dp)
>>> {
>>> - struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>>> - struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
>>> - enum phy phy = intel_port_to_phy(dev_priv, dig_port->base.port);
>>> -
>>> - if (intel_dp_is_edp(intel_dp) || is_low_voltage_sku(dev_priv, phy))
>>> - return 540000;
>>> -
>>> - return 810000;
>>> -}
>>> -
>>> -static int dg1_max_source_rate(struct intel_dp *intel_dp)
>>> -{
>>> - struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>>> - struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
>>> - enum phy phy = intel_port_to_phy(i915, dig_port->base.port);
>>> -
>>> - if (intel_phy_is_combo(i915, phy) && is_low_voltage_sku(i915, phy))
>>> + if (intel_dp_is_edp(intel_dp))
>>> return 540000;
>>>
>>> return 810000;
>>> @@ -380,7 +354,7 @@ intel_dp_set_source_rates(struct intel_dp *intel_dp)
>>> max_rate = dg2_max_source_rate(intel_dp);
>>> else if (IS_ALDERLAKE_P(dev_priv) || IS_ALDERLAKE_S(dev_priv) ||
>>> IS_DG1(dev_priv) || IS_ROCKETLAKE(dev_priv))
>>> - max_rate = dg1_max_source_rate(intel_dp);
>>> + max_rate = 810000;
>>> else if (IS_JSL_EHL(dev_priv))
>>> max_rate = ehl_max_source_rate(intel_dp);
>>> else
--
Jani Nikula, Intel Open Source Graphics Center
