From: Lee Jones <lee.jones@linaro.org> To: Sean Anderson <sean.anderson@seco.com> Cc: linux-pwm@vger.kernel.org, devicetree@vger.kernel.org, michal.simek@xilinx.com, linux-kernel@vger.kernel.org, Alvaro Gamez <alvaro.gamez@hazent.com>, linux-arm-kernel@lists.infradead.org, Daniel Lezcano <daniel.lezcano@linaro.org>, Thomas Gleixner <tglx@linutronix.de> Subject: Re: [PATCH v4 2/3] clocksource: Rewrite Xilinx AXI timer driver Date: Tue, 1 Jun 2021 09:47:34 +0100 [thread overview] Message-ID: <20210601084734.GX543307@dell> (raw) In-Reply-To: <20210528214522.617435-2-sean.anderson@seco.com> On Fri, 28 May 2021, Sean Anderson wrote: > This rewrites the Xilinx AXI timer driver to be more platform agnostic. > Some common code has been split off so it can be reused. These routines > currently live in drivers/mfd. The largest changes have taken place in the > initialization: > > - We now support any number of timer devices, possibly with only one > counter each. The first counter will be used as a clocksource. Every > other counter will be used as a clockevent. > - We do not use timer_of_init because we need to perform some tasks in > between different stages. For example, we must ensure that ->read and > ->write are initialized before registering the irq. This can only happen > after we have gotten the register base (to detect endianness). We also > have a rather unusual clock initialization sequence in order to remain > backwards compatible. Due to this, it's ok for the initial clock request > to fail, and we do not want other initialization to be undone. Lastly, it > is more convenient to do one allocation for xilinx_clockevent_device than > to do one for timer_of and one for xilinx_timer_priv. > - We now pay attention to xlnx,count-width and handle smaller width timers. > The default remains 32. > > Signed-off-by: Sean Anderson <sean.anderson@seco.com> > --- > This has been tested on microblaze qemu. > > Changes in v4: > - Break out clock* drivers into their own file > > arch/microblaze/kernel/Makefile | 3 +- > arch/microblaze/kernel/timer.c | 326 ----------------------------- > drivers/clocksource/Kconfig | 11 + > drivers/clocksource/Makefile | 1 + > drivers/clocksource/timer-xilinx.c | 300 ++++++++++++++++++++++++++ > drivers/mfd/Makefile | 4 + > drivers/mfd/xilinx-timer.c | 147 +++++++++++++ I'm confused! > include/linux/mfd/xilinx-timer.h | 134 ++++++++++++ > 8 files changed, 598 insertions(+), 328 deletions(-) > delete mode 100644 arch/microblaze/kernel/timer.c > create mode 100644 drivers/clocksource/timer-xilinx.c > create mode 100644 drivers/mfd/xilinx-timer.c > create mode 100644 include/linux/mfd/xilinx-timer.h [...] > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2021 Sean Anderson <sean.anderson@seco.com> > + * > + * For Xilinx LogiCORE IP AXI Timer documentation, refer to DS764: > + * https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf > + */ > + > +#include <linux/clk.h> > +#include <linux/mfd/xilinx-timer.h> > +#include <linux/of.h> > +#include <asm/io.h> RED FLAG: You are not using the MFD API here. > +#define TCSR0 0x00 > +#define TLR0 0x04 > +#define TCR0 0x08 > +#define TCSR1 0x10 > +#define TLR1 0x14 > +#define TCR1 0x18 > + > +#define TCSR_MDT BIT(0) > +#define TCSR_UDT BIT(1) > +#define TCSR_GENT BIT(2) > +#define TCSR_CAPT BIT(3) > +#define TCSR_ARHT BIT(4) > +#define TCSR_LOAD BIT(5) > +#define TCSR_ENIT BIT(6) > +#define TCSR_ENT BIT(7) > +#define TCSR_TINT BIT(8) > +#define TCSR_PWMA BIT(9) > +#define TCSR_ENALL BIT(10) > +#define TCSR_CASC BIT(11) > + > +/* readl/writel wrappers to support BE systems */ > + > +static u32 xilinx_ioread32be(const void __iomem *addr) > +{ > + return ioread32be(addr); > +} > + > +static void xilinx_iowrite32be(u32 value, void __iomem *addr) > +{ > + iowrite32be(value, addr); > +} > + > +static u32 xilinx_ioread32(const void __iomem *addr) > +{ > + return ioread32(addr); > +} > + > +static void xilinx_iowrite32(u32 value, void __iomem *addr) > +{ > + iowrite32(value, addr); > +} Abstraction for the sake of abstraction, is not allowed. Just use the io*() calls directly in-place. > +int xilinx_timer_tlr_cycles(struct xilinx_timer_priv *priv, u32 *tlr, > + u32 tcsr, u64 cycles) > +{ > + if (cycles < 2 || cycles > priv->max + 2) > + return -ERANGE; > + > + if (tcsr & TCSR_UDT) > + *tlr = cycles - 2; > + else > + *tlr = priv->max - cycles + 2; > + > + return 0; > +} > + > +int xilinx_timer_tlr_period(struct xilinx_timer_priv *priv, u32 *tlr, > + u32 tcsr, unsigned int period) > +{ > + u64 cycles = DIV_ROUND_DOWN_ULL((u64)period * clk_get_rate(priv->clk), > + NSEC_PER_SEC); > + > + return xilinx_timer_tlr_cycles(priv, tlr, tcsr, cycles); > +} > + > +unsigned int xilinx_timer_get_period(struct xilinx_timer_priv *priv, > + u32 tlr, u32 tcsr) > +{ > + u64 cycles; > + > + if (tcsr & TCSR_UDT) > + cycles = tlr + 2; > + else > + cycles = priv->max - tlr + 2; > + > + return DIV_ROUND_UP_ULL(cycles * NSEC_PER_SEC, > + clk_get_rate(priv->clk)); > +} > + > +int xilinx_timer_common_init(struct device_node *np, > + struct xilinx_timer_priv *priv, > + u32 *one_timer) > +{ > + int ret; > + u32 tcsr0, width; > + > + > + priv->read = xilinx_ioread32; > + priv->write = xilinx_iowrite32; > + /* > + * If PWM mode is enabled, we should try not to disturb it. Use > + * CAPT since if PWM mode is enabled then MDT will be set as > + * well. > + * > + * First, clear CAPT and verify that it has been cleared > + */ > + tcsr0 = xilinx_timer_read(priv, TCSR0); > + xilinx_timer_write(priv, tcsr0 & ~(TCSR_CAPT & swab(TCSR_CAPT)), TCSR0); > + tcsr0 = xilinx_timer_read(priv, TCSR0); > + if (tcsr0 & (TCSR_CAPT | swab(TCSR_CAPT))) { > + pr_err("%pOF: cannot determine endianness\n", np); > + return -EOPNOTSUPP; > + } > + > + /* Then check to make sure our write sticks */ > + xilinx_timer_write(priv, tcsr0 | TCSR_CAPT, TCSR0); > + if (!(xilinx_timer_read(priv, TCSR0) & TCSR_CAPT)) { > + priv->read = xilinx_ioread32be; > + priv->write = xilinx_iowrite32be; > + } > + > + ret = of_property_read_u32(np, "xlnx,one-timer-only", one_timer); > + if (ret) { > + pr_err("%pOF: err %d: xlnx,one-timer-only\n", np, ret); > + return ret; > + } else if (*one_timer && *one_timer != 1) { > + pr_err("%pOF: xlnx,one-timer-only must be 0 or 1\n", np); > + return -EINVAL; > + } > + > + ret = of_property_read_u32(np, "xlnx,count-width", &width); > + if (ret == -EINVAL) { > + width = 32; > + } else if (ret) { > + pr_err("%pOF: err %d: xlnx,count-width\n", np, ret); > + return ret; > + } else if (width < 8 || width > 32) { > + pr_err("%pOF: invalid counter width\n", np); > + return -EINVAL; > + } > + priv->max = BIT_ULL(width) - 1; > + > + return 0; > +} This is *all* timer stuff. What is your rationale for dumping this into MFD? -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
WARNING: multiple messages have this Message-ID (diff)
From: Lee Jones <lee.jones@linaro.org> To: Sean Anderson <sean.anderson@seco.com> Cc: linux-pwm@vger.kernel.org, devicetree@vger.kernel.org, michal.simek@xilinx.com, linux-kernel@vger.kernel.org, Alvaro Gamez <alvaro.gamez@hazent.com>, linux-arm-kernel@lists.infradead.org, Daniel Lezcano <daniel.lezcano@linaro.org>, Thomas Gleixner <tglx@linutronix.de> Subject: Re: [PATCH v4 2/3] clocksource: Rewrite Xilinx AXI timer driver Date: Tue, 1 Jun 2021 09:47:34 +0100 [thread overview] Message-ID: <20210601084734.GX543307@dell> (raw) In-Reply-To: <20210528214522.617435-2-sean.anderson@seco.com> On Fri, 28 May 2021, Sean Anderson wrote: > This rewrites the Xilinx AXI timer driver to be more platform agnostic. > Some common code has been split off so it can be reused. These routines > currently live in drivers/mfd. The largest changes have taken place in the > initialization: > > - We now support any number of timer devices, possibly with only one > counter each. The first counter will be used as a clocksource. Every > other counter will be used as a clockevent. > - We do not use timer_of_init because we need to perform some tasks in > between different stages. For example, we must ensure that ->read and > ->write are initialized before registering the irq. This can only happen > after we have gotten the register base (to detect endianness). We also > have a rather unusual clock initialization sequence in order to remain > backwards compatible. Due to this, it's ok for the initial clock request > to fail, and we do not want other initialization to be undone. Lastly, it > is more convenient to do one allocation for xilinx_clockevent_device than > to do one for timer_of and one for xilinx_timer_priv. > - We now pay attention to xlnx,count-width and handle smaller width timers. > The default remains 32. > > Signed-off-by: Sean Anderson <sean.anderson@seco.com> > --- > This has been tested on microblaze qemu. > > Changes in v4: > - Break out clock* drivers into their own file > > arch/microblaze/kernel/Makefile | 3 +- > arch/microblaze/kernel/timer.c | 326 ----------------------------- > drivers/clocksource/Kconfig | 11 + > drivers/clocksource/Makefile | 1 + > drivers/clocksource/timer-xilinx.c | 300 ++++++++++++++++++++++++++ > drivers/mfd/Makefile | 4 + > drivers/mfd/xilinx-timer.c | 147 +++++++++++++ I'm confused! > include/linux/mfd/xilinx-timer.h | 134 ++++++++++++ > 8 files changed, 598 insertions(+), 328 deletions(-) > delete mode 100644 arch/microblaze/kernel/timer.c > create mode 100644 drivers/clocksource/timer-xilinx.c > create mode 100644 drivers/mfd/xilinx-timer.c > create mode 100644 include/linux/mfd/xilinx-timer.h [...] > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2021 Sean Anderson <sean.anderson@seco.com> > + * > + * For Xilinx LogiCORE IP AXI Timer documentation, refer to DS764: > + * https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf > + */ > + > +#include <linux/clk.h> > +#include <linux/mfd/xilinx-timer.h> > +#include <linux/of.h> > +#include <asm/io.h> RED FLAG: You are not using the MFD API here. > +#define TCSR0 0x00 > +#define TLR0 0x04 > +#define TCR0 0x08 > +#define TCSR1 0x10 > +#define TLR1 0x14 > +#define TCR1 0x18 > + > +#define TCSR_MDT BIT(0) > +#define TCSR_UDT BIT(1) > +#define TCSR_GENT BIT(2) > +#define TCSR_CAPT BIT(3) > +#define TCSR_ARHT BIT(4) > +#define TCSR_LOAD BIT(5) > +#define TCSR_ENIT BIT(6) > +#define TCSR_ENT BIT(7) > +#define TCSR_TINT BIT(8) > +#define TCSR_PWMA BIT(9) > +#define TCSR_ENALL BIT(10) > +#define TCSR_CASC BIT(11) > + > +/* readl/writel wrappers to support BE systems */ > + > +static u32 xilinx_ioread32be(const void __iomem *addr) > +{ > + return ioread32be(addr); > +} > + > +static void xilinx_iowrite32be(u32 value, void __iomem *addr) > +{ > + iowrite32be(value, addr); > +} > + > +static u32 xilinx_ioread32(const void __iomem *addr) > +{ > + return ioread32(addr); > +} > + > +static void xilinx_iowrite32(u32 value, void __iomem *addr) > +{ > + iowrite32(value, addr); > +} Abstraction for the sake of abstraction, is not allowed. Just use the io*() calls directly in-place. > +int xilinx_timer_tlr_cycles(struct xilinx_timer_priv *priv, u32 *tlr, > + u32 tcsr, u64 cycles) > +{ > + if (cycles < 2 || cycles > priv->max + 2) > + return -ERANGE; > + > + if (tcsr & TCSR_UDT) > + *tlr = cycles - 2; > + else > + *tlr = priv->max - cycles + 2; > + > + return 0; > +} > + > +int xilinx_timer_tlr_period(struct xilinx_timer_priv *priv, u32 *tlr, > + u32 tcsr, unsigned int period) > +{ > + u64 cycles = DIV_ROUND_DOWN_ULL((u64)period * clk_get_rate(priv->clk), > + NSEC_PER_SEC); > + > + return xilinx_timer_tlr_cycles(priv, tlr, tcsr, cycles); > +} > + > +unsigned int xilinx_timer_get_period(struct xilinx_timer_priv *priv, > + u32 tlr, u32 tcsr) > +{ > + u64 cycles; > + > + if (tcsr & TCSR_UDT) > + cycles = tlr + 2; > + else > + cycles = priv->max - tlr + 2; > + > + return DIV_ROUND_UP_ULL(cycles * NSEC_PER_SEC, > + clk_get_rate(priv->clk)); > +} > + > +int xilinx_timer_common_init(struct device_node *np, > + struct xilinx_timer_priv *priv, > + u32 *one_timer) > +{ > + int ret; > + u32 tcsr0, width; > + > + > + priv->read = xilinx_ioread32; > + priv->write = xilinx_iowrite32; > + /* > + * If PWM mode is enabled, we should try not to disturb it. Use > + * CAPT since if PWM mode is enabled then MDT will be set as > + * well. > + * > + * First, clear CAPT and verify that it has been cleared > + */ > + tcsr0 = xilinx_timer_read(priv, TCSR0); > + xilinx_timer_write(priv, tcsr0 & ~(TCSR_CAPT & swab(TCSR_CAPT)), TCSR0); > + tcsr0 = xilinx_timer_read(priv, TCSR0); > + if (tcsr0 & (TCSR_CAPT | swab(TCSR_CAPT))) { > + pr_err("%pOF: cannot determine endianness\n", np); > + return -EOPNOTSUPP; > + } > + > + /* Then check to make sure our write sticks */ > + xilinx_timer_write(priv, tcsr0 | TCSR_CAPT, TCSR0); > + if (!(xilinx_timer_read(priv, TCSR0) & TCSR_CAPT)) { > + priv->read = xilinx_ioread32be; > + priv->write = xilinx_iowrite32be; > + } > + > + ret = of_property_read_u32(np, "xlnx,one-timer-only", one_timer); > + if (ret) { > + pr_err("%pOF: err %d: xlnx,one-timer-only\n", np, ret); > + return ret; > + } else if (*one_timer && *one_timer != 1) { > + pr_err("%pOF: xlnx,one-timer-only must be 0 or 1\n", np); > + return -EINVAL; > + } > + > + ret = of_property_read_u32(np, "xlnx,count-width", &width); > + if (ret == -EINVAL) { > + width = 32; > + } else if (ret) { > + pr_err("%pOF: err %d: xlnx,count-width\n", np, ret); > + return ret; > + } else if (width < 8 || width > 32) { > + pr_err("%pOF: invalid counter width\n", np); > + return -EINVAL; > + } > + priv->max = BIT_ULL(width) - 1; > + > + return 0; > +} This is *all* timer stuff. What is your rationale for dumping this into MFD? -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-06-01 8:47 UTC|newest] Thread overview: 78+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-28 21:45 [PATCH v4 1/3] dt-bindings: pwm: Add Xilinx AXI Timer Sean Anderson 2021-05-28 21:45 ` Sean Anderson 2021-05-28 21:45 ` [PATCH v4 2/3] clocksource: Rewrite Xilinx AXI timer driver Sean Anderson 2021-05-28 21:45 ` Sean Anderson 2021-06-01 8:47 ` Lee Jones [this message] 2021-06-01 8:47 ` Lee Jones 2021-06-01 14:24 ` Sean Anderson 2021-06-01 14:24 ` Sean Anderson 2021-05-28 21:45 ` [PATCH v4 3/3] pwm: Add support for Xilinx AXI Timer Sean Anderson 2021-05-28 21:45 ` Sean Anderson 2021-06-10 16:15 ` Sean Anderson 2021-06-10 16:15 ` Sean Anderson 2021-06-25 6:19 ` Uwe Kleine-König 2021-06-25 6:19 ` Uwe Kleine-König 2021-06-25 15:13 ` Sean Anderson 2021-06-25 15:13 ` Sean Anderson 2021-06-25 16:56 ` Uwe Kleine-König 2021-06-25 16:56 ` Uwe Kleine-König 2021-06-25 17:46 ` Sean Anderson 2021-06-25 17:46 ` Sean Anderson 2021-06-25 17:46 ` Sean Anderson 2021-06-25 17:46 ` Sean Anderson 2021-06-27 18:19 ` Uwe Kleine-König 2021-06-27 18:19 ` Uwe Kleine-König 2021-06-28 15:50 ` Sean Anderson 2021-06-28 15:50 ` Sean Anderson 2021-06-28 16:24 ` Uwe Kleine-König 2021-06-28 16:24 ` Uwe Kleine-König 2021-06-28 16:35 ` Sean Anderson 2021-06-28 16:35 ` Sean Anderson 2021-06-28 17:20 ` Uwe Kleine-König 2021-06-28 17:20 ` Uwe Kleine-König 2021-06-28 17:41 ` Sean Anderson 2021-06-28 17:41 ` Sean Anderson 2021-06-29 8:31 ` Uwe Kleine-König 2021-06-29 8:31 ` Uwe Kleine-König 2021-06-29 18:01 ` Sean Anderson 2021-06-29 18:01 ` Sean Anderson 2021-06-29 20:51 ` Uwe Kleine-König 2021-06-29 20:51 ` Uwe Kleine-König 2021-06-29 22:21 ` Sean Anderson 2021-06-29 22:21 ` Sean Anderson 2021-06-29 22:26 ` Sean Anderson 2021-06-29 22:26 ` Sean Anderson 2021-06-30 8:35 ` Uwe Kleine-König 2021-06-30 8:35 ` Uwe Kleine-König 2021-07-08 16:59 ` Sean Anderson 2021-07-08 16:59 ` Sean Anderson 2021-07-08 19:43 ` Uwe Kleine-König 2021-07-08 19:43 ` Uwe Kleine-König 2021-07-12 16:26 ` Sean Anderson 2021-07-12 16:26 ` Sean Anderson 2021-07-12 19:49 ` Uwe Kleine-König 2021-07-12 19:49 ` Uwe Kleine-König 2021-07-13 21:49 ` Sean Anderson 2021-07-13 21:49 ` Sean Anderson 2021-06-01 13:32 ` [PATCH v4 1/3] dt-bindings: pwm: Add " Rob Herring 2021-06-01 13:32 ` Rob Herring 2021-06-01 16:47 ` Sean Anderson 2021-06-01 16:47 ` Sean Anderson 2021-06-29 8:38 ` Uwe Kleine-König 2021-06-29 8:38 ` Uwe Kleine-König 2021-06-29 14:53 ` Sean Anderson 2021-06-29 14:53 ` Sean Anderson 2021-06-30 13:47 ` Michal Simek 2021-06-30 13:47 ` Michal Simek 2021-06-30 13:58 ` Michal Simek 2021-06-30 13:58 ` Michal Simek 2021-07-01 15:38 ` Sean Anderson 2021-07-01 15:38 ` Sean Anderson 2021-07-02 11:36 ` Michal Simek 2021-07-02 11:36 ` Michal Simek 2021-07-01 15:32 ` Sean Anderson 2021-07-01 15:32 ` Sean Anderson 2021-07-02 12:40 ` Michal Simek 2021-07-02 12:40 ` Michal Simek 2021-07-02 17:31 ` Sean Anderson 2021-07-02 17:31 ` Sean Anderson
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210601084734.GX543307@dell \ --to=lee.jones@linaro.org \ --cc=alvaro.gamez@hazent.com \ --cc=daniel.lezcano@linaro.org \ --cc=devicetree@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pwm@vger.kernel.org \ --cc=michal.simek@xilinx.com \ --cc=sean.anderson@seco.com \ --cc=tglx@linutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.