From: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
To: Guangjie Song <guangjie.song@mediatek.com>,
Michael Turquette <mturquette@baylibre.com>,
Stephen Boyd <sboyd@kernel.org>, Rob Herring <robh@kernel.org>,
Krzysztof Kozlowski <krzk+dt@kernel.org>,
Conor Dooley <conor+dt@kernel.org>,
Matthias Brugger <matthias.bgg@gmail.com>,
Richard Cochran <richardcochran@gmail.com>
Cc: linux-clk@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-mediatek@lists.infradead.org, netdev@vger.kernel.org,
Project_Global_Chrome_Upstream_Group@mediatek.com
Subject: Re: [PATCH 03/26] clk: mediatek: Support voting for mux
Date: Mon, 10 Mar 2025 15:12:19 +0100 [thread overview]
Message-ID: <cd8bd504-8d91-4420-8053-10ee814417bf@collabora.com> (raw)
In-Reply-To: <20250307032942.10447-4-guangjie.song@mediatek.com>
Il 07/03/25 04:26, Guangjie Song ha scritto:
> Add data fields, defines and ops to support voting for mux.
>
The main thing that is missing here is an answer to an obvious question....
...what are the advantages of hardware voting, and why do we need to use
HW voting instead of the refcount that is already kept by the common clock
framework?
As far as I can see here, the only difference is that the enable/disable
is more complex, losing more time for polling after writes and nothing else?
Is this to synchronize the clock voting between SCP and AP or what?!
If this is the answer, I don't see why we should use this HW voter for all
clocks, since it's simply more expensive (so the clock drivers are wrong as
they enable the voter for all clocks).
> Signed-off-by: Guangjie Song <guangjie.song@mediatek.com>
> ---
> drivers/clk/mediatek/clk-mux.c | 198 ++++++++++++++++++++++++++++++++-
> drivers/clk/mediatek/clk-mux.h | 79 +++++++++++++
> 2 files changed, 275 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c
> index 60990296450b..8a2c89cb3cd5 100644
> --- a/drivers/clk/mediatek/clk-mux.c
> +++ b/drivers/clk/mediatek/clk-mux.c
> @@ -15,11 +15,13 @@
> #include <linux/spinlock.h>
> #include <linux/slab.h>
>
> +#include "clk-mtk.h"
> #include "clk-mux.h"
>
> struct mtk_clk_mux {
> struct clk_hw hw;
> struct regmap *regmap;
> + struct regmap *vote_regmap;
> const struct mtk_mux *data;
> spinlock_t *lock;
> bool reparent;
> @@ -30,6 +32,46 @@ static inline struct mtk_clk_mux *to_mtk_clk_mux(struct clk_hw *hw)
> return container_of(hw, struct mtk_clk_mux, hw);
> }
>
> +static int mtk_clk_mux_fenc_enable_setclr(struct clk_hw *hw)
> +{
> + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> + unsigned long flags = 0;
> + u32 val = 0;
> + int i = 0;
> + int ret = 0;
> +
> + if (mux->lock)
> + spin_lock_irqsave(mux->lock, flags);
> + else
> + __acquire(mux->lock);
> +
> + regmap_write(mux->regmap, mux->data->clr_ofs, BIT(mux->data->gate_shift));
> +
> + while (1) {
> + regmap_read(mux->regmap, mux->data->fenc_sta_mon_ofs, &val);
Why are you reinventing the wheel instead of just using regmap_read_poll_timeout()?
> +
> + if ((val & BIT(mux->data->fenc_shift)) != 0)
> + break;
> +
> + if (i < MTK_WAIT_FENC_DONE_CNT) {
> + udelay(MTK_WAIT_FENC_DONE_US);
> + } else {
> + pr_err("%s wait fenc done timeout\n", clk_hw_get_name(hw));
> + ret = -EBUSY;
> + break;
> + }
> +
> + i++;
> + }
> +
> + if (mux->lock)
> + spin_unlock_irqrestore(mux->lock, flags);
> + else
> + __release(mux->lock);
> +
> + return ret;
> +}
> +
> static int mtk_clk_mux_enable_setclr(struct clk_hw *hw)
> {
> struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> @@ -70,6 +112,16 @@ static void mtk_clk_mux_disable_setclr(struct clk_hw *hw)
> BIT(mux->data->gate_shift));
> }
>
> +static int mtk_clk_mux_fenc_is_enabled(struct clk_hw *hw)
> +{
> + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> + u32 val = 0;
> +
> + regmap_read(mux->regmap, mux->data->fenc_sta_mon_ofs, &val);
> +
> + return (val & BIT(mux->data->fenc_shift)) != 0;
That's just `return val & BIT(mux->data->fenc_shift);` ...
> +}
> +
> static int mtk_clk_mux_is_enabled(struct clk_hw *hw)
> {
> struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> @@ -80,6 +132,106 @@ static int mtk_clk_mux_is_enabled(struct clk_hw *hw)
> return (val & BIT(mux->data->gate_shift)) == 0;
> }
>
> +static int mtk_clk_vote_mux_is_enabled(struct clk_hw *hw)
> +{
> + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> + u32 val = 0;
> +
> + regmap_read(mux->vote_regmap, mux->data->vote_set_ofs, &val);
> +
> + return (val & BIT(mux->data->gate_shift)) != 0;
same
> +}
> +
> +static int mtk_clk_vote_mux_is_done(struct clk_hw *hw)
> +{
> + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> + u32 val = 0;
> +
> + regmap_read(mux->vote_regmap, mux->data->vote_sta_ofs, &val);
> +
> + return (val & BIT(mux->data->gate_shift)) != 0;
ditto
> +}
> +
> +static int mtk_clk_mux_vote_fenc_enable(struct clk_hw *hw)
> +{
> + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> + u32 val = 0, val2 = 0;
> + bool is_done = false;
> + int i = 0;
> +
> + regmap_write(mux->vote_regmap, mux->data->vote_set_ofs, BIT(mux->data->gate_shift));
> +
> + while (!mtk_clk_vote_mux_is_enabled(hw)) {
> + if (i < MTK_WAIT_VOTE_PREPARE_CNT) {
> + udelay(MTK_WAIT_VOTE_PREPARE_US);
regmap_readl_poll_timeout().....
> + } else {
> + pr_err("%s mux prepare timeout(%x)\n", clk_hw_get_name(hw), val);
> + return -EBUSY;
> + }
> +
> + i++;
> + }
> +
> + i = 0;
> +
> + while (1) {
> + if (!is_done)
> + regmap_read(mux->vote_regmap, mux->data->vote_sta_ofs, &val);
> +
> + if (((val & BIT(mux->data->gate_shift)) != 0))
> + is_done = true;
> +
and again - twice.
> + if (is_done) {
> + regmap_read(mux->regmap, mux->data->fenc_sta_mon_ofs, &val2);
> + if ((val2 & BIT(mux->data->fenc_shift)) != 0)
> + break;
> + }
> +
> + if (i < MTK_WAIT_VOTE_DONE_CNT) {
> + udelay(MTK_WAIT_VOTE_DONE_US);
> + } else {
> + pr_err("%s mux enable timeout(%x %x)\n", clk_hw_get_name(hw), val, val2);
> + return -EBUSY;
> + }
> +
> + i++;
> + }
> +
> + return 0;
> +}
> +
> +static void mtk_clk_mux_vote_disable(struct clk_hw *hw)
> +{
> + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> + int i = 0;
> +
> + regmap_write(mux->vote_regmap, mux->data->vote_clr_ofs, BIT(mux->data->gate_shift));
> +
> + while (mtk_clk_vote_mux_is_enabled(hw)) {
> + if (i < MTK_WAIT_VOTE_PREPARE_CNT) {
> + udelay(MTK_WAIT_VOTE_PREPARE_US);
> + } else {
> + pr_err("%s mux unprepare timeout\n", clk_hw_get_name(hw));
> + return;
> + }
> +
....and again....
> + i++;
> + }
> +
> + i = 0;
> +
> + while (!mtk_clk_vote_mux_is_done(hw)) {
> + if (i < MTK_WAIT_VOTE_DONE_CNT) {
> + udelay(MTK_WAIT_VOTE_DONE_US);
> + } else {
> + pr_err("%s mux disable timeout\n", clk_hw_get_name(hw));
> + return;
> + }
> +
> + i++;
> + }
> +}
> +
> static u8 mtk_clk_mux_get_parent(struct clk_hw *hw)
> {
> struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> @@ -151,6 +303,12 @@ static int mtk_clk_mux_determine_rate(struct clk_hw *hw,
> return clk_mux_determine_rate_flags(hw, req, mux->data->flags);
> }
>
> +static void mtk_clk_mux_vote_fenc_disable_unused(struct clk_hw *hw)
> +{
> + mtk_clk_mux_vote_fenc_enable(hw);
> + mtk_clk_mux_vote_disable(hw);
Why would you need to enable and disable?
If this is not a mistake... this definitely needs a comment in the code.
> +}
> +
> const struct clk_ops mtk_mux_clr_set_upd_ops = {
> .get_parent = mtk_clk_mux_get_parent,
> .set_parent = mtk_clk_mux_set_parent_setclr_lock,
> @@ -168,9 +326,31 @@ const struct clk_ops mtk_mux_gate_clr_set_upd_ops = {
> };
> EXPORT_SYMBOL_GPL(mtk_mux_gate_clr_set_upd_ops);
>
> +const struct clk_ops mtk_mux_gate_fenc_clr_set_upd_ops = {
> + .enable = mtk_clk_mux_fenc_enable_setclr,
> + .disable = mtk_clk_mux_disable_setclr,
> + .is_enabled = mtk_clk_mux_fenc_is_enabled,
> + .get_parent = mtk_clk_mux_get_parent,
> + .set_parent = mtk_clk_mux_set_parent_setclr_lock,
> + .determine_rate = mtk_clk_mux_determine_rate,
> +};
> +EXPORT_SYMBOL_GPL(mtk_mux_gate_fenc_clr_set_upd_ops);
> +
> +const struct clk_ops mtk_mux_vote_fenc_ops = {
> + .enable = mtk_clk_mux_vote_fenc_enable,
> + .disable = mtk_clk_mux_vote_disable,
> + .is_enabled = mtk_clk_mux_fenc_is_enabled,
> + .get_parent = mtk_clk_mux_get_parent,
> + .set_parent = mtk_clk_mux_set_parent_setclr_lock,
> + .determine_rate = mtk_clk_mux_determine_rate,
> + .disable_unused = mtk_clk_mux_vote_fenc_disable_unused,
> +};
> +EXPORT_SYMBOL_GPL(mtk_mux_vote_fenc_ops);
> +
> static struct clk_hw *mtk_clk_register_mux(struct device *dev,
> const struct mtk_mux *mux,
> struct regmap *regmap,
> + struct regmap *vote_regmap,
> spinlock_t *lock)
> {
> struct mtk_clk_mux *clk_mux;
> @@ -185,9 +365,17 @@ static struct clk_hw *mtk_clk_register_mux(struct device *dev,
> init.flags = mux->flags;
> init.parent_names = mux->parent_names;
> init.num_parents = mux->num_parents;
> - init.ops = mux->ops;
> + if (mux->flags & CLK_USE_VOTE) {
> + if (vote_regmap)
> + init.ops = mux->ops;
> + else
> + init.ops = mux->dma_ops;
Sorry why is this called dma_ops?!
That's at least confusing, if not simply wrong.... please explain.
> + } else {
> + init.ops = mux->ops;
> + }
>
> clk_mux->regmap = regmap;
> + clk_mux->vote_regmap = vote_regmap;
> clk_mux->data = mux;
> clk_mux->lock = lock;
> clk_mux->hw.init = &init;
> @@ -220,6 +408,7 @@ int mtk_clk_register_muxes(struct device *dev,
> struct clk_hw_onecell_data *clk_data)
> {
> struct regmap *regmap;
> + struct regmap *vote_regmap = NULL;
> struct clk_hw *hw;
> int i;
>
> @@ -238,8 +427,13 @@ int mtk_clk_register_muxes(struct device *dev,
> continue;
> }
>
> - hw = mtk_clk_register_mux(dev, mux, regmap, lock);
> + if (mux->vote_comp) {
> + vote_regmap = syscon_regmap_lookup_by_phandle(node, mux->vote_comp);
> + if (IS_ERR(vote_regmap))
> + vote_regmap = NULL;
> + }
>
> + hw = mtk_clk_register_mux(dev, mux, regmap, vote_regmap, lock);
...and this change just breaks each and every MediaTek SoC that is currently
supported upstream.
Please test your changes on older platforms before submitting upstream.
Regards,
Angelo
next prev parent reply other threads:[~2025-03-10 14:12 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-03-07 3:26 [PATCH 00/26] clk: mediatek: Add MT8196 clock support Guangjie Song
2025-03-07 3:26 ` [PATCH 01/26] clk: mediatek: Add defines for vote Guangjie Song
2025-03-07 3:26 ` [PATCH 02/26] clk: mediatek: Support voting for pll Guangjie Song
2025-03-07 3:26 ` [PATCH 03/26] clk: mediatek: Support voting for mux Guangjie Song
2025-03-10 14:12 ` AngeloGioacchino Del Regno [this message]
2025-03-07 3:27 ` [PATCH 04/26] clk: mediatek: Support voting for gate Guangjie Song
2025-03-07 3:27 ` [PATCH 05/26] clk: mediatek: Add gate ops without disable Guangjie Song
2025-03-07 3:27 ` [PATCH 06/26] dt-bindings: clock: mediatek: Add new MT8196 clock Guangjie Song
2025-03-07 4:16 ` Rob Herring (Arm)
2025-03-07 7:29 ` Krzysztof Kozlowski
2025-04-16 9:20 ` Chen-Yu Tsai
2025-03-07 3:27 ` [PATCH 07/26] clk: mediatek: Add MT8196 apmixedsys clock support Guangjie Song
2025-03-11 17:05 ` Jeff Johnson
2025-03-07 3:27 ` [PATCH 08/26] clk: mediatek: Add MT8196 apmixedsys_gp2 " Guangjie Song
2025-03-07 3:27 ` [PATCH 09/26] clk: mediatek: Add MT8196 topckgen " Guangjie Song
2025-04-16 9:04 ` Chen-Yu Tsai
2025-03-07 3:27 ` [PATCH 10/26] clk: mediatek: Add MT8196 topckgen2 " Guangjie Song
2025-03-07 3:27 ` [PATCH 11/26] clk: mediatek: Add MT8196 vlpckgen " Guangjie Song
2025-03-07 3:27 ` [PATCH 12/26] clk: mediatek: Add MT8196 peripheral " Guangjie Song
2025-03-07 3:27 ` [PATCH 13/26] clk: mediatek: Add MT8196 adsp " Guangjie Song
2025-03-07 3:27 ` [PATCH 14/26] clk: mediatek: Add MT8196 i2c " Guangjie Song
2025-03-07 3:27 ` [PATCH 15/26] clk: mediatek: Add MT8196 mcu " Guangjie Song
2025-03-07 3:27 ` [PATCH 16/26] clk: mediatek: Add MT8196 mdpsys " Guangjie Song
2025-03-07 3:27 ` [PATCH 17/26] clk: mediatek: Add MT8196 mfg " Guangjie Song
2025-03-07 3:27 ` [PATCH 18/26] clk: mediatek: Add MT8196 disp0 " Guangjie Song
2025-03-07 3:27 ` [PATCH 19/26] clk: mediatek: Add MT8196 disp1 " Guangjie Song
2025-03-07 3:27 ` [PATCH 20/26] clk: mediatek: Add MT8196 disp-ao " Guangjie Song
2025-03-07 3:27 ` [PATCH 21/26] clk: mediatek: Add MT8196 ovl0 " Guangjie Song
2025-03-07 7:29 ` Krzysztof Kozlowski
2025-03-07 3:27 ` [PATCH 22/26] clk: mediatek: Add MT8196 ovl1 " Guangjie Song
2025-03-07 3:27 ` [PATCH 23/26] clk: mediatek: Add MT8196 pextpsys " Guangjie Song
2025-03-07 3:27 ` [PATCH 24/26] clk: mediatek: Add MT8196 ufssys " Guangjie Song
2025-03-07 3:27 ` [PATCH 25/26] clk: mediatek: Add MT8196 vdecsys " Guangjie Song
2025-03-07 3:27 ` [PATCH 26/26] clk: mediatek: Add MT8196 vencsys " Guangjie Song
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=cd8bd504-8d91-4420-8053-10ee814417bf@collabora.com \
--to=angelogioacchino.delregno@collabora.com \
--cc=Project_Global_Chrome_Upstream_Group@mediatek.com \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=guangjie.song@mediatek.com \
--cc=krzk+dt@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=matthias.bgg@gmail.com \
--cc=mturquette@baylibre.com \
--cc=netdev@vger.kernel.org \
--cc=richardcochran@gmail.com \
--cc=robh@kernel.org \
--cc=sboyd@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
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