| Message ID | 23d08fa45237efd83cb9dd51a259e2c980f01b3f.1716795485.git.zhoubinbin@loongson.cn |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | pwm: Introduce pwm driver for the Loongson family chips | expand |
Hello, On Mon, May 27, 2024 at 03:51:12PM +0800, Binbin Zhou wrote: > +static int pwm_loongson_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + int ret; > + u64 period, duty_cycle; > + bool enabled = pwm->state.enabled; > + > + period = min(state->period, NANOHZ_PER_HZ); > + duty_cycle = min(state->duty_cycle, NANOHZ_PER_HZ); > + > + if (state->polarity != pwm->state.polarity) { > + if (enabled) { > + pwm_loongson_disable(chip, pwm); > + enabled = false; > + } > + > + ret = pwm_loongson_set_polarity(chip, pwm, state->polarity); > + if (ret) > + return ret; > + } > + > + if (!state->enabled) { > + if (enabled) > + pwm_loongson_disable(chip, pwm); > + return 0; > + } Given that the configured polarity isn't relevant for a disabled PWM, I suggest to swap these two if blocks. However then you have to be a bit more careful for the polarity check because otherwise the following series of commands yields wrong results: pwm_apply_might_sleep(pwm, {.duty_cycle = D, .period = P, .polarity = PWM_POLARITY_NORMAL, .enabled = true}); pwm_apply_might_sleep(pwm, {.duty_cycle = D, .period = P, .polarity = PWM_POLARITY_INVERSED, .enabled = false}); pwm_apply_might_sleep(pwm, {.duty_cycle = D, .period = P, .polarity = PWM_POLARITY_INVERSED, .enabled = true}); > + ret = pwm_loongson_config(chip, pwm, duty_cycle, period); > + if (ret) > + return ret; > + > + if (!enabled) > + ret = pwm_loongson_enable(chip, pwm); > + > + return ret; > +} > + > +static int pwm_loongson_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + u32 duty, period, ctrl; > + struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip); > + > + /* duty_cycle = ddata->duty * NSEC_PER_SEC / ddata->clk_rate */ > + duty = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_DUTY); > + state->duty_cycle = mul_u64_u64_div_u64(duty, NSEC_PER_SEC, ddata->clk_rate); > + > + /* period = ddata->period * NSEC_PER_SEC / ddata->clk_rate */ > + period = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_PERIOD); > + state->period = mul_u64_u64_div_u64(period, NSEC_PER_SEC, ddata->clk_rate); > + > + ctrl = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_CTRL); > + state->polarity = (ctrl & LOONGSON_PWM_CTRL_INVERT) ? PWM_POLARITY_INVERSED : > + PWM_POLARITY_NORMAL; > + state->enabled = (ctrl & LOONGSON_PWM_CTRL_EN) ? true : false; > + > + return 0; You didn't test extensively with PWM_DEBUG enabled, right? You need to round up the divisions here otherwise you get strange rounding results: Consider ddata->clk_rate = 1594323. When a state with .period = 20000 is applied, LOONGSON_PWM_REG_PERIOD is assigned 31. Calling .get_state() in this situation gives .period = 19443. Reapplying .period = 19443 results in LOONGSON_PWM_REG_PERIOD := 30. Iterating this further yields: - .period = 18816 - LOONGSON_PWM_REG_PERIOD := 29 - .period = 18189 - LOONGSON_PWM_REG_PERIOD := 28 - ... > +} > + > +static const struct pwm_ops pwm_loongson_ops = { > + .apply = pwm_loongson_apply, > + .get_state = pwm_loongson_get_state, > +}; > + > +static int pwm_loongson_probe(struct platform_device *pdev) > +{ > + int ret; > + struct pwm_chip *chip; > + struct pwm_loongson_ddata *ddata; > + struct device *dev = &pdev->dev; > + > + chip = devm_pwmchip_alloc(dev, 1, sizeof(*ddata)); > + if (IS_ERR(chip)) > + return PTR_ERR(chip); > + ddata = to_pwm_loongson_ddata(chip); > + > + ddata->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(ddata->base)) > + return PTR_ERR(ddata->base); > + > + if (!has_acpi_companion(dev)) { > + ddata->clk = devm_clk_get_enabled(dev, NULL); > + if (IS_ERR(ddata->clk)) > + return dev_err_probe(dev, PTR_ERR(ddata->clk), > + "failed to get pwm clock\n"); > + ddata->clk_rate = clk_get_rate(ddata->clk); > + } else { > + ddata->clk_rate = LOONGSON_PWM_FREQ_STD; > + } > + > + chip->ops = &pwm_loongson_ops; > + dev_set_drvdata(dev, chip); > + > + ret = devm_pwmchip_add(dev, chip); > + if (ret < 0) { > + clk_disable_unprepare(ddata->clk); This is wrong. You aquired the clk using devm_clk_get_enabled(), so you don't need (and must not) care for disable. > + return dev_err_probe(dev, ret, "failed to add PWM chip\n"); > + } > + > + return 0; > +} Best regards Uwe
Hi Uwe: Thanks for your reply. On Sat, Jul 6, 2024 at 5:26 AM Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > > Hello, > > On Mon, May 27, 2024 at 03:51:12PM +0800, Binbin Zhou wrote: > > +static int pwm_loongson_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > + const struct pwm_state *state) > > +{ > > + int ret; > > + u64 period, duty_cycle; > > + bool enabled = pwm->state.enabled; > > + > > + period = min(state->period, NANOHZ_PER_HZ); > > + duty_cycle = min(state->duty_cycle, NANOHZ_PER_HZ); > > + > > + if (state->polarity != pwm->state.polarity) { > > + if (enabled) { > > + pwm_loongson_disable(chip, pwm); > > + enabled = false; > > + } > > + > > + ret = pwm_loongson_set_polarity(chip, pwm, state->polarity); > > + if (ret) > > + return ret; > > + } > > + > > + if (!state->enabled) { > > + if (enabled) > > + pwm_loongson_disable(chip, pwm); > > + return 0; > > + } > > Given that the configured polarity isn't relevant for a disabled PWM, I > suggest to swap these two if blocks. However then you have to be a bit > more careful for the polarity check because otherwise the following > series of commands yields wrong results: > > pwm_apply_might_sleep(pwm, {.duty_cycle = D, .period = P, .polarity = PWM_POLARITY_NORMAL, .enabled = true}); > pwm_apply_might_sleep(pwm, {.duty_cycle = D, .period = P, .polarity = PWM_POLARITY_INVERSED, .enabled = false}); > pwm_apply_might_sleep(pwm, {.duty_cycle = D, .period = P, .polarity = PWM_POLARITY_INVERSED, .enabled = true}); > Yes, we'd better make sure pwm is enabled first. I will swap the two if blocks. > > + ret = pwm_loongson_config(chip, pwm, duty_cycle, period); > > + if (ret) > > + return ret; > > + > > + if (!enabled) > > + ret = pwm_loongson_enable(chip, pwm); > > + > > + return ret; > > +} > > + > > +static int pwm_loongson_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > > + struct pwm_state *state) > > +{ > > + u32 duty, period, ctrl; > > + struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip); > > + > > + /* duty_cycle = ddata->duty * NSEC_PER_SEC / ddata->clk_rate */ > > + duty = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_DUTY); > > + state->duty_cycle = mul_u64_u64_div_u64(duty, NSEC_PER_SEC, ddata->clk_rate); > > + > > + /* period = ddata->period * NSEC_PER_SEC / ddata->clk_rate */ > > + period = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_PERIOD); > > + state->period = mul_u64_u64_div_u64(period, NSEC_PER_SEC, ddata->clk_rate); > > + > > + ctrl = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_CTRL); > > + state->polarity = (ctrl & LOONGSON_PWM_CTRL_INVERT) ? PWM_POLARITY_INVERSED : > > + PWM_POLARITY_NORMAL; > > + state->enabled = (ctrl & LOONGSON_PWM_CTRL_EN) ? true : false; > > + > > + return 0; > > You didn't test extensively with PWM_DEBUG enabled, right? You need to > round up the divisions here otherwise you get strange rounding results: > > Consider ddata->clk_rate = 1594323. When a state with .period = 20000 is > applied, LOONGSON_PWM_REG_PERIOD is assigned 31. > Calling .get_state() in this situation gives .period = 19443. Reapplying > .period = 19443 results in LOONGSON_PWM_REG_PERIOD := 30. Iterating this > further yields: > > - .period = 18816 > - LOONGSON_PWM_REG_PERIOD := 29 > - .period = 18189 > - LOONGSON_PWM_REG_PERIOD := 28 > - ... > Yes, I'm very sorry I didn't do extensive testing with PWM_DEBUG enabled. Here I do need to use DIV64_U64_ROUND_UP(). Below: /* duty_cycle = ddata->duty * NSEC_PER_SEC / ddata->clk_rate */ duty = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_DUTY); state->duty_cycle = DIV64_U64_ROUND_UP((u64)duty * NSEC_PER_SEC, ddata->clk_rate); /* period = ddata->period * NSEC_PER_SEC / ddata->clk_rate */ period = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_PERIOD); state->period = DIV64_U64_ROUND_UP((u64)period * NSEC_PER_SEC, ddata->clk_rate); I'd also like to ask which tests I still need to do to make sure the driver is more complete? > > +} > > + > > +static const struct pwm_ops pwm_loongson_ops = { > > + .apply = pwm_loongson_apply, > > + .get_state = pwm_loongson_get_state, > > +}; > > + > > +static int pwm_loongson_probe(struct platform_device *pdev) > > +{ > > + int ret; > > + struct pwm_chip *chip; > > + struct pwm_loongson_ddata *ddata; > > + struct device *dev = &pdev->dev; > > + > > + chip = devm_pwmchip_alloc(dev, 1, sizeof(*ddata)); > > + if (IS_ERR(chip)) > > + return PTR_ERR(chip); > > + ddata = to_pwm_loongson_ddata(chip); > > + > > + ddata->base = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(ddata->base)) > > + return PTR_ERR(ddata->base); > > + > > + if (!has_acpi_companion(dev)) { > > + ddata->clk = devm_clk_get_enabled(dev, NULL); > > + if (IS_ERR(ddata->clk)) > > + return dev_err_probe(dev, PTR_ERR(ddata->clk), > > + "failed to get pwm clock\n"); > > + ddata->clk_rate = clk_get_rate(ddata->clk); > > + } else { > > + ddata->clk_rate = LOONGSON_PWM_FREQ_STD; > > + } > > + > > + chip->ops = &pwm_loongson_ops; > > + dev_set_drvdata(dev, chip); > > + > > + ret = devm_pwmchip_add(dev, chip); > > + if (ret < 0) { > > + clk_disable_unprepare(ddata->clk); > > This is wrong. You aquired the clk using devm_clk_get_enabled(), so you > don't need (and must not) care for disable. > Sorry, I had some misunderstanding about devm_clk_get_enabled(), I will fix it. Thanks. Binbin > > + return dev_err_probe(dev, ret, "failed to add PWM chip\n"); > > + } > > + > > + return 0; > > +} > > Best regards > Uwe
Hello Binbin, On Sat, Jul 06, 2024 at 03:08:30PM +0600, Binbin Zhou wrote: > Hi Uwe: > > Thanks for your reply. > > On Sat, Jul 6, 2024 at 5:26 AM Uwe Kleine-König > <u.kleine-koenig@baylibre.com> wrote: > > > > Hello, > > > > On Mon, May 27, 2024 at 03:51:12PM +0800, Binbin Zhou wrote: > > > +static int pwm_loongson_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > + const struct pwm_state *state) > > > +{ > > > + int ret; > > > + u64 period, duty_cycle; > > > + bool enabled = pwm->state.enabled; > > > + > > > + period = min(state->period, NANOHZ_PER_HZ); > > > + duty_cycle = min(state->duty_cycle, NANOHZ_PER_HZ); > > > + > > > + if (state->polarity != pwm->state.polarity) { > > > + if (enabled) { > > > + pwm_loongson_disable(chip, pwm); > > > + enabled = false; > > > + } > > > + > > > + ret = pwm_loongson_set_polarity(chip, pwm, state->polarity); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > + if (!state->enabled) { > > > + if (enabled) > > > + pwm_loongson_disable(chip, pwm); > > > + return 0; > > > + } > > > > Given that the configured polarity isn't relevant for a disabled PWM, I > > suggest to swap these two if blocks. However then you have to be a bit > > more careful for the polarity check because otherwise the following > > series of commands yields wrong results: > > > > pwm_apply_might_sleep(pwm, {.duty_cycle = D, .period = P, .polarity = PWM_POLARITY_NORMAL, .enabled = true}); > > pwm_apply_might_sleep(pwm, {.duty_cycle = D, .period = P, .polarity = PWM_POLARITY_INVERSED, .enabled = false}); > > pwm_apply_might_sleep(pwm, {.duty_cycle = D, .period = P, .polarity = PWM_POLARITY_INVERSED, .enabled = true}); > > > > Yes, we'd better make sure pwm is enabled first. > I will swap the two if blocks. > > > > + ret = pwm_loongson_config(chip, pwm, duty_cycle, period); > > > + if (ret) > > > + return ret; > > > + > > > + if (!enabled) > > > + ret = pwm_loongson_enable(chip, pwm); > > > + > > > + return ret; > > > +} > > > + > > > +static int pwm_loongson_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > > > + struct pwm_state *state) > > > +{ > > > + u32 duty, period, ctrl; > > > + struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip); > > > + > > > + /* duty_cycle = ddata->duty * NSEC_PER_SEC / ddata->clk_rate */ > > > + duty = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_DUTY); > > > + state->duty_cycle = mul_u64_u64_div_u64(duty, NSEC_PER_SEC, ddata->clk_rate); > > > + > > > + /* period = ddata->period * NSEC_PER_SEC / ddata->clk_rate */ > > > + period = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_PERIOD); > > > + state->period = mul_u64_u64_div_u64(period, NSEC_PER_SEC, ddata->clk_rate); > > > + > > > + ctrl = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_CTRL); > > > + state->polarity = (ctrl & LOONGSON_PWM_CTRL_INVERT) ? PWM_POLARITY_INVERSED : > > > + PWM_POLARITY_NORMAL; > > > + state->enabled = (ctrl & LOONGSON_PWM_CTRL_EN) ? true : false; > > > + > > > + return 0; > > > > You didn't test extensively with PWM_DEBUG enabled, right? You need to > > round up the divisions here otherwise you get strange rounding results: > > > > Consider ddata->clk_rate = 1594323. When a state with .period = 20000 is > > applied, LOONGSON_PWM_REG_PERIOD is assigned 31. > > Calling .get_state() in this situation gives .period = 19443. Reapplying > > .period = 19443 results in LOONGSON_PWM_REG_PERIOD := 30. Iterating this > > further yields: > > > > - .period = 18816 > > - LOONGSON_PWM_REG_PERIOD := 29 > > - .period = 18189 > > - LOONGSON_PWM_REG_PERIOD := 28 > > - ... > > > Yes, I'm very sorry I didn't do extensive testing with PWM_DEBUG enabled. > Here I do need to use DIV64_U64_ROUND_UP(). > > Below: > > /* duty_cycle = ddata->duty * NSEC_PER_SEC / ddata->clk_rate */ > duty = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_DUTY); > state->duty_cycle = DIV64_U64_ROUND_UP((u64)duty * > NSEC_PER_SEC, ddata->clk_rate); > > /* period = ddata->period * NSEC_PER_SEC / ddata->clk_rate */ > period = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_PERIOD); > state->period = DIV64_U64_ROUND_UP((u64)period * NSEC_PER_SEC, > ddata->clk_rate); > > > I'd also like to ask which tests I still need to do to make sure the > driver is more complete? There is no official lower limit on tests. But the goal is that your driver behaves correctly and some of the typical errors can be catched by enabling PWM_DEBUG and then doing the following tests for sensible values of A and B: # Sequence of increasing periods for period in A ... B: configure 0/period # Sequence of decreasing periods for period in A ... B: configure 0/(B + A - period) for period in some set: # Sequence of increasing duty length for duty_cycle in 0 ... period: configure duty_cycle/period # Sequence of decreasing duty length for duty_cycle in 0 ... period: configure (period - duty_cycle)/period That should give you a good coverage. The idea of extensive testing on your side is also: Review capacities are a scarce resource and you suffer from that because it takes some patience between you sending a patch and a maintainer coming around to review it. If your code is well tested, there is less for the maintainers to find and so I save time because there are less revisions and they mature quicker. The direct consequence for you and the others waiting for my attention should be obvious. (This also means: If you look into other's patch submissions and point out the things you already learned to them, you also save maintainer time and so might get their attention earlier yourself.) I'm working on a bigger change for the pwm subsystem. One of the results will be a new userspace API. I intend to create a test program that does the above tests, so in the foreseeable future testing should get easier. (In return writing a lowlevel driver will become a bit harder, as the requirements for precision increase.) Best regards Uwe
Hi Uwe: On Sat, Jul 6, 2024 at 9:25 PM Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > > Hello Binbin, > > On Sat, Jul 06, 2024 at 03:08:30PM +0600, Binbin Zhou wrote: > > Hi Uwe: > > > > Thanks for your reply. > > > > On Sat, Jul 6, 2024 at 5:26 AM Uwe Kleine-König > > <u.kleine-koenig@baylibre.com> wrote: > > > > > > Hello, > > > > > > On Mon, May 27, 2024 at 03:51:12PM +0800, Binbin Zhou wrote: > > > > +static int pwm_loongson_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > > + const struct pwm_state *state) > > > > +{ > > > > + int ret; > > > > + u64 period, duty_cycle; > > > > + bool enabled = pwm->state.enabled; > > > > + > > > > + period = min(state->period, NANOHZ_PER_HZ); > > > > + duty_cycle = min(state->duty_cycle, NANOHZ_PER_HZ); > > > > + > > > > + if (state->polarity != pwm->state.polarity) { > > > > + if (enabled) { > > > > + pwm_loongson_disable(chip, pwm); > > > > + enabled = false; > > > > + } > > > > + > > > > + ret = pwm_loongson_set_polarity(chip, pwm, state->polarity); > > > > + if (ret) > > > > + return ret; > > > > + } > > > > + > > > > + if (!state->enabled) { > > > > + if (enabled) > > > > + pwm_loongson_disable(chip, pwm); > > > > + return 0; > > > > + } > > > > > > Given that the configured polarity isn't relevant for a disabled PWM, I > > > suggest to swap these two if blocks. However then you have to be a bit > > > more careful for the polarity check because otherwise the following > > > series of commands yields wrong results: > > > > > > pwm_apply_might_sleep(pwm, {.duty_cycle = D, .period = P, .polarity = PWM_POLARITY_NORMAL, .enabled = true}); > > > pwm_apply_might_sleep(pwm, {.duty_cycle = D, .period = P, .polarity = PWM_POLARITY_INVERSED, .enabled = false}); > > > pwm_apply_might_sleep(pwm, {.duty_cycle = D, .period = P, .polarity = PWM_POLARITY_INVERSED, .enabled = true}); > > > > > > > Yes, we'd better make sure pwm is enabled first. > > I will swap the two if blocks. > > > > > > + ret = pwm_loongson_config(chip, pwm, duty_cycle, period); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + if (!enabled) > > > > + ret = pwm_loongson_enable(chip, pwm); > > > > + > > > > + return ret; > > > > +} > > > > + > > > > +static int pwm_loongson_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > > > > + struct pwm_state *state) > > > > +{ > > > > + u32 duty, period, ctrl; > > > > + struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip); > > > > + > > > > + /* duty_cycle = ddata->duty * NSEC_PER_SEC / ddata->clk_rate */ > > > > + duty = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_DUTY); > > > > + state->duty_cycle = mul_u64_u64_div_u64(duty, NSEC_PER_SEC, ddata->clk_rate); > > > > + > > > > + /* period = ddata->period * NSEC_PER_SEC / ddata->clk_rate */ > > > > + period = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_PERIOD); > > > > + state->period = mul_u64_u64_div_u64(period, NSEC_PER_SEC, ddata->clk_rate); > > > > + > > > > + ctrl = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_CTRL); > > > > + state->polarity = (ctrl & LOONGSON_PWM_CTRL_INVERT) ? PWM_POLARITY_INVERSED : > > > > + PWM_POLARITY_NORMAL; > > > > + state->enabled = (ctrl & LOONGSON_PWM_CTRL_EN) ? true : false; > > > > + > > > > + return 0; > > > > > > You didn't test extensively with PWM_DEBUG enabled, right? You need to > > > round up the divisions here otherwise you get strange rounding results: > > > > > > Consider ddata->clk_rate = 1594323. When a state with .period = 20000 is > > > applied, LOONGSON_PWM_REG_PERIOD is assigned 31. > > > Calling .get_state() in this situation gives .period = 19443. Reapplying > > > .period = 19443 results in LOONGSON_PWM_REG_PERIOD := 30. Iterating this > > > further yields: > > > > > > - .period = 18816 > > > - LOONGSON_PWM_REG_PERIOD := 29 > > > - .period = 18189 > > > - LOONGSON_PWM_REG_PERIOD := 28 > > > - ... > > > > > Yes, I'm very sorry I didn't do extensive testing with PWM_DEBUG enabled. > > Here I do need to use DIV64_U64_ROUND_UP(). > > > > Below: > > > > /* duty_cycle = ddata->duty * NSEC_PER_SEC / ddata->clk_rate */ > > duty = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_DUTY); > > state->duty_cycle = DIV64_U64_ROUND_UP((u64)duty * > > NSEC_PER_SEC, ddata->clk_rate); > > > > /* period = ddata->period * NSEC_PER_SEC / ddata->clk_rate */ > > period = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_PERIOD); > > state->period = DIV64_U64_ROUND_UP((u64)period * NSEC_PER_SEC, > > ddata->clk_rate); > > > > > > I'd also like to ask which tests I still need to do to make sure the > > driver is more complete? > > There is no official lower limit on tests. But the goal is that your > driver behaves correctly and some of the typical errors can be catched > by enabling PWM_DEBUG and then doing the following tests for sensible > values of A and B: > > # Sequence of increasing periods > for period in A ... B: > configure 0/period > > # Sequence of decreasing periods > for period in A ... B: > configure 0/(B + A - period) > > for period in some set: > # Sequence of increasing duty length > for duty_cycle in 0 ... period: > configure duty_cycle/period > > # Sequence of decreasing duty length > for duty_cycle in 0 ... period: > configure (period - duty_cycle)/period > > That should give you a good coverage. Thanks for your advice. I would use this approach to test extensively, trying to make sure there are no typical errors. Thanks. Binbin > > The idea of extensive testing on your side is also: Review capacities > are a scarce resource and you suffer from that because it takes some > patience between you sending a patch and a maintainer coming around to > review it. If your code is well tested, there is less for the > maintainers to find and so I save time because there are less revisions > and they mature quicker. The direct consequence for you and the others > waiting for my attention should be obvious. (This also means: If you > look into other's patch submissions and point out the things you already > learned to them, you also save maintainer time and so might get their > attention earlier yourself.) > > I'm working on a bigger change for the pwm subsystem. One of the results > will be a new userspace API. I intend to create a test program that does > the above tests, so in the foreseeable future testing should get easier. > (In return writing a lowlevel driver will become a bit harder, as the > requirements for precision increase.) > > Best regards > Uwe
diff --git a/MAINTAINERS b/MAINTAINERS index 811a7a0dab84..224f684e5e48 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12997,6 +12997,7 @@ M: Binbin Zhou <zhoubinbin@loongson.cn> L: linux-pwm@vger.kernel.org S: Maintained F: Documentation/devicetree/bindings/pwm/loongson,ls7a-pwm.yaml +F: drivers/pwm/pwm-loongson.c LOONGSON-2 SOC SERIES CLOCK DRIVER M: Yinbo Zhu <zhuyinbo@loongson.cn> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 1dd7921194f5..4ba5453ee1f5 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -320,6 +320,18 @@ config PWM_KEEMBAY To compile this driver as a module, choose M here: the module will be called pwm-keembay. +config PWM_LOONGSON + tristate "Loongson PWM support" + depends on MACH_LOONGSON64 || COMPILE_TEST + depends on COMMON_CLK + help + Generic PWM framework driver for Loongson family. + It can be found on Loongson-2K series cpus and Loongson LS7A + bridge chips. + + To compile this driver as a module, choose M here: the module + will be called pwm-loongson. + config PWM_LP3943 tristate "TI/National Semiconductor LP3943 PWM support" depends on MFD_LP3943 diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 90913519f11a..032d73327509 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -27,6 +27,7 @@ obj-$(CONFIG_PWM_INTEL_LGM) += pwm-intel-lgm.o obj-$(CONFIG_PWM_IQS620A) += pwm-iqs620a.o obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o obj-$(CONFIG_PWM_KEEMBAY) += pwm-keembay.o +obj-$(CONFIG_PWM_LOONGSON) += pwm-loongson.o obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o diff --git a/drivers/pwm/pwm-loongson.c b/drivers/pwm/pwm-loongson.c new file mode 100644 index 000000000000..814cca4dc0f0 --- /dev/null +++ b/drivers/pwm/pwm-loongson.c @@ -0,0 +1,295 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Loongson PWM driver + * + * Author: Juxin Gao <gaojuxin@loongson.cn> + * Further cleanup and restructuring by: + * Binbin Zhou <zhoubinbin@loongson.cn> + * + * Copyright (C) 2017-2024 Loongson Technology Corporation Limited. + * + * Limitations: + * - The buffer register value should be written before the CTRL register. + * - When disabled the output is driven to 0 independent of the configured + * polarity. + */ + +#include <linux/acpi.h> +#include <linux/clk.h> +#include <linux/device.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/units.h> + +/* Loongson PWM registers */ +#define LOONGSON_PWM_REG_DUTY 0x4 /* Low Pulse Buffer Register */ +#define LOONGSON_PWM_REG_PERIOD 0x8 /* Pulse Period Buffer Register */ +#define LOONGSON_PWM_REG_CTRL 0xc /* Control Register */ + +/* Control register bits */ +#define LOONGSON_PWM_CTRL_EN BIT(0) /* Counter Enable Bit */ +#define LOONGSON_PWM_CTRL_OE BIT(3) /* Pulse Output Enable Control Bit, Valid Low */ +#define LOONGSON_PWM_CTRL_SINGLE BIT(4) /* Single Pulse Control Bit */ +#define LOONGSON_PWM_CTRL_INTE BIT(5) /* Interrupt Enable Bit */ +#define LOONGSON_PWM_CTRL_INT BIT(6) /* Interrupt Bit */ +#define LOONGSON_PWM_CTRL_RST BIT(7) /* Counter Reset Bit */ +#define LOONGSON_PWM_CTRL_CAPTE BIT(8) /* Measurement Pulse Enable Bit */ +#define LOONGSON_PWM_CTRL_INVERT BIT(9) /* Output flip-flop Enable Bit */ +#define LOONGSON_PWM_CTRL_DZONE BIT(10) /* Anti-dead Zone Enable Bit */ + +#define LOONGSON_PWM_FREQ_STD (50 * HZ_PER_KHZ) + +struct pwm_loongson_suspend_store { + u32 ctrl; + u32 duty; + u32 period; +}; + +struct pwm_loongson_ddata { + struct clk *clk; + void __iomem *base; + u64 clk_rate; + struct pwm_loongson_suspend_store lss; +}; + +static inline struct pwm_loongson_ddata *to_pwm_loongson_ddata(struct pwm_chip *chip) +{ + return pwmchip_get_drvdata(chip); +} + +static inline u32 pwm_loongson_readl(struct pwm_loongson_ddata *ddata, u32 offset) +{ + return readl(ddata->base + offset); +} + +static inline void pwm_loongson_writel(struct pwm_loongson_ddata *ddata, + u32 val, u32 offset) +{ + writel(val, ddata->base + offset); +} + +static int pwm_loongson_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm, + enum pwm_polarity polarity) +{ + u16 val; + struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip); + + val = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_CTRL); + + if (polarity == PWM_POLARITY_INVERSED) + /* Duty cycle defines LOW period of PWM */ + val |= LOONGSON_PWM_CTRL_INVERT; + else + /* Duty cycle defines HIGH period of PWM */ + val &= ~LOONGSON_PWM_CTRL_INVERT; + + pwm_loongson_writel(ddata, val, LOONGSON_PWM_REG_CTRL); + + return 0; +} + +static void pwm_loongson_disable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + u32 val; + struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip); + + val = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_CTRL); + val &= ~LOONGSON_PWM_CTRL_EN; + pwm_loongson_writel(ddata, val, LOONGSON_PWM_REG_CTRL); +} + +static int pwm_loongson_enable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + u32 val; + struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip); + + val = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_CTRL); + val |= LOONGSON_PWM_CTRL_EN; + pwm_loongson_writel(ddata, val, LOONGSON_PWM_REG_CTRL); + + return 0; +} + +static int pwm_loongson_config(struct pwm_chip *chip, struct pwm_device *pwm, + u64 duty_ns, u64 period_ns) +{ + u32 duty, period; + struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip); + + /* duty = duty_ns * ddata->clk_rate / NSEC_PER_SEC */ + duty = mul_u64_u64_div_u64(duty_ns, ddata->clk_rate, NSEC_PER_SEC); + pwm_loongson_writel(ddata, duty, LOONGSON_PWM_REG_DUTY); + + /* period = period_ns * ddata->clk_rate / NSEC_PER_SEC */ + period = mul_u64_u64_div_u64(period_ns, ddata->clk_rate, NSEC_PER_SEC); + pwm_loongson_writel(ddata, period, LOONGSON_PWM_REG_PERIOD); + + return 0; +} + +static int pwm_loongson_apply(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) +{ + int ret; + u64 period, duty_cycle; + bool enabled = pwm->state.enabled; + + period = min(state->period, NANOHZ_PER_HZ); + duty_cycle = min(state->duty_cycle, NANOHZ_PER_HZ); + + if (state->polarity != pwm->state.polarity) { + if (enabled) { + pwm_loongson_disable(chip, pwm); + enabled = false; + } + + ret = pwm_loongson_set_polarity(chip, pwm, state->polarity); + if (ret) + return ret; + } + + if (!state->enabled) { + if (enabled) + pwm_loongson_disable(chip, pwm); + return 0; + } + + ret = pwm_loongson_config(chip, pwm, duty_cycle, period); + if (ret) + return ret; + + if (!enabled) + ret = pwm_loongson_enable(chip, pwm); + + return ret; +} + +static int pwm_loongson_get_state(struct pwm_chip *chip, struct pwm_device *pwm, + struct pwm_state *state) +{ + u32 duty, period, ctrl; + struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip); + + /* duty_cycle = ddata->duty * NSEC_PER_SEC / ddata->clk_rate */ + duty = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_DUTY); + state->duty_cycle = mul_u64_u64_div_u64(duty, NSEC_PER_SEC, ddata->clk_rate); + + /* period = ddata->period * NSEC_PER_SEC / ddata->clk_rate */ + period = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_PERIOD); + state->period = mul_u64_u64_div_u64(period, NSEC_PER_SEC, ddata->clk_rate); + + ctrl = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_CTRL); + state->polarity = (ctrl & LOONGSON_PWM_CTRL_INVERT) ? PWM_POLARITY_INVERSED : + PWM_POLARITY_NORMAL; + state->enabled = (ctrl & LOONGSON_PWM_CTRL_EN) ? true : false; + + return 0; +} + +static const struct pwm_ops pwm_loongson_ops = { + .apply = pwm_loongson_apply, + .get_state = pwm_loongson_get_state, +}; + +static int pwm_loongson_probe(struct platform_device *pdev) +{ + int ret; + struct pwm_chip *chip; + struct pwm_loongson_ddata *ddata; + struct device *dev = &pdev->dev; + + chip = devm_pwmchip_alloc(dev, 1, sizeof(*ddata)); + if (IS_ERR(chip)) + return PTR_ERR(chip); + ddata = to_pwm_loongson_ddata(chip); + + ddata->base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(ddata->base)) + return PTR_ERR(ddata->base); + + if (!has_acpi_companion(dev)) { + ddata->clk = devm_clk_get_enabled(dev, NULL); + if (IS_ERR(ddata->clk)) + return dev_err_probe(dev, PTR_ERR(ddata->clk), + "failed to get pwm clock\n"); + ddata->clk_rate = clk_get_rate(ddata->clk); + } else { + ddata->clk_rate = LOONGSON_PWM_FREQ_STD; + } + + chip->ops = &pwm_loongson_ops; + dev_set_drvdata(dev, chip); + + ret = devm_pwmchip_add(dev, chip); + if (ret < 0) { + clk_disable_unprepare(ddata->clk); + return dev_err_probe(dev, ret, "failed to add PWM chip\n"); + } + + return 0; +} + +static int pwm_loongson_suspend(struct device *dev) +{ + struct pwm_chip *chip = dev_get_drvdata(dev); + struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip); + + ddata->lss.ctrl = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_CTRL); + ddata->lss.duty = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_DUTY); + ddata->lss.period = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_PERIOD); + + clk_disable_unprepare(ddata->clk); + + return 0; +} + +static int pwm_loongson_resume(struct device *dev) +{ + int ret; + struct pwm_chip *chip = dev_get_drvdata(dev); + struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip); + + ret = clk_prepare_enable(ddata->clk); + if (ret) + return ret; + + pwm_loongson_writel(ddata, ddata->lss.ctrl, LOONGSON_PWM_REG_CTRL); + pwm_loongson_writel(ddata, ddata->lss.duty, LOONGSON_PWM_REG_DUTY); + pwm_loongson_writel(ddata, ddata->lss.period, LOONGSON_PWM_REG_PERIOD); + + return 0; +} + +static DEFINE_SIMPLE_DEV_PM_OPS(pwm_loongson_pm_ops, pwm_loongson_suspend, + pwm_loongson_resume); + +static const struct of_device_id pwm_loongson_of_ids[] = { + { .compatible = "loongson,ls7a-pwm" }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, pwm_loongson_of_ids); + +static const struct acpi_device_id pwm_loongson_acpi_ids[] = { + { "LOON0006" }, + { } +}; +MODULE_DEVICE_TABLE(acpi, pwm_loongson_acpi_ids); + +static struct platform_driver pwm_loongson_driver = { + .probe = pwm_loongson_probe, + .driver = { + .name = "loongson-pwm", + .pm = pm_ptr(&pwm_loongson_pm_ops), + .of_match_table = pwm_loongson_of_ids, + .acpi_match_table = pwm_loongson_acpi_ids, + }, +}; +module_platform_driver(pwm_loongson_driver); + +MODULE_DESCRIPTION("Loongson PWM driver"); +MODULE_AUTHOR("Loongson Technology Corporation Limited."); +MODULE_LICENSE("GPL");