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

[Conor.Dooley]

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 <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.

Conor.