Hi Sean, > -----Original Message----- > From: Sean Anderson <sean.anderson@xxxxxxxx> > Sent: Tuesday, December 14, 2021 9:16 PM > To: Mubin Usman Sayyed <MUBINUSM@xxxxxxxxxx>; Michal Simek > <monstr@xxxxxxxxx>; linux-pwm@xxxxxxxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx; Thierry Reding <thierry.reding@xxxxxxxxx> > Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Alvaro Gamez > <alvaro.gamez@xxxxxxxxxx>; Lee Jones <lee.jones@xxxxxxxxxx>; Uwe > Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>; Michal Simek > <michals@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx > Subject: Re: [PATCH v11 2/2] pwm: Add support for Xilinx AXI Timer > > > > On 12/14/21 4:08 AM, Mubin Usman Sayyed wrote: > > Hi Sean, > > > >> -----Original Message----- > >> From: Michal Simek <monstr@xxxxxxxxx> > >> Sent: Tuesday, December 14, 2021 1:38 PM > >> To: Sean Anderson <sean.anderson@xxxxxxxx>; linux- > >> pwm@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Thierry Reding > >> <thierry.reding@xxxxxxxxx> > >> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Alvaro Gamez > >> <alvaro.gamez@xxxxxxxxxx>; Lee Jones <lee.jones@xxxxxxxxxx>; Uwe > >> Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>; Michal Simek > >> <michals@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; Mubin Usman > >> Sayyed <MUBINUSM@xxxxxxxxxx> > >> Subject: Re: [PATCH v11 2/2] pwm: Add support for Xilinx AXI Timer > >> > >> +Mubin > >> > >> On 11/24/21 00:25, Sean Anderson wrote: > >> > This adds PWM support for Xilinx LogiCORE IP AXI soft timers > >> > commonly found on Xilinx FPGAs. At the moment clock control is very > >> > basic: we just enable the clock during probe and pin the frequency. > >> > In the future, someone could add support for disabling the clock when > not in use. > >> > > >> > Some common code has been specially demarcated. While currently > >> > only used by the PWM driver, it is anticipated that it may be split > >> > off in the future to be used by the timer driver as well. > >> > > >> > This driver was written with reference to Xilinx DS764 for v1.03.a [1]. > >> > > >> > [1] > >> > > >> > https://www.xilinx.com/support/documentation/ip_documentation/axi_tim > >> e > >> > r/v1_03_a/axi_timer_ds764.pdf > >> > > >> > Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxx> > >> > Acked-by: Michal Simek <michal.simek@xxxxxxxxxx> > >> > --- > >> > > >> > Changes in v11: > >> > - Add comment about why we test for #pwm-cells > >> > - Clarify comment on generate out signal > >> > - Rename pwm variables to xilinx_pwm > >> > - Round like Uwe wants... > >> > - s/xilinx_timer/xilinx_pwm/ for non-common functions > >> > > >> > Changes in v10: > >> > - Fix compilation error in timer driver > >> > > >> > Changes in v9: > >> > - Refactor "if { return } else if { }" to "if { return } if { }" > >> > - Remove drivers/clocksource/timer-xilinx-common.c from > MAINTAINERS > >> > - Remove xilinx_timer_common_init and integrate it into > >> > xilinx_timer_probe > >> > > >> > Changes in v8: > >> > - Drop new timer driver; it has been deferred for future series > >> > > >> > Changes in v7: > >> > - Add dependency on OF_ADDRESS > >> > - Fix period_cycles calculation > >> > - Fix typo in limitations > >> > > >> > Changes in v6: > >> > - Capitalize error messages > >> > - Don't disable regmap locking to allow inspection of registers via > >> > debugfs > >> > - Prevent overflow when calculating period_cycles > >> > - Remove enabled variable from xilinx_pwm_apply > >> > - Swap order of period_cycle range comparisons > >> > > >> > Changes in v5: > >> > - Allow non-zero #pwm-cells > >> > - Correctly set duty_cycle in get_state when TLR0=TLR1 > >> > - Elaborate on limitation section > >> > - Perform some additional checks/rounding in apply_state > >> > - Remove xlnx,axi-timer-2.0 compatible string > >> > - Rework duty-cycle and period calculations with feedback from Uwe > >> > - Switch to regmap to abstract endianness issues > >> > - Use more verbose error messages > >> > > >> > Changes in v4: > >> > - Don't use volatile in read/write replacements. Some arches have it and > >> > some don't. > >> > - Put common timer properties into their own struct to better reuse > >> > code. > >> > - Remove references to properties which are not good enough for > Linux. > >> > > >> > Changes in v3: > >> > - Add clockevent and clocksource support > >> > - Remove old microblaze driver > >> > - Rewrite probe to only use a device_node, since timers may need to be > >> > initialized before we have proper devices. This does bloat the code a > bit > >> > since we can no longer rely on helpers such as dev_err_probe. We also > >> > cannot rely on device resources being free'd on failure, so we must > free > >> > them manually. > >> > - We now access registers through xilinx_timer_(read|write). This > allows us > >> > to deal with endianness issues, as originally seen in the microblaze > >> > driver. CAVEAT EMPTOR: I have not tested this on big-endian! > >> > > >> > Changes in v2: > >> > - Add comment describing device > >> > - Add comment explaining why we depend on !MICROBLAZE > >> > - Add dependencies on COMMON_CLK and HAS_IOMEM > >> > - Cast dividends to u64 to avoid overflow > >> > - Check for over- and underflow when calculating TLR > >> > - Check range of xlnx,count-width > >> > - Don't compile this module by default for arm64 > >> > - Don't set pwmchip.base to -1 > >> > - Ensure the clock is always running when the pwm is registered > >> > - Remove debugfs file :l > >> > - Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR) > >> > - Report errors with dev_error_probe > >> > - Set xilinx_pwm_ops.owner > >> > - Use NSEC_TO_SEC instead of defining our own > >> > - Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested > by > >> > Uwe > >> > > >> > MAINTAINERS | 6 + > >> > arch/microblaze/kernel/timer.c | 3 + > >> > drivers/pwm/Kconfig | 14 ++ > >> > drivers/pwm/Makefile | 1 + > >> > drivers/pwm/pwm-xilinx.c | 318 > +++++++++++++++++++++++++++++ > >> > include/clocksource/timer-xilinx.h | 91 +++++++++ > >> > 6 files changed, 433 insertions(+) > >> > create mode 100644 drivers/pwm/pwm-xilinx.c > >> > create mode 100644 include/clocksource/timer-xilinx.h > >> > > >> > diff --git a/MAINTAINERS b/MAINTAINERS index > >> > 5250298d2817..b2b3ce106e99 100644 > >> > --- a/MAINTAINERS > >> > +++ b/MAINTAINERS > >> > @@ -20897,6 +20897,12 @@ F: drivers/misc/Makefile > >> > F: drivers/misc/xilinx_sdfec.c > >> > F: include/uapi/misc/xilinx_sdfec.h > >> > > >> > +XILINX PWM DRIVER > >> > +M: Sean Anderson <sean.anderson@xxxxxxxx> > >> > +S: Maintained > >> > +F: drivers/pwm/pwm-xilinx.c > >> > +F: include/clocksource/timer-xilinx.h > >> > + > >> > XILINX UARTLITE SERIAL DRIVER > >> > M: Peter Korsgaard <jacmet@xxxxxxxxxx> > >> > L: linux-serial@xxxxxxxxxxxxxxx > >> > diff --git a/arch/microblaze/kernel/timer.c > >> > b/arch/microblaze/kernel/timer.c index f8832cf49384..dea34a3d4aa4 > >> > 100644 > >> > --- a/arch/microblaze/kernel/timer.c > >> > +++ b/arch/microblaze/kernel/timer.c > >> > @@ -251,6 +251,9 @@ static int __init xilinx_timer_init(struct > >> > device_node > >> *timer) > >> > u32 timer_num = 1; > >> > int ret; > >> > > >> > + if (of_property_read_bool(timer, "#pwm-cells")) > >> > + return 0; > > [Mubin]: Can you please return -ENODEV here, PWM driver would not be > probed if return value is 0. > > This needs to return 0 so that timer_probe doesn't spuriously announce the > "error". [Mubin]: I am trying similar implementation for cadence TTC timer. I observed that, pwm driver is not getting probed, if 0 is returned from clocksource driver (based on pwm-cells property). Thanks, Mubin > > --Sean
