Hi Uwe,
> Subject: RE: [PATCH v3 2/4] pwm: Add support for RZ/V2M PWM driver
>
> Hi Uwe,
>
> Thanks for the feedback.
>
> > Subject: Re: [PATCH v3 2/4] pwm: Add support for RZ/V2M PWM driver
> >
> > 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_share
> > > d
> > > ---
> > > 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-hardw
> > > +ar
> > > +e?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
>
> I have added below limitations now
>
> * Limitations:
> * - If the PWMLOW value is changed during PWM operation, the changed
> * value is reflected in the next PWM cycle.
> * - The duty cycle can be changed only by modifying the PWMLOW register
> * value and changing the pulse width at low level. The duty cycle becomes
> * 0% for the low width when the value of the PWMLOW register is 0x0h
> * and 100% for the low width when the value of the PWMLOW > PWMCYC.
> * - To change the setting value of the PWM cycle setting register
> * (PWMm_PWMCYC), set the PWME bit of the PWM control register
> (PWMm_PWMCTR)
> * to 0b and stop the counter operation. If it is changed during counter
> * operation, PWM output may not be performed correctly.
> * - The registers other than the PWM interrupt register (PWMINT) are always
> * synchronized with PWM_CLK at regular intervals. It takes some time
> * (Min: 2 × PCLK + 4 × PWM_CLK to Max: 6 × PCLK + 9 × PWM_CLK) for the
> * value set in the register to be reflected in the PWM circuit because
> * there is a synchronizer between the register and the PWM circuit.
>
> > > + 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?
I have restructured the probe() as below and tested various scenarios, It looks ok to me.
Please let me know, if you have any suggestions.
static int rzv2m_pwm_probe(struct platform_device *pdev)
{
...
...
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 pm_clk_disable;
}
rzv2m_pwm->chip.dev = &pdev->dev;
/*
* 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);
ret = devm_add_action_or_reset(&pdev->dev,
rzv2m_pwm_reset_assert_pm_disable,
rzv2m_pwm);
ret = -EINVAL;
if (ret < 0)
goto channel_disable;
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 channel_disable;
}
return 0;
pm_clk_disable:
pm_runtime_disable(&pdev->dev);
pm_runtime_set_suspended(&pdev->dev);
channel_disable:
if (rzv2m_pwm_is_ch_enabled(rzv2m_pwm))
rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWME, 0);
clk_disable_unprepare(rzv2m_pwm->pwm_clk);
clk_disable_unprepare(rzv2m_pwm->apb_clk);
return ret;
}
Test cases:
1) PWM-disabled by bootloader on case
----------------------------------
root@rzv2m:~# dmesg | grep pwm8
[ 3.739239] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk ON
[ 3.747804] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk OFF
root@rzv2m:~# /pwm-clk-dump.sh
Read at address 0xA3500434 (0xffffb55b6434): 0x00000801 --> BIT5 PWM8 clk
Read at address 0xA4010400 (0xffff98c3f400): 0x00000000 --> BIT2 PWM8 Channel enable
root@rzv2m:~# cd /sys/bus/platform/drivers/pwm-rzv2m
root@rzv2m:/sys/bus/platform/drivers/pwm-rzv2m# echo a4010400.pwm > unbind
[ 84.025155] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk ON
[ 84.039214] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk OFF
[ 84.060958] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk ON
[ 84.074759] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk OFF
root@rzv2m:/sys/bus/platform/drivers/pwm-rzv2m# /pwm-clk-dump.sh
Read at address 0xA3500434 (0xffffbbd5e434): 0x00000801
Read at address 0xA4010400 (0xffff8a796400): 0x00000000
root@rzv2m:/sys/bus/platform/drivers/pwm-rzv2m#
root@rzv2m:/sys/bus/platform/drivers/pwm-rzv2m# echo a4010400.pwm > bind
[ 139.273250] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk ON
[ 139.299582] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk OFF
root@rzv2m:/sys/bus/platform/drivers/pwm-rzv2m# /pwm-clk-dump.sh
Read at address 0xA3500434 (0xffffa0f28434): 0x00000801
Read at address 0xA4010400 (0xffffa1b66400): 0x00000000
root@rzv2m:/sys/bus/platform/drivers/pwm-rzv2m#
2) PWM-enabled by bootloader on case
---------------------------------
root@rzv2m:~# dmesg | grep pwm8
[ 3.738742] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk ON
root@rzv2m:~# /pwm-clk-dump.sh
Read at address 0xA3500434 (0xffffa68de434): 0x00000811
Read at address 0xA4010400 (0xffff9eaad400): 0x00000003
root@rzv2m:~# cd /sys/bus/platform/drivers/pwm-rzv2m
root@rzv2m:/sys/bus/platform/drivers/pwm-rzv2m# echo a4010400.pwm > unbind
[ 125.579970] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk OFF
root@rzv2m:/sys/bus/platform/drivers/pwm-rzv2m# /pwm-clk-dump.sh
Read at address 0xA3500434 (0xffffbb4db434): 0x00000801
Read at address 0xA4010400 (0xffffa7194400): 0x00000000
root@rzv2m:/sys/bus/platform/drivers/pwm-rzv2m# echo a4010400.pwm > bind
[ 170.837264] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk ON
[ 170.863046] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk OFF
root@rzv2m:/sys/bus/platform/drivers/pwm-rzv2m# /pwm-clk-dump.sh
Read at address 0xA3500434 (0xffff91fd4434): 0x00000801
Read at address 0xA4010400 (0xffff91010400): 0x00000000
root@rzv2m:/sys/bus/platform/drivers/pwm-rzv2m#
3) Error case 1:
---------------
ret = reset_control_deassert(rzv2m_pwm->rstc);
+ ret = -EINVAL;
PWM off/ON during boot:
[ 3.709913] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk ON
[ 3.716953] pwm-rzv2m a4010400.pwm: error -EINVAL: cannot deassert reset control
[ 3.725186] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk OFF
[ 3.732425] pwm-rzv2m: probe of a4010400.pwm failed with error -22
root@rzv2m:~# /pwm-clk-dump.sh
Read at address 0xA3500434 (0xffffa5ddc434): 0x00000801
Read at address 0xA3500614 (0xffffb5800614): 0x00000000
4) Error case 2:
---------------
ret = devm_add_action_or_reset(&pdev->dev,
rzv2m_pwm_reset_assert_pm_disable,
rzv2m_pwm);
+ ret = -EINVAL;
PWM on/off during boot:
[ 3.714182] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk ON
[ 3.721798] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk OFF
[ 3.730662] pwm-rzv2m: probe of a4010400.pwm failed with error -22
[ 3.739956] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk ON
[ 3.755238] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk OFF
root@rzv2m:~# /pwm-clk-dump.sh
Read at address 0xA3500434 (0xffff92dfc434): 0x00000801
Read at address 0xA4010400 (0xffffa7021400): 0x00000000
5) Error case 3:
----------------
ret = devm_pwmchip_add(&pdev->dev, &rzv2m_pwm->chip);
+ ret = -EINVAL;
PWM on/off during boot:
[ 3.712732] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk ON
[ 3.720408] pwm-rzv2m a4010400.pwm: error -EINVAL: failed to add PWM chip
[ 3.731991] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk OFF
[ 3.739237] pwm-rzv2m: probe of a4010400.pwm failed with error -22
[ 3.754369] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk ON
[ 3.761684] rzg2l-cpg a3500000.clock-controller: CLK_ON 1076/pwm8_clk OFF
root@rzv2m:~# /pwm-clk-dump.sh
Read at address 0xA3500434 (0xffff92dfc434): 0x00000801
Read at address 0xA4010400 (0xffffa7021400): 0x00000000
Cheers,
Biju
