[PATCH v3] clocksource: Add driver for the Ingenic JZ47xx OST

[PATCH v3] clocksource: Add driver for the Ingenic JZ47xx OST

[Paul Cercueil]

From: Maarten ter Huurne <maarten@treewalker.org>

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 <maarten@treewalker.org>
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Tested-by: Mathieu Malaterre <malat@debian.org>
Tested-by: Artur Rojek <contact@artur-rojek.eu>
---

Notes:
    v2: local_irq_save/restore were moved within sched_clock_register
    v3: Only register as 32-bit clocksource to simplify things

 drivers/clocksource/Kconfig       |   8 ++
 drivers/clocksource/Makefile      |   1 +
 drivers/clocksource/ingenic-ost.c | 183 ++++++++++++++++++++++++++++++
 3 files changed, 192 insertions(+)
 create mode 100644 drivers/clocksource/ingenic-ost.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 5fdd76cb1768..5be2f876f64f 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -697,4 +697,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 MFD_SYSCON
+	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 4dfe4225ece7..6bc97a6fd229 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -80,6 +80,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..2c91e3b34572
--- /dev/null
+++ b/drivers/clocksource/ingenic-ost.c
@@ -0,0 +1,183 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * JZ47xx SoCs TCU Operating System Timer driver
+ *
+ * Copyright (C) 2016 Maarten ter Huurne <maarten@treewalker.org>
+ * Copyright (C) 2020 Paul Cercueil <paul@crapouillou.net>
+ */
+
+#include <linux/clk.h>
+#include <linux/clocksource.h>
+#include <linux/mfd/ingenic-tcu.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/regmap.h>
+#include <linux/sched_clock.h>
+
+#define TCU_OST_TCSR_MASK	0xffc0
+#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 struct ingenic_ost *ingenic_ost;
+
+static u64 notrace ingenic_ost_read_cntl(void)
+{
+	u32 val;
+
+	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTL, &val);
+
+	return val;
+}
+
+static u64 notrace ingenic_ost_read_cnth(void)
+{
+	u32 val;
+
+	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTH, &val);
+
+	return val;
+}
+
+static u64 notrace ingenic_ost_clocksource_readl(struct clocksource *cs)
+{
+	return ingenic_ost_read_cntl();
+}
+
+static u64 notrace ingenic_ost_clocksource_readh(struct clocksource *cs)
+{
+	return ingenic_ost_read_cnth();
+}
+
+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;
+	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;
+
+	ingenic_ost = ost;
+
+	ost->map = device_node_to_regmap(dev->parent->of_node);
+	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;
+	cs->mask	= CLOCKSOURCE_MASK(32);
+
+	if (soc_info->is64bit)
+		cs->read = ingenic_ost_clocksource_readl;
+	else
+		cs->read = ingenic_ost_clocksource_readh;
+
+	err = clocksource_register_hz(cs, rate);
+	if (err) {
+		dev_err(dev, "clocksource registration failed: %d\n", err);
+		clk_disable_unprepare(ost->clk);
+		return err;
+	}
+
+	if (soc_info->is64bit)
+		sched_clock_register(ingenic_ost_read_cntl, 32, rate);
+	else
+		sched_clock_register(ingenic_ost_read_cnth, 32, rate);
+
+	return 0;
+}
+
+static int __maybe_unused ingenic_ost_suspend(struct device *dev)
+{
+	struct ingenic_ost *ost = dev_get_drvdata(dev);
+
+	clk_disable(ost->clk);
+
+	return 0;
+}
+
+static int __maybe_unused ingenic_ost_resume(struct device *dev)
+{
+	struct ingenic_ost *ost = dev_get_drvdata(dev);
+
+	return clk_enable(ost->clk);
+}
+
+static const struct dev_pm_ops __maybe_unused ingenic_ost_pm_ops = {
+	/* _noirq: We want the OST clock to be gated last / ungated first */
+	.suspend_noirq = ingenic_ost_suspend,
+	.resume_noirq  = ingenic_ost_resume,
+};
+
+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",
+#ifdef CONFIG_PM_SUSPEND
+		.pm = &ingenic_ost_pm_ops,
+#endif
+		.of_match_table = ingenic_ost_of_match,
+	},
+};
+builtin_platform_driver_probe(ingenic_ost_driver, ingenic_ost_probe);
-- 
2.24.1

Re: [PATCH v3] clocksource: Add driver for the Ingenic JZ47xx OST

[Daniel Lezcano]

On 14/01/2020 16:06, Paul Cercueil wrote:
> From: Maarten ter Huurne <maarten@treewalker.org>
> 
> 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.

Is the JZ47xx OST really a mfd needing a regmap? (Note regmap_read will
take a lock).


> Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Tested-by: Mathieu Malaterre <malat@debian.org>
> Tested-by: Artur Rojek <contact@artur-rojek.eu>
> ---
> 
> Notes:
>     v2: local_irq_save/restore were moved within sched_clock_register
>     v3: Only register as 32-bit clocksource to simplify things
> 
>  drivers/clocksource/Kconfig       |   8 ++
>  drivers/clocksource/Makefile      |   1 +
>  drivers/clocksource/ingenic-ost.c | 183 ++++++++++++++++++++++++++++++
>  3 files changed, 192 insertions(+)
>  create mode 100644 drivers/clocksource/ingenic-ost.c
> 
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 5fdd76cb1768..5be2f876f64f 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -697,4 +697,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 MFD_SYSCON
> +	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 4dfe4225ece7..6bc97a6fd229 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -80,6 +80,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..2c91e3b34572
> --- /dev/null
> +++ b/drivers/clocksource/ingenic-ost.c
> @@ -0,0 +1,183 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * JZ47xx SoCs TCU Operating System Timer driver
> + *
> + * Copyright (C) 2016 Maarten ter Huurne <maarten@treewalker.org>
> + * Copyright (C) 2020 Paul Cercueil <paul@crapouillou.net>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clocksource.h>
> +#include <linux/mfd/ingenic-tcu.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/regmap.h>
> +#include <linux/sched_clock.h>
> +
> +#define TCU_OST_TCSR_MASK	0xffc0
> +#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 struct ingenic_ost *ingenic_ost;
> +
> +static u64 notrace ingenic_ost_read_cntl(void)
> +{
> +	u32 val;
> +
> +	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTL, &val);
> +
> +	return val;
> +}
> +
> +static u64 notrace ingenic_ost_read_cnth(void)
> +{
> +	u32 val;
> +
> +	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTH, &val);
> +
> +	return val;
> +}
> +
> +static u64 notrace ingenic_ost_clocksource_readl(struct clocksource *cs)
> +{
> +	return ingenic_ost_read_cntl();
> +}
> +
> +static u64 notrace ingenic_ost_clocksource_readh(struct clocksource *cs)
> +{
> +	return ingenic_ost_read_cnth();
> +}
> +
> +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;
> +	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;
> +
> +	ingenic_ost = ost;
> +
> +	ost->map = device_node_to_regmap(dev->parent->of_node);
> +	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;
> +	cs->mask	= CLOCKSOURCE_MASK(32);
> +
> +	if (soc_info->is64bit)
> +		cs->read = ingenic_ost_clocksource_readl;
> +	else
> +		cs->read = ingenic_ost_clocksource_readh;
> +
> +	err = clocksource_register_hz(cs, rate);
> +	if (err) {
> +		dev_err(dev, "clocksource registration failed: %d\n", err);
> +		clk_disable_unprepare(ost->clk);
> +		return err;
> +	}
> +
> +	if (soc_info->is64bit)
> +		sched_clock_register(ingenic_ost_read_cntl, 32, rate);
> +	else
> +		sched_clock_register(ingenic_ost_read_cnth, 32, rate);
> +	return 0;
> +}
> +
> +static int __maybe_unused ingenic_ost_suspend(struct device *dev)
> +{
> +	struct ingenic_ost *ost = dev_get_drvdata(dev);
> +
> +	clk_disable(ost->clk);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused ingenic_ost_resume(struct device *dev)
> +{
> +	struct ingenic_ost *ost = dev_get_drvdata(dev);
> +
> +	return clk_enable(ost->clk);
> +}
> +
> +static const struct dev_pm_ops __maybe_unused ingenic_ost_pm_ops = {
> +	/* _noirq: We want the OST clock to be gated last / ungated first */
> +	.suspend_noirq = ingenic_ost_suspend,
> +	.resume_noirq  = ingenic_ost_resume,
> +};
> +
> +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",
> +#ifdef CONFIG_PM_SUSPEND
> +		.pm = &ingenic_ost_pm_ops,
> +#endif
> +		.of_match_table = ingenic_ost_of_match,
> +	},
> +};
> +builtin_platform_driver_probe(ingenic_ost_driver, ingenic_ost_probe);
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v3] clocksource: Add driver for the Ingenic JZ47xx OST

[Paul Cercueil]

Hi Daniel,


Le mer., janv. 15, 2020 at 14:44, Daniel Lezcano 
<daniel.lezcano@linaro.org> a écrit :
> On 14/01/2020 16:06, Paul Cercueil wrote:
>>  From: Maarten ter Huurne <maarten@treewalker.org>
>> 
>>  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.
> 
> Is the JZ47xx OST really a mfd needing a regmap? (Note regmap_read 
> will
> take a lock).

Yes, the TCU_REG_OST_TCSR register is shared with the clocks driver.

- Paul


> 
>>  Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  Tested-by: Mathieu Malaterre <malat@debian.org>
>>  Tested-by: Artur Rojek <contact@artur-rojek.eu>
>>  ---
>> 
>>  Notes:
>>      v2: local_irq_save/restore were moved within 
>> sched_clock_register
>>      v3: Only register as 32-bit clocksource to simplify things
>> 
>>   drivers/clocksource/Kconfig       |   8 ++
>>   drivers/clocksource/Makefile      |   1 +
>>   drivers/clocksource/ingenic-ost.c | 183 
>> ++++++++++++++++++++++++++++++
>>   3 files changed, 192 insertions(+)
>>   create mode 100644 drivers/clocksource/ingenic-ost.c
>> 
>>  diff --git a/drivers/clocksource/Kconfig 
>> b/drivers/clocksource/Kconfig
>>  index 5fdd76cb1768..5be2f876f64f 100644
>>  --- a/drivers/clocksource/Kconfig
>>  +++ b/drivers/clocksource/Kconfig
>>  @@ -697,4 +697,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 MFD_SYSCON
>>  +	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 4dfe4225ece7..6bc97a6fd229 100644
>>  --- a/drivers/clocksource/Makefile
>>  +++ b/drivers/clocksource/Makefile
>>  @@ -80,6 +80,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..2c91e3b34572
>>  --- /dev/null
>>  +++ b/drivers/clocksource/ingenic-ost.c
>>  @@ -0,0 +1,183 @@
>>  +// SPDX-License-Identifier: GPL-2.0
>>  +/*
>>  + * JZ47xx SoCs TCU Operating System Timer driver
>>  + *
>>  + * Copyright (C) 2016 Maarten ter Huurne <maarten@treewalker.org>
>>  + * Copyright (C) 2020 Paul Cercueil <paul@crapouillou.net>
>>  + */
>>  +
>>  +#include <linux/clk.h>
>>  +#include <linux/clocksource.h>
>>  +#include <linux/mfd/ingenic-tcu.h>
>>  +#include <linux/mfd/syscon.h>
>>  +#include <linux/of.h>
>>  +#include <linux/platform_device.h>
>>  +#include <linux/pm.h>
>>  +#include <linux/regmap.h>
>>  +#include <linux/sched_clock.h>
>>  +
>>  +#define TCU_OST_TCSR_MASK	0xffc0
>>  +#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 struct ingenic_ost *ingenic_ost;
>>  +
>>  +static u64 notrace ingenic_ost_read_cntl(void)
>>  +{
>>  +	u32 val;
>>  +
>>  +	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTL, &val);
>>  +
>>  +	return val;
>>  +}
>>  +
>>  +static u64 notrace ingenic_ost_read_cnth(void)
>>  +{
>>  +	u32 val;
>>  +
>>  +	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTH, &val);
>>  +
>>  +	return val;
>>  +}
>>  +
>>  +static u64 notrace ingenic_ost_clocksource_readl(struct 
>> clocksource *cs)
>>  +{
>>  +	return ingenic_ost_read_cntl();
>>  +}
>>  +
>>  +static u64 notrace ingenic_ost_clocksource_readh(struct 
>> clocksource *cs)
>>  +{
>>  +	return ingenic_ost_read_cnth();
>>  +}
>>  +
>>  +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;
>>  +	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;
>>  +
>>  +	ingenic_ost = ost;
>>  +
>>  +	ost->map = device_node_to_regmap(dev->parent->of_node);
>>  +	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;
>>  +	cs->mask	= CLOCKSOURCE_MASK(32);
>>  +
>>  +	if (soc_info->is64bit)
>>  +		cs->read = ingenic_ost_clocksource_readl;
>>  +	else
>>  +		cs->read = ingenic_ost_clocksource_readh;
>>  +
>>  +	err = clocksource_register_hz(cs, rate);
>>  +	if (err) {
>>  +		dev_err(dev, "clocksource registration failed: %d\n", err);
>>  +		clk_disable_unprepare(ost->clk);
>>  +		return err;
>>  +	}
>>  +
>>  +	if (soc_info->is64bit)
>>  +		sched_clock_register(ingenic_ost_read_cntl, 32, rate);
>>  +	else
>>  +		sched_clock_register(ingenic_ost_read_cnth, 32, rate);
>>  +	return 0;
>>  +}
>>  +
>>  +static int __maybe_unused ingenic_ost_suspend(struct device *dev)
>>  +{
>>  +	struct ingenic_ost *ost = dev_get_drvdata(dev);
>>  +
>>  +	clk_disable(ost->clk);
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static int __maybe_unused ingenic_ost_resume(struct device *dev)
>>  +{
>>  +	struct ingenic_ost *ost = dev_get_drvdata(dev);
>>  +
>>  +	return clk_enable(ost->clk);
>>  +}
>>  +
>>  +static const struct dev_pm_ops __maybe_unused ingenic_ost_pm_ops = 
>> {
>>  +	/* _noirq: We want the OST clock to be gated last / ungated first 
>> */
>>  +	.suspend_noirq = ingenic_ost_suspend,
>>  +	.resume_noirq  = ingenic_ost_resume,
>>  +};
>>  +
>>  +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",
>>  +#ifdef CONFIG_PM_SUSPEND
>>  +		.pm = &ingenic_ost_pm_ops,
>>  +#endif
>>  +		.of_match_table = ingenic_ost_of_match,
>>  +	},
>>  +};
>>  +builtin_platform_driver_probe(ingenic_ost_driver, 
>> ingenic_ost_probe);
>> 
> 
> 
> --
>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM 
> SoCs
> 
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
> 

Re: [PATCH v3] clocksource: Add driver for the Ingenic JZ47xx OST

[Maarten ter Huurne]

On Wednesday, 15 January 2020 14:57:01 CET Paul Cercueil wrote:
> Le mer., janv. 15, 2020 at 14:44, Daniel Lezcano
> <daniel.lezcano@linaro.org> a écrit :
> > Is the JZ47xx OST really a mfd needing a regmap? (Note regmap_read
> > will take a lock).
> 
> Yes, the TCU_REG_OST_TCSR register is shared with the clocks driver.

The TCU_REG_OST_TCSR register is only used in the probe though.

To get the counter value from TCU_REG_OST_CNTL/TCU_REG_OST_CNTH you 
could technically do it by reading the register directly, if performance 
concerns make it necessary to bypass the usual kernel infrastructure for 
dealing with shared registers.

Bye,
		Maarten

Re: [PATCH v3] clocksource: Add driver for the Ingenic JZ47xx OST

[Paul Cercueil]

Le mer., janv. 15, 2020 at 18:48, Maarten ter Huurne 
<maarten@treewalker.org> a écrit :
> On Wednesday, 15 January 2020 14:57:01 CET Paul Cercueil wrote:
>>  Le mer., janv. 15, 2020 at 14:44, Daniel Lezcano
>>  <daniel.lezcano@linaro.org> a écrit :
>>  > Is the JZ47xx OST really a mfd needing a regmap? (Note regmap_read
>>  > will take a lock).
>> 
>>  Yes, the TCU_REG_OST_TCSR register is shared with the clocks driver.
> 
> The TCU_REG_OST_TCSR register is only used in the probe though.
> 
> To get the counter value from TCU_REG_OST_CNTL/TCU_REG_OST_CNTH you
> could technically do it by reading the register directly, if 
> performance
> concerns make it necessary to bypass the usual kernel infrastructure 
> for
> dealing with shared registers.

In theory yes, in practice there's no easy way to do that (the 
underlying mmio pointer is not obtainable from the regmap), and 
besides, the lock is just a spinlock and not a mutex.

-Paul

Re: [PATCH v3] clocksource: Add driver for the Ingenic JZ47xx OST

[Thomas Gleixner]

Paul Cercueil <paul@crapouillou.net> writes:
> Le mer., janv. 15, 2020 at 18:48, Maarten ter Huurne 
> <maarten@treewalker.org> a écrit :
>> On Wednesday, 15 January 2020 14:57:01 CET Paul Cercueil wrote:
>>>  Le mer., janv. 15, 2020 at 14:44, Daniel Lezcano
>>>  <daniel.lezcano@linaro.org> a écrit :
>>>  > Is the JZ47xx OST really a mfd needing a regmap? (Note regmap_read
>>>  > will take a lock).
>>> 
>>>  Yes, the TCU_REG_OST_TCSR register is shared with the clocks driver.
>> 
>> The TCU_REG_OST_TCSR register is only used in the probe though.
>> 
>> To get the counter value from TCU_REG_OST_CNTL/TCU_REG_OST_CNTH you
>> could technically do it by reading the register directly, if 
>> performance
>> concerns make it necessary to bypass the usual kernel infrastructure 
>> for
>> dealing with shared registers.
>
> In theory yes, in practice there's no easy way to do that (the 
> underlying mmio pointer is not obtainable from the regmap), and 
> besides, the lock is just a spinlock and not a mutex.

That lock still a massive contention point as clock readouts can be pretty
frequent depending on workloads. Just think about tracing ...

So I really would avoid both the lock and that ugly 64bit readout thing.

Thanks,

        tglx

Re: [PATCH v3] clocksource: Add driver for the Ingenic JZ47xx OST

[Paul Cercueil]

Le mer., janv. 15, 2020 at 20:54, Thomas Gleixner <tglx@linutronix.de> 
a écrit :
> Paul Cercueil <paul@crapouillou.net> writes:
>>  Le mer., janv. 15, 2020 at 18:48, Maarten ter Huurne
>>  <maarten@treewalker.org> a écrit :
>>>  On Wednesday, 15 January 2020 14:57:01 CET Paul Cercueil wrote:
>>>>   Le mer., janv. 15, 2020 at 14:44, Daniel Lezcano
>>>>   <daniel.lezcano@linaro.org> a écrit :
>>>>   > Is the JZ47xx OST really a mfd needing a regmap? (Note 
>>>> regmap_read
>>>>   > will take a lock).
>>>> 
>>>>   Yes, the TCU_REG_OST_TCSR register is shared with the clocks 
>>>> driver.
>>> 
>>>  The TCU_REG_OST_TCSR register is only used in the probe though.
>>> 
>>>  To get the counter value from TCU_REG_OST_CNTL/TCU_REG_OST_CNTH you
>>>  could technically do it by reading the register directly, if
>>>  performance
>>>  concerns make it necessary to bypass the usual kernel 
>>> infrastructure
>>>  for
>>>  dealing with shared registers.
>> 
>>  In theory yes, in practice there's no easy way to do that (the
>>  underlying mmio pointer is not obtainable from the regmap), and
>>  besides, the lock is just a spinlock and not a mutex.
> 
> That lock still a massive contention point as clock readouts can be 
> pretty
> frequent depending on workloads. Just think about tracing ...
> 
> So I really would avoid both the lock and that ugly 64bit readout 
> thing.

The 64bit readout thing is gone in V3.

The lock cannot go away unless we have a way to retrieve the underlying 
mmio pointer from the regmap, which the regmap maintainers will never 
accept. So I can't really change that now. Besides, 
drivers/clocksource/ingenic-timer.c also registers a clocksource that's 
read with the regmap, and nobody complained.

-Paul

Re: [PATCH v3] clocksource: Add driver for the Ingenic JZ47xx OST

[Thomas Gleixner]

Paul Cercueil <paul@crapouillou.net> writes:
> Le mer., janv. 15, 2020 at 20:54, Thomas Gleixner <tglx@linutronix.de> 
>> That lock still a massive contention point as clock readouts can be 
>> pretty
>> frequent depending on workloads. Just think about tracing ...
>> 
>> So I really would avoid both the lock and that ugly 64bit readout 
>> thing.
>
> The 64bit readout thing is gone in V3.
>
> The lock cannot go away unless we have a way to retrieve the underlying 
> mmio pointer from the regmap, which the regmap maintainers will never 
> accept. So I can't really change that now. Besides, 
> drivers/clocksource/ingenic-timer.c also registers a clocksource that's 
> read with the regmap, and nobody complained.

I don't complain. I just told you that a spinlock in that code path is
really suboptimal.

I missed the one in the other driver, but the same problem exists there.

Thanks,

        tglx

Re: [PATCH v3] clocksource: Add driver for the Ingenic JZ47xx OST

[Daniel Lezcano]

Hi Paul,

On 15/01/2020 21:58, Paul Cercueil wrote:
> 
> 
> Le mer., janv. 15, 2020 at 20:54, Thomas Gleixner <tglx@linutronix.de> a
> écrit :
>> Paul Cercueil <paul@crapouillou.net> writes:
>>>  Le mer., janv. 15, 2020 at 18:48, Maarten ter Huurne
>>>  <maarten@treewalker.org> a écrit :
>>>>  On Wednesday, 15 January 2020 14:57:01 CET Paul Cercueil wrote:
>>>>>   Le mer., janv. 15, 2020 at 14:44, Daniel Lezcano
>>>>>   <daniel.lezcano@linaro.org> a écrit :
>>>>>   > Is the JZ47xx OST really a mfd needing a regmap? (Note regmap_read
>>>>>   > will take a lock).
>>>>>
>>>>>   Yes, the TCU_REG_OST_TCSR register is shared with the clocks driver.
>>>>
>>>>  The TCU_REG_OST_TCSR register is only used in the probe though.
>>>>
>>>>  To get the counter value from TCU_REG_OST_CNTL/TCU_REG_OST_CNTH you
>>>>  could technically do it by reading the register directly, if
>>>>  performance
>>>>  concerns make it necessary to bypass the usual kernel infrastructure
>>>>  for
>>>>  dealing with shared registers.
>>>
>>>  In theory yes, in practice there's no easy way to do that (the
>>>  underlying mmio pointer is not obtainable from the regmap), and
>>>  besides, the lock is just a spinlock and not a mutex.
>>
>> That lock still a massive contention point as clock readouts can be
>> pretty
>> frequent depending on workloads. Just think about tracing ...
>>
>> So I really would avoid both the lock and that ugly 64bit readout thing.
> 
> The 64bit readout thing is gone in V3.
> 
> The lock cannot go away unless we have a way to retrieve the underlying
> mmio pointer from the regmap, which the regmap maintainers will never
> accept. So I can't really change that now. Besides,
> drivers/clocksource/ingenic-timer.c also registers a clocksource that's
> read with the regmap, and nobody complained.

Is there any progress on this? Having a lock in this code path is very
impacting.


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v3] clocksource: Add driver for the Ingenic JZ47xx OST

[Paul Cercueil]

Hi Daniel,

Le sam., févr. 8, 2020 at 08:09, Daniel Lezcano 
<daniel.lezcano@linaro.org> a écrit :
> Hi Paul,
> 
> On 15/01/2020 21:58, Paul Cercueil wrote:
>> 
>> 
>>  Le mer., janv. 15, 2020 at 20:54, Thomas Gleixner 
>> <tglx@linutronix.de> a
>>  écrit :
>>>  Paul Cercueil <paul@crapouillou.net> writes:
>>>>   Le mer., janv. 15, 2020 at 18:48, Maarten ter Huurne
>>>>   <maarten@treewalker.org> a écrit :
>>>>>   On Wednesday, 15 January 2020 14:57:01 CET Paul Cercueil wrote:
>>>>>>    Le mer., janv. 15, 2020 at 14:44, Daniel Lezcano
>>>>>>    <daniel.lezcano@linaro.org> a écrit :
>>>>>>    > Is the JZ47xx OST really a mfd needing a regmap? (Note 
>>>>>> regmap_read
>>>>>>    > will take a lock).
>>>>>> 
>>>>>>    Yes, the TCU_REG_OST_TCSR register is shared with the clocks 
>>>>>> driver.
>>>>> 
>>>>>   The TCU_REG_OST_TCSR register is only used in the probe though.
>>>>> 
>>>>>   To get the counter value from TCU_REG_OST_CNTL/TCU_REG_OST_CNTH 
>>>>> you
>>>>>   could technically do it by reading the register directly, if
>>>>>   performance
>>>>>   concerns make it necessary to bypass the usual kernel 
>>>>> infrastructure
>>>>>   for
>>>>>   dealing with shared registers.
>>>> 
>>>>   In theory yes, in practice there's no easy way to do that (the
>>>>   underlying mmio pointer is not obtainable from the regmap), and
>>>>   besides, the lock is just a spinlock and not a mutex.
>>> 
>>>  That lock still a massive contention point as clock readouts can be
>>>  pretty
>>>  frequent depending on workloads. Just think about tracing ...
>>> 
>>>  So I really would avoid both the lock and that ugly 64bit readout 
>>> thing.
>> 
>>  The 64bit readout thing is gone in V3.
>> 
>>  The lock cannot go away unless we have a way to retrieve the 
>> underlying
>>  mmio pointer from the regmap, which the regmap maintainers will 
>> never
>>  accept. So I can't really change that now. Besides,
>>  drivers/clocksource/ingenic-timer.c also registers a clocksource 
>> that's
>>  read with the regmap, and nobody complained.
> 
> Is there any progress on this? Having a lock in this code path is very
> impacting.

I have a v4 ready, I'm just waiting for 5.6-rc1 to start to rebase on 
top and send it.

-Paul