[PATCH v2 0/2] Add initial CoreSight support for the Qualcomm 8x16 chipsets

[PATCH v2 0/2] Add initial CoreSight support for the Qualcomm 8x16 chipsets

[Ivan T. Ivanov]

Add initial CoreSight support for the Qualcomm 8x16 chipsets

This patch series add initial set of CoreSight components for the
8x16 chipsets.

Components will not be functional, because of missing clock controller
driver, which is under internal testing.

Patches are based on "Enable CoreSight for the Ux500 "[1] and
"Support for coresight ETMv4 tracer" [2]

Changes since first version [3]:

* Added comments about not described funnels ports in DT files.
* Removed MODULE_ macros.
* Print replicator version at probe time.
* Fixed Kconfig driver description.
* Added "1x" suffix to compatible string, to reflect supported replicator version
* Add comment how replicator output port is disabled.

[1] http://www.spinics.net/lists/arm-kernel/msg412873.html
[2] https://lwn.net/Articles/641585/
[3] https://lkml.org/lkml/2015/4/29/241

Ivan T. Ivanov (1):
  arm64: dts: qcom: Add msm8916 CoreSight components

Pratik Patel (1):
  coresight: replicator: Add Qualcomm CoreSight Replicator driver

 .../devicetree/bindings/arm/coresight.txt          |   1 +
 arch/arm64/boot/dts/qcom/msm8916-coresight.dtsi    | 254 +++++++++++++++++++++
 drivers/hwtracing/coresight/Kconfig                |   8 +
 drivers/hwtracing/coresight/Makefile               |   1 +
 .../coresight/coresight-replicator-qcom.c          | 214 +++++++++++++++++
 5 files changed, 478 insertions(+)
 create mode 100644 arch/arm64/boot/dts/qcom/msm8916-coresight.dtsi
 create mode 100644 drivers/hwtracing/coresight/coresight-replicator-qcom.c

--
1.9.1

[PATCH v2 1/2] coresight: replicator: Add Qualcomm CoreSight Replicator driver

[Ivan T. Ivanov]

From: Pratik Patel <pratikp@codeaurora.org>

This driver manages Qualcomm CoreSight Replicator device, which
resides on the AMBA bus. Replicator has been made programmable to
allow software to turn of the replicator branch to sink that is not
being used. This avoids trace traffic to the unused/non-current sink
from causing back pressure that results in overflows at the source.

Signed-off-by: Pratik Patel <pratikp@codeaurora.org>
Signed-off-by: Ivan T. Ivanov <ivan.ivanov@linaro.org>
---
 .../devicetree/bindings/arm/coresight.txt          |   1 +
 drivers/hwtracing/coresight/Kconfig                |   8 +
 drivers/hwtracing/coresight/Makefile               |   1 +
 .../coresight/coresight-replicator-qcom.c          | 214 +++++++++++++++++++++
 4 files changed, 224 insertions(+)
 create mode 100644 drivers/hwtracing/coresight/coresight-replicator-qcom.c

diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
index f4d6a86..661a1be 100644
--- a/Documentation/devicetree/bindings/arm/coresight.txt
+++ b/Documentation/devicetree/bindings/arm/coresight.txt
@@ -18,6 +18,7 @@ its hardware characteristcs.
 		- "arm,coresight-funnel", "arm,primecell";
 		- "arm,coresight-etm3x", "arm,primecell";
 		- "arm,coresight-etm4x", "arm,primecell";
+		- "qcom,coresight-replicator1x", "arm,primecell";

 	* reg: physical base address and length of the register
 	  set(s) of the component.
diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index 6b331d4..a57a51d 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -68,4 +68,12 @@ config CORESIGHT_SOURCE_ETM4X
 	  instructions that a processor is executing. This is primarily useful
 	  for instruction level tracing. Depending on the implemented version
 	  data tracing may also be available.
+
+config CORESIGHT_QCOM_REPLICATOR
+	bool "Qualcomm CoreSight Replicator driver"
+	help
+	  This enables support for Qualcomm CoreSight link driver. The programmable
+	  ATB replicator sends the ATB trace stream from the ETB/ETF to the TPIU
+	  and ETR.
+
 endif
diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
index 0af28d4..99f8e5f 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-funnel.o \
 					   coresight-replicator.o
 obj-$(CONFIG_CORESIGHT_SOURCE_ETM3X) += coresight-etm3x.o coresight-etm-cp14.o
 obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o
+obj-$(CONFIG_CORESIGHT_QCOM_REPLICATOR) += coresight-replicator-qcom.o
diff --git a/drivers/hwtracing/coresight/coresight-replicator-qcom.c b/drivers/hwtracing/coresight/coresight-replicator-qcom.c
new file mode 100644
index 0000000..deacea4
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-replicator-qcom.c
@@ -0,0 +1,214 @@
+/*
+ * Copyright (c) 2011-2015, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/amba/bus.h>
+#include <linux/clk.h>
+#include <linux/coresight.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+
+#include "coresight-priv.h"
+
+#define REPLICATOR_IDFILTER0		0x000
+#define REPLICATOR_IDFILTER1		0x004
+
+/**
+ * struct replicator_state - specifics associated to a replicator component
+ * @base:	memory mapped base address for this component.
+ * @dev:	the device entity associated with this component
+ * @atclk:	optional clock for the core parts of the replicator.
+ * @csdev:	component vitals needed by the framework
+ */
+struct replicator_state {
+	void __iomem		*base;
+	struct device		*dev;
+	struct clk		*atclk;
+	struct coresight_device	*csdev;
+};
+
+static int replicator_enable(struct coresight_device *csdev, int inport,
+			      int outport)
+{
+	struct replicator_state *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	pm_runtime_get_sync(drvdata->dev);
+
+	CS_UNLOCK(drvdata->base);
+
+	/*
+	 * Ensure that the other port is disabled
+	 * 0x00 - passing through the replicator unimpeded
+	 * 0xff - disable (or impede) the flow of ATB data
+	 */
+	if (outport == 0) {
+		writel_relaxed(0x00, drvdata->base + REPLICATOR_IDFILTER0);
+		writel_relaxed(0xff, drvdata->base + REPLICATOR_IDFILTER1);
+	} else {
+		writel_relaxed(0x00, drvdata->base + REPLICATOR_IDFILTER1);
+		writel_relaxed(0xff, drvdata->base + REPLICATOR_IDFILTER0);
+	}
+
+	CS_LOCK(drvdata->base);
+
+	dev_info(drvdata->dev, "REPLICATOR enabled\n");
+	return 0;
+}
+
+static void replicator_disable(struct coresight_device *csdev, int inport,
+				int outport)
+{
+	struct replicator_state *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	CS_UNLOCK(drvdata->base);
+
+	/* disable the flow of ATB data through port */
+	if (outport == 0)
+		writel_relaxed(0xff, drvdata->base + REPLICATOR_IDFILTER0);
+	else
+		writel_relaxed(0xff, drvdata->base + REPLICATOR_IDFILTER1);
+
+	CS_LOCK(drvdata->base);
+
+	pm_runtime_put(drvdata->dev);
+
+	dev_info(drvdata->dev, "REPLICATOR disabled\n");
+}
+
+static const struct coresight_ops_link replicator_link_ops = {
+	.enable		= replicator_enable,
+	.disable	= replicator_disable,
+};
+
+static const struct coresight_ops replicator_cs_ops = {
+	.link_ops	= &replicator_link_ops,
+};
+
+static int replicator_probe(struct amba_device *adev, const struct amba_id *id)
+{
+	int ret;
+	struct device *dev = &adev->dev;
+	struct resource *res = &adev->res;
+	struct coresight_platform_data *pdata = NULL;
+	struct replicator_state *drvdata;
+	struct coresight_desc *desc;
+	struct device_node *np = adev->dev.of_node;
+	void __iomem *base;
+
+	if (np) {
+		pdata = of_get_coresight_platform_data(dev, np);
+		if (IS_ERR(pdata))
+			return PTR_ERR(pdata);
+		adev->dev.platform_data = pdata;
+	}
+
+	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+
+	drvdata->dev = &adev->dev;
+	drvdata->atclk = devm_clk_get(&adev->dev, "atclk"); /* optional */
+	if (!IS_ERR(drvdata->atclk)) {
+		ret = clk_prepare_enable(drvdata->atclk);
+		if (ret)
+			return ret;
+	}
+
+	/* Validity for the resource is already checked by the AMBA core */
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	drvdata->base = base;
+	dev_set_drvdata(dev, drvdata);
+	pm_runtime_put(&adev->dev);
+
+	desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
+	if (!desc)
+		return -ENOMEM;
+
+	desc->type = CORESIGHT_DEV_TYPE_LINK;
+	desc->subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_SPLIT;
+	desc->ops = &replicator_cs_ops;
+	desc->pdata = adev->dev.platform_data;
+	desc->dev = &adev->dev;
+	drvdata->csdev = coresight_register(desc);
+	if (IS_ERR(drvdata->csdev))
+		return PTR_ERR(drvdata->csdev);
+
+	dev_info(dev, "%s initialized\n", (char *)id->data);
+	return 0;
+}
+
+static int replicator_remove(struct amba_device *adev)
+{
+	struct replicator_state *drvdata = amba_get_drvdata(adev);
+
+	pm_runtime_disable(&adev->dev);
+	coresight_unregister(drvdata->csdev);
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int replicator_runtime_suspend(struct device *dev)
+{
+	struct replicator_state *drvdata = dev_get_drvdata(dev);
+
+	if (drvdata && !IS_ERR(drvdata->atclk))
+		clk_disable_unprepare(drvdata->atclk);
+
+	return 0;
+}
+
+static int replicator_runtime_resume(struct device *dev)
+{
+	struct replicator_state *drvdata = dev_get_drvdata(dev);
+
+	if (drvdata && !IS_ERR(drvdata->atclk))
+		clk_prepare_enable(drvdata->atclk);
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops replicator_dev_pm_ops = {
+	SET_RUNTIME_PM_OPS(replicator_runtime_suspend,
+			   replicator_runtime_resume,
+			   NULL)
+};
+
+static struct amba_id replicator_ids[] = {
+	{
+		.id     = 0x0003b909,
+		.mask   = 0x0003ffff,
+		.data	= "REPLICATOR 1.0",
+	},
+	{ 0, 0 },
+};
+
+static struct amba_driver replicator_driver = {
+	.drv = {
+		.name	= "coresight-replicator-qcom",
+		.pm	= &replicator_dev_pm_ops,
+	},
+	.probe		= replicator_probe,
+	.remove		= replicator_remove,
+	.id_table	= replicator_ids,
+};
+
+module_amba_driver(replicator_driver);
--
1.9.1

Re: [PATCH v2 1/2] coresight: replicator: Add Qualcomm CoreSight Replicator driver

[Mathieu Poirier]

On 7 May 2015 at 09:36, Ivan T. Ivanov <ivan.ivanov@linaro.org> wrote:
> From: Pratik Patel <pratikp@codeaurora.org>
>
> This driver manages Qualcomm CoreSight Replicator device, which
> resides on the AMBA bus. Replicator has been made programmable to
> allow software to turn of the replicator branch to sink that is not
> being used. This avoids trace traffic to the unused/non-current sink
> from causing back pressure that results in overflows at the source.
>
> Signed-off-by: Pratik Patel <pratikp@codeaurora.org>
> Signed-off-by: Ivan T. Ivanov <ivan.ivanov@linaro.org>
> ---
>  .../devicetree/bindings/arm/coresight.txt          |   1 +
>  drivers/hwtracing/coresight/Kconfig                |   8 +
>  drivers/hwtracing/coresight/Makefile               |   1 +
>  .../coresight/coresight-replicator-qcom.c          | 214 +++++++++++++++++++++
>  4 files changed, 224 insertions(+)
>  create mode 100644 drivers/hwtracing/coresight/coresight-replicator-qcom.c
>
> diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
> index f4d6a86..661a1be 100644
> --- a/Documentation/devicetree/bindings/arm/coresight.txt
> +++ b/Documentation/devicetree/bindings/arm/coresight.txt
> @@ -18,6 +18,7 @@ its hardware characteristcs.
>                 - "arm,coresight-funnel", "arm,primecell";
>                 - "arm,coresight-etm3x", "arm,primecell";
>                 - "arm,coresight-etm4x", "arm,primecell";
> +               - "qcom,coresight-replicator1x", "arm,primecell";
>
>         * reg: physical base address and length of the register
>           set(s) of the component.
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index 6b331d4..a57a51d 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -68,4 +68,12 @@ config CORESIGHT_SOURCE_ETM4X
>           instructions that a processor is executing. This is primarily useful
>           for instruction level tracing. Depending on the implemented version
>           data tracing may also be available.
> +
> +config CORESIGHT_QCOM_REPLICATOR
> +       bool "Qualcomm CoreSight Replicator driver"
> +       help
> +         This enables support for Qualcomm CoreSight link driver. The programmable
> +         ATB replicator sends the ATB trace stream from the ETB/ETF to the TPIU
> +         and ETR.
> +
>  endif
> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> index 0af28d4..99f8e5f 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-funnel.o \
>                                            coresight-replicator.o
>  obj-$(CONFIG_CORESIGHT_SOURCE_ETM3X) += coresight-etm3x.o coresight-etm-cp14.o
>  obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o
> +obj-$(CONFIG_CORESIGHT_QCOM_REPLICATOR) += coresight-replicator-qcom.o
> diff --git a/drivers/hwtracing/coresight/coresight-replicator-qcom.c b/drivers/hwtracing/coresight/coresight-replicator-qcom.c
> new file mode 100644
> index 0000000..deacea4
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-replicator-qcom.c
> @@ -0,0 +1,214 @@
> +/*
> + * Copyright (c) 2011-2015, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/amba/bus.h>
> +#include <linux/clk.h>
> +#include <linux/coresight.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +
> +#include "coresight-priv.h"
> +
> +#define REPLICATOR_IDFILTER0           0x000
> +#define REPLICATOR_IDFILTER1           0x004
> +
> +/**
> + * struct replicator_state - specifics associated to a replicator component
> + * @base:      memory mapped base address for this component.
> + * @dev:       the device entity associated with this component
> + * @atclk:     optional clock for the core parts of the replicator.
> + * @csdev:     component vitals needed by the framework
> + */
> +struct replicator_state {
> +       void __iomem            *base;
> +       struct device           *dev;
> +       struct clk              *atclk;
> +       struct coresight_device *csdev;
> +};
> +
> +static int replicator_enable(struct coresight_device *csdev, int inport,
> +                             int outport)
> +{
> +       struct replicator_state *drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> +       pm_runtime_get_sync(drvdata->dev);
> +
> +       CS_UNLOCK(drvdata->base);
> +
> +       /*
> +        * Ensure that the other port is disabled
> +        * 0x00 - passing through the replicator unimpeded
> +        * 0xff - disable (or impede) the flow of ATB data
> +        */
> +       if (outport == 0) {
> +               writel_relaxed(0x00, drvdata->base + REPLICATOR_IDFILTER0);
> +               writel_relaxed(0xff, drvdata->base + REPLICATOR_IDFILTER1);
> +       } else {
> +               writel_relaxed(0x00, drvdata->base + REPLICATOR_IDFILTER1);
> +               writel_relaxed(0xff, drvdata->base + REPLICATOR_IDFILTER0);
> +       }
> +
> +       CS_LOCK(drvdata->base);
> +
> +       dev_info(drvdata->dev, "REPLICATOR enabled\n");
> +       return 0;
> +}
> +
> +static void replicator_disable(struct coresight_device *csdev, int inport,
> +                               int outport)
> +{
> +       struct replicator_state *drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> +       CS_UNLOCK(drvdata->base);
> +
> +       /* disable the flow of ATB data through port */
> +       if (outport == 0)
> +               writel_relaxed(0xff, drvdata->base + REPLICATOR_IDFILTER0);
> +       else
> +               writel_relaxed(0xff, drvdata->base + REPLICATOR_IDFILTER1);
> +
> +       CS_LOCK(drvdata->base);
> +
> +       pm_runtime_put(drvdata->dev);
> +
> +       dev_info(drvdata->dev, "REPLICATOR disabled\n");
> +}
> +
> +static const struct coresight_ops_link replicator_link_ops = {
> +       .enable         = replicator_enable,
> +       .disable        = replicator_disable,
> +};
> +
> +static const struct coresight_ops replicator_cs_ops = {
> +       .link_ops       = &replicator_link_ops,
> +};
> +
> +static int replicator_probe(struct amba_device *adev, const struct amba_id *id)
> +{
> +       int ret;
> +       struct device *dev = &adev->dev;
> +       struct resource *res = &adev->res;
> +       struct coresight_platform_data *pdata = NULL;
> +       struct replicator_state *drvdata;
> +       struct coresight_desc *desc;
> +       struct device_node *np = adev->dev.of_node;
> +       void __iomem *base;
> +
> +       if (np) {
> +               pdata = of_get_coresight_platform_data(dev, np);
> +               if (IS_ERR(pdata))
> +                       return PTR_ERR(pdata);
> +               adev->dev.platform_data = pdata;
> +       }
> +
> +       drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> +       if (!drvdata)
> +               return -ENOMEM;
> +
> +       drvdata->dev = &adev->dev;
> +       drvdata->atclk = devm_clk_get(&adev->dev, "atclk"); /* optional */
> +       if (!IS_ERR(drvdata->atclk)) {
> +               ret = clk_prepare_enable(drvdata->atclk);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       /* Validity for the resource is already checked by the AMBA core */
> +       base = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(base))
> +               return PTR_ERR(base);
> +
> +       drvdata->base = base;
> +       dev_set_drvdata(dev, drvdata);
> +       pm_runtime_put(&adev->dev);
> +
> +       desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
> +       if (!desc)
> +               return -ENOMEM;
> +
> +       desc->type = CORESIGHT_DEV_TYPE_LINK;
> +       desc->subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_SPLIT;
> +       desc->ops = &replicator_cs_ops;
> +       desc->pdata = adev->dev.platform_data;
> +       desc->dev = &adev->dev;
> +       drvdata->csdev = coresight_register(desc);
> +       if (IS_ERR(drvdata->csdev))
> +               return PTR_ERR(drvdata->csdev);
> +
> +       dev_info(dev, "%s initialized\n", (char *)id->data);
> +       return 0;
> +}
> +
> +static int replicator_remove(struct amba_device *adev)
> +{
> +       struct replicator_state *drvdata = amba_get_drvdata(adev);
> +
> +       pm_runtime_disable(&adev->dev);
> +       coresight_unregister(drvdata->csdev);
> +       return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int replicator_runtime_suspend(struct device *dev)
> +{
> +       struct replicator_state *drvdata = dev_get_drvdata(dev);
> +
> +       if (drvdata && !IS_ERR(drvdata->atclk))
> +               clk_disable_unprepare(drvdata->atclk);
> +
> +       return 0;
> +}
> +
> +static int replicator_runtime_resume(struct device *dev)
> +{
> +       struct replicator_state *drvdata = dev_get_drvdata(dev);
> +
> +       if (drvdata && !IS_ERR(drvdata->atclk))
> +               clk_prepare_enable(drvdata->atclk);
> +
> +       return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops replicator_dev_pm_ops = {
> +       SET_RUNTIME_PM_OPS(replicator_runtime_suspend,
> +                          replicator_runtime_resume,
> +                          NULL)
> +};
> +
> +static struct amba_id replicator_ids[] = {
> +       {
> +               .id     = 0x0003b909,
> +               .mask   = 0x0003ffff,
> +               .data   = "REPLICATOR 1.0",
> +       },
> +       { 0, 0 },
> +};
> +
> +static struct amba_driver replicator_driver = {
> +       .drv = {
> +               .name   = "coresight-replicator-qcom",
> +               .pm     = &replicator_dev_pm_ops,
> +       },
> +       .probe          = replicator_probe,
> +       .remove         = replicator_remove,
> +       .id_table       = replicator_ids,
> +};
> +
> +module_amba_driver(replicator_driver);
> --
> 1.9.1
>

Applied - thanks.

Re: [PATCH v2 1/2] coresight: replicator: Add Qualcomm CoreSight Replicator driver

[Ivan T. Ivanov]

On Mon, 2015-05-11 at 13:07 -0600, Mathieu Poirier wrote:
> On 7 May 2015 at 09:36, Ivan T. Ivanov ivanov@linaro.org> wrote:
> > 
> > +
> > +module_amba_driver(replicator_driver);
> > --
> > 1.9.1
> > 
> 
> Applied - thanks.


Thank you. Which tree did you use to collect these patches?

Regards,
Ivan

[PATCH v2 2/2] arm64: dts: qcom: Add msm8916 CoreSight components

[Ivan T. Ivanov]

Add initial set of CoreSight components found on Qualcomm's 8x16 chipset.

Signed-off-by: Ivan T. Ivanov <ivan.ivanov@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8916-coresight.dtsi | 254 ++++++++++++++++++++++++
 1 file changed, 254 insertions(+)
 create mode 100644 arch/arm64/boot/dts/qcom/msm8916-coresight.dtsi

diff --git a/arch/arm64/boot/dts/qcom/msm8916-coresight.dtsi b/arch/arm64/boot/dts/qcom/msm8916-coresight.dtsi
new file mode 100644
index 0000000..33ae981
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/msm8916-coresight.dtsi
@@ -0,0 +1,254 @@
+/*
+ * Copyright (c) 2013 - 2015, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+&soc {
+
+	tpiu@820000 {
+		compatible = "arm,coresight-tpiu", "arm,primecell";
+		reg = <0x820000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
+		clock-names = "apb_pclk", "atclk";
+
+		port {
+			tpiu_in: endpoint {
+				slave-mode;
+				remote-endpoint = <&replicator_out1>;
+			};
+		};
+	};
+
+	funnel@821000 {
+		compatible = "arm,coresight-funnel", "arm,primecell";
+		reg = <0x821000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
+		clock-names = "apb_pclk", "atclk";
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			/*
+			 * Not described input ports:
+			 * 0 - connected to Resource and Power Manger CPU ETM
+			 * 1 - not-connected
+			 * 2 - connected to Modem CPU ETM
+			 * 3 - not-connected
+			 * 5 - not-connected
+			 * 6 - connected trought funnel to Wireless CPU ETM
+			 * 7 - connected to STM component
+			 */
+			port@4 {
+				reg = <4>;
+				funnel0_in4: endpoint {
+					slave-mode;
+					remote-endpoint = <&funnel1_out>;
+				};
+			};
+			port@8 {
+				reg = <0>;
+				funnel0_out: endpoint {
+					remote-endpoint = <&etf_in>;
+				};
+			};
+		};
+	};
+
+	replicator@824000 {
+		compatible = "qcom,coresight-replicator", "arm,primecell";
+		reg = <0x824000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
+		clock-names = "apb_pclk", "atclk";
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				replicator_out0: endpoint {
+					remote-endpoint = <&etr_in>;
+				};
+			};
+			port@1 {
+				reg = <1>;
+				replicator_out1: endpoint {
+					remote-endpoint = <&tpiu_in>;
+				};
+			};
+			port@2 {
+				reg = <0>;
+				replicator_in: endpoint {
+					slave-mode;
+					remote-endpoint = <&etf_out>;
+				};
+			};
+		};
+	};
+
+	etf@825000 {
+		compatible = "arm,coresight-tmc", "arm,primecell";
+		reg = <0x825000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
+		clock-names = "apb_pclk", "atclk";
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				etf_out: endpoint {
+					slave-mode;
+					remote-endpoint = <&funnel0_out>;
+				};
+			};
+			port@1 {
+				reg = <0>;
+				etf_in: endpoint {
+					remote-endpoint = <&replicator_in>;
+				};
+			};
+		};
+	};
+
+	etr@826000 {
+		compatible = "arm,coresight-tmc", "arm,primecell";
+		reg = <0x826000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
+		clock-names = "apb_pclk", "atclk";
+
+		port {
+			etr_in: endpoint {
+				slave-mode;
+				remote-endpoint = <&replicator_out0>;
+			};
+		};
+	};
+
+	funnel@841000 {	/* APSS funnel only 4 inputs are used */
+		compatible = "arm,coresight-funnel", "arm,primecell";
+		reg = <0x841000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
+		clock-names = "apb_pclk", "atclk";
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				funnel1_in0: endpoint {
+					slave-mode;
+					remote-endpoint = <&etm0_out>;
+				};
+			};
+			port@1 {
+				reg = <1>;
+				funnel1_in1: endpoint {
+					slave-mode;
+					remote-endpoint = <&etm1_out>;
+				};
+			};
+			port@2 {
+				reg = <2>;
+				funnel1_in2: endpoint {
+					slave-mode;
+					remote-endpoint = <&etm2_out>;
+				};
+			};
+			port@3 {
+				reg = <3>;
+				funnel1_in3: endpoint {
+					slave-mode;
+					remote-endpoint = <&etm3_out>;
+				};
+			};
+			port@4 {
+				reg = <0>;
+				funnel1_out: endpoint {
+					remote-endpoint = <&funnel0_in4>;
+				};
+			};
+		};
+	};
+
+	etm@85c000 {
+		compatible = "arm,coresight-etm4x", "arm,primecell";
+		reg = <0x85c000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
+		clock-names = "apb_pclk", "atclk";
+
+		cpu = <&CPU0>;
+
+		port {
+			etm0_out: endpoint {
+				remote-endpoint = <&funnel1_in0>;
+			};
+		};
+	};
+
+	etm@85d000 {
+		compatible = "arm,coresight-etm4x", "arm,primecell";
+		reg = <0x85d000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
+		clock-names = "apb_pclk", "atclk";
+
+		cpu = <&CPU1>;
+
+		port {
+			etm1_out: endpoint {
+				remote-endpoint = <&funnel1_in1>;
+			};
+		};
+	};
+
+	etm@85e000 {
+		compatible = "arm,coresight-etm4x", "arm,primecell";
+		reg = <0x85e000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
+		clock-names = "apb_pclk", "atclk";
+
+		cpu = <&CPU2>;
+
+		port {
+			etm2_out: endpoint {
+				remote-endpoint = <&funnel1_in2>;
+			};
+		};
+	};
+
+	etm@85f000 {
+		compatible = "arm,coresight-etm4x", "arm,primecell";
+		reg = <0x85f000 0x1000>;
+
+		clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
+		clock-names = "apb_pclk", "atclk";
+
+		cpu = <&CPU3>;
+
+		port {
+			etm3_out: endpoint {
+				remote-endpoint = <&funnel1_in3>;
+			};
+		};
+	};
+};
--
1.9.1

Re: [PATCH v2 2/2] arm64: dts: qcom: Add msm8916 CoreSight components

[Mathieu Poirier]

On 7 May 2015 at 09:36, Ivan T. Ivanov <ivan.ivanov@linaro.org> wrote:
> Add initial set of CoreSight components found on Qualcomm's 8x16 chipset.
>
> Signed-off-by: Ivan T. Ivanov <ivan.ivanov@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/msm8916-coresight.dtsi | 254 ++++++++++++++++++++++++
>  1 file changed, 254 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/qcom/msm8916-coresight.dtsi
>
> diff --git a/arch/arm64/boot/dts/qcom/msm8916-coresight.dtsi b/arch/arm64/boot/dts/qcom/msm8916-coresight.dtsi
> new file mode 100644
> index 0000000..33ae981
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/msm8916-coresight.dtsi
> @@ -0,0 +1,254 @@
> +/*
> + * Copyright (c) 2013 - 2015, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +&soc {
> +
> +       tpiu@820000 {
> +               compatible = "arm,coresight-tpiu", "arm,primecell";
> +               reg = <0x820000 0x1000>;
> +
> +               clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> +               clock-names = "apb_pclk", "atclk";
> +
> +               port {
> +                       tpiu_in: endpoint {
> +                               slave-mode;
> +                               remote-endpoint = <&replicator_out1>;
> +                       };
> +               };
> +       };
> +
> +       funnel@821000 {
> +               compatible = "arm,coresight-funnel", "arm,primecell";
> +               reg = <0x821000 0x1000>;
> +
> +               clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> +               clock-names = "apb_pclk", "atclk";
> +
> +               ports {
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +
> +                       /*
> +                        * Not described input ports:
> +                        * 0 - connected to Resource and Power Manger CPU ETM
> +                        * 1 - not-connected
> +                        * 2 - connected to Modem CPU ETM
> +                        * 3 - not-connected
> +                        * 5 - not-connected
> +                        * 6 - connected trought funnel to Wireless CPU ETM
> +                        * 7 - connected to STM component
> +                        */
> +                       port@4 {
> +                               reg = <4>;
> +                               funnel0_in4: endpoint {
> +                                       slave-mode;
> +                                       remote-endpoint = <&funnel1_out>;
> +                               };
> +                       };
> +                       port@8 {
> +                               reg = <0>;
> +                               funnel0_out: endpoint {
> +                                       remote-endpoint = <&etf_in>;
> +                               };
> +                       };
> +               };
> +       };
> +
> +       replicator@824000 {
> +               compatible = "qcom,coresight-replicator", "arm,primecell";

Shouldn't it be "qcom,coresight-replicator1x" ?


> +               reg = <0x824000 0x1000>;
> +
> +               clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> +               clock-names = "apb_pclk", "atclk";
> +
> +               ports {
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +
> +                       port@0 {
> +                               reg = <0>;
> +                               replicator_out0: endpoint {
> +                                       remote-endpoint = <&etr_in>;
> +                               };
> +                       };
> +                       port@1 {
> +                               reg = <1>;
> +                               replicator_out1: endpoint {
> +                                       remote-endpoint = <&tpiu_in>;
> +                               };
> +                       };
> +                       port@2 {
> +                               reg = <0>;
> +                               replicator_in: endpoint {
> +                                       slave-mode;
> +                                       remote-endpoint = <&etf_out>;
> +                               };
> +                       };
> +               };
> +       };
> +
> +       etf@825000 {
> +               compatible = "arm,coresight-tmc", "arm,primecell";
> +               reg = <0x825000 0x1000>;
> +
> +               clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> +               clock-names = "apb_pclk", "atclk";
> +
> +               ports {
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +
> +                       port@0 {
> +                               reg = <0>;
> +                               etf_out: endpoint {
> +                                       slave-mode;
> +                                       remote-endpoint = <&funnel0_out>;
> +                               };
> +                       };
> +                       port@1 {
> +                               reg = <0>;
> +                               etf_in: endpoint {
> +                                       remote-endpoint = <&replicator_in>;
> +                               };
> +                       };
> +               };
> +       };
> +
> +       etr@826000 {
> +               compatible = "arm,coresight-tmc", "arm,primecell";
> +               reg = <0x826000 0x1000>;
> +
> +               clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> +               clock-names = "apb_pclk", "atclk";
> +
> +               port {
> +                       etr_in: endpoint {
> +                               slave-mode;
> +                               remote-endpoint = <&replicator_out0>;
> +                       };
> +               };
> +       };
> +
> +       funnel@841000 { /* APSS funnel only 4 inputs are used */
> +               compatible = "arm,coresight-funnel", "arm,primecell";
> +               reg = <0x841000 0x1000>;
> +
> +               clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> +               clock-names = "apb_pclk", "atclk";
> +
> +               ports {
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +
> +                       port@0 {
> +                               reg = <0>;
> +                               funnel1_in0: endpoint {
> +                                       slave-mode;
> +                                       remote-endpoint = <&etm0_out>;
> +                               };
> +                       };
> +                       port@1 {
> +                               reg = <1>;
> +                               funnel1_in1: endpoint {
> +                                       slave-mode;
> +                                       remote-endpoint = <&etm1_out>;
> +                               };
> +                       };
> +                       port@2 {
> +                               reg = <2>;
> +                               funnel1_in2: endpoint {
> +                                       slave-mode;
> +                                       remote-endpoint = <&etm2_out>;
> +                               };
> +                       };
> +                       port@3 {
> +                               reg = <3>;
> +                               funnel1_in3: endpoint {
> +                                       slave-mode;
> +                                       remote-endpoint = <&etm3_out>;
> +                               };
> +                       };
> +                       port@4 {
> +                               reg = <0>;
> +                               funnel1_out: endpoint {
> +                                       remote-endpoint = <&funnel0_in4>;
> +                               };
> +                       };
> +               };
> +       };
> +
> +       etm@85c000 {
> +               compatible = "arm,coresight-etm4x", "arm,primecell";
> +               reg = <0x85c000 0x1000>;
> +
> +               clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> +               clock-names = "apb_pclk", "atclk";
> +
> +               cpu = <&CPU0>;
> +
> +               port {
> +                       etm0_out: endpoint {
> +                               remote-endpoint = <&funnel1_in0>;
> +                       };
> +               };
> +       };
> +
> +       etm@85d000 {
> +               compatible = "arm,coresight-etm4x", "arm,primecell";
> +               reg = <0x85d000 0x1000>;
> +
> +               clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> +               clock-names = "apb_pclk", "atclk";
> +
> +               cpu = <&CPU1>;
> +
> +               port {
> +                       etm1_out: endpoint {
> +                               remote-endpoint = <&funnel1_in1>;
> +                       };
> +               };
> +       };
> +
> +       etm@85e000 {
> +               compatible = "arm,coresight-etm4x", "arm,primecell";
> +               reg = <0x85e000 0x1000>;
> +
> +               clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> +               clock-names = "apb_pclk", "atclk";
> +
> +               cpu = <&CPU2>;
> +
> +               port {
> +                       etm2_out: endpoint {
> +                               remote-endpoint = <&funnel1_in2>;
> +                       };
> +               };
> +       };
> +
> +       etm@85f000 {
> +               compatible = "arm,coresight-etm4x", "arm,primecell";
> +               reg = <0x85f000 0x1000>;
> +
> +               clocks = <&rpmcc RPM_QDSS_CLK>, <&rpmcc RPM_QDSS_A_CLK>;
> +               clock-names = "apb_pclk", "atclk";
> +
> +               cpu = <&CPU3>;
> +
> +               port {
> +                       etm3_out: endpoint {
> +                               remote-endpoint = <&funnel1_in3>;
> +                       };
> +               };
> +       };
> +};
> --
> 1.9.1
>

Re: [PATCH v2 2/2] arm64: dts: qcom: Add msm8916 CoreSight components

[Ivan T. Ivanov]

On Fri, 2015-05-08 at 07:38 -0600, Mathieu Poirier wrote:
> On 7 May 2015 at 09:36, Ivan T. Ivanov ivanov@linaro.org> wrote:
> > Add initial set of CoreSight components found on Qualcomm's 8x16 chipset.
> > 
> > 
> > +       replicator@824000 {
> > +               compatible = "qcom,coresight-replicator", "arm,primecell";
> 
> Shouldn't it be "qcom,coresight-replicator1x" ?
> 



True, I still wonder, why we have to have this compatible string? 
Drivers are probed by amba_id and "arm,primecell", after all.

Thanks,
Ivan

Re: [PATCH v2 2/2] arm64: dts: qcom: Add msm8916 CoreSight components

[Mathieu Poirier]

On 8 May 2015 at 07:47, Ivan T. Ivanov <ivan.ivanov@linaro.org> wrote:
>
> On Fri, 2015-05-08 at 07:38 -0600, Mathieu Poirier wrote:
>> On 7 May 2015 at 09:36, Ivan T. Ivanov ivanov@linaro.org> wrote:
>> > Add initial set of CoreSight components found on Qualcomm's 8x16 chipset.
>> >
>> >
>> > +       replicator@824000 {
>> > +               compatible = "qcom,coresight-replicator", "arm,primecell";
>>
>> Shouldn't it be "qcom,coresight-replicator1x" ?
>>
>
>
>
> True, I still wonder, why we have to have this compatible string?
> Drivers are probed by amba_id and "arm,primecell", after all.
>

Drivers have their own compatible strings for historical reasons,
something I've been meaning to fix for a long time now...

> Thanks,
> Ivan

Re: [PATCH v2 2/2] arm64: dts: qcom: Add msm8916 CoreSight components

[Ivan T. Ivanov]

On Fri, 2015-05-08 at 08:13 -0600, Mathieu Poirier wrote:
> On 8 May 2015 at 07:47, Ivan T. Ivanov ivanov@linaro.org> wrote:
> > On Fri, 2015-05-08 at 07:38 -0600, Mathieu Poirier wrote:
> > > On 7 May 2015 at 09:36, Ivan T. Ivanov ivanov@linaro.org> wrote:
> > > > Add initial set of CoreSight components found on Qualcomm's 8x16 chipset.
> > > > 
> > > > 
> > > > +       replicator@824000 {
> > > > +               compatible = "qcom,coresight-replicator", "arm,primecell";
> > > 
> > > Shouldn't it be "qcom,coresight-replicator1x" ?
> > > 
> > 
> > 
> > 
> > True, I still wonder, why we have to have this compatible string?
> > Drivers are probed by amba_id and "arm,primecell", after all.
> > 
> 
> Drivers have their own compatible strings for historical reasons,
> something I've been meaning to fix for a long time now...
> 

Yep, I see that they have been platform drivers in the past, but now 
they are not, except coresight-replicator driver. IMHO, having
additional compatible string could lead just to confusion. 

Regards,
Ivan

Re: [PATCH v2 2/2] arm64: dts: qcom: Add msm8916 CoreSight components

[Mathieu Poirier]

On 8 May 2015 at 08:17, Ivan T. Ivanov <ivan.ivanov@linaro.org> wrote:
>
> On Fri, 2015-05-08 at 08:13 -0600, Mathieu Poirier wrote:
>> On 8 May 2015 at 07:47, Ivan T. Ivanov ivanov@linaro.org> wrote:
>> > On Fri, 2015-05-08 at 07:38 -0600, Mathieu Poirier wrote:
>> > > On 7 May 2015 at 09:36, Ivan T. Ivanov ivanov@linaro.org> wrote:
>> > > > Add initial set of CoreSight components found on Qualcomm's 8x16 chipset.
>> > > >
>> > > >
>> > > > +       replicator@824000 {
>> > > > +               compatible = "qcom,coresight-replicator", "arm,primecell";
>> > >
>> > > Shouldn't it be "qcom,coresight-replicator1x" ?
>> > >
>> >
>> >
>> >
>> > True, I still wonder, why we have to have this compatible string?
>> > Drivers are probed by amba_id and "arm,primecell", after all.
>> >
>>
>> Drivers have their own compatible strings for historical reasons,
>> something I've been meaning to fix for a long time now...
>>
>
> Yep, I see that they have been platform drivers in the past, but now
> they are not, except coresight-replicator driver. IMHO, having
> additional compatible string could lead just to confusion.

I did a little more research on this and based on what I found in the
kernel it may not need "fixing" after all.  The majority of drivers
that do specify "arm,primecell" also specify a device-specific
compatible string.  And in the case of CoreSight devices were
implementers can do pretty much whatever they  want with the ID
strings, it is only a matter of time before we need to call something
like of_device_is_compatible() to fix a quirk.

Unless someone heavy asks to remove the device-specific compatible
strings I'd prefer keeping the current trend set forth by other
drivers and as such, will ask you to add the "1x" in this bindings.

Thanks,
Mathieu

>
> Regards,
> Ivan

Re: [PATCH v2 2/2] arm64: dts: qcom: Add msm8916 CoreSight components

[Ivan T. Ivanov]

> On May 8, 2015, at 7:17 PM, Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
> 
> On 8 May 2015 at 08:17, Ivan T. Ivanov <ivan.ivanov@linaro.org> wrote:
>> 
>> On Fri, 2015-05-08 at 08:13 -0600, Mathieu Poirier wrote:
>>> On 8 May 2015 at 07:47, Ivan T. Ivanov ivanov@linaro.org> wrote:
>>>> On Fri, 2015-05-08 at 07:38 -0600, Mathieu Poirier wrote:
>>>>> On 7 May 2015 at 09:36, Ivan T. Ivanov ivanov@linaro.org> wrote:
>>>>>> Add initial set of CoreSight components found on Qualcomm's 8x16 chipset.
>>>>>> 
>>>>>> 
>>>>>> +       replicator@824000 {
>>>>>> +               compatible = "qcom,coresight-replicator", "arm,primecell";
>>>>> 
>>>>> Shouldn't it be "qcom,coresight-replicator1x" ?
>>>>> 
>>>> 
>>>> 
>>>> 
>>>> True, I still wonder, why we have to have this compatible string?
>>>> Drivers are probed by amba_id and "arm,primecell", after all.
>>>> 
>>> 
>>> Drivers have their own compatible strings for historical reasons,
>>> something I've been meaning to fix for a long time now...
>>> 
>> 
>> Yep, I see that they have been platform drivers in the past, but now
>> they are not, except coresight-replicator driver. IMHO, having
>> additional compatible string could lead just to confusion.
> 
> I did a little more research on this and based on what I found in the
> kernel it may not need "fixing" after all.  The majority of drivers
> that do specify "arm,primecell" also specify a device-specific
> compatible string.  And in the case of CoreSight devices were
> implementers can do pretty much whatever they  want with the ID
> strings, it is only a matter of time before we need to call something
> like of_device_is_compatible() to fix a quirk.
> 
> Unless someone heavy asks to remove the device-specific compatible
> strings I'd prefer keeping the current trend set forth by other
> drivers and as such, will ask you to add the "1x" in this bindings.


Well, I don’t strongly object against this “1x”, I will add it. 
My point is that if we can dynamically detect device version, 
which we can do in this case, it will be more robust to do it
in this way. 

If there are not issues with patch 1/2, I will like to fix and 
resend only this patch.

Regards,
Ivan

  

Re: [PATCH v2 2/2] arm64: dts: qcom: Add msm8916 CoreSight components

[Mathieu Poirier]

On 8 May 2015 at 11:57, Ivan T. Ivanov <ivan.ivanov@linaro.org> wrote:
>
>> On May 8, 2015, at 7:17 PM, Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
>>
>> On 8 May 2015 at 08:17, Ivan T. Ivanov <ivan.ivanov@linaro.org> wrote:
>>>
>>> On Fri, 2015-05-08 at 08:13 -0600, Mathieu Poirier wrote:
>>>> On 8 May 2015 at 07:47, Ivan T. Ivanov ivanov@linaro.org> wrote:
>>>>> On Fri, 2015-05-08 at 07:38 -0600, Mathieu Poirier wrote:
>>>>>> On 7 May 2015 at 09:36, Ivan T. Ivanov ivanov@linaro.org> wrote:
>>>>>>> Add initial set of CoreSight components found on Qualcomm's 8x16 chipset.
>>>>>>>
>>>>>>>
>>>>>>> +       replicator@824000 {
>>>>>>> +               compatible = "qcom,coresight-replicator", "arm,primecell";
>>>>>>
>>>>>> Shouldn't it be "qcom,coresight-replicator1x" ?
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> True, I still wonder, why we have to have this compatible string?
>>>>> Drivers are probed by amba_id and "arm,primecell", after all.
>>>>>
>>>>
>>>> Drivers have their own compatible strings for historical reasons,
>>>> something I've been meaning to fix for a long time now...
>>>>
>>>
>>> Yep, I see that they have been platform drivers in the past, but now
>>> they are not, except coresight-replicator driver. IMHO, having
>>> additional compatible string could lead just to confusion.
>>
>> I did a little more research on this and based on what I found in the
>> kernel it may not need "fixing" after all.  The majority of drivers
>> that do specify "arm,primecell" also specify a device-specific
>> compatible string.  And in the case of CoreSight devices were
>> implementers can do pretty much whatever they  want with the ID
>> strings, it is only a matter of time before we need to call something
>> like of_device_is_compatible() to fix a quirk.
>>
>> Unless someone heavy asks to remove the device-specific compatible
>> strings I'd prefer keeping the current trend set forth by other
>> drivers and as such, will ask you to add the "1x" in this bindings.
>
>
> Well, I don’t strongly object against this “1x”, I will add it.
> My point is that if we can dynamically detect device version,
> which we can do in this case, it will be more robust to do it
> in this way.

I agree with you.  The device specific bindings will come handy when
there is a problem with the device version, something that is bound to
happen.

>
> If there are not issues with patch 1/2, I will like to fix and
> resend only this patch.

I'm good with 1/2, just this patch will be fine.

>
> Regards,
> Ivan
>
>

Re: [PATCH v2 2/2] arm64: dts: qcom: Add msm8916 CoreSight components

[Mark Rutland]

On Fri, May 08, 2015 at 02:47:57PM +0100, Ivan T. Ivanov wrote:
> 
> On Fri, 2015-05-08 at 07:38 -0600, Mathieu Poirier wrote:
> > On 7 May 2015 at 09:36, Ivan T. Ivanov ivanov@linaro.org> wrote:
> > > Add initial set of CoreSight components found on Qualcomm's 8x16 chipset.
> > > 
> > > 
> > > +       replicator@824000 {
> > > +               compatible = "qcom,coresight-replicator", "arm,primecell";
> > 
> > Shouldn't it be "qcom,coresight-replicator1x" ?
> > 
> 
> 
> 
> True, I still wonder, why we have to have this compatible string? 
> Drivers are probed by amba_id and "arm,primecell", after all.

The compatible string tells you both the device _and_ the format of the
other properties, because it tells you which binding applies.

So the compatible string should be present regardless, as
"arm,primecell" does not define the majority of the properties you need
for the replicator node.

Thanks,
Mark.

Re: [PATCH v2 2/2] arm64: dts: qcom: Add msm8916 CoreSight components

[Ivan T. Ivanov]

> On May 8, 2015, at 9:00 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> 
> On Fri, May 08, 2015 at 02:47:57PM +0100, Ivan T. Ivanov wrote:
>> 
>> On Fri, 2015-05-08 at 07:38 -0600, Mathieu Poirier wrote:
>>> On 7 May 2015 at 09:36, Ivan T. Ivanov ivanov@linaro.org> wrote:
>>>> Add initial set of CoreSight components found on Qualcomm's 8x16 chipset.
>>>> 
>>>> 
>>>> +       replicator@824000 {
>>>> +               compatible = "qcom,coresight-replicator", "arm,primecell";
>>> 
>>> Shouldn't it be "qcom,coresight-replicator1x" ?
>>> 
>> 
>> 
>> 
>> True, I still wonder, why we have to have this compatible string? 
>> Drivers are probed by amba_id and "arm,primecell", after all.
> 
> The compatible string tells you both the device _and_ the format of the
> other properties, because it tells you which binding applies.
> 
> So the compatible string should be present regardless, as
> "arm,primecell" does not define the majority of the properties you need
> for the replicator node.

Mmm, only if vendors don’t bother to update version information
fused to revision id registers, which happens. And this could 
be workaround by "arm,primecell-periphid”, no?

Regards,
Ivan

Re: [PATCH v2 2/2] arm64: dts: qcom: Add msm8916 CoreSight components

[Mark Rutland]

On Fri, May 08, 2015 at 07:17:12PM +0100, Ivan T. Ivanov wrote:
> 
> > On May 8, 2015, at 9:00 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > 
> > On Fri, May 08, 2015 at 02:47:57PM +0100, Ivan T. Ivanov wrote:
> >> 
> >> On Fri, 2015-05-08 at 07:38 -0600, Mathieu Poirier wrote:
> >>> On 7 May 2015 at 09:36, Ivan T. Ivanov ivanov@linaro.org> wrote:
> >>>> Add initial set of CoreSight components found on Qualcomm's 8x16 chipset.
> >>>> 
> >>>> 
> >>>> +       replicator@824000 {
> >>>> +               compatible = "qcom,coresight-replicator", "arm,primecell";
> >>> 
> >>> Shouldn't it be "qcom,coresight-replicator1x" ?
> >>> 
> >> 
> >> 
> >> 
> >> True, I still wonder, why we have to have this compatible string? 
> >> Drivers are probed by amba_id and "arm,primecell", after all.
> > 
> > The compatible string tells you both the device _and_ the format of the
> > other properties, because it tells you which binding applies.
> > 
> > So the compatible string should be present regardless, as
> > "arm,primecell" does not define the majority of the properties you need
> > for the replicator node.
> 
> Mmm, only if vendors don’t bother to update version information
> fused to revision id registers, which happens. And this could 
> be workaround by "arm,primecell-periphid”, no?

No. That only tells you the identity of the device, not the format of
the binding.

Thanks,
Mark.