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 X-Spam-Level: X-Spam-Status: No, score=-5.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 900BFC3E8C5 for ; Mon, 30 Nov 2020 00:36:38 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 0790420657 for ; Mon, 30 Nov 2020 00:36:37 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="cUCc8EPD" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0790420657 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=simonsouth.net Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-rockchip-bounces+linux-rockchip=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:Message-ID:In-Reply-To:Date:References: Subject:To:From:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=UoG4r2VNYUDYALeOZVa4Fk2aMmxTPPt2mcgwHIZv6+Q=; b=cUCc8EPDlkMBGXlWbocjQMtKU Us+yBngrGxrYtaPEuDT2QbUJeU267G5L9KRHohSsooRUKBe+v/g92JojhU+3Q5Q9plPUhYBQNdbfZ lWuH6GjTEqVCrtoAxs4CY5FdycqWRiNFMqJKvECd1tn9Cga6+dnRkOI+NhnpOWVAfyExQvHjAuWyr IpvYlACnnkAZRxNAwsTIiV+LZsVDM5ZLQY+6W815ouhJn4P0b5cN5moiR8LINJbpSv6vzzNj0d1c2 +1kL+L/FIHEQ97Z+qZ7biJJ4WfBPayZI/S59vNdReJLkpEKhDu1MoWtj9LCY++Dw090ryDkJ3drCr MhHW7FJDg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kjXB6-0006Rc-GV; Mon, 30 Nov 2020 00:36:28 +0000 Received: from mailout.easymail.ca ([64.68.200.34]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kjXB3-0006R6-Bn; Mon, 30 Nov 2020 00:36:26 +0000 Received: from localhost (localhost [127.0.0.1]) by mailout.easymail.ca (Postfix) with ESMTP id 4C519C00B0; Mon, 30 Nov 2020 00:36:22 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at emo04-pco.easydns.vpn Received: from mailout.easymail.ca ([127.0.0.1]) by localhost (emo04-pco.easydns.vpn [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 1V6n0WLH-Fzm; Mon, 30 Nov 2020 00:36:22 +0000 (UTC) Received: from jupiter (unknown [108.162.141.195]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mailout.easymail.ca (Postfix) with ESMTPSA id 6D7F3BFE42; Mon, 30 Nov 2020 00:36:10 +0000 (UTC) From: Simon South To: Trent Piepho Subject: Re: [PATCH v2] pwm: rockchip: Keep enabled PWMs running while probing References: <20200919193306.1023-1-simon@simonsouth.net> <2177360.ElGaqSPkdT@zen.local> Date: Sun, 29 Nov 2020 19:36:09 -0500 In-Reply-To: <2177360.ElGaqSPkdT@zen.local> (Trent Piepho's message of "Fri, 20 Nov 2020 17:09:58 -0800") Message-ID: <875z5nof46.fsf@simonsouth.net> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201129_193625_418154_23E2D6BC X-CRM114-Status: GOOD ( 25.50 ) X-BeenThere: linux-rockchip@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Upstream kernel work for Rockchip platforms List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-pwm@vger.kernel.org, heiko@sntech.de, Boris Brezillon , linux-rockchip@lists.infradead.org, thierry.reding@gmail.com, u.kleine-koenig@pengutronix.de, lee.jones@linaro.org, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+linux-rockchip=archiver.kernel.org@lists.infradead.org Trent Piepho writes: > Anyway, it seems like this solution has a race. Isn't the pwm live > and requestable as soon as pwmchip_add() returns? Which would mean > that disabling the clock here could race with other code requesting > and enabling the pwm. Yes, I think that's true. Glad you caught this. > Seems like it would be safer to check the initial state and turn off > the clock before calling pwmchip_add. Yes. I tested this and it works, but on further consideration it seems to me the code is actually doing things backwards: Instead of enabling every PWM's clock and then turning off the clocks for PWMs that are not themselves enabled, it should enable only the clocks for PWMs that appear to be in use and leave the remaining clocks in their default (presumably disabled) state. This should produce the same end result but without the driver enabling clocks indiscriminately and without creating a race condition. I'll follow up with a patch for review that implements this change. I've tested it as best I can on my own Pinebook Pro; everything works as it did previously, with the display backlight on, no kernel hang and no other apparent side effects. > It seems like the issue is the driver was calling pwm_is_enabled() > without first requesting the pwm with pwm_get(). This used to work because pwmchip_add() happened to invoke rockchip_pwm_get_state(), which populates the PWM's pwm_state structure from which pwm_is_enabled() reads. And I think that's why the code was written the way it was originally: By waiting until pwmchip_add() returned the PWM's state could be queried in a convenient manner, without resorting to peeking at the hardware's registers. Commit cfc4c189bc70 ("pwm: Read initial hardware state at request time") changed this and pwmchip_add() no longer has the side effect of populating the state structure, so the call to pwm_is_enabled() no longer worked reliably. That's what I fixed with the patch you're replying to, though now I wish I'd seen the larger issue. As to why this code is needed in the first place (as I wondered recently while working on it again), it seems to be a somewhat hacky way of initializing the enable_count reference counter (see drivers/clk/clk.c) of the already running clock to 1 instead of 0. This is necessary because on SoCs like the RK3399 that use only a single clock for each PWM, the driver treats the "APB" and "bus" clocks as referring to the same physical device ("pc->pclk = pc->clk"), and without it functions like rockchip_pwm_get_state() that enable the APB clock on entry and disable it on exit would end up halting the PWM entirely. -- Simon South simon@simonsouth.net _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip