Re: [PATCH v2 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC

[Youngmin Nam]

On Wed, Nov 03, 2021 at 09:18:07AM +0100, Krzysztof Kozlowski wrote:
> On 03/11/2021 01:09, Youngmin Nam wrote:
> > On Tue, Nov 02, 2021 at 10:28:10AM +0000, Mark Rutland wrote:
> >> On Tue, Nov 02, 2021 at 09:11:21AM +0900, Youngmin Nam wrote:
> >>> Exynos MCT version 2 is composed of 1 FRC and 12 comparators.
> >>> There are no global timer and local timer anymore.
> >>> The 1 of 64bit FRC serves as "up-counter"(not "comparators").
> >>> The 12 comaprators(not "counter") can be used as per-cpu event timer
> >>> so that it can support upto 12 cores.
> >>> And a RTC source can be used as backup clock source.
> >>
> >> [...]
> >>
> >>> +static int exynos_mct_starting_cpu(unsigned int cpu)
> >>> +{
> >>> +	struct mct_clock_event_device *mevt = per_cpu_ptr(&percpu_mct_tick, cpu);
> >>> +	struct clock_event_device *evt = &mevt->evt;
> >>> +
> >>> +	snprintf(mevt->name, sizeof(mevt->name), "mct_comp%d", cpu);
> >>> +
> >>> +	evt->name = mevt->name;
> >>> +	evt->cpumask = cpumask_of(cpu);
> >>> +	evt->set_next_event = exynos_comp_set_next_event;
> >>> +	evt->set_state_periodic = mct_set_state_periodic;
> >>> +	evt->set_state_shutdown = mct_set_state_shutdown;
> >>> +	evt->set_state_oneshot = mct_set_state_shutdown;
> >>> +	evt->set_state_oneshot_stopped = mct_set_state_shutdown;
> >>> +	evt->tick_resume = mct_set_state_shutdown;
> >>> +	evt->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
> >>> +	evt->rating = 500;	/* use value higher than ARM arch timer */
> >>
> >> Previously Will asked you to try CLOCK_EVT_FEAT_PERCPU here, and to set
> >> the C3STOP flag on the arch timer via the DT when necessary, rather than
> >> trying to override the arch timer like this:
> >>
> >>   https://protect2.fireeye.com/v1/url?k=72526080-2dc9598b-7253ebcf-002590f5b904-ca603717c6462908&q=1&e=be56aa83-dbac-4639-913d-d388620fe3fc&u=https%3A%2F%2Flore.kernel.org%2Fr%2F20211027073458.GA22231%40willie-the-truck
> >>
> >> There are a bunch of things that depend on the architected timer working
> >> as a clocksource (e.g. vdso, kvm), and it *should* work as a lock
> >> clockevent_device if configured correctly, and it's much more consistent
> >> with *everyone else* to use the arhcitected timer by default.
> >>
> >> Please try as Will suggested above, so that this works from day one.
> >>
> >> Thanks,
> >> Mark.
> >>
> > 
> > Hi Mark.
> > It looks like you missed my previous mail.
> > https://protect2.fireeye.com/v1/url?k=ab15817a-cbf71c27-ab140a35-000babd9f1ba-123b7f313b1b1ccc&q=1&e=34c8716e-6d2e-4d8e-82fe-04777ebc5eb3&u=https%3A%2F%2Flore.kernel.org%2Fall%2F20211029035422.GA30523%40perf%2F%23t
> > 
> > Yes, I believe Will's suggestion definitely will work.
> > But that is for performance not functionality.
> > As a driver for new H/W IP I would like to confirm functionality first.
> > 
> > We need more time to test this feature with our exynos core power down feature.
> > And we need to do a various regression test whether there is another corner case or not.
> > So, how about we apply Will's suggetion later after the current patchset is merged first?
> > After doing our regression test with our exynos core power down feature, we can confirm this.
> > 
> 
> Not really, because once it is merged there is no incentive to fix it or
> simply changing it can be forgotten. Also similarly to commit
> 6282edb72bed ("clocksource/drivers/exynos_mct: Increase priority over
> ARM arch timer"), there should be a valid and serious reason to
> prioritize Exynos MCT.
> 

No, it's not. I also want to decrease MCTv2 timer rating so that we want to use arm arch timer as a default.
But this feature has to be confirmed with core power down feature enabled.
Without core power down feature, we can't comfirm this.
Ater that we need to check whether there is regression or not related power, stability, and so on.
I'm not saying I will not apply Will's suggestion but I just want to apply later after some hard test.

> 
> Best regards,
> Krzysztof
>