ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

If the kernel is built to support multi-arm configurmation with shmobile support built in, then the drivers/sh is not built. This contains drivers that are essential to devices support by that configuration, including the PM runtime code in drivers/sh/pm_runtime.c (which implicitly enables the bus clocks for all devices).

If CONFIG_ARCH_SHMOBILE_MULTI then build the drivers/sh directory, but ensure that bits that may conflict (drivers/sh/clk if the common clock framework is not enabled) are built.

The ARCH_SHMOBILE_MULTI was added by efacfce5f8a ("ARM: shmobile: Introduce ARCH_SHMOBILE_MULTI") but this has only just recently been found due to building device-tree only kernels.

Cc Linux Kernel list <[email protected]>

Cc Linus SH list <[email protected]>

Cc Simon Horman <[email protected]>

Cc Magnus Damm <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Signed-off-by Ben Dooks <[email protected]>

---
 drivers/Makefile    | 1 +
 drivers/sh/Makefile | 3 +++
 2 files changed, 4 insertions(+)

drivers/Makefile +0 -0

index 8e3b8b0..abc4744 100644

drivers/Makefile +0 -0

 ifndef CONFIG_ARCH_USES_GETTIMEOFFSET
 obj-y				+= clocksource/

drivers/sh/Makefile +3 -0

index fc67f56..86604a5 100644
@@ -3,7 +3,10 @@
 #
 obj-y	:= intc/
 
+ifeq ($(CONFIG_COMMON_CLK),n)
 obj-$(CONFIG_HAVE_CLK)		+= clk/
+endif
+
 obj-$(CONFIG_MAPLE)		+= maple/
 obj-$(CONFIG_SUPERHYWAY)	+= superhyway/
 

Signature · 1 line

1.8.5.2

Quoted 25 lines · Ben Dooks

If the kernel is built to support multi-arm configurmation with shmobile support built in, then the drivers/sh is not built. This contains drivers that are essential to devices support by that configuration, including the PM runtime code in drivers/sh/pm_runtime.c (which implicitly enables the bus clocks for all devices).

If CONFIG_ARCH_SHMOBILE_MULTI then build the drivers/sh directory, but ensure that bits that may conflict (drivers/sh/clk if the common clock framework is not enabled) are built.

The ARCH_SHMOBILE_MULTI was added by efacfce5f8a ("ARM: shmobile: Introduce ARCH_SHMOBILE_MULTI") but this has only just recently been found due to building device-tree only kernels.

Cc Linux Kernel list <[email protected]>

Cc Linus SH list <[email protected]>

Cc Simon Horman <[email protected]>

Cc Magnus Damm <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Signed-off-by Ben Dooks <[email protected]>

---
  drivers/Makefile    | 1 +
  drivers/sh/Makefile | 3 +++
  2 files changed, 4 insertions(+)

drivers/Makefile obj-$(CONFIG_SGI_SN) += sn/ · line 122

Quoted 10 lines

drivers/Makefile +1 -0

index 8e3b8b0..abc4744 100644
@@ -118,6 +118,7 @@ obj-$(CONFIG_SGI_SN)		+= sn/
  obj-y				+= firmware/
  obj-$(CONFIG_CRYPTO)		+= crypto/
  obj-$(CONFIG_SUPERH)		+= sh/
+obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= sh/
  obj-$(CONFIG_ARCH_SHMOBILE_LEGACY)	+= sh/

Can't we just do the following?

drivers/sh/Makefile line 9

Quoted 10 lines · Ben Dooks

drivers/sh/Makefile +3 -0

@@ -3,7 +3,10 @@
  #
  obj-y  := intc/
 
+ifeq ($(CONFIG_COMMON_CLK),n)
  obj-$(CONFIG_HAVE_CLK)         += clk/
+endif
+

This part breaks my Koelsch legacy (non-reference) build:

arch/arm/kernel/built-in.o: In function `twd_local_timer_common_register':
io.c:(.init.text+0x1804): undefined reference to clk_enable' io.c:(.init.text+0x1828): undefined reference to clk_get_rate'

Both of my Koelsch legacy and reference configs have CONFIG_HAVE_CLK=y, but only the reference once has CONFIG_COMMON_CLK=y.

Quoted 17 lines · Geert Uytterhoeven

drivers/sh/Makefile line 9

Quoted 11 lines · Ben Dooks

drivers/sh/Makefile +3 -0

@@ -3,7 +3,10 @@
  #
  obj-y  := intc/
 
+ifeq ($(CONFIG_COMMON_CLK),n)
  obj-$(CONFIG_HAVE_CLK)         += clk/
+endif
+

This part breaks my Koelsch legacy (non-reference) build:

Quoted 3 lines

arch/arm/kernel/built-in.o: In function `twd_local_timer_common_register':
io.c:(.init.text+0x1804): undefined reference to clk_enable' io.c:(.init.text+0x1828): undefined reference to clk_get_rate'

...

Quoted 3 lines

Both of my Koelsch legacy and reference configs have CONFIG_HAVE_CLK=y, but only the reference once has CONFIG_COMMON_CLK=y.

Hmm, thought undefined symbols got set to 'n'.

I can either fix this by

ifneq ($(CONFIG_COMMON_CLK),y)
endif

or adding an extra Kconfig for SH specific legacy clock code.

If the kernel is built to support multi-arm configurmation with shmobile support built in, then the drivers/sh is not built. This contains drivers that are essential to devices support by that configuration, including the PM runtime code in drivers/sh/pm_runtime.c (which implicitly enables the bus clocks for all devices).

If CONFIG_ARCH_SHMOBILE_MULTI then build the drivers/sh directory, but ensure that bits that may conflict (drivers/sh/clk if the common clock framework is not enabled) are built.

The ARCH_SHMOBILE_MULTI was added by efacfce5f8a ("ARM: shmobile: Introduce ARCH_SHMOBILE_MULTI") but this has only just recently been found due to changes currently only in Simon Horman's tree. This patch is a partial revert of bf98c1eac1d4a6b ("ARM: Rename ARCH_SHMOBILE to ARCH_SHMOBILE_LEGACY") to address the issue of drivers not being built.

It is also possible the drivers/sh/intc will also need to be built however the lack of intc is not causing a number of drivers to fail to properly manage their clocks. This is left as an future patch for that is perfectly fine. someone who understands that part of the code.

Cc Linus SH list <[email protected]>

Cc [email protected]

Cc Simon Horman <[email protected]>

Cc Magnus Damm <[email protected]>

Signed-off-by Ben Dooks <[email protected]>

---
 drivers/Makefile    | 2 +-
 drivers/sh/Makefile | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

drivers/Makefile +1 -1

index 8e3b8b0..3cc8214 100644
@@ -118,7 +118,7 @@ obj-$(CONFIG_SGI_SN)		+= sn/
 obj-y				+= firmware/
 obj-$(CONFIG_CRYPTO)		+= crypto/
 obj-$(CONFIG_SUPERH)		+= sh/
-obj-$(CONFIG_ARCH_SHMOBILE_LEGACY)	+= sh/
+obj-$(CONFIG_ARCH_SHMOBILE)	+= sh/
 ifndef CONFIG_ARCH_USES_GETTIMEOFFSET
 obj-y				+= clocksource/
 endif

drivers/sh/Makefile +3 -0

index fc67f56..b4d588c 100644
@@ -3,7 +3,10 @@
 #
 obj-y	:= intc/
 
+ifneq ($(CONFIG_COMMON_CLK),y)
 obj-$(CONFIG_HAVE_CLK)		+= clk/
+endif
+
 obj-$(CONFIG_MAPLE)		+= maple/
 obj-$(CONFIG_SUPERHYWAY)	+= superhyway/
 

Signature · 1 line

1.8.5.2

Quoted 16 lines · Ben Dooks

If the kernel is built to support multi-arm configurmation with shmobile support built in, then the drivers/sh is not built. This contains drivers that are essential to devices support by that configuration, including the PM runtime code in drivers/sh/pm_runtime.c (which implicitly enables the bus clocks for all devices).

If CONFIG_ARCH_SHMOBILE_MULTI then build the drivers/sh directory, but ensure that bits that may conflict (drivers/sh/clk if the common clock framework is not enabled) are built.

The ARCH_SHMOBILE_MULTI was added by efacfce5f8a ("ARM: shmobile:
Introduce ARCH_SHMOBILE_MULTI") but this has only just recently been found due to changes currently only in Simon Horman's tree. This patch is a partial revert of bf98c1eac1d4a6b ("ARM: Rename ARCH_SHMOBILE to ARCH_SHMOBILE_LEGACY") to address the issue of drivers not being built.

Quoted 4 lines

It is also possible the drivers/sh/intc will also need to be built however the lack of intc is not causing a number of drivers to fail to properly manage their clocks. This is left as an future patch for that is perfectly fine. someone who understands that part of the code.

Laurent, what are you feelings on this?

Quoted 38 lines

Cc Linus SH list <[email protected]>

Cc [email protected]

Cc Simon Horman <[email protected]>

Cc Magnus Damm <[email protected]>

Signed-off-by Ben Dooks <[email protected]>

---

 drivers/Makefile    | 2 +-
 drivers/sh/Makefile | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

drivers/Makefile +1 -1

index 8e3b8b0..3cc8214 100644
@@ -118,7 +118,7 @@ obj-$(CONFIG_SGI_SN)		+= sn/
 obj-y				+= firmware/
 obj-$(CONFIG_CRYPTO)		+= crypto/
 obj-$(CONFIG_SUPERH)		+= sh/
-obj-$(CONFIG_ARCH_SHMOBILE_LEGACY)	+= sh/
+obj-$(CONFIG_ARCH_SHMOBILE)	+= sh/
 ifndef CONFIG_ARCH_USES_GETTIMEOFFSET
 obj-y				+= clocksource/
 endif

drivers/sh/Makefile +3 -0

index fc67f56..b4d588c 100644
@@ -3,7 +3,10 @@
 #
 obj-y	:= intc/
 
+ifneq ($(CONFIG_COMMON_CLK),y)
 obj-$(CONFIG_HAVE_CLK)		+= clk/
+endif
+
 obj-$(CONFIG_MAPLE)		+= maple/
 obj-$(CONFIG_SUPERHYWAY)	+= superhyway/

Hi Simon,

Quoted 17 lines · On Wednesday 15 January 2014 08:55:05 Simon Horman wrote:

Quoted 16 lines · Ben Dooks

If the kernel is built to support multi-arm configurmation with shmobile support built in, then the drivers/sh is not built. This contains drivers that are essential to devices support by that configuration, including the PM runtime code in drivers/sh/pm_runtime.c (which implicitly enables the bus clocks for all devices).

If CONFIG_ARCH_SHMOBILE_MULTI then build the drivers/sh directory, but ensure that bits that may conflict (drivers/sh/clk if the common clock framework is not enabled) are built.

The ARCH_SHMOBILE_MULTI was added by efacfce5f8a ("ARM: shmobile:
Introduce ARCH_SHMOBILE_MULTI") but this has only just recently been found due to changes currently only in Simon Horman's tree. This patch is a partial revert of bf98c1eac1d4a6b ("ARM: Rename ARCH_SHMOBILE to ARCH_SHMOBILE_LEGACY") to address the issue of drivers not being built.

Quoted 6 lines

Quoted 4 lines

It is also possible the drivers/sh/intc will also need to be built however the lack of intc is not causing a number of drivers to fail to properly manage their clocks. This is left as an future patch for that is perfectly fine. someone who understands that part of the code.

Laurent, what are you feelings on this?

If we end up needing an interrupt controller supported by drivers/sh/intc for a multiplatform kernel I would rather like to port the driver to drivers/irqchip instead of compiling drivers/sh/intc for the platform.

Regarding drivers/sh/pm_runtime.c, compiling it for ARCH_SHMOBILE_MULTI will cause multiplatform kernels running on non-Renesas platforms to add a pm clock notifier. We need to at least add a runtime check.

Quoted 37 lines · Laurent Pinchart

Hi Simon,

Quoted 22 lines · On Wednesday 15 January 2014 08:55:05 Simon Horman wrote:

Quoted 21 lines · Ben Dooks

If the kernel is built to support multi-arm configurmation with shmobile
support built in, then the drivers/sh is not built. This contains drivers
that are essential to devices support by that configuration, including the
PM runtime code in drivers/sh/pm_runtime.c (which implicitly enables the
bus clocks for all devices).

If CONFIG_ARCH_SHMOBILE_MULTI then build the drivers/sh directory, but ensure that bits that may conflict (drivers/sh/clk if the common clock framework is not enabled) are built.

The ARCH_SHMOBILE_MULTI was added by efacfce5f8a ("ARM: shmobile:
Introduce ARCH_SHMOBILE_MULTI") but this has only just recently been found
due to changes currently only in Simon Horman's tree. This patch is a partial revert of bf98c1eac1d4a6b ("ARM: Rename ARCH_SHMOBILE to ARCH_SHMOBILE_LEGACY") to address the issue of drivers not being built.

Quoted 6 lines

Quoted 4 lines

It is also possible the drivers/sh/intc will also need to be built however the lack of intc is not causing a number of drivers to fail to properly manage their clocks. This is left as an future patch for that is perfectly fine. someone who understands that part of the code.

Laurent, what are you feelings on this?

If we end up needing an interrupt controller supported by drivers/sh/intc for
a multiplatform kernel I would rather like to port the driver to drivers/irqchip instead of compiling drivers/sh/intc for the platform.

Quoted 5 lines

Regarding drivers/sh/pm_runtime.c, compiling it for ARCH_SHMOBILE_MULTI will
cause multiplatform kernels running on non-Renesas platforms to add a pm clock
notifier. We need to at least add a runtime check.

Hmm, could we move that to the mstp code or add bindings to the specific architectures that need it?

Quoted 31 lines · Ben Dooks

Quoted 30 lines · Laurent Pinchart

Quoted 17 lines · On Wednesday 15 January 2014 08:55:05 Simon Horman wrote:

Quoted 16 lines · Ben Dooks

If the kernel is built to support multi-arm configurmation with shmobile support built in, then the drivers/sh is not built. This contains drivers that are essential to devices support by that configuration, including the PM runtime code in drivers/sh/pm_runtime.c (which implicitly enables the bus clocks for all devices).

If CONFIG_ARCH_SHMOBILE_MULTI then build the drivers/sh directory, but ensure that bits that may conflict (drivers/sh/clk if the common clock framework is not enabled) are built.

The ARCH_SHMOBILE_MULTI was added by efacfce5f8a ("ARM: shmobile:
Introduce ARCH_SHMOBILE_MULTI") but this has only just recently been found due to changes currently only in Simon Horman's tree. This patch is a partial revert of bf98c1eac1d4a6b ("ARM: Rename ARCH_SHMOBILE to ARCH_SHMOBILE_LEGACY") to address the issue of drivers not being built.

Quoted 6 lines

Quoted 4 lines

It is also possible the drivers/sh/intc will also need to be built however the lack of intc is not causing a number of drivers to fail to properly manage their clocks. This is left as an future patch for that is perfectly fine. someone who understands that part of the code.

Laurent, what are you feelings on this?

If we end up needing an interrupt controller supported by drivers/sh/intc for a multiplatform kernel I would rather like to port the driver to drivers/irqchip instead of compiling drivers/sh/intc for the platform.

Quoted 6 lines

Quoted 3 lines

Regarding drivers/sh/pm_runtime.c, compiling it for ARCH_SHMOBILE_MULTI will cause multiplatform kernels running on non-Renesas platforms to add a pm clock notifier. We need to at least add a runtime check.

Hmm, could we move that to the mstp code or add bindings to the specific architectures that need it?

I was having a think, and how about adding the following to each driver that expects clock management to happen for it, such as the following in the probe sequence:

	pm_runtime_manage_clock(dev);

This would mean the following:

- People would know a driver had its clock managed elsewhere
- You couldn't build a system with the clock_ops disabled.
- A system where a mix of drivers where used would work fine
- drivers/sh/pm_runtime.c could be deleted too.

Quoted 4 lines · On Thursday 16 January 2014 17:25:09 Ben Dooks wrote:

I was having a think, and how about adding the following to each driver that expects clock management to happen for it, such as the following in the probe sequence:

Quoted 8 lines

	pm_runtime_manage_clock(dev);

This would mean the following:

- People would know a driver had its clock managed elsewhere
- You couldn't build a system with the clock_ops disabled.
- A system where a mix of drivers where used would work fine
- drivers/sh/pm_runtime.c could be deleted too.

That sounds like a good idea to me. I like how drivers will be responsible for explicitly delegating clock handling to generic code. This combines simplicity with flexibility, and doesn't hide clock handling.

Mark, Rafael, any opinion ?

Quoted 15 lines · Laurent Pinchart

Quoted 4 lines · On Thursday 16 January 2014 17:25:09 Ben Dooks wrote:

I was having a think, and how about adding the following to each driver that expects clock management to happen for it, such as the following in the probe sequence:

Quoted 8 lines

	pm_runtime_manage_clock(dev);

This would mean the following:

- People would know a driver had its clock managed elsewhere
- You couldn't build a system with the clock_ops disabled.
- A system where a mix of drivers where used would work fine
- drivers/sh/pm_runtime.c could be deleted too.

Quoted 5 lines

That sounds like a good idea to me. I like how drivers will be responsible for explicitly delegating clock handling to generic code. This combines simplicity with flexibility, and doesn't hide clock handling.

Mark, Rafael, any opinion ?

I think that just makes things more complicated and isn't adding anything over pm_runtime_enable(), it's just boilerplate code. In theory essentially every driver running on platforms which don't have explicit management of core IP clocks ought to be calling this since potentially the IP might be deployed on another platform which does have clock management (this does actually happen with things like the DesignWare IPs) and it doesn't do anything like say which clocks are expected to be managed in this way which is another thing that can come up when moving devices between platforms.

I'm also struggling to see how it provides any sort of build time protection, it would allow the generation of a warning at runtime at best.

As far as I can tell the problem that Ben has seen here is that the platform really, really needs the code for its power domains running to be functional (this doesn't seem unreasonable and may not be related to clocks, this may be required to have the IPs powered up at all). I'd expect this is something for the platform to sort out rather than something for individual drivers to have to carry code for.

If it was going to be drivers carrying code for this I would expect it to be something like providing a list of clocks to be managed along with runtime PM - this would also make the code more widely applicable since it's quite common for the runtime PM callbacks to do nothing more than just enable and disable clocks.

Quoted 9 lines · Mark Brown

I think that just makes things more complicated and isn't adding anything over pm_runtime_enable(), it's just boilerplate code. In theory essentially every driver running on platforms which don't have explicit management of core IP clocks ought to be calling this since potentially the IP might be deployed on another platform which does have clock management (this does actually happen with things like the DesignWare IPs) and it doesn't do anything like say which clocks are expected to be managed in this way which is another thing that can come up when moving devices between platforms.

That sounds like a real headache where you have two sets of code looking at possibly the same clocks, and behaving differently between different platforms.

Quoted 3 lines

I'm also struggling to see how it provides any sort of build time protection, it would allow the generation of a warning at runtime at best.

If you don't have the code it WILL NOT LINK. At the moment it is entirely possible to link a kernel which will produce a set of confusing errors as drivers fail to initialise at startup.

We've already had other people run into the issue where they do not know why a driver is failing to work (rcar-thermal is one) as the code that was managing the clock for it magically vanished during the development cycle.

Quoted 6 lines

As far as I can tell the problem that Ben has seen here is that the platform really, really needs the code for its power domains running to be functional (this doesn't seem unreasonable and may not be related to clocks, this may be required to have the IPs powered up at all). I'd expect this is something for the platform to sort out rather than something for individual drivers to have to carry code for.

What's power domains got to do with this? You keep bringing this up but the error is purely to do with clock management. The code happens to be sitting in the drivers/base/pm directory, but could easily sit elsewhere.

Quoted 5 lines

If it was going to be drivers carrying code for this I would expect it to be something like providing a list of clocks to be managed along with runtime PM - this would also make the code more widely applicable since it's quite common for the runtime PM callbacks to do nothing more than just enable and disable clocks.

If we are really saying that bus clocks should not be managed by the drivers, then we should just enable this across the whole kernel and remove the management from any extant drivers (or make it so that any drivers that must manage their bus clocks have a call to do so).

Quoted 12 lines · Ben Dooks

Quoted 9 lines · Mark Brown

I think that just makes things more complicated and isn't adding anything over pm_runtime_enable(), it's just boilerplate code. In theory essentially every driver running on platforms which don't have explicit management of core IP clocks ought to be calling this since potentially the IP might be deployed on another platform which does have clock management (this does actually happen with things like the DesignWare IPs) and it doesn't do anything like say which clocks are expected to be managed in this way which is another thing that can come up when moving devices between platforms.

Quoted 3 lines

That sounds like a real headache where you have two sets of code looking at possibly the same clocks, and behaving differently between different platforms.

Yup, it's usually only one set of code on the client side but it's annoying that platform integrations aren't done consistently in hardware and that this flows through into software.

Quoted 7 lines

Quoted 3 lines

I'm also struggling to see how it provides any sort of build time protection, it would allow the generation of a warning at runtime at best.

If you don't have the code it WILL NOT LINK. At the moment it is entirely possible to link a kernel which will produce a set of confusing errors as drivers fail to initialise at startup.

How will this prevent the code linking? I would expect the "manage the clocks via runtime PM" call to be a bit of core code, I wouldn't expect it to be provided by a specific platform otherwise we're in for fail in a multiplatform environment. Besides, if it fails to link there's some missing Kconfig anyway since randconfig is supposed to work.

Quoted 4 lines

We've already had other people run into the issue where they do not know why a driver is failing to work (rcar-thermal is one) as the code that was managing the clock for it magically vanished during the development cycle.

I agree there's a usability problem here, I just don't think that this is the best fix for it, I think it's breaking abstractions.

Quoted 11 lines

Quoted 6 lines

As far as I can tell the problem that Ben has seen here is that the platform really, really needs the code for its power domains running to be functional (this doesn't seem unreasonable and may not be related to clocks, this may be required to have the IPs powered up at all). I'd expect this is something for the platform to sort out rather than something for individual drivers to have to carry code for.

What's power domains got to do with this? You keep bringing this up but the error is purely to do with clock management. The code happens to be sitting in the drivers/base/pm directory, but could easily sit elsewhere.

What these platforms are saying is esssentially that the clocks are part of the power domain - they're a fundamental part of getting the IP available for use so there is no point in managing them separately. This is where I think the abstraction problem is coming, to me it seems like the platform isn't successfully ensuring that the code to manage the power domain is there. The fact that part of what's missing is the clocks shouldn't matter to the drivers, in doing things like this the platform is trying to abstract that detail away from the drivers.

Quoted 11 lines

Quoted 5 lines

If it was going to be drivers carrying code for this I would expect it to be something like providing a list of clocks to be managed along with runtime PM - this would also make the code more widely applicable since it's quite common for the runtime PM callbacks to do nothing more than just enable and disable clocks.

If we are really saying that bus clocks should not be managed by the drivers, then we should just enable this across the whole kernel and remove the management from any extant drivers (or make it so that any drivers that must manage their bus clocks have a call to do so).

Yes, I think that would be a useful way to go since it would factor out some common code patterns that keep cropping up (as I suggested to you previously). It may not just be the main IP clocks that the drivers are managing like this (some drivers end up managing other clocks at the same time even if they could be more flexible) so if it was something drivers could make use of themselves that'd be good.

I've added linux-arm-kernel to the list to get a wider view from people who actively use the clock subsystem.

Quoted 75 lines · Mark Brown

Quoted 11 lines · Ben Dooks

Quoted 9 lines · Mark Brown

I think that just makes things more complicated and isn't adding anything over pm_runtime_enable(), it's just boilerplate code. In theory essentially every driver running on platforms which don't have explicit management of core IP clocks ought to be calling this since potentially the IP might be deployed on another platform which does have clock management (this does actually happen with things like the DesignWare IPs) and it doesn't do anything like say which clocks are expected to be managed in this way which is another thing that can come up when moving devices between platforms.

Quoted 3 lines

That sounds like a real headache where you have two sets of code looking at possibly the same clocks, and behaving differently between different platforms.

Yup, it's usually only one set of code on the client side but it's annoying that platform integrations aren't done consistently in hardware and that this flows through into software.

Quoted 7 lines

Quoted 3 lines

I'm also struggling to see how it provides any sort of build time protection, it would allow the generation of a warning at runtime at best.

If you don't have the code it WILL NOT LINK. At the moment it is entirely possible to link a kernel which will produce a set of confusing errors as drivers fail to initialise at startup.

How will this prevent the code linking? I would expect the "manage the clocks via runtime PM" call to be a bit of core code, I wouldn't expect it to be provided by a specific platform otherwise we're in for fail in a multiplatform environment. Besides, if it fails to link there's some missing Kconfig anyway since randconfig is supposed to work.

Quoted 4 lines

We've already had other people run into the issue where they do not know why a driver is failing to work (rcar-thermal is one) as the code that was managing the clock for it magically vanished during the development cycle.

I agree there's a usability problem here, I just don't think that this is the best fix for it, I think it's breaking abstractions.

Quoted 11 lines

Quoted 6 lines

As far as I can tell the problem that Ben has seen here is that the platform really, really needs the code for its power domains running to be functional (this doesn't seem unreasonable and may not be related to clocks, this may be required to have the IPs powered up at all). I'd expect this is something for the platform to sort out rather than something for individual drivers to have to carry code for.

What's power domains got to do with this? You keep bringing this up but the error is purely to do with clock management. The code happens to be sitting in the drivers/base/pm directory, but could easily sit elsewhere.

What these platforms are saying is esssentially that the clocks are part of the power domain - they're a fundamental part of getting the IP available for use so there is no point in managing them separately. This is where I think the abstraction problem is coming, to me it seems like the platform isn't successfully ensuring that the code to manage the power domain is there. The fact that part of what's missing is the clocks shouldn't matter to the drivers, in doing things like this the platform is trying to abstract that detail away from the drivers.

Quoted 11 lines

Quoted 5 lines

If it was going to be drivers carrying code for this I would expect it to be something like providing a list of clocks to be managed along with runtime PM - this would also make the code more widely applicable since it's quite common for the runtime PM callbacks to do nothing more than just enable and disable clocks.

If we are really saying that bus clocks should not be managed by the drivers, then we should just enable this across the whole kernel and remove the management from any extant drivers (or make it so that any drivers that must manage their bus clocks have a call to do so).

Quoted 6 lines

Yes, I think that would be a useful way to go since it would factor out some common code patterns that keep cropping up (as I suggested to you previously). It may not just be the main IP clocks that the drivers are managing like this (some drivers end up managing other clocks at the same time even if they could be more flexible) so if it was something drivers could make use of themselves that'd be good.

At the moment, the code to actually use the support is sitting in drivers/sh and gets initialised if it is built. This is the source of the original confusions.

I think it would be ok if drivers could /opt out/

if the bus clock handling was done like this for all platforms and we simply noted that the pm code is doing it. At that point it would be nice just to build the code in drivers/base/power unconditionally.

At the moment we could quite easily enable this code for all the ARM platforms as the clk framework should deal nicely with properly balanced clk_enable() and clk_disable() calls.

Enabling it across the board would also remove any issues with changes in platform behaviour and find any bugs sooner rather than later.

The problem isn't as simple as it seems, and more advanced implementations that would allow listing clocks that should be managed automatically (or the other way around) would also add another level of complexity. The required information is platform-dependent, but we currently don't express it as such in DT.

Quoted 5 lines · Laurent Pinchart

The problem isn't as simple as it seems, and more advanced implementations that would allow listing clocks that should be managed automatically (or the other way around) would also add another level of complexity. The required information is platform-dependent, but we currently don't express it as such in DT.

Well, the set of clocks an IP requires will tend to be the same - it's normally just that integrators may have done things like tie them together or decide to spread confusion by renaming them.

Quoted 7 lines · On Monday 20 January 2014 15:56:43 Mark Brown wrote:

Quoted 5 lines · Laurent Pinchart

The problem isn't as simple as it seems, and more advanced implementations that would allow listing clocks that should be managed automatically (or the other way around) would also add another level of complexity. The required information is platform-dependent, but we currently don't express it as such in DT.

Quoted 3 lines

Well, the set of clocks an IP requires will tend to be the same - it's normally just that integrators may have done things like tie them together or decide to spread confusion by renaming them.

That's the problem :-) How should the runtime PM core be given the list of clocks it needs to manage ? That information needs to come from somewhere.

Quoted 8 lines · Laurent Pinchart

Quoted 7 lines · On Monday 20 January 2014 15:56:43 Mark Brown wrote:

Quoted 5 lines · Laurent Pinchart

The problem isn't as simple as it seems, and more advanced implementations that would allow listing clocks that should be managed automatically (or the other way around) would also add another level of complexity. The required information is platform-dependent, but we currently don't express it as such in DT.

Quoted 6 lines

Quoted 3 lines

Well, the set of clocks an IP requires will tend to be the same - it's normally just that integrators may have done things like tie them together or decide to spread confusion by renaming them.

That's the problem :-) How should the runtime PM core be given the list of clocks it needs to manage ? That information needs to come from somewhere.

Stirring the pot again...

Which clocks a device needs is expressed in DT with CCF.
In the simple case, the runtime PM core can just control them based on device use.
In the complex case, the driver can regain control using its own pm callbacks, right?

Probably I'm still missing something, as I haven't had enough exposure to runtime PM and CCF ;-)

Quoted 17 lines · On Tuesday 11 March 2014 20:15:04 Geert Uytterhoeven wrote:

Quoted 8 lines · Laurent Pinchart

Quoted 7 lines · On Monday 20 January 2014 15:56:43 Mark Brown wrote:

Quoted 5 lines · Laurent Pinchart

The problem isn't as simple as it seems, and more advanced implementations that would allow listing clocks that should be managed automatically (or the other way around) would also add another level of complexity. The required information is platform- dependent, but we currently don't express it as such in DT.

Quoted 7 lines

Quoted 3 lines

Well, the set of clocks an IP requires will tend to be the same - it's normally just that integrators may have done things like tie them together or decide to spread confusion by renaming them.

That's the problem :-) How should the runtime PM core be given the list of clocks it needs to manage ? That information needs to come from somewhere.

Quoted 7 lines

Stirring the pot again...

Which clocks a device needs is expressed in DT with CCF.
In the simple case, the runtime PM core can just control them based on device use.
In the complex case, the driver can regain control using its own pm callbacks, right?

Sure, the driver can of course control the clocks manually in its PM callbacks if it needs to. The point, however, is to control the clocks from core code whenever possible. We thus need to define exact semantics to make sure each side knows what tasks to perform, and what to expect from the other side. For instance, in the DT case, the runtime PM core can easily get the list of the clocks used by the device from its DT node, but how can it know which clock(s) it should manage automatically and which clock(s) it should leave for the driver to control ?

Quoted 2 lines

Probably I'm still missing something, as I haven't had enough exposure to runtime PM and CCF ;-)