[PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[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 <linux-kernel@vger.kernel.org>
Cc: Linus SH list <linux-sh@vger.kernel.org>
Cc: Simon Horman <horms@verge.net.au>
Cc: Magnus Damm <magnus.damm@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/Makefile    | 1 +
 drivers/sh/Makefile | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/Makefile b/drivers/Makefile
index 8e3b8b0..abc4744 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -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/
 ifndef CONFIG_ARCH_USES_GETTIMEOFFSET
 obj-y				+= clocksource/
diff --git a/drivers/sh/Makefile b/drivers/sh/Makefile
index fc67f56..86604a5 100644
--- a/drivers/sh/Makefile
+++ b/drivers/sh/Makefile
@@ -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/
 
-- 
1.8.5.2

[PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[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 <linux-kernel@vger.kernel.org>
Cc: Linus SH list <linux-sh@vger.kernel.org>
Cc: Simon Horman <horms@verge.net.au>
Cc: Magnus Damm <magnus.damm@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/Makefile    | 1 +
 drivers/sh/Makefile | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/Makefile b/drivers/Makefile
index 8e3b8b0..abc4744 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -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/
 ifndef CONFIG_ARCH_USES_GETTIMEOFFSET
 obj-y				+= clocksource/
diff --git a/drivers/sh/Makefile b/drivers/sh/Makefile
index fc67f56..86604a5 100644
--- a/drivers/sh/Makefile
+++ b/drivers/sh/Makefile
@@ -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/
 
-- 
1.8.5.2

[PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[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 <linux-kernel@vger.kernel.org>
Cc: Linus SH list <linux-sh@vger.kernel.org>
Cc: Simon Horman <horms@verge.net.au>
Cc: Magnus Damm <magnus.damm@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/Makefile    | 1 +
 drivers/sh/Makefile | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/Makefile b/drivers/Makefile
index 8e3b8b0..abc4744 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -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/
 ifndef CONFIG_ARCH_USES_GETTIMEOFFSET
 obj-y				+= clocksource/
diff --git a/drivers/sh/Makefile b/drivers/sh/Makefile
index fc67f56..86604a5 100644
--- a/drivers/sh/Makefile
+++ b/drivers/sh/Makefile
@@ -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/
 
-- 
1.8.5.2

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Laurent Pinchart]

Hi Ben,

Thank you for the patch.

On Saturday 11 January 2014 13:06:29 Ben Dooks wrote:
> 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 <linux-kernel@vger.kernel.org>
> Cc: Linus SH list <linux-sh@vger.kernel.org>
> Cc: Simon Horman <horms@verge.net.au>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  drivers/Makefile    | 1 +
>  drivers/sh/Makefile | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 8e3b8b0..abc4744 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -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/
>  ifndef CONFIG_ARCH_USES_GETTIMEOFFSET
>  obj-y				+= clocksource/
> diff --git a/drivers/sh/Makefile b/drivers/sh/Makefile
> index fc67f56..86604a5 100644
> --- a/drivers/sh/Makefile
> +++ b/drivers/sh/Makefile
> @@ -3,7 +3,10 @@
>  #
>  obj-y	:= intc/

Is intc needed as well ?

> 
> +ifeq ($(CONFIG_COMMON_CLK),n)
>  obj-$(CONFIG_HAVE_CLK)		+= clk/
> +endif
> +
>  obj-$(CONFIG_MAPLE)		+= maple/
>  obj-$(CONFIG_SUPERHYWAY)	+= superhyway/
-- 
Regards,

Laurent Pinchart

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Laurent Pinchart]

Hi Ben,

Thank you for the patch.

On Saturday 11 January 2014 13:06:29 Ben Dooks wrote:
> 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 <linux-kernel@vger.kernel.org>
> Cc: Linus SH list <linux-sh@vger.kernel.org>
> Cc: Simon Horman <horms@verge.net.au>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  drivers/Makefile    | 1 +
>  drivers/sh/Makefile | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 8e3b8b0..abc4744 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -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/
>  ifndef CONFIG_ARCH_USES_GETTIMEOFFSET
>  obj-y				+= clocksource/
> diff --git a/drivers/sh/Makefile b/drivers/sh/Makefile
> index fc67f56..86604a5 100644
> --- a/drivers/sh/Makefile
> +++ b/drivers/sh/Makefile
> @@ -3,7 +3,10 @@
>  #
>  obj-y	:= intc/

Is intc needed as well ?

> 
> +ifeq ($(CONFIG_COMMON_CLK),n)
>  obj-$(CONFIG_HAVE_CLK)		+= clk/
> +endif
> +
>  obj-$(CONFIG_MAPLE)		+= maple/
>  obj-$(CONFIG_SUPERHYWAY)	+= superhyway/
-- 
Regards,

Laurent Pinchart

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Laurent Pinchart]

Hi Ben,

On Sunday 12 January 2014 22:54:15 Laurent Pinchart wrote:
> Hi Ben,
> 
> Thank you for the patch.
> 
> On Saturday 11 January 2014 13:06:29 Ben Dooks wrote:
> > 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).

Thinking a bit more about this, I think the approach taken in 
drivers/sh/pm_runtime.c isn't good. The code enables device clocks when 
devices are bound to a driver, increasing power consumption when devices are 
idle. Instead of enabling it for ARCH_SHMOBILE_MULTI I'd like to either add 
explicit clock support to drivers, or to integrate clocks with runtime PM 
only.

> > 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 <linux-kernel@vger.kernel.org>
> > Cc: Linus SH list <linux-sh@vger.kernel.org>
> > Cc: Simon Horman <horms@verge.net.au>
> > Cc: Magnus Damm <magnus.damm@gmail.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> > ---
> > 
> >  drivers/Makefile    | 1 +
> >  drivers/sh/Makefile | 3 +++
> >  2 files changed, 4 insertions(+)
> > 
> > diff --git a/drivers/Makefile b/drivers/Makefile
> > index 8e3b8b0..abc4744 100644
> > --- a/drivers/Makefile
> > +++ b/drivers/Makefile
> > @@ -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/
> >  ifndef CONFIG_ARCH_USES_GETTIMEOFFSET
> >  obj-y				+= clocksource/
> > 
> > diff --git a/drivers/sh/Makefile b/drivers/sh/Makefile
> > index fc67f56..86604a5 100644
> > --- a/drivers/sh/Makefile
> > +++ b/drivers/sh/Makefile
> > @@ -3,7 +3,10 @@
> > 
> >  #
> >  obj-y	:= intc/
> 
> Is intc needed as well ?
> 
> > +ifeq ($(CONFIG_COMMON_CLK),n)
> > 
> >  obj-$(CONFIG_HAVE_CLK)		+= clk/
> > 
> > +endif
> > +
> > 
> >  obj-$(CONFIG_MAPLE)		+= maple/
> >  obj-$(CONFIG_SUPERHYWAY)	+= superhyway/
-- 
Regards,

Laurent Pinchart

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Laurent Pinchart]

Hi Ben,

On Sunday 12 January 2014 22:54:15 Laurent Pinchart wrote:
> Hi Ben,
> 
> Thank you for the patch.
> 
> On Saturday 11 January 2014 13:06:29 Ben Dooks wrote:
> > 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).

