[PATCH v4 00/21] Add perf support to the rockchip-dfi driver

[PATCH v4 00/21] Add perf support to the rockchip-dfi driver

[Sascha Hauer]

This is v4 of the patches adding DDR perf support to the rockchip-dfi
driver.

There was no reaction to v3 of this series. Heiko, as the devfreq maintainers
are unreactive, would it be an option to take this through your tree?

Compared to the last version I added RK3588 support this time which required
some minor refactoring of the driver code. Only the driver changes are
included, not yet the device tree changes.

To make a bit of advertisement for this series: It allows us to measure the
DDR utilization which can be quite nice to identify performance bottlenecks.

Usage can be something 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

The above was called with running a test program in the background which copies
memory around. The measurements actually match quite well to the amount of
memory really copied as output by the test program.

Sascha

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 (21):
  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: Make pmu regmap mandatory
  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: 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
  arm64: dts: rockchip: rk3399: Enable DFI
  arm64: dts: rockchip: rk356x: Add DFI
  dt-bindings: devfreq: event: convert Rockchip DFI binding to yaml

 .../bindings/devfreq/event/rockchip-dfi.txt   |  18 -
 .../bindings/devfreq/event/rockchip-dfi.yaml  |  39 +
 arch/arm64/boot/dts/rockchip/rk3399.dtsi      |   1 -
 arch/arm64/boot/dts/rockchip/rk356x.dtsi      |   6 +
 drivers/devfreq/event/rockchip-dfi.c          | 723 +++++++++++++++---
 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           |  16 +
 10 files changed, 718 insertions(+), 135 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/devfreq/event/rockchip-dfi.txt
 create mode 100644 Documentation/devicetree/bindings/devfreq/event/rockchip-dfi.yaml
 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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v4 01/21] PM / devfreq: rockchip-dfi: Embed desc into private data struct

[Sascha Hauer]

No need for an extra allocation, just embed the struct
devfreq_event_desc into the private data struct.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 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 39ac069cabc75..570f1b36c3153 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;
@@ -203,14 +203,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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 01/21] PM / devfreq: rockchip-dfi: Embed desc into private data struct

[Heiko Stübner]

Am Freitag, 5. Mai 2023, 13:38:36 CEST schrieb Sascha Hauer:
> No need for an extra allocation, just embed the struct
> devfreq_event_desc into the private data struct.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Reviewed-by: Heiko Stuebner <heiko@sntech.de>



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 01/21] PM / devfreq: rockchip-dfi: Embed desc into private data struct

[Jonathan Cameron]

On Fri,  5 May 2023 13:38:36 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> No need for an extra allocation, just embed the struct
> devfreq_event_desc into the private data struct.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Seems sensible
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  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 39ac069cabc75..570f1b36c3153 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;
> @@ -203,14 +203,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)) {


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v4 02/21] PM / devfreq: rockchip-dfi: use consistent name for private data struct

[Sascha Hauer]

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 <s.hauer@pengutronix.de>
---
 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 570f1b36c3153..98712ac68aa5f 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,46 +176,46 @@ 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");
 
 	/* 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);
+		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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 02/21] PM / devfreq: rockchip-dfi: use consistent name for private data struct

[Heiko Stübner]

Am Freitag, 5. Mai 2023, 13:38:37 CEST schrieb Sascha Hauer:
> 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 <s.hauer@pengutronix.de>

Reviewed-by: Heiko Stuebner <heiko@sntech.de>



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 02/21] PM / devfreq: rockchip-dfi: use consistent name for private data struct

[Jonathan Cameron]

On Fri,  5 May 2023 13:38:37 +0200
Sascha Hauer <s.hauer@pengutronix.de> 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 <s.hauer@pengutronix.de>
LGTM
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

I'm really here for the perf part, but meh, given I'm looking I'll
look at it all :)

Jonathan

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v4 03/21] PM / devfreq: rockchip-dfi: Make pmu regmap mandatory

[Sascha Hauer]

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.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/devfreq/event/rockchip-dfi.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 98712ac68aa5f..47cc9e48dafab 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -193,14 +193,14 @@ static int rockchip_dfi_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(dfi->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) {
-		dfi->regmap_pmu = syscon_node_to_regmap(node);
-		of_node_put(node);
-		if (IS_ERR(dfi->regmap_pmu))
-			return PTR_ERR(dfi->regmap_pmu);
-	}
+	if (!node)
+		return dev_err_probe(&pdev->dev, -ENODEV, "Can't find pmu_grf registers\n");
+
+	dfi->regmap_pmu = syscon_node_to_regmap(node);
+	if (IS_ERR(dfi->regmap_pmu))
+		return PTR_ERR(dfi->regmap_pmu);
+
 	dfi->dev = dev;
 
 	desc = &dfi->desc;
-- 
2.39.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 03/21] PM / devfreq: rockchip-dfi: Make pmu regmap mandatory

[Jonathan Cameron]

On Fri,  5 May 2023 13:38:38 +0200
Sascha Hauer <s.hauer@pengutronix.de> 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.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Agreed on it uses it unconditionally currently - this is
a fix, so maybe a fixes tag?


> ---
>  drivers/devfreq/event/rockchip-dfi.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 98712ac68aa5f..47cc9e48dafab 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -193,14 +193,14 @@ static int rockchip_dfi_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, PTR_ERR(dfi->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) {
> -		dfi->regmap_pmu = syscon_node_to_regmap(node);
> -		of_node_put(node);
> -		if (IS_ERR(dfi->regmap_pmu))
> -			return PTR_ERR(dfi->regmap_pmu);
> -	}
> +	if (!node)
> +		return dev_err_probe(&pdev->dev, -ENODEV, "Can't find pmu_grf registers\n");
> +
> +	dfi->regmap_pmu = syscon_node_to_regmap(node);

of_node_put(node);
needed, or an explanation of why not.

> +	if (IS_ERR(dfi->regmap_pmu))
> +		return PTR_ERR(dfi->regmap_pmu);
> +
>  	dfi->dev = dev;
>  
>  	desc = &dfi->desc;


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v4 04/21] PM / devfreq: rockchip-dfi: Add SoC specific init function

[Sascha Hauer]

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 <s.hauer@pengutronix.de>
---
 drivers/devfreq/event/rockchip-dfi.c | 59 +++++++++++++++++++++-------
 1 file changed, 44 insertions(+), 15 deletions(-)

diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 47cc9e48dafab..f317d3d063e9c 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,34 @@ 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;
+};
+
+struct rockchip_dfi_devtype_data {
+	int (*init)(struct rockchip_dfi *dfi);
+};
+
+static struct rockchip_dfi_devtype_data rk3399_devtype_data = {
+	.init = rk3399_dfi_init,
+};
+
 static const struct of_device_id rockchip_dfi_id_match[] = {
-	{ .compatible = "rockchip,rk3399-dfi" },
+	{ .compatible = "rockchip,rk3399-dfi", .data = &rk3399_devtype_data },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, rockchip_dfi_id_match);
@@ -179,6 +200,15 @@ 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;
+	const struct of_device_id *of_id;
+	const struct rockchip_dfi_devtype_data *devtype;
+	int ret;
+
+	of_id = of_match_device(rockchip_dfi_id_match, &pdev->dev);
+	if (!of_id)
+		return -ENODEV;
+
+	devtype = of_id->data;
 
 	dfi = devm_kzalloc(dev, sizeof(*dfi), GFP_KERNEL);
 	if (!dfi)
@@ -188,11 +218,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");
@@ -208,6 +233,10 @@ static int rockchip_dfi_probe(struct platform_device *pdev)
 	desc->driver_data = dfi;
 	desc->name = np->name;
 
+	ret = devtype->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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 04/21] PM / devfreq: rockchip-dfi: Add SoC specific init function

[Jonathan Cameron]

On Fri,  5 May 2023 13:38:39 +0200
Sascha Hauer <s.hauer@pengutronix.de> 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 <s.hauer@pengutronix.de>
Hi Sascha,

A few things inline,

Jonathan

> ---
>  drivers/devfreq/event/rockchip-dfi.c | 59 +++++++++++++++++++++-------
>  1 file changed, 44 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 47cc9e48dafab..f317d3d063e9c 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,34 @@ 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;

I don't suppose you fancy converting whole driver to
FIELD_GET / FIELD_PREP and masks that include the shift? :)
Guess probably not or maybe you do it later in this set.

> +
> +	return 0;
> +};
> +
> +struct rockchip_dfi_devtype_data {
> +	int (*init)(struct rockchip_dfi *dfi);
> +};
> +
> +static struct rockchip_dfi_devtype_data rk3399_devtype_data = {
> +	.init = rk3399_dfi_init,

I guess the 'why' will become apparent later in the set, but at this
stage a callback to get the ddr type would have made more sense and
kept the data local to where it was needed.

> +};
> +
>  static const struct of_device_id rockchip_dfi_id_match[] = {
> -	{ .compatible = "rockchip,rk3399-dfi" },
> +	{ .compatible = "rockchip,rk3399-dfi", .data = &rk3399_devtype_data },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, rockchip_dfi_id_match);
> @@ -179,6 +200,15 @@ 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;
> +	const struct of_device_id *of_id;
> +	const struct rockchip_dfi_devtype_data *devtype;
> +	int ret;
> +
> +	of_id = of_match_device(rockchip_dfi_id_match, &pdev->dev);
> +	if (!of_id)
> +		return -ENODEV;
> +
> +	devtype = of_id->data;

device_get_match_data()

or I guess if you really want stick to the of specific form.

of_device_get_match_data()


>  
>  	dfi = devm_kzalloc(dev, sizeof(*dfi), GFP_KERNEL);
>  	if (!dfi)
> @@ -188,11 +218,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");
> @@ -208,6 +233,10 @@ static int rockchip_dfi_probe(struct platform_device *pdev)
>  	desc->driver_data = dfi;
>  	desc->name = np->name;
>  
> +	ret = devtype->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,


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 04/21] PM / devfreq: rockchip-dfi: Add SoC specific init function

[Sascha Hauer]

On Tue, May 16, 2023 at 04:40:17PM +0100, Jonathan Cameron wrote:
> On Fri,  5 May 2023 13:38:39 +0200
> Sascha Hauer <s.hauer@pengutronix.de> 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 <s.hauer@pengutronix.de>
> Hi Sascha,
> 
> A few things inline,
> 
> Jonathan
> 
> > ---
> >  drivers/devfreq/event/rockchip-dfi.c | 59 +++++++++++++++++++++-------
> >  1 file changed, 44 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> > index 47cc9e48dafab..f317d3d063e9c 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,34 @@ 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;
> 
> I don't suppose you fancy converting whole driver to
> FIELD_GET / FIELD_PREP and masks that include the shift? :)
> Guess probably not or maybe you do it later in this set.

That's done later for the cases that make sense, but you already noticed
that ;)

> 
> > +
> > +	return 0;
> > +};
> > +
> > +struct rockchip_dfi_devtype_data {
> > +	int (*init)(struct rockchip_dfi *dfi);
> > +};
> > +
> > +static struct rockchip_dfi_devtype_data rk3399_devtype_data = {
> > +	.init = rk3399_dfi_init,
> 
> I guess the 'why' will become apparent later in the set, but at this
> stage a callback to get the ddr type would have made more sense and
> kept the data local to where it was needed.

struct rockchip_dfi_devtype_data will still have a single member by the
end of this series. The reason I still prefer a struct type is that I've
seen enough drivers that passed a single ad-hoc value in devtype data
that I had to convert to pass a struct type because I had to add another
value.

Well, I just realized that I have configured several values that I could
have put in that struct in the SoC specific init function instead, so
there's no real point in using a struct type.

> 
> > +};
> > +
> >  static const struct of_device_id rockchip_dfi_id_match[] = {
> > -	{ .compatible = "rockchip,rk3399-dfi" },
> > +	{ .compatible = "rockchip,rk3399-dfi", .data = &rk3399_devtype_data },
> >  	{ },
> >  };
> >  MODULE_DEVICE_TABLE(of, rockchip_dfi_id_match);
> > @@ -179,6 +200,15 @@ 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;
> > +	const struct of_device_id *of_id;
> > +	const struct rockchip_dfi_devtype_data *devtype;
> > +	int ret;
> > +
> > +	of_id = of_match_device(rockchip_dfi_id_match, &pdev->dev);
> > +	if (!of_id)
> > +		return -ENODEV;
> > +
> > +	devtype = of_id->data;
> 
> device_get_match_data()
> 
> or I guess if you really want stick to the of specific form.
> 
> of_device_get_match_data()

Yes, will change.

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 |

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 04/21] PM / devfreq: rockchip-dfi: Add SoC specific init function

[Jonathan Cameron]

> >   
> > > +
> > > +	return 0;
> > > +};
> > > +
> > > +struct rockchip_dfi_devtype_data {
> > > +	int (*init)(struct rockchip_dfi *dfi);
> > > +};
> > > +
> > > +static struct rockchip_dfi_devtype_data rk3399_devtype_data = {
> > > +	.init = rk3399_dfi_init,  
> > 
> > I guess the 'why' will become apparent later in the set, but at this
> > stage a callback to get the ddr type would have made more sense and
> > kept the data local to where it was needed.  
> 
> struct rockchip_dfi_devtype_data will still have a single member by the
> end of this series. The reason I still prefer a struct type is that I've
> seen enough drivers that passed a single ad-hoc value in devtype data
> that I had to convert to pass a struct type because I had to add another
> value.
> 
> Well, I just realized that I have configured several values that I could
> have put in that struct in the SoC specific init function instead, so
> there's no real point in using a struct type.

I'm fine with the above being a struct - it was more the question of why
do everything in one 'init' function rather than having more specific
callbacks, e.g. one to just read the DDR type where you need it.



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v4 05/21] PM / devfreq: rockchip-dfi: dfi store raw values in counter struct

[Sascha Hauer]

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 <s.hauer@pengutronix.de>
---
 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 f317d3d063e9c..383fe8a17a512 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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 05/21] PM / devfreq: rockchip-dfi: dfi store raw values in counter struct

[Jonathan Cameron]

On Fri,  5 May 2023 13:38:40 +0200
Sascha Hauer <s.hauer@pengutronix.de> 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 <s.hauer@pengutronix.de>
Given there are no docs for that structure anyway and access is vague
enough that it could be any scale, this looks fine to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  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 f317d3d063e9c..383fe8a17a512 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;


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v4 06/21] PM / devfreq: rockchip-dfi: Use free running counter

[Sascha Hauer]

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 a monotonicly increasing counter.

This patch removes resetting the counters and keeps them running
instead. Counter overflows are handled with modular arithmetics. 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 <s.hauer@pengutronix.de>
---
 drivers/devfreq/event/rockchip-dfi.c | 53 ++++++++++++++++------------
 1 file changed, 31 insertions(+), 22 deletions(-)

diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 383fe8a17a512..25d64d9166a9a 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,8 @@ 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 count;
+	struct dmc_count last_event_count;
 	struct device *dev;
 	void __iomem *regs;
 	struct regmap *regmap_pmu;
@@ -85,30 +90,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 +138,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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 06/21] PM / devfreq: rockchip-dfi: Use free running counter

[Jonathan Cameron]

On Fri,  5 May 2023 13:38:41 +0200
Sascha Hauer <s.hauer@pengutronix.de> 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 a monotonicly increasing counter.

In general, perf doesn't, but it does need to know no one else is modifying it,
so I guess this makes sense.

> 
> This patch removes resetting the counters and keeps them running
> instead. Counter overflows are handled with modular arithmetics. 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 <s.hauer@pengutronix.de>


> ---
>  drivers/devfreq/event/rockchip-dfi.c | 53 ++++++++++++++++------------
>  1 file changed, 31 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 383fe8a17a512..25d64d9166a9a 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,8 @@ 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 count;

Is count used?  Looking at the code I'd guess you changed to a local variable at
some point and forgot to drop this.

> +	struct dmc_count last_event_count;
>  	struct device *dev;
>  	void __iomem *regs;
>  	struct regmap *regmap_pmu;
> @@ -85,30 +90,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 +138,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;
>  }


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 06/21] PM / devfreq: rockchip-dfi: Use free running counter

[Sascha Hauer]

On Tue, May 16, 2023 at 04:48:43PM +0100, Jonathan Cameron wrote:
> On Fri,  5 May 2023 13:38:41 +0200
> Sascha Hauer <s.hauer@pengutronix.de> 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 a monotonicly increasing counter.
> 
> In general, perf doesn't, but it does need to know no one else is modifying it,
> so I guess this makes sense.

Makes sense. I'll rephrase that accordingly.

> > @@ -51,7 +55,8 @@ 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 count;
> 
> Is count used?  Looking at the code I'd guess you changed to a local variable at
> some point and forgot to drop this.

Yes, exactly. Dropped.

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 |

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v4 07/21] PM / devfreq: rockchip-dfi: introduce channel mask

[Sascha Hauer]

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 <s.hauer@pengutronix.de>
---
 drivers/devfreq/event/rockchip-dfi.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 25d64d9166a9a..18d578730fd0c 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -21,7 +21,7 @@
 
 #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 +44,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];
 };
 
 /*
@@ -62,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)
@@ -96,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 +
@@ -146,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;
@@ -186,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 = 3;
+
 	return 0;
 };
 
-- 
2.39.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 07/21] PM / devfreq: rockchip-dfi: introduce channel mask

[Jonathan Cameron]

On Fri,  5 May 2023 13:38:42 +0200
Sascha Hauer <s.hauer@pengutronix.de> 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.

If it's just numbers, why not a count rather than a mask?

> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/devfreq/event/rockchip-dfi.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 25d64d9166a9a..18d578730fd0c 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -21,7 +21,7 @@
>  
>  #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 +44,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];
>  };
>  
>  /*
> @@ -62,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)
> @@ -96,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 +
> @@ -146,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;
> @@ -186,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 = 3;

GENMASK(1, 0)

> +
>  	return 0;
>  };
>  


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 07/21] PM / devfreq: rockchip-dfi: introduce channel mask

[Sascha Hauer]

On Tue, May 16, 2023 at 04:50:09PM +0100, Jonathan Cameron wrote:
> On Fri,  5 May 2023 13:38:42 +0200
> Sascha Hauer <s.hauer@pengutronix.de> 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.
> 
> If it's just numbers, why not a count rather than a mask?

It's a mask in the downstream driver. I guess it's not necessarily the
first channels that are enabled when not all channels are enabled. It
could also be channels 0 and 2 that are enabled.

I don't have any example board for this case though, so I can only
guess.

> > -	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;
> > @@ -186,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 = 3;
> 
> GENMASK(1, 0)

OK.

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 |

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v4 08/21] PM / devfreq: rk3399_dmc,dfi: generalize DDRTYPE defines

[Sascha Hauer]

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 <s.hauer@pengutronix.de>
---
 drivers/devfreq/event/rockchip-dfi.c | 10 ++++++----
 drivers/devfreq/rk3399_dmc.c         | 10 +++++-----
 include/soc/rockchip/rk3399_grf.h    |  7 +------
 include/soc/rockchip/rockchip_grf.h  | 15 +++++++++++++++
 4 files changed, 27 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 18d578730fd0c..7896cd8beb143 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -18,7 +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 +77,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 +194,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 = 3;
 
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..dc77bb762a05a
--- /dev/null
+++ b/include/soc/rockchip/rockchip_grf.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Rockchip General Register Files definitions
+ */
+
+#ifndef __SOC_ROCKCHIP_GRF_H
+#define __SOC_ROCKCHIP_GRF_H
+
+/* Rockchip DDRTYPE defines */
+#define ROCKCHIP_DDRTYPE_DDR3	3
+#define ROCKCHIP_DDRTYPE_LPDDR2	5
+#define ROCKCHIP_DDRTYPE_LPDDR3	6
+#define ROCKCHIP_DDRTYPE_LPDDR4	7
+
+#endif /* __SOC_ROCKCHIP_GRF_H */
-- 
2.39.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 08/21] PM / devfreq: rk3399_dmc,dfi: generalize DDRTYPE defines

