From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id D3D72C433FE for ; Thu, 27 Jan 2022 09:33:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238716AbiA0Jdp (ORCPT ); Thu, 27 Jan 2022 04:33:45 -0500 Received: from alexa-out-sd-02.qualcomm.com ([199.106.114.39]:19192 "EHLO alexa-out-sd-02.qualcomm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238644AbiA0Jdg (ORCPT ); Thu, 27 Jan 2022 04:33:36 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; i=@quicinc.com; q=dns/txt; s=qcdkim; t=1643276015; x=1674812015; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=ozq+LTwpMXjth0rTIKHrkM8marZVKxy8MHKPzE4YwIs=; b=Zwod3tojI8jd8fL/tO8sCswfAnx8Wl5ilYegyohCuwLCCpRmEN/cDw/S PA9IzaHwoBZJni+cWGWPM4fAL+LgoZ2kSANzuj8e+5+3DuTZK8xtZiO3m 1c+iXpRZnTgUVa8nqij90/3JLt02+cAuGMUY89RZhRFMbg95WKulYrjIo E=; Received: from unknown (HELO ironmsg02-sd.qualcomm.com) ([10.53.140.142]) by alexa-out-sd-02.qualcomm.com with ESMTP; 27 Jan 2022 01:33:35 -0800 X-QCInternal: smtphost Received: from nasanex01c.na.qualcomm.com ([10.47.97.222]) by ironmsg02-sd.qualcomm.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Jan 2022 01:33:34 -0800 Received: from nalasex01a.na.qualcomm.com (10.47.209.196) by nasanex01c.na.qualcomm.com (10.47.97.222) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.922.19; Thu, 27 Jan 2022 01:33:34 -0800 Received: from [10.253.77.50] (10.80.80.8) by nalasex01a.na.qualcomm.com (10.47.209.196) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.922.19; Thu, 27 Jan 2022 01:33:29 -0800 Message-ID: <25d9a681-b899-5936-868d-34f645ee40ef@quicinc.com> Date: Thu, 27 Jan 2022 17:33:26 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.3.0 Subject: Re: [PATCH v2 2/9] Coresight: Add coresight TPDM source driver Content-Language: en-US To: Mathieu Poirier CC: Mike Leach , Suzuki K Poulose , Alexander Shishkin , Leo Yan , "Greg Kroah-Hartman" , , , , Tingwei Zhang , Yuanfang Zhang , Tao Zhang , Trilok Soni , References: <20211209141543.21314-1-quic_jinlmao@quicinc.com> <20211209141543.21314-3-quic_jinlmao@quicinc.com> <20211214185714.GB1549991@p14s> <85ad17fb-c885-831d-d841-4c6f10920355@quicinc.com> <20211216190223.GA78825@p14s> <2703bf83-3a87-e69f-2392-7e0568e91712@quicinc.com> <20220121171552.GA1811357@p14s> <24c09945-bcda-81e4-f53c-af871f696094@quicinc.com> <20220126153437.GA2144852@p14s> From: Jinlong Mao In-Reply-To: <20220126153437.GA2144852@p14s> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01b.na.qualcomm.com (10.46.141.250) To nalasex01a.na.qualcomm.com (10.47.209.196) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 1/26/2022 11:34 PM, Mathieu Poirier wrote: > On Wed, Jan 26, 2022 at 03:07:38PM +0800, Jinlong Mao wrote: >> Hi Mathieu, >> >> Good afternoon. >> >> On 1/22/2022 1:15 AM, Mathieu Poirier wrote: >>> Hey Jinlong, >>> >>> On Fri, Jan 21, 2022 at 10:01:47PM +0800, Jinlong Mao wrote: >>>> Hi Mathieu, >>>> >>>> Good Day. >>>> >>>> On 12/17/2021 3:02 AM, Mathieu Poirier wrote: >>>>> [...] >>>>> >>>>>>>>> + >>>>>>>>> +static void tpdm_init_default_data(struct tpdm_drvdata *drvdata) >>>>>>>>> +{ >>>>>>>>> + static int traceid = TPDM_TRACE_ID_START; >>>>>>>>> + >>>>>>>>> + drvdata->traceid = traceid++; >>>>>>>>> +} >>>>>>>> I have been specific on how to properly do this in the last revision. Given the >>>>>>>> above about the MAINTAINERS file, I am not sure that I will continue reviewing this set. >>>>>>>> >>>>>>>> There is also no need to rush another revision as I won't have the bandwidth to >>>>>>>> process it before the holidays. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Mathieu >>>>>>> Hi Mathieu, >>>>>>> >>>>>>> Sorry, not addressed your previous comments here. >>>>>>> >>>>>>> For the trace id, each coresight component has 7 bits to store the trace >>>>>>> id. So the trace id should be from 1 to 127 as 0 is invalid. >>>>>> IDs 0x70 - 0x7F (`112 - 127 ) are reserved - see the ARM Coresight >>>>>> Architecture specification v3.0 >>>>>> >>>>> Correct >>>>> >>>>>>> Apart from TPDMs/STM/ETMs, we also have other coresight components in >>>>>>> our internal device. About 80 ids are already used. >>>>>>> >>>>>>> Some components have fixed trace id in HW. If we use functions below to >>>>>>> count the trace id, there will be conflict to other components. >>>>>>> >>>>>>> Can we use 1-15 for etm trace ids and 16 - 127 for other coresight >>>>>>> components ? And handle trace ids in its' own driver ? >>>>>>> >>>>>> This will limit systems to 15 cores - some have more! >>>>>> >>>>> Correct >>>>> >>>>>>> static inline int coresight_get_system_trace_id(int id) >>>>>>> { >>>>>>> /* Start system IDs above the highest per CPU trace ID. */ >>>>>>> return coresigth_get_trace_id(cpumask_last(cpu_possible_mask) + 1); >>>>>>> } >>>>> Looking at my own suggestion again this won't work since it returns the same traceID >>>>> when called multiple times. >>>>> >>>>> For this patchset and _without_ taking into account internal devices that have >>>>> their traceID set in HW: >>>>> >>>>> 1. Define a bitmask that is 7 bit wide. >>>> Should it be a 128 bit wide bitmask  (0--127)? >>> Yes, you are correct. >>> >>>>> 2. By default, set bits under 0x10 and between 0x70 - 0x7F. >>>>> 3. In coresight_get_system_trace_id(), drop the @id parameter and allocate the >>>>> first available bit after cpumask_last(cpu_possible_mask) + 1. >>>> Should it allocate the first available bit after (cpumask_last(cpu_possible_mask) *2 ) + 0x10 ? >>>> Return the first zero bit position as the trace id and set the bit. >>> I need to clarify something with Mike on this - I will get back to you on >>> Monday. >> Do you have more comments on this ? > I just received clarifications on this - there is no need to continue enacting > the current scheme and as such reserving bits following the above formula will > not be needed either. Simply assigning free traceIDs based on request will > work just fine. > > That of course is taking into account that 0x0, 0x1 and 0x70 - 0x7f are > reserved. 0x1 is currently used for STM and should eventually be fixed to > simply request a traceID the same way any other component do. You can fix it as > part of this set but it is not mandatory. > > Let me know if there are things I haven't been clear on. > > Thanks, > Mathieu Hi Mathieu, Do you mean ETM sources and other sources can share the same "coresight_get_trace_id" function ? int trace_id = 1; int coresight_get_trace_id () {     trace_id += 1;     if (trace_id > 1 && trace_id < 0x70)         return trace_id;     else         return -EINVAL; } or do we still need a different function for other sources like use "coresight_get_system_trace_id" ? Return the id from 1 and skip the ids which equeals to CORESIGHT_ETM_PMU_SEED + (cpu * 2). Thanks Jinlong Mao >>>>> 4. Define a new function called coresight_put_system_trace_id(int id) that >>>>> clears the bit in the mask corresponding to @id. >>>>> >>>>> For now that should work. >>>>> >>>>>>> static inline int coresight_get_trace_id(int cpu) >>>>>>> { >>>>>>> /* >>>>>>> * A trace ID of value 0 is invalid, so let's start at some >>>>>>> * random value that fits in 7 bits and go from there. Since >>>>>>> * the common convention is to have data trace IDs be I(N) + 1, >>>>>>> * set instruction trace IDs as a function of the CPU number. >>>>>>> */ >>>>>>> return (CORESIGHT_ETM_PMU_SEED + (cpu * 2)); >>>>>>> } >>>>>>> >>>>>> This fixed relationship between cpu and trace ID is used in the perf >>>>>> tooling to populate the elements in the perf.data file to correctly >>>>>> allow association between CPU and trace data, and thus allow correct >>>>>> trace decode. >>>>> TraceIDs associated to CPUs are communicated to the perf tooling by way of the >>>>> perf header - theoretically we should be able to change the allocation scheme >>>>> without impacting the decoding process. >>>>> >>>>>> It should be possible to create another more dynamic mapping scheme - >>>>>> but this must include a way to support the perf requirements too. >>>>>> >>>>> TraceIDs have been a lurking problem for as long as the subsystem has existed. >>>>> For now what I have suggested above should be sufficient to provide an >>>>> in-between solution that doesn't hold back this patchset. >>>>> >>>>> That being said, we need to start thinking about the best way to do this. I >>>>> will put a patchset together in the new year that aims in that direction. >>>>> >>>>>> Regards >>>>>> >>>>>> Mike >>>>>> >>>>>>> Thanks >>>>>>> >>>>>>> Jinlong Mao >>>>>>> >>>>>>>>> + >>>>>>>>> +static int tpdm_probe(struct amba_device *adev, const struct amba_id *id) >>>>>>>>> +{ >>>>>>>>> + struct device *dev = &adev->dev; >>>>>>>>> + struct coresight_platform_data *pdata; >>>>>>>>> + struct tpdm_drvdata *drvdata; >>>>>>>>> + struct coresight_desc desc = { 0 }; >>>>>>>>> + >>>>>>>>> + desc.name = coresight_alloc_device_name(&tpdm_devs, dev); >>>>>>>>> + if (!desc.name) >>>>>>>>> + return -ENOMEM; >>>>>>>>> + pdata = coresight_get_platform_data(dev); >>>>>>>>> + if (IS_ERR(pdata)) >>>>>>>>> + return PTR_ERR(pdata); >>>>>>>>> + adev->dev.platform_data = pdata; >>>>>>>>> + >>>>>>>>> + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); >>>>>>>>> + if (!drvdata) >>>>>>>>> + return -ENOMEM; >>>>>>>>> + drvdata->dev = &adev->dev; >>>>>>>>> + dev_set_drvdata(dev, drvdata); >>>>>>>>> + >>>>>>>>> + drvdata->base = devm_ioremap_resource(dev, &adev->res); >>>>>>>>> + if (!drvdata->base) >>>>>>>>> + return -ENOMEM; >>>>>>>>> + >>>>>>>>> + mutex_init(&drvdata->lock); >>>>>>>>> + >>>>>>>>> + desc.type = CORESIGHT_DEV_TYPE_SOURCE; >>>>>>>>> + desc.subtype.source_subtype = CORESIGHT_DEV_SUBTYPE_SOURCE_SYS; >>>>>>>>> + desc.ops = &tpdm_cs_ops; >>>>>>>>> + desc.pdata = adev->dev.platform_data; >>>>>>>>> + desc.dev = &adev->dev; >>>>>>>>> + drvdata->csdev = coresight_register(&desc); >>>>>>>>> + if (IS_ERR(drvdata->csdev)) >>>>>>>>> + return PTR_ERR(drvdata->csdev); >>>>>>>>> + >>>>>>>>> + tpdm_init_default_data(drvdata); >>>>>>>>> + pm_runtime_put(&adev->dev); >>>>>>>>> + >>>>>>>>> + return 0; >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> +static void __exit tpdm_remove(struct amba_device *adev) >>>>>>>>> +{ >>>>>>>>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(&adev->dev); >>>>>>>>> + >>>>>>>>> + coresight_unregister(drvdata->csdev); >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> +static struct amba_id tpdm_ids[] = { >>>>>>>>> + { >>>>>>>>> + .id = 0x000f0e00, >>>>>>>>> + .mask = 0x000fff00, >>>>>>>>> + }, >>>>>>>>> + { 0, 0}, >>>>>>>>> +}; >>>>>>>>> + >>>>>>>>> +static struct amba_driver tpdm_driver = { >>>>>>>>> + .drv = { >>>>>>>>> + .name = "coresight-tpdm", >>>>>>>>> + .owner = THIS_MODULE, >>>>>>>>> + .suppress_bind_attrs = true, >>>>>>>>> + }, >>>>>>>>> + .probe = tpdm_probe, >>>>>>>>> + .id_table = tpdm_ids, >>>>>>>>> +}; >>>>>>>>> + >>>>>>>>> +module_amba_driver(tpdm_driver); >>>>>>>>> + >>>>>>>>> +MODULE_LICENSE("GPL v2"); >>>>>>>>> +MODULE_DESCRIPTION("Trace, Profiling & Diagnostic Monitor driver"); >>>>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h >>>>>>>>> new file mode 100644 >>>>>>>>> index 000000000000..980ae90ff1c8 >>>>>>>>> --- /dev/null >>>>>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h >>>>>>>>> @@ -0,0 +1,31 @@ >>>>>>>>> +/* SPDX-License-Identifier: GPL-2.0 */ >>>>>>>>> +/* >>>>>>>>> + * Copyright (c) 2021 Qualcomm Innovation Center, Inc. All rights reserved. >>>>>>>>> + */ >>>>>>>>> + >>>>>>>>> +#ifndef _CORESIGHT_CORESIGHT_TPDM_H >>>>>>>>> +#define _CORESIGHT_CORESIGHT_TPDM_H >>>>>>>>> + >>>>>>>>> +/* Default value of the traceid */ >>>>>>>>> +#define TPDM_TRACE_ID_START 128 >>>>>>>>> + >>>>>>>>> +/** >>>>>>>>> + * struct tpdm_drvdata - specifics associated to an TPDM component >>>>>>>>> + * @base: memory mapped base address for this component. >>>>>>>>> + * @dev: The device entity associated to this component. >>>>>>>>> + * @csdev: component vitals needed by the framework. >>>>>>>>> + * @lock: lock for the enable value. >>>>>>>>> + * @enable: enable status of the component. >>>>>>>>> + * @traceid: value of the current ID for this component. >>>>>>>>> + */ >>>>>>>>> + >>>>>>>>> +struct tpdm_drvdata { >>>>>>>>> + void __iomem *base; >>>>>>>>> + struct device *dev; >>>>>>>>> + struct coresight_device *csdev; >>>>>>>>> + struct mutex lock; >>>>>>>>> + bool enable; >>>>>>>>> + int traceid; >>>>>>>>> +}; >>>>>>>>> + >>>>>>>>> +#endif /* _CORESIGHT_CORESIGHT_TPDM_H */ >>>>>>>>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h >>>>>>>>> index 93a2922b7653..e48d463be63b 100644 >>>>>>>>> --- a/include/linux/coresight.h >>>>>>>>> +++ b/include/linux/coresight.h >>>>>>>>> @@ -65,6 +65,7 @@ enum coresight_dev_subtype_source { >>>>>>>>> CORESIGHT_DEV_SUBTYPE_SOURCE_PROC, >>>>>>>>> CORESIGHT_DEV_SUBTYPE_SOURCE_BUS, >>>>>>>>> CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE, >>>>>>>>> + CORESIGHT_DEV_SUBTYPE_SOURCE_SYS, >>>>>>>>> }; >>>>>>>>> >>>>>>>>> enum coresight_dev_subtype_helper { >>>>>>>>> -- >>>>>>>>> 2.17.1 >>>>>>>>> >>>>>> -- >>>>>> Mike Leach >>>>>> Principal Engineer, ARM Ltd. >>>>>> Manchester Design Centre. UK