Linux-MIPS Archive on lore.kernel.org
 help / color / Atom feed
From: Paul Cercueil <paul@crapouillou.net>
To: "周琰杰 (Zhou Yanjie)" <zhouyanjie@wanyeetech.com>
Cc: linux-mips@vger.kernel.org, linux-clk@vger.kernel.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	mturquette@baylibre.com, sboyd@kernel.org, robh+dt@kernel.org,
	mark.rutland@arm.com, ralf@linux-mips.org, paulburton@kernel.org,
	jiaxun.yang@flygoat.com, chenhc@lemote.com, allison@lohutok.net,
	tglx@linutronix.de, daniel.lezcano@linaro.org,
	geert+renesas@glider.be, krzk@kernel.org, keescook@chromium.org,
	ebiederm@xmission.com, miquel.raynal@bootlin.com,
	paul@boddie.org.uk, hns@goldelico.com, sernia.zhou@foxmail.com,
	zhenwenjin@gmail.com, mips-creator-ci20-dev@googlegroups.com,
	1326991897@qq.com
Subject: Re: [PATCH v4 2/6] clocksource: Ingenic: Add high resolution timer support for SMP.
Date: Fri, 14 Feb 2020 15:31:12 -0300
Message-ID: <1581705072.3.1@crapouillou.net> (raw)
In-Reply-To: <1581703360-112557-4-git-send-email-zhouyanjie@wanyeetech.com>

Hi Zhou,


Le sam., févr. 15, 2020 at 02:02, 周琰杰 (Zhou Yanjie) 
<zhouyanjie@wanyeetech.com> a écrit :
> Enable clock event handling on per CPU core basis.
> Make sure that interrupts raised on the first core execute
> event handlers on the correct CPU core.
> 
> Tested-by: H. Nikolaus Schaller <hns@goldelico.com>
> Tested-by: Paul Boddie <paul@boddie.org.uk>
> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
> ---
> 
> Notes:
>     v1->v2:
>     1.Adjust function naming to make it more reasonable.
>     2.Replace function smp_call_function_single() with
>       smp_call_function_single_async() in order to resolve
>       the warning below:
> 
>     [    0.350942] smp: Brought up 1 node, 2 CPUs
>     [    0.365497] ------------[ cut here ]------------
>     [    0.365522] WARNING: CPU: 0 PID: 1 at kernel/smp.c:300 
> smp_call_function_single+0x110/0x200
>     [    0.365533] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 
> 5.5.0-rc1+ #5
>     [    0.365537] Stack : 00000000 59c73bcd 00000037 80074e80 
> 80000000 80670000 805a0000 80620590
>     [    0.365557]         8065ce38 8fc0dc8c 806d0000 00000000 
> 80670000 00000001 8fc0dc20 59c73bcd
>     [    0.365574]         00000000 00000000 806f0000 80670000 
> 00000000 806dab00 00000000 2d302e35
>     [    0.365591]         203a6d6d 806e0000 806e0000 70617773 
> 80670000 00000000 00000000 00000009
>     [    0.365610]         00000000 8fc94e20 8fc0de30 80690000 
> 00000018 803592dc 00000000 806d0000
>     [    0.365627]         ...
>     [    0.365634] Call Trace:
>     [    0.365647] [<8001b9a0>] show_stack+0x6c/0x12c
>     [    0.365663] [<804aed20>] dump_stack+0x98/0xc8
>     [    0.365673] [<8003044c>] __warn+0xc4/0xe8
>     [    0.365682] [<800304f4>] warn_slowpath_fmt+0x84/0xb8
>     [    0.365690] [<800a886c>] smp_call_function_single+0x110/0x200
>     [    0.365703] ---[ end trace 5785856ca39c79d5 ]---
>     [    0.365557]         8065ce38 8fc0dc8c 806d0000 00000000 
> 80670000 00000001 8fc0dc20 59c73bcd
>     [    0.365574]         00000000 00000000 806f0000 80670000 
> 00000000 806dab00 00000000 2d302e35
>     [    0.365591]         203a6d6d 806e0000 806e0000 70617773 
> 80670000 00000000 00000000 00000009
>     [    0.365610]         00000000 8fc94e20 8fc0de30 80690000 
> 00000018 803592dc 00000000 806d0000
>     [    0.365627]         ...
>     [    0.365634] Call Trace:
>     [    0.365647] [<8001b9a0>] show_stack+0x6c/0x12c
>     [    0.365663] [<804aed20>] dump_stack+0x98/0xc8
>     [    0.365673] [<8003044c>] __warn+0xc4/0xe8
>     [    0.365682] [<800304f4>] warn_slowpath_fmt+0x84/0xb8
>     [    0.365690] [<800a886c>] smp_call_function_single+0x110/0x200
>     [    0.365703] ---[ end trace 5785856ca39c79d5 ]---
> 
>     v2->v3:
>     No Change.
> 
>     v3->v4:
>     Rebase on top of kernel 5.6-rc1.
> 
>  drivers/clocksource/ingenic-timer.c | 200 
> ++++++++++++++++++++++++------------
>  1 file changed, 134 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/clocksource/ingenic-timer.c 
> b/drivers/clocksource/ingenic-timer.c
> index 4bbdb3d..9127c73 100644
> --- a/drivers/clocksource/ingenic-timer.c
> +++ b/drivers/clocksource/ingenic-timer.c
> @@ -1,7 +1,8 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
> - * JZ47xx SoCs TCU IRQ driver
> + * XBurst SoCs TCU IRQ driver
>   * Copyright (C) 2019 Paul Cercueil <paul@crapouillou.net>
> + * Copyright (C) 2020 周琰杰 (Zhou Yanjie) 
> <zhouyanjie@wanyeetech.com>
>   */
> 
>  #include <linux/bitops.h>
> @@ -21,18 +22,23 @@
> 
>  #include <dt-bindings/clock/ingenic,tcu.h>
> 
> +static DEFINE_PER_CPU(call_single_data_t, ingenic_cevt_csd);
> +
>  struct ingenic_soc_info {
>  	unsigned int num_channels;
>  };
> 
>  struct ingenic_tcu {
>  	struct regmap *map;
> +	struct device_node *np;
>  	struct clk *timer_clk, *cs_clk;
> +	unsigned int timer_local[NR_CPUS];
>  	unsigned int timer_channel, cs_channel;
>  	struct clock_event_device cevt;
>  	struct clocksource cs;
> -	char name[4];
> +	char name[16];
>  	unsigned long pwm_channels_mask;
> +	int cpu;
>  };
> 
>  static struct ingenic_tcu *ingenic_tcu;
> @@ -81,6 +87,25 @@ static int ingenic_tcu_cevt_set_next(unsigned long 
> next,
>  	return 0;
>  }
> 
> +static void ingenic_per_cpu_event_handler(void *info)
> +{
> +	struct clock_event_device *cevt = (struct clock_event_device *) 
> info;
> +
> +	if (cevt->event_handler)
> +		cevt->event_handler(cevt);
> +}
> +
> +static void ingenic_tcu_per_cpu_cb(struct clock_event_device *evt)
> +{
> +	struct ingenic_tcu *tcu = to_ingenic_tcu(evt);
> +	call_single_data_t *csd;
> +
> +	csd = &per_cpu(ingenic_cevt_csd, tcu->cpu);
> +	csd->info = (void *) evt;
> +	csd->func = ingenic_per_cpu_event_handler;
> +	smp_call_function_single_async(tcu->cpu, csd);
> +}
> +
>  static irqreturn_t ingenic_tcu_cevt_cb(int irq, void *dev_id)
>  {
>  	struct clock_event_device *evt = dev_id;
> @@ -88,8 +113,7 @@ static irqreturn_t ingenic_tcu_cevt_cb(int irq, 
> void *dev_id)
> 
>  	regmap_write(tcu->map, TCU_REG_TECR, BIT(tcu->timer_channel));
> 
> -	if (evt->event_handler)
> -		evt->event_handler(evt);
> +	ingenic_tcu_per_cpu_cb(evt);

I guess the check for (evt->event_handler) is still valid here. There's 
no point in calling ingenic_tcu_per_cpu_cb() if in the end the handler 
won't do anything.

> 
>  	return IRQ_HANDLED;
>  }
> @@ -105,14 +129,73 @@ static struct clk * __init 
> ingenic_tcu_get_clock(struct device_node *np, int id)
>  	return of_clk_get_from_provider(&args);
>  }
> 
> -static int __init ingenic_tcu_timer_init(struct device_node *np,
> -					 struct ingenic_tcu *tcu)
> +static int __init ingenic_tcu_clocksource_init(struct device_node 
> *np,
> +					       struct ingenic_tcu *tcu)

Please - don't move functions around. It makes the patch extremely hard 
to review. Just add a forward declaration for 
ingenic_tcu_clocksource_init() at the top of the file.

> +{
> +	unsigned int channel = tcu->cs_channel;
> +	struct clocksource *cs = &tcu->cs;
> +	unsigned long rate;
> +	int err;
> +
> +	tcu->cs_clk = ingenic_tcu_get_clock(np, channel);
> +	if (IS_ERR(tcu->cs_clk))
> +		return PTR_ERR(tcu->cs_clk);
> +
> +	err = clk_prepare_enable(tcu->cs_clk);
> +	if (err)
> +		goto err_clk_put;
> +
> +	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 = ingenic_tcu_timer_cs_read;
> +
> +	err = clocksource_register_hz(cs, rate);
> +	if (err)
> +		goto err_clk_disable;
> +
> +	return 0;
> +
> +err_clk_disable:
> +	clk_disable_unprepare(tcu->cs_clk);
> +err_clk_put:
> +	clk_put(tcu->cs_clk);
> +	return err;
> +}
> +
> +static int ingenic_tcu_setup_per_cpu_cevt(struct device_node *np,
> +				     unsigned int channel)
>  {
> -	unsigned int timer_virq, channel = tcu->timer_channel;
> +	unsigned int timer_virq;
>  	struct irq_domain *domain;
> +	struct ingenic_tcu *tcu;
>  	unsigned long rate;
>  	int err;
> 
> +	tcu = kzalloc(sizeof(*tcu), GFP_KERNEL);
> +	if (!tcu)
> +		return -ENOMEM;
> +
> +	tcu->map = ingenic_tcu->map;
> +
>  	tcu->timer_clk = ingenic_tcu_get_clock(np, channel);
>  	if (IS_ERR(tcu->timer_clk))
>  		return PTR_ERR(tcu->timer_clk);
> @@ -139,13 +222,15 @@ static int __init ingenic_tcu_timer_init(struct 
> device_node *np,
>  		goto err_clk_disable;
>  	}
> 
> -	snprintf(tcu->name, sizeof(tcu->name), "TCU");
> +	snprintf(tcu->name, sizeof(tcu->name), "TCU channel.%u", channel);

Make it TCU%u

> 
>  	err = request_irq(timer_virq, ingenic_tcu_cevt_cb, IRQF_TIMER,
>  			  tcu->name, &tcu->cevt);
>  	if (err)
>  		goto err_irq_dispose_mapping;
> 
> +	tcu->cpu = smp_processor_id();
> +	tcu->timer_channel = channel;
>  	tcu->cevt.cpumask = cpumask_of(smp_processor_id());
>  	tcu->cevt.features = CLOCK_EVT_FEAT_ONESHOT;
>  	tcu->cevt.name = tcu->name;
> @@ -166,56 +251,23 @@ static int __init ingenic_tcu_timer_init(struct 
> device_node *np,
>  	return err;
>  }
> 
> -static int __init ingenic_tcu_clocksource_init(struct device_node 
> *np,
> -					       struct ingenic_tcu *tcu)
> +static int ingenic_tcu_setup_cevt(unsigned int cpu)
>  {
> -	unsigned int channel = tcu->cs_channel;
> -	struct clocksource *cs = &tcu->cs;
> -	unsigned long rate;
> -	int err;
> -
> -	tcu->cs_clk = ingenic_tcu_get_clock(np, channel);
> -	if (IS_ERR(tcu->cs_clk))
> -		return PTR_ERR(tcu->cs_clk);
> -
> -	err = clk_prepare_enable(tcu->cs_clk);
> -	if (err)
> -		goto err_clk_put;
> -
> -	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 = ingenic_tcu_timer_cs_read;
> +	int ret;
> 
> -	err = clocksource_register_hz(cs, rate);
> -	if (err)
> -		goto err_clk_disable;
> +	ret = ingenic_tcu_setup_per_cpu_cevt(ingenic_tcu->np,
> +						ingenic_tcu->timer_local[cpu]);
> +	if (ret)
> +		goto err_tcu_clocksource_cleanup;
> 
>  	return 0;
> 
> -err_clk_disable:
> -	clk_disable_unprepare(tcu->cs_clk);
> -err_clk_put:
> -	clk_put(tcu->cs_clk);
> -	return err;
> +err_tcu_clocksource_cleanup:
> +	clocksource_unregister(&ingenic_tcu->cs);
> +	clk_disable_unprepare(ingenic_tcu->cs_clk);
> +	clk_put(ingenic_tcu->cs_clk);
> +	kfree(ingenic_tcu);
> +	return ret;
>  }
> 
>  static const struct ingenic_soc_info jz4740_soc_info = {
> @@ -239,6 +291,7 @@ static int __init ingenic_tcu_init(struct 
> device_node *np)
>  	const struct ingenic_soc_info *soc_info = id->data;
>  	struct ingenic_tcu *tcu;
>  	struct regmap *map;
> +	unsigned cpu = 0;
>  	long rate;
>  	int ret;
> 
> @@ -253,12 +306,14 @@ static int __init ingenic_tcu_init(struct 
> device_node *np)
>  		return -ENOMEM;
> 
>  	/* Enable all TCU channels for PWM use by default except channels 
> 0/1 */
> -	tcu->pwm_channels_mask = GENMASK(soc_info->num_channels - 1, 2);
> +	tcu->pwm_channels_mask = GENMASK(soc_info->num_channels - 1,
> +								NR_CPUS + 1);
>  	of_property_read_u32(np, "ingenic,pwm-channels-mask",
>  			     (u32 *)&tcu->pwm_channels_mask);
> 
>  	/* Verify that we have at least two free channels */
> -	if (hweight8(tcu->pwm_channels_mask) > soc_info->num_channels - 2) {
> +	if (hweight8(tcu->pwm_channels_mask) >
> +			soc_info->num_channels - NR_CPUS + 1) {
>  		pr_crit("%s: Invalid PWM channel mask: 0x%02lx\n", __func__,
>  			tcu->pwm_channels_mask);
>  		ret = -EINVAL;
> @@ -266,13 +321,29 @@ static int __init ingenic_tcu_init(struct 
> device_node *np)
>  	}
> 
>  	tcu->map = map;
> +	tcu->np = np;
>  	ingenic_tcu = tcu;
> 
> -	tcu->timer_channel = find_first_zero_bit(&tcu->pwm_channels_mask,
> +	tcu->timer_local[cpu] = find_first_zero_bit(&tcu->pwm_channels_mask,
>  						 soc_info->num_channels);
> -	tcu->cs_channel = find_next_zero_bit(&tcu->pwm_channels_mask,
> -					     soc_info->num_channels,
> -					     tcu->timer_channel + 1);
> +
> +	if (NR_CPUS > 1) {
> +		for (cpu = 1; cpu < NR_CPUS; cpu++)
> +			tcu->timer_local[cpu] = find_next_zero_bit(
> +						&tcu->pwm_channels_mask,
> +						soc_info->num_channels,
> +						tcu->timer_local[cpu - 1] + 1);
> +
> +		tcu->cs_channel = find_next_zero_bit(&tcu->pwm_channels_mask,
> +					soc_info->num_channels,
> +					tcu->timer_local[cpu-1] + 1);
> +	} else {
> +		tcu->cs_channel = find_next_zero_bit(&tcu->pwm_channels_mask,
> +					soc_info->num_channels,
> +					tcu->timer_local[cpu] + 1);
> +	}
> +
> +
> 
>  	ret = ingenic_tcu_clocksource_init(np, tcu);
>  	if (ret) {
> @@ -280,9 +351,10 @@ static int __init ingenic_tcu_init(struct 
> device_node *np)
>  		goto err_free_ingenic_tcu;
>  	}
> 
> -	ret = ingenic_tcu_timer_init(np, tcu);
> -	if (ret)
> -		goto err_tcu_clocksource_cleanup;
> +	/* Setup clock events on each CPU core */
> +	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "Ingenic XBurst: 
> online",
> +				ingenic_tcu_setup_cevt, NULL);
> +	WARN_ON(ret < 0);
> 
>  	/* Register the sched_clock at the end as there's no way to undo it 
> */
>  	rate = clk_get_rate(tcu->cs_clk);
> @@ -290,10 +362,6 @@ static int __init ingenic_tcu_init(struct 
> device_node *np)
> 
>  	return 0;
> 
> -err_tcu_clocksource_cleanup:
> -	clocksource_unregister(&tcu->cs);
> -	clk_disable_unprepare(tcu->cs_clk);
> -	clk_put(tcu->cs_clk);
>  err_free_ingenic_tcu:
>  	kfree(tcu);
>  	return ret;
> --
> 2.7.4
> 



  reply index

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-14 18:02 Introduce SMP support for CI20 (based on JZ4780) v4 周琰杰 (Zhou Yanjie)
2020-02-14 18:02 ` [PATCH v4 0/6] Introduce SMP support for JZ4780 周琰杰 (Zhou Yanjie)
2020-02-14 18:02 ` [PATCH v4 1/6] MIPS: JZ4780: Introduce SMP support 周琰杰 (Zhou Yanjie)
2020-02-14 18:21   ` Paul Cercueil
2020-02-15 14:03     ` Zhou Yanjie
2020-02-15 15:03   ` afzal mohammed
2020-02-15 16:17     ` Zhou Yanjie
2020-02-14 18:02 ` [PATCH v4 2/6] clocksource: Ingenic: Add high resolution timer support for SMP 周琰杰 (Zhou Yanjie)
2020-02-14 18:31   ` Paul Cercueil [this message]
2020-02-15 14:04     ` Zhou Yanjie
2020-02-14 18:02 ` [PATCH v4 3/6] dt-bindings: MIPS: Document Ingenic SoCs binding 周琰杰 (Zhou Yanjie)
2020-02-14 18:02 ` [PATCH v4 4/6] MIPS: Ingenic: Add 'cpus' node for Ingenic SoCs 周琰杰 (Zhou Yanjie)
2020-02-14 18:02 ` [PATCH v4 5/6] MIPS: CI20: Modify DTS to support high resolution timer for SMP 周琰杰 (Zhou Yanjie)
2020-02-14 18:37   ` Paul Cercueil
2020-02-15 14:05     ` Zhou Yanjie
2020-02-14 18:02 ` [PATCH v4 6/6] MIPS: CI20: Update defconfig to support SMP 周琰杰 (Zhou Yanjie)

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1581705072.3.1@crapouillou.net \
    --to=paul@crapouillou.net \
    --cc=1326991897@qq.com \
    --cc=allison@lohutok.net \
    --cc=chenhc@lemote.com \
    --cc=daniel.lezcano@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=ebiederm@xmission.com \
    --cc=geert+renesas@glider.be \
    --cc=hns@goldelico.com \
    --cc=jiaxun.yang@flygoat.com \
    --cc=keescook@chromium.org \
    --cc=krzk@kernel.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mips@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mips-creator-ci20-dev@googlegroups.com \
    --cc=miquel.raynal@bootlin.com \
    --cc=mturquette@baylibre.com \
    --cc=paul@boddie.org.uk \
    --cc=paulburton@kernel.org \
    --cc=ralf@linux-mips.org \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    --cc=sernia.zhou@foxmail.com \
    --cc=tglx@linutronix.de \
    --cc=zhenwenjin@gmail.com \
    --cc=zhouyanjie@wanyeetech.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-MIPS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mips/0 linux-mips/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mips linux-mips/ https://lore.kernel.org/linux-mips \
		linux-mips@vger.kernel.org
	public-inbox-index linux-mips

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-mips


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git