[PATCH 0/2] MediaTek clocks: Support mux indices list and 8195 DP

[PATCH 0/2] MediaTek clocks: Support mux indices list and 8195 DP

[AngeloGioacchino Del Regno]

This series adds support to specify custom parent indices for MediaTek
MUX clocks, necessary to avoid setting the same parent PLL for MT8195's
top_dp and top_edp clocks, solving DP+eDP concurrent output issues.

No fixes tags are provided as the clk-mux commit introduces new logic
and the actual MT8195 fix depends on that.

This commit was tested on the Acer Tomato Chromebook (MT8195) with
dual concurrent display outputs (internal eDP panel and TypeC->DP->HDMI
adapter connected to Samsung UE40JU6400 4k TV); resolution switch on
DP was also tested; eDP output is not paused and internal display keeps
working as expected.

AngeloGioacchino Del Regno (2):
  clk: mediatek: clk-mux: Support custom parent indices for muxes
  clk: mediatek: mt8195-topckgen: Refactor parents for top_dp/edp muxes

 drivers/clk/mediatek/clk-mt8195-topckgen.c | 22 +++++++----
 drivers/clk/mediatek/clk-mux.c             | 14 +++++++
 drivers/clk/mediatek/clk-mux.h             | 43 ++++++++++++++++++++--
 3 files changed, 67 insertions(+), 12 deletions(-)

-- 
2.40.1


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

[PATCH 1/2] clk: mediatek: clk-mux: Support custom parent indices for muxes

[AngeloGioacchino Del Regno]

Add support for customized parent indices for MediaTek muxes: this is
necessary for the case in which we want to exclude some clocks from
a mux's parent clocks list, where the exclusions are not from the
very bottom of the list but either in the middle or the beginning.

Example:
- MUX1 (all parents)
  - parent1; idx=0
  - parent2; idx=1
  - parent3; idx=2

- MUX1 (wanted parents)
  - parent1; idx=0
  - parent3; idx=2

To achieve that add a `parent_index` array pointer to struct mtk_mux,
then in .set_parent(), .get_parent() callbacks check if this array
was populated and eventually get the index from that.

Also, to avoid updating all clock drivers for all SoCs, rename the
"main" macro to __GATE_CLR_SET_UPD_FLAGS (so, `__` was added) and
add the new member to it; furthermore, GATE_CLK_SET_UPD_FLAGS has
been reintroduced as being fully compatible with the older version.

The new parent_index can be specified with the new `_INDEXED`
variants of the MUX_GATE_CLR_SET_UPD_xxxx macros.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/clk/mediatek/clk-mux.c | 14 +++++++++++
 drivers/clk/mediatek/clk-mux.h | 43 ++++++++++++++++++++++++++++++----
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c
index c93bc7f926e5..60990296450b 100644
--- a/drivers/clk/mediatek/clk-mux.c
+++ b/drivers/clk/mediatek/clk-mux.c
@@ -89,6 +89,17 @@ static u8 mtk_clk_mux_get_parent(struct clk_hw *hw)
 	regmap_read(mux->regmap, mux->data->mux_ofs, &val);
 	val = (val >> mux->data->mux_shift) & mask;
 
+	if (mux->data->parent_index) {
+		int i;
+
+		for (i = 0; i < mux->data->num_parents; i++)
+			if (mux->data->parent_index[i] == val)
+				return i;
+
+		/* Not found: return an impossible index to generate error */
+		return mux->data->num_parents + 1;
+	}
+
 	return val;
 }
 
@@ -104,6 +115,9 @@ static int mtk_clk_mux_set_parent_setclr_lock(struct clk_hw *hw, u8 index)
 	else
 		__acquire(mux->lock);
 
+	if (mux->data->parent_index)
+		index = mux->data->parent_index[index];
+
 	regmap_read(mux->regmap, mux->data->mux_ofs, &orig);
 	val = (orig & ~(mask << mux->data->mux_shift))
 			| (index << mux->data->mux_shift);
diff --git a/drivers/clk/mediatek/clk-mux.h b/drivers/clk/mediatek/clk-mux.h
index 7ecb963b0ec6..943ad1d7ce4b 100644
--- a/drivers/clk/mediatek/clk-mux.h
+++ b/drivers/clk/mediatek/clk-mux.h
@@ -21,6 +21,7 @@ struct mtk_mux {
 	int id;
 	const char *name;
 	const char * const *parent_names;
+	const u8 *parent_index;
 	unsigned int flags;
 
 	u32 mux_ofs;
@@ -37,9 +38,10 @@ struct mtk_mux {
 	signed char num_parents;
 };
 
-#define GATE_CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs,		\
-			_mux_set_ofs, _mux_clr_ofs, _shift, _width,	\
-			_gate, _upd_ofs, _upd, _flags, _ops) {		\
+#define __GATE_CLR_SET_UPD_FLAGS(_id, _name, _parents, _paridx,		\
+			 _num_parents, _mux_ofs, _mux_set_ofs,		\
+			 _mux_clr_ofs, _shift, _width, _gate, _upd_ofs,	\
+			 _upd, _flags, _ops) {				\
 		.id = _id,						\
 		.name = _name,						\
 		.mux_ofs = _mux_ofs,					\
@@ -51,11 +53,28 @@ struct mtk_mux {
 		.gate_shift = _gate,					\
 		.upd_shift = _upd,					\
 		.parent_names = _parents,				\
-		.num_parents = ARRAY_SIZE(_parents),			\
+		.parent_index = _paridx,				\
+		.num_parents = _num_parents,				\
 		.flags = _flags,					\
 		.ops = &_ops,						\
 	}
 
+#define GATE_CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs,		\
+			_mux_set_ofs, _mux_clr_ofs, _shift, _width,	\
+			_gate, _upd_ofs, _upd, _flags, _ops)		\
+		__GATE_CLR_SET_UPD_FLAGS(_id, _name, _parents,		\
+			NULL, ARRAY_SIZE(_parents), _mux_ofs,		\
+			_mux_set_ofs, _mux_clr_ofs, _shift, _width,	\
+			_gate, _upd_ofs, _upd, _flags, _ops)		\
+
+#define GATE_CLR_SET_UPD_FLAGS_INDEXED(_id, _name, _parents, _paridx,	\
+			 _mux_ofs, _mux_set_ofs, _mux_clr_ofs, _shift,	\
+			 _width, _gate, _upd_ofs, _upd, _flags, _ops)	\
+		__GATE_CLR_SET_UPD_FLAGS(_id, _name, _parents,		\
+			_paridx, ARRAY_SIZE(_paridx), _mux_ofs,		\
+			_mux_set_ofs, _mux_clr_ofs, _shift, _width,	\
+			_gate, _upd_ofs, _upd, _flags, _ops)		\
+
 extern const struct clk_ops mtk_mux_clr_set_upd_ops;
 extern const struct clk_ops mtk_mux_gate_clr_set_upd_ops;
 
@@ -67,6 +86,14 @@ extern const struct clk_ops mtk_mux_gate_clr_set_upd_ops;
 			_gate, _upd_ofs, _upd, _flags,			\
 			mtk_mux_gate_clr_set_upd_ops)
 
+#define MUX_GATE_CLR_SET_UPD_FLAGS_INDEXED(_id, _name, _parents,	\
+			_paridx, _mux_ofs, _mux_set_ofs, _mux_clr_ofs,	\
+			_shift, _width, _gate, _upd_ofs, _upd, _flags)	\
+		GATE_CLR_SET_UPD_FLAGS_INDEXED(_id, _name, _parents,	\
+			_paridx, _mux_ofs, _mux_set_ofs, _mux_clr_ofs,	\
+			_shift, _width, _gate, _upd_ofs, _upd, _flags,	\
+			mtk_mux_gate_clr_set_upd_ops)
+
 #define MUX_GATE_CLR_SET_UPD(_id, _name, _parents, _mux_ofs,		\
 			_mux_set_ofs, _mux_clr_ofs, _shift, _width,	\
 			_gate, _upd_ofs, _upd)				\
@@ -75,6 +102,14 @@ extern const struct clk_ops mtk_mux_gate_clr_set_upd_ops;
 			_width, _gate, _upd_ofs, _upd,			\
 			CLK_SET_RATE_PARENT)
 
+#define MUX_GATE_CLR_SET_UPD_INDEXED(_id, _name, _parents, _paridx,	\
+			_mux_ofs, _mux_set_ofs, _mux_clr_ofs, _shift,	\
+			_width, _gate, _upd_ofs, _upd)			\
+		MUX_GATE_CLR_SET_UPD_FLAGS_INDEXED(_id, _name,		\
+			_parents, _paridx, _mux_ofs, _mux_set_ofs,	\
+			_mux_clr_ofs, _shift, _width, _gate, _upd_ofs,	\
+			_upd, CLK_SET_RATE_PARENT)
+
 #define MUX_CLR_SET_UPD(_id, _name, _parents, _mux_ofs,			\
 			_mux_set_ofs, _mux_clr_ofs, _shift, _width,	\
 			_upd_ofs, _upd)					\
-- 
2.40.1


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

[PATCH 2/2] clk: mediatek: mt8195-topckgen: Refactor parents for top_dp/edp muxes

[AngeloGioacchino Del Regno]

The top_dp and top_edp muxes can be both parented to either TVDPLL1
or TVDPLL2, two identically specced PLLs for the specific purpose of
giving out pixel clock: this becomes a problem when the MediaTek
DisplayPort Interface (DPI) driver tries to set the pixel clock rate.

In the usecase of two simultaneous outputs (using two controllers),
it was seen that one of the displays would sometimes display garbled
output (if any at all) and this was because:
 - top_edp was set to TVDPLL1, outputting X GHz
 - top_dp was set to TVDPLL2, outputting Y GHz
   - mtk_dpi calls clk_set_rate(top_edp, Z GHz)
     - top_dp is switched to TVDPLL1
     - TVDPLL1 changes its rate, top_edp outputs the wrong rate.
     - eDP display is garbled

To solve this issue, remove all TVDPLL1 parents from `top_dp` and
all TVDPLL2 parents from `top_edp`, plus, necessarily switch both
clocks to use the new MUX_GATE_CLR_SET_UPD_INDEXED() macro to be
able to use the right bit index for the new parents list.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/clk/mediatek/clk-mt8195-topckgen.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt8195-topckgen.c b/drivers/clk/mediatek/clk-mt8195-topckgen.c
index 81daa24cadde..abb3721f6e1b 100644
--- a/drivers/clk/mediatek/clk-mt8195-topckgen.c
+++ b/drivers/clk/mediatek/clk-mt8195-topckgen.c
@@ -417,15 +417,21 @@ static const char * const pwrmcu_parents[] = {
 
 static const char * const dp_parents[] = {
 	"clk26m",
-	"tvdpll1_d2",
 	"tvdpll2_d2",
-	"tvdpll1_d4",
 	"tvdpll2_d4",
-	"tvdpll1_d8",
 	"tvdpll2_d8",
-	"tvdpll1_d16",
 	"tvdpll2_d16"
 };
+static const u8 dp_parents_idx[] = { 0, 2, 4, 6, 8 };
+
+static const char * const edp_parents[] = {
+	"clk26m",
+	"tvdpll1_d2",
+	"tvdpll1_d4",
+	"tvdpll1_d8",
+	"tvdpll1_d16"
+};
+static const u8 edp_parents_idx[] = { 0, 1, 3, 5, 7 };
 
 static const char * const disp_pwm_parents[] = {
 	"clk26m",
@@ -957,11 +963,11 @@ static const struct mtk_mux top_mtk_muxes[] = {
 	MUX_GATE_CLR_SET_UPD_FLAGS(CLK_TOP_PWRMCU, "top_pwrmcu",
 		pwrmcu_parents, 0x08C, 0x090, 0x094, 16, 3, 23, 0x08, 6,
 		CLK_IS_CRITICAL | CLK_SET_RATE_PARENT),
-	MUX_GATE_CLR_SET_UPD(CLK_TOP_DP, "top_dp",
-		dp_parents, 0x08C, 0x090, 0x094, 24, 4, 31, 0x08, 7),
+	MUX_GATE_CLR_SET_UPD_INDEXED(CLK_TOP_DP, "top_dp",
+		dp_parents, dp_parents_idx, 0x08C, 0x090, 0x094, 24, 4, 31, 0x08, 7),
 	/* CLK_CFG_10 */
-	MUX_GATE_CLR_SET_UPD(CLK_TOP_EDP, "top_edp",
-		dp_parents, 0x098, 0x09C, 0x0A0, 0, 4, 7, 0x08, 8),
+	MUX_GATE_CLR_SET_UPD_INDEXED(CLK_TOP_EDP, "top_edp",
+		edp_parents, edp_parents_idx, 0x098, 0x09C, 0x0A0, 0, 4, 7, 0x08, 8),
 	MUX_GATE_CLR_SET_UPD(CLK_TOP_DPI, "top_dpi",
 		dp_parents, 0x098, 0x09C, 0x0A0, 8, 4, 15, 0x08, 9),
 	MUX_GATE_CLR_SET_UPD(CLK_TOP_DISP_PWM0, "top_disp_pwm0",
-- 
2.40.1


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

Re: [PATCH 2/2] clk: mediatek: mt8195-topckgen: Refactor parents for top_dp/edp muxes

[Alexandre Mergnat]

On 13/07/2023 09:21, AngeloGioacchino Del Regno wrote:
> The top_dp and top_edp muxes can be both parented to either TVDPLL1
> or TVDPLL2, two identically specced PLLs for the specific purpose of
> giving out pixel clock: this becomes a problem when the MediaTek
> DisplayPort Interface (DPI) driver tries to set the pixel clock rate.
> 
> In the usecase of two simultaneous outputs (using two controllers),
> it was seen that one of the displays would sometimes display garbled
> output (if any at all) and this was because:
>   - top_edp was set to TVDPLL1, outputting X GHz
>   - top_dp was set to TVDPLL2, outputting Y GHz
>     - mtk_dpi calls clk_set_rate(top_edp, Z GHz)
>       - top_dp is switched to TVDPLL1
>       - TVDPLL1 changes its rate, top_edp outputs the wrong rate.
>       - eDP display is garbled
> 
> To solve this issue, remove all TVDPLL1 parents from `top_dp` and
> all TVDPLL2 parents from `top_edp`, plus, necessarily switch both
> clocks to use the new MUX_GATE_CLR_SET_UPD_INDEXED() macro to be
> able to use the right bit index for the new parents list.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/clk/mediatek/clk-mt8195-topckgen.c | 22 ++++++++++++++--------
>   1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/mediatek/clk-mt8195-topckgen.c b/drivers/clk/mediatek/clk-mt8195-topckgen.c
> index 81daa24cadde..abb3721f6e1b 100644
> --- a/drivers/clk/mediatek/clk-mt8195-topckgen.c
> +++ b/drivers/clk/mediatek/clk-mt8195-topckgen.c
> @@ -417,15 +417,21 @@ static const char * const pwrmcu_parents[] = {
>   
>   static const char * const dp_parents[] = {
>   	"clk26m",
> -	"tvdpll1_d2",
>   	"tvdpll2_d2",
> -	"tvdpll1_d4",
>   	"tvdpll2_d4",
> -	"tvdpll1_d8",
>   	"tvdpll2_d8",
> -	"tvdpll1_d16",
>   	"tvdpll2_d16"
>   };
> +static const u8 dp_parents_idx[] = { 0, 2, 4, 6, 8 };
> +
> +static const char * const edp_parents[] = {
> +	"clk26m",
> +	"tvdpll1_d2",
> +	"tvdpll1_d4",
> +	"tvdpll1_d8",
> +	"tvdpll1_d16"
> +};
> +static const u8 edp_parents_idx[] = { 0, 1, 3, 5, 7 };

AFAII your solution is to force a specific TVDPLLX for each display, and 
it isn't dynamic.

Do you think it's possible to do that using the DTS ? I'm asking 
because, IMHO, this kind of setup is more friendly/readable/flexible in 
the DTS than hardcoded into the driver.

>   
>   static const char * const disp_pwm_parents[] = {
>   	"clk26m",
> @@ -957,11 +963,11 @@ static const struct mtk_mux top_mtk_muxes[] = {
>   	MUX_GATE_CLR_SET_UPD_FLAGS(CLK_TOP_PWRMCU, "top_pwrmcu",
>   		pwrmcu_parents, 0x08C, 0x090, 0x094, 16, 3, 23, 0x08, 6,
>   		CLK_IS_CRITICAL | CLK_SET_RATE_PARENT),
> -	MUX_GATE_CLR_SET_UPD(CLK_TOP_DP, "top_dp",
> -		dp_parents, 0x08C, 0x090, 0x094, 24, 4, 31, 0x08, 7),
> +	MUX_GATE_CLR_SET_UPD_INDEXED(CLK_TOP_DP, "top_dp",
> +		dp_parents, dp_parents_idx, 0x08C, 0x090, 0x094, 24, 4, 31, 0x08, 7),
>   	/* CLK_CFG_10 */
> -	MUX_GATE_CLR_SET_UPD(CLK_TOP_EDP, "top_edp",
> -		dp_parents, 0x098, 0x09C, 0x0A0, 0, 4, 7, 0x08, 8),
> +	MUX_GATE_CLR_SET_UPD_INDEXED(CLK_TOP_EDP, "top_edp",
> +		edp_parents, edp_parents_idx, 0x098, 0x09C, 0x0A0, 0, 4, 7, 0x08, 8),
>   	MUX_GATE_CLR_SET_UPD(CLK_TOP_DPI, "top_dpi",
>   		dp_parents, 0x098, 0x09C, 0x0A0, 8, 4, 15, 0x08, 9),
>   	MUX_GATE_CLR_SET_UPD(CLK_TOP_DISP_PWM0, "top_disp_pwm0",

-- 
Regards,
Alexandre

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

Re: [PATCH 2/2] clk: mediatek: mt8195-topckgen: Refactor parents for top_dp/edp muxes

[Chen-Yu Tsai]

On Thu, Jul 13, 2023 at 9:22 PM Alexandre Mergnat <amergnat@baylibre.com> wrote:
> On 13/07/2023 09:21, AngeloGioacchino Del Regno wrote:
> > The top_dp and top_edp muxes can be both parented to either TVDPLL1
> > or TVDPLL2, two identically specced PLLs for the specific purpose of
> > giving out pixel clock: this becomes a problem when the MediaTek
> > DisplayPort Interface (DPI) driver tries to set the pixel clock rate.
> >
> > In the usecase of two simultaneous outputs (using two controllers),
> > it was seen that one of the displays would sometimes display garbled
> > output (if any at all) and this was because:
> >   - top_edp was set to TVDPLL1, outputting X GHz
> >   - top_dp was set to TVDPLL2, outputting Y GHz
> >     - mtk_dpi calls clk_set_rate(top_edp, Z GHz)
> >       - top_dp is switched to TVDPLL1
> >       - TVDPLL1 changes its rate, top_edp outputs the wrong rate.
> >       - eDP display is garbled
> >
> > To solve this issue, remove all TVDPLL1 parents from `top_dp` and
> > all TVDPLL2 parents from `top_edp`, plus, necessarily switch both
> > clocks to use the new MUX_GATE_CLR_SET_UPD_INDEXED() macro to be
> > able to use the right bit index for the new parents list.
> >
> > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > ---
> >   drivers/clk/mediatek/clk-mt8195-topckgen.c | 22 ++++++++++++++--------
> >   1 file changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/clk/mediatek/clk-mt8195-topckgen.c b/drivers/clk/mediatek/clk-mt8195-topckgen.c
> > index 81daa24cadde..abb3721f6e1b 100644
> > --- a/drivers/clk/mediatek/clk-mt8195-topckgen.c
> > +++ b/drivers/clk/mediatek/clk-mt8195-topckgen.c
> > @@ -417,15 +417,21 @@ static const char * const pwrmcu_parents[] = {
> >
> >   static const char * const dp_parents[] = {
> >       "clk26m",
> > -     "tvdpll1_d2",
> >       "tvdpll2_d2",
> > -     "tvdpll1_d4",
> >       "tvdpll2_d4",
> > -     "tvdpll1_d8",
> >       "tvdpll2_d8",
> > -     "tvdpll1_d16",
> >       "tvdpll2_d16"
> >   };
> > +static const u8 dp_parents_idx[] = { 0, 2, 4, 6, 8 };
> > +
> > +static const char * const edp_parents[] = {
> > +     "clk26m",
> > +     "tvdpll1_d2",
> > +     "tvdpll1_d4",
> > +     "tvdpll1_d8",
> > +     "tvdpll1_d16"
> > +};
> > +static const u8 edp_parents_idx[] = { 0, 1, 3, 5, 7 };
>
> AFAII your solution is to force a specific TVDPLLX for each display, and
> it isn't dynamic.
>
> Do you think it's possible to do that using the DTS ? I'm asking
> because, IMHO, this kind of setup is more friendly/readable/flexible in
> the DTS than hardcoded into the driver.

(CC-ing Maxime, who has some experience in the matter.)

assigned-parents doesn't prevent your system from reparenting the clocks
back to a conflicting configuration.

AFAIK the recommended way to deal with this is to use
clk_set_rate_exclusive() and co. in whatever consumer driver that needs
exclusive control on the clock rate. However I'm not sure if that works
for parents. It should, given the original use case was for the sunxi
platforms, which like the MediaTek platform here has 2 PLLs for video
related consumers, but I couldn't find code verifying it.


ChenYu

> >
> >   static const char * const disp_pwm_parents[] = {
> >       "clk26m",
> > @@ -957,11 +963,11 @@ static const struct mtk_mux top_mtk_muxes[] = {
> >       MUX_GATE_CLR_SET_UPD_FLAGS(CLK_TOP_PWRMCU, "top_pwrmcu",
> >               pwrmcu_parents, 0x08C, 0x090, 0x094, 16, 3, 23, 0x08, 6,
> >               CLK_IS_CRITICAL | CLK_SET_RATE_PARENT),
> > -     MUX_GATE_CLR_SET_UPD(CLK_TOP_DP, "top_dp",
> > -             dp_parents, 0x08C, 0x090, 0x094, 24, 4, 31, 0x08, 7),
> > +     MUX_GATE_CLR_SET_UPD_INDEXED(CLK_TOP_DP, "top_dp",
> > +             dp_parents, dp_parents_idx, 0x08C, 0x090, 0x094, 24, 4, 31, 0x08, 7),
> >       /* CLK_CFG_10 */
> > -     MUX_GATE_CLR_SET_UPD(CLK_TOP_EDP, "top_edp",
> > -             dp_parents, 0x098, 0x09C, 0x0A0, 0, 4, 7, 0x08, 8),
> > +     MUX_GATE_CLR_SET_UPD_INDEXED(CLK_TOP_EDP, "top_edp",
> > +             edp_parents, edp_parents_idx, 0x098, 0x09C, 0x0A0, 0, 4, 7, 0x08, 8),
> >       MUX_GATE_CLR_SET_UPD(CLK_TOP_DPI, "top_dpi",
> >               dp_parents, 0x098, 0x09C, 0x0A0, 8, 4, 15, 0x08, 9),
> >       MUX_GATE_CLR_SET_UPD(CLK_TOP_DISP_PWM0, "top_disp_pwm0",
>
> --
> Regards,
> Alexandre

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

Re: [PATCH 2/2] clk: mediatek: mt8195-topckgen: Refactor parents for top_dp/edp muxes

[AngeloGioacchino Del Regno]

Il 13/07/23 15:22, Alexandre Mergnat ha scritto:
> 
> 
> On 13/07/2023 09:21, AngeloGioacchino Del Regno wrote:
>> The top_dp and top_edp muxes can be both parented to either TVDPLL1
>> or TVDPLL2, two identically specced PLLs for the specific purpose of
>> giving out pixel clock: this becomes a problem when the MediaTek
>> DisplayPort Interface (DPI) driver tries to set the pixel clock rate.
>>
>> In the usecase of two simultaneous outputs (using two controllers),
>> it was seen that one of the displays would sometimes display garbled
>> output (if any at all) and this was because:
>>   - top_edp was set to TVDPLL1, outputting X GHz
>>   - top_dp was set to TVDPLL2, outputting Y GHz
>>     - mtk_dpi calls clk_set_rate(top_edp, Z GHz)
>>       - top_dp is switched to TVDPLL1
>>       - TVDPLL1 changes its rate, top_edp outputs the wrong rate.
>>       - eDP display is garbled
>>
>> To solve this issue, remove all TVDPLL1 parents from `top_dp` and
>> all TVDPLL2 parents from `top_edp`, plus, necessarily switch both
>> clocks to use the new MUX_GATE_CLR_SET_UPD_INDEXED() macro to be
>> able to use the right bit index for the new parents list.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/clk/mediatek/clk-mt8195-topckgen.c | 22 ++++++++++++++--------
>>   1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/clk/mediatek/clk-mt8195-topckgen.c 
>> b/drivers/clk/mediatek/clk-mt8195-topckgen.c
>> index 81daa24cadde..abb3721f6e1b 100644
>> --- a/drivers/clk/mediatek/clk-mt8195-topckgen.c
>> +++ b/drivers/clk/mediatek/clk-mt8195-topckgen.c
>> @@ -417,15 +417,21 @@ static const char * const pwrmcu_parents[] = {
>>   static const char * const dp_parents[] = {
>>       "clk26m",
>> -    "tvdpll1_d2",
>>       "tvdpll2_d2",
>> -    "tvdpll1_d4",
>>       "tvdpll2_d4",
>> -    "tvdpll1_d8",
>>       "tvdpll2_d8",
>> -    "tvdpll1_d16",
>>       "tvdpll2_d16"
>>   };
>> +static const u8 dp_parents_idx[] = { 0, 2, 4, 6, 8 };
>> +
>> +static const char * const edp_parents[] = {
>> +    "clk26m",
>> +    "tvdpll1_d2",
>> +    "tvdpll1_d4",
>> +    "tvdpll1_d8",
>> +    "tvdpll1_d16"
>> +};
>> +static const u8 edp_parents_idx[] = { 0, 1, 3, 5, 7 };
> 
> AFAII your solution is to force a specific TVDPLLX for each display, and it isn't 
> dynamic.
> 
> Do you think it's possible to do that using the DTS ? I'm asking because, IMHO, 
> this kind of setup is more friendly/readable/flexible in the DTS than hardcoded 
> into the driver.
> 

No, there's no way. In DT you can assign one specific parent to a specific clock,
but we need to dynamically switch between the TVDPLL dividers with clk_set_rate()
calls.

Besides, can you please explain why you're worried about having TVDPLL1 on DP
instead of eDP and vice-versa?
The two PLLs are powered from the same power domain and are identical in spec,
so one or the other doesn't make any difference... you could use TVDPLL2 while
TVDPLL1 is OFF and vice-versa too.

Cheers,
Angelo

>>   static const char * const disp_pwm_parents[] = {
>>       "clk26m",
>> @@ -957,11 +963,11 @@ static const struct mtk_mux top_mtk_muxes[] = {
>>       MUX_GATE_CLR_SET_UPD_FLAGS(CLK_TOP_PWRMCU, "top_pwrmcu",
>>           pwrmcu_parents, 0x08C, 0x090, 0x094, 16, 3, 23, 0x08, 6,
>>           CLK_IS_CRITICAL | CLK_SET_RATE_PARENT),
>> -    MUX_GATE_CLR_SET_UPD(CLK_TOP_DP, "top_dp",
>> -        dp_parents, 0x08C, 0x090, 0x094, 24, 4, 31, 0x08, 7),
>> +    MUX_GATE_CLR_SET_UPD_INDEXED(CLK_TOP_DP, "top_dp",
>> +        dp_parents, dp_parents_idx, 0x08C, 0x090, 0x094, 24, 4, 31, 0x08, 7),
>>       /* CLK_CFG_10 */
>> -    MUX_GATE_CLR_SET_UPD(CLK_TOP_EDP, "top_edp",
>> -        dp_parents, 0x098, 0x09C, 0x0A0, 0, 4, 7, 0x08, 8),
>> +    MUX_GATE_CLR_SET_UPD_INDEXED(CLK_TOP_EDP, "top_edp",
>> +        edp_parents, edp_parents_idx, 0x098, 0x09C, 0x0A0, 0, 4, 7, 0x08, 8),
>>       MUX_GATE_CLR_SET_UPD(CLK_TOP_DPI, "top_dpi",
>>           dp_parents, 0x098, 0x09C, 0x0A0, 8, 4, 15, 0x08, 9),
>>       MUX_GATE_CLR_SET_UPD(CLK_TOP_DISP_PWM0, "top_disp_pwm0",
> 


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

Re: [PATCH 2/2] clk: mediatek: mt8195-topckgen: Refactor parents for top_dp/edp muxes

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 4259 bytes --]

Hi,

On Fri, Jul 14, 2023 at 12:19:00PM +0800, Chen-Yu Tsai wrote:
> On Thu, Jul 13, 2023 at 9:22 PM Alexandre Mergnat <amergnat@baylibre.com> wrote:
> > On 13/07/2023 09:21, AngeloGioacchino Del Regno wrote:
> > > The top_dp and top_edp muxes can be both parented to either TVDPLL1
> > > or TVDPLL2, two identically specced PLLs for the specific purpose of
> > > giving out pixel clock: this becomes a problem when the MediaTek
> > > DisplayPort Interface (DPI) driver tries to set the pixel clock rate.
> > >
> > > In the usecase of two simultaneous outputs (using two controllers),
> > > it was seen that one of the displays would sometimes display garbled
> > > output (if any at all) and this was because:
> > >   - top_edp was set to TVDPLL1, outputting X GHz
> > >   - top_dp was set to TVDPLL2, outputting Y GHz
> > >     - mtk_dpi calls clk_set_rate(top_edp, Z GHz)
> > >       - top_dp is switched to TVDPLL1
> > >       - TVDPLL1 changes its rate, top_edp outputs the wrong rate.
> > >       - eDP display is garbled
> > >
> > > To solve this issue, remove all TVDPLL1 parents from `top_dp` and
> > > all TVDPLL2 parents from `top_edp`, plus, necessarily switch both
> > > clocks to use the new MUX_GATE_CLR_SET_UPD_INDEXED() macro to be
> > > able to use the right bit index for the new parents list.
> > >
> > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > > ---
> > >   drivers/clk/mediatek/clk-mt8195-topckgen.c | 22 ++++++++++++++--------
> > >   1 file changed, 14 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/clk/mediatek/clk-mt8195-topckgen.c b/drivers/clk/mediatek/clk-mt8195-topckgen.c
> > > index 81daa24cadde..abb3721f6e1b 100644
> > > --- a/drivers/clk/mediatek/clk-mt8195-topckgen.c
> > > +++ b/drivers/clk/mediatek/clk-mt8195-topckgen.c
> > > @@ -417,15 +417,21 @@ static const char * const pwrmcu_parents[] = {
> > >
> > >   static const char * const dp_parents[] = {
> > >       "clk26m",
> > > -     "tvdpll1_d2",
> > >       "tvdpll2_d2",
> > > -     "tvdpll1_d4",
> > >       "tvdpll2_d4",
> > > -     "tvdpll1_d8",
> > >       "tvdpll2_d8",
> > > -     "tvdpll1_d16",
> > >       "tvdpll2_d16"
> > >   };
> > > +static const u8 dp_parents_idx[] = { 0, 2, 4, 6, 8 };
> > > +
> > > +static const char * const edp_parents[] = {
> > > +     "clk26m",
> > > +     "tvdpll1_d2",
> > > +     "tvdpll1_d4",
> > > +     "tvdpll1_d8",
> > > +     "tvdpll1_d16"
> > > +};
> > > +static const u8 edp_parents_idx[] = { 0, 1, 3, 5, 7 };
> >
> > AFAII your solution is to force a specific TVDPLLX for each display, and
> > it isn't dynamic.
> >
> > Do you think it's possible to do that using the DTS ? I'm asking
> > because, IMHO, this kind of setup is more friendly/readable/flexible in
> > the DTS than hardcoded into the driver.
> 
> (CC-ing Maxime, who has some experience in the matter.)

It's not clear to me what the context is, but I'll try my best :)

> assigned-parents doesn't prevent your system from reparenting the clocks
> back to a conflicting configuration.

Yep, it's very much a one-off thing. There's no guarantee at the moment,
and semantics-wise we could change the whole thing at probe time and it
would be fine.

> AFAIK the recommended way to deal with this is to use
> clk_set_rate_exclusive() and co. in whatever consumer driver that
> needs exclusive control on the clock rate.

I guess it works, but it looks to me like the issue here is that the
provider should disable it entirely? My expectation for
clk_set_rate_exclusive() is that one user needs to lock the clock rate
to operate properly.

If the provider expectation is that the rate or parent should never
changed, then that needs to be dealt with at the provider level, ie
through the clk_ops.

> However I'm not sure if that works for parents. It should, given the
> original use case was for the sunxi platforms, which like the MediaTek
> platform here has 2 PLLs for video related consumers, but I couldn't
> find code verifying it.

If you want to prevent clocks from ever being reparented, you can use
the new clk_hw_determine_rate_no_reparent() determine_rate
implementation.

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH 2/2] clk: mediatek: mt8195-topckgen: Refactor parents for top_dp/edp muxes

[AngeloGioacchino Del Regno]

Il 17/07/23 09:48, Maxime Ripard ha scritto:
> Hi,
> 
> On Fri, Jul 14, 2023 at 12:19:00PM +0800, Chen-Yu Tsai wrote:
>> On Thu, Jul 13, 2023 at 9:22 PM Alexandre Mergnat <amergnat@baylibre.com> wrote:
>>> On 13/07/2023 09:21, AngeloGioacchino Del Regno wrote:
>>>> The top_dp and top_edp muxes can be both parented to either TVDPLL1
>>>> or TVDPLL2, two identically specced PLLs for the specific purpose of
>>>> giving out pixel clock: this becomes a problem when the MediaTek
>>>> DisplayPort Interface (DPI) driver tries to set the pixel clock rate.
>>>>
>>>> In the usecase of two simultaneous outputs (using two controllers),
>>>> it was seen that one of the displays would sometimes display garbled
>>>> output (if any at all) and this was because:
>>>>    - top_edp was set to TVDPLL1, outputting X GHz
>>>>    - top_dp was set to TVDPLL2, outputting Y GHz
>>>>      - mtk_dpi calls clk_set_rate(top_edp, Z GHz)
>>>>        - top_dp is switched to TVDPLL1
>>>>        - TVDPLL1 changes its rate, top_edp outputs the wrong rate.
>>>>        - eDP display is garbled
>>>>
>>>> To solve this issue, remove all TVDPLL1 parents from `top_dp` and
>>>> all TVDPLL2 parents from `top_edp`, plus, necessarily switch both
>>>> clocks to use the new MUX_GATE_CLR_SET_UPD_INDEXED() macro to be
>>>> able to use the right bit index for the new parents list.
>>>>
>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>>> ---
>>>>    drivers/clk/mediatek/clk-mt8195-topckgen.c | 22 ++++++++++++++--------
>>>>    1 file changed, 14 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/mediatek/clk-mt8195-topckgen.c b/drivers/clk/mediatek/clk-mt8195-topckgen.c
>>>> index 81daa24cadde..abb3721f6e1b 100644
>>>> --- a/drivers/clk/mediatek/clk-mt8195-topckgen.c
>>>> +++ b/drivers/clk/mediatek/clk-mt8195-topckgen.c
>>>> @@ -417,15 +417,21 @@ static const char * const pwrmcu_parents[] = {
>>>>
>>>>    static const char * const dp_parents[] = {
>>>>        "clk26m",
>>>> -     "tvdpll1_d2",
>>>>        "tvdpll2_d2",
>>>> -     "tvdpll1_d4",
>>>>        "tvdpll2_d4",
>>>> -     "tvdpll1_d8",
>>>>        "tvdpll2_d8",
>>>> -     "tvdpll1_d16",
>>>>        "tvdpll2_d16"
>>>>    };
>>>> +static const u8 dp_parents_idx[] = { 0, 2, 4, 6, 8 };
>>>> +
>>>> +static const char * const edp_parents[] = {
>>>> +     "clk26m",
>>>> +     "tvdpll1_d2",
>>>> +     "tvdpll1_d4",
>>>> +     "tvdpll1_d8",
>>>> +     "tvdpll1_d16"
>>>> +};
>>>> +static const u8 edp_parents_idx[] = { 0, 1, 3, 5, 7 };
>>>
>>> AFAII your solution is to force a specific TVDPLLX for each display, and
>>> it isn't dynamic.
>>>
>>> Do you think it's possible to do that using the DTS ? I'm asking
>>> because, IMHO, this kind of setup is more friendly/readable/flexible in
>>> the DTS than hardcoded into the driver.
>>
>> (CC-ing Maxime, who has some experience in the matter.)
> 
> It's not clear to me what the context is, but I'll try my best :)
> 

I'll try to explain briefly.

On *some* MediaTek platforms, there are two *identical* PLLs for HDMI/(e)DP,
which are internal to the SoC; clocks for HDMI/eDP/DP controller*s* can be
parented either PLL (as you see from this commit).

The PLL's rate can be changed in runtime and you want to use PLL dividers to
get the final pixel clock (that's to obviously reduce the PLL jitter).

>> assigned-parents doesn't prevent your system from reparenting the clocks
>> back to a conflicting configuration.
> 
> Yep, it's very much a one-off thing. There's no guarantee at the moment,
> and semantics-wise we could change the whole thing at probe time and it
> would be fine.
> 

Would be fine... but more complicated I think?

>> AFAIK the recommended way to deal with this is to use
>> clk_set_rate_exclusive() and co. in whatever consumer driver that
>> needs exclusive control on the clock rate.
> 
> I guess it works, but it looks to me like the issue here is that the
> provider should disable it entirely? My expectation for
> clk_set_rate_exclusive() is that one user needs to lock the clock rate
> to operate properly.
> 
> If the provider expectation is that the rate or parent should never
> changed, then that needs to be dealt with at the provider level, ie
> through the clk_ops.
> 
>> However I'm not sure if that works for parents. It should, given the
>> original use case was for the sunxi platforms, which like the MediaTek
>> platform here has 2 PLLs for video related consumers, but I couldn't
>> find code verifying it.
> 
> If you want to prevent clocks from ever being reparented, you can use
> the new clk_hw_determine_rate_no_reparent() determine_rate
> implementation.
> 

We want the clocks to be reparented, as we need them to switch parents as
explained before... that's more or less how the tree looks:

TVDPLL(x) -> PLL Divider (fixed) -> MUX -> Gate -> Controller

Besides, I think that forcing *one* parent to the dp/edp mux would produce a
loss of the flexibility that the clock framework provides.

I again want to emphasize on the fact that TVDPLL1 and TVDPLL2 are *identical*
in specs, and on that there will never be a MT8195 SoC that has only one of
the two PLLs, for obvious reasons...

P.S.: If you need more context, I'll be glad to answer to any other question!

Cheers,
Angelo

> Maxime



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

Re: [PATCH 2/2] clk: mediatek: mt8195-topckgen: Refactor parents for top_dp/edp muxes

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 6259 bytes --]

On Mon, Jul 17, 2023 at 11:13:26AM +0200, AngeloGioacchino Del Regno wrote:
> Il 17/07/23 09:48, Maxime Ripard ha scritto:
> > Hi,
> > 
> > On Fri, Jul 14, 2023 at 12:19:00PM +0800, Chen-Yu Tsai wrote:
> > > On Thu, Jul 13, 2023 at 9:22 PM Alexandre Mergnat <amergnat@baylibre.com> wrote:
> > > > On 13/07/2023 09:21, AngeloGioacchino Del Regno wrote:
> > > > > The top_dp and top_edp muxes can be both parented to either TVDPLL1
> > > > > or TVDPLL2, two identically specced PLLs for the specific purpose of
> > > > > giving out pixel clock: this becomes a problem when the MediaTek
> > > > > DisplayPort Interface (DPI) driver tries to set the pixel clock rate.
> > > > > 
> > > > > In the usecase of two simultaneous outputs (using two controllers),
> > > > > it was seen that one of the displays would sometimes display garbled
> > > > > output (if any at all) and this was because:
> > > > >    - top_edp was set to TVDPLL1, outputting X GHz
> > > > >    - top_dp was set to TVDPLL2, outputting Y GHz
> > > > >      - mtk_dpi calls clk_set_rate(top_edp, Z GHz)
> > > > >        - top_dp is switched to TVDPLL1
> > > > >        - TVDPLL1 changes its rate, top_edp outputs the wrong rate.
> > > > >        - eDP display is garbled
> > > > > 
> > > > > To solve this issue, remove all TVDPLL1 parents from `top_dp` and
> > > > > all TVDPLL2 parents from `top_edp`, plus, necessarily switch both
> > > > > clocks to use the new MUX_GATE_CLR_SET_UPD_INDEXED() macro to be
> > > > > able to use the right bit index for the new parents list.
> > > > > 
> > > > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > > > > ---
> > > > >    drivers/clk/mediatek/clk-mt8195-topckgen.c | 22 ++++++++++++++--------
> > > > >    1 file changed, 14 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/clk/mediatek/clk-mt8195-topckgen.c b/drivers/clk/mediatek/clk-mt8195-topckgen.c
> > > > > index 81daa24cadde..abb3721f6e1b 100644
> > > > > --- a/drivers/clk/mediatek/clk-mt8195-topckgen.c
> > > > > +++ b/drivers/clk/mediatek/clk-mt8195-topckgen.c
> > > > > @@ -417,15 +417,21 @@ static const char * const pwrmcu_parents[] = {
> > > > > 
> > > > >    static const char * const dp_parents[] = {
> > > > >        "clk26m",
> > > > > -     "tvdpll1_d2",
> > > > >        "tvdpll2_d2",
> > > > > -     "tvdpll1_d4",
> > > > >        "tvdpll2_d4",
> > > > > -     "tvdpll1_d8",
> > > > >        "tvdpll2_d8",
> > > > > -     "tvdpll1_d16",
> > > > >        "tvdpll2_d16"
> > > > >    };
> > > > > +static const u8 dp_parents_idx[] = { 0, 2, 4, 6, 8 };
> > > > > +
> > > > > +static const char * const edp_parents[] = {
> > > > > +     "clk26m",
> > > > > +     "tvdpll1_d2",
> > > > > +     "tvdpll1_d4",
> > > > > +     "tvdpll1_d8",
> > > > > +     "tvdpll1_d16"
> > > > > +};
> > > > > +static const u8 edp_parents_idx[] = { 0, 1, 3, 5, 7 };
> > > > 
> > > > AFAII your solution is to force a specific TVDPLLX for each display, and
> > > > it isn't dynamic.
> > > > 
> > > > Do you think it's possible to do that using the DTS ? I'm asking
> > > > because, IMHO, this kind of setup is more friendly/readable/flexible in
> > > > the DTS than hardcoded into the driver.
> > > 
> > > (CC-ing Maxime, who has some experience in the matter.)
> > 
> > It's not clear to me what the context is, but I'll try my best :)
> > 
> 
> I'll try to explain briefly.
> 
> On *some* MediaTek platforms, there are two *identical* PLLs for HDMI/(e)DP,
> which are internal to the SoC; clocks for HDMI/eDP/DP controller*s* can be
> parented either PLL (as you see from this commit)

So the HDMI controller can be parented either to the first or second PLL
(and same thing for the (e)DP controllers)?

> The PLL's rate can be changed in runtime and you want to use PLL dividers to
> get the final pixel clock (that's to obviously reduce the PLL jitter).
> 
> > > assigned-parents doesn't prevent your system from reparenting the clocks
> > > back to a conflicting configuration.
> > 
> > Yep, it's very much a one-off thing. There's no guarantee at the moment,
> > and semantics-wise we could change the whole thing at probe time and it
> > would be fine.
> > 
> 
> Would be fine... but more complicated I think?

My point wasn't that you should do it, but that you can't rely on the
parent or rate sticking around.

> > > AFAIK the recommended way to deal with this is to use
> > > clk_set_rate_exclusive() and co. in whatever consumer driver that
> > > needs exclusive control on the clock rate.
> > 
> > I guess it works, but it looks to me like the issue here is that the
> > provider should disable it entirely? My expectation for
> > clk_set_rate_exclusive() is that one user needs to lock the clock rate
> > to operate properly.
> > 
> > If the provider expectation is that the rate or parent should never
> > changed, then that needs to be dealt with at the provider level, ie
> > through the clk_ops.
> > 
> > > However I'm not sure if that works for parents. It should, given the
> > > original use case was for the sunxi platforms, which like the MediaTek
> > > platform here has 2 PLLs for video related consumers, but I couldn't
> > > find code verifying it.
> > 
> > If you want to prevent clocks from ever being reparented, you can use
> > the new clk_hw_determine_rate_no_reparent() determine_rate
> > implementation.
> > 
> 
> We want the clocks to be reparented, as we need them to switch parents as
> explained before... that's more or less how the tree looks:
> 
> TVDPLL(x) -> PLL Divider (fixed) -> MUX -> Gate -> Controller
> 
> Besides, I think that forcing *one* parent to the dp/edp mux would produce a
> loss of the flexibility that the clock framework provides.
> 
> I again want to emphasize on the fact that TVDPLL1 and TVDPLL2 are *identical*
> in specs, and on that there will never be a MT8195 SoC that has only one of
> the two PLLs, for obvious reasons...
> 
> P.S.: If you need more context, I'll be glad to answer to any other question!

Then I have no idea what the question is :)

What are you trying to achieve / fix, and how can I help you ? :)

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH 2/2] clk: mediatek: mt8195-topckgen: Refactor parents for top_dp/edp muxes

[AngeloGioacchino Del Regno]

Il 17/07/23 13:24, Maxime Ripard ha scritto:
> On Mon, Jul 17, 2023 at 11:13:26AM +0200, AngeloGioacchino Del Regno wrote:
>> Il 17/07/23 09:48, Maxime Ripard ha scritto:
>>> Hi,
>>>
>>> On Fri, Jul 14, 2023 at 12:19:00PM +0800, Chen-Yu Tsai wrote:
>>>> On Thu, Jul 13, 2023 at 9:22 PM Alexandre Mergnat <amergnat@baylibre.com> wrote:
>>>>> On 13/07/2023 09:21, AngeloGioacchino Del Regno wrote:
>>>>>> The top_dp and top_edp muxes can be both parented to either TVDPLL1
>>>>>> or TVDPLL2, two identically specced PLLs for the specific purpose of
>>>>>> giving out pixel clock: this becomes a problem when the MediaTek
>>>>>> DisplayPort Interface (DPI) driver tries to set the pixel clock rate.
>>>>>>
>>>>>> In the usecase of two simultaneous outputs (using two controllers),
>>>>>> it was seen that one of the displays would sometimes display garbled
>>>>>> output (if any at all) and this was because:
>>>>>>     - top_edp was set to TVDPLL1, outputting X GHz
>>>>>>     - top_dp was set to TVDPLL2, outputting Y GHz
>>>>>>       - mtk_dpi calls clk_set_rate(top_edp, Z GHz)
>>>>>>         - top_dp is switched to TVDPLL1
>>>>>>         - TVDPLL1 changes its rate, top_edp outputs the wrong rate.
>>>>>>         - eDP display is garbled
>>>>>>
>>>>>> To solve this issue, remove all TVDPLL1 parents from `top_dp` and
>>>>>> all TVDPLL2 parents from `top_edp`, plus, necessarily switch both
>>>>>> clocks to use the new MUX_GATE_CLR_SET_UPD_INDEXED() macro to be
>>>>>> able to use the right bit index for the new parents list.
>>>>>>
>>>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>>>>> ---
>>>>>>     drivers/clk/mediatek/clk-mt8195-topckgen.c | 22 ++++++++++++++--------
>>>>>>     1 file changed, 14 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/clk/mediatek/clk-mt8195-topckgen.c b/drivers/clk/mediatek/clk-mt8195-topckgen.c
>>>>>> index 81daa24cadde..abb3721f6e1b 100644
>>>>>> --- a/drivers/clk/mediatek/clk-mt8195-topckgen.c
>>>>>> +++ b/drivers/clk/mediatek/clk-mt8195-topckgen.c
>>>>>> @@ -417,15 +417,21 @@ static const char * const pwrmcu_parents[] = {
>>>>>>
>>>>>>     static const char * const dp_parents[] = {
>>>>>>         "clk26m",
>>>>>> -     "tvdpll1_d2",
>>>>>>         "tvdpll2_d2",
>>>>>> -     "tvdpll1_d4",
>>>>>>         "tvdpll2_d4",
>>>>>> -     "tvdpll1_d8",
>>>>>>         "tvdpll2_d8",
>>>>>> -     "tvdpll1_d16",
>>>>>>         "tvdpll2_d16"
>>>>>>     };
>>>>>> +static const u8 dp_parents_idx[] = { 0, 2, 4, 6, 8 };
>>>>>> +
>>>>>> +static const char * const edp_parents[] = {
>>>>>> +     "clk26m",
>>>>>> +     "tvdpll1_d2",
>>>>>> +     "tvdpll1_d4",
>>>>>> +     "tvdpll1_d8",
>>>>>> +     "tvdpll1_d16"
>>>>>> +};
>>>>>> +static const u8 edp_parents_idx[] = { 0, 1, 3, 5, 7 };
>>>>>
>>>>> AFAII your solution is to force a specific TVDPLLX for each display, and
>>>>> it isn't dynamic.
>>>>>
>>>>> Do you think it's possible to do that using the DTS ? I'm asking
>>>>> because, IMHO, this kind of setup is more friendly/readable/flexible in
>>>>> the DTS than hardcoded into the driver.
>>>>
>>>> (CC-ing Maxime, who has some experience in the matter.)
>>>
>>> It's not clear to me what the context is, but I'll try my best :)
>>>
>>
>> I'll try to explain briefly.
>>
>> On *some* MediaTek platforms, there are two *identical* PLLs for HDMI/(e)DP,
>> which are internal to the SoC; clocks for HDMI/eDP/DP controller*s* can be
>> parented either PLL (as you see from this commit)
> 
> So the HDMI controller can be parented either to the first or second PLL
> (and same thing for the (e)DP controllers)?
> 

We're talking about DP/eDP specifically here, but yeah, you got it! :-)

>> The PLL's rate can be changed in runtime and you want to use PLL dividers to
>> get the final pixel clock (that's to obviously reduce the PLL jitter).
>>
>>>> assigned-parents doesn't prevent your system from reparenting the clocks
>>>> back to a conflicting configuration.
>>>
>>> Yep, it's very much a one-off thing. There's no guarantee at the moment,
>>> and semantics-wise we could change the whole thing at probe time and it
>>> would be fine.
>>>
>>
>> Would be fine... but more complicated I think?
> 
> My point wasn't that you should do it, but that you can't rely on the
> parent or rate sticking around.
> 

Cool, I'm happy that we think alike. That's also my point...

>>>> AFAIK the recommended way to deal with this is to use
>>>> clk_set_rate_exclusive() and co. in whatever consumer driver that
>>>> needs exclusive control on the clock rate.
>>>
>>> I guess it works, but it looks to me like the issue here is that the
>>> provider should disable it entirely? My expectation for
>>> clk_set_rate_exclusive() is that one user needs to lock the clock rate
>>> to operate properly.
>>>
>>> If the provider expectation is that the rate or parent should never
>>> changed, then that needs to be dealt with at the provider level, ie
>>> through the clk_ops.
>>>
>>>> However I'm not sure if that works for parents. It should, given the
>>>> original use case was for the sunxi platforms, which like the MediaTek
>>>> platform here has 2 PLLs for video related consumers, but I couldn't
>>>> find code verifying it.
>>>
>>> If you want to prevent clocks from ever being reparented, you can use
>>> the new clk_hw_determine_rate_no_reparent() determine_rate
>>> implementation.
>>>
>>
>> We want the clocks to be reparented, as we need them to switch parents as
>> explained before... that's more or less how the tree looks:
>>
>> TVDPLL(x) -> PLL Divider (fixed) -> MUX -> Gate -> Controller
>>
>> Besides, I think that forcing *one* parent to the dp/edp mux would produce a
>> loss of the flexibility that the clock framework provides.
>>
>> I again want to emphasize on the fact that TVDPLL1 and TVDPLL2 are *identical*
>> in specs, and on that there will never be a MT8195 SoC that has only one of
>> the two PLLs, for obvious reasons...
>>
>> P.S.: If you need more context, I'll be glad to answer to any other question!
> 
> Then I have no idea what the question is :)
> 
> What are you trying to achieve / fix, and how can I help you ? :)
> 

Chen-Yu, Alexandre had/have questions about if there was any other solution instead
of using the solution of *this* commit, so, if there's any other better solution
than the one that I've sent as this commit.

I'm the one saying that this commit is the best solution :-P

Cheers,
Angelo


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

Re: [PATCH 2/2] clk: mediatek: mt8195-topckgen: Refactor parents for top_dp/edp muxes

[Alexandre Mergnat]

On 17/07/2023 16:30, AngeloGioacchino Del Regno wrote:
>>>>> However I'm not sure if that works for parents. It should, given the
>>>>> original use case was for the sunxi platforms, which like the MediaTek
>>>>> platform here has 2 PLLs for video related consumers, but I couldn't
>>>>> find code verifying it.
>>>>
>>>> If you want to prevent clocks from ever being reparented, you can use
>>>> the new clk_hw_determine_rate_no_reparent() determine_rate
>>>> implementation.
>>>>
>>>
>>> We want the clocks to be reparented, as we need them to switch 
>>> parents as
>>> explained before... that's more or less how the tree looks:
>>>
>>> TVDPLL(x) -> PLL Divider (fixed) -> MUX -> Gate -> Controller
>>>
>>> Besides, I think that forcing *one* parent to the dp/edp mux would 
>>> produce a
>>> loss of the flexibility that the clock framework provides.
>>>
>>> I again want to emphasize on the fact that TVDPLL1 and TVDPLL2 are 
>>> *identical*
>>> in specs, and on that there will never be a MT8195 SoC that has only 
>>> one of
>>> the two PLLs, for obvious reasons...
>>>
>>> P.S.: If you need more context, I'll be glad to answer to any other 
>>> question!
>>
>> Then I have no idea what the question is :)
>>
>> What are you trying to achieve / fix, and how can I help you ? :)
>>
> 
> Chen-Yu, Alexandre had/have questions about if there was any other 
> solution instead
> of using the solution of *this* commit, so, if there's any other better 
> solution
> than the one that I've sent as this commit.
> 
> I'm the one saying that this commit is the best solution :-P

Hi Angelo,

My solution is based on PLL static allocation, because I missed it could 
be reparented actually. I think I've a better understanding of this 
commit now, thanks to your explanations. Looks fine for me.

Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>

-- 
Regards,
Alexandre

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

Re: [PATCH 1/2] clk: mediatek: clk-mux: Support custom parent indices for muxes

[Alexandre Mergnat]

On 13/07/2023 09:21, AngeloGioacchino Del Regno wrote:
> Add support for customized parent indices for MediaTek muxes: this is
> necessary for the case in which we want to exclude some clocks from
> a mux's parent clocks list, where the exclusions are not from the
> very bottom of the list but either in the middle or the beginning.
> 
> Example:
> - MUX1 (all parents)
>    - parent1; idx=0
>    - parent2; idx=1
>    - parent3; idx=2
> 
> - MUX1 (wanted parents)
>    - parent1; idx=0
>    - parent3; idx=2
> 
> To achieve that add a `parent_index` array pointer to struct mtk_mux,
> then in .set_parent(), .get_parent() callbacks check if this array
> was populated and eventually get the index from that.
> 
> Also, to avoid updating all clock drivers for all SoCs, rename the
> "main" macro to __GATE_CLR_SET_UPD_FLAGS (so, `__` was added) and
> add the new member to it; furthermore, GATE_CLK_SET_UPD_FLAGS has
> been reintroduced as being fully compatible with the older version.
> 
> The new parent_index can be specified with the new `_INDEXED`
> variants of the MUX_GATE_CLR_SET_UPD_xxxx macros.

Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>

-- 
Regards,
Alexandre

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

Re: [PATCH 2/2] clk: mediatek: mt8195-topckgen: Refactor parents for top_dp/edp muxes

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 3841 bytes --]

On Mon, Jul 17, 2023 at 04:30:48PM +0200, AngeloGioacchino Del Regno wrote:
> > > > > AFAIK the recommended way to deal with this is to use
> > > > > clk_set_rate_exclusive() and co. in whatever consumer driver that
> > > > > needs exclusive control on the clock rate.
> > > > 
> > > > I guess it works, but it looks to me like the issue here is that the
> > > > provider should disable it entirely? My expectation for
> > > > clk_set_rate_exclusive() is that one user needs to lock the clock rate
> > > > to operate properly.
> > > > 
> > > > If the provider expectation is that the rate or parent should never
> > > > changed, then that needs to be dealt with at the provider level, ie
> > > > through the clk_ops.
> > > > 
> > > > > However I'm not sure if that works for parents. It should, given the
> > > > > original use case was for the sunxi platforms, which like the MediaTek
> > > > > platform here has 2 PLLs for video related consumers, but I couldn't
> > > > > find code verifying it.
> > > > 
> > > > If you want to prevent clocks from ever being reparented, you can use
> > > > the new clk_hw_determine_rate_no_reparent() determine_rate
> > > > implementation.
> > > > 
> > > 
> > > We want the clocks to be reparented, as we need them to switch parents as
> > > explained before... that's more or less how the tree looks:
> > > 
> > > TVDPLL(x) -> PLL Divider (fixed) -> MUX -> Gate -> Controller
> > > 
> > > Besides, I think that forcing *one* parent to the dp/edp mux would produce a
> > > loss of the flexibility that the clock framework provides.
> > > 
> > > I again want to emphasize on the fact that TVDPLL1 and TVDPLL2 are *identical*
> > > in specs, and on that there will never be a MT8195 SoC that has only one of
> > > the two PLLs, for obvious reasons...
> > > 
> > > P.S.: If you need more context, I'll be glad to answer to any other question!
> > 
> > Then I have no idea what the question is :)
> > 
> > What are you trying to achieve / fix, and how can I help you ? :)
>
> Chen-Yu, Alexandre had/have questions about if there was any other solution instead
> of using the solution of *this* commit, so, if there's any other better solution
> than the one that I've sent as this commit.
> 
> I'm the one saying that this commit is the best solution :-P

I went back to the original patch, and my understanding is that, when
running two output in parallel, the modeset of one can affect the second
one, and that's bad, right?

If so, then you usually have multiple ways to fix this:

 - This patch
 - Using clk_set_rate_exclusive like Chen-Yu suggested
 - Using a notifier to react to a rate change and adjust

I'm not aware of any "official" guidelines at the clock framework level
regarding which to pick and all are fine.

My opinion though would be to use clk_set_rate_exclusive(), for multiple
reasons.

The first one is that it models correctly what you consumer expects:
that the rate is left untouched. This can happen in virtually any
situation where you have one clock in the same subtree changing rate,
while the patch above will only fix that particular interference.

The second one is that, especially with DP, you only have a handful of
rates you'll need to reach. 148MHz, 297MHz, 594MHz, and possibly a bunch
of others for eDP panels. It's thus likely to have both controllers
having the same frequency requirement, and thus it makes it possible to
run from only one PLL and shut the other down.

This patch will introduce orphan clocks issues that are always a bit
bothersome. A notifier would be troublesome to use and will probably
introduce glitches plus some weird interaction with scrambling if you
ever support it.

So, yeah, using clk_set_rate_exclusive() seems like the best option to me :)

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH 2/2] clk: mediatek: mt8195-topckgen: Refactor parents for top_dp/edp muxes

[AngeloGioacchino Del Regno]

Il 18/07/23 11:03, Maxime Ripard ha scritto:
> On Mon, Jul 17, 2023 at 04:30:48PM +0200, AngeloGioacchino Del Regno wrote:
>>>>>> AFAIK the recommended way to deal with this is to use
>>>>>> clk_set_rate_exclusive() and co. in whatever consumer driver that
>>>>>> needs exclusive control on the clock rate.
>>>>>
>>>>> I guess it works, but it looks to me like the issue here is that the
>>>>> provider should disable it entirely? My expectation for
>>>>> clk_set_rate_exclusive() is that one user needs to lock the clock rate
>>>>> to operate properly.
>>>>>
>>>>> If the provider expectation is that the rate or parent should never
>>>>> changed, then that needs to be dealt with at the provider level, ie
>>>>> through the clk_ops.
>>>>>
>>>>>> However I'm not sure if that works for parents. It should, given the
>>>>>> original use case was for the sunxi platforms, which like the MediaTek
>>>>>> platform here has 2 PLLs for video related consumers, but I couldn't
>>>>>> find code verifying it.
>>>>>
>>>>> If you want to prevent clocks from ever being reparented, you can use
>>>>> the new clk_hw_determine_rate_no_reparent() determine_rate
>>>>> implementation.
>>>>>
>>>>
>>>> We want the clocks to be reparented, as we need them to switch parents as
>>>> explained before... that's more or less how the tree looks:
>>>>
>>>> TVDPLL(x) -> PLL Divider (fixed) -> MUX -> Gate -> Controller
>>>>
>>>> Besides, I think that forcing *one* parent to the dp/edp mux would produce a
>>>> loss of the flexibility that the clock framework provides.
>>>>
>>>> I again want to emphasize on the fact that TVDPLL1 and TVDPLL2 are *identical*
>>>> in specs, and on that there will never be a MT8195 SoC that has only one of
>>>> the two PLLs, for obvious reasons...
>>>>
>>>> P.S.: If you need more context, I'll be glad to answer to any other question!
>>>
>>> Then I have no idea what the question is :)
>>>
>>> What are you trying to achieve / fix, and how can I help you ? :)
>>
>> Chen-Yu, Alexandre had/have questions about if there was any other solution instead
>> of using the solution of *this* commit, so, if there's any other better solution
>> than the one that I've sent as this commit.
>>
>> I'm the one saying that this commit is the best solution :-P
> 
> I went back to the original patch, and my understanding is that, when
> running two output in parallel, the modeset of one can affect the second
> one, and that's bad, right?
> 
> If so, then you usually have multiple ways to fix this:
> 
>   - This patch
>   - Using clk_set_rate_exclusive like Chen-Yu suggested
>   - Using a notifier to react to a rate change and adjust
> 
> I'm not aware of any "official" guidelines at the clock framework level
> regarding which to pick and all are fine.
> 
> My opinion though would be to use clk_set_rate_exclusive(), for multiple
> reasons.
> 
> The first one is that it models correctly what you consumer expects:
> that the rate is left untouched. This can happen in virtually any
> situation where you have one clock in the same subtree changing rate,
> while the patch above will only fix that particular interference.
> 
> The second one is that, especially with DP, you only have a handful of
> rates you'll need to reach. 148MHz, 297MHz, 594MHz, and possibly a bunch
> of others for eDP panels. It's thus likely to have both controllers
> having the same frequency requirement, and thus it makes it possible to
> run from only one PLL and shut the other down.
> 
> This patch will introduce orphan clocks issues that are always a bit
> bothersome. A notifier would be troublesome to use and will probably
> introduce glitches plus some weird interaction with scrambling if you
> ever support it.
> 
> So, yeah, using clk_set_rate_exclusive() seems like the best option to me :)
> 
> Maxime

Sorry for resurrecting a very old thread, I was able to come back to this issue
right now: there's an issue that I can't really think about how to solve with
just the usage of clk_set_rate_exclusive().

Remembering that the clock tree is as following:
TVDPLL(x) -> PLL Divider (fixed) ->
-> MUX (can choose any of TVDPLL(1/2)_d(2/4/6/8/16)) -> Gate -> Controller

The DPI driver is doing:
1. Check the best factor for setting rate of a TVDPLL
2. Set rate of one TVDPLL (specified in DT): clk_set_rate(dpi->tvd_clk, rate);
    2a. Read the rate of that PLL again to know the precise clock output
3. Set rate on the Gate clock (forwards to MUX, selecting TVDPLL(x)_d(y)):
    clk_set_rate(dpi->pixel_clk, rate);


Now, the issue is: if I change the final pixel_clk rate setting to _exclusive(),
nothing still guarantees that we will be selecting the TVDPLL that we have
manipulated in step 2, look at the following example.

tvd_clk == TVDPLL1
pixel_clk == TOP_DP (can be muxed to any tvdpll1/2 dividers!)

clk_set_rate(tvdpll1, something); new_rate = clk_get_rate(tvdpll1)

...calculations... new_rate = pixclk * factor;
...more calculations....

clk_set_rate(pixel_clk, calculated_something)
        ^^^^^^

There is still no guarantee that pixel_clk is getting parented to one of the
TVDPLL1 dividers, as it could still get parented to a TVDPLL2 divider instead
if the other controller has set TVDPLL2 to "an acceptable rate": it's true that
this would work - yes but suboptimally! - because we want to set a specific
factor to reduce jitter on the final pixel clock.


....And I came back to this commit being again the best solution for me because....

1. You also seem to agree with me that a notifier would be troublesome and would
    probably introduce glitches; and
2. clk_set_rate_exclusive() doesn't give me any guarantee about selecting the same
    PLL that the driver was manipulating before.


Am I underestimating and/or ignoring anything else in all of that?

Cheers,
Angelo

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

Re: [PATCH 2/2] clk: mediatek: mt8195-topckgen: Refactor parents for top_dp/edp muxes

[Alexandre Mergnat]

On 04/10/2023 18:29, AngeloGioacchino Del Regno wrote:
> Il 18/07/23 11:03, Maxime Ripard ha scritto:
>> On Mon, Jul 17, 2023 at 04:30:48PM +0200, AngeloGioacchino Del Regno 
>> wrote:
>>>>>>> AFAIK the recommended way to deal with this is to use
>>>>>>> clk_set_rate_exclusive() and co. in whatever consumer driver that
>>>>>>> needs exclusive control on the clock rate.
>>>>>>
>>>>>> I guess it works, but it looks to me like the issue here is that the
>>>>>> provider should disable it entirely? My expectation for
>>>>>> clk_set_rate_exclusive() is that one user needs to lock the clock 
>>>>>> rate
>>>>>> to operate properly.
>>>>>>
>>>>>> If the provider expectation is that the rate or parent should never
>>>>>> changed, then that needs to be dealt with at the provider level, ie
>>>>>> through the clk_ops.
>>>>>>
>>>>>>> However I'm not sure if that works for parents. It should, given the
>>>>>>> original use case was for the sunxi platforms, which like the 
>>>>>>> MediaTek
>>>>>>> platform here has 2 PLLs for video related consumers, but I couldn't
>>>>>>> find code verifying it.
>>>>>>
>>>>>> If you want to prevent clocks from ever being reparented, you can use
>>>>>> the new clk_hw_determine_rate_no_reparent() determine_rate
>>>>>> implementation.
>>>>>>
>>>>>
>>>>> We want the clocks to be reparented, as we need them to switch 
>>>>> parents as
>>>>> explained before... that's more or less how the tree looks:
>>>>>
>>>>> TVDPLL(x) -> PLL Divider (fixed) -> MUX -> Gate -> Controller
>>>>>
>>>>> Besides, I think that forcing *one* parent to the dp/edp mux would 
>>>>> produce a
>>>>> loss of the flexibility that the clock framework provides.
>>>>>
>>>>> I again want to emphasize on the fact that TVDPLL1 and TVDPLL2 are 
>>>>> *identical*
>>>>> in specs, and on that there will never be a MT8195 SoC that has 
>>>>> only one of
>>>>> the two PLLs, for obvious reasons...
>>>>>
>>>>> P.S.: If you need more context, I'll be glad to answer to any other 
>>>>> question!
>>>>
>>>> Then I have no idea what the question is :)
>>>>
>>>> What are you trying to achieve / fix, and how can I help you ? :)
>>>
>>> Chen-Yu, Alexandre had/have questions about if there was any other 
>>> solution instead
>>> of using the solution of *this* commit, so, if there's any other 
>>> better solution
>>> than the one that I've sent as this commit.
>>>
>>> I'm the one saying that this commit is the best solution :-P
>>
>> I went back to the original patch, and my understanding is that, when
>> running two output in parallel, the modeset of one can affect the second
>> one, and that's bad, right?
>>
>> If so, then you usually have multiple ways to fix this:
>>
>>   - This patch
>>   - Using clk_set_rate_exclusive like Chen-Yu suggested
>>   - Using a notifier to react to a rate change and adjust
>>
>> I'm not aware of any "official" guidelines at the clock framework level
>> regarding which to pick and all are fine.
>>
>> My opinion though would be to use clk_set_rate_exclusive(), for multiple
>> reasons.
>>
>> The first one is that it models correctly what you consumer expects:
>> that the rate is left untouched. This can happen in virtually any
>> situation where you have one clock in the same subtree changing rate,
>> while the patch above will only fix that particular interference.
>>
>> The second one is that, especially with DP, you only have a handful of
>> rates you'll need to reach. 148MHz, 297MHz, 594MHz, and possibly a bunch
>> of others for eDP panels. It's thus likely to have both controllers
>> having the same frequency requirement, and thus it makes it possible to
>> run from only one PLL and shut the other down.
>>
>> This patch will introduce orphan clocks issues that are always a bit
>> bothersome. A notifier would be troublesome to use and will probably
>> introduce glitches plus some weird interaction with scrambling if you
>> ever support it.
>>
>> So, yeah, using clk_set_rate_exclusive() seems like the best option to 
>> me :)
>>
>> Maxime
> 
> Sorry for resurrecting a very old thread, I was able to come back to 
> this issue
> right now: there's an issue that I can't really think about how to solve 
> with
> just the usage of clk_set_rate_exclusive().
> 
> Remembering that the clock tree is as following:
> TVDPLL(x) -> PLL Divider (fixed) ->
> -> MUX (can choose any of TVDPLL(1/2)_d(2/4/6/8/16)) -> Gate -> Controller
> 
> The DPI driver is doing:
> 1. Check the best factor for setting rate of a TVDPLL
> 2. Set rate of one TVDPLL (specified in DT): clk_set_rate(dpi->tvd_clk, 
> rate);
>     2a. Read the rate of that PLL again to know the precise clock output
> 3. Set rate on the Gate clock (forwards to MUX, selecting TVDPLL(x)_d(y)):
>     clk_set_rate(dpi->pixel_clk, rate);
> 
> 
> Now, the issue is: if I change the final pixel_clk rate setting to 
> _exclusive(),
> nothing still guarantees that we will be selecting the TVDPLL that we have
> manipulated in step 2, look at the following example.
> 
> tvd_clk == TVDPLL1
> pixel_clk == TOP_DP (can be muxed to any tvdpll1/2 dividers!)
> 
> clk_set_rate(tvdpll1, something); new_rate = clk_get_rate(tvdpll1)
> 
> ...calculations... new_rate = pixclk * factor;
> ...more calculations....
> 
> clk_set_rate(pixel_clk, calculated_something)
>         ^^^^^^
> 
> There is still no guarantee that pixel_clk is getting parented to one of 
> the
> TVDPLL1 dividers, as it could still get parented to a TVDPLL2 divider 
> instead
> if the other controller has set TVDPLL2 to "an acceptable rate": it's 
> true that
> this would work - yes but suboptimally! - because we want to set a specific
> factor to reduce jitter on the final pixel clock.
> 
> 
> ....And I came back to this commit being again the best solution for me 
> because....
> 
> 1. You also seem to agree with me that a notifier would be troublesome 
> and would
>     probably introduce glitches; and
> 2. clk_set_rate_exclusive() doesn't give me any guarantee about 
> selecting the same
>     PLL that the driver was manipulating before.
> 
> 
> Am I underestimating and/or ignoring anything else in all of that?

Thanks for the detailed explanation. I've no solution for you.
You still have my ReviewBy.

-- 
Regards,
Alexandre

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

Re: [PATCH 2/2] clk: mediatek: mt8195-topckgen: Refactor parents for top_dp/edp muxes

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 6732 bytes --]

On Wed, Oct 04, 2023 at 06:29:41PM +0200, AngeloGioacchino Del Regno wrote:
> Il 18/07/23 11:03, Maxime Ripard ha scritto:
> > On Mon, Jul 17, 2023 at 04:30:48PM +0200, AngeloGioacchino Del Regno wrote:
> > > > > > > AFAIK the recommended way to deal with this is to use
> > > > > > > clk_set_rate_exclusive() and co. in whatever consumer driver that
> > > > > > > needs exclusive control on the clock rate.
> > > > > > 
> > > > > > I guess it works, but it looks to me like the issue here is that the
> > > > > > provider should disable it entirely? My expectation for
> > > > > > clk_set_rate_exclusive() is that one user needs to lock the clock rate
> > > > > > to operate properly.
> > > > > > 
> > > > > > If the provider expectation is that the rate or parent should never
> > > > > > changed, then that needs to be dealt with at the provider level, ie
> > > > > > through the clk_ops.
> > > > > > 
> > > > > > > However I'm not sure if that works for parents. It should, given the
> > > > > > > original use case was for the sunxi platforms, which like the MediaTek
> > > > > > > platform here has 2 PLLs for video related consumers, but I couldn't
> > > > > > > find code verifying it.
> > > > > > 
> > > > > > If you want to prevent clocks from ever being reparented, you can use
> > > > > > the new clk_hw_determine_rate_no_reparent() determine_rate
> > > > > > implementation.
> > > > > > 
> > > > > 
> > > > > We want the clocks to be reparented, as we need them to switch parents as
> > > > > explained before... that's more or less how the tree looks:
> > > > > 
> > > > > TVDPLL(x) -> PLL Divider (fixed) -> MUX -> Gate -> Controller
> > > > > 
> > > > > Besides, I think that forcing *one* parent to the dp/edp mux would produce a
> > > > > loss of the flexibility that the clock framework provides.
> > > > > 
> > > > > I again want to emphasize on the fact that TVDPLL1 and TVDPLL2 are *identical*
> > > > > in specs, and on that there will never be a MT8195 SoC that has only one of
> > > > > the two PLLs, for obvious reasons...
> > > > > 
> > > > > P.S.: If you need more context, I'll be glad to answer to any other question!
> > > > 
> > > > Then I have no idea what the question is :)
> > > > 
> > > > What are you trying to achieve / fix, and how can I help you ? :)
> > > 
> > > Chen-Yu, Alexandre had/have questions about if there was any other solution instead
> > > of using the solution of *this* commit, so, if there's any other better solution
> > > than the one that I've sent as this commit.
> > > 
> > > I'm the one saying that this commit is the best solution :-P
> > 
> > I went back to the original patch, and my understanding is that, when
> > running two output in parallel, the modeset of one can affect the second
> > one, and that's bad, right?
> > 
> > If so, then you usually have multiple ways to fix this:
> > 
> >   - This patch
> >   - Using clk_set_rate_exclusive like Chen-Yu suggested
> >   - Using a notifier to react to a rate change and adjust
> > 
> > I'm not aware of any "official" guidelines at the clock framework level
> > regarding which to pick and all are fine.
> > 
> > My opinion though would be to use clk_set_rate_exclusive(), for multiple
> > reasons.
> > 
> > The first one is that it models correctly what you consumer expects:
> > that the rate is left untouched. This can happen in virtually any
> > situation where you have one clock in the same subtree changing rate,
> > while the patch above will only fix that particular interference.
> > 
> > The second one is that, especially with DP, you only have a handful of
> > rates you'll need to reach. 148MHz, 297MHz, 594MHz, and possibly a bunch
> > of others for eDP panels. It's thus likely to have both controllers
> > having the same frequency requirement, and thus it makes it possible to
> > run from only one PLL and shut the other down.
> > 
> > This patch will introduce orphan clocks issues that are always a bit
> > bothersome. A notifier would be troublesome to use and will probably
> > introduce glitches plus some weird interaction with scrambling if you
> > ever support it.
> > 
> > So, yeah, using clk_set_rate_exclusive() seems like the best option to me :)
> > 
> > Maxime
> 
> Sorry for resurrecting a very old thread, I was able to come back to this issue
> right now: there's an issue that I can't really think about how to solve with
> just the usage of clk_set_rate_exclusive().
> 
> Remembering that the clock tree is as following:
> TVDPLL(x) -> PLL Divider (fixed) ->
> -> MUX (can choose any of TVDPLL(1/2)_d(2/4/6/8/16)) -> Gate -> Controller
> 
> The DPI driver is doing:
> 1. Check the best factor for setting rate of a TVDPLL
> 2. Set rate of one TVDPLL (specified in DT): clk_set_rate(dpi->tvd_clk, rate);
>    2a. Read the rate of that PLL again to know the precise clock output
> 3. Set rate on the Gate clock (forwards to MUX, selecting TVDPLL(x)_d(y)):
>    clk_set_rate(dpi->pixel_clk, rate);
> 
> 
> Now, the issue is: if I change the final pixel_clk rate setting to _exclusive(),
> nothing still guarantees that we will be selecting the TVDPLL that we have
> manipulated in step 2, look at the following example.
> 
> tvd_clk == TVDPLL1
> pixel_clk == TOP_DP (can be muxed to any tvdpll1/2 dividers!)
> 
> clk_set_rate(tvdpll1, something); new_rate = clk_get_rate(tvdpll1)
> 
> ...calculations... new_rate = pixclk * factor;
> ...more calculations....
> 
> clk_set_rate(pixel_clk, calculated_something)
>        ^^^^^^
> 
> There is still no guarantee that pixel_clk is getting parented to one of the
> TVDPLL1 dividers, as it could still get parented to a TVDPLL2 divider instead
> if the other controller has set TVDPLL2 to "an acceptable rate": it's true that
> this would work - yes but suboptimally! - because we want to set a specific
> factor to reduce jitter on the final pixel clock.

If your clock ends up with a suboptimal set of parameters, you have a
problem with determine_rate.

> ....And I came back to this commit being again the best solution for me because....
> 
> 1. You also seem to agree with me that a notifier would be troublesome and would
>    probably introduce glitches; and
> 2. clk_set_rate_exclusive() doesn't give me any guarantee about selecting the same
>    PLL that the driver was manipulating before.
> 
> 
> Am I underestimating and/or ignoring anything else in all of that?

I guess I'm still confused about why you want to allow reparenting in
the first place, but still don't want to reparent to the other PLL?

Anyway, it's not a big deal. Whatever works for you I guess :)

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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