[RFC] drm/msm: DT support for 8960/8064

[RFC] drm/msm: DT support for 8960/8064

[Rob Clark]

Now that we (almost) have enough dependencies in place (MMCC, RPM, etc),
add necessary DT support so that we can use drm/msm on upstream kernel.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
Commence bikeshedding :-)

 Documentation/devicetree/bindings/drm/msm/gpu.txt  | 51 ++++++++++++++++++++
 Documentation/devicetree/bindings/drm/msm/hdmi.txt | 43 +++++++++++++++++
 Documentation/devicetree/bindings/drm/msm/msm.txt  | 40 ++++++++++++++++
 drivers/gpu/drm/msm/adreno/a3xx_gpu.c              |  2 +
 drivers/gpu/drm/msm/hdmi/hdmi.c                    | 55 ++++++++++++++--------
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c            | 10 ++--
 drivers/gpu/drm/msm/msm_drv.c                      | 29 ++++++++++--
 7 files changed, 204 insertions(+), 26 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/drm/msm/gpu.txt
 create mode 100644 Documentation/devicetree/bindings/drm/msm/hdmi.txt
 create mode 100644 Documentation/devicetree/bindings/drm/msm/msm.txt

diff --git a/Documentation/devicetree/bindings/drm/msm/gpu.txt b/Documentation/devicetree/bindings/drm/msm/gpu.txt
new file mode 100644
index 0000000..6e33efe
--- /dev/null
+++ b/Documentation/devicetree/bindings/drm/msm/gpu.txt
@@ -0,0 +1,51 @@
+Qualcomm adreno/snapdragon GPU
+
+Required properties:
+- compatible: "qcom,adreno-3xx"
+- reg: Physical base address and length of the controller's registers.
+- interrupts: The interrupt outputs from the controller.
+- clocks: device clocks
+  See ../clocks/clock-bindings.txt for details.
+- qcom,chipid: gpu chip-id.  Note this may become optional for future
+  devices if we can reliably read the chipid from hw
+- qcom,gpu-pwrlevels: array of OPPs, sorted highest to lowest
+  - compatible: "qcom,gpu-pwrlevels"
+  - for each qcom,gpu-pwrlevel:
+    - qcom,gpu-freq: requested gpu clock speed
+    - NOTE: downstream android driver defines additional parameters to
+      configure memory bandwidth scaling per OPP.
+
+Optional properties:
+- n/a
+
+Example:
+
+/ {
+	...
+
+	gpu: qcom,kgsl-3d0@4300000 {
+		compatible = "qcom,adreno-3xx";
+		reg = <0x04300000 0x20000>;
+		reg-names = "kgsl_3d0_reg_memory";
+		interrupts = <GIC_SPI 80 0>;
+		interrupt-names = "kgsl_3d0_irq";
+		clock-names =
+		    "core_clk",
+		    "iface_clk",
+		    "mem_iface_clk";
+		clocks =
+		    <&mmcc GFX3D_CLK>,
+		    <&mmcc GFX3D_AHB_CLK>,
+		    <&mmcc MMSS_IMEM_AHB_CLK>;
+		qcom,chipid = <0x03020100>;
+		qcom,gpu-pwrlevels {
+			compatible = "qcom,gpu-pwrlevels";
+			qcom,gpu-pwrlevel@0 {
+				qcom,gpu-freq = <450000000>;
+			};
+			qcom,gpu-pwrlevel@1 {
+				qcom,gpu-freq = <27000000>;
+			};
+		};
+	};
+};
diff --git a/Documentation/devicetree/bindings/drm/msm/hdmi.txt b/Documentation/devicetree/bindings/drm/msm/hdmi.txt
new file mode 100644
index 0000000..051a49f
--- /dev/null
+++ b/Documentation/devicetree/bindings/drm/msm/hdmi.txt
@@ -0,0 +1,43 @@
+Qualcomm adreno/snapdragon hdmi output
+
+Required properties:
+- compatible: "qcom,hdmi-tx-8x60", "qcom,hdmi-tx-8960", "qcom,hdmi-tx-8x74"
+- reg: Physical base address and length of the controller's registers.
+- interrupts: The interrupt outputs from the controller.
+- clocks: device clocks
+  See ../clocks/clock-bindings.txt for details.
+- qcom,hdmi-tx-ddc-clk: ddc clk pin
+- qcom,hdmi-tx-ddc-data: ddc data pin
+- qcom,hdmi-tx-hpd: hpd pin
+- core-vdda-supply: phandle to supply regulator
+- hdmi-mux-supply: phandle to mux regulator
+
+Optional properties:
+- qcom,hdmi-tx-mux-en: hdmi mux enable pin
+- qcom,hdmi-tx-mux-sel: hdmi mux select pin
+
+Example:
+
+/ {
+	...
+
+	hdmi: qcom,hdmi-tx-8960@4a00000 {
+		compatible = "qcom,hdmi-tx-8960";
+		reg-names = "core_physical";
+		reg = <0x04a00000 0x1000>;
+		interrupts = <GIC_SPI 79 0>;
+		clock-names =
+		    "core_clk",
+		    "master_iface_clk",
+		    "slave_iface_clk";
+		clocks =
+		    <&mmcc HDMI_APP_CLK>,
+		    <&mmcc HDMI_M_AHB_CLK>,
+		    <&mmcc HDMI_S_AHB_CLK>;
+		qcom,hdmi-tx-ddc-clk = <&msmgpio 70 GPIO_ACTIVE_HIGH>;
+		qcom,hdmi-tx-ddc-data = <&msmgpio 71 GPIO_ACTIVE_HIGH>;
+		qcom,hdmi-tx-hpd = <&msmgpio 72 GPIO_ACTIVE_HIGH>;
+		core-vdda-supply = <&pm8921_hdmi_mvs>;
+		hdmi-mux-supply = <&ext_3p3v>;
+	};
+};
diff --git a/Documentation/devicetree/bindings/drm/msm/msm.txt b/Documentation/devicetree/bindings/drm/msm/msm.txt
new file mode 100644
index 0000000..484cc12
--- /dev/null
+++ b/Documentation/devicetree/bindings/drm/msm/msm.txt
@@ -0,0 +1,40 @@
+Qualcomm adreno/snapdragon
+
+Required properties:
+- compatible: "qcom,mdp" (mdp4) or "qcom,mdss_mdp" (mdp5)
+- reg: Physical base address and length of the controller's registers.
+- interrupts: The interrupt outputs from the controller.
+- connectors: array of phandles for output device(s)
+- clocks: device clocks
+  See ../clocks/clock-bindings.txt for details.
+
+Optional properties:
+- gpus: phandle for gpu device
+
+Example:
+
+/ {
+	...
+
+	mdp: qcom,mdp@5100000 {
+		compatible = "qcom,mdp";
+		reg = <0x05100000 0xf0000>;
+		interrupts = <GIC_SPI 75 0>;
+		connectors = <&hdmi>;
+		gpus = <&gpu>;
+		clock-names =
+		    "core_clk",
+		    "iface_clk",
+		    "lut_clk",
+		    "src_clk",
+		    "hdmi_clk",
+		    "mdp_clk";
+		clocks =
+		    <&mmcc MDP_SRC>,
+		    <&mmcc MDP_AHB_CLK>,
+		    <&mmcc MDP_LUT_CLK>,
+		    <&mmcc TV_SRC>,
+		    <&mmcc HDMI_TV_CLK>,
+		    <&mmcc MDP_TV_CLK>;
+	};
+};
diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
index a2cee06..2773600 100644
--- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
@@ -680,6 +680,8 @@ static int a3xx_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id dt_match[] = {
+	{ .compatible = "qcom,adreno-3xx" },
+	/* for backwards compat w/ downstream kgsl DT files: */
 	{ .compatible = "qcom,kgsl-3d0" },
 	{}
 };
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 7f7aade..0ff8d46 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -123,7 +123,8 @@ struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder)
 	for (i = 0; i < config->hpd_reg_cnt; i++) {
 		struct regulator *reg;
 
-		reg = devm_regulator_get(&pdev->dev, config->hpd_reg_names[i]);
+		reg = devm_regulator_get_exclusive(&pdev->dev,
+				config->hpd_reg_names[i]);
 		if (IS_ERR(reg)) {
 			ret = PTR_ERR(reg);
 			dev_err(dev->dev, "failed to get hpd regulator: %s (%d)\n",
@@ -138,7 +139,8 @@ struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder)
 	for (i = 0; i < config->pwr_reg_cnt; i++) {
 		struct regulator *reg;
 
-		reg = devm_regulator_get(&pdev->dev, config->pwr_reg_names[i]);
+		reg = devm_regulator_get_exclusive(&pdev->dev,
+				config->pwr_reg_names[i]);
 		if (IS_ERR(reg)) {
 			ret = PTR_ERR(reg);
 			dev_err(dev->dev, "failed to get pwr regulator: %s (%d)\n",
@@ -273,24 +275,37 @@ static int hdmi_bind(struct device *dev, struct device *master, void *data)
 		return gpio;
 	}
 
-	/* TODO actually use DT.. */
-	static const char *hpd_reg_names[] = {"hpd-gdsc", "hpd-5v"};
-	static const char *pwr_reg_names[] = {"core-vdda", "core-vcc"};
-	static const char *hpd_clk_names[] = {"iface_clk", "core_clk", "mdp_core_clk"};
-	static unsigned long hpd_clk_freq[] = {0, 19200000, 0};
-	static const char *pwr_clk_names[] = {"extp_clk", "alt_iface_clk"};
+	if (of_device_is_compatible(of_node, "qcom,hdmi-tx-8x74")) {
+		static const char *hpd_reg_names[] = {"hpd-gdsc", "hpd-5v"};
+		static const char *pwr_reg_names[] = {"core-vdda", "core-vcc"};
+		static const char *hpd_clk_names[] = {"iface_clk", "core_clk", "mdp_core_clk"};
+		static unsigned long hpd_clk_freq[] = {0, 19200000, 0};
+		static const char *pwr_clk_names[] = {"extp_clk", "alt_iface_clk"};
+		config.phy_init      = hdmi_phy_8x74_init;
+		config.hpd_reg_names = hpd_reg_names;
+		config.hpd_reg_cnt   = ARRAY_SIZE(hpd_reg_names);
+		config.pwr_reg_names = pwr_reg_names;
+		config.pwr_reg_cnt   = ARRAY_SIZE(pwr_reg_names);
+		config.hpd_clk_names = hpd_clk_names;
+		config.hpd_freq      = hpd_clk_freq;
+		config.hpd_clk_cnt   = ARRAY_SIZE(hpd_clk_names);
+		config.pwr_clk_names = pwr_clk_names;
+		config.pwr_clk_cnt   = ARRAY_SIZE(pwr_clk_names);
+	} else if (of_device_is_compatible(of_node, "qcom,hdmi-tx-8960")) {
+		static const char *hpd_clk_names[] = {"core_clk", "master_iface_clk", "slave_iface_clk"};
+		static const char *hpd_reg_names[] = {"core-vdda", "hdmi-mux"};
+		config.phy_init      = hdmi_phy_8960_init;
+		config.hpd_reg_names = hpd_reg_names;
+		config.hpd_reg_cnt   = ARRAY_SIZE(hpd_reg_names);
+		config.hpd_clk_names = hpd_clk_names;
+		config.hpd_clk_cnt   = ARRAY_SIZE(hpd_clk_names);
+	} else if (of_device_is_compatible(of_node, "qcom,hdmi-tx-8x60")) {
+		config.phy_init      = hdmi_phy_8x60_init;
+	} else {
+		dev_err(dev, "unknown phy: %s\n", of_node->name);
+	}
 
-	config.phy_init      = hdmi_phy_8x74_init;
 	config.mmio_name     = "core_physical";
-	config.hpd_reg_names = hpd_reg_names;
-	config.hpd_reg_cnt   = ARRAY_SIZE(hpd_reg_names);
-	config.pwr_reg_names = pwr_reg_names;
-	config.pwr_reg_cnt   = ARRAY_SIZE(pwr_reg_names);
-	config.hpd_clk_names = hpd_clk_names;
-	config.hpd_freq      = hpd_clk_freq;
-	config.hpd_clk_cnt   = ARRAY_SIZE(hpd_clk_names);
-	config.pwr_clk_names = pwr_clk_names;
-	config.pwr_clk_cnt   = ARRAY_SIZE(pwr_clk_names);
 	config.ddc_clk_gpio  = get_gpio("qcom,hdmi-tx-ddc-clk");
 	config.ddc_data_gpio = get_gpio("qcom,hdmi-tx-ddc-data");
 	config.hpd_gpio      = get_gpio("qcom,hdmi-tx-hpd");
@@ -373,7 +388,9 @@ static int hdmi_dev_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id dt_match[] = {
-	{ .compatible = "qcom,hdmi-tx" },
+	{ .compatible = "qcom,hdmi-tx-8x74" },
+	{ .compatible = "qcom,hdmi-tx-8960" },
+	{ .compatible = "qcom,hdmi-tx-8x60" },
 	{}
 };
 
diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
index 0bb4faa..5a7bfd4 100644
--- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
@@ -294,15 +294,17 @@ struct msm_kms *mdp4_kms_init(struct drm_device *dev)
 		goto fail;
 	}
 
-	mdp4_kms->dsi_pll_vdda = devm_regulator_get(&pdev->dev, "dsi_pll_vdda");
+	mdp4_kms->dsi_pll_vdda =
+			devm_regulator_get_optional(&pdev->dev, "dsi_pll_vdda");
 	if (IS_ERR(mdp4_kms->dsi_pll_vdda))
 		mdp4_kms->dsi_pll_vdda = NULL;
 
-	mdp4_kms->dsi_pll_vddio = devm_regulator_get(&pdev->dev, "dsi_pll_vddio");
+	mdp4_kms->dsi_pll_vddio =
+			devm_regulator_get_optional(&pdev->dev, "dsi_pll_vddio");
 	if (IS_ERR(mdp4_kms->dsi_pll_vddio))
 		mdp4_kms->dsi_pll_vddio = NULL;
 
-	mdp4_kms->vdd = devm_regulator_get(&pdev->dev, "vdd");
+	mdp4_kms->vdd = devm_regulator_get_exclusive(&pdev->dev, "vdd");
 	if (IS_ERR(mdp4_kms->vdd))
 		mdp4_kms->vdd = NULL;
 
@@ -406,6 +408,8 @@ static struct mdp4_platform_config *mdp4_get_config(struct platform_device *dev)
 	static struct mdp4_platform_config config = {};
 #ifdef CONFIG_OF
 	/* TODO */
+	config.max_clk = 266667000;
+	config.iommu = iommu_domain_alloc(&platform_bus_type);
 #else
 	if (cpu_is_apq8064())
 		config.max_clk = 266667000;
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 9a5d87d..1ad0155 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -906,7 +906,8 @@ static int compare_of(struct device *dev, void *data)
 	return dev->of_node == data;
 }
 
-static int msm_drm_add_components(struct device *master, struct master *m)
+static int add_components(struct device *master, struct master *m,
+		const char *name)
 {
 	struct device_node *np = master->of_node;
 	unsigned i;
@@ -915,16 +916,35 @@ static int msm_drm_add_components(struct device *master, struct master *m)
 	for (i = 0; ; i++) {
 		struct device_node *node;
 
-		node = of_parse_phandle(np, "connectors", i);
+		node = of_parse_phandle(np, name, i);
 		if (!node)
 			break;
 
 		ret = component_master_add_child(m, compare_of, node);
 		of_node_put(node);
 
-		if (ret)
+		if (ret) {
+			dev_err(master, "could not add %s child %s: %d\n",
+					name, node->name, ret);
 			return ret;
+		}
 	}
+
+	return 0;
+}
+
+static int msm_drm_add_components(struct device *master, struct master *m)
+{
+	int ret;
+
+	ret = add_components(master, m, "connectors");
+	if (ret)
+		return ret;
+
+	ret = add_components(master, m, "gpus");
+	if (ret)
+		return ret;
+
 	return 0;
 }
 #else
@@ -1008,7 +1028,8 @@ static const struct platform_device_id msm_id[] = {
 };
 
 static const struct of_device_id dt_match[] = {
-	{ .compatible = "qcom,mdss_mdp" },
+	{ .compatible = "qcom,mdp" },      /* mdp4 */
+	{ .compatible = "qcom,mdss_mdp" }, /* mdp5 */
 	{}
 };
 MODULE_DEVICE_TABLE(of, dt_match);
-- 
1.9.3

Re: [RFC] drm/msm: DT support for 8960/8064

[Jordan Crouse]

On 07/01/2014 12:57 PM, Rob Clark wrote:
> diff --git a/Documentation/devicetree/bindings/drm/msm/gpu.txt b/Documentation/devicetree/bindings/drm/msm/gpu.txt
> new file mode 100644
> index 0000000..6e33efe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/drm/msm/gpu.txt
> @@ -0,0 +1,51 @@
> +Qualcomm adreno/snapdragon GPU
> +
> +Required properties:
> +- compatible: "qcom,adreno-3xx"

3xx won't always be the only game in town. Not that I have a better name,
just pointing out that it has a limited shelf life.

Jordan

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [RFC] drm/msm: DT support for 8960/8064

[Rob Clark]

On Wed, Jul 2, 2014 at 10:26 AM, Jordan Crouse <jcrouse@codeaurora.org> wrote:
>
> On 07/01/2014 12:57 PM, Rob Clark wrote:
>>
>> diff --git a/Documentation/devicetree/bindings/drm/msm/gpu.txt b/Documentation/devicetree/bindings/drm/msm/gpu.txt
>> new file mode 100644
>> index 0000000..6e33efe
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/drm/msm/gpu.txt
>> @@ -0,0 +1,51 @@
>> +Qualcomm adreno/snapdragon GPU
>> +
>> +Required properties:
>> +- compatible: "qcom,adreno-3xx"
>
>
> 3xx won't always be the only game in town. Not that I have a better name,
> just pointing out that it has a limited shelf life.
>

yup, I'm expecting we'll add new compatible strings, ie.
qcom,adreno-4xx, etc, as we add support for various future generations

BR,
-R

> Jordan
>
> --
> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation

Re: [RFC] drm/msm: DT support for 8960/8064

[Jordan Crouse]

On 07/02/2014 08:42 AM, Rob Clark wrote:
> On Wed, Jul 2, 2014 at 10:26 AM, Jordan Crouse <jcrouse@codeaurora.org> wrote:
>>
>> On 07/01/2014 12:57 PM, Rob Clark wrote:
>>>
>>> diff --git a/Documentation/devicetree/bindings/drm/msm/gpu.txt b/Documentation/devicetree/bindings/drm/msm/gpu.txt
>>> new file mode 100644
>>> index 0000000..6e33efe
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/drm/msm/gpu.txt
>>> @@ -0,0 +1,51 @@
>>> +Qualcomm adreno/snapdragon GPU
>>> +
>>> +Required properties:
>>> +- compatible: "qcom,adreno-3xx"
>>
>>
>> 3xx won't always be the only game in town. Not that I have a better name,
>> just pointing out that it has a limited shelf life.
>>
>
> yup, I'm expecting we'll add new compatible strings, ie.
> qcom,adreno-4xx, etc, as we add support for various future generations
>
> BR,
> -R

Alright, fair enough.  Compatible strings are cheap.

Jordan

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [RFC] drm/msm: DT support for 8960/8064

[Olof Johansson]

On Wed, Jul 2, 2014 at 7:26 AM, Jordan Crouse <jcrouse@codeaurora.org> wrote:
> On 07/01/2014 12:57 PM, Rob Clark wrote:
>>
>> diff --git a/Documentation/devicetree/bindings/drm/msm/gpu.txt
>> b/Documentation/devicetree/bindings/drm/msm/gpu.txt
>> new file mode 100644
>> index 0000000..6e33efe
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/drm/msm/gpu.txt
>> @@ -0,0 +1,51 @@
>> +Qualcomm adreno/snapdragon GPU
>> +
>> +Required properties:
>> +- compatible: "qcom,adreno-3xx"
>
>
> 3xx won't always be the only game in town. Not that I have a better name,
> just pointing out that it has a limited shelf life.

Actually, we prefer to avoid "xx" compatible strings. The compatible
should be the first actual device that is part of the xx series, and
newer IP should claim to be compatible with their own version plus the
original one (if they are possible to use with the same exact
programming model and only need extra code to deal with extensions).

That way you'll also have a way to tell them apart in the driver if
you can't derive it out of register contents, etc.


-Olof

Re: [RFC] drm/msm: DT support for 8960/8064

[Mark Rutland]

On Tue, Jul 01, 2014 at 07:57:35PM +0100, Rob Clark wrote:
> Now that we (almost) have enough dependencies in place (MMCC, RPM, etc),
> add necessary DT support so that we can use drm/msm on upstream kernel.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
> Commence bikeshedding :-)
> 
>  Documentation/devicetree/bindings/drm/msm/gpu.txt  | 51 ++++++++++++++++++++
>  Documentation/devicetree/bindings/drm/msm/hdmi.txt | 43 +++++++++++++++++
>  Documentation/devicetree/bindings/drm/msm/msm.txt  | 40 ++++++++++++++++
>  drivers/gpu/drm/msm/adreno/a3xx_gpu.c              |  2 +
>  drivers/gpu/drm/msm/hdmi/hdmi.c                    | 55 ++++++++++++++--------
>  drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c            | 10 ++--
>  drivers/gpu/drm/msm/msm_drv.c                      | 29 ++++++++++--
>  7 files changed, 204 insertions(+), 26 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/drm/msm/gpu.txt
>  create mode 100644 Documentation/devicetree/bindings/drm/msm/hdmi.txt
>  create mode 100644 Documentation/devicetree/bindings/drm/msm/msm.txt
> 
> diff --git a/Documentation/devicetree/bindings/drm/msm/gpu.txt b/Documentation/devicetree/bindings/drm/msm/gpu.txt
> new file mode 100644
> index 0000000..6e33efe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/drm/msm/gpu.txt
> @@ -0,0 +1,51 @@
> +Qualcomm adreno/snapdragon GPU
> +
> +Required properties:
> +- compatible: "qcom,adreno-3xx"

As Olof said, choose a definite name here, and have variants claim
compatibility with that in addition to a variant specific string.

> +- reg: Physical base address and length of the controller's registers.

Just the one reg entry?

The example has reg-names. Either document the name or drop it.

> +- interrupts: The interrupt outputs from the controller.

How many? Which ones? Names?

> +- clocks: device clocks
> +  See ../clocks/clock-bindings.txt for details.

Similarly?

I should be able to read the binding and put together a DTS. Currently I
cannot.

> +- qcom,chipid: gpu chip-id.  Note this may become optional for future
> +  devices if we can reliably read the chipid from hw

What's the problem with reading chipid from HW at the moment?

Should this possibly be optional, and only if not possible to read from
HW?

> +- qcom,gpu-pwrlevels: array of OPPs, sorted highest to lowest

This is confusing. This is a node rather than a property, and in general
it's not a good idea to rely on node ordering (there's no such thing as
an array of nodes in the DTB).

> +  - compatible: "qcom,gpu-pwrlevels"
> +  - for each qcom,gpu-pwrlevel:

Is that a compatible string for each node, name, or what?

Any reason for having this as a container node at all?

> +    - qcom,gpu-freq: requested gpu clock speed
> +    - NOTE: downstream android driver defines additional parameters to
> +      configure memory bandwidth scaling per OPP.

So? Either define those or don't bother. Having a halfway house binding
is not helpful.

> +
> +Optional properties:
> +- n/a

Drop this if there are no optional properties.

> +
> +Example:
> +
> +/ {
> +       ...
> +
> +       gpu: qcom,kgsl-3d0@4300000 {
> +               compatible = "qcom,adreno-3xx";
> +               reg = <0x04300000 0x20000>;
> +               reg-names = "kgsl_3d0_reg_memory";
> +               interrupts = <GIC_SPI 80 0>;
> +               interrupt-names = "kgsl_3d0_irq";
> +               clock-names =
> +                   "core_clk",
> +                   "iface_clk",
> +                   "mem_iface_clk";
> +               clocks =
> +                   <&mmcc GFX3D_CLK>,
> +                   <&mmcc GFX3D_AHB_CLK>,
> +                   <&mmcc MMSS_IMEM_AHB_CLK>;
> +               qcom,chipid = <0x03020100>;
> +               qcom,gpu-pwrlevels {
> +                       compatible = "qcom,gpu-pwrlevels";
> +                       qcom,gpu-pwrlevel@0 {
> +                               qcom,gpu-freq = <450000000>;
> +                       };
> +                       qcom,gpu-pwrlevel@1 {
> +                               qcom,gpu-freq = <27000000>;
> +                       };
> +               };
> +       };
> +};
> diff --git a/Documentation/devicetree/bindings/drm/msm/hdmi.txt b/Documentation/devicetree/bindings/drm/msm/hdmi.txt
> new file mode 100644
> index 0000000..051a49f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/drm/msm/hdmi.txt
> @@ -0,0 +1,43 @@
> +Qualcomm adreno/snapdragon hdmi output
> +
> +Required properties:
> +- compatible: "qcom,hdmi-tx-8x60", "qcom,hdmi-tx-8960", "qcom,hdmi-tx-8x74"

Please don't use wildcard strings.

Is a node expected to have one of these entries, or all?

I would recommend using a bulleted list to describe compatible strings.
It allows for easier extension and the easy addition of notes. e.g.

- compatible: should contain:
  * "foo,bar-xtreme" for bar variants with the xtreme extensions.
  * "foo,bar" for all bar variants (inc. xtreme).
  * "foo,baz" for baz variants.
  * "foo,foo" for all variants.

> +- reg: Physical base address and length of the controller's registers.
> +- interrupts: The interrupt outputs from the controller.
> +- clocks: device clocks

One entry? Multiple?

What do they correspond to on the data sheet?

Names?

> +  See ../clocks/clock-bindings.txt for details.
> +- qcom,hdmi-tx-ddc-clk: ddc clk pin
> +- qcom,hdmi-tx-ddc-data: ddc data pin
> +- qcom,hdmi-tx-hpd: hpd pin

GPIOs should be called *-gpios.

> +- core-vdda-supply: phandle to supply regulator
> +- hdmi-mux-supply: phandle to mux regulator
> +
> +Optional properties:
> +- qcom,hdmi-tx-mux-en: hdmi mux enable pin
> +- qcom,hdmi-tx-mux-sel: hdmi mux select pin

GPIOs again.

> +
> +Example:
> +
> +/ {
> +       ...
> +
> +       hdmi: qcom,hdmi-tx-8960@4a00000 {
> +               compatible = "qcom,hdmi-tx-8960";
> +               reg-names = "core_physical";
> +               reg = <0x04a00000 0x1000>;
> +               interrupts = <GIC_SPI 79 0>;
> +               clock-names =
> +                   "core_clk",
> +                   "master_iface_clk",
> +                   "slave_iface_clk";
> +               clocks =
> +                   <&mmcc HDMI_APP_CLK>,
> +                   <&mmcc HDMI_M_AHB_CLK>,
> +                   <&mmcc HDMI_S_AHB_CLK>;
> +               qcom,hdmi-tx-ddc-clk = <&msmgpio 70 GPIO_ACTIVE_HIGH>;
> +               qcom,hdmi-tx-ddc-data = <&msmgpio 71 GPIO_ACTIVE_HIGH>;
> +               qcom,hdmi-tx-hpd = <&msmgpio 72 GPIO_ACTIVE_HIGH>;
> +               core-vdda-supply = <&pm8921_hdmi_mvs>;
> +               hdmi-mux-supply = <&ext_3p3v>;
> +       };
> +};
> diff --git a/Documentation/devicetree/bindings/drm/msm/msm.txt b/Documentation/devicetree/bindings/drm/msm/msm.txt
> new file mode 100644
> index 0000000..484cc12
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/drm/msm/msm.txt
> @@ -0,0 +1,40 @@
> +Qualcomm adreno/snapdragon

What exactly is this unit? Judging by the GPU phandle it's not the GPU
itself.

> +
> +Required properties:
> +- compatible: "qcom,mdp" (mdp4) or "qcom,mdss_mdp" (mdp5)
> +- reg: Physical base address and length of the controller's registers.
> +- interrupts: The interrupt outputs from the controller.

All of my prior points for these properties stand.

> +- connectors: array of phandles for output device(s)

What are valid devices to point this at?

> +- clocks: device clocks

Likewise, see my above points regarding clocks.

> +  See ../clocks/clock-bindings.txt for details.
> +
> +Optional properties:
> +- gpus: phandle for gpu device

What exactly is this used for?

Thanks,
Mark.

Re: [RFC] drm/msm: DT support for 8960/8064

[Rob Clark]

On Wed, Jul 2, 2014 at 1:12 PM, Olof Johansson <olof@lixom.net> wrote:
> On Wed, Jul 2, 2014 at 7:26 AM, Jordan Crouse <jcrouse@codeaurora.org> wrote:
>> On 07/01/2014 12:57 PM, Rob Clark wrote:
>>>
>>> diff --git a/Documentation/devicetree/bindings/drm/msm/gpu.txt
>>> b/Documentation/devicetree/bindings/drm/msm/gpu.txt
>>> new file mode 100644
>>> index 0000000..6e33efe
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/drm/msm/gpu.txt
>>> @@ -0,0 +1,51 @@
>>> +Qualcomm adreno/snapdragon GPU
>>> +
>>> +Required properties:
>>> +- compatible: "qcom,adreno-3xx"
>>
>>
>> 3xx won't always be the only game in town. Not that I have a better name,
>> just pointing out that it has a limited shelf life.
>
> Actually, we prefer to avoid "xx" compatible strings. The compatible
> should be the first actual device that is part of the xx series, and
> newer IP should claim to be compatible with their own version plus the
> original one (if they are possible to use with the same exact
> programming model and only need extra code to deal with extensions).
>
> That way you'll also have a way to tell them apart in the driver if
> you can't derive it out of register contents, etc.

well, we could put entire chip-id (including the patch revision) in
the compatible string.. ie. qcom,adreno-320.2

BR,
-R

>
> -Olof

Re: [RFC] drm/msm: DT support for 8960/8064

[Rob Clark]

On Wed, Jul 2, 2014 at 2:09 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Jul 01, 2014 at 07:57:35PM +0100, Rob Clark wrote:
>> Now that we (almost) have enough dependencies in place (MMCC, RPM, etc),
>> add necessary DT support so that we can use drm/msm on upstream kernel.
>>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> ---
>> Commence bikeshedding :-)
>>
>>  Documentation/devicetree/bindings/drm/msm/gpu.txt  | 51 ++++++++++++++++++++
>>  Documentation/devicetree/bindings/drm/msm/hdmi.txt | 43 +++++++++++++++++
>>  Documentation/devicetree/bindings/drm/msm/msm.txt  | 40 ++++++++++++++++
>>  drivers/gpu/drm/msm/adreno/a3xx_gpu.c              |  2 +
>>  drivers/gpu/drm/msm/hdmi/hdmi.c                    | 55 ++++++++++++++--------
>>  drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c            | 10 ++--
>>  drivers/gpu/drm/msm/msm_drv.c                      | 29 ++++++++++--
>>  7 files changed, 204 insertions(+), 26 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/drm/msm/gpu.txt
>>  create mode 100644 Documentation/devicetree/bindings/drm/msm/hdmi.txt
>>  create mode 100644 Documentation/devicetree/bindings/drm/msm/msm.txt
>>
>> diff --git a/Documentation/devicetree/bindings/drm/msm/gpu.txt b/Documentation/devicetree/bindings/drm/msm/gpu.txt
>> new file mode 100644
>> index 0000000..6e33efe
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/drm/msm/gpu.txt
>> @@ -0,0 +1,51 @@
>> +Qualcomm adreno/snapdragon GPU
>> +
>> +Required properties:
>> +- compatible: "qcom,adreno-3xx"
>
> As Olof said, choose a definite name here, and have variants claim
> compatibility with that in addition to a variant specific string.
>
>> +- reg: Physical base address and length of the controller's registers.
>
> Just the one reg entry?

as far as I know.. keep in mind, I'm working without docs

> The example has reg-names. Either document the name or drop it.
>
>> +- interrupts: The interrupt outputs from the controller.
>
> How many? Which ones? Names?
>
>> +- clocks: device clocks
>> +  See ../clocks/clock-bindings.txt for details.
>
> Similarly?
>
> I should be able to read the binding and put together a DTS. Currently I
> cannot.
>
>> +- qcom,chipid: gpu chip-id.  Note this may become optional for future
>> +  devices if we can reliably read the chipid from hw
>
> What's the problem with reading chipid from HW at the moment?
>
> Should this possibly be optional, and only if not possible to read from
> HW?

As far as I can tell from diving downstream android kgsl code, seems
like some a2xx you might be able to read the value from hw.  Beyond
that I'm not entirely sure.  (Remember, no docs.)

I'm leaving it as-is and if someone who has docs wants to submit patch
to change it, that is fine.

>> +- qcom,gpu-pwrlevels: array of OPPs, sorted highest to lowest
>
> This is confusing. This is a node rather than a property, and in general
> it's not a good idea to rely on node ordering (there's no such thing as
> an array of nodes in the DTB).
>
>> +  - compatible: "qcom,gpu-pwrlevels"
>> +  - for each qcom,gpu-pwrlevel:
>
> Is that a compatible string for each node, name, or what?
>
> Any reason for having this as a container node at all?

some of the decisions are made to make it easier to re-use/support
existing bindings in downstream kernel..

>> +    - qcom,gpu-freq: requested gpu clock speed
>> +    - NOTE: downstream android driver defines additional parameters to
>> +      configure memory bandwidth scaling per OPP.
>
> So? Either define those or don't bother. Having a halfway house binding
> is not helpful.

I'm not really in a position to document them at this point, sorry.

>> +
>> +Optional properties:
>> +- n/a
>
> Drop this if there are no optional properties.
>
>> +
>> +Example:
>> +
>> +/ {
>> +       ...
>> +
>> +       gpu: qcom,kgsl-3d0@4300000 {
>> +               compatible = "qcom,adreno-3xx";
>> +               reg = <0x04300000 0x20000>;
>> +               reg-names = "kgsl_3d0_reg_memory";
>> +               interrupts = <GIC_SPI 80 0>;
>> +               interrupt-names = "kgsl_3d0_irq";
>> +               clock-names =
>> +                   "core_clk",
>> +                   "iface_clk",
>> +                   "mem_iface_clk";
>> +               clocks =
>> +                   <&mmcc GFX3D_CLK>,
>> +                   <&mmcc GFX3D_AHB_CLK>,
>> +                   <&mmcc MMSS_IMEM_AHB_CLK>;
>> +               qcom,chipid = <0x03020100>;
>> +               qcom,gpu-pwrlevels {
>> +                       compatible = "qcom,gpu-pwrlevels";
>> +                       qcom,gpu-pwrlevel@0 {
>> +                               qcom,gpu-freq = <450000000>;
>> +                       };
>> +                       qcom,gpu-pwrlevel@1 {
>> +                               qcom,gpu-freq = <27000000>;
>> +                       };
>> +               };
>> +       };
>> +};
>> diff --git a/Documentation/devicetree/bindings/drm/msm/hdmi.txt b/Documentation/devicetree/bindings/drm/msm/hdmi.txt
>> new file mode 100644
>> index 0000000..051a49f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/drm/msm/hdmi.txt
>> @@ -0,0 +1,43 @@
>> +Qualcomm adreno/snapdragon hdmi output
>> +
>> +Required properties:
>> +- compatible: "qcom,hdmi-tx-8x60", "qcom,hdmi-tx-8960", "qcom,hdmi-tx-8x74"
>
> Please don't use wildcard strings.
>
> Is a node expected to have one of these entries, or all?

not quite sure what you mean.. Yes a node is expected to have
'compatible'.  Which should have one of those values (with potentially
more values added later)

> I would recommend using a bulleted list to describe compatible strings.
> It allows for easier extension and the easy addition of notes. e.g.
>
> - compatible: should contain:
>   * "foo,bar-xtreme" for bar variants with the xtreme extensions.
>   * "foo,bar" for all bar variants (inc. xtreme).
>   * "foo,baz" for baz variants.
>   * "foo,foo" for all variants.

sure

>> +- reg: Physical base address and length of the controller's registers.
>> +- interrupts: The interrupt outputs from the controller.
>> +- clocks: device clocks
>
> One entry? Multiple?
>
> What do they correspond to on the data sheet?

get someone to send me a data sheet.  I'd love to tell you then ;-)

> Names?
>
>> +  See ../clocks/clock-bindings.txt for details.
>> +- qcom,hdmi-tx-ddc-clk: ddc clk pin
>> +- qcom,hdmi-tx-ddc-data: ddc data pin
>> +- qcom,hdmi-tx-hpd: hpd pin
>
> GPIOs should be called *-gpios.

again just trying to be compatible with some existing downstream DT..

>> +- core-vdda-supply: phandle to supply regulator
>> +- hdmi-mux-supply: phandle to mux regulator
>> +
>> +Optional properties:
>> +- qcom,hdmi-tx-mux-en: hdmi mux enable pin
>> +- qcom,hdmi-tx-mux-sel: hdmi mux select pin
>
> GPIOs again.
>
>> +
>> +Example:
>> +
>> +/ {
>> +       ...
>> +
>> +       hdmi: qcom,hdmi-tx-8960@4a00000 {
>> +               compatible = "qcom,hdmi-tx-8960";
>> +               reg-names = "core_physical";
>> +               reg = <0x04a00000 0x1000>;
>> +               interrupts = <GIC_SPI 79 0>;
>> +               clock-names =
>> +                   "core_clk",
>> +                   "master_iface_clk",
>> +                   "slave_iface_clk";
>> +               clocks =
>> +                   <&mmcc HDMI_APP_CLK>,
>> +                   <&mmcc HDMI_M_AHB_CLK>,
>> +                   <&mmcc HDMI_S_AHB_CLK>;
>> +               qcom,hdmi-tx-ddc-clk = <&msmgpio 70 GPIO_ACTIVE_HIGH>;
>> +               qcom,hdmi-tx-ddc-data = <&msmgpio 71 GPIO_ACTIVE_HIGH>;
>> +               qcom,hdmi-tx-hpd = <&msmgpio 72 GPIO_ACTIVE_HIGH>;
>> +               core-vdda-supply = <&pm8921_hdmi_mvs>;
>> +               hdmi-mux-supply = <&ext_3p3v>;
>> +       };
>> +};
>> diff --git a/Documentation/devicetree/bindings/drm/msm/msm.txt b/Documentation/devicetree/bindings/drm/msm/msm.txt
>> new file mode 100644
>> index 0000000..484cc12
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/drm/msm/msm.txt
>> @@ -0,0 +1,40 @@
>> +Qualcomm adreno/snapdragon
>
> What exactly is this unit? Judging by the GPU phandle it's not the GPU
> itself.

the display

>> +
>> +Required properties:
>> +- compatible: "qcom,mdp" (mdp4) or "qcom,mdss_mdp" (mdp5)
>> +- reg: Physical base address and length of the controller's registers.
>> +- interrupts: The interrupt outputs from the controller.
>
> All of my prior points for these properties stand.
>
>> +- connectors: array of phandles for output device(s)
>
> What are valid devices to point this at?
>
>> +- clocks: device clocks
>
> Likewise, see my above points regarding clocks.
>
>> +  See ../clocks/clock-bindings.txt for details.
>> +
>> +Optional properties:
>> +- gpus: phandle for gpu device
>
> What exactly is this used for?

'gpus' and 'connectors' is basically part of the componentized device
support.  It is how the master/toplevel drm device knows what
sub-devices exist.

Currently there is just one valid possibility for each, but as we add
DSI, DP, etc, and support for various other generations there will
hopefully be more.  (For example, some older snapdragons had one 3d
core and zero to two 2d cores.)

BR,
-R

> Thanks,
> Mark.

Re: [RFC] drm/msm: DT support for 8960/8064

[Jordan Crouse]

On 07/02/2014 03:01 PM, Rob Clark wrote:
> As far as I can tell from diving downstream android kgsl code, seems
> like some a2xx you might be able to read the value from hw.  Beyond
> that I'm not entirely sure.  (Remember, no docs.)

Ohhh! I get to tell a story!

The on chip registers have a long and consistent history of being wrong.
And not just the spin ID being wrong - the major and minor have had their
issues too. It got to the point that sometime around the A320 timeframe
we put our foot down and decided that we would never again rely on the
hardware. The reasoning is that the SoC and the GPU are on the same die
so we can use the one to identify the other. And thats what we've done
since then - first in the board files and now in the DT.

So long story short - the registers are there, we don't trust them as
far as we could throw them.  Use the SoC instead.

Jordan

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [RFC] drm/msm: DT support for 8960/8064

[Mark Rutland]

On Wed, Jul 02, 2014 at 10:01:40PM +0100, Rob Clark wrote:
> On Wed, Jul 2, 2014 at 2:09 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Tue, Jul 01, 2014 at 07:57:35PM +0100, Rob Clark wrote:
> >> Now that we (almost) have enough dependencies in place (MMCC, RPM, etc),
> >> add necessary DT support so that we can use drm/msm on upstream kernel.
> >>
> >> Signed-off-by: Rob Clark <robdclark@gmail.com>
> >> ---
> >> Commence bikeshedding :-)
> >>
> >>  Documentation/devicetree/bindings/drm/msm/gpu.txt  | 51 ++++++++++++++++++++
> >>  Documentation/devicetree/bindings/drm/msm/hdmi.txt | 43 +++++++++++++++++
> >>  Documentation/devicetree/bindings/drm/msm/msm.txt  | 40 ++++++++++++++++
> >>  drivers/gpu/drm/msm/adreno/a3xx_gpu.c              |  2 +
> >>  drivers/gpu/drm/msm/hdmi/hdmi.c                    | 55 ++++++++++++++--------
> >>  drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c            | 10 ++--
> >>  drivers/gpu/drm/msm/msm_drv.c                      | 29 ++++++++++--
> >>  7 files changed, 204 insertions(+), 26 deletions(-)
> >>  create mode 100644 Documentation/devicetree/bindings/drm/msm/gpu.txt
> >>  create mode 100644 Documentation/devicetree/bindings/drm/msm/hdmi.txt
> >>  create mode 100644 Documentation/devicetree/bindings/drm/msm/msm.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/drm/msm/gpu.txt b/Documentation/devicetree/bindings/drm/msm/gpu.txt
> >> new file mode 100644
> >> index 0000000..6e33efe
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/drm/msm/gpu.txt
> >> @@ -0,0 +1,51 @@
> >> +Qualcomm adreno/snapdragon GPU
> >> +
> >> +Required properties:
> >> +- compatible: "qcom,adreno-3xx"
> >
> > As Olof said, choose a definite name here, and have variants claim
> > compatibility with that in addition to a variant specific string.
> >
> >> +- reg: Physical base address and length of the controller's registers.
> >
> > Just the one reg entry?
> 
> as far as I know.. keep in mind, I'm working without docs

Sure. If we only know about one reg entrty that's fine.

> 
> > The example has reg-names. Either document the name or drop it.
> >
> >> +- interrupts: The interrupt outputs from the controller.
> >
> > How many? Which ones? Names?
> >
> >> +- clocks: device clocks
> >> +  See ../clocks/clock-bindings.txt for details.
> >
> > Similarly?
> >
> > I should be able to read the binding and put together a DTS. Currently I
> > cannot.
> >
> >> +- qcom,chipid: gpu chip-id.  Note this may become optional for future
> >> +  devices if we can reliably read the chipid from hw
> >
> > What's the problem with reading chipid from HW at the moment?
> >
> > Should this possibly be optional, and only if not possible to read from
> > HW?
> 
> As far as I can tell from diving downstream android kgsl code, seems
> like some a2xx you might be able to read the value from hw.  Beyond
> that I'm not entirely sure.  (Remember, no docs.)
> 
> I'm leaving it as-is and if someone who has docs wants to submit patch
> to change it, that is fine.

My concern is that the statement "this may become optional" is useless,
as it doesn't tell you what expected if it's not present.

I would reword this as:

- qcom,chip-id: The gpu chip-id, if the hardware value is not reliable.

And then read from the HW if it's not present.

> >> +- qcom,gpu-pwrlevels: array of OPPs, sorted highest to lowest
> >
> > This is confusing. This is a node rather than a property, and in general
> > it's not a good idea to rely on node ordering (there's no such thing as
> > an array of nodes in the DTB).
> >
> >> +  - compatible: "qcom,gpu-pwrlevels"
> >> +  - for each qcom,gpu-pwrlevel:
> >
> > Is that a compatible string for each node, name, or what?
> >
> > Any reason for having this as a container node at all?
> 
> some of the decisions are made to make it easier to re-use/support
> existing bindings in downstream kernel..

I don't see why we should be beholden to downstream kernel bindings.

> 
> >> +    - qcom,gpu-freq: requested gpu clock speed
> >> +    - NOTE: downstream android driver defines additional parameters to
> >> +      configure memory bandwidth scaling per OPP.
> >
> > So? Either define those or don't bother. Having a halfway house binding
> > is not helpful.
> 
> I'm not really in a position to document them at this point, sorry.

Then drop it.

> >> +
> >> +Optional properties:
> >> +- n/a
> >
> > Drop this if there are no optional properties.
> >
> >> +
> >> +Example:
> >> +
> >> +/ {
> >> +       ...
> >> +
> >> +       gpu: qcom,kgsl-3d0@4300000 {
> >> +               compatible = "qcom,adreno-3xx";
> >> +               reg = <0x04300000 0x20000>;
> >> +               reg-names = "kgsl_3d0_reg_memory";
> >> +               interrupts = <GIC_SPI 80 0>;
> >> +               interrupt-names = "kgsl_3d0_irq";
> >> +               clock-names =
> >> +                   "core_clk",
> >> +                   "iface_clk",
> >> +                   "mem_iface_clk";
> >> +               clocks =
> >> +                   <&mmcc GFX3D_CLK>,
> >> +                   <&mmcc GFX3D_AHB_CLK>,
> >> +                   <&mmcc MMSS_IMEM_AHB_CLK>;
> >> +               qcom,chipid = <0x03020100>;
> >> +               qcom,gpu-pwrlevels {
> >> +                       compatible = "qcom,gpu-pwrlevels";
> >> +                       qcom,gpu-pwrlevel@0 {
> >> +                               qcom,gpu-freq = <450000000>;
> >> +                       };
> >> +                       qcom,gpu-pwrlevel@1 {
> >> +                               qcom,gpu-freq = <27000000>;
> >> +                       };
> >> +               };
> >> +       };
> >> +};
> >> diff --git a/Documentation/devicetree/bindings/drm/msm/hdmi.txt b/Documentation/devicetree/bindings/drm/msm/hdmi.txt
> >> new file mode 100644
> >> index 0000000..051a49f
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/drm/msm/hdmi.txt
> >> @@ -0,0 +1,43 @@
> >> +Qualcomm adreno/snapdragon hdmi output
> >> +
> >> +Required properties:
> >> +- compatible: "qcom,hdmi-tx-8x60", "qcom,hdmi-tx-8960", "qcom,hdmi-tx-8x74"
> >
> > Please don't use wildcard strings.
> >
> > Is a node expected to have one of these entries, or all?
> 
> not quite sure what you mean.. Yes a node is expected to have
> 'compatible'.  Which should have one of those values (with potentially
> more values added later)

Sorry for the lack of clarity. I meant is it expected that I should
have:

compatible = "qcom,hdmi-tx-8x60", "qcom,hdmi-tx-8960",
	     "qcom,hdmi-tx-8x74";

Or just a single string for a given device?

I assume the latter.

> 
> > I would recommend using a bulleted list to describe compatible strings.
> > It allows for easier extension and the easy addition of notes. e.g.
> >
> > - compatible: should contain:
> >   * "foo,bar-xtreme" for bar variants with the xtreme extensions.
> >   * "foo,bar" for all bar variants (inc. xtreme).
> >   * "foo,baz" for baz variants.
> >   * "foo,foo" for all variants.
> 
> sure
> 
> >> +- reg: Physical base address and length of the controller's registers.
> >> +- interrupts: The interrupt outputs from the controller.
> >> +- clocks: device clocks
> >
> > One entry? Multiple?
> >
> > What do they correspond to on the data sheet?
> 
> get someone to send me a data sheet.  I'd love to tell you then ;-)

Sure :)

> > Names?
> >
> >> +  See ../clocks/clock-bindings.txt for details.
> >> +- qcom,hdmi-tx-ddc-clk: ddc clk pin
> >> +- qcom,hdmi-tx-ddc-data: ddc data pin
> >> +- qcom,hdmi-tx-hpd: hpd pin
> >
> > GPIOs should be called *-gpios.
> 
> again just trying to be compatible with some existing downstream DT..

And again I'm not certain that's a good idea...

> >> +- core-vdda-supply: phandle to supply regulator
> >> +- hdmi-mux-supply: phandle to mux regulator
> >> +
> >> +Optional properties:
> >> +- qcom,hdmi-tx-mux-en: hdmi mux enable pin
> >> +- qcom,hdmi-tx-mux-sel: hdmi mux select pin
> >
> > GPIOs again.
> >
> >> +
> >> +Example:
> >> +
> >> +/ {
> >> +       ...
> >> +
> >> +       hdmi: qcom,hdmi-tx-8960@4a00000 {
> >> +               compatible = "qcom,hdmi-tx-8960";
> >> +               reg-names = "core_physical";
> >> +               reg = <0x04a00000 0x1000>;
> >> +               interrupts = <GIC_SPI 79 0>;
> >> +               clock-names =
> >> +                   "core_clk",
> >> +                   "master_iface_clk",
> >> +                   "slave_iface_clk";
> >> +               clocks =
> >> +                   <&mmcc HDMI_APP_CLK>,
> >> +                   <&mmcc HDMI_M_AHB_CLK>,
> >> +                   <&mmcc HDMI_S_AHB_CLK>;
> >> +               qcom,hdmi-tx-ddc-clk = <&msmgpio 70 GPIO_ACTIVE_HIGH>;
> >> +               qcom,hdmi-tx-ddc-data = <&msmgpio 71 GPIO_ACTIVE_HIGH>;
> >> +               qcom,hdmi-tx-hpd = <&msmgpio 72 GPIO_ACTIVE_HIGH>;
> >> +               core-vdda-supply = <&pm8921_hdmi_mvs>;
> >> +               hdmi-mux-supply = <&ext_3p3v>;
> >> +       };
> >> +};
> >> diff --git a/Documentation/devicetree/bindings/drm/msm/msm.txt b/Documentation/devicetree/bindings/drm/msm/msm.txt
> >> new file mode 100644
> >> index 0000000..484cc12
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/drm/msm/msm.txt
> >> @@ -0,0 +1,40 @@
> >> +Qualcomm adreno/snapdragon
> >
> > What exactly is this unit? Judging by the GPU phandle it's not the GPU
> > itself.
> 
> the display

I see. Could that be mentioned in the heading line above?

> 
> >> +
> >> +Required properties:
> >> +- compatible: "qcom,mdp" (mdp4) or "qcom,mdss_mdp" (mdp5)
> >> +- reg: Physical base address and length of the controller's registers.
> >> +- interrupts: The interrupt outputs from the controller.
> >
> > All of my prior points for these properties stand.
> >
> >> +- connectors: array of phandles for output device(s)
> >
> > What are valid devices to point this at?
> >
> >> +- clocks: device clocks
> >
> > Likewise, see my above points regarding clocks.
> >
> >> +  See ../clocks/clock-bindings.txt for details.
> >> +
> >> +Optional properties:
> >> +- gpus: phandle for gpu device
> >
> > What exactly is this used for?
> 
> 'gpus' and 'connectors' is basically part of the componentized device
> support.  It is how the master/toplevel drm device knows what
> sub-devices exist.
> 
> Currently there is just one valid possibility for each, but as we add
> DSI, DP, etc, and support for various other generations there will
> hopefully be more.  (For example, some older snapdragons had one 3d
> core and zero to two 2d cores.)

Ok. I must admit to being unfamiliar with how that's expected to work.

Thanks,
Mark.

Re: [RFC] drm/msm: DT support for 8960/8064

[Rob Clark]

