Hi,
Here's the version 8 and hopefully final version of my patchset, which
adds support for the Timer/Counter Unit found in JZ47xx SoCs from
Ingenic.
The big change is that the timer driver has been simplified. The code to
dynamically update the system timer or clocksource to a new channel has
been removed. Now, the system timer and clocksource are provided as
children nodes in the devicetree, and the TCU channel to use for these
is deduced from their respective memory resource. The PWM driver will
also deduce from its memory resources whether a given PWM channel can be
used, or is reserved for the system timers.
Kind regards,
- Paul Cercueil
The OST driver provides a clocksource and sched_clock that are much more
accurate than the default ones.
Signed-off-by: Paul Cercueil <[email protected]>
---
Notes:
v8: New patch
arch/mips/configs/gcw0_defconfig | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/arch/mips/configs/gcw0_defconfig b/arch/mips/configs/gcw0_defconfig
index 99ac1fa3b35f..7116400e8cbf 100644
--- a/arch/mips/configs/gcw0_defconfig
+++ b/arch/mips/configs/gcw0_defconfig
@@ -1,14 +1,14 @@
+CONFIG_NO_HZ_IDLE=y
+CONFIG_HIGH_RES_TIMERS=y
+CONFIG_PREEMPT_VOLUNTARY=y
+CONFIG_EMBEDDED=y
CONFIG_MACH_INGENIC=y
CONFIG_JZ4770_GCW0=y
CONFIG_HIGHMEM=y
-# CONFIG_BOUNCE is not set
-CONFIG_PREEMPT_VOLUNTARY=y
# CONFIG_SECCOMP is not set
-CONFIG_NO_HZ_IDLE=y
-CONFIG_HIGH_RES_TIMERS=y
-CONFIG_EMBEDDED=y
-# CONFIG_BLK_DEV_BSG is not set
# CONFIG_SUSPEND is not set
+# CONFIG_BLK_DEV_BSG is not set
+# CONFIG_BOUNCE is not set
CONFIG_NET=y
CONFIG_PACKET=y
CONFIG_UNIX=y
@@ -20,8 +20,13 @@ CONFIG_SERIAL_8250=y
# CONFIG_SERIAL_8250_DEPRECATED_OPTIONS is not set
CONFIG_SERIAL_8250_CONSOLE=y
CONFIG_SERIAL_8250_INGENIC=y
+CONFIG_WATCHDOG=y
+CONFIG_JZ4740_WDT=y
CONFIG_USB=y
CONFIG_USB_OHCI_HCD=y
CONFIG_USB_OHCI_HCD_PLATFORM=y
CONFIG_NOP_USB_XCEIV=y
+CONFIG_INGENIC_OST=y
+CONFIG_PWM=y
+CONFIG_PWM_JZ4740=y
CONFIG_TMPFS=y
--
2.11.0
The PWM in the JZ4725B works the same as in the JZ4740, except that it
only has 6 channels available instead of 8.
Signed-off-by: Paul Cercueil <[email protected]>
Acked-by: Thierry Reding <[email protected]>
---
Notes:
v5: New patch
v6: - Move of_device_id structure back at the bottom (less noise in
patch)
- Use device_get_match_data() instead of of_* variant
v7: No change
v8: No change
drivers/pwm/pwm-jz4740.c | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index cb8d8cec353f..a3a8da8af0de 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -24,6 +24,10 @@
#define NUM_PWM 8
+struct jz4740_soc_info {
+ unsigned int num_pwms;
+};
+
struct jz4740_pwm_chip {
struct pwm_chip chip;
struct clk *clks[NUM_PWM];
@@ -217,9 +221,14 @@ static const struct pwm_ops jz4740_pwm_ops = {
static int jz4740_pwm_probe(struct platform_device *pdev)
{
+ const struct jz4740_soc_info *soc_info;
struct jz4740_pwm_chip *jz4740;
struct device *dev = &pdev->dev;
+ soc_info = device_get_match_data(dev);
+ if (!soc_info)
+ return -EINVAL;
+
jz4740 = devm_kzalloc(dev, sizeof(*jz4740), GFP_KERNEL);
if (!jz4740)
return -ENOMEM;
@@ -238,7 +247,7 @@ static int jz4740_pwm_probe(struct platform_device *pdev)
jz4740->chip.dev = dev;
jz4740->chip.ops = &jz4740_pwm_ops;
- jz4740->chip.npwm = NUM_PWM;
+ jz4740->chip.npwm = soc_info->num_pwms;
jz4740->chip.base = -1;
jz4740->chip.of_xlate = of_pwm_xlate_with_flags;
jz4740->chip.of_pwm_n_cells = 3;
@@ -256,8 +265,17 @@ static int jz4740_pwm_remove(struct platform_device *pdev)
}
#ifdef CONFIG_OF
+static const struct jz4740_soc_info jz4740_soc_info = {
+ .num_pwms = 8,
+};
+
+static const struct jz4740_soc_info jz4725b_soc_info = {
+ .num_pwms = 6,
+};
+
static const struct of_device_id jz4740_pwm_dt_ids[] = {
- { .compatible = "ingenic,jz4740-pwm", },
+ { .compatible = "ingenic,jz4740-pwm", .data = &jz4740_soc_info },
+ { .compatible = "ingenic,jz4725b-pwm", .data = &jz4725b_soc_info },
{},
};
MODULE_DEVICE_TABLE(of, jz4740_pwm_dt_ids);
--
2.11.0
This driver handles the TCU (Timer Counter Unit) present on the Ingenic
JZ47xx SoCs, and provides the kernel with a system timer, and optionally
with a clocksource and a sched_clock.
It also provides clocks and interrupt handling to client drivers.
Signed-off-by: Paul Cercueil <[email protected]>
---
Notes:
v2: Use SPDX identifier for the license
v3: - Move documentation to its own patch
- Search the devicetree for PWM clients, and use all the TCU
channels that won't be used for PWM
v4: - Add documentation about why we search for PWM clients
- Verify that the PWM clients are for the TCU PWM driver
v5: Major overhaul. Too many changes to list. Consider it's a new
patch.
v6: - Add two API functions ingenic_tcu_request_channel and
ingenic_tcu_release_channel. To be used by the PWM driver to
request the use of a TCU channel. The driver will now dynamically
move away the system timer or clocksource to a new TCU channel.
- The system timer now defaults to channel 0, the clocksource now
defaults to channel 1 and is no more optional. The
ingenic,timer-channel and ingenic,clocksource-channel devicetree
properties are now gone.
- Fix round_rate / set_rate not calculating the prescale divider
the same way. This caused problems when (parent_rate / div) would
give a non-integer result. The behaviour is correct now.
- The clocksource clock is turned off on suspend now.
v7: Fix section mismatch by using builtin_platform_driver_probe()
v8: - Removed ingenic_tcu_[request,release]_channel, and the mechanism
to dynamically change the TCU channel of the system timer or
the clocksource.
- The driver's devicetree node can now have two more children
nodes, that correspond to the system timer and clocksource.
For these two, the driver will use the TCU timer that
correspond to the memory resource supplied in their
respective node.
drivers/clocksource/Kconfig | 10 +
drivers/clocksource/Makefile | 1 +
drivers/clocksource/ingenic-timer.c | 913 ++++++++++++++++++++++++++++++++++++
drivers/clocksource/ingenic-timer.h | 15 +
4 files changed, 939 insertions(+)
create mode 100644 drivers/clocksource/ingenic-timer.c
create mode 100644 drivers/clocksource/ingenic-timer.h
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 55c77e44bb2d..4e69af15c3e7 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -638,4 +638,14 @@ config GX6605S_TIMER
help
This option enables support for gx6605s SOC's timer.
+config INGENIC_TIMER
+ bool "Clocksource/timer using the TCU in Ingenic JZ SoCs"
+ depends on MIPS || COMPILE_TEST
+ depends on COMMON_CLK
+ select TIMER_OF
+ select IRQ_DOMAIN
+ select REGMAP
+ help
+ Support for the timer/counter unit of the Ingenic JZ SoCs.
+
endmenu
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index dd9138104568..7c8f790dcf67 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -75,6 +75,7 @@ obj-$(CONFIG_ASM9260_TIMER) += asm9260_timer.o
obj-$(CONFIG_H8300_TMR8) += h8300_timer8.o
obj-$(CONFIG_H8300_TMR16) += h8300_timer16.o
obj-$(CONFIG_H8300_TPU) += h8300_tpu.o
+obj-$(CONFIG_INGENIC_TIMER) += ingenic-timer.o
obj-$(CONFIG_CLKSRC_ST_LPC) += clksrc_st_lpc.o
obj-$(CONFIG_X86_NUMACHIP) += numachip.o
obj-$(CONFIG_ATCPIT100_TIMER) += timer-atcpit100.o
diff --git a/drivers/clocksource/ingenic-timer.c b/drivers/clocksource/ingenic-timer.c
new file mode 100644
index 000000000000..4cffd2e1f2a5
--- /dev/null
+++ b/drivers/clocksource/ingenic-timer.c
@@ -0,0 +1,913 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * JZ47xx SoCs TCU IRQ driver
+ * Copyright (C) 2018 Paul Cercueil <[email protected]>
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/clockchips.h>
+#include <linux/clocksource.h>
+#include <linux/interrupt.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/mfd/ingenic-tcu.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/sched_clock.h>
+
+#include <dt-bindings/clock/ingenic,tcu.h>
+
+#include "ingenic-timer.h"
+
+/* 8 channels max + watchdog + OST */
+#define TCU_CLK_COUNT 10
+
+enum tcu_clk_parent {
+ TCU_PARENT_PCLK,
+ TCU_PARENT_RTC,
+ TCU_PARENT_EXT,
+};
+
+struct ingenic_soc_info {
+ unsigned char num_channels;
+ bool has_ost;
+};
+
+struct ingenic_tcu_clk_info {
+ struct clk_init_data init_data;
+ u8 gate_bit;
+ u8 tcsr_reg;
+};
+
+struct ingenic_tcu_clk {
+ struct clk_hw hw;
+
+ struct regmap *map;
+ const struct ingenic_tcu_clk_info *info;
+
+ unsigned int idx;
+};
+
+#define to_tcu_clk(_hw) container_of(_hw, struct ingenic_tcu_clk, hw)
+
+struct ingenic_tcu {
+ const struct ingenic_soc_info *soc_info;
+ struct regmap *map;
+ struct clk *clk, *timer_clk, *cs_clk;
+
+ struct irq_domain *domain;
+ unsigned int nb_parent_irqs;
+ u32 parent_irqs[3];
+
+ struct clk_hw_onecell_data *clocks;
+
+ unsigned int timer_channel, cs_channel;
+ struct clock_event_device cevt;
+ struct clocksource cs;
+ char name[4];
+};
+
+static struct ingenic_tcu *ingenic_tcu;
+
+void __iomem *ingenic_tcu_base;
+EXPORT_SYMBOL_GPL(ingenic_tcu_base);
+
+static int ingenic_tcu_enable(struct clk_hw *hw)
+{
+ struct ingenic_tcu_clk *tcu_clk = to_tcu_clk(hw);
+ const struct ingenic_tcu_clk_info *info = tcu_clk->info;
+
+ regmap_write(tcu_clk->map, TCU_REG_TSCR, BIT(info->gate_bit));
+ return 0;
+}
+
+static void ingenic_tcu_disable(struct clk_hw *hw)
+{
+ struct ingenic_tcu_clk *tcu_clk = to_tcu_clk(hw);
+ const struct ingenic_tcu_clk_info *info = tcu_clk->info;
+
+ regmap_write(tcu_clk->map, TCU_REG_TSSR, BIT(info->gate_bit));
+}
+
+static int ingenic_tcu_is_enabled(struct clk_hw *hw)
+{
+ struct ingenic_tcu_clk *tcu_clk = to_tcu_clk(hw);
+ const struct ingenic_tcu_clk_info *info = tcu_clk->info;
+ unsigned int value;
+
+ regmap_read(tcu_clk->map, TCU_REG_TSR, &value);
+
+ return !(value & BIT(info->gate_bit));
+}
+
+static u8 ingenic_tcu_get_parent(struct clk_hw *hw)
+{
+ struct ingenic_tcu_clk *tcu_clk = to_tcu_clk(hw);
+ const struct ingenic_tcu_clk_info *info = tcu_clk->info;
+ unsigned int val = 0;
+ int ret;
+
+ ret = regmap_read(tcu_clk->map, info->tcsr_reg, &val);
+ WARN_ONCE(ret < 0, "Unable to read TCSR %i", tcu_clk->idx);
+
+ return ffs(val & TCU_TCSR_PARENT_CLOCK_MASK) - 1;
+}
+
+static int ingenic_tcu_set_parent(struct clk_hw *hw, u8 idx)
+{
+ struct ingenic_tcu_clk *tcu_clk = to_tcu_clk(hw);
+ const struct ingenic_tcu_clk_info *info = tcu_clk->info;
+ struct regmap *map = tcu_clk->map;
+ int ret;
+
+ /*
+ * Our clock provider has the CLK_SET_PARENT_GATE flag set, so we know
+ * that the clk is in unprepared state. To be able to access TCSR
+ * we must ungate the clock supply and we gate it again when done.
+ */
+
+ regmap_write(map, TCU_REG_TSCR, BIT(info->gate_bit));
+
+ ret = regmap_update_bits(map, info->tcsr_reg,
+ TCU_TCSR_PARENT_CLOCK_MASK, BIT(idx));
+ WARN_ONCE(ret < 0, "Unable to update TCSR %i", tcu_clk->idx);
+
+ regmap_write(map, TCU_REG_TSSR, BIT(info->gate_bit));
+
+ return 0;
+}
+
+static unsigned long ingenic_tcu_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct ingenic_tcu_clk *tcu_clk = to_tcu_clk(hw);
+ const struct ingenic_tcu_clk_info *info = tcu_clk->info;
+ unsigned int prescale;
+ int ret;
+
+ ret = regmap_read(tcu_clk->map, info->tcsr_reg, &prescale);
+ WARN_ONCE(ret < 0, "Unable to read TCSR %i", tcu_clk->idx);
+
+ prescale = (prescale & TCU_TCSR_PRESCALE_MASK) >> TCU_TCSR_PRESCALE_LSB;
+
+ return parent_rate >> (prescale * 2);
+}
+
+static u8 ingenic_tcu_get_prescale(unsigned long rate, unsigned long req_rate)
+{
+ u8 prescale;
+
+ for (prescale = 0; prescale < 5; prescale++)
+ if ((rate >> (prescale * 2)) <= req_rate)
+ return prescale;
+
+ return 5; /* /1024 divider */
+}
+
+static long ingenic_tcu_round_rate(struct clk_hw *hw, unsigned long req_rate,
+ unsigned long *parent_rate)
+{
+ unsigned long rate = *parent_rate;
+ u8 prescale;
+
+ if (req_rate > rate)
+ return -EINVAL;
+
+ prescale = ingenic_tcu_get_prescale(rate, req_rate);
+ return rate >> (prescale * 2);
+}
+
+static int ingenic_tcu_set_rate(struct clk_hw *hw, unsigned long req_rate,
+ unsigned long parent_rate)
+{
+ struct ingenic_tcu_clk *tcu_clk = to_tcu_clk(hw);
+ const struct ingenic_tcu_clk_info *info = tcu_clk->info;
+ struct regmap *map = tcu_clk->map;
+ u8 prescale = ingenic_tcu_get_prescale(parent_rate, req_rate);
+ int ret;
+
+ /*
+ * Our clock provider has the CLK_SET_RATE_GATE flag set, so we know
+ * that the clk is in unprepared state. To be able to access TCSR
+ * we must ungate the clock supply and we gate it again when done.
+ */
+
+ regmap_write(map, TCU_REG_TSCR, BIT(info->gate_bit));
+
+ ret = regmap_update_bits(map, info->tcsr_reg,
+ TCU_TCSR_PRESCALE_MASK,
+ prescale << TCU_TCSR_PRESCALE_LSB);
+ WARN_ONCE(ret < 0, "Unable to update TCSR %i", tcu_clk->idx);
+
+ regmap_write(map, TCU_REG_TSSR, BIT(info->gate_bit));
+
+ return 0;
+}
+
+static const struct clk_ops ingenic_tcu_clk_ops = {
+ .get_parent = ingenic_tcu_get_parent,
+ .set_parent = ingenic_tcu_set_parent,
+
+ .recalc_rate = ingenic_tcu_recalc_rate,
+ .round_rate = ingenic_tcu_round_rate,
+ .set_rate = ingenic_tcu_set_rate,
+
+ .enable = ingenic_tcu_enable,
+ .disable = ingenic_tcu_disable,
+ .is_enabled = ingenic_tcu_is_enabled,
+};
+
+static const char * const ingenic_tcu_timer_parents[] = {
+ [TCU_PARENT_PCLK] = "pclk",
+ [TCU_PARENT_RTC] = "rtc",
+ [TCU_PARENT_EXT] = "ext",
+};
+
+#define DEF_TIMER(_name, _gate_bit, _tcsr) \
+ { \
+ .init_data = { \
+ .name = _name, \
+ .parent_names = ingenic_tcu_timer_parents, \
+ .num_parents = ARRAY_SIZE(ingenic_tcu_timer_parents),\
+ .ops = &ingenic_tcu_clk_ops, \
+ .flags = CLK_SET_RATE_GATE | CLK_SET_PARENT_GATE,\
+ }, \
+ .gate_bit = _gate_bit, \
+ .tcsr_reg = _tcsr, \
+ }
+static const struct ingenic_tcu_clk_info ingenic_tcu_clk_info[] = {
+ [TCU_CLK_TIMER0] = DEF_TIMER("timer0", 0, TCU_REG_TCSRc(0)),
+ [TCU_CLK_TIMER1] = DEF_TIMER("timer1", 1, TCU_REG_TCSRc(1)),
+ [TCU_CLK_TIMER2] = DEF_TIMER("timer2", 2, TCU_REG_TCSRc(2)),
+ [TCU_CLK_TIMER3] = DEF_TIMER("timer3", 3, TCU_REG_TCSRc(3)),
+ [TCU_CLK_TIMER4] = DEF_TIMER("timer4", 4, TCU_REG_TCSRc(4)),
+ [TCU_CLK_TIMER5] = DEF_TIMER("timer5", 5, TCU_REG_TCSRc(5)),
+ [TCU_CLK_TIMER6] = DEF_TIMER("timer6", 6, TCU_REG_TCSRc(6)),
+ [TCU_CLK_TIMER7] = DEF_TIMER("timer7", 7, TCU_REG_TCSRc(7)),
+};
+
+static const struct ingenic_tcu_clk_info ingenic_tcu_watchdog_clk_info =
+ DEF_TIMER("wdt", 16, TCU_REG_WDT_TCSR);
+static const struct ingenic_tcu_clk_info ingenic_tcu_ost_clk_info =
+ DEF_TIMER("ost", 15, TCU_REG_OST_TCSR);
+#undef DEF_TIMER
+
+static void ingenic_tcu_intc_cascade(struct irq_desc *desc)
+{
+ struct irq_chip *irq_chip = irq_data_get_irq_chip(&desc->irq_data);
+ struct irq_domain *domain = irq_desc_get_handler_data(desc);
+ struct irq_chip_generic *gc = irq_get_domain_generic_chip(domain, 0);
+ struct regmap *map = gc->private;
+ uint32_t irq_reg, irq_mask;
+ unsigned int i;
+
+ regmap_read(map, TCU_REG_TFR, &irq_reg);
+ regmap_read(map, TCU_REG_TMR, &irq_mask);
+
+ chained_irq_enter(irq_chip, desc);
+
+ irq_reg &= ~irq_mask;
+
+ for_each_set_bit(i, (unsigned long *)&irq_reg, 32)
+ generic_handle_irq(irq_linear_revmap(domain, i));
+
+ chained_irq_exit(irq_chip, desc);
+}
+
+static void ingenic_tcu_gc_unmask_enable_reg(struct irq_data *d)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+ struct irq_chip_type *ct = irq_data_get_chip_type(d);
+ struct regmap *map = gc->private;
+ u32 mask = d->mask;
+
+ irq_gc_lock(gc);
+ regmap_write(map, ct->regs.ack, mask);
+ regmap_write(map, ct->regs.enable, mask);
+ *ct->mask_cache |= mask;
+ irq_gc_unlock(gc);
+}
+
+static void ingenic_tcu_gc_mask_disable_reg(struct irq_data *d)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+ struct irq_chip_type *ct = irq_data_get_chip_type(d);
+ struct regmap *map = gc->private;
+ u32 mask = d->mask;
+
+ irq_gc_lock(gc);
+ regmap_write(map, ct->regs.disable, mask);
+ *ct->mask_cache &= ~mask;
+ irq_gc_unlock(gc);
+}
+
+static void ingenic_tcu_gc_mask_disable_reg_and_ack(struct irq_data *d)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+ struct irq_chip_type *ct = irq_data_get_chip_type(d);
+ struct regmap *map = gc->private;
+ u32 mask = d->mask;
+
+ irq_gc_lock(gc);
+ regmap_write(map, ct->regs.ack, mask);
+ regmap_write(map, ct->regs.disable, mask);
+ irq_gc_unlock(gc);
+}
+
+static u64 notrace ingenic_tcu_timer_read(void)
+{
+ unsigned int channel = ingenic_tcu->cs_channel;
+ u16 count;
+
+ count = readw(ingenic_tcu_base + TCU_REG_TCNTc(channel));
+
+ return count;
+}
+
+static inline struct ingenic_tcu *to_ingenic_tcu(struct clock_event_device *evt)
+{
+ return container_of(evt, struct ingenic_tcu, cevt);
+}
+
+static int ingenic_tcu_cevt_set_state_shutdown(struct clock_event_device *evt)
+{
+ struct ingenic_tcu *tcu = to_ingenic_tcu(evt);
+
+ regmap_write(tcu->map, TCU_REG_TECR, BIT(tcu->timer_channel));
+ return 0;
+}
+
+static int ingenic_tcu_cevt_set_next(unsigned long next,
+ struct clock_event_device *evt)
+{
+ struct ingenic_tcu *tcu = to_ingenic_tcu(evt);
+
+ if (next > 0xffff)
+ return -EINVAL;
+
+ regmap_write(tcu->map, TCU_REG_TDFRc(tcu->timer_channel), next);
+ regmap_write(tcu->map, TCU_REG_TCNTc(tcu->timer_channel), 0);
+ regmap_write(tcu->map, TCU_REG_TESR, BIT(tcu->timer_channel));
+
+ return 0;
+}
+
+static irqreturn_t ingenic_tcu_cevt_cb(int irq, void *dev_id)
+{
+ struct clock_event_device *evt = dev_id;
+ struct ingenic_tcu *tcu = to_ingenic_tcu(evt);
+
+ regmap_write(tcu->map, TCU_REG_TECR, BIT(tcu->timer_channel));
+
+ if (evt->event_handler)
+ evt->event_handler(evt);
+
+ return IRQ_HANDLED;
+}
+
+static int __init ingenic_tcu_register_clock(struct ingenic_tcu *tcu,
+ unsigned int idx, enum tcu_clk_parent parent,
+ const struct ingenic_tcu_clk_info *info,
+ struct clk_hw_onecell_data *clocks)
+{
+ struct ingenic_tcu_clk *tcu_clk;
+ int err;
+
+ tcu_clk = kzalloc(sizeof(*tcu_clk), GFP_KERNEL);
+ if (!tcu_clk)
+ return -ENOMEM;
+
+ tcu_clk->hw.init = &info->init_data;
+ tcu_clk->idx = idx;
+ tcu_clk->info = info;
+ tcu_clk->map = tcu->map;
+
+ /* Reset channel and clock divider, set default parent */
+ ingenic_tcu_enable(&tcu_clk->hw);
+ regmap_update_bits(tcu->map, info->tcsr_reg, 0xffff, BIT(parent));
+ ingenic_tcu_disable(&tcu_clk->hw);
+
+ err = clk_hw_register(NULL, &tcu_clk->hw);
+ if (err)
+ goto err_free_tcu_clk;
+
+ err = clk_hw_register_clkdev(&tcu_clk->hw, info->init_data.name, NULL);
+ if (err)
+ goto err_clk_unregister;
+
+ clocks->hws[idx] = &tcu_clk->hw;
+ return 0;
+
+err_clk_unregister:
+ clk_hw_unregister(&tcu_clk->hw);
+err_free_tcu_clk:
+ kfree(tcu_clk);
+ return err;
+}
+
+static int __init ingenic_tcu_clk_init(struct ingenic_tcu *tcu,
+ struct device_node *np)
+{
+ size_t i;
+ int ret;
+
+ tcu->clocks = kzalloc(sizeof(*tcu->clocks) +
+ sizeof(*tcu->clocks->hws) * TCU_CLK_COUNT,
+ GFP_KERNEL);
+ if (!tcu->clocks)
+ return -ENOMEM;
+
+ tcu->clocks->num = TCU_CLK_COUNT;
+
+ for (i = 0; i < tcu->soc_info->num_channels; i++) {
+ ret = ingenic_tcu_register_clock(tcu, i, TCU_PARENT_EXT,
+ &ingenic_tcu_clk_info[i], tcu->clocks);
+ if (ret) {
+ pr_err("ingenic-timer: cannot register clock %i\n", i);
+ goto err_unregister_timer_clocks;
+ }
+ }
+
+ /*
+ * We set EXT as the default parent clock for all the TCU clocks
+ * except for the watchdog one, where we set the RTC clock as the
+ * parent. Since the EXT and PCLK are much faster than the RTC clock,
+ * the watchdog would kick after a maximum time of 5s, and we might
+ * want a slower kicking time.
+ */
+ ret = ingenic_tcu_register_clock(tcu, TCU_CLK_WDT, TCU_PARENT_RTC,
+ &ingenic_tcu_watchdog_clk_info, tcu->clocks);
+ if (ret) {
+ pr_err("ingenic-timer: cannot register watchdog clock\n");
+ goto err_unregister_timer_clocks;
+ }
+
+ if (tcu->soc_info->has_ost) {
+ ret = ingenic_tcu_register_clock(tcu, TCU_CLK_OST,
+ TCU_PARENT_EXT,
+ &ingenic_tcu_ost_clk_info,
+ tcu->clocks);
+ if (ret) {
+ pr_err("ingenic-timer: cannot register ost clock\n");
+ goto err_unregister_watchdog_clock;
+ }
+ }
+
+ ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, tcu->clocks);
+ if (ret) {
+ pr_err("ingenic-timer: cannot add OF clock provider\n");
+ goto err_unregister_ost_clock;
+ }
+
+ return 0;
+
+err_unregister_ost_clock:
+ if (tcu->soc_info->has_ost)
+ clk_hw_unregister(tcu->clocks->hws[i + 1]);
+err_unregister_watchdog_clock:
+ clk_hw_unregister(tcu->clocks->hws[i]);
+err_unregister_timer_clocks:
+ for (i = 0; i < tcu->clocks->num; i++)
+ if (tcu->clocks->hws[i])
+ clk_hw_unregister(tcu->clocks->hws[i]);
+ kfree(tcu->clocks);
+ return ret;
+}
+
+static void __init ingenic_tcu_clk_cleanup(struct ingenic_tcu *tcu,
+ struct device_node *np)
+{
+ unsigned int i;
+
+ of_clk_del_provider(np);
+
+ for (i = 0; i < tcu->clocks->num; i++)
+ clk_hw_unregister(tcu->clocks->hws[i]);
+ kfree(tcu->clocks);
+}
+
+static int __init ingenic_tcu_intc_init(struct ingenic_tcu *tcu,
+ struct device_node *np)
+{
+ struct irq_chip_generic *gc;
+ struct irq_chip_type *ct;
+ int err, i, irqs;
+
+ irqs = of_property_count_elems_of_size(np, "interrupts", sizeof(u32));
+ if (irqs < 0 || irqs > ARRAY_SIZE(tcu->parent_irqs))
+ return -EINVAL;
+
+ tcu->nb_parent_irqs = irqs;
+
+ tcu->domain = irq_domain_add_linear(np, 32,
+ &irq_generic_chip_ops, NULL);
+ if (!tcu->domain)
+ return -ENOMEM;
+
+ err = irq_alloc_domain_generic_chips(tcu->domain, 32, 1, "TCU",
+ handle_level_irq, 0, IRQ_NOPROBE | IRQ_LEVEL, 0);
+ if (err)
+ goto out_domain_remove;
+
+ gc = irq_get_domain_generic_chip(tcu->domain, 0);
+ ct = gc->chip_types;
+
+ gc->wake_enabled = IRQ_MSK(32);
+ gc->private = tcu->map;
+
+ ct->regs.disable = TCU_REG_TMSR;
+ ct->regs.enable = TCU_REG_TMCR;
+ ct->regs.ack = TCU_REG_TFCR;
+ ct->chip.irq_unmask = ingenic_tcu_gc_unmask_enable_reg;
+ ct->chip.irq_mask = ingenic_tcu_gc_mask_disable_reg;
+ ct->chip.irq_mask_ack = ingenic_tcu_gc_mask_disable_reg_and_ack;
+ ct->chip.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE;
+
+ /* Mask all IRQs by default */
+ regmap_write(tcu->map, TCU_REG_TMSR, IRQ_MSK(32));
+
+ /* On JZ4740, timer 0 and timer 1 have their own interrupt line;
+ * timers 2-7 share one interrupt.
+ * On SoCs >= JZ4770, timer 5 has its own interrupt line;
+ * timers 0-4 and 6-7 share one single interrupt.
+ *
+ * To keep things simple, we just register the same handler to
+ * all parent interrupts. The handler will properly detect which
+ * channel fired the interrupt.
+ */
+ for (i = 0; i < irqs; i++) {
+ tcu->parent_irqs[i] = irq_of_parse_and_map(np, i);
+ if (!tcu->parent_irqs[i]) {
+ err = -EINVAL;
+ goto out_unmap_irqs;
+ }
+
+ irq_set_chained_handler_and_data(tcu->parent_irqs[i],
+ ingenic_tcu_intc_cascade, tcu->domain);
+ }
+
+ return 0;
+
+out_unmap_irqs:
+ for (; i > 0; i--)
+ irq_dispose_mapping(tcu->parent_irqs[i - 1]);
+out_domain_remove:
+ irq_domain_remove(tcu->domain);
+ return err;
+}
+
+static void __init ingenic_tcu_intc_cleanup(struct ingenic_tcu *tcu)
+{
+ unsigned int i;
+
+ for (i = 0; i < tcu->nb_parent_irqs; i++)
+ irq_dispose_mapping(tcu->parent_irqs[i]);
+
+ irq_domain_remove(tcu->domain);
+}
+
+static int __init ingenic_tcu_timer_init(struct ingenic_tcu *tcu,
+ struct device_node *np)
+{
+ unsigned int timer_virq;
+ unsigned long rate;
+ int err;
+
+ tcu->timer_clk = of_clk_get_by_name(np, "timer");
+
+ err = clk_prepare_enable(tcu->timer_clk);
+ if (err)
+ return err;
+
+ rate = clk_get_rate(tcu->timer_clk);
+ if (!rate) {
+ err = -EINVAL;
+ goto err_clk_disable;
+ }
+
+ timer_virq = irq_of_parse_and_map(np, 0);
+ if (!timer_virq) {
+ err = -EINVAL;
+ goto err_clk_disable;
+ }
+
+ snprintf(tcu->name, sizeof(tcu->name), "TCU");
+
+ err = request_irq(timer_virq, ingenic_tcu_cevt_cb, IRQF_TIMER,
+ tcu->name, &tcu->cevt);
+ if (err)
+ goto err_irq_dispose_mapping;
+
+ tcu->cevt.cpumask = cpumask_of(smp_processor_id());
+ tcu->cevt.features = CLOCK_EVT_FEAT_ONESHOT;
+ tcu->cevt.name = tcu->name;
+ tcu->cevt.rating = 200;
+ tcu->cevt.set_state_shutdown = ingenic_tcu_cevt_set_state_shutdown;
+ tcu->cevt.set_next_event = ingenic_tcu_cevt_set_next;
+
+ clockevents_config_and_register(&tcu->cevt, rate, 10, 0xffff);
+
+ return 0;
+
+err_irq_dispose_mapping:
+ irq_dispose_mapping(timer_virq);
+err_clk_disable:
+ clk_disable_unprepare(tcu->timer_clk);
+ return err;
+}
+
+static int __init ingenic_tcu_clocksource_init(struct ingenic_tcu *tcu,
+ struct device_node *np)
+{
+ unsigned int channel = tcu->cs_channel;
+ struct clocksource *cs = &tcu->cs;
+ unsigned long rate;
+ int err;
+
+ tcu->cs_clk = of_clk_get_by_name(np, "timer");
+
+ err = clk_prepare_enable(tcu->cs_clk);
+ if (err)
+ return err;
+
+ rate = clk_get_rate(tcu->cs_clk);
+ if (!rate) {
+ err = -EINVAL;
+ goto err_clk_disable;
+ }
+
+ /* Reset channel */
+ regmap_update_bits(tcu->map, TCU_REG_TCSRc(channel),
+ 0xffff & ~TCU_TCSR_RESERVED_BITS, 0);
+
+ /* Reset counter */
+ regmap_write(tcu->map, TCU_REG_TDFRc(channel), 0xffff);
+ regmap_write(tcu->map, TCU_REG_TCNTc(channel), 0);
+
+ /* Enable channel */
+ regmap_write(tcu->map, TCU_REG_TESR, BIT(channel));
+
+ cs->name = "ingenic-timer";
+ cs->rating = 200;
+ cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
+ cs->mask = CLOCKSOURCE_MASK(16);
+ cs->read = (u64 (*)(struct clocksource *))ingenic_tcu_timer_read;
+
+ err = clocksource_register_hz(cs, rate);
+ if (err)
+ goto err_clk_disable;
+
+ sched_clock_register(ingenic_tcu_timer_read, 16, rate);
+
+ return 0;
+
+err_clk_disable:
+ clk_disable_unprepare(tcu->cs_clk);
+ return err;
+}
+
+static void __init ingenic_tcu_clocksource_cleanup(struct ingenic_tcu *tcu)
+{
+ if (tcu->cs_clk) {
+ clocksource_unregister(&tcu->cs);
+ clk_disable_unprepare(tcu->cs_clk);
+ }
+}
+
+static int __init ingenic_tcu_get_tcu_channel(struct ingenic_tcu *tcu,
+ struct device_node *np,
+ struct resource *parent_res)
+{
+ int ret, channel;
+ struct resource res;
+ unsigned long offset;
+
+ ret = of_address_to_resource(np, 0, &res);
+ if (ret < 0)
+ return ret;
+
+ if ((res.start % TCU_CHANNEL_STRIDE) ||
+ (resource_size(&res) != TCU_CHANNEL_STRIDE))
+ return -EINVAL;
+
+ offset = res.start - TCU_REG_TDFR0 - parent_res->start;
+ channel = offset / TCU_CHANNEL_STRIDE;
+
+ if (channel >= tcu->soc_info->num_channels)
+ return -EINVAL;
+
+ return channel;
+}
+
+static const struct regmap_config ingenic_tcu_regmap_config = {
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+};
+
+static const struct ingenic_soc_info jz4740_soc_info = {
+ .num_channels = 8,
+ .has_ost = false,
+};
+
+static const struct ingenic_soc_info jz4725b_soc_info = {
+ .num_channels = 6,
+ .has_ost = true,
+};
+
+static const struct ingenic_soc_info jz4770_soc_info = {
+ .num_channels = 8,
+ .has_ost = true,
+};
+
+static const struct of_device_id ingenic_tcu_of_match[] = {
+ { .compatible = "ingenic,jz4740-tcu", .data = &jz4740_soc_info, },
+ { .compatible = "ingenic,jz4725b-tcu", .data = &jz4725b_soc_info, },
+ { .compatible = "ingenic,jz4770-tcu", .data = &jz4770_soc_info, },
+ { }
+};
+
+static int __init ingenic_tcu_init(struct device_node *np)
+{
+ const struct of_device_id *id = of_match_node(ingenic_tcu_of_match, np);
+ struct device_node *timer_node, *cs_node;
+ struct ingenic_tcu *tcu;
+ struct resource res;
+ void __iomem *base;
+ int ret;
+
+ of_node_clear_flag(np, OF_POPULATED);
+
+ tcu = kzalloc(sizeof(*tcu), GFP_KERNEL);
+ if (!tcu)
+ return -ENOMEM;
+
+ tcu->soc_info = (const struct ingenic_soc_info *)id->data;
+ ingenic_tcu = tcu;
+
+ base = of_io_request_and_map(np, 0, NULL);
+ if (IS_ERR(base)) {
+ ret = PTR_ERR(base);
+ goto err_free_ingenic_tcu;
+ }
+
+ of_address_to_resource(np, 0, &res);
+
+ ingenic_tcu_base = base;
+
+ tcu->map = regmap_init_mmio(NULL, base, &ingenic_tcu_regmap_config);
+ if (IS_ERR(tcu->map)) {
+ ret = PTR_ERR(tcu->map);
+ goto err_iounmap;
+ }
+
+ tcu->clk = of_clk_get_by_name(np, "tcu");
+ if (IS_ERR(tcu->clk)) {
+ ret = PTR_ERR(tcu->clk);
+ pr_crit("ingenic-tcu: Unable to find TCU clock: %i\n", ret);
+ goto err_free_regmap;
+ }
+
+ ret = clk_prepare_enable(tcu->clk);
+ if (ret) {
+ pr_crit("ingenic-tcu: Unable to enable TCU clock: %i\n", ret);
+ goto err_clk_put;
+ }
+
+ ret = ingenic_tcu_intc_init(tcu, np);
+ if (ret)
+ goto err_clk_disable;
+
+ ret = ingenic_tcu_clk_init(tcu, np);
+ if (ret)
+ goto err_tcu_intc_cleanup;
+
+ cs_node = of_find_compatible_node(np, NULL,
+ "ingenic,jz4740-tcu-clocksource");
+ if (of_device_is_available(cs_node)) {
+ ret = ingenic_tcu_get_tcu_channel(tcu, cs_node, &res);
+ if (ret < 0)
+ goto err_tcu_clk_cleanup;
+
+ tcu->cs_channel = ret;
+
+ ret = ingenic_tcu_clocksource_init(tcu, cs_node);
+ if (ret)
+ goto err_tcu_clk_cleanup;
+ }
+
+ timer_node = of_find_compatible_node(np, NULL,
+ "ingenic,jz4740-tcu-timer");
+ if (of_device_is_available(timer_node)) {
+ ret = ingenic_tcu_get_tcu_channel(tcu, timer_node, &res);
+ if (ret < 0)
+ goto err_tcu_clocksource_cleanup;
+
+ tcu->timer_channel = ret;
+
+ ret = ingenic_tcu_timer_init(tcu, timer_node);
+ if (ret)
+ goto err_tcu_clocksource_cleanup;
+ }
+
+
+ return 0;
+
+err_tcu_clocksource_cleanup:
+ ingenic_tcu_clocksource_cleanup(tcu);
+err_tcu_clk_cleanup:
+ ingenic_tcu_clk_cleanup(tcu, np);
+err_tcu_intc_cleanup:
+ ingenic_tcu_intc_cleanup(tcu);
+err_clk_disable:
+ clk_disable_unprepare(tcu->clk);
+err_clk_put:
+ clk_put(tcu->clk);
+err_free_regmap:
+ regmap_exit(tcu->map);
+err_iounmap:
+ iounmap(base);
+ release_mem_region(res.start, resource_size(&res));
+err_free_ingenic_tcu:
+ kfree(tcu);
+ return ret;
+}
+
+TIMER_OF_DECLARE(jz4740_tcu_intc, "ingenic,jz4740-tcu", ingenic_tcu_init);
+TIMER_OF_DECLARE(jz4725b_tcu_intc, "ingenic,jz4725b-tcu", ingenic_tcu_init);
+TIMER_OF_DECLARE(jz4770_tcu_intc, "ingenic,jz4770-tcu", ingenic_tcu_init);
+
+
+static int __init ingenic_tcu_probe(struct platform_device *pdev)
+{
+ platform_set_drvdata(pdev, ingenic_tcu);
+
+ regmap_attach_dev(&pdev->dev, ingenic_tcu->map,
+ &ingenic_tcu_regmap_config);
+
+ return devm_of_platform_populate(&pdev->dev);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int ingenic_tcu_suspend(struct device *dev)
+{
+ struct ingenic_tcu *tcu = dev_get_drvdata(dev);
+
+ clk_disable(tcu->cs_clk);
+ clk_disable(tcu->timer_clk);
+ clk_disable(tcu->clk);
+ return 0;
+}
+
+static int ingenic_tcu_resume(struct device *dev)
+{
+ struct ingenic_tcu *tcu = dev_get_drvdata(dev);
+ int ret;
+
+ ret = clk_enable(tcu->clk);
+ if (ret)
+ return ret;
+
+ ret = clk_enable(tcu->timer_clk);
+ if (ret)
+ goto err_tcu_clk_disable;
+
+ ret = clk_enable(tcu->cs_clk);
+ if (ret)
+ goto err_tcu_timer_clk_disable;
+
+ return 0;
+
+err_tcu_timer_clk_disable:
+ clk_disable(tcu->timer_clk);
+err_tcu_clk_disable:
+ clk_disable(tcu->clk);
+ return ret;
+}
+
+static const struct dev_pm_ops ingenic_tcu_pm_ops = {
+ /* _noirq: We want the TCU clock to be gated last / ungated first */
+ .suspend_noirq = ingenic_tcu_suspend,
+ .resume_noirq = ingenic_tcu_resume,
+};
+#define INGENIC_TCU_PM_OPS (&ingenic_tcu_pm_ops)
+
+#else
+#define INGENIC_TCU_PM_OPS NULL
+#endif /* CONFIG_PM_SUSPEND */
+
+static struct platform_driver ingenic_tcu_driver = {
+ .driver = {
+ .name = "ingenic-tcu",
+ .pm = INGENIC_TCU_PM_OPS,
+ .of_match_table = ingenic_tcu_of_match,
+ },
+};
+builtin_platform_driver_probe(ingenic_tcu_driver, ingenic_tcu_probe);
diff --git a/drivers/clocksource/ingenic-timer.h b/drivers/clocksource/ingenic-timer.h
new file mode 100644
index 000000000000..fa43da836ab6
--- /dev/null
+++ b/drivers/clocksource/ingenic-timer.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __DRIVERS_CLOCKSOURCE_INGENIC_TIMER_H__
+#define __DRIVERS_CLOCKSOURCE_INGENIC_TIMER_H__
+
+#include <linux/compiler_types.h>
+
+/*
+ * README: For use *ONLY* by the ingenic-ost driver.
+ * Regular drivers which want to access the TCU registers
+ * must have ingenic-timer as parent and retrieve the regmap
+ * doing dev_get_regmap(pdev->dev.parent);
+ */
+extern void __iomem *ingenic_tcu_base;
+
+#endif /* __DRIVERS_CLOCKSOURCE_INGENIC_TIMER_H__ */
--
2.11.0
Depending on MACH_JZ47xx prevent us from creating a generic kernel that
works on more than one MIPS board. Instead, we just depend on MIPS being
set.
On other architectures, this driver can still be built, thanks to
COMPILE_TEST. This is used by automated tools to find bugs, for
instance.
Signed-off-by: Paul Cercueil <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
---
Notes:
v5: New patch
v6: No change
v7: No change
v8: No change
drivers/watchdog/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index cfd7368fc3c0..eb5dbb1db64d 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1496,7 +1496,7 @@ config INDYDOG
config JZ4740_WDT
tristate "Ingenic jz4740 SoC hardware watchdog"
- depends on MACH_JZ4740 || MACH_JZ4780
+ depends on MIPS || COMPILE_TEST
depends on COMMON_CLK
select WATCHDOG_CORE
select INGENIC_TIMER
--
2.11.0
Add DTS nodes for the JZ4780, JZ4770 and JZ4740 devicetree files.
Signed-off-by: Paul Cercueil <[email protected]>
---
Notes:
v5: New patch
v6: Fix register lengths in watchdog/pwm nodes
v7: No change
v8: - Fix wrong start address for PWM node
- Add system timer and clocksource sub-nodes
arch/mips/boot/dts/ingenic/jz4740.dtsi | 69 ++++++++++++++++++++++++--
arch/mips/boot/dts/ingenic/jz4770.dtsi | 81 +++++++++++++++++++++++++++++++
arch/mips/boot/dts/ingenic/jz4780.dtsi | 88 ++++++++++++++++++++++++++++++----
3 files changed, 225 insertions(+), 13 deletions(-)
diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi b/arch/mips/boot/dts/ingenic/jz4740.dtsi
index 6fb16fd24035..d625e8acb695 100644
--- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
#include <dt-bindings/clock/jz4740-cgu.h>
+#include <dt-bindings/clock/ingenic,tcu.h>
/ {
#address-cells = <1>;
@@ -45,12 +46,70 @@
#clock-cells = <1>;
};
- watchdog: watchdog@10002000 {
- compatible = "ingenic,jz4740-watchdog";
- reg = <0x10002000 0x10>;
+ tcu: timer@10002000 {
+ compatible = "ingenic,jz4740-tcu";
+ reg = <0x10002000 0x1000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x10002000 0x1000>;
- clocks = <&cgu JZ4740_CLK_RTC>;
- clock-names = "rtc";
+ #clock-cells = <1>;
+
+ clocks = <&cgu JZ4740_CLK_RTC
+ &cgu JZ4740_CLK_EXT
+ &cgu JZ4740_CLK_PCLK
+ &cgu JZ4740_CLK_TCU>;
+ clock-names = "rtc", "ext", "pclk", "tcu";
+
+ interrupt-controller;
+ #interrupt-cells = <1>;
+
+ interrupt-parent = <&intc>;
+ interrupts = <23 22 21>;
+
+ watchdog: watchdog@0 {
+ compatible = "ingenic,jz4740-watchdog";
+ reg = <0x0 0xc>;
+
+ clocks = <&tcu TCU_CLK_WDT>;
+ clock-names = "wdt";
+ };
+
+ timer: timer@40 {
+ compatible = "ingenic,jz4740-tcu-timer";
+ reg = <0x40 0x10>;
+
+ clocks = <&tcu TCU_CLK_TIMER0>;
+ clock-names = "timer";
+
+ interrupts = <0>;
+ };
+
+ clocksource: timer@50 {
+ compatible = "ingenic,jz4740-tcu-clocksource";
+ reg = <0x50 0x10>;
+
+ clocks = <&tcu TCU_CLK_TIMER1>;
+ clock-names = "timer";
+ };
+
+ pwm: pwm@60 {
+ compatible = "ingenic,jz4740-pwm";
+ reg = <0x60 0x60>;
+
+ #pwm-cells = <3>;
+
+ clocks = <&tcu TCU_CLK_TIMER0
+ &tcu TCU_CLK_TIMER1
+ &tcu TCU_CLK_TIMER2
+ &tcu TCU_CLK_TIMER3
+ &tcu TCU_CLK_TIMER4
+ &tcu TCU_CLK_TIMER5
+ &tcu TCU_CLK_TIMER6
+ &tcu TCU_CLK_TIMER7>;
+ clock-names = "timer0", "timer1", "timer2", "timer3",
+ "timer4", "timer5", "timer6", "timer7";
+ };
};
rtc_dev: rtc@10003000 {
diff --git a/arch/mips/boot/dts/ingenic/jz4770.dtsi b/arch/mips/boot/dts/ingenic/jz4770.dtsi
index 49ede6c14ff3..6008975a307f 100644
--- a/arch/mips/boot/dts/ingenic/jz4770.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4770.dtsi
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
#include <dt-bindings/clock/jz4770-cgu.h>
+#include <dt-bindings/clock/ingenic,tcu.h>
/ {
#address-cells = <1>;
@@ -46,6 +47,86 @@
#clock-cells = <1>;
};
+ tcu: timer@10002000 {
+ compatible = "ingenic,jz4770-tcu";
+ reg = <0x10002000 0x1000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x10002000 0x1000>;
+
+ #clock-cells = <1>;
+
+ clocks = <&cgu JZ4770_CLK_RTC
+ &cgu JZ4770_CLK_EXT
+ &cgu JZ4770_CLK_PCLK
+ &cgu JZ4770_CLK_EXT>;
+ clock-names = "rtc", "ext", "pclk", "tcu";
+
+ interrupt-controller;
+ #interrupt-cells = <1>;
+
+ interrupt-parent = <&intc>;
+ interrupts = <27 26 25>;
+
+ watchdog: watchdog@0 {
+ compatible = "ingenic,jz4740-watchdog";
+ reg = <0x0 0xc>;
+
+ clocks = <&tcu TCU_CLK_WDT>;
+ clock-names = "wdt";
+ };
+
+ timer: timer@40 {
+ compatible = "ingenic,jz4740-tcu-timer";
+ reg = <0x40 0x10>;
+
+ clocks = <&tcu TCU_CLK_TIMER0>;
+ clock-names = "timer";
+
+ interrupts = <0>;
+ };
+
+ clocksource: timer@50 {
+ compatible = "ingenic,jz4740-tcu-clocksource";
+ reg = <0x50 0x10>;
+
+ clocks = <&tcu TCU_CLK_TIMER1>;
+ clock-names = "timer";
+
+ /* Disable by default, since the OST is more precise */
+ status = "disabled";
+ };
+
+
+ pwm: pwm@60 {
+ compatible = "ingenic,jz4740-pwm";
+ reg = <0x60 0x60>;
+
+ #pwm-cells = <3>;
+
+ clocks = <&tcu TCU_CLK_TIMER0
+ &tcu TCU_CLK_TIMER1
+ &tcu TCU_CLK_TIMER2
+ &tcu TCU_CLK_TIMER3
+ &tcu TCU_CLK_TIMER4
+ &tcu TCU_CLK_TIMER5
+ &tcu TCU_CLK_TIMER6
+ &tcu TCU_CLK_TIMER7>;
+ clock-names = "timer0", "timer1", "timer2", "timer3",
+ "timer4", "timer5", "timer6", "timer7";
+ };
+
+ ost: timer@e0 {
+ compatible = "ingenic,jz4770-ost";
+ reg = <0xe0 0x20>;
+
+ clocks = <&tcu TCU_CLK_OST>;
+ clock-names = "ost";
+
+ interrupts = <15>;
+ };
+ };
+
pinctrl: pin-controller@10010000 {
compatible = "ingenic,jz4770-pinctrl";
reg = <0x10010000 0x600>;
diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index b03cdec56de9..b8eb8a385651 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
#include <dt-bindings/clock/jz4780-cgu.h>
+#include <dt-bindings/clock/ingenic,tcu.h>
#include <dt-bindings/dma/jz4780-dma.h>
/ {
@@ -46,6 +47,85 @@
#clock-cells = <1>;
};
+ tcu: timer@10002000 {
+ compatible = "ingenic,jz4770-tcu";
+ reg = <0x10002000 0x1000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x10002000 0x1000>;
+
+ #clock-cells = <1>;
+
+ clocks = <&cgu JZ4780_CLK_RTCLK
+ &cgu JZ4780_CLK_EXCLK
+ &cgu JZ4780_CLK_PCLK
+ &cgu JZ4780_CLK_EXCLK>;
+ clock-names = "rtc", "ext", "pclk", "tcu";
+
+ interrupt-controller;
+ #interrupt-cells = <1>;
+
+ interrupt-parent = <&intc>;
+ interrupts = <27 26 25>;
+
+ watchdog: watchdog@0 {
+ compatible = "ingenic,jz4780-watchdog";
+ reg = <0x0 0xc>;
+
+ clocks = <&tcu TCU_CLK_WDT>;
+ clock-names = "wdt";
+ };
+
+ timer: timer@40 {
+ compatible = "ingenic,jz4740-tcu-timer";
+ reg = <0x40 0x10>;
+
+ clocks = <&tcu TCU_CLK_TIMER0>;
+ clock-names = "timer";
+
+ interrupts = <0>;
+ };
+
+ clocksource: timer@50 {
+ compatible = "ingenic,jz4740-tcu-clocksource";
+ reg = <0x50 0x10>;
+
+ clocks = <&tcu TCU_CLK_TIMER1>;
+ clock-names = "timer";
+
+ /* Disable by default, since the OST is more precise */
+ status = "disabled";
+ };
+
+ pwm: pwm@60 {
+ compatible = "ingenic,jz4740-pwm";
+ reg = <0x60 0x60>;
+
+ #pwm-cells = <3>;
+
+ clocks = <&tcu TCU_CLK_TIMER0
+ &tcu TCU_CLK_TIMER1
+ &tcu TCU_CLK_TIMER2
+ &tcu TCU_CLK_TIMER3
+ &tcu TCU_CLK_TIMER4
+ &tcu TCU_CLK_TIMER5
+ &tcu TCU_CLK_TIMER6
+ &tcu TCU_CLK_TIMER7>;
+ clock-names = "timer0", "timer1", "timer2", "timer3",
+ "timer4", "timer5", "timer6", "timer7";
+ };
+
+ ost: timer@e0 {
+ compatible = "ingenic,jz4770-ost";
+ reg = <0xe0 0x20>;
+
+ clocks = <&tcu TCU_CLK_OST>;
+ clock-names = "ost";
+
+ interrupts = <15>;
+ };
+ };
+
rtc_dev: rtc@10003000 {
compatible = "ingenic,jz4780-rtc";
reg = <0x10003000 0x4c>;
@@ -239,14 +319,6 @@
status = "disabled";
};
- watchdog: watchdog@10002000 {
- compatible = "ingenic,jz4780-watchdog";
- reg = <0x10002000 0x10>;
-
- clocks = <&cgu JZ4780_CLK_RTCLK>;
- clock-names = "rtc";
- };
-
nemc: nemc@13410000 {
compatible = "ingenic,jz4780-nemc";
reg = <0x13410000 0x10000>;
--
2.11.0
Add a documentation file about the Timer/Counter Unit (TCU) present in
the Ingenic JZ47xx SoCs.
The Timer/Counter Unit (TCU) in Ingenic JZ47xx SoCs is a multi-function
hardware block. It features up to to eight channels, that can be used as
counters, timers, or PWM.
- JZ4725B, JZ4750, JZ4755 only have six TCU channels. The other SoCs all
have eight channels.
- JZ4725B introduced a separate channel, called Operating System Timer
(OST). It is a 32-bit programmable timer. On JZ4770 and above, it is
64-bit.
- Each one of the TCU channels has its own clock, which can be reparented
to three different clocks (pclk, ext, rtc), gated, and reclocked, through
their TCSR register.
* The watchdog and OST hardware blocks also feature a TCSR register with
the same format in their register space.
* The TCU registers used to gate/ungate can also gate/ungate the watchdog
and OST clocks.
- Each TCU channel works in one of two modes:
* mode TCU1: channels cannot work in sleep mode, but are easier to
operate.
* mode TCU2: channels can work in sleep mode, but the operation is a bit
more complicated than with TCU1 channels.
- The mode of each TCU channel depends on the SoC used:
* On the oldest SoCs (up to JZ4740), all of the eight channels operate in
TCU1 mode.
* On JZ4725B, channel 5 operates as TCU2, the others operate as TCU1.
* On newest SoCs (JZ4750 and above), channels 1-2 operate as TCU2, the
others operate as TCU1.
- Each channel can generate an interrupt. Some channels share an interrupt
line, some don't, and this changes between SoC versions:
* on older SoCs (JZ4740 and below), channel 0 and channel 1 have their
own interrupt line; channels 2-7 share the last interrupt line.
* On JZ4725B, channel 0 has its own interrupt; channels 1-5 share one
interrupt line; the OST uses the last interrupt line.
* on newer SoCs (JZ4750 and above), channel 5 has its own interrupt;
channels 0-4 and (if eight channels) 6-7 all share one interrupt line;
the OST uses the last interrupt line.
Signed-off-by: Paul Cercueil <[email protected]>
---
Notes:
v4: New patch in this series
v5: Added information about number of channels, and improved
documentation about channel modes
v6: Add info about OST (can be 32-bit on older SoCs)
v7: No change
v8: No change
Documentation/mips/ingenic-tcu.txt | 60 ++++++++++++++++++++++++++++++++++++++
1 file changed, 60 insertions(+)
create mode 100644 Documentation/mips/ingenic-tcu.txt
diff --git a/Documentation/mips/ingenic-tcu.txt b/Documentation/mips/ingenic-tcu.txt
new file mode 100644
index 000000000000..0ea35b2a46da
--- /dev/null
+++ b/Documentation/mips/ingenic-tcu.txt
@@ -0,0 +1,60 @@
+Ingenic JZ47xx SoCs Timer/Counter Unit hardware
+-----------------------------------------------
+
+The Timer/Counter Unit (TCU) in Ingenic JZ47xx SoCs is a multi-function
+hardware block. It features up to to eight channels, that can be used as
+counters, timers, or PWM.
+
+- JZ4725B, JZ4750, JZ4755 only have six TCU channels. The other SoCs all
+ have eight channels.
+
+- JZ4725B introduced a separate channel, called Operating System Timer
+ (OST). It is a 32-bit programmable timer. On JZ4770 and above, it is
+ 64-bit.
+
+- Each one of the TCU channels has its own clock, which can be reparented
+ to three different clocks (pclk, ext, rtc), gated, and reclocked, through
+ their TCSR register.
+ * The watchdog and OST hardware blocks also feature a TCSR register with
+ the same format in their register space.
+ * The TCU registers used to gate/ungate can also gate/ungate the watchdog
+ and OST clocks.
+
+- Each TCU channel works in one of two modes:
+ * mode TCU1: channels cannot work in sleep mode, but are easier to
+ operate.
+ * mode TCU2: channels can work in sleep mode, but the operation is a bit
+ more complicated than with TCU1 channels.
+
+- The mode of each TCU channel depends on the SoC used:
+ * On the oldest SoCs (up to JZ4740), all of the eight channels operate in
+ TCU1 mode.
+ * On JZ4725B, channel 5 operates as TCU2, the others operate as TCU1.
+ * On newest SoCs (JZ4750 and above), channels 1-2 operate as TCU2, the
+ others operate as TCU1.
+
+- Each channel can generate an interrupt. Some channels share an interrupt
+ line, some don't, and this changes between SoC versions:
+ * on older SoCs (JZ4740 and below), channel 0 and channel 1 have their
+ own interrupt line; channels 2-7 share the last interrupt line.
+ * On JZ4725B, channel 0 has its own interrupt; channels 1-5 share one
+ interrupt line; the OST uses the last interrupt line.
+ * on newer SoCs (JZ4750 and above), channel 5 has its own interrupt;
+ channels 0-4 and (if eight channels) 6-7 all share one interrupt line;
+ the OST uses the last interrupt line.
+
+Implementation
+--------------
+
+The functionalities of the TCU hardware are spread across multiple drivers:
+- clocks/irq/timer: drivers/clocksource/ingenic-timer.c
+- PWM: drivers/pwm/pwm-jz4740.c
+- watchdog: drivers/watchdog/jz4740_wdt.c
+- OST: drivers/clocksource/ingenic-ost.c
+
+Because various functionalities of the TCU that belong to different drivers
+and frameworks can be controlled from the same registers, all of these
+drivers access their registers through the same regmap.
+
+For more information regarding the devicetree bindings of the TCU drivers,
+have a look at Documentation/devicetree/bindings/mfd/ingenic,tcu.txt.
--
2.11.0
Add myself as maintainer for the ingenic-timer and ingenic-ost drivers.
Signed-off-by: Paul Cercueil <[email protected]>
---
Notes:
v2: No change
v3: No change
v4: No change
v5: Update with new files
v6: No change
v7: No change
v8: No change
MAINTAINERS | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 8119141a926f..7a035cd5787a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7387,6 +7387,15 @@ L: [email protected]
S: Maintained
F: drivers/mtd/nand/raw/jz4780_*
+INGENIC TCU driver
+M: Paul Cercueil <[email protected]>
+S: Maintained
+F: drivers/clocksource/ingenic-ost.c
+F: drivers/clocksource/ingenic-timer.c
+F: drivers/clocksource/ingenic-timer.h
+F: include/linux/mfd/ingenic-tcu.h
+F: include/dt-bindings/clock/ingenic,tcu.h
+
INOTIFY
M: Jan Kara <[email protected]>
R: Amir Goldstein <[email protected]>
--
2.11.0
Depending on MACH_INGENIC prevent us from creating a generic kernel that
works on more than one MIPS board. Instead, we just depend on MIPS being
set.
On other architectures, this driver can still be built, thanks to
COMPILE_TEST. This is used by automated tools to find bugs, for
instance.
Signed-off-by: Paul Cercueil <[email protected]>
Acked-by: Thierry Reding <[email protected]>
---
Notes:
v5: New patch
v6: No change
v7: No change
v8: No change
drivers/pwm/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 0343f0c1238e..d6f62d9cdc18 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -201,7 +201,7 @@ config PWM_IMX
config PWM_JZ4740
tristate "Ingenic JZ47xx PWM support"
- depends on MACH_INGENIC
+ depends on MIPS || COMPILE_TEST
depends on COMMON_CLK
select INGENIC_TIMER
help
--
2.11.0
From: Maarten ter Huurne <[email protected]>
OST is the OS Timer, a 64-bit timer/counter with buffered reading.
SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 and
JZ4780 have a 64-bit OST.
This driver will register both a clocksource and a sched_clock to the
system.
Signed-off-by: Maarten ter Huurne <[email protected]>
Signed-off-by: Paul Cercueil <[email protected]>
---
Notes:
v5: New patch
v6: - Get rid of SoC IDs; pass pointer to ingenic_ost_soc_info as
devicetree match data instead.
- Use device_get_match_data() instead of the of_* variant
- Handle error of dev_get_regmap() properly
v7: Fix section mismatch by using builtin_platform_driver_probe()
v8: builtin_platform_driver_probe() does not work anymore in
4.20-rc6? The probe function won't be called. Work around this
for now by using late_initcall.
drivers/clocksource/Kconfig | 8 ++
drivers/clocksource/Makefile | 1 +
drivers/clocksource/ingenic-ost.c | 215 ++++++++++++++++++++++++++++++++++++++
3 files changed, 224 insertions(+)
create mode 100644 drivers/clocksource/ingenic-ost.c
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 4e69af15c3e7..e0646878b0de 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -648,4 +648,12 @@ config INGENIC_TIMER
help
Support for the timer/counter unit of the Ingenic JZ SoCs.
+config INGENIC_OST
+ bool "Ingenic JZ47xx Operating System Timer"
+ depends on MIPS || COMPILE_TEST
+ depends on COMMON_CLK
+ select INGENIC_TIMER
+ help
+ Support for the OS Timer of the Ingenic JZ4770 or similar SoC.
+
endmenu
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 7c8f790dcf67..7faa8108574a 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -75,6 +75,7 @@ obj-$(CONFIG_ASM9260_TIMER) += asm9260_timer.o
obj-$(CONFIG_H8300_TMR8) += h8300_timer8.o
obj-$(CONFIG_H8300_TMR16) += h8300_timer16.o
obj-$(CONFIG_H8300_TPU) += h8300_tpu.o
+obj-$(CONFIG_INGENIC_OST) += ingenic-ost.o
obj-$(CONFIG_INGENIC_TIMER) += ingenic-timer.o
obj-$(CONFIG_CLKSRC_ST_LPC) += clksrc_st_lpc.o
obj-$(CONFIG_X86_NUMACHIP) += numachip.o
diff --git a/drivers/clocksource/ingenic-ost.c b/drivers/clocksource/ingenic-ost.c
new file mode 100644
index 000000000000..cc0fee3efd29
--- /dev/null
+++ b/drivers/clocksource/ingenic-ost.c
@@ -0,0 +1,215 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * JZ47xx SoCs TCU Operating System Timer driver
+ *
+ * Copyright (C) 2016 Maarten ter Huurne <[email protected]>
+ * Copyright (C) 2018 Paul Cercueil <[email protected]>
+ */
+
+#include <linux/clk.h>
+#include <linux/clocksource.h>
+#include <linux/mfd/ingenic-tcu.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/regmap.h>
+#include <linux/sched_clock.h>
+
+#include "ingenic-timer.h"
+
+#define TCU_OST_TCSR_MASK 0xc0
+#define TCU_OST_TCSR_CNT_MD BIT(15)
+
+#define TCU_OST_CHANNEL 15
+
+struct ingenic_ost_soc_info {
+ bool is64bit;
+};
+
+struct ingenic_ost {
+ struct regmap *map;
+ struct clk *clk;
+
+ struct clocksource cs;
+};
+
+static u64 notrace ingenic_ost_read_cntl(void)
+{
+ /* Bypass the regmap here as we must return as soon as possible */
+ return readl(ingenic_tcu_base + TCU_REG_OST_CNTL);
+}
+
+static u64 notrace ingenic_ost_read_cnth(void)
+{
+ /* Bypass the regmap here as we must return as soon as possible */
+ return readl(ingenic_tcu_base + TCU_REG_OST_CNTH);
+}
+
+static u64 notrace ingenic_ost_clocksource_read(struct clocksource *cs)
+{
+ u32 val1, val2;
+ u64 count, recount;
+ s64 diff;
+
+ /*
+ * The buffering of the upper 32 bits of the timer prevents wrong
+ * results from the bottom 32 bits overflowing due to the timer ticking
+ * along. However, it does not prevent wrong results from simultaneous
+ * reads of the timer, which could reset the buffer mid-read.
+ * Since this kind of wrong read can happen only when the bottom bits
+ * overflow, there will be minutes between wrong reads, so if we read
+ * twice in succession, at least one of the reads will be correct.
+ */
+
+ /* Bypass the regmap here as we must return as soon as possible */
+ val1 = readl(ingenic_tcu_base + TCU_REG_OST_CNTL);
+ val2 = readl(ingenic_tcu_base + TCU_REG_OST_CNTHBUF);
+ count = (u64)val1 | (u64)val2 << 32;
+
+ val1 = readl(ingenic_tcu_base + TCU_REG_OST_CNTL);
+ val2 = readl(ingenic_tcu_base + TCU_REG_OST_CNTHBUF);
+ recount = (u64)val1 | (u64)val2 << 32;
+
+ /*
+ * A wrong read will produce a result that is 1<<32 too high: the bottom
+ * part from before overflow and the upper part from after overflow.
+ * Therefore, the lower value of the two reads is the correct value.
+ */
+
+ diff = (s64)(recount - count);
+ if (unlikely(diff < 0))
+ count = recount;
+
+ return count;
+}
+
+static int __init ingenic_ost_probe(struct platform_device *pdev)
+{
+ const struct ingenic_ost_soc_info *soc_info;
+ struct device *dev = &pdev->dev;
+ struct ingenic_ost *ost;
+ struct clocksource *cs;
+ unsigned long rate, flags;
+ int err;
+
+ soc_info = device_get_match_data(dev);
+ if (!soc_info)
+ return -EINVAL;
+
+ ost = devm_kzalloc(dev, sizeof(*ost), GFP_KERNEL);
+ if (!ost)
+ return -ENOMEM;
+
+ ost->map = dev_get_regmap(dev->parent, NULL);
+ if (!ost->map) {
+ dev_err(dev, "regmap not found\n");
+ return -EINVAL;
+ }
+
+ ost->clk = devm_clk_get(dev, "ost");
+ if (IS_ERR(ost->clk))
+ return PTR_ERR(ost->clk);
+
+ err = clk_prepare_enable(ost->clk);
+ if (err)
+ return err;
+
+ /* Clear counter high/low registers */
+ if (soc_info->is64bit)
+ regmap_write(ost->map, TCU_REG_OST_CNTL, 0);
+ regmap_write(ost->map, TCU_REG_OST_CNTH, 0);
+
+ /* Don't reset counter at compare value. */
+ regmap_update_bits(ost->map, TCU_REG_OST_TCSR,
+ TCU_OST_TCSR_MASK, TCU_OST_TCSR_CNT_MD);
+
+ rate = clk_get_rate(ost->clk);
+
+ /* Enable OST TCU channel */
+ regmap_write(ost->map, TCU_REG_TESR, BIT(TCU_OST_CHANNEL));
+
+ cs = &ost->cs;
+ cs->name = "ingenic-ost";
+ cs->rating = 320;
+ cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
+
+ if (soc_info->is64bit) {
+ cs->mask = CLOCKSOURCE_MASK(64);
+ cs->read = ingenic_ost_clocksource_read;
+ } else {
+ cs->mask = CLOCKSOURCE_MASK(32);
+ cs->read = (u64 (*)(struct clocksource *))ingenic_ost_read_cnth;
+ }
+
+ err = clocksource_register_hz(cs, rate);
+ if (err) {
+ dev_err(dev, "clocksource registration failed: %d\n", err);
+ clk_disable_unprepare(ost->clk);
+ return err;
+ }
+
+ /* Cannot register a sched_clock with interrupts on */
+ local_irq_save(flags);
+ if (soc_info->is64bit)
+ sched_clock_register(ingenic_ost_read_cntl, 32, rate);
+ else
+ sched_clock_register(ingenic_ost_read_cnth, 32, rate);
+ local_irq_restore(flags);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int ingenic_ost_suspend(struct device *dev)
+{
+ struct ingenic_ost *ost = dev_get_drvdata(dev);
+
+ clk_disable(ost->clk);
+ return 0;
+}
+
+static int ingenic_ost_resume(struct device *dev)
+{
+ struct ingenic_ost *ost = dev_get_drvdata(dev);
+
+ return clk_enable(ost->clk);
+}
+
+static SIMPLE_DEV_PM_OPS(ingenic_ost_pm_ops, ingenic_ost_suspend,
+ ingenic_ost_resume);
+#define INGENIC_OST_PM_OPS (&ingenic_ost_pm_ops)
+#else
+#define INGENIC_OST_PM_OPS NULL
+#endif /* CONFIG_PM_SUSPEND */
+
+static const struct ingenic_ost_soc_info jz4725b_ost_soc_info = {
+ .is64bit = false,
+};
+
+static const struct ingenic_ost_soc_info jz4770_ost_soc_info = {
+ .is64bit = true,
+};
+
+static const struct of_device_id ingenic_ost_of_match[] = {
+ { .compatible = "ingenic,jz4725b-ost", .data = &jz4725b_ost_soc_info, },
+ { .compatible = "ingenic,jz4770-ost", .data = &jz4770_ost_soc_info, },
+ { }
+};
+
+static struct platform_driver ingenic_ost_driver = {
+ .driver = {
+ .name = "ingenic-ost",
+ .pm = INGENIC_OST_PM_OPS,
+ .of_match_table = ingenic_ost_of_match,
+ },
+};
+
+/* FIXME: Using device_initcall (or buildin_platform_driver_probe) results
+ * in the driver not being probed at all. It worked in 4.18...
+ */
+static int __init ingenic_ost_drv_register(void)
+{
+ return platform_driver_probe(&ingenic_ost_driver,
+ ingenic_ost_probe);
+}
+late_initcall(ingenic_ost_drv_register);
--
2.11.0
Add the missing TCU clock to the list of clocks supplied by the CGU for
the JZ4740 SoC.
Signed-off-by: Paul Cercueil <[email protected]>
Acked-by: Stephen Boyd <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
Notes:
v5: New patch
v6: No change
v7: No change
v8: No change
drivers/clk/ingenic/jz4740-cgu.c | 6 ++++++
include/dt-bindings/clock/jz4740-cgu.h | 1 +
2 files changed, 7 insertions(+)
diff --git a/drivers/clk/ingenic/jz4740-cgu.c b/drivers/clk/ingenic/jz4740-cgu.c
index 4479c102e899..d8ac7f2e183a 100644
--- a/drivers/clk/ingenic/jz4740-cgu.c
+++ b/drivers/clk/ingenic/jz4740-cgu.c
@@ -211,6 +211,12 @@ static const struct ingenic_cgu_clk_info jz4740_cgu_clocks[] = {
.parents = { JZ4740_CLK_EXT, -1, -1, -1 },
.gate = { CGU_REG_CLKGR, 5 },
},
+
+ [JZ4740_CLK_TCU] = {
+ "tcu", CGU_CLK_GATE,
+ .parents = { JZ4740_CLK_EXT, -1, -1, -1 },
+ .gate = { CGU_REG_CLKGR, 1 },
+ },
};
static void __init jz4740_cgu_init(struct device_node *np)
diff --git a/include/dt-bindings/clock/jz4740-cgu.h b/include/dt-bindings/clock/jz4740-cgu.h
index 6ed83f926ae7..e82d77028581 100644
--- a/include/dt-bindings/clock/jz4740-cgu.h
+++ b/include/dt-bindings/clock/jz4740-cgu.h
@@ -34,5 +34,6 @@
#define JZ4740_CLK_ADC 19
#define JZ4740_CLK_I2C 20
#define JZ4740_CLK_AIC 21
+#define JZ4740_CLK_TCU 22
#endif /* __DT_BINDINGS_CLOCK_JZ4740_CGU_H__ */
--
2.11.0
Instead of requesting the "ext" clock and handling the watchdog clock
divider and gating in the watchdog driver, we now request and use the
"wdt" clock that is supplied by the ingenic-timer "TCU" driver.
The major benefit is that the watchdog's clock rate and parent can now
be specified from within devicetree, instead of hardcoded in the driver.
Also, this driver won't poke anymore into the TCU registers to
enable/disable the clock, as this is now handled by the TCU driver.
On the bad side, we break the ABI with devicetree - as we now request a
different clock. In this very specific case it is still okay, as every
Ingenic JZ47xx-based board out there compile the devicetree within the
kernel; so it's still time to push breaking changes, in order to get a
clean devicetree that won't break once it musn't.
Signed-off-by: Paul Cercueil <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
---
Notes:
v5: New patch
v6: - Split regmap change to new patch 09/24
- The code now sets the WDT clock to the smallest rate possible and
calculates the maximum timeout from that
v7: No change
v8: No change
drivers/watchdog/Kconfig | 2 +
drivers/watchdog/jz4740_wdt.c | 86 +++++++++++++++++--------------------------
2 files changed, 36 insertions(+), 52 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 2d64333f4782..cfd7368fc3c0 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1497,7 +1497,9 @@ config INDYDOG
config JZ4740_WDT
tristate "Ingenic jz4740 SoC hardware watchdog"
depends on MACH_JZ4740 || MACH_JZ4780
+ depends on COMMON_CLK
select WATCHDOG_CORE
+ select INGENIC_TIMER
help
Hardware driver for the built-in watchdog timer on Ingenic jz4740 SoCs.
diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index ec4d99a830ba..1d504ecf45e1 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -26,25 +26,9 @@
#include <linux/err.h>
#include <linux/of.h>
-#include <asm/mach-jz4740/timer.h>
-
#define JZ_REG_WDT_TIMER_DATA 0x0
#define JZ_REG_WDT_COUNTER_ENABLE 0x4
#define JZ_REG_WDT_TIMER_COUNTER 0x8
-#define JZ_REG_WDT_TIMER_CONTROL 0xC
-
-#define JZ_WDT_CLOCK_PCLK 0x1
-#define JZ_WDT_CLOCK_RTC 0x2
-#define JZ_WDT_CLOCK_EXT 0x4
-
-#define JZ_WDT_CLOCK_DIV_SHIFT 3
-
-#define JZ_WDT_CLOCK_DIV_1 (0 << JZ_WDT_CLOCK_DIV_SHIFT)
-#define JZ_WDT_CLOCK_DIV_4 (1 << JZ_WDT_CLOCK_DIV_SHIFT)
-#define JZ_WDT_CLOCK_DIV_16 (2 << JZ_WDT_CLOCK_DIV_SHIFT)
-#define JZ_WDT_CLOCK_DIV_64 (3 << JZ_WDT_CLOCK_DIV_SHIFT)
-#define JZ_WDT_CLOCK_DIV_256 (4 << JZ_WDT_CLOCK_DIV_SHIFT)
-#define JZ_WDT_CLOCK_DIV_1024 (5 << JZ_WDT_CLOCK_DIV_SHIFT)
#define DEFAULT_HEARTBEAT 5
#define MAX_HEARTBEAT 2048
@@ -65,7 +49,8 @@ MODULE_PARM_DESC(heartbeat,
struct jz4740_wdt_drvdata {
struct watchdog_device wdt;
void __iomem *base;
- struct clk *rtc_clk;
+ struct clk *clk;
+ unsigned long clk_rate;
};
static int jz4740_wdt_ping(struct watchdog_device *wdt_dev)
@@ -80,31 +65,12 @@ static int jz4740_wdt_set_timeout(struct watchdog_device *wdt_dev,
unsigned int new_timeout)
{
struct jz4740_wdt_drvdata *drvdata = watchdog_get_drvdata(wdt_dev);
- unsigned int rtc_clk_rate;
- unsigned int timeout_value;
- unsigned short clock_div = JZ_WDT_CLOCK_DIV_1;
-
- rtc_clk_rate = clk_get_rate(drvdata->rtc_clk);
-
- timeout_value = rtc_clk_rate * new_timeout;
- while (timeout_value > 0xffff) {
- if (clock_div == JZ_WDT_CLOCK_DIV_1024) {
- /* Requested timeout too high;
- * use highest possible value. */
- timeout_value = 0xffff;
- break;
- }
- timeout_value >>= 2;
- clock_div += (1 << JZ_WDT_CLOCK_DIV_SHIFT);
- }
+ u16 timeout_value = (u16)(drvdata->clk_rate * new_timeout);
writeb(0x0, drvdata->base + JZ_REG_WDT_COUNTER_ENABLE);
- writew(clock_div, drvdata->base + JZ_REG_WDT_TIMER_CONTROL);
writew((u16)timeout_value, drvdata->base + JZ_REG_WDT_TIMER_DATA);
writew(0x0, drvdata->base + JZ_REG_WDT_TIMER_COUNTER);
- writew(clock_div | JZ_WDT_CLOCK_RTC,
- drvdata->base + JZ_REG_WDT_TIMER_CONTROL);
writeb(0x1, drvdata->base + JZ_REG_WDT_COUNTER_ENABLE);
@@ -114,7 +80,13 @@ static int jz4740_wdt_set_timeout(struct watchdog_device *wdt_dev,
static int jz4740_wdt_start(struct watchdog_device *wdt_dev)
{
- jz4740_timer_enable_watchdog();
+ struct jz4740_wdt_drvdata *drvdata = watchdog_get_drvdata(wdt_dev);
+ int ret;
+
+ ret = clk_prepare_enable(drvdata->clk);
+ if (ret)
+ return ret;
+
jz4740_wdt_set_timeout(wdt_dev, wdt_dev->timeout);
return 0;
@@ -125,7 +97,7 @@ static int jz4740_wdt_stop(struct watchdog_device *wdt_dev)
struct jz4740_wdt_drvdata *drvdata = watchdog_get_drvdata(wdt_dev);
writeb(0x0, drvdata->base + JZ_REG_WDT_COUNTER_ENABLE);
- jz4740_timer_disable_watchdog();
+ clk_disable_unprepare(drvdata->clk);
return 0;
}
@@ -163,26 +135,42 @@ MODULE_DEVICE_TABLE(of, jz4740_wdt_of_matches);
static int jz4740_wdt_probe(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
struct jz4740_wdt_drvdata *drvdata;
struct watchdog_device *jz4740_wdt;
struct resource *res;
+ long rate;
int ret;
- drvdata = devm_kzalloc(&pdev->dev, sizeof(struct jz4740_wdt_drvdata),
- GFP_KERNEL);
+ drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
if (!drvdata)
return -ENOMEM;
- if (heartbeat < 1 || heartbeat > MAX_HEARTBEAT)
- heartbeat = DEFAULT_HEARTBEAT;
+ drvdata->clk = devm_clk_get(&pdev->dev, "wdt");
+ if (IS_ERR(drvdata->clk)) {
+ dev_err(&pdev->dev, "cannot find WDT clock\n");
+ return PTR_ERR(drvdata->clk);
+ }
+
+ /* Set smallest clock possible */
+ rate = clk_round_rate(drvdata->clk, 1);
+ if (rate < 0)
+ return rate;
+ ret = clk_set_rate(drvdata->clk, rate);
+ if (ret)
+ return ret;
+
+ drvdata->clk_rate = rate;
jz4740_wdt = &drvdata->wdt;
jz4740_wdt->info = &jz4740_wdt_info;
jz4740_wdt->ops = &jz4740_wdt_ops;
- jz4740_wdt->timeout = heartbeat;
jz4740_wdt->min_timeout = 1;
- jz4740_wdt->max_timeout = MAX_HEARTBEAT;
- jz4740_wdt->parent = &pdev->dev;
+ jz4740_wdt->max_timeout = 0xffff / rate;
+ jz4740_wdt->timeout = clamp(heartbeat,
+ jz4740_wdt->min_timeout,
+ jz4740_wdt->max_timeout);
+ jz4740_wdt->parent = dev;
watchdog_set_nowayout(jz4740_wdt, nowayout);
watchdog_set_drvdata(jz4740_wdt, drvdata);
@@ -191,12 +179,6 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
if (IS_ERR(drvdata->base))
return PTR_ERR(drvdata->base);
- drvdata->rtc_clk = devm_clk_get(&pdev->dev, "rtc");
- if (IS_ERR(drvdata->rtc_clk)) {
- dev_err(&pdev->dev, "cannot find RTC clock\n");
- return PTR_ERR(drvdata->rtc_clk);
- }
-
ret = devm_watchdog_register_device(&pdev->dev, &drvdata->wdt);
if (ret < 0)
return ret;
--
2.11.0
The TCU channels 0 and 1 were previously reserved for system tasks, and
thus unavailable for PWM.
The driver will now only allow a PWM channel to be requested if memory
resources corresponding to the register area of the channel were
supplied to the driver. This allows the TCU channels to be reserved for
system tasks from within the devicetree.
Signed-off-by: Paul Cercueil <[email protected]>
---
Notes:
v6: New patch
v7: No change
v8: ingenic_tcu_[request,release]_channel are dropped. We now use
the memory resources provided to the driver to detect whether
or not we are allowed to use the TCU channel.
drivers/pwm/pwm-jz4740.c | 38 +++++++++++++++++++++++++++++++++-----
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index 6b865c14f789..7b12e5628f4f 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -28,6 +28,7 @@ struct jz4740_pwm_chip {
struct pwm_chip chip;
struct clk *clks[NUM_PWM];
struct regmap *map;
+ struct resource *parent_res;
};
static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
@@ -35,6 +36,31 @@ static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
return container_of(chip, struct jz4740_pwm_chip, chip);
}
+static bool jz4740_pwm_can_use_chn(struct jz4740_pwm_chip *jz, unsigned int chn)
+{
+ struct platform_device *pdev = to_platform_device(jz->chip.dev);
+ struct resource chn_res, *res;
+ unsigned int i;
+
+ chn_res.start = jz->parent_res->start + TCU_REG_TDFRc(chn);
+ chn_res.end = chn_res.start + TCU_CHANNEL_STRIDE - 1;
+ chn_res.flags = IORESOURCE_MEM;
+
+ /*
+ * Walk the list of resources, find if there's one that contains the
+ * registers for the requested TCU channel
+ */
+ for (i = 0; ; i++) {
+ res = platform_get_resource(pdev, IORESOURCE_MEM, i);
+ if (!res)
+ break;
+ if (resource_contains(res, &chn_res))
+ return true;
+ }
+
+ return false;
+}
+
static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
{
struct jz4740_pwm_chip *jz = to_jz4740(chip);
@@ -42,11 +68,7 @@ static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
char clk_name[16];
int ret;
- /*
- * Timers 0 and 1 are used for system tasks, so they are unavailable
- * for use as PWMs.
- */
- if (pwm->hwpwm < 2)
+ if (!jz4740_pwm_can_use_chn(jz, pwm->hwpwm))
return -EBUSY;
snprintf(clk_name, sizeof(clk_name), "timer%u", pwm->hwpwm);
@@ -208,6 +230,12 @@ static int jz4740_pwm_probe(struct platform_device *pdev)
return -EINVAL;
}
+ jz4740->parent_res = platform_get_resource(
+ to_platform_device(dev->parent),
+ IORESOURCE_MEM, 0);
+ if (!jz4740->parent_res)
+ return -EINVAL;
+
jz4740->chip.dev = dev;
jz4740->chip.ops = &jz4740_pwm_ops;
jz4740->chip.npwm = NUM_PWM;
--
2.11.0
The default clock (12 MHz) is too fast for the system timer, which fails
to report time accurately.
Signed-off-by: Paul Cercueil <[email protected]>
---
Notes:
v5: New patch
v6: Remove ingenic,clocksource-channel property
v7: No change
v8: No change
arch/mips/boot/dts/ingenic/qi_lb60.dts | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/mips/boot/dts/ingenic/qi_lb60.dts b/arch/mips/boot/dts/ingenic/qi_lb60.dts
index 85529a142409..de9d45e2c24a 100644
--- a/arch/mips/boot/dts/ingenic/qi_lb60.dts
+++ b/arch/mips/boot/dts/ingenic/qi_lb60.dts
@@ -45,3 +45,9 @@
bias-disable;
};
};
+
+&tcu {
+ /* 750 kHz for the system timer and clocksource */
+ assigned-clocks = <&tcu TCU_CLK_TIMER0>, <&tcu TCU_CLK_TIMER1>;
+ assigned-clock-rates = <750000>, <750000>;
+};
--
2.11.0
Since we broke the ABI by changing the clock, the driver was also
updated to use the regmap provided by the TCU driver.
Signed-off-by: Paul Cercueil <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
---
Notes:
v6: New patch
v7: No change
v8: No change
drivers/watchdog/jz4740_wdt.c | 30 ++++++++++++++----------------
1 file changed, 14 insertions(+), 16 deletions(-)
diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index 1d504ecf45e1..0f54306aee25 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -13,6 +13,7 @@
*
*/
+#include <linux/mfd/ingenic-tcu.h>
#include <linux/module.h>
#include <linux/moduleparam.h>
#include <linux/types.h>
@@ -25,10 +26,7 @@
#include <linux/slab.h>
#include <linux/err.h>
#include <linux/of.h>
-
-#define JZ_REG_WDT_TIMER_DATA 0x0
-#define JZ_REG_WDT_COUNTER_ENABLE 0x4
-#define JZ_REG_WDT_TIMER_COUNTER 0x8
+#include <linux/regmap.h>
#define DEFAULT_HEARTBEAT 5
#define MAX_HEARTBEAT 2048
@@ -48,7 +46,7 @@ MODULE_PARM_DESC(heartbeat,
struct jz4740_wdt_drvdata {
struct watchdog_device wdt;
- void __iomem *base;
+ struct regmap *map;
struct clk *clk;
unsigned long clk_rate;
};
@@ -57,7 +55,7 @@ static int jz4740_wdt_ping(struct watchdog_device *wdt_dev)
{
struct jz4740_wdt_drvdata *drvdata = watchdog_get_drvdata(wdt_dev);
- writew(0x0, drvdata->base + JZ_REG_WDT_TIMER_COUNTER);
+ regmap_write(drvdata->map, TCU_REG_WDT_TCNT, 0);
return 0;
}
@@ -67,12 +65,12 @@ static int jz4740_wdt_set_timeout(struct watchdog_device *wdt_dev,
struct jz4740_wdt_drvdata *drvdata = watchdog_get_drvdata(wdt_dev);
u16 timeout_value = (u16)(drvdata->clk_rate * new_timeout);
- writeb(0x0, drvdata->base + JZ_REG_WDT_COUNTER_ENABLE);
+ regmap_write(drvdata->map, TCU_REG_WDT_TCER, 0);
- writew((u16)timeout_value, drvdata->base + JZ_REG_WDT_TIMER_DATA);
- writew(0x0, drvdata->base + JZ_REG_WDT_TIMER_COUNTER);
+ regmap_write(drvdata->map, TCU_REG_WDT_TDR, timeout_value);
+ regmap_write(drvdata->map, TCU_REG_WDT_TCNT, 0);
- writeb(0x1, drvdata->base + JZ_REG_WDT_COUNTER_ENABLE);
+ regmap_write(drvdata->map, TCU_REG_WDT_TCER, TCU_WDT_TCER_TCEN);
wdt_dev->timeout = new_timeout;
return 0;
@@ -96,7 +94,7 @@ static int jz4740_wdt_stop(struct watchdog_device *wdt_dev)
{
struct jz4740_wdt_drvdata *drvdata = watchdog_get_drvdata(wdt_dev);
- writeb(0x0, drvdata->base + JZ_REG_WDT_COUNTER_ENABLE);
+ regmap_write(drvdata->map, TCU_REG_WDT_TCER, 0);
clk_disable_unprepare(drvdata->clk);
return 0;
@@ -138,7 +136,6 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct jz4740_wdt_drvdata *drvdata;
struct watchdog_device *jz4740_wdt;
- struct resource *res;
long rate;
int ret;
@@ -174,10 +171,11 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
watchdog_set_nowayout(jz4740_wdt, nowayout);
watchdog_set_drvdata(jz4740_wdt, drvdata);
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- drvdata->base = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(drvdata->base))
- return PTR_ERR(drvdata->base);
+ drvdata->map = dev_get_regmap(dev->parent, NULL);
+ if (!drvdata->map) {
+ dev_err(dev, "regmap not found\n");
+ return -EINVAL;
+ }
ret = devm_watchdog_register_device(&pdev->dev, &drvdata->wdt);
if (ret < 0)
--
2.11.0
Previously the jz4740_wdt_set_timeout() function was starting the timer
unconditionally, even if it was stopped when that function was entered.
Now, the timer will be restarted only if it was already running before
this function is called.
Signed-off-by: Paul Cercueil <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
---
Notes:
v6: New patch
v7: No change
v8: No change
drivers/watchdog/jz4740_wdt.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index 0f54306aee25..45d9495170e5 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -64,13 +64,15 @@ static int jz4740_wdt_set_timeout(struct watchdog_device *wdt_dev,
{
struct jz4740_wdt_drvdata *drvdata = watchdog_get_drvdata(wdt_dev);
u16 timeout_value = (u16)(drvdata->clk_rate * new_timeout);
+ u32 tcer;
+ regmap_read(drvdata->map, TCU_REG_WDT_TCER, &tcer);
regmap_write(drvdata->map, TCU_REG_WDT_TCER, 0);
regmap_write(drvdata->map, TCU_REG_WDT_TDR, timeout_value);
regmap_write(drvdata->map, TCU_REG_WDT_TCNT, 0);
- regmap_write(drvdata->map, TCU_REG_WDT_TCER, TCU_WDT_TCER_TCEN);
+ regmap_write(drvdata->map, TCU_REG_WDT_TCER, tcer & TCU_WDT_TCER_TCEN);
wdt_dev->timeout = new_timeout;
return 0;
@@ -86,6 +88,7 @@ static int jz4740_wdt_start(struct watchdog_device *wdt_dev)
return ret;
jz4740_wdt_set_timeout(wdt_dev, wdt_dev->timeout);
+ regmap_write(drvdata->map, TCU_REG_WDT_TCER, TCU_WDT_TCER_TCEN);
return 0;
}
--
2.11.0
We cannot boot to userspace (not even initramfs) if the timer driver is
not present; so it makes sense to enable it unconditionally when
MACH_INGENIC is set.
Signed-off-by: Paul Cercueil <[email protected]>
---
Notes:
v5: New patch
v6: No change
v7: No change
v8: No change
arch/mips/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index 8272ea4c7264..8bd53a4e871b 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -390,6 +390,7 @@ config MACH_INGENIC
select BUILTIN_DTB
select USE_OF
select LIBFDT
+ select INGENIC_TIMER
config LANTIQ
bool "Lantiq based platforms"
--
2.11.0
Add documentation about how to properly use the Ingenic TCU
(Timer/Counter Unit) drivers from devicetree.
Signed-off-by: Paul Cercueil <[email protected]>
---
Notes:
v4: New patch in this series. Corresponds to V2 patches 3-4-5 with
added content.
v5: - Edited PWM/watchdog DT bindings documentation to point to the new
document.
- Moved main document to
Documentation/devicetree/bindings/timer/ingenic,tcu.txt
- Updated documentation to reflect the new devicetree bindings.
v6: - Removed PWM/watchdog documentation files as asked by upstream
- Removed doc about properties that should be implicit
- Removed doc about ingenic,timer-channel /
ingenic,clocksource-channel as they are gone
- Fix WDT clock name in the binding doc
- Fix lengths of register areas in watchdog/pwm nodes
v7: No change
v8: - Fix address of the PWM node
- Added doc about system timer and clocksource children nodes
.../devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt | 25 ---
.../devicetree/bindings/timer/ingenic,tcu.txt | 176 +++++++++++++++++++++
.../bindings/watchdog/ingenic,jz4740-wdt.txt | 17 --
3 files changed, 176 insertions(+), 42 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
create mode 100644 Documentation/devicetree/bindings/timer/ingenic,tcu.txt
delete mode 100644 Documentation/devicetree/bindings/watchdog/ingenic,jz4740-wdt.txt
diff --git a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt b/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
deleted file mode 100644
index 7d9d3f90641b..000000000000
--- a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
+++ /dev/null
@@ -1,25 +0,0 @@
-Ingenic JZ47xx PWM Controller
-=============================
-
-Required properties:
-- compatible: One of:
- * "ingenic,jz4740-pwm"
- * "ingenic,jz4770-pwm"
- * "ingenic,jz4780-pwm"
-- #pwm-cells: Should be 3. See pwm.txt in this directory for a description
- of the cells format.
-- clocks : phandle to the external clock.
-- clock-names : Should be "ext".
-
-
-Example:
-
- pwm: pwm@10002000 {
- compatible = "ingenic,jz4740-pwm";
- reg = <0x10002000 0x1000>;
-
- #pwm-cells = <3>;
-
- clocks = <&ext>;
- clock-names = "ext";
- };
diff --git a/Documentation/devicetree/bindings/timer/ingenic,tcu.txt b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
new file mode 100644
index 000000000000..8a4ce7edf50f
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
@@ -0,0 +1,176 @@
+Ingenic JZ47xx SoCs Timer/Counter Unit devicetree bindings
+==========================================================
+
+For a description of the TCU hardware and drivers, have a look at
+Documentation/mips/ingenic-tcu.txt.
+
+Required properties:
+
+- compatible: Must be one of:
+ * ingenic,jz4740-tcu
+ * ingenic,jz4725b-tcu
+ * ingenic,jz4770-tcu
+- reg: Should be the offset/length value corresponding to the TCU registers
+- clocks: List of phandle & clock specifiers for clocks external to the TCU.
+ The "pclk", "rtc", "ext" and "tcu" clocks should be provided.
+- clock-names: List of name strings for the external clocks.
+- #clock-cells: Should be <1>;
+ Clock consumers specify this argument to identify a clock. The valid values
+ may be found in <dt-bindings/clock/ingenic,tcu.h>.
+- interrupt-controller : Identifies the node as an interrupt controller
+- #interrupt-cells : Specifies the number of cells needed to encode an
+ interrupt source. The value should be 1.
+- interrupt-parent : phandle of the interrupt controller.
+- interrupts : Specifies the interrupt the controller is connected to.
+
+
+Children nodes
+==========================================================
+
+
+PWM node:
+---------
+
+Required properties:
+
+- compatible: Must be one of:
+ * ingenic,jz4740-pwm
+ * ingenic,jz4725b-pwm
+- #pwm-cells: Should be 3. See ../pwm/pwm.txt for a description of the cell
+ format.
+- clocks: List of phandle & clock specifiers for the TCU clocks.
+- clock-names: List of name strings for the TCU clocks.
+
+
+Watchdog node:
+--------------
+
+Required properties:
+
+- compatible: Must be one of:
+ * ingenic,jz4740-watchdog
+ * ingenic,jz4780-watchdog
+- clocks: phandle to the WDT clock
+- clock-names: should be "wdt"
+
+
+OST node:
+---------
+
+Required properties:
+
+- compatible: Must be one of:
+ * ingenic,jz4725b-ost
+ * ingenic,jz4770-ost
+- clocks: phandle to the OST clock
+- clock-names: should be "ost"
+- interrupts : Specifies the interrupt the OST is connected to.
+
+
+System timer node:
+------------------
+
+Required properties:
+
+- compatible: Must be "ingenic,jz4740-tcu-timer"
+- clocks: phandle to the clock of the TCU channel used
+- clock-names: Should be "timer"
+- interrupts : Specifies the interrupt line of the TCU channel used.
+
+
+System clocksource node:
+------------------------
+
+Required properties:
+
+- compatible: Must be "ingenic,jz4740-tcu-clocksource"
+- clocks: phandle to the clock of the TCU channel used
+- clock-names: Should be "timer"
+
+
+Example
+==========================================================
+
+#include <dt-bindings/clock/jz4770-cgu.h>
+#include <dt-bindings/clock/ingenic,tcu.h>
+
+/ {
+ tcu: timer@10002000 {
+ compatible = "ingenic,jz4770-tcu";
+ reg = <0x10002000 0x1000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x10002000 0x1000>;
+
+ #clock-cells = <1>;
+
+ clocks = <&cgu JZ4770_CLK_RTC
+ &cgu JZ4770_CLK_EXT
+ &cgu JZ4770_CLK_PCLK
+ &cgu JZ4770_CLK_EXT>;
+ clock-names = "rtc", "ext", "pclk", "tcu";
+
+ interrupt-controller;
+ #interrupt-cells = <1>;
+
+ interrupt-parent = <&intc>;
+ interrupts = <27 26 25>;
+
+ watchdog: watchdog@0 {
+ compatible = "ingenic,jz4740-watchdog";
+ reg = <0x0 0xc>;
+
+ clocks = <&tcu TCU_CLK_WDT>;
+ clock-names = "wdt";
+ };
+
+ timer: timer@40 {
+ compatible = "ingenic,jz4740-tcu-timer";
+ reg = <0x40 0x10>;
+
+ clocks = <&tcu TCU_CLK_TIMER0>;
+ clock-names = "timer";
+
+ interrupts = <0>;
+ };
+
+ clocksource: timer@50 {
+ compatible = "ingenic,jz4740-tcu-clocksource";
+ reg = <0x50 0x10>;
+
+ clocks = <&tcu TCU_CLK_TIMER1>;
+ clock-names = "timer";
+
+ /* Disable by default, since the OST is more precise */
+ status = "disabled";
+ };
+
+ pwm: pwm@60 {
+ compatible = "ingenic,jz4740-pwm";
+ reg = <0x60 0x60>;
+
+ #pwm-cells = <3>;
+
+ clocks = <&tcu TCU_CLK_TIMER0
+ &tcu TCU_CLK_TIMER1
+ &tcu TCU_CLK_TIMER2
+ &tcu TCU_CLK_TIMER3
+ &tcu TCU_CLK_TIMER4
+ &tcu TCU_CLK_TIMER5
+ &tcu TCU_CLK_TIMER6
+ &tcu TCU_CLK_TIMER7>;
+ clock-names = "timer0", "timer1", "timer2", "timer3",
+ "timer4", "timer5", "timer6", "timer7";
+ };
+
+ ost: timer@e0 {
+ compatible = "ingenic,jz4770-ost";
+ reg = <0xe0 0x20>;
+
+ clocks = <&tcu TCU_CLK_OST>;
+ clock-names = "ost";
+
+ interrupts = <15>;
+ };
+ };
+};
diff --git a/Documentation/devicetree/bindings/watchdog/ingenic,jz4740-wdt.txt b/Documentation/devicetree/bindings/watchdog/ingenic,jz4740-wdt.txt
deleted file mode 100644
index ce1cb72d5345..000000000000
--- a/Documentation/devicetree/bindings/watchdog/ingenic,jz4740-wdt.txt
+++ /dev/null
@@ -1,17 +0,0 @@
-Ingenic Watchdog Timer (WDT) Controller for JZ4740 & JZ4780
-
-Required properties:
-compatible: "ingenic,jz4740-watchdog" or "ingenic,jz4780-watchdog"
-reg: Register address and length for watchdog registers
-clocks: phandle to the RTC clock
-clock-names: should be "rtc"
-
-Example:
-
-watchdog: jz4740-watchdog@10002000 {
- compatible = "ingenic,jz4740-watchdog";
- reg = <0x10002000 0x10>;
-
- clocks = <&cgu JZ4740_CLK_RTC>;
- clock-names = "rtc";
-};
--
2.11.0
Right now none of the Ingenic-based boards probe this driver from
devicetree. This driver defined three compatible strings for the exact
same behaviour. Before these strings are used, we can remove two of
them.
Signed-off-by: Paul Cercueil <[email protected]>
Acked-by: Thierry Reding <[email protected]>
---
Notes:
v5: New patch
v6: No change
v7: No change
v8: No change
drivers/pwm/pwm-jz4740.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index 7b12e5628f4f..cb8d8cec353f 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -258,8 +258,6 @@ static int jz4740_pwm_remove(struct platform_device *pdev)
#ifdef CONFIG_OF
static const struct of_device_id jz4740_pwm_dt_ids[] = {
{ .compatible = "ingenic,jz4740-pwm", },
- { .compatible = "ingenic,jz4770-pwm", },
- { .compatible = "ingenic,jz4780-pwm", },
{},
};
MODULE_DEVICE_TABLE(of, jz4740_pwm_dt_ids);
--
2.11.0
The ingenic-timer "TCU" driver provides us with a regmap, that we can
use to safely access the TCU registers.
It also provides us with clocks, that can be (un)gated, reparented or
reclocked from devicetree, instead of having these settings hardcoded in
this driver.
While this driver is devicetree-compatible, it is never (as of now)
probed from devicetree, so this change does not introduce a ABI problem
with current devicetree files.
Signed-off-by: Paul Cercueil <[email protected]>
---
Notes:
v5: New patch
v6: Drop requirement of probing from devicetree
v7: No change
v8: Drop useless <linux/clk-provider.h> include
drivers/pwm/Kconfig | 2 +
drivers/pwm/pwm-jz4740.c | 123 ++++++++++++++++++++++++++++++-----------------
2 files changed, 82 insertions(+), 43 deletions(-)
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 27e5dd47a01f..0343f0c1238e 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -202,6 +202,8 @@ config PWM_IMX
config PWM_JZ4740
tristate "Ingenic JZ47xx PWM support"
depends on MACH_INGENIC
+ depends on COMMON_CLK
+ select INGENIC_TIMER
help
Generic PWM framework driver for Ingenic JZ47xx based
machines.
diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index a7b134af5e04..6b865c14f789 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -15,20 +15,19 @@
#include <linux/clk.h>
#include <linux/err.h>
-#include <linux/gpio.h>
#include <linux/kernel.h>
+#include <linux/mfd/ingenic-tcu.h>
#include <linux/module.h>
-#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/pwm.h>
-
-#include <asm/mach-jz4740/timer.h>
+#include <linux/regmap.h>
#define NUM_PWM 8
struct jz4740_pwm_chip {
struct pwm_chip chip;
- struct clk *clk;
+ struct clk *clks[NUM_PWM];
+ struct regmap *map;
};
static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
@@ -38,6 +37,11 @@ static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
{
+ struct jz4740_pwm_chip *jz = to_jz4740(chip);
+ struct clk *clk;
+ char clk_name[16];
+ int ret;
+
/*
* Timers 0 and 1 are used for system tasks, so they are unavailable
* for use as PWMs.
@@ -45,65 +49,89 @@ static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
if (pwm->hwpwm < 2)
return -EBUSY;
- jz4740_timer_start(pwm->hwpwm);
+ snprintf(clk_name, sizeof(clk_name), "timer%u", pwm->hwpwm);
+
+ clk = clk_get(chip->dev, clk_name);
+ if (IS_ERR(clk))
+ return PTR_ERR(clk);
+ ret = clk_prepare_enable(clk);
+ if (ret) {
+ clk_put(clk);
+ return ret;
+ }
+
+ jz->clks[pwm->hwpwm] = clk;
return 0;
}
static void jz4740_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
{
- jz4740_timer_set_ctrl(pwm->hwpwm, 0);
+ struct jz4740_pwm_chip *jz = to_jz4740(chip);
+ struct clk *clk = jz->clks[pwm->hwpwm];
- jz4740_timer_stop(pwm->hwpwm);
+ clk_disable_unprepare(clk);
+ clk_put(clk);
}
static int jz4740_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
{
- uint32_t ctrl = jz4740_timer_get_ctrl(pwm->pwm);
+ struct jz4740_pwm_chip *jz = to_jz4740(chip);
- ctrl |= JZ_TIMER_CTRL_PWM_ENABLE;
- jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
- jz4740_timer_enable(pwm->hwpwm);
+ /* Enable PWM output */
+ regmap_update_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm),
+ TCU_TCSR_PWM_EN, TCU_TCSR_PWM_EN);
+ /* Start counter */
+ regmap_write(jz->map, TCU_REG_TESR, BIT(pwm->hwpwm));
return 0;
}
static void jz4740_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
{
- uint32_t ctrl = jz4740_timer_get_ctrl(pwm->hwpwm);
+ struct jz4740_pwm_chip *jz = to_jz4740(chip);
/* Disable PWM output.
* In TCU2 mode (channel 1/2 on JZ4750+), this must be done before the
* counter is stopped, while in TCU1 mode the order does not matter.
*/
- ctrl &= ~JZ_TIMER_CTRL_PWM_ENABLE;
- jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
+ regmap_update_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm),
+ TCU_TCSR_PWM_EN, 0);
/* Stop counter */
- jz4740_timer_disable(pwm->hwpwm);
+ regmap_write(jz->map, TCU_REG_TECR, BIT(pwm->hwpwm));
}
static int jz4740_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
int duty_ns, int period_ns)
{
struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
+ struct clk *clk = jz4740->clks[pwm->hwpwm];
+ unsigned long rate, new_rate, period, duty;
unsigned long long tmp;
- unsigned long period, duty;
- unsigned int prescaler = 0;
- uint16_t ctrl;
+ unsigned int tcsr;
bool is_enabled;
- tmp = (unsigned long long)clk_get_rate(jz4740->clk) * period_ns;
- do_div(tmp, 1000000000);
- period = tmp;
+ rate = clk_get_rate(clk);
+
+ for (;;) {
+ tmp = (unsigned long long) rate * period_ns;
+ do_div(tmp, 1000000000);
- while (period > 0xffff && prescaler < 6) {
- period >>= 2;
- ++prescaler;
+ if (tmp <= 0xffff)
+ break;
+
+ new_rate = clk_round_rate(clk, rate / 2);
+
+ if (new_rate < rate)
+ rate = new_rate;
+ else
+ return -EINVAL;
}
- if (prescaler == 6)
- return -EINVAL;
+ clk_set_rate(clk, rate);
+
+ period = tmp;
tmp = (unsigned long long)period * duty_ns;
do_div(tmp, period_ns);
@@ -112,18 +140,23 @@ static int jz4740_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
if (duty >= period)
duty = period - 1;
- is_enabled = jz4740_timer_is_enabled(pwm->hwpwm);
+ regmap_read(jz4740->map, TCU_REG_TER, &tcsr);
+ is_enabled = tcsr & BIT(pwm->hwpwm);
if (is_enabled)
jz4740_pwm_disable(chip, pwm);
- jz4740_timer_set_count(pwm->hwpwm, 0);
- jz4740_timer_set_duty(pwm->hwpwm, duty);
- jz4740_timer_set_period(pwm->hwpwm, period);
+ /* Set abrupt shutdown */
+ regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
+ TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD);
+
+ /* Reset counter to 0 */
+ regmap_write(jz4740->map, TCU_REG_TCNTc(pwm->hwpwm), 0);
- ctrl = JZ_TIMER_CTRL_PRESCALER(prescaler) | JZ_TIMER_CTRL_SRC_EXT |
- JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
+ /* Set duty */
+ regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), duty);
- jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
+ /* Set period */
+ regmap_write(jz4740->map, TCU_REG_TDFRc(pwm->hwpwm), period);
if (is_enabled)
jz4740_pwm_enable(chip, pwm);
@@ -134,18 +167,19 @@ static int jz4740_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
static int jz4740_pwm_set_polarity(struct pwm_chip *chip,
struct pwm_device *pwm, enum pwm_polarity polarity)
{
- uint32_t ctrl = jz4740_timer_get_ctrl(pwm->pwm);
+ struct jz4740_pwm_chip *jz = to_jz4740(chip);
switch (polarity) {
case PWM_POLARITY_NORMAL:
- ctrl &= ~JZ_TIMER_CTRL_PWM_ACTIVE_LOW;
+ regmap_update_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm),
+ TCU_TCSR_PWM_INITL_HIGH, 0);
break;
case PWM_POLARITY_INVERSED:
- ctrl |= JZ_TIMER_CTRL_PWM_ACTIVE_LOW;
+ regmap_update_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm),
+ TCU_TCSR_PWM_INITL_HIGH, TCU_TCSR_PWM_INITL_HIGH);
break;
}
- jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
return 0;
}
@@ -162,16 +196,19 @@ static const struct pwm_ops jz4740_pwm_ops = {
static int jz4740_pwm_probe(struct platform_device *pdev)
{
struct jz4740_pwm_chip *jz4740;
+ struct device *dev = &pdev->dev;
- jz4740 = devm_kzalloc(&pdev->dev, sizeof(*jz4740), GFP_KERNEL);
+ jz4740 = devm_kzalloc(dev, sizeof(*jz4740), GFP_KERNEL);
if (!jz4740)
return -ENOMEM;
- jz4740->clk = devm_clk_get(&pdev->dev, "ext");
- if (IS_ERR(jz4740->clk))
- return PTR_ERR(jz4740->clk);
+ jz4740->map = dev_get_regmap(dev->parent, NULL);
+ if (!jz4740->map) {
+ dev_err(dev, "regmap not found\n");
+ return -EINVAL;
+ }
- jz4740->chip.dev = &pdev->dev;
+ jz4740->chip.dev = dev;
jz4740->chip.ops = &jz4740_pwm_ops;
jz4740->chip.npwm = NUM_PWM;
jz4740->chip.base = -1;
--
2.11.0
This header provides clock numbers for the ingenic,tcu
DT binding.
Signed-off-by: Paul Cercueil <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
Notes:
v2: Use SPDX identifier for the license
v3: No change
v4: No change
v5: s/JZ47*_/TCU_/ and dropped *_CLK_LAST defines
v6: No change
v7: No change
v8: No change
include/dt-bindings/clock/ingenic,tcu.h | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
create mode 100644 include/dt-bindings/clock/ingenic,tcu.h
diff --git a/include/dt-bindings/clock/ingenic,tcu.h b/include/dt-bindings/clock/ingenic,tcu.h
new file mode 100644
index 000000000000..d569650a7945
--- /dev/null
+++ b/include/dt-bindings/clock/ingenic,tcu.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * This header provides clock numbers for the ingenic,tcu DT binding.
+ */
+
+#ifndef __DT_BINDINGS_CLOCK_INGENIC_TCU_H__
+#define __DT_BINDINGS_CLOCK_INGENIC_TCU_H__
+
+#define TCU_CLK_TIMER0 0
+#define TCU_CLK_TIMER1 1
+#define TCU_CLK_TIMER2 2
+#define TCU_CLK_TIMER3 3
+#define TCU_CLK_TIMER4 4
+#define TCU_CLK_TIMER5 5
+#define TCU_CLK_TIMER6 6
+#define TCU_CLK_TIMER7 7
+#define TCU_CLK_WDT 8
+#define TCU_CLK_OST 9
+
+#endif /* __DT_BINDINGS_CLOCK_INGENIC_TCU_H__ */
--
2.11.0
Probe the few drivers using PWMs from devicetree, now that we have a
devicetree node for the PWM driver.
Signed-off-by: Paul Cercueil <[email protected]>
---
Notes:
v5: New patch
v6: No change
v7: No change
v8: No change
arch/mips/boot/dts/ingenic/qi_lb60.dts | 14 ++++++++++++++
arch/mips/jz4740/board-qi_lb60.c | 19 -------------------
2 files changed, 14 insertions(+), 19 deletions(-)
diff --git a/arch/mips/boot/dts/ingenic/qi_lb60.dts b/arch/mips/boot/dts/ingenic/qi_lb60.dts
index 76aaf8982554..85529a142409 100644
--- a/arch/mips/boot/dts/ingenic/qi_lb60.dts
+++ b/arch/mips/boot/dts/ingenic/qi_lb60.dts
@@ -9,6 +9,14 @@
chosen {
stdout-path = &uart0;
};
+
+ beeper {
+ compatible = "pwm-beeper";
+ pwms = <&pwm 4 0 0>;
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&pins_pwm4>;
+ };
};
&ext {
@@ -30,4 +38,10 @@
groups = "uart0-data";
bias-disable;
};
+
+ pins_pwm4: pwm4 {
+ function = "pwm4";
+ groups = "pwm4";
+ bias-disable;
+ };
};
diff --git a/arch/mips/jz4740/board-qi_lb60.c b/arch/mips/jz4740/board-qi_lb60.c
index af0c8ace0141..cc556be656d6 100644
--- a/arch/mips/jz4740/board-qi_lb60.c
+++ b/arch/mips/jz4740/board-qi_lb60.c
@@ -27,7 +27,6 @@
#include <linux/power_supply.h>
#include <linux/power/jz4740-battery.h>
#include <linux/power/gpio-charger.h>
-#include <linux/pwm.h>
#include <linux/platform_data/jz4740/jz4740_nand.h>
@@ -392,17 +391,6 @@ static struct jz4740_mmc_platform_data qi_lb60_mmc_pdata = {
.power_active_low = 1,
};
-/* beeper */
-static struct pwm_lookup qi_lb60_pwm_lookup[] = {
- PWM_LOOKUP("jz4740-pwm", 4, "pwm-beeper", NULL, 0,
- PWM_POLARITY_NORMAL),
-};
-
-static struct platform_device qi_lb60_pwm_beeper = {
- .name = "pwm-beeper",
- .id = -1,
-};
-
/* charger */
static char *qi_lb60_batteries[] = {
"battery",
@@ -451,10 +439,8 @@ static struct platform_device *jz_platform_devices[] __initdata = {
&jz4740_i2s_device,
&jz4740_codec_device,
&jz4740_adc_device,
- &jz4740_pwm_device,
&jz4740_dma_device,
&qi_lb60_gpio_keys,
- &qi_lb60_pwm_beeper,
&qi_lb60_charger_device,
&qi_lb60_audio_device,
};
@@ -483,10 +469,6 @@ static struct pinctrl_map pin_map[] __initdata = {
"10010000.jz4740-pinctrl", "PD0", pin_cfg_bias_disable),
PIN_MAP_CONFIGS_PIN_DEFAULT("jz4740-mmc.0",
"10010000.jz4740-pinctrl", "PD2", pin_cfg_bias_disable),
-
- /* PWM pin configuration */
- PIN_MAP_MUX_GROUP_DEFAULT("jz4740-pwm",
- "10010000.jz4740-pinctrl", "pwm4", "pwm4"),
};
@@ -504,7 +486,6 @@ static int __init qi_lb60_init_platform_devices(void)
spi_register_board_info(qi_lb60_spi_board_info,
ARRAY_SIZE(qi_lb60_spi_board_info));
- pwm_add_table(qi_lb60_pwm_lookup, ARRAY_SIZE(qi_lb60_pwm_lookup));
pinctrl_register_mappings(pin_map, ARRAY_SIZE(pin_map));
return platform_add_devices(jz_platform_devices,
--
2.11.0
The default clock (48 MHz) is too fast for the system timer, which fails
to report time accurately.
Signed-off-by: Paul Cercueil <[email protected]>
---
Notes:
v5: New patch
v6: Set also the rate for the clocksource channel's clock
v7: No change
v8: No change
arch/mips/boot/dts/ingenic/ci20.dts | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts
index 50cff3cbcc6d..f64d32443097 100644
--- a/arch/mips/boot/dts/ingenic/ci20.dts
+++ b/arch/mips/boot/dts/ingenic/ci20.dts
@@ -238,3 +238,9 @@
bias-disable;
};
};
+
+&tcu {
+ /* 3 MHz for the system timer and clocksource */
+ assigned-clocks = <&tcu TCU_CLK_TIMER0>, <&tcu TCU_CLK_TIMER1>;
+ assigned-clock-rates = <3000000>, <3000000>;
+};
--
2.11.0
The OST driver provides a clocksource and sched_clock that are much more
accurate than the default ones.
Signed-off-by: Paul Cercueil <[email protected]>
---
Notes:
v5: New patch
v6: No change
v7: No change
v8: No change
arch/mips/configs/ci20_defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/mips/configs/ci20_defconfig b/arch/mips/configs/ci20_defconfig
index 030ff9c205fb..9b09b9a7f943 100644
--- a/arch/mips/configs/ci20_defconfig
+++ b/arch/mips/configs/ci20_defconfig
@@ -111,6 +111,7 @@ CONFIG_RTC_CLASS=y
CONFIG_RTC_DRV_JZ4740=y
CONFIG_DMADEVICES=y
CONFIG_DMA_JZ4780=y
+CONFIG_INGENIC_OST=y
# CONFIG_IOMMU_SUPPORT is not set
CONFIG_MEMORY=y
CONFIG_EXT4_FS=y
--
2.11.0
The TCU channel 1, which is the default for the clocksource, is used as
PWM on the GCW Zero as it drives the backlight. Therefore we must use a
different TCU channel for the clocksource.
Signed-off-by: Paul Cercueil <[email protected]>
---
Notes:
v8: New patch
arch/mips/boot/dts/ingenic/gcw0.dts | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/arch/mips/boot/dts/ingenic/gcw0.dts b/arch/mips/boot/dts/ingenic/gcw0.dts
index 35f0291e8d38..8abab14eb852 100644
--- a/arch/mips/boot/dts/ingenic/gcw0.dts
+++ b/arch/mips/boot/dts/ingenic/gcw0.dts
@@ -60,3 +60,14 @@
/* The WiFi module is connected to the UHC. */
status = "okay";
};
+
+&pwm {
+ /* Channels 1 and 3-7 are for PWM use */
+ reg = <0x50 0x10>, <0x70 0x50>;
+};
+
+&clocksource {
+ /* Use channel 2 for the clocksource */
+ reg = <0x60 0x10>;
+ clocks = <&tcu TCU_CLK_TIMER2>;
+};
--
2.11.0
The default clock (12 MHz) is too fast for the system timer, which fails
to report time accurately.
Signed-off-by: Paul Cercueil <[email protected]>
---
Notes:
v8: New patch
arch/mips/boot/dts/ingenic/gcw0.dts | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/mips/boot/dts/ingenic/gcw0.dts b/arch/mips/boot/dts/ingenic/gcw0.dts
index 8abab14eb852..651c3f505fa5 100644
--- a/arch/mips/boot/dts/ingenic/gcw0.dts
+++ b/arch/mips/boot/dts/ingenic/gcw0.dts
@@ -61,6 +61,12 @@
status = "okay";
};
+&tcu {
+ /* 750 kHz for the system timer and clocksource */
+ assigned-clocks = <&tcu TCU_CLK_TIMER0>, <&tcu TCU_CLK_TIMER2>;
+ assigned-clock-rates = <750000>, <750000>;
+};
+
&pwm {
/* Channels 1 and 3-7 are for PWM use */
reg = <0x50 0x10>, <0x70 0x50>;
--
2.11.0
The old clocksource/timer platform code is now obsoleted by the newly
introduced ingenic-timer and ingenic-ost drivers.
Signed-off-by: Paul Cercueil <[email protected]>
---
Notes:
v5: New patch
v6: No change
v7: No change
v8: No change
arch/mips/include/asm/mach-jz4740/platform.h | 1 -
arch/mips/include/asm/mach-jz4740/timer.h | 135 --------------------
arch/mips/jz4740/Makefile | 3 +-
arch/mips/jz4740/platform.c | 6 -
arch/mips/jz4740/setup.c | 8 ++
arch/mips/jz4740/time.c | 176 ---------------------------
arch/mips/jz4740/timer.c | 51 --------
7 files changed, 9 insertions(+), 371 deletions(-)
delete mode 100644 arch/mips/include/asm/mach-jz4740/timer.h
delete mode 100644 arch/mips/jz4740/time.c
delete mode 100644 arch/mips/jz4740/timer.c
diff --git a/arch/mips/include/asm/mach-jz4740/platform.h b/arch/mips/include/asm/mach-jz4740/platform.h
index c0c932ac72a7..cd464d956882 100644
--- a/arch/mips/include/asm/mach-jz4740/platform.h
+++ b/arch/mips/include/asm/mach-jz4740/platform.h
@@ -29,7 +29,6 @@ extern struct platform_device jz4740_i2s_device;
extern struct platform_device jz4740_pcm_device;
extern struct platform_device jz4740_codec_device;
extern struct platform_device jz4740_adc_device;
-extern struct platform_device jz4740_pwm_device;
extern struct platform_device jz4740_dma_device;
#endif
diff --git a/arch/mips/include/asm/mach-jz4740/timer.h b/arch/mips/include/asm/mach-jz4740/timer.h
deleted file mode 100644
index 8750a1d04e22..000000000000
--- a/arch/mips/include/asm/mach-jz4740/timer.h
+++ /dev/null
@@ -1,135 +0,0 @@
-/*
- * Copyright (C) 2010, Lars-Peter Clausen <[email protected]>
- * JZ4740 platform timer support
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License as published by the
- * Free Software Foundation; either version 2 of the License, or (at your
- * option) any later version.
- *
- * You should have received a copy of the GNU General Public License along
- * with this program; if not, write to the Free Software Foundation, Inc.,
- * 675 Mass Ave, Cambridge, MA 02139, USA.
- *
- */
-
-#ifndef __ASM_MACH_JZ4740_TIMER
-#define __ASM_MACH_JZ4740_TIMER
-
-#define JZ_REG_TIMER_STOP 0x0C
-#define JZ_REG_TIMER_STOP_SET 0x1C
-#define JZ_REG_TIMER_STOP_CLEAR 0x2C
-#define JZ_REG_TIMER_ENABLE 0x00
-#define JZ_REG_TIMER_ENABLE_SET 0x04
-#define JZ_REG_TIMER_ENABLE_CLEAR 0x08
-#define JZ_REG_TIMER_FLAG 0x10
-#define JZ_REG_TIMER_FLAG_SET 0x14
-#define JZ_REG_TIMER_FLAG_CLEAR 0x18
-#define JZ_REG_TIMER_MASK 0x20
-#define JZ_REG_TIMER_MASK_SET 0x24
-#define JZ_REG_TIMER_MASK_CLEAR 0x28
-
-#define JZ_REG_TIMER_DFR(x) (((x) * 0x10) + 0x30)
-#define JZ_REG_TIMER_DHR(x) (((x) * 0x10) + 0x34)
-#define JZ_REG_TIMER_CNT(x) (((x) * 0x10) + 0x38)
-#define JZ_REG_TIMER_CTRL(x) (((x) * 0x10) + 0x3C)
-
-#define JZ_TIMER_IRQ_HALF(x) BIT((x) + 0x10)
-#define JZ_TIMER_IRQ_FULL(x) BIT(x)
-
-#define JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN BIT(9)
-#define JZ_TIMER_CTRL_PWM_ACTIVE_LOW BIT(8)
-#define JZ_TIMER_CTRL_PWM_ENABLE BIT(7)
-#define JZ_TIMER_CTRL_PRESCALE_MASK 0x1c
-#define JZ_TIMER_CTRL_PRESCALE_OFFSET 0x3
-#define JZ_TIMER_CTRL_PRESCALE_1 (0 << 3)
-#define JZ_TIMER_CTRL_PRESCALE_4 (1 << 3)
-#define JZ_TIMER_CTRL_PRESCALE_16 (2 << 3)
-#define JZ_TIMER_CTRL_PRESCALE_64 (3 << 3)
-#define JZ_TIMER_CTRL_PRESCALE_256 (4 << 3)
-#define JZ_TIMER_CTRL_PRESCALE_1024 (5 << 3)
-
-#define JZ_TIMER_CTRL_PRESCALER(x) ((x) << JZ_TIMER_CTRL_PRESCALE_OFFSET)
-
-#define JZ_TIMER_CTRL_SRC_EXT BIT(2)
-#define JZ_TIMER_CTRL_SRC_RTC BIT(1)
-#define JZ_TIMER_CTRL_SRC_PCLK BIT(0)
-
-extern void __iomem *jz4740_timer_base;
-void __init jz4740_timer_init(void);
-
-void jz4740_timer_enable_watchdog(void);
-void jz4740_timer_disable_watchdog(void);
-
-static inline void jz4740_timer_stop(unsigned int timer)
-{
- writel(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_STOP_SET);
-}
-
-static inline void jz4740_timer_start(unsigned int timer)
-{
- writel(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_STOP_CLEAR);
-}
-
-static inline bool jz4740_timer_is_enabled(unsigned int timer)
-{
- return readb(jz4740_timer_base + JZ_REG_TIMER_ENABLE) & BIT(timer);
-}
-
-static inline void jz4740_timer_enable(unsigned int timer)
-{
- writeb(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_ENABLE_SET);
-}
-
-static inline void jz4740_timer_disable(unsigned int timer)
-{
- writeb(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_ENABLE_CLEAR);
-}
-
-static inline void jz4740_timer_set_period(unsigned int timer, uint16_t period)
-{
- writew(period, jz4740_timer_base + JZ_REG_TIMER_DFR(timer));
-}
-
-static inline void jz4740_timer_set_duty(unsigned int timer, uint16_t duty)
-{
- writew(duty, jz4740_timer_base + JZ_REG_TIMER_DHR(timer));
-}
-
-static inline void jz4740_timer_set_count(unsigned int timer, uint16_t count)
-{
- writew(count, jz4740_timer_base + JZ_REG_TIMER_CNT(timer));
-}
-
-static inline uint16_t jz4740_timer_get_count(unsigned int timer)
-{
- return readw(jz4740_timer_base + JZ_REG_TIMER_CNT(timer));
-}
-
-static inline void jz4740_timer_ack_full(unsigned int timer)
-{
- writel(JZ_TIMER_IRQ_FULL(timer), jz4740_timer_base + JZ_REG_TIMER_FLAG_CLEAR);
-}
-
-static inline void jz4740_timer_irq_full_enable(unsigned int timer)
-{
- writel(JZ_TIMER_IRQ_FULL(timer), jz4740_timer_base + JZ_REG_TIMER_FLAG_CLEAR);
- writel(JZ_TIMER_IRQ_FULL(timer), jz4740_timer_base + JZ_REG_TIMER_MASK_CLEAR);
-}
-
-static inline void jz4740_timer_irq_full_disable(unsigned int timer)
-{
- writel(JZ_TIMER_IRQ_FULL(timer), jz4740_timer_base + JZ_REG_TIMER_MASK_SET);
-}
-
-static inline void jz4740_timer_set_ctrl(unsigned int timer, uint16_t ctrl)
-{
- writew(ctrl, jz4740_timer_base + JZ_REG_TIMER_CTRL(timer));
-}
-
-static inline uint16_t jz4740_timer_get_ctrl(unsigned int timer)
-{
- return readw(jz4740_timer_base + JZ_REG_TIMER_CTRL(timer));
-}
-
-#endif
diff --git a/arch/mips/jz4740/Makefile b/arch/mips/jz4740/Makefile
index 88d6aa7d000b..72eb805028a4 100644
--- a/arch/mips/jz4740/Makefile
+++ b/arch/mips/jz4740/Makefile
@@ -5,8 +5,7 @@
# Object file lists.
-obj-y += prom.o time.o reset.o setup.o \
- platform.o timer.o
+obj-y += prom.o reset.o setup.o platform.o
CFLAGS_setup.o = -I$(src)/../../../scripts/dtc/libfdt
diff --git a/arch/mips/jz4740/platform.c b/arch/mips/jz4740/platform.c
index cbc5f8e87230..af0ecaeb4931 100644
--- a/arch/mips/jz4740/platform.c
+++ b/arch/mips/jz4740/platform.c
@@ -233,12 +233,6 @@ struct platform_device jz4740_adc_device = {
.resource = jz4740_adc_resources,
};
-/* PWM */
-struct platform_device jz4740_pwm_device = {
- .name = "jz4740-pwm",
- .id = -1,
-};
-
/* DMA */
static struct resource jz4740_dma_resources[] = {
{
diff --git a/arch/mips/jz4740/setup.c b/arch/mips/jz4740/setup.c
index afb40f8bce96..099e4164afff 100644
--- a/arch/mips/jz4740/setup.c
+++ b/arch/mips/jz4740/setup.c
@@ -14,6 +14,8 @@
*
*/
+#include <linux/clk-provider.h>
+#include <linux/clocksource.h>
#include <linux/init.h>
#include <linux/io.h>
#include <linux/irqchip.h>
@@ -101,3 +103,9 @@ void __init arch_init_irq(void)
{
irqchip_init();
}
+
+void __init plat_time_init(void)
+{
+ of_clk_init(NULL);
+ timer_probe();
+}
diff --git a/arch/mips/jz4740/time.c b/arch/mips/jz4740/time.c
deleted file mode 100644
index 2ca9160f642a..000000000000
--- a/arch/mips/jz4740/time.c
+++ /dev/null
@@ -1,176 +0,0 @@
-/*
- * Copyright (C) 2010, Lars-Peter Clausen <[email protected]>
- * JZ4740 platform time support
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License as published by the
- * Free Software Foundation; either version 2 of the License, or (at your
- * option) any later version.
- *
- * You should have received a copy of the GNU General Public License along
- * with this program; if not, write to the Free Software Foundation, Inc.,
- * 675 Mass Ave, Cambridge, MA 02139, USA.
- *
- */
-
-#include <linux/clk.h>
-#include <linux/clk-provider.h>
-#include <linux/interrupt.h>
-#include <linux/kernel.h>
-#include <linux/time.h>
-
-#include <linux/clockchips.h>
-#include <linux/sched_clock.h>
-
-#include <asm/mach-jz4740/clock.h>
-#include <asm/mach-jz4740/irq.h>
-#include <asm/mach-jz4740/timer.h>
-#include <asm/time.h>
-
-#include "clock.h"
-
-#define TIMER_CLOCKEVENT 0
-#define TIMER_CLOCKSOURCE 1
-
-static uint16_t jz4740_jiffies_per_tick;
-
-static u64 jz4740_clocksource_read(struct clocksource *cs)
-{
- return jz4740_timer_get_count(TIMER_CLOCKSOURCE);
-}
-
-static struct clocksource jz4740_clocksource = {
- .name = "jz4740-timer",
- .rating = 200,
- .read = jz4740_clocksource_read,
- .mask = CLOCKSOURCE_MASK(16),
- .flags = CLOCK_SOURCE_IS_CONTINUOUS,
-};
-
-static u64 notrace jz4740_read_sched_clock(void)
-{
- return jz4740_timer_get_count(TIMER_CLOCKSOURCE);
-}
-
-static irqreturn_t jz4740_clockevent_irq(int irq, void *devid)
-{
- struct clock_event_device *cd = devid;
-
- jz4740_timer_ack_full(TIMER_CLOCKEVENT);
-
- if (!clockevent_state_periodic(cd))
- jz4740_timer_disable(TIMER_CLOCKEVENT);
-
- cd->event_handler(cd);
-
- return IRQ_HANDLED;
-}
-
-static int jz4740_clockevent_set_periodic(struct clock_event_device *evt)
-{
- jz4740_timer_set_count(TIMER_CLOCKEVENT, 0);
- jz4740_timer_set_period(TIMER_CLOCKEVENT, jz4740_jiffies_per_tick);
- jz4740_timer_irq_full_enable(TIMER_CLOCKEVENT);
- jz4740_timer_enable(TIMER_CLOCKEVENT);
-
- return 0;
-}
-
-static int jz4740_clockevent_resume(struct clock_event_device *evt)
-{
- jz4740_timer_irq_full_enable(TIMER_CLOCKEVENT);
- jz4740_timer_enable(TIMER_CLOCKEVENT);
-
- return 0;
-}
-
-static int jz4740_clockevent_shutdown(struct clock_event_device *evt)
-{
- jz4740_timer_disable(TIMER_CLOCKEVENT);
-
- return 0;
-}
-
-static int jz4740_clockevent_set_next(unsigned long evt,
- struct clock_event_device *cd)
-{
- jz4740_timer_set_count(TIMER_CLOCKEVENT, 0);
- jz4740_timer_set_period(TIMER_CLOCKEVENT, evt);
- jz4740_timer_enable(TIMER_CLOCKEVENT);
-
- return 0;
-}
-
-static struct clock_event_device jz4740_clockevent = {
- .name = "jz4740-timer",
- .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
- .set_next_event = jz4740_clockevent_set_next,
- .set_state_shutdown = jz4740_clockevent_shutdown,
- .set_state_periodic = jz4740_clockevent_set_periodic,
- .set_state_oneshot = jz4740_clockevent_shutdown,
- .tick_resume = jz4740_clockevent_resume,
- .rating = 200,
-#ifdef CONFIG_MACH_JZ4740
- .irq = JZ4740_IRQ_TCU0,
-#endif
-#if defined(CONFIG_MACH_JZ4770) || defined(CONFIG_MACH_JZ4780)
- .irq = JZ4780_IRQ_TCU2,
-#endif
-};
-
-static struct irqaction timer_irqaction = {
- .handler = jz4740_clockevent_irq,
- .flags = IRQF_PERCPU | IRQF_TIMER,
- .name = "jz4740-timerirq",
- .dev_id = &jz4740_clockevent,
-};
-
-void __init plat_time_init(void)
-{
- int ret;
- uint32_t clk_rate;
- uint16_t ctrl;
- struct clk *ext_clk;
-
- of_clk_init(NULL);
- jz4740_timer_init();
-
- ext_clk = clk_get(NULL, "ext");
- if (IS_ERR(ext_clk))
- panic("unable to get ext clock");
- clk_rate = clk_get_rate(ext_clk) >> 4;
- clk_put(ext_clk);
-
- jz4740_jiffies_per_tick = DIV_ROUND_CLOSEST(clk_rate, HZ);
-
- clockevent_set_clock(&jz4740_clockevent, clk_rate);
- jz4740_clockevent.min_delta_ns = clockevent_delta2ns(100, &jz4740_clockevent);
- jz4740_clockevent.min_delta_ticks = 100;
- jz4740_clockevent.max_delta_ns = clockevent_delta2ns(0xffff, &jz4740_clockevent);
- jz4740_clockevent.max_delta_ticks = 0xffff;
- jz4740_clockevent.cpumask = cpumask_of(0);
-
- clockevents_register_device(&jz4740_clockevent);
-
- ret = clocksource_register_hz(&jz4740_clocksource, clk_rate);
-
- if (ret)
- printk(KERN_ERR "Failed to register clocksource: %d\n", ret);
-
- sched_clock_register(jz4740_read_sched_clock, 16, clk_rate);
-
- setup_irq(jz4740_clockevent.irq, &timer_irqaction);
-
- ctrl = JZ_TIMER_CTRL_PRESCALE_16 | JZ_TIMER_CTRL_SRC_EXT;
-
- jz4740_timer_set_ctrl(TIMER_CLOCKEVENT, ctrl);
- jz4740_timer_set_ctrl(TIMER_CLOCKSOURCE, ctrl);
-
- jz4740_timer_set_period(TIMER_CLOCKEVENT, jz4740_jiffies_per_tick);
- jz4740_timer_irq_full_enable(TIMER_CLOCKEVENT);
-
- jz4740_timer_set_period(TIMER_CLOCKSOURCE, 0xffff);
-
- jz4740_timer_enable(TIMER_CLOCKEVENT);
- jz4740_timer_enable(TIMER_CLOCKSOURCE);
-}
diff --git a/arch/mips/jz4740/timer.c b/arch/mips/jz4740/timer.c
deleted file mode 100644
index 777877feef71..000000000000
--- a/arch/mips/jz4740/timer.c
+++ /dev/null
@@ -1,51 +0,0 @@
-/*
- * Copyright (C) 2010, Lars-Peter Clausen <[email protected]>
- * JZ4740 platform timer support
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License as published by the
- * Free Software Foundation; either version 2 of the License, or (at your
- * option) any later version.
- *
- * You should have received a copy of the GNU General Public License along
- * with this program; if not, write to the Free Software Foundation, Inc.,
- * 675 Mass Ave, Cambridge, MA 02139, USA.
- *
- */
-
-#include <linux/export.h>
-#include <linux/io.h>
-#include <linux/init.h>
-#include <linux/kernel.h>
-
-#include <asm/mach-jz4740/base.h>
-#include <asm/mach-jz4740/timer.h>
-
-void __iomem *jz4740_timer_base;
-EXPORT_SYMBOL_GPL(jz4740_timer_base);
-
-void jz4740_timer_enable_watchdog(void)
-{
- writel(BIT(16), jz4740_timer_base + JZ_REG_TIMER_STOP_CLEAR);
-}
-EXPORT_SYMBOL_GPL(jz4740_timer_enable_watchdog);
-
-void jz4740_timer_disable_watchdog(void)
-{
- writel(BIT(16), jz4740_timer_base + JZ_REG_TIMER_STOP_SET);
-}
-EXPORT_SYMBOL_GPL(jz4740_timer_disable_watchdog);
-
-void __init jz4740_timer_init(void)
-{
- jz4740_timer_base = ioremap(JZ4740_TCU_BASE_ADDR, 0x100);
-
- if (!jz4740_timer_base)
- panic("Failed to ioremap timer registers");
-
- /* Disable all timer clocks except for those used as system timers */
- writel(0x000100fc, jz4740_timer_base + JZ_REG_TIMER_STOP_SET);
-
- /* Timer irqs are unmasked by default, mask them */
- writel(0x00ff00ff, jz4740_timer_base + JZ_REG_TIMER_MASK_SET);
-}
--
2.11.0
On Wed, Dec 12, 2018 at 11:09:07PM +0100, Paul Cercueil wrote:
> The TCU channels 0 and 1 were previously reserved for system tasks, and
> thus unavailable for PWM.
>
> The driver will now only allow a PWM channel to be requested if memory
> resources corresponding to the register area of the channel were
> supplied to the driver. This allows the TCU channels to be reserved for
> system tasks from within the devicetree.
>
> Signed-off-by: Paul Cercueil <[email protected]>
While there is someone caring for this driver I'd like to complete (a
bit) my picture about the different capabilities and specialities of the
supported PWMs. So I have a few questions:
Is there a publicly available reference manual for this device? (If
yes, adding a link to the driver would be great.)
jz4740_pwm_config looks as if the currently running period isn't
completed before the new config is in effect. Is that correct? If yes,
can this be fixed? A similar question for set_polarity: Does setting the
JZ_TIMER_CTRL_PWM_ACTIVE_LOW bit in the control register take effect
immediately or is this shadowed until the next period starts?
How does the device's output behave after the PWM is disabled?
Does it complete the currently running period? How does the output
behave then? (active/inactive/high/low/high-z?)
> @@ -42,11 +68,7 @@ static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> char clk_name[16];
> int ret;
>
> - /*
> - * Timers 0 and 1 are used for system tasks, so they are unavailable
> - * for use as PWMs.
> - */
> - if (pwm->hwpwm < 2)
> + if (!jz4740_pwm_can_use_chn(jz, pwm->hwpwm))
> return -EBUSY;
Maybe EBUSY isn't the best choice here. If the needed register space for
the requested pwm is not included in the memory resources provided to
the device I'd prefer ENXIO or ENODEV.
> snprintf(clk_name, sizeof(clk_name), "timer%u", pwm->hwpwm);
> @@ -208,6 +230,12 @@ static int jz4740_pwm_probe(struct platform_device *pdev)
> return -EINVAL;
> }
>
> + jz4740->parent_res = platform_get_resource(
> + to_platform_device(dev->parent),
> + IORESOURCE_MEM, 0);
> + if (!jz4740->parent_res)
> + return -EINVAL;
> +
> jz4740->chip.dev = dev;
> jz4740->chip.ops = &jz4740_pwm_ops;
> jz4740->chip.npwm = NUM_PWM;
Thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hello,
On Wed, Dec 12, 2018 at 11:09:10PM +0100, Paul Cercueil wrote:
> The PWM in the JZ4725B works the same as in the JZ4740, except that it
> only has 6 channels available instead of 8.
this driver is probed only from device tree? If yes, it might be
sensible to specify the number of PWMs there and get it from there.
There doesn't seem to be a generic binding for that, but there are
several drivers that could benefit from it. (This is a bigger project
though and shouldn't stop your patch. Still more as it already got
Thierry's ack.)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hello,
On Wed, Dec 12, 2018 at 11:09:06PM +0100, Paul Cercueil wrote:
> [...]
> static int jz4740_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> {
> - uint32_t ctrl = jz4740_timer_get_ctrl(pwm->pwm);
> + struct jz4740_pwm_chip *jz = to_jz4740(chip);
>
> - ctrl |= JZ_TIMER_CTRL_PWM_ENABLE;
> - jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
> - jz4740_timer_enable(pwm->hwpwm);
> + /* Enable PWM output */
> + regmap_update_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm),
> + TCU_TCSR_PWM_EN, TCU_TCSR_PWM_EN);
Usually follow-up lines are indented to the matching parenthesis.
> [...]
> static int jz4740_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> int duty_ns, int period_ns)
> {
> struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
> + struct clk *clk = jz4740->clks[pwm->hwpwm];
> + unsigned long rate, new_rate, period, duty;
> unsigned long long tmp;
> - unsigned long period, duty;
> - unsigned int prescaler = 0;
> - uint16_t ctrl;
> + unsigned int tcsr;
> bool is_enabled;
>
> - tmp = (unsigned long long)clk_get_rate(jz4740->clk) * period_ns;
> - do_div(tmp, 1000000000);
> - period = tmp;
> + rate = clk_get_rate(clk);
> +
> + for (;;) {
> + tmp = (unsigned long long) rate * period_ns;
> + do_div(tmp, 1000000000);
>
> - while (period > 0xffff && prescaler < 6) {
> - period >>= 2;
> - ++prescaler;
> + if (tmp <= 0xffff)
> + break;
> +
> + new_rate = clk_round_rate(clk, rate / 2);
> +
> + if (new_rate < rate)
> + rate = new_rate;
> + else
> + return -EINVAL;
> }
>
> - if (prescaler == 6)
> - return -EINVAL;
> + clk_set_rate(clk, rate);
Maybe this could better live in a separate patch. If you split still
further to have the conversion to regmap in a single patch, then the
conversion to the clk_* functions and then improve the algorithm for the
clk settings each of the patches is easier to review than this one patch
that does all three things at once.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi,
Le jeu. 13 d?c. 2018 ? 10:18, Uwe Kleine-K?nig
<[email protected]> a ?crit :
> On Wed, Dec 12, 2018 at 11:09:07PM +0100, Paul Cercueil wrote:
>> The TCU channels 0 and 1 were previously reserved for system tasks,
>> and
>> thus unavailable for PWM.
>>
>> The driver will now only allow a PWM channel to be requested if
>> memory
>> resources corresponding to the register area of the channel were
>> supplied to the driver. This allows the TCU channels to be reserved
>> for
>> system tasks from within the devicetree.
>>
>> Signed-off-by: Paul Cercueil <[email protected]>
>
> While there is someone caring for this driver I'd like to complete (a
> bit) my picture about the different capabilities and specialities of
> the
> supported PWMs. So I have a few questions:
>
> Is there a publicly available reference manual for this device? (If
> yes, adding a link to the driver would be great.)
I have them here: https://zcrc.me/~paul/jz_docs/
> jz4740_pwm_config looks as if the currently running period isn't
> completed before the new config is in effect. Is that correct? If yes,
> can this be fixed? A similar question for set_polarity: Does setting
> the
> JZ_TIMER_CTRL_PWM_ACTIVE_LOW bit in the control register take effect
> immediately or is this shadowed until the next period starts?
I don't really know. We only use this driver for a rumble motor and
backlight.
Somebody would have to check with a logic analyzer.
> How does the device's output behave after the PWM is disabled?
> Does it complete the currently running period? How does the output
> behave then? (active/inactive/high/low/high-z?)
There's a bit to toggle between "graceful" shutdown (bit clear) and
"abrupt"
shutdown (bit set). TCSR bit 9. I think that graceful shutdown will
complete
the running period, then keep the level active. Abrupt shutdown will
keep the
current level of the line.
>> @@ -42,11 +68,7 @@ static int jz4740_pwm_request(struct pwm_chip
>> *chip, struct pwm_device *pwm)
>> char clk_name[16];
>> int ret;
>>
>> - /*
>> - * Timers 0 and 1 are used for system tasks, so they are
>> unavailable
>> - * for use as PWMs.
>> - */
>> - if (pwm->hwpwm < 2)
>> + if (!jz4740_pwm_can_use_chn(jz, pwm->hwpwm))
>> return -EBUSY;
>
> Maybe EBUSY isn't the best choice here. If the needed register space
> for
> the requested pwm is not included in the memory resources provided to
> the device I'd prefer ENXIO or ENODEV.
The idea was that if we don't get the register space we need, that means
the channel is used for something else, hence the EBUSY. Should I switch
it to ENXIO?
>> snprintf(clk_name, sizeof(clk_name), "timer%u", pwm->hwpwm);
>> @@ -208,6 +230,12 @@ static int jz4740_pwm_probe(struct
>> platform_device *pdev)
>> return -EINVAL;
>> }
>>
>> + jz4740->parent_res = platform_get_resource(
>> + to_platform_device(dev->parent),
>> + IORESOURCE_MEM, 0);
>> + if (!jz4740->parent_res)
>> + return -EINVAL;
>> +
>> jz4740->chip.dev = dev;
>> jz4740->chip.ops = &jz4740_pwm_ops;
>> jz4740->chip.npwm = NUM_PWM;
>
> Thanks
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-K?nig
> |
> Industrial Linux Solutions |
> http://www.pengutronix.de/ |
Hi,
Le jeu. 13 d?c. 2018 ? 10:24, Uwe Kleine-K?nig
<[email protected]> a ?crit :
> Hello,
>
> On Wed, Dec 12, 2018 at 11:09:10PM +0100, Paul Cercueil wrote:
>> The PWM in the JZ4725B works the same as in the JZ4740, except that
>> it
>> only has 6 channels available instead of 8.
>
> this driver is probed only from device tree? If yes, it might be
> sensible to specify the number of PWMs there and get it from there.
> There doesn't seem to be a generic binding for that, but there are
> several drivers that could benefit from it. (This is a bigger project
> though and shouldn't stop your patch. Still more as it already got
> Thierry's ack.)
I think there needs to be a proper guideline, as there doesn't seem to
be
a consensus about this. I learned from emails with Rob and Linus
(Walleij)
that I should not have in devicetree what I can deduce from the
compatible
string.
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-K?nig
> |
> Industrial Linux Solutions |
> http://www.pengutronix.de/ |
Hi,
Le jeu. 13 d?c. 2018 ? 10:30, Uwe Kleine-K?nig
<[email protected]> a ?crit :
> Hello,
>
> On Wed, Dec 12, 2018 at 11:09:06PM +0100, Paul Cercueil wrote:
>> [...]
>> static int jz4740_pwm_enable(struct pwm_chip *chip, struct
>> pwm_device *pwm)
>> {
>> - uint32_t ctrl = jz4740_timer_get_ctrl(pwm->pwm);
>> + struct jz4740_pwm_chip *jz = to_jz4740(chip);
>>
>> - ctrl |= JZ_TIMER_CTRL_PWM_ENABLE;
>> - jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
>> - jz4740_timer_enable(pwm->hwpwm);
>> + /* Enable PWM output */
>> + regmap_update_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm),
>> + TCU_TCSR_PWM_EN, TCU_TCSR_PWM_EN);
>
> Usually follow-up lines are indented to the matching parenthesis.
OK.
>> [...]
>> static int jz4740_pwm_config(struct pwm_chip *chip, struct
>> pwm_device *pwm,
>> int duty_ns, int period_ns)
>> {
>> struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
>> + struct clk *clk = jz4740->clks[pwm->hwpwm];
>> + unsigned long rate, new_rate, period, duty;
>> unsigned long long tmp;
>> - unsigned long period, duty;
>> - unsigned int prescaler = 0;
>> - uint16_t ctrl;
>> + unsigned int tcsr;
>> bool is_enabled;
>>
>> - tmp = (unsigned long long)clk_get_rate(jz4740->clk) * period_ns;
>> - do_div(tmp, 1000000000);
>> - period = tmp;
>> + rate = clk_get_rate(clk);
>> +
>> + for (;;) {
>> + tmp = (unsigned long long) rate * period_ns;
>> + do_div(tmp, 1000000000);
>>
>> - while (period > 0xffff && prescaler < 6) {
>> - period >>= 2;
>> - ++prescaler;
>> + if (tmp <= 0xffff)
>> + break;
>> +
>> + new_rate = clk_round_rate(clk, rate / 2);
>> +
>> + if (new_rate < rate)
>> + rate = new_rate;
>> + else
>> + return -EINVAL;
>> }
>>
>> - if (prescaler == 6)
>> - return -EINVAL;
>> + clk_set_rate(clk, rate);
>
> Maybe this could better live in a separate patch. If you split still
> further to have the conversion to regmap in a single patch, then the
> conversion to the clk_* functions and then improve the algorithm for
> the
> clk settings each of the patches is easier to review than this one
> patch
> that does all three things at once.
I can try.
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-K?nig
> |
> Industrial Linux Solutions |
> http://www.pengutronix.de/ |
On Thu, Dec 13, 2018 at 03:03:15PM +0100, Paul Cercueil wrote:
> Hi,
>
> Le jeu. 13 déc. 2018 à 10:24, Uwe Kleine-König
> <[email protected]> a écrit :
> > Hello,
> >
> > On Wed, Dec 12, 2018 at 11:09:10PM +0100, Paul Cercueil wrote:
> > > The PWM in the JZ4725B works the same as in the JZ4740, except that
> > > it
> > > only has 6 channels available instead of 8.
> >
> > this driver is probed only from device tree? If yes, it might be
> > sensible to specify the number of PWMs there and get it from there.
> > There doesn't seem to be a generic binding for that, but there are
> > several drivers that could benefit from it. (This is a bigger project
> > though and shouldn't stop your patch. Still more as it already got
> > Thierry's ack.)
>
> I think there needs to be a proper guideline, as there doesn't seem to be
> a consensus about this. I learned from emails with Rob and Linus (Walleij)
> that I should not have in devicetree what I can deduce from the compatible
> string.
Correct. If the compatible string already defines the number of PWMs
that the hardware exposes, there's no need to explicitly say so again in
DT.
Thierry
On Thu, Dec 13, 2018 at 02:58:31PM +0100, Paul Cercueil wrote:
> Hi,
>
> Le jeu. 13 d?c. 2018 ? 10:18, Uwe Kleine-K?nig
> <[email protected]> a ?crit :
> > On Wed, Dec 12, 2018 at 11:09:07PM +0100, Paul Cercueil wrote:
> > > The TCU channels 0 and 1 were previously reserved for system tasks,
> > > and
> > > thus unavailable for PWM.
> > >
> > > The driver will now only allow a PWM channel to be requested if
> > > memory
> > > resources corresponding to the register area of the channel were
> > > supplied to the driver. This allows the TCU channels to be reserved
> > > for
> > > system tasks from within the devicetree.
> > >
> > > Signed-off-by: Paul Cercueil <[email protected]>
> >
> > While there is someone caring for this driver I'd like to complete (a
> > bit) my picture about the different capabilities and specialities of the
> > supported PWMs. So I have a few questions:
> >
> > Is there a publicly available reference manual for this device? (If
> > yes, adding a link to the driver would be great.)
>
> I have them here: https://zcrc.me/~paul/jz_docs/
Is this link good enough to add it to the driver? From a quick view I'd
say this is another pwm implementation that gets active on pwm_disable().
Can you confirm this?
> > jz4740_pwm_config looks as if the currently running period isn't
> > completed before the new config is in effect. Is that correct? If yes,
> > can this be fixed? A similar question for set_polarity: Does setting the
> > JZ_TIMER_CTRL_PWM_ACTIVE_LOW bit in the control register take effect
> > immediately or is this shadowed until the next period starts?
>
> I don't really know. We only use this driver for a rumble motor and
> backlight.
> Somebody would have to check with a logic analyzer.
depending on the possible timings you might also be able to test this
e.g. by setting:
duty_cycle=1ms, period=5s
and then
duty_cycle=5s, period=5s
. If the implementation is right your display should be dark for nearly
5 seconds. (And the second call to pwm_apply should also block until the
display is on.)
> > How does the device's output behave after the PWM is disabled?
> > Does it complete the currently running period? How does the output
> > behave then? (active/inactive/high/low/high-z?)
>
> There's a bit to toggle between "graceful" shutdown (bit clear) and "abrupt"
> shutdown (bit set). TCSR bit 9. I think that graceful shutdown will complete
> the running period, then keep the level active. Abrupt shutdown will keep
> the
> current level of the line.
Can you confirm the things you think above? I'd like to have them
eventually documented in the driver.
> > > @@ -42,11 +68,7 @@ static int jz4740_pwm_request(struct pwm_chip
> > > *chip, struct pwm_device *pwm)
> > > char clk_name[16];
> > > int ret;
> > >
> > > - /*
> > > - * Timers 0 and 1 are used for system tasks, so they are
> > > unavailable
> > > - * for use as PWMs.
> > > - */
> > > - if (pwm->hwpwm < 2)
> > > + if (!jz4740_pwm_can_use_chn(jz, pwm->hwpwm))
> > > return -EBUSY;
> >
> > Maybe EBUSY isn't the best choice here. If the needed register space for
> > the requested pwm is not included in the memory resources provided to
> > the device I'd prefer ENXIO or ENODEV.
>
> The idea was that if we don't get the register space we need, that means
> the channel is used for something else, hence the EBUSY. Should I switch
> it to ENXIO?
I understand your reasoning, but I think it's misleading. If I get
-EBUSY from a PWM driver I'd start searching for another user of said
resource. With ENXIO or ENODEV it's more obvious that the driver isn't
responsible for the resource that was requested.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[Adding Linus Walleij to Cc:]
Hello,
On Thu, Dec 13, 2018 at 03:03:15PM +0100, Paul Cercueil wrote:
> Le jeu. 13 d?c. 2018 ? 10:24, Uwe Kleine-K?nig
> <[email protected]> a ?crit :
> > On Wed, Dec 12, 2018 at 11:09:10PM +0100, Paul Cercueil wrote:
> > > The PWM in the JZ4725B works the same as in the JZ4740, except that
> > > it
> > > only has 6 channels available instead of 8.
> >
> > this driver is probed only from device tree? If yes, it might be
> > sensible to specify the number of PWMs there and get it from there.
> > There doesn't seem to be a generic binding for that, but there are
> > several drivers that could benefit from it. (This is a bigger project
> > though and shouldn't stop your patch. Still more as it already got
> > Thierry's ack.)
>
> I think there needs to be a proper guideline, as there doesn't seem to be
> a consensus about this. I learned from emails with Rob and Linus (Walleij)
> that I should not have in devicetree what I can deduce from the compatible
> string.
I understood them a bit differently. It is ok to deduce things from the
compatible string. But if you define a generic property (say) "num-pwms"
that is used uniformly in most bindings this is ok, too. (And then the
two different devices could use the same compatible.)
An upside of the generic "num-pwms" property is that the pwm core could
sanity check pwm phandles before passing them to the hardware drivers.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Thu, Dec 13, 2018 at 9:42 PM Uwe Kleine-König
<[email protected]> wrote:
> [Adding Linus Walleij to Cc:]
>
> Hello,
>
> On Thu, Dec 13, 2018 at 03:03:15PM +0100, Paul Cercueil wrote:
> > Le jeu. 13 déc. 2018 à 10:24, Uwe Kleine-König
> > <[email protected]> a écrit :
> > > On Wed, Dec 12, 2018 at 11:09:10PM +0100, Paul Cercueil wrote:
> > > > The PWM in the JZ4725B works the same as in the JZ4740, except that
> > > > it
> > > > only has 6 channels available instead of 8.
> > >
> > > this driver is probed only from device tree? If yes, it might be
> > > sensible to specify the number of PWMs there and get it from there.
> > > There doesn't seem to be a generic binding for that, but there are
> > > several drivers that could benefit from it. (This is a bigger project
> > > though and shouldn't stop your patch. Still more as it already got
> > > Thierry's ack.)
> >
> > I think there needs to be a proper guideline, as there doesn't seem to be
> > a consensus about this. I learned from emails with Rob and Linus (Walleij)
> > that I should not have in devicetree what I can deduce from the compatible
> > string.
>
> I understood them a bit differently. It is ok to deduce things from the
> compatible string. But if you define a generic property (say) "num-pwms"
> that is used uniformly in most bindings this is ok, too. (And then the
> two different devices could use the same compatible.)
>
> An upside of the generic "num-pwms" property is that the pwm core could
> sanity check pwm phandles before passing them to the hardware drivers.
I don't know if this helps, but in GPIO we have "ngpios" which is
used to augment an existing block as to the number of lines actually
used with it.
The typical case is that an ASIC engineer synthesize a block for
32 GPIOs but only 12 of them are routed to external pads. So
we augment the behaviour of that driver to only use 12 of the
32 lines.
I guess using the remaining 20 lines "works" in a sense but they
have no practical use and will just bias electrons in the silicon
for no use.
So if the PWM case is something similar, then by all means add
num-pwms.
Yours,
Linus Walleij
Hello,
On Fri, Dec 14, 2018 at 02:50:20PM +0100, Linus Walleij wrote:
> On Thu, Dec 13, 2018 at 9:42 PM Uwe Kleine-K?nig
> <[email protected]> wrote:
> > [Adding Linus Walleij to Cc:]
> > On Thu, Dec 13, 2018 at 03:03:15PM +0100, Paul Cercueil wrote:
> > > Le jeu. 13 d?c. 2018 ? 10:24, Uwe Kleine-K?nig
> > > <[email protected]> a ?crit :
> > > > On Wed, Dec 12, 2018 at 11:09:10PM +0100, Paul Cercueil wrote:
> > > > > The PWM in the JZ4725B works the same as in the JZ4740, except that
> > > > > it
> > > > > only has 6 channels available instead of 8.
> > > >
> > > > this driver is probed only from device tree? If yes, it might be
> > > > sensible to specify the number of PWMs there and get it from there.
> > > > There doesn't seem to be a generic binding for that, but there are
> > > > several drivers that could benefit from it. (This is a bigger project
> > > > though and shouldn't stop your patch. Still more as it already got
> > > > Thierry's ack.)
> > >
> > > I think there needs to be a proper guideline, as there doesn't seem to be
> > > a consensus about this. I learned from emails with Rob and Linus (Walleij)
> > > that I should not have in devicetree what I can deduce from the compatible
> > > string.
> >
> > I understood them a bit differently. It is ok to deduce things from the
> > compatible string. But if you define a generic property (say) "num-pwms"
> > that is used uniformly in most bindings this is ok, too. (And then the
> > two different devices could use the same compatible.)
> >
> > An upside of the generic "num-pwms" property is that the pwm core could
> > sanity check pwm phandles before passing them to the hardware drivers.
>
> I don't know if this helps, but in GPIO we have "ngpios" which is
> used to augment an existing block as to the number of lines actually
> used with it.
>
> The typical case is that an ASIC engineer synthesize a block for
> 32 GPIOs but only 12 of them are routed to external pads. So
> we augment the behaviour of that driver to only use 12 of the
> 32 lines.
>
> I guess using the remaining 20 lines "works" in a sense but they
> have no practical use and will just bias electrons in the silicon
> for no use.
This looks very similar to the case under discussion.
> So if the PWM case is something similar, then by all means add
> num-pwms.
.. or "npwms" to use the same nomenclature as the gpio binding?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Fri, Dec 14, 2018 at 3:26 PM Uwe Kleine-König
<[email protected]> wrote:
> On Fri, Dec 14, 2018 at 02:50:20PM +0100, Linus Walleij wrote:
> > So if the PWM case is something similar, then by all means add
> > num-pwms.
>
> .. or "npwms" to use the same nomenclature as the gpio binding?
Either works for me, I don't have a very high need of consistency
but there are others that have so your suggestion seems wise.
Yours,
Linus Walleij
Hi,
Le jeu. 13 d?c. 2018 ? 21:32, Uwe Kleine-K?nig
<[email protected]> a ?crit :
> On Thu, Dec 13, 2018 at 02:58:31PM +0100, Paul Cercueil wrote:
>> Hi,
>>
>> Le jeu. 13 d?c. 2018 ? 10:18, Uwe Kleine-K?nig
>> <[email protected]> a ?crit :
>> > On Wed, Dec 12, 2018 at 11:09:07PM +0100, Paul Cercueil wrote:
>> > > The TCU channels 0 and 1 were previously reserved for system
>> tasks,
>> > > and
>> > > thus unavailable for PWM.
>> > >
>> > > The driver will now only allow a PWM channel to be requested if
>> > > memory
>> > > resources corresponding to the register area of the channel
>> were
>> > > supplied to the driver. This allows the TCU channels to be
>> reserved
>> > > for
>> > > system tasks from within the devicetree.
>> > >
>> > > Signed-off-by: Paul Cercueil <[email protected]>
>> >
>> > While there is someone caring for this driver I'd like to
>> complete (a
>> > bit) my picture about the different capabilities and specialities
>> of the
>> > supported PWMs. So I have a few questions:
>> >
>> > Is there a publicly available reference manual for this device?
>> (If
>> > yes, adding a link to the driver would be great.)
>>
>> I have them here: https://zcrc.me/~paul/jz_docs/
>
> Is this link good enough to add it to the driver? From a quick view
> I'd
> say this is another pwm implementation that gets active on
> pwm_disable().
> Can you confirm this?
It's my website, so if these get moved, I can update the link.
What do you mean it gets active on pwm_disable()? If pwm_disable() gets
called
the PWM line goes back to inactive state, which is what it should do.
>> > jz4740_pwm_config looks as if the currently running period isn't
>> > completed before the new config is in effect. Is that correct? If
>> yes,
>> > can this be fixed? A similar question for set_polarity: Does
>> setting the
>> > JZ_TIMER_CTRL_PWM_ACTIVE_LOW bit in the control register take
>> effect
>> > immediately or is this shadowed until the next period starts?
>>
>> I don't really know. We only use this driver for a rumble motor and
>> backlight.
>> Somebody would have to check with a logic analyzer.
>
> depending on the possible timings you might also be able to test this
> e.g. by setting:
>
> duty_cycle=1ms, period=5s
>
> and then
>
> duty_cycle=5s, period=5s
>
> . If the implementation is right your display should be dark for
> nearly
> 5 seconds. (And the second call to pwm_apply should also block until
> the
> display is on.)
So it switches to full ON as soon as I set the duty cycle to 5s. Same
for
the polarity, it is updated as soon as the register is written. So the
registers are not shadowed.
>> > How does the device's output behave after the PWM is disabled?
>> > Does it complete the currently running period? How does the output
>> > behave then? (active/inactive/high/low/high-z?)
>>
>> There's a bit to toggle between "graceful" shutdown (bit clear) and
>> "abrupt"
>> shutdown (bit set). TCSR bit 9. I think that graceful shutdown will
>> complete
>> the running period, then keep the level active. Abrupt shutdown
>> will keep
>> the
>> current level of the line.
>
> Can you confirm the things you think above? I'd like to have them
> eventually documented in the driver.
From what I can see, with "abrupt" shutdown the line will always
return to
its inactive state (be it low or high, depending on the polarity).
Setting
this bit to "graceful" shutdown, the only difference is that the
hardware
will keep its current state, active or inactive. That's why we use the
"abrupt" shutdown in the PWM driver.
>> > > @@ -42,11 +68,7 @@ static int jz4740_pwm_request(struct
>> pwm_chip
>> > > *chip, struct pwm_device *pwm)
>> > > char clk_name[16];
>> > > int ret;
>> > >
>> > > - /*
>> > > - * Timers 0 and 1 are used for system tasks, so they are
>> > > unavailable
>> > > - * for use as PWMs.
>> > > - */
>> > > - if (pwm->hwpwm < 2)
>> > > + if (!jz4740_pwm_can_use_chn(jz, pwm->hwpwm))
>> > > return -EBUSY;
>> >
>> > Maybe EBUSY isn't the best choice here. If the needed register
>> space for
>> > the requested pwm is not included in the memory resources
>> provided to
>> > the device I'd prefer ENXIO or ENODEV.
>>
>> The idea was that if we don't get the register space we need, that
>> means
>> the channel is used for something else, hence the EBUSY. Should I
>> switch
>> it to ENXIO?
>
> I understand your reasoning, but I think it's misleading. If I get
> -EBUSY from a PWM driver I'd start searching for another user of said
> resource. With ENXIO or ENODEV it's more obvious that the driver isn't
> responsible for the resource that was requested.
OK.
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-K?nig
> |
> Industrial Linux Solutions |
> http://www.pengutronix.de/ |
Hi,
Le ven. 14 d?c. 2018 ? 15:26, Uwe Kleine-K?nig
<[email protected]> a ?crit :
> Hello,
>
> On Fri, Dec 14, 2018 at 02:50:20PM +0100, Linus Walleij wrote:
>> On Thu, Dec 13, 2018 at 9:42 PM Uwe Kleine-K?nig
>> <[email protected]> wrote:
>> > [Adding Linus Walleij to Cc:]
>> > On Thu, Dec 13, 2018 at 03:03:15PM +0100, Paul Cercueil wrote:
>> > > Le jeu. 13 d?c. 2018 ? 10:24, Uwe Kleine-K?nig
>> > > <[email protected]> a ?crit :
>> > > > On Wed, Dec 12, 2018 at 11:09:10PM +0100, Paul Cercueil wrote:
>> > > > > The PWM in the JZ4725B works the same as in the JZ4740,
>> except that
>> > > > > it
>> > > > > only has 6 channels available instead of 8.
>> > > >
>> > > > this driver is probed only from device tree? If yes, it might
>> be
>> > > > sensible to specify the number of PWMs there and get it from
>> there.
>> > > > There doesn't seem to be a generic binding for that, but
>> there are
>> > > > several drivers that could benefit from it. (This is a bigger
>> project
>> > > > though and shouldn't stop your patch. Still more as it
>> already got
>> > > > Thierry's ack.)
>> > >
>> > > I think there needs to be a proper guideline, as there doesn't
>> seem to be
>> > > a consensus about this. I learned from emails with Rob and
>> Linus (Walleij)
>> > > that I should not have in devicetree what I can deduce from the
>> compatible
>> > > string.
>> >
>> > I understood them a bit differently. It is ok to deduce things
>> from the
>> > compatible string. But if you define a generic property (say)
>> "num-pwms"
>> > that is used uniformly in most bindings this is ok, too. (And
>> then the
>> > two different devices could use the same compatible.)
>> >
>> > An upside of the generic "num-pwms" property is that the pwm core
>> could
>> > sanity check pwm phandles before passing them to the hardware
>> drivers.
>>
>> I don't know if this helps, but in GPIO we have "ngpios" which is
>> used to augment an existing block as to the number of lines actually
>> used with it.
>>
>> The typical case is that an ASIC engineer synthesize a block for
>> 32 GPIOs but only 12 of them are routed to external pads. So
>> we augment the behaviour of that driver to only use 12 of the
>> 32 lines.
>>
>> I guess using the remaining 20 lines "works" in a sense but they
>> have no practical use and will just bias electrons in the silicon
>> for no use.
>
> This looks very similar to the case under discussion.
>
>> So if the PWM case is something similar, then by all means add
>> num-pwms.
>
> .. or "npwms" to use the same nomenclature as the gpio binding?
If we're going to do something like this, should it be the drivers or
the core (within pwmchip_add) that checks for this "npwms" property?
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-K?nig
> |
> Industrial Linux Solutions |
> http://www.pengutronix.de/ |
Hello Paul,
On Sun, Dec 16, 2018 at 02:36:03PM +0100, Paul Cercueil wrote:
> Le jeu. 13 d?c. 2018 ? 21:32, Uwe Kleine-K?nig
> <[email protected]> a ?crit :
> > On Thu, Dec 13, 2018 at 02:58:31PM +0100, Paul Cercueil wrote:
> > > Hi,
> > >
> > > Le jeu. 13 d?c. 2018 ? 10:18, Uwe Kleine-K?nig
> > > <[email protected]> a ?crit :
> > > > On Wed, Dec 12, 2018 at 11:09:07PM +0100, Paul Cercueil wrote:
> > > > > The TCU channels 0 and 1 were previously reserved for system
> > > tasks,
> > > > > and
> > > > > thus unavailable for PWM.
> > > > >
> > > > > The driver will now only allow a PWM channel to be requested if
> > > > > memory
> > > > > resources corresponding to the register area of the channel
> > > were
> > > > > supplied to the driver. This allows the TCU channels to be
> > > reserved
> > > > > for
> > > > > system tasks from within the devicetree.
> > > > >
> > > > > Signed-off-by: Paul Cercueil <[email protected]>
> > > >
> > > > While there is someone caring for this driver I'd like to
> > > complete (a
> > > > bit) my picture about the different capabilities and specialities
> > > of the
> > > > supported PWMs. So I have a few questions:
> > > >
> > > > Is there a publicly available reference manual for this device?
> > > (If
> > > > yes, adding a link to the driver would be great.)
> > >
> > > I have them here: https://zcrc.me/~paul/jz_docs/
> >
> > Is this link good enough to add it to the driver? From a quick view I'd
> > say this is another pwm implementation that gets active on
> > pwm_disable().
> > Can you confirm this?
>
> It's my website, so if these get moved, I can update the link.
>
> What do you mean it gets active on pwm_disable()? If pwm_disable() gets
> called the PWM line goes back to inactive state, which is what it
> should do.
The register description for TCSRn.PWM_EN reads:
1: PWM pin output enable
0: PWM pin output disable, and the PWM pin will be set to the
initial level according to INITL
As I read the manual (but that differes from the driver) you should use
INITL=1 for an uninverted PWM such that the period starts with the
output being 1. And with that I'd expect that the output goes high on
disable. Given that the driver inverts this and sets INITL (called
JZ_TIMER_CTRL_PWM_ACTIVE_LOW in the driver) for an inverted pwm that
behaviour is ok. With this approach the bug is not the level when
pwm_disable was called but that the period doesn't start with the active
part of the period.
> > > > jz4740_pwm_config looks as if the currently running period isn't
> > > > completed before the new config is in effect. Is that correct? If
> > > yes,
> > > > can this be fixed? A similar question for set_polarity: Does
> > > setting the
> > > > JZ_TIMER_CTRL_PWM_ACTIVE_LOW bit in the control register take
> > > effect
> > > > immediately or is this shadowed until the next period starts?
> > >
> > > I don't really know. We only use this driver for a rumble motor and
> > > backlight.
> > > Somebody would have to check with a logic analyzer.
> >
> > depending on the possible timings you might also be able to test this
> > e.g. by setting:
> >
> > duty_cycle=1ms, period=5s
> >
> > and then
> >
> > duty_cycle=5s, period=5s
> >
> > . If the implementation is right your display should be dark for nearly
> > 5 seconds. (And the second call to pwm_apply should also block until the
> > display is on.)
>
> So it switches to full ON as soon as I set the duty cycle to 5s. Same for
> the polarity, it is updated as soon as the register is written. So the
> registers are not shadowed.
Then I think the right thing to do is to gracefully disable the hardware
on a duty/period change first to ensure the currently running period is
completed before the next configuration gets active.
> > > > How does the device's output behave after the PWM is disabled?
> > > > Does it complete the currently running period? How does the output
> > > > behave then? (active/inactive/high/low/high-z?)
> > >
> > > There's a bit to toggle between "graceful" shutdown (bit clear) and
> > > "abrupt"
> > > shutdown (bit set). TCSR bit 9. I think that graceful shutdown will
> > > complete
> > > the running period, then keep the level active. Abrupt shutdown
> > > will keep
> > > the
> > > current level of the line.
> >
> > Can you confirm the things you think above? I'd like to have them
> > eventually documented in the driver.
>
> From what I can see, with "abrupt" shutdown the line will always return to
> its inactive state (be it low or high, depending on the polarity). Setting
> this bit to "graceful" shutdown, the only difference is that the hardware
> will keep its current state, active or inactive. That's why we use the
> "abrupt" shutdown in the PWM driver.
That sounds backwards from your last mail and the reference manual. As I
read the manual "graceful" means the period is completed until FULL
matches. And I understand "aprupt" as "immediatly freeze". If this is
the case the names in the manual are well chosen.
For the pwm framework the intended behaviour is the graceful one. (For
both, disable and duty/period change.)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Sun, Dec 16, 2018 at 03:18:52PM +0100, Paul Cercueil wrote:
> Hi,
>
> Le ven. 14 d?c. 2018 ? 15:26, Uwe Kleine-K?nig
> <[email protected]> a ?crit :
> > Hello,
> >
> > On Fri, Dec 14, 2018 at 02:50:20PM +0100, Linus Walleij wrote:
> > > On Thu, Dec 13, 2018 at 9:42 PM Uwe Kleine-K?nig
> > > <[email protected]> wrote:
> > > > [Adding Linus Walleij to Cc:]
> > > > On Thu, Dec 13, 2018 at 03:03:15PM +0100, Paul Cercueil wrote:
> > > > > Le jeu. 13 d?c. 2018 ? 10:24, Uwe Kleine-K?nig
> > > > > <[email protected]> a ?crit :
> > > > > > On Wed, Dec 12, 2018 at 11:09:10PM +0100, Paul Cercueil wrote:
> > > > > > > The PWM in the JZ4725B works the same as in the JZ4740,
> > > > > > > except that it only has 6 channels available instead of
> > > > > > > 8.
> > > > > >
> > > > > > this driver is probed only from device tree? If yes, it
> > > > > > might be sensible to specify the number of PWMs there and
> > > > > > get it from there.
> > > > > > There doesn't seem to be a generic binding for that, but there are
> > > > > > several drivers that could benefit from it. (This is a bigger project
> > > > > > though and shouldn't stop your patch. Still more as it already got
> > > > > > Thierry's ack.)
> > > > >
> > > > > I think there needs to be a proper guideline, as there doesn't seem to be
> > > > > a consensus about this. I learned from emails with Rob and Linus (Walleij)
> > > > > that I should not have in devicetree what I can deduce from the compatible
> > > > > string.
> > > >
> > > > I understood them a bit differently. It is ok to deduce things from the
> > > > compatible string. But if you define a generic property (say) "num-pwms"
> > > > that is used uniformly in most bindings this is ok, too. (And then the
> > > > two different devices could use the same compatible.)
> > > >
> > > > An upside of the generic "num-pwms" property is that the pwm core could
> > > > sanity check pwm phandles before passing them to the hardware drivers.
> > >
> > > I don't know if this helps, but in GPIO we have "ngpios" which is
> > > used to augment an existing block as to the number of lines actually
> > > used with it.
> > >
> > > The typical case is that an ASIC engineer synthesize a block for
> > > 32 GPIOs but only 12 of them are routed to external pads. So
> > > we augment the behaviour of that driver to only use 12 of the
> > > 32 lines.
> > >
> > > I guess using the remaining 20 lines "works" in a sense but they
> > > have no practical use and will just bias electrons in the silicon
> > > for no use.
> >
> > This looks very similar to the case under discussion.
> >
> > > So if the PWM case is something similar, then by all means add
> > > num-pwms.
> >
> > .. or "npwms" to use the same nomenclature as the gpio binding?
>
> If we're going to do something like this, should it be the drivers or
> the core (within pwmchip_add) that checks for this "npwms" property?
Of course this should be done in the core. The driver than can rely on
the validity of the index. But as I wrote before, this shouldn't stop
your patch from going in.
But if Thierry agrees that this npmws (or num-pwms) is a good idea, it
would be great to start early to convert drivers.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Wed, Dec 12, 2018 at 11:08:58PM +0100, Paul Cercueil wrote:
> Add documentation about how to properly use the Ingenic TCU
> (Timer/Counter Unit) drivers from devicetree.
>
> Signed-off-by: Paul Cercueil <[email protected]>
> ---
>
> Notes:
> v4: New patch in this series. Corresponds to V2 patches 3-4-5 with
> added content.
>
> v5: - Edited PWM/watchdog DT bindings documentation to point to the new
> document.
> - Moved main document to
> Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> - Updated documentation to reflect the new devicetree bindings.
>
> v6: - Removed PWM/watchdog documentation files as asked by upstream
> - Removed doc about properties that should be implicit
> - Removed doc about ingenic,timer-channel /
> ingenic,clocksource-channel as they are gone
> - Fix WDT clock name in the binding doc
> - Fix lengths of register areas in watchdog/pwm nodes
>
> v7: No change
>
> v8: - Fix address of the PWM node
> - Added doc about system timer and clocksource children nodes
I thought we'd sorted this out...
>
> .../devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt | 25 ---
> .../devicetree/bindings/timer/ingenic,tcu.txt | 176 +++++++++++++++++++++
> .../bindings/watchdog/ingenic,jz4740-wdt.txt | 17 --
> 3 files changed, 176 insertions(+), 42 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
> create mode 100644 Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> delete mode 100644 Documentation/devicetree/bindings/watchdog/ingenic,jz4740-wdt.txt
>
> diff --git a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt b/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
> deleted file mode 100644
> index 7d9d3f90641b..000000000000
> --- a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
> +++ /dev/null
> @@ -1,25 +0,0 @@
> -Ingenic JZ47xx PWM Controller
> -=============================
> -
> -Required properties:
> -- compatible: One of:
> - * "ingenic,jz4740-pwm"
> - * "ingenic,jz4770-pwm"
> - * "ingenic,jz4780-pwm"
> -- #pwm-cells: Should be 3. See pwm.txt in this directory for a description
> - of the cells format.
> -- clocks : phandle to the external clock.
> -- clock-names : Should be "ext".
> -
> -
> -Example:
> -
> - pwm: pwm@10002000 {
> - compatible = "ingenic,jz4740-pwm";
> - reg = <0x10002000 0x1000>;
> -
> - #pwm-cells = <3>;
> -
> - clocks = <&ext>;
> - clock-names = "ext";
> - };
> diff --git a/Documentation/devicetree/bindings/timer/ingenic,tcu.txt b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> new file mode 100644
> index 000000000000..8a4ce7edf50f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> @@ -0,0 +1,176 @@
> +Ingenic JZ47xx SoCs Timer/Counter Unit devicetree bindings
> +==========================================================
> +
> +For a description of the TCU hardware and drivers, have a look at
> +Documentation/mips/ingenic-tcu.txt.
> +
> +Required properties:
> +
> +- compatible: Must be one of:
> + * ingenic,jz4740-tcu
> + * ingenic,jz4725b-tcu
> + * ingenic,jz4770-tcu
> +- reg: Should be the offset/length value corresponding to the TCU registers
> +- clocks: List of phandle & clock specifiers for clocks external to the TCU.
> + The "pclk", "rtc", "ext" and "tcu" clocks should be provided.
> +- clock-names: List of name strings for the external clocks.
> +- #clock-cells: Should be <1>;
> + Clock consumers specify this argument to identify a clock. The valid values
> + may be found in <dt-bindings/clock/ingenic,tcu.h>.
> +- interrupt-controller : Identifies the node as an interrupt controller
> +- #interrupt-cells : Specifies the number of cells needed to encode an
> + interrupt source. The value should be 1.
> +- interrupt-parent : phandle of the interrupt controller.
> +- interrupts : Specifies the interrupt the controller is connected to.
> +
> +
> +Children nodes
> +==========================================================
> +
> +
> +PWM node:
> +---------
> +
> +Required properties:
> +
> +- compatible: Must be one of:
> + * ingenic,jz4740-pwm
> + * ingenic,jz4725b-pwm
> +- #pwm-cells: Should be 3. See ../pwm/pwm.txt for a description of the cell
> + format.
> +- clocks: List of phandle & clock specifiers for the TCU clocks.
> +- clock-names: List of name strings for the TCU clocks.
> +
> +
> +Watchdog node:
> +--------------
> +
> +Required properties:
> +
> +- compatible: Must be one of:
> + * ingenic,jz4740-watchdog
> + * ingenic,jz4780-watchdog
> +- clocks: phandle to the WDT clock
> +- clock-names: should be "wdt"
> +
> +
> +OST node:
> +---------
> +
> +Required properties:
> +
> +- compatible: Must be one of:
> + * ingenic,jz4725b-ost
> + * ingenic,jz4770-ost
> +- clocks: phandle to the OST clock
> +- clock-names: should be "ost"
> +- interrupts : Specifies the interrupt the OST is connected to.
> +
> +
> +System timer node:
> +------------------
> +
> +Required properties:
> +
> +- compatible: Must be "ingenic,jz4740-tcu-timer"
Is this an actual sub-block? Or just a way to assign a timer to a
clockevent?
> +- clocks: phandle to the clock of the TCU channel used
> +- clock-names: Should be "timer"
> +- interrupts : Specifies the interrupt line of the TCU channel used.
> +
> +
> +System clocksource node:
> +------------------------
> +
> +Required properties:
> +
> +- compatible: Must be "ingenic,jz4740-tcu-clocksource"
The h/w has a block called 'clocksource'?
If there's a difference in the timer channels, then that difference
should be described in DT, not just 'use timer X for clocksource'.
> +- clocks: phandle to the clock of the TCU channel used
> +- clock-names: Should be "timer"
Hi Rob,
Le lun. 17 d?c. 2018 ? 22:05, Rob Herring <[email protected]> a ?crit :
> On Wed, Dec 12, 2018 at 11:08:58PM +0100, Paul Cercueil wrote:
>> Add documentation about how to properly use the Ingenic TCU
>> (Timer/Counter Unit) drivers from devicetree.
>>
>> Signed-off-by: Paul Cercueil <[email protected]>
>> ---
>>
>> Notes:
>> v4: New patch in this series. Corresponds to V2 patches 3-4-5
>> with
>> added content.
>>
>> v5: - Edited PWM/watchdog DT bindings documentation to point
>> to the new
>> document.
>> - Moved main document to
>> Documentation/devicetree/bindings/timer/ingenic,tcu.txt
>> - Updated documentation to reflect the new devicetree
>> bindings.
>>
>> v6: - Removed PWM/watchdog documentation files as asked by
>> upstream
>> - Removed doc about properties that should be implicit
>> - Removed doc about ingenic,timer-channel /
>> ingenic,clocksource-channel as they are gone
>> - Fix WDT clock name in the binding doc
>> - Fix lengths of register areas in watchdog/pwm nodes
>>
>> v7: No change
>>
>> v8: - Fix address of the PWM node
>> - Added doc about system timer and clocksource children
>> nodes
>
> I thought we'd sorted this out...
Yeah, well I just can't please everybody. V6/V7 didn't have the
system timer or clocksource in devicetree, which was good for
you, but then the driver nearly doubled in size and complexity,
and Thierry rightfully refused it. Now I'm at the point where
I'm trying alternative ways of encoding the information in
devicetree, as suggested by various people, just so that you
accept it. Because I don't see any other option.
>>
>> .../devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt | 25 ---
>> .../devicetree/bindings/timer/ingenic,tcu.txt | 176
>> +++++++++++++++++++++
>> .../bindings/watchdog/ingenic,jz4740-wdt.txt | 17 --
>> 3 files changed, 176 insertions(+), 42 deletions(-)
>> delete mode 100644
>> Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
>> create mode 100644
>> Documentation/devicetree/bindings/timer/ingenic,tcu.txt
>> delete mode 100644
>> Documentation/devicetree/bindings/watchdog/ingenic,jz4740-wdt.txt
>>
>> diff --git
>> a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
>> b/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
>> deleted file mode 100644
>> index 7d9d3f90641b..000000000000
>> --- a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
>> +++ /dev/null
>> @@ -1,25 +0,0 @@
>> -Ingenic JZ47xx PWM Controller
>> -=============================
>> -
>> -Required properties:
>> -- compatible: One of:
>> - * "ingenic,jz4740-pwm"
>> - * "ingenic,jz4770-pwm"
>> - * "ingenic,jz4780-pwm"
>> -- #pwm-cells: Should be 3. See pwm.txt in this directory for a
>> description
>> - of the cells format.
>> -- clocks : phandle to the external clock.
>> -- clock-names : Should be "ext".
>> -
>> -
>> -Example:
>> -
>> - pwm: pwm@10002000 {
>> - compatible = "ingenic,jz4740-pwm";
>> - reg = <0x10002000 0x1000>;
>> -
>> - #pwm-cells = <3>;
>> -
>> - clocks = <&ext>;
>> - clock-names = "ext";
>> - };
>> diff --git
>> a/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
>> b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
>> new file mode 100644
>> index 000000000000..8a4ce7edf50f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
>> @@ -0,0 +1,176 @@
>> +Ingenic JZ47xx SoCs Timer/Counter Unit devicetree bindings
>> +==========================================================
>> +
>> +For a description of the TCU hardware and drivers, have a look at
>> +Documentation/mips/ingenic-tcu.txt.
>> +
>> +Required properties:
>> +
>> +- compatible: Must be one of:
>> + * ingenic,jz4740-tcu
>> + * ingenic,jz4725b-tcu
>> + * ingenic,jz4770-tcu
>> +- reg: Should be the offset/length value corresponding to the TCU
>> registers
>> +- clocks: List of phandle & clock specifiers for clocks external
>> to the TCU.
>> + The "pclk", "rtc", "ext" and "tcu" clocks should be provided.
>> +- clock-names: List of name strings for the external clocks.
>> +- #clock-cells: Should be <1>;
>> + Clock consumers specify this argument to identify a clock. The
>> valid values
>> + may be found in <dt-bindings/clock/ingenic,tcu.h>.
>> +- interrupt-controller : Identifies the node as an interrupt
>> controller
>> +- #interrupt-cells : Specifies the number of cells needed to
>> encode an
>> + interrupt source. The value should be 1.
>> +- interrupt-parent : phandle of the interrupt controller.
>> +- interrupts : Specifies the interrupt the controller is connected
>> to.
>> +
>> +
>> +Children nodes
>> +==========================================================
>> +
>> +
>> +PWM node:
>> +---------
>> +
>> +Required properties:
>> +
>> +- compatible: Must be one of:
>> + * ingenic,jz4740-pwm
>> + * ingenic,jz4725b-pwm
>> +- #pwm-cells: Should be 3. See ../pwm/pwm.txt for a description of
>> the cell
>> + format.
>> +- clocks: List of phandle & clock specifiers for the TCU clocks.
>> +- clock-names: List of name strings for the TCU clocks.
>> +
>> +
>> +Watchdog node:
>> +--------------
>> +
>> +Required properties:
>> +
>> +- compatible: Must be one of:
>> + * ingenic,jz4740-watchdog
>> + * ingenic,jz4780-watchdog
>> +- clocks: phandle to the WDT clock
>> +- clock-names: should be "wdt"
>> +
>> +
>> +OST node:
>> +---------
>> +
>> +Required properties:
>> +
>> +- compatible: Must be one of:
>> + * ingenic,jz4725b-ost
>> + * ingenic,jz4770-ost
>> +- clocks: phandle to the OST clock
>> +- clock-names: should be "ost"
>> +- interrupts : Specifies the interrupt the OST is connected to.
>> +
>> +
>> +System timer node:
>> +------------------
>> +
>> +Required properties:
>> +
>> +- compatible: Must be "ingenic,jz4740-tcu-timer"
>
> Is this an actual sub-block? Or just a way to assign a timer to a
> clockevent?
Just a way to assign a timer to a clockevent.
>> +- clocks: phandle to the clock of the TCU channel used
>> +- clock-names: Should be "timer"
>> +- interrupts : Specifies the interrupt line of the TCU channel
>> used.
>> +
>> +
>> +System clocksource node:
>> +------------------------
>> +
>> +Required properties:
>> +
>> +- compatible: Must be "ingenic,jz4740-tcu-clocksource"
>
> The h/w has a block called 'clocksource'?
>
> If there's a difference in the timer channels, then that difference
> should be described in DT, not just 'use timer X for clocksource'.
Same as above, this is just a way to assign a channel to the
clocksource.
>>
> +- clock-names: Should be "timer"
> +- clocks: phandle to the clock of the TCU channel use
Regards,
- Paul Cercueil
Quoting Paul Cercueil (2018-12-12 14:08:56)
> This header provides clock numbers for the ingenic,tcu
> DT binding.
>
> Signed-off-by: Paul Cercueil <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
> ---
Acked-by: Stephen Boyd <[email protected]>
On Mon, Dec 17, 2018 at 4:04 PM Paul Cercueil <[email protected]> wrote:
>
> Hi Rob,
>
> Le lun. 17 déc. 2018 à 22:05, Rob Herring <[email protected]> a écrit :
> > On Wed, Dec 12, 2018 at 11:08:58PM +0100, Paul Cercueil wrote:
> >> Add documentation about how to properly use the Ingenic TCU
> >> (Timer/Counter Unit) drivers from devicetree.
> >>
> >> Signed-off-by: Paul Cercueil <[email protected]>
> >> ---
> >>
> >> Notes:
> >> v4: New patch in this series. Corresponds to V2 patches 3-4-5
> >> with
> >> added content.
> >>
> >> v5: - Edited PWM/watchdog DT bindings documentation to point
> >> to the new
> >> document.
> >> - Moved main document to
> >> Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> >> - Updated documentation to reflect the new devicetree
> >> bindings.
> >>
> >> v6: - Removed PWM/watchdog documentation files as asked by
> >> upstream
> >> - Removed doc about properties that should be implicit
> >> - Removed doc about ingenic,timer-channel /
> >> ingenic,clocksource-channel as they are gone
> >> - Fix WDT clock name in the binding doc
> >> - Fix lengths of register areas in watchdog/pwm nodes
> >>
> >> v7: No change
> >>
> >> v8: - Fix address of the PWM node
> >> - Added doc about system timer and clocksource children
> >> nodes
> >
> > I thought we'd sorted this out...
>
> Yeah, well I just can't please everybody. V6/V7 didn't have the
> system timer or clocksource in devicetree, which was good for
> you, but then the driver nearly doubled in size and complexity,
> and Thierry rightfully refused it. Now I'm at the point where
You mean Daniel?
> I'm trying alternative ways of encoding the information in
> devicetree, as suggested by various people, just so that you
> accept it. Because I don't see any other option.
Does the problem boil down to needing to reserve channel x to use PWMx
pin? If so, just do a mask property of reserved for PWM channels.
Sorry this is going in circles.
Rob
On Mon, Dec 17, 2018 at 08:53:21AM +0100, Uwe Kleine-König wrote:
> On Sun, Dec 16, 2018 at 03:18:52PM +0100, Paul Cercueil wrote:
> > Hi,
> >
> > Le ven. 14 déc. 2018 à 15:26, Uwe Kleine-König
> > <[email protected]> a écrit :
> > > Hello,
> > >
> > > On Fri, Dec 14, 2018 at 02:50:20PM +0100, Linus Walleij wrote:
> > > > On Thu, Dec 13, 2018 at 9:42 PM Uwe Kleine-König
> > > > <[email protected]> wrote:
> > > > > [Adding Linus Walleij to Cc:]
> > > > > On Thu, Dec 13, 2018 at 03:03:15PM +0100, Paul Cercueil wrote:
> > > > > > Le jeu. 13 déc. 2018 à 10:24, Uwe Kleine-König
> > > > > > <[email protected]> a écrit :
> > > > > > > On Wed, Dec 12, 2018 at 11:09:10PM +0100, Paul Cercueil wrote:
> > > > > > > > The PWM in the JZ4725B works the same as in the JZ4740,
> > > > > > > > except that it only has 6 channels available instead of
> > > > > > > > 8.
> > > > > > >
> > > > > > > this driver is probed only from device tree? If yes, it
> > > > > > > might be sensible to specify the number of PWMs there and
> > > > > > > get it from there.
> > > > > > > There doesn't seem to be a generic binding for that, but there are
> > > > > > > several drivers that could benefit from it. (This is a bigger project
> > > > > > > though and shouldn't stop your patch. Still more as it already got
> > > > > > > Thierry's ack.)
> > > > > >
> > > > > > I think there needs to be a proper guideline, as there doesn't seem to be
> > > > > > a consensus about this. I learned from emails with Rob and Linus (Walleij)
> > > > > > that I should not have in devicetree what I can deduce from the compatible
> > > > > > string.
> > > > >
> > > > > I understood them a bit differently. It is ok to deduce things from the
> > > > > compatible string. But if you define a generic property (say) "num-pwms"
> > > > > that is used uniformly in most bindings this is ok, too. (And then the
> > > > > two different devices could use the same compatible.)
> > > > >
> > > > > An upside of the generic "num-pwms" property is that the pwm core could
> > > > > sanity check pwm phandles before passing them to the hardware drivers.
> > > >
> > > > I don't know if this helps, but in GPIO we have "ngpios" which is
> > > > used to augment an existing block as to the number of lines actually
> > > > used with it.
> > > >
> > > > The typical case is that an ASIC engineer synthesize a block for
> > > > 32 GPIOs but only 12 of them are routed to external pads. So
> > > > we augment the behaviour of that driver to only use 12 of the
> > > > 32 lines.
> > > >
> > > > I guess using the remaining 20 lines "works" in a sense but they
> > > > have no practical use and will just bias electrons in the silicon
> > > > for no use.
> > >
> > > This looks very similar to the case under discussion.
> > >
> > > > So if the PWM case is something similar, then by all means add
> > > > num-pwms.
> > >
> > > .. or "npwms" to use the same nomenclature as the gpio binding?
> >
> > If we're going to do something like this, should it be the drivers or
> > the core (within pwmchip_add) that checks for this "npwms" property?
>
> Of course this should be done in the core. The driver than can rely on
> the validity of the index. But as I wrote before, this shouldn't stop
> your patch from going in.
>
> But if Thierry agrees that this npmws (or num-pwms) is a good idea, it
> would be great to start early to convert drivers.
Do we actually need this? It seems like Paul's patch here properly
derives the number of available PWMs from the compatible string, so I
don't see what the extra num-pwms (or whatever) property would add.
Thierry
On Thu, Dec 20, 2018 at 06:39:04PM +0100, Thierry Reding wrote:
> On Mon, Dec 17, 2018 at 08:53:21AM +0100, Uwe Kleine-K?nig wrote:
> > On Sun, Dec 16, 2018 at 03:18:52PM +0100, Paul Cercueil wrote:
> > > Hi,
> > >
> > > Le ven. 14 d?c. 2018 ? 15:26, Uwe Kleine-K?nig
> > > <[email protected]> a ?crit :
> > > > Hello,
> > > >
> > > > On Fri, Dec 14, 2018 at 02:50:20PM +0100, Linus Walleij wrote:
> > > > > On Thu, Dec 13, 2018 at 9:42 PM Uwe Kleine-K?nig
> > > > > <[email protected]> wrote:
> > > > > > [Adding Linus Walleij to Cc:]
> > > > > > On Thu, Dec 13, 2018 at 03:03:15PM +0100, Paul Cercueil wrote:
> > > > > > > Le jeu. 13 d?c. 2018 ? 10:24, Uwe Kleine-K?nig
> > > > > > > <[email protected]> a ?crit :
> > > > > > > > On Wed, Dec 12, 2018 at 11:09:10PM +0100, Paul Cercueil wrote:
> > > > > > > > > The PWM in the JZ4725B works the same as in the JZ4740,
> > > > > > > > > except that it only has 6 channels available instead of
> > > > > > > > > 8.
> > > > > > > >
> > > > > > > > this driver is probed only from device tree? If yes, it
> > > > > > > > might be sensible to specify the number of PWMs there and
> > > > > > > > get it from there.
> > > > > > > > There doesn't seem to be a generic binding for that, but there are
> > > > > > > > several drivers that could benefit from it. (This is a bigger project
> > > > > > > > though and shouldn't stop your patch. Still more as it already got
> > > > > > > > Thierry's ack.)
> > > > > > >
> > > > > > > I think there needs to be a proper guideline, as there doesn't seem to be
> > > > > > > a consensus about this. I learned from emails with Rob and Linus (Walleij)
> > > > > > > that I should not have in devicetree what I can deduce from the compatible
> > > > > > > string.
> > > > > >
> > > > > > I understood them a bit differently. It is ok to deduce things from the
> > > > > > compatible string. But if you define a generic property (say) "num-pwms"
> > > > > > that is used uniformly in most bindings this is ok, too. (And then the
> > > > > > two different devices could use the same compatible.)
> > > > > >
> > > > > > An upside of the generic "num-pwms" property is that the pwm core could
> > > > > > sanity check pwm phandles before passing them to the hardware drivers.
> > > > >
> > > > > I don't know if this helps, but in GPIO we have "ngpios" which is
> > > > > used to augment an existing block as to the number of lines actually
> > > > > used with it.
> > > > >
> > > > > The typical case is that an ASIC engineer synthesize a block for
> > > > > 32 GPIOs but only 12 of them are routed to external pads. So
> > > > > we augment the behaviour of that driver to only use 12 of the
> > > > > 32 lines.
> > > > >
> > > > > I guess using the remaining 20 lines "works" in a sense but they
> > > > > have no practical use and will just bias electrons in the silicon
> > > > > for no use.
> > > >
> > > > This looks very similar to the case under discussion.
> > > >
> > > > > So if the PWM case is something similar, then by all means add
> > > > > num-pwms.
> > > >
> > > > .. or "npwms" to use the same nomenclature as the gpio binding?
> > >
> > > If we're going to do something like this, should it be the drivers or
> > > the core (within pwmchip_add) that checks for this "npwms" property?
> >
> > Of course this should be done in the core. The driver than can rely on
> > the validity of the index. But as I wrote before, this shouldn't stop
> > your patch from going in.
Actually the core already takes care of the validity of the index with
the number of pwms being provided to pwmchip_add().
> > But if Thierry agrees that this npmws (or num-pwms) is a good idea, it
> > would be great to start early to convert drivers.
>
> Do we actually need this? It seems like Paul's patch here properly
> derives the number of available PWMs from the compatible string, so I
> don't see what the extra num-pwms (or whatever) property would add.
Given that the only difference between the two different implementations
is just the number of pwms provided, this could just be expressed in the
dts as:
pwm@2000000 {
compatible = "jz4740";
num-pwms = <8>;
}
and
pwm@2000000 {
compatible = "jz4740";
num-pwms = <6>;
}
instead of
pwm@2000000 {
compatible = "jz4740";
}
and
pwm@2000000 {
compatible = "jz4725";
}
. According to my metric the former is more descriptive and so better.
And then the pwm core could provide parsing of that property (e.g. if
chip.npwm == 0) which simplifies the driver (and all others using that
mechanism).
Regarding the question "Do we actually need this?": We don't, but I
think it would make a nice step to get more descriptive device trees and
so I consider it an improvement. It would also allow to check a dtb
because even without the driver you can notice that
pwms = <&pwm 12 0>;
is invalid if &pwm has "num-pwms = <8>". Drivers that could benefit are
(at least): hibvt, sun4i, tegra, lpss.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Le mar. 18 d?c. 2018 ? 17:36, Rob Herring <[email protected]> a ?crit :
> On Mon, Dec 17, 2018 at 4:04 PM Paul Cercueil <[email protected]>
> wrote:
>>
>> Hi Rob,
>>
>> Le lun. 17 d?c. 2018 ? 22:05, Rob Herring <[email protected]> a
>> ?crit :
>> > On Wed, Dec 12, 2018 at 11:08:58PM +0100, Paul Cercueil wrote:
>> >> Add documentation about how to properly use the Ingenic TCU
>> >> (Timer/Counter Unit) drivers from devicetree.
>> >>
>> >> Signed-off-by: Paul Cercueil <[email protected]>
>> >> ---
>> >>
>> >> Notes:
>> >> v4: New patch in this series. Corresponds to V2 patches
>> 3-4-5
>> >> with
>> >> added content.
>> >>
>> >> v5: - Edited PWM/watchdog DT bindings documentation to
>> point
>> >> to the new
>> >> document.
>> >> - Moved main document to
>> >>
>> Documentation/devicetree/bindings/timer/ingenic,tcu.txt
>> >> - Updated documentation to reflect the new devicetree
>> >> bindings.
>> >>
>> >> v6: - Removed PWM/watchdog documentation files as asked by
>> >> upstream
>> >> - Removed doc about properties that should be implicit
>> >> - Removed doc about ingenic,timer-channel /
>> >> ingenic,clocksource-channel as they are gone
>> >> - Fix WDT clock name in the binding doc
>> >> - Fix lengths of register areas in watchdog/pwm nodes
>> >>
>> >> v7: No change
>> >>
>> >> v8: - Fix address of the PWM node
>> >> - Added doc about system timer and clocksource children
>> >> nodes
>> >
>> > I thought we'd sorted this out...
>>
>> Yeah, well I just can't please everybody. V6/V7 didn't have the
>> system timer or clocksource in devicetree, which was good for
>> you, but then the driver nearly doubled in size and complexity,
>> and Thierry rightfully refused it. Now I'm at the point where
>
> You mean Daniel?
Oops - I meant Daniel yes.
>> I'm trying alternative ways of encoding the information in
>> devicetree, as suggested by various people, just so that you
>> accept it. Because I don't see any other option.
>
> Does the problem boil down to needing to reserve channel x to use PWMx
> pin? If so, just do a mask property of reserved for PWM channels.
Yes, that's exactly the problem. I will go with a property then. Thanks!
> Sorry this is going in circles.
>
> Rob
On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil <[email protected]> wrote:
>
> From: Maarten ter Huurne <[email protected]>
>
> OST is the OS Timer, a 64-bit timer/counter with buffered reading.
>
> SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 and
> JZ4780 have a 64-bit OST.
>
> This driver will register both a clocksource and a sched_clock to the
> system.
>
> Signed-off-by: Maarten ter Huurne <[email protected]>
> Signed-off-by: Paul Cercueil <[email protected]>
> ---
>
> Notes:
> v5: New patch
>
> v6: - Get rid of SoC IDs; pass pointer to ingenic_ost_soc_info as
> devicetree match data instead.
> - Use device_get_match_data() instead of the of_* variant
> - Handle error of dev_get_regmap() properly
>
> v7: Fix section mismatch by using builtin_platform_driver_probe()
>
> v8: builtin_platform_driver_probe() does not work anymore in
> 4.20-rc6? The probe function won't be called. Work around this
> for now by using late_initcall.
>
> drivers/clocksource/Kconfig | 8 ++
> drivers/clocksource/Makefile | 1 +
> drivers/clocksource/ingenic-ost.c | 215 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 224 insertions(+)
> create mode 100644 drivers/clocksource/ingenic-ost.c
>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 4e69af15c3e7..e0646878b0de 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -648,4 +648,12 @@ config INGENIC_TIMER
> help
> Support for the timer/counter unit of the Ingenic JZ SoCs.
>
> +config INGENIC_OST
> + bool "Ingenic JZ47xx Operating System Timer"
> + depends on MIPS || COMPILE_TEST
> + depends on COMMON_CLK
> + select INGENIC_TIMER
> + help
> + Support for the OS Timer of the Ingenic JZ4770 or similar SoC.
> +
> endmenu
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 7c8f790dcf67..7faa8108574a 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -75,6 +75,7 @@ obj-$(CONFIG_ASM9260_TIMER) += asm9260_timer.o
> obj-$(CONFIG_H8300_TMR8) += h8300_timer8.o
> obj-$(CONFIG_H8300_TMR16) += h8300_timer16.o
> obj-$(CONFIG_H8300_TPU) += h8300_tpu.o
> +obj-$(CONFIG_INGENIC_OST) += ingenic-ost.o
> obj-$(CONFIG_INGENIC_TIMER) += ingenic-timer.o
> obj-$(CONFIG_CLKSRC_ST_LPC) += clksrc_st_lpc.o
> obj-$(CONFIG_X86_NUMACHIP) += numachip.o
> diff --git a/drivers/clocksource/ingenic-ost.c b/drivers/clocksource/ingenic-ost.c
> new file mode 100644
> index 000000000000..cc0fee3efd29
> --- /dev/null
> +++ b/drivers/clocksource/ingenic-ost.c
> @@ -0,0 +1,215 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * JZ47xx SoCs TCU Operating System Timer driver
> + *
> + * Copyright (C) 2016 Maarten ter Huurne <[email protected]>
> + * Copyright (C) 2018 Paul Cercueil <[email protected]>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clocksource.h>
> +#include <linux/mfd/ingenic-tcu.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/regmap.h>
> +#include <linux/sched_clock.h>
> +
> +#include "ingenic-timer.h"
> +
> +#define TCU_OST_TCSR_MASK 0xc0
> +#define TCU_OST_TCSR_CNT_MD BIT(15)
> +
> +#define TCU_OST_CHANNEL 15
> +
> +struct ingenic_ost_soc_info {
> + bool is64bit;
> +};
> +
> +struct ingenic_ost {
> + struct regmap *map;
> + struct clk *clk;
> +
> + struct clocksource cs;
> +};
> +
> +static u64 notrace ingenic_ost_read_cntl(void)
> +{
> + /* Bypass the regmap here as we must return as soon as possible */
> + return readl(ingenic_tcu_base + TCU_REG_OST_CNTL);
> +}
> +
> +static u64 notrace ingenic_ost_read_cnth(void)
> +{
> + /* Bypass the regmap here as we must return as soon as possible */
> + return readl(ingenic_tcu_base + TCU_REG_OST_CNTH);
> +}
> +
> +static u64 notrace ingenic_ost_clocksource_read(struct clocksource *cs)
> +{
> + u32 val1, val2;
> + u64 count, recount;
> + s64 diff;
> +
> + /*
> + * The buffering of the upper 32 bits of the timer prevents wrong
> + * results from the bottom 32 bits overflowing due to the timer ticking
> + * along. However, it does not prevent wrong results from simultaneous
> + * reads of the timer, which could reset the buffer mid-read.
> + * Since this kind of wrong read can happen only when the bottom bits
> + * overflow, there will be minutes between wrong reads, so if we read
> + * twice in succession, at least one of the reads will be correct.
> + */
> +
> + /* Bypass the regmap here as we must return as soon as possible */
> + val1 = readl(ingenic_tcu_base + TCU_REG_OST_CNTL);
> + val2 = readl(ingenic_tcu_base + TCU_REG_OST_CNTHBUF);
> + count = (u64)val1 | (u64)val2 << 32;
> +
> + val1 = readl(ingenic_tcu_base + TCU_REG_OST_CNTL);
> + val2 = readl(ingenic_tcu_base + TCU_REG_OST_CNTHBUF);
> + recount = (u64)val1 | (u64)val2 << 32;
> +
> + /*
> + * A wrong read will produce a result that is 1<<32 too high: the bottom
> + * part from before overflow and the upper part from after overflow.
> + * Therefore, the lower value of the two reads is the correct value.
> + */
> +
> + diff = (s64)(recount - count);
> + if (unlikely(diff < 0))
> + count = recount;
> +
> + return count;
> +}
> +
> +static int __init ingenic_ost_probe(struct platform_device *pdev)
> +{
> + const struct ingenic_ost_soc_info *soc_info;
> + struct device *dev = &pdev->dev;
> + struct ingenic_ost *ost;
> + struct clocksource *cs;
> + unsigned long rate, flags;
> + int err;
> +
> + soc_info = device_get_match_data(dev);
> + if (!soc_info)
> + return -EINVAL;
> +
> + ost = devm_kzalloc(dev, sizeof(*ost), GFP_KERNEL);
> + if (!ost)
> + return -ENOMEM;
> +
> + ost->map = dev_get_regmap(dev->parent, NULL);
> + if (!ost->map) {
> + dev_err(dev, "regmap not found\n");
> + return -EINVAL;
> + }
> +
> + ost->clk = devm_clk_get(dev, "ost");
> + if (IS_ERR(ost->clk))
> + return PTR_ERR(ost->clk);
> +
> + err = clk_prepare_enable(ost->clk);
> + if (err)
> + return err;
> +
> + /* Clear counter high/low registers */
> + if (soc_info->is64bit)
> + regmap_write(ost->map, TCU_REG_OST_CNTL, 0);
> + regmap_write(ost->map, TCU_REG_OST_CNTH, 0);
> +
> + /* Don't reset counter at compare value. */
> + regmap_update_bits(ost->map, TCU_REG_OST_TCSR,
> + TCU_OST_TCSR_MASK, TCU_OST_TCSR_CNT_MD);
> +
> + rate = clk_get_rate(ost->clk);
> +
> + /* Enable OST TCU channel */
> + regmap_write(ost->map, TCU_REG_TESR, BIT(TCU_OST_CHANNEL));
> +
> + cs = &ost->cs;
> + cs->name = "ingenic-ost";
> + cs->rating = 320;
> + cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
> +
> + if (soc_info->is64bit) {
> + cs->mask = CLOCKSOURCE_MASK(64);
> + cs->read = ingenic_ost_clocksource_read;
> + } else {
> + cs->mask = CLOCKSOURCE_MASK(32);
> + cs->read = (u64 (*)(struct clocksource *))ingenic_ost_read_cnth;
The function is declared as ingenic_ost_read_cnth(void), are you sure
this is going to work ?
> + }
> +
> + err = clocksource_register_hz(cs, rate);
> + if (err) {
> + dev_err(dev, "clocksource registration failed: %d\n", err);
> + clk_disable_unprepare(ost->clk);
> + return err;
> + }
> +
> + /* Cannot register a sched_clock with interrupts on */
> + local_irq_save(flags);
> + if (soc_info->is64bit)
> + sched_clock_register(ingenic_ost_read_cntl, 32, rate);
> + else
> + sched_clock_register(ingenic_ost_read_cnth, 32, rate);
> + local_irq_restore(flags);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int ingenic_ost_suspend(struct device *dev)
> +{
> + struct ingenic_ost *ost = dev_get_drvdata(dev);
> +
> + clk_disable(ost->clk);
> + return 0;
> +}
> +
> +static int ingenic_ost_resume(struct device *dev)
> +{
> + struct ingenic_ost *ost = dev_get_drvdata(dev);
> +
> + return clk_enable(ost->clk);
> +}
> +
> +static SIMPLE_DEV_PM_OPS(ingenic_ost_pm_ops, ingenic_ost_suspend,
> + ingenic_ost_resume);
> +#define INGENIC_OST_PM_OPS (&ingenic_ost_pm_ops)
> +#else
> +#define INGENIC_OST_PM_OPS NULL
> +#endif /* CONFIG_PM_SUSPEND */
> +
> +static const struct ingenic_ost_soc_info jz4725b_ost_soc_info = {
> + .is64bit = false,
> +};
> +
> +static const struct ingenic_ost_soc_info jz4770_ost_soc_info = {
> + .is64bit = true,
> +};
> +
> +static const struct of_device_id ingenic_ost_of_match[] = {
> + { .compatible = "ingenic,jz4725b-ost", .data = &jz4725b_ost_soc_info, },
> + { .compatible = "ingenic,jz4770-ost", .data = &jz4770_ost_soc_info, },
> + { }
> +};
> +
> +static struct platform_driver ingenic_ost_driver = {
> + .driver = {
> + .name = "ingenic-ost",
> + .pm = INGENIC_OST_PM_OPS,
> + .of_match_table = ingenic_ost_of_match,
> + },
> +};
> +
> +/* FIXME: Using device_initcall (or buildin_platform_driver_probe) results
> + * in the driver not being probed at all. It worked in 4.18...
> + */
> +static int __init ingenic_ost_drv_register(void)
> +{
> + return platform_driver_probe(&ingenic_ost_driver,
> + ingenic_ost_probe);
> +}
> +late_initcall(ingenic_ost_drv_register);
> --
> 2.11.0
>
On 1/23/19 4:58 AM, Mathieu Malaterre wrote:
> On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil <[email protected]> wrote:
>>
>> From: Maarten ter Huurne <[email protected]>
>>
>> OST is the OS Timer, a 64-bit timer/counter with buffered reading.
>>
>> SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 and
>> JZ4780 have a 64-bit OST.
>>
>> This driver will register both a clocksource and a sched_clock to the
>> system.
>>
>> Signed-off-by: Maarten ter Huurne <[email protected]>
>> Signed-off-by: Paul Cercueil <[email protected]>
>> ---
>>
>> Notes:
>> v5: New patch
>>
>> v6: - Get rid of SoC IDs; pass pointer to ingenic_ost_soc_info as
>> devicetree match data instead.
>> - Use device_get_match_data() instead of the of_* variant
>> - Handle error of dev_get_regmap() properly
>>
>> v7: Fix section mismatch by using builtin_platform_driver_probe()
>>
>> v8: builtin_platform_driver_probe() does not work anymore in
>> 4.20-rc6? The probe function won't be called. Work around this
>> for now by using late_initcall.
>>
Did anyone notice this ? Either something is wrong with the driver, or
with the kernel core. Hacking around it seems like the worst possible
"solution".
Guenter
Hi,
Le mer. 23 janv. 2019 ? 11:31, Guenter Roeck <[email protected]> a
?crit :
> On 1/23/19 4:58 AM, Mathieu Malaterre wrote:
>> On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil
>> <[email protected]> wrote:
>>>
>>> From: Maarten ter Huurne <[email protected]>
>>>
>>> OST is the OS Timer, a 64-bit timer/counter with buffered reading.
>>>
>>> SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 and
>>> JZ4780 have a 64-bit OST.
>>>
>>> This driver will register both a clocksource and a sched_clock to
>>> the
>>> system.
>>>
>>> Signed-off-by: Maarten ter Huurne <[email protected]>
>>> Signed-off-by: Paul Cercueil <[email protected]>
>>> ---
>>>
>>> Notes:
>>> v5: New patch
>>>
>>> v6: - Get rid of SoC IDs; pass pointer to
>>> ingenic_ost_soc_info as
>>> devicetree match data instead.
>>> - Use device_get_match_data() instead of the of_* variant
>>> - Handle error of dev_get_regmap() properly
>>>
>>> v7: Fix section mismatch by using
>>> builtin_platform_driver_probe()
>>>
>>> v8: builtin_platform_driver_probe() does not work anymore in
>>> 4.20-rc6? The probe function won't be called. Work around
>>> this
>>> for now by using late_initcall.
>>>
>
> Did anyone notice this ? Either something is wrong with the driver, or
> with the kernel core. Hacking around it seems like the worst possible
> "solution".
I can confirm it still happens on 5.0-rc3.
Just to explain what I'm doing:
My ingenic-timer driver probes with builtin_platform_driver_probe (this
works),
and then calls of_platform_populate to probe its children. This driver,
ingenic-ost, is one of them, and will fail to probe with
builtin_platform_driver_probe.
-Paul
(re-send, in plain text this time, sorry for those who got it twice)
Le mer. 23 janv. 2019 ? 9:58, Mathieu Malaterre <[email protected]> a
?crit :
> On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil <[email protected]>
> wrote:
>>
>> From: Maarten ter Huurne <[email protected]>
>>
>> OST is the OS Timer, a 64-bit timer/counter with buffered reading.
>>
>> SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 and
>> JZ4780 have a 64-bit OST.
>>
>> This driver will register both a clocksource and a sched_clock to
>> the
>> system.
>>
>> Signed-off-by: Maarten ter Huurne <[email protected]>
>> Signed-off-by: Paul Cercueil <[email protected]>
>> ---
>>
>> Notes:
>> v5: New patch
>>
>> v6: - Get rid of SoC IDs; pass pointer to ingenic_ost_soc_info
>> as
>> devicetree match data instead.
>> - Use device_get_match_data() instead of the of_* variant
>> - Handle error of dev_get_regmap() properly
>>
>> v7: Fix section mismatch by using
>> builtin_platform_driver_probe()
>>
>> v8: builtin_platform_driver_probe() does not work anymore in
>> 4.20-rc6? The probe function won't be called. Work around
>> this
>> for now by using late_initcall.
>>
>> drivers/clocksource/Kconfig | 8 ++
>> drivers/clocksource/Makefile | 1 +
>> drivers/clocksource/ingenic-ost.c | 215
>> ++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 224 insertions(+)
>> create mode 100644 drivers/clocksource/ingenic-ost.c
>>
>> diff --git a/drivers/clocksource/Kconfig
>> b/drivers/clocksource/Kconfig
>> index 4e69af15c3e7..e0646878b0de 100644
>> --- a/drivers/clocksource/Kconfig
>> +++ b/drivers/clocksource/Kconfig
>> @@ -648,4 +648,12 @@ config INGENIC_TIMER
>> help
>> Support for the timer/counter unit of the Ingenic JZ SoCs.
>>
>> +config INGENIC_OST
>> + bool "Ingenic JZ47xx Operating System Timer"
>> + depends on MIPS || COMPILE_TEST
>> + depends on COMMON_CLK
>> + select INGENIC_TIMER
>> + help
>> + Support for the OS Timer of the Ingenic JZ4770 or similar
>> SoC.
>> +
>> endmenu
>> diff --git a/drivers/clocksource/Makefile
>> b/drivers/clocksource/Makefile
>> index 7c8f790dcf67..7faa8108574a 100644
>> --- a/drivers/clocksource/Makefile
>> +++ b/drivers/clocksource/Makefile
>> @@ -75,6 +75,7 @@ obj-$(CONFIG_ASM9260_TIMER) +=
>> asm9260_timer.o
>> obj-$(CONFIG_H8300_TMR8) += h8300_timer8.o
>> obj-$(CONFIG_H8300_TMR16) += h8300_timer16.o
>> obj-$(CONFIG_H8300_TPU) += h8300_tpu.o
>> +obj-$(CONFIG_INGENIC_OST) += ingenic-ost.o
>> obj-$(CONFIG_INGENIC_TIMER) += ingenic-timer.o
>> obj-$(CONFIG_CLKSRC_ST_LPC) += clksrc_st_lpc.o
>> obj-$(CONFIG_X86_NUMACHIP) += numachip.o
>> diff --git a/drivers/clocksource/ingenic-ost.c
>> b/drivers/clocksource/ingenic-ost.c
>> new file mode 100644
>> index 000000000000..cc0fee3efd29
>> --- /dev/null
>> +++ b/drivers/clocksource/ingenic-ost.c
>> @@ -0,0 +1,215 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * JZ47xx SoCs TCU Operating System Timer driver
>> + *
>> + * Copyright (C) 2016 Maarten ter Huurne <[email protected]>
>> + * Copyright (C) 2018 Paul Cercueil <[email protected]>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clocksource.h>
>> +#include <linux/mfd/ingenic-tcu.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm.h>
>> +#include <linux/regmap.h>
>> +#include <linux/sched_clock.h>
>> +
>> +#include "ingenic-timer.h"
>> +
>> +#define TCU_OST_TCSR_MASK 0xc0
>> +#define TCU_OST_TCSR_CNT_MD BIT(15)
>> +
>> +#define TCU_OST_CHANNEL 15
>> +
>> +struct ingenic_ost_soc_info {
>> + bool is64bit;
>> +};
>> +
>> +struct ingenic_ost {
>> + struct regmap *map;
>> + struct clk *clk;
>> +
>> + struct clocksource cs;
>> +};
>> +
>> +static u64 notrace ingenic_ost_read_cntl(void)
>> +{
>> + /* Bypass the regmap here as we must return as soon as
>> possible */
>> + return readl(ingenic_tcu_base + TCU_REG_OST_CNTL);
>> +}
>> +
>> +static u64 notrace ingenic_ost_read_cnth(void)
>> +{
>> + /* Bypass the regmap here as we must return as soon as
>> possible */
>> + return readl(ingenic_tcu_base + TCU_REG_OST_CNTH);
>> +}
>> +
>> +static u64 notrace ingenic_ost_clocksource_read(struct clocksource
>> *cs)
>> +{
>> + u32 val1, val2;
>> + u64 count, recount;
>> + s64 diff;
>> +
>> + /*
>> + * The buffering of the upper 32 bits of the timer prevents
>> wrong
>> + * results from the bottom 32 bits overflowing due to the
>> timer ticking
>> + * along. However, it does not prevent wrong results from
>> simultaneous
>> + * reads of the timer, which could reset the buffer
>> mid-read.
>> + * Since this kind of wrong read can happen only when the
>> bottom bits
>> + * overflow, there will be minutes between wrong reads, so
>> if we read
>> + * twice in succession, at least one of the reads will be
>> correct.
>> + */
>> +
>> + /* Bypass the regmap here as we must return as soon as
>> possible */
>> + val1 = readl(ingenic_tcu_base + TCU_REG_OST_CNTL);
>> + val2 = readl(ingenic_tcu_base + TCU_REG_OST_CNTHBUF);
>> + count = (u64)val1 | (u64)val2 << 32;
>> +
>> + val1 = readl(ingenic_tcu_base + TCU_REG_OST_CNTL);
>> + val2 = readl(ingenic_tcu_base + TCU_REG_OST_CNTHBUF);
>> + recount = (u64)val1 | (u64)val2 << 32;
>> +
>> + /*
>> + * A wrong read will produce a result that is 1<<32 too
>> high: the bottom
>> + * part from before overflow and the upper part from after
>> overflow.
>> + * Therefore, the lower value of the two reads is the
>> correct value.
>> + */
>> +
>> + diff = (s64)(recount - count);
>> + if (unlikely(diff < 0))
>> + count = recount;
>> +
>> + return count;
>> +}
>> +
>> +static int __init ingenic_ost_probe(struct platform_device *pdev)
>> +{
>> + const struct ingenic_ost_soc_info *soc_info;
>> + struct device *dev = &pdev->dev;
>> + struct ingenic_ost *ost;
>> + struct clocksource *cs;
>> + unsigned long rate, flags;
>> + int err;
>> +
>> + soc_info = device_get_match_data(dev);
>> + if (!soc_info)
>> + return -EINVAL;
>> +
>> + ost = devm_kzalloc(dev, sizeof(*ost), GFP_KERNEL);
>> + if (!ost)
>> + return -ENOMEM;
>> +
>> + ost->map = dev_get_regmap(dev->parent, NULL);
>> + if (!ost->map) {
>> + dev_err(dev, "regmap not found\n");
>> + return -EINVAL;
>> + }
>> +
>> + ost->clk = devm_clk_get(dev, "ost");
>> + if (IS_ERR(ost->clk))
>> + return PTR_ERR(ost->clk);
>> +
>> + err = clk_prepare_enable(ost->clk);
>> + if (err)
>> + return err;
>> +
>> + /* Clear counter high/low registers */
>> + if (soc_info->is64bit)
>> + regmap_write(ost->map, TCU_REG_OST_CNTL, 0);
>> + regmap_write(ost->map, TCU_REG_OST_CNTH, 0);
>> +
>> + /* Don't reset counter at compare value. */
>> + regmap_update_bits(ost->map, TCU_REG_OST_TCSR,
>> + TCU_OST_TCSR_MASK, TCU_OST_TCSR_CNT_MD);
>> +
>> + rate = clk_get_rate(ost->clk);
>> +
>> + /* Enable OST TCU channel */
>> + regmap_write(ost->map, TCU_REG_TESR, BIT(TCU_OST_CHANNEL));
>> +
>> + cs = &ost->cs;
>> + cs->name = "ingenic-ost";
>> + cs->rating = 320;
>> + cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
>> +
>> + if (soc_info->is64bit) {
>> + cs->mask = CLOCKSOURCE_MASK(64);
>> + cs->read = ingenic_ost_clocksource_read;
>> + } else {
>> + cs->mask = CLOCKSOURCE_MASK(32);
>> + cs->read = (u64 (*)(struct clocksource
>> *))ingenic_ost_read_cnth;
>
> The function is declared as ingenic_ost_read_cnth(void), are you sure
> this is going to work ?
Hence the cast. The clocksource pointer passed as the first argument
will
simply be ignored by the function. This works fine.
>> + }
>> +
>> + err = clocksource_register_hz(cs, rate);
>> + if (err) {
>> + dev_err(dev, "clocksource registration failed:
>> %d\n", err);
>> + clk_disable_unprepare(ost->clk);
>> + return err;
>> + }
>> +
>> + /* Cannot register a sched_clock with interrupts on */
>> + local_irq_save(flags);
>> + if (soc_info->is64bit)
>> + sched_clock_register(ingenic_ost_read_cntl, 32,
>> rate);
>> + else
>> + sched_clock_register(ingenic_ost_read_cnth, 32,
>> rate);
>> + local_irq_restore(flags);
>> +
>> + return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int ingenic_ost_suspend(struct device *dev)
>> +{
>> + struct ingenic_ost *ost = dev_get_drvdata(dev);
>> +
>> + clk_disable(ost->clk);
>> + return 0;
>> +}
>> +
>> +static int ingenic_ost_resume(struct device *dev)
>> +{
>> + struct ingenic_ost *ost = dev_get_drvdata(dev);
>> +
>> + return clk_enable(ost->clk);
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(ingenic_ost_pm_ops, ingenic_ost_suspend,
>> + ingenic_ost_resume);
>> +#define INGENIC_OST_PM_OPS (&ingenic_ost_pm_ops)
>> +#else
>> +#define INGENIC_OST_PM_OPS NULL
>> +#endif /* CONFIG_PM_SUSPEND */
>> +
>> +static const struct ingenic_ost_soc_info jz4725b_ost_soc_info = {
>> + .is64bit = false,
>> +};
>> +
>> +static const struct ingenic_ost_soc_info jz4770_ost_soc_info = {
>> + .is64bit = true,
>> +};
>> +
>> +static const struct of_device_id ingenic_ost_of_match[] = {
>> + { .compatible = "ingenic,jz4725b-ost", .data =
>> &jz4725b_ost_soc_info, },
>> + { .compatible = "ingenic,jz4770-ost", .data =
>> &jz4770_ost_soc_info, },
>> + { }
>> +};
>> +
>> +static struct platform_driver ingenic_ost_driver = {
>> + .driver = {
>> + .name = "ingenic-ost",
>> + .pm = INGENIC_OST_PM_OPS,
>> + .of_match_table = ingenic_ost_of_match,
>> + },
>> +};
>> +
>> +/* FIXME: Using device_initcall (or buildin_platform_driver_probe)
>> results
>> + * in the driver not being probed at all. It worked in 4.18...
>> + */
>> +static int __init ingenic_ost_drv_register(void)
>> +{
>> + return platform_driver_probe(&ingenic_ost_driver,
>> + ingenic_ost_probe);
>> +}
>> +late_initcall(ingenic_ost_drv_register);
>> --
>> 2.11.0
>>
On Wed, Jan 23, 2019 at 02:25:53PM -0300, Paul Cercueil wrote:
> Hi,
>
> Le mer. 23 janv. 2019 ? 11:31, Guenter Roeck <[email protected]> a ?crit :
> >On 1/23/19 4:58 AM, Mathieu Malaterre wrote:
> >>On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil <[email protected]>
> >>wrote:
> >>>
> >>>From: Maarten ter Huurne <[email protected]>
> >>>
> >>>OST is the OS Timer, a 64-bit timer/counter with buffered reading.
> >>>
> >>>SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 and
> >>>JZ4780 have a 64-bit OST.
> >>>
> >>>This driver will register both a clocksource and a sched_clock to the
> >>>system.
> >>>
> >>>Signed-off-by: Maarten ter Huurne <[email protected]>
> >>>Signed-off-by: Paul Cercueil <[email protected]>
> >>>---
> >>>
> >>>Notes:
> >>> v5: New patch
> >>>
> >>> v6: - Get rid of SoC IDs; pass pointer to ingenic_ost_soc_info
> >>>as
> >>> devicetree match data instead.
> >>> - Use device_get_match_data() instead of the of_* variant
> >>> - Handle error of dev_get_regmap() properly
> >>>
> >>> v7: Fix section mismatch by using
> >>>builtin_platform_driver_probe()
> >>>
> >>> v8: builtin_platform_driver_probe() does not work anymore in
> >>> 4.20-rc6? The probe function won't be called. Work around
> >>>this
> >>> for now by using late_initcall.
> >>>
> >
> >Did anyone notice this ? Either something is wrong with the driver, or
> >with the kernel core. Hacking around it seems like the worst possible
> >"solution".
>
> I can confirm it still happens on 5.0-rc3.
>
> Just to explain what I'm doing:
>
> My ingenic-timer driver probes with builtin_platform_driver_probe (this
> works),
> and then calls of_platform_populate to probe its children. This driver,
> ingenic-ost, is one of them, and will fail to probe with
> builtin_platform_driver_probe.
>
The big question is _why_ it fails to probe.
Guenter
Quoting Guenter Roeck (2019-01-23 10:01:55)
> On Wed, Jan 23, 2019 at 02:25:53PM -0300, Paul Cercueil wrote:
> > Hi,
> >
> > Le mer. 23 janv. 2019 à 11:31, Guenter Roeck <[email protected]> a écrit :
> > >On 1/23/19 4:58 AM, Mathieu Malaterre wrote:
> > >>On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil <[email protected]>
> > >>wrote:
> > >>>
> > >>>From: Maarten ter Huurne <[email protected]>
> > >>>
> > >>>OST is the OS Timer, a 64-bit timer/counter with buffered reading.
> > >>>
> > >>>SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 and
> > >>>JZ4780 have a 64-bit OST.
> > >>>
> > >>>This driver will register both a clocksource and a sched_clock to the
> > >>>system.
> > >>>
> > >>>Signed-off-by: Maarten ter Huurne <[email protected]>
> > >>>Signed-off-by: Paul Cercueil <[email protected]>
> > >>>---
> > >>>
> > >>>Notes:
> > >>> v5: New patch
> > >>>
> > >>> v6: - Get rid of SoC IDs; pass pointer to ingenic_ost_soc_info
> > >>>as
> > >>> devicetree match data instead.
> > >>> - Use device_get_match_data() instead of the of_* variant
> > >>> - Handle error of dev_get_regmap() properly
> > >>>
> > >>> v7: Fix section mismatch by using
> > >>>builtin_platform_driver_probe()
> > >>>
> > >>> v8: builtin_platform_driver_probe() does not work anymore in
> > >>> 4.20-rc6? The probe function won't be called. Work around
> > >>>this
> > >>> for now by using late_initcall.
> > >>>
> > >
> > >Did anyone notice this ? Either something is wrong with the driver, or
> > >with the kernel core. Hacking around it seems like the worst possible
> > >"solution".
> >
> > I can confirm it still happens on 5.0-rc3.
> >
> > Just to explain what I'm doing:
> >
> > My ingenic-timer driver probes with builtin_platform_driver_probe (this
> > works),
> > and then calls of_platform_populate to probe its children. This driver,
> > ingenic-ost, is one of them, and will fail to probe with
> > builtin_platform_driver_probe.
> >
>
> The big question is _why_ it fails to probe.
>
Are you sharing the device tree node between a 'normal' platform device
driver and something more low level DT that marks the device's backing
DT node as OF_POPULATED early on? That's my only guess why it's not
working.
Le jeu. 24 janv. 2019 ? 16:28, Stephen Boyd <[email protected]> a
?crit :
> Quoting Guenter Roeck (2019-01-23 10:01:55)
>> On Wed, Jan 23, 2019 at 02:25:53PM -0300, Paul Cercueil wrote:
>> > Hi,
>> >
>> > Le mer. 23 janv. 2019 ? 11:31, Guenter Roeck
>> <[email protected]> a écrit :
>> > >On 1/23/19 4:58 AM, Mathieu Malaterre wrote:
>> > >>On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil
>> <[email protected]>
>> > >>wrote:
>> > >>>
>> > >>>From: Maarten ter Huurne <[email protected]>
>> > >>>
>> > >>>OST is the OS Timer, a 64-bit timer/counter with buffered
>> reading.
>> > >>>
>> > >>>SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770
>> and
>> > >>>JZ4780 have a 64-bit OST.
>> > >>>
>> > >>>This driver will register both a clocksource and a sched_clock
>> to the
>> > >>>system.
>> > >>>
>> > >>>Signed-off-by: Maarten ter Huurne <[email protected]>
>> > >>>Signed-off-by: Paul Cercueil <[email protected]>
>> > >>>---
>> > >>>
>> > >>>Notes:
>> > >>> v5: New patch
>> > >>>
>> > >>> v6: - Get rid of SoC IDs; pass pointer to
>> ingenic_ost_soc_info
>> > >>>as
>> > >>> devicetree match data instead.
>> > >>> - Use device_get_match_data() instead of the of_*
>> variant
>> > >>> - Handle error of dev_get_regmap() properly
>> > >>>
>> > >>> v7: Fix section mismatch by using
>> > >>>builtin_platform_driver_probe()
>> > >>>
>> > >>> v8: builtin_platform_driver_probe() does not work
>> anymore in
>> > >>> 4.20-rc6? The probe function won't be called. Work
>> around
>> > >>>this
>> > >>> for now by using late_initcall.
>> > >>>
>> > >
>> > >Did anyone notice this ? Either something is wrong with the
>> driver, or
>> > >with the kernel core. Hacking around it seems like the worst
>> possible
>> > >"solution".
>> >
>> > I can confirm it still happens on 5.0-rc3.
>> >
>> > Just to explain what I'm doing:
>> >
>> > My ingenic-timer driver probes with builtin_platform_driver_probe
>> (this
>> > works),
>> > and then calls of_platform_populate to probe its children. This
>> driver,
>> > ingenic-ost, is one of them, and will fail to probe with
>> > builtin_platform_driver_probe.
>> >
>>
>> The big question is _why_ it fails to probe.
>>
>
> Are you sharing the device tree node between a 'normal' platform
> device
> driver and something more low level DT that marks the device's backing
> DT node as OF_POPULATED early on? That's my only guess why it's not
> working.
I do, but I clear the OF_POPULATED flag so that it is then probed as a
normal platform device, and it's not on this driver's node but its
parent.
Paul,
On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil <[email protected]> wrote:
>
> Hi,
>
> Here's the version 8 and hopefully final version of my patchset, which
> adds support for the Timer/Counter Unit found in JZ47xx SoCs from
> Ingenic.
I can no longer boot my MIPS Creator CI20 with this series (merged
opendingux/for-upstream-timer-v8).
Using screen+ttyUSB, I can see messages stopping at:
...
[ OK ] Started Cgroup management daemon.
Starting Regular background program processing daemon...
[ OK ] Started Regular background program processing daemon.
Starting System Logging Service...
Starting Provide limited super user privileges to specific users...
Starting Restore /etc/resolv.conf if the system cras...s shut down....
Starting WPA supplicant...
Starting D-Bus System Message Bus...
[ OK ] Started D-Bus System Message Bus.
Nothing really stands out in the error messages. Could you suggest
things to try out to get into a bootable state ?
> The big change is that the timer driver has been simplified. The code to
> dynamically update the system timer or clocksource to a new channel has
> been removed. Now, the system timer and clocksource are provided as
> children nodes in the devicetree, and the TCU channel to use for these
> is deduced from their respective memory resource. The PWM driver will
> also deduce from its memory resources whether a given PWM channel can be
> used, or is reserved for the system timers.
>
> Kind regards,
> - Paul Cercueil
>
Hi Mathieu,
Le jeu. 24 janv. 2019 ? 18:26, Mathieu Malaterre <[email protected]> a
?crit :
> Paul,
>
> On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil <[email protected]>
> wrote:
>>
>> Hi,
>>
>> Here's the version 8 and hopefully final version of my patchset,
>> which
>> adds support for the Timer/Counter Unit found in JZ47xx SoCs from
>> Ingenic.
>
> I can no longer boot my MIPS Creator CI20 with this series (merged
> opendingux/for-upstream-timer-v8).
>
> Using screen+ttyUSB, I can see messages stopping at:
>
> ...
> [ OK ] Started Cgroup management daemon.
> Starting Regular background program processing daemon...
> [ OK ] Started Regular background program processing daemon.
> Starting System Logging Service...
> Starting Provide limited super user privileges to specific
> users...
> Starting Restore /etc/resolv.conf if the system cras...s
> shut down....
> Starting WPA supplicant...
> Starting D-Bus System Message Bus...
> [ OK ] Started D-Bus System Message Bus.
>
> Nothing really stands out in the error messages. Could you suggest
> things to try out to get into a bootable state ?
I'm debugging it right now on jz4740, it seems to happen when the
clocksource
from the ingenic-timer driver is used. Is it your case? It should not
happen
if you have CONFIG_INGENIC_OST set.
>> The big change is that the timer driver has been simplified. The
>> code to
>> dynamically update the system timer or clocksource to a new channel
>> has
>> been removed. Now, the system timer and clocksource are provided as
>> children nodes in the devicetree, and the TCU channel to use for
>> these
>> is deduced from their respective memory resource. The PWM driver
>> will
>> also deduce from its memory resources whether a given PWM channel
>> can be
>> used, or is reserved for the system timers.
>>
>> Kind regards,
>> - Paul Cercueil
>>
Quoting Paul Cercueil (2019-01-24 12:46:28)
>
>
> Le jeu. 24 janv. 2019 à 16:28, Stephen Boyd <[email protected]> a
> écrit :
> > Quoting Guenter Roeck (2019-01-23 10:01:55)
> >> On Wed, Jan 23, 2019 at 02:25:53PM -0300, Paul Cercueil wrote:
> >> > Hi,
> >> >
> >> > Le mer. 23 janv. 2019 Ã 11:31, Guenter Roeck
> >> <[email protected]> a écrit :
> >> > >On 1/23/19 4:58 AM, Mathieu Malaterre wrote:
> >> > >>On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil
> >> <[email protected]>
> >> > >>wrote:
> >> > >>>
> >> > >>>From: Maarten ter Huurne <[email protected]>
> >> > >>>
> >> > >>>OST is the OS Timer, a 64-bit timer/counter with buffered
> >> reading.
> >> > >>>
> >> > >>>SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770
> >> and
> >> > >>>JZ4780 have a 64-bit OST.
> >> > >>>
> >> > >>>This driver will register both a clocksource and a sched_clock
> >> to the
> >> > >>>system.
> >> > >>>
> >> > >>>Signed-off-by: Maarten ter Huurne <[email protected]>
> >> > >>>Signed-off-by: Paul Cercueil <[email protected]>
> >> > >>>---
> >> > >>>
> >> > >>>Notes:
> >> > >>> v5: New patch
> >> > >>>
> >> > >>> v6: - Get rid of SoC IDs; pass pointer to
> >> ingenic_ost_soc_info
> >> > >>>as
> >> > >>> devicetree match data instead.
> >> > >>> - Use device_get_match_data() instead of the of_*
> >> variant
> >> > >>> - Handle error of dev_get_regmap() properly
> >> > >>>
> >> > >>> v7: Fix section mismatch by using
> >> > >>>builtin_platform_driver_probe()
> >> > >>>
> >> > >>> v8: builtin_platform_driver_probe() does not work
> >> anymore in
> >> > >>> 4.20-rc6? The probe function won't be called. Work
> >> around
> >> > >>>this
> >> > >>> for now by using late_initcall.
> >> > >>>
> >> > >
> >> > >Did anyone notice this ? Either something is wrong with the
> >> driver, or
> >> > >with the kernel core. Hacking around it seems like the worst
> >> possible
> >> > >"solution".
> >> >
> >> > I can confirm it still happens on 5.0-rc3.
> >> >
> >> > Just to explain what I'm doing:
> >> >
> >> > My ingenic-timer driver probes with builtin_platform_driver_probe
> >> (this
> >> > works),
> >> > and then calls of_platform_populate to probe its children. This
> >> driver,
> >> > ingenic-ost, is one of them, and will fail to probe with
> >> > builtin_platform_driver_probe.
> >> >
> >>
> >> The big question is _why_ it fails to probe.
> >>
> >
> > Are you sharing the device tree node between a 'normal' platform
> > device
> > driver and something more low level DT that marks the device's backing
> > DT node as OF_POPULATED early on? That's my only guess why it's not
> > working.
>
> I do, but I clear the OF_POPULATED flag so that it is then probed as a
> normal platform device, and it's not on this driver's node but its
> parent.
>
Where do you clear the OF_POPULATED flag?
Le jeu. 24 janv. 2019 ? 19:46, Stephen Boyd <[email protected]> a
?crit :
> Quoting Paul Cercueil (2019-01-24 12:46:28)
>>
>>
>> Le jeu. 24 janv. 2019 ? 16:28, Stephen Boyd <[email protected]> a
>> ?crit :
>> > Quoting Guenter Roeck (2019-01-23 10:01:55)
>> >> On Wed, Jan 23, 2019 at 02:25:53PM -0300, Paul Cercueil wrote:
>> >> > Hi,
>> >> >
>> >> > Le mer. 23 janv. 2019 ? 11:31, Guenter Roeck
>> >> <[email protected]> a écrit :
>> >> > >On 1/23/19 4:58 AM, Mathieu Malaterre wrote:
>> >> > >>On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil
>> >> <[email protected]>
>> >> > >>wrote:
>> >> > >>>
>> >> > >>>From: Maarten ter Huurne <[email protected]>
>> >> > >>>
>> >> > >>>OST is the OS Timer, a 64-bit timer/counter with buffered
>> >> reading.
>> >> > >>>
>> >> > >>>SoCs before the JZ4770 had (if any) a 32-bit OST; the
>> JZ4770
>> >> and
>> >> > >>>JZ4780 have a 64-bit OST.
>> >> > >>>
>> >> > >>>This driver will register both a clocksource and a
>> sched_clock
>> >> to the
>> >> > >>>system.
>> >> > >>>
>> >> > >>>Signed-off-by: Maarten ter Huurne <[email protected]>
>> >> > >>>Signed-off-by: Paul Cercueil <[email protected]>
>> >> > >>>---
>> >> > >>>
>> >> > >>>Notes:
>> >> > >>> v5: New patch
>> >> > >>>
>> >> > >>> v6: - Get rid of SoC IDs; pass pointer to
>> >> ingenic_ost_soc_info
>> >> > >>>as
>> >> > >>> devicetree match data instead.
>> >> > >>> - Use device_get_match_data() instead of the of_*
>> >> variant
>> >> > >>> - Handle error of dev_get_regmap() properly
>> >> > >>>
>> >> > >>> v7: Fix section mismatch by using
>> >> > >>>builtin_platform_driver_probe()
>> >> > >>>
>> >> > >>> v8: builtin_platform_driver_probe() does not work
>> >> anymore in
>> >> > >>> 4.20-rc6? The probe function won't be called.
>> Work
>> >> around
>> >> > >>>this
>> >> > >>> for now by using late_initcall.
>> >> > >>>
>> >> > >
>> >> > >Did anyone notice this ? Either something is wrong with the
>> >> driver, or
>> >> > >with the kernel core. Hacking around it seems like the worst
>> >> possible
>> >> > >"solution".
>> >> >
>> >> > I can confirm it still happens on 5.0-rc3.
>> >> >
>> >> > Just to explain what I'm doing:
>> >> >
>> >> > My ingenic-timer driver probes with
>> builtin_platform_driver_probe
>> >> (this
>> >> > works),
>> >> > and then calls of_platform_populate to probe its children.
>> This
>> >> driver,
>> >> > ingenic-ost, is one of them, and will fail to probe with
>> >> > builtin_platform_driver_probe.
>> >> >
>> >>
>> >> The big question is _why_ it fails to probe.
>> >>
>> >
>> > Are you sharing the device tree node between a 'normal' platform
>> > device
>> > driver and something more low level DT that marks the device's
>> backing
>> > DT node as OF_POPULATED early on? That's my only guess why it's
>> not
>> > working.
>>
>> I do, but I clear the OF_POPULATED flag so that it is then probed
>> as a
>> normal platform device, and it's not on this driver's node but its
>> parent.
>>
>
> Where do you clear the OF_POPULATED flag?
>
In the ingenic-timer driver introduced in patch [04/26], inside the
probe function.
Paul,
On Thu, Jan 24, 2019 at 10:41 PM Paul Cercueil <[email protected]> wrote:
>
> Hi Mathieu,
>
> Le jeu. 24 janv. 2019 à 18:26, Mathieu Malaterre <[email protected]> a
> écrit :
> > Paul,
> >
> > On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil <[email protected]>
> > wrote:
> >>
> >> Hi,
> >>
> >> Here's the version 8 and hopefully final version of my patchset,
> >> which
> >> adds support for the Timer/Counter Unit found in JZ47xx SoCs from
> >> Ingenic.
> >
> > I can no longer boot my MIPS Creator CI20 with this series (merged
> > opendingux/for-upstream-timer-v8).
> >
> > Using screen+ttyUSB, I can see messages stopping at:
> >
> > ...
> > [ OK ] Started Cgroup management daemon.
> > Starting Regular background program processing daemon...
> > [ OK ] Started Regular background program processing daemon.
> > Starting System Logging Service...
> > Starting Provide limited super user privileges to specific
> > users...
> > Starting Restore /etc/resolv.conf if the system cras...s
> > shut down....
> > Starting WPA supplicant...
> > Starting D-Bus System Message Bus...
> > [ OK ] Started D-Bus System Message Bus.
> >
> > Nothing really stands out in the error messages. Could you suggest
> > things to try out to get into a bootable state ?
>
> I'm debugging it right now on jz4740, it seems to happen when the
> clocksource
> from the ingenic-timer driver is used. Is it your case? It should not
> happen
> if you have CONFIG_INGENIC_OST set.
Here is what I see:
$ grep CONFIG_INGENIC_OST arch/mips/configs/ci20_defconfig
CONFIG_INGENIC_OST=y
$ make O=ci20 ARCH=mips CROSS_COMPILE=mipsel-linux-gnu- ci20_defconfig
$ grep CONFIG_INGENIC_OST ci20/.config
CONFIG_INGENIC_OST=y
The setting is coming from your commit:
8f66e6b9c98f MIPS: CI20: defconfig: enable OST driver
In an attempt to solve the symptoms I even played with the clock rates
with no success:
&tcu {
/* 3 MHz for the system timer and clocksource */
assigned-clocks = <&tcu TCU_CLK_TIMER0>, <&tcu TCU_CLK_TIMER1>;
assigned-clock-rates = <750000>, <750000>;
};
> >> The big change is that the timer driver has been simplified. The
> >> code to
> >> dynamically update the system timer or clocksource to a new channel
> >> has
> >> been removed. Now, the system timer and clocksource are provided as
> >> children nodes in the devicetree, and the TCU channel to use for
> >> these
> >> is deduced from their respective memory resource. The PWM driver
> >> will
> >> also deduce from its memory resources whether a given PWM channel
> >> can be
> >> used, or is reserved for the system timers.
> >>
> >> Kind regards,
> >> - Paul Cercueil
> >>
>
Hi,
On Fri, Jan 25, 2019 at 5:21 AM, Mathieu Malaterre <[email protected]>
wrote:
> Paul,
>
> On Thu, Jan 24, 2019 at 10:41 PM Paul Cercueil <[email protected]
> <mailto:[email protected]>> wrote:
>>
>> Hi Mathieu,
>>
>> Le jeu. 24 janv. 2019 ? 18:26, Mathieu Malaterre <[email protected]
>> <mailto:[email protected]>> a
>> ?crit :
>> > Paul,
>> >
>> > On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil
>> <[email protected] <mailto:[email protected]>>
>> > wrote:
>> >>
>> >> Hi,
>> >>
>> >> Here's the version 8 and hopefully final version of my patchset,
>> >> which
>> >> adds support for the Timer/Counter Unit found in JZ47xx SoCs
>> from
>> >> Ingenic.
>> >
>> > I can no longer boot my MIPS Creator CI20 with this series (merged
>> > opendingux/for-upstream-timer-v8).
>> >
>> > Using screen+ttyUSB, I can see messages stopping at:
>> >
>> > ...
>> > [ OK ] Started Cgroup management daemon.
>> > Starting Regular background program processing daemon...
>> > [ OK ] Started Regular background program processing daemon.
>> > Starting System Logging Service...
>> > Starting Provide limited super user privileges to
>> specific
>> > users...
>> > Starting Restore /etc/resolv.conf if the system cras...s
>> > shut down....
>> > Starting WPA supplicant...
>> > Starting D-Bus System Message Bus...
>> > [ OK ] Started D-Bus System Message Bus.
>> >
>> > Nothing really stands out in the error messages. Could you suggest
>> > things to try out to get into a bootable state ?
>>
>> I'm debugging it right now on jz4740, it seems to happen when the
>> clocksource
>> from the ingenic-timer driver is used. Is it your case? It should
>> not
>> happen
>> if you have CONFIG_INGENIC_OST set.
>
> Here is what I see:
>
> $ grep CONFIG_INGENIC_OST arch/mips/configs/ci20_defconfig
> CONFIG_INGENIC_OST=y
> $ make O=ci20 ARCH=mips CROSS_COMPILE=mipsel-linux-gnu- ci20_defconfig
> $ grep CONFIG_INGENIC_OST ci20/.config
> CONFIG_INGENIC_OST=y
>
> The setting is coming from your commit:
>
> 8f66e6b9c98f MIPS: CI20: defconfig: enable OST driver
>
> In an attempt to solve the symptoms I even played with the clock rates
> with no success:
>
> &tcu {
> /* 3 MHz for the system timer and clocksource */
> assigned-clocks = <&tcu TCU_CLK_TIMER0>, <&tcu TCU_CLK_TIMER1>;
> assigned-clock-rates = <750000>, <750000>;
> };
This driver didn't see any big change since v6, and we're at v9 now.
I swear it worked fine before, I think even Paul Burton tested it and
reported it working fine. What kernel are you testing on? Could you try
on top of an older kernel, e.g. 4.18?
>> >> The big change is that the timer driver has been simplified. The
>> >> code to
>> >> dynamically update the system timer or clocksource to a new
>> channel
>> >> has
>> >> been removed. Now, the system timer and clocksource are
>> provided as
>> >> children nodes in the devicetree, and the TCU channel to use for
>> >> these
>> >> is deduced from their respective memory resource. The PWM driver
>> >> will
>> >> also deduce from its memory resources whether a given PWM
>> channel
>> >> can be
>> >> used, or is reserved for the system timers.
>> >>
>> >> Kind regards,
>> >> - Paul Cercueil
>> >>
>>
Hi,
Le jeu. 24 janv. 2019 ? 19:53, Paul Cercueil <[email protected]> a
?crit :
>
>
> Le jeu. 24 janv. 2019 ? 19:46, Stephen Boyd <[email protected]> a
> ?crit :
>> Quoting Paul Cercueil (2019-01-24 12:46:28)
>>>
>>>
>>> Le jeu. 24 janv. 2019 ? 16:28, Stephen Boyd <[email protected]> a
>>> ?crit :
>>> > Quoting Guenter Roeck (2019-01-23 10:01:55)
>>> >> On Wed, Jan 23, 2019 at 02:25:53PM -0300, Paul Cercueil wrote:
>>> >> > Hi,
>>> >> >
>>> >> > Le mer. 23 janv. 2019 ? 11:31, Guenter Roeck
>>> >> <[email protected]> a écrit :
>>> >> > >On 1/23/19 4:58 AM, Mathieu Malaterre wrote:
>>> >> > >>On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil
>>> >> <[email protected]>
>>> >> > >>wrote:
>>> >> > >>>
>>> >> > >>>From: Maarten ter Huurne <[email protected]>
>>> >> > >>>
>>> >> > >>>OST is the OS Timer, a 64-bit timer/counter with buffered
>>> >> reading.
>>> >> > >>>
>>> >> > >>>SoCs before the JZ4770 had (if any) a 32-bit OST; the
>>> JZ4770
>>> >> and
>>> >> > >>>JZ4780 have a 64-bit OST.
>>> >> > >>>
>>> >> > >>>This driver will register both a clocksource and a
>>> sched_clock
>>> >> to the
>>> >> > >>>system.
>>> >> > >>>
>>> >> > >>>Signed-off-by: Maarten ter Huurne <[email protected]>
>>> >> > >>>Signed-off-by: Paul Cercueil <[email protected]>
>>> >> > >>>---
>>> >> > >>>
>>> >> > >>>Notes:
>>> >> > >>> v5: New patch
>>> >> > >>>
>>> >> > >>> v6: - Get rid of SoC IDs; pass pointer to
>>> >> ingenic_ost_soc_info
>>> >> > >>>as
>>> >> > >>> devicetree match data instead.
>>> >> > >>> - Use device_get_match_data() instead of the
>>> of_*
>>> >> variant
>>> >> > >>> - Handle error of dev_get_regmap() properly
>>> >> > >>>
>>> >> > >>> v7: Fix section mismatch by using
>>> >> > >>>builtin_platform_driver_probe()
>>> >> > >>>
>>> >> > >>> v8: builtin_platform_driver_probe() does not work
>>> >> anymore in
>>> >> > >>> 4.20-rc6? The probe function won't be called.
>>> Work
>>> >> around
>>> >> > >>>this
>>> >> > >>> for now by using late_initcall.
>>> >> > >>>
>>> >> > >
>>> >> > >Did anyone notice this ? Either something is wrong with the
>>> >> driver, or
>>> >> > >with the kernel core. Hacking around it seems like the worst
>>> >> possible
>>> >> > >"solution".
>>> >> >
>>> >> > I can confirm it still happens on 5.0-rc3.
>>> >> >
>>> >> > Just to explain what I'm doing:
>>> >> >
>>> >> > My ingenic-timer driver probes with
>>> builtin_platform_driver_probe
>>> >> (this
>>> >> > works),
>>> >> > and then calls of_platform_populate to probe its children.
>>> This
>>> >> driver,
>>> >> > ingenic-ost, is one of them, and will fail to probe with
>>> >> > builtin_platform_driver_probe.
>>> >> >
>>> >>
>>> >> The big question is _why_ it fails to probe.
>>> >>
>>> >
>>> > Are you sharing the device tree node between a 'normal' platform
>>> > device
>>> > driver and something more low level DT that marks the device's
>>> backing
>>> > DT node as OF_POPULATED early on? That's my only guess why it's
>>> not
>>> > working.
>>>
>>> I do, but I clear the OF_POPULATED flag so that it is then probed
>>> as a
>>> normal platform device, and it's not on this driver's node but its
>>> parent.
>>>
>>
>> Where do you clear the OF_POPULATED flag?
>>
>
> In the ingenic-timer driver introduced in patch [04/26], inside the
> probe function.
Anything new on this? It still happens on 5.0-rc7.
It probes with late_initcall, and not with device_initcall.
I have no clue what's going on.
-Paul
Quoting Paul Cercueil (2019-02-22 19:17:25)
> Hi,
>
> Anything new on this? It still happens on 5.0-rc7.
> It probes with late_initcall, and not with device_initcall.
> I have no clue what's going on.
>
I'm not sure what's going on either. You'll probably have to debug when
the device is created and when it is probed by enabling the debug
printing in the driver core or by adding in extra debug prints to narrow
down the problem. For example, add a '#define DEBUG 1' at the top of
drivers/base/dd.c and see if that helps give some info on what's going
on with the drivers and devices.
Hi,
Le lun. 25 f?vr. 2019 ? 15:05, Stephen Boyd <[email protected]> a
?crit :
> Quoting Paul Cercueil (2019-02-22 19:17:25)
>> Hi,
>>
>> Anything new on this? It still happens on 5.0-rc7.
>> It probes with late_initcall, and not with device_initcall.
>> I have no clue what's going on.
>>
>
> I'm not sure what's going on either. You'll probably have to debug
> when
> the device is created and when it is probed by enabling the debug
> printing in the driver core or by adding in extra debug prints to
> narrow
> down the problem. For example, add a '#define DEBUG 1' at the top of
> drivers/base/dd.c and see if that helps give some info on what's going
> on with the drivers and devices.
The doc of __platform_driver_probe says:
"Use this instead of platform_driver_register() when you know the device
is not hotpluggable and has already been registered".
When the parent device and child device are both probed with
builtin_platform_driver_probe(), and the parent calls
devm_of_platform_populate(), it is not certain that the parent's
probe will happen before the child's, and if it does not, the child
device has not been registered and its probe is not allowed to defer.
So it turned out not to be a core bug, rather a misuse of the API.
So I will keep the builtin_platform_driver_probe() in the child, and
use a
subsys_initcall() in the parent. That works fine.
Regards,
-Paul