Message-ID: <20170608084217.doa7d7hw7pr2bryj@piout.net>
Date:   Thu, 8 Jun 2017 10:42:17 +0200
From:   Alexandre Belloni <alexandre.belloni@...e-electrons.com>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     Boris Brezillon <boris.brezillon@...e-electrons.com>,
        Rob Herring <robh@...nel.org>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Thomas Gleixner <tglx@...utronix.de>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH 46/58] clocksource/drivers: Add a new driver for the
 Atmel ARM TC blocks

On 08/06/2017 at 10:24:17 +0200, Daniel Lezcano wrote:
> On Thu, Jun 08, 2017 at 09:59:01AM +0200, Alexandre Belloni wrote:
> > On 08/06/2017 at 09:44:46 +0200, Daniel Lezcano wrote:
> > > 
> > > +Mark Rutland, +Rob Herring
> > > 
> > > 
> > > Alexandre, Boris, have a look at https://www.spinics.net/lists/arm-kernel/msg572652.html
> > > 
> > > That will tell you the story.
> > > 
> > 
> > Ok, so is the solution putting the driver back in mach-at91 were we can
> > do whatever we want like mach-omap2 is doing?
> 
> No. And putting a driver in mach-<whatever> does not give the permission to do
> whatever you want. I won't tell you how OSS works, but moving code around or
> using another tree to circumvent a code review is just the best way to upset
> maintainers in general and hurt your karma.
> 

I know that but some SoCs will not be able to even boot until we have a
solution. And it has been one year since we raised the issue without any
solution coming from the DT maintainers.


> That said, I think you misunderstood my comment (or I was not clear). In the
> discussion given in the link above, I am in favor, somehow, to distinguish
> clockevent and clocksource to solve exactly what you are facing.
> 
> Rob Herring told me it could be acceptable to have a property to tell if it is
> a clockevent or a clocksource.
> 

Ok, then let's do it!


> Mark Rutland disagreed on this.
> 
> I was alone in the discussion, no consensus have been found.
> 
> Now, you have a particular use case and I would like to resurrect the
> discussion in order to find a solution which can apply to all DT drivers.
> 

Again, it has been one year and it seems nobody actually cares. There is
a whole family of SoCs that can't boot because of that. Should I resort
to the evil vendor tree argument again?




> > > On Thu, Jun 08, 2017 at 07:42:36AM +0200, Boris Brezillon wrote:
> > > > Le Thu, 8 Jun 2017 01:17:15 +0200,
> > > > Alexandre Belloni <alexandre.belloni@...e-electrons.com> a écrit :
> > > > 
> > > > > On 07/06/2017 at 23:08:48 +0200, Daniel Lezcano wrote:
> > > > > > > I was going to agree but this is not flexible enough because the
> > > > > > > quadrature decoder always uses the first two channels. So on some
> > > > > > > products, we may have:
> > > > > > >  - TCB0:
> > > > > > >    o channels 0,1: qdec
> > > > > > >    o channel 2: clocksource
> > > > > > > 
> > > > > > >  - TCB1:
> > > > > > >    o channels 0,1: qdec
> > > > > > >    o channel 2: clockevent
> > > > > > > 
> > > > > > > This avoids wasting TCB channels.  
> > > > > > 
> > > > > > Ok. In this case you can check if the interrupt is specified for the node, if
> > > > > > yes, then it is a clockevent.
> > > > > >   
> > > > > 
> > > > > But currently it is always specified in the SoC's dtsi. I don't find
> > > > > that too practical to push that to the board's dts. Also, lying by
> > > > > omission (the IRQ is always wired) in the DT is not different from
> > > > > having a property selecting which timer is the clocksource and which is
> > > > > the clockevent.
> > > > > 
> > > > 
> > > > I agree with Alexandre here. Really, there's not much we can do to
> > > > detect which timer should be used as a clockevent and which one should
> > > > be used as a clocksource except explicitly specifying it in the DT.
> > > > Having an interrupt defined in one case (clockevent) and undefined in
> > > > the other case (clocksource), is just as hack-ish as the detection logic
> > > > Alexandre developed to avoid explicitly specifying the function
> > > > assigned to a specific timer.
> > > > 
> > > > Can we please find a solution that makes everyone happy (DT,
> > > > clocksoure/clockevent and at91 maintainers)?
> > > > 
> > > > How about adding a linux,timer-function property to specify which
> > > > function this timer is providing?
> > > > 
> > > > Something like that for example:
> > > > 
> > > > 	tcb0: timer@...7c000 {
> > > > 		compatible = "atmel,at91rm9200-tcb", "simple-mfd", "syscon";
> > > > 		#address-cells = <1>;
> > > > 		#size-cells = <0>;
> > > > 		reg = <0xfff7c000 0x100>;
> > > > 		interrupts = <18 4>;
> > > > 		clocks = <&tcb0_clk>, <&clk32k>;
> > > > 		clock-names = "t0_clk", "slow_clk";
> > > > 
> > > > 		timer@0 {
> > > > 			compatible = "atmel,tcb-timer";
> > > > 			reg = <0>, <1>;
> > > > 			linux,timer-function = "clocksource";
> > > > 		};
> > > > 
> > > > 		timer@2 {
> > > > 			compatible = "atmel,tcb-timer";
> > > > 			reg = <2>;
> > > > 			linux,timer-function = "clockevent";
> > > > 		};
> > > > 	};
> > > > 
> > > > Alternatively, we could have a property or a node in chosen describing which
> > > > timer should be used:
> > > > 
> > > > 	chosen {
> > > > 		clockevent {
> > > > 			timer = <&timer2>;
> > > > 		};
> > > > 
> > > > 		clocksource {
> > > > 			timer = <&timer0>;
> > > > 		};
> > > > 
> > > > 		/*
> > > > 		 * or
> > > > 		 *
> > > > 		 * clockevent = <&timer2>;
> > > > 		 * clocksource = <&timer0>;
> > > > 		 *
> > > > 		 * but I think the clocksource/clockevent node approach
> > > > 		 * is more future proof in case we need to add extra
> > > > 		 * information like the expected resolution/precision or
> > > > 		 * anything that could be tweakable.
> > > > 		 */
> > > > 	};
> > > > 
> > > > 	tcb0: timer@...7c000 {
> > > > 		compatible = "atmel,at91rm9200-tcb", "simple-mfd", "syscon";
> > > > 		#address-cells = <1>;
> > > > 		#size-cells = <0>;
> > > > 		reg = <0xfff7c000 0x100>;
> > > > 		interrupts = <18 4>;
> > > > 		clocks = <&tcb0_clk>, <&clk32k>;
> > > > 		clock-names = "t0_clk", "slow_clk";
> > > > 
> > > > 		timer0: timer@0 {
> > > > 			compatible = "atmel,tcb-timer";
> > > > 			reg = <0>, <1>;
> > > > 		};
> > > > 
> > > > 		timer2: timer@2 {
> > > > 			compatible = "atmel,tcb-timer";
> > > > 			reg = <2>;
> > > > 		};
> > > > 	};
> > > 
> > > -- 
> > > 
> > >  <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
> > 
> > -- 
> > Alexandre Belloni, Free Electrons
> > Embedded Linux and Kernel engineering
> > http://free-electrons.com
> 
> -- 
> 
>  <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

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Powered by blists - more mailing lists