Message ID | 20201028220945.3826358-21-suzuki.poulose@arm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | coresight: Support for ETM system instructions | expand |
On Wed, Oct 28, 2020 at 10:09:38PM +0000, Suzuki K Poulose wrote: > In preparation to detect the support for system instruction > support, move the detection of the device access to the target > CPU. > > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> > --- > .../coresight/coresight-etm4x-core.c | 45 ++++++++++++++++--- > 1 file changed, 40 insertions(+), 5 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > index f038bb10bc78..308674ab746c 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > @@ -56,6 +56,11 @@ static u64 etm4_get_access_type(struct etmv4_config *config); > > static enum cpuhp_state hp_online; > > +struct etm_init_arg { s/etm_init_arg/etm4_init_arg > + struct etmv4_drvdata *drvdata; > + struct csdev_access *csa; > +}; > + > u64 etm4x_sysreg_read(struct csdev_access *csa, > u32 offset, > bool _relaxed, > @@ -669,6 +674,22 @@ static const struct coresight_ops etm4_cs_ops = { > .source_ops = &etm4_source_ops, > }; > > +static bool etm_init_iomem_access(struct etmv4_drvdata *drvdata, > + struct csdev_access *csa) > +{ > + *csa = CSDEV_ACCESS_IOMEM(drvdata->base); > + return true; > +} > + > +static bool etm_init_csdev_access(struct etmv4_drvdata *drvdata, > + struct csdev_access *csa) > +{ > + if (drvdata->base) > + return etm_init_iomem_access(drvdata, csa); > + > + return false; > +} Returning a boolean rather than an int for the above two functions seems odd to me. > + > static void etm4_init_arch_data(void *info) > { > u32 etmidr0; > @@ -677,11 +698,22 @@ static void etm4_init_arch_data(void *info) > u32 etmidr3; > u32 etmidr4; > u32 etmidr5; > - struct etmv4_drvdata *drvdata = info; > - struct csdev_access tmp_csa = CSDEV_ACCESS_IOMEM(drvdata->base); > - struct csdev_access *csa = &tmp_csa; > + struct etm_init_arg *init_arg = info; > + struct etmv4_drvdata *drvdata; > + struct csdev_access *csa; > int i; > > + drvdata = init_arg->drvdata; > + csa = init_arg->csa; > + > + /* > + * If we are unable to detect the access mechanism, > + * or unable to detect the trace unit type, fail > + * early. > + */ > + if (!etm_init_csdev_access(drvdata, csa)) > + return; > + > /* Make sure all registers are accessible */ > etm4_os_unlock_csa(drvdata, csa); > etm4_cs_unlock(drvdata, csa); > @@ -1524,6 +1556,7 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) > struct etmv4_drvdata *drvdata; > struct resource *res = &adev->res; > struct coresight_desc desc = { 0 }; > + struct etm_init_arg init_arg = { 0 }; > > drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); > if (!drvdata) > @@ -1551,7 +1584,6 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) > return PTR_ERR(base); > > drvdata->base = base; > - desc.access = CSDEV_ACCESS_IOMEM(base); > > spin_lock_init(&drvdata->spinlock); > > @@ -1563,8 +1595,11 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) > if (!desc.name) > return -ENOMEM; > > + init_arg.drvdata = drvdata; > + init_arg.csa = &desc.access; > + > if (smp_call_function_single(drvdata->cpu, > - etm4_init_arch_data, drvdata, 1)) > + etm4_init_arch_data, &init_arg, 1)) > dev_err(dev, "ETM arch init failed\n"); > > if (etm4_arch_supported(drvdata->arch) == false) > -- > 2.24.1 >
On Wed, Oct 28, 2020 at 10:09:38PM +0000, Suzuki K Poulose wrote: > In preparation to detect the support for system instruction > support, move the detection of the device access to the target > CPU. > > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> > --- > .../coresight/coresight-etm4x-core.c | 45 ++++++++++++++++--- > 1 file changed, 40 insertions(+), 5 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > index f038bb10bc78..308674ab746c 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > @@ -56,6 +56,11 @@ static u64 etm4_get_access_type(struct etmv4_config *config); > > static enum cpuhp_state hp_online; > > +struct etm_init_arg { > + struct etmv4_drvdata *drvdata; > + struct csdev_access *csa; > +}; > + > u64 etm4x_sysreg_read(struct csdev_access *csa, > u32 offset, > bool _relaxed, > @@ -669,6 +674,22 @@ static const struct coresight_ops etm4_cs_ops = { > .source_ops = &etm4_source_ops, > }; > > +static bool etm_init_iomem_access(struct etmv4_drvdata *drvdata, > + struct csdev_access *csa) > +{ > + *csa = CSDEV_ACCESS_IOMEM(drvdata->base); > + return true; > +} > + > +static bool etm_init_csdev_access(struct etmv4_drvdata *drvdata, > + struct csdev_access *csa) > +{ > + if (drvdata->base) > + return etm_init_iomem_access(drvdata, csa); > + > + return false; > +} I would also prefix the above two functions with "etm4_" rather than "etm_" to follow what is already done in this file. > + > static void etm4_init_arch_data(void *info) > { > u32 etmidr0; > @@ -677,11 +698,22 @@ static void etm4_init_arch_data(void *info) > u32 etmidr3; > u32 etmidr4; > u32 etmidr5; > - struct etmv4_drvdata *drvdata = info; > - struct csdev_access tmp_csa = CSDEV_ACCESS_IOMEM(drvdata->base); > - struct csdev_access *csa = &tmp_csa; > + struct etm_init_arg *init_arg = info; > + struct etmv4_drvdata *drvdata; > + struct csdev_access *csa; > int i; > > + drvdata = init_arg->drvdata; > + csa = init_arg->csa; > + > + /* > + * If we are unable to detect the access mechanism, > + * or unable to detect the trace unit type, fail > + * early. > + */ > + if (!etm_init_csdev_access(drvdata, csa)) > + return; > + > /* Make sure all registers are accessible */ > etm4_os_unlock_csa(drvdata, csa); > etm4_cs_unlock(drvdata, csa); > @@ -1524,6 +1556,7 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) > struct etmv4_drvdata *drvdata; > struct resource *res = &adev->res; > struct coresight_desc desc = { 0 }; > + struct etm_init_arg init_arg = { 0 }; > > drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); > if (!drvdata) > @@ -1551,7 +1584,6 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) > return PTR_ERR(base); > > drvdata->base = base; > - desc.access = CSDEV_ACCESS_IOMEM(base); > > spin_lock_init(&drvdata->spinlock); > > @@ -1563,8 +1595,11 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) > if (!desc.name) > return -ENOMEM; > > + init_arg.drvdata = drvdata; > + init_arg.csa = &desc.access; > + > if (smp_call_function_single(drvdata->cpu, > - etm4_init_arch_data, drvdata, 1)) > + etm4_init_arch_data, &init_arg, 1)) > dev_err(dev, "ETM arch init failed\n"); > > if (etm4_arch_supported(drvdata->arch) == false) > -- > 2.24.1 >
On 11/6/20 8:34 PM, Mathieu Poirier wrote: > On Wed, Oct 28, 2020 at 10:09:38PM +0000, Suzuki K Poulose wrote: >> In preparation to detect the support for system instruction >> support, move the detection of the device access to the target >> CPU. >> >> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> >> --- >> .../coresight/coresight-etm4x-core.c | 45 ++++++++++++++++--- >> 1 file changed, 40 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c >> index f038bb10bc78..308674ab746c 100644 >> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c >> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c >> @@ -56,6 +56,11 @@ static u64 etm4_get_access_type(struct etmv4_config *config); >> >> static enum cpuhp_state hp_online; >> >> +struct etm_init_arg { > > s/etm_init_arg/etm4_init_arg Part of the reason was to add a future IP support where it is not all ETM4. Again it doesn't really matter. I could change it. > >> + struct etmv4_drvdata *drvdata; >> + struct csdev_access *csa; >> +}; >> + >> u64 etm4x_sysreg_read(struct csdev_access *csa, >> u32 offset, >> bool _relaxed, >> @@ -669,6 +674,22 @@ static const struct coresight_ops etm4_cs_ops = { >> .source_ops = &etm4_source_ops, >> }; >> >> +static bool etm_init_iomem_access(struct etmv4_drvdata *drvdata, >> + struct csdev_access *csa) >> +{ >> + *csa = CSDEV_ACCESS_IOMEM(drvdata->base); >> + return true; >> +} >> + >> +static bool etm_init_csdev_access(struct etmv4_drvdata *drvdata, >> + struct csdev_access *csa) >> +{ >> + if (drvdata->base) >> + return etm_init_iomem_access(drvdata, csa); >> + >> + return false; >> +} > > Returning a boolean rather than an int for the above two functions seems odd to > me. > We don't return an error from the caller of these functions. So, all we need to know is, if the operation was success or failure. Having bool makes it explicit for the checkings, rather than documenting the expected return values. Hence the choice. But I am open to changing them if you prefer it that way. Cheers Suzuki
On Mon, Nov 09, 2020 at 09:48:07AM +0000, Suzuki K Poulose wrote: > On 11/6/20 8:34 PM, Mathieu Poirier wrote: > > On Wed, Oct 28, 2020 at 10:09:38PM +0000, Suzuki K Poulose wrote: > > > In preparation to detect the support for system instruction > > > support, move the detection of the device access to the target > > > CPU. > > > > > > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> > > > --- > > > .../coresight/coresight-etm4x-core.c | 45 ++++++++++++++++--- > > > 1 file changed, 40 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > > > index f038bb10bc78..308674ab746c 100644 > > > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > > > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > > > @@ -56,6 +56,11 @@ static u64 etm4_get_access_type(struct etmv4_config *config); > > > static enum cpuhp_state hp_online; > > > +struct etm_init_arg { > > > > s/etm_init_arg/etm4_init_arg > > Part of the reason was to add a future IP support where it is not all > ETM4. Again it doesn't really matter. I could change it. > I thought about that too but the inclusion of etmv4_drvdata cancels any attempts at making things generic. > > > > > + struct etmv4_drvdata *drvdata; > > > + struct csdev_access *csa; > > > +}; > > > + > > > u64 etm4x_sysreg_read(struct csdev_access *csa, > > > u32 offset, > > > bool _relaxed, > > > @@ -669,6 +674,22 @@ static const struct coresight_ops etm4_cs_ops = { > > > .source_ops = &etm4_source_ops, > > > }; > > > +static bool etm_init_iomem_access(struct etmv4_drvdata *drvdata, > > > + struct csdev_access *csa) > > > +{ > > > + *csa = CSDEV_ACCESS_IOMEM(drvdata->base); > > > + return true; > > > +} > > > + > > > +static bool etm_init_csdev_access(struct etmv4_drvdata *drvdata, > > > + struct csdev_access *csa) > > > +{ > > > + if (drvdata->base) > > > + return etm_init_iomem_access(drvdata, csa); > > > + > > > + return false; > > > +} > > > > Returning a boolean rather than an int for the above two functions seems odd to > > me. > > > > We don't return an error from the caller of these functions. So, all we And this is done from smp_call_function_single() where returning a meaningful error value would mandate changes to struct etm_init_arg, which is needlessly messy for this set. Void my comment. > need to know is, if the operation was success or failure. Having bool > makes it explicit for the checkings, rather than documenting the > expected return values. Hence the choice. But I am open to changing them > if you prefer it that way. > > > > Cheers > Suzuki
On 11/6/20 8:46 PM, Mathieu Poirier wrote: > On Wed, Oct 28, 2020 at 10:09:38PM +0000, Suzuki K Poulose wrote: >> In preparation to detect the support for system instruction >> support, move the detection of the device access to the target >> CPU. >> >> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> >> --- >> .../coresight/coresight-etm4x-core.c | 45 ++++++++++++++++--- >> 1 file changed, 40 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c >> index f038bb10bc78..308674ab746c 100644 >> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c >> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c >> @@ -56,6 +56,11 @@ static u64 etm4_get_access_type(struct etmv4_config *config); >> >> static enum cpuhp_state hp_online; >> >> +struct etm_init_arg { >> + struct etmv4_drvdata *drvdata; >> + struct csdev_access *csa; >> +}; >> + >> u64 etm4x_sysreg_read(struct csdev_access *csa, >> u32 offset, >> bool _relaxed, >> @@ -669,6 +674,22 @@ static const struct coresight_ops etm4_cs_ops = { >> .source_ops = &etm4_source_ops, >> }; >> >> +static bool etm_init_iomem_access(struct etmv4_drvdata *drvdata, >> + struct csdev_access *csa) >> +{ >> + *csa = CSDEV_ACCESS_IOMEM(drvdata->base); >> + return true; >> +} >> + >> +static bool etm_init_csdev_access(struct etmv4_drvdata *drvdata, >> + struct csdev_access *csa) >> +{ >> + if (drvdata->base) >> + return etm_init_iomem_access(drvdata, csa); >> + >> + return false; >> +} > > I would also prefix the above two functions with "etm4_" rather than "etm_" to > follow what is already done in this file. sure, will do. suzuki
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index f038bb10bc78..308674ab746c 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -56,6 +56,11 @@ static u64 etm4_get_access_type(struct etmv4_config *config); static enum cpuhp_state hp_online; +struct etm_init_arg { + struct etmv4_drvdata *drvdata; + struct csdev_access *csa; +}; + u64 etm4x_sysreg_read(struct csdev_access *csa, u32 offset, bool _relaxed, @@ -669,6 +674,22 @@ static const struct coresight_ops etm4_cs_ops = { .source_ops = &etm4_source_ops, }; +static bool etm_init_iomem_access(struct etmv4_drvdata *drvdata, + struct csdev_access *csa) +{ + *csa = CSDEV_ACCESS_IOMEM(drvdata->base); + return true; +} + +static bool etm_init_csdev_access(struct etmv4_drvdata *drvdata, + struct csdev_access *csa) +{ + if (drvdata->base) + return etm_init_iomem_access(drvdata, csa); + + return false; +} + static void etm4_init_arch_data(void *info) { u32 etmidr0; @@ -677,11 +698,22 @@ static void etm4_init_arch_data(void *info) u32 etmidr3; u32 etmidr4; u32 etmidr5; - struct etmv4_drvdata *drvdata = info; - struct csdev_access tmp_csa = CSDEV_ACCESS_IOMEM(drvdata->base); - struct csdev_access *csa = &tmp_csa; + struct etm_init_arg *init_arg = info; + struct etmv4_drvdata *drvdata; + struct csdev_access *csa; int i; + drvdata = init_arg->drvdata; + csa = init_arg->csa; + + /* + * If we are unable to detect the access mechanism, + * or unable to detect the trace unit type, fail + * early. + */ + if (!etm_init_csdev_access(drvdata, csa)) + return; + /* Make sure all registers are accessible */ etm4_os_unlock_csa(drvdata, csa); etm4_cs_unlock(drvdata, csa); @@ -1524,6 +1556,7 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) struct etmv4_drvdata *drvdata; struct resource *res = &adev->res; struct coresight_desc desc = { 0 }; + struct etm_init_arg init_arg = { 0 }; drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); if (!drvdata) @@ -1551,7 +1584,6 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) return PTR_ERR(base); drvdata->base = base; - desc.access = CSDEV_ACCESS_IOMEM(base); spin_lock_init(&drvdata->spinlock); @@ -1563,8 +1595,11 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) if (!desc.name) return -ENOMEM; + init_arg.drvdata = drvdata; + init_arg.csa = &desc.access; + if (smp_call_function_single(drvdata->cpu, - etm4_init_arch_data, drvdata, 1)) + etm4_init_arch_data, &init_arg, 1)) dev_err(dev, "ETM arch init failed\n"); if (etm4_arch_supported(drvdata->arch) == false)
In preparation to detect the support for system instruction support, move the detection of the device access to the target CPU. Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> --- .../coresight/coresight-etm4x-core.c | 45 ++++++++++++++++--- 1 file changed, 40 insertions(+), 5 deletions(-)