Message ID | 20230130093229.27489-3-nylon.chen@sifive.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:eb09:0:0:0:0:0 with SMTP id s9csp2088359wrn; Mon, 30 Jan 2023 01:36:25 -0800 (PST) X-Google-Smtp-Source: AK7set9r51UfnwMnMZcwRw/n2FWpbO6IEsZ9n9gHureGWd5eB/dZC85GJYUWgDB3f9fGdIYSBVTW X-Received: by 2002:a05:6a00:a18:b0:590:6a57:9904 with SMTP id p24-20020a056a000a1800b005906a579904mr20426391pfh.14.1675071384921; Mon, 30 Jan 2023 01:36:24 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1675071384; cv=none; d=google.com; s=arc-20160816; b=RjYwPtKhAQMUvuad+wsfDmy3q5ftyvbvDACDk5VpuM6JfPiW+qsdvKt3TqoYuUP5OF XO3oUMz9qy8gLKKFZrXs6RUGNoePOgj7TDbc3uqCdgyQsAVkUVutOOzz5zC7/uEMymuO m0lrXuQ0OzL4AgA7/RJqrOB08MgPaedYU4o2MghObVimrU1huHp5HC7obqO54eXtLUpn zkS3fXH1VxYiIv+56aP5Pex58AaFAvw8c6Lr7z6NKnPpmPtjDyyVVvIQ+uzJd9PQeWdO maWBZWPWI0Y9aNPK/9laQzlI507siTFReUe0yBgkpeNNrEYA5IvW9S/BO7g0TOK1q6XN Oigg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=w06XLuTGv42ExA7kvsEzi3HmqNn0e+XIKH+7gH1xWFk=; b=blDcpiTib/AYU7ZaXvc4p06kEhvbBpMycwI/ipACAgdNo8tl3n5akLt5igsIKQ16xK KkAroJ4lZZS+8CuWj86o56QEsclT0ZqjDapdJvjI18HS9K0uAflgsIf1G36Ahund9SVH 1/0M9+KHFdoLyqwGLijrLNnDS4g6ZlPlsWeam9Ssrr9VPlS6K1R1MbyTXQUswIMHyaIM fCL3p8sTpnTp8OWsyu2vQP42Ns7iwoYECe+oQQe+Ja05be+8mi1LJF2fPJc5e0lXqqMX yvJkbRP9IOiWaYs5IdNn2LYucp4ccyYOTslaVzrJ5ZWWVaFCRnQoP3JMtjc4i8XmL7Q7 r2jg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@sifive.com header.s=google header.b=PPfshnun; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id a204-20020a621ad5000000b00592591f1972si11824132pfa.64.2023.01.30.01.36.12; Mon, 30 Jan 2023 01:36:24 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@sifive.com header.s=google header.b=PPfshnun; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236123AbjA3Jer (ORCPT <rfc822;n2h9z4@gmail.com> + 99 others); Mon, 30 Jan 2023 04:34:47 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34894 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235949AbjA3JeP (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 30 Jan 2023 04:34:15 -0500 Received: from mail-pj1-x1036.google.com (mail-pj1-x1036.google.com [IPv6:2607:f8b0:4864:20::1036]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id CEE05976E for <linux-kernel@vger.kernel.org>; Mon, 30 Jan 2023 01:32:55 -0800 (PST) Received: by mail-pj1-x1036.google.com with SMTP id rm7-20020a17090b3ec700b0022c05558d22so10467149pjb.5 for <linux-kernel@vger.kernel.org>; Mon, 30 Jan 2023 01:32:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sifive.com; s=google; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=w06XLuTGv42ExA7kvsEzi3HmqNn0e+XIKH+7gH1xWFk=; b=PPfshnuneZVuaGMsWsf0abmeP49jp4AxSkkBj/mh1QI/DzHKXdaR6hl+H+ItCxqyBK 8EvW4xXW6VegaasREhK2bUhShb9wvty8UOLr3wp/rbISnmKVHRVvA0rpCOSUt0MCj0ZR oPhRoLukyOzJLiU4C/lp/aLdGjZGUoPkShPRj19HLGlGJVneY/5JrOxKgYSDAs+kHckA ZbAar3Gjnh03WMx7A3k5Ncx3kq8vJdKp0fzptQZusU7UMj0/chZLjSo/ClNBAAPUYkSd 8bm/XpTxdY/MmYvytxFvGcJV04KPmAG08wqBrYkaeFcuRA8URaK7P1WcmVaJR2IZIpdw jMhQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=w06XLuTGv42ExA7kvsEzi3HmqNn0e+XIKH+7gH1xWFk=; b=YfFZfKKUvIPcHiSYYWNTwp//vEHixxZ161ii8aDnHEdsGW+qoACooiJPd7edgh1XYT 09q0ODlqcAtCPs++lcqg2KXHJ3328BPTRdzHer0sejwWVJBDFePcjKD0GgWjV56PPzIM QCo5cJxp54BXHzV9r7RzWfCVqfF/5EMW5VbtLbkW8lroTM1WFsIpNdUvCNK6gvGFXyCK tYLeNp5fkiuIcv/cT3BWyU5xGI6ivH3Hyt6SsKtyCqueucPCgW7qQl9TUk1qgdc9UCin 9WfL8SaY7jwqL1PM0qO2Kh+ubjbXudjShmp5eJ6iiLIs3rtfgGNZiUPpV+DIjXCbkfa5 ug0g== X-Gm-Message-State: AFqh2kol0n6srg/Ib2UH1kTZbsl+iIuBxQBNYT1Gjc8WJ3ls5Ok0xKdJ Ims2+V30UMskhQmPXcn/3wR1rA== X-Received: by 2002:a05:6a20:1596:b0:b8:7905:b6c4 with SMTP id h22-20020a056a20159600b000b87905b6c4mr62293777pzj.61.1675071165340; Mon, 30 Jan 2023 01:32:45 -0800 (PST) Received: from hsinchu15.internal.sifive.com (59-124-168-89.hinet-ip.hinet.net. [59.124.168.89]) by smtp.gmail.com with ESMTPSA id t13-20020a6564cd000000b004db2b310f95sm6245704pgv.16.2023.01.30.01.32.42 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 30 Jan 2023 01:32:44 -0800 (PST) From: Nylon Chen <nylon.chen@sifive.com> To: aou@eecs.berkeley.edu, conor@kernel.org, emil.renner.berthing@canonical.com, geert+renesas@glider.be, heiko@sntech.de, krzysztof.kozlowski+dt@linaro.org, palmer@dabbelt.com, paul.walmsley@sifive.com, robh+dt@kernel.org, thierry.reding@gmail.com, u.kleine-koenig@pengutronix.de, devicetree@vger.kernel.org, linux-pwm@vger.kernel.org, linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org Cc: nylon.chen@sifive.com, nylon7717@gmail.com, zong.li@sifive.com, greentime.hu@sifive.com, vincent.chen@sifive.com Subject: [PATCH v2 2/2] pwm: sifive: change the PWM controlled LED algorithm Date: Mon, 30 Jan 2023 17:32:29 +0800 Message-Id: <20230130093229.27489-3-nylon.chen@sifive.com> X-Mailer: git-send-email 2.36.1 In-Reply-To: <20230130093229.27489-1-nylon.chen@sifive.com> References: <20230130093229.27489-1-nylon.chen@sifive.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1756439652556513946?= X-GMAIL-MSGID: =?utf-8?q?1756439652556513946?= |
Series |
Change PWM-controlled LED pin active mode and algorithm
|
|
Commit Message
Nylon Chen
Jan. 30, 2023, 9:32 a.m. UTC
The `frac` variable represents the pulse inactive time, and the result of
this algorithm is the pulse active time. Therefore, we must reverse the
result.
The reference is SiFive FU740-C000 Manual[0].
[0]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf
Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
---
drivers/pwm/pwm-sifive.c | 1 +
1 file changed, 1 insertion(+)
Comments
Hi Nylon, On Mon, Jan 30, 2023 at 10:32 AM Nylon Chen <nylon.chen@sifive.com> wrote: > The `frac` variable represents the pulse inactive time, and the result of > this algorithm is the pulse active time. Therefore, we must reverse the > result. > > The reference is SiFive FU740-C000 Manual[0]. > > [0]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf > > Signed-off-by: Nylon Chen <nylon.chen@sifive.com> Thanks for your patch! > --- a/drivers/pwm/pwm-sifive.c > +++ b/drivers/pwm/pwm-sifive.c > @@ -158,6 +158,7 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm, > frac = DIV64_U64_ROUND_CLOSEST(num, state->period); > /* The hardware cannot generate a 100% duty cycle */ > frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1); > + frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac; Shouldn't the inversion be done before the hardware limitation fixup? > > mutex_lock(&ddata->lock); > if (state->period != ddata->approx_period) { Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Mon, Jan 30, 2023 at 05:32:29PM +0800, Nylon Chen wrote: > The `frac` variable represents the pulse inactive time, and the result of > this algorithm is the pulse active time. Therefore, we must reverse the > result. > > The reference is SiFive FU740-C000 Manual[0]. > > [0]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf > > Signed-off-by: Nylon Chen <nylon.chen@sifive.com> > --- > drivers/pwm/pwm-sifive.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c > index 62b6acc6373d..a5eda165d071 100644 > --- a/drivers/pwm/pwm-sifive.c > +++ b/drivers/pwm/pwm-sifive.c > @@ -158,6 +158,7 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm, > frac = DIV64_U64_ROUND_CLOSEST(num, state->period); > /* The hardware cannot generate a 100% duty cycle */ > frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1); > + frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac; The same problem exists in pwm_sifive_get_state(), doesn't it? As fixing this is an interruptive change anyhow, this is the opportunity to align the driver to the rules tested by PWM_DEBUG. The problems I see in the driver (only checked quickly, so I might be wrong): - state->period != ddata->approx_period isn't necessarily a problem. If state->period > ddata->real_period that's fine and the driver should continue - frac = DIV64_U64_ROUND_CLOSEST(num, state->period); is wrong for two reasons: it should round down and use the real period. Best regards Uwe
Hi Geert, Thanks for your reply. Geert Uytterhoeven <geert@linux-m68k.org> 於 2023年1月30日 週一 下午5:53寫道: > > Hi Nylon, > > On Mon, Jan 30, 2023 at 10:32 AM Nylon Chen <nylon.chen@sifive.com> wrote: > > The `frac` variable represents the pulse inactive time, and the result of > > this algorithm is the pulse active time. Therefore, we must reverse the > > result. > > > > The reference is SiFive FU740-C000 Manual[0]. > > > > [0]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf > > > > Signed-off-by: Nylon Chen <nylon.chen@sifive.com> > > Thanks for your patch! > > > --- a/drivers/pwm/pwm-sifive.c > > +++ b/drivers/pwm/pwm-sifive.c > > @@ -158,6 +158,7 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > frac = DIV64_U64_ROUND_CLOSEST(num, state->period); > > /* The hardware cannot generate a 100% duty cycle */ > > frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1); > > + frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac; > > Shouldn't the inversion be done before the hardware limitation fixup? I think your inference is correct, I will use it. thanks a lot. > > > > > mutex_lock(&ddata->lock); > > if (state->period != ddata->approx_period) { > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi Uwe, Thanks for your reply. Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 於 2023年1月30日 週一 下午6:17寫道: > > On Mon, Jan 30, 2023 at 05:32:29PM +0800, Nylon Chen wrote: > > The `frac` variable represents the pulse inactive time, and the result of > > this algorithm is the pulse active time. Therefore, we must reverse the > > result. > > > > The reference is SiFive FU740-C000 Manual[0]. > > > > [0]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf > > > > Signed-off-by: Nylon Chen <nylon.chen@sifive.com> > > --- > > drivers/pwm/pwm-sifive.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c > > index 62b6acc6373d..a5eda165d071 100644 > > --- a/drivers/pwm/pwm-sifive.c > > +++ b/drivers/pwm/pwm-sifive.c > > @@ -158,6 +158,7 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > frac = DIV64_U64_ROUND_CLOSEST(num, state->period); > > /* The hardware cannot generate a 100% duty cycle */ > > frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1); > > + frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac; > > The same problem exists in pwm_sifive_get_state(), doesn't it? > > As fixing this is an interruptive change anyhow, this is the opportunity > to align the driver to the rules tested by PWM_DEBUG. > > The problems I see in the driver (only checked quickly, so I might be > wrong): > > - state->period != ddata->approx_period isn't necessarily a problem. If > state->period > ddata->real_period that's fine and the driver should > continue > > - frac = DIV64_U64_ROUND_CLOSEST(num, state->period); > is wrong for two reasons: > it should round down and use the real period. > I need a little time to clarify your assumptions. If possible, I will make similar changes. e.g. rounddown(num, state->period); if (state->period < ddata->approx_period) ... thanks a lot. > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi Uwe, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 於 2023年1月30日 週一 下午6:17寫道: > > On Mon, Jan 30, 2023 at 05:32:29PM +0800, Nylon Chen wrote: > > The `frac` variable represents the pulse inactive time, and the result of > > this algorithm is the pulse active time. Therefore, we must reverse the > > result. > > > > The reference is SiFive FU740-C000 Manual[0]. > > > > [0]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf > > > > Signed-off-by: Nylon Chen <nylon.chen@sifive.com> > > --- > > drivers/pwm/pwm-sifive.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c > > index 62b6acc6373d..a5eda165d071 100644 > > --- a/drivers/pwm/pwm-sifive.c > > +++ b/drivers/pwm/pwm-sifive.c > > @@ -158,6 +158,7 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > frac = DIV64_U64_ROUND_CLOSEST(num, state->period); > > /* The hardware cannot generate a 100% duty cycle */ > > frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1); > > + frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac; > > The same problem exists in pwm_sifive_get_state(), doesn't it? > > As fixing this is an interruptive change anyhow, this is the opportunity > to align the driver to the rules tested by PWM_DEBUG. > > The problems I see in the driver (only checked quickly, so I might be > wrong): > > - state->period != ddata->approx_period isn't necessarily a problem. If > state->period > ddata->real_period that's fine and the driver should > continue > > - frac = DIV64_U64_ROUND_CLOSEST(num, state->period); > is wrong for two reasons: > it should round down and use the real period. are you mean state->period is a redundancy variable so we can use ddata->real_period directly? it seems reasonable, but I don't get your point, why do we need to change the algorithm to DIV_ROUND_DOWN_ULL() and change the if-else condition. frac = DIV_ROUND_DOWN_ULL(num, ddata->real_period); if (state->period < ddata->approx_period) { ... } > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi Uwe, Nylon Chen <nylon.chen@sifive.com> 於 2023年2月3日 週五 下午4:06寫道: > > Hi Uwe, > > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 於 2023年1月30日 週一 下午6:17寫道: > > > > On Mon, Jan 30, 2023 at 05:32:29PM +0800, Nylon Chen wrote: > > > The `frac` variable represents the pulse inactive time, and the result of > > > this algorithm is the pulse active time. Therefore, we must reverse the > > > result. > > > > > > The reference is SiFive FU740-C000 Manual[0]. > > > > > > [0]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf > > > > > > Signed-off-by: Nylon Chen <nylon.chen@sifive.com> > > > --- > > > drivers/pwm/pwm-sifive.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c > > > index 62b6acc6373d..a5eda165d071 100644 > > > --- a/drivers/pwm/pwm-sifive.c > > > +++ b/drivers/pwm/pwm-sifive.c > > > @@ -158,6 +158,7 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > frac = DIV64_U64_ROUND_CLOSEST(num, state->period); > > > /* The hardware cannot generate a 100% duty cycle */ > > > frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1); > > > + frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac; > > > > The same problem exists in pwm_sifive_get_state(), doesn't it? > > > > As fixing this is an interruptive change anyhow, this is the opportunity > > to align the driver to the rules tested by PWM_DEBUG. > > > > The problems I see in the driver (only checked quickly, so I might be > > wrong): > > > > > - state->period != ddata->approx_period isn't necessarily a problem. If > > state->period > ddata->real_period that's fine and the driver should > > continue > > > > - frac = DIV64_U64_ROUND_CLOSEST(num, state->period); > > is wrong for two reasons: > > it should round down and use the real period. I have some results from my observations regarding the questions you raised. I don't know if what we are thinking is the same thing. If my assumptions are different from yours, please let me know. Thanks. > are you mean state->period is a redundancy variable so we can use > ddata->real_period directly? > > it seems reasonable, but I don't get your point, why do we need to > change the algorithm to DIV_ROUND_DOWN_ULL() and change the if-else > condition. > > frac = DIV_ROUND_DOWN_ULL(num, ddata->real_period); > if (state->period < ddata->approx_period) { > ... > } > > > > > Best regards > > Uwe > > > > -- > > Pengutronix e.K. | Uwe Kleine-König | > > Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello Nylon, On Wed, Feb 01, 2023 at 04:56:42PM +0800, Nylon Chen wrote: > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 於 2023年1月30日 週一 下午6:17寫道: > > On Mon, Jan 30, 2023 at 05:32:29PM +0800, Nylon Chen wrote: > > > The `frac` variable represents the pulse inactive time, and the result of > > > this algorithm is the pulse active time. Therefore, we must reverse the > > > result. > > > > > > The reference is SiFive FU740-C000 Manual[0]. > > > > > > [0]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf > > > > > > Signed-off-by: Nylon Chen <nylon.chen@sifive.com> > > > --- > > > drivers/pwm/pwm-sifive.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c > > > index 62b6acc6373d..a5eda165d071 100644 > > > --- a/drivers/pwm/pwm-sifive.c > > > +++ b/drivers/pwm/pwm-sifive.c > > > @@ -158,6 +158,7 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > frac = DIV64_U64_ROUND_CLOSEST(num, state->period); > > > /* The hardware cannot generate a 100% duty cycle */ > > > frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1); > > > + frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac; > > > > The same problem exists in pwm_sifive_get_state(), doesn't it? > > > > As fixing this is an interruptive change anyhow, this is the opportunity > > to align the driver to the rules tested by PWM_DEBUG. > > > > The problems I see in the driver (only checked quickly, so I might be > > wrong): > > > > - state->period != ddata->approx_period isn't necessarily a problem. If > > state->period > ddata->real_period that's fine and the driver should > > continue > > > > - frac = DIV64_U64_ROUND_CLOSEST(num, state->period); > > is wrong for two reasons: > > it should round down and use the real period. > > > I need a little time to clarify your assumptions. If possible, I will > make similar changes. > > e.g. > rounddown(num, state->period); > if (state->period < ddata->approx_period) > ... the idea is that for a given request apply should do the following to select the hardware setting: - Check polarity, if the hardware doesn't support it, return -EINVAL. (A period always starts with the active phase for the duration of duty_cycle. For normal polarity active = high.) - Pick the biggest period length possible that is not bigger than the requested period. - For the picked period, select the biggest duty_cycle possible that is not bigger than the requested duty_cycle. Then if possible switch to the selected setting in an atomic step. Does this clearify your doubts? Best regards Uwe
Hi Uwe Thanks for your reply. Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 於 2023年3月1日 週三 下午5:21寫道: > > Hello Nylon, > > On Wed, Feb 01, 2023 at 04:56:42PM +0800, Nylon Chen wrote: > > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 於 2023年1月30日 週一 下午6:17寫道: > > > On Mon, Jan 30, 2023 at 05:32:29PM +0800, Nylon Chen wrote: > > > > The `frac` variable represents the pulse inactive time, and the result of > > > > this algorithm is the pulse active time. Therefore, we must reverse the > > > > result. > > > > > > > > The reference is SiFive FU740-C000 Manual[0]. > > > > > > > > [0]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf > > > > > > > > Signed-off-by: Nylon Chen <nylon.chen@sifive.com> > > > > --- > > > > drivers/pwm/pwm-sifive.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c > > > > index 62b6acc6373d..a5eda165d071 100644 > > > > --- a/drivers/pwm/pwm-sifive.c > > > > +++ b/drivers/pwm/pwm-sifive.c > > > > @@ -158,6 +158,7 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > > frac = DIV64_U64_ROUND_CLOSEST(num, state->period); > > > > /* The hardware cannot generate a 100% duty cycle */ > > > > frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1); > > > > + frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac; > > > > > > The same problem exists in pwm_sifive_get_state(), doesn't it? > > > > > > As fixing this is an interruptive change anyhow, this is the opportunity > > > to align the driver to the rules tested by PWM_DEBUG. > > > > > > The problems I see in the driver (only checked quickly, so I might be > > > wrong): > > > > > > - state->period != ddata->approx_period isn't necessarily a problem. If > > > state->period > ddata->real_period that's fine and the driver should > > > continue > > > > > > - frac = DIV64_U64_ROUND_CLOSEST(num, state->period); > > > is wrong for two reasons: > > > it should round down and use the real period. > > > > > I need a little time to clarify your assumptions. If possible, I will > > make similar changes. > > > > e.g. > > rounddown(num, state->period); > > if (state->period < ddata->approx_period) > > ... > > the idea is that for a given request apply should do the following to > select the hardware setting: > > - Check polarity, if the hardware doesn't support it, return -EINVAL. > (A period always starts with the active phase for the duration of > duty_cycle. For normal polarity active = high.) > - Pick the biggest period length possible that is not bigger than the > requested period. > - For the picked period, select the biggest duty_cycle possible that is > not bigger than the requested duty_cycle. > > Then if possible switch to the selected setting in an atomic step. > > Does this clearify your doubts? I need a little time to clarify your assumptions. Thanks again. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello, On Fri, Sep 08, 2023 at 06:41:00PM +0800, Nylon Chen wrote: > Sorry it's so long ago. > > I have completed the implementation of the new version, but there is > one thing about this letter that I still don't quite understand. > > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 於 2023年1月30日 週一 下午6:17寫道: > > > > On Mon, Jan 30, 2023 at 05:32:29PM +0800, Nylon Chen wrote: > > > The `frac` variable represents the pulse inactive time, and the result of > > > this algorithm is the pulse active time. Therefore, we must reverse the > > > result. > > > > > > The reference is SiFive FU740-C000 Manual[0]. > > > > > > [0]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf > > > > > > Signed-off-by: Nylon Chen <nylon.chen@sifive.com> > > > --- > > > drivers/pwm/pwm-sifive.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c > > > index 62b6acc6373d..a5eda165d071 100644 > > > --- a/drivers/pwm/pwm-sifive.c > > > +++ b/drivers/pwm/pwm-sifive.c > > > @@ -158,6 +158,7 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > frac = DIV64_U64_ROUND_CLOSEST(num, state->period); > > > /* The hardware cannot generate a 100% duty cycle */ > > > frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1); > > > + frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac; > > > > The same problem exists in pwm_sifive_get_state(), doesn't it? > > > > As fixing this is an interruptive change anyhow, this is the opportunity > > to align the driver to the rules tested by PWM_DEBUG. > > > > The problems I see in the driver (only checked quickly, so I might be > > wrong): > > > > - state->period != ddata->approx_period isn't necessarily a problem. If > > state->period > ddata->real_period that's fine and the driver should > > continue > > I still don’t quite understand the description of this paragraph. > > state->period != ddate->approx_period seems to be used to compare the > results of the previous and next two times. There are two things to consider: - usually the hardware doesn't support all requestable states because the hardware's quantum is > 1 ns. That is, it might for example support periods in the form (160 ns * i / 3) for i in 1 .. 1023. If this hardware runs with i = 500 (that is period ~= 26666.66 ns) because the first channel is configured to run with period = 26667, and .request is called for the 2nd channel with .period = 26700 ns, there is no need to refuse that, because 26666.66 is the best possible approximation for 26700 ns anyhow. - .apply is supposed to implement the highest possible period that isn't bigger than the requested period. So in the above case even if the hardware runs at 26666.66 ns without the possibility to change that, a request to configure for period = 30000 ns could succeed (and keep 26666.66 ns). > Would you suggest I send the new implementation version before > continuing the discussion? Note that the above implements the optimal behaviour for a driver. (For some definition of "optimal" that admittedly also yields strange behaviour at times. The reasoning for this to the be thing to implement is, that's the corner cases are easier to implement, idempotency is possible and it's easier to work with than rounding to the nearest possible value.) While I'd like to see the sifive driver to implement this optimal behaviour, it's not mandatory that you convert the driver to that behaviour. Just make sure to not make it worse. So to answer your question: If you understood what I wrote above and are motivated to improve the driver, it would be great to do that before the next review round. Best regards Uwe
diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c index 62b6acc6373d..a5eda165d071 100644 --- a/drivers/pwm/pwm-sifive.c +++ b/drivers/pwm/pwm-sifive.c @@ -158,6 +158,7 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm, frac = DIV64_U64_ROUND_CLOSEST(num, state->period); /* The hardware cannot generate a 100% duty cycle */ frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1); + frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac; mutex_lock(&ddata->lock); if (state->period != ddata->approx_period) {