From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ot1-f50.google.com (mail-ot1-f50.google.com [209.85.210.50]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3AFB250272; Tue, 5 Mar 2024 09:24:43 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.210.50 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709630684; cv=none; b=u8lauUEldPa7q61MADobxRanFGB116TRX/58ssU39g8C5Z85Fn3IMXS2MtRxIGehd2/yE+4kHEZBq9oAFTfLIYKIAwGh9NDWdz+Zv6wNObp5VQwTGm7yqdbVC4hCpdDvKvE72JwhDuimhvKT1/u7SuC7QKNAP2Km1RBuUFWHVRs= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709630684; c=relaxed/simple; bh=tPfQYkjbgATtz+kEhN3abAcfDDsv0QHzHdZUDzvbn1s=; h=Message-ID:From:To:Cc:Subject:Date:In-Reply-To:References: MIME-Version; b=NTneupFy0tnApcUlJjJ+GIGP6W5ZY42BKLYCCShdXY3khDgWs1PzG9kb128r1y0UBnG0KrX/Ty5UjHMVzBm/jyd7+e3s/84YblYOjFfqm4TDDjyI5WOBqBxRczRVw/if4n8GmRr9oHRX+kq9nefZ4KOphw/NY5OhPVMzzzx5QRw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=IHImajwr; arc=none smtp.client-ip=209.85.210.50 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="IHImajwr" Received: by mail-ot1-f50.google.com with SMTP id 46e09a7af769-6e4b34f2455so2961472a34.2; 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=vger.kernel.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=IHImajwr3tDZAeDJSS59h9INfwOrvNmxSrX0JJKXrkk4EZFNJC+VVRvYyM/C15B+CV DK2DiTy7J88ETFUMaitJx9A8EPWX9MoKx7RxclO+5RPfhadT7T2PUL7L58TByZh/h0nc rB0iB4gmwUM75le4UFJPnn0eq5TTAOOkF9uu0tK7Vm3/UWIvYO5Qk5THTg5Aftgamq79 uZUOOG6v/ZJjOMFFpC+mWcgI0VsrySNkpJufNTIdPG7R166Lc3R9bKt5T0Np81UIH4mG yj1kU5GjNe5bru0F65QzIYZmRUDJmrP/4K9SDNiCvD30MRmOPmwqDu6OFPVDAhnVHBXX clNQ== 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=OrG1f2U8Io9kzJ0nFIMb/zTBaFIZOdsnwp4qgWV6co2+rD9H2EzxU8B1wgxDSNmeQM p2nIAaCgkIl0ziCT3IeHkyMToUQgph5h/FuyyARsP3ZzzhWKJ9bAeIfP7CEqghYTUpSH trCshXOHLUeR7ClaF6f9VMK8D5VyQD0lyXxWG3BkqqvXTHJWxZ8zZJiDR1fasTfCNZLm LAxhOgeTkUhGaAo+Z3Zd6UDi7ZxGbYurNFI71IVXd25Ydr+HhwIoEljgPCeRYJC4TNWJ HnYLFhkwJLBcjS+BOmjOOtG68wEuF0WgFEXlJC0G4DPELknFlw5jucNbQrNv6P7WjnJd HMvA== X-Forwarded-Encrypted: i=1; AJvYcCXzUmNTMtVP+nOAVnGZCSP15HPcv402sRpqtHKrbr7R6YB6bcmhOMC7umQjmmAYCI31V9vulGlbiKI88v7ecgdnFE9UKUgnKQ2HsXa7LJ8XVoQBbu16Jll1TqijSegbZpqVahXg4qMK X-Gm-Message-State: AOJu0YxG9YgOC1L/COTL4e2TEwQ1qwIVFQbse64ew1YTN2yVue6wX4lc rDVGBw0FPqGa32r5xf1UmLAxs9teX6epgw8NoXvtph6HiS9pPTnn 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: Precedence: bulk X-Mailing-List: linux-pwm@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Hi Stefan,=0D =0D Sorry for the abysmal delay.=0D Thanks for your review and suggestions.=0D =0D >Hi Pratik,=0D >=0D >Am 04.02.24 um 07:36 schrieb pratikmanvar09@gmail.com:=0D >> Hi Stefan,=0D >>=0D >> Thanks for your review.=0D >> Please see my reply below inline.=0D >>=0D >>>> From: Clark Wang =0D >>>>=0D >>>> This fixes the pwm output bug when decrease the duty cycle.=0D >>>> This is a limited workaround for the PWM IP issue TKT0577206.=0D >>> this looks like a patch from the vendor tree.=0D >> [Pratik]: Yes, this is the patch from NXP. Please see original link of t= he patch https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0= e5a479990507816317=0D >>=0D >>> Could you please provide a link to the origin or at least to the=0D >>> document which describes TKT0577206?=0D >> [Pratik]: Please refer i.MX8MN errata #ERR051198 in https://www.nxp.com/= docs/en/errata/IMX8MN_0N14Y.pdf.=0D >Thanks, i think this ERR... reference is better than TKT... because it's=0D >links to the errata documents and other Freescale/NXP drivers use them=0D >too. So having this code in a comment would be great.=0D Sure, I will mention this #ERR051198 code in commit message.=0D >>=0D >>> As a i.MX6ULL user i couldn't find this issue in the chip errata. So ar= e=0D >>> you sure that every PWM IP handled by this driver is affected?=0D >> [Pratik]: Yes, looks like this issue is on all platforms which uses this= PWM IP.=0D >>=0D >>>> Root cause:=0D >>>> When the SAR FIFO is empty, the new write value will be directly appli= ed=0D >>>> to SAR even the current period is not over.=0D >>>> If the new SAR value is less than the old one, and the counter is=0D >>>> greater than the new SAR value, the current period will not filp the=0D >>> s/filp/flip/ ?=0D >>>> level. This will result in a pulse with a duty cycle of 100%.=0D >>>>=0D >>>> Workaround:=0D >>>> Add an old value SAR write before updating the new duty cycle to SAR.= =0D >>>> This will keep the new value is always in a not empty fifo, and can be= =0D >>>> wait to update after a period finished.=0D >>>>=0D >>>> Limitation:=0D >>>> This workaround can only solve this issue when the PWM period is longe= r=0D >>>> than 2us(or <500KHz).=0D >>>>=0D >>>> Reviewed-by: Jun Li =0D >>>> Signed-off-by: Clark Wang =0D >>>> Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbab= a0e5a479990507816317=0D >>>> Tested-by: Pratik Manvar =0D >>>> ---=0D >>>> V1 -> V2: fix sparse warnings reported-by: kernel test robot =0D >>>> Closes: https://lore.kernel.org/oe-kbuild-all/20231230090= 7.RGtYsKxb-lkp@intel.com/=0D >>>>=0D >>>> drivers/pwm/pwm-imx27.c | 67 ++++++++++++++++++++++++++++++++++++++= ---=0D >>>> 1 file changed, 62 insertions(+), 5 deletions(-)=0D >>>>=0D >>>> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c=0D >>>> index 7d9bc43f12b0..1e500a5bf564 100644=0D >>>> --- a/drivers/pwm/pwm-imx27.c=0D >>>> +++ b/drivers/pwm/pwm-imx27.c=0D >>>> @@ -21,11 +21,13 @@=0D >>>> #include =0D >>>> #include =0D >>>> #include =0D >>>> +#include =0D >>>>=0D >>>> #define MX3_PWMCR 0x00 /* PWM Control Register */=0D >>>> #define MX3_PWMSR 0x04 /* PWM Status Register */=0D >>>> #define MX3_PWMSAR 0x0C /* PWM Sample Register */=0D >>>> #define MX3_PWMPR 0x10 /* PWM Period Register */=0D >>>> +#define MX3_PWMCNR 0x14 /* PWM Counter Register */=0D >>>>=0D >>>> #define MX3_PWMCR_FWM GENMASK(27, 26)=0D >>>> #define MX3_PWMCR_STOPEN BIT(25)=0D >>>> @@ -91,6 +93,7 @@ struct pwm_imx27_chip {=0D >>>> * value to return in that case.=0D >>>> */=0D >>>> unsigned int duty_cycle;=0D >>>> + spinlock_t lock;=0D >>>> };=0D >>>>=0D >>>> #define to_pwm_imx27_chip(chip) container_of(chip, struct pwm_imx27= _chip, chip)=0D >>>> @@ -203,10 +206,10 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_= chip *chip,=0D >>>>=0D >>>> sr =3D readl(imx->mmio_base + MX3_PWMSR);=0D >>>> fifoav =3D FIELD_GET(MX3_PWMSR_FIFOAV, sr);=0D >>>> - if (fifoav =3D=3D MX3_PWMSR_FIFOAV_4WORDS) {=0D >>>> + if (fifoav >=3D MX3_PWMSR_FIFOAV_3WORDS) {=0D >>>> period_ms =3D DIV_ROUND_UP_ULL(pwm_get_period(pwm),=0D >>>> NSEC_PER_MSEC);=0D >>>> - msleep(period_ms);=0D >>>> + msleep(period_ms * (fifoav - 2));=0D >>> This touches a different workaround ("pwm: imx: Avoid sample FIFO=0D >>> overflow for i.MX PWM version2") without any explanation.=0D >> [Pratik]: Sure, I will look into this. Thanks!=0D >>>> sr =3D readl(imx->mmio_base + MX3_PWMSR);=0D >>>> if (fifoav =3D=3D FIELD_GET(MX3_PWMSR_FIFOAV, sr))=0D >>>> @@ -217,13 +220,15 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_= chip *chip,=0D >>>> static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device= *pwm,=0D >>>> const struct pwm_state *state)=0D >>>> {=0D >>>> - unsigned long period_cycles, duty_cycles, prescale;=0D >>>> + unsigned long period_cycles, duty_cycles, prescale, counter_check, f= lags;=0D >>>> struct pwm_imx27_chip *imx =3D to_pwm_imx27_chip(chip);=0D >>>> + void __iomem *reg_sar =3D imx->mmio_base + MX3_PWMSAR;=0D >>>> + __force u32 sar_last, sar_current;=0D >>>> struct pwm_state cstate;=0D >>>> unsigned long long c;=0D >>>> unsigned long long clkrate;=0D >>>> int ret;=0D >>>> - u32 cr;=0D >>>> + u32 cr, timeout =3D 1000;=0D >>>>=0D >>>> pwm_get_state(pwm, &cstate);=0D >>>>=0D >>>> @@ -264,7 +269,57 @@ static int pwm_imx27_apply(struct pwm_chip *chip,= struct pwm_device *pwm,=0D >>>> pwm_imx27_sw_reset(chip);=0D >>>> }=0D >>>>=0D >>>> - writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);=0D >>>> + /*=0D >>>> + * This is a limited workaround. When the SAR FIFO is empty, the new= =0D >>>> + * write value will be directly applied to SAR even the current peri= od=0D >>>> + * is not over.=0D >>>> + * If the new SAR value is less than the old one, and the counter is= =0D >>>> + * greater than the new SAR value, the current period will not filp= =0D >>> The same typo as in the commit message.=0D >>>> + * the level. This will result in a pulse with a duty cycle of 100%.= =0D >>>> + * So, writing the current value of the SAR to SAR here before updat= ing=0D >>>> + * the new SAR value can avoid this issue.=0D >>>> + *=0D >>>> + * Add a spin lock and turn off the interrupt to ensure that the=0D >>>> + * real-time performance can be guaranteed as much as possible when= =0D >>>> + * operating the following operations.=0D >>>> + *=0D >>>> + * 1. Add a threshold of 1.5us. If the time T between the read curre= nt=0D >>>> + * count value CNR and the end of the cycle is less than 1.5us, wait= =0D >>>> + * for T to be longer than 1.5us before updating the SAR register.=0D >>>> + * This is to avoid the situation that when the first SAR is written= ,=0D >>>> + * the current cycle just ends and the SAR FIFO that just be written= =0D >>>> + * is emptied again.=0D >>>> + *=0D >>>> + * 2. Use __raw_writel() to minimize the interval between two writes= to=0D >>>> + * the SAR register to increase the fastest pwm frequency supported.= =0D >>>> + *=0D >>>> + * When the PWM period is longer than 2us(or <500KHz), this workarou= nd=0D >>>> + * can solve this problem.=0D >>>> + */=0D >>>> + if (duty_cycles < imx->duty_cycle) {=0D >>>> + c =3D clkrate * 1500;=0D >>>> + do_div(c, NSEC_PER_SEC);=0D >>>> + counter_check =3D c;=0D >>>> + sar_last =3D (__force u32) cpu_to_le32(imx->duty_cycle);=0D >>>> + sar_current =3D (__force u32) cpu_to_le32(duty_cycles);=0D >>>> +=0D >>>> + spin_lock_irqsave(&imx->lock, flags);=0D >>>> + if (state->period >=3D 2000) {=0D >>>> + while ((period_cycles -=0D >>>> + readl_relaxed(imx->mmio_base + MX3_PWMCNR))=0D >>>> + < counter_check) {=0D >>>> + if (!--timeout)=0D >>>> + break;=0D >>>> + };=0D >>>> + }=0D >>>> + if (!(MX3_PWMSR_FIFOAV &=0D >>>> + readl_relaxed(imx->mmio_base + MX3_PWMSR)))=0D >>>> + __raw_writel(sar_last, reg_sar);=0D >>>> + __raw_writel(sar_current, reg_sar);=0D >>>> + spin_unlock_irqrestore(&imx->lock, flags);=0D >>>> + } else=0D >>>> + writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);=0D >>>> +=0D >>> This is hard to believe that checkpatch.pl is fine with this patch.=0D >>> Please use it before submission.=0D >> [Pratik]: I used the checkpatch.pl in this patch and that runs without a= ny warnings/errors!=0D >Okay, AFAIR the coding style suggests braces for the else case.=0D >>=0D >>>> writel(period_cycles, imx->mmio_base + MX3_PWMPR);=0D >>>>=0D >>>> /*=0D >>>> @@ -324,6 +379,8 @@ static int pwm_imx27_probe(struct platform_device = *pdev)=0D >>>> return dev_err_probe(&pdev->dev, PTR_ERR(imx->clk_per),=0D >>>> "failed to get peripheral clock\n");=0D >>>>=0D >>>> + spin_lock_init(&imx->lock);=0D >>>> + imx->duty_cycle =3D 0;=0D >>> This line looks unrelated and unnecessary.=0D >> [Pratik]: Right. I will remove this line in next patch version.=0D >Could you also please look at Uwe's comments [1]?=0D >=0D >Thanks=0D >=0D >[1] -=0D >https://lore.kernel.org/all/20211220105555.zwq22vip7onafrck@pengutronix.de= /=0D Actually, I did not get much time to work on this. But, I will look into th= is now.=0D >>=0D >>> Best regards=0D >>>> imx->chip.ops =3D &pwm_imx27_ops;=0D >>>> imx->chip.dev =3D &pdev->dev;=0D >>>> imx->chip.npwm =3D 1;=0D =0D Thanks & Regards,=0D Pratik Manvar= 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