Re: [PATCH 2/2]: Be stric clocksource/drivers/fttmr010 on IRQs

From: Guenter Roeck <linux@roeck-us.net>
To: "Cédric Le Goater" <clg@kaod.org>,
	"Andrew Jeffery" <andrew@aj.id.au>,
	"Linus Walleij" <linus.walleij@linaro.org>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Joel Stanley <joel@jms.id.au>
Subject: Re: [PATCH 2/2]: Be stric clocksource/drivers/fttmr010 on IRQs
Date: Mon, 30 Aug 2021 08:24:12 -0700	[thread overview]
Message-ID: <7f46191c-2400-2679-9abd-e517fb78b57b@roeck-us.net> (raw)
In-Reply-To: <cc934baf-75a9-d7f6-44ac-e7fa1c6f69b6@kaod.org>

On 8/30/21 12:47 AM, Cédric Le Goater wrote:
> On 8/30/21 6:58 AM, Guenter Roeck wrote:
>> On 8/29/21 9:16 PM, Andrew Jeffery wrote:
>> [ ... ]
>>>>
>>>>> I don't have the manuals, so I can't say what the correct behavior is,
>>>>> but at least there is some evidence that TIMER_INTR_STATE may not exist
>>>>> on ast2400 and ast2500 SOCs.
>>>>
>>>> On Aspeed SoCs AST2400 and AST2500, the TMC[34] register is a
>>>> "control register #2" whereas on the AST2600 it is an "interruptarch/arm/boot/dts/ast2600-facebook-netbmc-common.dtsi:#include
>>>> status register" with bits [0-7] holding the timers status.
>>>>
>>>> I would say that the patch simply should handle the "is_aspeed" case.
>>>
>>> Well, is_aspeed is set true in the driver for all of the 2400, 2500 and
>>> 2600. 0x34 behaves the way this patch expects on the 2600. So I think
>>> we need something less coarse than is_aspeed?
>>>
>>
>> If I understand the code correctly, ast2400 and ast2500 execute
>> fttmr010_timer_interrupt(), while ast2600 has its own interrupt handler.
>> To make this work, it would probably be necessary to check for is_aspeed
>> in fttmr010_timer_interrupt(), and only execute the new code if the flag
>> is false. The existing flag in struct fttmr010 should be good enough
>> for that.
> 
> yes.
> 
> I wonder why we have ast2600 support in fttmr010. The AST2600 boards use
> the arm_arch_timer AFAICT.
> 

It was introduced and enabled, but later disabled with commit c998f40f2ae6a4
("ARM: dts: aspeed: ast2600: Set arch timer always-on"):

"According to ASPEED, FTTMR010 is not intended to be used in the AST2600.
  The arch timer should be used, but Linux doesn't enable high-res timers
  without being assured that the arch timer is always on, so set that
  property in the devicetree."

That commit also disables the FTTMR010 timer, but doesn't remove the devicetree
node. Maybe it would make sense to remove the ast2600 code from the fttmr010
driver, including the devicetree node. After all, it looks like it is dead.

Guenter