Message-ID: <dea025e1-98d4-2dcf-e729-19c9d49bf3ae@ti.com>
Date: Tue, 28 Jan 2025 18:46:53 +0530
From: Devarsh Thakkar <devarsht@...com>
To: Aradhya Bhatia <aradhya.bhatia@...ux.dev>,
        "jyri.sarha@....fi"
	<jyri.sarha@....fi>,
        "tomi.valkeinen@...asonboard.com"
	<tomi.valkeinen@...asonboard.com>,
        "airlied@...il.com" <airlied@...il.com>,
        "maarten.lankhorst@...ux.intel.com" <maarten.lankhorst@...ux.intel.com>,
        "mripard@...nel.org" <mripard@...nel.org>,
        "tzimmermann@...e.de"
	<tzimmermann@...e.de>,
        "dri-devel@...ts.freedesktop.org"
	<dri-devel@...ts.freedesktop.org>,
        "simona@...ll.ch" <simona@...ll.ch>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "robh@...nel.org"
	<robh@...nel.org>,
        "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
        "conor+dt@...nel.org" <conor+dt@...nel.org>
CC: "Bajjuri, Praneeth" <praneeth@...com>,
        "Raghavendra, Vignesh"
	<vigneshr@...com>,
        "Jain, Swamil" <s-jain1@...com>,
        "Donadkar, Rishikesh"
	<r-donadkar@...com>,
        "Choudhary, Jayesh" <j-choudhary@...com>,
        "Shenoy,
 Harikrishna" <h-shenoy@...com>
Subject: Re: [PATCH 2/2] drm/tidss: Add support for AM62L display subsystem

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

>> +
>> +    .num_vps = 1,
>> +    .vp_name = { "vp1" },
>> +    .ovr_name = { "ovr1" },
>> +    .vpclk_name =  { "vp1" },
>> +    .vp_bus_type = { DISPC_VP_DPI },
>> +
>> +    .vp_feat = { .color = {
>> +                    .has_ctm = true,
>> +                    .gamma_size = 256,
>> +                    .gamma_type = TIDSS_GAMMA_8BIT,
>> +            },
>> +    },
>> +
>> +    .num_planes = 1,
>> +    .vid_name = { "vidl1" },
>> +    .vid_lite = { true },
>> +    .vid_order = { 0 },
>
> ...
>
> Usually, VID1 is the first video pipeline, while VIDL1 remains the
> second. Which is why vid1 occupies the index 0, and vidl1 occupies 1 -
> even from the hardware perspective.
>
> While AM62L has only one video (lite) pipeline - vidl1, and there is no
> vid1, the hardware still treats vidl1 at index 1.
>
> For example, the TRM has defined DISPC_VID_IRQ{STATUS, ENABLE}_1 (and
> not _0) in the common region.
>
> So, the vid_order here should be 1, not 0.
>

We will have a separate register space for AM62L which would only have
DISPC_VID_IRQ{STATUS, ENABLE}_1 mapped,
so that should handle it.Also I think vid_order semantically maps to zorder
and since there is only a single plane available on AM62L
it is not correct to assign vid_order as 1 just to align with VIDL reg bit fields.
Furthermore, vid_order set to 1 won't work too, since driver also uses
vid_order for indexing the reg space for vid. For e.g.
if we use vid_order as 1 then hw_plane would get assigned as 1 too, then
dispc_vid* functions (as seen below) would fail as
there is no base_vid region at index 1 for AM62L as it has only single video
register space i.e. for VIDL.

static                                                                                                 
 
void dispc_vid_write(struct dispc_device *dispc, u32 hw_plane, u16 reg, u32
val)                         
{                                                                                                      
 
        void __iomem *base =
dispc->base_vid[hw_plane];                                                  
                                                                                                       
 
        iowrite32(val, base +
reg);                                                                      
}                                                                                                      
 

But with hw_plane set to 0 we also need to handle bit fields for VIDL which
are still at index 5 for registers DSS_COMMON_DISPC_IRQENABLE_SET,
DSS_COMMON_DISPC_IRQENABLE_CLR and DSS_COMMON_DISPC_IRQSTATUS_(RAW) and also
DSS_OVR1_ATTRIBUTES_0 which expects 1 to be set in bits 4:1,
and existing dispc_k3* functions managing these registers were handling it via
hw_plane set to 1.  To handle this and also since AM62L is the only one K3 SoC
which only supports VIDL plane, I will be adding separate wrapper implementer
functions dispc_am62l_ovr_set_plane, dispc_am62l_set_irqenable to handle this.

Regards
Devarsh

Powered by blists - more mailing lists