On 03-Feb-23 20:49, Tomi Valkeinen wrote:
> 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@xxxxxx>
>> ---
>>
>> 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.
It doesn't. Not directly atleast. In the previous version of this patch,
there was a mention of 'oldi_supported' and your tag was conditional to
that variable getting removed. Instead, I renamed the variable.
>
>> 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.
Alright. I will make the edit.
>
>> + 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@xxxxxxxxxxxxxxxx>
>
> Tomi
>
Regards
Aradhya
