Hi Geert,
Thank you for your review.
On Friday, January 13, 2017, Geert Uytterhoeven wrote:
> > +The OSTM comes with 2 independent channels.
> > +We will use the first channel (OSTM0) as a free running clocksource
> > +and the second channel (OSTM1) as a interrupt driven clock event.
> > +
> > +Additionally we will use the clocksource channel (OTSM0) for the
> > +system schedule timer sched_clock().
>
> The above two sentences are software policy, not hardware description.
> Hence they do not belong in the DT bindings document.
> You can move them to the commit description, though.
OK.
> > +Required Properties:
> > +
> > + - compatible: must be one or more of the following:
> > + - "renesas,ostm-r7s72100" for the r7s72100 OSTM
>
> Please use "renesas,r7s72100-ostm" instead, to match current practices.
If I look at the current r7s72100.dtsi:
compatible = "renesas,r7s72100-cpg-clocks", "renesas,rz-cpg-clocks";
compatible = "renesas,r7s72100-mstp-clocks", "renesas,cpg-mstp-clocks";
compatible = "renesas,scif-r7s72100", "renesas,scif";
compatible = "renesas,rspi-r7s72100", "renesas,rspi-rz";
compatible = "renesas,riic-r7s72100", "renesas,riic-rz";
compatible = "renesas,mtu2-r7s72100", "renesas,mtu2";
compatible = "renesas,mmcif-r7s72100", "renesas,sh-mmcif";
compatible = "renesas,sdhi-r7s72100";
Is "renesas,xxx-r7s7210" the old way, and "renesas,r7s72100-xxx" is the new way??
> > + - reg: base address and length of the registers block for each timer
> channel.
> > + There should be 2 sets of addresses, one for each channel.
> > +
> > + - interrupts: interrupt specifiers for the timers. There should be 2
> > + interupts, one for each channel.
> > +
> > + - clocks: a list of phandle + clock-specifier pairs, one for each
> entry
> > + channel. There should be 2 sets, one for each channel.
>
> Are the channels truly independent? If yes, I think it's better to have
> two separate device nodes, one for each channel.
> Each channel has its own module clock, so using separate devices means
> Runtime PM can manage both channels through their module clocks as soon as
> you add a "power-domains" property pointing to the clock domain controller.
Yes, technically they are independent channels.
The way the driver is currently written, 1 instance of the driver uses 2 channels
for different things. Ch0 will be set up as a 'clocksource', and ch1 will be set up
as a 'clock event'.
As in:
static int __init ostm_timer_init(struct ostm_device *ostm)
{
int ret = 0;
/* ostm0 will be clock source */
ret = ostm_init_clksrc(ostm);
if (ret)
goto err;
/* use ostm0 as system scheduling clock */
ret = ostm_init_sched_clock(&ostm->clksrc);
if (ret)
goto err;
/* ostm1 will be clock event */
ret = ostm_init_clkevt(ostm);
err:
return ret;
}
Do you think it would be better if a driver instance only does 1 thing: Either
'clocksource' or 'clock event'??
Then, I would make 2 ostm nodes and pass in the mode I would like it operate in?
For example:
&ostm0 {
mode = "clocksource";
status = "okay";
};
&ostm1 {
mode = "clock-event";
status = "okay";
};
Thank you,
Chris