On Thu, Jul 3, 2014 at 5:15 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Jul 02, 2014 at 10:01:40PM +0100, Rob Clark wrote:
>> On Wed, Jul 2, 2014 at 2:09 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Tue, Jul 01, 2014 at 07:57:35PM +0100, Rob Clark wrote:
>> >> Now that we (almost) have enough dependencies in place (MMCC, RPM, etc),
>> >> add necessary DT support so that we can use drm/msm on upstream kernel.
>> >>
>> >> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> >> ---
>> >> Commence bikeshedding :-)
>> >>
>> >>  Documentation/devicetree/bindings/drm/msm/gpu.txt  | 51 ++++++++++++++++++++
>> >>  Documentation/devicetree/bindings/drm/msm/hdmi.txt | 43 +++++++++++++++++
>> >>  Documentation/devicetree/bindings/drm/msm/msm.txt  | 40 ++++++++++++++++
>> >>  drivers/gpu/drm/msm/adreno/a3xx_gpu.c              |  2 +
>> >>  drivers/gpu/drm/msm/hdmi/hdmi.c                    | 55 ++++++++++++++--------
>> >>  drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c            | 10 ++--
>> >>  drivers/gpu/drm/msm/msm_drv.c                      | 29 ++++++++++--
>> >>  7 files changed, 204 insertions(+), 26 deletions(-)
>> >>  create mode 100644 Documentation/devicetree/bindings/drm/msm/gpu.txt
>> >>  create mode 100644 Documentation/devicetree/bindings/drm/msm/hdmi.txt
>> >>  create mode 100644 Documentation/devicetree/bindings/drm/msm/msm.txt
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/drm/msm/gpu.txt b/Documentation/devicetree/bindings/drm/msm/gpu.txt
>> >> new file mode 100644
>> >> index 0000000..6e33efe
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/drm/msm/gpu.txt
>> >> @@ -0,0 +1,51 @@
>> >> +Qualcomm adreno/snapdragon GPU
>> >> +
>> >> +Required properties:
>> >> +- compatible: "qcom,adreno-3xx"
>> >
>> > As Olof said, choose a definite name here, and have variants claim
>> > compatibility with that in addition to a variant specific string.
>> >
>> >> +- reg: Physical base address and length of the controller's registers.
>> >
>> > Just the one reg entry?
>>
>> as far as I know.. keep in mind, I'm working without docs
>
> Sure. If we only know about one reg entrty that's fine.
>
>>
>> > The example has reg-names. Either document the name or drop it.
>> >
>> >> +- interrupts: The interrupt outputs from the controller.
>> >
>> > How many? Which ones? Names?
>> >
>> >> +- clocks: device clocks
>> >> +  See ../clocks/clock-bindings.txt for details.
>> >
>> > Similarly?
>> >
>> > I should be able to read the binding and put together a DTS. Currently I
>> > cannot.
>> >
>> >> +- qcom,chipid: gpu chip-id.  Note this may become optional for future
>> >> +  devices if we can reliably read the chipid from hw
>> >
>> > What's the problem with reading chipid from HW at the moment?
>> >
>> > Should this possibly be optional, and only if not possible to read from
>> > HW?
>>
>> As far as I can tell from diving downstream android kgsl code, seems
>> like some a2xx you might be able to read the value from hw.  Beyond
>> that I'm not entirely sure.  (Remember, no docs.)
>>
>> I'm leaving it as-is and if someone who has docs wants to submit patch
>> to change it, that is fine.
>
> My concern is that the statement "this may become optional" is useless,
> as it doesn't tell you what expected if it's not present.
>
> I would reword this as:
>
> - qcom,chip-id: The gpu chip-id, if the hardware value is not reliable.

yeah, ok.  That is more clear.

> And then read from the HW if it's not present.
>
>> >> +- qcom,gpu-pwrlevels: array of OPPs, sorted highest to lowest
>> >
>> > This is confusing. This is a node rather than a property, and in general
>> > it's not a good idea to rely on node ordering (there's no such thing as
>> > an array of nodes in the DTB).

btw, we don't actually rely on the ordering in the parsing code.
Although I think it is more readable if it is kept sorted.  So I
suppose consider that as more of a suggestion.

>> >
>> >> +  - compatible: "qcom,gpu-pwrlevels"
>> >> +  - for each qcom,gpu-pwrlevel:
>> >
>> > Is that a compatible string for each node, name, or what?
>> >
>> > Any reason for having this as a container node at all?
>>
>> some of the decisions are made to make it easier to re-use/support
>> existing bindings in downstream kernel..
>
> I don't see why we should be beholden to downstream kernel bindings.

Well, unfortunately since devices that run upstream kernel are few and
far between, I get to port upstream driver to more downstream device
kernels than I'd care to.  To simplify this, as much as possible I'd
like to share DT binding code.  The alternative is more error prone
and time consuming, and it's not exactly like I'm running out of
things to do.

>>
>> >> +    - qcom,gpu-freq: requested gpu clock speed
>> >> +    - NOTE: downstream android driver defines additional parameters to
>> >> +      configure memory bandwidth scaling per OPP.
>> >
>> > So? Either define those or don't bother. Having a halfway house binding
>> > is not helpful.
>>
>> I'm not really in a position to document them at this point, sorry.
>
> Then drop it.

Not sure I understand.  The gpu-freq we need.  I could drop the NOTE,
although it does kind of explain why a simple list of integers is not
used.

Anything added later would be optional, ofc, for backwards compat.

>> >> +
>> >> +Optional properties:
>> >> +- n/a
>> >
>> > Drop this if there are no optional properties.
>> >
>> >> +
>> >> +Example:
>> >> +
>> >> +/ {
>> >> +       ...
>> >> +
>> >> +       gpu: qcom,kgsl-3d0@4300000 {
>> >> +               compatible = "qcom,adreno-3xx";
>> >> +               reg = <0x04300000 0x20000>;
>> >> +               reg-names = "kgsl_3d0_reg_memory";
>> >> +               interrupts = <GIC_SPI 80 0>;
>> >> +               interrupt-names = "kgsl_3d0_irq";
>> >> +               clock-names =
>> >> +                   "core_clk",
>> >> +                   "iface_clk",
>> >> +                   "mem_iface_clk";
>> >> +               clocks =
>> >> +                   <&mmcc GFX3D_CLK>,
>> >> +                   <&mmcc GFX3D_AHB_CLK>,
>> >> +                   <&mmcc MMSS_IMEM_AHB_CLK>;
>> >> +               qcom,chipid = <0x03020100>;
>> >> +               qcom,gpu-pwrlevels {
>> >> +                       compatible = "qcom,gpu-pwrlevels";
>> >> +                       qcom,gpu-pwrlevel@0 {
>> >> +                               qcom,gpu-freq = <450000000>;
>> >> +                       };
>> >> +                       qcom,gpu-pwrlevel@1 {
>> >> +                               qcom,gpu-freq = <27000000>;
>> >> +                       };
>> >> +               };
>> >> +       };
>> >> +};
>> >> diff --git a/Documentation/devicetree/bindings/drm/msm/hdmi.txt b/Documentation/devicetree/bindings/drm/msm/hdmi.txt
>> >> new file mode 100644
>> >> index 0000000..051a49f
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/drm/msm/hdmi.txt
>> >> @@ -0,0 +1,43 @@
>> >> +Qualcomm adreno/snapdragon hdmi output
>> >> +
>> >> +Required properties:
>> >> +- compatible: "qcom,hdmi-tx-8x60", "qcom,hdmi-tx-8960", "qcom,hdmi-tx-8x74"
>> >
>> > Please don't use wildcard strings.
>> >
>> > Is a node expected to have one of these entries, or all?
>>
>> not quite sure what you mean.. Yes a node is expected to have
>> 'compatible'.  Which should have one of those values (with potentially
>> more values added later)
>
> Sorry for the lack of clarity. I meant is it expected that I should
> have:
>
> compatible = "qcom,hdmi-tx-8x60", "qcom,hdmi-tx-8960",
>              "qcom,hdmi-tx-8x74";
>
> Or just a single string for a given device?
>
> I assume the latter.

oh, yes.  I kinda thought the use of "compatible" everywhere was
enough that it would be obvious that this meant "pick one"..

I guess re-arranging as a bullet-list would make that more clear.

>>
>> > I would recommend using a bulleted list to describe compatible strings.
>> > It allows for easier extension and the easy addition of notes. e.g.
>> >
>> > - compatible: should contain:
>> >   * "foo,bar-xtreme" for bar variants with the xtreme extensions.
>> >   * "foo,bar" for all bar variants (inc. xtreme).
>> >   * "foo,baz" for baz variants.
>> >   * "foo,foo" for all variants.
>>
>> sure
>>
>> >> +- reg: Physical base address and length of the controller's registers.
>> >> +- interrupts: The interrupt outputs from the controller.
>> >> +- clocks: device clocks
>> >
>> > One entry? Multiple?
>> >
>> > What do they correspond to on the data sheet?
>>
>> get someone to send me a data sheet.  I'd love to tell you then ;-)
>
> Sure :)
>
>> > Names?
>> >
>> >> +  See ../clocks/clock-bindings.txt for details.
>> >> +- qcom,hdmi-tx-ddc-clk: ddc clk pin
>> >> +- qcom,hdmi-tx-ddc-data: ddc data pin
>> >> +- qcom,hdmi-tx-hpd: hpd pin
>> >
>> > GPIOs should be called *-gpios.
>>
>> again just trying to be compatible with some existing downstream DT..
>
> And again I'm not certain that's a good idea...
>
>> >> +- core-vdda-supply: phandle to supply regulator
>> >> +- hdmi-mux-supply: phandle to mux regulator
>> >> +
>> >> +Optional properties:
>> >> +- qcom,hdmi-tx-mux-en: hdmi mux enable pin
>> >> +- qcom,hdmi-tx-mux-sel: hdmi mux select pin
>> >
>> > GPIOs again.
>> >
>> >> +
>> >> +Example:
>> >> +
>> >> +/ {
>> >> +       ...
>> >> +
>> >> +       hdmi: qcom,hdmi-tx-8960@4a00000 {
>> >> +               compatible = "qcom,hdmi-tx-8960";
>> >> +               reg-names = "core_physical";
>> >> +               reg = <0x04a00000 0x1000>;
>> >> +               interrupts = <GIC_SPI 79 0>;
>> >> +               clock-names =
>> >> +                   "core_clk",
>> >> +                   "master_iface_clk",
>> >> +                   "slave_iface_clk";
>> >> +               clocks =
>> >> +                   <&mmcc HDMI_APP_CLK>,
>> >> +                   <&mmcc HDMI_M_AHB_CLK>,
>> >> +                   <&mmcc HDMI_S_AHB_CLK>;
>> >> +               qcom,hdmi-tx-ddc-clk = <&msmgpio 70 GPIO_ACTIVE_HIGH>;
>> >> +               qcom,hdmi-tx-ddc-data = <&msmgpio 71 GPIO_ACTIVE_HIGH>;
>> >> +               qcom,hdmi-tx-hpd = <&msmgpio 72 GPIO_ACTIVE_HIGH>;
>> >> +               core-vdda-supply = <&pm8921_hdmi_mvs>;
>> >> +               hdmi-mux-supply = <&ext_3p3v>;
>> >> +       };
>> >> +};
>> >> diff --git a/Documentation/devicetree/bindings/drm/msm/msm.txt b/Documentation/devicetree/bindings/drm/msm/msm.txt
>> >> new file mode 100644
>> >> index 0000000..484cc12
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/drm/msm/msm.txt
>> >> @@ -0,0 +1,40 @@
>> >> +Qualcomm adreno/snapdragon
>> >
>> > What exactly is this unit? Judging by the GPU phandle it's not the GPU
>> > itself.
>>
>> the display
>
> I see. Could that be mentioned in the heading line above?

will do

>>
>> >> +
>> >> +Required properties:
>> >> +- compatible: "qcom,mdp" (mdp4) or "qcom,mdss_mdp" (mdp5)
>> >> +- reg: Physical base address and length of the controller's registers.
>> >> +- interrupts: The interrupt outputs from the controller.
>> >
>> > All of my prior points for these properties stand.
>> >
>> >> +- connectors: array of phandles for output device(s)
>> >
>> > What are valid devices to point this at?
>> >
>> >> +- clocks: device clocks
>> >
>> > Likewise, see my above points regarding clocks.
>> >
>> >> +  See ../clocks/clock-bindings.txt for details.
>> >> +
>> >> +Optional properties:
>> >> +- gpus: phandle for gpu device
>> >
>> > What exactly is this used for?
>>
>> 'gpus' and 'connectors' is basically part of the componentized device
>> support.  It is how the master/toplevel drm device knows what
>> sub-devices exist.
>>
>> Currently there is just one valid possibility for each, but as we add
>> DSI, DP, etc, and support for various other generations there will
>> hopefully be more.  (For example, some older snapdragons had one 3d
>> core and zero to two 2d cores.)
>
> Ok. I must admit to being unfamiliar with how that's expected to work.

in case you are curious:
http://lists.freedesktop.org/archives/dri-devel/2014-January/051249.html

BR,
-R

> Thanks,
> Mark.

Re: [RFC] drm/msm: DT support for 8960/8064

[sviau]

Hi Rob,

> Now that we (almost) have enough dependencies in place (MMCC, RPM, etc),
> add necessary DT support so that we can use drm/msm on upstream kernel.
>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
> Commence bikeshedding :-)
>
<snip>
> diff --git a/Documentation/devicetree/bindings/drm/msm/hdmi.txt
> b/Documentation/devicetree/bindings/drm/msm/hdmi.txt
> new file mode 100644
> index 0000000..051a49f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/drm/msm/hdmi.txt
> @@ -0,0 +1,43 @@
> +Qualcomm adreno/snapdragon hdmi output
> +
> +Required properties:
> +- compatible: "qcom,hdmi-tx-8x60", "qcom,hdmi-tx-8960",
> "qcom,hdmi-tx-8x74"
> +- reg: Physical base address and length of the controller's registers.

Since you are adding "qcom,hdmi-tx-8x74" (separate address space for PHY
registers) in the compatible entry, how about this for the register
description:
- reg: Physical base address and length of the controllers' registers.
- reg-names: names corresponding to the defined register sets,
    - "core_physical": HDMI Core registers
    - (optional) "phy_physical": HDMI PHY registers

> +- interrupts: The interrupt outputs from the controller.
> +- clocks: device clocks
> +  See ../clocks/clock-bindings.txt for details.
> +- qcom,hdmi-tx-ddc-clk: ddc clk pin
> +- qcom,hdmi-tx-ddc-data: ddc data pin
> +- qcom,hdmi-tx-hpd: hpd pin
> +- core-vdda-supply: phandle to supply regulator
> +- hdmi-mux-supply: phandle to mux regulator
> +
> +Optional properties:
> +- qcom,hdmi-tx-mux-en: hdmi mux enable pin
> +- qcom,hdmi-tx-mux-sel: hdmi mux select pin
> +
> +Example:
> +
> +/ {
> +	...
> +
> +	hdmi: qcom,hdmi-tx-8960@4a00000 {
> +		compatible = "qcom,hdmi-tx-8960";
> +		reg-names = "core_physical";
> +		reg = <0x04a00000 0x1000>;
> +		interrupts = <GIC_SPI 79 0>;
> +		clock-names =
> +		    "core_clk",
> +		    "master_iface_clk",
> +		    "slave_iface_clk";
> +		clocks =
> +		    <&mmcc HDMI_APP_CLK>,
> +		    <&mmcc HDMI_M_AHB_CLK>,
> +		    <&mmcc HDMI_S_AHB_CLK>;
> +		qcom,hdmi-tx-ddc-clk = <&msmgpio 70 GPIO_ACTIVE_HIGH>;
> +		qcom,hdmi-tx-ddc-data = <&msmgpio 71 GPIO_ACTIVE_HIGH>;
> +		qcom,hdmi-tx-hpd = <&msmgpio 72 GPIO_ACTIVE_HIGH>;
> +		core-vdda-supply = <&pm8921_hdmi_mvs>;
> +		hdmi-mux-supply = <&ext_3p3v>;
> +	};
> +};
> diff --git a/Documentation/devicetree/bindings/drm/msm/msm.txt
b/Documentation/devicetree/bindings/drm/msm/msm.txt
> new file mode 100644
> index 0000000..484cc12
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/drm/msm/msm.txt
>  <at>  <at>  -0,0 +1,40  <at>  <at>
> +Qualcomm adreno/snapdragon
> +
> +Required properties:
> +- compatible: "qcom,mdp" (mdp4) or "qcom,mdss_mdp" (mdp5)
> +- reg: Physical base address and length of the controller's registers.

As per the code (mdp5_kms.c), there are two sets of registers: "mdp_phys"
and "vbif_phys". They should probably be added in the description here.
"reg-names" entry might be needed as well.

> +- interrupts: The interrupt outputs from the controller.
> +- connectors: array of phandles for output device(s)
> +- clocks: device clocks
> +  See ../clocks/clock-bindings.txt for details.
> +
> +Optional properties:
> +- gpus: phandle for gpu device
> +
> +Example:
> +
> +/ {
> +	...
> +
> +	mdp: qcom,mdp <at> 5100000 {
> +		compatible = "qcom,mdp";
> +		reg = <0x05100000 0xf0000>;
> +		interrupts = <GIC_SPI 75 0>;
> +		connectors = <&hdmi>;
> +		gpus = <&gpu>;
> +		clock-names =
> +		    "core_clk",
> +		    "iface_clk",
> +		    "lut_clk",
> +		    "src_clk",
> +		    "hdmi_clk",
> +		    "mdp_clk";
> +		clocks =
> +		    <&mmcc MDP_SRC>,
> +		    <&mmcc MDP_AHB_CLK>,
> +		    <&mmcc MDP_LUT_CLK>,
> +		    <&mmcc TV_SRC>,
> +		    <&mmcc HDMI_TV_CLK>,
> +		    <&mmcc MDP_TV_CLK>;
> +	};
> +};
<snip>
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c
b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index 7f7aade..0ff8d46 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
<snip>
>  <at>  <at>  -273,24 +275,37  <at>  <at>  static int hdmi_bind(struct
device *dev, struct device *master, void *data)
>  		return gpio;
>  	}
>
> -	/* TODO actually use DT.. */
> -	static const char *hpd_reg_names[] = {"hpd-gdsc", "hpd-5v"};
> -	static const char *pwr_reg_names[] = {"core-vdda", "core-vcc"};
> -	static const char *hpd_clk_names[] = {"iface_clk", "core_clk",
"mdp_core_clk"};
> -	static unsigned long hpd_clk_freq[] = {0, 19200000, 0};
> -	static const char *pwr_clk_names[] = {"extp_clk", "alt_iface_clk"};
> +	if (of_device_is_compatible(of_node, "qcom,hdmi-tx-8x74")) {
> +		static const char *hpd_reg_names[] = {"hpd-gdsc", "hpd-5v"};
> +		static const char *pwr_reg_names[] = {"core-vdda", "core-vcc"};
> +		static const char *hpd_clk_names[] = {"iface_clk", "core_clk",
"mdp_core_clk"};
> +		static unsigned long hpd_clk_freq[] = {0, 19200000, 0};
> +		static const char *pwr_clk_names[] = {"extp_clk", "alt_iface_clk"};
> +		config.phy_init      = hdmi_phy_8x74_init;
> +		config.hpd_reg_names = hpd_reg_names;
> +		config.hpd_reg_cnt   = ARRAY_SIZE(hpd_reg_names);
> +		config.pwr_reg_names = pwr_reg_names;
> +		config.pwr_reg_cnt   = ARRAY_SIZE(pwr_reg_names);
> +		config.hpd_clk_names = hpd_clk_names;
> +		config.hpd_freq      = hpd_clk_freq;
> +		config.hpd_clk_cnt   = ARRAY_SIZE(hpd_clk_names);
> +		config.pwr_clk_names = pwr_clk_names;
> +		config.pwr_clk_cnt   = ARRAY_SIZE(pwr_clk_names);
> +	} else if (of_device_is_compatible(of_node, "qcom,hdmi-tx-8960")) {
> +		static const char *hpd_clk_names[] = {"core_clk", "master_iface_clk",
"slave_iface_clk"};
> +		static const char *hpd_reg_names[] = {"core-vdda", "hdmi-mux"};
> +		config.phy_init      = hdmi_phy_8960_init;
> +		config.hpd_reg_names = hpd_reg_names;
> +		config.hpd_reg_cnt   = ARRAY_SIZE(hpd_reg_names);
> +		config.hpd_clk_names = hpd_clk_names;
> +		config.hpd_clk_cnt   = ARRAY_SIZE(hpd_clk_names);
> +	} else if (of_device_is_compatible(of_node, "qcom,hdmi-tx-8x60")) {
> +		config.phy_init      = hdmi_phy_8x60_init;
> +	} else {
> +		dev_err(dev, "unknown phy: %s\n", of_node->name);
> +	}

Stéphane.

Re: [RFC] drm/msm: DT support for 8960/8064

[Rob Clark]

On Thu, Jul 3, 2014 at 12:36 PM,  <sviau@codeaurora.org> wrote:
> Hi Rob,
>
>> Now that we (almost) have enough dependencies in place (MMCC, RPM, etc),
>> add necessary DT support so that we can use drm/msm on upstream kernel.
>>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> ---
>> Commence bikeshedding :-)
>>
> <snip>
>> diff --git a/Documentation/devicetree/bindings/drm/msm/hdmi.txt
>> b/Documentation/devicetree/bindings/drm/msm/hdmi.txt
>> new file mode 100644
>> index 0000000..051a49f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/drm/msm/hdmi.txt
>> @@ -0,0 +1,43 @@
>> +Qualcomm adreno/snapdragon hdmi output
>> +
>> +Required properties:
>> +- compatible: "qcom,hdmi-tx-8x60", "qcom,hdmi-tx-8960",
>> "qcom,hdmi-tx-8x74"
>> +- reg: Physical base address and length of the controller's registers.
>
> Since you are adding "qcom,hdmi-tx-8x74" (separate address space for PHY
> registers) in the compatible entry, how about this for the register
> description:
> - reg: Physical base address and length of the controllers' registers.
> - reg-names: names corresponding to the defined register sets,
>     - "core_physical": HDMI Core registers
>     - (optional) "phy_physical": HDMI PHY registers

Oh, right.. I forgot 8x74 had phy in a separate region..

>> +- interrupts: The interrupt outputs from the controller.
>> +- clocks: device clocks
>> +  See ../clocks/clock-bindings.txt for details.
>> +- qcom,hdmi-tx-ddc-clk: ddc clk pin
>> +- qcom,hdmi-tx-ddc-data: ddc data pin
>> +- qcom,hdmi-tx-hpd: hpd pin
>> +- core-vdda-supply: phandle to supply regulator
>> +- hdmi-mux-supply: phandle to mux regulator
>> +
>> +Optional properties:
>> +- qcom,hdmi-tx-mux-en: hdmi mux enable pin
>> +- qcom,hdmi-tx-mux-sel: hdmi mux select pin
>> +
>> +Example:
>> +
>> +/ {
>> +     ...
>> +
>> +     hdmi: qcom,hdmi-tx-8960@4a00000 {
>> +             compatible = "qcom,hdmi-tx-8960";
>> +             reg-names = "core_physical";
>> +             reg = <0x04a00000 0x1000>;
>> +             interrupts = <GIC_SPI 79 0>;
>> +             clock-names =
>> +                 "core_clk",
>> +                 "master_iface_clk",
>> +                 "slave_iface_clk";
>> +             clocks =
>> +                 <&mmcc HDMI_APP_CLK>,
>> +                 <&mmcc HDMI_M_AHB_CLK>,
>> +                 <&mmcc HDMI_S_AHB_CLK>;
>> +             qcom,hdmi-tx-ddc-clk = <&msmgpio 70 GPIO_ACTIVE_HIGH>;
>> +             qcom,hdmi-tx-ddc-data = <&msmgpio 71 GPIO_ACTIVE_HIGH>;
>> +             qcom,hdmi-tx-hpd = <&msmgpio 72 GPIO_ACTIVE_HIGH>;
>> +             core-vdda-supply = <&pm8921_hdmi_mvs>;
>> +             hdmi-mux-supply = <&ext_3p3v>;
>> +     };
>> +};
>> diff --git a/Documentation/devicetree/bindings/drm/msm/msm.txt
> b/Documentation/devicetree/bindings/drm/msm/msm.txt
>> new file mode 100644
>> index 0000000..484cc12
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/drm/msm/msm.txt
>>  <at>  <at>  -0,0 +1,40  <at>  <at>
>> +Qualcomm adreno/snapdragon
>> +
>> +Required properties:
>> +- compatible: "qcom,mdp" (mdp4) or "qcom,mdss_mdp" (mdp5)
>> +- reg: Physical base address and length of the controller's registers.
>
> As per the code (mdp5_kms.c), there are two sets of registers: "mdp_phys"
> and "vbif_phys". They should probably be added in the description here.
> "reg-names" entry might be needed as well.

hmm.. well, we aren't actually using vbif_phys *yet*.. although not
really sure if that will change.

I am wondering if maybe we should split out mdp4 vs mdp5 DT bindings.
Or at least remove the mdp5 compat string for now.  So far I've only
got mdp4 working upstream, so probably a few things I've missed for
mdp5.  Maybe just removing "qcom,mdss_mdp" for the time being is safer
until we decide.

I guess we should be really close to having all the needed
dependencies for 8074/dragonboard, so I suppose that is something I
should play with.  Although seems unlikely that I'll have time until
after 3.17 merge window.  Mostly I wanted to get mdp4 working first,
since I already have enough overlay support working there to be useful
for atomic modeset/pageflip ;-)

BR,
-R

