linux-kernel - Re: [PATCH 2/3] clocksource: add device-tree support for PXA timer

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: <53B6471D.3010001@linaro.org>
Date:	Fri, 04 Jul 2014 08:18:05 +0200
From:	Daniel Lezcano <daniel.lezcano@...aro.org>
To:	Robert Jarzmik <robert.jarzmik@...e.fr>
CC:	Haojian Zhuang <haojian.zhuang@...il.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] clocksource: add device-tree support for PXA timer

On 07/03/2014 07:31 PM, Robert Jarzmik wrote:
> Daniel Lezcano <daniel.lezcano@...aro.org> writes:
>
>>> -#include <mach/regs-ost.h>
>>>    #include <mach/irqs.h>
>>> +#include <mach/hardware.h>
>>
>> Now as the driver is in 'drivers', do not reference the headers files in
>> mach. Moving the driver to the drivers directory implies some cleanup with the
>> headers dependencies.
> I don't see that very possible.
> Or said another way, I don't see how the irq number, IRQ_OST0 (in mach/irqs.h)
> can be guessed for non device-tree configuration.
>
>>> +#define OSMR0		0x00	/* */
>>> +#define OSMR1		0x04	/* */
>>> +#define OSMR2		0x08	/* */
>>> +#define OSMR3		0x0C	/* */
>>> +#define OSMR4		0x80	/* */
>>
>> Can you please remove those unused empty comment or fill them with something
>> appropriate.
> Sure.
>
>>
>>> +#define OSCR		0x10	/* OS Timer Counter Register */
>>> +#define OSCR4		0x40	/* OS Timer Counter Register */
>>> +#define OMCR4		0xC0	/* */
>>> +#define OSSR		0x14	/* OS Timer Status Register */
>>> +#define OWER		0x18	/* OS Timer Watchdog Enable Register */
>>> +#define OIER		0x1C	/* OS Timer Interrupt Enable Register */
>>> +
>>> +#define OSSR_M3		(1 << 3)	/* Match status channel 3 */
>>> +#define OSSR_M2		(1 << 2)	/* Match status channel 2 */
>>> +#define OSSR_M1		(1 << 1)	/* Match status channel 1 */
>>> +#define OSSR_M0		(1 << 0)	/* Match status channel 0 */
>>> +
>>> +#define OWER_WME	(1 << 0)	/* Watchdog Match Enable */
>>> +
>>> +#define OIER_E3		(1 << 3)	/* Interrupt enable channel 3 */
>>> +#define OIER_E2		(1 << 2)	/* Interrupt enable channel 2 */
>>> +#define OIER_E1		(1 << 1)	/* Interrupt enable channel 1 */
>>> +#define OIER_E0		(1 << 0)	/* Interrupt enable channel 0 */
>>
>> Is it possible to do some cleanups around regs-ost.h and here in order to remove
>> the unused macros. Also, it seems some define will be duplicate as they are
>> shared with the watchdog. Any plan to fix that ?
> For the cleanup, yes, will do.
>
> For the watchdog I don't have any plan yet. This patch's purpose is only to
> bring the PXA time source to drivers/clocksource, and make it compatible with
> both device-tree and non device-tree builds.
>
>>> @@ -33,9 +60,14 @@
>>>     * calls to sched_clock() which should always be the case in practice.
>>>     */
>>>
>>> +#define timer_readl(reg) readl_relaxed(timer_base + (reg))
>>> +#define timer_writel(val, reg) writel_relaxed((val), timer_base + (reg))
>>> +
>>> +static void __iomem *timer_base;
>>> +
>>>    static u64 notrace pxa_read_sched_clock(void)
>>>    {
>>> -	return readl_relaxed(OSCR);
>>
>> So here there is a change which is not explained in the changelog (timer_base
>> offset).
>>
>> Even it is obvious for me because I am used to see this kind of code, that would
>> deserve a better description in the changelog.
> OK, I'll add the backward compatibility explanation with non device-tree builds,
> and the necessary timer_base iomem hard encoded value. And the Janus double face
> explanation of the driver (both DT and non-DT oriented).
>
> Another question brought up by this : if I remove all 'mach/' includes, I loose
> io_p2v() right ? How can I guess timer_base then ?
>
>>> +	/* we are only interested in OS-timer0 irq */
>>> +	irq = irq_of_parse_and_map(np, 0);
>>> +	if (irq <= 0)
>>> +		panic("%s: unable to parse OS-timer0 irq\n", np->name);
>>
>> Is the 'panic' desirable ? The clksrc-of is written in a way to use different
>> clocks, no ?
> Good question.
>
> Maybe not, I followed the same rationale as in orion-timer, which is :
>   - as this timer is the only possible timer for PXA boards, and because without
>   it the kernel boot will stall (scheduling will be blocked), it's better to
>   panic early that to remain stalled.

There isn't the arm global timer ?

> Isn't this a good approach ?

I suppose we can live with that. IMO, the right fix would be in 
clksrc-of to pr_crit a message when an initialization fails. But that 
means to change all the init functions for all drivers which is out of 
the scope of this patchset.


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/