[PATCH v2 0/4] clk: mediatek: mt8183: Fix GPU/MFG clock rate changing

[PATCH v2 0/4] clk: mediatek: mt8183: Fix GPU/MFG clock rate changing

[Chen-Yu Tsai]

Hi everyone,

This is v2 of my MT8183 GPU clock rate fix series.

Changes since v1;
- Moved clk notifier registration into separate function
- Fixed comment style

This series fixes the clock rate changing for the GPU. This work came
about as part of adding DVFS support for the Mali GPU on MT8183, to
support efforts in testing the SVS patches [1] on MT8183.

This series fixes a couple things:

1. Fix the clock reference for the GPU. The device tree incorrectly
   references the top level PLL, when in fact it is fed from the clock
   gate in the MFGCFG block. Fixed in patch 1.

2. Clock rate requests on the MFG clock gate aren't propagated up the
   tree. Fixed in patch 2 by adding CLK_SET_RATE_PARENT.

3. MFG clock needs to be temporarily muxed away from MFG PLL during PLL
   reconfiguration, to avoid glitches. This is done using a notifier.
   The framework is added in patch 3, and added to the driver in patch 4.

This is based on my "clk: mediatek: Move to struct clk_hw provider APIs"
series version 3 [2], which was just merged.

Please have a look.

The GPU DVFS stuff will be sent separately, as that part is a bit more
contentious, and the changes span more subsystems.


Regards
ChenYu

[1] https://lore.kernel.org/linux-mediatek/20220516004311.18358-1-roger.lu@mediatek.com/
[2] https://lore.kernel.org/linux-mediatek/20220519071610.423372-1-wenst@chromium.org/

Chen-Yu Tsai (4):
  arm64: dts: mt8183: Fix Mali GPU clock
  clk: mediatek: mt8183: mfgcfg: Propagate rate changes to parent
  clk: mediatek: mux: add clk notifier functions
  clk: mediatek: mt8183: Add clk mux notifier for MFG mux

 arch/arm64/boot/dts/mediatek/mt8183.dtsi |  2 +-
 drivers/clk/mediatek/clk-mt8183-mfgcfg.c |  6 ++--
 drivers/clk/mediatek/clk-mt8183.c        | 28 ++++++++++++++++
 drivers/clk/mediatek/clk-mux.c           | 42 ++++++++++++++++++++++++
 drivers/clk/mediatek/clk-mux.h           | 15 +++++++++
 5 files changed, 89 insertions(+), 4 deletions(-)

-- 
2.36.1.124.g0e6072fb45-goog

[PATCH v2 0/4] clk: mediatek: mt8183: Fix GPU/MFG clock rate changing

[Chen-Yu Tsai]

Hi everyone,

This is v2 of my MT8183 GPU clock rate fix series.

Changes since v1;
- Moved clk notifier registration into separate function
- Fixed comment style

This series fixes the clock rate changing for the GPU. This work came
about as part of adding DVFS support for the Mali GPU on MT8183, to
support efforts in testing the SVS patches [1] on MT8183.

This series fixes a couple things:

1. Fix the clock reference for the GPU. The device tree incorrectly
   references the top level PLL, when in fact it is fed from the clock
   gate in the MFGCFG block. Fixed in patch 1.

2. Clock rate requests on the MFG clock gate aren't propagated up the
   tree. Fixed in patch 2 by adding CLK_SET_RATE_PARENT.

3. MFG clock needs to be temporarily muxed away from MFG PLL during PLL
   reconfiguration, to avoid glitches. This is done using a notifier.
   The framework is added in patch 3, and added to the driver in patch 4.

This is based on my "clk: mediatek: Move to struct clk_hw provider APIs"
series version 3 [2], which was just merged.

Please have a look.

The GPU DVFS stuff will be sent separately, as that part is a bit more
contentious, and the changes span more subsystems.


Regards
ChenYu

[1] https://lore.kernel.org/linux-mediatek/20220516004311.18358-1-roger.lu@mediatek.com/
[2] https://lore.kernel.org/linux-mediatek/20220519071610.423372-1-wenst@chromium.org/

Chen-Yu Tsai (4):
  arm64: dts: mt8183: Fix Mali GPU clock
  clk: mediatek: mt8183: mfgcfg: Propagate rate changes to parent
  clk: mediatek: mux: add clk notifier functions
  clk: mediatek: mt8183: Add clk mux notifier for MFG mux

 arch/arm64/boot/dts/mediatek/mt8183.dtsi |  2 +-
 drivers/clk/mediatek/clk-mt8183-mfgcfg.c |  6 ++--
 drivers/clk/mediatek/clk-mt8183.c        | 28 ++++++++++++++++
 drivers/clk/mediatek/clk-mux.c           | 42 ++++++++++++++++++++++++
 drivers/clk/mediatek/clk-mux.h           | 15 +++++++++
 5 files changed, 89 insertions(+), 4 deletions(-)

-- 
2.36.1.124.g0e6072fb45-goog


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH v2 0/4] clk: mediatek: mt8183: Fix GPU/MFG clock rate changing

[Chen-Yu Tsai]

Hi everyone,

This is v2 of my MT8183 GPU clock rate fix series.

Changes since v1;
- Moved clk notifier registration into separate function
- Fixed comment style

This series fixes the clock rate changing for the GPU. This work came
about as part of adding DVFS support for the Mali GPU on MT8183, to
support efforts in testing the SVS patches [1] on MT8183.

This series fixes a couple things:

1. Fix the clock reference for the GPU. The device tree incorrectly
   references the top level PLL, when in fact it is fed from the clock
   gate in the MFGCFG block. Fixed in patch 1.

2. Clock rate requests on the MFG clock gate aren't propagated up the
   tree. Fixed in patch 2 by adding CLK_SET_RATE_PARENT.

3. MFG clock needs to be temporarily muxed away from MFG PLL during PLL
   reconfiguration, to avoid glitches. This is done using a notifier.
   The framework is added in patch 3, and added to the driver in patch 4.

This is based on my "clk: mediatek: Move to struct clk_hw provider APIs"
series version 3 [2], which was just merged.

Please have a look.

The GPU DVFS stuff will be sent separately, as that part is a bit more
contentious, and the changes span more subsystems.


Regards
ChenYu

[1] https://lore.kernel.org/linux-mediatek/20220516004311.18358-1-roger.lu@mediatek.com/
[2] https://lore.kernel.org/linux-mediatek/20220519071610.423372-1-wenst@chromium.org/

Chen-Yu Tsai (4):
  arm64: dts: mt8183: Fix Mali GPU clock
  clk: mediatek: mt8183: mfgcfg: Propagate rate changes to parent
  clk: mediatek: mux: add clk notifier functions
  clk: mediatek: mt8183: Add clk mux notifier for MFG mux

 arch/arm64/boot/dts/mediatek/mt8183.dtsi |  2 +-
 drivers/clk/mediatek/clk-mt8183-mfgcfg.c |  6 ++--
 drivers/clk/mediatek/clk-mt8183.c        | 28 ++++++++++++++++
 drivers/clk/mediatek/clk-mux.c           | 42 ++++++++++++++++++++++++
 drivers/clk/mediatek/clk-mux.h           | 15 +++++++++
 5 files changed, 89 insertions(+), 4 deletions(-)

-- 
2.36.1.124.g0e6072fb45-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/4] arm64: dts: mt8183: Fix Mali GPU clock

[Chen-Yu Tsai]

The actual clock feeding into the Mali GPU on the MT8183 is from the
clock gate in the MFGCFG block, not CLK_TOP_MFGPLL_CK from the TOPCKGEN
block, which itself is simply a pass-through placeholder for the MFGPLL
in the APMIXEDSYS block.

Fix the hardware description with the correct clock reference.

Fixes: a8168cebf1bc ("arm64: dts: mt8183: Add node for the Mali GPU")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 arch/arm64/boot/dts/mediatek/mt8183.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
index 01e650251928..6ced76a60aab 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
@@ -1368,7 +1368,7 @@ gpu: gpu@13040000 {
 				<GIC_SPI 278 IRQ_TYPE_LEVEL_LOW>;
 			interrupt-names = "job", "mmu", "gpu";
 
-			clocks = <&topckgen CLK_TOP_MFGPLL_CK>;
+			clocks = <&mfgcfg CLK_MFG_BG3D>;
 
 			power-domains =
 				<&spm MT8183_POWER_DOMAIN_MFG_CORE0>,
-- 
2.36.1.124.g0e6072fb45-goog

[PATCH v2 1/4] arm64: dts: mt8183: Fix Mali GPU clock

[Chen-Yu Tsai]

The actual clock feeding into the Mali GPU on the MT8183 is from the
clock gate in the MFGCFG block, not CLK_TOP_MFGPLL_CK from the TOPCKGEN
block, which itself is simply a pass-through placeholder for the MFGPLL
in the APMIXEDSYS block.

Fix the hardware description with the correct clock reference.

Fixes: a8168cebf1bc ("arm64: dts: mt8183: Add node for the Mali GPU")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 arch/arm64/boot/dts/mediatek/mt8183.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
index 01e650251928..6ced76a60aab 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
@@ -1368,7 +1368,7 @@ gpu: gpu@13040000 {
 				<GIC_SPI 278 IRQ_TYPE_LEVEL_LOW>;
 			interrupt-names = "job", "mmu", "gpu";
 
-			clocks = <&topckgen CLK_TOP_MFGPLL_CK>;
+			clocks = <&mfgcfg CLK_MFG_BG3D>;
 
 			power-domains =
 				<&spm MT8183_POWER_DOMAIN_MFG_CORE0>,
-- 
2.36.1.124.g0e6072fb45-goog


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH v2 1/4] arm64: dts: mt8183: Fix Mali GPU clock

[Chen-Yu Tsai]

The actual clock feeding into the Mali GPU on the MT8183 is from the
clock gate in the MFGCFG block, not CLK_TOP_MFGPLL_CK from the TOPCKGEN
block, which itself is simply a pass-through placeholder for the MFGPLL
in the APMIXEDSYS block.

Fix the hardware description with the correct clock reference.

Fixes: a8168cebf1bc ("arm64: dts: mt8183: Add node for the Mali GPU")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 arch/arm64/boot/dts/mediatek/mt8183.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
index 01e650251928..6ced76a60aab 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
@@ -1368,7 +1368,7 @@ gpu: gpu@13040000 {
 				<GIC_SPI 278 IRQ_TYPE_LEVEL_LOW>;
 			interrupt-names = "job", "mmu", "gpu";
 
-			clocks = <&topckgen CLK_TOP_MFGPLL_CK>;
+			clocks = <&mfgcfg CLK_MFG_BG3D>;
 
 			power-domains =
 				<&spm MT8183_POWER_DOMAIN_MFG_CORE0>,
-- 
2.36.1.124.g0e6072fb45-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/4] clk: mediatek: mt8183: mfgcfg: Propagate rate changes to parent

[Chen-Yu Tsai]

The only clock in the MT8183 MFGCFG block feeds the GPU. Propagate its
rate change requests to its parent, so that DVFS for the GPU can work
properly.

Fixes: acddfc2c261b ("clk: mediatek: Add MT8183 clock support")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/clk/mediatek/clk-mt8183-mfgcfg.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt8183-mfgcfg.c b/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
index d774edaf760b..230299728859 100644
--- a/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
+++ b/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
@@ -18,9 +18,9 @@ static const struct mtk_gate_regs mfg_cg_regs = {
 	.sta_ofs = 0x0,
 };
 
-#define GATE_MFG(_id, _name, _parent, _shift)			\
-	GATE_MTK(_id, _name, _parent, &mfg_cg_regs, _shift,	\
-		&mtk_clk_gate_ops_setclr)
+#define GATE_MFG(_id, _name, _parent, _shift)				\
+	GATE_MTK_FLAGS(_id, _name, _parent, &mfg_cg_regs, _shift,	\
+		       &mtk_clk_gate_ops_setclr, CLK_SET_RATE_PARENT)
 
 static const struct mtk_gate mfg_clks[] = {
 	GATE_MFG(CLK_MFG_BG3D, "mfg_bg3d", "mfg_sel", 0)
-- 
2.36.1.124.g0e6072fb45-goog

[PATCH v2 2/4] clk: mediatek: mt8183: mfgcfg: Propagate rate changes to parent

[Chen-Yu Tsai]

The only clock in the MT8183 MFGCFG block feeds the GPU. Propagate its
rate change requests to its parent, so that DVFS for the GPU can work
properly.

Fixes: acddfc2c261b ("clk: mediatek: Add MT8183 clock support")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/clk/mediatek/clk-mt8183-mfgcfg.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt8183-mfgcfg.c b/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
index d774edaf760b..230299728859 100644
--- a/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
+++ b/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
@@ -18,9 +18,9 @@ static const struct mtk_gate_regs mfg_cg_regs = {
 	.sta_ofs = 0x0,
 };
 
-#define GATE_MFG(_id, _name, _parent, _shift)			\
-	GATE_MTK(_id, _name, _parent, &mfg_cg_regs, _shift,	\
-		&mtk_clk_gate_ops_setclr)
+#define GATE_MFG(_id, _name, _parent, _shift)				\
+	GATE_MTK_FLAGS(_id, _name, _parent, &mfg_cg_regs, _shift,	\
+		       &mtk_clk_gate_ops_setclr, CLK_SET_RATE_PARENT)
 
 static const struct mtk_gate mfg_clks[] = {
 	GATE_MFG(CLK_MFG_BG3D, "mfg_bg3d", "mfg_sel", 0)
-- 
2.36.1.124.g0e6072fb45-goog


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH v2 2/4] clk: mediatek: mt8183: mfgcfg: Propagate rate changes to parent

[Chen-Yu Tsai]

The only clock in the MT8183 MFGCFG block feeds the GPU. Propagate its
rate change requests to its parent, so that DVFS for the GPU can work
properly.

Fixes: acddfc2c261b ("clk: mediatek: Add MT8183 clock support")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/clk/mediatek/clk-mt8183-mfgcfg.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt8183-mfgcfg.c b/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
index d774edaf760b..230299728859 100644
--- a/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
+++ b/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
@@ -18,9 +18,9 @@ static const struct mtk_gate_regs mfg_cg_regs = {
 	.sta_ofs = 0x0,
 };
 
-#define GATE_MFG(_id, _name, _parent, _shift)			\
-	GATE_MTK(_id, _name, _parent, &mfg_cg_regs, _shift,	\
-		&mtk_clk_gate_ops_setclr)
+#define GATE_MFG(_id, _name, _parent, _shift)				\
+	GATE_MTK_FLAGS(_id, _name, _parent, &mfg_cg_regs, _shift,	\
+		       &mtk_clk_gate_ops_setclr, CLK_SET_RATE_PARENT)
 
 static const struct mtk_gate mfg_clks[] = {
 	GATE_MFG(CLK_MFG_BG3D, "mfg_bg3d", "mfg_sel", 0)
-- 
2.36.1.124.g0e6072fb45-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/4] clk: mediatek: mux: add clk notifier functions

[Chen-Yu Tsai]

With device frequency scaling, the mux clock that (indirectly) feeds the
device selects between a dedicated PLL, and some other stable clocks.

When a clk rate change is requested, the (normally) upstream PLL is
reconfigured. It's possible for the clock output of the PLL to become
unstable during this process.

To avoid causing the device to glitch, the mux should temporarily be
switched over to another "stable" clock during the PLL rate change.
This is done with clk notifiers.

This patch adds common functions for notifiers to temporarily and
transparently reparent mux clocks.

This was loosely based on commit 8adfb08605a9 ("clk: sunxi-ng: mux: Add
clk notifier functions").

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/clk/mediatek/clk-mux.c | 42 ++++++++++++++++++++++++++++++++++
 drivers/clk/mediatek/clk-mux.h | 15 ++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c
index cd5f9fd8cb98..f84a5a753c09 100644
--- a/drivers/clk/mediatek/clk-mux.c
+++ b/drivers/clk/mediatek/clk-mux.c
@@ -4,6 +4,7 @@
  * Author: Owen Chen <owen.chen@mediatek.com>
  */
 
+#include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/compiler_types.h>
 #include <linux/container_of.h>
@@ -259,4 +260,45 @@ void mtk_clk_unregister_muxes(const struct mtk_mux *muxes, int num,
 }
 EXPORT_SYMBOL_GPL(mtk_clk_unregister_muxes);
 
+/*
+ * This clock notifier is called when the frequency of the of the parent
+ * PLL clock is to be changed. The idea is to switch the parent to a
+ * stable clock, such as the main oscillator, while the PLL frequency
+ * stabilizes.
+ */
+static int mtk_clk_mux_notifier_cb(struct notifier_block *nb,
+				   unsigned long event, void *_data)
+{
+	struct clk_notifier_data *data = _data;
+	struct mtk_mux_nb *mux_nb = to_mtk_mux_nb(nb);
+	const struct mtk_mux *mux = mux_nb->mux;
+	struct clk_hw *hw;
+	int ret = 0;
+
+	hw = __clk_get_hw(data->clk);
+
+	switch (event) {
+	case PRE_RATE_CHANGE:
+		mux_nb->original_index = mux->ops->get_parent(hw);
+		ret = mux->ops->set_parent(hw, mux_nb->bypass_index);
+		break;
+
+	case POST_RATE_CHANGE:
+	case ABORT_RATE_CHANGE:
+		ret = mux->ops->set_parent(hw, mux_nb->original_index);
+		break;
+	}
+
+	return notifier_from_errno(ret);
+}
+
+int devm_mtk_clk_mux_notifier_register(struct device *dev, struct clk *clk,
+				       struct mtk_mux_nb *mux_nb)
+{
+	mux_nb->nb.notifier_call = mtk_clk_mux_notifier_cb;
+
+	return devm_clk_notifier_register(dev, clk, &mux_nb->nb);
+}
+EXPORT_SYMBOL_GPL(devm_mtk_clk_mux_notifier_register);
+
 MODULE_LICENSE("GPL");
diff --git a/drivers/clk/mediatek/clk-mux.h b/drivers/clk/mediatek/clk-mux.h
index 6539c58f5d7d..506e91125a3d 100644
--- a/drivers/clk/mediatek/clk-mux.h
+++ b/drivers/clk/mediatek/clk-mux.h
@@ -7,12 +7,14 @@
 #ifndef __DRV_CLK_MTK_MUX_H
 #define __DRV_CLK_MTK_MUX_H
 
+#include <linux/notifier.h>
 #include <linux/spinlock.h>
 #include <linux/types.h>
 
 struct clk;
 struct clk_hw_onecell_data;
 struct clk_ops;
+struct device;
 struct device_node;
 
 struct mtk_mux {
@@ -89,4 +91,17 @@ int mtk_clk_register_muxes(const struct mtk_mux *muxes,
 void mtk_clk_unregister_muxes(const struct mtk_mux *muxes, int num,
 			      struct clk_hw_onecell_data *clk_data);
 
+struct mtk_mux_nb {
+	struct notifier_block	nb;
+	const struct mtk_mux	*mux;
+
+	u8	bypass_index;	/* Which parent to temporarily use */
+	u8	original_index;	/* Set by notifier callback */
+};
+
+#define to_mtk_mux_nb(_nb)	container_of(_nb, struct mtk_mux_nb, nb)
+
+int devm_mtk_clk_mux_notifier_register(struct device *dev, struct clk *clk,
+				       struct mtk_mux_nb *mux_nb);
+
 #endif /* __DRV_CLK_MTK_MUX_H */
-- 
2.36.1.124.g0e6072fb45-goog

[PATCH v2 3/4] clk: mediatek: mux: add clk notifier functions

[Chen-Yu Tsai]

With device frequency scaling, the mux clock that (indirectly) feeds the
device selects between a dedicated PLL, and some other stable clocks.

When a clk rate change is requested, the (normally) upstream PLL is
reconfigured. It's possible for the clock output of the PLL to become
unstable during this process.

To avoid causing the device to glitch, the mux should temporarily be
switched over to another "stable" clock during the PLL rate change.
This is done with clk notifiers.

This patch adds common functions for notifiers to temporarily and
transparently reparent mux clocks.

This was loosely based on commit 8adfb08605a9 ("clk: sunxi-ng: mux: Add
clk notifier functions").

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/clk/mediatek/clk-mux.c | 42 ++++++++++++++++++++++++++++++++++
 drivers/clk/mediatek/clk-mux.h | 15 ++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c
index cd5f9fd8cb98..f84a5a753c09 100644
--- a/drivers/clk/mediatek/clk-mux.c
+++ b/drivers/clk/mediatek/clk-mux.c
@@ -4,6 +4,7 @@
  * Author: Owen Chen <owen.chen@mediatek.com>
  */
 
+#include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/compiler_types.h>
 #include <linux/container_of.h>
@@ -259,4 +260,45 @@ void mtk_clk_unregister_muxes(const struct mtk_mux *muxes, int num,
 }
 EXPORT_SYMBOL_GPL(mtk_clk_unregister_muxes);
 
+/*
+ * This clock notifier is called when the frequency of the of the parent
+ * PLL clock is to be changed. The idea is to switch the parent to a
+ * stable clock, such as the main oscillator, while the PLL frequency
+ * stabilizes.
+ */
+static int mtk_clk_mux_notifier_cb(struct notifier_block *nb,
+				   unsigned long event, void *_data)
+{
+	struct clk_notifier_data *data = _data;
+	struct mtk_mux_nb *mux_nb = to_mtk_mux_nb(nb);
+	const struct mtk_mux *mux = mux_nb->mux;
+	struct clk_hw *hw;
+	int ret = 0;
+
+	hw = __clk_get_hw(data->clk);
+
+	switch (event) {
+	case PRE_RATE_CHANGE:
+		mux_nb->original_index = mux->ops->get_parent(hw);
+		ret = mux->ops->set_parent(hw, mux_nb->bypass_index);
+		break;
+
+	case POST_RATE_CHANGE:
+	case ABORT_RATE_CHANGE:
+		ret = mux->ops->set_parent(hw, mux_nb->original_index);
+		break;
+	}
+
+	return notifier_from_errno(ret);
+}
+
+int devm_mtk_clk_mux_notifier_register(struct device *dev, struct clk *clk,
+				       struct mtk_mux_nb *mux_nb)
+{
+	mux_nb->nb.notifier_call = mtk_clk_mux_notifier_cb;
+
+	return devm_clk_notifier_register(dev, clk, &mux_nb->nb);
+}
+EXPORT_SYMBOL_GPL(devm_mtk_clk_mux_notifier_register);
+
 MODULE_LICENSE("GPL");
diff --git a/drivers/clk/mediatek/clk-mux.h b/drivers/clk/mediatek/clk-mux.h
index 6539c58f5d7d..506e91125a3d 100644
--- a/drivers/clk/mediatek/clk-mux.h
+++ b/drivers/clk/mediatek/clk-mux.h
@@ -7,12 +7,14 @@
 #ifndef __DRV_CLK_MTK_MUX_H
 #define __DRV_CLK_MTK_MUX_H
 
+#include <linux/notifier.h>
 #include <linux/spinlock.h>
 #include <linux/types.h>
 
 struct clk;
 struct clk_hw_onecell_data;
 struct clk_ops;
+struct device;
 struct device_node;
 
 struct mtk_mux {
@@ -89,4 +91,17 @@ int mtk_clk_register_muxes(const struct mtk_mux *muxes,
 void mtk_clk_unregister_muxes(const struct mtk_mux *muxes, int num,
 			      struct clk_hw_onecell_data *clk_data);
 
+struct mtk_mux_nb {
+	struct notifier_block	nb;
+	const struct mtk_mux	*mux;
+
+	u8	bypass_index;	/* Which parent to temporarily use */
+	u8	original_index;	/* Set by notifier callback */
+};
+
+#define to_mtk_mux_nb(_nb)	container_of(_nb, struct mtk_mux_nb, nb)
+
+int devm_mtk_clk_mux_notifier_register(struct device *dev, struct clk *clk,
+				       struct mtk_mux_nb *mux_nb);
+
 #endif /* __DRV_CLK_MTK_MUX_H */
-- 
2.36.1.124.g0e6072fb45-goog


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH v2 3/4] clk: mediatek: mux: add clk notifier functions

[Chen-Yu Tsai]

With device frequency scaling, the mux clock that (indirectly) feeds the
device selects between a dedicated PLL, and some other stable clocks.

When a clk rate change is requested, the (normally) upstream PLL is
reconfigured. It's possible for the clock output of the PLL to become
unstable during this process.

To avoid causing the device to glitch, the mux should temporarily be
switched over to another "stable" clock during the PLL rate change.
This is done with clk notifiers.

This patch adds common functions for notifiers to temporarily and
transparently reparent mux clocks.

This was loosely based on commit 8adfb08605a9 ("clk: sunxi-ng: mux: Add
clk notifier functions").

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/clk/mediatek/clk-mux.c | 42 ++++++++++++++++++++++++++++++++++
 drivers/clk/mediatek/clk-mux.h | 15 ++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c
index cd5f9fd8cb98..f84a5a753c09 100644
--- a/drivers/clk/mediatek/clk-mux.c
+++ b/drivers/clk/mediatek/clk-mux.c
@@ -4,6 +4,7 @@
  * Author: Owen Chen <owen.chen@mediatek.com>
  */
 
+#include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/compiler_types.h>
 #include <linux/container_of.h>
@@ -259,4 +260,45 @@ void mtk_clk_unregister_muxes(const struct mtk_mux *muxes, int num,
 }
 EXPORT_SYMBOL_GPL(mtk_clk_unregister_muxes);
 
+/*
+ * This clock notifier is called when the frequency of the of the parent
+ * PLL clock is to be changed. The idea is to switch the parent to a
+ * stable clock, such as the main oscillator, while the PLL frequency
+ * stabilizes.
+ */
+static int mtk_clk_mux_notifier_cb(struct notifier_block *nb,
+				   unsigned long event, void *_data)
+{
+	struct clk_notifier_data *data = _data;
+	struct mtk_mux_nb *mux_nb = to_mtk_mux_nb(nb);
+	const struct mtk_mux *mux = mux_nb->mux;
+	struct clk_hw *hw;
+	int ret = 0;
+
+	hw = __clk_get_hw(data->clk);
+
+	switch (event) {
+	case PRE_RATE_CHANGE:
+		mux_nb->original_index = mux->ops->get_parent(hw);
+		ret = mux->ops->set_parent(hw, mux_nb->bypass_index);
+		break;
+
+	case POST_RATE_CHANGE:
+	case ABORT_RATE_CHANGE:
+		ret = mux->ops->set_parent(hw, mux_nb->original_index);
+		break;
+	}
+
+	return notifier_from_errno(ret);
+}
+
+int devm_mtk_clk_mux_notifier_register(struct device *dev, struct clk *clk,
+				       struct mtk_mux_nb *mux_nb)
+{
+	mux_nb->nb.notifier_call = mtk_clk_mux_notifier_cb;
+
+	return devm_clk_notifier_register(dev, clk, &mux_nb->nb);
+}
+EXPORT_SYMBOL_GPL(devm_mtk_clk_mux_notifier_register);
+
 MODULE_LICENSE("GPL");
diff --git a/drivers/clk/mediatek/clk-mux.h b/drivers/clk/mediatek/clk-mux.h
index 6539c58f5d7d..506e91125a3d 100644
--- a/drivers/clk/mediatek/clk-mux.h
+++ b/drivers/clk/mediatek/clk-mux.h
@@ -7,12 +7,14 @@
 #ifndef __DRV_CLK_MTK_MUX_H
 #define __DRV_CLK_MTK_MUX_H
 
+#include <linux/notifier.h>
 #include <linux/spinlock.h>
 #include <linux/types.h>
 
 struct clk;
 struct clk_hw_onecell_data;
 struct clk_ops;
+struct device;
 struct device_node;
 
 struct mtk_mux {
@@ -89,4 +91,17 @@ int mtk_clk_register_muxes(const struct mtk_mux *muxes,
 void mtk_clk_unregister_muxes(const struct mtk_mux *muxes, int num,
 			      struct clk_hw_onecell_data *clk_data);
 
+struct mtk_mux_nb {
+	struct notifier_block	nb;
+	const struct mtk_mux	*mux;
+
+	u8	bypass_index;	/* Which parent to temporarily use */
+	u8	original_index;	/* Set by notifier callback */
+};
+
+#define to_mtk_mux_nb(_nb)	container_of(_nb, struct mtk_mux_nb, nb)
+
+int devm_mtk_clk_mux_notifier_register(struct device *dev, struct clk *clk,
+				       struct mtk_mux_nb *mux_nb);
+
 #endif /* __DRV_CLK_MTK_MUX_H */
-- 
2.36.1.124.g0e6072fb45-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/4] clk: mediatek: mt8183: Add clk mux notifier for MFG mux

[Chen-Yu Tsai]

When the MFG PLL clock, which is upstream of the MFG clock, is changed,
the downstream clock and consumers need to be switched away from the PLL
over to a stable clock to avoid glitches.

This is done through the use of the newly added clk mux notifier. The
notifier is set on the mux itself instead of the upstream PLL, but in
practice this works, as the rate change notifitcations are propogated
throughout the sub-tree hanging off the PLL. Just before rate changes,
the MFG mux is temporarily and transparently switched to the 26 MHz
main crystal. After the rate change, the mux is switched back.

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
Changes since v1;
- Moved clk notifier registration into separate function
- Fixed comment style

 drivers/clk/mediatek/clk-mt8183.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/clk/mediatek/clk-mt8183.c b/drivers/clk/mediatek/clk-mt8183.c
index b5c17988c337..d66acf2e5e19 100644
--- a/drivers/clk/mediatek/clk-mt8183.c
+++ b/drivers/clk/mediatek/clk-mt8183.c
@@ -1188,10 +1188,33 @@ static void clk_mt8183_top_init_early(struct device_node *node)
 CLK_OF_DECLARE_DRIVER(mt8183_topckgen, "mediatek,mt8183-topckgen",
 			clk_mt8183_top_init_early);
 
+/* Register mux notifier for MFG mux */
+static int clk_mt8183_reg_mfg_mux_notifier(struct device *dev, struct clk *clk)
+{
+	struct mtk_mux_nb *mfg_mux_nb;
+	int i;
+
+	mfg_mux_nb = devm_kzalloc(dev, sizeof(*mfg_mux_nb), GFP_KERNEL);
+	if (!mfg_mux_nb)
+		return -ENOMEM;
+
+	for (i = 0; i < ARRAY_SIZE(top_muxes); i++)
+		if (top_muxes[i].id == CLK_TOP_MUX_MFG)
+			break;
+	if (i == ARRAY_SIZE(top_muxes))
+		return -EINVAL;
+
+	mfg_mux_nb->mux = &top_muxes[i];
+	mfg_mux_nb->bypass_index = 0; /* Bypass to 26M crystal */
+
+	return devm_mtk_clk_mux_notifier_register(dev, clk, mfg_mux_nb);
+}
+
 static int clk_mt8183_top_probe(struct platform_device *pdev)
 {
 	void __iomem *base;
 	struct device_node *node = pdev->dev.of_node;
+	int ret;
 
 	base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(base))
@@ -1217,6 +1240,11 @@ static int clk_mt8183_top_probe(struct platform_device *pdev)
 	mtk_clk_register_gates(node, top_clks, ARRAY_SIZE(top_clks),
 		top_clk_data);
 
+	ret = clk_mt8183_reg_mfg_mux_notifier(&pdev->dev,
+					      top_clk_data->hws[CLK_TOP_MUX_MFG]->clk);
+	if (ret)
+		return ret;
+
 	return of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
 				      top_clk_data);
 }
-- 
2.36.1.124.g0e6072fb45-goog

[PATCH v2 4/4] clk: mediatek: mt8183: Add clk mux notifier for MFG mux

[Chen-Yu Tsai]

When the MFG PLL clock, which is upstream of the MFG clock, is changed,
the downstream clock and consumers need to be switched away from the PLL
over to a stable clock to avoid glitches.

This is done through the use of the newly added clk mux notifier. The
notifier is set on the mux itself instead of the upstream PLL, but in
practice this works, as the rate change notifitcations are propogated
throughout the sub-tree hanging off the PLL. Just before rate changes,
the MFG mux is temporarily and transparently switched to the 26 MHz
main crystal. After the rate change, the mux is switched back.

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
Changes since v1;
- Moved clk notifier registration into separate function
- Fixed comment style

 drivers/clk/mediatek/clk-mt8183.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/clk/mediatek/clk-mt8183.c b/drivers/clk/mediatek/clk-mt8183.c
index b5c17988c337..d66acf2e5e19 100644
--- a/drivers/clk/mediatek/clk-mt8183.c
+++ b/drivers/clk/mediatek/clk-mt8183.c
@@ -1188,10 +1188,33 @@ static void clk_mt8183_top_init_early(struct device_node *node)
 CLK_OF_DECLARE_DRIVER(mt8183_topckgen, "mediatek,mt8183-topckgen",
 			clk_mt8183_top_init_early);
 
+/* Register mux notifier for MFG mux */
+static int clk_mt8183_reg_mfg_mux_notifier(struct device *dev, struct clk *clk)
+{
+	struct mtk_mux_nb *mfg_mux_nb;
+	int i;
+
+	mfg_mux_nb = devm_kzalloc(dev, sizeof(*mfg_mux_nb), GFP_KERNEL);
+	if (!mfg_mux_nb)
+		return -ENOMEM;
+
+	for (i = 0; i < ARRAY_SIZE(top_muxes); i++)
+		if (top_muxes[i].id == CLK_TOP_MUX_MFG)
+			break;
+	if (i == ARRAY_SIZE(top_muxes))
+		return -EINVAL;
+
+	mfg_mux_nb->mux = &top_muxes[i];
+	mfg_mux_nb->bypass_index = 0; /* Bypass to 26M crystal */
+
+	return devm_mtk_clk_mux_notifier_register(dev, clk, mfg_mux_nb);
+}
+
 static int clk_mt8183_top_probe(struct platform_device *pdev)
 {
 	void __iomem *base;
 	struct device_node *node = pdev->dev.of_node;
+	int ret;
 
 	base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(base))
@@ -1217,6 +1240,11 @@ static int clk_mt8183_top_probe(struct platform_device *pdev)
 	mtk_clk_register_gates(node, top_clks, ARRAY_SIZE(top_clks),
 		top_clk_data);
 
+	ret = clk_mt8183_reg_mfg_mux_notifier(&pdev->dev,
+					      top_clk_data->hws[CLK_TOP_MUX_MFG]->clk);
+	if (ret)
+		return ret;
+
 	return of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
 				      top_clk_data);
 }
-- 
2.36.1.124.g0e6072fb45-goog


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH v2 4/4] clk: mediatek: mt8183: Add clk mux notifier for MFG mux

[Chen-Yu Tsai]

When the MFG PLL clock, which is upstream of the MFG clock, is changed,
the downstream clock and consumers need to be switched away from the PLL
over to a stable clock to avoid glitches.

This is done through the use of the newly added clk mux notifier. The
notifier is set on the mux itself instead of the upstream PLL, but in
practice this works, as the rate change notifitcations are propogated
throughout the sub-tree hanging off the PLL. Just before rate changes,
the MFG mux is temporarily and transparently switched to the 26 MHz
main crystal. After the rate change, the mux is switched back.

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
Changes since v1;
- Moved clk notifier registration into separate function
- Fixed comment style

 drivers/clk/mediatek/clk-mt8183.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/clk/mediatek/clk-mt8183.c b/drivers/clk/mediatek/clk-mt8183.c
index b5c17988c337..d66acf2e5e19 100644
--- a/drivers/clk/mediatek/clk-mt8183.c
+++ b/drivers/clk/mediatek/clk-mt8183.c
@@ -1188,10 +1188,33 @@ static void clk_mt8183_top_init_early(struct device_node *node)
 CLK_OF_DECLARE_DRIVER(mt8183_topckgen, "mediatek,mt8183-topckgen",
 			clk_mt8183_top_init_early);
 
+/* Register mux notifier for MFG mux */
+static int clk_mt8183_reg_mfg_mux_notifier(struct device *dev, struct clk *clk)
+{
+	struct mtk_mux_nb *mfg_mux_nb;
+	int i;
+
+	mfg_mux_nb = devm_kzalloc(dev, sizeof(*mfg_mux_nb), GFP_KERNEL);
+	if (!mfg_mux_nb)
+		return -ENOMEM;
+
+	for (i = 0; i < ARRAY_SIZE(top_muxes); i++)
+		if (top_muxes[i].id == CLK_TOP_MUX_MFG)
+			break;
+	if (i == ARRAY_SIZE(top_muxes))
+		return -EINVAL;
+
+	mfg_mux_nb->mux = &top_muxes[i];
+	mfg_mux_nb->bypass_index = 0; /* Bypass to 26M crystal */
+
+	return devm_mtk_clk_mux_notifier_register(dev, clk, mfg_mux_nb);
+}
+
 static int clk_mt8183_top_probe(struct platform_device *pdev)
 {
 	void __iomem *base;
 	struct device_node *node = pdev->dev.of_node;
+	int ret;
 
 	base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(base))
@@ -1217,6 +1240,11 @@ static int clk_mt8183_top_probe(struct platform_device *pdev)
 	mtk_clk_register_gates(node, top_clks, ARRAY_SIZE(top_clks),
 		top_clk_data);
 
+	ret = clk_mt8183_reg_mfg_mux_notifier(&pdev->dev,
+					      top_clk_data->hws[CLK_TOP_MUX_MFG]->clk);
+	if (ret)
+		return ret;
+
 	return of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
 				      top_clk_data);
 }
-- 
2.36.1.124.g0e6072fb45-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/4] clk: mediatek: mux: add clk notifier functions

[AngeloGioacchino Del Regno]

Il 23/05/22 10:59, Chen-Yu Tsai ha scritto:
> With device frequency scaling, the mux clock that (indirectly) feeds the
> device selects between a dedicated PLL, and some other stable clocks.
> 
> When a clk rate change is requested, the (normally) upstream PLL is
> reconfigured. It's possible for the clock output of the PLL to become
> unstable during this process.
> 
> To avoid causing the device to glitch, the mux should temporarily be
> switched over to another "stable" clock during the PLL rate change.
> This is done with clk notifiers.
> 
> This patch adds common functions for notifiers to temporarily and
> transparently reparent mux clocks.
> 
> This was loosely based on commit 8adfb08605a9 ("clk: sunxi-ng: mux: Add
> clk notifier functions").
> 
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
>   drivers/clk/mediatek/clk-mux.c | 42 ++++++++++++++++++++++++++++++++++
>   drivers/clk/mediatek/clk-mux.h | 15 ++++++++++++
>   2 files changed, 57 insertions(+)
> 
> diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c
> index cd5f9fd8cb98..f84a5a753c09 100644
> --- a/drivers/clk/mediatek/clk-mux.c
> +++ b/drivers/clk/mediatek/clk-mux.c
> @@ -4,6 +4,7 @@
>    * Author: Owen Chen <owen.chen@mediatek.com>
>    */
>   
> +#include <linux/clk.h>
>   #include <linux/clk-provider.h>
>   #include <linux/compiler_types.h>
>   #include <linux/container_of.h>
> @@ -259,4 +260,45 @@ void mtk_clk_unregister_muxes(const struct mtk_mux *muxes, int num,
>   }
>   EXPORT_SYMBOL_GPL(mtk_clk_unregister_muxes);
>   
> +/*
> + * This clock notifier is called when the frequency of the of the parent
> + * PLL clock is to be changed. The idea is to switch the parent to a
> + * stable clock, such as the main oscillator, while the PLL frequency
> + * stabilizes.
> + */
> +static int mtk_clk_mux_notifier_cb(struct notifier_block *nb,
> +				   unsigned long event, void *_data)
> +{
> +	struct clk_notifier_data *data = _data;
> +	struct mtk_mux_nb *mux_nb = to_mtk_mux_nb(nb);
> +	const struct mtk_mux *mux = mux_nb->mux;
> +	struct clk_hw *hw;
> +	int ret = 0;
> +
> +	hw = __clk_get_hw(data->clk);
> +
> +	switch (event) {
> +	case PRE_RATE_CHANGE:
> +		mux_nb->original_index = mux->ops->get_parent(hw);
> +		ret = mux->ops->set_parent(hw, mux_nb->bypass_index);
> +		break;
> +
> +	case POST_RATE_CHANGE:
> +	case ABORT_RATE_CHANGE:

I agree with this change, entirely - but there's an issue here.
If we enter ABORT_RATE_CHANGE, this means that "something has failed": now,
what if the failure point was the PLL being unable to lock?

In that case, we would switch the parent back to a PLL that's not outputting
any clock, crashing the GPU, or a bogus rate, potentially undervolting the GPU.

I think that the best idea here would be to do something like..

	switch (event) {
	case PRE_RATE_CHANGE:
		mux_nb->old_parent_idx = mux->ops->get_parent(hw);
		ret = mux->ops->set_parent(hw, mux_nb->safe_parent_idx);
		break;
	case POST_RATE_CHANGE:
		ret = mux->ops->set_parent(hw, mux_nb->old_parent_idx);
		break;
	case ABORT_RATE_CHANGE:
		ret = -EINVAL; /* or -ECANCELED, whatever... */
		break;
	}

> +		ret = mux->ops->set_parent(hw, mux_nb->original_index);
> +		break;
> +	}
> +
> +	return notifier_from_errno(ret);
> +}
> +
> +int devm_mtk_clk_mux_notifier_register(struct device *dev, struct clk *clk,
> +				       struct mtk_mux_nb *mux_nb)
> +{
> +	mux_nb->nb.notifier_call = mtk_clk_mux_notifier_cb;
> +
> +	return devm_clk_notifier_register(dev, clk, &mux_nb->nb);
> +}
> +EXPORT_SYMBOL_GPL(devm_mtk_clk_mux_notifier_register);
> +
>   MODULE_LICENSE("GPL");
> diff --git a/drivers/clk/mediatek/clk-mux.h b/drivers/clk/mediatek/clk-mux.h
> index 6539c58f5d7d..506e91125a3d 100644
> --- a/drivers/clk/mediatek/clk-mux.h
> +++ b/drivers/clk/mediatek/clk-mux.h
> @@ -7,12 +7,14 @@
>   #ifndef __DRV_CLK_MTK_MUX_H
>   #define __DRV_CLK_MTK_MUX_H
>   
> +#include <linux/notifier.h>
>   #include <linux/spinlock.h>
>   #include <linux/types.h>
>   
>   struct clk;
>   struct clk_hw_onecell_data;
>   struct clk_ops;
> +struct device;
>   struct device_node;
>   
>   struct mtk_mux {
> @@ -89,4 +91,17 @@ int mtk_clk_register_muxes(const struct mtk_mux *muxes,
>   void mtk_clk_unregister_muxes(const struct mtk_mux *muxes, int num,
>   			      struct clk_hw_onecell_data *clk_data);
>   
> +struct mtk_mux_nb {
> +	struct notifier_block	nb;
> +	const struct mtk_mux	*mux;
> +
> +	u8	bypass_index;	/* Which parent to temporarily use */
> +	u8	original_index;	/* Set by notifier callback */

I think that the following names are more explanatory:

	u8	safe_parent_idx;
	u8	old_parent_idx;

...because I see this as a mechanism to switch the mux to a "safe" clock output
and then back to the PLL (like it's done on some qcom clocks as well).

You're free to ignore this comment, as this is, of course, just a personal opinion.

Cheers,
Angelo

Re: [PATCH v2 3/4] clk: mediatek: mux: add clk notifier functions

[AngeloGioacchino Del Regno]

Il 23/05/22 10:59, Chen-Yu Tsai ha scritto:
> With device frequency scaling, the mux clock that (indirectly) feeds the
> device selects between a dedicated PLL, and some other stable clocks.
> 
> When a clk rate change is requested, the (normally) upstream PLL is
> reconfigured. It's possible for the clock output of the PLL to become
> unstable during this process.
> 
> To avoid causing the device to glitch, the mux should temporarily be
> switched over to another "stable" clock during the PLL rate change.
> This is done with clk notifiers.
> 
> This patch adds common functions for notifiers to temporarily and
> transparently reparent mux clocks.
> 
> This was loosely based on commit 8adfb08605a9 ("clk: sunxi-ng: mux: Add
> clk notifier functions").
> 
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
>   drivers/clk/mediatek/clk-mux.c | 42 ++++++++++++++++++++++++++++++++++
>   drivers/clk/mediatek/clk-mux.h | 15 ++++++++++++
>   2 files changed, 57 insertions(+)
> 
> diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c
> index cd5f9fd8cb98..f84a5a753c09 100644
> --- a/drivers/clk/mediatek/clk-mux.c
> +++ b/drivers/clk/mediatek/clk-mux.c
> @@ -4,6 +4,7 @@
>    * Author: Owen Chen <owen.chen@mediatek.com>
>    */
>   
> +#include <linux/clk.h>
>   #include <linux/clk-provider.h>
>   #include <linux/compiler_types.h>
>   #include <linux/container_of.h>
> @@ -259,4 +260,45 @@ void mtk_clk_unregister_muxes(const struct mtk_mux *muxes, int num,
>   }
>   EXPORT_SYMBOL_GPL(mtk_clk_unregister_muxes);
>   
> +/*
> + * This clock notifier is called when the frequency of the of the parent
> + * PLL clock is to be changed. The idea is to switch the parent to a
> + * stable clock, such as the main oscillator, while the PLL frequency
> + * stabilizes.
> + */
> +static int mtk_clk_mux_notifier_cb(struct notifier_block *nb,
> +				   unsigned long event, void *_data)
> +{
> +	struct clk_notifier_data *data = _data;
> +	struct mtk_mux_nb *mux_nb = to_mtk_mux_nb(nb);
> +	const struct mtk_mux *mux = mux_nb->mux;
> +	struct clk_hw *hw;
> +	int ret = 0;
> +
> +	hw = __clk_get_hw(data->clk);
> +
> +	switch (event) {
> +	case PRE_RATE_CHANGE:
> +		mux_nb->original_index = mux->ops->get_parent(hw);
> +		ret = mux->ops->set_parent(hw, mux_nb->bypass_index);
> +		break;
> +
> +	case POST_RATE_CHANGE:
> +	case ABORT_RATE_CHANGE:

I agree with this change, entirely - but there's an issue here.
If we enter ABORT_RATE_CHANGE, this means that "something has failed": now,
what if the failure point was the PLL being unable to lock?

In that case, we would switch the parent back to a PLL that's not outputting
any clock, crashing the GPU, or a bogus rate, potentially undervolting the GPU.

I think that the best idea here would be to do something like..

	switch (event) {
	case PRE_RATE_CHANGE:
		mux_nb->old_parent_idx = mux->ops->get_parent(hw);
		ret = mux->ops->set_parent(hw, mux_nb->safe_parent_idx);
		break;
	case POST_RATE_CHANGE:
		ret = mux->ops->set_parent(hw, mux_nb->old_parent_idx);
		break;
	case ABORT_RATE_CHANGE:
		ret = -EINVAL; /* or -ECANCELED, whatever... */
		break;
	}

> +		ret = mux->ops->set_parent(hw, mux_nb->original_index);
> +		break;
> +	}
> +
> +	return notifier_from_errno(ret);
> +}
> +
> +int devm_mtk_clk_mux_notifier_register(struct device *dev, struct clk *clk,
> +				       struct mtk_mux_nb *mux_nb)
> +{
> +	mux_nb->nb.notifier_call = mtk_clk_mux_notifier_cb;
> +
> +	return devm_clk_notifier_register(dev, clk, &mux_nb->nb);
> +}
> +EXPORT_SYMBOL_GPL(devm_mtk_clk_mux_notifier_register);
> +
>   MODULE_LICENSE("GPL");
> diff --git a/drivers/clk/mediatek/clk-mux.h b/drivers/clk/mediatek/clk-mux.h
> index 6539c58f5d7d..506e91125a3d 100644
> --- a/drivers/clk/mediatek/clk-mux.h
> +++ b/drivers/clk/mediatek/clk-mux.h
> @@ -7,12 +7,14 @@
>   #ifndef __DRV_CLK_MTK_MUX_H
>   #define __DRV_CLK_MTK_MUX_H
>   
> +#include <linux/notifier.h>
>   #include <linux/spinlock.h>
>   #include <linux/types.h>
>   
>   struct clk;
>   struct clk_hw_onecell_data;
>   struct clk_ops;
> +struct device;
>   struct device_node;
>   
>   struct mtk_mux {
> @@ -89,4 +91,17 @@ int mtk_clk_register_muxes(const struct mtk_mux *muxes,
>   void mtk_clk_unregister_muxes(const struct mtk_mux *muxes, int num,
>   			      struct clk_hw_onecell_data *clk_data);
>   
> +struct mtk_mux_nb {
> +	struct notifier_block	nb;
> +	const struct mtk_mux	*mux;
> +
> +	u8	bypass_index;	/* Which parent to temporarily use */
> +	u8	original_index;	/* Set by notifier callback */

I think that the following names are more explanatory:

	u8	safe_parent_idx;
	u8	old_parent_idx;

...because I see this as a mechanism to switch the mux to a "safe" clock output
and then back to the PLL (like it's done on some qcom clocks as well).

You're free to ignore this comment, as this is, of course, just a personal opinion.

Cheers,
Angelo



_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v2 3/4] clk: mediatek: mux: add clk notifier functions

[AngeloGioacchino Del Regno]

Il 23/05/22 10:59, Chen-Yu Tsai ha scritto:
> With device frequency scaling, the mux clock that (indirectly) feeds the
> device selects between a dedicated PLL, and some other stable clocks.
> 
> When a clk rate change is requested, the (normally) upstream PLL is
> reconfigured. It's possible for the clock output of the PLL to become
> unstable during this process.
> 
> To avoid causing the device to glitch, the mux should temporarily be
> switched over to another "stable" clock during the PLL rate change.
> This is done with clk notifiers.
> 
> This patch adds common functions for notifiers to temporarily and
> transparently reparent mux clocks.
> 
> This was loosely based on commit 8adfb08605a9 ("clk: sunxi-ng: mux: Add
> clk notifier functions").
> 
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
>   drivers/clk/mediatek/clk-mux.c | 42 ++++++++++++++++++++++++++++++++++
>   drivers/clk/mediatek/clk-mux.h | 15 ++++++++++++
>   2 files changed, 57 insertions(+)
> 
> diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c
> index cd5f9fd8cb98..f84a5a753c09 100644
> --- a/drivers/clk/mediatek/clk-mux.c
> +++ b/drivers/clk/mediatek/clk-mux.c
> @@ -4,6 +4,7 @@
>    * Author: Owen Chen <owen.chen@mediatek.com>
>    */
>   
> +#include <linux/clk.h>
>   #include <linux/clk-provider.h>
>   #include <linux/compiler_types.h>
>   #include <linux/container_of.h>
> @@ -259,4 +260,45 @@ void mtk_clk_unregister_muxes(const struct mtk_mux *muxes, int num,
>   }
>   EXPORT_SYMBOL_GPL(mtk_clk_unregister_muxes);
>   
> +/*
> + * This clock notifier is called when the frequency of the of the parent
> + * PLL clock is to be changed. The idea is to switch the parent to a
> + * stable clock, such as the main oscillator, while the PLL frequency
> + * stabilizes.
> + */
> +static int mtk_clk_mux_notifier_cb(struct notifier_block *nb,
> +				   unsigned long event, void *_data)
> +{
> +	struct clk_notifier_data *data = _data;
> +	struct mtk_mux_nb *mux_nb = to_mtk_mux_nb(nb);
> +	const struct mtk_mux *mux = mux_nb->mux;
> +	struct clk_hw *hw;
> +	int ret = 0;
> +
> +	hw = __clk_get_hw(data->clk);
> +
> +	switch (event) {
> +	case PRE_RATE_CHANGE:
> +		mux_nb->original_index = mux->ops->get_parent(hw);
> +		ret = mux->ops->set_parent(hw, mux_nb->bypass_index);
> +		break;
> +
> +	case POST_RATE_CHANGE:
> +	case ABORT_RATE_CHANGE:

I agree with this change, entirely - but there's an issue here.
If we enter ABORT_RATE_CHANGE, this means that "something has failed": now,
what if the failure point was the PLL being unable to lock?

In that case, we would switch the parent back to a PLL that's not outputting
any clock, crashing the GPU, or a bogus rate, potentially undervolting the GPU.

I think that the best idea here would be to do something like..

	switch (event) {
	case PRE_RATE_CHANGE:
		mux_nb->old_parent_idx = mux->ops->get_parent(hw);
		ret = mux->ops->set_parent(hw, mux_nb->safe_parent_idx);
		break;
	case POST_RATE_CHANGE:
		ret = mux->ops->set_parent(hw, mux_nb->old_parent_idx);
		break;
	case ABORT_RATE_CHANGE:
		ret = -EINVAL; /* or -ECANCELED, whatever... */
		break;
	}

> +		ret = mux->ops->set_parent(hw, mux_nb->original_index);
> +		break;
> +	}
> +
> +	return notifier_from_errno(ret);
> +}
> +
> +int devm_mtk_clk_mux_notifier_register(struct device *dev, struct clk *clk,
> +				       struct mtk_mux_nb *mux_nb)
> +{
> +	mux_nb->nb.notifier_call = mtk_clk_mux_notifier_cb;
> +
> +	return devm_clk_notifier_register(dev, clk, &mux_nb->nb);
> +}
> +EXPORT_SYMBOL_GPL(devm_mtk_clk_mux_notifier_register);
> +
>   MODULE_LICENSE("GPL");
> diff --git a/drivers/clk/mediatek/clk-mux.h b/drivers/clk/mediatek/clk-mux.h
> index 6539c58f5d7d..506e91125a3d 100644
> --- a/drivers/clk/mediatek/clk-mux.h
> +++ b/drivers/clk/mediatek/clk-mux.h
> @@ -7,12 +7,14 @@
>   #ifndef __DRV_CLK_MTK_MUX_H
>   #define __DRV_CLK_MTK_MUX_H
>   
> +#include <linux/notifier.h>
>   #include <linux/spinlock.h>
>   #include <linux/types.h>
>   
>   struct clk;
>   struct clk_hw_onecell_data;
>   struct clk_ops;
> +struct device;
>   struct device_node;
>   
>   struct mtk_mux {
> @@ -89,4 +91,17 @@ int mtk_clk_register_muxes(const struct mtk_mux *muxes,
>   void mtk_clk_unregister_muxes(const struct mtk_mux *muxes, int num,
>   			      struct clk_hw_onecell_data *clk_data);
>   
> +struct mtk_mux_nb {
> +	struct notifier_block	nb;
> +	const struct mtk_mux	*mux;
> +
> +	u8	bypass_index;	/* Which parent to temporarily use */
> +	u8	original_index;	/* Set by notifier callback */

I think that the following names are more explanatory:

	u8	safe_parent_idx;
	u8	old_parent_idx;

...because I see this as a mechanism to switch the mux to a "safe" clock output
and then back to the PLL (like it's done on some qcom clocks as well).

You're free to ignore this comment, as this is, of course, just a personal opinion.

Cheers,
Angelo



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/4] clk: mediatek: mt8183: mfgcfg: Propagate rate changes to parent

[AngeloGioacchino Del Regno]

Il 23/05/22 10:59, Chen-Yu Tsai ha scritto:
> The only clock in the MT8183 MFGCFG block feeds the GPU. Propagate its
> rate change requests to its parent, so that DVFS for the GPU can work
> properly.
> 
> Fixes: acddfc2c261b ("clk: mediatek: Add MT8183 clock support")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH v2 2/4] clk: mediatek: mt8183: mfgcfg: Propagate rate changes to parent

[AngeloGioacchino Del Regno]

Il 23/05/22 10:59, Chen-Yu Tsai ha scritto:
> The only clock in the MT8183 MFGCFG block feeds the GPU. Propagate its
> rate change requests to its parent, so that DVFS for the GPU can work
> properly.
> 
> Fixes: acddfc2c261b ("clk: mediatek: Add MT8183 clock support")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v2 2/4] clk: mediatek: mt8183: mfgcfg: Propagate rate changes to parent

[AngeloGioacchino Del Regno]

Il 23/05/22 10:59, Chen-Yu Tsai ha scritto:
> The only clock in the MT8183 MFGCFG block feeds the GPU. Propagate its
> rate change requests to its parent, so that DVFS for the GPU can work
> properly.
> 
> Fixes: acddfc2c261b ("clk: mediatek: Add MT8183 clock support")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/4] arm64: dts: mt8183: Fix Mali GPU clock

[AngeloGioacchino Del Regno]

Il 23/05/22 10:59, Chen-Yu Tsai ha scritto:
> The actual clock feeding into the Mali GPU on the MT8183 is from the
> clock gate in the MFGCFG block, not CLK_TOP_MFGPLL_CK from the TOPCKGEN
> block, which itself is simply a pass-through placeholder for the MFGPLL
> in the APMIXEDSYS block.
> 
> Fix the hardware description with the correct clock reference.
> 
> Fixes: a8168cebf1bc ("arm64: dts: mt8183: Add node for the Mali GPU")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

> ---
>   arch/arm64/boot/dts/mediatek/mt8183.dtsi | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> index 01e650251928..6ced76a60aab 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> @@ -1368,7 +1368,7 @@ gpu: gpu@13040000 {
>   				<GIC_SPI 278 IRQ_TYPE_LEVEL_LOW>;
>   			interrupt-names = "job", "mmu", "gpu";
>   
> -			clocks = <&topckgen CLK_TOP_MFGPLL_CK>;
> +			clocks = <&mfgcfg CLK_MFG_BG3D>;
>   
>   			power-domains =
>   				<&spm MT8183_POWER_DOMAIN_MFG_CORE0>,

Re: [PATCH v2 1/4] arm64: dts: mt8183: Fix Mali GPU clock

[AngeloGioacchino Del Regno]

Il 23/05/22 10:59, Chen-Yu Tsai ha scritto:
> The actual clock feeding into the Mali GPU on the MT8183 is from the
> clock gate in the MFGCFG block, not CLK_TOP_MFGPLL_CK from the TOPCKGEN
> block, which itself is simply a pass-through placeholder for the MFGPLL
> in the APMIXEDSYS block.
> 
> Fix the hardware description with the correct clock reference.
> 
> Fixes: a8168cebf1bc ("arm64: dts: mt8183: Add node for the Mali GPU")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

> ---
>   arch/arm64/boot/dts/mediatek/mt8183.dtsi | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> index 01e650251928..6ced76a60aab 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> @@ -1368,7 +1368,7 @@ gpu: gpu@13040000 {
>   				<GIC_SPI 278 IRQ_TYPE_LEVEL_LOW>;
>   			interrupt-names = "job", "mmu", "gpu";
>   
> -			clocks = <&topckgen CLK_TOP_MFGPLL_CK>;
> +			clocks = <&mfgcfg CLK_MFG_BG3D>;
>   
>   			power-domains =
>   				<&spm MT8183_POWER_DOMAIN_MFG_CORE0>,

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v2 1/4] arm64: dts: mt8183: Fix Mali GPU clock

[AngeloGioacchino Del Regno]

Il 23/05/22 10:59, Chen-Yu Tsai ha scritto:
> The actual clock feeding into the Mali GPU on the MT8183 is from the
> clock gate in the MFGCFG block, not CLK_TOP_MFGPLL_CK from the TOPCKGEN
> block, which itself is simply a pass-through placeholder for the MFGPLL
> in the APMIXEDSYS block.
> 
> Fix the hardware description with the correct clock reference.
> 
> Fixes: a8168cebf1bc ("arm64: dts: mt8183: Add node for the Mali GPU")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

> ---
>   arch/arm64/boot/dts/mediatek/mt8183.dtsi | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> index 01e650251928..6ced76a60aab 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> @@ -1368,7 +1368,7 @@ gpu: gpu@13040000 {
>   				<GIC_SPI 278 IRQ_TYPE_LEVEL_LOW>;
>   			interrupt-names = "job", "mmu", "gpu";
>   
> -			clocks = <&topckgen CLK_TOP_MFGPLL_CK>;
> +			clocks = <&mfgcfg CLK_MFG_BG3D>;
>   
>   			power-domains =
>   				<&spm MT8183_POWER_DOMAIN_MFG_CORE0>,

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/4] clk: mediatek: mt8183: Add clk mux notifier for MFG mux

[AngeloGioacchino Del Regno]

Il 23/05/22 10:59, Chen-Yu Tsai ha scritto:
> When the MFG PLL clock, which is upstream of the MFG clock, is changed,
> the downstream clock and consumers need to be switched away from the PLL
> over to a stable clock to avoid glitches.
> 
> This is done through the use of the newly added clk mux notifier. The
> notifier is set on the mux itself instead of the upstream PLL, but in
> practice this works, as the rate change notifitcations are propogated
> throughout the sub-tree hanging off the PLL. Just before rate changes,
> the MFG mux is temporarily and transparently switched to the 26 MHz
> main crystal. After the rate change, the mux is switched back.
> 
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH v2 4/4] clk: mediatek: mt8183: Add clk mux notifier for MFG mux

[AngeloGioacchino Del Regno]

Il 23/05/22 10:59, Chen-Yu Tsai ha scritto:
> When the MFG PLL clock, which is upstream of the MFG clock, is changed,
> the downstream clock and consumers need to be switched away from the PLL
> over to a stable clock to avoid glitches.
> 
> This is done through the use of the newly added clk mux notifier. The
> notifier is set on the mux itself instead of the upstream PLL, but in
> practice this works, as the rate change notifitcations are propogated
> throughout the sub-tree hanging off the PLL. Just before rate changes,
> the MFG mux is temporarily and transparently switched to the 26 MHz
> main crystal. After the rate change, the mux is switched back.
> 
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v2 4/4] clk: mediatek: mt8183: Add clk mux notifier for MFG mux

[AngeloGioacchino Del Regno]

Il 23/05/22 10:59, Chen-Yu Tsai ha scritto:
> When the MFG PLL clock, which is upstream of the MFG clock, is changed,
> the downstream clock and consumers need to be switched away from the PLL
> over to a stable clock to avoid glitches.
> 
> This is done through the use of the newly added clk mux notifier. The
> notifier is set on the mux itself instead of the upstream PLL, but in
> practice this works, as the rate change notifitcations are propogated
> throughout the sub-tree hanging off the PLL. Just before rate changes,
> the MFG mux is temporarily and transparently switched to the 26 MHz
> main crystal. After the rate change, the mux is switched back.
> 
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/4] clk: mediatek: mux: add clk notifier functions

[Chen-Yu Tsai]

On Mon, May 23, 2022 at 6:04 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 23/05/22 10:59, Chen-Yu Tsai ha scritto:
> > With device frequency scaling, the mux clock that (indirectly) feeds the
> > device selects between a dedicated PLL, and some other stable clocks.
> >
> > When a clk rate change is requested, the (normally) upstream PLL is
> > reconfigured. It's possible for the clock output of the PLL to become
> > unstable during this process.
> >
> > To avoid causing the device to glitch, the mux should temporarily be
> > switched over to another "stable" clock during the PLL rate change.
> > This is done with clk notifiers.
> >
> > This patch adds common functions for notifiers to temporarily and
> > transparently reparent mux clocks.
> >
> > This was loosely based on commit 8adfb08605a9 ("clk: sunxi-ng: mux: Add
> > clk notifier functions").
> >
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> >   drivers/clk/mediatek/clk-mux.c | 42 ++++++++++++++++++++++++++++++++++
> >   drivers/clk/mediatek/clk-mux.h | 15 ++++++++++++
> >   2 files changed, 57 insertions(+)
> >
> > diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c
> > index cd5f9fd8cb98..f84a5a753c09 100644
> > --- a/drivers/clk/mediatek/clk-mux.c
> > +++ b/drivers/clk/mediatek/clk-mux.c
> > @@ -4,6 +4,7 @@
> >    * Author: Owen Chen <owen.chen@mediatek.com>
> >    */
> >
> > +#include <linux/clk.h>
> >   #include <linux/clk-provider.h>
> >   #include <linux/compiler_types.h>
> >   #include <linux/container_of.h>
> > @@ -259,4 +260,45 @@ void mtk_clk_unregister_muxes(const struct mtk_mux *muxes, int num,
> >   }
> >   EXPORT_SYMBOL_GPL(mtk_clk_unregister_muxes);
> >
> > +/*
> > + * This clock notifier is called when the frequency of the of the parent
> > + * PLL clock is to be changed. The idea is to switch the parent to a
> > + * stable clock, such as the main oscillator, while the PLL frequency
> > + * stabilizes.
> > + */
> > +static int mtk_clk_mux_notifier_cb(struct notifier_block *nb,
> > +                                unsigned long event, void *_data)
> > +{
> > +     struct clk_notifier_data *data = _data;
> > +     struct mtk_mux_nb *mux_nb = to_mtk_mux_nb(nb);
> > +     const struct mtk_mux *mux = mux_nb->mux;
> > +     struct clk_hw *hw;
> > +     int ret = 0;
> > +
> > +     hw = __clk_get_hw(data->clk);
> > +
> > +     switch (event) {
> > +     case PRE_RATE_CHANGE:
> > +             mux_nb->original_index = mux->ops->get_parent(hw);
> > +             ret = mux->ops->set_parent(hw, mux_nb->bypass_index);
> > +             break;
> > +
> > +     case POST_RATE_CHANGE:
> > +     case ABORT_RATE_CHANGE:
>
> I agree with this change, entirely - but there's an issue here.
> If we enter ABORT_RATE_CHANGE, this means that "something has failed": now,
> what if the failure point was the PLL being unable to lock?

I think this is a valid concern. But the MediaTek clk driver library doesn't
actually check if the PLL locked or not. It simply sets it and returns.

Also, if the notifier weren't there, there'd be no transparent muxing
either, and the GPU would always be clocked from the PLL, locked or
not.

Considering that muxing away is temporary and transparent, muxing back
simply restores the system to the way the CCF thinks it is. If the PLL
fails to lock, we need other fixes or constraints.

> In that case, we would switch the parent back to a PLL that's not outputting
> any clock, crashing the GPU, or a bogus rate, potentially undervolting the GPU.

For MT8183, there is no other "safe" PLL. The stable ones run faster than
the lowest OPP.

> I think that the best idea here would be to do something like..
>
>         switch (event) {
>         case PRE_RATE_CHANGE:
>                 mux_nb->old_parent_idx = mux->ops->get_parent(hw);
>                 ret = mux->ops->set_parent(hw, mux_nb->safe_parent_idx);
>                 break;
>         case POST_RATE_CHANGE:
>                 ret = mux->ops->set_parent(hw, mux_nb->old_parent_idx);
>                 break;
>         case ABORT_RATE_CHANGE:
>                 ret = -EINVAL; /* or -ECANCELED, whatever... */
>                 break;
>         }
>
> > +             ret = mux->ops->set_parent(hw, mux_nb->original_index);
> > +             break;
> > +     }
> > +
> > +     return notifier_from_errno(ret);
> > +}
> > +
> > +int devm_mtk_clk_mux_notifier_register(struct device *dev, struct clk *clk,
> > +                                    struct mtk_mux_nb *mux_nb)
> > +{
> > +     mux_nb->nb.notifier_call = mtk_clk_mux_notifier_cb;
> > +
> > +     return devm_clk_notifier_register(dev, clk, &mux_nb->nb);
> > +}
> > +EXPORT_SYMBOL_GPL(devm_mtk_clk_mux_notifier_register);
> > +
> >   MODULE_LICENSE("GPL");
> > diff --git a/drivers/clk/mediatek/clk-mux.h b/drivers/clk/mediatek/clk-mux.h
> > index 6539c58f5d7d..506e91125a3d 100644
> > --- a/drivers/clk/mediatek/clk-mux.h
> > +++ b/drivers/clk/mediatek/clk-mux.h
> > @@ -7,12 +7,14 @@
> >   #ifndef __DRV_CLK_MTK_MUX_H
> >   #define __DRV_CLK_MTK_MUX_H
> >
> > +#include <linux/notifier.h>
> >   #include <linux/spinlock.h>
> >   #include <linux/types.h>
> >
> >   struct clk;
> >   struct clk_hw_onecell_data;
> >   struct clk_ops;
> > +struct device;
> >   struct device_node;
> >
> >   struct mtk_mux {
> > @@ -89,4 +91,17 @@ int mtk_clk_register_muxes(const struct mtk_mux *muxes,
> >   void mtk_clk_unregister_muxes(const struct mtk_mux *muxes, int num,
> >                             struct clk_hw_onecell_data *clk_data);
> >
> > +struct mtk_mux_nb {
> > +     struct notifier_block   nb;
> > +     const struct mtk_mux    *mux;
> > +
> > +     u8      bypass_index;   /* Which parent to temporarily use */
> > +     u8      original_index; /* Set by notifier callback */
>
> I think that the following names are more explanatory:
>
>         u8      safe_parent_idx;
>         u8      old_parent_idx;
>
> ...because I see this as a mechanism to switch the mux to a "safe" clock output
> and then back to the PLL (like it's done on some qcom clocks as well).

The names were taken from the sunxi-ng driver, which always muxed to a
crystal clock, so in a way was bypassing the PLL.

ChenYu

> You're free to ignore this comment, as this is, of course, just a personal opinion.
>
> Cheers,
> Angelo
>
>

Re: [PATCH v2 3/4] clk: mediatek: mux: add clk notifier functions

[Chen-Yu Tsai]

On Mon, May 23, 2022 at 6:04 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 23/05/22 10:59, Chen-Yu Tsai ha scritto:
> > With device frequency scaling, the mux clock that (indirectly) feeds the
> > device selects between a dedicated PLL, and some other stable clocks.
> >
> > When a clk rate change is requested, the (normally) upstream PLL is
> > reconfigured. It's possible for the clock output of the PLL to become
> > unstable during this process.
> >
> > To avoid causing the device to glitch, the mux should temporarily be
> > switched over to another "stable" clock during the PLL rate change.
> > This is done with clk notifiers.
> >
> > This patch adds common functions for notifiers to temporarily and
> > transparently reparent mux clocks.
> >
> > This was loosely based on commit 8adfb08605a9 ("clk: sunxi-ng: mux: Add
> > clk notifier functions").
> >
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> >   drivers/clk/mediatek/clk-mux.c | 42 ++++++++++++++++++++++++++++++++++
> >   drivers/clk/mediatek/clk-mux.h | 15 ++++++++++++
> >   2 files changed, 57 insertions(+)
> >
> > diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c
> > index cd5f9fd8cb98..f84a5a753c09 100644
> > --- a/drivers/clk/mediatek/clk-mux.c
> > +++ b/drivers/clk/mediatek/clk-mux.c
> > @@ -4,6 +4,7 @@
> >    * Author: Owen Chen <owen.chen@mediatek.com>
> >    */
> >
> > +#include <linux/clk.h>
> >   #include <linux/clk-provider.h>
> >   #include <linux/compiler_types.h>
> >   #include <linux/container_of.h>
> > @@ -259,4 +260,45 @@ void mtk_clk_unregister_muxes(const struct mtk_mux *muxes, int num,
> >   }
> >   EXPORT_SYMBOL_GPL(mtk_clk_unregister_muxes);
> >
> > +/*
> > + * This clock notifier is called when the frequency of the of the parent
> > + * PLL clock is to be changed. The idea is to switch the parent to a
> > + * stable clock, such as the main oscillator, while the PLL frequency
> > + * stabilizes.
> > + */
> > +static int mtk_clk_mux_notifier_cb(struct notifier_block *nb,
> > +                                unsigned long event, void *_data)
> > +{
> > +     struct clk_notifier_data *data = _data;
> > +     struct mtk_mux_nb *mux_nb = to_mtk_mux_nb(nb);
> > +     const struct mtk_mux *mux = mux_nb->mux;
> > +     struct clk_hw *hw;
> > +     int ret = 0;
> > +
> > +     hw = __clk_get_hw(data->clk);
> > +
> > +     switch (event) {
> > +     case PRE_RATE_CHANGE:
> > +             mux_nb->original_index = mux->ops->get_parent(hw);
> > +             ret = mux->ops->set_parent(hw, mux_nb->bypass_index);
> > +             break;
> > +
> > +     case POST_RATE_CHANGE:
> > +     case ABORT_RATE_CHANGE:
>
> I agree with this change, entirely - but there's an issue here.
> If we enter ABORT_RATE_CHANGE, this means that "something has failed": now,
> what if the failure point was the PLL being unable to lock?

I think this is a valid concern. But the MediaTek clk driver library doesn't
actually check if the PLL locked or not. It simply sets it and returns.

Also, if the notifier weren't there, there'd be no transparent muxing
either, and the GPU would always be clocked from the PLL, locked or
not.

Considering that muxing away is temporary and transparent, muxing back
simply restores the system to the way the CCF thinks it is. If the PLL
fails to lock, we need other fixes or constraints.

> In that case, we would switch the parent back to a PLL that's not outputting
> any clock, crashing the GPU, or a bogus rate, potentially undervolting the GPU.

For MT8183, there is no other "safe" PLL. The stable ones run faster than
the lowest OPP.

> I think that the best idea here would be to do something like..
>
>         switch (event) {
>         case PRE_RATE_CHANGE:
>                 mux_nb->old_parent_idx = mux->ops->get_parent(hw);
>                 ret = mux->ops->set_parent(hw, mux_nb->safe_parent_idx);
>                 break;
>         case POST_RATE_CHANGE:
>                 ret = mux->ops->set_parent(hw, mux_nb->old_parent_idx);
>                 break;
>         case ABORT_RATE_CHANGE:
>                 ret = -EINVAL; /* or -ECANCELED, whatever... */
>                 break;
>         }
>
> > +             ret = mux->ops->set_parent(hw, mux_nb->original_index);
> > +             break;
> > +     }
> > +
> > +     return notifier_from_errno(ret);
> > +}
> > +
> > +int devm_mtk_clk_mux_notifier_register(struct device *dev, struct clk *clk,
> > +                                    struct mtk_mux_nb *mux_nb)
> > +{
> > +     mux_nb->nb.notifier_call = mtk_clk_mux_notifier_cb;
> > +
> > +     return devm_clk_notifier_register(dev, clk, &mux_nb->nb);
> > +}
> > +EXPORT_SYMBOL_GPL(devm_mtk_clk_mux_notifier_register);
> > +
> >   MODULE_LICENSE("GPL");
> > diff --git a/drivers/clk/mediatek/clk-mux.h b/drivers/clk/mediatek/clk-mux.h
> > index 6539c58f5d7d..506e91125a3d 100644
> > --- a/drivers/clk/mediatek/clk-mux.h
> > +++ b/drivers/clk/mediatek/clk-mux.h
> > @@ -7,12 +7,14 @@
> >   #ifndef __DRV_CLK_MTK_MUX_H
> >   #define __DRV_CLK_MTK_MUX_H
> >
> > +#include <linux/notifier.h>
> >   #include <linux/spinlock.h>
> >   #include <linux/types.h>
> >
> >   struct clk;
> >   struct clk_hw_onecell_data;
> >   struct clk_ops;
> > +struct device;
> >   struct device_node;
> >
> >   struct mtk_mux {
> > @@ -89,4 +91,17 @@ int mtk_clk_register_muxes(const struct mtk_mux *muxes,
> >   void mtk_clk_unregister_muxes(const struct mtk_mux *muxes, int num,
> >                             struct clk_hw_onecell_data *clk_data);
> >
> > +struct mtk_mux_nb {
> > +     struct notifier_block   nb;
> > +     const struct mtk_mux    *mux;
> > +
> > +     u8      bypass_index;   /* Which parent to temporarily use */
> > +     u8      original_index; /* Set by notifier callback */
>
> I think that the following names are more explanatory:
>
>         u8      safe_parent_idx;
>         u8      old_parent_idx;
>
> ...because I see this as a mechanism to switch the mux to a "safe" clock output
> and then back to the PLL (like it's done on some qcom clocks as well).

The names were taken from the sunxi-ng driver, which always muxed to a
crystal clock, so in a way was bypassing the PLL.

ChenYu

> You're free to ignore this comment, as this is, of course, just a personal opinion.
>
> Cheers,
> Angelo
>
>

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v2 3/4] clk: mediatek: mux: add clk notifier functions

[Chen-Yu Tsai]

On Mon, May 23, 2022 at 6:04 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 23/05/22 10:59, Chen-Yu Tsai ha scritto:
> > With device frequency scaling, the mux clock that (indirectly) feeds the
> > device selects between a dedicated PLL, and some other stable clocks.
> >
> > When a clk rate change is requested, the (normally) upstream PLL is
> > reconfigured. It's possible for the clock output of the PLL to become
> > unstable during this process.
> >
> > To avoid causing the device to glitch, the mux should temporarily be
> > switched over to another "stable" clock during the PLL rate change.
> > This is done with clk notifiers.
> >
> > This patch adds common functions for notifiers to temporarily and
> > transparently reparent mux clocks.
> >
> > This was loosely based on commit 8adfb08605a9 ("clk: sunxi-ng: mux: Add
> > clk notifier functions").
> >
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> >   drivers/clk/mediatek/clk-mux.c | 42 ++++++++++++++++++++++++++++++++++
> >   drivers/clk/mediatek/clk-mux.h | 15 ++++++++++++
> >   2 files changed, 57 insertions(+)
> >
> > diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c
> > index cd5f9fd8cb98..f84a5a753c09 100644
> > --- a/drivers/clk/mediatek/clk-mux.c
> > +++ b/drivers/clk/mediatek/clk-mux.c
> > @@ -4,6 +4,7 @@
> >    * Author: Owen Chen <owen.chen@mediatek.com>
> >    */
> >
> > +#include <linux/clk.h>
> >   #include <linux/clk-provider.h>
> >   #include <linux/compiler_types.h>
> >   #include <linux/container_of.h>
> > @@ -259,4 +260,45 @@ void mtk_clk_unregister_muxes(const struct mtk_mux *muxes, int num,
> >   }
> >   EXPORT_SYMBOL_GPL(mtk_clk_unregister_muxes);
> >
> > +/*
> > + * This clock notifier is called when the frequency of the of the parent
> > + * PLL clock is to be changed. The idea is to switch the parent to a
> > + * stable clock, such as the main oscillator, while the PLL frequency
> > + * stabilizes.
> > + */
> > +static int mtk_clk_mux_notifier_cb(struct notifier_block *nb,
> > +                                unsigned long event, void *_data)
> > +{
> > +     struct clk_notifier_data *data = _data;
> > +     struct mtk_mux_nb *mux_nb = to_mtk_mux_nb(nb);
> > +     const struct mtk_mux *mux = mux_nb->mux;
> > +     struct clk_hw *hw;
> > +     int ret = 0;
> > +
> > +     hw = __clk_get_hw(data->clk);
> > +
> > +     switch (event) {
> > +     case PRE_RATE_CHANGE:
> > +             mux_nb->original_index = mux->ops->get_parent(hw);
> > +             ret = mux->ops->set_parent(hw, mux_nb->bypass_index);
> > +             break;
> > +
> > +     case POST_RATE_CHANGE:
> > +     case ABORT_RATE_CHANGE:
>
> I agree with this change, entirely - but there's an issue here.
> If we enter ABORT_RATE_CHANGE, this means that "something has failed": now,
> what if the failure point was the PLL being unable to lock?

I think this is a valid concern. But the MediaTek clk driver library doesn't
actually check if the PLL locked or not. It simply sets it and returns.

Also, if the notifier weren't there, there'd be no transparent muxing
either, and the GPU would always be clocked from the PLL, locked or
not.

Considering that muxing away is temporary and transparent, muxing back
simply restores the system to the way the CCF thinks it is. If the PLL
fails to lock, we need other fixes or constraints.

> In that case, we would switch the parent back to a PLL that's not outputting
> any clock, crashing the GPU, or a bogus rate, potentially undervolting the GPU.

For MT8183, there is no other "safe" PLL. The stable ones run faster than
the lowest OPP.

> I think that the best idea here would be to do something like..
>
>         switch (event) {
>         case PRE_RATE_CHANGE:
>                 mux_nb->old_parent_idx = mux->ops->get_parent(hw);
>                 ret = mux->ops->set_parent(hw, mux_nb->safe_parent_idx);
>                 break;
>         case POST_RATE_CHANGE:
>                 ret = mux->ops->set_parent(hw, mux_nb->old_parent_idx);
>                 break;
>         case ABORT_RATE_CHANGE:
>                 ret = -EINVAL; /* or -ECANCELED, whatever... */
>                 break;
>         }
>
> > +             ret = mux->ops->set_parent(hw, mux_nb->original_index);
> > +             break;
> > +     }
> > +
> > +     return notifier_from_errno(ret);
> > +}
> > +
> > +int devm_mtk_clk_mux_notifier_register(struct device *dev, struct clk *clk,
> > +                                    struct mtk_mux_nb *mux_nb)
> > +{
> > +     mux_nb->nb.notifier_call = mtk_clk_mux_notifier_cb;
> > +
> > +     return devm_clk_notifier_register(dev, clk, &mux_nb->nb);
> > +}
> > +EXPORT_SYMBOL_GPL(devm_mtk_clk_mux_notifier_register);
> > +
> >   MODULE_LICENSE("GPL");
> > diff --git a/drivers/clk/mediatek/clk-mux.h b/drivers/clk/mediatek/clk-mux.h
> > index 6539c58f5d7d..506e91125a3d 100644
> > --- a/drivers/clk/mediatek/clk-mux.h
> > +++ b/drivers/clk/mediatek/clk-mux.h
> > @@ -7,12 +7,14 @@
> >   #ifndef __DRV_CLK_MTK_MUX_H
> >   #define __DRV_CLK_MTK_MUX_H
> >
> > +#include <linux/notifier.h>
> >   #include <linux/spinlock.h>
> >   #include <linux/types.h>
> >
> >   struct clk;
> >   struct clk_hw_onecell_data;
> >   struct clk_ops;
> > +struct device;
> >   struct device_node;
> >
> >   struct mtk_mux {
> > @@ -89,4 +91,17 @@ int mtk_clk_register_muxes(const struct mtk_mux *muxes,
> >   void mtk_clk_unregister_muxes(const struct mtk_mux *muxes, int num,
> >                             struct clk_hw_onecell_data *clk_data);
> >
> > +struct mtk_mux_nb {
> > +     struct notifier_block   nb;
> > +     const struct mtk_mux    *mux;
> > +
> > +     u8      bypass_index;   /* Which parent to temporarily use */
> > +     u8      original_index; /* Set by notifier callback */
>
> I think that the following names are more explanatory:
>
>         u8      safe_parent_idx;
>         u8      old_parent_idx;
>
> ...because I see this as a mechanism to switch the mux to a "safe" clock output
> and then back to the PLL (like it's done on some qcom clocks as well).

The names were taken from the sunxi-ng driver, which always muxed to a
crystal clock, so in a way was bypassing the PLL.

ChenYu

> You're free to ignore this comment, as this is, of course, just a personal opinion.
>
> Cheers,
> Angelo
>
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel