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

[Youngmin Nam]

On Wed, Oct 27, 2021 at 08:39:47AM +0200, Krzysztof Kozlowski wrote:
> On 27/10/2021 03:38, Youngmin Nam wrote:
> > On Tue, Oct 26, 2021 at 01:00:51PM +0200, Krzysztof Kozlowski wrote:
> >> On 26/10/2021 12:45, Youngmin Nam wrote:
> >>> On Tue, Oct 26, 2021 at 09:10:28AM +0200, Krzysztof Kozlowski wrote:
> >>>> On 26/10/2021 03:47, Youngmin Nam wrote:
> >>>>>> If everyone added a new driver to avoid integrating with existing code,
> >>>>>> we would have huge kernel with thousands of duplicated solutions. The
> >>>>>> kernel also would be unmaintained.
> >>>>>>
> >>>>>> Such arguments were brought before several times - "I don't want to
> >>>>>> integrating with existing code", "My use case is different", "I would
> >>>>>> need to test the other cases", "It's complicated for me".
> >>>>>>
> >>>>>> Instead of pushing a new vendor driver you should integrate it with
> >>>>>> existing code.
> >>>>>>
> >>>>> Let me ask you one question.
> >>>>> If we maintain as one driver, how can people who don't have the new MCT test the new driver?
> >>>>
> >>>> I assume you talk about a case when someone else later changes something
> >>>> in the driver. Such person doesn't necessarily have to test it. The same
> >>>> as in all other cases (Exynos MCT is not special here): just ask for
> >>>> testing on platform one doesn't have.
> >>>>
> >>>> Even if you submit this as separate driver, there is the exact same
> >>>> problem. People will change the MCTv2 driver without access to hardware.
> >>>>
> >>> Yes, I can test the new MCT driver if someone ask for testing after modifying the new driver.
> >>> But in this case, we don't need to test the previous MCT driver. We have only to test the new MCT driver.
> >>
> >> Like with everything in Linux kernel. We merge instead of duplicate.
> >> It's not an argument.
> >>
> >>>> None of these differ for Exynos MCT from other drivers, e.g. mentioned
> >>>> Samsung PMIC drivers, recently modified (by Will and Sam) the SoC clock
> >>>> drivers or the ChipID drivers (changed by Chanho).
> >>> From HW point of view, the previous MCT is almost 10-year-old IP without any major change and
> >>> it will not be used on next new Exynos SoC.
> >>> MCTv2 is the totally newly designed IP and it will replace the Exynos system timer.
> >>> Device driver would be dependent with H/W. We are going to apply a lot of changes for this new MCT.
> >>> For maintenance, I think we should separate the new MCT driver for maintenance.
> >>>
> >>
> >> There are several similarities which actually suggest that you
> >> exaggerate the differences.
> >>
> >> The number of interrupts is the same (4+8 in older one, 12 in new one...).
> > 
> > I didn't "exaggerate" at all.
> > The numer of interrups is the same. But their usage is completely different.
> > The type of each timer is different.
> > And previous MCT can only support upto 8 cores.
> > 
> > * MCTv1 (Let me call previous MCT as MCTv1)
> >  - 4 global timer + 8 local timer
> >  - Global timer and local timer are totally different.
> >  - 4 global timer have only one 64bit FRC that serves as the "up-counter" with 4 "comparators"
> >  - 8 local timer have 8 of 32bit FRC that serves as the "down-counter" without any "comparators".(just expire timer)
> >  - local timer can be used as per-cpu event timer, so it can only support upto 8 cores.
> > 
> > * MCTv2
> >  - There are no global timer and local timer anymore.
> >  - 1 of 64bit FRC that serves as "up-counter" (just counter without "comparators")
> >  - 12 comaprators (These are not "counter") can be used as per-cpu event timer so that it can support upto 12 cores.
> >  - RTC source can be used as backup source.
> > 
> >> You assign the MCT priority also as higher than Architected Timer
> >> (+Cc Will and Mark - is it ok for you?)
> >>     evt->rating = 500;      /* use value higher than ARM arch timer *
> >>
> > Yes, this is absolutely correct on event timer.
> > We cannot use arm arch timer which is operating based on PPI as per-cpu event timer because of poewr mode.
> > We have to use SPI for per-cpu timer interrupt. (This is the same in all Exynos platform)
> 
> No, this is not correct, was explained several times and it is one of
> the reasons why I am holding back to reuse of existing driver. You
> copied few 32-bit ARM (ARMv7) solutions from old MCT driver into a new
> one which is only 64-bit. These 32-bit solutions are some optimizations
> or hacks matching 32-bit ARM Exynos processors. If you copy them to
> entirely new driver for entirely different IP block, it means this is
> not entirely different IP block.
> 
> Therefore I see a point now in having a Exynos MCTv2 driver for new IP
> blocks after removing all that legacy 32-bit ARM stuff.
> 
> Therefore:
> > evt->rating = 350;
> Not 500. Use the same value as old timer driver for ARMv8. See previous
> discussions and commits from Marek and Will.
> 

I've read the whole history from Marek and Will.
As I explained to Will, we need more time to test regarding decreasing the rating of MCTv2.
Decreasing the rating is related only performance not functionality.
So from regression perspective, it's not easy to change the rating on this new driver at this time.

> Other:
> > static u32 exynos_read_count_32(void)
> 
> This is u64, not u32. Get rid of 32-bit optimization/hack or explain it
> similarly as Doug did (40 lines of reasoning).
> 
> >  .mask           = CLOCKSOURCE_MASK(32),
> 
> Mask is 64.
> 
> These are the 32-bit legacies I found now (maybe there are more...).
> Don't copy them if this is a new driver getting rid of legacy.
> 

Thanks for pointing out and sharing "Doug Anderson"'s explanation.
As we use ARM arch timer as a clock source currently, we should get rid of it.

> > 
> >> All these point that block is not different. Again, let me repeat, we
> >> support old Samsung PMICs with new Samsung PMICs in one driver. Even
> >> though the "old one" won't be changed, as you mentioned here. The same
> >> Samsung SoC clock drivers are used for old Exynos and for new ones...
> >> Similarly to pinctrl drivers. The same ChipId.
> >>
> >> Everywhere we follow the same concept of unification instead of
> >> duplication. Maybe Exynos MCT timer is an exception but you did not
> >> provide any arguments supporting this. Why Exynos MCTv2 should be
> >> treated differently than Exynos850 clocks, chipid, pinctrl and other blocks?
> >>
> > 
> > If MCTv2 has only changes in register layout, I can consider merging work.
> > But this is not that case.
> > 
> > You gave a example with PMIC, SoC clock, Pinctrl, ChipId.
> > These H/W IP have only changes in register layout which came from difference of each SoC.
> > 
> > Were these H/W IP version changed?
> > Were these H/W IP control method changed ?
> > No. It only has minor chagnes not major changes.
> > 
> > * PMIC - controls the PMIC reigster with I2C interface regarding their SoC usecase.
> >        - there is no changes on H/W control method itself.
> > 
> > * SoC Clock - changes only in register layout regarding SoC
> >             - Clock control method still the same.
> > 
> > * Pinctrl - changes only in gpio pin register layout (pin number, pin type, pin map..) regarding SoC.
> > 	  - Is there any changes on control method ?
> > 
> > * Chipid - This is very simple H/W IP. It only supports unique chip id value with read-only register.
> >          - It really only have changes in register layout.
> > 
> > MCTv2 is different.
> > Not only register layout but also it's control method has to be changed regarding H/W difference.
> 
> Yes, I see some differences in HW control which we also solve in several
> other cases through structure ops. Indeed here it looks like the number
> of differences in control is bigger than in other cases.
> 
> If Daniel is okay in having two drivers and you get rid of all 32-bit
> legacy (including ordering against architected timers), I am fine with it.
> 

Thanks for understanding.
Let me send patch v2 soon.

> >> Daniel,
> >> Any preferences from you? Integrating MCT into existing driver (thus
> >> growing it) or having a new one?
> 
> 
> 
> Best regards,
> Krzysztof
>