| Message ID | 1405774529-26027-3-git-send-email-caesar.wang@rock-chips.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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
于 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.:-) -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. 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 linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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
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
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? -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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",
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(-)