Thinking a bit more about this, I think the approach taken in 
drivers/sh/pm_runtime.c isn't good. The code enables device clocks when 
devices are bound to a driver, increasing power consumption when devices are 
idle. Instead of enabling it for ARCH_SHMOBILE_MULTI I'd like to either add 
explicit clock support to drivers, or to integrate clocks with runtime PM 
only.

> > 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 <linux-kernel@vger.kernel.org>
> > Cc: Linus SH list <linux-sh@vger.kernel.org>
> > Cc: Simon Horman <horms@verge.net.au>
> > Cc: Magnus Damm <magnus.damm@gmail.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> > ---
> > 
> >  drivers/Makefile    | 1 +
> >  drivers/sh/Makefile | 3 +++
> >  2 files changed, 4 insertions(+)
> > 
> > diff --git a/drivers/Makefile b/drivers/Makefile
> > index 8e3b8b0..abc4744 100644
> > --- a/drivers/Makefile
> > +++ b/drivers/Makefile
> > @@ -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/
> >  ifndef CONFIG_ARCH_USES_GETTIMEOFFSET
> >  obj-y				+= clocksource/
> > 
> > diff --git a/drivers/sh/Makefile b/drivers/sh/Makefile
> > index fc67f56..86604a5 100644
> > --- a/drivers/sh/Makefile
> > +++ b/drivers/sh/Makefile
> > @@ -3,7 +3,10 @@
> > 
> >  #
> >  obj-y	:= intc/
> 
> Is intc needed as well ?
> 
> > +ifeq ($(CONFIG_COMMON_CLK),n)
> > 
> >  obj-$(CONFIG_HAVE_CLK)		+= clk/
> > 
> > +endif
> > +
> > 
> >  obj-$(CONFIG_MAPLE)		+= maple/
> >  obj-$(CONFIG_SUPERHYWAY)	+= superhyway/
-- 
Regards,

Laurent Pinchart

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

On Fri, Jan 10, 2014 at 03:18:15PM +0000, Ben Dooks wrote:
> 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 <linux-kernel@vger.kernel.org>
> Cc: Linus SH list <linux-sh@vger.kernel.org>
> Cc: Simon Horman <horms@verge.net.au>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  drivers/Makefile    | 1 +
>  drivers/sh/Makefile | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 8e3b8b0..abc4744 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -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?

  obj-$(CONFIG_ARCH_SHMOBILE)	+= sh/

>  ifndef CONFIG_ARCH_USES_GETTIMEOFFSET
>  obj-y				+= clocksource/
> diff --git a/drivers/sh/Makefile b/drivers/sh/Makefile
> index fc67f56..86604a5 100644
> --- a/drivers/sh/Makefile
> +++ b/drivers/sh/Makefile
> @@ -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/
>  
> -- 
> 1.8.5.2
> 

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Simon Horman]

On Fri, Jan 10, 2014 at 03:18:15PM +0000, Ben Dooks wrote:
> 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 <linux-kernel@vger.kernel.org>
> Cc: Linus SH list <linux-sh@vger.kernel.org>
> Cc: Simon Horman <horms@verge.net.au>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  drivers/Makefile    | 1 +
>  drivers/sh/Makefile | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 8e3b8b0..abc4744 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -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?

  obj-$(CONFIG_ARCH_SHMOBILE)	+= sh/

>  ifndef CONFIG_ARCH_USES_GETTIMEOFFSET
>  obj-y				+= clocksource/
> diff --git a/drivers/sh/Makefile b/drivers/sh/Makefile
> index fc67f56..86604a5 100644
> --- a/drivers/sh/Makefile
> +++ b/drivers/sh/Makefile
> @@ -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/
>  
> -- 
> 1.8.5.2
> 

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Ben Dooks]

On 13/01/14 00:30, Simon Horman wrote:
> On Fri, Jan 10, 2014 at 03:18:15PM +0000, Ben Dooks wrote:
>> 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 <linux-kernel@vger.kernel.org>
>> Cc: Linus SH list <linux-sh@vger.kernel.org>
>> Cc: Simon Horman <horms@verge.net.au>
>> Cc: Magnus Damm <magnus.damm@gmail.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>>   drivers/Makefile    | 1 +
>>   drivers/sh/Makefile | 3 +++
>>   2 files changed, 4 insertions(+)
>>
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index 8e3b8b0..abc4744 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -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?

I think this is probably a better fix. I am wondering what the original
logic of not doing this was.


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Ben Dooks]

On 12/01/14 22:01, Laurent Pinchart wrote:
> Hi Ben,
>
> On Sunday 12 January 2014 22:54:15 Laurent Pinchart wrote:
>> Hi Ben,
>>
>> Thank you for the patch.
>>
>> On Saturday 11 January 2014 13:06:29 Ben Dooks wrote:
>>> 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).
>
> Thinking a bit more about this, I think the approach taken in
> drivers/sh/pm_runtime.c isn't good. The code enables device clocks when
> devices are bound to a driver, increasing power consumption when devices are
> idle. Instead of enabling it for ARCH_SHMOBILE_MULTI I'd like to either add
> explicit clock support to drivers, or to integrate clocks with runtime PM
> only.

If pm-runtime is enabled, then I believe that the device clocks are
kept in sync with the active state of the device, which means that
they should be shut down when the device is not needed. There have
been recent discussions about this with respect to the PCI bridges
used by the USB host system.

Given the above, I'm not sure exactly what you mean by the statement
"I think the approach taken in drivers/sh/pm_runtime.c isn't good."
as if we're going to abstract the clock management we have the following
issues.

- If pm-runtime is not enabled then we need something to manage the
   clocks for the driver. If we put that code in the driver then there
   is not a lot of point in having the pm-runtime clock code here as
   the driver really only needs a helper to turn them on and off at
   the right place.

- If just standard power management is enabled, then do we really care
   about the power consumption of leaving peripherals running when their
   devices are bound? Managing the device clock optimally is hardly
   a concern if device drivers are not going to be idled when they are
   not being used.

When discussing this on freenode's #armkernel channel, several people
including Mark Brown wanted to keep this as it made driver's handling
of clocks much easier (there was no longer any need to deal with the
clk code when writing a simple driver). My view is it is a pain as we
now have a mix of drivers which expect to do their own clock work and
some that do not. (It is possible there are even some shmobile drivers
that still do their own clock management).

Personally I do not like hiding the implementation of this, as it ends
up confusing people when they first come to it.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Geert Uytterhoeven]

On Fri, Jan 10, 2014 at 4:18 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> --- a/drivers/sh/Makefile
> +++ b/drivers/sh/Makefile
> @@ -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'
drivers/built-in.o: In function `sci_port_disable':
clkdev.c:(.text+0x2ae30): undefined reference to `clk_disable'
clkdev.c:(.text+0x2ae38): undefined reference to `clk_disable'
drivers/built-in.o: In function `sci_port_enable':
clkdev.c:(.text+0x2ae74): undefined reference to `clk_enable'
clkdev.c:(.text+0x2ae7c): undefined reference to `clk_get_rate'
clkdev.c:(.text+0x2ae88): undefined reference to `clk_enable'
drivers/built-in.o: In function `sci_notifier':
clkdev.c:(.text+0x2c85c): undefined reference to `clk_get_rate'
drivers/built-in.o: In function `__pm_clk_remove':
clkdev.c:(.text+0x40844): undefined reference to `clk_disable'
drivers/built-in.o: In function `pm_clk_suspend':
clkdev.c:(.text+0x40c6c): undefined reference to `clk_disable'
drivers/built-in.o: In function `pm_clk_resume':
clkdev.c:(.text+0x40ce0): undefined reference to `clk_enable'
drivers/built-in.o: In function `qspi_set_config_register':
clkdev.c:(.text+0x515a4): undefined reference to `clk_get_rate'
drivers/built-in.o: In function `rspi_set_config_register':
clkdev.c:(.text+0x516dc): undefined reference to `clk_get_rate'
drivers/built-in.o: In function `rspi_remove':
clkdev.c:(.text+0x518ac): undefined reference to `clk_disable'
drivers/built-in.o: In function `rspi_probe':
clkdev.c:(.text+0x51a20): undefined reference to `clk_enable'
clkdev.c:(.text+0x51d60): undefined reference to `clk_disable'
drivers/built-in.o: In function `hspi_transfer_one_message':
clkdev.c:(.text+0x5312c): undefined reference to `clk_get_rate'
drivers/built-in.o: In function `sh_msiof_spi_txrx':
clkdev.c:(.text+0x53d90): undefined reference to `clk_get_rate'
drivers/built-in.o: In function `sh_msiof_spi_chipselect':
clkdev.c:(.text+0x54200): undefined reference to `clk_enable'
clkdev.c:(.text+0x542b8): undefined reference to `clk_disable'
drivers/built-in.o: In function `sh_cmt_start':
clkdev.c:(.text+0x6f900): undefined reference to `clk_enable'
clkdev.c:(.text+0x6f93c): undefined reference to `clk_get_rate'
clkdev.c:(.text+0x6f95c): undefined reference to `clk_get_rate'
clkdev.c:(.text+0x6fa08): undefined reference to `clk_disable'
drivers/built-in.o: In function `sh_cmt_stop':
clkdev.c:(.text+0x6fc14): undefined reference to `clk_disable'
drivers/built-in.o: In function `__sh_tmu_disable':
clkdev.c:(.text+0x70590): undefined reference to `clk_disable'
drivers/built-in.o: In function `__sh_tmu_enable':
clkdev.c:(.text+0x70980): undefined reference to `clk_enable'
clkdev.c:(.text+0x709f0): undefined reference to `clk_get_rate'
drivers/built-in.o: In function `em_sti_start':
clkdev.c:(.text+0x70f44): undefined reference to `clk_enable'
clkdev.c:(.text+0x70f68): undefined reference to `clk_get_rate'
drivers/built-in.o: In function `em_sti_stop':
clkdev.c:(.text+0x710d0): undefined reference to `clk_disable'
arch/arm/mach-shmobile/built-in.o: In function `shmobile_clk_init':
platsmp-apmu.c:(.init.text+0x7ac): undefined reference to
`recalculate_root_clocks'
platsmp-apmu.c:(.init.text+0x7b0): undefined reference to
`clk_enable_init_clocks'
arch/arm/mach-shmobile/built-in.o: In function `r8a7791_clock_init':
platsmp-apmu.c:(.init.text+0x8fc): undefined reference to `clk_register'
platsmp-apmu.c:(.init.text+0x920): undefined reference to `sh_clk_mstp_register'
arch/arm/mach-shmobile/built-in.o:(.data+0x16f0): undefined reference
to `followparent_recalc'

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

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Geert Uytterhoeven]

On Fri, Jan 10, 2014 at 4:18 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> --- a/drivers/sh/Makefile
> +++ b/drivers/sh/Makefile
> @@ -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'
drivers/built-in.o: In function `sci_port_disable':
clkdev.c:(.text+0x2ae30): undefined reference to `clk_disable'
clkdev.c:(.text+0x2ae38): undefined reference to `clk_disable'
drivers/built-in.o: In function `sci_port_enable':
clkdev.c:(.text+0x2ae74): undefined reference to `clk_enable'
clkdev.c:(.text+0x2ae7c): undefined reference to `clk_get_rate'
clkdev.c:(.text+0x2ae88): undefined reference to `clk_enable'
drivers/built-in.o: In function `sci_notifier':
clkdev.c:(.text+0x2c85c): undefined reference to `clk_get_rate'
drivers/built-in.o: In function `__pm_clk_remove':
clkdev.c:(.text+0x40844): undefined reference to `clk_disable'
drivers/built-in.o: In function `pm_clk_suspend':
clkdev.c:(.text+0x40c6c): undefined reference to `clk_disable'
drivers/built-in.o: In function `pm_clk_resume':
clkdev.c:(.text+0x40ce0): undefined reference to `clk_enable'
drivers/built-in.o: In function `qspi_set_config_register':
clkdev.c:(.text+0x515a4): undefined reference to `clk_get_rate'
drivers/built-in.o: In function `rspi_set_config_register':
clkdev.c:(.text+0x516dc): undefined reference to `clk_get_rate'
drivers/built-in.o: In function `rspi_remove':
clkdev.c:(.text+0x518ac): undefined reference to `clk_disable'
drivers/built-in.o: In function `rspi_probe':
clkdev.c:(.text+0x51a20): undefined reference to `clk_enable'
clkdev.c:(.text+0x51d60): undefined reference to `clk_disable'
drivers/built-in.o: In function `hspi_transfer_one_message':
clkdev.c:(.text+0x5312c): undefined reference to `clk_get_rate'
drivers/built-in.o: In function `sh_msiof_spi_txrx':
clkdev.c:(.text+0x53d90): undefined reference to `clk_get_rate'
drivers/built-in.o: In function `sh_msiof_spi_chipselect':
clkdev.c:(.text+0x54200): undefined reference to `clk_enable'
clkdev.c:(.text+0x542b8): undefined reference to `clk_disable'
drivers/built-in.o: In function `sh_cmt_start':
clkdev.c:(.text+0x6f900): undefined reference to `clk_enable'
clkdev.c:(.text+0x6f93c): undefined reference to `clk_get_rate'
clkdev.c:(.text+0x6f95c): undefined reference to `clk_get_rate'
clkdev.c:(.text+0x6fa08): undefined reference to `clk_disable'
drivers/built-in.o: In function `sh_cmt_stop':
clkdev.c:(.text+0x6fc14): undefined reference to `clk_disable'
drivers/built-in.o: In function `__sh_tmu_disable':
clkdev.c:(.text+0x70590): undefined reference to `clk_disable'
drivers/built-in.o: In function `__sh_tmu_enable':
clkdev.c:(.text+0x70980): undefined reference to `clk_enable'
clkdev.c:(.text+0x709f0): undefined reference to `clk_get_rate'
drivers/built-in.o: In function `em_sti_start':
clkdev.c:(.text+0x70f44): undefined reference to `clk_enable'
clkdev.c:(.text+0x70f68): undefined reference to `clk_get_rate'
drivers/built-in.o: In function `em_sti_stop':
clkdev.c:(.text+0x710d0): undefined reference to `clk_disable'
arch/arm/mach-shmobile/built-in.o: In function `shmobile_clk_init':
platsmp-apmu.c:(.init.text+0x7ac): undefined reference to
`recalculate_root_clocks'
platsmp-apmu.c:(.init.text+0x7b0): undefined reference to
`clk_enable_init_clocks'
arch/arm/mach-shmobile/built-in.o: In function `r8a7791_clock_init':
platsmp-apmu.c:(.init.text+0x8fc): undefined reference to `clk_register'
platsmp-apmu.c:(.init.text+0x920): undefined reference to `sh_clk_mstp_register'
arch/arm/mach-shmobile/built-in.o:(.data+0x16f0): undefined reference
to `followparent_recalc'

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

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Ben Dooks]

On 13/01/14 09:28, Geert Uytterhoeven wrote:
> On Fri, Jan 10, 2014 at 4:18 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>> --- a/drivers/sh/Makefile
>> +++ b/drivers/sh/Makefile
>> @@ -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.

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.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Geert Uytterhoeven]

Hi Ben,

