Hello Quentin,
On Mon, Jul 15, 2024 at 02:16:15PM +0200, Quentin Schulz wrote:
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -425,6 +425,16 @@ config PWM_MICROCHIP_CORE
> > > To compile this driver as a module, choose M here: the module
> > > will be called pwm-microchip-core.
> > > +config PWM_MULE
> > > + tristate "Mule PWM-over-I2C support"
> > > + depends on I2C && OF
> >
> > It would be easy to drop the hard dependency on OF. Please do that.
> >
>
> Just being curious here, what would be the benefit?
Increasing easy compile coverage.
> [...]
>
> > > diff --git a/drivers/pwm/pwm-mule.c b/drivers/pwm/pwm-mule.c
> > > new file mode 100644
> > > index 000000000000..e8593a48b16e
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-mule.c
> > > @@ -0,0 +1,115 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Mule PWM-over-I2C controller driver
> > > + *
> > > + * Copyright (C) 2024 Theobroma Systems Design und Consulting GmbH
> >
> > Is there a publicly available datasheet? I guess not. (I ask because
> > adding a link there to such a document would be nice.)
> >
>
> Unfortunately no. It's also only part of our product line and there's no
> plan to start selling it standalone or selling the IP.
>
> > > + */
> > > +
> > > +#include <linux/i2c.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/pwm.h>
> > > +#include <linux/regmap.h>
> > > +
> > > +struct mule_pwm {
> > > + struct mutex lock;
> > > + struct regmap *regmap;
> > > +};
> > > +
> > > +static const struct regmap_config pwm_mule_config = {
> > > + .reg_bits = 8,
> > > + .val_bits = 8,
> > > +};
> > > +
> > > +#define MULE_PWM_DCY_REG 0x0
> > > +#define MULE_PWM_FREQ_L_REG 0x1 /* LSB register */
> > > +#define MULE_PWM_FREQ_H_REG 0x2 /* MSB register */
> > > +
> > > +#define NANOSECONDS_TO_HZ(x) (1000000000UL/(x))
> >
> > Don't introduce such a macro if you only use it once. Having the
> > division in the function results in code that is easier to read (IMHO).
> >
> > > +static int pwm_mule_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > + const struct pwm_state *state)
> > > +{
> > > + struct mule_pwm *priv = pwmchip_get_drvdata(chip);
> > > + u8 duty_cycle;
> > > + u64 freq;
> > > + int ret;
> > > +
> > > + freq = NANOSECONDS_TO_HZ(state->period);
> > > +
> > > + if (freq > U16_MAX) /* Frequency is 16-bit wide */ {
> > > + dev_err(chip->dev,
> > > + "Failed to set frequency: %llu Hz: out of 16-bit range\n", freq);
> > > + return -EINVAL;
> > > + }
> >
> > You're supposed to configure the biggest possible period not bigger than
> > the requested period. So this should be:
> >
> > /*
> > * The period is configured using a 16 bit wide register holding
> > * the frequency in Hz.
> > */
> > unsigned int period = min_t(u64, state->period, NSEC_PER_SEC);
> > unsigned int freq = DIV_ROUND_UP(NSEC_PER_SEC, period);
> >
> > if (freq > U16_MAX)
> > return -EINVAL;
> >
> > > + if (state->enabled)
> > > + duty_cycle = pwm_get_relative_duty_cycle(state, 100);
> >
> > This is wrong for two reasons:
> >
> > - It uses rounding to the nearest duty_cycle, however you're supposed
> > to round down.
> > - It uses the requested period instead of the real one.
> >
>
> I assume you want:
>
> unsigned int real_period = ((u64) NSEC_PER_SEC * 100) / freq;
>
> which rounds down?
Yes. And then to calculate the duty_cycle setting use real_period.
> > I wonder why the hardware doesn't use the whole 8 bits here.
> >
>
> It's even a 16b register that the HW uses. I guess we just went with the
> most human-friendly API :) I believe it's something we should be able to
> change in the FW before releasing if this is something that really makes
> sense. FYI, the register stores the number of clock ticks for the signal to
> be high, once reached, put it low (or the opposite). So it's necessarily a
> fraction of the clock frequency. 100% was easy because we know that every
> clock frequency we support is a multiple of 100 so there's no issue around
> rounding for example since we definitely do not want to do float maths in
> MCUs :)
Interesting perspective. I'd still go for using the register completely.
> > > + else
> > > + duty_cycle = 0;
> > > +
> > > + mutex_lock(&priv->lock);
> >
> > If you use the guard helper, you don't need to resort to goto for error
> > handling.
> >
>
> I would have liked a link or more precise hint at what this "guard helper"
> was, but found something myself so here it is for anyone wondering:
>
> https://lwn.net/Articles/934679/
>
> Had never heard of that one before, neat!
Right. A conversion example is available at
https://lore.kernel.org/linux-pwm/2102fe8189bdf1f02ff3785b551a69be27a65af4.1719520143.git.u.kleine-koenig@xxxxxxxxxxxx/
> > > + ret = regmap_bulk_write(priv->regmap, MULE_PWM_FREQ_L_REG, &freq, 2);
> > > + if (ret) {
> > > + dev_err(chip->dev,
> > > + "Failed to set frequency: %llu Hz: %d\n", freq, ret);
> > > + goto out;
> > > + }
> > > +
> > > + ret = regmap_write(priv->regmap, MULE_PWM_DCY_REG, duty_cycle);
> > > + if (ret)
> > > + dev_err(chip->dev,
> > > + "Failed to set duty cycle: %u: %d\n", duty_cycle, ret);
> >
> > Please document how the hardware behaves here in a "Limitations" section
> > as several other drivers do. Questions to answer include: Does it
> > complete a period when the parameters are updated? Can it happen that a
> > glitch is emitted while MULE_PWM_FREQ_[LH]_REG is updated but
> > MULE_PWM_DCY_REG isn't yet? Maybe updating MULE_PWM_FREQ_[LH]_REG isn't
> > even atomic? "Doesn't support disabling, configures duty_cycle=0 when
> > disabled is requested."
> >
>
> Updating MULE_PWM_FREQ_[LH]_REG is atomic (L is stored in SRAM until H reg
> is written, when LH are then written to the hardware IP).
>
> We use double-buffering (supported by the HW directly) for the period and
> comparator registers. I believe we still have a possible glitch during a
> small time-frame in the current version of the FW. Basically, trying to
> change the period AND duty cycle at the same time, the following could
> happen:
>
> - period A + duty-cycle AA
> - period B + duty-cycle AA
> - period B + duty-cycle BB
>
> Depending on what we consider a glitch, the second element in the list could
> be one. Even if we do a multibyte write to the actual HW, I'm not sure if
> this window can be eliminated.
I'd call that a glitch, yes.
> To give a bit more info on this, there are two possible flavors of the MCU,
> ATtiny 816 (datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/ATtiny416-816-DataSheet-DS40001913B.pdf)
> and STM32F072CB (datasheet: https://www.st.com/content/ccc/resource/technical/document/reference_manual/c2/f8/8a/f2/18/e6/43/96/DM00031936.pdf/files/DM00031936.pdf/jcr:content/translations/en.DM00031936.pdf).
>
> FYI, on ATtiny, we use TCA in single-slope PWM generation mode and PERBUF
> and CMP2BUF as period and duty-cycle registers. On STM32, we use TIM15 in
> PWM mode and ARR and CCR1 as period and duty-cycle registers.
Wouldn't it be more natural with these to have duty in a base-2 register
for duty, in the assumption that your MCUs habe this, too?
> Re-reading both datasheets, and if I understand correctly, we could have
> glitch-free transitions by controlling the ARPE bit on STM32 and LUPD bit on
> ATtiny816.
>
> @Farouk, please confirm but the above makes sense to me and I guess we could
> implement something in the FW. The question is how to detect if we want to
> change both the duty-cycle and period at the same time (we could decide to
> document this as a requirement for the API user though: "changes to
> MULE_PWM_FREQ_[LH]_REG are only applied when MULE_PWM_DCY_REG is written
> to").
>
> > Maybe write all three registers in a bulk write, then you might even be
> > able to drop the lock.
> >
>
> The bulk write wouldn't help with the glitch, but it's a good idea for
> getting rid of the lock.
>
> > Also please fail silently.
>
> Would dev_dbg() be fine here or would you rather see them gone entirely?
dev_dbg is silent by default, so that's fine for me.
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature
