[v2,1/2] dt-bindings: pwm: Add Xilinx AXI Timer

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 properties
must be kept as they are.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v2:
- Use 32-bit addresses for example binding

 .../bindings/pwm/xlnx,axi-timer.yaml          | 91 +++++++++++++++++++
 1 file changed, 91 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml

On 5/4/21 8:49 PM, Sean Anderson wrote:
> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
> found on Xilinx FPGAs. There is another driver for this device located
> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping.
> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
> 
> [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> I tried adding a XILINX_PWM_ prefix to all the defines, but IMO it
> really hurt readability. That prefix almost doubles the size the
> defines, and is particularly excessive in something like
> XILINX_PWM_TCSR_RUN_MASK.
> 
> Changes in v2:
> - Don't compile this module by default for arm64
> - Add dependencies on COMMON_CLK and HAS_IOMEM
> - Add comment explaining why we depend on !MICROBLAZE
> - Add comment describing device
> - Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
> - Use NSEC_TO_SEC instead of defining our own
> - Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by Uwe
> - Cast dividends to u64 to avoid overflow
> - Check for over- and underflow when calculating TLR
> - Set xilinx_pwm_ops.owner
> - Don't set pwmchip.base to -1
> - Check range of xlnx,count-width
> - Ensure the clock is always running when the pwm is registered
> - Remove debugfs file :l
> - Report errors with dev_error_probe
> 
>  drivers/pwm/Kconfig      |  13 ++
>  drivers/pwm/Makefile     |   1 +
>  drivers/pwm/pwm-xilinx.c | 301 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 315 insertions(+)
>  create mode 100644 drivers/pwm/pwm-xilinx.c

Without looking below another driver which target the same IP is just
wrong that's why NACK from me.

Thanks,
Michal

On 5/4/21 8:49 PM, 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 properties
> must be kept as they are.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
> Changes in v2:
> - Use 32-bit addresses for example binding
> 
>  .../bindings/pwm/xlnx,axi-timer.yaml          | 91 +++++++++++++++++++
>  1 file changed, 91 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
> new file mode 100644
> index 000000000000..bd014134c322
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
> @@ -0,0 +1,91 @@
> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/xlnx,axi-timer.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx LogiCORE IP AXI Timer Device Tree Binding
> +
> +maintainers:
> +  - Sean Anderson <sean.anderson@seco.com>
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: xlnx,axi-timer-2.0
> +      - const: xlnx,xps-timer-1.00.a
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: s_axi_aclk
> +
> +  reg:
> +    maxItems: 1
> +
> +  xlnx,count-width:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 8
> +    maximum: 32
> +    description:
> +      The width of the counters, in bits.
> +
> +  xlnx,gen0-assert:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 0, 1 ]
> +    description:
> +      The polarity of the generateout0 signal. 0 for active-low, 1 for active-high.
> +
> +  xlnx,gen1-assert:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 0, 1 ]
> +    description:
> +      The polarity of the generateout1 signal. 0 for active-low, 1 for active-high.
> +
> +  xlnx,one-timer-only:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 0, 1 ]
> +    description:
> +      Whether only one timer is present in this block.
> +
> +  xlnx,trig0-assert:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 0, 1 ]
> +    description:
> +      The polarity of the capturetrig0 signal. 0 for active-low, 1 for active-high.
> +
> +  xlnx,trig1-assert:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 0, 1 ]
> +    description:
> +      The polarity of the capturetrig1 signal. 0 for active-low, 1 for active-high.
> +

Based on xilinx design tool selection there is also mode_64bit option
which I expect will be translate to xlnx,mode-64bit [0, 1].
But any coverage of this as bool property should be fine.

> +required:
> +  - compatible
> +  - clocks
> +  - reg
> +  - xlnx,count-width
> +  - xlnx,gen0-assert
> +  - xlnx,gen1-assert

these 3 shouldn't be required.

> +  - xlnx,one-timer-only
> +  - xlnx,trig0-assert
> +  - xlnx,trig1-assert

these 2 are also not required.


> +
> +additionalProperties: true
> +
> +examples:
> +  - |
> +    axi_timer_0: timer@800e0000 {
> +        clock-frequency = <99999001>;

I can't see this listed above. It is allowed to have additional
properties but I don't think it is good to list it here.

> +        clock-names = "s_axi_aclk";
> +        clocks = <&zynqmp_clk 71>;
> +        compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
> +        reg = <0x800e0000 0x10000>;
> +        xlnx,count-width = <0x20>;
> +        xlnx,gen0-assert = <0x1>;
> +        xlnx,gen1-assert = <0x1>;
> +        xlnx,one-timer-only = <0x0>;
> +        xlnx,trig0-assert = <0x1>;
> +        xlnx,trig1-assert = <0x1>;
> +    };
> 

Thanks,
Michal

On 5/5/21 2:46 AM, Michal Simek wrote:
 >
 >
 > On 5/4/21 8:49 PM, 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 properties
 >> must be kept as they are.
 >>
 >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
 >> ---
 >>
 >> Changes in v2:
 >> - Use 32-bit addresses for example binding
 >>
 >>   .../bindings/pwm/xlnx,axi-timer.yaml          | 91 +++++++++++++++++++
 >>   1 file changed, 91 insertions(+)
 >>   create mode 100644 Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
 >>
 >> diff --git a/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
 >> new file mode 100644
 >> index 000000000000..bd014134c322
 >> --- /dev/null
 >> +++ b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
 >> @@ -0,0 +1,91 @@
 >> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
 >> +%YAML 1.2
 >> +---
 >> +$id: http://devicetree.org/schemas/pwm/xlnx,axi-timer.yaml#
 >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
 >> +
 >> +title: Xilinx LogiCORE IP AXI Timer Device Tree Binding
 >> +
 >> +maintainers:
 >> +  - Sean Anderson <sean.anderson@seco.com>
 >> +
 >> +properties:
 >> +  compatible:
 >> +    items:
 >> +      - const: xlnx,axi-timer-2.0
 >> +      - const: xlnx,xps-timer-1.00.a
 >> +
 >> +  clocks:
 >> +    maxItems: 1
 >> +
 >> +  clock-names:
 >> +    const: s_axi_aclk
 >> +
 >> +  reg:
 >> +    maxItems: 1
 >> +
 >> +  xlnx,count-width:
 >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >> +    minimum: 8
 >> +    maximum: 32
 >> +    description:
 >> +      The width of the counters, in bits.
 >> +
 >> +  xlnx,gen0-assert:
 >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >> +    enum: [ 0, 1 ]
 >> +    description:
 >> +      The polarity of the generateout0 signal. 0 for active-low, 1 for active-high.
 >> +
 >> +  xlnx,gen1-assert:
 >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >> +    enum: [ 0, 1 ]
 >> +    description:
 >> +      The polarity of the generateout1 signal. 0 for active-low, 1 for active-high.
 >> +
 >> +  xlnx,one-timer-only:
 >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >> +    enum: [ 0, 1 ]
 >> +    description:
 >> +      Whether only one timer is present in this block.
 >> +
 >> +  xlnx,trig0-assert:
 >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >> +    enum: [ 0, 1 ]
 >> +    description:
 >> +      The polarity of the capturetrig0 signal. 0 for active-low, 1 for active-high.
 >> +
 >> +  xlnx,trig1-assert:
 >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >> +    enum: [ 0, 1 ]
 >> +    description:
 >> +      The polarity of the capturetrig1 signal. 0 for active-low, 1 for active-high.
 >> +
 >
 > Based on xilinx design tool selection there is also mode_64bit option
 > which I expect will be translate to xlnx,mode-64bit [0, 1].
 > But any coverage of this as bool property should be fine.

I believe that just selects count-width=32 and one-timer-only=0. From
the data sheet, there doesn't appear to be a separate mode-64bit
parameter.

 >
 >> +required:
 >> +  - compatible
 >> +  - clocks
 >> +  - reg
 >> +  - xlnx,count-width
 >> +  - xlnx,gen0-assert
 >> +  - xlnx,gen1-assert
 >
 > these 3 shouldn't be required.

Count width is certainly required so that we can determine the correct
value to program into TLR. For the PWM driver, gen?-assert are required
to determine whether PWM mode is enabled.

 >
 >> +  - xlnx,one-timer-only
 >> +  - xlnx,trig0-assert
 >> +  - xlnx,trig1-assert
 >
 > these 2 are also not required.

These are not currently required by the driver, but might be in the
future if capture mode support is enabled. In general, since these
properties cannot be determined from the hardware, I think they should
be present in the devicetree.

 >
 >
 >> +
 >> +additionalProperties: true
 >> +
 >> +examples:
 >> +  - |
 >> +    axi_timer_0: timer@800e0000 {
 >> +        clock-frequency = <99999001>;
 >
 > I can't see this listed above. It is allowed to have additional
 > properties but I don't think it is good to list it here.

This is just the direct output of Xilinx's generated device tree (but
with address width reduced to 32-bit).

--Sean

 >
 >> +        clock-names = "s_axi_aclk";
 >> +        clocks = <&zynqmp_clk 71>;
 >> +        compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
 >> +        reg = <0x800e0000 0x10000>;
 >> +        xlnx,count-width = <0x20>;
 >> +        xlnx,gen0-assert = <0x1>;
 >> +        xlnx,gen1-assert = <0x1>;
 >> +        xlnx,one-timer-only = <0x0>;
 >> +        xlnx,trig0-assert = <0x1>;
 >> +        xlnx,trig1-assert = <0x1>;
 >> +    };
 >>
 >
 > Thanks,
 > Michal
 >

On 5/5/21 2:37 AM, Michal Simek wrote:
 >
 >
 > On 5/4/21 8:49 PM, Sean Anderson wrote:
 >> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
 >> found on Xilinx FPGAs. There is another driver for this device located
 >> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping.
 >> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
 >>
 >> [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
 >>
 >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
 >> ---
 >> I tried adding a XILINX_PWM_ prefix to all the defines, but IMO it
 >> really hurt readability. That prefix almost doubles the size the
 >> defines, and is particularly excessive in something like
 >> XILINX_PWM_TCSR_RUN_MASK.
 >>
 >> Changes in v2:
 >> - Don't compile this module by default for arm64
 >> - Add dependencies on COMMON_CLK and HAS_IOMEM
 >> - Add comment explaining why we depend on !MICROBLAZE
 >> - Add comment describing device
 >> - Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
 >> - Use NSEC_TO_SEC instead of defining our own
 >> - Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by Uwe
 >> - Cast dividends to u64 to avoid overflow
 >> - Check for over- and underflow when calculating TLR
 >> - Set xilinx_pwm_ops.owner
 >> - Don't set pwmchip.base to -1
 >> - Check range of xlnx,count-width
 >> - Ensure the clock is always running when the pwm is registered
 >> - Remove debugfs file :l
 >> - Report errors with dev_error_probe
 >>
 >>   drivers/pwm/Kconfig      |  13 ++
 >>   drivers/pwm/Makefile     |   1 +
 >>   drivers/pwm/pwm-xilinx.c | 301 +++++++++++++++++++++++++++++++++++++++
 >>   3 files changed, 315 insertions(+)
 >>   create mode 100644 drivers/pwm/pwm-xilinx.c
 >
 > Without looking below another driver which target the same IP is just
 > wrong that's why NACK from me.

Can you elaborate on this position a bit more? I don't think a rework of
the microblaze driver should hold back this one. They cannot be enabled
at the same time. I think it is OK to leave the work of making them
coexist for a future series (written by someone with microblaze hardware
to test on).

--Sean

Hi,

On 5/6/21 4:28 PM, Sean Anderson wrote:
> 
> 
> On 5/5/21 2:37 AM, Michal Simek wrote:
>>
>>
>> On 5/4/21 8:49 PM, Sean Anderson wrote:
>>> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
>>> found on Xilinx FPGAs. There is another driver for this device located
>>> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping.
>>> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
>>>
>>> [1]
> https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
> 
>>>
>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>>> ---
>>> I tried adding a XILINX_PWM_ prefix to all the defines, but IMO it
>>> really hurt readability. That prefix almost doubles the size the
>>> defines, and is particularly excessive in something like
>>> XILINX_PWM_TCSR_RUN_MASK.
>>>
>>> Changes in v2:
>>> - Don't compile this module by default for arm64
>>> - Add dependencies on COMMON_CLK and HAS_IOMEM
>>> - Add comment explaining why we depend on !MICROBLAZE
>>> - Add comment describing device
>>> - Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
>>> - Use NSEC_TO_SEC instead of defining our own
>>> - Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by Uwe
>>> - Cast dividends to u64 to avoid overflow
>>> - Check for over- and underflow when calculating TLR
>>> - Set xilinx_pwm_ops.owner
>>> - Don't set pwmchip.base to -1
>>> - Check range of xlnx,count-width
>>> - Ensure the clock is always running when the pwm is registered
>>> - Remove debugfs file :l
>>> - Report errors with dev_error_probe
>>>
>>>   drivers/pwm/Kconfig      |  13 ++
>>>   drivers/pwm/Makefile     |   1 +
>>>   drivers/pwm/pwm-xilinx.c | 301 +++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 315 insertions(+)
>>>   create mode 100644 drivers/pwm/pwm-xilinx.c
>>
>> Without looking below another driver which target the same IP is just
>> wrong that's why NACK from me.
> 
> Can you elaborate on this position a bit more? I don't think a rework of
> the microblaze driver should hold back this one. They cannot be enabled
> at the same time. I think it is OK to leave the work of making them
> coexist for a future series (written by someone with microblaze hardware
> to test on).

I am here to test it on Microblaze. In a lot of cases you don't have
access to all HW you should test things on but that's why others can
help with this.
As I said in previous thread driver duplication is not good way to go
and never was.

This patch targets axi timer IP which is already in the tree just for
Microblaze. You want to use it on other HW which is good but it needs to
be done properly which is not create another copy.
The right way is to get axi timer out of arch/microblaze to
drivers/clocksource (or any other driver folder) and add PMW
functionality on the top of it.
I would expect that PWM guys will say how to add PWM support to timer
driver which is not unique configuration.

Thanks,
Michal

On Tue, May 04, 2021 at 02:49:24PM -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 properties
> must be kept as they are.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
> Changes in v2:
> - Use 32-bit addresses for example binding
> 
>  .../bindings/pwm/xlnx,axi-timer.yaml          | 91 +++++++++++++++++++
>  1 file changed, 91 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
> new file mode 100644
> index 000000000000..bd014134c322
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
> @@ -0,0 +1,91 @@
> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/xlnx,axi-timer.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx LogiCORE IP AXI Timer Device Tree Binding
> +
> +maintainers:
> +  - Sean Anderson <sean.anderson@seco.com>
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: xlnx,axi-timer-2.0
> +      - const: xlnx,xps-timer-1.00.a
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: s_axi_aclk
> +
> +  reg:
> +    maxItems: 1
> +
> +  xlnx,count-width:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 8
> +    maximum: 32
> +    description:
> +      The width of the counters, in bits.
> +
> +  xlnx,gen0-assert:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 0, 1 ]
> +    description:
> +      The polarity of the generateout0 signal. 0 for active-low, 1 for active-high.
> +
> +  xlnx,gen1-assert:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 0, 1 ]
> +    description:
> +      The polarity of the generateout1 signal. 0 for active-low, 1 for active-high.
> +
> +  xlnx,one-timer-only:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 0, 1 ]
> +    description:
> +      Whether only one timer is present in this block.
> +
> +  xlnx,trig0-assert:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 0, 1 ]
> +    description:
> +      The polarity of the capturetrig0 signal. 0 for active-low, 1 for active-high.
> +
> +  xlnx,trig1-assert:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 0, 1 ]
> +    description:
> +      The polarity of the capturetrig1 signal. 0 for active-low, 1 for active-high.

