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

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

[Wesley Terpstra]

On Mon, Apr 30, 2018 at 1:27 AM, Thierry Reding
 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 = ;
>
> or
>
> period-range = ;

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

[Wesley Terpstra]

On Mon, Apr 30, 2018 at 1:27 AM, Thierry Reding
 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 = ;
>
> or
>
> period-range = ;

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

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

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

[Thierry Reding]

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


signature.asc
Description: PGP signature

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

[Thierry Reding]

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


signature.asc
Description: PGP signature

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

[Thierry Reding]

On Sun, Apr 29, 2018 at 01:51:34PM -0700, Wesley Terpstra wrote:
> On Sat, Apr 28, 2018 at 10:54 PM, Thierry Reding
>  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 = ;

or

period-range = ;

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


signature.asc
Description: PGP signature

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

[Thierry Reding]

On Sun, Apr 29, 2018 at 01:51:34PM -0700, Wesley Terpstra wrote:
> On Sat, Apr 28, 2018 at 10:54 PM, Thierry Reding
>  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 = ;

or

period-range = ;

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


signature.asc
Description: PGP signature

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

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


signature.asc
Description: PGP signature

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

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


signature.asc
Description: PGP signature

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

[Wesley Terpstra]

On Sun, Apr 29, 2018 at 2:01 PM, Andreas Färber  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

[Andreas Färber]

Am 29.04.2018 um 22:51 schrieb Wesley Terpstra:
> On Sat, Apr 28, 2018 at 10:54 PM, Thierry Reding
>  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

[Andreas Färber]

Am 29.04.2018 um 22:51 schrieb Wesley Terpstra:
> On Sat, Apr 28, 2018 at 10:54 PM, Thierry Reding
>  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 Sat, Apr 28, 2018 at 10:54 PM, Thierry Reding
 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

[Wesley Terpstra]

On Sat, Apr 28, 2018 at 10:54 PM, Thierry Reding
 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

[Thierry Reding]

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


signature.asc
Description: PGP signature

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

[Thierry Reding]

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


signature.asc
Description: PGP signature

[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 
---
 .../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 000..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@1002 {
+   compatible = "sifive,pwm0";
+   reg = <0x0 0x1002 0x0 0x1000>;
+   clocks = <>;
+   interrupt-parent = <>;
+   interrupts = <42 43 44 45>;
+   #pwm-cells = <2>;
+   sifive,approx-period = <100>;
+};
-- 
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 
---
 .../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 000..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@1002 {
+   compatible = "sifive,pwm0";
+   reg = <0x0 0x1002 0x0 0x1000>;
+   clocks = <>;
+   interrupt-parent = <>;
+   interrupts = <42 43 44 45>;
+   #pwm-cells = <2>;
+   sifive,approx-period = <100>;
+};
-- 
2.7.4