On Mon, Jan 13, 2014 at 10:35 AM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> On 13/01/14 09:28, Geert Uytterhoeven wrote:
>> On Fri, Jan 10, 2014 at 4:18 PM, Ben Dooks <ben.dooks@codethink.co.uk>
>> wrote:
>>>
>>> --- a/drivers/sh/Makefile
>>> +++ b/drivers/sh/Makefile
>>> @@ -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.
>
> Hmm, thought undefined symbols got set to 'n'.

No, they're empty.
Doh, should have realized that myself...

> I can either fix this by
>
> ifneq ($(CONFIG_COMMON_CLK),y)
> endif

That's the typical pattern for this.

Sometimes people use

    obj-$(CONFIG_COMMON_CLK)$(CONFIG_HAVE_CLK) += clk/

but that's less readable.

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

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Geert Uytterhoeven]

Hi Ben,

On Mon, Jan 13, 2014 at 10:35 AM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> On 13/01/14 09:28, Geert Uytterhoeven wrote:
>> On Fri, Jan 10, 2014 at 4:18 PM, Ben Dooks <ben.dooks@codethink.co.uk>
>> wrote:
>>>
>>> --- a/drivers/sh/Makefile
>>> +++ b/drivers/sh/Makefile
>>> @@ -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.
>
> Hmm, thought undefined symbols got set to 'n'.

No, they're empty.
Doh, should have realized that myself...

> I can either fix this by
>
> ifneq ($(CONFIG_COMMON_CLK),y)
> endif

That's the typical pattern for this.

Sometimes people use

    obj-$(CONFIG_COMMON_CLK)$(CONFIG_HAVE_CLK) += clk/

but that's less readable.

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

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Laurent Pinchart]

Hi Ben,

On Monday 13 January 2014 06:45:36 Ben Dooks wrote:
> On 12/01/14 22:01, Laurent Pinchart wrote:
> > On Sunday 12 January 2014 22:54:15 Laurent Pinchart wrote:
> >> Hi Ben,
> >> 
> >> Thank you for the patch.
> >> 
> >> On Saturday 11 January 2014 13:06:29 Ben Dooks wrote:
> >>> 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).
> > 
> > Thinking a bit more about this, I think the approach taken in
> > drivers/sh/pm_runtime.c isn't good. The code enables device clocks when
> > devices are bound to a driver, increasing power consumption when devices
> > are idle. Instead of enabling it for ARCH_SHMOBILE_MULTI I'd like to
> > either add explicit clock support to drivers, or to integrate clocks with
> > runtime PM only.
> 
> If pm-runtime is enabled, then I believe that the device clocks are
> kept in sync with the active state of the device, which means that
> they should be shut down when the device is not needed. There have
> been recent discussions about this with respect to the PCI bridges
> used by the USB host system.
> 
> Given the above, I'm not sure exactly what you mean by the statement
> "I think the approach taken in drivers/sh/pm_runtime.c isn't good."

I might have read the code too fast. I was under the impression that the code 
enabled the device clock as soon as the device was bound to a driver. Upon 
closer inspection this doesn't seem to be the case.

> as if we're going to abstract the clock management we have the following
> issues.
> 
> - If pm-runtime is not enabled then we need something to manage the clocks
>   for the driver. If we put that code in the driver then there is not a lot
>   of point in having the pm-runtime clock code here as the driver really
>   only needs a helper to turn them on and off at the right place.
> 
> - If just standard power management is enabled, then do we really care
>   about the power consumption of leaving peripherals running when their
>   devices are bound? Managing the device clock optimally is hardly a concern
>   if device drivers are not going to be idled when they are not being used.

Many devices are not bound to a power domain handled by runtime PM. Adding a 
runtime PM dependency to clock handling for those devices doesn't make me 
really happy.

Does automatic clock handling even work at all with runtime PM disabled ? The 
clk_enable() call seems to come from pm_clk_resume() only, which is used as 
the runtime_resume handler. with runtime PM disabled that code looks like it 
won't be called at all.

> When discussing this on freenode's #armkernel channel, several people
> including Mark Brown wanted to keep this as it made driver's handling
> of clocks much easier (there was no longer any need to deal with the
> clk code when writing a simple driver). My view is it is a pain as we
> now have a mix of drivers which expect to do their own clock work and
> some that do not. (It is possible there are even some shmobile drivers
> that still do their own clock management).

We could remove manual clock handling from some drivers, but the drivers that 
need to handle several clocks will still need to do so manually. As long as 
the core code allows this (which I think it does, but I'm not too familiar 
with the code) I won't complain (too much at least :-)).

> Personally I do not like hiding the implementation of this, as it ends
> up confusing people when they first come to it.

I like explicit implementations as well, but I have to admit that devices that 
require a single clock and have clock requirements that are in sync with the 
PM runtime requirements would probably benefit from automatic clock handling, 
at least from a driver code complexity point of view, to the expense of a less 
explicit implementation.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Laurent Pinchart]

Hi Ben,

On Monday 13 January 2014 06:45:36 Ben Dooks wrote:
> On 12/01/14 22:01, Laurent Pinchart wrote:
> > On Sunday 12 January 2014 22:54:15 Laurent Pinchart wrote:
> >> Hi Ben,
> >> 
> >> Thank you for the patch.
> >> 
> >> On Saturday 11 January 2014 13:06:29 Ben Dooks wrote:
> >>> 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).
> > 
> > Thinking a bit more about this, I think the approach taken in
> > drivers/sh/pm_runtime.c isn't good. The code enables device clocks when
> > devices are bound to a driver, increasing power consumption when devices
> > are idle. Instead of enabling it for ARCH_SHMOBILE_MULTI I'd like to
> > either add explicit clock support to drivers, or to integrate clocks with
> > runtime PM only.
> 
> If pm-runtime is enabled, then I believe that the device clocks are
> kept in sync with the active state of the device, which means that
> they should be shut down when the device is not needed. There have
> been recent discussions about this with respect to the PCI bridges
> used by the USB host system.
> 
> Given the above, I'm not sure exactly what you mean by the statement
> "I think the approach taken in drivers/sh/pm_runtime.c isn't good."

I might have read the code too fast. I was under the impression that the code 
enabled the device clock as soon as the device was bound to a driver. Upon 
closer inspection this doesn't seem to be the case.

> as if we're going to abstract the clock management we have the following
> issues.
> 
> - If pm-runtime is not enabled then we need something to manage the clocks
>   for the driver. If we put that code in the driver then there is not a lot
>   of point in having the pm-runtime clock code here as the driver really
>   only needs a helper to turn them on and off at the right place.
> 
> - If just standard power management is enabled, then do we really care
>   about the power consumption of leaving peripherals running when their
>   devices are bound? Managing the device clock optimally is hardly a concern
>   if device drivers are not going to be idled when they are not being used.

Many devices are not bound to a power domain handled by runtime PM. Adding a 
runtime PM dependency to clock handling for those devices doesn't make me 
really happy.

Does automatic clock handling even work at all with runtime PM disabled ? The 
clk_enable() call seems to come from pm_clk_resume() only, which is used as 
the runtime_resume handler. with runtime PM disabled that code looks like it 
won't be called at all.

> When discussing this on freenode's #armkernel channel, several people
> including Mark Brown wanted to keep this as it made driver's handling
> of clocks much easier (there was no longer any need to deal with the
> clk code when writing a simple driver). My view is it is a pain as we
> now have a mix of drivers which expect to do their own clock work and
> some that do not. (It is possible there are even some shmobile drivers
> that still do their own clock management).

We could remove manual clock handling from some drivers, but the drivers that 
need to handle several clocks will still need to do so manually. As long as 
the core code allows this (which I think it does, but I'm not too familiar 
with the code) I won't complain (too much at least :-)).

> Personally I do not like hiding the implementation of this, as it ends
> up confusing people when they first come to it.

I like explicit implementations as well, but I have to admit that devices that 
require a single clock and have clock requirements that are in sync with the 
PM runtime requirements would probably benefit from automatic clock handling, 
at least from a driver code complexity point of view, to the expense of a less 
explicit implementation.

-- 
Regards,

Laurent Pinchart

[PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[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.

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
someone who understands that part of the code.

Cc: Linus SH list <linux-sh@vger.kernel.org>
Cc: laurent.pinchart+renesas@ideasonboard.com
Cc: Simon Horman <horms@verge.net.au>
Cc: Magnus Damm <magnus.damm@gmail.com>
Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/Makefile    | 2 +-
 drivers/sh/Makefile | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/Makefile b/drivers/Makefile
index 8e3b8b0..3cc8214 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -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
diff --git a/drivers/sh/Makefile b/drivers/sh/Makefile
index fc67f56..b4d588c 100644
--- a/drivers/sh/Makefile
+++ b/drivers/sh/Makefile
@@ -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/
 
-- 
1.8.5.2

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Simon Horman]

On Tue, Jan 14, 2014 at 01:56:16PM +0000, Ben Dooks wrote:
> 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.

Laurent, what are you feelings on this?

> Cc: Linus SH list <linux-sh@vger.kernel.org>
> Cc: laurent.pinchart+renesas@ideasonboard.com
> Cc: Simon Horman <horms@verge.net.au>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  drivers/Makefile    | 2 +-
>  drivers/sh/Makefile | 3 +++
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 8e3b8b0..3cc8214 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -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
> diff --git a/drivers/sh/Makefile b/drivers/sh/Makefile
> index fc67f56..b4d588c 100644
> --- a/drivers/sh/Makefile
> +++ b/drivers/sh/Makefile
> @@ -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/
>  
> -- 
> 1.8.5.2
> 

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Laurent Pinchart]

Hi Simon,

On Wednesday 15 January 2014 08:55:05 Simon Horman wrote:
> On Tue, Jan 14, 2014 at 01:56:16PM +0000, Ben Dooks wrote:
> > 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.
> 
> 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.

> > Cc: Linus SH list <linux-sh@vger.kernel.org>
> > Cc: laurent.pinchart+renesas@ideasonboard.com
> > Cc: Simon Horman <horms@verge.net.au>
> > Cc: Magnus Damm <magnus.damm@gmail.com>
> > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> > ---
> > 
> >  drivers/Makefile    | 2 +-
> >  drivers/sh/Makefile | 3 +++
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/Makefile b/drivers/Makefile
> > index 8e3b8b0..3cc8214 100644
> > --- a/drivers/Makefile
> > +++ b/drivers/Makefile
> > @@ -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
> > diff --git a/drivers/sh/Makefile b/drivers/sh/Makefile
> > index fc67f56..b4d588c 100644
> > --- a/drivers/sh/Makefile
> > +++ b/drivers/sh/Makefile
> > @@ -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/
-- 
Regards,

Laurent Pinchart

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Ben Dooks]

On 15/01/14 19:46, Laurent Pinchart wrote:
> Hi Simon,
>
> On Wednesday 15 January 2014 08:55:05 Simon Horman wrote:
>> On Tue, Jan 14, 2014 at 01:56:16PM +0000, Ben Dooks wrote:
>>> 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.
>>
>> 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.

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

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Ben Dooks]

On 16/01/14 10:38, Ben Dooks wrote:
> On 15/01/14 19:46, Laurent Pinchart wrote:
>> Hi Simon,
>>
>> On Wednesday 15 January 2014 08:55:05 Simon Horman wrote:
>>> On Tue, Jan 14, 2014 at 01:56:16PM +0000, Ben Dooks wrote:
>>>> 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.
>>>
>>> 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.
>
> 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.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1918 bytes --]

