Date:	Thu, 04 Sep 2014 12:41:48 +0200
From:	Bart Tanghe <bart.tanghe@...masmore.be>
To:	monstr@...str.eu, Arnd Bergmann <arnd@...db.de>
CC:	thierry.reding@...il.com, michal.simek@...inx.com,
	robh+dt@...nel.org, pawel.moll@....com, mark.rutland@....com,
	ijc+devicetree@...lion.org.uk, galak@...eaurora.org,
	rob@...dley.net, grant.likely@...aro.org,
	linux-pwm@...r.kernel.org, devicetree@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org
Subject: Re: [rfc]pwm: add xilinx pwm driver

See below

On 2014-05-15 15:56, Michal Simek wrote:
> On 05/15/2014 02:20 PM, Arnd Bergmann wrote:
>> On Thursday 15 May 2014 11:48:52 Michal Simek wrote:
>>> On 05/14/2014 01:26 PM, Bart Tanghe wrote:
>>>> 	add Xilinx PWM support - axi timer hardware block 
>>>>
>>>> Signed-off-by: Bart Tanghe <bart.tanghe@...masmore.be>
>>>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-xlnx.txt b/Documentation/devicetree/bindings/pwm/pwm-xlnx.txt
>>>> new file mode 100644
>>>> index 0000000..cb48926
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/pwm/pwm-xlnx.txt
>>>> @@ -0,0 +1,20 @@
>>>> +Xilinx PWM controller
>>>> +
>>>> +Required properties:
>>>> +- compatible: should be "xlnx,pwm-xlnx"
>>>> +- add a clock source to the description
>>>> +
>>>> +Examples:
>>>> +
>>>> +		axi_timer_0: timer@...00000 {
>>>> +			clock-frequency = <100000000>;
>>>
>>> just remove this from example it is not needed and unused.
>>>
>>>
>>>> +			clocks = <&clkc 15>;
>>>> +			compatible = "xlnx,xlnx-pwm";
>>>> +			reg = <0x42800000 0x10000>;
>>>> +			xlnx,count-width = <0x20>;
>>>> +			xlnx,gen0-assert = <0x1>;
>>>> +			xlnx,gen1-assert = <0x1>;
>>>> +			xlnx,one-timer-only = <0x0>;
>>>> +			xlnx,trig0-assert = <0x1>;
>>>> +			xlnx,trig1-assert = <0x1>;
>>>> +		} ;
>>>
>>> ok. This has to be done completely differently.
>>> I have looked around and found one a little bit older
>>> example but it is in the Linux kernel.
>>> It is pwm-atmel-tcb.c driver and atmel_tclib.c and tcb_clksrc.c.
>>>
>>> Arnd: Isn't there any newer better example how to manage it?
>>
>> Note that we have two atmel pwm drivers: drivers/misc/atmel_pwm.c 
>> and drivers/pwm/pwm-atmel.c. If you want to take that as an example,
>> make sure you use base on the latter.
> 
> Yes, I have seen that there are two drivers. atmel_pwm is older one
> without using that tclib.
> 
>>
>>> Back to atmel example - they are maintaining
>>> internal list of timers (atmel_tclib.c)
>>> and then clocksource driver (tcb_clksrc.c) is asking with atmel_tc_alloc() for it.
>>> The same is for pwm driver (pwm-atmel-tcb.c).
>>>
>>> They probably have all timers the same which is not
>>> our case because they can be different but this can be solved
>>> with flags.
>>>
>>> From DT point of view I think this is the reasonable description
>>>
>>> axi_timer_0: timer@...00000 {
>>> 	clocks = <&clkc 15>;
>>> 	compatible = "xlnx,xps-timer-1.00.a";
>>> 	interrupt-parent = <&xps_intc_0>;
>>> 	interrupts = < 3 2 >;
>>> 	reg = <0x42800000 0x10000>;
>>> 	xlnx,count-width = <0x20>;
>>> 	xlnx,gen0-assert = <0x1>;
>>> 	xlnx,gen1-assert = <0x1>;
>>> 	xlnx,one-timer-only = <0x0>;
>>> 	xlnx,trig0-assert = <0x1>;
>>> 	xlnx,trig1-assert = <0x1>;
>>> } ;
>>>
>>>
>>> pwm {
>>>         compatible = "xlnx,timer-pwm";
>>>         #pwm-cells = <2>;
>>>         timer = <&axi_timer_0>;
>>> };
>>>
>>>
>>> Allocation function will also require to do a change
>>> in clocksource driver because currently the first
>>> timer in the DTS/system is taken for clocksource/clockevents
>>> (others are just ignored).
>>> That's why will be necessary to add clock-handle property
>>> to cpu node to exactly describe which timer is system timer.
>>> The same as is for interrupt-handle (more intc's can be in design).
>>
>> If there is just one set of registers, I wouldn't object to
>> having just a single node in DT, and just one driver that registers
>> to both the clocksource and the PWM interfaces. That might
>> simplify the code.
> 
> IP is configurable as is normal for us.
> You can select IP with just one timer.
> It means register locations for specific timer are fixed.
> http://www.xilinx.com/support/documentation/ip_documentation/xps_timer.pdf
> 
> timer0 - offset 0x0
> timer1 - offset 0x10 (doesn't need to be synthesized)
> 
> There is one interrupt for both timers.
> 
> Timers can be as timers (up/down count/ reload with or without IRQs)
> But then one options is to use both timers and generate PWM signal.
> From full ip description in DT you can see xlnx,gen0-assert = <1>;
> which can suggest that this IP can output PMW signal.
> (We can also detect if PWM0 signal is connected just to be sure
> that PWM can be enabled).
> 
> There is also capture trigger mode where external signal start/stop
> timer counting.
> 
> It means there are 3 modes - timer, capture and PWM.
> Timer (clocksource, clockevent) requires specific handling,
> PWM has own subsystem and not sure if there is any subsystem for
> capture mode. Is there any?
> 
> Not every timer configuration is suitable for all things
> but fully configured timer can be used in all 3 modes.
> 
> 
> I think that make sense to register and map all timers in the system
> and classify them with flags (like can't be used for capture or PMW
> if there are not outputs connected) and then use the best
> timer for clocksource and clockevent. The best candidate is IP
> with the lowest IRQ number in dual timer mode but in general
> whatever timer can be used that's why we can't probably avoid
> timer specification in DT.
> In spite of this smells because you are saying via DT
> to Linux which timer should be used for what purpose.
> 
Can anyone help me with a correct example devicetree description of this block?
As metioned above, this can be used as timer, pwm and capture device.
Is a description as below a good starting point?

axi_timer_0: timer@...00000 {
 	clocks = <&clkc 15>;
 	compatible = "xlnx,xps-timer-1.00.a";
 	interrupt-parent = <&xps_intc_0>;
 	interrupts = < 3 2 >;
 	reg = <0x42800000 0x10000>;
 	xlnx,count-width = <0x20>;
 	xlnx,gen0-assert = <0x1>;
 	xlnx,gen1-assert = <0x1>;
 	xlnx,one-timer-only = <0x0>;
 	xlnx,trig0-assert = <0x1>;
 	xlnx,trig1-assert = <0x1>;
 } ;

 pwm {
         compatible = "xlnx,timer-pwm";
         #pwm-cells = <2>;
         timer = <&axi_timer_0>;
 };


 clocksource {
         compatible = "xlnx,timer-clk";
         timer = <&axi_timer_0>;
 };

 IIOcapture {
         compatible = "xlnx,timer-IIO";
         timer = <&axi_timer_0>;
 };

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

Powered by blists - more mailing lists