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

[Tomi Valkeinen]

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@xxxxxx>
> > > ---
>
> <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