Hello Sean,

On Fri, Jun 25, 2021 at 11:13:33AM -0400, Sean Anderson wrote:
> On 6/25/21 2:19 AM, Uwe Kleine-König wrote:
> > On Fri, May 28, 2021 at 05:45:22PM -0400, Sean Anderson wrote:
> >> + * Hardware limitations:

Please make this "* Limitations:" to match what the other drivers do and
so ease grepping for this info.

> >> + * - When changing both duty cycle and period, we may end up with one cycle
> >> + *   with the old duty cycle and the new period.
> >
> > That means it doesn't reset the counter when a new period is set, right?
> 
> Correct. The only way to write to the counter is to stop the timer and
> restart it.

ok.

> >> + * - Cannot produce 100% duty cycle.
> >
> > Can it produce a 0% duty cycle? Below you're calling
> > xilinx_timer_tlr_period(..., ..., ..., 0) then which returns -ERANGE.
> 
> Yes. This is what you get when you try to specify 100% duty cycle (e.g.
> TLR0 == TLR1).

OK, so the hardware can do it, but your driver doesn't make use of it,
right?

> >> + * - Only produces "normal" output.
> >
> > Does the output emit a low level when it's disabled?
> 
> I believe so.

Is there a possibility to be sure? I'd like to know that to complete my
picture about the behaviour of the supported PWMs.

> >> + */
> >> +
> >> [...]
> >> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
> >> +			    const struct pwm_state *state)
> >> +{
> >> +	int ret;
> >> +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
> >> +	u32 tlr0, tlr1;
> >> +	u32 tcsr0 = xilinx_timer_read(priv, TCSR0);
> >> +	u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
> >> +	bool enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
> >> +
> >> +	if (state->polarity != PWM_POLARITY_NORMAL)
> >> +		return -EINVAL;
> >> +
> >> +	ret = xilinx_timer_tlr_period(priv, &tlr0, tcsr0, state->period);
> >> +	if (ret)
> >> +		return ret;
> >
> > The implementation of xilinx_timer_tlr_period (in patch 2/3) returns
> > -ERANGE for big periods. The good behaviour to implement is to cap to
> > the biggest period possible in this case.
> 
> Ok. Is this documented anywhere?

I tried but Thierry didn't like the result and I didn't retry. The
problem is also that many drivers we already have in the tree don't
behave like this (because for a long time nobody cared). That new
drivers should behave this way is my effort to get some consistent
behaviour.

> And wouldn't this result in the wrong duty cycle? E.g. say the max
> value is 100 and I try to apply a period of 150 and a duty_cycle of 75
> (for a 50% duty cycle). If we cap at 100, then I will instead have a
> 75% duty cycle, and there will be no error.

Yes that is right. That there is no feedback is a problem that we have
for a long time. I have a prototype patch that implements a
pwm_round_state() function that lets a consumer know the result of
applying a certain pwm_state in advance. But we're not there yet.

> So I will silently get the wrong duty cycle, even when that duty cycle
> is probably more important than the period.

It depends on the use case and every policy is wrong for some cases. So
I picked the policy I already explained because it is a) easy to
implement for lowlevel drivers and b) it's easy to work with for
consumers once we have pwm_round_state().

> > Also note that state->period is an u64 but it is casted to unsigned int
> > as this is the type of the forth parameter of xilinx_timer_tlr_period.
> 
> Hm, it looks like I immediately cast period to a u64. I will change the
> signature for this function next revision.

Then note that period * clk_get_rate(priv->clk) might overflow.
 
> >> +	ret = xilinx_timer_tlr_period(priv, &tlr1, tcsr1, state->duty_cycle);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	xilinx_timer_write(priv, tlr0, TLR0);
> >> +	xilinx_timer_write(priv, tlr1, TLR1);
> >> +
> >> +	if (state->enabled) {
> >> +		/* Only touch the TCSRs if we aren't already running */
> >> +		if (!enabled) {
> >> +			/* Load TLR into TCR */
> >> +			xilinx_timer_write(priv, tcsr0 | TCSR_LOAD, TCSR0);
> >> +			xilinx_timer_write(priv, tcsr1 | TCSR_LOAD, TCSR1);
> >> +			/* Enable timers all at once with ENALL */
> >> +			tcsr0 = (TCSR_PWM_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
> >> +			tcsr1 = TCSR_PWM_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
> >> +			xilinx_timer_write(priv, tcsr0, TCSR0);
> >> +			xilinx_timer_write(priv, tcsr1, TCSR1);
> >> +		}
> >> +	} else {
> >> +		xilinx_timer_write(priv, 0, TCSR0);
> >> +		xilinx_timer_write(priv, 0, TCSR1);
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static void xilinx_pwm_get_state(struct pwm_chip *chip,
> >> +				 struct pwm_device *unused,
> >> +				 struct pwm_state *state)
> >> +{
> >> +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
> >> +	u32 tlr0 = xilinx_timer_read(priv, TLR0);
> >> +	u32 tlr1 = xilinx_timer_read(priv, TLR1);
> >> +	u32 tcsr0 = xilinx_timer_read(priv, TCSR0);
> >> +	u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
> >> +
> >> +	state->period = xilinx_timer_get_period(priv, tlr0, tcsr0);
> >> +	state->duty_cycle = xilinx_timer_get_period(priv, tlr1, tcsr1);
> >> +	state->enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
> >> +	state->polarity = PWM_POLARITY_NORMAL;
> >
> > Are the values returned here sensible if the hardware isn't in PWM mode?
> 
> Yes. If the hardware isn't in PWM mode, then state->enabled will be
> false.

Ah right. Good enough.

> >> +	else if (pwm_cells)
> >> +		return dev_err_probe(dev, -EINVAL, "#pwm-cells must be 0\n");
> >
> > What is the rationale here to not support #pwm-cells = <2>?
> 
> Only one PWM is supported. But otherwise there is no particular
> reason.

The usual binding is to have 3 additional parameters.
 1) chip-local pwm number (which can only be 0 for a pwmchip having
    .npwm = 1)
 2) the "typical" period
 3) some flags (like PWM_POLARITY_*)

I don't care much if you implement it with or without 1), but 2) and 3)
should IMHO be here. If you don't want 1),
http://patchwork.ozlabs.org/project/linux-pwm/patch/20210622030948.966748-1-bjorn.andersson@linaro.org/
might be interesting for you. (But note, Thierry didn't give feedback to
this yet, it might be possible he wants 1)-3) for new drivers.)

Best regards
Uwe

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