[PATCH 1/2] drivers/clocksource/timer-of: Remove __init markings

[PATCH 1/2] drivers/clocksource/timer-of: Remove __init markings

[Baolin Wang]

From: Saravana Kannan <saravanak@google.com>

This allows timer drivers to be compiled as modules.

Signed-off-by: Saravana Kannan <saravanak@google.com>
Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
---
 drivers/clocksource/timer-of.c | 17 +++++++++--------
 drivers/clocksource/timer-of.h |  4 ++--
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/clocksource/timer-of.c b/drivers/clocksource/timer-of.c
index 572da47..fd3b868 100644
--- a/drivers/clocksource/timer-of.c
+++ b/drivers/clocksource/timer-of.c
@@ -19,7 +19,7 @@
  *
  * Free the irq resource
  */
-static __init void timer_of_irq_exit(struct of_timer_irq *of_irq)
+static void timer_of_irq_exit(struct of_timer_irq *of_irq)
 {
 	struct timer_of *to = container_of(of_irq, struct timer_of, of_irq);
 
@@ -47,7 +47,7 @@ static __init void timer_of_irq_exit(struct of_timer_irq *of_irq)
  *
  * Returns 0 on success, < 0 otherwise
  */
-static __init int timer_of_irq_init(struct device_node *np,
+static int timer_of_irq_init(struct device_node *np,
 				    struct of_timer_irq *of_irq)
 {
 	int ret;
@@ -91,7 +91,7 @@ static __init int timer_of_irq_init(struct device_node *np,
  *
  * Disables and releases the refcount on the clk
  */
-static __init void timer_of_clk_exit(struct of_timer_clk *of_clk)
+static void timer_of_clk_exit(struct of_timer_clk *of_clk)
 {
 	of_clk->rate = 0;
 	clk_disable_unprepare(of_clk->clk);
@@ -107,7 +107,7 @@ static __init void timer_of_clk_exit(struct of_timer_clk *of_clk)
  *
  * Returns 0 on success, < 0 otherwise
  */
-static __init int timer_of_clk_init(struct device_node *np,
+static int timer_of_clk_init(struct device_node *np,
 				    struct of_timer_clk *of_clk)
 {
 	int ret;
@@ -146,12 +146,12 @@ static __init int timer_of_clk_init(struct device_node *np,
 	goto out;
 }
 
-static __init void timer_of_base_exit(struct of_timer_base *of_base)
+static void timer_of_base_exit(struct of_timer_base *of_base)
 {
 	iounmap(of_base->base);
 }
 
-static __init int timer_of_base_init(struct device_node *np,
+static int timer_of_base_init(struct device_node *np,
 				     struct of_timer_base *of_base)
 {
 	of_base->base = of_base->name ?
@@ -165,7 +165,7 @@ static __init int timer_of_base_init(struct device_node *np,
 	return 0;
 }
 
-int __init timer_of_init(struct device_node *np, struct timer_of *to)
+int timer_of_init(struct device_node *np, struct timer_of *to)
 {
 	int ret = -EINVAL;
 	int flags = 0;
@@ -209,6 +209,7 @@ int __init timer_of_init(struct device_node *np, struct timer_of *to)
 		timer_of_base_exit(&to->of_base);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(timer_of_init);
 
 /**
  * timer_of_cleanup - release timer_of ressources
@@ -217,7 +218,7 @@ int __init timer_of_init(struct device_node *np, struct timer_of *to)
  * Release the ressources that has been used in timer_of_init().
  * This function should be called in init error cases
  */
-void __init timer_of_cleanup(struct timer_of *to)
+void timer_of_cleanup(struct timer_of *to)
 {
 	if (to->flags & TIMER_OF_IRQ)
 		timer_of_irq_exit(&to->of_irq);
diff --git a/drivers/clocksource/timer-of.h b/drivers/clocksource/timer-of.h
index a5478f3..1b8cfac5 100644
--- a/drivers/clocksource/timer-of.h
+++ b/drivers/clocksource/timer-of.h
@@ -66,9 +66,9 @@ static inline unsigned long timer_of_period(struct timer_of *to)
 	return to->of_clk.period;
 }
 
-extern int __init timer_of_init(struct device_node *np,
+extern int timer_of_init(struct device_node *np,
 				struct timer_of *to);
 
-extern void __init timer_of_cleanup(struct timer_of *to);
+extern void timer_of_cleanup(struct timer_of *to);
 
 #endif
-- 
1.9.1

[PATCH 2/2] clocksource/drivers/sprd: Add module support to Spreadtrum timer

[Baolin Wang]

Timers still have devices created for them. So, when compiling a timer
driver as a module, implement it as a normal platform device driver.

Signed-off-by: Saravana Kannan <saravanak@google.com>
Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
---
 drivers/clocksource/Kconfig      |  2 +-
 drivers/clocksource/timer-sprd.c | 37 +++++++++++++++++++++++++++++++++++--
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index cc909e4..404f7dc 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -481,7 +481,7 @@ config MTK_TIMER
 	  Support for Mediatek timer driver.
 
 config SPRD_TIMER
-	bool "Spreadtrum timer driver" if EXPERT
+	tristate "Spreadtrum timer driver" if EXPERT
 	depends on HAS_IOMEM
 	depends on (ARCH_SPRD || COMPILE_TEST)
 	default ARCH_SPRD
diff --git a/drivers/clocksource/timer-sprd.c b/drivers/clocksource/timer-sprd.c
index 430cb99..5461789 100644
--- a/drivers/clocksource/timer-sprd.c
+++ b/drivers/clocksource/timer-sprd.c
@@ -5,6 +5,8 @@
 
 #include <linux/init.h>
 #include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
 
 #include "timer-of.h"
 
@@ -141,7 +143,7 @@ static irqreturn_t sprd_timer_interrupt(int irq, void *dev_id)
 	},
 };
 
-static int __init sprd_timer_init(struct device_node *np)
+static int sprd_timer_init(struct device_node *np)
 {
 	int ret;
 
@@ -190,7 +192,7 @@ static void sprd_suspend_timer_disable(struct clocksource *cs)
 	.flags	= CLOCK_SOURCE_IS_CONTINUOUS | CLOCK_SOURCE_SUSPEND_NONSTOP,
 };
 
-static int __init sprd_suspend_timer_init(struct device_node *np)
+static int sprd_suspend_timer_init(struct device_node *np)
 {
 	int ret;
 
@@ -204,6 +206,37 @@ static int __init sprd_suspend_timer_init(struct device_node *np)
 	return 0;
 }
 
+#ifdef MODULE
+static int sprd_timer_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+
+	if (of_property_read_bool(np, "interrupts"))
+		return sprd_timer_init(np);
+
+	return sprd_suspend_timer_init(np);
+}
+
+static const struct of_device_id sprd_timer_match_table[] = {
+	{ .compatible = "sprd,sc9860-suspend-timer" },
+	{ .compatible = "sprd,sc9860-timer" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sprd_timer_match_table);
+
+static struct platform_driver sprd_timer_driver = {
+	.probe		= sprd_timer_probe,
+	.driver		= {
+		.name	= "sprd-timer",
+		.of_match_table = sprd_timer_match_table,
+	},
+};
+module_platform_driver(sprd_timer_driver);
+
+#else
 TIMER_OF_DECLARE(sc9860_timer, "sprd,sc9860-timer", sprd_timer_init);
 TIMER_OF_DECLARE(sc9860_persistent_timer, "sprd,sc9860-suspend-timer",
 		 sprd_suspend_timer_init);
+#endif
+
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH 1/2] drivers/clocksource/timer-of: Remove __init markings

[Baolin Wang]

Hi Daniel,

On Tue, Mar 24, 2020 at 1:59 PM Baolin Wang <baolin.wang7@gmail.com> wrote:
>
> From: Saravana Kannan <saravanak@google.com>
>
> This allows timer drivers to be compiled as modules.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>

Do you have any comments for this patch set? Thanks.

> ---
>  drivers/clocksource/timer-of.c | 17 +++++++++--------
>  drivers/clocksource/timer-of.h |  4 ++--
>  2 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/clocksource/timer-of.c b/drivers/clocksource/timer-of.c
> index 572da47..fd3b868 100644
> --- a/drivers/clocksource/timer-of.c
> +++ b/drivers/clocksource/timer-of.c
> @@ -19,7 +19,7 @@
>   *
>   * Free the irq resource
>   */
> -static __init void timer_of_irq_exit(struct of_timer_irq *of_irq)
> +static void timer_of_irq_exit(struct of_timer_irq *of_irq)
>  {
>         struct timer_of *to = container_of(of_irq, struct timer_of, of_irq);
>
> @@ -47,7 +47,7 @@ static __init void timer_of_irq_exit(struct of_timer_irq *of_irq)
>   *
>   * Returns 0 on success, < 0 otherwise
>   */
> -static __init int timer_of_irq_init(struct device_node *np,
> +static int timer_of_irq_init(struct device_node *np,
>                                     struct of_timer_irq *of_irq)
>  {
>         int ret;
> @@ -91,7 +91,7 @@ static __init int timer_of_irq_init(struct device_node *np,
>   *
>   * Disables and releases the refcount on the clk
>   */
> -static __init void timer_of_clk_exit(struct of_timer_clk *of_clk)
> +static void timer_of_clk_exit(struct of_timer_clk *of_clk)
>  {
>         of_clk->rate = 0;
>         clk_disable_unprepare(of_clk->clk);
> @@ -107,7 +107,7 @@ static __init void timer_of_clk_exit(struct of_timer_clk *of_clk)
>   *
>   * Returns 0 on success, < 0 otherwise
>   */
> -static __init int timer_of_clk_init(struct device_node *np,
> +static int timer_of_clk_init(struct device_node *np,
>                                     struct of_timer_clk *of_clk)
>  {
>         int ret;
> @@ -146,12 +146,12 @@ static __init int timer_of_clk_init(struct device_node *np,
>         goto out;
>  }
>
> -static __init void timer_of_base_exit(struct of_timer_base *of_base)
> +static void timer_of_base_exit(struct of_timer_base *of_base)
>  {
>         iounmap(of_base->base);
>  }
>
> -static __init int timer_of_base_init(struct device_node *np,
> +static int timer_of_base_init(struct device_node *np,
>                                      struct of_timer_base *of_base)
>  {
>         of_base->base = of_base->name ?
> @@ -165,7 +165,7 @@ static __init int timer_of_base_init(struct device_node *np,
>         return 0;
>  }
>
> -int __init timer_of_init(struct device_node *np, struct timer_of *to)
> +int timer_of_init(struct device_node *np, struct timer_of *to)
>  {
>         int ret = -EINVAL;
>         int flags = 0;
> @@ -209,6 +209,7 @@ int __init timer_of_init(struct device_node *np, struct timer_of *to)
>                 timer_of_base_exit(&to->of_base);
>         return ret;
>  }
> +EXPORT_SYMBOL_GPL(timer_of_init);
>
>  /**
>   * timer_of_cleanup - release timer_of ressources
> @@ -217,7 +218,7 @@ int __init timer_of_init(struct device_node *np, struct timer_of *to)
>   * Release the ressources that has been used in timer_of_init().
>   * This function should be called in init error cases
>   */
> -void __init timer_of_cleanup(struct timer_of *to)
> +void timer_of_cleanup(struct timer_of *to)
>  {
>         if (to->flags & TIMER_OF_IRQ)
>                 timer_of_irq_exit(&to->of_irq);
> diff --git a/drivers/clocksource/timer-of.h b/drivers/clocksource/timer-of.h
> index a5478f3..1b8cfac5 100644
> --- a/drivers/clocksource/timer-of.h
> +++ b/drivers/clocksource/timer-of.h
> @@ -66,9 +66,9 @@ static inline unsigned long timer_of_period(struct timer_of *to)
>         return to->of_clk.period;
>  }
>
> -extern int __init timer_of_init(struct device_node *np,
> +extern int timer_of_init(struct device_node *np,
>                                 struct timer_of *to);
>
> -extern void __init timer_of_cleanup(struct timer_of *to);
> +extern void timer_of_cleanup(struct timer_of *to);
>
>  #endif
> --
> 1.9.1
>


-- 
Baolin Wang

Re: [PATCH 1/2] drivers/clocksource/timer-of: Remove __init markings

[Daniel Lezcano]

On 13/04/2020 04:55, Baolin Wang wrote:
> Hi Daniel,
> 
> On Tue, Mar 24, 2020 at 1:59 PM Baolin Wang <baolin.wang7@gmail.com> wrote:
>>
>> From: Saravana Kannan <saravanak@google.com>
>>
>> This allows timer drivers to be compiled as modules.
>>
>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>> Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
> 
> Do you have any comments for this patch set? Thanks.

If my understanding is correct, this patch is part of the GKI picture
where hardware drivers are converted to modules.

But do we really want to convert timer drivers to modules ?

Is the core time framework able to support that (eg. load + unload )


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/2] drivers/clocksource/timer-of: Remove __init markings

[Saravana Kannan]

On Mon, Apr 27, 2020 at 10:13 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 13/04/2020 04:55, Baolin Wang wrote:
> > Hi Daniel,
> >
> > On Tue, Mar 24, 2020 at 1:59 PM Baolin Wang <baolin.wang7@gmail.com> wrote:
> >>
> >> From: Saravana Kannan <saravanak@google.com>
> >>
> >> This allows timer drivers to be compiled as modules.
> >>
> >> Signed-off-by: Saravana Kannan <saravanak@google.com>
> >> Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
> >
> > Do you have any comments for this patch set? Thanks.
>
> If my understanding is correct, this patch is part of the GKI picture
> where hardware drivers are converted to modules.
>
> But do we really want to convert timer drivers to modules ?
>
> Is the core time framework able to support that (eg. load + unload )

So this will mainly be used for secondary timers that the system
supports. Not for the main one that's set up during early boot for
sched timer to work. For the primary timer during boot up, we still
expect that to be the default ARM timer and don't want/expect that to
be a module (it can't be).

-Saravana

Re: [PATCH 1/2] drivers/clocksource/timer-of: Remove __init markings

[Daniel Lezcano]

On 27/04/2020 21:04, Saravana Kannan wrote:
> On Mon, Apr 27, 2020 at 10:13 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 13/04/2020 04:55, Baolin Wang wrote:
>>> Hi Daniel,
>>>
>>> On Tue, Mar 24, 2020 at 1:59 PM Baolin Wang <baolin.wang7@gmail.com> wrote:
>>>>
>>>> From: Saravana Kannan <saravanak@google.com>
>>>>
>>>> This allows timer drivers to be compiled as modules.
>>>>
>>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>>>> Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
>>>
>>> Do you have any comments for this patch set? Thanks.
>>
>> If my understanding is correct, this patch is part of the GKI picture
>> where hardware drivers are converted to modules.
>>
>> But do we really want to convert timer drivers to modules ?
>>
>> Is the core time framework able to support that (eg. load + unload )
> 
> So this will mainly be used for secondary timers that the system
> supports. Not for the main one that's set up during early boot for
> sched timer to work. For the primary timer during boot up, we still
> expect that to be the default ARM timer and don't want/expect that to
> be a module (it can't be).

My question is about clockevents_config_and_register() for instance, is
there a function to unregister in the core framework ?


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/2] drivers/clocksource/timer-of: Remove __init markings

[Saravana Kannan]

On Mon, Apr 27, 2020 at 1:09 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 27/04/2020 21:04, Saravana Kannan wrote:
> > On Mon, Apr 27, 2020 at 10:13 AM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 13/04/2020 04:55, Baolin Wang wrote:
> >>> Hi Daniel,
> >>>
> >>> On Tue, Mar 24, 2020 at 1:59 PM Baolin Wang <baolin.wang7@gmail.com> wrote:
> >>>>
> >>>> From: Saravana Kannan <saravanak@google.com>
> >>>>
> >>>> This allows timer drivers to be compiled as modules.
> >>>>
> >>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
> >>>> Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
> >>>
> >>> Do you have any comments for this patch set? Thanks.
> >>
> >> If my understanding is correct, this patch is part of the GKI picture
> >> where hardware drivers are converted to modules.
> >>
> >> But do we really want to convert timer drivers to modules ?
> >>
> >> Is the core time framework able to support that (eg. load + unload )
> >
> > So this will mainly be used for secondary timers that the system
> > supports. Not for the main one that's set up during early boot for
> > sched timer to work. For the primary timer during boot up, we still
> > expect that to be the default ARM timer and don't want/expect that to
> > be a module (it can't be).
>
> My question is about clockevents_config_and_register() for instance, is
> there a function to unregister in the core framework ?

We can just have these modules be "permanent" modules that can't be
unloaded. They just need to not implement module_exit().

-Saravana

Re: [PATCH 1/2] drivers/clocksource/timer-of: Remove __init markings

[Daniel Lezcano]

On 27/04/2020 22:12, Saravana Kannan wrote:
> On Mon, Apr 27, 2020 at 1:09 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 27/04/2020 21:04, Saravana Kannan wrote:
>>> On Mon, Apr 27, 2020 at 10:13 AM Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> On 13/04/2020 04:55, Baolin Wang wrote:
>>>>> Hi Daniel,
>>>>>
>>>>> On Tue, Mar 24, 2020 at 1:59 PM Baolin Wang <baolin.wang7@gmail.com> wrote:
>>>>>>
>>>>>> From: Saravana Kannan <saravanak@google.com>
>>>>>>
>>>>>> This allows timer drivers to be compiled as modules.
>>>>>>
>>>>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>>>>>> Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
>>>>>
>>>>> Do you have any comments for this patch set? Thanks.
>>>>
>>>> If my understanding is correct, this patch is part of the GKI picture
>>>> where hardware drivers are converted to modules.
>>>>
>>>> But do we really want to convert timer drivers to modules ?
>>>>
>>>> Is the core time framework able to support that (eg. load + unload )
>>>
>>> So this will mainly be used for secondary timers that the system
>>> supports. Not for the main one that's set up during early boot for
>>> sched timer to work. For the primary timer during boot up, we still
>>> expect that to be the default ARM timer and don't want/expect that to
>>> be a module (it can't be).
>>
>> My question is about clockevents_config_and_register() for instance, is
>> there a function to unregister in the core framework ?
> 
> We can just have these modules be "permanent" modules that can't be
> unloaded. They just need to not implement module_exit().

You are right.

I can understand the goal of making everything as much modular as possible.

But TBH, I have a bad feeling about this. Sounds like GKI will give the
opportunity to companies to stop upstreaming their drivers and favoring
fragmentation like what we had several years ago. Not sure it is a good
thing, especially for upstream SoC support.


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/2] drivers/clocksource/timer-of: Remove __init markings

[Sandeep Patil]

Hi Daniel,

On Mon, Apr 27, 2020 at 10:50:24PM +0200, Daniel Lezcano wrote:
> On 27/04/2020 22:12, Saravana Kannan wrote:
> > On Mon, Apr 27, 2020 at 1:09 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 27/04/2020 21:04, Saravana Kannan wrote:
> >>> On Mon, Apr 27, 2020 at 10:13 AM Daniel Lezcano
> >>> <daniel.lezcano@linaro.org> wrote:
> >>>>
> >>>> On 13/04/2020 04:55, Baolin Wang wrote:
> >>>>> Hi Daniel,
> >>>>>
> >>>>> On Tue, Mar 24, 2020 at 1:59 PM Baolin Wang <baolin.wang7@gmail.com> wrote:
> >>>>>>
> >>>>>> From: Saravana Kannan <saravanak@google.com>
> >>>>>>
> >>>>>> This allows timer drivers to be compiled as modules.
> >>>>>>
> >>>>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
> >>>>>> Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
> >>>>>
> >>>>> Do you have any comments for this patch set? Thanks.
> >>>>
> >>>> If my understanding is correct, this patch is part of the GKI picture
> >>>> where hardware drivers are converted to modules.
> >>>>
> >>>> But do we really want to convert timer drivers to modules ?
> >>>>
> >>>> Is the core time framework able to support that (eg. load + unload )
> >>>
> >>> So this will mainly be used for secondary timers that the system
> >>> supports. Not for the main one that's set up during early boot for
> >>> sched timer to work. For the primary timer during boot up, we still
> >>> expect that to be the default ARM timer and don't want/expect that to
> >>> be a module (it can't be).
> >>
> >> My question is about clockevents_config_and_register() for instance, is
> >> there a function to unregister in the core framework ?
> > 
> > We can just have these modules be "permanent" modules that can't be
> > unloaded. They just need to not implement module_exit().
> 
> You are right.
> 
> I can understand the goal of making everything as much modular as possible.
> 
> But TBH, I have a bad feeling about this. Sounds like GKI will give the
> opportunity to companies to stop upstreaming their drivers and favoring
> fragmentation like what we had several years ago. Not sure it is a good
> thing, especially for upstream SoC support.

... and that is a very valid concern too IMO.

However, the way we see it, as things stand today, we don't even know what
goes into Linux on all android phones out there. We know what we add, as part
of the AOSP kernel, however, what actually runs on the device is normally
about a million lines of code changes on top of what we do.

So, for the GKI parts, we are doing the following

1. Making the peripheral drivers modules also means the GKI must have all the
core framework changes built-in. This gets us the list of core kernel changes
that ship on Android devices so we can work on upstreaming them OR find the
appropriate alternative. For Android, that answers the canonical
- "Where is the use case?" question coming from anyone.

You can see the list of these out-of-tree changes is growing by the day in
AOSP right now[1]. Its there for everyone to find in exactly *one place*.
Note that, almost all of those patches have been posted on the list already.
That's the first pre-requisite for any change that goes into AOSP kernel[2].

2. Once we have a core kernel that *truly* works on all Android devices, we
will have built up list of changes we want to upstream (or anyone can pick
them from our public tree). Android will still continue to move to newer
kernel versions easily (may be at a difference cadence ..)

3. About the incentive for upstream SoC support: As part of GKI, we are not
promising a forever stable kernel<->module interface. We still change it each
year. The *best way* for anyone to have their SoC / peripheral supported is
_still_ "going upstream". In fact, we advertise it as such[2]. The modularity
aspect just brings a much needed flexibility for execution.  The flexibility
is needed given the number of stakeholders involved just in the kernel as of
today. (Its a mix of Upstream, Google, SoC manufacturer, device maker and
many other small parts).


4. We also haven't been so keen on the "unloading" of a module. We know there
were subsystems where unloading may not work as expected. Then again, to my
knowledge, nobody has been stress testing with 500+ different modules that
register to all core frameworks being loaded and unloaded at random times.
Even if someone did, we just didn't think its worth the hassle or time at
this moment. Unloading the module didn't buy us anything. (Although, I do get
the point about "correctness"  -- so it shouldn't also be obviously broken)

- ssp

1. https://android.googlesource.com/kernel/common-patches/
2. https://android.googlesource.com/kernel/common/+/refs/heads/android-5.4/README.md

Re: [PATCH 1/2] drivers/clocksource/timer-of: Remove __init markings

[Daniel Lezcano]

Hi Saravana,

On 28/04/2020 00:17, Sandeep Patil wrote:
> Hi Daniel,
> 
> On Mon, Apr 27, 2020 at 10:50:24PM +0200, Daniel Lezcano wrote:
>> On 27/04/2020 22:12, Saravana Kannan wrote:
>>> On Mon, Apr 27, 2020 at 1:09 PM Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> On 27/04/2020 21:04, Saravana Kannan wrote:
>>>>> On Mon, Apr 27, 2020 at 10:13 AM Daniel Lezcano
>>>>> <daniel.lezcano@linaro.org> wrote:
>>>>>>
>>>>>> On 13/04/2020 04:55, Baolin Wang wrote:
>>>>>>> Hi Daniel,
>>>>>>>
>>>>>>> On Tue, Mar 24, 2020 at 1:59 PM Baolin Wang <baolin.wang7@gmail.com> wrote:
>>>>>>>>
>>>>>>>> From: Saravana Kannan <saravanak@google.com>
>>>>>>>>
>>>>>>>> This allows timer drivers to be compiled as modules.
>>>>>>>>
>>>>>>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>>>>>>>> Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
>>>>>>>
>>>>>>> Do you have any comments for this patch set? Thanks.
>>>>>>
>>>>>> If my understanding is correct, this patch is part of the GKI picture
>>>>>> where hardware drivers are converted to modules.
>>>>>>
>>>>>> But do we really want to convert timer drivers to modules ?
>>>>>>
>>>>>> Is the core time framework able to support that (eg. load + unload )
>>>>>
>>>>> So this will mainly be used for secondary timers that the system
>>>>> supports. Not for the main one that's set up during early boot for
>>>>> sched timer to work. For the primary timer during boot up, we still
>>>>> expect that to be the default ARM timer and don't want/expect that to
>>>>> be a module (it can't be).
>>>>
>>>> My question is about clockevents_config_and_register() for instance, is
>>>> there a function to unregister in the core framework ?
>>>
>>> We can just have these modules be "permanent" modules that can't be
>>> unloaded. They just need to not implement module_exit().
>>
>> You are right.
>>
>> I can understand the goal of making everything as much modular as possible.
>>
>> But TBH, I have a bad feeling about this. Sounds like GKI will give the
>> opportunity to companies to stop upstreaming their drivers and favoring
>> fragmentation like what we had several years ago. Not sure it is a good
>> thing, especially for upstream SoC support.
> 
> ... and that is a very valid concern too IMO.
> 
> However, the way we see it, as things stand today, we don't even know what
> goes into Linux on all android phones out there. We know what we add, as part
> of the AOSP kernel, however, what actually runs on the device is normally
> about a million lines of code changes on top of what we do.
> 
> So, for the GKI parts, we are doing the following
> 
> 1. Making the peripheral drivers modules also means the GKI must have all the
> core framework changes built-in. This gets us the list of core kernel changes
> that ship on Android devices so we can work on upstreaming them OR find the
> appropriate alternative. For Android, that answers the canonical
> - "Where is the use case?" question coming from anyone.
> 
> You can see the list of these out-of-tree changes is growing by the day in
> AOSP right now[1]. Its there for everyone to find in exactly *one place*.
> Note that, almost all of those patches have been posted on the list already.
> That's the first pre-requisite for any change that goes into AOSP kernel[2].
> 
> 2. Once we have a core kernel that *truly* works on all Android devices, we
> will have built up list of changes we want to upstream (or anyone can pick
> them from our public tree). Android will still continue to move to newer
> kernel versions easily (may be at a difference cadence ..)
> 
> 3. About the incentive for upstream SoC support: As part of GKI, we are not
> promising a forever stable kernel<->module interface. We still change it each
> year. The *best way* for anyone to have their SoC / peripheral supported is
> _still_ "going upstream". In fact, we advertise it as such[2]. The modularity
> aspect just brings a much needed flexibility for execution.  The flexibility
> is needed given the number of stakeholders involved just in the kernel as of
> today. (Its a mix of Upstream, Google, SoC manufacturer, device maker and
> many other small parts).
> 
> 
> 4. We also haven't been so keen on the "unloading" of a module. We know there
> were subsystems where unloading may not work as expected. Then again, to my
> knowledge, nobody has been stress testing with 500+ different modules that
> register to all core frameworks being loaded and unloaded at random times.
> Even if someone did, we just didn't think its worth the hassle or time at
> this moment. Unloading the module didn't buy us anything. (Although, I do get
> the point about "correctness"  -- so it shouldn't also be obviously broken)

That was my understanding of the GKI, thanks for confirming.

Putting apart the non-technical aspect of these changes, the benefit I
see is the memory usage optimization regarding the single kernel image.

With the ARM64 defconfig, multiple platforms and their corresponding
drivers are compiled-in. It results in a big kernel image which fails to
load because of overlapping on DT load address (or something else). When
that is detected, it is fine to adjust the load addresses, otherwise it
is painful to narrow down the root cause.

In order to prevent this, we have to customize the defconfig each
version release.



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/2] drivers/clocksource/timer-of: Remove __init markings

[Saravana Kannan]

On Tue, Apr 28, 2020 at 12:02 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
>
> Hi Saravana,

You were replying to Sandeep :)

> On 28/04/2020 00:17, Sandeep Patil wrote:
> > Hi Daniel,
> >
> > On Mon, Apr 27, 2020 at 10:50:24PM +0200, Daniel Lezcano wrote:
> >> On 27/04/2020 22:12, Saravana Kannan wrote:
> >>> On Mon, Apr 27, 2020 at 1:09 PM Daniel Lezcano
> >>> <daniel.lezcano@linaro.org> wrote:
> >>>>
> >>>> On 27/04/2020 21:04, Saravana Kannan wrote:
> >>>>> On Mon, Apr 27, 2020 at 10:13 AM Daniel Lezcano
> >>>>> <daniel.lezcano@linaro.org> wrote:
> >>>>>>
> >>>>>> On 13/04/2020 04:55, Baolin Wang wrote:
> >>>>>>> Hi Daniel,
> >>>>>>>
> >>>>>>> On Tue, Mar 24, 2020 at 1:59 PM Baolin Wang <baolin.wang7@gmail.com> wrote:
> >>>>>>>>
> >>>>>>>> From: Saravana Kannan <saravanak@google.com>
> >>>>>>>>
> >>>>>>>> This allows timer drivers to be compiled as modules.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
> >>>>>>>> Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
> >>>>>>>
> >>>>>>> Do you have any comments for this patch set? Thanks.
> >>>>>>
> >>>>>> If my understanding is correct, this patch is part of the GKI picture
> >>>>>> where hardware drivers are converted to modules.
> >>>>>>
> >>>>>> But do we really want to convert timer drivers to modules ?
> >>>>>>
> >>>>>> Is the core time framework able to support that (eg. load + unload )
> >>>>>
> >>>>> So this will mainly be used for secondary timers that the system
> >>>>> supports. Not for the main one that's set up during early boot for
> >>>>> sched timer to work. For the primary timer during boot up, we still
> >>>>> expect that to be the default ARM timer and don't want/expect that to
> >>>>> be a module (it can't be).
> >>>>
> >>>> My question is about clockevents_config_and_register() for instance, is
> >>>> there a function to unregister in the core framework ?
> >>>
> >>> We can just have these modules be "permanent" modules that can't be
> >>> unloaded. They just need to not implement module_exit().
> >>
> >> You are right.
> >>
> >> I can understand the goal of making everything as much modular as possible.
> >>
> >> But TBH, I have a bad feeling about this. Sounds like GKI will give the
> >> opportunity to companies to stop upstreaming their drivers and favoring
> >> fragmentation like what we had several years ago. Not sure it is a good
> >> thing, especially for upstream SoC support.
> >
> > ... and that is a very valid concern too IMO.
> >
> > However, the way we see it, as things stand today, we don't even know what
> > goes into Linux on all android phones out there. We know what we add, as part
> > of the AOSP kernel, however, what actually runs on the device is normally
> > about a million lines of code changes on top of what we do.
> >
> > So, for the GKI parts, we are doing the following
> >
> > 1. Making the peripheral drivers modules also means the GKI must have all the
> > core framework changes built-in. This gets us the list of core kernel changes
> > that ship on Android devices so we can work on upstreaming them OR find the
> > appropriate alternative. For Android, that answers the canonical
> > - "Where is the use case?" question coming from anyone.
> >
> > You can see the list of these out-of-tree changes is growing by the day in
> > AOSP right now[1]. Its there for everyone to find in exactly *one place*.
> > Note that, almost all of those patches have been posted on the list already.
> > That's the first pre-requisite for any change that goes into AOSP kernel[2].
> >
> > 2. Once we have a core kernel that *truly* works on all Android devices, we
> > will have built up list of changes we want to upstream (or anyone can pick
> > them from our public tree). Android will still continue to move to newer
> > kernel versions easily (may be at a difference cadence ..)
> >
> > 3. About the incentive for upstream SoC support: As part of GKI, we are not
> > promising a forever stable kernel<->module interface. We still change it each
> > year. The *best way* for anyone to have their SoC / peripheral supported is
> > _still_ "going upstream". In fact, we advertise it as such[2]. The modularity
> > aspect just brings a much needed flexibility for execution.  The flexibility
> > is needed given the number of stakeholders involved just in the kernel as of
> > today. (Its a mix of Upstream, Google, SoC manufacturer, device maker and
> > many other small parts).
> >
> >
> > 4. We also haven't been so keen on the "unloading" of a module. We know there
> > were subsystems where unloading may not work as expected. Then again, to my
> > knowledge, nobody has been stress testing with 500+ different modules that
> > register to all core frameworks being loaded and unloaded at random times.
> > Even if someone did, we just didn't think its worth the hassle or time at
> > this moment. Unloading the module didn't buy us anything. (Although, I do get
> > the point about "correctness"  -- so it shouldn't also be obviously broken)
>
> That was my understanding of the GKI, thanks for confirming.
>
> Putting apart the non-technical aspect of these changes, the benefit I
> see is the memory usage optimization regarding the single kernel image.
>
> With the ARM64 defconfig, multiple platforms and their corresponding
> drivers are compiled-in. It results in a big kernel image which fails to
> load because of overlapping on DT load address (or something else). When
> that is detected, it is fine to adjust the load addresses, otherwise it
> is painful to narrow down the root cause.
>
> In order to prevent this, we have to customize the defconfig each
> version release.

Sorry, I'm not sure I understand where you are going with this. Are
you agreeing to pick up this change?

-Saravana

Re: [PATCH 1/2] drivers/clocksource/timer-of: Remove __init markings

[Daniel Lezcano]

On 28/04/2020 20:23, Saravana Kannan wrote:
> On Tue, Apr 28, 2020 at 12:02 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>>
>> Hi Saravana,
> 
> You were replying to Sandeep :)

Oh, right :)

Sorry Sandeep. Thanks for taking the time to elaborate a clear statement
of the GKI.

[ ... ]

>> That was my understanding of the GKI, thanks for confirming.
>>
>> Putting apart the non-technical aspect of these changes, the benefit I
>> see is the memory usage optimization regarding the single kernel image.
>>
>> With the ARM64 defconfig, multiple platforms and their corresponding
>> drivers are compiled-in. It results in a big kernel image which fails to
>> load because of overlapping on DT load address (or something else). When
>> that is detected, it is fine to adjust the load addresses, otherwise it
>> is painful to narrow down the root cause.
>>
>> In order to prevent this, we have to customize the defconfig each
>> version release.
> 
> Sorry, I'm not sure I understand where you are going with this. Are
> you agreeing to pick up this change?

Right. I agree with the change but I would like to have Thomas opinion
on this before picking the patch.

Thomas ?


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/2] drivers/clocksource/timer-of: Remove __init markings

[Ulf Hansson]

On Tue, 28 Apr 2020 at 20:55, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 28/04/2020 20:23, Saravana Kannan wrote:
> > On Tue, Apr 28, 2020 at 12:02 AM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >>
> >> Hi Saravana,
> >
> > You were replying to Sandeep :)
>
> Oh, right :)
>
> Sorry Sandeep. Thanks for taking the time to elaborate a clear statement
> of the GKI.
>
> [ ... ]
>
> >> That was my understanding of the GKI, thanks for confirming.
> >>
> >> Putting apart the non-technical aspect of these changes, the benefit I
> >> see is the memory usage optimization regarding the single kernel image.
> >>
> >> With the ARM64 defconfig, multiple platforms and their corresponding
> >> drivers are compiled-in. It results in a big kernel image which fails to
> >> load because of overlapping on DT load address (or something else). When
> >> that is detected, it is fine to adjust the load addresses, otherwise it
> >> is painful to narrow down the root cause.
> >>
> >> In order to prevent this, we have to customize the defconfig each
> >> version release.
> >
> > Sorry, I'm not sure I understand where you are going with this. Are
> > you agreeing to pick up this change?
>
> Right. I agree with the change but I would like to have Thomas opinion
> on this before picking the patch.
>
> Thomas ?

I am not Thomas :-) But just wanted to provide some feedback from my side.

In general we are careful when deciding to export symbols. And at
least, I think at least we should require one user of it before
allowing it to be exported (I assume that is what is happening in
patch2/2 - I couldn't find it)

Kind regards
Uffe

Re: [PATCH 1/2] drivers/clocksource/timer-of: Remove __init markings

[Saravana Kannan]

On Thu, Apr 30, 2020 at 5:57 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 28 Apr 2020 at 20:55, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >
> > On 28/04/2020 20:23, Saravana Kannan wrote:
> > > On Tue, Apr 28, 2020 at 12:02 AM Daniel Lezcano
> > > <daniel.lezcano@linaro.org> wrote:
> > >>
> > >>
> > >> Hi Saravana,
> > >
> > > You were replying to Sandeep :)
> >
> > Oh, right :)
> >
> > Sorry Sandeep. Thanks for taking the time to elaborate a clear statement
> > of the GKI.
> >
> > [ ... ]
> >
> > >> That was my understanding of the GKI, thanks for confirming.
> > >>
> > >> Putting apart the non-technical aspect of these changes, the benefit I
> > >> see is the memory usage optimization regarding the single kernel image.
> > >>
> > >> With the ARM64 defconfig, multiple platforms and their corresponding
> > >> drivers are compiled-in. It results in a big kernel image which fails to
> > >> load because of overlapping on DT load address (or something else). When
> > >> that is detected, it is fine to adjust the load addresses, otherwise it
> > >> is painful to narrow down the root cause.
> > >>
> > >> In order to prevent this, we have to customize the defconfig each
> > >> version release.
> > >
> > > Sorry, I'm not sure I understand where you are going with this. Are
> > > you agreeing to pick up this change?
> >
> > Right. I agree with the change but I would like to have Thomas opinion
> > on this before picking the patch.
> >
> > Thomas ?
>
> I am not Thomas :-) But just wanted to provide some feedback from my side.
>
> In general we are careful when deciding to export symbols. And at
> least, I think at least we should require one user of it before
> allowing it to be exported (I assume that is what is happening in
> patch2/2 - I couldn't find it)

I believe it is done in Patch 2/2, but I think you don't see them in
the diff because they are already called from the driver, but the
driver won't compile as a module due to the removal of __init markings
in the driver.
https://lore.kernel.org/lkml/182aae1ed5e5d2b124f1a32686e5566c9a27c980.1585021186.git.baolin.wang7@gmail.com/

-Saravana