All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Sean Anderson <sean.anderson@seco.com>
Cc: linux-pwm@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, michal.simek@xilinx.com,
	Alvaro Gamez <alvaro.gamez@hazent.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	kernel@pengutronix.de, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4 3/3] pwm: Add support for Xilinx AXI Timer
Date: Fri, 25 Jun 2021 18:56:42 +0200	[thread overview]
Message-ID: <20210625165642.5iuorl5guuq5c7gc@pengutronix.de> (raw)
In-Reply-To: <a748143d-f157-562e-795d-dcd9a0cf9d85@seco.com>

[-- Attachment #1: Type: text/plain, Size: 6559 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

WARNING: multiple messages have this Message-ID (diff)
From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Sean Anderson <sean.anderson@seco.com>
Cc: linux-pwm@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, michal.simek@xilinx.com,
	Alvaro Gamez <alvaro.gamez@hazent.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	kernel@pengutronix.de, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4 3/3] pwm: Add support for Xilinx AXI Timer
Date: Fri, 25 Jun 2021 18:56:42 +0200	[thread overview]
Message-ID: <20210625165642.5iuorl5guuq5c7gc@pengutronix.de> (raw)
In-Reply-To: <a748143d-f157-562e-795d-dcd9a0cf9d85@seco.com>


[-- Attachment #1.1: Type: text/plain, Size: 6559 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2021-06-25 16:57 UTC|newest]

Thread overview: 78+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-28 21:45 [PATCH v4 1/3] dt-bindings: pwm: Add Xilinx AXI Timer Sean Anderson
2021-05-28 21:45 ` Sean Anderson
2021-05-28 21:45 ` [PATCH v4 2/3] clocksource: Rewrite Xilinx AXI timer driver Sean Anderson
2021-05-28 21:45   ` Sean Anderson
2021-06-01  8:47   ` Lee Jones
2021-06-01  8:47     ` Lee Jones
2021-06-01 14:24     ` Sean Anderson
2021-06-01 14:24       ` Sean Anderson
2021-05-28 21:45 ` [PATCH v4 3/3] pwm: Add support for Xilinx AXI Timer Sean Anderson
2021-05-28 21:45   ` Sean Anderson
2021-06-10 16:15   ` Sean Anderson
2021-06-10 16:15     ` Sean Anderson
2021-06-25  6:19   ` Uwe Kleine-König
2021-06-25  6:19     ` Uwe Kleine-König
2021-06-25 15:13     ` Sean Anderson
2021-06-25 15:13       ` Sean Anderson
2021-06-25 16:56       ` Uwe Kleine-König [this message]
2021-06-25 16:56         ` Uwe Kleine-König
2021-06-25 17:46         ` Sean Anderson
2021-06-25 17:46           ` Sean Anderson
2021-06-25 17:46         ` Sean Anderson
2021-06-25 17:46           ` Sean Anderson
2021-06-27 18:19           ` Uwe Kleine-König
2021-06-27 18:19             ` Uwe Kleine-König
2021-06-28 15:50             ` Sean Anderson
2021-06-28 15:50               ` Sean Anderson
2021-06-28 16:24               ` Uwe Kleine-König
2021-06-28 16:24                 ` Uwe Kleine-König
2021-06-28 16:35                 ` Sean Anderson
2021-06-28 16:35                   ` Sean Anderson
2021-06-28 17:20                   ` Uwe Kleine-König
2021-06-28 17:20                     ` Uwe Kleine-König
2021-06-28 17:41                     ` Sean Anderson
2021-06-28 17:41                       ` Sean Anderson
2021-06-29  8:31                       ` Uwe Kleine-König
2021-06-29  8:31                         ` Uwe Kleine-König
2021-06-29 18:01                         ` Sean Anderson
2021-06-29 18:01                           ` Sean Anderson
2021-06-29 20:51                           ` Uwe Kleine-König
2021-06-29 20:51                             ` Uwe Kleine-König
2021-06-29 22:21                             ` Sean Anderson
2021-06-29 22:21                               ` Sean Anderson
2021-06-29 22:26                               ` Sean Anderson
2021-06-29 22:26                                 ` Sean Anderson
2021-06-30  8:35                               ` Uwe Kleine-König
2021-06-30  8:35                                 ` Uwe Kleine-König
2021-07-08 16:59                                 ` Sean Anderson
2021-07-08 16:59                                   ` Sean Anderson
2021-07-08 19:43                                   ` Uwe Kleine-König
2021-07-08 19:43                                     ` Uwe Kleine-König
2021-07-12 16:26                                     ` Sean Anderson
2021-07-12 16:26                                       ` Sean Anderson
2021-07-12 19:49                                       ` Uwe Kleine-König
2021-07-12 19:49                                         ` Uwe Kleine-König
2021-07-13 21:49                                         ` Sean Anderson
2021-07-13 21:49                                           ` Sean Anderson
2021-06-01 13:32 ` [PATCH v4 1/3] dt-bindings: pwm: Add " Rob Herring
2021-06-01 13:32   ` Rob Herring
2021-06-01 16:47   ` Sean Anderson
2021-06-01 16:47     ` Sean Anderson
2021-06-29  8:38 ` Uwe Kleine-König
2021-06-29  8:38   ` Uwe Kleine-König
2021-06-29 14:53   ` Sean Anderson
2021-06-29 14:53     ` Sean Anderson
2021-06-30 13:47 ` Michal Simek
2021-06-30 13:47   ` Michal Simek
2021-06-30 13:58   ` Michal Simek
2021-06-30 13:58     ` Michal Simek
2021-07-01 15:38     ` Sean Anderson
2021-07-01 15:38       ` Sean Anderson
2021-07-02 11:36       ` Michal Simek
2021-07-02 11:36         ` Michal Simek
2021-07-01 15:32   ` Sean Anderson
2021-07-01 15:32     ` Sean Anderson
2021-07-02 12:40     ` Michal Simek
2021-07-02 12:40       ` Michal Simek
2021-07-02 17:31       ` Sean Anderson
2021-07-02 17:31         ` Sean Anderson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210625165642.5iuorl5guuq5c7gc@pengutronix.de \
    --to=u.kleine-koenig@pengutronix.de \
    --cc=alvaro.gamez@hazent.com \
    --cc=devicetree@vger.kernel.org \
    --cc=kernel@pengutronix.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=michal.simek@xilinx.com \
    --cc=sean.anderson@seco.com \
    --cc=thierry.reding@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.