Messages in this thread Patch in this message | | | Date | Wed, 20 Jul 2022 14:11:50 +0800 | Subject | Re: [RESEND PATCH v2 2/3] drivers/perf: add DDR Sub-System Driveway PMU driver for Yitian 710 SoC | From | Shuai Xue <> |
| |
在 2022/7/19 PM9:19, Jonathan Cameron 写道: > 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.
I see your concern. I will add COMPILE_TEST in Kconfig in next version. Thank you.
> 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
Thank you, Jonathan. I will explicitly include dependent header file in next version.
Best Regards, Shuai
> >> --- >> 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.
Got it. Agree to remove ACPI dependency, widen the compile-test coverage. I prefer leaving ARM64 as ARM_CMN does.
config ARM_CMN tristate "Arm CMN-600 PMU support" depends on ARM64 || COMPILE_TEST help Support for PMU events monitoring on the Arm CMN-600 Coherent Mesh Network interconnect.
>> + default m > > You've addressed this in response to Randy's review.
Yep, will address it in next version. Thank you.
> >> + 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...
Got it, good point. Thank you.
>> +#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
Will add it in next version, thanks.
>> + >> +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.
Will add them in next version, thanks.
>> + 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
Will add it in next version, thanks.
> >> + 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.
Got it. I will remove ACPI dependency and drop ACPI_PTR(). Thank you very much. > >> + {"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, >> +}; >> +
| |