Re: [PATCH v3 2/2] clocksource: Ingenic: Add support for the Ingenic X1000 OST.

[Zhou Yanjie]

Hi Paul,

å 2020/7/1 äå3:38, Paul Cercueil åé:
> Hi Zhou,
>
> Le mer. 1 juil. 2020 Ã 1:15, åçæ (Zhou Yanjie) 
> <zhouyanjie@xxxxxxxxxxxxxx> a Ãcrit :
> > X1000 and SoCs after X1000 (such as X1500 and X1830) had a separate
> > OST, it no longer belongs to TCU. This driver will register both a
> > clocksource and a sched_clock to the system.
> >
> > Tested-by: åæ (Zhou Zheng) <sernia.zhou@xxxxxxxxxxx>
> > Co-developed-by: æéæ (Qi Pengzhen) <aric.pzqi@xxxxxxxxxxx>
> > Signed-off-by: æéæ (Qi Pengzhen) <aric.pzqi@xxxxxxxxxxx>
> > Signed-off-by: åçæ (Zhou Yanjie) <zhouyanjie@xxxxxxxxxxxxxx>
> > ---
> >
> > Notes:
> > ÂÂÂ v1->v2:
> > ÂÂÂ Fix compile warnings.
> > ÂÂÂ Reported-by: kernel test robot <lkp@xxxxxxxxx>
> >
> > ÂÂÂ v2->v3:
> > ÂÂÂ No change.
> >
> > Âdrivers/clocksource/KconfigÂÂÂÂÂÂÂÂÂ |Â 19 +-
> > Âdrivers/clocksource/MakefileÂÂÂÂÂÂÂÂ |ÂÂ 1 +
> > Âdrivers/clocksource/ingenic-sysost.c | 509 
> > +++++++++++++++++++++++++++++++++++
> > Â3 files changed, 525 insertions(+), 4 deletions(-)
> > Âcreate mode 100644 drivers/clocksource/ingenic-sysost.c
> >
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index 91418381fcd4..172397a00f3e 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -686,7 +686,7 @@ config MILBEAUT_TIMER
> > ÂÂÂÂÂÂ Enables the support for Milbeaut timer driver.
> >
> > Âconfig INGENIC_TIMER
> > -ÂÂÂ bool "Clocksource/timer using the TCU in Ingenic JZ SoCs"
> > +ÂÂÂ bool "Clocksource/timer using the TCU in Ingenic SoCs"
>
> This cosmetic change is unrelated to the patch, please drop it.

Sure.


> > ÂÂÂÂ default MACH_INGENIC
> > ÂÂÂÂ depends on MIPS || COMPILE_TEST
> > ÂÂÂÂ depends on COMMON_CLK
> > @@ -694,15 +694,26 @@ config INGENIC_TIMER
> > ÂÂÂÂ select TIMER_OF
> > ÂÂÂÂ select IRQ_DOMAIN
> > ÂÂÂÂ help
> > -ÂÂÂÂÂ Support for the timer/counter unit of the Ingenic JZ SoCs.
> > +ÂÂÂÂÂ Support for the timer/counter unit of the Ingenic SoCs.
>
> Same here.

Sure.


> > +
> > +config INGENIC_SYSOST
> > +ÂÂÂ bool "Clocksource/timer using the SYSOST in Ingenic SoCs"
> > +ÂÂÂ default MACH_INGENIC
> > +ÂÂÂ depends on MIPS || COMPILE_TEST
> > +ÂÂÂ depends on COMMON_CLK
> > +ÂÂÂ select MFD_SYSCON
> > +ÂÂÂ select TIMER_OF
> > +ÂÂÂ select IRQ_DOMAIN
> > +ÂÂÂ help
> > +ÂÂÂÂÂ This option enables support for SYSOST in Ingenic SoCs .
> >
> > Âconfig INGENIC_OST
> > -ÂÂÂ bool "Clocksource for Ingenic OS Timer"
> > +ÂÂÂ bool "Clocksource for Ingenic OST"
>
> Same here.

OK.


> > ÂÂÂÂ depends on MIPS || COMPILE_TEST
> > ÂÂÂÂ depends on COMMON_CLK
> > ÂÂÂÂ select MFD_SYSCON
> > ÂÂÂÂ help
> > -ÂÂÂÂÂ Support for the Operating System Timer of the Ingenic JZ SoCs.
> > +ÂÂÂÂÂ Support for the Operating System Timer of the Ingenic SoCs.
>
> Same here.

Sure.


> > Âconfig MICROCHIP_PIT64B
> > ÂÂÂÂ bool "Microchip PIT64B support"
> > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> > index bdda1a2e4097..3994e221e262 100644
> > --- a/drivers/clocksource/Makefile
> > +++ b/drivers/clocksource/Makefile
> > @@ -82,6 +82,7 @@ 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_SYSOST)ÂÂÂ += ingenic-sysost.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-sysost.c 
> > b/drivers/clocksource/ingenic-sysost.c
> > new file mode 100644
> > index 000000000000..d3e1b7582221
> > --- /dev/null
> > +++ b/drivers/clocksource/ingenic-sysost.c
> > @@ -0,0 +1,509 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Ingenic XBurst SoCs SYSOST clocks driver
> > + * Copyright (c) 2020 åçæ (Zhou Yanjie) <zhouyanjie@xxxxxxxxxxxxxx>
> > + */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/clockchips.h>
> > +#include <linux/clocksource.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/sched_clock.h>
> > +#include <linux/slab.h>
> > +#include <linux/syscore_ops.h>
> > +
> > +#include <dt-bindings/clock/ingenic,ost.h>
> > +
> > +/* OST register offsets */
> > +#define OST_REG_OSTCCRÂÂÂÂÂÂÂÂÂÂÂ 0x00
> > +#define OST_REG_OSTCRÂÂÂÂÂÂÂÂÂÂÂ 0x08
> > +#define OST_REG_OSTFRÂÂÂÂÂÂÂÂÂÂÂ 0x0c
> > +#define OST_REG_OSTMRÂÂÂÂÂÂÂÂÂÂÂ 0x10
> > +#define OST_REG_OST1DFRÂÂÂÂÂÂÂÂÂÂÂ 0x14
> > +#define OST_REG_OST1CNTÂÂÂÂÂÂÂÂÂÂÂ 0x18
> > +#define OST_REG_OST2CNTLÂÂÂÂÂÂÂ 0x20
> > +#define OST_REG_OSTCNT2HBUFÂÂÂÂÂÂÂ 0x24
> > +#define OST_REG_OSTESRÂÂÂÂÂÂÂÂÂÂÂ 0x34
> > +#define OST_REG_OSTECRÂÂÂÂÂÂÂÂÂÂÂ 0x38
> > +
> > +/* bits within the OSTCCR register */
> > +#define OSTCCR_PRESCALE1_MASKÂÂÂ 0x3
> > +#define OSTCCR_PRESCALE2_MASKÂÂÂ 0xc
> > +#define OSTCCR_PRESCALE1_LSBÂÂÂ 0
> > +#define OSTCCR_PRESCALE2_LSBÂÂÂ 2
> > +
> > +/* bits within the OSTCR register */
> > +#define OSTCR_OST1CLRÂÂÂÂÂÂÂÂÂÂÂ BIT(0)
> > +#define OSTCR_OST2CLRÂÂÂÂÂÂÂÂÂÂÂ BIT(1)
> > +
> > +/* bits within the OSTFR register */
> > +#define OSTFR_FFLAGÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ BIT(0)
> > +
> > +/* bits within the OSTMR register */
> > +#define OSTMR_FMASKÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ BIT(0)
> > +
> > +/* bits within the OSTESR register */
> > +#define OSTESR_OST1ENSÂÂÂÂÂÂÂÂÂÂÂ BIT(0)
> > +#define OSTESR_OST2ENSÂÂÂÂÂÂÂÂÂÂÂ BIT(1)
> > +
> > +/* bits within the OSTECR register */
> > +#define OSTECR_OST1ENCÂÂÂÂÂÂÂÂÂÂÂ BIT(0)
> > +#define OSTECR_OST2ENCÂÂÂÂÂÂÂÂÂÂÂ BIT(1)
> > +
> > +enum ost_clk_parent {
> > +ÂÂÂ OST_PARENT_EXT,
> > +};
> > +
> > +struct ingenic_soc_info {
> > +ÂÂÂ unsigned int num_channels;
> > +};
>
> Unless you plan to add support for other SoCs which have more than 2 
> channels, I think you can drop this - just add a OST_NUM_CHANNELS macro.

When X2000 turned on SMT, a total of 3 channels are required, this is 
reserved for adding support for X2000 in the future.


> > +
> > +struct ingenic_ost_clk_info {
> > +ÂÂÂ struct clk_init_data init_data;
> > +ÂÂÂ u8 ostccr_reg;
> > +};
> > +
> > +struct ingenic_ost_clk {
> > +ÂÂÂ struct clk_hw hw;
> > +ÂÂÂ unsigned int idx;
> > +ÂÂÂ struct ingenic_ost *ost;
> > +ÂÂÂ const struct ingenic_ost_clk_info *info;
> > +};
> > +
> > +struct ingenic_ost {
> > +ÂÂÂ void __iomem *base;
> > +ÂÂÂ const struct ingenic_soc_info *soc_info;
> > +ÂÂÂ struct clk *clk, *percpu_timer_clk, *global_timer_clk;
> > +ÂÂÂ unsigned int percpu_timer_channel, global_timer_channel;
> > +ÂÂÂ struct clock_event_device cevt;
> > +ÂÂÂ struct clocksource cs;
> > +ÂÂÂ char name[20];
> > +
> > +ÂÂÂ struct clk_hw_onecell_data *clocks;
> > +};
> > +
> > +static struct ingenic_ost *ingenic_ost;
> > +
> > +static inline struct ingenic_ost_clk *to_ost_clk(struct clk_hw *hw)
> > +{
> > +ÂÂÂ return container_of(hw, struct ingenic_ost_clk, hw);
> > +}
> > +
> > +static unsigned long ingenic_ost_recalc_rate(struct clk_hw *hw,
> > +ÂÂÂÂÂÂÂ unsigned long parent_rate)
> > +{
> > +ÂÂÂ struct ingenic_ost_clk *ost_clk = to_ost_clk(hw);
> > +ÂÂÂ const struct ingenic_ost_clk_info *info = ost_clk->info;
> > +ÂÂÂ unsigned int prescale;
> > +
> > +ÂÂÂ prescale = readl(ost_clk->ost->base + info->ostccr_reg);
> > +
> > +ÂÂÂ if (ost_clk->idx == OST_CLK_PERCPU_TIMER)
> > +ÂÂÂÂÂÂÂ prescale = (prescale & OSTCCR_PRESCALE1_MASK) >> 
> > OSTCCR_PRESCALE1_LSB;
> > +ÂÂÂ else if (ost_clk->idx == OST_CLK_GLOBAL_TIMER)
> > +ÂÂÂÂÂÂÂ prescale = (prescale & OSTCCR_PRESCALE2_MASK) >> 
> > OSTCCR_PRESCALE2_LSB;
>
> Instead of checking which clock you're manipulating here, just set a 
> different .recalc_rate to the two clocks.

Sure.


> > +
> > +ÂÂÂ return parent_rate >> (prescale * 2);
> > +}
> > +
> > +static u8 ingenic_ost_get_prescale(unsigned long rate, unsigned long 
> > req_rate)
> > +{
> > +ÂÂÂ u8 prescale;
> > +
> > +ÂÂÂ for (prescale = 0; prescale < 2; prescale++)
> > +ÂÂÂÂÂÂÂ if ((rate >> (prescale * 2)) <= req_rate)
> > +ÂÂÂÂÂÂÂÂÂÂÂ return prescale;
> > +
> > +ÂÂÂ return 2; /* /16 divider */
> > +}
> > +
> > +static long ingenic_ost_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 rate;
> > +
> > +ÂÂÂ prescale = ingenic_ost_get_prescale(rate, req_rate);
> > +
> > +ÂÂÂ return rate >> (prescale * 2);
> > +}
> > +
> > +static int ingenic_ost_set_rate(struct clk_hw *hw, unsigned long 
> > req_rate,
> > +ÂÂÂÂÂÂÂ unsigned long parent_rate)
> > +{
> > +ÂÂÂ struct ingenic_ost_clk *ost_clk = to_ost_clk(hw);
> > +ÂÂÂ const struct ingenic_ost_clk_info *info = ost_clk->info;
> > +ÂÂÂ u8 prescale = ingenic_ost_get_prescale(parent_rate, req_rate);
> > +ÂÂÂ int val;
> > +
> > +ÂÂÂ if (ost_clk->idx == OST_CLK_PERCPU_TIMER) {
> > +ÂÂÂÂÂÂÂ val = readl(ost_clk->ost->base + info->ostccr_reg);
> > +ÂÂÂÂÂÂÂ val = (val & ~OSTCCR_PRESCALE1_MASK) | (prescale << 
> > OSTCCR_PRESCALE1_LSB);
> > +ÂÂÂÂÂÂÂ writel(val, ost_clk->ost->base + info->ostccr_reg);
> > +ÂÂÂ } else if (ost_clk->idx == OST_CLK_GLOBAL_TIMER) {
> > +ÂÂÂÂÂÂÂ val = readl(ost_clk->ost->base + info->ostccr_reg);
> > +ÂÂÂÂÂÂÂ val = (val & ~OSTCCR_PRESCALE2_MASK) | (prescale << 
> > OSTCCR_PRESCALE2_LSB);
> > +ÂÂÂÂÂÂÂ writel(val, ost_clk->ost->base + info->ostccr_reg);
> > +ÂÂÂ }
>
> Same here - set a different .set_rate() callback to the two clocks.

Sure.


> > +
> > +ÂÂÂ return 0;
> > +}
> > +
> > +static const struct clk_ops ingenic_ost_clk_ops = {
> > +ÂÂÂ .recalc_rateÂÂÂ = ingenic_ost_recalc_rate,
> > +ÂÂÂ .round_rateÂÂÂ = ingenic_ost_round_rate,
> > +ÂÂÂ .set_rateÂÂÂ = ingenic_ost_set_rate,
> > +};
> > +
> > +static const char * const ingenic_ost_clk_parents[] = {
> > +ÂÂÂ [OST_PARENT_EXT]Â = "ext",
> > +};
>
> just { "ext" } will be fine, no need to specify the index, which is 
> zero anyway. Then you can drop the ost_clock_parent enum.

OK.


> > +
> > +#define DEF_TIMER(_name, _ostccr) { \
> > +ÂÂÂ .init_data = { \
> > +ÂÂÂÂÂÂÂ .name = _name, \
> > +ÂÂÂÂÂÂÂ .parent_names = ingenic_ost_clk_parents, \
> > +ÂÂÂÂÂÂÂ .num_parents = ARRAY_SIZE(ingenic_ost_clk_parents), \
> > +ÂÂÂÂÂÂÂ .ops = &ingenic_ost_clk_ops, \
> > +ÂÂÂÂÂÂÂ .flags = CLK_SET_RATE_UNGATE, \
> > +ÂÂÂ }, \
> > +ÂÂÂ .ostccr_reg = _ostccr, \
> > +}
> > +
> > +static const struct ingenic_ost_clk_info ingenic_ost_clk_info[] = {
> > +ÂÂÂ [OST_CLK_PERCPU_TIMER] = DEF_TIMER("percpu timer", OST_REG_OSTCCR),
> > +ÂÂÂ [OST_CLK_GLOBAL_TIMER] = DEF_TIMER("global timer", OST_REG_OSTCCR),
> > +};
> > +
> > +#undef DEF_TIMER
> > +
> > +static u64 notrace ingenic_ost_global_timer_read_cntl(void)
> > +{
> > +ÂÂÂ struct ingenic_ost *ost = ingenic_ost;
> > +ÂÂÂ unsigned int count;
> > +
> > +ÂÂÂ count = readl(ost->base + OST_REG_OST2CNTL);
> > +
> > +ÂÂÂ return count;
> > +}
> > +
> > +static u64 notrace ingenic_ost_clocksource_read(struct clocksource *cs)
> > +{
> > +ÂÂÂ return ingenic_ost_global_timer_read_cntl();
> > +}
> > +
> > +static inline struct ingenic_ost *to_ingenic_ost(struct 
> > clock_event_device *evt)
> > +{
> > +ÂÂÂ return container_of(evt, struct ingenic_ost, cevt);
> > +}
> > +
> > +static int ingenic_ost_cevt_set_state_shutdown(struct 
> > clock_event_device *evt)
> > +{
> > +ÂÂÂ struct ingenic_ost *ost = to_ingenic_ost(evt);
> > +
> > +ÂÂÂ writel(OSTECR_OST1ENC, ost->base + OST_REG_OSTECR);
> > +
> > +ÂÂÂ return 0;
> > +}
> > +
> > +static int ingenic_ost_cevt_set_next(unsigned long next,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct clock_event_device *evt)
> > +{
> > +ÂÂÂ struct ingenic_ost *ost = to_ingenic_ost(evt);
> > +
> > +ÂÂÂ writel((u32)~OSTFR_FFLAG, ost->base + OST_REG_OSTFR);
> > +ÂÂÂ writel(next, ost->base + OST_REG_OST1DFR);
> > +ÂÂÂ writel(OSTCR_OST1CLR, ost->base + OST_REG_OSTCR);
> > +ÂÂÂ writel(OSTESR_OST1ENS, ost->base + OST_REG_OSTESR);
> > +ÂÂÂ writel((u32)~OSTMR_FMASK, ost->base + OST_REG_OSTMR);
> > +
> > +ÂÂÂ return 0;
> > +}
> > +
> > +static irqreturn_t ingenic_ost_cevt_cb(int irq, void *dev_id)
> > +{
> > +ÂÂÂ struct clock_event_device *evt = dev_id;
> > +ÂÂÂ struct ingenic_ost *ost = to_ingenic_ost(evt);
> > +
> > +ÂÂÂ writel(OSTECR_OST1ENC, ost->base + OST_REG_OSTECR);
> > +
> > +ÂÂÂ if (evt->event_handler)
> > +ÂÂÂÂÂÂÂ evt->event_handler(evt);
> > +
> > +ÂÂÂ return IRQ_HANDLED;
> > +}
> > +
> > +static int __init ingenic_ost_register_clock(struct ingenic_ost *ost,
> > +ÂÂÂÂÂÂÂÂÂÂÂ unsigned int idx, const struct ingenic_ost_clk_info *info,
> > +ÂÂÂÂÂÂÂÂÂÂÂ struct clk_hw_onecell_data *clocks)
> > +{
> > +ÂÂÂ struct ingenic_ost_clk *ost_clk;
> > +ÂÂÂ int val, err;
> > +
> > +ÂÂÂ ost_clk = kzalloc(sizeof(*ost_clk), GFP_KERNEL);
> > +ÂÂÂ if (!ost_clk)
> > +ÂÂÂÂÂÂÂ return -ENOMEM;
> > +
> > +ÂÂÂ ost_clk->hw.init = &info->init_data;
> > +ÂÂÂ ost_clk->idx = idx;
> > +ÂÂÂ ost_clk->info = info;
> > +ÂÂÂ ost_clk->ost = ost;
> > +
> > +ÂÂÂ /* Reset clock divider */
> > +ÂÂÂ val = readl(ost->base + info->ostccr_reg);
> > +ÂÂÂ val &= ~(OSTCCR_PRESCALE1_MASK | OSTCCR_PRESCALE2_MASK);
> > +ÂÂÂ writel(val, ost->base + info->ostccr_reg);
> > +
> > +ÂÂÂ err = clk_hw_register(NULL, &ost_clk->hw);
> > +ÂÂÂ if (err) {
> > +ÂÂÂÂÂÂÂ kfree(ost_clk);
> > +ÂÂÂÂÂÂÂ return err;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ clocks->hws[idx] = &ost_clk->hw;
> > +
> > +ÂÂÂ return 0;
> > +}
> > +
> > +static struct clk * __init ingenic_ost_get_clock(struct device_node 
> > *np, int id)
> > +{
> > +ÂÂÂ struct of_phandle_args args;
> > +
> > +ÂÂÂ args.np = np;
> > +ÂÂÂ args.args_count = 1;
> > +ÂÂÂ args.args[0] = id;
> > +
> > +ÂÂÂ return of_clk_get_from_provider(&args);
> > +}
> > +
> > +static int __init ingenic_ost_percpu_timer_init(struct device_node *np,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct ingenic_ost *ost)
> > +{
> > +ÂÂÂ unsigned int timer_virq, channel = ost->percpu_timer_channel;
> > +ÂÂÂ unsigned long rate;
> > +ÂÂÂ int err;
> > +
> > +ÂÂÂ ost->percpu_timer_clk = ingenic_ost_get_clock(np, channel);
> > +ÂÂÂ if (IS_ERR(ost->percpu_timer_clk))
> > +ÂÂÂÂÂÂÂ return PTR_ERR(ost->percpu_timer_clk);
> > +
> > +ÂÂÂ err = clk_prepare_enable(ost->percpu_timer_clk);
> > +ÂÂÂ if (err)
> > +ÂÂÂÂÂÂÂ goto err_clk_put;
> > +
> > +ÂÂÂ rate = clk_get_rate(ost->percpu_timer_clk);
> > +ÂÂÂ if (!rate) {
> > +ÂÂÂÂÂÂÂ err = -EINVAL;
> > +ÂÂÂÂÂÂÂ goto err_clk_disable;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ timer_virq = of_irq_get(np, 0);
> > +ÂÂÂ if (!timer_virq) {
> > +ÂÂÂÂÂÂÂ err = -EINVAL;
> > +ÂÂÂÂÂÂÂ goto err_clk_disable;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ snprintf(ost->name, sizeof(ost->name), "OST percpu timer");
> > +
> > +ÂÂÂ err = request_irq(timer_virq, ingenic_ost_cevt_cb, IRQF_TIMER,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂ ost->name, &ost->cevt);
> > +ÂÂÂ if (err)
> > +ÂÂÂÂÂÂÂ goto err_irq_dispose_mapping;
> > +
> > +ÂÂÂ ost->cevt.cpumask = cpumask_of(smp_processor_id());
> > +ÂÂÂ ost->cevt.features = CLOCK_EVT_FEAT_ONESHOT;
> > +ÂÂÂ ost->cevt.name = ost->name;
> > +ÂÂÂ ost->cevt.rating = 400;
> > +ÂÂÂ ost->cevt.set_state_shutdown = ingenic_ost_cevt_set_state_shutdown;
> > +ÂÂÂ ost->cevt.set_next_event = ingenic_ost_cevt_set_next;
> > +
> > +ÂÂÂ clockevents_config_and_register(&ost->cevt, rate, 4, 0xffffffff);
> > +
> > +ÂÂÂ return 0;
> > +
> > +err_irq_dispose_mapping:
> > +ÂÂÂ irq_dispose_mapping(timer_virq);
> > +err_clk_disable:
> > +ÂÂÂ clk_disable_unprepare(ost->percpu_timer_clk);
> > +err_clk_put:
> > +ÂÂÂ clk_put(ost->percpu_timer_clk);
> > +ÂÂÂ return err;
> > +}
> > +
> > +static int __init ingenic_ost_global_timer_init(struct device_node *np,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct ingenic_ost *ost)
> > +{
> > +ÂÂÂ unsigned int channel = ost->global_timer_channel;
> > +ÂÂÂ struct clocksource *cs = &ost->cs;
> > +ÂÂÂ unsigned long rate;
> > +ÂÂÂ int err;
> > +
> > +ÂÂÂ ost->global_timer_clk = ingenic_ost_get_clock(np, channel);
> > +ÂÂÂ if (IS_ERR(ost->global_timer_clk))
> > +ÂÂÂÂÂÂÂ return PTR_ERR(ost->global_timer_clk);
> > +
> > +ÂÂÂ err = clk_prepare_enable(ost->global_timer_clk);
> > +ÂÂÂ if (err)
> > +ÂÂÂÂÂÂÂ goto err_clk_put;
> > +
> > +ÂÂÂ rate = clk_get_rate(ost->global_timer_clk);
> > +ÂÂÂ if (!rate) {
> > +ÂÂÂÂÂÂÂ err = -EINVAL;
> > +ÂÂÂÂÂÂÂ goto err_clk_disable;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ /* Clear counter CNT registers */
> > +ÂÂÂ writel(OSTCR_OST2CLR, ost->base + OST_REG_OSTCR);
> > +
> > +ÂÂÂ /* Enable OST channel */
> > +ÂÂÂ writel(OSTESR_OST2ENS, ost->base + OST_REG_OSTESR);
> > +
> > +ÂÂÂ cs->name = "ingenic-ost";
> > +ÂÂÂ cs->rating = 400;
> > +ÂÂÂ cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
> > +ÂÂÂ cs->mask = CLOCKSOURCE_MASK(32);
> > +ÂÂÂ cs->read = ingenic_ost_clocksource_read;
> > +
> > +ÂÂÂ err = clocksource_register_hz(cs, rate);
> > +ÂÂÂ if (err)
> > +ÂÂÂÂÂÂÂ goto err_clk_disable;
> > +
> > +ÂÂÂ return 0;
> > +
> > +err_clk_disable:
> > +ÂÂÂ clk_disable_unprepare(ost->global_timer_clk);
> > +err_clk_put:
> > +ÂÂÂ clk_put(ost->global_timer_clk);
> > +ÂÂÂ return err;
> > +}
> > +
> > +static const struct ingenic_soc_info x1000_soc_info = {
> > +ÂÂÂ .num_channels = 2,
> > +};
> > +
> > +static const struct of_device_id __maybe_unused 
> > ingenic_ost_of_match[] __initconst = {
> > +ÂÂÂ { .compatible = "ingenic,x1000-ost", .data = &x1000_soc_info, },
> > +ÂÂÂ { /* sentinel */ }
> > +};
> > +
> > +static int __init ingenic_ost_probe(struct device_node *np)
> > +{
> > +ÂÂÂ const struct of_device_id *id = 
> > of_match_node(ingenic_ost_of_match, np);
> > +ÂÂÂ struct ingenic_ost *ost;
> > +ÂÂÂ unsigned int i;
> > +ÂÂÂ int ret;
> > +
> > +ÂÂÂ ost = kzalloc(sizeof(*ost), GFP_KERNEL);
> > +ÂÂÂ if (!ost)
> > +ÂÂÂÂÂÂÂ return -ENOMEM;
> > +
> > +ÂÂÂ ost->base = of_iomap(np, 0);
> > +ÂÂÂ if (IS_ERR(ost->base)) {
> > +ÂÂÂÂÂÂÂ pr_err("%s: Failed to map OST registers\n", __func__);
> > +ÂÂÂÂÂÂÂ ret = PTR_ERR(ost->base);
> > +ÂÂÂÂÂÂÂ goto err_free_ost;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ ost->clk = of_clk_get_by_name(np, "ost");
> > +ÂÂÂ if (IS_ERR(ost->clk)) {
> > +ÂÂÂÂÂÂÂ ret = PTR_ERR(ost->clk);
> > +ÂÂÂÂÂÂÂ pr_crit("%s: Cannot get OST clock\n", __func__);
> > +ÂÂÂÂÂÂÂ goto err_free_ost;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ ret = clk_prepare_enable(ost->clk);
> > +ÂÂÂ if (ret) {
> > +ÂÂÂÂÂÂÂ pr_crit("%s: Unable to enable OST clock\n", __func__);
> > +ÂÂÂÂÂÂÂ goto err_put_clk;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ ost->soc_info = id->data;
> > +
> > +ÂÂÂ ost->clocks = kzalloc(struct_size(ost->clocks, hws, 
> > ost->soc_info->num_channels),
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ GFP_KERNEL);
> > +ÂÂÂ if (!ost->clocks) {
> > +ÂÂÂÂÂÂÂ ret = -ENOMEM;
> > +ÂÂÂÂÂÂÂ goto err_clk_disable;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ ost->clocks->num = ost->soc_info->num_channels;
> > +
> > +ÂÂÂ for (i = 0; i < ost->clocks->num; i++) {
> > +ÂÂÂÂÂÂÂ ret = ingenic_ost_register_clock(ost, i, 
> > &ingenic_ost_clk_info[i], ost->clocks);
> > +ÂÂÂÂÂÂÂ if (ret) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ pr_crit("%s: Cannot register clock %d\n", __func__, i);
> > +ÂÂÂÂÂÂÂÂÂÂÂ goto err_unregister_ost_clocks;
> > +ÂÂÂÂÂÂÂ }
> > +ÂÂÂ }
> > +
> > +ÂÂÂ ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, 
> > ost->clocks);
> > +ÂÂÂ if (ret) {
> > +ÂÂÂÂÂÂÂ pr_crit("%s: Cannot add OF clock provider\n", __func__);
> > +ÂÂÂÂÂÂÂ goto err_unregister_ost_clocks;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ ost->percpu_timer_channel = OST_CLK_PERCPU_TIMER;
> > +ÂÂÂ ost->global_timer_channel = OST_CLK_GLOBAL_TIMER;
>
> If that's a constant, just use the constant where 
> 'ost->percpu_timer_channel' and 'ost->global_timer_channel' are used, 
> I think.

Sure.


> > +
> > +ÂÂÂ ingenic_ost = ost;
> > +
> > +ÂÂÂ return 0;
> > +
> > +err_unregister_ost_clocks:
> > +ÂÂÂ for (i = 0; i < ost->clocks->num; i++)
> > +ÂÂÂÂÂÂÂ if (ost->clocks->hws[i])
> > +ÂÂÂÂÂÂÂÂÂÂÂ clk_hw_unregister(ost->clocks->hws[i]);
> > +ÂÂÂ kfree(ost->clocks);
> > +err_clk_disable:
> > +ÂÂÂ clk_disable_unprepare(ost->clk);
> > +err_put_clk:
> > +ÂÂÂ clk_put(ost->clk);
> > +err_free_ost:
> > +ÂÂÂ kfree(ost);
> > +ÂÂÂ return ret;
> > +}
> > +
> > +static int __init ingenic_ost_init(struct device_node *np)
> > +{
> > +ÂÂÂ unsigned long rate;
> > +ÂÂÂ int ret = ingenic_ost_probe(np);
>
> Don't call functions in variable declarations - they can be overlooked 
> easily.

OK


> > +
> > +ÂÂÂ if (ret)
> > +ÂÂÂÂÂÂÂ pr_crit("%s: Failed to initialize OST clocks: %d\n", 
> > __func__, ret);
>
> return ret?

I'll add it in the next version.


> > +
> > +ÂÂÂ of_node_clear_flag(np, OF_POPULATED);
> > +
> > +ÂÂÂ ret = ingenic_ost_global_timer_init(np, ingenic_ost);
>
> The 'ingenic_ost' variable is just a dirty workaround for the 
> sched_clock callback, which does not take any parameter. Avoid using 
> it here. Make ingenic_ost_probe() return a pointer to your ingenic_ost 
> instance (and a PTR_ERR() on error), and use that pointer for the rest.

Sure.

Thanks and best regards!


> Cheers,
> -Paul
>
> > +ÂÂÂ if (ret) {
> > +ÂÂÂÂÂÂÂ pr_crit("%s: Unable to init global timer: %x\n", __func__, 
> > ret);
> > +ÂÂÂÂÂÂÂ goto err_free_ingenic_ost;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ ret = ingenic_ost_percpu_timer_init(np, ingenic_ost);
> > +ÂÂÂ if (ret)
> > +ÂÂÂÂÂÂÂ goto err_ost_global_timer_cleanup;
> > +
> > +ÂÂÂ /* Register the sched_clock at the end as there's no way to undo 
> > it */
> > +ÂÂÂ rate = clk_get_rate(ingenic_ost->global_timer_clk);
> > +ÂÂÂ sched_clock_register(ingenic_ost_global_timer_read_cntl, 32, rate);
> > +
> > +ÂÂÂ return 0;
> > +
> > +err_ost_global_timer_cleanup:
> > +ÂÂÂ clocksource_unregister(&ingenic_ost->cs);
> > +ÂÂÂ clk_disable_unprepare(ingenic_ost->global_timer_clk);
> > +ÂÂÂ clk_put(ingenic_ost->global_timer_clk);
> > +err_free_ingenic_ost:
> > +ÂÂÂ kfree(ingenic_ost);
> > +ÂÂÂ return ret;
> > +}
> > +
> > +TIMER_OF_DECLARE(x1000_ost, "ingenic,x1000-ost", ingenic_ost_init);
> > --
> > 2.11.0