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

[Robert Jarzmik]

Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> 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.

Isn't this a good approach ?

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