RE: [PATCH] PM / devfreq: rockchip-dfi: Add perf support

["Chanwoo Choi" <cw00.choi@samsung.com>]

> -----Original Message-----
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Sent: Thursday, October 19, 2023 3:48 PM
> To: linux-rockchip@lists.infradead.org
> Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> linux-pm@vger.kernel.org; Heiko Stuebner <heiko@sntech.de>; Chanwoo Choi
> <chanwoo@kernel.org>; Kyungmin Park <kyungmin.park@samsung.com>; MyungJoo
> Ham <myungjoo.ham@samsung.com>; Will Deacon <will@kernel.org>; Mark
> Rutland <mark.rutland@arm.com>; kernel@pengutronix.de; Michael Riesch
> <michael.riesch@wolfvision.net>; Robin Murphy <robin.murphy@arm.com>;
> Vincent Legoll <vincent.legoll@gmail.com>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> devicetree@vger.kernel.org; Sebastian Reichel
> <sebastian.reichel@collabora.com>; Sascha Hauer <s.hauer@pengutronix.de>;
> Chanwoo Choi <cw00.choi@samsung.com>
> Subject: [PATCH] PM / devfreq: rockchip-dfi: Add perf support
> 
> The DFI is a unit which is suitable for measuring DDR utilization, but so
> far it could only be used as an event driver for the DDR frequency scaling
> driver. This adds perf support to the DFI driver.
> 
> Usage with the 'perf' tool can look like:
> 
> perf stat -a -e rockchip_ddr/cycles/,\
> 		rockchip_ddr/read-bytes/,\
> 		rockchip_ddr/write-bytes/,\
> 		rockchip_ddr/bytes/ sleep 1
> 
>  Performance counter stats for 'system wide':
> 
>         1582524826      rockchip_ddr/cycles/
>            1802.25 MB   rockchip_ddr/read-bytes/
>            1793.72 MB   rockchip_ddr/write-bytes/
>            3595.90 MB   rockchip_ddr/bytes/
> 
>        1.014369709 seconds time elapsed
> 
> perf support has been tested on a RK3568 and a RK3399, the latter with
> dual channel DDR.
> 
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
> 
> Notes:
>     Changes since v8:
>     - Move rockchip_ddr_perf_counters_add() inside #ifdef
> CONFIG_PERF_EVENTS
>       to avoid unused function warning with CONFIG_PERF_EVENTS disabled
> 
>     Changes since v7:
>      - rename variable 'c' to 'count'
> 
>     Changes since v5:
>      - Add missing initialization of &dfi->last_perf_count
> 
>     Changes since v4:
> 
>      - use __stringify to ensure event type definitions and event numbers
> in sysfs are consistent
>      - only use 64bit values in structs holding counters
>      - support monitoring individual DDR channels
>      - fix return value in rockchip_ddr_perf_event_init(): -EOPNOTSUPP ->
-
> EINVAL
>      - check for invalid event->attr.config values
>      - start hrtimer to trigger in one second, not immediately
>      - use devm_add_action_or_reset()
>      - add suppress_bind_attrs
>      - enable DDRMON during probe when perf is enabled
>      - use a seqlock to protect perf reading the counters from the hrtimer
> callback modifying them
> 
>  drivers/devfreq/event/rockchip-dfi.c | 440 ++++++++++++++++++++++++++-
>  include/soc/rockchip/rk3399_grf.h    |   2 +
>  include/soc/rockchip/rk3568_grf.h    |   1 +
>  3 files changed, 438 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/devfreq/event/rockchip-dfi.c
> b/drivers/devfreq/event/rockchip-dfi.c
> index 3d5c6d737ccd9..a7d7b61518fec 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -16,10 +16,12 @@
>  #include <linux/regmap.h>
>  #include <linux/slab.h>
>  #include <linux/list.h>
> +#include <linux/seqlock.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/bitfield.h>
>  #include <linux/bits.h>
> +#include <linux/perf_event.h>
> 
>  #include <soc/rockchip/rockchip_grf.h>
>  #include <soc/rockchip/rk3399_grf.h>
> @@ -41,19 +43,39 @@
>  					 DDRMON_CTRL_LPDDR4 | \
>  					 DDRMON_CTRL_LPDDR23)
> 
> +#define DDRMON_CH0_WR_NUM		0x20
> +#define DDRMON_CH0_RD_NUM		0x24
>  #define DDRMON_CH0_COUNT_NUM		0x28
>  #define DDRMON_CH0_DFI_ACCESS_NUM	0x2c
>  #define DDRMON_CH1_COUNT_NUM		0x3c
>  #define DDRMON_CH1_DFI_ACCESS_NUM	0x40
> 
> +#define PERF_EVENT_CYCLES		0x0
> +#define PERF_EVENT_READ_BYTES		0x1
> +#define PERF_EVENT_WRITE_BYTES		0x2
> +#define PERF_EVENT_READ_BYTES0		0x3
> +#define PERF_EVENT_WRITE_BYTES0		0x4
> +#define PERF_EVENT_READ_BYTES1		0x5
> +#define PERF_EVENT_WRITE_BYTES1		0x6
> +#define PERF_EVENT_READ_BYTES2		0x7
> +#define PERF_EVENT_WRITE_BYTES2		0x8
> +#define PERF_EVENT_READ_BYTES3		0x9
> +#define PERF_EVENT_WRITE_BYTES3		0xa
> +#define PERF_EVENT_BYTES		0xb
> +#define PERF_ACCESS_TYPE_MAX		0xc
> +
>  /**
>   * struct dmc_count_channel - structure to hold counter values from the
> DDR controller
>   * @access:       Number of read and write accesses
>   * @clock_cycles: DDR clock cycles
> + * @read_access:  number of read accesses
> + * @write_acccess: number of write accesses


Need to change it to 'write_access' from 'write_acccess'.
When I merge it, I fix it by myself.

And,
Applied it with "Acked-by: Heiko Stuebner <heiko@sntech.de>"
according to https://lore.kernel.org/all/27832786.gRfpFWEtPU@diego/.

If you have other opinion, please let me know.

Best Regards,
Chanwoo Choi