[PATCHv2 03/22] drm/bridge: tc358767: fix ansi 8b10b use

[Andrey Gusakov]

Hi Laurent,

On Tue, Apr 23, 2019 at 5:56 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Andrey,
>
> On Tue, Apr 23, 2019 at 11:19:17AM +0300, Andrey Gusakov wrote:
> > On Sun, Apr 21, 2019 at 12:14 AM Laurent Pinchart wrote:
> > > On Tue, Mar 26, 2019 at 12:31:27PM +0200, Tomi Valkeinen wrote:
> > >> DP always uses ANSI 8B10B encoding. Some monitors (old?) may not have
> > >> the ANSI 8B10B bit set in DPCD, even if it should always be set.
> > >
> > > Makes you wonder why the bit is present :-) I've checked the DP v1.0
> > > specification, and even though the bit isn't documented as being always
> > > 1, 8B/10B encoding is mandatory, so this should be safe from a DP point
> > > of view.
> > >
> > > Without access to the TC358767 datasheet I can't tell what use cases
> > > were intended for disabling 8B/10B encoding. Could it be related to
> > > video sources that already provide X3.230-1994 encoded data ? In any
> > > case this shouldn't be driven by the sink but by the source, so
> >
> > Datasheet only describes this bit in register, without any additional
> > information how it should be handled.
> >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > >
> > > Andrey, as this feature was present in the initial driver version that
> > > you authored, do you have more information about its intended use cases
> > > ?
> >
> > During initial driver development I had one eDP display that reports 0 in Bit 0
> > (ANSI 8B/10B) of DPCD reg 0x0006 (MAIN_LINK_CHANNEL_CODING).
> > Also it does not react on setting Bit 0 (SET_ANSI 8B10B) in 0x0108
> > (MAIN_LINK_CHANNEL_CODING_SET) - after reading back it was 0 again.
> > So I had to disable 8B10 encoding on tc358767 side to make this display
> > work.
>
> Out of curiosity, how does the eDP display recover the clock without
> 8B/10B encoding ?
Sorry, I have no much knowledge how it works on phy level.
I thought additional 2 bit are used for DC balancing only.

Andrey.
>
> > On other hand if there are displays that report zero bit 0 in
> > MAIN_LINK_CHANNEL_CODING while needing 8b10b then this patch looks
> > reasonable.
> >
> > May be driver should read back MAIN_LINK_CHANNEL_CODING_SET
> > register after setting it and check if 8b10b actually enabled?
>
> This sounds like a reasonable thing to try. Tomi, do you still have
> accesss to those faulty monitors ?
>
> > >> The tc358767 driver currently respects that flag, and turns the encoding
> > >> off if the monitor does not have the bit set, which then results in the
> > >> monitor not working.
> > >>
> > >> This patch makes the driver to always use ANSI 8B10B encoding, and drops
> > >> the 'coding8b10b' field which is no longer used.
> > >>
> > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ti.com>
> > >> ---
> > >>  drivers/gpu/drm/bridge/tc358767.c | 11 +++--------
> > >>  1 file changed, 3 insertions(+), 8 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > >> index 11a50f7bb4be..163c594fa6ac 100644
> > >> --- a/drivers/gpu/drm/bridge/tc358767.c
> > >> +++ b/drivers/gpu/drm/bridge/tc358767.c
> > >> @@ -188,7 +188,6 @@ struct tc_edp_link {
> > >>       u8                      assr;
> > >>       int                     scrambler_dis;
> > >>       int                     spread;
> > >> -     int                     coding8b10b;
> > >>       u8                      swing;
> > >>       u8                      preemp;
> > >>  };
> > >> @@ -390,13 +389,10 @@ static u32 tc_srcctrl(struct tc_data *tc)
> > >>        * No training pattern, skew lane 1 data by two LSCLK cycles with
> > >>        * respect to lane 0 data, AutoCorrect Mode = 0
> > >>        */
> > >> -     u32 reg = DP0_SRCCTRL_NOTP | DP0_SRCCTRL_LANESKEW;
> > >> +     u32 reg = DP0_SRCCTRL_NOTP | DP0_SRCCTRL_LANESKEW | DP0_SRCCTRL_EN810B;
> > >>
> > >>       if (tc->link.scrambler_dis)
> > >>               reg |= DP0_SRCCTRL_SCRMBLDIS;   /* Scrambler Disabled */
> > >> -     if (tc->link.coding8b10b)
> > >> -             /* Enable 8/10B Encoder (TxData[19:16] not used) */
> > >> -             reg |= DP0_SRCCTRL_EN810B;
> > >>       if (tc->link.spread)
> > >>               reg |= DP0_SRCCTRL_SSCG;        /* Spread Spectrum Enable */
> > >>       if (tc->link.base.num_lanes == 2)
> > >> @@ -635,7 +631,7 @@ static int tc_get_display_props(struct tc_data *tc)
> > >>       ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, tmp);
> > >>       if (ret < 0)
> > >>               goto err_dpcd_read;
> > >> -     tc->link.coding8b10b = tmp[0] & BIT(0);
> > >> +
> > >>       tc->link.scrambler_dis = 0;
> > >>       /* read assr */
> > >>       ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, tmp);
> > >> @@ -649,7 +645,6 @@ static int tc_get_display_props(struct tc_data *tc)
> > >>               tc->link.base.num_lanes,
> > >>               (tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ?
> > >>               "enhanced" : "non-enhanced");
> > >> -     dev_dbg(tc->dev, "ANSI 8B/10B: %d\n", tc->link.coding8b10b);
> > >>       dev_dbg(tc->dev, "Display ASSR: %d, TC358767 ASSR: %d\n",
> > >>               tc->link.assr, tc->assr);
> > >>
> > >> @@ -951,7 +946,7 @@ static int tc_main_link_setup(struct tc_data *tc)
> > >>       /* DOWNSPREAD_CTRL */
> > >>       tmp[0] = tc->link.spread ? DP_SPREAD_AMP_0_5 : 0x00;
> > >>       /* MAIN_LINK_CHANNEL_CODING_SET */
> > >> -     tmp[1] =  tc->link.coding8b10b ? DP_SET_ANSI_8B10B : 0x00;
> > >> +     tmp[1] =  DP_SET_ANSI_8B10B;
> > >>       ret = drm_dp_dpcd_write(aux, DP_DOWNSPREAD_CTRL, tmp, 2);
> > >>       if (ret < 0)
> > >>               goto err_dpcd_write;
>
> --
> Regards,
>
> Laurent Pinchart