Can't all these be boolean?

> +
> +required:
> +  - compatible
> +  - clocks
> +  - reg
> +  - xlnx,count-width
> +  - xlnx,gen0-assert
> +  - xlnx,gen1-assert
> +  - xlnx,one-timer-only
> +  - xlnx,trig0-assert
> +  - xlnx,trig1-assert
> +
> +additionalProperties: true
> +
> +examples:
> +  - |
> +    axi_timer_0: timer@800e0000 {
> +        clock-frequency = <99999001>;
> +        clock-names = "s_axi_aclk";
> +        clocks = <&zynqmp_clk 71>;
> +        compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
> +        reg = <0x800e0000 0x10000>;
> +        xlnx,count-width = <0x20>;
> +        xlnx,gen0-assert = <0x1>;
> +        xlnx,gen1-assert = <0x1>;
> +        xlnx,one-timer-only = <0x0>;
> +        xlnx,trig0-assert = <0x1>;
> +        xlnx,trig1-assert = <0x1>;
> +    };
> -- 
> 2.25.1
>

On 5/6/21 5:05 PM, Rob Herring wrote:
 > On Tue, May 04, 2021 at 02:49:24PM -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 properties
 >> must be kept as they are.
 >>
 >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
 >> ---
 >>
 >> Changes in v2:
 >> - Use 32-bit addresses for example binding
 >>
 >>   .../bindings/pwm/xlnx,axi-timer.yaml          | 91 +++++++++++++++++++
 >>   1 file changed, 91 insertions(+)
 >>   create mode 100644 Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
 >>
 >> diff --git a/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
 >> new file mode 100644
 >> index 000000000000..bd014134c322
 >> --- /dev/null
 >> +++ b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
 >> @@ -0,0 +1,91 @@
 >> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
 >> +%YAML 1.2
 >> +---
 >> +$id: http://devicetree.org/schemas/pwm/xlnx,axi-timer.yaml#
 >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
 >> +
 >> +title: Xilinx LogiCORE IP AXI Timer Device Tree Binding
 >> +
 >> +maintainers:
 >> +  - Sean Anderson <sean.anderson@seco.com>
 >> +
 >> +properties:
 >> +  compatible:
 >> +    items:
 >> +      - const: xlnx,axi-timer-2.0
 >> +      - const: xlnx,xps-timer-1.00.a
 >> +
 >> +  clocks:
 >> +    maxItems: 1
 >> +
 >> +  clock-names:
 >> +    const: s_axi_aclk
 >> +
 >> +  reg:
 >> +    maxItems: 1
 >> +
 >> +  xlnx,count-width:
 >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >> +    minimum: 8
 >> +    maximum: 32
 >> +    description:
 >> +      The width of the counters, in bits.
 >> +
 >> +  xlnx,gen0-assert:
 >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >> +    enum: [ 0, 1 ]
 >> +    description:
 >> +      The polarity of the generateout0 signal. 0 for active-low, 1 for active-high.
 >> +
 >> +  xlnx,gen1-assert:
 >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >> +    enum: [ 0, 1 ]
 >> +    description:
 >> +      The polarity of the generateout1 signal. 0 for active-low, 1 for active-high.
 >> +
 >> +  xlnx,one-timer-only:
 >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >> +    enum: [ 0, 1 ]
 >> +    description:
 >> +      Whether only one timer is present in this block.
 >> +
 >> +  xlnx,trig0-assert:
 >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >> +    enum: [ 0, 1 ]
 >> +    description:
 >> +      The polarity of the capturetrig0 signal. 0 for active-low, 1 for active-high.
 >> +
 >> +  xlnx,trig1-assert:
 >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >> +    enum: [ 0, 1 ]
 >> +    description:
 >> +      The polarity of the capturetrig1 signal. 0 for active-low, 1 for active-high.
 >
 > Can't all these be boolean?

They could, but

 > This binding is usually automatically generated by Xilinx's tools, so
 > the names and values of properties must be kept as they are.

Because this is a soft device, the binding may be (very conveniently)
auto-generated. I am not opposed to adding additional properties which
could be used by new code, but we should still accept this auto-generated
output.

