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=-3.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED,USER_AGENT_NEOMUTT 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 DA10DC43381 for ; Wed, 20 Mar 2019 10:57:56 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id A56C62175B for ; Wed, 20 Mar 2019 10:57:56 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727891AbfCTK5z (ORCPT ); Wed, 20 Mar 2019 06:57:55 -0400 Received: from metis.ext.pengutronix.de ([85.220.165.71]:53733 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727363AbfCTK5y (ORCPT ); Wed, 20 Mar 2019 06:57:54 -0400 Received: from pty.hi.pengutronix.de ([2001:67c:670:100:1d::c5]) by metis.ext.pengutronix.de with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89) (envelope-from ) id 1h6Yum-0001P8-FD; Wed, 20 Mar 2019 11:57:44 +0100 Received: from ukl by pty.hi.pengutronix.de with local (Exim 4.89) (envelope-from ) id 1h6Yuk-0008QV-Se; Wed, 20 Mar 2019 11:57:42 +0100 Date: Wed, 20 Mar 2019 11:57:42 +0100 From: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= To: Anson Huang Cc: "thierry.reding@gmail.com" , "robh+dt@kernel.org" , "mark.rutland@arm.com" , "shawnguo@kernel.org" , "s.hauer@pengutronix.de" , "kernel@pengutronix.de" , "festevam@gmail.com" , "linux@armlinux.org.uk" , "otavio@ossystems.com.br" , "stefan@agner.ch" , Leonard Crestez , "schnitzeltony@gmail.com" , Robin Gong , "linux-pwm@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , dl-linux-imx Subject: Re: [PATCH V7 2/5] pwm: Add i.MX TPM PWM driver support Message-ID: <20190320105742.jdabsbcoebbyftqr@pengutronix.de> References: <1553058067-18793-1-git-send-email-Anson.Huang@nxp.com> <1553058067-18793-3-git-send-email-Anson.Huang@nxp.com> <20190320081856.wkw55pmw57i4ifdj@pengutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c5 X-SA-Exim-Mail-From: ukl@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello Anson, On Wed, Mar 20, 2019 at 10:17:50AM +0000, Anson Huang wrote: > > On Wed, Mar 20, 2019 at 05:06:21AM +0000, Anson Huang wrote: > > > + /* make sure counter is disabled for programming prescale */ > > > + val = readl(tpm->base + PWM_IMX_TPM_SC); > > > + saved_cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val); > > > + if (saved_cmod) { > > > + val &= ~PWM_IMX_TPM_SC_CMOD; > > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > > > I thought we agreed on not stopping the counter if the PS field isn't changed? > > If the PS field no need to change, the round state should already return the period > equal to current period settings, so this function will NOT be called, right? > > if (real_state.period != tpm->real_period) { > if (tpm->user_count > 1) { > ret = -EBUSY; > goto exit; > } > > pwm_imx_tpm_setup_period(chip, param); > tpm->real_period = real_state.period; > } If the PS field is already right .period might still not match as its value depends on SC.PS and MOD.MOD. > > > + val &= ~PWM_IMX_TPM_SC_PS; > > > + val |= FIELD_PREP(PWM_IMX_TPM_SC_PS, p.prescale); > > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > > + > > > + /* > > > + * set period count: according to RM, the MOD register is > > > + * updated immediately after CMOD[1:0] = 2b'00 above > > > + */ > > > > So the current period isn't completed? That's wrong. > > So you mean we have to wait for the current period to finish here? > I did NOT know this, so we have to compare the counter value with Yeah, see https://patchwork.ozlabs.org/patch/1021566/ which documents this but waits for feedback by Thierry since some time. > the MOD value until they match then proceed the period change? If the hardware doesn't support you here (usually it does) I think it's not reliable enough to try to sync in software. I assume you can get the right wave form if you don't change SC.PS but only MOD.MOD? So maybe the sanest approach is to refuse changing SC.PS if the PWM is running. Or disable (which also should wait for the current period to finish), poll for the end (or use an irq?) and then setup the new configuration. > > > +{ > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > > + struct pwm_state c; > > > + u32 val, sc_val; > > > + u64 tmp; > > > + > > > + pwm_imx_tpm_get_state(chip, pwm, &c); > > > + > > > + if (state.duty_cycle != c.duty_cycle) { > > > + /* set duty counter */ > > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) & PWM_IMX_TPM_MOD_MOD; > > > + tmp *= state.duty_cycle; > > > + val = DIV_ROUND_CLOSEST_ULL(tmp, state.period); > > > + writel(val, tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)); > > > + } > > > + > > > + if (state.enabled != c.enabled) { > > > > This is wrong. If the PWM was running (c.enabled == true) and you are > > supposed to disable (state.enabled == false) you enable the hardware once > > more. > > A little confused here, as the case you assume, inside this block, there is another > check for state.enabled, if it is false, the polarity will be set to channel disabled mode, > the polarity setting is combined together with the enable status here, am I wrong? Ah, you're right. I missed that probably because I saw register accesses after the state.enabled != c.enabled check. > > > + val |= (state.polarity == PWM_POLARITY_NORMAL) ? > > > + FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x2) : > > > + FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x1); > > > > Introduce PWM_IMX_TPM_CnSC_ELS_POLARITY_NORMAL and use it > > together with PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED here. If you > > put the FIELD_PREP into the definition the line doesn't get excessively long. > > > > I put the FIELD_PREP into definition, the line still long, but I agree using definition is better. > > #define PWM_IMX_TPM_CnSC_ELS_INVERSED FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 1) > #define PWM_IMX_TPM_CnSC_ELS_NORMAL FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 2) > > val |= (state->polarity == PWM_POLARITY_NORMAL) ? > PWM_IMX_TPM_CnSC_ELS_NORMAL : > PWM_IMX_TPM_CnSC_ELS_INVERSED; > > > Maybe also add > > > > #define PWM_IMX_TPM_CnSC_ELS_INACTIVE FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0) > > > > I did NOT use the FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0) at all, so why add it? > I don't quite understand. You use it implicitly in pwm_imx_tpm_apply_hw() if state.enabled == false and c.enabled == true: val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); val &= ~(PWM_IMX_TPM_CnSC_ELS | ...); ... writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); > > > +static int pwm_imx_tpm_apply(struct pwm_chip *chip, > > > + struct pwm_device *pwm, > > > + struct pwm_state *state) > > > +{ > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > > + struct imx_tpm_pwm_param param; > > > + struct pwm_state real_state; > > > + int ret; > > > + > > > + ret = pwm_imx_tpm_round_state(chip, ¶m, state, &real_state); > > > + if (ret) > > > + return -EINVAL; > > > + > > > + mutex_lock(&tpm->lock); > > > + > > > + /* > > > + * TPM counter is shared by multiple channels, so > > > + * prescale and period can NOT be modified when > > > + * there are multiple channels in use with different > > > + * period settings. > > > + */ > > > + if (real_state.period != tpm->real_period) { > > > + if (tpm->user_count > 1) { > > > + ret = -EBUSY; > > > + goto exit; > > > + } > > > + > > > + pwm_imx_tpm_config_counter(chip, param); > > > + tpm->real_period = real_state.period; > > > + } > > > > Maybe add a comment that this could still be optimized. For example if > > pwm_imx_tpm_round_state returned prescale = 5 but prescale is currently 6, > > you might still be able to configure > > You meant for multiple users request different period case? In this block, if there is > ONLY one user and the requested period can be met by HW, it will anyway re-configure > the HW for the prescale and period I think, or I did NOT get your point? My idea has a flaw. I thought that if there is another user, the duty_cycle can still be represented if the actually used prescale value is slightly higher. But then there is still a problem with the period length that I missed. So my remark was wrong, sorry for that. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | From mboxrd@z Thu Jan 1 00:00:00 1970 From: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= Subject: Re: [PATCH V7 2/5] pwm: Add i.MX TPM PWM driver support Date: Wed, 20 Mar 2019 11:57:42 +0100 Message-ID: <20190320105742.jdabsbcoebbyftqr@pengutronix.de> References: <1553058067-18793-1-git-send-email-Anson.Huang@nxp.com> <1553058067-18793-3-git-send-email-Anson.Huang@nxp.com> <20190320081856.wkw55pmw57i4ifdj@pengutronix.de> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Anson Huang Cc: "thierry.reding@gmail.com" , "robh+dt@kernel.org" , "mark.rutland@arm.com" , "shawnguo@kernel.org" , "s.hauer@pengutronix.de" , "kernel@pengutronix.de" , "festevam@gmail.com" , "linux@armlinux.org.uk" , "otavio@ossystems.com.br" , "stefan@agner.ch" , Leonard Crestez , "schnitzeltony@gmail.com" , Robin Gong , "linux-pwm@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" List-Id: devicetree@vger.kernel.org Hello Anson, On Wed, Mar 20, 2019 at 10:17:50AM +0000, Anson Huang wrote: > > On Wed, Mar 20, 2019 at 05:06:21AM +0000, Anson Huang wrote: > > > + /* make sure counter is disabled for programming prescale */ > > > + val = readl(tpm->base + PWM_IMX_TPM_SC); > > > + saved_cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val); > > > + if (saved_cmod) { > > > + val &= ~PWM_IMX_TPM_SC_CMOD; > > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > > > I thought we agreed on not stopping the counter if the PS field isn't changed? > > If the PS field no need to change, the round state should already return the period > equal to current period settings, so this function will NOT be called, right? > > if (real_state.period != tpm->real_period) { > if (tpm->user_count > 1) { > ret = -EBUSY; > goto exit; > } > > pwm_imx_tpm_setup_period(chip, param); > tpm->real_period = real_state.period; > } If the PS field is already right .period might still not match as its value depends on SC.PS and MOD.MOD. > > > + val &= ~PWM_IMX_TPM_SC_PS; > > > + val |= FIELD_PREP(PWM_IMX_TPM_SC_PS, p.prescale); > > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > > + > > > + /* > > > + * set period count: according to RM, the MOD register is > > > + * updated immediately after CMOD[1:0] = 2b'00 above > > > + */ > > > > So the current period isn't completed? That's wrong. > > So you mean we have to wait for the current period to finish here? > I did NOT know this, so we have to compare the counter value with Yeah, see https://patchwork.ozlabs.org/patch/1021566/ which documents this but waits for feedback by Thierry since some time. > the MOD value until they match then proceed the period change? If the hardware doesn't support you here (usually it does) I think it's not reliable enough to try to sync in software. I assume you can get the right wave form if you don't change SC.PS but only MOD.MOD? So maybe the sanest approach is to refuse changing SC.PS if the PWM is running. Or disable (which also should wait for the current period to finish), poll for the end (or use an irq?) and then setup the new configuration. > > > +{ > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > > + struct pwm_state c; > > > + u32 val, sc_val; > > > + u64 tmp; > > > + > > > + pwm_imx_tpm_get_state(chip, pwm, &c); > > > + > > > + if (state.duty_cycle != c.duty_cycle) { > > > + /* set duty counter */ > > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) & PWM_IMX_TPM_MOD_MOD; > > > + tmp *= state.duty_cycle; > > > + val = DIV_ROUND_CLOSEST_ULL(tmp, state.period); > > > + writel(val, tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)); > > > + } > > > + > > > + if (state.enabled != c.enabled) { > > > > This is wrong. If the PWM was running (c.enabled == true) and you are > > supposed to disable (state.enabled == false) you enable the hardware once > > more. > > A little confused here, as the case you assume, inside this block, there is another > check for state.enabled, if it is false, the polarity will be set to channel disabled mode, > the polarity setting is combined together with the enable status here, am I wrong? Ah, you're right. I missed that probably because I saw register accesses after the state.enabled != c.enabled check. > > > + val |= (state.polarity == PWM_POLARITY_NORMAL) ? > > > + FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x2) : > > > + FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x1); > > > > Introduce PWM_IMX_TPM_CnSC_ELS_POLARITY_NORMAL and use it > > together with PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED here. If you > > put the FIELD_PREP into the definition the line doesn't get excessively long. > > > > I put the FIELD_PREP into definition, the line still long, but I agree using definition is better. > > #define PWM_IMX_TPM_CnSC_ELS_INVERSED FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 1) > #define PWM_IMX_TPM_CnSC_ELS_NORMAL FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 2) > > val |= (state->polarity == PWM_POLARITY_NORMAL) ? > PWM_IMX_TPM_CnSC_ELS_NORMAL : > PWM_IMX_TPM_CnSC_ELS_INVERSED; > > > Maybe also add > > > > #define PWM_IMX_TPM_CnSC_ELS_INACTIVE FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0) > > > > I did NOT use the FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0) at all, so why add it? > I don't quite understand. You use it implicitly in pwm_imx_tpm_apply_hw() if state.enabled == false and c.enabled == true: val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); val &= ~(PWM_IMX_TPM_CnSC_ELS | ...); ... writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); > > > +static int pwm_imx_tpm_apply(struct pwm_chip *chip, > > > + struct pwm_device *pwm, > > > + struct pwm_state *state) > > > +{ > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > > + struct imx_tpm_pwm_param param; > > > + struct pwm_state real_state; > > > + int ret; > > > + > > > + ret = pwm_imx_tpm_round_state(chip, ¶m, state, &real_state); > > > + if (ret) > > > + return -EINVAL; > > > + > > > + mutex_lock(&tpm->lock); > > > + > > > + /* > > > + * TPM counter is shared by multiple channels, so > > > + * prescale and period can NOT be modified when > > > + * there are multiple channels in use with different > > > + * period settings. > > > + */ > > > + if (real_state.period != tpm->real_period) { > > > + if (tpm->user_count > 1) { > > > + ret = -EBUSY; > > > + goto exit; > > > + } > > > + > > > + pwm_imx_tpm_config_counter(chip, param); > > > + tpm->real_period = real_state.period; > > > + } > > > > Maybe add a comment that this could still be optimized. For example if > > pwm_imx_tpm_round_state returned prescale = 5 but prescale is currently 6, > > you might still be able to configure > > You meant for multiple users request different period case? In this block, if there is > ONLY one user and the requested period can be met by HW, it will anyway re-configure > the HW for the prescale and period I think, or I did NOT get your point? My idea has a flaw. I thought that if there is another user, the duty_cycle can still be represented if the actually used prescale value is slightly higher. But then there is still a problem with the period length that I missed. So my remark was wrong, sorry for that. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | 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=-3.0 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS, URIBL_BLOCKED,USER_AGENT_NEOMUTT 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 AEBD9C43381 for ; Wed, 20 Mar 2019 10:58:00 +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 7BAEB2175B for ; Wed, 20 Mar 2019 10:58:00 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="g793WqzK" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7BAEB2175B Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=pengutronix.de 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:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=PMblxt3XpivazIBEPMkvjAwNJOW1XaXBhBPsdMqdefU=; b=g793WqzK/NVHla LsIWojflHui3R4vXvtUNRw7nveFS5BLSxGcs9+YCogVp0BZMIYFdDcznj6amCa5p0tzwcFkA7/ue4 Kw1tNxpQhzloUs1MJAfJzsgSf89BEQ/p8ZNFOnmstxKK9J/VZr5Yt1KMWkHWlGmTyJ0bB8IDtG7+T ZsNRAoc65Neo2MtDTUQLVtfysKySQmEc1cB/UJ+SooSv1rs62CU5s2BlYn4K43wbw4ZkWulGnjUj2 lS4CNE15RzAJSqohfSXfscTDG7GBI92d+eRmVH30kLfBC117Cr1DZYG/3Stt76LtWLbHoGNny/laj y5dZvtWat3X6nc3xhalg==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1h6Yuy-0006Zn-Cc; Wed, 20 Mar 2019 10:57:56 +0000 Received: from metis.ext.pengutronix.de ([2001:67c:670:201:290:27ff:fe1d:cc33]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1h6Yut-0006ZC-Id for linux-arm-kernel@lists.infradead.org; Wed, 20 Mar 2019 10:57:53 +0000 Received: from pty.hi.pengutronix.de ([2001:67c:670:100:1d::c5]) by metis.ext.pengutronix.de with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89) (envelope-from ) id 1h6Yum-0001P8-FD; Wed, 20 Mar 2019 11:57:44 +0100 Received: from ukl by pty.hi.pengutronix.de with local (Exim 4.89) (envelope-from ) id 1h6Yuk-0008QV-Se; Wed, 20 Mar 2019 11:57:42 +0100 Date: Wed, 20 Mar 2019 11:57:42 +0100 From: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= To: Anson Huang Subject: Re: [PATCH V7 2/5] pwm: Add i.MX TPM PWM driver support Message-ID: <20190320105742.jdabsbcoebbyftqr@pengutronix.de> References: <1553058067-18793-1-git-send-email-Anson.Huang@nxp.com> <1553058067-18793-3-git-send-email-Anson.Huang@nxp.com> <20190320081856.wkw55pmw57i4ifdj@pengutronix.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c5 X-SA-Exim-Mail-From: ukl@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-arm-kernel@lists.infradead.org X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190320_035751_777848_796E73CA X-CRM114-Status: GOOD ( 30.67 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "mark.rutland@arm.com" , "linux-pwm@vger.kernel.org" , Robin Gong , "schnitzeltony@gmail.com" , "otavio@ossystems.com.br" , "devicetree@vger.kernel.org" , "festevam@gmail.com" , "s.hauer@pengutronix.de" , "linux@armlinux.org.uk" , "robh+dt@kernel.org" , "linux-kernel@vger.kernel.org" , "thierry.reding@gmail.com" , "stefan@agner.ch" , "kernel@pengutronix.de" , Leonard Crestez , "shawnguo@kernel.org" , "linux-arm-kernel@lists.infradead.org" , dl-linux-imx Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hello Anson, On Wed, Mar 20, 2019 at 10:17:50AM +0000, Anson Huang wrote: > > On Wed, Mar 20, 2019 at 05:06:21AM +0000, Anson Huang wrote: > > > + /* make sure counter is disabled for programming prescale */ > > > + val =3D readl(tpm->base + PWM_IMX_TPM_SC); > > > + saved_cmod =3D FIELD_GET(PWM_IMX_TPM_SC_CMOD, val); > > > + if (saved_cmod) { > > > + val &=3D ~PWM_IMX_TPM_SC_CMOD; > > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > = > > I thought we agreed on not stopping the counter if the PS field isn't c= hanged? > = > If the PS field no need to change, the round state should already return = the period > equal to current period settings, so this function will NOT be called, ri= ght? > = > if (real_state.period !=3D tpm->real_period) { > if (tpm->user_count > 1) { > ret =3D -EBUSY; > goto exit; > } > = > pwm_imx_tpm_setup_period(chip, param); > tpm->real_period =3D real_state.period; > } = If the PS field is already right .period might still not match as its value depends on SC.PS and MOD.MOD. > > > + val &=3D ~PWM_IMX_TPM_SC_PS; > > > + val |=3D FIELD_PREP(PWM_IMX_TPM_SC_PS, p.prescale); > > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > > + > > > + /* > > > + * set period count: according to RM, the MOD register is > > > + * updated immediately after CMOD[1:0] =3D 2b'00 above > > > + */ > > = > > So the current period isn't completed? That's wrong. > = > So you mean we have to wait for the current period to finish here? > I did NOT know this, so we have to compare the counter value with Yeah, see https://patchwork.ozlabs.org/patch/1021566/ which documents this but waits for feedback by Thierry since some time. > the MOD value until they match then proceed the period change? If the hardware doesn't support you here (usually it does) I think it's not reliable enough to try to sync in software. I assume you can get the right wave form if you don't change SC.PS but only MOD.MOD? So maybe the sanest approach is to refuse changing SC.PS if the PWM is running. Or disable (which also should wait for the current period to finish), poll for the end (or use an irq?) and then setup the new configuration. > > > +{ > > > + struct imx_tpm_pwm_chip *tpm =3D to_imx_tpm_pwm_chip(chip); > > > + struct pwm_state c; > > > + u32 val, sc_val; > > > + u64 tmp; > > > + > > > + pwm_imx_tpm_get_state(chip, pwm, &c); > > > + > > > + if (state.duty_cycle !=3D c.duty_cycle) { > > > + /* set duty counter */ > > > + tmp =3D readl(tpm->base + PWM_IMX_TPM_MOD) & PWM_IMX_TPM_MOD_MOD; > > > + tmp *=3D state.duty_cycle; > > > + val =3D DIV_ROUND_CLOSEST_ULL(tmp, state.period); > > > + writel(val, tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)); > > > + } > > > + > > > + if (state.enabled !=3D c.enabled) { > > = > > This is wrong. If the PWM was running (c.enabled =3D=3D true) and you a= re > > supposed to disable (state.enabled =3D=3D false) you enable the hardwar= e once > > more. > = > A little confused here, as the case you assume, inside this block, there = is another > check for state.enabled, if it is false, the polarity will be set to chan= nel disabled mode, > the polarity setting is combined together with the enable status here, am= I wrong? = Ah, you're right. I missed that probably because I saw register accesses after the state.enabled !=3D c.enabled check. > > > + val |=3D (state.polarity =3D=3D PWM_POLARITY_NORMAL) ? > > > + FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x2) : > > > + FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x1); > > = > > Introduce PWM_IMX_TPM_CnSC_ELS_POLARITY_NORMAL and use it > > together with PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED here. If you > > put the FIELD_PREP into the definition the line doesn't get excessively= long. > > = > = > I put the FIELD_PREP into definition, the line still long, but I agree us= ing definition is better. > = > #define PWM_IMX_TPM_CnSC_ELS_INVERSED FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, = 1) > #define PWM_IMX_TPM_CnSC_ELS_NORMAL FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, = 2) > = > val |=3D (state->polarity =3D=3D PWM_POLARITY_NO= RMAL) ? > PWM_IMX_TPM_CnSC_ELS_NORMAL : > PWM_IMX_TPM_CnSC_ELS_INVERSED; > = > > Maybe also add > > = > > #define PWM_IMX_TPM_CnSC_ELS_INACTIVE FIELD_PREP(PWM_IMX_TPM_CnSC_ELS,= 0) > > = > = > I did NOT use the FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0) at all, so why add = it? > I don't quite understand. You use it implicitly in pwm_imx_tpm_apply_hw() if state.enabled =3D=3D false and c.enabled =3D=3D true: val =3D readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); val &=3D ~(PWM_IMX_TPM_CnSC_ELS | ...); ... writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); > > > +static int pwm_imx_tpm_apply(struct pwm_chip *chip, > > > + struct pwm_device *pwm, > > > + struct pwm_state *state) > > > +{ > > > + struct imx_tpm_pwm_chip *tpm =3D to_imx_tpm_pwm_chip(chip); > > > + struct imx_tpm_pwm_param param; > > > + struct pwm_state real_state; > > > + int ret; > > > + > > > + ret =3D pwm_imx_tpm_round_state(chip, ¶m, state, &real_state); > > > + if (ret) > > > + return -EINVAL; > > > + > > > + mutex_lock(&tpm->lock); > > > + > > > + /* > > > + * TPM counter is shared by multiple channels, so > > > + * prescale and period can NOT be modified when > > > + * there are multiple channels in use with different > > > + * period settings. > > > + */ > > > + if (real_state.period !=3D tpm->real_period) { > > > + if (tpm->user_count > 1) { > > > + ret =3D -EBUSY; > > > + goto exit; > > > + } > > > + > > > + pwm_imx_tpm_config_counter(chip, param); > > > + tpm->real_period =3D real_state.period; > > > + } > > = > > Maybe add a comment that this could still be optimized. For example if > > pwm_imx_tpm_round_state returned prescale =3D 5 but prescale is current= ly 6, > > you might still be able to configure > = > You meant for multiple users request different period case? In this block= , if there is > ONLY one user and the requested period can be met by HW, it will anyway r= e-configure > the HW for the prescale and period I think, or I did NOT get your point? My idea has a flaw. I thought that if there is another user, the duty_cycle can still be represented if the actually used prescale value is slightly higher. But then there is still a problem with the period length that I missed. So my remark was wrong, sorry for that. Best regards Uwe -- = Pengutronix e.K. | Uwe Kleine-K=F6nig | Industrial Linux Solutions | http://www.pengutronix.de/ | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel