Re: [PATCH v1 2/3] drivers/perf: add DesignWare PCIe PMU driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2022-09-26 14:31, Shuai Xue wrote:
+ Bjorn Helgaas

在 2022/9/23 PM11:54, Jonathan Cameron 写道:


+#define RP_NUM_MAX				32 /* 2die * 4RC * 4Ctrol */

This driver is 'almost' generic. So if you an avoid defines based on a particular
platform that's definitely good!

Good idea. How about defining RP_NUM_MAX as 64? As fars as I know,
some platfrom use 2 sockets, 2 die per socket.
Then 2 sockets * 2 dies * 4 Root Complex * 4 root port.

Setting a reasonable maximum is fine - but make sure the code then fails with
a suitable error message if there are more!

OK, I will add a discovery logic here and count PMU number at runtime.



+#define DWC_PCIE_LANE_SHIFT			4
+#define DWC_PCIE_LANE_MASK			GENMASK(9, 4)
+
+#define DWC_PCIE_EVENT_CNT_CTRL			0x8
+#define DWC_PCIE__CNT_EVENT_SELECT_SHIFT	16

Why double __?  If point is , then
naming works better
DWC_PCIE_EVENT_CNT_CTRL_REG
DWC_PCIE_EVENT_CNT_CTRL_EV_SELECT_MSK etc

Yes, I point to use double `__` to indicate it is a field of register,
as CMN and CCN drivers do. I also considered naming with REG explicitly,
but the macro is so long that I often have to wrap code into multilines.
Any way, it's fine to rename if you still suggest to do so.

I don't particularly mind.  This convention was new to me.

Haha, then I will leave the double `__` as CMN and CCN drivers do.

FWIW I'm not sure there's really any convention. CCN seems to use double-underscores as distinct separators in a consistent CCN_REG_NAME__FIELD_NAME__SUFFIX pattern. Conversely in CMN I used it as an indication of the usual CMN_REG_NAME_FIELD_NAME_VALUE pattern being abbreviated where it would have been uncomfortably long otherwise (and particularly where the field name reflects the register name anyway); it just seemed like a good visual cue to imply that something was missing.

Robin.



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux