diff mbox

[v2,2/7] drm/tilcdc: Add blue-and-red-crossed devicetree property

Message ID d5e79ebd165f772758014f087a7f0fedba3efc84.1472126231.git.jsarha@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jyri Sarha Aug. 25, 2016, 12:03 p.m. UTC
Add "blue-and-red-crossed"-device tree property and update devicetree
binding document. The red and blue components are reversed between 24
and 16 bit modes on am335x LCDC output pins. To get 24 RGB format the
red and blue wires has to be crossed and this in turn causes 16 colors
output to be in BGR format. With straight wiring the 16 color is RGB
and 24 bit is BGR. The new property describes whether the red and blue
wires are crossed or not. If the wires are crossed this boolean property
should be present in the relevant devicetree node.

The am335x-evm and am335x-evm have the blue and red wires crossed for
24 bit RGB mode and after this patch their LCDC nodes should have this
property to get correct colors in the display.

For more details see section 3.1.1 in AM335x Silicon Errata:
http://www.ti.com/general/docs/lit/getliterature.tsp?baseLiteratureNumber=sprz360

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 .../devicetree/bindings/display/tilcdc/tilcdc.txt  | 12 +++++++++++
 drivers/gpu/drm/tilcdc/tilcdc_drv.c                | 24 ++++++++++++++++++++++
 drivers/gpu/drm/tilcdc/tilcdc_drv.h                |  4 ++++
 drivers/gpu/drm/tilcdc/tilcdc_plane.c              |  9 +++-----
 4 files changed, 43 insertions(+), 6 deletions(-)

Comments

Rob Herring (Arm) Aug. 26, 2016, 12:51 p.m. UTC | #1
On Thu, Aug 25, 2016 at 7:03 AM, Jyri Sarha <jsarha@ti.com> wrote:
> Add "blue-and-red-crossed"-device tree property and update devicetree
> binding document. The red and blue components are reversed between 24
> and 16 bit modes on am335x LCDC output pins. To get 24 RGB format the
> red and blue wires has to be crossed and this in turn causes 16 colors
> output to be in BGR format. With straight wiring the 16 color is RGB
> and 24 bit is BGR. The new property describes whether the red and blue
> wires are crossed or not. If the wires are crossed this boolean property
> should be present in the relevant devicetree node.
>
> The am335x-evm and am335x-evm have the blue and red wires crossed for
> 24 bit RGB mode and after this patch their LCDC nodes should have this
> property to get correct colors in the display.
>
> For more details see section 3.1.1 in AM335x Silicon Errata:
> http://www.ti.com/general/docs/lit/getliterature.tsp?baseLiteratureNumber=sprz360
>
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  .../devicetree/bindings/display/tilcdc/tilcdc.txt  | 12 +++++++++++
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c                | 24 ++++++++++++++++++++++
>  drivers/gpu/drm/tilcdc/tilcdc_drv.h                |  4 ++++
>  drivers/gpu/drm/tilcdc/tilcdc_plane.c              |  9 +++-----
>  4 files changed, 43 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt b/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt
> index 6efa4c5..48a660a 100644
> --- a/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt
> +++ b/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt
> @@ -17,6 +17,8 @@ Optional properties:
>     the lcd controller.
>   - max-pixelclock: The maximum pixel clock that can be supported
>     by the lcd controller in KHz.
> + - blue-and-red-crossed: Boolean property, set this of blue and red wires
> +   for LCD_DATA are crossed for 24-bit RGB support (and 16-bit BGR mode). [1]

Doesn't this need to be blue-and-red-straight for compatibility?

>
>  Optional nodes:
>
> @@ -28,6 +30,14 @@ Optional nodes:
>     Documentation/devicetree/bindings/display/tilcdc/tfp410.txt for connecting
>     tfp410 DVI encoder or lcd panel to lcdc
>
> +[1] There is an errata about AM335x color wiring. For 16-bit color mode
> +    the wires work as they should (LCD_DATA[0:4] is for Blue[3:7]),
> +    but for 24 bit color modes the wiring of blue and red components is
> +    crossed and LCD_DATA[0:4] is for Red[3:7] and LCD_DATA[11:15] is
> +    for Blue[3-7]. For more details see section 3.1.1 in AM335x
> +    Silicon Errata:
> +    http://www.ti.com/general/docs/lit/getliterature.tsp?baseLiteratureNumber=sprz360
> +
>  Example:
>
>         fb: fb@4830e000 {
> @@ -37,6 +47,8 @@ Example:
>                 interrupts = <36>;
>                 ti,hwmods = "lcdc";
>
> +               blue-and-red-wiring = "crossed";
> +

This needs to be updated.

Rob
Jyri Sarha Aug. 26, 2016, 5:44 p.m. UTC | #2
On 08/26/16 15:51, Rob Herring wrote:
>> --- a/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt
>> > +++ b/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt
>> > @@ -17,6 +17,8 @@ Optional properties:
>> >     the lcd controller.
>> >   - max-pixelclock: The maximum pixel clock that can be supported
>> >     by the lcd controller in KHz.
>> > + - blue-and-red-crossed: Boolean property, set this of blue and red wires
>> > +   for LCD_DATA are crossed for 24-bit RGB support (and 16-bit BGR mode). [1]
> Doesn't this need to be blue-and-red-straight for compatibility?
> 

There is no way to be backward compatible with all am3 based devices.
This choice keeps the compatibility with Beaglebone-Black as far as it
can be kept and it is more logical choice for the binding. After all BBB
is way more common device than am335x-evm and am335x-evmsk are together.

I don't think there is any problems with the boards that have dts file
in the mainline source tree, like these three boards, as if the user
uses the latest mainline kernel, he probably uses the latest dtb file too.

The problem is the custom boards with custom dts files. I can not think
of any way to deal with this problem while still being 100% compatible
with all the dtb out there. This default setting is compatible with
standard colour wiring that does not take the colour errata into account.

BR,
Jyri
Tomi Valkeinen Aug. 30, 2016, 12:46 p.m. UTC | #3
On 26/08/16 20:44, Jyri Sarha wrote:
> On 08/26/16 15:51, Rob Herring wrote:
>>> --- a/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt
>>>> +++ b/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt
>>>> @@ -17,6 +17,8 @@ Optional properties:
>>>>     the lcd controller.
>>>>   - max-pixelclock: The maximum pixel clock that can be supported
>>>>     by the lcd controller in KHz.
>>>> + - blue-and-red-crossed: Boolean property, set this of blue and red wires
>>>> +   for LCD_DATA are crossed for 24-bit RGB support (and 16-bit BGR mode). [1]
>> Doesn't this need to be blue-and-red-straight for compatibility?
>>
> 
> There is no way to be backward compatible with all am3 based devices.

Hmm, I guess it would be possible to have three options:

- No property set: driver advertises RG16 and RG24. This is wrong, but
that's what the current status is, right?
- Property set to "default" or "straight" or whatever: driver says RG16
and BG24
- Property set to "crossed": driver says BG16 and RG24

 Tomi
Jyri Sarha Aug. 30, 2016, 12:54 p.m. UTC | #4
On 08/30/16 15:46, Tomi Valkeinen wrote:
> 
> 
> On 26/08/16 20:44, Jyri Sarha wrote:
>> On 08/26/16 15:51, Rob Herring wrote:
>>>> --- a/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt
>>>>> +++ b/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt
>>>>> @@ -17,6 +17,8 @@ Optional properties:
>>>>>     the lcd controller.
>>>>>   - max-pixelclock: The maximum pixel clock that can be supported
>>>>>     by the lcd controller in KHz.
>>>>> + - blue-and-red-crossed: Boolean property, set this of blue and red wires
>>>>> +   for LCD_DATA are crossed for 24-bit RGB support (and 16-bit BGR mode). [1]
>>> Doesn't this need to be blue-and-red-straight for compatibility?
>>>
>>
>> There is no way to be backward compatible with all am3 based devices.
> 
> Hmm, I guess it would be possible to have three options:
> 
> - No property set: driver advertises RG16 and RG24. This is wrong, but
> that's what the current status is, right?
> - Property set to "default" or "straight" or whatever: driver says RG16
> and BG24
> - Property set to "crossed": driver says BG16 and RG24
> 

Yes, that would be backward compatible and probably the best approach.
As this way all applications would still run, even if the colours are
wrong (as they have always been).

I'll do that if no one suggests otherwise.

BR,
Jyri
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt b/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt
index 6efa4c5..48a660a 100644
--- a/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt
+++ b/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt
@@ -17,6 +17,8 @@  Optional properties:
    the lcd controller.
  - max-pixelclock: The maximum pixel clock that can be supported
    by the lcd controller in KHz.
+ - blue-and-red-crossed: Boolean property, set this of blue and red wires
+   for LCD_DATA are crossed for 24-bit RGB support (and 16-bit BGR mode). [1]
 
 Optional nodes:
 
@@ -28,6 +30,14 @@  Optional nodes:
    Documentation/devicetree/bindings/display/tilcdc/tfp410.txt for connecting
    tfp410 DVI encoder or lcd panel to lcdc
 
+[1] There is an errata about AM335x color wiring. For 16-bit color mode
+    the wires work as they should (LCD_DATA[0:4] is for Blue[3:7]),
+    but for 24 bit color modes the wiring of blue and red components is
+    crossed and LCD_DATA[0:4] is for Red[3:7] and LCD_DATA[11:15] is
+    for Blue[3-7]. For more details see section 3.1.1 in AM335x
+    Silicon Errata:
+    http://www.ti.com/general/docs/lit/getliterature.tsp?baseLiteratureNumber=sprz360
+
 Example:
 
 	fb: fb@4830e000 {
@@ -37,6 +47,8 @@  Example:
 		interrupts = <36>;
 		ti,hwmods = "lcdc";
 
+		blue-and-red-wiring = "crossed";
+
 		port {
 			lcdc_0: endpoint@0 {
 				remote-endpoint = <&hdmi_0>;
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index e45c268..bb65872 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -33,6 +33,16 @@ 
 
 static LIST_HEAD(module_list);
 
+static const u32 tilcdc_rev1_formats[] = { DRM_FORMAT_RGB565 };
+
+static const u32 tilcdc_straight_formats[] = { DRM_FORMAT_RGB565,
+					       DRM_FORMAT_BGR888,
+					       DRM_FORMAT_XBGR8888 };
+
+static const u32 tilcdc_crossed_formats[] = { DRM_FORMAT_BGR565,
+					      DRM_FORMAT_RGB888,
+					      DRM_FORMAT_XRGB8888 };
+
 void tilcdc_module_init(struct tilcdc_module *mod, const char *name,
 		const struct tilcdc_module_ops *funcs)
 {
@@ -318,6 +328,20 @@  static int tilcdc_load(struct drm_device *dev, unsigned long flags)
 
 	pm_runtime_put_sync(dev->dev);
 
+	if (priv->rev == 1) {
+		DBG("Revision 1 LCDC supports only RGB565 format");
+		priv->pixelformats = tilcdc_rev1_formats;
+		priv->num_pixelformats = ARRAY_SIZE(tilcdc_rev1_formats);
+	} else if (of_property_read_bool(node, "blue-and-red-crossed")) {
+		DBG("Configured for crossed blue and red wires");
+		priv->pixelformats = tilcdc_crossed_formats;
+		priv->num_pixelformats = ARRAY_SIZE(tilcdc_crossed_formats);
+	} else {
+		DBG("Configured for straight blue and red wires");
+		priv->pixelformats = tilcdc_straight_formats;
+		priv->num_pixelformats = ARRAY_SIZE(tilcdc_straight_formats);
+	}
+
 	ret = modeset_init(dev);
 	if (ret < 0) {
 		dev_err(dev->dev, "failed to initialize mode setting\n");
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
index 13001df..0e19c14 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
@@ -65,6 +65,10 @@  struct tilcdc_drm_private {
 	 */
 	uint32_t max_width;
 
+	/* Supported pixel formats */
+	const uint32_t *pixelformats;
+	uint32_t num_pixelformats;
+
 	/* The context for pm susped/resume cycle is stored here */
 	struct drm_atomic_state *saved_state;
 
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_plane.c b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
index 41911e3..74c65fa 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_plane.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
@@ -24,10 +24,6 @@ 
 
 #include "tilcdc_drv.h"
 
-static const u32 tilcdc_formats[] = { DRM_FORMAT_RGB565,
-				      DRM_FORMAT_RGB888,
-				      DRM_FORMAT_XRGB8888 };
-
 static struct drm_plane_funcs tilcdc_plane_funcs = {
 	.update_plane	= drm_atomic_helper_update_plane,
 	.disable_plane	= drm_atomic_helper_disable_plane,
@@ -114,12 +110,13 @@  static const struct drm_plane_helper_funcs plane_helper_funcs = {
 int tilcdc_plane_init(struct drm_device *dev,
 		      struct drm_plane *plane)
 {
+	struct tilcdc_drm_private *priv = dev->dev_private;
 	int ret;
 
 	ret = drm_plane_init(dev, plane, 1,
 			     &tilcdc_plane_funcs,
-			     tilcdc_formats,
-			     ARRAY_SIZE(tilcdc_formats),
+			     priv->pixelformats,
+			     priv->num_pixelformats,
 			     true);
 	if (ret) {
 		dev_err(dev->dev, "Failed to initialize plane: %d\n", ret);