[Jonathan Cameron]

On Fri,  5 May 2023 13:38:43 +0200
Sascha Hauer <s.hauer@pengutronix.de> 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.

Great - you covered this one a few patches later.

A few suggestions inline but this looks basically fine to me.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/devfreq/event/rockchip-dfi.c | 10 ++++++----
>  drivers/devfreq/rk3399_dmc.c         | 10 +++++-----
>  include/soc/rockchip/rk3399_grf.h    |  7 +------
>  include/soc/rockchip/rockchip_grf.h  | 15 +++++++++++++++
>  4 files changed, 27 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 18d578730fd0c..7896cd8beb143 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -18,7 +18,10 @@
>  #include <linux/list.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>

Why bits.h?

>  
> +#include <soc/rockchip/rockchip_grf.h>
>  #include <soc/rockchip/rk3399_grf.h>
>  
>  #define DMC_MAX_CHANNELS	2
> @@ -74,9 +77,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);

Maybe a switch statement here as well?  In particular I'm interested
that there is no sign of DDR3 or LPDDR2 here and I think it would be good to
make that explicit given it's defined.

>  
>  	/* enable count, use software mode */
> @@ -191,8 +194,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 = 3;
>  
> 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);

Excellent!  You did it a few patches later :)

>  
>  	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..dc77bb762a05a
> --- /dev/null
> +++ b/include/soc/rockchip/rockchip_grf.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Rockchip General Register Files definitions
> + */
> +
> +#ifndef __SOC_ROCKCHIP_GRF_H
> +#define __SOC_ROCKCHIP_GRF_H
> +
> +/* Rockchip DDRTYPE defines */
> +#define ROCKCHIP_DDRTYPE_DDR3	3
> +#define ROCKCHIP_DDRTYPE_LPDDR2	5
> +#define ROCKCHIP_DDRTYPE_LPDDR3	6
> +#define ROCKCHIP_DDRTYPE_LPDDR4	7

Maybe worth an enum so you can give ddr_type a named type and
the compiler can see if you've handled all the cases for
the switch statements?

> +
> +#endif /* __SOC_ROCKCHIP_GRF_H */


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 08/21] PM / devfreq: rk3399_dmc,dfi: generalize DDRTYPE defines

[Sascha Hauer]

On Tue, May 16, 2023 at 04:54:55PM +0100, Jonathan Cameron wrote:
> On Fri,  5 May 2023 13:38:43 +0200
> Sascha Hauer <s.hauer@pengutronix.de> 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.
> 
> Great - you covered this one a few patches later.
> 
> A few suggestions inline but this looks basically fine to me.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/devfreq/event/rockchip-dfi.c | 10 ++++++----
> >  drivers/devfreq/rk3399_dmc.c         | 10 +++++-----
> >  include/soc/rockchip/rk3399_grf.h    |  7 +------
> >  include/soc/rockchip/rockchip_grf.h  | 15 +++++++++++++++
> >  4 files changed, 27 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 18d578730fd0c..7896cd8beb143 100644
> > --- a/drivers/devfreq/event/rockchip-dfi.c
> > +++ b/drivers/devfreq/event/rockchip-dfi.c
> > @@ -18,7 +18,10 @@
> >  #include <linux/list.h>
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> 
> Why bits.h?

For GENMASK.

It's included indirectly anyway, but being explicit shouldn't hurt.

> 
> >  
> > +#include <soc/rockchip/rockchip_grf.h>
> >  #include <soc/rockchip/rk3399_grf.h>
> >  
> >  #define DMC_MAX_CHANNELS	2
> > @@ -74,9 +77,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);
> 
> Maybe a switch statement here as well?  In particular I'm interested
> that there is no sign of DDR3 or LPDDR2 here and I think it would be good to
> make that explicit given it's defined.

That's done later in this series, but you already noticed that.

> > diff --git a/include/soc/rockchip/rockchip_grf.h b/include/soc/rockchip/rockchip_grf.h
> > new file mode 100644
> > index 0000000000000..dc77bb762a05a
> > --- /dev/null
> > +++ b/include/soc/rockchip/rockchip_grf.h
> > @@ -0,0 +1,15 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Rockchip General Register Files definitions
> > + */
> > +
> > +#ifndef __SOC_ROCKCHIP_GRF_H
> > +#define __SOC_ROCKCHIP_GRF_H
> > +
> > +/* Rockchip DDRTYPE defines */
> > +#define ROCKCHIP_DDRTYPE_DDR3	3
> > +#define ROCKCHIP_DDRTYPE_LPDDR2	5
> > +#define ROCKCHIP_DDRTYPE_LPDDR3	6
> > +#define ROCKCHIP_DDRTYPE_LPDDR4	7
> 
> Maybe worth an enum so you can give ddr_type a named type and
> the compiler can see if you've handled all the cases for
> the switch statements?

Ok, will do.

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 |

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v4 09/21] PM / devfreq: rockchip-dfi: Clean up DDR type register defines

[Sascha Hauer]

Use the HIWORD_UPDATE() define known from other rockchip drivers to
make the defines look less odd to the readers who've seen other
rockchip drivers.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/devfreq/event/rockchip-dfi.c | 32 +++++++++++++++++-----------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 7896cd8beb143..035984d3c7b01 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -26,15 +26,19 @@
 
 #define DMC_MAX_CHANNELS	2
 
+#define HIWORD_UPDATE(val, mask)	((val) | (mask) << 16)
+
 /* DDRMON_CTRL */
 #define DDRMON_CTRL	0x04
-#define CLR_DDRMON_CTRL	(0x1f0000 << 0)
-#define LPDDR4_EN	(0x10001 << 4)
-#define HARDWARE_EN	(0x10001 << 3)
-#define LPDDR3_EN	(0x10001 << 2)
-#define SOFTWARE_EN	(0x10001 << 1)
-#define SOFTWARE_DIS	(0x10000 << 1)
-#define TIME_CNT_EN	(0x10001 << 0)
+#define DDRMON_CTRL_DDR4		BIT(5)
+#define DDRMON_CTRL_LPDDR4		BIT(4)
+#define DDRMON_CTRL_HARDWARE_EN		BIT(3)
+#define DDRMON_CTRL_LPDDR23		BIT(2)
+#define DDRMON_CTRL_SOFTWARE_EN		BIT(1)
+#define DDRMON_CTRL_TIMER_CNT_EN	BIT(0)
+#define DDRMON_CTRL_DDR_TYPE_MASK	(DDRMON_CTRL_DDR4 | \
+					 DDRMON_CTRL_LPDDR4 | \
+					 DDRMON_CTRL_LPDDR23)
 
 #define DDRMON_CH0_COUNT_NUM		0x28
 #define DDRMON_CH0_DFI_ACCESS_NUM	0x2c
@@ -74,16 +78,19 @@ static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
 	void __iomem *dfi_regs = dfi->regs;
 
 	/* clear DDRMON_CTRL setting */
-	writel_relaxed(CLR_DDRMON_CTRL, dfi_regs + DDRMON_CTRL);
+	writel_relaxed(HIWORD_UPDATE(0, 0xffff), dfi_regs + DDRMON_CTRL);
 
 	/* set ddr type to dfi */
 	if (dfi->ddr_type == ROCKCHIP_DDRTYPE_LPDDR3)
-		writel_relaxed(LPDDR3_EN, dfi_regs + DDRMON_CTRL);
+		writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_LPDDR23, DDRMON_CTRL_DDR_TYPE_MASK),
+			       dfi_regs + DDRMON_CTRL);
 	else if (dfi->ddr_type == ROCKCHIP_DDRTYPE_LPDDR4)
-		writel_relaxed(LPDDR4_EN, dfi_regs + DDRMON_CTRL);
+		writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_LPDDR4, DDRMON_CTRL_DDR_TYPE_MASK),
+			       dfi_regs + DDRMON_CTRL);
 
 	/* enable count, use software mode */
-	writel_relaxed(SOFTWARE_EN, dfi_regs + DDRMON_CTRL);
+	writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_SOFTWARE_EN, DDRMON_CTRL_SOFTWARE_EN),
+		       dfi_regs + DDRMON_CTRL);
 }
 
 static void rockchip_dfi_stop_hardware_counter(struct devfreq_event_dev *edev)
@@ -91,7 +98,8 @@ static void rockchip_dfi_stop_hardware_counter(struct devfreq_event_dev *edev)
 	struct rockchip_dfi *dfi = devfreq_event_get_drvdata(edev);
 	void __iomem *dfi_regs = dfi->regs;
 
-	writel_relaxed(SOFTWARE_DIS, dfi_regs + DDRMON_CTRL);
+	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)
-- 
2.39.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 09/21] PM / devfreq: rockchip-dfi: Clean up DDR type register defines

[Jonathan Cameron]

On Fri,  5 May 2023 13:38:44 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> Use the HIWORD_UPDATE() define known from other rockchip drivers to
> make the defines look less odd to the readers who've seen other
> rockchip drivers.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Whilst this might be fine, it's not a noop change.  So more
text needed to explain why it's fine to write the same 'mask' always
when previously only single bits were set in the mask.

> ---
>  drivers/devfreq/event/rockchip-dfi.c | 32 +++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 7896cd8beb143..035984d3c7b01 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -26,15 +26,19 @@
>  
>  #define DMC_MAX_CHANNELS	2
>  
> +#define HIWORD_UPDATE(val, mask)	((val) | (mask) << 16)
> +
>  /* DDRMON_CTRL */
>  #define DDRMON_CTRL	0x04
> -#define CLR_DDRMON_CTRL	(0x1f0000 << 0)
> -#define LPDDR4_EN	(0x10001 << 4)
> -#define HARDWARE_EN	(0x10001 << 3)
> -#define LPDDR3_EN	(0x10001 << 2)
> -#define SOFTWARE_EN	(0x10001 << 1)
> -#define SOFTWARE_DIS	(0x10000 << 1)
> -#define TIME_CNT_EN	(0x10001 << 0)
> +#define DDRMON_CTRL_DDR4		BIT(5)
> +#define DDRMON_CTRL_LPDDR4		BIT(4)
> +#define DDRMON_CTRL_HARDWARE_EN		BIT(3)
> +#define DDRMON_CTRL_LPDDR23		BIT(2)
> +#define DDRMON_CTRL_SOFTWARE_EN		BIT(1)
> +#define DDRMON_CTRL_TIMER_CNT_EN	BIT(0)
> +#define DDRMON_CTRL_DDR_TYPE_MASK	(DDRMON_CTRL_DDR4 | \
> +					 DDRMON_CTRL_LPDDR4 | \
> +					 DDRMON_CTRL_LPDDR23)
>  
>  #define DDRMON_CH0_COUNT_NUM		0x28
>  #define DDRMON_CH0_DFI_ACCESS_NUM	0x2c
> @@ -74,16 +78,19 @@ static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
>  	void __iomem *dfi_regs = dfi->regs;
>  
>  	/* clear DDRMON_CTRL setting */
> -	writel_relaxed(CLR_DDRMON_CTRL, dfi_regs + DDRMON_CTRL);
> +	writel_relaxed(HIWORD_UPDATE(0, 0xffff), dfi_regs + DDRMON_CTRL);
>  
>  	/* set ddr type to dfi */
>  	if (dfi->ddr_type == ROCKCHIP_DDRTYPE_LPDDR3)
> -		writel_relaxed(LPDDR3_EN, dfi_regs + DDRMON_CTRL);
> +		writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_LPDDR23, DDRMON_CTRL_DDR_TYPE_MASK),
> +			       dfi_regs + DDRMON_CTRL);
>  	else if (dfi->ddr_type == ROCKCHIP_DDRTYPE_LPDDR4)
> -		writel_relaxed(LPDDR4_EN, dfi_regs + DDRMON_CTRL);
Old value written is 0x10001 << 4 == 0x100010
> +		writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_LPDDR4, DDRMON_CTRL_DDR_TYPE_MASK),
> +			       dfi_regs + DDRMON_CTRL);
New value is (BIT(5) | BIT(4) | BIT(2)) | (BIT(4) << 16) 
0x100034

>  


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 09/21] PM / devfreq: rockchip-dfi: Clean up DDR type register defines

[Sascha Hauer]

On Tue, May 16, 2023 at 05:01:46PM +0100, Jonathan Cameron wrote:
> On Fri,  5 May 2023 13:38:44 +0200
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > Use the HIWORD_UPDATE() define known from other rockchip drivers to
> > make the defines look less odd to the readers who've seen other
> > rockchip drivers.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Whilst this might be fine, it's not a noop change.  So more
> text needed to explain why it's fine to write the same 'mask' always
> when previously only single bits were set in the mask.
> 
> > ---
> >  drivers/devfreq/event/rockchip-dfi.c | 32 +++++++++++++++++-----------
> >  1 file changed, 20 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> > index 7896cd8beb143..035984d3c7b01 100644
> > --- a/drivers/devfreq/event/rockchip-dfi.c
> > +++ b/drivers/devfreq/event/rockchip-dfi.c
> > @@ -26,15 +26,19 @@
> >  
> >  #define DMC_MAX_CHANNELS	2
> >  
> > +#define HIWORD_UPDATE(val, mask)	((val) | (mask) << 16)
> > +
> >  /* DDRMON_CTRL */
> >  #define DDRMON_CTRL	0x04
> > -#define CLR_DDRMON_CTRL	(0x1f0000 << 0)
> > -#define LPDDR4_EN	(0x10001 << 4)
> > -#define HARDWARE_EN	(0x10001 << 3)
> > -#define LPDDR3_EN	(0x10001 << 2)
> > -#define SOFTWARE_EN	(0x10001 << 1)
> > -#define SOFTWARE_DIS	(0x10000 << 1)
> > -#define TIME_CNT_EN	(0x10001 << 0)
> > +#define DDRMON_CTRL_DDR4		BIT(5)
> > +#define DDRMON_CTRL_LPDDR4		BIT(4)
> > +#define DDRMON_CTRL_HARDWARE_EN		BIT(3)
> > +#define DDRMON_CTRL_LPDDR23		BIT(2)
> > +#define DDRMON_CTRL_SOFTWARE_EN		BIT(1)
> > +#define DDRMON_CTRL_TIMER_CNT_EN	BIT(0)
> > +#define DDRMON_CTRL_DDR_TYPE_MASK	(DDRMON_CTRL_DDR4 | \
> > +					 DDRMON_CTRL_LPDDR4 | \
> > +					 DDRMON_CTRL_LPDDR23)
> >  
> >  #define DDRMON_CH0_COUNT_NUM		0x28
> >  #define DDRMON_CH0_DFI_ACCESS_NUM	0x2c
> > @@ -74,16 +78,19 @@ static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
> >  	void __iomem *dfi_regs = dfi->regs;
> >  
> >  	/* clear DDRMON_CTRL setting */
> > -	writel_relaxed(CLR_DDRMON_CTRL, dfi_regs + DDRMON_CTRL);
> > +	writel_relaxed(HIWORD_UPDATE(0, 0xffff), dfi_regs + DDRMON_CTRL);
> >  
> >  	/* set ddr type to dfi */
> >  	if (dfi->ddr_type == ROCKCHIP_DDRTYPE_LPDDR3)
> > -		writel_relaxed(LPDDR3_EN, dfi_regs + DDRMON_CTRL);
> > +		writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_LPDDR23, DDRMON_CTRL_DDR_TYPE_MASK),
> > +			       dfi_regs + DDRMON_CTRL);
> >  	else if (dfi->ddr_type == ROCKCHIP_DDRTYPE_LPDDR4)
> > -		writel_relaxed(LPDDR4_EN, dfi_regs + DDRMON_CTRL);
> Old value written is 0x10001 << 4 == 0x100010
> > +		writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_LPDDR4, DDRMON_CTRL_DDR_TYPE_MASK),
> > +			       dfi_regs + DDRMON_CTRL);
> New value is (BIT(5) | BIT(4) | BIT(2)) | (BIT(4) << 16) 
> 0x100034

Actually it's (BIT(5) | BIT(4) | BIT(2)) << 16 | BIT(4) = 0x340010

The hiword registers contain a mask in the upper 16 bits and functional
bits in the lower 16 bits. When writing to them only the functional bits that have the
corresponding mask bits set are modified, the others stay untouched.

Previously we had:

	writel_relaxed(CLR_DDRMON_CTRL, dfi_regs + DDRMON_CTRL);
	writel_relaxed(LPDDR4_EN, dfi_regs + DDRMON_CTRL);

The first access clears the lower 5 bits and the second sets BIT(4)

This now becomes:

	writel_relaxed(HIWORD_UPDATE(0, 0xffff), dfi_regs + DDRMON_CTRL);
	writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_LPDDR4, DDRMON_CTRL_DDR_TYPE_MASK), dfi_regs + DDRMON_CTRL);

The first access clears the lower 16 bits and the second sets BIT(4)

That's both identical except that my version clears the lower 16 bits
instead of lower 5 bits.

I'll see if I can make that a bit clearer in the commit message.

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 |

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v4 10/21] PM / devfreq: rockchip-dfi: Add RK3568 support

[Sascha Hauer]

This adds RK3568 support to the DFI driver. The driver itself doesn't
need a change, only initialization differs from the currently supported
RK3399.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/devfreq/event/rockchip-dfi.c | 24 ++++++++++++++++++++++++
 include/soc/rockchip/rk3568_grf.h    | 12 ++++++++++++
 2 files changed, 36 insertions(+)
 create mode 100644 include/soc/rockchip/rk3568_grf.h

diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 035984d3c7b01..78cb594bd2a81 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -23,6 +23,7 @@
 
 #include <soc/rockchip/rockchip_grf.h>
 #include <soc/rockchip/rk3399_grf.h>
+#include <soc/rockchip/rk3568_grf.h>
 
 #define DMC_MAX_CHANNELS	2
 
@@ -209,6 +210,24 @@ static int rk3399_dfi_init(struct rockchip_dfi *dfi)
 	return 0;
 };
 
+static int rk3568_dfi_init(struct rockchip_dfi *dfi)
+{
+	struct regmap *regmap_pmu = dfi->regmap_pmu;
+	u32 reg2, reg3;
+
+	regmap_read(regmap_pmu, RK3568_PMUGRF_OS_REG2, &reg2);
+	regmap_read(regmap_pmu, RK3568_PMUGRF_OS_REG3, &reg3);
+
+	dfi->ddr_type = FIELD_GET(RK3568_PMUGRF_OS_REG2_DRAMTYPE_INFO, reg2);
+
+	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->channel_mask = 1;
+
+	return 0;
+};
+
 struct rockchip_dfi_devtype_data {
 	int (*init)(struct rockchip_dfi *dfi);
 };
@@ -217,8 +236,13 @@ static struct rockchip_dfi_devtype_data rk3399_devtype_data = {
 	.init = rk3399_dfi_init,
 };
 
+static struct rockchip_dfi_devtype_data rk3568_devtype_data = {
+	.init = rk3568_dfi_init,
+};
+
 static const struct of_device_id rockchip_dfi_id_match[] = {
 	{ .compatible = "rockchip,rk3399-dfi", .data = &rk3399_devtype_data },
+	{ .compatible = "rockchip,rk3568-dfi", .data = &rk3568_devtype_data },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, rockchip_dfi_id_match);
diff --git a/include/soc/rockchip/rk3568_grf.h b/include/soc/rockchip/rk3568_grf.h
new file mode 100644
index 0000000000000..575584e9d8834
--- /dev/null
+++ b/include/soc/rockchip/rk3568_grf.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+#ifndef __SOC_RK3568_GRF_H
+#define __SOC_RK3568_GRF_H
+
+#define RK3568_PMUGRF_OS_REG2		0x208
+#define RK3568_PMUGRF_OS_REG2_DRAMTYPE_INFO		GENMASK(15, 13)
+
+#define RK3568_PMUGRF_OS_REG3		0x20c
+#define RK3568_PMUGRF_OS_REG3_DRAMTYPE_INFO_V3		GENMASK(13, 12)
+#define RK3568_PMUGRF_OS_REG3_SYSREG_VERSION		GENMASK(31, 28)
+
+#endif /* __SOC_RK3568_GRF_H */
-- 
2.39.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 10/21] PM / devfreq: rockchip-dfi: Add RK3568 support

[Jonathan Cameron]

On Fri,  5 May 2023 13:38:45 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> This adds RK3568 support to the DFI driver. The driver itself doesn't
> need a change, only initialization differs from the currently supported
> RK3399.
I'm not sure on distinction between driver and init (which is part of driver)
so that description might want to just say

Only initialization differs from currently supported RK3399.

and leave it at that!

> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/devfreq/event/rockchip-dfi.c | 24 ++++++++++++++++++++++++
>  include/soc/rockchip/rk3568_grf.h    | 12 ++++++++++++
>  2 files changed, 36 insertions(+)
>  create mode 100644 include/soc/rockchip/rk3568_grf.h
> 
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 035984d3c7b01..78cb594bd2a81 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -23,6 +23,7 @@
>  
>  #include <soc/rockchip/rockchip_grf.h>
>  #include <soc/rockchip/rk3399_grf.h>
> +#include <soc/rockchip/rk3568_grf.h>
>  
>  #define DMC_MAX_CHANNELS	2
>  
> @@ -209,6 +210,24 @@ static int rk3399_dfi_init(struct rockchip_dfi *dfi)
>  	return 0;
>  };
>  
> +static int rk3568_dfi_init(struct rockchip_dfi *dfi)
> +{
> +	struct regmap *regmap_pmu = dfi->regmap_pmu;
> +	u32 reg2, reg3;
> +
> +	regmap_read(regmap_pmu, RK3568_PMUGRF_OS_REG2, &reg2);
> +	regmap_read(regmap_pmu, RK3568_PMUGRF_OS_REG3, &reg3);
> +
> +	dfi->ddr_type = FIELD_GET(RK3568_PMUGRF_OS_REG2_DRAMTYPE_INFO, reg2);
> +
> +	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;

This is unusual enough that I'd suggest some comments to say how
the bits are distributed across the two registers.

> +
> +	dfi->channel_mask = 1;
GENMASK(0, 0);
Or just use a count as that still looks fine to me.  Maybe that will change!


> +
> +	return 0;
> +};
> +
>  struct rockchip_dfi_devtype_data {
>  	int (*init)(struct rockchip_dfi *dfi);
>  };
> @@ -217,8 +236,13 @@ static struct rockchip_dfi_devtype_data rk3399_devtype_data = {
>  	.init = rk3399_dfi_init,
>  };
>  
> +static struct rockchip_dfi_devtype_data rk3568_devtype_data = {
> +	.init = rk3568_dfi_init,
> +};
> +
>  static const struct of_device_id rockchip_dfi_id_match[] = {
>  	{ .compatible = "rockchip,rk3399-dfi", .data = &rk3399_devtype_data },
> +	{ .compatible = "rockchip,rk3568-dfi", .data = &rk3568_devtype_data },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, rockchip_dfi_id_match);
> diff --git a/include/soc/rockchip/rk3568_grf.h b/include/soc/rockchip/rk3568_grf.h
> new file mode 100644
> index 0000000000000..575584e9d8834
> --- /dev/null
> +++ b/include/soc/rockchip/rk3568_grf.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +#ifndef __SOC_RK3568_GRF_H
> +#define __SOC_RK3568_GRF_H
> +
> +#define RK3568_PMUGRF_OS_REG2		0x208
> +#define RK3568_PMUGRF_OS_REG2_DRAMTYPE_INFO		GENMASK(15, 13)
> +
> +#define RK3568_PMUGRF_OS_REG3		0x20c
> +#define RK3568_PMUGRF_OS_REG3_DRAMTYPE_INFO_V3		GENMASK(13, 12)
> +#define RK3568_PMUGRF_OS_REG3_SYSREG_VERSION		GENMASK(31, 28)
> +
> +#endif /* __SOC_RK3568_GRF_H */


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 10/21] PM / devfreq: rockchip-dfi: Add RK3568 support

[Sascha Hauer]

On Tue, May 16, 2023 at 05:04:35PM +0100, Jonathan Cameron wrote:
> On Fri,  5 May 2023 13:38:45 +0200
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > This adds RK3568 support to the DFI driver. The driver itself doesn't
> > need a change, only initialization differs from the currently supported
> > RK3399.
> I'm not sure on distinction between driver and init (which is part of driver)
> so that description might want to just say
> 
> Only initialization differs from currently supported RK3399.
> 
> and leave it at that!
> 
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/devfreq/event/rockchip-dfi.c | 24 ++++++++++++++++++++++++
> >  include/soc/rockchip/rk3568_grf.h    | 12 ++++++++++++
> >  2 files changed, 36 insertions(+)
> >  create mode 100644 include/soc/rockchip/rk3568_grf.h
> > 
> > diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> > index 035984d3c7b01..78cb594bd2a81 100644
> > --- a/drivers/devfreq/event/rockchip-dfi.c
> > +++ b/drivers/devfreq/event/rockchip-dfi.c
> > @@ -23,6 +23,7 @@
> >  
> >  #include <soc/rockchip/rockchip_grf.h>
> >  #include <soc/rockchip/rk3399_grf.h>
> > +#include <soc/rockchip/rk3568_grf.h>
> >  
> >  #define DMC_MAX_CHANNELS	2
> >  
> > @@ -209,6 +210,24 @@ static int rk3399_dfi_init(struct rockchip_dfi *dfi)
> >  	return 0;
> >  };
> >  
> > +static int rk3568_dfi_init(struct rockchip_dfi *dfi)
> > +{
> > +	struct regmap *regmap_pmu = dfi->regmap_pmu;
> > +	u32 reg2, reg3;
> > +
> > +	regmap_read(regmap_pmu, RK3568_PMUGRF_OS_REG2, &reg2);
> > +	regmap_read(regmap_pmu, RK3568_PMUGRF_OS_REG3, &reg3);
> > +
> > +	dfi->ddr_type = FIELD_GET(RK3568_PMUGRF_OS_REG2_DRAMTYPE_INFO, reg2);
> > +
> > +	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;
> 
> This is unusual enough that I'd suggest some comments to say how
> the bits are distributed across the two registers.

I'd say in version 3 of the register structure they realized that they
need two bits more for the ddr_type and have put them in BIT(12) and
BIT(13) of RK3568_PMUGRF_OS_REG3. I think the code makes that
sufficiently clear and apart from this code taken from the downstream
kernel I don't have any documentation of these registers.

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 |

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 10/21] PM / devfreq: rockchip-dfi: Add RK3568 support

[Jonathan Cameron]

> > > +
> > > +	regmap_read(regmap_pmu, RK3568_PMUGRF_OS_REG2, &reg2);
> > > +	regmap_read(regmap_pmu, RK3568_PMUGRF_OS_REG3, &reg3);
> > > +
> > > +	dfi->ddr_type = FIELD_GET(RK3568_PMUGRF_OS_REG2_DRAMTYPE_INFO, reg2);
> > > +
> > > +	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;  
> > 
> > This is unusual enough that I'd suggest some comments to say how
> > the bits are distributed across the two registers.  
> 
> I'd say in version 3 of the register structure they realized that they
> need two bits more for the ddr_type and have put them in BIT(12) and
> BIT(13) of RK3568_PMUGRF_OS_REG3. I think the code makes that
> sufficiently clear and apart from this code taken from the downstream
> kernel I don't have any documentation of these registers.
> 
> Sascha
OK, that's fine.

Jonathan

> 


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v4 11/21] PM / devfreq: rockchip-dfi: Handle LPDDR2 correctly

[Sascha Hauer]

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 <s.hauer@pengutronix.de>
---
 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 78cb594bd2a81..92ee61c96a1a9 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)
 	writel_relaxed(HIWORD_UPDATE(0, 0xffff), 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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 11/21] PM / devfreq: rockchip-dfi: Handle LPDDR2 correctly

[Jonathan Cameron]

On Fri,  5 May 2023 13:38:46 +0200
Sascha Hauer <s.hauer@pengutronix.de> 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 <s.hauer@pengutronix.de>

Ah that resolved an earlier comment.  Also in my mind it strengthens
advantage of using an enum for ddr_type as then with some minor changes
(drop the default or make it specific to which cases it covers) the
compiler can assist you in making sure there are no unhandled values.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Jonathan

> ---
>  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 78cb594bd2a81..92ee61c96a1a9 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)
>  	writel_relaxed(HIWORD_UPDATE(0, 0xffff), 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),


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v4 12/21] PM / devfreq: rockchip-dfi: Handle LPDDR4X

[Sascha Hauer]

In the DFI driver LPDDR4X can be handled in the same way as LPDDR4. Add
the missing case.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 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 92ee61c96a1a9..dc48d9c26f599 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 dc77bb762a05a..7150a3362b142 100644
--- a/include/soc/rockchip/rockchip_grf.h
+++ b/include/soc/rockchip/rockchip_grf.h
@@ -11,5 +11,6 @@
 #define ROCKCHIP_DDRTYPE_LPDDR2	5
 #define ROCKCHIP_DDRTYPE_LPDDR3	6
 #define ROCKCHIP_DDRTYPE_LPDDR4	7
+#define ROCKCHIP_DDRTYPE_LPDDR4X	8
 
 #endif /* __SOC_ROCKCHIP_GRF_H */
-- 
2.39.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 12/21] PM / devfreq: rockchip-dfi: Handle LPDDR4X

[Jonathan Cameron]

On Fri,  5 May 2023 13:38:47 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> In the DFI driver LPDDR4X can be handled in the same way as LPDDR4. Add
> the missing case.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
No equivalent for the DMC driver you moved to he defines earlier?
I'm guessing not in which case this seems fine to me

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  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 92ee61c96a1a9..dc48d9c26f599 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 dc77bb762a05a..7150a3362b142 100644
> --- a/include/soc/rockchip/rockchip_grf.h
> +++ b/include/soc/rockchip/rockchip_grf.h
> @@ -11,5 +11,6 @@
>  #define ROCKCHIP_DDRTYPE_LPDDR2	5
>  #define ROCKCHIP_DDRTYPE_LPDDR3	6
>  #define ROCKCHIP_DDRTYPE_LPDDR4	7
> +#define ROCKCHIP_DDRTYPE_LPDDR4X	8
>  
>  #endif /* __SOC_ROCKCHIP_GRF_H */


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 12/21] PM / devfreq: rockchip-dfi: Handle LPDDR4X

[Sascha Hauer]

On Tue, May 16, 2023 at 05:09:19PM +0100, Jonathan Cameron wrote:
> On Fri,  5 May 2023 13:38:47 +0200
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > In the DFI driver LPDDR4X can be handled in the same way as LPDDR4. Add
> > the missing case.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> No equivalent for the DMC driver you moved to he defines earlier?

The DMC driver needs its own set of changes. Like the DFI driver it's
currently RK3399 only. I'll leave that for someone who has active
interest in this driver.

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 |

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v4 13/21] PM / devfreq: rockchip-dfi: Pass private data struct to internal functions

[Sascha Hauer]

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 <s.hauer@pengutronix.de>
---
 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 dc48d9c26f599..c0b7b1e9805e9 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -73,9 +73,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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 13/21] PM / devfreq: rockchip-dfi: Pass private data struct to internal functions

[Jonathan Cameron]

On Fri,  5 May 2023 13:38:48 +0200
Sascha Hauer <s.hauer@pengutronix.de> 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 <s.hauer@pengutronix.de>

An improvement even without it being needed for the new code.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  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 dc48d9c26f599..c0b7b1e9805e9 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -73,9 +73,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++) {


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v4 14/21] PM / devfreq: rockchip-dfi: Prepare for multiple users

[Sascha Hauer]

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 <s.hauer@pengutronix.de>
---
 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 c0b7b1e9805e9..eae010644935a 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -69,13 +69,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, 0xffff), dfi_regs + DDRMON_CTRL);
@@ -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,
 };
@@ -285,6 +307,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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 14/21] PM / devfreq: rockchip-dfi: Prepare for multiple users

[Jonathan Cameron]

On Fri,  5 May 2023 13:38:49 +0200
Sascha Hauer <s.hauer@pengutronix.de> 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 <s.hauer@pengutronix.de>

The clock code is IIRC refcounted anyway and I think has appropriate
locks, so you code just enable / disable that in both paths.
Probably not that helpful though given you still need to refcount
to protect the register writes.

So this looks fine to me.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


> ---
>  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 c0b7b1e9805e9..eae010644935a 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -69,13 +69,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, 0xffff), dfi_regs + DDRMON_CTRL);
> @@ -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);

Similar to below. Up to you on whether you prefer shorter code vs
having the type of the parameter explicit in here.

>  
>  	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);

No real point in local variable any more, so maybe

	return rockchip_dfi_enable(devfreq_event_get_drvdata(edev));

>  }
>  
>  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,
>  };
> @@ -285,6 +307,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;


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v4 15/21] PM / devfreq: rockchip-dfi: Add perf support

[Sascha Hauer]

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 <s.hauer@pengutronix.de>
---
 drivers/devfreq/event/rockchip-dfi.c | 349 +++++++++++++++++++++++++++
 include/soc/rockchip/rk3399_grf.h    |   2 +
 include/soc/rockchip/rk3568_grf.h    |   1 +
 3 files changed, 352 insertions(+)

diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index eae010644935a..400b1b360e3c9 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -20,6 +20,7 @@
 #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,14 +42,30 @@
 					 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
 
+enum access_type {
+	PERF_EVENT_CYCLES,
+	PERF_EVENT_READ_BYTES,
+	PERF_EVENT_WRITE_BYTES,
+	PERF_EVENT_BYTES,
+	PERF_ACCESS_TYPE_MAX,
+};
+
 struct dmc_count_channel {
 	u32 access;
 	u32 total;
+	u32 read_access;
+	u32 write_access;
+};
+
+struct dmc_count_channel64 {
+	u64 count[PERF_ACCESS_TYPE_MAX];
 };
 
 struct dmc_count {
@@ -65,6 +82,7 @@ struct rockchip_dfi {
 	struct devfreq_event_desc desc;
 	struct dmc_count count;
 	struct dmc_count last_event_count;
+	struct dmc_count last;
 	struct device *dev;
 	void __iomem *regs;
 	struct regmap *regmap_pmu;
@@ -73,6 +91,15 @@ 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;
+	struct dmc_count_channel64 frr;
+	int active_events;
+	int burst_len;
+	int buswidth[DMC_MAX_CHANNELS];
 };
 
 static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
@@ -148,6 +175,10 @@ 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].read_access = readl_relaxed(dfi_regs +
+				DDRMON_CH0_RD_NUM + i * 20);
+		count->c[i].write_access = readl_relaxed(dfi_regs +
+				DDRMON_CH0_WR_NUM + i * 20);
 		count->c[i].access = readl_relaxed(dfi_regs +
 				DDRMON_CH0_DFI_ACCESS_NUM + i * 20);
 		count->c[i].total = readl_relaxed(dfi_regs +
@@ -218,6 +249,305 @@ 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=0x00")
+
+PMU_EVENT_ATTR_STRING(read-bytes, ddr_pmu_read_bytes, "event=0x01")
+PMU_EVENT_ATTR_STRING(read-bytes.unit, ddr_pmu_read_bytes_unit, "MB");
+PMU_EVENT_ATTR_STRING(read-bytes.scale, ddr_pmu_read_bytes_scale, "9.536743164e-07");
+
+PMU_EVENT_ATTR_STRING(write-bytes, ddr_pmu_write_bytes, "event=0x02")
+PMU_EVENT_ATTR_STRING(write-bytes.unit, ddr_pmu_write_bytes_unit, "MB");
+PMU_EVENT_ATTR_STRING(write-bytes.scale, ddr_pmu_write_bytes_scale, "9.536743164e-07");
+
+PMU_EVENT_ATTR_STRING(bytes, ddr_pmu_bytes, "event=0x03")
+PMU_EVENT_ATTR_STRING(bytes.unit, ddr_pmu_bytes_unit, "MB");
+PMU_EVENT_ATTR_STRING(bytes.scale, ddr_pmu_bytes_scale, "9.536743164e-07");
+
+static struct attribute *ddr_perf_events_attrs[] = {
+	&ddr_pmu_cycles.attr.attr,
+	&ddr_pmu_read_bytes.attr.attr,
+	&ddr_pmu_read_bytes_unit.attr.attr,
+	&ddr_pmu_read_bytes_scale.attr.attr,
+	&ddr_pmu_write_bytes.attr.attr,
+	&ddr_pmu_write_bytes_unit.attr.attr,
+	&ddr_pmu_write_bytes_scale.attr.attr,
+	&ddr_pmu_bytes.attr.attr,
+	&ddr_pmu_bytes_unit.attr.attr,
+	&ddr_pmu_bytes_scale.attr.attr,
+	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 -EOPNOTSUPP;
+
+	if (event->cpu < 0) {
+		dev_warn(dfi->dev, "Can't provide per-task data!\n");
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static void rockchip_ddr_perf_update_counters(struct rockchip_dfi *dfi)
+{
+	struct dmc_count count;
+	struct dmc_count *last = &dfi->last;
+	int blen = dfi->burst_len;
+	u32 diff;
+	int i;
+
+	rockchip_dfi_read_counters(dfi, &count);
+
+	diff = count.c[0].total - last->c[0].total;
+	dfi->frr.count[PERF_EVENT_CYCLES] += diff;
+
+	for (i = 0; i < DMC_MAX_CHANNELS; i++) {
+		if (!(dfi->channel_mask & BIT(i)))
+			continue;
+
+		diff = count.c[i].read_access - last->c[i].read_access;
+		dfi->frr.count[PERF_EVENT_READ_BYTES] += (u64)diff * blen * dfi->buswidth[i];
+
+		diff = count.c[i].write_access - last->c[i].write_access;
+		dfi->frr.count[PERF_EVENT_WRITE_BYTES] += (u64)diff * blen * dfi->buswidth[i];
+
+		diff = count.c[i].access - last->c[i].access;
+		dfi->frr.count[PERF_EVENT_BYTES] += (u64)diff * blen * dfi->buswidth[i];
+	}
+
+	dfi->last = count;
+}
+
+static void rockchip_ddr_perf_event_update(struct perf_event *event)
+{
+	struct rockchip_dfi *dfi = container_of(event->pmu, struct rockchip_dfi, pmu);
+	s64 prev;
+	u64 now;
+
+	rockchip_ddr_perf_update_counters(dfi);
+
+	now = dfi->frr.count[event->attr.config];
+	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)
+{
+	struct rockchip_dfi *dfi = container_of(event->pmu, struct rockchip_dfi, pmu);
+
+	rockchip_ddr_perf_update_counters(dfi);
+
+	local64_set(&event->hw.prev_count, dfi->frr.count[event->attr.config]);
+}
+
+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);
+	struct hw_perf_event *hwc = &event->hw;
+
+	hwc->state |= PERF_HES_STOPPED;
+
+	dfi->active_events++;
+
+	if (dfi->active_events == 1) {
+		rockchip_dfi_enable(dfi);
+		hrtimer_start(&dfi->timer, 0, 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);
+		rockchip_dfi_disable(dfi);
+	}
+}
+
+static enum hrtimer_restart rockchip_dfi_timer(struct hrtimer *timer)
+{
+	struct rockchip_dfi *dfi = container_of(timer, struct rockchip_dfi, timer);
+	ktime_t timeout;
+
+	rockchip_ddr_perf_update_counters(dfi);
+
+	timeout = ns_to_ktime(NSEC_PER_SEC);
+	hrtimer_forward_now(&dfi->timer, timeout);
+
+	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 int rockchip_ddr_perf_init(struct rockchip_dfi *dfi)
+{
+	struct pmu *pmu = &dfi->pmu;
+	int ret;
+
+	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;
+
+	/* Register the pmu instance for cpu hotplug */
+	ret = cpuhp_state_add_instance_nocalls(dfi->cpuhp_state, &dfi->node);
+	if (ret) {
+		dev_err(dfi->dev, "Error %d registering hotplug\n", ret);
+		goto cpuhp_instance_err;
+	}
+
+	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)
+		goto ddr_perf_err;
+
+	return 0;
+
+ddr_perf_err:
+	cpuhp_state_remove_instance_nocalls(dfi->cpuhp_state, &dfi->node);
+cpuhp_instance_err:
+	cpuhp_remove_multi_state(dfi->cpuhp_state);
+
+	return ret;
+}
+
+static void rockchip_ddr_perf_remove(struct rockchip_dfi *dfi)
+{
+	cpuhp_state_remove_instance_nocalls(dfi->cpuhp_state, &dfi->node);
+	cpuhp_remove_multi_state(dfi->cpuhp_state);
+
+	perf_pmu_unregister(&dfi->pmu);
+}
+
+#else
+static int rockchip_ddr_perf_init(struct rockchip_dfi *dfi)
+{
+	return 0;
+}
+
+static void rockchip_ddr_perf_remove(struct rockchip_dfi *dfi)
+{
+}
+#endif
+
 static int rk3399_dfi_init(struct rockchip_dfi *dfi)
 {
 	struct regmap *regmap_pmu = dfi->regmap_pmu;
@@ -234,6 +564,9 @@ static int rk3399_dfi_init(struct rockchip_dfi *dfi)
 
 	dfi->channel_mask = 3;
 
+	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;
 };
 
@@ -250,6 +583,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;
@@ -325,13 +660,27 @@ 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;
 }
 
+static int rockchip_dfi_remove(struct platform_device *pdev)
+{
+	struct rockchip_dfi *dfi = platform_get_drvdata(pdev);
+
+	rockchip_ddr_perf_remove(dfi);
+
+	return 0;
+}
+
 static struct platform_driver rockchip_dfi_driver = {
 	.probe	= rockchip_dfi_probe,
+	.remove	= rockchip_dfi_remove,
 	.driver = {
 		.name	= "rockchip-dfi",
 		.of_match_table = rockchip_dfi_id_match,
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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 15/21] PM / devfreq: rockchip-dfi: Add perf support

[Robin Murphy]

On 2023-05-05 12:38, 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.

Might it be useful to offer the option of monitoring each channel 
individually?

> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>   drivers/devfreq/event/rockchip-dfi.c | 349 +++++++++++++++++++++++++++
>   include/soc/rockchip/rk3399_grf.h    |   2 +
>   include/soc/rockchip/rk3568_grf.h    |   1 +
>   3 files changed, 352 insertions(+)
> 
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index eae010644935a..400b1b360e3c9 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -20,6 +20,7 @@
>   #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,14 +42,30 @@
>   					 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
>   
> +enum access_type {
> +	PERF_EVENT_CYCLES,
> +	PERF_EVENT_READ_BYTES,
> +	PERF_EVENT_WRITE_BYTES,
> +	PERF_EVENT_BYTES,
> +	PERF_ACCESS_TYPE_MAX,
> +};

The design you have here means these values *must* match the ones in the 
format attribute strings, so it might be nice to define them explicitly.

> +
>   struct dmc_count_channel {
>   	u32 access;
>   	u32 total;
> +	u32 read_access;
> +	u32 write_access;
> +};
> +
> +struct dmc_count_channel64 {
> +	u64 count[PERF_ACCESS_TYPE_MAX];
>   };
>   
>   struct dmc_count {
> @@ -65,6 +82,7 @@ struct rockchip_dfi {
>   	struct devfreq_event_desc desc;
>   	struct dmc_count count;
>   	struct dmc_count last_event_count;
> +	struct dmc_count last;
>   	struct device *dev;
>   	void __iomem *regs;
>   	struct regmap *regmap_pmu;
> @@ -73,6 +91,15 @@ 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;
> +	struct dmc_count_channel64 frr;
> +	int active_events;
> +	int burst_len;
> +	int buswidth[DMC_MAX_CHANNELS];
>   };
>   
>   static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
> @@ -148,6 +175,10 @@ 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].read_access = readl_relaxed(dfi_regs +
> +				DDRMON_CH0_RD_NUM + i * 20);
> +		count->c[i].write_access = readl_relaxed(dfi_regs +
> +				DDRMON_CH0_WR_NUM + i * 20);
>   		count->c[i].access = readl_relaxed(dfi_regs +
>   				DDRMON_CH0_DFI_ACCESS_NUM + i * 20);
>   		count->c[i].total = readl_relaxed(dfi_regs +
> @@ -218,6 +249,305 @@ 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=0x00")
> +
> +PMU_EVENT_ATTR_STRING(read-bytes, ddr_pmu_read_bytes, "event=0x01")
> +PMU_EVENT_ATTR_STRING(read-bytes.unit, ddr_pmu_read_bytes_unit, "MB");
> +PMU_EVENT_ATTR_STRING(read-bytes.scale, ddr_pmu_read_bytes_scale, "9.536743164e-07");
> +
> +PMU_EVENT_ATTR_STRING(write-bytes, ddr_pmu_write_bytes, "event=0x02")
> +PMU_EVENT_ATTR_STRING(write-bytes.unit, ddr_pmu_write_bytes_unit, "MB");
> +PMU_EVENT_ATTR_STRING(write-bytes.scale, ddr_pmu_write_bytes_scale, "9.536743164e-07");
> +
> +PMU_EVENT_ATTR_STRING(bytes, ddr_pmu_bytes, "event=0x03")
> +PMU_EVENT_ATTR_STRING(bytes.unit, ddr_pmu_bytes_unit, "MB");
> +PMU_EVENT_ATTR_STRING(bytes.scale, ddr_pmu_bytes_scale, "9.536743164e-07");
> +
> +static struct attribute *ddr_perf_events_attrs[] = {
> +	&ddr_pmu_cycles.attr.attr,
> +	&ddr_pmu_read_bytes.attr.attr,
> +	&ddr_pmu_read_bytes_unit.attr.attr,
> +	&ddr_pmu_read_bytes_scale.attr.attr,
> +	&ddr_pmu_write_bytes.attr.attr,
> +	&ddr_pmu_write_bytes_unit.attr.attr,
> +	&ddr_pmu_write_bytes_scale.attr.attr,
> +	&ddr_pmu_bytes.attr.attr,
> +	&ddr_pmu_bytes_unit.attr.attr,
> +	&ddr_pmu_bytes_scale.attr.attr,
> +	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 -EOPNOTSUPP;

IMO this should be -EINVAL - the event isn't something that the driver 
would consider valid in general but happens to not be supported by this 
particular PMU instance, it's something that's fundamentally meaningless 
because the memory controller has no notion of what a task or even a CPU 
is, much less the ability to ever attribute low-level DRAM accesses to one.

> +
> +	if (event->cpu < 0) {
> +		dev_warn(dfi->dev, "Can't provide per-task data!\n");
> +		return -EOPNOTSUPP;

Ditto.

> +	}
> +

If you can't snapshot multiple counters atomically (and don't want to 
start/stop the whole monitor to achieve the same effect - I guess it 
would be hard to do that without getting in the way of devfreq 
operation), then you should ideally also check for and reject event 
groups containing multiple hardware events.

Most of all, though you should definitely validate the event config 
itself...

> +	return 0;
> +}
> +
> +static void rockchip_ddr_perf_update_counters(struct rockchip_dfi *dfi)
> +{
> +	struct dmc_count count;
> +	struct dmc_count *last = &dfi->last;
> +	int blen = dfi->burst_len;
> +	u32 diff;
> +	int i;
> +
> +	rockchip_dfi_read_counters(dfi, &count);
> +
> +	diff = count.c[0].total - last->c[0].total;
> +	dfi->frr.count[PERF_EVENT_CYCLES] += diff;
> +
> +	for (i = 0; i < DMC_MAX_CHANNELS; i++) {
> +		if (!(dfi->channel_mask & BIT(i)))
> +			continue;
> +
> +		diff = count.c[i].read_access - last->c[i].read_access;
> +		dfi->frr.count[PERF_EVENT_READ_BYTES] += (u64)diff * blen * dfi->buswidth[i];
> +
> +		diff = count.c[i].write_access - last->c[i].write_access;
> +		dfi->frr.count[PERF_EVENT_WRITE_BYTES] += (u64)diff * blen * dfi->buswidth[i];
> +
> +		diff = count.c[i].access - last->c[i].access;
> +		dfi->frr.count[PERF_EVENT_BYTES] += (u64)diff * blen * dfi->buswidth[i];
> +	}
> +
> +	dfi->last = count;
> +}
> +
> +static void rockchip_ddr_perf_event_update(struct perf_event *event)
> +{
> +	struct rockchip_dfi *dfi = container_of(event->pmu, struct rockchip_dfi, pmu);
> +	s64 prev;
> +	u64 now;
> +
> +	rockchip_ddr_perf_update_counters(dfi);
> +
> +	now = dfi->frr.count[event->attr.config];

...otherwise these array dereferences could go absolutely anywhere :(

FWIW I'd also be inclined to keep the intermediate values in units of 
bursts, and defer the conversion to bytes until this point where we know 
it's defintely wanted, rather than have to repeat it every time we sync 
each internal shadow counter. On which note, there are so many seemingly 
redundant (but not always?) sets of shadow counter values in play here 
that it's hard to easily make sense of - for instance I spent a while 
thinking a race exists between rockchip_ddr_perf_update_counters() and 
rockchip_dfi_read_counters() since it was all too easy to overlook that 
"last" and "last" are actually different things, and is dfi->count ever 
even used at all?

> +	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)
> +{
> +	struct rockchip_dfi *dfi = container_of(event->pmu, struct rockchip_dfi, pmu);
> +
> +	rockchip_ddr_perf_update_counters(dfi);
> +
> +	local64_set(&event->hw.prev_count, dfi->frr.count[event->attr.config]);
> +}
> +
> +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);
> +	struct hw_perf_event *hwc = &event->hw;
> +
> +	hwc->state |= PERF_HES_STOPPED;

This is never used.

> +
> +	dfi->active_events++;
> +
> +	if (dfi->active_events == 1) {
> +		rockchip_dfi_enable(dfi);
> +		hrtimer_start(&dfi->timer, 0, 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);
> +		rockchip_dfi_disable(dfi);
> +	}
> +}
> +
> +static enum hrtimer_restart rockchip_dfi_timer(struct hrtimer *timer)
> +{
> +	struct rockchip_dfi *dfi = container_of(timer, struct rockchip_dfi, timer);
> +	ktime_t timeout;
> +
> +	rockchip_ddr_perf_update_counters(dfi);
> +
> +	timeout = ns_to_ktime(NSEC_PER_SEC);
> +	hrtimer_forward_now(&dfi->timer, timeout);
> +
> +	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 int rockchip_ddr_perf_init(struct rockchip_dfi *dfi)
> +{
> +	struct pmu *pmu = &dfi->pmu;
> +	int ret;
> +
> +	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);

So, each instance gets its own distinct multi-instance state to only 
support a single instance each, except there can clearly only ever be a 
single instance globally anyway, since the PMU is registered with a 
fixed name... I can only guess that maybe such a contrivance is born of 
the notion of "global variables are bad", but honestly, this is worse :/

> +	if (ret < 0) {
> +		dev_err(dfi->dev, "cpuhp_setup_state_multi failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	dfi->cpuhp_state = ret;
> +
> +	/* Register the pmu instance for cpu hotplug */
> +	ret = cpuhp_state_add_instance_nocalls(dfi->cpuhp_state, &dfi->node);
> +	if (ret) {
> +		dev_err(dfi->dev, "Error %d registering hotplug\n", ret);
> +		goto cpuhp_instance_err;
> +	}
> +
> +	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)
> +		goto ddr_perf_err;
> +
> +	return 0;
> +
> +ddr_perf_err:
> +	cpuhp_state_remove_instance_nocalls(dfi->cpuhp_state, &dfi->node);
> +cpuhp_instance_err:
> +	cpuhp_remove_multi_state(dfi->cpuhp_state);
> +
> +	return ret;
> +}
> +
> +static void rockchip_ddr_perf_remove(struct rockchip_dfi *dfi)
> +{
> +	cpuhp_state_remove_instance_nocalls(dfi->cpuhp_state, &dfi->node);
> +	cpuhp_remove_multi_state(dfi->cpuhp_state);
> +
> +	perf_pmu_unregister(&dfi->pmu);
> +}
> +
> +#else
> +static int rockchip_ddr_perf_init(struct rockchip_dfi *dfi)
> +{
> +	return 0;
> +}
> +
> +static void rockchip_ddr_perf_remove(struct rockchip_dfi *dfi)
> +{
> +}
> +#endif
> +
>   static int rk3399_dfi_init(struct rockchip_dfi *dfi)
>   {
>   	struct regmap *regmap_pmu = dfi->regmap_pmu;
> @@ -234,6 +564,9 @@ static int rk3399_dfi_init(struct rockchip_dfi *dfi)
>   
>   	dfi->channel_mask = 3;
>   
> +	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;
>   };
>   
> @@ -250,6 +583,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;
> @@ -325,13 +660,27 @@ 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;
>   }
>   
> +static int rockchip_dfi_remove(struct platform_device *pdev)
> +{
> +	struct rockchip_dfi *dfi = platform_get_drvdata(pdev);
> +
> +	rockchip_ddr_perf_remove(dfi);
> +
> +	return 0;
> +}
> +
>   static struct platform_driver rockchip_dfi_driver = {
>   	.probe	= rockchip_dfi_probe,
> +	.remove	= rockchip_dfi_remove,
>   	.driver = {
>   		.name	= "rockchip-dfi",
>   		.of_match_table = rockchip_dfi_id_match,

You also want to set suppress_bind_attrs to be safe - the module 
reference that perf holds while the PMU is active still can't prevent a 
manual unbind of the platform driver making the PMU device disappear 
unexpectedly.

Thanks,
Robin.

> 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)

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 15/21] PM / devfreq: rockchip-dfi: Add perf support

[Sascha Hauer]

On Tue, May 09, 2023 at 09:04:58PM +0100, Robin Murphy wrote:
> On 2023-05-05 12:38, 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.
> 
> Might it be useful to offer the option of monitoring each channel
> individually?

I gave it a quick try. For my normal testing workload the values are
spread quite homogeneously:

 Performance counter stats for 'system wide':

        1062183406      rockchip_ddr/cycles/
           1517.57 MB   rockchip_ddr/read-bytes0/
           1517.11 MB   rockchip_ddr/write-bytes0/
           1519.78 MB   rockchip_ddr/read-bytes1/
           1514.97 MB   rockchip_ddr/write-bytes1/
           1518.69 MB   rockchip_ddr/read-bytes2/
           1516.01 MB   rockchip_ddr/write-bytes2/
           1519.37 MB   rockchip_ddr/read-bytes3/
           1515.46 MB   rockchip_ddr/write-bytes3/
          12137.84 MB   rockchip_ddr/bytes/

       1.005686209 seconds time elapsed

I found one workload that looks strange though:

 Performance counter stats for 'system wide':

        1076756945      rockchip_ddr/cycles/
           2326.55 MB   rockchip_ddr/read-bytes0/
             30.60 MB   rockchip_ddr/write-bytes0/
              0.24 MB   rockchip_ddr/read-bytes1/
              0.09 MB   rockchip_ddr/write-bytes1/
              0.21 MB   rockchip_ddr/read-bytes2/
              0.09 MB   rockchip_ddr/write-bytes2/
              0.30 MB   rockchip_ddr/read-bytes3/
              0.10 MB   rockchip_ddr/write-bytes3/
           2357.87 MB   rockchip_ddr/bytes/

I am running this tool (likely not the exact version):
https://github.com/mturquette/memtest/blob/master/mtest.c

I don't know yet what this tool is doing, but it somehow manages
to use only a single channel.

We might take this as a sign that monitoring the individual channels can
indeed be useful.

I'll address your other comments in the next round.

Thanks for your input

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 |

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 15/21] PM / devfreq: rockchip-dfi: Add perf support

[Sascha Hauer]

On Wed, May 10, 2023 at 09:56:34PM +0200, Sascha Hauer wrote:
> On Tue, May 09, 2023 at 09:04:58PM +0100, Robin Murphy wrote:
> > On 2023-05-05 12:38, 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.
> > 
> > Might it be useful to offer the option of monitoring each channel
> > individually?
> 
> I gave it a quick try. For my normal testing workload the values are
> spread quite homogeneously:
> 
>  Performance counter stats for 'system wide':
> 
>         1062183406      rockchip_ddr/cycles/
>            1517.57 MB   rockchip_ddr/read-bytes0/
>            1517.11 MB   rockchip_ddr/write-bytes0/
>            1519.78 MB   rockchip_ddr/read-bytes1/
>            1514.97 MB   rockchip_ddr/write-bytes1/
>            1518.69 MB   rockchip_ddr/read-bytes2/
>            1516.01 MB   rockchip_ddr/write-bytes2/
>            1519.37 MB   rockchip_ddr/read-bytes3/
>            1515.46 MB   rockchip_ddr/write-bytes3/
>           12137.84 MB   rockchip_ddr/bytes/
> 
>        1.005686209 seconds time elapsed
> 
> I found one workload that looks strange though:
> 
>  Performance counter stats for 'system wide':
> 
>         1076756945      rockchip_ddr/cycles/
>            2326.55 MB   rockchip_ddr/read-bytes0/
>              30.60 MB   rockchip_ddr/write-bytes0/
>               0.24 MB   rockchip_ddr/read-bytes1/
>               0.09 MB   rockchip_ddr/write-bytes1/
>               0.21 MB   rockchip_ddr/read-bytes2/
>               0.09 MB   rockchip_ddr/write-bytes2/
>               0.30 MB   rockchip_ddr/read-bytes3/
>               0.10 MB   rockchip_ddr/write-bytes3/
>            2357.87 MB   rockchip_ddr/bytes/
> 
> I am running this tool (likely not the exact version):
> https://github.com/mturquette/memtest/blob/master/mtest.c
> 
> I don't know yet what this tool is doing, but it somehow manages
> to use only a single channel.

Ok, I found it out. On my RK3588 board with four channels the channels
seem to be interleaved with 512 bytes. When I only read/write the first
512 bytes of a page then only channel 0 is used, channel 1 for the
second 512 bytes etc.

The mtest tool allocates a bunch of pages, but only ever reads and
writes the first word of each page.

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 |

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 15/21] PM / devfreq: rockchip-dfi: Add perf support

[Sascha Hauer]

On Tue, May 09, 2023 at 09:04:58PM +0100, Robin Murphy wrote:
> On 2023-05-05 12:38, Sascha Hauer wrote:
> > diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> > index eae010644935a..400b1b360e3c9 100644
> > --- a/drivers/devfreq/event/rockchip-dfi.c
> > +++ b/drivers/devfreq/event/rockchip-dfi.c
> > @@ -20,6 +20,7 @@
> >   #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>

[...]

> > +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 -EOPNOTSUPP;
> 
> IMO this should be -EINVAL - the event isn't something that the driver would
> consider valid in general but happens to not be supported by this particular
> PMU instance, it's something that's fundamentally meaningless because the
> memory controller has no notion of what a task or even a CPU is, much less
> the ability to ever attribute low-level DRAM accesses to one.
> 
> > +
> > +	if (event->cpu < 0) {
> > +		dev_warn(dfi->dev, "Can't provide per-task data!\n");
> > +		return -EOPNOTSUPP;
> 
> Ditto.
> 
> > +	}
> > +
> 
> If you can't snapshot multiple counters atomically (and don't want to
> start/stop the whole monitor to achieve the same effect - I guess it would
> be hard to do that without getting in the way of devfreq operation), then
> you should ideally also check for and reject event groups containing
> multiple hardware events.

Yes, starting/stopping the monitor for atomic snapshots is hard to do
without influencing devfreq operation, that's why I decided against it.

OTOH I consider it very useful being able to monitor read-bytes and
write-bytes at the same time (or to simultaneously monitor multiple
channels in the next version).

Indeed non atomically reading the snapshots introduces a small error,
but normally the time it takes to read out all channels should be rather
small compared to the time between the snapshots, so I think this error
is negligible.


> > +static int rockchip_ddr_perf_init(struct rockchip_dfi *dfi)
> > +{
> > +	struct pmu *pmu = &dfi->pmu;
> > +	int ret;
> > +
> > +	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);
> 
> So, each instance gets its own distinct multi-instance state to only support
> a single instance each, except there can clearly only ever be a single
> instance globally anyway, since the PMU is registered with a fixed name... I
> can only guess that maybe such a contrivance is born of the notion of
> "global variables are bad", but honestly, this is worse :/

