Re: [PATCH v2 1/8] clk: sunxi-ng: de2: Fix Display Engine 3.3 definitions

Dne sobota, 9. maj 2026 ob 21:44:02 Srednjeevropski poletni čas je [email protected] napisal(a):
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The new DE33 clock definitions omit the bus clocks for MIXER1 and WB, leaving their bus interfaces unclocked and breaking hardware support.
> --
> 
> commit 7e5df4616faf2d32c9447b44291572215aac8a17
> Author: Jernej Skrabec <[email protected]>
> 
> clk: sunxi-ng: de2: Fix Display Engine 3.3 definitions
> 
> This commit fixes the Display Engine 3.3 resets and clocks to match
> the vendor driver, as the positions of clocks and resets changed in
> version 3.3.
> 
> > diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-de2.c b/drivers/clk/sunxi-ng/ccu-sun8i-de2.c
> > index a6cd0f988859d..6ec6c0dc4c261 100644
> > --- a/drivers/clk/sunxi-ng/ccu-sun8i-de2.c
> > +++ b/drivers/clk/sunxi-ng/ccu-sun8i-de2.c
> 
> [ ... ]
> 
> > @@ -54,6 +61,9 @@ static SUNXI_CCU_M(wb_div_a83_clk, "wb-div", "pll-de", 0x0c, 8, 4,
> >  static SUNXI_CCU_M(rot_div_a83_clk, "rot-div", "pll-de", 0x0c, 0x0c, 4,
> >  		   CLK_SET_RATE_PARENT);
> >  
> > +static SUNXI_CCU_GATE(bus_mixer0_h616_clk, "bus-mixer0", "bus-de",
> > +		      0x08, BIT(0), 0);
> > +
> 
> Will this omission cause a regression for mixer1 and wb?

No, I should explain in commit message that there is only one bus clock,
shared among mixer0, mixer1 and wb. CLK_BUS_MIXER0 is used for all
cores.

> 
> It looks like the bus clocks for mixer1 (BIT(1)) and wb (BIT(4)) at register
> offset 0x08 are missing here.
> 
> Since RST_MIXER1 was previously mapped to 0x08 BIT(1), the DRM driver
> deasserting that reset was inadvertently enabling the DE33 bus clock for
> mixer1.
> 
> Now that RST_MIXER1 is correctly moved to 0x00, will accessing the
> unclocked MIXER1 or WB hardware blocks cause a hard system hang due to MMIO
> access failures?

There are no such issues, as hardware testing showed. Old code worked by luck.

Best regards,
Jernej

> 
> [ ... ]
> 
> > @@ -147,6 +163,17 @@ static struct clk_hw_onecell_data sun50i_a64_de2_hw_clks = {
> >  	.num	= CLK_NUMBER_WITH_ROT,
> >  };
> >  
> > +static struct clk_hw_onecell_data sun50i_h616_de33_hw_clks = {
> > +	.hws	= {
> > +		[CLK_MIXER0]		= &mixer0_h616_clk.common.hw,
> > +		[CLK_MIXER1]		= &mixer1_h616_clk.common.hw,
> > +		[CLK_WB]		= &wb_h616_clk.common.hw,
> > +
> > +		[CLK_BUS_MIXER0]	= &bus_mixer0_h616_clk.common.hw,
> 
> Similarly, should the bus clocks for mixer1 and wb be populated in this
> array so the DRM driver can properly request and enable them?
> 
> > +	},
> > +	.num	= CLK_NUMBER_WITHOUT_ROT,
> > +};
> 
>