lkml.org 
[lkml]   [2022]   [Jul]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [RESEND PATCH v2 2/3] drivers/perf: add DDR Sub-System Driveway PMU driver for Yitian 710 SoC
On Fri, 15 Jul 2022 23:13:09 +0800
Shuai Xue <xueshuai@linux.alibaba.com> wrote:

> Add the DDR Sub-System Driveway Performance Monitoring Unit (PMU) driver
> support for Alibaba T-Head Yitian 710 SoC chip. Yitian supports DDR5/4
> DRAM and targets cloud computing and HPC.
>
> Each PMU is registered as a device in /sys/bus/event_source/devices, and
> users can select event to monitor in each sub-channel, independently. For
> example, ali_drw_21000 and ali_drw_21080 are two PMU devices for two
> sub-channels of the same channel in die 0. And the PMU device of die 1 is
> prefixed with ali_drw_400XXXXX, e.g. ali_drw_40021000.
>
> Due to hardware limitation, one of DDRSS Driveway PMU overflow interrupt
> shares the same irq number with MPAM ERR_IRQ. To register DDRSS PMU and
> MPAM drivers successfully, add IRQF_SHARED flag.
>
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> Co-developed-by: Hongbo Yao <yaohongbo@linux.alibaba.com>
> Signed-off-by: Hongbo Yao <yaohongbo@linux.alibaba.com>
> Co-developed-by: Neng Chen <nengchen@linux.alibaba.com>
> Signed-off-by: Neng Chen <nengchen@linux.alibaba.com>

Hi.

I'd like to see the build constraints relaxed so this gets much more build
coverage from the automated systems. COMPILE_TEST can keep the happy
balance of not showing the driver where it isn't supported, but still ensuring
it gets hit on lots of random config builds.

Only other significant thing is you are relying on a lot of 'implicit' includes.
Don't do that as it just makes it harder to refactor the core headers (which is
an active topic currently as reducing cross includes helps a lot with kernel build
time). Otherwise LGTM. So with those addressed.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks,

Jonathan

> ---
> drivers/perf/Kconfig | 8 +
> drivers/perf/Makefile | 1 +
> drivers/perf/alibaba_uncore_drw_pmu.c | 793 ++++++++++++++++++++++++++
> 3 files changed, 802 insertions(+)
> create mode 100644 drivers/perf/alibaba_uncore_drw_pmu.c
>
> diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> index 1e2d69453771..dfafba4cb066 100644
> --- a/drivers/perf/Kconfig
> +++ b/drivers/perf/Kconfig
> @@ -183,6 +183,14 @@ config APPLE_M1_CPU_PMU
> Provides support for the non-architectural CPU PMUs present on
> the Apple M1 SoCs and derivatives.
>
> +config ALIBABA_UNCORE_DRW_PMU
> + tristate "Alibaba T-Head Yitian 710 DDR Sub-system Driveway PMU driver"
> + depends on (ARM64 && ACPI)

Try to avoid limiting the ability to build the driver. I'm not seeing anything
in here that needs either ARM64 or ACPI to build. If you want to keep those
(I wouldn't) then || COMPILE_TEST
That way you'll get a lot more builds / random configs run against your code
and typically see any issues that occur due to changes elsewhere in the tree
faster than you will if you heavily restrict when your driver is built.

> + default m

You've addressed this in response to Randy's review.

> + help
> + Support for Driveway PMU events monitoring on Yitian 710 DDR
> + Sub-system.
> +
> source "drivers/perf/hisilicon/Kconfig"
>
> config MARVELL_CN10K_DDR_PMU
> diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
> index 57a279c61df5..050d04ee19dd 100644
> --- a/drivers/perf/Makefile
> +++ b/drivers/perf/Makefile
> @@ -20,3 +20,4 @@ obj-$(CONFIG_ARM_DMC620_PMU) += arm_dmc620_pmu.o
> obj-$(CONFIG_MARVELL_CN10K_TAD_PMU) += marvell_cn10k_tad_pmu.o
> obj-$(CONFIG_MARVELL_CN10K_DDR_PMU) += marvell_cn10k_ddr_pmu.o
> obj-$(CONFIG_APPLE_M1_CPU_PMU) += apple_m1_cpu_pmu.o
> +obj-$(CONFIG_ALIBABA_UNCORE_DRW_PMU) += alibaba_uncore_drw_pmu.o
> diff --git a/drivers/perf/alibaba_uncore_drw_pmu.c b/drivers/perf/alibaba_uncore_drw_pmu.c
> new file mode 100644
> index 000000000000..4022bc50ffae
> --- /dev/null
> +++ b/drivers/perf/alibaba_uncore_drw_pmu.c
> @@ -0,0 +1,793 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Alibaba DDR Sub-System Driveway PMU driver
> + *
> + * Copyright (C) 2022 Alibaba Inc
> + */
> +
> +#define ALI_DRW_PMUNAME "ali_drw"
> +#define ALI_DRW_DRVNAME ALI_DRW_PMUNAME "_pmu"
> +#define pr_fmt(fmt) ALI_DRW_DRVNAME ": " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/perf_event.h>
> +#include <linux/platform_device.h>
> +
See below for examples, but you should look closer at the includes to make
sure they are appropriate. Mostly you should not rely on one linux header
including another, but rather provide everything used in a given file.
There are some very generic headers where it is fine to ignore that...

> +#define ALI_DRW_PMU_COMMON_MAX_COUNTERS 16
> +#define ALI_DRW_PMU_TEST_SEL_COMMON_COUNTER_BASE 19
> +
> +#define ALI_DRW_PMU_PA_SHIFT 12
> +#define ALI_DRW_PMU_CNT_INIT 0x00000000
> +#define ALI_DRW_CNT_MAX_PERIOD 0xffffffff
> +#define ALI_DRW_PMU_CYCLE_EVT_ID 0x80
> +
> +#define ALI_DRW_PMU_CNT_CTRL 0xC00
> +#define ALI_DRW_PMU_CNT_RST BIT(2)
> +#define ALI_DRW_PMU_CNT_STOP BIT(1)
> +#define ALI_DRW_PMU_CNT_START BIT(0)
> +
> +#define ALI_DRW_PMU_CNT_STATE 0xC04
> +#define ALI_DRW_PMU_TEST_CTRL 0xC08
> +#define ALI_DRW_PMU_CNT_PRELOAD 0xC0C
> +
> +#define ALI_DRW_PMU_CYCLE_CNT_HIGH_MASK GENMASK(23, 0)
> +#define ALI_DRW_PMU_CYCLE_CNT_LOW_MASK GENMASK(31, 0)
> +#define ALI_DRW_PMU_CYCLE_CNT_HIGH 0xC10
> +#define ALI_DRW_PMU_CYCLE_CNT_LOW 0xC14
> +
> +/* PMU EVENT SEL 0-3 are paired in 32-bit registers on a 4-byte stride */
> +#define ALI_DRW_PMU_EVENT_SEL0 0xC68
> +/* counter 0-3 use sel0, counter 4-7 use sel1...*/
> +#define ALI_DRW_PMU_EVENT_SELn(n) \
> + (ALI_DRW_PMU_EVENT_SEL0 + (n / 4) * 0x4)
> +#define ALI_DRW_PMCOM_CNT_EN BIT(7)
> +#define ALI_DRW_PMCOM_CNT_EVENT_MASK GENMASK(5, 0)
> +#define ALI_DRW_PMCOM_CNT_EVENT_OFFSET(n) \
> + (8 * (n % 4))
> +
> +/* PMU COMMON COUNTER 0-15, are paired in 32-bit registers on a 4-byte stride */
> +#define ALI_DRW_PMU_COMMON_COUNTER0 0xC78
> +#define ALI_DRW_PMU_COMMON_COUNTERn(n) \
> + (ALI_DRW_PMU_COMMON_COUNTER0 + 0x4 * (n))
> +
> +#define ALI_DRW_PMU_OV_INTR_ENABLE_CTL 0xCB8
> +#define ALI_DRW_PMU_OV_INTR_DISABLE_CTL 0xCBC
> +#define ALI_DRW_PMU_OV_INTR_ENABLE_STATUS 0xCC0
> +#define ALI_DRW_PMU_OV_INTR_CLR 0xCC4
> +#define ALI_DRW_PMU_OV_INTR_STATUS 0xCC8
> +#define ALI_DRW_PMCOM_CNT_OV_INTR_MASK GENMASK(23, 8)
> +#define ALI_DRW_PMBW_CNT_OV_INTR_MASK GENMASK(7, 0)
> +#define ALI_DRW_PMU_OV_INTR_MASK GENMASK_ULL(63, 0)
> +
> +static int ali_drw_cpuhp_state_num;
> +
> +static LIST_HEAD(ali_drw_pmu_irqs);
> +static DEFINE_MUTEX(ali_drw_pmu_irqs_lock);

include linux/mutex.h

> +
> +struct ali_drw_pmu_irq {
> + struct hlist_node node;
> + struct list_head irqs_node;
> + struct list_head pmus_node;
Include appropriate headers for list_head, hlist_node etc directly rather than
relying on possibly unstable includes from within other headers.

> + int irq_num;
> + int cpu;
> + refcount_t refcount;
> +};
> +
> +struct ali_drw_pmu {
> + void __iomem *cfg_base;
> + struct device *dev;
> +
> + struct list_head pmus_node;
> + struct ali_drw_pmu_irq *irq;
> + int irq_num;
> + int cpu;
> + DECLARE_BITMAP(used_mask, ALI_DRW_PMU_COMMON_MAX_COUNTERS);

bitmap.h

> + struct perf_event *events[ALI_DRW_PMU_COMMON_MAX_COUNTERS];
> + int evtids[ALI_DRW_PMU_COMMON_MAX_COUNTERS];
> +
> + struct pmu pmu;
> +};
> +

> +
> +/* find a counter for event, then in add func, hw.idx will equal to counter */
> +static int ali_drw_get_counter_idx(struct perf_event *event)
> +{
> + struct ali_drw_pmu *drw_pmu = to_ali_drw_pmu(event->pmu);
> + int idx;
> +
> + for (idx = 0; idx < ALI_DRW_PMU_COMMON_MAX_COUNTERS; ++idx) {
> + if (!test_and_set_bit(idx, drw_pmu->used_mask))
> + return idx;
> + }
> +
> + /* The counters are all in use. */
> + return -EBUSY;
> +}
>


> +
> +/*
> + * Due to historical reasons, the HID used in the production environment is
> + * ARMHD700, so we leave ARMHD700 as Compatible ID.
> + */
> +static const struct acpi_device_id ali_drw_acpi_match[] = {

If you build without ACPI support and drop the requirement for it as suggested
above, this will give an unused warning.
Either drop the ACPI_PTR() - that's very rarely worth bothering or
add a __maybe_unused marking to this array.


> + {"BABA5000", 0},
> + {"ARMHD700", 0},
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, ali_drw_acpi_match);
> +
> +static struct platform_driver ali_drw_pmu_driver = {
> + .driver = {
> + .name = "ali_drw_pmu",
> + .acpi_match_table = ACPI_PTR(ali_drw_acpi_match),
> + },
> + .probe = ali_drw_pmu_probe,
> + .remove = ali_drw_pmu_remove,
> +};
> +

\
 
 \ /
  Last update: 2022-07-19 16:06    [W:0.155 / U:1.692 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site