Message-ID: <YVNKykYBA3rWJVuf@builder.lan>
Date:   Tue, 28 Sep 2021 12:03:06 -0500
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Uwe Kleine-K?nig <u.kleine-koenig@...gutronix.de>
Cc:     Andrzej Hajda <a.hajda@...sung.com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Robert Foss <robert.foss@...aro.org>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Jonas Karlman <jonas@...boo.se>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Thierry Reding <thierry.reding@...il.com>,
        Lee Jones <lee.jones@...aro.org>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        linux-pwm@...r.kernel.org, Doug Anderson <dianders@...gle.com>
Subject: Re: [PATCH v5 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip

On Sat 25 Sep 16:25 CDT 2021, Uwe Kleine-K?nig wrote:

> Hello Bjorn,
> 
> On Fri, Sep 24, 2021 at 05:04:41PM -0500, Bjorn Andersson wrote:
> > On Fri 24 Sep 02:54 CDT 2021, Uwe Kleine-K?nig wrote:
> > > > +static int ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
> > > > +				 unsigned int reg, u16 *val)
> > > > +{
> > > > +	unsigned int tmp;
> > > > +	int ret;
> > > > +
> > > > +	ret = regmap_read(pdata->regmap, reg, &tmp);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +	*val = tmp;
> > > > +
> > > > +	ret = regmap_read(pdata->regmap, reg + 1, &tmp);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +	*val |= tmp << 8;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata,
> > > >  				   unsigned int reg, u16 val)
> > > >  {
> > > > -	regmap_write(pdata->regmap, reg, val & 0xFF);
> > > > -	regmap_write(pdata->regmap, reg + 1, val >> 8);
> > > > +	u8 buf[2] = { val & 0xff, val >> 8 };
> > > > +
> > > > +	regmap_bulk_write(pdata->regmap, reg, &buf, ARRAY_SIZE(buf));
> > > 
> > > Given that ti_sn65dsi86_write_u16 uses regmap_bulk_write I wonder why
> > > ti_sn65dsi86_read_u16 doesn't use regmap_bulk_read.
> > 
> > Seems reasonable to make that use the bulk interface as well and this
> > would look better in its own patch.
> 
> Not sure I understand you here. My position here would be to introduce
> these two functions with a consistent behaviour. If both use bulk or
> both don't use it (and you introduce it later in a separate patch)
> doesn't matter to me.
> 

We're in agreement :)

> > > >  static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata)
> > > > @@ -253,6 +298,12 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata)
> > > >  
> > > >  	regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK,
> > > >  			   REFCLK_FREQ(i));
> > > > +
> > > > +	/*
> > > > +	 * The PWM refclk is based on the value written to SN_DPPLL_SRC_REG,
> > > > +	 * regardless of its actual sourcing.
> > > > +	 */
> > > > +	pdata->pwm_refclk_freq = ti_sn_bridge_refclk_lut[i];
> > > >  }
> > > >  
> > > >  static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata)
> > > > @@ -1259,9 +1310,283 @@ static struct auxiliary_driver ti_sn_bridge_driver = {
> > > >  };
> > > >  
> > > >  /* -----------------------------------------------------------------------------
> > > > - * GPIO Controller
> > > > + * PWM Controller
> > > > + */
> > > > +#if defined(CONFIG_PWM)
> > > > +static int ti_sn_pwm_pin_request(struct ti_sn65dsi86 *pdata)
> > > > +{
> > > > +	return atomic_xchg(&pdata->pwm_pin_busy, 1) ? -EBUSY : 0;
> > > > +}
> > > > +
> > > > +static void ti_sn_pwm_pin_release(struct ti_sn65dsi86 *pdata)
> > > > +{
> > > > +	atomic_set(&pdata->pwm_pin_busy, 0);
> > > > +}
> > > > +
> > > > +static struct ti_sn65dsi86 *pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip)
> > > > +{
> > > > +	return container_of(chip, struct ti_sn65dsi86, pchip);
> > > > +}
> > > > +
> > > > +static int ti_sn_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> > > > +{
> > > > +	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> > > > +
> > > > +	return ti_sn_pwm_pin_request(pdata);
> > > > +}
> > > > +
> > > > +static void ti_sn_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > > > +{
> > > > +	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> > > > +
> > > > +	ti_sn_pwm_pin_release(pdata);
> > > > +}
> > > > +
> > > > +/*
> > > > + * Limitations:
> > > > + * - The PWM signal is not driven when the chip is powered down, or in its
> > > > + *   reset state and the driver does not implement the "suspend state"
> > > > + *   described in the documentation. In order to save power, state->enabled is
> > > > + *   interpreted as denoting if the signal is expected to be valid, and is used
> > > > + *   to determine if the chip needs to be kept powered.
> > > > + * - Changing both period and duty_cycle is not done atomically, neither is the
> > > > + *   multi-byte register updates, so the output might briefly be undefined
> > > > + *   during update.
> > > >   */
> > > > +static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > +			   const struct pwm_state *state)
> > > > +{
> > > > +	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> > > > +	unsigned int pwm_en_inv;
> > > > +	unsigned int backlight;
> > > > +	unsigned int pre_div;
> > > > +	unsigned int scale;
> > > > +	u64 period_max;
> > > > +	u64 period;
> > > > +	int ret;
> > > > +
> > > > +	if (!pdata->pwm_enabled) {
> > > > +		ret = pm_runtime_get_sync(pdata->dev);
> > > > +		if (ret < 0) {
> > > > +			pm_runtime_put_sync(pdata->dev);
> > > > +			return ret;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	if (state->enabled) {
> > > > +		if (!pdata->pwm_enabled) {
> > > > +			/*
> > > > +			 * The chip might have been powered down while we
> > > > +			 * didn't hold a PM runtime reference, so mux in the
> > > > +			 * PWM function on the GPIO pin again.
> > > > +			 */
> > > > +			ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,
> > > > +						 SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO_IDX),
> > > > +						 SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO_IDX));
> > > > +			if (ret) {
> > > > +				dev_err(pdata->dev, "failed to mux in PWM function\n");
> > > > +				goto out;
> > > > +			}
> > > > +		}
> > > > +
> > > > +		/*
> > > > +		 * Per the datasheet the PWM frequency is given by:
> > > > +		 *
> > > > +		 *                          REFCLK_FREQ
> > > > +		 *   PWM_FREQ = -----------------------------------
> > > > +		 *               PWM_PRE_DIV * BACKLIGHT_SCALE + 1
> > > > +		 *
> > > > +		 * Unfortunately the math becomes overly complex unless we
> > > > +		 * assume that this formula is missing parenthesis around
> > > > +		 * BACKLIGHT_SCALE + 1.
> > > 
> > > We shouldn't assume a different formula than the reference manual
> > > describes because it's complex. The reasoning should be that the
> > > reference manual is wrong and that someone has convinced themselves the
> > > assumed formula is the right one instead.
> > 
> > Given the effort put in to conclude that there must be some parenthesis
> > there, I agree that "assume" might be to weak of a word...
> > 
> > > With the assumed formula: What happens if PWM_PRE_DIV is 0?
> > 
> > Looking at the specification again and I don't see anything prohibiting
> > from writing 0 in the PRE_DIV register, and writing a 0 to the register
> > causes no visual on my backlight from writing 1.
> 
> So the backlight behaves identically for PRE_DIV = 0 and PRE_DIV = 1 as
> far as you can tell?
> 

Flipping between them I see no difference on my screen.

> > Per our new understanding this should probably have resulted in a period
> > of 0.
> 
> ... but a period of zero doesn't make sense.
> 

Right, that's what I meant. If the hardware really did operate with a
zero period (or a period of 1 refclock cycle) that would be visually
noticeable.

> > That said, if the formula from the datasheet was correct then we'd
> > have a period T_pwm (given T_ref as refclk pulse length) of:
> > 
> >   T_pwm = T_ref * (div * scale + 1)
> > 
> > And with div = 0 we'd have:
> > 
> >   T_pwm = T_ref * 1
> > 
> > Which wouldn't give any room for adjusting the duty cycle, and I can
> > still do that. So that's not correct either.
> 
> I don't see a problem here (just the hardware would be unusual and
> complicated and so more expensive than the simple straigt forward way).
> 
> One way to find out if
> 
> 	 PWM_PRE_DIV * BACKLIGHT_SCALE + 1
> 	-----------------------------------
> 	            REFCLK_FREQ
> 
> or
> 
> 	 PWM_PRE_DIV * (BACKLIGHT_SCALE + 1)
> 	-------------------------------------
> 	             REFCLK_FREQ
> 
> is the right formula is to program PWM_PRE_DIV=1 and BACKLIGHT_SCALE=1
> and then check if the backlight is fully on with SN_BACKLIGHT_REG=2 (or
> only with SN_BACKLIGHT_REG=3).
> 
> > Unfortunately I still don't want to dismantle my working laptop, so I
> > don't know if I can do any better than this. Perhaps they do the typical
> > trick of encoding prediv off-by-1 and the PWM duty is off by a factor
> > of 2. Perhaps the comparator for the prediv counter trips at 1 even
> > though the register is configured at 0...
> 
> I would guess the latter.
> 

I will do some more experiments, but as we concluded previously, the
first formula (without parenthesis) doesn't make sense.

> > > > +		/*
> > > > +		 * Maximum T_pwm is 255 * (65535 + 1) / REFCLK_FREQ
> > > > +		 * Limit period to this to avoid overflows
> > > > +		 */
> > > > +		period_max = div_u64((u64)NSEC_PER_SEC * 255 * (65535 + 1), pdata->pwm_refclk_freq);
> > > > +		if (period > period_max)
> > > > +			period = period_max;
> > > > +		else
> > > > +			period = state->period;
> > > > +
> > > > +		pre_div = DIV64_U64_ROUND_UP(period * pdata->pwm_refclk_freq,
> > > > +					     (u64)NSEC_PER_SEC * (BACKLIGHT_SCALE_MAX + 1));
> > > > +		scale = div64_u64(period * pdata->pwm_refclk_freq, (u64)NSEC_PER_SEC * pre_div) - 1;
> > > 
> > > You're loosing precision here as you divide by the result of a division.
> > 
> > As described above, I'm trying to find suitable values for PRE_DIV and
> > BACKLIGHT_SCALE in:
> > 
> >   T_pwm * refclk_freq
> >  --------------------- = PRE_DIV * (BACKLIGHT_SCALE + 1)
> >      NSEC_PER_SEC
> > 
> > BACKLIGHT_SCALE must fit in 16 bits and in order to be useful for
> > driving the backlight control be large enough that the resolution
> > (BACKLIGTH = [0..BACKLIGHT_SCALE]) covers whatever resolution
> > pwm-backlight might expose.
> > 
> > For the intended use case this seems pretty good, but I presume you
> > could pick better values to get closer to the requested period.
> > 
> > Do you have a better suggestion for how to pick PRE_DIV and
> > BACKLIGHT_SCALE?
> 
> My motivation is limited to spend brain cycles here if we're not sure
> we're using the right formula. Maybe my concern is wrong given that
> pre_div isn't only an interim result but an actual register value. I
> will have to think about that when I'm not tired.
> 

The alternative to this approach, afaict, would be to search for PRE_DIV
and BACKLIGHT_SCALE that gives us the closest period to the requested
one.

But that would come at the expense of duty cycle resolution, which is
what this currently optimizes for.

> > > > +static void ti_sn_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > +				struct pwm_state *state)
> > > > +{
> > > > +	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> > > > +	unsigned int pwm_en_inv;
> > > > +	unsigned int pre_div;
> > > > +	u16 backlight;
> > > > +	u16 scale;
> > > > +	int ret;
> > > > +
> > > > +	ret = regmap_read(pdata->regmap, SN_PWM_EN_INV_REG, &pwm_en_inv);
> > > > +	if (ret)
> > > > +		return;
> > > > +
> > > > +	ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_SCALE_REG, &scale);
> > > > +	if (ret)
> > > > +		return;
> > > > +
> > > > +	ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_REG, &backlight);
> > > > +	if (ret)
> > > > +		return;
> > > > +
> > > > +	ret = regmap_read(pdata->regmap, SN_PWM_PRE_DIV_REG, &pre_div);
> > > > +	if (ret)
> > > > +		return;
> > > > +
> > > > +	state->enabled = FIELD_GET(SN_PWM_EN_MASK, pwm_en_inv);
> > > > +	if (FIELD_GET(SN_PWM_INV_MASK, pwm_en_inv))
> > > > +		state->polarity = PWM_POLARITY_INVERSED;
> > > > +	else
> > > > +		state->polarity = PWM_POLARITY_NORMAL;
> > > > +
> > > > +	state->period = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * pre_div * (scale + 1),
> > > > +					 pdata->pwm_refclk_freq);
> > > > +	state->duty_cycle = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * pre_div * backlight,
> > > > +					     pdata->pwm_refclk_freq);
> > > 
> > > What happens if scale + 1 < backlight?
> > 
> > When we write the backlight register above, we cap it at scale, so for
> > the typical case that shouldn't happen.
> 
> .get_state() is called before the first .apply(), to the values to
> interpret here originate from the bootloader which might write strange
> settings that the linux driver would never do. So being prudent here is
> IMHO a good idea.
> 

Didn't notice that as I traced things, I will add some sanity nudging
here.

> It's a shame that .get_state() cannot return an error.
> 
> > So I guess the one case when this could happen is if we get_state()
> > before pwm_apply(), when someone else has written broken values
> 
> ack.
> 

Thanks,
Bjorn

Powered by blists - more mailing lists