[PATCH v2 0/2] This series adds support for Rockchip SoCs integrated PWM.

[PATCH v2 0/2] This series adds support for Rockchip SoCs integrated PWM.

[Caesar Wang]

This patch will make applying on the top of Beniamino's submission,
the Beniamino's submission come from [1].

[1]:
https://git.kernel.org/cgit/linux/kernel/git/thierry.reding/linux-pwm.git/log/?h=for-next

Beniamino's submission won't be used from genenation Rockchip SoCs.
So I have to add the new pwm for next genenation Rockchip SoCs.

Tested on RK3288 SDK board.

Changes in v2:
    * address comments from Beniamino Galvani Reding:
      - remove #include <linux/of_address.h>. 
      - of_iomap be removed,and devm_ioremap replace it.
      - remove a line no be used.

Caesar Wang (2):
  [PATCH v2 1/2] pwm: add this patch to introduce for rk-pwm and vop-pwm

  [PATCH v2 2/2] pwm: add this patch to support the new pwm of Rockchip SoCs

 .../devicetree/bindings/pwm/pwm-rockchip.txt       | 13 +--
 drivers/pwm/pwm-rockchip.c                         | 97 ++++++++++++++++++----
 2 files changed, 87 insertions(+), 23 deletions(-)

-- 
1.9.1

[PATCH v2 1/2] pwm: add this patch to introduce for rk-pwm and vop-pwm

[Caesar Wang]

Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
---
 Documentation/devicetree/bindings/pwm/pwm-rockchip.txt | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt b/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
index 3182126..bb6e7f1 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
@@ -1,11 +1,14 @@
 Rockchip PWM controller
 
 Required properties:
- - compatible: should be "rockchip,rk2928-pwm"
- - reg: physical base address and length of the controller's registers
- - clocks: phandle and clock specifier of the PWM reference clock
- - #pwm-cells: should be 2. See pwm.txt in this directory for a
-   description of the cell format.
+- compatible: should be "rockchip,<name>-pwm"should be one of following:
+    "rockchip,rk2928-pwm" - support the old version PWM for RK29xx,RK3066,RK3188 SoCs.
+    "rockchip,rk3288-pwm" - support the new version PWM for RK3036,RK3288 SoCs.
+    "rockchip,vop-pwm" - support rockchip integrated VOP-PWM for RK32 SoCs.
+- reg: physical base address and length of the controller's registers.
+- clocks: phandle and clock specifier of the PWM reference clock.
+- #pwm-cells: should be 2. See pwm.txt in this directory for a
+  description of the cell format.
 
 Example:
 
-- 
1.9.1

Re: [PATCH v2 1/2] pwm: add this patch to introduce for rk-pwm and vop-pwm

[Thierry Reding]

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

On Sat, Jul 19, 2014 at 08:55:28PM +0800, Caesar Wang wrote:
> Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
> ---
>  Documentation/devicetree/bindings/pwm/pwm-rockchip.txt | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt b/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
> index 3182126..bb6e7f1 100644
> --- a/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
> +++ b/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
> @@ -1,11 +1,14 @@
>  Rockchip PWM controller
>  
>  Required properties:
> - - compatible: should be "rockchip,rk2928-pwm"
> - - reg: physical base address and length of the controller's registers
> - - clocks: phandle and clock specifier of the PWM reference clock
> - - #pwm-cells: should be 2. See pwm.txt in this directory for a
> -   description of the cell format.

Can you please stick to the original format rather than removing the
space in the first column? It makes it difficult to see what's being
changed by this patch.

> +- compatible: should be "rockchip,<name>-pwm"should be one of following:

I think it should be either 'should be "rockchip,<name>-pwm"' or 'should
be one of the following', not both.

> +    "rockchip,rk2928-pwm" - support the old version PWM for RK29xx,RK3066,RK3188 SoCs.
> +    "rockchip,rk3288-pwm" - support the new version PWM for RK3036,RK3288 SoCs.
> +    "rockchip,vop-pwm" - support rockchip integrated VOP-PWM for RK32 SoCs.

"old version PWM" and "new version PWM" can be dropped. Maybe:

	"rockchip,rk2928-pwm": found on RK29xx, RK3066 and RK3188 SoCs
	"rockchip,rk3288-pwm": found on RK3036 and RK3288 SoCs
	"rockchip,vop-pwm": found integrated in VOP on RK32xx SoCs

? Could you explain a little what this VOP is? Also since the above says
that "rockchip,rk3288-pwm" is supported on RK3036, the compatible value
should really be "rockchip,rk3036-pwm" since we usually list the chip
that an IP block was first used in (I'm assuming here that RK3036 is
older than RK3288).

> +- reg: physical base address and length of the controller's registers.
> +- clocks: phandle and clock specifier of the PWM reference clock.
> +- #pwm-cells: should be 2. See pwm.txt in this directory for a
> +  description of the cell format.

These are all unchanged (except for the first column's space being
removed and the full-stops for reg and clocks properties). Please keep
with the original formatting so that it becomes obvious what the patch
changes.

Thierry

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

Re: [PATCH v2 1/2] pwm: add this patch to introduce for rk-pwm and vop-pwm

[caesar]

Hi Thierry,

于 2014年07月21日 16:57, Thierry Reding 写道:
> On Sat, Jul 19, 2014 at 08:55:28PM +0800, Caesar Wang wrote:
>> Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
>> ---
>>   Documentation/devicetree/bindings/pwm/pwm-rockchip.txt | 13 ++++++++-----
>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt b/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
>> index 3182126..bb6e7f1 100644
>> --- a/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
>> +++ b/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
>> @@ -1,11 +1,14 @@
>>   Rockchip PWM controller
>>   
>>   Required properties:
>> - - compatible: should be "rockchip,rk2928-pwm"
>> - - reg: physical base address and length of the controller's registers
>> - - clocks: phandle and clock specifier of the PWM reference clock
>> - - #pwm-cells: should be 2. See pwm.txt in this directory for a
>> -   description of the cell format.
> Can you please stick to the original format rather than removing the
> space in the first column? It makes it difficult to see what's being
> changed by this patch.
ok，
>> +- compatible: should be "rockchip,<name>-pwm"should be one of following:
> I think it should be either 'should be "rockchip,<name>-pwm"' or 'should
> be one of the following', not both.
Seems reasonable.
>
>> +    "rockchip,rk2928-pwm" - support the old version PWM for RK29xx,RK3066,RK3188 SoCs.
>> +    "rockchip,rk3288-pwm" - support the new version PWM for RK3036,RK3288 SoCs.
>> +    "rockchip,vop-pwm" - support rockchip integrated VOP-PWM for RK32 SoCs.
> "old version PWM" and "new version PWM" can be dropped. Maybe:
>
> 	"rockchip,rk2928-pwm": found on RK29xx, RK3066 and RK3188 SoCs
> 	"rockchip,rk3288-pwm": found on RK3036 and RK3288 SoCs
> 	"rockchip,vop-pwm": found integrated in VOP on RK32xx SoCs
>
> ? Could you explain a little what this VOP is? Also since the above says
> that "rockchip,rk3288-pwm" is supported on RK3036, the compatible value
> should really be "rockchip,rk3036-pwm" since we usually list the chip
> that an IP block was first used in (I'm assuming here that RK3036 is
> older than RK3288).
the SoC rk3036 is newer than rk3288. Maybe I will remove the rk3036.
It's the genenation Soc.

VOP is the display interface from memory frame buffer to display device 
(LCD panel, LVDS,MIPI, eDP, HDMI and TV set).
VOP is connected to an AHB bus through an AHB slave and AXI bus through 
an AXI master.
The register setting is configured through the AHB slave interface and 
the display frame data
is read through the AXI master interface.
Furthermore, there is adata path between IEP and VOP, which can provide 
frame data from IEP to VOP.

If you are interested in it,I will sent the vop doc for you and explain it.
>> +- reg: physical base address and length of the controller's registers.
>> +- clocks: phandle and clock specifier of the PWM reference clock.
>> +- #pwm-cells: should be 2. See pwm.txt in this directory for a
>> +  description of the cell format.
> These are all unchanged (except for the first column's space being
> removed and the full-stops for reg and clocks properties). Please keep
> with the original formatting so that it becomes obvious what the patch
> changes.
>
> Thierry
I will fix this and the other issuses in v3,thanks.

Caesar

[PATCH v2 2/2] pwm: add this patch to support the new pwm of Rockchip SoCs

[Caesar Wang]

Suggested-by: Beniamino Galvani <b.galvani@gmail.com>
Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
---
 drivers/pwm/pwm-rockchip.c | 97 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 79 insertions(+), 18 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index eec2145..3628a1b 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -2,6 +2,7 @@
  * PWM driver for Rockchip SoCs
  *
  * Copyright (C) 2014 Beniamino Galvani <b.galvani@gmail.com>
+ * Copyright (C) 2014 Caesar Wang <caesar.wang@rock-chips.com>
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -12,6 +13,7 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
 #include <linux/time.h>
@@ -23,14 +25,36 @@
 #define PWM_CTRL_TIMER_EN	(1 << 0)
 #define PWM_CTRL_OUTPUT_EN	(1 << 3)
 
+#define RK_PRESCALER		1
 #define PRESCALER		2
 
+#define PWM_CONTINUOUS		(1 << 1)
+#define PWM_DUTY_POSITIVE	(1 << 3)
+#define PWM_INACTIVE_NEGATIVE	(0 << 4)
+#define PWM_OUTPUT_LEFT		(0 << 5)
+#define PWM_LP_DISABLE		(0 << 8)
+
+#define RK_PWM_ENABLE		(1 << 0)
+
+#define VOP_PWM_CTRL		0x00		/* VOP-PWM Control Register */
+#define VOP_PWM_CNTR		0x0c
+
 struct rockchip_pwm_chip {
 	struct pwm_chip chip;
 	struct clk *clk;
+	const struct rockchip_pwm_data *data;
 	void __iomem *base;
 };
 
+struct rockchip_pwm_data {
+	u32 duty;
+	u32 period;
+	u32 reg_cntr;
+	u32 reg_ctrl;
+	int prescaler;
+	u32 enable_conf;
+};
+
 static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c)
 {
 	return container_of(c, struct rockchip_pwm_chip, chip);
@@ -52,20 +76,20 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * default prescaler value for all practical clock rate values.
 	 */
 	div = clk_rate * period_ns;
-	do_div(div, PRESCALER * NSEC_PER_SEC);
+	do_div(div, pc->data->prescaler * NSEC_PER_SEC);
 	period = div;
 
 	div = clk_rate * duty_ns;
-	do_div(div, PRESCALER * NSEC_PER_SEC);
+	do_div(div, pc->data->prescaler * NSEC_PER_SEC);
 	duty = div;
 
 	ret = clk_enable(pc->clk);
 	if (ret)
 		return ret;
 
-	writel(period, pc->base + PWM_LRC);
-	writel(duty, pc->base + PWM_HRC);
-	writel(0, pc->base + PWM_CNTR);
+	writel(period, pc->base + pc->data->period);
+	writel(duty, pc->base + pc->data->duty);
+	writel(0, pc->base + pc->data->reg_cntr);
 
 	clk_disable(pc->clk);
 
@@ -82,9 +106,9 @@ static int rockchip_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 	if (ret)
 		return ret;
 
-	val = readl_relaxed(pc->base + PWM_CTRL);
-	val |= PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN;
-	writel_relaxed(val, pc->base + PWM_CTRL);
+	val = readl_relaxed(pc->base + pc->data->reg_ctrl);
+	val |= pc->data->enable_conf;
+	writel_relaxed(val, pc->base + pc->data->reg_ctrl);
 
 	return 0;
 }
@@ -94,9 +118,9 @@ static void rockchip_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
 	u32 val;
 
-	val = readl_relaxed(pc->base + PWM_CTRL);
-	val &= ~(PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN);
-	writel_relaxed(val, pc->base + PWM_CTRL);
+	val = readl_relaxed(pc->base + pc->data->reg_ctrl);
+	val &= ~(pc->data->enable_conf);
+	writel_relaxed(val, pc->base + pc->data->reg_ctrl);
 
 	clk_disable(pc->clk);
 }
@@ -108,8 +132,47 @@ static const struct pwm_ops rockchip_pwm_ops = {
 	.owner = THIS_MODULE,
 };
 
+static struct rockchip_pwm_data pwm_data_v1 = {
+	.duty = PWM_HRC,
+	.period = PWM_LRC,
+	.reg_cntr = PWM_CNTR,
+	.reg_ctrl = PWM_CTRL,
+	.prescaler = PRESCALER,
+	.enable_conf = PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN,
+};
+
+static struct rockchip_pwm_data pwm_data_v2 = {
+	.duty = PWM_LRC,
+	.period = PWM_HRC,
+	.reg_cntr = PWM_CNTR,
+	.reg_ctrl = PWM_CTRL,
+	.prescaler = RK_PRESCALER,
+	.enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | RK_PWM_ENABLE |
+		PWM_CONTINUOUS | PWM_DUTY_POSITIVE | PWM_INACTIVE_NEGATIVE,
+};
+
+static struct rockchip_pwm_data pwm_data_vop = {
+	.duty = PWM_LRC,
+	.period = PWM_HRC,
+	.reg_cntr = VOP_PWM_CNTR,
+	.reg_ctrl = VOP_PWM_CTRL,
+	.prescaler = RK_PRESCALER,
+	.enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | RK_PWM_ENABLE |
+		PWM_CONTINUOUS | PWM_DUTY_POSITIVE | PWM_INACTIVE_NEGATIVE,
+};
+
+static const struct of_device_id rockchip_pwm_dt_ids[] = {
+	{ .compatible = "rockchip,rk2928-pwm", .data = &pwm_data_v1},
+	{ .compatible = "rockchip,rk3288-pwm", .data = &pwm_data_v2},
+	{ .compatible = "rockchip,vop-pwm", .data = &pwm_data_vop},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rockchip_pwm_dt_ids);
+
 static int rockchip_pwm_probe(struct platform_device *pdev)
 {
+	const struct of_device_id *of_id =
+	    of_match_device(rockchip_pwm_dt_ids, &pdev->dev);
 	struct rockchip_pwm_chip *pc;
 	struct resource *r;
 	int ret;
@@ -119,7 +182,10 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	pc->base = devm_ioremap_resource(&pdev->dev, r);
+	if (!strcmp(of_id->compatible, "rockchip,vop-pwm"))
+		pc->base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
+	else
+		pc->base = devm_ioremap_resource(&pdev->dev, r);
 	if (IS_ERR(pc->base))
 		return PTR_ERR(pc->base);
 
@@ -133,6 +199,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, pc);
 
+	pc->data = of_id->data;
 	pc->chip.dev = &pdev->dev;
 	pc->chip.ops = &rockchip_pwm_ops;
 	pc->chip.base = -1;
@@ -156,12 +223,6 @@ static int rockchip_pwm_remove(struct platform_device *pdev)
 	return pwmchip_remove(&pc->chip);
 }
 
