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

  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(-)
  

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",
  

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
  

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",
>
  

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
  

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
  

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
  

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
  

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>
  

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
  

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
  

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.
  

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