Message-ID: <20210428154018.fiknoxraymestnnj@pengutronix.de>
Date:   Wed, 28 Apr 2021 17:40:18 +0200
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     fenglinw@...eaurora.org
Cc:     linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Thierry Reding <thierry.reding@...il.com>,
        Lee Jones <lee.jones@...aro.org>, linux-pwm@...r.kernel.org,
        subbaram@...eaurora.org, collinsd@...eaurora.org,
        aghayal@...eaurora.org
Subject: Re: [PATCH 2/2] pwm: pwm-qcom: add driver for PWM modules in QCOM
 PMICs

Hello,

On Wed, Apr 28, 2021 at 08:42:58PM +0800, fenglinw@...eaurora.org wrote:
> On 2021-04-28 01:07, Uwe Kleine-König wrote:
> > On Tue, Apr 27, 2021 at 06:22:10PM +0800, Fenglin Wu wrote:
> >  - Does the hardware complete the currently running period when the PWM
> >    is disabled?
>
> No, the output stops immediately as soon as the PWM channel is disabled

Is this only because you clear PWM_EN_GLITCH_REMOVAL on disable?

> >  - Does the output pin pull to the inactive level when the PWM is
> >    disabled?
>
> Actually, the QCOM PMIC PWM module doesn't have physical pin out. Its output
> is normally connected to other hardware module in the same PMIC as internal
> signals, such as: control signal for LED module for scaling LED brightness,
> input signal for vibrator module for vibration strength control. It's output
> can also be routed through PMIC GPIO or other pin using internal DTEST
> lines, and that depends on HW connection, and it will also need addition
> configuration in the GPIO module or the DTEST and that's outside of the
> PWM module scope.
>
> For the output signal itself, it's always inactive when the PWM module
> is disabled.

Inactive as in "outputs a 0" and not as in "driver is disabled", right?
(Not sure this is a well defined term given that the output isn't
normally externally visible.)

> > > +static int qcom_pwm_channel_set_glitch_removal(
> > > +		struct qcom_pwm_channel *pwm, bool en)
> > > +{
> > > +	u8 mask, val;
> > > +
> > > +	val = en ? PWM_EN_GLITCH_REMOVAL_MASK : 0;
> > > +	mask = PWM_EN_GLITCH_REMOVAL_MASK;
> > > +	return qcom_pwm_channel_masked_write(pwm,
> > > +			REG_PWM_TYPE_CONFIG, mask, val);
> >
> > What is the effect of this setting?
>
> As I explained at the beginning, enable this setting would garantee the PWM
> module complete current period before swtiching to the new settings.

Then there is no reason to unset this bit when the PWM is disabled and
the setting can better be done once in .probe()?

> > > +static void __qcom_pwm_calc_pwm_period(u64 period_ns,
> > > +			struct qcom_pwm_config *pwm_config)
> > > +{
> > > +	struct qcom_pwm_config configs[NUM_PWM_SIZE];
> > > +	int i, j, m, n;
> > > +	u64 tmp1, tmp2;
> > > +	u64 clk_period_ns = 0, pwm_clk_period_ns;
> > > +	u64 clk_delta_ns = U64_MAX, min_clk_delta_ns = U64_MAX;
> > > +	u64 pwm_period_delta = U64_MAX, min_pwm_period_delta = U64_MAX;
> > > +	int pwm_size_step;
> > > +
> > > +	/*
> > > +	 *              (2^pwm_size) * (2^pwm_exp) * prediv * NSEC_PER_SEC
> > > +	 * pwm_period = ---------------------------------------------------
> > > +	 *                               clk_freq_hz
> > > +	 *
> > > +	 * Searching the closest settings for the requested PWM period.
> >
> > Please don't pick the nearest setting, but the next smallest one.
>
> I am not quite sure here. You can see from the equation above, there are 4
> settings can impact PWM period calculation and each setting has an array of
> values. We can't easily sort out the sequence of settings to achieve an
> ascending
> or descending PWM periods and choose the closest one or the next smallest
> one,
> instead, the logic below is to walk through all of the settings and find the
> closest one.
> I am also confused about not choosing the nearest settings but the
> next smallest one, let's say if we are trying to achieve 1ms PWM period, and
> there are three settings can get 0.90ms, 0.99ms, 1.05ms respectively
> should we choose 0.99ms which is the closest one, or 1.05ms which is the
> next
> smallest one?

My wording was bad, you should pick the biggest period not bigger than
the requested period. So in your example you should pick 0.99ms.

And if your options are 0.90ms and 1.01 ms and the request is for 1 ms,
pick 0.90ms. I'm working on a series that allows a consumer to make an
informed choice. One precondition for that is that .apply picks a
setting according to the above algorithm though.

> > > +	 */
> > > +
> > > +	for (n = 0; n < ARRAY_SIZE(pwm_size); n++) {
> > > +		pwm_clk_period_ns = period_ns >> pwm_size[n];
> > > +		for (i = ARRAY_SIZE(clk_freq_hz) - 1; i >= 0; i--) {
> > > +			for (j = 0; j < ARRAY_SIZE(clk_prediv); j++) {
> > > +				for (m = 0; m < ARRAY_SIZE(pwm_exponent); m++) {
> > > +					tmp1 = 1 << pwm_exponent[m];
> > > +					tmp1 *= clk_prediv[j];
> > > +					tmp2 = NSEC_PER_SEC;
> > > +					do_div(tmp2, clk_freq_hz[i]);
> > > +
> > > +					clk_period_ns = tmp1 * tmp2;
> > > +
> > > +					clk_delta_ns = abs(pwm_clk_period_ns
> > > +						- clk_period_ns);
> > > +					/*
> > > +					 * Find the closest setting for
> > > +					 * PWM frequency predivide value
> > > +					 */
> > > +					if (clk_delta_ns < min_clk_delta_ns) {
> > > +						min_clk_delta_ns
> > > +							= clk_delta_ns;
> > > +						configs[n].pwm_clk
> > > +							= clk_freq_hz[i];
> > > +						configs[n].prediv
> > > +							= clk_prediv[j];
> > > +						configs[n].clk_exp
> > > +							= pwm_exponent[m];
> > > +						configs[n].pwm_size
> > > +							= pwm_size[n];
> > > +						configs[n].best_period_ns
> > > +							= clk_period_ns;
> > > +					}
> > > +				}
> > > +			}
> > > +		}
> > > +
> > > +		configs[n].best_period_ns *= 1 << pwm_size[n];
> > > +		/* Find the closest setting for PWM period */
> > > +		pwm_period_delta = min_clk_delta_ns << pwm_size[n];
> > > +		if (pwm_period_delta < min_pwm_period_delta) {
> > > +			min_pwm_period_delta = pwm_period_delta;
> > > +			memcpy(pwm_config, &configs[n],
> > > +					sizeof(struct qcom_pwm_config));
> > > +		}
> > > +	}
> >
> > Huh, this is complicated. It would be great if this could be simplified.
> > If you provide a reference manual or at least a .get_state function, I
> > can try to advise.
>
> Hmm, I am not sure how to describe the HW implementation here, but as I
> explained, there are four parameters impacting the PWM period calculation
> with different way, so the code logic here is trying to walk through all
> of the the settings and find the one which can achieve the closest PWM
> period.

OK, so we have:

              (2^pwm_size) * (2^pwm_exp) * prediv * NSEC_PER_SEC
 pwm_period = ---------------------------------------------------
                               clk_freq_hz

with pwm_size being either 6 or 9, pwm_exp is an integer in the range
[0..7], prediv is one of 1, 3, 5, or 6 and clk_freq_hz is one of 1024,
32768 or 19200000, right? And picking 9 for pwm_size has the advantage
that the duty-cycle setting has a finer resolution, right?

(BTW, I wonder about the choice for prediv, one of the set {1, 3, 5, 7}
would make more sense in my eyes and additionally it might even be a tad
cheaper to implement in hardware.)

This is indeed a strange formula that hardly allows to implement the
picking of parameters in a clever way.

I wonder if the hardware can emit a 100% duty cycle.

> > > +	for (i = 0; i < chip->num_channels; i++) {
> > > +		chip->pwms[i].chip = chip;
> > > +		chip->pwms[i].chan_idx = i;
> > > +		chip->pwms[i].reg_base = base + i * REG_SIZE_PER_CHANN;
> > > +		rc = qcom_pwm_channel_read(&chip->pwms[i], REG_PERPH_SUBTYPE,
> > > +				&chip->pwms[i].subtype);
> >
> > Can a single device have channels with different sub-types?
>
> Hmm, it has the possibility. Normally, in one PMIC, all PWM modules should
> have the same sub-type of PWM modules. But since each PWM module is accessed
> independantly, so we will need to check the sub-type here for each PWM
> module.

But if all channels are known to have the same subtype, you don't need
to test them all individually and a single member in qcom_pwm_chip
indicating the type would be enough.

> > > +		if (rc < 0) {
> > > +			dev_err(chip->dev, "Read PWM channel %d subtype failed, rc=%d\n",
> > > +					i, rc);
> > > +			return rc;
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int qcom_pwm_probe(struct platform_device *pdev)
> > > +{
> > > +	int rc;
> > > +	struct qcom_pwm_chip *chip;
> > > +
> > > +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> > > +	if (!chip)
> > > +		return -ENOMEM;
> >
> > If you parse qcom,num-channels already before allocating driver data you
> > can allocate driver and per channel data in a single chunk, making some
> > operations simpler and maybe even save some memory.
>
> In a single chunk do you mean by calling devm_zalloc() once?
> Can you let me know how to do that? The per channel data is anothe pointer
> which is different from the driver data, how can we make sure two different
> pointers can be allocated in the same chunk of memory?

You can do the following:

	struct qcom_pwm_channel {
		...
	};

	struct qcom_pwm_chip {
		...
		struct qcom_pwm_channel channel[];
	};

	qc = devm_kzalloc(&pdev->dev, sizeof(*qc) + num_channels * sizeof(struct qcom_pwm_channel), GFP_KERNEL);

Then the individual channels can be accessed using qc->channel[i].

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