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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7419 bytes --]

On Thu, Jan 26, 2023 at 07:12:34PM +0200, Svyatoslav Ryhel wrote:
> I may implement changes of Thierry Reding in a proper form as a
> separate patch or include in existing (depends on his choice).

I think it's ultimately better if this is properly integrated into the
series because the series would remain bisectible.

If the existing series is applied as-is, we have a few patches in the
middle during which Tegra210 is unusable. So if we ever have to track
down a regression that might be problematic.

It's not a big deal since we've rarely had regressions in U-Boot. It's
ultimately up to Tom to decide how he wants to handle this.

If you send out another series, can you please add me on Cc so I don't
miss it?

Thanks,
Thierry

> The only thing I need to know is if  ALL known T210 devices use
> 19.2MHz as calibration clock for timer?
> 
> Best regards.
> Svyatoslav R.
> 
> чт, 26 січ. 2023 р. о 18:50 Tom Warren <TWarren@nvidia.com> пише:
> >
> > Thanks for testing T210/T186, Thierry.
> >
> > I had Svyatoslav's patches ready to go for a PR yesterday, so I'll need either a patch from you for the T210 changes that I can apply on top of his, or I'll need Svyatoslav to adopt your changes as a 4th patch in his series. Once I have that and can pass buildman OK, I'll be ready to send a PR to TomR.
> >
> > Tom
> >
> > -----Original Message-----
> > From: Thierry Reding <thierry.reding@gmail.com>
> > Sent: Thursday, January 26, 2023 4:41 AM
> > To: Svyatoslav Ryhel <clamor95@gmail.com>
> > Cc: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>; Tom Warren <twarren@nvidia.com>; Marek Vasut <marex@denx.de>; Maxim Schwalm <maxim.schwalm@gmail.com>; Dmitry Osipenko <digetx@gmail.com>; Heinrich Schuchardt <xypron.glpk@gmx.de>; Michal Simek <michal.simek@amd.com>; Stefan Roese <sr@denx.de>; Eugen Hristev <eugen.hristev@microchip.com>; Michael Walle <michael@walle.cc>; Simon Glass <sjg@chromium.org>; Jim Liu <jim.t90615@gmail.com>; William Zhang <william.zhang@broadcom.com>; Rick Chen <rick@andestech.com>; Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>; Andre Przywara <andre.przywara@arm.com>; Jaehoon Chung <jh80.chung@samsung.com>; u-boot@lists.denx.de
> > Subject: Re: [PATCH v6 0/3] Timer support for ARM Tegra
> >
> > On Thu, Jan 26, 2023 at 11:34:59AM +0100, Thierry Reding wrote:
> > > 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:
> > >
> > > --- >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@60005000 {
> > > -             compatible = "nvidia,tegra210-timer", "nvidia,tegra20-timer";
> > > +             compatible = "nvidia,tegra210-timer", "nvidia,tegra30-timer",
> > > +"nvidia,tegra20-timer";
> > >               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;
> > > +
> > >       /*
> > >        * Configure microsecond timers to have 1MHz clock
> > >        * Config register is 0xqqww, where qq is "dividend", ww is "divisor"
> > >        * Uses n+1 scheme
> > >        */
> > > -     switch (clock_get_osc_freq()) {
> > > +     switch (freq) {
> > >       case CLOCK_OSC_FREQ_13_0:
> > >               usec_config = 0x000c; /* (12+1)/(0+1) */
> > >               break;
> > > @@ -113,6 +122,7 @@ static const struct udevice_id tegra_timer_ids[] = {
> > >       { .compatible = "nvidia,tegra30-timer" },
> > >       { .compatible = "nvidia,tegra114-timer" },
> > >       { .compatible = "nvidia,tegra124-timer" },
> > > +     { .compatible = "nvidia,tegra210-timer" },
> > >       { }
> > >  };
> > > --- >8 ---
> > >
> > > I've also tested this on Tegra186, though no additional changes were
> > > needed since Tegra186 doesn't use the Tegra timer.
> > >
> > > With the above folded in, the series is:
> > >
> > > Tested-by: Thierry Reding <treding@nvidia.com>
> >
> > I've also tested your series with the above on Tegra30 (Beaver) and
> > Tegra124 (Jetson TK1), both seem to work fine.
> >
> > Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]