[RFC PATCH 0/3] Update AMBA driver for enhanced component ID spec

[RFC PATCH 0/3] Update AMBA driver for enhanced component ID spec

[Mike Leach]

The latest ARM CoreSight specification updates the component identification
requirements for all components attached to an AMBA bus. (ARM IHI 0029E)

This specification defines bits 11:8 in the ComponentID (CID) value as the
device class. Identification requirements now depend on this class.
Class 0xF: Traditional components identified by Peripheral ID (PID) only.
Class 0x9: CoreSight components may be identified by a Universal Component
Identifier (UCI) consisting of the PID plus CoreSight DevType and DevArch
values.

Current and future ARM CoreSight IP will now use the same PID for
components on the same function - e.g. the ETM, CTI, PMU and Debug elements
associated with a core. The first core to use this UCI method is the A35,
which currently has binding entries in the ETMv4 driver.

This patchset prepares for the addition of the upcoming CTI driver, which
will need to correctly bind with A35 and future hardware, while overcoming
the limitation of binding by PID alone, which cannot now work.

The patchset updates the current AMBA Identification mechanism, which was
already differentiating between 0xF and 0x9 CIDs, to add
additional UCI compliant tests for the for the 0x9 device class.

Additional UCI structures are provided and added to the ETMv4 driver as
appropriate.

An additional test patch is provided to test the mechanism on the DB410C
96boards platform. This is not intended to be upstreamed.

Mike Leach (3):
  drivers: amba: Updates to component identification for driver
    matching.
  coresight: etmv4: Update ID register table to add  UCI support
  amba: coresight: Driver test for new CoreSight UCI matching

 arch/arm64/boot/dts/qcom/msm8916.dtsi         |  9 +++
 drivers/amba/bus.c                            | 59 +++++++++++++++++--
 drivers/hwtracing/coresight/coresight-etm4x.c | 27 +++++++--
 include/linux/amba/bus.h                      | 33 +++++++++++
 4 files changed, 119 insertions(+), 9 deletions(-)

-- 
2.19.1

[RFC PATCH 1/3] drivers: amba: Updates to component identification for driver matching.

[Mike Leach]

The CoreSight specification (ARM IHI 0029E), updates the ID register
requirements for components on an AMBA bus, to cover both traditional
ARM Primecell type devices, and newer CoreSight and other components.

The Peripheral ID (PID) / Component ID (CID) pair is extended in certain
cases to uniquely identify components. CoreSight components related to
a single function can share Peripheral ID values, and must be further
identified using a Unique Component Identifier (UCI). e.g. the ETM, CTI,
PMU and Debug hardware of the A35 all share the same PID.

Bits 11:8 of the CID are defined to be the device class.
Class 0xF remains for PrimeCell and legacy components.
Class 0x9 defines the component as CoreSight (CORESIGHT_CID above)
Class 0x0, 0x1, 0xB, 0xE define components that do not have driver support
at present.
Class 0x2-0x8,0xA and 0xD-0xD are presently reserved.

The specification futher defines which classes of device use the standard
CID/PID pair, and when additional ID registers are required.

The patches provide an update of amba_device and matching code to handle
the additional registers required for the Class 0x9 (CoreSight) UCI.
The *data pointer in the amba_id is used by the driver to provide extended
ID register values for matching.

CoreSight components where PID/CID pair is currently sufficient for
unique identification need not provide this additional information.

Signed-off-by: Mike Leach <mike.leach@linaro.org>
---
 drivers/amba/bus.c       | 48 ++++++++++++++++++++++++++++++++++++----
 include/linux/amba/bus.h | 33 +++++++++++++++++++++++++++
 2 files changed, 77 insertions(+), 4 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 41b706403ef7..6eab977f4314 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -26,6 +26,28 @@
 
 #define to_amba_driver(d)	container_of(d, struct amba_driver, drv)
 
+/* called on periphid match and class 0x9 coresight device. */
+static int amba_uci_match(const struct amba_id *table, struct amba_device *dev)
+{
+	int ret = 0;
+	struct amba_cs_uci_id *uci;
+
+	uci = table->data;
+
+	/* no table data - return match on periphid */
+	if (!uci)
+		return 1;
+
+	if (uci->devarch) {
+		ret = (dev->uci.devtype == uci->devtype) &&
+		       ((dev->uci.devarch & uci->mask) == uci->devarch);
+	} else {
+		/* devtype only if devarch set to 0 */
+		ret = dev->uci.devtype == uci->devtype;
+	}
+	return ret;
+}
+
 static const struct amba_id *
 amba_lookup(const struct amba_id *table, struct amba_device *dev)
 {
@@ -33,11 +55,17 @@ amba_lookup(const struct amba_id *table, struct amba_device *dev)
 
 	while (table->mask) {
 		ret = (dev->periphid & table->mask) == table->id;
-		if (ret)
-			break;
+		/* matched on periphid - check UCI if CoreSight */
+		if (ret) {
+			if (dev->cid == CORESIGHT_CID)	{
+				ret = amba_uci_match(table, dev);
+				if (ret)
+					break;
+			} else
+				break;
+		}
 		table++;
 	}
-
 	return ret ? table : NULL;
 }
 
@@ -399,10 +427,22 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
 			cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) <<
 				(i * 8);
 
+		if (cid == CORESIGHT_CID) {
+			/* set the base to the start of the last 4k block */
+			void __iomem *csbase = tmp + size - 4096;
+
+			dev->uci.devarch =
+				readl(csbase + UCI_REG_DEVARCH_OFFSET);
+			dev->uci.devtype =
+				readl(csbase + UCI_REG_DEVTYPE_OFFSET) & 0xff;
+		}
+
 		amba_put_disable_pclk(dev);
 
-		if (cid == AMBA_CID || cid == CORESIGHT_CID)
+		if (cid == AMBA_CID || cid == CORESIGHT_CID) {
 			dev->periphid = pid;
+			dev->cid = cid;
+		}
 
 		if (!dev->periphid)
 			ret = -ENODEV;
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index d143c13bed26..a83a0a3dece8 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -25,6 +25,36 @@
 #define AMBA_CID	0xb105f00d
 #define CORESIGHT_CID	0xb105900d
 
+/*
+ * CoreSight Architecture specification updates the ID specification
+ * for components on the AMBA bus. (ARM IHI 0029E)
+ *
+ * Bits 11:8 of the CID are the device class.
+ *
+ * Class 0xF remains for PrimeCell and legacy components. (AMBA_CID above)
+ * Class 0x9 defines the component as CoreSight (CORESIGHT_CID above)
+ * Class 0x0, 0x1, 0xB, 0xE define components that do not have driver support
+ * at present.
+ * Class 0x2-0x8,0xA and 0xD-0xD are presently reserved.
+ *
+ * Remaining CID bits stay as 0xb105-00d
+ */
+
+/*
+ * Class 0x9 components use additional values to form a Unique Component
+ * Identifier (UCI), where peripheral ID values are identical for different
+ * components. Passed to the amba bus code from the component driver via
+ * the amba_id->data pointer.
+ */
+struct amba_cs_uci_id {
+	unsigned int devarch;
+	unsigned int mask;
+	unsigned int devtype;
+};
+
+#define UCI_REG_DEVTYPE_OFFSET	0xFCC
+#define UCI_REG_DEVARCH_OFFSET	0xFBC
+
 struct clk;
 
 struct amba_device {
@@ -32,6 +62,8 @@ struct amba_device {
 	struct resource		res;
 	struct clk		*pclk;
 	unsigned int		periphid;
+	unsigned int		cid;
+	struct amba_cs_uci_id	uci;
 	unsigned int		irq[AMBA_NR_IRQS];
 	char			*driver_override;
 };
@@ -61,6 +93,7 @@ enum amba_vendor {
 	(((conf) & 0xff) << 24 | ((rev) & 0xf) << 20 | \
 	AMBA_VENDOR_LINUX << 12 | ((part) & 0xfff))
 
+
 extern struct bus_type amba_bustype;
 
 #define to_amba_device(d)	container_of(d, struct amba_device, dev)
-- 
2.19.1

[RFC PATCH 2/3] coresight: etmv4: Update ID register table to add UCI support

[Mike Leach]

Updates the ID register tables to contain a UCI entry for the A35 ETM
device to allow correct matching of driver in the amba bus code.

Signed-off-by: Mike Leach <mike.leach@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etm4x.c | 26 +++++++++++++++----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
index 53e2fb6e86f6..1dcb7e14ea6b 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x.c
@@ -1073,12 +1073,28 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id)
 		.mask	= 0x000fffff,		\
 	}
 
+static struct amba_cs_uci_id uci_id_etm4[] = {
+	{
+		/*  ETMv4 UCI data */
+		.devarch	= 0x00004a13,
+		.mask		= 0x0000ffff,
+		.devtype	= 0x00000013,
+	}
+};
+
+#define ETM4x_AMBA_UCI_ID(pid)			\
+	{					\
+		.id	= pid,			\
+		.mask	= 0x000fffff,		\
+		.data	= uci_id_etm4,		\
+	}
+
 static const struct amba_id etm4_ids[] = {
-	ETM4x_AMBA_ID(0x000bb95d),		/* Cortex-A53 */
-	ETM4x_AMBA_ID(0x000bb95e),		/* Cortex-A57 */
-	ETM4x_AMBA_ID(0x000bb95a),		/* Cortex-A72 */
-	ETM4x_AMBA_ID(0x000bb959),		/* Cortex-A73 */
-	ETM4x_AMBA_ID(0x000bb9da),		/* Cortex-A35 */
+	ETM4x_AMBA_ID(0x000bb95d),			/* Cortex-A53 */
+	ETM4x_AMBA_ID(0x000bb95e),			/* Cortex-A57 */
+	ETM4x_AMBA_ID(0x000bb95a),			/* Cortex-A72 */
+	ETM4x_AMBA_ID(0x000bb959),			/* Cortex-A73 */
+	ETM4x_AMBA_UCI_ID(0x000bb9da),			/* Cortex-A35 */
 	{},
 };
 
-- 
2.19.1

[RFC PATCH 3/3] amba: coresight: Driver test for new CoreSight UCI matching

[Mike Leach]

This patch adds in logging and modifications to amba driver, etmv4 driver
and DB410C device tree to allow testing of the new UCI component matching
algorithm used for certain class of components on an AMBA bus.

Signed-off-by: Mike Leach <mike.leach@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8916.dtsi         |  9 +++++++++
 drivers/amba/bus.c                            | 13 ++++++++++++-
 drivers/hwtracing/coresight/coresight-etm4x.c |  3 ++-
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index d302d8d639a1..c8b503a63b2c 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -1368,6 +1368,15 @@
 			};
 		};
 
+		/* add an as yet unsupported CTI for UCI test - CPU-0 */
+		cti at 858000 {
+			compatible = "arm,coresight-cti", "arm,primecell";
+			reg = <0x858000 0x1000>;
+
+			clocks = <&rpmcc RPM_QDSS_CLK>;
+			clock-names = "apb_pclk";
+		};
+
 		venus: video-codec at 1d00000 {
 			compatible = "qcom,msm8916-venus";
 			reg = <0x01d00000 0xff000>;
diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 6eab977f4314..3a27d655dcc9 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -31,19 +31,30 @@ static int amba_uci_match(const struct amba_id *table, struct amba_device *dev)
 {
 	int ret = 0;
 	struct amba_cs_uci_id *uci;
+	struct device *adev; /* device for test logging */
 
 	uci = table->data;
+	adev = &dev->dev;
 
 	/* no table data - return match on periphid */
-	if (!uci)
+	if (!uci) {
+		dev_info(adev, "uci_match: no UCI, use periphID\n");
 		return 1;
+	}
 
 	if (uci->devarch) {
 		ret = (dev->uci.devtype == uci->devtype) &&
 		       ((dev->uci.devarch & uci->mask) == uci->devarch);
+		dev_info(adev, "device: devtype[%x]; devarch[%x];\n",
+			 dev->uci.devtype, dev->uci.devarch);
+		dev_info(adev, "uci_match: devtype[%x]; devarch[%x]; (%s)\n",
+			 uci->devtype, uci->devarch,
+			 ret ? "match" : "no match");
 	} else {
 		/* devtype only if devarch set to 0 */
 		ret = dev->uci.devtype == uci->devtype;
+		dev_info(adev, "uci_match: devtype-only[%x]; (%s)\n",
+			 uci->devtype, ret ? "match" : "no match");
 	}
 	return ret;
 }
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
index 1dcb7e14ea6b..13d674a02194 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x.c
@@ -1090,11 +1090,12 @@ static struct amba_cs_uci_id uci_id_etm4[] = {
 	}
 
 static const struct amba_id etm4_ids[] = {
-	ETM4x_AMBA_ID(0x000bb95d),			/* Cortex-A53 */
+	ETM4x_AMBA_UCI_ID(0x000bb95d),	/* C-A53 - UCI optional for test */
 	ETM4x_AMBA_ID(0x000bb95e),			/* Cortex-A57 */
 	ETM4x_AMBA_ID(0x000bb95a),			/* Cortex-A72 */
 	ETM4x_AMBA_ID(0x000bb959),			/* Cortex-A73 */
 	ETM4x_AMBA_UCI_ID(0x000bb9da),			/* Cortex-A35 */
+	ETM4x_AMBA_UCI_ID(0x000bb9a8),	/* CTI PID A53 - test fail UCI match */
 	{},
 };
 
-- 
2.19.1

[RFC PATCH 1/3] drivers: amba: Updates to component identification for driver matching.

[Suzuki K Poulose]

Hi Mike,

On 11/15/2018 01:28 AM, Mike Leach wrote:
> The CoreSight specification (ARM IHI 0029E), updates the ID register
> requirements for components on an AMBA bus, to cover both traditional
> ARM Primecell type devices, and newer CoreSight and other components.
> 
> The Peripheral ID (PID) / Component ID (CID) pair is extended in certain
> cases to uniquely identify components. CoreSight components related to
> a single function can share Peripheral ID values, and must be further
> identified using a Unique Component Identifier (UCI). e.g. the ETM, CTI,
> PMU and Debug hardware of the A35 all share the same PID.
> 
> Bits 11:8 of the CID are defined to be the device class.
> Class 0xF remains for PrimeCell and legacy components.
> Class 0x9 defines the component as CoreSight (CORESIGHT_CID above)
> Class 0x0, 0x1, 0xB, 0xE define components that do not have driver support
> at present.
> Class 0x2-0x8,0xA and 0xD-0xD are presently reserved.
> 
> The specification futher defines which classes of device use the standard
> CID/PID pair, and when additional ID registers are required.
> 
> The patches provide an update of amba_device and matching code to handle
> the additional registers required for the Class 0x9 (CoreSight) UCI.
> The *data pointer in the amba_id is used by the driver to provide extended
> ID register values for matching.
> 
> CoreSight components where PID/CID pair is currently sufficient for
> unique identification need not provide this additional information.
> 
> Signed-off-by: Mike Leach <mike.leach@linaro.org>

The patch looks good to me. A couple of minor comments below.

> ---
>   drivers/amba/bus.c       | 48 ++++++++++++++++++++++++++++++++++++----
>   include/linux/amba/bus.h | 33 +++++++++++++++++++++++++++
>   2 files changed, 77 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 41b706403ef7..6eab977f4314 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -26,6 +26,28 @@
>   
>   #define to_amba_driver(d)	container_of(d, struct amba_driver, drv)
>   
> +/* called on periphid match and class 0x9 coresight device. */
> +static int amba_uci_match(const struct amba_id *table, struct amba_device *dev)
nit : Could we rename this function to make it explicit about the
"coresight" class  ? say, amba_cs_ucid_id_match() ?

> +{
> +	int ret = 0;
> +	struct amba_cs_uci_id *uci;
> +
> +	uci = table->data;
> +
> +	/* no table data - return match on periphid */
> +	if (!uci)
> +		return 1;
> +
> +	if (uci->devarch) {
> +		ret = (dev->uci.devtype == uci->devtype) &&
> +		       ((dev->uci.devarch & uci->mask) == uci->devarch);
> +	} else {
> +		/* devtype only if devarch set to 0 */
> +		ret = dev->uci.devtype == uci->devtype;
> +	}
> +	return ret;
> +}
> +
>   static const struct amba_id *
>   amba_lookup(const struct amba_id *table, struct amba_device *dev)
>   {
> @@ -33,11 +55,17 @@ amba_lookup(const struct amba_id *table, struct amba_device *dev)
>   
>   	while (table->mask) {
>   		ret = (dev->periphid & table->mask) == table->id;
> -		if (ret)
> -			break;
> +		/* matched on periphid - check UCI if CoreSight */
> +		if (ret) {
> +			if (dev->cid == CORESIGHT_CID)	{
> +				ret = amba_uci_match(table, dev);
> +				if (ret)
> +					break;
> +			} else
> +				break;
> +		}
>   		table++;

This code looks a bit too complex with the ifs and breaks. Could we
change it a bit to :

	/* For CoreSight class devices, match the UCI id */
	if ((dev->periphid & table->mask) == table->id) &&
	    ((dev->cid != CORESIGHT_CID) || amba_uci_match(table, dev)))
		return table;
	table++;

		
>   	}
> -
>   	return ret ? table : NULL;


>   }
>   
> @@ -399,10 +427,22 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
>   			cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) <<
>   				(i * 8);
>   
> +		if (cid == CORESIGHT_CID) {
> +			/* set the base to the start of the last 4k block */
> +			void __iomem *csbase = tmp + size - 4096;
> +
> +			dev->uci.devarch =
> +				readl(csbase + UCI_REG_DEVARCH_OFFSET);
> +			dev->uci.devtype =
> +				readl(csbase + UCI_REG_DEVTYPE_OFFSET) & 0xff;
> +		}
> +
>   		amba_put_disable_pclk(dev);
>   
> -		if (cid == AMBA_CID || cid == CORESIGHT_CID)
> +		if (cid == AMBA_CID || cid == CORESIGHT_CID) {
>   			dev->periphid = pid;
> +			dev->cid = cid;
> +		}
>   
>   		if (!dev->periphid)
>   			ret = -ENODEV;
> diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
> index d143c13bed26..a83a0a3dece8 100644
> --- a/include/linux/amba/bus.h
> +++ b/include/linux/amba/bus.h
> @@ -25,6 +25,36 @@
>   #define AMBA_CID	0xb105f00d
>   #define CORESIGHT_CID	0xb105900d
>   
> +/*
> + * CoreSight Architecture specification updates the ID specification
> + * for components on the AMBA bus. (ARM IHI 0029E)
> + *
> + * Bits 11:8 of the CID are the device class.
> + *
> + * Class 0xF remains for PrimeCell and legacy components. (AMBA_CID above)
> + * Class 0x9 defines the component as CoreSight (CORESIGHT_CID above)
> + * Class 0x0, 0x1, 0xB, 0xE define components that do not have driver support
> + * at present.
> + * Class 0x2-0x8,0xA and 0xD-0xD are presently reserved.
> + *
> + * Remaining CID bits stay as 0xb105-00d
> + */
> +
> +/*
> + * Class 0x9 components use additional values to form a Unique Component
> + * Identifier (UCI), where peripheral ID values are identical for different
> + * components. Passed to the amba bus code from the component driver via
> + * the amba_id->data pointer.
> + */
> +struct amba_cs_uci_id {
> +	unsigned int devarch;
> +	unsigned int mask;

It is worth changing the mask to reflect that it applies to the
"devarch" field, than being a generic mask which is unexplained.

> +	unsigned int devtype;
> +};
> +
> +#define UCI_REG_DEVTYPE_OFFSET	0xFCC
> +#define UCI_REG_DEVARCH_OFFSET	0xFBC
> +
>   struct clk;
>   
>   struct amba_device {
> @@ -32,6 +62,8 @@ struct amba_device {
>   	struct resource		res;
>   	struct clk		*pclk;
>   	unsigned int		periphid;
> +	unsigned int		cid;
> +	struct amba_cs_uci_id	uci;
>   	unsigned int		irq[AMBA_NR_IRQS];
>   	char			*driver_override;
>   };
> @@ -61,6 +93,7 @@ enum amba_vendor {
>   	(((conf) & 0xff) << 24 | ((rev) & 0xf) << 20 | \
>   	AMBA_VENDOR_LINUX << 12 | ((part) & 0xfff))
>   
> +

spurious change ?

>   extern struct bus_type amba_bustype;
>   
>   #define to_amba_device(d)	container_of(d, struct amba_device, dev)

Otherwise looks good to me.

Suzuki

[RFC PATCH 1/3] drivers: amba: Updates to component identification for driver matching.

[Mike Leach]

Hi Suzuki,

Thanks for the feedback.
Agree with all comments and will revise along the lines you suggest.

Mike

On Mon, 19 Nov 2018 at 14:54, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
> Hi Mike,
>
> On 11/15/2018 01:28 AM, Mike Leach wrote:
> > The CoreSight specification (ARM IHI 0029E), updates the ID register
> > requirements for components on an AMBA bus, to cover both traditional
> > ARM Primecell type devices, and newer CoreSight and other components.
> >
> > The Peripheral ID (PID) / Component ID (CID) pair is extended in certain
> > cases to uniquely identify components. CoreSight components related to
> > a single function can share Peripheral ID values, and must be further
> > identified using a Unique Component Identifier (UCI). e.g. the ETM, CTI,
> > PMU and Debug hardware of the A35 all share the same PID.
> >
> > Bits 11:8 of the CID are defined to be the device class.
> > Class 0xF remains for PrimeCell and legacy components.
> > Class 0x9 defines the component as CoreSight (CORESIGHT_CID above)
> > Class 0x0, 0x1, 0xB, 0xE define components that do not have driver support
> > at present.
> > Class 0x2-0x8,0xA and 0xD-0xD are presently reserved.
> >
> > The specification futher defines which classes of device use the standard
> > CID/PID pair, and when additional ID registers are required.
> >
> > The patches provide an update of amba_device and matching code to handle
> > the additional registers required for the Class 0x9 (CoreSight) UCI.
> > The *data pointer in the amba_id is used by the driver to provide extended
> > ID register values for matching.
> >
> > CoreSight components where PID/CID pair is currently sufficient for
> > unique identification need not provide this additional information.
> >
> > Signed-off-by: Mike Leach <mike.leach@linaro.org>
>
> The patch looks good to me. A couple of minor comments below.
>
> > ---
> >   drivers/amba/bus.c       | 48 ++++++++++++++++++++++++++++++++++++----
> >   include/linux/amba/bus.h | 33 +++++++++++++++++++++++++++
> >   2 files changed, 77 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> > index 41b706403ef7..6eab977f4314 100644
> > --- a/drivers/amba/bus.c
> > +++ b/drivers/amba/bus.c
> > @@ -26,6 +26,28 @@
> >
> >   #define to_amba_driver(d)   container_of(d, struct amba_driver, drv)
> >
> > +/* called on periphid match and class 0x9 coresight device. */
> > +static int amba_uci_match(const struct amba_id *table, struct amba_device *dev)
> nit : Could we rename this function to make it explicit about the
> "coresight" class  ? say, amba_cs_ucid_id_match() ?
>
> > +{
> > +     int ret = 0;
> > +     struct amba_cs_uci_id *uci;
> > +
> > +     uci = table->data;
> > +
> > +     /* no table data - return match on periphid */
> > +     if (!uci)
> > +             return 1;
> > +
> > +     if (uci->devarch) {
> > +             ret = (dev->uci.devtype == uci->devtype) &&
> > +                    ((dev->uci.devarch & uci->mask) == uci->devarch);
> > +     } else {
> > +             /* devtype only if devarch set to 0 */
> > +             ret = dev->uci.devtype == uci->devtype;
> > +     }
> > +     return ret;
> > +}
> > +
> >   static const struct amba_id *
> >   amba_lookup(const struct amba_id *table, struct amba_device *dev)
> >   {
> > @@ -33,11 +55,17 @@ amba_lookup(const struct amba_id *table, struct amba_device *dev)
> >
> >       while (table->mask) {
> >               ret = (dev->periphid & table->mask) == table->id;
> > -             if (ret)
> > -                     break;
> > +             /* matched on periphid - check UCI if CoreSight */
> > +             if (ret) {
> > +                     if (dev->cid == CORESIGHT_CID)  {
> > +                             ret = amba_uci_match(table, dev);
> > +                             if (ret)
> > +                                     break;
> > +                     } else
> > +                             break;
> > +             }
> >               table++;
>
> This code looks a bit too complex with the ifs and breaks. Could we
> change it a bit to :
>
>         /* For CoreSight class devices, match the UCI id */
>         if ((dev->periphid & table->mask) == table->id) &&
>             ((dev->cid != CORESIGHT_CID) || amba_uci_match(table, dev)))
>                 return table;
>         table++;
>
>
> >       }
> > -
> >       return ret ? table : NULL;
>
>
> >   }
> >
> > @@ -399,10 +427,22 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
> >                       cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) <<
> >                               (i * 8);
> >
> > +             if (cid == CORESIGHT_CID) {
> > +                     /* set the base to the start of the last 4k block */
> > +                     void __iomem *csbase = tmp + size - 4096;
> > +
> > +                     dev->uci.devarch =
> > +                             readl(csbase + UCI_REG_DEVARCH_OFFSET);
> > +                     dev->uci.devtype =
> > +                             readl(csbase + UCI_REG_DEVTYPE_OFFSET) & 0xff;
> > +             }
> > +
> >               amba_put_disable_pclk(dev);
> >
> > -             if (cid == AMBA_CID || cid == CORESIGHT_CID)
> > +             if (cid == AMBA_CID || cid == CORESIGHT_CID) {
> >                       dev->periphid = pid;
> > +                     dev->cid = cid;
> > +             }
> >
> >               if (!dev->periphid)
> >                       ret = -ENODEV;
> > diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
> > index d143c13bed26..a83a0a3dece8 100644
> > --- a/include/linux/amba/bus.h
> > +++ b/include/linux/amba/bus.h
> > @@ -25,6 +25,36 @@
> >   #define AMBA_CID    0xb105f00d
> >   #define CORESIGHT_CID       0xb105900d
> >
> > +/*
> > + * CoreSight Architecture specification updates the ID specification
> > + * for components on the AMBA bus. (ARM IHI 0029E)
> > + *
> > + * Bits 11:8 of the CID are the device class.
> > + *
> > + * Class 0xF remains for PrimeCell and legacy components. (AMBA_CID above)
> > + * Class 0x9 defines the component as CoreSight (CORESIGHT_CID above)
> > + * Class 0x0, 0x1, 0xB, 0xE define components that do not have driver support
> > + * at present.
> > + * Class 0x2-0x8,0xA and 0xD-0xD are presently reserved.
> > + *
> > + * Remaining CID bits stay as 0xb105-00d
> > + */
> > +
> > +/*
> > + * Class 0x9 components use additional values to form a Unique Component
> > + * Identifier (UCI), where peripheral ID values are identical for different
> > + * components. Passed to the amba bus code from the component driver via
> > + * the amba_id->data pointer.
> > + */
> > +struct amba_cs_uci_id {
> > +     unsigned int devarch;
> > +     unsigned int mask;
>
> It is worth changing the mask to reflect that it applies to the
> "devarch" field, than being a generic mask which is unexplained.
>
> > +     unsigned int devtype;
> > +};
> > +
> > +#define UCI_REG_DEVTYPE_OFFSET       0xFCC
> > +#define UCI_REG_DEVARCH_OFFSET       0xFBC
> > +
> >   struct clk;
> >
> >   struct amba_device {
> > @@ -32,6 +62,8 @@ struct amba_device {
> >       struct resource         res;
> >       struct clk              *pclk;
> >       unsigned int            periphid;
> > +     unsigned int            cid;
> > +     struct amba_cs_uci_id   uci;
> >       unsigned int            irq[AMBA_NR_IRQS];
> >       char                    *driver_override;
> >   };
> > @@ -61,6 +93,7 @@ enum amba_vendor {
> >       (((conf) & 0xff) << 24 | ((rev) & 0xf) << 20 | \
> >       AMBA_VENDOR_LINUX << 12 | ((part) & 0xfff))
> >
> > +
>
> spurious change ?
>
> >   extern struct bus_type amba_bustype;
> >
> >   #define to_amba_device(d)   container_of(d, struct amba_device, dev)
>
> Otherwise looks good to me.
>
> Suzuki



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

[RFC PATCH 1/3] drivers: amba: Updates to component identification for driver matching.

[Mathieu Poirier]

Hi Mike,

On Thu, Nov 15, 2018 at 01:28:40AM +0000, Mike Leach wrote:
> The CoreSight specification (ARM IHI 0029E), updates the ID register
> requirements for components on an AMBA bus, to cover both traditional
> ARM Primecell type devices, and newer CoreSight and other components.
> 
> The Peripheral ID (PID) / Component ID (CID) pair is extended in certain
> cases to uniquely identify components. CoreSight components related to
> a single function can share Peripheral ID values, and must be further
> identified using a Unique Component Identifier (UCI). e.g. the ETM, CTI,
> PMU and Debug hardware of the A35 all share the same PID.
> 
> Bits 11:8 of the CID are defined to be the device class.
> Class 0xF remains for PrimeCell and legacy components.
> Class 0x9 defines the component as CoreSight (CORESIGHT_CID above)
> Class 0x0, 0x1, 0xB, 0xE define components that do not have driver support
> at present.
> Class 0x2-0x8,0xA and 0xD-0xD are presently reserved.
> 
> The specification futher defines which classes of device use the standard
> CID/PID pair, and when additional ID registers are required.
> 
> The patches provide an update of amba_device and matching code to handle
> the additional registers required for the Class 0x9 (CoreSight) UCI.
> The *data pointer in the amba_id is used by the driver to provide extended
> ID register values for matching.
> 
> CoreSight components where PID/CID pair is currently sufficient for
> unique identification need not provide this additional information.
> 
> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> ---
>  drivers/amba/bus.c       | 48 ++++++++++++++++++++++++++++++++++++----
>  include/linux/amba/bus.h | 33 +++++++++++++++++++++++++++
>  2 files changed, 77 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 41b706403ef7..6eab977f4314 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -26,6 +26,28 @@
>  
>  #define to_amba_driver(d)	container_of(d, struct amba_driver, drv)
>  
> +/* called on periphid match and class 0x9 coresight device. */
> +static int amba_uci_match(const struct amba_id *table, struct amba_device *dev)
> +{
> +	int ret = 0;
> +	struct amba_cs_uci_id *uci;
> +
> +	uci = table->data;
> +
> +	/* no table data - return match on periphid */
> +	if (!uci)
> +		return 1;
> +
> +	if (uci->devarch) {
> +		ret = (dev->uci.devtype == uci->devtype) &&
> +		       ((dev->uci.devarch & uci->mask) == uci->devarch);
> +	} else {
> +		/* devtype only if devarch set to 0 */
> +		ret = dev->uci.devtype == uci->devtype;
> +	}
> +	return ret;
> +}
> +
>  static const struct amba_id *
>  amba_lookup(const struct amba_id *table, struct amba_device *dev)
>  {
> @@ -33,11 +55,17 @@ amba_lookup(const struct amba_id *table, struct amba_device *dev)
>  
>  	while (table->mask) {
>  		ret = (dev->periphid & table->mask) == table->id;
> -		if (ret)
> -			break;
> +		/* matched on periphid - check UCI if CoreSight */
> +		if (ret) {
> +			if (dev->cid == CORESIGHT_CID)	{
> +				ret = amba_uci_match(table, dev);
> +				if (ret)
> +					break;
> +			} else
> +				break;
> +		}
>  		table++;
>  	}
> -

Carefull with the line removal.

>  	return ret ? table : NULL;
>  }
>  
> @@ -399,10 +427,22 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
>  			cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) <<
>  				(i * 8);
>  
> +		if (cid == CORESIGHT_CID) {
> +			/* set the base to the start of the last 4k block */
> +			void __iomem *csbase = tmp + size - 4096;
> +
> +			dev->uci.devarch =
> +				readl(csbase + UCI_REG_DEVARCH_OFFSET);
> +			dev->uci.devtype =
> +				readl(csbase + UCI_REG_DEVTYPE_OFFSET) & 0xff;

Any reason for not masking @devarch the same way you're doing for @devtype?  If
we were to mask here we wouldn't have to carry the mask in struct amba_cs_uci_id
and deal with it in amba_uci_match().

Russell King maintains AMBA.  Please address your next revision to him and CC
the rest of us, as specified by get_maintainer.pl.

Thanks,
Mathieu

> +		}
> +
>  		amba_put_disable_pclk(dev);
>  
> -		if (cid == AMBA_CID || cid == CORESIGHT_CID)
> +		if (cid == AMBA_CID || cid == CORESIGHT_CID) {
>  			dev->periphid = pid;
> +			dev->cid = cid;
> +		}
>  
>  		if (!dev->periphid)
>  			ret = -ENODEV;
> diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
> index d143c13bed26..a83a0a3dece8 100644
> --- a/include/linux/amba/bus.h
> +++ b/include/linux/amba/bus.h
> @@ -25,6 +25,36 @@
>  #define AMBA_CID	0xb105f00d
>  #define CORESIGHT_CID	0xb105900d
>  
> +/*
> + * CoreSight Architecture specification updates the ID specification
> + * for components on the AMBA bus. (ARM IHI 0029E)
> + *
> + * Bits 11:8 of the CID are the device class.
> + *
> + * Class 0xF remains for PrimeCell and legacy components. (AMBA_CID above)
> + * Class 0x9 defines the component as CoreSight (CORESIGHT_CID above)
> + * Class 0x0, 0x1, 0xB, 0xE define components that do not have driver support
> + * at present.
> + * Class 0x2-0x8,0xA and 0xD-0xD are presently reserved.
> + *
> + * Remaining CID bits stay as 0xb105-00d
> + */
> +
> +/*
> + * Class 0x9 components use additional values to form a Unique Component
> + * Identifier (UCI), where peripheral ID values are identical for different
> + * components. Passed to the amba bus code from the component driver via
> + * the amba_id->data pointer.
> + */
> +struct amba_cs_uci_id {
> +	unsigned int devarch;
> +	unsigned int mask;
> +	unsigned int devtype;
> +};
> +
> +#define UCI_REG_DEVTYPE_OFFSET	0xFCC
> +#define UCI_REG_DEVARCH_OFFSET	0xFBC
> +
>  struct clk;
>  
>  struct amba_device {
> @@ -32,6 +62,8 @@ struct amba_device {
>  	struct resource		res;
>  	struct clk		*pclk;
>  	unsigned int		periphid;
> +	unsigned int		cid;
> +	struct amba_cs_uci_id	uci;
>  	unsigned int		irq[AMBA_NR_IRQS];
>  	char			*driver_override;
>  };
> @@ -61,6 +93,7 @@ enum amba_vendor {
>  	(((conf) & 0xff) << 24 | ((rev) & 0xf) << 20 | \
>  	AMBA_VENDOR_LINUX << 12 | ((part) & 0xfff))
>  
> +
>  extern struct bus_type amba_bustype;
>  
>  #define to_amba_device(d)	container_of(d, struct amba_device, dev)
> -- 
> 2.19.1
> 

[RFC PATCH 2/3] coresight: etmv4: Update ID register table to add UCI support

[Mathieu Poirier]

On Thu, Nov 15, 2018 at 01:28:41AM +0000, Mike Leach wrote:
> Updates the ID register tables to contain a UCI entry for the A35 ETM
> device to allow correct matching of driver in the amba bus code.
> 
> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> ---
>  drivers/hwtracing/coresight/coresight-etm4x.c | 26 +++++++++++++++----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
> index 53e2fb6e86f6..1dcb7e14ea6b 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
> @@ -1073,12 +1073,28 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id)
>  		.mask	= 0x000fffff,		\
>  	}
>  
> +static struct amba_cs_uci_id uci_id_etm4[] = {
> +	{
> +		/*  ETMv4 UCI data */
> +		.devarch	= 0x00004a13,
> +		.mask		= 0x0000ffff,
> +		.devtype	= 0x00000013,
> +	}
> +};
> +
> +#define ETM4x_AMBA_UCI_ID(pid)			\
> +	{					\
> +		.id	= pid,			\
> +		.mask	= 0x000fffff,		\
> +		.data	= uci_id_etm4,		\
> +	}
> +
>  static const struct amba_id etm4_ids[] = {
> -	ETM4x_AMBA_ID(0x000bb95d),		/* Cortex-A53 */
> -	ETM4x_AMBA_ID(0x000bb95e),		/* Cortex-A57 */
> -	ETM4x_AMBA_ID(0x000bb95a),		/* Cortex-A72 */
> -	ETM4x_AMBA_ID(0x000bb959),		/* Cortex-A73 */
> -	ETM4x_AMBA_ID(0x000bb9da),		/* Cortex-A35 */
> +	ETM4x_AMBA_ID(0x000bb95d),			/* Cortex-A53 */
> +	ETM4x_AMBA_ID(0x000bb95e),			/* Cortex-A57 */
> +	ETM4x_AMBA_ID(0x000bb95a),			/* Cortex-A72 */
> +	ETM4x_AMBA_ID(0x000bb959),			/* Cortex-A73 */

There is no need to add the extra tabulation for the above.

> +	ETM4x_AMBA_UCI_ID(0x000bb9da),			/* Cortex-A35 */
>  	{},
>  };
>  
> -- 
> 2.19.1
> 

[RFC PATCH 1/3] drivers: amba: Updates to component identification for driver matching.

[Mike Leach]

Hi Matthieu,

On Tue, 20 Nov 2018 at 20:57, Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
>
> Hi Mike,
>
> On Thu, Nov 15, 2018 at 01:28:40AM +0000, Mike Leach wrote:
> > The CoreSight specification (ARM IHI 0029E), updates the ID register
> > requirements for components on an AMBA bus, to cover both traditional
> > ARM Primecell type devices, and newer CoreSight and other components.
> >
> > The Peripheral ID (PID) / Component ID (CID) pair is extended in certain
> > cases to uniquely identify components. CoreSight components related to
> > a single function can share Peripheral ID values, and must be further
> > identified using a Unique Component Identifier (UCI). e.g. the ETM, CTI,
> > PMU and Debug hardware of the A35 all share the same PID.
> >
> > Bits 11:8 of the CID are defined to be the device class.
> > Class 0xF remains for PrimeCell and legacy components.
> > Class 0x9 defines the component as CoreSight (CORESIGHT_CID above)
> > Class 0x0, 0x1, 0xB, 0xE define components that do not have driver support
> > at present.
> > Class 0x2-0x8,0xA and 0xD-0xD are presently reserved.
> >
> > The specification futher defines which classes of device use the standard
> > CID/PID pair, and when additional ID registers are required.
> >
> > The patches provide an update of amba_device and matching code to handle
> > the additional registers required for the Class 0x9 (CoreSight) UCI.
> > The *data pointer in the amba_id is used by the driver to provide extended
> > ID register values for matching.
> >
> > CoreSight components where PID/CID pair is currently sufficient for
> > unique identification need not provide this additional information.
> >
> > Signed-off-by: Mike Leach <mike.leach@linaro.org>
> > ---
> >  drivers/amba/bus.c       | 48 ++++++++++++++++++++++++++++++++++++----
> >  include/linux/amba/bus.h | 33 +++++++++++++++++++++++++++
> >  2 files changed, 77 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> > index 41b706403ef7..6eab977f4314 100644
> > --- a/drivers/amba/bus.c
> > +++ b/drivers/amba/bus.c
> > @@ -26,6 +26,28 @@
> >
> >  #define to_amba_driver(d)    container_of(d, struct amba_driver, drv)
> >
> > +/* called on periphid match and class 0x9 coresight device. */
> > +static int amba_uci_match(const struct amba_id *table, struct amba_device *dev)
> > +{
> > +     int ret = 0;
> > +     struct amba_cs_uci_id *uci;
> > +
> > +     uci = table->data;
> > +
> > +     /* no table data - return match on periphid */
> > +     if (!uci)
> > +             return 1;
> > +
> > +     if (uci->devarch) {
> > +             ret = (dev->uci.devtype == uci->devtype) &&
> > +                    ((dev->uci.devarch & uci->mask) == uci->devarch);
> > +     } else {
> > +             /* devtype only if devarch set to 0 */
> > +             ret = dev->uci.devtype == uci->devtype;
> > +     }
> > +     return ret;
> > +}
> > +
> >  static const struct amba_id *
> >  amba_lookup(const struct amba_id *table, struct amba_device *dev)
> >  {
> > @@ -33,11 +55,17 @@ amba_lookup(const struct amba_id *table, struct amba_device *dev)
> >
> >       while (table->mask) {
> >               ret = (dev->periphid & table->mask) == table->id;
> > -             if (ret)
> > -                     break;
> > +             /* matched on periphid - check UCI if CoreSight */
> > +             if (ret) {
> > +                     if (dev->cid == CORESIGHT_CID)  {
> > +                             ret = amba_uci_match(table, dev);
> > +                             if (ret)
> > +                                     break;
> > +                     } else
> > +                             break;
> > +             }
> >               table++;
> >       }
> > -
>
> Carefull with the line removal.
>

This function is significantly re-written in the next version per
Suzuki's suggestions.

> >       return ret ? table : NULL;
> >  }
> >
> > @@ -399,10 +427,22 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
> >                       cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) <<
> >                               (i * 8);
> >
> > +             if (cid == CORESIGHT_CID) {
> > +                     /* set the base to the start of the last 4k block */
> > +                     void __iomem *csbase = tmp + size - 4096;
> > +
> > +                     dev->uci.devarch =
> > +                             readl(csbase + UCI_REG_DEVARCH_OFFSET);
> > +                     dev->uci.devtype =
> > +                             readl(csbase + UCI_REG_DEVTYPE_OFFSET) & 0xff;
>
> Any reason for not masking @devarch the same way you're doing for @devtype?  If
> we were to mask here we wouldn't have to carry the mask in struct amba_cs_uci_id
> and deal with it in amba_uci_match().
>

@devarch is defined as a 32 bit value. This is masked to widen /
narrow matching. By default the mask will be the ARCHID field - bits
15:0, selected in the driver in the same way as the drivers currently
have a mask for peripheral ID. This ignores the architect and version
fields as most CS component drivers should handle differences.

However - this is also an optional register - if all 32 bits are 0,
then we ignore and use just the @devtype. This logic is contained in
amba_uci_match()

@devtype is architecturally specified as having only 8 significant
bits, thus is masked at this point.

> Russell King maintains AMBA.  Please address your next revision to him and CC
> the rest of us, as specified by get_maintainer.pl.
>

get_maintainer.pl returned no specific 'maintainer' for this patch -
though looking again I do see Russell noted as 'odd fixer'.
Will ensure this is directed to him next revision.

Thanks for the feedback,

Mike


> Thanks,
> Mathieu
>
> > +             }
> > +
> >               amba_put_disable_pclk(dev);
> >
> > -             if (cid == AMBA_CID || cid == CORESIGHT_CID)
> > +             if (cid == AMBA_CID || cid == CORESIGHT_CID) {
> >                       dev->periphid = pid;
> > +                     dev->cid = cid;
> > +             }
> >
> >               if (!dev->periphid)
> >                       ret = -ENODEV;
> > diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
> > index d143c13bed26..a83a0a3dece8 100644
> > --- a/include/linux/amba/bus.h
> > +++ b/include/linux/amba/bus.h
> > @@ -25,6 +25,36 @@
> >  #define AMBA_CID     0xb105f00d
> >  #define CORESIGHT_CID        0xb105900d
> >
> > +/*
> > + * CoreSight Architecture specification updates the ID specification
> > + * for components on the AMBA bus. (ARM IHI 0029E)
> > + *
> > + * Bits 11:8 of the CID are the device class.
> > + *
> > + * Class 0xF remains for PrimeCell and legacy components. (AMBA_CID above)
> > + * Class 0x9 defines the component as CoreSight (CORESIGHT_CID above)
> > + * Class 0x0, 0x1, 0xB, 0xE define components that do not have driver support
> > + * at present.
> > + * Class 0x2-0x8,0xA and 0xD-0xD are presently reserved.
> > + *
> > + * Remaining CID bits stay as 0xb105-00d
> > + */
> > +
> > +/*
> > + * Class 0x9 components use additional values to form a Unique Component
> > + * Identifier (UCI), where peripheral ID values are identical for different
> > + * components. Passed to the amba bus code from the component driver via
> > + * the amba_id->data pointer.
> > + */
> > +struct amba_cs_uci_id {
> > +     unsigned int devarch;
> > +     unsigned int mask;
> > +     unsigned int devtype;
> > +};
> > +
> > +#define UCI_REG_DEVTYPE_OFFSET       0xFCC
> > +#define UCI_REG_DEVARCH_OFFSET       0xFBC
> > +
> >  struct clk;
> >
> >  struct amba_device {
> > @@ -32,6 +62,8 @@ struct amba_device {
> >       struct resource         res;
> >       struct clk              *pclk;
> >       unsigned int            periphid;
> > +     unsigned int            cid;
> > +     struct amba_cs_uci_id   uci;
> >       unsigned int            irq[AMBA_NR_IRQS];
> >       char                    *driver_override;
> >  };
> > @@ -61,6 +93,7 @@ enum amba_vendor {
> >       (((conf) & 0xff) << 24 | ((rev) & 0xf) << 20 | \
> >       AMBA_VENDOR_LINUX << 12 | ((part) & 0xfff))
> >
> > +
> >  extern struct bus_type amba_bustype;
> >
> >  #define to_amba_device(d)    container_of(d, struct amba_device, dev)
> > --
> > 2.19.1
> >



--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

[RFC PATCH 1/3] drivers: amba: Updates to component identification for driver matching.

[Al Grant]

> @devarch is defined as a 32 bit value. This is masked to widen / narrow
> matching. By default the mask will be the ARCHID field - bits 15:0, selected in the
> driver in the same way as the drivers currently have a mask for peripheral ID.
> This ignores the architect and version fields as most CS component drivers
> should handle differences.

The architect field defines what the ARCHID field means (the ARCHID
values are specific to each architect), it's not the same as the implementer
field. You need to check both if you want to know what component driver
to use. The correct mask for that is 0xfff0ffff.

Selecting on ARCHID alone, would only be valid if you are requiring the
architect always to be ARM (0x23b) and have already checked that.

Al
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[RFC PATCH 1/3] drivers: amba: Updates to component identification for driver matching.

[Mike Leach]

Hi Al,

On Wed, 21 Nov 2018 at 14:25, Al Grant <Al.Grant@arm.com> wrote:
>
> > @devarch is defined as a 32 bit value. This is masked to widen / narrow
> > matching. By default the mask will be the ARCHID field - bits 15:0, selected in the
> > driver in the same way as the drivers currently have a mask for peripheral ID.
> > This ignores the architect and version fields as most CS component drivers
> > should handle differences.
>
> The architect field defines what the ARCHID field means (the ARCHID
> values are specific to each architect), it's not the same as the implementer
> field. You need to check both if you want to know what component driver
> to use. The correct mask for that is 0xfff0ffff.
>
> Selecting on ARCHID alone, would only be valid if you are requiring the
> architect always to be ARM (0x23b) and have already checked that.
>

This is a fair point - thought at present the peripheral ID matching
takes in the JDEC ID code of the component designer - in the ETM case
ARM (though without a JDEC continuation code check).
Given this field too is specified as "designer not implementer", it
seems logically unlikely that designer != architect.

If we did feel the need to alter the match value / mask values - this
happens in the CS driver code, not in this patch - so will need to be
addressed there.

Thanks

Mike

> Al
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

[RFC PATCH 2/3] coresight: etmv4: Update ID register table to add UCI support

[Mike Leach]

Hi Mathieu
On Tue, 20 Nov 2018 at 20:59, Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
>
> On Thu, Nov 15, 2018 at 01:28:41AM +0000, Mike Leach wrote:
> > Updates the ID register tables to contain a UCI entry for the A35 ETM
> > device to allow correct matching of driver in the amba bus code.
> >
> > Signed-off-by: Mike Leach <mike.leach@linaro.org>
> > ---
> >  drivers/hwtracing/coresight/coresight-etm4x.c | 26 +++++++++++++++----
> >  1 file changed, 21 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
> > index 53e2fb6e86f6..1dcb7e14ea6b 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
> > @@ -1073,12 +1073,28 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id)
> >               .mask   = 0x000fffff,           \
> >       }
> >
> > +static struct amba_cs_uci_id uci_id_etm4[] = {
> > +     {
> > +             /*  ETMv4 UCI data */
> > +             .devarch        = 0x00004a13,
> > +             .mask           = 0x0000ffff,
> > +             .devtype        = 0x00000013,
> > +     }
> > +};
> > +
> > +#define ETM4x_AMBA_UCI_ID(pid)                       \
> > +     {                                       \
> > +             .id     = pid,                  \
> > +             .mask   = 0x000fffff,           \
> > +             .data   = uci_id_etm4,          \
> > +     }
> > +
> >  static const struct amba_id etm4_ids[] = {
> > -     ETM4x_AMBA_ID(0x000bb95d),              /* Cortex-A53 */
> > -     ETM4x_AMBA_ID(0x000bb95e),              /* Cortex-A57 */
> > -     ETM4x_AMBA_ID(0x000bb95a),              /* Cortex-A72 */
> > -     ETM4x_AMBA_ID(0x000bb959),              /* Cortex-A73 */
> > -     ETM4x_AMBA_ID(0x000bb9da),              /* Cortex-A35 */
> > +     ETM4x_AMBA_ID(0x000bb95d),                      /* Cortex-A53 */
> > +     ETM4x_AMBA_ID(0x000bb95e),                      /* Cortex-A57 */
> > +     ETM4x_AMBA_ID(0x000bb95a),                      /* Cortex-A72 */
> > +     ETM4x_AMBA_ID(0x000bb959),                      /* Cortex-A73 */
>
> There is no need to add the extra tabulation for the above.
>

sorry - unnecessary churn - will fix.

Mike


> > +     ETM4x_AMBA_UCI_ID(0x000bb9da),                  /* Cortex-A35 */
> >       {},
> >  };
> >
> > --
> > 2.19.1
> >



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

[RFC PATCH 1/3] drivers: amba: Updates to component identification for driver matching.

[Al Grant]

> > The architect field defines what the ARCHID field means (the ARCHID
> > values are specific to each architect), it's not the same as the
> > implementer field. You need to check both if you want to know what
> > component driver to use. The correct mask for that is 0xfff0ffff.
>
> This is a fair point - thought at present the peripheral ID matching takes in the
> JDEC ID code of the component designer - in the ETM case ARM (though without
> a JDEC continuation code check).
> Given this field too is specified as "designer not implementer", it seems logically
> unlikely that designer != architect.

It might be exactly like that. On the system I'm using right now:

  DEVARCH=0x47714a13  meaning architect=ARM architecture=ETM
  PIDR= <somebody else>

This is not a Cortex core designed by ARM, it's someone else's ARM
compatible core and they designed their own ARM-compatible ETM.
It conforms to the ETM architecture defined by ARM, so they set
DEVARCH to indicate that (architect=ARM), but in the implementer/
designer code in PIDR they put their own JEDEC code.

If you've got DEVARCH, that should be all you need to select the right
driver and then the driver should be able to get everything it needs
from the feature ID regs. It should only need to look at implementer
code / product code as a last resort for errata workarounds or similar
(e.g. if the feature ID regs are known to be incorrect).

If you haven't got DEVARCH, you have to look at implementer code /
product code. So for example if we see a componet with no DEVARCH
but with designer=ARM product=0x961, that's "ARM product 0x961"
i.e. a TMC, and we can write the driver according to the programming
interface described in the manual for that product.

Al



>
> If we did feel the need to alter the match value / mask values - this happens in
> the CS driver code, not in this patch - so will need to be addressed there.
>
> Thanks
>
> Mike
>
> > Al
> > IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended recipient,
> please notify the sender immediately and do not disclose the contents to any
> other person, use it for any purpose, or store or copy the information in any
> medium. Thank you.
>
>
>
> --
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.