diff mbox

[rfc] pwm: add xilinx pwm driver

Message ID 1400066773-14393-1-git-send-email-bart.tanghe@thomasmore.be
State Superseded, archived
Headers show

Commit Message

Bart Tanghe May 14, 2014, 11:26 a.m. UTC
add Xilinx PWM support - axi timer hardware block 

Signed-off-by: Bart Tanghe <bart.tanghe@thomasmore.be>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Arnd Bergmann May 15, 2014, 7:23 a.m. UTC | #1
On Wednesday 14 May 2014 13:26:13 Bart Tanghe wrote:
> @@ -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@42800000 {
> +                       clock-frequency = <100000000>;
> +                       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>;
> +               } ;
> 

It seems you are missing a mandatory "#pwm-cells" property.
How is anybody supposed to use this?

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Tanghe May 15, 2014, 8:50 a.m. UTC | #2
On 05/15/2014 09:23 AM, Arnd Bergmann wrote:
> On Wednesday 14 May 2014 13:26:13 Bart Tanghe wrote:
>> @@ -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@42800000 {
>> +                       clock-frequency = <100000000>;
>> +                       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>;
>> +               } ;
>>
>
> It seems you are missing a mandatory "#pwm-cells" property.
> How is anybody supposed to use this?
>
> 	Arnd
>

I've added some additional information in the documentation



Xilinx PWM controller

This driver works together with the Xilinx Axi timer hardware core.
The core is available for the microblaze, powerpc and arm based Xilinx
platforms.

The axi timer core is implemented in the pl (programmable logic) of the
fpga. The amount is user defined. Each core has two timers and one pwm 
output.

After genereating the design and bitstream, you have to use the Xilinx 
device tree generator (git://git.xilinx.com/device-tree.git) to generate 
the dts file or the petalinux tools 
(http://www.xilinx.com/tools/petalinux-sdk.htm).

Within this dts file, you find the axi-timer and clock definitions.

		axi_timer_0: timer@42800000 {
			clock-frequency = <100000000>;
			compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
			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>;
		} ;

		...

ps7_slcr_0: ps7-slcr@f8000000 {
compatible = "xlnx,ps7-slcr-1.00.a", "xlnx,zynqslcr";
reg = <0xf8000000 0x1000>;
clocks {
	#address-cells = <1>;
	#size-cells = <0>;
	clkc: clkc {
		#clock-cells = <1>;
		clock-output-names = "armpll", "ddrpll", "iopll", "cpu_6or4x", 
"cpu_3or2x",
		"cpu_2x", "cpu_1x", "ddr2x", "ddr3x", "dci",
		"lqspi", "smc", "pcap", "gem0", "gem1",
		"fclk0", "fclk1", "fclk2", "fclk3", "can0",
		"can1", "sdio0", "sdio1", "uart0", "uart1",
		"spi0", "spi1", "dma", "usb0_aper", "usb1_aper",
		"gem0_aper", "gem1_aper", "sdio0_aper", "sdio1_aper", "spi0_aper",
		"spi1_aper", "can0_aper", "can1_aper", "i2c0_aper", "i2c1_aper",
		"uart0_aper", "uart1_aper", "gpio_aper", "lqspi_aper", "smc_aper",
		"swdt", "dbg_trc", "dbg_apb";
	compatible = "xlnx,ps7-clkc";
	fclk-enable = <0xf>;
	ps-clk-frequency = <33333333>;
	} ;
} ;
} ;

Some required properties should be changed
- compatible: should be "xlnx,pwm-xlnx"
- add the right clock source to the description - clocks = <&clkc 15>; 
fclk0 in this design
- add #pwm-cells = <1>;

Examples:

		axi_timer_0: timer@42800000 {
			clock-frequency = <100000000>;
			#pwm-cells = <1>;
			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>;
		} ;

Generate the dtb file and load it on the target platform.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michal Simek May 15, 2014, 9:48 a.m. UTC | #3
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@thomasmore.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@42800000 {
> +			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?

The latest timer driver is available here
arch/microblaze/kernel/timer.c which has support for big
and little endian.
There is probably no timer driver for old xilinx ppc405/ppc440
platforms but this timer can be also used there.

The timer driver needs to be moved from microblaze
folder to drivers/clocksource/ and should be just git mv
because I have done some changes some time ago
that it is compatible with clocksource drivers.

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@42800000 {
	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).

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



> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index eece329..1d59b02 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -233,4 +233,17 @@ config PWM_VT8500
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-vt8500.
>  
> +config PWM_XLNX
> +	tristate "Xilinx PWM support"
> +	help
> +	  Generic PWM framework driver for xilinx axi timer.
> +
> +	  This driver implements the pwm channel of a xilinx axi timer hardware

in the next email you are writing about ppc. It means xilinx timer just
here. Because it can be opb/plb/axi now.

> +	  block.
> +	  The hardware block has to be configured with generate output signal
> +	  of timer 1 and timer 2 active high.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-xlnx.
> +
>  endif
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 8b754e4..e0c5f59 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -21,3 +21,4 @@ obj-$(CONFIG_PWM_TIPWMSS)	+= pwm-tipwmss.o
>  obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
>  obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
>  obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
> +obj-$(CONFIG_PWM_XLNX)		+= pwm-xlnx.o
> diff --git a/drivers/pwm/pwm-xlnx.c b/drivers/pwm/pwm-xlnx.c
> new file mode 100644
> index 0000000..9c1be71
> --- /dev/null
> +++ b/drivers/pwm/pwm-xlnx.c
> @@ -0,0 +1,197 @@
> +/*
> + * pwm-xlnx driver
> + * Tested on zedboard - axi timer v2.00a
> + *
> + * Copyright (C) 2014 Thomas more

more?

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +
> +/*mmio regiser mapping*/

typo and use proper comment format, fix comment format globally.

> +
> +#define OFFSET		0x10
> +#define DUTY		0x14
> +#define PERIOD		0x04

this has to reuse proper register name from datasheet.

> +
> +/* configure the counters as 32 bit counters */
> +
> +#define PWM_CONF	0x00000206
> +#define PWM_ENABLE	0x00000080

same for bits.

> +
> +#define DRIVER_AUTHOR "Bart Tanghe <bart.tanghe@thomasmore.be>"
> +#define DRIVER_DESC "A Xilinx pwm driver"

Just use it directly below.

> +
> +struct xlnx_pwm_chip {
> +	struct pwm_chip chip;
> +	struct device *dev;
> +	int scaler;
> +	void __iomem *mmio_base;
> +};
> +
> +static inline struct xlnx_pwm_chip *to_xlnx_pwm_chip(
> +					struct pwm_chip *chip){
> +
> +	return container_of(chip, struct xlnx_pwm_chip, chip);
> +}
> +
> +static int xlnx_pwm_config(struct pwm_chip *chip,
> +			      struct pwm_device *pwm,
> +			      int duty_ns, int period_ns){
> +
> +	struct xlnx_pwm_chip *pc;
> +
> +	pc = container_of(chip, struct xlnx_pwm_chip, chip);
> +
> +	iowrite32(duty_ns/pc->scaler - 2, pc->mmio_base + DUTY);
> +	iowrite32(period_ns/pc->scaler - 2, pc->mmio_base + PERIOD);

not a proper coding style.


> +
> +	return 0;
> +}
> +
> +static int xlnx_pwm_enable(struct pwm_chip *chip,
> +			      struct pwm_device *pwm){
> +
> +	struct xlnx_pwm_chip *pc;
> +
> +	pc = container_of(chip, struct xlnx_pwm_chip, chip);
> +
> +	iowrite32(ioread32(pc->mmio_base) | PWM_ENABLE, pc->mmio_base);
> +	iowrite32(ioread32(pc->mmio_base + OFFSET) | PWM_ENABLE,
> +						pc->mmio_base + OFFSET);
> +	return 0;
> +}
> +
> +static void xlnx_pwm_disable(struct pwm_chip *chip,
> +				struct pwm_device *pwm)
> +{
> +	struct xlnx_pwm_chip *pc;
> +
> +	pc = to_xlnx_pwm_chip(chip);
> +
> +	iowrite32(ioread32(pc->mmio_base) & ~PWM_ENABLE, pc->mmio_base);
> +	iowrite32(ioread32(pc->mmio_base + OFFSET) & ~PWM_ENABLE,
> +						pc->mmio_base + OFFSET);
> +}
> +
> +static int xlnx_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> +				enum pwm_polarity polarity)
> +{
> +	struct xlnx_pwm_chip *pc;
> +
> +	pc = to_xlnx_pwm_chip(chip);
> +
> +	/* no implementation of polarity function
> +	   the axi timer hw block doesn't support this
> +	*/

wrong comment.

> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops xlnx_pwm_ops = {
> +	.config = xlnx_pwm_config,
> +	.enable = xlnx_pwm_enable,
> +	.disable = xlnx_pwm_disable,
> +	.set_polarity = xlnx_set_polarity,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int xlnx_pwm_probe(struct platform_device *pdev)
> +{
> +	struct xlnx_pwm_chip *pwm;
> +
> +	int ret;
> +	struct resource *r;
> +	u32 start, end;
> +	struct clk *clk;
> +
> +	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm) {
> +		dev_err(&pdev->dev, "failed to allocate memory\n");

don't need this error message. It is reported by core automatically.

> +		return -ENOMEM;
> +	}
> +
> +	pwm->dev = &pdev->dev;
> +
> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk)) {
> +		dev_err(&pdev->dev, "could not find clk: %ld\n", PTR_ERR(clk));
> +		devm_kfree(&pdev->dev, pwm);

this is called automatically that's why you are calling devm_ functions.

> +		return PTR_ERR(clk);
> +	}
> +
> +	pwm->scaler = (int)1000000000/clk_get_rate(clk);

magic value - comment will be useful.

> +
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	pwm->mmio_base = devm_ioremap_resource(&pdev->dev, r);
> +	if (IS_ERR(pwm->mmio_base)) {
> +		devm_kfree(&pdev->dev, pwm);

ditto.

> +		return PTR_ERR(pwm->mmio_base);
> +	}
> +
> +	start = r->start;
> +	end = r->end;
> +
> +	pwm->chip.dev = &pdev->dev;
> +	pwm->chip.ops = &xlnx_pwm_ops;
> +	pwm->chip.base = (int)&pdev->id;
> +	pwm->chip.npwm = 1;
> +
> +	ret = pwmchip_add(&pwm->chip);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> +		devm_kfree(&pdev->dev, pwm);
> +		return -1;
> +	}
> +
> +	/*set the pwm0 configuration*/
> +	iowrite32(PWM_CONF, pwm->mmio_base);
> +	iowrite32(PWM_CONF, pwm->mmio_base + OFFSET);
> +
> +	platform_set_drvdata(pdev, pwm);
> +
> +	return 0;
> +}
> +
> +static int xlnx_pwm_remove(struct platform_device *pdev)
> +{
> +
> +	struct xlnx_pwm_chip *pc;
> +	pc  = platform_get_drvdata(pdev);

two spaces.

> +
> +	if (WARN_ON(!pc))
> +		return -ENODEV;
> +
> +	return pwmchip_remove(&pc->chip);
> +}
> +
> +static const struct of_device_id xlnx_pwm_of_match[] = {
> +	{ .compatible = "xlnx,pwm-xlnx", },
> +	{ /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, xlnx_pwm_of_match);
> +
> +static struct platform_driver xlnx_pwm_driver = {
> +	.driver = {
> +		.name = "pwm-xlnx",
> +		.owner = THIS_MODULE,
> +		.of_match_table = xlnx_pwm_of_match,
> +	},
> +	.probe = xlnx_pwm_probe,
> +	.remove = xlnx_pwm_remove,
> +};
> +module_platform_driver(xlnx_pwm_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(DRIVER_DESC);


Thanks,
Michal
Michal Simek May 15, 2014, 10:33 a.m. UTC | #4
On 05/15/2014 10:50 AM, Bart Tanghe wrote:
> On 05/15/2014 09:23 AM, Arnd Bergmann wrote:
>> On Wednesday 14 May 2014 13:26:13 Bart Tanghe wrote:
>>> @@ -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@42800000 {
>>> +                       clock-frequency = <100000000>;
>>> +                       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>;
>>> +               } ;
>>>
>>
>> It seems you are missing a mandatory "#pwm-cells" property.
>> How is anybody supposed to use this?
>>
>>     Arnd
>>
> 
> I've added some additional information in the documentation
> 
> 
> 
> Xilinx PWM controller
> 
> This driver works together with the Xilinx Axi timer hardware core.
> The core is available for the microblaze, powerpc and arm based Xilinx
> platforms.
> 
> The axi timer core is implemented in the pl (programmable logic) of the
> fpga. The amount is user defined. Each core has two timers and one pwm output.

For PWM you have to have 2 timers but core itself can be configured
to any configuration and I expect that any output has to be used.

Can you describe me your testing enviroment with zedboard?
Do you use any additional hw or are using just leds or any device
on zedboard?

Thanks,
Michal


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Tanghe May 15, 2014, 11:30 a.m. UTC | #5
On 05/15/2014 12:33 PM, Michal Simek wrote:
> On 05/15/2014 10:50 AM, Bart Tanghe wrote:
>> On 05/15/2014 09:23 AM, Arnd Bergmann wrote:
>>> On Wednesday 14 May 2014 13:26:13 Bart Tanghe wrote:
>>>> @@ -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@42800000 {
>>>> +                       clock-frequency = <100000000>;
>>>> +                       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>;
>>>> +               } ;
>>>>
>>>
>>> It seems you are missing a mandatory "#pwm-cells" property.
>>> How is anybody supposed to use this?
>>>
>>>      Arnd
>>>
>>
>> I've added some additional information in the documentation
>>
>>
>>
>> Xilinx PWM controller
>>
>> This driver works together with the Xilinx Axi timer hardware core.
>> The core is available for the microblaze, powerpc and arm based Xilinx
>> platforms.
>>
>> The axi timer core is implemented in the pl (programmable logic) of the
>> fpga. The amount is user defined. Each core has two timers and one pwm output.
>
> For PWM you have to have 2 timers but core itself can be configured
> to any configuration and I expect that any output has to be used.
>
> Can you describe me your testing enviroment with zedboard?
> Do you use any additional hw or are using just leds or any device
> on zedboard?
>
> Thanks,
> Michal
>
>

I'm using the 2 internal timers of the axi_timer core. I've attached the 
pwm output of the timer core to one of the leds. The generate out0 and 
generate out1 signals aren't attached.
I've added 4 axi timers to the design, so I can use the 4 leds of 
zedboard to visualize and measure the pwm signal.

Regards,

Bart
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michal Simek May 15, 2014, 11:54 a.m. UTC | #6
On 05/15/2014 01:30 PM, Bart Tanghe wrote:
> 
> 
> On 05/15/2014 12:33 PM, Michal Simek wrote:
>> On 05/15/2014 10:50 AM, Bart Tanghe wrote:
>>> On 05/15/2014 09:23 AM, Arnd Bergmann wrote:
>>>> On Wednesday 14 May 2014 13:26:13 Bart Tanghe wrote:
>>>>> @@ -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@42800000 {
>>>>> +                       clock-frequency = <100000000>;
>>>>> +                       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>;
>>>>> +               } ;
>>>>>
>>>>
>>>> It seems you are missing a mandatory "#pwm-cells" property.
>>>> How is anybody supposed to use this?
>>>>
>>>>      Arnd
>>>>
>>>
>>> I've added some additional information in the documentation
>>>
>>>
>>>
>>> Xilinx PWM controller
>>>
>>> This driver works together with the Xilinx Axi timer hardware core.
>>> The core is available for the microblaze, powerpc and arm based Xilinx
>>> platforms.
>>>
>>> The axi timer core is implemented in the pl (programmable logic) of the
>>> fpga. The amount is user defined. Each core has two timers and one pwm output.
>>
>> For PWM you have to have 2 timers but core itself can be configured
>> to any configuration and I expect that any output has to be used.
>>
>> Can you describe me your testing enviroment with zedboard?
>> Do you use any additional hw or are using just leds or any device
>> on zedboard?
>>
>> Thanks,
>> Michal
>>
>>
> 
> I'm using the 2 internal timers of the axi_timer core. I've attached the pwm output of the timer core to one of the leds. The generate out0 and generate out1 signals aren't attached.
> I've added 4 axi timers to the design, so I can use the 4 leds of zedboard to visualize and measure the pwm signal.

ok, that's configuration what I expected.
Can you add or just send me this hw design? I can create it myself but
no reason to spend time on it when you have it.

Thanks,
Michal
Arnd Bergmann May 15, 2014, 12:20 p.m. UTC | #7
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@thomasmore.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@42800000 {
> > +			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@42800000 {
> 	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 devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michal Simek May 15, 2014, 1:56 p.m. UTC | #8
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@thomasmore.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@42800000 {
>>> +			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@42800000 {
>> 	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.

Thanks,
Michal
Arnd Bergmann May 15, 2014, 4:30 p.m. UTC | #9
On Thursday 15 May 2014 15:56:03 Michal Simek wrote:
> 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?

I don't think so. Possibly somewhere in IIO.

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

We had discussions about this in the past, but I don't remember the
outcome of that.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding May 15, 2014, 8:49 p.m. UTC | #10
On Thu, May 15, 2014 at 06:30:13PM +0200, Arnd Bergmann wrote:
> On Thursday 15 May 2014 15:56:03 Michal Simek wrote:
> > 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?
> 
> I don't think so. Possibly somewhere in IIO.

I think so too. There was a patch set not so long ago that added PWM
capture support for one of the TI PWM controllers to IIO.

Thierry
Michal Simek May 16, 2014, 7:53 a.m. UTC | #11
On 05/15/2014 06:30 PM, Arnd Bergmann wrote:
> On Thursday 15 May 2014 15:56:03 Michal Simek wrote:
>> 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?
> 
> I don't think so. Possibly somewhere in IIO.
> 
>> 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.
> 
> We had discussions about this in the past, but I don't remember the
> outcome of that.

Will be great if someone is able to find out this outcome.
IRC: You can register as many clocksources as you like and core
itself choose the best one - the most accurate one.

But I don't think for our case there is any universal algorithm
which we can use to find out which timer should be used for this
purpose that's why selecting it via DT is probably the best what we
can do.

Thanks,
Michal
Michal Simek May 16, 2014, 8:44 a.m. UTC | #12
On 05/15/2014 10:49 PM, Thierry Reding wrote:
> On Thu, May 15, 2014 at 06:30:13PM +0200, Arnd Bergmann wrote:
>> On Thursday 15 May 2014 15:56:03 Michal Simek wrote:
>>> 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?
>>
>> I don't think so. Possibly somewhere in IIO.
> 
> I think so too. There was a patch set not so long ago that added PWM
> capture support for one of the TI PWM controllers to IIO.

It is probably this.
https://lwn.net/Articles/583998/
https://lkml.org/lkml/2014/2/5/378

There were some comments in v3 which haven't been fixed.
Matt: What's the status on this?

Thanks,
Michal
Bart Tanghe Sept. 4, 2014, 10:41 a.m. UTC | #13
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@thomasmore.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@42800000 {
>>>> +			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@42800000 {
>>> 	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@42800000 {
 	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 devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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@42800000 {
+			clock-frequency = <100000000>;
+			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>;
+		} ;
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index eece329..1d59b02 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -233,4 +233,17 @@  config PWM_VT8500
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-vt8500.
 
+config PWM_XLNX
+	tristate "Xilinx PWM support"
+	help
+	  Generic PWM framework driver for xilinx axi timer.
+
+	  This driver implements the pwm channel of a xilinx axi timer hardware
+	  block.
+	  The hardware block has to be configured with generate output signal
+	  of timer 1 and timer 2 active high.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-xlnx.
+
 endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 8b754e4..e0c5f59 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -21,3 +21,4 @@  obj-$(CONFIG_PWM_TIPWMSS)	+= pwm-tipwmss.o
 obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
 obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
 obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
+obj-$(CONFIG_PWM_XLNX)		+= pwm-xlnx.o
diff --git a/drivers/pwm/pwm-xlnx.c b/drivers/pwm/pwm-xlnx.c
new file mode 100644
index 0000000..9c1be71
--- /dev/null
+++ b/drivers/pwm/pwm-xlnx.c
@@ -0,0 +1,197 @@ 
+/*
+ * pwm-xlnx driver
+ * Tested on zedboard - axi timer v2.00a
+ *
+ * Copyright (C) 2014 Thomas more
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+
+/*mmio regiser mapping*/
+
+#define OFFSET		0x10
+#define DUTY		0x14
+#define PERIOD		0x04
+
+/* configure the counters as 32 bit counters */
+
+#define PWM_CONF	0x00000206
+#define PWM_ENABLE	0x00000080
+
+#define DRIVER_AUTHOR "Bart Tanghe <bart.tanghe@thomasmore.be>"
+#define DRIVER_DESC "A Xilinx pwm driver"
+
+struct xlnx_pwm_chip {
+	struct pwm_chip chip;
+	struct device *dev;
+	int scaler;
+	void __iomem *mmio_base;
+};
+
+static inline struct xlnx_pwm_chip *to_xlnx_pwm_chip(
+					struct pwm_chip *chip){
+
+	return container_of(chip, struct xlnx_pwm_chip, chip);
+}
+
+static int xlnx_pwm_config(struct pwm_chip *chip,
+			      struct pwm_device *pwm,
+			      int duty_ns, int period_ns){
+
+	struct xlnx_pwm_chip *pc;
+
+	pc = container_of(chip, struct xlnx_pwm_chip, chip);
+
+	iowrite32(duty_ns/pc->scaler - 2, pc->mmio_base + DUTY);
+	iowrite32(period_ns/pc->scaler - 2, pc->mmio_base + PERIOD);
+
+	return 0;
+}
+
+static int xlnx_pwm_enable(struct pwm_chip *chip,
+			      struct pwm_device *pwm){
+
+	struct xlnx_pwm_chip *pc;
+
+	pc = container_of(chip, struct xlnx_pwm_chip, chip);
+
+	iowrite32(ioread32(pc->mmio_base) | PWM_ENABLE, pc->mmio_base);
+	iowrite32(ioread32(pc->mmio_base + OFFSET) | PWM_ENABLE,
+						pc->mmio_base + OFFSET);
+	return 0;
+}
+
+static void xlnx_pwm_disable(struct pwm_chip *chip,
+				struct pwm_device *pwm)
+{
+	struct xlnx_pwm_chip *pc;
+
+	pc = to_xlnx_pwm_chip(chip);
+
+	iowrite32(ioread32(pc->mmio_base) & ~PWM_ENABLE, pc->mmio_base);
+	iowrite32(ioread32(pc->mmio_base + OFFSET) & ~PWM_ENABLE,
+						pc->mmio_base + OFFSET);
+}
+
+static int xlnx_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
+				enum pwm_polarity polarity)
+{
+	struct xlnx_pwm_chip *pc;
+
+	pc = to_xlnx_pwm_chip(chip);
+
+	/* no implementation of polarity function
+	   the axi timer hw block doesn't support this
+	*/
+
+	return 0;
+}
+
+static const struct pwm_ops xlnx_pwm_ops = {
+	.config = xlnx_pwm_config,
+	.enable = xlnx_pwm_enable,
+	.disable = xlnx_pwm_disable,
+	.set_polarity = xlnx_set_polarity,
+	.owner = THIS_MODULE,
+};
+
+static int xlnx_pwm_probe(struct platform_device *pdev)
+{
+	struct xlnx_pwm_chip *pwm;
+
+	int ret;
+	struct resource *r;
+	u32 start, end;
+	struct clk *clk;
+
+	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
+	if (!pwm) {
+		dev_err(&pdev->dev, "failed to allocate memory\n");
+		return -ENOMEM;
+	}
+
+	pwm->dev = &pdev->dev;
+
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk)) {
+		dev_err(&pdev->dev, "could not find clk: %ld\n", PTR_ERR(clk));
+		devm_kfree(&pdev->dev, pwm);
+		return PTR_ERR(clk);
+	}
+
+	pwm->scaler = (int)1000000000/clk_get_rate(clk);
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	pwm->mmio_base = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(pwm->mmio_base)) {
+		devm_kfree(&pdev->dev, pwm);
+		return PTR_ERR(pwm->mmio_base);
+	}
+
+	start = r->start;
+	end = r->end;
+
+	pwm->chip.dev = &pdev->dev;
+	pwm->chip.ops = &xlnx_pwm_ops;
+	pwm->chip.base = (int)&pdev->id;
+	pwm->chip.npwm = 1;
+
+	ret = pwmchip_add(&pwm->chip);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
+		devm_kfree(&pdev->dev, pwm);
+		return -1;
+	}
+
+	/*set the pwm0 configuration*/
+	iowrite32(PWM_CONF, pwm->mmio_base);
+	iowrite32(PWM_CONF, pwm->mmio_base + OFFSET);
+
+	platform_set_drvdata(pdev, pwm);
+
+	return 0;
+}
+
+static int xlnx_pwm_remove(struct platform_device *pdev)
+{
+
+	struct xlnx_pwm_chip *pc;
+	pc  = platform_get_drvdata(pdev);
+
+	if (WARN_ON(!pc))
+		return -ENODEV;
+
+	return pwmchip_remove(&pc->chip);
+}
+
+static const struct of_device_id xlnx_pwm_of_match[] = {
+	{ .compatible = "xlnx,pwm-xlnx", },
+	{ /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, xlnx_pwm_of_match);
+
+static struct platform_driver xlnx_pwm_driver = {
+	.driver = {
+		.name = "pwm-xlnx",
+		.owner = THIS_MODULE,
+		.of_match_table = xlnx_pwm_of_match,
+	},
+	.probe = xlnx_pwm_probe,
+	.remove = xlnx_pwm_remove,
+};
+module_platform_driver(xlnx_pwm_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);