[PATCH v2 0/3] drm/tilcdc: Add bridge support and sync-lost flood recovery

[PATCH v2 0/3] drm/tilcdc: Add bridge support and sync-lost flood recovery

[Jyri Sarha]

Changes since first version of the series:
- "drm/tilcdc: Recover from sync lost error flood by resetting the LCDC"
  - no change
- "drm/bridge: Add ti-tfp410 DVI transmitter driver"
  - HDMI -> DVI
  - DT Binding document
    - Prepare for tfp410 connected trough i2c by optional reg property
    - Require two port nodes
  - Implementation
    - Implement connector node functionality with in tfp410 bridge
      drive, but follow generic connector binding by pulling the
      ddc-i2c-bus property from the connector node.
- "drm/tilcdc: Add drm bridge support for attaching drm bridge drivers"
  - Remove earlier change in TD binding document. There is no need to
    mention DRM implementation details, like bridge support, in DT
    binding.

The first patch is an independent on and I've been testing it for
quite a while now.

The tfp410 bridge driver and the tilcdc bridge support are tested with
BeagleBone DVI-D Cape Rev A3. The tfp410 bridge driver is missing a
lot of features, because the DVI-D cape does not have too many wires
connected. The missing features can be added later when they are
needed.

Jyri Sarha (3):
  drm/tilcdc: Recover from sync lost error flood by resetting the LCDC
  drm/bridge: Add ti-tfp410 DVI transmitter driver
  drm/tilcdc: Add drm bridge support for attaching drm bridge drivers

 .../bindings/display/bridge/ti,tfp410.txt          |  41 ++++
 drivers/gpu/drm/bridge/Kconfig                     |   7 +
 drivers/gpu/drm/bridge/Makefile                    |   1 +
 drivers/gpu/drm/bridge/ti-tfp410.c                 | 223 +++++++++++++++++++++
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c               |  26 ++-
 drivers/gpu/drm/tilcdc/tilcdc_drv.c                |   7 +-
 drivers/gpu/drm/tilcdc/tilcdc_drv.h                |   2 +
 drivers/gpu/drm/tilcdc/tilcdc_external.c           | 140 +++++++++++--
 drivers/gpu/drm/tilcdc/tilcdc_external.h           |   1 +
 9 files changed, 428 insertions(+), 20 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
 create mode 100644 drivers/gpu/drm/bridge/ti-tfp410.c

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/3] drm/tilcdc: Recover from sync lost error flood by resetting the LCDC

[Jyri Sarha]

Recover from sync lost error flood by resetting the LCDC instead of
turning off the SYNC_LOST error IRQ. When LCDC starves on limited
memory bandwidth it may sometimes result an error situation when the
picture may have shifted couple of pixels to right and SYNC_LOST
interrupt is generated on every frame. LCDC main reset recovers from
this situation and causes a brief blanking on the screen.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 0d09acc..c787349 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -55,6 +55,7 @@ struct tilcdc_crtc {
 
 	int sync_lost_count;
 	bool frame_intact;
+	struct work_struct recover_work;
 };
 #define to_tilcdc_crtc(x) container_of(x, struct tilcdc_crtc, base)
 
@@ -252,6 +253,25 @@ static bool tilcdc_crtc_is_on(struct drm_crtc *crtc)
 	return crtc->state && crtc->state->enable && crtc->state->active;
 }
 
+static void tilcdc_crtc_recover_work(struct work_struct *work)
+{
+	struct tilcdc_crtc *tilcdc_crtc =
+		container_of(work, struct tilcdc_crtc, recover_work);
+	struct drm_crtc *crtc = &tilcdc_crtc->base;
+
+	dev_info(crtc->dev->dev, "%s: Reset CRTC", __func__);
+
+	drm_modeset_lock_crtc(crtc, NULL);
+
+	if (!tilcdc_crtc_is_on(crtc))
+		goto out;
+
+	tilcdc_crtc_disable(crtc);
+	tilcdc_crtc_enable(crtc);
+out:
+	drm_modeset_unlock_crtc(crtc);
+}
+
 static void tilcdc_crtc_destroy(struct drm_crtc *crtc)
 {
 	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
@@ -838,9 +858,12 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc)
 			tilcdc_crtc->frame_intact = false;
 			if (tilcdc_crtc->sync_lost_count++ >
 			    SYNC_LOST_COUNT_LIMIT) {
-				dev_err(dev->dev, "%s(0x%08x): Sync lost flood detected, disabling the interrupt", __func__, stat);
+				dev_err(dev->dev, "%s(0x%08x): Sync lost flood detected, recovering", __func__, stat);
+				queue_work(system_wq,
+					   &tilcdc_crtc->recover_work);
 				tilcdc_write(dev, LCDC_INT_ENABLE_CLR_REG,
 					     LCDC_SYNC_LOST);
+				tilcdc_crtc->sync_lost_count = 0;
 			}
 		}
 
@@ -880,6 +903,7 @@ struct drm_crtc *tilcdc_crtc_create(struct drm_device *dev)
 			"unref", unref_worker);
 
 	spin_lock_init(&tilcdc_crtc->irq_lock);
+	INIT_WORK(&tilcdc_crtc->recover_work, tilcdc_crtc_recover_work);
 
 	ret = drm_crtc_init_with_planes(dev, crtc,
 					&tilcdc_crtc->primary,
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/3] drm/bridge: Add ti-tfp410 DVI transmitter driver

[Jyri Sarha]

Add very basic ti-ftp410 DVI transmitter driver. The only feature
separating this from a completely dummy bridge is the EDID read
support trough DDC I2C. Even that functionality should be in a
separate generic connector driver. However, because of missing DRM
infrastructure support the connector is implemented within the bridge
driver. Some tfp410 HW specific features may be added later if needed,
because there is a set of registers behind i2c if it is connected.

This implementations is tested against my new tilcdc bridge support
and it works with BeagleBone DVI-D Cape Rev A3. A DT binding document
is also added.

Signed-off-by: Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>
---
 .../bindings/display/bridge/ti,tfp410.txt          |  41 ++++
 drivers/gpu/drm/bridge/Kconfig                     |   7 +
 drivers/gpu/drm/bridge/Makefile                    |   1 +
 drivers/gpu/drm/bridge/ti-tfp410.c                 | 223 +++++++++++++++++++++
 4 files changed, 272 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
 create mode 100644 drivers/gpu/drm/bridge/ti-tfp410.c

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
new file mode 100644
index 0000000..7446b2b
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
@@ -0,0 +1,41 @@
+TFP410 DVI bridge bindings
+
+Required properties:
+	- compatible: "ti,tfp410"
+
+Optional properties
+	- reg: I2C address. If and only if present the driver node
+	  should be placed into the i2c controller node where the
+	  tfp410 i2c is connected to (the current implementation does
+	  not yet support this).
+
+Required subnodes:
+	- port@0: Video input port node to connect the bridge to a
+	  display controller output [1].
+	- port@1: Video output port node to connect the bridge to a
+	  connector input [1].
+
+[1]: Documentation/devicetree/bindings/media/video-interfaces.txt
+
+Example:
+	hdmi-bridge {
+		compatible = "ti,tfp410";
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				bridge_in: endpoint {
+					remote-endpoint = <&dc_out>;
+				};
+			};
+
+			port@1 {
+				reg = <1>;
+				bridge_out: endpoint {
+					remote-endpoint = <&hdmi_in>;
+				};
+			};
+		};
+	};
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index bd6acc8..a424e03 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -81,6 +81,13 @@ config DRM_TOSHIBA_TC358767
 	---help---
 	  Toshiba TC358767 eDP bridge chip driver.
 
+config DRM_TI_TFP410
+	tristate "TI TFP410 DVI/HDMI bridge"
+	depends on OF
+	select DRM_KMS_HELPER
+	---help---
+	  Texas Instruments TFP410 DVI/HDMI Transmitter driver
+
 source "drivers/gpu/drm/bridge/analogix/Kconfig"
 
 source "drivers/gpu/drm/bridge/adv7511/Kconfig"
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 97ed1a5..8b065d9 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_DRM_SII902X) += sii902x.o
 obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
 obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
 obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
+obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
new file mode 100644
index 0000000..a806cd6
--- /dev/null
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -0,0 +1,223 @@
+/*
+ * Copyright (C) 2016 Texas Instruments
+ * Author: Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/of_graph.h>
+
+#include <drm/drmP.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_crtc_helper.h>
+
+struct tfp410 {
+	struct drm_bridge	bridge;
+	struct drm_connector	connector;
+
+	struct i2c_adapter	*ddc;
+
+	struct device *dev;
+};
+
+static inline struct tfp410 *
+drm_bridge_to_tfp410(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct tfp410, bridge);
+}
+
+static inline struct tfp410 *
+drm_connector_to_tfp410(struct drm_connector *connector)
+{
+	return container_of(connector, struct tfp410, connector);
+}
+
+static int tfp410_get_modes(struct drm_connector *connector)
+{
+	struct tfp410 *dvi = drm_connector_to_tfp410(connector);
+	struct edid *edid;
+	int ret;
+
+	if (!dvi->ddc)
+		goto fallback;
+
+	edid = drm_get_edid(connector, dvi->ddc);
+	if (!edid) {
+		DRM_INFO("EDID read failed. Fallback to standard modes\n");
+		goto fallback;
+	}
+
+	drm_mode_connector_update_edid_property(connector, edid);
+
+	return drm_add_edid_modes(connector, edid);
+fallback:
+	/* No EDID, fallback on the XGA standard modes */
+	ret = drm_add_modes_noedid(connector, 1920, 1200);
+
+	/* And prefer a mode pretty much anything can handle */
+	drm_set_preferred_mode(connector, 1024, 768);
+
+	return ret;
+}
+
+static const struct drm_connector_helper_funcs tfp410_con_helper_funcs = {
+	.get_modes	= tfp410_get_modes,
+};
+
+static enum drm_connector_status
+tfp410_connector_detect(struct drm_connector *connector, bool force)
+{
+	struct tfp410 *dvi = drm_connector_to_tfp410(connector);
+
+	if (dvi->ddc) {
+		if (drm_probe_ddc(dvi->ddc))
+			return connector_status_connected;
+		else
+			return connector_status_disconnected;
+	}
+
+	return connector_status_unknown;
+}
+
+static const struct drm_connector_funcs tfp410_con_funcs = {
+	.dpms			= drm_atomic_helper_connector_dpms,
+	.detect			= tfp410_connector_detect,
+	.fill_modes		= drm_helper_probe_single_connector_modes,
+	.destroy		= drm_connector_cleanup,
+	.reset			= drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state	= drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
+};
+
+static int tfp410_attach(struct drm_bridge *bridge)
+{
+	struct tfp410 *dvi = drm_bridge_to_tfp410(bridge);
+	int ret;
+
+	if (!bridge->encoder) {
+		dev_err(dvi->dev, "Missing encoder\n");
+		return -ENODEV;
+	}
+
+	drm_connector_helper_add(&dvi->connector,
+				 &tfp410_con_helper_funcs);
+	ret = drm_connector_init(bridge->dev, &dvi->connector,
+				 &tfp410_con_funcs, DRM_MODE_CONNECTOR_HDMIA);
+	if (ret) {
+		dev_err(dvi->dev, "drm_connector_init() failed: %d\n", ret);
+		return ret;
+	}
+
+	drm_mode_connector_attach_encoder(&dvi->connector,
+					  bridge->encoder);
+
+	return 0;
+}
+
+static const struct drm_bridge_funcs tfp410_bridge_funcs = {
+	.attach		= tfp410_attach,
+};
+
+static int tfp410_get_connector_ddc(struct tfp410 *dvi)
+{
+	struct device_node *ep = NULL, *connector_node = NULL;
+	struct device_node *ddc_phandle = NULL;
+	int ret = 0;
+
+	/* port@1 is the connector node */
+	ep = of_graph_get_endpoint_by_regs(dvi->dev->of_node, 1, -1);
+	if (!ep)
+		goto fail;
+
+	connector_node = of_graph_get_remote_port_parent(ep);
+	if (!connector_node)
+		goto fail;
+
+	ddc_phandle = of_parse_phandle(connector_node, "ddc-i2c-bus", 0);
+	if (!ddc_phandle)
+		goto fail;
+
+	dvi->ddc = of_get_i2c_adapter_by_node(ddc_phandle);
+	if (dvi->ddc)
+		dev_info(dvi->dev, "Connector's ddc i2c bus found\n");
+	else
+		ret = -EPROBE_DEFER;
+
+fail:
+	of_node_put(ep);
+	of_node_put(connector_node);
+	of_node_put(ddc_phandle);
+	return ret;
+}
+
+static int tfp410_probe(struct platform_device *pdev)
+{
+	struct tfp410 *dvi;
+	int ret;
+
+	if (!pdev->dev.of_node) {
+		dev_err(&pdev->dev, "device-tree data is missing\n");
+		return -ENXIO;
+	}
+
+	dvi = devm_kzalloc(&pdev->dev, sizeof(*dvi), GFP_KERNEL);
+	if (!dvi)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, dvi);
+
+	dvi->bridge.funcs = &tfp410_bridge_funcs;
+	dvi->bridge.of_node = pdev->dev.of_node;
+	dvi->dev = &pdev->dev;
+
+	ret = tfp410_get_connector_ddc(dvi);
+	if (ret)
+		goto fail;
+
+	ret = drm_bridge_add(&dvi->bridge);
+	if (ret) {
+		dev_err(&pdev->dev, "drm_bridge_add() failed: %d\n", ret);
+		goto fail;
+	}
+
+	return 0;
+fail:
+	i2c_put_adapter(dvi->ddc);
+	return ret;
+}
+
+static int tfp410_remove(struct platform_device *pdev)
+{
+	struct tfp410 *dvi = platform_get_drvdata(pdev);
+
+	drm_bridge_remove(&dvi->bridge);
+
+	if (dvi->ddc)
+		i2c_put_adapter(dvi->ddc);
+
+	return 0;
+}
+
+static const struct of_device_id tfp410_match[] = {
+	{ .compatible = "ti,tfp410" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, tfp410_match);
+
+struct platform_driver tfp410_driver = {
+	.probe	= tfp410_probe,
+	.remove	= tfp410_remove,
+	.driver	= {
+		.name		= "tfp410-bridge",
+		.of_match_table	= tfp410_match,
+	},
+};
+module_platform_driver(tfp410_driver);
+
+MODULE_AUTHOR("Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>");
+MODULE_DESCRIPTION("TI TFP410 DVI bridge driver");
+MODULE_LICENSE("GPL");
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] drm/bridge: Add ti-tfp410 DVI transmitter driver

[Rob Herring]

On Mon, Nov 14, 2016 at 06:54:17PM +0200, Jyri Sarha wrote:
> Add very basic ti-ftp410 DVI transmitter driver. The only feature
> separating this from a completely dummy bridge is the EDID read
> support trough DDC I2C. Even that functionality should be in a
> separate generic connector driver. However, because of missing DRM
> infrastructure support the connector is implemented within the bridge
> driver. Some tfp410 HW specific features may be added later if needed,
> because there is a set of registers behind i2c if it is connected.
> 
> This implementations is tested against my new tilcdc bridge support
> and it works with BeagleBone DVI-D Cape Rev A3. A DT binding document
> is also added.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  .../bindings/display/bridge/ti,tfp410.txt          |  41 ++++
>  drivers/gpu/drm/bridge/Kconfig                     |   7 +
>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>  drivers/gpu/drm/bridge/ti-tfp410.c                 | 223 +++++++++++++++++++++
>  4 files changed, 272 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
>  create mode 100644 drivers/gpu/drm/bridge/ti-tfp410.c
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
> new file mode 100644
> index 0000000..7446b2b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
> @@ -0,0 +1,41 @@
> +TFP410 DVI bridge bindings
> +
> +Required properties:
> +	- compatible: "ti,tfp410"
> +
> +Optional properties
> +	- reg: I2C address. If and only if present the driver node
> +	  should be placed into the i2c controller node where the
> +	  tfp410 i2c is connected to (the current implementation does
> +	  not yet support this).

So this chip can work without programming I guess?

reg should only be not present if I2C is not connected in the design. It 
can't be a function of what the driver supports. In otherwords, you 
can't be moving this node around based on when you add I2C control.

Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/3] drm/bridge: Add ti-tfp410 DVI transmitter driver

[Jyri Sarha]

On 11/16/16 15:33, Rob Herring wrote:
>> +Optional properties
>> > +	- reg: I2C address. If and only if present the driver node
>> > +	  should be placed into the i2c controller node where the
>> > +	  tfp410 i2c is connected to (the current implementation does
>> > +	  not yet support this).
> So this chip can work without programming I guess?
> 

Yes. Just powering it up is enough for most application.

> reg should only be not present if I2C is not connected in the design. It 
> can't be a function of what the driver supports. In otherwords, you 
> can't be moving this node around based on when you add I2C control.
> 

Ok, I'll try to implement a dummy i2c driver at the same time too. I can
not test anything related to it because I do not have a piece of HW with
tfp410 i2c wires connected, but it should not matter as long as I am
able to probe it as a i2c client.

Thanks,
Jyri

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] drm/bridge: Add ti-tfp410 DVI transmitter driver

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 729 bytes --]



On 16/11/16 16:39, Jyri Sarha wrote:
> On 11/16/16 15:33, Rob Herring wrote:
>>> +Optional properties
>>>> +	- reg: I2C address. If and only if present the driver node
>>>> +	  should be placed into the i2c controller node where the
>>>> +	  tfp410 i2c is connected to (the current implementation does
>>>> +	  not yet support this).
>> So this chip can work without programming I guess?
>>
> 
> Yes. Just powering it up is enough for most application.

Right, and not only that, but in all the TI boards I have seen TFP410's
i2c pins are not even connected. The data sheet says "[TFP410] can be
controlled in two ways: 1) configuration and state pins or 2) the
programmable I2C serial interface".

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/3] drm/bridge: Add ti-tfp410 DVI transmitter driver

[Laurent Pinchart]

Hi Jyri,

On Wednesday 16 Nov 2016 16:39:28 Jyri Sarha wrote:
> On 11/16/16 15:33, Rob Herring wrote:
> >> +Optional properties
> >> 
> >>> +	- reg: I2C address. If and only if present the driver node

I assume you meant device node, not driver node ?

> >>> +	  should be placed into the i2c controller node where the
> >>> +	  tfp410 i2c is connected to (the current implementation does
> >>> +	  not yet support this).
> > 
> > So this chip can work without programming I guess?
> 
> Yes. Just powering it up is enough for most application.
> 
> > reg should only be not present if I2C is not connected in the design. It
> > can't be a function of what the driver supports. In otherwords, you
> > can't be moving this node around based on when you add I2C control.
> 
> Ok, I'll try to implement a dummy i2c driver at the same time too. I can
> not test anything related to it because I do not have a piece of HW with
> tfp410 i2c wires connected, but it should not matter as long as I am
> able to probe it as a i2c client.

I think that Rob's point was that whether the current implementation supports 
this or not is irrelevant from a DT bindings point of view. It should not be 
mentioned in the bindings document.

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] drm/bridge: Add ti-tfp410 DVI transmitter driver

[Rob Herring]

On Thu, Nov 17, 2016 at 3:45 AM, Laurent Pinchart
<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> wrote:
> Hi Jyri,
>
> On Wednesday 16 Nov 2016 16:39:28 Jyri Sarha wrote:
>> On 11/16/16 15:33, Rob Herring wrote:
>> >> +Optional properties
>> >>
>> >>> + - reg: I2C address. If and only if present the driver node
>
> I assume you meant device node, not driver node ?
>
>> >>> +   should be placed into the i2c controller node where the
>> >>> +   tfp410 i2c is connected to (the current implementation does
>> >>> +   not yet support this).
>> >
>> > So this chip can work without programming I guess?
>>
>> Yes. Just powering it up is enough for most application.
>>
>> > reg should only be not present if I2C is not connected in the design. It
>> > can't be a function of what the driver supports. In otherwords, you
>> > can't be moving this node around based on when you add I2C control.
>>
>> Ok, I'll try to implement a dummy i2c driver at the same time too. I can
>> not test anything related to it because I do not have a piece of HW with
>> tfp410 i2c wires connected, but it should not matter as long as I am
>> able to probe it as a i2c client.
>
> I think that Rob's point was that whether the current implementation supports
> this or not is irrelevant from a DT bindings point of view. It should not be
> mentioned in the bindings document.

Right. Now, whether the h/w has I2C wires connected or not is relevant
to the binding doc. So the doc just needs some rewording, a dummy
driver isn't needed.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/3] drm/tilcdc: Add drm bridge support for attaching drm bridge drivers

[Jyri Sarha]

Adds drm bride support for attaching drm bridge drivers to tilcdc. The
decision whether a video port leads to an external encoder or bridge
is made simply based on remote device's compatible string. The code
has been tested with BeagleBone-Black with and without BeagleBone
DVI-D Cape Rev A3 using ti-tfp410 driver.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c      |   7 +-
 drivers/gpu/drm/tilcdc/tilcdc_drv.h      |   2 +
 drivers/gpu/drm/tilcdc/tilcdc_external.c | 140 +++++++++++++++++++++++++++----
 drivers/gpu/drm/tilcdc/tilcdc_external.h |   1 +
 4 files changed, 131 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index dcc9621..ec22576 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -384,9 +384,14 @@ static int tilcdc_init(struct drm_driver *ddrv, struct device *dev)
 		ret = tilcdc_add_external_encoders(ddev);
 		if (ret < 0)
 			goto init_failed;
+	} else {
+		ret = tilcdc_attach_remote_device(ddev);
+		if (ret)
+			goto init_failed;
 	}
 
-	if ((priv->num_encoders == 0) || (priv->num_connectors == 0)) {
+	if (!priv->remote_encoder &&
+	    ((priv->num_encoders == 0) || (priv->num_connectors == 0))) {
 		dev_err(dev, "no encoders/connectors found\n");
 		ret = -ENXIO;
 		goto init_failed;
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
index d31fe5d..283ff28 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
@@ -90,6 +90,8 @@ struct tilcdc_drm_private {
 	struct drm_connector *connectors[8];
 	const struct drm_connector_helper_funcs *connector_funcs[8];
 
+	struct drm_encoder *remote_encoder;
+
 	bool is_registered;
 	bool is_componentized;
 };
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_external.c b/drivers/gpu/drm/tilcdc/tilcdc_external.c
index 06a4c58..e1576ba 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_external.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_external.c
@@ -28,6 +28,18 @@
 		.raster_order           = 0,
 };
 
+static const struct tilcdc_panel_info panel_info_default = {
+		.ac_bias                = 255,
+		.ac_bias_intrpt         = 0,
+		.dma_burst_sz           = 16,
+		.bpp                    = 16,
+		.fdd                    = 0x80,
+		.tft_alt_mode           = 0,
+		.sync_edge              = 0,
+		.sync_ctrl              = 1,
+		.raster_order           = 0,
+};
+
 static int tilcdc_external_mode_valid(struct drm_connector *connector,
 				      struct drm_display_mode *mode)
 {
@@ -130,6 +142,101 @@ void tilcdc_remove_external_encoders(struct drm_device *dev)
 						 priv->connector_funcs[i]);
 }
 
+static const struct drm_encoder_funcs tilcdc_remote_encoder_funcs = {
+	.destroy	= drm_encoder_cleanup,
+};
+
+static
+int tilcdc_attach_bridge(struct drm_device *ddev, struct drm_bridge *bridge)
+{
+	struct tilcdc_drm_private *priv = ddev->dev_private;
+	int ret;
+
+	priv->remote_encoder->possible_crtcs = BIT(0);
+	priv->remote_encoder->bridge = bridge;
+	bridge->encoder = priv->remote_encoder;
+
+	ret = drm_bridge_attach(ddev, bridge);
+	if (ret) {
+		dev_err(ddev->dev, "drm_bridge_attach() failed %d\n", ret);
+		return ret;
+	}
+
+	tilcdc_crtc_set_panel_info(priv->crtc, &panel_info_default);
+
+	return 0;
+}
+
+static int tilcdc_node_has_port(struct device_node *dev_node)
+{
+	struct device_node *node;
+
+	node = of_get_child_by_name(dev_node, "ports");
+	if (!node)
+		node = of_get_child_by_name(dev_node, "port");
+	if (!node)
+		return 0;
+	of_node_put(node);
+
+	return 1;
+}
+
+static
+struct device_node *tilcdc_get_remote_node(struct device_node *node)
+{
+	struct device_node *ep;
+	struct device_node *parent;
+
+	if (!tilcdc_node_has_port(node))
+		return NULL;
+
+	ep = of_graph_get_next_endpoint(node, NULL);
+	if (!ep)
+		return NULL;
+
+	parent = of_graph_get_remote_port_parent(ep);
+	of_node_put(ep);
+
+	return parent;
+}
+
+int tilcdc_attach_remote_device(struct drm_device *ddev)
+{
+	struct tilcdc_drm_private *priv = ddev->dev_private;
+	struct device_node *remote_node;
+	struct drm_bridge *bridge;
+	int ret;
+
+	remote_node = tilcdc_get_remote_node(ddev->dev->of_node);
+	if (!remote_node)
+		return 0;
+
+	bridge = of_drm_find_bridge(remote_node);
+	of_node_put(remote_node);
+	if (!bridge)
+		return -EPROBE_DEFER;
+
+	priv->remote_encoder = devm_kzalloc(ddev->dev,
+					    sizeof(*priv->remote_encoder),
+					    GFP_KERNEL);
+	if (!priv->remote_encoder)
+		return -ENOMEM;
+
+	ret = drm_encoder_init(ddev, priv->remote_encoder,
+			       &tilcdc_remote_encoder_funcs,
+			       DRM_MODE_ENCODER_NONE, NULL);
+	if (ret) {
+		dev_err(ddev->dev, "drm_encoder_init() failed %d\n", ret);
+		return ret;
+	}
+
+	ret = tilcdc_attach_bridge(ddev, bridge);
+	if (ret)
+		drm_encoder_cleanup(priv->remote_encoder);
+
+	return ret;
+}
+
 static int dev_match_of(struct device *dev, void *data)
 {
 	return dev->of_node == data;
@@ -141,16 +248,10 @@ int tilcdc_get_external_components(struct device *dev,
 	struct device_node *node;
 	struct device_node *ep = NULL;
 	int count = 0;
+	int ret = 0;
 
-	/* Avoid error print by of_graph_get_next_endpoint() if there
-	 * is no ports present.
-	 */
-	node = of_get_child_by_name(dev->of_node, "ports");
-	if (!node)
-		node = of_get_child_by_name(dev->of_node, "port");
-	if (!node)
+	if (!tilcdc_node_has_port(dev->of_node))
 		return 0;
-	of_node_put(node);
 
 	while ((ep = of_graph_get_next_endpoint(dev->of_node, ep))) {
 		node = of_graph_get_remote_port_parent(ep);
@@ -160,17 +261,20 @@ int tilcdc_get_external_components(struct device *dev,
 		}
 
 		dev_dbg(dev, "Subdevice node '%s' found\n", node->name);
-		if (match)
-			drm_of_component_match_add(dev, match, dev_match_of,
-						   node);
-		of_node_put(node);
-		count++;
-	}
 
-	if (count > 1) {
-		dev_err(dev, "Only one external encoder is supported\n");
-		return -EINVAL;
+		if (of_device_is_compatible(node, "nxp,tda998x")) {
+			if (match)
+				drm_of_component_match_add(dev, match,
+							   dev_match_of, node);
+			ret = 1;
+		}
+
+		of_node_put(node);
+		if (count++ > 1) {
+			dev_err(dev, "Only one port is supported\n");
+			return -EINVAL;
+		}
 	}
 
-	return count;
+	return ret;
 }
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_external.h b/drivers/gpu/drm/tilcdc/tilcdc_external.h
index c700e0c..a27c365 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_external.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_external.h
@@ -22,4 +22,5 @@
 void tilcdc_remove_external_encoders(struct drm_device *dev);
 int tilcdc_get_external_components(struct device *dev,
 				   struct component_match **match);
+int tilcdc_attach_remote_device(struct drm_device *ddev);
 #endif /* __TILCDC_SLAVE_H__ */
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/3] drm/tilcdc: Add drm bridge support for attaching drm bridge drivers

[Bartosz Golaszewski]

2016-11-14 17:54 GMT+01:00 Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>:
> Adds drm bride support for attaching drm bridge drivers to tilcdc. The
> decision whether a video port leads to an external encoder or bridge
> is made simply based on remote device's compatible string. The code
> has been tested with BeagleBone-Black with and without BeagleBone
> DVI-D Cape Rev A3 using ti-tfp410 driver.
>
> Signed-off-by: Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>
> ---

Hi Jyri,

thanks a lot for doing this.

One issue I see with this patch is that tilcdc doesn't seem to support
deferred probe correctly (if modules are built-in). The following
happens on my setup:

The dump-vga-dac module is loaded first, but the i2c0 is not ready yet
- probe returns EPROBE_DEFER and it's propagated to tilcdc probe.

    [drm] Initialized
    dumb-vga-dac vga_bridge: Couldn't retrieve i2c bus

Then the i2c bus is initialized and dump-vga-dac probe succeeds, but
the second probe of tilcdc gives me:

    [drm:drm_debugfs_init] *ERROR* Cannot create /sys/kernel/debug/dri/64
    [drm:drm_minor_register] *ERROR* DRM: Failed to initialize
/sys/kernel/debug/dri.
    tilcdc: probe of da8xx_lcdc.0 failed with error -1

I was able to work around this issue by loading modules in correct order.

I then tried testing the patch with a da850-lcdk, but I don't get
anything on the display (no signal), even though the LCDC seems to
work fine (modetest and dmesg messages work just like when using the
tilcdc panel). Also: I see the EDID info is correctly retrieved from
the display.

Could you take a look at my DT[1] and see if you find it correct?

Best regards,
Bartosz Golaszewski

[1] http://pastebin.com/dfUX7PyL
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] drm/tilcdc: Add drm bridge support for attaching drm bridge drivers

[Jyri Sarha]

On 11/15/16 19:36, Bartosz Golaszewski wrote:
> 2016-11-14 17:54 GMT+01:00 Jyri Sarha <jsarha@ti.com>:
>> Adds drm bride support for attaching drm bridge drivers to tilcdc. The
>> decision whether a video port leads to an external encoder or bridge
>> is made simply based on remote device's compatible string. The code
>> has been tested with BeagleBone-Black with and without BeagleBone
>> DVI-D Cape Rev A3 using ti-tfp410 driver.
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> ---
> 
> Hi Jyri,
> 
> thanks a lot for doing this.
> 
> One issue I see with this patch is that tilcdc doesn't seem to support
> deferred probe correctly (if modules are built-in). The following
> happens on my setup:
> 
> The dump-vga-dac module is loaded first, but the i2c0 is not ready yet
> - probe returns EPROBE_DEFER and it's propagated to tilcdc probe.
> 
>     [drm] Initialized
>     dumb-vga-dac vga_bridge: Couldn't retrieve i2c bus
> 
> Then the i2c bus is initialized and dump-vga-dac probe succeeds, but
> the second probe of tilcdc gives me:
> 
>     [drm:drm_debugfs_init] *ERROR* Cannot create /sys/kernel/debug/dri/64
>     [drm:drm_minor_register] *ERROR* DRM: Failed to initialize
> /sys/kernel/debug/dri.
>     tilcdc: probe of da8xx_lcdc.0 failed with error -1
> 
> I was able to work around this issue by loading modules in correct order.
> 

Did you have any conflicts when applying my patch? I have done quite a
few changes lately and especially the initialization sequence and back
off from deferred probe may get broken easily broken if the source base
is not correct. I try to come up with a pull-request candidate branch
soon (hopefully tomorrow) for you to test.

> I then tried testing the patch with a da850-lcdk, but I don't get
> anything on the display (no signal), even though the LCDC seems to
> work fine (modetest and dmesg messages work just like when using the
> tilcdc panel). Also: I see the EDID info is correctly retrieved from
> the display.
> 
> Could you take a look at my DT[1] and see if you find it correct?
> 

It is hard to follow the dts diff, but if it probes and tilcdc is able
to read EDID modes, there should not be anything more to it.

Cheers,
Jyri

> Best regards,
> Bartosz Golaszewski
> 
> [1] http://pastebin.com/dfUX7PyL
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/3] drm/tilcdc: Add drm bridge support for attaching drm bridge drivers

[Bartosz Golaszewski]

2016-11-15 21:46 GMT+01:00 Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>:
> On 11/15/16 19:36, Bartosz Golaszewski wrote:
>> 2016-11-14 17:54 GMT+01:00 Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>:
>>> Adds drm bride support for attaching drm bridge drivers to tilcdc. The
>>> decision whether a video port leads to an external encoder or bridge
>>> is made simply based on remote device's compatible string. The code
>>> has been tested with BeagleBone-Black with and without BeagleBone
>>> DVI-D Cape Rev A3 using ti-tfp410 driver.
>>>
>>> Signed-off-by: Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>
>>> ---
>>
>> Hi Jyri,
>>
>> thanks a lot for doing this.
>>
>> One issue I see with this patch is that tilcdc doesn't seem to support
>> deferred probe correctly (if modules are built-in). The following
>> happens on my setup:
>>
>> The dump-vga-dac module is loaded first, but the i2c0 is not ready yet
>> - probe returns EPROBE_DEFER and it's propagated to tilcdc probe.
>>
>>     [drm] Initialized
>>     dumb-vga-dac vga_bridge: Couldn't retrieve i2c bus
>>
>> Then the i2c bus is initialized and dump-vga-dac probe succeeds, but
>> the second probe of tilcdc gives me:
>>
>>     [drm:drm_debugfs_init] *ERROR* Cannot create /sys/kernel/debug/dri/64
>>     [drm:drm_minor_register] *ERROR* DRM: Failed to initialize
>> /sys/kernel/debug/dri.
>>     tilcdc: probe of da8xx_lcdc.0 failed with error -1
>>
>> I was able to work around this issue by loading modules in correct order.
>>
>
> Did you have any conflicts when applying my patch? I have done quite a
> few changes lately and especially the initialization sequence and back
> off from deferred probe may get broken easily broken if the source base
> is not correct. I try to come up with a pull-request candidate branch
> soon (hopefully tomorrow) for you to test.
>

I only had some trivial conflicts, but you're right, maybe I'm missing
some parts. It would be great to have a branch for testing - I could
then rebase my follow-up work on tilcdc against it.

>> I then tried testing the patch with a da850-lcdk, but I don't get
>> anything on the display (no signal), even though the LCDC seems to
>> work fine (modetest and dmesg messages work just like when using the
>> tilcdc panel). Also: I see the EDID info is correctly retrieved from
>> the display.
>>
>> Could you take a look at my DT[1] and see if you find it correct?
>>
>
> It is hard to follow the dts diff, but if it probes and tilcdc is able
> to read EDID modes, there should not be anything more to it.
>

Yes, this is what I thought too.

Let me know when you'll have the testing branch ready.

Best regards,
Bartosz Golaszewski
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html