AXI performance monitor driver #2222

JamesH65 · 2017-10-11T15:25:30Z

Uses the debugfs I/F to provide access to the AXI
bus performance monitors.

Requires the new mailbox peripheral access for access
to the VPU performance registers, system bus access
is done using direct register reads.

pelwell

Device tree is supposed to describe the hardware, not specify a driver. I don't see any reason not call this bcm2835-axiperf, even though we wrote it.

I've hit my comments limit. Try running checkpatch and I'll have another go on the result.

pelwell · 2017-10-11T15:41:47Z

drivers/perf/raspberrypi_axi_monitor.c

+	},
+};
+
+static int __init rpi_axiperf_init(void)


Replace from this line...

pelwell · 2017-10-11T15:42:48Z

drivers/perf/raspberrypi_axi_monitor.c

+}
+
+module_init(rpi_axiperf_init);
+module_exit(rpi_axiperf_exit);


...to this one, inclusive, with:

module_platform_driver(rpi_axiperf_driver);

pelwell · 2017-10-11T15:43:46Z

drivers/perf/raspberrypi_axi_monitor.c

+	// Now storage for the per monitor settings and the resulting
+	// performance figures
+	struct
+	{


Don't forget to stick to the ugly kernel brace placement rules.

pelwell · 2017-10-11T15:46:01Z

drivers/perf/raspberrypi_axi_monitor.c

+// Record results from the three Bus watchers per monitor and push to the sysfs
+
+// General reg
+const int GEN_CTRL = 0x00;


The kernel devs prefer #defines to const ints (go figure), but if you choose to stick with consts you'll have to change the symbols to lower case.

pelwell · 2017-10-11T15:47:08Z

drivers/perf/raspberrypi_axi_monitor.c

+		int i;
+		void __iomem *addr = state->monitor[monitor].base_address
+				+ watcher + BW_ATRANS_OFFSET;
+		for (i=0;i<NUM_BUS_WATCHER_RESULTS ;i++, addr+=4)


Spaces after semicolons here, none before.

pelwell · 2017-10-11T15:51:57Z

drivers/perf/raspberrypi_axi_monitor.c

+		 * spread our 3 actual monitors over them
+		 */
+		num_buses[monitor] = hweight32(state->monitor[monitor].bus_enabled);
+		num_buses[monitor] = min(num_buses[monitor],


The double assignment gave me a momentary WTF... It looks like the line is already over-long, so why not split it like this instead?

num_buses[monitor] = min( hweight32(state->monitor[monitor].bus_enabled), NUM_BUS_WATCHERS_PER_MONITOR);

But see previous comment about a local pointer to state->monitor[monitor].

pelwell · 2017-10-11T15:54:24Z

drivers/perf/raspberrypi_axi_monitor.c

+			// No, disable all monitoring for this monitor
+			set_monitor_control(monitor, GEN_CTL_RESET_BIT);
+		} else {
+		int i;


The indentation for this section is wrong. You've got enough logic in here that you might want to split out a helper function.

pelwell · 2017-10-11T15:55:56Z

drivers/perf/raspberrypi_axi_monitor.c

+
+	for (monitor = 0; monitor < NUM_MONITORS; monitor++) {
+		// Anything enabled?
+		if (state->monitor[monitor].bus_enabled == 0) {


This loop is crying out for a local pointer to replace "state->monitor[monitor]".

pelwell · 2017-10-11T15:56:44Z

drivers/perf/raspberrypi_axi_monitor.c

+			do {
+				state->monitor[monitor].last_monitored++;
+				state->monitor[monitor].last_monitored &= 0xf; //wrap around
+			} while ((state->monitor[monitor].bus_enabled &


Kernel devs prefer "!x" to "x == 0".

JamesH65 · 2017-10-13T14:43:32Z

I've pushed a much cleaned up version. Please be gentle.

JamesH65 · 2017-10-24T13:41:55Z

@pelwell @6by9 Should be mostly checkpatch compliant....couple of long lines I don't know what to do with - splitting would make it look fairly horrible.

firmware: New mailbox fns for peripheral access See: raspberrypi/linux#2222

Dismissed, waiting for next review

JamesH65 · 2017-11-02T12:28:06Z

@popcornmix @pelwell @6by9 I have updated to latest tree so this can now be merged if it is regarded as OK. COuld probably still do with some good 'ole revoooing.

6by9 · 2017-11-02T13:11:13Z

drivers/perf/raspberrypi_axi_monitor.c

+
+/* Two monitors, System and VPU, each with the following register sets
+ * Each monitor can only monitor one bus at a time, so we time share them,
+ * giving each bus 100ms (default, settable via debufs) of time on its


s/debufs/debugfs

6by9 · 2017-11-02T13:13:26Z

drivers/perf/raspberrypi_axi_monitor.c

+
+#include <linux/device.h>
+#include <linux/debugfs.h>
+#include <linux/netdevice.h>


Upstream would whinge about alphabetical order.

6by9 · 2017-11-02T13:14:17Z

drivers/perf/raspberrypi_axi_monitor.c

+
+	struct rpi_firmware *firmware;
+
+	// Sample time spent on for each bus


Phil: Are we being fussy about /* */ commenting over C99 // ?

I'm not fussed provided checkpatch is happy.

I'll change them all anyway.

Am I not looking at the latest, or are we ignoring them (somehow checkpatch is letting that through).
Ah, I have --strict --codespell set as arguments to checkpatch by default (although that is still allowing C99 commenting!)

6by9 · 2017-11-02T13:15:42Z

drivers/perf/raspberrypi_axi_monitor.c

+ * raspberrypi_axi_monitor.c
+ *
+ *  Created on: 26 Sep 2017
+ *      Author: jamesh


6by9 · 2017-11-02T13:15:59Z

drivers/perf/raspberrypi_axi_monitor.c

+
+static struct rpi_axiperf *state;
+
+/* Two monitors, System and VPU, each with the following register sets


Fullstop on the end of the line.
Upstream typically want:

/* * Comments here */

for multi line comments.

6by9 · 2017-11-02T13:17:23Z

drivers/perf/raspberrypi_axi_monitor.c

+// general registers
+const int GEN_CTRL;
+
+const int GEN_CTL_ENABLE_BIT	= 0x01;


Use the BIT(x) macros throughout?
GENMASK(<top_bit>, <bottom_bit>) for masks.

6by9 · 2017-11-02T13:17:40Z

drivers/perf/raspberrypi_axi_monitor.c

+const int BW_CTRL_BUS_WATCH_MASK	= 0x3d << 0; // 6 bits
+const int BW_CTRL_BUS_FILTER_SHIFT = 8;
+
+static char *bus_filter_strings[] = {


const for all these arrays and sizes?

6by9 · 2017-11-02T13:20:51Z

drivers/perf/raspberrypi_axi_monitor.c

+		if (kthread_should_stop())
+			return 0;
+
+		msleep(100);


Comments said default was 100ms settable in debugfs, or is this a different delay?

OOOh, bug, that shouldn't be there I don't think.

6by9 · 2017-11-02T13:23:05Z

drivers/perf/raspberrypi_axi_monitor.c

+			return PTR_ERR(state->monitor[i].base_address);
+
+		// Enable all buses by default
+		state->monitor[i].bus_enabled = 0xffff;


All enabled by default, and driver enabled in DT. Is that the desired default? Does it have any real overhead?

Intention is for this module only to be built in if specifically requested for debugging purposes, overhead in the not built case is clearly 0, but there is overhead when it is installed and running.

In which case shouldn't the default status in DT be "disabled"? It's currently "okay" in bcm270x.dtsi.

(Or have it in the build but disabled in DT by default? Still zero overhead for most, but easier to enable when desired).

6by9 · 2017-11-02T13:25:27Z

drivers/perf/raspberrypi_axi_monitor.c

+	.probe =	rpi_axiperf_probe,
+	.remove =	rpi_axiperf_remove,
+	.driver = {
+		.name   = "bcm2835-axiperf",


Same discussion as with the GPIO expander driver. This isn't generic for the BCM283x chips as it relies on the firmware.
"raspberrypi,axiperf" instead? (It is meant to be "vendor,driver", and you had no vendor)

We haven't always followed the principle, but device tree is supposed to describe the hardware, not the driver. The performance monitors are Broadcom parts, even if the driver is pi-specific.

So, change the function names to bcm-blah-blah instead of rpi-blah-blah as well as the driver name?

No, I'm agreeing with you. I think the DT compatible string should remain "bcm2835,axiperf", and I'm happy with the downstream "rpi" function names. I'd go further and change the driver name to include "rpi" or "raspberrypi".

I'll defer to Phil on this, but the usage is a hybrid where half of it is talking to the firmware (compatible string "raspberrypi,bcm2835-firmware") and not directly to the hardware.

pelwell · 2017-11-02T12:53:14Z

drivers/perf/raspberrypi_axi_monitor.c

+static ssize_t mywriter(struct file *fp, const char __user *user_buffer,
+			size_t count, loff_t *position)
+{
+	// TODO: this really needs to zero the registers, not this stuff.


Are you going to implement this?

Will remove the comment - its not really necessary. I'd like to leave the function so it can be used at a later date although I suspect upstream wouldn't like that.

pelwell · 2017-11-02T13:37:10Z

drivers/perf/raspberrypi_axi_monitor.c

+	.probe =	rpi_axiperf_probe,
+	.remove =	rpi_axiperf_remove,
+	.driver = {
+		.name   = "bcm2835-axiperf",


We haven't always followed the principle, but device tree is supposed to describe the hardware, not the driver. The performance monitors are Broadcom parts, even if the driver is pi-specific.

6by9 · 2017-11-02T15:36:26Z

drivers/perf/raspberrypi_axi_monitor.c

+
+	// Special case for the VPU monitor, we must use the mailbox interface
+	// as it is not accessible from the ARM address space.
+	state->monitor[VPU_MONITOR].use_mailbox_interface = 1;


Sorry, an extra one.
This is a nasty dependency between driver and DT that the addresses for the VPU_MONITOR must be second in the DT list of regs. Is there a better way of reflecting that?

You've also hard coded the address as 0x7ee08000 at line 551, so the DT value doesn't actually matter.

It's hardcoded, because the values I was getting back were not the ones I had in the DT entry, and I never figured out why. Probably should look at that but not got too much time for polishing.

There are precedents for defining an order for items under reg. The alternative is to have a reg-names property containing an array of names, and use platform_get_resource_byname().

The reason the address has changed is that the bus address has been converted into an ARM address. You can retrieve the original value using of_get_property_read_u32_index() or of_get_property_read_u32_array().

JamesH65 · 2017-11-02T17:23:27Z

Ah ha, as Alan Partridge would say. Will fix that up.

…

On 2 Nov 2017 16:04, "Phil Elwell" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In drivers/perf/raspberrypi_axi_monitor.c <#2222 (comment)>: > + return -ENOMEM; + + /* Get the firmware handle for future rpi-firmware-xxx calls */ + fw_node = of_parse_phandle(np, "firmware", 0); + if (!fw_node) { + dev_err(dev, "Missing firmware node\n"); + return -ENOENT; + } + + state->firmware = rpi_firmware_get(fw_node); + if (!state->firmware) + return -EPROBE_DEFER; + + // Special case for the VPU monitor, we must use the mailbox interface + // as it is not accessible from the ARM address space. + state->monitor[VPU_MONITOR].use_mailbox_interface = 1; There are precedents for defining an order for items under reg. The alternative is to have a reg-names property containing an array of names, and use platform_get_resource_byname(). The reason the address has changed is that the bus address has been converted into an ARM address. You can retrieve the original value using of_get_property_read_u32_index() or of_get_property_read_u32_array(). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2222 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADqrHVYDnjWSxeKOTP8ga9zIQpagBJlmks5syegXgaJpZM4P1q72> .

JamesH65 · 2017-11-03T14:51:01Z

@pelwell @6by9 Have just pushed new commit which hopefully covers the points above.

6by9

Generally happy.

6by9 · 2017-11-03T15:04:50Z

drivers/perf/raspberrypi_axi_monitor.c

+/*
+ * raspberrypi_axi_monitor.c
+ *
+ *  Created on: 26 Sep 2017


Odd line to have in the header.

Ah, added by Eclipse when I created the file. Can remove.

6by9 · 2017-11-03T15:06:04Z

drivers/perf/raspberrypi_axi_monitor.c

+
+	struct rpi_firmware *firmware;
+
+	// Sample time spent on for each bus


Am I not looking at the latest, or are we ignoring them (somehow checkpatch is letting that through).
Ah, I have --strict --codespell set as arguments to checkpatch by default (although that is still allowing C99 commenting!)

6by9 · 2017-11-03T15:32:47Z

arch/arm/boot/dts/bcm270x.dtsi

+			reg = <0x7e009800 0x100>,
+			      <0x7ee08000 0x100>;
+			firmware = <&firmware>;
+			status = "okay";


@pelwell Are you happy for this to be enabled by default even though the driver won't be built?

I think I'd rather have it disabled at this level then enabled higher up, e.g. by bcm2708-rpi.dtsi (even though the two are equivalent).

In general it seems to be frowned on to introduce nodes with an explicit status of "okay", since that is the default.

I have no idea what to put here, I copied that section from another section, so do not know what the status parameter actually means.

6by9

Really nit-picking.

6by9 · 2017-11-07T14:40:16Z

drivers/perf/raspberrypi_axi_monitor.c

+	state->firmware = rpi_firmware_get(fw_node);
+	if (!state->firmware)
+	{
+		dev_err(dev, "Probe deferred\n");


dev_err is a touch high for a probe deferral.

Sorry, see previous comment.

6by9 · 2017-11-07T14:41:14Z

drivers/perf/raspberrypi_axi_monitor.c

+
+const static char *monitor_name[] = {
+	"System Monitor",
+	"VPU Monitor"


These are also the directory names and they have spaces in :-(
We know these are going to be monitoring things, so is the word "monitor" really required in either case?

Bugger, put those in this morning for debugging, forgot to take them out. Inclined to remove both.

Oop, wrong comment. I'll remove spaces, been meaning to for ages anyway...

Uses the debugfs I/F to provide access to the AXI bus performance monitors. Requires the new mailbox peripheral access for access to the VPU performance registers, system bus access is done using direct register reads. Signed-off-by: James Hughes <james.hughes@raspberrypi.org> raspberrypi_axi_monitor: suppress warning Suppress the following warning by casting the pointer to and uintptr_t before to u32: Signed-off-by: Matteo Croce <mcroce@redhat.com> perf/raspberry: Add support for 2712 axi performance monitors Also handle 2711 correctly which has a different configuration from 2835. Signed-off-by: Dom Cobley <popcornmix@gmail.com>

pelwell previously requested changes Oct 11, 2017

View reviewed changes

JamesH65 force-pushed the AxiPerfMonPR branch from 1da1327 to e13a34c Compare October 12, 2017 11:09

popcornmix force-pushed the rpi-4.13.y branch from e262255 to 1e16a53 Compare October 13, 2017 12:54

JamesH65 force-pushed the AxiPerfMonPR branch from e13a34c to a855b93 Compare October 13, 2017 14:42

popcornmix force-pushed the rpi-4.13.y branch 3 times, most recently from f4b3ddd to ecbbac9 Compare October 22, 2017 12:37

JamesH65 force-pushed the AxiPerfMonPR branch from a855b93 to 5e93a76 Compare October 24, 2017 13:37

popcornmix added a commit to raspberrypi/firmware that referenced this pull request Oct 24, 2017

kernel: Bump to 4.9.58

d5088b0

firmware: New mailbox fns for peripheral access See: raspberrypi/linux#2222

popcornmix added a commit to Hexxeh/rpi-firmware that referenced this pull request Oct 24, 2017

kernel: Bump to 4.9.58

a07ea5c

firmware: New mailbox fns for peripheral access See: raspberrypi/linux#2222

popcornmix force-pushed the rpi-4.13.y branch from fec87ed to b76f96e Compare October 27, 2017 20:24

JamesH65 force-pushed the AxiPerfMonPR branch from 5e93a76 to e9ea6dc Compare November 2, 2017 12:26

JamesH65 force-pushed the AxiPerfMonPR branch from e9ea6dc to 8296317 Compare November 2, 2017 12:43

JamesH65 changed the title ~~AXI performance monitor driver - RFC~~ AXI performance monitor driver Nov 2, 2017

6by9 reviewed Nov 2, 2017

View reviewed changes

pelwell reviewed Nov 2, 2017

View reviewed changes

6by9 reviewed Nov 2, 2017

View reviewed changes

popcornmix force-pushed the rpi-4.13.y branch from b76f96e to 59341bd Compare November 3, 2017 14:39

JamesH65 force-pushed the AxiPerfMonPR branch 2 times, most recently from 2e52aff to d7c8487 Compare November 3, 2017 14:49

6by9 reviewed Nov 3, 2017

View reviewed changes

JamesH65 force-pushed the AxiPerfMonPR branch from d7c8487 to a2af72c Compare November 7, 2017 14:10

6by9 reviewed Nov 7, 2017

View reviewed changes

JamesH65 force-pushed the AxiPerfMonPR branch from a2af72c to 7d11e59 Compare November 7, 2017 16:00


		struct rpi_firmware *firmware;

		// Sample time spent on for each bus


		static struct rpi_axiperf *state;

		/* Two monitors, System and VPU, each with the following register sets

AXI performance monitor driver #2222

AXI performance monitor driver #2222

Conversation

JamesH65 commented Oct 11, 2017 • edited Loading

pelwell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesH65 commented Oct 13, 2017

JamesH65 commented Oct 24, 2017

JamesH65 commented Nov 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesH65 commented Nov 2, 2017 via email

JamesH65 commented Nov 3, 2017

6by9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesH65 Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

6by9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesH65 commented Oct 11, 2017 •

edited

Loading

JamesH65 Nov 3, 2017 •

edited

Loading