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 2BBBFC77B72 for ; Mon, 17 Apr 2023 07:24:42 +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-Type: Content-Transfer-Encoding:Reply-To:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:From:References:Cc:To: Subject:MIME-Version:Date:Message-ID:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=5eNA4kkIu5C7pmzD1iklx7axz7sx4oRXLLVQQFirMGU=; b=Tidhq6q8Bu8UId +u3W82J4EiBoZBXJy4YQSRBPtUreMiQ8Gn8eq8Sb7FmlyZpGiDQamjqqBxXEtiSnQGqL3GhMMJuK5 1+usZUSmcax1Lw54O/f/Zbn7wv8OAEcUJdIqPEsW6LLDV1QMZAB5gqLp/OoHKex+1JQOsp4/skCS+ 20kosKmOnrPiaiaq2cCIJyl9317i0hDJaageDlv4IHSCfRDj6jqyoFqLTwxmJpzga+KLObRLdALMf RKKKtf+fjQ85jE/sFpOxmDr9IIc3fWwUmB2HHKnNmSkpx3j0kXPk5E52HG9qnEIh5LpE9yAYAaj9/ bpbzwv+SRUb0OHQsk/wg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1poJDD-00FAN3-33; Mon, 17 Apr 2023 07:23:43 +0000 Received: from mail-wm1-x332.google.com ([2a00:1450:4864:20::332]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1poJD9-00FAKO-32 for linux-arm-kernel@lists.infradead.org; Mon, 17 Apr 2023 07:23:42 +0000 Received: by mail-wm1-x332.google.com with SMTP id 5b1f17b1804b1-3f09b4a1584so8206195e9.2 for ; Mon, 17 Apr 2023 00:23:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1681716216; x=1684308216; h=content-transfer-encoding:in-reply-to:organization:from:references :cc:to:content-language:subject:reply-to:user-agent:mime-version :date:message-id:from:to:cc:subject:date:message-id:reply-to; bh=U4bj2H9qmeVSso11SML7BIrdtQZppfzayCc2jlV8HSA=; b=Q2DSizZMUn8VJnRy7rHbmzMtasXXXxVoZBGXJC4yGiV9tabUw8E7dDSenuk+PTFlnR N26pwNntWAk7KVsDzsjmiITsplldcOklQ6iXqvOyHkPSudVIID0N8y3McU8b/Whki/w1 xQx468MKmsjpbEihR5MHQbCPdpiSTS56QmvdZZ8VhdGZUcruUbiYh+f+KmB7PRinETvg Cxcg4b1i84uFPUZLPVJkRnl5qZPGyO/mY45CUXwMIVUgBsTupznFbytbIF2JAyZ4cVhy wYIVnX00ilhaW2zoIhwD/SgMCknRdkYzvlZx/wraTaash0nNeWerauK58FpghdDiB466 U9lQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1681716216; x=1684308216; h=content-transfer-encoding:in-reply-to:organization:from:references :cc:to:content-language:subject:reply-to:user-agent:mime-version :date:message-id:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=U4bj2H9qmeVSso11SML7BIrdtQZppfzayCc2jlV8HSA=; b=RE0+z7gRYBNpsQN8+Qfc7R0hpz6+5RuCkERRUddxv+ILVYwSZyxFE/jLga/zzXmJ3x kFZhdXEvC+hl+U/hL/ZMwI8wAwiCXjWn6ALta4drFIqt1qad52omO3dquSHFdOvLuLKc G3VKEGYMi+9fUE64Gms2fBKMfmUkVwWrNShBQ7Gaz89IsrzgJFryidEHwFg7SMQJ7+B9 KTSuNps7b2HR7EHfYm2UXmW/yr3vlIDi0oCX6dGMQrnRZBQJISNdvyQI3TLGSCu8YbWA NMszVtUBBoTtfZzDZHJgl0fdZKJ8DHMdxwfMeqqFJ/pvNaE0/paSKLAoqlftHeFKaS6B cpMA== X-Gm-Message-State: AAQBX9eaAuQ4x1h0E50FMYtOm7/86YmTxdYSihDkYDaRxIswloQ2DlG+ tuaVhmUEM8AHfKvcK2UDu00cNA== X-Google-Smtp-Source: AKy350aA9XNG4LmNRdZi0YP7UNqmvXbzNkiIAU7N6IRbZEuVy28KvvZKl/jTW71vAjv6ZX3wJuaDBw== X-Received: by 2002:a5d:6e86:0:b0:2e5:56f8:61fc with SMTP id k6-20020a5d6e86000000b002e556f861fcmr4118050wrz.15.1681716215886; Mon, 17 Apr 2023 00:23:35 -0700 (PDT) Received: from ?IPV6:2a01:e0a:982:cbb0:a1e1:81a8:1acc:2b91? ([2a01:e0a:982:cbb0:a1e1:81a8:1acc:2b91]) by smtp.gmail.com with ESMTPSA id x15-20020a5d6b4f000000b002c8476dde7asm9775536wrw.114.2023.04.17.00.23.34 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 17 Apr 2023 00:23:35 -0700 (PDT) Message-ID: Date: Mon, 17 Apr 2023 09:23:35 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0 Subject: Re: [PATCH v4 4/4] pwm: meson: make full use of common clock framework Content-Language: en-US To: Heiner Kallweit , Jerome Brunet , Martin Blumenstingl , Neil Armstrong , Kevin Hilman , =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= , "thierry.reding@gmail.com" Cc: "linux-arm-kernel@lists.infradead.org" , "open list:ARM/Amlogic Meson..." , linux-pwm@vger.kernel.org References: <9faca2e6-b7a1-4748-7eb0-48f8064e323e@gmail.com> From: Neil Armstrong Organization: Linaro Developer Services In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230417_002339_977710_E36C14BD X-CRM114-Status: GOOD ( 46.51 ) 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: , Reply-To: neil.armstrong@linaro.org Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 13/04/2023 07:54, Heiner Kallweit wrote: > Newer versions of the PWM block use a core clock with external mux, > divider, and gate. These components either don't exist any longer in > the PWM block, or they are bypassed. > To minimize needed changes for supporting the new version, the internal > divider and gate should be handled by CCF too. > > I didn't see a good way to split the patch, therefore it's somewhat > bigger. What it does: > > - The internal mux is handled by CCF already. Register also internal > divider and gate with CCF, so that we have one representation of the > input clock: [mux] parent of [divider] parent of [gate] > > - Now that CCF selects an appropriate mux parent, we don't need the > DT-provided default parent any longer. Accordingly we can also omit > setting the mux parent directly in the driver. > > - Instead of manually handling the pre-div divider value, let CCF > set the input clock. Targeted input clock frequency is > 0xffff * 1/period for best precision. > > - For the "inverted pwm disabled" scenario target an input clock > frequency of 1GHz. This ensures that the remaining low pulses > have minimum length. > > I don't have hw with the old PWM block, therefore I couldn't test this > patch. With the not yet included extension for the new PWM block > (channel->clk coming directly from get_clk(external_clk)) I didn't > notice any problem. My system uses PWM for the CPU voltage regulator > and for the SDIO 32kHz clock. > > Note: The clock gate in the old PWM block is permanently disabled. > This seems to indicate that it's not used by the new PWM block. > > Tested-by: Martin Blumenstingl > Signed-off-by: Heiner Kallweit > --- > Changes to RFT/RFC version: > - use parent_hws instead of parent_names for div/gate clock > - use devm_clk_hw_register where the struct clk * returned by > devm_clk_register isn't needed > > v2: > - add patch 1 > - add patch 3 > - switch to using clk_parent_data in all relevant places > v3: > - add flag CLK_IGNORE_UNUSED > v4: > - remove variable tmp in meson_pwm_get_state > - don't use deprecated function devm_clk_register > --- > drivers/pwm/pwm-meson.c | 142 +++++++++++++++++++++++----------------- > 1 file changed, 83 insertions(+), 59 deletions(-) > > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c > index 40a8709ff..80ac71cbc 100644 > --- a/drivers/pwm/pwm-meson.c > +++ b/drivers/pwm/pwm-meson.c > @@ -51,7 +51,7 @@ > #define REG_MISC_AB 0x8 > #define MISC_B_CLK_EN 23 > #define MISC_A_CLK_EN 15 > -#define MISC_CLK_DIV_MASK 0x7f > +#define MISC_CLK_DIV_WIDTH 7 > #define MISC_B_CLK_DIV_SHIFT 16 > #define MISC_A_CLK_DIV_SHIFT 8 > #define MISC_B_CLK_SEL_SHIFT 6 > @@ -87,12 +87,13 @@ static struct meson_pwm_channel_data { > }; > > struct meson_pwm_channel { > + unsigned long rate; > unsigned int hi; > unsigned int lo; > - u8 pre_div; > > - struct clk *clk_parent; > struct clk_mux mux; > + struct clk_divider div; > + struct clk_gate gate; > struct clk *clk; > }; > > @@ -125,16 +126,6 @@ static int meson_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) > struct device *dev = chip->dev; > int err; > > - if (channel->clk_parent) { > - err = clk_set_parent(channel->clk, channel->clk_parent); > - if (err < 0) { > - dev_err(dev, "failed to set parent %s for %s: %d\n", > - __clk_get_name(channel->clk_parent), > - __clk_get_name(channel->clk), err); > - return err; > - } > - } > - > err = clk_prepare_enable(channel->clk); > if (err < 0) { > dev_err(dev, "failed to enable clock %s: %d\n", > @@ -157,8 +148,9 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, > const struct pwm_state *state) > { > struct meson_pwm_channel *channel = &meson->channels[pwm->hwpwm]; > - unsigned int duty, period, pre_div, cnt, duty_cnt; > + unsigned int duty, period, cnt, duty_cnt; > unsigned long fin_freq; > + u64 freq; > > duty = state->duty_cycle; > period = state->period; > @@ -166,7 +158,11 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, > if (state->polarity == PWM_POLARITY_INVERSED) > duty = period - duty; > > - fin_freq = clk_get_rate(channel->clk); > + freq = div64_u64(NSEC_PER_SEC * (u64)0xffff, period); > + if (freq > ULONG_MAX) > + freq = ULONG_MAX; > + > + fin_freq = clk_round_rate(channel->clk, freq); > if (fin_freq == 0) { > dev_err(meson->chip.dev, "invalid source clock frequency\n"); > return -EINVAL; > @@ -174,46 +170,35 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, > > dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq); > > - pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL); > - if (pre_div > MISC_CLK_DIV_MASK) { > - dev_err(meson->chip.dev, "unable to get period pre_div\n"); > - return -EINVAL; > - } > - > - cnt = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * (pre_div + 1)); > + cnt = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC); > if (cnt > 0xffff) { > dev_err(meson->chip.dev, "unable to get period cnt\n"); > return -EINVAL; > } > > - dev_dbg(meson->chip.dev, "period=%u pre_div=%u cnt=%u\n", period, > - pre_div, cnt); > + dev_dbg(meson->chip.dev, "period=%u cnt=%u\n", period, cnt); > > if (duty == period) { > - channel->pre_div = pre_div; > channel->hi = cnt; > channel->lo = 0; > } else if (duty == 0) { > - channel->pre_div = pre_div; > channel->hi = 0; > channel->lo = cnt; > } else { > - /* Then check is we can have the duty with the same pre_div */ > - duty_cnt = div64_u64(fin_freq * (u64)duty, > - NSEC_PER_SEC * (pre_div + 1)); > + duty_cnt = div64_u64(fin_freq * (u64)duty, NSEC_PER_SEC); > if (duty_cnt > 0xffff) { > dev_err(meson->chip.dev, "unable to get duty cycle\n"); > return -EINVAL; > } > > - dev_dbg(meson->chip.dev, "duty=%u pre_div=%u duty_cnt=%u\n", > - duty, pre_div, duty_cnt); > + dev_dbg(meson->chip.dev, "duty=%u duty_cnt=%u\n", duty, duty_cnt); > > - channel->pre_div = pre_div; > channel->hi = duty_cnt; > channel->lo = cnt - duty_cnt; > } > > + channel->rate = fin_freq; > + > return 0; > } > > @@ -223,16 +208,15 @@ static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm) > struct meson_pwm_channel_data *channel_data; > unsigned long flags; > u32 value; > + int err; > > channel_data = &meson_pwm_per_channel_data[pwm->hwpwm]; > > - spin_lock_irqsave(&meson->lock, flags); > + err = clk_set_rate(channel->clk, channel->rate); > + if (err) > + dev_err(meson->chip.dev, "setting clock rate failed\n"); > > - value = readl(meson->base + REG_MISC_AB); > - value &= ~(MISC_CLK_DIV_MASK << channel_data->clk_div_shift); > - value |= channel->pre_div << channel_data->clk_div_shift; > - value |= BIT(channel_data->clk_en_bit); > - writel(value, meson->base + REG_MISC_AB); > + spin_lock_irqsave(&meson->lock, flags); > > value = FIELD_PREP(PWM_HIGH_MASK, channel->hi) | > FIELD_PREP(PWM_LOW_MASK, channel->lo); > @@ -271,16 +255,16 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > /* > * This IP block revision doesn't have an "always high" > * setting which we can use for "inverted disabled". > - * Instead we achieve this using the same settings > - * that we use a pre_div of 0 (to get the shortest > - * possible duration for one "count") and > + * Instead we achieve this by setting an arbitrary, > + * very high frequency, resulting in the shortest > + * possible duration for one "count" and > * "period == duty_cycle". This results in a signal > * which is LOW for one "count", while being HIGH for > * the rest of the (so the signal is HIGH for slightly > * less than 100% of the period, but this is the best > * we can achieve). > */ > - channel->pre_div = 0; > + channel->rate = 1000000000; > channel->hi = ~0; > channel->lo = 0; This looks like a really bad idea... please don't do that and instead introduce some pinctrl states where we set the PWM pin as GPIO mode high/low state like we did for SPI: https://lore.kernel.org/r/20221004-up-aml-fix-spi-v4-2-0342d8e10c49@baylibre.com > > @@ -305,7 +289,6 @@ static unsigned int meson_pwm_cnt_to_ns(struct pwm_chip *chip, > struct meson_pwm *meson = to_meson_pwm(chip); > struct meson_pwm_channel *channel; > unsigned long fin_freq; > - u32 fin_ns; > > /* to_meson_pwm() can only be used after .get_state() is called */ > channel = &meson->channels[pwm->hwpwm]; > @@ -314,9 +297,7 @@ static unsigned int meson_pwm_cnt_to_ns(struct pwm_chip *chip, > if (fin_freq == 0) > return 0; > > - fin_ns = div_u64(NSEC_PER_SEC, fin_freq); > - > - return cnt * fin_ns * (channel->pre_div + 1); > + return div_u64(NSEC_PER_SEC * (u64)cnt, fin_freq); > } > > static int meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > @@ -325,7 +306,7 @@ static int meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > struct meson_pwm *meson = to_meson_pwm(chip); > struct meson_pwm_channel_data *channel_data; > struct meson_pwm_channel *channel; > - u32 value, tmp; > + u32 value; > > if (!state) > return 0; > @@ -335,11 +316,8 @@ static int meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > > value = readl(meson->base + REG_MISC_AB); > > - tmp = BIT(channel_data->pwm_en_bit) | BIT(channel_data->clk_en_bit); > - state->enabled = (value & tmp) == tmp; > - > - tmp = value >> channel_data->clk_div_shift; > - channel->pre_div = FIELD_GET(MISC_CLK_DIV_MASK, tmp); > + state->enabled = __clk_is_enabled(channel->clk) && > + value & BIT(channel_data->pwm_en_bit); > > value = readl(meson->base + channel_data->reg_offset); > > @@ -480,6 +458,7 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) > > for (i = 0; i < meson->chip.npwm; i++) { > struct meson_pwm_channel *channel = &meson->channels[i]; > + struct clk_parent_data div_parent = {}, gate_parent = {}; > struct clk_init_data init = {}; > > snprintf(name, sizeof(name), "%s#mux%u", dev_name(dev), i); > @@ -499,18 +478,63 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) > channel->mux.table = NULL; > channel->mux.hw.init = &init; > > - channel->clk = devm_clk_register(dev, &channel->mux.hw); > - if (IS_ERR(channel->clk)) { > - err = PTR_ERR(channel->clk); > + err = devm_clk_hw_register(dev, &channel->mux.hw); > + if (err) { > dev_err(dev, "failed to register %s: %d\n", name, err); > return err; > } > > - snprintf(name, sizeof(name), "clkin%u", i); > + snprintf(name, sizeof(name), "%s#div%u", dev_name(dev), i); > + > + init.name = name; > + init.ops = &clk_divider_ops; > + init.flags = CLK_SET_RATE_PARENT; > + div_parent.index = -1; > + div_parent.hw = &channel->mux.hw; > + init.parent_data = &div_parent; > + init.num_parents = 1; > + > + channel->div.reg = meson->base + REG_MISC_AB; > + channel->div.shift = meson_pwm_per_channel_data[i].clk_div_shift; > + channel->div.width = MISC_CLK_DIV_WIDTH; > + channel->div.hw.init = &init; > + channel->div.flags = 0; > + channel->div.lock = &meson->lock; > + > + err = devm_clk_hw_register(dev, &channel->div.hw); > + if (err) { > + dev_err(dev, "failed to register %s: %d\n", name, err); > + return err; > + } > + > + snprintf(name, sizeof(name), "%s#gate%u", dev_name(dev), i); > + > + init.name = name; > + init.ops = &clk_gate_ops; > + init.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED; > + gate_parent.index = -1; > + gate_parent.hw = &channel->div.hw; > + init.parent_data = &gate_parent; > + init.num_parents = 1; > + > + channel->gate.reg = meson->base + REG_MISC_AB; > + channel->gate.bit_idx = meson_pwm_per_channel_data[i].clk_en_bit; > + channel->gate.hw.init = &init; > + channel->gate.flags = 0; > + channel->gate.lock = &meson->lock; > + > + err = devm_clk_hw_register(dev, &channel->gate.hw); > + if (err) { > + dev_err(dev, "failed to register %s: %d\n", name, err); > + return err; > + } > > - channel->clk_parent = devm_clk_get_optional(dev, name); > - if (IS_ERR(channel->clk_parent)) > - return PTR_ERR(channel->clk_parent); > + channel->clk = devm_clk_hw_get_clk(dev, &channel->gate.hw, NULL); > + if (IS_ERR(channel->clk)) { > + err = PTR_ERR(channel->clk); > + dev_err(dev, "failed to register %s: %d\n", name, err); > + return err; > + } > } > > return 0; _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel