Quoting Jani Nikula (2023-07-31 12:14:42-03:00)
>On Mon, 31 Jul 2023, Gustavo Sousa <gustavo.sousa@xxxxxxxxx> wrote:
>> Quoting Jani Nikula (2023-07-31 08:04:12-03:00)
>>>On Tue, 25 Jul 2023, Gustavo Sousa <gustavo.sousa@xxxxxxxxx> wrote:
>>>> It is possible to generalize the "disable" value for the transmitters to
>>>> be a bit mask based on the port width and the port reversal boolean,
>>>> with a small exception for DP-alt mode with "x1" port width.
>>>>
>>>> Simplify the code by using such a mask and a for-loop instead of using
>>>> switch-case statements.
>>>>
>>>> BSpec: 64539
>>>> Signed-off-by: Gustavo Sousa <gustavo.sousa@xxxxxxxxx>
>>>> ---
>>>> drivers/gpu/drm/i915/display/intel_cx0_phy.c | 79 +++++---------------
>>>> 1 file changed, 20 insertions(+), 59 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>>>> index b903ceb0b56a..f10ebdfd696a 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>>>> @@ -2604,7 +2604,8 @@ static void intel_cx0_program_phy_lane(struct drm_i915_private *i915,
>>>> struct intel_encoder *encoder, int lane_count,
>>>> bool lane_reversal)
>>>> {
>>>> - u8 l0t1, l0t2, l1t1, l1t2;
>>>> + int i;
>>>> + u8 disables;
>>>> bool dp_alt_mode = intel_tc_port_in_dp_alt_mode(enc_to_dig_port(encoder));
>>>> enum port port = encoder->port;
>>>>
>>>> @@ -2614,66 +2615,26 @@ static void intel_cx0_program_phy_lane(struct drm_i915_private *i915,
>>>> C10_VDR_CTRL_MSGBUS_ACCESS,
>>>> MB_WRITE_COMMITTED);
>>>>
>>>> - /* TODO: DP-alt MFD case where only one PHY lane should be programmed. */
>>>> - l0t1 = intel_cx0_read(i915, port, INTEL_CX0_LANE0, PHY_CX0_TX_CONTROL(1, 2));
>>>> - l0t2 = intel_cx0_read(i915, port, INTEL_CX0_LANE0, PHY_CX0_TX_CONTROL(2, 2));
>>>> - l1t1 = intel_cx0_read(i915, port, INTEL_CX0_LANE1, PHY_CX0_TX_CONTROL(1, 2));
>>>> - l1t2 = intel_cx0_read(i915, port, INTEL_CX0_LANE1, PHY_CX0_TX_CONTROL(2, 2));
>>>> -
>>>> - l0t1 |= CONTROL2_DISABLE_SINGLE_TX;
>>>> - l0t2 |= CONTROL2_DISABLE_SINGLE_TX;
>>>> - l1t1 |= CONTROL2_DISABLE_SINGLE_TX;
>>>> - l1t2 |= CONTROL2_DISABLE_SINGLE_TX;
>>>> -
>>>> - if (lane_reversal) {
>>>> - switch (lane_count) {
>>>> - case 4:
>>>> - l0t1 &= ~CONTROL2_DISABLE_SINGLE_TX;
>>>> - fallthrough;
>>>> - case 3:
>>>> - l0t2 &= ~CONTROL2_DISABLE_SINGLE_TX;
>>>> - fallthrough;
>>>> - case 2:
>>>> - l1t1 &= ~CONTROL2_DISABLE_SINGLE_TX;
>>>> - fallthrough;
>>>> - case 1:
>>>> - l1t2 &= ~CONTROL2_DISABLE_SINGLE_TX;
>>>> - break;
>>>> - default:
>>>> - MISSING_CASE(lane_count);
>>>> - }
>>>> - } else {
>>>> - switch (lane_count) {
>>>> - case 4:
>>>> - l1t2 &= ~CONTROL2_DISABLE_SINGLE_TX;
>>>> - fallthrough;
>>>> - case 3:
>>>> - l1t1 &= ~CONTROL2_DISABLE_SINGLE_TX;
>>>> - fallthrough;
>>>> - case 2:
>>>> - l0t2 &= ~CONTROL2_DISABLE_SINGLE_TX;
>>>> - l0t1 &= ~CONTROL2_DISABLE_SINGLE_TX;
>>>> - break;
>>>> - case 1:
>>>> - if (dp_alt_mode)
>>>> - l0t2 &= ~CONTROL2_DISABLE_SINGLE_TX;
>>>> - else
>>>> - l0t1 &= ~CONTROL2_DISABLE_SINGLE_TX;
>>>> - break;
>>>> - default:
>>>> - MISSING_CASE(lane_count);
>>>> - }
>>>> + if (lane_reversal)
>>>> + disables = REG_GENMASK8(3, 0) >> lane_count;
>>>> + else
>>>> + disables = REG_GENMASK8(3, 0) << lane_count;
>>>> +
>>>> + if (dp_alt_mode && lane_count == 1) {
>>>> + disables &= ~REG_GENMASK8(1, 0);
>>>> + disables |= REG_FIELD_PREP8(REG_GENMASK8(1, 0), 0x1);
>>>> }
>>>>
>>>> - /* disable MLs */
>>>> - intel_cx0_write(i915, port, INTEL_CX0_LANE0, PHY_CX0_TX_CONTROL(1, 2),
>>>> - l0t1, MB_WRITE_COMMITTED);
>>>> - intel_cx0_write(i915, port, INTEL_CX0_LANE0, PHY_CX0_TX_CONTROL(2, 2),
>>>> - l0t2, MB_WRITE_COMMITTED);
>>>> - intel_cx0_write(i915, port, INTEL_CX0_LANE1, PHY_CX0_TX_CONTROL(1, 2),
>>>> - l1t1, MB_WRITE_COMMITTED);
>>>> - intel_cx0_write(i915, port, INTEL_CX0_LANE1, PHY_CX0_TX_CONTROL(2, 2),
>>>> - l1t2, MB_WRITE_COMMITTED);
>>>> + /* TODO: DP-alt MFD case where only one PHY lane should be programmed. */
>>>> + for (i = 0; i < 4; i++) {
>>>> + int tx = i % 2 + 1;
>>>> + u8 lane_mask = i / 2 == 0 ? INTEL_CX0_LANE0 : INTEL_CX0_LANE1;
>>>
>>>I'm just catching up on mails and quickly eyeballing stuff, but
>>>
>>> i / 2 == 0
>>>
>>>looks suspect.
>>
>> i / 2 == 0 should give us the correct selection of lane_mask: the first two
>> iterations are for the first PHY lane and the last two are for the last PHY
>> lane.
>
>I think the most obvious way to express that is i < 2.
Indeed. Thanks!
--
Gustavo Sousa
>
>BR,
>Jani.
>
>>
>> --
>> Gustavo Sousa
>>
>>>
>>>BR,
>>>Jani.
>>>
>>>> +
>>>> + intel_cx0_rmw(i915, port, lane_mask, PHY_CX0_TX_CONTROL(tx, 2),
>>>> + CONTROL2_DISABLE_SINGLE_TX,
>>>> + disables & BIT(i) ? CONTROL2_DISABLE_SINGLE_TX : 0,
>>>> + MB_WRITE_COMMITTED);
>>>> + }
>>>>
>>>> if (intel_is_c10phy(i915, intel_port_to_phy(i915, port)))
>>>> intel_cx0_rmw(i915, port, INTEL_CX0_BOTH_LANES,
>>>
>>>--
>>>Jani Nikula, Intel Open Source Graphics Center
>
>--
>Jani Nikula, Intel Open Source Graphics Center
