linux-kernel - Re: [rfc]pwm: add xilinx pwm driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5049141.XmehsUeakV@wuerfel>
Date:	Thu, 15 May 2014 14:20:11 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	monstr@...str.eu
Cc:	Bart Tanghe <bart.tanghe@...masmore.be>, 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
Subject: Re: [rfc]pwm: add xilinx pwm driver

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.

> 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.

> 	cpus {
> 		#address-cells = <1>;
> 		#cpus = <1>;
> 		#size-cells = <0>;
> 		microblaze_0: cpu@0 {
> 			compatible = "xlnx,microblaze-8.50.a";
> 			interrupt-handle = <&microblaze_0_intc>;
> 			...
> 			clock-handle = <&axi_timer_X>;
> 			...
> 		} ;
> 	} ;
> 
> Then clocksource driver will ask just exact timer which is suitable
> to provide clocksource/clockevent. It can be also split and
> use 2 cells format to specify which timer should be used.
> clocksource-handle = <&axi_timer_X 0>;
> clockeven-handle = <&axi_timer_X 1>;

This seems fine, too.

	Arnd
--
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/