[PATCH 0/3] SiFive SoC PWM driver

[PATCH 0/3] SiFive SoC PWM driver

[Wesley W. Terpstra]

SiFive SoCs can contain one or more PWM IP blocks.
This adds a driver for them. Tested on the HiFive Unleashed.

This patch series is broken into three parts:
  The driver itself, in drivers/pwm
  The device tree binding description, in Documentation/devicetree/bindings
  The SiFive vendor prefix, in Documentation/devicetree/bindings

Wesley W. Terpstra (3):
  dt-bindings: added new pwm-sifive driver documentation
  dt-bindings: Add "sifive" vendor prefix
  pwm-sifive: add a driver for SiFive SoC PWM

 .../devicetree/bindings/pwm/pwm-sifive.txt         |  28 +++
 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 drivers/pwm/Kconfig                                |  11 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-sifive.c                           | 259 +++++++++++++++++++++
 5 files changed, 300 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt
 create mode 100644 drivers/pwm/pwm-sifive.c

-- 
2.7.4

[PATCH 1/3] dt-bindings: added new pwm-sifive driver documentation

[Wesley W. Terpstra]

Document new PWM device tree bindings for SiFive SoCs.

Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
---
 .../devicetree/bindings/pwm/pwm-sifive.txt         | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt

diff --git a/Documentation/devicetree/bindings/pwm/pwm-sifive.txt b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
new file mode 100644
index 0000000..7cea20d
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
@@ -0,0 +1,28 @@
+SiFive PWM controller
+
+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.
+
+Required properties:
+- compatible: should be "sifive,pwm0"
+- reg: physical base address and length of the controller's registers
+- clocks: The frequency the controller runs at
+- #pwm-cells: Should be 2.
+  The first cell is the PWM channel number
+  The second cell is the PWM polarity
+- sifive,approx-period: the driver will get as close to this period as it can
+- interrupts: one interrupt per PWM channel (currently unused in the driver)
+
+Examples:
+
+pwm:  pwm@10020000 {
+	compatible = "sifive,pwm0";
+	reg = <0x0 0x10020000 0x0 0x1000>;
+	clocks = <&tlclk>;
+	interrupt-parent = <&plic>;
+	interrupts = <42 43 44 45>;
+	#pwm-cells = <2>;
+	sifive,approx-period = <1000000>;
+};
-- 
2.7.4

Re: [PATCH 1/3] dt-bindings: added new pwm-sifive driver documentation

[Thierry Reding]

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

On Fri, Apr 27, 2018 at 03:59:56PM -0700, Wesley W. Terpstra wrote:
> Document new PWM device tree bindings for SiFive SoCs.
> 
> Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> ---
>  .../devicetree/bindings/pwm/pwm-sifive.txt         | 28 ++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-sifive.txt b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> new file mode 100644
> index 0000000..7cea20d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> @@ -0,0 +1,28 @@
> +SiFive PWM controller
> +
> +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.

If the exact period is achieved in a deterministic way, it should be
possible for board designers to specify it exactly, so the consumer's
pwms property can simply take the correct period.

> +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.

Thierry

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

Re: [PATCH 1/3] dt-bindings: added new pwm-sifive driver documentation

[Wesley Terpstra]

On Sat, Apr 28, 2018 at 10:54 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> 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?

Re: [PATCH 1/3] dt-bindings: added new pwm-sifive driver documentation

[Andreas Färber]

Am 29.04.2018 um 22:51 schrieb Wesley Terpstra:
> On Sat, Apr 28, 2018 at 10:54 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
>> On Fri, Apr 27, 2018 at 03:59:56PM -0700, Wesley W. Terpstra wrote:
>>> +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?

"pwm0" sounds like a zero-indexed instance of some pwm block. If 0 is
the version here, I'd suggest to make it "pwm-0" for example - you might
want to take a look at the Xilinx bindings, which use a strict x.yy suffix.

Most SoCs don't have clearly versioned IP though, that's why for
community-contributed bindings the first SoC we encounter the IP in
usually gets the name.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 1/3] dt-bindings: added new pwm-sifive driver documentation

[Wesley Terpstra]

On Sun, Apr 29, 2018 at 2:01 PM, Andreas Färber <afaerber@suse.de> wrote:
> "pwm0" sounds like a zero-indexed instance of some pwm block. If 0 is
> the version here, I'd suggest to make it "pwm-0" for example - you might
> want to take a look at the Xilinx bindings, which use a strict x.yy suffix.

That's fine. I'll change it to pwm-0.00 in the next patch series.

> Most SoCs don't have clearly versioned IP though, that's why for
> community-contributed bindings the first SoC we encounter the IP in
> usually gets the name.

In this particular case, we do have versioned IP, and we will be
contributing drivers for those which wind up in linux-capable chips.

Re: [PATCH 1/3] dt-bindings: added new pwm-sifive driver documentation

[Thierry Reding]

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

On Sun, Apr 29, 2018 at 02:08:07PM -0700, Wesley Terpstra wrote:
> On Sun, Apr 29, 2018 at 2:01 PM, Andreas Färber <afaerber@suse.de> wrote:
> > "pwm0" sounds like a zero-indexed instance of some pwm block. If 0 is
> > the version here, I'd suggest to make it "pwm-0" for example - you might
> > want to take a look at the Xilinx bindings, which use a strict x.yy suffix.
> 
> That's fine. I'll change it to pwm-0.00 in the next patch series.

This should match the version that you use. If you're internal
versioning uses single digits, or a single version number, then I think
there's no need to use 0.00, because that would just be confusing.
However I think it'd be good to make sure it is discernible as a version
number. Perhaps something like sifive,pwm-v0. That seems to be a fairly
common scheme.

Thierry

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

Re: [PATCH 1/3] dt-bindings: added new pwm-sifive driver documentation

[Andreas Färber]

Am 30.04.2018 um 10:19 schrieb Thierry Reding:
> On Sun, Apr 29, 2018 at 02:08:07PM -0700, Wesley Terpstra wrote:
>> On Sun, Apr 29, 2018 at 2:01 PM, Andreas Färber <afaerber@suse.de> wrote:
>>> "pwm0" sounds like a zero-indexed instance of some pwm block. If 0 is
>>> the version here, I'd suggest to make it "pwm-0" for example - you might
>>> want to take a look at the Xilinx bindings, which use a strict x.yy suffix.
>>
>> That's fine. I'll change it to pwm-0.00 in the next patch series.
> 
> This should match the version that you use. If you're internal
> versioning uses single digits, or a single version number, then I think
> there's no need to use 0.00, because that would just be confusing.
> However I think it'd be good to make sure it is discernible as a version
> number. Perhaps something like sifive,pwm-v0. That seems to be a fairly
> common scheme.

Yes. My point was not to adopt another vendor's versioning scheme but to
adopt _some_ consistent scheme and document it, e.g., in a sifive.txt
similar to xilinx.txt:

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/xilinx.txt

It should be made clear what in the compatible string the version is
(thus my proposal of using a dash as separator), and there you may want
to document how to map between IP/documentation and compatibles for any
new bindings.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 1/3] dt-bindings: added new pwm-sifive driver documentation

[Rob Herring]

On Mon, Apr 30, 2018 at 12:45:20PM +0200, Andreas Färber wrote:
> Am 30.04.2018 um 10:19 schrieb Thierry Reding:
> > On Sun, Apr 29, 2018 at 02:08:07PM -0700, Wesley Terpstra wrote:
> >> On Sun, Apr 29, 2018 at 2:01 PM, Andreas Färber <afaerber@suse.de> wrote:
> >>> "pwm0" sounds like a zero-indexed instance of some pwm block. If 0 is
> >>> the version here, I'd suggest to make it "pwm-0" for example - you might
> >>> want to take a look at the Xilinx bindings, which use a strict x.yy suffix.
> >>
> >> That's fine. I'll change it to pwm-0.00 in the next patch series.
> > 
> > This should match the version that you use. If you're internal
> > versioning uses single digits, or a single version number, then I think
> > there's no need to use 0.00, because that would just be confusing.
> > However I think it'd be good to make sure it is discernible as a version
> > number. Perhaps something like sifive,pwm-v0. That seems to be a fairly
> > common scheme.
> 
> Yes. My point was not to adopt another vendor's versioning scheme but to
> adopt _some_ consistent scheme and document it, e.g., in a sifive.txt
> similar to xilinx.txt:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/xilinx.txt
> 
> It should be made clear what in the compatible string the version is
> (thus my proposal of using a dash as separator), and there you may want
> to document how to map between IP/documentation and compatibles for any
> new bindings.

Yes. And using versions in compatible strings is only accepted when 
there is a well defined versioning process. FPGAs tend to be the main 
case as most SoC vendors don't have rigorous versioning processes. I 
guess it makes sense for SiFive from the little I know about them. What 
doesn't make sense or get accepted is software folks just making up v1, 
v2, v3, etc.

Rob

Re: [PATCH 1/3] dt-bindings: added new pwm-sifive driver documentation

[Thierry Reding]

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

On Sun, Apr 29, 2018 at 01:51:34PM -0700, Wesley Terpstra wrote:
> On Sat, Apr 28, 2018 at 10:54 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > 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.

I don't like the idea of specifying something in DT that is completely
approximate because it doesn't give users any kind of control over what
is considered acceptable. In some cases an approximation to within 10%
might be acceptable, in other cases users may only want to allow 5%. In
this case there's even no way to catch or report a deviation from the
desired value.

How about you allow users to specify a valid frequency range with
something like:

	frequency-range = <MIN MAX>;

or

	period-range = <MIN MAX>;

That would on one hand give users a way of specifying the valid range of
frequencies and give your driver enough data to reject a change if it'd
result in a frequency outside of the configured range. You would also
have the possibility to react if a change in core bus clock frequency
causes the PWM period to go out of range. I wonder if there's a
mechanism that allows the clock change to be prevented (via a PRE_CHANGE
notifier perhaps?), but if not at least you could disable the PWM if it
was fed with an invalid range.

Rob, any additional thoughts from you on this matter?

Thierry

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

Re: [PATCH 1/3] dt-bindings: added new pwm-sifive driver documentation

[Wesley Terpstra]

On Mon, Apr 30, 2018 at 1:27 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> I don't like the idea of specifying something in DT that is completely
> approximate because it doesn't give users any kind of control over what
> is considered acceptable. In some cases an approximation to within 10%
> might be acceptable, in other cases users may only want to allow 5%. In
> this case there's even no way to catch or report a deviation from the
> desired value.

My view was that you basically don't have period control on this IP.
You can give it a target that it tries to get as close to as possible,
but there's no guarantee of any kind wrt. the period.

