RE: [PATCH 1/2] dt-bindings: document renesas-ostm timer

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux