From: Walter Chang <[email protected]>
This patch exports functions in kernel so that timer drivers,
such as timer-mediatek.c can become loadable modules in GKI.
Chun-Hung Wu (3):
time/sched_clock: Export sched_clock_register()
clocksource/drivers/mmio: Export clocksource_mmio_init()
clocksource/drivers/timer-of: Remove __init markings
drivers/clocksource/mmio.c | 8 +++++---
drivers/clocksource/timer-of.c | 23 ++++++++++++-----------
drivers/clocksource/timer-of.h | 6 +++---
kernel/time/sched_clock.c | 4 ++--
4 files changed, 22 insertions(+), 19 deletions(-)
--
2.18.0
From: Chun-Hung Wu <[email protected]>
Export clocksource_mmio_init() and clocksource_mmio_readl_up()
to support building clocksource driver as module,
such as timer-mediatek.c.
Signed-off-by: Chun-Hung Wu <[email protected]>
---
drivers/clocksource/mmio.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/clocksource/mmio.c b/drivers/clocksource/mmio.c
index 9de751531831..b08b2f9d7a8b 100644
--- a/drivers/clocksource/mmio.c
+++ b/drivers/clocksource/mmio.c
@@ -21,6 +21,7 @@ u64 clocksource_mmio_readl_up(struct clocksource *c)
{
return (u64)readl_relaxed(to_mmio_clksrc(c)->reg);
}
+EXPORT_SYMBOL_GPL(clocksource_mmio_readl_up);
u64 clocksource_mmio_readl_down(struct clocksource *c)
{
@@ -46,9 +47,9 @@ u64 clocksource_mmio_readw_down(struct clocksource *c)
* @bits: Number of valid bits
* @read: One of clocksource_mmio_read*() above
*/
-int __init clocksource_mmio_init(void __iomem *base, const char *name,
- unsigned long hz, int rating, unsigned bits,
- u64 (*read)(struct clocksource *))
+int clocksource_mmio_init(void __iomem *base, const char *name,
+ unsigned long hz, int rating, unsigned bits,
+ u64 (*read)(struct clocksource *))
{
struct clocksource_mmio *cs;
@@ -68,3 +69,4 @@ int __init clocksource_mmio_init(void __iomem *base, const char *name,
return clocksource_register_hz(&cs->clksrc, hz);
}
+EXPORT_SYMBOL_GPL(clocksource_mmio_init);
--
2.18.0
From: Chun-Hung Wu <[email protected]>
clocksource driver may use sched_clock_register()
to resigter itself as a sched_clock source.
Export it to support building such driver
as module, like timer-mediatek.c
Signed-off-by: Chun-Hung Wu <[email protected]>
---
kernel/time/sched_clock.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
index 8464c5acc913..8e49e87d1221 100644
--- a/kernel/time/sched_clock.c
+++ b/kernel/time/sched_clock.c
@@ -150,8 +150,7 @@ static enum hrtimer_restart sched_clock_poll(struct hrtimer *hrt)
return HRTIMER_RESTART;
}
-void __init
-sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
+void sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
{
u64 res, wrap, new_mask, new_epoch, cyc, ns;
u32 new_mult, new_shift;
@@ -223,6 +222,7 @@ sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
pr_debug("Registered %pS as sched_clock source\n", read);
}
+EXPORT_SYMBOL_GPL(sched_clock_register);
void __init generic_sched_clock_init(void)
{
--
2.18.0
From: Chun-Hung Wu <[email protected]>
This allows timer drivers to be compiled as modules.
Link: https://lore.kernel.org/patchwork/patch/1214447/
Signed-off-by: Chun-Hung Wu <[email protected]>
---
drivers/clocksource/timer-of.c | 23 ++++++++++++-----------
drivers/clocksource/timer-of.h | 6 +++---
2 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/drivers/clocksource/timer-of.c b/drivers/clocksource/timer-of.c
index c3f54d9912be..59bc5921acad 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,8 +47,8 @@ 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,
- struct of_timer_irq *of_irq)
+static int timer_of_irq_init(struct device_node *np,
+ struct of_timer_irq *of_irq)
{
int ret;
struct timer_of *to = container_of(of_irq, struct timer_of, of_irq);
@@ -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,8 +107,8 @@ 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,
- struct of_timer_clk *of_clk)
+static int timer_of_clk_init(struct device_node *np,
+ struct of_timer_clk *of_clk)
{
int ret;
@@ -146,13 +146,13 @@ 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,
- struct of_timer_base *of_base)
+static int timer_of_base_init(struct device_node *np,
+ struct of_timer_base *of_base)
{
of_base->base = of_base->name ?
of_io_request_and_map(np, of_base->index, 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 resources
@@ -217,7 +218,7 @@ int __init timer_of_init(struct device_node *np, struct timer_of *to)
* Release the resources 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 a5478f3e8589..5d1472846346 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,
- struct timer_of *to);
+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
--
2.18.0
On 08/02/2023 10:48, [email protected] wrote:
> From: Chun-Hung Wu <[email protected]>
>
> clocksource driver may use sched_clock_register()
> to resigter itself as a sched_clock source.
> Export it to support building such driver
> as module, like timer-mediatek.c
>
> Signed-off-by: Chun-Hung Wu <[email protected]>
> ---
> kernel/time/sched_clock.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
> index 8464c5acc913..8e49e87d1221 100644
> --- a/kernel/time/sched_clock.c
> +++ b/kernel/time/sched_clock.c
> @@ -150,8 +150,7 @@ static enum hrtimer_restart sched_clock_poll(struct hrtimer *hrt)
> return HRTIMER_RESTART;
> }
>
> -void __init
> -sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
> +void sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
Is there a non-init caller?
> {
> u64 res, wrap, new_mask, new_epoch, cyc, ns;
> u32 new_mult, new_shift;
> @@ -223,6 +222,7 @@ sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
>
> pr_debug("Registered %pS as sched_clock source\n", read);
> }
> +EXPORT_SYMBOL_GPL(sched_clock_register);
Where is the module using it?
You need to bring users of these two changes, not just prepare something
for your out of tree patches.
Best regards,
Krzysztof
On 08/02/2023 15:24, Krzysztof Kozlowski wrote:
> On 08/02/2023 10:48, [email protected] wrote:
>> From: Chun-Hung Wu <[email protected]>
>>
>> clocksource driver may use sched_clock_register()
>> to resigter itself as a sched_clock source.
>> Export it to support building such driver
>> as module, like timer-mediatek.c
>>
>> Signed-off-by: Chun-Hung Wu <[email protected]>
>> ---
>> kernel/time/sched_clock.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
>> index 8464c5acc913..8e49e87d1221 100644
>> --- a/kernel/time/sched_clock.c
>> +++ b/kernel/time/sched_clock.c
>> @@ -150,8 +150,7 @@ static enum hrtimer_restart sched_clock_poll(struct hrtimer *hrt)
>> return HRTIMER_RESTART;
>> }
>>
>> -void __init
>> -sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
>> +void sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
>
> Is there a non-init caller?
>
>> {
>> u64 res, wrap, new_mask, new_epoch, cyc, ns;
>> u32 new_mult, new_shift;
>> @@ -223,6 +222,7 @@ sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
>>
>> pr_debug("Registered %pS as sched_clock source\n", read);
>> }
>> +EXPORT_SYMBOL_GPL(sched_clock_register);
>
> Where is the module using it?
>
> You need to bring users of these two changes, not just prepare something
> for your out of tree patches.
>
I'd propose to add at least one driver that will need these changes, to make it
clear why you need that.
Regards,
Matthias
On 08/02/2023 20:41, Matthias Brugger wrote:
>
>
> On 08/02/2023 15:24, Krzysztof Kozlowski wrote:
>> On 08/02/2023 10:48, [email protected] wrote:
>>> From: Chun-Hung Wu <[email protected]>
>>>
>>> clocksource driver may use sched_clock_register()
>>> to resigter itself as a sched_clock source.
>>> Export it to support building such driver
>>> as module, like timer-mediatek.c
>>>
>>> Signed-off-by: Chun-Hung Wu <[email protected]>
>>> ---
>>> kernel/time/sched_clock.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
>>> index 8464c5acc913..8e49e87d1221 100644
>>> --- a/kernel/time/sched_clock.c
>>> +++ b/kernel/time/sched_clock.c
>>> @@ -150,8 +150,7 @@ static enum hrtimer_restart sched_clock_poll(struct hrtimer *hrt)
>>> return HRTIMER_RESTART;
>>> }
>>>
>>> -void __init
>>> -sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
>>> +void sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
>>
>> Is there a non-init caller?
>>
>>> {
>>> u64 res, wrap, new_mask, new_epoch, cyc, ns;
>>> u32 new_mult, new_shift;
>>> @@ -223,6 +222,7 @@ sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
>>>
>>> pr_debug("Registered %pS as sched_clock source\n", read);
>>> }
>>> +EXPORT_SYMBOL_GPL(sched_clock_register);
>>
>> Where is the module using it?
>>
>> You need to bring users of these two changes, not just prepare something
>> for your out of tree patches.
>>
>
> I'd propose to add at least one driver that will need these changes, to make it
> clear why you need that.
... and actually test if the system works fine when booted from such
clocksource as a module. I have doubts that and unfortunately folks
working on GKI like to put whatever stuff from mainline into modules
even if it does not make sense for us (see long time ago discussion
about pinctrl drivers).
Best regards,
Krzysztof
On 08/02/2023 20:45, Krzysztof Kozlowski wrote:
> On 08/02/2023 20:41, Matthias Brugger wrote:
>>
>>
>> On 08/02/2023 15:24, Krzysztof Kozlowski wrote:
>>> On 08/02/2023 10:48, [email protected] wrote:
>>>> From: Chun-Hung Wu <[email protected]>
>>>>
>>>> clocksource driver may use sched_clock_register()
>>>> to resigter itself as a sched_clock source.
>>>> Export it to support building such driver
>>>> as module, like timer-mediatek.c
>>>>
>>>> Signed-off-by: Chun-Hung Wu <[email protected]>
>>>> ---
>>>> kernel/time/sched_clock.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
>>>> index 8464c5acc913..8e49e87d1221 100644
>>>> --- a/kernel/time/sched_clock.c
>>>> +++ b/kernel/time/sched_clock.c
>>>> @@ -150,8 +150,7 @@ static enum hrtimer_restart sched_clock_poll(struct hrtimer *hrt)
>>>> return HRTIMER_RESTART;
>>>> }
>>>>
>>>> -void __init
>>>> -sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
>>>> +void sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
>>>
>>> Is there a non-init caller?
>>>
>>>> {
>>>> u64 res, wrap, new_mask, new_epoch, cyc, ns;
>>>> u32 new_mult, new_shift;
>>>> @@ -223,6 +222,7 @@ sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
>>>>
>>>> pr_debug("Registered %pS as sched_clock source\n", read);
>>>> }
>>>> +EXPORT_SYMBOL_GPL(sched_clock_register);
>>>
>>> Where is the module using it?
>>>
>>> You need to bring users of these two changes, not just prepare something
>>> for your out of tree patches.
>>>
>>
>> I'd propose to add at least one driver that will need these changes, to make it
>> clear why you need that.
>
> ... and actually test if the system works fine when booted from such
> clocksource as a module. I have doubts that and unfortunately folks
> working on GKI like to put whatever stuff from mainline into modules
> even if it does not make sense for us (see long time ago discussion
> about pinctrl drivers).
>
Yes thinking about it twice, it makes only sense if the Arm architecture timer
is running. Otherwise the system will hang on boot. I know that older MediaTek
devices had problems with that...
Il 08/02/23 23:22, Matthias Brugger ha scritto:
>
>
> On 08/02/2023 20:45, Krzysztof Kozlowski wrote:
>> On 08/02/2023 20:41, Matthias Brugger wrote:
>>>
>>>
>>> On 08/02/2023 15:24, Krzysztof Kozlowski wrote:
>>>> On 08/02/2023 10:48, [email protected] wrote:
>>>>> From: Chun-Hung Wu <[email protected]>
>>>>>
>>>>> clocksource driver may use sched_clock_register()
>>>>> to resigter itself as a sched_clock source.
>>>>> Export it to support building such driver
>>>>> as module, like timer-mediatek.c
>>>>>
>>>>> Signed-off-by: Chun-Hung Wu <[email protected]>
>>>>> ---
>>>>> kernel/time/sched_clock.c | 4 ++--
>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
>>>>> index 8464c5acc913..8e49e87d1221 100644
>>>>> --- a/kernel/time/sched_clock.c
>>>>> +++ b/kernel/time/sched_clock.c
>>>>> @@ -150,8 +150,7 @@ static enum hrtimer_restart sched_clock_poll(struct
>>>>> hrtimer *hrt)
>>>>> return HRTIMER_RESTART;
>>>>> }
>>>>> -void __init
>>>>> -sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
>>>>> +void sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
>>>>
>>>> Is there a non-init caller?
>>>>
>>>>> {
>>>>> u64 res, wrap, new_mask, new_epoch, cyc, ns;
>>>>> u32 new_mult, new_shift;
>>>>> @@ -223,6 +222,7 @@ sched_clock_register(u64 (*read)(void), int bits, unsigned
>>>>> long rate)
>>>>> pr_debug("Registered %pS as sched_clock source\n", read);
>>>>> }
>>>>> +EXPORT_SYMBOL_GPL(sched_clock_register);
>>>>
>>>> Where is the module using it?
>>>>
>>>> You need to bring users of these two changes, not just prepare something
>>>> for your out of tree patches.
>>>>
>>>
>>> I'd propose to add at least one driver that will need these changes, to make it
>>> clear why you need that.
>>
>> ... and actually test if the system works fine when booted from such
>> clocksource as a module. I have doubts that and unfortunately folks
>> working on GKI like to put whatever stuff from mainline into modules
>> even if it does not make sense for us (see long time ago discussion
>> about pinctrl drivers).
>>
>
> Yes thinking about it twice, it makes only sense if the Arm architecture timer is
> running. Otherwise the system will hang on boot. I know that older MediaTek devices
> had problems with that...
I think also some very old Qualcomm SoCs have the same timer "issue" and I bet that
some others as well do.
Now, I won't argue about the benefits or drawbacks of having X, Y or Z as a module
because it's probably not the right time/place to... but!
I was trying to get my brain ticking on this one and I immediately didn't like it:
as a matter of fact, this kind of clocksources (especially the arch timer[s]) is
boot critical and that's not limited to ARM, anyway... this means that a such a
change can't be *that* easy, at all.
If you really want to register such a clocksource driver, I would suggest instead
to make an addition that allows you to do so, while *not* touching common code
paths that are called by multiple architectures and that may need those to stay
as they are for one reason or another.
*If* this kind of modularization will ever happen, it's something that must be
done slowly and again, not in one shot, especially not with one series "taking
care of 'em all". Please be careful when touching *core* code.
That was just an opinion on something that I can envision to be eventually going
wrong in many, many ways...
Regards,
Angelo
On 08/02/2023 20:45, Krzysztof Kozlowski wrote:
> On 08/02/2023 20:41, Matthias Brugger wrote:
>>
>>
>> On 08/02/2023 15:24, Krzysztof Kozlowski wrote:
>>> On 08/02/2023 10:48, [email protected] wrote:
>>>> From: Chun-Hung Wu <[email protected]>
>>>>
>>>> clocksource driver may use sched_clock_register()
>>>> to resigter itself as a sched_clock source.
>>>> Export it to support building such driver
>>>> as module, like timer-mediatek.c
>>>>
>>>> Signed-off-by: Chun-Hung Wu <[email protected]>
>>>> ---
[ ... ]
> ... and actually test if the system works fine when booted from such
> clocksource as a module. I have doubts that and unfortunately folks
> working on GKI like to put whatever stuff from mainline into modules
> even if it does not make sense for us (see long time ago discussion
> about pinctrl drivers).
+1
It is not the first time there is a proposal to convert the timers to
modules. After asking, nobody came with a real study regarding the
impact of the modularization of these drivers vs the time core framework
and the benefit.
My gut feeling is that is not that simple.
--
<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
On 08/02/2023 10:48, [email protected] wrote:
> From: Walter Chang <[email protected]>
>
> This patch exports functions in kernel so that timer drivers,
> such as timer-mediatek.c can become loadable modules in GKI.
What for ?
--
<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
On Wed, Feb 08, 2023 at 08:45:15PM +0100, Krzysztof Kozlowski wrote:
> On 08/02/2023 20:41, Matthias Brugger wrote:
> > On 08/02/2023 15:24, Krzysztof Kozlowski wrote:
> >> On 08/02/2023 10:48, [email protected] wrote:
> >>> From: Chun-Hung Wu <[email protected]>
> >>>
> >>> clocksource driver may use sched_clock_register()
> >>> to resigter itself as a sched_clock source.
> >>> Export it to support building such driver
> >>> as module, like timer-mediatek.c
> >>>
> >>> Signed-off-by: Chun-Hung Wu <[email protected]>
> >>> ---
> >>> kernel/time/sched_clock.c | 4 ++--
> >>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
> >>> index 8464c5acc913..8e49e87d1221 100644
> >>> --- a/kernel/time/sched_clock.c
> >>> +++ b/kernel/time/sched_clock.c
> >>> @@ -150,8 +150,7 @@ static enum hrtimer_restart sched_clock_poll(struct hrtimer *hrt)
> >>> return HRTIMER_RESTART;
> >>> }
> >>>
> >>> -void __init
> >>> -sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
> >>> +void sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
> >>
> >> Is there a non-init caller?
> >>
> >>> {
> >>> u64 res, wrap, new_mask, new_epoch, cyc, ns;
> >>> u32 new_mult, new_shift;
> >>> @@ -223,6 +222,7 @@ sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
> >>>
> >>> pr_debug("Registered %pS as sched_clock source\n", read);
> >>> }
> >>> +EXPORT_SYMBOL_GPL(sched_clock_register);
> >>
> >> Where is the module using it?
> >>
> >> You need to bring users of these two changes, not just prepare something
> >> for your out of tree patches.
> >>
> >
> > I'd propose to add at least one driver that will need these changes, to make it
> > clear why you need that.
>
> ... and actually test if the system works fine when booted from such
> clocksource as a module. I have doubts that and unfortunately folks
> working on GKI like to put whatever stuff from mainline into modules
> even if it does not make sense for us (see long time ago discussion
> about pinctrl drivers).
Just to add, system boot test with these as modules are not sufficient
based on the usage of such timers. I am aware of systems where arm
architected timer are functional in normal running state of the CPU.
They may have bugs/errata where these arch timers(mmio access) are not
available when CPUs enter low power idle states. Generally, CPUs enter
idle states on boot, but one CPU may not as it will act as broadcast timer.
With this added timer, they may achieve all CPUs entering idle states
properly.
In short the system might be bootable w/o such timers but not power
efficient. We need to tests that involve loading and unloading of such
modules to see if the transition between this timer as broadcast and one
CPU itself as broadcast happens correctly and system survives across such
loading and unloading of the modules.
Note I am not for/against allowing the such timer as module, but just
adding possible testcase to pass to consider if we are allowing it.
--
Regards,
Sudeep
On Thu, Feb 9, 2023 at 7:36 AM Daniel Lezcano <[email protected]> wrote:
>
> On 08/02/2023 10:48, [email protected] wrote:
> > From: Walter Chang <[email protected]>
> >
> > This patch exports functions in kernel so that timer drivers,
> > such as timer-mediatek.c can become loadable modules in GKI.
>
> What for ?
In general, it's the same reason why modules exist: We want to be able
to support a wide array of devices with a single kernel, but we don't
want all devices to pay the memory cost of code that will never be
used there. So being able to support loading device-specific bits like
clocksources (along with other device specific logic) helps.
Obviously it still has to make sense, and others have raised concerns
of stability issues if the hardware support is needed before we can
get to module loading, but I think if this allows drivers (such as
timer-mediatek) to be loadable safely, I see it as beneficial.
(And as others pointed out: this patch series is incomplete as it
doesn't modularize the timer-mediatek driver, which would be a prereq
to supporting it upstream)
thanks
-john
On Thu, Feb 09, 2023 at 11:50:49AM -0800, John Stultz wrote:
> On Thu, Feb 9, 2023 at 7:36 AM Daniel Lezcano <[email protected]> wrote:
> >
> > On 08/02/2023 10:48, [email protected] wrote:
> > > From: Walter Chang <[email protected]>
> > >
> > > This patch exports functions in kernel so that timer drivers,
> > > such as timer-mediatek.c can become loadable modules in GKI.
> >
> > What for ?
>
> In general, it's the same reason why modules exist: We want to be able
> to support a wide array of devices with a single kernel, but we don't
> want all devices to pay the memory cost of code that will never be
> used there. So being able to support loading device-specific bits like
> clocksources (along with other device specific logic) helps.
Agree, that is why modules are for.
> Obviously it still has to make sense, and others have raised concerns
> of stability issues if the hardware support is needed before we can
> get to module loading, but I think if this allows drivers (such as
> timer-mediatek) to be loadable safely, I see it as beneficial.
From a technical point of view, it is arguable.
But my main concern is the real reason of changing this to the module
format. I see that as a way to overcome the effort to upstream the
drivers. And the GKI is an alibi to justify the module conversion.
Given the timers is a base brick of the core subsystems, without
proper support of the timer (eg. bug fixes), the platform support will
be wobbly.
--
<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
On Wed, 2023-02-08 at 20:41 +0100, Matthias Brugger wrote:
>
> On 08/02/2023 15:24, Krzysztof Kozlowski wrote:
> > On 08/02/2023 10:48, [email protected] wrote:
> > > From: Chun-Hung Wu <[email protected]>
> > >
> > > clocksource driver may use sched_clock_register()
> > > to resigter itself as a sched_clock source.
> > > Export it to support building such driver
> > > as module, like timer-mediatek.c
> > >
> > > Signed-off-by: Chun-Hung Wu <[email protected]>
> > > ---
> > > kernel/time/sched_clock.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/kernel/time/sched_clock.c
> > > b/kernel/time/sched_clock.c
> > > index 8464c5acc913..8e49e87d1221 100644
> > > --- a/kernel/time/sched_clock.c
> > > +++ b/kernel/time/sched_clock.c
> > > @@ -150,8 +150,7 @@ static enum hrtimer_restart
> > > sched_clock_poll(struct hrtimer *hrt)
> > > return HRTIMER_RESTART;
> > > }
> > >
> > > -void __init
> > > -sched_clock_register(u64 (*read)(void), int bits, unsigned long
> > > rate)
> > > +void sched_clock_register(u64 (*read)(void), int bits, unsigned
> > > long rate)
> >
> > Is there a non-init caller?
> >
> > > {
> > > u64 res, wrap, new_mask, new_epoch, cyc, ns;
> > > u32 new_mult, new_shift;
> > > @@ -223,6 +222,7 @@ sched_clock_register(u64 (*read)(void), int
> > > bits, unsigned long rate)
> > >
> > > pr_debug("Registered %pS as sched_clock source\n",
> > > read);
> > > }
> > > +EXPORT_SYMBOL_GPL(sched_clock_register);
> >
> > Where is the module using it?
> >
> > You need to bring users of these two changes, not just prepare
> > something
> > for your out of tree patches.
> >
>
> I'd propose to add at least one driver that will need these changes,
> to make it
> clear why you need that.
>
> Regards,
> Matthias
I have uploaded another patch to make timer-mediatek.c driver become
loadable module.
https://lore.kernel.org/lkml/[email protected]/T/#u
This driver registers an always-on timer as tick_broadcast_device
on MediaTek SoCs. If the system does not load this module, system
would also boot normally since Arm Generic Timer will take over
this part.
Thanks,
Walter Chang
On Fri, Feb 10, 2023 at 12:52 AM Daniel Lezcano
<[email protected]> wrote:
> On Thu, Feb 09, 2023 at 11:50:49AM -0800, John Stultz wrote:
> > On Thu, Feb 9, 2023 at 7:36 AM Daniel Lezcano <[email protected]> wrote:
> > > What for ?
> >
> > In general, it's the same reason why modules exist: We want to be able
> > to support a wide array of devices with a single kernel, but we don't
> > want all devices to pay the memory cost of code that will never be
> > used there. So being able to support loading device-specific bits like
> > clocksources (along with other device specific logic) helps.
>
> Agree, that is why modules are for.
>
> > Obviously it still has to make sense, and others have raised concerns
> > of stability issues if the hardware support is needed before we can
> > get to module loading, but I think if this allows drivers (such as
> > timer-mediatek) to be loadable safely, I see it as beneficial.
>
> From a technical point of view, it is arguable.
>
> But my main concern is the real reason of changing this to the module
> format. I see that as a way to overcome the effort to upstream the
> drivers. And the GKI is an alibi to justify the module conversion.
[Putting on my Android Antennae for a moment]
I can promise the GKI is no alibi - it is a real thing. Part of
convincing vendors to ship the same kernel is that we have to be able
to bring the security benefits of being able to update the unified
kernel without major impact to memory. Utilizing modules (all over -
as a lot of small cuts add up) is crucial for that.
Some vendors haven't historically been great about upstreaming device
support, and I understand the concern that allowing modules might
enable vendors to keep modules out of tree. But vendors inclined to do
that will find a way regardless (and because at a practical level,
because the need to keep the GKI size down the android tree will have
to carry a similar change to the one submitted here), so I don't think
rejecting such patches is a real disincentive.
Instead it just creates further needless fragmentation between
upstream and android kernels, which makes it further difficult to
justify and motivate upstream-hesitant vendors to submit their code to
lkml.
And again, there *has* to be upstream users, as we should not have any
maintenance burden for out of tree code! But in this case the upstream
timer-mediatek driver was named as a candidate module (obviously those
patches are needed when this series is re-sent).
Additionally, while I understand the concern and skepticism, for folks
who are working on upstreaming (Mediatek along with folks at Collabora
have done some great work!), having to deal with this meta-issue of
questioning of one's purity-of-intent, when one is actually submitting
patches I think makes the process of dealing with the community seem
even more difficult (making some folks question why bother).
The upstream kernel community is an amazing thing! And I understand
why we want to be protective of it. But I also worry that if we get
too wrapped up in suspicions of ill intent, we aren't going to be able
to bring folks into the fold and grow the community. The license
doesn't require one to work with the community, so we should probably
be using more carrots and fewer sticks.
> Given the timers is a base brick of the core subsystems, without
> proper support of the timer (eg. bug fixes), the platform support will
> be wobbly.
As for bug-fixes, it should be noted that with the GKI, not all
modules are vendor modules! There are GKI-modules, which are common
drivers/subsystem-infrastructure used by many devices (such as NFC,
Bluetooth, etc), and these in-tree drivers are updated with the GKI
kernel as modules. So there is motivation to ensure bug fixes to those
upstream drivers land upstream so they can be included when the GKI
and GKI modules are updated.
And to your point about it being a base-brick - Yes, obviously not
everything can be loaded from a module safely, and that's fine. But in
cases where devices can boot with a built in architected timer, then
are able to switch to more device specific clocksources, we'd really
like those device specific clocksources to be modules.
thanks
-john