[PATCH v6 0/3] Timer support for ARM Tegra

[Dmitry Osipenko]

27.01.2023 01:00, Dmitry Osipenko пишет:
> 26.01.2023 20:54, Thierry Reding пишет:
>> On Thu, Jan 26, 2023 at 07:08:54PM +0200, Svyatoslav Ryhel wrote:
>>> чт, 26 січ. 2023 р. о 12:35 Thierry Reding <thierry.reding at gmail.com> пише:
>>>>
>>>> On Wed, Jan 25, 2023 at 05:41:08PM +0100, Thierry Reding wrote:
>>>>> On Tue, Jan 24, 2023 at 08:57:48AM +0200, Svyatoslav Ryhel wrote:
>>>>>> - ARM: tegra: remap clock_osc_freq for all Tegra family
>>>>>> Enum clock_osc_freq was designed to use only with T20.
>>>>>> This patch remaps it to use additional frequencies, added in
>>>>>> T30+ SoC while maintaining backwards compatibility with T20.
>>>>>>
>>>>>> - drivers: timer: add timer driver for ARMv7 based Tegra devices
>>>>>> Add timer support for T20/T30/T114 and T124 based devices.
>>>>>> Driver is based on DM, has device tree support and can be
>>>>>> used on SPL and early boot stage.
>>>>>>
>>>>>> - ARM: tegra: include timer as default option
>>>>>> Enable TIMER as default option for all Tegra devices and
>>>>>> enable TEGRA_TIMER for TEGRA_ARMV7_COMMON. Additionally
>>>>>> enable SPL_TIMER if build as SPL part and drop deprecated
>>>>>> configs from common header.
>>>>>>
>>>>>> P. S. I have no arm64 Tegra and according to comment in
>>>>>> tegra-common.h
>>>>>> Use the Tegra US timer on ARMv7, but the architected timer on ARMv8.
>>>>>>
>>>>>> Svyatoslav Ryhel (3):
>>>>>>   ARM: tegra: remap clock_osc_freq for all Tegra family
>>>>>>   drivers: timer: add timer driver for ARMv7 based Tegra devices
>>>>>>   ARM: tegra: include timer as default option
>>>>>
>>>>> This causes a regression on Tegra210 (Jetson TX1). I'm trying to
>>>>> investigate, but it's complicated by the fact that I'm not getting out
>>>>> any debug prints, so I suspect the issue is happening quite early.
>>>>
>>>> Alright, I managed to make this work on Tegra210 using the following
>>>> patch on top of this series:
>>>>
>>>
>>> Hello! Thanks for testing this on T210 SoC.
>>>
>>>> --- >8 ---
>>>> diff --git a/arch/arm/dts/tegra210.dtsi b/arch/arm/dts/tegra210.dtsi
>>>> index a521a43d6cfd..ccb5a927da89 100644
>>>> --- a/arch/arm/dts/tegra210.dtsi
>>>> +++ b/arch/arm/dts/tegra210.dtsi
>>>> @@ -318,7 +318,7 @@
>>>>         };
>>>>
>>>>         timer at 60005000 {
>>>> -               compatible = "nvidia,tegra210-timer", "nvidia,tegra20-timer";
>>>> +               compatible = "nvidia,tegra210-timer", "nvidia,tegra30-timer", "nvidia,tegra20-timer";
>>>
>>> This compatibe should not be needed since the driver will have t210
>>> compatible included.
>>
>> Yes, it should be fine to leave this as-is. I had included this before
>> updating the driver, to get the driver to bind to this. Upstream Linux
>> doesn't include "nvidia,tegra20-timer", so it only has the compatible
>> string for Tegra210. I think that's slightly better because the register
>> interface isn't quite compatible. That's a separate issue and we can do
>> that in a follow-up patch.
>>
>>>
>>>>                 reg = <0x0 0x60005000 0x0 0x400>;
>>>>                 interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>,
>>>>                              <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
>>>> diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig
>>>> index cc3f00e50128..b50eec5b8c9b 100644
>>>> --- a/arch/arm/mach-tegra/Kconfig
>>>> +++ b/arch/arm/mach-tegra/Kconfig
>>>> @@ -136,6 +136,7 @@ config TEGRA210
>>>>         select TEGRA_PINCTRL
>>>>         select TEGRA_PMC
>>>>         select TEGRA_PMC_SECURE
>>>> +       select TEGRA_TIMER
>>>>
>>>>  config TEGRA186
>>>>         bool "Tegra186 family"
>>>> diff --git a/drivers/timer/tegra-timer.c b/drivers/timer/tegra-timer.c
>>>> index d2d163cf3fef..235532ba8926 100644
>>>> --- a/drivers/timer/tegra-timer.c
>>>> +++ b/drivers/timer/tegra-timer.c
>>>> @@ -58,17 +58,26 @@ static notrace u64 tegra_timer_get_count(struct udevice *dev)
>>>>  static int tegra_timer_probe(struct udevice *dev)
>>>>  {
>>>>         struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
>>>> +       enum clock_osc_freq freq;
>>>>         u32 usec_config, value;
>>>>
>>>>         /* Timer rate has to be set unconditionally */
>>>>         uc_priv->clock_rate = TEGRA_TIMER_RATE;
>>>>
>>>> +       /*
>>>> +        * The microsecond timer runs off of clk_m on Tegra210, and clk_m
>>>> +        * runs at half the OSC, so fake this up.
>>>> +        */
>>>> +       freq = clock_get_osc_freq();
>>>> +       if (freq == CLOCK_OSC_FREQ_38_4)
>>>> +               freq = CLOCK_OSC_FREQ_19_2;
>>>> +
>>>
>>> May you confirm that ALL known T210 devices use 19.2MHz as calibration
>>> clock for timer?
>>
>> According to the Tegra210 TRM, the TIMERUS depends on the rate of clk_m
>> and clk_m is derived from OSC and either divided by 1, 2, 3 or 4. The
>> TRM goes on to say that:
>>
>> 	The expectation is that this CLK_M_DIVISOR will only be changed
>> 	once after powering VDD_SOC on in cold boot/LP0 exit path. So
>> 	these sequences are verified with an oscillator clock of 38.4
>> 	MHz; div2 setting of the CLK_M divisor must be used. The result
>> 	is 19.2 MHz clk_m.
>>
>> And then it says:
>>
>> 	Note: Div2 is the recommended clk_m divisor value. Do not use
>> 	any other value.
>>
>> This is from Section 5.1.2 "Clk_m Divisor" of the Tegra210 TRM.
>>
>>> If yes I can set this change in simpler as a separate commit or
>>> including into existing patches.
>>
>> Anything you have in mind? I tried a couple of variations to the above
>> and they all failed because in other places it's important that OSC is
>> recognized as running at 38.4 MHz. If not, then other PLLs will not
>> work properly and even basic things like the debug UART won't work.
>>
>> Technically the right thing to do would be to base the USEC config off
>> the clk_m rate. We didn't do that back at the time, IIRC, because most
>> of the clock infrastructure didn't exist, but it might be possible to
>> achieve this today. I kept the above because it is still a bit simpler
>> and as the TRM suggests nobody should be using anything other than the
>> div2 setting for clk_m. I'm certainly not aware of any devices that do
>> something different. And U-Boot has always had this assumption as well,
>> so I think it's safe to use.
> 
> Am I understanding correctly that for T210 we can/should use
> clk_m_get_rate() instead of clock_get_osc_freq() in tegra_timer_probe()?
> Have you tested this option?

Although, looking at the kernel code, I see that clk_m is the parent
clock for timer on all SoCs. Hence replacing clock_get_osc_freq() with
clk_m_get_rate() should be the proper solution and then no T210-specific
workarounds are needed.