Message-ID: <c610ce52-e1ef-393c-0948-57a4c6f07d72@conchuod.ie>
Date:   Sat, 5 Feb 2022 12:48:02 +0000
From:   Conor Dooley <mail@...chuod.ie>
To:     Conor.Dooley@...rochip.com, geert@...ux-m68k.org,
        u.kleine-koenig@...gutronix.de
Cc:     a.zummo@...ertech.it, alexandre.belloni@...tlin.com,
        aou@...s.berkeley.edu, atishp@...osinc.com, bin.meng@...driver.com,
        brgl@...ev.pl, Daire.McNamara@...rochip.com,
        devicetree@...r.kernel.org, heiko@...ech.de,
        Ivan.Griffin@...rochip.com, jassisinghbrar@...il.com,
        krzysztof.kozlowski@...onical.com, lee.jones@...aro.org,
        Lewis.Hanly@...rochip.com, linus.walleij@...aro.org,
        linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org,
        linux-riscv@...ts.infradead.org, linux-rtc@...r.kernel.org,
        palmer@...belt.com, paul.walmsley@...ive.com, robh+dt@...nel.org,
        robh@...nel.org, thierry.reding@...il.com
Subject: Re: [PATCH v5 06/12] dt-bindings: pwm: add microchip corepwm binding

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@...rochip.com 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@...rochip.com> wrote:
>>> On 02/02/2022 13:28, Geert Uytterhoeven wrote:
>>>> On Wed, Feb 2, 2022 at 1:33 PM <conor.dooley@...rochip.com> 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@...rochip.com wrote:
>>>>>>> From: Conor Dooley <conor.dooley@...rochip.com>
>>>>>>>
>>>>>>> Add device tree bindings for the Microchip fpga fabric based "core" PWM
>>>>>>> controller.
>>>>>>>
>>>>>>> Reviewed-by: Rob Herring <robh@...nel.org>
>>>>>>>
>>>>>>> Signed-off-by: Conor Dooley <conor.dooley@...rochip.com>
>>>>>>> ---
>>>>>>> .../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.
> 

Powered by blists - more mailing lists