| Message ID | 1461982153-19139-6-git-send-email-sre@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi! > Merge omap_ssi and omap_ssi_port into one module. This > fixes problems with module cycle dependencies introduced > by future patches. Interesting way of saying that this prepares us for future patch :-). > +++ b/drivers/hsi/controllers/Makefile > @@ -2,5 +2,5 @@ > # Makefile for HSI controllers drivers > # > > -obj-$(CONFIG_OMAP_SSI) += omap_ssi.o > -obj-$(CONFIG_OMAP_SSI_PORT) += omap_ssi_port.o > +omap_ssi-objs += omap_ssi_core.o omap_ssi_port.o > +obj-$(CONFIG_OMAP_SSI) += omap_ssi.o Can you simply do obj-$(CONFIG_OMAP_SSI) += omap_ssi_core.o omap_ssi_port.o instead? Thanks, Pavel
Hi, On Sun, May 01, 2016 at 11:43:49AM +0200, Pavel Machek wrote: > > +++ b/drivers/hsi/controllers/Makefile > > @@ -2,5 +2,5 @@ > > # Makefile for HSI controllers drivers > > # > > > > -obj-$(CONFIG_OMAP_SSI) += omap_ssi.o > > -obj-$(CONFIG_OMAP_SSI_PORT) += omap_ssi_port.o > > +omap_ssi-objs += omap_ssi_core.o omap_ssi_port.o > > +obj-$(CONFIG_OMAP_SSI) += omap_ssi.o > > Can you simply do > > obj-$(CONFIG_OMAP_SSI) += omap_ssi_core.o omap_ssi_port.o No, that would result in omap_ssi_core.ko and omap_ssi_port.ko if CONFIG_OMAP_SSI is enabled. Basically it's an optimized variant of the current behaviour, but it still builds two modules. -- Sebastian
On Sun 2016-05-01 21:34:10, Sebastian Reichel wrote: > Hi, > > On Sun, May 01, 2016 at 11:43:49AM +0200, Pavel Machek wrote: > > > +++ b/drivers/hsi/controllers/Makefile > > > @@ -2,5 +2,5 @@ > > > # Makefile for HSI controllers drivers > > > # > > > > > > -obj-$(CONFIG_OMAP_SSI) += omap_ssi.o > > > -obj-$(CONFIG_OMAP_SSI_PORT) += omap_ssi_port.o > > > +omap_ssi-objs += omap_ssi_core.o omap_ssi_port.o > > > +obj-$(CONFIG_OMAP_SSI) += omap_ssi.o > > > > Can you simply do > > > > obj-$(CONFIG_OMAP_SSI) += omap_ssi_core.o omap_ssi_port.o > > No, that would result in omap_ssi_core.ko and omap_ssi_port.ko if > CONFIG_OMAP_SSI is enabled. Basically it's an optimized variant of > the current behaviour, but it still builds two modules. Aha... I was not thinking about modular build. Acked-by: Pavel Machek <pavel@ucw.cz> Thanks, Pavel
* Sebastian Reichel <sre@kernel.org> [160429 19:11]: > Merge omap_ssi and omap_ssi_port into one module. This > fixes problems with module cycle dependencies introduced > by future patches. Can you please check against the hardware for the split? For reference, below is what I dumped out from dm3730 for the modules on the L4 interconnect: 0x48000000 + 0x40000 + 0x18000 = 0x48058000, size 0x1000, parent with sysc 0x48000000 + 0x40000 + 0x19000 = 0x48059000, size 0x1000, gdd 0x48000000 + 0x40000 + 0x1a000 = 0x4805a000, size 0x1000, ssi_port1 0x48000000 + 0x40000 + 0x1b000 = 0x4805b000, size 0x1000, ssi_port2 0x48000000 + 0x40000 + 0x1c000 = 0x4805c000, size 0x1000, target agent So the parent target module at 0x48058000 controls everything with the sysc register. The gdd, ssi_port1 and ssi_port2 are children of the parent target module at 0x48058000 and should not have any sysc related registers. Can you please check if gdd, ssi_port1 and ssi_port2 have any sysc related registers too? :) If they do, then they too can idle on their own but most likely still depend on the parent module. The target agent above is a separate module with the interconnect related registers, no need to do anything with that AFAIK. I believe this is the same for 34xx too but have not dumped it out of the hardware. I can do that if the above does not match what you're seeing. If we want to have separate driver modules, you can do this: 1. Have the parent target module at 0x4805800 do PM runtime calls, they then propagate to the hwmod code properly for the ti,hwmods = "ssi" entry. This module can be minimal, and can also have child devices within it's first 0x1000 sized range if needed. 2. Have the parent target module probe the child device drivers as needed with of_platform_populate() at the end of it's probe. The children can't be pm_runtime_irq_safe as it permanently blocks the idling of the parent. 3. Have the the parent target module at 0x4805800 implement PM runtime for it's children by registering struct dev_pm_ops for them. If you really want to have them all as a single module then that should work too as long as there's only one set of sysc related registers. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, May 03, 2016 at 10:32:39AM -0700, Tony Lindgren wrote: > * Sebastian Reichel <sre@kernel.org> [160429 19:11]: > > Merge omap_ssi and omap_ssi_port into one module. This > > fixes problems with module cycle dependencies introduced > > by future patches. > > Can you please check against the hardware for the split? This only merges the kernel modules. There are still multiple devices. > For reference, below is what I dumped out from dm3730 for the > modules on the L4 interconnect: > > 0x48000000 + 0x40000 + 0x18000 = 0x48058000, size 0x1000, parent with sysc > 0x48000000 + 0x40000 + 0x19000 = 0x48059000, size 0x1000, gdd > 0x48000000 + 0x40000 + 0x1a000 = 0x4805a000, size 0x1000, ssi_port1 > 0x48000000 + 0x40000 + 0x1b000 = 0x4805b000, size 0x1000, ssi_port2 > > 0x48000000 + 0x40000 + 0x1c000 = 0x4805c000, size 0x1000, target agent > > So the parent target module at 0x48058000 controls everything > with the sysc register. The gdd, ssi_port1 and ssi_port2 are > children of the parent target module at 0x48058000 and should > not have any sysc related registers. > > Can you please check if gdd, ssi_port1 and ssi_port2 have any > sysc related registers too? :) If they do, then they too can > idle on their own but most likely still depend on the parent > module. The original driver from Nokia (I don't have proper documentation [the SSI related parts are censored in the public OMAP TRM]) does not give hints about any port related SYSC registers. Also it used just one platform device for the whole ssi module. I'm pretty sure, that the SSI stuff shares one set of SYSC registers. > The target agent above is a separate module with the interconnect > related registers, no need to do anything with that AFAIK. The target agent is not referenced at all in Nokia's driver. > I believe this is the same for 34xx too but have not dumped it > out of the hardware. I can do that if the above does not match > what you're seeing. Parent with sysc/gdd/port1/port2 looks familiar. > If we want to have separate driver modules, you can do this: > > 1. Have the parent target module at 0x4805800 do PM runtime > calls, they then propagate to the hwmod code properly for > the ti,hwmods = "ssi" entry. This module can be minimal, > and can also have child devices within it's first 0x1000 > sized range if needed. > > 2. Have the parent target module probe the child device > drivers as needed with of_platform_populate() at the end > of it's probe. The children can't be pm_runtime_irq_safe > as it permanently blocks the idling of the parent. > > 3. Have the the parent target module at 0x4805800 implement > PM runtime for it's children by registering > struct dev_pm_ops for them. > > If you really want to have them all as a single module then > that should work too as long as there's only one set of sysc > related registers. AFAIK there is only one set of sysc registers for the whole SSI module, which must be active if any of the SSI related registers is accessed. I think we should keep the current structure (ports being sub-devices of the core), so runtime PM API will just work. At the moment it does not work because of pm_runtime_irq_safe. I'm currently working on that (my work-in-progress branch is [0]). With the changes from this branch runtime PM status looks fine in sysfs (I have not yet checked if SoC goes to idle if I enable runtime pm for tty e.t.c.) also there are most likely still some "sleeping function call from atomic context" bugs. [0] https://git.kernel.org/cgit/linux/kernel/git/sre/linux-hsi.git/log/?h=runtime-pm-fixes -- Sebastian
* Sebastian Reichel <sre@kernel.org> [160509 13:44]: > Hi, > > On Tue, May 03, 2016 at 10:32:39AM -0700, Tony Lindgren wrote: > > * Sebastian Reichel <sre@kernel.org> [160429 19:11]: > > > Merge omap_ssi and omap_ssi_port into one module. This > > > fixes problems with module cycle dependencies introduced > > > by future patches. > > > > Can you please check against the hardware for the split? > > This only merges the kernel modules. There are still > multiple devices. OK > > For reference, below is what I dumped out from dm3730 for the > > modules on the L4 interconnect: > > > > 0x48000000 + 0x40000 + 0x18000 = 0x48058000, size 0x1000, parent with sysc > > 0x48000000 + 0x40000 + 0x19000 = 0x48059000, size 0x1000, gdd > > 0x48000000 + 0x40000 + 0x1a000 = 0x4805a000, size 0x1000, ssi_port1 > > 0x48000000 + 0x40000 + 0x1b000 = 0x4805b000, size 0x1000, ssi_port2 > > > > 0x48000000 + 0x40000 + 0x1c000 = 0x4805c000, size 0x1000, target agent > > > > So the parent target module at 0x48058000 controls everything > > with the sysc register. The gdd, ssi_port1 and ssi_port2 are > > children of the parent target module at 0x48058000 and should > > not have any sysc related registers. > > > > Can you please check if gdd, ssi_port1 and ssi_port2 have any > > sysc related registers too? :) If they do, then they too can > > idle on their own but most likely still depend on the parent > > module. > > The original driver from Nokia (I don't have proper documentation > [the SSI related parts are censored in the public OMAP TRM]) does > not give hints about any port related SYSC registers. Also it used > just one platform device for the whole ssi module. I'm pretty sure, > that the SSI stuff shares one set of SYSC registers. OK > > The target agent above is a separate module with the interconnect > > related registers, no need to do anything with that AFAIK. > > The target agent is not referenced at all in Nokia's driver. Yes chances are you don't have to do anything with that. > > I believe this is the same for 34xx too but have not dumped it > > out of the hardware. I can do that if the above does not match > > what you're seeing. > > Parent with sysc/gdd/port1/port2 looks familiar. > > > If we want to have separate driver modules, you can do this: > > > > 1. Have the parent target module at 0x4805800 do PM runtime > > calls, they then propagate to the hwmod code properly for > > the ti,hwmods = "ssi" entry. This module can be minimal, > > and can also have child devices within it's first 0x1000 > > sized range if needed. > > > > 2. Have the parent target module probe the child device > > drivers as needed with of_platform_populate() at the end > > of it's probe. The children can't be pm_runtime_irq_safe > > as it permanently blocks the idling of the parent. > > > > 3. Have the the parent target module at 0x4805800 implement > > PM runtime for it's children by registering > > struct dev_pm_ops for them. > > > > If you really want to have them all as a single module then > > that should work too as long as there's only one set of sysc > > related registers. > > AFAIK there is only one set of sysc registers for the whole SSI > module, which must be active if any of the SSI related registers is > accessed. I think we should keep the current structure (ports being > sub-devices of the core), so runtime PM API will just work. OK makes sense, good to hear there's only one sysc register. > At the moment it does not work because of pm_runtime_irq_safe. I'm > currently working on that (my work-in-progress branch is [0]). With > the changes from this branch runtime PM status looks fine in sysfs > (I have not yet checked if SoC goes to idle if I enable runtime pm > for tty e.t.c.) also there are most likely still some "sleeping > function call from atomic context" bugs. Yes pm_runtime_irq_safe is not nice as it permanently enables the parent.. To avoid that you should just remove that and set up delayed work where needed. Regards, Tony > [0] https://git.kernel.org/cgit/linux/kernel/git/sre/linux-hsi.git/log/?h=runtime-pm-fixes -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/hsi/controllers/Kconfig b/drivers/hsi/controllers/Kconfig index 6aba27808172..084ec97eec64 100644 --- a/drivers/hsi/controllers/Kconfig +++ b/drivers/hsi/controllers/Kconfig @@ -12,8 +12,3 @@ config OMAP_SSI If you say Y here, you will enable the OMAP SSI hardware driver. If unsure, say N. - -config OMAP_SSI_PORT - tristate - default m if OMAP_SSI=m - default y if OMAP_SSI=y diff --git a/drivers/hsi/controllers/Makefile b/drivers/hsi/controllers/Makefile index d2665cf9c545..7aba9c7f71bb 100644 --- a/drivers/hsi/controllers/Makefile +++ b/drivers/hsi/controllers/Makefile @@ -2,5 +2,5 @@ # Makefile for HSI controllers drivers # -obj-$(CONFIG_OMAP_SSI) += omap_ssi.o -obj-$(CONFIG_OMAP_SSI_PORT) += omap_ssi_port.o +omap_ssi-objs += omap_ssi_core.o omap_ssi_port.o +obj-$(CONFIG_OMAP_SSI) += omap_ssi.o diff --git a/drivers/hsi/controllers/omap_ssi.h b/drivers/hsi/controllers/omap_ssi.h index 1fa028078a3c..e493321cb0c3 100644 --- a/drivers/hsi/controllers/omap_ssi.h +++ b/drivers/hsi/controllers/omap_ssi.h @@ -164,4 +164,6 @@ struct omap_ssi_controller { #endif }; +extern struct platform_driver ssi_port_pdriver; + #endif /* __LINUX_HSI_OMAP_SSI_H__ */ diff --git a/drivers/hsi/controllers/omap_ssi.c b/drivers/hsi/controllers/omap_ssi_core.c similarity index 97% rename from drivers/hsi/controllers/omap_ssi.c rename to drivers/hsi/controllers/omap_ssi_core.c index 68dfdaa19938..535c76038288 100644 --- a/drivers/hsi/controllers/omap_ssi.c +++ b/drivers/hsi/controllers/omap_ssi_core.c @@ -605,7 +605,22 @@ static struct platform_driver ssi_pdriver = { }, }; -module_platform_driver(ssi_pdriver); +static int __init ssi_init(void) { + int ret; + + ret = platform_driver_register(&ssi_pdriver); + if (ret) + return ret; + + return platform_driver_register(&ssi_port_pdriver); +} +module_init(ssi_init); + +static void __exit ssi_exit(void) { + platform_driver_unregister(&ssi_port_pdriver); + platform_driver_unregister(&ssi_pdriver); +} +module_exit(ssi_exit); MODULE_ALIAS("platform:omap_ssi"); MODULE_AUTHOR("Carlos Chinea <carlos.chinea@nokia.com>"); diff --git a/drivers/hsi/controllers/omap_ssi_port.c b/drivers/hsi/controllers/omap_ssi_port.c index 530095ed39e7..1569bbb53ee8 100644 --- a/drivers/hsi/controllers/omap_ssi_port.c +++ b/drivers/hsi/controllers/omap_ssi_port.c @@ -1117,11 +1117,6 @@ static int ssi_port_probe(struct platform_device *pd) dev_dbg(&pd->dev, "init ssi port...\n"); - if (!try_module_get(ssi->owner)) { - dev_err(&pd->dev, "could not increment parent module refcount\n"); - return -ENODEV; - } - if (!ssi->port || !omap_ssi->port) { dev_err(&pd->dev, "ssi controller not initialized!\n"); err = -ENODEV; @@ -1242,7 +1237,6 @@ static int ssi_port_remove(struct platform_device *pd) omap_ssi->port[omap_port->port_id] = NULL; platform_set_drvdata(pd, NULL); - module_put(ssi->owner); pm_runtime_disable(&pd->dev); return 0; @@ -1369,7 +1363,7 @@ MODULE_DEVICE_TABLE(of, omap_ssi_port_of_match); #define omap_ssi_port_of_match NULL #endif -static struct platform_driver ssi_port_pdriver = { +struct platform_driver ssi_port_pdriver = { .probe = ssi_port_probe, .remove = ssi_port_remove, .driver = { @@ -1378,11 +1372,3 @@ static struct platform_driver ssi_port_pdriver = { .pm = DEV_PM_OPS, }, }; - -module_platform_driver(ssi_port_pdriver); - -MODULE_ALIAS("platform:omap_ssi_port"); -MODULE_AUTHOR("Carlos Chinea <carlos.chinea@nokia.com>"); -MODULE_AUTHOR("Sebastian Reichel <sre@kernel.org>"); -MODULE_DESCRIPTION("Synchronous Serial Interface Port Driver"); -MODULE_LICENSE("GPL v2");
Merge omap_ssi and omap_ssi_port into one module. This fixes problems with module cycle dependencies introduced by future patches. Signed-off-by: Sebastian Reichel <sre@kernel.org> --- drivers/hsi/controllers/Kconfig | 5 ----- drivers/hsi/controllers/Makefile | 4 ++-- drivers/hsi/controllers/omap_ssi.h | 2 ++ drivers/hsi/controllers/{omap_ssi.c => omap_ssi_core.c} | 17 ++++++++++++++++- drivers/hsi/controllers/omap_ssi_port.c | 16 +--------------- 5 files changed, 21 insertions(+), 23 deletions(-) rename drivers/hsi/controllers/{omap_ssi.c => omap_ssi_core.c} (97%)