Hi, On 5/4/21 6:13 PM, Uwe Kleine-König wrote: > Hello Sean, > > [Adding Rob to the list of recipents, as he for sure has a valuable > opinion on this matter.] > > On Tue, May 04, 2021 at 11:57:20AM -0400, Sean Anderson wrote: >> On 5/4/21 8:32 AM, Michal Simek wrote: >>> On 5/4/21 10:51 AM, Uwe Kleine-König wrote: >>>> On Mon, May 03, 2021 at 05:44:13PM -0400, 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@xxxxxxxx> >>>>> --- >>>>> >>>>> arch/arm64/configs/defconfig | 1 + >>>>> drivers/pwm/Kconfig | 11 ++ >>>>> drivers/pwm/Makefile | 1 + >>>>> drivers/pwm/pwm-xilinx.c | 322 +++++++++++++++++++++++++++++++++++ >>>>> 4 files changed, 335 insertions(+) >>>>> create mode 100644 drivers/pwm/pwm-xilinx.c >>>>> >>>>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig >>>>> index 08c6f769df9a..81794209f287 100644 >>>>> --- a/arch/arm64/configs/defconfig >>>>> +++ b/arch/arm64/configs/defconfig >>>>> @@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y >>>>> CONFIG_PWM_SL28CPLD=m >>>>> CONFIG_PWM_SUN4I=m >>>>> CONFIG_PWM_TEGRA=m >>>>> +CONFIG_PWM_XILINX=m >>>>> CONFIG_SL28CPLD_INTC=y >>>>> CONFIG_QCOM_PDC=y >>>>> CONFIG_RESET_IMX7=y >>>> >>>> I think this should go into a separate patch once this driver is >>>> accepted. This can then go via the ARM people. >>>> >>>>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >>>>> index d3371ac7b871..01e62928f4bf 100644 >>>>> --- a/drivers/pwm/Kconfig >>>>> +++ b/drivers/pwm/Kconfig >>>>> @@ -628,4 +628,15 @@ config PWM_VT8500 >>>>> To compile this driver as a module, choose M here: the module >>>>> will be called pwm-vt8500. >>>>> >>>>> +config PWM_XILINX >>>>> + tristate "Xilinx AXI Timer PWM support" >>>>> + depends on !MICROBLAZE >>>> >>>> I don't understand this dependency. >>> >>> The dependency is clear here because microblaze has already driver for >>> this timer here arch/microblaze/kernel/timer.c. > > Then at least add a comment. I don't think that comment will really solve this. We should never duplicate driver for the same IP to two locations. Maybe this should be MFD driver. > >>> And that's exactly pointing to the way how this should be done. >>> IP itself is single or dual timer and in case of dual timer you can >>> select if there is pwm output and use it for PWM generation. >>> >>> It means it is timer with PMW together. >>> I didn't have a time but Uwe likely knows this better how to design it. >>> >>> I see that gpio-mvebu driver instantiate pwm driver. Maybe that's the >>> way to go. >> >> I think drivers/clocksource/samsung_pwm_timer.c and >> drivers/pwm/pwm-samsung.c provide another example for how to go about >> this. > > I recently had a similar problem (with code that isn't (yet) in > mainline), where a timer can be used as a counter. I chose to change the > compatible. Transferred to this example this would mean to use e.g. > > static const struct of_device_id xilinx_pwm_of_match[] = { > { .compatible = "xlnx,xps-timer-pwm-1.00.a" }, > ... > }; > > and if you want to use the hardware as a PWM, you overwrite the > compatible in your machine.dts. It is HW selection inside that IP. It means you will get dt properly when PWM output is selected. I understand that this shortcut can work but I don't think it is proper design. > Not sure however that this is nice enough to be accepted by the dt > people?! up to Rob. Thanks, Michal
