Hello,
now I come around to review your driver. Sorry that it took so long, I
was busy with other stuff.
On Tue, Dec 13, 2022 at 06:58:25PM +0000, Biju Das wrote:
> The RZ/V2{M, MA} PWM Timer supports the following functions:
>
> * The PWM has 24-bit counters which operate at PWM_CLK (48 MHz).
> * The frequency division ratio for internal counter operation is
> selectable as PWM_CLK divided by 1, 16, 256, or 2048.
> * The period as well as the duty cycle is adjustable.
> * The low-level and high-level order of the PWM signals can be
> inverted.
> * The duty cycle of the PWM signal is selectable in the range from
> 0 to 100%.
> * The minimum resolution is 20.83 ns.
> * Three interrupt sources: Rising and falling edges of the PWM signal
> and clearing of the counter
> * Counter operation and the bus interface are asynchronous and both
> can operate independently of the magnitude relationship of the
> respective clock periods.
>
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> ---
> v2->v3:
> * Added return code for rzv2m_pwm_get_state()
> * Added comment in rzv2m_pwm_reset_assert_pm_disable()
> v1->v2:
> * Replaced devm_reset_control_get_optional_shared->devm_reset_control_get_shared
> ---
> drivers/pwm/Kconfig | 11 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-rzv2m.c | 398 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 410 insertions(+)
> create mode 100644 drivers/pwm/pwm-rzv2m.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index dae023d783a2..31cdc9dae3c5 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -473,6 +473,17 @@ config PWM_RENESAS_TPU
> To compile this driver as a module, choose M here: the module
> will be called pwm-renesas-tpu.
>
> +config PWM_RZV2M
> + tristate "Renesas RZ/V2M PWM support"
> + depends on ARCH_R9A09G011 || COMPILE_TEST
> + depends on HAS_IOMEM
> + help
> + This driver exposes the PWM controller found in Renesas
> + RZ/V2M like chips through the PWM API.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-rzv2m.
> +
> config PWM_ROCKCHIP
> tristate "Rockchip PWM support"
> depends on ARCH_ROCKCHIP || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 7bf1a29f02b8..a95aabae9115 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -43,6 +43,7 @@ obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
> obj-$(CONFIG_PWM_RASPBERRYPI_POE) += pwm-raspberrypi-poe.o
> obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o
> obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o
> +obj-$(CONFIG_PWM_RZV2M) += pwm-rzv2m.o
> obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
> obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
> obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
> diff --git a/drivers/pwm/pwm-rzv2m.c b/drivers/pwm/pwm-rzv2m.c
> new file mode 100644
> index 000000000000..80fb3523026d
> --- /dev/null
> +++ b/drivers/pwm/pwm-rzv2m.c
> @@ -0,0 +1,398 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/V2M PWM Timer (PWM) driver
> + *
> + * Copyright (C) 2022 Renesas Electronics Corporation
> + *
> + * Hardware manual for this IP can be found here
> + * https://www.renesas.com/in/en/document/mah/rzv2m-users-manual-hardware?language=en
> + *
Please document the hardware properties here in a "Limitations"
paragraph similar to e.g. pwm-sl28cpld.c. The idea is to get the
information about how the hardware behaves on .apply (are there
glitches? Can a mixed period happen that has the previous period but the
new duty_cycle? Or maybe duty_cycle and period are shadowed until the
currently running period ends, but polarity takes effect immediately?
etc. pp) when doing
sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/pwm-rzv2m.c
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/pwm.h>
> +#include <linux/reset.h>
> +#include <linux/time.h>
> +
> +#define U24_MASK GENMASK(23, 0)
> +#define U24_MAX (U24_MASK)
> +
> +#define RZV2M_PWMCTR 0x0
> +#define RZV2M_PWMCYC 0x4
> +#define RZV2M_PWMLOW 0x8
> +#define RZV2M_PWMCNT 0xc
> +
> +#define RZV2M_PWMCTR_PWMPS GENMASK(17, 16)
> +#define RZV2M_PWMCTR_PWMHL BIT(3)
> +#define RZV2M_PWMCTR_PWMTM BIT(2)
> +#define RZV2M_PWMCTR_PWME BIT(1)
> +
> +#define F2CYCLE_NSEC(f) (1000000000 / (f))
Driver prefix please.
> +
> +struct rzv2m_pwm_chip {
> + struct pwm_chip chip;
> + void __iomem *mmio;
> + struct reset_control *rstc;
> + struct clk *apb_clk;
> + struct clk *pwm_clk;
> + unsigned long rate;
> + unsigned long delay;
> +};
> +
> +static const int rzv2m_pwm_freq_div[] = { 1, 16, 256, 2048 };
These are powers of 16, so you can use
1 << (4 * i)
instead of rzv2m_pwm_freq_div[i]. This might make it easier for the
compiler to optimize.
> [...]
> +static int rzv2m_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct rzv2m_pwm_chip *rzv2m_pwm = to_rzv2m_pwm_chip(chip);
> + unsigned long pwm_cyc, pwm_low;
> + u8 prescale;
> + u64 pc, dc;
> + int err;
> +
> + /*
> + * Refuse clk rates > 1 GHz to prevent overflowing the following
> + * calculation.
> + */
> + if (rzv2m_pwm->rate > NSEC_PER_SEC)
> + return -EINVAL;
> +
> + /*
> + * Formula for calculating PWM Cycle Setting Register
> + * PWM cycle = (PWM period(ns) / (PWM_CLK period(ns) × Div ratio)) - 1
> + */
> +
> + pc = mul_u64_u32_div(state->period, rzv2m_pwm->rate, NSEC_PER_SEC);
> + dc = mul_u64_u32_div(state->duty_cycle, rzv2m_pwm->rate, NSEC_PER_SEC);
> + prescale = rzv2m_pwm_calculate_prescale(rzv2m_pwm, pc);
> +
> + pwm_cyc = pc / rzv2m_pwm_freq_div[prescale];
That's a 64 bit division, that's not allowed. (I guess the compiler
optimizes that, otherwise I would have expected a build bot to point out
that problem.)
> + if (pc / rzv2m_pwm_freq_div[prescale] <= U24_MAX)
> + pwm_cyc = pwm_cyc ? (pwm_cyc - 1) : 0;
> + else
> + pwm_cyc = U24_MAX;
I'd do that part as follows:
#define RZV2M_PWMCYC_PERIOD GENMASK(23, 0)
pwm_cyc = pc >> (4 * prescale);
if (pwm_cyc)
pwm_cyc -= 1;
if (!FIELD_FIT(RZV2M_PWMCYC_PERIOD, pwm_cyc))
pwm_cyc = FIELD_MAX(RZV2M_PWMCYC_PERIOD);
(Ideally the hardware manual specifies a name for the bitfield. Then of
course pick that one instead of PERIOD.)
> + pwm_low = dc / rzv2m_pwm_freq_div[prescale];
> + if (pwm_low <= U24_MAX)
> + pwm_low = pwm_low ? (pwm_low - 1) : 0;
> + else
> + pwm_low = U24_MAX;
> +
> + /*
> + * If the PWM channel is disabled, make sure to turn on the clock
> + * before writing the register.
> + */
> + if (!pwm_is_enabled(pwm)) {
> + err = pm_runtime_resume_and_get(rzv2m_pwm->chip.dev);
> + if (err)
> + return err;
> + }
> +
> + rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWMTM, 0);
> + rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWMPS,
> + FIELD_PREP(RZV2M_PWMCTR_PWMPS, prescale));
Is there a need to do this in two register writes? If yes, please add
this information in a comment, if not, please do it in one access.
> + rzv2m_pwm_write(rzv2m_pwm, RZV2M_PWMCYC, pwm_cyc);
> + rzv2m_pwm_write(rzv2m_pwm, RZV2M_PWMLOW, pwm_low);
> +
> + if (state->polarity == PWM_POLARITY_NORMAL)
> + rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWMHL, 0);
> + else
> + rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWMHL,
> + RZV2M_PWMCTR_PWMHL);
This can be merged into the above write to RZV2M_PWMCTR, too?!
> + rzv2m_pwm_wait_delay(rzv2m_pwm);
> +
> + /*
> + * If the PWM is not enabled, turn the clock off again to save power.
> + */
> + if (!pwm_is_enabled(pwm))
> + pm_runtime_put(rzv2m_pwm->chip.dev);
This is wrong for two reasons: First, you should not use pwm consumer
functions in a low level driver and second, this gives information about
the old state, I think you want:
if (!state->enabled)
pm_runtime_put(rzv2m_pwm->chip.dev);
Thinking again: rzv2m_pwm_config() is only ever called when
state->enabled is true, to this can be further simplified.
(And for the check above you want:
if (!pwm->state.enabled) {
err = pm_runtime_resume_and_get(...)
...
}
)
> + return 0;
> +}
> +
> +static int rzv2m_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct rzv2m_pwm_chip *rzv2m_pwm = to_rzv2m_pwm_chip(chip);
> + u8 prescale;
> + u64 tmp;
> + u32 val;
> +
> + pm_runtime_get_sync(chip->dev);
> + val = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCTR);
> + state->enabled = FIELD_GET(RZV2M_PWMCTR_PWME, val);
> + state->polarity = FIELD_GET(RZV2M_PWMCTR_PWMHL, val);
Please don't use a bit value as enum pwm_polarity.
Something like
state->polarity = FIELD_GET(RZV2M_PWMCTR_PWMHL, val) ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
should be identical for the compiler, but doesn't implicitly hardcode
the value of the PWM_POLARITY_* constants.
> + prescale = FIELD_GET(RZV2M_PWMCTR_PWMPS, val);
> + val = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCYC);
> + val = val ? val + 1 : 0;
oh, how unusual. Are you sure here?
> + tmp = DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)val, rzv2m_pwm->rate);
> + state->period = tmp * rzv2m_pwm_freq_div[prescale];
rate is usually 48 MHz, right? Assuming val = 1 and prescale = 3 the
actual period value is
1000000000 * 1 * 2048 / (48 MHz) ≅ 42666.6667
however your calculation yields 43008 in that case. Can you please try
to make this 42667 instead? The needed calculation is:
divroundup(NSEC_PER_SEC * (u64)val * rzv2m_pwm_freq_div[prescale], rzv2m_pwm->rate);
To prevent an overflow, we might need an uprounding variant of mul_u64_u64_div_u64().
(Mental note for myself: Update PWM_DEBUG to catch that problem.)
> + val = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMLOW);
> + val = val ? val + 1 : 0;
> + tmp = DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)val, rzv2m_pwm->rate);
> + state->duty_cycle = tmp * rzv2m_pwm_freq_div[prescale];
> + pm_runtime_put(chip->dev);
> +
> + return 0;
> +}
I'd reorder the functions to have rzv2m_pwm_config() directly before
rzv2m_pwm_apply().
> +static int rzv2m_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> [...]
> +static int rzv2m_pwm_probe(struct platform_device *pdev)
> +{
> + struct rzv2m_pwm_chip *rzv2m_pwm;
> + unsigned long apb_clk_rate;
> + int ret;
> +
> + rzv2m_pwm = devm_kzalloc(&pdev->dev, sizeof(*rzv2m_pwm), GFP_KERNEL);
> + if (!rzv2m_pwm)
> + return -ENOMEM;
> +
> + rzv2m_pwm->mmio = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(rzv2m_pwm->mmio))
> + return PTR_ERR(rzv2m_pwm->mmio);
> +
> + rzv2m_pwm->apb_clk = devm_clk_get(&pdev->dev, "apb");
> + if (IS_ERR(rzv2m_pwm->apb_clk))
> + return dev_err_probe(&pdev->dev, PTR_ERR(rzv2m_pwm->apb_clk),
> + "cannot get apb clock\n");
> +
> + apb_clk_rate = clk_get_rate(rzv2m_pwm->apb_clk);
> + if (!apb_clk_rate)
> + return dev_err_probe(&pdev->dev, -EINVAL, "apb clk rate is 0");
> +
> + rzv2m_pwm->pwm_clk = devm_clk_get(&pdev->dev, "pwm");
> + if (IS_ERR(rzv2m_pwm->pwm_clk))
> + return dev_err_probe(&pdev->dev, PTR_ERR(rzv2m_pwm->pwm_clk),
> + "cannot get pwm clock\n");
> +
> + rzv2m_pwm->rate = clk_get_rate(rzv2m_pwm->pwm_clk);
> + if (!rzv2m_pwm->rate)
> + return dev_err_probe(&pdev->dev, -EINVAL, "pwm clk rate is 0");
> +
> + /* delay = 6 * PCLK + 9 * PWM_CLK */
> + rzv2m_pwm->delay = F2CYCLE_NSEC(apb_clk_rate) * 6 +
> + F2CYCLE_NSEC(rzv2m_pwm->rate) * 9;
F2CYCLE_NSEC contains a division, so you're loosing precision here. Also
as this is a minimal delay, you might want to use up-rounding division.
> + rzv2m_pwm->rstc = devm_reset_control_get_shared(&pdev->dev, NULL);
> + if (IS_ERR(rzv2m_pwm->rstc))
> + return dev_err_probe(&pdev->dev, PTR_ERR(rzv2m_pwm->rstc),
> + "get reset failed\n");
> +
> + platform_set_drvdata(pdev, rzv2m_pwm);
> + clk_prepare_enable(rzv2m_pwm->apb_clk);
> + clk_prepare_enable(rzv2m_pwm->pwm_clk);
> + pm_runtime_set_active(&pdev->dev);
> + pm_runtime_enable(&pdev->dev);
> + ret = reset_control_deassert(rzv2m_pwm->rstc);
> + if (ret) {
> + dev_err_probe(&pdev->dev, ret,
> + "cannot deassert reset control\n");
> + goto clk_disable;
> + }
> +
> + ret = devm_add_action_or_reset(&pdev->dev,
> + rzv2m_pwm_reset_assert_pm_disable,
> + rzv2m_pwm);
> + if (ret < 0)
> + goto clk_disable;
> +
> + /*
> + * We need to keep the clock on, in case the bootloader has enabled the
> + * PWM and is running during probe().
> + */
> + if (rzv2m_pwm_is_ch_enabled(rzv2m_pwm))
> + pm_runtime_get_sync(&pdev->dev);
> +
> + rzv2m_pwm->chip.dev = &pdev->dev;
> + rzv2m_pwm->chip.ops = &rzv2m_pwm_ops;
> + rzv2m_pwm->chip.npwm = 1;
> + ret = devm_pwmchip_add(&pdev->dev, &rzv2m_pwm->chip);
> + if (ret) {
> + dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> + goto clk_disable;
If you fail here, rzv2m_pwm_reset_assert_pm_disable is called which
calls pm_runtime_set_suspended(). I assume that results in
rzv2m_pwm_pm_runtime_suspend() being called and to the clks are already
disabled?
> + }
> +
> + return 0;
> +
> +clk_disable:
> + clk_disable_unprepare(rzv2m_pwm->pwm_clk);
> + clk_disable_unprepare(rzv2m_pwm->apb_clk);
> + return ret;
> +}
> [...]
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
