This is v5 of the series adding perf support to the rockchip DFI driver.
A lot has changed in the perf driver since v4. First of all the review
feedback from Robin and Jonathan has been integrated. The perf driver
now not only supports monitoring the total DDR utilization, but also the
individual channels. I also reworked the way the raw 32bit counter
values are summed up to 64bit perf values, so hopefully the code is
easier to follow now.
lockdep found out that that locking in the perf driver was broken, so I
reworked that as well. None of the perf hooks allows locking with
mutexes or spinlocks, so in perf it's not possible to enable the DFI
controller when needed. Instead I now unconditionally enable the DFI
controller during probe when perf is enabled.
Furthermore the hrtimer I use for reading out the hardware counter
values before they overflow race with perf. Now a seqlock is used to
prevent that.
The RK3588 device tree changes for the DFI were not part of v4. As
Vincent Legoll showed interest in testing this series the necessary
device tree changes are now part of this series.
Changes since v4:
- Add device tree changes for RK3588
- Use seqlock to protect perf counter values from hrtimer
- Unconditionally enable DFI when perf is enabled
- Bring back changes to dts/binding patches that were lost in v4
Changes since v3:
- Add RK3588 support
Changes since v2:
- Fix broken reference to binding
- Add Reviewed-by from Rob
Changes since v1:
- Fix example to actually match the binding and fix the warnings resulted thereof
- Make addition of rockchip,rk3568-dfi an extra patch
Sascha Hauer (25):
PM / devfreq: rockchip-dfi: Make pmu regmap mandatory
PM / devfreq: rockchip-dfi: Embed desc into private data struct
PM / devfreq: rockchip-dfi: use consistent name for private data
struct
PM / devfreq: rockchip-dfi: Add SoC specific init function
PM / devfreq: rockchip-dfi: dfi store raw values in counter struct
PM / devfreq: rockchip-dfi: Use free running counter
PM / devfreq: rockchip-dfi: introduce channel mask
PM / devfreq: rk3399_dmc,dfi: generalize DDRTYPE defines
PM / devfreq: rockchip-dfi: Clean up DDR type register defines
PM / devfreq: rockchip-dfi: Add RK3568 support
PM / devfreq: rockchip-dfi: Handle LPDDR2 correctly
PM / devfreq: rockchip-dfi: Handle LPDDR4X
PM / devfreq: rockchip-dfi: Pass private data struct to internal
functions
PM / devfreq: rockchip-dfi: Prepare for multiple users
PM / devfreq: rockchip-dfi: give variable a better name
PM / devfreq: rockchip-dfi: Add perf support
PM / devfreq: rockchip-dfi: make register stride SoC specific
PM / devfreq: rockchip-dfi: account for multiple DDRMON_CTRL registers
PM / devfreq: rockchip-dfi: add support for RK3588
dt-bindings: devfreq: event: convert Rockchip DFI binding to yaml
dt-bindings: devfreq: event: rockchip,dfi: Add rk3568 support
dt-bindings: devfreq: event: rockchip,dfi: Add rk3588 support
arm64: dts: rockchip: rk3399: Enable DFI
arm64: dts: rockchip: rk356x: Add DFI
arm64: dts: rockchip: rk3588s: Add DFI
.../bindings/devfreq/event/rockchip,dfi.yaml | 84 ++
.../bindings/devfreq/event/rockchip-dfi.txt | 18 -
.../rockchip,rk3399-dmc.yaml | 2 +-
arch/arm64/boot/dts/rockchip/rk3399.dtsi | 1 -
arch/arm64/boot/dts/rockchip/rk356x.dtsi | 7 +
arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 16 +
drivers/devfreq/event/rockchip-dfi.c | 796 +++++++++++++++---
drivers/devfreq/rk3399_dmc.c | 10 +-
include/soc/rockchip/rk3399_grf.h | 9 +-
include/soc/rockchip/rk3568_grf.h | 13 +
include/soc/rockchip/rk3588_grf.h | 18 +
include/soc/rockchip/rockchip_grf.h | 18 +
12 files changed, 854 insertions(+), 138 deletions(-)
create mode 100644 Documentation/devicetree/bindings/devfreq/event/rockchip,dfi.yaml
delete mode 100644 Documentation/devicetree/bindings/devfreq/event/rockchip-dfi.txt
create mode 100644 include/soc/rockchip/rk3568_grf.h
create mode 100644 include/soc/rockchip/rk3588_grf.h
create mode 100644 include/soc/rockchip/rockchip_grf.h
--
2.39.2
This adds the rockchip,rk3568-dfi compatible to the binding. Make clocks
optional for this SoC as the RK3568 doesn't have a kernel controllable
PCLK.
Signed-off-by: Sascha Hauer <[email protected]>
---
.../bindings/devfreq/event/rockchip,dfi.yaml | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/devfreq/event/rockchip,dfi.yaml b/Documentation/devicetree/bindings/devfreq/event/rockchip,dfi.yaml
index 7a82f6ae0701e..e8b64494ee8bd 100644
--- a/Documentation/devicetree/bindings/devfreq/event/rockchip,dfi.yaml
+++ b/Documentation/devicetree/bindings/devfreq/event/rockchip,dfi.yaml
@@ -13,6 +13,7 @@ properties:
compatible:
enum:
- rockchip,rk3399-dfi
+ - rockchip,rk3568-dfi
clocks:
maxItems: 1
@@ -34,11 +35,21 @@ properties:
required:
- compatible
- - clocks
- - clock-names
- interrupts
- reg
+if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - rockchip,rk3399-dfi
+
+then:
+ required:
+ - clocks
+ - clock-names
+
additionalProperties: false
examples:
--
2.39.2
The DDR_MON counters are free running counters. These are resetted to 0
when starting them over like currently done when reading the current
counter values.
Resetting the counters becomes a problem with perf support we want to
add later, because perf needs counters that are not modified elsewhere.
This patch removes resetting the counters and keeps them running
instead. That means we no longer use the absolute counter values but
instead compare them with the counter values we read last time. Not
stopping the counters also has the impact that they are running while
we are reading them. We cannot read multiple timers atomically, so
the values do not exactly fit together. The effect should be negligible
though as the time between two measurements is some orders of magnitude
bigger than the time we need to read multiple registers.
Signed-off-by: Sascha Hauer <[email protected]>
---
Notes:
Changes since v4:
- rephrase commit message
- Drop unused variable
drivers/devfreq/event/rockchip-dfi.c | 52 ++++++++++++++++------------
1 file changed, 30 insertions(+), 22 deletions(-)
diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 680f629da64fc..126bb744645b6 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -38,11 +38,15 @@
#define DDRMON_CH1_COUNT_NUM 0x3c
#define DDRMON_CH1_DFI_ACCESS_NUM 0x40
-struct dmc_usage {
+struct dmc_count_channel {
u32 access;
u32 total;
};
+struct dmc_count {
+ struct dmc_count_channel c[RK3399_DMC_NUM_CH];
+};
+
/*
* The dfi controller can monitor DDR load. It has an upper and lower threshold
* for the operating points. Whenever the usage leaves these bounds an event is
@@ -51,7 +55,7 @@ struct dmc_usage {
struct rockchip_dfi {
struct devfreq_event_dev *edev;
struct devfreq_event_desc desc;
- struct dmc_usage ch_usage[RK3399_DMC_NUM_CH];
+ struct dmc_count last_event_count;
struct device *dev;
void __iomem *regs;
struct regmap *regmap_pmu;
@@ -85,30 +89,18 @@ static void rockchip_dfi_stop_hardware_counter(struct devfreq_event_dev *edev)
writel_relaxed(SOFTWARE_DIS, dfi_regs + DDRMON_CTRL);
}
-static int rockchip_dfi_get_busier_ch(struct devfreq_event_dev *edev)
+static void rockchip_dfi_read_counters(struct devfreq_event_dev *edev, struct dmc_count *count)
{
struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
- u32 tmp, max = 0;
- u32 i, busier_ch = 0;
+ u32 i;
void __iomem *dfi_regs = dfi->regs;
- rockchip_dfi_stop_hardware_counter(edev);
-
- /* Find out which channel is busier */
for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
- dfi->ch_usage[i].access = readl_relaxed(dfi_regs +
+ count->c[i].access = readl_relaxed(dfi_regs +
DDRMON_CH0_DFI_ACCESS_NUM + i * 20);
- dfi->ch_usage[i].total = readl_relaxed(dfi_regs +
+ count->c[i].total = readl_relaxed(dfi_regs +
DDRMON_CH0_COUNT_NUM + i * 20);
- tmp = dfi->ch_usage[i].access;
- if (tmp > max) {
- busier_ch = i;
- max = tmp;
- }
}
- rockchip_dfi_start_hardware_counter(edev);
-
- return busier_ch;
}
static int rockchip_dfi_disable(struct devfreq_event_dev *edev)
@@ -145,12 +137,28 @@ static int rockchip_dfi_get_event(struct devfreq_event_dev *edev,
struct devfreq_event_data *edata)
{
struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
- int busier_ch;
+ struct dmc_count count;
+ struct dmc_count *last = &dfi->last_event_count;
+ u32 access = 0, total = 0;
+ int i;
+
+ rockchip_dfi_read_counters(edev, &count);
+
+ /* We can only report one channel, so find the busiest one */
+ for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
+ u32 a = count.c[i].access - last->c[i].access;
+ u32 t = count.c[i].total - last->c[i].total;
+
+ if (a > access) {
+ access = a;
+ total = t;
+ }
+ }
- busier_ch = rockchip_dfi_get_busier_ch(edev);
+ edata->load_count = access * 4;
+ edata->total_count = total;
- edata->load_count = dfi->ch_usage[busier_ch].access * 4;
- edata->total_count = dfi->ch_usage[busier_ch].total;
+ dfi->last_event_count = count;
return 0;
}
--
2.39.2
As a matter of fact the regmap_pmu already is mandatory because
it is used unconditionally in the driver. Bail out gracefully in
probe() rather than crashing later.
Fixes: b9d1262bca0af ("PM / devfreq: event: support rockchip dfi controller")
Signed-off-by: Sascha Hauer <[email protected]>
---
Notes:
Changes since v4:
- move to beginning of the series to make it easier to backport to stable
- Add a Fixes: tag
- add missing of_node_put()
drivers/devfreq/event/rockchip-dfi.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 39ac069cabc75..74893c06aa087 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -193,14 +193,15 @@ static int rockchip_dfi_probe(struct platform_device *pdev)
return dev_err_probe(dev, PTR_ERR(data->clk),
"Cannot get the clk pclk_ddr_mon\n");
- /* try to find the optional reference to the pmu syscon */
node = of_parse_phandle(np, "rockchip,pmu", 0);
- if (node) {
- data->regmap_pmu = syscon_node_to_regmap(node);
- of_node_put(node);
- if (IS_ERR(data->regmap_pmu))
- return PTR_ERR(data->regmap_pmu);
- }
+ if (!node)
+ return dev_err_probe(&pdev->dev, -ENODEV, "Can't find pmu_grf registers\n");
+
+ data->regmap_pmu = syscon_node_to_regmap(node);
+ of_node_put(node);
+ if (IS_ERR(data->regmap_pmu))
+ return PTR_ERR(data->regmap_pmu);
+
data->dev = dev;
desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
--
2.39.2
Move the RK3399 specifics to a SoC specific init function to make
the way free for supporting other SoCs later.
Signed-off-by: Sascha Hauer <[email protected]>
---
Notes:
Changes since v4:
- use of_device_get_match_data()
- use a callback rather than a struct type as driver data
drivers/devfreq/event/rockchip-dfi.c | 48 +++++++++++++++++++---------
1 file changed, 33 insertions(+), 15 deletions(-)
diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index e19e5acaa362c..6b1ef29df7048 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -17,6 +17,7 @@
#include <linux/slab.h>
#include <linux/list.h>
#include <linux/of.h>
+#include <linux/of_device.h>
#include <soc/rockchip/rk3399_grf.h>
@@ -55,27 +56,21 @@ struct rockchip_dfi {
void __iomem *regs;
struct regmap *regmap_pmu;
struct clk *clk;
+ u32 ddr_type;
};
static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
{
struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
void __iomem *dfi_regs = dfi->regs;
- u32 val;
- u32 ddr_type;
-
- /* get ddr type */
- regmap_read(dfi->regmap_pmu, RK3399_PMUGRF_OS_REG2, &val);
- ddr_type = (val >> RK3399_PMUGRF_DDRTYPE_SHIFT) &
- RK3399_PMUGRF_DDRTYPE_MASK;
/* clear DDRMON_CTRL setting */
writel_relaxed(CLR_DDRMON_CTRL, dfi_regs + DDRMON_CTRL);
/* set ddr type to dfi */
- if (ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR3)
+ if (dfi->ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR3)
writel_relaxed(LPDDR3_EN, dfi_regs + DDRMON_CTRL);
- else if (ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR4)
+ else if (dfi->ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR4)
writel_relaxed(LPDDR4_EN, dfi_regs + DDRMON_CTRL);
/* enable count, use software mode */
@@ -167,8 +162,26 @@ static const struct devfreq_event_ops rockchip_dfi_ops = {
.set_event = rockchip_dfi_set_event,
};
+static int rk3399_dfi_init(struct rockchip_dfi *dfi)
+{
+ struct regmap *regmap_pmu = dfi->regmap_pmu;
+ u32 val;
+
+ dfi->clk = devm_clk_get(dfi->dev, "pclk_ddr_mon");
+ if (IS_ERR(dfi->clk))
+ return dev_err_probe(dfi->dev, PTR_ERR(dfi->clk),
+ "Cannot get the clk pclk_ddr_mon\n");
+
+ /* get ddr type */
+ regmap_read(regmap_pmu, RK3399_PMUGRF_OS_REG2, &val);
+ dfi->ddr_type = (val >> RK3399_PMUGRF_DDRTYPE_SHIFT) &
+ RK3399_PMUGRF_DDRTYPE_MASK;
+
+ return 0;
+};
+
static const struct of_device_id rockchip_dfi_id_match[] = {
- { .compatible = "rockchip,rk3399-dfi" },
+ { .compatible = "rockchip,rk3399-dfi", .data = rk3399_dfi_init },
{ },
};
MODULE_DEVICE_TABLE(of, rockchip_dfi_id_match);
@@ -179,6 +192,12 @@ static int rockchip_dfi_probe(struct platform_device *pdev)
struct rockchip_dfi *dfi;
struct devfreq_event_desc *desc;
struct device_node *np = pdev->dev.of_node, *node;
+ int (*soc_init)(struct rockchip_dfi *dfi);
+ int ret;
+
+ soc_init = of_device_get_match_data(&pdev->dev);
+ if (!soc_init)
+ return -EINVAL;
dfi = devm_kzalloc(dev, sizeof(*dfi), GFP_KERNEL);
if (!dfi)
@@ -188,11 +207,6 @@ static int rockchip_dfi_probe(struct platform_device *pdev)
if (IS_ERR(dfi->regs))
return PTR_ERR(dfi->regs);
- dfi->clk = devm_clk_get(dev, "pclk_ddr_mon");
- if (IS_ERR(dfi->clk))
- return dev_err_probe(dev, PTR_ERR(dfi->clk),
- "Cannot get the clk pclk_ddr_mon\n");
-
node = of_parse_phandle(np, "rockchip,pmu", 0);
if (!node)
return dev_err_probe(&pdev->dev, -ENODEV, "Can't find pmu_grf registers\n");
@@ -209,6 +223,10 @@ static int rockchip_dfi_probe(struct platform_device *pdev)
desc->driver_data = dfi;
desc->name = np->name;
+ ret = soc_init(dfi);
+ if (ret)
+ return ret;
+
dfi->edev = devm_devfreq_event_add_edev(&pdev->dev, desc);
if (IS_ERR(dfi->edev)) {
dev_err(&pdev->dev,
--
2.39.2
When adding perf support to the DFI driver the perf part will
need the raw counter values, so move the fixed * 4 factor to
rockchip_dfi_get_event().
Signed-off-by: Sascha Hauer <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
---
drivers/devfreq/event/rockchip-dfi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 6b1ef29df7048..680f629da64fc 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -97,7 +97,7 @@ static int rockchip_dfi_get_busier_ch(struct devfreq_event_dev *edev)
/* Find out which channel is busier */
for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
dfi->ch_usage[i].access = readl_relaxed(dfi_regs +
- DDRMON_CH0_DFI_ACCESS_NUM + i * 20) * 4;
+ DDRMON_CH0_DFI_ACCESS_NUM + i * 20);
dfi->ch_usage[i].total = readl_relaxed(dfi_regs +
DDRMON_CH0_COUNT_NUM + i * 20);
tmp = dfi->ch_usage[i].access;
@@ -149,7 +149,7 @@ static int rockchip_dfi_get_event(struct devfreq_event_dev *edev,
busier_ch = rockchip_dfi_get_busier_ch(edev);
- edata->load_count = dfi->ch_usage[busier_ch].access;
+ edata->load_count = dfi->ch_usage[busier_ch].access * 4;
edata->total_count = dfi->ch_usage[busier_ch].total;
return 0;
--
2.39.2
The internal functions do not need the struct devfreq_event_dev *,
so pass them the struct rockchip_dfi *. This is a preparation for
adding perf support later which doesn't have a struct devfreq_event_dev *.
Signed-off-by: Sascha Hauer <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
---
drivers/devfreq/event/rockchip-dfi.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 0a568c5551699..d39db5de7f19c 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -72,9 +72,8 @@ struct rockchip_dfi {
unsigned int channel_mask;
};
-static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
+static void rockchip_dfi_start_hardware_counter(struct rockchip_dfi *dfi)
{
- struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
void __iomem *dfi_regs = dfi->regs;
/* clear DDRMON_CTRL setting */
@@ -102,18 +101,16 @@ static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
dfi_regs + DDRMON_CTRL);
}
-static void rockchip_dfi_stop_hardware_counter(struct devfreq_event_dev *edev)
+static void rockchip_dfi_stop_hardware_counter(struct rockchip_dfi *dfi)
{
- struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
void __iomem *dfi_regs = dfi->regs;
writel_relaxed(HIWORD_UPDATE(0, DDRMON_CTRL_SOFTWARE_EN),
dfi_regs + DDRMON_CTRL);
}
-static void rockchip_dfi_read_counters(struct devfreq_event_dev *edev, struct dmc_count *count)
+static void rockchip_dfi_read_counters(struct rockchip_dfi *dfi, struct dmc_count *count)
{
- struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
u32 i;
void __iomem *dfi_regs = dfi->regs;
@@ -131,7 +128,7 @@ static int rockchip_dfi_disable(struct devfreq_event_dev *edev)
{
struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
- rockchip_dfi_stop_hardware_counter(edev);
+ rockchip_dfi_stop_hardware_counter(dfi);
clk_disable_unprepare(dfi->clk);
return 0;
@@ -148,7 +145,7 @@ static int rockchip_dfi_enable(struct devfreq_event_dev *edev)
return ret;
}
- rockchip_dfi_start_hardware_counter(edev);
+ rockchip_dfi_start_hardware_counter(dfi);
return 0;
}
@@ -166,7 +163,7 @@ static int rockchip_dfi_get_event(struct devfreq_event_dev *edev,
u32 access = 0, total = 0;
int i;
- rockchip_dfi_read_counters(edev, &count);
+ rockchip_dfi_read_counters(dfi, &count);
/* We can only report one channel, so find the busiest one */
for (i = 0; i < DMC_MAX_CHANNELS; i++) {
--
2.39.2
This adds rockchip,rk3588-dfi to the list of compatibles. Unlike ealier
SoCs the rk3588 has four interrupts (one for each channel) instead of
only one, so increase the number of allowed interrupts to four and also
add interrupt-names.
Signed-off-by: Sascha Hauer <[email protected]>
---
Notes:
Changes since v4:
- new patch
.../bindings/devfreq/event/rockchip,dfi.yaml | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/devfreq/event/rockchip,dfi.yaml b/Documentation/devicetree/bindings/devfreq/event/rockchip,dfi.yaml
index e8b64494ee8bd..4e647a9560560 100644
--- a/Documentation/devicetree/bindings/devfreq/event/rockchip,dfi.yaml
+++ b/Documentation/devicetree/bindings/devfreq/event/rockchip,dfi.yaml
@@ -14,6 +14,7 @@ properties:
enum:
- rockchip,rk3399-dfi
- rockchip,rk3568-dfi
+ - rockchip,rk3588-dfi
clocks:
maxItems: 1
@@ -23,7 +24,18 @@ properties:
- const: pclk_ddr_mon
interrupts:
- maxItems: 1
+ minItems: 1
+ maxItems: 4
+
+ interrupt-names:
+ oneOf:
+ - items:
+ - const: ch0
+ - items:
+ - const: ch0
+ - const: ch1
+ - const: ch2
+ - const: ch3
reg:
maxItems: 1
--
2.39.2
the DFI unit can provide useful data for measuring DDR utilization
and works without any configuration from the board, so enable it in the
dtsi file directly.
Signed-off-by: Sascha Hauer <[email protected]>
---
arch/arm64/boot/dts/rockchip/rk3399.dtsi | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index 928948e7c7bbb..fa0a5dbd1b0ec 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -1331,7 +1331,6 @@ dfi: dfi@ff630000 {
interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH 0>;
clocks = <&cru PCLK_DDR_MON>;
clock-names = "pclk_ddr_mon";
- status = "disabled";
};
vpu: video-codec@ff650000 {
--
2.39.2
In the DFI driver LPDDR4X can be handled in the same way as LPDDR4. Add
the missing case.
Signed-off-by: Sascha Hauer <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
---
drivers/devfreq/event/rockchip-dfi.c | 1 +
include/soc/rockchip/rockchip_grf.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 16cd5365671f7..0a568c5551699 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -89,6 +89,7 @@ static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
dfi_regs + DDRMON_CTRL);
break;
case ROCKCHIP_DDRTYPE_LPDDR4:
+ case ROCKCHIP_DDRTYPE_LPDDR4X:
writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_LPDDR4, DDRMON_CTRL_DDR_TYPE_MASK),
dfi_regs + DDRMON_CTRL);
break;
diff --git a/include/soc/rockchip/rockchip_grf.h b/include/soc/rockchip/rockchip_grf.h
index dde1a9796ccb5..e46fd72aea8d1 100644
--- a/include/soc/rockchip/rockchip_grf.h
+++ b/include/soc/rockchip/rockchip_grf.h
@@ -12,6 +12,7 @@ enum {
ROCKCHIP_DDRTYPE_LPDDR2 = 5,
ROCKCHIP_DDRTYPE_LPDDR3 = 6,
ROCKCHIP_DDRTYPE_LPDDR4 = 7,
+ ROCKCHIP_DDRTYPE_LPDDR4X = 8,
};
#endif /* __SOC_ROCKCHIP_GRF_H */
--
2.39.2
When adding perf support later the DFI must be enabled when
either of devfreq-event or perf is active. Prepare for that
by adding a usage counter for the DFI. Also move enabling
and disabling of the clock away from the devfreq-event specific
functions to which the perf specific part won't have access.
Signed-off-by: Sascha Hauer <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
---
drivers/devfreq/event/rockchip-dfi.c | 57 +++++++++++++++++++---------
1 file changed, 40 insertions(+), 17 deletions(-)
diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index d39db5de7f19c..8a7af7c32ae0d 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -68,13 +68,28 @@ struct rockchip_dfi {
void __iomem *regs;
struct regmap *regmap_pmu;
struct clk *clk;
+ int usecount;
+ struct mutex mutex;
u32 ddr_type;
unsigned int channel_mask;
};
-static void rockchip_dfi_start_hardware_counter(struct rockchip_dfi *dfi)
+static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
{
void __iomem *dfi_regs = dfi->regs;
+ int ret = 0;
+
+ mutex_lock(&dfi->mutex);
+
+ dfi->usecount++;
+ if (dfi->usecount > 1)
+ goto out;
+
+ ret = clk_prepare_enable(dfi->clk);
+ if (ret) {
+ dev_err(&dfi->edev->dev, "failed to enable dfi clk: %d\n", ret);
+ goto out;
+ }
/* clear DDRMON_CTRL setting */
writel_relaxed(HIWORD_UPDATE(0, DDRMON_CTRL_TIMER_CNT_EN | DDRMON_CTRL_SOFTWARE_EN |
@@ -99,14 +114,30 @@ static void rockchip_dfi_start_hardware_counter(struct rockchip_dfi *dfi)
/* enable count, use software mode */
writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_SOFTWARE_EN, DDRMON_CTRL_SOFTWARE_EN),
dfi_regs + DDRMON_CTRL);
+out:
+ mutex_unlock(&dfi->mutex);
+
+ return ret;
}
-static void rockchip_dfi_stop_hardware_counter(struct rockchip_dfi *dfi)
+static void rockchip_dfi_disable(struct rockchip_dfi *dfi)
{
void __iomem *dfi_regs = dfi->regs;
+ mutex_lock(&dfi->mutex);
+
+ dfi->usecount--;
+
+ WARN_ON_ONCE(dfi->usecount < 0);
+
+ if (dfi->usecount > 0)
+ goto out;
+
writel_relaxed(HIWORD_UPDATE(0, DDRMON_CTRL_SOFTWARE_EN),
dfi_regs + DDRMON_CTRL);
+ clk_disable_unprepare(dfi->clk);
+out:
+ mutex_unlock(&dfi->mutex);
}
static void rockchip_dfi_read_counters(struct rockchip_dfi *dfi, struct dmc_count *count)
@@ -124,29 +155,20 @@ static void rockchip_dfi_read_counters(struct rockchip_dfi *dfi, struct dmc_coun
}
}
-static int rockchip_dfi_disable(struct devfreq_event_dev *edev)
+static int rockchip_dfi_event_disable(struct devfreq_event_dev *edev)
{
struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
- rockchip_dfi_stop_hardware_counter(dfi);
- clk_disable_unprepare(dfi->clk);
+ rockchip_dfi_disable(dfi);
return 0;
}
-static int rockchip_dfi_enable(struct devfreq_event_dev *edev)
+static int rockchip_dfi_event_enable(struct devfreq_event_dev *edev)
{
struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
- int ret;
-
- ret = clk_prepare_enable(dfi->clk);
- if (ret) {
- dev_err(&edev->dev, "failed to enable dfi clk: %d\n", ret);
- return ret;
- }
- rockchip_dfi_start_hardware_counter(dfi);
- return 0;
+ return rockchip_dfi_enable(dfi);
}
static int rockchip_dfi_set_event(struct devfreq_event_dev *edev)
@@ -190,8 +212,8 @@ static int rockchip_dfi_get_event(struct devfreq_event_dev *edev,
}
static const struct devfreq_event_ops rockchip_dfi_ops = {
- .disable = rockchip_dfi_disable,
- .enable = rockchip_dfi_enable,
+ .disable = rockchip_dfi_event_disable,
+ .enable = rockchip_dfi_event_enable,
.get_event = rockchip_dfi_get_event,
.set_event = rockchip_dfi_set_event,
};
@@ -272,6 +294,7 @@ static int rockchip_dfi_probe(struct platform_device *pdev)
return PTR_ERR(dfi->regmap_pmu);
dfi->dev = dev;
+ mutex_init(&dfi->mutex);
desc = &dfi->desc;
desc->ops = &rockchip_dfi_ops;
--
2.39.2
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.
Signed-off-by: Sascha Hauer <[email protected]>
---
Notes:
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 | 439 ++++++++++++++++++++++++++-
include/soc/rockchip/rk3399_grf.h | 2 +
include/soc/rockchip/rk3568_grf.h | 1 +
3 files changed, 437 insertions(+), 5 deletions(-)
diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 50e497455dc69..88145688e3d9c 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
*/
struct dmc_count_channel {
- u32 access;
- u32 clock_cycles;
+ u64 access;
+ u64 clock_cycles;
+ u64 read_access;
+ u64 write_access;
};
struct dmc_count {
@@ -69,6 +91,11 @@ struct rockchip_dfi {
struct devfreq_event_dev *edev;
struct devfreq_event_desc desc;
struct dmc_count last_event_count;
+
+ struct dmc_count last_perf_count;
+ struct dmc_count total_count;
+ seqlock_t count_seqlock; /* protects last_perf_count and total_count */
+
struct device *dev;
void __iomem *regs;
struct regmap *regmap_pmu;
@@ -77,6 +104,14 @@ struct rockchip_dfi {
struct mutex mutex;
u32 ddr_type;
unsigned int channel_mask;
+ enum cpuhp_state cpuhp_state;
+ struct hlist_node node;
+ struct pmu pmu;
+ struct hrtimer timer;
+ unsigned int cpu;
+ int active_events;
+ int burst_len;
+ int buswidth[DMC_MAX_CHANNELS];
};
static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
@@ -145,7 +180,7 @@ static void rockchip_dfi_disable(struct rockchip_dfi *dfi)
mutex_unlock(&dfi->mutex);
}
-static void rockchip_dfi_read_counters(struct rockchip_dfi *dfi, struct dmc_count *count)
+static void rockchip_dfi_read_counters(struct rockchip_dfi *dfi, struct dmc_count *c)
{
u32 i;
void __iomem *dfi_regs = dfi->regs;
@@ -153,13 +188,36 @@ static void rockchip_dfi_read_counters(struct rockchip_dfi *dfi, struct dmc_coun
for (i = 0; i < DMC_MAX_CHANNELS; i++) {
if (!(dfi->channel_mask & BIT(i)))
continue;
- count->c[i].access = readl_relaxed(dfi_regs +
+ c->c[i].read_access = readl_relaxed(dfi_regs +
+ DDRMON_CH0_RD_NUM + i * 20);
+ c->c[i].write_access = readl_relaxed(dfi_regs +
+ DDRMON_CH0_WR_NUM + i * 20);
+ c->c[i].access = readl_relaxed(dfi_regs +
DDRMON_CH0_DFI_ACCESS_NUM + i * 20);
- count->c[i].clock_cycles = readl_relaxed(dfi_regs +
+ c->c[i].clock_cycles = readl_relaxed(dfi_regs +
DDRMON_CH0_COUNT_NUM + i * 20);
}
}
+static void rockchip_ddr_perf_counters_add(struct rockchip_dfi *dfi,
+ const struct dmc_count *now,
+ struct dmc_count *res)
+{
+ const struct dmc_count *last = &dfi->last_perf_count;
+ int i;
+
+ for (i = 0; i < DMC_MAX_CHANNELS; i++) {
+ res->c[i].read_access = dfi->total_count.c[i].read_access +
+ (u32)(now->c[i].read_access - last->c[i].read_access);
+ res->c[i].write_access = dfi->total_count.c[i].write_access +
+ (u32)(now->c[i].write_access - last->c[i].write_access);
+ res->c[i].access = dfi->total_count.c[i].access +
+ (u32)(now->c[i].access - last->c[i].access);
+ res->c[i].clock_cycles = dfi->total_count.c[i].clock_cycles +
+ (u32)(now->c[i].clock_cycles - last->c[i].clock_cycles);
+ }
+}
+
static int rockchip_dfi_event_disable(struct devfreq_event_dev *edev)
{
struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
@@ -223,6 +281,367 @@ static const struct devfreq_event_ops rockchip_dfi_ops = {
.set_event = rockchip_dfi_set_event,
};
+#ifdef CONFIG_PERF_EVENTS
+
+static ssize_t ddr_perf_cpumask_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct pmu *pmu = dev_get_drvdata(dev);
+ struct rockchip_dfi *dfi = container_of(pmu, struct rockchip_dfi, pmu);
+
+ return cpumap_print_to_pagebuf(true, buf, cpumask_of(dfi->cpu));
+}
+
+static struct device_attribute ddr_perf_cpumask_attr =
+ __ATTR(cpumask, 0444, ddr_perf_cpumask_show, NULL);
+
+static struct attribute *ddr_perf_cpumask_attrs[] = {
+ &ddr_perf_cpumask_attr.attr,
+ NULL,
+};
+
+static const struct attribute_group ddr_perf_cpumask_attr_group = {
+ .attrs = ddr_perf_cpumask_attrs,
+};
+
+PMU_EVENT_ATTR_STRING(cycles, ddr_pmu_cycles, "event="__stringify(PERF_EVENT_CYCLES))
+
+#define DFI_PMU_EVENT_ATTR(_name, _var, _str) \
+ PMU_EVENT_ATTR_STRING(_name, _var, _str); \
+ PMU_EVENT_ATTR_STRING(_name.unit, _var##_unit, "MB"); \
+ PMU_EVENT_ATTR_STRING(_name.scale, _var##_scale, "9.536743164e-07")
+
+DFI_PMU_EVENT_ATTR(read-bytes0, ddr_pmu_read_bytes0, "event="__stringify(PERF_EVENT_READ_BYTES0));
+DFI_PMU_EVENT_ATTR(write-bytes0, ddr_pmu_write_bytes0, "event="__stringify(PERF_EVENT_WRITE_BYTES0));
+
+DFI_PMU_EVENT_ATTR(read-bytes1, ddr_pmu_read_bytes1, "event="__stringify(PERF_EVENT_READ_BYTES1));
+DFI_PMU_EVENT_ATTR(write-bytes1, ddr_pmu_write_bytes1, "event="__stringify(PERF_EVENT_WRITE_BYTES1));
+
+DFI_PMU_EVENT_ATTR(read-bytes2, ddr_pmu_read_bytes2, "event="__stringify(PERF_EVENT_READ_BYTES2));
+DFI_PMU_EVENT_ATTR(write-bytes2, ddr_pmu_write_bytes2, "event="__stringify(PERF_EVENT_WRITE_BYTES2));
+
+DFI_PMU_EVENT_ATTR(read-bytes3, ddr_pmu_read_bytes3, "event="__stringify(PERF_EVENT_READ_BYTES3));
+DFI_PMU_EVENT_ATTR(write-bytes3, ddr_pmu_write_bytes3, "event="__stringify(PERF_EVENT_WRITE_BYTES3));
+
+DFI_PMU_EVENT_ATTR(read-bytes, ddr_pmu_read_bytes, "event="__stringify(PERF_EVENT_READ_BYTES));
+DFI_PMU_EVENT_ATTR(write-bytes, ddr_pmu_write_bytes, "event="__stringify(PERF_EVENT_WRITE_BYTES));
+
+DFI_PMU_EVENT_ATTR(bytes, ddr_pmu_bytes, "event="__stringify(PERF_EVENT_BYTES));
+
+#define DFI_ATTR_MB(_name) \
+ &_name.attr.attr, \
+ &_name##_unit.attr.attr, \
+ &_name##_scale.attr.attr
+
+static struct attribute *ddr_perf_events_attrs[] = {
+ &ddr_pmu_cycles.attr.attr,
+ DFI_ATTR_MB(ddr_pmu_read_bytes),
+ DFI_ATTR_MB(ddr_pmu_write_bytes),
+ DFI_ATTR_MB(ddr_pmu_read_bytes0),
+ DFI_ATTR_MB(ddr_pmu_write_bytes0),
+ DFI_ATTR_MB(ddr_pmu_read_bytes1),
+ DFI_ATTR_MB(ddr_pmu_write_bytes1),
+ DFI_ATTR_MB(ddr_pmu_read_bytes2),
+ DFI_ATTR_MB(ddr_pmu_write_bytes2),
+ DFI_ATTR_MB(ddr_pmu_read_bytes3),
+ DFI_ATTR_MB(ddr_pmu_write_bytes3),
+ DFI_ATTR_MB(ddr_pmu_bytes),
+ NULL,
+};
+
+static const struct attribute_group ddr_perf_events_attr_group = {
+ .name = "events",
+ .attrs = ddr_perf_events_attrs,
+};
+
+PMU_FORMAT_ATTR(event, "config:0-7");
+
+static struct attribute *ddr_perf_format_attrs[] = {
+ &format_attr_event.attr,
+ NULL,
+};
+
+static const struct attribute_group ddr_perf_format_attr_group = {
+ .name = "format",
+ .attrs = ddr_perf_format_attrs,
+};
+
+static const struct attribute_group *attr_groups[] = {
+ &ddr_perf_events_attr_group,
+ &ddr_perf_cpumask_attr_group,
+ &ddr_perf_format_attr_group,
+ NULL,
+};
+
+static int rockchip_ddr_perf_event_init(struct perf_event *event)
+{
+ struct rockchip_dfi *dfi = container_of(event->pmu, struct rockchip_dfi, pmu);
+
+ if (event->attr.type != event->pmu->type)
+ return -ENOENT;
+
+ if (event->attach_state & PERF_ATTACH_TASK)
+ return -EINVAL;
+
+ if (event->cpu < 0) {
+ dev_warn(dfi->dev, "Can't provide per-task data!\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static u64 rockchip_ddr_perf_event_get_count(struct perf_event *event)
+{
+ struct rockchip_dfi *dfi = container_of(event->pmu, struct rockchip_dfi, pmu);
+ int blen = dfi->burst_len;
+ struct dmc_count total, now;
+ unsigned int seq;
+ u64 c = 0;
+ int i;
+
+ rockchip_dfi_read_counters(dfi, &now);
+
+ do {
+ seq = read_seqbegin(&dfi->count_seqlock);
+
+ rockchip_ddr_perf_counters_add(dfi, &now, &total);
+
+ } while (read_seqretry(&dfi->count_seqlock, seq));
+
+ switch (event->attr.config) {
+ case PERF_EVENT_CYCLES:
+ c = total.c[0].clock_cycles;
+ break;
+ case PERF_EVENT_READ_BYTES:
+ for (i = 0; i < DMC_MAX_CHANNELS; i++)
+ c += total.c[i].read_access * blen * dfi->buswidth[i];
+ break;
+ case PERF_EVENT_WRITE_BYTES:
+ for (i = 0; i < DMC_MAX_CHANNELS; i++)
+ c += total.c[i].write_access * blen * dfi->buswidth[i];
+ break;
+ case PERF_EVENT_READ_BYTES0:
+ c = total.c[0].read_access * blen * dfi->buswidth[0];
+ break;
+ case PERF_EVENT_WRITE_BYTES0:
+ c = total.c[0].write_access * blen * dfi->buswidth[0];
+ break;
+ case PERF_EVENT_READ_BYTES1:
+ c = total.c[1].read_access * blen * dfi->buswidth[1];
+ break;
+ case PERF_EVENT_WRITE_BYTES1:
+ c = total.c[1].write_access * blen * dfi->buswidth[1];
+ break;
+ case PERF_EVENT_READ_BYTES2:
+ c = total.c[2].read_access * blen * dfi->buswidth[2];
+ break;
+ case PERF_EVENT_WRITE_BYTES2:
+ c = total.c[2].write_access * blen * dfi->buswidth[2];
+ break;
+ case PERF_EVENT_READ_BYTES3:
+ c = total.c[3].read_access * blen * dfi->buswidth[3];
+ break;
+ case PERF_EVENT_WRITE_BYTES3:
+ c = total.c[3].write_access * blen * dfi->buswidth[3];
+ break;
+ case PERF_EVENT_BYTES:
+ for (i = 0; i < DMC_MAX_CHANNELS; i++)
+ c += total.c[i].access * blen * dfi->buswidth[i];
+ break;
+ }
+
+ return c;
+}
+
+static void rockchip_ddr_perf_event_update(struct perf_event *event)
+{
+ u64 now;
+ s64 prev;
+
+ if (event->attr.config >= PERF_ACCESS_TYPE_MAX)
+ return;
+
+ now = rockchip_ddr_perf_event_get_count(event);
+ prev = local64_xchg(&event->hw.prev_count, now);
+ local64_add(now - prev, &event->count);
+}
+
+static void rockchip_ddr_perf_event_start(struct perf_event *event, int flags)
+{
+ u64 now = rockchip_ddr_perf_event_get_count(event);
+
+ local64_set(&event->hw.prev_count, now);
+}
+
+static int rockchip_ddr_perf_event_add(struct perf_event *event, int flags)
+{
+ struct rockchip_dfi *dfi = container_of(event->pmu, struct rockchip_dfi, pmu);
+
+ dfi->active_events++;
+
+ if (dfi->active_events == 1)
+ hrtimer_start(&dfi->timer, ns_to_ktime(NSEC_PER_SEC), HRTIMER_MODE_REL);
+
+ if (flags & PERF_EF_START)
+ rockchip_ddr_perf_event_start(event, flags);
+
+ return 0;
+}
+
+static void rockchip_ddr_perf_event_stop(struct perf_event *event, int flags)
+{
+ rockchip_ddr_perf_event_update(event);
+}
+
+static void rockchip_ddr_perf_event_del(struct perf_event *event, int flags)
+{
+ struct rockchip_dfi *dfi = container_of(event->pmu, struct rockchip_dfi, pmu);
+
+ rockchip_ddr_perf_event_stop(event, PERF_EF_UPDATE);
+
+ dfi->active_events--;
+
+ if (dfi->active_events == 0)
+ hrtimer_cancel(&dfi->timer);
+}
+
+static enum hrtimer_restart rockchip_dfi_timer(struct hrtimer *timer)
+{
+ struct rockchip_dfi *dfi = container_of(timer, struct rockchip_dfi, timer);
+ struct dmc_count now, total;
+
+ rockchip_dfi_read_counters(dfi, &now);
+
+ write_seqlock(&dfi->count_seqlock);
+
+ rockchip_ddr_perf_counters_add(dfi, &now, &total);
+ dfi->total_count = total;
+ dfi->last_perf_count = now;
+
+ write_sequnlock(&dfi->count_seqlock);
+
+ hrtimer_forward_now(&dfi->timer, ns_to_ktime(NSEC_PER_SEC));
+
+ return HRTIMER_RESTART;
+};
+
+static int ddr_perf_offline_cpu(unsigned int cpu, struct hlist_node *node)
+{
+ struct rockchip_dfi *dfi = hlist_entry_safe(node, struct rockchip_dfi, node);
+ int target;
+
+ if (cpu != dfi->cpu)
+ return 0;
+
+ target = cpumask_any_but(cpu_online_mask, cpu);
+ if (target >= nr_cpu_ids)
+ return 0;
+
+ perf_pmu_migrate_context(&dfi->pmu, cpu, target);
+ dfi->cpu = target;
+
+ return 0;
+}
+
+static void rockchip_ddr_cpuhp_remove_state(void *data)
+{
+ struct rockchip_dfi *dfi = data;
+
+ cpuhp_remove_multi_state(dfi->cpuhp_state);
+
+ rockchip_dfi_disable(dfi);
+}
+
+static void rockchip_ddr_cpuhp_remove_instance(void *data)
+{
+ struct rockchip_dfi *dfi = data;
+
+ cpuhp_state_remove_instance_nocalls(dfi->cpuhp_state, &dfi->node);
+}
+
+static void rockchip_ddr_perf_remove(void *data)
+{
+ struct rockchip_dfi *dfi = data;
+
+ perf_pmu_unregister(&dfi->pmu);
+}
+
+static int rockchip_ddr_perf_init(struct rockchip_dfi *dfi)
+{
+ struct pmu *pmu = &dfi->pmu;
+ int ret;
+
+ seqlock_init(&dfi->count_seqlock);
+
+ pmu->module = THIS_MODULE;
+ pmu->capabilities = PERF_PMU_CAP_NO_EXCLUDE;
+ pmu->task_ctx_nr = perf_invalid_context;
+ pmu->attr_groups = attr_groups;
+ pmu->event_init = rockchip_ddr_perf_event_init;
+ pmu->add = rockchip_ddr_perf_event_add;
+ pmu->del = rockchip_ddr_perf_event_del;
+ pmu->start = rockchip_ddr_perf_event_start;
+ pmu->stop = rockchip_ddr_perf_event_stop;
+ pmu->read = rockchip_ddr_perf_event_update;
+
+ dfi->cpu = raw_smp_processor_id();
+
+ ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN,
+ "rockchip_ddr_perf_pmu",
+ NULL,
+ ddr_perf_offline_cpu);
+
+ if (ret < 0) {
+ dev_err(dfi->dev, "cpuhp_setup_state_multi failed: %d\n", ret);
+ return ret;
+ }
+
+ dfi->cpuhp_state = ret;
+
+ rockchip_dfi_enable(dfi);
+
+ ret = devm_add_action_or_reset(dfi->dev, rockchip_ddr_cpuhp_remove_state, dfi);
+ if (ret)
+ return ret;
+
+ ret = cpuhp_state_add_instance_nocalls(dfi->cpuhp_state, &dfi->node);
+ if (ret) {
+ dev_err(dfi->dev, "Error %d registering hotplug\n", ret);
+ return ret;
+ }
+
+ ret = devm_add_action_or_reset(dfi->dev, rockchip_ddr_cpuhp_remove_instance, dfi);
+ if (ret)
+ return ret;
+
+ hrtimer_init(&dfi->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+ dfi->timer.function = rockchip_dfi_timer;
+
+ switch (dfi->ddr_type) {
+ case ROCKCHIP_DDRTYPE_LPDDR2:
+ case ROCKCHIP_DDRTYPE_LPDDR3:
+ dfi->burst_len = 8;
+ break;
+ case ROCKCHIP_DDRTYPE_LPDDR4:
+ case ROCKCHIP_DDRTYPE_LPDDR4X:
+ dfi->burst_len = 16;
+ break;
+ }
+
+ ret = perf_pmu_register(pmu, "rockchip_ddr", -1);
+ if (ret)
+ return ret;
+
+ return devm_add_action_or_reset(dfi->dev, rockchip_ddr_perf_remove, dfi);
+}
+#else
+static int rockchip_ddr_perf_init(struct rockchip_dfi *dfi)
+{
+ return 0;
+}
+#endif
+
static int rk3399_dfi_init(struct rockchip_dfi *dfi)
{
struct regmap *regmap_pmu = dfi->regmap_pmu;
@@ -239,6 +658,9 @@ static int rk3399_dfi_init(struct rockchip_dfi *dfi)
dfi->channel_mask = GENMASK(1, 0);
+ dfi->buswidth[0] = FIELD_GET(RK3399_PMUGRF_OS_REG2_BW_CH0, val) == 0 ? 4 : 2;
+ dfi->buswidth[1] = FIELD_GET(RK3399_PMUGRF_OS_REG2_BW_CH1, val) == 0 ? 4 : 2;
+
return 0;
};
@@ -255,6 +677,8 @@ static int rk3568_dfi_init(struct rockchip_dfi *dfi)
if (FIELD_GET(RK3568_PMUGRF_OS_REG3_SYSREG_VERSION, reg3) >= 0x3)
dfi->ddr_type |= FIELD_GET(RK3568_PMUGRF_OS_REG3_DRAMTYPE_INFO_V3, reg3) << 3;
+ dfi->buswidth[0] = FIELD_GET(RK3568_PMUGRF_OS_REG2_BW_CH0, reg2) == 0 ? 4 : 2;
+
dfi->channel_mask = 1;
return 0;
@@ -317,6 +741,10 @@ static int rockchip_dfi_probe(struct platform_device *pdev)
return PTR_ERR(dfi->edev);
}
+ ret = rockchip_ddr_perf_init(dfi);
+ if (ret)
+ return ret;
+
platform_set_drvdata(pdev, dfi);
return 0;
@@ -327,6 +755,7 @@ static struct platform_driver rockchip_dfi_driver = {
.driver = {
.name = "rockchip-dfi",
.of_match_table = rockchip_dfi_id_match,
+ .suppress_bind_attrs = true,
},
};
module_platform_driver(rockchip_dfi_driver);
diff --git a/include/soc/rockchip/rk3399_grf.h b/include/soc/rockchip/rk3399_grf.h
index 775f8444bea8d..39cd44cec982f 100644
--- a/include/soc/rockchip/rk3399_grf.h
+++ b/include/soc/rockchip/rk3399_grf.h
@@ -12,5 +12,7 @@
/* PMU GRF Registers */
#define RK3399_PMUGRF_OS_REG2 0x308
#define RK3399_PMUGRF_OS_REG2_DDRTYPE GENMASK(15, 13)
+#define RK3399_PMUGRF_OS_REG2_BW_CH0 GENMASK(3, 2)
+#define RK3399_PMUGRF_OS_REG2_BW_CH1 GENMASK(19, 18)
#endif
diff --git a/include/soc/rockchip/rk3568_grf.h b/include/soc/rockchip/rk3568_grf.h
index 575584e9d8834..52853efd6720e 100644
--- a/include/soc/rockchip/rk3568_grf.h
+++ b/include/soc/rockchip/rk3568_grf.h
@@ -4,6 +4,7 @@
#define RK3568_PMUGRF_OS_REG2 0x208
#define RK3568_PMUGRF_OS_REG2_DRAMTYPE_INFO GENMASK(15, 13)
+#define RK3568_PMUGRF_OS_REG2_BW_CH0 GENMASK(3, 2)
#define RK3568_PMUGRF_OS_REG3 0x20c
#define RK3568_PMUGRF_OS_REG3_DRAMTYPE_INFO_V3 GENMASK(13, 12)
--
2.39.2
Different Rockchip SoC variants have a different number of channels.
Introduce a channel mask to make the number of channels configurable
from SoC initialization code.
Signed-off-by: Sascha Hauer <[email protected]>
---
drivers/devfreq/event/rockchip-dfi.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 126bb744645b6..82de24a027579 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -18,10 +18,11 @@
#include <linux/list.h>
#include <linux/of.h>
#include <linux/of_device.h>
+#include <linux/bits.h>
#include <soc/rockchip/rk3399_grf.h>
-#define RK3399_DMC_NUM_CH 2
+#define DMC_MAX_CHANNELS 2
/* DDRMON_CTRL */
#define DDRMON_CTRL 0x04
@@ -44,7 +45,7 @@ struct dmc_count_channel {
};
struct dmc_count {
- struct dmc_count_channel c[RK3399_DMC_NUM_CH];
+ struct dmc_count_channel c[DMC_MAX_CHANNELS];
};
/*
@@ -61,6 +62,7 @@ struct rockchip_dfi {
struct regmap *regmap_pmu;
struct clk *clk;
u32 ddr_type;
+ unsigned int channel_mask;
};
static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
@@ -95,7 +97,9 @@ static void rockchip_dfi_read_counters(struct devfreq_event_dev *edev, struct dm
u32 i;
void __iomem *dfi_regs = dfi->regs;
- for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
+ for (i = 0; i < DMC_MAX_CHANNELS; i++) {
+ if (!(dfi->channel_mask & BIT(i)))
+ continue;
count->c[i].access = readl_relaxed(dfi_regs +
DDRMON_CH0_DFI_ACCESS_NUM + i * 20);
count->c[i].total = readl_relaxed(dfi_regs +
@@ -145,9 +149,14 @@ static int rockchip_dfi_get_event(struct devfreq_event_dev *edev,
rockchip_dfi_read_counters(edev, &count);
/* We can only report one channel, so find the busiest one */
- for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
- u32 a = count.c[i].access - last->c[i].access;
- u32 t = count.c[i].total - last->c[i].total;
+ for (i = 0; i < DMC_MAX_CHANNELS; i++) {
+ u32 a, t;
+
+ if (!(dfi->channel_mask & BIT(i)))
+ continue;
+
+ a = count.c[i].access - last->c[i].access;
+ t = count.c[i].total - last->c[i].total;
if (a > access) {
access = a;
@@ -185,6 +194,8 @@ static int rk3399_dfi_init(struct rockchip_dfi *dfi)
dfi->ddr_type = (val >> RK3399_PMUGRF_DDRTYPE_SHIFT) &
RK3399_PMUGRF_DDRTYPE_MASK;
+ dfi->channel_mask = GENMASK(1, 0);
+
return 0;
};
--
2.39.2
The DDRTYPE defines are named to be RK3399 specific, but they can be
used for other Rockchip SoCs as well, so replace the RK3399_PMUGRF_
prefix with ROCKCHIP_. They are defined in a SoC specific header
file, so when generalizing the prefix also move the new defines to
a SoC agnostic header file. While at it use GENMASK to define the
DDRTYPE bitfield and give it a name including the full register name.
Signed-off-by: Sascha Hauer <[email protected]>
---
drivers/devfreq/event/rockchip-dfi.c | 9 +++++----
drivers/devfreq/rk3399_dmc.c | 10 +++++-----
include/soc/rockchip/rk3399_grf.h | 7 +------
include/soc/rockchip/rockchip_grf.h | 17 +++++++++++++++++
4 files changed, 28 insertions(+), 15 deletions(-)
create mode 100644 include/soc/rockchip/rockchip_grf.h
diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 82de24a027579..6bccb6fbcfc0c 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -18,8 +18,10 @@
#include <linux/list.h>
#include <linux/of.h>
#include <linux/of_device.h>
+#include <linux/bitfield.h>
#include <linux/bits.h>
+#include <soc/rockchip/rockchip_grf.h>
#include <soc/rockchip/rk3399_grf.h>
#define DMC_MAX_CHANNELS 2
@@ -74,9 +76,9 @@ static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
writel_relaxed(CLR_DDRMON_CTRL, dfi_regs + DDRMON_CTRL);
/* set ddr type to dfi */
- if (dfi->ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR3)
+ if (dfi->ddr_type == ROCKCHIP_DDRTYPE_LPDDR3)
writel_relaxed(LPDDR3_EN, dfi_regs + DDRMON_CTRL);
- else if (dfi->ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR4)
+ else if (dfi->ddr_type == ROCKCHIP_DDRTYPE_LPDDR4)
writel_relaxed(LPDDR4_EN, dfi_regs + DDRMON_CTRL);
/* enable count, use software mode */
@@ -191,8 +193,7 @@ static int rk3399_dfi_init(struct rockchip_dfi *dfi)
/* get ddr type */
regmap_read(regmap_pmu, RK3399_PMUGRF_OS_REG2, &val);
- dfi->ddr_type = (val >> RK3399_PMUGRF_DDRTYPE_SHIFT) &
- RK3399_PMUGRF_DDRTYPE_MASK;
+ dfi->ddr_type = FIELD_GET(RK3399_PMUGRF_OS_REG2_DDRTYPE, val);
dfi->channel_mask = GENMASK(1, 0);
diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
index daff407026157..fd2c5ffedf41e 100644
--- a/drivers/devfreq/rk3399_dmc.c
+++ b/drivers/devfreq/rk3399_dmc.c
@@ -22,6 +22,7 @@
#include <linux/suspend.h>
#include <soc/rockchip/pm_domains.h>
+#include <soc/rockchip/rockchip_grf.h>
#include <soc/rockchip/rk3399_grf.h>
#include <soc/rockchip/rockchip_sip.h>
@@ -381,17 +382,16 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
}
regmap_read(data->regmap_pmu, RK3399_PMUGRF_OS_REG2, &val);
- ddr_type = (val >> RK3399_PMUGRF_DDRTYPE_SHIFT) &
- RK3399_PMUGRF_DDRTYPE_MASK;
+ ddr_type = FIELD_GET(RK3399_PMUGRF_OS_REG2_DDRTYPE, val);
switch (ddr_type) {
- case RK3399_PMUGRF_DDRTYPE_DDR3:
+ case ROCKCHIP_DDRTYPE_DDR3:
data->odt_dis_freq = data->ddr3_odt_dis_freq;
break;
- case RK3399_PMUGRF_DDRTYPE_LPDDR3:
+ case ROCKCHIP_DDRTYPE_LPDDR3:
data->odt_dis_freq = data->lpddr3_odt_dis_freq;
break;
- case RK3399_PMUGRF_DDRTYPE_LPDDR4:
+ case ROCKCHIP_DDRTYPE_LPDDR4:
data->odt_dis_freq = data->lpddr4_odt_dis_freq;
break;
default:
diff --git a/include/soc/rockchip/rk3399_grf.h b/include/soc/rockchip/rk3399_grf.h
index 3eebabcb28123..775f8444bea8d 100644
--- a/include/soc/rockchip/rk3399_grf.h
+++ b/include/soc/rockchip/rk3399_grf.h
@@ -11,11 +11,6 @@
/* PMU GRF Registers */
#define RK3399_PMUGRF_OS_REG2 0x308
-#define RK3399_PMUGRF_DDRTYPE_SHIFT 13
-#define RK3399_PMUGRF_DDRTYPE_MASK 7
-#define RK3399_PMUGRF_DDRTYPE_DDR3 3
-#define RK3399_PMUGRF_DDRTYPE_LPDDR2 5
-#define RK3399_PMUGRF_DDRTYPE_LPDDR3 6
-#define RK3399_PMUGRF_DDRTYPE_LPDDR4 7
+#define RK3399_PMUGRF_OS_REG2_DDRTYPE GENMASK(15, 13)
#endif
diff --git a/include/soc/rockchip/rockchip_grf.h b/include/soc/rockchip/rockchip_grf.h
new file mode 100644
index 0000000000000..dde1a9796ccb5
--- /dev/null
+++ b/include/soc/rockchip/rockchip_grf.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Rockchip General Register Files definitions
+ */
+
+#ifndef __SOC_ROCKCHIP_GRF_H
+#define __SOC_ROCKCHIP_GRF_H
+
+/* Rockchip DDRTYPE defines */
+enum {
+ ROCKCHIP_DDRTYPE_DDR3 = 3,
+ ROCKCHIP_DDRTYPE_LPDDR2 = 5,
+ ROCKCHIP_DDRTYPE_LPDDR3 = 6,
+ ROCKCHIP_DDRTYPE_LPDDR4 = 7,
+};
+
+#endif /* __SOC_ROCKCHIP_GRF_H */
--
2.39.2
The currently supported RK3399 has a set of registers per channel, but
it has only a single DDRMON_CTRL register. With upcoming RK3588 this
will be different, the RK3588 has a DDRMON_CTRL register per channel.
Instead of expecting a single DDRMON_CTRL register, loop over the
channels and write the channel specific DDRMON_CTRL register. Break
out early out of the loop when there is only a single DDRMON_CTRL
register like on the RK3399.
Signed-off-by: Sascha Hauer <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
---
drivers/devfreq/event/rockchip-dfi.c | 72 ++++++++++++++++++----------
1 file changed, 48 insertions(+), 24 deletions(-)
diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index a872550a7caf5..23d66fe737975 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -113,12 +113,13 @@ struct rockchip_dfi {
int burst_len;
int buswidth[DMC_MAX_CHANNELS];
int ddrmon_stride;
+ bool ddrmon_ctrl_single;
};
static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
{
void __iomem *dfi_regs = dfi->regs;
- int ret = 0;
+ int i, ret = 0;
mutex_lock(&dfi->mutex);
@@ -132,29 +133,41 @@ static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
goto out;
}
- /* clear DDRMON_CTRL setting */
- writel_relaxed(HIWORD_UPDATE(0, DDRMON_CTRL_TIMER_CNT_EN | DDRMON_CTRL_SOFTWARE_EN |
- DDRMON_CTRL_HARDWARE_EN), dfi_regs + DDRMON_CTRL);
+ for (i = 0; i < DMC_MAX_CHANNELS; i++) {
+ u32 ctrl = 0;
- /* set ddr type to dfi */
- switch (dfi->ddr_type) {
- case ROCKCHIP_DDRTYPE_LPDDR2:
- case ROCKCHIP_DDRTYPE_LPDDR3:
- writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_LPDDR23, DDRMON_CTRL_DDR_TYPE_MASK),
- dfi_regs + DDRMON_CTRL);
- break;
- case ROCKCHIP_DDRTYPE_LPDDR4:
- case ROCKCHIP_DDRTYPE_LPDDR4X:
- writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_LPDDR4, DDRMON_CTRL_DDR_TYPE_MASK),
- dfi_regs + DDRMON_CTRL);
- break;
- default:
- break;
- }
+ if (!(dfi->channel_mask & BIT(i)))
+ continue;
- /* enable count, use software mode */
- writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_SOFTWARE_EN, DDRMON_CTRL_SOFTWARE_EN),
- dfi_regs + DDRMON_CTRL);
+ /* clear DDRMON_CTRL setting */
+ writel_relaxed(HIWORD_UPDATE(0, DDRMON_CTRL_TIMER_CNT_EN |
+ DDRMON_CTRL_SOFTWARE_EN | DDRMON_CTRL_HARDWARE_EN),
+ dfi_regs + i * dfi->ddrmon_stride + DDRMON_CTRL);
+
+ /* set ddr type to dfi */
+ switch (dfi->ddr_type) {
+ case ROCKCHIP_DDRTYPE_LPDDR2:
+ case ROCKCHIP_DDRTYPE_LPDDR3:
+ ctrl = DDRMON_CTRL_LPDDR23;
+ break;
+ case ROCKCHIP_DDRTYPE_LPDDR4:
+ case ROCKCHIP_DDRTYPE_LPDDR4X:
+ ctrl = DDRMON_CTRL_LPDDR4;
+ break;
+ default:
+ break;
+ }
+
+ writel_relaxed(HIWORD_UPDATE(ctrl, DDRMON_CTRL_DDR_TYPE_MASK),
+ dfi_regs + i * dfi->ddrmon_stride + DDRMON_CTRL);
+
+ /* enable count, use software mode */
+ writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_SOFTWARE_EN, DDRMON_CTRL_SOFTWARE_EN),
+ dfi_regs + i * dfi->ddrmon_stride + DDRMON_CTRL);
+
+ if (dfi->ddrmon_ctrl_single)
+ break;
+ }
out:
mutex_unlock(&dfi->mutex);
@@ -164,6 +177,7 @@ static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
static void rockchip_dfi_disable(struct rockchip_dfi *dfi)
{
void __iomem *dfi_regs = dfi->regs;
+ int i;
mutex_lock(&dfi->mutex);
@@ -174,8 +188,17 @@ static void rockchip_dfi_disable(struct rockchip_dfi *dfi)
if (dfi->usecount > 0)
goto out;
- writel_relaxed(HIWORD_UPDATE(0, DDRMON_CTRL_SOFTWARE_EN),
- dfi_regs + DDRMON_CTRL);
+ for (i = 0; i < DMC_MAX_CHANNELS; i++) {
+ if (!(dfi->channel_mask & BIT(i)))
+ continue;
+
+ writel_relaxed(HIWORD_UPDATE(0, DDRMON_CTRL_SOFTWARE_EN),
+ dfi_regs + i * dfi->ddrmon_stride + DDRMON_CTRL);
+
+ if (dfi->ddrmon_ctrl_single)
+ break;
+ }
+
clk_disable_unprepare(dfi->clk);
out:
mutex_unlock(&dfi->mutex);
@@ -663,6 +686,7 @@ static int rk3399_dfi_init(struct rockchip_dfi *dfi)
dfi->buswidth[1] = FIELD_GET(RK3399_PMUGRF_OS_REG2_BW_CH1, val) == 0 ? 4 : 2;
dfi->ddrmon_stride = 0x14;
+ dfi->ddrmon_ctrl_single = true;
return 0;
};
--
2.39.2
No need for an extra allocation, just embed the struct
devfreq_event_desc into the private data struct.
Signed-off-by: Sascha Hauer <[email protected]>
Reviewed-by: Heiko Stuebner <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
---
drivers/devfreq/event/rockchip-dfi.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 74893c06aa087..467f9f42d38f7 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -49,7 +49,7 @@ struct dmc_usage {
*/
struct rockchip_dfi {
struct devfreq_event_dev *edev;
- struct devfreq_event_desc *desc;
+ struct devfreq_event_desc desc;
struct dmc_usage ch_usage[RK3399_DMC_NUM_CH];
struct device *dev;
void __iomem *regs;
@@ -204,14 +204,10 @@ static int rockchip_dfi_probe(struct platform_device *pdev)
data->dev = dev;
- desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
- if (!desc)
- return -ENOMEM;
-
+ desc = &data->desc;
desc->ops = &rockchip_dfi_ops;
desc->driver_data = data;
desc->name = np->name;
- data->desc = desc;
data->edev = devm_devfreq_event_add_edev(&pdev->dev, desc);
if (IS_ERR(data->edev)) {
--
2.39.2
According to the downstream driver the DDRMON_CTRL_LPDDR23 bit must be
set for both LPDDR2 and LPDDR3. Add the missing LPDDR2 case and while
at it turn the if/else if/else into switch/case which makes it easier
to read.
Signed-off-by: Sascha Hauer <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
---
drivers/devfreq/event/rockchip-dfi.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 261d112580c9e..16cd5365671f7 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -82,12 +82,19 @@ static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
DDRMON_CTRL_HARDWARE_EN), dfi_regs + DDRMON_CTRL);
/* set ddr type to dfi */
- if (dfi->ddr_type == ROCKCHIP_DDRTYPE_LPDDR3)
+ switch (dfi->ddr_type) {
+ case ROCKCHIP_DDRTYPE_LPDDR2:
+ case ROCKCHIP_DDRTYPE_LPDDR3:
writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_LPDDR23, DDRMON_CTRL_DDR_TYPE_MASK),
dfi_regs + DDRMON_CTRL);
- else if (dfi->ddr_type == ROCKCHIP_DDRTYPE_LPDDR4)
+ break;
+ case ROCKCHIP_DDRTYPE_LPDDR4:
writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_LPDDR4, DDRMON_CTRL_DDR_TYPE_MASK),
dfi_regs + DDRMON_CTRL);
+ break;
+ default:
+ break;
+ }
/* enable count, use software mode */
writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_SOFTWARE_EN, DDRMON_CTRL_SOFTWARE_EN),
--
2.39.2
The DFI unit can be used to measure DRAM utilization using perf. Add the
node to the device tree.
Signed-off-by: Sascha Hauer <[email protected]>
---
arch/arm64/boot/dts/rockchip/rk356x.dtsi | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
index f62e0fd881a95..910d8a84ea8f2 100644
--- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
@@ -948,6 +948,13 @@ qos_vop_m1: qos@fe1a8100 {
reg = <0x0 0xfe1a8100 0x0 0x20>;
};
+ dfi: dfi@fe230000 {
+ compatible = "rockchip,rk3568-dfi";
+ reg = <0x00 0xfe230000 0x00 0x400>;
+ interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
+ rockchip,pmu = <&pmugrf>;
+ };
+
pcie2x1: pcie@fe260000 {
compatible = "rockchip,rk3568-pcie";
reg = <0x3 0xc0000000 0x0 0x00400000>,
--
2.39.2
The variable name for the private data struct is 'info' in some
functions and 'data' in others. Both names do not give a clue what
type the variable has, so consistently use 'dfi'.
Signed-off-by: Sascha Hauer <[email protected]>
Reviewed-by: Heiko Stuebner <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
---
drivers/devfreq/event/rockchip-dfi.c | 72 ++++++++++++++--------------
1 file changed, 36 insertions(+), 36 deletions(-)
diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 467f9f42d38f7..e19e5acaa362c 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -59,13 +59,13 @@ struct rockchip_dfi {
static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
{
- struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
- void __iomem *dfi_regs = info->regs;
+ struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
+ void __iomem *dfi_regs = dfi->regs;
u32 val;
u32 ddr_type;
/* get ddr type */
- regmap_read(info->regmap_pmu, RK3399_PMUGRF_OS_REG2, &val);
+ regmap_read(dfi->regmap_pmu, RK3399_PMUGRF_OS_REG2, &val);
ddr_type = (val >> RK3399_PMUGRF_DDRTYPE_SHIFT) &
RK3399_PMUGRF_DDRTYPE_MASK;
@@ -84,28 +84,28 @@ static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
static void rockchip_dfi_stop_hardware_counter(struct devfreq_event_dev *edev)
{
- struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
- void __iomem *dfi_regs = info->regs;
+ struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
+ void __iomem *dfi_regs = dfi->regs;
writel_relaxed(SOFTWARE_DIS, dfi_regs + DDRMON_CTRL);
}
static int rockchip_dfi_get_busier_ch(struct devfreq_event_dev *edev)
{
- struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
+ struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
u32 tmp, max = 0;
u32 i, busier_ch = 0;
- void __iomem *dfi_regs = info->regs;
+ void __iomem *dfi_regs = dfi->regs;
rockchip_dfi_stop_hardware_counter(edev);
/* Find out which channel is busier */
for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
- info->ch_usage[i].access = readl_relaxed(dfi_regs +
+ dfi->ch_usage[i].access = readl_relaxed(dfi_regs +
DDRMON_CH0_DFI_ACCESS_NUM + i * 20) * 4;
- info->ch_usage[i].total = readl_relaxed(dfi_regs +
+ dfi->ch_usage[i].total = readl_relaxed(dfi_regs +
DDRMON_CH0_COUNT_NUM + i * 20);
- tmp = info->ch_usage[i].access;
+ tmp = dfi->ch_usage[i].access;
if (tmp > max) {
busier_ch = i;
max = tmp;
@@ -118,20 +118,20 @@ static int rockchip_dfi_get_busier_ch(struct devfreq_event_dev *edev)
static int rockchip_dfi_disable(struct devfreq_event_dev *edev)
{
- struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
+ struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
rockchip_dfi_stop_hardware_counter(edev);
- clk_disable_unprepare(info->clk);
+ clk_disable_unprepare(dfi->clk);
return 0;
}
static int rockchip_dfi_enable(struct devfreq_event_dev *edev)
{
- struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
+ struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
int ret;
- ret = clk_prepare_enable(info->clk);
+ ret = clk_prepare_enable(dfi->clk);
if (ret) {
dev_err(&edev->dev, "failed to enable dfi clk: %d\n", ret);
return ret;
@@ -149,13 +149,13 @@ static int rockchip_dfi_set_event(struct devfreq_event_dev *edev)
static int rockchip_dfi_get_event(struct devfreq_event_dev *edev,
struct devfreq_event_data *edata)
{
- struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
+ struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
int busier_ch;
busier_ch = rockchip_dfi_get_busier_ch(edev);
- edata->load_count = info->ch_usage[busier_ch].access;
- edata->total_count = info->ch_usage[busier_ch].total;
+ edata->load_count = dfi->ch_usage[busier_ch].access;
+ edata->total_count = dfi->ch_usage[busier_ch].total;
return 0;
}
@@ -176,47 +176,47 @@ MODULE_DEVICE_TABLE(of, rockchip_dfi_id_match);
static int rockchip_dfi_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
- struct rockchip_dfi *data;
+ struct rockchip_dfi *dfi;
struct devfreq_event_desc *desc;
struct device_node *np = pdev->dev.of_node, *node;
- data = devm_kzalloc(dev, sizeof(struct rockchip_dfi), GFP_KERNEL);
- if (!data)
+ dfi = devm_kzalloc(dev, sizeof(*dfi), GFP_KERNEL);
+ if (!dfi)
return -ENOMEM;
- data->regs = devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(data->regs))
- return PTR_ERR(data->regs);
+ dfi->regs = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(dfi->regs))
+ return PTR_ERR(dfi->regs);
- data->clk = devm_clk_get(dev, "pclk_ddr_mon");
- if (IS_ERR(data->clk))
- return dev_err_probe(dev, PTR_ERR(data->clk),
+ dfi->clk = devm_clk_get(dev, "pclk_ddr_mon");
+ if (IS_ERR(dfi->clk))
+ return dev_err_probe(dev, PTR_ERR(dfi->clk),
"Cannot get the clk pclk_ddr_mon\n");
node = of_parse_phandle(np, "rockchip,pmu", 0);
if (!node)
return dev_err_probe(&pdev->dev, -ENODEV, "Can't find pmu_grf registers\n");
- data->regmap_pmu = syscon_node_to_regmap(node);
+ dfi->regmap_pmu = syscon_node_to_regmap(node);
of_node_put(node);
- if (IS_ERR(data->regmap_pmu))
- return PTR_ERR(data->regmap_pmu);
+ if (IS_ERR(dfi->regmap_pmu))
+ return PTR_ERR(dfi->regmap_pmu);
- data->dev = dev;
+ dfi->dev = dev;
- desc = &data->desc;
+ desc = &dfi->desc;
desc->ops = &rockchip_dfi_ops;
- desc->driver_data = data;
+ desc->driver_data = dfi;
desc->name = np->name;
- data->edev = devm_devfreq_event_add_edev(&pdev->dev, desc);
- if (IS_ERR(data->edev)) {
+ dfi->edev = devm_devfreq_event_add_edev(&pdev->dev, desc);
+ if (IS_ERR(dfi->edev)) {
dev_err(&pdev->dev,
"failed to add devfreq-event device\n");
- return PTR_ERR(data->edev);
+ return PTR_ERR(dfi->edev);
}
- platform_set_drvdata(pdev, data);
+ platform_set_drvdata(pdev, dfi);
return 0;
}
--
2.39.2
On Wed, May 24, 2023 at 10:31:50AM +0200, Sascha Hauer wrote:
> This adds rockchip,rk3588-dfi to the list of compatibles. Unlike ealier
> SoCs the rk3588 has four interrupts (one for each channel) instead of
> only one, so increase the number of allowed interrupts to four and also
> add interrupt-names.
>
> Signed-off-by: Sascha Hauer <[email protected]>
> ---
>
> Notes:
> Changes since v4:
> - new patch
>
> .../bindings/devfreq/event/rockchip,dfi.yaml | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/devfreq/event/rockchip,dfi.yaml b/Documentation/devicetree/bindings/devfreq/event/rockchip,dfi.yaml
> index e8b64494ee8bd..4e647a9560560 100644
> --- a/Documentation/devicetree/bindings/devfreq/event/rockchip,dfi.yaml
> +++ b/Documentation/devicetree/bindings/devfreq/event/rockchip,dfi.yaml
> @@ -14,6 +14,7 @@ properties:
> enum:
> - rockchip,rk3399-dfi
> - rockchip,rk3568-dfi
> + - rockchip,rk3588-dfi
>
> clocks:
> maxItems: 1
> @@ -23,7 +24,18 @@ properties:
> - const: pclk_ddr_mon
>
> interrupts:
> - maxItems: 1
> + minItems: 1
> + maxItems: 4
> +
> + interrupt-names:
> + oneOf:
> + - items:
> + - const: ch0
> + - items:
> + - const: ch0
> + - const: ch1
> + - const: ch2
> + - const: ch3
Is it worth adding restrictions so that only the new compatible is
allowed to have more than 1 interrupt?
Heiko?
On Wed, May 24, 2023 at 10:31:49AM +0200, Sascha Hauer wrote:
> This adds the rockchip,rk3568-dfi compatible to the binding. Make clocks
> optional for this SoC as the RK3568 doesn't have a kernel controllable
> PCLK.
>
> Signed-off-by: Sascha Hauer <[email protected]>
This one looks fine to me,
Reviewed-by: Conor Dooley <[email protected]>
Thanks,
Conor.
> ---
> .../bindings/devfreq/event/rockchip,dfi.yaml | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/devfreq/event/rockchip,dfi.yaml b/Documentation/devicetree/bindings/devfreq/event/rockchip,dfi.yaml
> index 7a82f6ae0701e..e8b64494ee8bd 100644
> --- a/Documentation/devicetree/bindings/devfreq/event/rockchip,dfi.yaml
> +++ b/Documentation/devicetree/bindings/devfreq/event/rockchip,dfi.yaml
> @@ -13,6 +13,7 @@ properties:
> compatible:
> enum:
> - rockchip,rk3399-dfi
> + - rockchip,rk3568-dfi
>
> clocks:
> maxItems: 1
> @@ -34,11 +35,21 @@ properties:
>
> required:
> - compatible
> - - clocks
> - - clock-names
> - interrupts
> - reg
>
> +if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - rockchip,rk3399-dfi
> +
> +then:
> + required:
> + - clocks
> + - clock-names
> +
> additionalProperties: false
>
> examples:
> --
> 2.39.2
>
On Wed, May 24, 2023 at 10:31:50AM +0200, Sascha Hauer wrote:
> This adds rockchip,rk3588-dfi to the list of compatibles. Unlike ealier
> SoCs the rk3588 has four interrupts (one for each channel) instead of
> only one, so increase the number of allowed interrupts to four and also
> add interrupt-names.
>
> Signed-off-by: Sascha Hauer <[email protected]>
> ---
>
> Notes:
> Changes since v4:
> - new patch
>
> .../bindings/devfreq/event/rockchip,dfi.yaml | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/devfreq/event/rockchip,dfi.yaml b/Documentation/devicetree/bindings/devfreq/event/rockchip,dfi.yaml
> index e8b64494ee8bd..4e647a9560560 100644
> --- a/Documentation/devicetree/bindings/devfreq/event/rockchip,dfi.yaml
> +++ b/Documentation/devicetree/bindings/devfreq/event/rockchip,dfi.yaml
> @@ -14,6 +14,7 @@ properties:
> enum:
> - rockchip,rk3399-dfi
> - rockchip,rk3568-dfi
> + - rockchip,rk3588-dfi
>
> clocks:
> maxItems: 1
> @@ -23,7 +24,18 @@ properties:
> - const: pclk_ddr_mon
>
> interrupts:
> - maxItems: 1
> + minItems: 1
> + maxItems: 4
> +
> + interrupt-names:
> + oneOf:
> + - items:
> + - const: ch0
> + - items:
> + - const: ch0
> + - const: ch1
> + - const: ch2
> + - const: ch3
Names that are just an index are generally pointless.
Hi,
On Wed, May 24, 2023 at 10:31:32AM +0200, Sascha Hauer wrote:
> Move the RK3399 specifics to a SoC specific init function to make
> the way free for supporting other SoCs later.
>
> Signed-off-by: Sascha Hauer <[email protected]>
> ---
>
> Notes:
> Changes since v4:
> - use of_device_get_match_data()
> - use a callback rather than a struct type as driver data
>
> drivers/devfreq/event/rockchip-dfi.c | 48 +++++++++++++++++++---------
> 1 file changed, 33 insertions(+), 15 deletions(-)
>
Reviewed-by: Sebastian Reichel <[email protected]>
-- Sebastian
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index e19e5acaa362c..6b1ef29df7048 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -17,6 +17,7 @@
> #include <linux/slab.h>
> #include <linux/list.h>
> #include <linux/of.h>
> +#include <linux/of_device.h>
>
> #include <soc/rockchip/rk3399_grf.h>
>
> @@ -55,27 +56,21 @@ struct rockchip_dfi {
> void __iomem *regs;
> struct regmap *regmap_pmu;
> struct clk *clk;
> + u32 ddr_type;
> };
>
> static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
> {
> struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
> void __iomem *dfi_regs = dfi->regs;
> - u32 val;
> - u32 ddr_type;
> -
> - /* get ddr type */
> - regmap_read(dfi->regmap_pmu, RK3399_PMUGRF_OS_REG2, &val);
> - ddr_type = (val >> RK3399_PMUGRF_DDRTYPE_SHIFT) &
> - RK3399_PMUGRF_DDRTYPE_MASK;
>
> /* clear DDRMON_CTRL setting */
> writel_relaxed(CLR_DDRMON_CTRL, dfi_regs + DDRMON_CTRL);
>
> /* set ddr type to dfi */
> - if (ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR3)
> + if (dfi->ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR3)
> writel_relaxed(LPDDR3_EN, dfi_regs + DDRMON_CTRL);
> - else if (ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR4)
> + else if (dfi->ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR4)
> writel_relaxed(LPDDR4_EN, dfi_regs + DDRMON_CTRL);
>
> /* enable count, use software mode */
> @@ -167,8 +162,26 @@ static const struct devfreq_event_ops rockchip_dfi_ops = {
> .set_event = rockchip_dfi_set_event,
> };
>
> +static int rk3399_dfi_init(struct rockchip_dfi *dfi)
> +{
> + struct regmap *regmap_pmu = dfi->regmap_pmu;
> + u32 val;
> +
> + dfi->clk = devm_clk_get(dfi->dev, "pclk_ddr_mon");
> + if (IS_ERR(dfi->clk))
> + return dev_err_probe(dfi->dev, PTR_ERR(dfi->clk),
> + "Cannot get the clk pclk_ddr_mon\n");
> +
> + /* get ddr type */
> + regmap_read(regmap_pmu, RK3399_PMUGRF_OS_REG2, &val);
> + dfi->ddr_type = (val >> RK3399_PMUGRF_DDRTYPE_SHIFT) &
> + RK3399_PMUGRF_DDRTYPE_MASK;
> +
> + return 0;
> +};
> +
> static const struct of_device_id rockchip_dfi_id_match[] = {
> - { .compatible = "rockchip,rk3399-dfi" },
> + { .compatible = "rockchip,rk3399-dfi", .data = rk3399_dfi_init },
> { },
> };
> MODULE_DEVICE_TABLE(of, rockchip_dfi_id_match);
> @@ -179,6 +192,12 @@ static int rockchip_dfi_probe(struct platform_device *pdev)
> struct rockchip_dfi *dfi;
> struct devfreq_event_desc *desc;
> struct device_node *np = pdev->dev.of_node, *node;
> + int (*soc_init)(struct rockchip_dfi *dfi);
> + int ret;
> +
> + soc_init = of_device_get_match_data(&pdev->dev);
> + if (!soc_init)
> + return -EINVAL;
>
> dfi = devm_kzalloc(dev, sizeof(*dfi), GFP_KERNEL);
> if (!dfi)
> @@ -188,11 +207,6 @@ static int rockchip_dfi_probe(struct platform_device *pdev)
> if (IS_ERR(dfi->regs))
> return PTR_ERR(dfi->regs);
>
> - dfi->clk = devm_clk_get(dev, "pclk_ddr_mon");
> - if (IS_ERR(dfi->clk))
> - return dev_err_probe(dev, PTR_ERR(dfi->clk),
> - "Cannot get the clk pclk_ddr_mon\n");
> -
> node = of_parse_phandle(np, "rockchip,pmu", 0);
> if (!node)
> return dev_err_probe(&pdev->dev, -ENODEV, "Can't find pmu_grf registers\n");
> @@ -209,6 +223,10 @@ static int rockchip_dfi_probe(struct platform_device *pdev)
> desc->driver_data = dfi;
> desc->name = np->name;
>
> + ret = soc_init(dfi);
> + if (ret)
> + return ret;
> +
> dfi->edev = devm_devfreq_event_add_edev(&pdev->dev, desc);
> if (IS_ERR(dfi->edev)) {
> dev_err(&pdev->dev,
> --
> 2.39.2
>
Hi,
On Wed, May 24, 2023 at 10:31:33AM +0200, Sascha Hauer wrote:
> When adding perf support to the DFI driver the perf part will
> need the raw counter values, so move the fixed * 4 factor to
> rockchip_dfi_get_event().
>
> Signed-off-by: Sascha Hauer <[email protected]>
> Reviewed-by: Jonathan Cameron <[email protected]>
> ---
Reviewed-by: Sebastian Reichel <[email protected]>
-- Sebastian
> drivers/devfreq/event/rockchip-dfi.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 6b1ef29df7048..680f629da64fc 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -97,7 +97,7 @@ static int rockchip_dfi_get_busier_ch(struct devfreq_event_dev *edev)
> /* Find out which channel is busier */
> for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
> dfi->ch_usage[i].access = readl_relaxed(dfi_regs +
> - DDRMON_CH0_DFI_ACCESS_NUM + i * 20) * 4;
> + DDRMON_CH0_DFI_ACCESS_NUM + i * 20);
> dfi->ch_usage[i].total = readl_relaxed(dfi_regs +
> DDRMON_CH0_COUNT_NUM + i * 20);
> tmp = dfi->ch_usage[i].access;
> @@ -149,7 +149,7 @@ static int rockchip_dfi_get_event(struct devfreq_event_dev *edev,
>
> busier_ch = rockchip_dfi_get_busier_ch(edev);
>
> - edata->load_count = dfi->ch_usage[busier_ch].access;
> + edata->load_count = dfi->ch_usage[busier_ch].access * 4;
> edata->total_count = dfi->ch_usage[busier_ch].total;
>
> return 0;
> --
> 2.39.2
>
Hi,
On Wed, May 24, 2023 at 10:31:36AM +0200, Sascha Hauer wrote:
> The DDRTYPE defines are named to be RK3399 specific, but they can be
> used for other Rockchip SoCs as well, so replace the RK3399_PMUGRF_
> prefix with ROCKCHIP_. They are defined in a SoC specific header
> file, so when generalizing the prefix also move the new defines to
> a SoC agnostic header file. While at it use GENMASK to define the
> DDRTYPE bitfield and give it a name including the full register name.
>
> Signed-off-by: Sascha Hauer <[email protected]>
> ---
Reviewed-by: Sebastian Reichel <[email protected]>
-- Sebastian
> drivers/devfreq/event/rockchip-dfi.c | 9 +++++----
> drivers/devfreq/rk3399_dmc.c | 10 +++++-----
> include/soc/rockchip/rk3399_grf.h | 7 +------
> include/soc/rockchip/rockchip_grf.h | 17 +++++++++++++++++
> 4 files changed, 28 insertions(+), 15 deletions(-)
> create mode 100644 include/soc/rockchip/rockchip_grf.h
>
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 82de24a027579..6bccb6fbcfc0c 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -18,8 +18,10 @@
> #include <linux/list.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> +#include <linux/bitfield.h>
> #include <linux/bits.h>
>
> +#include <soc/rockchip/rockchip_grf.h>
> #include <soc/rockchip/rk3399_grf.h>
>
> #define DMC_MAX_CHANNELS 2
> @@ -74,9 +76,9 @@ static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
> writel_relaxed(CLR_DDRMON_CTRL, dfi_regs + DDRMON_CTRL);
>
> /* set ddr type to dfi */
> - if (dfi->ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR3)
> + if (dfi->ddr_type == ROCKCHIP_DDRTYPE_LPDDR3)
> writel_relaxed(LPDDR3_EN, dfi_regs + DDRMON_CTRL);
> - else if (dfi->ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR4)
> + else if (dfi->ddr_type == ROCKCHIP_DDRTYPE_LPDDR4)
> writel_relaxed(LPDDR4_EN, dfi_regs + DDRMON_CTRL);
>
> /* enable count, use software mode */
> @@ -191,8 +193,7 @@ static int rk3399_dfi_init(struct rockchip_dfi *dfi)
>
> /* get ddr type */
> regmap_read(regmap_pmu, RK3399_PMUGRF_OS_REG2, &val);
> - dfi->ddr_type = (val >> RK3399_PMUGRF_DDRTYPE_SHIFT) &
> - RK3399_PMUGRF_DDRTYPE_MASK;
> + dfi->ddr_type = FIELD_GET(RK3399_PMUGRF_OS_REG2_DDRTYPE, val);
>
> dfi->channel_mask = GENMASK(1, 0);
>
> diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
> index daff407026157..fd2c5ffedf41e 100644
> --- a/drivers/devfreq/rk3399_dmc.c
> +++ b/drivers/devfreq/rk3399_dmc.c
> @@ -22,6 +22,7 @@
> #include <linux/suspend.h>
>
> #include <soc/rockchip/pm_domains.h>
> +#include <soc/rockchip/rockchip_grf.h>
> #include <soc/rockchip/rk3399_grf.h>
> #include <soc/rockchip/rockchip_sip.h>
>
> @@ -381,17 +382,16 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
> }
>
> regmap_read(data->regmap_pmu, RK3399_PMUGRF_OS_REG2, &val);
> - ddr_type = (val >> RK3399_PMUGRF_DDRTYPE_SHIFT) &
> - RK3399_PMUGRF_DDRTYPE_MASK;
> + ddr_type = FIELD_GET(RK3399_PMUGRF_OS_REG2_DDRTYPE, val);
>
> switch (ddr_type) {
> - case RK3399_PMUGRF_DDRTYPE_DDR3:
> + case ROCKCHIP_DDRTYPE_DDR3:
> data->odt_dis_freq = data->ddr3_odt_dis_freq;
> break;
> - case RK3399_PMUGRF_DDRTYPE_LPDDR3:
> + case ROCKCHIP_DDRTYPE_LPDDR3:
> data->odt_dis_freq = data->lpddr3_odt_dis_freq;
> break;
> - case RK3399_PMUGRF_DDRTYPE_LPDDR4:
> + case ROCKCHIP_DDRTYPE_LPDDR4:
> data->odt_dis_freq = data->lpddr4_odt_dis_freq;
> break;
> default:
> diff --git a/include/soc/rockchip/rk3399_grf.h b/include/soc/rockchip/rk3399_grf.h
> index 3eebabcb28123..775f8444bea8d 100644
> --- a/include/soc/rockchip/rk3399_grf.h
> +++ b/include/soc/rockchip/rk3399_grf.h
> @@ -11,11 +11,6 @@
>
> /* PMU GRF Registers */
> #define RK3399_PMUGRF_OS_REG2 0x308
> -#define RK3399_PMUGRF_DDRTYPE_SHIFT 13
> -#define RK3399_PMUGRF_DDRTYPE_MASK 7
> -#define RK3399_PMUGRF_DDRTYPE_DDR3 3
> -#define RK3399_PMUGRF_DDRTYPE_LPDDR2 5
> -#define RK3399_PMUGRF_DDRTYPE_LPDDR3 6
> -#define RK3399_PMUGRF_DDRTYPE_LPDDR4 7
> +#define RK3399_PMUGRF_OS_REG2_DDRTYPE GENMASK(15, 13)
>
> #endif
> diff --git a/include/soc/rockchip/rockchip_grf.h b/include/soc/rockchip/rockchip_grf.h
> new file mode 100644
> index 0000000000000..dde1a9796ccb5
> --- /dev/null
> +++ b/include/soc/rockchip/rockchip_grf.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Rockchip General Register Files definitions
> + */
> +
> +#ifndef __SOC_ROCKCHIP_GRF_H
> +#define __SOC_ROCKCHIP_GRF_H
> +
> +/* Rockchip DDRTYPE defines */
> +enum {
> + ROCKCHIP_DDRTYPE_DDR3 = 3,
> + ROCKCHIP_DDRTYPE_LPDDR2 = 5,
> + ROCKCHIP_DDRTYPE_LPDDR3 = 6,
> + ROCKCHIP_DDRTYPE_LPDDR4 = 7,
> +};
> +
> +#endif /* __SOC_ROCKCHIP_GRF_H */
> --
> 2.39.2
>
hi,
On Wed, May 24, 2023 at 10:31:40AM +0200, Sascha Hauer wrote:
> In the DFI driver LPDDR4X can be handled in the same way as LPDDR4. Add
> the missing case.
>
> Signed-off-by: Sascha Hauer <[email protected]>
> Reviewed-by: Jonathan Cameron <[email protected]>
> ---
Reviewed-by: Sebastian Reichel <[email protected]>
-- Sebastian
> drivers/devfreq/event/rockchip-dfi.c | 1 +
> include/soc/rockchip/rockchip_grf.h | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 16cd5365671f7..0a568c5551699 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -89,6 +89,7 @@ static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
> dfi_regs + DDRMON_CTRL);
> break;
> case ROCKCHIP_DDRTYPE_LPDDR4:
> + case ROCKCHIP_DDRTYPE_LPDDR4X:
> writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_LPDDR4, DDRMON_CTRL_DDR_TYPE_MASK),
> dfi_regs + DDRMON_CTRL);
> break;
> diff --git a/include/soc/rockchip/rockchip_grf.h b/include/soc/rockchip/rockchip_grf.h
> index dde1a9796ccb5..e46fd72aea8d1 100644
> --- a/include/soc/rockchip/rockchip_grf.h
> +++ b/include/soc/rockchip/rockchip_grf.h
> @@ -12,6 +12,7 @@ enum {
> ROCKCHIP_DDRTYPE_LPDDR2 = 5,
> ROCKCHIP_DDRTYPE_LPDDR3 = 6,
> ROCKCHIP_DDRTYPE_LPDDR4 = 7,
> + ROCKCHIP_DDRTYPE_LPDDR4X = 8,
> };
>
> #endif /* __SOC_ROCKCHIP_GRF_H */
> --
> 2.39.2
>
Hi,
On Wed, May 24, 2023 at 10:31:31AM +0200, Sascha Hauer wrote:
> The variable name for the private data struct is 'info' in some
> functions and 'data' in others. Both names do not give a clue what
> type the variable has, so consistently use 'dfi'.
>
> Signed-off-by: Sascha Hauer <[email protected]>
> Reviewed-by: Heiko Stuebner <[email protected]>
> Reviewed-by: Jonathan Cameron <[email protected]>
> ---
Reviewed-by: Sebastian Reichel <[email protected]>
-- Sebastian
> drivers/devfreq/event/rockchip-dfi.c | 72 ++++++++++++++--------------
> 1 file changed, 36 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 467f9f42d38f7..e19e5acaa362c 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -59,13 +59,13 @@ struct rockchip_dfi {
>
> static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
> {
> - struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
> - void __iomem *dfi_regs = info->regs;
> + struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
> + void __iomem *dfi_regs = dfi->regs;
> u32 val;
> u32 ddr_type;
>
> /* get ddr type */
> - regmap_read(info->regmap_pmu, RK3399_PMUGRF_OS_REG2, &val);
> + regmap_read(dfi->regmap_pmu, RK3399_PMUGRF_OS_REG2, &val);
> ddr_type = (val >> RK3399_PMUGRF_DDRTYPE_SHIFT) &
> RK3399_PMUGRF_DDRTYPE_MASK;
>
> @@ -84,28 +84,28 @@ static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
>
> static void rockchip_dfi_stop_hardware_counter(struct devfreq_event_dev *edev)
> {
> - struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
> - void __iomem *dfi_regs = info->regs;
> + struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
> + void __iomem *dfi_regs = dfi->regs;
>
> writel_relaxed(SOFTWARE_DIS, dfi_regs + DDRMON_CTRL);
> }
>
> static int rockchip_dfi_get_busier_ch(struct devfreq_event_dev *edev)
> {
> - struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
> + struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
> u32 tmp, max = 0;
> u32 i, busier_ch = 0;
> - void __iomem *dfi_regs = info->regs;
> + void __iomem *dfi_regs = dfi->regs;
>
> rockchip_dfi_stop_hardware_counter(edev);
>
> /* Find out which channel is busier */
> for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
> - info->ch_usage[i].access = readl_relaxed(dfi_regs +
> + dfi->ch_usage[i].access = readl_relaxed(dfi_regs +
> DDRMON_CH0_DFI_ACCESS_NUM + i * 20) * 4;
> - info->ch_usage[i].total = readl_relaxed(dfi_regs +
> + dfi->ch_usage[i].total = readl_relaxed(dfi_regs +
> DDRMON_CH0_COUNT_NUM + i * 20);
> - tmp = info->ch_usage[i].access;
> + tmp = dfi->ch_usage[i].access;
> if (tmp > max) {
> busier_ch = i;
> max = tmp;
> @@ -118,20 +118,20 @@ static int rockchip_dfi_get_busier_ch(struct devfreq_event_dev *edev)
>
> static int rockchip_dfi_disable(struct devfreq_event_dev *edev)
> {
> - struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
> + struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
>
> rockchip_dfi_stop_hardware_counter(edev);
> - clk_disable_unprepare(info->clk);
> + clk_disable_unprepare(dfi->clk);
>
> return 0;
> }
>
> static int rockchip_dfi_enable(struct devfreq_event_dev *edev)
> {
> - struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
> + struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
> int ret;
>
> - ret = clk_prepare_enable(info->clk);
> + ret = clk_prepare_enable(dfi->clk);
> if (ret) {
> dev_err(&edev->dev, "failed to enable dfi clk: %d\n", ret);
> return ret;
> @@ -149,13 +149,13 @@ static int rockchip_dfi_set_event(struct devfreq_event_dev *edev)
> static int rockchip_dfi_get_event(struct devfreq_event_dev *edev,
> struct devfreq_event_data *edata)
> {
> - struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
> + struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
> int busier_ch;
>
> busier_ch = rockchip_dfi_get_busier_ch(edev);
>
> - edata->load_count = info->ch_usage[busier_ch].access;
> - edata->total_count = info->ch_usage[busier_ch].total;
> + edata->load_count = dfi->ch_usage[busier_ch].access;
> + edata->total_count = dfi->ch_usage[busier_ch].total;
>
> return 0;
> }
> @@ -176,47 +176,47 @@ MODULE_DEVICE_TABLE(of, rockchip_dfi_id_match);
> static int rockchip_dfi_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> - struct rockchip_dfi *data;
> + struct rockchip_dfi *dfi;
> struct devfreq_event_desc *desc;
> struct device_node *np = pdev->dev.of_node, *node;
>
> - data = devm_kzalloc(dev, sizeof(struct rockchip_dfi), GFP_KERNEL);
> - if (!data)
> + dfi = devm_kzalloc(dev, sizeof(*dfi), GFP_KERNEL);
> + if (!dfi)
> return -ENOMEM;
>
> - data->regs = devm_platform_ioremap_resource(pdev, 0);
> - if (IS_ERR(data->regs))
> - return PTR_ERR(data->regs);
> + dfi->regs = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(dfi->regs))
> + return PTR_ERR(dfi->regs);
>
> - data->clk = devm_clk_get(dev, "pclk_ddr_mon");
> - if (IS_ERR(data->clk))
> - return dev_err_probe(dev, PTR_ERR(data->clk),
> + dfi->clk = devm_clk_get(dev, "pclk_ddr_mon");
> + if (IS_ERR(dfi->clk))
> + return dev_err_probe(dev, PTR_ERR(dfi->clk),
> "Cannot get the clk pclk_ddr_mon\n");
>
> node = of_parse_phandle(np, "rockchip,pmu", 0);
> if (!node)
> return dev_err_probe(&pdev->dev, -ENODEV, "Can't find pmu_grf registers\n");
>
> - data->regmap_pmu = syscon_node_to_regmap(node);
> + dfi->regmap_pmu = syscon_node_to_regmap(node);
> of_node_put(node);
> - if (IS_ERR(data->regmap_pmu))
> - return PTR_ERR(data->regmap_pmu);
> + if (IS_ERR(dfi->regmap_pmu))
> + return PTR_ERR(dfi->regmap_pmu);
>
> - data->dev = dev;
> + dfi->dev = dev;
>
> - desc = &data->desc;
> + desc = &dfi->desc;
> desc->ops = &rockchip_dfi_ops;
> - desc->driver_data = data;
> + desc->driver_data = dfi;
> desc->name = np->name;
>
> - data->edev = devm_devfreq_event_add_edev(&pdev->dev, desc);
> - if (IS_ERR(data->edev)) {
> + dfi->edev = devm_devfreq_event_add_edev(&pdev->dev, desc);
> + if (IS_ERR(dfi->edev)) {
> dev_err(&pdev->dev,
> "failed to add devfreq-event device\n");
> - return PTR_ERR(data->edev);
> + return PTR_ERR(dfi->edev);
> }
>
> - platform_set_drvdata(pdev, data);
> + platform_set_drvdata(pdev, dfi);
>
> return 0;
> }
> --
> 2.39.2
>
Hi,
On Wed, May 24, 2023 at 10:31:35AM +0200, Sascha Hauer wrote:
> Different Rockchip SoC variants have a different number of channels.
> Introduce a channel mask to make the number of channels configurable
> from SoC initialization code.
>
> Signed-off-by: Sascha Hauer <[email protected]>
> ---
Reviewed-by: Sebastian Reichel <[email protected]>
-- Sebastian
> drivers/devfreq/event/rockchip-dfi.c | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 126bb744645b6..82de24a027579 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -18,10 +18,11 @@
> #include <linux/list.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> +#include <linux/bits.h>
>
> #include <soc/rockchip/rk3399_grf.h>
>
> -#define RK3399_DMC_NUM_CH 2
> +#define DMC_MAX_CHANNELS 2
>
> /* DDRMON_CTRL */
> #define DDRMON_CTRL 0x04
> @@ -44,7 +45,7 @@ struct dmc_count_channel {
> };
>
> struct dmc_count {
> - struct dmc_count_channel c[RK3399_DMC_NUM_CH];
> + struct dmc_count_channel c[DMC_MAX_CHANNELS];
> };
>
> /*
> @@ -61,6 +62,7 @@ struct rockchip_dfi {
> struct regmap *regmap_pmu;
> struct clk *clk;
> u32 ddr_type;
> + unsigned int channel_mask;
> };
>
> static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
> @@ -95,7 +97,9 @@ static void rockchip_dfi_read_counters(struct devfreq_event_dev *edev, struct dm
> u32 i;
> void __iomem *dfi_regs = dfi->regs;
>
> - for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
> + for (i = 0; i < DMC_MAX_CHANNELS; i++) {
> + if (!(dfi->channel_mask & BIT(i)))
> + continue;
> count->c[i].access = readl_relaxed(dfi_regs +
> DDRMON_CH0_DFI_ACCESS_NUM + i * 20);
> count->c[i].total = readl_relaxed(dfi_regs +
> @@ -145,9 +149,14 @@ static int rockchip_dfi_get_event(struct devfreq_event_dev *edev,
> rockchip_dfi_read_counters(edev, &count);
>
> /* We can only report one channel, so find the busiest one */
> - for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
> - u32 a = count.c[i].access - last->c[i].access;
> - u32 t = count.c[i].total - last->c[i].total;
> + for (i = 0; i < DMC_MAX_CHANNELS; i++) {
> + u32 a, t;
> +
> + if (!(dfi->channel_mask & BIT(i)))
> + continue;
> +
> + a = count.c[i].access - last->c[i].access;
> + t = count.c[i].total - last->c[i].total;
>
> if (a > access) {
> access = a;
> @@ -185,6 +194,8 @@ static int rk3399_dfi_init(struct rockchip_dfi *dfi)
> dfi->ddr_type = (val >> RK3399_PMUGRF_DDRTYPE_SHIFT) &
> RK3399_PMUGRF_DDRTYPE_MASK;
>
> + dfi->channel_mask = GENMASK(1, 0);
> +
> return 0;
> };
>
> --
> 2.39.2
>
Hi,
On Wed, May 24, 2023 at 10:31:29AM +0200, Sascha Hauer wrote:
> As a matter of fact the regmap_pmu already is mandatory because
> it is used unconditionally in the driver. Bail out gracefully in
> probe() rather than crashing later.
>
> Fixes: b9d1262bca0af ("PM / devfreq: event: support rockchip dfi controller")
> Signed-off-by: Sascha Hauer <[email protected]>
> ---
Reviewed-by: Sebastian Reichel <[email protected]>
-- Sebastian
>
> Notes:
> Changes since v4:
> - move to beginning of the series to make it easier to backport to stable
> - Add a Fixes: tag
> - add missing of_node_put()
>
> drivers/devfreq/event/rockchip-dfi.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 39ac069cabc75..74893c06aa087 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -193,14 +193,15 @@ static int rockchip_dfi_probe(struct platform_device *pdev)
> return dev_err_probe(dev, PTR_ERR(data->clk),
> "Cannot get the clk pclk_ddr_mon\n");
>
> - /* try to find the optional reference to the pmu syscon */
> node = of_parse_phandle(np, "rockchip,pmu", 0);
> - if (node) {
> - data->regmap_pmu = syscon_node_to_regmap(node);
> - of_node_put(node);
> - if (IS_ERR(data->regmap_pmu))
> - return PTR_ERR(data->regmap_pmu);
> - }
> + if (!node)
> + return dev_err_probe(&pdev->dev, -ENODEV, "Can't find pmu_grf registers\n");
> +
> + data->regmap_pmu = syscon_node_to_regmap(node);
> + of_node_put(node);
> + if (IS_ERR(data->regmap_pmu))
> + return PTR_ERR(data->regmap_pmu);
> +
> data->dev = dev;
>
> desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
> --
> 2.39.2
>
Hi,
On Wed, May 24, 2023 at 10:31:39AM +0200, Sascha Hauer wrote:
> According to the downstream driver the DDRMON_CTRL_LPDDR23 bit must be
> set for both LPDDR2 and LPDDR3. Add the missing LPDDR2 case and while
> at it turn the if/else if/else into switch/case which makes it easier
> to read.
>
> Signed-off-by: Sascha Hauer <[email protected]>
> Reviewed-by: Jonathan Cameron <[email protected]>
> ---
Reviewed-by: Sebastian Reichel <[email protected]>
-- Sebastian
> drivers/devfreq/event/rockchip-dfi.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 261d112580c9e..16cd5365671f7 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -82,12 +82,19 @@ static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
> DDRMON_CTRL_HARDWARE_EN), dfi_regs + DDRMON_CTRL);
>
> /* set ddr type to dfi */
> - if (dfi->ddr_type == ROCKCHIP_DDRTYPE_LPDDR3)
> + switch (dfi->ddr_type) {
> + case ROCKCHIP_DDRTYPE_LPDDR2:
> + case ROCKCHIP_DDRTYPE_LPDDR3:
> writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_LPDDR23, DDRMON_CTRL_DDR_TYPE_MASK),
> dfi_regs + DDRMON_CTRL);
> - else if (dfi->ddr_type == ROCKCHIP_DDRTYPE_LPDDR4)
> + break;
> + case ROCKCHIP_DDRTYPE_LPDDR4:
> writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_LPDDR4, DDRMON_CTRL_DDR_TYPE_MASK),
> dfi_regs + DDRMON_CTRL);
> + break;
> + default:
> + break;
> + }
>
> /* enable count, use software mode */
> writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_SOFTWARE_EN, DDRMON_CTRL_SOFTWARE_EN),
> --
> 2.39.2
>
Hi,
On Wed, May 24, 2023 at 10:31:30AM +0200, Sascha Hauer wrote:
> No need for an extra allocation, just embed the struct
> devfreq_event_desc into the private data struct.
>
> Signed-off-by: Sascha Hauer <[email protected]>
> Reviewed-by: Heiko Stuebner <[email protected]>
> Reviewed-by: Jonathan Cameron <[email protected]>
> ---
Reviewed-by: Sebastian Reichel <[email protected]>
-- Sebastian
> drivers/devfreq/event/rockchip-dfi.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 74893c06aa087..467f9f42d38f7 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -49,7 +49,7 @@ struct dmc_usage {
> */
> struct rockchip_dfi {
> struct devfreq_event_dev *edev;
> - struct devfreq_event_desc *desc;
> + struct devfreq_event_desc desc;
> struct dmc_usage ch_usage[RK3399_DMC_NUM_CH];
> struct device *dev;
> void __iomem *regs;
> @@ -204,14 +204,10 @@ static int rockchip_dfi_probe(struct platform_device *pdev)
>
> data->dev = dev;
>
> - desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
> - if (!desc)
> - return -ENOMEM;
> -
> + desc = &data->desc;
> desc->ops = &rockchip_dfi_ops;
> desc->driver_data = data;
> desc->name = np->name;
> - data->desc = desc;
>
> data->edev = devm_devfreq_event_add_edev(&pdev->dev, desc);
> if (IS_ERR(data->edev)) {
> --
> 2.39.2
>
Hi,
On Wed, May 24, 2023 at 10:31:34AM +0200, Sascha Hauer wrote:
> The DDR_MON counters are free running counters. These are resetted to 0
> when starting them over like currently done when reading the current
> counter values.
>
> Resetting the counters becomes a problem with perf support we want to
> add later, because perf needs counters that are not modified elsewhere.
>
> This patch removes resetting the counters and keeps them running
> instead. That means we no longer use the absolute counter values but
> instead compare them with the counter values we read last time. Not
> stopping the counters also has the impact that they are running while
> we are reading them. We cannot read multiple timers atomically, so
> the values do not exactly fit together. The effect should be negligible
> though as the time between two measurements is some orders of magnitude
> bigger than the time we need to read multiple registers.
>
> Signed-off-by: Sascha Hauer <[email protected]>
> ---
Reviewed-by: Sebastian Reichel <[email protected]>
-- Sebastian
>
> Notes:
> Changes since v4:
> - rephrase commit message
> - Drop unused variable
>
> drivers/devfreq/event/rockchip-dfi.c | 52 ++++++++++++++++------------
> 1 file changed, 30 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 680f629da64fc..126bb744645b6 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -38,11 +38,15 @@
> #define DDRMON_CH1_COUNT_NUM 0x3c
> #define DDRMON_CH1_DFI_ACCESS_NUM 0x40
>
> -struct dmc_usage {
> +struct dmc_count_channel {
> u32 access;
> u32 total;
> };
>
> +struct dmc_count {
> + struct dmc_count_channel c[RK3399_DMC_NUM_CH];
> +};
> +
> /*
> * The dfi controller can monitor DDR load. It has an upper and lower threshold
> * for the operating points. Whenever the usage leaves these bounds an event is
> @@ -51,7 +55,7 @@ struct dmc_usage {
> struct rockchip_dfi {
> struct devfreq_event_dev *edev;
> struct devfreq_event_desc desc;
> - struct dmc_usage ch_usage[RK3399_DMC_NUM_CH];
> + struct dmc_count last_event_count;
> struct device *dev;
> void __iomem *regs;
> struct regmap *regmap_pmu;
> @@ -85,30 +89,18 @@ static void rockchip_dfi_stop_hardware_counter(struct devfreq_event_dev *edev)
> writel_relaxed(SOFTWARE_DIS, dfi_regs + DDRMON_CTRL);
> }
>
> -static int rockchip_dfi_get_busier_ch(struct devfreq_event_dev *edev)
> +static void rockchip_dfi_read_counters(struct devfreq_event_dev *edev, struct dmc_count *count)
> {
> struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
> - u32 tmp, max = 0;
> - u32 i, busier_ch = 0;
> + u32 i;
> void __iomem *dfi_regs = dfi->regs;
>
> - rockchip_dfi_stop_hardware_counter(edev);
> -
> - /* Find out which channel is busier */
> for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
> - dfi->ch_usage[i].access = readl_relaxed(dfi_regs +
> + count->c[i].access = readl_relaxed(dfi_regs +
> DDRMON_CH0_DFI_ACCESS_NUM + i * 20);
> - dfi->ch_usage[i].total = readl_relaxed(dfi_regs +
> + count->c[i].total = readl_relaxed(dfi_regs +
> DDRMON_CH0_COUNT_NUM + i * 20);
> - tmp = dfi->ch_usage[i].access;
> - if (tmp > max) {
> - busier_ch = i;
> - max = tmp;
> - }
> }
> - rockchip_dfi_start_hardware_counter(edev);
> -
> - return busier_ch;
> }
>
> static int rockchip_dfi_disable(struct devfreq_event_dev *edev)
> @@ -145,12 +137,28 @@ static int rockchip_dfi_get_event(struct devfreq_event_dev *edev,
> struct devfreq_event_data *edata)
> {
> struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
> - int busier_ch;
> + struct dmc_count count;
> + struct dmc_count *last = &dfi->last_event_count;
> + u32 access = 0, total = 0;
> + int i;
> +
> + rockchip_dfi_read_counters(edev, &count);
> +
> + /* We can only report one channel, so find the busiest one */
> + for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
> + u32 a = count.c[i].access - last->c[i].access;
> + u32 t = count.c[i].total - last->c[i].total;
> +
> + if (a > access) {
> + access = a;
> + total = t;
> + }
> + }
>
> - busier_ch = rockchip_dfi_get_busier_ch(edev);
> + edata->load_count = access * 4;
> + edata->total_count = total;
>
> - edata->load_count = dfi->ch_usage[busier_ch].access * 4;
> - edata->total_count = dfi->ch_usage[busier_ch].total;
> + dfi->last_event_count = count;
>
> return 0;
> }
> --
> 2.39.2
>
Hi,
On Wed, May 24, 2023 at 10:31:41AM +0200, Sascha Hauer wrote:
> The internal functions do not need the struct devfreq_event_dev *,
> so pass them the struct rockchip_dfi *. This is a preparation for
> adding perf support later which doesn't have a struct devfreq_event_dev *.
>
> Signed-off-by: Sascha Hauer <[email protected]>
> Reviewed-by: Jonathan Cameron <[email protected]>
> ---
Reviewed-by: Sebastian Reichel <[email protected]>
-- Sebastian
> drivers/devfreq/event/rockchip-dfi.c | 15 ++++++---------
> 1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 0a568c5551699..d39db5de7f19c 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -72,9 +72,8 @@ struct rockchip_dfi {
> unsigned int channel_mask;
> };
>
> -static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
> +static void rockchip_dfi_start_hardware_counter(struct rockchip_dfi *dfi)
> {
> - struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
> void __iomem *dfi_regs = dfi->regs;
>
> /* clear DDRMON_CTRL setting */
> @@ -102,18 +101,16 @@ static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
> dfi_regs + DDRMON_CTRL);
> }
>
> -static void rockchip_dfi_stop_hardware_counter(struct devfreq_event_dev *edev)
> +static void rockchip_dfi_stop_hardware_counter(struct rockchip_dfi *dfi)
> {
> - struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
> void __iomem *dfi_regs = dfi->regs;
>
> writel_relaxed(HIWORD_UPDATE(0, DDRMON_CTRL_SOFTWARE_EN),
> dfi_regs + DDRMON_CTRL);
> }
>
> -static void rockchip_dfi_read_counters(struct devfreq_event_dev *edev, struct dmc_count *count)
> +static void rockchip_dfi_read_counters(struct rockchip_dfi *dfi, struct dmc_count *count)
> {
> - struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
> u32 i;
> void __iomem *dfi_regs = dfi->regs;
>
> @@ -131,7 +128,7 @@ static int rockchip_dfi_disable(struct devfreq_event_dev *edev)
> {
> struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
>
> - rockchip_dfi_stop_hardware_counter(edev);
> + rockchip_dfi_stop_hardware_counter(dfi);
> clk_disable_unprepare(dfi->clk);
>
> return 0;
> @@ -148,7 +145,7 @@ static int rockchip_dfi_enable(struct devfreq_event_dev *edev)
> return ret;
> }
>
> - rockchip_dfi_start_hardware_counter(edev);
> + rockchip_dfi_start_hardware_counter(dfi);
> return 0;
> }
>
> @@ -166,7 +163,7 @@ static int rockchip_dfi_get_event(struct devfreq_event_dev *edev,
> u32 access = 0, total = 0;
> int i;
>
> - rockchip_dfi_read_counters(edev, &count);
> + rockchip_dfi_read_counters(dfi, &count);
>
> /* We can only report one channel, so find the busiest one */
> for (i = 0; i < DMC_MAX_CHANNELS; i++) {
> --
> 2.39.2
>
Hi,
On Wed, May 24, 2023 at 10:31:42AM +0200, Sascha Hauer wrote:
> When adding perf support later the DFI must be enabled when
> either of devfreq-event or perf is active. Prepare for that
> by adding a usage counter for the DFI. Also move enabling
> and disabling of the clock away from the devfreq-event specific
> functions to which the perf specific part won't have access.
>
> Signed-off-by: Sascha Hauer <[email protected]>
> Reviewed-by: Jonathan Cameron <[email protected]>
> ---
Reviewed-by: Sebastian Reichel <[email protected]>
-- Sebastian
> drivers/devfreq/event/rockchip-dfi.c | 57 +++++++++++++++++++---------
> 1 file changed, 40 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index d39db5de7f19c..8a7af7c32ae0d 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -68,13 +68,28 @@ struct rockchip_dfi {
> void __iomem *regs;
> struct regmap *regmap_pmu;
> struct clk *clk;
> + int usecount;
> + struct mutex mutex;
> u32 ddr_type;
> unsigned int channel_mask;
> };
>
> -static void rockchip_dfi_start_hardware_counter(struct rockchip_dfi *dfi)
> +static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
> {
> void __iomem *dfi_regs = dfi->regs;
> + int ret = 0;
> +
> + mutex_lock(&dfi->mutex);
> +
> + dfi->usecount++;
> + if (dfi->usecount > 1)
> + goto out;
> +
> + ret = clk_prepare_enable(dfi->clk);
> + if (ret) {
> + dev_err(&dfi->edev->dev, "failed to enable dfi clk: %d\n", ret);
> + goto out;
> + }
>
> /* clear DDRMON_CTRL setting */
> writel_relaxed(HIWORD_UPDATE(0, DDRMON_CTRL_TIMER_CNT_EN | DDRMON_CTRL_SOFTWARE_EN |
> @@ -99,14 +114,30 @@ static void rockchip_dfi_start_hardware_counter(struct rockchip_dfi *dfi)
> /* enable count, use software mode */
> writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_SOFTWARE_EN, DDRMON_CTRL_SOFTWARE_EN),
> dfi_regs + DDRMON_CTRL);
> +out:
> + mutex_unlock(&dfi->mutex);
> +
> + return ret;
> }
>
> -static void rockchip_dfi_stop_hardware_counter(struct rockchip_dfi *dfi)
> +static void rockchip_dfi_disable(struct rockchip_dfi *dfi)
> {
> void __iomem *dfi_regs = dfi->regs;
>
> + mutex_lock(&dfi->mutex);
> +
> + dfi->usecount--;
> +
> + WARN_ON_ONCE(dfi->usecount < 0);
> +
> + if (dfi->usecount > 0)
> + goto out;
> +
> writel_relaxed(HIWORD_UPDATE(0, DDRMON_CTRL_SOFTWARE_EN),
> dfi_regs + DDRMON_CTRL);
> + clk_disable_unprepare(dfi->clk);
> +out:
> + mutex_unlock(&dfi->mutex);
> }
>
> static void rockchip_dfi_read_counters(struct rockchip_dfi *dfi, struct dmc_count *count)
> @@ -124,29 +155,20 @@ static void rockchip_dfi_read_counters(struct rockchip_dfi *dfi, struct dmc_coun
> }
> }
>
> -static int rockchip_dfi_disable(struct devfreq_event_dev *edev)
> +static int rockchip_dfi_event_disable(struct devfreq_event_dev *edev)
> {
> struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
>
> - rockchip_dfi_stop_hardware_counter(dfi);
> - clk_disable_unprepare(dfi->clk);
> + rockchip_dfi_disable(dfi);
>
> return 0;
> }
>
> -static int rockchip_dfi_enable(struct devfreq_event_dev *edev)
> +static int rockchip_dfi_event_enable(struct devfreq_event_dev *edev)
> {
> struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
> - int ret;
> -
> - ret = clk_prepare_enable(dfi->clk);
> - if (ret) {
> - dev_err(&edev->dev, "failed to enable dfi clk: %d\n", ret);
> - return ret;
> - }
>
> - rockchip_dfi_start_hardware_counter(dfi);
> - return 0;
> + return rockchip_dfi_enable(dfi);
> }
>
> static int rockchip_dfi_set_event(struct devfreq_event_dev *edev)
> @@ -190,8 +212,8 @@ static int rockchip_dfi_get_event(struct devfreq_event_dev *edev,
> }
>
> static const struct devfreq_event_ops rockchip_dfi_ops = {
> - .disable = rockchip_dfi_disable,
> - .enable = rockchip_dfi_enable,
> + .disable = rockchip_dfi_event_disable,
> + .enable = rockchip_dfi_event_enable,
> .get_event = rockchip_dfi_get_event,
> .set_event = rockchip_dfi_set_event,
> };
> @@ -272,6 +294,7 @@ static int rockchip_dfi_probe(struct platform_device *pdev)
> return PTR_ERR(dfi->regmap_pmu);
>
> dfi->dev = dev;
> + mutex_init(&dfi->mutex);
>
> desc = &dfi->desc;
> desc->ops = &rockchip_dfi_ops;
> --
> 2.39.2
>
Hi,
On Wed, May 24, 2023 at 10:31:46AM +0200, Sascha Hauer wrote:
> The currently supported RK3399 has a set of registers per channel, but
> it has only a single DDRMON_CTRL register. With upcoming RK3588 this
> will be different, the RK3588 has a DDRMON_CTRL register per channel.
>
> Instead of expecting a single DDRMON_CTRL register, loop over the
> channels and write the channel specific DDRMON_CTRL register. Break
> out early out of the loop when there is only a single DDRMON_CTRL
> register like on the RK3399.
>
> Signed-off-by: Sascha Hauer <[email protected]>
> Reviewed-by: Jonathan Cameron <[email protected]>
> ---
Reviewed-by: Sebastian Reichel <[email protected]>
-- Sebastian
> drivers/devfreq/event/rockchip-dfi.c | 72 ++++++++++++++++++----------
> 1 file changed, 48 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index a872550a7caf5..23d66fe737975 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -113,12 +113,13 @@ struct rockchip_dfi {
> int burst_len;
> int buswidth[DMC_MAX_CHANNELS];
> int ddrmon_stride;
> + bool ddrmon_ctrl_single;
> };
>
> static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
> {
> void __iomem *dfi_regs = dfi->regs;
> - int ret = 0;
> + int i, ret = 0;
>
> mutex_lock(&dfi->mutex);
>
> @@ -132,29 +133,41 @@ static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
> goto out;
> }
>
> - /* clear DDRMON_CTRL setting */
> - writel_relaxed(HIWORD_UPDATE(0, DDRMON_CTRL_TIMER_CNT_EN | DDRMON_CTRL_SOFTWARE_EN |
> - DDRMON_CTRL_HARDWARE_EN), dfi_regs + DDRMON_CTRL);
> + for (i = 0; i < DMC_MAX_CHANNELS; i++) {
> + u32 ctrl = 0;
>
> - /* set ddr type to dfi */
> - switch (dfi->ddr_type) {
> - case ROCKCHIP_DDRTYPE_LPDDR2:
> - case ROCKCHIP_DDRTYPE_LPDDR3:
> - writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_LPDDR23, DDRMON_CTRL_DDR_TYPE_MASK),
> - dfi_regs + DDRMON_CTRL);
> - break;
> - case ROCKCHIP_DDRTYPE_LPDDR4:
> - case ROCKCHIP_DDRTYPE_LPDDR4X:
> - writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_LPDDR4, DDRMON_CTRL_DDR_TYPE_MASK),
> - dfi_regs + DDRMON_CTRL);
> - break;
> - default:
> - break;
> - }
> + if (!(dfi->channel_mask & BIT(i)))
> + continue;
>
> - /* enable count, use software mode */
> - writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_SOFTWARE_EN, DDRMON_CTRL_SOFTWARE_EN),
> - dfi_regs + DDRMON_CTRL);
> + /* clear DDRMON_CTRL setting */
> + writel_relaxed(HIWORD_UPDATE(0, DDRMON_CTRL_TIMER_CNT_EN |
> + DDRMON_CTRL_SOFTWARE_EN | DDRMON_CTRL_HARDWARE_EN),
> + dfi_regs + i * dfi->ddrmon_stride + DDRMON_CTRL);
> +
> + /* set ddr type to dfi */
> + switch (dfi->ddr_type) {
> + case ROCKCHIP_DDRTYPE_LPDDR2:
> + case ROCKCHIP_DDRTYPE_LPDDR3:
> + ctrl = DDRMON_CTRL_LPDDR23;
> + break;
> + case ROCKCHIP_DDRTYPE_LPDDR4:
> + case ROCKCHIP_DDRTYPE_LPDDR4X:
> + ctrl = DDRMON_CTRL_LPDDR4;
> + break;
> + default:
> + break;
> + }
> +
> + writel_relaxed(HIWORD_UPDATE(ctrl, DDRMON_CTRL_DDR_TYPE_MASK),
> + dfi_regs + i * dfi->ddrmon_stride + DDRMON_CTRL);
> +
> + /* enable count, use software mode */
> + writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_SOFTWARE_EN, DDRMON_CTRL_SOFTWARE_EN),
> + dfi_regs + i * dfi->ddrmon_stride + DDRMON_CTRL);
> +
> + if (dfi->ddrmon_ctrl_single)
> + break;
> + }
> out:
> mutex_unlock(&dfi->mutex);
>
> @@ -164,6 +177,7 @@ static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
> static void rockchip_dfi_disable(struct rockchip_dfi *dfi)
> {
> void __iomem *dfi_regs = dfi->regs;
> + int i;
>
> mutex_lock(&dfi->mutex);
>
> @@ -174,8 +188,17 @@ static void rockchip_dfi_disable(struct rockchip_dfi *dfi)
> if (dfi->usecount > 0)
> goto out;
>
> - writel_relaxed(HIWORD_UPDATE(0, DDRMON_CTRL_SOFTWARE_EN),
> - dfi_regs + DDRMON_CTRL);
> + for (i = 0; i < DMC_MAX_CHANNELS; i++) {
> + if (!(dfi->channel_mask & BIT(i)))
> + continue;
> +
> + writel_relaxed(HIWORD_UPDATE(0, DDRMON_CTRL_SOFTWARE_EN),
> + dfi_regs + i * dfi->ddrmon_stride + DDRMON_CTRL);
> +
> + if (dfi->ddrmon_ctrl_single)
> + break;
> + }
> +
> clk_disable_unprepare(dfi->clk);
> out:
> mutex_unlock(&dfi->mutex);
> @@ -663,6 +686,7 @@ static int rk3399_dfi_init(struct rockchip_dfi *dfi)
> dfi->buswidth[1] = FIELD_GET(RK3399_PMUGRF_OS_REG2_BW_CH1, val) == 0 ? 4 : 2;
>
> dfi->ddrmon_stride = 0x14;
> + dfi->ddrmon_ctrl_single = true;
>
> return 0;
> };
> --
> 2.39.2
>
Hello Sascha, Sebastian,
On Wed, Jun 14, 2023 at 1:40 PM Sebastian Reichel
<[email protected]> wrote:
> On Wed, May 24, 2023 at 10:31:28AM +0200, Sascha Hauer wrote:
> > This is v5 of the series adding perf support to the rockchip DFI driver.
> > [...]
> > The RK3588 device tree changes for the DFI were not part of v4. As
> > Vincent Legoll showed interest in testing this series the necessary
> > device tree changes are now part of this series.
>
> I tested the series on RK3588 EVB1. The read/write byts looks
> sensible. Sometimes cycles reads unrealistic values, though:
> [...]
> Otherwise the series is
>
> Tested-by: Sebastian Reichel <[email protected]>
>
> -- Sebastian
I also tested this new version of the series on a Pine64 QuartzPro64 dev board.
I applied the series on top of my local branch, which is based on Collabora's
rockchip-3588 plus some QP64 DTS patches, and your V5 patch series.
Looks like this is still working properly:
-bash-5.1# uname -a
Linux qp64 6.4.0-rc1-00140-g658dd2200e2a #24 SMP PREEMPT Wed Jun 14
15:50:34 CEST 2023 aarch64 GNU/Linux
-bash-5.1# zgrep -i _dfi /proc/config.gz
CONFIG_DEVFREQ_EVENT_ROCKCHIP_DFI=y
-bash-5.1# perf list | grep rockchip_ddr
rockchip_ddr/bytes/ [Kernel PMU event]
rockchip_ddr/cycles/ [Kernel PMU event]
rockchip_ddr/read-bytes/ [Kernel PMU event]
rockchip_ddr/read-bytes0/ [Kernel PMU event]
rockchip_ddr/read-bytes1/ [Kernel PMU event]
rockchip_ddr/read-bytes2/ [Kernel PMU event]
rockchip_ddr/read-bytes3/ [Kernel PMU event]
rockchip_ddr/write-bytes/ [Kernel PMU event]
rockchip_ddr/write-bytes0/ [Kernel PMU event]
rockchip_ddr/write-bytes1/ [Kernel PMU event]
rockchip_ddr/write-bytes2/ [Kernel PMU event]
rockchip_ddr/write-bytes3/ [Kernel PMU event]
# With no memory load
-bash-5.1# 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':
1058691047 rockchip_ddr/cycles/
9.35 MB rockchip_ddr/read-bytes/
0.57 MB rockchip_ddr/write-bytes/
9.90 MB rockchip_ddr/bytes/
1.002616498 seconds time elapsed
# With a hog
-bash-5.1# memtester 4G > /dev/null 2>&1 &
-bash-5.1# perf stat -a -e
rockchip_ddr/cycles/,rockchip_ddr/read-bytes/,rockchip_ddr/write-bytes/,rockchip_ddr/bytes/
sleep 10
Performance counter stats for 'system wide':
10561540038 rockchip_ddr/cycles/
60212.59 MB rockchip_ddr/read-bytes/
31313.03 MB rockchip_ddr/write-bytes/
91525.60 MB rockchip_ddr/bytes/
10.001651886 seconds time elapsed
You can add my T-B, for the whole series:
Tested-by: Vincent Legoll <[email protected]>
Or is there something else you want me to test ?
Thanks for your work
Regards
--
Vincent Legoll
Hi,
On Wed, May 24, 2023 at 10:31:44AM +0200, Sascha Hauer wrote:
> 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.
>
> Signed-off-by: Sascha Hauer <[email protected]>
Reviewed-by: Sebastian Reichel <[email protected]>
-- Sebastian
> ---
>
> Notes:
> 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 | 439 ++++++++++++++++++++++++++-
> include/soc/rockchip/rk3399_grf.h | 2 +
> include/soc/rockchip/rk3568_grf.h | 1 +
> 3 files changed, 437 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 50e497455dc69..88145688e3d9c 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
> */
> struct dmc_count_channel {
> - u32 access;
> - u32 clock_cycles;
> + u64 access;
> + u64 clock_cycles;
> + u64 read_access;
> + u64 write_access;
> };
>
> struct dmc_count {
> @@ -69,6 +91,11 @@ struct rockchip_dfi {
> struct devfreq_event_dev *edev;
> struct devfreq_event_desc desc;
> struct dmc_count last_event_count;
> +
> + struct dmc_count last_perf_count;
> + struct dmc_count total_count;
> + seqlock_t count_seqlock; /* protects last_perf_count and total_count */
> +
> struct device *dev;
> void __iomem *regs;
> struct regmap *regmap_pmu;
> @@ -77,6 +104,14 @@ struct rockchip_dfi {
> struct mutex mutex;
> u32 ddr_type;
> unsigned int channel_mask;
> + enum cpuhp_state cpuhp_state;
> + struct hlist_node node;
> + struct pmu pmu;
> + struct hrtimer timer;
> + unsigned int cpu;
> + int active_events;
> + int burst_len;
> + int buswidth[DMC_MAX_CHANNELS];
> };
>
> static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
> @@ -145,7 +180,7 @@ static void rockchip_dfi_disable(struct rockchip_dfi *dfi)
> mutex_unlock(&dfi->mutex);
> }
>
> -static void rockchip_dfi_read_counters(struct rockchip_dfi *dfi, struct dmc_count *count)
> +static void rockchip_dfi_read_counters(struct rockchip_dfi *dfi, struct dmc_count *c)
> {
> u32 i;
> void __iomem *dfi_regs = dfi->regs;
> @@ -153,13 +188,36 @@ static void rockchip_dfi_read_counters(struct rockchip_dfi *dfi, struct dmc_coun
> for (i = 0; i < DMC_MAX_CHANNELS; i++) {
> if (!(dfi->channel_mask & BIT(i)))
> continue;
> - count->c[i].access = readl_relaxed(dfi_regs +
> + c->c[i].read_access = readl_relaxed(dfi_regs +
> + DDRMON_CH0_RD_NUM + i * 20);
> + c->c[i].write_access = readl_relaxed(dfi_regs +
> + DDRMON_CH0_WR_NUM + i * 20);
> + c->c[i].access = readl_relaxed(dfi_regs +
> DDRMON_CH0_DFI_ACCESS_NUM + i * 20);
> - count->c[i].clock_cycles = readl_relaxed(dfi_regs +
> + c->c[i].clock_cycles = readl_relaxed(dfi_regs +
> DDRMON_CH0_COUNT_NUM + i * 20);
> }
> }
>
> +static void rockchip_ddr_perf_counters_add(struct rockchip_dfi *dfi,
> + const struct dmc_count *now,
> + struct dmc_count *res)
> +{
> + const struct dmc_count *last = &dfi->last_perf_count;
> + int i;
> +
> + for (i = 0; i < DMC_MAX_CHANNELS; i++) {
> + res->c[i].read_access = dfi->total_count.c[i].read_access +
> + (u32)(now->c[i].read_access - last->c[i].read_access);
> + res->c[i].write_access = dfi->total_count.c[i].write_access +
> + (u32)(now->c[i].write_access - last->c[i].write_access);
> + res->c[i].access = dfi->total_count.c[i].access +
> + (u32)(now->c[i].access - last->c[i].access);
> + res->c[i].clock_cycles = dfi->total_count.c[i].clock_cycles +
> + (u32)(now->c[i].clock_cycles - last->c[i].clock_cycles);
> + }
> +}
> +
> static int rockchip_dfi_event_disable(struct devfreq_event_dev *edev)
> {
> struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
> @@ -223,6 +281,367 @@ static const struct devfreq_event_ops rockchip_dfi_ops = {
> .set_event = rockchip_dfi_set_event,
> };
>
> +#ifdef CONFIG_PERF_EVENTS
> +
> +static ssize_t ddr_perf_cpumask_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct pmu *pmu = dev_get_drvdata(dev);
> + struct rockchip_dfi *dfi = container_of(pmu, struct rockchip_dfi, pmu);
> +
> + return cpumap_print_to_pagebuf(true, buf, cpumask_of(dfi->cpu));
> +}
> +
> +static struct device_attribute ddr_perf_cpumask_attr =
> + __ATTR(cpumask, 0444, ddr_perf_cpumask_show, NULL);
> +
> +static struct attribute *ddr_perf_cpumask_attrs[] = {
> + &ddr_perf_cpumask_attr.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group ddr_perf_cpumask_attr_group = {
> + .attrs = ddr_perf_cpumask_attrs,
> +};
> +
> +PMU_EVENT_ATTR_STRING(cycles, ddr_pmu_cycles, "event="__stringify(PERF_EVENT_CYCLES))
> +
> +#define DFI_PMU_EVENT_ATTR(_name, _var, _str) \
> + PMU_EVENT_ATTR_STRING(_name, _var, _str); \
> + PMU_EVENT_ATTR_STRING(_name.unit, _var##_unit, "MB"); \
> + PMU_EVENT_ATTR_STRING(_name.scale, _var##_scale, "9.536743164e-07")
> +
> +DFI_PMU_EVENT_ATTR(read-bytes0, ddr_pmu_read_bytes0, "event="__stringify(PERF_EVENT_READ_BYTES0));
> +DFI_PMU_EVENT_ATTR(write-bytes0, ddr_pmu_write_bytes0, "event="__stringify(PERF_EVENT_WRITE_BYTES0));
> +
> +DFI_PMU_EVENT_ATTR(read-bytes1, ddr_pmu_read_bytes1, "event="__stringify(PERF_EVENT_READ_BYTES1));
> +DFI_PMU_EVENT_ATTR(write-bytes1, ddr_pmu_write_bytes1, "event="__stringify(PERF_EVENT_WRITE_BYTES1));
> +
> +DFI_PMU_EVENT_ATTR(read-bytes2, ddr_pmu_read_bytes2, "event="__stringify(PERF_EVENT_READ_BYTES2));
> +DFI_PMU_EVENT_ATTR(write-bytes2, ddr_pmu_write_bytes2, "event="__stringify(PERF_EVENT_WRITE_BYTES2));
> +
> +DFI_PMU_EVENT_ATTR(read-bytes3, ddr_pmu_read_bytes3, "event="__stringify(PERF_EVENT_READ_BYTES3));
> +DFI_PMU_EVENT_ATTR(write-bytes3, ddr_pmu_write_bytes3, "event="__stringify(PERF_EVENT_WRITE_BYTES3));
> +
> +DFI_PMU_EVENT_ATTR(read-bytes, ddr_pmu_read_bytes, "event="__stringify(PERF_EVENT_READ_BYTES));
> +DFI_PMU_EVENT_ATTR(write-bytes, ddr_pmu_write_bytes, "event="__stringify(PERF_EVENT_WRITE_BYTES));
> +
> +DFI_PMU_EVENT_ATTR(bytes, ddr_pmu_bytes, "event="__stringify(PERF_EVENT_BYTES));
> +
> +#define DFI_ATTR_MB(_name) \
> + &_name.attr.attr, \
> + &_name##_unit.attr.attr, \
> + &_name##_scale.attr.attr
> +
> +static struct attribute *ddr_perf_events_attrs[] = {
> + &ddr_pmu_cycles.attr.attr,
> + DFI_ATTR_MB(ddr_pmu_read_bytes),
> + DFI_ATTR_MB(ddr_pmu_write_bytes),
> + DFI_ATTR_MB(ddr_pmu_read_bytes0),
> + DFI_ATTR_MB(ddr_pmu_write_bytes0),
> + DFI_ATTR_MB(ddr_pmu_read_bytes1),
> + DFI_ATTR_MB(ddr_pmu_write_bytes1),
> + DFI_ATTR_MB(ddr_pmu_read_bytes2),
> + DFI_ATTR_MB(ddr_pmu_write_bytes2),
> + DFI_ATTR_MB(ddr_pmu_read_bytes3),
> + DFI_ATTR_MB(ddr_pmu_write_bytes3),
> + DFI_ATTR_MB(ddr_pmu_bytes),
> + NULL,
> +};
> +
> +static const struct attribute_group ddr_perf_events_attr_group = {
> + .name = "events",
> + .attrs = ddr_perf_events_attrs,
> +};
> +
> +PMU_FORMAT_ATTR(event, "config:0-7");
> +
> +static struct attribute *ddr_perf_format_attrs[] = {
> + &format_attr_event.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group ddr_perf_format_attr_group = {
> + .name = "format",
> + .attrs = ddr_perf_format_attrs,
> +};
> +
> +static const struct attribute_group *attr_groups[] = {
> + &ddr_perf_events_attr_group,
> + &ddr_perf_cpumask_attr_group,
> + &ddr_perf_format_attr_group,
> + NULL,
> +};
> +
> +static int rockchip_ddr_perf_event_init(struct perf_event *event)
> +{
> + struct rockchip_dfi *dfi = container_of(event->pmu, struct rockchip_dfi, pmu);
> +
> + if (event->attr.type != event->pmu->type)
> + return -ENOENT;
> +
> + if (event->attach_state & PERF_ATTACH_TASK)
> + return -EINVAL;
> +
> + if (event->cpu < 0) {
> + dev_warn(dfi->dev, "Can't provide per-task data!\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static u64 rockchip_ddr_perf_event_get_count(struct perf_event *event)
> +{
> + struct rockchip_dfi *dfi = container_of(event->pmu, struct rockchip_dfi, pmu);
> + int blen = dfi->burst_len;
> + struct dmc_count total, now;
> + unsigned int seq;
> + u64 c = 0;
> + int i;
> +
> + rockchip_dfi_read_counters(dfi, &now);
> +
> + do {
> + seq = read_seqbegin(&dfi->count_seqlock);
> +
> + rockchip_ddr_perf_counters_add(dfi, &now, &total);
> +
> + } while (read_seqretry(&dfi->count_seqlock, seq));
> +
> + switch (event->attr.config) {
> + case PERF_EVENT_CYCLES:
> + c = total.c[0].clock_cycles;
> + break;
> + case PERF_EVENT_READ_BYTES:
> + for (i = 0; i < DMC_MAX_CHANNELS; i++)
> + c += total.c[i].read_access * blen * dfi->buswidth[i];
> + break;
> + case PERF_EVENT_WRITE_BYTES:
> + for (i = 0; i < DMC_MAX_CHANNELS; i++)
> + c += total.c[i].write_access * blen * dfi->buswidth[i];
> + break;
> + case PERF_EVENT_READ_BYTES0:
> + c = total.c[0].read_access * blen * dfi->buswidth[0];
> + break;
> + case PERF_EVENT_WRITE_BYTES0:
> + c = total.c[0].write_access * blen * dfi->buswidth[0];
> + break;
> + case PERF_EVENT_READ_BYTES1:
> + c = total.c[1].read_access * blen * dfi->buswidth[1];
> + break;
> + case PERF_EVENT_WRITE_BYTES1:
> + c = total.c[1].write_access * blen * dfi->buswidth[1];
> + break;
> + case PERF_EVENT_READ_BYTES2:
> + c = total.c[2].read_access * blen * dfi->buswidth[2];
> + break;
> + case PERF_EVENT_WRITE_BYTES2:
> + c = total.c[2].write_access * blen * dfi->buswidth[2];
> + break;
> + case PERF_EVENT_READ_BYTES3:
> + c = total.c[3].read_access * blen * dfi->buswidth[3];
> + break;
> + case PERF_EVENT_WRITE_BYTES3:
> + c = total.c[3].write_access * blen * dfi->buswidth[3];
> + break;
> + case PERF_EVENT_BYTES:
> + for (i = 0; i < DMC_MAX_CHANNELS; i++)
> + c += total.c[i].access * blen * dfi->buswidth[i];
> + break;
> + }
> +
> + return c;
> +}
> +
> +static void rockchip_ddr_perf_event_update(struct perf_event *event)
> +{
> + u64 now;
> + s64 prev;
> +
> + if (event->attr.config >= PERF_ACCESS_TYPE_MAX)
> + return;
> +
> + now = rockchip_ddr_perf_event_get_count(event);
> + prev = local64_xchg(&event->hw.prev_count, now);
> + local64_add(now - prev, &event->count);
> +}
> +
> +static void rockchip_ddr_perf_event_start(struct perf_event *event, int flags)
> +{
> + u64 now = rockchip_ddr_perf_event_get_count(event);
> +
> + local64_set(&event->hw.prev_count, now);
> +}
> +
> +static int rockchip_ddr_perf_event_add(struct perf_event *event, int flags)
> +{
> + struct rockchip_dfi *dfi = container_of(event->pmu, struct rockchip_dfi, pmu);
> +
> + dfi->active_events++;
> +
> + if (dfi->active_events == 1)
> + hrtimer_start(&dfi->timer, ns_to_ktime(NSEC_PER_SEC), HRTIMER_MODE_REL);
> +
> + if (flags & PERF_EF_START)
> + rockchip_ddr_perf_event_start(event, flags);
> +
> + return 0;
> +}
> +
> +static void rockchip_ddr_perf_event_stop(struct perf_event *event, int flags)
> +{
> + rockchip_ddr_perf_event_update(event);
> +}
> +
> +static void rockchip_ddr_perf_event_del(struct perf_event *event, int flags)
> +{
> + struct rockchip_dfi *dfi = container_of(event->pmu, struct rockchip_dfi, pmu);
> +
> + rockchip_ddr_perf_event_stop(event, PERF_EF_UPDATE);
> +
> + dfi->active_events--;
> +
> + if (dfi->active_events == 0)
> + hrtimer_cancel(&dfi->timer);
> +}
> +
> +static enum hrtimer_restart rockchip_dfi_timer(struct hrtimer *timer)
> +{
> + struct rockchip_dfi *dfi = container_of(timer, struct rockchip_dfi, timer);
> + struct dmc_count now, total;
> +
> + rockchip_dfi_read_counters(dfi, &now);
> +
> + write_seqlock(&dfi->count_seqlock);
> +
> + rockchip_ddr_perf_counters_add(dfi, &now, &total);
> + dfi->total_count = total;
> + dfi->last_perf_count = now;
> +
> + write_sequnlock(&dfi->count_seqlock);
> +
> + hrtimer_forward_now(&dfi->timer, ns_to_ktime(NSEC_PER_SEC));
> +
> + return HRTIMER_RESTART;
> +};
> +
> +static int ddr_perf_offline_cpu(unsigned int cpu, struct hlist_node *node)
> +{
> + struct rockchip_dfi *dfi = hlist_entry_safe(node, struct rockchip_dfi, node);
> + int target;
> +
> + if (cpu != dfi->cpu)
> + return 0;
> +
> + target = cpumask_any_but(cpu_online_mask, cpu);
> + if (target >= nr_cpu_ids)
> + return 0;
> +
> + perf_pmu_migrate_context(&dfi->pmu, cpu, target);
> + dfi->cpu = target;
> +
> + return 0;
> +}
> +
> +static void rockchip_ddr_cpuhp_remove_state(void *data)
> +{
> + struct rockchip_dfi *dfi = data;
> +
> + cpuhp_remove_multi_state(dfi->cpuhp_state);
> +
> + rockchip_dfi_disable(dfi);
> +}
> +
> +static void rockchip_ddr_cpuhp_remove_instance(void *data)
> +{
> + struct rockchip_dfi *dfi = data;
> +
> + cpuhp_state_remove_instance_nocalls(dfi->cpuhp_state, &dfi->node);
> +}
> +
> +static void rockchip_ddr_perf_remove(void *data)
> +{
> + struct rockchip_dfi *dfi = data;
> +
> + perf_pmu_unregister(&dfi->pmu);
> +}
> +
> +static int rockchip_ddr_perf_init(struct rockchip_dfi *dfi)
> +{
> + struct pmu *pmu = &dfi->pmu;
> + int ret;
> +
> + seqlock_init(&dfi->count_seqlock);
> +
> + pmu->module = THIS_MODULE;
> + pmu->capabilities = PERF_PMU_CAP_NO_EXCLUDE;
> + pmu->task_ctx_nr = perf_invalid_context;
> + pmu->attr_groups = attr_groups;
> + pmu->event_init = rockchip_ddr_perf_event_init;
> + pmu->add = rockchip_ddr_perf_event_add;
> + pmu->del = rockchip_ddr_perf_event_del;
> + pmu->start = rockchip_ddr_perf_event_start;
> + pmu->stop = rockchip_ddr_perf_event_stop;
> + pmu->read = rockchip_ddr_perf_event_update;
> +
> + dfi->cpu = raw_smp_processor_id();
> +
> + ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN,
> + "rockchip_ddr_perf_pmu",
> + NULL,
> + ddr_perf_offline_cpu);
> +
> + if (ret < 0) {
> + dev_err(dfi->dev, "cpuhp_setup_state_multi failed: %d\n", ret);
> + return ret;
> + }
> +
> + dfi->cpuhp_state = ret;
> +
> + rockchip_dfi_enable(dfi);
> +
> + ret = devm_add_action_or_reset(dfi->dev, rockchip_ddr_cpuhp_remove_state, dfi);
> + if (ret)
> + return ret;
> +
> + ret = cpuhp_state_add_instance_nocalls(dfi->cpuhp_state, &dfi->node);
> + if (ret) {
> + dev_err(dfi->dev, "Error %d registering hotplug\n", ret);
> + return ret;
> + }
> +
> + ret = devm_add_action_or_reset(dfi->dev, rockchip_ddr_cpuhp_remove_instance, dfi);
> + if (ret)
> + return ret;
> +
> + hrtimer_init(&dfi->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + dfi->timer.function = rockchip_dfi_timer;
> +
> + switch (dfi->ddr_type) {
> + case ROCKCHIP_DDRTYPE_LPDDR2:
> + case ROCKCHIP_DDRTYPE_LPDDR3:
> + dfi->burst_len = 8;
> + break;
> + case ROCKCHIP_DDRTYPE_LPDDR4:
> + case ROCKCHIP_DDRTYPE_LPDDR4X:
> + dfi->burst_len = 16;
> + break;
> + }
> +
> + ret = perf_pmu_register(pmu, "rockchip_ddr", -1);
> + if (ret)
> + return ret;
> +
> + return devm_add_action_or_reset(dfi->dev, rockchip_ddr_perf_remove, dfi);
> +}
> +#else
> +static int rockchip_ddr_perf_init(struct rockchip_dfi *dfi)
> +{
> + return 0;
> +}
> +#endif
> +
> static int rk3399_dfi_init(struct rockchip_dfi *dfi)
> {
> struct regmap *regmap_pmu = dfi->regmap_pmu;
> @@ -239,6 +658,9 @@ static int rk3399_dfi_init(struct rockchip_dfi *dfi)
>
> dfi->channel_mask = GENMASK(1, 0);
>
> + dfi->buswidth[0] = FIELD_GET(RK3399_PMUGRF_OS_REG2_BW_CH0, val) == 0 ? 4 : 2;
> + dfi->buswidth[1] = FIELD_GET(RK3399_PMUGRF_OS_REG2_BW_CH1, val) == 0 ? 4 : 2;
> +
> return 0;
> };
>
> @@ -255,6 +677,8 @@ static int rk3568_dfi_init(struct rockchip_dfi *dfi)
> if (FIELD_GET(RK3568_PMUGRF_OS_REG3_SYSREG_VERSION, reg3) >= 0x3)
> dfi->ddr_type |= FIELD_GET(RK3568_PMUGRF_OS_REG3_DRAMTYPE_INFO_V3, reg3) << 3;
>
> + dfi->buswidth[0] = FIELD_GET(RK3568_PMUGRF_OS_REG2_BW_CH0, reg2) == 0 ? 4 : 2;
> +
> dfi->channel_mask = 1;
>
> return 0;
> @@ -317,6 +741,10 @@ static int rockchip_dfi_probe(struct platform_device *pdev)
> return PTR_ERR(dfi->edev);
> }
>
> + ret = rockchip_ddr_perf_init(dfi);
> + if (ret)
> + return ret;
> +
> platform_set_drvdata(pdev, dfi);
>
> return 0;
> @@ -327,6 +755,7 @@ static struct platform_driver rockchip_dfi_driver = {
> .driver = {
> .name = "rockchip-dfi",
> .of_match_table = rockchip_dfi_id_match,
> + .suppress_bind_attrs = true,
> },
> };
> module_platform_driver(rockchip_dfi_driver);
> diff --git a/include/soc/rockchip/rk3399_grf.h b/include/soc/rockchip/rk3399_grf.h
> index 775f8444bea8d..39cd44cec982f 100644
> --- a/include/soc/rockchip/rk3399_grf.h
> +++ b/include/soc/rockchip/rk3399_grf.h
> @@ -12,5 +12,7 @@
> /* PMU GRF Registers */
> #define RK3399_PMUGRF_OS_REG2 0x308
> #define RK3399_PMUGRF_OS_REG2_DDRTYPE GENMASK(15, 13)
> +#define RK3399_PMUGRF_OS_REG2_BW_CH0 GENMASK(3, 2)
> +#define RK3399_PMUGRF_OS_REG2_BW_CH1 GENMASK(19, 18)
>
> #endif
> diff --git a/include/soc/rockchip/rk3568_grf.h b/include/soc/rockchip/rk3568_grf.h
> index 575584e9d8834..52853efd6720e 100644
> --- a/include/soc/rockchip/rk3568_grf.h
> +++ b/include/soc/rockchip/rk3568_grf.h
> @@ -4,6 +4,7 @@
>
> #define RK3568_PMUGRF_OS_REG2 0x208
> #define RK3568_PMUGRF_OS_REG2_DRAMTYPE_INFO GENMASK(15, 13)
> +#define RK3568_PMUGRF_OS_REG2_BW_CH0 GENMASK(3, 2)
>
> #define RK3568_PMUGRF_OS_REG3 0x20c
> #define RK3568_PMUGRF_OS_REG3_DRAMTYPE_INFO_V3 GENMASK(13, 12)
> --
> 2.39.2
>
Hi,
On Wed, May 24, 2023 at 10:31:28AM +0200, Sascha Hauer wrote:
> This is v5 of the series adding perf support to the rockchip DFI driver.
>
> A lot has changed in the perf driver since v4. First of all the review
> feedback from Robin and Jonathan has been integrated. The perf driver
> now not only supports monitoring the total DDR utilization, but also the
> individual channels. I also reworked the way the raw 32bit counter
> values are summed up to 64bit perf values, so hopefully the code is
> easier to follow now.
>
> lockdep found out that that locking in the perf driver was broken, so I
> reworked that as well. None of the perf hooks allows locking with
> mutexes or spinlocks, so in perf it's not possible to enable the DFI
> controller when needed. Instead I now unconditionally enable the DFI
> controller during probe when perf is enabled.
>
> Furthermore the hrtimer I use for reading out the hardware counter
> values before they overflow race with perf. Now a seqlock is used to
> prevent that.
>
> The RK3588 device tree changes for the DFI were not part of v4. As
> Vincent Legoll showed interest in testing this series the necessary
> device tree changes are now part of this series.
I tested the series on RK3588 EVB1. The read/write byts looks
sensible. Sometimes cycles reads unrealistic values, though:
Performance counter stats for 'system wide':
18446744070475110400 rockchip_ddr/cycles/
828.63 MB rockchip_ddr/read-bytes/
207.19 MB rockchip_ddr/read-bytes0/
207.15 MB rockchip_ddr/read-bytes1/
207.14 MB rockchip_ddr/read-bytes2/
207.15 MB rockchip_ddr/read-bytes3/
1.48 MB rockchip_ddr/write-bytes/
0.37 MB rockchip_ddr/write-bytes0/
0.37 MB rockchip_ddr/write-bytes1/
0.37 MB rockchip_ddr/write-bytes2/
0.38 MB rockchip_ddr/write-bytes3/
830.12 MB rockchip_ddr/bytes/
1.004239766 seconds time elapsed
(This is with memdump running in parallel)
Otherwise the series is
Tested-by: Sebastian Reichel <[email protected]>
-- Sebastian
> Changes since v4:
> - Add device tree changes for RK3588
> - Use seqlock to protect perf counter values from hrtimer
> - Unconditionally enable DFI when perf is enabled
> - Bring back changes to dts/binding patches that were lost in v4
>
> Changes since v3:
> - Add RK3588 support
>
> Changes since v2:
> - Fix broken reference to binding
> - Add Reviewed-by from Rob
>
> Changes since v1:
> - Fix example to actually match the binding and fix the warnings resulted thereof
> - Make addition of rockchip,rk3568-dfi an extra patch
>
> Sascha Hauer (25):
> PM / devfreq: rockchip-dfi: Make pmu regmap mandatory
> PM / devfreq: rockchip-dfi: Embed desc into private data struct
> PM / devfreq: rockchip-dfi: use consistent name for private data
> struct
> PM / devfreq: rockchip-dfi: Add SoC specific init function
> PM / devfreq: rockchip-dfi: dfi store raw values in counter struct
> PM / devfreq: rockchip-dfi: Use free running counter
> PM / devfreq: rockchip-dfi: introduce channel mask
> PM / devfreq: rk3399_dmc,dfi: generalize DDRTYPE defines
> PM / devfreq: rockchip-dfi: Clean up DDR type register defines
> PM / devfreq: rockchip-dfi: Add RK3568 support
> PM / devfreq: rockchip-dfi: Handle LPDDR2 correctly
> PM / devfreq: rockchip-dfi: Handle LPDDR4X
> PM / devfreq: rockchip-dfi: Pass private data struct to internal
> functions
> PM / devfreq: rockchip-dfi: Prepare for multiple users
> PM / devfreq: rockchip-dfi: give variable a better name
> PM / devfreq: rockchip-dfi: Add perf support
> PM / devfreq: rockchip-dfi: make register stride SoC specific
> PM / devfreq: rockchip-dfi: account for multiple DDRMON_CTRL registers
> PM / devfreq: rockchip-dfi: add support for RK3588
> dt-bindings: devfreq: event: convert Rockchip DFI binding to yaml
> dt-bindings: devfreq: event: rockchip,dfi: Add rk3568 support
> dt-bindings: devfreq: event: rockchip,dfi: Add rk3588 support
> arm64: dts: rockchip: rk3399: Enable DFI
> arm64: dts: rockchip: rk356x: Add DFI
> arm64: dts: rockchip: rk3588s: Add DFI
>
> .../bindings/devfreq/event/rockchip,dfi.yaml | 84 ++
> .../bindings/devfreq/event/rockchip-dfi.txt | 18 -
> .../rockchip,rk3399-dmc.yaml | 2 +-
> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 1 -
> arch/arm64/boot/dts/rockchip/rk356x.dtsi | 7 +
> arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 16 +
> drivers/devfreq/event/rockchip-dfi.c | 796 +++++++++++++++---
> drivers/devfreq/rk3399_dmc.c | 10 +-
> include/soc/rockchip/rk3399_grf.h | 9 +-
> include/soc/rockchip/rk3568_grf.h | 13 +
> include/soc/rockchip/rk3588_grf.h | 18 +
> include/soc/rockchip/rockchip_grf.h | 18 +
> 12 files changed, 854 insertions(+), 138 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/devfreq/event/rockchip,dfi.yaml
> delete mode 100644 Documentation/devicetree/bindings/devfreq/event/rockchip-dfi.txt
> create mode 100644 include/soc/rockchip/rk3568_grf.h
> create mode 100644 include/soc/rockchip/rk3588_grf.h
> create mode 100644 include/soc/rockchip/rockchip_grf.h
>
> --
> 2.39.2
>
Hi,
On Wed, Jun 14, 2023 at 03:40:34PM +0200, Sebastian Reichel wrote:
> I tested the series on RK3588 EVB1. The read/write byts looks
> sensible. Sometimes cycles reads unrealistic values, though:
>
> 18446744070475110400 rockchip_ddr/cycles/
I have seen this going off a few times with and without memory
pressure. If it's way off, it always seems to follow the same
pattern: The upper 32 bits are 0xffffffff instead of 0x00000000
with the lower 32 bits containing sensible data.
-- Sebastian
Hi,
On Wed, Jun 14, 2023 at 3:27 PM Sebastian Reichel
<[email protected]> wrote:
> On Wed, Jun 14, 2023 at 03:40:34PM +0200, Sebastian Reichel wrote:
> > I tested the series on RK3588 EVB1. The read/write byts looks
> > sensible. Sometimes cycles reads unrealistic values, though:
> >
> > 18446744070475110400 rockchip_ddr/cycles/
>
> I have seen this going off a few times with and without memory
> pressure. If it's way off, it always seems to follow the same
> pattern: The upper 32 bits are 0xffffffff instead of 0x00000000
> with the lower 32 bits containing sensible data.
How often is that happening ?
I have been running this in a loop with varying sleep duration for
the last few hours without hitting it, with and without memtester.
REPEATS=1000
MAX_DURATION=100
OUT="/tmp/perf-dfi-rk3588-${REPEATS}x${MAX_DURATION}s.txt"
echo -n > $OUT
for i in $(seq $REPEATS) ; do
DURATION=$(shuf -i "0-${MAX_DURATION}" -n 1)
echo -n "${DURATION} : " >> $OUT
perf stat -a -e rockchip_ddr/cycles/ sleep $DURATION 2>&1 | awk
'/rockchip_ddr/ {printf("%X\n", int($1))}' >> $OUT
done
BTW, it's on a musl-libc arm64 void linux userspace.
--
Vincent Legoll
Hi,
On Wed, Jun 14, 2023 at 07:51:21PM +0000, Vincent Legoll wrote:
> On Wed, Jun 14, 2023 at 3:27 PM Sebastian Reichel
> <[email protected]> wrote:
> > On Wed, Jun 14, 2023 at 03:40:34PM +0200, Sebastian Reichel wrote:
> > > I tested the series on RK3588 EVB1. The read/write byts looks
> > > sensible. Sometimes cycles reads unrealistic values, though:
> > >
> > > 18446744070475110400 rockchip_ddr/cycles/
> >
> > I have seen this going off a few times with and without memory
> > pressure. If it's way off, it always seems to follow the same
> > pattern: The upper 32 bits are 0xffffffff instead of 0x00000000
> > with the lower 32 bits containing sensible data.
>
> How often is that happening ?
I saw it multiple times (4 or 5) within a few tries (I guess around
20). I could see it with and without applying load on the memory.
Tests have been running globally for a second using 'sleep 1' (just
like the example from Sascha Hauer in the perf patch)
> BTW, it's on a musl-libc arm64 void linux userspace.
In my case it's Debian unstable.
-- Sebastian
Hi Sebastian,
On Wed, Jun 14, 2023 at 03:40:34PM +0200, Sebastian Reichel wrote:
> Hi,
>
> On Wed, May 24, 2023 at 10:31:28AM +0200, Sascha Hauer wrote:
> > This is v5 of the series adding perf support to the rockchip DFI driver.
> >
> > A lot has changed in the perf driver since v4. First of all the review
> > feedback from Robin and Jonathan has been integrated. The perf driver
> > now not only supports monitoring the total DDR utilization, but also the
> > individual channels. I also reworked the way the raw 32bit counter
> > values are summed up to 64bit perf values, so hopefully the code is
> > easier to follow now.
> >
> > lockdep found out that that locking in the perf driver was broken, so I
> > reworked that as well. None of the perf hooks allows locking with
> > mutexes or spinlocks, so in perf it's not possible to enable the DFI
> > controller when needed. Instead I now unconditionally enable the DFI
> > controller during probe when perf is enabled.
> >
> > Furthermore the hrtimer I use for reading out the hardware counter
> > values before they overflow race with perf. Now a seqlock is used to
> > prevent that.
> >
> > The RK3588 device tree changes for the DFI were not part of v4. As
> > Vincent Legoll showed interest in testing this series the necessary
> > device tree changes are now part of this series.
>
> I tested the series on RK3588 EVB1. The read/write byts looks
> sensible. Sometimes cycles reads unrealistic values, though:
>
> Performance counter stats for 'system wide':
>
> 18446744070475110400 rockchip_ddr/cycles/
I'll have a look later this day. I remember seeing this, but I thought
this had been resolved already.
Thanks for your feedback so far.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Wed, May 24, 2023 at 10:31:44AM +0200, Sascha Hauer wrote:
> +static int rockchip_ddr_perf_event_add(struct perf_event *event, int flags)
> +{
> + struct rockchip_dfi *dfi = container_of(event->pmu, struct rockchip_dfi, pmu);
> +
> + dfi->active_events++;
> +
> + if (dfi->active_events == 1)
We need to initialize dfi->last_perf_count here:
rockchip_dfi_read_counters(dfi, &dfi->last_perf_count);
dfi->last_perf_count contains the start values for the counters and
without initializing this the u32 counter overflows are not correctly
handled.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Wed, Jun 14, 2023 at 03:40:34PM +0200, Sebastian Reichel wrote:
> Hi,
>
> On Wed, May 24, 2023 at 10:31:28AM +0200, Sascha Hauer wrote:
> > This is v5 of the series adding perf support to the rockchip DFI driver.
> >
> > A lot has changed in the perf driver since v4. First of all the review
> > feedback from Robin and Jonathan has been integrated. The perf driver
> > now not only supports monitoring the total DDR utilization, but also the
> > individual channels. I also reworked the way the raw 32bit counter
> > values are summed up to 64bit perf values, so hopefully the code is
> > easier to follow now.
> >
> > lockdep found out that that locking in the perf driver was broken, so I
> > reworked that as well. None of the perf hooks allows locking with
> > mutexes or spinlocks, so in perf it's not possible to enable the DFI
> > controller when needed. Instead I now unconditionally enable the DFI
> > controller during probe when perf is enabled.
> >
> > Furthermore the hrtimer I use for reading out the hardware counter
> > values before they overflow race with perf. Now a seqlock is used to
> > prevent that.
> >
> > The RK3588 device tree changes for the DFI were not part of v4. As
> > Vincent Legoll showed interest in testing this series the necessary
> > device tree changes are now part of this series.
>
> I tested the series on RK3588 EVB1. The read/write byts looks
> sensible. Sometimes cycles reads unrealistic values, though:
>
> Performance counter stats for 'system wide':
>
> 18446744070475110400 rockchip_ddr/cycles/
This goes down to missing initialization of &dfi->last_perf_count, see
my other mail. Will fix this in the next round.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |