linux-kernel - Re: [PATCH v3 7/9] clocksource: realtek: Add timer driver for rtl-otto platforms

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5c496019-b2ff-4b65-93dd-1184aa4d776d@alliedtelesis.co.nz>
Date: Mon, 1 Jul 2024 02:07:21 +0000
From: Chris Packham <Chris.Packham@...iedtelesis.co.nz>
To: Sander Vanheule <sander@...nheule.net>, "tglx@...utronix.de"
	<tglx@...utronix.de>, "robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>, "tsbogend@...ha.franken.de"
	<tsbogend@...ha.franken.de>, "daniel.lezcano@...aro.org"
	<daniel.lezcano@...aro.org>, "paulburton@...nel.org" <paulburton@...nel.org>,
	"peterz@...radead.org" <peterz@...radead.org>, "mail@...ger-koblitz.de"
	<mail@...ger-koblitz.de>, "bert@...t.com" <bert@...t.com>, "john@...ozen.org"
	<john@...ozen.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-mips@...r.kernel.org" <linux-mips@...r.kernel.org>, "kabel@...nel.org"
	<kabel@...nel.org>, "ericwouds@...il.com" <ericwouds@...il.com>, "Markus
 Stockhausen" <markus.stockhausen@....de>
Subject: Re: [PATCH v3 7/9] clocksource: realtek: Add timer driver for
 rtl-otto platforms


On 30/06/24 09:03, Sander Vanheule wrote:
> Hi Chris,
>
> On Thu, 2024-06-27 at 16:33 +1200, Chris Packham wrote:
>> The timer/counter block on the Realtek SoCs provides up to 5 timers. It
>> also includes a watchdog timer but this isn't being used currently (it
>> will be added as a separate wdt driver).
> Do you mean the watchdog timer supported by drivers/watchdog/realtek_otto_wdt.c? Or are
> you referring to another watchdog timer?

Yes realtek_otto_wdt.c. I thought that was only on openwrt but looks 
like you've already landed that one. I'll reword my message to reflect that.

>> One timer will be used per CPU as a local clock event generator. An
>> additional timer will be used as an overal stable clocksource.
>>
>> Signed-off-by: Markus Stockhausen <markus.stockhausen@....de>
>> Signed-off-by: Sander Vanheule <sander@...nheule.net>
>> Signed-off-by: Chris Packham <chris.packham@...iedtelesis.co.nz>
>> ---
> For reference, I submitted a driver for the same hardware back in 2022, but didn't manage
> to follow up to finalize the submission:
>
> https://lore.kernel.org/all/cover.1642369117.git.sander@svanheule.net/
OK thanks for the link. I'll try and make sure I'm not rehashing things 
that have already been discussed.

>> +
>> +/* Module initialization part. */
>> +static DEFINE_PER_CPU(struct timer_of, rttm_to) = {
>> +	.flags				= TIMER_OF_BASE | TIMER_OF_CLOCK |
>> TIMER_OF_IRQ,
>> +	.of_irq = {
>> +		.flags			= IRQF_PERCPU | IRQF_TIMER,
> In the original review of this code, I had some doubts about the use of IRQF_PERCPU. Maybe
> the people in Cc can shed some light on this.
>
> If I understand correctly, the SoC interrupts these timers use are not per-cpu interrupts.
> (For comparison, AFAICT the MIPS CPU interrupts are)

I'm not exactly sure either and I've only got the single core RTL9302 to 
test with so if there were some difference I may not notice. Eventually 
we're planning a RTL931x based board so that may be something I can 
check then.

>> +		.handler		= rttm_timer_interrupt,
>> +	},
>> +	.clkevt = {
>> +		.rating			= 400,
>> +		.features		= CLOCK_EVT_FEAT_PERIODIC |
>> CLOCK_EVT_FEAT_ONESHOT,
> If the use of IRQF_PERCPU is appropriate, I wonder if the driver should also use
> CLOCK_EVT_FEAT_PERCPU.
>
>
> Best,
> Sander
>