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=-8.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham 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 27598C3F68F for ; Fri, 14 Feb 2020 16:49:14 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id DC13A2468F for ; Fri, 14 Feb 2020 16:49:13 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="Af6CHTy5"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=st.com header.i=@st.com header.b="I0qwzolz" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org DC13A2468F Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=st.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date: Message-ID:From:References:To:Subject:Reply-To:Content-ID:Content-Description :Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=2llSmikGyOgrUiw9IwnnlsV1kv+Qel5IGESceWptgJM=; b=Af6CHTy5KObDFG jB64ZkNfbxz1LgPYilc928yiNCo5odSddKs8FWyuja3kro8Byg8f/OlohREP+vLRm2g3x0U1B9cVm epq0cMdpLVljp2zSys4I3xd75Iqv/R8bumEBITvgR5gWmKEnc/QExxUyoact+M4vj8dLj1/1H4NHs J4cvqfpAMQXnj2QTOlPs2oF8FA29K2q+i2JAUFxWu9SC6hcvukgZAjb3xN/jYMVtxBWFMzhzlgVAI 4DKz/m0jCeANV3kNoISWyo21xBz6j0YkcpQFpZ9yY9m+TX4NRUhie5QngrVIELo8wOIWJqAvmHd2I VRBtZNhCVavFk9OaLl8Q==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1j2e9J-0003fP-5y; Fri, 14 Feb 2020 16:49:05 +0000 Received: from mx07-00178001.pphosted.com ([62.209.51.94]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1j2dfw-00021b-U0 for linux-arm-kernel@lists.infradead.org; Fri, 14 Feb 2020 16:18:54 +0000 Received: from pps.filterd (m0046037.ppops.net [127.0.0.1]) by mx07-00178001.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 01EGDRWH032306; Fri, 14 Feb 2020 17:18:39 +0100 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=st.com; h=subject : to : cc : references : from : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=STMicroelectronics; bh=167O7uzVAnLo+b8RedO68+CO5iVI8gwV++GYvThY8tE=; b=I0qwzolzmvw5pmkijtKd50sajybNnAZP/wLhxg1MnkDyPQSTjAWRW9VAFofGtYWp4Bbf J5oB3eGXIPFgBUFZiDXQ33wSBcPTwkUkGP/rbX5MFbP4AvrWdVs5QYhr66Gh5iKtr7Lv 18eQMdUFLh9K7UKSCpsW7RrJYvdRR4QnokZusjrzC0iyzglUCdJX5D04O2ro4wbUECoS jEoujRogcD+TSOI2KYWMF6RzNRvedT71D0DdjGyGSrw2xK2vAYTBig3a0Ni6XOK0Eduo u/YChjE1Vo5oYGdC6GLc4ht2x+S9vGwmBSvw64JgOWTWEiYTyxbukodthMv162c+tFah +w== Received: from beta.dmz-eu.st.com (beta.dmz-eu.st.com [164.129.1.35]) by mx07-00178001.pphosted.com with ESMTP id 2y1urhvw51-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 14 Feb 2020 17:18:39 +0100 Received: from euls16034.sgp.st.com (euls16034.sgp.st.com [10.75.44.20]) by beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id C2DC1100039; Fri, 14 Feb 2020 17:18:36 +0100 (CET) Received: from Webmail-eu.st.com (sfhdag5node3.st.com [10.75.127.15]) by euls16034.sgp.st.com (STMicroelectronics) with ESMTP id AFF762BEC46; Fri, 14 Feb 2020 17:18:36 +0100 (CET) Received: from [10.48.0.71] (10.75.127.45) by SFHDAG5NODE3.st.com (10.75.127.15) with Microsoft SMTP Server (TLS) id 15.0.1473.3; Fri, 14 Feb 2020 17:18:35 +0100 Subject: Re: [PATCH v2] iio: trigger: stm32-timer: enable clock when in master mode To: Jonathan Cameron References: <1581093031-9871-1-git-send-email-fabrice.gasnier@st.com> <20200214142035.576e11e1@archlinux> <3a9dfc05-f6c3-a283-5791-5d9b72bb44f7@st.com> <20200214151408.570c1658@archlinux> From: Fabrice Gasnier Message-ID: Date: Fri, 14 Feb 2020 17:18:35 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: <20200214151408.570c1658@archlinux> Content-Language: en-US X-Originating-IP: [10.75.127.45] X-ClientProxiedBy: SFHDAG1NODE3.st.com (10.75.127.3) To SFHDAG5NODE3.st.com (10.75.127.15) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.138, 18.0.572 definitions=2020-02-14_04:2020-02-12, 2020-02-14 signatures=0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200214_081845_762315_8897F815 X-CRM114-Status: GOOD ( 27.72 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: lars@metafoo.de, olivier.moysan@st.com, alexandre.torgue@st.com, linux-iio@vger.kernel.org, pmeerw@pmeerw.net, linux-kernel@vger.kernel.org, mcoquelin.stm32@gmail.com, knaack.h@gmx.de, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, benjamin.gaignard@st.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 2/14/20 4:14 PM, Jonathan Cameron wrote: > On Fri, 14 Feb 2020 15:50:57 +0100 > Fabrice Gasnier wrote: > >> On 2/14/20 3:20 PM, Jonathan Cameron wrote: >>> On Fri, 7 Feb 2020 17:30:31 +0100 >>> Fabrice Gasnier wrote: >>> >>>> Clock should be enabled as soon as using master modes, even before >>>> enabling timer. Or, this may provoke bad behavior on the other end >>>> (slave timer). Then, introduce 'clk_enabled' flag, instead of relying >>>> on CR1 EN bit, to keep track of clock being enabled (balanced refcount). >>>> Propagate this anywhere else in the driver. >>>> >>>> Also add 'remove' routine to stop timer and disable clock in case it >>>> has been left enabled. Enforce the user interface has been unregistered >>>> in the remove routine, before disabling the hardware to avoid possible >>>> race. So, remove use of devm_ variant to register triggers and unregister >>>> them before the hardware gets disabled [1]. >>>> [1] https://patchwork.kernel.org/patch/9956247/ >>>> >>>> Signed-off-by: Fabrice Gasnier >>> Patch looks fine. So is this a fix for a known issue or a theoretical one? >>> Just a question of whether to line it up for an rc, or wait for next >>> merge window. Ideally please give me a fixes tag as well if this >>> fixes a problem that is being seen in the wild. >> >> Hi Jonathan, >> >> I got no complain for now. So I think this is fine to wait for the next >> merge window. > Hi Fabrice, > > Great. Unfortunately this has crossed with other changes on the driver. > Would you mind doing a rebase on my testing branch? It looked fairly > simple but I wasn't totally confident to do it without being able > to test! Yes, I'll do this shortly ;-) Thanks, Fabrice > > Thanks, > > Jonathan > >> >> Thanks, >> Fabrice >> >>> >>> Thanks, >>> >>> Jonathan >>> >>>> --- >>>> Changes in v2: >>>> - enforce the user interface has been unregistered in the remove routine, >>>> before disabling the hardware to avoid possible race. >>>> --- >>>> drivers/iio/trigger/stm32-timer-trigger.c | 98 ++++++++++++++++++++++++------- >>>> 1 file changed, 76 insertions(+), 22 deletions(-) >>>> >>>> diff --git a/drivers/iio/trigger/stm32-timer-trigger.c b/drivers/iio/trigger/stm32-timer-trigger.c >>>> index a5dfe65..473853e 100644 >>>> --- a/drivers/iio/trigger/stm32-timer-trigger.c >>>> +++ b/drivers/iio/trigger/stm32-timer-trigger.c >>>> @@ -79,10 +79,13 @@ struct stm32_timer_trigger { >>>> struct device *dev; >>>> struct regmap *regmap; >>>> struct clk *clk; >>>> + bool clk_enabled; >>>> u32 max_arr; >>>> const void *triggers; >>>> const void *valids; >>>> bool has_trgo2; >>>> + struct mutex lock; /* concurrent sysfs configuration */ >>>> + struct list_head tr_list; >>>> }; >>>> >>>> struct stm32_timer_trigger_cfg { >>>> @@ -106,7 +109,7 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv, >>>> { >>>> unsigned long long prd, div; >>>> int prescaler = 0; >>>> - u32 ccer, cr1; >>>> + u32 ccer; >>>> >>>> /* Period and prescaler values depends of clock rate */ >>>> div = (unsigned long long)clk_get_rate(priv->clk); >>>> @@ -136,9 +139,11 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv, >>>> if (ccer & TIM_CCER_CCXE) >>>> return -EBUSY; >>>> >>>> - regmap_read(priv->regmap, TIM_CR1, &cr1); >>>> - if (!(cr1 & TIM_CR1_CEN)) >>>> + mutex_lock(&priv->lock); >>>> + if (!priv->clk_enabled) { >>>> + priv->clk_enabled = true; >>>> clk_enable(priv->clk); >>>> + } >>>> >>>> regmap_write(priv->regmap, TIM_PSC, prescaler); >>>> regmap_write(priv->regmap, TIM_ARR, prd - 1); >>>> @@ -157,22 +162,20 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv, >>>> >>>> /* Enable controller */ >>>> regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, TIM_CR1_CEN); >>>> + mutex_unlock(&priv->lock); >>>> >>>> return 0; >>>> } >>>> >>>> static void stm32_timer_stop(struct stm32_timer_trigger *priv) >>>> { >>>> - u32 ccer, cr1; >>>> + u32 ccer; >>>> >>>> regmap_read(priv->regmap, TIM_CCER, &ccer); >>>> if (ccer & TIM_CCER_CCXE) >>>> return; >>>> >>>> - regmap_read(priv->regmap, TIM_CR1, &cr1); >>>> - if (cr1 & TIM_CR1_CEN) >>>> - clk_disable(priv->clk); >>>> - >>>> + mutex_lock(&priv->lock); >>>> /* Stop timer */ >>>> regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE, 0); >>>> regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, 0); >>>> @@ -181,6 +184,12 @@ static void stm32_timer_stop(struct stm32_timer_trigger *priv) >>>> >>>> /* Make sure that registers are updated */ >>>> regmap_update_bits(priv->regmap, TIM_EGR, TIM_EGR_UG, TIM_EGR_UG); >>>> + >>>> + if (priv->clk_enabled) { >>>> + priv->clk_enabled = false; >>>> + clk_disable(priv->clk); >>>> + } >>>> + mutex_unlock(&priv->lock); >>>> } >>>> >>>> static ssize_t stm32_tt_store_frequency(struct device *dev, >>>> @@ -295,11 +304,18 @@ static ssize_t stm32_tt_store_master_mode(struct device *dev, >>>> for (i = 0; i <= master_mode_max; i++) { >>>> if (!strncmp(master_mode_table[i], buf, >>>> strlen(master_mode_table[i]))) { >>>> + mutex_lock(&priv->lock); >>>> + if (!priv->clk_enabled) { >>>> + /* Clock should be enabled first */ >>>> + priv->clk_enabled = true; >>>> + clk_enable(priv->clk); >>>> + } >>>> regmap_update_bits(priv->regmap, TIM_CR2, mask, >>>> i << shift); >>>> /* Make sure that registers are updated */ >>>> regmap_update_bits(priv->regmap, TIM_EGR, >>>> TIM_EGR_UG, TIM_EGR_UG); >>>> + mutex_unlock(&priv->lock); >>>> return len; >>>> } >>>> } >>>> @@ -357,11 +373,21 @@ static const struct attribute_group *stm32_trigger_attr_groups[] = { >>>> static const struct iio_trigger_ops timer_trigger_ops = { >>>> }; >>>> >>>> -static int stm32_setup_iio_triggers(struct stm32_timer_trigger *priv) >>>> +static void stm32_unregister_iio_triggers(struct stm32_timer_trigger *priv) >>>> +{ >>>> + struct iio_trigger *tr; >>>> + >>>> + list_for_each_entry(tr, &priv->tr_list, alloc_list) >>>> + iio_trigger_unregister(tr); >>>> +} >>>> + >>>> +static int stm32_register_iio_triggers(struct stm32_timer_trigger *priv) >>>> { >>>> int ret; >>>> const char * const *cur = priv->triggers; >>>> >>>> + INIT_LIST_HEAD(&priv->tr_list); >>>> + >>>> while (cur && *cur) { >>>> struct iio_trigger *trig; >>>> bool cur_is_trgo = stm32_timer_is_trgo_name(*cur); >>>> @@ -388,9 +414,13 @@ static int stm32_setup_iio_triggers(struct stm32_timer_trigger *priv) >>>> >>>> iio_trigger_set_drvdata(trig, priv); >>>> >>>> - ret = devm_iio_trigger_register(priv->dev, trig); >>>> - if (ret) >>>> + ret = iio_trigger_register(trig); >>>> + if (ret) { >>>> + stm32_unregister_iio_triggers(priv); >>>> return ret; >>>> + } >>>> + >>>> + list_add_tail(&trig->alloc_list, &priv->tr_list); >>>> cur++; >>>> } >>>> >>>> @@ -437,7 +467,6 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev, >>>> int val, int val2, long mask) >>>> { >>>> struct stm32_timer_trigger *priv = iio_priv(indio_dev); >>>> - u32 dat; >>>> >>>> switch (mask) { >>>> case IIO_CHAN_INFO_RAW: >>>> @@ -448,19 +477,23 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev, >>>> return -EINVAL; >>>> >>>> case IIO_CHAN_INFO_ENABLE: >>>> + mutex_lock(&priv->lock); >>>> if (val) { >>>> - regmap_read(priv->regmap, TIM_CR1, &dat); >>>> - if (!(dat & TIM_CR1_CEN)) >>>> + if (!priv->clk_enabled) { >>>> + priv->clk_enabled = true; >>>> clk_enable(priv->clk); >>>> + } >>>> regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, >>>> TIM_CR1_CEN); >>>> } else { >>>> - regmap_read(priv->regmap, TIM_CR1, &dat); >>>> regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, >>>> 0); >>>> - if (dat & TIM_CR1_CEN) >>>> + if (priv->clk_enabled) { >>>> + priv->clk_enabled = false; >>>> clk_disable(priv->clk); >>>> + } >>>> } >>>> + mutex_unlock(&priv->lock); >>>> return 0; >>>> } >>>> >>>> @@ -556,7 +589,6 @@ static int stm32_set_enable_mode(struct iio_dev *indio_dev, >>>> { >>>> struct stm32_timer_trigger *priv = iio_priv(indio_dev); >>>> int sms = stm32_enable_mode2sms(mode); >>>> - u32 val; >>>> >>>> if (sms < 0) >>>> return sms; >>>> @@ -564,11 +596,12 @@ static int stm32_set_enable_mode(struct iio_dev *indio_dev, >>>> * Triggered mode sets CEN bit automatically by hardware. So, first >>>> * enable counter clock, so it can use it. Keeps it in sync with CEN. >>>> */ >>>> - if (sms == 6) { >>>> - regmap_read(priv->regmap, TIM_CR1, &val); >>>> - if (!(val & TIM_CR1_CEN)) >>>> - clk_enable(priv->clk); >>>> + mutex_lock(&priv->lock); >>>> + if (sms == 6 && !priv->clk_enabled) { >>>> + clk_enable(priv->clk); >>>> + priv->clk_enabled = true; >>>> } >>>> + mutex_unlock(&priv->lock); >>>> >>>> regmap_update_bits(priv->regmap, TIM_SMCR, TIM_SMCR_SMS, sms); >>>> >>>> @@ -752,8 +785,9 @@ static int stm32_timer_trigger_probe(struct platform_device *pdev) >>>> priv->triggers = triggers_table[index]; >>>> priv->valids = cfg->valids_table[index]; >>>> stm32_timer_detect_trgo2(priv); >>>> + mutex_init(&priv->lock); >>>> >>>> - ret = stm32_setup_iio_triggers(priv); >>>> + ret = stm32_register_iio_triggers(priv); >>>> if (ret) >>>> return ret; >>>> >>>> @@ -762,6 +796,25 @@ static int stm32_timer_trigger_probe(struct platform_device *pdev) >>>> return 0; >>>> } >>>> >>>> +static int stm32_timer_trigger_remove(struct platform_device *pdev) >>>> +{ >>>> + struct stm32_timer_trigger *priv = platform_get_drvdata(pdev); >>>> + u32 val; >>>> + >>>> + /* Unregister triggers before everything can be safely turned off */ >>>> + stm32_unregister_iio_triggers(priv); >>>> + >>>> + /* Check if nobody else use the timer, then disable it */ >>>> + regmap_read(priv->regmap, TIM_CCER, &val); >>>> + if (!(val & TIM_CCER_CCXE)) >>>> + regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, 0); >>>> + >>>> + if (priv->clk_enabled) >>>> + clk_disable(priv->clk); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static const struct stm32_timer_trigger_cfg stm32_timer_trg_cfg = { >>>> .valids_table = valids_table, >>>> .num_valids_table = ARRAY_SIZE(valids_table), >>>> @@ -786,6 +839,7 @@ MODULE_DEVICE_TABLE(of, stm32_trig_of_match); >>>> >>>> static struct platform_driver stm32_timer_trigger_driver = { >>>> .probe = stm32_timer_trigger_probe, >>>> + .remove = stm32_timer_trigger_remove, >>>> .driver = { >>>> .name = "stm32-timer-trigger", >>>> .of_match_table = stm32_trig_of_match, >>> > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel