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