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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.




[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux