Details

Reviewers

mhorne
gnn

Group Reviewers

pmc

Commits

rG1459a22787ea: The Arm CoreLink DMC-620 Dynamic Memory Controller PMU driver

Summary

Add the Arm CoreLink DMC-620 Dynamic Memory Controller PMU driver
Add DMC-620 support to hwpmc(4)

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 42426
Build 39314: arc lint + arc unit

Event Timeline

ray created this revision.Oct 26 2021, 12:55 PM

Herald added a subscriber: imp. · View Herald TranscriptOct 26 2021, 12:55 PM

ray requested review of this revision.Oct 26 2021, 12:55 PM

Harbormaster completed remote builds in B42379: Diff 97478.Oct 26 2021, 12:55 PM

ray retitled this revision from the Arm CoreLink DMC-620 Dynamic Memory Controller PMU driver to The Arm CoreLink DMC-620 Dynamic Memory Controller PMU driver.Oct 26 2021, 12:57 PM

ray added subscribers: allanjude, mhorne, emaste, andrew.

ray added a reviewer: pmc.Oct 28 2021, 10:37 PM

Remove debug leftover.

Harbormaster completed remote builds in B42426: Diff 97639.Oct 28 2021, 10:41 PM

Cleanup.

Harbormaster completed remote builds in B42427: Diff 97640.Oct 28 2021, 10:45 PM

Looks good, with some very minor comments.

sys/dev/hwpmc/hwpmc_dmc620.c
164	`c == DMC620_TYPE_CLKDIV2` ?
229
333–334	These capability checks are no longer needed.
555–556
690	Cast not required.
sys/dev/hwpmc/pmu_dmc620.c
233
241

This revision is now accepted and ready to land.Nov 23 2021, 4:42 PM

Oh yes, and one question. Should sys/dev/hwpmc/pmu_dmc620.c be located elsewhere, similar to sys/arm64/arm64/cmn600.c? To me, it does not seem to belong in the hwpmc sub-directory.

In D32670#747725, @mhorne wrote:

Oh yes, and one question. Should sys/dev/hwpmc/pmu_dmc620.c be located elsewhere, similar to sys/arm64/arm64/cmn600.c? To me, it does not seem to belong in the hwpmc sub-directory.

My idea was:
cmn600.c - is a device that provide PMU and trace support. Maybe even some topology information can be read from it.
pmu_dmc620.c - is the part of the DMC-620 device, which is memory controller, but ACPI deliver only its PMU memory and irq config, so ACPI hinted device is exactly PMU. And from another side, everything we have in dev/hwpmc is a drivers of the in-CPU PMUs. So IMO pmu_dmc620.c in correct place :)

Thanks for review!

style(9) changes.

This revision now requires review to proceed.Dec 19 2021, 11:57 AM

Harbormaster completed remote builds in B43473: Diff 100286.Dec 19 2021, 11:57 AM

All fixed. Thanks for review, Mitchell!

In D32670#747730, @ray wrote:

In D32670#747725, @mhorne wrote:

Oh yes, and one question. Should sys/dev/hwpmc/pmu_dmc620.c be located elsewhere, similar to sys/arm64/arm64/cmn600.c? To me, it does not seem to belong in the hwpmc sub-directory.

My idea was:
cmn600.c - is a device that provide PMU and trace support. Maybe even some topology information can be read from it.
pmu_dmc620.c - is the part of the DMC-620 device, which is memory controller, but ACPI deliver only its PMU memory and irq config, so ACPI hinted device is exactly PMU. And from another side, everything we have in dev/hwpmc is a drivers of the in-CPU PMUs. So IMO pmu_dmc620.c in correct place :)

Thanks for review!

Okay, thanks for the rationale. I don't oppose where you've placed it, but as another datapoint, we have sys/arm/arm/pmu.c which is an equivalent in-CPU PMU driver.

This revision is now accepted and ready to land.Jan 5 2022, 4:50 PM

allanjude added a subscriber: gallatin.Feb 10 2022, 4:40 PM

Closed by commit rG1459a22787ea: The Arm CoreLink DMC-620 Dynamic Memory Controller PMU driver (authored by ray, committed by tsoome). · Explain WhyJun 26 2022, 10:21 PM

This revision was automatically updated to reflect the committed changes.

tsoome added a commit: rG1459a22787ea: The Arm CoreLink DMC-620 Dynamic Memory Controller PMU driver.

Herald added a reviewer: gnn. · View Herald TranscriptJun 26 2022, 10:21 PM