--Sean

 >
 >> +
 >> +required:
 >> +  - compatible
 >> +  - clocks
 >> +  - reg
 >> +  - xlnx,count-width
 >> +  - xlnx,gen0-assert
 >> +  - xlnx,gen1-assert
 >> +  - xlnx,one-timer-only
 >> +  - xlnx,trig0-assert
 >> +  - xlnx,trig1-assert
 >> +
 >> +additionalProperties: true
 >> +
 >> +examples:
 >> +  - |
 >> +    axi_timer_0: timer@800e0000 {
 >> +        clock-frequency = <99999001>;
 >> +        clock-names = "s_axi_aclk";
 >> +        clocks = <&zynqmp_clk 71>;
 >> +        compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
 >> +        reg = <0x800e0000 0x10000>;
 >> +        xlnx,count-width = <0x20>;
 >> +        xlnx,gen0-assert = <0x1>;
 >> +        xlnx,gen1-assert = <0x1>;
 >> +        xlnx,one-timer-only = <0x0>;
 >> +        xlnx,trig0-assert = <0x1>;
 >> +        xlnx,trig1-assert = <0x1>;
 >> +    };
 >> --
 >> 2.25.1
 >>

On 5/6/21 12:54 PM, Michal Simek wrote:
 > Hi,
 >
 > On 5/6/21 4:28 PM, Sean Anderson wrote:
 >>
 >>
 >> On 5/5/21 2:37 AM, Michal Simek wrote:
 >>>
 >>>
 >>> On 5/4/21 8:49 PM, Sean Anderson wrote:
 >>>> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
 >>>> found on Xilinx FPGAs. There is another driver for this device located
 >>>> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping.
 >>>> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
 >>>>
 >>>> [1]
 >> https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
 >>
 >>>>
 >>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
 >>>> ---
 >>>> I tried adding a XILINX_PWM_ prefix to all the defines, but IMO it
 >>>> really hurt readability. That prefix almost doubles the size the
 >>>> defines, and is particularly excessive in something like
 >>>> XILINX_PWM_TCSR_RUN_MASK.
 >>>>
 >>>> Changes in v2:
 >>>> - Don't compile this module by default for arm64
 >>>> - Add dependencies on COMMON_CLK and HAS_IOMEM
 >>>> - Add comment explaining why we depend on !MICROBLAZE
 >>>> - Add comment describing device
 >>>> - Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
 >>>> - Use NSEC_TO_SEC instead of defining our own
 >>>> - Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by Uwe
 >>>> - Cast dividends to u64 to avoid overflow
 >>>> - Check for over- and underflow when calculating TLR
 >>>> - Set xilinx_pwm_ops.owner
 >>>> - Don't set pwmchip.base to -1
 >>>> - Check range of xlnx,count-width
 >>>> - Ensure the clock is always running when the pwm is registered
 >>>> - Remove debugfs file :l
 >>>> - Report errors with dev_error_probe
 >>>>
 >>>>     drivers/pwm/Kconfig      |  13 ++
 >>>>     drivers/pwm/Makefile     |   1 +
 >>>>     drivers/pwm/pwm-xilinx.c | 301 +++++++++++++++++++++++++++++++++++++++
 >>>>     3 files changed, 315 insertions(+)
 >>>>     create mode 100644 drivers/pwm/pwm-xilinx.c
 >>>
 >>> Without looking below another driver which target the same IP is just
 >>> wrong that's why NACK from me.
 >>
 >> Can you elaborate on this position a bit more? I don't think a rework of
 >> the microblaze driver should hold back this one. They cannot be enabled
 >> at the same time. I think it is OK to leave the work of making them
 >> coexist for a future series (written by someone with microblaze hardware
 >> to test on).
 >
 > I am here to test it on Microblaze. In a lot of cases you don't have
 > access to all HW you should test things on but that's why others can
 > help with this.

Ok, can you convert the microblaze driver then? I'm afraid I can't work
on a driver if I don't have a system to test it on. There are too many
small bugs which can creep in without anything to work with. If you are
insistant that there must be no driver duplication (even temporarily),
then you should help with the deduplication :)

I would also be willing to try and get a microblaze qemu setup working,
but I have found no good instructions for doing so with mainline linux.
The best I found was [1]. Do you have a working setup for this?

--Sean

[1] https://blog.waldemar-brodkorb.de/index.php?/archives/10-qemu-microblaze-system-emulation-tipps.html

 > As I said in previous thread driver duplication is not good way to go
 > and never was.
 >
 > This patch targets axi timer IP which is already in the tree just for
 > Microblaze. You want to use it on other HW which is good but it needs to
 > be done properly which is not create another copy.
 > The right way is to get axi timer out of arch/microblaze to
 > drivers/clocksource (or any other driver folder) and add PMW
 > functionality on the top of it.
 > I would expect that PWM guys will say how to add PWM support to timer
 > driver which is not unique configuration.
 >
 > Thanks,
 > Michal
 >

On 5/6/21 11:10 PM, Sean Anderson wrote:
> 
> 
> On 5/6/21 5:05 PM, Rob Herring wrote:
>> On Tue, May 04, 2021 at 02:49:24PM -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 properties
>>> must be kept as they are.
>>>
>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>>> ---
>>>
>>> Changes in v2:
>>> - Use 32-bit addresses for example binding
>>>
>>>   .../bindings/pwm/xlnx,axi-timer.yaml          | 91 +++++++++++++++++++
>>>   1 file changed, 91 insertions(+)
>>>   create mode 100644
> Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
>>>
>>> diff --git
> a/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
> b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
>>> new file mode 100644
>>> index 000000000000..bd014134c322
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
>>> @@ -0,0 +1,91 @@
>>> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/pwm/xlnx,axi-timer.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Xilinx LogiCORE IP AXI Timer Device Tree Binding
>>> +
>>> +maintainers:
>>> +  - Sean Anderson <sean.anderson@seco.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - const: xlnx,axi-timer-2.0
>>> +      - const: xlnx,xps-timer-1.00.a
>>> +
>>> +  clocks:
>>> +    maxItems: 1
>>> +
>>> +  clock-names:
>>> +    const: s_axi_aclk
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  xlnx,count-width:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    minimum: 8
>>> +    maximum: 32
>>> +    description:
>>> +      The width of the counters, in bits.
>>> +
>>> +  xlnx,gen0-assert:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum: [ 0, 1 ]
>>> +    description:
>>> +      The polarity of the generateout0 signal. 0 for active-low, 1
> for active-high.
>>> +
>>> +  xlnx,gen1-assert:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum: [ 0, 1 ]
>>> +    description:
>>> +      The polarity of the generateout1 signal. 0 for active-low, 1
> for active-high.
>>> +
>>> +  xlnx,one-timer-only:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum: [ 0, 1 ]
>>> +    description:
>>> +      Whether only one timer is present in this block.
>>> +
>>> +  xlnx,trig0-assert:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum: [ 0, 1 ]
>>> +    description:
>>> +      The polarity of the capturetrig0 signal. 0 for active-low, 1
> for active-high.
>>> +
>>> +  xlnx,trig1-assert:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum: [ 0, 1 ]
>>> +    description:
>>> +      The polarity of the capturetrig1 signal. 0 for active-low, 1
> for active-high.
>>
>> Can't all these be boolean?
> 
> They could, but
> 
>> This binding is usually automatically generated by Xilinx's tools, so
>> the names and values of properties must be kept as they are.
> 
> Because this is a soft device, the binding may be (very conveniently)
> auto-generated. I am not opposed to adding additional properties which
> could be used by new code, but we should still accept this auto-generated
> output.

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.

Thanks,
Michal

On 5/7/21 2:35 AM, Michal Simek wrote:
 >
 >
 > On 5/6/21 11:10 PM, Sean Anderson wrote:
 >>
 >>
 >> On 5/6/21 5:05 PM, Rob Herring wrote:
 >>> On Tue, May 04, 2021 at 02:49:24PM -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 properties
 >>>> must be kept as they are.
 >>>>
 >>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
 >>>> ---
 >>>>
 >>>> Changes in v2:
 >>>> - Use 32-bit addresses for example binding
 >>>>
 >>>>     .../bindings/pwm/xlnx,axi-timer.yaml          | 91 +++++++++++++++++++
 >>>>     1 file changed, 91 insertions(+)
 >>>>     create mode 100644
 >> Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
 >>>>
 >>>> diff --git
 >> a/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
 >> b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
 >>>> new file mode 100644
 >>>> index 000000000000..bd014134c322
 >>>> --- /dev/null
 >>>> +++ b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
 >>>> @@ -0,0 +1,91 @@
 >>>> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
 >>>> +%YAML 1.2
 >>>> +---
 >>>> +$id: http://devicetree.org/schemas/pwm/xlnx,axi-timer.yaml#
 >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
 >>>> +
 >>>> +title: Xilinx LogiCORE IP AXI Timer Device Tree Binding
 >>>> +
 >>>> +maintainers:
 >>>> +  - Sean Anderson <sean.anderson@seco.com>
 >>>> +
 >>>> +properties:
 >>>> +  compatible:
 >>>> +    items:
 >>>> +      - const: xlnx,axi-timer-2.0
 >>>> +      - const: xlnx,xps-timer-1.00.a
 >>>> +
 >>>> +  clocks:
 >>>> +    maxItems: 1
 >>>> +
 >>>> +  clock-names:
 >>>> +    const: s_axi_aclk
 >>>> +
 >>>> +  reg:
 >>>> +    maxItems: 1
 >>>> +
 >>>> +  xlnx,count-width:
 >>>> +    $ref: /schemas/types.yaml#/definitions/uint32
 >>>> +    minimum: 8
 >>>> +    maximum: 32
 >>>> +    description:
 >>>> +      The width of the counters, in bits.
 >>>> +
 >>>> +  xlnx,gen0-assert:
 >>>> +    $ref: /schemas/types.yaml#/definitions/uint32
 >>>> +    enum: [ 0, 1 ]
 >>>> +    description:
 >>>> +      The polarity of the generateout0 signal. 0 for active-low, 1
 >> for active-high.
 >>>> +
 >>>> +  xlnx,gen1-assert:
 >>>> +    $ref: /schemas/types.yaml#/definitions/uint32
 >>>> +    enum: [ 0, 1 ]
 >>>> +    description:
 >>>> +      The polarity of the generateout1 signal. 0 for active-low, 1
 >> for active-high.
 >>>> +
 >>>> +  xlnx,one-timer-only:
 >>>> +    $ref: /schemas/types.yaml#/definitions/uint32
 >>>> +    enum: [ 0, 1 ]
 >>>> +    description:
 >>>> +      Whether only one timer is present in this block.
 >>>> +
 >>>> +  xlnx,trig0-assert:
 >>>> +    $ref: /schemas/types.yaml#/definitions/uint32
 >>>> +    enum: [ 0, 1 ]
 >>>> +    description:
 >>>> +      The polarity of the capturetrig0 signal. 0 for active-low, 1
 >> for active-high.
 >>>> +
 >>>> +  xlnx,trig1-assert:
 >>>> +    $ref: /schemas/types.yaml#/definitions/uint32
 >>>> +    enum: [ 0, 1 ]
 >>>> +    description:
 >>>> +      The polarity of the capturetrig1 signal. 0 for active-low, 1
 >> for active-high.
 >>>
 >>> Can't all these be boolean?
 >>
 >> They could, but
 >>
 >>> This binding is usually automatically generated by Xilinx's tools, so
 >>> the names and values of properties must be kept as they are.
 >>
 >> Because this is a soft device, the binding may be (very conveniently)
 >> auto-generated. I am not opposed to adding additional properties which
 >> could be used by new code, but we should still accept this auto-generated
 >> output.
 >
 > 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.

Well, every single one of these is in the microblaze devicetree since
2009. And fundamentally, all of these are required for a complete driver
implementation. They are generally not discoverable from hardware
(though I think it might be possible for one-timer-only or perhaps
counter-width by inspecting whether register writes stick). However, the
signal polarity properties are required to determine whether PWM mode is
possible, and to determine the polarity of PWM capture (if that is
implemented in the future).

I think allowing more conventional usage of devicetree is a good idea.
E.g. we could accept both something like 'xlnx,gen0-assert = <0>;' and
'xlnx,gen0-active-low;'. But I think we should still parse older
devicetree properties, given the (likely extensive) amount of existing
devicetrees with this binding. And this would also require Xilinx to
adopt whatever we decide on, and update their devicetree generators
accordingly.

--Sean

Hi,

On 5/7/21 12:36 AM, Sean Anderson wrote:
> 
> 
> On 5/6/21 12:54 PM, Michal Simek wrote:
>> Hi,
>>
>> On 5/6/21 4:28 PM, Sean Anderson wrote:
>>>
>>>
>>> On 5/5/21 2:37 AM, Michal Simek wrote:
>>>>
>>>>
>>>> On 5/4/21 8:49 PM, Sean Anderson wrote:
>>>>> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
>>>>> found on Xilinx FPGAs. There is another driver for this device located
>>>>> at arch/microblaze/kernel/timer.c, but it is only used for
> timekeeping.
>>>>> This driver was written with reference to Xilinx DS764 for v1.03.a
> [1].
>>>>>
>>>>> [1]
>>>
> https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
> 
>>>
>>>>>
>>>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>>>>> ---
>>>>> I tried adding a XILINX_PWM_ prefix to all the defines, but IMO it
>>>>> really hurt readability. That prefix almost doubles the size the
>>>>> defines, and is particularly excessive in something like
>>>>> XILINX_PWM_TCSR_RUN_MASK.
>>>>>
>>>>> Changes in v2:
>>>>> - Don't compile this module by default for arm64
>>>>> - Add dependencies on COMMON_CLK and HAS_IOMEM
>>>>> - Add comment explaining why we depend on !MICROBLAZE
>>>>> - Add comment describing device
>>>>> - Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
>>>>> - Use NSEC_TO_SEC instead of defining our own
>>>>> - Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by
> Uwe
>>>>> - Cast dividends to u64 to avoid overflow
>>>>> - Check for over- and underflow when calculating TLR
>>>>> - Set xilinx_pwm_ops.owner
>>>>> - Don't set pwmchip.base to -1
>>>>> - Check range of xlnx,count-width
>>>>> - Ensure the clock is always running when the pwm is registered
>>>>> - Remove debugfs file :l
>>>>> - Report errors with dev_error_probe
>>>>>
>>>>>     drivers/pwm/Kconfig      |  13 ++
>>>>>     drivers/pwm/Makefile     |   1 +
>>>>>     drivers/pwm/pwm-xilinx.c | 301
> +++++++++++++++++++++++++++++++++++++++
>>>>>     3 files changed, 315 insertions(+)
>>>>>     create mode 100644 drivers/pwm/pwm-xilinx.c
>>>>
>>>> Without looking below another driver which target the same IP is just
>>>> wrong that's why NACK from me.
>>>
>>> Can you elaborate on this position a bit more? I don't think a rework of
>>> the microblaze driver should hold back this one. They cannot be enabled
>>> at the same time. I think it is OK to leave the work of making them
>>> coexist for a future series (written by someone with microblaze hardware
>>> to test on).
>>
>> I am here to test it on Microblaze. In a lot of cases you don't have
>> access to all HW you should test things on but that's why others can
>> help with this.
> 
> Ok, can you convert the microblaze driver then? I'm afraid I can't work
> on a driver if I don't have a system to test it on. There are too many
> small bugs which can creep in without anything to work with. If you are
> insistant that there must be no driver duplication (even temporarily),
> then you should help with the deduplication :)
> 
> I would also be willing to try and get a microblaze qemu setup working,
> but I have found no good instructions for doing so with mainline linux.
> The best I found was [1]. Do you have a working setup for this?


