[PATCH v2 1/2] video: ARM CLCD: Add DT support

[Pawel Moll]

On Wed, 2013-09-11 at 18:39 +0100, Stephen Warren wrote:
> > From the core's perspective it's just a bit of extra memory, you could,
> > for example, put a MTD ram disk on it. It seems to deserve a
> > representation in the tree then.
> 
> Yes, that's a good point. Perhaps it is reasonable to represent the
> memory somewhere then.
> 
> I don't see why this memory couldn't exist in the regular /memory node;
> it is after all (admittedly slow) RAM. Obviously you'd want to cover the
> region with a /memreserve/ to avoid it being used just like any other
> RAM. 

This would be pretty tricky in my particular case... The sram chip lives
on a motherboard, which lives behind a static memory interface (Chip
Select + offset). And the memreserve can only take "top level" address,
which can be different depending on the test chip (SoC) or FPGA SMM
being used.

> Perhaps the CLCD could reference the memreserve then?

How do you mean reference the memreserve? It's not a node, I don't think you can get
a phandle to it...

> Alternatively, if you want to represent the region as a regular node
> rather than a /memory entry, I'd expect there to be a binding defining
> what that node was, and the node to at least have a compatible value as
> well. I'm not sure how you would indicate that the node should be MTD
> vs. a resource for CLCD though; perhaps you'd use a different compatible
> value depending on the intended use of the memory?

Yes, that's exactly what I have now:

                psram at 1,00000000 { 
                        compatible = "arm,vexpress-psram", "mtd-ram";
                        reg = <1 0x00000000 0x02000000>;
                        bank-width = <4>;
                };
                
                vram at 2,00000000 {
                        compatible = "arm,vexpress-vram";
                        reg = <2 0x00000000 0x00800000>;
                };

which allows mtd to use the psram.

> >> I'm not sure what the benefit of making this a standalone node is; why
> >> not just put the base/size directly into the video-ram property in the
> >> CLCD node?
> > 
> > This is certainly an option. I've considered this as well, but thought
> > that the above made more sense.
> > 
> > Having said that, there is actually a use case, although a very
> > non-probable one, for having a direct number there... The interconnect
> > that CLCD is connected to could have different mapping than the
> > processor's one. In other word, the memory seen by the core at X, could
> > be accessed by the CLCD at Y. Or, in even more quirky situation, the
> > core may not have access to the memory at all, with the graphics being
> > generated only by GPU (or any other third party). Then the value would
> > mean "the address you have to use when generating AMBA read transactions
> > to be get some meaningful data" and would have to be known explicitly.
> > 
> > I guess it won't be hard to convince me it's a better option ;-)
> 
> That's true. Everything in DT is currently set up to describe the CPU's
> memory map. Either we need to add some more properties to describe the
> potentially different memory map for other bus masters, and/or we should
> represent the various buses in DT, and any driver doing DMA should ask
> each bus's driver in turn to translate the core-visible address to a bus
> address so we can eventually end up with the address needed by the bus
> masters.
> 
> That is indeed complex, and could at least in many situations simple be
> short-circuited by putting the relevant base address in each other bus
> master's own node/property. Hopefully we wouldn't get bitten by how
> non-general that could be.

Ok, so I'll make it a arm,pl11x,specific property. Actually two
properties - one bit I missed was the dual STN panel case, where they
have separate frame buffers. Something along the lines of:

Optional:

- arm,pl11x,framebuffer-base: a pair of two values, address and size,
				defining the framebuffer for the upper
				(or the only one) panel
- arm,pl11x,lower-framebuffer-base: as above, for the lower STN panel,
					when present

Being very hardware-specific, it covers all possible quirky scenarios,
at the same time does not exclude the option of using the CMA region
phandles, if a particular system needed it.

> > - max-memory-bandwidth: maximum bandwidth in bytes per second that the
> > 			cell's memory interface can handle
> > 
> > and can be then used to calculate maximum bpp depending on the selected
> > mode.
> 
> Yes, that's a better property definition.

Ok, I'll go that way then. Whether derived from the system design
characteristics or by "hands on experiments", seems hardware-y enough.

> >>> +- display-timings: standard display timings sub-node, see
> >>> +			Documentation/devicetree/bindings/video/display-timing.txt
> >>
> >> Should that be in a "Required sub-nodes" section (I assume required not
> >> optional) rather than "Optional Properties"?
> > 
> > Right, the whole "panel" section is kept separately in a hope that CDF
> > (or DRM or whatever ;-) generic display pipeline bindings will deprecate
> > it. So the display-timings is required when optional panel properties
> > are present. Maybe I should split them into a separate sub-node?
> > Something along these lines (including the bandwidth example):
> 
> I would assume that TFT-vs-STN is a pretty direct representation of the
> HW (IO bus to panel in particular), and hence wouldn't be replaced by
> CDF? I would expect CDF to represent the more generic properties. But, I
> haven't been following CDF too closely, so perhaps that's not true.

I'm not expecting CDF *bindings* to carry this information, no. I'm
expecting CDF core to provide me with this data in runtime (here's the
panel; what kind of panel are you? what modes do you provide? etc.) and
this is what my initial experiments used. The CLCD node only had the
video-ram and bandwidth-like properties plus a phandle to the display. 

> If you're expecting this binding to change if/when CDF appears, perhaps
> it'd be better to wait for CDF, or plan for a new compatible property at
> that time, or add some property indicating old-style configuration vs
> new-style configuration once CDF is supported?

I'm expecting the panel description inside the CLCD node to be
deprecated, not changed, by the mythical CDF. As to waiting for it...
well. It's been over a year since first proposal ("generic panel
framework" as was it called then ;-). And over 2 years since VE gained
"DT support except for CLCD, which should be ready soon" ;-) And than 2
or 3 new platforms had to keep auxdata only for CLCD's sake. So, sure -
we can wait :-)

Paweł