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 EF387C54E41 for ; Tue, 5 Mar 2024 09:25:02 +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:References:In-Reply-To: Date:Subject:Cc:To:From:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=7USCRWf3axhw1t576+dj9tbG54oShBpHq7v0l5+vDiI=; b=Ob+UYw2nx/cDMX uTeNwT7JUhLQyOrzVbBh7MdgfxunGeP3d5oBa50lxqqMbUlSjK7brsI29haBvNoD7i4twGiGr015J 3ODwgascl0nhHKvuQSNIT/yQzzSGG9X4yQ/8inRhe7aGgnbzQp1o9I78Y1E/tcN3OqxmsRFto42j6 XS/gczVse2NrIO8hKEBAnjXBknJC1ZsftioYme9laRm10sU2/m3+yWQs7gPvf+ZsyPh9Lr+FNGty7 iGdtFsbtTQyLF1zpQabRCRL5/2fL/kvzZKlenXzbHkPlilYox54M8eozxUxTlf4LTAXMN3jxMtn0y nzzX4WQRYhRMlIyTJ6xQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rhR2Z-0000000CoBX-0O9G; Tue, 05 Mar 2024 09:24:51 +0000 Received: from mail-ot1-x32e.google.com ([2607:f8b0:4864:20::32e]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rhR2W-0000000Co9s-0OJf for linux-arm-kernel@lists.infradead.org; Tue, 05 Mar 2024 09:24:49 +0000 Received: by mail-ot1-x32e.google.com with SMTP id 46e09a7af769-6e4b34f2455so2961471a34.2 for ; Tue, 05 Mar 2024 01:24:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1709630682; x=1710235482; darn=lists.infradead.org; h=content-transfer-encoding:mime-version:references:in-reply-to:date :subject:cc:to:from:message-id:from:to:cc:subject:date:message-id :reply-to; bh=2n6XxnRJcK3S7HaVQoAKao47crHz+00Pci1WifCRN34=; b=UFhD7kB3pTDlbilXgjSABzncJSS0v0OF6fKn4ZnsYQHF17F1IUkWrPsAVkleHYd0Xt F4SR60VAq0HJdq1gou3Tw14ooWhyiqFQcxWAViSRjgKIJvPwLi/T31JguGpMatF3+NWy /Cmfq7/6TS0EBD1LEsMtqGmdOweU8NR0uC26CuEaokNJptF20d610+2NGDrKcNAI7nbq +rlKyOZAbq5yiMX/2nqBCvEE9lZLw0CLsV7NZEnFLEOYcjqETVaC0AD84Ze2LXdQFQYS 7NWn/NpXnkeKPVYt+GEAqKS4uXZ6oIYHYk1RhI+bCRFe70+DBIAa/oc1ogfe/XIt48m4 7YZw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1709630682; x=1710235482; h=content-transfer-encoding:mime-version:references:in-reply-to:date :subject:cc:to:from:message-id:x-gm-message-state:from:to:cc:subject :date:message-id:reply-to; bh=2n6XxnRJcK3S7HaVQoAKao47crHz+00Pci1WifCRN34=; b=O7qVRDm1KtOvxi3Z95khVJ6ZSddJ/Ae4qOQFS3/N0maMgt1gnsygMKTvviqLGTAKNz g9aa5cMEBUB+e2tcV/i34jqAF68nqW2IBrecb4sax6dWXNa9M6o7Sb3p+CHYKZ0rRn0o mbtPWYE07ylhR4d/aTHBwO2iGW3VJtgM5gvjVdDBXE+Q9YFEuoRGFwW/9Kk7W96SugUV sCwItxOCxJHYqvCa1tPxTVloR498WLlex5ls8eYESY/tF6uVCnhHDiVdE/LExjPHIK3b Bo6TKZ/uo4XWnRFPxAT+Bsizg3pYZoEm7YnQuV0v4Ic2O3ZhK+nBPA+ChtmToy2PQrof QMuw== X-Forwarded-Encrypted: i=1; AJvYcCXsMtEroAxiwwF3IACJcdyI0vT7SiuA/ooAkHERUuiwG9HZCmxso/AN1n7M+NG5WKYM0af2uUmy8qH+kBu1I5It3aTuuQZqEVKvAT/XmdQfVS+lcfE= X-Gm-Message-State: AOJu0YzBGtjhyUI1CTEaxCtyPzaFXQbvBJARBXXGL4Rew20gd4dF+dCO XxoZfJ7kqHIDb/82ODxttsLS9h+2f6lTqopF4iM4ODui0QvM8Oxs X-Google-Smtp-Source: AGHT+IEf3Th5yXrUurmy310rbfGfsm6zvHyKT8USnhxeMIOTIZ0B8RtQzE21FA69Y+hhaHydWopuag== X-Received: by 2002:a9d:6944:0:b0:6e4:fa1a:dae2 with SMTP id p4-20020a9d6944000000b006e4fa1adae2mr516840oto.2.1709630682163; Tue, 05 Mar 2024 01:24:42 -0800 (PST) Received: from localhost.localdomain ([2402:3a80:863:befc:ec5c:8cb:6f1e:1f69]) by smtp.gmail.com with ESMTPSA id f4-20020a63de04000000b005dc4ce8d2a4sm8717493pgg.58.2024.03.05.01.24.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 05 Mar 2024 01:24:41 -0800 (PST) Message-ID: <65e6e4d9.630a0220.8d898.19f4@mx.google.com> X-Google-Original-Message-ID: (raw) From: pratikmanvar09@gmail.com To: wahrenst@gmx.net Cc: festevam@gmail.com, jun.li@nxp.com, kernel@pengutronix.de, linux-arm-kernel@lists.infradead.org, linux-imx@nxp.com, linux-kernel@vger.kernel.org, linux-pwm@vger.kernel.org, lkp@intel.com, oe-kbuild-all@lists.linux.dev, pratik.manvar@ifm.com, pratikmanvar09@gmail.com, s.hauer@pengutronix.de, shawnguo@kernel.org, thierry.reding@gmail.com, u.kleine-koenig@pengutronix.de, xiaoning.wang@nxp.com Subject: Re: [PATCH v2] pwm: imx27: workaround of the pwm output bug Date: Tue, 5 Mar 2024 14:54:28 +0530 X-Mailer: git-send-email 2.25.1 In-Reply-To: <65bf307d.170a0220.4d544.f32b@mx.google.com> References: MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240305_012448_202152_7DF2BCDC X-CRM114-Status: GOOD ( 29.68 ) X-BeenThere: linux-arm-kernel@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-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Stefan, Sorry for the abysmal delay. Thanks for your review and suggestions. >Hi Pratik, > >Am 04.02.24 um 07:36 schrieb pratikmanvar09@gmail.com: >> Hi Stefan, >> >> Thanks for your review. >> Please see my reply below inline. >> >>>> From: Clark Wang >>>> >>>> This fixes the pwm output bug when decrease the duty cycle. >>>> This is a limited workaround for the PWM IP issue TKT0577206. >>> this looks like a patch from the vendor tree. >> [Pratik]: Yes, this is the patch from NXP. Please see original link of the patch https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317 >> >>> Could you please provide a link to the origin or at least to the >>> document which describes TKT0577206? >> [Pratik]: Please refer i.MX8MN errata #ERR051198 in https://www.nxp.com/docs/en/errata/IMX8MN_0N14Y.pdf. >Thanks, i think this ERR... reference is better than TKT... because it's >links to the errata documents and other Freescale/NXP drivers use them >too. So having this code in a comment would be great. Sure, I will mention this #ERR051198 code in commit message. >> >>> As a i.MX6ULL user i couldn't find this issue in the chip errata. So are >>> you sure that every PWM IP handled by this driver is affected? >> [Pratik]: Yes, looks like this issue is on all platforms which uses this PWM IP. >> >>>> Root cause: >>>> When the SAR FIFO is empty, the new write value will be directly applied >>>> to SAR even the current period is not over. >>>> If the new SAR value is less than the old one, and the counter is >>>> greater than the new SAR value, the current period will not filp the >>> s/filp/flip/ ? >>>> level. This will result in a pulse with a duty cycle of 100%. >>>> >>>> Workaround: >>>> Add an old value SAR write before updating the new duty cycle to SAR. >>>> This will keep the new value is always in a not empty fifo, and can be >>>> wait to update after a period finished. >>>> >>>> Limitation: >>>> This workaround can only solve this issue when the PWM period is longer >>>> than 2us(or <500KHz). >>>> >>>> Reviewed-by: Jun Li >>>> Signed-off-by: Clark Wang >>>> Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317 >>>> Tested-by: Pratik Manvar >>>> --- >>>> V1 -> V2: fix sparse warnings reported-by: kernel test robot >>>> Closes: https://lore.kernel.org/oe-kbuild-all/202312300907.RGtYsKxb-lkp@intel.com/ >>>> >>>> drivers/pwm/pwm-imx27.c | 67 ++++++++++++++++++++++++++++++++++++++--- >>>> 1 file changed, 62 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c >>>> index 7d9bc43f12b0..1e500a5bf564 100644 >>>> --- a/drivers/pwm/pwm-imx27.c >>>> +++ b/drivers/pwm/pwm-imx27.c >>>> @@ -21,11 +21,13 @@ >>>> #include >>>> #include >>>> #include >>>> +#include >>>> >>>> #define MX3_PWMCR 0x00 /* PWM Control Register */ >>>> #define MX3_PWMSR 0x04 /* PWM Status Register */ >>>> #define MX3_PWMSAR 0x0C /* PWM Sample Register */ >>>> #define MX3_PWMPR 0x10 /* PWM Period Register */ >>>> +#define MX3_PWMCNR 0x14 /* PWM Counter Register */ >>>> >>>> #define MX3_PWMCR_FWM GENMASK(27, 26) >>>> #define MX3_PWMCR_STOPEN BIT(25) >>>> @@ -91,6 +93,7 @@ struct pwm_imx27_chip { >>>> * value to return in that case. >>>> */ >>>> unsigned int duty_cycle; >>>> + spinlock_t lock; >>>> }; >>>> >>>> #define to_pwm_imx27_chip(chip) container_of(chip, struct pwm_imx27_chip, chip) >>>> @@ -203,10 +206,10 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip, >>>> >>>> sr = readl(imx->mmio_base + MX3_PWMSR); >>>> fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr); >>>> - if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) { >>>> + if (fifoav >= MX3_PWMSR_FIFOAV_3WORDS) { >>>> period_ms = DIV_ROUND_UP_ULL(pwm_get_period(pwm), >>>> NSEC_PER_MSEC); >>>> - msleep(period_ms); >>>> + msleep(period_ms * (fifoav - 2)); >>> This touches a different workaround ("pwm: imx: Avoid sample FIFO >>> overflow for i.MX PWM version2") without any explanation. >> [Pratik]: Sure, I will look into this. Thanks! >>>> sr = readl(imx->mmio_base + MX3_PWMSR); >>>> if (fifoav == FIELD_GET(MX3_PWMSR_FIFOAV, sr)) >>>> @@ -217,13 +220,15 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip, >>>> static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, >>>> const struct pwm_state *state) >>>> { >>>> - unsigned long period_cycles, duty_cycles, prescale; >>>> + unsigned long period_cycles, duty_cycles, prescale, counter_check, flags; >>>> struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip); >>>> + void __iomem *reg_sar = imx->mmio_base + MX3_PWMSAR; >>>> + __force u32 sar_last, sar_current; >>>> struct pwm_state cstate; >>>> unsigned long long c; >>>> unsigned long long clkrate; >>>> int ret; >>>> - u32 cr; >>>> + u32 cr, timeout = 1000; >>>> >>>> pwm_get_state(pwm, &cstate); >>>> >>>> @@ -264,7 +269,57 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, >>>> pwm_imx27_sw_reset(chip); >>>> } >>>> >>>> - writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); >>>> + /* >>>> + * This is a limited workaround. When the SAR FIFO is empty, the new >>>> + * write value will be directly applied to SAR even the current period >>>> + * is not over. >>>> + * If the new SAR value is less than the old one, and the counter is >>>> + * greater than the new SAR value, the current period will not filp >>> The same typo as in the commit message. >>>> + * the level. This will result in a pulse with a duty cycle of 100%. >>>> + * So, writing the current value of the SAR to SAR here before updating >>>> + * the new SAR value can avoid this issue. >>>> + * >>>> + * Add a spin lock and turn off the interrupt to ensure that the >>>> + * real-time performance can be guaranteed as much as possible when >>>> + * operating the following operations. >>>> + * >>>> + * 1. Add a threshold of 1.5us. If the time T between the read current >>>> + * count value CNR and the end of the cycle is less than 1.5us, wait >>>> + * for T to be longer than 1.5us before updating the SAR register. >>>> + * This is to avoid the situation that when the first SAR is written, >>>> + * the current cycle just ends and the SAR FIFO that just be written >>>> + * is emptied again. >>>> + * >>>> + * 2. Use __raw_writel() to minimize the interval between two writes to >>>> + * the SAR register to increase the fastest pwm frequency supported. >>>> + * >>>> + * When the PWM period is longer than 2us(or <500KHz), this workaround >>>> + * can solve this problem. >>>> + */ >>>> + if (duty_cycles < imx->duty_cycle) { >>>> + c = clkrate * 1500; >>>> + do_div(c, NSEC_PER_SEC); >>>> + counter_check = c; >>>> + sar_last = (__force u32) cpu_to_le32(imx->duty_cycle); >>>> + sar_current = (__force u32) cpu_to_le32(duty_cycles); >>>> + >>>> + spin_lock_irqsave(&imx->lock, flags); >>>> + if (state->period >= 2000) { >>>> + while ((period_cycles - >>>> + readl_relaxed(imx->mmio_base + MX3_PWMCNR)) >>>> + < counter_check) { >>>> + if (!--timeout) >>>> + break; >>>> + }; >>>> + } >>>> + if (!(MX3_PWMSR_FIFOAV & >>>> + readl_relaxed(imx->mmio_base + MX3_PWMSR))) >>>> + __raw_writel(sar_last, reg_sar); >>>> + __raw_writel(sar_current, reg_sar); >>>> + spin_unlock_irqrestore(&imx->lock, flags); >>>> + } else >>>> + writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); >>>> + >>> This is hard to believe that checkpatch.pl is fine with this patch. >>> Please use it before submission. >> [Pratik]: I used the checkpatch.pl in this patch and that runs without any warnings/errors! >Okay, AFAIR the coding style suggests braces for the else case. >> >>>> writel(period_cycles, imx->mmio_base + MX3_PWMPR); >>>> >>>> /* >>>> @@ -324,6 +379,8 @@ static int pwm_imx27_probe(struct platform_device *pdev) >>>> return dev_err_probe(&pdev->dev, PTR_ERR(imx->clk_per), >>>> "failed to get peripheral clock\n"); >>>> >>>> + spin_lock_init(&imx->lock); >>>> + imx->duty_cycle = 0; >>> This line looks unrelated and unnecessary. >> [Pratik]: Right. I will remove this line in next patch version. >Could you also please look at Uwe's comments [1]? > >Thanks > >[1] - >https://lore.kernel.org/all/20211220105555.zwq22vip7onafrck@pengutronix.de/ Actually, I did not get much time to work on this. But, I will look into this now. >> >>> Best regards >>>> imx->chip.ops = &pwm_imx27_ops; >>>> imx->chip.dev = &pdev->dev; >>>> imx->chip.npwm = 1; Thanks & Regards, Pratik Manvar _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel