| Message ID | 20240506-tc358775-fix-powerup-v1-9-545dcf00b8dd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: tc358775: proper bridge bringup and code cleanup | expand |
Hello Michael, On Mon, May 6, 2024 at 3:35 PM Michael Walle <mwalle@kernel.org> wrote: > > To cite the datasheet on VSDELAY: > During DSI link speed is slower than that of LVDS link’s, data needs > to be buffer within 775XBG before outputting to prevent data from > underflow. Register field VPCTRL[VSDELAY] is used to for this purpose > > This driver assumes that the DSI link speed is the pixel clock (as does > every DSI bridge driver), after all the LVDS clock is derived from the > DSI clock. Thus we know for a fact, that the DSI link is not slower than > the LVDS side. Just use the (sane) default value of the bridge and drop > the complicated calculation here. I am not convinced this is a good idea to revert to a default VSdelay value. I tested your patch series with RK3399 platform and default value (5) was not enough there. There was small data underflow visible, resulting in display offset. Removing this patch and using the original calculation formula fixed the problem. The calculated VSDELAY value seems to be a lot bigger than required, but keeps us on the safe side. It looks that hback-porch value for panel is used also on DSI link, effectively delaying hactive data delivered to TC358775 bridge. I suspect this causes the requirement for higher VSDELAY. > > While at it, replace the TC358775_VPCTRL_MSF() and > TC358775_VPCTRL_OPXLFMT() inline functions by the usual macros for a bit > flag. > > Signed-off-by: Michael Walle <mwalle@kernel.org> > --- > drivers/gpu/drm/bridge/tc358775.c | 49 +++++++-------------------------------- > 1 file changed, 8 insertions(+), 41 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c > index 54aea58a3406..a9d731e87970 100644 > --- a/drivers/gpu/drm/bridge/tc358775.c > +++ b/drivers/gpu/drm/bridge/tc358775.c > @@ -109,7 +109,9 @@ > #define RDPKTLN 0x0404 /* Command Read Packet Length */ > > #define VPCTRL 0x0450 /* Video Path Control */ > -#define EVTMODE BIT(5) /* Video event mode enable, tc35876x only */ > +#define VPCTRL_MSF BIT(0) > +#define VPCTRL_OPXLFMT BIT(8) > +#define VPCTRL_EVTMODE BIT(5) /* Video event mode enable, tc35876x only */ > #define HTIM1 0x0454 /* Horizontal Timing Control 1 */ > #define HTIM2 0x0458 /* Horizontal Timing Control 2 */ > #define VTIM1 0x045C /* Vertical Timing Control 1 */ > @@ -187,30 +189,6 @@ enum { > > #define L0EN BIT(1) > > -#define TC358775_VPCTRL_VSDELAY__MASK 0x3FF00000 > -#define TC358775_VPCTRL_VSDELAY__SHIFT 20 > -static inline u32 TC358775_VPCTRL_VSDELAY(uint32_t val) > -{ > - return ((val) << TC358775_VPCTRL_VSDELAY__SHIFT) & > - TC358775_VPCTRL_VSDELAY__MASK; > -} > - > -#define TC358775_VPCTRL_OPXLFMT__MASK 0x00000100 > -#define TC358775_VPCTRL_OPXLFMT__SHIFT 8 > -static inline u32 TC358775_VPCTRL_OPXLFMT(uint32_t val) > -{ > - return ((val) << TC358775_VPCTRL_OPXLFMT__SHIFT) & > - TC358775_VPCTRL_OPXLFMT__MASK; > -} > - > -#define TC358775_VPCTRL_MSF__MASK 0x00000001 > -#define TC358775_VPCTRL_MSF__SHIFT 0 > -static inline u32 TC358775_VPCTRL_MSF(uint32_t val) > -{ > - return ((val) << TC358775_VPCTRL_MSF__SHIFT) & > - TC358775_VPCTRL_MSF__MASK; > -} > - > #define TC358775_LVCFG_PCLKDIV__MASK 0x000000f0 > #define TC358775_LVCFG_PCLKDIV__SHIFT 4 > static inline u32 TC358775_LVCFG_PCLKDIV(uint32_t val) > @@ -350,7 +328,6 @@ static void tc_bridge_enable(struct drm_bridge *bridge) > u32 hback_porch, hsync_len, hfront_porch, hactive, htime1, htime2; > u32 vback_porch, vsync_len, vfront_porch, vactive, vtime1, vtime2; > unsigned int val = 0; > - u16 dsiclk, clkdiv, byteclk, t1, t2, t3, vsdelay; > struct drm_display_mode *mode; > struct drm_connector *connector = get_connector(bridge->encoder); > > @@ -398,27 +375,17 @@ static void tc_bridge_enable(struct drm_bridge *bridge) > > /* Video event mode vs pulse mode bit, does not exist for tc358775 */ > if (tc->type == TC358765) > - val = EVTMODE; > + val = VPCTRL_EVTMODE; > else > val = 0; > > if (tc->bpc == 8) > - val |= TC358775_VPCTRL_OPXLFMT(1); > + val |= VPCTRL_OPXLFMT; > else /* bpc = 6; */ > - val |= TC358775_VPCTRL_MSF(1); > - > - dsiclk = mode->crtc_clock * 3 * tc->bpc / tc->num_dsi_lanes / 1000; > - clkdiv = dsiclk / (tc->lvds_link == DUAL_LINK ? DIVIDE_BY_6 : DIVIDE_BY_3); > - byteclk = dsiclk / 4; > - t1 = hactive * (tc->bpc * 3 / 8) / tc->num_dsi_lanes; > - t2 = ((100000 / clkdiv)) * (hactive + hback_porch + hsync_len + hfront_porch) / 1000; > - t3 = ((t2 * byteclk) / 100) - (hactive * (tc->bpc * 3 / 8) / > - tc->num_dsi_lanes); > - > - vsdelay = (clkdiv * (t1 + t3) / byteclk) - hback_porch - hsync_len - hactive; > + val |= VPCTRL_MSF; > > - val |= TC358775_VPCTRL_VSDELAY(vsdelay); > - regmap_write(tc->regmap, VPCTRL, val); > + regmap_update_bits(tc->regmap, VPCTRL, val, > + VPCTRL_OPXLFMT | VPCTRL_MSF | VPCTRL_EVTMODE); > > regmap_write(tc->regmap, HTIM1, htime1); > regmap_write(tc->regmap, VTIM1, vtime1); > > -- > 2.39.2 > Kind regards Daniel
Hi Daniel, > > To cite the datasheet on VSDELAY: > > During DSI link speed is slower than that of LVDS link’s, data needs > > to be buffer within 775XBG before outputting to prevent data from > > underflow. Register field VPCTRL[VSDELAY] is used to for this purpose > > > > This driver assumes that the DSI link speed is the pixel clock (as does > > every DSI bridge driver), after all the LVDS clock is derived from the > > DSI clock. Thus we know for a fact, that the DSI link is not slower than > > the LVDS side. Just use the (sane) default value of the bridge and drop > > the complicated calculation here. > > I am not convinced this is a good idea to revert to a default > VSdelay value. I tested your patch series with RK3399 platform > and default value (5) was not enough there. There was small data > underflow visible, resulting in display offset. Removing this patch > and using the original calculation formula fixed the problem. > The calculated VSDELAY value seems to be a lot bigger than required, > but keeps us on the safe side. Did you use just parts of this series or did you port the "lp11 notify" mechanism to the rk3399 platform? Please keep in mind, that this bridge doesn't really work if the reset isn't deasserted during lp-11 mode and lots of odd things happen. Also, do you know if you have an EEPROM attached to the bridge or does any firmware part initialize that bridge? > It looks that hback-porch value for panel is used also on DSI link, > effectively delaying hactive data delivered to TC358775 bridge. > I suspect this causes the requirement for higher VSDELAY. It was ages ago since I've worked on this bridge and extensively tested and even measured and decoded the DSI link and the LVDS stream. But IIRC this delay was only to compensate the difference between the DSI clock and the LVDS clock, that is, if you push the pixel stream slower into the bridge than the bridge is pushing it out to the LVDS panel. So the back porch should be irrelevant here (?!). -michael
On Fri, Sep 6, 2024 at 4:34 PM Michael Walle <mwalle@kernel.org> wrote: > > Hi Daniel, > > > > To cite the datasheet on VSDELAY: > > > During DSI link speed is slower than that of LVDS link’s, data needs > > > to be buffer within 775XBG before outputting to prevent data from > > > underflow. Register field VPCTRL[VSDELAY] is used to for this purpose > > > > > > This driver assumes that the DSI link speed is the pixel clock (as does > > > every DSI bridge driver), after all the LVDS clock is derived from the > > > DSI clock. Thus we know for a fact, that the DSI link is not slower than > > > the LVDS side. Just use the (sane) default value of the bridge and drop > > > the complicated calculation here. > > > > I am not convinced this is a good idea to revert to a default > > VSdelay value. I tested your patch series with RK3399 platform > > and default value (5) was not enough there. There was small data > > underflow visible, resulting in display offset. Removing this patch > > and using the original calculation formula fixed the problem. > > The calculated VSDELAY value seems to be a lot bigger than required, > > but keeps us on the safe side. > > Did you use just parts of this series or did you port the "lp11 > notify" mechanism to the rk3399 platform? Please keep in mind, that > this bridge doesn't really work if the reset isn't deasserted during > lp-11 mode and lots of odd things happen. I pulled in your complete series and ported "lp11 notify" to RK3399. Both reset and standby pins are connected to the bridge on my board. > > Also, do you know if you have an EEPROM attached to the bridge or > does any firmware part initialize that bridge? The initialization is done only by Linux kernel driver with your patches. > > > It looks that hback-porch value for panel is used also on DSI link, > > effectively delaying hactive data delivered to TC358775 bridge. > > I suspect this causes the requirement for higher VSDELAY. > > It was ages ago since I've worked on this bridge and extensively > tested and even measured and decoded the DSI link and the LVDS > stream. But IIRC this delay was only to compensate the difference > between the DSI clock and the LVDS clock, that is, if you push the > pixel stream slower into the bridge than the bridge is pushing it > out to the LVDS panel. > > So the back porch should be irrelevant here (?!). That was also my initial understanding, but for some reason original algorithm for VSDELAY in tc358775 driver, takes blanking period into calculation. It does not base only on DSI and LVDS clocks. Additionally, my experiments show that the default value is too low in my configuration. > > -michael Kind regards Daniel
diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c index 54aea58a3406..a9d731e87970 100644 --- a/drivers/gpu/drm/bridge/tc358775.c +++ b/drivers/gpu/drm/bridge/tc358775.c @@ -109,7 +109,9 @@ #define RDPKTLN 0x0404 /* Command Read Packet Length */ #define VPCTRL 0x0450 /* Video Path Control */ -#define EVTMODE BIT(5) /* Video event mode enable, tc35876x only */ +#define VPCTRL_MSF BIT(0) +#define VPCTRL_OPXLFMT BIT(8) +#define VPCTRL_EVTMODE BIT(5) /* Video event mode enable, tc35876x only */ #define HTIM1 0x0454 /* Horizontal Timing Control 1 */ #define HTIM2 0x0458 /* Horizontal Timing Control 2 */ #define VTIM1 0x045C /* Vertical Timing Control 1 */ @@ -187,30 +189,6 @@ enum { #define L0EN BIT(1) -#define TC358775_VPCTRL_VSDELAY__MASK 0x3FF00000 -#define TC358775_VPCTRL_VSDELAY__SHIFT 20 -static inline u32 TC358775_VPCTRL_VSDELAY(uint32_t val) -{ - return ((val) << TC358775_VPCTRL_VSDELAY__SHIFT) & - TC358775_VPCTRL_VSDELAY__MASK; -} - -#define TC358775_VPCTRL_OPXLFMT__MASK 0x00000100 -#define TC358775_VPCTRL_OPXLFMT__SHIFT 8 -static inline u32 TC358775_VPCTRL_OPXLFMT(uint32_t val) -{ - return ((val) << TC358775_VPCTRL_OPXLFMT__SHIFT) & - TC358775_VPCTRL_OPXLFMT__MASK; -} - -#define TC358775_VPCTRL_MSF__MASK 0x00000001 -#define TC358775_VPCTRL_MSF__SHIFT 0 -static inline u32 TC358775_VPCTRL_MSF(uint32_t val) -{ - return ((val) << TC358775_VPCTRL_MSF__SHIFT) & - TC358775_VPCTRL_MSF__MASK; -} - #define TC358775_LVCFG_PCLKDIV__MASK 0x000000f0 #define TC358775_LVCFG_PCLKDIV__SHIFT 4 static inline u32 TC358775_LVCFG_PCLKDIV(uint32_t val) @@ -350,7 +328,6 @@ static void tc_bridge_enable(struct drm_bridge *bridge) u32 hback_porch, hsync_len, hfront_porch, hactive, htime1, htime2; u32 vback_porch, vsync_len, vfront_porch, vactive, vtime1, vtime2; unsigned int val = 0; - u16 dsiclk, clkdiv, byteclk, t1, t2, t3, vsdelay; struct drm_display_mode *mode; struct drm_connector *connector = get_connector(bridge->encoder); @@ -398,27 +375,17 @@ static void tc_bridge_enable(struct drm_bridge *bridge) /* Video event mode vs pulse mode bit, does not exist for tc358775 */ if (tc->type == TC358765) - val = EVTMODE; + val = VPCTRL_EVTMODE; else val = 0; if (tc->bpc == 8) - val |= TC358775_VPCTRL_OPXLFMT(1); + val |= VPCTRL_OPXLFMT; else /* bpc = 6; */ - val |= TC358775_VPCTRL_MSF(1); - - dsiclk = mode->crtc_clock * 3 * tc->bpc / tc->num_dsi_lanes / 1000; - clkdiv = dsiclk / (tc->lvds_link == DUAL_LINK ? DIVIDE_BY_6 : DIVIDE_BY_3); - byteclk = dsiclk / 4; - t1 = hactive * (tc->bpc * 3 / 8) / tc->num_dsi_lanes; - t2 = ((100000 / clkdiv)) * (hactive + hback_porch + hsync_len + hfront_porch) / 1000; - t3 = ((t2 * byteclk) / 100) - (hactive * (tc->bpc * 3 / 8) / - tc->num_dsi_lanes); - - vsdelay = (clkdiv * (t1 + t3) / byteclk) - hback_porch - hsync_len - hactive; + val |= VPCTRL_MSF; - val |= TC358775_VPCTRL_VSDELAY(vsdelay); - regmap_write(tc->regmap, VPCTRL, val); + regmap_update_bits(tc->regmap, VPCTRL, val, + VPCTRL_OPXLFMT | VPCTRL_MSF | VPCTRL_EVTMODE); regmap_write(tc->regmap, HTIM1, htime1); regmap_write(tc->regmap, VTIM1, vtime1);
To cite the datasheet on VSDELAY: During DSI link speed is slower than that of LVDS link’s, data needs to be buffer within 775XBG before outputting to prevent data from underflow. Register field VPCTRL[VSDELAY] is used to for this purpose This driver assumes that the DSI link speed is the pixel clock (as does every DSI bridge driver), after all the LVDS clock is derived from the DSI clock. Thus we know for a fact, that the DSI link is not slower than the LVDS side. Just use the (sane) default value of the bridge and drop the complicated calculation here. While at it, replace the TC358775_VPCTRL_MSF() and TC358775_VPCTRL_OPXLFMT() inline functions by the usual macros for a bit flag. Signed-off-by: Michael Walle <mwalle@kernel.org> --- drivers/gpu/drm/bridge/tc358775.c | 49 +++++++-------------------------------- 1 file changed, 8 insertions(+), 41 deletions(-)