| Message ID | 4f6e3110b4d7e0a2f7ab317bba98a933de12e5da.1565703607.git.baolin.wang@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] dt-bindings: pwm: sprd: Add Spreadtrum PWM documentation | expand |
Hi Uwe, On Wed, 14 Aug 2019 at 18:55, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello Baolin, > > On Wed, Aug 14, 2019 at 06:01:50PM +0800, Baolin Wang wrote: > > On Wed, 14 Aug 2019 at 17:23, Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > On Wed, Aug 14, 2019 at 04:42:28PM +0800, Baolin Wang wrote: > > > > On Tue, 13 Aug 2019 at 23:16, Uwe Kleine-König > > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > On Tue, Aug 13, 2019 at 09:46:41PM +0800, Baolin Wang wrote: > > > > > [...] > > > > Not really, our hardware's method is, when you changed a new > > > > configuration (MOD or duty is changed) , the hardware will wait for a > > > > while to complete current period, then change to the new period. > > > > > > Can you describe that in more detail? This doesn't explain why MOD must be > > > configured before DUTY. Is there another reason for that? > > > > Sorry, I did not explain this explicitly. When we change a new PWM > > configuration, the PWM controller will make sure the current period is > > completed before changing to a new period. Once setting the MOD > > register (since we always set MOD firstly), that will tell the > > hardware that a new period need to change. > > So if the current period just ended after you reconfigured MOD but > before you wrote to DUTY we'll see a bogus period, right? I assume the > same holds true for writing the prescale value? I confirmed again, I am sorry I missed something before. Yes, like you said before, writing DUTY triggers the hardware to actually apply the values written to MOD and DUTY to the output. So write DUTY last. I will update the comments and change the PWM configure like: sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PRESCALE, prescale); sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_MOD, SPRD_PWM_MOD_MAX); sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_DUTY, duty); > > > The reason MOD must be configured before DUTY is that, if we > > configured DUTY firstly, the PWM can work abnormally if the current > > DUTY is larger than previous MOD. That is also our hardware's > > limitation. > > OK, so you must not get into a situation where DUTY > MOD, right? > > Now if the hardware was configured for > > period = 8s, duty = 4s > > and now you are supposed to change to > > period = 2s, duty = 1s > > you'd need to write DUTY first, don't you? > > > > > > > +static int sprd_pwm_remove(struct platform_device *pdev) > > > > > > +{ > > > > > > + struct sprd_pwm_chip *spc = platform_get_drvdata(pdev); > > > > > > + int ret, i; > > > > > > + > > > > > > + ret = pwmchip_remove(&spc->chip); > > > > > > + > > > > > > + for (i = 0; i < spc->num_pwms; i++) { > > > > > > + struct sprd_pwm_chn *chn = &spc->chn[i]; > > > > > > + > > > > > > + clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks); > > > > > > > > > > If a PWM was still running you're effectively stopping it here, right? > > > > > Are you sure you don't disable once more than you enabled? > > > > > > > > Yes, you are right. I should check current enable status of the PWM channel. > > > > Thanks for your comments. > > > > > > I didn't recheck, but I think the right approach is to not fiddle with > > > the clocks at all and rely on the PWM framework to not let someone call > > > sprd_pwm_remove when a PWM is still in use. > > > > So you mean just return pwmchip_remove(&spc->chip); ? > > right. > > I just rechecked: If there is still a pwm in use, pwmchip_remove returns > -EBUSY. So this should be safe. Yes. Thanks for your comments. -- Baolin Wang Best Regards
On 14. 08. 19 11:23, Uwe Kleine-König wrote: > Hello Baolin, > > On Wed, Aug 14, 2019 at 04:42:28PM +0800, Baolin Wang wrote: >> On Tue, 13 Aug 2019 at 23:16, Uwe Kleine-König >> <u.kleine-koenig@pengutronix.de> wrote: >>> On Tue, Aug 13, 2019 at 09:46:41PM +0800, Baolin Wang wrote: >>> [...] >> Not really, our hardware's method is, when you changed a new >> configuration (MOD or duty is changed) , the hardware will wait for a >> while to complete current period, then change to the new period. > > Can you describe that in more detail? This doesn't explain why MOD must be > configured before DUTY. Is there another reason for that? > >>>> +static int sprd_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, >>>> + struct pwm_state *state) >>>> +{ >>>> + struct sprd_pwm_chip *spc = >>>> + container_of(chip, struct sprd_pwm_chip, chip); >>>> + struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm]; >>>> + struct pwm_state cstate; >>>> + int ret; >>>> + >>>> + pwm_get_state(pwm, &cstate); >>> >>> I don't like it when pwm drivers call pwm_get_state(). If ever >>> pwm_get_state would take a lock, this would deadlock as the lock is >>> probably already taken when your .apply() callback is running. Moreover >>> the (expensive) calculations are not used appropriately. See below. >> >> I do not think so, see: >> >> static inline void pwm_get_state(const struct pwm_device *pwm, struct >> pwm_state *state) >> { >> *state = pwm->state; >> } > > OK, the PWM framework currently caches this for you. Still I would > prefer if you didn't call PWM API functions in your lowlevel driver. > There is (up to now) nothing bad that will happen if you do it anyhow, > but when the PWM framework evolves it might (and I want to work on such > an evolution). You must not call clk_get_rate() in a .set_rate() > callback of a clock either. > >>>> + if (state->enabled) { >>>> + if (!cstate.enabled) { >>> >>> To just know the value of cstate.enabled you only need to read the >>> register with the ENABLE flag. That is cheaper than calling get_state. >>> >>>> + /* >>>> + * The clocks to PWM channel has to be enabled first >>>> + * before writing to the registers. >>>> + */ >>>> + ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS, >>>> + chn->clks); >>>> + if (ret) { >>>> + dev_err(spc->dev, >>>> + "failed to enable pwm%u clocks\n", >>>> + pwm->hwpwm); >>>> + return ret; >>>> + } >>>> + } >>>> + >>>> + if (state->period != cstate.period || >>>> + state->duty_cycle != cstate.duty_cycle) { >>> >>> This is a coarse check. If state->period and cstate.period only differ >>> by one calling sprd_pwm_config(spc, pwm, state->duty_cycle, >>> state->period) probably results in a noop. So you're doing an expensive >>> division to get an unreliable check. It would be better to calculate the >>> register values from the requested state and compare the register >>> values. The costs are more or less the same than calling .get_state and >>> the check is reliable. And you don't need to spend another division to >>> calculate the new register values. >> >> Same as above comment. > > When taking the caching into account (which I wouldn't) the check is > still incomplete. OK, you could argue avoiding the recalculation in 90% > (to just say some number) of the cases where it is unnecessary is good. > >>> >>>> + ret = sprd_pwm_config(spc, pwm, state->duty_cycle, >>>> + state->period); >>>> + if (ret) >>>> + return ret; >>>> + } >>>> + >>>> + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 1); >>>> + } else if (cstate.enabled) { >>>> + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 0); >>>> + >>>> + clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks); >>> >>> Assuming writing SPRD_PWM_ENABLE = 0 to the hardware completes the >>> currently running period and the write doesn't block that long: Does >>> disabling the clocks interfere with completing the period? >> >> Writing SPRD_PWM_ENABLE = 0 will stop the PWM immediately, will not >> waiting for completing the currently period. > > The currently active period is supposed to be completed. If you cannot > implement this please point this out as limitation at the top of the > driver. > > Honestly I think most pwm users won't mind and they should get the > possibility to tell they prefer pwm_apply_state to return immediately > even if this could result in a spike. But we're not there yet. > (Actually there are three things a PWM consumer might want: > > a) stop immediately; > b) complete the currently running period, then stop and return only > when the period is completed; or > c) complete the currently running period and then stop, return immediately if > possible. > > Currently the expected semantic is b). Hi Uwe et all, I am sorry for interrupting your discussion. From my (last year or so) observation of the PWM mailing list I see this as a common pattern in almost every submission of a new PWM driver. That is PWM driver authors keep submitting solution a) and you tell them the expected behavior is b). Why is that? I hope that the fact that implementing a) is simpler than b) is not the main reason. I believe that people think a) is the correct solution. I agree that smooth transition from one period/duty setting to different setting makes sense. But I also believe the expected behavior of setting enabled=0 is different. That is to stop immediately the technology that is fed by the PWM signal. Otherwise the concept of enabled/disabled does not make sense because setting duty=0 would have the same effect. So I still wonder where the expectation for b) is coming from. What would be the physical/electrical reasoning for b)? Why/how it can be dangerous/harmful for any device to stop the PWM signal immediately? Would be great to finally reach consensus on that so the expected behavior can be documented. I know you already attempted that [1]. I think you have done a great job but I still consider the part about state changes little controversial and unresolved. Best regards, Michal [1] http://patchwork.ozlabs.org/patch/1021566/
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index a7e5751..31dfc88 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -423,6 +423,17 @@ config PWM_SPEAR To compile this driver as a module, choose M here: the module will be called pwm-spear. +config PWM_SPRD + tristate "Spreadtrum PWM support" + depends on ARCH_SPRD || COMPILE_TEST + depends on HAS_IOMEM + help + Generic PWM framework driver for the PWM controller on + Spreadtrum SoCs. + + To compile this driver as a module, choose M here: the module + will be called pwm-sprd. + config PWM_STI tristate "STiH4xx PWM support" depends on ARCH_STI diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 76b555b..26326ad 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -41,6 +41,7 @@ obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o +obj-$(CONFIG_PWM_SPRD) += pwm-sprd.o obj-$(CONFIG_PWM_STI) += pwm-sti.o obj-$(CONFIG_PWM_STM32) += pwm-stm32.o obj-$(CONFIG_PWM_STM32_LP) += pwm-stm32-lp.o diff --git a/drivers/pwm/pwm-sprd.c b/drivers/pwm/pwm-sprd.c new file mode 100644 index 0000000..067e711 --- /dev/null +++ b/drivers/pwm/pwm-sprd.c @@ -0,0 +1,307 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2019 Spreadtrum Communications Inc. + */ + +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/math64.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> + +#define SPRD_PWM_PRESCALE 0x0 +#define SPRD_PWM_MOD 0x4 +#define SPRD_PWM_DUTY 0x8 +#define SPRD_PWM_ENABLE 0x18 + +#define SPRD_PWM_MOD_MAX GENMASK(7, 0) +#define SPRD_PWM_DUTY_MSK GENMASK(15, 0) +#define SPRD_PWM_PRESCALE_MSK GENMASK(7, 0) +#define SPRD_PWM_ENABLE_BIT BIT(0) + +#define SPRD_PWM_NUM 4 +#define SPRD_PWM_REGS_SHIFT 5 +#define SPRD_PWM_NUM_CLKS 2 +#define SPRD_PWM_OUTPUT_CLK 1 + +struct sprd_pwm_chn { + struct clk_bulk_data clks[SPRD_PWM_NUM_CLKS]; + u32 clk_rate; +}; + +struct sprd_pwm_chip { + void __iomem *base; + struct device *dev; + struct pwm_chip chip; + int num_pwms; + struct sprd_pwm_chn chn[SPRD_PWM_NUM]; +}; + +/* + * The list of clocks required by PWM channels, and each channel has 2 clocks: + * enable clock and pwm clock. + */ +static const char * const sprd_pwm_clks[] = { + "enable0", "pwm0", + "enable1", "pwm1", + "enable2", "pwm2", + "enable3", "pwm3", +}; + +static u32 sprd_pwm_read(struct sprd_pwm_chip *spc, u32 hwid, u32 reg) +{ + u32 offset = reg + (hwid << SPRD_PWM_REGS_SHIFT); + + return readl_relaxed(spc->base + offset); +} + +static void sprd_pwm_write(struct sprd_pwm_chip *spc, u32 hwid, + u32 reg, u32 val) +{ + u32 offset = reg + (hwid << SPRD_PWM_REGS_SHIFT); + + writel_relaxed(val, spc->base + offset); +} + +static void sprd_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, + struct pwm_state *state) +{ + struct sprd_pwm_chip *spc = + container_of(chip, struct sprd_pwm_chip, chip); + struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm]; + u32 val, duty, prescale; + u64 tmp; + int ret; + + /* + * The clocks to PWM channel has to be enabled first before + * reading to the registers. + */ + ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS, chn->clks); + if (ret) { + dev_err(spc->dev, "failed to enable pwm%u clocks\n", + pwm->hwpwm); + return; + } + + val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_ENABLE); + if (val & SPRD_PWM_ENABLE_BIT) + state->enabled = true; + else + state->enabled = false; + + /* + * The hardware provides a counter that is feed by the source clock. + * The period length is (PRESCALE + 1) * MOD counter steps. + * The duty cycle length is (PRESCALE + 1) * DUTY counter steps. + * Thus the period_ns and duty_ns calculation formula should be: + * period_ns = NSEC_PER_SEC * (prescale + 1) * mod / clk_rate + * duty_ns = NSEC_PER_SEC * (prescale + 1) * duty / clk_rate + */ + val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE); + prescale = val & SPRD_PWM_PRESCALE_MSK; + tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX; + state->period = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate); + + val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY); + duty = val & SPRD_PWM_DUTY_MSK; + tmp = (prescale + 1) * NSEC_PER_SEC * duty; + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate); + + /* Disable PWM clocks if the PWM channel is not in enable state. */ + if (!state->enabled) + clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks); +} + +static int sprd_pwm_config(struct sprd_pwm_chip *spc, struct pwm_device *pwm, + int duty_ns, int period_ns) +{ + struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm]; + u64 div, tmp; + u32 prescale, duty; + + /* + * The hardware provides a counter that is feed by the source clock. + * The period length is (PRESCALE + 1) * MOD counter steps. + * The duty cycle length is (PRESCALE + 1) * DUTY counter steps. + * + * To keep the maths simple we're always using MOD = SPRD_PWM_MOD_MAX. + * The value for PRESCALE is selected such that the resulting period + * gets the maximal length not bigger than the requested one with the + * given settings (MOD = SPRD_PWM_MOD_MAX and input clock). + */ + duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns; + + tmp = (u64)chn->clk_rate * period_ns; + div = NSEC_PER_SEC * SPRD_PWM_MOD_MAX; + prescale = div64_u64(tmp, div) - 1; + if (prescale > SPRD_PWM_PRESCALE_MSK) + prescale = SPRD_PWM_PRESCALE_MSK; + + /* + * Note: The MOD must be configured before DUTY, and the hardware can + * ensure current running period is completed before changing a new + * configuration to avoid mixed settings. + */ + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_MOD, SPRD_PWM_MOD_MAX); + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_DUTY, duty); + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PRESCALE, prescale); + + return 0; +} + +static int sprd_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, + struct pwm_state *state) +{ + struct sprd_pwm_chip *spc = + container_of(chip, struct sprd_pwm_chip, chip); + struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm]; + struct pwm_state cstate; + int ret; + + pwm_get_state(pwm, &cstate); + + if (state->enabled) { + if (!cstate.enabled) { + /* + * The clocks to PWM channel has to be enabled first + * before writing to the registers. + */ + ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS, + chn->clks); + if (ret) { + dev_err(spc->dev, + "failed to enable pwm%u clocks\n", + pwm->hwpwm); + return ret; + } + } + + if (state->period != cstate.period || + state->duty_cycle != cstate.duty_cycle) { + ret = sprd_pwm_config(spc, pwm, state->duty_cycle, + state->period); + if (ret) + return ret; + } + + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 1); + } else if (cstate.enabled) { + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 0); + + clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks); + } + + return 0; +} + +static const struct pwm_ops sprd_pwm_ops = { + .apply = sprd_pwm_apply, + .get_state = sprd_pwm_get_state, + .owner = THIS_MODULE, +}; + +static int sprd_pwm_clk_init(struct sprd_pwm_chip *spc) +{ + struct clk *clk_pwm; + int ret, i, clk_index = 0; + + for (i = 0; i < SPRD_PWM_NUM; i++) { + struct sprd_pwm_chn *chn = &spc->chn[i]; + int j; + + for (j = 0; j < SPRD_PWM_NUM_CLKS; ++j) + chn->clks[j].id = sprd_pwm_clks[clk_index++]; + + ret = devm_clk_bulk_get(spc->dev, SPRD_PWM_NUM_CLKS, chn->clks); + if (ret) { + if (ret == -ENOENT) + break; + + dev_err(spc->dev, "failed to get channel clocks\n"); + return ret; + } + + clk_pwm = chn->clks[SPRD_PWM_OUTPUT_CLK].clk; + chn->clk_rate = clk_get_rate(clk_pwm); + } + + if (!i) { + dev_err(spc->dev, "no available PWM channels\n"); + return -EINVAL; + } + + spc->num_pwms = i; + + return 0; +} + +static int sprd_pwm_probe(struct platform_device *pdev) +{ + struct sprd_pwm_chip *spc; + int ret; + + spc = devm_kzalloc(&pdev->dev, sizeof(*spc), GFP_KERNEL); + if (!spc) + return -ENOMEM; + + spc->base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(spc->base)) + return PTR_ERR(spc->base); + + spc->dev = &pdev->dev; + platform_set_drvdata(pdev, spc); + + ret = sprd_pwm_clk_init(spc); + if (ret) + return ret; + + spc->chip.dev = &pdev->dev; + spc->chip.ops = &sprd_pwm_ops; + spc->chip.base = -1; + spc->chip.npwm = spc->num_pwms; + + ret = pwmchip_add(&spc->chip); + if (ret) + dev_err(&pdev->dev, "failed to add PWM chip\n"); + + return ret; +} + +static int sprd_pwm_remove(struct platform_device *pdev) +{ + struct sprd_pwm_chip *spc = platform_get_drvdata(pdev); + int ret, i; + + ret = pwmchip_remove(&spc->chip); + + for (i = 0; i < spc->num_pwms; i++) { + struct sprd_pwm_chn *chn = &spc->chn[i]; + + clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks); + } + + return ret; +} + +static const struct of_device_id sprd_pwm_of_match[] = { + { .compatible = "sprd,ums512-pwm", }, + { }, +}; +MODULE_DEVICE_TABLE(of, sprd_pwm_of_match); + +static struct platform_driver sprd_pwm_driver = { + .driver = { + .name = "sprd-pwm", + .of_match_table = sprd_pwm_of_match, + }, + .probe = sprd_pwm_probe, + .remove = sprd_pwm_remove, +}; + +module_platform_driver(sprd_pwm_driver); + +MODULE_DESCRIPTION("Spreadtrum PWM Driver"); +MODULE_LICENSE("GPL v2");