From: Brian Masney <bmasney@redhat•com>
To: Yu-Chun Lin <eleanor.lin@realtek•com>
Cc: mturquette@baylibre•com, sboyd@kernel•org, robh@kernel•org,
krzk+dt@kernel•org, conor+dt@kernel•org, p.zabel@pengutronix•de,
cylee12@realtek•com, afaerber@suse•com, jyanchou@realtek•com,
devicetree@vger•kernel.org, linux-clk@vger•kernel.org,
linux-kernel@vger•kernel.org,
linux-arm-kernel@lists•infradead.org,
linux-realtek-soc@lists•infradead.org, james.tai@realtek•com,
cy.huang@realtek•com, stanley_chang@realtek•com
Subject: Re: [PATCH v6 07/10] clk: realtek: Add support for MMC-tuned PLL clocks
Date: Fri, 3 Apr 2026 11:10:52 -0400 [thread overview]
Message-ID: <ac_YfHe0dscb3MPw@redhat.com> (raw)
In-Reply-To: <20260402073957.2742459-8-eleanor.lin@realtek.com>
Hi Yu-Chun,
I should have finished going through Sashiko while manually reviewing
your patches.
On Thu, Apr 02, 2026 at 03:39:54PM +0800, Yu-Chun Lin wrote:
> From: Cheng-Yu Lee <cylee12@realtek•com>
>
> Add clk_pll_mmc_ops for enable/disable, prepare, rate control, and status
> operations on MMC PLL clocks.
>
> Also add clk_pll_mmc_phase_ops to support phase get/set operations.
>
> Signed-off-by: Cheng-Yu Lee <cylee12@realtek•com>
> Co-developed-by: Jyan Chou <jyanchou@realtek•com>
> Signed-off-by: Jyan Chou <jyanchou@realtek•com>
> Co-developed-by: Yu-Chun Lin <eleanor.lin@realtek•com>
> Signed-off-by: Yu-Chun Lin <eleanor.lin@realtek•com>
> ---
> +static int clk_pll_mmc_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate)
> +{
> + struct clk_pll_mmc *clkm = to_clk_pll_mmc(hw);
> + u32 val = PLL_MMC_SSC_DIV_N_VAL;
> + int ret;
> +
> + ret = regmap_update_bits(clkm->clkr.regmap,
> + clkm->ssc_dig_ofs + PLL_SSC_DIG_EMMC1_OFFSET,
> + PLL_FLAG_INITAL_EMMC_MASK, 0x0 << PLL_FLAG_INITAL_EMMC_SHIFT);
> + if (ret)
> + return ret;
> +
> + ret = set_ssc_div_n(clkm, val);
> + if (ret)
> + return ret;
> +
> + ret = set_ssc_div_ext_f(clkm, 1517);
> + if (ret)
> + return ret;
> +
> + switch (val) {
> + case 31 ... 46:
> + ret |= set_pi_ibselh(clkm, 3);
> + ret |= set_sscpll_rs(clkm, 3);
> + ret |= set_sscpll_icp(clkm, 2);
Sashiko reports:
https://sashiko.dev/#/patchset/20260402073957.2742459-1-eleanor.lin%40realtek.com
Is it intended to use bitwise OR to accumulate these return values? Because
these hardware operations return standard negative error codes on failure,
performing a bitwise OR on multiple negative integers will merge their bit
patterns and create a corrupted error code.
> + break;
> +
> + case 20 ... 30:
> + ret |= set_pi_ibselh(clkm, 2);
> + ret |= set_sscpll_rs(clkm, 3);
> + ret |= set_sscpll_icp(clkm, 1);
> + break;
> +
> + case 10 ... 19:
> + ret |= set_pi_ibselh(clkm, 1);
> + ret |= set_sscpll_rs(clkm, 2);
> + ret |= set_sscpll_icp(clkm, 1);
> + break;
> +
> + case 5 ... 9:
> + ret |= set_pi_ibselh(clkm, 0);
> + ret |= set_sscpll_rs(clkm, 2);
> + ret |= set_sscpll_icp(clkm, 0);
> + break;
> + }
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(clkm->clkr.regmap,
> + clkm->ssc_dig_ofs + PLL_SSC_DIG_EMMC3_OFFSET,
> + PLL_NCODE_SSC_EMMC_MASK,
> + 27 << PLL_NCODE_SSC_EMMC_SHIFT);
Sashiko reports:
https://sashiko.dev/#/patchset/20260402073957.2742459-1-eleanor.lin%40realtek.com
Are the mask and shift values mismatched here? PLL_FLAG_INITAL_EMMC_MASK is
defined as BIT(1) (0x02), but PLL_FLAG_INITAL_EMMC_SHIFT is 8.
When regmap_update_bits() applies the 0x02 mask to a value shifted by 8,
won't it evaluate to 0 and fail to set the intended initialization flag?
Brian
next prev parent reply other threads:[~2026-04-03 15:11 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-04-02 7:39 [PATCH v6 00/10] clk: realtek: Add RTD1625 clock support Yu-Chun Lin
2026-04-02 7:39 ` [PATCH v6 01/10] dt-bindings: clock: Add Realtek RTD1625 Clock & Reset Controller Yu-Chun Lin
2026-04-02 7:39 ` [PATCH v6 02/10] reset: Add Realtek basic reset support Yu-Chun Lin
2026-04-02 9:15 ` Philipp Zabel
2026-04-10 6:49 ` Yu-Chun Lin [林祐君]
2026-04-02 7:39 ` [PATCH v6 03/10] clk: realtek: Introduce a common probe() Yu-Chun Lin
2026-04-03 14:21 ` Brian Masney
2026-04-10 7:22 ` Yu-Chun Lin [林祐君]
2026-04-02 7:39 ` [PATCH v6 04/10] clk: realtek: Add support for phase locked loops (PLLs) Yu-Chun Lin
2026-04-03 14:34 ` Brian Masney
2026-04-10 7:43 ` Yu-Chun Lin [林祐君]
2026-04-03 14:44 ` Brian Masney
2026-04-10 7:53 ` Yu-Chun Lin
2026-04-02 7:39 ` [PATCH v6 05/10] clk: realtek: Add support for gate clock Yu-Chun Lin
2026-04-03 14:40 ` Brian Masney
2026-04-10 8:19 ` Yu-Chun Lin
2026-04-02 7:39 ` [PATCH v6 06/10] clk: realtek: Add support for mux clock Yu-Chun Lin
2026-04-03 14:54 ` Brian Masney
2026-04-10 8:24 ` Yu-Chun Lin [林祐君]
2026-04-02 7:39 ` [PATCH v6 07/10] clk: realtek: Add support for MMC-tuned PLL clocks Yu-Chun Lin
2026-04-03 15:07 ` Brian Masney
2026-04-17 7:40 ` Yu-Chun Lin
2026-04-03 15:10 ` Brian Masney [this message]
2026-04-17 7:43 ` Yu-Chun Lin
2026-04-02 7:39 ` [PATCH v6 08/10] clk: realtek: Add RTD1625-CRT clock controller driver Yu-Chun Lin
2026-04-03 15:24 ` Brian Masney
2026-04-17 7:45 ` Yu-Chun Lin
2026-04-02 7:39 ` [PATCH v6 09/10] clk: realtek: Add RTD1625-ISO " Yu-Chun Lin
2026-04-03 15:29 ` Brian Masney
2026-04-17 8:09 ` Yu-Chun Lin
2026-04-02 7:39 ` [PATCH v6 10/10] arm64: dts: realtek: Add clock support for RTD1625 Yu-Chun Lin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ac_YfHe0dscb3MPw@redhat.com \
--to=bmasney@redhat$(echo .)com \
--cc=afaerber@suse$(echo .)com \
--cc=conor+dt@kernel$(echo .)org \
--cc=cy.huang@realtek$(echo .)com \
--cc=cylee12@realtek$(echo .)com \
--cc=devicetree@vger$(echo .)kernel.org \
--cc=eleanor.lin@realtek$(echo .)com \
--cc=james.tai@realtek$(echo .)com \
--cc=jyanchou@realtek$(echo .)com \
--cc=krzk+dt@kernel$(echo .)org \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
--cc=linux-clk@vger$(echo .)kernel.org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linux-realtek-soc@lists$(echo .)infradead.org \
--cc=mturquette@baylibre$(echo .)com \
--cc=p.zabel@pengutronix$(echo .)de \
--cc=robh@kernel$(echo .)org \
--cc=sboyd@kernel$(echo .)org \
--cc=stanley_chang@realtek$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox