[PATCH 0/4] omapdss: Add video output support for gta04 board

[PATCH 0/4] omapdss: Add video output support for gta04 board

[Marek Belisko]

This patch series add support for video output on gta04 board. It consist
from opa362 video amplifier driver + DT bindings, adding venc -> opa362 -> svideo-connecor
setup in DT and finally static setup for tvbypass anc acbias bin in devconf1 register
via pinctrl subsystem.

Marek Belisko (4):
  video: omapdss: Add opa362 driver
  Documentation: DT: Add documentation for ti,opa362 bindings
  arm: dts: omap3-gta04: Add handling for tv output
  arm: dts: omap3-gta04: Add static configuration for devconf1 register

 .../devicetree/bindings/video/ti,opa362.txt        |  38 +++
 arch/arm/boot/dts/omap3-gta04.dtsi                 |  70 +++++
 drivers/video/fbdev/omap2/displays-new/Kconfig     |   6 +
 drivers/video/fbdev/omap2/displays-new/Makefile    |   1 +
 .../fbdev/omap2/displays-new/amplifier-opa362.c    | 347 +++++++++++++++++++++
 5 files changed, 462 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/ti,opa362.txt
 create mode 100644 drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c

-- 
1.9.1

[PATCH 1/4] video: omapdss: Add opa362 driver

[Marek Belisko]

opa362 is amplifier for videoand can be connected to the tvout pads
of the OMAP3. It has one gpio control for enable/disable of the output
(high impedance).

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/video/fbdev/omap2/displays-new/Kconfig     |   6 +
 drivers/video/fbdev/omap2/displays-new/Makefile    |   1 +
 .../fbdev/omap2/displays-new/amplifier-opa362.c    | 347 +++++++++++++++++++++
 3 files changed, 354 insertions(+)
 create mode 100644 drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c

diff --git a/drivers/video/fbdev/omap2/displays-new/Kconfig b/drivers/video/fbdev/omap2/displays-new/Kconfig
index e6cfc38..211b3ec 100644
--- a/drivers/video/fbdev/omap2/displays-new/Kconfig
+++ b/drivers/video/fbdev/omap2/displays-new/Kconfig
@@ -1,6 +1,12 @@
 menu "OMAP Display Device Drivers (new device model)"
         depends on OMAP2_DSS
 
+config DISPLAY_AMPLIFIER_OPA362
+        tristate "external analog amplifier with output disable/high-Z (e.g. OPA362)"
+	help
+	  Driver to enable an external analog TV amplifier (e.g. OPA362)
+	  through a GPIO.
+
 config DISPLAY_ENCODER_TFP410
         tristate "TFP410 DPI to DVI Encoder"
 	help
diff --git a/drivers/video/fbdev/omap2/displays-new/Makefile b/drivers/video/fbdev/omap2/displays-new/Makefile
index 0323a8a..b311542 100644
--- a/drivers/video/fbdev/omap2/displays-new/Makefile
+++ b/drivers/video/fbdev/omap2/displays-new/Makefile
@@ -1,3 +1,4 @@
+obj-$(CONFIG_DISPLAY_AMPLIFIER_OPA362) += amplifier-opa362.o
 obj-$(CONFIG_DISPLAY_ENCODER_TFP410) += encoder-tfp410.o
 obj-$(CONFIG_DISPLAY_ENCODER_TPD12S015) += encoder-tpd12s015.o
 obj-$(CONFIG_DISPLAY_CONNECTOR_DVI) += connector-dvi.o
diff --git a/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
new file mode 100644
index 0000000..37b2443
--- /dev/null
+++ b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
@@ -0,0 +1,347 @@
+/*
+ * OPA362 analog video amplifier with output/power control
+ *
+ * Copyright (C) 2014 Golden Delicious Computers
+ * Author: H. Nikolaus Schaller <hns@goldelico.com>
+ *
+ * based on encoder-tfp410
+ *
+ * Copyright (C) 2013 Texas Instruments
+ * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
+ *
+ * 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/gpio.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/of_gpio.h>
+
+#include <video/omapdss.h>
+#include <video/omap-panel-data.h>
+
+struct panel_drv_data {
+	struct omap_dss_device dssdev;
+	struct omap_dss_device *in;
+
+	int enable_gpio;
+	bool bypass;
+	bool acbias;
+
+	struct omap_video_timings timings;
+};
+
+#define to_panel_data(x) container_of(x, struct panel_drv_data, dssdev)
+
+static int opa362_connect(struct omap_dss_device *dssdev,
+		struct omap_dss_device *dst)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+	int r;
+
+	dev_dbg(dssdev->dev, "connect\n");
+
+	if (omapdss_device_is_connected(dssdev))
+		return -EBUSY;
+
+	r = in->ops.atv->connect(in, dssdev);
+	if (r)
+		return r;
+
+	dst->src = dssdev;
+	dssdev->dst = dst;
+
+	return 0;
+}
+
+static void opa362_disconnect(struct omap_dss_device *dssdev,
+		struct omap_dss_device *dst)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+
+	dev_dbg(dssdev->dev, "disconnect\n");
+
+	WARN_ON(!omapdss_device_is_connected(dssdev));
+	if (!omapdss_device_is_connected(dssdev))
+		return;
+
+	WARN_ON(dst != dssdev->dst);
+	if (dst != dssdev->dst)
+		return;
+
+	dst->src = NULL;
+	dssdev->dst = NULL;
+
+	in->ops.atv->disconnect(in, &ddata->dssdev);
+}
+
+static int opa362_enable(struct omap_dss_device *dssdev)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+	int r;
+
+	dev_dbg(dssdev->dev, "enable\n");
+
+	if (!omapdss_device_is_connected(dssdev))
+		return -ENODEV;
+
+	if (omapdss_device_is_enabled(dssdev))
+		return 0;
+
+	in->ops.atv->set_timings(in, &ddata->timings);
+	/* fixme: should we receive the invert from our consumer, i.e. the connector? */
+	in->ops.atv->invert_vid_out_polarity(in, true);
+
+	r = in->ops.atv->enable(in);
+	if (r)
+		return r;
+
+	if (gpio_is_valid(ddata->enable_gpio))
+		gpio_set_value_cansleep(ddata->enable_gpio, 1);
+
+	dssdev->state = OMAP_DSS_DISPLAY_ACTIVE;
+
+	return 0;
+}
+
+static void opa362_disable(struct omap_dss_device *dssdev)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+
+	dev_dbg(dssdev->dev, "disable\n");
+
+	if (!omapdss_device_is_enabled(dssdev))
+		return;
+
+	if (gpio_is_valid(ddata->enable_gpio))
+		gpio_set_value_cansleep(ddata->enable_gpio, 0);
+
+	in->ops.atv->disable(in);
+
+	dssdev->state = OMAP_DSS_DISPLAY_DISABLED;
+}
+
+static void opa362_set_timings(struct omap_dss_device *dssdev,
+		struct omap_video_timings *timings)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+
+	dev_dbg(dssdev->dev, "set_timings\n");
+
+	ddata->timings = *timings;
+	dssdev->panel.timings = *timings;
+
+	in->ops.atv->set_timings(in, timings);
+}
+
+static void opa362_get_timings(struct omap_dss_device *dssdev,
+		struct omap_video_timings *timings)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+
+	dev_dbg(dssdev->dev, "get_timings\n");
+
+	*timings = ddata->timings;
+}
+
+static int opa362_check_timings(struct omap_dss_device *dssdev,
+		struct omap_video_timings *timings)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+
+	dev_dbg(dssdev->dev, "check_timings\n");
+
+	return in->ops.atv->check_timings(in, timings);
+}
+
+static void opa362_set_type(struct omap_dss_device *dssdev,
+		enum omap_dss_venc_type type)
+{
+	/* we can only drive a COMPOSITE output */
+	WARN_ON(type != OMAP_DSS_VENC_TYPE_COMPOSITE);
+
+}
+
+static void opa362_invert_vid_out_polarity(struct omap_dss_device *dssdev,
+		bool invert_polarity)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+
+	/* OPA362 inverts the polarity */
+	in->ops.atv->invert_vid_out_polarity(in, !invert_polarity);
+}
+
+static const struct omapdss_atv_ops opa362_atv_ops = {
+	.connect	= opa362_connect,
+	.disconnect	= opa362_disconnect,
+
+	.enable		= opa362_enable,
+	.disable	= opa362_disable,
+
+	.check_timings	= opa362_check_timings,
+	.set_timings	= opa362_set_timings,
+	.get_timings	= opa362_get_timings,
+
+	.set_type	= opa362_set_type,
+	.invert_vid_out_polarity	= opa362_invert_vid_out_polarity,
+};
+
+static int opa362_probe_pdata(struct platform_device *pdev)
+{
+	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
+	struct amplifier_opa362_platform_data *pdata;
+	struct omap_dss_device *dssdev, *in;
+
+	pdata = dev_get_platdata(&pdev->dev);
+
+	ddata->enable_gpio = pdata->enable_gpio;
+	ddata->bypass = pdata->bypass;
+	ddata->acbias = pdata->acbias;
+
+	in = omap_dss_find_output(pdata->source);
+	if (in == NULL) {
+		dev_err(&pdev->dev, "Failed to find video source\n");
+		return -ENODEV;
+	}
+
+	ddata->in = in;
+
+	dssdev = &ddata->dssdev;
+	dssdev->name = pdata->name;
+
+	return 0;
+}
+
+static int opa362_probe_of(struct platform_device *pdev)
+{
+	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
+	struct device_node *node = pdev->dev.of_node;
+	struct omap_dss_device *in;
+	int gpio;
+
+	gpio = of_get_gpio(node, 0);
+
+	if (gpio_is_valid(gpio) || gpio == -ENOENT) {
+		ddata->enable_gpio = gpio;
+	} else {
+		dev_err(&pdev->dev, "failed to parse enable gpio\n");
+		return gpio;
+	}
+
+	in = omapdss_of_find_source_for_first_ep(node);
+	if (IS_ERR(in)) {
+		dev_err(&pdev->dev, "failed to find video source\n");
+		return PTR_ERR(in);
+	}
+
+	ddata->in = in;
+
+	return 0;
+}
+
+static int opa362_probe(struct platform_device *pdev)
+{
+	struct panel_drv_data *ddata;
+	struct omap_dss_device *dssdev;
+	int r;
+
+	dev_dbg(&pdev->dev, "probe\n");
+
+	ddata = devm_kzalloc(&pdev->dev, sizeof(*ddata), GFP_KERNEL);
+	if (!ddata)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, ddata);
+
+	if (dev_get_platdata(&pdev->dev)) {
+		r = opa362_probe_pdata(pdev);
+		if (r)
+			return r;
+	} else {
+		r = opa362_probe_of(pdev);
+		if (r)
+			return r;
+	}
+
+	if (gpio_is_valid(ddata->enable_gpio)) {
+		r = devm_gpio_request_one(&pdev->dev, ddata->enable_gpio,
+				GPIOF_OUT_INIT_LOW, "opa362 enable");
+		if (r) {
+			dev_err(&pdev->dev, "Failed to request enable GPIO %d\n",
+					ddata->enable_gpio);
+			goto err_gpio;
+		}
+	}
+
+	dssdev = &ddata->dssdev;
+	dssdev->ops.atv = &opa362_atv_ops;
+	dssdev->dev = &pdev->dev;
+	dssdev->type = OMAP_DISPLAY_TYPE_VENC;
+	dssdev->output_type = OMAP_DISPLAY_TYPE_VENC;
+	dssdev->owner = THIS_MODULE;
+
+	r = omapdss_register_output(dssdev);
+	if (r) {
+		dev_err(&pdev->dev, "Failed to register output\n");
+		goto err_reg;
+	}
+
+	return 0;
+err_reg:
+err_gpio:
+	omap_dss_put_device(ddata->in);
+	return r;
+}
+
+static int __exit opa362_remove(struct platform_device *pdev)
+{
+	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
+	struct omap_dss_device *dssdev = &ddata->dssdev;
+	struct omap_dss_device *in = ddata->in;
+
+	omapdss_unregister_output(&ddata->dssdev);
+
+	WARN_ON(omapdss_device_is_enabled(dssdev));
+	if (omapdss_device_is_enabled(dssdev))
+		opa362_disable(dssdev);
+
+	WARN_ON(omapdss_device_is_connected(dssdev));
+	if (omapdss_device_is_connected(dssdev))
+		opa362_disconnect(dssdev, dssdev->dst);
+
+	omap_dss_put_device(in);
+
+	return 0;
+}
+
+static const struct of_device_id opa362_of_match[] = {
+	{ .compatible = "omapdss,ti,opa362", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, opa362_of_match);
+
+static struct platform_driver opa362_driver = {
+	.probe	= opa362_probe,
+	.remove	= __exit_p(opa362_remove),
+	.driver	= {
+		.name	= "amplifier-opa362",
+		.owner	= THIS_MODULE,
+		.of_match_table = opa362_of_match,
+	},
+};
+
+module_platform_driver(opa362_driver);
+
+MODULE_AUTHOR("H. Nikolaus Schaller <hns@goldelico.com>");
+MODULE_DESCRIPTION("OPA362 analog video amplifier with output/power control");
+MODULE_LICENSE("GPL");
-- 
1.9.1

[PATCH 2/4] Documentation: DT: Add documentation for ti,opa362 bindings

[Marek Belisko]

Signed-off-by: Marek Belisko <marek@goldelico.com>
---
 .../devicetree/bindings/video/ti,opa362.txt        | 38 ++++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/ti,opa362.txt

diff --git a/Documentation/devicetree/bindings/video/ti,opa362.txt b/Documentation/devicetree/bindings/video/ti,opa362.txt
new file mode 100644
index 0000000..d7ed11a
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/ti,opa362.txt
@@ -0,0 +1,38 @@
+OPA362 analog video amplifier
+
+Required properties:
+- compatible: "ti,opa362"
+- gpio: enable/disable output gpio
+
+Required node:
+- Video port 0 for opa362 input
+- Video port 1 for opa362 output
+
+Example:
+
+tv_amp: opa362 {
+	 compatible = "ti,opa362";
+	 gpios = <&gpio1 23 0>;  /* GPIO to enable video out amplifier */
+	 
+	label = "opa362";
+	 ports {
+		 #address-cells = <1>;
+		 #size-cells = <0>;
+		 port@0 {
+			 reg = <0>;
+			 opa_in: endpoint@0 {
+				 remote-endpoint = <&venc_out>;
+			 };
+		 };
+
+		 port@1 {
+			 reg = <1>;
+			 opa_out: endpoint@0 {
+				 remote-endpoint = <&tv_connector_in>;
+			 };
+		 };
+	 };
+ };
+
+
+
-- 
1.9.1

[PATCH 3/4] arm: dts: omap3-gta04: Add handling for tv output

[Marek Belisko]

Add handling for gta04 tv out chain:
venc -> opa362 -> svideo

Signed-off-by: Marek Belisko <marek@goldelico.com>
---
 arch/arm/boot/dts/omap3-gta04.dtsi | 48 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
index fd34f91..e4d05f0 100644
--- a/arch/arm/boot/dts/omap3-gta04.dtsi
+++ b/arch/arm/boot/dts/omap3-gta04.dtsi
@@ -83,6 +83,41 @@
 		compatible = "usb-nop-xceiv";
 		reset-gpios = <&gpio6 14 GPIO_ACTIVE_LOW>;
 	};
+
+	tv0: connector@1 {
+		compatible = "svideo-connector";
+		label = "tv";
+
+		port {
+			tv_connector_in: endpoint {
+				remote-endpoint = <&opa_out>;
+			};
+		};
+	};
+
+	tv_amp: opa362 {
+		compatible = "ti,opa362";
+		gpios = <&gpio1 23 0>;
+
+		label = "opa362";
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			port@0 {
+				reg = <0>;
+				opa_in: endpoint@0 {
+					remote-endpoint = <&venc_out>;
+				};
+			};
+
+			port@1 {
+				reg = <1>;
+				opa_out: endpoint@0 {
+					remote-endpoint = <&tv_connector_in>;
+				};
+			};
+		};
+	};
 };
 
 &omap3_pmx_core {
@@ -396,6 +431,19 @@
 	};
 };
 
+&venc {
+	status = "okay";
+
+	vdda-supply = <&vdac>;
+
+	port {
+		venc_out: endpoint {
+			remote-endpoint = <&opa_in>;
+			ti,channels = <2>;
+		};
+	};
+};
+
 &gpmc {
 	ranges = <0 0 0x30000000 0x04>; /* CS0: NAND */
 
-- 
1.9.1

[PATCH 4/4] arm: dts: omap3-gta04: Add static configuration for devconf1 register

[Marek Belisko]

gta04 board need for tvout enabled 2 bits in devconf1 register (tvbypass and acbias).
Add single pinmux entry and enable it.

Signed-off-by: Marek Belisko <marek@goldelico.com>
---
 arch/arm/boot/dts/omap3-gta04.dtsi | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
index e4d05f0..a456d37 100644
--- a/arch/arm/boot/dts/omap3-gta04.dtsi
+++ b/arch/arm/boot/dts/omap3-gta04.dtsi
@@ -118,6 +118,17 @@
 			};
 		};
 	};
+
+	/* pinmux for devconf1 */
+	control_devconf1: pinmux@480022d8 {
+		compatible = "pinctrl-single";
+		reg = <0x480022d8 4>;	/* single register */
+		#address-cells = <1>;
+		#size-cells = <0>;
+		pinctrl-single,bit-per-mux;
+		pinctrl-single,register-width = <32>;
+		pinctrl-single,function-mask = <0xfc0bd5>;
+	};
 };
 
 &omap3_pmx_core {
@@ -497,3 +508,14 @@
 		};
 	};
 };
+
+&control_devconf1 {
+	pinctrl-name = "default";
+	pinctrl-0 = < &tv_acbias_pins>;
+
+	tv_acbias_pins: pinmux_tv_acbias_pins {
+		pinctrl-single,bits = <
+			0 0x40800 0x40800
+		>;
+	};
+};
-- 
1.9.1

Re: [PATCH 3/4] arm: dts: omap3-gta04: Add handling for tv output

[Tony Lindgren]

* Marek Belisko <marek@goldelico.com> [141103 14:01]:
> Add handling for gta04 tv out chain:
> venc -> opa362 -> svideo
> 
> Signed-off-by: Marek Belisko <marek@goldelico.com>
> ---
>  arch/arm/boot/dts/omap3-gta04.dtsi | 48 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)

Applying this patch into omap-for-v3.19/dt thanks.

Tony

Re: [PATCH 4/4] arm: dts: omap3-gta04: Add static configuration for devconf1 register

[Tony Lindgren]

* Marek Belisko <marek@goldelico.com> [141103 14:01]:
> gta04 board need for tvout enabled 2 bits in devconf1 register (tvbypass and acbias).
> Add single pinmux entry and enable it.
> 
> Signed-off-by: Marek Belisko <marek@goldelico.com>
> ---
>  arch/arm/boot/dts/omap3-gta04.dtsi | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
> index e4d05f0..a456d37 100644
> --- a/arch/arm/boot/dts/omap3-gta04.dtsi
> +++ b/arch/arm/boot/dts/omap3-gta04.dtsi
> @@ -118,6 +118,17 @@
>  			};
>  		};
>  	};
> +
> +	/* pinmux for devconf1 */
> +	control_devconf1: pinmux@480022d8 {
> +		compatible = "pinctrl-single";
> +		reg = <0x480022d8 4>;	/* single register */
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		pinctrl-single,bit-per-mux;
> +		pinctrl-single,register-width = <32>;
> +		pinctrl-single,function-mask = <0xfc0bd5>;
> +	};
>  };
>  

The pinctrl-single entry should be in omap3.dtsi as the mux register is there
for all the omap3 devices, can you please update the patch for that?

Regards,

Tony

>  &omap3_pmx_core {
> @@ -497,3 +508,14 @@
>  		};
>  	};
>  };
> +
> +&control_devconf1 {
> +	pinctrl-name = "default";
> +	pinctrl-0 = < &tv_acbias_pins>;
> +
> +	tv_acbias_pins: pinmux_tv_acbias_pins {
> +		pinctrl-single,bits = <
> +			0 0x40800 0x40800
> +		>;
> +	};
> +};
> -- 
> 1.9.1
> 

Re: [PATCH 3/4] arm: dts: omap3-gta04: Add handling for tv output

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 570 bytes --]

Tony,

On 11/11/14 01:30, Tony Lindgren wrote:
> * Marek Belisko <marek@goldelico.com> [141103 14:01]:
>> Add handling for gta04 tv out chain:
>> venc -> opa362 -> svideo
>>
>> Signed-off-by: Marek Belisko <marek@goldelico.com>
>> ---
>>  arch/arm/boot/dts/omap3-gta04.dtsi | 48 ++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 48 insertions(+)
> 
> Applying this patch into omap-for-v3.19/dt thanks.

There are changes needed for this series, so don't apply yet.

I'll try to find the time to do a review and send comments soon.

 Tomi



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

Re: [PATCH 3/4] arm: dts: omap3-gta04: Add handling for tv output

[Tony Lindgren]

* Tomi Valkeinen <tomi.valkeinen@ti.com> [141110 23:44]:
> Tony,
> 
> On 11/11/14 01:30, Tony Lindgren wrote:
> > * Marek Belisko <marek@goldelico.com> [141103 14:01]:
> >> Add handling for gta04 tv out chain:
> >> venc -> opa362 -> svideo
> >>
> >> Signed-off-by: Marek Belisko <marek@goldelico.com>
> >> ---
> >>  arch/arm/boot/dts/omap3-gta04.dtsi | 48 ++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 48 insertions(+)
> > 
> > Applying this patch into omap-for-v3.19/dt thanks.
> 
> There are changes needed for this series, so don't apply yet.
> 
> I'll try to find the time to do a review and send comments soon.

I've already pushed out the omap-for-v3.19/dt branch with
this patch buried in it. I'd rather not start redoing the
branch if the $subject patch is mostly correct and usable
with changes to the driver patches.

However, if the $subject patch needs to be redone for
the driver changes, let me know and I'll redo the branch.

Regards,

Tony

Re: [PATCH 4/4] arm: dts: omap3-gta04: Add static configuration for devconf1 register

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2183 bytes --]

Hi,

On 03/11/14 23:59, Marek Belisko wrote:
> gta04 board need for tvout enabled 2 bits in devconf1 register (tvbypass and acbias).
> Add single pinmux entry and enable it.
> 
> Signed-off-by: Marek Belisko <marek@goldelico.com>
> ---
>  arch/arm/boot/dts/omap3-gta04.dtsi | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
> index e4d05f0..a456d37 100644
> --- a/arch/arm/boot/dts/omap3-gta04.dtsi
> +++ b/arch/arm/boot/dts/omap3-gta04.dtsi
> @@ -118,6 +118,17 @@
>  			};
>  		};
>  	};
> +
> +	/* pinmux for devconf1 */
> +	control_devconf1: pinmux@480022d8 {
> +		compatible = "pinctrl-single";
> +		reg = <0x480022d8 4>;	/* single register */
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		pinctrl-single,bit-per-mux;
> +		pinctrl-single,register-width = <32>;
> +		pinctrl-single,function-mask = <0xfc0bd5>;
> +	};
>  };
>  
>  &omap3_pmx_core {
> @@ -497,3 +508,14 @@
>  		};
>  	};
>  };
> +
> +&control_devconf1 {
> +	pinctrl-name = "default";
> +	pinctrl-0 = < &tv_acbias_pins>;
> +
> +	tv_acbias_pins: pinmux_tv_acbias_pins {
> +		pinctrl-single,bits = <
> +			0 0x40800 0x40800

I think it would be good to have a comment in the .dts above, mentioning
that TVOUTBYPASS and TVACEN bits are being set.

> +		>;
> +	};
> +};
> 

OMAP3630 seems to have CONTROL_AVDAC1 and CONTROL_AVDAC2 registers. Did
you check if the SoC you use have those? It looks like they need
configuration also, if the exist.

So, I don't think tvbypass and acbias are really pinmux stuff, but it
does seem like an easy way to handle the devconf1 register, and I don't
see any issues with the setting being fixed.

However, devconf1 register seems to have bits for many devices,
including mcbsp, mmc, and even some "Force MPU writes to be nonposted" bit.

And, with a quick grep, I see CONTROL_DEVCONF1 touched in multiple
places in the kernel. I wonder if adding a pinmux entry for it could
cause some rather odd problems.

Tony, any idea about this? How should CONTROL_DEVCONFx registers be
accessed?

 Tomi



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

Re: [PATCH 1/4] video: omapdss: Add opa362 driver

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 915 bytes --]

Hi,


On 03/11/14 23:59, Marek Belisko wrote:
> opa362 is amplifier for videoand can be connected to the tvout pads
> of the OMAP3. It has one gpio control for enable/disable of the output
> (high impedance).
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  drivers/video/fbdev/omap2/displays-new/Kconfig     |   6 +
>  drivers/video/fbdev/omap2/displays-new/Makefile    |   1 +
>  .../fbdev/omap2/displays-new/amplifier-opa362.c    | 347 +++++++++++++++++++++
>  3 files changed, 354 insertions(+)
>  create mode 100644 drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c

This doesn't even compile:

drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c:207:28: error:
dereferencing pointer to incomplete type
  ddata->enable_gpio = pdata->enable_gpio;

And it seems to have fields for bypass and bypass which are handled in
the devconf1 patch.

 Tomi



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

Re: [PATCH 2/4] Documentation: DT: Add documentation for ti,opa362 bindings

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1196 bytes --]

On 03/11/14 23:59, Marek Belisko wrote:
> Signed-off-by: Marek Belisko <marek@goldelico.com>
> ---
>  .../devicetree/bindings/video/ti,opa362.txt        | 38 ++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/ti,opa362.txt
> 
> diff --git a/Documentation/devicetree/bindings/video/ti,opa362.txt b/Documentation/devicetree/bindings/video/ti,opa362.txt
> new file mode 100644
> index 0000000..d7ed11a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/ti,opa362.txt
> @@ -0,0 +1,38 @@
> +OPA362 analog video amplifier
> +
> +Required properties:
> +- compatible: "ti,opa362"
> +- gpio: enable/disable output gpio
> +
> +Required node:
> +- Video port 0 for opa362 input
> +- Video port 1 for opa362 output
> +
> +Example:
> +
> +tv_amp: opa362 {
> +	 compatible = "ti,opa362";
> +	 gpios = <&gpio1 23 0>;  /* GPIO to enable video out amplifier */
> +	 
> +	label = "opa362";

opa shouldn't have label property. label is meant for the
end-of-the-chain component, like the connector.

> +	 ports {

Hmm, I think there is extra space before 'ports', maybe below also.

 Tomi



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

Re: [PATCH 1/4] video: omapdss: Add opa362 driver

[Belisko Marek]

Hi Tomi,

On Wed, Nov 12, 2014 at 2:57 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Hi,
>
>
> On 03/11/14 23:59, Marek Belisko wrote:
>> opa362 is amplifier for videoand can be connected to the tvout pads
>> of the OMAP3. It has one gpio control for enable/disable of the output
>> (high impedance).
>>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>>  drivers/video/fbdev/omap2/displays-new/Kconfig     |   6 +
>>  drivers/video/fbdev/omap2/displays-new/Makefile    |   1 +
>>  .../fbdev/omap2/displays-new/amplifier-opa362.c    | 347 +++++++++++++++++++++
>>  3 files changed, 354 insertions(+)
>>  create mode 100644 drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>
> This doesn't even compile:
>
> drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c:207:28: error:
> dereferencing pointer to incomplete type
>   ddata->enable_gpio = pdata->enable_gpio;
Hmm sorry about that. I must mixed something up. I'll post updated version soon.
Thanks for review.
>
> And it seems to have fields for bypass and bypass which are handled in
> the devconf1 patch.
>
>  Tomi
>
>

BR,

marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
twitter: #opennandra
web: http://open-nandra.com

Re: [PATCH 4/4] arm: dts: omap3-gta04: Add static configuration for devconf1 register

[Tony Lindgren]

* Tomi Valkeinen <tomi.valkeinen@ti.com> [141112 04:56]:
> Hi,
> 
> On 03/11/14 23:59, Marek Belisko wrote:
> > gta04 board need for tvout enabled 2 bits in devconf1 register (tvbypass and acbias).
> > Add single pinmux entry and enable it.
> > 
> > Signed-off-by: Marek Belisko <marek@goldelico.com>
> > ---
> >  arch/arm/boot/dts/omap3-gta04.dtsi | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
> > index e4d05f0..a456d37 100644
> > --- a/arch/arm/boot/dts/omap3-gta04.dtsi
> > +++ b/arch/arm/boot/dts/omap3-gta04.dtsi
> > @@ -118,6 +118,17 @@
> >  			};
> >  		};
> >  	};
> > +
> > +	/* pinmux for devconf1 */
> > +	control_devconf1: pinmux@480022d8 {
> > +		compatible = "pinctrl-single";
> > +		reg = <0x480022d8 4>;	/* single register */
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +		pinctrl-single,bit-per-mux;
> > +		pinctrl-single,register-width = <32>;
> > +		pinctrl-single,function-mask = <0xfc0bd5>;
> > +	};
> >  };
> >  
> >  &omap3_pmx_core {
> > @@ -497,3 +508,14 @@
> >  		};
> >  	};
> >  };
> > +
> > +&control_devconf1 {
> > +	pinctrl-name = "default";
> > +	pinctrl-0 = < &tv_acbias_pins>;
> > +
> > +	tv_acbias_pins: pinmux_tv_acbias_pins {
> > +		pinctrl-single,bits = <
> > +			0 0x40800 0x40800
> 
> I think it would be good to have a comment in the .dts above, mentioning
> that TVOUTBYPASS and TVACEN bits are being set.
> 
> > +		>;
> > +	};
> > +};
> > 
> 
> OMAP3630 seems to have CONTROL_AVDAC1 and CONTROL_AVDAC2 registers. Did
> you check if the SoC you use have those? It looks like they need
> configuration also, if the exist.

Those look like AVDAC specific control registers that are not mux
registers. So those should be accessed the existing SCM (System Control
Mmodule) syscon area by the DSS code. For examples, see what
pbias-regulator.c is doing for some other registers in the syscon area.
 
> So, I don't think tvbypass and acbias are really pinmux stuff, but it
> does seem like an easy way to handle the devconf1 register, and I don't
> see any issues with the setting being fixed.

The CONTROL_DEVCONF registers seem to be all related to muxing signals
and configuring ping signal levels. So I think the pinctrl-single is
OK to use with these.
 
> However, devconf1 register seems to have bits for many devices,
> including mcbsp, mmc, and even some "Force MPU writes to be nonposted" bit.

Yes the"Force MPU writes to be nonposted" debug bit is an odd one
there :) But we're not using that luckily anywhere..
 
> And, with a quick grep, I see CONTROL_DEVCONF1 touched in multiple
> places in the kernel. I wonder if adding a pinmux entry for it could
> cause some rather odd problems.

They can all use pinctrl-single no problem.
 
> Tony, any idea about this? How should CONTROL_DEVCONFx registers be
> accessed?

If they are pinctrl related like the CONTROL_DEVCONF registers, then
pincatrl-single is OK. However, for any registers in the SCM that are
not just routing signals, then the syscon mapping should be used. And
we should have a separate driver implementing some standard Linux
generic framework driver. For example a regulator or clock driver.

Anyways, I'll drop this $subject patch for now and set up a new branch
for the .dts changes.

Regards,

Tony

Re: [PATCH 4/4] arm: dts: omap3-gta04: Add static configuration for devconf1 register

[Belisko Marek]

Hi Tony,

On Tue, Nov 11, 2014 at 12:36 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Marek Belisko <marek@goldelico.com> [141103 14:01]:
>> gta04 board need for tvout enabled 2 bits in devconf1 register (tvbypass and acbias).
>> Add single pinmux entry and enable it.
>>
>> Signed-off-by: Marek Belisko <marek@goldelico.com>
>> ---
>>  arch/arm/boot/dts/omap3-gta04.dtsi | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
>> index e4d05f0..a456d37 100644
>> --- a/arch/arm/boot/dts/omap3-gta04.dtsi
>> +++ b/arch/arm/boot/dts/omap3-gta04.dtsi
>> @@ -118,6 +118,17 @@
>>                       };
>>               };
>>       };
>> +
>> +     /* pinmux for devconf1 */
>> +     control_devconf1: pinmux@480022d8 {
>> +             compatible = "pinctrl-single";
>> +             reg = <0x480022d8 4>;   /* single register */
>> +             #address-cells = <1>;
>> +             #size-cells = <0>;
>> +             pinctrl-single,bit-per-mux;
>> +             pinctrl-single,register-width = <32>;
>> +             pinctrl-single,function-mask = <0xfc0bd5>;
>> +     };
>>  };
>>
>
> The pinctrl-single entry should be in omap3.dtsi as the mux register is there
> for all the omap3 devices, can you please update the patch for that?
Ok I'll do. Just one question. I checked TRM for omap3430 and omap3630 and
reserved bits in devconf1 are different. So keep function-mask for
omap3430 in omap3.dtsi
and redefine in omap36xx.dtsi (not sure if this will work in this
way)? Or exist other way how to deal with that? Thanks.
>
> Regards,
>
> Tony
>
>>  &omap3_pmx_core {
>> @@ -497,3 +508,14 @@
>>               };
>>       };
>>  };
>> +
>> +&control_devconf1 {
>> +     pinctrl-name = "default";
>> +     pinctrl-0 = < &tv_acbias_pins>;
>> +
>> +     tv_acbias_pins: pinmux_tv_acbias_pins {
>> +             pinctrl-single,bits = <
>> +                     0 0x40800 0x40800
>> +             >;
>> +     };
>> +};
>> --
>> 1.9.1
>>

BR,

marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
twitter: #opennandra
web: http://open-nandra.com

Re: [PATCH 4/4] arm: dts: omap3-gta04: Add static configuration for devconf1 register

[Tony Lindgren]

* Belisko Marek <marek.belisko@gmail.com> [141112 13:04]:
> Hi Tony,
> 
> On Tue, Nov 11, 2014 at 12:36 AM, Tony Lindgren <tony@atomide.com> wrote:
> > * Marek Belisko <marek@goldelico.com> [141103 14:01]:
> >> gta04 board need for tvout enabled 2 bits in devconf1 register (tvbypass and acbias).
> >> Add single pinmux entry and enable it.
> >>
> >> Signed-off-by: Marek Belisko <marek@goldelico.com>
> >> ---
> >>  arch/arm/boot/dts/omap3-gta04.dtsi | 22 ++++++++++++++++++++++
> >>  1 file changed, 22 insertions(+)
> >>
> >> diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
> >> index e4d05f0..a456d37 100644
> >> --- a/arch/arm/boot/dts/omap3-gta04.dtsi
> >> +++ b/arch/arm/boot/dts/omap3-gta04.dtsi
> >> @@ -118,6 +118,17 @@
> >>                       };
> >>               };
> >>       };
> >> +
> >> +     /* pinmux for devconf1 */
> >> +     control_devconf1: pinmux@480022d8 {
> >> +             compatible = "pinctrl-single";
> >> +             reg = <0x480022d8 4>;   /* single register */
> >> +             #address-cells = <1>;
> >> +             #size-cells = <0>;
> >> +             pinctrl-single,bit-per-mux;
> >> +             pinctrl-single,register-width = <32>;
> >> +             pinctrl-single,function-mask = <0xfc0bd5>;
> >> +     };
> >>  };
> >>
> >
> > The pinctrl-single entry should be in omap3.dtsi as the mux register is there
> > for all the omap3 devices, can you please update the patch for that?
> Ok I'll do. Just one question. I checked TRM for omap3430 and omap3630 and
> reserved bits in devconf1 are different. So keep function-mask for
> omap3430 in omap3.dtsi
> and redefine in omap36xx.dtsi (not sure if this will work in this
> way)? Or exist other way how to deal with that? Thanks.

Oh OK. Yes if they are different you should have the common entry in
omap3.dtsi and the SoC specific fields in omap34xx.dtsi and
omap36xx.dtsi. And please leave out the bit for configuring the
MPUFORCEWRNP.

Regards,

Tony

Re: [PATCH 4/4] arm: dts: omap3-gta04: Add static configuration for devconf1 register

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 728 bytes --]

On 12/11/14 17:02, Tony Lindgren wrote:

>> And, with a quick grep, I see CONTROL_DEVCONF1 touched in multiple
>> places in the kernel. I wonder if adding a pinmux entry for it could
>> cause some rather odd problems.
> 
> They can all use pinctrl-single no problem.

Can, but don't. That's my worry. If we touch the DEVCONF1 via pinmux,
and we have code in mach-omap2 that also touch DEVCONF1, without any
knowledge (and locking) between those...

So _maybe_ that's not an issue, as the pinmux config we have here is
fixed, and done once at boot time, and maybe the code in mach-omap2 that
touch DEVCONF1 is also ran just once and not at the same time as the
pinmux. But I don't know if that's so.

 Tomi



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

Re: [PATCH 4/4] arm: dts: omap3-gta04: Add static configuration for devconf1 register

[Tony Lindgren]

* Tomi Valkeinen <tomi.valkeinen@ti.com> [141113 03:33]:
> On 12/11/14 17:02, Tony Lindgren wrote:
> 
> >> And, with a quick grep, I see CONTROL_DEVCONF1 touched in multiple
> >> places in the kernel. I wonder if adding a pinmux entry for it could
> >> cause some rather odd problems.
> > 
> > They can all use pinctrl-single no problem.
> 
> Can, but don't. That's my worry. If we touch the DEVCONF1 via pinmux,
> and we have code in mach-omap2 that also touch DEVCONF1, without any
> knowledge (and locking) between those...

Hmm yeah the McBSP clock mux could be racy as the mux register for
McBSP is treated as a clock. This register muxes the clock between
external pin and internal clock. Considering that this should be
selectable at board level as the external clock probably needs to be
used if level shifters are being used, it should be really handled by
pinctrl-single.

The other use for hsmmc.c and pdata-quirks.c for the one time mux for
MMC clock from the MMC clock pin. That can be done with pinctrl-single
from the MMC driver too for DT based booting.

Then we just have the save and restore of the registers for
off-idle.
 
> So _maybe_ that's not an issue, as the pinmux config we have here is
> fixed, and done once at boot time, and maybe the code in mach-omap2 that
> touch DEVCONF1 is also ran just once and not at the same time as the
> pinmux. But I don't know if that's so.

It seems we could just do a read-only check for McBSP in the clock
code for the mux register, or even completely drop that code from
cclock3xxx_data.c and start using the pinctrl for that mux.

Paul & Tero, got any comments here?

Regards,

Tony

Re: [PATCH 4/4] arm: dts: omap3-gta04: Add static configuration for devconf1 register

[Paul Walmsley]

Hi

On Thu, 13 Nov 2014, Tony Lindgren wrote:

> * Tomi Valkeinen <tomi.valkeinen@ti.com> [141113 03:33]:
> > On 12/11/14 17:02, Tony Lindgren wrote:
> > 
> > >> And, with a quick grep, I see CONTROL_DEVCONF1 touched in multiple
> > >> places in the kernel. I wonder if adding a pinmux entry for it could
> > >> cause some rather odd problems.
> > > 
> > > They can all use pinctrl-single no problem.
> > 
> > Can, but don't. That's my worry. If we touch the DEVCONF1 via pinmux,
> > and we have code in mach-omap2 that also touch DEVCONF1, without any
> > knowledge (and locking) between those...
> 
> Hmm yeah the McBSP clock mux could be racy as the mux register for
> McBSP is treated as a clock. This register muxes the clock between
> external pin and internal clock. Considering that this should be
> selectable at board level as the external clock probably needs to be
> used if level shifters are being used, it should be really handled by
> pinctrl-single.
> 
> The other use for hsmmc.c and pdata-quirks.c for the one time mux for
> MMC clock from the MMC clock pin. That can be done with pinctrl-single
> from the MMC driver too for DT based booting.
> 
> Then we just have the save and restore of the registers for
> off-idle.
>  
> > So _maybe_ that's not an issue, as the pinmux config we have here is
> > fixed, and done once at boot time, and maybe the code in mach-omap2 that
> > touch DEVCONF1 is also ran just once and not at the same time as the
> > pinmux. But I don't know if that's so.
> 
> It seems we could just do a read-only check for McBSP in the clock
> code for the mux register, or even completely drop that code from
> cclock3xxx_data.c and start using the pinctrl for that mux.
> 
> Paul & Tero, got any comments here?

It's best to move all of the SCM register reads/writes to an SCM IP block 
driver.  This driver would be the only entity that would touch the SCM IP 
block registers - no other code on the system would touch it (perhaps 
aside from anything needed for early init).  The SCM driver would enforce 
mutual exclusion via a spinlock, so concurrent SCM register modifications 
wouldn't flake out.  Then the SCM driver would register clocks with the 
CCF, register pins with the pinctrl subsystem, etc. etc.

- Paul

Re: [PATCH 4/4] arm: dts: omap3-gta04: Add static configuration for devconf1 register

[Tony Lindgren]

* Paul Walmsley <paul@pwsan.com> [141113 15:01]:
> Hi
> 
> On Thu, 13 Nov 2014, Tony Lindgren wrote:
> 
> > * Tomi Valkeinen <tomi.valkeinen@ti.com> [141113 03:33]:
> > > On 12/11/14 17:02, Tony Lindgren wrote:
> > > 
> > > >> And, with a quick grep, I see CONTROL_DEVCONF1 touched in multiple
> > > >> places in the kernel. I wonder if adding a pinmux entry for it could
> > > >> cause some rather odd problems.
> > > > 
> > > > They can all use pinctrl-single no problem.
> > > 
> > > Can, but don't. That's my worry. If we touch the DEVCONF1 via pinmux,
> > > and we have code in mach-omap2 that also touch DEVCONF1, without any
> > > knowledge (and locking) between those...
> > 
> > Hmm yeah the McBSP clock mux could be racy as the mux register for
> > McBSP is treated as a clock. This register muxes the clock between
> > external pin and internal clock. Considering that this should be
> > selectable at board level as the external clock probably needs to be
> > used if level shifters are being used, it should be really handled by
> > pinctrl-single.
> > 
> > The other use for hsmmc.c and pdata-quirks.c for the one time mux for
> > MMC clock from the MMC clock pin. That can be done with pinctrl-single
> > from the MMC driver too for DT based booting.
> > 
> > Then we just have the save and restore of the registers for
> > off-idle.
> >  
> > > So _maybe_ that's not an issue, as the pinmux config we have here is
> > > fixed, and done once at boot time, and maybe the code in mach-omap2 that
> > > touch DEVCONF1 is also ran just once and not at the same time as the
> > > pinmux. But I don't know if that's so.
> > 
> > It seems we could just do a read-only check for McBSP in the clock
> > code for the mux register, or even completely drop that code from
> > cclock3xxx_data.c and start using the pinctrl for that mux.
> > 
> > Paul & Tero, got any comments here?
> 
> It's best to move all of the SCM register reads/writes to an SCM IP block 
> driver.  This driver would be the only entity that would touch the SCM IP 
> block registers - no other code on the system would touch it (perhaps 
> aside from anything needed for early init).  The SCM driver would enforce 
> mutual exclusion via a spinlock, so concurrent SCM register modifications 
> wouldn't flake out.  Then the SCM driver would register clocks with the 
> CCF, register pins with the pinctrl subsystem, etc. etc.

We actually do have that with pinctrl-single + syscon. We certainly
need to implement more Linux framework drivers for the SCM registers.
Things like regulators, clocks, and PHYs, but they should use
pinctrl-single + syscon. See the the pbias-regulator.c for example.

Looking at the McBSP clock handling, threre's yet more handling of
the same DEVCONF1 mux register in omap2_mcbsp_set_clks_src that gets 
alled from omap_mcbsp_dai_set_dai_sysclk.

To me it seems that if we handle the DEVCONF with pinctrl-single, we
don't need most of the McBSP fck code or the omap2_mcbsp_set_clks_src.
Having the mux register as the clock enable register is not nice..
Who knows what the clock coming from the external pin might be :)

Regards,

Tony

Re: [PATCH 4/4] arm: dts: omap3-gta04: Add static configuration for devconf1 register

[Tero Kristo]

On 11/14/2014 01:58 AM, Tony Lindgren wrote:
> * Paul Walmsley <paul@pwsan.com> [141113 15:01]:
>> Hi
>>
>> On Thu, 13 Nov 2014, Tony Lindgren wrote:
>>
>>> * Tomi Valkeinen <tomi.valkeinen@ti.com> [141113 03:33]:
>>>> On 12/11/14 17:02, Tony Lindgren wrote:
>>>>
>>>>>> And, with a quick grep, I see CONTROL_DEVCONF1 touched in multiple
>>>>>> places in the kernel. I wonder if adding a pinmux entry for it could
>>>>>> cause some rather odd problems.
>>>>>
>>>>> They can all use pinctrl-single no problem.
>>>>
>>>> Can, but don't. That's my worry. If we touch the DEVCONF1 via pinmux,
>>>> and we have code in mach-omap2 that also touch DEVCONF1, without any
>>>> knowledge (and locking) between those...
>>>
>>> Hmm yeah the McBSP clock mux could be racy as the mux register for
>>> McBSP is treated as a clock. This register muxes the clock between
>>> external pin and internal clock. Considering that this should be
>>> selectable at board level as the external clock probably needs to be
>>> used if level shifters are being used, it should be really handled by
>>> pinctrl-single.
>>>
>>> The other use for hsmmc.c and pdata-quirks.c for the one time mux for
>>> MMC clock from the MMC clock pin. That can be done with pinctrl-single
>>> from the MMC driver too for DT based booting.
>>>
>>> Then we just have the save and restore of the registers for
>>> off-idle.
>>>
>>>> So _maybe_ that's not an issue, as the pinmux config we have here is
>>>> fixed, and done once at boot time, and maybe the code in mach-omap2 that
>>>> touch DEVCONF1 is also ran just once and not at the same time as the
>>>> pinmux. But I don't know if that's so.
>>>
>>> It seems we could just do a read-only check for McBSP in the clock
>>> code for the mux register, or even completely drop that code from
>>> cclock3xxx_data.c and start using the pinctrl for that mux.
>>>
>>> Paul & Tero, got any comments here?
>>
>> It's best to move all of the SCM register reads/writes to an SCM IP block
>> driver.  This driver would be the only entity that would touch the SCM IP
>> block registers - no other code on the system would touch it (perhaps
>> aside from anything needed for early init).  The SCM driver would enforce
>> mutual exclusion via a spinlock, so concurrent SCM register modifications
>> wouldn't flake out.  Then the SCM driver would register clocks with the
>> CCF, register pins with the pinctrl subsystem, etc. etc.
>
> We actually do have that with pinctrl-single + syscon. We certainly
> need to implement more Linux framework drivers for the SCM registers.
> Things like regulators, clocks, and PHYs, but they should use
> pinctrl-single + syscon. See the the pbias-regulator.c for example.
>
> Looking at the McBSP clock handling, threre's yet more handling of
> the same DEVCONF1 mux register in omap2_mcbsp_set_clks_src that gets
> alled from omap_mcbsp_dai_set_dai_sysclk.
>
> To me it seems that if we handle the DEVCONF with pinctrl-single, we
> don't need most of the McBSP fck code or the omap2_mcbsp_set_clks_src.
> Having the mux register as the clock enable register is not nice..
> Who knows what the clock coming from the external pin might be :)

The PRCM/clock cleanups that I have under work basically splits the 
clock inits under their respective IP blocks; currently everything is 
registered under generic PRCM. System control module will be one of the 
clock providers (and is going to look like a driver), which will be 
registering its own clocks. This doesn't change the fact that pinctrl is 
directly mapping its own register space atm though, it might be possible 
to re-route this to use the generic system control module if need be though.

I guess its just a political decision which way we want to go, currently 
we have lots of system control clocks under the clock data (for 
AM33xx,AM43xx,OMAP3), but we can remove these easily if need be. In some 
cases it is nicer to have the data in the clock tree though, the drivers 
don't need to care if they are touching a clock or a pinctrl entity. 
Some people have been converting additional stuff to CCF outside of 
PRCM, like Archit did some work to try and get control module clock 
support for DRA7, and Tomi has been talking to convert some of the DSS 
internal clocks to CCF also.

-Tero

Re: [PATCH 4/4] arm: dts: omap3-gta04: Add static configuration for devconf1 register

[Grazvydas Ignotas]

On Fri, Nov 14, 2014 at 1:58 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Paul Walmsley <paul@pwsan.com> [141113 15:01]:
>> Hi
>>
>> On Thu, 13 Nov 2014, Tony Lindgren wrote:
>>
>> > * Tomi Valkeinen <tomi.valkeinen@ti.com> [141113 03:33]:
>> > > On 12/11/14 17:02, Tony Lindgren wrote:
>> > >
>> > > >> And, with a quick grep, I see CONTROL_DEVCONF1 touched in multiple
>> > > >> places in the kernel. I wonder if adding a pinmux entry for it could
>> > > >> cause some rather odd problems.
>> > > >
>> > > > They can all use pinctrl-single no problem.
>> > >
>> > > Can, but don't. That's my worry. If we touch the DEVCONF1 via pinmux,
>> > > and we have code in mach-omap2 that also touch DEVCONF1, without any
>> > > knowledge (and locking) between those...
>> >
>> > Hmm yeah the McBSP clock mux could be racy as the mux register for
>> > McBSP is treated as a clock. This register muxes the clock between
>> > external pin and internal clock. Considering that this should be
>> > selectable at board level as the external clock probably needs to be
>> > used if level shifters are being used, it should be really handled by
>> > pinctrl-single.
>> >
>> > The other use for hsmmc.c and pdata-quirks.c for the one time mux for
>> > MMC clock from the MMC clock pin. That can be done with pinctrl-single
>> > from the MMC driver too for DT based booting.
>> >
>> > Then we just have the save and restore of the registers for
>> > off-idle.
>> >
>> > > So _maybe_ that's not an issue, as the pinmux config we have here is
>> > > fixed, and done once at boot time, and maybe the code in mach-omap2 that
>> > > touch DEVCONF1 is also ran just once and not at the same time as the
>> > > pinmux. But I don't know if that's so.
>> >
>> > It seems we could just do a read-only check for McBSP in the clock
>> > code for the mux register, or even completely drop that code from
>> > cclock3xxx_data.c and start using the pinctrl for that mux.
>> >
>> > Paul & Tero, got any comments here?
>>
>> It's best to move all of the SCM register reads/writes to an SCM IP block
>> driver.  This driver would be the only entity that would touch the SCM IP
>> block registers - no other code on the system would touch it (perhaps
>> aside from anything needed for early init).  The SCM driver would enforce
>> mutual exclusion via a spinlock, so concurrent SCM register modifications
>> wouldn't flake out.  Then the SCM driver would register clocks with the
>> CCF, register pins with the pinctrl subsystem, etc. etc.
>
> We actually do have that with pinctrl-single + syscon. We certainly
> need to implement more Linux framework drivers for the SCM registers.
> Things like regulators, clocks, and PHYs, but they should use
> pinctrl-single + syscon. See the the pbias-regulator.c for example.
>
> Looking at the McBSP clock handling, threre's yet more handling of
> the same DEVCONF1 mux register in omap2_mcbsp_set_clks_src that gets
> alled from omap_mcbsp_dai_set_dai_sysclk.
>
> To me it seems that if we handle the DEVCONF with pinctrl-single, we
> don't need most of the McBSP fck code or the omap2_mcbsp_set_clks_src.
> Having the mux register as the clock enable register is not nice..
> Who knows what the clock coming from the external pin might be :)

How will audio do dynamic muxing without that code?
The pin must be remuxed back to internal clock when audio stops, or
else PM breaks.


Gražvydas

Re: [PATCH 4/4] arm: dts: omap3-gta04: Add static configuration for devconf1 register

[Tony Lindgren]

* Grazvydas Ignotas <notasas@gmail.com> [141114 06:38]:
> On Fri, Nov 14, 2014 at 1:58 AM, Tony Lindgren <tony@atomide.com> wrote:
> > * Paul Walmsley <paul@pwsan.com> [141113 15:01]:
> >> Hi
> >>
> >> On Thu, 13 Nov 2014, Tony Lindgren wrote:
> >>
> >> > * Tomi Valkeinen <tomi.valkeinen@ti.com> [141113 03:33]:
> >> > > On 12/11/14 17:02, Tony Lindgren wrote:
> >> > >
> >> > > >> And, with a quick grep, I see CONTROL_DEVCONF1 touched in multiple
> >> > > >> places in the kernel. I wonder if adding a pinmux entry for it could
> >> > > >> cause some rather odd problems.
> >> > > >
> >> > > > They can all use pinctrl-single no problem.
> >> > >
> >> > > Can, but don't. That's my worry. If we touch the DEVCONF1 via pinmux,
> >> > > and we have code in mach-omap2 that also touch DEVCONF1, without any
> >> > > knowledge (and locking) between those...
> >> >
> >> > Hmm yeah the McBSP clock mux could be racy as the mux register for
> >> > McBSP is treated as a clock. This register muxes the clock between
> >> > external pin and internal clock. Considering that this should be
> >> > selectable at board level as the external clock probably needs to be
> >> > used if level shifters are being used, it should be really handled by
> >> > pinctrl-single.
> >> >
> >> > The other use for hsmmc.c and pdata-quirks.c for the one time mux for
> >> > MMC clock from the MMC clock pin. That can be done with pinctrl-single
> >> > from the MMC driver too for DT based booting.
> >> >
> >> > Then we just have the save and restore of the registers for
> >> > off-idle.
> >> >
> >> > > So _maybe_ that's not an issue, as the pinmux config we have here is
> >> > > fixed, and done once at boot time, and maybe the code in mach-omap2 that
> >> > > touch DEVCONF1 is also ran just once and not at the same time as the
> >> > > pinmux. But I don't know if that's so.
> >> >
> >> > It seems we could just do a read-only check for McBSP in the clock
> >> > code for the mux register, or even completely drop that code from
> >> > cclock3xxx_data.c and start using the pinctrl for that mux.
> >> >
> >> > Paul & Tero, got any comments here?
> >>
> >> It's best to move all of the SCM register reads/writes to an SCM IP block
> >> driver.  This driver would be the only entity that would touch the SCM IP
> >> block registers - no other code on the system would touch it (perhaps
> >> aside from anything needed for early init).  The SCM driver would enforce
> >> mutual exclusion via a spinlock, so concurrent SCM register modifications
> >> wouldn't flake out.  Then the SCM driver would register clocks with the
> >> CCF, register pins with the pinctrl subsystem, etc. etc.
> >
> > We actually do have that with pinctrl-single + syscon. We certainly
> > need to implement more Linux framework drivers for the SCM registers.
> > Things like regulators, clocks, and PHYs, but they should use
> > pinctrl-single + syscon. See the the pbias-regulator.c for example.
> >
> > Looking at the McBSP clock handling, threre's yet more handling of
> > the same DEVCONF1 mux register in omap2_mcbsp_set_clks_src that gets
> > alled from omap_mcbsp_dai_set_dai_sysclk.
> >
> > To me it seems that if we handle the DEVCONF with pinctrl-single, we
> > don't need most of the McBSP fck code or the omap2_mcbsp_set_clks_src.
> > Having the mux register as the clock enable register is not nice..
> > Who knows what the clock coming from the external pin might be :)
> 
> How will audio do dynamic muxing without that code?
> The pin must be remuxed back to internal clock when audio stops, or
> else PM breaks.

There's a standard way of dealing with that already available. We can
just have runtime_pm functions call pinctrl_pm_select_sleep_state()
and pinctrl_pm_select_default_state().

Regards,

Tony

Re: [PATCH 4/4] arm: dts: omap3-gta04: Add static configuration for devconf1 register

[Tony Lindgren]

* Tero Kristo <t-kristo@ti.com> [141113 23:33]:
> On 11/14/2014 01:58 AM, Tony Lindgren wrote:
> 
> The PRCM/clock cleanups that I have under work basically splits the clock
> inits under their respective IP blocks; currently everything is registered
> under generic PRCM. System control module will be one of the clock providers
> (and is going to look like a driver), which will be registering its own
> clocks.

Yes that's nice. The clock modules in the SCM should probably use the
syscon mapping unless there's a clear separate IO area for them. And
then use pinctrl for registers that are muxes for external pins unless
they are in some dedicated clock register area.

> This doesn't change the fact that pinctrl is directly mapping its
> own register space atm though, it might be possible to re-route this to use
> the generic system control module if need be though.

Mapping dedicated IO areas to individual drivers is not a problem. These
drivers can eventually be children of a core SCM driver if needed.
 
> I guess its just a political decision which way we want to go, currently we
> have lots of system control clocks under the clock data (for
> AM33xx,AM43xx,OMAP3), but we can remove these easily if need be. In some
> cases it is nicer to have the data in the clock tree though, the drivers
> don't need to care if they are touching a clock or a pinctrl entity. Some
> people have been converting additional stuff to CCF outside of PRCM, like
> Archit did some work to try and get control module clock support for DRA7,
> and Tomi has been talking to convert some of the DSS internal clocks to CCF
> also.

Setting up CCF drivers for SCM makes sense to me. I suggest the
following guidelines:

1. If there's a clear separate dedicated IO area in SCM, it can be
   a driver implementing a Linux generic framework for CCF, regulators,
   pinctrl, or PHY.

2. For the random control registers, we should use syscon or
   pinctrl-single to implement Linux generic framwork functions for
   CCF, regulators, pinctrl or PHY.

3. For resource management, we can have a core SCM driver that takes
   care of the save and restore of registers and clocking if needed.
   I believe currently SCM clocks are always enabled though. We can
   set the drivers in #1 and #2 abobe to be childer of the core SCM
   driver if we ever need to manage clocks during runtime.

Regards,

Tony