RE: [PATCH v12 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM driver

From: Biju Das <biju.das.jz@bp.renesas.com>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Thierry Reding <thierry.reding@gmail.com>,
	"linux-pwm@vger.kernel.org" <linux-pwm@vger.kernel.org>,
	Lee Jones <lee@kernel.org>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	William Breathitt Gray <william.gray@linaro.org>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Chris Paterson <Chris.Paterson2@renesas.com>,
	Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	"linux-renesas-soc@vger.kernel.org" 
	<linux-renesas-soc@vger.kernel.org>
Subject: RE: [PATCH v12 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM driver
Date: Wed, 15 Feb 2023 10:31:20 +0000	[thread overview]
Message-ID: <OS0PR01MB59227E0E659C292D775474BF86A39@OS0PR01MB5922.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <20230215083037.ivlmqhp4xngnexca@pengutronix.de>

Hi Uwe,

Thanks for the feedback.

> Subject: Re: [PATCH v12 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM driver
> 
> Hello,
> 
> I'm working on my review backlog, sorry that it took so long.

OK.

> 
> On Thu, Feb 02, 2023 at 04:57:32PM +0000, Biju Das wrote:
> > Add support for RZ/G2L MTU3a PWM driver. The IP supports following PWM
> > modes
> >
> > 1) PWM mode{1,2}
> > 2) Reset-synchronized PWM mode
> > 3) Complementary PWM mode{1,2,3}
> 
> It's unclear to me what "PWM mode1" and the other modes are. I suspect this
> is some chip specific naming that isn't understandable for outsiders? Would
> be great to explain that a bit more.

I will give some details about PWM modes mentioned in the HW manual here.
I will respond to other comments later.

PWM Mode 1
------------
n = {0,1,2,3,4,6,7}
MTIOC0A:-MTU0 TGRA input capture input/output compare output/PWM output pin
TGRA: Timer General Register A
TIOR: Timer I/O control register
In PWM mode 1, PWM waveforms in up to 12 phases can be output

PWM waveforms are output from the MTIOCnA and MTIOCnC pins by pairing TGRA
with TGRB and TGRC with TGRD. The levels specified by the TIOR.IOA[3:0] and
IOC[3:0] bits are output from the MTIOCnA and MTIOCnC pins at compare matches
A and C, and the level specified by the TIOR.IOB[3:0] and IOD[3:0] bits are
output at compare matches B and D (n = 0 to 4, 6, 7). The initial output value
is set in TGRA or TGRC. If the values set in paired TGRs are identical, the
output value does not change even when a compare match occurs.

PWM Mode 2
----------
n = {0,1,2}

PWM waveform output is generated using one TGR as the cycle register and the
others as duty registers. The level specified in TIOR is output at compare matches.
Upon counter clearing by a cycle register compare match, the initial value set
in TIOR is output from each pin. If the values set in the cycle and duty registers
are identical, the output value does not change even when a compare match occurs.

In PWM mode 2, up to eight phases of PWM waveforms can be output when synchronous
clearing is used as synchronous operation in the channels that cannot be placed in
PWM mode 2.

Reset-Synchronized PWM Mode:
---------------------------
In the reset-synchronized PWM mode, three phases of positive and negative PWM
waveforms (six phases in total) that share a common wave transition point can
be output by combining MTU3 and MTU4 and MTU6 and MTU7.

When set for reset-synchronized PWM mode, the MTIOC3B, MTIOC3D, MTIOC4A, MTIOC4C,
MTIOC4B, MTIOC4D, MTIOC6B, MTIOC6D, MTIOC7A, MTIOC7C, MTIOC7B, and MTIOC7D pins
function as PWM output pins and timer counters 6 and 12 (MTU3.TCNT and MTU6.TCNT)
functions as an up-counter

Complementary PWM Mode:
----------------------

In complementary PWM mode, dead time can be set for PWM waveforms to be output.
The dead time is the period during which the upper and lower arm transistors are
set to the inactive level in order to prevent short-circuiting of the arms.
Six positive-phase and six negative-phase PWM waveforms (12 phases in total) 
with dead time can be output by combining MTU3/ MTU4 and MTU6/MTU7. PWM waveforms
without dead time can also be output.

In complementary PWM mode, nine registers (compare registers, buffer registers,
and temporary registers) are used to control the duty ratio for the PWM output.

Complementary PWM mode 1 (transfer at crest)
Complementary PWM mode 2 (transfer at trough)
Complementary PWM mode 3 (transfer at crest and trough)

Note:
I will respond to other comments later.

Cheers,
Biju

> 
> > This patch adds basic pwm mode 1 support for RZ/G2L MTU3a pwm driver
> > by creating separate logical channels for each IOs.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v11->v12:
> >  * Updated header file to <linux/mfd/rz-mtu3.h> as core driver is in MFD.
> >  * Reordered get_state()
> > v10->v11:
> >  * No change.
> > v9->v10:
> >  * No change.
> > v8->v9:
> >  * Added prescale/duty_cycle variables to struct rz_mtu3_pwm_chip and
> >    cached this values in rz_mtu3_pwm_config and used this cached values
> >    in get_state(), if PWM is disabled.
> >  * Added return code for get_state()
> > v7->v8:
> >  * Simplified rz_mtu3_pwm_request by calling rz_mtu3_request_channel()
> >  * Simplified rz_mtu3_pwm_free by calling rz_mtu3_release_channel()
> > v6->v7:
> >  * Added channel specific mutex lock to avoid race between counter
> >    device and rz_mtu3_pwm_{request,free}
> >  * Added pm_runtime_resume_and_get in rz_mtu3_pwm_enable()
> >  * Added pm_runtime_put_sync in rz_mtu3_pwm_disable()
> >  * Updated rz_mtu3_pwm_config()
> >  * Updated rz_mtu3_pwm_apply()
> > v5->v6:
> >  * Updated commit and Kconfig description
> >  * Sorted the header
> >  * Replaced dev_get_drvdata from rz_mtu3_pwm_pm_disable()
> >  * Replaced SET_RUNTIME_PM_OPS->DEFINE_RUNTIME_DEV_PM_OPS and removed
> >    __maybe_unused from suspend/resume()
> > v4->v5:
> >  * pwm device is instantiated by mtu3a core driver.
> > v3->v4:
> >  * There is no resource associated with "rz-mtu3-pwm" compatible
> >    and moved the code to mfd subsystem as it binds against "rz-mtu".
> >  * Removed struct platform_driver rz_mtu3_pwm_driver.
> > v2->v3:
> >  * No change.
> > v1->v2:
> >  * Modelled as a single PWM device handling multiple channles.
> >  * Used PM framework to manage the clocks.
> > ---
> >  drivers/pwm/Kconfig       |  11 +
> >  drivers/pwm/Makefile      |   1 +
> >  drivers/pwm/pwm-rz-mtu3.c | 485
> > ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 497 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-rz-mtu3.c
> >