So you are suggesting that struct pmu should be statically initialized
rather than dynamically allocated, right?
Are you referring to struct pmu only or also to struct rockchip_dfi?

I am not sure I would like this. Yes, the fixed name makes the driver
inherently single instance only, nevertheless I think the driver code is
easier to follow when it's written the usual way, with lifetime of the
dynamic data between probe() and remove().

As a compromise I could allocate the name dynamically as well, but I
have no idea how to make up a good id. Some other drivers use the base
address of the device, but that would mean the 'perf stat' calls would
differ between different SoCs without any real gain.

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 |

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 15/21] PM / devfreq: rockchip-dfi: Add perf support

[Jonathan Cameron]

On Fri,  5 May 2023 13:38:50 +0200
Sascha Hauer <s.hauer@pengutronix.de> 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 <s.hauer@pengutronix.de>

A few comments to add to Robin's review.

> ---
>  drivers/devfreq/event/rockchip-dfi.c | 349 +++++++++++++++++++++++++++
>  include/soc/rockchip/rk3399_grf.h    |   2 +
>  include/soc/rockchip/rk3568_grf.h    |   1 +
>  3 files changed, 352 insertions(+)
> 
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index eae010644935a..400b1b360e3c9 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -20,6 +20,7 @@
>  #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,14 +42,30 @@
>  					 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
>  
> +enum access_type {
> +	PERF_EVENT_CYCLES,
> +	PERF_EVENT_READ_BYTES,
> +	PERF_EVENT_WRITE_BYTES,
> +	PERF_EVENT_BYTES,
> +	PERF_ACCESS_TYPE_MAX,
> +};
> +
>  struct dmc_count_channel {
>  	u32 access;
>  	u32 total;
> +	u32 read_access;
> +	u32 write_access;

Silly question, but is access = read_access + write_access?
If so no need for keeping track of all 3 around.

Come to think of it, total could do with a more meaningful name
or a comment at least.  total what?

> +};
> +
> +struct dmc_count_channel64 {
> +	u64 count[PERF_ACCESS_TYPE_MAX];
>  };

Why bother with the structure?

>  
>  struct dmc_count {
> @@ -65,6 +82,7 @@ struct rockchip_dfi {
>  	struct devfreq_event_desc desc;
>  	struct dmc_count count;
>  	struct dmc_count last_event_count;
> +	struct dmc_count last;

Needs a more descriptive name...

>  	struct device *dev;
>  	void __iomem *regs;
>  	struct regmap *regmap_pmu;
> @@ -73,6 +91,15 @@ 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;
> +	struct dmc_count_channel64 frr;
> +	int active_events;
> +	int burst_len;
> +	int buswidth[DMC_MAX_CHANNELS];
>  };
>  
>  static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
> @@ -148,6 +175,10 @@ 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].read_access = readl_relaxed(dfi_regs +
> +				DDRMON_CH0_RD_NUM + i * 20);
> +		count->c[i].write_access = readl_relaxed(dfi_regs +
> +				DDRMON_CH0_WR_NUM + i * 20);

I guess no expensive, but you could gate these on the perf support being built given
not used for anything else

>  		count->c[i].access = readl_relaxed(dfi_regs +
>  				DDRMON_CH0_DFI_ACCESS_NUM + i * 20);
>  		count->c[i].total = readl_relaxed(dfi_regs +
> @@ -218,6 +249,305 @@ static const struct devfreq_event_ops rockchip_dfi_ops = {
>  	.set_event = rockchip_dfi_set_event,
>  };
>  

> +
> +static void rockchip_ddr_perf_event_start(struct perf_event *event, int flags)
> +{
> +	struct rockchip_dfi *dfi = container_of(event->pmu, struct rockchip_dfi, pmu);
> +
> +	rockchip_ddr_perf_update_counters(dfi);
> +
> +	local64_set(&event->hw.prev_count, dfi->frr.count[event->attr.config]);
> +}
> +
> +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);
> +	struct hw_perf_event *hwc = &event->hw;
> +
> +	hwc->state |= PERF_HES_STOPPED;
> +
> +	dfi->active_events++;
> +
> +	if (dfi->active_events == 1) {
> +		rockchip_dfi_enable(dfi);
> +		hrtimer_start(&dfi->timer, 0, HRTIMER_MODE_REL);

Trigger immediately?  Lot of work to just call the function.  Perhaps
instead wrap the contents of the callback with two functions, the callback
itself and one that does same reads etc and starts the timer.

I guess it doesn't really matter though.

To my mind the timer start should be in the event_start callback, but I
see there is plenty of precedence for doing it add and I doubt it matters.

> +	}
> +
> +	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);
> +		rockchip_dfi_disable(dfi);
> +	}
> +}
> +
> +static enum hrtimer_restart rockchip_dfi_timer(struct hrtimer *timer)
> +{
> +	struct rockchip_dfi *dfi = container_of(timer, struct rockchip_dfi, timer);
> +	ktime_t timeout;
> +
> +	rockchip_ddr_perf_update_counters(dfi);
> +
> +	timeout = ns_to_ktime(NSEC_PER_SEC);
> +	hrtimer_forward_now(&dfi->timer, timeout);

Trivial: No real advantage in local variable.

	hrtimer_forward-now(&dfi->timer, ns_to_ktime(NSEC_PER_SEC));

> +
> +	return HRTIMER_RESTART;
> +};

...

> +static int rockchip_ddr_perf_init(struct rockchip_dfi *dfi)
> +{

...

> +
> +	dfi->cpuhp_state = ret;
> +
> +	/* Register the pmu instance for cpu hotplug */

I'd argue that's pretty obvious so comment not needed.

> +	ret = cpuhp_state_add_instance_nocalls(dfi->cpuhp_state, &dfi->node);
> +	if (ret) {
> +		dev_err(dfi->dev, "Error %d registering hotplug\n", ret);
> +		goto cpuhp_instance_err;
> +	}
> +
> +	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)
> +		goto ddr_perf_err;
> +
> +	return 0;
> +
> +ddr_perf_err:
> +	cpuhp_state_remove_instance_nocalls(dfi->cpuhp_state, &dfi->node);
> +cpuhp_instance_err:
> +	cpuhp_remove_multi_state(dfi->cpuhp_state);

Instead of a single devm callback as suggested below, you could
do them separately for the various steps.  That way you can avoid
the need to do explicit error handling in this function.

> +
> +	return ret;
> +}


> @@ -325,13 +660,27 @@ 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;
>  }
>  
> +static int rockchip_dfi_remove(struct platform_device *pdev)
> +{
> +	struct rockchip_dfi *dfi = platform_get_drvdata(pdev);
> +
> +	rockchip_ddr_perf_remove(dfi);

If this is all you have in remove, use devm_add_action_or_reset()
to make it a devm managed callback and keep the whole flow
devm based.

Obviously makes no practical difference today, but it makes ordering
bugs as a result of future changes less likely.

Or, given this is a nice devm managed driver, do it at finer granularity
(see above).



> +
> +	return 0;
> +}
> +
>  static struct platform_driver rockchip_dfi_driver = {
>  	.probe	= rockchip_dfi_probe,
> +	.remove	= rockchip_dfi_remove,
>  	.driver = {
>  		.name	= "rockchip-dfi",
>  		.of_match_table = rockchip_dfi_id_match,



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 15/21] PM / devfreq: rockchip-dfi: Add perf support

[Sascha Hauer]

On Wed, May 17, 2023 at 11:53:59AM +0100, Jonathan Cameron wrote:
> On Fri,  5 May 2023 13:38:50 +0200
> Sascha Hauer <s.hauer@pengutronix.de> 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 <s.hauer@pengutronix.de>
> 
> A few comments to add to Robin's review.
> 
> > ---
> >  drivers/devfreq/event/rockchip-dfi.c | 349 +++++++++++++++++++++++++++
> >  include/soc/rockchip/rk3399_grf.h    |   2 +
> >  include/soc/rockchip/rk3568_grf.h    |   1 +
> >  3 files changed, 352 insertions(+)
> > 
> > diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> > index eae010644935a..400b1b360e3c9 100644
> > --- a/drivers/devfreq/event/rockchip-dfi.c
> > +++ b/drivers/devfreq/event/rockchip-dfi.c
> > @@ -20,6 +20,7 @@
> >  #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,14 +42,30 @@
> >  					 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
> >  
> > +enum access_type {
> > +	PERF_EVENT_CYCLES,
> > +	PERF_EVENT_READ_BYTES,
> > +	PERF_EVENT_WRITE_BYTES,
> > +	PERF_EVENT_BYTES,
> > +	PERF_ACCESS_TYPE_MAX,
> > +};
> > +
> >  struct dmc_count_channel {
> >  	u32 access;
> >  	u32 total;
> > +	u32 read_access;
> > +	u32 write_access;
> 
> Silly question, but is access = read_access + write_access?
> If so no need for keeping track of all 3 around.

Yes, 'access' is the sum of 'read_access' and 'write_access'.

The number of accesses can be read from a separate register. The original
devfreq driver used that register only, the perf support now wants to
have the read and write accesses counted separately.

You are right, we could now remove 'access'. Not sure if it's worth it
though, at least during development I was happy to see that the values
are consistent.

> 
> Come to think of it, total could do with a more meaningful name
> or a comment at least.  total what?

'total' counts the number of clock cycles in the DDR controller. I'll
add a patch to rename it accordingly.

> 
> > +};
> > +
> > +struct dmc_count_channel64 {
> > +	u64 count[PERF_ACCESS_TYPE_MAX];
> >  };
> 
> Why bother with the structure?

because copying a struct like this works:

	struct dmc_count_channel64 a, b;

	a = b;

With plain arrays it doesn't work without looping over the array or
using memcpy.

> 
> >  
> >  struct dmc_count {
> > @@ -65,6 +82,7 @@ struct rockchip_dfi {
> >  	struct devfreq_event_desc desc;
> >  	struct dmc_count count;
> >  	struct dmc_count last_event_count;
> > +	struct dmc_count last;
> 
> Needs a more descriptive name...

Yes, I already changed that here. This will look differently overall
next round.

> 
> >  	struct device *dev;
> >  	void __iomem *regs;
> >  	struct regmap *regmap_pmu;
> > @@ -73,6 +91,15 @@ 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;
> > +	struct dmc_count_channel64 frr;
> > +	int active_events;
> > +	int burst_len;
> > +	int buswidth[DMC_MAX_CHANNELS];
> >  };
> >  
> >  static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
> > @@ -148,6 +175,10 @@ 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].read_access = readl_relaxed(dfi_regs +
> > +				DDRMON_CH0_RD_NUM + i * 20);
> > +		count->c[i].write_access = readl_relaxed(dfi_regs +
> > +				DDRMON_CH0_WR_NUM + i * 20);
> 
> I guess no expensive, but you could gate these on the perf support being built given
> not used for anything else

Right, I think it's better readable without this ifdef or IS_ENABLED().

> 
> >  		count->c[i].access = readl_relaxed(dfi_regs +
> >  				DDRMON_CH0_DFI_ACCESS_NUM + i * 20);
> >  		count->c[i].total = readl_relaxed(dfi_regs +
> > @@ -218,6 +249,305 @@ static const struct devfreq_event_ops rockchip_dfi_ops = {
> >  	.set_event = rockchip_dfi_set_event,
> >  };
> >  
> 
> > +
> > +static void rockchip_ddr_perf_event_start(struct perf_event *event, int flags)
> > +{
> > +	struct rockchip_dfi *dfi = container_of(event->pmu, struct rockchip_dfi, pmu);
> > +
> > +	rockchip_ddr_perf_update_counters(dfi);
> > +
> > +	local64_set(&event->hw.prev_count, dfi->frr.count[event->attr.config]);
> > +}
> > +
> > +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);
> > +	struct hw_perf_event *hwc = &event->hw;
> > +
> > +	hwc->state |= PERF_HES_STOPPED;
> > +
> > +	dfi->active_events++;
> > +
> > +	if (dfi->active_events == 1) {
> > +		rockchip_dfi_enable(dfi);
> > +		hrtimer_start(&dfi->timer, 0, HRTIMER_MODE_REL);
> 
> Trigger immediately?  Lot of work to just call the function.  Perhaps
> instead wrap the contents of the callback with two functions, the callback
> itself and one that does same reads etc and starts the timer.

No need to trigger immediately, it just has to trigger before the timers
overflow. I can pass ns_to_ktime(NSEC_PER_SEC) instead of 0.

> 
> I guess it doesn't really matter though.
> 
> To my mind the timer start should be in the event_start callback, but I
> see there is plenty of precedence for doing it add and I doubt it matters.
> 
> > +	}
> > +
> > +	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);
> > +		rockchip_dfi_disable(dfi);
> > +	}
> > +}
> > +
> > +static enum hrtimer_restart rockchip_dfi_timer(struct hrtimer *timer)
> > +{
> > +	struct rockchip_dfi *dfi = container_of(timer, struct rockchip_dfi, timer);
> > +	ktime_t timeout;
> > +
> > +	rockchip_ddr_perf_update_counters(dfi);
> > +
> > +	timeout = ns_to_ktime(NSEC_PER_SEC);
> > +	hrtimer_forward_now(&dfi->timer, timeout);
> 
> Trivial: No real advantage in local variable.
> 
> 	hrtimer_forward-now(&dfi->timer, ns_to_ktime(NSEC_PER_SEC));
> 
> > +
> > +	return HRTIMER_RESTART;
> > +};
> 
> ...
> 
> > +static int rockchip_ddr_perf_init(struct rockchip_dfi *dfi)
> > +{
> 
> ...
> 
> > +
> > +	dfi->cpuhp_state = ret;
> > +
> > +	/* Register the pmu instance for cpu hotplug */
> 
> I'd argue that's pretty obvious so comment not needed.
> 
> > +	ret = cpuhp_state_add_instance_nocalls(dfi->cpuhp_state, &dfi->node);
> > +	if (ret) {
> > +		dev_err(dfi->dev, "Error %d registering hotplug\n", ret);
> > +		goto cpuhp_instance_err;
> > +	}
> > +
> > +	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)
> > +		goto ddr_perf_err;
> > +
> > +	return 0;
> > +
> > +ddr_perf_err:
> > +	cpuhp_state_remove_instance_nocalls(dfi->cpuhp_state, &dfi->node);
> > +cpuhp_instance_err:
> > +	cpuhp_remove_multi_state(dfi->cpuhp_state);
> 
> Instead of a single devm callback as suggested below, you could
> do them separately for the various steps.  That way you can avoid
> the need to do explicit error handling in this function.

Ok, will add some devm_add_action_or_reset().

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 |

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v4 16/21] PM / devfreq: rockchip-dfi: make register stride SoC specific

[Sascha Hauer]

The currently supported RK3399 has a stride of 20 between the channel
specific registers. Upcoming RK3588 has a different stride, so put
the stride into driver data to make it configurable.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/devfreq/event/rockchip-dfi.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 400b1b360e3c9..3d76e58c602b2 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -100,6 +100,7 @@ struct rockchip_dfi {
 	int active_events;
 	int burst_len;
 	int buswidth[DMC_MAX_CHANNELS];
+	int ddrmon_stride;
 };
 
 static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
@@ -176,13 +177,13 @@ static void rockchip_dfi_read_counters(struct rockchip_dfi *dfi, struct dmc_coun
 		if (!(dfi->channel_mask & BIT(i)))
 			continue;
 		count->c[i].read_access = readl_relaxed(dfi_regs +
-				DDRMON_CH0_RD_NUM + i * 20);
+				DDRMON_CH0_RD_NUM + i * dfi->ddrmon_stride);
 		count->c[i].write_access = readl_relaxed(dfi_regs +
-				DDRMON_CH0_WR_NUM + i * 20);
+				DDRMON_CH0_WR_NUM + i * dfi->ddrmon_stride);
 		count->c[i].access = readl_relaxed(dfi_regs +
-				DDRMON_CH0_DFI_ACCESS_NUM + i * 20);
+				DDRMON_CH0_DFI_ACCESS_NUM + i * dfi->ddrmon_stride);
 		count->c[i].total = readl_relaxed(dfi_regs +
-				DDRMON_CH0_COUNT_NUM + i * 20);
+				DDRMON_CH0_COUNT_NUM + i * dfi->ddrmon_stride);
 	}
 }
 
