[PATCH v2 0/2] add Microchip PIT64B timer

[PATCH v2 0/2] add Microchip PIT64B timer

[Claudiu Beznea]

Hi,

This series adds driver for Microchip PIT64B timer.
Timer could be used in continuous or oneshot mode. It has 2x32 bit registers
to emulate a 64 bit timer. The timer's period could be configured via LSB_PR
and MSB_PR registers. The current timer's value could be checked via TLSB and
TMSB registers. When (TMSB << 32) | TLSB value reach the (MSB_PR << 32) | LSB_PR
interrupt is raised. If in contiuous mode the TLSB and TMSB resets and restart
counting.

This drivers uses PIT64B capabilities for clocksource and clockevent.
The first requested PIT64B timer is used for clockevent. The second one is used
for clocksource. Individual PIT64B hardware resources were used for clocksource
and clockevent to be able to support high resolution timers with this PIT64B
implementation.

Thank you,
Claudiu Beznea

Changes in v2:
- remove clock-frequency DT binding and hardcoded it in the driver
- initialize best_pres variable in mchp_pit64b_pres_prepare()
- remove MCHP_PIT64B_DEF_FREQ 
- get rid of patches 3-5 from v1 [1] since there is no entry in MAINTAINERS file
  for this entry. It was removed in commit
  44015a8181a5 ("MAINTAINERS: at91: remove the TC entry")

[1] https://lore.kernel.org/lkml/1552580772-8499-1-git-send-email-claudiu.beznea@microchip.com/

Claudiu Beznea (2):
  dt-bindings: arm: atmel: add bindings for PIT64B
  clocksource/drivers/timer-microchip-pit64b: add Microchip PIT64B
    support

 .../devicetree/bindings/arm/atmel-sysregs.txt      |   6 +
 drivers/clocksource/Kconfig                        |   6 +
 drivers/clocksource/Makefile                       |   1 +
 drivers/clocksource/timer-microchip-pit64b.c       | 462 +++++++++++++++++++++
 4 files changed, 475 insertions(+)
 create mode 100644 drivers/clocksource/timer-microchip-pit64b.c

-- 
2.7.4

[PATCH v2 1/2] dt-bindings: arm: atmel: add bindings for PIT64B

[Claudiu Beznea]

Add device tree bindings for PIT64B timer.

Cc: Rob Herring <robh@kernel.org>
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---

Hi Rob, Nicolas,

You Reviewed-by/Acked-by v1 [1] of this patch but I didn't collect it in this
version since I removed the clock-frequency binding in this version. 

Thank you!

Change in v2:
- removed clock-frequency binding

[1] https://lore.kernel.org/lkml/1552580772-8499-1-git-send-email-claudiu.beznea@microchip.com/

 Documentation/devicetree/bindings/arm/atmel-sysregs.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/atmel-sysregs.txt b/Documentation/devicetree/bindings/arm/atmel-sysregs.txt
index 9fbde401a090..e003a553b986 100644
--- a/Documentation/devicetree/bindings/arm/atmel-sysregs.txt
+++ b/Documentation/devicetree/bindings/arm/atmel-sysregs.txt
@@ -10,6 +10,12 @@ PIT Timer required properties:
 - interrupts: Should contain interrupt for the PIT which is the IRQ line
   shared across all System Controller members.
 
+PIT64B Timer required properties:
+- compatible: Should be "microchip,sam9x60-pit64b"
+- reg: Should contain registers location and length
+- interrupts: Should contain interrupt for PIT64B timer
+- clocks: Should contain the available clock sources for PIT64B timer.
+
 System Timer (ST) required properties:
 - compatible: Should be "atmel,at91rm9200-st", "syscon", "simple-mfd"
 - reg: Should contain registers location and length
-- 
2.7.4

[PATCH v2 2/2] clocksource/drivers/timer-microchip-pit64b: add Microchip PIT64B support

[Claudiu Beznea]

Add driver for Microchip PIT64B timer. Timer could be used in continuous
mode or oneshot mode. The hardware has 2x32 bit registers for period
emulating a 64 bit timer. The LSB_PR and MSB_PR registers are used to
set the period value (compare value). TLSB and TMSB keeps the current value
of the counter. After a compare the TLSB and TMSB register resets. Apart
from this the hardware has SMOD bit in mode register that allow to
reconfigure the timer without reset and start commands (start command
while timer is active is ignored).
The driver uses PIT64B timer as clocksource or clockevent. First requested
timer would be registered as clockevent, second one would be registered as
clocksource. Individual PIT64B hardware resources were used for clocksource
and clockevent to be able to support high resolution timers with this
hardware implementation.

Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---

Hi Nicolas,

You Acked-by on v1 [1] of this patch but I didn't collect it in this version
because I removed the the DT bindings for clock-frequency and intead hardcoded
it in the driver: 32KHz for clockevent, 5MHz for clocksource.

Thank you,
Claudiu Beznea

Changes in v2:
- do not select timer's frequency via DT binding but instead hardcoded it
- initialize best_pres variable in mchp_pit64b_pres_prepare()
- remove MCHP_PIT64B_DEF_FREQ 

[1] https://lore.kernel.org/lkml/1552580772-8499-1-git-send-email-claudiu.beznea@microchip.com/

 drivers/clocksource/Kconfig                  |   6 +
 drivers/clocksource/Makefile                 |   1 +
 drivers/clocksource/timer-microchip-pit64b.c | 462 +++++++++++++++++++++++++++
 3 files changed, 469 insertions(+)
 create mode 100644 drivers/clocksource/timer-microchip-pit64b.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index f35a53ce8988..e423610cd627 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -472,6 +472,12 @@ config OXNAS_RPS_TIMER
 config SYS_SUPPORTS_SH_CMT
         bool
 
+config MICROCHIP_PIT64B
+	bool "Microchip PIT64B support"
+	depends on OF || COMPILE_TEST
+	help
+	  This option enables Microchip PIT64B timer.
+
 config MTK_TIMER
 	bool "Mediatek timer driver" if COMPILE_TEST
 	depends on HAS_IOMEM
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 4dfe4225ece7..a41dcfffb87d 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_U300_TIMER)	+= timer-u300.o
 obj-$(CONFIG_SUN4I_TIMER)	+= timer-sun4i.o
 obj-$(CONFIG_SUN5I_HSTIMER)	+= timer-sun5i.o
 obj-$(CONFIG_MESON6_TIMER)	+= timer-meson6.o
+obj-$(CONFIG_MICROCHIP_PIT64B)	+= timer-microchip-pit64b.o
 obj-$(CONFIG_TEGRA_TIMER)	+= timer-tegra.o
 obj-$(CONFIG_VT8500_TIMER)	+= timer-vt8500.o
 obj-$(CONFIG_NSPIRE_TIMER)	+= timer-zevio.o
diff --git a/drivers/clocksource/timer-microchip-pit64b.c b/drivers/clocksource/timer-microchip-pit64b.c
new file mode 100644
index 000000000000..3fa65c7b4470
--- /dev/null
+++ b/drivers/clocksource/timer-microchip-pit64b.c
@@ -0,0 +1,462 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (C) 2019 Microchip Technology Inc.
+// Copyright (C) 2019 Claudiu Beznea (claudiu.beznea@microchip.com)
+
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/sched_clock.h>
+#include <linux/slab.h>
+
+#define MCHP_PIT64B_CR		0x00	/* Control Register */
+#define MCHP_PIT64B_CR_START	BIT(0)
+#define MCHP_PIT64B_CR_SWRST	BIT(8)
+
+#define MCHP_PIT64B_MR		0x04	/* Mode Register */
+#define MCHP_PIT64B_MR_CONT	BIT(0)
+#define MCHP_PIT64B_MR_SGCLK	BIT(3)
+#define MCHP_PIT64B_MR_SMOD	BIT(4)
+#define MCHP_PIT64B_MR_PRES	GENMASK(11, 8)
+
+#define MCHP_PIT64B_LSB_PR	0x08	/* LSB Period Register */
+
+#define MCHP_PIT64B_MSB_PR	0x0C	/* MSB Period Register */
+
+#define MCHP_PIT64B_IER		0x10	/* Interrupt Enable Register */
+#define MCHP_PIT64B_IER_PERIOD	BIT(0)
+
+#define MCHP_PIT64B_ISR		0x1C	/* Interrupt Status Register */
+#define MCHP_PIT64B_ISR_PERIOD	BIT(0)
+
+#define MCHP_PIT64B_TLSBR	0x20	/* Timer LSB Register */
+
+#define MCHP_PIT64B_TMSBR	0x24	/* Timer MSB Register */
+
+#define MCHP_PIT64B_PRES_MAX	0x10
+#define MCHP_PIT64B_LSBMASK	GENMASK_ULL(31, 0)
+#define MCHP_PIT64B_PRESCALER(p)	(MCHP_PIT64B_MR_PRES & ((p) << 8))
+#define MCHP_PIT64B_DEF_CS_FREQ		5000000UL	/* 5 MHz */
+#define MCHP_PIT64B_DEF_CE_FREQ		32768		/* 32 KHz */
+
+#define MCHP_PIT64B_NAME	"pit64b"
+
+struct mchp_pit64b_common_data {
+	void __iomem *base;
+	struct clk *pclk;
+	struct clk *gclk;
+	u64 cycles;
+	u8 pres;
+};
+
+struct mchp_pit64b_clksrc_data {
+	struct clocksource *clksrc;
+	struct mchp_pit64b_common_data *cd;
+};
+
+struct mchp_pit64b_clkevt_data {
+	struct clock_event_device *clkevt;
+	struct mchp_pit64b_common_data *cd;
+};
+
+static struct mchp_pit64b_data {
+	struct mchp_pit64b_clksrc_data *csd;
+	struct mchp_pit64b_clkevt_data *ced;
+} data;
+
+static inline u32 mchp_pit64b_read(void __iomem *base, u32 offset)
+{
+	return readl_relaxed(base + offset);
+}
+
+static inline void mchp_pit64b_write(void __iomem *base, u32 offset, u32 val)
+{
+	writel_relaxed(val, base + offset);
+}
+
+static inline u64 mchp_pit64b_get_period(void __iomem *base)
+{
+	u32 lsb, msb;
+
+	/* LSB must be read first to guarantee an atomic read of the 64 bit
+	 * timer.
+	 */
+	lsb = mchp_pit64b_read(base, MCHP_PIT64B_TLSBR);
+	msb = mchp_pit64b_read(base, MCHP_PIT64B_TMSBR);
+
+	return (((u64)msb << 32) | lsb);
+}
+
+static inline void mchp_pit64b_set_period(void __iomem *base, u64 cycles)
+{
+	u32 lsb, msb;
+
+	lsb = cycles & MCHP_PIT64B_LSBMASK;
+	msb = cycles >> 32;
+
+	/* LSB must be write last to guarantee an atomic update of the timer
+	 * even when SMOD=1.
+	 */
+	mchp_pit64b_write(base, MCHP_PIT64B_MSB_PR, msb);
+	mchp_pit64b_write(base, MCHP_PIT64B_LSB_PR, lsb);
+}
+
+static inline void mchp_pit64b_reset(struct mchp_pit64b_common_data *data,
+				     u32 mode, bool irq_ena)
+{
+	mode |= MCHP_PIT64B_PRESCALER(data->pres);
+	if (data->gclk)
+		mode |= MCHP_PIT64B_MR_SGCLK;
+
+	mchp_pit64b_write(data->base, MCHP_PIT64B_CR, MCHP_PIT64B_CR_SWRST);
+	mchp_pit64b_write(data->base, MCHP_PIT64B_MR, mode);
+	mchp_pit64b_set_period(data->base, data->cycles);
+	if (irq_ena)
+		mchp_pit64b_write(data->base, MCHP_PIT64B_IER,
+				  MCHP_PIT64B_IER_PERIOD);
+	mchp_pit64b_write(data->base, MCHP_PIT64B_CR, MCHP_PIT64B_CR_START);
+}
+
+static u64 mchp_pit64b_read_clk(struct clocksource *cs)
+{
+	return mchp_pit64b_get_period(data.csd->cd->base);
+}
+
+static u64 mchp_sched_read_clk(void)
+{
+	return mchp_pit64b_get_period(data.csd->cd->base);
+}
+
+static struct clocksource mchp_pit64b_clksrc = {
+	.name = MCHP_PIT64B_NAME,
+	.mask = CLOCKSOURCE_MASK(64),
+	.flags = CLOCK_SOURCE_IS_CONTINUOUS,
+	.rating = 210,
+	.read = mchp_pit64b_read_clk,
+};
+
+static int mchp_pit64b_clkevt_shutdown(struct clock_event_device *cedev)
+{
+	mchp_pit64b_write(data.ced->cd->base, MCHP_PIT64B_CR,
+			  MCHP_PIT64B_CR_SWRST);
+
+	return 0;
+}
+
+static int mchp_pit64b_clkevt_set_periodic(struct clock_event_device *cedev)
+{
+	mchp_pit64b_reset(data.ced->cd, MCHP_PIT64B_MR_CONT, true);
+
+	return 0;
+}
+
+static int mchp_pit64b_clkevt_set_oneshot(struct clock_event_device *cedev)
+{
+	mchp_pit64b_reset(data.ced->cd, MCHP_PIT64B_MR_SMOD, true);
+
+	return 0;
+}
+
+static int mchp_pit64b_clkevt_set_next_event(unsigned long evt,
+					     struct clock_event_device *cedev)
+{
+	mchp_pit64b_set_period(data.ced->cd->base, evt);
+	mchp_pit64b_write(data.ced->cd->base, MCHP_PIT64B_CR,
+			  MCHP_PIT64B_CR_START);
+
+	return 0;
+}
+
+static void mchp_pit64b_clkevt_suspend(struct clock_event_device *cedev)
+{
+	mchp_pit64b_write(data.ced->cd->base, MCHP_PIT64B_CR,
+			  MCHP_PIT64B_CR_SWRST);
+	if (data.ced->cd->gclk)
+		clk_disable_unprepare(data.ced->cd->gclk);
+	clk_disable_unprepare(data.ced->cd->pclk);
+}
+
+static void mchp_pit64b_clkevt_resume(struct clock_event_device *cedev)
+{
+	u32 mode = MCHP_PIT64B_MR_SMOD;
+
+	clk_prepare_enable(data.ced->cd->pclk);
+	if (data.ced->cd->gclk)
+		clk_prepare_enable(data.ced->cd->gclk);
+
+	if (clockevent_state_periodic(data.ced->clkevt))
+		mode = MCHP_PIT64B_MR_CONT;
+
+	mchp_pit64b_reset(data.ced->cd, mode, true);
+}
+
+static struct clock_event_device mchp_pit64b_clkevt = {
+	.name = MCHP_PIT64B_NAME,
+	.features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC,
+	.rating = 150,
+	.set_state_shutdown = mchp_pit64b_clkevt_shutdown,
+	.set_state_periodic = mchp_pit64b_clkevt_set_periodic,
+	.set_state_oneshot = mchp_pit64b_clkevt_set_oneshot,
+	.set_next_event = mchp_pit64b_clkevt_set_next_event,
+	.suspend = mchp_pit64b_clkevt_suspend,
+	.resume = mchp_pit64b_clkevt_resume,
+};
+
+static irqreturn_t mchp_pit64b_interrupt(int irq, void *dev_id)
+{
+	struct mchp_pit64b_clkevt_data *irq_data = dev_id;
+
+	if (data.ced != irq_data)
+		return IRQ_NONE;
+
+	if (mchp_pit64b_read(irq_data->cd->base, MCHP_PIT64B_ISR) &
+	    MCHP_PIT64B_ISR_PERIOD) {
+		irq_data->clkevt->event_handler(irq_data->clkevt);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static int __init mchp_pit64b_pres_compute(u32 *pres, u32 clk_rate,
+					   u32 max_rate)
+{
+	u32 tmp;
+
+	for (*pres = 0; *pres < MCHP_PIT64B_PRES_MAX; (*pres)++) {
+		tmp = clk_rate / (*pres + 1);
+		if (tmp <= max_rate)
+			break;
+	}
+
+	if (*pres == MCHP_PIT64B_PRES_MAX)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int __init mchp_pit64b_pres_prepare(struct mchp_pit64b_common_data *cd,
+					   unsigned long max_rate)
+{
+	unsigned long pclk_rate, diff = 0, best_diff = ULONG_MAX;
+	long gclk_round = 0;
+	u32 pres, best_pres = 0;
+	int ret = 0;
+
+	pclk_rate = clk_get_rate(cd->pclk);
+	if (!pclk_rate)
+		return -EINVAL;
+
+	if (cd->gclk) {
+		gclk_round = clk_round_rate(cd->gclk, max_rate);
+		if (gclk_round < 0)
+			goto pclk;
+
+		if (pclk_rate / gclk_round < 3)
+			goto pclk;
+
+		ret = mchp_pit64b_pres_compute(&pres, gclk_round, max_rate);
+		if (ret)
+			best_diff = abs(gclk_round - max_rate);
+		else
+			best_diff = abs(gclk_round / (pres + 1) - max_rate);
+		best_pres = pres;
+	}
+
+pclk:
+	/* Check if requested rate could be obtained using PCLK. */
+	ret = mchp_pit64b_pres_compute(&pres, pclk_rate, max_rate);
+	if (ret)
+		diff = abs(pclk_rate - max_rate);
+	else
+		diff = abs(pclk_rate / (pres + 1) - max_rate);
+
+	if (best_diff > diff) {
+		/* Use PCLK. */
+		cd->gclk = NULL;
+		best_pres = pres;
+	} else {
+		clk_set_rate(cd->gclk, gclk_round);
+	}
+
+	cd->pres = best_pres;
+
+	pr_info("PIT64B: using clk=%s with prescaler %u, freq=%lu [Hz]\n",
+		cd->gclk ? "gclk" : "pclk", cd->pres,
+		cd->gclk ? gclk_round / (cd->pres + 1)
+			 : pclk_rate / (cd->pres + 1));
+
+	return 0;
+}
+
+static int __init mchp_pit64b_dt_init_clksrc(struct mchp_pit64b_common_data *cd)
+{
+	struct mchp_pit64b_clksrc_data *csd;
+	unsigned long clk_rate;
+	int ret;
+
+	csd = kzalloc(sizeof(*csd), GFP_KERNEL);
+	if (!csd)
+		return -ENOMEM;
+
+	csd->cd = cd;
+
+	if (csd->cd->gclk)
+		clk_rate = clk_get_rate(csd->cd->gclk);
+	else
+		clk_rate = clk_get_rate(csd->cd->pclk);
+
+	clk_rate = clk_rate / (cd->pres + 1);
+	csd->cd->cycles = ULLONG_MAX;
+	mchp_pit64b_reset(csd->cd, MCHP_PIT64B_MR_CONT, false);
+
+	data.csd = csd;
+
+	csd->clksrc = &mchp_pit64b_clksrc;
+
+	ret = clocksource_register_hz(csd->clksrc, clk_rate);
+	if (ret) {
+		pr_debug("clksrc: Failed to register PIT64B clocksource!\n");
+		goto free;
+	}
+
+	sched_clock_register(mchp_sched_read_clk, 64, clk_rate);
+
+	return 0;
+
+free:
+	kfree(csd);
+	data.csd = NULL;
+
+	return ret;
+}
+
+static int __init mchp_pit64b_dt_init_clkevt(struct mchp_pit64b_common_data *cd,
+					     u32 irq)
+{
+	struct mchp_pit64b_clkevt_data *ced;
+	unsigned long clk_rate;
+	int ret;
+
+	ced = kzalloc(sizeof(*ced), GFP_KERNEL);
+	if (!ced)
+		return -ENOMEM;
+
+	ced->cd = cd;
+
+	if (ced->cd->gclk)
+		clk_rate = clk_get_rate(ced->cd->gclk);
+	else
+		clk_rate = clk_get_rate(ced->cd->pclk);
+
+	clk_rate = clk_rate / (ced->cd->pres + 1);
+	ced->cd->cycles = DIV_ROUND_CLOSEST(clk_rate, HZ);
+
+	ret = request_irq(irq, mchp_pit64b_interrupt, IRQF_TIMER, "pit64b_tick",
+			  ced);
+	if (ret) {
+		pr_debug("clkevt: Failed to setup PIT64B IRQ\n");
+		goto free;
+	}
+
+	data.ced = ced;
+
+	/* Set up and register clockevents. */
+	ced->clkevt = &mchp_pit64b_clkevt;
+	ced->clkevt->cpumask = cpumask_of(0);
+	ced->clkevt->irq = irq;
+	clockevents_config_and_register(ced->clkevt, clk_rate, 1, ULONG_MAX);
+
+	return 0;
+
+free:
+	kfree(ced);
+	data.ced = NULL;
+
+	return ret;
+}
+
+static int __init mchp_pit64b_dt_init(struct device_node *node)
+{
+	struct mchp_pit64b_common_data *cd;
+	u32 irq;
+	int ret;
+
+	if (data.csd && data.ced)
+		return -EBUSY;
+
+	cd = kzalloc(sizeof(*cd), GFP_KERNEL);
+	if (!cd)
+		return -ENOMEM;
+
+	cd->pclk = of_clk_get_by_name(node, "pclk");
+	if (IS_ERR(cd->pclk)) {
+		ret = PTR_ERR(cd->pclk);
+		goto free;
+	}
+
+	cd->gclk = of_clk_get_by_name(node, "gclk");
+	if (IS_ERR(cd->gclk))
+		cd->gclk = NULL;
+
+	ret = mchp_pit64b_pres_prepare(cd, data.ced ? MCHP_PIT64B_DEF_CS_FREQ :
+						      MCHP_PIT64B_DEF_CE_FREQ);
+	if (ret)
+		goto free;
+
+	cd->base = of_iomap(node, 0);
+	if (!cd->base) {
+		pr_debug("%s: Could not map PIT64B address!\n",
+			 MCHP_PIT64B_NAME);
+		ret = -ENXIO;
+		goto free;
+	}
+
+	ret = clk_prepare_enable(cd->pclk);
+	if (ret)
+		goto unmap;
+
+	if (cd->gclk) {
+		ret = clk_prepare_enable(cd->gclk);
+		if (ret)
+			goto pclk_unprepare;
+	}
+
+	if (!data.ced) {
+		irq = irq_of_parse_and_map(node, 0);
+		if (!irq) {
+			pr_debug("%s: Failed to get PIT64B clockevent IRQ!\n",
+				 MCHP_PIT64B_NAME);
+			ret = -ENODEV;
+			goto gclk_unprepare;
+		}
+		ret = mchp_pit64b_dt_init_clkevt(cd, irq);
+		if (ret)
+			goto irq_unmap;
+	} else {
+		ret = mchp_pit64b_dt_init_clksrc(cd);
+		if (ret)
+			goto gclk_unprepare;
+	}
+
+	return 0;
+
+irq_unmap:
+	irq_dispose_mapping(irq);
+gclk_unprepare:
+	if (cd->gclk)
+		clk_disable_unprepare(cd->gclk);
+pclk_unprepare:
+	clk_disable_unprepare(cd->pclk);
+unmap:
+	iounmap(cd->base);
+free:
+	kfree(cd);
+
+	return ret;
+}
+
+TIMER_OF_DECLARE(mchp_pit64b_clksrc, "microchip,sam9x60-pit64b",
+		 mchp_pit64b_dt_init);
-- 
2.7.4

Re: [PATCH v2 2/2] clocksource/drivers/timer-microchip-pit64b: add Microchip PIT64B support

[Thomas Gleixner]

On Mon, 4 Nov 2019, Claudiu Beznea wrote:
> +struct mchp_pit64b_common_data {
> +	void __iomem *base;
> +	struct clk *pclk;
> +	struct clk *gclk;
> +	u64 cycles;
> +	u8 pres;

Can you please make the members tabular for readability sake in all the
structs?

struct mchp_pit64b_common_data {
	void __iomem	*base;
	struct clk	*pclk;
	struct clk	*gclk;
	u64		cycles;
	u8		pres;
};


> +static struct mchp_pit64b_data {
> +	struct mchp_pit64b_clksrc_data *csd;
> +	struct mchp_pit64b_clkevt_data *ced;
> +} data;

This is suboptimal style for two reasons:

     1) Having a seperate struct and instance declaration is way simpler to
     	parse.

     2) Naming a global variable with a generic name is unintuitive and is
     	too easily confused with local variable names. See below.

> +static inline u64 mchp_pit64b_get_period(void __iomem *base)
> +{
> +	u32 lsb, msb;

lsb and msb are not really correct here. They stand for Least/Most
Significant Bit (Byte).

lsw/msw would be more correct, but 'high/low' would be sufficiently self
explaining as well.

      /*
       * Please use proper multi-line comments and not the network style.
       * below. Can you spot the difference?
       */

> +	/* LSB must be read first to guarantee an atomic read of the 64 bit
> +	 * timer.
> +	 */

Does that mean that the hardware latches the upper 32bit when the lower
32bit are read? If so, please write it out.

But aside of that this is fundamentally broken not only on SMP, but also on
UP because the clocksource read function can be called in preemptible
and/or interruptible context.

   thread()
     ktime_get))
       t = clocksource->read()
          low = read(LSW); <- Latches MSW

---> interrupt or preemption

       ktime_get))
         t = clocksource->read()
            low = read(LSW);    <- Latches MSW
	    high = read(MSW);   <- Reads correct MSW

<--- interrupt or preemption ends

          high = read(MSW);     <- Read incorrect MSW

On SMP the same issue exists between two CPUs....

> +	lsb = mchp_pit64b_read(base, MCHP_PIT64B_TLSBR);
> +	msb = mchp_pit64b_read(base, MCHP_PIT64B_TMSBR);

> +static inline void mchp_pit64b_set_period(void __iomem *base, u64 cycles)
> +{
> +	u32 lsb, msb;
> +
> +	lsb = cycles & MCHP_PIT64B_LSBMASK;
> +	msb = cycles >> 32;
> +
> +	/* LSB must be write last to guarantee an atomic update of the timer

s/write/written/

> +	 * even when SMOD=1.
> +	 */
> +	mchp_pit64b_write(base, MCHP_PIT64B_MSB_PR, msb);
> +	mchp_pit64b_write(base, MCHP_PIT64B_LSB_PR, lsb);
> +}
> +
> +static inline void mchp_pit64b_reset(struct mchp_pit64b_common_data *data,

And this is exactly the issue I mentioned above. You have a local argument
name which shadows a global variable name. Bah.

> +				     u32 mode, bool irq_ena)
> +{
> +	mode |= MCHP_PIT64B_PRESCALER(data->pres);
> +	if (data->gclk)
> +		mode |= MCHP_PIT64B_MR_SGCLK;
> +
> +	mchp_pit64b_write(data->base, MCHP_PIT64B_CR, MCHP_PIT64B_CR_SWRST);
> +	mchp_pit64b_write(data->base, MCHP_PIT64B_MR, mode);
> +	mchp_pit64b_set_period(data->base, data->cycles);
> +	if (irq_ena)
> +		mchp_pit64b_write(data->base, MCHP_PIT64B_IER,
> +				  MCHP_PIT64B_IER_PERIOD);

This lacks brackets as after the condition follows a multi-line statement.
It's techincally a single line, but visually a multi-line statement due to
the line break.

> +	mchp_pit64b_write(data->base, MCHP_PIT64B_CR, MCHP_PIT64B_CR_START);
> +}
> +
> +static u64 mchp_pit64b_read_clk(struct clocksource *cs)
> +{
> +	return mchp_pit64b_get_period(data.csd->cd->base);

Lot of indirection here in the hotpath. You surely could avoid touching
multiple cache-lines here by restructuring your data layout so that you
have the only interesting element of 'common data', i.e. base, in the
structure which encapsulates the 'clocksource'.

struct mchp_cs {
	void __iomem		*base;
	struct clocksource 	cs;
};

And then your read function becomes either:
{
    struct mchp_cs *mcs = container_of(cs, struct mchp_cs, cs);

    return read_cs(mcs->base);
}

or if you have he clocksource statically allocated, i.e.:

struct mchp_cs mchp_clksource = { /* init here */ };

{
	return read_cs(mchp_clksource.base);
}
	
> +static u64 mchp_sched_read_clk(void)
> +{
> +	return mchp_pit64b_get_period(data.csd->cd->base);

Ditto

> +
> +static int mchp_pit64b_clkevt_set_next_event(unsigned long evt,
> +					     struct clock_event_device *cedev)
> +{
> +	mchp_pit64b_set_period(data.ced->cd->base, evt);
> +	mchp_pit64b_write(data.ced->cd->base, MCHP_PIT64B_CR,
> +			  MCHP_PIT64B_CR_START);

Same issue here.

> +static irqreturn_t mchp_pit64b_interrupt(int irq, void *dev_id)
> +{
> +	struct mchp_pit64b_clkevt_data *irq_data = dev_id;
> +
> +	if (data.ced != irq_data)
> +		return IRQ_NONE;

How is this supposed to happen?

> +
> +	if (mchp_pit64b_read(irq_data->cd->base, MCHP_PIT64B_ISR) &
> +	    MCHP_PIT64B_ISR_PERIOD) {

Why are you reading this from the device and not from the mode information
of the clockevent which would be faster obviously?

> +static int __init mchp_pit64b_pres_compute(u32 *pres, u32 clk_rate,
> +					   u32 max_rate)
> +{
> +	u32 tmp;
> +
> +	for (*pres = 0; *pres < MCHP_PIT64B_PRES_MAX; (*pres)++) {
> +		tmp = clk_rate / (*pres + 1);
> +		if (tmp <= max_rate)
> +			break;
> +	}
> +
> +	if (*pres == MCHP_PIT64B_PRES_MAX)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int __init mchp_pit64b_pres_prepare(struct mchp_pit64b_common_data *cd,
> +					   unsigned long max_rate)
> +{
> +	unsigned long pclk_rate, diff = 0, best_diff = ULONG_MAX;
> +	long gclk_round = 0;
> +	u32 pres, best_pres = 0;
> +	int ret = 0;
> +
> +	pclk_rate = clk_get_rate(cd->pclk);
> +	if (!pclk_rate)
> +		return -EINVAL;
> +
> +	if (cd->gclk) {
> +		gclk_round = clk_round_rate(cd->gclk, max_rate);
> +		if (gclk_round < 0)
> +			goto pclk;
> +
> +		if (pclk_rate / gclk_round < 3)
> +			goto pclk;
> +
> +		ret = mchp_pit64b_pres_compute(&pres, gclk_round, max_rate);
> +		if (ret)
> +			best_diff = abs(gclk_round - max_rate);
> +		else
> +			best_diff = abs(gclk_round / (pres + 1) - max_rate);
> +		best_pres = pres;
> +	}
> +
> +pclk:
> +	/* Check if requested rate could be obtained using PCLK. */
> +	ret = mchp_pit64b_pres_compute(&pres, pclk_rate, max_rate);
> +	if (ret)
> +		diff = abs(pclk_rate - max_rate);
> +	else
> +		diff = abs(pclk_rate / (pres + 1) - max_rate);
> +
> +	if (best_diff > diff) {
> +		/* Use PCLK. */
> +		cd->gclk = NULL;
> +		best_pres = pres;
> +	} else {
> +		clk_set_rate(cd->gclk, gclk_round);
> +	}
> +
> +	cd->pres = best_pres;
> +
> +	pr_info("PIT64B: using clk=%s with prescaler %u, freq=%lu [Hz]\n",
> +		cd->gclk ? "gclk" : "pclk", cd->pres,
> +		cd->gclk ? gclk_round / (cd->pres + 1)
> +			 : pclk_rate / (cd->pres + 1));
> +
> +	return 0;

Lots of undocumented functionality which open codes stuff which exists
already in the clk framework AFAICT.

Why are you not simply implementing this as clk framework components?


            |-----|
  gclk ---->|     |    |---------|
            | MUX |--->| Divider |->
  pclk ---->|     |    |---------|
            |-----|

which is exaxtly how your hardware looks like. The clk framework has all
the selection mechanisms in place and all this conditional clock stuff can
be removed all over the place simply because you just ask for the desired
frequency on init. Also suspend/resume and all the other stuff just works
without all the mess involved.

> +free:
> +	kfree(csd);
> +	data.csd = NULL;

It does not matter here, but for correctness sake this is the wrong
order and triggers my built-in UAF-race detector.

You need to NULL the pointer _before_ freeing the underlying memory.

> +static int __init mchp_pit64b_dt_init(struct device_node *node)
> +{
> +	struct mchp_pit64b_common_data *cd;
> +	u32 irq;
> +	int ret;
> +
> +	if (data.csd && data.ced)
> +		return -EBUSY;

Huch?

> +	cd = kzalloc(sizeof(*cd), GFP_KERNEL);
> +	if (!cd)
> +		return -ENOMEM;

If either data.csd or data.ced exists then the common data exists as
well. Why would you allocate another instance?

> +
> +	cd->pclk = of_clk_get_by_name(node, "pclk");
> +	if (IS_ERR(cd->pclk)) {
> +		ret = PTR_ERR(cd->pclk);
> +		goto free;
> +	}

....

> +	if (!data.ced) {

And here you actually have a conditional which is confusing at best.

> +		irq = irq_of_parse_and_map(node, 0);
> +		if (!irq) {
> +			pr_debug("%s: Failed to get PIT64B clockevent IRQ!\n",
> +				 MCHP_PIT64B_NAME);
> +			ret = -ENODEV;
> +			goto gclk_unprepare;
> +		}
> +		ret = mchp_pit64b_dt_init_clkevt(cd, irq);
> +		if (ret)
> +			goto irq_unmap;
> +	} else {
> +		ret = mchp_pit64b_dt_init_clksrc(cd);
> +		if (ret)
> +			goto gclk_unprepare;
> +	}

So the first invocation of this init function is supposed to init the clock
event device and the second one inits the clock source. And both allocate
common data. How is that common?

Thanks,

	tglx

Re: [PATCH v2 1/2] dt-bindings: arm: atmel: add bindings for PIT64B

[Rob Herring]

On Mon, Nov 04, 2019 at 05:10:03PM +0200, Claudiu Beznea wrote:
> Add device tree bindings for PIT64B timer.
> 
> Cc: Rob Herring <robh@kernel.org>
> Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
> 
> Hi Rob, Nicolas,
> 
> You Reviewed-by/Acked-by v1 [1] of this patch but I didn't collect it in this
> version since I removed the clock-frequency binding in this version. 
> 
> Thank you!
> 
> Change in v2:
> - removed clock-frequency binding
> 
> [1] https://lore.kernel.org/lkml/1552580772-8499-1-git-send-email-claudiu.beznea@microchip.com/
> 
>  Documentation/devicetree/bindings/arm/atmel-sysregs.txt | 6 ++++++
>  1 file changed, 6 insertions(+)

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 2/2] clocksource/drivers/timer-microchip-pit64b: add Microchip PIT64B support

[Claudiu.Beznea]

On 04.11.2019 21:05, Thomas Gleixner wrote:
> On Mon, 4 Nov 2019, Claudiu Beznea wrote:
>> +struct mchp_pit64b_common_data {
>> +	void __iomem *base;
>> +	struct clk *pclk;
>> +	struct clk *gclk;
>> +	u64 cycles;
>> +	u8 pres;
> 
> Can you please make the members tabular for readability sake in all the
> structs?

OK!

> 
> struct mchp_pit64b_common_data {
> 	void __iomem	*base;
> 	struct clk	*pclk;
> 	struct clk	*gclk;
> 	u64		cycles;
> 	u8		pres;
> };
> 
> 
>> +static struct mchp_pit64b_data {
>> +	struct mchp_pit64b_clksrc_data *csd;
>> +	struct mchp_pit64b_clkevt_data *ced;
>> +} data;
> 
> This is suboptimal style for two reasons:
> 
>      1) Having a seperate struct and instance declaration is way simpler to
>      	parse.
> 
>      2) Naming a global variable with a generic name is unintuitive and is
>      	too easily confused with local variable names. See below.

OK, I will get rid of this.

> 
>> +static inline u64 mchp_pit64b_get_period(void __iomem *base)
>> +{
>> +	u32 lsb, msb;
> 
> lsb and msb are not really correct here. They stand for Least/Most
> Significant Bit (Byte).
> 
> lsw/msw would be more correct, but 'high/low' would be sufficiently self
> explaining as well.

I used lsb and msb to be in sync with the datasheet.

> 
>       /*
>        * Please use proper multi-line comments and not the network style.
>        * below. Can you spot the difference?
>        */

OK!

> 
>> +	/* LSB must be read first to guarantee an atomic read of the 64 bit
>> +	 * timer.
>> +	 */
> 
> Does that mean that the hardware latches the upper 32bit when the lower
> 32bit are read? If so, please write it out.

OK, I'll document it better. I though that the comment above was enough.
The datasheet is also open for the product that is using this block.

> 
> But aside of that this is fundamentally broken not only on SMP, but also on
> UP because the clocksource read function can be called in preemptible
> and/or interruptible context.
> 
>    thread()
>      ktime_get))
>        t = clocksource->read()
>           low = read(LSW); <- Latches MSW
> 
> ---> interrupt or preemption
> 
>        ktime_get))
>          t = clocksource->read()
>             low = read(LSW);    <- Latches MSW
> 	    high = read(MSW);   <- Reads correct MSW
> 
> <--- interrupt or preemption ends
> 
>           high = read(MSW);     <- Read incorrect MSW
> 
> On SMP the same issue exists between two CPUs....

Yes, I agree on this. My mistake. Thank you for pointing this out.

> 
>> +	lsb = mchp_pit64b_read(base, MCHP_PIT64B_TLSBR);
>> +	msb = mchp_pit64b_read(base, MCHP_PIT64B_TMSBR);
> 
>> +static inline void mchp_pit64b_set_period(void __iomem *base, u64 cycles)
>> +{
>> +	u32 lsb, msb;
>> +
>> +	lsb = cycles & MCHP_PIT64B_LSBMASK;
>> +	msb = cycles >> 32;
>> +
>> +	/* LSB must be write last to guarantee an atomic update of the timer
> 
> s/write/written/

OK!

> 
>> +	 * even when SMOD=1.
>> +	 */
>> +	mchp_pit64b_write(base, MCHP_PIT64B_MSB_PR, msb);
>> +	mchp_pit64b_write(base, MCHP_PIT64B_LSB_PR, lsb);
>> +}
>> +
>> +static inline void mchp_pit64b_reset(struct mchp_pit64b_common_data *data,
> 
> And this is exactly the issue I mentioned above. You have a local argument
> name which shadows a global variable name. Bah.

I'll get rid of it.

> 
>> +				     u32 mode, bool irq_ena)
>> +{
>> +	mode |= MCHP_PIT64B_PRESCALER(data->pres);
>> +	if (data->gclk)
>> +		mode |= MCHP_PIT64B_MR_SGCLK;
>> +
>> +	mchp_pit64b_write(data->base, MCHP_PIT64B_CR, MCHP_PIT64B_CR_SWRST);
>> +	mchp_pit64b_write(data->base, MCHP_PIT64B_MR, mode);
>> +	mchp_pit64b_set_period(data->base, data->cycles);
>> +	if (irq_ena)
>> +		mchp_pit64b_write(data->base, MCHP_PIT64B_IER,
>> +				  MCHP_PIT64B_IER_PERIOD);
> 
> This lacks brackets as after the condition follows a multi-line statement.
> It's techincally a single line, but visually a multi-line statement due to
> the line break.

OK, I'll use brackets on these scenarios.

> 
>> +	mchp_pit64b_write(data->base, MCHP_PIT64B_CR, MCHP_PIT64B_CR_START);
>> +}
>> +
>> +static u64 mchp_pit64b_read_clk(struct clocksource *cs)
>> +{
>> +	return mchp_pit64b_get_period(data.csd->cd->base);
> 
> Lot of indirection here in the hotpath. You surely could avoid touching
> multiple cache-lines here by restructuring your data layout so that you
> have the only interesting element of 'common data', i.e. base, in the
> structure which encapsulates the 'clocksource'.>
> struct mchp_cs {
> 	void __iomem		*base;
> 	struct clocksource 	cs;
> };
> 
> And then your read function becomes either:
> {
>     struct mchp_cs *mcs = container_of(cs, struct mchp_cs, cs);
> 
>     return read_cs(mcs->base);
> }
> 
> or if you have he clocksource statically allocated, i.e.:
> 
> struct mchp_cs mchp_clksource = { /* init here */ };
> 
> {
> 	return read_cs(mchp_clksource.base);
> }
> 	

Agree! I'll switch to your proposed approach.

>> +static u64 mchp_sched_read_clk(void)
>> +{
>> +	return mchp_pit64b_get_period(data.csd->cd->base);
> 
> Ditto
> 
>> +
>> +static int mchp_pit64b_clkevt_set_next_event(unsigned long evt,
>> +					     struct clock_event_device *cedev)
>> +{
>> +	mchp_pit64b_set_period(data.ced->cd->base, evt);
>> +	mchp_pit64b_write(data.ced->cd->base, MCHP_PIT64B_CR,
>> +			  MCHP_PIT64B_CR_START);
> 
> Same issue here.
> 
>> +static irqreturn_t mchp_pit64b_interrupt(int irq, void *dev_id)
>> +{
>> +	struct mchp_pit64b_clkevt_data *irq_data = dev_id;
>> +
>> +	if (data.ced != irq_data)
>> +		return IRQ_NONE;
> 
> How is this supposed to happen?

It should not. I'll remove it.

> 
>> +
>> +	if (mchp_pit64b_read(irq_data->cd->base, MCHP_PIT64B_ISR) &
>> +	    MCHP_PIT64B_ISR_PERIOD) {
> 
> Why are you reading this from the device and not from the mode information
> of the clockevent which would be faster obviously?

The IP can generate multiple interrupts: period interrupt is generated when
the configured periods end. The period interrupt is generated in both
periodic and one shot mode after the current programmed period has been
expired. I used the read from hardware to be sure the expected interrupt
arrived.

> 
>> +static int __init mchp_pit64b_pres_compute(u32 *pres, u32 clk_rate,
>> +					   u32 max_rate)
>> +{
>> +	u32 tmp;
>> +
>> +	for (*pres = 0; *pres < MCHP_PIT64B_PRES_MAX; (*pres)++) {
>> +		tmp = clk_rate / (*pres + 1);
>> +		if (tmp <= max_rate)
>> +			break;
>> +	}
>> +
>> +	if (*pres == MCHP_PIT64B_PRES_MAX)
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +static int __init mchp_pit64b_pres_prepare(struct mchp_pit64b_common_data *cd,
>> +					   unsigned long max_rate)
>> +{
>> +	unsigned long pclk_rate, diff = 0, best_diff = ULONG_MAX;
>> +	long gclk_round = 0;
>> +	u32 pres, best_pres = 0;
>> +	int ret = 0;
>> +
>> +	pclk_rate = clk_get_rate(cd->pclk);
>> +	if (!pclk_rate)
>> +		return -EINVAL;
>> +
>> +	if (cd->gclk) {
>> +		gclk_round = clk_round_rate(cd->gclk, max_rate);
>> +		if (gclk_round < 0)
>> +			goto pclk;
>> +
>> +		if (pclk_rate / gclk_round < 3)
>> +			goto pclk;
>> +
>> +		ret = mchp_pit64b_pres_compute(&pres, gclk_round, max_rate);
>> +		if (ret)
>> +			best_diff = abs(gclk_round - max_rate);
>> +		else
>> +			best_diff = abs(gclk_round / (pres + 1) - max_rate);
>> +		best_pres = pres;
>> +	}
>> +
>> +pclk:
>> +	/* Check if requested rate could be obtained using PCLK. */
>> +	ret = mchp_pit64b_pres_compute(&pres, pclk_rate, max_rate);
>> +	if (ret)
>> +		diff = abs(pclk_rate - max_rate);
>> +	else
>> +		diff = abs(pclk_rate / (pres + 1) - max_rate);
>> +
>> +	if (best_diff > diff) {
>> +		/* Use PCLK. */
>> +		cd->gclk = NULL;
>> +		best_pres = pres;
>> +	} else {
>> +		clk_set_rate(cd->gclk, gclk_round);
>> +	}
>> +
>> +	cd->pres = best_pres;
>> +
>> +	pr_info("PIT64B: using clk=%s with prescaler %u, freq=%lu [Hz]\n",
>> +		cd->gclk ? "gclk" : "pclk", cd->pres,
>> +		cd->gclk ? gclk_round / (cd->pres + 1)
>> +			 : pclk_rate / (cd->pres + 1));
>> +
>> +	return 0;
> 
> Lots of undocumented functionality which open codes stuff which exists
> already in the clk framework AFAICT.
> 
> Why are you not simply implementing this as clk framework components?
> 
> 
>             |-----|
>   gclk ---->|     |    |---------|
>             | MUX |--->| Divider |->
>   pclk ---->|     |    |---------|
>             |-----|
> 
> which is exaxtly how your hardware looks like. The clk framework has all
> the selection mechanisms in place and all this conditional clock stuff can
> be removed all over the place simply because you just ask for the desired
> frequency on init. Also suspend/resume and all the other stuff just works
> without all the mess involved.
> 

Ok, I'll look over this.

>> +free:
>> +	kfree(csd);
>> +	data.csd = NULL;
> 
> It does not matter here, but for correctness sake this is the wrong
> order and triggers my built-in UAF-race detector.
> 
> You need to NULL the pointer _before_ freeing the underlying memory.
> 

OK, agree.

>> +static int __init mchp_pit64b_dt_init(struct device_node *node)
>> +{
>> +	struct mchp_pit64b_common_data *cd;
>> +	u32 irq;
>> +	int ret;
>> +
>> +	if (data.csd && data.ced)
>> +		return -EBUSY;
> 
> Huch?

On some platforms I may have one instance on this hardware block, on some
other I may have more than one instances of this hardware block. When only
one hardware instance is present I want to use it as clockevent. When at
least 2 are present I want to use one as clockevent, another one as
clocksource and the rest, if any, to ignore them for clocksource, clockevent.

> 
>> +	cd = kzalloc(sizeof(*cd), GFP_KERNEL);
>> +	if (!cd)
>> +		return -ENOMEM;
> 
> If either data.csd or data.ced exists then the common data exists as
> well. Why would you allocate another instance?

I want to probe this driver at most twice in a Linux system:
- 1st time to offer clockevent functionality using one PIT64B hardware
  block
- 2nd time to offer clocksource functionality using another PIT64B hardware
  block

These 2 hardware instances are identical. I am not using only one PIT64B
hardware block for both clocksource and clockevent because I cannot achieve
high resolution timer functionality with it due to hardware limitations.

I used the following data structures for clocksource and clockevent:

struct mchp_pit64b_clksrc_data {
	struct clocksource *clksrc;
	struct mchp_pit64b_common_data *cd;
};

struct mchp_pit64b_clkevt_data {
	struct clock_event_device *clkevt;
	struct mchp_pit64b_common_data *cd;
};

So, for both hardware blocks I need different data structures, one for
clocksource functionality, one for clockevent functionality. Every hardware
block has its own base memory address, its own clocks and timers' frequency
and this is what I modeled with the mchp_pit64b_common_data:

struct mchp_pit64b_common_data {
        void __iomem *base;
        struct clk *pclk;
        struct clk *gclk;
        u64 cycles;
        u8 pres;
};

I will try to change the name of this data structure if it seems confusing.

> 
>> +
>> +	cd->pclk = of_clk_get_by_name(node, "pclk");
>> +	if (IS_ERR(cd->pclk)) {
>> +		ret = PTR_ERR(cd->pclk);
>> +		goto free;
>> +	}
> 
> ....
> 
>> +	if (!data.ced) {
> 
> And here you actually have a conditional which is confusing at best.

I want the first probe of this hardware to register the PIT64B hardware
block with clockevent functionality and the 2nd one to register a PIT64B
hardware block with clocksource functionality.

> 
>> +		irq = irq_of_parse_and_map(node, 0);
>> +		if (!irq) {
>> +			pr_debug("%s: Failed to get PIT64B clockevent IRQ!\n",
>> +				 MCHP_PIT64B_NAME);
>> +			ret = -ENODEV;
>> +			goto gclk_unprepare;
>> +		}
>> +		ret = mchp_pit64b_dt_init_clkevt(cd, irq);
>> +		if (ret)
>> +			goto irq_unmap;
>> +	} else {
>> +		ret = mchp_pit64b_dt_init_clksrc(cd);
>> +		if (ret)
>> +			goto gclk_unprepare;
>> +	}
> 
> So the first invocation of this init function is supposed to init the clock
> event device and the second one inits the clock source.

Yes.

> And both allocate
> common data. How is that common?

Ok, I will chose another name for this field.

Thank you for your review,
Claudiu Beznea

> 
> Thanks,
> 
> 	tglx
> 
> 

Re: [PATCH v2 2/2] clocksource/drivers/timer-microchip-pit64b: add Microchip PIT64B support

[Claudiu.Beznea]

On 04.11.2019 21:05, Thomas Gleixner wrote:
> On Mon, 4 Nov 2019, Claudiu Beznea wrote:
>> +struct mchp_pit64b_common_data {
>> +	void __iomem *base;
>> +	struct clk *pclk;
>> +	struct clk *gclk;
>> +	u64 cycles;
>> +	u8 pres;

[...]

>> +static int __init mchp_pit64b_pres_compute(u32 *pres, u32 clk_rate,
>> +					   u32 max_rate)
>> +{
>> +	u32 tmp;
>> +
>> +	for (*pres = 0; *pres < MCHP_PIT64B_PRES_MAX; (*pres)++) {
>> +		tmp = clk_rate / (*pres + 1);
>> +		if (tmp <= max_rate)
>> +			break;
>> +	}
>> +
>> +	if (*pres == MCHP_PIT64B_PRES_MAX)
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +static int __init mchp_pit64b_pres_prepare(struct mchp_pit64b_common_data *cd,
>> +					   unsigned long max_rate)
>> +{
>> +	unsigned long pclk_rate, diff = 0, best_diff = ULONG_MAX;
>> +	long gclk_round = 0;
>> +	u32 pres, best_pres = 0;
>> +	int ret = 0;
>> +
>> +	pclk_rate = clk_get_rate(cd->pclk);
>> +	if (!pclk_rate)
>> +		return -EINVAL;
>> +
>> +	if (cd->gclk) {
>> +		gclk_round = clk_round_rate(cd->gclk, max_rate);
>> +		if (gclk_round < 0)
>> +			goto pclk;
>> +
>> +		if (pclk_rate / gclk_round < 3)
>> +			goto pclk;
>> +
>> +		ret = mchp_pit64b_pres_compute(&pres, gclk_round, max_rate);
>> +		if (ret)
>> +			best_diff = abs(gclk_round - max_rate);
>> +		else
>> +			best_diff = abs(gclk_round / (pres + 1) - max_rate);
>> +		best_pres = pres;
>> +	}
>> +
>> +pclk:
>> +	/* Check if requested rate could be obtained using PCLK. */
>> +	ret = mchp_pit64b_pres_compute(&pres, pclk_rate, max_rate);
>> +	if (ret)
>> +		diff = abs(pclk_rate - max_rate);
>> +	else
>> +		diff = abs(pclk_rate / (pres + 1) - max_rate);
>> +
>> +	if (best_diff > diff) {
>> +		/* Use PCLK. */
>> +		cd->gclk = NULL;
>> +		best_pres = pres;
>> +	} else {
>> +		clk_set_rate(cd->gclk, gclk_round);
>> +	}
>> +
>> +	cd->pres = best_pres;
>> +
>> +	pr_info("PIT64B: using clk=%s with prescaler %u, freq=%lu [Hz]\n",
>> +		cd->gclk ? "gclk" : "pclk", cd->pres,
>> +		cd->gclk ? gclk_round / (cd->pres + 1)
>> +			 : pclk_rate / (cd->pres + 1));
>> +
>> +	return 0;
> 
> Lots of undocumented functionality which open codes stuff which exists
> already in the clk framework AFAICT.
> 
> Why are you not simply implementing this as clk framework components?
> 
> 
>             |-----|
>   gclk ---->|     |    |---------|
>             | MUX |--->| Divider |->
>   pclk ---->|     |    |---------|
>             |-----|
> 
> which is exaxtly how your hardware looks like. The clk framework has all
> the selection mechanisms in place and all this conditional clock stuff can
> be removed all over the place simply because you just ask for the desired
> frequency on init. Also suspend/resume and all the other stuff just works
> without all the mess involved.
> 

With regards to this: gclk and pclk are clock inputs to PIT64B hardware
block, GCLK rate could be requested and set from clock subsystem, PCLK rate
is fixed and could not be set via clock subsystem.
PCLK is the one that is feeding the PIT64B hardware block. The hardware
block would not work without it. At the same time it could be selected as
clock source for PIT64B's timer.
GCLK could only be selected as clock source for PIT64B's timer. PIT64B
hardware block with its timer functionality could work with only PCLK but
could not work only with GCLK.

The block diagram is as you pointed it:

                               PIT64B
PMC             +------------------------------------+
----+           |   |-----|                          |
    |-->gclk -->|-->|     |    |---------|  +-----+  |
    |           |   | MUX |--->| Divider |->|timer|  |
    |-->pclk -->|-->|     |    |---------|  +-----+  |
----+           |   |-----|                          |
                |      ^                             |
                |     sel                            |
                +------------------------------------+

The divider could be b/w 1 and 16.
Peripheral clock rate, on the platform that I'm using, is fixed at 200MHz.
In this case the minimum clock rate that could be used for peripheral clock
is 200MHz/16 = 12,5Mhz.
Generic clock rate vary depending on its clock source and dividers.
Lowest clock source of generic clock could be 32Khz, highest clock source
for generic clock could be 600Mhz (on the platform that I'm using).

Implementing a clock driver for this as you pointed it would involve
breaking the timer's driver in 2 parts: one
related to clock selection, one related to timer's functionalities.

Since, I'm thinking, the right place to put this driver is at91 clock tree
(drivers/clk/at91/) I should take a syscon to PIT64B in there so that to be
able to set the proper PIT64B's bits for MUX selection and divider.
Breaking this in multiple blocks will complicate a bit the things (e.g. I
would need to select 2 config flags for PIT64B block).

In the DT bindings I should anyway need to have a phandle to the peripheral
clock and one related to the MUX.

pmc: pmc@fffffc00 {
	compatible = "microchip,sam9x60-pmc", "syscon";
	reg = <0xfffffc00 0x200>;
	interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
	#clock-cells = <2>;
	clocks = <&clk32k 1>, <&clk32k 0>, <&main_xtal>;
	clock-names = "td_slck", "md_slck", "main_xtal";
};

pit64b: timer@f0028000 {
	compatible = "microchip,sam9x60-pit64b";
	reg = <0xf0028000 0x100>;
	interrupts = <37 IRQ_TYPE_LEVEL_HIGH 7>;
	clocks = <&pmc PMC_TYPE_PERIPHERAL 37>, <&pmc PMC_TYPE_MUX>;
	clock-names = "pclk", "mux";
};

One aspect here is that PCLK should be enabled all the time and the mux
should only select b/w PCLK and GCLK. And the mux is not actually part of
the PMC.

One other simpler thing that I can do is to assign GCLK rate directly from
device tree with "assigned-clock-rates" property and go all the time
directly with GCLK (assuming PCLK rate/GCLK rate ratio is all the time the
good one (it should be at least 3 for this IP)) and in this driver to
select all the time the GCLK clock.

Thank you,
Claudiu Beznea


[...]

> 
> So the first invocation of this init function is supposed to init the clock
> event device and the second one inits the clock source. And both allocate
> common data. How is that common?
> 
> Thanks,
> 
> 	tglx
> 
>