[v5,06/12] dt-bindings: pwm: add microchip corepwm binding

Commit Message

Conor Dooley Jan. 31, 2022, 11:47 a.m. UTC

From: Conor Dooley <[e-mail]>

Add device tree bindings for the Microchip fpga fabric based "core" PWM
controller.

Reviewed-by: Rob Herring <[e-mail]>

Signed-off-by: Conor Dooley <[e-mail]>
---
 .../bindings/pwm/microchip,corepwm.yaml       | 75 +++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/microchip,corepwm.yaml

Comments

Rob Herring Jan. 31, 2022, 1:37 p.m. UTC | #1

On Mon, 31 Jan 2022 11:47:21 +0000, [e-mail] wrote:
> From: Conor Dooley <[e-mail]>
> 
> Add device tree bindings for the Microchip fpga fabric based "core" PWM
> controller.
> 
> Reviewed-by: Rob Herring <[e-mail]>
> 
> Signed-off-by: Conor Dooley <[e-mail]>
> ---
>  .../bindings/pwm/microchip,corepwm.yaml       | 75 +++++++++++++++++++
>  1 file changed, 75 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/microchip,corepwm.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/pwm/microchip,corepwm.example.dts:19:18: fatal error: dt-bindings/clock/microchip,mpfs-clock.h: No such file or directory
   19 |         #include "dt-bindings/clock/microchip,mpfs-clock.h"
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[1]: *** [scripts/Makefile.lib:378: Documentation/devicetree/bindings/pwm/microchip,corepwm.example.dt.yaml] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1398: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1586680

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Uwe Kleine-König Feb. 1, 2022, 7:58 a.m. UTC | #2

On Mon, Jan 31, 2022 at 11:47:21AM +0000, [e-mail] wrote:
> From: Conor Dooley <[e-mail]>
> 
> Add device tree bindings for the Microchip fpga fabric based "core" PWM
> controller.
> 
> Reviewed-by: Rob Herring <[e-mail]>
> 
> Signed-off-by: Conor Dooley <[e-mail]>
> ---
>  .../bindings/pwm/microchip,corepwm.yaml       | 75 +++++++++++++++++++
>  1 file changed, 75 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/microchip,corepwm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pwm/microchip,corepwm.yaml b/Documentation/devicetree/bindings/pwm/microchip,corepwm.yaml
> new file mode 100644
> index 000000000000..26a77cde2465
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/microchip,corepwm.yaml
> @@ -0,0 +1,75 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/microchip,corepwm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip ip core PWM controller bindings
> +
> +maintainers:
> +  - Conor Dooley <[e-mail]>
> +
> +description: |
> +  corePWM is an 16 channel pulse width modulator FPGA IP
> +
> +  https://www.microsemi.com/existing-parts/parts/152118
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: microchip,corepwm-rtl-v4
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  "#pwm-cells":
> +    const: 2
> +
> +  microchip,sync-update:
> +    description: |
> +      In synchronous mode, all channels are updated at the beginning of the PWM period.
> +      Asynchronous mode is relevant to applications such as LED control, where
> +      synchronous updates are not required. Asynchronous mode lowers the area size,
> +      reducing shadow register requirements. This can be set at run time, provided
> +      SHADOW_REG_EN is asserted. SHADOW_REG_EN is set by the FPGA bitstream programmed
> +      to the device.
> +      Each bit corresponds to a PWM channel & represents whether synchronous mode is
> +      possible for the PWM channel.
> +
> +    $ref: /schemas/types.yaml#/definitions/uint16
> +    default: 0

I'm not sure I understand this correctly. This is a soft-core and you
can synthesize it either with or without the ability to do synchronous
updates or not, right? All 16 channels share the same period length and
in the simple implementation changing the duty cycle is done at once
(maybe introducing a glitch) and in the more expensive implementation
there is a register to implement both variants?


> +  microchip,dac-mode:
> +    description: |
> +      Optional, per-channel Low Ripple DAC mode is possible on this IP core. It creates
> +      a minimum period pulse train whose High/Low average is that of the chosen duty
> +      cycle. This "DAC" will have far better bandwidth and ripple performance than the
> +      standard PWM algorithm can achieve.
> +      Each bit corresponds to a PWM channel & represents whether dac mode is enabled
> +      that PWM channel.

In the last sentence a "for" is missing?

These two properties are not detectable in software?

Best regards
Uwe

Conor Dooley Feb. 2, 2022, 12:35 p.m. UTC | #3

>On 01/02/2022 07:58, Uwe Kleine-König wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>> On Mon, Jan 31, 2022 at 11:47:21AM +0000, [e-mail] wrote:
>> From: Conor Dooley <[e-mail]>
>>
>> Add device tree bindings for the Microchip fpga fabric based "core" PWM
>> controller.
>>
>> Reviewed-by: Rob Herring <[e-mail]>
>>
>> Signed-off-by: Conor Dooley <[e-mail]>
>> ---
>> .../bindings/pwm/microchip,corepwm.yaml       | 75 +++++++++++++++++++

<snip>

>> +  microchip,sync-update:
>> +    description: |
>> +      In synchronous mode, all channels are updated at the beginning of the PWM period.
>> +      Asynchronous mode is relevant to applications such as LED control, where
>> +      synchronous updates are not required. Asynchronous mode lowers the area size,
>> +      reducing shadow register requirements. This can be set at run time, provided
>> +      SHADOW_REG_EN is asserted. SHADOW_REG_EN is set by the FPGA bitstream programmed
>> +      to the device.
>> +      Each bit corresponds to a PWM channel & represents whether synchronous mode is
>> +      possible for the PWM channel.
>> +
>> +    $ref: /schemas/types.yaml#/definitions/uint16
>> +    default: 0
>
>I'm not sure I understand this correctly. This is a soft-core and you
>can synthesize it either with or without the ability to do synchronous
>updates or not, right? All 16 channels share the same period length and
>in the simple implementation changing the duty cycle is done at once
>(maybe introducing a glitch) and in the more expensive implementation
>there is a register to implement both variants?

Correct. If the IP is instantiated with SHADOW_REG_ENx=1, both
registers that control the duty cycle for channel x have a second
"shadow reg" synthesised. At runtime a bit wide register exposed to
APB can be used to toggle on/off synchronised mode for all channels
it has been synthesised for.

I will reword this description since it is not clear.

>> +  microchip,dac-mode:
>> +    description: |
>> +      Optional, per-channel Low Ripple DAC mode is possible on this IP core. It creates
>> +      a minimum period pulse train whose High/Low average is that of the chosen duty
>> +      cycle. This "DAC" will have far better bandwidth and ripple performance than the
>> +      standard PWM algorithm can achieve.
>> +      Each bit corresponds to a PWM channel & represents whether dac mode is enabled
>> +      that PWM channel.
>
>In the last sentence a "for" is missing?

It is missing, thanks.

>These two properties are not detectable in software?

Unfortunately not. THe configuration for these options are only
accessible in the fpga design. You make a good point however & they
really should be visible to software. I'll suggest that for future
revisions of this IP that both configurations are accessible over APB

Thanks,
Conor.

Geert Uytterhoeven Feb. 2, 2022, 1:28 p.m. UTC | #4

Hi Conor,

On Wed, Feb 2, 2022 at 1:33 PM <[e-mail]> wrote:
> >On 01/02/2022 07:58, Uwe Kleine-König wrote:
> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >> On Mon, Jan 31, 2022 at 11:47:21AM +0000, [e-mail] wrote:
> >> From: Conor Dooley <[e-mail]>
> >>
> >> Add device tree bindings for the Microchip fpga fabric based "core" PWM
> >> controller.
> >>
> >> Reviewed-by: Rob Herring <[e-mail]>
> >>
> >> Signed-off-by: Conor Dooley <[e-mail]>
> >> ---
> >> .../bindings/pwm/microchip,corepwm.yaml       | 75 +++++++++++++++++++

> >> +  microchip,sync-update:
> >> +    description: |
> >> +      In synchronous mode, all channels are updated at the beginning of the PWM period.
> >> +      Asynchronous mode is relevant to applications such as LED control, where
> >> +      synchronous updates are not required. Asynchronous mode lowers the area size,
> >> +      reducing shadow register requirements. This can be set at run time, provided
> >> +      SHADOW_REG_EN is asserted. SHADOW_REG_EN is set by the FPGA bitstream programmed
> >> +      to the device.
> >> +      Each bit corresponds to a PWM channel & represents whether synchronous mode is
> >> +      possible for the PWM channel.
> >> +
> >> +    $ref: /schemas/types.yaml#/definitions/uint16
> >> +    default: 0
> >
> >I'm not sure I understand this correctly. This is a soft-core and you
> >can synthesize it either with or without the ability to do synchronous
> >updates or not, right? All 16 channels share the same period length and
> >in the simple implementation changing the duty cycle is done at once
> >(maybe introducing a glitch) and in the more expensive implementation
> >there is a register to implement both variants?
>
> Correct. If the IP is instantiated with SHADOW_REG_ENx=1, both
> registers that control the duty cycle for channel x have a second
> "shadow reg" synthesised. At runtime a bit wide register exposed to
> APB can be used to toggle on/off synchronised mode for all channels
> it has been synthesised for.
>
> I will reword this description since it is not clear.

Shouldn't it use a different compatible value instead?
Differentiation by properties is not recommended, as it's easy to
miss a difference.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Conor Dooley Feb. 2, 2022, 1:46 p.m. UTC | #5

On 02/02/2022 13:28, Geert Uytterhoeven wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Conor,
> 
> On Wed, Feb 2, 2022 at 1:33 PM <[e-mail]> wrote:
>>> On 01/02/2022 07:58, Uwe Kleine-König wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>> On Mon, Jan 31, 2022 at 11:47:21AM +0000, [e-mail] wrote:
>>>> From: Conor Dooley <[e-mail]>
>>>>
>>>> Add device tree bindings for the Microchip fpga fabric based "core" PWM
>>>> controller.
>>>>
>>>> Reviewed-by: Rob Herring <[e-mail]>
>>>>
>>>> Signed-off-by: Conor Dooley <[e-mail]>
>>>> ---
>>>> .../bindings/pwm/microchip,corepwm.yaml       | 75 +++++++++++++++++++
> 
>>>> +  microchip,sync-update:
>>>> +    description: |
>>>> +      In synchronous mode, all channels are updated at the beginning of the PWM period.
>>>> +      Asynchronous mode is relevant to applications such as LED control, where
>>>> +      synchronous updates are not required. Asynchronous mode lowers the area size,
>>>> +      reducing shadow register requirements. This can be set at run time, provided
>>>> +      SHADOW_REG_EN is asserted. SHADOW_REG_EN is set by the FPGA bitstream programmed
>>>> +      to the device.
>>>> +      Each bit corresponds to a PWM channel & represents whether synchronous mode is
>>>> +      possible for the PWM channel.
>>>> +
>>>> +    $ref: /schemas/types.yaml#/definitions/uint16
>>>> +    default: 0
>>>
>>> I'm not sure I understand this correctly. This is a soft-core and you
>>> can synthesize it either with or without the ability to do synchronous
>>> updates or not, right? All 16 channels share the same period length and
>>> in the simple implementation changing the duty cycle is done at once
>>> (maybe introducing a glitch) and in the more expensive implementation
>>> there is a register to implement both variants?
>>
>> Correct. If the IP is instantiated with SHADOW_REG_ENx=1, both
>> registers that control the duty cycle for channel x have a second
>> "shadow reg" synthesised. At runtime a bit wide register exposed to
>> APB can be used to toggle on/off synchronised mode for all channels
>> it has been synthesised for.
>>
>> I will reword this description since it is not clear.
> 
> Shouldn't it use a different compatible value instead?
> Differentiation by properties is not recommended, as it's easy to
> miss a difference.

Either you have something in mind that I've not thought of, or I've done 
a bad job of explaining again. The buffer/"shadow" registers are 
synthesised on a per channel basis, so any combination of the 16 
channels may have this capability. The same applies to the DAC mode, per 
channel there too.

Conor.

Geert Uytterhoeven Feb. 2, 2022, 2:02 p.m. UTC | #6

Hi Conor,

On Wed, Feb 2, 2022 at 2:46 PM <[e-mail]> wrote:
> On 02/02/2022 13:28, Geert Uytterhoeven wrote:
> > On Wed, Feb 2, 2022 at 1:33 PM <[e-mail]> wrote:
> >>> On 01/02/2022 07:58, Uwe Kleine-König wrote:
> >>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>>> On Mon, Jan 31, 2022 at 11:47:21AM +0000, [e-mail] wrote:
> >>>> From: Conor Dooley <[e-mail]>
> >>>>
> >>>> Add device tree bindings for the Microchip fpga fabric based "core" PWM
> >>>> controller.
> >>>>
> >>>> Reviewed-by: Rob Herring <[e-mail]>
> >>>>
> >>>> Signed-off-by: Conor Dooley <[e-mail]>
> >>>> ---
> >>>> .../bindings/pwm/microchip,corepwm.yaml       | 75 +++++++++++++++++++
> >
> >>>> +  microchip,sync-update:
> >>>> +    description: |
> >>>> +      In synchronous mode, all channels are updated at the beginning of the PWM period.
> >>>> +      Asynchronous mode is relevant to applications such as LED control, where
> >>>> +      synchronous updates are not required. Asynchronous mode lowers the area size,
> >>>> +      reducing shadow register requirements. This can be set at run time, provided
> >>>> +      SHADOW_REG_EN is asserted. SHADOW_REG_EN is set by the FPGA bitstream programmed
> >>>> +      to the device.
> >>>> +      Each bit corresponds to a PWM channel & represents whether synchronous mode is
> >>>> +      possible for the PWM channel.
> >>>> +
> >>>> +    $ref: /schemas/types.yaml#/definitions/uint16
> >>>> +    default: 0
> >>>
> >>> I'm not sure I understand this correctly. This is a soft-core and you
> >>> can synthesize it either with or without the ability to do synchronous
> >>> updates or not, right? All 16 channels share the same period length and
> >>> in the simple implementation changing the duty cycle is done at once
> >>> (maybe introducing a glitch) and in the more expensive implementation
> >>> there is a register to implement both variants?
> >>
> >> Correct. If the IP is instantiated with SHADOW_REG_ENx=1, both
> >> registers that control the duty cycle for channel x have a second
> >> "shadow reg" synthesised. At runtime a bit wide register exposed to
> >> APB can be used to toggle on/off synchronised mode for all channels
> >> it has been synthesised for.
> >>
> >> I will reword this description since it is not clear.
> >
> > Shouldn't it use a different compatible value instead?
> > Differentiation by properties is not recommended, as it's easy to
> > miss a difference.
>
> Either you have something in mind that I've not thought of, or I've done
> a bad job of explaining again. The buffer/"shadow" registers are
> synthesised on a per channel basis, so any combination of the 16
> channels may have this capability. The same applies to the DAC mode, per
> channel there too.

Oops, hadn't noticed this is per channel. Indeed, then a different
compatible value is futile.
So since "microchip,sync-update" is a bitmask, perhaps it should be
called "microchip,sync-update-mask"?
Same for "microchip,dac-mode" -> "microchip,dac-mode-mask"?

Also, using different integer sizes than uint32 is frowned upon, unless
there is a very good reason to do so. I can imagine a future version
would support more channels, and then uint16 becomes a limitation.

For both: Rob?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Conor Dooley Feb. 2, 2022, 2:37 p.m. UTC | #7

On 02/02/2022 14:02, Geert Uytterhoeven wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> On Wed, Feb 2, 2022 at 2:46 PM <[e-mail]> wrote:
>> On 02/02/2022 13:28, Geert Uytterhoeven wrote:
>>> On Wed, Feb 2, 2022 at 1:33 PM <[e-mail]> wrote:
>>>>> On 01/02/2022 07:58, Uwe Kleine-König wrote:
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>> On Mon, Jan 31, 2022 at 11:47:21AM +0000, [e-mail] wrote:
>>>>>> From: Conor Dooley <[e-mail]>
>>>>>>
>>>>>> Add device tree bindings for the Microchip fpga fabric based "core" PWM
>>>>>> controller.
>>>>>>
>>>>>> Reviewed-by: Rob Herring <[e-mail]>
>>>>>>
>>>>>> Signed-off-by: Conor Dooley <[e-mail]>
>>>>>> ---
>>>>>> .../bindings/pwm/microchip,corepwm.yaml       | 75 +++++++++++++++++++
>>>
>>>>>> +  microchip,sync-update:
>>>>>> +    description: |
>>>>>> +      In synchronous mode, all channels are updated at the beginning of the PWM period.
>>>>>> +      Asynchronous mode is relevant to applications such as LED control, where
>>>>>> +      synchronous updates are not required. Asynchronous mode lowers the area size,
>>>>>> +      reducing shadow register requirements. This can be set at run time, provided
>>>>>> +      SHADOW_REG_EN is asserted. SHADOW_REG_EN is set by the FPGA bitstream programmed
>>>>>> +      to the device.
>>>>>> +      Each bit corresponds to a PWM channel & represents whether synchronous mode is
>>>>>> +      possible for the PWM channel.
>>>>>> +
>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint16
>>>>>> +    default: 0
>>>>>
>>>>> I'm not sure I understand this correctly. This is a soft-core and you
>>>>> can synthesize it either with or without the ability to do synchronous
>>>>> updates or not, right? All 16 channels share the same period length and
>>>>> in the simple implementation changing the duty cycle is done at once
>>>>> (maybe introducing a glitch) and in the more expensive implementation
>>>>> there is a register to implement both variants?
>>>>
>>>> Correct. If the IP is instantiated with SHADOW_REG_ENx=1, both
>>>> registers that control the duty cycle for channel x have a second
>>>> "shadow reg" synthesised. At runtime a bit wide register exposed to
>>>> APB can be used to toggle on/off synchronised mode for all channels
>>>> it has been synthesised for.
>>>>
>>>> I will reword this description since it is not clear.
>>>
>>> Shouldn't it use a different compatible value instead?
>>> Differentiation by properties is not recommended, as it's easy to
>>> miss a difference.
>>
>> Either you have something in mind that I've not thought of, or I've done
>> a bad job of explaining again. The buffer/"shadow" registers are
>> synthesised on a per channel basis, so any combination of the 16
>> channels may have this capability. The same applies to the DAC mode, per
>> channel there too.
> 
> Oops, hadn't noticed this is per channel. Indeed, then a different
> compatible value is futile.
> So since "microchip,sync-update" is a bitmask, perhaps it should be
> called "microchip,sync-update-mask"?
> Same for "microchip,dac-mode" -> "microchip,dac-mode-mask"?

Adding -mask sounds good to me.

> Also, using different integer sizes than uint32 is frowned upon, unless
> there is a very good reason to do so. I can imagine a future version
> would support more channels, and then uint16 becomes a limitation.

Sure, uint32 it is.

> For both: Rob?

Both of these properties fall under the "DO attempt to make bindings 
complete even if a driver doesn’t support some features" category, so I 
am perfectly happy to change these properties to whatever is convention 
(or ultimately drop them for the sake of the remainder of the series).

Thanks,
Conor.

Conor Dooley Feb. 5, 2022, 12:48 p.m. UTC | #8

Geert, Uwe,

Hopefully the following does a better job of explaining the two parameters?

Thanks,
Conor.

