Re: [PATCH v5 06/12] dt-bindings: pwm: add microchip corepwm binding
From: Conor Dooley
Date: Sat Feb 05 2022 - 07:49:03 EST
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, Conor.Dooley@xxxxxxxxxxxxx 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 <Conor.Dooley@xxxxxxxxxxxxx> wrote:
On 02/02/2022 13:28, Geert Uytterhoeven wrote:
On Wed, Feb 2, 2022 at 1:33 PM <conor.dooley@xxxxxxxxxxxxx> 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, conor.dooley@xxxxxxxxxxxxx wrote:
From: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
Add device tree bindings for the Microchip fpga fabric based "core" PWM
controller.
Reviewed-by: Rob Herring <robh@xxxxxxxxxx>
Signed-off-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
---
.../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.