>> +- interrupts: The interrupt outputs from the controller.
>> +- connectors: array of phandles for output device(s)
>> +- clocks: device clocks
>> +  See ../clocks/clock-bindings.txt for details.
>> +
>> +Optional properties:
>> +- gpus: phandle for gpu device
>> +
>> +Example:
>> +
>> +/ {
>> +     ...
>> +
>> +     mdp: qcom,mdp <at> 5100000 {
>> +             compatible = "qcom,mdp";
>> +             reg = <0x05100000 0xf0000>;
>> +             interrupts = <GIC_SPI 75 0>;
>> +             connectors = <&hdmi>;
>> +             gpus = <&gpu>;
>> +             clock-names =
>> +                 "core_clk",
>> +                 "iface_clk",
>> +                 "lut_clk",
>> +                 "src_clk",
>> +                 "hdmi_clk",
>> +                 "mdp_clk";
>> +             clocks =
>> +                 <&mmcc MDP_SRC>,
>> +                 <&mmcc MDP_AHB_CLK>,
>> +                 <&mmcc MDP_LUT_CLK>,
>> +                 <&mmcc TV_SRC>,
>> +                 <&mmcc HDMI_TV_CLK>,
>> +                 <&mmcc MDP_TV_CLK>;
>> +     };
>> +};
> <snip>
>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c
> b/drivers/gpu/drm/msm/hdmi/hdmi.c
>> index 7f7aade..0ff8d46 100644
>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> <snip>
>>  <at>  <at>  -273,24 +275,37  <at>  <at>  static int hdmi_bind(struct
> device *dev, struct device *master, void *data)
>>               return gpio;
>>       }
>>
>> -     /* TODO actually use DT.. */
>> -     static const char *hpd_reg_names[] = {"hpd-gdsc", "hpd-5v"};
>> -     static const char *pwr_reg_names[] = {"core-vdda", "core-vcc"};
>> -     static const char *hpd_clk_names[] = {"iface_clk", "core_clk",
> "mdp_core_clk"};
>> -     static unsigned long hpd_clk_freq[] = {0, 19200000, 0};
>> -     static const char *pwr_clk_names[] = {"extp_clk", "alt_iface_clk"};
>> +     if (of_device_is_compatible(of_node, "qcom,hdmi-tx-8x74")) {
>> +             static const char *hpd_reg_names[] = {"hpd-gdsc", "hpd-5v"};
>> +             static const char *pwr_reg_names[] = {"core-vdda", "core-vcc"};
>> +             static const char *hpd_clk_names[] = {"iface_clk", "core_clk",
> "mdp_core_clk"};
>> +             static unsigned long hpd_clk_freq[] = {0, 19200000, 0};
>> +             static const char *pwr_clk_names[] = {"extp_clk", "alt_iface_clk"};
>> +             config.phy_init      = hdmi_phy_8x74_init;
>> +             config.hpd_reg_names = hpd_reg_names;
>> +             config.hpd_reg_cnt   = ARRAY_SIZE(hpd_reg_names);
>> +             config.pwr_reg_names = pwr_reg_names;
>> +             config.pwr_reg_cnt   = ARRAY_SIZE(pwr_reg_names);
>> +             config.hpd_clk_names = hpd_clk_names;
>> +             config.hpd_freq      = hpd_clk_freq;
>> +             config.hpd_clk_cnt   = ARRAY_SIZE(hpd_clk_names);
>> +             config.pwr_clk_names = pwr_clk_names;
>> +             config.pwr_clk_cnt   = ARRAY_SIZE(pwr_clk_names);
>> +     } else if (of_device_is_compatible(of_node, "qcom,hdmi-tx-8960")) {
>> +             static const char *hpd_clk_names[] = {"core_clk", "master_iface_clk",
> "slave_iface_clk"};
>> +             static const char *hpd_reg_names[] = {"core-vdda", "hdmi-mux"};
>> +             config.phy_init      = hdmi_phy_8960_init;
>> +             config.hpd_reg_names = hpd_reg_names;
>> +             config.hpd_reg_cnt   = ARRAY_SIZE(hpd_reg_names);
>> +             config.hpd_clk_names = hpd_clk_names;
>> +             config.hpd_clk_cnt   = ARRAY_SIZE(hpd_clk_names);
>> +     } else if (of_device_is_compatible(of_node, "qcom,hdmi-tx-8x60")) {
>> +             config.phy_init      = hdmi_phy_8x60_init;
>> +     } else {
>> +             dev_err(dev, "unknown phy: %s\n", of_node->name);
>> +     }
>
> Stéphane.
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFCv2] drm/msm: DT support for 8960/8064

[Rob Clark]

Now that we (almost) have enough dependencies in place (MMCC, RPM, etc),
add necessary DT support so that we can use drm/msm on upstream kernel.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
I thought I sent this already, but looks like I've forgot.

I've updated the names for the GPIO properties (and, via helper, made
driver check both with and without -gpio suffix).  Added clock-names,
etc.

For now I've removed the compatible string for 8074 hdmi phy.  Due to
integration differences between mdp4 and mdp5 devices, it may end up
being easier to document the bindings separately (even though most of
the code is in common).  When paired with mdp5 display controller, for
example, the HDMI block does not have it's own irq.

 Documentation/devicetree/bindings/drm/msm/gpu.txt  | 52 +++++++++++++++++
 Documentation/devicetree/bindings/drm/msm/hdmi.txt | 46 +++++++++++++++
 Documentation/devicetree/bindings/drm/msm/mdp.txt  | 48 +++++++++++++++
 drivers/gpu/drm/msm/adreno/a3xx_gpu.c              |  2 +
 drivers/gpu/drm/msm/hdmi/hdmi.c                    | 68 ++++++++++++++--------
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c            | 10 +++-
 drivers/gpu/drm/msm/msm_drv.c                      | 29 +++++++--
 7 files changed, 225 insertions(+), 30 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/drm/msm/gpu.txt
 create mode 100644 Documentation/devicetree/bindings/drm/msm/hdmi.txt
 create mode 100644 Documentation/devicetree/bindings/drm/msm/mdp.txt

