| Message ID | 1378808726-32535-1-git-send-email-pawel.moll@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/10/2013 04:25 AM, Pawel Moll wrote: > This patch adds basic DT bindings for the PL11x CLCD cells > and make their fbdev driver use them. > diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt > +Optional properties: > + > +- video-ram: phandle to a node describing specialized video memory > + (that is *not* described in the top level "memory" node) > + that must be used as a framebuffer, eg. due to restrictions > + of the system interconnect; the node must contain a > + standard reg property describing the address and the size > + of the memory area Should this use the "CMA bindings" that are being proposed at the moment? Even if not, I'm not quite clear on what the referenced node is supposed to contain; is just a reg property enough, so you'd see the following at a completely arbitrary location in the DT: framebuffer-mem { reg = <0x80000000 0x00100000>; }; 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? > +- max-framebuffer-size: maximum size in bytes of the framebuffer the > + system can handle, eg. in terms of available > + memory bandwidth Size doesn't imply bandwidth, due to the potential for varying bpp, frame-rates, margin/porch sizes, etc. If this is a bandwidth limit, shouldn't we instead represent that value directly, perhaps along with some multiplier to convert theoretical bandwidth to practical bandwidth (to account for memory protocol and controller overheads)? ... > +- panel-type: (required) must be "tft" or "stn", defines panel type ... > +- panel-stn-4bit: (for monochrome "stn" panel) if present means > + that the monochrome display is connected > + via 4 bit-wide interface I just wanted to confirm that those are a complete/direct representation of all the HW options the module supports? > +- 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"?
On Tue, Sep 10, 2013 at 11:25:25AM +0100, Pawel Moll wrote: > +#ifdef CONFIG_OF > +static int clcdfb_of_get_tft_panel(struct device_node *node, > + struct clcd_panel *panel) > +{ > + int err; > + u32 rgb[3]; > + > + err = of_property_read_u32_array(node, "panel-tft-interface", rgb, 3); > + if (err) > + return err; > + > + /* Bypass pixel clock divider, data output on the falling edge */ > + panel->tim2 = TIM2_BCD | TIM2_IPC; > + > + /* TFT display, vert. comp. interrupt at the start of the back porch */ > + panel->cntl |= CNTL_LCDTFT | CNTL_LCDVCOMP(1); > + > + if (rgb[0] >= 4 && rgb[1] >= 4 && rgb[2] >= 4) > + panel->caps |= CLCD_CAP_444; > + if (rgb[0] >= 5 && rgb[1] >= 5 && rgb[2] >= 5) > + panel->caps |= CLCD_CAP_5551; > + if (rgb[0] >= 5 && rgb[1] >= 6 && rgb[2] >= 5) > + panel->caps |= CLCD_CAP_565; > + if (rgb[0] >= 8 && rgb[1] >= 8 && rgb[2] >= 8) > + panel->caps |= CLCD_CAP_888; This definitely isn't right. Why does a panel which has 8 bits of RGB get to indicate that it supports everything below it? This is not how this hardware works. Look at table A-7 in the TRM. If you wire the CLCD controller directly to a panel which is 8-bit RGB only, it can't support any of the other formats. The CLCD controller itself can only support on the hardware 8-bits of RGB or 5 bits of RGB plus an intensity bit, so that's two formats. However, things are complicated by ARMs addition of an external mux on the CLCD output, which gives us the other format possibilities by munging the signals to give appropriate formats in the framebuffer memory. In other words, in RGB444 mode, the mux ends up taking red from CLD[4:1], green from CLD[9:7,5], blue from CLD[14:13,11:10]. So, it's not true that if you have a RGB888 panel, you can support all the lower BPPs. This is one of the dangers of directly converting what's in the kernel to DT properties without getting the hardware documentation and fully understanding that first - remember, DT properties are supposed to be based on the hardware, not the Linux implementation.
On Tue, 2013-09-10 at 18:32 +0100, Stephen Warren wrote: > > +Optional properties: > > + > > +- video-ram: phandle to a node describing specialized video memory > > + (that is *not* described in the top level "memory" node) > > + that must be used as a framebuffer, eg. due to restrictions > > + of the system interconnect; the node must contain a > > + standard reg property describing the address and the size > > + of the memory area > > Should this use the "CMA bindings" that are being proposed at the moment? I expected this bit to be the hardest to get through :-) The memory in question is *not* a part of "normal" RAM, therefore CMA doesn't even know about it. The situation I have to deal with is a system when CLCD's AHB master can *not* access the normal RAM, so the designers kindly ;-) provided some static RAM which it can address. > Even if not, I'm not quite clear on what the referenced node is supposed > to contain; is just a reg property enough, so you'd see the following at > a completely arbitrary location in the DT: > > framebuffer-mem { > reg = <0x80000000 0x00100000>; > }; The place wouldn't be random, no. Getting back to my scenario, the "video" RAM, being close to CLCD, is (obviously) also addressable by the core, as any other memory-mapped device. So its position is determined by the platform memory map: \ { #address-cell = <2>; #size-cell = <2>; static-memory-bus { #address-cell = <2>; #size-cell = <1>; ranges = <2 0 0 0x18000000 64M>; motherboard { ranges; video-ram { reg = <2 0 8MB>; }; }; }; }; 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. > 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 ;-) > > +- max-framebuffer-size: maximum size in bytes of the framebuffer the > > + system can handle, eg. in terms of available > > + memory bandwidth > > Size doesn't imply bandwidth, due to the potential for varying bpp, > frame-rates, margin/porch sizes, etc. If this is a bandwidth limit, > shouldn't we instead represent that value directly, perhaps along with > some multiplier to convert theoretical bandwidth to practical bandwidth > (to account for memory protocol and controller overheads)? It's a *memory interface* bandwidth, so synchronization fields are irrelevant here. And the bpp limit is actually being calculated from this value, not the other way round. But I forgot about differing frame rates, that's fact. So it probably should be: - 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. As to the multipliers... Although I hope that the SOC designer can provide theoretical throughput data, the only practical way of getting the value I was able to come up with was just trying different combinations till cross the line, so there isn't much math to be done further. > ... > > +- panel-type: (required) must be "tft" or "stn", defines panel type > ... > > +- panel-stn-4bit: (for monochrome "stn" panel) if present means > > + that the monochrome display is connected > > + via 4 bit-wide interface > > I just wanted to confirm that those are a complete/direct representation > of all the HW options the module supports? Yes. TFT or STN. There can be one or STN panels connected. STN(s) can be color or mono. Mono STN(s) can be driven via 4 or 8 bit wide interfaces. Disclaimer: I'm not trying to pretend an expert here. I'm just basing the above on the cell's spec. > > +- 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): clcd@1f0000 { compatible = "arm,pl111", "arm,primecell"; max-memory-bandwidth = <36864000>; /* bps, 640x480@60 16bpp */ panel { type = "tft"; tft-interface = <8 8 8>; display-timings { ... } }; }; Then the panel subnode would be optional, but type and display-timings inside would be required. Also, I will have another look at what the CDF is offering, to make it as future-proof as possible. Pawe?
On Tue, 2013-09-10 at 20:43 +0100, Russell King - ARM Linux wrote: > On Tue, Sep 10, 2013 at 11:25:25AM +0100, Pawel Moll wrote: > > +#ifdef CONFIG_OF > > +static int clcdfb_of_get_tft_panel(struct device_node *node, > > + struct clcd_panel *panel) > > +{ > > + int err; > > + u32 rgb[3]; > > + > > + err = of_property_read_u32_array(node, "panel-tft-interface", rgb, 3); > > + if (err) > > + return err; > > + > > + /* Bypass pixel clock divider, data output on the falling edge */ > > + panel->tim2 = TIM2_BCD | TIM2_IPC; > > + > > + /* TFT display, vert. comp. interrupt at the start of the back porch */ > > + panel->cntl |= CNTL_LCDTFT | CNTL_LCDVCOMP(1); > > + > > + if (rgb[0] >= 4 && rgb[1] >= 4 && rgb[2] >= 4) > > + panel->caps |= CLCD_CAP_444; > > + if (rgb[0] >= 5 && rgb[1] >= 5 && rgb[2] >= 5) > > + panel->caps |= CLCD_CAP_5551; > > + if (rgb[0] >= 5 && rgb[1] >= 6 && rgb[2] >= 5) > > + panel->caps |= CLCD_CAP_565; > > + if (rgb[0] >= 8 && rgb[1] >= 8 && rgb[2] >= 8) > > + panel->caps |= CLCD_CAP_888; > > This definitely isn't right. Why does a panel which has 8 bits of RGB > get to indicate that it supports everything below it? > > This is not how this hardware works. Look at table A-7 in the TRM. > If you wire the CLCD controller directly to a panel which is 8-bit RGB > only, it can't support any of the other formats. > > The CLCD controller itself can only support on the hardware 8-bits of > RGB or 5 bits of RGB plus an intensity bit, so that's two formats. I must admit I didn't checked the PL110 CLD multiplexing scheme, naively expecting it to be the same as PL111. My bad, thanks for pointing this out. The code above works for PL111 - the signals are laid so with full 888 wiring and 444 mode the bottom bits are reserved (see table A.10). In reality they're zeros, so the colour space is narrowed. Seems that the code will have to behave differently for PL110 and PL111 here. > However, things are complicated by ARMs addition of an external mux > on the CLCD output, which gives us the other format possibilities > by munging the signals to give appropriate formats in the framebuffer > memory. In other words, in RGB444 mode, the mux ends up taking > red from CLD[4:1], green from CLD[9:7,5], blue from CLD[14:13,11:10]. It's the Versatile AB/PB hacked CLCD, PL110/111 hybrid, right? It doesn't really fall into the "arm,pl110" and "arm,pl111" compatible value, so would need to be something like "arm,versatile,clcdc" and require custom code. I don't think I want to solve this problem right now. > This is one of the dangers of directly converting what's in the kernel > to DT properties without getting the hardware documentation and fully > understanding that first - remember, DT properties are supposed to be > based on the hardware, not the Linux implementation. I don't think I've directly converted what's in the kernel to DT properties. There is no "caps" property in the binding. There is only information how is the cell wired up to a panel, and this information is valid for both PL110 and PL111 as it is. My fault I've believed when was told that "the two are basically the same." and should have compared the TRMs in details. Will fix the code. Pawe?
On 09/11/2013 05:45 AM, Pawel Moll wrote: > On Tue, 2013-09-10 at 18:32 +0100, Stephen Warren wrote: >>> +Optional properties: >>> + >>> +- video-ram: phandle to a node describing specialized video memory >>> + (that is *not* described in the top level "memory" node) >>> + that must be used as a framebuffer, eg. due to restrictions >>> + of the system interconnect; the node must contain a >>> + standard reg property describing the address and the size >>> + of the memory area >> >> Should this use the "CMA bindings" that are being proposed at the moment? > > I expected this bit to be the hardest to get through :-) > > The memory in question is *not* a part of "normal" RAM, therefore CMA > doesn't even know about it. The situation I have to deal with is a > system when CLCD's AHB master can *not* access the normal RAM, so the > designers kindly ;-) provided some static RAM which it can address. > >> Even if not, I'm not quite clear on what the referenced node is supposed >> to contain; is just a reg property enough, so you'd see the following at >> a completely arbitrary location in the DT: >> >> framebuffer-mem { >> reg = <0x80000000 0x00100000>; >> }; > > The place wouldn't be random, no. Getting back to my scenario, the > "video" RAM, being close to CLCD, is (obviously) also addressable by the > core, as any other memory-mapped device. So its position is determined > by the platform memory map: > > \ { > #address-cell = <2>; > #size-cell = <2>; > static-memory-bus { > #address-cell = <2>; > #size-cell = <1>; > ranges = <2 0 0 0x18000000 64M>; > motherboard { > ranges; > video-ram { > reg = <2 0 8MB>; > }; > }; > }; > }; > > 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. Perhaps the CLCD could reference the memreserve then? 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? >> 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. >>> +- max-framebuffer-size: maximum size in bytes of the framebuffer the >>> + system can handle, eg. in terms of available >>> + memory bandwidth >> >> Size doesn't imply bandwidth, due to the potential for varying bpp, >> frame-rates, margin/porch sizes, etc. If this is a bandwidth limit, >> shouldn't we instead represent that value directly, perhaps along with >> some multiplier to convert theoretical bandwidth to practical bandwidth >> (to account for memory protocol and controller overheads)? > > It's a *memory interface* bandwidth, so synchronization fields are > irrelevant here. For average bandwidth, sure. However, the peak bandwidth is affected; if your active region is 80% vs 95% of your line length, that's a difference in the required region during the active period only. Of course, your HW may start pre-fetching much earlier than the start of the active region, so there are many variables. > And the bpp limit is actually being calculated from > this value, not the other way round. But I forgot about differing frame > rates, that's fact. So it probably should be: > > - 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. > As to the multipliers... Although I hope that the SOC designer can > provide theoretical throughput data, the only practical way of getting > the value I was able to come up with was just trying different > combinations till cross the line, so there isn't much math to be done > further. I don't know how constrained of a system CLCD is, but I do know that mode validation is a very complex process in some real-life graphics drivers. There are many complex calculations/modelling/heuristics applied. I guess it would be very difficult to fully parametrize this through DT; a real/complete solution is going to have to know enormous detail of the memory system. So it's probably not worth pushing for DT to represent the information required for this. I suppose in practice, for a simple solution, the best idea is to revise max-memory-bandwidth down (internally to the driver to maintain DT ABI I suppose) if any problems are found in practice with the calculations. >> ... >>> +- panel-type: (required) must be "tft" or "stn", defines panel type >> ... >>> +- panel-stn-4bit: (for monochrome "stn" panel) if present means >>> + that the monochrome display is connected >>> + via 4 bit-wide interface >> >> I just wanted to confirm that those are a complete/direct representation >> of all the HW options the module supports? > > Yes. TFT or STN. There can be one or STN panels connected. STN(s) can be > color or mono. Mono STN(s) can be driven via 4 or 8 bit wide interfaces. > Disclaimer: I'm not trying to pretend an expert here. I'm just basing > the above on the cell's spec. > >>> +- 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. 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?
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@1,00000000 { compatible = "arm,vexpress-psram", "mtd-ram"; reg = <1 0x00000000 0x02000000>; bank-width = <4>; }; vram@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?
On 09/12/2013 07:03 AM, Pawel Moll wrote: > On Wed, 2013-09-11 at 18:39 +0100, Stephen Warren wrote: ... >> 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... Perhaps it's an extension that hasn't actually been implemented yet. I thought I saw some reference to it in some long-past discussions. It's possible that this feature has been dropped in preference to the "CMA" bindings for defining reserved memory regions? ... > 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. What do "upper" and "lower" mean in this context? I assume this somehow refers to placement of the displays on the board. If so, it sounds like there should be some better terminology; something that's specific to the CLCD controller itself, rather than the environment it's used in. Aalthough it does sound like this is in an FPGA and hence the controller is board-specific, but I don't see why it couldn't be re-used in some other FPGA-based board with different panel layout. Would "A" and "B" or "main" and "primary" or "first" and "second" work better?
On Thu, 2013-09-12 at 15:55 +0100, Stephen Warren wrote: > > 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. > > What do "upper" and "lower" mean in this context? I assume this somehow > refers to placement of the displays on the board. If so, it sounds like > there should be some better terminology; something that's specific to > the CLCD controller itself, It is very CLCD specific indeed and nothing to do with the board :-) http://arminfo.emea.arm.com/help/topic/com.arm.doc.ddi0293c/I899134.html "Upper and Lower Panel Frame Base Address Registers". > rather than the environment it's used in. > Aalthough it does sound like this is in an FPGA and hence the controller > is board-specific, but I don't see why it couldn't be re-used in some > other FPGA-based board with different panel layout. Would "A" and "B" or > "main" and "primary" or "first" and "second" work better? Funnily enough I typed "secondary-framebuffer-base" first, but than thought "no, let's match the spec" :-) Pawe?
On Thu, 2013-09-12 at 16:26 +0100, Pawel Moll wrote:
> http://arminfo.emea.arm.com/help/topic/com.arm.doc.ddi0293c/I899134.html
Sorry, should be:
http://infocenter.arm.com/help/topic/com.arm.doc.ddi0161e/I899134.html
Pawe?
On Wed, Sep 11, 2013 at 11:39:55AM -0600, Stephen Warren wrote: > I don't know how constrained of a system CLCD is, but I do know that > mode validation is a very complex process in some real-life graphics > drivers. Apart from maximum memory bus bandwidth, probably maximum output bandwidth, maximum resolution (determined by what will fit in the registers and RAM) there aren't that much constraints. The hardware block is quite simple in that regard. More the problem is to deal with two situations: 1. you have a particular panel connected to it which requires a certain fixed timing regime. 2. you have the CLCD connected to a VGA or HDMI connector where the timing is dependent on the connected display. The former would be the subject of some kind of *common* DT representation of the timing requirements of the connected panel. For the latter, DT needs to specify how the EDID data is retrieved, or if there is no mechanism for that, being able to provide a set of allowable timing parameters (such as min/max vsync, min/max hsync, max dotclock - in other words, the data which used to be provided to X11 in the past.) None of that is specific to CLCD though: it's the same problem as the SA11x0 LCD controller or any other scanned video controller.
On Thu, Sep 12, 2013 at 08:55:54AM -0600, Stephen Warren wrote: > On 09/12/2013 07:03 AM, Pawel Moll wrote: > > 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. > > What do "upper" and "lower" mean in this context? I assume this somehow > refers to placement of the displays on the board. CLCD is only one controller - it can only produce one image from one framebuffer. What this refers to is a requirement for dual STN displays - rather than starting at one corner and working logically to the opposite corner, these start at one corner and half way through the display at the same time. So you scan out the upper half of the image at the same time that you scan out the lower half of the image. In order to give an image on the display where the conventional formula works: address = start + y * bytes_per_line + x * bits_per_pixel / 8 this means that the "upper" and "lower" start addresses are related to the parameters of the display - you want the lower half to start at the address where y = panel_y_res / 2 and x = 0. You may also wish to change the start address (if you have enough RAM) to support page flipping - where you switch between two framebuffers - one which is currently being displayed while the other is being updated by the CPU. You would use this to provide smooth video playback for example. So, DT has no business directly encoding this information - it should be calculated from the information provided by the panel and the timings required, and all that the CLCD driver should know is that "we have this RAM which starts here, and extends for N bytes" - or to use system memory instead.
diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt new file mode 100644 index 0000000..7eb77aa --- /dev/null +++ b/Documentation/devicetree/bindings/video/arm,pl11x.txt @@ -0,0 +1,87 @@ +* ARM PrimeCell Color LCD (CLCD) Controller PL110/PL111 + +See also Documentation/devicetree/bindings/arm/primecell.txt + +Required properties: + +- compatible: must be one of: + "arm,pl110", "arm,primecell" + "arm,pl111", "arm,primecell" +- reg: base address and size of the control registers block +- interrupts: either a single interrupt specifier representing the + combined interrupt output (CLCDINTR) or an array of + four interrupt specifiers for CLCDMBEINTR, + CLCDVCOMPINTR, CLCDLNBUINTR, CLCDFUFINTR; in the + latter case interrupt names must be specified + (see below) +- interrupt-names: when four interrupts are specified, their names: + "mbe", "vcomp", "lnbu", "fuf" + must be specified in order respective to the + interrupt specifiers +- clocks: contains phandle and clock specifier pairs for the entries + in the clock-names property. See + Documentation/devicetree/binding/clock/clock-bindings.txt +- clocks names: should contain "clcdclk" and "apb_pclk" + +Optional properties: + +- video-ram: phandle to a node describing specialized video memory + (that is *not* described in the top level "memory" node) + that must be used as a framebuffer, eg. due to restrictions + of the system interconnect; the node must contain a + standard reg property describing the address and the size + of the memory area +- max-framebuffer-size: maximum size in bytes of the framebuffer the + system can handle, eg. in terms of available + memory bandwidth + +In the simplest case of a display panel being connected directly to the +CLCD, it can be described in the node: + +- panel-dimensions: (optional) array of two numbers (width and height) + describing physical dimension in mm of the panel +- panel-type: (required) must be "tft" or "stn", defines panel type +- panel-tft-interface: (for "tft" panel type) array of 3 numbers defining + widths in bits of the R, G and B components +- panel-tft-rb-swapped: (for "tft" panel type) if present means that + the R & B components are swapped on the board +- panel-stn-color: (for "stn" panel type) if present means that + the panel is a colour STN display, if missing + is a monochrome display +- panel-stn-dual: (for "stn" panel type) if present means that there + are two STN displays connected +- panel-stn-4bit: (for monochrome "stn" panel) if present means + that the monochrome display is connected + via 4 bit-wide interface +- display-timings: standard display timings sub-node, see + Documentation/devicetree/bindings/video/display-timing.txt + +Example: + + clcd@1f0000 { + compatible = "arm,pl111", "arm,primecell"; + reg = <0x1f0000 0x1000>; + interrupts = <14>; + clocks = <&v2m_oscclk1>, <&smbclk>; + clock-names = "clcdclk", "apb_pclk"; + + video-ram = <&v2m_vram>; + max-framebuffer-size = <614400>; /* 640x480 16bpp */ + + panel-type = "tft"; + panel-tft-interface = <8 8 8>; + display-timings { + native-mode = <&v2m_clcd_timing0>; + v2m_clcd_timing0: vga { + clock-frequency = <25175000>; + hactive = <640>; + hback-porch = <40>; + hfront-porch = <24>; + hsync-len = <96>; + vactive = <480>; + vback-porch = <32>; + vfront-porch = <11>; + vsync-len = <2>; + }; + }; + }; diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index 4cf1e1d..375bf63 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig @@ -316,6 +316,7 @@ config FB_ARMCLCD select FB_CFB_FILLRECT select FB_CFB_COPYAREA select FB_CFB_IMAGEBLIT + select VIDEOMODE_HELPERS if OF help This framebuffer device driver is for the ARM PrimeCell PL110 Colour LCD controller. ARM PrimeCells provide the building diff --git a/drivers/video/amba-clcd.c b/drivers/video/amba-clcd.c index 0a2cce7..7f36964 100644 --- a/drivers/video/amba-clcd.c +++ b/drivers/video/amba-clcd.c @@ -25,6 +25,11 @@ #include <linux/amba/clcd.h> #include <linux/clk.h> #include <linux/hardirq.h> +#include <linux/dma-mapping.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <video/of_display_timing.h> +#include <video/of_videomode.h> #include <asm/sizes.h> @@ -542,6 +547,209 @@ static int clcdfb_register(struct clcd_fb *fb) return ret; } +#ifdef CONFIG_OF +static int clcdfb_of_get_tft_panel(struct device_node *node, + struct clcd_panel *panel) +{ + int err; + u32 rgb[3]; + + err = of_property_read_u32_array(node, "panel-tft-interface", rgb, 3); + if (err) + return err; + + /* Bypass pixel clock divider, data output on the falling edge */ + panel->tim2 = TIM2_BCD | TIM2_IPC; + + /* TFT display, vert. comp. interrupt at the start of the back porch */ + panel->cntl |= CNTL_LCDTFT | CNTL_LCDVCOMP(1); + + if (rgb[0] >= 4 && rgb[1] >= 4 && rgb[2] >= 4) + panel->caps |= CLCD_CAP_444; + if (rgb[0] >= 5 && rgb[1] >= 5 && rgb[2] >= 5) + panel->caps |= CLCD_CAP_5551; + if (rgb[0] >= 5 && rgb[1] >= 6 && rgb[2] >= 5) + panel->caps |= CLCD_CAP_565; + if (rgb[0] >= 8 && rgb[1] >= 8 && rgb[2] >= 8) + panel->caps |= CLCD_CAP_888; + + if (of_get_property(node, "panel-tft-rb-swapped", NULL)) + panel->caps &= ~CLCD_CAP_RGB; + else + panel->caps &= ~CLCD_CAP_BGR; + + return 0; +} + +static int clcdfb_snprintf_mode(char *buf, int size, struct fb_videomode *mode) +{ + return snprintf(buf, size, "%ux%u@%u", mode->xres, mode->yres, + mode->refresh); +} + +static int clcdfb_of_init_display(struct clcd_fb *fb) +{ + struct device_node *node = fb->dev->dev.of_node; + int err, len; + char *mode_name; + u32 max_size; + u32 dimensions[2]; + const char *panel_type; + + fb->panel = devm_kzalloc(&fb->dev->dev, sizeof(*fb->panel), GFP_KERNEL); + if (!fb->panel) + return -ENOMEM; + + err = of_get_fb_videomode(node, &fb->panel->mode, OF_USE_NATIVE_MODE); + if (err) + return err; + + len = clcdfb_snprintf_mode(NULL, 0, &fb->panel->mode); + mode_name = devm_kzalloc(&fb->dev->dev, len + 1, GFP_KERNEL); + clcdfb_snprintf_mode(mode_name, len + 1, &fb->panel->mode); + fb->panel->mode.name = mode_name; + + err = of_property_read_u32(node, "max-framebuffer-size", &max_size); + if (!err) + fb->panel->bpp = max_size / (fb->panel->mode.xres * + fb->panel->mode.yres) * 8; + else + fb->panel->bpp = 32; + +#ifdef CONFIG_CPU_BIG_ENDIAN + fb->panel->cntl |= CNTL_BEBO; +#endif + + if (of_property_read_u32_array(node, "panel-dimensions", + dimensions, 2) == 0) { + fb->panel->width = dimensions[0]; + fb->panel->height = dimensions[1]; + } else { + fb->panel->width = -1; + fb->panel->height = -1; + } + + panel_type = of_get_property(node, "panel-type", &len); + if (strncmp(panel_type, "tft", len) == 0) + return clcdfb_of_get_tft_panel(node, fb->panel); + else + return -EINVAL; +} + +static int clcdfb_of_vram_setup(struct clcd_fb *fb) +{ + struct device_node *node = of_parse_phandle(fb->dev->dev.of_node, + "video-ram", 0); + u64 size; + int err; + + if (!node) + return -ENODEV; + + err = clcdfb_of_init_display(fb); + if (err) + return err; + + fb->fb.screen_base = of_iomap(node, 0); + if (!fb->fb.screen_base) + return -ENOMEM; + + fb->fb.fix.smem_start = of_translate_address(node, + of_get_address(node, 0, &size, NULL)); + fb->fb.fix.smem_len = size; + + return 0; +} + +static int clcdfb_of_vram_mmap(struct clcd_fb *fb, struct vm_area_struct *vma) +{ + unsigned long off, user_size, kernel_size; + + off = vma->vm_pgoff << PAGE_SHIFT; + user_size = vma->vm_end - vma->vm_start; + kernel_size = fb->fb.fix.smem_len; + + if (off >= kernel_size || user_size > (kernel_size - off)) + return -ENXIO; + + return remap_pfn_range(vma, vma->vm_start, + __phys_to_pfn(fb->fb.fix.smem_start) + vma->vm_pgoff, + user_size, + pgprot_writecombine(vma->vm_page_prot)); +} + +static void clcdfb_of_vram_remove(struct clcd_fb *fb) +{ + iounmap(fb->fb.screen_base); +} + +static int clcdfb_of_dma_setup(struct clcd_fb *fb) +{ + unsigned long framesize; + dma_addr_t dma; + int err; + + err = clcdfb_of_init_display(fb); + if (err) + return err; + + framesize = fb->panel->mode.xres * fb->panel->mode.yres * + fb->panel->bpp / 8; + fb->fb.screen_base = dma_alloc_writecombine(&fb->dev->dev, framesize, + &dma, GFP_KERNEL); + if (!fb->fb.screen_base) + return -ENOMEM; + + fb->fb.fix.smem_start = dma; + fb->fb.fix.smem_len = framesize; + + return 0; +} + +static int clcdfb_of_dma_mmap(struct clcd_fb *fb, struct vm_area_struct *vma) +{ + return dma_mmap_writecombine(&fb->dev->dev, vma, fb->fb.screen_base, + fb->fb.fix.smem_start, fb->fb.fix.smem_len); +} + +static void clcdfb_of_dma_remove(struct clcd_fb *fb) +{ + dma_free_writecombine(&fb->dev->dev, fb->fb.fix.smem_len, + fb->fb.screen_base, fb->fb.fix.smem_start); +} + +static struct clcd_board *clcdfb_of_get_board(struct amba_device *dev) +{ + struct clcd_board *board = devm_kzalloc(&dev->dev, sizeof(*board), + GFP_KERNEL); + struct device_node *node = dev->dev.of_node; + + if (!board) + return NULL; + + board->name = of_node_full_name(node); + board->caps = CLCD_CAP_ALL; + board->check = clcdfb_check; + board->decode = clcdfb_decode; + if (of_find_property(node, "video-ram", NULL)) { + board->setup = clcdfb_of_vram_setup; + board->mmap = clcdfb_of_vram_mmap; + board->remove = clcdfb_of_vram_remove; + } else { + board->setup = clcdfb_of_dma_setup; + board->mmap = clcdfb_of_dma_mmap; + board->remove = clcdfb_of_dma_remove; + } + + return board; +} +#else +static struct clcd_board *clcdfb_of_get_board(struct amba_dev *dev) +{ + return NULL; +} +#endif + static int clcdfb_probe(struct amba_device *dev, const struct amba_id *id) { struct clcd_board *board = dev->dev.platform_data; @@ -549,6 +757,9 @@ static int clcdfb_probe(struct amba_device *dev, const struct amba_id *id) int ret; if (!board) + board = clcdfb_of_get_board(dev); + + if (!board) return -EINVAL; ret = amba_request_regions(dev, NULL);
This patch adds basic DT bindings for the PL11x CLCD cells and make their fbdev driver use them. Signed-off-by: Pawel Moll <pawel.moll@arm.com> --- Changes since v1: - minor code cleanups as suggested by Sylwester Nawrocki .../devicetree/bindings/video/arm,pl11x.txt | 87 +++++++++ drivers/video/Kconfig | 1 + drivers/video/amba-clcd.c | 211 +++++++++++++++++++++ 3 files changed, 299 insertions(+) create mode 100644 Documentation/devicetree/bindings/video/arm,pl11x.txt