All of lore.kernel.org
 help / color / mirror / Atom feed
From: Steve Clevenger <scclevenger@os.amperecomputing.com>
To: Mike Leach <mike.leach@linaro.org>
Cc: mathieu.poirier@linaro.org, suzuki.poulose@arm.com,
	leo.yan@linaro.org, coresight@lists.linaro.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 1/3] coresight etm4x: Early clear TRCOSLAR.OSLK prior to TRCIDR1 read
Date: Fri, 20 Jan 2023 23:31:00 -0800	[thread overview]
Message-ID: <679cae64-5ecf-aedf-aebe-1309b4482f68@os.amperecomputing.com> (raw)
In-Reply-To: <CAJ9a7VgJf6epBrhTAPr59eq9iS=U5jpRRDCQthT3e63aiY_GAQ@mail.gmail.com>


Hi Mike,

Comments in-line.

Steve

On 1/20/2023 3:45 AM, Mike Leach wrote:
> Hi Steve,
> 
> On Fri, 20 Jan 2023 at 00:52, Steve Clevenger
> <scclevenger@os.amperecomputing.com> wrote:
>>
>> Add Ampere early clear of ETM TRCOSLAR.OSLK prior to TRCIDR1 access.
>> Ampere Computing erratum AC03_DEBUG_06 describes an Ampere
>> Computing design decision MMIO reads are considered the same as an
>> external debug access. If TRCOSLAR.OSLK is set, the TRCIDR1 access
>> results in a bus fault followed by a kernel panic.  A TRCIDR1 read
>> is valid regardless of TRCOSLAR.OSLK provided MMIO access
>> (now deprecated) is supported.
>> AC03_DEBUG_06 is described in the AmpereOne Developer Errata:
>> https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation
>>
>> Add Ampere ETM PID required for Coresight ETM driver support.
>>
>> Signed-off-by: Steve Clevenger <scclevenger@os.amperecomputing.com>
>> ---
>>  .../coresight/coresight-etm4x-core.c          | 36 +++++++++++++++----
>>  drivers/hwtracing/coresight/coresight-etm4x.h |  2 ++
>>  2 files changed, 32 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> index 1cc052979e01..533be1928a09 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> @@ -1091,19 +1091,34 @@ static void etm4_init_arch_data(void *info)
>>         drvdata = dev_get_drvdata(init_arg->dev);
>>         csa = init_arg->csa;
>>
> 
> As far as I can tell there appears to be an initialisation issue here.
> etm_probe()
> ...
> struct csdev_access access = { 0 };
> ...
> init_arg.csa = &access
> 
> ::call=> etm4_init_arch_data(init_arg)
> 
> Thus csa is uninitialised?
It looks to me csa is intended to be initialized to zero? In any case,
the Ampere check uses only the ETM pid, which is initialized directly above.

> 
>> +       /* Detect the support for OS Lock before we actually use it */
>> +       etm_detect_os_lock(drvdata, csa);
>> +
>> +       /*
>> +        * For ETM implementations that consider MMIO an external access
>> +        * clear TRCOSLAR.OSLK early.
>> +        */
>> +       if (drvdata->mmio_external)
>> +               etm4_os_unlock_csa(drvdata, csa);
>> +
>>         /*
>>          * If we are unable to detect the access mechanism,
>>          * or unable to detect the trace unit type, fail
>> -        * early.
>> +        * early. Reset TRCOSLAR.OSLK if cleared.
>>          */
>> -       if (!etm4_init_csdev_access(drvdata, csa))
>> +       if (!etm4_init_csdev_access(drvdata, csa)) {
> 
> This call initialises csa according to sysreg / iomem access requirements
csa is initialized only when no drvdata->base exists. Under what
circumstance would there be no ETM base given the recommended CoreSight
ACPI implementation? See the examples in ARM Document number: DEN0067.
> 
> 
> 
>> +               if (drvdata->mmio_external)
>> +                       etm4_os_lock(drvdata);
>>                 return;
>> +       }
>>
>> -       /* Detect the support for OS Lock before we actually use it */
>> -       etm_detect_os_lock(drvdata, csa);
>> +       /*
>> +        * Make sure all registers are accessible
>> +        * TRCOSLAR.OSLK may already be clear
>> +        */
>> +       if (!drvdata->mmio_external)
>> +               etm4_os_unlock_csa(drvdata, csa);
>>
>> -       /* Make sure all registers are accessible */
>> -       etm4_os_unlock_csa(drvdata, csa);
>>         etm4_cs_unlock(drvdata, csa);
>>
>>         etm4_check_arch_features(drvdata, init_arg->pid);
>> @@ -2027,6 +2042,14 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
>>         init_arg.csa = &access;
>>         init_arg.pid = etm_pid;
>>
>> +       /*
>> +        * Ampere ETM v4.6 considers MMIO access as external. This mask
>> +        * isolates the manufacturer JEP106 ID in the PID.
>> +        * TRCPIDR2 (JEDC|DES_1) << 16 | TRCPIDR1 (DES_0) << 8)
>> +        */
>> +       if ((init_arg.pid & 0x000FF000) == 0x00096000)
>> +               drvdata->mmio_external = true;
>> +
>>         /*
>>          * Serialize against CPUHP callbacks to avoid race condition
>>          * between the smp call and saving the delayed probe.
>> @@ -2192,6 +2215,7 @@ static const struct amba_id etm4_ids[] = {
>>         CS_AMBA_ID(0x000bb95e),                 /* Cortex-A57 */
>>         CS_AMBA_ID(0x000bb95a),                 /* Cortex-A72 */
>>         CS_AMBA_ID(0x000bb959),                 /* Cortex-A73 */
>> +       CS_AMBA_UCI_ID(0x00096000, uci_id_etm4),/* Ampere ARMv8 */
>>         CS_AMBA_UCI_ID(0x000bb9da, uci_id_etm4),/* Cortex-A35 */
>>         CS_AMBA_UCI_ID(0x000bbd05, uci_id_etm4),/* Cortex-A55 */
>>         CS_AMBA_UCI_ID(0x000bbd0a, uci_id_etm4),/* Cortex-A75 */
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
>> index 4b21bb79f168..cf4f9f2e1807 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
>> @@ -1015,6 +1015,7 @@ struct etmv4_save_state {
>>   * @skip_power_up: Indicates if an implementation can skip powering up
>>   *                the trace unit.
>>   * @arch_features: Bitmap of arch features of etmv4 devices.
>> + * @mmio_external: True if ETM considers MMIO an external access.
>>   */
>>  struct etmv4_drvdata {
>>         void __iomem                    *base;
>> @@ -1067,6 +1068,7 @@ struct etmv4_drvdata {
>>         bool                            state_needs_restore;
>>         bool                            skip_power_up;
>>         DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
> 
> Rather than continue to add bools - is it not worthwhile adding to the
> bitmap above and extending the arch features API to allow a
> "has_feature" call?
I can look into this. I agree using a bool for every exception doesn't
scale well. Referring to one Suzuki Poulose review comment, his proposal
to clear TRCOSLAR.OSLK early for all parts would mean one of these bools
could go away. Otherwise, possibly add one (or more) bit definitions for
use by the etm4_disable_arch_specific call. The order of this call would
need to change, depending.

> 
>> +       bool                            mmio_external;
>>  };
>>
>>  /* Address comparator access types */
>> --
>> 2.25.1
>>
> Regards
> 
> Mike

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2023-01-21  7:32 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-20  0:51 [PATCH 0/3] Ampere Computing ETMv4.x Support Steve Clevenger
2023-01-20  0:51 ` [PATCH 1/3] coresight etm4x: Early clear TRCOSLAR.OSLK prior to TRCIDR1 read Steve Clevenger
2023-01-20 11:12   ` Suzuki K Poulose
2023-01-21  7:30     ` Steve Clevenger
2023-01-23 10:54       ` Suzuki K Poulose
2023-01-23 17:22         ` Steve Clevenger
2023-01-23 17:33           ` Suzuki K Poulose
2023-01-23 19:48             ` Steve Clevenger
2023-01-23 22:18               ` Suzuki K Poulose
2023-01-23 22:51                 ` Suzuki K Poulose
2023-02-02  5:20                   ` Steve Clevenger
2023-02-02 11:16                     ` Suzuki K Poulose
2023-02-02 17:12                       ` Steve Clevenger
2023-02-02 17:27                         ` Suzuki K Poulose
2023-02-02 23:03                           ` Steve Clevenger
2023-03-01 10:01                             ` Suzuki K Poulose
2023-01-20 11:45   ` Mike Leach
2023-01-21  7:31     ` Steve Clevenger [this message]
2023-01-23 10:54       ` Mike Leach
2023-01-23 19:47         ` Steve Clevenger
2023-01-23 22:49           ` Suzuki K Poulose
2023-01-20  0:51 ` [PATCH 2/3] coresight etm4x: Add 32-bit read/write option to split 64-bit words Steve Clevenger
2023-01-20 11:19   ` Suzuki K Poulose
2023-01-22  8:32     ` Steve Clevenger
2023-01-23 17:58       ` Suzuki K Poulose
2023-03-06 10:37     ` James Clark
2023-03-07  1:24       ` Steve Clevenger
2023-01-20  0:51 ` [PATCH 3/3] coresight etm4x: Add pr_debug statement for Coresight component PID/CID Steve Clevenger
2023-01-20 11:23   ` Suzuki K Poulose
2023-01-20 12:40     ` Russell King (Oracle)
2023-01-21  7:31       ` Steve Clevenger

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=679cae64-5ecf-aedf-aebe-1309b4482f68@os.amperecomputing.com \
    --to=scclevenger@os.amperecomputing.com \
    --cc=coresight@lists.linaro.org \
    --cc=leo.yan@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=mathieu.poirier@linaro.org \
    --cc=mike.leach@linaro.org \
    --cc=suzuki.poulose@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.