Hi, Jonathan Sorry for the late reply. Also, the person in charge here has changed from Furudera to Okuno. > Hi, Jonathan > Thanks for you review/comments. > > > On Thu, 16 Jan 2025 04:59:10 +0000 > > Yoshihiro Furudera <fj5100bi@xxxxxxxxxxx> wrote: > > > > > This adds a new dynamic PMU to the Perf Events framework to program > > > and control the Uncore MAC PMUs in Fujitsu chips. > > > > > > This driver was created with reference to drivers/perf/qcom_l3_pmu.c. > > > > > > This driver exports formatting and event information to sysfs so it > > > can be used by the perf user space tools with the syntaxes: > > > > > > perf stat -e mac_iod0_mac0_ch0/ea-mac/ ls perf stat -e > > > mac_iod0_mac0_ch0/event=0x80/ ls > > > > > > FUJITSU-MONAKA Specification URL: > > > https://github.com/fujitsu/FUJITSU-MONAKA > > > > > > Signed-off-by: Yoshihiro Furudera <fj5100bi@xxxxxxxxxxx> > > Hi, > > Other than the docs issue, just minor comments inline. > > > > With the docs adjusted, > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > > I've got it. > > > > > It's been a while since I closely reviewed a PMU driver so I may have > > missed some stuff! > > > > Jonathan > > > > > --- > > > .../admin-guide/perf/fujitsu_mac_pmu.rst | 75 +++ > > > Documentation/admin-guide/perf/index.rst | 1 + > > > drivers/perf/Kconfig | 9 + > > > drivers/perf/Makefile | 1 + > > > drivers/perf/fujitsu_mac_pmu.c | 573 > > ++++++++++++++++++ > > > 5 files changed, 659 insertions(+) > > > create mode 100644 > > > Documentation/admin-guide/perf/fujitsu_mac_pmu.rst > > > create mode 100644 drivers/perf/fujitsu_mac_pmu.c > > > > > > diff --git a/Documentation/admin-guide/perf/fujitsu_mac_pmu.rst > > > b/Documentation/admin-guide/perf/fujitsu_mac_pmu.rst > > > new file mode 100644 > > > index 000000000000..b6676526bde2 > > > --- /dev/null > > > +++ b/Documentation/admin-guide/perf/fujitsu_mac_pmu.rst > > > @@ -0,0 +1,75 @@ > > > > > > +================================================== > > == > > > +Fujitsu Uncore MAC Performance Monitoring Unit (PMU) > > > > > > +================================================== > > == > > > + > > > +This driver supports the Uncore MAC PMUs found in Fujitsu chips. > > > +Each MAC PMU on these chips is exposed as a uncore perf PMU with > > > +device name mac_iod<iod>_mac<mac>_ch<ch>. > > > + > > > +The driver provides a description of its available events and > > > +configuration options in sysfs, see > > /sys/bus/event_sources/devices/mac_iod<iod>_mac<mac>_ch<ch>/. > > > +This driver exports: > > > +- formats, used by perf user space and other tools to configure > > > +events > > > +- events, used by perf user space and other tools to create events > > > + symbolically, e.g.: > > > + perf stat -a -e mac_iod0_mac0_ch0/event=0x21/ ls > > > +- cpumask, used by perf user space and other tools to know on which > > > +CPUs > > > + to open the events > > > + > > > +This driver supports the following events: > > > +- cycles > > > + This event counts MAC cycles at MAC frequency. > > > +- read-count > > > + This event counts the number of read requests to MAC. > > > +- read-count-request > > > + This event counts the number of read requests including retry to MAC. > > > +- read-count-return > > > + This event counts the number of read requests to MAC. > > > +- read-count-request-pftgt > > > + This event counts the number of read requests including retry > > > +with PFTGT > > > + flag. > > > +- read-count-request-normal > > > + This event counts the number of read requests including retry > > > +without PFTGT > > > + flag. > > > +- read-count-return-pftgt-hit > > > + This event counts the number of read requests which hit the PFTGT > buffer. > > > +- read-count-return-pftgt-miss > > > + This event counts the number of read requests which miss the > > > +PFTGT > > buffer. > > > +- read-wait > > > + This event counts outstanding read requests issued by DDR memory > > > +controller > > > + per cycle. > > > +- write-count > > > + This event counts the number of write requests to MAC (including > > > +zero write, > > > + full write, partial write, write cancel). > > > +- write-count-write > > > + This event counts the number of full write requests to MAC (not > > > +including > > > + zero write). > > > +- write-count-pwrite > > > + This event counts the number of partial write requests to MAC. > > > +- memory-read-count > > > + This event counts the number of read requests from MAC to memory. > > > +- memory-write-count > > > + This event counts the number of full write requests from MAC to memory. > > > +- memory-pwrite-count > > > + This event counts the number of partial write requests from MAC > > > +to > > memory. > > > +- ea-mac > > > + This event counts energy consumption of the MAC. > > > +- ea-memory > > > + This event counts energy consumption of the memory. > > > +- ea-memory-mac-read > > > + This event counts the number of read requests from MAC to memory. > > > +- ea-memory-mac-write > > > + This event counts the number of write requests from MAC to memory. > > > +- ea-memory-mac-pwrite > > > + This event counts the number of partial write requests from MAC > > > +to memory > > > > Text identical to memory-pwrite-count > > which suggest two things. > > a) naming inconsistent. Why is mac mentioned here and not in the name > earlier. > > b) This comment is perhaps wrong as I assume has something more tod owtih > with > > energy estimation? > > We are currently checking and will reply later. After checking with the hardware team, the 'ea' events are measured at different points and may therefore return different values. Since memory-pwrite-count and ea-memory-mac-pwrite currently return the same value, they share the same description. However, we have defined distinct event names to accommodate potential future enhancements. > > > +- ea-ha > > > + This event counts energy consumption of the HA. > > > + > > > + 'ea' is the abbreviation for 'Energy Analyzer'. > > > > > diff --git a/drivers/perf/fujitsu_mac_pmu.c > > > b/drivers/perf/fujitsu_mac_pmu.c new file mode 100644 index > > > 000000000000..788bf05b05e7 > > > --- /dev/null > > > +++ b/drivers/perf/fujitsu_mac_pmu.c > > > @@ -0,0 +1,573 @@ > > > +// SPDX-License-Identifier: GPL-2.0-only > > > +/* > > > + * Driver for the Uncore MAC PMUs in Fujitsu chips. > > > + * > > > + * See Documentation/admin-guide/perf/fujitsu_mac_pmu.rst for more > > details. > > > + * > > > + * This driver is based on drivers/perf/qcom_l3_pmu.c > > > + * Copyright (c) 2015-2017, The Linux Foundation. All rights reserved. > > > + * Copyright (c) 2024 Fujitsu. All rights reserved. > > > + */ > > > + > > > +#include <linux/acpi.h> > > > +#include <linux/bitops.h> > > > +#include <linux/interrupt.h> > > > +#include <linux/io.h> > > > +#include <linux/list.h> > > > +#include <linux/module.h> > > > > mod_devicetable.h > > for the acpi device table. > > After checking, I found that > linux/mod_devicetable.h is included in linux/acpi.h. > > > > > > +#include <linux/perf_event.h> > > > +#include <linux/platform_device.h> > > > + > > > +/* Number of counters on each PMU */ #define MAC_NUM_COUNTERS 8 > > > +/* Mask for the event type field within perf_event_attr.config and > > > +EVTYPE reg > > */ > > > +#define MAC_EVTYPE_MASK 0xFF > > > + > > > +/* Perfmon registers */ > > > +#define MAC_PM_EVCNTR(__cntr) (0x000 + __cntr * 8) #define > > > +MAC_PM_CNTCTL(__cntr) (0x100 + __cntr * 8) #define > > > +MAC_PM_EVTYPE(__cntr) (0x200 + __cntr * 8) > > (0x200 + (__cntr) * 8) > > preferred as avoids any possibility of precedence issues if __cntr > > isn't simply a number. > > I'll change __cntr to (__cntr). > > > > > > > > > > +/* > > > + * We must NOT create groups containing events from multiple > > > +hardware PMUs, > > > + * although mixing different software and hardware PMUs is allowed. > > > + */ > > > +static bool fujitsu_mac__validate_event_group(struct perf_event > > > +*event) { > > > + struct perf_event *leader = event->group_leader; > > > + struct perf_event *sibling; > > > + int counters = 0; > > > + > > > + if (leader->pmu != event->pmu && !is_software_event(leader)) > > > + return false; > > > + > > > + /* The sum of the counters used by the event and its leader event */ > > > + counters = 2; > > > + > > > + for_each_sibling_event(sibling, leader) { > > > + if (is_software_event(sibling)) > > > + continue; > > > + if (sibling->pmu != event->pmu) > > > + return false; > > > + counters += 1; > > > > counters++; ? > > I'll change "counters += 1" to "counters++" > > > > > > + } > > > + > > > + /* > > > + * If the group requires more counters than the HW has, it > > > + * cannot ever be scheduled. > > > + */ > > > + return counters <= MAC_NUM_COUNTERS; } > > > > > > > + > > > +#define MAC_PMU_FORMAT_ATTR(_name, _config) > > \ > > > + (&((struct dev_ext_attribute[]) { \ > > > + { .attr = __ATTR(_name, 0444, device_show_string, NULL), > > \ > > > + .var = (void *) _config, } > > \ > > > + })[0].attr.attr) > > > + > > > +static struct attribute *fujitsu_mac_pmu_formats[] = { > > > + MAC_PMU_FORMAT_ATTR(event, "config:0-7"), > > > + NULL > > > +}; > > > + > > > +static const struct attribute_group fujitsu_mac_pmu_format_group = { > > > + .name = "format", > > > + .attrs = fujitsu_mac_pmu_formats > > Add trailing comma. > > I'll add trailing comma. > I'll do the same for the other Non-NULL parts too. > > > > +}; > > > > > + > > > +static const struct attribute_group fujitsu_mac_pmu_events_group = { > > > + .name = "events", > > > + .attrs = fujitsu_mac_pmu_events > > Add trailing comma. There might be other fields in future. > > > > > +}; > > > + > > > +static ssize_t cpumask_show(struct device *dev, > > > + struct device_attribute *attr, char *buf) { > > > + struct mac_pmu *macpmu = to_mac_pmu(dev_get_drvdata(dev)); > > > + > > > + return cpumap_print_to_pagebuf(true, buf, &macpmu->cpumask); } > > > + > > I would drop this blank line to tightly associate the following > > declaration with the function above. > > I'll drop this blank line > > > > > > +static DEVICE_ATTR_RO(cpumask); > > > + > > > > > +static int fujitsu_mac_pmu_probe(struct platform_device *pdev) { > > > + struct device *dev = &pdev->dev; > > > + struct acpi_device *acpi_dev; > > > + struct mac_pmu *macpmu; > > > + struct resource *memrc; > > > + char *name; > > > + int ret; > > > + u64 uid; > > > + > > > + acpi_dev = ACPI_COMPANION(dev); > > > + if (!acpi_dev) > > > + return -ENODEV; > > > + > > > + ret = acpi_dev_uid_to_integer(acpi_dev, &uid); > > > + if (ret) > > > + return dev_err_probe(dev, ret, "unable to read ACPI uid\n"); > > > + > > > + macpmu = devm_kzalloc(dev, sizeof(*macpmu), GFP_KERNEL); > > > + if (!macpmu) > > > + return -ENOMEM; > > > + > > > + name = devm_kasprintf(dev, GFP_KERNEL, > > "mac_iod%llu_mac%llu_ch%llu", > > > + (uid >> 8) & 0xF, (uid >> 4) & 0xF, uid & 0xF); > > > > Slightly novel to encode that much in a UID, but nothing stops you > > doing that so I don't really mind. We always went with additional > > properties for our platforms but this is fine I think. > > Thanks, I'll leave it as is. > > > > > > + if (!name) > > > + return -ENOMEM; > > > + > > > + macpmu->pmu = (struct pmu) { > > > + .parent = dev, > > > + .task_ctx_nr = perf_invalid_context, > > > + > > > + .pmu_enable = fujitsu_mac__pmu_enable, > > > + .pmu_disable = fujitsu_mac__pmu_disable, > > > + .event_init = fujitsu_mac__event_init, > > > + .add = fujitsu_mac__event_add, > > > + .del = fujitsu_mac__event_del, > > > + .start = fujitsu_mac__event_start, > > > + .stop = fujitsu_mac__event_stop, > > > + .read = fujitsu_mac__event_read, > > > + > > > + .attr_groups = fujitsu_mac_pmu_attr_grps, > > > + .capabilities = PERF_PMU_CAP_NO_EXCLUDE > > > + }; > > > + > > > + macpmu->regs = devm_platform_get_and_ioremap_resource(pdev, 0, > > &memrc); > > > + if (IS_ERR(macpmu->regs)) > > > + return PTR_ERR(macpmu->regs); > > > + > > > + fujitsu_mac__init(macpmu); > > > + > > > + ret = platform_get_irq(pdev, 0); > > > + if (ret <= 0) > > > + return ret; > > > > If it were 0 you'd not want to return that as would look like your > > driver probed successfully and none of the devm cleanup would happen. > > > > if (ret < 0) > > is fine - see docs for platform_get_irq() that make it clear 0 is never returned. > > I'll change "if (ret <= 0)" to "if (ret < 0)". > > > > > > > > + > > > + ret = devm_request_irq(dev, ret, fujitsu_mac__handle_irq, 0, > > > + name, macpmu); > > > + if (ret) > > > + return dev_err_probe(dev, ret, "Request for IRQ failed for slice > > > +@%pa\n", > > > > I would wrap this under d of dev. > > > > > + &memrc->start); > > > > Indent of this line is also unusual. > > I'll align the indent to the dev. > > > > > > + > > > + /* Add this instance to the list used by the offline callback */ > > > + ret = cpuhp_state_add_instance(mac_pmu_cpuhp_state, > > &macpmu->node); > > > + if (ret) > > > + return dev_err_probe(dev, ret, "Error registering hotplug"); > > > + > > > + ret = perf_pmu_register(&macpmu->pmu, name, -1); > > > + if (ret < 0) > > > + return dev_err_probe(dev, ret, "Failed to register MAC PMU\n"); > > > + > > > + dev_dbg(dev, "Registered %s, type: %d\n", name, macpmu->pmu.type); > > > + > > > + return 0; > > > +} > > > + > > > +static const struct acpi_device_id fujitsu_mac_pmu_acpi_match[] = { > > > + { "FUJI200C", }, > > > + { } > > > +}; > > > +MODULE_DEVICE_TABLE(acpi, fujitsu_mac_pmu_acpi_match); > > > + > > > +static struct platform_driver fujitsu_mac_pmu_driver = { > > > + .driver = { > > > + .name = "fujitsu-mac-pmu", > > > + .acpi_match_table = fujitsu_mac_pmu_acpi_match, > > > + .suppress_bind_attrs = true > > > > Add trailing comma. Not impossible we will want to set another field > > in here one day. So we should not make that harder. > > > > > + }, > > > + .probe = fujitsu_mac_pmu_probe > > > +}; > > Best Regards, > Yoshihiro Furudera Best Regards, Koichi Okuno
