[v2,4/4] clocksource/drivers/timer-mediatek: Make timer-mediatek become loadable module

  From: Chun-Hung Wu <chun-hung.wu@mediatek.com>

Make the timer-mediatek driver which can register
an always-on timer as tick_broadcast_device on
MediaTek SoCs become loadable module in GKI.

Signed-off-by: Chun-Hung Wu <chun-hung.wu@mediatek.com>
---
 drivers/clocksource/Kconfig          |  2 +-
 drivers/clocksource/timer-mediatek.c | 43 ++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 1 deletion(-)
  

On 14/02/2023 11:53, walter.chang@mediatek.com wrote:
> From: Chun-Hung Wu <chun-hung.wu@mediatek.com>
> 
> Make the timer-mediatek driver which can register
> an always-on timer as tick_broadcast_device on
> MediaTek SoCs become loadable module in GKI.

Other questions are unanswered. Please do not ignore feedback and
respond to it.

Best regards,
Krzysztof
  

On Tue, Feb 14, 2023 at 06:53:14PM +0800, walter.chang@mediatek.com wrote:
> From: Chun-Hung Wu <chun-hung.wu@mediatek.com>
> 
> Make the timer-mediatek driver which can register
> an always-on timer as tick_broadcast_device on
> MediaTek SoCs become loadable module in GKI.
> 
> Signed-off-by: Chun-Hung Wu <chun-hung.wu@mediatek.com>
> ---
>  drivers/clocksource/Kconfig          |  2 +-
>  drivers/clocksource/timer-mediatek.c | 43 ++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+), 1 deletion(-)

[...]

> diff --git a/drivers/clocksource/timer-mediatek.c b/drivers/clocksource/timer-mediatek.c
> index d5b29fd03ca2..3358758ea694 100644
> --- a/drivers/clocksource/timer-mediatek.c
> +++ b/drivers/clocksource/timer-mediatek.c

[...]

> +static const struct of_device_id mtk_timer_match_table[] = {
> +	{
> +		.compatible = "mediatek,mt6577-timer",
> +		.data = mtk_gpt_init,
> +	},
> +	{
> +		.compatible = "mediatek,mt6765-timer",
> +		.data = mtk_syst_init,
> +	},
> +	{
> +		.compatible = "mediatek,mt6795-systimer",
> +		.data = mtk_cpux_init,
> +	},
> +	{}
> +};
> +
> +static struct platform_driver mtk_timer_driver = {
> +	.probe = mtk_timer_probe,
> +	.driver = {
> +		.name = "mtk-timer",
> +		.of_match_table = mtk_timer_match_table,
> +	},
> +};
> +module_platform_driver(mtk_timer_driver);
> +
> +MODULE_DESCRIPTION("MediaTek Module Timer driver");
> +MODULE_LICENSE("GPL v2");
> +#else
>  TIMER_OF_DECLARE(mtk_mt6577, "mediatek,mt6577-timer", mtk_gpt_init);
>  TIMER_OF_DECLARE(mtk_mt6765, "mediatek,mt6765-timer", mtk_syst_init);
>  TIMER_OF_DECLARE(mtk_mt6795, "mediatek,mt6795-systimer", mtk_cpux_init);

Why do you need these ? If this driver can work as a module, it can be
built-in module and doesn't need to be initialised early using of_timer_init
(can't recall the exact name)
  

On Tue, Feb 14, 2023 at 3:09 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
> On 14/02/2023 11:53, walter.chang@mediatek.com wrote:
> > From: Chun-Hung Wu <chun-hung.wu@mediatek.com>
> >
> > Make the timer-mediatek driver which can register
> > an always-on timer as tick_broadcast_device on
> > MediaTek SoCs become loadable module in GKI.
>
> Other questions are unanswered. Please do not ignore feedback and
> respond to it.
>

Apologies, I know it can be a pain to repeat yourself, but would you
clarify which questions were unanswered?
My initial skim made it seem like the items you raised were addressed
in some form (though maybe not sufficiently?).

thanks
-john
  

Il 14/02/23 23:20, Sudeep Holla ha scritto:
> On Tue, Feb 14, 2023 at 06:53:14PM +0800, walter.chang@mediatek.com wrote:
>> From: Chun-Hung Wu <chun-hung.wu@mediatek.com>
>>
>> Make the timer-mediatek driver which can register
>> an always-on timer as tick_broadcast_device on
>> MediaTek SoCs become loadable module in GKI.
>>
>> Signed-off-by: Chun-Hung Wu <chun-hung.wu@mediatek.com>
>> ---
>>   drivers/clocksource/Kconfig          |  2 +-
>>   drivers/clocksource/timer-mediatek.c | 43 ++++++++++++++++++++++++++++
>>   2 files changed, 44 insertions(+), 1 deletion(-)
> 
> [...]
> 
>> diff --git a/drivers/clocksource/timer-mediatek.c b/drivers/clocksource/timer-mediatek.c
>> index d5b29fd03ca2..3358758ea694 100644
>> --- a/drivers/clocksource/timer-mediatek.c
>> +++ b/drivers/clocksource/timer-mediatek.c
> 
> [...]
> 
>> +static const struct of_device_id mtk_timer_match_table[] = {
>> +	{
>> +		.compatible = "mediatek,mt6577-timer",
>> +		.data = mtk_gpt_init,
>> +	},
>> +	{
>> +		.compatible = "mediatek,mt6765-timer",
>> +		.data = mtk_syst_init,
>> +	},
>> +	{
>> +		.compatible = "mediatek,mt6795-systimer",
>> +		.data = mtk_cpux_init,
>> +	},
>> +	{}
>> +};
>> +
>> +static struct platform_driver mtk_timer_driver = {
>> +	.probe = mtk_timer_probe,
>> +	.driver = {
>> +		.name = "mtk-timer",
>> +		.of_match_table = mtk_timer_match_table,
>> +	},
>> +};
>> +module_platform_driver(mtk_timer_driver);
>> +
>> +MODULE_DESCRIPTION("MediaTek Module Timer driver");
>> +MODULE_LICENSE("GPL v2");
>> +#else
>>   TIMER_OF_DECLARE(mtk_mt6577, "mediatek,mt6577-timer", mtk_gpt_init);
>>   TIMER_OF_DECLARE(mtk_mt6765, "mediatek,mt6765-timer", mtk_syst_init);
>>   TIMER_OF_DECLARE(mtk_mt6795, "mediatek,mt6795-systimer", mtk_cpux_init);
> 
> Why do you need these ? If this driver can work as a module, it can be
> built-in module and doesn't need to be initialised early using of_timer_init
> (can't recall the exact name)
> 
> 

Some platforms need early initialization; this is seen on ones for which the
bootloader does not initialize the "CPUXGPT" timer, which is used as the ARM
arch timer. (No, on those platforms you can't upgrade the bootloader, as it's
signed with a OEM key which is not obtainable, and signature verified earlier
in the bootchain).

As a matter of fact (and somehow obvious), on those platforms (for example,
MT6795.. but many other as well, really), you *need* this driver to be
built-in and, well, initialize the CPUX timer as early as possible :-)

Regards,
Angelo
  

On Wed, Feb 15, 2023 at 01:43:19PM +0100, AngeloGioacchino Del Regno wrote:
> Il 14/02/23 23:20, Sudeep Holla ha scritto:
> > On Tue, Feb 14, 2023 at 06:53:14PM +0800, walter.chang@mediatek.com wrote:
> > > From: Chun-Hung Wu <chun-hung.wu@mediatek.com>
> > > 
> > > Make the timer-mediatek driver which can register
> > > an always-on timer as tick_broadcast_device on
> > > MediaTek SoCs become loadable module in GKI.
> > > 
> > > Signed-off-by: Chun-Hung Wu <chun-hung.wu@mediatek.com>
> > > ---
> > >   drivers/clocksource/Kconfig          |  2 +-
> > >   drivers/clocksource/timer-mediatek.c | 43 ++++++++++++++++++++++++++++
> > >   2 files changed, 44 insertions(+), 1 deletion(-)
> > 
> > [...]
> > 
> > > diff --git a/drivers/clocksource/timer-mediatek.c b/drivers/clocksource/timer-mediatek.c
> > > index d5b29fd03ca2..3358758ea694 100644
> > > --- a/drivers/clocksource/timer-mediatek.c
> > > +++ b/drivers/clocksource/timer-mediatek.c
> > 
> > [...]
> > 
> > > +static const struct of_device_id mtk_timer_match_table[] = {
> > > +	{
> > > +		.compatible = "mediatek,mt6577-timer",
> > > +		.data = mtk_gpt_init,
> > > +	},
> > > +	{
> > > +		.compatible = "mediatek,mt6765-timer",
> > > +		.data = mtk_syst_init,
> > > +	},
> > > +	{
> > > +		.compatible = "mediatek,mt6795-systimer",
> > > +		.data = mtk_cpux_init,
> > > +	},
> > > +	{}
> > > +};
> > > +
> > > +static struct platform_driver mtk_timer_driver = {
> > > +	.probe = mtk_timer_probe,
> > > +	.driver = {
> > > +		.name = "mtk-timer",
> > > +		.of_match_table = mtk_timer_match_table,
> > > +	},
> > > +};
> > > +module_platform_driver(mtk_timer_driver);
> > > +
> > > +MODULE_DESCRIPTION("MediaTek Module Timer driver");
> > > +MODULE_LICENSE("GPL v2");
> > > +#else
> > >   TIMER_OF_DECLARE(mtk_mt6577, "mediatek,mt6577-timer", mtk_gpt_init);
> > >   TIMER_OF_DECLARE(mtk_mt6765, "mediatek,mt6765-timer", mtk_syst_init);
> > >   TIMER_OF_DECLARE(mtk_mt6795, "mediatek,mt6795-systimer", mtk_cpux_init);
> >
> > Why do you need these ? If this driver can work as a module, it can be
> > built-in module and doesn't need to be initialised early using of_timer_init
> > (can't recall the exact name)
> >
>
> Some platforms need early initialization; this is seen on ones for which the
> bootloader does not initialize the "CPUXGPT" timer, which is used as the ARM
> arch timer. (No, on those platforms you can't upgrade the bootloader, as it's
> signed with a OEM key which is not obtainable, and signature verified earlier
> in the bootchain).
>

Is this arm32 or arm64 platform? Do you mean that these platforms don't have
working architected timers ?

Quick grep suggests the below list of platforms/SoC:

 | arch/arm64/boot/dts/mediatek/mt6795.dtsi
 | arch/arm64/boot/dts/mediatek/mt8173.dtsi
 | arch/arm64/boot/dts/mediatek/mt8183.dtsi
 | arch/arm64/boot/dts/mediatek/mt8186.dtsi
 | arch/arm64/boot/dts/mediatek/mt8192.dtsi
 | arch/arm64/boot/dts/mediatek/mt8195.dtsi
 | arch/arm64/boot/dts/mediatek/mt8516.dtsi
 | arch/arm/boot/dts/mt7623.dtsi
 | arch/arm/boot/dts/mt7629.dtsi
 | arch/arm/boot/dts/mt8127.dtsi
 | arch/arm/boot/dts/mt8135.dtsi

All the above has architected timers and can have the other timer initialised
at module_initcall level.

 | arch/arm/boot/dts/mt2701.dtsi
 | arch/arm/boot/dts/mt6580.dtsi
 | arch/arm/boot/dts/mt6582.dtsi
 | arch/arm/boot/dts/mt6589.dtsi
 | arch/arm/boot/dts/mt6592.dtsi

The above ones have cortex-a7 but still don't have architected timers listed
in the DT. Anyways all use "mediatek,mt6577-timer", so except that other
2 can be dropped from the else and force them to be initialised later.

> As a matter of fact (and somehow obvious), on those platforms (for example,
> MT6795.. but many other as well, really), you *need* this driver to be
> built-in and, well, initialize the CPUX timer as early as possible :-)
>

Built-in is not a problem, you can still remove TIMER_OF_DECLARE as
the initialisation happens later at module_initcall level.
  

Il 15/02/23 14:18, Sudeep Holla ha scritto:
> On Wed, Feb 15, 2023 at 01:43:19PM +0100, AngeloGioacchino Del Regno wrote:
>> Il 14/02/23 23:20, Sudeep Holla ha scritto:
>>> On Tue, Feb 14, 2023 at 06:53:14PM +0800, walter.chang@mediatek.com wrote:
>>>> From: Chun-Hung Wu <chun-hung.wu@mediatek.com>
>>>>
>>>> Make the timer-mediatek driver which can register
>>>> an always-on timer as tick_broadcast_device on
>>>> MediaTek SoCs become loadable module in GKI.
>>>>
>>>> Signed-off-by: Chun-Hung Wu <chun-hung.wu@mediatek.com>
>>>> ---
>>>>    drivers/clocksource/Kconfig          |  2 +-
>>>>    drivers/clocksource/timer-mediatek.c | 43 ++++++++++++++++++++++++++++
>>>>    2 files changed, 44 insertions(+), 1 deletion(-)
>>>
>>> [...]
>>>
>>>> diff --git a/drivers/clocksource/timer-mediatek.c b/drivers/clocksource/timer-mediatek.c
>>>> index d5b29fd03ca2..3358758ea694 100644
>>>> --- a/drivers/clocksource/timer-mediatek.c
>>>> +++ b/drivers/clocksource/timer-mediatek.c
>>>
>>> [...]
>>>
>>>> +static const struct of_device_id mtk_timer_match_table[] = {
>>>> +	{
>>>> +		.compatible = "mediatek,mt6577-timer",
>>>> +		.data = mtk_gpt_init,
>>>> +	},
>>>> +	{
>>>> +		.compatible = "mediatek,mt6765-timer",
>>>> +		.data = mtk_syst_init,
>>>> +	},
>>>> +	{
>>>> +		.compatible = "mediatek,mt6795-systimer",
>>>> +		.data = mtk_cpux_init,
>>>> +	},
>>>> +	{}
>>>> +};
>>>> +
>>>> +static struct platform_driver mtk_timer_driver = {
>>>> +	.probe = mtk_timer_probe,
>>>> +	.driver = {
>>>> +		.name = "mtk-timer",
>>>> +		.of_match_table = mtk_timer_match_table,
>>>> +	},
>>>> +};
>>>> +module_platform_driver(mtk_timer_driver);
>>>> +
>>>> +MODULE_DESCRIPTION("MediaTek Module Timer driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>> +#else
>>>>    TIMER_OF_DECLARE(mtk_mt6577, "mediatek,mt6577-timer", mtk_gpt_init);
>>>>    TIMER_OF_DECLARE(mtk_mt6765, "mediatek,mt6765-timer", mtk_syst_init);
>>>>    TIMER_OF_DECLARE(mtk_mt6795, "mediatek,mt6795-systimer", mtk_cpux_init);
>>>
>>> Why do you need these ? If this driver can work as a module, it can be
>>> built-in module and doesn't need to be initialised early using of_timer_init
>>> (can't recall the exact name)
>>>
>>
>> Some platforms need early initialization; this is seen on ones for which the
>> bootloader does not initialize the "CPUXGPT" timer, which is used as the ARM
>> arch timer. (No, on those platforms you can't upgrade the bootloader, as it's
>> signed with a OEM key which is not obtainable, and signature verified earlier
>> in the bootchain).
>>
> 
> Is this arm32 or arm64 platform? Do you mean that these platforms don't have
> working architected timers ?
> 

Both. I mean that these platforms do have architected timers, but they are stopped
before the bootloader jumps to the kernel, or they are never started at all.

Please refer to:

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/clocksource/timer-mediatek.c?h=next-20230215&id=327e93cf9a59b0d04eb3a31a7fdbf0f11cf13ecb

For a nice explanation.

> Quick grep suggests the below list of platforms/SoC:
> 
>   | arch/arm64/boot/dts/mediatek/mt6795.dtsi
>   | arch/arm64/boot/dts/mediatek/mt8173.dtsi
>   | arch/arm64/boot/dts/mediatek/mt8183.dtsi
>   | arch/arm64/boot/dts/mediatek/mt8186.dtsi
>   | arch/arm64/boot/dts/mediatek/mt8192.dtsi
>   | arch/arm64/boot/dts/mediatek/mt8195.dtsi
>   | arch/arm64/boot/dts/mediatek/mt8516.dtsi
>   | arch/arm/boot/dts/mt7623.dtsi
>   | arch/arm/boot/dts/mt7629.dtsi
>   | arch/arm/boot/dts/mt8127.dtsi
>   | arch/arm/boot/dts/mt8135.dtsi
> 
> All the above has architected timers and can have the other timer initialised
> at module_initcall level.
> 
>   | arch/arm/boot/dts/mt2701.dtsi
>   | arch/arm/boot/dts/mt6580.dtsi
>   | arch/arm/boot/dts/mt6582.dtsi
>   | arch/arm/boot/dts/mt6589.dtsi
>   | arch/arm/boot/dts/mt6592.dtsi
> 
> The above ones have cortex-a7 but still don't have architected timers listed
> in the DT. Anyways all use "mediatek,mt6577-timer", so except that other
> 2 can be dropped from the else and force them to be initialised later.
> 
>> As a matter of fact (and somehow obvious), on those platforms (for example,
>> MT6795.. but many other as well, really), you *need* this driver to be
>> built-in and, well, initialize the CPUX timer as early as possible :-)
>>
> 
> Built-in is not a problem, you can still remove TIMER_OF_DECLARE as
> the initialisation happens later at module_initcall level.
>
  

On Wed, Feb 15, 2023 at 02:30:51PM +0100, AngeloGioacchino Del Regno wrote:
> 
> Both. I mean that these platforms do have architected timers, but they are stopped
> before the bootloader jumps to the kernel, or they are never started at all.
> 
> Please refer to:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/clocksource/timer-mediatek.c?h=next-20230215&id=327e93cf9a59b0d04eb3a31a7fdbf0f11cf13ecb
> 
> For a nice explanation.
> 

Thanks for that. Well then I see no point in making these modules if you
can't have generic Image that boots on all the platform. I now tend to think
that these are made modules just because GKI demands and it *might* work
on one or 2 platforms. One we move this as modules, how will be know the
Image without these timers or with them built as modules will boot or not
on a given mediatek platform. Sorry, I initially saw some point in making
these timers as modules but if they are required for boot on some systems
then I see no point. So if that is the case, NACK for these as it just
creates more confusion after these are merged as why some Images or
even why defconfig image(if we push the config change as well) is not
booting on these platforms.

It is no longer just for system timer useful in low power CPU idle states
as I initial thought.
  

On 15/02/2023 00:20, John Stultz wrote:
> On Tue, Feb 14, 2023 at 3:09 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>> On 14/02/2023 11:53, walter.chang@mediatek.com wrote:
>>> From: Chun-Hung Wu <chun-hung.wu@mediatek.com>
>>>
>>> Make the timer-mediatek driver which can register
>>> an always-on timer as tick_broadcast_device on
>>> MediaTek SoCs become loadable module in GKI.
>>
>> Other questions are unanswered. Please do not ignore feedback and
>> respond to it.
>>
> 
> Apologies, I know it can be a pain to repeat yourself, but would you
> clarify which questions were unanswered?
> My initial skim made it seem like the items you raised were addressed
> in some form (though maybe not sufficiently?).

Questions were:

1. IOW, does the system boot fine? What's the impact of this being a module?

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

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


All these emails or comments were simply ignored.


Best regards,
Krzysztof
  

On Wed, Feb 15, 2023 at 10:35 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 15/02/2023 00:20, John Stultz wrote:
> > On Tue, Feb 14, 2023 at 3:09 AM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >> On 14/02/2023 11:53, walter.chang@mediatek.com wrote:
> >>> From: Chun-Hung Wu <chun-hung.wu@mediatek.com>
> >>>
> >>> Make the timer-mediatek driver which can register
> >>> an always-on timer as tick_broadcast_device on
> >>> MediaTek SoCs become loadable module in GKI.
> >>
> >> Other questions are unanswered. Please do not ignore feedback and
> >> respond to it.
> >>
> >
> > Apologies, I know it can be a pain to repeat yourself, but would you
> > clarify which questions were unanswered?
> > My initial skim made it seem like the items you raised were addressed
> > in some form (though maybe not sufficiently?).
>
> Questions were:
>
> 1. IOW, does the system boot fine? What's the impact of this being a module?

I believe this was answered in the cover letter.
" If the system does not load this module at startup, system will also
boot normally by using built-in architecture timer (in this case is
Arm Generic Timer) instead."

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

Maybe it would be helpful to be more specific in the criteria you're
looking for in such a study?

At least with the GKI, there is a need to support a wide array of
hardware, while minimizing the memory overhead of unnecessary code on
each device. As I mentioned in an earlier reply, this is kernel wide,
so while moving a single driver out to being a module isn't going to
be very substantial, the cumulative effect of not having to build in
every driver needed adds up.

So I think asking to see how much the kernel size changes by
modularizing this one initial driver is a reasonable request, though
I'd not expect it to be a huge gain on its own, but a reduction is a
reduction, and I'm not sure there are many clear downsides.

Again, it's not expected that every driver can be moved to a module,
as there are a number of cases where we need the functionality to be
present early in boot, and that's fine.

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

Modules may be permanent and not unloadable (this patch doesn't
provide a remove hook). While unloadable modules are abstractly nicer,
for supporting a wide array of hardware with minimal memory impact,
permanent modules are equally beneficial (only load them on hardware
that needs them, from that point there is no need to unload them). So
I'm not sure there's much practical value in unloading them.

As for testing the loading side, that sounds fair, though I'd expect
that to be done as part of developing the patch.

So maybe Walter can confirm the device works appropriately across many
boot ups where the module is loaded in their testing? If there is a
specific test criteria you would like to see, please clarify.

And, many thanks for re-outlining your concerns here! I appreciate it!

thanks
-john
  

On Wed, Feb 15, 2023 at 6:46 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Wed, Feb 15, 2023 at 02:30:51PM +0100, AngeloGioacchino Del Regno wrote:
> >
> > Both. I mean that these platforms do have architected timers, but they are stopped
> > before the bootloader jumps to the kernel, or they are never started at all.
> >
> > Please refer to:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/clocksource/timer-mediatek.c?h=next-20230215&id=327e93cf9a59b0d04eb3a31a7fdbf0f11cf13ecb
> >
> > For a nice explanation.
> >
>
> Thanks for that. Well then I see no point in making these modules if you
> can't have generic Image that boots on all the platform. I now tend to think
> that these are made modules just because GKI demands and it *might* work
> on one or 2 platforms. One we move this as modules, how will be know the
> Image without these timers or with them built as modules will boot or not
> on a given mediatek platform. Sorry, I initially saw some point in making
> these timers as modules but if they are required for boot on some systems
> then I see no point. So if that is the case, NACK for these as it just
> creates more confusion after these are merged as why some Images or
> even why defconfig image(if we push the config change as well) is not
> booting on these platforms.

Indeed, if some hardware does have a hard requirement then the
timer-mediatek driver probably isn't a good candidate for being a
module.

Though I wonder if it would make sense to "virtually" split the driver
in two? Have one config for hardware where it is safe to be modular,
and another for the problematic hardware that forces it to be built
in?
That way we can support the safe approach (ends up built in if both
options are selected), but for GKI devices where the hardware isn't
problematic we can still leave it as a module?

thanks
-john
  

Il 15/02/23 15:46, Sudeep Holla ha scritto:
> On Wed, Feb 15, 2023 at 02:30:51PM +0100, AngeloGioacchino Del Regno wrote:
>>
>> Both. I mean that these platforms do have architected timers, but they are stopped
>> before the bootloader jumps to the kernel, or they are never started at all.
>>
>> Please refer to:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/clocksource/timer-mediatek.c?h=next-20230215&id=327e93cf9a59b0d04eb3a31a7fdbf0f11cf13ecb
>>
>> For a nice explanation.
>>
> 
> Thanks for that. Well then I see no point in making these modules if you
> can't have generic Image that boots on all the platform. I now tend to think
> that these are made modules just because GKI demands and it *might* work
> on one or 2 platforms. One we move this as modules, how will be know the
> Image without these timers or with them built as modules will boot or not
> on a given mediatek platform. Sorry, I initially saw some point in making
> these timers as modules but if they are required for boot on some systems
> then I see no point. So if that is the case, NACK for these as it just
> creates more confusion after these are merged as why some Images or
> even why defconfig image(if we push the config change as well) is not
> booting on these platforms.
> 
> It is no longer just for system timer useful in low power CPU idle states
> as I initial thought.
> 

I think that there is still a point in modularization for this driver and I
can propose a rather simple solution, even though this may add some, rather
little, code duplication to the mix.

The platforms that I've described (like mt6795) need the system timer to be
initialized as early as possible - that's true - but that timer is always
"CPUXGPT".

On those platforms, you *still* have multiple timers:
  - CPUX (short for cpuxgpt), used only as system timer;
  - SYST, as another system timer implementation (additional timers) but
    those are always initialized (AFAIK) from the bootloader before booting;
  - GPT (General Purpose Timer).

On one SoC, you may have:
  - CPUX *and* SYST
  - CPUX *and* GPT
  - CPUX *and* SYST *and* GPT

... where the only one that is boot critical and needs to be initialized early
is always only CPUX.

Hence this proposal: to still allow modularization of timers on MediaTek platforms,
we could eventually split the CPUX as a separated driver that *cannot be*, due to
the previously explained constraints, compiled as module, hence always built-in,
from a timer-mediatek driver that could be a module and capable of handling only
SYST and GPT timers.

In that case, we'd hence have...
  - timer-mediatek-cpux.o (bool)
  - timer-mediatek.c (tristate)

Counting that the CPUX timers are actually even using different `tick_resume`
and `set_state_shutdown` callbacks (doing only a IRQ clear/restore and nothing
else), the amount of duplication would be .. well, again, minimal, but then
this means that timer-mediatek-cpux would be `default y if ARCH_MEDIATEK`, or
even selected by ARCH_MEDIATEK itself.

If you think that this could be a good solution, I can send a "fast" patch to
split it out, preparing the ground for the people doing this module work.

Any considerations?

Regards,
Angelo
  

On 16/02/2023 11:22, AngeloGioacchino Del Regno wrote:
> Il 15/02/23 15:46, Sudeep Holla ha scritto:
>> On Wed, Feb 15, 2023 at 02:30:51PM +0100, AngeloGioacchino Del Regno wrote:
>>>
>>> Both. I mean that these platforms do have architected timers, but they are 
>>> stopped
>>> before the bootloader jumps to the kernel, or they are never started at all.
>>>
>>> Please refer to:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/clocksource/timer-mediatek.c?h=next-20230215&id=327e93cf9a59b0d04eb3a31a7fdbf0f11cf13ecb
>>>
>>> For a nice explanation.
>>>
>>
>> Thanks for that. Well then I see no point in making these modules if you
>> can't have generic Image that boots on all the platform. I now tend to think
>> that these are made modules just because GKI demands and it *might* work
>> on one or 2 platforms. One we move this as modules, how will be know the
>> Image without these timers or with them built as modules will boot or not
>> on a given mediatek platform. Sorry, I initially saw some point in making
>> these timers as modules but if they are required for boot on some systems
>> then I see no point. So if that is the case, NACK for these as it just
>> creates more confusion after these are merged as why some Images or
>> even why defconfig image(if we push the config change as well) is not
>> booting on these platforms.
>>
>> It is no longer just for system timer useful in low power CPU idle states
>> as I initial thought.
>>
> 
> I think that there is still a point in modularization for this driver and I
> can propose a rather simple solution, even though this may add some, rather
> little, code duplication to the mix.
> 
> The platforms that I've described (like mt6795) need the system timer to be
> initialized as early as possible - that's true - but that timer is always
> "CPUXGPT".
> 
> On those platforms, you *still* have multiple timers:
>   - CPUX (short for cpuxgpt), used only as system timer;
>   - SYST, as another system timer implementation (additional timers) but
>     those are always initialized (AFAIK) from the bootloader before booting;
>   - GPT (General Purpose Timer).
> 
> On one SoC, you may have:
>   - CPUX *and* SYST
>   - CPUX *and* GPT
>   - CPUX *and* SYST *and* GPT
> 
> ... where the only one that is boot critical and needs to be initialized early
> is always only CPUX.
> 
> Hence this proposal: to still allow modularization of timers on MediaTek platforms,
> we could eventually split the CPUX as a separated driver that *cannot be*, due to
> the previously explained constraints, compiled as module, hence always built-in,
> from a timer-mediatek driver that could be a module and capable of handling only
> SYST and GPT timers.
> 
> In that case, we'd hence have...
>   - timer-mediatek-cpux.o (bool)
>   - timer-mediatek.c (tristate)
> 
> Counting that the CPUX timers are actually even using different `tick_resume`
> and `set_state_shutdown` callbacks (doing only a IRQ clear/restore and nothing
> else), the amount of duplication would be .. well, again, minimal, but then
> this means that timer-mediatek-cpux would be `default y if ARCH_MEDIATEK`, or
> even selected by ARCH_MEDIATEK itself.
> 
> If you think that this could be a good solution, I can send a "fast" patch to
> split it out, preparing the ground for the people doing this module work.
> 
> Any considerations?
> 

I think your proposal sounds acceptable, but we would need to make sure that all 
SoCs can boot with the CPUX driver. I'm aware of some armv7 SoCs that use a kind 
of hack to enable the architecture timer [1]. This, for one part, should be 
moved to CPUX, if possible. For the other part it makes me wonder if really all 
supported MediaTek platforms will boot with SYST/GPT being a module. I think we 
will need some effort from the community to test that.

So as a resume, yes I think your approach is feasible but we should collect 
tested-by tags before merging it.

Regards,
Matthias


[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/mach-mediatek/mediatek.c?h=v6.2-rc8#n16
  

Il 16/02/23 12:23, Matthias Brugger ha scritto:
> 
> 
> On 16/02/2023 11:22, AngeloGioacchino Del Regno wrote:
>> Il 15/02/23 15:46, Sudeep Holla ha scritto:
>>> On Wed, Feb 15, 2023 at 02:30:51PM +0100, AngeloGioacchino Del Regno wrote:
>>>>
>>>> Both. I mean that these platforms do have architected timers, but they are stopped
>>>> before the bootloader jumps to the kernel, or they are never started at all.
>>>>
>>>> Please refer to:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/clocksource/timer-mediatek.c?h=next-20230215&id=327e93cf9a59b0d04eb3a31a7fdbf0f11cf13ecb
>>>>
>>>> For a nice explanation.
>>>>
>>>
>>> Thanks for that. Well then I see no point in making these modules if you
>>> can't have generic Image that boots on all the platform. I now tend to think
>>> that these are made modules just because GKI demands and it *might* work
>>> on one or 2 platforms. One we move this as modules, how will be know the
>>> Image without these timers or with them built as modules will boot or not
>>> on a given mediatek platform. Sorry, I initially saw some point in making
>>> these timers as modules but if they are required for boot on some systems
>>> then I see no point. So if that is the case, NACK for these as it just
>>> creates more confusion after these are merged as why some Images or
>>> even why defconfig image(if we push the config change as well) is not
>>> booting on these platforms.
>>>
>>> It is no longer just for system timer useful in low power CPU idle states
>>> as I initial thought.
>>>
>>
>> I think that there is still a point in modularization for this driver and I
>> can propose a rather simple solution, even though this may add some, rather
>> little, code duplication to the mix.
>>
>> The platforms that I've described (like mt6795) need the system timer to be
>> initialized as early as possible - that's true - but that timer is always
>> "CPUXGPT".
>>
>> On those platforms, you *still* have multiple timers:
>>   - CPUX (short for cpuxgpt), used only as system timer;
>>   - SYST, as another system timer implementation (additional timers) but
>>     those are always initialized (AFAIK) from the bootloader before booting;
>>   - GPT (General Purpose Timer).
>>
>> On one SoC, you may have:
>>   - CPUX *and* SYST
>>   - CPUX *and* GPT
>>   - CPUX *and* SYST *and* GPT
>>
>> ... where the only one that is boot critical and needs to be initialized early
>> is always only CPUX.
>>
>> Hence this proposal: to still allow modularization of timers on MediaTek platforms,
>> we could eventually split the CPUX as a separated driver that *cannot be*, due to
>> the previously explained constraints, compiled as module, hence always built-in,
>> from a timer-mediatek driver that could be a module and capable of handling only
>> SYST and GPT timers.
>>
>> In that case, we'd hence have...
>>   - timer-mediatek-cpux.o (bool)
>>   - timer-mediatek.c (tristate)
>>
>> Counting that the CPUX timers are actually even using different `tick_resume`
>> and `set_state_shutdown` callbacks (doing only a IRQ clear/restore and nothing
>> else), the amount of duplication would be .. well, again, minimal, but then
>> this means that timer-mediatek-cpux would be `default y if ARCH_MEDIATEK`, or
>> even selected by ARCH_MEDIATEK itself.
>>
>> If you think that this could be a good solution, I can send a "fast" patch to
>> split it out, preparing the ground for the people doing this module work.
>>
>> Any considerations?
>>
> 
> I think your proposal sounds acceptable, but we would need to make sure that all 
> SoCs can boot with the CPUX driver. I'm aware of some armv7 SoCs that use a kind of 
> hack to enable the architecture timer [1]. This, for one part, should be moved to 
> CPUX, if possible. For the other part it makes me wonder if really all supported 
> MediaTek platforms will boot with SYST/GPT being a module. I think we will need 
> some effort from the community to test that.
> 
> So as a resume, yes I think your approach is feasible but we should collect 
> tested-by tags before merging it.
> 
> Regards,
> Matthias
> 
> 
> [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/mach-mediatek/mediatek.c?h=v6.2-rc8#n16

Right. I completely forgot about those platforms, even though my intention was
to actually try and migrate them once the CPUX was picked. My bad.

Well, I think that this module conversion will take quite a while, so there
should be no need to rush... I'll send a series later with the split *and* a
conversion of those platforms, so that we will see a removal of that
mediatek_timer_init() function.

Some encouraging words ahead: I'm totally confident that the conversion will
Just Work, because the MT6795 SoC had the same handling for CPUXGPT as the
older MT6589/7623/8127/8135... as that SoC had two implementations initially,
one as ARM, one as ARM64.

Whatever - let's see what I can come up with, then.

Cheers,
Angelo
  

On Thu, 2023-02-16 at 12:36 +0100, AngeloGioacchino Del Regno wrote:
> Il 16/02/23 12:23, Matthias Brugger ha scritto:
> > 
> > 
> > On 16/02/2023 11:22, AngeloGioacchino Del Regno wrote:
> > > Il 15/02/23 15:46, Sudeep Holla ha scritto:
> > > > On Wed, Feb 15, 2023 at 02:30:51PM +0100, AngeloGioacchino Del
> > > > Regno wrote:
> > > > > 
> > > > > Both. I mean that these platforms do have architected timers,
> > > > > but they are stopped
> > > > > before the bootloader jumps to the kernel, or they are never
> > > > > started at all.
> > > > > 
> > > > > Please refer to:
> > > > > 
> > > > > 
https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/clocksource/timer-mediatek.c?h=next-20230215&id=327e93cf9a59b0d04eb3a31a7fdbf0f11cf13ecb__;!!CTRNKA9wMg0ARbw!jOX04TQn1HXcKOdzAiK3ZlqsSE3j3p6Zo-Cajr0khXC9ANlbkl8kQrUgx6wadR8b_46o9J0SbJgjhoS03rQ7somJfA5Z9L6G_g$ 
> > > > >  
> > > > > 
> > > > > For a nice explanation.
> > > > > 
> > > > 
> > > > Thanks for that. Well then I see no point in making these
> > > > modules if you
> > > > can't have generic Image that boots on all the platform. I now
> > > > tend to think
> > > > that these are made modules just because GKI demands and it
> > > > *might* work
> > > > on one or 2 platforms. One we move this as modules, how will be
> > > > know the
> > > > Image without these timers or with them built as modules will
> > > > boot or not
> > > > on a given mediatek platform. Sorry, I initially saw some point
> > > > in making
> > > > these timers as modules but if they are required for boot on
> > > > some systems
> > > > then I see no point. So if that is the case, NACK for these as
> > > > it just
> > > > creates more confusion after these are merged as why some
> > > > Images or
> > > > even why defconfig image(if we push the config change as well)
> > > > is not
> > > > booting on these platforms.
> > > > 
> > > > It is no longer just for system timer useful in low power CPU
> > > > idle states
> > > > as I initial thought.
> > > > 
> > > 
> > > I think that there is still a point in modularization for this
> > > driver and I
> > > can propose a rather simple solution, even though this may add
> > > some, rather
> > > little, code duplication to the mix.
> > > 
> > > The platforms that I've described (like mt6795) need the system
> > > timer to be
> > > initialized as early as possible - that's true - but that timer
> > > is always
> > > "CPUXGPT".
> > > 
> > > On those platforms, you *still* have multiple timers:
> > >   - CPUX (short for cpuxgpt), used only as system timer;
> > >   - SYST, as another system timer implementation (additional
> > > timers) but
> > >     those are always initialized (AFAIK) from the bootloader
> > > before booting;
> > >   - GPT (General Purpose Timer).
> > > 
> > > On one SoC, you may have:
> > >   - CPUX *and* SYST
> > >   - CPUX *and* GPT
> > >   - CPUX *and* SYST *and* GPT
> > > 
> > > ... where the only one that is boot critical and needs to be
> > > initialized early
> > > is always only CPUX.
> > > 
> > > Hence this proposal: to still allow modularization of timers on
> > > MediaTek platforms,
> > > we could eventually split the CPUX as a separated driver that
> > > *cannot be*, due to
> > > the previously explained constraints, compiled as module, hence
> > > always built-in,
> > > from a timer-mediatek driver that could be a module and capable
> > > of handling only
> > > SYST and GPT timers.
> > > 
> > > In that case, we'd hence have...
> > >   - timer-mediatek-cpux.o (bool)
> > >   - timer-mediatek.c (tristate)
> > > 
> > > Counting that the CPUX timers are actually even using different
> > > `tick_resume`
> > > and `set_state_shutdown` callbacks (doing only a IRQ
> > > clear/restore and nothing
> > > else), the amount of duplication would be .. well, again,
> > > minimal, but then
> > > this means that timer-mediatek-cpux would be `default y if
> > > ARCH_MEDIATEK`, or
> > > even selected by ARCH_MEDIATEK itself.
> > > 
> > > If you think that this could be a good solution, I can send a
> > > "fast" patch to
> > > split it out, preparing the ground for the people doing this
> > > module work.
> > > 
> > > Any considerations?
> > > 
> > 
> > I think your proposal sounds acceptable, but we would need to make
> > sure that all 
> > SoCs can boot with the CPUX driver. I'm aware of some armv7 SoCs
> > that use a kind of 
> > hack to enable the architecture timer [1]. This, for one part,
> > should be moved to 
> > CPUX, if possible. For the other part it makes me wonder if really
> > all supported 
> > MediaTek platforms will boot with SYST/GPT being a module. I think
> > we will need 
> > some effort from the community to test that.
> > 
> > So as a resume, yes I think your approach is feasible but we should
> > collect 
> > tested-by tags before merging it.
> > 
> > Regards,
> > Matthias
> > 
> > 
> > [1] 
> > 
https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/mach-mediatek/mediatek.c?h=v6.2-rc8*n16__;Iw!!CTRNKA9wMg0ARbw!jOX04TQn1HXcKOdzAiK3ZlqsSE3j3p6Zo-Cajr0khXC9ANlbkl8kQrUgx6wadR8b_46o9J0SbJgjhoS03rQ7somJfA5mnCQ6Fw$ 
> >  
> 
> Right. I completely forgot about those platforms, even though my
> intention was
> to actually try and migrate them once the CPUX was picked. My bad.
> 
> Well, I think that this module conversion will take quite a while, so
> there
> should be no need to rush... I'll send a series later with the split
> *and* a
> conversion of those platforms, so that we will see a removal of that
> mediatek_timer_init() function.
> 
> Some encouraging words ahead: I'm totally confident that the
> conversion will
> Just Work, because the MT6795 SoC had the same handling for CPUXGPT
> as the
> older MT6589/7623/8127/8135... as that SoC had two implementations
> initially,
> one as ARM, one as ARM64.
> 
> Whatever - let's see what I can come up with, then.
> 
> Cheers,
> Angelo

First, I'd like to thank Angelo for assisting us in spliting out the
CPUX driver [1], allowing this driver to become a module that can be
loaded when needed.

In response to Matthias's concern about whether SYST/GPT on the old
MediaTek platforms can boot properly with the loadable module, I
recently conducted a few tests and found that both hardwares are
capable of booting normally with this loadable module.

For my experiments, I used MT6768 with this patch:

1. When the module was not loaded or load the module but the
corresponding compatible property was not specified in the dts, the
following results were obtained:

# cat /proc/timer_list | grep "Broadcast device" -A 15
Broadcast device
Clock Event Device: bc_hrtimer
 max_delta_ns:   9223372036854775807
 min_delta_ns:   1
 mult:           1
 shift:          0
 mode:           3
 next_event:     490204000000 nsecs
 set_next_event: <0000000000000000>
 shutdown: bc_shutdown.cfi_jt
 event_handler:  tick_handle_oneshot_broadcast.cfi_jt
 retries:        0

tick_broadcast_mask: ff
tick_broadcast_oneshot_mask: 7e

The built-in `bc_hrtimer` was used as the broadcast device, whereby one
CPU was kept awake to wake up the other CPUs. As a result, one CPU
could not enter the idle state.

2. When the module was loaded and the GPT compatible property was
specified in the dts:

# cat /proc/timer_list | grep "Broadcast device" -A 17
Broadcast device
Clock Event Device: mtk-clkevt
 max_delta_ns:   330382104634
 min_delta_ns:   1000
 mult:           27917287
 shift:          31
 mode:           3
 next_event:     1483221016462 nsecs
 set_next_event: mtk_gpt_clkevt_next_event.cfi_jt
 shutdown: mtk_gpt_clkevt_shutdown.cfi_jt
 periodic: mtk_gpt_clkevt_set_periodic.cfi_jt
 oneshot:  mtk_gpt_clkevt_shutdown.cfi_jt
 resume:   mtk_gpt_clkevt_shutdown.cfi_jt
 event_handler:  tick_handle_oneshot_broadcast.cfi_jt
 retries:        17

tick_broadcast_mask: ff
tick_broadcast_oneshot_mask: bf

3. When the module was loaded and the SYST compatible property was
specified in the dts:

# cat /proc/timer_list | grep "Broadcast device" -A 17
Broadcast device
Clock Event Device: mtk-clkevt
 max_delta_ns:   330382104634
 min_delta_ns:   1000
 mult:           27917287
 shift:          31
 mode:           3
 next_event:     132252000000 nsecs
 set_next_event: mtk_syst_clkevt_next_event.cfi_jt
 shutdown: mtk_syst_clkevt_shutdown.cfi_jt
 oneshot:  mtk_syst_clkevt_oneshot.cfi_jt
 resume:   mtk_syst_clkevt_resume.cfi_jt
 event_handler:  tick_handle_oneshot_broadcast.cfi_jt
 retries:        8

tick_broadcast_mask: ff
tick_broadcast_oneshot_mask: 3f

These two experiments show that SYST/GPT on the old MediaTek platforms
can also work properly with the loadable module, and will not cause any
booting issues. Therefore, making timer-mediatek.c driver into a
loadable module is feasible.

Thanks,
Walter Chang

[1] 
https://lore.kernel.org/lkml/20230309103913.116775-1-angelogioacchino.delregno@collabora.com/
  

On Wed, 2023-03-29 at 14:22 +0800, Walter.Chang wrote:
> First, I'd like to thank Angelo for assisting us in spliting out the
> CPUX driver [1], allowing this driver to become a module that can be
> loaded when needed.
> 
> In response to Matthias's concern about whether SYST/GPT on the old
> MediaTek platforms can boot properly with the loadable module, I
> recently conducted a few tests and found that both hardwares are
> capable of booting normally with this loadable module.
> 
> For my experiments, I used MT6768 with this patch:
> 
> 1. When the module was not loaded or load the module but the
> corresponding compatible property was not specified in the dts, the
> following results were obtained:
> 
> # cat /proc/timer_list | grep "Broadcast device" -A 15
> Broadcast device
> Clock Event Device: bc_hrtimer
>  max_delta_ns:   9223372036854775807
>  min_delta_ns:   1
>  mult:           1
>  shift:          0
>  mode:           3
>  next_event:     490204000000 nsecs
>  set_next_event: <0000000000000000>
>  shutdown: bc_shutdown.cfi_jt
>  event_handler:  tick_handle_oneshot_broadcast.cfi_jt
>  retries:        0
> 
> tick_broadcast_mask: ff
> tick_broadcast_oneshot_mask: 7e
> 
> The built-in `bc_hrtimer` was used as the broadcast device, whereby
> one
> CPU was kept awake to wake up the other CPUs. As a result, one CPU
> could not enter the idle state.
> 
> 2. When the module was loaded and the GPT compatible property was
> specified in the dts:
> 
> # cat /proc/timer_list | grep "Broadcast device" -A 17
> Broadcast device
> Clock Event Device: mtk-clkevt
>  max_delta_ns:   330382104634
>  min_delta_ns:   1000
>  mult:           27917287
>  shift:          31
>  mode:           3
>  next_event:     1483221016462 nsecs
>  set_next_event: mtk_gpt_clkevt_next_event.cfi_jt
>  shutdown: mtk_gpt_clkevt_shutdown.cfi_jt
>  periodic: mtk_gpt_clkevt_set_periodic.cfi_jt
>  oneshot:  mtk_gpt_clkevt_shutdown.cfi_jt
>  resume:   mtk_gpt_clkevt_shutdown.cfi_jt
>  event_handler:  tick_handle_oneshot_broadcast.cfi_jt
>  retries:        17
> 
> tick_broadcast_mask: ff
> tick_broadcast_oneshot_mask: bf
> 
> 3. When the module was loaded and the SYST compatible property was
> specified in the dts:
> 
> # cat /proc/timer_list | grep "Broadcast device" -A 17
> Broadcast device
> Clock Event Device: mtk-clkevt
>  max_delta_ns:   330382104634
>  min_delta_ns:   1000
>  mult:           27917287
>  shift:          31
>  mode:           3
>  next_event:     132252000000 nsecs
>  set_next_event: mtk_syst_clkevt_next_event.cfi_jt
>  shutdown: mtk_syst_clkevt_shutdown.cfi_jt
>  oneshot:  mtk_syst_clkevt_oneshot.cfi_jt
>  resume:   mtk_syst_clkevt_resume.cfi_jt
>  event_handler:  tick_handle_oneshot_broadcast.cfi_jt
>  retries:        8
> 
> tick_broadcast_mask: ff
> tick_broadcast_oneshot_mask: 3f
> 
> These two experiments show that SYST/GPT on the old MediaTek
> platforms
> can also work properly with the loadable module, and will not cause
> any
> booting issues. Therefore, making timer-mediatek.c driver into a
> loadable module is feasible.
> 
> Thanks,
> Walter Chang
> 
> [1] 
> 
https://lore.kernel.org/lkml/20230309103913.116775-1-angelogioacchino.delregno@collabora.com/

Gentle ping.

Thanks,
Walter Chang