Message ID | 20230602103211.2199283-1-gnstark@sberdevices.ru |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp934667vqr; Fri, 2 Jun 2023 03:51:47 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ7NlNBS9QzGpxuPuErkGtzKZGDrbg5+avtuSeicuVneFi3QXyObplWDkqwIv4J5o7lQJASu X-Received: by 2002:a17:90a:bc85:b0:253:9754:bed1 with SMTP id x5-20020a17090abc8500b002539754bed1mr2008305pjr.6.1685703107312; Fri, 02 Jun 2023 03:51:47 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1685703107; cv=none; d=google.com; s=arc-20160816; b=P2r9c+Kbg3guweppoATyfnqrAAo1AXta/GQVRClQmuQdyMCQyeLrQWyG0pfuDzD/9B fG9Y96uRIOtoJvSi8rJwi2gUx1JUCEowMmdf6z8hhat6mQf9mvgVy/Fcy0ewSy8wU83q 87GV5vdg3y1N90xjf8Bu1wkz5Ru5csDZH5v4jOhSC8jQ9GnhF5uuaKQFuKN+FjCyhZAI mQB6t4Vaoh79tQ5yB2xmnGsglRxcaedTkon3xTOitqoFyBsjvUO5vRfMYntpZaG3JSow ZgftIOGkQ+S5En60jNcRZFltXvmi2bqSpTBMDN516pm8VJ/hRiR+5GwqzvTcBWctJbJm tDZw== 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 :message-id:date:subject:cc:to:from:dkim-signature; bh=U2UnoUMYRs9m/BLycNQ2iLq6bQeVm56LTFKDcGw5E4A=; b=kTjfAuFbfwKV+ARGn7rnGW7Mu/orT1lPm2i3F5qzMSUnt3YgVFh+ANF5UgkVCWHZpP E0yBbhEVbTzmDo0cJO9twBh3rjCbRWb14L5MbMZWae8j/uWRozEDtmVaEaJv3bdp5dQ5 qQrF6NyrWgXW7WnbjW8DL8EEkQFmAPdApC/wSk413Gw/wF55Arq7PhzP6YxJbB3erfbn Vn6vXA7c1RGroK1RuL8zbjQSUD16y1Bna5GDGaJCrPPjTGaEP8budBR+5auMIMdOt+HO tC6ciT4Ubj9IVZHZM/SzCV7VLXi4okEbTsxanL0gl+pJRHcxQ5M/VKuAkI/YNMz89BK1 dHFw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@sberdevices.ru header.s=mail header.b=dDpfb4ns; 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; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=sberdevices.ru Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id o64-20020a17090a0a4600b002533ea2ad58si2596983pjo.140.2023.06.02.03.51.35; Fri, 02 Jun 2023 03:51:47 -0700 (PDT) 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=@sberdevices.ru header.s=mail header.b=dDpfb4ns; 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; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=sberdevices.ru Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235425AbjFBKfi (ORCPT <rfc822;limurcpp@gmail.com> + 99 others); Fri, 2 Jun 2023 06:35:38 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54648 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235834AbjFBKe6 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 2 Jun 2023 06:34:58 -0400 Received: from mx.sberdevices.ru (mx.sberdevices.ru [45.89.227.171]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0312910E3; Fri, 2 Jun 2023 03:33:40 -0700 (PDT) Received: from s-lin-edge02.sberdevices.ru (localhost [127.0.0.1]) by mx.sberdevices.ru (Postfix) with ESMTP id 2390C5FD03; Fri, 2 Jun 2023 13:32:30 +0300 (MSK) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sberdevices.ru; s=mail; t=1685701950; bh=U2UnoUMYRs9m/BLycNQ2iLq6bQeVm56LTFKDcGw5E4A=; h=From:To:Subject:Date:Message-ID:MIME-Version:Content-Type; b=dDpfb4nspYq2IX79ggRqGaSgsWFxQt9B9+ivAjCuKkPYK+q/bDLe82NvRXQUIoP+J 1UgfCSEnRVAf5ne/aOauc+fEm69F8FQcHNj3jabszSu48HnRuxfrN2df70IxJudXUL EbvNvDia+9mnO4Yc2jgEksvdVUhgVqGchpdhHZ3m6PqX55BggvX0jUK6f9Y8poxFpZ cWQX7YPm52iN0PKBRwsknc5qKJgZX6pM4h4ofywpJyYrEW003YvZ8+H+HYySWIge0D wMaJq5LKgKm+TuCkbyW0xzWe44pGE7/VvS1mPdJvdQtxM6Wr43p6CZh8SGtVssXyPz w2WiIR79N/Yvg== Received: from S-MS-EXCH01.sberdevices.ru (S-MS-EXCH01.sberdevices.ru [172.16.1.4]) by mx.sberdevices.ru (Postfix) with ESMTP; Fri, 2 Jun 2023 13:32:29 +0300 (MSK) From: George Stark <gnstark@sberdevices.ru> To: <thierry.reding@gmail.com>, <u.kleine-koenig@pengutronix.de>, <neil.armstrong@linaro.org>, <khilman@baylibre.com>, <jbrunet@baylibre.com>, <martin.blumenstingl@googlemail.com>, <hkallweit1@gmail.com> CC: <linux-iio@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>, <linux-amlogic@lists.infradead.org>, <kernel@sberdevices.ru>, George Stark <GNStark@sberdevices.ru>, Dmitry Rokosov <ddrokosov@sberdevices.ru> Subject: [PATCH] pwm: meson: compute cnt register value in proper way Date: Fri, 2 Jun 2023 13:32:11 +0300 Message-ID: <20230602103211.2199283-1-gnstark@sberdevices.ru> X-Mailer: git-send-email 2.40.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-Originating-IP: [172.16.1.6] X-ClientProxiedBy: S-MS-EXCH02.sberdevices.ru (172.16.1.5) To S-MS-EXCH01.sberdevices.ru (172.16.1.4) X-KSMG-Rule-ID: 4 X-KSMG-Message-Action: clean X-KSMG-AntiSpam-Status: not scanned, disabled by settings X-KSMG-AntiSpam-Interceptor-Info: not scanned X-KSMG-AntiPhishing: not scanned, disabled by settings X-KSMG-AntiVirus: Kaspersky Secure Mail Gateway, version 1.1.2.30, bases: 2023/06/02 03:06:00 #21401484 X-KSMG-AntiVirus-Status: Clean, skipped X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_NONE, T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED 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?1767587821552309572?= X-GMAIL-MSGID: =?utf-8?q?1767587821552309572?= |
Series |
pwm: meson: compute cnt register value in proper way
|
|
Commit Message
George Stark
June 2, 2023, 10:32 a.m. UTC
According to the datasheet, the PWM high and low clock count values should be set to at least one. Therefore, setting the clock count register to 0 actually means 1 clock count. Signed-off-by: George Stark <GNStark@sberdevices.ru> Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru> --- This patch is based on currently unmerged patch by Heiner Kallweit https://lore.kernel.org/linux-amlogic/23fe625e-dc23-4db8-3dce-83167cd3b206@gmail.com ---
Comments
On 02.06.2023 12:32, George Stark wrote: > According to the datasheet, the PWM high and low clock count values > should be set to at least one. Therefore, setting the clock count > register to 0 actually means 1 clock count. > > Signed-off-by: George Stark <GNStark@sberdevices.ru> > Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru> > --- > This patch is based on currently unmerged patch by Heiner Kallweit > https://lore.kernel.org/linux-amlogic/23fe625e-dc23-4db8-3dce-83167cd3b206@gmail.com > --- > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c > index 834acd7..57e7d9c 100644 > --- a/drivers/pwm/pwm-meson.c > +++ b/drivers/pwm/pwm-meson.c > @@ -206,6 +206,11 @@ > channel->pre_div = pre_div; > channel->hi = duty_cnt; > channel->lo = cnt - duty_cnt; > + > + if (channel->hi) > + channel->hi--; > + if (channel->lo) > + channel->lo--; I'm not sure whether we should do this. duty_cnt and cnt are results of an integer division and therefore potentially rounded down. The chip-internal increment may help to compensate such rounding errors, so to say. With the proposed change we may end up with the effective period being shorter than the requested one. And IIRC this should not happen. > } > > return 0; > @@ -340,7 +345,8 @@ > channel->lo = FIELD_GET(PWM_LOW_MASK, value); > channel->hi = FIELD_GET(PWM_HIGH_MASK, value); > > - state->period = meson_pwm_cnt_to_ns(chip, pwm, channel->lo + channel->hi); > + state->period = meson_pwm_cnt_to_ns(chip, pwm, > + channel->lo + 1 + channel->hi + 1); > state->duty_cycle = meson_pwm_cnt_to_ns(chip, pwm, channel->hi); > Doesn't channel->hi have to be incremented here too? > return 0;
On 6/2/23 23:52, Heiner Kallweit wrote: > On 02.06.2023 12:32, George Stark wrote: >> According to the datasheet, the PWM high and low clock count values >> should be set to at least one. Therefore, setting the clock count >> register to 0 actually means 1 clock count. >> >> Signed-off-by: George Stark <GNStark@sberdevices.ru> >> Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru> >> --- >> This patch is based on currently unmerged patch by Heiner Kallweit >> https://lore.kernel.org/linux-amlogic/23fe625e-dc23-4db8-3dce-83167cd3b206@gmail.com >> --- >> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c >> index 834acd7..57e7d9c 100644 >> --- a/drivers/pwm/pwm-meson.c >> +++ b/drivers/pwm/pwm-meson.c >> @@ -206,6 +206,11 @@ >> channel->pre_div = pre_div; >> channel->hi = duty_cnt; >> channel->lo = cnt - duty_cnt; >> + >> + if (channel->hi) >> + channel->hi--; >> + if (channel->lo) >> + channel->lo--; Hello Heiner Thanks for review > I'm not sure whether we should do this. duty_cnt and cnt are results > of an integer division and therefore potentially rounded down. > The chip-internal increment may help to compensate such rounding > errors, so to say. With the proposed change we may end up with the > effective period being shorter than the requested one. Although chip-internal increment sometimes may help accidentally there are cases when the increment ruins precise calculation in unexpected way. Here's our experience on meson a113l (meson-a1) with pwm driver based on ccf: we need to get pwm period as close as possible to 32768hz. config pwm to period 1/32768 = 30517ns, duty 15258n How driver calculates hi\lo regs: rate = NSEC_PER_SEC * 0xffff / 30517 = ~2147Mhz rate = clk_round_rate(rate) clk_round_rate selects fastest parent clock which is 64Mhz in our case then calculating hi\lo at last: period= mul_u64_u64_div_u64(rate, state->period, NSEC_PER_SEC); // 1953 duty= mul_u64_u64_div_u64(rate, state->duty_cycle, NSEC_PER_SEC); // 976 channel->hi= duty; channel->lo= period- duty; with the internal increment we'll have real output (1953-976 + 1 + 976 + 1) * 1 / 64Mhz = 32736.57Hz but we should have (1953-976 + 976) * 1 / 64Mhz = 32770.09Hz | And IIRC this should not happen. Could you please explain why or point out doc/description where it's stated? If so we can add explicit check to prevent such a case >> } >> >> return 0; >> @@ -340,7 +345,8 @@ >> channel->lo = FIELD_GET(PWM_LOW_MASK, value); >> channel->hi = FIELD_GET(PWM_HIGH_MASK, value); >> >> - state->period = meson_pwm_cnt_to_ns(chip, pwm, channel->lo + channel->hi); >> + state->period = meson_pwm_cnt_to_ns(chip, pwm, >> + channel->lo + 1 + channel->hi + 1); >> state->duty_cycle = meson_pwm_cnt_to_ns(chip, pwm, channel->hi); >> > Doesn't channel->hi have to be incremented here too? Yes, lost the line. I'll fix it Best regards George >> return 0; >
On 05.06.2023 09:11, George Stark wrote: > On 6/2/23 23:52, Heiner Kallweit wrote: >> On 02.06.2023 12:32, George Stark wrote: >>> According to the datasheet, the PWM high and low clock count values >>> should be set to at least one. Therefore, setting the clock count >>> register to 0 actually means 1 clock count. >>> >>> Signed-off-by: George Stark <GNStark@sberdevices.ru> >>> Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru> >>> --- >>> This patch is based on currently unmerged patch by Heiner Kallweit >>> https://lore.kernel.org/linux-amlogic/23fe625e-dc23-4db8-3dce-83167cd3b206@gmail.com >>> --- >>> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c >>> index 834acd7..57e7d9c 100644 >>> --- a/drivers/pwm/pwm-meson.c >>> +++ b/drivers/pwm/pwm-meson.c >>> @@ -206,6 +206,11 @@ >>> channel->pre_div = pre_div; >>> channel->hi = duty_cnt; >>> channel->lo = cnt - duty_cnt; >>> + >>> + if (channel->hi) >>> + channel->hi--; >>> + if (channel->lo) >>> + channel->lo--; > Hello Heiner > > Thanks for review >> I'm not sure whether we should do this. duty_cnt and cnt are results >> of an integer division and therefore potentially rounded down. >> The chip-internal increment may help to compensate such rounding >> errors, so to say. With the proposed change we may end up with the >> effective period being shorter than the requested one. > Although chip-internal increment sometimes may help accidentally > there are cases when the increment ruins precise calculation in unexpected way. > > Here's our experience on meson a113l (meson-a1) with pwm driver based on ccf: > we need to get pwm period as close as possible to 32768hz. > config pwm to period 1/32768 = 30517ns, duty 15258n > How driver calculates hi\lo regs: > rate = NSEC_PER_SEC * 0xffff / 30517 = ~2147Mhz > rate = clk_round_rate(rate) clk_round_rate selects fastest parent clock which is 64Mhz in our case then calculating hi\lo at last: period= mul_u64_u64_div_u64(rate, state->period, NSEC_PER_SEC); // 1953 > duty= mul_u64_u64_div_u64(rate, state->duty_cycle, NSEC_PER_SEC); // 976 > channel->hi= duty; > channel->lo= period- duty; > with the internal increment we'll have real output (1953-976 + 1 + 976 + 1) * 1 / 64Mhz = 32736.57Hz but we should have (1953-976 + 976) * 1 / 64Mhz = 32770.09Hz Supposedly, depending on the prior rounding errors, something incrementing, and sometimes not incrementing may provide the more precise result. Another source of error is shown your example, the duty cycle isn't 50% due to the rounding. Not sure however where there's any use case where such small deviations would cause problems. Therefore I don't have a strong opinion. > | And IIRC this should not happen. > Could you please explain why or point out doc/description where it's stated? > If so we can add explicit check to prevent such a case I think I got this wrong. When checking where I got this information from I found the following in pwm_apply_state_debug(): if (state->enabled && state->period < s2.period) dev_warn(chip->dev, ".apply is supposed to round down period (requested: %llu, applied: %llu)\n", state->period, s2.period); >>> } >>> return 0; >>> @@ -340,7 +345,8 @@ >>> channel->lo = FIELD_GET(PWM_LOW_MASK, value); >>> channel->hi = FIELD_GET(PWM_HIGH_MASK, value); >>> - state->period = meson_pwm_cnt_to_ns(chip, pwm, channel->lo + channel->hi); >>> + state->period = meson_pwm_cnt_to_ns(chip, pwm, >>> + channel->lo + 1 + channel->hi + 1); >>> state->duty_cycle = meson_pwm_cnt_to_ns(chip, pwm, channel->hi); >>> >> Doesn't channel->hi have to be incremented here too? > Yes, lost the line. I'll fix it > > Best regards > George >>> return 0; >> >
Hello, just a small note: This patch wasn't sent to the pwm mailing list and so is missing in our patchwork instance. I didn't follow the discussion, but please (and in your own interest) make sure to Cc: linux-pwm@vger.kernel.org for the next revision (or resend if the conclusion of the discussion is that the patch is fine). Best regards Uwe
On Mon, Jun 05, 2023 at 11:01:40PM +0200, Heiner Kallweit wrote: > On 05.06.2023 09:11, George Stark wrote: > > On 6/2/23 23:52, Heiner Kallweit wrote: > >> On 02.06.2023 12:32, George Stark wrote: > >>> According to the datasheet, the PWM high and low clock count values > >>> should be set to at least one. Therefore, setting the clock count > >>> register to 0 actually means 1 clock count. > >>> > >>> Signed-off-by: George Stark <GNStark@sberdevices.ru> > >>> Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru> > >>> --- > >>> This patch is based on currently unmerged patch by Heiner Kallweit > >>> https://lore.kernel.org/linux-amlogic/23fe625e-dc23-4db8-3dce-83167cd3b206@gmail.com > >>> --- > >>> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c > >>> index 834acd7..57e7d9c 100644 > >>> --- a/drivers/pwm/pwm-meson.c > >>> +++ b/drivers/pwm/pwm-meson.c > >>> @@ -206,6 +206,11 @@ > >>> channel->pre_div = pre_div; > >>> channel->hi = duty_cnt; > >>> channel->lo = cnt - duty_cnt; > >>> + > >>> + if (channel->hi) > >>> + channel->hi--; > >>> + if (channel->lo) > >>> + channel->lo--; > > Hello Heiner > > > > Thanks for review > >> I'm not sure whether we should do this. duty_cnt and cnt are results > >> of an integer division and therefore potentially rounded down. > >> The chip-internal increment may help to compensate such rounding > >> errors, so to say. With the proposed change we may end up with the > >> effective period being shorter than the requested one. > > Although chip-internal increment sometimes may help accidentally > > there are cases when the increment ruins precise calculation in unexpected way. > > > > Here's our experience on meson a113l (meson-a1) with pwm driver based on ccf: > > we need to get pwm period as close as possible to 32768hz. > > config pwm to period 1/32768 = 30517ns, duty 15258n > > How driver calculates hi\lo regs: > > rate = NSEC_PER_SEC * 0xffff / 30517 = ~2147Mhz > > rate = clk_round_rate(rate) clk_round_rate selects fastest parent clock which is 64Mhz in our case then calculating hi\lo at last: period= mul_u64_u64_div_u64(rate, state->period, NSEC_PER_SEC); // 1953 > > duty= mul_u64_u64_div_u64(rate, state->duty_cycle, NSEC_PER_SEC); // 976 > > channel->hi= duty; > > channel->lo= period- duty; > > with the internal increment we'll have real output (1953-976 + 1 + 976 + 1) * 1 / 64Mhz = 32736.57Hz but we should have (1953-976 + 976) * 1 / 64Mhz = 32770.09Hz > > Supposedly, depending on the prior rounding errors, something incrementing, > and sometimes not incrementing may provide the more precise result. > Another source of error is shown your example, the duty cycle isn't 50% > due to the rounding. > Not sure however where there's any use case where such small deviations > would cause problems. Therefore I don't have a strong opinion. My strong opinion here is: .apply should implement the the best round-down setting. That is it should pick the maximal period that isn't bigger than the requested one, and for that period pick the maximal duty_cycle that isn't bigger than the requested one. I have some implementation ideas to allow consumers to want different rounding, but for that to work the .apply functions should all round the same way. > > | And IIRC this should not happen. > > Could you please explain why or point out doc/description where it's stated? > > If so we can add explicit check to prevent such a case > > I think I got this wrong. When checking where I got this information from > I found the following in pwm_apply_state_debug(): > > if (state->enabled && state->period < s2.period) > dev_warn(chip->dev, > ".apply is supposed to round down period (requested: %llu, applied: %llu)\n", > state->period, s2.period); pwm_apply_state_debug() also checks the above sketched rule. Best regards Uwe
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c index 834acd7..57e7d9c 100644 --- a/drivers/pwm/pwm-meson.c +++ b/drivers/pwm/pwm-meson.c @@ -206,6 +206,11 @@ channel->pre_div = pre_div; channel->hi = duty_cnt; channel->lo = cnt - duty_cnt; + + if (channel->hi) + channel->hi--; + if (channel->lo) + channel->lo--; } return 0; @@ -340,7 +345,8 @@ channel->lo = FIELD_GET(PWM_LOW_MASK, value); channel->hi = FIELD_GET(PWM_HIGH_MASK, value); - state->period = meson_pwm_cnt_to_ns(chip, pwm, channel->lo + channel->hi); + state->period = meson_pwm_cnt_to_ns(chip, pwm, + channel->lo + 1 + channel->hi + 1); state->duty_cycle = meson_pwm_cnt_to_ns(chip, pwm, channel->hi); return 0;