-static const struct of_device_id rockchip_pwm_dt_ids[] = {
-	{ .compatible = "rockchip,rk2928-pwm" },
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, rockchip_pwm_dt_ids);
-
 static struct platform_driver rockchip_pwm_driver = {
 	.driver = {
 		.name = "rockchip-pwm",
-- 
1.9.1

Re: [PATCH v2 2/2] pwm: add this patch to support the new pwm of Rockchip SoCs

[Thierry Reding]

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

On Sat, Jul 19, 2014 at 08:55:29PM +0800, Caesar Wang wrote:
> Suggested-by: Beniamino Galvani <b.galvani@gmail.com>
> Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
> ---
>  drivers/pwm/pwm-rockchip.c | 97 +++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 79 insertions(+), 18 deletions(-)

This patch is missing a proper description. Also "pwm" should be spelled
"PWM" in prose.

And the subject is somewhat long and generic. How about:

	pwm: rockchip: Add support for RK3288 SoCs

?

> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
[...]
> @@ -12,6 +13,7 @@
>  #include <linux/io.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/pwm.h>
>  #include <linux/time.h>
> @@ -23,14 +25,36 @@
>  #define PWM_CTRL_TIMER_EN	(1 << 0)
>  #define PWM_CTRL_OUTPUT_EN	(1 << 3)
>  
> +#define RK_PRESCALER		1
>  #define PRESCALER		2

I don't think there's a need to keep these around now that they're part
of the rockchip_pwm_data structure.

> +#define PWM_CONTINUOUS		(1 << 1)
> +#define PWM_DUTY_POSITIVE	(1 << 3)
> +#define PWM_INACTIVE_NEGATIVE	(0 << 4)
> +#define PWM_OUTPUT_LEFT		(0 << 5)
> +#define PWM_LP_DISABLE		(0 << 8)
> +
> +#define RK_PWM_ENABLE		(1 << 0)

I think you can drop the RK_ prefix here since none of the above have it
either and they're used in the same context.

> +
> +#define VOP_PWM_CTRL		0x00		/* VOP-PWM Control Register */
> +#define VOP_PWM_CNTR		0x0c

Similarly for these.

> +
>  struct rockchip_pwm_chip {
>  	struct pwm_chip chip;
>  	struct clk *clk;
> +	const struct rockchip_pwm_data *data;
>  	void __iomem *base;
>  };
>  
> +struct rockchip_pwm_data {
> +	u32 duty;
> +	u32 period;
> +	u32 reg_cntr;
> +	u32 reg_ctrl;

duty and period are register offsets, right? Why don't they have the
reg_ prefix? Alternatively you could drop the reg_ prefix altogether.
Yet another alternative would be to introduce a substructure that
contains the registers, to separate the fields logically:

	struct rockchip_pwm_data {
		struct {
			unsigned long duty;
			unsigned long period;
			unsigned long cntr;
			unsigned long ctrl;
		} regs;
		...
	};

As shown, I think it's more natural for register offsets to be unsigned
long rather than a strictly-sized type.

> +	int prescaler;

unsigned int

> +	u32 enable_conf;
> +};

For this I think it would be more readable to provide function pointers
rather than a variable. That is:

	struct rockchip_pwm_data {
		...
		int (*enable)(struct pwm_chip *chip, struct pwm_device *pwm);
		int (*disable)(struct pwm_chip *chip, struct pwm_device *pwm);
	};

Then you can implement these for each variant of the chip and call them
from the common rockchip_pwm_enable(), somewhat like this:

	static int rockchip_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
	{
		struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
		int ret;

		ret = clk_enable(pc->clk);
		if (ret)
			return ret;

		ret = pc->data->enable(chip, pwm);
		if (ret)
			clk_disable(pc->clk);

		return ret;
	}

And similarly for rockchip_pwm_disable().

> +static struct rockchip_pwm_data pwm_data_v1 = {

static const please.

> +static struct rockchip_pwm_data pwm_data_v2 = {

Here too.

> +static struct rockchip_pwm_data pwm_data_vop = {

And here.

> +	const struct of_device_id *of_id =

I don't think the of_ prefix is necessary here.

> +	    of_match_device(rockchip_pwm_dt_ids, &pdev->dev);

And perhaps rather than split this over two lines here you can move this
assignment somewhere below the variable declarations?

>  	struct rockchip_pwm_chip *pc;
>  	struct resource *r;
>  	int ret;
> @@ -119,7 +182,10 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	pc->base = devm_ioremap_resource(&pdev->dev, r);
> +	if (!strcmp(of_id->compatible, "rockchip,vop-pwm"))
> +		pc->base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
> +	else
> +		pc->base = devm_ioremap_resource(&pdev->dev, r);

Sorry, this still isn't an option. You really shouldn't remap I/O
regions that other drivers may be using. You hinted at a shared register
space during the review of the initial version. Can you provide more
detail about what exactly the memory map looks like of the rk3288? Is
there some kind of technical reference manual that I could look at? Or
do you have a device tree extract that shows what the memory map looks
like?

Thierry

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

Re: [PATCH v2 2/2] pwm: add this patch to support the new pwm of Rockchip SoCs

[Thierry Reding]

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

On Mon, Jul 21, 2014 at 08:58:42PM +0800, caesar wrote:
> 于 2014年07月21日 16:50, Thierry Reding 写道:
> >On Sat, Jul 19, 2014 at 08:55:29PM +0800, Caesar Wang wrote:
[...]
> >>  	struct rockchip_pwm_chip *pc;
> >>  	struct resource *r;
> >>  	int ret;
> >>@@ -119,7 +182,10 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
> >>  		return -ENOMEM;
> >>  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >>-	pc->base = devm_ioremap_resource(&pdev->dev, r);
> >>+	if (!strcmp(of_id->compatible, "rockchip,vop-pwm"))
> >>+		pc->base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
> >>+	else
> >>+		pc->base = devm_ioremap_resource(&pdev->dev, r);
> >Sorry, this still isn't an option. You really shouldn't remap I/O
> >regions that other drivers may be using. You hinted at a shared register
> >space during the review of the initial version. Can you provide more
> >detail about what exactly the memory map looks like of the rk3288? Is
> >there some kind of technical reference manual that I could look at? Or
> >do you have a device tree extract that shows what the memory map looks
> >like?
> >
> >Thierry
> Maybe,you can look at the ARM: dts: rk3288:
> https://github.com/rkchrome/kernel/blob/master/arch/arm/boot/dts/rk3288.dtsi
> There is some lcdc and vop-pwm map address for rk3288.
> 
> ,and you can look at the vop-introduce.pdf and vop-register.pdf in Annex.
> 
> Maybe,I should put the vop-pwm in lcdc driver,but I don't hope do so it.
> 
> Could you give a suggestion to solve it? Thanks.

It looks like you could turn the lcdc device into an MFD device so that
it can instantiate two devices, one for the display controller, the
other for the PWM. Or perhaps it would even work with only a single
child device.

The device tree would become something like this:

	lcdc@ff930000 {
		compatible = "rockchip,rk3288-lcdc";
		...

		pwm@ff9301a0 {
			compatible = "rockchip,vop-pwm";
			...
		};
	};

And your driver would do something like:

	static const struct resource pwm_resources[] = {
		{
			.start = 0x1a0,
			.end = 0x1af,
			.flags = IORESOURCE_MEM,
		},
	};

	static const struct mfd_cell subdevices[] = {
		{
			.name = "pwm",
			.id = 1,
			.of_compatible = "rockchip,vop-pwm",
			.num_resources = ARRAY_SIZE(pwm_resources),
			.resources = pwm_resources,
		},
	};

	static int lcdc_probe(struct platform_device *pdev)
	{
		struct resource *regs;
		...

		regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);

		...

		err = mfd_add_devices(&pdev->dev, 0, subdevices, ARRAY_SIZE(subdevices),
				      regs, NULL, NULL);
		...
	}

Thierry

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

Re: [PATCH v2 2/2] pwm: add this patch to support the new pwm of Rockchip SoCs

[caesar]

于 2014年07月21日 21:27, Thierry Reding 写道:
> On Mon, Jul 21, 2014 at 08:58:42PM +0800, caesar wrote:
>> 于 2014年07月21日 16:50, Thierry Reding 写道:
>>> On Sat, Jul 19, 2014 at 08:55:29PM +0800, Caesar Wang wrote:
> [...]
>>>>   	struct rockchip_pwm_chip *pc;
>>>>   	struct resource *r;
>>>>   	int ret;
>>>> @@ -119,7 +182,10 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>>>>   		return -ENOMEM;
>>>>   	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> -	pc->base = devm_ioremap_resource(&pdev->dev, r);
>>>> +	if (!strcmp(of_id->compatible, "rockchip,vop-pwm"))
>>>> +		pc->base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
>>>> +	else
>>>> +		pc->base = devm_ioremap_resource(&pdev->dev, r);
>>> Sorry, this still isn't an option. You really shouldn't remap I/O
>>> regions that other drivers may be using. You hinted at a shared register
>>> space during the review of the initial version. Can you provide more
>>> detail about what exactly the memory map looks like of the rk3288? Is
>>> there some kind of technical reference manual that I could look at? Or
>>> do you have a device tree extract that shows what the memory map looks
>>> like?
>>>
>>> Thierry
>> Maybe,you can look at the ARM: dts: rk3288:
>> https://github.com/rkchrome/kernel/blob/master/arch/arm/boot/dts/rk3288.dtsi
>> There is some lcdc and vop-pwm map address for rk3288.
>>
>> ,and you can look at the vop-introduce.pdf and vop-register.pdf in Annex.
>>
>> Maybe,I should put the vop-pwm in lcdc driver,but I don't hope do so it.
>>
>> Could you give a suggestion to solve it? Thanks.
> It looks like you could turn the lcdc device into an MFD device so that
> it can instantiate two devices, one for the display controller, the
> other for the PWM. Or perhaps it would even work with only a single
> child device.
>
> The device tree would become something like this:
>
> 	lcdc@ff930000 {
> 		compatible = "rockchip,rk3288-lcdc";
> 		...
>
> 		pwm@ff9301a0 {
> 			compatible = "rockchip,vop-pwm";
> 			...
> 		};
> 	};
>
> And your driver would do something like:
>
> 	static const struct resource pwm_resources[] = {
> 		{
> 			.start = 0x1a0,
> 			.end = 0x1af,
> 			.flags = IORESOURCE_MEM,
> 		},
> 	};
>
> 	static const struct mfd_cell subdevices[] = {
> 		{
> 			.name = "pwm",
> 			.id = 1,
> 			.of_compatible = "rockchip,vop-pwm",
> 			.num_resources = ARRAY_SIZE(pwm_resources),
> 			.resources = pwm_resources,
> 		},
> 	};
>
> 	static int lcdc_probe(struct platform_device *pdev)
> 	{
> 		struct resource *regs;
> 		...
>
> 		regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>
> 		...
>
> 		err = mfd_add_devices(&pdev->dev, 0, subdevices, ARRAY_SIZE(subdevices),
> 				      regs, NULL, NULL);
> 		...
> 	}
>
> Thierry

Seems resonable.

I will fix this and the other issues v3,Thanks.

Caesar

Re: [PATCH v2 2/2] pwm: add this patch to support the new pwm of Rockchip SoCs

[caesar]

Hi Thierry,



在 2014年07月21日 21:27, Thierry Reding 写道:
> On Mon, Jul 21, 2014 at 08:58:42PM +0800, caesar wrote:
>> 于 2014年07月21日 16:50, Thierry Reding 写道:
>>> On Sat, Jul 19, 2014 at 08:55:29PM +0800, Caesar Wang wrote:
> [...]
>>>>   	struct rockchip_pwm_chip *pc;
>>>>   	struct resource *r;
>>>>   	int ret;
>>>> @@ -119,7 +182,10 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>>>>   		return -ENOMEM;
>>>>   	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> -	pc->base = devm_ioremap_resource(&pdev->dev, r);
>>>> +	if (!strcmp(of_id->compatible, "rockchip,vop-pwm"))
>>>> +		pc->base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
>>>> +	else
>>>> +		pc->base = devm_ioremap_resource(&pdev->dev, r);
>>> Sorry, this still isn't an option. You really shouldn't remap I/O
>>> regions that other drivers may be using. You hinted at a shared register
>>> space during the review of the initial version. Can you provide more
>>> detail about what exactly the memory map looks like of the rk3288? Is
>>> there some kind of technical reference manual that I could look at? Or
>>> do you have a device tree extract that shows what the memory map looks
>>> like?
>>>
>>> Thierry
>> Maybe,you can look at the ARM: dts: rk3288:
>> https://github.com/rkchrome/kernel/blob/master/arch/arm/boot/dts/rk3288.dtsi
>> There is some lcdc and vop-pwm map address for rk3288.
>>
>> ,and you can look at the vop-introduce.pdf and vop-register.pdf in Annex.
>>
>> Maybe,I should put the vop-pwm in lcdc driver,but I don't hope do so it.
>>
>> Could you give a suggestion to solve it? Thanks.
> It looks like you could turn the lcdc device into an MFD device so that
> it can instantiate two devices, one for the display controller, the
> other for the PWM. Or perhaps it would even work with only a single
> child device.
>
> The device tree would become something like this:
>
> 	lcdc@ff930000 {
> 		compatible = "rockchip,rk3288-lcdc";
> 		...
>
> 		pwm@ff9301a0 {
> 			compatible = "rockchip,vop-pwm";
> 			...
> 		};
> 	};
>
> And your driver would do something like:
>
> 	static const struct resource pwm_resources[] = {
> 		{
> 			.start = 0x1a0,
> 			.end = 0x1af,
> 			.flags = IORESOURCE_MEM,
> 		},
> 	};
>
> 	static const struct mfd_cell subdevices[] = {
> 		{
> 			.name = "pwm",
> 			.id = 1,
> 			.of_compatible = "rockchip,vop-pwm",
> 			.num_resources = ARRAY_SIZE(pwm_resources),
> 			.resources = pwm_resources,
> 		},
> 	};
>
> 	static int lcdc_probe(struct platform_device *pdev)
> 	{
> 		struct resource *regs;
> 		...
>
> 		regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>
> 		...
>
> 		err = mfd_add_devices(&pdev->dev, 0, subdevices, ARRAY_SIZE(subdevices),
> 				      regs, NULL, NULL);
> 		...
> 	}
>
> Thierry
Sorry,I might a little trouble for the changes.

The driver changes only for lcdc? If that is the case,I suddenly don't 
know how to do it ?

Maybe,I didn't say it clearly.

lcdc0: lcdc@ff930000                   |     vop0pwm: pwm@ff9301a0
reg = <0xff930000 0x10000>     |      reg = <0xff9301a0 0x10>;

The lcdc has to add resource's address  from  0xff930000 to 0xff93ffff.

When the  pwm driver is loading vop0pwm. the "devm_ioremap_resource()" 
will be used in probe();

I think it will be occur a fail. because the resource [mem 
0xff9301a0-0xff9301af] has be requested by lcdc.
=>rockchip-pwm ff9301a0.pwm: can't request region for resource [mem 
0xff9301a0-0xff9301af]

If I do the changes in pwm driver,do you have a other suggestion for 
it?    thanks.:-)

Re: [PATCH v2 2/2] pwm: add this patch to support the new pwm of Rockchip SoCs

[Doug Anderson]

caesar,

On Fri, Jul 25, 2014 at 3:29 AM, caesar <caesar.wang@rock-chips.com> wrote:
> Hi Thierry,
>
>
>
>
> 在 2014年07月21日 21:27, Thierry Reding 写道:
>>
>> On Mon, Jul 21, 2014 at 08:58:42PM +0800, caesar wrote:
>>
>>> 于 2014年07月21日 16:50, Thierry Reding 写道:
>>>>
>>>> On Sat, Jul 19, 2014 at 08:55:29PM +0800, Caesar Wang wrote:
>>
>> [...]
>>>>>
>>>>>         struct rockchip_pwm_chip *pc;
>>>>>         struct resource *r;
>>>>>         int ret;
>>>>> @@ -119,7 +182,10 @@ static int rockchip_pwm_probe(struct
>>>>> platform_device *pdev)
>>>>>                 return -ENOMEM;
>>>>>         r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>> -       pc->base = devm_ioremap_resource(&pdev->dev, r);
>>>>> +       if (!strcmp(of_id->compatible, "rockchip,vop-pwm"))
>>>>> +               pc->base = devm_ioremap(&pdev->dev, r->start,
>>>>> resource_size(r));
>>>>> +       else
>>>>> +               pc->base = devm_ioremap_resource(&pdev->dev, r);
>>>>
>>>> Sorry, this still isn't an option. You really shouldn't remap I/O
>>>> regions that other drivers may be using. You hinted at a shared register
>>>> space during the review of the initial version. Can you provide more
>>>> detail about what exactly the memory map looks like of the rk3288? Is
>>>> there some kind of technical reference manual that I could look at? Or
>>>> do you have a device tree extract that shows what the memory map looks
>>>> like?
>>>>
>>>> Thierry
>>>
>>> Maybe,you can look at the ARM: dts: rk3288:
>>>
>>> https://github.com/rkchrome/kernel/blob/master/arch/arm/boot/dts/rk3288.dtsi
>>> There is some lcdc and vop-pwm map address for rk3288.
>>>
>>> ,and you can look at the vop-introduce.pdf and vop-register.pdf in Annex.
>>>
>>> Maybe,I should put the vop-pwm in lcdc driver,but I don't hope do so it.
>>>
>>> Could you give a suggestion to solve it? Thanks.
>>
>> It looks like you could turn the lcdc device into an MFD device so that
>> it can instantiate two devices, one for the display controller, the
>> other for the PWM. Or perhaps it would even work with only a single
>> child device.
>>
>> The device tree would become something like this:
>>
>>         lcdc@ff930000 {
>>                 compatible = "rockchip,rk3288-lcdc";
>>                 ...
>>
>>                 pwm@ff9301a0 {
>>                         compatible = "rockchip,vop-pwm";
>>                         ...
>>                 };
>>         };
>>
>> And your driver would do something like:
>>
>>         static const struct resource pwm_resources[] = {
>>                 {
>>                         .start = 0x1a0,
>>                         .end = 0x1af,
>>                         .flags = IORESOURCE_MEM,
>>                 },
>>         };
>>
>>         static const struct mfd_cell subdevices[] = {
>>                 {
>>                         .name = "pwm",
>>                         .id = 1,
>>                         .of_compatible = "rockchip,vop-pwm",
>>                         .num_resources = ARRAY_SIZE(pwm_resources),
>>                         .resources = pwm_resources,
>>                 },
>>         };
>>
>>         static int lcdc_probe(struct platform_device *pdev)
>>         {
>>                 struct resource *regs;
>>                 ...
>>
>>                 regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>
>>                 ...
>>
>>                 err = mfd_add_devices(&pdev->dev, 0, subdevices,
>> ARRAY_SIZE(subdevices),
>>                                       regs, NULL, NULL);
>>                 ...
>>         }
>>
>> Thierry
>
> Sorry,I might a little trouble for the changes.
>
> The driver changes only for lcdc? If that is the case,I suddenly don't know
> how to do it ?
>
> Maybe,I didn't say it clearly.
>
> lcdc0: lcdc@ff930000                   |     vop0pwm: pwm@ff9301a0
> reg = <0xff930000 0x10000>     |      reg = <0xff9301a0 0x10>;
>
> The lcdc has to add resource's address  from  0xff930000 to 0xff93ffff.
>
> When the  pwm driver is loading vop0pwm. the "devm_ioremap_resource()" will
> be used in probe();
>
> I think it will be occur a fail. because the resource [mem
> 0xff9301a0-0xff9301af] has be requested by lcdc.
> =>rockchip-pwm ff9301a0.pwm: can't request region for resource [mem
> 0xff9301a0-0xff9301af]
>
> If I do the changes in pwm driver,do you have a other suggestion for it?
> thanks.:-)

Sorry if this is stupid (and I haven't tried it), but does "ranges"
help solve this problem?  AKA:

         lcdc@ff930000 {
                 compatible = "rockchip,rk3288-lcdc";
                 reg = <0xff930000 0x10000>;
                 #address-cells = <2>;
                 #size-cells = <1>;
                 ranges = <0 0xff9301a0 0x10>;
                 ...

                 pwm@0,0 {
                         compatible = "rockchip,vop-pwm";
                         reg = <0 0 0x10>;
                        ...
                 };
         };

Does that avoid the failure?  The lcdc driver would need to call
of_platform_populate() to make the PWM show up.

Re: [PATCH v2 2/2] pwm: add this patch to support the new pwm of Rockchip SoCs

[Doug Anderson]

Caesar,

On Sun, Jul 27, 2014 at 7:00 AM, caesar <caesar.wang@rock-chips.com> wrote:
> /*I think will be show the faill log:->
>
> * rockchip-pwm ff9301a0.pwm: can't request region for resource [mem
> 0xff9301a0-0xff93019f]
> */
>
> pc->base = devm_ioremap_resource(dev, regs);

Did you actually code this up and try it and get this error?  I hadn't
tried it but I researched other dts files and it looked as if there
was one example that was doing this.  ...but perhaps it wasn't
actually doing the ioremap_resource on both ranges.

I'd imagine that this is _probably_ equivalent to what Thierry was
suggesting, so if it didn't work then maybe Thierry's won't work
either?

I don't have any other great suggestions other than doing two memory
ranges for lcdc:

         lcdc@ff930000 {
                 compatible = "rockchip,rk3288-lcdc";
                 reg = <0xff930000 0x1a0>, <0xff9301b0 0xfe50>;
                 ...
         };
         pwm@ff9301a0 {
                 compatible = "rockchip,vop-pwm";
                 reg = <0xff9301a0 0x10>;
                 ...
         };

...but I am certainly nowhere near an expert on this stuff...

-Doug

Re: [PATCH v2 2/2] pwm: add this patch to support the new pwm of Rockchip SoCs

[caesar]

Doug,
在 2014年07月28日 12:01, Doug Anderson 写道:
> Caesar,
>
> On Sun, Jul 27, 2014 at 7:00 AM, caesar <caesar.wang@rock-chips.com> wrote:
>> /*I think will be show the faill log:->
>>
>> * rockchip-pwm ff9301a0.pwm: can't request region for resource [mem
>> 0xff9301a0-0xff93019f]
>> */
>>
>> pc->base = devm_ioremap_resource(dev, regs);
> Did you actually code this up and try it and get this error?
Yeah.
> I hadn't
> tried it but I researched other dts files and it looked as if there
> was one example that was doing this.  ...but perhaps it wasn't
> actually doing the ioremap_resource on both ranges.
>
> I'd imagine that this is _probably_ equivalent to what Thierry was
> suggesting, so if it didn't work then maybe Thierry's won't work
> either?
>
> I don't have any other great suggestions other than doing two memory
> ranges for lcdc:
>
>           lcdc@ff930000 {
>                   compatible = "rockchip,rk3288-lcdc";
>                   reg = <0xff930000 0x1a0>, <0xff9301b0 0xfe50>;
>                   ...
>           };
>           pwm@ff9301a0 {
>                   compatible = "rockchip,vop-pwm";
>                   reg = <0xff9301a0 0x10>;
>                   ...
>           };
>
> ...but I am certainly nowhere near an expert on this stuff...
>
> -Doug
>
>
>
I has solve in lcdc driver,but I always feel awkward. I think a good way 
to solve in pwm driver.
Unfortunately that  so far ,I have not a good idle in pwm driver.

Maybe,I  let do it that way in lcdc driver.

Re: [PATCH v2 2/2] pwm: add this patch to support the new pwm of Rockchip SoCs

[Olof Johansson]

On Mon, Jul 28, 2014 at 4:19 AM, caesar <caesar.wang-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
> Doug,
> 在 2014年07月28日 12:01, Doug Anderson 写道:
>
>> Caesar,
>>
>> On Sun, Jul 27, 2014 at 7:00 AM, caesar <caesar.wang-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> wrote:
>>>
>>> /*I think will be show the faill log:->
>>>
>>> * rockchip-pwm ff9301a0.pwm: can't request region for resource [mem
>>> 0xff9301a0-0xff93019f]
>>> */
>>>
>>> pc->base = devm_ioremap_resource(dev, regs);
>>
>> Did you actually code this up and try it and get this error?
>
> Yeah.
>
>> I hadn't
>> tried it but I researched other dts files and it looked as if there
>> was one example that was doing this.  ...but perhaps it wasn't
>> actually doing the ioremap_resource on both ranges.
>>
>> I'd imagine that this is _probably_ equivalent to what Thierry was
>> suggesting, so if it didn't work then maybe Thierry's won't work
>> either?
>>
>> I don't have any other great suggestions other than doing two memory
>> ranges for lcdc:
>>
>>           lcdc@ff930000 {
>>                   compatible = "rockchip,rk3288-lcdc";
>>                   reg = <0xff930000 0x1a0>, <0xff9301b0 0xfe50>;
>>                   ...
>>           };
>>           pwm@ff9301a0 {
>>                   compatible = "rockchip,vop-pwm";
>>                   reg = <0xff9301a0 0x10>;
>>                   ...
>>           };
>>
>> ...but I am certainly nowhere near an expert on this stuff...
>>
>> -Doug
>>
>>
>>
> I has solve in lcdc driver,but I always feel awkward. I think a good way to
> solve in pwm driver.
> Unfortunately that  so far ,I have not a good idle in pwm driver.
>
> Maybe,I  let do it that way in lcdc driver.

I think there's an easier way to do this, by not focusing _too_ much
on the device tree:

* Define a platform_data structure for the PWM driver, which contains
readl/writel accessors as well as the device type (i.e. what you use
the compatible field and the lookup table for today).
* Populate the platform_device in the clcd driver, and register that
* Make the PWM driver probe as a regular platform device if pdata is passed
* Make a readl/writel wrapper that either falls back to native
readl/writel when there aren't any passed in, or make the DT code fill
in the pdata with the native versions in that case.

Going full MFD on this seems overkill, unless there is also a shared
interrupt that needs to be handled.

-Olof
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] pwm: add this patch to support the new pwm of Rockchip SoCs

[caesar]

Hi Olof,

Sorry, I didn't understand all of what you mean.
Please allow me to paste the following code [1].

在 2014年07月29日 00:58, Olof Johansson 写道:
> On Mon, Jul 28, 2014 at 4:19 AM, caesar <caesar.wang@rock-chips.com> wrote:
>> Doug,
>> 在 2014年07月28日 12:01, Doug Anderson 写道:
>>
>>> Caesar,
>>>
>>> On Sun, Jul 27, 2014 at 7:00 AM, caesar <caesar.wang@rock-chips.com>
>>> wrote:
>>>> /*I think will be show the faill log:->
>>>>
>>>> * rockchip-pwm ff9301a0.pwm: can't request region for resource [mem
>>>> 0xff9301a0-0xff93019f]
>>>> */
>>>>
>>>> pc->base = devm_ioremap_resource(dev, regs);
>>> Did you actually code this up and try it and get this error?
>> Yeah.
>>
>>> I hadn't
>>> tried it but I researched other dts files and it looked as if there
>>> was one example that was doing this.  ...but perhaps it wasn't
>>> actually doing the ioremap_resource on both ranges.
>>>
>>> I'd imagine that this is _probably_ equivalent to what Thierry was
>>> suggesting, so if it didn't work then maybe Thierry's won't work
>>> either?
>>>
>>> I don't have any other great suggestions other than doing two memory
>>> ranges for lcdc:
>>>
>>>            lcdc@ff930000 {
>>>                    compatible = "rockchip,rk3288-lcdc";
>>>                    reg = <0xff930000 0x1a0>, <0xff9301b0 0xfe50>;
>>>                    ...
>>>            };
>>>            pwm@ff9301a0 {
>>>                    compatible = "rockchip,vop-pwm";
>>>                    reg = <0xff9301a0 0x10>;
>>>                    ...
>>>            };
>>>
>>> ...but I am certainly nowhere near an expert on this stuff...
>>>
>>> -Doug
>>>
>>>
>>>
>> I has solve in lcdc driver,but I always feel awkward. I think a good way to
>> solve in pwm driver.
>> Unfortunately that  so far ,I have not a good idle in pwm driver.
>>
>> Maybe,I  let do it that way in lcdc driver.
> I think there's an easier way to do this, by not focusing _too_ much
> on the device tree:
>
> * Define a platform_data structure for the PWM driver, which contains
> readl/writel accessors as well as the device type (i.e. what you use
> the compatible field and the lookup table for today).
Maybe, as the following  code: "pwm_data_vop" has been implement it. right?
> * Populate the platform_device in the clcd driver, and register that
Yeah,the lcdc driver can register it.
> * Make the PWM driver probe as a regular platform device if pdata is passed
> * Make a readl/writel wrapper that either falls back to native
> readl/writel when there aren't any passed in, or make the DT code fill
> in the pdata with the native versions in that case.
Sorry,This step I don't understand it. Perhaps, could you give a simple 
case for it ?:-P

> Going full MFD on this seems overkill, unless there is also a shared
> interrupt that needs to be handled.
>
> -Olof
>
>
>
>

[1]:The driver/pwm/pwm-rockchip.c

#include <linux/clk.h>
#include <linux/io.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/pwm.h>
#include <linux/time.h>

#define PWM_CNTR        0x00        /* Counter register */
#define PWM_HRC            0x04        /* High reference register */
#define PWM_LRC            0x08        /* Low reference register */
#define PWM_CTRL        0x0c        /* Control register */
#define PWM_CTRL_TIMER_EN    (1 << 0)
#define PWM_CTRL_OUTPUT_EN    (1 << 3)

#define PRESCALER        2

#define PWM_ENABLE        (1 << 0)
#define PWM_CONTINUOUS        (1 << 1)
#define PWM_DUTY_POSITIVE    (1 << 3)
#define PWM_INACTIVE_NEGATIVE    (0 << 4)
#define PWM_OUTPUT_LEFT        (0 << 5)
#define PWM_LP_DISABLE        (0 << 8)

struct rockchip_pwm_chip {
     struct pwm_chip chip;
     struct clk *clk;
     const struct rockchip_pwm_data *data;
     void __iomem *base;
};

struct rockchip_pwm_regs {
     unsigned long duty;
     unsigned long period;
     unsigned long cntr;
     unsigned long ctrl;
};

struct rockchip_pwm_data {
     struct rockchip_pwm_regs regs;
     unsigned int prescaler;

     void (*set_enable)(struct pwm_chip *chip, bool enable);
};

static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct 
pwm_chip *c)
{
     return container_of(c, struct rockchip_pwm_chip, chip);
}

static void rockchip_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
{
     struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
     u32 val = 0;
     u32 enable_conf = PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN;

     val = readl_relaxed(pc->base + pc->data->regs.ctrl);

     if (enable)
         val |= enable_conf;
     else
         val &= ~enable_conf;

     writel_relaxed(val, pc->base + pc->data->regs.ctrl);
}

static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
{
     struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
     u32 val = 0;
     u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
         PWM_CONTINUOUS | PWM_DUTY_POSITIVE | PWM_INACTIVE_NEGATIVE;

     val = readl_relaxed(pc->base + pc->data->regs.ctrl);

     if (enable)
         val |= enable_conf;
     else
         val &= ~enable_conf;

     writel_relaxed(val, pc->base + pc->data->regs.ctrl);
}

static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device 
*pwm,
                    int duty_ns, int period_ns)
{
     struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
     unsigned long period, duty;
     u64 clk_rate, div;
     int ret;

     clk_rate = clk_get_rate(pc->clk);

     /*
      * Since period and duty cycle registers have a width of 32
      * bits, every possible input period can be obtained using the
      * default prescaler value for all practical clock rate values.
      */
     div = clk_rate * period_ns;
     do_div(div, pc->data->prescaler * NSEC_PER_SEC);
     period = div;

     div = clk_rate * duty_ns;
     do_div(div, pc->data->prescaler * NSEC_PER_SEC);
     duty = div;

     ret = clk_enable(pc->clk);
     if (ret)
         return ret;

     writel(period, pc->base + pc->data->regs.period);
     writel(duty, pc->base + pc->data->regs.duty);
     writel(0, pc->base + pc->data->regs.cntr);

     clk_disable(pc->clk);

     return 0;
}

static int rockchip_pwm_enable(struct pwm_chip *chip, struct pwm_device 
*pwm)
{
     struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
     int ret;

     ret = clk_enable(pc->clk);
     if (ret)
         return ret;

     pc->data->set_enable(chip, true);

     return 0;
}

static void rockchip_pwm_disable(struct pwm_chip *chip, struct 
pwm_device *pwm)
{
     struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);

     pc->data->set_enable(chip, false);

     clk_disable(pc->clk);
}

static const struct pwm_ops rockchip_pwm_ops = {
     .config = rockchip_pwm_config,
     .enable = rockchip_pwm_enable,
     .disable = rockchip_pwm_disable,
     .owner = THIS_MODULE,
};

static const struct rockchip_pwm_data pwm_data_v1 = {
     .regs.duty = PWM_HRC,
     .regs.period = PWM_LRC,
     .regs.cntr = PWM_CNTR,
     .regs.ctrl = PWM_CTRL,
     .prescaler = PRESCALER,
     .set_enable = rockchip_pwm_set_enable_v1,
};

static const struct rockchip_pwm_data pwm_data_v2 = {
     .regs.duty = PWM_LRC,
     .regs.period = PWM_HRC,
     .regs.cntr = PWM_CNTR,
     .regs.ctrl = PWM_CTRL,
     .prescaler = PRESCALER-1,
     .set_enable = rockchip_pwm_set_enable_v2,
};

static const struct rockchip_pwm_data pwm_data_vop = {
     .regs.duty = PWM_LRC,
     .regs.period = PWM_HRC,
     .regs.cntr = PWM_CTRL,
     .regs.ctrl = PWM_CNTR,
     .prescaler = PRESCALER-1,
     .set_enable = rockchip_pwm_set_enable_v2,
};

static const struct of_device_id rockchip_pwm_dt_ids[] = {
     { .compatible = "rockchip,rk2928-pwm", .data = &pwm_data_v1},
     { .compatible = "rockchip,rk3288-pwm", .data = &pwm_data_v2},
     { .compatible = "rockchip,vop-pwm", .data = &pwm_data_vop},
     { /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, rockchip_pwm_dt_ids);

static int rockchip_pwm_probe(struct platform_device *pdev)
{
     const struct of_device_id *id;
     struct rockchip_pwm_chip *pc;
     struct resource *r;
     int ret;

     id = of_match_device(rockchip_pwm_dt_ids, &pdev->dev);
     if (!id)
         return -EINVAL;

     pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
     if (!pc)
         return -ENOMEM;

     r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
     pc->base = devm_ioremap_resource(&pdev->dev, r);
     if (IS_ERR(pc->base))
         return PTR_ERR(pc->base);

     pc->clk = devm_clk_get(&pdev->dev, NULL);
     if (IS_ERR(pc->clk))
         return PTR_ERR(pc->clk);

     ret = clk_prepare(pc->clk);
     if (ret)
         return ret;

     platform_set_drvdata(pdev, pc);

     pc->data = id->data;
     pc->chip.dev = &pdev->dev;
     pc->chip.ops = &rockchip_pwm_ops;
     pc->chip.base = -1;
     pc->chip.npwm = 1;

     ret = pwmchip_add(&pc->chip);
     if (ret < 0) {
         clk_unprepare(pc->clk);
         dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
     }

     return ret;
}

static int rockchip_pwm_remove(struct platform_device *pdev)
{
     struct rockchip_pwm_chip *pc = platform_get_drvdata(pdev);

     clk_unprepare(pc->clk);

     return pwmchip_remove(&pc->chip);
}

static struct platform_driver rockchip_pwm_driver = {
     .driver = {
         .name = "rockchip-pwm",
         .of_match_table = rockchip_pwm_dt_ids,
     },
     .probe = rockchip_pwm_probe,
     .remove = rockchip_pwm_remove,
};
module_platform_driver(rockchip_pwm_driver);

MODULE_AUTHOR("Beniamino Galvani <b.galvani@gmail.com>");
MODULE_DESCRIPTION("Rockchip SoC PWM driver");
MODULE_LICENSE("GPL v2");

-Caesar

Re: [PATCH v2 2/2] pwm: add this patch to support the new pwm of Rockchip SoCs

[Thierry Reding]

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

On Mon, Jul 28, 2014 at 09:58:22AM -0700, Olof Johansson wrote:
> On Mon, Jul 28, 2014 at 4:19 AM, caesar <caesar.wang@rock-chips.com> wrote:
> > Doug,
> > 在 2014年07月28日 12:01, Doug Anderson 写道:
> >
> >> Caesar,
> >>
> >> On Sun, Jul 27, 2014 at 7:00 AM, caesar <caesar.wang@rock-chips.com>
> >> wrote:
> >>>
> >>> /*I think will be show the faill log:->
> >>>
> >>> * rockchip-pwm ff9301a0.pwm: can't request region for resource [mem
> >>> 0xff9301a0-0xff93019f]
> >>> */
> >>>
> >>> pc->base = devm_ioremap_resource(dev, regs);
> >>
> >> Did you actually code this up and try it and get this error?
> >
> > Yeah.
> >
> >> I hadn't
> >> tried it but I researched other dts files and it looked as if there
> >> was one example that was doing this.  ...but perhaps it wasn't
> >> actually doing the ioremap_resource on both ranges.
> >>
> >> I'd imagine that this is _probably_ equivalent to what Thierry was
> >> suggesting, so if it didn't work then maybe Thierry's won't work
> >> either?
> >>
> >> I don't have any other great suggestions other than doing two memory
> >> ranges for lcdc:
> >>
> >>           lcdc@ff930000 {
> >>                   compatible = "rockchip,rk3288-lcdc";
> >>                   reg = <0xff930000 0x1a0>, <0xff9301b0 0xfe50>;
> >>                   ...
> >>           };
> >>           pwm@ff9301a0 {
> >>                   compatible = "rockchip,vop-pwm";
> >>                   reg = <0xff9301a0 0x10>;
> >>                   ...
> >>           };
> >>
> >> ...but I am certainly nowhere near an expert on this stuff...
> >>
> >> -Doug
> >>
> >>
> >>
> > I has solve in lcdc driver,but I always feel awkward. I think a good way to
> > solve in pwm driver.
> > Unfortunately that  so far ,I have not a good idle in pwm driver.
> >
> > Maybe,I  let do it that way in lcdc driver.
> 
> I think there's an easier way to do this, by not focusing _too_ much
> on the device tree:
> 
> * Define a platform_data structure for the PWM driver, which contains
> readl/writel accessors as well as the device type (i.e. what you use
> the compatible field and the lookup table for today).
> * Populate the platform_device in the clcd driver, and register that
> * Make the PWM driver probe as a regular platform device if pdata is passed
> * Make a readl/writel wrapper that either falls back to native
> readl/writel when there aren't any passed in, or make the DT code fill
> in the pdata with the native versions in that case.

That's one option, but it isn't going to work if you ever want to refer
to the PWM subdevice by phandle, since no phandle will exist.

Thierry

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

Re: [PATCH v2 2/2] pwm: add this patch to support the new pwm of Rockchip SoCs

[Thierry Reding]

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

On Mon, Jul 28, 2014 at 07:19:18PM +0800, caesar wrote:
> Doug,
> 在 2014年07月28日 12:01, Doug Anderson 写道:
> >Caesar,
> >
> >On Sun, Jul 27, 2014 at 7:00 AM, caesar <caesar.wang@rock-chips.com> wrote:
> >>/*I think will be show the faill log:->
> >>
> >>* rockchip-pwm ff9301a0.pwm: can't request region for resource [mem
> >>0xff9301a0-0xff93019f]
> >>*/
> >>
> >>pc->base = devm_ioremap_resource(dev, regs);
> >Did you actually code this up and try it and get this error?
> Yeah.

This should work if you properly set up the PWM subregion as a child of
the LCDC region, which is what MFD will do for you.

Thierry

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

Re: [PATCH v2 2/2] pwm: add this patch to support the new pwm of Rockchip SoCs

[caesar]

Thierry,

在 2014年07月29日 18:22, Thierry Reding 写道:
> On Mon, Jul 28, 2014 at 07:19:18PM +0800, caesar wrote:
>> Doug,
>> 在 2014年07月28日 12:01, Doug Anderson 写道:
>>> Caesar,
>>>
>>> On Sun, Jul 27, 2014 at 7:00 AM, caesar <caesar.wang@rock-chips.com> wrote:
>>>> /*I think will be show the faill log:->
>>>>
>>>> * rockchip-pwm ff9301a0.pwm: can't request region for resource [mem
>>>> 0xff9301a0-0xff93019f]
>>>> */
>>>>
>>>> pc->base = devm_ioremap_resource(dev, regs);
>>> Did you actually code this up and try it and get this error?
>> Yeah.
> This should work if you properly set up the PWM subregion as a child of
> the LCDC region, which is what MFD will do for you.
>
> Thierry
As you say,should this change be occured by lcdc driver and dts?

The PWM driver don't need do any changes?

Re: [PATCH v2 2/2] pwm: add this patch to support the new pwm of Rockchip SoCs

[Thierry Reding]

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

On Tue, Jul 29, 2014 at 07:09:07PM +0800, caesar wrote:
> Thierry,
> 
> 在 2014年07月29日 18:22, Thierry Reding 写道:
> >On Mon, Jul 28, 2014 at 07:19:18PM +0800, caesar wrote:
> >>Doug,
> >>在 2014年07月28日 12:01, Doug Anderson 写道:
> >>>Caesar,
> >>>
> >>>On Sun, Jul 27, 2014 at 7:00 AM, caesar <caesar.wang@rock-chips.com> wrote:
> >>>>/*I think will be show the faill log:->
> >>>>
> >>>>* rockchip-pwm ff9301a0.pwm: can't request region for resource [mem
> >>>>0xff9301a0-0xff93019f]
> >>>>*/
> >>>>
> >>>>pc->base = devm_ioremap_resource(dev, regs);
> >>>Did you actually code this up and try it and get this error?
> >>Yeah.
> >This should work if you properly set up the PWM subregion as a child of
> >the LCDC region, which is what MFD will do for you.
> >
> >Thierry
> As you say,should this change be occured by lcdc driver and dts?
> 
> The PWM driver don't need do any changes?

No, I don't think the PWM driver needs to be changed for the above to
work.

Thierry

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

Re: [PATCH v2 2/2] pwm: add this patch to support the new pwm of Rockchip SoCs

[caesar]

Thierry,

在 2014年07月29日 19:38, Thierry Reding 写道:
> On Tue, Jul 29, 2014 at 07:09:07PM +0800, caesar wrote:
>> Thierry,
>>
>> 在 2014年07月29日 18:22, Thierry Reding 写道:
>>> On Mon, Jul 28, 2014 at 07:19:18PM +0800, caesar wrote:
>>>> Doug,
>>>> 在 2014年07月28日 12:01, Doug Anderson 写道:
>>>>> Caesar,
>>>>>
>>>>> On Sun, Jul 27, 2014 at 7:00 AM, caesar <caesar.wang@rock-chips.com> wrote:
>>>>>> /*I think will be show the faill log:->
>>>>>>
>>>>>> * rockchip-pwm ff9301a0.pwm: can't request region for resource [mem
>>>>>> 0xff9301a0-0xff93019f]
>>>>>> */
>>>>>>
>>>>>> pc->base = devm_ioremap_resource(dev, regs);
>>>>> Did you actually code this up and try it and get this error?
>>>> Yeah.
>>> This should work if you properly set up the PWM subregion as a child of
>>> the LCDC region, which is what MFD will do for you.
>>>
>>> Thierry
>> As you say,should this change be occured by lcdc driver and dts?
>>
>> The PWM driver don't need do any changes?
> No, I don't think the PWM driver needs to be changed for the above to
> work.
>
> Thierry
Ok, as you suggestions, The PWM driver :

static int rockchip_pwm_probe (...)
{

	...

         r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-       pc->base = devm_ioremap_resource(&pdev->dev, r);
+       if (!strcmp(of_id->compatible, "rockchip,vop-pwm"))
+               pc->base = devm_ioremap(&pdev->dev, r->start,
resource_size(r));
+       else
+               pc->base = devm_ioremap_resource(&pdev->dev, r);

	...

}

This will be fixed for following:

static int rockchip_pwm_probe (...)
{

	...

         r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
	pc->base = devm_ioremap_resource(&pdev->dev, r);

	...

}

I will discuss with lcdc of upstream's people tomorrow.

I has sent the PWM in patch v4 the last few days,Hope you can help check 
and accept it,thanks.:-)

-caesar

Re: [PATCH v2 2/2] pwm: add this patch to support the new pwm of Rockchip SoCs

[Thierry Reding]

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

On Sat, Jul 26, 2014 at 09:59:35PM -0700, Doug Anderson wrote:
> caesar,
> 
> On Fri, Jul 25, 2014 at 3:29 AM, caesar <caesar.wang@rock-chips.com> wrote:
> > Hi Thierry,
> >
> >
> >
> >
> > 在 2014年07月21日 21:27, Thierry Reding 写道:
> >>
> >> On Mon, Jul 21, 2014 at 08:58:42PM +0800, caesar wrote:
> >>
> >>> 于 2014年07月21日 16:50, Thierry Reding 写道:
> >>>>
> >>>> On Sat, Jul 19, 2014 at 08:55:29PM +0800, Caesar Wang wrote:
> >>
> >> [...]
> >>>>>
> >>>>>         struct rockchip_pwm_chip *pc;
> >>>>>         struct resource *r;
> >>>>>         int ret;
> >>>>> @@ -119,7 +182,10 @@ static int rockchip_pwm_probe(struct
> >>>>> platform_device *pdev)
> >>>>>                 return -ENOMEM;
> >>>>>         r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >>>>> -       pc->base = devm_ioremap_resource(&pdev->dev, r);
> >>>>> +       if (!strcmp(of_id->compatible, "rockchip,vop-pwm"))
> >>>>> +               pc->base = devm_ioremap(&pdev->dev, r->start,
> >>>>> resource_size(r));
> >>>>> +       else
> >>>>> +               pc->base = devm_ioremap_resource(&pdev->dev, r);
> >>>>
> >>>> Sorry, this still isn't an option. You really shouldn't remap I/O
> >>>> regions that other drivers may be using. You hinted at a shared register
> >>>> space during the review of the initial version. Can you provide more
> >>>> detail about what exactly the memory map looks like of the rk3288? Is
> >>>> there some kind of technical reference manual that I could look at? Or
> >>>> do you have a device tree extract that shows what the memory map looks
> >>>> like?
> >>>>
> >>>> Thierry
> >>>
> >>> Maybe,you can look at the ARM: dts: rk3288:
> >>>
> >>> https://github.com/rkchrome/kernel/blob/master/arch/arm/boot/dts/rk3288.dtsi
> >>> There is some lcdc and vop-pwm map address for rk3288.
> >>>
> >>> ,and you can look at the vop-introduce.pdf and vop-register.pdf in Annex.
> >>>
> >>> Maybe,I should put the vop-pwm in lcdc driver,but I don't hope do so it.
> >>>
> >>> Could you give a suggestion to solve it? Thanks.
> >>
> >> It looks like you could turn the lcdc device into an MFD device so that
> >> it can instantiate two devices, one for the display controller, the
> >> other for the PWM. Or perhaps it would even work with only a single
> >> child device.
> >>
> >> The device tree would become something like this:
> >>
> >>         lcdc@ff930000 {
> >>                 compatible = "rockchip,rk3288-lcdc";
> >>                 ...
> >>
> >>                 pwm@ff9301a0 {
> >>                         compatible = "rockchip,vop-pwm";
> >>                         ...
> >>                 };
> >>         };
> >>
> >> And your driver would do something like:
> >>
> >>         static const struct resource pwm_resources[] = {
> >>                 {
> >>                         .start = 0x1a0,
> >>                         .end = 0x1af,
> >>                         .flags = IORESOURCE_MEM,
> >>                 },
> >>         };
> >>
> >>         static const struct mfd_cell subdevices[] = {
> >>                 {
> >>                         .name = "pwm",
> >>                         .id = 1,
> >>                         .of_compatible = "rockchip,vop-pwm",
> >>                         .num_resources = ARRAY_SIZE(pwm_resources),
> >>                         .resources = pwm_resources,
> >>                 },
> >>         };
> >>
> >>         static int lcdc_probe(struct platform_device *pdev)
> >>         {
> >>                 struct resource *regs;
> >>                 ...
> >>
> >>                 regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >>
> >>                 ...
> >>
> >>                 err = mfd_add_devices(&pdev->dev, 0, subdevices,
> >> ARRAY_SIZE(subdevices),
> >>                                       regs, NULL, NULL);
> >>                 ...
> >>         }
> >>
> >> Thierry
> >
> > Sorry,I might a little trouble for the changes.
> >
> > The driver changes only for lcdc? If that is the case,I suddenly don't know
> > how to do it ?
> >
> > Maybe,I didn't say it clearly.
> >
> > lcdc0: lcdc@ff930000                   |     vop0pwm: pwm@ff9301a0
> > reg = <0xff930000 0x10000>     |      reg = <0xff9301a0 0x10>;
> >
> > The lcdc has to add resource's address  from  0xff930000 to 0xff93ffff.
> >
> > When the  pwm driver is loading vop0pwm. the "devm_ioremap_resource()" will
> > be used in probe();
> >
> > I think it will be occur a fail. because the resource [mem
> > 0xff9301a0-0xff9301af] has be requested by lcdc.
> > =>rockchip-pwm ff9301a0.pwm: can't request region for resource [mem
> > 0xff9301a0-0xff9301af]
> >
> > If I do the changes in pwm driver,do you have a other suggestion for it?
> > thanks.:-)
> 
> Sorry if this is stupid (and I haven't tried it), but does "ranges"
> help solve this problem?  AKA:
> 
>          lcdc@ff930000 {
>                  compatible = "rockchip,rk3288-lcdc";
>                  reg = <0xff930000 0x10000>;
>                  #address-cells = <2>;
>                  #size-cells = <1>;
>                  ranges = <0 0xff9301a0 0x10>;
>                  ...
> 
>                  pwm@0,0 {
>                          compatible = "rockchip,vop-pwm";
>                          reg = <0 0 0x10>;
>                         ...
>                  };
>          };
> 
> Does that avoid the failure?  The lcdc driver would need to call
> of_platform_populate() to make the PWM show up.

If you add "simple-bus" to the lcdc compatible string, like so:

	lcdc@ff930000 {
		compatible = "rockchip,rk3288-lcdc", "simple-bus";
		...
	};

Then of_platform_populate() will be called automatically.

Thierry

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