microchip,sync-update-mask:
   description: |
     Depending on how the IP is instantiated, there are two modes of
     operation. In synchronous mode, all channels are updated at the
     beginning of the PWM period, and in asynchronous mode updates
     happen as the control registers are written. A 16 bit wide
     "SHADOW_REG_EN" parameter of the IP core controls whether
     synchronous mode is possible for each channel, and is set by the
     bitstream programmed to the FPGA. If the IP core is instantiated
     with SHADOW_REG_ENx=1, both registers that control the duty cycle
     for channel x have a second "shadow"/buffer reg synthesised.
     At runtime a bit wide register exposed to APB can be used to toggle
     on/off synchronised mode for all channels it has been synthesised
     for.
     Each bit corresponds to a PWM channel & represents whether
     synchronous mode is possible for that channel.

   $ref: /schemas/types.yaml#/definitions/uint32
   default: 0

microchip,dac-mode-mask:
   description: |
     Optional, per-channel Low Ripple DAC mode is possible on this IP
     core. It creates a minimum period pulse train whose High/Low
     average is that of the chosen duty cycle. This "DAC" will have far
     better bandwidth and ripple performance than the standard PWM
     algorithm can achieve. A 16 bit DAC_MODE module parameter of the IP
     core, set at instantiation and by the bitstream programmed to the
     FPGA, determines whether a given channel operates in regular PWM or
     DAC mode.
     Each bit corresponds to a PWM channel & represents whether DAC mode
     is enabled for that channel.

   $ref: /schemas/types.yaml#/definitions/uint32
   default: 0

On 02/02/2022 14:37, [e-mail] wrote:
> On 02/02/2022 14:02, Geert Uytterhoeven wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>> On Wed, Feb 2, 2022 at 2:46 PM <[e-mail]> wrote:
>>> On 02/02/2022 13:28, Geert Uytterhoeven wrote:
>>>> On Wed, Feb 2, 2022 at 1:33 PM <[e-mail]> wrote:
>>>>>> On 01/02/2022 07:58, Uwe Kleine-König wrote:
>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>> On Mon, Jan 31, 2022 at 11:47:21AM +0000, [e-mail] wrote:
>>>>>>> From: Conor Dooley <[e-mail]>
>>>>>>>
>>>>>>> Add device tree bindings for the Microchip fpga fabric based "core" PWM
>>>>>>> controller.
>>>>>>>
>>>>>>> Reviewed-by: Rob Herring <[e-mail]>
>>>>>>>
>>>>>>> Signed-off-by: Conor Dooley <[e-mail]>
>>>>>>> ---
>>>>>>> .../bindings/pwm/microchip,corepwm.yaml       | 75 +++++++++++++++++++
>>>>
>>>>>>> +  microchip,sync-update:
>>>>>>> +    description: |
>>>>>>> +      In synchronous mode, all channels are updated at the beginning of the PWM period.
>>>>>>> +      Asynchronous mode is relevant to applications such as LED control, where
>>>>>>> +      synchronous updates are not required. Asynchronous mode lowers the area size,
>>>>>>> +      reducing shadow register requirements. This can be set at run time, provided
>>>>>>> +      SHADOW_REG_EN is asserted. SHADOW_REG_EN is set by the FPGA bitstream programmed
>>>>>>> +      to the device.
>>>>>>> +      Each bit corresponds to a PWM channel & represents whether synchronous mode is
>>>>>>> +      possible for the PWM channel.
>>>>>>> +
>>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint16
>>>>>>> +    default: 0
>>>>>>
>>>>>> I'm not sure I understand this correctly. This is a soft-core and you
>>>>>> can synthesize it either with or without the ability to do synchronous
>>>>>> updates or not, right? All 16 channels share the same period length and
>>>>>> in the simple implementation changing the duty cycle is done at once
>>>>>> (maybe introducing a glitch) and in the more expensive implementation
>>>>>> there is a register to implement both variants?
>>>>>
>>>>> Correct. If the IP is instantiated with SHADOW_REG_ENx=1, both
>>>>> registers that control the duty cycle for channel x have a second
>>>>> "shadow reg" synthesised. At runtime a bit wide register exposed to
>>>>> APB can be used to toggle on/off synchronised mode for all channels
>>>>> it has been synthesised for.
>>>>>
>>>>> I will reword this description since it is not clear.
>>>>
>>>> Shouldn't it use a different compatible value instead?
>>>> Differentiation by properties is not recommended, as it's easy to
>>>> miss a difference.
>>>
>>> Either you have something in mind that I've not thought of, or I've done
>>> a bad job of explaining again. The buffer/"shadow" registers are
>>> synthesised on a per channel basis, so any combination of the 16
>>> channels may have this capability. The same applies to the DAC mode, per
>>> channel there too.
>>
>> Oops, hadn't noticed this is per channel. Indeed, then a different
>> compatible value is futile.
>> So since "microchip,sync-update" is a bitmask, perhaps it should be
>> called "microchip,sync-update-mask"?
>> Same for "microchip,dac-mode" -> "microchip,dac-mode-mask"?
> 
> Adding -mask sounds good to me.
> 
>> Also, using different integer sizes than uint32 is frowned upon, unless
>> there is a very good reason to do so. I can imagine a future version
>> would support more channels, and then uint16 becomes a limitation.
> 
> Sure, uint32 it is.
> 
>> For both: Rob?
> 
> Both of these properties fall under the "DO attempt to make bindings
> complete even if a driver doesn’t support some features" category, so I
> am perfectly happy to change these properties to whatever is convention
> (or ultimately drop them for the sake of the remainder of the series).
> 
> Thanks,
> Conor.
>

Patch

diff --git a/Documentation/devicetree/bindings/pwm/microchip,corepwm.yaml b/Documentation/devicetree/bindings/pwm/microchip,corepwm.yaml
new file mode 100644
index 000000000000..26a77cde2465
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/microchip,corepwm.yaml
@@ -0,0 +1,75 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/microchip,corepwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip ip core PWM controller bindings
+
+maintainers:
+  - Conor Dooley <[e-mail]>
+
+description: |
+  corePWM is an 16 channel pulse width modulator FPGA IP
+
+  https://www.microsemi.com/existing-parts/parts/152118
+
+properties:
+  compatible:
+    items:
+      - const: microchip,corepwm-rtl-v4
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 2
+
+  microchip,sync-update:
+    description: |
+      In synchronous mode, all channels are updated at the beginning of the PWM period.
+      Asynchronous mode is relevant to applications such as LED control, where
+      synchronous updates are not required. Asynchronous mode lowers the area size,
+      reducing shadow register requirements. This can be set at run time, provided
+      SHADOW_REG_EN is asserted. SHADOW_REG_EN is set by the FPGA bitstream programmed
+      to the device.
+      Each bit corresponds to a PWM channel & represents whether synchronous mode is
+      possible for the PWM channel.
+
+    $ref: /schemas/types.yaml#/definitions/uint16
+    default: 0
+
+  microchip,dac-mode:
+    description: |
+      Optional, per-channel Low Ripple DAC mode is possible on this IP core. It creates
+      a minimum period pulse train whose High/Low average is that of the chosen duty
+      cycle. This "DAC" will have far better bandwidth and ripple performance than the
+      standard PWM algorithm can achieve.
+      Each bit corresponds to a PWM channel & represents whether dac mode is enabled
+      that PWM channel.
+
+    $ref: /schemas/types.yaml#/definitions/uint16
+    default: 0
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - "#pwm-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include "dt-bindings/clock/microchip,mpfs-clock.h"
+    pwm@41000000 {
+      compatible = "microchip,corepwm-rtl-v4";
+      microchip,sync-update = /bits/ 16 <0>;
+      clocks = <&clkcfg CLK_FIC3>;
+      reg = <0x41000000 0xF0>;
+      #pwm-cells = <2>;
+    };