You can look at Guenter's files which he uses for testing here.
http://server.roeck-us.net/qemu/microblazeel/

Or you can use Xilinx petalinux distribution or Yocto layer which should
have qemu integrated.

Thanks,
Michal

On 5/10/21 6:20 AM, Michal Simek wrote:
 > Hi,
 >
 > On 5/7/21 12:36 AM, Sean Anderson wrote:
 >>
 >>
 >> On 5/6/21 12:54 PM, Michal Simek wrote:
 >>> Hi,
 >>>
 >>> On 5/6/21 4:28 PM, Sean Anderson wrote:
 >>>>
 >>>>
 >>>> On 5/5/21 2:37 AM, Michal Simek wrote:
 >>>>>
 >>>>>
 >>>>> On 5/4/21 8:49 PM, Sean Anderson wrote:
 >>>>>> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
 >>>>>> found on Xilinx FPGAs. There is another driver for this device located
 >>>>>> at arch/microblaze/kernel/timer.c, but it is only used for
 >> timekeeping.
 >>>>>> This driver was written with reference to Xilinx DS764 for v1.03.a
 >> [1].
 >>>>>>
 >>>>>> [1]
 >>>>
 >> https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
 >>
 >>>>
 >>>>>>
 >>>>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
 >>>>>> ---
 >>>>>> I tried adding a XILINX_PWM_ prefix to all the defines, but IMO it
 >>>>>> really hurt readability. That prefix almost doubles the size the
 >>>>>> defines, and is particularly excessive in something like
 >>>>>> XILINX_PWM_TCSR_RUN_MASK.
 >>>>>>
 >>>>>> Changes in v2:
 >>>>>> - Don't compile this module by default for arm64
 >>>>>> - Add dependencies on COMMON_CLK and HAS_IOMEM
 >>>>>> - Add comment explaining why we depend on !MICROBLAZE
 >>>>>> - Add comment describing device
 >>>>>> - Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
 >>>>>> - Use NSEC_TO_SEC instead of defining our own
 >>>>>> - Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by
 >> Uwe
 >>>>>> - Cast dividends to u64 to avoid overflow
 >>>>>> - Check for over- and underflow when calculating TLR
 >>>>>> - Set xilinx_pwm_ops.owner
 >>>>>> - Don't set pwmchip.base to -1
 >>>>>> - Check range of xlnx,count-width
 >>>>>> - Ensure the clock is always running when the pwm is registered
 >>>>>> - Remove debugfs file :l
 >>>>>> - Report errors with dev_error_probe
 >>>>>>
 >>>>>>       drivers/pwm/Kconfig      |  13 ++
 >>>>>>       drivers/pwm/Makefile     |   1 +
 >>>>>>       drivers/pwm/pwm-xilinx.c | 301
 >> +++++++++++++++++++++++++++++++++++++++
 >>>>>>       3 files changed, 315 insertions(+)
 >>>>>>       create mode 100644 drivers/pwm/pwm-xilinx.c
 >>>>>
 >>>>> Without looking below another driver which target the same IP is just
 >>>>> wrong that's why NACK from me.
 >>>>
 >>>> Can you elaborate on this position a bit more? I don't think a rework of
 >>>> the microblaze driver should hold back this one. They cannot be enabled
 >>>> at the same time. I think it is OK to leave the work of making them
 >>>> coexist for a future series (written by someone with microblaze hardware
 >>>> to test on).
 >>>
 >>> I am here to test it on Microblaze. In a lot of cases you don't have
 >>> access to all HW you should test things on but that's why others can
 >>> help with this.
 >>
 >> Ok, can you convert the microblaze driver then? I'm afraid I can't work
 >> on a driver if I don't have a system to test it on. There are too many
 >> small bugs which can creep in without anything to work with. If you are
 >> insistant that there must be no driver duplication (even temporarily),
 >> then you should help with the deduplication :)
 >>
 >> I would also be willing to try and get a microblaze qemu setup working,
 >> but I have found no good instructions for doing so with mainline linux.
 >> The best I found was [1]. Do you have a working setup for this?
 >
 >
 > You can look at Guenter's files which he uses for testing here.
 > http://server.roeck-us.net/qemu/microblazeel/

Thanks! These really helped when developing. In particular, I was unable
to get a working system with GCC 9, but I haven't looked into it
further.

--Sean

 >
 > Or you can use Xilinx petalinux distribution or Yocto layer which should
 > have qemu integrated.
 >
 > Thanks,
 > Michal
 >