Re: [PATCH 2/6] clk: rockchip: fix wrong clk_ref_usb3otg parent for rk3328

Peter Geis
2024-12-10T13:27:24Z

On Tue, Dec 10, 2024 at 4:44 AM Dragan Simic <dsimic@manjaro.org> wrote:
>
> Hello Peter,
>
> On 2024-12-10 02:30, Peter Geis wrote:
> > Correct the clk_ref_usb3otg parent to fix clock control for the usb3
> > controller on rk3328. Verified against the rk3328 trm and usb3 clock
> > tree
> > documentation.
> >
> > Fixes: fe3511ad8a1c ("clk: rockchip: add clock controller for rk3328")
> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> > ---
> >
> > drivers/clk/rockchip/clk-rk3328.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/rockchip/clk-rk3328.c
> > b/drivers/clk/rockchip/clk-rk3328.c
> > index 3bb87b27b662..cf60fcf2fa5c 100644
> > --- a/drivers/clk/rockchip/clk-rk3328.c
> > +++ b/drivers/clk/rockchip/clk-rk3328.c
> > @@ -201,7 +201,7 @@ PNAME(mux_aclk_peri_pre_p) = { "cpll_peri",
> > "gpll_peri",
> > "hdmiphy_peri" };
> > PNAME(mux_ref_usb3otg_src_p) = { "xin24m",
> > - "clk_usb3otg_ref" };
> > + "clk_ref_usb3otg_src" };
> > PNAME(mux_xin24m_32k_p) = { "xin24m",
> > "clk_rtc32k" };
> > PNAME(mux_mac2io_src_p) = { "clk_mac2io_src",
>
> Sorry, but I was unable to verify this in the part 1 of the
> RK3328 TRM, in both versions 1.1 and 1.2, which is all I have
> when it comes to the RK3328 TRM. Is that maybe described in
> the part 2, which I've been unable to locate for years?
>
> Moreover, the downstream kernel source from Rockchip does it
> the way [1] it's currently done in the mainline kernel, which
> makes me confused a bit? Could you, please, provide more
> details about the two references you mentioned in the patch
> description, or maybe even you could provide the links to
> those two references?
>
> [1]
> https://raw.githubusercontent.com/rockchip-linux/kernel/refs/heads/develop-4.4/drivers/clk/rockchip/clk-rk3328.c

It is unfortunate the TRM doesn't include the clock maps, because they
are extremely helpful when one can acquire them. It also doesn't help
that the TRM register definition only referred to this as "pll". I was
sent specifically the usb3 phy clock map for my work on the driver,
which had the location of each switch and divider along with the
register and bit that controlled it. That combined with the TRM
register map allowed me to find this error.

Thanks!
Peter