Message-ID: <CAJsYDVJnQyOXHKWztoE72KKnMinJL+1AZGy_CvTT6oOs5m5U2w@mail.gmail.com>
Date:   Sun, 18 Aug 2019 16:44:32 +0800
From:   Chuanhong Guo <gch981213@...il.com>
To:     Oleksij Rempel <linux@...pel-privat.de>
Cc:     Rob Herring <robh@...nel.org>,
        "open list:COMMON CLK FRAMEWORK" <linux-clk@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>,
        "open list:MIPS" <linux-mips@...r.kernel.org>,
        "open list:STAGING SUBSYSTEM" <devel@...verdev.osuosl.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Ralf Baechle <ralf@...ux-mips.org>,
        Paul Burton <paul.burton@...s.com>,
        James Hogan <jhogan@...nel.org>,
        John Crispin <john@...ozen.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Weijie Gao <hackpascal@...il.com>, NeilBrown <neil@...wn.name>,
        Paul Fertser <fercerpav@...il.com>
Subject: Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation

On Sun, Aug 18, 2019 at 4:26 PM Chuanhong Guo <gch981213@...il.com> wrote:
>
> Hi!
>
> On Sun, Aug 18, 2019 at 3:59 PM Oleksij Rempel <linux@...pel-privat.de> wrote:
> >
> > Am 18.08.19 um 09:19 schrieb Chuanhong Guo:
> > > Hi!
> > >
> > > On Sun, Aug 18, 2019 at 2:10 PM Oleksij Rempel <linux@...pel-privat.de> wrote:
> > >>
> > >>>> We have at least 2 know registers:
> > >>>> SYSC_REG_CPLL_CLKCFG0 - it provides some information about boostrapped
> > >>>> refclock. PLL and dividers used for CPU and some sort of BUS (AHB?).
> > >>>> SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable clocks for
> > >>>> all or some ip cores.
> > >>>> What is probably missing is a set of dividers for
> > >>>> each ip core. From your words it is not document.
> > >>>
> > >>> The specific missing part I was referring to, is parent clocks for
> > >>> every gates. I'm not going to assume this with current openwrt device
> > >>> tree because some peripherals doesn't have a clock binding at all or
> > >>> have a dummy one there.
> > >>
> > >> Ok, then I do not understand what is the motivation to upstream
> > >> something what is not nearly ready for use.
> > >
> > > Why isn't it "ready for use" then?
> > > A complete mt7621-pll driver will contain two parts:
> > > 1. A clock provider which outputs several clocks
> > > 2. A clock gate with parent clocks properly configured
> > >
> > > Two clocks provided here are just two clocks that can't be controlled
> > > in kernel no matter where it goes (arch/mips/ralink or drivers/clk).
> > > Having a working CPU clock provider is better than defining a fixed
> > > clock in dts because CPU clock can be controlled by bootloader.
> > > (BTW description for CPU PLL register is also missing in datasheet.)
> > > Clock gate is an unrelated part and there is no information to
> > > properly implement it unless MTK decided to release a clock plan
> > > somehow.
> >
> > With other words, your complete system is running with unknown clock
> > rates.
>
> And without this patchset the complete system is running with unknown
> clock and, even worse, we make assumptions about what clock bootloader
> uses, hardcoded it in dts and hope it is the correct value.
>
> > The source clock in the clock three can be configured differently
> > by bootloader but you don't know how it is done how and it is not
> > documented.
>
> Actually, I don't know about this and I didn't wrote the original
> clock calculation code. I just ported it from downstream OpenWrt
> kernel. Here's a piece of code from Mediatek's SDK kernel:
>
> case 0:
>         reg = (*(volatile u32 *)(RALINK_SYSCTL_BASE + 0x44));
>         cpu_fdiv = ((reg >> 8) & 0x1F);
>         cpu_ffrac = (reg & 0x1F);
> mips_cpu_feq = (500 * cpu_ffrac / cpu_fdiv) * 1000 * 1000;
> break;
> case 1: //CPU PLL
>         reg = (*(volatile u32 *)(RALINK_MEMCTRL_BASE + 0x648));
>         fbdiv = ((reg >> 4) & 0x7F) + 1;
>         reg = (*(volatile u32 *)(RALINK_SYSCTL_BASE + 0x10));
>         reg = (reg >> 6) & 0x7;
>         if(reg >= 6) { //25Mhz Xtal
>             mips_cpu_feq = 25 * fbdiv * 1000 * 1000;
>         } else if(reg >=3) { //40Mhz Xtal
>             mips_cpu_feq = 20 * fbdiv * 1000 * 1000;
>         } else { // 20Mhz Xtal
>             /* TODO */
>         }
>         break;
>
>
>
> >
> > >> This code is currently on prototyping phase
> > >
> > > Code for clock calculation is done, not "prototyping".
> > >
> > >> It means, we cannot expect that this driver will be fixed any time soon.
> > >
> > > I think clock gating is a separated feature instead of a broken part
> > > that has to be fixed.
> >
> > Ok, i would agree with it. But from what you said, this feature will be
> > never implemented.
> >
> > So, I repeat my question. What is the point to upstream code for a
> > system, which has not enough information to get proper clock rate even
> > for uart? or is uart running with cpu or bus clock rate?
>
> uart runs of a fixed 50MHz clock according to another piece of code
> from MTK SDK:
> (a pastebin version here for better readability. This specific
> question has nothing to do with patch reviewing and doesn't need to be
> preserved in mail forever.)
> https://paste.ubuntu.com/p/fYmtDFW9nh/
>
> I could ask the same question:
> What is the point of upstreaming an incomplete MT7621 support in the
> first place? Current MT7621 support in upstream kernel works only for
> mt7621a not mt7621s and it runs of unknown clocks. These kind of code
> should stay in downstream projects like OpenWrt forever isn't it?

And in fact you've upstreamed a broken ag71xx driver anyway.

This debate goes nowhere. I've clarified the situation and made my
point. Of course I can't read through the ancient and heavily hacked
vendor kernel to figure out a clock plan myself.

Regards,
Chuanhong Guo

Powered by blists - more mailing lists