Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a69dfdc6-31da-f813-858a-fdf246dea4fe@os.amperecomputing.com>
Date:   Fri, 31 Mar 2023 14:24:12 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Reply-To: scclevenger@os.amperecomputing.com
Subject: Re: [PATCH V2 3/5] coresight: etm4x: Drop pid argument from
 etm4_probe()
Content-Language: en-US
To:     Suzuki K Poulose <suzuki.poulose@arm.com>,
        Anshuman Khandual <anshuman.khandual@arm.com>,
        linux-arm-kernel@lists.infradead.org, coresight@lists.linaro.org
Cc:     Rob Herring <robh+dt@kernel.org>,
        Frank Rowand <frowand.list@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Len Brown <lenb@kernel.org>,
        Sudeep Holla <sudeep.holla@arm.com>,
        Lorenzo Pieralisi <lpieralisi@kernel.org>,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        Mike Leach <mike.leach@linaro.org>,
        Leo Yan <leo.yan@linaro.org>, devicetree@vger.kernel.org,
        linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230327050537.30861-1-anshuman.khandual@arm.com>
 <20230327050537.30861-4-anshuman.khandual@arm.com>
 <d995fec6-1d3f-df37-724e-67d929e9e0db@arm.com>
From:   Steve Clevenger <scclevenger@os.amperecomputing.com>
Organization: Ampere Computing
In-Reply-To: <d995fec6-1d3f-df37-724e-67d929e9e0db@arm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?RGVRTmVyTGlFNVZkb3RvRFVMMnN1SzVXZ3hFMFN4a1dQS1lVSEJjRy9LaVNP?=
 =?utf-8?B?cUpneGxGOHVzaDBuSndIRWNRdU5zUTliVG13bXRlM0RBWGRjczZXL04rZWxk?=
 =?utf-8?B?dHEraFBZWi85TW02VTJINzQ5OXAwNUtidkxRZzdWdjBLUVdpSjNHTFkwRW8r?=
 =?utf-8?B?eXQ1eU1LWWV6TGp1UFRyb3FsbUVmUkMreEkwNlI1N2dxOWZlbllhcndhN1VV?=
 =?utf-8?B?Y2ppb0hsallCSGlOemN2bmZ2cThMbGlKRVRVWmE2c2hEVXBSL1dOUjhpUmZo?=
 =?utf-8?B?bFM0amhhTVlJVkN1aWk5bTZJU1pQSGIzMndNelpYZGNlZWFYZnRIOWFSR1ZV?=
 =?utf-8?B?bzg3T1BTU0k1VExoSGk4clVsRVhYV2JOL3Ria1F5VHJNbENXcUhvVVJqN2I1?=
 =?utf-8?B?bk5YN2l2cUxaK2pVTm9CSW01SHduZnJ0UVU5TGpCazhEd24xRVhHaGpVQ2g3?=
 =?utf-8?B?RzQ5NHd1QU5mYUZtL2JjT1NYSkN0OGdxSW5jdVFTUTBVdFBHdzR5dngvYXJj?=
 =?utf-8?B?NjZydlNqT3FCeVd0VW81eEhaQW9XMit1WVpaTWIrQ2VhMWJRQ0VtRmdKdlpU?=
 =?utf-8?B?ZlVTbW5VL0JIdXRYRmx3a2JsSFRvdFcwcmlCYlh0WWsxVGhVVXlWUnRJaFhn?=
 =?utf-8?B?ZHREcDRMeVR2ZU9xczdsVE5aNHUvMVVMMnAybzNYRG5vMFptZnZtbERBb1F4?=
 =?utf-8?B?dGtkVWVMdHNPYzdNUEh1N1lkNzVVOFFycmhoUkxIa0JUdTVsV2pBYmlxYnlQ?=
 =?utf-8?B?V3ZUVXh4cWtOZ0ZTS2dzQnh5Q3krcU94SEd6aU1iSU4zSWE0dThnQm9rOU12?=
 =?utf-8?B?QU5Zd3F1c1YxZ1p2VlBsUk9qTzhOUDkySFNBZmV0Y3dGVkwzOWxLOEk1dmI4?=
 =?utf-8?B?R2FJSVY2aHpvamZEVEFnd2pEY2lPYkJxbkZ1dnU4NDR4dk1KSG9HR2gvbXNH?=
 =?utf-8?B?cHRsREFoYUZYKzl4QStFaGw2RFRSVjFSclo0cGdkWGhYYXRSeDRQL2JnNkdM?=
 =?utf-8?B?RmJ6dUdYTG1KSHFlcVo3blF2enpiZFg5dkFFaUFRN09jRHhRVnN6UTVTUzhY?=
 =?utf-8?B?REFzRmpGV1M0OE8rQzgrZGZOTDF2M2YrV2FVQmdualg4VDdrL2ltOCtieS9G?=
 =?utf-8?B?d3JvZG5PNE5NVXd0L0ZDeVlWcTliS2NxNi9oMFkyM0d2UHJwWGE3OTlKNUl1?=
 =?utf-8?B?a3ZMUkZQMm9WNURGQXNtZzl2Z09VUnQrMVRsU1JPQ3E0MXVDb0VabjdIVTNv?=
 =?utf-8?B?c2hlLy9pU0JNZzBXc1FmVkllcUlKQWd0R3M3TlZSaURxcFpRbEkvUFZwOWpn?=
 =?utf-8?B?VmZEQThwSExWeGV1eXJDbXZ0TlBMWkUzSHRSZ3I0YjdXYUtTUWtWWmNJbmwz?=
 =?utf-8?B?WHdIeTdHbFFYSjVhdms0VFhNWmFiRDFSNUZ0VE0zQjA0YUtIMzI1YlFEUlg4?=
 =?utf-8?B?YWNLTG5IWXZXdTI1YXZxeW5VMjRwMkdVNG1CYzhyT0dGL1lRK1kwVzdhRGg0?=
 =?utf-8?B?QWNiRkJUOUhtTjZSK1hkUnZuSUdkb3dvamkyclJMNG5TV2FOTmlGQzhJTHhm?=
 =?utf-8?B?amhyekJ1S1cxWmZuOWRBcGN1WVp1UUVrcForditnOS93RFZuSnZ3Wi90Q3pp?=
 =?utf-8?B?eWt6QkRJR29hREIwNURUV0k4NFVKNFl1YjNTWUY3bXV2MzI5dnN3YXJPMEtv?=
 =?utf-8?B?dDhaMDVFWjgwb2FMUFN4SXdTMlJ6QXRSV0pudFZWR2JtYnlmL3VHbW9VUlJs?=
 =?utf-8?B?YTFsTjkvOW5Yb2JKTUxncE0wWjJQanp5T2JyZm5EUWhRNXJna2VNVHp6MXZI?=
 =?utf-8?B?bFd6WXJnSWw4NEQ5TTZvai9OUVIxNmNHVmhnS1p5bWlOVTdhTTlUSktQT1g3?=
 =?utf-8?B?YXgvWjZYaVJwUXRKRmk5elZHU0o1Rm1ESC9zb3k5VE5mUWxvdU5GMXMwTmtv?=
 =?utf-8?B?RDErb1M0azR5a2RZSS9hK2RndDVyQXhBZUpwUGlrYVYvSzRKSXdLSXEvUEhO?=
 =?utf-8?B?NURLY3h5SEp0ZnZQYjNWTFpnNHQxWEZLYjRQcEVtV1FvRFYxMml0NXBBZ3ZJ?=
 =?utf-8?B?WG1laVkzTHNRQVZrbU94clFueE5YaUZ4YjlaSXZqQnZiaFp3TU5GaVZUWjBM?=
 =?utf-8?B?OFZ2WlNDSXp6amZHSFgyaGM0VVkyaFNRcWpySE5QamRrWk82YWNEbmY0RFlr?=
 =?utf-8?Q?/RzwzTMtjqlbDlG5lyWIVpA=3D?=
X-OriginatorOrg: os.amperecomputing.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d3e5f3df-68bb-4209-83d9-08db322e496f
X-MS-Exchange-CrossTenant-AuthSource: SA1PR01MB8062.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Mar 2023 21:24:18.6667
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3bc2b170-fd94-476d-b0ce-4229bdc904a7
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: XShx+qmFYbEyJpZLDOMIgTCqQT9N+gZOnneYcvREE1JwIci35Yw9IdpWcAbVIbuoLh6x3lsCBAoj6+3bdD8thIVLqHKYAcZEfEYfzLUzvWh850JYBtAQjhC8cphJmBIp
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR01MB8250
X-Spam-Status: No, score=-0.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
        DKIM_VALID_AU,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,
        SPF_HELO_PASS,SPF_PASS autolearn=unavailable autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 3/31/2023 4:06 AM, Suzuki K Poulose wrote:
> On 27/03/2023 06:05, Anshuman Khandual wrote:
>> Coresight device pid can be retrieved from its iomem base address,
>> which is
>> stored in 'struct etm4x_drvdata'. This drops pid argument from
>> etm4_probe()
>> and 'struct etm4_init_arg'. Instead etm4_check_arch_features() derives
>> the
>> coresight device pid with a new helper coresight_get_pid(), right
>> before it
>> is consumed in etm4_hisi_match_pid().
>>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Cc: Mike Leach <mike.leach@linaro.org>
>> Cc: Leo Yan <leo.yan@linaro.org>
>> Cc: coresight@lists.linaro.org
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>   .../coresight/coresight-etm4x-core.c          | 21 +++++++------------
>>   include/linux/coresight.h                     | 12 +++++++++++
>>   2 files changed, 20 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> index 5d77571a8df9..3521838ab4fb 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> @@ -66,7 +66,6 @@ static u64 etm4_get_access_type(struct etmv4_config
>> *config);
>>   static enum cpuhp_state hp_online;
>>     struct etm4_init_arg {
>> -    unsigned int        pid;
>>       struct device        *dev;
>>       struct csdev_access    *csa;
>>   };
>> @@ -370,8 +369,10 @@ static void etm4_disable_arch_specific(struct
>> etmv4_drvdata *drvdata)
>>   }
>>     static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
>> -                      unsigned int id)
>> +                     struct csdev_access *csa)
>>   {
>> +    unsigned int id = coresight_get_pid(csa);
>> +
> 
> This throws up the following error on an ETE.
> 
> ete: trying to read unsupported register @fe0
> 
> So, I guess this must be performed only for iomem based
> devices. System instruction based device must be identified
> by MIDR_EL1/REVIDR_EL1 if needed for specific erratum.
> This is not required now. So, we could bail out early
> if we are system instruction based device.

Besides this, the PID is limited to (I think) 4 bits of ID. TRCIDRs
offer revision information, but nothing manufacturer specific save for
the designer. Register fields like MIDR_EL1 Variant + PartNum + Revision
and TRCPIDR3 REVAND offer help. It may be a combination of registers are
needed for a manufacturer to adequately ID a part to apply an erratum.
Perhaps you could at least cache MIDR_EL1 for possible future use?

> 
> 
>>       if (etm4_hisi_match_pid(id))
>>           set_bit(ETM4_IMPDEF_HISI_CORE_COMMIT, drvdata->arch_features);
>>   }
>> @@ -385,7 +386,7 @@ static void etm4_disable_arch_specific(struct
>> etmv4_drvdata *drvdata)
>>   }
>>     static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
>> -                     unsigned int id)
>> +                     struct csdev_access *csa)
>>   {
>>   }
>>   #endif /* CONFIG_ETM4X_IMPDEF_FEATURE */
>> @@ -1165,7 +1166,7 @@ static void etm4_init_arch_data(void *info)
>>       etm4_os_unlock_csa(drvdata, csa);
>>       etm4_cs_unlock(drvdata, csa);
>>   -    etm4_check_arch_features(drvdata, init_arg->pid);
>> +    etm4_check_arch_features(drvdata, csa);
>>         /* find all capabilities of the tracing unit */
>>       etmidr0 = etm4x_relaxed_read32(csa, TRCIDR0);
>> @@ -2048,7 +2049,7 @@ static int etm4_add_coresight_dev(struct
>> etm4_init_arg *init_arg)
>>       return 0;
>>   }
>>   -static int etm4_probe(struct device *dev, u32 etm_pid)
>> +static int etm4_probe(struct device *dev)
>>   {
>>       struct etmv4_drvdata *drvdata = dev_get_drvdata(dev);
>>       struct csdev_access access = { 0 };
>> @@ -2077,7 +2078,6 @@ static int etm4_probe(struct device *dev, u32
>> etm_pid)
>>         init_arg.dev = dev;
>>       init_arg.csa = &access;
>> -    init_arg.pid = etm_pid;
>>         /*
>>        * Serialize against CPUHP callbacks to avoid race condition
>> @@ -2124,7 +2124,7 @@ static int etm4_probe_amba(struct amba_device
>> *adev, const struct amba_id *id)
>>         drvdata->base = base;
>>       dev_set_drvdata(dev, drvdata);
>> -    ret = etm4_probe(dev, id->id);
>> +    ret = etm4_probe(dev);
>>       if (!ret)
>>           pm_runtime_put(&adev->dev);
>>   @@ -2146,12 +2146,7 @@ static int etm4_probe_platform_dev(struct
>> platform_device *pdev)
>>       pm_runtime_set_active(&pdev->dev);
>>       pm_runtime_enable(&pdev->dev);
>>   -    /*
>> -     * System register based devices could match the
>> -     * HW by reading appropriate registers on the HW
>> -     * and thus we could skip the PID.
>> -     */
>> -    ret = etm4_probe(&pdev->dev, 0);
>> +    ret = etm4_probe(&pdev->dev);
>>         pm_runtime_put(&pdev->dev);
>>       return ret;
>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>> index f19a47b9bb5a..f85b041ea475 100644
>> --- a/include/linux/coresight.h
>> +++ b/include/linux/coresight.h
>> @@ -370,6 +370,18 @@ static inline u32
>> csdev_access_relaxed_read32(struct csdev_access *csa,
>>       return csa->read(offset, true, false);
>>   }
>>   +#define CORESIGHT_PIDRn(i)    (0xFE0 + ((i) * 4))
>> +
>> +static inline u32 coresight_get_pid(struct csdev_access *csa)
>> +{
>> +    u32 i, pid = 0;
>> +
>> +    for (i = 0; i < 4; i++)
>> +        pid |= csdev_access_relaxed_read32(csa, CORESIGHT_PIDRn(i))
>> << (i * 8);
> 
> Given the above, we could make this iomem specific.
> 
> Suzuki
> 
> 
>> +
>> +    return pid;
>> +}
>> +
>>   static inline u64 csdev_access_relaxed_read_pair(struct csdev_access
>> *csa,
>>                            u32 lo_offset, u32 hi_offset)
>>   {
>