| Message ID | 1391855002-26098-1-git-send-email-21cnbao@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
On Sat, Feb 08, 2014 at 06:23:22PM +0800, Barry Song wrote: > From: Rongjun Ying <Rongjun.ying@csr.com> > > PWM controller of CSR SiRFSoC can generate 7 independent outputs. Each output "The PWM controller of the CSR SiRFSoC..." and "Each output's..." > duty cycle can be adjusted by setting the corresponding wait & hold registers. > > Supports 7 independent channel output: 6 for external(channel0-5) and 1 for > internal(channel6). This repeats part of the first sentence. Perhaps: There are 6 external channels (0 to 5) and 1 internal channel (6). ? > Supports wide frequency range: divide by 2 to 65536*2 of source clock. "Supports a wide frequency range: the source clock divider can be from 2 up to 65536*2". > Documentation/devicetree/bindings/pwm/pwm-sirf.txt | 17 + > arch/arm/boot/dts/atlas6.dtsi | 3 +- > arch/arm/boot/dts/prima2.dtsi | 3 +- > drivers/pwm/Kconfig | 9 + > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-sirf.c | 308 ++++++++++++++++++++ > 6 files changed, 339 insertions(+), 2 deletions(-) > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sirf.txt > create mode 100644 drivers/pwm/pwm-sirf.c > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-sirf.txt b/Documentation/devicetree/bindings/pwm/pwm-sirf.txt > new file mode 100644 > index 0000000..4b10109 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/pwm-sirf.txt > @@ -0,0 +1,17 @@ > +SiRF prima2 & atlas6 PWM drivers > + > +Required properties: > +- compatible: "sirf,prima2-pwm" > +- reg: physical base address and length of the controller's registers > +- #pwm-cells: should be 2. The first cell specifies the per-chip index > + of the PWM to use and the second cell is the period in nanoseconds. > +- clocks: from common clock binding: the 1st clock is for PWM controller > + the 2nd clock is the source to generate PWM waves > + > +Example: > +pwm: pwm@b0130000 { > + compatible = "sirf,prima2-pwm"; > + #pwm-cells = <2>; > + reg = <0xb0130000 0x10000>; > + clocks = <&clks 21>, <&clks 1>; > +}; Please move this into a separate patch and Cc the device tree bindings maintainers. There's nothing non-standard in this binding as far as I can tell, but I'd still like to give them an opportunity to object. Also you should be documenting the clock-names property here as well. > diff --git a/arch/arm/boot/dts/atlas6.dtsi b/arch/arm/boot/dts/atlas6.dtsi > index f8674bc..5a09815 100644 > --- a/arch/arm/boot/dts/atlas6.dtsi > +++ b/arch/arm/boot/dts/atlas6.dtsi > @@ -614,8 +614,9 @@ > > pwm@b0130000 { > compatible = "sirf,prima2-pwm"; > + #pwm-cells = <2>; > reg = <0xb0130000 0x10000>; > - clocks = <&clks 21>; > + clocks = <&clks 21>, <&clks 1>; > }; > > efusesys@b0140000 { > diff --git a/arch/arm/boot/dts/prima2.dtsi b/arch/arm/boot/dts/prima2.dtsi > index 0e21993..3439e48 100644 > --- a/arch/arm/boot/dts/prima2.dtsi > +++ b/arch/arm/boot/dts/prima2.dtsi > @@ -642,8 +642,9 @@ > > pwm@b0130000 { > compatible = "sirf,prima2-pwm"; > + #pwm-cells = <2>; > reg = <0xb0130000 0x10000>; > - clocks = <&clks 21>; > + clocks = <&clks 21>, <&clks 1>; > }; > > efusesys@b0140000 { These changes need to go into separate patches and go in through the arm-soc tree. > diff --git a/drivers/pwm/pwm-sirf.c b/drivers/pwm/pwm-sirf.c > new file mode 100644 > index 0000000..b717de0 > --- /dev/null > +++ b/drivers/pwm/pwm-sirf.c > @@ -0,0 +1,308 @@ > +/* > + * SIRF serial SoC PWM device core driver > + * > + * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group company. > + * > + * Licensed under GPLv2 or later. > + */ [...] > +#define SIRF_PWM_CHL_NUM 7 This is only used in a single location with a well-known meaning, so no need to use a symbolic name. > +#define SIRF_PWM_BLS_GRP_NUM 16 This isn't used as far as I can tell. > +struct sirf_pwm { > + void __iomem *base; > + struct clk *clk; > + struct pwm_chip chip; > + unsigned long src_clk_rate; > +}; Please drop the alignment of the structure field. Also perhaps move the chip field to be the first, so that the upcasting below can be turned into a noop. > +#define to_sirf_chip(chip) container_of(chip, struct sirf_pwm, chip) Please make this a static inline function. > + > +static unsigned int sirf_pwm_ns_to_cycles(struct pwm_chip *chip, unsigned int time_ns) Please wrap this to go on a single line. > +{ > + struct sirf_pwm *spwm = to_sirf_chip(chip); > + u64 dividend; > + unsigned int cycle; > + > + dividend = spwm->src_clk_rate * time_ns + NSEC_PER_SEC / 2; > + do_div(dividend, NSEC_PER_SEC); > + > + cycle = dividend & 0xFFFFFFFFUL; I don't think you need the mask, since you're casting to a 32-bit unsigned integer anyway. > + > + return cycle > 1 ? cycle : 1; > +} > + > +static int sirf_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > + int duty_ns, int period_ns) Please align arguments on subsequent lines with those of the first. > +{ > + unsigned int period_cycles, high_cycles, low_cycles; > + unsigned int val; u32 please. > + struct sirf_pwm *spwm = to_sirf_chip(chip); > + > + period_cycles = sirf_pwm_ns_to_cycles(chip, period_ns); > + > + high_cycles = sirf_pwm_ns_to_cycles(chip, duty_ns); No need for the blank line above, since there's no blank line below either. > + low_cycles = period_cycles - high_cycles; > + > + if (period_cycles == 1) { > + /* bypass mode */ > + val = readl(spwm->base + SIRF_PWM_SELECT_PRECLK); > + val |= 0x1 << (BYPASS_MODE_BIT + pwm->hwpwm); > + writel(val, spwm->base + SIRF_PWM_SELECT_PRECLK); > + dev_warn(chip->dev, "period is too short!\n"); What does the bypass mode do? Would it perhaps be better to make this an outright error rather than only warning about it? > + } else { > + /* divider mode */ > + val = readl(spwm->base + SIRF_PWM_SELECT_PRECLK); > + val &= ~(0x1 << (BYPASS_MODE_BIT + pwm->hwpwm)); > + writel(val, spwm->base + SIRF_PWM_SELECT_PRECLK); > + > + if (high_cycles == period_cycles) { > + high_cycles--; > + low_cycles = 1; > + } > + > + writel(high_cycles, spwm->base + SIRF_PWM_GET_WAIT_OFFSET(pwm->hwpwm)); > + writel(low_cycles, spwm->base + SIRF_PWM_GET_HOLD_OFFSET(pwm->hwpwm)); > + } > + > + return 0; > +} > + > +static int sirf_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct sirf_pwm *spwm = to_sirf_chip(chip); > + unsigned int val; The type used by readl() and writel() is u32. > + /* disable preclock */ > + val = readl(spwm->base + SIRF_PWM_ENABLE_PRECLOCK); > + val &= ~(1 << pwm->hwpwm); > + writel(val, spwm->base + SIRF_PWM_ENABLE_PRECLOCK); > + > + /* select preclock source must after disable preclk*/ > + val = readl(spwm->base + SIRF_PWM_SELECT_PRECLK); > + val &= ~(0x7 << (SRC_FIELD_SIZE * pwm->hwpwm)); > + writel(val, spwm->base + SIRF_PWM_SELECT_PRECLK); > + /* wait for some time */ > + udelay(100); usleep_range() perhaps? > +static void sirf_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + unsigned int val; u32 again. > + struct sirf_pwm *spwm = to_sirf_chip(chip); > + /* disable output */ Blank line between the above two, please. > + val = readl(spwm->base + SIRF_PWM_OE); > + val &= ~(1 << pwm->hwpwm); > + writel(val, spwm->base + SIRF_PWM_OE); > + > + /* disable postclock */ > + val = readl(spwm->base + SIRF_PWM_ENABLE_POSTCLOCK); > + val &= ~(1 << pwm->hwpwm); > + writel(val, spwm->base + SIRF_PWM_ENABLE_POSTCLOCK); > + > + /* disable preclock */ > + val = readl(spwm->base + SIRF_PWM_ENABLE_PRECLOCK); > + val &= ~(1 << pwm->hwpwm); > + writel(val, spwm->base + SIRF_PWM_ENABLE_PRECLOCK); I think you need a lock to synchronize accesses to these registers. > +} > + > +static struct pwm_ops sirf_pwm_ops = { static const please. > + .enable = sirf_pwm_enable, > + .disable = sirf_pwm_disable, > + .config = sirf_pwm_config, > + .owner = THIS_MODULE, > +}; > + > +static int sirf_pwm_probe(struct platform_device *pdev) > +{ > + struct sirf_pwm *spwm; > + struct resource *mem_res; > + struct clk *clk_pwm_src; > + int ret; > + > + spwm = devm_kzalloc(&pdev->dev, sizeof(struct sirf_pwm), > + GFP_KERNEL); "sizeof(*spwm)", please. And the above fits on a single line, no need to wrap them. > + if (!spwm) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, spwm); > + > + mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + spwm->base = devm_ioremap_resource(&pdev->dev, mem_res); > + if (!spwm->base) > + return -ENOMEM; > + > + /* > + * the 1st clock is for PWM controller > + */ > + spwm->clk = of_clk_get(pdev->dev.of_node, 0); Use a clock-names property and devm_clk_get() here, please. > + if (IS_ERR(spwm->clk)) { > + dev_err(&pdev->dev, "Get PWM controller clock failed.\n"); "failed to get PWM controller clock"? > + return PTR_ERR(spwm->clk); > + } > + clk_prepare_enable(spwm->clk); Space between the above two. Also you need to check the return value of clk_prepare_enable(). > + > + /* > + * the 2nd clock is the source to generate PWM waves I'd prefer "signals" over "waves". > + * it is the OSC on SiRFSoC The clock is configurable via DT, so this isn't necessarily true. Even if all hardware in existence currently works that way, it isn't a given to remain like that forever. > + */ > + clk_pwm_src = of_clk_get(pdev->dev.of_node, 1); > + if (IS_ERR(clk_pwm_src)) { > + dev_err(&pdev->dev, "Get PWM source clock failed.\n"); "failed to get PWM source clock"? > + return PTR_ERR(clk_pwm_src); > + } > + spwm->src_clk_rate = clk_get_rate(clk_pwm_src); Space between the above two please. > + clk_put(clk_pwm_src); Why drop the reference to it here? Shouldn't you keep it around and even call clk_prepare_enable() on it to make sure it's actually on? > + > + spwm->chip.dev = &pdev->dev; > + spwm->chip.ops = &sirf_pwm_ops; > + spwm->chip.base = 0; > + spwm->chip.npwm = SIRF_PWM_CHL_NUM; > + > + ret = pwmchip_add(&spwm->chip); > + if (ret < 0) { > + dev_err(&pdev->dev, "failed to register pwm\n"); "PWM" please. > + clk_disable_unprepare(spwm->clk); > + return ret; > + } > + > + return 0; > +} > + > +static int sirf_pwm_remove(struct platform_device *pdev) > +{ > + struct sirf_pwm *spwm = platform_get_drvdata(pdev); > + clk_disable_unprepare(spwm->clk); I'd prefer a blank line between the two above. > + clk_put(spwm->clk); > + > + pwmchip_remove(&spwm->chip); > + return 0; This should be "return pwmchip_remove(...);". > +} > + > +#ifdef CONFIG_PM_SLEEP > +static int sirf_pwm_suspend(struct device *dev) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct sirf_pwm *spwm = platform_get_drvdata(pdev); > + > + clk_disable_unprepare(spwm->clk); > + > + return 0; > +} > + > +static void sirf_pwm_config_restore(struct sirf_pwm *spwm) > +{ > + struct pwm_device *pwm; > + int i; > + > + for (i = 0; i < spwm->chip.npwm; i++) { > + pwm = &spwm->chip.pwms[i]; > + /* > + * while restoring from hibernation, state of pwm is enabled, > + * but PWM hardware is not re-enabled > + */ > + if (test_bit(PWMF_REQUESTED, &pwm->flags) && > + test_bit(PWMF_ENABLED, &pwm->flags)) Indentation is off by one space here. > + sirf_pwm_enable(&spwm->chip, pwm); > + } > +} > + > +static int sirf_pwm_resume(struct device *dev) > +{ > + struct sirf_pwm *spwm = dev_get_drvdata(dev); > + > + clk_prepare_enable(spwm->clk); > + > + sirf_pwm_config_restore(spwm); > + > + return 0; > +} > + > +static int sirf_pwm_restore(struct device *dev) > +{ > + struct sirf_pwm *spwm = dev_get_drvdata(dev); > + > + /* back from hibernation, clock is already enabled */ > + sirf_pwm_config_restore(spwm); Why is the clock already enabled? Shouldn't it be off until this driver enables it? > + > + return 0; > +} > + > +#else > +#define sirf_pwm_resume NULL > +#define sirf_pwm_suspend NULL > +#define sirf_pwm_restore NULL > +#endif > + > +static const struct dev_pm_ops sirf_pwm_pm_ops = { > + .suspend = sirf_pwm_suspend, > + .resume = sirf_pwm_resume, > + .restore = sirf_pwm_restore, > +}; Because if you can unify _resume and _restore, this could all be simplified using SIMPLE_DEV_PM_OPS(). > + > +static const struct of_device_id sirf_pwm_of_match[] = { > + { .compatible = "sirf,prima2-pwm", }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, sirf_pwm_of_match); > + > +static struct platform_driver sirf_pwm_driver = { > + .driver = { > + .name = "prima2-pwm", "sirf-pwm"? > + .owner = THIS_MODULE, This is no longer necessary. > + .pm = &sirf_pwm_pm_ops, > + .of_match_table = sirf_pwm_of_match, > + }, > + .probe = sirf_pwm_probe, > + .remove = sirf_pwm_remove, > +}; > + > +module_platform_driver(sirf_pwm_driver); > + > +MODULE_DESCRIPTION("SIRF serial SoC PWM device core driver"); > +MODULE_AUTHOR("RongJun Ying <Rongjun.Ying@csr.com>, " > + "Huayi Li <huayi.li@csr.com>"); These could simply be two separate MODULE_AUTHOR() entries. > +MODULE_LICENSE("GPL v2"); The file header says GPL v2 or later, which would be "GPL". "GPL v2" is GPL v2 only.
2014-02-26 22:10 GMT+08:00 Thierry Reding <thierry.reding@gmail.com>: > On Sat, Feb 08, 2014 at 06:23:22PM +0800, Barry Song wrote: >> From: Rongjun Ying <Rongjun.ying@csr.com> >> >> PWM controller of CSR SiRFSoC can generate 7 independent outputs. Each output > > "The PWM controller of the CSR SiRFSoC..." and "Each output's..." > >> duty cycle can be adjusted by setting the corresponding wait & hold registers. >> >> Supports 7 independent channel output: 6 for external(channel0-5) and 1 for >> internal(channel6). > > This repeats part of the first sentence. Perhaps: > > There are 6 external channels (0 to 5) and 1 internal channel (6). > > ? > >> Supports wide frequency range: divide by 2 to 65536*2 of source clock. > > "Supports a wide frequency range: the source clock divider can be from 2 > up to 65536*2". well. this was copied from datasheet :-) > >> Documentation/devicetree/bindings/pwm/pwm-sirf.txt | 17 + >> arch/arm/boot/dts/atlas6.dtsi | 3 +- >> arch/arm/boot/dts/prima2.dtsi | 3 +- >> drivers/pwm/Kconfig | 9 + >> drivers/pwm/Makefile | 1 + >> drivers/pwm/pwm-sirf.c | 308 ++++++++++++++++++++ >> 6 files changed, 339 insertions(+), 2 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sirf.txt >> create mode 100644 drivers/pwm/pwm-sirf.c >> >> diff --git a/Documentation/devicetree/bindings/pwm/pwm-sirf.txt b/Documentation/devicetree/bindings/pwm/pwm-sirf.txt >> new file mode 100644 >> index 0000000..4b10109 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pwm/pwm-sirf.txt >> @@ -0,0 +1,17 @@ >> +SiRF prima2 & atlas6 PWM drivers >> + >> +Required properties: >> +- compatible: "sirf,prima2-pwm" >> +- reg: physical base address and length of the controller's registers >> +- #pwm-cells: should be 2. The first cell specifies the per-chip index >> + of the PWM to use and the second cell is the period in nanoseconds. >> +- clocks: from common clock binding: the 1st clock is for PWM controller >> + the 2nd clock is the source to generate PWM waves >> + >> +Example: >> +pwm: pwm@b0130000 { >> + compatible = "sirf,prima2-pwm"; >> + #pwm-cells = <2>; >> + reg = <0xb0130000 0x10000>; >> + clocks = <&clks 21>, <&clks 1>; >> +}; > > Please move this into a separate patch and Cc the device tree bindings > maintainers. There's nothing non-standard in this binding as far as I > can tell, but I'd still like to give them an opportunity to object. ok. > > Also you should be documenting the clock-names property here as well. ok. > >> diff --git a/arch/arm/boot/dts/atlas6.dtsi b/arch/arm/boot/dts/atlas6.dtsi >> index f8674bc..5a09815 100644 >> --- a/arch/arm/boot/dts/atlas6.dtsi >> +++ b/arch/arm/boot/dts/atlas6.dtsi >> @@ -614,8 +614,9 @@ >> >> pwm@b0130000 { >> compatible = "sirf,prima2-pwm"; >> + #pwm-cells = <2>; >> reg = <0xb0130000 0x10000>; >> - clocks = <&clks 21>; >> + clocks = <&clks 21>, <&clks 1>; >> }; >> >> efusesys@b0140000 { >> diff --git a/arch/arm/boot/dts/prima2.dtsi b/arch/arm/boot/dts/prima2.dtsi >> index 0e21993..3439e48 100644 >> --- a/arch/arm/boot/dts/prima2.dtsi >> +++ b/arch/arm/boot/dts/prima2.dtsi >> @@ -642,8 +642,9 @@ >> >> pwm@b0130000 { >> compatible = "sirf,prima2-pwm"; >> + #pwm-cells = <2>; >> reg = <0xb0130000 0x10000>; >> - clocks = <&clks 21>; >> + clocks = <&clks 21>, <&clks 1>; >> }; >> >> efusesys@b0140000 { > > These changes need to go into separate patches and go in through the > arm-soc tree. ok. i will include this in my tree with your ack after you give and send pull-request including this one, hoping before the 3.15 merge window. > >> diff --git a/drivers/pwm/pwm-sirf.c b/drivers/pwm/pwm-sirf.c >> new file mode 100644 >> index 0000000..b717de0 >> --- /dev/null >> +++ b/drivers/pwm/pwm-sirf.c >> @@ -0,0 +1,308 @@ >> +/* >> + * SIRF serial SoC PWM device core driver >> + * >> + * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group company. >> + * >> + * Licensed under GPLv2 or later. >> + */ > [...] >> +#define SIRF_PWM_CHL_NUM 7 > > This is only used in a single location with a well-known meaning, so no > need to use a symbolic name. ok. so you prefer: "spwm->chip.npwm = 7;" ? > >> +#define SIRF_PWM_BLS_GRP_NUM 16 > > This isn't used as far as I can tell. > real. >> +struct sirf_pwm { >> + void __iomem *base; >> + struct clk *clk; >> + struct pwm_chip chip; >> + unsigned long src_clk_rate; >> +}; > > Please drop the alignment of the structure field. Also perhaps move the > chip field to be the first, so that the upcasting below can be turned > into a noop. > this and all other indentation, whitespace, inline and u32 etc. are ok to me. >> +#define to_sirf_chip(chip) container_of(chip, struct sirf_pwm, chip) > > Please make this a static inline function. > >> + >> +static unsigned int sirf_pwm_ns_to_cycles(struct pwm_chip *chip, unsigned int time_ns) > > Please wrap this to go on a single line. > >> +{ >> + struct sirf_pwm *spwm = to_sirf_chip(chip); >> + u64 dividend; >> + unsigned int cycle; >> + >> + dividend = spwm->src_clk_rate * time_ns + NSEC_PER_SEC / 2; >> + do_div(dividend, NSEC_PER_SEC); >> + >> + cycle = dividend & 0xFFFFFFFFUL; > > I don't think you need the mask, since you're casting to a 32-bit > unsigned integer anyway. > >> + >> + return cycle > 1 ? cycle : 1; >> +} >> + >> +static int sirf_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, >> + int duty_ns, int period_ns) > > Please align arguments on subsequent lines with those of the first. > >> +{ >> + unsigned int period_cycles, high_cycles, low_cycles; >> + unsigned int val; > > u32 please. > >> + struct sirf_pwm *spwm = to_sirf_chip(chip); >> + >> + period_cycles = sirf_pwm_ns_to_cycles(chip, period_ns); >> + >> + high_cycles = sirf_pwm_ns_to_cycles(chip, duty_ns); > > No need for the blank line above, since there's no blank line below > either. > >> + low_cycles = period_cycles - high_cycles; > >> + >> + if (period_cycles == 1) { >> + /* bypass mode */ >> + val = readl(spwm->base + SIRF_PWM_SELECT_PRECLK); >> + val |= 0x1 << (BYPASS_MODE_BIT + pwm->hwpwm); >> + writel(val, spwm->base + SIRF_PWM_SELECT_PRECLK); >> + dev_warn(chip->dev, "period is too short!\n"); > > What does the bypass mode do? Would it perhaps be better to make this an > outright error rather than only warning about it? it means the clock can not serve the pwm (freq,duty) requirement, so move to a "workaround" frequency. but this can be dropped and return error code. > >> + } else { >> + /* divider mode */ >> + val = readl(spwm->base + SIRF_PWM_SELECT_PRECLK); >> + val &= ~(0x1 << (BYPASS_MODE_BIT + pwm->hwpwm)); >> + writel(val, spwm->base + SIRF_PWM_SELECT_PRECLK); >> + >> + if (high_cycles == period_cycles) { >> + high_cycles--; >> + low_cycles = 1; >> + } >> + >> + writel(high_cycles, spwm->base + SIRF_PWM_GET_WAIT_OFFSET(pwm->hwpwm)); >> + writel(low_cycles, spwm->base + SIRF_PWM_GET_HOLD_OFFSET(pwm->hwpwm)); >> + } >> + >> + return 0; >> +} >> + >> +static int sirf_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) >> +{ >> + struct sirf_pwm *spwm = to_sirf_chip(chip); >> + unsigned int val; > > The type used by readl() and writel() is u32. > >> + /* disable preclock */ >> + val = readl(spwm->base + SIRF_PWM_ENABLE_PRECLOCK); >> + val &= ~(1 << pwm->hwpwm); >> + writel(val, spwm->base + SIRF_PWM_ENABLE_PRECLOCK); >> + >> + /* select preclock source must after disable preclk*/ >> + val = readl(spwm->base + SIRF_PWM_SELECT_PRECLK); >> + val &= ~(0x7 << (SRC_FIELD_SIZE * pwm->hwpwm)); >> + writel(val, spwm->base + SIRF_PWM_SELECT_PRECLK); >> + /* wait for some time */ >> + udelay(100); > > usleep_range() perhaps? > >> +static void sirf_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) >> +{ >> + unsigned int val; > > u32 again. > >> + struct sirf_pwm *spwm = to_sirf_chip(chip); >> + /* disable output */ > > Blank line between the above two, please. > >> + val = readl(spwm->base + SIRF_PWM_OE); >> + val &= ~(1 << pwm->hwpwm); >> + writel(val, spwm->base + SIRF_PWM_OE); >> + >> + /* disable postclock */ >> + val = readl(spwm->base + SIRF_PWM_ENABLE_POSTCLOCK); >> + val &= ~(1 << pwm->hwpwm); >> + writel(val, spwm->base + SIRF_PWM_ENABLE_POSTCLOCK); >> + >> + /* disable preclock */ >> + val = readl(spwm->base + SIRF_PWM_ENABLE_PRECLOCK); >> + val &= ~(1 << pwm->hwpwm); >> + writel(val, spwm->base + SIRF_PWM_ENABLE_PRECLOCK); > > I think you need a lock to synchronize accesses to these registers. > >> +} >> + >> +static struct pwm_ops sirf_pwm_ops = { > > static const please. > >> + .enable = sirf_pwm_enable, >> + .disable = sirf_pwm_disable, >> + .config = sirf_pwm_config, >> + .owner = THIS_MODULE, >> +}; >> + >> +static int sirf_pwm_probe(struct platform_device *pdev) >> +{ >> + struct sirf_pwm *spwm; >> + struct resource *mem_res; >> + struct clk *clk_pwm_src; >> + int ret; >> + >> + spwm = devm_kzalloc(&pdev->dev, sizeof(struct sirf_pwm), >> + GFP_KERNEL); > > "sizeof(*spwm)", please. And the above fits on a single line, no need to > wrap them. > >> + if (!spwm) >> + return -ENOMEM; >> + >> + platform_set_drvdata(pdev, spwm); >> + >> + mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + spwm->base = devm_ioremap_resource(&pdev->dev, mem_res); >> + if (!spwm->base) >> + return -ENOMEM; >> + >> + /* >> + * the 1st clock is for PWM controller >> + */ >> + spwm->clk = of_clk_get(pdev->dev.of_node, 0); > > Use a clock-names property and devm_clk_get() here, please. > >> + if (IS_ERR(spwm->clk)) { >> + dev_err(&pdev->dev, "Get PWM controller clock failed.\n"); > > "failed to get PWM controller clock"? > >> + return PTR_ERR(spwm->clk); >> + } >> + clk_prepare_enable(spwm->clk); > > Space between the above two. Also you need to check the return value of > clk_prepare_enable(). > >> + >> + /* >> + * the 2nd clock is the source to generate PWM waves > > I'd prefer "signals" over "waves". > >> + * it is the OSC on SiRFSoC > > The clock is configurable via DT, so this isn't necessarily true. Even > if all hardware in existence currently works that way, it isn't a given > to remain like that forever. the current consideration is that it is something SoC specific, the clock source is an OSC with fixed frequency 26MHz. but if we look pwm as a IP module, it is better to look this clock as a normal clock as you said. > >> + */ >> + clk_pwm_src = of_clk_get(pdev->dev.of_node, 1); >> + if (IS_ERR(clk_pwm_src)) { >> + dev_err(&pdev->dev, "Get PWM source clock failed.\n"); > > "failed to get PWM source clock"? > >> + return PTR_ERR(clk_pwm_src); >> + } >> + spwm->src_clk_rate = clk_get_rate(clk_pwm_src); > > Space between the above two please. > >> + clk_put(clk_pwm_src); > > Why drop the reference to it here? Shouldn't you keep it around and even > call clk_prepare_enable() on it to make sure it's actually on? same with above. > >> + >> + spwm->chip.dev = &pdev->dev; >> + spwm->chip.ops = &sirf_pwm_ops; >> + spwm->chip.base = 0; >> + spwm->chip.npwm = SIRF_PWM_CHL_NUM; >> + >> + ret = pwmchip_add(&spwm->chip); >> + if (ret < 0) { >> + dev_err(&pdev->dev, "failed to register pwm\n"); > > "PWM" please. > >> + clk_disable_unprepare(spwm->clk); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static int sirf_pwm_remove(struct platform_device *pdev) >> +{ >> + struct sirf_pwm *spwm = platform_get_drvdata(pdev); >> + clk_disable_unprepare(spwm->clk); > > I'd prefer a blank line between the two above. > >> + clk_put(spwm->clk); >> + >> + pwmchip_remove(&spwm->chip); >> + return 0; > > This should be "return pwmchip_remove(...);". > >> +} >> + >> +#ifdef CONFIG_PM_SLEEP >> +static int sirf_pwm_suspend(struct device *dev) >> +{ >> + struct platform_device *pdev = to_platform_device(dev); >> + struct sirf_pwm *spwm = platform_get_drvdata(pdev); >> + >> + clk_disable_unprepare(spwm->clk); >> + >> + return 0; >> +} >> + >> +static void sirf_pwm_config_restore(struct sirf_pwm *spwm) >> +{ >> + struct pwm_device *pwm; >> + int i; >> + >> + for (i = 0; i < spwm->chip.npwm; i++) { >> + pwm = &spwm->chip.pwms[i]; >> + /* >> + * while restoring from hibernation, state of pwm is enabled, >> + * but PWM hardware is not re-enabled >> + */ >> + if (test_bit(PWMF_REQUESTED, &pwm->flags) && >> + test_bit(PWMF_ENABLED, &pwm->flags)) > > Indentation is off by one space here. > >> + sirf_pwm_enable(&spwm->chip, pwm); >> + } >> +} >> + >> +static int sirf_pwm_resume(struct device *dev) >> +{ >> + struct sirf_pwm *spwm = dev_get_drvdata(dev); >> + >> + clk_prepare_enable(spwm->clk); >> + >> + sirf_pwm_config_restore(spwm); >> + >> + return 0; >> +} >> + >> +static int sirf_pwm_restore(struct device *dev) >> +{ >> + struct sirf_pwm *spwm = dev_get_drvdata(dev); >> + >> + /* back from hibernation, clock is already enabled */ >> + sirf_pwm_config_restore(spwm); > > Why is the clock already enabled? Shouldn't it be off until this driver > enables it? this issue is special. the PWM is not disabled during hibernation. it is not like a normal device because user-experiences want to keep the LCD light during the produre of hibernation and before the final shutdown. it is something similar with commit 1dea1fd0: https://git.kernel.org/cgit/linux/kernel/git/baohua/linux.git/commit/?id=1dea1fd09246ada581a99d0669108eea94b7bfee > >> + >> + return 0; >> +} >> + >> +#else >> +#define sirf_pwm_resume NULL >> +#define sirf_pwm_suspend NULL >> +#define sirf_pwm_restore NULL >> +#endif >> + >> +static const struct dev_pm_ops sirf_pwm_pm_ops = { >> + .suspend = sirf_pwm_suspend, >> + .resume = sirf_pwm_resume, >> + .restore = sirf_pwm_restore, >> +}; > > Because if you can unify _resume and _restore, this could all be > simplified using SIMPLE_DEV_PM_OPS(). it seems not real. suspend to ram, lcd light is off, during suspend to disk, lcd is on before the final shutdown. > >> + >> +static const struct of_device_id sirf_pwm_of_match[] = { >> + { .compatible = "sirf,prima2-pwm", }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, sirf_pwm_of_match); >> + >> +static struct platform_driver sirf_pwm_driver = { >> + .driver = { >> + .name = "prima2-pwm", > > "sirf-pwm"? > >> + .owner = THIS_MODULE, > > This is no longer necessary. > >> + .pm = &sirf_pwm_pm_ops, >> + .of_match_table = sirf_pwm_of_match, >> + }, >> + .probe = sirf_pwm_probe, >> + .remove = sirf_pwm_remove, >> +}; >> + >> +module_platform_driver(sirf_pwm_driver); >> + >> +MODULE_DESCRIPTION("SIRF serial SoC PWM device core driver"); >> +MODULE_AUTHOR("RongJun Ying <Rongjun.Ying@csr.com>, " >> + "Huayi Li <huayi.li@csr.com>"); > > These could simply be two separate MODULE_AUTHOR() entries. ok. > >> +MODULE_LICENSE("GPL v2"); > > The file header says GPL v2 or later, which would be "GPL". "GPL v2" is > GPL v2 only. > -barry -- 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
Hello Barry, On 2014-02-08, Barry Song <21cnbao@gmail.com> wrote: > From: Rongjun Ying <Rongjun.ying@csr.com> > > PWM controller of CSR SiRFSoC can generate 7 independent outputs. Each output > duty cycle can be adjusted by setting the corresponding wait & hold registers. > > Supports 7 independent channel output: 6 for external(channel0-5) and 1 for > internal(channel6). > > Supports wide frequency range: divide by 2 to 65536*2 of source clock. > > Signed-off-by: Rongjun Ying <Rongjun.ying@csr.com> > Signed-off-by: Huayi Li <Huayi.Li@csr.com> > Signed-off-by: Barry Song <Baohua.Song@csr.com> > --- > -v3: > add "depends on" COMPILE_TEST according to Arnd's feedback; > move the pwm clock source to dts according to Arnd's feedback; > add lost dt-binding document > > Documentation/devicetree/bindings/pwm/pwm-sirf.txt | 17 + > arch/arm/boot/dts/atlas6.dtsi | 3 +- > arch/arm/boot/dts/prima2.dtsi | 3 +- > drivers/pwm/Kconfig | 9 + > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-sirf.c | 308 ++++++++++++++++++++ > 6 files changed, 339 insertions(+), 2 deletions(-) > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sirf.txt > create mode 100644 drivers/pwm/pwm-sirf.c > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-sirf.txt b/Documentation/devicetree/bindings/pwm/pwm-sirf.txt > new file mode 100644 > index 0000000..4b10109 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/pwm-sirf.txt > @@ -0,0 +1,17 @@ > +SiRF prima2 & atlas6 PWM drivers > + > +Required properties: > +- compatible: "sirf,prima2-pwm" > +- reg: physical base address and length of the controller's registers > +- #pwm-cells: should be 2. The first cell specifies the per-chip index > + of the PWM to use and the second cell is the period in nanoseconds. > +- clocks: from common clock binding: the 1st clock is for PWM controller > + the 2nd clock is the source to generate PWM waves > + Describing the clocks this way seems limiting to me. Each output of the PWM controller on SiRFatlasVI and SiRFprimaII can independently use one clock among many for signal generation, with three PLLs and two external clock inputs available. If we specify the source clock for the signal generation in the device tree as you do here, we will prevent the use of those different input clocks when incompatible frequencies are required, such as 32,768 Hz on one output line and 44,100 Hz on another one. As the clock tree for SiRF SoCs is described in the source files in drivers/clk/sirf/*, it does not appear to me that the functional clock needs to be configured in the device tree: the names of available clock sources can be provided statically by the device tree matching table the pwm-sirf.c. Moreover, this would continue to work with the current (undocumented) bindings for SiRFatlasVI and SiRFprimaII. Alternatively, to keep the configuration in the device tree, all valid inputs for this SoC could be described here, instead of a single one. The choice of the clock used for each output can then be selected when configuring an output, with a selection algorithm using the best input for a given period in of_xlate() and config() callbacks.
Hello Romain, 2014-02-27 0:19 GMT+08:00 Romain Izard <romain.izard.pro@gmail.com>: > Hello Barry, > > On 2014-02-08, Barry Song <21cnbao@gmail.com> wrote: >> From: Rongjun Ying <Rongjun.ying@csr.com> >> >> PWM controller of CSR SiRFSoC can generate 7 independent outputs. Each output >> duty cycle can be adjusted by setting the corresponding wait & hold registers. >> >> Supports 7 independent channel output: 6 for external(channel0-5) and 1 for >> internal(channel6). >> >> Supports wide frequency range: divide by 2 to 65536*2 of source clock. >> >> Signed-off-by: Rongjun Ying <Rongjun.ying@csr.com> >> Signed-off-by: Huayi Li <Huayi.Li@csr.com> >> Signed-off-by: Barry Song <Baohua.Song@csr.com> >> --- >> -v3: >> add "depends on" COMPILE_TEST according to Arnd's feedback; >> move the pwm clock source to dts according to Arnd's feedback; >> add lost dt-binding document >> >> Documentation/devicetree/bindings/pwm/pwm-sirf.txt | 17 + >> arch/arm/boot/dts/atlas6.dtsi | 3 +- >> arch/arm/boot/dts/prima2.dtsi | 3 +- >> drivers/pwm/Kconfig | 9 + >> drivers/pwm/Makefile | 1 + >> drivers/pwm/pwm-sirf.c | 308 ++++++++++++++++++++ >> 6 files changed, 339 insertions(+), 2 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sirf.txt >> create mode 100644 drivers/pwm/pwm-sirf.c >> >> diff --git a/Documentation/devicetree/bindings/pwm/pwm-sirf.txt b/Documentation/devicetree/bindings/pwm/pwm-sirf.txt >> new file mode 100644 >> index 0000000..4b10109 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pwm/pwm-sirf.txt >> @@ -0,0 +1,17 @@ >> +SiRF prima2 & atlas6 PWM drivers >> + >> +Required properties: >> +- compatible: "sirf,prima2-pwm" >> +- reg: physical base address and length of the controller's registers >> +- #pwm-cells: should be 2. The first cell specifies the per-chip index >> + of the PWM to use and the second cell is the period in nanoseconds. >> +- clocks: from common clock binding: the 1st clock is for PWM controller >> + the 2nd clock is the source to generate PWM waves >> + > > Describing the clocks this way seems limiting to me. > > Each output of the PWM controller on SiRFatlasVI and SiRFprimaII can > independently use one clock among many for signal generation, with > three PLLs and two external clock inputs available. yes, that is the hardware spec. and i feel so happy you have read it. now i get one more sirfsoc friend. 32K of RTC is too small to generate a normal PWM signal, PLLs in the design can change for DVFS. so that means a notifier callback is needed if PLL is changed for CPUFreq, this makes SW buggy and complex. when i reviewed the source codes in our local git, i decided to move to a general and clean SW solution at the first step. > > If we specify the source clock for the signal generation in the device > tree as you do here, we will prevent the use of those different input > clocks when incompatible frequencies are required, such as 32,768 Hz on > one output line and 44,100 Hz on another one. i prefer we use a normal clock source which can provide normal clock rates. 26MHz is a suitable one which can provide a big scale. even though we can use multiple clock sources, clock sources are the knowledge of PWM and should not be visible to pwm clients as you mentioned below. > > As the clock tree for SiRF SoCs is described in the source files in > drivers/clk/sirf/*, it does not appear to me that the functional clock > needs to be configured in the device tree: the names of available clock > sources can be provided statically by the device tree matching table the > pwm-sirf.c. Moreover, this would continue to work with the current > (undocumented) bindings for SiRFatlasVI and SiRFprimaII. i don't understand "it does not appear to me that the functional clock needs to be configured in the device tree", we are now using the clock index in dts. the index is mapping with the clock indexes in driver codes as you said. > > Alternatively, to keep the configuration in the device tree, all valid > inputs for this SoC could be described here, instead of a single one. > > The choice of the clock used for each output can then be selected when > configuring an output, with a selection algorithm using the best input for > a given period in of_xlate() and config() callbacks. this is right in case we don't make clock sources visible to PWM clients. but for the moment, current driver have met the requirement of PWM clients like LCD and audio codec in our tests. so i think if there is some special rate we can not met in the future, we have an incremental patch for that. we might be able to have a "best-fit" algorithm to figure out "best-fit" clock source, even a quick check-up table for these special rates since they are few. > > -- > Romain Izard > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -barry -- 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
2014-02-27 3:49 GMT+01:00 Barry Song <21cnbao@gmail.com>: > Hello Romain, > > 2014-02-27 0:19 GMT+08:00 Romain Izard <romain.izard.pro@gmail.com>: >> >> Describing the clocks this way seems limiting to me. >> >> Each output of the PWM controller on SiRFatlasVI and SiRFprimaII can >> independently use one clock among many for signal generation, with >> three PLLs and two external clock inputs available. > > yes, that is the hardware spec. and i feel so happy you have read it. > now i get one more sirfsoc friend. > > 32K of RTC is too small to generate a normal PWM signal, PLLs in the > design can change for DVFS. > so that means a notifier callback is needed if PLL is changed for > CPUFreq, this makes SW buggy and complex. > Well, my specific concern was the use of the bypass mode on the 32kHz clock. As the PWM driver is also the interface for external clock distribution, and 32 kHz is a common requirement for external devices, I wish to avoid compounding error rates. I will get a less precise clock if I divide the 26 MHz instead. > when i reviewed the source codes in our local git, i decided to move > to a general and clean SW solution at the first step. >> >> If we specify the source clock for the signal generation in the device >> tree as you do here, we will prevent the use of those different input >> clocks when incompatible frequencies are required, such as 32,768 Hz on >> one output line and 44,100 Hz on another one. > > i prefer we use a normal clock source which can provide normal clock > rates. 26MHz is a suitable one which can provide a big scale. > even though we can use multiple clock sources, clock sources are the > knowledge of PWM and should not be visible to pwm clients as you > mentioned below. > Thanks, I understand how you got to this conclusion. >> >> As the clock tree for SiRF SoCs is described in the source files in >> drivers/clk/sirf/*, it does not appear to me that the functional clock >> needs to be configured in the device tree: the names of available clock >> sources can be provided statically by the device tree matching table the >> pwm-sirf.c. Moreover, this would continue to work with the current >> (undocumented) bindings for SiRFatlasVI and SiRFprimaII. > > i don't understand "it does not appear to me that the functional clock > needs to be configured in the device tree", we are now using the clock > index in dts. > the index is mapping with the clock indexes in driver codes as you said. > What I meant is that the clock tree in SiRF chips is not described in the DTS, as you can get for example with OMAP chips, but it is defined in tables in the source code. By extension, I meant that the functional clocks used by the PWM module can be hidden from the DTS in the same way. But you're right that it is not was your proposition does, and after checking the previous discussions I see Arnd asked you for this change. >> >> Alternatively, to keep the configuration in the device tree, all valid >> inputs for this SoC could be described here, instead of a single one. >> >> The choice of the clock used for each output can then be selected when >> configuring an output, with a selection algorithm using the best input for >> a given period in of_xlate() and config() callbacks. > > this is right in case we don't make clock sources visible to PWM > clients. but for the moment, current driver have met the requirement > of PWM clients like LCD and audio codec in our tests. > so i think if there is some special rate we can not met in the future, > we have an incremental patch for that. > we might be able to have a "best-fit" algorithm to figure out > "best-fit" clock source, even a quick check-up table for these special > rates since they are few. Perhaps this can be linked with the missing "clock-names" property, as this will make it possible to add multiple functional clocks, with "iclk" matching the interface clock (i.e. pwmc), and "fclk0", "fclk1", etc. matching all supported functional clocks. With only the 26 MHz clock, the node would look like: pwm: pwm@b0130000 { compatible = "sirf,prima2-pwm"; #pwm-cells = <2>; reg = <0xb0130000 0x10000>; clocks = <&clks 21>, <&clks 1>, <&clks 0>; clock-names = "iclk", "fclk0", "fclk3"; }; The result would look like this with both the 26 MHz and the 32 kHz clocks: pwm: pwm@b0130000 { compatible = "sirf,prima2-pwm"; #pwm-cells = <2>; reg = <0xb0130000 0x10000>; clocks = <&clks 21>, <&clks 1>, <&clks 0>; clock-names = "iclk", "fclk0", "fclk3"; }; And with all possible clocks: pwm: pwm@b0130000 { compatible = "sirf,prima2-pwm"; #pwm-cells = <2>; reg = <0xb0130000 0x10000>; clocks = <&clks 21>, <&clks 1>, <&clks 2>, <&clks 3>, <&clks 0>, <&clks 4>; clock-names = "iclk", "fclk0", "fclk1", "fclk2", "fclk3", "fclk4"; }; The code in the probe function would interpret the clock-name to build the clock/index mapping table. This would not change a lot the binding you proposed, as you still can describe only one functional clock, but it describes which index in the configuration register is linked to the proposed clock.
2014-02-27 18:51 GMT+08:00 Romain Izard <romain.izard.pro@gmail.com>: > 2014-02-27 3:49 GMT+01:00 Barry Song <21cnbao@gmail.com>: >> Hello Romain, >> >> 2014-02-27 0:19 GMT+08:00 Romain Izard <romain.izard.pro@gmail.com>: >>> >>> Describing the clocks this way seems limiting to me. >>> >>> Each output of the PWM controller on SiRFatlasVI and SiRFprimaII can >>> independently use one clock among many for signal generation, with >>> three PLLs and two external clock inputs available. >> >> yes, that is the hardware spec. and i feel so happy you have read it. >> now i get one more sirfsoc friend. >> >> 32K of RTC is too small to generate a normal PWM signal, PLLs in the >> design can change for DVFS. >> so that means a notifier callback is needed if PLL is changed for >> CPUFreq, this makes SW buggy and complex. >> > Well, my specific concern was the use of the bypass mode on the 32kHz > clock. As the PWM driver is also the interface for external clock distribution, > and 32 kHz is a common requirement for external devices, I wish to avoid > compounding error rates. I will get a less precise clock if I divide the 26 MHz > instead. Hi Romain, do you have a real user scenarios for this 32KHz? as the 1st step, i want a clean and general enough pwm driver, if there is any special requirement, i want to execute a "demanding page" when the "page fault" happened as this will make the whole flow more smooth. that means we make it lazy by incremental patches. but if you do want to use it for the moment, it is a "page fault" now. so we can have it immediately and it is better you can help to double-test :-) > >> when i reviewed the source codes in our local git, i decided to move >> to a general and clean SW solution at the first step. >>> >>> If we specify the source clock for the signal generation in the device >>> tree as you do here, we will prevent the use of those different input >>> clocks when incompatible frequencies are required, such as 32,768 Hz on >>> one output line and 44,100 Hz on another one. >> >> i prefer we use a normal clock source which can provide normal clock >> rates. 26MHz is a suitable one which can provide a big scale. >> even though we can use multiple clock sources, clock sources are the >> knowledge of PWM and should not be visible to pwm clients as you >> mentioned below. >> > Thanks, I understand how you got to this conclusion. > >>> >>> As the clock tree for SiRF SoCs is described in the source files in >>> drivers/clk/sirf/*, it does not appear to me that the functional clock >>> needs to be configured in the device tree: the names of available clock >>> sources can be provided statically by the device tree matching table the >>> pwm-sirf.c. Moreover, this would continue to work with the current >>> (undocumented) bindings for SiRFatlasVI and SiRFprimaII. >> >> i don't understand "it does not appear to me that the functional clock >> needs to be configured in the device tree", we are now using the clock >> index in dts. >> the index is mapping with the clock indexes in driver codes as you said. >> > What I meant is that the clock tree in SiRF chips is not described in the > DTS, as you can get for example with OMAP chips, but it is defined in > tables in the source code. By extension, I meant that the functional clocks > used by the PWM module can be hidden from the DTS in the same way. > But you're right that it is not was your proposition does, and after checking > the previous discussions I see Arnd asked you for this change. > >>> understood. >>> Alternatively, to keep the configuration in the device tree, all valid >>> inputs for this SoC could be described here, instead of a single one. >>> >>> The choice of the clock used for each output can then be selected when >>> configuring an output, with a selection algorithm using the best input for >>> a given period in of_xlate() and config() callbacks. >> >> this is right in case we don't make clock sources visible to PWM >> clients. but for the moment, current driver have met the requirement >> of PWM clients like LCD and audio codec in our tests. >> so i think if there is some special rate we can not met in the future, >> we have an incremental patch for that. >> we might be able to have a "best-fit" algorithm to figure out >> "best-fit" clock source, even a quick check-up table for these special >> rates since they are few. > > Perhaps this can be linked with the missing "clock-names" property, as this > will make it possible to add multiple functional clocks, with "iclk" matching > the interface clock (i.e. pwmc), and "fclk0", "fclk1", etc. matching > all supported > functional clocks. > > With only the 26 MHz clock, the node would look like: > > pwm: pwm@b0130000 { > compatible = "sirf,prima2-pwm"; > #pwm-cells = <2>; > reg = <0xb0130000 0x10000>; > clocks = <&clks 21>, <&clks 1>, <&clks 0>; > clock-names = "iclk", "fclk0", "fclk3"; > }; > > The result would look like this with both the 26 MHz and the 32 kHz clocks: > > pwm: pwm@b0130000 { > compatible = "sirf,prima2-pwm"; > #pwm-cells = <2>; > reg = <0xb0130000 0x10000>; > clocks = <&clks 21>, <&clks 1>, <&clks 0>; > clock-names = "iclk", "fclk0", "fclk3"; > }; > > And with all possible clocks: > > pwm: pwm@b0130000 { > compatible = "sirf,prima2-pwm"; > #pwm-cells = <2>; > reg = <0xb0130000 0x10000>; > clocks = <&clks 21>, <&clks 1>, <&clks 2>, <&clks 3>, <&clks > 0>, <&clks 4>; > clock-names = "iclk", "fclk0", "fclk1", "fclk2", "fclk3", "fclk4"; > }; > > The code in the probe function would interpret the clock-name to build > the clock/index mapping table. This would not change a lot the binding you > proposed, as you still can describe only one functional clock, but it describes > which index in the configuration register is linked to the proposed clock. > yes. very clear. the only two things left 1. fclk should be named for pwm not for rtc, osc and pll, so the names might be ugly by fclk0~N. 2. the driver need to manage all of the clocks. yes, it can. but it might be ugly again. we can either request the clk at the time of pwm configuration, or get all directy in probe and maintain an array. i am thinking what is the best way for it. if we do that by an incremental patch, it might be more concentrated and understandable. > -- > Romain Izard -barry -- 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
2014-02-28 11:01 GMT+08:00 Barry Song <21cnbao@gmail.com>: > 2014-02-27 18:51 GMT+08:00 Romain Izard <romain.izard.pro@gmail.com>: >> 2014-02-27 3:49 GMT+01:00 Barry Song <21cnbao@gmail.com>: >>> Hello Romain, >>> >>> 2014-02-27 0:19 GMT+08:00 Romain Izard <romain.izard.pro@gmail.com>: >>>> >>>> Describing the clocks this way seems limiting to me. >>>> >>>> Each output of the PWM controller on SiRFatlasVI and SiRFprimaII can >>>> independently use one clock among many for signal generation, with >>>> three PLLs and two external clock inputs available. >>> >>> yes, that is the hardware spec. and i feel so happy you have read it. >>> now i get one more sirfsoc friend. >>> >>> 32K of RTC is too small to generate a normal PWM signal, PLLs in the >>> design can change for DVFS. >>> so that means a notifier callback is needed if PLL is changed for >>> CPUFreq, this makes SW buggy and complex. >>> >> Well, my specific concern was the use of the bypass mode on the 32kHz >> clock. As the PWM driver is also the interface for external clock distribution, >> and 32 kHz is a common requirement for external devices, I wish to avoid >> compounding error rates. I will get a less precise clock if I divide the 26 MHz >> instead. > > Hi Romain, > > do you have a real user scenarios for this 32KHz? as the 1st step, i > want a clean and general enough pwm driver, if there is any special > requirement, i want to execute a "demanding page" when the "page > fault" happened as this will make the whole flow more smooth. that > means we make it lazy by incremental patches. but if you do want to > use it for the moment, it is a "page fault" now. so we can have it > immediately and it is better you can help to double-test :-) Huayi told me bluetooth/wifi is using 32768. if we bypass rtc directly, it is better. currently the 26MHz can be divided to 32K, but i agree with you that bypassing rtc is better. -barry -- 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
Hello Barry, 2014-02-28 4:01 GMT+01:00 Barry Song <21cnbao@gmail.com>: > > Hi Romain, > > do you have a real user scenarios for this 32KHz? as the 1st step, i > want a clean and general enough pwm driver, if there is any special > requirement, i want to execute a "demanding page" when the "page > fault" happened as this will make the whole flow more smooth. that > means we make it lazy by incremental patches. but if you do want to > use it for the moment, it is a "page fault" now. so we can have it > immediately and it is better you can help to double-test :-) My use case is to connect a Bluetooth module, in fact with a CSR chip. For testing, I will try to see what I can do, as for now my boards are not running with the mainline. But I'm very interested in transitioning on the mainline in due time, so that's the reason I keep an eye on the mailing lists. Conversely, it's not critical at all to get the feature in right now, but it should not be impossible to do so in the future, especially when taking the 'stable interface' aspect of device tree bindings into account. > 2014-02-27 18:51 GMT+08:00 Romain Izard <romain.izard.pro@gmail.com>: >> Perhaps this can be linked with the missing "clock-names" property, as this >> will make it possible to add multiple functional clocks, with "iclk" matching >> the interface clock (i.e. pwmc), and "fclk0", "fclk1", etc. matching >> all supported >> functional clocks. >> >> With only the 26 MHz clock, the node would look like: >> >> pwm: pwm@b0130000 { >> compatible = "sirf,prima2-pwm"; >> #pwm-cells = <2>; >> reg = <0xb0130000 0x10000>; >> clocks = <&clks 21>, <&clks 1>, <&clks 0>; >> clock-names = "iclk", "fclk0", "fclk3"; >> }; >> Aargh. I meant this: pwm: pwm@b0130000 { compatible = "sirf,prima2-pwm"; #pwm-cells = <2>; reg = <0xb0130000 0x10000>; clocks = <&clks 21>, <&clks 1>; clock-names = "iclk", "fclk0"; }; >> The result would look like this with both the 26 MHz and the 32 kHz clocks: >> >> pwm: pwm@b0130000 { >> compatible = "sirf,prima2-pwm"; >> #pwm-cells = <2>; >> reg = <0xb0130000 0x10000>; >> clocks = <&clks 21>, <&clks 1>, <&clks 0>; >> clock-names = "iclk", "fclk0", "fclk3"; >> }; >> >> And with all possible clocks: >> >> pwm: pwm@b0130000 { >> compatible = "sirf,prima2-pwm"; >> #pwm-cells = <2>; >> reg = <0xb0130000 0x10000>; >> clocks = <&clks 21>, <&clks 1>, <&clks 2>, <&clks 3>, <&clks >> 0>, <&clks 4>; >> clock-names = "iclk", "fclk0", "fclk1", "fclk2", "fclk3", "fclk4"; >> }; >> >> The code in the probe function would interpret the clock-name to build >> the clock/index mapping table. This would not change a lot the binding you >> proposed, as you still can describe only one functional clock, but it describes >> which index in the configuration register is linked to the proposed clock. >> > > yes. very clear. the only two things left > > 1. fclk should be named for pwm not for rtc, osc and pll, so the > names might be ugly by fclk0~N. > For me, it's important to keep the number in the name of the clock as it is the source of the information for binding the device tree clock on one side and the clock source index in the selection register on the other side. If we do not do it this way, we cannot easily handle new clocks in the future in the binding. On the other hand, I strongly dislike the current clock specification as <&clks #x>. It would be probably a good thing to add a header linking the numbers to symbols, and we would then get: clocks = <&clks SIRFCLK_A6_PWM>, <&clks SIRFCLK_A6_OSC>, <&clks SIRFCLK_A6_PLL1>, <&clks SIRFCLK_A6_PLL2>, <&clks SIRFCLK_A6_RTC>, <&clks SIRFCLK_16_PLL3>; clock-names = "iclk", "fclk0", "fclk1", "fclk2", "fclk3", "fclk4"; Note that as the binary output of the device tree would not change with this, we do not need to worry about backwards compatibility in this precise case. > 2. the driver need to manage all of the clocks. yes, it can. but it > might be ugly again. we can either request the clk at the time of pwm > configuration, or get all directy in probe and maintain an array. For me, what is important right now is to get the binding right. If the driver cannot do everything the binding allows, this is not a problem. But we define a binding that cannot easily be expanded, we will have problems when we need the new features. > i am thinking what is the best way for it. if we do that by an > incremental patch, it might be more concentrated and understandable. I'm all for this as well, that why for me the first syntax (corrected up there) should be valid. This way, you do not need to integrate the bypass support in this step, but you can add it later. Best regards,
2014-02-28 17:06 GMT+08:00 Romain Izard <romain.izard.pro@gmail.com>: > Hello Barry, > > 2014-02-28 4:01 GMT+01:00 Barry Song <21cnbao@gmail.com>: >> >> Hi Romain, >> >> do you have a real user scenarios for this 32KHz? as the 1st step, i >> want a clean and general enough pwm driver, if there is any special >> requirement, i want to execute a "demanding page" when the "page >> fault" happened as this will make the whole flow more smooth. that >> means we make it lazy by incremental patches. but if you do want to >> use it for the moment, it is a "page fault" now. so we can have it >> immediately and it is better you can help to double-test :-) > > My use case is to connect a Bluetooth module, in fact with a CSR chip. > For testing, I will try to see what I can do, as for now my boards are not > running with the mainline. But I'm very interested in transitioning on the > mainline in due time, so that's the reason I keep an eye on the mailing > lists. Conversely, it's not critical at all to get the feature in right now, but > it should not be impossible to do so in the future, especially when taking > the 'stable interface' aspect of device tree bindings into account. > >> 2014-02-27 18:51 GMT+08:00 Romain Izard <romain.izard.pro@gmail.com>: >>> Perhaps this can be linked with the missing "clock-names" property, as this >>> will make it possible to add multiple functional clocks, with "iclk" matching >>> the interface clock (i.e. pwmc), and "fclk0", "fclk1", etc. matching >>> all supported >>> functional clocks. >>> >>> With only the 26 MHz clock, the node would look like: >>> >>> pwm: pwm@b0130000 { >>> compatible = "sirf,prima2-pwm"; >>> #pwm-cells = <2>; >>> reg = <0xb0130000 0x10000>; >>> clocks = <&clks 21>, <&clks 1>, <&clks 0>; >>> clock-names = "iclk", "fclk0", "fclk3"; >>> }; >>> > > Aargh. I meant this: > > pwm: pwm@b0130000 { > compatible = "sirf,prima2-pwm"; > #pwm-cells = <2>; > reg = <0xb0130000 0x10000>; > clocks = <&clks 21>, <&clks 1>; > clock-names = "iclk", "fclk0"; > }; > >>> The result would look like this with both the 26 MHz and the 32 kHz clocks: >>> >>> pwm: pwm@b0130000 { >>> compatible = "sirf,prima2-pwm"; >>> #pwm-cells = <2>; >>> reg = <0xb0130000 0x10000>; >>> clocks = <&clks 21>, <&clks 1>, <&clks 0>; >>> clock-names = "iclk", "fclk0", "fclk3"; >>> }; >>> >>> And with all possible clocks: >>> >>> pwm: pwm@b0130000 { >>> compatible = "sirf,prima2-pwm"; >>> #pwm-cells = <2>; >>> reg = <0xb0130000 0x10000>; >>> clocks = <&clks 21>, <&clks 1>, <&clks 2>, <&clks 3>, <&clks >>> 0>, <&clks 4>; >>> clock-names = "iclk", "fclk0", "fclk1", "fclk2", "fclk3", "fclk4"; >>> }; >>> >>> The code in the probe function would interpret the clock-name to build >>> the clock/index mapping table. This would not change a lot the binding you >>> proposed, as you still can describe only one functional clock, but it describes >>> which index in the configuration register is linked to the proposed clock. >>> >> >> yes. very clear. the only two things left >> >> 1. fclk should be named for pwm not for rtc, osc and pll, so the >> names might be ugly by fclk0~N. >> > For me, it's important to keep the number in the name of the clock as it is > the source of the information for binding the device tree clock on one side > and the clock source index in the selection register on the other side. If > we do not do it this way, we cannot easily handle new clocks in the future > in the binding. > > On the other hand, I strongly dislike the current clock specification > as <&clks #x>. > It would be probably a good thing to add a header linking the numbers > to symbols, > and we would then get: > > clocks = <&clks SIRFCLK_A6_PWM>, > <&clks SIRFCLK_A6_OSC>, > <&clks SIRFCLK_A6_PLL1>, > <&clks SIRFCLK_A6_PLL2>, > <&clks SIRFCLK_A6_RTC>, > <&clks SIRFCLK_16_PLL3>; > clock-names = "iclk", "fclk0", "fclk1", "fclk2", "fclk3", "fclk4"; > > Note that as the binary output of the device tree would not change with this, we > do not need to worry about backwards compatibility in this precise case. this pwm controller is prima2-compatible, so it is actually a pwm controller highly bound with the special SoC. really strange things are we actually care about the clock types. we actually can't think fclk0~fclk4 as same things. for pll, i think it is buggy to use. for rtc, it only service special target for providing bypass 32KHz clock. if we look this controller a separate IP, we need to look 5 clock source to be coequal. but it is not the real case. i think people will hate the things that we have a separate bypass mode for fclk3, why it is 3 but not 2, 1, 4 and 5? that is why i move to a MACRO 26MHz in the original codes as i think it is a prima2 controller but not a controller used by prima2 even though we always want a IP module to be a IP module suitable for all SoCs. > >> 2. the driver need to manage all of the clocks. yes, it can. but it >> might be ugly again. we can either request the clk at the time of pwm >> configuration, or get all directy in probe and maintain an array. > > For me, what is important right now is to get the binding right. If the driver > cannot do everything the binding allows, this is not a problem. But we define > a binding that cannot easily be expanded, we will have problems when we > need the new features. > >> i am thinking what is the best way for it. if we do that by an >> incremental patch, it might be more concentrated and understandable. i think bypass mode for rtc will be included immediately since there is a real user. > > I'm all for this as well, that why for me the first syntax (corrected > up there) should > be valid. This way, you do not need to integrate the bypass support in > this step, > but you can add it later. > > Best regards, > -- > Romain Izard -barry -- 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
2014-02-28 18:07 GMT+08:00 Barry Song <21cnbao@gmail.com>: > 2014-02-28 17:06 GMT+08:00 Romain Izard <romain.izard.pro@gmail.com>: >> Hello Barry, >> >> 2014-02-28 4:01 GMT+01:00 Barry Song <21cnbao@gmail.com>: >>> >>> Hi Romain, >>> >>> do you have a real user scenarios for this 32KHz? as the 1st step, i >>> want a clean and general enough pwm driver, if there is any special >>> requirement, i want to execute a "demanding page" when the "page >>> fault" happened as this will make the whole flow more smooth. that >>> means we make it lazy by incremental patches. but if you do want to >>> use it for the moment, it is a "page fault" now. so we can have it >>> immediately and it is better you can help to double-test :-) >> >> My use case is to connect a Bluetooth module, in fact with a CSR chip. >> For testing, I will try to see what I can do, as for now my boards are not >> running with the mainline. But I'm very interested in transitioning on the >> mainline in due time, so that's the reason I keep an eye on the mailing >> lists. Conversely, it's not critical at all to get the feature in right now, but >> it should not be impossible to do so in the future, especially when taking >> the 'stable interface' aspect of device tree bindings into account. >> >>> 2014-02-27 18:51 GMT+08:00 Romain Izard <romain.izard.pro@gmail.com>: >>>> Perhaps this can be linked with the missing "clock-names" property, as this >>>> will make it possible to add multiple functional clocks, with "iclk" matching >>>> the interface clock (i.e. pwmc), and "fclk0", "fclk1", etc. matching >>>> all supported >>>> functional clocks. >>>> >>>> With only the 26 MHz clock, the node would look like: >>>> >>>> pwm: pwm@b0130000 { >>>> compatible = "sirf,prima2-pwm"; >>>> #pwm-cells = <2>; >>>> reg = <0xb0130000 0x10000>; >>>> clocks = <&clks 21>, <&clks 1>, <&clks 0>; >>>> clock-names = "iclk", "fclk0", "fclk3"; >>>> }; >>>> >> >> Aargh. I meant this: >> >> pwm: pwm@b0130000 { >> compatible = "sirf,prima2-pwm"; >> #pwm-cells = <2>; >> reg = <0xb0130000 0x10000>; >> clocks = <&clks 21>, <&clks 1>; >> clock-names = "iclk", "fclk0"; >> }; >> >>>> The result would look like this with both the 26 MHz and the 32 kHz clocks: >>>> >>>> pwm: pwm@b0130000 { >>>> compatible = "sirf,prima2-pwm"; >>>> #pwm-cells = <2>; >>>> reg = <0xb0130000 0x10000>; >>>> clocks = <&clks 21>, <&clks 1>, <&clks 0>; >>>> clock-names = "iclk", "fclk0", "fclk3"; >>>> }; >>>> >>>> And with all possible clocks: >>>> >>>> pwm: pwm@b0130000 { >>>> compatible = "sirf,prima2-pwm"; >>>> #pwm-cells = <2>; >>>> reg = <0xb0130000 0x10000>; >>>> clocks = <&clks 21>, <&clks 1>, <&clks 2>, <&clks 3>, <&clks >>>> 0>, <&clks 4>; >>>> clock-names = "iclk", "fclk0", "fclk1", "fclk2", "fclk3", "fclk4"; >>>> }; >>>> >>>> The code in the probe function would interpret the clock-name to build >>>> the clock/index mapping table. This would not change a lot the binding you >>>> proposed, as you still can describe only one functional clock, but it describes >>>> which index in the configuration register is linked to the proposed clock. >>>> >>> >>> yes. very clear. the only two things left >>> >>> 1. fclk should be named for pwm not for rtc, osc and pll, so the >>> names might be ugly by fclk0~N. >>> >> For me, it's important to keep the number in the name of the clock as it is >> the source of the information for binding the device tree clock on one side >> and the clock source index in the selection register on the other side. If >> we do not do it this way, we cannot easily handle new clocks in the future >> in the binding. >> >> On the other hand, I strongly dislike the current clock specification >> as <&clks #x>. >> It would be probably a good thing to add a header linking the numbers >> to symbols, >> and we would then get: >> >> clocks = <&clks SIRFCLK_A6_PWM>, >> <&clks SIRFCLK_A6_OSC>, >> <&clks SIRFCLK_A6_PLL1>, >> <&clks SIRFCLK_A6_PLL2>, >> <&clks SIRFCLK_A6_RTC>, >> <&clks SIRFCLK_16_PLL3>; >> clock-names = "iclk", "fclk0", "fclk1", "fclk2", "fclk3", "fclk4"; >> >> Note that as the binary output of the device tree would not change with this, we >> do not need to worry about backwards compatibility in this precise case. > > this pwm controller is prima2-compatible, so it is actually a pwm > controller highly bound with the special SoC. > really strange things are we actually care about the clock types. we > actually can't think fclk0~fclk4 as same things. for pll, i think it > is buggy to use. for rtc, it only service special target for providing > bypass 32KHz clock. > > if we look this controller a separate IP, we need to look 5 clock > source to be coequal. but it is not the real case. > i think people will hate the things that we have a separate bypass > mode for fclk3, why it is 3 but not 2, 1, 4 and 5? > > that is why i move to a MACRO 26MHz in the original codes as i think > it is a prima2 controller but not a controller used by prima2 even > though we always want a IP module to be a IP module suitable for all > SoCs. > let's have a draft to look whether this is making anything better: /* * SIRF serial SoC PWM device core driver * * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group company. * * Licensed under GPLv2. */ #include <linux/kernel.h> #include <linux/init.h> #include <linux/module.h> #include <linux/device.h> #include <linux/platform_device.h> #include <linux/clk.h> #include <linux/delay.h> #include <linux/pwm.h> #include <linux/of.h> #include <linux/io.h> #define SIRF_PWM_SELECT_PRECLK 0x0 #define SIRF_PWM_OE 0x4 #define SIRF_PWM_ENABLE_PRECLOCK 0x8 #define SIRF_PWM_ENABLE_POSTCLOCK 0xC #define SIRF_PWM_GET_WAIT_OFFSET(n) (0x10 + 0x8*n) #define SIRF_PWM_GET_HOLD_OFFSET(n) (0x14 + 0x8*n) #define SIRF_PWM_TR_STEP(n) (0x48 + 0x8*n) #define SIRF_PWM_STEP_HOLD(n) (0x4c + 0x8*n) #define SRC_FIELD_SIZE 3 #define BYPASS_MODE_BIT 21 #define TRANS_MODE_SELECT_BIT 7 struct sirf_pwm { struct pwm_chip chip; struct mutex mutex; void __iomem *base; struct clk *pwmc_clk; struct clk *sigsrc0_clk; struct clk *sigsrc3_clk; /* * PWM controller uses OSC(default 26MHz) or RTC(default 32768Hz) clock * to generate PWM signals instead of the clock of the controller */ unsigned long sigsrc0_clk_rate; unsigned long sigsrc3_clk_rate; }; static inline struct sirf_pwm *to_sirf_pwm_chip(struct pwm_chip *chip) { return container_of(chip, struct sirf_pwm, chip); } static u32 sirf_pwm_ns_to_cycles(struct pwm_chip *chip, u32 time_ns) { struct sirf_pwm *spwm = to_sirf_pwm_chip(chip); u64 dividend; u32 cycle; dividend = (u64)spwm->sigsrc0_clk_rate * time_ns + NSEC_PER_SEC / 2; do_div(dividend, NSEC_PER_SEC); cycle = dividend; return cycle > 1 ? cycle : 1; } static int sirf_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, int duty_ns, int period_ns) { u32 period_cycles, high_cycles, low_cycles; u32 val; struct sirf_pwm *spwm = to_sirf_pwm_chip(chip); if (unlikely(period_ns == NSEC_PER_SEC/spwm->sigsrc3_clk_rate)) { /* * sigsrc 3 is RTC with typical frequency 32KHz, * bypass RTC clock to WiFi/Bluetooth module */ mutex_lock(&spwm->mutex); val = readl(spwm->base + SIRF_PWM_SELECT_PRECLK); val |= 0x1 << (BYPASS_MODE_BIT + pwm->hwpwm); val &= ~(0x7 << (SRC_FIELD_SIZE * pwm->hwpwm)); val |= 3 << (SRC_FIELD_SIZE * pwm->hwpwm); writel(val, spwm->base + SIRF_PWM_SELECT_PRECLK); mutex_unlock(&spwm->mutex); } else { /* use OSC to generate PWM signals */ period_cycles = sirf_pwm_ns_to_cycles(chip, period_ns); if (period_cycles == 1) return -EINVAL; high_cycles = sirf_pwm_ns_to_cycles(chip, duty_ns); low_cycles = period_cycles - high_cycles; mutex_lock(&spwm->mutex); val = readl(spwm->base + SIRF_PWM_SELECT_PRECLK); val &= ~(0x1 << (BYPASS_MODE_BIT + pwm->hwpwm)); val &= ~(0x7 << (SRC_FIELD_SIZE * pwm->hwpwm)); writel(val, spwm->base + SIRF_PWM_SELECT_PRECLK); if (high_cycles == period_cycles) { high_cycles--; low_cycles = 1; } writel(high_cycles - 1, spwm->base + SIRF_PWM_GET_WAIT_OFFSET(pwm->hwpwm)); writel(low_cycles - 1, spwm->base + SIRF_PWM_GET_HOLD_OFFSET(pwm->hwpwm)); mutex_unlock(&spwm->mutex); } return 0; } static int sirf_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) { struct sirf_pwm *spwm = to_sirf_pwm_chip(chip); u32 val; mutex_lock(&spwm->mutex); /* disable preclock */ val = readl(spwm->base + SIRF_PWM_ENABLE_PRECLOCK); val &= ~(1 << pwm->hwpwm); writel(val, spwm->base + SIRF_PWM_ENABLE_PRECLOCK); /* select preclock source must after disable preclk*/ val = readl(spwm->base + SIRF_PWM_SELECT_PRECLK); val &= ~(0x7 << (SRC_FIELD_SIZE * pwm->hwpwm)); writel(val, spwm->base + SIRF_PWM_SELECT_PRECLK); /* wait for some time */ usleep_range(100, 100); /* enable preclock */ val = readl(spwm->base + SIRF_PWM_ENABLE_PRECLOCK); val |= (1 << pwm->hwpwm); writel(val, spwm->base + SIRF_PWM_ENABLE_PRECLOCK); /* enable post clock*/ val = readl(spwm->base + SIRF_PWM_ENABLE_POSTCLOCK); val |= (1 << pwm->hwpwm); writel(val, spwm->base + SIRF_PWM_ENABLE_POSTCLOCK); /* enable output */ val = readl(spwm->base + SIRF_PWM_OE); val |= 1 << pwm->hwpwm; val |= 1 << (pwm->hwpwm + TRANS_MODE_SELECT_BIT); writel(val, spwm->base + SIRF_PWM_OE); mutex_unlock(&spwm->mutex); return 0; } static void sirf_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) { u32 val; struct sirf_pwm *spwm = to_sirf_pwm_chip(chip); mutex_lock(&spwm->mutex); /* disable output */ val = readl(spwm->base + SIRF_PWM_OE); val &= ~(1 << pwm->hwpwm); writel(val, spwm->base + SIRF_PWM_OE); /* disable postclock */ val = readl(spwm->base + SIRF_PWM_ENABLE_POSTCLOCK); val &= ~(1 << pwm->hwpwm); writel(val, spwm->base + SIRF_PWM_ENABLE_POSTCLOCK); /* disable preclock */ val = readl(spwm->base + SIRF_PWM_ENABLE_PRECLOCK); val &= ~(1 << pwm->hwpwm); writel(val, spwm->base + SIRF_PWM_ENABLE_PRECLOCK); mutex_unlock(&spwm->mutex); } static const struct pwm_ops sirf_pwm_ops = { .enable = sirf_pwm_enable, .disable = sirf_pwm_disable, .config = sirf_pwm_config, .owner = THIS_MODULE, }; static int sirf_pwm_probe(struct platform_device *pdev) { struct sirf_pwm *spwm; struct resource *mem_res; int ret; spwm = devm_kzalloc(&pdev->dev, sizeof(*spwm), GFP_KERNEL); if (!spwm) return -ENOMEM; platform_set_drvdata(pdev, spwm); mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); spwm->base = devm_ioremap_resource(&pdev->dev, mem_res); if (!spwm->base) return -ENOMEM; /* * clock for PWM controller */ spwm->pwmc_clk = devm_clk_get(&pdev->dev, "pwmc"); if (IS_ERR(spwm->pwmc_clk)) { dev_err(&pdev->dev, "failed to get PWM controller clock\n"); return PTR_ERR(spwm->pwmc_clk); } ret = clk_prepare_enable(spwm->pwmc_clk); if (ret) return ret; /* * clocks to generate PWM signals */ spwm->sigsrc0_clk = devm_clk_get(&pdev->dev, "sigsrc0"); if (IS_ERR(spwm->sigsrc0_clk)) { dev_err(&pdev->dev, "failed to get PWM signal source clock0\n"); ret = PTR_ERR(spwm->sigsrc0_clk); goto err_src0_clk; } ret = clk_prepare_enable(spwm->sigsrc0_clk); if (ret) goto err_src0_clk; spwm->sigsrc0_clk_rate = clk_get_rate(spwm->sigsrc0_clk); spwm->sigsrc3_clk = devm_clk_get(&pdev->dev, "sigsrc3"); if (IS_ERR(spwm->sigsrc3_clk)) { dev_err(&pdev->dev, "failed to get PWM signal source clock3\n"); ret = PTR_ERR(spwm->sigsrc3_clk); goto err_src3_clk; } ret = clk_prepare_enable(spwm->sigsrc3_clk); if (ret) goto err_src3_clk; spwm->sigsrc3_clk_rate = clk_get_rate(spwm->sigsrc3_clk); spwm->chip.dev = &pdev->dev; spwm->chip.ops = &sirf_pwm_ops; spwm->chip.base = 0; spwm->chip.npwm = 7; mutex_init(&spwm->mutex); ret = pwmchip_add(&spwm->chip); if (ret < 0) { dev_err(&pdev->dev, "failed to register PWM\n"); goto err_pwmadd; } return 0; err_pwmadd: clk_disable_unprepare(spwm->sigsrc3_clk); err_src3_clk: clk_disable_unprepare(spwm->sigsrc0_clk); err_src0_clk: clk_disable_unprepare(spwm->pwmc_clk); return ret; } static int sirf_pwm_remove(struct platform_device *pdev) { struct sirf_pwm *spwm = platform_get_drvdata(pdev); clk_disable_unprepare(spwm->pwmc_clk); clk_disable_unprepare(spwm->sigsrc0_clk); clk_disable_unprepare(spwm->sigsrc3_clk); return pwmchip_remove(&spwm->chip); } #ifdef CONFIG_PM_SLEEP static int sirf_pwm_suspend(struct device *dev) { struct platform_device *pdev = to_platform_device(dev); struct sirf_pwm *spwm = platform_get_drvdata(pdev); clk_disable_unprepare(spwm->pwmc_clk); return 0; } static void sirf_pwm_config_restore(struct sirf_pwm *spwm) { struct pwm_device *pwm; int i; for (i = 0; i < spwm->chip.npwm; i++) { pwm = &spwm->chip.pwms[i]; /* * while restoring from hibernation, state of pwm is enabled, * but PWM hardware is not re-enabled */ if (test_bit(PWMF_REQUESTED, &pwm->flags) && test_bit(PWMF_ENABLED, &pwm->flags)) sirf_pwm_enable(&spwm->chip, pwm); } } static int sirf_pwm_resume(struct device *dev) { struct sirf_pwm *spwm = dev_get_drvdata(dev); clk_prepare_enable(spwm->pwmc_clk); sirf_pwm_config_restore(spwm); return 0; } static int sirf_pwm_restore(struct device *dev) { struct sirf_pwm *spwm = dev_get_drvdata(dev); /* back from hibernation, clock is already enabled */ sirf_pwm_config_restore(spwm); return 0; } #else #define sirf_pwm_resume NULL #define sirf_pwm_suspend NULL #define sirf_pwm_restore NULL #endif static const struct dev_pm_ops sirf_pwm_pm_ops = { .suspend = sirf_pwm_suspend, .resume = sirf_pwm_resume, .restore = sirf_pwm_restore, }; static const struct of_device_id sirf_pwm_of_match[] = { { .compatible = "sirf,prima2-pwm", }, {} }; MODULE_DEVICE_TABLE(of, sirf_pwm_of_match); static struct platform_driver sirf_pwm_driver = { .driver = { .name = "sirf-pwm", .pm = &sirf_pwm_pm_ops, .of_match_table = sirf_pwm_of_match, }, .probe = sirf_pwm_probe, .remove = sirf_pwm_remove, }; module_platform_driver(sirf_pwm_driver); MODULE_DESCRIPTION("SIRF serial SoC PWM device core driver"); MODULE_AUTHOR("RongJun Ying <Rongjun.Ying@csr.com>"); MODULE_AUTHOR("Huayi Li <huayi.li@csr.com>"); MODULE_LICENSE("GPL v2"); -- 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
2014-02-28 11:07 GMT+01:00 Barry Song <21cnbao@gmail.com>: > 2014-02-28 17:06 GMT+08:00 Romain Izard <romain.izard.pro@gmail.com>: >> 2014-02-28 4:01 GMT+01:00 Barry Song <21cnbao@gmail.com>: >>> >>> 1. fclk should be named for pwm not for rtc, osc and pll, so the >>> names might be ugly by fclk0~N. >>> >> For me, it's important to keep the number in the name of the clock as it is >> the source of the information for binding the device tree clock on one side >> and the clock source index in the selection register on the other side. If >> we do not do it this way, we cannot easily handle new clocks in the future >> in the binding. >> >> On the other hand, I strongly dislike the current clock specification >> as <&clks #x>. >> It would be probably a good thing to add a header linking the numbers >> to symbols, >> and we would then get: >> >> clocks = <&clks SIRFCLK_A6_PWM>, >> <&clks SIRFCLK_A6_OSC>, >> <&clks SIRFCLK_A6_PLL1>, >> <&clks SIRFCLK_A6_PLL2>, >> <&clks SIRFCLK_A6_RTC>, >> <&clks SIRFCLK_16_PLL3>; >> clock-names = "iclk", "fclk0", "fclk1", "fclk2", "fclk3", "fclk4"; >> >> Note that as the binary output of the device tree would not change with this, we >> do not need to worry about backwards compatibility in this precise case. > > this pwm controller is prima2-compatible, so it is actually a pwm > controller highly bound with the special SoC. > really strange things are we actually care about the clock types. we > actually can't think fclk0~fclk4 as same things. for pll, i think it > is buggy to use. for rtc, it only service special target for providing > bypass 32KHz clock. > If you're not using DVFS, it may be useful to use the PLLs. I do not think it's a good thing to say 'we will never use it' or 'we will not get it to work'. I now remember our use case for low-speed PWM: buzzers. here we will need to have a output range of typically 40-2000 Hz, which cannot be provided by the 26 MHz input source. > if we look this controller a separate IP, we need to look 5 clock > source to be coequal. but it is not the real case. > i think people will hate the things that we have a separate bypass > mode for fclk3, why it is 3 but not 2, 1, 4 and 5? I believe we can make the code to handle all clocks almost as equals, because for the IP it is the case. The DVFS case has an inpact on the PLLs, so it's not perfectly the case, but from the register point of view, the only difference is the value in the input selection register. If we want to output a 26 MHz clock, we need to enable bypass also on fclk0: otherwise we're limited by the minimum divisor of 2. But for me, the bypass should not be a 'visible' feature of the PWM for its users. The users specify a PWM line, and a period. We give them what they ask, or we return them an error if we cannot do that with the available input clocks. > that is why i move to a MACRO 26MHz in the original codes as i think > it is a prima2 controller but not a controller used by prima2 even > though we always want a IP module to be a IP module suitable for all > SoCs. The PWM controller is already used in prima2 and atlas6, it's almost the same as the controller in atlas4/5 which won't get supported in the kernel anytime soon. I expect the controller to be reused in future products: at the end, the sirfsoc family has multiple SoCs. And the xin clock is 12 MHz on atlas5 but 26 MHz on atlas6, so the clock specification through device tree is a pretty good idea. >>> 2. the driver need to manage all of the clocks. yes, it can. but it >>> might be ugly again. we can either request the clk at the time of pwm >>> configuration, or get all directy in probe and maintain an array. >> >> For me, what is important right now is to get the binding right. If the driver >> cannot do everything the binding allows, this is not a problem. But we define >> a binding that cannot easily be expanded, we will have problems when we >> need the new features. >> >>> i am thinking what is the best way for it. if we do that by an >>> incremental patch, it might be more concentrated and understandable. > > i think bypass mode for rtc will be included immediately since there > is a real user. > For reviewing, I believe it is better if the features are added one by one. This makes it somehow easier to understand each separate feature. But I recognize it can be tiring as each patch needs to be tested separately.
2014-02-28 19:41 GMT+08:00 Romain Izard <romain.izard.pro@gmail.com>: > 2014-02-28 11:07 GMT+01:00 Barry Song <21cnbao@gmail.com>: >> 2014-02-28 17:06 GMT+08:00 Romain Izard <romain.izard.pro@gmail.com>: >>> 2014-02-28 4:01 GMT+01:00 Barry Song <21cnbao@gmail.com>: >>>> >>>> 1. fclk should be named for pwm not for rtc, osc and pll, so the >>>> names might be ugly by fclk0~N. >>>> >>> For me, it's important to keep the number in the name of the clock as it is >>> the source of the information for binding the device tree clock on one side >>> and the clock source index in the selection register on the other side. If >>> we do not do it this way, we cannot easily handle new clocks in the future >>> in the binding. >>> >>> On the other hand, I strongly dislike the current clock specification >>> as <&clks #x>. >>> It would be probably a good thing to add a header linking the numbers >>> to symbols, >>> and we would then get: >>> >>> clocks = <&clks SIRFCLK_A6_PWM>, >>> <&clks SIRFCLK_A6_OSC>, >>> <&clks SIRFCLK_A6_PLL1>, >>> <&clks SIRFCLK_A6_PLL2>, >>> <&clks SIRFCLK_A6_RTC>, >>> <&clks SIRFCLK_16_PLL3>; >>> clock-names = "iclk", "fclk0", "fclk1", "fclk2", "fclk3", "fclk4"; >>> >>> Note that as the binary output of the device tree would not change with this, we >>> do not need to worry about backwards compatibility in this precise case. >> >> this pwm controller is prima2-compatible, so it is actually a pwm >> controller highly bound with the special SoC. >> really strange things are we actually care about the clock types. we >> actually can't think fclk0~fclk4 as same things. for pll, i think it >> is buggy to use. for rtc, it only service special target for providing >> bypass 32KHz clock. >> > If you're not using DVFS, it may be useful to use the PLLs. I do not think > it's a good thing to say 'we will never use it' or 'we will not get it to work'. > > I now remember our use case for low-speed PWM: buzzers. here we will > need to have a output range of typically 40-2000 Hz, which cannot be provided > by the 26 MHz input source. > >> if we look this controller a separate IP, we need to look 5 clock >> source to be coequal. but it is not the real case. >> i think people will hate the things that we have a separate bypass >> mode for fclk3, why it is 3 but not 2, 1, 4 and 5? > > I believe we can make the code to handle all clocks almost as equals, > because for the IP it is the case. The DVFS case has an inpact on the > PLLs, so it's not perfectly the case, but from the register point of view, > the only difference is the value in the input selection register. i don't think it is worthy if we make everything equal even though we can. but it is just making things worse. if there is something we can do for special cases, that is adding a check-up table for them. there is a module like dev_pm_domain which can coordinate the dependency of device power domain, if we can have similar things to coordinate the change of clock frequency, we can update PWM divider after PLL is updated. but it will break the "best-fit" as you might select pll1 as best-fit at beginning, then after pll1 changes, pll2 becomes "best-fit". > > If we want to output a 26 MHz clock, we need to enable bypass also > on fclk0: otherwise we're limited by the minimum divisor of 2. But for > me, the bypass should not be a 'visible' feature of the PWM for its users. > The users specify a PWM line, and a period. We give them what they > ask, or we return them an error if we cannot do that with the available > input clocks. i don't think bypass is something related to typical PWM. so we need some comments to make things really acceptable. when you say a period, you are just ignoring duty. note bypass is something ignoring duty. and make the codes be a little look be out of place . > >> that is why i move to a MACRO 26MHz in the original codes as i think >> it is a prima2 controller but not a controller used by prima2 even >> though we always want a IP module to be a IP module suitable for all >> SoCs. > > The PWM controller is already used in prima2 and atlas6, it's almost the > same as the controller in atlas4/5 which won't get supported in the kernel > anytime soon. I expect the controller to be reused in future products: at > the end, the sirfsoc family has multiple SoCs. And the xin clock is 12 > MHz on atlas5 but 26 MHz on atlas6, so the clock specification through > device tree is a pretty good idea. when atlas6 uses the same PWM, it is still prima2-compatible. there is no difference for source clock layout. when i said prima2-compatible, it is not only prima2, as atlas6 is also prima2-compatible for pwm. i do agree putting fixed clock in dts is a good way. a4/a5 is not supported in the new kernel version any more, the coming soc will have completely different clock layout. so that means the current clock tree is just for prima2 and atlas6. if you do have some idea about clock, it is highly welcomed that you send the codes in clk and i will ack. > >>>> 2. the driver need to manage all of the clocks. yes, it can. but it >>>> might be ugly again. we can either request the clk at the time of pwm >>>> configuration, or get all directy in probe and maintain an array. >>> >>> For me, what is important right now is to get the binding right. If the driver >>> cannot do everything the binding allows, this is not a problem. But we define >>> a binding that cannot easily be expanded, we will have problems when we >>> need the new features. >>> >>>> i am thinking what is the best way for it. if we do that by an >>>> incremental patch, it might be more concentrated and understandable. >> >> i think bypass mode for rtc will be included immediately since there >> is a real user. >> > For reviewing, I believe it is better if the features are added one by one. > This makes it somehow easier to understand each separate feature. > But I recognize it can be tiring as each patch needs to be tested separately. that is my original idea, doing things by incremental patch. and you asked the feature of 32KHz, and i think 32KHz is not a difficult case to handle. doing 32Khz here can show the possible clock sources and make things smooth if we want to some other similar items. as people have seen several source clocks before the beginning. > > -- > Romain Izard -barry -- 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
2014-02-28 11:33 GMT+01:00 Barry Song <21cnbao@gmail.com>: > > let's have a draft to look whether this is making anything better: > > /* > * SIRF serial SoC PWM device core driver > * > * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group company. > * > * Licensed under GPLv2. > */ > #include <linux/kernel.h> > #include <linux/init.h> > #include <linux/module.h> > #include <linux/device.h> > #include <linux/platform_device.h> > #include <linux/clk.h> > #include <linux/delay.h> > #include <linux/pwm.h> > #include <linux/of.h> > #include <linux/io.h> > > #define SIRF_PWM_SELECT_PRECLK 0x0 > #define SIRF_PWM_OE 0x4 > #define SIRF_PWM_ENABLE_PRECLOCK 0x8 > #define SIRF_PWM_ENABLE_POSTCLOCK 0xC > #define SIRF_PWM_GET_WAIT_OFFSET(n) (0x10 + 0x8*n) > #define SIRF_PWM_GET_HOLD_OFFSET(n) (0x14 + 0x8*n) > > #define SIRF_PWM_TR_STEP(n) (0x48 + 0x8*n) > #define SIRF_PWM_STEP_HOLD(n) (0x4c + 0x8*n) > Missing spaces around * > #define SRC_FIELD_SIZE 3 > #define BYPASS_MODE_BIT 21 > #define TRANS_MODE_SELECT_BIT 7 > > struct sirf_pwm { > struct pwm_chip chip; > struct mutex mutex; > void __iomem *base; > struct clk *pwmc_clk; > struct clk *sigsrc0_clk; > struct clk *sigsrc3_clk; > /* > * PWM controller uses OSC(default 26MHz) or RTC(default 32768Hz) clock > * to generate PWM signals instead of the clock of the controller > */ > unsigned long sigsrc0_clk_rate; > unsigned long sigsrc3_clk_rate; > }; > > static inline struct sirf_pwm *to_sirf_pwm_chip(struct pwm_chip *chip) > { > return container_of(chip, struct sirf_pwm, chip); > } > > static u32 sirf_pwm_ns_to_cycles(struct pwm_chip *chip, u32 time_ns) > { > struct sirf_pwm *spwm = to_sirf_pwm_chip(chip); > u64 dividend; > u32 cycle; > > dividend = (u64)spwm->sigsrc0_clk_rate * time_ns + NSEC_PER_SEC / 2; > do_div(dividend, NSEC_PER_SEC); > > cycle = dividend; > > return cycle > 1 ? cycle : 1; > } > > static int sirf_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > int duty_ns, int period_ns) > { > u32 period_cycles, high_cycles, low_cycles; > u32 val; > struct sirf_pwm *spwm = to_sirf_pwm_chip(chip); > if (unlikely(period_ns == NSEC_PER_SEC/spwm->sigsrc3_clk_rate)) { > /* > * sigsrc 3 is RTC with typical frequency 32KHz, > * bypass RTC clock to WiFi/Bluetooth module > */ This means that only a single, precise period is recognized as triggering this clock rate, and as the clock is precisely specified in Hz, we will not have a very definite value: the period will be 30517,578 ns, which gives a rounded value of 30518, but a truncated value of 30517. Here, it is the truncated value that needs to be used. > mutex_lock(&spwm->mutex); > > val = readl(spwm->base + SIRF_PWM_SELECT_PRECLK); > val |= 0x1 << (BYPASS_MODE_BIT + pwm->hwpwm); > val &= ~(0x7 << (SRC_FIELD_SIZE * pwm->hwpwm)); > val |= 3 << (SRC_FIELD_SIZE * pwm->hwpwm); > writel(val, spwm->base + SIRF_PWM_SELECT_PRECLK); > > mutex_unlock(&spwm->mutex); > } else { > /* use OSC to generate PWM signals */ > period_cycles = sirf_pwm_ns_to_cycles(chip, period_ns); > if (period_cycles == 1) > return -EINVAL; > But if we allow the bypass mode, we could continue here if the error rate is good. The conversion function does not give this information, so we can't continue. > high_cycles = sirf_pwm_ns_to_cycles(chip, duty_ns); > low_cycles = period_cycles - high_cycles; > > mutex_lock(&spwm->mutex); > > val = readl(spwm->base + SIRF_PWM_SELECT_PRECLK); > val &= ~(0x1 << (BYPASS_MODE_BIT + pwm->hwpwm)); > val &= ~(0x7 << (SRC_FIELD_SIZE * pwm->hwpwm)); > writel(val, spwm->base + SIRF_PWM_SELECT_PRECLK); > > if (high_cycles == period_cycles) { > high_cycles--; > low_cycles = 1; > } > > writel(high_cycles - 1, > spwm->base + SIRF_PWM_GET_WAIT_OFFSET(pwm->hwpwm)); > writel(low_cycles - 1, > spwm->base + SIRF_PWM_GET_HOLD_OFFSET(pwm->hwpwm)); > > mutex_unlock(&spwm->mutex); > } > > return 0; > } > > static int sirf_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > { > struct sirf_pwm *spwm = to_sirf_pwm_chip(chip); > u32 val; > > mutex_lock(&spwm->mutex); > > /* disable preclock */ > val = readl(spwm->base + SIRF_PWM_ENABLE_PRECLOCK); > val &= ~(1 << pwm->hwpwm); > writel(val, spwm->base + SIRF_PWM_ENABLE_PRECLOCK); > > /* select preclock source must after disable preclk*/ /* The preclock source must be configured only when disabled */ > val = readl(spwm->base + SIRF_PWM_SELECT_PRECLK); > val &= ~(0x7 << (SRC_FIELD_SIZE * pwm->hwpwm)); > writel(val, spwm->base + SIRF_PWM_SELECT_PRECLK); Here, we clear the value set in sirf_pwm_config if it is not 0. We are losing the information set in sirf_pwm_config!!! Do we even need to touch it here, or should this code be in the configuration function? > /* wait for some time */ > usleep_range(100, 100); > Checkpatch does not like this. > /* enable preclock */ > val = readl(spwm->base + SIRF_PWM_ENABLE_PRECLOCK); > val |= (1 << pwm->hwpwm); > writel(val, spwm->base + SIRF_PWM_ENABLE_PRECLOCK); > > /* enable post clock*/ > val = readl(spwm->base + SIRF_PWM_ENABLE_POSTCLOCK); > val |= (1 << pwm->hwpwm); > writel(val, spwm->base + SIRF_PWM_ENABLE_POSTCLOCK); > > /* enable output */ > val = readl(spwm->base + SIRF_PWM_OE); > val |= 1 << pwm->hwpwm; > val |= 1 << (pwm->hwpwm + TRANS_MODE_SELECT_BIT); > > writel(val, spwm->base + SIRF_PWM_OE); > > mutex_unlock(&spwm->mutex); > > return 0; > } > > static void sirf_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) > { > u32 val; > struct sirf_pwm *spwm = to_sirf_pwm_chip(chip); > > mutex_lock(&spwm->mutex); > > /* disable output */ > val = readl(spwm->base + SIRF_PWM_OE); > val &= ~(1 << pwm->hwpwm); > writel(val, spwm->base + SIRF_PWM_OE); > > /* disable postclock */ > val = readl(spwm->base + SIRF_PWM_ENABLE_POSTCLOCK); > val &= ~(1 << pwm->hwpwm); > writel(val, spwm->base + SIRF_PWM_ENABLE_POSTCLOCK); > > /* disable preclock */ > val = readl(spwm->base + SIRF_PWM_ENABLE_PRECLOCK); > val &= ~(1 << pwm->hwpwm); > writel(val, spwm->base + SIRF_PWM_ENABLE_PRECLOCK); > > mutex_unlock(&spwm->mutex); > } > > static const struct pwm_ops sirf_pwm_ops = { > .enable = sirf_pwm_enable, > .disable = sirf_pwm_disable, > .config = sirf_pwm_config, > .owner = THIS_MODULE, > }; > > static int sirf_pwm_probe(struct platform_device *pdev) > { > struct sirf_pwm *spwm; > struct resource *mem_res; > int ret; > > spwm = devm_kzalloc(&pdev->dev, sizeof(*spwm), > GFP_KERNEL); > if (!spwm) > return -ENOMEM; > > platform_set_drvdata(pdev, spwm); > > mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > spwm->base = devm_ioremap_resource(&pdev->dev, mem_res); > if (!spwm->base) > return -ENOMEM; > > /* > * clock for PWM controller > */ > spwm->pwmc_clk = devm_clk_get(&pdev->dev, "pwmc"); > if (IS_ERR(spwm->pwmc_clk)) { > dev_err(&pdev->dev, "failed to get PWM controller clock\n"); > return PTR_ERR(spwm->pwmc_clk); > } > > ret = clk_prepare_enable(spwm->pwmc_clk); > if (ret) > return ret; > > /* > * clocks to generate PWM signals > */ > spwm->sigsrc0_clk = devm_clk_get(&pdev->dev, "sigsrc0"); > if (IS_ERR(spwm->sigsrc0_clk)) { > dev_err(&pdev->dev, "failed to get PWM signal source clock0\n"); > ret = PTR_ERR(spwm->sigsrc0_clk); > goto err_src0_clk; > } > > ret = clk_prepare_enable(spwm->sigsrc0_clk); > if (ret) > goto err_src0_clk; > > spwm->sigsrc0_clk_rate = clk_get_rate(spwm->sigsrc0_clk); > > spwm->sigsrc3_clk = devm_clk_get(&pdev->dev, "sigsrc3"); > if (IS_ERR(spwm->sigsrc3_clk)) { > dev_err(&pdev->dev, "failed to get PWM signal source clock3\n"); > ret = PTR_ERR(spwm->sigsrc3_clk); > goto err_src3_clk; > } > > ret = clk_prepare_enable(spwm->sigsrc3_clk); > if (ret) > goto err_src3_clk; > > spwm->sigsrc3_clk_rate = clk_get_rate(spwm->sigsrc3_clk); > So in the end, you require both functional clocks 0 & 3 to be specified in the device tree. And if any of them is not specified, you refuse to load the driver. > spwm->chip.dev = &pdev->dev; > spwm->chip.ops = &sirf_pwm_ops; > spwm->chip.base = 0; > spwm->chip.npwm = 7; > > mutex_init(&spwm->mutex); > > ret = pwmchip_add(&spwm->chip); > if (ret < 0) { > dev_err(&pdev->dev, "failed to register PWM\n"); > goto err_pwmadd; > } > > return 0; > > err_pwmadd: > clk_disable_unprepare(spwm->sigsrc3_clk); > err_src3_clk: > clk_disable_unprepare(spwm->sigsrc0_clk); > err_src0_clk: > clk_disable_unprepare(spwm->pwmc_clk); > > return ret; > } > > static int sirf_pwm_remove(struct platform_device *pdev) > { > struct sirf_pwm *spwm = platform_get_drvdata(pdev); > > clk_disable_unprepare(spwm->pwmc_clk); > clk_disable_unprepare(spwm->sigsrc0_clk); > clk_disable_unprepare(spwm->sigsrc3_clk); > > return pwmchip_remove(&spwm->chip); > } > > #ifdef CONFIG_PM_SLEEP > static int sirf_pwm_suspend(struct device *dev) > { > struct platform_device *pdev = to_platform_device(dev); > struct sirf_pwm *spwm = platform_get_drvdata(pdev); > > clk_disable_unprepare(spwm->pwmc_clk); > > return 0; > } > > static void sirf_pwm_config_restore(struct sirf_pwm *spwm) > { > struct pwm_device *pwm; > int i; > > for (i = 0; i < spwm->chip.npwm; i++) { > pwm = &spwm->chip.pwms[i]; > /* > * while restoring from hibernation, state of pwm is enabled, > * but PWM hardware is not re-enabled > */ > if (test_bit(PWMF_REQUESTED, &pwm->flags) && > test_bit(PWMF_ENABLED, &pwm->flags)) > sirf_pwm_enable(&spwm->chip, pwm); > } > } > > static int sirf_pwm_resume(struct device *dev) > { > struct sirf_pwm *spwm = dev_get_drvdata(dev); > > clk_prepare_enable(spwm->pwmc_clk); > > sirf_pwm_config_restore(spwm); > > return 0; > } > > static int sirf_pwm_restore(struct device *dev) > { > struct sirf_pwm *spwm = dev_get_drvdata(dev); > > /* back from hibernation, clock is already enabled */ > sirf_pwm_config_restore(spwm); > > return 0; > } > > #else > #define sirf_pwm_resume NULL > #define sirf_pwm_suspend NULL > #define sirf_pwm_restore NULL > #endif > > static const struct dev_pm_ops sirf_pwm_pm_ops = { > .suspend = sirf_pwm_suspend, > .resume = sirf_pwm_resume, > .restore = sirf_pwm_restore, > }; > > static const struct of_device_id sirf_pwm_of_match[] = { > { .compatible = "sirf,prima2-pwm", }, > {} > }; > MODULE_DEVICE_TABLE(of, sirf_pwm_of_match); > > static struct platform_driver sirf_pwm_driver = { > .driver = { > .name = "sirf-pwm", > .pm = &sirf_pwm_pm_ops, > .of_match_table = sirf_pwm_of_match, > }, > .probe = sirf_pwm_probe, > .remove = sirf_pwm_remove, > }; > > module_platform_driver(sirf_pwm_driver); > > MODULE_DESCRIPTION("SIRF serial SoC PWM device core driver"); > MODULE_AUTHOR("RongJun Ying <Rongjun.Ying@csr.com>"); > MODULE_AUTHOR("Huayi Li <huayi.li@csr.com>"); > MODULE_LICENSE("GPL v2"); -- 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
2014-02-28 21:36 GMT+08:00 Romain Izard <romain.izard.pro@gmail.com>: > 2014-02-28 11:33 GMT+01:00 Barry Song <21cnbao@gmail.com>: >> >> let's have a draft to look whether this is making anything better: >> >> /* >> * SIRF serial SoC PWM device core driver >> * >> * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group company. >> * >> * Licensed under GPLv2. >> */ >> #include <linux/kernel.h> >> #include <linux/init.h> >> #include <linux/module.h> >> #include <linux/device.h> >> #include <linux/platform_device.h> >> #include <linux/clk.h> >> #include <linux/delay.h> >> #include <linux/pwm.h> >> #include <linux/of.h> >> #include <linux/io.h> >> >> #define SIRF_PWM_SELECT_PRECLK 0x0 >> #define SIRF_PWM_OE 0x4 >> #define SIRF_PWM_ENABLE_PRECLOCK 0x8 >> #define SIRF_PWM_ENABLE_POSTCLOCK 0xC >> #define SIRF_PWM_GET_WAIT_OFFSET(n) (0x10 + 0x8*n) >> #define SIRF_PWM_GET_HOLD_OFFSET(n) (0x14 + 0x8*n) >> >> #define SIRF_PWM_TR_STEP(n) (0x48 + 0x8*n) >> #define SIRF_PWM_STEP_HOLD(n) (0x4c + 0x8*n) >> > Missing spaces around * > > >> #define SRC_FIELD_SIZE 3 >> #define BYPASS_MODE_BIT 21 >> #define TRANS_MODE_SELECT_BIT 7 >> >> struct sirf_pwm { >> struct pwm_chip chip; >> struct mutex mutex; >> void __iomem *base; >> struct clk *pwmc_clk; >> struct clk *sigsrc0_clk; >> struct clk *sigsrc3_clk; >> /* >> * PWM controller uses OSC(default 26MHz) or RTC(default 32768Hz) clock >> * to generate PWM signals instead of the clock of the controller >> */ >> unsigned long sigsrc0_clk_rate; >> unsigned long sigsrc3_clk_rate; >> }; >> >> static inline struct sirf_pwm *to_sirf_pwm_chip(struct pwm_chip *chip) >> { >> return container_of(chip, struct sirf_pwm, chip); >> } >> >> static u32 sirf_pwm_ns_to_cycles(struct pwm_chip *chip, u32 time_ns) >> { >> struct sirf_pwm *spwm = to_sirf_pwm_chip(chip); >> u64 dividend; >> u32 cycle; >> >> dividend = (u64)spwm->sigsrc0_clk_rate * time_ns + NSEC_PER_SEC / 2; >> do_div(dividend, NSEC_PER_SEC); >> >> cycle = dividend; >> >> return cycle > 1 ? cycle : 1; >> } >> >> static int sirf_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, >> int duty_ns, int period_ns) >> { >> u32 period_cycles, high_cycles, low_cycles; >> u32 val; >> struct sirf_pwm *spwm = to_sirf_pwm_chip(chip); >> if (unlikely(period_ns == NSEC_PER_SEC/spwm->sigsrc3_clk_rate)) { >> /* >> * sigsrc 3 is RTC with typical frequency 32KHz, >> * bypass RTC clock to WiFi/Bluetooth module >> */ > > This means that only a single, precise period is recognized as triggering > this clock rate, and as the clock is precisely specified in Hz, we will not > have a very definite value: the period will be 30517,578 ns, which gives a > rounded value of 30518, but a truncated value of 30517. Here, it is the > truncated value that needs to be used. well. but i am thinking the client of PWM wants to call the function by a predefined MACRO "NSEC_PER_SEC / freq" if it thinks it is calling PWM. that makes non-accurate value even more readable. > >> mutex_lock(&spwm->mutex); >> >> val = readl(spwm->base + SIRF_PWM_SELECT_PRECLK); >> val |= 0x1 << (BYPASS_MODE_BIT + pwm->hwpwm); >> val &= ~(0x7 << (SRC_FIELD_SIZE * pwm->hwpwm)); >> val |= 3 << (SRC_FIELD_SIZE * pwm->hwpwm); >> writel(val, spwm->base + SIRF_PWM_SELECT_PRECLK); >> >> mutex_unlock(&spwm->mutex); >> } else { >> /* use OSC to generate PWM signals */ >> period_cycles = sirf_pwm_ns_to_cycles(chip, period_ns); >> if (period_cycles == 1) >> return -EINVAL; >> > But if we allow the bypass mode, we could continue here if the error > rate is good. The conversion function does not give this information, > so we can't continue. personally, i am ok. if PWM guys don't object, i support. if I am a PWM guy, i will not like it too. and if you see the original V3, and comments from PWM guys, you will find that. when we think again and carefully, don't you think "bypass" is something belong to clock subsystem but not PWM subsystem? if we do care about frequency without duty, it is something in clock subsystem not PWM subsystem. i think what we can make everything clear is making the channel clocks become clock nodes of the whole clock tree, the node can be requested by a non-PWM device and PWM-channel through clk_get. if someone wants a clock, get it from clock subsystem, and the PWM channel here can request the clock as well. but i don't have plan to do that for the moment. > >> high_cycles = sirf_pwm_ns_to_cycles(chip, duty_ns); >> low_cycles = period_cycles - high_cycles; >> >> mutex_lock(&spwm->mutex); >> >> val = readl(spwm->base + SIRF_PWM_SELECT_PRECLK); >> val &= ~(0x1 << (BYPASS_MODE_BIT + pwm->hwpwm)); >> val &= ~(0x7 << (SRC_FIELD_SIZE * pwm->hwpwm)); >> writel(val, spwm->base + SIRF_PWM_SELECT_PRECLK); >> >> if (high_cycles == period_cycles) { >> high_cycles--; >> low_cycles = 1; >> } >> >> writel(high_cycles - 1, >> spwm->base + SIRF_PWM_GET_WAIT_OFFSET(pwm->hwpwm)); >> writel(low_cycles - 1, >> spwm->base + SIRF_PWM_GET_HOLD_OFFSET(pwm->hwpwm)); >> >> mutex_unlock(&spwm->mutex); >> } >> >> return 0; >> } >> >> static int sirf_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) >> { >> struct sirf_pwm *spwm = to_sirf_pwm_chip(chip); >> u32 val; >> >> mutex_lock(&spwm->mutex); >> >> /* disable preclock */ >> val = readl(spwm->base + SIRF_PWM_ENABLE_PRECLOCK); >> val &= ~(1 << pwm->hwpwm); >> writel(val, spwm->base + SIRF_PWM_ENABLE_PRECLOCK); >> >> /* select preclock source must after disable preclk*/ > > /* The preclock source must be configured only when disabled */ > >> val = readl(spwm->base + SIRF_PWM_SELECT_PRECLK); >> val &= ~(0x7 << (SRC_FIELD_SIZE * pwm->hwpwm)); >> writel(val, spwm->base + SIRF_PWM_SELECT_PRECLK); > > Here, we clear the value set in sirf_pwm_config if it is not 0. We are > losing the information set in sirf_pwm_config!!! Do we even need to > touch it here, or should this code be in the configuration function? yes, you are right. sorry for misoperating the register. it is just a quick draft to show the result of our discussion as i think we need the source codes to continue. > >> /* wait for some time */ >> usleep_range(100, 100); >> > Checkpatch does not like this. it does dislike. checkpatch dislikes min=max. > >> /* enable preclock */ >> val = readl(spwm->base + SIRF_PWM_ENABLE_PRECLOCK); >> val |= (1 << pwm->hwpwm); >> writel(val, spwm->base + SIRF_PWM_ENABLE_PRECLOCK); >> >> /* enable post clock*/ >> val = readl(spwm->base + SIRF_PWM_ENABLE_POSTCLOCK); >> val |= (1 << pwm->hwpwm); >> writel(val, spwm->base + SIRF_PWM_ENABLE_POSTCLOCK); >> >> /* enable output */ >> val = readl(spwm->base + SIRF_PWM_OE); >> val |= 1 << pwm->hwpwm; >> val |= 1 << (pwm->hwpwm + TRANS_MODE_SELECT_BIT); >> >> writel(val, spwm->base + SIRF_PWM_OE); >> >> mutex_unlock(&spwm->mutex); >> >> return 0; >> } >> >> static void sirf_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) >> { >> u32 val; >> struct sirf_pwm *spwm = to_sirf_pwm_chip(chip); >> >> mutex_lock(&spwm->mutex); >> >> /* disable output */ >> val = readl(spwm->base + SIRF_PWM_OE); >> val &= ~(1 << pwm->hwpwm); >> writel(val, spwm->base + SIRF_PWM_OE); >> >> /* disable postclock */ >> val = readl(spwm->base + SIRF_PWM_ENABLE_POSTCLOCK); >> val &= ~(1 << pwm->hwpwm); >> writel(val, spwm->base + SIRF_PWM_ENABLE_POSTCLOCK); >> >> /* disable preclock */ >> val = readl(spwm->base + SIRF_PWM_ENABLE_PRECLOCK); >> val &= ~(1 << pwm->hwpwm); >> writel(val, spwm->base + SIRF_PWM_ENABLE_PRECLOCK); >> >> mutex_unlock(&spwm->mutex); >> } >> >> static const struct pwm_ops sirf_pwm_ops = { >> .enable = sirf_pwm_enable, >> .disable = sirf_pwm_disable, >> .config = sirf_pwm_config, >> .owner = THIS_MODULE, >> }; >> >> static int sirf_pwm_probe(struct platform_device *pdev) >> { >> struct sirf_pwm *spwm; >> struct resource *mem_res; >> int ret; >> >> spwm = devm_kzalloc(&pdev->dev, sizeof(*spwm), >> GFP_KERNEL); >> if (!spwm) >> return -ENOMEM; >> >> platform_set_drvdata(pdev, spwm); >> >> mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> spwm->base = devm_ioremap_resource(&pdev->dev, mem_res); >> if (!spwm->base) >> return -ENOMEM; >> >> /* >> * clock for PWM controller >> */ >> spwm->pwmc_clk = devm_clk_get(&pdev->dev, "pwmc"); >> if (IS_ERR(spwm->pwmc_clk)) { >> dev_err(&pdev->dev, "failed to get PWM controller clock\n"); >> return PTR_ERR(spwm->pwmc_clk); >> } >> >> ret = clk_prepare_enable(spwm->pwmc_clk); >> if (ret) >> return ret; >> >> /* >> * clocks to generate PWM signals >> */ >> spwm->sigsrc0_clk = devm_clk_get(&pdev->dev, "sigsrc0"); >> if (IS_ERR(spwm->sigsrc0_clk)) { >> dev_err(&pdev->dev, "failed to get PWM signal source clock0\n"); >> ret = PTR_ERR(spwm->sigsrc0_clk); >> goto err_src0_clk; >> } >> >> ret = clk_prepare_enable(spwm->sigsrc0_clk); >> if (ret) >> goto err_src0_clk; >> >> spwm->sigsrc0_clk_rate = clk_get_rate(spwm->sigsrc0_clk); >> >> spwm->sigsrc3_clk = devm_clk_get(&pdev->dev, "sigsrc3"); >> if (IS_ERR(spwm->sigsrc3_clk)) { >> dev_err(&pdev->dev, "failed to get PWM signal source clock3\n"); >> ret = PTR_ERR(spwm->sigsrc3_clk); >> goto err_src3_clk; >> } >> >> ret = clk_prepare_enable(spwm->sigsrc3_clk); >> if (ret) >> goto err_src3_clk; >> >> spwm->sigsrc3_clk_rate = clk_get_rate(spwm->sigsrc3_clk); >> > So in the end, you require both functional clocks 0 & 3 to be specified > in the device tree. And if any of them is not specified, you refuse to > load the driver. yes. i prefer both the bypass mode for 32KHz and PWM mode from 26MHz as 32KHz should be built-in even though i think bypass mode is not a thing of PWM. but it is the quickest way for the moment to continue and not break other things. -barry -- 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
> > i think what we can make everything clear is making the channel clocks > become clock nodes of the whole clock tree, the node can be requested > by a non-PWM device and PWM-channel through clk_get. if someone wants > a clock, get it from clock subsystem, and the PWM channel here can > request the clock as well. but i don't have plan to do that for the > moment. > i am not sure whether i have described everything clear enough. the point is if the bluetooth want a clock with 32KHz, it should want it by clk_get(), but not get it from pwm_config. it is just a workaround. the pwm is the concept of prima2 hardware, lcd can think it is asking pwm, why is bluetooth thinking it is requesting a PWM? in the view of bluetooth, the source is a clock not a PWM thing for backlight. -barry -- 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/Documentation/devicetree/bindings/pwm/pwm-sirf.txt b/Documentation/devicetree/bindings/pwm/pwm-sirf.txt new file mode 100644 index 0000000..4b10109 --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/pwm-sirf.txt @@ -0,0 +1,17 @@ +SiRF prima2 & atlas6 PWM drivers + +Required properties: +- compatible: "sirf,prima2-pwm" +- reg: physical base address and length of the controller's registers +- #pwm-cells: should be 2. The first cell specifies the per-chip index + of the PWM to use and the second cell is the period in nanoseconds. +- clocks: from common clock binding: the 1st clock is for PWM controller + the 2nd clock is the source to generate PWM waves + +Example: +pwm: pwm@b0130000 { + compatible = "sirf,prima2-pwm"; + #pwm-cells = <2>; + reg = <0xb0130000 0x10000>; + clocks = <&clks 21>, <&clks 1>; +}; diff --git a/arch/arm/boot/dts/atlas6.dtsi b/arch/arm/boot/dts/atlas6.dtsi index f8674bc..5a09815 100644 --- a/arch/arm/boot/dts/atlas6.dtsi +++ b/arch/arm/boot/dts/atlas6.dtsi @@ -614,8 +614,9 @@ pwm@b0130000 { compatible = "sirf,prima2-pwm"; + #pwm-cells = <2>; reg = <0xb0130000 0x10000>; - clocks = <&clks 21>; + clocks = <&clks 21>, <&clks 1>; }; efusesys@b0140000 { diff --git a/arch/arm/boot/dts/prima2.dtsi b/arch/arm/boot/dts/prima2.dtsi index 0e21993..3439e48 100644 --- a/arch/arm/boot/dts/prima2.dtsi +++ b/arch/arm/boot/dts/prima2.dtsi @@ -642,8 +642,9 @@ pwm@b0130000 { compatible = "sirf,prima2-pwm"; + #pwm-cells = <2>; reg = <0xb0130000 0x10000>; - clocks = <&clks 21>; + clocks = <&clks 21>, <&clks 1>; }; efusesys@b0140000 { diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 22f2f28..f2165eb 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -176,6 +176,15 @@ config PWM_SAMSUNG To compile this driver as a module, choose M here: the module will be called pwm-samsung. +config PWM_SIRF + tristate "SiRF PWM support" + depends on (ARCH_SIRF || COMPILE_TEST) && COMMON_CLK && OF + help + Generic PWM framework driver for SiRF SoC. + + To compile this driver as a module, choose M here: the module + will be called pwm-sirf. + config PWM_SPEAR tristate "STMicroelectronics SPEAr PWM support" depends on PLAT_SPEAR diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index d8906ec..aa222eb 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -15,6 +15,7 @@ obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o obj-$(CONFIG_PWM_PXA) += pwm-pxa.o obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o +obj-$(CONFIG_PWM_SIRF) += pwm-sirf.o obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o diff --git a/drivers/pwm/pwm-sirf.c b/drivers/pwm/pwm-sirf.c new file mode 100644 index 0000000..b717de0 --- /dev/null +++ b/drivers/pwm/pwm-sirf.c @@ -0,0 +1,308 @@ +/* + * SIRF serial SoC PWM device core driver + * + * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group company. + * + * Licensed under GPLv2 or later. + */ +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/module.h> +#include <linux/device.h> +#include <linux/platform_device.h> +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/pwm.h> +#include <linux/of.h> +#include <linux/io.h> + +#define SIRF_PWM_SELECT_PRECLK 0x0 +#define SIRF_PWM_OE 0x4 +#define SIRF_PWM_ENABLE_PRECLOCK 0x8 +#define SIRF_PWM_ENABLE_POSTCLOCK 0xC +#define SIRF_PWM_GET_WAIT_OFFSET(n) (0x10 + 0x8*n) +#define SIRF_PWM_GET_HOLD_OFFSET(n) (0x14 + 0x8*n) + +#define SIRF_PWM_TR_STEP(n) (0x48 + 0x8*n) +#define SIRF_PWM_STEP_HOLD(n) (0x4c + 0x8*n) + +#define SRC_FIELD_SIZE 3 +#define BYPASS_MODE_BIT 21 +#define TRANS_MODE_SELECT_BIT 7 + +#define SIRF_PWM_CHL_NUM 7 +#define SIRF_PWM_BLS_GRP_NUM 16 + +struct sirf_pwm { + void __iomem *base; + struct clk *clk; + struct pwm_chip chip; + unsigned long src_clk_rate; +}; + +#define to_sirf_chip(chip) container_of(chip, struct sirf_pwm, chip) + +static unsigned int sirf_pwm_ns_to_cycles(struct pwm_chip *chip, unsigned int time_ns) +{ + struct sirf_pwm *spwm = to_sirf_chip(chip); + u64 dividend; + unsigned int cycle; + + dividend = spwm->src_clk_rate * time_ns + NSEC_PER_SEC / 2; + do_div(dividend, NSEC_PER_SEC); + + cycle = dividend & 0xFFFFFFFFUL; + + return cycle > 1 ? cycle : 1; +} + +static int sirf_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, + int duty_ns, int period_ns) +{ + unsigned int period_cycles, high_cycles, low_cycles; + unsigned int val; + struct sirf_pwm *spwm = to_sirf_chip(chip); + + period_cycles = sirf_pwm_ns_to_cycles(chip, period_ns); + + high_cycles = sirf_pwm_ns_to_cycles(chip, duty_ns); + low_cycles = period_cycles - high_cycles; + + if (period_cycles == 1) { + /* bypass mode */ + val = readl(spwm->base + SIRF_PWM_SELECT_PRECLK); + val |= 0x1 << (BYPASS_MODE_BIT + pwm->hwpwm); + writel(val, spwm->base + SIRF_PWM_SELECT_PRECLK); + dev_warn(chip->dev, "period is too short!\n"); + } else { + /* divider mode */ + val = readl(spwm->base + SIRF_PWM_SELECT_PRECLK); + val &= ~(0x1 << (BYPASS_MODE_BIT + pwm->hwpwm)); + writel(val, spwm->base + SIRF_PWM_SELECT_PRECLK); + + if (high_cycles == period_cycles) { + high_cycles--; + low_cycles = 1; + } + + writel(high_cycles, spwm->base + SIRF_PWM_GET_WAIT_OFFSET(pwm->hwpwm)); + writel(low_cycles, spwm->base + SIRF_PWM_GET_HOLD_OFFSET(pwm->hwpwm)); + } + + return 0; +} + +static int sirf_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct sirf_pwm *spwm = to_sirf_chip(chip); + unsigned int val; + + /* disable preclock */ + val = readl(spwm->base + SIRF_PWM_ENABLE_PRECLOCK); + val &= ~(1 << pwm->hwpwm); + writel(val, spwm->base + SIRF_PWM_ENABLE_PRECLOCK); + + /* select preclock source must after disable preclk*/ + val = readl(spwm->base + SIRF_PWM_SELECT_PRECLK); + val &= ~(0x7 << (SRC_FIELD_SIZE * pwm->hwpwm)); + writel(val, spwm->base + SIRF_PWM_SELECT_PRECLK); + /* wait for some time */ + udelay(100); + + /* enable preclock */ + val = readl(spwm->base + SIRF_PWM_ENABLE_PRECLOCK); + val |= (1 << pwm->hwpwm); + writel(val, spwm->base + SIRF_PWM_ENABLE_PRECLOCK); + + /* enable post clock*/ + val = readl(spwm->base + SIRF_PWM_ENABLE_POSTCLOCK); + val |= (1 << pwm->hwpwm); + writel(val, spwm->base + SIRF_PWM_ENABLE_POSTCLOCK); + + /* enable output */ + val = readl(spwm->base + SIRF_PWM_OE); + val |= 1 << pwm->hwpwm; + val &= ~(1 << (pwm->hwpwm + TRANS_MODE_SELECT_BIT)); + + writel(val, spwm->base + SIRF_PWM_OE); + + return 0; +} + +static void sirf_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + unsigned int val; + struct sirf_pwm *spwm = to_sirf_chip(chip); + /* disable output */ + val = readl(spwm->base + SIRF_PWM_OE); + val &= ~(1 << pwm->hwpwm); + writel(val, spwm->base + SIRF_PWM_OE); + + /* disable postclock */ + val = readl(spwm->base + SIRF_PWM_ENABLE_POSTCLOCK); + val &= ~(1 << pwm->hwpwm); + writel(val, spwm->base + SIRF_PWM_ENABLE_POSTCLOCK); + + /* disable preclock */ + val = readl(spwm->base + SIRF_PWM_ENABLE_PRECLOCK); + val &= ~(1 << pwm->hwpwm); + writel(val, spwm->base + SIRF_PWM_ENABLE_PRECLOCK); +} + +static struct pwm_ops sirf_pwm_ops = { + .enable = sirf_pwm_enable, + .disable = sirf_pwm_disable, + .config = sirf_pwm_config, + .owner = THIS_MODULE, +}; + +static int sirf_pwm_probe(struct platform_device *pdev) +{ + struct sirf_pwm *spwm; + struct resource *mem_res; + struct clk *clk_pwm_src; + int ret; + + spwm = devm_kzalloc(&pdev->dev, sizeof(struct sirf_pwm), + GFP_KERNEL); + if (!spwm) + return -ENOMEM; + + platform_set_drvdata(pdev, spwm); + + mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + spwm->base = devm_ioremap_resource(&pdev->dev, mem_res); + if (!spwm->base) + return -ENOMEM; + + /* + * the 1st clock is for PWM controller + */ + spwm->clk = of_clk_get(pdev->dev.of_node, 0); + if (IS_ERR(spwm->clk)) { + dev_err(&pdev->dev, "Get PWM controller clock failed.\n"); + return PTR_ERR(spwm->clk); + } + clk_prepare_enable(spwm->clk); + + /* + * the 2nd clock is the source to generate PWM waves + * it is the OSC on SiRFSoC + */ + clk_pwm_src = of_clk_get(pdev->dev.of_node, 1); + if (IS_ERR(clk_pwm_src)) { + dev_err(&pdev->dev, "Get PWM source clock failed.\n"); + return PTR_ERR(clk_pwm_src); + } + spwm->src_clk_rate = clk_get_rate(clk_pwm_src); + clk_put(clk_pwm_src); + + spwm->chip.dev = &pdev->dev; + spwm->chip.ops = &sirf_pwm_ops; + spwm->chip.base = 0; + spwm->chip.npwm = SIRF_PWM_CHL_NUM; + + ret = pwmchip_add(&spwm->chip); + if (ret < 0) { + dev_err(&pdev->dev, "failed to register pwm\n"); + clk_disable_unprepare(spwm->clk); + return ret; + } + + return 0; +} + +static int sirf_pwm_remove(struct platform_device *pdev) +{ + struct sirf_pwm *spwm = platform_get_drvdata(pdev); + clk_disable_unprepare(spwm->clk); + clk_put(spwm->clk); + + pwmchip_remove(&spwm->chip); + return 0; +} + +#ifdef CONFIG_PM_SLEEP +static int sirf_pwm_suspend(struct device *dev) +{ + struct platform_device *pdev = to_platform_device(dev); + struct sirf_pwm *spwm = platform_get_drvdata(pdev); + + clk_disable_unprepare(spwm->clk); + + return 0; +} + +static void sirf_pwm_config_restore(struct sirf_pwm *spwm) +{ + struct pwm_device *pwm; + int i; + + for (i = 0; i < spwm->chip.npwm; i++) { + pwm = &spwm->chip.pwms[i]; + /* + * while restoring from hibernation, state of pwm is enabled, + * but PWM hardware is not re-enabled + */ + if (test_bit(PWMF_REQUESTED, &pwm->flags) && + test_bit(PWMF_ENABLED, &pwm->flags)) + sirf_pwm_enable(&spwm->chip, pwm); + } +} + +static int sirf_pwm_resume(struct device *dev) +{ + struct sirf_pwm *spwm = dev_get_drvdata(dev); + + clk_prepare_enable(spwm->clk); + + sirf_pwm_config_restore(spwm); + + return 0; +} + +static int sirf_pwm_restore(struct device *dev) +{ + struct sirf_pwm *spwm = dev_get_drvdata(dev); + + /* back from hibernation, clock is already enabled */ + sirf_pwm_config_restore(spwm); + + return 0; +} + +#else +#define sirf_pwm_resume NULL +#define sirf_pwm_suspend NULL +#define sirf_pwm_restore NULL +#endif + +static const struct dev_pm_ops sirf_pwm_pm_ops = { + .suspend = sirf_pwm_suspend, + .resume = sirf_pwm_resume, + .restore = sirf_pwm_restore, +}; + +static const struct of_device_id sirf_pwm_of_match[] = { + { .compatible = "sirf,prima2-pwm", }, + {} +}; +MODULE_DEVICE_TABLE(of, sirf_pwm_of_match); + +static struct platform_driver sirf_pwm_driver = { + .driver = { + .name = "prima2-pwm", + .owner = THIS_MODULE, + .pm = &sirf_pwm_pm_ops, + .of_match_table = sirf_pwm_of_match, + }, + .probe = sirf_pwm_probe, + .remove = sirf_pwm_remove, +}; + +module_platform_driver(sirf_pwm_driver); + +MODULE_DESCRIPTION("SIRF serial SoC PWM device core driver"); +MODULE_AUTHOR("RongJun Ying <Rongjun.Ying@csr.com>, " + "Huayi Li <huayi.li@csr.com>"); +MODULE_LICENSE("GPL v2");