[PATCH v2 00/10] Support for AM33xx PWM Susbsytem

[PATCH v2 00/10] Support for AM33xx PWM Susbsytem

[Philip, Avinash]

In AM33xx PWM sub modules like ECAP, EHRPWM & EQEP are  integrated to
PWM subsystem. All these submodules shares the resources (clock) & has
a clock gating register in PWM Subsystem. This patch series creates a
parent PWM Subsystem driver to handle access synchronization of shared
resources & clock gating from PWM Subsystem configuration space.
Also Device tree nodes populated to support parent child relation
between PWMSS, ECAP & EHRPWM submodules.
In addition EHRPWM module requires explicit clock gating from control
module & is handled by patch #2 & 8.

Patch #4 & 5 submitted is a second revision as suggested by  Thierry
for handling clock gating with a global function . This requires config
space handling done independent from driver and is done at parent driver.
So the parent<->child relation adopted to handle
1. pm runtime synchronization
2. PWM subsystem common config space clock gating for PWM submodules.

Patches supports
- Driver support for parent child relation handled patch #1
- Optional EHRPWM tb clock in patch #2
- Parent child in HWMOD handled at patch #3
- Device tree binding support handled in patch #4, 6 &8
- pinctrl support in patch #5 & 7.
- DT node populated in patch #9 & 10.

This patch series based on linux-next/20121031 and tested on AM33xx.
It depends on [1]

1. https://lkml.org/lkml/2012/10/29/589
pwm: Device tree support for PWM polarity

Philip, Avinash (10):
  PWMSS: Add PWM Subsystem driver for parent<->child relationship
  ARM: am33xx: clk: Add optional clock for EHRPWM
  ARM: OMAP: AM33xx hwmod: Add parent-child relationship for PWM
    subsystem
  pwm: pwm-tiecap: Add device-tree binding support for APWM driver
  pwm: pwm-tiecap: pinctrl support
  pwm: pwm-tiehrpwm: Add device-tree binding support for EHRPWM driver
  pwm: pwm-tiehrpwm: pinctrl support
  pwm: pwm-tiehrpwm: Adding TBCLK gating support.
  ARM: dts: AM33XX: Add PWMSS device tree nodes
  ARM: dts: AM33XX: Add PWM backlight DT data to am335x-evm

 .../devicetree/bindings/pwm/pwm-tiecap.txt         |   22 +
 .../devicetree/bindings/pwm/pwm-tiehrpwm.txt       |   25 ++
 Documentation/devicetree/bindings/pwm/tipwmss.txt  |   30 ++
 arch/arm/boot/dts/am335x-evm.dts                   |   21 +
 arch/arm/boot/dts/am33xx.dtsi                      |   90 +++++
 arch/arm/mach-omap2/clock33xx_data.c               |   37 ++
 arch/arm/mach-omap2/control.h                      |    8 +
 arch/arm/mach-omap2/omap_hwmod_33xx_data.c         |  419 +++++++++++++-------
 drivers/pwm/Kconfig                                |   11 +
 drivers/pwm/Makefile                               |    1 +
 drivers/pwm/pwm-tiecap.c                           |   54 +++-
 drivers/pwm/pwm-tiehrpwm.c                         |   75 ++++-
 drivers/pwm/tipwmss.c                              |  142 +++++++
 drivers/pwm/tipwmss.h                              |   30 ++
 14 files changed, 820 insertions(+), 145 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
 create mode 100644 Documentation/devicetree/bindings/pwm/tipwmss.txt
 create mode 100644 drivers/pwm/tipwmss.c
 create mode 100644 drivers/pwm/tipwmss.h

[PATCH v2 04/10] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

[Philip, Avinash]

This patch
1. Add support for device-tree binding for ECAP APWM driver.
2. Set size of pwm-cells set to 3 to support PWM channel number, PWM
   period & polarity configuration from device tree.
3. Add enable/disable clock gating in PWM subsystem common config space.
4. When here set .owner member in platform_driver structure to
   THIS_MODULE.

Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
Cc:	Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Rob Landley <rob@landley.net>
---
Changes since v1:
	- Add separate patch for pinctrl support
	- Add conditional check for PWM subsystem clock enable.
	- Combined with HWMOD changes & DT bindings.
	- Remove the custom of xlate support.

:000000 100644 0000000... fe24cac... A	Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
:100644 100644 d6d4cf0... 0d43266... M	drivers/pwm/pwm-tiecap.c
 .../devicetree/bindings/pwm/pwm-tiecap.txt         |   22 +++++++++
 drivers/pwm/pwm-tiecap.c                           |   48 +++++++++++++++++++-
 2 files changed, 69 insertions(+), 1 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt b/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
new file mode 100644
index 0000000..fe24cac
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
@@ -0,0 +1,22 @@
+TI SOC ECAP based APWM controller
+
+Required properties:
+- compatible: Must be "ti,am33xx-ecap"
+- #pwm-cells: Should be 3. Number of cells being used to specify PWM property.
+  First cell specifies the per-chip index of the PWM to use, the second
+  cell is the period cycle in nanoseconds and bit 0 in the third cell is
+  used to encode the polarity of PWM output.
+- reg: physical base address and size of the registers map.
+
+Optional properties:
+- ti,hwmods: Name of the hwmod associated to the ECAP:
+  "ecap<x>", <x> being the 0-based instance number from the HW spec
+
+Example:
+
+ecap0: ecap@0 {
+	compatible = "ti,am33xx-ecap";
+	#pwm-cells = <3>;
+	reg = <0x48300100 0x80>;
+	ti,hwmods = "ecap0";
+};
diff --git a/drivers/pwm/pwm-tiecap.c b/drivers/pwm/pwm-tiecap.c
index d6d4cf0..0d43266 100644
--- a/drivers/pwm/pwm-tiecap.c
+++ b/drivers/pwm/pwm-tiecap.c
@@ -25,6 +25,9 @@
 #include <linux/clk.h>
 #include <linux/pm_runtime.h>
 #include <linux/pwm.h>
+#include <linux/of_device.h>
+
+#include "tipwmss.h"
 
 /* ECAP registers and bits definitions */
 #define CAP1			0x08
@@ -37,6 +40,13 @@
 #define ECCTL2_SYNC_SEL_DISA	(BIT(7) | BIT(6))
 #define ECCTL2_TSCTR_FREERUN	BIT(4)
 
+#define ECAPCLK_EN			BIT(0)
+#define ECAPCLK_STOP_REQ	BIT(1)
+
+#define ECAPCLK_EN_ACK		BIT(0)
+
+#define PWM_CELL_SIZE		3
+
 struct ecap_pwm_chip {
 	struct pwm_chip	chip;
 	unsigned int	clk_rate;
@@ -184,6 +194,16 @@ static const struct pwm_ops ecap_pwm_ops = {
 	.owner		= THIS_MODULE,
 };
 
+#ifdef CONFIG_OF
+static const struct of_device_id ecap_of_match[] = {
+	{
+		.compatible	= "ti,am33xx-ecap",
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, ecap_of_match);
+#endif
+
 static int __devinit ecap_pwm_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -211,6 +231,7 @@ static int __devinit ecap_pwm_probe(struct platform_device *pdev)
 
 	pc->chip.dev = &pdev->dev;
 	pc->chip.ops = &ecap_pwm_ops;
+	pc->chip.of_pwm_n_cells = PWM_CELL_SIZE;
 	pc->chip.base = -1;
 	pc->chip.npwm = 1;
 
@@ -231,14 +252,37 @@ static int __devinit ecap_pwm_probe(struct platform_device *pdev)
 	}
 
 	pm_runtime_enable(&pdev->dev);
+	pm_runtime_get_sync(&pdev->dev);
+	if (!(pwmss_submodule_state_change(pdev->dev.parent, ECAPCLK_EN) &
+				ECAPCLK_EN_ACK)) {
+		dev_err(&pdev->dev, "PWMSS config space clock enable failure\n");
+		ret = -EINVAL;
+		goto pwmss_clk_failure;
+	}
+	pm_runtime_put_sync(&pdev->dev);
+
 	platform_set_drvdata(pdev, pc);
 	return 0;
+
+pwmss_clk_failure:
+	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	pwmchip_remove(&pc->chip);
+	return ret;
 }
 
 static int __devexit ecap_pwm_remove(struct platform_device *pdev)
 {
 	struct ecap_pwm_chip *pc = platform_get_drvdata(pdev);
 
+	pm_runtime_get_sync(&pdev->dev);
+	/*
+	 * Due to hardware misbehaviour, acknowledge of the stop_req
+	 * is missing. Hence checking of the status bit skipped.
+	 */
+	pwmss_submodule_state_change(pdev->dev.parent, ECAPCLK_STOP_REQ);
+	pm_runtime_put_sync(&pdev->dev);
+
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	return pwmchip_remove(&pc->chip);
@@ -246,7 +290,9 @@ static int __devexit ecap_pwm_remove(struct platform_device *pdev)
 
 static struct platform_driver ecap_pwm_driver = {
 	.driver = {
-		.name = "ecap",
+		.name	= "ecap",
+		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(ecap_of_match),
 	},
 	.probe = ecap_pwm_probe,
 	.remove = __devexit_p(ecap_pwm_remove),
-- 
1.7.0.4

Re: [PATCH v2 04/10] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6341 bytes --]

On Thu, Nov 08, 2012 at 01:23:11PM +0530, Philip, Avinash wrote:
> This patch
> 1. Add support for device-tree binding for ECAP APWM driver.
> 2. Set size of pwm-cells set to 3 to support PWM channel number, PWM
>    period & polarity configuration from device tree.
> 3. Add enable/disable clock gating in PWM subsystem common config space.
> 4. When here set .owner member in platform_driver structure to
>    THIS_MODULE.
> 
> Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> Cc:	Grant Likely <grant.likely@secretlab.ca>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Rob Landley <rob@landley.net>
> ---
> Changes since v1:
> 	- Add separate patch for pinctrl support
> 	- Add conditional check for PWM subsystem clock enable.
> 	- Combined with HWMOD changes & DT bindings.
> 	- Remove the custom of xlate support.
> 
> :000000 100644 0000000... fe24cac... A	Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
> :100644 100644 d6d4cf0... 0d43266... M	drivers/pwm/pwm-tiecap.c
>  .../devicetree/bindings/pwm/pwm-tiecap.txt         |   22 +++++++++
>  drivers/pwm/pwm-tiecap.c                           |   48 +++++++++++++++++++-
>  2 files changed, 69 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt b/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
> new file mode 100644
> index 0000000..fe24cac
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
> @@ -0,0 +1,22 @@
> +TI SOC ECAP based APWM controller
> +
> +Required properties:
> +- compatible: Must be "ti,am33xx-ecap"
> +- #pwm-cells: Should be 3. Number of cells being used to specify PWM property.
> +  First cell specifies the per-chip index of the PWM to use, the second
> +  cell is the period cycle in nanoseconds and bit 0 in the third cell is

I think this should be "period in nanoseconds". I haven't heard "period
cycle" before.

> +  used to encode the polarity of PWM output.

Maybe you should explicitly say how this is encoded.

> +- reg: physical base address and size of the registers map.
> +
> +Optional properties:
> +- ti,hwmods: Name of the hwmod associated to the ECAP:
> +  "ecap<x>", <x> being the 0-based instance number from the HW spec
> +
> +Example:
> +
> +ecap0: ecap@0 {
> +	compatible = "ti,am33xx-ecap";
> +	#pwm-cells = <3>;
> +	reg = <0x48300100 0x80>;
> +	ti,hwmods = "ecap0";
> +};
> diff --git a/drivers/pwm/pwm-tiecap.c b/drivers/pwm/pwm-tiecap.c
> index d6d4cf0..0d43266 100644
> --- a/drivers/pwm/pwm-tiecap.c
> +++ b/drivers/pwm/pwm-tiecap.c
> @@ -25,6 +25,9 @@
>  #include <linux/clk.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pwm.h>
> +#include <linux/of_device.h>
> +
> +#include "tipwmss.h"
>  
>  /* ECAP registers and bits definitions */
>  #define CAP1			0x08
> @@ -37,6 +40,13 @@
>  #define ECCTL2_SYNC_SEL_DISA	(BIT(7) | BIT(6))
>  #define ECCTL2_TSCTR_FREERUN	BIT(4)
>  
> +#define ECAPCLK_EN			BIT(0)
> +#define ECAPCLK_STOP_REQ	BIT(1)

This one doesn't seem to align with the rest. Also, why is bit 0 called
_EN and bit 1 _STOP_REQ? Couldn't they be made more consistent, i.e.
_START and _STOP? Or _ENABLE and _DISABLE?

> +
> +#define ECAPCLK_EN_ACK		BIT(0)
> +
> +#define PWM_CELL_SIZE		3

You don't need a define for this.

> +
>  struct ecap_pwm_chip {
>  	struct pwm_chip	chip;
>  	unsigned int	clk_rate;
> @@ -184,6 +194,16 @@ static const struct pwm_ops ecap_pwm_ops = {
>  	.owner		= THIS_MODULE,
>  };
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id ecap_of_match[] = {
> +	{
> +		.compatible	= "ti,am33xx-ecap",
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ecap_of_match);
> +#endif
> +

I'm not sure if I remember correctly, but wasn't AM33xx support supposed
to be DT only? In that case you don't need the CONFIG_OF guards.

>  static int __devinit ecap_pwm_probe(struct platform_device *pdev)

__devinit can go away.

>  {
>  	int ret;
> @@ -211,6 +231,7 @@ static int __devinit ecap_pwm_probe(struct platform_device *pdev)
>  
>  	pc->chip.dev = &pdev->dev;
>  	pc->chip.ops = &ecap_pwm_ops;
> +	pc->chip.of_pwm_n_cells = PWM_CELL_SIZE;
>  	pc->chip.base = -1;
>  	pc->chip.npwm = 1;
>  
> @@ -231,14 +252,37 @@ static int __devinit ecap_pwm_probe(struct platform_device *pdev)
>  	}
>  
>  	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_get_sync(&pdev->dev);

Maybe put a blank line after this for readability.

> +	if (!(pwmss_submodule_state_change(pdev->dev.parent, ECAPCLK_EN) &
> +				ECAPCLK_EN_ACK)) {

This is very hard to read, can you split this up into something like the
following please?

	status = pwmss_submodule_state_change(pdev->dev.parent, ECAPCLK_EN);
	if (!(status & ECAPCLK_EN_ACK)) {
		...
	}

> +		dev_err(&pdev->dev, "PWMSS config space clock enable failure\n");
> +		ret = -EINVAL;
> +		goto pwmss_clk_failure;
> +	}
> +	pm_runtime_put_sync(&pdev->dev);

Another blank line between the two above would be good.

> +
>  	platform_set_drvdata(pdev, pc);
>  	return 0;
> +
> +pwmss_clk_failure:
> +	pm_runtime_put_sync(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +	pwmchip_remove(&pc->chip);
> +	return ret;
>  }
>  
>  static int __devexit ecap_pwm_remove(struct platform_device *pdev)

No __devexit please.

>  {
>  	struct ecap_pwm_chip *pc = platform_get_drvdata(pdev);
>  
> +	pm_runtime_get_sync(&pdev->dev);
> +	/*
> +	 * Due to hardware misbehaviour, acknowledge of the stop_req
> +	 * is missing. Hence checking of the status bit skipped.
> +	 */
> +	pwmss_submodule_state_change(pdev->dev.parent, ECAPCLK_STOP_REQ);
> +	pm_runtime_put_sync(&pdev->dev);
> +
>  	pm_runtime_put_sync(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  	return pwmchip_remove(&pc->chip);
> @@ -246,7 +290,9 @@ static int __devexit ecap_pwm_remove(struct platform_device *pdev)
>  
>  static struct platform_driver ecap_pwm_driver = {
>  	.driver = {
> -		.name = "ecap",
> +		.name	= "ecap",
> +		.owner	= THIS_MODULE,
> +		.of_match_table = of_match_ptr(ecap_of_match),

Here as well, if AM33xx is DT-only, then the of_match_ptr() can be
dropped.

>  	},
>  	.probe = ecap_pwm_probe,
>  	.remove = __devexit_p(ecap_pwm_remove),

No __devexit_p() please.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 04/10] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 618 bytes --]

On Fri, Nov 09, 2012 at 08:52:19AM +0100, Thierry Reding wrote:
> On Thu, Nov 08, 2012 at 01:23:11PM +0530, Philip, Avinash wrote:
[...]
> >  static int __devinit ecap_pwm_probe(struct platform_device *pdev)
> 
> __devinit can go away.
[...]
> >  static int __devexit ecap_pwm_remove(struct platform_device *pdev)
> 
> No __devexit please.
[...]
> >  	.remove = __devexit_p(ecap_pwm_remove),
> 
> No __devexit_p() please.

Okay, ignore these comments. The annotation were not added in this patch
so they can be removed in a separate patch. Or when Greg finally removes
all traces of them.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH v2 04/10] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

[Philip, Avinash]

On Fri, Nov 09, 2012 at 13:22:19, Thierry Reding wrote:
> On Thu, Nov 08, 2012 at 01:23:11PM +0530, Philip, Avinash wrote:
> > This patch
> > 1. Add support for device-tree binding for ECAP APWM driver.
> > 2. Set size of pwm-cells set to 3 to support PWM channel number, PWM
> >    period & polarity configuration from device tree.
> > 3. Add enable/disable clock gating in PWM subsystem common config space.
> > 4. When here set .owner member in platform_driver structure to
> >    THIS_MODULE.
> > 
> > Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> > Cc:	Grant Likely <grant.likely@secretlab.ca>
> > Cc: Rob Herring <rob.herring@calxeda.com>
> > Cc: Rob Landley <rob@landley.net>
> > ---
> > Changes since v1:
> > 	- Add separate patch for pinctrl support
> > 	- Add conditional check for PWM subsystem clock enable.
> > 	- Combined with HWMOD changes & DT bindings.
> > 	- Remove the custom of xlate support.
> > 
> > :000000 100644 0000000... fe24cac... A	Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
> > :100644 100644 d6d4cf0... 0d43266... M	drivers/pwm/pwm-tiecap.c
> >  .../devicetree/bindings/pwm/pwm-tiecap.txt         |   22 +++++++++
> >  drivers/pwm/pwm-tiecap.c                           |   48 +++++++++++++++++++-
> >  2 files changed, 69 insertions(+), 1 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt b/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
> > new file mode 100644
> > index 0000000..fe24cac
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
> > @@ -0,0 +1,22 @@
> > +TI SOC ECAP based APWM controller
> > +
> > +Required properties:
> > +- compatible: Must be "ti,am33xx-ecap"
> > +- #pwm-cells: Should be 3. Number of cells being used to specify PWM property.
> > +  First cell specifies the per-chip index of the PWM to use, the second
> > +  cell is the period cycle in nanoseconds and bit 0 in the third cell is
> 
> I think this should be "period in nanoseconds". I haven't heard "period
> cycle" before.

Ok

> 
> > +  used to encode the polarity of PWM output.
> 
> Maybe you should explicitly say how this is encoded.

Ok I will add details

> 
...
> >  
> > +#define ECAPCLK_EN			BIT(0)
> > +#define ECAPCLK_STOP_REQ	BIT(1)
> 
> This one doesn't seem to align with the rest. Also, why is bit 0 called
> _EN and bit 1 _STOP_REQ? Couldn't they be made more consistent, i.e.
> _START and _STOP? Or _ENABLE and _DISABLE?

Ok I will change to PWMSS_ECAPCLK_EN & PWMSS_ECAPCLK_STPO_REQ

> 
> > +
> > +#define ECAPCLK_EN_ACK		BIT(0)
> > +
> > +#define PWM_CELL_SIZE		3
> 
> You don't need a define for this.

I remove.

> 
> > +
> >  struct ecap_pwm_chip {
> >  	struct pwm_chip	chip;
> >  	unsigned int	clk_rate;
> > @@ -184,6 +194,16 @@ static const struct pwm_ops ecap_pwm_ops = {
> >  	.owner		= THIS_MODULE,
> >  };
> >  
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id ecap_of_match[] = {
> > +	{
> > +		.compatible	= "ti,am33xx-ecap",
> > +	},
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, ecap_of_match);
> > +#endif
> > +
> 
> I'm not sure if I remember correctly, but wasn't AM33xx support supposed
> to be DT only? In that case you don't need the CONFIG_OF guards.

I will remove

> 
...
> >  	pm_runtime_enable(&pdev->dev);
> > +	pm_runtime_get_sync(&pdev->dev);
> 
> Maybe put a blank line after this for readability.

Ok

> 
> > +	if (!(pwmss_submodule_state_change(pdev->dev.parent, ECAPCLK_EN) &
> > +				ECAPCLK_EN_ACK)) {
> 
> This is very hard to read, can you split this up into something like the
> following please?
> 
> 	status = pwmss_submodule_state_change(pdev->dev.parent, ECAPCLK_EN);
> 	if (!(status & ECAPCLK_EN_ACK)) {
> 		...
> 	}
> 

Ok I will correct it.

> > +		dev_err(&pdev->dev, "PWMSS config space clock enable failure\n");
> > +		ret = -EINVAL;
> > +		goto pwmss_clk_failure;
> > +	}
> > +	pm_runtime_put_sync(&pdev->dev);
> 
> Another blank line between the two above would be good.

Ok

> 
...
> > +		.owner	= THIS_MODULE,
> > +		.of_match_table = of_match_ptr(ecap_of_match),
> 
> Here as well, if AM33xx is DT-only, then the of_match_ptr() can be
> dropped.

Ok I will drop.

Thanks
Avinash
> 
> >  	},
> >  	.probe = ecap_pwm_probe,
> >  	.remove = __devexit_p(ecap_pwm_remove),
> 
> No __devexit_p() please.
> 
> Thierry
> 

Re: [PATCH v2 04/10] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 705 bytes --]

On Fri, Nov 09, 2012 at 10:59:30AM +0000, Philip, Avinash wrote:
> On Fri, Nov 09, 2012 at 13:22:19, Thierry Reding wrote:
> > On Thu, Nov 08, 2012 at 01:23:11PM +0530, Philip, Avinash wrote:
> > > +#define ECAPCLK_EN			BIT(0)
> > > +#define ECAPCLK_STOP_REQ	BIT(1)
> > 
> > This one doesn't seem to align with the rest. Also, why is bit 0 called
> > _EN and bit 1 _STOP_REQ? Couldn't they be made more consistent, i.e.
> > _START and _STOP? Or _ENABLE and _DISABLE?
> 
> Ok I will change to PWMSS_ECAPCLK_EN & PWMSS_ECAPCLK_STPO_REQ

While at it, maybe move these defines to the pwm-tipwmss.h header file?
After all that's the module that accesses the register that contains
these bits.


[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v2 05/10] pwm: pwm-tiecap: pinctrl support

[Philip, Avinash]

Enable pinctrl for pwm-tiecap

Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
---
:100644 100644 0d43266... 6071f7a... M	drivers/pwm/pwm-tiecap.c
 drivers/pwm/pwm-tiecap.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/pwm/pwm-tiecap.c b/drivers/pwm/pwm-tiecap.c
index 0d43266..6071f7a 100644
--- a/drivers/pwm/pwm-tiecap.c
+++ b/drivers/pwm/pwm-tiecap.c
@@ -26,6 +26,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/pwm.h>
 #include <linux/of_device.h>
+#include <linux/pinctrl/consumer.h>
 
 #include "tipwmss.h"
 
@@ -210,6 +211,11 @@ static int __devinit ecap_pwm_probe(struct platform_device *pdev)
 	struct resource *r;
 	struct clk *clk;
 	struct ecap_pwm_chip *pc;
+	struct pinctrl *pinctrl;
+
+	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+	if (IS_ERR(pinctrl))
+		dev_warn(&pdev->dev, "failed to configure pins from driver\n");
 
 	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
 	if (!pc) {
-- 
1.7.0.4

[PATCH v2 06/10] pwm: pwm-tiehrpwm: Add device-tree binding support for EHRPWM driver

[Philip, Avinash]

This patch
1. Add support for device-tree binding for EHRWPM driver.
2. Set size of pwm-cells set to 3 to support PWM channel number, PWM
   period & polarity configuration from device tree.
3. Add enable/disable clock gating in PWM subsystem common config space.
4. When here set .owner member in platform_driver structure to
   THIS_MODULE.

Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
Cc:	Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Rob Landley <rob@landley.net>
---
Changes since v1:
	- Add separate patch for pinctrl support
	- Add conditional check for PWM subsystem clock enable.
	- Combined with HWMOD changes & DT bindings.
	- Remove the custom of xlate support.

:000000 100644 0000000... aa2ed0a... A	Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
:100644 100644 d3c1dff... fba7f9b... M	drivers/pwm/pwm-tiehrpwm.c
 .../devicetree/bindings/pwm/pwm-tiehrpwm.txt       |   25 ++++++++++
 drivers/pwm/pwm-tiehrpwm.c                         |   49 +++++++++++++++++++-
 2 files changed, 72 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
new file mode 100644
index 0000000..aa2ed0a
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
@@ -0,0 +1,25 @@
+TI SOC EHRPWM based PWM controller
+
+Required properties:
+- compatible : Must be "ti,am33xx-ehrpwm"
+- #pwm-cells: Should be 3. Number of cells being used to specify PWM property.
+  First cell specifies the per-chip index of the PWM to use, the second
+  cell is the period cycle in nanoseconds and bit 0 in the third cell is
+  used to encode the polarity of PWM output.
+- reg: physical base address and size of the registers map.
+
+Optional properties:
+- ti,hwmods: Name of the hwmod associated to the EHRPWM:
+  "ehrpwm<x>", <x> being the 0-based instance number from the HW spec
+- tbclkgating: platforms require tbclk gating from control module
+  should populate
+
+Example:
+
+ehrpwm0: ehrpwm@0 {
+	compatible = "ti,am33xx-ehrpwm";
+	#pwm-cells = <3>;
+	reg = <0x48300200 0x100>;
+	ti,hwmods = "ehrpwm0";
+	tbclkgating;
+};
diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index d3c1dff..fba7f9b 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -25,6 +25,9 @@
 #include <linux/err.h>
 #include <linux/clk.h>
 #include <linux/pm_runtime.h>
+#include <linux/of_device.h>
+
+#include "tipwmss.h"
 
 /* EHRPWM registers and bits definitions */
 
@@ -107,6 +110,13 @@
 #define AQCSFRC_CSFA_FRCHIGH	BIT(1)
 #define AQCSFRC_CSFA_DISSWFRC	(BIT(1) | BIT(0))
 
+#define EPWMCLK_EN			BIT(8)
+#define EPWMCLK_STOP_REQ	BIT(9)
+
+#define EPWMCLK_EN_ACK		BIT(8)
+
+#define PWM_CELL_SIZE		3
+
 #define NUM_PWM_CHANNEL		2	/* EHRPWM channels */
 
 struct ehrpwm_pwm_chip {
@@ -392,6 +402,16 @@ static const struct pwm_ops ehrpwm_pwm_ops = {
 	.owner		= THIS_MODULE,
 };
 
+#ifdef CONFIG_OF
+static const struct of_device_id ehrpwm_of_match[] = {
+	{
+		.compatible	= "ti,am33xx-ehrpwm",
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, ehrpwm_of_match);
+#endif
+
 static int __devinit ehrpwm_pwm_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -419,6 +439,7 @@ static int __devinit ehrpwm_pwm_probe(struct platform_device *pdev)
 
 	pc->chip.dev = &pdev->dev;
 	pc->chip.ops = &ehrpwm_pwm_ops;
+	pc->chip.of_pwm_n_cells = PWM_CELL_SIZE;
 	pc->chip.base = -1;
 	pc->chip.npwm = NUM_PWM_CHANNEL;
 
@@ -437,16 +458,38 @@ static int __devinit ehrpwm_pwm_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
 		return ret;
 	}
-
 	pm_runtime_enable(&pdev->dev);
+	pm_runtime_get_sync(&pdev->dev);
+	if (!(pwmss_submodule_state_change(pdev->dev.parent, EPWMCLK_EN) &
+				EPWMCLK_EN_ACK)) {
+		dev_err(&pdev->dev, "PWMSS config space clock enable failure\n");
+		ret = -EINVAL;
+		goto pwmss_clk_failure;
+	}
+	pm_runtime_put_sync(&pdev->dev);
+
 	platform_set_drvdata(pdev, pc);
 	return 0;
+
+pwmss_clk_failure:
+	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	pwmchip_remove(&pc->chip);
+	return ret;
 }
 
 static int __devexit ehrpwm_pwm_remove(struct platform_device *pdev)
 {
 	struct ehrpwm_pwm_chip *pc = platform_get_drvdata(pdev);
 
+	pm_runtime_get_sync(&pdev->dev);
+	/*
+	 * Due to hardware misbehaviour, acknowledge of the stop_req
+	 * is missing. Hence checking of the status bit skipped.
+	 */
+	pwmss_submodule_state_change(pdev->dev.parent, EPWMCLK_STOP_REQ);
+	pm_runtime_put_sync(&pdev->dev);
+
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	return pwmchip_remove(&pc->chip);
@@ -454,7 +497,9 @@ static int __devexit ehrpwm_pwm_remove(struct platform_device *pdev)
 
 static struct platform_driver ehrpwm_pwm_driver = {
 	.driver = {
-		.name = "ehrpwm",
+		.name	= "ehrpwm",
+		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(ehrpwm_of_match),
 	},
 	.probe = ehrpwm_pwm_probe,
 	.remove = __devexit_p(ehrpwm_pwm_remove),
-- 
1.7.0.4

Re: [PATCH v2 06/10] pwm: pwm-tiehrpwm: Add device-tree binding support for EHRPWM driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1299 bytes --]

On Thu, Nov 08, 2012 at 01:23:13PM +0530, Philip, Avinash wrote:
> This patch
> 1. Add support for device-tree binding for EHRWPM driver.
> 2. Set size of pwm-cells set to 3 to support PWM channel number, PWM
>    period & polarity configuration from device tree.
> 3. Add enable/disable clock gating in PWM subsystem common config space.
> 4. When here set .owner member in platform_driver structure to
>    THIS_MODULE.
> 
> Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> Cc:	Grant Likely <grant.likely@secretlab.ca>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Rob Landley <rob@landley.net>
> ---
> Changes since v1:
> 	- Add separate patch for pinctrl support
> 	- Add conditional check for PWM subsystem clock enable.
> 	- Combined with HWMOD changes & DT bindings.
> 	- Remove the custom of xlate support.
> 
> :000000 100644 0000000... aa2ed0a... A	Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
> :100644 100644 d3c1dff... fba7f9b... M	drivers/pwm/pwm-tiehrpwm.c
>  .../devicetree/bindings/pwm/pwm-tiehrpwm.txt       |   25 ++++++++++
>  drivers/pwm/pwm-tiehrpwm.c                         |   49 +++++++++++++++++++-
>  2 files changed, 72 insertions(+), 2 deletions(-)

The same comments apply as for the pwm-tiecap driver patch.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v2 07/10] pwm: pwm-tiehrpwm: pinctrl support

[Philip, Avinash]

Enable pinctrl for pwm-tiehrpwm

Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
---
:100644 100644 fba7f9b... 07911e6... M	drivers/pwm/pwm-tiehrpwm.c
 drivers/pwm/pwm-tiehrpwm.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index fba7f9b..07911e6 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -26,6 +26,7 @@
 #include <linux/clk.h>
 #include <linux/pm_runtime.h>
 #include <linux/of_device.h>
+#include <linux/pinctrl/consumer.h>
 
 #include "tipwmss.h"
 
@@ -418,6 +419,11 @@ static int __devinit ehrpwm_pwm_probe(struct platform_device *pdev)
 	struct resource *r;
 	struct clk *clk;
 	struct ehrpwm_pwm_chip *pc;
+	struct pinctrl *pinctrl;
+
+	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+	if (IS_ERR(pinctrl))
+		dev_warn(&pdev->dev, "failed to configure pins from driver\n");
 
 	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
 	if (!pc) {
-- 
1.7.0.4

[PATCH v2 01/10] PWMSS: Add PWM Subsystem driver for parent<->child relationship

[Philip, Avinash]

In some platforms (like am33xx), PWM sub modules (ECAP, EHRPWM, EQEP)
are integrated to PWM subsystem. These PWM submodules has resources
shared and only one register bit-field is provided to control
module/clock enable/disable, makes it difficult to handle common
resources from independent PWMSS submodule drivers.

So the solution here implemented in this patch is, to create driver for
PWMSS and take the role of parent driver for PWM submodules. PWMSS
parent driver enumerates all the child nodes under PWMSS module. Also
symbol "pwmss_submodule_state_change" exported to enable clock gating
for individual PWMSS submodules, and submodule drivers has to enable
clock gating from their drivers.

As this is only supported during DT boot, the parent<->child relationship
is created and populated in DT execution flow. The only required change
is inside DTS file, making EHRPWM & ECAP as a child to PWMSS node.

Signed-off-by: Philip, Avinash <avinashphilip-l0cyMroinI0@public.gmane.org>
---
Changes since v1:
	- Add conditional check for PWM susbsystem clock enabling.
	- Add context save/restore for PWM subsystem clock config register.

:000000 100644 0000000... b6c2814... A	Documentation/devicetree/bindings/pwm/tipwmss.txt
:100644 100644 6e556c7... 384a346... M	drivers/pwm/Kconfig
:100644 100644 3b3f4c9a.. 55f6fb2... M	drivers/pwm/Makefile
:000000 100644 0000000... c188348... A	drivers/pwm/tipwmss.c
:000000 100644 0000000... f9cb2e2... A	drivers/pwm/tipwmss.h
 Documentation/devicetree/bindings/pwm/tipwmss.txt |   30 +++++
 drivers/pwm/Kconfig                               |   11 ++
 drivers/pwm/Makefile                              |    1 +
 drivers/pwm/tipwmss.c                             |  142 +++++++++++++++++++++
 drivers/pwm/tipwmss.h                             |   30 +++++
 5 files changed, 214 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/tipwmss.txt b/Documentation/devicetree/bindings/pwm/tipwmss.txt
new file mode 100644
index 0000000..b6c2814
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/tipwmss.txt
@@ -0,0 +1,30 @@
+TI SOC based PWM Subsystem
+
+Required properties:
+- compatible: Must be "ti,am33xx-pwmss";
+- reg: physical base address and size of the registers map. For am33xx,
+	4 set of register maps present, PWMSS config space, ECAP register space,
+	EQEP register space, EHRPWM register space.
+- address-cells: Specify the number of u32 entries needed in child nodes.
+		 Should set to 1.
+- size-cells: specify inumber of u32 entries needed to specify child nodes size
+		in reg property. Should set to 1.
+- ranges: describes the address mapping of a memory-mapped bus. Should set to empty
+	  as parent and child address space is identical.
+
+Also child nodes should also populated under PWMSS DT node.
+Example:
+pwmss0: pwmss@48300000 {
+	compatible = "ti,am33xx-pwmss";
+	reg = <0x48300000 0x10
+		0x48300100 0x80
+		0x48300180 0x80
+		0x48300200 0x80>;
+	ti,hwmods = "epwmss0";
+	#address-cells = <1>;
+	#size-cells = <1>;
+	status = "disabled";
+	ranges;
+
+	/* child nodes go here */
+};
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 6e556c7..384a346 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -136,6 +136,7 @@ config PWM_TEGRA
 config  PWM_TIECAP
 	tristate "ECAP PWM support"
 	depends on SOC_AM33XX
+	select PWM_TIPWMSS
 	help
 	  PWM driver support for the ECAP APWM controller found on AM33XX
 	  TI SOC
@@ -146,6 +147,7 @@ config  PWM_TIECAP
 config  PWM_TIEHRPWM
 	tristate "EHRPWM PWM support"
 	depends on SOC_AM33XX
+	select PWM_TIPWMSS
 	help
 	  PWM driver support for the EHRPWM controller found on AM33XX
 	  TI SOC
@@ -153,6 +155,15 @@ config  PWM_TIEHRPWM
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-tiehrpwm.
 
+config  PWM_TIPWMSS
+	tristate "TI PWM Subsytem parent support"
+	depends on SOC_AM33XX && (PWM_TIEHRPWM || PWM_TIECAP)
+	help
+	  PWM Subsystem driver support for AM33xx SOC.
+
+	  PWM submodules require PWM config space access from submodule
+	  drivers and require common parent driver support.
+
 config PWM_TWL6030
 	tristate "TWL6030 PWM support"
 	depends on TWL4030_CORE
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 3b3f4c9..55f6fb2 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -12,5 +12,6 @@ obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
 obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
 obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
 obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
+obj-$(CONFIG_PWM_TIPWMSS)	+= tipwmss.o
 obj-$(CONFIG_PWM_TWL6030)	+= pwm-twl6030.o
 obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
diff --git a/drivers/pwm/tipwmss.c b/drivers/pwm/tipwmss.c
new file mode 100644
index 0000000..c188348
--- /dev/null
+++ b/drivers/pwm/tipwmss.c
@@ -0,0 +1,142 @@
+/*
+ * TI PWM Subsystem driver
+ *
+ * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/err.h>
+#include <linux/pm_runtime.h>
+#include <linux/of_device.h>
+
+#include "tipwmss.h"
+
+#define PWMSS_CLKCONFIG		0x8	/* Clock gaitng reg, for PWM submodules */
+#define PWMSS_CLKSTATUS		0xc	/* Clock gating status reg */
+
+struct pwmss_info {
+	void __iomem	*mmio_base;
+	struct mutex	pwmss_lock;
+	u16				pwmss_clkconfig;
+};
+
+u16 pwmss_submodule_state_change(struct device *dev, int set)
+{
+	struct pwmss_info *info = dev_get_drvdata(dev);
+	u16 val;
+
+	val = readw(info->mmio_base + PWMSS_CLKCONFIG);
+	val |= set;
+	mutex_lock(&info->pwmss_lock);
+	writew(val , info->mmio_base + PWMSS_CLKCONFIG);
+	mutex_unlock(&info->pwmss_lock);
+	return readw(info->mmio_base + PWMSS_CLKSTATUS);
+}
+EXPORT_SYMBOL(pwmss_submodule_state_change);
+
+static const struct of_device_id pwmss_of_match[] = {
+	{
+		.compatible	= "ti,am33xx-pwmss",
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, pwmss_of_match);
+
+static int __devinit pwmss_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct resource *r;
+	struct pwmss_info *info;
+	struct device_node *node = pdev->dev.of_node;
+
+	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
+	if (!info) {
+		dev_err(&pdev->dev, "failed to allocate memory\n");
+		return -ENOMEM;
+	}
+
+	mutex_init(&info->pwmss_lock);
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!r) {
+		dev_err(&pdev->dev, "no memory resource defined\n");
+		return -ENODEV;
+	}
+
+	info->mmio_base = devm_request_and_ioremap(&pdev->dev, r);
+	if (!info->mmio_base)
+		return -EADDRNOTAVAIL;
+
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_get_sync(&pdev->dev);
+	platform_set_drvdata(pdev, info);
+
+	/* Populate all the child nodes here... */
+	ret = of_platform_populate(node, NULL, NULL, &pdev->dev);
+	if (ret)
+		dev_warn(&pdev->dev, "Doesn't have any child node\n");
+
+	return ret;
+}
+
+static int __devexit pwmss_remove(struct platform_device *pdev)
+{
+	struct pwmss_info *info = platform_get_drvdata(pdev);
+
+	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	mutex_destroy(&info->pwmss_lock);
+	return 0;
+}
+
+static int pwmss_suspend(struct device *dev)
+{
+	struct pwmss_info *info = dev_get_drvdata(dev);
+
+	info->pwmss_clkconfig = readw(info->mmio_base + PWMSS_CLKCONFIG);
+	pm_runtime_put_sync(dev);
+	return 0;
+}
+
+static int pwmss_resume(struct device *dev)
+{
+	struct pwmss_info *info = dev_get_drvdata(dev);
+
+	pm_runtime_get_sync(dev);
+	writew(info->pwmss_clkconfig, info->mmio_base + PWMSS_CLKCONFIG);
+	return 0;
+}
+
+static const struct dev_pm_ops pwmss_pm_ops = {
+	.suspend	= pwmss_suspend,
+	.resume		= pwmss_resume,
+};
+
+static struct platform_driver pwmss_driver = {
+	.driver	= {
+		.name	= "pwmss",
+		.owner	= THIS_MODULE,
+		.pm	= &pwmss_pm_ops,
+		.of_match_table	= of_match_ptr(pwmss_of_match),
+	},
+	.probe	= pwmss_probe,
+	.remove	= __devexit_p(pwmss_remove),
+};
+
+module_platform_driver(pwmss_driver);
+
+MODULE_DESCRIPTION("pwmss driver");
+MODULE_AUTHOR("Texas Instruments");
+MODULE_LICENSE("GPL");
diff --git a/drivers/pwm/tipwmss.h b/drivers/pwm/tipwmss.h
new file mode 100644
index 0000000..f9cb2e2
--- /dev/null
+++ b/drivers/pwm/tipwmss.h
@@ -0,0 +1,30 @@
+/*
+ * TI PWM Subsystem driver
+ *
+ * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef __TIPWMSS_H
+#define __TIPWMSS_H
+
+#ifdef CONFIG_PWM_TIPWMSS
+extern u16 pwmss_submodule_state_change(struct device *dev, int set);
+#else
+static inline u16 pwmss_submodule_state_change(struct device *dev, int set)
+{
+	/* return success status value */
+	return 0xFFFF;
+}
+#endif
+#endif	/* __TIPWMSS_H */
-- 
1.7.0.4

Re: [PATCH v2 01/10] PWMSS: Add PWM Subsystem driver for parent<->child relationship

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 10236 bytes --]

On Thu, Nov 08, 2012 at 01:23:08PM +0530, Philip, Avinash wrote:
> diff --git a/Documentation/devicetree/bindings/pwm/tipwmss.txt b/Documentation/devicetree/bindings/pwm/tipwmss.txt
> new file mode 100644
> index 0000000..b6c2814
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/tipwmss.txt
> @@ -0,0 +1,30 @@
[...]
> +Also child nodes should also populated under PWMSS DT node.
> +Example:

Maybe put an blank line between these two lines for readability?

> +pwmss0: pwmss@48300000 {
> +	compatible = "ti,am33xx-pwmss";
> +	reg = <0x48300000 0x10
> +		0x48300100 0x80
> +		0x48300180 0x80
> +		0x48300200 0x80>;

I don't think you should list the register spaces of the children here.
From what I understand, all regions listed in the reg property are
supposed to be requested by the corresponding driver and therefore
cannot be used by any other device.

> +	ti,hwmods = "epwmss0";
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	status = "disabled";
> +	ranges;

I think to represent which memory regions go to the children, you should
put them in this ranges property, which would then look like this:

	ranges = <0x48300100 0x48300100 0x80   /* ECAP */
		  0x48300180 0x48300180 0x80   /* EQEP */
		  0x48300200 0x48300200 0x80>; /* EHRPWM */

> +
> +	/* child nodes go here */
> +};

Maybe you should actually list a full set of children here?

> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 6e556c7..384a346 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -136,6 +136,7 @@ config PWM_TEGRA
>  config  PWM_TIECAP
>  	tristate "ECAP PWM support"
>  	depends on SOC_AM33XX
> +	select PWM_TIPWMSS
>  	help
>  	  PWM driver support for the ECAP APWM controller found on AM33XX
>  	  TI SOC
> @@ -146,6 +147,7 @@ config  PWM_TIECAP
>  config  PWM_TIEHRPWM
>  	tristate "EHRPWM PWM support"
>  	depends on SOC_AM33XX
> +	select PWM_TIPWMSS
>  	help
>  	  PWM driver support for the EHRPWM controller found on AM33XX
>  	  TI SOC
> @@ -153,6 +155,15 @@ config  PWM_TIEHRPWM
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-tiehrpwm.
>  
> +config  PWM_TIPWMSS
> +	tristate "TI PWM Subsytem parent support"
> +	depends on SOC_AM33XX && (PWM_TIEHRPWM || PWM_TIECAP)

Since you select the symbol from the PWM_TIECAP and PWM_TIEHRPWM symbols
there is no need to depend on them, right? Oh, but maybe that's to make
sure the symbol is deselected automatically if neither user is selected.

Perhaps this should actually be a hidden symbol (i.e. leave away the
prompt string in the tristate option) since it's purely a dependency and
not useful of its own.

> +	help
> +	  PWM Subsystem driver support for AM33xx SOC.
> +
> +	  PWM submodules require PWM config space access from submodule
> +	  drivers and require common parent driver support.
> +
>  config PWM_TWL6030
>  	tristate "TWL6030 PWM support"
>  	depends on TWL4030_CORE
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 3b3f4c9..55f6fb2 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -12,5 +12,6 @@ obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
>  obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
>  obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
>  obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
> +obj-$(CONFIG_PWM_TIPWMSS)	+= tipwmss.o

This should have a pwm- prefix as well.

>  obj-$(CONFIG_PWM_TWL6030)	+= pwm-twl6030.o
>  obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
> diff --git a/drivers/pwm/tipwmss.c b/drivers/pwm/tipwmss.c
> new file mode 100644
> index 0000000..c188348
> --- /dev/null
> +++ b/drivers/pwm/tipwmss.c
> @@ -0,0 +1,142 @@
[...]
> +#include "tipwmss.h"
> +
> +#define PWMSS_CLKCONFIG		0x8	/* Clock gaitng reg, for PWM submodules */

"gating"

> +#define PWMSS_CLKSTATUS		0xc	/* Clock gating status reg */
> +
> +struct pwmss_info {
> +	void __iomem	*mmio_base;
> +	struct mutex	pwmss_lock;
> +	u16				pwmss_clkconfig;

The indentation looks weird on this last field.

> +};
> +
> +u16 pwmss_submodule_state_change(struct device *dev, int set)
> +{
> +	struct pwmss_info *info = dev_get_drvdata(dev);
> +	u16 val;
> +
> +	val = readw(info->mmio_base + PWMSS_CLKCONFIG);
> +	val |= set;
> +	mutex_lock(&info->pwmss_lock);
> +	writew(val , info->mmio_base + PWMSS_CLKCONFIG);
> +	mutex_unlock(&info->pwmss_lock);

The mutex needs to span the whole read-modify-write sequence, not just
the write.

Also, how do you clear this state?

> +	return readw(info->mmio_base + PWMSS_CLKSTATUS);
> +}
> +EXPORT_SYMBOL(pwmss_submodule_state_change);
> +
> +static const struct of_device_id pwmss_of_match[] = {
> +	{
> +		.compatible	= "ti,am33xx-pwmss",
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, pwmss_of_match);
> +
> +static int __devinit pwmss_probe(struct platform_device *pdev)

__dev* annotation usage is deprecated, you should drop it.

> +{
> +	int ret;
> +	struct resource *r;
> +	struct pwmss_info *info;
> +	struct device_node *node = pdev->dev.of_node;
> +
> +	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
> +	if (!info) {
> +		dev_err(&pdev->dev, "failed to allocate memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	mutex_init(&info->pwmss_lock);
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);

Blank line between those two lines?

> +	if (!r) {
> +		dev_err(&pdev->dev, "no memory resource defined\n");
> +		return -ENODEV;
> +	}
> +
> +	info->mmio_base = devm_request_and_ioremap(&pdev->dev, r);
> +	if (!info->mmio_base)
> +		return -EADDRNOTAVAIL;
> +
> +	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_get_sync(&pdev->dev);
> +	platform_set_drvdata(pdev, info);
> +
> +	/* Populate all the child nodes here... */
> +	ret = of_platform_populate(node, NULL, NULL, &pdev->dev);
> +	if (ret)
> +		dev_warn(&pdev->dev, "Doesn't have any child node\n");

This reads oddly compared to the other error messages, maybe something
like "no children found" or similar would be more consistent.

> +
> +	return ret;

Then again, since you return of_platform_populate()'s error code here,
you may just want to skip the above warning since the driver probe won't
succeed anyway. Or if you really want to give a hint as to why loading
failed, maybe it would be better to make it an error message instead.

There is one little problem with registering the children here, which is
that if you build the drivers as modules, because once the pwm-tipwmss
module is unloaded, reloading it will fail since it will try to create
the children again.

AFAICT there are two solutions to this: a) do not allow the pwm-tipwmss
code to be built as a module and b) have of_platform_populate() called
by the architecture initialization code. Both are relatively easy to
implement. a) can be done by making the PWM_TIPWMSS symbol bool instead
of tristate, and b) can be done by adding "simple-bus" to the end of the
compatible list in the DT.

> +}
> +
> +static int __devexit pwmss_remove(struct platform_device *pdev)

Again, no need for __devexit anymore.

> +{
> +	struct pwmss_info *info = platform_get_drvdata(pdev);
> +
> +	pm_runtime_put_sync(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +	mutex_destroy(&info->pwmss_lock);
> +	return 0;
> +}
> +
> +static int pwmss_suspend(struct device *dev)
> +{
> +	struct pwmss_info *info = dev_get_drvdata(dev);
> +
> +	info->pwmss_clkconfig = readw(info->mmio_base + PWMSS_CLKCONFIG);
> +	pm_runtime_put_sync(dev);
> +	return 0;
> +}
> +
> +static int pwmss_resume(struct device *dev)
> +{
> +	struct pwmss_info *info = dev_get_drvdata(dev);
> +
> +	pm_runtime_get_sync(dev);
> +	writew(info->pwmss_clkconfig, info->mmio_base + PWMSS_CLKCONFIG);
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops pwmss_pm_ops = {
> +	.suspend	= pwmss_suspend,
> +	.resume		= pwmss_resume,
> +};

Shouldn't these functions be conditionalized on CONFIG_PM_SLEEP? And
maybe you want to use the SIMPLE_DEV_PM_OPS macro here.

> +
> +static struct platform_driver pwmss_driver = {
> +	.driver	= {
> +		.name	= "pwmss",
> +		.owner	= THIS_MODULE,
> +		.pm	= &pwmss_pm_ops,
> +		.of_match_table	= of_match_ptr(pwmss_of_match),

You already define the pwmss_of_match table unconditionally, so you
don't need the of_match_ptr() either.

> +	},
> +	.probe	= pwmss_probe,
> +	.remove	= __devexit_p(pwmss_remove),

__devexit_p() can go away.

> +};
> +
> +module_platform_driver(pwmss_driver);
> +
> +MODULE_DESCRIPTION("pwmss driver");

This description could be better.

> +MODULE_AUTHOR("Texas Instruments");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/pwm/tipwmss.h b/drivers/pwm/tipwmss.h
> new file mode 100644
> index 0000000..f9cb2e2
> --- /dev/null
> +++ b/drivers/pwm/tipwmss.h

I think this should also get the pwm- prefix for consistency with the
source file.

> @@ -0,0 +1,30 @@
> +/*
> + * TI PWM Subsystem driver
> + *
> + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
> + *
> + * 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; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef __TIPWMSS_H
> +#define __TIPWMSS_H
> +
> +#ifdef CONFIG_PWM_TIPWMSS
> +extern u16 pwmss_submodule_state_change(struct device *dev, int set);
> +#else
> +static inline u16 pwmss_submodule_state_change(struct device *dev, int set)
> +{
> +	/* return success status value */
> +	return 0xFFFF;
> +}
> +#endif
> +#endif	/* __TIPWMSS_H */

Is it really necessary to provide a !PWM_TIPWMSS version of this
function? All users that want to use it can select it and get the
correct version, right?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

RE: [PATCH v2 01/10] PWMSS: Add PWM Subsystem driver for parent<->child relationship

[Philip, Avinash]

On Fri, Nov 09, 2012 at 12:59:57, Thierry Reding wrote:
> On Thu, Nov 08, 2012 at 01:23:08PM +0530, Philip, Avinash wrote:
> > diff --git a/Documentation/devicetree/bindings/pwm/tipwmss.txt b/Documentation/devicetree/bindings/pwm/tipwmss.txt
> > new file mode 100644
> > index 0000000..b6c2814
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/tipwmss.txt
> > @@ -0,0 +1,30 @@
> [...]
> > +Also child nodes should also populated under PWMSS DT node.
> > +Example:
> 
> Maybe put an blank line between these two lines for readability?

Ok I will add.

> 
> > +pwmss0: pwmss@48300000 {
> > +	compatible = "ti,am33xx-pwmss";
> > +	reg = <0x48300000 0x10
> > +		0x48300100 0x80
> > +		0x48300180 0x80
> > +		0x48300200 0x80>;
> 
> I don't think you should list the register spaces of the children here.
> From what I understand, all regions listed in the reg property are
> supposed to be requested by the corresponding driver and therefore
> cannot be used by any other device.

I will check & correct it.

> 
> > +	ti,hwmods = "epwmss0";
> > +	#address-cells = <1>;
> > +	#size-cells = <1>;
> > +	status = "disabled";
> > +	ranges;
> 
> I think to represent which memory regions go to the children, you should
> put them in this ranges property, which would then look like this:
> 
> 	ranges = <0x48300100 0x48300100 0x80   /* ECAP */
> 		  0x48300180 0x48300180 0x80   /* EQEP */
> 		  0x48300200 0x48300200 0x80>; /* EHRPWM */

I will correct it.

> 
> > +
> > +	/* child nodes go here */
> > +};
> 
> Maybe you should actually list a full set of children here?

Ok.

> 
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 6e556c7..384a346 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -136,6 +136,7 @@ config PWM_TEGRA
> >  config  PWM_TIECAP
> >  	tristate "ECAP PWM support"
> >  	depends on SOC_AM33XX
> > +	select PWM_TIPWMSS
> >  	help
> >  	  PWM driver support for the ECAP APWM controller found on AM33XX
> >  	  TI SOC
> > @@ -146,6 +147,7 @@ config  PWM_TIECAP
> >  config  PWM_TIEHRPWM
> >  	tristate "EHRPWM PWM support"
> >  	depends on SOC_AM33XX
> > +	select PWM_TIPWMSS
> >  	help
> >  	  PWM driver support for the EHRPWM controller found on AM33XX
> >  	  TI SOC
> > @@ -153,6 +155,15 @@ config  PWM_TIEHRPWM
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called pwm-tiehrpwm.
> >  
> > +config  PWM_TIPWMSS
> > +	tristate "TI PWM Subsytem parent support"
> > +	depends on SOC_AM33XX && (PWM_TIEHRPWM || PWM_TIECAP)
> 
> Since you select the symbol from the PWM_TIECAP and PWM_TIEHRPWM symbols
> there is no need to depend on them, right? Oh, but maybe that's to make
> sure the symbol is deselected automatically if neither user is selected.

I want to make sure that, symbol selected only if either are selected.
> 
> Perhaps this should actually be a hidden symbol (i.e. leave away the
> prompt string in the tristate option) since it's purely a dependency and
> not useful of its own.

Ok I will take care.

> 
> > +	help
> > +	  PWM Subsystem driver support for AM33xx SOC.
> > +
> > +	  PWM submodules require PWM config space access from submodule
> > +	  drivers and require common parent driver support.
> > +
> >  config PWM_TWL6030
> >  	tristate "TWL6030 PWM support"
> >  	depends on TWL4030_CORE
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index 3b3f4c9..55f6fb2 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -12,5 +12,6 @@ obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
> >  obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
> >  obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
> >  obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
> > +obj-$(CONFIG_PWM_TIPWMSS)	+= tipwmss.o
> 
> This should have a pwm- prefix as well.

Ok I will add.

> 
> >  obj-$(CONFIG_PWM_TWL6030)	+= pwm-twl6030.o
> >  obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
> > diff --git a/drivers/pwm/tipwmss.c b/drivers/pwm/tipwmss.c
> > new file mode 100644
> > index 0000000..c188348
> > --- /dev/null
> > +++ b/drivers/pwm/tipwmss.c
> > @@ -0,0 +1,142 @@
> [...]
> > +#include "tipwmss.h"
> > +
> > +#define PWMSS_CLKCONFIG		0x8	/* Clock gaitng reg, for PWM submodules */
> 
> "gating"

Ok

> 
> > +#define PWMSS_CLKSTATUS		0xc	/* Clock gating status reg */
> > +
> > +struct pwmss_info {
> > +	void __iomem	*mmio_base;
> > +	struct mutex	pwmss_lock;
> > +	u16				pwmss_clkconfig;
> 
> The indentation looks weird on this last field.

Ok I will correct it.

> 
> > +};
> > +
> > +u16 pwmss_submodule_state_change(struct device *dev, int set)
> > +{
> > +	struct pwmss_info *info = dev_get_drvdata(dev);
> > +	u16 val;
> > +
> > +	val = readw(info->mmio_base + PWMSS_CLKCONFIG);
> > +	val |= set;
> > +	mutex_lock(&info->pwmss_lock);
> > +	writew(val , info->mmio_base + PWMSS_CLKCONFIG);
> > +	mutex_unlock(&info->pwmss_lock);
> 
> The mutex needs to span the whole read-modify-write sequence, not just
> the write.

I missed. I will correct it.

> 
> Also, how do you clear this state?

Here is the hardware trick comes. 
For enabling should write enable bit.
For disabling stop_req bit should set & it will clear enable bit also.

> 
> > +	return readw(info->mmio_base + PWMSS_CLKSTATUS);
> > +}
> > +EXPORT_SYMBOL(pwmss_submodule_state_change);
> > +
> > +static const struct of_device_id pwmss_of_match[] = {
> > +	{
> > +		.compatible	= "ti,am33xx-pwmss",
> > +	},
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, pwmss_of_match);
> > +
> > +static int __devinit pwmss_probe(struct platform_device *pdev)
> 
> __dev* annotation usage is deprecated, you should drop it.

Ok

> 
> > +{
> > +	int ret;
> > +	struct resource *r;
> > +	struct pwmss_info *info;
> > +	struct device_node *node = pdev->dev.of_node;
> > +
> > +	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
> > +	if (!info) {
> > +		dev_err(&pdev->dev, "failed to allocate memory\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	mutex_init(&info->pwmss_lock);
> > +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> 
> Blank line between those two lines?

Ok

> 
> > +	if (!r) {
> > +		dev_err(&pdev->dev, "no memory resource defined\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	info->mmio_base = devm_request_and_ioremap(&pdev->dev, r);
> > +	if (!info->mmio_base)
> > +		return -EADDRNOTAVAIL;
> > +
> > +	pm_runtime_enable(&pdev->dev);
> > +	pm_runtime_get_sync(&pdev->dev);
> > +	platform_set_drvdata(pdev, info);
> > +
> > +	/* Populate all the child nodes here... */
> > +	ret = of_platform_populate(node, NULL, NULL, &pdev->dev);
> > +	if (ret)
> > +		dev_warn(&pdev->dev, "Doesn't have any child node\n");
> 
> This reads oddly compared to the other error messages, maybe something
> like "no children found" or similar would be more consistent.

Ok

> 
> > +
> > +	return ret;
> 
> Then again, since you return of_platform_populate()'s error code here,
> you may just want to skip the above warning since the driver probe won't
> succeed anyway. Or if you really want to give a hint as to why loading
> failed, maybe it would be better to make it an error message instead.

Ok I will change to error message.

> 
> There is one little problem with registering the children here, which is
> that if you build the drivers as modules, because once the pwm-tipwmss
> module is unloaded, reloading it will fail since it will try to create
> the children again.
> 
> AFAICT there are two solutions to this: a) do not allow the pwm-tipwmss
> code to be built as a module and b) have of_platform_populate() called
> by the architecture initialization code. Both are relatively easy to
> implement. a) can be done by making the PWM_TIPWMSS symbol bool instead
> of tristate, and b) can be done by adding "simple-bus" to the end of the
> compatible list in the DT.

Ok I will go for option a.

> 
> > +}
> > +
> > +static int __devexit pwmss_remove(struct platform_device *pdev)
> 
> Again, no need for __devexit anymore.

I will remove

> 
...
> > +
> > +static const struct dev_pm_ops pwmss_pm_ops = {
> > +	.suspend	= pwmss_suspend,
> > +	.resume		= pwmss_resume,
> > +};
> 
> Shouldn't these functions be conditionalized on CONFIG_PM_SLEEP? And
> maybe you want to use the SIMPLE_DEV_PM_OPS macro here.

I will check and correct it.

> 
> > +
> > +static struct platform_driver pwmss_driver = {
> > +	.driver	= {
> > +		.name	= "pwmss",
> > +		.owner	= THIS_MODULE,
> > +		.pm	= &pwmss_pm_ops,
> > +		.of_match_table	= of_match_ptr(pwmss_of_match),
> 
> You already define the pwmss_of_match table unconditionally, so you
> don't need the of_match_ptr() either.

Will remove

> 
> > +	},
> > +	.probe	= pwmss_probe,
> > +	.remove	= __devexit_p(pwmss_remove),
> 
> __devexit_p() can go away.

Will remove

> 
> > +};
> > +
> > +module_platform_driver(pwmss_driver);
> > +
> > +MODULE_DESCRIPTION("pwmss driver");
> 
> This description could be better.

Will add.
> 
> > +MODULE_AUTHOR("Texas Instruments");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/pwm/tipwmss.h b/drivers/pwm/tipwmss.h
> > new file mode 100644
> > index 0000000..f9cb2e2
> > --- /dev/null
> > +++ b/drivers/pwm/tipwmss.h
> 
> I think this should also get the pwm- prefix for consistency with the
> source file.

Will add the prefix.

> 
> > @@ -0,0 +1,30 @@
> > +/*
> > + * TI PWM Subsystem driver
> > + *
> > + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
> > + *
> > + * 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; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + */
> > +
> > +#ifndef __TIPWMSS_H
> > +#define __TIPWMSS_H
> > +
> > +#ifdef CONFIG_PWM_TIPWMSS
> > +extern u16 pwmss_submodule_state_change(struct device *dev, int set);
> > +#else
> > +static inline u16 pwmss_submodule_state_change(struct device *dev, int set)
> > +{
> > +	/* return success status value */
> > +	return 0xFFFF;
> > +}
> > +#endif
> > +#endif	/* __TIPWMSS_H */
> 
> Is it really necessary to provide a !PWM_TIPWMSS version of this
> function? All users that want to use it can select it and get the
> correct version, right?

Added the !PWM_TIPWMSS part to support platforms that didn't
have PWM Subsytem support.


I will send next version on nov 19th as I was on leave on next week.

Thanks
Avinash
> 
> Thierry
> 

[PATCH v2 02/10] ARM: am33xx: clk: Add optional clock for EHRPWM

[Philip, Avinash]

EHRPWM module requires explicit clock gating from control module.
Hence add clock node in clock tree for EHRPWM modules.

Signed-off-by: Philip, Avinash <avinashphilip-l0cyMroinI0@public.gmane.org>
---
:100644 100644 17e3de5... 833260f... M	arch/arm/mach-omap2/clock33xx_data.c
:100644 100644 a89e825... c0e34e6... M	arch/arm/mach-omap2/control.h
 arch/arm/mach-omap2/clock33xx_data.c |   37 ++++++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/control.h        |    8 +++++++
 2 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/clock33xx_data.c b/arch/arm/mach-omap2/clock33xx_data.c
index 17e3de5..833260f 100644
--- a/arch/arm/mach-omap2/clock33xx_data.c
+++ b/arch/arm/mach-omap2/clock33xx_data.c
@@ -995,6 +995,40 @@ static struct clk wdt1_fck = {
 };
 
 /*
+ * PWMSS Time based module clock node. This node is
+ * requred to enable clock gating for EHRPWM TBCLK.
+ */
+static struct clk ehrpwm0_tbclk = {
+	.name		= "ehrpwm0_tbclk",
+	.clkdm_name	= "l4ls_clkdm",
+	.enable_reg	= AM33XX_CTRL_REGADDR(AM33XX_PWMSS_TBCLK_CLKCTRL),
+	.enable_bit	= AM33XX_PWMSS0_TBCLKEN_SHIFT,
+	.ops		= &clkops_omap2_dflt,
+	.parent		= &l4ls_gclk,
+	.recalc		= &followparent_recalc,
+};
+
+static struct clk ehrpwm1_tbclk = {
+	.name		= "ehrpwm1_tbclk",
+	.clkdm_name	= "l4ls_clkdm",
+	.enable_reg	= AM33XX_CTRL_REGADDR(AM33XX_PWMSS_TBCLK_CLKCTRL),
+	.enable_bit	= AM33XX_PWMSS1_TBCLKEN_SHIFT,
+	.ops		= &clkops_omap2_dflt,
+	.parent		= &l4ls_gclk,
+	.recalc		= &followparent_recalc,
+};
+
+static struct clk ehrpwm2_tbclk = {
+	.name		= "ehrpwm2_tbclk",
+	.clkdm_name	= "l4ls_clkdm",
+	.enable_reg	= AM33XX_CTRL_REGADDR(AM33XX_PWMSS_TBCLK_CLKCTRL),
+	.enable_bit	= AM33XX_PWMSS2_TBCLKEN_SHIFT,
+	.ops		= &clkops_omap2_dflt,
+	.parent		= &l4ls_gclk,
+	.recalc		= &followparent_recalc,
+};
+
+/*
  * clkdev
  */
 static struct omap_clk am33xx_clks[] = {
@@ -1074,6 +1108,9 @@ static struct omap_clk am33xx_clks[] = {
 	CLK(NULL,	"clkout2_ck",		&clkout2_ck,	CK_AM33XX),
 	CLK(NULL,	"timer_32k_ck",		&clkdiv32k_ick,	CK_AM33XX),
 	CLK(NULL,	"timer_sys_ck",		&sys_clkin_ck,	CK_AM33XX),
+	CLK(NULL,	"ehrpwm0_tbclk",	&ehrpwm0_tbclk,	CK_AM33XX),
+	CLK(NULL,	"ehrpwm1_tbclk",	&ehrpwm1_tbclk,	CK_AM33XX),
+	CLK(NULL,	"ehrpwm2_tbclk",	&ehrpwm2_tbclk,	CK_AM33XX),
 };
 
 int __init am33xx_clk_init(void)
diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h
index a89e825..c0e34e6 100644
--- a/arch/arm/mach-omap2/control.h
+++ b/arch/arm/mach-omap2/control.h
@@ -357,6 +357,14 @@
 #define AM33XX_CONTROL_STATUS_SYSBOOT1_WIDTH		0x2
 #define AM33XX_CONTROL_STATUS_SYSBOOT1_MASK		(0x3 << 22)
 
+/* AM33XX PWMSS Control register */
+#define AM33XX_PWMSS_TBCLK_CLKCTRL			0x664
+
+/* AM33XX  PWMSS Control bitfields */
+#define AM33XX_PWMSS0_TBCLKEN_SHIFT			0
+#define AM33XX_PWMSS1_TBCLKEN_SHIFT			1
+#define AM33XX_PWMSS2_TBCLKEN_SHIFT			2
+
 /* CONTROL OMAP STATUS register to identify OMAP3 features */
 #define OMAP3_CONTROL_OMAP_STATUS	0x044c
 
-- 
1.7.0.4

[PATCH v2 03/10] ARM: OMAP: AM33xx hwmod: Add parent-child relationship for PWM subsystem

[Philip, Avinash]

As part of PWM subsystem integration, PWM subsystem are sharing
resources like clock across submodules (ECAP, EQEP & EHRPWM).
To handle resource sharing & IP integration
1. Rework on parent child relation between PWMSS and
   ECAP, EQEP & EHRPWM child devices to support runtime PM.
2. Add support for opt_clks in EHRPWM HWMOD entry to handle additional
   clock gating from control module.
3. Add HWMOD entries for EQEP PWM submodule.

Signed-off-by: Philip, Avinash <avinashphilip-l0cyMroinI0@public.gmane.org>
---
Changes since v1:
	- Remove ADDR_TYPE_RT for PWM sub module register entries.

:100644 100644 ad8d43b... de2301c... M	arch/arm/mach-omap2/omap_hwmod_33xx_data.c
 arch/arm/mach-omap2/omap_hwmod_33xx_data.c |  419 ++++++++++++++++++----------
 1 files changed, 276 insertions(+), 143 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod_33xx_data.c b/arch/arm/mach-omap2/omap_hwmod_33xx_data.c
index ad8d43b..de2301c 100644
--- a/arch/arm/mach-omap2/omap_hwmod_33xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_33xx_data.c
@@ -768,9 +768,7 @@ static struct omap_hwmod am33xx_elm_hwmod = {
 	},
 };
 
-/*
- * 'epwmss' class: ecap0,1,2,  ehrpwm0,1,2
- */
+/* pwmss */
 static struct omap_hwmod_class_sysconfig am33xx_epwmss_sysc = {
 	.rev_offs	= 0x0,
 	.sysc_offs	= 0x4,
@@ -786,18 +784,23 @@ static struct omap_hwmod_class am33xx_epwmss_hwmod_class = {
 	.sysc		= &am33xx_epwmss_sysc,
 };
 
-/* ehrpwm0 */
-static struct omap_hwmod_irq_info am33xx_ehrpwm0_irqs[] = {
-	{ .name = "int", .irq = 86 + OMAP_INTC_START, },
-	{ .name = "tzint", .irq = 58 + OMAP_INTC_START, },
-	{ .irq = -1 },
+
+static struct omap_hwmod_class am33xx_ecap_hwmod_class = {
+	.name		= "ecap",
 };
 
-static struct omap_hwmod am33xx_ehrpwm0_hwmod = {
-	.name		= "ehrpwm0",
+static struct omap_hwmod_class am33xx_eqep_hwmod_class = {
+	.name		= "eqep",
+};
+
+static struct omap_hwmod_class am33xx_ehrpwm_hwmod_class = {
+	.name		= "ehrpwm",
+};
+/* epwmss0 */
+static struct omap_hwmod am33xx_epwmss0_hwmod = {
+	.name		= "epwmss0",
 	.class		= &am33xx_epwmss_hwmod_class,
 	.clkdm_name	= "l4ls_clkdm",
-	.mpu_irqs	= am33xx_ehrpwm0_irqs,
 	.main_clk	= "l4ls_gclk",
 	.prcm		= {
 		.omap4	= {
@@ -807,63 +810,68 @@ static struct omap_hwmod am33xx_ehrpwm0_hwmod = {
 	},
 };
 
-/* ehrpwm1 */
-static struct omap_hwmod_irq_info am33xx_ehrpwm1_irqs[] = {
-	{ .name = "int", .irq = 87 + OMAP_INTC_START, },
-	{ .name = "tzint", .irq = 59 + OMAP_INTC_START, },
+/* ecap0 */
+static struct omap_hwmod_irq_info am33xx_ecap0_irqs[] = {
+	{ .irq = 31 + OMAP_INTC_START, },
 	{ .irq = -1 },
 };
 
-static struct omap_hwmod am33xx_ehrpwm1_hwmod = {
-	.name		= "ehrpwm1",
-	.class		= &am33xx_epwmss_hwmod_class,
+static struct omap_hwmod am33xx_ecap0_hwmod = {
+	.name		= "ecap0",
+	.class		= &am33xx_ecap_hwmod_class,
 	.clkdm_name	= "l4ls_clkdm",
-	.mpu_irqs	= am33xx_ehrpwm1_irqs,
+	.mpu_irqs	= am33xx_ecap0_irqs,
 	.main_clk	= "l4ls_gclk",
-	.prcm		= {
-		.omap4	= {
-			.clkctrl_offs	= AM33XX_CM_PER_EPWMSS1_CLKCTRL_OFFSET,
-			.modulemode	= MODULEMODE_SWCTRL,
-		},
-	},
 };
 
-/* ehrpwm2 */
-static struct omap_hwmod_irq_info am33xx_ehrpwm2_irqs[] = {
-	{ .name = "int", .irq = 39 + OMAP_INTC_START, },
-	{ .name = "tzint", .irq = 60 + OMAP_INTC_START, },
+/* eqep0 */
+static struct omap_hwmod_irq_info am33xx_eqep0_irqs[] = {
+	{ .irq = 79 + OMAP_INTC_START, },
 	{ .irq = -1 },
 };
 
-static struct omap_hwmod am33xx_ehrpwm2_hwmod = {
-	.name		= "ehrpwm2",
-	.class		= &am33xx_epwmss_hwmod_class,
+static struct omap_hwmod am33xx_eqep0_hwmod = {
+	.name		= "eqep0",
+	.class		= &am33xx_eqep_hwmod_class,
 	.clkdm_name	= "l4ls_clkdm",
-	.mpu_irqs	= am33xx_ehrpwm2_irqs,
+	.mpu_irqs	= am33xx_eqep0_irqs,
 	.main_clk	= "l4ls_gclk",
-	.prcm		= {
-		.omap4	= {
-			.clkctrl_offs	= AM33XX_CM_PER_EPWMSS2_CLKCTRL_OFFSET,
-			.modulemode	= MODULEMODE_SWCTRL,
-		},
-	},
 };
 
-/* ecap0 */
-static struct omap_hwmod_irq_info am33xx_ecap0_irqs[] = {
-	{ .irq = 31 + OMAP_INTC_START, },
+/* ehrpwm0 */
+static struct omap_hwmod_irq_info am33xx_ehrpwm0_irqs[] = {
+	{ .name = "int", .irq = 86 + OMAP_INTC_START, },
+	{ .name = "tzint", .irq = 58 + OMAP_INTC_START, },
 	{ .irq = -1 },
 };
 
-static struct omap_hwmod am33xx_ecap0_hwmod = {
-	.name		= "ecap0",
+/*
+ * Optional clock entry is provided to support additional clock
+ * gating for EHRPWM module functional from control module.
+ */
+static struct omap_hwmod_opt_clk ehrpwm0_opt_clks[] = {
+	{ .role = "tbclk", .clk = "ehrpwm0_tbclk" },
+};
+
+static struct omap_hwmod am33xx_ehrpwm0_hwmod = {
+	.name		= "ehrpwm0",
+	.class		= &am33xx_ehrpwm_hwmod_class,
+	.clkdm_name	= "l4ls_clkdm",
+	.mpu_irqs	= am33xx_ehrpwm0_irqs,
+	.main_clk	= "l4ls_gclk",
+	.opt_clks	= ehrpwm0_opt_clks,
+	.opt_clks_cnt	= ARRAY_SIZE(ehrpwm0_opt_clks),
+};
+
+/* epwmss1 */
+static struct omap_hwmod am33xx_epwmss1_hwmod = {
+	.name		= "epwmss1",
 	.class		= &am33xx_epwmss_hwmod_class,
 	.clkdm_name	= "l4ls_clkdm",
-	.mpu_irqs	= am33xx_ecap0_irqs,
 	.main_clk	= "l4ls_gclk",
 	.prcm		= {
 		.omap4	= {
-			.clkctrl_offs	= AM33XX_CM_PER_EPWMSS0_CLKCTRL_OFFSET,
+			.clkctrl_offs	= AM33XX_CM_PER_EPWMSS1_CLKCTRL_OFFSET,
 			.modulemode	= MODULEMODE_SWCTRL,
 		},
 	},
@@ -877,13 +885,60 @@ static struct omap_hwmod_irq_info am33xx_ecap1_irqs[] = {
 
 static struct omap_hwmod am33xx_ecap1_hwmod = {
 	.name		= "ecap1",
-	.class		= &am33xx_epwmss_hwmod_class,
+	.class		= &am33xx_ecap_hwmod_class,
 	.clkdm_name	= "l4ls_clkdm",
 	.mpu_irqs	= am33xx_ecap1_irqs,
 	.main_clk	= "l4ls_gclk",
+};
+
+/* eqep1 */
+static struct omap_hwmod_irq_info am33xx_eqep1_irqs[] = {
+	{ .irq = 88 + OMAP_INTC_START, },
+	{ .irq = -1 },
+};
+
+static struct omap_hwmod am33xx_eqep1_hwmod = {
+	.name		= "eqep1",
+	.class		= &am33xx_eqep_hwmod_class,
+	.clkdm_name	= "l4ls_clkdm",
+	.mpu_irqs	= am33xx_eqep1_irqs,
+	.main_clk	= "l4ls_gclk",
+};
+
+/* ehrpwm1 */
+static struct omap_hwmod_irq_info am33xx_ehrpwm1_irqs[] = {
+	{ .name = "int", .irq = 87 + OMAP_INTC_START, },
+	{ .name = "tzint", .irq = 59 + OMAP_INTC_START, },
+	{ .irq = -1 },
+};
+
+/*
+ * Optional clock entry is provided to support additional clock
+ * gating for EHRPWM module functional from control module.
+ */
+static struct omap_hwmod_opt_clk ehrpwm1_opt_clks[] = {
+	{ .role = "tbclk", .clk = "ehrpwm1_tbclk" },
+};
+
+static struct omap_hwmod am33xx_ehrpwm1_hwmod = {
+	.name		= "ehrpwm1",
+	.class		= &am33xx_ehrpwm_hwmod_class,
+	.clkdm_name	= "l4ls_clkdm",
+	.mpu_irqs	= am33xx_ehrpwm1_irqs,
+	.main_clk	= "l4ls_gclk",
+	.opt_clks	= ehrpwm1_opt_clks,
+	.opt_clks_cnt	= ARRAY_SIZE(ehrpwm1_opt_clks),
+};
+
+/* epwmss2 */
+static struct omap_hwmod am33xx_epwmss2_hwmod = {
+	.name		= "epwmss2",
+	.class		= &am33xx_epwmss_hwmod_class,
+	.clkdm_name	= "l4ls_clkdm",
+	.main_clk	= "l4ls_gclk",
 	.prcm		= {
 		.omap4	= {
-			.clkctrl_offs	= AM33XX_CM_PER_EPWMSS1_CLKCTRL_OFFSET,
+			.clkctrl_offs	= AM33XX_CM_PER_EPWMSS2_CLKCTRL_OFFSET,
 			.modulemode	= MODULEMODE_SWCTRL,
 		},
 	},
@@ -897,16 +952,49 @@ static struct omap_hwmod_irq_info am33xx_ecap2_irqs[] = {
 
 static struct omap_hwmod am33xx_ecap2_hwmod = {
 	.name		= "ecap2",
+	.class		= &am33xx_ecap_hwmod_class,
+	.clkdm_name	= "l4ls_clkdm",
 	.mpu_irqs	= am33xx_ecap2_irqs,
-	.class		= &am33xx_epwmss_hwmod_class,
+	.main_clk	= "l4ls_gclk",
+};
+
+/* eqep2 */
+static struct omap_hwmod_irq_info am33xx_eqep2_irqs[] = {
+	{ .irq = 89 + OMAP_INTC_START, },
+	{ .irq = -1 },
+};
+
+static struct omap_hwmod am33xx_eqep2_hwmod = {
+	.name		= "eqep2",
+	.class		= &am33xx_eqep_hwmod_class,
 	.clkdm_name	= "l4ls_clkdm",
+	.mpu_irqs	= am33xx_eqep2_irqs,
 	.main_clk	= "l4ls_gclk",
-	.prcm		= {
-		.omap4	= {
-			.clkctrl_offs	= AM33XX_CM_PER_EPWMSS2_CLKCTRL_OFFSET,
-			.modulemode	= MODULEMODE_SWCTRL,
-		},
-	},
+};
+
+/* ehrpwm2 */
+static struct omap_hwmod_irq_info am33xx_ehrpwm2_irqs[] = {
+	{ .name = "int", .irq = 39 + OMAP_INTC_START, },
+	{ .name = "tzint", .irq = 60 + OMAP_INTC_START, },
+	{ .irq = -1 },
+};
+
+/*
+ * Optional clock entry is provided to support additional clock
+ * gating for EHRPWM module functional from control module.
+ */
+static struct omap_hwmod_opt_clk ehrpwm2_opt_clks[] = {
+	{ .role = "tbclk", .clk = "ehrpwm2_tbclk" },
+};
+
+static struct omap_hwmod am33xx_ehrpwm2_hwmod = {
+	.name		= "ehrpwm2",
+	.class		= &am33xx_ehrpwm_hwmod_class,
+	.clkdm_name	= "l4ls_clkdm",
+	.mpu_irqs	= am33xx_ehrpwm2_irqs,
+	.main_clk	= "l4ls_gclk",
+	.opt_clks	= ehrpwm2_opt_clks,
+	.opt_clks_cnt	= ARRAY_SIZE(ehrpwm2_opt_clks),
 };
 
 /*
@@ -2518,162 +2606,201 @@ static struct omap_hwmod_ocp_if am33xx_l4_ls__elm = {
 	.user		= OCP_USER_MPU,
 };
 
-/*
- * Splitting the resources to handle access of PWMSS config space
- * and module specific part independently
- */
-static struct omap_hwmod_addr_space am33xx_ehrpwm0_addr_space[] = {
+static struct omap_hwmod_addr_space am33xx_epwmss0_addr_space[] = {
 	{
 		.pa_start	= 0x48300000,
 		.pa_end		= 0x48300000 + SZ_16 - 1,
 		.flags		= ADDR_TYPE_RT
 	},
-	{
-		.pa_start	= 0x48300200,
-		.pa_end		= 0x48300200 + SZ_256 - 1,
-		.flags		= ADDR_TYPE_RT
-	},
 	{ }
 };
 
-static struct omap_hwmod_ocp_if am33xx_l4_ls__ehrpwm0 = {
+static struct omap_hwmod_ocp_if am33xx_l4_ls__epwmss0 = {
 	.master		= &am33xx_l4_ls_hwmod,
-	.slave		= &am33xx_ehrpwm0_hwmod,
+	.slave		= &am33xx_epwmss0_hwmod,
 	.clk		= "l4ls_gclk",
-	.addr		= am33xx_ehrpwm0_addr_space,
+	.addr		= am33xx_epwmss0_addr_space,
 	.user		= OCP_USER_MPU,
 };
 
-/*
- * Splitting the resources to handle access of PWMSS config space
- * and module specific part independently
- */
-static struct omap_hwmod_addr_space am33xx_ehrpwm1_addr_space[] = {
-	{
-		.pa_start	= 0x48302000,
-		.pa_end		= 0x48302000 + SZ_16 - 1,
-		.flags		= ADDR_TYPE_RT
-	},
+static struct omap_hwmod_addr_space am33xx_ecap0_addr_space[] = {
 	{
-		.pa_start	= 0x48302200,
-		.pa_end		= 0x48302200 + SZ_256 - 1,
-		.flags		= ADDR_TYPE_RT
+		.pa_start	= 0x48300100,
+		.pa_end		= 0x48300100 + SZ_128 - 1,
 	},
 	{ }
 };
 
-static struct omap_hwmod_ocp_if am33xx_l4_ls__ehrpwm1 = {
-	.master		= &am33xx_l4_ls_hwmod,
-	.slave		= &am33xx_ehrpwm1_hwmod,
+static struct omap_hwmod_ocp_if am33xx_epwmss0__ecap0 = {
+	.master		= &am33xx_epwmss0_hwmod,
+	.slave		= &am33xx_ecap0_hwmod,
 	.clk		= "l4ls_gclk",
-	.addr		= am33xx_ehrpwm1_addr_space,
+	.addr		= am33xx_ecap0_addr_space,
 	.user		= OCP_USER_MPU,
 };
 
-/*
- * Splitting the resources to handle access of PWMSS config space
- * and module specific part independently
- */
-static struct omap_hwmod_addr_space am33xx_ehrpwm2_addr_space[] = {
+static struct omap_hwmod_addr_space am33xx_eqep0_addr_space[] = {
 	{
-		.pa_start	= 0x48304000,
-		.pa_end		= 0x48304000 + SZ_16 - 1,
-		.flags		= ADDR_TYPE_RT
-	},
-	{
-		.pa_start	= 0x48304200,
-		.pa_end		= 0x48304200 + SZ_256 - 1,
-		.flags		= ADDR_TYPE_RT
+		.pa_start	= 0x48300180,
+		.pa_end		= 0x48300180 + SZ_128 - 1,
 	},
 	{ }
 };
 
-static struct omap_hwmod_ocp_if am33xx_l4_ls__ehrpwm2 = {
-	.master		= &am33xx_l4_ls_hwmod,
-	.slave		= &am33xx_ehrpwm2_hwmod,
+static struct omap_hwmod_ocp_if am33xx_epwmss0__eqep0 = {
+	.master		= &am33xx_epwmss0_hwmod,
+	.slave		= &am33xx_eqep0_hwmod,
 	.clk		= "l4ls_gclk",
-	.addr		= am33xx_ehrpwm2_addr_space,
+	.addr		= am33xx_eqep0_addr_space,
 	.user		= OCP_USER_MPU,
 };
 
-/*
- * Splitting the resources to handle access of PWMSS config space
- * and module specific part independently
- */
-static struct omap_hwmod_addr_space am33xx_ecap0_addr_space[] = {
-	{
-		.pa_start	= 0x48300000,
-		.pa_end		= 0x48300000 + SZ_16 - 1,
-		.flags		= ADDR_TYPE_RT
-	},
+static struct omap_hwmod_addr_space am33xx_ehrpwm0_addr_space[] = {
 	{
-		.pa_start	= 0x48300100,
-		.pa_end		= 0x48300100 + SZ_256 - 1,
-		.flags		= ADDR_TYPE_RT
+		.pa_start	= 0x48300200,
+		.pa_end		= 0x48300200 + SZ_128 - 1,
 	},
 	{ }
 };
 
-static struct omap_hwmod_ocp_if am33xx_l4_ls__ecap0 = {
-	.master		= &am33xx_l4_ls_hwmod,
-	.slave		= &am33xx_ecap0_hwmod,
+static struct omap_hwmod_ocp_if am33xx_epwmss0__ehrpwm0 = {
+	.master		= &am33xx_epwmss0_hwmod,
+	.slave		= &am33xx_ehrpwm0_hwmod,
 	.clk		= "l4ls_gclk",
-	.addr		= am33xx_ecap0_addr_space,
+	.addr		= am33xx_ehrpwm0_addr_space,
 	.user		= OCP_USER_MPU,
 };
 
-/*
- * Splitting the resources to handle access of PWMSS config space
- * and module specific part independently
- */
-static struct omap_hwmod_addr_space am33xx_ecap1_addr_space[] = {
+static struct omap_hwmod_addr_space am33xx_epwmss1_addr_space[] = {
 	{
 		.pa_start	= 0x48302000,
 		.pa_end		= 0x48302000 + SZ_16 - 1,
 		.flags		= ADDR_TYPE_RT
 	},
+	{ }
+};
+
+static struct omap_hwmod_ocp_if am33xx_l4_ls__epwmss1 = {
+	.master		= &am33xx_l4_ls_hwmod,
+	.slave		= &am33xx_epwmss1_hwmod,
+	.clk		= "l4ls_gclk",
+	.addr		= am33xx_epwmss1_addr_space,
+	.user		= OCP_USER_MPU,
+};
+
+static struct omap_hwmod_addr_space am33xx_ecap1_addr_space[] = {
 	{
 		.pa_start	= 0x48302100,
-		.pa_end		= 0x48302100 + SZ_256 - 1,
-		.flags		= ADDR_TYPE_RT
+		.pa_end		= 0x48302100 + SZ_128 - 1,
 	},
 	{ }
 };
 
-static struct omap_hwmod_ocp_if am33xx_l4_ls__ecap1 = {
-	.master		= &am33xx_l4_ls_hwmod,
+static struct omap_hwmod_ocp_if am33xx_epwmss1__ecap1 = {
+	.master		= &am33xx_epwmss1_hwmod,
 	.slave		= &am33xx_ecap1_hwmod,
 	.clk		= "l4ls_gclk",
 	.addr		= am33xx_ecap1_addr_space,
 	.user		= OCP_USER_MPU,
 };
 
-/*
- * Splitting the resources to handle access of PWMSS config space
- * and module specific part independently
- */
-static struct omap_hwmod_addr_space am33xx_ecap2_addr_space[] = {
+static struct omap_hwmod_addr_space am33xx_eqep1_addr_space[] = {
+	{
+		.pa_start	= 0x48302180,
+		.pa_end		= 0x48302180 + SZ_128 - 1,
+	},
+	{ }
+};
+
+static struct omap_hwmod_ocp_if am33xx_epwmss1__eqep1 = {
+	.master		= &am33xx_epwmss1_hwmod,
+	.slave		= &am33xx_eqep1_hwmod,
+	.clk		= "l4ls_gclk",
+	.addr		= am33xx_eqep1_addr_space,
+	.user		= OCP_USER_MPU,
+};
+
+static struct omap_hwmod_addr_space am33xx_ehrpwm1_addr_space[] = {
+	{
+		.pa_start	= 0x48302200,
+		.pa_end		= 0x48302200 + SZ_128 - 1,
+	},
+	{ }
+};
+
+static struct omap_hwmod_ocp_if am33xx_epwmss1__ehrpwm1 = {
+	.master		= &am33xx_epwmss1_hwmod,
+	.slave		= &am33xx_ehrpwm1_hwmod,
+	.clk		= "l4ls_gclk",
+	.addr		= am33xx_ehrpwm1_addr_space,
+	.user		= OCP_USER_MPU,
+};
+
+static struct omap_hwmod_addr_space am33xx_epwmss2_addr_space[] = {
 	{
 		.pa_start	= 0x48304000,
 		.pa_end		= 0x48304000 + SZ_16 - 1,
 		.flags		= ADDR_TYPE_RT
 	},
+	{ }
+};
+
+static struct omap_hwmod_ocp_if am33xx_l4_ls__epwmss2 = {
+	.master		= &am33xx_l4_ls_hwmod,
+	.slave		= &am33xx_epwmss2_hwmod,
+	.clk		= "l4ls_gclk",
+	.addr		= am33xx_epwmss2_addr_space,
+	.user		= OCP_USER_MPU,
+};
+
+static struct omap_hwmod_addr_space am33xx_ecap2_addr_space[] = {
 	{
 		.pa_start	= 0x48304100,
-		.pa_end		= 0x48304100 + SZ_256 - 1,
-		.flags		= ADDR_TYPE_RT
+		.pa_end		= 0x48304100 + SZ_128 - 1,
 	},
 	{ }
 };
 
-static struct omap_hwmod_ocp_if am33xx_l4_ls__ecap2 = {
-	.master		= &am33xx_l4_ls_hwmod,
+static struct omap_hwmod_ocp_if am33xx_epwmss2__ecap2 = {
+	.master		= &am33xx_epwmss2_hwmod,
 	.slave		= &am33xx_ecap2_hwmod,
 	.clk		= "l4ls_gclk",
 	.addr		= am33xx_ecap2_addr_space,
 	.user		= OCP_USER_MPU,
 };
 
+static struct omap_hwmod_addr_space am33xx_eqep2_addr_space[] = {
+	{
+		.pa_start	= 0x48304180,
+		.pa_end		= 0x48304180 + SZ_128 - 1,
+	},
+	{ }
+};
+
+static struct omap_hwmod_ocp_if am33xx_epwmss2__eqep2 = {
+	.master		= &am33xx_epwmss2_hwmod,
+	.slave		= &am33xx_eqep2_hwmod,
+	.clk		= "l4ls_gclk",
+	.addr		= am33xx_eqep2_addr_space,
+	.user		= OCP_USER_MPU,
+};
+
+static struct omap_hwmod_addr_space am33xx_ehrpwm2_addr_space[] = {
+	{
+		.pa_start	= 0x48304200,
+		.pa_end		= 0x48304200 + SZ_128 - 1,
+	},
+	{ }
+};
+
+static struct omap_hwmod_ocp_if am33xx_epwmss2__ehrpwm2 = {
+	.master		= &am33xx_epwmss2_hwmod,
+	.slave		= &am33xx_ehrpwm2_hwmod,
+	.clk		= "l4ls_gclk",
+	.addr		= am33xx_ehrpwm2_addr_space,
+	.user		= OCP_USER_MPU,
+};
+
 /* l3s cfg -> gpmc */
 static struct omap_hwmod_addr_space am33xx_gpmc_addr_space[] = {
 	{
@@ -3356,12 +3483,18 @@ static struct omap_hwmod_ocp_if *am33xx_hwmod_ocp_ifs[] __initdata = {
 	&am33xx_l4_ls__uart6,
 	&am33xx_l4_ls__spinlock,
 	&am33xx_l4_ls__elm,
-	&am33xx_l4_ls__ehrpwm0,
-	&am33xx_l4_ls__ehrpwm1,
-	&am33xx_l4_ls__ehrpwm2,
-	&am33xx_l4_ls__ecap0,
-	&am33xx_l4_ls__ecap1,
-	&am33xx_l4_ls__ecap2,
+	&am33xx_l4_ls__epwmss0,
+	&am33xx_epwmss0__ecap0,
+	&am33xx_epwmss0__eqep0,
+	&am33xx_epwmss0__ehrpwm0,
+	&am33xx_l4_ls__epwmss1,
+	&am33xx_epwmss1__ecap1,
+	&am33xx_epwmss1__eqep1,
+	&am33xx_epwmss1__ehrpwm1,
+	&am33xx_l4_ls__epwmss2,
+	&am33xx_epwmss2__ecap2,
+	&am33xx_epwmss2__eqep2,
+	&am33xx_epwmss2__ehrpwm2,
 	&am33xx_l3_s__gpmc,
 	&am33xx_l3_main__lcdc,
 	&am33xx_l4_ls__mcspi0,
-- 
1.7.0.4

[PATCH v2 08/10] pwm: pwm-tiehrpwm: Adding TBCLK gating support.

[Philip, Avinash]

Some platforms (like AM33XX) requires clock gating from control module
explicitly for TBCLK. Enabling of this clock required for the
functioning of the time base sub module in EHRPWM module. So adding
optional TBCLK handling if DT node populated with tbclkgating. This
helps the driver can coexist for Davinci platforms.

Signed-off-by: Philip, Avinash <avinashphilip-l0cyMroinI0@public.gmane.org>
Cc:	Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
Cc: Rob Landley <rob-VoJi6FS/r0vR7s880joybQ@public.gmane.org>
---
Changes since v1:
	- Moved TBCLK enable from probe to .pwm_enable & disable from
	  remove to .pwm_disable

:100644 100644 07911e6... 927a8ed... M	drivers/pwm/pwm-tiehrpwm.c
 drivers/pwm/pwm-tiehrpwm.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index 07911e6..927a8ed 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -126,6 +126,7 @@ struct ehrpwm_pwm_chip {
 	void __iomem	*mmio_base;
 	unsigned long period_cycles[NUM_PWM_CHANNEL];
 	enum pwm_polarity polarity[NUM_PWM_CHANNEL];
+	struct	clk	*tbclk;
 };
 
 static inline struct ehrpwm_pwm_chip *to_ehrpwm_pwm_chip(struct pwm_chip *chip)
@@ -346,6 +347,13 @@ static int ehrpwm_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 	/* Channels polarity can be configured from action qualifier module */
 	configure_polarity(pc, pwm->hwpwm);
 
+	/*
+	 * Platforms require explicit clock enabling of TBCLK has
+	 * to enable TBCLK explicitly before enabling PWM device
+	 */
+	if (pc->tbclk)
+		clk_enable(pc->tbclk);
+
 	/* Enable time counter for free_run */
 	ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_RUN_MASK, TBCTL_FREE_RUN);
 	return 0;
@@ -374,6 +382,10 @@ static void ehrpwm_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 
 	ehrpwm_modify(pc->mmio_base, AQCSFRC, aqcsfrc_mask, aqcsfrc_val);
 
+	/* Disabling TBCLK on PWM disable */
+	if (pc->tbclk)
+		clk_disable(pc->tbclk);
+
 	/* Stop Time base counter */
 	ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_RUN_MASK, TBCTL_STOP_NEXT);
 
@@ -464,6 +476,16 @@ static int __devinit ehrpwm_pwm_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
 		return ret;
 	}
+
+	/* Some platforms require explicit tbclk gating */
+	if (of_property_read_bool(pdev->dev.of_node, "tbclkgating")) {
+		pc->tbclk = clk_get(&pdev->dev, "tbclk");
+		if (IS_ERR(pc->tbclk)) {
+			dev_err(&pdev->dev, "Could not get EHRPWM TBCLK\n");
+			return PTR_ERR(pc->tbclk);
+		}
+	}
+
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_get_sync(&pdev->dev);
 	if (!(pwmss_submodule_state_change(pdev->dev.parent, EPWMCLK_EN) &
-- 
1.7.0.4

Re: [PATCH v2 08/10] pwm: pwm-tiehrpwm: Adding TBCLK gating support.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2946 bytes --]

On Thu, Nov 08, 2012 at 01:23:15PM +0530, Philip, Avinash wrote:
> Some platforms (like AM33XX) requires clock gating from control module
> explicitly for TBCLK. Enabling of this clock required for the
> functioning of the time base sub module in EHRPWM module. So adding
> optional TBCLK handling if DT node populated with tbclkgating. This
> helps the driver can coexist for Davinci platforms.
> 
> Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> Cc:	Grant Likely <grant.likely@secretlab.ca>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Rob Landley <rob@landley.net>
> ---
> Changes since v1:
> 	- Moved TBCLK enable from probe to .pwm_enable & disable from
> 	  remove to .pwm_disable
> 
> :100644 100644 07911e6... 927a8ed... M	drivers/pwm/pwm-tiehrpwm.c
>  drivers/pwm/pwm-tiehrpwm.c |   22 ++++++++++++++++++++++
>  1 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
> index 07911e6..927a8ed 100644
> --- a/drivers/pwm/pwm-tiehrpwm.c
> +++ b/drivers/pwm/pwm-tiehrpwm.c
> @@ -126,6 +126,7 @@ struct ehrpwm_pwm_chip {
>  	void __iomem	*mmio_base;
>  	unsigned long period_cycles[NUM_PWM_CHANNEL];
>  	enum pwm_polarity polarity[NUM_PWM_CHANNEL];
> +	struct	clk	*tbclk;
>  };
>  
>  static inline struct ehrpwm_pwm_chip *to_ehrpwm_pwm_chip(struct pwm_chip *chip)
> @@ -346,6 +347,13 @@ static int ehrpwm_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	/* Channels polarity can be configured from action qualifier module */
>  	configure_polarity(pc, pwm->hwpwm);
>  
> +	/*
> +	 * Platforms require explicit clock enabling of TBCLK has
> +	 * to enable TBCLK explicitly before enabling PWM device
> +	 */
> +	if (pc->tbclk)
> +		clk_enable(pc->tbclk);
> +
>  	/* Enable time counter for free_run */
>  	ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_RUN_MASK, TBCTL_FREE_RUN);
>  	return 0;
> @@ -374,6 +382,10 @@ static void ehrpwm_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  
>  	ehrpwm_modify(pc->mmio_base, AQCSFRC, aqcsfrc_mask, aqcsfrc_val);
>  
> +	/* Disabling TBCLK on PWM disable */
> +	if (pc->tbclk)
> +		clk_disable(pc->tbclk);
> +
>  	/* Stop Time base counter */
>  	ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_RUN_MASK, TBCTL_STOP_NEXT);
>  
> @@ -464,6 +476,16 @@ static int __devinit ehrpwm_pwm_probe(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
>  		return ret;
>  	}
> +
> +	/* Some platforms require explicit tbclk gating */
> +	if (of_property_read_bool(pdev->dev.of_node, "tbclkgating")) {

Is it really necessary to have an extra boolean property for this?
Couldn't this just be handled by not defining a clock for the tbclk
consumer in board setup/DT

> +		pc->tbclk = clk_get(&pdev->dev, "tbclk");

You should be using devm_clk_get() or add a matching clk_put() in
.remove().

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH v2 08/10] pwm: pwm-tiehrpwm: Adding TBCLK gating support.

[Philip, Avinash]

On Fri, Nov 09, 2012 at 13:41:04, Thierry Reding wrote:
> On Thu, Nov 08, 2012 at 01:23:15PM +0530, Philip, Avinash wrote:
...
> > +	/* Some platforms require explicit tbclk gating */
> > +	if (of_property_read_bool(pdev->dev.of_node, "tbclkgating")) {
> 
> Is it really necessary to have an extra boolean property for this?
> Couldn't this just be handled by not defining a clock for the tbclk
> consumer in board setup/DT

If no clk node defined, driver can still continue expecting this platform 
the requirement is not there. So I will check for tbclk with devm_clk_get()
and continue by removing Boolean property.

> 
> > +		pc->tbclk = clk_get(&pdev->dev, "tbclk");
> 
> You should be using devm_clk_get() or add a matching clk_put() in
> .remove().

Ok

Thanks
Avinash

> 
> Thierry
> 

[PATCH v2 09/10] ARM: dts: AM33XX: Add PWMSS device tree nodes

[Philip, Avinash]

Add PWMSS device tree nodes in relation with ECAP & EHRPWM DT nodes to
AM33XX SoC family. Also populates device tree nodes for ECAP & EHRPWM by
adding necessary properties like pwm-cells, base reg & set disabled as
status.

Signed-off-by: Philip, Avinash <avinashphilip-l0cyMroinI0@public.gmane.org>
---
:100644 100644 bb31bff... cf5e049... M	arch/arm/boot/dts/am33xx.dtsi
 arch/arm/boot/dts/am33xx.dtsi |   90 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 90 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index bb31bff..cf5e049 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -210,5 +210,95 @@
 			interrupt-parent = <&intc>;
 			interrupts = <91>;
 		};
+
+		epwmss0: epwmss@48300000 {
+			compatible = "ti,am33xx-pwmss";
+			reg = <0x48300000 0x10
+				0x48300100 0x80
+				0x48300180 0x80
+				0x48300200 0x80>;
+			ti,hwmods = "epwmss0";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			status = "disabled";
+			ranges;
+
+			ecap0: ecap@48300100 {
+				compatible = "ti,am33xx-ecap";
+				#pwm-cells = <3>;
+				reg = <0x48300100 0x80>;
+				ti,hwmods = "ecap0";
+				status = "disabled";
+			};
+
+			ehrpwm0: ehrpwm@48300200 {
+				compatible = "ti,am33xx-ehrpwm";
+				#pwm-cells = <3>;
+				reg = <0x48300200 0x80>;
+				ti,hwmods = "ehrpwm0";
+				status = "disabled";
+				tbclkgating;
+			};
+		};
+
+		epwmss1: epwmss@48302000 {
+			compatible = "ti,am33xx-pwmss";
+			reg = <0x48302000 0x10
+				0x48302100 0x80
+				0x48302180 0x80
+				0x48302200 0x80>;
+			ti,hwmods = "epwmss1";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			status = "disabled";
+			ranges;
+
+			ecap1: ecap@48302100 {
+				compatible = "ti,am33xx-ecap";
+				#pwm-cells = <3>;
+				reg = <0x48302100 0x80>;
+				ti,hwmods = "ecap1";
+				status = "disabled";
+			};
+
+			ehrpwm1: ehrpwm@48302200 {
+				compatible = "ti,am33xx-ehrpwm";
+				#pwm-cells = <3>;
+				reg = <0x48302200 0x80>;
+				ti,hwmods = "ehrpwm1";
+				status = "disabled";
+				tbclkgating;
+			};
+		};
+
+		epwmss2: epwmss@48304000 {
+			compatible = "ti,am33xx-pwmss";
+			reg = <0x48304000 0x10
+				0x48304100 0x80
+				0x48304180 0x80
+				0x48304200 0x80>;
+			ti,hwmods = "epwmss2";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			status = "disabled";
+			ranges;
+
+			ecap2: ecap@48304100 {
+				compatible = "ti,am33xx-ecap";
+				#pwm-cells = <3>;
+				reg = <0x48304100 0x80>;
+				ti,hwmods = "ecap2";
+				status = "disabled";
+			};
+
+			ehrpwm2: ehrpwm@48304200 {
+				compatible = "ti,am33xx-ehrpwm";
+				#pwm-cells = <3>;
+				reg = <0x48304200 0x80>;
+				ti,hwmods = "ehrpwm2";
+				status = "disabled";
+				tbclkgating;
+			};
+		};
 	};
 };
-- 
1.7.0.4

[PATCH v2 10/10] ARM: dts: AM33XX: Add PWM backlight DT data to am335x-evm

[Philip, Avinash]

PWM output from ecap0 uses as backlight source. Also adds low threshold
value to have a uniform divisions in brightness-levels scales.

Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
---
:100644 100644 185d632... 9857050... M	arch/arm/boot/dts/am335x-evm.dts
 arch/arm/boot/dts/am335x-evm.dts |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
index 185d632..9857050 100644
--- a/arch/arm/boot/dts/am335x-evm.dts
+++ b/arch/arm/boot/dts/am335x-evm.dts
@@ -18,6 +18,14 @@
 		reg = <0x80000000 0x10000000>; /* 256 MB */
 	};
 
+	am33xx_pinmux: pinmux@44e10800 {
+		ecap0_pins: backlight_pins {
+			pinctrl-single,pins = <
+				0x164 0x0	/* eCAP0_in_PWM0_out.eCAP0_in_PWM0_out MODE0 */
+			>;
+		};
+	};
+
 	ocp {
 		uart1: serial@44e09000 {
 			status = "okay";
@@ -31,6 +39,12 @@
 				reg = <0x2d>;
 			};
 		};
+
+		ecap0: ecap@48300100 {
+			status = "okay";
+			pinctrl-names = "default";
+			pinctrl-0 = <&ecap0_pins>;
+		};
 	};
 
 	vbat: fixedregulator@0 {
@@ -40,6 +54,13 @@
 		regulator-max-microvolt = <5000000>;
 		regulator-boot-on;
 	};
+
+	backlight {
+		compatible	= "pwm-backlight";
+		pwms = <&ecap0 0 50000 0>;
+		brightness-levels = <0 51 53 56 62 75 101 152 255>;
+		default-brightness-level = <8>;
+	};
 };
 
 /include/ "tps65910.dtsi"
-- 
1.7.0.4