Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v6 2/2] drivers/perf: Add CCPI2 PMU support in ThunderX2
 UNCORE driver.
To:     Ganapatrao Prabhakerrao Kulkarni <gkulkarni@marvell.com>,
        "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>
References: <1571218608-15933-1-git-send-email-gkulkarni@marvell.com>
 <1571218608-15933-3-git-send-email-gkulkarni@marvell.com>
CC:     "mark.rutland@arm.com" <mark.rutland@arm.com>,
        "corbet@lwn.net" <corbet@lwn.net>,
        Jan Glauber <jglauber@marvell.com>,
        "Jayachandran Chandrasekharan Nair" <jnair@marvell.com>,
        "gklkml16@gmail.com" <gklkml16@gmail.com>,
        Robert Richter <rrichter@marvell.com>,
        "will@kernel.org" <will@kernel.org>,
        Zhangshaokun <zhangshaokun@hisilicon.com>
From:   John Garry <john.garry@huawei.com>
Message-ID: <b8e1a637-faf4-4567-7d3e-a4f13dfa1cf0@huawei.com>
Date:   Wed, 16 Oct 2019 14:30:30 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.3.0
MIME-Version: 1.0
In-Reply-To: <1571218608-15933-3-git-send-email-gkulkarni@marvell.com>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> +TX2_EVENT_ATTR(req_pktsent, CCPI2_EVENT_REQ_PKT_SENT);
> +TX2_EVENT_ATTR(snoop_pktsent, CCPI2_EVENT_SNOOP_PKT_SENT);
> +TX2_EVENT_ATTR(data_pktsent, CCPI2_EVENT_DATA_PKT_SENT);
> +TX2_EVENT_ATTR(gic_pktsent, CCPI2_EVENT_GIC_PKT_SENT);
> +
> +static struct attribute *ccpi2_pmu_events_attrs[] = {
> +	&tx2_pmu_event_attr_req_pktsent.attr.attr,
> +	&tx2_pmu_event_attr_snoop_pktsent.attr.attr,
> +	&tx2_pmu_event_attr_data_pktsent.attr.attr,
> +	&tx2_pmu_event_attr_gic_pktsent.attr.attr,
> +	NULL,
> +};

Hi Ganapatrao,

Have you considered adding these as uncore pmu-events in the perf tool?

Some advantages I see:
- perf list is not swamped with all these uncore events per PMU
For the Hisi uncore events, we get 100s of events (>600 on the board I 
just tested, which is crazy)
- can add more description in the JSON files
- less stuff in the kernel

> +
>  static const struct attribute_group l3c_pmu_events_attr_group = {
>  	.name = "events",
>  	.attrs = l3c_pmu_events_attrs,
> @@ -174,6 +240,11 @@ static const struct attribute_group dmc_pmu_events_attr_group = {
>  	.attrs = dmc_pmu_events_attrs,
>  };

[...]

>  		tx2_pmu->attr_groups = l3c_pmu_attr_groups;
>  		tx2_pmu->name = devm_kasprintf(dev, GFP_KERNEL,
>  				"uncore_l3c_%d", tx2_pmu->node);
> @@ -665,10 +846,13 @@ static struct tx2_uncore_pmu *tx2_uncore_pmu_init_dev(struct device *dev,
>  		tx2_pmu->stop_event = uncore_stop_event_l3c;
>  		break;
>  	case PMU_TYPE_DMC:
> -		tx2_pmu->max_counters = TX2_PMU_MAX_COUNTERS;
> +		tx2_pmu->max_counters = TX2_PMU_DMC_L3C_MAX_COUNTERS;
> +		tx2_pmu->counters_mask = 0x3;
>  		tx2_pmu->prorate_factor = TX2_PMU_DMC_CHANNELS;
>  		tx2_pmu->max_events = DMC_EVENT_MAX;
> +		tx2_pmu->events_mask = 0x1f;
>  		tx2_pmu->hrtimer_interval = TX2_PMU_HRTIMER_INTERVAL;
> +		tx2_pmu->hrtimer_callback = tx2_hrtimer_callback;
>  		tx2_pmu->attr_groups = dmc_pmu_attr_groups;
>  		tx2_pmu->name = devm_kasprintf(dev, GFP_KERNEL,
>  				"uncore_dmc_%d", tx2_pmu->node);
> @@ -676,6 +860,21 @@ static struct tx2_uncore_pmu *tx2_uncore_pmu_init_dev(struct device *dev,
>  		tx2_pmu->start_event = uncore_start_event_dmc;
>  		tx2_pmu->stop_event = uncore_stop_event_dmc;
>  		break;
> +	case PMU_TYPE_CCPI2:
> +		/* CCPI2 has 8 counters */
> +		tx2_pmu->max_counters = TX2_PMU_CCPI2_MAX_COUNTERS;
> +		tx2_pmu->counters_mask = 0x7;
> +		tx2_pmu->prorate_factor = 1;
> +		tx2_pmu->max_events = CCPI2_EVENT_MAX;
> +		tx2_pmu->events_mask = 0x1ff;
> +		tx2_pmu->attr_groups = ccpi2_pmu_attr_groups;
> +		tx2_pmu->name = devm_kasprintf(dev, GFP_KERNEL,
> +				"uncore_ccpi2_%d", tx2_pmu->node);

Do you need to check this for name == NULL?

> +		tx2_pmu->init_cntr_base = init_cntr_base_ccpi2;
> +		tx2_pmu->start_event = uncore_start_event_ccpi2;
> +		tx2_pmu->stop_event = uncore_stop_event_ccpi2;
> +		tx2_pmu->hrtimer_callback = NULL;
> +		break;
>  	case PMU_TYPE_INVALID:
>  		devm_kfree(dev, tx2_pmu);
>  		return NULL;
> @@ -744,7 +943,9 @@ static int tx2_uncore_pmu_offline_cpu(unsigned int cpu,
>  	if (cpu != tx2_pmu->cpu)
>  		return 0;
>
> -	hrtimer_cancel(&tx2_pmu->hrtimer);
> +	if (tx2_pmu->hrtimer_callback)
> +		hrtimer_cancel(&tx2_pmu->hrtimer);
> +
>  	cpumask_copy(&cpu_online_mask_temp, cpu_online_mask);
>  	cpumask_clear_cpu(cpu, &cpu_online_mask_temp);
>  	new_cpu = cpumask_any_and(
>

Thanks,
John