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] [day] [month] [year] [list]

Message-ID: <3520a743-2962-43ee-83f5-3e9384fe4c42@alliedtelesis.co.nz>
Date: Mon, 1 Jul 2024 04:41:52 +0000
From: Chris Packham <Chris.Packham@...iedtelesis.co.nz>
To: Marek Behún <kabel@...nel.org>
CC: "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>, "sander@...nheule.net" <sander@...nheule.net>,
	"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>,
	"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 27/06/24 23:17, Marek Behún wrote:
> On Thu, 27 Jun 2024 16:33:15 +1200
> Chris Packham <chris.packham@...iedtelesis.co.nz> wrote:
>
>> +/* Simple internal register functions */
>> +static inline void rttm_set_counter(void __iomem *base, unsigned int counter)
>> +{
>> +	iowrite32(counter, base + RTTM_CNT);
> These require #include <asm/io.h>
linux/io.h I'm guessing.
>> +/* Aggregated control functions for kernel clock framework */
>> +#define RTTM_DEBUG(base)			\
>> +	pr_debug("------------- %d %p\n",	\
>> +		 smp_processor_id(), base)
> #include <linux/printk.h>
ack
>> +static irqreturn_t rttm_timer_interrupt(int irq, void *dev_id)
>> +{
>> +	struct clock_event_device *clkevt = dev_id;
>> +	struct timer_of *to = to_timer_of(clkevt);
>> +
>> +	rttm_ack_irq(to->of_base.base);
>> +	RTTM_DEBUG(to->of_base.base);
>> +	clkevt->event_handler(clkevt);
> Although you include "timer-of.h", which includes clockchips.h, please
> do also explicit #include <linux/clockchips.h>
>
>> +	rttm_set_period(to->of_base.base, RTTM_TICKS_PER_SEC / HZ);
> HZ -> linux/jiffies.h, or maybe asm/param.h
ack
>> +static u64 rttm_read_clocksource(struct clocksource *cs)
>> +{
>> +	struct rttm_cs *rcs = container_of(cs, struct rttm_cs, cs);
>> +
>> +	return (u64)rttm_get_counter(rcs->to.of_base.base);
> Redundant cast to u64.
ack
>> +	rttm_enable_timer(rcs->to.of_base.base, RTTM_CTRL_TIMER,
>> +			  rcs->to.of_clk.rate / RTTM_TICKS_PER_SEC);
> Is this correct? Sometimes it makes sense to use DIV_ROUND_CLOSEST, but
> maybe not here.

It's OK for me because the Lexra bus clock is 175mhz so plain division 
works fine. The docs do say something about a configurable divisor but 
the range seems fairly limited so I don't think there's a specific need 
to use DIV_ROUND_CLOSEST.

>
>> +static u64 notrace rttm_read_clock(void)
>> +{
>> +	return (u64)rttm_get_counter(rttm_cs.to.of_base.base);
> Redundant cast to u64.
ack
>> +static int __init rttm_probe(struct device_node *np)
>> +{
>> +	int cpu, cpu_rollback;
> unsigned int?
ack
>> +	struct timer_of *to;
>> +	int clkidx = num_possible_cpus();
> linux/cpumask.h, unsigned int
ack
> Marek