[RFC,1/4] pwm: sifive: Add DT documentation for SiFive PWM Controller.

Message ID 1539111085-25502-2-git-send-email-atish.patra@wdc.com
State RFC
Headers show
Series
  • GPIO & PWM support for HiFive Unleashed
Related show

Commit Message

Atish Patra Oct. 9, 2018, 6:51 p.m.
From: "Wesley W. Terpstra" <wesley@sifive.com>

DT documentation for PWM controller added with updated compatible
string.

Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
[Atish: Compatible string update]
Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
 .../devicetree/bindings/pwm/pwm-sifive.txt         | 32 ++++++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt

Comments

Thierry Reding Oct. 10, 2018, 1:49 p.m. | #1
On Tue, Oct 09, 2018 at 11:51:22AM -0700, Atish Patra wrote:
> From: "Wesley W. Terpstra" <wesley@sifive.com>
> 
> DT documentation for PWM controller added with updated compatible
> string.
> 
> Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> [Atish: Compatible string update]
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> ---
>  .../devicetree/bindings/pwm/pwm-sifive.txt         | 32 ++++++++++++++++++++++
>  1 file changed, 32 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 00000000..532b10fc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> @@ -0,0 +1,32 @@
> +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.

What restrictions are these? If "nearest achievable" is too far off the
target period it might be preferable to error out.

> +Required properties:
> +- compatible: should be one of
> +	"sifive,fu540-c000-pwm0","sifive,pwm0".

What's the '0' in here? A version number?

> +	PWM controller is HiFive Unleashed specific chip which warrants a
> +	specific compatible string. The second string is kept for backward
> +	compatibility until a firmware update with latest compatible string.
> +- 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

Given the above comment, maybe "sifive,period"?

Thierry
Thierry Reding Oct. 10, 2018, 1:51 p.m. | #2
On Tue, Oct 09, 2018 at 11:51:22AM -0700, Atish Patra wrote:
[...]
> +- interrupts: one interrupt per PWM channel (currently unused in the driver)

This should probably say what the interrupt is used for. And once you
have that, remove the comment about it being unused in the driver. DT
is OS agnostic, so "driver" is very unspecific and your claim may
actually be false.

Thierry
Atish Patra Oct. 15, 2018, 10:45 p.m. | #3
On 10/10/18 6:51 AM, Thierry Reding wrote:
> On Tue, Oct 09, 2018 at 11:51:22AM -0700, Atish Patra wrote:
> [...]
>> +- interrupts: one interrupt per PWM channel (currently unused in the driver)
> 
> This should probably say what the interrupt is used for. And once you
> have that, remove the comment about it being unused in the driver. DT
> is OS agnostic, so "driver" is very unspecific and your claim may
> actually be false.
> 
> Thierry
> 
As per my understanding, they are generated by hardware but no usage of 
pwm interrupts as of now.

I am not sure if removing the entire entry is a good idea.
What would be the best way to represent that information ?

May be this ?

+-interrupts: one interrupt per PWM channel. No usage in HiFive 
Unleashed SoC.

Regards,
Atish
Atish Patra Oct. 15, 2018, 10:57 p.m. | #4
On 10/10/18 6:49 AM, Thierry Reding wrote:
> On Tue, Oct 09, 2018 at 11:51:22AM -0700, Atish Patra wrote:
>> From: "Wesley W. Terpstra" <wesley@sifive.com>
>>
>> DT documentation for PWM controller added with updated compatible
>> string.
>>
>> Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
>> [Atish: Compatible string update]
>> Signed-off-by: Atish Patra <atish.patra@wdc.com>
>> ---
>>   .../devicetree/bindings/pwm/pwm-sifive.txt         | 32 ++++++++++++++++++++++
>>   1 file changed, 32 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 00000000..532b10fc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
>> @@ -0,0 +1,32 @@
>> +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.
> 
> What restrictions are these? If "nearest achievable" is too far off the
> target period it might be preferable to error out.
> 

@Wes: Any comments?

>> +Required properties:
>> +- compatible: should be one of
>> +	"sifive,fu540-c000-pwm0","sifive,pwm0".
> 
> What's the '0' in here? A version number?
>

I think yes. Since fu540 is the first Linux capable RISC-V core, SiFive 
Guys decided mark it as version 0.

@Wesly: Please correct me if I am wrong.

>> +	PWM controller is HiFive Unleashed specific chip which warrants a
>> +	specific compatible string. The second string is kept for backward
>> +	compatibility until a firmware update with latest compatible string.
>> +- 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
> 
> Given the above comment, maybe "sifive,period"?
> 

ok.
In Unleashed board, the DT is loaded by FSBL (first stage boot loader). 
Thus, changing device tree entries requires a FSBL update. If we update 
this string, we need to update the driver to parse both properties so 
that existing devices with older firmware continue to work.

This is probably ok as we anyways do that for compatible strings. Just 
wanted to update that here for the record.

Regards,
Atish
> Thierry
>
Wesley Terpstra Oct. 15, 2018, 11:19 p.m. | #5
On Mon, Oct 15, 2018 at 3:57 PM Atish Patra <atish.patra@wdc.com> wrote:
> >> +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.
> >
> > What restrictions are these? If "nearest achievable" is too far off the
> > target period it might be preferable to error out.
> >
>
> @Wes: Any comments?

When I last looked at this driver and hardware, I briefly considered
throwing up my hands and pretending that this PWM device had no period
control at all, only duty-cycle. There are several examples of PWM
controllers in linux already that behave that way.

Most of the uses of this PWM are only going to care about the
duty-cycle, not the period. So failing when the period is unachievable
seems worse to me than just completely eliminating access to period
control.

> I think yes. Since fu540 is the first Linux capable RISC-V core, SiFive
> Guys decided mark it as version 0.
>
> @Wesly: Please correct me if I am wrong.

Correct.
Thierry Reding Oct. 16, 2018, 10:51 a.m. | #6
On Mon, Oct 15, 2018 at 03:45:46PM -0700, Atish Patra wrote:
> On 10/10/18 6:51 AM, Thierry Reding wrote:
> > On Tue, Oct 09, 2018 at 11:51:22AM -0700, Atish Patra wrote:
> > [...]
> > > +- interrupts: one interrupt per PWM channel (currently unused in the driver)
> > 
> > This should probably say what the interrupt is used for. And once you
> > have that, remove the comment about it being unused in the driver. DT
> > is OS agnostic, so "driver" is very unspecific and your claim may
> > actually be false.
> > 
> > Thierry
> > 
> As per my understanding, they are generated by hardware but no usage of pwm
> interrupts as of now.

It might be useful to say when they are generated. Are they generated
once per period? At the beginning or the end of the period? That kind
of thing.

> I am not sure if removing the entire entry is a good idea.
> What would be the best way to represent that information ?
> 
> May be this ?
> 
> +-interrupts: one interrupt per PWM channel. No usage in HiFive Unleashed
> SoC.

Why do you think you need to say that they are unused? If the hardware
generates these interrupts, then they are "used". If no driver currently
has a use for them, that's driver specific and doesn't belong in the DT
bindings.

Thierry
Thierry Reding Oct. 16, 2018, 11:01 a.m. | #7
On Mon, Oct 15, 2018 at 03:57:35PM -0700, Atish Patra wrote:
> On 10/10/18 6:49 AM, Thierry Reding wrote:
> > On Tue, Oct 09, 2018 at 11:51:22AM -0700, Atish Patra wrote:
> > > From: "Wesley W. Terpstra" <wesley@sifive.com>
> > > 
> > > DT documentation for PWM controller added with updated compatible
> > > string.
> > > 
> > > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > > [Atish: Compatible string update]
> > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > ---
> > >   .../devicetree/bindings/pwm/pwm-sifive.txt         | 32 ++++++++++++++++++++++
> > >   1 file changed, 32 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 00000000..532b10fc
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> > > @@ -0,0 +1,32 @@
> > > +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.
> > 
> > What restrictions are these? If "nearest achievable" is too far off the
> > target period it might be preferable to error out.
> > 
> 
> @Wes: Any comments?
> 
> > > +Required properties:
> > > +- compatible: should be one of
> > > +	"sifive,fu540-c000-pwm0","sifive,pwm0".
> > 
> > What's the '0' in here? A version number?
> > 
> 
> I think yes. Since fu540 is the first Linux capable RISC-V core, SiFive Guys
> decided mark it as version 0.
> 
> @Wesly: Please correct me if I am wrong.

It seems fairly superfluous to me to have a version number in additon to
the fu540-c000, which already seems to be the core plus some sort of
part number. Do you really expect there to be any changes in the SoC
that would require a different compatible string at this point? If the
SoC has taped out, how will you ever get a different version of the PWM
IP in it?

I would expect any improvements or changes to the PWM IP to show up in a
different SoC generation, at which point it would be something like
"sifive,fu640-c000" maybe, or perhaps "sifive,fu540-d000", or whatever
the numbering is.

> > > +	PWM controller is HiFive Unleashed specific chip which warrants a
> > > +	specific compatible string. The second string is kept for backward
> > > +	compatibility until a firmware update with latest compatible string.
> > > +- 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
> > 
> > Given the above comment, maybe "sifive,period"?
> > 
> 
> ok.
> In Unleashed board, the DT is loaded by FSBL (first stage boot loader).
> Thus, changing device tree entries requires a FSBL update. If we update this
> string, we need to update the driver to parse both properties so that
> existing devices with older firmware continue to work.
> 
> This is probably ok as we anyways do that for compatible strings. Just
> wanted to update that here for the record.

I'm going to defer to Rob on this. He's said in the past that he doesn't
care about existing bindings that haven't been vetted through upstream,
even if that means that bootloaders may have to be updated. I know he's
been fairly strict on this in the past, but let's see if there is room
for exceptions.

Thierry
Thierry Reding Oct. 16, 2018, 11:13 a.m. | #8
On Mon, Oct 15, 2018 at 04:19:20PM -0700, Wesley Terpstra wrote:
> On Mon, Oct 15, 2018 at 3:57 PM Atish Patra <atish.patra@wdc.com> wrote:
> > >> +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.
> > >
> > > What restrictions are these? If "nearest achievable" is too far off the
> > > target period it might be preferable to error out.
> > >
> >
> > @Wes: Any comments?
> 
> When I last looked at this driver and hardware, I briefly considered
> throwing up my hands and pretending that this PWM device had no period
> control at all, only duty-cycle. There are several examples of PWM
> controllers in linux already that behave that way.

Can you point those out? So far we've always opted to refuse changing
the period of PWM channels that share a period if it didn't match the
current period.

> Most of the uses of this PWM are only going to care about the
> duty-cycle, not the period. So failing when the period is unachievable
> seems worse to me than just completely eliminating access to period
> control.

I'm not sure we've ever tried to completely take period out of the
picture. You could probably do it if you use only the atomic API because
then you just leave the period untouched. And if you have a post-clock
change you just need to make sure to record the new period and update
the duty cycle so that the ratio remains the same.

I think that could work, but I think it'd be best to be explicit about
it, rather than just handwaving it.

Thierry
Paul Walmsley Oct. 16, 2018, 5:31 p.m. | #9
On 10/16/18 4:01 AM, Thierry Reding wrote:
> On Mon, Oct 15, 2018 at 03:57:35PM -0700, Atish Patra wrote:
>> On 10/10/18 6:49 AM, Thierry Reding wrote:
>>> On Tue, Oct 09, 2018 at 11:51:22AM -0700, Atish Patra wrote:
>>>> +Required properties:
>>>> +- compatible: should be one of
>>>> +	"sifive,fu540-c000-pwm0","sifive,pwm0".
>>> What's the '0' in here? A version number?
>>>
>> I think yes. Since fu540 is the first Linux capable RISC-V core, SiFive Guys
>> decided mark it as version 0.
>>
>> @Wesly: Please correct me if I am wrong.
> It seems fairly superfluous to me to have a version number in additon to
> the fu540-c000, which already seems to be the core plus some sort of
> part number. Do you really expect there to be any changes in the SoC
> that would require a different compatible string at this point? If the
> SoC has taped out, how will you ever get a different version of the PWM
> IP in it?
>
> I would expect any improvements or changes to the PWM IP to show up in a
> different SoC generation, at which point it would be something like
> "sifive,fu640-c000" maybe, or perhaps "sifive,fu540-d000", or whatever
> the numbering is.


The "0" suffix refers to a revision number for the underlying PWM IP block.

It's certainly important to keep that version number on the 
"sifive,pwm0" compatible string that doesn't have the chip name 
associated with it.

As to whether there could ever be a FU540-C000 part with different IP 
block versions on it: FU540-C000 is ultimately a marketing name.  While 
theoretically we shouldn't have another "FU540-C000" chip with different 
peripheral IP block versions on it, I don't think any engineer can 
guarantee that it won't happen.


- Paul
Thierry Reding Oct. 16, 2018, 10:04 p.m. | #10
On Tue, Oct 16, 2018 at 10:31:42AM -0700, Paul Walmsley wrote:
> 
> On 10/16/18 4:01 AM, Thierry Reding wrote:
> > On Mon, Oct 15, 2018 at 03:57:35PM -0700, Atish Patra wrote:
> > > On 10/10/18 6:49 AM, Thierry Reding wrote:
> > > > On Tue, Oct 09, 2018 at 11:51:22AM -0700, Atish Patra wrote:
> > > > > +Required properties:
> > > > > +- compatible: should be one of
> > > > > +	"sifive,fu540-c000-pwm0","sifive,pwm0".
> > > > What's the '0' in here? A version number?
> > > > 
> > > I think yes. Since fu540 is the first Linux capable RISC-V core, SiFive Guys
> > > decided mark it as version 0.
> > > 
> > > @Wesly: Please correct me if I am wrong.
> > It seems fairly superfluous to me to have a version number in additon to
> > the fu540-c000, which already seems to be the core plus some sort of
> > part number. Do you really expect there to be any changes in the SoC
> > that would require a different compatible string at this point? If the
> > SoC has taped out, how will you ever get a different version of the PWM
> > IP in it?
> > 
> > I would expect any improvements or changes to the PWM IP to show up in a
> > different SoC generation, at which point it would be something like
> > "sifive,fu640-c000" maybe, or perhaps "sifive,fu540-d000", or whatever
> > the numbering is.
> 
> 
> The "0" suffix refers to a revision number for the underlying PWM IP block.
> 
> It's certainly important to keep that version number on the "sifive,pwm0"
> compatible string that doesn't have the chip name associated with it.

Isn't the hardware identified by "sifive,pwm0" and "sifive,fu540-c000"
effectively identical? Is there a need to have two compatible strings
that refer to the exact same hardware?

> As to whether there could ever be a FU540-C000 part with different IP block
> versions on it: FU540-C000 is ultimately a marketing name.  While
> theoretically we shouldn't have another "FU540-C000" chip with different
> peripheral IP block versions on it, I don't think any engineer can guarantee
> that it won't happen.

I would argue that if at some point there was indeed a chip with the
same name but a different IP block version in it, we can figure out what
to call it. Sure there are no guarantees, but it's still fairly unlikely
in my opinion, so I personally wouldn't worry about this up front.

Anyway, I don't feel strongly either way, I'm just pointing out that
this is somewhat unusual. If you want to keep it, feel free to.

Thierry
Atish Patra Oct. 16, 2018, 10:20 p.m. | #11
On 10/16/18 3:04 PM, Thierry Reding wrote:
> On Tue, Oct 16, 2018 at 10:31:42AM -0700, Paul Walmsley wrote:
>>
>> On 10/16/18 4:01 AM, Thierry Reding wrote:
>>> On Mon, Oct 15, 2018 at 03:57:35PM -0700, Atish Patra wrote:
>>>> On 10/10/18 6:49 AM, Thierry Reding wrote:
>>>>> On Tue, Oct 09, 2018 at 11:51:22AM -0700, Atish Patra wrote:
>>>>>> +Required properties:
>>>>>> +- compatible: should be one of
>>>>>> +	"sifive,fu540-c000-pwm0","sifive,pwm0".
>>>>> What's the '0' in here? A version number?
>>>>>
>>>> I think yes. Since fu540 is the first Linux capable RISC-V core, SiFive Guys
>>>> decided mark it as version 0.
>>>>
>>>> @Wesly: Please correct me if I am wrong.
>>> It seems fairly superfluous to me to have a version number in additon to
>>> the fu540-c000, which already seems to be the core plus some sort of
>>> part number. Do you really expect there to be any changes in the SoC
>>> that would require a different compatible string at this point? If the
>>> SoC has taped out, how will you ever get a different version of the PWM
>>> IP in it?
>>>
>>> I would expect any improvements or changes to the PWM IP to show up in a
>>> different SoC generation, at which point it would be something like
>>> "sifive,fu640-c000" maybe, or perhaps "sifive,fu540-d000", or whatever
>>> the numbering is.
>>
>>
>> The "0" suffix refers to a revision number for the underlying PWM IP block.
>>
>> It's certainly important to keep that version number on the "sifive,pwm0"
>> compatible string that doesn't have the chip name associated with it.
> 
> Isn't the hardware identified by "sifive,pwm0" and "sifive,fu540-c000"
> effectively identical? 

Yes.

Is there a need to have two compatible strings
> that refer to the exact same hardware?
> 

The DT in the hardware has only sifive,pwm0. I have added 
"sifive,fu540-c000" as that was concluded as the correct compatible 
string from platform level interrupt controller patch(PLIC) discussion.

(http://lists.infradead.org/pipermail/linux-riscv/2018-August/001135.html)

"sifive,pwm0" is required to until all the Unleashed SoC gets an updated 
firmware with correct compatible string "sifive,fu540-c000". I agree 
this is a mess. But we have to carry it until all every DT(corresponding 
to each driver) is finalized. I guess SiFive will release a firmware 
update that contains all the updated DT once that is done. We can get 
rid of all the redundant compatible strings at that time.

Regards,
Atish
>> As to whether there could ever be a FU540-C000 part with different IP block
>> versions on it: FU540-C000 is ultimately a marketing name.  While
>> theoretically we shouldn't have another "FU540-C000" chip with different
>> peripheral IP block versions on it, I don't think any engineer can guarantee
>> that it won't happen.
> 
> I would argue that if at some point there was indeed a chip with the
> same name but a different IP block version in it, we can figure out what
> to call it. Sure there are no guarantees, but it's still fairly unlikely
> in my opinion, so I personally wouldn't worry about this up front.
> 
> Anyway, I don't feel strongly either way, I'm just pointing out that
> this is somewhat unusual. If you want to keep it, feel free to.
> 
> Thierry
>
Atish Patra Oct. 16, 2018, 10:42 p.m. | #12
On 10/16/18 3:51 AM, Thierry Reding wrote:
> On Mon, Oct 15, 2018 at 03:45:46PM -0700, Atish Patra wrote:
>> On 10/10/18 6:51 AM, Thierry Reding wrote:
>>> On Tue, Oct 09, 2018 at 11:51:22AM -0700, Atish Patra wrote:
>>> [...]
>>>> +- interrupts: one interrupt per PWM channel (currently unused in the driver)
>>>
>>> This should probably say what the interrupt is used for. And once you
>>> have that, remove the comment about it being unused in the driver. DT
>>> is OS agnostic, so "driver" is very unspecific and your claim may
>>> actually be false.
>>>
>>> Thierry
>>>
>> As per my understanding, they are generated by hardware but no usage of pwm
>> interrupts as of now.
> 
> It might be useful to say when they are generated. Are they generated
> once per period? At the beginning or the end of the period? That kind
> of thing.
> 

Sure. I might have over simplified the statement above.
I could only find this about pwm interrupts in spec.
"The PWM can be configured to provide periodic counter interrupts by 
enabling auto-zeroing of the count register when a comparator 0 fires"

I may be wrong here but it looks like we need to configure the hardware 
to generate periodic interrupts. I will confirm with Wesly and update it 
in v2.

>> I am not sure if removing the entire entry is a good idea.
>> What would be the best way to represent that information ?
>>
>> May be this ?
>>
>> +-interrupts: one interrupt per PWM channel. No usage in HiFive Unleashed
>> SoC.
> 
> Why do you think you need to say that they are unused? If the hardware
> generates these interrupts, then they are "used". If no driver currently
> has a use for them, that's driver specific and doesn't belong in the DT
> bindings.
> 

Sounds good. I will update accordingly.

Regards,
Atish
> Thierry
>
Rob Herring Oct. 17, 2018, 3:58 p.m. | #13
On Tue, Oct 16, 2018 at 03:20:34PM -0700, Atish Patra wrote:
> On 10/16/18 3:04 PM, Thierry Reding wrote:
> > On Tue, Oct 16, 2018 at 10:31:42AM -0700, Paul Walmsley wrote:
> > > 
> > > On 10/16/18 4:01 AM, Thierry Reding wrote:
> > > > On Mon, Oct 15, 2018 at 03:57:35PM -0700, Atish Patra wrote:
> > > > > On 10/10/18 6:49 AM, Thierry Reding wrote:
> > > > > > On Tue, Oct 09, 2018 at 11:51:22AM -0700, Atish Patra wrote:
> > > > > > > +Required properties:
> > > > > > > +- compatible: should be one of
> > > > > > > +	"sifive,fu540-c000-pwm0","sifive,pwm0".
> > > > > > What's the '0' in here? A version number?
> > > > > > 
> > > > > I think yes. Since fu540 is the first Linux capable RISC-V core, SiFive Guys
> > > > > decided mark it as version 0.
> > > > > 
> > > > > @Wesly: Please correct me if I am wrong.
> > > > It seems fairly superfluous to me to have a version number in additon to
> > > > the fu540-c000, which already seems to be the core plus some sort of
> > > > part number. Do you really expect there to be any changes in the SoC
> > > > that would require a different compatible string at this point? If the
> > > > SoC has taped out, how will you ever get a different version of the PWM
> > > > IP in it?
> > > > 
> > > > I would expect any improvements or changes to the PWM IP to show up in a
> > > > different SoC generation, at which point it would be something like
> > > > "sifive,fu640-c000" maybe, or perhaps "sifive,fu540-d000", or whatever
> > > > the numbering is.
> > > 
> > > 
> > > The "0" suffix refers to a revision number for the underlying PWM IP block.
> > > 
> > > It's certainly important to keep that version number on the "sifive,pwm0"
> > > compatible string that doesn't have the chip name associated with it.
> > 
> > Isn't the hardware identified by "sifive,pwm0" and "sifive,fu540-c000"
> > effectively identical?
> 
> Yes.
> 
> Is there a need to have two compatible strings
> > that refer to the exact same hardware?
> > 
> 
> The DT in the hardware has only sifive,pwm0. I have added
> "sifive,fu540-c000" as that was concluded as the correct compatible string
> from platform level interrupt controller patch(PLIC) discussion.
> 
> (http://lists.infradead.org/pipermail/linux-riscv/2018-August/001135.html)
> 
> "sifive,pwm0" is required to until all the Unleashed SoC gets an updated
> firmware with correct compatible string "sifive,fu540-c000". I agree this is
> a mess. But we have to carry it until all every DT(corresponding to each
> driver) is finalized. I guess SiFive will release a firmware update that
> contains all the updated DT once that is done. We can get rid of all the
> redundant compatible strings at that time.

I don't want to repeat compatible string discussions on each and every 
IP block. I already have to do this with some vendors.

The RiscV vendors' needs and design flow are a bit different from 
traditional SoC vendors AIUI for the last discussion. If you need to do 
something that doesn't follow normal conventions, that's fine. Just 
please document a convention that works for you. This should explain 
where the '0' above comes from for example. And I'm not a fan of s/w 
folks making up version numbers.

Rob
Atish Patra Oct. 17, 2018, 9:45 p.m. | #14
On 10/17/18 8:58 AM, Rob Herring wrote:
> On Tue, Oct 16, 2018 at 03:20:34PM -0700, Atish Patra wrote:
>> On 10/16/18 3:04 PM, Thierry Reding wrote:
>>> On Tue, Oct 16, 2018 at 10:31:42AM -0700, Paul Walmsley wrote:
>>>>
>>>> On 10/16/18 4:01 AM, Thierry Reding wrote:
>>>>> On Mon, Oct 15, 2018 at 03:57:35PM -0700, Atish Patra wrote:
>>>>>> On 10/10/18 6:49 AM, Thierry Reding wrote:
>>>>>>> On Tue, Oct 09, 2018 at 11:51:22AM -0700, Atish Patra wrote:
>>>>>>>> +Required properties:
>>>>>>>> +- compatible: should be one of
>>>>>>>> +	"sifive,fu540-c000-pwm0","sifive,pwm0".
>>>>>>> What's the '0' in here? A version number?
>>>>>>>
>>>>>> I think yes. Since fu540 is the first Linux capable RISC-V core, SiFive Guys
>>>>>> decided mark it as version 0.
>>>>>>
>>>>>> @Wesly: Please correct me if I am wrong.
>>>>> It seems fairly superfluous to me to have a version number in additon to
>>>>> the fu540-c000, which already seems to be the core plus some sort of
>>>>> part number. Do you really expect there to be any changes in the SoC
>>>>> that would require a different compatible string at this point? If the
>>>>> SoC has taped out, how will you ever get a different version of the PWM
>>>>> IP in it?
>>>>>
>>>>> I would expect any improvements or changes to the PWM IP to show up in a
>>>>> different SoC generation, at which point it would be something like
>>>>> "sifive,fu640-c000" maybe, or perhaps "sifive,fu540-d000", or whatever
>>>>> the numbering is.
>>>>
>>>>
>>>> The "0" suffix refers to a revision number for the underlying PWM IP block.
>>>>
>>>> It's certainly important to keep that version number on the "sifive,pwm0"
>>>> compatible string that doesn't have the chip name associated with it.
>>>
>>> Isn't the hardware identified by "sifive,pwm0" and "sifive,fu540-c000"
>>> effectively identical?
>>
>> Yes.
>>
>> Is there a need to have two compatible strings
>>> that refer to the exact same hardware?
>>>
>>
>> The DT in the hardware has only sifive,pwm0. I have added
>> "sifive,fu540-c000" as that was concluded as the correct compatible string
>> from platform level interrupt controller patch(PLIC) discussion.
>>
>> (http://lists.infradead.org/pipermail/linux-riscv/2018-August/001135.html)
>>
>> "sifive,pwm0" is required to until all the Unleashed SoC gets an updated
>> firmware with correct compatible string "sifive,fu540-c000". I agree this is
>> a mess. But we have to carry it until all every DT(corresponding to each
>> driver) is finalized. I guess SiFive will release a firmware update that
>> contains all the updated DT once that is done. We can get rid of all the
>> redundant compatible strings at that time.
> 
> I don't want to repeat compatible string discussions on each and every
> IP block. I already have to do this with some vendors.
> 



> The RiscV vendors' needs and design flow are a bit different from
> traditional SoC vendors AIUI for the last discussion. If you need to do
> something that doesn't follow normal conventions, that's fine. Just
> please document a convention that works for you. This should explain
> where the '0' above comes from for example. And I'm not a fan of s/w
> folks making up version numbers.
> Sorry for bringing up the same discussion. My aim was just to reiterate 
the suggestion you made on the other other thread (i.e. PLIC compatible 
strings) and use the same format used in PLIC block. As these IP 
blocks(pwm & gpio) are also from SiFive for the same Soc (HiFive 
Unleashed board), I was just trying to clarify that this driver also 
follows the exact same convention adopted for PLIC IP block.

Regards,
Atish

> Rob
>
Paul Walmsley Nov. 10, 2018, 5:38 a.m. | #15
On 10/16/18 3:04 PM, Thierry Reding wrote:
> On Tue, Oct 16, 2018 at 10:31:42AM -0700, Paul Walmsley wrote:
>> On 10/16/18 4:01 AM, Thierry Reding wrote:
>>> On Mon, Oct 15, 2018 at 03:57:35PM -0700, Atish Patra wrote:
>>>> On 10/10/18 6:49 AM, Thierry Reding wrote:
>>>>> On Tue, Oct 09, 2018 at 11:51:22AM -0700, Atish Patra wrote:
>>>>>> +Required properties:
>>>>>> +- compatible: should be one of
>>>>>> +	"sifive,fu540-c000-pwm0","sifive,pwm0".
>>>>> What's the '0' in here? A version number?
>>>>>
>>>> I think yes. Since fu540 is the first Linux capable RISC-V core, SiFive Guys
>>>> decided mark it as version 0.
>>>>
>>>> @Wesly: Please correct me if I am wrong.
>>> It seems fairly superfluous to me to have a version number in additon to
>>> the fu540-c000, which already seems to be the core plus some sort of
>>> part number. Do you really expect there to be any changes in the SoC
>>> that would require a different compatible string at this point? If the
>>> SoC has taped out, how will you ever get a different version of the PWM
>>> IP in it?
>>>
>>> I would expect any improvements or changes to the PWM IP to show up in a
>>> different SoC generation, at which point it would be something like
>>> "sifive,fu640-c000" maybe, or perhaps "sifive,fu540-d000", or whatever
>>> the numbering is.
>>
>> The "0" suffix refers to a revision number for the underlying PWM IP block.
>>
>> It's certainly important to keep that version number on the "sifive,pwm0"
>> compatible string that doesn't have the chip name associated with it.
> Isn't the hardware identified by "sifive,pwm0" and "sifive,fu540-c000"
> effectively identical? 


The intention was that the "sifive,pwm0" compatible string specifies a
register interface and programming model that the IP block exposes to
the software, rather than a particular underlying hardware
implementation.  That is in contrast to a string like
"sifive,fu540-c000-pwm" which might activate particular workarounds or
quirks that are specific to the integration of the IP block on a given SoC.


The idea is that, for this and similar open-source hardware IP blocks,
the driver code can just match on a generic "sifive,pwm0" compatible
string.  The SoC DT data would include both the SoC-specific
"sifive,fu540-c000-pwm0" and the common interface "sifive,pwm0".  But
the driver would only need the SoC-specific compatible string if the SoC
wound up needing some SoC-specific quirks.


In the past, some folks have had a problem with that idea, since for
closed-source IP blocks, it's been difficult to determine what changes
went into a specific version of the IP block.  Thus folks generating
data for later SoCs usually specify a compatible string for another,
older, SoC that seems to have the desired behavior.  But since this
particular IP block has open-source RTL, and contains a "sifive,pwmX"
version string in the RTL itself:


https://github.com/sifive/sifive-blocks/blob/master/src/main/scala/devices/pwm/PWM.scala#L74


... it's straightforward to see what interface the hardware exposes to
the software for a given compatible string.


> Is there a need to have two compatible strings
> that refer to the exact same hardware?


There's no intention that "sifive,pwm0" and "sifive,fu540-c000-pwm0"
refer to the same hardware; just the same software interface and
programming model.  Even now, it's usually pretty unlikely that two
different SoCs that refer to (say) "nvidia,tegra20-pwm" contain the same
hardware, since differences in synthesis, place and route, ECOs, and
integration change the actual realization of the hardware.  Some folks
interpreted that compatible string reuse as implying the same "hardware"
is in use on both SoCs, but we're really just identifying a software
interface.


>> As to whether there could ever be a FU540-C000 part with different IP block
>> versions on it: FU540-C000 is ultimately a marketing name.  While
>> theoretically we shouldn't have another "FU540-C000" chip with different
>> peripheral IP block versions on it, I don't think any engineer can guarantee
>> that it won't happen.
> I would argue that if at some point there was indeed a chip with the
> same name but a different IP block version in it, we can figure out what
> to call it. Sure there are no guarantees, but it's still fairly unlikely
> in my opinion, so I personally wouldn't worry about this up front.
>
> Anyway, I don't feel strongly either way, I'm just pointing out that
> this is somewhat unusual. If you want to keep it, feel free to.


Thanks for the review, Thierry -


- Paul

Patch

diff --git a/Documentation/devicetree/bindings/pwm/pwm-sifive.txt b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
new file mode 100644
index 00000000..532b10fc
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
@@ -0,0 +1,32 @@ 
+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 one of
+	"sifive,fu540-c000-pwm0","sifive,pwm0".
+	PWM controller is HiFive Unleashed specific chip which warrants a
+	specific compatible string. The second string is kept for backward
+	compatibility until a firmware update with latest compatible string.
+- 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,fu540-c000-pwm0","sifive,pwm0";
+	reg = <0x0 0x10020000 0x0 0x1000>;
+	clocks = <&tlclk>;
+	interrupt-parent = <&plic>;
+	interrupts = <42 43 44 45>;
+	#pwm-cells = <2>;
+	sifive,approx-period = <1000000>;
+};