LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Wesley Terpstra <firstname.lastname@example.org>
To: Thierry Reding <email@example.com>
Cc: "Rob Herring" <firstname.lastname@example.org>,
"Mark Rutland" <email@example.com>,
"Andreas Färber" <firstname.lastname@example.org>,
"Noralf Trønnes" <email@example.com>,
"David Lechner" <firstname.lastname@example.org>,
"Alexandre Belloni" <email@example.com>,
"SZ Lin" <firstname.lastname@example.org>,
Subject: Re: [PATCH 1/3] dt-bindings: added new pwm-sifive driver documentation
Date: Sun, 29 Apr 2018 13:51:34 -0700 [thread overview]
Message-ID: <CAMgXwTgN26aKaSuVdSj2TDUgbR7bAmy4TdtOuKj_QC0kNNbphg@mail.gmail.com> (raw)
On Sat, Apr 28, 2018 at 10:54 PM, Thierry Reding
> On Fri, Apr 27, 2018 at 03:59:56PM -0700, Wesley W. Terpstra wrote:
>> +Unlike most other PWM controllers, the SiFive PWM controller currently only
>> +supports one period for all channels in the PWM. This is set globally in DTS.
>> +The period also has significant restrictions on the values it can achieve,
>> +which the driver rounds to the nearest achievable frequency.
> How about you encode this in the driver rather than DT? We have several
> drivers with similar restrictions and they usually allow the first PWM
> channel to choose an arbitrary period and return an error if subsequent
> channels request a period that can't be supported.
> I think something similar could work with that second restriction. Why
> not return an error if the exact period can't be set? Or perhaps allow
> some percentage of deviation.
Interesting. There are two ways to use this PWM. In one mode you use
all channels of the PWM as outputs. This is the mode the driver
supports because the HiFive Unleashed board uses all channels
connected to LEDs.
In this case, the PWM period must always be a power-of-two reduction
from the core bus clock frequency. The core bus clock frequency can
vary. Therefore, even if the caller's frequency can be achieved at the
time of the method call, it may not remain achievable. You might say
this is a ridiculous PWM design, and I'd agree with you, but sadly
this is what is found in these chips. So effectively, the only thing
we can guarantee is that the period is within a multiple of sqrt(2)
from the requested period, except even that is not true due to
saturation restrictions that could push the period even further from
what you ask.
In the other mode (where you sacrifice one of the output channels),
you have finer control of the period, but it still affects all
channels. This mode might be a better fit for your proposed API,
except that the driver would still be subject to saturation
restrictions on the period. And those restrictions will change as the
core bus frequency is changed, which means that again, even if your
target period can be achieved (common to all channels) at invocation,
it might not be achievable later.
IMO the only real control this PWM can offer reliably is the duty
cycle, which is why I've written it how I have.
If you see a better solution to the above problems, I am open to suggestions.
>> +Required properties:
>> +- compatible: should be "sifive,pwm0"
> Why not simply "sifive,pwm"? If this is supposed to be some sort of
> version number, then it is more customary to use the name of the first
> SoC that integrates the IP. There are some exceptions, like for example
> when the IP is third-party and is integrated in a number of different
> SoCs. In such cases the IP is often properly versioned. But that doesn't
> seem to be the case here.
It is indeed a version number. The first SoC which integrated this IP
cannot run linux. We've put a version number like this into all of our
IP blocks. Isn't an increasing number, which clearly indicates
increased functionality, better than a reference to a sequence of SoCs
whose relationships are not all that clear?
next prev parent reply other threads:[~2018-04-29 20:51 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-27 22:59 [PATCH 0/3] SiFive SoC PWM driver Wesley W. Terpstra
2018-04-27 22:59 ` [PATCH 1/3] dt-bindings: added new pwm-sifive driver documentation Wesley W. Terpstra
2018-04-29 5:54 ` Thierry Reding
2018-04-29 20:51 ` Wesley Terpstra [this message]
2018-04-29 21:01 ` Andreas Färber
2018-04-29 21:08 ` Wesley Terpstra
2018-04-30 8:19 ` Thierry Reding
2018-04-30 10:45 ` Andreas Färber
2018-05-01 16:11 ` Rob Herring
2018-04-30 8:27 ` Thierry Reding
2018-04-30 19:09 ` Wesley Terpstra
2018-04-30 9:42 ` Thierry Reding
2018-04-27 22:59 ` [PATCH 2/3] dt-bindings: Add "sifive" vendor prefix Wesley W. Terpstra
2018-04-28 11:21 ` Andreas Färber
[not found] ` <CAMgXwThXvdzi27GjTD-q4Fw7kM5WOCtMewh7U3M4wcLwEx+VQw@mail.gmail.com>
2018-05-01 16:14 ` Rob Herring
2018-05-01 16:32 ` Palmer Dabbelt
2018-04-27 22:59 ` [PATCH 3/3] pwm-sifive: add a driver for SiFive SoC PWM Wesley W. Terpstra
2018-04-30 9:39 ` Thierry Reding
2018-04-30 19:09 ` Wesley Terpstra
2018-05-04 8:43 ` kbuild test robot
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:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--subject='Re: [PATCH 1/3] dt-bindings: added new pwm-sifive driver documentation' \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).