Messages in this thread | | | Date | Fri, 3 Feb 2023 17:19:22 +0200 | Subject | Re: [PATCH v7 5/6] drm/tidss: Add IO CTRL and Power support for OLDI TX in am625 | From | Tomi Valkeinen <> |
| |
On 25/01/2023 13:35, Aradhya Bhatia wrote: > The ctrl mmr module of the AM625 is different from the AM65X SoC. Thus > the ctrl mmr registers that supported the OLDI TX power have become > different in AM625 SoC. > > The common mode voltage of the LVDS buffers becomes random when the > bandgap reference is turned off. This causes uncertainity in the LVDS > Data and Clock signal outputs, making it behave differently under > different conditions and panel setups. The bandgap reference must be > powered on before using the OLDI IOs, to keep the common voltage trimmed > down to desired levels. > > Add support to enable/disable OLDI IO signals as well as the bandgap > reference circuit for the LVDS signals. > > Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> > --- > > Note: > - Dropped Tomi Valkeinen's reviewed-by tag in this patch because I did > not implement one of his comments which suggested to remove the > 'oldi_supported' variable. While the oldi support is indeed based on > SoC variations, keeping that variable helps take into account the case > where an OLDI supporting SoC by-passes OLDI TXes and gives out DPI > video signals straight from DSS.
Hmm why is that relevent for this patch? It doesn't use oldi_supported or the new has_oldi.
> drivers/gpu/drm/tidss/tidss_dispc.c | 57 +++++++++++++++++++----- > drivers/gpu/drm/tidss/tidss_dispc_regs.h | 40 ++++++++++++----- > 2 files changed, 76 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c > index 37a73e309330..0e03557bc142 100644 > --- a/drivers/gpu/drm/tidss/tidss_dispc.c > +++ b/drivers/gpu/drm/tidss/tidss_dispc.c > @@ -934,21 +934,56 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport, > > static void dispc_oldi_tx_power(struct dispc_device *dispc, bool power) > { > - u32 val = power ? 0 : OLDI_PWRDN_TX; > + u32 val; > > if (WARN_ON(!dispc->oldi_io_ctrl)) > return; > > - regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT0_IO_CTRL, > - OLDI_PWRDN_TX, val); > - regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT1_IO_CTRL, > - OLDI_PWRDN_TX, val); > - regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT2_IO_CTRL, > - OLDI_PWRDN_TX, val); > - regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT3_IO_CTRL, > - OLDI_PWRDN_TX, val); > - regmap_update_bits(dispc->oldi_io_ctrl, OLDI_CLK_IO_CTRL, > - OLDI_PWRDN_TX, val); > + if (dispc->feat->subrev == DISPC_AM65X) {
Slight nitpick, but I think switch-case makes sense for the subrev. Even if there are just two options here, using switch makes the structure clearer.
> + val = power ? 0 : AM65X_OLDI_PWRDN_TX; > + > + regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT0_IO_CTRL, > + AM65X_OLDI_PWRDN_TX, val); > + regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT1_IO_CTRL, > + AM65X_OLDI_PWRDN_TX, val); > + regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT2_IO_CTRL, > + AM65X_OLDI_PWRDN_TX, val); > + regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_DAT3_IO_CTRL, > + AM65X_OLDI_PWRDN_TX, val); > + regmap_update_bits(dispc->oldi_io_ctrl, AM65X_OLDI_CLK_IO_CTRL, > + AM65X_OLDI_PWRDN_TX, val); > + > + } else if (dispc->feat->subrev == DISPC_AM625) { > + if (power) { > + switch (dispc->oldi_mode) { > + case OLDI_MODE_SINGLE_LINK: > + /* Power down OLDI TX 1 */ > + val = AM625_OLDI1_PWRDN_TX; > + break; > + > + case OLDI_MODE_CLONE_SINGLE_LINK: > + case OLDI_MODE_DUAL_LINK: > + /* No Power down */ > + val = 0; > + break; > + > + default: > + /* Power down both OLDI TXes and LVDS Bandgap */ > + val = AM625_OLDI0_PWRDN_TX | AM625_OLDI1_PWRDN_TX | > + AM625_OLDI_PWRDN_BG; > + break; > + } > + > + } else { > + /* Power down both OLDI TXes and LVDS Bandgap */ > + val = AM625_OLDI0_PWRDN_TX | AM625_OLDI1_PWRDN_TX | > + AM625_OLDI_PWRDN_BG; > + } > + > + regmap_update_bits(dispc->oldi_io_ctrl, AM625_OLDI_PD_CTRL, > + AM625_OLDI0_PWRDN_TX | AM625_OLDI1_PWRDN_TX | > + AM625_OLDI_PWRDN_BG, val); > + } > } > > static void dispc_set_num_datalines(struct dispc_device *dispc, > diff --git a/drivers/gpu/drm/tidss/tidss_dispc_regs.h b/drivers/gpu/drm/tidss/tidss_dispc_regs.h > index 13feedfe5d6d..b2a148e96022 100644 > --- a/drivers/gpu/drm/tidss/tidss_dispc_regs.h > +++ b/drivers/gpu/drm/tidss/tidss_dispc_regs.h > @@ -227,17 +227,37 @@ enum dispc_common_regs { > #define DISPC_VP_DSS_DMA_THREADSIZE_STATUS 0x174 /* J721E */ > > /* > - * OLDI IO_CTRL register offsets. On AM654 the registers are found > - * from CTRL_MMR0, there the syscon regmap should map 0x14 bytes from > - * CTRLMMR0P1_OLDI_DAT0_IO_CTRL to CTRLMMR0P1_OLDI_CLK_IO_CTRL > - * register range. > + * OLDI IO and PD CTRL register offsets. > + * These registers are found in the CTRL_MMR0, where the syscon regmap should map > + * > + * 1. 0x14 bytes from CTRLMMR0P1_OLDI_DAT0_IO_CTRL to CTRLMMR0P1_OLDI_CLK_IO_CTRL > + * register range for the AM65X DSS, and > + * > + * 2. 0x200 bytes from OLDI0_DAT0_IO_CTRL to OLDI_LB_CTRL register range for the > + * AM625 DSS. > */ > -#define OLDI_DAT0_IO_CTRL 0x00 > -#define OLDI_DAT1_IO_CTRL 0x04 > -#define OLDI_DAT2_IO_CTRL 0x08 > -#define OLDI_DAT3_IO_CTRL 0x0C > -#define OLDI_CLK_IO_CTRL 0x10 > > -#define OLDI_PWRDN_TX BIT(8) > +/* -- For AM65X OLDI TX -- */ > +/* Register offsets */ > +#define AM65X_OLDI_DAT0_IO_CTRL 0x00 > +#define AM65X_OLDI_DAT1_IO_CTRL 0x04 > +#define AM65X_OLDI_DAT2_IO_CTRL 0x08 > +#define AM65X_OLDI_DAT3_IO_CTRL 0x0C > +#define AM65X_OLDI_CLK_IO_CTRL 0x10 > + > +/* Power control bits */ > +#define AM65X_OLDI_PWRDN_TX BIT(8) > + > +/* -- For AM625 OLDI TX -- */ > +/* Register offsets */ > +#define AM625_OLDI_PD_CTRL 0x100 > +#define AM625_OLDI_LB_CTRL 0x104 > + > +/* Power control bits */ > +#define AM625_OLDI0_PWRDN_TX BIT(0) > +#define AM625_OLDI1_PWRDN_TX BIT(1) > + > +/* LVDS Bandgap reference Enable/Disable */ > +#define AM625_OLDI_PWRDN_BG BIT(8) > > #endif /* __TIDSS_DISPC_REGS_H */
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Tomi
| |