diff --git a/Documentation/devicetree/bindings/drm/msm/gpu.txt b/Documentation/devicetree/bindings/drm/msm/gpu.txt
new file mode 100644
index 0000000..67d0a58
--- /dev/null
+++ b/Documentation/devicetree/bindings/drm/msm/gpu.txt
@@ -0,0 +1,52 @@
+Qualcomm adreno/snapdragon GPU
+
+Required properties:
+- compatible: "qcom,adreno-3xx"
+- reg: Physical base address and length of the controller's registers.
+- interrupts: The interrupt signal from the gpu.
+- clocks: device clocks
+  See ../clocks/clock-bindings.txt for details.
+- clock-names: the following clocks are required:
+  * "core_clk"
+  * "iface_clk"
+  * "mem_iface_clk"
+- qcom,chipid: gpu chip-id.  Note this may become optional for future
+  devices if we can reliably read the chipid from hw
+- qcom,gpu-pwrlevels: list of operating points
+  - compatible: "qcom,gpu-pwrlevels"
+  - for each qcom,gpu-pwrlevel:
+    - qcom,gpu-freq: requested gpu clock speed
+    - NOTE: downstream android driver defines additional parameters to
+      configure memory bandwidth scaling per OPP.
+
+Example:
+
+/ {
+	...
+
+	gpu: qcom,kgsl-3d0@4300000 {
+		compatible = "qcom,adreno-3xx";
+		reg = <0x04300000 0x20000>;
+		reg-names = "kgsl_3d0_reg_memory";
+		interrupts = <GIC_SPI 80 0>;
+		interrupt-names = "kgsl_3d0_irq";
+		clock-names =
+		    "core_clk",
+		    "iface_clk",
+		    "mem_iface_clk";
+		clocks =
+		    <&mmcc GFX3D_CLK>,
+		    <&mmcc GFX3D_AHB_CLK>,
+		    <&mmcc MMSS_IMEM_AHB_CLK>;
+		qcom,chipid = <0x03020100>;
+		qcom,gpu-pwrlevels {
+			compatible = "qcom,gpu-pwrlevels";
+			qcom,gpu-pwrlevel@0 {
+				qcom,gpu-freq = <450000000>;
+			};
+			qcom,gpu-pwrlevel@1 {
+				qcom,gpu-freq = <27000000>;
+			};
+		};
+	};
+};
diff --git a/Documentation/devicetree/bindings/drm/msm/hdmi.txt b/Documentation/devicetree/bindings/drm/msm/hdmi.txt
new file mode 100644
index 0000000..aca917f
--- /dev/null
+++ b/Documentation/devicetree/bindings/drm/msm/hdmi.txt
@@ -0,0 +1,46 @@
+Qualcomm adreno/snapdragon hdmi output
+
+Required properties:
+- compatible: one of the following
+   * "qcom,hdmi-tx-8660"
+   * "qcom,hdmi-tx-8960"
+- reg: Physical base address and length of the controller's registers
+- reg-names: "core_physical"
+- interrupts: The interrupt signal from the hdmi block.
+- clocks: device clocks
+  See ../clocks/clock-bindings.txt for details.
+- qcom,hdmi-tx-ddc-clk-gpio: ddc clk pin
+- qcom,hdmi-tx-ddc-data-gpio: ddc data pin
+- qcom,hdmi-tx-hpd-gpio: hpd pin
+- core-vdda-supply: phandle to supply regulator
+- hdmi-mux-supply: phandle to mux regulator
+
+Optional properties:
+- qcom,hdmi-tx-mux-en-gpio: hdmi mux enable pin
+- qcom,hdmi-tx-mux-sel-gpio: hdmi mux select pin
+
+Example:
+
+/ {
+	...
+
+	hdmi: qcom,hdmi-tx-8960@4a00000 {
+		compatible = "qcom,hdmi-tx-8960";
+		reg-names = "core_physical";
+		reg = <0x04a00000 0x1000>;
+		interrupts = <GIC_SPI 79 0>;
+		clock-names =
+		    "core_clk",
+		    "master_iface_clk",
+		    "slave_iface_clk";
+		clocks =
+		    <&mmcc HDMI_APP_CLK>,
+		    <&mmcc HDMI_M_AHB_CLK>,
+		    <&mmcc HDMI_S_AHB_CLK>;
+		qcom,hdmi-tx-ddc-clk = <&msmgpio 70 GPIO_ACTIVE_HIGH>;
+		qcom,hdmi-tx-ddc-data = <&msmgpio 71 GPIO_ACTIVE_HIGH>;
+		qcom,hdmi-tx-hpd = <&msmgpio 72 GPIO_ACTIVE_HIGH>;
+		core-vdda-supply = <&pm8921_hdmi_mvs>;
+		hdmi-mux-supply = <&ext_3p3v>;
+	};
+};
diff --git a/Documentation/devicetree/bindings/drm/msm/mdp.txt b/Documentation/devicetree/bindings/drm/msm/mdp.txt
new file mode 100644
index 0000000..1a0598e
--- /dev/null
+++ b/Documentation/devicetree/bindings/drm/msm/mdp.txt
@@ -0,0 +1,48 @@
+Qualcomm adreno/snapdragon display controller
+
+Required properties:
+- compatible:
+  * "qcom,mdp" - mdp4
+- reg: Physical base address and length of the controller's registers.
+- interrupts: The interrupt signal from the display controller.
+- connectors: array of phandles for output device(s)
+- clocks: device clocks
+  See ../clocks/clock-bindings.txt for details.
+- clock-names: the following clocks are required:
+  * "core_clk"
+  * "iface_clk"
+  * "lut_clk"
+  * "src_clk"
+  * "hdmi_clk"
+  * "mpd_clk"
+
+Optional properties:
+- gpus: phandle for gpu device
+
+Example:
+
+/ {
+	...
+
+	mdp: qcom,mdp@5100000 {
+		compatible = "qcom,mdp";
+		reg = <0x05100000 0xf0000>;
+		interrupts = <GIC_SPI 75 0>;
+		connectors = <&hdmi>;
+		gpus = <&gpu>;
+		clock-names =
+		    "core_clk",
+		    "iface_clk",
+		    "lut_clk",
+		    "src_clk",
+		    "hdmi_clk",
+		    "mdp_clk";
+		clocks =
+		    <&mmcc MDP_SRC>,
+		    <&mmcc MDP_AHB_CLK>,
+		    <&mmcc MDP_LUT_CLK>,
+		    <&mmcc TV_SRC>,
+		    <&mmcc HDMI_TV_CLK>,
+		    <&mmcc MDP_TV_CLK>;
+	};
+};
diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
index a2cee06..2773600 100644
--- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
@@ -680,6 +680,8 @@ static int a3xx_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id dt_match[] = {
+	{ .compatible = "qcom,adreno-3xx" },
+	/* for backwards compat w/ downstream kgsl DT files: */
 	{ .compatible = "qcom,kgsl-3d0" },
 	{}
 };
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 7f7aade..041c2fc 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -123,7 +123,8 @@ struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder)
 	for (i = 0; i < config->hpd_reg_cnt; i++) {
 		struct regulator *reg;
 
-		reg = devm_regulator_get(&pdev->dev, config->hpd_reg_names[i]);
+		reg = devm_regulator_get_exclusive(&pdev->dev,
+				config->hpd_reg_names[i]);
 		if (IS_ERR(reg)) {
 			ret = PTR_ERR(reg);
 			dev_err(dev->dev, "failed to get hpd regulator: %s (%d)\n",
@@ -138,7 +139,8 @@ struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder)
 	for (i = 0; i < config->pwr_reg_cnt; i++) {
 		struct regulator *reg;
 
-		reg = devm_regulator_get(&pdev->dev, config->pwr_reg_names[i]);
+		reg = devm_regulator_get_exclusive(&pdev->dev,
+				config->pwr_reg_names[i]);
 		if (IS_ERR(reg)) {
 			ret = PTR_ERR(reg);
 			dev_err(dev->dev, "failed to get pwr regulator: %s (%d)\n",
@@ -266,37 +268,55 @@ static int hdmi_bind(struct device *dev, struct device *master, void *data)
 	{
 		int gpio = of_get_named_gpio(of_node, name, 0);
 		if (gpio < 0) {
-			dev_err(dev, "failed to get gpio: %s (%d)\n",
-					name, gpio);
-			gpio = -1;
+			char name2[32];
+			snprintf(name2, sizeof(name2), "%s-gpio", name);
+			gpio = of_get_named_gpio(of_node, name2, 0);
+			if (gpio < 0) {
+				dev_err(dev, "failed to get gpio: %s (%d)\n",
+						name, gpio);
+				gpio = -1;
+			}
 		}
 		return gpio;
 	}
 
-	/* TODO actually use DT.. */
-	static const char *hpd_reg_names[] = {"hpd-gdsc", "hpd-5v"};
-	static const char *pwr_reg_names[] = {"core-vdda", "core-vcc"};
-	static const char *hpd_clk_names[] = {"iface_clk", "core_clk", "mdp_core_clk"};
-	static unsigned long hpd_clk_freq[] = {0, 19200000, 0};
-	static const char *pwr_clk_names[] = {"extp_clk", "alt_iface_clk"};
+	if (of_device_is_compatible(of_node, "qcom,hdmi-tx-8074")) {
+		static const char *hpd_reg_names[] = {"hpd-gdsc", "hpd-5v"};
+		static const char *pwr_reg_names[] = {"core-vdda", "core-vcc"};
+		static const char *hpd_clk_names[] = {"iface_clk", "core_clk", "mdp_core_clk"};
+		static unsigned long hpd_clk_freq[] = {0, 19200000, 0};
+		static const char *pwr_clk_names[] = {"extp_clk", "alt_iface_clk"};
+		config.phy_init      = hdmi_phy_8x74_init;
+		config.hpd_reg_names = hpd_reg_names;
+		config.hpd_reg_cnt   = ARRAY_SIZE(hpd_reg_names);
+		config.pwr_reg_names = pwr_reg_names;
+		config.pwr_reg_cnt   = ARRAY_SIZE(pwr_reg_names);
+		config.hpd_clk_names = hpd_clk_names;
+		config.hpd_freq      = hpd_clk_freq;
+		config.hpd_clk_cnt   = ARRAY_SIZE(hpd_clk_names);
+		config.pwr_clk_names = pwr_clk_names;
+		config.pwr_clk_cnt   = ARRAY_SIZE(pwr_clk_names);
+		config.shared_irq    = true;
+	} else if (of_device_is_compatible(of_node, "qcom,hdmi-tx-8960")) {
+		static const char *hpd_clk_names[] = {"core_clk", "master_iface_clk", "slave_iface_clk"};
+		static const char *hpd_reg_names[] = {"core-vdda", "hdmi-mux"};
+		config.phy_init      = hdmi_phy_8960_init;
+		config.hpd_reg_names = hpd_reg_names;
+		config.hpd_reg_cnt   = ARRAY_SIZE(hpd_reg_names);
+		config.hpd_clk_names = hpd_clk_names;
+		config.hpd_clk_cnt   = ARRAY_SIZE(hpd_clk_names);
+	} else if (of_device_is_compatible(of_node, "qcom,hdmi-tx-8660")) {
+		config.phy_init      = hdmi_phy_8x60_init;
+	} else {
+		dev_err(dev, "unknown phy: %s\n", of_node->name);
+	}
 
-	config.phy_init      = hdmi_phy_8x74_init;
 	config.mmio_name     = "core_physical";
-	config.hpd_reg_names = hpd_reg_names;
-	config.hpd_reg_cnt   = ARRAY_SIZE(hpd_reg_names);
-	config.pwr_reg_names = pwr_reg_names;
-	config.pwr_reg_cnt   = ARRAY_SIZE(pwr_reg_names);
-	config.hpd_clk_names = hpd_clk_names;
-	config.hpd_freq      = hpd_clk_freq;
-	config.hpd_clk_cnt   = ARRAY_SIZE(hpd_clk_names);
-	config.pwr_clk_names = pwr_clk_names;
-	config.pwr_clk_cnt   = ARRAY_SIZE(pwr_clk_names);
 	config.ddc_clk_gpio  = get_gpio("qcom,hdmi-tx-ddc-clk");
 	config.ddc_data_gpio = get_gpio("qcom,hdmi-tx-ddc-data");
 	config.hpd_gpio      = get_gpio("qcom,hdmi-tx-hpd");
 	config.mux_en_gpio   = get_gpio("qcom,hdmi-tx-mux-en");
 	config.mux_sel_gpio  = get_gpio("qcom,hdmi-tx-mux-sel");
-	config.shared_irq    = true;
 
 #else
 	static const char *hpd_clk_names[] = {
@@ -373,7 +393,9 @@ static int hdmi_dev_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id dt_match[] = {
-	{ .compatible = "qcom,hdmi-tx" },
+	{ .compatible = "qcom,hdmi-tx-8074" },
+	{ .compatible = "qcom,hdmi-tx-8960" },
+	{ .compatible = "qcom,hdmi-tx-8660" },
 	{}
 };
 
diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
index 0bb4faa..5a7bfd4 100644
--- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
@@ -294,15 +294,17 @@ struct msm_kms *mdp4_kms_init(struct drm_device *dev)
 		goto fail;
 	}
 
-	mdp4_kms->dsi_pll_vdda = devm_regulator_get(&pdev->dev, "dsi_pll_vdda");
+	mdp4_kms->dsi_pll_vdda =
+			devm_regulator_get_optional(&pdev->dev, "dsi_pll_vdda");
 	if (IS_ERR(mdp4_kms->dsi_pll_vdda))
 		mdp4_kms->dsi_pll_vdda = NULL;
 
-	mdp4_kms->dsi_pll_vddio = devm_regulator_get(&pdev->dev, "dsi_pll_vddio");
+	mdp4_kms->dsi_pll_vddio =
+			devm_regulator_get_optional(&pdev->dev, "dsi_pll_vddio");
 	if (IS_ERR(mdp4_kms->dsi_pll_vddio))
 		mdp4_kms->dsi_pll_vddio = NULL;
 
-	mdp4_kms->vdd = devm_regulator_get(&pdev->dev, "vdd");
+	mdp4_kms->vdd = devm_regulator_get_exclusive(&pdev->dev, "vdd");
 	if (IS_ERR(mdp4_kms->vdd))
 		mdp4_kms->vdd = NULL;
 
@@ -406,6 +408,8 @@ static struct mdp4_platform_config *mdp4_get_config(struct platform_device *dev)
 	static struct mdp4_platform_config config = {};
 #ifdef CONFIG_OF
 	/* TODO */
+	config.max_clk = 266667000;
+	config.iommu = iommu_domain_alloc(&platform_bus_type);
 #else
 	if (cpu_is_apq8064())
 		config.max_clk = 266667000;
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 9a5d87d..1ad0155 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -906,7 +906,8 @@ static int compare_of(struct device *dev, void *data)
 	return dev->of_node == data;
 }
 
-static int msm_drm_add_components(struct device *master, struct master *m)
+static int add_components(struct device *master, struct master *m,
+		const char *name)
 {
 	struct device_node *np = master->of_node;
 	unsigned i;
@@ -915,16 +916,35 @@ static int msm_drm_add_components(struct device *master, struct master *m)
 	for (i = 0; ; i++) {
 		struct device_node *node;
 
-		node = of_parse_phandle(np, "connectors", i);
+		node = of_parse_phandle(np, name, i);
 		if (!node)
 			break;
 
 		ret = component_master_add_child(m, compare_of, node);
 		of_node_put(node);
 
-		if (ret)
+		if (ret) {
+			dev_err(master, "could not add %s child %s: %d\n",
+					name, node->name, ret);
 			return ret;
+		}
 	}
+
+	return 0;
+}
+
+static int msm_drm_add_components(struct device *master, struct master *m)
+{
+	int ret;
+
+	ret = add_components(master, m, "connectors");
+	if (ret)
+		return ret;
+
+	ret = add_components(master, m, "gpus");
+	if (ret)
+		return ret;
+
 	return 0;
 }
 #else
@@ -1008,7 +1028,8 @@ static const struct platform_device_id msm_id[] = {
 };
 
 static const struct of_device_id dt_match[] = {
-	{ .compatible = "qcom,mdss_mdp" },
+	{ .compatible = "qcom,mdp" },      /* mdp4 */
+	{ .compatible = "qcom,mdss_mdp" }, /* mdp5 */
 	{}
 };
 MODULE_DEVICE_TABLE(of, dt_match);
-- 
1.9.3

Re: [RFCv2] drm/msm: DT support for 8960/8064

[divya ojha]

Hi Rob,

On Tue, Jul 8, 2014 at 9:30 PM, Rob Clark <robdclark@gmail.com> wrote:
> Now that we (almost) have enough dependencies in place (MMCC, RPM, etc),
> add necessary DT support so that we can use drm/msm on upstream kernel.
>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
> I thought I sent this already, but looks like I've forgot.
..
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index 7f7aade..041c2fc 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -123,7 +123,8 @@ struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder)
>         for (i = 0; i < config->hpd_reg_cnt; i++) {
>                 struct regulator *reg;
>
> -               reg = devm_regulator_get(&pdev->dev, config->hpd_reg_names[i]);
> +               reg = devm_regulator_get_exclusive(&pdev->dev,
> +                               config->hpd_reg_names[i]);
>                 if (IS_ERR(reg)) {
>                         ret = PTR_ERR(reg);
>                         dev_err(dev->dev, "failed to get hpd regulator: %s (%d)\n",
> @@ -138,7 +139,8 @@ struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder)
>         for (i = 0; i < config->pwr_reg_cnt; i++) {
>                 struct regulator *reg;
>
> -               reg = devm_regulator_get(&pdev->dev, config->pwr_reg_names[i]);
> +               reg = devm_regulator_get_exclusive(&pdev->dev,
> +                               config->pwr_reg_names[i]);
>                 if (IS_ERR(reg)) {
>                         ret = PTR_ERR(reg);
>                         dev_err(dev->dev, "failed to get pwr regulator: %s (%d)\n",

Don't we need to have a if(regulator_enabled) check after
devm_regulator_get function ?
I see a similar test after camera regulator_get function call.

-Regards
Divya

Re: [RFCv2] drm/msm: DT support for 8960/8064

[Rob Clark]

On Thu, Jul 17, 2014 at 4:10 AM, divya ojha <odivya77@gmail.com> wrote:
> Hi Rob,
>
> On Tue, Jul 8, 2014 at 9:30 PM, Rob Clark <robdclark@gmail.com> wrote:
>> Now that we (almost) have enough dependencies in place (MMCC, RPM, etc),
>> add necessary DT support so that we can use drm/msm on upstream kernel.
>>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> ---
>> I thought I sent this already, but looks like I've forgot.
> ..
>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
>> index 7f7aade..041c2fc 100644
>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
>> @@ -123,7 +123,8 @@ struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder)
>>         for (i = 0; i < config->hpd_reg_cnt; i++) {
>>                 struct regulator *reg;
>>
>> -               reg = devm_regulator_get(&pdev->dev, config->hpd_reg_names[i]);
>> +               reg = devm_regulator_get_exclusive(&pdev->dev,
>> +                               config->hpd_reg_names[i]);
>>                 if (IS_ERR(reg)) {
>>                         ret = PTR_ERR(reg);
>>                         dev_err(dev->dev, "failed to get hpd regulator: %s (%d)\n",
>> @@ -138,7 +139,8 @@ struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder)
>>         for (i = 0; i < config->pwr_reg_cnt; i++) {
>>                 struct regulator *reg;
>>
>> -               reg = devm_regulator_get(&pdev->dev, config->pwr_reg_names[i]);
>> +               reg = devm_regulator_get_exclusive(&pdev->dev,
>> +                               config->pwr_reg_names[i]);
>>                 if (IS_ERR(reg)) {
>>                         ret = PTR_ERR(reg);
>>                         dev_err(dev->dev, "failed to get pwr regulator: %s (%d)\n",
>
> Don't we need to have a if(regulator_enabled) check after
> devm_regulator_get function ?
> I see a similar test after camera regulator_get function call.

tbh, I'm not 100% sure.

Normally I would say that any driver using some resource (regulator,
etc) should take it's own reference irrespective of whether some other
driver already has the regulator/etc, enabled.  Otherwise the
reference counting doesn't work out.

The thing I'm not 100% sure about is interaction w/ bootloader, in
cases where bootloader does initial modeset.  I've had some problems
in this area in the past when I was trying to get DSI working on my
nexus4.  I doubt the correct solution is for the driver to check if
regulator is already enabled by bootloader before enabling it, but I'm
not sure if something else somewhere needs to drop references to
resources enabled by bootloader.

BR,
-R

> -Regards
> Divya

Re: [RFCv2] drm/msm: DT support for 8960/8064

[Bjorn Andersson]

On Thu, Jul 17, 2014 at 8:29 AM, Rob Clark <robdclark@gmail.com> wrote:
> On Thu, Jul 17, 2014 at 4:10 AM, divya ojha <odivya77@gmail.com> wrote:
[...]
>> Don't we need to have a if(regulator_enabled) check after
>> devm_regulator_get function ?
>> I see a similar test after camera regulator_get function call.
>
> tbh, I'm not 100% sure.
>

Regulators in the Qualcomm platform works by casting votes with the
RPM and there is no way to query the current state of a regulator, so
the regulator code will only return information regarding the Linux
systems votes.

So, we can unfortunately not check this.

This setup also makes it important that if we want a regulator on and
set to a certain voltage then we need to cast our vote with these
values, or some other subsystem might lower it by releasing their
votes.

Regards,
Bjorn