Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

From: Liu Ying <victor.liu@oss.nxp.com>
To: Marek Vasut <marex@denx.de>, dri-devel@lists.freedesktop.org
Cc: devicetree@vger.kernel.org, Peng Fan <peng.fan@nxp.com>,
	Alexander Stein <alexander.stein@ew.tq-group.com>,
	Rob Herring <robh+dt@kernel.org>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Robby Cai <robby.cai@nxp.com>, Sam Ravnborg <sam@ravnborg.org>
Subject: Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
Date: Mon, 28 Feb 2022 16:18:45 +0800	[thread overview]
Message-ID: <a3ab4ec2dd0c7b87698bc7902509a4de6950dd25.camel@oss.nxp.com> (raw)
In-Reply-To: <8eac8a2c-bc6d-0c79-c727-bdaa2cd9abee@denx.de>

On Mon, 2022-02-28 at 07:57 +0100, Marek Vasut wrote:
> On 2/28/22 07:37, Liu Ying wrote:
> > Hi Marek,
> 
> Hi,
> 
> > On Mon, 2022-02-28 at 01:45 +0100, Marek Vasut wrote:
> > > Add compatible string for i.MX8MP LCDIF variant. This is called LCDIFv3
> > > and is completely different from the LCDIFv3 found in i.MX23 in that it
> > 
> > In i.MX23 reference manual, there is no LCDIFv3 found, but only LCDIF.
> 
> See i.MX23 HW_LCDIF_VERSION MAJOR=0x3 , that's LCDIF V3 . MX28 has LCDIF 
> V4 .

Ok, got it now. AFAIK, the SoC design team calls i.MX8MP display
controller as 'LCDIFv3'. Those in other SoCs are called 'LCDIF'.  There
is not even a register in i.MX8MP display controller to decribe the
version.

> 
> > > has a completely scrambled register layout compared to all previous LCDIF
> > 
> > It looks like no single register of i.MX8MP LCDIFv3 overlaps with
> > registers in other i.MX2x/6x/7x/8x LCDIFs. The LCDIFv3 block diagram is
> > totally different from the LCDIF block diagram, according to the SoC
> > reference manuals. LCDIFv3 supports SHADOW_EN bit to update horizontal
> > and vertical size of graphic, position of graphic on the panel, address
> > of graphic in memory and color formats or color palettes, which is not
> > supported by LCDIF and impacts display driver control mechanism
> > considerably. LCDIF supports DOTCLK interface, MPU interface and VSYNC
> > interface, while LCDIFv3 only supports parallel output as a counterpart
> > of the DOTCLK interface.
> > 
> > Generally speaking, LCDIFv3 is just a new display IP which happens to
> > have the word 'LCDIF' in its name.  Although both of LCDIFv3 and LCDIF
> > are display controllers for scanning out frames onto display devices, I
> > don't think they are in one family.
> > 
> > So, LCDIFv3 deserves a new separate dt-binding, IMO.
> 
> It seems to me a lot of those bits just map to their previous 
> equivalents in older LCDIF, others were dropped, so this is some sort of 
> new LCDIF mutation, is it not ?

I say 'LCDIFv3' and 'LCDIF' are totally two IPs, if I compare the names
of registers and the names of register bits . 

> 
> I am aware NXP has a separate driver in its downstream, but I'm not 
> convinced the duplication of boilerplate code by introducing a separate 
> driver for what looks like another LCDIF variant is the right approach.

Hmmm, given the two IPs, I think there should be separate drivers.
 With one single driver, there would be too many 'if/else' checks to
separate the logics for the IPs, just like Patch 9/9 does.  The
boilerplate code to do things like registering a drm device is
acceptable, IMO.

Aside from that, with separate drivers, we don't have to test too many
SoCs if we only want to touch either 'LCDIFv3' or 'LCDIF'. 

> 
> > > variants. The new LCDIFv3 also supports 36bit address space. However,
> > > except for the complete bit reshuffling, this is still LCDIF and it still
> > > works like one.
> 
> [...]