diff mbox series

[8/8] drm/tidss: Enable Dual and Duplicate Modes for OLDI

Message ID 20220719080845.22122-9-a-bhatia1@ti.com (mailing list archive)
State New, archived
Headers show
Series [1/8] dt-bindings: display: ti, am65x-dss: Add port properties for DSS | expand

Commit Message

Aradhya Bhatia July 19, 2022, 8:08 a.m. UTC
The AM625 DSS peripheral supports 2 OLDI TXes which can work to enable 2
duplicated displays of smaller resolutions or enable a single Dual-Link
display with a higher resolution (1920x1200).

Configure the necessary register to enable the different modes.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 44 +++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 3 deletions(-)

Comments

Tomi Valkeinen July 27, 2022, 1:22 p.m. UTC | #1
Hi,

On 19/07/2022 11:08, Aradhya Bhatia wrote:
> The AM625 DSS peripheral supports 2 OLDI TXes which can work to enable 2
> duplicated displays of smaller resolutions or enable a single Dual-Link
> display with a higher resolution (1920x1200).
> 
> Configure the necessary register to enable the different modes.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>   drivers/gpu/drm/tidss/tidss_dispc.c | 44 +++++++++++++++++++++++++++--
>   1 file changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 0b9689453ee8..28cb61259471 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -1021,8 +1021,8 @@ static void dispc_enable_oldi(struct dispc_device *dispc, u32 hw_videoport,
>   	int count = 0;
>   
>   	/*
> -	 * For the moment DUALMODESYNC, MASTERSLAVE, MODE, and SRC
> -	 * bits of DISPC_VP_DSS_OLDI_CFG are set statically to 0.
> +	 * For the moment MASTERSLAVE, and SRC bits of DISPC_VP_DSS_OLDI_CFG are
> +	 * set statically to 0.
>   	 */
>   
>   	if (fmt->data_width == 24)
> @@ -1039,7 +1039,45 @@ static void dispc_enable_oldi(struct dispc_device *dispc, u32 hw_videoport,
>   
>   	oldi_cfg |= BIT(0); /* ENABLE */
>   
> -	dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> +	/*
> +	 * As per all the current implementations of DSS, the OLDI TXes are present only on
> +	 * hw_videoport = 0 (OLDI TX 0). However, the config register for 2nd OLDI TX (OLDI TX 1)
> +	 * is present in the address space of hw_videoport = 1. Hence, using "hw_videoport + 1" to
> +	 * configure OLDI TX 1.
> +	 */
> +
> +	switch (dispc->oldi_mode) {
> +	case OLDI_MODE_OFF:
> +		oldi_cfg &= ~BIT(0); /* DISABLE */
> +		dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> +		dispc_vp_write(dispc, hw_videoport + 1, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> +		break;
> +
> +	case OLDI_SINGLE_LINK_SINGLE_MODE_0:
> +		dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> +		break;
> +
> +	case OLDI_SINGLE_LINK_SINGLE_MODE_1:
> +		dispc_vp_write(dispc, hw_videoport + 1, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> +		break;
> +
> +	case OLDI_SINGLE_LINK_DUPLICATE_MODE:
> +		oldi_cfg |= BIT(5); /* DUPLICATE MODE */
> +		dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> +		dispc_vp_write(dispc, hw_videoport + 1, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> +		break;
> +
> +	case OLDI_DUAL_LINK:
> +		oldi_cfg |= BIT(11); /* DUALMODESYNC */
> +		dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> +		dispc_vp_write(dispc, hw_videoport + 1, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> +		break;
> +
> +	default:
> +		dev_warn(dispc->dev, "%s: Incorrect oldi mode. Returning.\n",
> +			 __func__);
> +		return;
> +	}
>   
>   	while (!(oldi_reset_bit & dispc_read(dispc, DSS_SYSSTATUS)) &&
>   	       count < 10000)

This feels a bit hacky:

- The function is dispc_enable_oldi, but the above code also disables 
oldi. We have code in dispc_vp_unprepare() which disables OLDI at the 
moment.

- The function takes hw_videoport as a parameter, and is designed to 
work on that videoport. The above operates on two videoports. Isn't the 
function also called for hw_videoport +1, which would result in reg 
writes to hw_videoport + 2?

- No matching code in dispc_vp_unprepare

Obviously the duplicate mode (I presume that's "cloning") and the dual 
link complicate things here, and I have to say I haven't worked with 
such setups. But I think somehow this should be restructured so that 
common configuration (common to the OLDIs) is done somewhere else.

I would guess that there are other drivers that support cloning and dual 
mode. Did you have a look how they handle things?

  Tomi
Tomi Valkeinen July 28, 2022, 6:46 a.m. UTC | #2
On 27/07/2022 16:22, Tomi Valkeinen wrote:
> Hi,
> 
> On 19/07/2022 11:08, Aradhya Bhatia wrote:
>> The AM625 DSS peripheral supports 2 OLDI TXes which can work to enable 2
>> duplicated displays of smaller resolutions or enable a single Dual-Link
>> display with a higher resolution (1920x1200).
>>
>> Configure the necessary register to enable the different modes.
>>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>   drivers/gpu/drm/tidss/tidss_dispc.c | 44 +++++++++++++++++++++++++++--
>>   1 file changed, 41 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>> index 0b9689453ee8..28cb61259471 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>> @@ -1021,8 +1021,8 @@ static void dispc_enable_oldi(struct 
>> dispc_device *dispc, u32 hw_videoport,
>>       int count = 0;
>>       /*
>> -     * For the moment DUALMODESYNC, MASTERSLAVE, MODE, and SRC
>> -     * bits of DISPC_VP_DSS_OLDI_CFG are set statically to 0.
>> +     * For the moment MASTERSLAVE, and SRC bits of 
>> DISPC_VP_DSS_OLDI_CFG are
>> +     * set statically to 0.
>>        */
>>       if (fmt->data_width == 24)
>> @@ -1039,7 +1039,45 @@ static void dispc_enable_oldi(struct 
>> dispc_device *dispc, u32 hw_videoport,
>>       oldi_cfg |= BIT(0); /* ENABLE */
>> -    dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>> oldi_cfg);
>> +    /*
>> +     * As per all the current implementations of DSS, the OLDI TXes 
>> are present only on
>> +     * hw_videoport = 0 (OLDI TX 0). However, the config register for 
>> 2nd OLDI TX (OLDI TX 1)
>> +     * is present in the address space of hw_videoport = 1. Hence, 
>> using "hw_videoport + 1" to
>> +     * configure OLDI TX 1.
>> +     */
>> +
>> +    switch (dispc->oldi_mode) {
>> +    case OLDI_MODE_OFF:
>> +        oldi_cfg &= ~BIT(0); /* DISABLE */
>> +        dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>> oldi_cfg);
>> +        dispc_vp_write(dispc, hw_videoport + 1, 
>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>> +        break;
>> +
>> +    case OLDI_SINGLE_LINK_SINGLE_MODE_0:
>> +        dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>> oldi_cfg);
>> +        break;
>> +
>> +    case OLDI_SINGLE_LINK_SINGLE_MODE_1:
>> +        dispc_vp_write(dispc, hw_videoport + 1, 
>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>> +        break;
>> +
>> +    case OLDI_SINGLE_LINK_DUPLICATE_MODE:
>> +        oldi_cfg |= BIT(5); /* DUPLICATE MODE */
>> +        dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>> oldi_cfg);
>> +        dispc_vp_write(dispc, hw_videoport + 1, 
>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>> +        break;
>> +
>> +    case OLDI_DUAL_LINK:
>> +        oldi_cfg |= BIT(11); /* DUALMODESYNC */
>> +        dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>> oldi_cfg);
>> +        dispc_vp_write(dispc, hw_videoport + 1, 
>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>> +        break;
>> +
>> +    default:
>> +        dev_warn(dispc->dev, "%s: Incorrect oldi mode. Returning.\n",
>> +             __func__);
>> +        return;
>> +    }
>>       while (!(oldi_reset_bit & dispc_read(dispc, DSS_SYSSTATUS)) &&
>>              count < 10000)
> 
> This feels a bit hacky:
> 
> - The function is dispc_enable_oldi, but the above code also disables 
> oldi. We have code in dispc_vp_unprepare() which disables OLDI at the 
> moment.
> 
> - The function takes hw_videoport as a parameter, and is designed to 
> work on that videoport. The above operates on two videoports. Isn't the 
> function also called for hw_videoport +1, which would result in reg 
> writes to hw_videoport + 2?
> 
> - No matching code in dispc_vp_unprepare
> 
> Obviously the duplicate mode (I presume that's "cloning") and the dual 
> link complicate things here, and I have to say I haven't worked with 
> such setups. But I think somehow this should be restructured so that 
> common configuration (common to the OLDIs) is done somewhere else.
> 
> I would guess that there are other drivers that support cloning and dual 
> mode. Did you have a look how they handle things?

Oh, I see now... There's just one dss video port for OLDI, the same as 
in am65x, but that single video port is now connected to two OLDI TXes. 
And thus this function will only be called for the single video port.

But... The registers for the second OLDI are part of the second video 
port (DPI) register block?

  Tomi
Aradhya Bhatia July 28, 2022, 8:49 a.m. UTC | #3
Hi Tomi,

On 28-Jul-22 12:16, Tomi Valkeinen wrote:
> On 27/07/2022 16:22, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 19/07/2022 11:08, Aradhya Bhatia wrote:
>>> The AM625 DSS peripheral supports 2 OLDI TXes which can work to enable 2
>>> duplicated displays of smaller resolutions or enable a single Dual-Link
>>> display with a higher resolution (1920x1200).
>>>
>>> Configure the necessary register to enable the different modes.
>>>
>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>> ---
>>>   drivers/gpu/drm/tidss/tidss_dispc.c | 44 +++++++++++++++++++++++++++--
>>>   1 file changed, 41 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
>>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>>> index 0b9689453ee8..28cb61259471 100644
>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>> @@ -1021,8 +1021,8 @@ static void dispc_enable_oldi(struct 
>>> dispc_device *dispc, u32 hw_videoport,
>>>       int count = 0;
>>>       /*
>>> -     * For the moment DUALMODESYNC, MASTERSLAVE, MODE, and SRC
>>> -     * bits of DISPC_VP_DSS_OLDI_CFG are set statically to 0.
>>> +     * For the moment MASTERSLAVE, and SRC bits of 
>>> DISPC_VP_DSS_OLDI_CFG are
>>> +     * set statically to 0.
>>>        */
>>>       if (fmt->data_width == 24)
>>> @@ -1039,7 +1039,45 @@ static void dispc_enable_oldi(struct 
>>> dispc_device *dispc, u32 hw_videoport,
>>>       oldi_cfg |= BIT(0); /* ENABLE */
>>> -    dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>>> oldi_cfg);
>>> +    /*
>>> +     * As per all the current implementations of DSS, the OLDI TXes 
>>> are present only on
>>> +     * hw_videoport = 0 (OLDI TX 0). However, the config register 
>>> for 2nd OLDI TX (OLDI TX 1)
>>> +     * is present in the address space of hw_videoport = 1. Hence, 
>>> using "hw_videoport + 1" to
>>> +     * configure OLDI TX 1.
>>> +     */
>>> +
>>> +    switch (dispc->oldi_mode) {
>>> +    case OLDI_MODE_OFF:
>>> +        oldi_cfg &= ~BIT(0); /* DISABLE */
>>> +        dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>>> oldi_cfg);
>>> +        dispc_vp_write(dispc, hw_videoport + 1, 
>>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>>> +        break;
>>> +
>>> +    case OLDI_SINGLE_LINK_SINGLE_MODE_0:
>>> +        dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>>> oldi_cfg);
>>> +        break;
>>> +
>>> +    case OLDI_SINGLE_LINK_SINGLE_MODE_1:
>>> +        dispc_vp_write(dispc, hw_videoport + 1, 
>>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>>> +        break;
>>> +
>>> +    case OLDI_SINGLE_LINK_DUPLICATE_MODE:
>>> +        oldi_cfg |= BIT(5); /* DUPLICATE MODE */
>>> +        dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>>> oldi_cfg);
>>> +        dispc_vp_write(dispc, hw_videoport + 1, 
>>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>>> +        break;
>>> +
>>> +    case OLDI_DUAL_LINK:
>>> +        oldi_cfg |= BIT(11); /* DUALMODESYNC */
>>> +        dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>>> oldi_cfg);
>>> +        dispc_vp_write(dispc, hw_videoport + 1, 
>>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>>> +        break;
>>> +
>>> +    default:
>>> +        dev_warn(dispc->dev, "%s: Incorrect oldi mode. Returning.\n",
>>> +             __func__);
>>> +        return;
>>> +    }
>>>       while (!(oldi_reset_bit & dispc_read(dispc, DSS_SYSSTATUS)) &&
>>>              count < 10000)
>>
>> This feels a bit hacky:
>>
>> - The function is dispc_enable_oldi, but the above code also disables 
>> oldi. We have code in dispc_vp_unprepare() which disables OLDI at the 
>> moment.
>>
>> - The function takes hw_videoport as a parameter, and is designed to 
>> work on that videoport. The above operates on two videoports. Isn't 
>> the function also called for hw_videoport +1, which would result in 
>> reg writes to hw_videoport + 2?
>>
>> - No matching code in dispc_vp_unprepare
>>
>> Obviously the duplicate mode (I presume that's "cloning") and the dual 
>> link complicate things here, and I have to say I haven't worked with 
>> such setups. But I think somehow this should be restructured so that 
>> common configuration (common to the OLDIs) is done somewhere else.
>>
>> I would guess that there are other drivers that support cloning and 
>> dual mode. Did you have a look how they handle things?
> 
> Oh, I see now... There's just one dss video port for OLDI, the same as 
> in am65x, but that single video port is now connected to two OLDI TXes. 
> And thus this function will only be called for the single video port.
> > But... The registers for the second OLDI are part of the second video
> port (DPI) register block?

Yes! The config register for the second OLDI TX has been (incorrectly)
added in the register space for the DPI video port. 'dispc_vp_prepare'
is the only function calling 'dispc_enable_oldi', and
'dispc_enable_oldi' would not be called for hw_videoports = 1 (DPI)
because of the conditional check.

Hence, to activate both the OLDI-TXes connected to the OLDI video port,
I had to use the (hw_videoport + 1) way.

However, I will remove the disable part from the 'dispc_enable_oldi' and
I will implement the disabling properly under 'dispc_vp_unprepare', in
the next version.

Regards
Aradhya
Tomi Valkeinen July 28, 2022, 11:29 a.m. UTC | #4
On 28/07/2022 11:49, Aradhya Bhatia wrote:
> Hi Tomi,
> 
> On 28-Jul-22 12:16, Tomi Valkeinen wrote:
>> On 27/07/2022 16:22, Tomi Valkeinen wrote:
>>> Hi,
>>>
>>> On 19/07/2022 11:08, Aradhya Bhatia wrote:
>>>> The AM625 DSS peripheral supports 2 OLDI TXes which can work to 
>>>> enable 2
>>>> duplicated displays of smaller resolutions or enable a single Dual-Link
>>>> display with a higher resolution (1920x1200).
>>>>
>>>> Configure the necessary register to enable the different modes.
>>>>
>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>>> ---
>>>>   drivers/gpu/drm/tidss/tidss_dispc.c | 44 
>>>> +++++++++++++++++++++++++++--
>>>>   1 file changed, 41 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
>>>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> index 0b9689453ee8..28cb61259471 100644
>>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> @@ -1021,8 +1021,8 @@ static void dispc_enable_oldi(struct 
>>>> dispc_device *dispc, u32 hw_videoport,
>>>>       int count = 0;
>>>>       /*
>>>> -     * For the moment DUALMODESYNC, MASTERSLAVE, MODE, and SRC
>>>> -     * bits of DISPC_VP_DSS_OLDI_CFG are set statically to 0.
>>>> +     * For the moment MASTERSLAVE, and SRC bits of 
>>>> DISPC_VP_DSS_OLDI_CFG are
>>>> +     * set statically to 0.
>>>>        */
>>>>       if (fmt->data_width == 24)
>>>> @@ -1039,7 +1039,45 @@ static void dispc_enable_oldi(struct 
>>>> dispc_device *dispc, u32 hw_videoport,
>>>>       oldi_cfg |= BIT(0); /* ENABLE */
>>>> -    dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>>>> oldi_cfg);
>>>> +    /*
>>>> +     * As per all the current implementations of DSS, the OLDI TXes 
>>>> are present only on
>>>> +     * hw_videoport = 0 (OLDI TX 0). However, the config register 
>>>> for 2nd OLDI TX (OLDI TX 1)
>>>> +     * is present in the address space of hw_videoport = 1. Hence, 
>>>> using "hw_videoport + 1" to
>>>> +     * configure OLDI TX 1.
>>>> +     */
>>>> +
>>>> +    switch (dispc->oldi_mode) {
>>>> +    case OLDI_MODE_OFF:
>>>> +        oldi_cfg &= ~BIT(0); /* DISABLE */
>>>> +        dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>>>> oldi_cfg);
>>>> +        dispc_vp_write(dispc, hw_videoport + 1, 
>>>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>>>> +        break;
>>>> +
>>>> +    case OLDI_SINGLE_LINK_SINGLE_MODE_0:
>>>> +        dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>>>> oldi_cfg);
>>>> +        break;
>>>> +
>>>> +    case OLDI_SINGLE_LINK_SINGLE_MODE_1:
>>>> +        dispc_vp_write(dispc, hw_videoport + 1, 
>>>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>>>> +        break;
>>>> +
>>>> +    case OLDI_SINGLE_LINK_DUPLICATE_MODE:
>>>> +        oldi_cfg |= BIT(5); /* DUPLICATE MODE */
>>>> +        dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>>>> oldi_cfg);
>>>> +        dispc_vp_write(dispc, hw_videoport + 1, 
>>>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>>>> +        break;
>>>> +
>>>> +    case OLDI_DUAL_LINK:
>>>> +        oldi_cfg |= BIT(11); /* DUALMODESYNC */
>>>> +        dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>>>> oldi_cfg);
>>>> +        dispc_vp_write(dispc, hw_videoport + 1, 
>>>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>>>> +        break;
>>>> +
>>>> +    default:
>>>> +        dev_warn(dispc->dev, "%s: Incorrect oldi mode. Returning.\n",
>>>> +             __func__);
>>>> +        return;
>>>> +    }
>>>>       while (!(oldi_reset_bit & dispc_read(dispc, DSS_SYSSTATUS)) &&
>>>>              count < 10000)
>>>
>>> This feels a bit hacky:
>>>
>>> - The function is dispc_enable_oldi, but the above code also disables 
>>> oldi. We have code in dispc_vp_unprepare() which disables OLDI at the 
>>> moment.
>>>
>>> - The function takes hw_videoport as a parameter, and is designed to 
>>> work on that videoport. The above operates on two videoports. Isn't 
>>> the function also called for hw_videoport +1, which would result in 
>>> reg writes to hw_videoport + 2?
>>>
>>> - No matching code in dispc_vp_unprepare
>>>
>>> Obviously the duplicate mode (I presume that's "cloning") and the 
>>> dual link complicate things here, and I have to say I haven't worked 
>>> with such setups. But I think somehow this should be restructured so 
>>> that common configuration (common to the OLDIs) is done somewhere else.
>>>
>>> I would guess that there are other drivers that support cloning and 
>>> dual mode. Did you have a look how they handle things?
>>
>> Oh, I see now... There's just one dss video port for OLDI, the same as 
>> in am65x, but that single video port is now connected to two OLDI 
>> TXes. And thus this function will only be called for the single video 
>> port.
>> > But... The registers for the second OLDI are part of the second video
>> port (DPI) register block?
> 
> Yes! The config register for the second OLDI TX has been (incorrectly)
> added in the register space for the DPI video port. 'dispc_vp_prepare'
> is the only function calling 'dispc_enable_oldi', and
> 'dispc_enable_oldi' would not be called for hw_videoports = 1 (DPI)
> because of the conditional check.
> 
> Hence, to activate both the OLDI-TXes connected to the OLDI video port,
> I had to use the (hw_videoport + 1) way.

I think this should be highlighted in the comment more clearly. Also, I 
don't think hw_videoport + 1 usage is good. While in this case the 
registers are in vp + 1, it's, in a way, a coincidence.

Instead, have a new variable for the VP register block which contains 
the OLDI TX 1 registers, and just set that to 1 with a comment 
clarifying that the registers for OLDI 1 are in VP1 register block, even 
if OLDI 1 is connected to VP0.

And then use that variable when calling dispc_vp_write().

  Tomi
diff mbox series

Patch

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 0b9689453ee8..28cb61259471 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -1021,8 +1021,8 @@  static void dispc_enable_oldi(struct dispc_device *dispc, u32 hw_videoport,
 	int count = 0;
 
 	/*
-	 * For the moment DUALMODESYNC, MASTERSLAVE, MODE, and SRC
-	 * bits of DISPC_VP_DSS_OLDI_CFG are set statically to 0.
+	 * For the moment MASTERSLAVE, and SRC bits of DISPC_VP_DSS_OLDI_CFG are
+	 * set statically to 0.
 	 */
 
 	if (fmt->data_width == 24)
@@ -1039,7 +1039,45 @@  static void dispc_enable_oldi(struct dispc_device *dispc, u32 hw_videoport,
 
 	oldi_cfg |= BIT(0); /* ENABLE */
 
-	dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+	/*
+	 * As per all the current implementations of DSS, the OLDI TXes are present only on
+	 * hw_videoport = 0 (OLDI TX 0). However, the config register for 2nd OLDI TX (OLDI TX 1)
+	 * is present in the address space of hw_videoport = 1. Hence, using "hw_videoport + 1" to
+	 * configure OLDI TX 1.
+	 */
+
+	switch (dispc->oldi_mode) {
+	case OLDI_MODE_OFF:
+		oldi_cfg &= ~BIT(0); /* DISABLE */
+		dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+		dispc_vp_write(dispc, hw_videoport + 1, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+		break;
+
+	case OLDI_SINGLE_LINK_SINGLE_MODE_0:
+		dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+		break;
+
+	case OLDI_SINGLE_LINK_SINGLE_MODE_1:
+		dispc_vp_write(dispc, hw_videoport + 1, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+		break;
+
+	case OLDI_SINGLE_LINK_DUPLICATE_MODE:
+		oldi_cfg |= BIT(5); /* DUPLICATE MODE */
+		dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+		dispc_vp_write(dispc, hw_videoport + 1, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+		break;
+
+	case OLDI_DUAL_LINK:
+		oldi_cfg |= BIT(11); /* DUALMODESYNC */
+		dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+		dispc_vp_write(dispc, hw_videoport + 1, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+		break;
+
+	default:
+		dev_warn(dispc->dev, "%s: Incorrect oldi mode. Returning.\n",
+			 __func__);
+		return;
+	}
 
 	while (!(oldi_reset_bit & dispc_read(dispc, DSS_SYSSTATUS)) &&
 	       count < 10000)