On Mon, Jan 13, 2014 at 06:45:36AM +0000, Ben Dooks wrote:

> - If pm-runtime is not enabled then we need something to manage the
>   clocks for the driver. If we put that code in the driver then there
>   is not a lot of point in having the pm-runtime clock code here as
>   the driver really only needs a helper to turn them on and off at
>   the right place.

Last time I looked at the code the runtime PM stuff was also being used
to manage actual power domains in at least some of those SoCs (which is
the more common use for power domains).  This was a while ago while I
was doing power domain support for s3c64xx though.

> When discussing this on freenode's #armkernel channel, several people
> including Mark Brown wanted to keep this as it made driver's handling
> of clocks much easier (there was no longer any need to deal with the
> clk code when writing a simple driver). My view is it is a pain as we
> now have a mix of drivers which expect to do their own clock work and
> some that do not. (It is possible there are even some shmobile drivers
> that still do their own clock management).

I don't massively care one way or another but it is a totally reasonable
decision for a platform to do this especially if the clocks are tied to
the power domains in some way, for example a single functional clock
shared over the domain.

The arguments people have for doing this have been more about removing
knowledge of the SoC integration from the driver - having the functional
clocks for the IP visible in the kernel can make IPs harder to share
with platforms that lack meaningful clock management - and factoring out
boilerplate code that just acquires, enables and disables clocks.

> Personally I do not like hiding the implementation of this, as it ends
> up confusing people when they first come to it.

It wouldn't do that if we did it all the time of course; there is an
argument for consistency.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Laurent Pinchart]

Hi Ben,

On Thursday 16 January 2014 17:25:09 Ben Dooks wrote:
> On 16/01/14 10:38, Ben Dooks wrote:
> > On 15/01/14 19:46, Laurent Pinchart wrote:
> >> On Wednesday 15 January 2014 08:55:05 Simon Horman wrote:
> >>> On Tue, Jan 14, 2014 at 01:56:16PM +0000, Ben Dooks wrote:
> >>>> 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.
> >>> 
> >>> 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.
> > 
> > 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.

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 ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2379 bytes --]

On Sun, Jan 19, 2014 at 10:44:53PM +0100, Laurent Pinchart wrote:
> 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:

> > 	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 ?

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Ben Dooks]

On 20/01/14 11:47, Mark Brown wrote:
> On Sun, Jan 19, 2014 at 10:44:53PM +0100, Laurent Pinchart wrote:
>> 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:
>
>>> 	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 ?
>
> 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.

> 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.

> 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.

> 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).

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 4365 bytes --]

On Mon, Jan 20, 2014 at 12:01:00PM +0000, Ben Dooks wrote:
> On 20/01/14 11:47, Mark Brown wrote:

> >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.

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.

> >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.

> 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.

> >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.

> >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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Ben Dooks]

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

On 20/01/14 12:54, Mark Brown wrote:
> On Mon, Jan 20, 2014 at 12:01:00PM +0000, Ben Dooks wrote:
>> On 20/01/14 11:47, Mark Brown wrote:
>
>>> 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.
>
> 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.
>
>>> 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.
>
>> 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.
>
>>> 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.
>
>>> 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.

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.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

[PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Ben Dooks]

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

On 20/01/14 12:54, Mark Brown wrote:
> On Mon, Jan 20, 2014 at 12:01:00PM +0000, Ben Dooks wrote:
>> On 20/01/14 11:47, Mark Brown wrote:
>
>>> 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.
>
> 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.
>
>>> 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.
>
>> 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.
>
>>> 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.
>
>>> 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.

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.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Laurent Pinchart]

On Monday 20 January 2014 13:19:07 Ben Dooks wrote:
> I've added linux-arm-kernel to the list to get a wider view from
> people who actively use the clock subsystem.
> 
> On 20/01/14 12:54, Mark Brown wrote:
> > On Mon, Jan 20, 2014 at 12:01:00PM +0000, Ben Dooks wrote:
> >> On 20/01/14 11:47, Mark Brown wrote:
> >>> 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.
> > 
> > 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.
> > 
> >>> 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.
> > 
> >> 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.
> > 
> >>> 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.
> > 
> >>> 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.
> 
> 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/

Most drivers have a single functional clock (if any) and only need it to be 
enabled/disabled in sync with runtime PM. As this requirements is common, 
making it the default sounds reasonable. Other drivers need to manage their 
clocks, or at least part of their clocks, manually. There are good and bad 
reasons for this, and some bad reasons come from driver code that should be 
fixed, but we can't ignore the problems for now.

The simplest implementation would be a way to let a driver opt-out completely. 
Unless a driver opts out, the runtime PM core will get the default clock for 
the device (with a NULL con ID) and manage it automatically. I'm not sure what 
to do when the runtime PM core fails to get the clock, as this can be caused 
by various problems or non-problems.

One particular problem that needs to be considered is deferred probing. An 
option would be to have all non-SoC devices opt-out from automatic clock 
management, but that will make the opt-out case the most common one. We also 
need to consider IP cores that can be present on-SoC or off-SoC in discrete 
chips.

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.

> 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.
-- 
Regards,

Laurent Pinchart

[PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Laurent Pinchart]

On Monday 20 January 2014 13:19:07 Ben Dooks wrote:
> I've added linux-arm-kernel to the list to get a wider view from
> people who actively use the clock subsystem.
> 
> On 20/01/14 12:54, Mark Brown wrote:
> > On Mon, Jan 20, 2014 at 12:01:00PM +0000, Ben Dooks wrote:
> >> On 20/01/14 11:47, Mark Brown wrote:
> >>> 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.
> > 
> > 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.
> > 
> >>> 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.
> > 
> >> 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.
> > 
> >>> 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.
> > 
> >>> 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.
> 
> 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/

Most drivers have a single functional clock (if any) and only need it to be 
enabled/disabled in sync with runtime PM. As this requirements is common, 
making it the default sounds reasonable. Other drivers need to manage their 
clocks, or at least part of their clocks, manually. There are good and bad 
reasons for this, and some bad reasons come from driver code that should be 
fixed, but we can't ignore the problems for now.

The simplest implementation would be a way to let a driver opt-out completely. 
Unless a driver opts out, the runtime PM core will get the default clock for 
the device (with a NULL con ID) and manage it automatically. I'm not sure what 
to do when the runtime PM core fails to get the clock, as this can be caused 
by various problems or non-problems.

One particular problem that needs to be considered is deferred probing. An 
option would be to have all non-SoC devices opt-out from automatic clock 
management, but that will make the opt-out case the most common one. We also 
need to consider IP cores that can be present on-SoC or off-SoC in discrete 
chips.

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.

> 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.
-- 
Regards,

Laurent Pinchart

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 598 bytes --]

On Mon, Jan 20, 2014 at 04:48:10PM +0100, Laurent Pinchart wrote:

> 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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Mark Brown]

On Mon, Jan 20, 2014 at 04:48:10PM +0100, Laurent Pinchart wrote:

> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140120/4673e8f6/attachment.sig>

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Laurent Pinchart]

[-- Attachment #1: Type: text/plain, Size: 870 bytes --]

Hi Mark,

On Monday 20 January 2014 15:56:43 Mark Brown wrote:
> On Mon, Jan 20, 2014 at 04:48:10PM +0100, Laurent Pinchart wrote:
> > 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.

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.

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Laurent Pinchart]

Hi Mark,

On Monday 20 January 2014 15:56:43 Mark Brown wrote:
> On Mon, Jan 20, 2014 at 04:48:10PM +0100, Laurent Pinchart wrote:
> > 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.

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.

-- 
Regards,

Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140120/a2303b43/attachment.sig>

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Geert Uytterhoeven]

Hi Laurent,

On Mon, Jan 20, 2014 at 11:52 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Monday 20 January 2014 15:56:43 Mark Brown wrote:
>> On Mon, Jan 20, 2014 at 04:48:10PM +0100, Laurent Pinchart wrote:
>> > 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.
>
> 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 ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Geert Uytterhoeven]

Hi Laurent,

On Mon, Jan 20, 2014 at 11:52 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Monday 20 January 2014 15:56:43 Mark Brown wrote:
>> On Mon, Jan 20, 2014 at 04:48:10PM +0100, Laurent Pinchart wrote:
>> > 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.
>
> 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 ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Laurent Pinchart]

Hi Geert,

On Tuesday 11 March 2014 20:15:04 Geert Uytterhoeven wrote:
> On Mon, Jan 20, 2014 at 11:52 PM, Laurent Pinchart wrote:
> > On Monday 20 January 2014 15:56:43 Mark Brown wrote:
> >> On Mon, Jan 20, 2014 at 04:48:10PM +0100, Laurent Pinchart wrote:
> >> > 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.
> > 
> > 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?

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 ?

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

-- 
Regards,

Laurent Pinchart

[PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Laurent Pinchart]

Hi Geert,

On Tuesday 11 March 2014 20:15:04 Geert Uytterhoeven wrote:
> On Mon, Jan 20, 2014 at 11:52 PM, Laurent Pinchart wrote:
> > On Monday 20 January 2014 15:56:43 Mark Brown wrote:
> >> On Mon, Jan 20, 2014 at 04:48:10PM +0100, Laurent Pinchart wrote:
> >> > 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.
> > 
> > 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?

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 ?

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Laurent Pinchart]

Hi Geert,

On Wednesday 12 March 2014 15:18:46 Laurent Pinchart wrote:
> On Tuesday 11 March 2014 20:15:04 Geert Uytterhoeven wrote:
> > On Mon, Jan 20, 2014 at 11:52 PM, Laurent Pinchart wrote:
> > > On Monday 20 January 2014 15:56:43 Mark Brown wrote:
> > >> On Mon, Jan 20, 2014 at 04:48:10PM +0100, Laurent Pinchart wrote:
> > >> > 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.
> > > 
> > > 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?
> 
> 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 ?
> 
> > Probably I'm still missing something, as I haven't had enough exposure to
> > runtime PM and CCF ;-)

If you haven't seen it already, https://lkml.org/lkml/2014/1/31/290 
("[RFC/PATCH] base: platform: add generic clock handling for platform-bus") 
might be related.

-- 
Regards,

Laurent Pinchart

[PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI

[Laurent Pinchart]

Hi Geert,

On Wednesday 12 March 2014 15:18:46 Laurent Pinchart wrote:
> On Tuesday 11 March 2014 20:15:04 Geert Uytterhoeven wrote:
> > On Mon, Jan 20, 2014 at 11:52 PM, Laurent Pinchart wrote:
> > > On Monday 20 January 2014 15:56:43 Mark Brown wrote:
> > >> On Mon, Jan 20, 2014 at 04:48:10PM +0100, Laurent Pinchart wrote:
> > >> > 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.
> > > 
> > > 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?
> 
> 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 ?
> 
> > Probably I'm still missing something, as I haven't had enough exposure to
> > runtime PM and CCF ;-)

If you haven't seen it already, https://lkml.org/lkml/2014/1/31/290 
("[RFC/PATCH] base: platform: add generic clock handling for platform-bus") 
might be related.

-- 
Regards,

Laurent Pinchart