Re: [PATCH v7 1/3] Documentation: common clk API

Re: [PATCH v7 1/3] Documentation: common clk API

[Paul Walmsley]

Hi

On Fri, 16 Mar 2012, Arnd Bergmann wrote:

> On Friday 16 March 2012, Arnd Bergmann wrote:
> > > 
> > > Can we shoe-horn this thing into 3.4 (it is a bit late, i know) so
> > > that platform ports can gather speed? Several people are waiting for a
> > > somewhat stable version before starting their ports.
> > > 
> > > And what is the path into mainline - will Russell carry it or Arnd
> > > (with Russell's blessing)?
> > 
> > I would suggest putting it into a late/* branch of arm-soc so it finally
> > gets some linux-next exposure, and then sending it in the second week of the
> > merge window.
> > 
> > Russell, please let me know if you want to carry it instead or if you
> > have found any last-minute show stoppers in the code.
> 
> FWIW, it's in arm-soc now [...]

If the common clock code is to go upstream now, it should be marked as 
experimental.  This is because we know the API is not well-defined, and 
that both the API and the underlying mechanics will almost certainly need 
to change for non-trivial uses of the rate changing code (e.g., DVFS with 
external I/O devices).

While I understand and accept the motivation to get the common clk code 
upstream now, if platforms start to use it, they need to be aware that the 
behavior of the code can change significantly.  These platforms may need 
to revalidate their implementations or change the way that many of their 
drivers use the clock functions.

It also seems important to recognize that significant changes are still 
coming into the common clk code (e.g., the clk_set_rate() changes that 
came in just a few days ago).

So, the following is a patch to mark it as experimental.  It applies on 
the current head of arm-soc/next/clk 
(9d9f78ed9af0e465d2fd15550471956e7f559b9f).  This should be a good 
compromise: it allows simple platforms or platforms with maintainers with 
lots of time to start converting over to the common clk code, while still 
clearly indicating that the API and underlying code is likely to change in 
significant ways.

Once at least two major SoC ports have DVFS with external I/O devices 
safely up and running on their mainline kernels with the common clk code, 
I'd suggest removing the experimental designation.

...

None of this is meant to reflect on Mike, by the way, who is doing a good 
job in a difficult situation.

 
- Paul

From: Paul Walmsley <paul@pwsan.com>
Date: Fri, 16 Mar 2012 16:06:30 -0600
Subject: [PATCH] clk: mark the common clk code as EXPERIMENTAL for now

Mark the common clk code as depending on CONFIG_EXPERIMENTAL.  The API
is not well-defined and both it and the underlying mechanics are likely
to need significant changes to support non-trivial uses of the rate
changing code, such as DVFS with external I/O devices.  So any platforms
that switch their implementation over to this may need to revise much
of their driver code and revalidate their implementations until the
behavior of the code is better-defined.

A good time for removing this EXPERIMENTAL designation would be after at
least two platforms that do DVFS on groups of external I/O devices have
ported their clock implementations over to the common clk code.

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Mike Turquette <mturquette@ti.com>
---
 drivers/clk/Kconfig |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 2eaf17e..a0a83de 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -12,6 +12,7 @@ config HAVE_MACH_CLKDEV
 menuconfig COMMON_CLK
 	bool "Common Clock Framework"
 	select HAVE_CLK_PREPARE
+	depends on EXPERIMENTAL
 	---help---
 	  The common clock framework is a single definition of struct
 	  clk, useful across many platforms, as well as an
-- 
1.7.9.1

Re: [PATCH v7 1/3] Documentation: common clk API

[Turquette, Mike]

On Fri, Mar 16, 2012 at 3:21 PM, Paul Walmsley <paul@pwsan.com> wrote:
> From: Paul Walmsley <paul@pwsan.com>
> Date: Fri, 16 Mar 2012 16:06:30 -0600
> Subject: [PATCH] clk: mark the common clk code as EXPERIMENTAL for now
>
> Mark the common clk code as depending on CONFIG_EXPERIMENTAL.  The API
> is not well-defined and both it and the underlying mechanics are likely
> to need significant changes to support non-trivial uses of the rate
> changing code, such as DVFS with external I/O devices.  So any platforms
> that switch their implementation over to this may need to revise much
> of their driver code and revalidate their implementations until the
> behavior of the code is better-defined.
>
> A good time for removing this EXPERIMENTAL designation would be after at
> least two platforms that do DVFS on groups of external I/O devices have
> ported their clock implementations over to the common clk code.
>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Cc: Mike Turquette <mturquette@ti.com>

ACK.  This will set some reasonable expectations while things are in flux.

Arnd are you willing to take this in?

Thanks,
Mike

> ---
>  drivers/clk/Kconfig |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 2eaf17e..a0a83de 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -12,6 +12,7 @@ config HAVE_MACH_CLKDEV
>  menuconfig COMMON_CLK
>        bool "Common Clock Framework"
>        select HAVE_CLK_PREPARE
> +       depends on EXPERIMENTAL
>        ---help---
>          The common clock framework is a single definition of struct
>          clk, useful across many platforms, as well as an
> --
> 1.7.9.1
>

Re: [PATCH v7 1/3] Documentation: common clk API

[Arnd Bergmann]

On Friday 16 March 2012, Turquette, Mike wrote:
> On Fri, Mar 16, 2012 at 3:21 PM, Paul Walmsley <paul-DWxLp4Yu+b8AvxtiuMwx3w@public.gmane.org> wrote:
> > From: Paul Walmsley <paul-DWxLp4Yu+b8AvxtiuMwx3w@public.gmane.org>
> > Date: Fri, 16 Mar 2012 16:06:30 -0600
> > Subject: [PATCH] clk: mark the common clk code as EXPERIMENTAL for now
> >
> > Mark the common clk code as depending on CONFIG_EXPERIMENTAL.  The API
> > is not well-defined and both it and the underlying mechanics are likely
> > to need significant changes to support non-trivial uses of the rate
> > changing code, such as DVFS with external I/O devices.  So any platforms
> > that switch their implementation over to this may need to revise much
> > of their driver code and revalidate their implementations until the
> > behavior of the code is better-defined.
> >
> > A good time for removing this EXPERIMENTAL designation would be after at
> > least two platforms that do DVFS on groups of external I/O devices have
> > ported their clock implementations over to the common clk code.
> >
> > Signed-off-by: Paul Walmsley <paul-DWxLp4Yu+b8AvxtiuMwx3w@public.gmane.org>
> > Cc: Mike Turquette <mturquette-l0cyMroinI0@public.gmane.org>
> 
> ACK.  This will set some reasonable expectations while things are in flux.
> 
> Arnd are you willing to take this in?

I think it's rather pointless, because the option is not going to
be user selectable but will get selected by the platform unless I'm
mistaken. The platform maintainers that care already know the state
of the framework. Also, there are no user space interfaces that we
have to warn users about not being stable, because the framework
is internal to the kernel.

However, I wonder whether we need the patch below to prevent
users from accidentally enabling COMMON_CLK on platforms that
already provide their own implementation.

	Arnd

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 2eaf17e..a0a83de 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -12,7 +12,7 @@ config HAVE_MACH_CLKDEV

 menuconfig COMMON_CLK
-       bool "Common Clock Framework"
+	bool
        select HAVE_CLK_PREPARE
        ---help---
          The common clock framework is a single definition of struct
          clk, useful across many platforms, as well as an

Re: [PATCH v7 1/3] Documentation: common clk API

[Turquette, Mike]

On Sat, Mar 17, 2012 at 2:05 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 16 March 2012, Turquette, Mike wrote:
>> On Fri, Mar 16, 2012 at 3:21 PM, Paul Walmsley <paul@pwsan.com> wrote:
>> > From: Paul Walmsley <paul@pwsan.com>
>> > Date: Fri, 16 Mar 2012 16:06:30 -0600
>> > Subject: [PATCH] clk: mark the common clk code as EXPERIMENTAL for now
>> >
>> > Mark the common clk code as depending on CONFIG_EXPERIMENTAL.  The API
>> > is not well-defined and both it and the underlying mechanics are likely
>> > to need significant changes to support non-trivial uses of the rate
>> > changing code, such as DVFS with external I/O devices.  So any platforms
>> > that switch their implementation over to this may need to revise much
>> > of their driver code and revalidate their implementations until the
>> > behavior of the code is better-defined.
>> >
>> > A good time for removing this EXPERIMENTAL designation would be after at
>> > least two platforms that do DVFS on groups of external I/O devices have
>> > ported their clock implementations over to the common clk code.
>> >
>> > Signed-off-by: Paul Walmsley <paul@pwsan.com>
>> > Cc: Mike Turquette <mturquette@ti.com>
>>
>> ACK.  This will set some reasonable expectations while things are in flux.
>>
>> Arnd are you willing to take this in?
>
> I think it's rather pointless, because the option is not going to
> be user selectable but will get selected by the platform unless I'm
> mistaken. The platform maintainers that care already know the state
> of the framework. Also, there are no user space interfaces that we
> have to warn users about not being stable, because the framework
> is internal to the kernel.

The consensus seems to be to not set experimental.

> However, I wonder whether we need the patch below to prevent
> users from accidentally enabling COMMON_CLK on platforms that
> already provide their own implementation.
>
>        Arnd
>
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 2eaf17e..a0a83de 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -12,7 +12,7 @@ config HAVE_MACH_CLKDEV
>
>  menuconfig COMMON_CLK
> -       bool "Common Clock Framework"
> +       bool
>        select HAVE_CLK_PREPARE
>        ---help---
>          The common clock framework is a single definition of struct
>          clk, useful across many platforms, as well as an

Much like experimental I'm not sure how needed this change is.  The
help section does say to leave it disabled by default, if unsure.  If
you merge it I won't object but this might be fixing an imaginary
problem.

Regards,
Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/3] Documentation: common clk API

[Arnd Bergmann]

On Saturday 17 March 2012, Turquette, Mike wrote:
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > index 2eaf17e..a0a83de 100644
> > --- a/drivers/clk/Kconfig
> > +++ b/drivers/clk/Kconfig
> > @@ -12,7 +12,7 @@ config HAVE_MACH_CLKDEV
> >
> >  menuconfig COMMON_CLK
> > -       bool "Common Clock Framework"
> > +       bool
> >        select HAVE_CLK_PREPARE
> >        ---help---
> >          The common clock framework is a single definition of struct
> >          clk, useful across many platforms, as well as an
> 
> Much like experimental I'm not sure how needed this change is.  The
> help section does say to leave it disabled by default, if unsure.  If
> you merge it I won't object but this might be fixing an imaginary
> problem.

Well, it doesn't have any consequences for real users, but it I think it
does for randconfig builds, which we are trying to repair currently.

	Arnd

Re: [PATCH v7 1/3] Documentation: common clk API

[Sascha Hauer]

On Sat, Mar 17, 2012 at 11:02:11AM -0700, Turquette, Mike wrote:
> On Sat, Mar 17, 2012 at 2:05 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> > On Friday 16 March 2012, Turquette, Mike wrote:
> >> On Fri, Mar 16, 2012 at 3:21 PM, Paul Walmsley <paul-DWxLp4Yu+b8AvxtiuMwx3w@public.gmane.org> wrote:
> >> > From: Paul Walmsley <paul-DWxLp4Yu+b8AvxtiuMwx3w@public.gmane.org>
> >> > Date: Fri, 16 Mar 2012 16:06:30 -0600
> >> > Subject: [PATCH] clk: mark the common clk code as EXPERIMENTAL for now
> >> >
> >> > Mark the common clk code as depending on CONFIG_EXPERIMENTAL.  The API
> >> > is not well-defined and both it and the underlying mechanics are likely
> >> > to need significant changes to support non-trivial uses of the rate
> >> > changing code, such as DVFS with external I/O devices.  So any platforms
> >> > that switch their implementation over to this may need to revise much
> >> > of their driver code and revalidate their implementations until the
> >> > behavior of the code is better-defined.
> >> >
> >> > A good time for removing this EXPERIMENTAL designation would be after at
> >> > least two platforms that do DVFS on groups of external I/O devices have
> >> > ported their clock implementations over to the common clk code.
> >> >
> >> > Signed-off-by: Paul Walmsley <paul-DWxLp4Yu+b8AvxtiuMwx3w@public.gmane.org>
> >> > Cc: Mike Turquette <mturquette-l0cyMroinI0@public.gmane.org>
> >>
> >> ACK.  This will set some reasonable expectations while things are in flux.
> >>
> >> Arnd are you willing to take this in?
> >
> > I think it's rather pointless, because the option is not going to
> > be user selectable but will get selected by the platform unless I'm
> > mistaken. The platform maintainers that care already know the state
> > of the framework. Also, there are no user space interfaces that we
> > have to warn users about not being stable, because the framework
> > is internal to the kernel.
> 
> The consensus seems to be to not set experimental.
> 
> > However, I wonder whether we need the patch below to prevent
> > users from accidentally enabling COMMON_CLK on platforms that
> > already provide their own implementation.
> >
> >        Arnd
> >
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > index 2eaf17e..a0a83de 100644
> > --- a/drivers/clk/Kconfig
> > +++ b/drivers/clk/Kconfig
> > @@ -12,7 +12,7 @@ config HAVE_MACH_CLKDEV
> >
> >  menuconfig COMMON_CLK
> > -       bool "Common Clock Framework"
> > +       bool
> >        select HAVE_CLK_PREPARE
> >        ---help---
> >          The common clock framework is a single definition of struct
> >          clk, useful across many platforms, as well as an
> 
> Much like experimental I'm not sure how needed this change is.  The
> help section does say to leave it disabled by default, if unsure.  If
> you merge it I won't object but this might be fixing an imaginary
> problem.

Architectures without common clock support won't build with this option
enabled (multiple definition of clk_enable etc), so I think this should
not be user visible.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v7 1/3] Documentation: common clk API

[Arnd Bergmann]

On Saturday 17 March 2012, Sascha Hauer wrote:
> On Sat, Mar 17, 2012 at 11:02:11AM -0700, Turquette, Mike wrote:
> >
> > Much like experimental I'm not sure how needed this change is.  The
> > help section does say to leave it disabled by default, if unsure.  If
> > you merge it I won't object but this might be fixing an imaginary
> > problem.
> 
> Architectures without common clock support won't build with this option
> enabled (multiple definition of clk_enable etc), so I think this should
> not be user visible.

I've applied this patch now.

	Arnd

commit c173033d154e9792b1b5059783b802f82536d48f
Author: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Date:   Sat Mar 17 21:10:51 2012 +0000

    clk: make CONFIG_COMMON_CLK invisible
    
    All platforms that use the common clk infrastructure should select
    COMMON_CLK from platform code, and on all other platforms, it must
    not be enabled, so there is no point making the option visible to
    users, and when it is visible, we break randconfig builds.
    
    Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 2eaf17e..82bcfbd 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -10,18 +10,14 @@ config HAVE_MACH_CLKDEV
 	bool
 
 menuconfig COMMON_CLK
-	bool "Common Clock Framework"
+	bool
 	select HAVE_CLK_PREPARE
 	---help---
 	  The common clock framework is a single definition of struct
 	  clk, useful across many platforms, as well as an
 	  implementation of the clock API in include/linux/clk.h.
 	  Architectures utilizing the common struct clk should select
-	  this automatically, but it may be necessary to manually select
-	  this option for loadable modules requiring the common clock
-	  framework.
-
-	  If in doubt, say "N".
+	  this option.
 
 if COMMON_CLK

Re: [PATCH v7 1/3] Documentation: common clk API

[Paul Walmsley]

Hello Arnd,

On Sat, 17 Mar 2012, Arnd Bergmann wrote:

> I think it's rather pointless, because the option is not going to
> be user selectable but will get selected by the platform unless I'm
> mistaken. The platform maintainers that care already know the state
> of the framework.

This is where we have differing views, I think.  Clearly, Sascha, 
Saravana, Rob, and I have at least slightly different opinions on the 
durability of the existing API and code.  So it seems reasonable to assume 
that others who have not followed the development of the common clock code 
might mistake the implementation or API as being stable and well-defined.

It sounds like the primary objection is to the use of CONFIG_EXPERIMENTAL.  
So here is a patch to simply note the status of this code in its Kconfig 
text.


- Paul

From: Paul Walmsley <paul@pwsan.com>
Date: Tue, 20 Mar 2012 17:19:06 -0600
Subject: [PATCH] clk: note that the common clk code is still subject to
 significant change

Indicate that the common clk code and API is still likely to require
significant change.  The API is not well-defined and both it and the
underlying mechanics are likely to need significant changes to support
non-trivial uses of the rate changing code, such as DVFS with external
I/O devices.  So any platforms that switch their implementation over
to this may need to revise much of their driver code and revalidate
their implementations until the behavior of the code is
better-defined.

A good time for removing this help text would be after at least two
platforms that do DVFS on groups of external I/O devices have ported
their clock implementations over to the common clk code.

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Mike Turquette <mturquette@ti.com>
---
 drivers/clk/Kconfig |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 2eaf17e..dd2d528 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -21,6 +21,11 @@ menuconfig COMMON_CLK
 	  this option for loadable modules requiring the common clock
 	  framework.
 
+	  The API and implementation enabled by this option is still
+	  incomplete.  The API and implementation is expected to be
+	  fluid and subject to change at any time, potentially
+	  breaking existing users.
+
 	  If in doubt, say "N".
 
 if COMMON_CLK
-- 
1.7.9.1

Re: [PATCH v7 1/3] Documentation: common clk API

[Arnd Bergmann]

On Tuesday 20 March 2012, Paul Walmsley wrote:
> Hello Arnd,
> 
> On Sat, 17 Mar 2012, Arnd Bergmann wrote:
> 
> > I think it's rather pointless, because the option is not going to
> > be user selectable but will get selected by the platform unless I'm
> > mistaken. The platform maintainers that care already know the state
> > of the framework.
> 
> This is where we have differing views, I think.  Clearly, Sascha, 
> Saravana, Rob, and I have at least slightly different opinions on the 
> durability of the existing API and code.  So it seems reasonable to assume 
> that others who have not followed the development of the common clock code 
> might mistake the implementation or API as being stable and well-defined.
> 
> It sounds like the primary objection is to the use of CONFIG_EXPERIMENTAL.  
> So here is a patch to simply note the status of this code in its Kconfig 
> text.

Yes, looks good to me. If there are no objections, I'll apply this one.

Thanks,

	Arnd

Re: [PATCH v7 1/3] Documentation: common clk API

[Sascha Hauer]

Hi Paul,

On Fri, Mar 16, 2012 at 04:21:17PM -0600, Paul Walmsley wrote:
> Hi
> 
> On Fri, 16 Mar 2012, Arnd Bergmann wrote:
> 
> 
> If the common clock code is to go upstream now, it should be marked as 
> experimental.

No, please don't do this. This effectively marks the architectures using
the generic clock framework experimental. We can mark drivers as 'you
have been warned', but marking an architecture as experimental is the
wrong sign for both its users and people willing to adopt the framework.
Also we get this:

warning: (ARCH_MX1 && MACH_MX21 && ARCH_MX25 && MACH_MX27) selects COMMON_CLK which has unmet direct dependencies (EXPERIMENTAL)

(and no, I don't want to support to clock frameworks in parallel)

> This is because we know the API is not well-defined, and 
> that both the API and the underlying mechanics will almost certainly need 
> to change for non-trivial uses of the rate changing code (e.g., DVFS with 
> external I/O devices).

Please leave DVFS out of the game. DVFS will use the clock framework for
the F part and the regulator framework for the V part, but the clock
framework should not get extended with DVFS features. The notifiers we
currently have in the clock framework should give enough information
for DVFS implementations. Even if they don't and we have to change
something here this will have no influence on the architectures
implementing their clock tree with the common clock framework.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v7 1/3] Documentation: common clk API

[Rob Herring]

On 03/16/2012 06:47 PM, Sascha Hauer wrote:
> Hi Paul,
> 
> On Fri, Mar 16, 2012 at 04:21:17PM -0600, Paul Walmsley wrote:
>> Hi
>>
>> On Fri, 16 Mar 2012, Arnd Bergmann wrote:
>>
>>
>> If the common clock code is to go upstream now, it should be marked as 
>> experimental.
> 
> No, please don't do this. This effectively marks the architectures using
> the generic clock framework experimental. We can mark drivers as 'you
> have been warned', but marking an architecture as experimental is the
> wrong sign for both its users and people willing to adopt the framework.
> Also we get this:
> 
> warning: (ARCH_MX1 && MACH_MX21 && ARCH_MX25 && MACH_MX27) selects COMMON_CLK which has unmet direct dependencies (EXPERIMENTAL)
> 
> (and no, I don't want to support to clock frameworks in parallel)
> 

+1

For simple users at least, the api is perfectly adequate and it is not
experimental (unless new means experimental).

Rob

>> This is because we know the API is not well-defined, and 
>> that both the API and the underlying mechanics will almost certainly need 
>> to change for non-trivial uses of the rate changing code (e.g., DVFS with 
>> external I/O devices).
> 
> Please leave DVFS out of the game. DVFS will use the clock framework for
> the F part and the regulator framework for the V part, but the clock
> framework should not get extended with DVFS features. The notifiers we
> currently have in the clock framework should give enough information
> for DVFS implementations. Even if they don't and we have to change
> something here this will have no influence on the architectures
> implementing their clock tree with the common clock framework.
> 
> Sascha
> 

Re: [PATCH v7 1/3] Documentation: common clk API

[Saravana Kannan]

On 03/16/2012 05:54 PM, Rob Herring wrote:
> On 03/16/2012 06:47 PM, Sascha Hauer wrote:
>> Hi Paul,
>>
>> On Fri, Mar 16, 2012 at 04:21:17PM -0600, Paul Walmsley wrote:
>>> Hi
>>>
>>> On Fri, 16 Mar 2012, Arnd Bergmann wrote:
>>>
>>>
>>> If the common clock code is to go upstream now, it should be marked as
>>> experimental.
>>
>> No, please don't do this. This effectively marks the architectures using
>> the generic clock framework experimental. We can mark drivers as 'you
>> have been warned', but marking an architecture as experimental is the
>> wrong sign for both its users and people willing to adopt the framework.
>> Also we get this:
>>
>> warning: (ARCH_MX1&&  MACH_MX21&&  ARCH_MX25&&  MACH_MX27) selects COMMON_CLK which has unmet direct dependencies (EXPERIMENTAL)
>>
>> (and no, I don't want to support to clock frameworks in parallel)
>>
>
> +1
>
> For simple users at least, the api is perfectly adequate and it is not
> experimental (unless new means experimental).
>

+1 - not experimental please.

While I agree with Mike and Paul that there is room for a lot of 
improvements to the clock APIs, I think the existing APIs in this patch 
series can become macro wrappers around any new APIs improvements we add 
in the future.

We should have really done that for the 
clk_prepare_enable/unprepare_disable(), but it should certainly be 
doable for future API additions now that we have the atomic/non-atomic 
differentiation out of the way.

Thanks,
Saravana


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH v7 1/3] Documentation: common clk API

[Paul Walmsley]

Hello Sascha,

On Sat, 17 Mar 2012, Sascha Hauer wrote:

> On Fri, Mar 16, 2012 at 04:21:17PM -0600, Paul Walmsley wrote:
>  
> > If the common clock code is to go upstream now, it should be marked as 
> > experimental.
> 
> No, please don't do this. This effectively marks the architectures using
> the generic clock framework experimental. We can mark drivers as 'you
> have been warned', but marking an architecture as experimental is the
> wrong sign for both its users and people willing to adopt the framework.
> Also we get this:
> 
> warning: (ARCH_MX1 && MACH_MX21 && ARCH_MX25 && MACH_MX27) selects COMMON_CLK which has unmet direct dependencies (EXPERIMENTAL)
> 
> (and no, I don't want to support to clock frameworks in parallel)

It sounds as if your objection is with CONFIG_EXPERIMENTAL.  If that is 
indeed your objection, I personally have no objection to simply marking 
the code experimental in the Kconfig text.  (Patch at the bottom of this 
message.)

We need to indicate in some way that the existing code and API is very 
likely to change in ways that could involve quite a bit of work for 
adopters.

> > This is because we know the API is not well-defined, and 
> > that both the API and the underlying mechanics will almost certainly need 
> > to change for non-trivial uses of the rate changing code (e.g., DVFS with 
> > external I/O devices).
> 
> Please leave DVFS out of the game. DVFS will use the clock framework for
> the F part and the regulator framework for the V part, but the clock
> framework should not get extended with DVFS features. The notifiers we
> currently have in the clock framework should give enough information
> for DVFS implementations.

Sadly, that's not so.

Consider a CPUFreq driver as one common clock framework user.  This driver 
will attempt to repeatedly change the rate of a clock.  Changing that 
clock's rate may also involve changing the rate of several other clocks 
used by active devices.  So drivers for these devices will need to 
register rate change notifiers.  The notifier callbacks might involve 
heavyweight work, such as asserting flow control to an 
externally-connected device.

Suppose now that the last registered device in the notifier chain cannot 
handle the frequency transition and must abort it.  This in turn will 
require notifier callbacks to all of the previously-notified devices to 
abort the change.  And then shortly after that, CPUFreq is likely to 
request the same frequency change again: hammering a notifier chain that 
can never succeed.

Bad enough.  We know at least one way to solve this problem.  We can use 
something like the clk_{block,allow}_changes() functions that have been 
discussed for some time now.  But these quickly reveal another problem in 
the existing API.  By default, when there are multiple enabled users of a 
clock, another entity is allowed to change the clock's rate, or the rate 
of any parent of that clock (!).

This has several implications.  One that is significant is that for any 
non-trivial clock tree, any driver that cares about its clock's rate will 
need to implement notifier callbacks.  This is because the driver has no 
way of knowing if or when any other code on the system will attempt to 
change that clock's rate or source.  Or any parent clock's rate or source 
might change.  Should we really force all current drivers using the clock 
code to implement these callbacks?  Or can we restrict the situations in 
which the clock's rate or parent can change by placing restrictions on the 
API?  But then, driver code may need to be rewritten, and behavior 
assumptions may change.

> Even if they don't and we have to change something here this will have 
> no influence on the architectures implementing their clock tree with the 
> common clock framework.

That sounds pretty confident.  Are you sure that the semantics are so well 
understood?

For example, should we allow clk_set_rate() on disabled clocks?  How about 
prepared, but disabled clocks?  If so, what exactly should the clock 
framework do in these circumstances?  Should notifier callbacks go out 
immediately to registered callbacks?  Or should those callbacks be delayed 
until the clock is prepared or enabled?  How should that work when 
clk_enable() cannot block?  And are you confident that any other user of 
the clock framework will answer these undefined questions in the same way 
you would?

The above questions are simply "scratching the surface."  (Just as 
examples, there are still significant questions about locking, reentrancy, 
and so on - [1] is just one example)

These questions have reasonable answers that I think can be mostly aligned 
on.  Thinking through the use-cases, and implications, and answering them, 
should have been the first task in working on the common clock code.  I am 
sorry to say -- and perhaps this is partially my fault -- that it seems as 
if most people are not even aware that these questions exist, despite 
discussions at several conferences and on the mailing lists.

Anyway.  It is okay if we want to have some starter common clock framework 
in mainline; this is why deferring the merge hasn't been proposed.  But 
the point is that anyone who bases their code or platform on the common 
clock framework needs to be aware that, to satisfy one of its major 
use-cases, the behavior and/or API of the common clock code may need to 
change significantly.

Explicitly stating this is not only simple courtesy to its users, many of 
whom won't have been privy to its development.  It also is intended to 
make the code easier to change once it reaches mainline.  Once several 
platforms start using it, there will naturally be resistance and 
conservatism in changing its semantics and interface.  Many drivers may 
have to be changed, across many different maintainers.  And power 
management code may well need to be revalidated on the platforms that use 
it.  PM code, in my opinion, is generally the most difficult code to debug 
in the kernel.

So, until the API is well-defined and does all that it needs to do for its 
major users, we should at least have something like the following patch 
applied.


- Paul

1. King, Russell.  _Re: [PATCH v3 3/5] clk: introduce the common clock 
framework_.  2 Dec 2011 20:23:06 +0000.  linux-omap mailing list.  
http://www.spinics.net/lists/linux-omap/msg61024.html


From: Paul Walmsley <paul@pwsan.com>
Date: Tue, 20 Mar 2012 17:19:06 -0600
Subject: [PATCH] clk: note that the common clk code is still subject to
 significant change

Indicate that the common clk code and API is still likely to require
significant change.  The API is not well-defined and both it and the
underlying mechanics are likely to need significant changes to support
non-trivial uses of the rate changing code, such as DVFS with external
I/O devices.  So any platforms that switch their implementation over
to this may need to revise much of their driver code and revalidate
their implementations until the behavior of the code is
better-defined.

A good time for removing this help text would be after at least two
platforms that do DVFS on groups of external I/O devices have ported
their clock implementations over to the common clk code.

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Mike Turquette <mturquette@ti.com>
---
 drivers/clk/Kconfig |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 2eaf17e..dd2d528 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -21,6 +21,11 @@ menuconfig COMMON_CLK
 	  this option for loadable modules requiring the common clock
 	  framework.
 
+	  The API and implementation enabled by this option is still
+	  incomplete.  The API and implementation is expected to be
+	  fluid and subject to change at any time, potentially
+	  breaking existing users.
+
 	  If in doubt, say "N".
 
 if COMMON_CLK
-- 
1.7.9.1

Re: [PATCH v7 1/3] Documentation: common clk API

[Nicolas Pitre]

On Tue, 20 Mar 2012, Paul Walmsley wrote:

> We need to indicate in some way that the existing code and API is very 
> likely to change in ways that could involve quite a bit of work for 
> adopters.

[...]

> Anyway.  It is okay if we want to have some starter common clock framework 
> in mainline; this is why deferring the merge hasn't been proposed.  But 
> the point is that anyone who bases their code or platform on the common 
> clock framework needs to be aware that, to satisfy one of its major 
> use-cases, the behavior and/or API of the common clock code may need to 
> change significantly.

Paul,

While I understand your concern, please don't forget that the perfect is 
the enemy of the good.

This common clk API has been under development for over *two* years 
already, with several attempts to merge it.  And each previous merge 
attempt aborted because someone came along at the last minute to do 
exactly what you are doing i.e. underline all the flaws and call for a 
redesign.  This is becoming a bad joke.

We finally have something that the majority of reviewers are happy with 
and which should cover the majority of existing cases out there.  Let's 
give it a chance, shall we? Otherwise one might ask where were you 
during those development years if you claim that the behavior and/or API 
of the common clock code still need to change significantly?

> Explicitly stating this is not only simple courtesy to its users, many of 
> whom won't have been privy to its development.  It also is intended to 
> make the code easier to change once it reaches mainline.

The code will be easier to change once it is in mainline, simply due to 
the fact that you can also change all its users at once.  And it is well 
possible that most users won't have to deal with the same magnitude of 
complexity as yours, again reducing the scope for resistance to changes.

Let's see how things go with the current code and improve it 
incrementally.  Otherwise no one will get involved with this API which 
is almost just as bad as not having the code merged at all.

> So, until the API is well-defined and does all that it needs to do for its 
> major users, [...]

No, the API simply can't ever be well defined if people don't actually 
start using it to eventually refine it further.  Major users were 
converted to it, and in most cases The API does all that it needs to do 
already.  Otherwise you'll be stuck in a catch22 situation forever.

And APIs in the Linux kernel do change all the time.  There is no stable 
API in the kernel.  Extensions will come about eventually, and existing 
users (simple ones by definition if the current API already meets their 
needs) should be converted over easily.


Nicolas

Re: [PATCH v7 1/3] Documentation: common clk API

[Saravana Kannan]

On 03/20/2012 08:15 PM, Nicolas Pitre wrote:
> On Tue, 20 Mar 2012, Paul Walmsley wrote:
>
>> We need to indicate in some way that the existing code and API is very
>> likely to change in ways that could involve quite a bit of work for
>> adopters.
>
> [...]
>
>> Anyway.  It is okay if we want to have some starter common clock framework
>> in mainline; this is why deferring the merge hasn't been proposed.  But
>> the point is that anyone who bases their code or platform on the common
>> clock framework needs to be aware that, to satisfy one of its major
>> use-cases, the behavior and/or API of the common clock code may need to
>> change significantly.
>
> Paul,
>
> While I understand your concern, please don't forget that the perfect is
> the enemy of the good.
>
> This common clk API has been under development for over *two* years
> already, with several attempts to merge it.  And each previous merge
> attempt aborted because someone came along at the last minute to do
> exactly what you are doing i.e. underline all the flaws and call for a
> redesign.  This is becoming a bad joke.
>
> We finally have something that the majority of reviewers are happy with
> and which should cover the majority of existing cases out there.  Let's
> give it a chance, shall we? Otherwise one might ask where were you
> during those development years if you claim that the behavior and/or API
> of the common clock code still need to change significantly?
>
>> Explicitly stating this is not only simple courtesy to its users, many of
>> whom won't have been privy to its development.  It also is intended to
>> make the code easier to change once it reaches mainline.
>
> The code will be easier to change once it is in mainline, simply due to
> the fact that you can also change all its users at once.  And it is well
> possible that most users won't have to deal with the same magnitude of
> complexity as yours, again reducing the scope for resistance to changes.
>
> Let's see how things go with the current code and improve it
> incrementally.  Otherwise no one will get involved with this API which
> is almost just as bad as not having the code merged at all.
>
>> So, until the API is well-defined and does all that it needs to do for its
>> major users, [...]
>
> No, the API simply can't ever be well defined if people don't actually
> start using it to eventually refine it further.  Major users were
> converted to it, and in most cases The API does all that it needs to do
> already.  Otherwise you'll be stuck in a catch22 situation forever.
>
> And APIs in the Linux kernel do change all the time.  There is no stable
> API in the kernel.  Extensions will come about eventually, and existing
> users (simple ones by definition if the current API already meets their
> needs) should be converted over easily.

+1 to the general idea in Nicolas's email.

To add a few more thoughts, while I agree with Paul that there is room 
for improvement in the APIs, I think the difference in opinion comes 
when we ask the question:

"When we eventually refine the APIs in the future to be more expressive, 
do we think that the current APIs can or cannot be made as wrappers 
around the new more expressive APIs?"

Absolutes are almost never right, so I can't give an absolute answer. 
But I'm strongly leaning towards "we can" as the answer to the question. 
That combined with the reasons Nicholas mentioned is why I think the 
APIs should not be marked as experimental in anyway.

-Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH v7 1/3] Documentation: common clk API

[Paul Walmsley]

Hello Saravana,

On Tue, 20 Mar 2012, Saravana Kannan wrote:

> To add a few more thoughts, while I agree with Paul that there is room for
> improvement in the APIs, I think the difference in opinion comes when we ask
> the question:
> 
> "When we eventually refine the APIs in the future to be more expressive, do we
> think that the current APIs can or cannot be made as wrappers around the new
> more expressive APIs?"
> 
> Absolutes are almost never right, so I can't give an absolute answer. 
> But I'm strongly leaning towards "we can" as the answer to the question.  
> That combined with the reasons Nicholas mentioned is why I think the 
> APIs should not be marked as experimental in anyway.

The resistance to documenting that the clock interface is not 
well-defined, and that therefore it is likely to change, and that users 
should not expect it to be stable, is perplexing to me.

Certainly a Kconfig help text change seems trivial enough.  But even the 
resistance to CONFIG_EXPERIMENTAL has been quite surprising to me, given 
that every single defconfig in arch/arm/defconfig sets it:

$ find arch/arm/configs -type f  | wc -l
122
$ fgrep -r CONFIG_EXPERIMENTAL=y arch/arm/configs | wc -l
122
$

(that includes iMX, by the way...)

Certainly, neither Kconfig change is going to prevent us on OMAP from 
figuring out what else is needed to convert our platform to the common 
clock code.  And given the level of enthusiasm on the lists, I don't think 
it's going to prevent many of the other ARM platforms from experimenting 
with the conversion, either.

So it would be interesting to know more about why you (or anyone else) 
perceive that the Kconfig changes would be harmful.


- Paul

Re: [PATCH v7 1/3] Documentation: common clk API

[Sascha Hauer]

On Wed, Mar 21, 2012 at 01:44:01AM -0600, Paul Walmsley wrote:
> Hello Saravana,
> 
> Certainly a Kconfig help text change seems trivial enough.  But even the 
> resistance to CONFIG_EXPERIMENTAL has been quite surprising to me, given 
> that every single defconfig in arch/arm/defconfig sets it:
> 
> $ find arch/arm/configs -type f  | wc -l
> 122
> $ fgrep -r CONFIG_EXPERIMENTAL=y arch/arm/configs | wc -l
> 122
> $
> 
> (that includes iMX, by the way...)
> 
> Certainly, neither Kconfig change is going to prevent us on OMAP from 
> figuring out what else is needed to convert our platform to the common 
> clock code.  And given the level of enthusiasm on the lists, I don't think 
> it's going to prevent many of the other ARM platforms from experimenting 
> with the conversion, either.
> 
> So it would be interesting to know more about why you (or anyone else) 
> perceive that the Kconfig changes would be harmful.

Mainly because COMMON_CLK is an invisible option which has to be
selected by architectures. So with the Kconfig change we either have to:

config ARCH_MXC
	depends on EXPERIMENTAL

or:

config ARCH_MXC
	select EXPERIMENTAL
	select COMMON_CLK

Neither of both seems very appealing to me.

You can add a warning to the Kconfig help text if you like, I
have no problem with that. As you said it will prevent noone
from using it anyway.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v7 1/3] Documentation: common clk API

[Saravana Kannan]

On 03/21/2012 12:44 AM, Paul Walmsley wrote:
> Hello Saravana,
>
> On Tue, 20 Mar 2012, Saravana Kannan wrote:
>
>> To add a few more thoughts, while I agree with Paul that there is room for
>> improvement in the APIs, I think the difference in opinion comes when we ask
>> the question:
>>
>> "When we eventually refine the APIs in the future to be more expressive, do we
>> think that the current APIs can or cannot be made as wrappers around the new
>> more expressive APIs?"
>>
>> Absolutes are almost never right, so I can't give an absolute answer.
>> But I'm strongly leaning towards "we can" as the answer to the question.
>> That combined with the reasons Nicholas mentioned is why I think the
>> APIs should not be marked as experimental in anyway.
>
> The resistance to documenting that the clock interface is not
> well-defined, and that therefore it is likely to change, and that users
> should not expect it to be stable, is perplexing to me.
>
> Certainly a Kconfig help text change seems trivial enough.  But even the
> resistance to CONFIG_EXPERIMENTAL has been quite surprising to me, given
> that every single defconfig in arch/arm/defconfig sets it:
>
> $ find arch/arm/configs -type f  | wc -l
> 122
> $ fgrep -r CONFIG_EXPERIMENTAL=y arch/arm/configs | wc -l
> 122
> $
>
> (that includes iMX, by the way...)
>
> Certainly, neither Kconfig change is going to prevent us on OMAP from
> figuring out what else is needed to convert our platform to the common
> clock code.  And given the level of enthusiasm on the lists,

I think the enthusiasm we are seeing are from the clock driver 
developers. And for good reasons. Everyone is tired of having to write 
or rewrite their own implementation.

> I don't think
> it's going to prevent many of the other ARM platforms from experimenting
> with the conversion, either.
>
> So it would be interesting to know more about why you (or anyone else)
> perceive that the Kconfig changes would be harmful.

But the enthusiasm of the clock driver developers doesn't necessarily 
translate to users of the clock APIs (other driver devs). My worry with 
marking it as experimental in Kconfig and to a certain extent in the 
documentation is that it will discourage the driver devs from switching 
to the new APIs. If no one is using the new APIs, then platforms can't 
switch to the common clock framework either. If a lot of platform don't 
convert to the common clock framework, the effort to get it merged in 
now is not also very meaningful.

Also, at the rate at which we come to an agreement on new APIs, I think 
these new APIs should be considered quite stable :-) It's certainly 
better than what we have today. We should encourage driver devs to move 
to these new APIs and get the benefits while we go on another 2 yr cycle 
to agree on the next set of APIs improvements.

And as I said before, I think it's much less likely that we will break 
backward source compatibility when we do the next improvement. We could 
have mostly avoid this large scale change by calling the APIs 
prepare/unprepare/gate/ungate (or whatever) and make clk_enable/disable 
similar to clk_prepare_enable/clk_disable_unprepare(). That would have 
avoid a lot of drivers from having to mess with their clock calls. But 
we didn't think about that in the excitement for improved APIs. I think 
this will still be seared in our minds enough to make sure we don't 
repeat that in the future.

That covers all I have to say on this topic.

-Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH v7 1/3] Documentation: common clk API

[Mark Brown]

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

On Wed, Mar 21, 2012 at 11:38:58AM -0700, Saravana Kannan wrote:

> >So it would be interesting to know more about why you (or anyone else)
> >perceive that the Kconfig changes would be harmful.

> But the enthusiasm of the clock driver developers doesn't
> necessarily translate to users of the clock APIs (other driver
> devs). My worry with marking it as experimental in Kconfig and to a
> certain extent in the documentation is that it will discourage the
> driver devs from switching to the new APIs. If no one is using the
> new APIs, then platforms can't switch to the common clock framework

These aren't new APIs, the clock API has been around since forever.
For driver authors working on anything that isn't platform specific the
issue has been that it's not implemented at all on the overwhelming
majority of architectures and those that do all have their own random
implementations with their own random quirks and with no ability for
anything except the platform to even try to do incredibly basic stuff
like register a new clock.

Simply having something, anything, in place even if it's going to churn
is a massive step forward here for people working with clocks.

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

Re: [PATCH v7 1/3] Documentation: common clk API

[Tony Lindgren]

* Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> [120321 12:11]:
> On Wed, Mar 21, 2012 at 11:38:58AM -0700, Saravana Kannan wrote:
> 
> > >So it would be interesting to know more about why you (or anyone else)
> > >perceive that the Kconfig changes would be harmful.
> 
> > But the enthusiasm of the clock driver developers doesn't
> > necessarily translate to users of the clock APIs (other driver
> > devs). My worry with marking it as experimental in Kconfig and to a
> > certain extent in the documentation is that it will discourage the
> > driver devs from switching to the new APIs. If no one is using the
> > new APIs, then platforms can't switch to the common clock framework
> 
> These aren't new APIs, the clock API has been around since forever.
> For driver authors working on anything that isn't platform specific the
> issue has been that it's not implemented at all on the overwhelming
> majority of architectures and those that do all have their own random
> implementations with their own random quirks and with no ability for
> anything except the platform to even try to do incredibly basic stuff
> like register a new clock.
> 
> Simply having something, anything, in place even if it's going to churn
> is a massive step forward here for people working with clocks.

Right, and now at least the people reading this thread are pretty
aware of the yet unsolved issues with clock fwk api :)

Regards,

Tony

Re: [PATCH v7 1/3] Documentation: common clk API

[Saravana Kannan]

On 03/21/2012 12:33 PM, Tony Lindgren wrote:
> * Mark Brown<broonie@opensource.wolfsonmicro.com>  [120321 12:11]:
>> On Wed, Mar 21, 2012 at 11:38:58AM -0700, Saravana Kannan wrote:
>>
>>>> So it would be interesting to know more about why you (or anyone else)
>>>> perceive that the Kconfig changes would be harmful.
>>
>>> But the enthusiasm of the clock driver developers doesn't
>>> necessarily translate to users of the clock APIs (other driver
>>> devs). My worry with marking it as experimental in Kconfig and to a
>>> certain extent in the documentation is that it will discourage the
>>> driver devs from switching to the new APIs. If no one is using the
>>> new APIs, then platforms can't switch to the common clock framework
>>
>> These aren't new APIs, the clock API has been around since forever.

I disagree. When I say clock API, I mean the actual functions and their 
semantics. Not the existence of a header file.

The meaning of clk_enable/disable has been changed and they won't work 
without calling clk_prepare/unprepare. So, these are definitely new 
APIs. If it weren't new APIs, then none of the general drivers would 
need to change.

>> For driver authors working on anything that isn't platform specific the
>> issue has been that it's not implemented at all on the overwhelming
>> majority of architectures and those that do all have their own random
>> implementations with their own random quirks and with no ability for
>> anything except the platform to even try to do incredibly basic stuff
>> like register a new clock.
>>
>> Simply having something, anything, in place even if it's going to churn
>> is a massive step forward here for people working with clocks.
>
> Right, and now at least the people reading this thread are pretty
> aware of the yet unsolved issues with clock fwk api :)

:-) Shhh... not so loud!

-Saravana


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH v7 1/3] Documentation: common clk API

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 798 bytes --]

On Wed, Mar 21, 2012 at 12:41:41PM -0700, Saravana Kannan wrote:
> On 03/21/2012 12:33 PM, Tony Lindgren wrote:
> >* Mark Brown<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>  [120321 12:11]:

> >>These aren't new APIs, the clock API has been around since forever.

> I disagree. When I say clock API, I mean the actual functions and
> their semantics. Not the existence of a header file.

> The meaning of clk_enable/disable has been changed and they won't
> work without calling clk_prepare/unprepare. So, these are definitely
> new APIs. If it weren't new APIs, then none of the general drivers
> would need to change.

clk_prepare() and clk_unprepare() are already there for any clk.h user
and are stubbed in no matter what, they're really not a meaningful
barrier here.

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

[-- Attachment #2: Type: text/plain, Size: 175 bytes --]

_______________________________________________
linaro-dev mailing list
linaro-dev-cunTk1MwBs8s++Sfvej+rw@public.gmane.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [PATCH v7 1/3] Documentation: common clk API

[Saravana Kannan]

On 03/21/2012 12:56 PM, Mark Brown wrote:
> On Wed, Mar 21, 2012 at 12:41:41PM -0700, Saravana Kannan wrote:
>> On 03/21/2012 12:33 PM, Tony Lindgren wrote:
>>> * Mark Brown<broonie@opensource.wolfsonmicro.com>   [120321 12:11]:
>
>>>> These aren't new APIs, the clock API has been around since forever.
>
>> I disagree. When I say clock API, I mean the actual functions and
>> their semantics. Not the existence of a header file.
>
>> The meaning of clk_enable/disable has been changed and they won't
>> work without calling clk_prepare/unprepare. So, these are definitely
>> new APIs. If it weren't new APIs, then none of the general drivers
>> would need to change.
>
> clk_prepare() and clk_unprepare() are already there for any clk.h user
> and are stubbed in no matter what, they're really not a meaningful
> barrier here.

Isn't this whole experimental vs non-experimental discussion about the 
actual external facing clock APIs and not the platform driver facing APIs?

Sure, prepare/unprepare are already there in the .h file. But they are 
stubs and have no impact till we move to the common clock framework or 
platforms move to them with their own implementation (certainly not 
happening in upstream, so let's leave that part out of this discussion).

So. IMO, for all practical purposes, common clk fwk forces the move to 
these new APIs and hence IMO forces the new APIs.

-Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH v7 1/3] Documentation: common clk API

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 762 bytes --]

On Wed, Mar 21, 2012 at 01:04:22PM -0700, Saravana Kannan wrote:

> Sure, prepare/unprepare are already there in the .h file. But they
> are stubs and have no impact till we move to the common clock
> framework or platforms move to them with their own implementation
> (certainly not happening in upstream, so let's leave that part out
> of this discussion).

> So. IMO, for all practical purposes, common clk fwk forces the move
> to these new APIs and hence IMO forces the new APIs.

Sure, if you want to look at it from that point of view - anything
wanting to run on a platform which uses the generic API needs to use
them, but there's no blocker on the user from this (it can convert with
or without the platform it runs on) - but it's hardly a tough sell.

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

[-- Attachment #2: Type: text/plain, Size: 175 bytes --]

_______________________________________________
linaro-dev mailing list
linaro-dev-cunTk1MwBs8s++Sfvej+rw@public.gmane.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [PATCH v7 1/3] Documentation: common clk API

[Russell King - ARM Linux]

On Wed, Mar 21, 2012 at 12:41:41PM -0700, Saravana Kannan wrote:
> The meaning of clk_enable/disable has been changed and they won't work  
> without calling clk_prepare/unprepare. So, these are definitely new  
> APIs. If it weren't new APIs, then none of the general drivers would  
> need to change.

Yes and no.  I disagree that the meaning of clk_enable/disable() has
changed.  It hasn't.  What has changed is the preconditions for calling
those functions, and necessarily so in the interest of being able to
unify the different implementations.

Re: [PATCH v7 1/3] Documentation: common clk API

[Paul Walmsley]

Hello Nico,

On Tue, 20 Mar 2012, Nicolas Pitre wrote:

> This common clk API has been under development for over *two* years 
> already, with several attempts to merge it.  And each previous merge 
> attempt aborted because someone came along at the last minute to do 
> exactly what you are doing i.e. underline all the flaws and call for a 
> redesign.  This is becoming a bad joke.

There is a misunderstanding.  I am not calling for a redesign.  I am 
simply stating that the current maturity level of the API and code should 
be documented in the Kconfig dependencies or description text before the 
code goes upstream.  The objectives are to make future changes easier, set 
expectations, and clearly disclose the extent to which the API and code 
will need to change.

> The code will be easier to change once it is in mainline, simply due to 
> the fact that you can also change all its users at once.  And it is well 
> possible that most users won't have to deal with the same magnitude of 
> complexity as yours, again reducing the scope for resistance to changes.

I hope you are right.  To me, these conclusions seem unlikely.  It seems 
equally likely that these rationales will make it much more difficult to 
change the code once it's upstream and platforms are depending on it.  
Particularly given the ongoing sensitivity to reducing "churn" of mainline 
code, so publicly discussed.  So it seems like a good idea to attempt to 
address these potential roadblocks and criticisms to major clock framework 
changes early.

> And APIs in the Linux kernel do change all the time.  There is no stable 
> API in the kernel.  Extensions will come about eventually, and existing 
> users (simple ones by definition if the current API already meets their 
> needs) should be converted over easily.

Yes, simple extensions should be straightforward.  Of greater concern are 
changes to the existing interface that will probably be required.  These 
could involve significant changes to driver or platform code.  It seems 
likely that there will be strong inertia to making these changes after 
platforms and drivers are converted.

However, if we clearly state that these API changes are likely until the 
API is well-defined, we will hopefully reduce some angst by disclosing
what some of us know.

...

One last comment to address which is orthogonal to the technical content 
of this discussion.

> Otherwise one might ask where were you during those development years if 
> you claim that the behavior and/or API of the common clock code still 
> need to change significantly?

One might ask this.  If one were to ask this, another might briefly 
outline the participation in public and private clock discussions at 
Linaro Connect in Budapest and Redwood Shores, at LPC in Santa Rosa, at 
ELCE/KS in Prague, at ELC in Redwood Shores, in conference calls, IRC 
sessions, and private E-mails with many of the people included in the 
header of this message, not to mention the list posts.

None of the concerns that have been described are new.  There has been an 
endeavour to discuss them with anyone who seemed even remotely interested.

Of course it is a personal source of regret that the participation could 
not have been greater, but this regret is hardly limited to the common 
clock project.


regards,

- Paul

Re: [PATCH v7 1/3] Documentation: common clk API

[Nicolas Pitre]

On Wed, 21 Mar 2012, Paul Walmsley wrote:

> Hello Nico,
> 
> On Tue, 20 Mar 2012, Nicolas Pitre wrote:
> 
> > This common clk API has been under development for over *two* years 
> > already, with several attempts to merge it.  And each previous merge 
> > attempt aborted because someone came along at the last minute to do 
> > exactly what you are doing i.e. underline all the flaws and call for a 
> > redesign.  This is becoming a bad joke.
> 
> There is a misunderstanding.  I am not calling for a redesign.  I am 
> simply stating that the current maturity level of the API and code should 
> be documented in the Kconfig dependencies or description text before the 
> code goes upstream.  The objectives are to make future changes easier, set 
> expectations, and clearly disclose the extent to which the API and code 
> will need to change.

Maybe there is no actual consensus on that extent.

> > The code will be easier to change once it is in mainline, simply due to 
> > the fact that you can also change all its users at once.  And it is well 
> > possible that most users won't have to deal with the same magnitude of 
> > complexity as yours, again reducing the scope for resistance to changes.
> 
> I hope you are right.  To me, these conclusions seem unlikely.  It seems 
> equally likely that these rationales will make it much more difficult to 
> change the code once it's upstream and platforms are depending on it.  

No code should go upstream if no one depends on it.  Therefore we change 
code that many platforms depend on all the time.  What is killing us is 
the need to synchronize with multiple external code bases scattered 
around.

> Particularly given the ongoing sensitivity to reducing "churn" of mainline 
> code, so publicly discussed.

Please stop buying into that crap.  We congratulate ourselves every 
merge cycles with the current process being able to deal with around 
half a million of lines changed every 3 months or so.  It's pointless 
churn that is frowned upon, not useful and incremental API changes.  I 
don't think that "pointless" would apply here.

> So it seems like a good idea to attempt to address these potential 
> roadblocks and criticisms to major clock framework changes early.

I understand your concern, however I don't think it is as serious as you 
are making it.

> One last comment to address which is orthogonal to the technical content 
> of this discussion.
> 
> > Otherwise one might ask where were you during those development years if 
> > you claim that the behavior and/or API of the common clock code still 
> > need to change significantly?
> 
> One might ask this.  If one were to ask this, another might briefly 
> outline the participation in public and private clock discussions at 
> Linaro Connect in Budapest and Redwood Shores, at LPC in Santa Rosa, at 
> ELCE/KS in Prague, at ELC in Redwood Shores, in conference calls, IRC 
> sessions, and private E-mails with many of the people included in the 
> header of this message, not to mention the list posts.

Sure.  I was there too and came across you many times.  I just don't 
understand why some apparent consensus was built around the current 
submission, that people involved appeared to be highly satisfied at 
last, given the emphasis you are giving to your present concern which 
doesn't seem to be widely shared.  This is a bit surprising.

> None of the concerns that have been described are new.  There has been an 
> endeavour to discuss them with anyone who seemed even remotely interested.

Let's change tactics then.  We've been discussing this code for over two 
years and no one could really benefit from it during that time.  Maybe 
future discussion could become more productive and concrete when some 
code is merged _and_ used at last.


Nicolas