I saw there were a couple other PWM drivers which had no period
control whatsoever. They just allowed duty cycle control. Because this
IP has such a broken period-control interface, I was essentially
trying to bin it in the same category as those drivers.

Perhaps I should just remove all pretense of supporting period and
just always default to the fastest period possible?

> How about you allow users to specify a valid frequency range with
> something like:
>
>         frequency-range = <MIN MAX>;
>
> or
>
>         period-range = <MIN MAX>;

Ok, but now you have to define what happens if a clock change prevents
you from staying within this range.

Rejecting the clock frequency change does not seem a good option for
the actual SoC for which I wrote this driver. There, all the PWM does
is drive an LED bank and clock changes are used to change the core
frequency.

> you could disable the PWM if it was fed with an invalid range.

Is that really desirable behavior? If the period is defined as being
best effort for this PWM IP, which is essentially what I've done, it's
clear you want the PWM to continue to operate. That's certainly the
behavior I want on the actual SoC with this IP.

I'll make all the DTS changes you guys suggested. ie: "-v0", clarified
clocks description, and remove unused interrupts comment.

Re: [PATCH 1/3] dt-bindings: added new pwm-sifive driver documentation

[Thierry Reding]

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

On Fri, Apr 27, 2018 at 03:59:56PM -0700, Wesley W. Terpstra wrote:
> Document new PWM device tree bindings for SiFive SoCs.
> 
> Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> ---
>  .../devicetree/bindings/pwm/pwm-sifive.txt         | 28 ++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-sifive.txt b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> new file mode 100644
> index 0000000..7cea20d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> @@ -0,0 +1,28 @@
> +SiFive PWM controller
> +
> +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.
> +
> +Required properties:
> +- compatible: should be "sifive,pwm0"
> +- reg: physical base address and length of the controller's registers
> +- clocks: The frequency the controller runs at

That's not quite correct. According to the example below, this is a
phandle to the clock that drives the PWM at a given frequency.

> +- #pwm-cells: Should be 2.
> +  The first cell is the PWM channel number
> +  The second cell is the PWM polarity
> +- sifive,approx-period: the driver will get as close to this period as it can
> +- interrupts: one interrupt per PWM channel (currently unused in the driver)

Those parentheses provide information that doesn't belong in tha DT
bindings. What you this binding is used in a different operating system
that does use the interrupts?

Thierry

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

[PATCH 2/3] dt-bindings: Add "sifive" vendor prefix

[Wesley W. Terpstra]

This adds a vendor prefix "sifive" for SiFive, Inc.
We make chips.

Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index ae850d6..c98faf7 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -315,6 +315,7 @@ sgx	SGX Sensortech
 sharp	Sharp Corporation
 shimafuji	Shimafuji Electric, Inc.
 si-en	Si-En Technology Ltd.
+sifive	SiFive, Inc.
 sigma	Sigma Designs, Inc.
 sii	Seiko Instruments, Inc.
 sil	Silicon Image
-- 
2.7.4

Re: [PATCH 2/3] dt-bindings: Add "sifive" vendor prefix

[Andreas Färber]

Am 28.04.2018 um 00:59 schrieb Wesley W. Terpstra:
> This adds a vendor prefix "sifive" for SiFive, Inc.
> We make chips.
> 
> Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> ---
>  Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Andreas Färber <afaerber@suse.de>

but please reorder the patches, so that the prefix definition comes
before its first use in the pwm binding.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 2/3] dt-bindings: Add "sifive" vendor prefix

[Wesley Terpstra]

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

Will do, thanks!

On Sat, Apr 28, 2018, 4:21 AM Andreas Färber <afaerber@suse.de> wrote:

> Am 28.04.2018 um 00:59 schrieb Wesley W. Terpstra:
> > This adds a vendor prefix "sifive" for SiFive, Inc.
> > We make chips.
> >
> > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > ---
> >  Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
> >  1 file changed, 1 insertion(+)
>
> Reviewed-by: Andreas Färber <afaerber@suse.de>
>
> but please reorder the patches, so that the prefix definition comes
> before its first use in the pwm binding.
>
> Regards,
> Andreas
>
> --
> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
>

[-- Attachment #2: Type: text/html, Size: 1241 bytes --]

Re: [PATCH 2/3] dt-bindings: Add "sifive" vendor prefix

[Rob Herring]

On Sat, Apr 28, 2018 at 10:37:56PM +0000, Wesley Terpstra wrote:
> Will do, thanks!

Don't top post to lists.

I've applied this. I assume you'll have things other than the PWM...
 
> On Sat, Apr 28, 2018, 4:21 AM Andreas Färber <afaerber@suse.de> wrote:
> 
> > Am 28.04.2018 um 00:59 schrieb Wesley W. Terpstra:
> > > This adds a vendor prefix "sifive" for SiFive, Inc.
> > > We make chips.
> > >
> > > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > > ---
> > >  Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
> > >  1 file changed, 1 insertion(+)
> >
> > Reviewed-by: Andreas Färber <afaerber@suse.de>
> >
> > but please reorder the patches, so that the prefix definition comes
> > before its first use in the pwm binding.
> >
> > Regards,
> > Andreas
> >
> > --
> > SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> > GF: Felix Imendörffer, Jane Smithard, Graham Norton
> > HRB 21284 (AG Nürnberg)
> >

Re: [PATCH 2/3] dt-bindings: Add "sifive" vendor prefix

[Palmer Dabbelt]

On Tue, 01 May 2018 09:14:39 PDT (-0700), robh@kernel.org wrote:
> On Sat, Apr 28, 2018 at 10:37:56PM +0000, Wesley Terpstra wrote:
>> Will do, thanks!
>
> Don't top post to lists.
>
> I've applied this. I assume you'll have things other than the PWM...

Thanks!

We have a handful of out-of-tree drivers for our current SOC (UART, GPIO, two 
clocks, SPI, I2C) that we plan on submitting upstream as time permits.

[PATCH 3/3] pwm-sifive: add a driver for SiFive SoC PWM

[Wesley W. Terpstra]

SiFive SoCs can contain one or more PWM IP blocks.
This adds a driver for them. Tested on the HiFive Unleashed.

Signed-off-by: Wesley W. Terpstra <wesley@terpstra.ca>
---
 drivers/pwm/Kconfig      |  11 ++
 drivers/pwm/Makefile     |   1 +
 drivers/pwm/pwm-sifive.c | 259 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 271 insertions(+)
 create mode 100644 drivers/pwm/pwm-sifive.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 763ee50..49e9824 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -387,6 +387,17 @@ config PWM_SAMSUNG
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-samsung.
 
+config PWM_SIFIVE
+	tristate "SiFive PWM support"
+	depends on OF
+	depends on COMMON_CLK
+	help
+	  Generic PWM framework driver for SiFive SoCs, such as those
+          found on the HiFive Unleashed series boards.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-sifive.
+
 config PWM_SPEAR
 	tristate "STMicroelectronics SPEAr PWM support"
 	depends on PLAT_SPEAR
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 0258a74..17c5eb9 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -38,6 +38,7 @@ obj-$(CONFIG_PWM_RCAR)		+= pwm-rcar.o
 obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
 obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
+obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
 obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
 obj-$(CONFIG_PWM_STI)		+= pwm-sti.o
 obj-$(CONFIG_PWM_STM32)		+= pwm-stm32.o
diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
new file mode 100644
index 0000000..587d4d4
--- /dev/null
+++ b/drivers/pwm/pwm-sifive.c
@@ -0,0 +1,259 @@
+/*
+ * SiFive PWM driver
+ *
+ * Copyright (C) 2018 SiFive, Inc
+ *
+ * Author: Wesley W. Terpstra <wesley@sifive.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2, as published by
+ * the Free Software Foundation.
+ */
+
+#include <dt-bindings/pwm/pwm.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+
+#define MAX_PWM			4
+
+/* Register offsets */
+#define REG_PWMCFG		0x0
+#define REG_PWMCOUNT		0x8
+#define REG_PWMS		0x10
+#define	REG_PWMCMP0		0x20
+
+/* PWMCFG fields */
+#define BIT_PWM_SCALE		0
+#define BIT_PWM_STICKY		8
+#define BIT_PWM_ZERO_ZMP	9
+#define BIT_PWM_DEGLITCH	10
+#define BIT_PWM_EN_ALWAYS	12
+#define BIT_PWM_EN_ONCE		13
+#define BIT_PWM0_CENTER		16
+#define BIT_PWM0_GANG		24
+#define BIT_PWM0_IP		28
+
+#define SIZE_PWMCMP		4
+#define MASK_PWM_SCALE		0xf
+
+struct sifive_pwm_device {
+	struct pwm_chip		chip;
+	struct notifier_block	notifier;
+	struct clk		*clk;
+	void __iomem		*regs;
+	int			irq;
+	unsigned int		approx_period;
+	unsigned int		real_period;
+};
+
+static inline struct sifive_pwm_device *chip_to_sifive(struct pwm_chip *c)
+{
+	return container_of(c, struct sifive_pwm_device, chip);
+}
+
+static inline struct sifive_pwm_device *notifier_to_sifive(struct notifier_block *nb)
+{
+	return container_of(nb, struct sifive_pwm_device, notifier);
+}
+
+static int sifive_pwm_apply(struct pwm_chip *chip,
+			    struct pwm_device *dev,
+			    struct pwm_state *state)
+{
+	struct sifive_pwm_device *pwm = chip_to_sifive(chip);
+	unsigned int duty_cycle;
+	u32 frac;
+
+	duty_cycle = state->duty_cycle;
+	if (!state->enabled)
+		duty_cycle = 0;
+	if (state->polarity == PWM_POLARITY_NORMAL)
+		duty_cycle = state->period - duty_cycle;
+
+	frac = ((u64)duty_cycle << 16) / state->period;
+	frac = min(frac, 0xFFFFU);
+
+	iowrite32(frac, pwm->regs + REG_PWMCMP0 + (dev->hwpwm * SIZE_PWMCMP));
+
+	if (state->enabled) {
+		state->period = pwm->real_period;
+		state->duty_cycle = ((u64)frac * pwm->real_period) >> 16;
+		if (state->polarity == PWM_POLARITY_NORMAL)
+			state->duty_cycle = state->period - state->duty_cycle;
+	}
+
+	return 0;
+}
+
+static void sifive_pwm_get_state(struct pwm_chip *chip,
+				 struct pwm_device *dev,
+				 struct pwm_state *state)
+{
+	struct sifive_pwm_device *pwm = chip_to_sifive(chip);
+	unsigned long duty;
+
+	duty = ioread32(pwm->regs + REG_PWMCMP0 + (dev->hwpwm * SIZE_PWMCMP));
+
+	state->period     = pwm->real_period;
+	state->duty_cycle = ((u64)duty * pwm->real_period) >> 16;
+	state->polarity   = PWM_POLARITY_INVERSED;
+	state->enabled    = duty > 0;
+}
+
+static const struct pwm_ops sifive_pwm_ops = {
+	.get_state	= sifive_pwm_get_state,
+	.apply		= sifive_pwm_apply,
+	.owner		= THIS_MODULE,
+};
+
+static struct pwm_device *sifive_pwm_xlate(struct pwm_chip *chip,
+					   const struct of_phandle_args *args)
+{
+	struct sifive_pwm_device *pwm = chip_to_sifive(chip);
+	struct pwm_device *dev;
+
+	if (args->args[0] >= chip->npwm)
+		return ERR_PTR(-EINVAL);
+
+	dev = pwm_request_from_chip(chip, args->args[0], NULL);
+	if (IS_ERR(dev))
+		return dev;
+
+	/* The period cannot be changed on a per-PWM basis */
+	dev->args.period   = pwm->real_period;
+	dev->args.polarity = PWM_POLARITY_NORMAL;
+	if (args->args[1] & PWM_POLARITY_INVERTED)
+		dev->args.polarity = PWM_POLARITY_INVERSED;
+
+	return dev;
+}
+
+static void sifive_pwm_update_clock(struct sifive_pwm_device *pwm,
+				    unsigned long rate)
+{
+	/* (1 << (16+scale)) * 10^9/rate = real_period */
+	unsigned long scalePow = (pwm->approx_period * (u64)rate) / 1000000000;
+	int scale = ilog2(scalePow) - 16;
+
+	scale = clamp(scale, 0, 0xf);
+	iowrite32((1 << BIT_PWM_EN_ALWAYS) | (scale << BIT_PWM_SCALE),
+		  pwm->regs + REG_PWMCFG);
+
+	pwm->real_period = (1000000000ULL << (16 + scale)) / rate;
+}
+
+static int sifive_pwm_clock_notifier(struct notifier_block *nb,
+				     unsigned long event,
+				     void *data)
+{
+	struct clk_notifier_data *ndata = data;
+	struct sifive_pwm_device *pwm = notifier_to_sifive(nb);
+
+	if (event == POST_RATE_CHANGE)
+		sifive_pwm_update_clock(pwm, ndata->new_rate);
+
+	return NOTIFY_OK;
+}
+
+static int sifive_pwm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *node = pdev->dev.of_node;
+	struct sifive_pwm_device *pwm;
+	struct pwm_chip *chip;
+	struct resource *res;
+	int ret;
+
+	pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+
+	chip = &pwm->chip;
+	chip->dev = dev;
+	chip->ops = &sifive_pwm_ops;
+	chip->of_xlate = sifive_pwm_xlate;
+	chip->of_pwm_n_cells = 2;
+	chip->base = -1;
+
+	ret = of_property_read_u32(node, "sifive,npwm", &chip->npwm);
+	if (ret < 0 || chip->npwm > MAX_PWM)
+		chip->npwm = MAX_PWM;
+
+	ret = of_property_read_u32(node, "sifive,approx-period",
+				   &pwm->approx_period);
+	if (ret < 0) {
+		dev_err(dev, "Unable to read sifive,approx-period from DTS\n");
+		return -ENOENT;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	pwm->regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(pwm->regs)) {
+		dev_err(dev, "Unable to map IO resources\n");
+		return PTR_ERR(pwm->regs);
+	}
+
+	pwm->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(pwm->clk)) {
+		dev_err(dev, "Unable to find controller clock\n");
+		return PTR_ERR(pwm->clk);
+	}
+
+	pwm->irq = platform_get_irq(pdev, 0);
+	if (pwm->irq < 0) {
+		dev_err(dev, "Unable to find interrupt\n");
+		return pwm->irq;
+	}
+
+	/* Watch for changes to underlying clock frequency */
+	pwm->notifier.notifier_call = sifive_pwm_clock_notifier;
+	clk_notifier_register(pwm->clk, &pwm->notifier);
+
+	/* Initialize PWM config */
+	sifive_pwm_update_clock(pwm, clk_get_rate(pwm->clk));
+
+	ret = pwmchip_add(chip);
+	if (ret < 0) {
+		dev_err(dev, "cannot register PWM: %d\n", ret);
+		clk_notifier_unregister(pwm->clk, &pwm->notifier);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, pwm);
+	dev_info(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm);
+
+	return 0;
+}
+
+static int sifive_pwm_remove(struct platform_device *dev)
+{
+	struct sifive_pwm_device *pwm = platform_get_drvdata(dev);
+	struct pwm_chip *chip = &pwm->chip;
+
+	clk_notifier_unregister(pwm->clk, &pwm->notifier);
+	return pwmchip_remove(chip);
+}
+
+static const struct of_device_id sifive_pwm_of_match[] = {
+	{ .compatible = "sifive,pwm0" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sifive_pwm_of_match);
+
+static struct platform_driver sifive_pwm_driver = {
+	.probe = sifive_pwm_probe,
+	.remove = sifive_pwm_remove,
+	.driver = {
+		.name = "pwm-sifivem",
+		.of_match_table = of_match_ptr(sifive_pwm_of_match),
+	},
+};
+module_platform_driver(sifive_pwm_driver);
+
+MODULE_AUTHOR("Wesley W. Terpstra <wesley@sifive.com>");
+MODULE_DESCRIPTION("SiFive PWM driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH 3/3] pwm-sifive: add a driver for SiFive SoC PWM

[Thierry Reding]

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

On Fri, Apr 27, 2018 at 03:59:58PM -0700, Wesley W. Terpstra wrote:
> SiFive SoCs can contain one or more PWM IP blocks.
> This adds a driver for them. Tested on the HiFive Unleashed.
> 
> Signed-off-by: Wesley W. Terpstra <wesley@terpstra.ca>
> ---
>  drivers/pwm/Kconfig      |  11 ++
>  drivers/pwm/Makefile     |   1 +
>  drivers/pwm/pwm-sifive.c | 259 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 271 insertions(+)
>  create mode 100644 drivers/pwm/pwm-sifive.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 763ee50..49e9824 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -387,6 +387,17 @@ config PWM_SAMSUNG
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-samsung.
>  
> +config PWM_SIFIVE
> +	tristate "SiFive PWM support"
> +	depends on OF
> +	depends on COMMON_CLK
> +	help
> +	  Generic PWM framework driver for SiFive SoCs, such as those
> +          found on the HiFive Unleashed series boards.

There's an inconsistent use of tabs and spaces for indentation here.

> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-sifive.
> +
>  config PWM_SPEAR
>  	tristate "STMicroelectronics SPEAr PWM support"
>  	depends on PLAT_SPEAR
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 0258a74..17c5eb9 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -38,6 +38,7 @@ obj-$(CONFIG_PWM_RCAR)		+= pwm-rcar.o
>  obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
>  obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
>  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
> +obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
>  obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
>  obj-$(CONFIG_PWM_STI)		+= pwm-sti.o
>  obj-$(CONFIG_PWM_STM32)		+= pwm-stm32.o
> diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> new file mode 100644
> index 0000000..587d4d4
> --- /dev/null
> +++ b/drivers/pwm/pwm-sifive.c
> @@ -0,0 +1,259 @@
> +/*
> + * SiFive PWM driver
> + *
> + * Copyright (C) 2018 SiFive, Inc
> + *
> + * Author: Wesley W. Terpstra <wesley@sifive.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2, as published by
> + * the Free Software Foundation.
> + */

Please include a SPDX license identifier here. That's the preferred way
to specify the license these days.

> +
> +#include <dt-bindings/pwm/pwm.h>

There should be no need to include this in a driver.

> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +
> +#define MAX_PWM			4
> +
> +/* Register offsets */
> +#define REG_PWMCFG		0x0
> +#define REG_PWMCOUNT		0x8
> +#define REG_PWMS		0x10
> +#define	REG_PWMCMP0		0x20
> +
> +/* PWMCFG fields */
> +#define BIT_PWM_SCALE		0
> +#define BIT_PWM_STICKY		8
> +#define BIT_PWM_ZERO_ZMP	9
> +#define BIT_PWM_DEGLITCH	10
> +#define BIT_PWM_EN_ALWAYS	12
> +#define BIT_PWM_EN_ONCE		13
> +#define BIT_PWM0_CENTER		16
> +#define BIT_PWM0_GANG		24
> +#define BIT_PWM0_IP		28
> +
> +#define SIZE_PWMCMP		4
> +#define MASK_PWM_SCALE		0xf
> +
> +struct sifive_pwm_device {
> +	struct pwm_chip		chip;
> +	struct notifier_block	notifier;
> +	struct clk		*clk;
> +	void __iomem		*regs;
> +	int			irq;
> +	unsigned int		approx_period;
> +	unsigned int		real_period;
> +};
> +
> +static inline struct sifive_pwm_device *chip_to_sifive(struct pwm_chip *c)
> +{
> +	return container_of(c, struct sifive_pwm_device, chip);
> +}
> +
> +static inline struct sifive_pwm_device *notifier_to_sifive(struct notifier_block *nb)
> +{
> +	return container_of(nb, struct sifive_pwm_device, notifier);
> +}
> +
> +static int sifive_pwm_apply(struct pwm_chip *chip,
> +			    struct pwm_device *dev,
> +			    struct pwm_state *state)
> +{
> +	struct sifive_pwm_device *pwm = chip_to_sifive(chip);
> +	unsigned int duty_cycle;
> +	u32 frac;
> +
> +	duty_cycle = state->duty_cycle;
> +	if (!state->enabled)
> +		duty_cycle = 0;
> +	if (state->polarity == PWM_POLARITY_NORMAL)
> +		duty_cycle = state->period - duty_cycle;
> +
> +	frac = ((u64)duty_cycle << 16) / state->period;
> +	frac = min(frac, 0xFFFFU);
> +
> +	iowrite32(frac, pwm->regs + REG_PWMCMP0 + (dev->hwpwm * SIZE_PWMCMP));
> +
> +	if (state->enabled) {
> +		state->period = pwm->real_period;
> +		state->duty_cycle = ((u64)frac * pwm->real_period) >> 16;
> +		if (state->polarity == PWM_POLARITY_NORMAL)
> +			state->duty_cycle = state->period - state->duty_cycle;

That's not the right way to handle polarity. The above often has the
same effect as inversed polarity, but it's not generally the right thing
to do. Please only support polarity if your hardware can actually really
reverse it. The above is something that PWM consumers (such as the
backlight driver) should take care of.

> +	}
> +
> +	return 0;
> +}
> +
> +static void sifive_pwm_get_state(struct pwm_chip *chip,
> +				 struct pwm_device *dev,
> +				 struct pwm_state *state)
> +{
> +	struct sifive_pwm_device *pwm = chip_to_sifive(chip);
> +	unsigned long duty;
> +
> +	duty = ioread32(pwm->regs + REG_PWMCMP0 + (dev->hwpwm * SIZE_PWMCMP));
> +
> +	state->period     = pwm->real_period;
> +	state->duty_cycle = ((u64)duty * pwm->real_period) >> 16;
> +	state->polarity   = PWM_POLARITY_INVERSED;

Is the polarity really reversed by default on this IP?

> +	state->enabled    = duty > 0;
> +}
> +
> +static const struct pwm_ops sifive_pwm_ops = {
> +	.get_state	= sifive_pwm_get_state,
> +	.apply		= sifive_pwm_apply,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static struct pwm_device *sifive_pwm_xlate(struct pwm_chip *chip,
> +					   const struct of_phandle_args *args)
> +{
> +	struct sifive_pwm_device *pwm = chip_to_sifive(chip);
> +	struct pwm_device *dev;
> +
> +	if (args->args[0] >= chip->npwm)
> +		return ERR_PTR(-EINVAL);
> +
> +	dev = pwm_request_from_chip(chip, args->args[0], NULL);
> +	if (IS_ERR(dev))
> +		return dev;
> +
> +	/* The period cannot be changed on a per-PWM basis */
> +	dev->args.period   = pwm->real_period;
> +	dev->args.polarity = PWM_POLARITY_NORMAL;
> +	if (args->args[1] & PWM_POLARITY_INVERTED)
> +		dev->args.polarity = PWM_POLARITY_INVERSED;
> +
> +	return dev;
> +}
> +
> +static void sifive_pwm_update_clock(struct sifive_pwm_device *pwm,
> +				    unsigned long rate)
> +{
> +	/* (1 << (16+scale)) * 10^9/rate = real_period */
> +	unsigned long scalePow = (pwm->approx_period * (u64)rate) / 1000000000;
> +	int scale = ilog2(scalePow) - 16;
> +
> +	scale = clamp(scale, 0, 0xf);
> +	iowrite32((1 << BIT_PWM_EN_ALWAYS) | (scale << BIT_PWM_SCALE),
> +		  pwm->regs + REG_PWMCFG);
> +
> +	pwm->real_period = (1000000000ULL << (16 + scale)) / rate;
> +}
> +
> +static int sifive_pwm_clock_notifier(struct notifier_block *nb,
> +				     unsigned long event,
> +				     void *data)
> +{
> +	struct clk_notifier_data *ndata = data;
> +	struct sifive_pwm_device *pwm = notifier_to_sifive(nb);
> +
> +	if (event == POST_RATE_CHANGE)
> +		sifive_pwm_update_clock(pwm, ndata->new_rate);
> +
> +	return NOTIFY_OK;
> +}

I don't think this is a good idea. Nobody should be allowed to just
change the PWM period without letting the PWM controller have a say in
the matter. Is this ever really an issue? Does the PWM controller use
a shared clock that changes rate frequently?

> +static int sifive_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = pdev->dev.of_node;
> +	struct sifive_pwm_device *pwm;
> +	struct pwm_chip *chip;
> +	struct resource *res;
> +	int ret;
> +
> +	pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +
> +	chip = &pwm->chip;
> +	chip->dev = dev;
> +	chip->ops = &sifive_pwm_ops;
> +	chip->of_xlate = sifive_pwm_xlate;
> +	chip->of_pwm_n_cells = 2;
> +	chip->base = -1;
> +
> +	ret = of_property_read_u32(node, "sifive,npwm", &chip->npwm);
> +	if (ret < 0 || chip->npwm > MAX_PWM)
> +		chip->npwm = MAX_PWM;

I don't see this documented in the DT bindings. I also don't see a need
for it. Under what circumstances would you want to change this?

> +
> +	ret = of_property_read_u32(node, "sifive,approx-period",
> +				   &pwm->approx_period);
> +	if (ret < 0) {
> +		dev_err(dev, "Unable to read sifive,approx-period from DTS\n");
> +		return -ENOENT;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	pwm->regs = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(pwm->regs)) {
> +		dev_err(dev, "Unable to map IO resources\n");
> +		return PTR_ERR(pwm->regs);
> +	}
> +
> +	pwm->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(pwm->clk)) {
> +		dev_err(dev, "Unable to find controller clock\n");
> +		return PTR_ERR(pwm->clk);
> +	}
> +
> +	pwm->irq = platform_get_irq(pdev, 0);
> +	if (pwm->irq < 0) {
> +		dev_err(dev, "Unable to find interrupt\n");
> +		return pwm->irq;
> +	}

You don't do anything with this, why bother retrieving it?

> +
> +	/* Watch for changes to underlying clock frequency */
> +	pwm->notifier.notifier_call = sifive_pwm_clock_notifier;
> +	clk_notifier_register(pwm->clk, &pwm->notifier);
> +
> +	/* Initialize PWM config */
> +	sifive_pwm_update_clock(pwm, clk_get_rate(pwm->clk));
> +
> +	ret = pwmchip_add(chip);
> +	if (ret < 0) {
> +		dev_err(dev, "cannot register PWM: %d\n", ret);
> +		clk_notifier_unregister(pwm->clk, &pwm->notifier);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, pwm);
> +	dev_info(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm);

Please don't do this. Having informational messages like this show up in
the boot log only makes it more difficult to find actually relevant
messages like warnings or errors.

Note that it is expected that a driver will succeed to probe, that's not
something to brag about. Let the user know when something unexpected
happens.

If you really want to have this for debugging purposes, make it a
dev_dbg() message. But I'd still suggest dropping it, there are better
ways to check that your driver was successfully loaded (by inspecting
sysfs for example).

Thierry

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

Re: [PATCH 3/3] pwm-sifive: add a driver for SiFive SoC PWM

[Wesley Terpstra]

First of all, thank you very much for this detailed review! I really
appreciate it, as this is just the first driver of several we would
like to upstream and getting the reviews front-loaded like this will
really help us improve the subsequent drivers before trying to
upstream them.

On Mon, Apr 30, 2018 at 2:39 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> Please include a SPDX license identifier here. That's the preferred way
> to specify the license these days.

Done

>> +#include <dt-bindings/pwm/pwm.h>
> There should be no need to include this in a driver.

Done

>> +             if (state->polarity == PWM_POLARITY_NORMAL)
>> +                     state->duty_cycle = state->period - state->duty_cycle;
>
> That's not the right way to handle polarity. The above often has the
> same effect as inversed polarity, but it's not generally the right thing
> to do. Please only support polarity if your hardware can actually really
> reverse it. The above is something that PWM consumers (such as the
> backlight driver) should take care of.

I don't know how to declare non-support for polarity. How do I do that?

I still want DTS references to state whether or not the polarity
should be reversed, because the PWM might be connected with either
positive or negative logic to an LED, for example.

>> +     state->polarity   = PWM_POLARITY_INVERSED;
> Is the polarity really reversed by default on this IP?

Yes. In the sense that the PWM is low while the counter is less than
the duty-cycle, and high when >= the duty-cycle.

Note that this also means it's possible to achieve a constant high
value, but impossible to achieve a constant low value, other than
approximate.

> I don't think this is a good idea. Nobody should be allowed to just
> change the PWM period without letting the PWM controller have a say in
> the matter.

I agree and I intend to fight hard to make sure that future chips don't do this.

> Is this ever really an issue?

Unfortunately, yes. This chip has only one PLL output that controls
almost everything. The core runs at the PLL's output. The bus runs at
half the PLL's output. Each peripheral clock is a divided down version
of the bus clock.

> Does the PWM controller use a shared clock that changes rate frequently?

When you want to change the CPU frequency, you have to update all the
peripheral device clock dividers. It sucks, but that's what has to
happen if the core clock is changed. Fortunately, this is not done
dynamically, but might be done during boot.

For PWM, this is not a big issue, because the period mostly does not
matter as it's broken already. However, for an active SPI transfer,
this can be problematic.

>> +     ret = of_property_read_u32(node, "sifive,npwm", &chip->npwm);
>> +     if (ret < 0 || chip->npwm > MAX_PWM)
>> +             chip->npwm = MAX_PWM;
>
> I don't see this documented in the DT bindings. I also don't see a need
> for it. Under what circumstances would you want to change this?

The PWM controller can have less than 4 channels. I could try to probe
how many there are by poking registers, but that seems risky if the
PWM is attached to something.

Shall I just add this to the dt-bindings?

>> +     pwm->irq = platform_get_irq(pdev, 0);
>> +     if (pwm->irq < 0) {
>> +             dev_err(dev, "Unable to find interrupt\n");
>> +             return pwm->irq;
>> +     }
>
> You don't do anything with this, why bother retrieving it?

Mostly to ensure that it is included in the DTS so that I can rely on
it being there in any future driver updates.

>> +     dev_info(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm);
> Please don't do this.

Removed.

Re: [PATCH 3/3] pwm-sifive: add a driver for SiFive SoC PWM

[kbuild test robot]

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

Hi Wesley,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.17-rc3]
[cannot apply to next-20180503]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Wesley-W-Terpstra/SiFive-SoC-PWM-driver/20180429-174212
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arm-allyesconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/pwm/pwm-sifive.o: In function `sifive_pwm_apply':
>> pwm-sifive.c:(.text+0xe8): undefined reference to `__aeabi_uldivmod'
   drivers/pwm/pwm-sifive.o: In function `sifive_pwm_update_clock':
   pwm-sifive.c:(.text+0x1f0): undefined reference to `__aeabi_uldivmod'
   pwm-sifive.c:(.text+0x240): undefined reference to `__aeabi_uldivmod'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 64542 bytes --]

Re: [PATCH 3/3] pwm-sifive: add a driver for SiFive SoC PWM

[kbuild test robot]

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

Hi Wesley,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.17-rc3]
[cannot apply to next-20180503]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Wesley-W-Terpstra/SiFive-SoC-PWM-driver/20180429-174212
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arm-allyesconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/pwm/pwm-sifive.o: In function `sifive_pwm_apply':
>> pwm-sifive.c:(.text+0xe8): undefined reference to `__aeabi_uldivmod'
   drivers/pwm/pwm-sifive.o: In function `sifive_pwm_update_clock':
   pwm-sifive.c:(.text+0x1f0): undefined reference to `__aeabi_uldivmod'
   pwm-sifive.c:(.text+0x240): undefined reference to `__aeabi_uldivmod'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 64542 bytes --]