Details

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