Re: [PATCH] clk: sunxi-ng: sun50i: h6: Modify GPU clock configuration to support DFS

[Roman Stratiienko]

Hello Samuel,

Thanks for having a look.

вс, 3 июл. 2022 г. в 09:50, Samuel Holland <samuel@xxxxxxxxxxxx>:
>
> On 6/24/22 11:52 AM, Roman Stratiienko wrote:
> > Using simple bash script it was discovered that not all CCU registers
> > can be safely used for DFS, e.g.:
> >
> >     while true
> >     do
> >         devmem 0x3001030 4 0xb0003e02
> >         devmem 0x3001030 4 0xb0001e02
> >     done
> >
> > Script above changes the GPU_PLL multiplier register value. While the
> > script is running, the user should interact with the user interface.
> >
> > Using this method the following results were obtained:
> >
> > | Register  | Name           | Bits  | Values | Result |
> > | --        | --             | --    | --     | --     |
> > | 0x3001030 | GPU_PLL.MULT   | 15..8 | 20-62  | OK     |
> > | 0x3001030 | GPU_PLL.INDIV  |     1 | 0-1    | OK     |
> > | 0x3001030 | GPU_PLL.OUTDIV |     0 | 0-1    | FAIL   |
> > | 0x3001670 | GPU_CLK.DIV    |  3..0 | ANY    | FAIL   |
> >
> > Once bits that caused system failure disabled (kept default 0),
> > it was discovered that GPU_CLK.MUX was used during DFS for some
> > reason and was causing the failure too.
>
> The GPU module clock has only one parent declared, so it is surprising that the
> mux would get set. Did this happen while the kernel driver was changing the
> frequency?

I looked through the ccu code and didn't see anything that may cause
issues, so I tested again and DFS works with MUX this time.

I'll drop this change in v2.

>
> > After disabling GPU_PLL.OUTDIV the system started to fail during
> > booting for some reason until the maximum frequency of GPU_PLL
> > clock was limited to 756MHz.
>
> The manual lists PLL_GPU's maximum frequency as 800 MHz. I assume you chose 756
> MHz because that is the highest OPP. That should be okay, too.

Setting the frequency higher than 756 makes the GPU very unstable.

I decided to validate it again and removed the frequency limitation
and can't see any issues so far.

I'll also drop this change in v2.

>
> > After all the changes made DVFS started to work seamlessly.
> >
> > Signed-off-by: Roman Stratiienko <r.stratiienko@xxxxxxxxx>
> > ---
> >  drivers/clk/sunxi-ng/ccu-sun50i-h6.c | 12 +++++-------
> >  1 file changed, 5 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h6.c b/drivers/clk/sunxi-ng/ccu-sun50i-h6.c
> > index 2ddf0a0da526f..d941238cd178a 100644
> > --- a/drivers/clk/sunxi-ng/ccu-sun50i-h6.c
> > +++ b/drivers/clk/sunxi-ng/ccu-sun50i-h6.c
> > @@ -95,13 +95,14 @@ static struct ccu_nkmp pll_periph1_clk = {
> >       },
> >  };
> >
> > +/* For GPU PLL, using an output divider for DFS causes system to fail */
> >  #define SUN50I_H6_PLL_GPU_REG                0x030
> >  static struct ccu_nkmp pll_gpu_clk = {
> >       .enable         = BIT(31),
> >       .lock           = BIT(28),
> >       .n              = _SUNXI_CCU_MULT_MIN(8, 8, 12),
> >       .m              = _SUNXI_CCU_DIV(1, 1), /* input divider */
> > -     .p              = _SUNXI_CCU_DIV(0, 1), /* output divider */
> > +     .max_rate       = 756000000UL,
> >       .common         = {
> >               .reg            = 0x030,
> >               .hw.init        = CLK_HW_INIT("pll-gpu", "osc24M",
> > @@ -294,12 +295,9 @@ static SUNXI_CCU_M_WITH_MUX_GATE(deinterlace_clk, "deinterlace",
> >  static SUNXI_CCU_GATE(bus_deinterlace_clk, "bus-deinterlace", "psi-ahb1-ahb2",
> >                     0x62c, BIT(0), 0);
> >
> > -static const char * const gpu_parents[] = { "pll-gpu" };
> > -static SUNXI_CCU_M_WITH_MUX_GATE(gpu_clk, "gpu", gpu_parents, 0x670,
> > -                                    0, 3,    /* M */
> > -                                    24, 1,   /* mux */
> > -                                    BIT(31), /* gate */
> > -                                    CLK_SET_RATE_PARENT);
> > +/* GPU_CLK divider kept disabled to avoid interferences with DFS */
> > +static SUNXI_CCU_GATE(gpu_clk, "gpu", "pll-gpu", 0x670,
> > +                   BIT(31), CLK_SET_RATE_PARENT);
>
> These changes look fine to me. You also need to set the initial value for the
> fixed fields in the driver's probe function.

Will do that in v2.

I have no idea what was causing additional issues in my previous test
session. Let's forget about them for now.

Regards,
Roman.

>
> Regards,
> Samuel