Re: [PATCH 1/7] PM / devfreq: event: Add new Exynos NoC probe driver

From: Chanwoo Choi
Date: Fri Apr 15 2016 - 01:24:11 EST

Next message: Darren Hart: "Re: [PATCH] fujitsu-laptop: Support radio LED"
Previous message: Darren Hart: "Re: [PATCH] platform:x86 decouple telemetry driver from the optional IPC resources"
In reply to: Krzysztof Kozlowski: "Re: [PATCH 1/7] PM / devfreq: event: Add new Exynos NoC probe driver"
Next in thread: Chanwoo Choi: "[PATCH 3/7] ARM: dts: Add NoC Probe dt node for Exynos542x SoC"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 2016ë 04ì 12ì 17:07, Krzysztof Kozlowski wrote:
> On 04/08/2016 07:00 AM, Chanwoo Choi wrote:
>> This patch adds NoC (Network on Chip) Probe driver which provides
>> the primitive values to get the performance data. The packets that the Network
>> on Chip (NoC) probes detects are transported over the network infrastructure.
>> Exynos542x bus has multiple NoC probes to provide bandwidth information about
>> behavior of the SoC that you can use while analyzing system performance.
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
>> ---
>> .../bindings/devfreq/event/exynos-nocp.txt | 86 +++++++
>> drivers/devfreq/event/Kconfig | 8 +
>> drivers/devfreq/event/Makefile | 2 +
>> drivers/devfreq/event/exynos-nocp.c | 247 +++++++++++++++++++++
>> drivers/devfreq/event/exynos-nocp.h | 78 +++++++
>> 5 files changed, 421 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/devfreq/event/exynos-nocp.txt
>> create mode 100644 drivers/devfreq/event/exynos-nocp.c
>> create mode 100644 drivers/devfreq/event/exynos-nocp.h
>>
>> diff --git a/Documentation/devicetree/bindings/devfreq/event/exynos-nocp.txt b/Documentation/devicetree/bindings/devfreq/event/exynos-nocp.txt
>> new file mode 100644
>> index 000000000000..03b74fed034c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/devfreq/event/exynos-nocp.txt
>> @@ -0,0 +1,86 @@
>> +
>> +* Samsung Exynos NoC (Network on Chip) Probe device
>> +
>> +The Samsung Exynos542x SoC has NoC (Network on Chip) Probe for NoC bus.
>> +NoC provides the primitive values to get the performance data. The packets
>> +that the Network on Chip (NoC) probes detects are transported over
>> +the network infrastructure to observer units. You can configure probes to
>> +capture packets with header or data on the data request response network,
>> +or as traffic debug or statistic collectors. Exynos542x bus has multiple
>> +NoC probes to provide bandwidth information about behavior of the SoC
>> +that you can use while analyzing system performance.
>> +
>> +Required properties:
>> +- compatible: Should be "samsung,exynos5420-nocp"
>> +- reg: physical base address of each NoC Probe and length of memory mapped region.
>> +
>> +Optional properties:
>> +- clock-names : the name of clock used by the NoC Probe, "nocp"
>> +- clocks : phandles for clock specified in "clock-names" property
>> +
>> +Example1 : NoC Probe nodes in exynos5420.dtsi are listed below.
>> +
>> + nocp_mem0_0: nocp_mem0_0@10CA1000 {
>> + compatible = "samsung,exynos5420-nocp";
>> + reg = <0x10CA1000 0x200>;
>> + status = "disabled";
>> + };
>> +
>> + nocp_mem0_1: nocp_mem0_1@10CA1400 {
>> + compatible = "samsung,exynos5420-nocp";
>> + reg = <0x10CA1400 0x200>;
>> + status = "disabled";
>> + };
>> +
>> + nocp_mem0_2: nocp_mem0_2@10CA1800 {
>> + compatible = "samsung,exynos5420-nocp";
>> + reg = <0x10CA1800 0x200>;
>> + status = "disabled";
>> + };
>> +
>> + nocp_mem0_3: nocp_mem0_0@10CA1C00 {
>> + compatible = "samsung,exynos5420-nocp";
>> + reg = <0x10CA1C00 0x200>;
>> + status = "disabled";
>> + };
>> +
>> + nocp_g3d_0: nocp_g3d_0@11A51000 {
>> + compatible = "samsung,exynos5420-nocp";
>> + reg = <0x11A51000 0x200>;
>> + status = "disabled";
>> + };
>> +
>> + nocp_g3d_1: nocp_g3d_1@11A51400 {
>> + compatible = "samsung,exynos5420-nocp";
>> + reg = <0x11A51400 0x200>;
>> + status = "disabled";
>> + };
>> +
>> +
>> +Example2 : Events of each NoC Probe node in exynos5422-odroidxu3-common.dtsi
>> + are listed below.
>> +
>> +
>> + &nocp_mem0_0 {
>> + status = "okay";
>> + };
>> +
>> + &nocp_mem0_1 {
>> + status = "okay";
>> + };
>> +
>> + &nocp_mem0_2 {
>> + status = "okay";
>> + };
>> +
>> + &nocp_mem0_3 {
>> + status = "okay";
>> + };
>> +
>> + &nocp_g3d_0 {
>> + status = "okay";
>> + };
>> +
>> + &nocp_g3d_1 {
>> + status = "okay";
>> + };
>
> I think this split in documentation between DTSI and DTS is not needed
> for example. Just add one example without the status and it should be
> sufficient to get the idea.

I'll modify it as following:

Example1 : NoC Probe nodes in Device Tree are listed below.

nocp_mem0_0: nocp@10CA1000 {
compatible = "samsung,exynos5420-nocp";
reg = <0x10CA1000 0x200>;
};

>
>> diff --git a/drivers/devfreq/event/Kconfig b/drivers/devfreq/event/Kconfig
>> index a11720affc31..1e8b4f469f38 100644
>> --- a/drivers/devfreq/event/Kconfig
>> +++ b/drivers/devfreq/event/Kconfig
>> @@ -13,6 +13,14 @@ menuconfig PM_DEVFREQ_EVENT
>>
>> if PM_DEVFREQ_EVENT
>>
>> +config DEVFREQ_EVENT_EXYNOS_NOCP
>> + bool "EXYNOS NoC (Network On Chip) Probe DEVFREQ event Driver"
>> + depends on ARCH_EXYNOS
>> + select PM_OPP
>> + help
>> + This add the devfreq-event driver for Exynos SoC. It provides NoC
>> + (Network on Chip) Probe counters to measure the bandwidth of AXI bus.
>> +
>> config DEVFREQ_EVENT_EXYNOS_PPMU
>> bool "EXYNOS PPMU (Platform Performance Monitoring Unit) DEVFREQ event Driver"
>> depends on ARCH_EXYNOS
>> diff --git a/drivers/devfreq/event/Makefile b/drivers/devfreq/event/Makefile
>> index be146ead79cf..3d6afd352253 100644
>> --- a/drivers/devfreq/event/Makefile
>> +++ b/drivers/devfreq/event/Makefile
>> @@ -1,2 +1,4 @@
>> # Exynos DEVFREQ Event Drivers
>> +
>> +obj-$(CONFIG_DEVFREQ_EVENT_EXYNOS_NOCP) += exynos-nocp.o
>> obj-$(CONFIG_DEVFREQ_EVENT_EXYNOS_PPMU) += exynos-ppmu.o
>> diff --git a/drivers/devfreq/event/exynos-nocp.c b/drivers/devfreq/event/exynos-nocp.c
>> new file mode 100644
>> index 000000000000..b9468a6143f6
>> --- /dev/null
>> +++ b/drivers/devfreq/event/exynos-nocp.c
>> @@ -0,0 +1,247 @@
>> +/*
>> + * exynos-nocp.c - EXYNOS NoC (Network On Chip) Probe support
>> + *
>> + * Copyright (c) 2016 Samsung Electronics Co., Ltd.
>> + * Author : Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/module.h>
>> +#include <linux/devfreq-event.h>
>> +#include <linux/kernel.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +#include "exynos-nocp.h"
>> +
>> +struct exynos_nocp {
>> + struct devfreq_event_dev *edev;
>> + struct devfreq_event_desc desc;
>> +
>> + struct device *dev;
>> +
>> + struct regmap *regmap;
>> + struct clk *clk;
>> +};
>> +
>> +/*
>> + * The devfreq-event ops structure for nocp probe.
>> + */
>> +static int exynos_nocp_set_event(struct devfreq_event_dev *edev)
>> +{
>> + struct exynos_nocp *nocp = devfreq_event_get_drvdata(edev);
>> +
>> + /* Disable NoC probe */
>> + regmap_update_bits(nocp->regmap, NOCP_MAIN_CTL,
>> + NOCP_MAIN_CTL_STATEN_MASK, 0);
>> +
>> + /* Set a statistics dump period to 0 */
>> + regmap_write(nocp->regmap, NOCP_STAT_PERIOD, 0x0);
>> +
>> + /* Set the IntEvent fields of *_SRC */
>> + regmap_update_bits(nocp->regmap, NOCP_COUNTERS_0_SRC,
>> + NOCP_CNT_SRC_INTEVENT_MASK,
>> + NOCP_CNT_SRC_INTEVENT_BYTE_MASK);
>> +
>> + regmap_update_bits(nocp->regmap, NOCP_COUNTERS_1_SRC,
>> + NOCP_CNT_SRC_INTEVENT_MASK,
>> + NOCP_CNT_SRC_INTEVENT_CHAIN_MASK);
>> +
>> + regmap_update_bits(nocp->regmap, NOCP_COUNTERS_2_SRC,
>> + NOCP_CNT_SRC_INTEVENT_MASK,
>> + NOCP_CNT_SRC_INTEVENT_CYCLE_MASK);
>> +
>> + regmap_update_bits(nocp->regmap, NOCP_COUNTERS_3_SRC,
>> + NOCP_CNT_SRC_INTEVENT_MASK,
>> + NOCP_CNT_SRC_INTEVENT_CHAIN_MASK);
>> +
>> +
>> + /* Set an alarm with a max/min value of 0 to generate StatALARM */
>> + regmap_write(nocp->regmap, NOCP_STAT_ALARM_MIN, 0x0);
>> + regmap_write(nocp->regmap, NOCP_STAT_ALARM_MAX, 0x0);
>> +
>> + /* Set AlarmMode */
>> + regmap_update_bits(nocp->regmap, NOCP_COUNTERS_0_ALARM_MODE,
>> + NOCP_CNT_ALARM_MODE_MASK,
>> + NOCP_CNT_ALARM_MODE_MIN_MAX_MASK);
>> + regmap_update_bits(nocp->regmap, NOCP_COUNTERS_1_ALARM_MODE,
>> + NOCP_CNT_ALARM_MODE_MASK,
>> + NOCP_CNT_ALARM_MODE_MIN_MAX_MASK);
>> + regmap_update_bits(nocp->regmap, NOCP_COUNTERS_2_ALARM_MODE,
>> + NOCP_CNT_ALARM_MODE_MASK,
>> + NOCP_CNT_ALARM_MODE_MIN_MAX_MASK);
>> + regmap_update_bits(nocp->regmap, NOCP_COUNTERS_3_ALARM_MODE,
>> + NOCP_CNT_ALARM_MODE_MASK,
>> + NOCP_CNT_ALARM_MODE_MIN_MAX_MASK);
>> +
>> + /* Enable the measurements by setting AlarmEn and StatEn */
>> + regmap_update_bits(nocp->regmap, NOCP_MAIN_CTL,
>> + NOCP_MAIN_CTL_STATEN_MASK | NOCP_MAIN_CTL_ALARMEN_MASK,
>> + NOCP_MAIN_CTL_STATEN_MASK | NOCP_MAIN_CTL_ALARMEN_MASK);
>> +
>> + /* Set GlobalEN */
>> + regmap_update_bits(nocp->regmap, NOCP_CFG_CTL,
>> + NOCP_CFG_CTL_GLOBALEN_MASK,
>> + NOCP_CFG_CTL_GLOBALEN_MASK);
>> +
>> + /* Enable NoC probe */
>> + regmap_update_bits(nocp->regmap, NOCP_MAIN_CTL,
>> + NOCP_MAIN_CTL_STATEN_MASK,
>> + NOCP_MAIN_CTL_STATEN_MASK);
>
> In all these regmap_update_bits() calls you are ignoring the return
> value. This does not look right.

OK. I'll check the return value of regmap function.

>
>> +
>> + return 0;
>> +}
>> +
>> +static int exynos_nocp_get_event(struct devfreq_event_dev *edev,
>> + struct devfreq_event_data *edata)
>> +{
>> + struct exynos_nocp *nocp = devfreq_event_get_drvdata(edev);
>> + unsigned int counter[4];
>> +
>> + /* Read cycle count */
>> + regmap_read(nocp->regmap, NOCP_COUNTERS_0_VAL, &counter[0]);
>> + regmap_read(nocp->regmap, NOCP_COUNTERS_1_VAL, &counter[1]);
>> + regmap_read(nocp->regmap, NOCP_COUNTERS_2_VAL, &counter[2]);
>> + regmap_read(nocp->regmap, NOCP_COUNTERS_3_VAL, &counter[3]);
>
> Ditto. If read fails, the data in counter should not be trusted (not
> initialized) so the devfreq_event_get_event() should not use it. The
> simplest would be to return error on each failure.

ditto.

>
>> +
>> + edata->load_count = ((counter[1] << 16) | counter[0]);
>> + edata->total_count = ((counter[3] << 16) | counter[2]);
>> +
>> + dev_dbg(&edev->dev, "%s (event: %ld/%ld)\n", edev->desc->name,
>> + edata->load_count, edata->total_count);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct devfreq_event_ops exynos_nocp_ops = {
>> + .set_event = exynos_nocp_set_event,
>> + .get_event = exynos_nocp_get_event,
>> +};
>> +
>> +static const struct of_device_id exynos_nocp_id_match[] = {
>> + { .compatible = "samsung,exynos5420-nocp", },
>> + { /* sentinel */ },
>> +};
>> +
>> +static struct regmap_config exynos_nocp_regmap_config = {
>> + .reg_bits = 32,
>> + .val_bits = 32,
>> + .reg_stride = 4,
>> + .max_register = NOCP_COUNTERS_3_VAL,
>> +};
>> +
>> +static int exynos_nocp_parse_dt(struct platform_device *pdev,
>> + struct exynos_nocp *nocp)
>> +{
>> + struct device *dev = nocp->dev;
>> + struct device_node *np = dev->of_node;
>> + struct resource *res;
>> + void __iomem *base;
>> + int ret = 0;
>> +
>> + if (!np) {
>> + dev_err(dev, "failed to find devicetree node\n");
>> + return -EINVAL;
>> + }
>> +
>> + nocp->clk = devm_clk_get(dev, "nocp");
>> + if (IS_ERR(nocp->clk))
>> + nocp->clk = NULL;
>> +
>> + /* Maps the memory mapped IO to control nocp register */
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (IS_ERR(res))
>> + return PTR_ERR(res);
>> +
>> + base = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(base))
>> + return PTR_ERR(base);
>> + exynos_nocp_regmap_config.max_register = resource_size(res) - 4;
>> +
>> + nocp->regmap = devm_regmap_init_mmio(dev, base,
>> + &exynos_nocp_regmap_config);
>> + if (IS_ERR(nocp->regmap)) {
>> + dev_err(dev, "failed to initialize regmap\n");
>> + ret = PTR_ERR(nocp->regmap);
>> + goto err;
>> + }
>> +
>> + return 0;
>> +
>> +err:
>> + devm_iounmap(dev, base);
>
> Why you need this? This is obtained by devm-like() interface so it
> should be released by the core.

I'll remove it.

>
>> +
>> + return ret;
>> +}
>> +
>> +static int exynos_nocp_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct device_node *np = dev->of_node;
>> + struct exynos_nocp *nocp;
>> + int ret;
>> +
>> + nocp = devm_kzalloc(&pdev->dev, sizeof(*nocp), GFP_KERNEL);
>> + if (!nocp)
>> + return -ENOMEM;
>> +
>> + nocp->dev = &pdev->dev;
>> +
>> + /* Parse dt data to get resource */
>> + ret = exynos_nocp_parse_dt(pdev, nocp);
>> + if (ret < 0) {
>> + dev_err(&pdev->dev,
>> + "failed to parse devicetree for resource\n");
>> + return ret;
>> + }
>> +
>> + /* Add devfreq-event device to measure the bandwidth of NoC */
>> + nocp->desc.ops = &exynos_nocp_ops;
>> + nocp->desc.driver_data = nocp;
>> + nocp->desc.name = np->name;
>> + nocp->edev = devm_devfreq_event_add_edev(&pdev->dev, &nocp->desc);
>> + if (IS_ERR(nocp->edev)) {
>> + ret = PTR_ERR(nocp->edev);
>> + dev_err(&pdev->dev,
>> + "failed to add devfreq-event device\n");
>> + goto err;
>
> This looks not needed and does not improve readability to me. Just
> 'return ret' always. At this point (before this if() statement) 'ret'
> equals to 0. Then you could remove the 'err' label and always 'return ret'.

OK.

Best Regards,
Chanwoo Choi

Next message: Darren Hart: "Re: [PATCH] fujitsu-laptop: Support radio LED"
Previous message: Darren Hart: "Re: [PATCH] platform:x86 decouple telemetry driver from the optional IPC resources"
In reply to: Krzysztof Kozlowski: "Re: [PATCH 1/7] PM / devfreq: event: Add new Exynos NoC probe driver"
Next in thread: Chanwoo Choi: "[PATCH 3/7] ARM: dts: Add NoC Probe dt node for Exynos542x SoC"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]