@@ -567,6 +568,8 @@ static int rk3399_dfi_init(struct rockchip_dfi *dfi)
 	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;
 
+	dfi->ddrmon_stride = 0x14;
+
 	return 0;
 };
 
-- 
2.39.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 16/21] PM / devfreq: rockchip-dfi: make register stride SoC specific

[Jonathan Cameron]

On Fri,  5 May 2023 13:38:51 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> The currently supported RK3399 has a stride of 20 between the channel
> specific registers. Upcoming RK3588 has a different stride, so put
> the stride into driver data to make it configurable.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
LGTM though the flip from decimal to hex is a little odd. Which
makes sense probably depends on what the datasheet uses...

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/devfreq/event/rockchip-dfi.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 400b1b360e3c9..3d76e58c602b2 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -100,6 +100,7 @@ struct rockchip_dfi {
>  	int active_events;
>  	int burst_len;
>  	int buswidth[DMC_MAX_CHANNELS];
> +	int ddrmon_stride;
>  };
>  
>  static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
> @@ -176,13 +177,13 @@ static void rockchip_dfi_read_counters(struct rockchip_dfi *dfi, struct dmc_coun
>  		if (!(dfi->channel_mask & BIT(i)))
>  			continue;
>  		count->c[i].read_access = readl_relaxed(dfi_regs +
> -				DDRMON_CH0_RD_NUM + i * 20);
> +				DDRMON_CH0_RD_NUM + i * dfi->ddrmon_stride);
>  		count->c[i].write_access = readl_relaxed(dfi_regs +
> -				DDRMON_CH0_WR_NUM + i * 20);
> +				DDRMON_CH0_WR_NUM + i * dfi->ddrmon_stride);
>  		count->c[i].access = readl_relaxed(dfi_regs +
> -				DDRMON_CH0_DFI_ACCESS_NUM + i * 20);
> +				DDRMON_CH0_DFI_ACCESS_NUM + i * dfi->ddrmon_stride);
>  		count->c[i].total = readl_relaxed(dfi_regs +
> -				DDRMON_CH0_COUNT_NUM + i * 20);
> +				DDRMON_CH0_COUNT_NUM + i * dfi->ddrmon_stride);
>  	}
>  }
>  
> @@ -567,6 +568,8 @@ static int rk3399_dfi_init(struct rockchip_dfi *dfi)
>  	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;
>  
> +	dfi->ddrmon_stride = 0x14;
> +
>  	return 0;
>  };
>  


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 16/21] PM / devfreq: rockchip-dfi: make register stride SoC specific

[Sascha Hauer]

On Tue, May 16, 2023 at 05:18:27PM +0100, Jonathan Cameron wrote:
> On Fri,  5 May 2023 13:38:51 +0200
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > The currently supported RK3399 has a stride of 20 between the channel
> > specific registers. Upcoming RK3588 has a different stride, so put
> > the stride into driver data to make it configurable.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> LGTM though the flip from decimal to hex is a little odd. Which
> makes sense probably depends on what the datasheet uses...

The datasheet only describes the actual register offsets, no distance
between them. However, the rk3588 has a stride of 0x4000 which
definitely reads better in hex. For consistency I changed to hex here as
well.

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 |

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v4 17/21] PM / devfreq: rockchip-dfi: account for multiple DDRMON_CTRL registers

[Sascha Hauer]

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 <s.hauer@pengutronix.de>
---
 drivers/devfreq/event/rockchip-dfi.c | 69 ++++++++++++++++++----------
 1 file changed, 46 insertions(+), 23 deletions(-)

diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 3d76e58c602b2..74d69153e6386 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -101,12 +101,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);
 
@@ -120,28 +121,39 @@ static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
 		goto out;
 	}
 
-	/* clear DDRMON_CTRL setting */
-	writel_relaxed(HIWORD_UPDATE(0, 0xffff), dfi_regs + DDRMON_CTRL);
+	for (i = 0; i < DMC_MAX_CHANNELS; i++) {
+		if (!(dfi->channel_mask & BIT(i)))
+			continue;
 
-	/* 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;
-	}
+		/* clear DDRMON_CTRL setting */
+		writel_relaxed(HIWORD_UPDATE(0, 0xffff),
+			       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:
+			writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_LPDDR23,
+						     DDRMON_CTRL_DDR_TYPE_MASK),
+				       dfi_regs + i * dfi->ddrmon_stride + 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 + i * dfi->ddrmon_stride + DDRMON_CTRL);
+			break;
+		default:
+			break;
+		}
+
+		/* 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);
 
-	/* enable count, use software mode */
-	writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_SOFTWARE_EN, DDRMON_CTRL_SOFTWARE_EN),
-		       dfi_regs + DDRMON_CTRL);
+		if (dfi->ddrmon_ctrl_single)
+			break;
+	}
 out:
 	mutex_unlock(&dfi->mutex);
 
@@ -151,6 +163,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);
 
@@ -161,8 +174,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);
@@ -569,6 +591,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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 17/21] PM / devfreq: rockchip-dfi: account for multiple DDRMON_CTRL registers

[Jonathan Cameron]

On Fri,  5 May 2023 13:38:52 +0200
Sascha Hauer <s.hauer@pengutronix.de> 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 <s.hauer@pengutronix.de>
Trivial suggestion inline, but either way this looks fine to me.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/devfreq/event/rockchip-dfi.c | 69 ++++++++++++++++++----------
>  1 file changed, 46 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 3d76e58c602b2..74d69153e6386 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -101,12 +101,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);
>  
> @@ -120,28 +121,39 @@ static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
>  		goto out;
>  	}
>  
> -	/* clear DDRMON_CTRL setting */
> -	writel_relaxed(HIWORD_UPDATE(0, 0xffff), dfi_regs + DDRMON_CTRL);
> +	for (i = 0; i < DMC_MAX_CHANNELS; i++) {
> +		if (!(dfi->channel_mask & BIT(i)))
> +			continue;
>  
> -	/* 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;
> -	}
> +		/* clear DDRMON_CTRL setting */

Indent is getting a bit deep. Perhaps it's better to factor out as a
*write_ddr_type() ?

> +		writel_relaxed(HIWORD_UPDATE(0, 0xffff),
> +			       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:
> +			writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_LPDDR23,
> +						     DDRMON_CTRL_DDR_TYPE_MASK),
> +				       dfi_regs + i * dfi->ddrmon_stride + 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 + i * dfi->ddrmon_stride + DDRMON_CTRL);
> +			break;
> +		default:
> +			break;
> +		}
> +
> +		/* 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);
>  
> -	/* enable count, use software mode */
> -	writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_SOFTWARE_EN, DDRMON_CTRL_SOFTWARE_EN),
> -		       dfi_regs + DDRMON_CTRL);
> +		if (dfi->ddrmon_ctrl_single)
> +			break;
> +	}
>  out:
>  	mutex_unlock(&dfi->mutex);
>  
> @@ -151,6 +163,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);
>  
> @@ -161,8 +174,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);
> @@ -569,6 +591,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;
>  };


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v4 18/21] PM / devfreq: rockchip-dfi: add support for RK3588

[Sascha Hauer]

Add support for the RK3588 to the driver. The RK3588 has four DDR
channels with a register stride of 0x4000 between the channel
registers, also it has a DDRMON_CTRL register per channel.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/devfreq/event/rockchip-dfi.c | 34 +++++++++++++++++++++++++++-
 include/soc/rockchip/rk3588_grf.h    | 18 +++++++++++++++
 2 files changed, 51 insertions(+), 1 deletion(-)
 create mode 100644 include/soc/rockchip/rk3588_grf.h

diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 74d69153e6386..3630981505c6d 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -25,8 +25,9 @@
 #include <soc/rockchip/rockchip_grf.h>
 #include <soc/rockchip/rk3399_grf.h>
 #include <soc/rockchip/rk3568_grf.h>
+#include <soc/rockchip/rk3588_grf.h>
 
-#define DMC_MAX_CHANNELS	2
+#define DMC_MAX_CHANNELS	4
 
 #define HIWORD_UPDATE(val, mask)	((val) | (mask) << 16)
 
@@ -616,6 +617,32 @@ static int rk3568_dfi_init(struct rockchip_dfi *dfi)
 	return 0;
 };
 
+static int rk3588_dfi_init(struct rockchip_dfi *dfi)
+{
+	struct regmap *regmap_pmu = dfi->regmap_pmu;
+	u32 reg2, reg3, reg4;
+
+	regmap_read(regmap_pmu, RK3588_PMUGRF_OS_REG2, &reg2);
+	regmap_read(regmap_pmu, RK3588_PMUGRF_OS_REG3, &reg3);
+	regmap_read(regmap_pmu, RK3588_PMUGRF_OS_REG4, &reg4);
+
+	dfi->ddr_type = FIELD_GET(RK3588_PMUGRF_OS_REG2_DRAMTYPE_INFO, reg2);
+
+	if (FIELD_GET(RK3588_PMUGRF_OS_REG3_SYSREG_VERSION, reg3) >= 0x3)
+		dfi->ddr_type |= FIELD_GET(RK3588_PMUGRF_OS_REG3_DRAMTYPE_INFO_V3, reg3) << 3;
+
+	dfi->buswidth[0] = FIELD_GET(RK3588_PMUGRF_OS_REG2_BW_CH0, reg2) == 0 ? 4 : 2;
+	dfi->buswidth[1] = FIELD_GET(RK3588_PMUGRF_OS_REG2_BW_CH1, reg2) == 0 ? 4 : 2;
+	dfi->buswidth[2] = FIELD_GET(RK3568_PMUGRF_OS_REG2_BW_CH0, reg4) == 0 ? 4 : 2;
+	dfi->buswidth[3] = FIELD_GET(RK3588_PMUGRF_OS_REG2_BW_CH1, reg4) == 0 ? 4 : 2;
+	dfi->channel_mask = FIELD_GET(RK3588_PMUGRF_OS_REG2_CH_INFO, reg2) |
+			    FIELD_GET(RK3588_PMUGRF_OS_REG2_CH_INFO, reg4) << 2;
+
+	dfi->ddrmon_stride = 0x4000;
+
+	return 0;
+};
+
 struct rockchip_dfi_devtype_data {
 	int (*init)(struct rockchip_dfi *dfi);
 };
@@ -628,9 +655,14 @@ static struct rockchip_dfi_devtype_data rk3568_devtype_data = {
 	.init = rk3568_dfi_init,
 };
 
+static struct rockchip_dfi_devtype_data rk3588_devtype_data = {
+	.init = rk3588_dfi_init,
+};
+
 static const struct of_device_id rockchip_dfi_id_match[] = {
 	{ .compatible = "rockchip,rk3399-dfi", .data = &rk3399_devtype_data },
 	{ .compatible = "rockchip,rk3568-dfi", .data = &rk3568_devtype_data },
+	{ .compatible = "rockchip,rk3588-dfi", .data = &rk3588_devtype_data },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, rockchip_dfi_id_match);
diff --git a/include/soc/rockchip/rk3588_grf.h b/include/soc/rockchip/rk3588_grf.h
new file mode 100644
index 0000000000000..630b35a550640
--- /dev/null
+++ b/include/soc/rockchip/rk3588_grf.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+#ifndef __SOC_RK3588_GRF_H
+#define __SOC_RK3588_GRF_H
+
+#define RK3588_PMUGRF_OS_REG2		0x208
+#define RK3588_PMUGRF_OS_REG2_DRAMTYPE_INFO		GENMASK(15, 13)
+#define RK3588_PMUGRF_OS_REG2_BW_CH0			GENMASK(3, 2)
+#define RK3588_PMUGRF_OS_REG2_BW_CH1                    GENMASK(19, 18)
+#define RK3588_PMUGRF_OS_REG2_CH_INFO                   GENMASK(29, 28)
+
+#define RK3588_PMUGRF_OS_REG3		0x20c
+#define RK3588_PMUGRF_OS_REG3_DRAMTYPE_INFO_V3		GENMASK(13, 12)
+#define RK3588_PMUGRF_OS_REG3_SYSREG_VERSION		GENMASK(31, 28)
+
+#define RK3588_PMUGRF_OS_REG4           0x210
+#define RK3588_PMUGRF_OS_REG5           0x214
+
+#endif /* __SOC_RK3588_GRF_H */
-- 
2.39.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 18/21] PM / devfreq: rockchip-dfi: add support for RK3588

[Jonathan Cameron]

On Fri,  5 May 2023 13:38:53 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> Add support for the RK3588 to the driver. The RK3588 has four DDR
> channels with a register stride of 0x4000 between the channel
> registers, also it has a DDRMON_CTRL register per channel.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
No idea if registers are right as I've not checked any datasheets, but
the code looks fine to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/devfreq/event/rockchip-dfi.c | 34 +++++++++++++++++++++++++++-
>  include/soc/rockchip/rk3588_grf.h    | 18 +++++++++++++++
>  2 files changed, 51 insertions(+), 1 deletion(-)
>  create mode 100644 include/soc/rockchip/rk3588_grf.h
> 
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 74d69153e6386..3630981505c6d 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -25,8 +25,9 @@
>  #include <soc/rockchip/rockchip_grf.h>
>  #include <soc/rockchip/rk3399_grf.h>
>  #include <soc/rockchip/rk3568_grf.h>
> +#include <soc/rockchip/rk3588_grf.h>
>  
> -#define DMC_MAX_CHANNELS	2
> +#define DMC_MAX_CHANNELS	4
>  
>  #define HIWORD_UPDATE(val, mask)	((val) | (mask) << 16)
>  
> @@ -616,6 +617,32 @@ static int rk3568_dfi_init(struct rockchip_dfi *dfi)
>  	return 0;
>  };
>  
> +static int rk3588_dfi_init(struct rockchip_dfi *dfi)
> +{
> +	struct regmap *regmap_pmu = dfi->regmap_pmu;
> +	u32 reg2, reg3, reg4;
> +
> +	regmap_read(regmap_pmu, RK3588_PMUGRF_OS_REG2, &reg2);
> +	regmap_read(regmap_pmu, RK3588_PMUGRF_OS_REG3, &reg3);
> +	regmap_read(regmap_pmu, RK3588_PMUGRF_OS_REG4, &reg4);
> +
> +	dfi->ddr_type = FIELD_GET(RK3588_PMUGRF_OS_REG2_DRAMTYPE_INFO, reg2);
> +
> +	if (FIELD_GET(RK3588_PMUGRF_OS_REG3_SYSREG_VERSION, reg3) >= 0x3)
> +		dfi->ddr_type |= FIELD_GET(RK3588_PMUGRF_OS_REG3_DRAMTYPE_INFO_V3, reg3) << 3;
> +
> +	dfi->buswidth[0] = FIELD_GET(RK3588_PMUGRF_OS_REG2_BW_CH0, reg2) == 0 ? 4 : 2;
> +	dfi->buswidth[1] = FIELD_GET(RK3588_PMUGRF_OS_REG2_BW_CH1, reg2) == 0 ? 4 : 2;
> +	dfi->buswidth[2] = FIELD_GET(RK3568_PMUGRF_OS_REG2_BW_CH0, reg4) == 0 ? 4 : 2;
> +	dfi->buswidth[3] = FIELD_GET(RK3588_PMUGRF_OS_REG2_BW_CH1, reg4) == 0 ? 4 : 2;
> +	dfi->channel_mask = FIELD_GET(RK3588_PMUGRF_OS_REG2_CH_INFO, reg2) |
> +			    FIELD_GET(RK3588_PMUGRF_OS_REG2_CH_INFO, reg4) << 2;
> +
> +	dfi->ddrmon_stride = 0x4000;
> +
> +	return 0;
> +};
> +
>  struct rockchip_dfi_devtype_data {
>  	int (*init)(struct rockchip_dfi *dfi);
>  };
> @@ -628,9 +655,14 @@ static struct rockchip_dfi_devtype_data rk3568_devtype_data = {
>  	.init = rk3568_dfi_init,
>  };
>  
> +static struct rockchip_dfi_devtype_data rk3588_devtype_data = {
> +	.init = rk3588_dfi_init,
> +};
> +
>  static const struct of_device_id rockchip_dfi_id_match[] = {
>  	{ .compatible = "rockchip,rk3399-dfi", .data = &rk3399_devtype_data },
>  	{ .compatible = "rockchip,rk3568-dfi", .data = &rk3568_devtype_data },
> +	{ .compatible = "rockchip,rk3588-dfi", .data = &rk3588_devtype_data },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, rockchip_dfi_id_match);
> diff --git a/include/soc/rockchip/rk3588_grf.h b/include/soc/rockchip/rk3588_grf.h
> new file mode 100644
> index 0000000000000..630b35a550640
> --- /dev/null
> +++ b/include/soc/rockchip/rk3588_grf.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +#ifndef __SOC_RK3588_GRF_H
> +#define __SOC_RK3588_GRF_H
> +
> +#define RK3588_PMUGRF_OS_REG2		0x208
> +#define RK3588_PMUGRF_OS_REG2_DRAMTYPE_INFO		GENMASK(15, 13)
> +#define RK3588_PMUGRF_OS_REG2_BW_CH0			GENMASK(3, 2)
> +#define RK3588_PMUGRF_OS_REG2_BW_CH1                    GENMASK(19, 18)
> +#define RK3588_PMUGRF_OS_REG2_CH_INFO                   GENMASK(29, 28)
> +
> +#define RK3588_PMUGRF_OS_REG3		0x20c
> +#define RK3588_PMUGRF_OS_REG3_DRAMTYPE_INFO_V3		GENMASK(13, 12)
> +#define RK3588_PMUGRF_OS_REG3_SYSREG_VERSION		GENMASK(31, 28)
> +
> +#define RK3588_PMUGRF_OS_REG4           0x210
> +#define RK3588_PMUGRF_OS_REG5           0x214
> +
> +#endif /* __SOC_RK3588_GRF_H */


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v4 19/21] arm64: dts: rockchip: rk3399: Enable DFI

[Sascha Hauer]

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 <s.hauer@pengutronix.de>
---
 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 1881b4b71f91d..4806afb8fd8ae 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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v4 20/21] arm64: dts: rockchip: rk356x: Add DFI

[Sascha Hauer]

The DFI unit can be used to measure DRAM utilization using perf. Add the
node to the device tree.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm64/boot/dts/rockchip/rk356x.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
index eed0059a68b8d..2fe297e125410 100644
--- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
@@ -948,6 +948,12 @@ qos_vop_m1: qos@fe1a8100 {
 		reg = <0x0 0xfe1a8100 0x0 0x20>;
 	};
 
+	dfi: dfi@fe230000 {
+		compatible = "rockchip,rk3568-dfi";
+		reg = <0x00 0xfe230000 0x00 0x400>;
+		rockchip,pmu = <&pmugrf>;
+	};
+
 	pcie2x1: pcie@fe260000 {
 		compatible = "rockchip,rk3568-pcie";
 		reg = <0x3 0xc0000000 0x0 0x00400000>,
-- 
2.39.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v4 21/21] dt-bindings: devfreq: event: convert Rockchip DFI binding to yaml

[Sascha Hauer]

Convert the Rockchip DFI binding to yaml. While at it add the newly
supported rk3568-dfi to the binding.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 .../bindings/devfreq/event/rockchip-dfi.txt   | 18 ---------
 .../bindings/devfreq/event/rockchip-dfi.yaml  | 39 +++++++++++++++++++
 2 files changed, 39 insertions(+), 18 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/devfreq/event/rockchip-dfi.txt
 create mode 100644 Documentation/devicetree/bindings/devfreq/event/rockchip-dfi.yaml

diff --git a/Documentation/devicetree/bindings/devfreq/event/rockchip-dfi.txt b/Documentation/devicetree/bindings/devfreq/event/rockchip-dfi.txt
deleted file mode 100644
index 148191b0fc158..0000000000000
--- a/Documentation/devicetree/bindings/devfreq/event/rockchip-dfi.txt
+++ /dev/null
@@ -1,18 +0,0 @@
-
-* Rockchip rk3399 DFI device
-
-Required properties:
-- compatible: Must be "rockchip,rk3399-dfi".
-- reg: physical base address of each DFI and length of memory mapped region
-- rockchip,pmu: phandle to the syscon managing the "pmu general register files"
-- clocks: phandles for clock specified in "clock-names" property
-- clock-names : the name of clock used by the DFI, must be "pclk_ddr_mon";
-
-Example:
-	dfi: dfi@ff630000 {
-		compatible = "rockchip,rk3399-dfi";
-		reg = <0x00 0xff630000 0x00 0x4000>;
-		rockchip,pmu = <&pmugrf>;
-		clocks = <&cru PCLK_DDR_MON>;
-		clock-names = "pclk_ddr_mon";
-	};
diff --git a/Documentation/devicetree/bindings/devfreq/event/rockchip-dfi.yaml b/Documentation/devicetree/bindings/devfreq/event/rockchip-dfi.yaml
new file mode 100644
index 0000000000000..c0ef02a2d262b
--- /dev/null
+++ b/Documentation/devicetree/bindings/devfreq/event/rockchip-dfi.yaml
@@ -0,0 +1,39 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/devfreq/event/rockchip-dfi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Rockchip DFI
+
+maintainers:
+  - Sascha Hauer <s.hauer@pengutronix.de>
+
+properties:
+  compatible:
+    enum:
+      - rk3399-dfi
+      - rk3568-dfi
+      - rk3588-dfi
+
+  clocks:
+    maxItems: 1
+
+  rockchip,pmu:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Phandle to the syscon managing the "PMU general register files".
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    dfi: dfi@fe230000 {
+        compatible = "rockchip,rk3568-dfi";
+        reg = <0x00 0xfe230000 0x00 0x400>;
+        rockchip,pmu = <&pmugrf>;
+    };
-- 
2.39.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 21/21] dt-bindings: devfreq: event: convert Rockchip DFI binding to yaml

[Krzysztof Kozlowski]

On 05/05/2023 13:38, Sascha Hauer wrote:
> Convert the Rockchip DFI binding to yaml. While at it add the newly
> supported rk3568-dfi to the binding.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

You missed at least DT list (maybe more), so this won't be tested.
Please resend and include all necessary entries.

> ---
>  .../bindings/devfreq/event/rockchip-dfi.txt   | 18 ---------
>  .../bindings/devfreq/event/rockchip-dfi.yaml  | 39 +++++++++++++++++++

Filename matching compatibles style.

>  2 files changed, 39 insertions(+), 18 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/devfreq/event/rockchip-dfi.txt
>  create mode 100644 Documentation/devicetree/bindings/devfreq/event/rockchip-dfi.yaml
> 
> diff --git a/Documentation/devicetree/bindings/devfreq/event/rockchip-dfi.txt b/Documentation/devicetree/bindings/devfreq/event/rockchip-dfi.txt
> deleted file mode 100644
> index 148191b0fc158..0000000000000
> --- a/Documentation/devicetree/bindings/devfreq/event/rockchip-dfi.txt
> +++ /dev/null
> @@ -1,18 +0,0 @@
> -
> -* Rockchip rk3399 DFI device
> -
> -Required properties:
> -- compatible: Must be "rockchip,rk3399-dfi".
> -- reg: physical base address of each DFI and length of memory mapped region
> -- rockchip,pmu: phandle to the syscon managing the "pmu general register files"
> -- clocks: phandles for clock specified in "clock-names" property
> -- clock-names : the name of clock used by the DFI, must be "pclk_ddr_mon";
> -
> -Example:
> -	dfi: dfi@ff630000 {
> -		compatible = "rockchip,rk3399-dfi";
> -		reg = <0x00 0xff630000 0x00 0x4000>;
> -		rockchip,pmu = <&pmugrf>;
> -		clocks = <&cru PCLK_DDR_MON>;
> -		clock-names = "pclk_ddr_mon";
> -	};
> diff --git a/Documentation/devicetree/bindings/devfreq/event/rockchip-dfi.yaml b/Documentation/devicetree/bindings/devfreq/event/rockchip-dfi.yaml
> new file mode 100644
> index 0000000000000..c0ef02a2d262b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/devfreq/event/rockchip-dfi.yaml
> @@ -0,0 +1,39 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/devfreq/event/rockchip-dfi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Rockchip DFI
> +
> +maintainers:
> +  - Sascha Hauer <s.hauer@pengutronix.de>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - rk3399-dfi
> +      - rk3568-dfi
> +      - rk3588-dfi

These are not real compatibles.

Does not look like you tested the bindings. Please run `make
dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).


Best regards,
Krzysztof


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 21/21] dt-bindings: devfreq: event: convert Rockchip DFI binding to yaml

[Krzysztof Kozlowski]

On 05/05/2023 18:29, Krzysztof Kozlowski wrote:
> On 05/05/2023 13:38, Sascha Hauer wrote:
>> Convert the Rockchip DFI binding to yaml. While at it add the newly
>> supported rk3568-dfi to the binding.
>>
>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> 
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC.  It might happen, that command when run on an older
> kernel, gives you outdated entries.  Therefore please be sure you base
> your patches on recent Linux kernel.
> 
> You missed at least DT list (maybe more), so this won't be tested.
> Please resend and include all necessary entries.
> 

Actually you already got such comments and nothing improved here. You
just resend same patch with same bugs.

This is a waste of our time. Ignoring output of maintainers means there
is no testing and the patch does not appear in the inboxes.

I don't understand why you insist on ignoring that output.

NAK.

Best regards,
Krzysztof


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 21/21] dt-bindings: devfreq: event: convert Rockchip DFI binding to yaml

[Sascha Hauer]

On Fri, May 05, 2023 at 06:31:35PM +0200, Krzysztof Kozlowski wrote:
> On 05/05/2023 18:29, Krzysztof Kozlowski wrote:
> > On 05/05/2023 13:38, Sascha Hauer wrote:
> >> Convert the Rockchip DFI binding to yaml. While at it add the newly
> >> supported rk3568-dfi to the binding.
> >>
> >> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > 
> > Please use scripts/get_maintainers.pl to get a list of necessary people
> > and lists to CC.  It might happen, that command when run on an older
> > kernel, gives you outdated entries.  Therefore please be sure you base
> > your patches on recent Linux kernel.
> > 
> > You missed at least DT list (maybe more), so this won't be tested.
> > Please resend and include all necessary entries.
> > 
> 
> Actually you already got such comments and nothing improved here. You
> just resend same patch with same bugs.
> 
> This is a waste of our time. Ignoring output of maintainers means there
> is no testing and the patch does not appear in the inboxes.
> 
> I don't understand why you insist on ignoring that output.

I skipped the device tree list because there was no change in the dts or
binding part compared to the previous version.

But ok, it's worse. I only *intended* to not change the dts/binding
part. In fact I did, because the things you mentioned were already fixed
in an earlier version and I indeed ran dtbs_check and dt_binding_check on
this ealier version.

Somehow old patches sneaked into this series. Sorry for that, I'll send
a new version once I sorted this out.

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 |

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 21/21] dt-bindings: devfreq: event: convert Rockchip DFI binding to yaml

[Krzysztof Kozlowski]

On 09/05/2023 11:37, Sascha Hauer wrote:
> On Fri, May 05, 2023 at 06:31:35PM +0200, Krzysztof Kozlowski wrote:
>> On 05/05/2023 18:29, Krzysztof Kozlowski wrote:
>>> On 05/05/2023 13:38, Sascha Hauer wrote:
>>>> Convert the Rockchip DFI binding to yaml. While at it add the newly
>>>> supported rk3568-dfi to the binding.
>>>>
>>>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>>>
>>> Please use scripts/get_maintainers.pl to get a list of necessary people
>>> and lists to CC.  It might happen, that command when run on an older
>>> kernel, gives you outdated entries.  Therefore please be sure you base
>>> your patches on recent Linux kernel.
>>>
>>> You missed at least DT list (maybe more), so this won't be tested.
>>> Please resend and include all necessary entries.
>>>
>>
>> Actually you already got such comments and nothing improved here. You
>> just resend same patch with same bugs.
>>
>> This is a waste of our time. Ignoring output of maintainers means there
>> is no testing and the patch does not appear in the inboxes.
>>
>> I don't understand why you insist on ignoring that output.
> 
> I skipped the device tree list because there was no change in the dts or
> binding part compared to the previous version.

Please always CC DT list and all maintainers. Just because something did
not change here, does not mean that nothing changed elsewhere, e.g. in
dependant schemas.

What's more, the binding should have changed because I asked to fix
several things.

> 
> But ok, it's worse. I only *intended* to not change the dts/binding
> part. In fact I did, because the things you mentioned were already fixed
> in an earlier version and I indeed ran dtbs_check and dt_binding_check on
> this ealier version.
> 
> Somehow old patches sneaked into this series. Sorry for that, I'll send
> a new version once I sorted this out.
> 
> Sascha
> 

Best regards,
Krzysztof


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 21/21] dt-bindings: devfreq: event: convert Rockchip DFI binding to yaml

[Sascha Hauer]

On Tue, May 09, 2023 at 11:40:26AM +0200, Krzysztof Kozlowski wrote:
> On 09/05/2023 11:37, Sascha Hauer wrote:
> > On Fri, May 05, 2023 at 06:31:35PM +0200, Krzysztof Kozlowski wrote:
> >> On 05/05/2023 18:29, Krzysztof Kozlowski wrote:
> >>> On 05/05/2023 13:38, Sascha Hauer wrote:
> >>>> Convert the Rockchip DFI binding to yaml. While at it add the newly
> >>>> supported rk3568-dfi to the binding.
> >>>>
> >>>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> >>>
> >>> Please use scripts/get_maintainers.pl to get a list of necessary people
> >>> and lists to CC.  It might happen, that command when run on an older
> >>> kernel, gives you outdated entries.  Therefore please be sure you base
> >>> your patches on recent Linux kernel.
> >>>
> >>> You missed at least DT list (maybe more), so this won't be tested.
> >>> Please resend and include all necessary entries.
> >>>
> >>
> >> Actually you already got such comments and nothing improved here. You
> >> just resend same patch with same bugs.
> >>
> >> This is a waste of our time. Ignoring output of maintainers means there
> >> is no testing and the patch does not appear in the inboxes.
> >>
> >> I don't understand why you insist on ignoring that output.
> > 
> > I skipped the device tree list because there was no change in the dts or
> > binding part compared to the previous version.
> 
> Please always CC DT list and all maintainers. Just because something did
> not change here, does not mean that nothing changed elsewhere, e.g. in
> dependant schemas.

Ok.

> 
> What's more, the binding should have changed because I asked to fix
> several things.

You commented on v1 of this series and the things you mentioned were
fixed in v2 and v3. It's v4 where I lost them.

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 |

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 00/21] Add perf support to the rockchip-dfi driver

[Vincent Legoll]

Hello,

On Fri, May 5, 2023 at 11:41 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> This is v4 of the patches adding DDR perf support to the rockchip-dfi
> driver.

I tested this series on a Pine64 QuartzPro64 dev board.

I applied the series on top of my branch, which is based on Collabora's
rockchip-3588 plus some QP64 DTS patches, and your WIP patch to
enable the series in the rk3588s.dtsi.

Looks like this is working properly:

# 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/write-bytes/                          [Kernel PMU event]

# and while doing nothing...
# 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':

        1058497566      rockchip_ddr/cycles/
              9.01 MB   rockchip_ddr/read-bytes/
              0.66 MB   rockchip_ddr/write-bytes/
              9.65 MB   rockchip_ddr/bytes/

       1.002460137 seconds time elapsed

# then with a background memory (ab-)user...
# memtester 4G > /dev/null 2>&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':

       10561937833      rockchip_ddr/cycles/
          59192.90 MB   rockchip_ddr/read-bytes/
          29104.02 MB   rockchip_ddr/write-bytes/
          88296.92 MB   rockchip_ddr/bytes/

      10.001867723 seconds time elapsed

You can add my T-B, for the whole series:

Tested-by: Vincent Legoll <vincent.legoll@gmail.com>

Thanks for your work
Regards

--
Vincent Legoll

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip