Message-ID: <20210504171552.uccctqzbcffiqhu7@pengutronix.de>
Date:   Tue, 4 May 2021 19:15:52 +0200
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Sean Anderson <sean.anderson@...o.com>
Cc:     linux-pwm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        michal.simek@...inx.com, Lee Jones <lee.jones@...aro.org>,
        Thierry Reding <thierry.reding@...il.com>
Subject: Re: [PATCH 2/2] pwm: Add support for Xilinx AXI Timer

Hello Sean,

On Tue, May 04, 2021 at 10:46:39AM -0400, Sean Anderson wrote:
> On 5/4/21 4:51 AM, Uwe Kleine-König wrote:
> > On Mon, May 03, 2021 at 05:44:13PM -0400, Sean Anderson wrote:
> >> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
> >> found on Xilinx FPGAs. There is another driver for this device located
> >> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping.
> >> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
> >>
> >> [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
> >>
> >> Signed-off-by: Sean Anderson <sean.anderson@...o.com>
> >> ---
> >>
> >>   arch/arm64/configs/defconfig |   1 +
> >>   drivers/pwm/Kconfig          |  11 ++
> >>   drivers/pwm/Makefile         |   1 +
> >>   drivers/pwm/pwm-xilinx.c     | 322 +++++++++++++++++++++++++++++++++++
> >>   4 files changed, 335 insertions(+)
> >>   create mode 100644 drivers/pwm/pwm-xilinx.c
> >>
> >> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> >> index 08c6f769df9a..81794209f287 100644
> >> --- a/arch/arm64/configs/defconfig
> >> +++ b/arch/arm64/configs/defconfig
> >> @@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y
> >>   CONFIG_PWM_SL28CPLD=m
> >>   CONFIG_PWM_SUN4I=m
> >>   CONFIG_PWM_TEGRA=m
> >> +CONFIG_PWM_XILINX=m
> >>   CONFIG_SL28CPLD_INTC=y
> >>   CONFIG_QCOM_PDC=y
> >>   CONFIG_RESET_IMX7=y
> >
> > I think this should go into a separate patch once this driver is
> > accepted. This can then go via the ARM people.
> 
> Sure.
> 
> >
> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >> index d3371ac7b871..01e62928f4bf 100644
> >> --- a/drivers/pwm/Kconfig
> >> +++ b/drivers/pwm/Kconfig
> >> @@ -628,4 +628,15 @@ config PWM_VT8500
> >>   	  To compile this driver as a module, choose M here: the module
> >>   	  will be called pwm-vt8500.
> >>
> >> +config PWM_XILINX
> >> +	tristate "Xilinx AXI Timer PWM support"
> >> +	depends on !MICROBLAZE
> >
> > I don't understand this dependency.
> 
> As noted in the commit message, there is already a driver for this
> device for microblaze systems. To prevent conflicts, this driver is
> disabled on microblaze.

OK, already understood that after reading the other replies to this
thread. As noted there, please add a comment describing the problem. You
probably also need some more dependencies, at least COMMON_CLK for
clk_rate_exclusive_get() and probably also HAS_IOMEM for readl/writel.

> >> +#include <asm/io.h>
> >> +#include <linux/clk.h>
> >> +#include <linux/device.h>
> >> +#include <linux/debugfs.h>
> >> +#include <linux/module.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/pwm.h>
> >> +
> >> +#define TCSR0	0x00
> >> +#define TLR0	0x04
> >> +#define TCR0	0x08
> >> +#define TCSR1	0x10
> >> +#define TLR1	0x14
> >> +#define TCR1	0x18
> >> +
> >> +#define TCSR_MDT	BIT(0)
> >> +#define TCSR_UDT	BIT(1)
> >> +#define TCSR_GENT	BIT(2)
> >> +#define TCSR_CAPT	BIT(3)
> >> +#define TCSR_ARHT	BIT(4)
> >> +#define TCSR_LOAD	BIT(5)
> >> +#define TCSR_ENIT	BIT(6)
> >> +#define TCSR_ENT	BIT(7)
> >> +#define TCSR_TINT	BIT(8)
> >> +#define TCSR_PWMA	BIT(9)
> >> +#define TCSR_ENALL	BIT(10)
> >> +#define TCSR_CASC	BIT(11)
> >
> > I'd like to see a prefix for these defines, something like XILINX_PWM_
> > maybe?
> 
> I don't think that's necessary, since these defines are used only for
> this file. In particular, adding a prefix like that would significantly
> lengthen lines which add several flags.

This is something where Thierry and I don't agree, so you can at least
expect support for your point. Still I see a value that is usually worth
the additional horizontal space.

The prefix would make it obvious that this is a local symbol. It also
helps to jump to the right definition if you use ctags or something
similar.

The most affected code lines are probably:

+	return !(~tcsr0 & TCSR_SET || tcsr0 & (TCSR_CLEAR | TCSR_CASC) ||
+		 ~tcsr1 & TCSR_SET || tcsr1 & TCSR_CLEAR);

which IMHO is already hard to read and adding a newline after each ||
doesn't hurt.

Or write it as:

	#define XILINX_PWM_TCSR_RUN_SET			\
			(XILINX_PWM_TCSR_GENT |		\
			 XILINX_PWM_TCSR_ARHT |		\
			 XILINX_PWM_TCSR_ENT |		\
			 XILINX_PWM_TCSR_PWMA)

	#define XILINX_PWM_TCSR_RUN_CLEAR		\
			 (XILINX_PWM_TCSR_MDT | \
			 XILINX_PWM_TCSR_LOAD)

	#define XILINX_PWM_TCSR_RUN_MASK		\
			(XILINX_PWM_TCSR_RUN_SET | XILINX_PWM_TCSR_RUN_CLEAR)


	...

	/*
	 * Both TCSR registers must be setup according to the running
	 * value, TCSR0 must also have the CASC bit set.
	 */
	if ((tcsr0 & XILINX_PWM_TCSR_RUN_MASK) != XILINX_PWM_TCSR_RUN_SET ||
	    !(tcsr0 & TCSR_CASC) ||
	    (tcsr1 & XILINX_PWM_TCSR_RUN_MASK) != XILINX_PWM_TCSR_RUN_SET)

which IMHO is better understandable and still fits the line length.
(Or simplify the check to only test for a single bit?)

+			tcsr0 = (TCSR_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
+			tcsr1 = TCSR_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);

is similar. I wonder if it makes sense to drop ENT from the default bit
mask and if after that xilinx_pwm_is_enabled() yields true. I also don't
understand the semantic of the UDT bit. You never modify it. Assuming it
has a relevant meaning the driver's behaviour depends on the value the
bootloader initialized that bit to?

So these lines might need some explanation anyhow and having to write it
over several lines doesn't actually hurt.

	bool enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0),
					     readl(pwm->regs + TCSR1));

could be rewritten as

	u32 tcsr0 = readl(pwm->regs + XILINX_PWM_TCSR0);
	u32 tcsr1 = readl(pwm->regs + XILINX_PWM_TCSR1);
	bool enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1);

which IMHO reads better and the required horizontal space is even less
than with your lines even tough I have added a prefix.

> >> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
> >> +			    const struct pwm_state *state)
> >> +{
> >> +	struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip);
> >> +	u32 tlr0, tlr1;
> >> +	u32 tcsr0 = readl(pwm->regs + TCSR0);
> >> +	u32 tcsr1 = readl(pwm->regs + TCSR1);
> >> +	bool enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1);
> >> +
> >> +	if (state->polarity != PWM_POLARITY_NORMAL)
> >> +		return -EINVAL;
> >> +
> >> +	if (!enabled && state->enabled)
> >> +		clk_rate_exclusive_get(pwm->clk);
> >
> > Missing error checking.
> 
> This cannot fail.

Oh indeed. IMHO it should be changed to return void and the comment
"Returns 0 on success, -EERROR otherwise" should be adapted to reality.

> >> +	tlr1 = xilinx_pwm_calc_tlr(pwm, tcsr1, state->duty_cycle);
> >> +	writel(tlr0, pwm->regs + TLR0);
> >> +	writel(tlr1, pwm->regs + TLR1);
> >
> > How does the hardware behave here? E.g. can it happen that after writing
> > TLR0 we can see a cycle with the new period but the old duty_cycle? Does
> > it complete the currently running period?
> 
> TLR is the value loaded into TCR when the counter resets. I believe this
> happens at the rising edge for TCR0 and at the falling edge for TCR1,
> but the exact behavior is not documented. Therefore, changing the value
> of TLR will not affect the current cycle.

So we might still get a mixed period if the counter resets between the
two writel commands, right?

> > If state->enabled == 0 you want to disable first to prevent that the
> > (maybe) new values for period and duty_cycle are emitted by the
> > hardware.
> >
> >> +	if (state->enabled) {
> >> +		/* Only touch the TCSRs if we aren't already running */
> >> +		if (!enabled) {
> >> +			/* Load TLR into TCR */
> >> +			writel(tcsr0 | TCSR_LOAD, pwm->regs + TCSR0);
> >> +			writel(tcsr1 | TCSR_LOAD, pwm->regs + TCSR1);
> >> +			/* Enable timers all at once with ENALL */
> >> +			tcsr0 = (TCSR_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
> >> +			tcsr1 = TCSR_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
> >> +			writel(tcsr0, pwm->regs + TCSR0);
> >> +			writel(tcsr1, pwm->regs + TCSR1);
> >> +		}
> >> +	} else {
> >> +		writel(tcsr0 & ~TCSR_ENT, pwm->regs + TCSR0);
> >> +		writel(tcsr1 & ~TCSR_ENT, pwm->regs + TCSR1);
> >
> > How does the hardware behave on disable? Does it always emit a low
> > level? Does it complete the currently running period?
> 
> I don't know. My suspicion is that the output stops at whatever its
> current value is.

Would be great if you could find out that. Please document that in the
Limitations paragraph at the top of the driver.
 
> > This is unusual to have in a driver. Something like memtool[1] should
> > work just fine, then you don't need this debugfs file. Together with pwm
> > tracing this should be good enough.
> 
> I didn't have memtool on my test system, so I added this. I can remove
> it from the next patch if you'd like.

Yes, please remove it. (And install memtool or one of its alternatives
:-).

> >> +	pwm->clk = devm_clk_get(dev, NULL);
> >> +	if (IS_ERR(pwm->clk))
> >> +		return dev_err_probe(dev, PTR_ERR(pwm->clk), "missing clock\n");
> >> +
> >> +	ret = clk_prepare_enable(pwm->clk);
> >> +	if (ret) {
> >> +		dev_err(dev, "clock enable failed\n");
> >> +		return ret;
> >
> > You can use dev_err_probe here, too, for consistency.
> 
> Ok. Though in this case we will never get -EPROBE_DEFER from
> clk_prepare_enable, so dev_err_probe is not as useful.

Everything it does here is useful, and the overhead is only the check
against -EPROBE_DEFER. In return you get consistent error handling and
the messages are also all in the same format. (And compared to your
version the error message also gives a hint about the actual problem as
it emits the error code.)

> >> +static int xilinx_pwm_remove(struct platform_device *pdev)
> >> +{
> >> +	struct xilinx_pwm_device *pwm = platform_get_drvdata(pdev);
> >> +	bool enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0),
> >> +					     readl(pwm->regs + TCSR1));
> >> +
> >> +	if (enabled)
> >> +		clk_rate_exclusive_put(pwm->clk);

Note that I misunderstood what clk_rate_exclusive_put does. This only
drops the lock on the clock that prevents that others modify the clock
rate. So that call is fine.

> > This looks wrong. You should rely on the consumer that they disable the
> > PWM.
> 
> What about a PWM regulator with always-on?

So in general: What do you think should happen? When the remove function
is called the PWM regulator driver is already gone, so there is no way
to get the information that the PWM should stay on ...
 
> >> +	clk_disable_unprepare(pwm->clk);

... and still you stop the PWM here (or does that only affect access to
its registers?)

So it's a delicate topic and there isn't the single right thing to do
here. So usually I request to free all resources allocated in .probe()
and otherwise leave the hardware alone. Now that I understood
clk_rate_exclusive_put() the status quo is fine for me. Just call
pwmchip_remove first and only then release the resources needed to
operate the PWM.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists