perf: arm64: Support for Hisilicon SoC Hardware event counters

From: Tan Xiaojun <[email protected]>

1) Add Hisilicon HiP05/06/07 CPU and ALGSUB system controller dts bindings.
2) Add Hisilicon Djtag dts binding.

Signed-off-by Tan Xiaojun <[email protected]>

Signed-off-by Anurup M <[email protected]>

---
 .../bindings/arm/hisilicon/hisilicon.txt           | 82 ++++++++++++++++++++++
 1 file changed, 82 insertions(+)

Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +0 -0

index 7df79a7..341cbb9 100644
@@ -270,3 +270,85 @@ Required Properties:
   [1]: bootwrapper size

+- reg : Register address and size
+- djtag :

Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +18 -0

+  - compatible : "hisilicon,hip06-cpu-djtag-v1"
+  - reg : Register address and size
+
+Hisilicon HiP07 djtag for CPU sysctrl
+Required properties:
+  - compatible : "hisilicon,hip07-cpu-djtag-v2"
+  - reg : Register address and size
+
+Example:
+	/* for Hisilicon HiP05 djtag for CPU sysctrl */
+	djtag0: djtag at 80010000 {
+		compatible = "hisilicon,hip05-cpu-djtag-v1";
+		reg = <0x0 0x80010000 0x0 0x10000>;
+
+		/* For L3 cache PMU */
+		pmul3c0 {
+			compatible = "hisilicon,hisi-pmu-l3c-v1";
+			scl-id = <0x02>;

Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +21 -0

+Hisilicon HiP05 djtag for ALGSUB sysctrl
+Required properties:
+  - compatible : "hisilicon,hip05-io-djtag-v1"
+  - reg : Register address and size
+
+Hisilicon HiP06 djtag for ALGSUB sysctrl
+Required properties:
+  - compatible : "hisilicon,hip06-io-djtag-v2"
+  - reg : Register address and size
+
+Hisilicon HiP07 djtag for ALGSUB sysctrl
+Required properties:
+  - compatible : "hisilicon,hip07-io-djtag-v2"
+  - reg : Register address and size
+
+Example:
+	/* for Hisilicon HiP05 djtag for alg sysctrl */
+	djtag0: djtag at d0000000 {
+	       compatible = "hisilicon,hip05-io-djtag-v1";
+		reg = <0x0 0xd0000000 0x0 0x10000>;
+	};

Signature · 1 line

2.1.4

Hi,

Quoted 17 lines · Anurup M

From: Tan Xiaojun <[email protected]>

1) Add Hisilicon HiP05/06/07 CPU and ALGSUB system controller dts bindings.
2) Add Hisilicon Djtag dts binding.

Signed-off-by Tan Xiaojun <[email protected]>

Signed-off-by Anurup M <[email protected]>

---
  .../bindings/arm/hisilicon/hisilicon.txt           | 82 ++++++++++++++++++++++
  1 file changed, 82 insertions(+)

Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +0 -0

index 7df79a7..341cbb9 100644

Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt Required Properties: · line 279

Quoted 10 lines

Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +6 -0

@@ -270,3 +270,85 @@ Required Properties:
    [1]: bootwrapper size
    [2]: relocation physical address
    [3]: relocation size
 
+-----------------------------------------------------------------------
+The Hisilicon Djtag in CPU die is an independent component which connects with
+some other components in the SoC by Debug Bus. This driver can be configured
+to access the registers of connecting components (like L3 cache, l3 cache PMU
+ etc.) during real time debugging by sysctrl. These components appear as child
+nodes of djtag.

Please put the djtag binding in a new file.

It's clearly unrelated to many other things in this file, which should also be split out.

Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt Required Properties: · line 284

Quoted 4 lines

Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +4 -0

+The Hip05/06/07 CPU system controller(sysctrl) support to manage some important
+components (such as clock, reset, soft reset, secure debugger, etc.).
+The CPU sysctrl registers in hip05/06/07 doesnot use syscon but will be mapped
+by djtag driver for use by connecting components.

The djtag driver is irrelvant here.

If there's a realtionship between the two, please define that in the binding rather than implicitly assuming it in the driver.

Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt Required Properties: · line 293

Quoted 8 lines

Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +8 -0

+
+Required properties:
+  - compatible : "hisilicon,hip05-cpu-djtag-v1"
+  - reg : Register address and size
+
+Hisilicon HiP06 djtag for CPU sysctrl
+Required properties:
+- compatible : "hisilicon,hip06-sysctrl", "syscon", "simple-mfd";

This looks messy. Why is this syscon and a simple-mfd?

We should kill off / deprecate the syscon binding. It's completely meaningless.

Quoted 20 lines

+- reg : Register address and size
+- djtag :

Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +18 -0

+  - compatible : "hisilicon,hip06-cpu-djtag-v1"
+  - reg : Register address and size
+
+Hisilicon HiP07 djtag for CPU sysctrl
+Required properties:
+  - compatible : "hisilicon,hip07-cpu-djtag-v2"
+  - reg : Register address and size
+
+Example:
+	/* for Hisilicon HiP05 djtag for CPU sysctrl */
+	djtag0: djtag at 80010000 {
+		compatible = "hisilicon,hip05-cpu-djtag-v1";
+		reg = <0x0 0x80010000 0x0 0x10000>;
+
+		/* For L3 cache PMU */
+		pmul3c0 {
+			compatible = "hisilicon,hisi-pmu-l3c-v1";
+			scl-id = <0x02>;

Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt Required Properties: · line 323

Quoted 10 lines

Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +10 -0

+			num-events = <0x16>;
+			num-counters = <0x08>;
+			module-id = <0x04>;
+			num-banks = <0x04>;
+			cfgen-map = <0x02 0x04 0x01 0x08>;
+			counter-reg = <0x170>;
+			evctrl-reg = <0x04>;
+			event-en = <0x1000000>;
+			evtype-reg = <0x140>;
+		};

This sub-node needs a binding document.

These properties are completely opaque

Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt Required Properties: · line 332

Quoted 9 lines

Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +9 -0

+	};
+
+-----------------------------------------------------------------------
+The Hisilicon HiP05/06/07 ALGSUB system controller(sysctrl) is in IO die
+of SoC. It has a similar function as the Hisilicon HiP05/06/07 CPU system
+controller in CPU die and it manage different components, like RSA, etc.
+The Hisilicon Djtag in IO die has a similar function as in CPU die and maps
+the sysctrl registers for use by connecting components.
+All connecting components shall appear as child nodes of djtag.

I don't follow the above. This describes an ALGSUB system controllerm but the documentation below is all about djtag.

Thanks,
Mark.

Quoted 24 lines

Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +21 -0

+Hisilicon HiP05 djtag for ALGSUB sysctrl
+Required properties:
+  - compatible : "hisilicon,hip05-io-djtag-v1"
+  - reg : Register address and size
+
+Hisilicon HiP06 djtag for ALGSUB sysctrl
+Required properties:
+  - compatible : "hisilicon,hip06-io-djtag-v2"
+  - reg : Register address and size
+
+Hisilicon HiP07 djtag for ALGSUB sysctrl
+Required properties:
+  - compatible : "hisilicon,hip07-io-djtag-v2"
+  - reg : Register address and size
+
+Example:
+	/* for Hisilicon HiP05 djtag for alg sysctrl */
+	djtag0: djtag at d0000000 {
+	       compatible = "hisilicon,hip05-io-djtag-v1";
+		reg = <0x0 0xd0000000 0x0 0x10000>;
+	};

Signature · 1 line

2.1.4

Quoted 26 lines · Mark Rutland

Quoted 12 lines · Anurup M

Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +10 -0

 
+Example:
+	/* for Hisilicon HiP05 djtag for CPU sysctrl */
+	djtag0: djtag at 80010000 {
+		compatible = "hisilicon,hip05-cpu-djtag-v1";
+		reg = <0x0 0x80010000 0x0 0x10000>;
+
+		/* For L3 cache PMU */
+		pmul3c0 {
+			compatible = "hisilicon,hisi-pmu-l3c-v1";
+			scl-id = <0x02>;

Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt Required Properties: · line 323

Quoted 10 lines

Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +10 -0

+			num-events = <0x16>;
+			num-counters = <0x08>;
+			module-id = <0x04>;
+			num-banks = <0x04>;
+			cfgen-map = <0x02 0x04 0x01 0x08>;
+			counter-reg = <0x170>;
+			evctrl-reg = <0x04>;
+			event-en = <0x1000000>;
+			evtype-reg = <0x140>;
+		};

This sub-node needs a binding document.

Quoted 1 line

These properties are completely opaque

The L3 cache PMU bindings are defined @bindings/arm/hisilicon/pmu.txt. Is it OK that I document here(hisilicon/djtag.txt), a reference to the PMU bindings doc ?

Quoted 26 lines · Anurup M

Quoted 25 lines · Mark Rutland

Quoted 11 lines · Anurup M

Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +10 -0

+Example:
+	/* for Hisilicon HiP05 djtag for CPU sysctrl */
+	djtag0: djtag at 80010000 {
+		compatible = "hisilicon,hip05-cpu-djtag-v1";
+		reg = <0x0 0x80010000 0x0 0x10000>;
+
+		/* For L3 cache PMU */
+		pmul3c0 {
+			compatible = "hisilicon,hisi-pmu-l3c-v1";
+			scl-id = <0x02>;

Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt Required Properties: · line 323

Quoted 10 lines

Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +10 -0

+			num-events = <0x16>;
+			num-counters = <0x08>;
+			module-id = <0x04>;
+			num-banks = <0x04>;
+			cfgen-map = <0x02 0x04 0x01 0x08>;
+			counter-reg = <0x170>;
+			evctrl-reg = <0x04>;
+			event-en = <0x1000000>;
+			evtype-reg = <0x140>;
+		};

This sub-node needs a binding document.

These properties are completely opaque

Quoted 3 lines

The L3 cache PMU bindings are defined @bindings/arm/hisilicon/pmu.txt. Is it OK that I document here(hisilicon/djtag.txt), a reference to the PMU bindings doc ?

At this point in the series, that file does not exist yet, and this is an undocumented beinding.

Please introduce this sub-node long with the PMU bindings, later in the series.

Thanks,
Mark.

From: Tan Xiaojun <[email protected]>

The Hisilicon Djtag is an independent component which connects with some other components in the SoC by Debug Bus. This driver can be configured to access the registers of connecting components (like L3 cache) during real time debugging.

Signed-off-by Tan Xiaojun <[email protected]>

Signed-off-by John Garry <[email protected]>

Signed-off-by Anurup M <[email protected]>

---
 drivers/soc/Kconfig                 |   1 +
 drivers/soc/Makefile                |   1 +
 drivers/soc/hisilicon/Kconfig       |  12 +
 drivers/soc/hisilicon/Makefile      |   1 +
 drivers/soc/hisilicon/djtag.c       | 639 ++++++++++++++++++++++++++++++++++++
 include/linux/soc/hisilicon/djtag.h |  38 +++
 6 files changed, 692 insertions(+)
 create mode 100644 drivers/soc/hisilicon/Kconfig
 create mode 100644 drivers/soc/hisilicon/Makefile
 create mode 100644 drivers/soc/hisilicon/djtag.c
 create mode 100644 include/linux/soc/hisilicon/djtag.h

+#define SC_DJTAG_TIMEOUT 100000 /* 100ms */

drivers/soc/hisilicon/djtag.c new file +56 -0

+
+/*
+ * djtag_readwrite_v1/v2: djtag read/write interface
+ * @reg_base:	djtag register base address
+ * @offset:	register's offset
+ * @mod_sel:	module selection
+ * @mod_mask:	mask to select specific modules for write
+ * @is_w:	write -> true, read -> false
+ * @wval:	value to register for write
+ * @chain_id:	which sub module for read
+ * @rval:	value in register for read
+ *
+ * Return non-zero if error, else return 0.
+ */
+static int djtag_readwrite_v1(void __iomem *regs_base, u32 offset, u32 mod_sel,
+		u32 mod_mask, bool is_w, u32 wval, int chain_id, u32 *rval)
+{
+	u32 rd;
+	int timeout = SC_DJTAG_TIMEOUT;
+
+	if (!(mod_mask & CHAIN_UNIT_CFG_EN)) {
+		pr_warn("djtag: do nothing.\n");
+		return 0;
+	}
+
+	/* djtag mster enable & accelerate R,W */
+	djtag_write32(regs_base, SC_DJTAG_MSTR_EN,
+			DJTAG_NOR_CFG | DJTAG_MSTR_EN);
+
+	/* select module */
+	djtag_write32(regs_base, SC_DJTAG_DEBUG_MODULE_SEL, mod_sel);
+	djtag_write32(regs_base, SC_DJTAG_CHAIN_UNIT_CFG_EN,
+				mod_mask & CHAIN_UNIT_CFG_EN);
+
+	if (is_w) {
+		djtag_write32(regs_base, SC_DJTAG_MSTR_WR, DJTAG_MSTR_W);
+		djtag_write32(regs_base, SC_DJTAG_MSTR_DATA, wval);
+	} else
+		djtag_write32(regs_base, SC_DJTAG_MSTR_WR, DJTAG_MSTR_R);
+
+	/* address offset */
+	djtag_write32(regs_base, SC_DJTAG_MSTR_ADDR, offset);
+
+	/* start to write to djtag register */
+	djtag_write32(regs_base, SC_DJTAG_MSTR_START_EN, DJTAG_MSTR_START_EN);
+
+	/* ensure the djtag operation is done */
+	do {
+		djtag_read32_relaxed(regs_base, SC_DJTAG_MSTR_START_EN, &rd);
+		if (!(rd & DJTAG_MSTR_EN))
+			break;
+
+		udelay(1);
+	} while (timeout--);
+
+	if (timeout < 0) {

drivers/soc/hisilicon/djtag.c new file +11 -0

+static const struct of_device_id djtag_of_match[] = {
+	/* for hip05(D02) cpu die */
+	{ .compatible = "hisilicon,hip05-cpu-djtag-v1",
+		.data = (void *)djtag_readwrite_v1 },
+	/* for hip05(D02) io die */
+	{ .compatible = "hisilicon,hip05-io-djtag-v1",
+		.data = (void *)djtag_readwrite_v1 },
+	/* for hip06(D03) cpu die */
+	{ .compatible = "hisilicon,hip06-cpu-djtag-v1",
+		.data = (void *)djtag_readwrite_v1 },
+	/* for hip06(D03) io die */

drivers/soc/hisilicon/djtag.c new file +8 -0

+MODULE_DEVICE_TABLE(of, djtag_of_match);
+
+static ssize_t
+show_modalias(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct hisi_djtag_client *client = to_hisi_djtag_client(dev);
+
+	return sprintf(buf, "%s%s\n", MODULE_PREFIX, client->name);

drivers/soc/hisilicon/djtag.c new file +6 -0

+struct hisi_djtag_client *hisi_djtag_new_device(struct hisi_djtag_host *host,
+						struct device_node *node)
+{
+	struct hisi_djtag_client *client;
+	int status;
+

drivers/soc/hisilicon/djtag.c new file +15 -0

+	snprintf(client->name, DJTAG_CLIENT_NAME_LEN, "%s%s",
+					DJTAG_PREFIX, node->name);
+	dev_set_name(&client->dev, "%s", client->name);
+
+	status = device_register(&client->dev);
+	if (status < 0) {
+		pr_err("error adding new device, status=%d\n", status);
+		kfree(client);
+		return NULL;
+	}
+
+	return client;
+}
+
+static struct hisi_djtag_client *hisi_djtag_of_register_device(

drivers/soc/hisilicon/djtag.c new file +11 -0

+		return ERR_PTR(-EINVAL);
+	}
+
+	return client;
+}
+
+static void djtag_register_devices(struct hisi_djtag_host *host)
+{
+	struct device_node *node;
+	struct hisi_djtag_client *client;
+

drivers/soc/hisilicon/djtag.c new file +7 -0

+static int djtag_host_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct hisi_djtag_host *host;
+	const struct of_device_id *of_id;
+	struct resource *res;
+	int rc;

drivers/soc/hisilicon/djtag.c new file +14 -0

+	host->djtag_readwrite = of_id->data;
+	spin_lock_init(&host->lock);
+
+	INIT_LIST_HEAD(&host->client_list);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "No reg resorces!\n");
+		kfree(host);
+		return -EINVAL;
+	}
+
+	if (!resource_size(res)) {
+		dev_err(&pdev->dev, "Zero reg entry!\n");

Quoted 7 lines · Anurup M

From: Tan Xiaojun <[email protected]>

The Hisilicon Djtag is an independent component which connects with some other components in the SoC by Debug Bus. This driver can be configured to access the registers of connecting components (like L3 cache) during real time debugging.

Just to check, is this likely to be used in multi-socket hardware, and if so, are instances always-on?

Quoted 11 lines

Signed-off-by Tan Xiaojun <[email protected]>

Signed-off-by John Garry <[email protected]>

Signed-off-by Anurup M <[email protected]>

---
  drivers/soc/Kconfig                 |   1 +
  drivers/soc/Makefile                |   1 +
  drivers/soc/hisilicon/Kconfig       |  12 +
  drivers/soc/hisilicon/Makefile      |   1 +
  drivers/soc/hisilicon/djtag.c       | 639 ++++++++++++++++++++++++++++++++++++
  include/linux/soc/hisilicon/djtag.h |  38 +++
  6 files changed, 692 insertions(+)

Quoted 4 lines

create mode 100644 drivers/soc/hisilicon/Kconfig create mode 100644 drivers/soc/hisilicon/Makefile create mode 100644 drivers/soc/hisilicon/djtag.c create mode 100644 include/linux/soc/hisilicon/djtag.h

Other than the PMU driver(s), what is going to use this?

If you don't have something in particular, please also place this under drivers/perf/hisilicon, along with the PMU driver(s).

We can always move it later if necessary.

[...]

Quoted 1 line

+#define SC_DJTAG_TIMEOUT 100000 /* 100ms */

This would be better as:

#define SC_DJTAG_TIMEOUT_US (100 * USEC_PER_MSEC)

(you'll need to include <linux/time64.h>)

[...]

drivers/soc/hisilicon/djtag.c line 93

Quoted 13 lines

drivers/soc/hisilicon/djtag.c new file +13 -0

+static void djtag_read32_relaxed(void __iomem *regs_base, u32 off, u32 *value)
+{
+	void __iomem *reg_addr = regs_base + off;
+
+	*value = readl_relaxed(reg_addr);
+}
+
+static void djtag_write32(void __iomem *regs_base, u32 off, u32 val)
+{
+	void __iomem *reg_addr = regs_base + off;
+
+	writel(val, reg_addr);
+}

I think these make the driver harder to read, especially given the read function is void and takes an output pointer.

In either case you can call readl/writel directly with base + off for the __iomem ptr. Please do that.

Quoted 61 lines

drivers/soc/hisilicon/djtag.c new file +61 -0

+
+/*
+ * djtag_readwrite_v1/v2: djtag read/write interface
+ * @reg_base:	djtag register base address
+ * @offset:	register's offset
+ * @mod_sel:	module selection
+ * @mod_mask:	mask to select specific modules for write
+ * @is_w:	write -> true, read -> false
+ * @wval:	value to register for write
+ * @chain_id:	which sub module for read
+ * @rval:	value in register for read
+ *
+ * Return non-zero if error, else return 0.
+ */
+static int djtag_readwrite_v1(void __iomem *regs_base, u32 offset, u32 mod_sel,
+		u32 mod_mask, bool is_w, u32 wval, int chain_id, u32 *rval)
+{
+	u32 rd;
+	int timeout = SC_DJTAG_TIMEOUT;
+
+	if (!(mod_mask & CHAIN_UNIT_CFG_EN)) {
+		pr_warn("djtag: do nothing.\n");
+		return 0;
+	}
+
+	/* djtag mster enable & accelerate R,W */
+	djtag_write32(regs_base, SC_DJTAG_MSTR_EN,
+			DJTAG_NOR_CFG | DJTAG_MSTR_EN);
+
+	/* select module */
+	djtag_write32(regs_base, SC_DJTAG_DEBUG_MODULE_SEL, mod_sel);
+	djtag_write32(regs_base, SC_DJTAG_CHAIN_UNIT_CFG_EN,
+				mod_mask & CHAIN_UNIT_CFG_EN);
+
+	if (is_w) {
+		djtag_write32(regs_base, SC_DJTAG_MSTR_WR, DJTAG_MSTR_W);
+		djtag_write32(regs_base, SC_DJTAG_MSTR_DATA, wval);
+	} else
+		djtag_write32(regs_base, SC_DJTAG_MSTR_WR, DJTAG_MSTR_R);
+
+	/* address offset */
+	djtag_write32(regs_base, SC_DJTAG_MSTR_ADDR, offset);
+
+	/* start to write to djtag register */
+	djtag_write32(regs_base, SC_DJTAG_MSTR_START_EN, DJTAG_MSTR_START_EN);
+
+	/* ensure the djtag operation is done */
+	do {
+		djtag_read32_relaxed(regs_base, SC_DJTAG_MSTR_START_EN, &rd);
+		if (!(rd & DJTAG_MSTR_EN))
+			break;
+
+		udelay(1);
+	} while (timeout--);
+
+	if (timeout < 0) {
+		pr_err("djtag: %s timeout!\n", is_w ? "write" : "read");
+		return -EBUSY;
+	}
+
+	if (!is_w)

drivers/soc/hisilicon/djtag.c line 159

Quoted 5 lines

drivers/soc/hisilicon/djtag.c new file +5 -0

+		djtag_read32_relaxed(regs_base,
+			SC_DJTAG_RD_DATA_BASE + chain_id * 0x4, rval);
+
+	return 0;
+}

Please factor out the common bits into helpers and have separate read/write functions. It's incredibly difficult to follow the code with read/write hidden behind a boolean parameter.

Quoted 16 lines

drivers/soc/hisilicon/djtag.c new file +16 -0

+static const struct of_device_id djtag_of_match[] = {
+	/* for hip05(D02) cpu die */
+	{ .compatible = "hisilicon,hip05-cpu-djtag-v1",
+		.data = (void *)djtag_readwrite_v1 },
+	/* for hip05(D02) io die */
+	{ .compatible = "hisilicon,hip05-io-djtag-v1",
+		.data = (void *)djtag_readwrite_v1 },
+	/* for hip06(D03) cpu die */
+	{ .compatible = "hisilicon,hip06-cpu-djtag-v1",
+		.data = (void *)djtag_readwrite_v1 },
+	/* for hip06(D03) io die */
+	{ .compatible = "hisilicon,hip06-io-djtag-v2",
+		.data = (void *)djtag_readwrite_v2 },
+	/* for hip07(D05) cpu die */
+	{ .compatible = "hisilicon,hip07-cpu-djtag-v2",
+		.data = (void *)djtag_readwrite_v2 },

drivers/soc/hisilicon/djtag.c line 307

Quoted 5 lines

drivers/soc/hisilicon/djtag.c new file +5 -0

+	/* for hip07(D05) io die */
+	{ .compatible = "hisilicon,hip07-io-djtag-v2",
+		.data = (void *)djtag_readwrite_v2 },
+	{},
+};

Binding documentation for all of these should be added *before* this patch, per Documentation/devicetree/bindings/submitting-patches.txt. Please move any relevant binding documentation earlier in the series.

Quoted 11 lines

drivers/soc/hisilicon/djtag.c new file +11 -0

+MODULE_DEVICE_TABLE(of, djtag_of_match);
+
+static ssize_t
+show_modalias(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct hisi_djtag_client *client = to_hisi_djtag_client(dev);
+
+	return sprintf(buf, "%s%s\n", MODULE_PREFIX, client->name);
+}
+static DEVICE_ATTR(modalias, 0444, show_modalias, NULL);
+

drivers/soc/hisilicon/djtag.c line 326

Quoted 7 lines

drivers/soc/hisilicon/djtag.c new file +7 -0

+static struct attribute *hisi_djtag_dev_attrs[] = {
+	NULL,
+	/* modalias helps coldplug:  modprobe $(cat .../modalias) */
+	&dev_attr_modalias.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(hisi_djtag_dev);

Why do we need to expose this under sysfs?

drivers/soc/hisilicon/djtag.c line 417

Quoted 6 lines

drivers/soc/hisilicon/djtag.c new file +6 -0

+struct bus_type hisi_djtag_bus = {
+	.name		= "hisi-djtag",
+	.match		= hisi_djtag_device_match,
+	.probe		= hisi_djtag_device_probe,
+	.remove		= hisi_djtag_device_remove,
+};

I take it the core bus code handles reference-counting users of the bus?

Quoted 6 lines

drivers/soc/hisilicon/djtag.c new file +6 -0

+struct hisi_djtag_client *hisi_djtag_new_device(struct hisi_djtag_host *host,
+						struct device_node *node)
+{
+	struct hisi_djtag_client *client;
+	int status;
+

drivers/soc/hisilicon/djtag.c line 434

Quoted 10 lines

drivers/soc/hisilicon/djtag.c new file +10 -0

+	client = kzalloc(sizeof(*client), GFP_KERNEL);
+	if (!client)
+		return NULL;
+
+	client->host = host;
+
+	client->dev.parent = &client->host->dev;
+	client->dev.bus = &hisi_djtag_bus;
+	client->dev.type = &hisi_djtag_client_type;
+	client->dev.of_node = node;

I suspect that we should do:

	client->dev.of_node = of_node_get(node);

... so that it's guaranteed we retain a reference.

Quoted 19 lines

drivers/soc/hisilicon/djtag.c new file +19 -0

+	snprintf(client->name, DJTAG_CLIENT_NAME_LEN, "%s%s",
+					DJTAG_PREFIX, node->name);
+	dev_set_name(&client->dev, "%s", client->name);
+
+	status = device_register(&client->dev);
+	if (status < 0) {
+		pr_err("error adding new device, status=%d\n", status);
+		kfree(client);
+		return NULL;
+	}
+
+	return client;
+}
+
+static struct hisi_djtag_client *hisi_djtag_of_register_device(
+						struct hisi_djtag_host *host,
+						struct device_node *node)
+{
+	struct hisi_djtag_client *client;

drivers/soc/hisilicon/djtag.c line 459

Quoted 6 lines

drivers/soc/hisilicon/djtag.c new file +6 -0

+
+	client = hisi_djtag_new_device(host, node);
+	if (client == NULL) {
+		dev_err(&host->dev, "error registering device %s\n",
+			node->full_name);
+		of_node_put(node);

I don't think this should be here, given what djtag_register_devices() does.

Quoted 15 lines

drivers/soc/hisilicon/djtag.c new file +15 -0

+		return ERR_PTR(-EINVAL);
+	}
+
+	return client;
+}
+
+static void djtag_register_devices(struct hisi_djtag_host *host)
+{
+	struct device_node *node;
+	struct hisi_djtag_client *client;
+
+	if (!host->of_node)
+		return;
+
+	for_each_available_child_of_node(host->of_node, node) {

drivers/soc/hisilicon/djtag.c line 480

Quoted 6 lines

drivers/soc/hisilicon/djtag.c new file +6 -0

+		if (of_node_test_and_set_flag(node, OF_POPULATED))
+			continue;
+		client = hisi_djtag_of_register_device(host, node);
+		list_add(&client->next, &host->client_list);
+	}
+}

Given hisi_djtag_of_register_device() can return ERR_PTR(-EINVAL), the list_add is not safe.

Quoted 7 lines

drivers/soc/hisilicon/djtag.c new file +7 -0

+static int djtag_host_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct hisi_djtag_host *host;
+	const struct of_device_id *of_id;
+	struct resource *res;
+	int rc;

drivers/soc/hisilicon/djtag.c line 527

Quoted 10 lines

drivers/soc/hisilicon/djtag.c new file +10 -0

+
+	of_id = of_match_device(djtag_of_match, dev);
+	if (!of_id)
+		return -EINVAL;
+
+	host = kzalloc(sizeof(*host), GFP_KERNEL);
+	if (!host)
+		return -ENOMEM;
+
+	host->of_node = dev->of_node;

	host->of_node = of_node_get(dev->of_node);

Quoted 14 lines

drivers/soc/hisilicon/djtag.c new file +14 -0

+	host->djtag_readwrite = of_id->data;
+	spin_lock_init(&host->lock);
+
+	INIT_LIST_HEAD(&host->client_list);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "No reg resorces!\n");
+		kfree(host);
+		return -EINVAL;
+	}
+
+	if (!resource_size(res)) {
+		dev_err(&pdev->dev, "Zero reg entry!\n");

drivers/soc/hisilicon/djtag.c line 551

Quoted 10 lines

drivers/soc/hisilicon/djtag.c new file +10 -0

+		kfree(host);
+		return -EINVAL;
+	}
+
+	host->sysctl_reg_map = devm_ioremap_resource(dev, res);
+	if (IS_ERR(host->sysctl_reg_map)) {
+		dev_warn(dev, "Unable to map sysctl registers.\n");
+		kfree(host);
+		return -EINVAL;
+	}

Please have a common error path rather than duplicating this free.

e.g. at the end of the function have:

	err_free:
		kfree(host);
		return err;

... and in cases like this, have:

	if (whatever()) {
		dev_warn(dev, "this failed xxx\n");
		err = -EINVAL;
		goto err_free;
	}

Thanks,
Mark.

1) Device tree bindings for Hisilicon SoC PMU.
2) Add example for Hisilicon L3 cache, MN and DDRC PMU.

Signed-off-by Anurup M <[email protected]>

Signed-off-by Shaokun Zhang <[email protected]>

---
 .../devicetree/bindings/arm/hisilicon/pmu.txt      | 127 +++++++++++++++++++++
 1 file changed, 127 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/hisilicon/pmu.txt

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +14 -0

new file mode 100644
index 0000000..e7b35e0
@@ -0,0 +1,127 @@
+Hisilicon SoC hip05/06/07 ARMv8 PMU
+===================================
+
+The Hisilicon SoC chips like hip05/06/07 etc. consist of varous independent
+system device PMU's such as L3 cache (L3C), Miscellaneous Nodes(MN) and DDR
+comtroller. These PMU devices are independent and have hardware logic to
+gather statistics and performance information.
+
+HiSilicon SoC chip is encapsulated by multiple CPU and IO die's. The CPU die
+is called as Super CPU cluster (SCCL) which includes 16 cpu-cores. Every SCCL
+is further grouped as CPU clusters (CCL) which includes 4 cpu-cores each.
+e.g. In the case of hip05/06/07, each SCCL has 1 L3 cache and 1 MN PMU device.
+
+The Hisilicon SoC PMU DT node bindigs for uncore PMU devices are as below.

Quoted 2 lines · Anurup M

1) Device tree bindings for Hisilicon SoC PMU.
2) Add example for Hisilicon L3 cache, MN and DDRC PMU.

Get rid of this weird indentation in all patches.

Quoted 28 lines

Signed-off-by Anurup M <[email protected]>

Signed-off-by Shaokun Zhang <[email protected]>

---
  .../devicetree/bindings/arm/hisilicon/pmu.txt      | 127 +++++++++++++++++++++
  1 file changed, 127 insertions(+)

create mode 100644 Documentation/devicetree/bindings/arm/hisilicon/pmu.txt

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +14 -0

new file mode 100644
index 0000000..e7b35e0
@@ -0,0 +1,127 @@
+Hisilicon SoC hip05/06/07 ARMv8 PMU
+===================================
+
+The Hisilicon SoC chips like hip05/06/07 etc. consist of varous independent
+system device PMU's such as L3 cache (L3C), Miscellaneous Nodes(MN) and DDR
+comtroller. These PMU devices are independent and have hardware logic to
+gather statistics and performance information.
+
+HiSilicon SoC chip is encapsulated by multiple CPU and IO die's. The CPU die
+is called as Super CPU cluster (SCCL) which includes 16 cpu-cores. Every SCCL
+is further grouped as CPU clusters (CCL) which includes 4 cpu-cores each.
+e.g. In the case of hip05/06/07, each SCCL has 1 L3 cache and 1 MN PMU device.
+
+The Hisilicon SoC PMU DT node bindigs for uncore PMU devices are as below.

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt line 24

Quoted 10 lines

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +10 -0

+For PMU devices like L3 cache. MN etc. which are accessed using the djtag,
+the parent node will be the djtag node of the corresponding CPU die(SCCL).
+
+For uncore PMU devices there are some common required properties as detailed
+below.
+
+Required properties:
+	- compatible : This field contain two values. The first value is
+		always "hisilicon" and second value is the Module type as shown
+		in below examples:

Over-complicated sentence. Just:

- compatible : One of:
"hisilicon,hisi-pmu-l3c-v1" for Hisilicon SoC L3C PMU
device (Version 1)
...
...

BTW, No need of CC-ing me. I am not a maintainer of relevant subsystems.

Best regards,
Krzysztof

1) Device tree bindings for Hisilicon SoC PMU.
2) Add example for Hisilicon L3 cache, MN and DDRC PMU.

Signed-off-by Anurup M <[email protected]>

Signed-off-by Shaokun Zhang <[email protected]>

---
 .../devicetree/bindings/arm/hisilicon/pmu.txt      | 127 +++++++++++++++++++++
 1 file changed, 127 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/hisilicon/pmu.txt

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +0 -0

new file mode 100644
index 0000000..e7b35e0

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +12 -0

+	- reg : Register base address and range for the DDRC channel.
+
+Example:
+	/* DDRC for CPU die scl #2 Channel #1 for hip05 */
+	pmu_sccl0_ddrc1: pmu_ddrc1 at 80358000 {
+		 compatible = "hisilicon,hisi-pmu-ddrc-v1";
+		 scl-id = <0x02>;
+		 ch-id = <0x1>;
+		 num-events = <0x0D>;
+		 num-counters = <0x04>;
+		 reg = <0x80358000 0x10000>; /* TOTEMC DDRC1 */
+	 };

Signature · 1 line

2.1.4

Hi,

Quoted 10 lines · Anurup M

1) Device tree bindings for Hisilicon SoC PMU.
2) Add example for Hisilicon L3 cache, MN and DDRC PMU.

Signed-off-by Anurup M<[email protected]>

Signed-off-by Shaokun Zhang<[email protected]>

---
  .../devicetree/bindings/arm/hisilicon/pmu.txt      | 127 +++++++++++++++++++++
  1 file changed, 127 insertions(+)

create mode 100644 Documentation/devicetree/bindings/arm/hisilicon/pmu.txt

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt line 5

Quoted 11 lines

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +5 -0

new file mode 100644
index 0000000..e7b35e0
@@ -0,0 +1,127 @@
+Hisilicon SoC hip05/06/07 ARMv8 PMU
+===================================
+
+The Hisilicon SoC chips like hip05/06/07 etc. consist of varous independent
+system device PMU's such as L3 cache (L3C), Miscellaneous Nodes(MN) and DDR

s/PMU's/PMUs/

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt line 6

Quoted 1 line

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +1 -0

+comtroller. These PMU devices are independent and have hardware logic to

s/comtroller/controller/

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt line 9

Quoted 3 lines

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +3 -0

+gather statistics and performance information.
+
+HiSilicon SoC chip is encapsulated by multiple CPU and IO die's. The CPU die

s/die's/dies/

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt line 14

Quoted 5 lines

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +5 -0

+is called as Super CPU cluster (SCCL) which includes 16 cpu-cores. Every SCCL
+is further grouped as CPU clusters (CCL) which includes 4 cpu-cores each.
+e.g. In the case of hip05/06/07, each SCCL has 1 L3 cache and 1 MN PMU device.
+
+The Hisilicon SoC PMU DT node bindigs for uncore PMU devices are as below.

s/bindigs/bindings/

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt line 24

Quoted 10 lines

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +10 -0

+For PMU devices like L3 cache. MN etc. which are accessed using the djtag,
+the parent node will be the djtag node of the corresponding CPU die(SCCL).
+
+For uncore PMU devices there are some common required properties as detailed
+below.
+
+Required properties:
+	- compatible : This field contain two values. The first value is
+		always "hisilicon" and second value is the Module type as shown
+		in below examples:

Just say:

- Compatible: should contain one of:

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt line 34

Quoted 10 lines

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +10 -0

+		(a) "hisilicon,hisi-pmu-l3c-v1" for Hisilicon SoC L3C PMU
+			device (Version 1)
+		(b) "hisilicon,hisi-pmu-mn-v1" for Hisilicon SoC MN PMU
+			device (Version 1)
+		(c) "hisilicon,hisi-pmu-ddrc-v1" for Hisilicon SoC DDRC PMU
+			device (Version 1)
+		The hip05/06/07 chips have v1 hardware for L3C, MN and DDRC.
+
+	- scl-id : The Super Cluster ID. This can be the ID of the CPU die
+		   or IO die in the chip.

What's this needed for?

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt line 38

Quoted 3 lines

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +3 -0

+	- num-events : No of events supported by this PMU device.
+
+	- num-counters : No of hardware counters available for counting.

This isn't probeable or well-known?

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt line 49

Quoted 11 lines

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +11 -0

+
+L3 cache
+--------
+The L3 cache is dedicated for each SCCL and hence there are separate DT nodes
+for L3 cache for each SCCL. For L3 cache PMU the additional required properties
+are
+	- counter-reg : Counter register offset.
+
+	- evtype-reg : Event select register offset.
+
+	- evctrl-reg : Event counting control(LAUCTRL) register offset.

Surely for a given revision of the chip these offsets are known? i.e. surely the compatible string implies specific offsets?

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt line 49

Quoted 1 line

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +1 -0

+	- event-en : Event enable value.

Huh?

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt line 56

Quoted 4 lines

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +4 -0

+	- module-id : Module ID to input for djtag. This property is an array of
+		      module_id for each L3 cache banks.
+
+	- num-banks : Number of banks or instances of the device.

What's a bank? Surely they have separate instances of the PMU?

What order are these in?

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt line 58

Quoted 1 line

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +1 -0

+	- cfgen-map : Config enable array to select the bank.

Huh?

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt line 68

Quoted 9 lines

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +9 -0

+Miscellaneous Node
+-------------------
+The MN is dedicated for each SCCL and hence there are separate DT nodes for MN
+for each SCCL. For MN PMU the additional required properties are
+	- counter-reg : Counter register offset.
+
+	- evtype-reg : Event select register offset.
+
+	- evctrl-reg : Event counting control register offset.

Likewise, surely this is well-known for a given revision of the chip?

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt line 75

Quoted 7 lines

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +7 -0

+
+	- module-id : Module ID to input for djtag. As MN doesnot have multiple banks
+		      this property is a single value.
+
+	- cfgen-map : Config enable to select the bank. For MN it is a single value
+
+	- event-en : Event enable value.

Same comments as for the L3 cache nodes

[...]

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt line 115

Quoted 7 lines

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +7 -0

+DDR controller
+--------------
+Each SCCL in Hip05/06/07 chips have 2 DDR channels and hence 2 DDR controllers.
+There are separate DT nodes for each DDR channel.
+For DDRC PMU the additional required properties are
+
+	- ch-id : DDRC Channel ID.

Why is this necessary?

Thanks,
Mark.

Quoted 15 lines

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +12 -0

+	- reg : Register base address and range for the DDRC channel.
+
+Example:
+	/* DDRC for CPU die scl #2 Channel #1 for hip05 */
+	pmu_sccl0_ddrc1: pmu_ddrc1 at 80358000 {
+		 compatible = "hisilicon,hisi-pmu-ddrc-v1";
+		 scl-id = <0x02>;
+		 ch-id = <0x1>;
+		 num-events = <0x0D>;
+		 num-counters = <0x04>;
+		 reg = <0x80358000 0x10000>; /* TOTEMC DDRC1 */
+	 };

Signature · 1 line

2.1.4

Quoted 5 lines · Mark Rutland

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt line 34

Quoted 2 lines · Anurup M

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +2 -0

+	- scl-id : The Super Cluster ID. This can be the ID of the CPU die
+		   or IO die in the chip.

What's this needed for?

This is used as suffix to the PMU name. hisi_l3c<scl-id>. (hisi_l3c2
- for scl-id = 2).
This is to identify the pmu correspond to which CPU die in the socket.

Quoted 4 lines

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt line 38

Quoted 3 lines

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +3 -0

+	- num-events : No of events supported by this PMU device.
+
+	- num-counters : No of hardware counters available for counting.

This isn't probeable or well-known?

My idea is to have the common properties of SoC PMU added here. The num-events, num-counters etc. So that handling can be made common in the driver.
Is it not recommended? Please share your comments.

The below two properties (module-id, cfgen-map) differs between chips hip05/06 and hip07.

Please suggest.

Quoted 5 lines

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt line 56

Quoted 4 lines

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +4 -0

+	- module-id : Module ID to input for djtag. This property is an array of
+		      module_id for each L3 cache banks.
+
+	- num-banks : Number of banks or instances of the device.

What's a bank? Surely they have separate instances of the PMU?

Yes each bank is a separate instance of PMU.
If it is recommended to have each L3 cache bank registered as separate PMU with perf, then this property will be removed.

Quoted 1 line

What order are these in?

The bank number will start from "1" till "4" for L3 cache as there are four banks in hip05/06/07 chips.

Quoted 2 lines

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt line 58

Quoted 1 line

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +1 -0

+	- cfgen-map : Config enable array to select the bank.

Huh?

Quoted 8 lines · Anurup M

Quoted 4 lines · Mark Rutland

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt line 34

Quoted 2 lines · Anurup M

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +2 -0

+	- scl-id : The Super Cluster ID. This can be the ID of the CPU die
+		   or IO die in the chip.

What's this needed for?

This is used as suffix to the PMU name. hisi_l3c<scl-id>. (hisi_l3c2
- for scl-id = 2).
This is to identify the pmu correspond to which CPU die in the socket.

Quoted 8 lines

Quoted 4 lines

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt line 38

Quoted 3 lines

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +3 -0

+	- num-events : No of events supported by this PMU device.
+
+	- num-counters : No of hardware counters available for counting.

This isn't probeable or well-known?

My idea is to have the common properties of SoC PMU added here. The num-events, num-counters etc. So that handling can be made common in the driver.
Is it not recommended? Please share your comments.

This feels like something that should be well-known for the programming model of the device. If the number of events and/or counters shange, I'd expect other things to also change such that the device is no longer compatible with previous versions.

[...]

Quoted 2 lines

The below two properties (module-id, cfgen-map) differs between chips hip05/06 and hip07.

The module-id property sounds like a HW description, but it's not entirely clear to me what cfgen-map is; more comments on that below.

Quoted 9 lines

Please suggest.

Quoted 5 lines

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt line 56

Quoted 4 lines

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +4 -0

+	- module-id : Module ID to input for djtag. This property is an array of
+		      module_id for each L3 cache banks.
+
+	- num-banks : Number of banks or instances of the device.

What's a bank? Surely they have separate instances of the PMU?

Yes each bank is a separate instance of PMU.
If it is recommended to have each L3 cache bank registered as separate PMU with perf, then this property will be removed.

Generally, I think that separate instances are preferable.

Quoted 5 lines

Quoted 1 line

What order are these in?

The bank number will start from "1" till "4" for L3 cache as there are four banks in hip05/06/07 chips.

Quoted 2 lines

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt line 58

Quoted 1 line

Documentation/devicetree/bindings/arm/hisilicon/pmu.txt new file +1 -0

+	- cfgen-map : Config enable array to select the bank.

Huh?

As above, it's not clear to me what this property represents. Could you please clarify?

Thanks,
Mark.

drivers/perf/hisilicon/hisi_uncore_l3c.c new file +16 -0

+	do {
+		/* Get count from individual L3C banks and sum them up */
+		for (i = 0; i < num_banks; i++) {
+			total_raw_count += hisi_read_l3c_counter(l3c_hwmod_data,
+									idx, i);
+		}
+		prev_raw_count = local64_read(&hwc->prev_count);
+
+		/*
+		 * As prev_raw_count is updated with average value of
+		 * L3 cache banks, we multiply it by no of banks and
+		 * compute the delta
+		 */
+		delta = (total_raw_count - (prev_raw_count * num_banks)) &
+								HISI_MAX_PERIOD;
+

drivers/perf/hisilicon/hisi_uncore_l3c.c new file +8 -0

+static int hisi_l3c_get_event_idx(struct hisi_pmu *pl3c_pmu)
+{
+	struct hisi_l3c_data *l3c_hwmod_data = pl3c_pmu->hwmod_data;
+	int event_idx;
+
+	event_idx =
+		find_first_zero_bit(
+			l3c_hwmod_data->hisi_l3c_event_used_mask,

drivers/perf/hisilicon/hisi_uncore_pmu.c new file +14 -0

+
+	if (event->attr.type != event->pmu->type)
+		return -ENOENT;
+
+	/* we do not support sampling as the counters are all
+	 * shared by all CPU cores in a CPU die(SCCL). Also we
+	 * donot support attach to a task(per-process mode)
+	 */
+	if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK)
+		return -EOPNOTSUPP;
+
+	/* counters do not have these bits */
+	if (event->attr.exclude_user	||
+	    event->attr.exclude_kernel	||

drivers/perf/hisilicon/hisi_uncore_pmu.c new file +27 -0

+}
+
+void hisi_pmu_set_event_period(struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	struct hisi_pmu *phisi_pmu = to_hisi_pmu(event->pmu);
+
+	/*
+	 * The Hisilicon PMU counters have a period of 2^32. To account for the
+	 * possiblity of extreme interrupt latency we program for a period of
+	 * half that. Hopefully we can handle the interrupt before another 2^31
+	 * events occur and the counter overtakes its previous value.
+	 */
+	u64 val = 1ULL << 31;
+
+	local64_set(&hwc->prev_count, val);
+
+	/* Write to the hardware event counter */
+	phisi_pmu->ops->write_counter(phisi_pmu, hwc, val);
+}
+
+void hisi_uncore_pmu_start(struct perf_event *event, int flags)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	struct hisi_pmu *phisi_pmu = to_hisi_pmu(event->pmu);
+	struct hisi_pmu_hw_events *hw_events;
+

Quoted 16 lines · Anurup M

drivers/perf/hisilicon/hisi_uncore_l3c.c new file +16 -0

+	do {
+		/* Get count from individual L3C banks and sum them up */
+		for (i = 0; i < num_banks; i++) {
+			total_raw_count += hisi_read_l3c_counter(l3c_hwmod_data,
+									idx, i);
+		}
+		prev_raw_count = local64_read(&hwc->prev_count);
+
+		/*
+		 * As prev_raw_count is updated with average value of
+		 * L3 cache banks, we multiply it by no of banks and
+		 * compute the delta
+		 */
+		delta = (total_raw_count - (prev_raw_count * num_banks)) &
+								HISI_MAX_PERIOD;
+

drivers/perf/hisilicon/hisi_uncore_l3c.c line 96

Quoted 10 lines

drivers/perf/hisilicon/hisi_uncore_l3c.c new file +10 -0

+		local64_add(delta, &event->count);
+
+		/*
+		 * Divide by num of banks to get average count and
+		 * update prev_count with this value
+		 */
+		avg_raw_count = total_raw_count / num_banks;
+	} while (local64_cmpxchg(
+			 &hwc->prev_count, prev_raw_count, avg_raw_count) !=
+							 prev_raw_count);

Please don't aggregate like this; expose separate PMUs instead.

This is racy, and by averaging and multiplying we're making up and/or throwing away data.

[...]

Quoted 4 lines

drivers/perf/hisilicon/hisi_uncore_l3c.c new file +4 -0

+	event_value = (val -
+			HISI_HWEVENT_L3C_READ_ALLOCATE);
+
+	/* Select the appropriate Event select register */

drivers/perf/hisilicon/hisi_uncore_l3c.c line 119

Quoted 6 lines

drivers/perf/hisilicon/hisi_uncore_l3c.c new file +6 -0

+	if (idx > 3)
+		reg_offset += 4;
+
+	/* Value to write to event type register */
+	val = event_value << (8 * idx);
+

Please add helpers for these, and explain *why* the transformations are necessary.

drivers/perf/hisilicon/hisi_uncore_l3c.c line 126

Quoted 7 lines

drivers/perf/hisilicon/hisi_uncore_l3c.c new file +7 -0

+	/* Find the djtag Identifier of the Unit */
+	client = l3c_hwmod_data->client;
+
+	/*
+	 * Set the event in L3C_EVENT_TYPEx Register
+	 * for all L3C banks
+	 */

As above, it seems like you should expose a separate PMU per bank instead. That applies for all the other instances where you iterate over banks.

[...]

drivers/perf/hisilicon/hisi_uncore_l3c.c line 176

Quoted 11 lines

drivers/perf/hisilicon/hisi_uncore_l3c.c new file +11 -0

+	for (i = 0; i < l3c_hwmod_data->l3c_hwcfg.num_banks; i++) {
+		module_id = l3c_hwmod_data->l3c_hwcfg.module_id[i];
+		cfg_en = l3c_hwmod_data->l3c_hwcfg.bank_cfgen[i];
+		ret = hisi_djtag_writereg(module_id,
+					cfg_en,
+					reg_offset,
+					value,
+					client);
+		if (!ret)
+			ret = value;
+	}

This is impossible to read. Please factor this into helpers such that you don't need this amount of indentation.

Please do similarly elsewhere when you see this indentation pattern.

[...]

Quoted 12 lines

drivers/perf/hisilicon/hisi_uncore_l3c.c new file +12 -0

+static int hisi_l3c_get_event_idx(struct hisi_pmu *pl3c_pmu)
+{
+	struct hisi_l3c_data *l3c_hwmod_data = pl3c_pmu->hwmod_data;
+	int event_idx;
+
+	event_idx =
+		find_first_zero_bit(
+			l3c_hwmod_data->hisi_l3c_event_used_mask,
+					 pl3c_pmu->num_counters);
+
+	if (event_idx == HISI_MAX_CFG_L3C_CNTR)
+		return -EAGAIN;

drivers/perf/hisilicon/hisi_uncore_l3c.c line 323

Quoted 6 lines

drivers/perf/hisilicon/hisi_uncore_l3c.c new file +6 -0

+
+	__set_bit(event_idx,
+		l3c_hwmod_data->hisi_l3c_event_used_mask);
+
+	return event_idx;
+}

Please get rid of the weird hungarian notation (i.e. don't use 'p' as a prefix for pointers), and use temporary variables consistently, e.g.

static int hisi_l3c_get_event_idx(struct hisi_pmu *l3c_pmu)

{
	struct hisi_l3c_data *l3c_hwmod_data = l3c_pmu->hwmod_data;
	unsigned long *used_mask = l3c_hwmod_data->hisi_l3c_event_used_mask;
	int num_counters = pl3c_pmu->num_counters
	int idx;

	idx = find_first_zero_bit(used_mask, num_counters);
	if (idx == num_counters)
		return -EAGAIN;

	set_bit(idx, used_mask);

	return idx;
}

[...]

drivers/perf/hisilicon/hisi_uncore_l3c.c line 343

Quoted 5 lines

drivers/perf/hisilicon/hisi_uncore_l3c.c new file +5 -0

+	if (of_property_read_u32(node, "counter-reg",
+				     &pl3c_hwcfg->counter_reg0_off)) {
+		dev_err(dev, "DT:Couldnot read counter-reg!\n");
+		return -EINVAL;
+	}

Please use spaces in these messages.

Otherwise, my comments on the binding apply here.

[...]

drivers/perf/hisilicon/hisi_uncore_l3c.c line 408

Quoted 9 lines

drivers/perf/hisilicon/hisi_uncore_l3c.c new file +9 -0

+static int init_hisi_l3c_data(struct device *dev,
+					struct hisi_pmu *pl3c_pmu,
+					struct hisi_djtag_client *client)
+{
+	struct hisi_l3c_data *l3c_hwmod_data = NULL;
+	int ret;
+
+	l3c_hwmod_data = kzalloc(sizeof(struct hisi_l3c_data),
+							GFP_KERNEL);

Use:

l3c_hwmod_data = kzalloc(sizeof(*l3c_hwmod_data, GFP_KERNEL):

[...]

drivers/perf/hisilicon/hisi_uncore_l3c.c line 479

Quoted 9 lines

drivers/perf/hisilicon/hisi_uncore_l3c.c new file +9 -0

+static int hisi_pmu_l3c_dev_probe(struct hisi_djtag_client *client)
+{
+	struct hisi_pmu *pl3c_pmu = NULL;
+	struct device *dev = &client->dev;
+	int ret;
+
+	pl3c_pmu = hisi_pmu_alloc(dev);
+	if (IS_ERR(pl3c_pmu))
+		return PTR_ERR(pl3c_pmu);

Why use error pointers for this?

hisi_pmu_alloc() only ever returns ERR_PTR(-ENOMEM) if it failed to allocate.

It's far simpler to have it pass on NULL there, and here do:

	pl3c_pmu = hisi_pmu_alloc(dev);
	if (!pl3c_pmu)
		return -ENOMEM;

Please also s/pl3c_pmu/l3c_pmu/ here, and elsewhere throughout the driver. The 'p' only serves to make this harder to read.

[...]

drivers/perf/hisilicon/hisi_uncore_l3c.c line 499

Quoted 11 lines

drivers/perf/hisilicon/hisi_uncore_l3c.c new file +11 -0

+	/* Register with perf PMU */
+	pl3c_pmu->pmu = (struct pmu) {
+		.name = pl3c_pmu->name,
+		.task_ctx_nr = perf_invalid_context,
+		.event_init = hisi_uncore_pmu_event_init,
+		.add = hisi_uncore_pmu_add,
+		.del = hisi_uncore_pmu_del,
+		.start = hisi_uncore_pmu_start,
+		.stop = hisi_uncore_pmu_stop,
+		.read = hisi_uncore_pmu_read,
+	};

Please remove the comment above this.

[...]

drivers/perf/hisilicon/hisi_uncore_pmu.c line 102

Quoted 4 lines

drivers/perf/hisilicon/hisi_uncore_pmu.c new file +4 -0

+int hisi_uncore_pmu_event_init(struct perf_event *event)
+{
+	int err;
+	struct hisi_pmu *phisi_pmu = to_hisi_pmu(event->pmu);

This is undefined behaviour. This must be done *after* we check the event->pmu->type.

Quoted 14 lines

drivers/perf/hisilicon/hisi_uncore_pmu.c new file +14 -0

+
+	if (event->attr.type != event->pmu->type)
+		return -ENOENT;
+
+	/* we do not support sampling as the counters are all
+	 * shared by all CPU cores in a CPU die(SCCL). Also we
+	 * donot support attach to a task(per-process mode)
+	 */
+	if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK)
+		return -EOPNOTSUPP;
+
+	/* counters do not have these bits */
+	if (event->attr.exclude_user	||
+	    event->attr.exclude_kernel	||

drivers/perf/hisilicon/hisi_uncore_pmu.c line 126

Quoted 10 lines

drivers/perf/hisilicon/hisi_uncore_pmu.c new file +10 -0

+	    event->attr.exclude_host	||
+	    event->attr.exclude_guest	||
+	    event->attr.exclude_hv	||
+	    event->attr.exclude_idle)
+		return -EINVAL;
+
+	if (event->cpu < 0)
+		return -EINVAL;
+
+	event->cpu = cpumask_first(&phisi_pmu->cpu);

You should also check the event grouping.

Take a look at what we do in arch/arm/mm/cache-l2x0-pmu.c.

[...]

Quoted 5 lines

drivers/perf/hisilicon/hisi_uncore_pmu.c new file +5 -0

+/*
+ * Enable counter and set the counter to count
+ * the event that we're interested in.
+ */
+void hisi_uncore_pmu_enable_event(struct perf_event *event)

drivers/perf/hisilicon/hisi_uncore_pmu.c line 144

Quoted 7 lines

drivers/perf/hisilicon/hisi_uncore_pmu.c new file +7 -0

+{
+	struct hw_perf_event *hwc = &event->hw;
+	struct hisi_pmu *phisi_pmu = to_hisi_pmu(event->pmu);
+
+	/* Disable the hardware event counting */
+	if (phisi_pmu->ops->disable_counter)
+		phisi_pmu->ops->disable_counter(phisi_pmu, GET_CNTR_IDX(hwc));

Why isn't the counter already disabled?

drivers/perf/hisilicon/hisi_uncore_pmu.c line 151

Quoted 6 lines

drivers/perf/hisilicon/hisi_uncore_pmu.c new file +6 -0

+	/*
+	 * Set event (if destined for Hisilicon SoC counters).
+	 */
+	if (phisi_pmu->ops->set_evtype)
+		phisi_pmu->ops->set_evtype(phisi_pmu, GET_CNTR_IDX(hwc),
+							hwc->config_base);

Why isn't this done in the pmu::event_add callback?

drivers/perf/hisilicon/hisi_uncore_pmu.c line 155

Quoted 4 lines

drivers/perf/hisilicon/hisi_uncore_pmu.c new file +4 -0

+
+	/* Enable the hardware event counting */
+	if (phisi_pmu->ops->enable_counter)
+		phisi_pmu->ops->enable_counter(phisi_pmu, GET_CNTR_IDX(hwc));

This should be the only necessary part of this function.

Quoted 30 lines

drivers/perf/hisilicon/hisi_uncore_pmu.c new file +30 -0

+}
+
+void hisi_pmu_set_event_period(struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	struct hisi_pmu *phisi_pmu = to_hisi_pmu(event->pmu);
+
+	/*
+	 * The Hisilicon PMU counters have a period of 2^32. To account for the
+	 * possiblity of extreme interrupt latency we program for a period of
+	 * half that. Hopefully we can handle the interrupt before another 2^31
+	 * events occur and the counter overtakes its previous value.
+	 */
+	u64 val = 1ULL << 31;
+
+	local64_set(&hwc->prev_count, val);
+
+	/* Write to the hardware event counter */
+	phisi_pmu->ops->write_counter(phisi_pmu, hwc, val);
+}
+
+void hisi_uncore_pmu_start(struct perf_event *event, int flags)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	struct hisi_pmu *phisi_pmu = to_hisi_pmu(event->pmu);
+	struct hisi_pmu_hw_events *hw_events;
+
+	hw_events = &phisi_pmu->hw_events;
+
+	if (WARN_ON_ONCE(!(hwc->state & PERF_HES_STOPPED)))

drivers/perf/hisilicon/hisi_uncore_pmu.c line 192

Quoted 7 lines

drivers/perf/hisilicon/hisi_uncore_pmu.c new file +7 -0

+		return;
+
+	WARN_ON_ONCE(!(hwc->state & PERF_HES_UPTODATE));
+	hwc->state = 0;
+
+	if (phisi_pmu->ops->set_event_period)
+		phisi_pmu->ops->set_event_period(event);

When will this differ from hisi_pmu_set_event_period() above?

drivers/perf/hisilicon/hisi_uncore_pmu.c line 199

Quoted 6 lines

drivers/perf/hisilicon/hisi_uncore_pmu.c new file +6 -0

+	if (flags & PERF_EF_RELOAD) {
+		u64 prev_raw_count =  local64_read(&hwc->prev_count);
+
+		phisi_pmu->ops->write_counter(phisi_pmu, hwc,
+						(u32)prev_raw_count);
+	}

If we always go through hisi_pmu_set_event_period(), this looks redundant.

drivers/perf/hisilicon/hisi_uncore_pmu.c line 201

Quoted 2 lines

drivers/perf/hisilicon/hisi_uncore_pmu.c new file +2 -0

+
+	hisi_uncore_pmu_enable_event(event);

There's no matching disable_event() call in this function, so this looks suspicious.

drivers/perf/hisilicon/hisi_uncore_pmu.c line 211

Quoted 10 lines

drivers/perf/hisilicon/hisi_uncore_pmu.c new file +10 -0

+	perf_event_update_userpage(event);
+}
+
+void hisi_uncore_pmu_stop(struct perf_event *event, int flags)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	struct hisi_pmu *phisi_pmu = to_hisi_pmu(event->pmu);
+
+	if (hwc->state & PERF_HES_UPTODATE)
+		return;

Why?

[...]

drivers/perf/hisilicon/hisi_uncore_pmu.c line 301

Quoted 8 lines

drivers/perf/hisilicon/hisi_uncore_pmu.c new file +8 -0

+int hisi_uncore_common_fwprop_read(struct device *dev,
+					struct hisi_pmu *phisi_pmu)
+{
+	if (device_property_read_u32(dev, "num-events",
+					&phisi_pmu->num_events)) {
+		dev_err(dev, "Cant read num-events from DT!\n");
+		return -EINVAL;
+	}

For consistency with the rest of the driver, and given there is no ACPI support, please use the of_property_* API here.

Thanks,
Mark.