Hi!

On Sun, Aug 18, 2019 at 3:59 PM Oleksij Rempel [off-list ref] wrote:

Am 18.08.19 um 09:19 schrieb Chuanhong Guo:

quoted

Hi!

On Sun, Aug 18, 2019 at 2:10 PM Oleksij Rempel [off-list ref] wrote:

quoted

quoted

quoted

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;

quoted

quoted

This code is currently on prototyping phase

Code for clock calculation is done, not "prototyping".

quoted

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?

Regards,
Chuanhong Guo