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

[Daniel Lezcano]

On 07/03/2014 07:31 PM, Robert Jarzmik wrote:
> 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.

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/