On Thu, 5 Apr 2018 15:33:26 -0600 Mathieu Poirier mathieu.poirier@linaro.org wrote:
On Thu, Mar 29, 2018 at 04:42:51PM -0500, Kim Phillips wrote:
config CORESIGHT_LINK_AND_SINK_TMC
- bool "Coresight generic TMC driver"
- tristate "Coresight generic TMC driver" depends on CORESIGHT_LINKS_AND_SINKS
It is probably a good time to get rid of the dependency on CORESIGHT_LINKS_AND_SINKS. Once upon a time it was true but not anymore. I suggest to drop it for all devices below. Please do a separate patch for that.
OK I'll try, but really not intending on addressing technically unrelated issues here, just wanting to focus on modularization for this series.
+++ b/drivers/hwtracing/coresight/Makefile @@ -2,19 +2,29 @@ # # Makefile for CoreSight drivers. # -obj-$(CONFIG_CORESIGHT) += coresight.o coresight-etm-perf.o -obj-$(CONFIG_OF) += of_coresight.o -obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o \
coresight-tmc-etf.o \coresight-tmc-etr.o+obj-$(CONFIG_CORESIGHT) += coresight-core.o
I find the "core" appendage heavy and suggest it be dropped. Same for ETMs and TMC.
It's needed to differentiate the name of the module from the name of the C file object: coresight-core can't == coresight unless coresight.c changes its name to something else.
+coresight-core-objs := coresight.o \
of_coresight.o+obj-$(CONFIG_CORESIGHT) += coresight-etm-perf.o
+obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-link-sink-tmc.o
Please move to coresight-tmc.o
...
+coresight-link-sink-tmc-objs := coresight-tmc.o \
coresight-tmc-etf.o \coresight-tmc-etr.o
...I can't because of the same reason as above: module name needs to be different from C object file name, otherwise I get cyclic dependency check failures.
In the above tmc case, the module name is coresight-link-sink-tmc, and one of the C object files is coresight-tmc, so they cannot be folded together if the module is to contain object code from the ..tmc-etf.c and ..tmc-etr.c files.
+++ b/drivers/hwtracing/coresight/coresight-etm4x.c @@ -905,6 +905,7 @@ void etm4_config_trace_mode(struct etmv4_config *config) config->addr_acc[ETM_DEFAULT_ADDR_COMP] = addr_acc; config->addr_acc[ETM_DEFAULT_ADDR_COMP + 1] = addr_acc; } +EXPORT_SYMBOL_GPL(etm4_config_trace_mode);
Not sure this is needed - I was able to compile and insmod without it.
Good catch, leftovers from when I thought the _sysfs.c could be its own module.
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index f1d0e21d8cab..f01e38838cfc 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -143,7 +143,7 @@ struct list_head *coresight_build_path(struct coresight_device *csdev, struct coresight_device *sink); void coresight_release_path(struct list_head *path); -#ifdef CONFIG_CORESIGHT_SOURCE_ETM3X +#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM3X)
Although trivial it should go in a separate patch.
Building as modules fails without it: IS_ENABLED is also true if the CONFIG_ is =m. Test an arm32 build?
+++ b/drivers/hwtracing/coresight/coresight-stm.c @@ -790,7 +790,7 @@ static void stm_init_generic_data(struct stm_drvdata *drvdata) drvdata->stm.set_options = stm_generic_set_options; } -static int stm_probe(struct amba_device *adev, const struct amba_id *id) +static int __init stm_probe(struct amba_device *adev, const struct amba_id *id)
This gives me a "section mismatch" warning when compiling (both built-in and as a module). I wonder how you don't see this on your side.
Odd, yes, I see it now. Removed.
+EXPORT_SYMBOL_GPL(tmc_read_unprepare_etb);
Once again I was able to compile and install the module without those exports.
...
+EXPORT_SYMBOL_GPL(tmc_read_unprepare_etr);
Same here.
...
+EXPORT_SYMBOL_GPL(tmc_disable_hw);
Same here.
Yep, thanks, leftovers from earlier, more ambitious moments, where tmc still had its submodules. Removed.
@@ -60,6 +83,7 @@ static struct list_head *stm_path; */ const u32 barrier_pkt[5] = {0x7fffffff, 0x7fffffff, 0x7fffffff, 0x7fffffff, 0x0}; +EXPORT_SYMBOL_GPL(barrier_pkt);
Please move barrier_pkt to coresight-priv.h and declare it there as "static const".
This and the rest of the 'add me/Pratik as author' comments have been addressed.
I'm going to hope that my explanations as to why the above comments are not addressed is OK and send a v2. If not, please comment again.
Thanks,
Kim