Re: [PATCH v3 1/2] dt-bindings: pwm: Add Xilinx AXI Timer

[Sean Anderson <sean.anderson@seco.com>]

On 5/14/21 4:50 AM, Michal Simek wrote:
 >
 >
 > On 5/13/21 10:43 PM, Rob Herring wrote:
 >> On Thu, May 13, 2021 at 10:28 AM Sean Anderson <sean.anderson@seco.com> wrote:
 >>>
 >>>
 >>>
 >>> On 5/13/21 10:33 AM, Sean Anderson wrote:
 >>>   >
 >>>   >
 >>>   > On 5/12/21 10:16 PM, Rob Herring wrote:
 >>>   >  > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
 >>>   >  >> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
 >>>   >  >> a "soft" block, so it has many parameters which would not be
 >>>   >  >> configurable in most hardware. This binding is usually automatically
 >>>   >  >> generated by Xilinx's tools, so the names and values of some properties
 >>>   >  >> must be kept as they are. Replacement properties have been provided for
 >>>   >  >> new device trees.
 >>>   >  >
 >>>   >  > Because you have some tool generating properties is not a reason we have
 >>>   >  > to accept them upstream.
 >>>   >
 >>>   > These properties are already in arch/microblaze/boot/dts/system.dts and
 >>>   > in the devicetree supplied to Linux by qemu. Removing these properties
 >>>   > will break existing setups, which I would like to avoid.
 >>
 >> Already in use in upstream dts files is different than just
 >> 'automatically generated' by vendor tools.
 >>
 >>>   >
 >>>   >  > 'deprecated' is for what *we* have deprecated.
 >>>   >
 >>>   > Ok. I will remove that then.
 >>>   >
 >>>   >  >
 >>>   >  > In this case, I don't really see the point in defining new properties
 >>>   >  > just to have bool.
 >>>   >
 >>>   > I don't either, but it was requested, by Michal...
 >>>
 >>> Err, your comment on the original bindings was
 >>>
 >>>   > Can't all these be boolean?
 >>
 >> With no other context, yes that's what I would ask. Now you've given
 >> me some context, between using the existing ones and 2 sets of
 >> properties to maintain, I choose the former.
 >>
 >>> And Michal commented
 >>>
 >>>   > I think in this case you should described what it is used by current
 >>>   > driver in Microblaze and these options are required. The rest are by
 >>>   > design optional.
 >>>   > If you want to change them to different value then current binding
 >>>   > should be deprecated and have any transition time with code alignment.
 >>>
 >>> So that is what I tried to accomplish with this revision. I also tried
 >>> allowing something like
 >>>
 >>>          xlnx,one-timer-only = <0>; /* two timers */
 >>>          xlnx,one-timer-only = <1>; /* one timer  */
 >>>          xlnx,one-timer-only; /* one timer */
 >>>          /* property absent means two timers */
 >>>
 >>> but I was unable to figure out how to express this with json-schema. I
 >>> don't think it's the best design either...
 >>
 >> json-schema would certainly let you, but generally we don't want
 >> properties to have more than 1 type.
 >
 > One thing is what it is in system.dts file which was committed in 2009
 > and there are just small alignments there. But none is really using it.
 > Maybe I should just delete it.
 > And this version was generated by Xilinx ancient tools at that time. All
 > parameters there are fully describing HW and they are not changing. Only
 > new one can be added.
 >
 >  From the current microblaze code you can see which properties are really
 > used.
 >
 > reg
 > interrupts
 > xlnx,one-timer-only
 > clocks
 > clock-frequency

There is also an implicit dependency on xlnx,count-width. Several times
the existing driver assumes the counter width is 32, but this should
instead be discovered from the devicetree.

 > It means from my point of view these should be listed in the binding.
 > clock-frequency is optional by code when clock is defined.
 >
 > All other properties listed in system.dts are from my perspective
 > optional and that's how it should be.

Here is the situation as I understand it

* This device has existed for around 15 years (since 2006)
* Because it is a soft device, there are several configurable parameters
* Although all of these parameters must be known for a complete
   implementation of this device, some are unnecessary if onlu reduced
   functionality is needed.
* A de facto devicetree binding for this device has existed for at least
   12 years (since 2009), but likely for as long as the device itself has
   existed. This binding has not changed substantially during this time.
* This binding is present in devicetrees from the Linux kernel, from
   qemu, in other existing systems, and in devicetrees generated by
   Xilinx's toolset.
* Because the existing driver for this device does not implement all
   functionality for this device, not all properties in the devicetree
   binding are used. In fact, there is (as noted above) one property
   which should be in use but is not because the current driver
   (implicitly) does not support some hardware configurations.
* To support additional functionality, it is necessary to
   use hardware parameters which were not previously necessary.

Based on the above, we can classify the properties of this binding into
several categories.

* Those which are currently read by the driver.
   * compatible
   * reg
   * clocks
   * clock-frequency
   * interrupts
   * xlnx,one-timer-only

* Those which reflect hardware parameters which are currently explicitly
   or implicitly relied upon by the driver.
   * reg
   * clocks
   * clock-frequency
   * interrupts
   * xlnx,counter-width
   * xlnx,one-timer-only

* Those which are currently present in device trees.
   * compatible
   * reg
   * interrupts
   * clocks
   * clock-frequency
   * xlnx,count-width
   * xlnx,one-timer-only
   * xlnx,trig0-assert
   * xlnx,trig1-assert
   * xlnx,gen0-assert
   * xlnx,gen1-assert

When choosing what properties to use, we must consider what the impact
of our changes will be on not just the kernel but also on existing users
of this binding:

* To use properties currently present in device trees, we just need to
   modify the kernel driver.
* To add additional properties (such as e.g. '#pwm-cells'), we must
   modify the kernel driver. In addition, users who would like to use
   these new properties must add them to their device trees. This may be
   done in a mechanical way using e.g. overlays.
* To deprecate existing properties and introduce new properties to
   expose the same underlying hardware parameters, we must modify the
   kernel driver. However, this has a large impact on existing users.
   They must modify their tools to generate this information in a
   different format. When this information is generated by upstream tools
   this may require updating a core part of their build system. For many
   projects, this may happen very infrequently because of the risk that
   such an upgrade will break things. Even if you suggest that Xilinx can
   easily modify its tools to generate any sort of output, the time for
   this upgrade to be deployed/adopted may be significantly longer.

Note that while all three types of changes are similar from a kernel
point of view, the impact on existing users is much large in the latter
case. For this reason, I think that wherever possible we should use
properties which are already present in existing device trees.

 > I think DT binding patch should reflect this state as patch itself.
 > And then PWM should be added on the top as separate patch.

I have no preference here.

--Sean

 >
 > Note: In past we were using only parameters and name we got from tools
 > but over years we were fine to use for example bool properties and we
 > just aligned Xilinx device tree generator to match it. That's why not a
 > problem to deprecate any property and move to new one. Xilinx DTG is
 > already prepared for it and it is easy to remap it.
 >
 > Thanks,
 > Michal