| Message ID | 20200911084119.1080694-19-suzuki.poulose@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: Support for ETMv4.4 system instructions | expand |
Hi Suzuki, On Fri, 11 Sep 2020 at 09:41, Suzuki K Poulose <suzuki.poulose@arm.com> wrote: > > Add support for devices with system instruction access only. > They don't have a memory mapped interface and thus are not > AMBA devices. > > Cc: Mathieu Poirier <mathieu.poirier@linaro.org> > Cc: Mike Leach <mike.leach@linaro.org> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> > --- > drivers/hwtracing/coresight/coresight-etm4x.c | 42 ++++++++++++++++++- > 1 file changed, 40 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c > index 7d5f942c2108..212713ffa37e 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x.c > @@ -26,6 +26,7 @@ > #include <linux/seq_file.h> > #include <linux/uaccess.h> > #include <linux/perf_event.h> > +#include <linux/platform_device.h> > #include <linux/pm_runtime.h> > #include <linux/property.h> > #include <asm/sections.h> > @@ -1712,6 +1713,20 @@ static int etm4_probe_amba(struct amba_device *adev, const struct amba_id *id) > return ret; > } > > +static int etm4_probe_platform_dev(struct platform_device *pdev) > +{ > + int ret; > + > + pm_runtime_get_noresume(&pdev->dev); > + pm_runtime_set_active(&pdev->dev); > + pm_runtime_enable(&pdev->dev); > + Right about here is where I would expect the sysreg access to TRCDEVARCH etc, to determine if this is an ETM device that can be supported by the driver. This matches approximately the similar ID table checks that the AMBA driver did to ensure a valid device match. This logically separates "is this a device we support" from "what features does this supported device have" > + ret = etm4_probe(&pdev->dev, NULL); > + > + pm_runtime_put(&pdev->dev); > + return ret; > +} > + > static struct amba_cs_uci_id uci_id_etm4[] = { > { > /* ETMv4 UCI data */ > @@ -1749,6 +1764,20 @@ static struct amba_driver etm4x_amba_driver = { > .id_table = etm4_ids, > }; > > +static const struct of_device_id etm4_4_match[] = { > + { .compatible = "arm,coresight-etm-v4.4" }, > + {} > +}; > + > +static struct platform_driver etm4_platform_driver = { > + .probe = etm4_probe_platform_dev, > + .driver = { > + .name = "coresight-etm4x", > + .of_match_table = etm4_4_match, > + .suppress_bind_attrs = true, > + }, > +}; > + > static int __init etm4x_init(void) > { > int ret; > @@ -1761,10 +1790,19 @@ static int __init etm4x_init(void) > > ret = amba_driver_register(&etm4x_amba_driver); > if (ret) { > - pr_err("Error registering etm4x driver\n"); > - etm4_pm_clear(); > + pr_err("Error registering etm4x AMBA driver\n"); > + goto clear_pm; > } > > + ret = platform_driver_register(&etm4_platform_driver); > + if (!ret) > + return 0; > + > + pr_err("Error registering etm4x platform driver\n"); > + amba_driver_unregister(&etm4x_amba_driver); > + > +clear_pm: > + etm4_pm_clear(); > return ret; > } > device_initcall(etm4x_init); > -- > 2.24.1 > Regards Mike -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
Hi Mike On 09/18/2020 04:35 PM, Mike Leach wrote: > Hi Suzuki, > > On Fri, 11 Sep 2020 at 09:41, Suzuki K Poulose <suzuki.poulose@arm.com> wrote: >> >> Add support for devices with system instruction access only. >> They don't have a memory mapped interface and thus are not >> AMBA devices. >> >> Cc: Mathieu Poirier <mathieu.poirier@linaro.org> >> Cc: Mike Leach <mike.leach@linaro.org> >> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> >> --- >> drivers/hwtracing/coresight/coresight-etm4x.c | 42 ++++++++++++++++++- >> 1 file changed, 40 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c >> index 7d5f942c2108..212713ffa37e 100644 >> --- a/drivers/hwtracing/coresight/coresight-etm4x.c >> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c >> @@ -26,6 +26,7 @@ >> #include <linux/seq_file.h> >> #include <linux/uaccess.h> >> #include <linux/perf_event.h> >> +#include <linux/platform_device.h> >> #include <linux/pm_runtime.h> >> #include <linux/property.h> >> #include <asm/sections.h> >> @@ -1712,6 +1713,20 @@ static int etm4_probe_amba(struct amba_device *adev, const struct amba_id *id) >> return ret; >> } >> >> +static int etm4_probe_platform_dev(struct platform_device *pdev) >> +{ >> + int ret; >> + >> + pm_runtime_get_noresume(&pdev->dev); >> + pm_runtime_set_active(&pdev->dev); >> + pm_runtime_enable(&pdev->dev); >> + > > Right about here is where I would expect the sysreg access to > TRCDEVARCH etc, to determine if this is an ETM device that can be > supported by the driver. > This matches approximately the similar ID table checks that the AMBA > driver did to ensure a valid device match. The problem is, we have to do this on the target CPU and later do another one for feature check. > This logically separates "is this a device we support" from "what > features does this supported device have" While I understand the logical argument, it doesn't buy us much. Even now we do an additional check on the supported architecture in the etm4x_probe() anyway and reject the unsupported CPUs there. The only change here is we move the supported architecture check in to the etm4_init_arch_data() and stop the hard work if it is not supported. I would prefer to keep the current method if possible, while cleaning up the detection of the supported (old) devices as agreed in the other patch. Cheers Suzuki
Hi Suzuki, On Wed, 23 Sep 2020 at 12:47, Suzuki K Poulose <suzuki.poulose@arm.com> wrote: > > Hi Mike > > On 09/18/2020 04:35 PM, Mike Leach wrote: > > Hi Suzuki, > > > > On Fri, 11 Sep 2020 at 09:41, Suzuki K Poulose <suzuki.poulose@arm.com> wrote: > >> > >> Add support for devices with system instruction access only. > >> They don't have a memory mapped interface and thus are not > >> AMBA devices. > >> > >> Cc: Mathieu Poirier <mathieu.poirier@linaro.org> > >> Cc: Mike Leach <mike.leach@linaro.org> > >> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> > >> --- > >> drivers/hwtracing/coresight/coresight-etm4x.c | 42 ++++++++++++++++++- > >> 1 file changed, 40 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c > >> index 7d5f942c2108..212713ffa37e 100644 > >> --- a/drivers/hwtracing/coresight/coresight-etm4x.c > >> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c > >> @@ -26,6 +26,7 @@ > >> #include <linux/seq_file.h> > >> #include <linux/uaccess.h> > >> #include <linux/perf_event.h> > >> +#include <linux/platform_device.h> > >> #include <linux/pm_runtime.h> > >> #include <linux/property.h> > >> #include <asm/sections.h> > >> @@ -1712,6 +1713,20 @@ static int etm4_probe_amba(struct amba_device *adev, const struct amba_id *id) > >> return ret; > >> } > >> > >> +static int etm4_probe_platform_dev(struct platform_device *pdev) > >> +{ > >> + int ret; > >> + > >> + pm_runtime_get_noresume(&pdev->dev); > >> + pm_runtime_set_active(&pdev->dev); > >> + pm_runtime_enable(&pdev->dev); > >> + > > > > Right about here is where I would expect the sysreg access to > > TRCDEVARCH etc, to determine if this is an ETM device that can be > > supported by the driver. > > This matches approximately the similar ID table checks that the AMBA > > driver did to ensure a valid device match. > > The problem is, we have to do this on the target CPU and later do another > one for feature check. > > > This logically separates "is this a device we support" from "what > > features does this supported device have" > The additional point is that it also keeps the register access specific code encapsulated in the register access specific area. The whole point of the access abstraction is that the common code just works. Is it not better that the access decision is made as early as possible and the access abstraction is set up, then the more code is common and maintenance is easier going forwards? > While I understand the logical argument, it doesn't buy us much. Even now we do an > additional check on the supported architecture in the etm4x_probe() anyway and reject > the unsupported CPUs there. I do question the value of that check. It's been around since the start of the driver, but perhaps has outlived its usefulness. Even in the AMBA memory mapped case, by the time we get there, we have matched the device based on supplied part numbers & binding information in the device tree, so checking 4 bits (major version number) out of a register does seem curious! If we have somehow got a bunch of other things wrong, there is a 1 in 16 chance that this check will fail too with a false positive. Obviously I can see why you wouldn't want to rely on just that to validate the component type for register access - DEVARCH is far better for that use case. The etmv3 version is a little more interesting - it checks 8 bits, both maj and min versions, with specific checks on 3.3, 3.5, 1.0 and 1.1 (pft), The remainder of the coresight drivers simply trust the part number detection mechanisms. Would it not be more consistent to do the same for ETMv4 as well? > The only change here is we move the supported architecture > check in to the etm4_init_arch_data() and stop the hard work if it is not supported. > > I would prefer to keep the current method if possible, while cleaning up the detection > of the supported (old) devices as agreed in the other patch. > > > Cheers > Suzuki Cheers Mike
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c index 7d5f942c2108..212713ffa37e 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.c +++ b/drivers/hwtracing/coresight/coresight-etm4x.c @@ -26,6 +26,7 @@ #include <linux/seq_file.h> #include <linux/uaccess.h> #include <linux/perf_event.h> +#include <linux/platform_device.h> #include <linux/pm_runtime.h> #include <linux/property.h> #include <asm/sections.h> @@ -1712,6 +1713,20 @@ static int etm4_probe_amba(struct amba_device *adev, const struct amba_id *id) return ret; } +static int etm4_probe_platform_dev(struct platform_device *pdev) +{ + int ret; + + pm_runtime_get_noresume(&pdev->dev); + pm_runtime_set_active(&pdev->dev); + pm_runtime_enable(&pdev->dev); + + ret = etm4_probe(&pdev->dev, NULL); + + pm_runtime_put(&pdev->dev); + return ret; +} + static struct amba_cs_uci_id uci_id_etm4[] = { { /* ETMv4 UCI data */ @@ -1749,6 +1764,20 @@ static struct amba_driver etm4x_amba_driver = { .id_table = etm4_ids, }; +static const struct of_device_id etm4_4_match[] = { + { .compatible = "arm,coresight-etm-v4.4" }, + {} +}; + +static struct platform_driver etm4_platform_driver = { + .probe = etm4_probe_platform_dev, + .driver = { + .name = "coresight-etm4x", + .of_match_table = etm4_4_match, + .suppress_bind_attrs = true, + }, +}; + static int __init etm4x_init(void) { int ret; @@ -1761,10 +1790,19 @@ static int __init etm4x_init(void) ret = amba_driver_register(&etm4x_amba_driver); if (ret) { - pr_err("Error registering etm4x driver\n"); - etm4_pm_clear(); + pr_err("Error registering etm4x AMBA driver\n"); + goto clear_pm; } + ret = platform_driver_register(&etm4_platform_driver); + if (!ret) + return 0; + + pr_err("Error registering etm4x platform driver\n"); + amba_driver_unregister(&etm4x_amba_driver); + +clear_pm: + etm4_pm_clear(); return ret; } device_initcall(etm4x_init);
Add support for devices with system instruction access only. They don't have a memory mapped interface and thus are not AMBA devices. Cc: Mathieu Poirier <mathieu.poirier@linaro.org> Cc: Mike Leach <mike.leach@linaro.org> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> --- drivers/hwtracing/coresight/coresight-etm4x.c | 42 ++++++++++++++++++- 1 file changed, 40 insertions(+), 2 deletions(-)