Re: [PATCH 2/2] drm/tidss: Add support for AM62L display subsystem

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

Hi,

On 28/01/2025 15:16, Devarsh Thakkar wrote:
> Hi Aradhya,
> 
> On 18/01/25 14:57, Aradhya Bhatia wrote:
>> Hi Devarsh,
>>
>> Thanks for the patches.
>>
> 
> Thanks for the review.
> 
>> On 31/12/24 14:34, Devarsh Thakkar wrote:
>>> Enable display for AM62L DSS [1] which supports only a single display
>>> pipeline using a single overlay manager, single video port and a single
>>> video lite pipeline which does not support scaling.
>>>
>>> The output of video port is routed to SoC boundary via DPI interface and
>>> the DPI signals from the video port are also routed to DSI Tx controller
>>> present within the SoC.
>>>
>>> [1]: Section 11.7 (Display Subsystem and Peripherals)
>>> Link : https://www.ti.com/lit/pdf/sprujb4
>>>
>>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>>> ---
> 
> <snip>
>>>   
>>> +const struct dispc_features dispc_am62l_feats = {
>>> +    .max_pclk_khz = {
>>> +            [DISPC_VP_DPI] = 165000,
>>
>> The TRM mentions that the max the VP PLL can go is 150MHz, so maybe you
>> might need to update this.
>>
>> That said, as far as I understand, the IP itself can support 165 MHz,
>> and I am wondering, what would we do if there comes out a new SoC that
>> uses AM62L DSS as is, but just bumps up the PLL capacity to 165MHz.
>>
>> It would be odd to have a ditto feats structure with just the frequency
>> updated.
> 
> TRM mentions max VP PLL frequency as 165 Mhz and not 150 Mhz. Please refer
> Table 11-343. DSS Signals for MIPI DPI 2.0 or BT.656/BT.1120 section in
> section 11.7.1.2.1 DSS Parallel Interface of  AM62L TRM [1].
>>
>>> +    },
>>> +
>>> +    .subrev = DISPC_AM62L,
>>> +
>>> +    .common = "common",
>>> +    .common_regs = tidss_am65x_common_regs,
>>
>> Also, I don't think we should utilize this as is.
>>
>> The AM62L common regions is different, and is, at best, a subset of the
>> AM65x/AM62x common register space.
>>
>> For example, registers VID_IRQ{STATUS, ENABLE}_0 have been dropped,
>> along with VP_IRQ{STATUS, ENABLE}_1.
>>
>> - Which brings to my next concern...
>>
> 
> Thanks for pointing out, I probably missed this since the use-case still
> worked since VP interrupts were still enabled and those were sufficient to
> drive the display
> but the VID underflow interrupts and VID specific bits were probably not
> enabled at-all due to above miss, so agreed
> we should probably go ahead with a different reg space for AM62L due to
> aforementioned differences.

I think I disagree here. Afaiu, AM62L has plane at hw index 1 (VIDL1), 
but the plane at hw index 0 (VID1) is not instantiated in the hardware. 
But the registers are the same, i.e. AM62L's registers for VIDL1 match 
AM65x/AM62x registers, right?

If so, we just need to tell the driver the hw index, instead of creating 
new register offsets as done in v2.

Or am I missing something here? (I haven't looked at the HW manual yet).

  Tomi