| Message ID | 2e25b4a0366c9f792730897f2ab76cc045ce6403.1674174972.git.scclevenger@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Ampere Computing ETMv4.x Support | expand |
Hi Steve Thanks for the patches. Have a few comments below. On 20/01/2023 00:51, Steve Clevenger 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 Please could you add this erratum to the : Documentation/arm64/silicon-errata.rst ? Given the ETM is v4.6, doesn't it support system instructions and that is causing this issue of "MMIO access is considered external" ? If it does, I think we should drop all of this and simply wire the system instruction access support. > > 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; > > + /* 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)) { > + if (drvdata->mmio_external) > + etm4_os_lock(drvdata); Couldn't this unlock/lock sequence be moved into the etm4_init_csdev_iomem_access() where it actually matters ? Or thinking more about it, we could actually move the unlock step early for all ETMs irrespective of whether they are affected by this erratum. Of course, putting this back, if we fail to detect the ETM properly. I don't see any issue with that. > 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) > + */ Does it affect all Ampere ETMs ? You seem to be ignoring the PDIR1.PART_1, PDIR0_PART_0 fields, which happens to be 0. > + if ((init_arg.pid & 0x000FF000) == 0x00096000) > + drvdata->mmio_external = true; Like I said, we may be able to get rid of this flag and do the step for all ETMs. But before all of that, I would like to see if this is problem because we are skipping the system instruction route. Suzuki > /* > * 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); > + bool mmio_external; > }; > > /* Address comparator access types */
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? > + /* 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 > + 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? > + bool mmio_external; > }; > > /* Address comparator access types */ > -- > 2.25.1 > Regards Mike
Hi Suzuki, Comments in-line. Please note the approach I attempted while adding in the Ampere support was to otherwise not disturb existing driver code for non-Ampere parts. Steve On 1/20/2023 3:12 AM, Suzuki K Poulose wrote: > Hi Steve > > Thanks for the patches. Have a few comments below. > > On 20/01/2023 00:51, Steve Clevenger 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 > > Please could you add this erratum to the : > > Documentation/arm64/silicon-errata.rst ? > > Given the ETM is v4.6, doesn't it support system instructions and > that is causing this issue of "MMIO access is considered external" ? > If it does, I think we should drop all of this and simply wire the > system instruction access support. That's not the issue in this case. This MMIO access should've been allowed by the Ampere ETMv4.6 implementation. Based on comments I've read in the driver code, the MMIO read access to TRCIDR1 occurs after a TRCDEVARCH access. The comments suggest this was to accommodate potentially unreliable TRCDEVARCH (and TRCIDR1) values. This Ampere ETMv4.6 allows an MMIO access to TRCDEVARCH, but not to TRCIDR1 unless the TRCOSLAR.OSLK lock is cleared first. > >> >> 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; >> + /* 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)) { >> + if (drvdata->mmio_external) >> + etm4_os_lock(drvdata); > > Couldn't this unlock/lock sequence be moved into the > etm4_init_csdev_iomem_access() where it actually matters ? > > Or thinking more about it, we could actually move the unlock step early > for all ETMs irrespective of whether they are affected by this erratum. > Of course, putting this back, if we fail to detect the ETM properly. > I don't see any issue with that. I agree the lock could be cleared earlier in the code. That's what this patch does for Ampere. If it's decided ok to do for other (or all) manufacturers, then the Ampere specific ID check goes away in this place. The Ampere ID check (and flag) to determine whether the [Patch 2/3] 64-bit access is split into 2 ea. 32-bit accesses would remain, or use an existing feature mask as suggested by Mike Leach in a later review. > >> 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) >> + */ > > Does it affect all Ampere ETMs ? You seem to be ignoring the > PDIR1.PART_1, PDIR0_PART_0 fields, which happens to be 0. This is the first Ampere ETMv4.x implementation. I wrote the ID check like this specifically because Ampere does not intend to address this for ETM designs in progress. > >> + if ((init_arg.pid & 0x000FF000) == 0x00096000) >> + drvdata->mmio_external = true; > Like I said, we may be able to get rid of this flag and do the step for > all ETMs. But before all of that, I would like to see if this is problem > because we are skipping the system instruction route. > We understand MMIO access is deprecated going forward. There is other Linux code to be concerned about. For example, AMBA code reads the component PID/CID. This discovery code uses mapped values digested from the CoreSight ACPI which are the descriptions and graphs for the manufacturer trace implementation. There may be other Linux code I'm not aware. Note the ASL examples in ARM Document number: DEN0067 specify MMIO locations for every CoreSight component. > Suzuki > >> /* >> * 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); >> + bool mmio_external; >> }; >> /* Address comparator access types */ >
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
On 21/01/2023 07:30, Steve Clevenger wrote: > > Hi Suzuki, > > Comments in-line. Please note the approach I attempted while adding in > the Ampere support was to otherwise not disturb existing driver code for > non-Ampere parts. > > Steve > > On 1/20/2023 3:12 AM, Suzuki K Poulose wrote: >> Hi Steve >> >> Thanks for the patches. Have a few comments below. >> >> On 20/01/2023 00:51, Steve Clevenger 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 >> >> Please could you add this erratum to the : >> >> Documentation/arm64/silicon-errata.rst ? >> >> Given the ETM is v4.6, doesn't it support system instructions and >> that is causing this issue of "MMIO access is considered external" ? >> If it does, I think we should drop all of this and simply wire the >> system instruction access support. > That's not the issue in this case. This MMIO access should've been > allowed by the Ampere ETMv4.6 implementation. Based on comments I've That doesn't answe the question. Please could you confirm the value of ID_AA64DFR0_EL1 on your system ? Or, are you able to try this on your ACPI based system and see if you are able to use the etm ? (UNTESTED hack !) diff --git a/drivers/acpi/acpi_amba.c b/drivers/acpi/acpi_amba.c index f5b443ab01c2..099966cbac5a 100644 --- a/drivers/acpi/acpi_amba.c +++ b/drivers/acpi/acpi_amba.c @@ -22,7 +22,6 @@ static const struct acpi_device_id amba_id_list[] = { {"ARMH0061", 0}, /* PL061 GPIO Device */ {"ARMH0330", 0}, /* ARM DMA Controller DMA-330 */ - {"ARMHC500", 0}, /* ARM CoreSight ETM4x */ {"ARMHC501", 0}, /* ARM CoreSight ETR */ {"ARMHC502", 0}, /* ARM CoreSight STM */ {"ARMHC503", 0}, /* ARM CoreSight Debug */ diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 1ea8f173cca0..66670533fd54 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -3,6 +3,7 @@ * Copyright (c) 2014, The Linux Foundation. All rights reserved. */ +#include <linux/acpi.h> #include <linux/bitops.h> #include <linux/kernel.h> #include <linux/moduleparam.h> @@ -2286,12 +2287,22 @@ static const struct of_device_id etm4_sysreg_match[] = { {} }; +#ifdef CONFIG_ACPI +static const struct acpi_device_id etm4x_acpi_ids[] = { + {"ARMHC500", 0}, /* ARM CoreSight ETM4x */ + {} +}; + +MODULE_DEVICE_TABLE(acpi, etm4x_acpi_ids); +#endif + static struct platform_driver etm4_platform_driver = { .probe = etm4_probe_platform_dev, .remove = etm4_remove_platform_dev, .driver = { .name = "coresight-etm4x", .of_match_table = etm4_sysreg_match, + .acpi_match_table = ACPI_PTR(etm4x_acpi_ids), .suppress_bind_attrs = true, }, }; > read in the driver code, the MMIO read access to TRCIDR1 occurs after a > TRCDEVARCH access. The comments suggest this was to accommodate > potentially unreliable TRCDEVARCH (and TRCIDR1) values. This Ampere > ETMv4.6 allows an MMIO access to TRCDEVARCH, but not to TRCIDR1 unless > the TRCOSLAR.OSLK lock is cleared first. > >> >>> >>> 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; >>> + /* 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)) { >>> + if (drvdata->mmio_external) >>> + etm4_os_lock(drvdata); >> >> Couldn't this unlock/lock sequence be moved into the >> etm4_init_csdev_iomem_access() where it actually matters ? >> >> Or thinking more about it, we could actually move the unlock step early >> for all ETMs irrespective of whether they are affected by this erratum. >> Of course, putting this back, if we fail to detect the ETM properly. >> I don't see any issue with that. > I agree the lock could be cleared earlier in the code. That's what this > patch does for Ampere. If it's decided ok to do for other (or all) > manufacturers, then the Ampere specific ID check goes away in this > place. The Ampere ID check (and flag) to determine whether the [Patch > 2/3] 64-bit access is split into 2 ea. 32-bit accesses would remain, or > use an existing feature mask as suggested by Mike Leach in a later review. > >> >>> 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) >>> + */ >> >> Does it affect all Ampere ETMs ? You seem to be ignoring the >> PDIR1.PART_1, PDIR0_PART_0 fields, which happens to be 0. > This is the first Ampere ETMv4.x implementation. I wrote the ID check > like this specifically because Ampere does not intend to address this > for ETM designs in progress. I would recommend to make this mask stricter and apply this to the current implementation. When there are more, we could add this here, rather than having to leave this work around for all the possible cores. > >> >>> + if ((init_arg.pid & 0x000FF000) == 0x00096000) >>> + drvdata->mmio_external = true; >> Like I said, we may be able to get rid of this flag and do the step for >> all ETMs. But before all of that, I would like to see if this is problem >> because we are skipping the system instruction route. >> > We understand MMIO access is deprecated going forward. There is other > Linux code to be concerned about. For example, AMBA code reads the > component PID/CID. This discovery code uses mapped values digested from > the CoreSight ACPI which are the descriptions and graphs for the With the "proposed" ACPI support for system register, AMBA would not be involved at all. > manufacturer trace implementation. There may be other Linux code I'm not > aware. Note the ASL examples in ARM Document number: DEN0067 specify > MMIO locations for every CoreSight component. Yes, but this was never updated to cover the system register based implementations. I will chase this up. Suzuki
Hi Steve, On Sat, 21 Jan 2023 at 07:31, Steve Clevenger <scclevenger@os.amperecomputing.com> wrote: > > > 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. > Sorry, I should have been more explicit. csa is the addressing abstraction used by all the underlying register read/write code. It is initialised to {0} in the calling code, probably to avoid the kernel tests complaining about uninitialised use of a variable. However in the etm4_init_csdev_access() function we are using a base address then it is initialised to:- struct csdev_access { io_mem = true; *base = io_mem_base_addr; }; and in the access using system registers for an etm4 to: struct csdev_access { io_mem = false; *read = etm4x_sysreg_read() *write = etm4x_sysreg_write() }; Thus all underlying register access can use the correct method for the device. > > > >> + /* Detect the support for OS Lock before we actually use it */ > >> + etm_detect_os_lock(drvdata, csa); > >> + Thus passing a 0 init csa object to the etm_detect_os_lock() fn above seems to be suspicious. > >> + /* > >> + * 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. Not so - csa is initialised in both circumstances as described above. > Under what > circumstance would there be no ETM base given the recommended CoreSight > ACPI implementation? See the examples in ARM Document number: DEN0067. This will be used in the ETE devices (which share the etm4 driver), or any ETM4.6+ that uses the "arm,coresight-etm4x-sysreg" device tree binding (not sure what the ACPI equivalent is). So, either way, you need an init csa, before passing it to the driver calls. Later in the initialisation sequence we generate a coresight_device object which the csa is bound to, and finally if all is well the coresight_device is bound to drvdata at which point the device is ready for use. It is unfortunate, but to handle the two methods of register access, the initilialisation process for the driver has become more complicated with ordering dependencies - to ensure that the rest of the driver remains simpler when accessing device registers. As Suzuki mentioned - moving this specific lock requirement into the _init function would be clearer and ensure that the initialisation sequences were observed. Regards Mike > > > > > > > >> + 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
On 1/23/2023 2:54 AM, Suzuki K Poulose wrote: > On 21/01/2023 07:30, Steve Clevenger wrote: >> >> Hi Suzuki, >> >> Comments in-line. Please note the approach I attempted while adding in >> the Ampere support was to otherwise not disturb existing driver code for >> non-Ampere parts. >> >> Steve >> >> On 1/20/2023 3:12 AM, Suzuki K Poulose wrote: >>> Hi Steve >>> >>> Thanks for the patches. Have a few comments below. >>> >>> On 20/01/2023 00:51, Steve Clevenger 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 >>> >>> Please could you add this erratum to the : >>> >>> Documentation/arm64/silicon-errata.rst ? >>> >>> Given the ETM is v4.6, doesn't it support system instructions and >>> that is causing this issue of "MMIO access is considered external" ? >>> If it does, I think we should drop all of this and simply wire the >>> system instruction access support. > >> That's not the issue in this case. This MMIO access should've been >> allowed by the Ampere ETMv4.6 implementation. Based on comments I've > > That doesn't answe the question. Please could you confirm the value of > ID_AA64DFR0_EL1 on your system ? This ID_AA64DFR0_EL1 value came from a TRACE32 debug session connected to this part. The ID_AA64DFR0_EL1 value is 0x000F01F210307619. So, TraceVer, bits [7:4] are b0001. My understanding is the system register interface must be implemented on all ETMv4.6 parts. > > Or, are you able to try this on your ACPI based system and see if you > are able to use the etm ? (UNTESTED hack !) > > > diff --git a/drivers/acpi/acpi_amba.c b/drivers/acpi/acpi_amba.c > index f5b443ab01c2..099966cbac5a 100644 > --- a/drivers/acpi/acpi_amba.c > +++ b/drivers/acpi/acpi_amba.c > @@ -22,7 +22,6 @@ > static const struct acpi_device_id amba_id_list[] = { > {"ARMH0061", 0}, /* PL061 GPIO Device */ > {"ARMH0330", 0}, /* ARM DMA Controller DMA-330 */ > - {"ARMHC500", 0}, /* ARM CoreSight ETM4x */ > {"ARMHC501", 0}, /* ARM CoreSight ETR */ > {"ARMHC502", 0}, /* ARM CoreSight STM */ > {"ARMHC503", 0}, /* ARM CoreSight Debug */ > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c > b/drivers/hwtracing/coresight/coresight-etm4x-core.c > index 1ea8f173cca0..66670533fd54 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > @@ -3,6 +3,7 @@ > * Copyright (c) 2014, The Linux Foundation. All rights reserved. > */ > > +#include <linux/acpi.h> > #include <linux/bitops.h> > #include <linux/kernel.h> > #include <linux/moduleparam.h> > @@ -2286,12 +2287,22 @@ static const struct of_device_id > etm4_sysreg_match[] = { > {} > }; > > +#ifdef CONFIG_ACPI > +static const struct acpi_device_id etm4x_acpi_ids[] = { > + {"ARMHC500", 0}, /* ARM CoreSight ETM4x */ > + {} > +}; > + > +MODULE_DEVICE_TABLE(acpi, etm4x_acpi_ids); > +#endif > + > static struct platform_driver etm4_platform_driver = { > .probe = etm4_probe_platform_dev, > .remove = etm4_remove_platform_dev, > .driver = { > .name = "coresight-etm4x", > .of_match_table = etm4_sysreg_match, > + .acpi_match_table = ACPI_PTR(etm4x_acpi_ids), > .suppress_bind_attrs = true, > }, > }; > > > > >> read in the driver code, the MMIO read access to TRCIDR1 occurs after a >> TRCDEVARCH access. The comments suggest this was to accommodate >> potentially unreliable TRCDEVARCH (and TRCIDR1) values. This Ampere >> ETMv4.6 allows an MMIO access to TRCDEVARCH, but not to TRCIDR1 unless >> the TRCOSLAR.OSLK lock is cleared first. >> >>> >>>> >>>> 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; >>>> + /* 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)) { >>>> + if (drvdata->mmio_external) >>>> + etm4_os_lock(drvdata); >>> >>> Couldn't this unlock/lock sequence be moved into the >>> etm4_init_csdev_iomem_access() where it actually matters ? >>> >>> Or thinking more about it, we could actually move the unlock step early >>> for all ETMs irrespective of whether they are affected by this erratum. >>> Of course, putting this back, if we fail to detect the ETM properly. >>> I don't see any issue with that. > > >> I agree the lock could be cleared earlier in the code. That's what this >> patch does for Ampere. If it's decided ok to do for other (or all) >> manufacturers, then the Ampere specific ID check goes away in this >> place. The Ampere ID check (and flag) to determine whether the [Patch >> 2/3] 64-bit access is split into 2 ea. 32-bit accesses would remain, or >> use an existing feature mask as suggested by Mike Leach in a later >> review. >> >>> >>>> 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) >>>> + */ >>> >>> Does it affect all Ampere ETMs ? You seem to be ignoring the >>> PDIR1.PART_1, PDIR0_PART_0 fields, which happens to be 0. > >> This is the first Ampere ETMv4.x implementation. I wrote the ID check >> like this specifically because Ampere does not intend to address this >> for ETM designs in progress. > > I would recommend to make this mask stricter and apply this to the > current implementation. When there are more, we could add this here, > rather than having to leave this work around for all the possible cores. > >> >>> >>>> + if ((init_arg.pid & 0x000FF000) == 0x00096000) >>>> + drvdata->mmio_external = true; >>> Like I said, we may be able to get rid of this flag and do the step for >>> all ETMs. But before all of that, I would like to see if this is problem >>> because we are skipping the system instruction route. >>> > >> We understand MMIO access is deprecated going forward. There is other >> Linux code to be concerned about. For example, AMBA code reads the >> component PID/CID. This discovery code uses mapped values digested from >> the CoreSight ACPI which are the descriptions and graphs for the > > With the "proposed" ACPI support for system register, AMBA would not be > involved at all. > >> manufacturer trace implementation. There may be other Linux code I'm not >> aware. Note the ASL examples in ARM Document number: DEN0067 specify >> MMIO locations for every CoreSight component. > > Yes, but this was never updated to cover the system register based > implementations. I will chase this up. > > > Suzuki >
On 23/01/2023 17:22, Steve Clevenger wrote: > > > On 1/23/2023 2:54 AM, Suzuki K Poulose wrote: >> On 21/01/2023 07:30, Steve Clevenger wrote: >>> >>> Hi Suzuki, >>> >>> Comments in-line. Please note the approach I attempted while adding in >>> the Ampere support was to otherwise not disturb existing driver code for >>> non-Ampere parts. >>> >>> Steve >>> >>> On 1/20/2023 3:12 AM, Suzuki K Poulose wrote: >>>> Hi Steve >>>> >>>> Thanks for the patches. Have a few comments below. >>>> >>>> On 20/01/2023 00:51, Steve Clevenger 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 >>>> >>>> Please could you add this erratum to the : >>>> >>>> Documentation/arm64/silicon-errata.rst ? >>>> >>>> Given the ETM is v4.6, doesn't it support system instructions and >>>> that is causing this issue of "MMIO access is considered external" ? >>>> If it does, I think we should drop all of this and simply wire the >>>> system instruction access support. >> >>> That's not the issue in this case. This MMIO access should've been >>> allowed by the Ampere ETMv4.6 implementation. Based on comments I've >> >> That doesn't answe the question. Please could you confirm the value of >> ID_AA64DFR0_EL1 on your system ? > This ID_AA64DFR0_EL1 value came from a TRACE32 debug session connected > to this part. The ID_AA64DFR0_EL1 value is 0x000F01F210307619. So, > TraceVer, bits [7:4] are b0001. My understanding is the system register > interface must be implemented on all ETMv4.6 parts. So, I don't understand why we are pushing towards enabling the "obsolete" MMIO interface ? Is this because "ACPI" mandates it ? Then, please don't. The spec needs an update to reflect the ETMs with sysreg access and ETEs. Why not stick to the system register access* ? * PS: The ACPI support for the ETM/ETE needs additional changes to the CoreSight driver, *not* the CoreSight ACPI spec. @Anshuman is working on this at the moment and will be available soon. The hack patch below should be sufficient to give it a try and if it works. Kind regards Suzuki > >> >> Or, are you able to try this on your ACPI based system and see if you >> are able to use the etm ? (UNTESTED hack !) >> >> >> diff --git a/drivers/acpi/acpi_amba.c b/drivers/acpi/acpi_amba.c >> index f5b443ab01c2..099966cbac5a 100644 >> --- a/drivers/acpi/acpi_amba.c >> +++ b/drivers/acpi/acpi_amba.c >> @@ -22,7 +22,6 @@ >> static const struct acpi_device_id amba_id_list[] = { >> {"ARMH0061", 0}, /* PL061 GPIO Device */ >> {"ARMH0330", 0}, /* ARM DMA Controller DMA-330 */ >> - {"ARMHC500", 0}, /* ARM CoreSight ETM4x */ >> {"ARMHC501", 0}, /* ARM CoreSight ETR */ >> {"ARMHC502", 0}, /* ARM CoreSight STM */ >> {"ARMHC503", 0}, /* ARM CoreSight Debug */ >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c >> b/drivers/hwtracing/coresight/coresight-etm4x-core.c >> index 1ea8f173cca0..66670533fd54 100644 >> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c >> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c >> @@ -3,6 +3,7 @@ >> * Copyright (c) 2014, The Linux Foundation. All rights reserved. >> */ >> >> +#include <linux/acpi.h> >> #include <linux/bitops.h> >> #include <linux/kernel.h> >> #include <linux/moduleparam.h> >> @@ -2286,12 +2287,22 @@ static const struct of_device_id >> etm4_sysreg_match[] = { >> {} >> }; >> >> +#ifdef CONFIG_ACPI >> +static const struct acpi_device_id etm4x_acpi_ids[] = { >> + {"ARMHC500", 0}, /* ARM CoreSight ETM4x */ >> + {} >> +}; >> + >> +MODULE_DEVICE_TABLE(acpi, etm4x_acpi_ids); >> +#endif >> + >> static struct platform_driver etm4_platform_driver = { >> .probe = etm4_probe_platform_dev, >> .remove = etm4_remove_platform_dev, >> .driver = { >> .name = "coresight-etm4x", >> .of_match_table = etm4_sysreg_match, >> + .acpi_match_table = ACPI_PTR(etm4x_acpi_ids), >> .suppress_bind_attrs = true, >> }, >> }; >> >> >> >> >>> read in the driver code, the MMIO read access to TRCIDR1 occurs after a >>> TRCDEVARCH access. The comments suggest this was to accommodate >>> potentially unreliable TRCDEVARCH (and TRCIDR1) values. This Ampere >>> ETMv4.6 allows an MMIO access to TRCDEVARCH, but not to TRCIDR1 unless >>> the TRCOSLAR.OSLK lock is cleared first. >>> >>>> >>>>> >>>>> 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; >>>>> + /* 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)) { >>>>> + if (drvdata->mmio_external) >>>>> + etm4_os_lock(drvdata); >>>> >>>> Couldn't this unlock/lock sequence be moved into the >>>> etm4_init_csdev_iomem_access() where it actually matters ? >>>> >>>> Or thinking more about it, we could actually move the unlock step early >>>> for all ETMs irrespective of whether they are affected by this erratum. >>>> Of course, putting this back, if we fail to detect the ETM properly. >>>> I don't see any issue with that. >> >> >>> I agree the lock could be cleared earlier in the code. That's what this >>> patch does for Ampere. If it's decided ok to do for other (or all) >>> manufacturers, then the Ampere specific ID check goes away in this >>> place. The Ampere ID check (and flag) to determine whether the [Patch >>> 2/3] 64-bit access is split into 2 ea. 32-bit accesses would remain, or >>> use an existing feature mask as suggested by Mike Leach in a later >>> review. >>> >>>> >>>>> 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) >>>>> + */ >>>> >>>> Does it affect all Ampere ETMs ? You seem to be ignoring the >>>> PDIR1.PART_1, PDIR0_PART_0 fields, which happens to be 0. >> >>> This is the first Ampere ETMv4.x implementation. I wrote the ID check >>> like this specifically because Ampere does not intend to address this >>> for ETM designs in progress. >> >> I would recommend to make this mask stricter and apply this to the >> current implementation. When there are more, we could add this here, >> rather than having to leave this work around for all the possible cores. >> >>> >>>> >>>>> + if ((init_arg.pid & 0x000FF000) == 0x00096000) >>>>> + drvdata->mmio_external = true; >>>> Like I said, we may be able to get rid of this flag and do the step for >>>> all ETMs. But before all of that, I would like to see if this is problem >>>> because we are skipping the system instruction route. >>>> >> >>> We understand MMIO access is deprecated going forward. There is other >>> Linux code to be concerned about. For example, AMBA code reads the >>> component PID/CID. This discovery code uses mapped values digested from >>> the CoreSight ACPI which are the descriptions and graphs for the >> >> With the "proposed" ACPI support for system register, AMBA would not be >> involved at all. >> >>> manufacturer trace implementation. There may be other Linux code I'm not >>> aware. Note the ASL examples in ARM Document number: DEN0067 specify >>> MMIO locations for every CoreSight component. >> >> Yes, but this was never updated to cover the system register based >> implementations. I will chase this up. >> >> >> Suzuki >>
On 1/23/2023 2:54 AM, Mike Leach wrote: > Hi Steve, > > On Sat, 21 Jan 2023 at 07:31, Steve Clevenger > <scclevenger@os.amperecomputing.com> wrote: >> >> >> 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. >> > > Sorry, I should have been more explicit. > > csa is the addressing abstraction used by all the underlying register > read/write code. > > It is initialised to {0} in the calling code, probably to avoid the > kernel tests complaining about uninitialised use of a variable. > > However in the etm4_init_csdev_access() function we are using a base > address then it is initialised to:- > > struct csdev_access { > io_mem = true; > *base = io_mem_base_addr; > }; > > and in the access using system registers for an etm4 to: > > struct csdev_access { > io_mem = false; > *read = etm4x_sysreg_read() > *write = etm4x_sysreg_write() > }; Yes, csa is initialized indirectly in etm4_init_csdev_access via calls to etm4_init_iomem_access and etm4_init_sysreg_access. So, you are correct. csa is zero initialized for the call to etm4_detect_os_lock (TRCOSLSR read) prior to my patch which clears TRCOSLAR.OSLK. The TRCOSLSR read and TRCOSLAR.OSLK clear default to sysreg access. The csa initialization problem is an existing bug I didn't see. > > Thus all underlying register access can use the correct method for the device. > >>> >>>> + /* Detect the support for OS Lock before we actually use it */ >>>> + etm_detect_os_lock(drvdata, csa); >>>> + > > Thus passing a 0 init csa object to the etm_detect_os_lock() fn above > seems to be suspicious. > >>>> + /* >>>> + * 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. > > Not so - csa is initialised in both circumstances as described above. > >> Under what >> circumstance would there be no ETM base given the recommended CoreSight >> ACPI implementation? See the examples in ARM Document number: DEN0067. > > > This will be used in the ETE devices (which share the etm4 driver), or > any ETM4.6+ that uses the "arm,coresight-etm4x-sysreg" device tree > binding (not sure what the ACPI equivalent is). > > So, either way, you need an init csa, before passing it to the driver calls. Agreed. See my summary above. > > Later in the initialisation sequence we generate a coresight_device > object which the csa is bound to, and finally if all is well the > coresight_device is bound to drvdata at which point the device is > ready for use. > > It is unfortunate, but to handle the two methods of register access, > the initilialisation process for the driver has become more > complicated with ordering dependencies - to ensure that the rest of > the driver remains simpler when accessing device registers. > > As Suzuki mentioned - moving this specific lock requirement into the > _init function would be clearer and ensure that the initialisation > sequences were observed. Getting back to the etm4_init_csdev_access implementation, if a drvdata->base exists, etm4_init_sysreg_access is never called. The driver doesn't initialize sysreg access if a memory map exists by design. The existing Coresight ACPI specification only has the option of describing the manufacturer trace implementation using descriptions of memory mapped components. > > Regards > > Mike > >>> >>> >>> >>>> + 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 > > >
On 1/23/2023 9:33 AM, Suzuki K Poulose wrote: > On 23/01/2023 17:22, Steve Clevenger wrote: >> >> >> On 1/23/2023 2:54 AM, Suzuki K Poulose wrote: >>> On 21/01/2023 07:30, Steve Clevenger wrote: >>>> >>>> Hi Suzuki, >>>> >>>> Comments in-line. Please note the approach I attempted while adding in >>>> the Ampere support was to otherwise not disturb existing driver code >>>> for >>>> non-Ampere parts. >>>> >>>> Steve >>>> >>>> On 1/20/2023 3:12 AM, Suzuki K Poulose wrote: >>>>> Hi Steve >>>>> >>>>> Thanks for the patches. Have a few comments below. >>>>> >>>>> On 20/01/2023 00:51, Steve Clevenger 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 >>>>> >>>>> Please could you add this erratum to the : >>>>> >>>>> Documentation/arm64/silicon-errata.rst ? >>>>> >>>>> Given the ETM is v4.6, doesn't it support system instructions and >>>>> that is causing this issue of "MMIO access is considered external" ? >>>>> If it does, I think we should drop all of this and simply wire the >>>>> system instruction access support. >>> >>>> That's not the issue in this case. This MMIO access should've been >>>> allowed by the Ampere ETMv4.6 implementation. Based on comments I've >>> >>> That doesn't answe the question. Please could you confirm the value of >>> ID_AA64DFR0_EL1 on your system ? >> This ID_AA64DFR0_EL1 value came from a TRACE32 debug session connected >> to this part. The ID_AA64DFR0_EL1 value is 0x000F01F210307619. So, >> TraceVer, bits [7:4] are b0001. My understanding is the system register >> interface must be implemented on all ETMv4.6 parts. > > So, I don't understand why we are pushing towards enabling the > "obsolete" MMIO interface ? Is this because "ACPI" mandates it ? > Then, please don't. The spec needs an update to reflect the ETMs > with sysreg access and ETEs. > > Why not stick to the system register access* ? > > * PS: The ACPI support for the ETM/ETE needs additional changes to the > CoreSight driver, *not* the CoreSight ACPI spec. @Anshuman is working on > this at the moment and will be available soon. > > The hack patch below should be sufficient to give it a try and if it works. I don't understand your postscript. Certainly there's driver work to be done, but I also think the DEN0067 CoreSight ACPI specification needs update. There's no example for defining a trace implementation without memory mapped component descriptions. Considering the ACPI as it exists, the component graphs are the most important. Please see my last exchange with Mike Leach. My patches were submitted based on the existing CoreSight driver, not what it should be. Ampere does not have the flexibility to wait for a decision on some details we've discussed. I'm available to work through any concerns the maintainers have with my submissions. What is the best way forward here? > Kind regards > Suzuki > >> >>> >>> Or, are you able to try this on your ACPI based system and see if you >>> are able to use the etm ? (UNTESTED hack !) >>> >>> >>> diff --git a/drivers/acpi/acpi_amba.c b/drivers/acpi/acpi_amba.c >>> index f5b443ab01c2..099966cbac5a 100644 >>> --- a/drivers/acpi/acpi_amba.c >>> +++ b/drivers/acpi/acpi_amba.c >>> @@ -22,7 +22,6 @@ >>> static const struct acpi_device_id amba_id_list[] = { >>> {"ARMH0061", 0}, /* PL061 GPIO Device */ >>> {"ARMH0330", 0}, /* ARM DMA Controller DMA-330 */ >>> - {"ARMHC500", 0}, /* ARM CoreSight ETM4x */ >>> {"ARMHC501", 0}, /* ARM CoreSight ETR */ >>> {"ARMHC502", 0}, /* ARM CoreSight STM */ >>> {"ARMHC503", 0}, /* ARM CoreSight Debug */ >>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c >>> b/drivers/hwtracing/coresight/coresight-etm4x-core.c >>> index 1ea8f173cca0..66670533fd54 100644 >>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c >>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c >>> @@ -3,6 +3,7 @@ >>> * Copyright (c) 2014, The Linux Foundation. All rights reserved. >>> */ >>> >>> +#include <linux/acpi.h> >>> #include <linux/bitops.h> >>> #include <linux/kernel.h> >>> #include <linux/moduleparam.h> >>> @@ -2286,12 +2287,22 @@ static const struct of_device_id >>> etm4_sysreg_match[] = { >>> {} >>> }; >>> >>> +#ifdef CONFIG_ACPI >>> +static const struct acpi_device_id etm4x_acpi_ids[] = { >>> + {"ARMHC500", 0}, /* ARM CoreSight ETM4x */ >>> + {} >>> +}; >>> + >>> +MODULE_DEVICE_TABLE(acpi, etm4x_acpi_ids); >>> +#endif >>> + >>> static struct platform_driver etm4_platform_driver = { >>> .probe = etm4_probe_platform_dev, >>> .remove = etm4_remove_platform_dev, >>> .driver = { >>> .name = "coresight-etm4x", >>> .of_match_table = etm4_sysreg_match, >>> + .acpi_match_table = ACPI_PTR(etm4x_acpi_ids), >>> .suppress_bind_attrs = true, >>> }, >>> }; >>> >>> >>> >>> >>>> read in the driver code, the MMIO read access to TRCIDR1 occurs after a >>>> TRCDEVARCH access. The comments suggest this was to accommodate >>>> potentially unreliable TRCDEVARCH (and TRCIDR1) values. This Ampere >>>> ETMv4.6 allows an MMIO access to TRCDEVARCH, but not to TRCIDR1 unless >>>> the TRCOSLAR.OSLK lock is cleared first. >>>> >>>>> >>>>>> >>>>>> 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; >>>>>> + /* 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)) { >>>>>> + if (drvdata->mmio_external) >>>>>> + etm4_os_lock(drvdata); >>>>> >>>>> Couldn't this unlock/lock sequence be moved into the >>>>> etm4_init_csdev_iomem_access() where it actually matters ? >>>>> >>>>> Or thinking more about it, we could actually move the unlock step >>>>> early >>>>> for all ETMs irrespective of whether they are affected by this >>>>> erratum. >>>>> Of course, putting this back, if we fail to detect the ETM properly. >>>>> I don't see any issue with that. >>> >>> >>>> I agree the lock could be cleared earlier in the code. That's what this >>>> patch does for Ampere. If it's decided ok to do for other (or all) >>>> manufacturers, then the Ampere specific ID check goes away in this >>>> place. The Ampere ID check (and flag) to determine whether the [Patch >>>> 2/3] 64-bit access is split into 2 ea. 32-bit accesses would remain, or >>>> use an existing feature mask as suggested by Mike Leach in a later >>>> review. >>>> >>>>> >>>>>> 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) >>>>>> + */ >>>>> >>>>> Does it affect all Ampere ETMs ? You seem to be ignoring the >>>>> PDIR1.PART_1, PDIR0_PART_0 fields, which happens to be 0. >>> >>>> This is the first Ampere ETMv4.x implementation. I wrote the ID check >>>> like this specifically because Ampere does not intend to address this >>>> for ETM designs in progress. >>> >>> I would recommend to make this mask stricter and apply this to the >>> current implementation. When there are more, we could add this here, >>> rather than having to leave this work around for all the possible cores. >>> >>>> >>>>> >>>>>> + if ((init_arg.pid & 0x000FF000) == 0x00096000) >>>>>> + drvdata->mmio_external = true; >>>>> Like I said, we may be able to get rid of this flag and do the step >>>>> for >>>>> all ETMs. But before all of that, I would like to see if this is >>>>> problem >>>>> because we are skipping the system instruction route. >>>>> >>> >>>> We understand MMIO access is deprecated going forward. There is other >>>> Linux code to be concerned about. For example, AMBA code reads the >>>> component PID/CID. This discovery code uses mapped values digested from >>>> the CoreSight ACPI which are the descriptions and graphs for the >>> >>> With the "proposed" ACPI support for system register, AMBA would not be >>> involved at all. >>> >>>> manufacturer trace implementation. There may be other Linux code I'm >>>> not >>>> aware. Note the ASL examples in ARM Document number: DEN0067 specify >>>> MMIO locations for every CoreSight component. >>> >>> Yes, but this was never updated to cover the system register based >>> implementations. I will chase this up. >>> >>> >>> Suzuki >>> >
On 23/01/2023 19:48, Steve Clevenger wrote: > > > On 1/23/2023 9:33 AM, Suzuki K Poulose wrote: >> On 23/01/2023 17:22, Steve Clevenger wrote: >>> >>> >>> On 1/23/2023 2:54 AM, Suzuki K Poulose wrote: >>>> On 21/01/2023 07:30, Steve Clevenger wrote: >>>>> >>>>> Hi Suzuki, >>>>> >>>>> Comments in-line. Please note the approach I attempted while adding in >>>>> the Ampere support was to otherwise not disturb existing driver code >>>>> for >>>>> non-Ampere parts. >>>>> >>>>> Steve >>>>> >>>>> On 1/20/2023 3:12 AM, Suzuki K Poulose wrote: >>>>>> Hi Steve >>>>>> >>>>>> Thanks for the patches. Have a few comments below. >>>>>> >>>>>> On 20/01/2023 00:51, Steve Clevenger 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 >>>>>> >>>>>> Please could you add this erratum to the : >>>>>> >>>>>> Documentation/arm64/silicon-errata.rst ? >>>>>> >>>>>> Given the ETM is v4.6, doesn't it support system instructions and >>>>>> that is causing this issue of "MMIO access is considered external" ? >>>>>> If it does, I think we should drop all of this and simply wire the >>>>>> system instruction access support. >>>> >>>>> That's not the issue in this case. This MMIO access should've been >>>>> allowed by the Ampere ETMv4.6 implementation. Based on comments I've >>>> >>>> That doesn't answe the question. Please could you confirm the value of >>>> ID_AA64DFR0_EL1 on your system ? >>> This ID_AA64DFR0_EL1 value came from a TRACE32 debug session connected >>> to this part. The ID_AA64DFR0_EL1 value is 0x000F01F210307619. So, >>> TraceVer, bits [7:4] are b0001. My understanding is the system register >>> interface must be implemented on all ETMv4.6 parts. >> >> So, I don't understand why we are pushing towards enabling the >> "obsolete" MMIO interface ? Is this because "ACPI" mandates it ? >> Then, please don't. The spec needs an update to reflect the ETMs >> with sysreg access and ETEs. >> >> Why not stick to the system register access* ? >> >> * PS: The ACPI support for the ETM/ETE needs additional changes to the >> CoreSight driver, *not* the CoreSight ACPI spec. @Anshuman is working on >> this at the moment and will be available soon. >> >> The hack patch below should be sufficient to give it a try and if it works. > I don't understand your postscript. Certainly there's driver work to be > done, but I also think the DEN0067 CoreSight ACPI specification needs The issue is having a single HID for ETMs (which from a spec point of view makes sense for) with and without MMIO access. That brings two different components in Linux (AMBA hook for ACPI and a platform driver) compete for the said HID. There are other reasons to disconnect the CoreSight from AMBA framework and manage them directly [0]. So, that needs a bit of ground work to move ETM driver away from AMBA and then we can have a single driver handling both devices. It is much easier on DT based systems, as we have different compatible strings. > update. There's no example for defining a trace implementation without > memory mapped component descriptions. Considering the ACPI as it exists, I have raised this with the concerned people in Arm and can share the update via other channels once we have it. Basically we should be able to reuse the one for MMIO based systems, except the memory resource. The Graph connections remain unaffected by this change. This never made it to the document. > the component graphs are the most important. Please see my last exchange > with Mike Leach. > > My patches were submitted based on the existing CoreSight driver, not > what it should be. Ampere does not have the flexibility to wait for a > decision on some details we've discussed. I'm available to work through Understand. But that doesn't mean we can push something in that is not meant to work the way it should. > any concerns the maintainers have with my submissions. What is the best > way forward here? If this is a system with system instruction access, it should be described as such and can be supported when the above mentioned change is in the kernel. Suzuki > >> Kind regards >> Suzuki >> >>> >>>> >>>> Or, are you able to try this on your ACPI based system and see if you >>>> are able to use the etm ? (UNTESTED hack !) >>>> >>>> >>>> diff --git a/drivers/acpi/acpi_amba.c b/drivers/acpi/acpi_amba.c >>>> index f5b443ab01c2..099966cbac5a 100644 >>>> --- a/drivers/acpi/acpi_amba.c >>>> +++ b/drivers/acpi/acpi_amba.c >>>> @@ -22,7 +22,6 @@ >>>> static const struct acpi_device_id amba_id_list[] = { >>>> {"ARMH0061", 0}, /* PL061 GPIO Device */ >>>> {"ARMH0330", 0}, /* ARM DMA Controller DMA-330 */ >>>> - {"ARMHC500", 0}, /* ARM CoreSight ETM4x */ >>>> {"ARMHC501", 0}, /* ARM CoreSight ETR */ >>>> {"ARMHC502", 0}, /* ARM CoreSight STM */ >>>> {"ARMHC503", 0}, /* ARM CoreSight Debug */ >>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c >>>> b/drivers/hwtracing/coresight/coresight-etm4x-core.c >>>> index 1ea8f173cca0..66670533fd54 100644 >>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c >>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c >>>> @@ -3,6 +3,7 @@ >>>> * Copyright (c) 2014, The Linux Foundation. All rights reserved. >>>> */ >>>> >>>> +#include <linux/acpi.h> >>>> #include <linux/bitops.h> >>>> #include <linux/kernel.h> >>>> #include <linux/moduleparam.h> >>>> @@ -2286,12 +2287,22 @@ static const struct of_device_id >>>> etm4_sysreg_match[] = { >>>> {} >>>> }; >>>> >>>> +#ifdef CONFIG_ACPI >>>> +static const struct acpi_device_id etm4x_acpi_ids[] = { >>>> + {"ARMHC500", 0}, /* ARM CoreSight ETM4x */ >>>> + {} >>>> +}; >>>> + >>>> +MODULE_DEVICE_TABLE(acpi, etm4x_acpi_ids); >>>> +#endif >>>> + >>>> static struct platform_driver etm4_platform_driver = { >>>> .probe = etm4_probe_platform_dev, >>>> .remove = etm4_remove_platform_dev, >>>> .driver = { >>>> .name = "coresight-etm4x", >>>> .of_match_table = etm4_sysreg_match, >>>> + .acpi_match_table = ACPI_PTR(etm4x_acpi_ids), >>>> .suppress_bind_attrs = true, >>>> }, >>>> }; >>>> >>>> >>>> >>>> >>>>> read in the driver code, the MMIO read access to TRCIDR1 occurs after a >>>>> TRCDEVARCH access. The comments suggest this was to accommodate >>>>> potentially unreliable TRCDEVARCH (and TRCIDR1) values. This Ampere >>>>> ETMv4.6 allows an MMIO access to TRCDEVARCH, but not to TRCIDR1 unless >>>>> the TRCOSLAR.OSLK lock is cleared first. >>>>> >>>>>> >>>>>>> >>>>>>> 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; >>>>>>> + /* 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)) { >>>>>>> + if (drvdata->mmio_external) >>>>>>> + etm4_os_lock(drvdata); >>>>>> >>>>>> Couldn't this unlock/lock sequence be moved into the >>>>>> etm4_init_csdev_iomem_access() where it actually matters ? >>>>>> >>>>>> Or thinking more about it, we could actually move the unlock step >>>>>> early >>>>>> for all ETMs irrespective of whether they are affected by this >>>>>> erratum. >>>>>> Of course, putting this back, if we fail to detect the ETM properly. >>>>>> I don't see any issue with that. >>>> >>>> >>>>> I agree the lock could be cleared earlier in the code. That's what this >>>>> patch does for Ampere. If it's decided ok to do for other (or all) >>>>> manufacturers, then the Ampere specific ID check goes away in this >>>>> place. The Ampere ID check (and flag) to determine whether the [Patch >>>>> 2/3] 64-bit access is split into 2 ea. 32-bit accesses would remain, or >>>>> use an existing feature mask as suggested by Mike Leach in a later >>>>> review. >>>>> >>>>>> >>>>>>> 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) >>>>>>> + */ >>>>>> >>>>>> Does it affect all Ampere ETMs ? You seem to be ignoring the >>>>>> PDIR1.PART_1, PDIR0_PART_0 fields, which happens to be 0. >>>> >>>>> This is the first Ampere ETMv4.x implementation. I wrote the ID check >>>>> like this specifically because Ampere does not intend to address this >>>>> for ETM designs in progress. >>>> >>>> I would recommend to make this mask stricter and apply this to the >>>> current implementation. When there are more, we could add this here, >>>> rather than having to leave this work around for all the possible cores. >>>> >>>>> >>>>>> >>>>>>> + if ((init_arg.pid & 0x000FF000) == 0x00096000) >>>>>>> + drvdata->mmio_external = true; >>>>>> Like I said, we may be able to get rid of this flag and do the step >>>>>> for >>>>>> all ETMs. But before all of that, I would like to see if this is >>>>>> problem >>>>>> because we are skipping the system instruction route. >>>>>> >>>> >>>>> We understand MMIO access is deprecated going forward. There is other >>>>> Linux code to be concerned about. For example, AMBA code reads the >>>>> component PID/CID. This discovery code uses mapped values digested from >>>>> the CoreSight ACPI which are the descriptions and graphs for the >>>> >>>> With the "proposed" ACPI support for system register, AMBA would not be >>>> involved at all. >>>> >>>>> manufacturer trace implementation. There may be other Linux code I'm >>>>> not >>>>> aware. Note the ASL examples in ARM Document number: DEN0067 specify >>>>> MMIO locations for every CoreSight component. >>>> >>>> Yes, but this was never updated to cover the system register based >>>> implementations. I will chase this up. >>>> >>>> >>>> Suzuki >>>> >>
On 23/01/2023 19:47, Steve Clevenger wrote: > > > On 1/23/2023 2:54 AM, Mike Leach wrote: >> Hi Steve, >> >> On Sat, 21 Jan 2023 at 07:31, Steve Clevenger >> <scclevenger@os.amperecomputing.com> wrote: >>> >>> >>> 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. >>> >> >> Sorry, I should have been more explicit. >> >> csa is the addressing abstraction used by all the underlying register >> read/write code. >> >> It is initialised to {0} in the calling code, probably to avoid the >> kernel tests complaining about uninitialised use of a variable. >> >> However in the etm4_init_csdev_access() function we are using a base >> address then it is initialised to:- >> >> struct csdev_access { >> io_mem = true; >> *base = io_mem_base_addr; >> }; >> >> and in the access using system registers for an etm4 to: >> >> struct csdev_access { >> io_mem = false; >> *read = etm4x_sysreg_read() >> *write = etm4x_sysreg_write() >> }; > Yes, csa is initialized indirectly in etm4_init_csdev_access via calls > to etm4_init_iomem_access and etm4_init_sysreg_access. So, you are > correct. csa is zero initialized for the call to etm4_detect_os_lock > (TRCOSLSR read) prior to my patch which clears TRCOSLAR.OSLK. The > TRCOSLSR read and TRCOSLAR.OSLK clear default to sysreg access. The csa > initialization problem is an existing bug I didn't see. >> >> Thus all underlying register access can use the correct method for the device. >> >>>> >>>>> + /* Detect the support for OS Lock before we actually use it */ >>>>> + etm_detect_os_lock(drvdata, csa); >>>>> + >> >> Thus passing a 0 init csa object to the etm_detect_os_lock() fn above >> seems to be suspicious. >> >>>>> + /* >>>>> + * 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. >> >> Not so - csa is initialised in both circumstances as described above. >> >>> Under what >>> circumstance would there be no ETM base given the recommended CoreSight >>> ACPI implementation? See the examples in ARM Document number: DEN0067. >> >> >> This will be used in the ETE devices (which share the etm4 driver), or >> any ETM4.6+ that uses the "arm,coresight-etm4x-sysreg" device tree >> binding (not sure what the ACPI equivalent is). >> >> So, either way, you need an init csa, before passing it to the driver calls. > Agreed. See my summary above. > >> >> Later in the initialisation sequence we generate a coresight_device >> object which the csa is bound to, and finally if all is well the >> coresight_device is bound to drvdata at which point the device is >> ready for use. >> >> It is unfortunate, but to handle the two methods of register access, >> the initilialisation process for the driver has become more >> complicated with ordering dependencies - to ensure that the rest of >> the driver remains simpler when accessing device registers. >> >> As Suzuki mentioned - moving this specific lock requirement into the >> _init function would be clearer and ensure that the initialisation >> sequences were observed. > Getting back to the etm4_init_csdev_access implementation, if a > drvdata->base exists, etm4_init_sysreg_access is never called. The > driver doesn't initialize sysreg access if a memory map exists by > design. The existing Coresight ACPI specification only has the option of > describing the manufacturer trace implementation using descriptions of > memory mapped components. As mentioned in my previous comments, this is only a matter of updating the spec. You should be able to use everything same for the ETM, excluding the "MMIO" region. i.e, Device(CPU0) { Name(_HID, "ACPI0010"), ... Device(ETM0) { Name (_HID, "ARMHC500") // ETM Name (_CID, "ARMHC500") // ETM /* No _CRS Memory Resource for sysreg access */ /* Graph Connections as usual, if any ATB is connected */ Name (_DSD , Package () { ... }) ... } // ETM0 } // CPU0 Suzuki > >> >> Regards >> >> Mike >> >>>> >>>> >>>> >>>>> + 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 >> >> >>
Missed the reference, see below. On 23/01/2023 22:18, Suzuki K Poulose wrote: > On 23/01/2023 19:48, Steve Clevenger wrote: >> >> >> On 1/23/2023 9:33 AM, Suzuki K Poulose wrote: >>> On 23/01/2023 17:22, Steve Clevenger wrote: >>>> >>>> >>>> On 1/23/2023 2:54 AM, Suzuki K Poulose wrote: >>>>> On 21/01/2023 07:30, Steve Clevenger wrote: >>>>>> >>>>>> Hi Suzuki, >>>>>> >>>>>> Comments in-line. Please note the approach I attempted while >>>>>> adding in >>>>>> the Ampere support was to otherwise not disturb existing driver code >>>>>> for >>>>>> non-Ampere parts. >>>>>> >>>>>> Steve >>>>>> >>>>>> On 1/20/2023 3:12 AM, Suzuki K Poulose wrote: >>>>>>> Hi Steve >>>>>>> >>>>>>> Thanks for the patches. Have a few comments below. >>>>>>> >>>>>>> On 20/01/2023 00:51, Steve Clevenger 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 >>>>>>> >>>>>>> Please could you add this erratum to the : >>>>>>> >>>>>>> Documentation/arm64/silicon-errata.rst ? >>>>>>> >>>>>>> Given the ETM is v4.6, doesn't it support system instructions and >>>>>>> that is causing this issue of "MMIO access is considered external" ? >>>>>>> If it does, I think we should drop all of this and simply wire the >>>>>>> system instruction access support. >>>>> >>>>>> That's not the issue in this case. This MMIO access should've been >>>>>> allowed by the Ampere ETMv4.6 implementation. Based on comments I've >>>>> >>>>> That doesn't answe the question. Please could you confirm the value of >>>>> ID_AA64DFR0_EL1 on your system ? >>>> This ID_AA64DFR0_EL1 value came from a TRACE32 debug session connected >>>> to this part. The ID_AA64DFR0_EL1 value is 0x000F01F210307619. So, >>>> TraceVer, bits [7:4] are b0001. My understanding is the system register >>>> interface must be implemented on all ETMv4.6 parts. >>> >>> So, I don't understand why we are pushing towards enabling the >>> "obsolete" MMIO interface ? Is this because "ACPI" mandates it ? >>> Then, please don't. The spec needs an update to reflect the ETMs >>> with sysreg access and ETEs. >>> >>> Why not stick to the system register access* ? >>> >>> * PS: The ACPI support for the ETM/ETE needs additional changes to the >>> CoreSight driver, *not* the CoreSight ACPI spec. @Anshuman is working on >>> this at the moment and will be available soon. >>> >>> The hack patch below should be sufficient to give it a try and if it >>> works. > >> I don't understand your postscript. Certainly there's driver work to be >> done, but I also think the DEN0067 CoreSight ACPI specification needs > > The issue is having a single HID for ETMs (which from a spec point of > view makes sense for) with and without MMIO access. That brings two > different components in Linux (AMBA hook for ACPI and a platform driver) > compete for the said HID. There are other reasons to disconnect the > CoreSight from AMBA framework and manage them directly [0]. [0] https://lkml.kernel.org/r/e37e12ab-9701-2883-724a-2a281ad35df2@arm.com
Hi Suzuki, I've split out the bug fix (i.e. nr_addr_cmp use) to a separate patch and added references to the Ampere erratum in silicon-errata.rst. These will be submitted as separate patches. The ETM4.x TRCOSLAR.OSLK early clear has moved to etm4_init_csdev_iomem for all manufacturers. I think this is what you asked for. The no_quad_mmio flag has moved to struct csdev_access, and the split 64-bit read/write logic has been implemented entirely in the header file coresight-etm4x.h is the existing calls. I'd like to retire this patch thread, and submit these as a new thread. Is there an objection? Thanks, Steve On 1/23/2023 2:51 PM, Suzuki K Poulose wrote: > > Missed the reference, see below. > > On 23/01/2023 22:18, Suzuki K Poulose wrote: >> On 23/01/2023 19:48, Steve Clevenger wrote: >>> >>> >>> On 1/23/2023 9:33 AM, Suzuki K Poulose wrote: >>>> On 23/01/2023 17:22, Steve Clevenger wrote: >>>>> >>>>> >>>>> On 1/23/2023 2:54 AM, Suzuki K Poulose wrote: >>>>>> On 21/01/2023 07:30, Steve Clevenger wrote: >>>>>>> >>>>>>> Hi Suzuki, >>>>>>> >>>>>>> Comments in-line. Please note the approach I attempted while >>>>>>> adding in >>>>>>> the Ampere support was to otherwise not disturb existing driver code >>>>>>> for >>>>>>> non-Ampere parts. >>>>>>> >>>>>>> Steve >>>>>>> >>>>>>> On 1/20/2023 3:12 AM, Suzuki K Poulose wrote: >>>>>>>> Hi Steve >>>>>>>> >>>>>>>> Thanks for the patches. Have a few comments below. >>>>>>>> >>>>>>>> On 20/01/2023 00:51, Steve Clevenger 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 >>>>>>>> >>>>>>>> Please could you add this erratum to the : >>>>>>>> >>>>>>>> Documentation/arm64/silicon-errata.rst ? >>>>>>>> >>>>>>>> Given the ETM is v4.6, doesn't it support system instructions and >>>>>>>> that is causing this issue of "MMIO access is considered >>>>>>>> external" ? >>>>>>>> If it does, I think we should drop all of this and simply wire the >>>>>>>> system instruction access support. >>>>>> >>>>>>> That's not the issue in this case. This MMIO access should've been >>>>>>> allowed by the Ampere ETMv4.6 implementation. Based on comments >>>>>>> I've >>>>>> >>>>>> That doesn't answe the question. Please could you confirm the >>>>>> value of >>>>>> ID_AA64DFR0_EL1 on your system ? >>>>> This ID_AA64DFR0_EL1 value came from a TRACE32 debug session connected >>>>> to this part. The ID_AA64DFR0_EL1 value is 0x000F01F210307619. So, >>>>> TraceVer, bits [7:4] are b0001. My understanding is the system >>>>> register >>>>> interface must be implemented on all ETMv4.6 parts. >>>> >>>> So, I don't understand why we are pushing towards enabling the >>>> "obsolete" MMIO interface ? Is this because "ACPI" mandates it ? >>>> Then, please don't. The spec needs an update to reflect the ETMs >>>> with sysreg access and ETEs. >>>> >>>> Why not stick to the system register access* ? >>>> >>>> * PS: The ACPI support for the ETM/ETE needs additional changes to the >>>> CoreSight driver, *not* the CoreSight ACPI spec. @Anshuman is >>>> working on >>>> this at the moment and will be available soon. >>>> >>>> The hack patch below should be sufficient to give it a try and if it >>>> works. >> >>> I don't understand your postscript. Certainly there's driver work to be >>> done, but I also think the DEN0067 CoreSight ACPI specification needs >> >> The issue is having a single HID for ETMs (which from a spec point of >> view makes sense for) with and without MMIO access. That brings two >> different components in Linux (AMBA hook for ACPI and a platform driver) >> compete for the said HID. There are other reasons to disconnect the >> CoreSight from AMBA framework and manage them directly [0]. > > [0] https://lkml.kernel.org/r/e37e12ab-9701-2883-724a-2a281ad35df2@arm.com > >
On 02/02/2023 05:20, Steve Clevenger wrote: > > Hi Suzuki, > > I've split out the bug fix (i.e. nr_addr_cmp use) to a separate patch Thanks for that. > and added references to the Ampere erratum in silicon-errata.rst. > These will be submitted as separate patches. > > The ETM4.x TRCOSLAR.OSLK early clear has moved to etm4_init_csdev_iomem > for all manufacturers. I think this is what you asked for. > The no_quad_mmio flag has moved to struct csdev_access, and the split > 64-bit read/write logic has been implemented entirely in the header file > coresight-etm4x.h is the existing calls. > I'd like to retire this patch thread, and submit these as a new thread. > Is there an objection? I would still like to use the system instructions method for the ETM, than hacking the MMIO access for something that is obsolete. The ACPI document for CoreSight will be published soon for review to accommodate the description for ETMs without MMIO and it no longer mandates the MemoryResource. What is the objection to using system instruction access here ? Thanks Suzuki > > Thanks, > Steve > > On 1/23/2023 2:51 PM, Suzuki K Poulose wrote: >> >> Missed the reference, see below. >> >> On 23/01/2023 22:18, Suzuki K Poulose wrote: >>> On 23/01/2023 19:48, Steve Clevenger wrote: >>>> >>>> >>>> On 1/23/2023 9:33 AM, Suzuki K Poulose wrote: >>>>> On 23/01/2023 17:22, Steve Clevenger wrote: >>>>>> >>>>>> >>>>>> On 1/23/2023 2:54 AM, Suzuki K Poulose wrote: >>>>>>> On 21/01/2023 07:30, Steve Clevenger wrote: >>>>>>>> >>>>>>>> Hi Suzuki, >>>>>>>> >>>>>>>> Comments in-line. Please note the approach I attempted while >>>>>>>> adding in >>>>>>>> the Ampere support was to otherwise not disturb existing driver code >>>>>>>> for >>>>>>>> non-Ampere parts. >>>>>>>> >>>>>>>> Steve >>>>>>>> >>>>>>>> On 1/20/2023 3:12 AM, Suzuki K Poulose wrote: >>>>>>>>> Hi Steve >>>>>>>>> >>>>>>>>> Thanks for the patches. Have a few comments below. >>>>>>>>> >>>>>>>>> On 20/01/2023 00:51, Steve Clevenger 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 >>>>>>>>> >>>>>>>>> Please could you add this erratum to the : >>>>>>>>> >>>>>>>>> Documentation/arm64/silicon-errata.rst ? >>>>>>>>> >>>>>>>>> Given the ETM is v4.6, doesn't it support system instructions and >>>>>>>>> that is causing this issue of "MMIO access is considered >>>>>>>>> external" ? >>>>>>>>> If it does, I think we should drop all of this and simply wire the >>>>>>>>> system instruction access support. >>>>>>> >>>>>>>> That's not the issue in this case. This MMIO access should've been >>>>>>>> allowed by the Ampere ETMv4.6 implementation. Based on comments >>>>>>>> I've >>>>>>> >>>>>>> That doesn't answe the question. Please could you confirm the >>>>>>> value of >>>>>>> ID_AA64DFR0_EL1 on your system ? >>>>>> This ID_AA64DFR0_EL1 value came from a TRACE32 debug session connected >>>>>> to this part. The ID_AA64DFR0_EL1 value is 0x000F01F210307619. So, >>>>>> TraceVer, bits [7:4] are b0001. My understanding is the system >>>>>> register >>>>>> interface must be implemented on all ETMv4.6 parts. >>>>> >>>>> So, I don't understand why we are pushing towards enabling the >>>>> "obsolete" MMIO interface ? Is this because "ACPI" mandates it ? >>>>> Then, please don't. The spec needs an update to reflect the ETMs >>>>> with sysreg access and ETEs. >>>>> >>>>> Why not stick to the system register access* ? >>>>> >>>>> * PS: The ACPI support for the ETM/ETE needs additional changes to the >>>>> CoreSight driver, *not* the CoreSight ACPI spec. @Anshuman is >>>>> working on >>>>> this at the moment and will be available soon. >>>>> >>>>> The hack patch below should be sufficient to give it a try and if it >>>>> works. >>> >>>> I don't understand your postscript. Certainly there's driver work to be >>>> done, but I also think the DEN0067 CoreSight ACPI specification needs >>> >>> The issue is having a single HID for ETMs (which from a spec point of >>> view makes sense for) with and without MMIO access. That brings two >>> different components in Linux (AMBA hook for ACPI and a platform driver) >>> compete for the said HID. There are other reasons to disconnect the >>> CoreSight from AMBA framework and manage them directly [0]. >> >> [0] https://lkml.kernel.org/r/e37e12ab-9701-2883-724a-2a281ad35df2@arm.com >> >>
Hi Suzuki, Commented in-line. Steve C. On 2/2/2023 3:16 AM, Suzuki K Poulose wrote: > On 02/02/2023 05:20, Steve Clevenger wrote: >> >> Hi Suzuki, >> >> I've split out the bug fix (i.e. nr_addr_cmp use) to a separate patch > > Thanks for that. > >> and added references to the Ampere erratum in silicon-errata.rst. >> These will be submitted as separate patches. >> >> The ETM4.x TRCOSLAR.OSLK early clear has moved to etm4_init_csdev_iomem >> for all manufacturers. I think this is what you asked for. >> The no_quad_mmio flag has moved to struct csdev_access, and the split >> 64-bit read/write logic has been implemented entirely in the header file >> coresight-etm4x.h is the existing calls. >> I'd like to retire this patch thread, and submit these as a new thread. >> Is there an objection? > > I would still like to use the system instructions method for the ETM, > than hacking the MMIO access for something that is obsolete. > The ACPI document for CoreSight will be published soon for review to > accommodate the description for ETMs without MMIO and it no longer > mandates the MemoryResource. > > What is the objection to using system instruction access here ? No objection going forward. For our initial release, we're not in a position to change the CoreSight DSDT based on a specification which is incomplete. Based on a quick sysreg only build, I didn't collect trace samples. I haven't had time to chase this, but the reported error is "timeout while waiting for Trace Idle Status" on a TRCSTATR read. More testing is required. If this isn't related to an Ampere sysreg access problem (doubtful), the next place I'd look is as a synchronization issue. > > Thanks > Suzuki > > > >> >> Thanks, >> Steve >> >> On 1/23/2023 2:51 PM, Suzuki K Poulose wrote: >>> >>> Missed the reference, see below. >>> >>> On 23/01/2023 22:18, Suzuki K Poulose wrote: >>>> On 23/01/2023 19:48, Steve Clevenger wrote: >>>>> >>>>> >>>>> On 1/23/2023 9:33 AM, Suzuki K Poulose wrote: >>>>>> On 23/01/2023 17:22, Steve Clevenger wrote: >>>>>>> >>>>>>> >>>>>>> On 1/23/2023 2:54 AM, Suzuki K Poulose wrote: >>>>>>>> On 21/01/2023 07:30, Steve Clevenger wrote: >>>>>>>>> >>>>>>>>> Hi Suzuki, >>>>>>>>> >>>>>>>>> Comments in-line. Please note the approach I attempted while >>>>>>>>> adding in >>>>>>>>> the Ampere support was to otherwise not disturb existing driver >>>>>>>>> code >>>>>>>>> for >>>>>>>>> non-Ampere parts. >>>>>>>>> >>>>>>>>> Steve >>>>>>>>> >>>>>>>>> On 1/20/2023 3:12 AM, Suzuki K Poulose wrote: >>>>>>>>>> Hi Steve >>>>>>>>>> >>>>>>>>>> Thanks for the patches. Have a few comments below. >>>>>>>>>> >>>>>>>>>> On 20/01/2023 00:51, Steve Clevenger 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 >>>>>>>>>> >>>>>>>>>> Please could you add this erratum to the : >>>>>>>>>> >>>>>>>>>> Documentation/arm64/silicon-errata.rst ? >>>>>>>>>> >>>>>>>>>> Given the ETM is v4.6, doesn't it support system instructions and >>>>>>>>>> that is causing this issue of "MMIO access is considered >>>>>>>>>> external" ? >>>>>>>>>> If it does, I think we should drop all of this and simply wire >>>>>>>>>> the >>>>>>>>>> system instruction access support. >>>>>>>> >>>>>>>>> That's not the issue in this case. This MMIO access should've been >>>>>>>>> allowed by the Ampere ETMv4.6 implementation. Based on comments >>>>>>>>> I've >>>>>>>> >>>>>>>> That doesn't answe the question. Please could you confirm the >>>>>>>> value of >>>>>>>> ID_AA64DFR0_EL1 on your system ? >>>>>>> This ID_AA64DFR0_EL1 value came from a TRACE32 debug session >>>>>>> connected >>>>>>> to this part. The ID_AA64DFR0_EL1 value is 0x000F01F210307619. So, >>>>>>> TraceVer, bits [7:4] are b0001. My understanding is the system >>>>>>> register >>>>>>> interface must be implemented on all ETMv4.6 parts. >>>>>> >>>>>> So, I don't understand why we are pushing towards enabling the >>>>>> "obsolete" MMIO interface ? Is this because "ACPI" mandates it ? >>>>>> Then, please don't. The spec needs an update to reflect the ETMs >>>>>> with sysreg access and ETEs. >>>>>> >>>>>> Why not stick to the system register access* ? >>>>>> >>>>>> * PS: The ACPI support for the ETM/ETE needs additional changes to >>>>>> the >>>>>> CoreSight driver, *not* the CoreSight ACPI spec. @Anshuman is >>>>>> working on >>>>>> this at the moment and will be available soon. >>>>>> >>>>>> The hack patch below should be sufficient to give it a try and if it >>>>>> works. >>>> >>>>> I don't understand your postscript. Certainly there's driver work >>>>> to be >>>>> done, but I also think the DEN0067 CoreSight ACPI specification needs >>>> >>>> The issue is having a single HID for ETMs (which from a spec point of >>>> view makes sense for) with and without MMIO access. That brings two >>>> different components in Linux (AMBA hook for ACPI and a platform >>>> driver) >>>> compete for the said HID. There are other reasons to disconnect the >>>> CoreSight from AMBA framework and manage them directly [0]. >>> >>> [0] >>> https://lkml.kernel.org/r/e37e12ab-9701-2883-724a-2a281ad35df2@arm.com >>> >>> >
On 02/02/2023 17:12, Steve Clevenger wrote: > Hi Suzuki, > > Commented in-line. > > Steve C. > > On 2/2/2023 3:16 AM, Suzuki K Poulose wrote: >> On 02/02/2023 05:20, Steve Clevenger wrote: >>> >>> Hi Suzuki, >>> >>> I've split out the bug fix (i.e. nr_addr_cmp use) to a separate patch >> >> Thanks for that. >> >>> and added references to the Ampere erratum in silicon-errata.rst. >>> These will be submitted as separate patches. >>> >>> The ETM4.x TRCOSLAR.OSLK early clear has moved to etm4_init_csdev_iomem >>> for all manufacturers. I think this is what you asked for. >>> The no_quad_mmio flag has moved to struct csdev_access, and the split >>> 64-bit read/write logic has been implemented entirely in the header file >>> coresight-etm4x.h is the existing calls. >>> I'd like to retire this patch thread, and submit these as a new thread. >>> Is there an objection? >> >> I would still like to use the system instructions method for the ETM, >> than hacking the MMIO access for something that is obsolete. >> The ACPI document for CoreSight will be published soon for review to >> accommodate the description for ETMs without MMIO and it no longer >> mandates the MemoryResource. >> >> What is the objection to using system instruction access here ? > No objection going forward. For our initial release, we're not in a > position to change the CoreSight DSDT based on a specification which is > incomplete. There is on change to the CoreSight DSDT specification as such. The only change to the "spec" is along the lines of : "MMIO interface is mandatory only if not accessible via system instruction access " > Based on a quick sysreg only build, I didn't collect trace samples. I > haven't had time to chase this, but the reported error is "timeout while > waiting for Trace Idle Status" on a TRCSTATR read. More testing is Are you able to access the other registers ? e.g, $ cat /sys/bus/coresight/devices/etm0/mgmt/trcpidr0 Suzuki > required. If this isn't related to an Ampere sysreg access problem > (doubtful), the next place I'd look is as a synchronization issue. > >> >> Thanks >> Suzuki >> >> >> >>> >>> Thanks, >>> Steve >>> >>> On 1/23/2023 2:51 PM, Suzuki K Poulose wrote: >>>> >>>> Missed the reference, see below. >>>> >>>> On 23/01/2023 22:18, Suzuki K Poulose wrote: >>>>> On 23/01/2023 19:48, Steve Clevenger wrote: >>>>>> >>>>>> >>>>>> On 1/23/2023 9:33 AM, Suzuki K Poulose wrote: >>>>>>> On 23/01/2023 17:22, Steve Clevenger wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 1/23/2023 2:54 AM, Suzuki K Poulose wrote: >>>>>>>>> On 21/01/2023 07:30, Steve Clevenger wrote: >>>>>>>>>> >>>>>>>>>> Hi Suzuki, >>>>>>>>>> >>>>>>>>>> Comments in-line. Please note the approach I attempted while >>>>>>>>>> adding in >>>>>>>>>> the Ampere support was to otherwise not disturb existing driver >>>>>>>>>> code >>>>>>>>>> for >>>>>>>>>> non-Ampere parts. >>>>>>>>>> >>>>>>>>>> Steve >>>>>>>>>> >>>>>>>>>> On 1/20/2023 3:12 AM, Suzuki K Poulose wrote: >>>>>>>>>>> Hi Steve >>>>>>>>>>> >>>>>>>>>>> Thanks for the patches. Have a few comments below. >>>>>>>>>>> >>>>>>>>>>> On 20/01/2023 00:51, Steve Clevenger 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 >>>>>>>>>>> >>>>>>>>>>> Please could you add this erratum to the : >>>>>>>>>>> >>>>>>>>>>> Documentation/arm64/silicon-errata.rst ? >>>>>>>>>>> >>>>>>>>>>> Given the ETM is v4.6, doesn't it support system instructions and >>>>>>>>>>> that is causing this issue of "MMIO access is considered >>>>>>>>>>> external" ? >>>>>>>>>>> If it does, I think we should drop all of this and simply wire >>>>>>>>>>> the >>>>>>>>>>> system instruction access support. >>>>>>>>> >>>>>>>>>> That's not the issue in this case. This MMIO access should've been >>>>>>>>>> allowed by the Ampere ETMv4.6 implementation. Based on comments >>>>>>>>>> I've >>>>>>>>> >>>>>>>>> That doesn't answe the question. Please could you confirm the >>>>>>>>> value of >>>>>>>>> ID_AA64DFR0_EL1 on your system ? >>>>>>>> This ID_AA64DFR0_EL1 value came from a TRACE32 debug session >>>>>>>> connected >>>>>>>> to this part. The ID_AA64DFR0_EL1 value is 0x000F01F210307619. So, >>>>>>>> TraceVer, bits [7:4] are b0001. My understanding is the system >>>>>>>> register >>>>>>>> interface must be implemented on all ETMv4.6 parts. >>>>>>> >>>>>>> So, I don't understand why we are pushing towards enabling the >>>>>>> "obsolete" MMIO interface ? Is this because "ACPI" mandates it ? >>>>>>> Then, please don't. The spec needs an update to reflect the ETMs >>>>>>> with sysreg access and ETEs. >>>>>>> >>>>>>> Why not stick to the system register access* ? >>>>>>> >>>>>>> * PS: The ACPI support for the ETM/ETE needs additional changes to >>>>>>> the >>>>>>> CoreSight driver, *not* the CoreSight ACPI spec. @Anshuman is >>>>>>> working on >>>>>>> this at the moment and will be available soon. >>>>>>> >>>>>>> The hack patch below should be sufficient to give it a try and if it >>>>>>> works. >>>>> >>>>>> I don't understand your postscript. Certainly there's driver work >>>>>> to be >>>>>> done, but I also think the DEN0067 CoreSight ACPI specification needs >>>>> >>>>> The issue is having a single HID for ETMs (which from a spec point of >>>>> view makes sense for) with and without MMIO access. That brings two >>>>> different components in Linux (AMBA hook for ACPI and a platform >>>>> driver) >>>>> compete for the said HID. There are other reasons to disconnect the >>>>> CoreSight from AMBA framework and manage them directly [0]. >>>> >>>> [0] >>>> https://lkml.kernel.org/r/e37e12ab-9701-2883-724a-2a281ad35df2@arm.com >>>> >>>> >>
On 2/2/2023 9:27 AM, Suzuki K Poulose wrote: > On 02/02/2023 17:12, Steve Clevenger wrote: >> Hi Suzuki, >> >> Commented in-line. >> >> Steve C. >> >> On 2/2/2023 3:16 AM, Suzuki K Poulose wrote: >>> On 02/02/2023 05:20, Steve Clevenger wrote: >>>> >>>> Hi Suzuki, >>>> >>>> I've split out the bug fix (i.e. nr_addr_cmp use) to a separate patch >>> >>> Thanks for that. >>> >>>> and added references to the Ampere erratum in silicon-errata.rst. >>>> These will be submitted as separate patches. >>>> >>>> The ETM4.x TRCOSLAR.OSLK early clear has moved to etm4_init_csdev_iomem >>>> for all manufacturers. I think this is what you asked for. >>>> The no_quad_mmio flag has moved to struct csdev_access, and the split >>>> 64-bit read/write logic has been implemented entirely in the header >>>> file >>>> coresight-etm4x.h is the existing calls. >>>> I'd like to retire this patch thread, and submit these as a new thread. >>>> Is there an objection? >>> >>> I would still like to use the system instructions method for the ETM, >>> than hacking the MMIO access for something that is obsolete. >>> The ACPI document for CoreSight will be published soon for review to >>> accommodate the description for ETMs without MMIO and it no longer >>> mandates the MemoryResource. >>> >>> What is the objection to using system instruction access here ? >> No objection going forward. For our initial release, we're not in a >> position to change the CoreSight DSDT based on a specification which is >> incomplete. > > There is on change to the CoreSight DSDT specification as such. The only > change to the "spec" is along the lines of : > > "MMIO interface is mandatory only if not accessible via system > instruction access " > > >> Based on a quick sysreg only build, I didn't collect trace samples. I >> haven't had time to chase this, but the reported error is "timeout while >> waiting for Trace Idle Status" on a TRCSTATR read. More testing is > > Are you able to access the other registers ? > > e.g, > > $ cat /sys/bus/coresight/devices/etm0/mgmt/trcpidr0 > I'll need to determine which of the 3 coresight_timeout calls which read the TRCSTATR idle bit times out. In each case a number of sysreg accesses have occurred prior to any of these 3 calls. > Suzuki > > >> required. If this isn't related to an Ampere sysreg access problem >> (doubtful), the next place I'd look is as a synchronization issue. >> >>> >>> Thanks >>> Suzuki >>> >>> >>> >>>> >>>> Thanks, >>>> Steve >>>> >>>> On 1/23/2023 2:51 PM, Suzuki K Poulose wrote: >>>>> >>>>> Missed the reference, see below. >>>>> >>>>> On 23/01/2023 22:18, Suzuki K Poulose wrote: >>>>>> On 23/01/2023 19:48, Steve Clevenger wrote: >>>>>>> >>>>>>> >>>>>>> On 1/23/2023 9:33 AM, Suzuki K Poulose wrote: >>>>>>>> On 23/01/2023 17:22, Steve Clevenger wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> On 1/23/2023 2:54 AM, Suzuki K Poulose wrote: >>>>>>>>>> On 21/01/2023 07:30, Steve Clevenger wrote: >>>>>>>>>>> >>>>>>>>>>> Hi Suzuki, >>>>>>>>>>> >>>>>>>>>>> Comments in-line. Please note the approach I attempted while >>>>>>>>>>> adding in >>>>>>>>>>> the Ampere support was to otherwise not disturb existing driver >>>>>>>>>>> code >>>>>>>>>>> for >>>>>>>>>>> non-Ampere parts. >>>>>>>>>>> >>>>>>>>>>> Steve >>>>>>>>>>> >>>>>>>>>>> On 1/20/2023 3:12 AM, Suzuki K Poulose wrote: >>>>>>>>>>>> Hi Steve >>>>>>>>>>>> >>>>>>>>>>>> Thanks for the patches. Have a few comments below. >>>>>>>>>>>> >>>>>>>>>>>> On 20/01/2023 00:51, Steve Clevenger 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 >>>>>>>>>>>> >>>>>>>>>>>> Please could you add this erratum to the : >>>>>>>>>>>> >>>>>>>>>>>> Documentation/arm64/silicon-errata.rst ? >>>>>>>>>>>> >>>>>>>>>>>> Given the ETM is v4.6, doesn't it support system >>>>>>>>>>>> instructions and >>>>>>>>>>>> that is causing this issue of "MMIO access is considered >>>>>>>>>>>> external" ? >>>>>>>>>>>> If it does, I think we should drop all of this and simply wire >>>>>>>>>>>> the >>>>>>>>>>>> system instruction access support. >>>>>>>>>> >>>>>>>>>>> That's not the issue in this case. This MMIO access should've >>>>>>>>>>> been >>>>>>>>>>> allowed by the Ampere ETMv4.6 implementation. Based on comments >>>>>>>>>>> I've >>>>>>>>>> >>>>>>>>>> That doesn't answe the question. Please could you confirm the >>>>>>>>>> value of >>>>>>>>>> ID_AA64DFR0_EL1 on your system ? >>>>>>>>> This ID_AA64DFR0_EL1 value came from a TRACE32 debug session >>>>>>>>> connected >>>>>>>>> to this part. The ID_AA64DFR0_EL1 value is 0x000F01F210307619. So, >>>>>>>>> TraceVer, bits [7:4] are b0001. My understanding is the system >>>>>>>>> register >>>>>>>>> interface must be implemented on all ETMv4.6 parts. >>>>>>>> >>>>>>>> So, I don't understand why we are pushing towards enabling the >>>>>>>> "obsolete" MMIO interface ? Is this because "ACPI" mandates it ? >>>>>>>> Then, please don't. The spec needs an update to reflect the ETMs >>>>>>>> with sysreg access and ETEs. >>>>>>>> >>>>>>>> Why not stick to the system register access* ? >>>>>>>> >>>>>>>> * PS: The ACPI support for the ETM/ETE needs additional changes to >>>>>>>> the >>>>>>>> CoreSight driver, *not* the CoreSight ACPI spec. @Anshuman is >>>>>>>> working on >>>>>>>> this at the moment and will be available soon. >>>>>>>> >>>>>>>> The hack patch below should be sufficient to give it a try and >>>>>>>> if it >>>>>>>> works. >>>>>> >>>>>>> I don't understand your postscript. Certainly there's driver work >>>>>>> to be >>>>>>> done, but I also think the DEN0067 CoreSight ACPI specification >>>>>>> needs >>>>>> >>>>>> The issue is having a single HID for ETMs (which from a spec point of >>>>>> view makes sense for) with and without MMIO access. That brings two >>>>>> different components in Linux (AMBA hook for ACPI and a platform >>>>>> driver) >>>>>> compete for the said HID. There are other reasons to disconnect the >>>>>> CoreSight from AMBA framework and manage them directly [0]. >>>>> >>>>> [0] >>>>> https://lkml.kernel.org/r/e37e12ab-9701-2883-724a-2a281ad35df2@arm.com >>>>> >>>>> >>> >
Hi Steve fyi, the ACPI spec for CoreSight has been updated to relax the MMIO region requirement for ETMv4x and ETE. Please find it here : https://developer.arm.com/documentation/den0067/latest Kind regards Suzuki On 02/02/2023 23:03, Steve Clevenger wrote: > > > On 2/2/2023 9:27 AM, Suzuki K Poulose wrote: >> On 02/02/2023 17:12, Steve Clevenger wrote: >>> Hi Suzuki, >>> >>> Commented in-line. >>> >>> Steve C. >>> >>> On 2/2/2023 3:16 AM, Suzuki K Poulose wrote: >>>> On 02/02/2023 05:20, Steve Clevenger wrote: >>>>> >>>>> Hi Suzuki, >>>>> >>>>> I've split out the bug fix (i.e. nr_addr_cmp use) to a separate patch >>>> >>>> Thanks for that. >>>> >>>>> and added references to the Ampere erratum in silicon-errata.rst. >>>>> These will be submitted as separate patches. >>>>> >>>>> The ETM4.x TRCOSLAR.OSLK early clear has moved to etm4_init_csdev_iomem >>>>> for all manufacturers. I think this is what you asked for. >>>>> The no_quad_mmio flag has moved to struct csdev_access, and the split >>>>> 64-bit read/write logic has been implemented entirely in the header >>>>> file >>>>> coresight-etm4x.h is the existing calls. >>>>> I'd like to retire this patch thread, and submit these as a new thread. >>>>> Is there an objection? >>>> >>>> I would still like to use the system instructions method for the ETM, >>>> than hacking the MMIO access for something that is obsolete. >>>> The ACPI document for CoreSight will be published soon for review to >>>> accommodate the description for ETMs without MMIO and it no longer >>>> mandates the MemoryResource. >>>> >>>> What is the objection to using system instruction access here ? >>> No objection going forward. For our initial release, we're not in a >>> position to change the CoreSight DSDT based on a specification which is >>> incomplete. >> >> There is on change to the CoreSight DSDT specification as such. The only >> change to the "spec" is along the lines of : >> >> "MMIO interface is mandatory only if not accessible via system >> instruction access " >> >> >>> Based on a quick sysreg only build, I didn't collect trace samples. I >>> haven't had time to chase this, but the reported error is "timeout while >>> waiting for Trace Idle Status" on a TRCSTATR read. More testing is >> >> Are you able to access the other registers ? >> >> e.g, >> >> $ cat /sys/bus/coresight/devices/etm0/mgmt/trcpidr0 >> > I'll need to determine which of the 3 coresight_timeout calls which read > the TRCSTATR idle bit times out. In each case a number of sysreg > accesses have occurred prior to any of these 3 calls. > >> Suzuki >> >> >>> required. If this isn't related to an Ampere sysreg access problem >>> (doubtful), the next place I'd look is as a synchronization issue. >>> >>>> >>>> Thanks >>>> Suzuki >>>> >>>> >>>> >>>>> >>>>> Thanks, >>>>> Steve >>>>> >>>>> On 1/23/2023 2:51 PM, Suzuki K Poulose wrote: >>>>>> >>>>>> Missed the reference, see below. >>>>>> >>>>>> On 23/01/2023 22:18, Suzuki K Poulose wrote: >>>>>>> On 23/01/2023 19:48, Steve Clevenger wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 1/23/2023 9:33 AM, Suzuki K Poulose wrote: >>>>>>>>> On 23/01/2023 17:22, Steve Clevenger wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 1/23/2023 2:54 AM, Suzuki K Poulose wrote: >>>>>>>>>>> On 21/01/2023 07:30, Steve Clevenger wrote: >>>>>>>>>>>> >>>>>>>>>>>> Hi Suzuki, >>>>>>>>>>>> >>>>>>>>>>>> Comments in-line. Please note the approach I attempted while >>>>>>>>>>>> adding in >>>>>>>>>>>> the Ampere support was to otherwise not disturb existing driver >>>>>>>>>>>> code >>>>>>>>>>>> for >>>>>>>>>>>> non-Ampere parts. >>>>>>>>>>>> >>>>>>>>>>>> Steve >>>>>>>>>>>> >>>>>>>>>>>> On 1/20/2023 3:12 AM, Suzuki K Poulose wrote: >>>>>>>>>>>>> Hi Steve >>>>>>>>>>>>> >>>>>>>>>>>>> Thanks for the patches. Have a few comments below. >>>>>>>>>>>>> >>>>>>>>>>>>> On 20/01/2023 00:51, Steve Clevenger 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 >>>>>>>>>>>>> >>>>>>>>>>>>> Please could you add this erratum to the : >>>>>>>>>>>>> >>>>>>>>>>>>> Documentation/arm64/silicon-errata.rst ? >>>>>>>>>>>>> >>>>>>>>>>>>> Given the ETM is v4.6, doesn't it support system >>>>>>>>>>>>> instructions and >>>>>>>>>>>>> that is causing this issue of "MMIO access is considered >>>>>>>>>>>>> external" ? >>>>>>>>>>>>> If it does, I think we should drop all of this and simply wire >>>>>>>>>>>>> the >>>>>>>>>>>>> system instruction access support. >>>>>>>>>>> >>>>>>>>>>>> That's not the issue in this case. This MMIO access should've >>>>>>>>>>>> been >>>>>>>>>>>> allowed by the Ampere ETMv4.6 implementation. Based on comments >>>>>>>>>>>> I've >>>>>>>>>>> >>>>>>>>>>> That doesn't answe the question. Please could you confirm the >>>>>>>>>>> value of >>>>>>>>>>> ID_AA64DFR0_EL1 on your system ? >>>>>>>>>> This ID_AA64DFR0_EL1 value came from a TRACE32 debug session >>>>>>>>>> connected >>>>>>>>>> to this part. The ID_AA64DFR0_EL1 value is 0x000F01F210307619. So, >>>>>>>>>> TraceVer, bits [7:4] are b0001. My understanding is the system >>>>>>>>>> register >>>>>>>>>> interface must be implemented on all ETMv4.6 parts. >>>>>>>>> >>>>>>>>> So, I don't understand why we are pushing towards enabling the >>>>>>>>> "obsolete" MMIO interface ? Is this because "ACPI" mandates it ? >>>>>>>>> Then, please don't. The spec needs an update to reflect the ETMs >>>>>>>>> with sysreg access and ETEs. >>>>>>>>> >>>>>>>>> Why not stick to the system register access* ? >>>>>>>>> >>>>>>>>> * PS: The ACPI support for the ETM/ETE needs additional changes to >>>>>>>>> the >>>>>>>>> CoreSight driver, *not* the CoreSight ACPI spec. @Anshuman is >>>>>>>>> working on >>>>>>>>> this at the moment and will be available soon. >>>>>>>>> >>>>>>>>> The hack patch below should be sufficient to give it a try and >>>>>>>>> if it >>>>>>>>> works. >>>>>>> >>>>>>>> I don't understand your postscript. Certainly there's driver work >>>>>>>> to be >>>>>>>> done, but I also think the DEN0067 CoreSight ACPI specification >>>>>>>> needs >>>>>>> >>>>>>> The issue is having a single HID for ETMs (which from a spec point of >>>>>>> view makes sense for) with and without MMIO access. That brings two >>>>>>> different components in Linux (AMBA hook for ACPI and a platform >>>>>>> driver) >>>>>>> compete for the said HID. There are other reasons to disconnect the >>>>>>> CoreSight from AMBA framework and manage them directly [0]. >>>>>> >>>>>> [0] >>>>>> https://lkml.kernel.org/r/e37e12ab-9701-2883-724a-2a281ad35df2@arm.com >>>>>> >>>>>> >>>> >>
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; + /* 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)) { + 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); + bool mmio_external; }; /* Address comparator access types */
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(-)