Message-ID: <25d71c19-6e94-477d-8d04-758015ca4b2c@cherry.de>
Date: Mon, 15 Jul 2024 14:16:15 +0200
From: Quentin Schulz <quentin.schulz@...rry.de>
To: Uwe Kleine-König <u.kleine-koenig@...libre.com>,
 Farouk Bouabid <farouk.bouabid@...rry.de>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
 linux-pwm@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH 2/6] pwm: add mule pwm-over-i2c driver

Hi Uwe,

Answering since Farouk's on PTO.

Just to clarify, below when I say HW, I mean the actual HW, the MCU 
basically. There are two flavours of those, ATtiny816 and STM32F072CB. 
They need to be swappable, so same API. When I saw FW, I mean the 
firmware running on both MCUs (though we do have two different FW, one 
for each MCU, but they expose the same API and we expect the same 
behavior, which can be challenging).

The FW is only supposed to run on Cherry products fitted with one of 
those MCUs, we do not plan on selling them separately or releasing the 
FW for consumption in other devices. As such, there's no need on our 
side for public documentation. I'll try to answer the FW questions to 
the best of my ability though.

On 7/12/24 10:54 AM, Uwe Kleine-König wrote:
> Hello,
> 
> On Wed, May 29, 2024 at 12:10:31PM +0200, Farouk Bouabid wrote:
>> Mule is a device that can output a PWM signal based on I2C commands.
>>
>> Add pwm driver for Mule PWM-over-I2C controller.
>>
>> Signed-off-by: Farouk Bouabid <farouk.bouabid@...rry.de>
>> ---
>>   drivers/pwm/Kconfig    |  10 +++++
>>   drivers/pwm/Makefile   |   1 +
>>   drivers/pwm/pwm-mule.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 126 insertions(+)
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 4b956d661755..eb8cfa113ec7 100644
>> --- 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?

[...]

>> 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?

> 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 :)

>> +	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!

The suggested implementation would then be:

guard(mutex)(&priv->lock);

I believe.

>> +	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.

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.

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?

Cheers,
Quentin

Powered by blists - more mailing lists