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 68741C4828F for ; Sun, 4 Feb 2024 06:37:05 +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=XIhcqoqraZc+c6et04R6bQnSBbluGWHRKhRTY9vCq/Q=; b=Z7B+2OWMe03mMI SFSQpreIz7DNMt+vLt/XHB3tPsMXkFo09r9PSD/HGjCADJ+2XaDl+L6jpZ92WQeNccoooeIsPH4zv 6U7S9lZzXrIheuk6t7tON9GH92E5ozR7yiUNVrNc7ZBgwt0Bcz90F6FYXjG5yVyiaFCvNHEyd8AEJ qYTLq6FsEYIO6bsh0Gmc1Dc6YvJA4dKMCGvhNjDAAOgeUTmVBnX6ekvpYsA5XD0Xc/scGIUX3o4Jr /yGiZM/ecMW7wZ1McEtAz87ySdQp50E6ADw29QHBGWbWDuwcNsmRfZfvNer/XCfPcwnmYCMU876xe SHFkPmpsZb6yO6TalzbQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rWW7Y-00000000F3G-3CeX; Sun, 04 Feb 2024 06:36:52 +0000 Received: from mail-pl1-x62a.google.com ([2607:f8b0:4864:20::62a]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rWW7V-00000000F25-1X2M for linux-arm-kernel@lists.infradead.org; Sun, 04 Feb 2024 06:36:50 +0000 Received: by mail-pl1-x62a.google.com with SMTP id d9443c01a7336-1d780a392fdso29016685ad.3 for ; Sat, 03 Feb 2024 22:36:46 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1707028606; x=1707633406; 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=AhrdsLXyharQtwk+YSZTkf4E5FkLiyUBnotfivIVHMw=; b=liag9Ob+tr2hp5ZlDW8Wdehznj5cf50VMhFqlzOEFTH4+8rMKZ0wRQvo24AhlPktV2 sq3ovyw/wn09VcQm1pHlaYwX7Nyi5BwQ30YoQxT+E/5LB6kuq8C1c1jyFMmTPTcdGUyC Iavl07imSEZslENLhBSuXb9r3dbe50RNSIhqk8k8mHIOi8NyR46KZfx4BzGpy9nplr15 l9AtbQveA/W6E1bnbQDdiilcoSPFoOurTtz6BZNXouhbEfCq/jfqA8vCNkjYBiST6nRe 3SqL0dRxYgnhKLOJ8uS73XqJscJVTlPyYVaYdwIkWtjlDrlbLuHsv5muJ/fjgGDi+oe6 KFyw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707028606; x=1707633406; 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=AhrdsLXyharQtwk+YSZTkf4E5FkLiyUBnotfivIVHMw=; b=llC4XQuhOKiyWskvnXMlD+CxB1wJXJmDrvMsJbNdTzgVRCEwipRaGHRnQK+gepDkUf KlTsefM0zD7RmPj4I2BM6NiNjkwIVpIgr9rw4mqYf/l1SWotTIrNE7z5xcM+vQgHq6tj 3Q+cC803R3W5X2Uj8tYSIKjXKs6Rih8eCnZi3IU+Yf/uVdodzPo41f1R91adcOba2Trs cmlwvQfmVJ9cOTNCvRGtdMMGWPDIf/3hsqfJEWSVCLvVJ93Y8u95g8MmFX8SNyrNapgu 0gCMT7h7UMDfr7d6b/oVTJrgZ75ulYiZ+KcTIJZD461ilrdmAk7PPCdU3cfwBpioW+7L 7Zuw== X-Gm-Message-State: AOJu0YxHCWL0cauyTGCt62xSEKPlmSmgebBp7qzeMjIaiTeK518YqqPH 3dhFX/1xno7qbE0gIGU1GZIeGiPtVqHrF9di50a4J2vt6XzxOFgM X-Google-Smtp-Source: AGHT+IGnQ0gSe5sRpzcfC4U19xJj5IwPh91we/gcJADZ/cNtw5aEKY3XtZ+g5/rZ/QsrOXtmYBoN0w== X-Received: by 2002:a17:902:c40c:b0:1d9:a6e8:1c8c with SMTP id k12-20020a170902c40c00b001d9a6e81c8cmr1073283plk.15.1707028606124; Sat, 03 Feb 2024 22:36:46 -0800 (PST) X-Forwarded-Encrypted: i=0; AJvYcCVDEKtZeceoHlPvaOg73HN92iSg8sP7+W8dEcfEgNCEvv/jgv75qW3RU97O4MqStQPwo5+uHpAyAd3wP0BXXZwElPYOSX8tdtd3ygWncV9LHZ3nvgXeny3ZrZNsMPKRYaLDZxOBDeXnQoA+cuP30OYEv6Xkcjd+vB1mfran7VE7jGSNzbKCLGm17D5ThM71drXZ2FCtrq1zODnNVqhkKCk3l9ZDAQRxFkqWChEJjFHZqbjTEOwCPqJ2Xj2pg53qLJUmJDQ85PjELaqkS9i355rhUJ9TYMbah6EKkbjbeBBi662c0lEdB/GtOJMKfbjNrWH+Cz0pU+AKgyWSdelHdm0ACHhCjU16X60HSsOcS7hE+xnni33HZBGe+zMFJjbcFH8whuXTg5JU8FzKU3UGumqc0AoDyE5+6mgqiDX7Lv2SDZWC0IaRG4O8Xcc7q3Gr4MCK0ncEskRkfsQ3uWf+Ufu4fqOzzoY3NcsNr1G1iy1Bep93KWGNhJuNZNAG+9F9lNXMEHSQWI6BiI+fXQG/O2Qp5GnlIOsW2EFeS2TvRuJrcnISdMs= Received: from localhost.localdomain ([58.84.61.91]) by smtp.gmail.com with ESMTPSA id q21-20020a170902c75500b001d8b8bf8e45sm4102848plq.37.2024.02.03.22.36.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 03 Feb 2024 22:36:45 -0800 (PST) Message-ID: <65bf307d.170a0220.4d544.f32b@mx.google.com> X-Google-Original-Message-ID: <9b5f9e1b-645a-4b44-a30c-fe93e7bf3532@gmx.net> (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: Sun, 4 Feb 2024 12:06:26 +0530 X-Mailer: git-send-email 2.25.1 In-Reply-To: <20240103063454.1795-1-pratikmanvar09@gmail.com> References: <9b5f9e1b-645a-4b44-a30c-fe93e7bf3532@gmx.net> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240203_223649_441358_DF8DFE00 X-CRM114-Status: GOOD ( 29.20 ) 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, 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. >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! >> 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. >Best regards >> imx->chip.ops = &pwm_imx27_ops; >> imx->chip.dev = &pdev->dev; >> imx->chip.npwm = 1; _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel