From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 6B301C54798 for ; Tue, 27 Feb 2024 16:33:13 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:Message-ID:Date:References :In-Reply-To:Subject:Cc:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=tjlG692egYSL/8xyf2F8MIOUmkWnuD/LtqaUX9aR1c0=; b=cVGaek4drMIswU nDGfAjFTRRo1uaacABo1iAaZf6lRWiamjd+Ysj5k2wfw5NwoYt8sR7/KjY20iNE6jRX8/tUbLR/aZ VmDb5iyN9WLMmqDBtNko1dtsTDyBhc//Zl0ACjUVK55sl5FEOZuMUrFqCwrAN/DjO/orsDE3qrYIl 50KYP+hV3J0V/IKG++dug71Vbm8aWcAewBV0goCnDuyHd142yAqhOujhgtJEY8z7xz8oy4yoV3WKE JRbpDYgBIBdZUjbEbQJOr22LOeX+Pvh9lD6+xuf1HatC6h8SCt3LrXNoUeEjG44RC7KJMj1o4posf SXWIb7Q5/HpHt89QHpOQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rf0OC-00000005zO9-1uQ9; Tue, 27 Feb 2024 16:33:08 +0000 Received: from galois.linutronix.de ([193.142.43.55]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rf0O8-00000005zLn-1QlW for linux-riscv@lists.infradead.org; Tue, 27 Feb 2024 16:33:07 +0000 From: Thomas Gleixner DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1709051574; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=SyV9bHeQYh2STSu1Aki+bC1PiR0RG4rZPyWUkh6MdoI=; b=R0+sMRFv9b8X+E5PlHqAnffQ9ul2R0l+89oJsQQqPEMzobmGax2CGW1kgeoxp2g7FE7y/H ZduHQ6oyjWkm9ZA5CLqotQqDCEtKhwhNagXbhtZj6x2P+eXwHgq8bn9YKdtPqmPMLZ9XkY 0M9kSqf3ORH7ZF9SDePLRmHMbqIQkX3kvhftVrbUGqMyDdK+NZuwrz7YR5i9fjoBUyHlEZ SK+iNEKoWW3HFdwTwutyndjzwqx2Aso9GloWF+coyojZ7RwUTRw16z46SpvLt8Du7dXC26 ttqFrpCCC/bMioXhJFtXB4Q09O1kXf9MzyxWlYi+Q24GgLZuajw5i7BshrGUfw== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1709051574; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=SyV9bHeQYh2STSu1Aki+bC1PiR0RG4rZPyWUkh6MdoI=; b=mfiDY1/jYFYVkT3N2Y3yq9fwkvbnzRzZ3jJHlKwl01nm/DAsIpCmKZZwXdjDRmVLCuGQl6 9QNGSBRBpVnBP7Cg== To: Xingyu Wu , Daniel Lezcano , Emil Renner Berthing , Christophe JAILLET Cc: linux-riscv@lists.infradead.org, devicetree@vger.kernel.org, Rob Herring , Krzysztof Kozlowski , Paul Walmsley , Palmer Dabbelt , Albert Ou , Philipp Zabel , Walker Chen , Xingyu Wu , linux-kernel@vger.kernel.org, Conor Dooley Subject: Re: [PATCH v8 2/3] clocksource: Add JH7110 timer driver In-Reply-To: <20231219145402.7879-3-xingyu.wu@starfivetech.com> References: <20231219145402.7879-1-xingyu.wu@starfivetech.com> <20231219145402.7879-3-xingyu.wu@starfivetech.com> Date: Tue, 27 Feb 2024 17:32:54 +0100 Message-ID: <877cipamvd.ffs@tglx> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240227_083304_692465_4A7A9F02 X-CRM114-Status: GOOD ( 18.02 ) X-BeenThere: linux-riscv@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-riscv" Errors-To: linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org On Tue, Dec 19 2023 at 22:54, Xingyu Wu wrote: > + > +struct jh7110_clkevt { > + struct clk *clk; > + struct reset_control *rst; > + void __iomem *base; > + u32 reload_val; > + int irq; > + char name[sizeof("jh7110-timer.chX")]; > +}; > + > +struct jh7110_timer_priv { > + struct reset_control *prst; > + struct device *dev; > + struct jh7110_clkevt clkevt[JH7110_TIMER_CH_MAX]; > +}; Please format your data structures according to documentation: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers > +/* IRQ handler for the timer */ > +static irqreturn_t jh7110_timer_interrupt(int irq, void *data) > +{ > + struct clock_event_device *evt = data; > + struct jh7110_clkevt *clkevt = &jh7110_timer_info.clkevt[0]; > + u8 cpu_id = smp_processor_id(); > + u32 reg = readl(clkevt->base + JH7110_TIMER_INT_STATUS); https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#variable-declarations > + /* Check interrupt status and channel(cpu) ID */ > + if (!(reg & BIT(cpu_id))) > + return IRQ_NONE; > + > + clkevt = &jh7110_timer_info.clkevt[cpu_id]; > + writel(JH7110_TIMER_INT_CLR_ENA, (clkevt->base + JH7110_TIMER_INT_CLR)); > + > + if (evt->event_handler) > + evt->event_handler(evt); > + > + return IRQ_HANDLED; > +} > + > +static int jh7110_timer_set_periodic(struct clock_event_device *evt) > +{ > + struct jh7110_clkevt *clkevt = JH7110_PERCPU_GET_CLKEVT; > + > + writel(JH7110_TIMER_MODE_CONTIN, clkevt->base + JH7110_TIMER_CTL); > + return 0; > +} > + > +static int jh7110_timer_set_oneshot(struct clock_event_device *evt) > +{ > + struct jh7110_clkevt *clkevt = JH7110_PERCPU_GET_CLKEVT; > + > + writel(JH7110_TIMER_MODE_SINGLE, clkevt->base + JH7110_TIMER_CTL); > + return 0; > +} > + > +static int jh7110_timer_set_next_event(unsigned long next, > + struct clock_event_device *evt) > +{ > + struct jh7110_clkevt *clkevt = JH7110_PERCPU_GET_CLKEVT; > + > + writel(JH7110_TIMER_MODE_SINGLE, clkevt->base + JH7110_TIMER_CTL); > + writel(next, clkevt->base + JH7110_TIMER_LOAD); > + > + return jh7110_timer_start(clkevt); > +} > + > +static DEFINE_PER_CPU(struct clock_event_device, jh7110_clock_event) = { > + .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, > + .rating = JH7110_CLOCKEVENT_RATING, > + .set_state_shutdown = jh7110_timer_shutdown, > + .set_state_periodic = jh7110_timer_set_periodic, > + .set_state_oneshot = jh7110_timer_set_oneshot, > + .set_state_oneshot_stopped = jh7110_timer_shutdown, > + .tick_resume = jh7110_timer_tick_resume, > + .set_next_event = jh7110_timer_set_next_event, > + .suspend = jh7110_timer_suspend, > + .resume = jh7110_timer_resume, > +}; See formatting rules. > +static int jh7110_timer_starting_cpu(unsigned int cpu) > +{ > + struct clock_event_device *evt = per_cpu_ptr(&jh7110_clock_event, cpu); > + struct jh7110_timer_priv *priv = &jh7110_timer_info; > + int ret; > + u32 rate; > + > + if (cpu >= JH7110_TIMER_CH_MAX) > + return -ENOMEM; > + > + ret = clk_prepare_enable(priv->clkevt[cpu].clk); > + if (ret) > + return ret; > + > + ret = reset_control_deassert(priv->clkevt[cpu].rst); > + if (ret) > + return ret; > + > + rate = clk_get_rate(priv->clkevt[cpu].clk); > + evt->cpumask = cpumask_of(cpu); > + evt->irq = priv->clkevt[cpu].irq; > + evt->name = priv->clkevt[cpu].name; > + clockevents_config_and_register(evt, rate, JH7110_TIMER_MIN_TICKS, > + JH7110_TIMER_MAX_TICKS); > + > + ret = devm_request_irq(priv->dev, evt->irq, jh7110_timer_interrupt, > + IRQF_TIMER | IRQF_IRQPOLL, > + evt->name, evt); How is this all supposed to work from a CPU hotplug state callback which runs in the early bringup phase with interrupts disabled? clk_prepare() which is invoked from clk_prepare_enable() can sleep and devm_request_irq() can sleep too. Did you ever test this with the required debug config options enabled? https://www.kernel.org/doc/html/latest/process/submit-checklist.html Obviously not. > + if (ret) > + return ret; > + > + ret = irq_set_affinity(evt->irq, cpumask_of(cpu)); > + if (ret) > + return ret; > + /* Use one-shot mode */ > + writel(JH7110_TIMER_MODE_SINGLE, (priv->clkevt[cpu].base + JH7110_TIMER_CTL)); > + > + return jh7110_timer_start(&priv->clkevt[cpu]); > +} > + > +static void jh7110_timer_release(void *data) > +{ > + struct jh7110_timer_priv *priv = data; > + int i; > + > + for (i = 0; i < JH7110_TIMER_CH_MAX; i++) { > + /* Disable each channel of timer */ > + if (priv->clkevt[i].base) > + writel(JH7110_TIMER_DIS, priv->clkevt[i].base + JH7110_TIMER_ENABLE); > + > + /* Avoid no initialization in the loop of the probe */ > + if (!IS_ERR_OR_NULL(priv->clkevt[i].rst)) > + reset_control_assert(priv->clkevt[i].rst); > + > + if (!IS_ERR_OR_NULL(priv->clkevt[i].clk)) > + clk_disable_unprepare(priv->clkevt[i].clk); Same problem. And of course this does not undo the other steps of jh7110_timer_starting_cpu() so if you offline a CPU and then online it again the callback will fail because the clockevent is already registered and the interrupt requested. Clearly untested too. Thanks, tglx _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv