From: Chuanhong Guo <gch981213@gmail.com> To: Oleksij Rempel <linux@rempel-privat.de> Cc: Rob Herring <robh@kernel.org>, "open list:COMMON CLK FRAMEWORK" <linux-clk@vger.kernel.org>, "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree@vger.kernel.org>, open list <linux-kernel@vger.kernel.org>, "open list:MIPS" <linux-mips@vger.kernel.org>, "open list:STAGING SUBSYSTEM" <devel@driverdev.osuosl.org>, Michael Turquette <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Ralf Baechle <ralf@linux-mips.org>, Paul Burton <paul.burton@mips.com>, James Hogan <jhogan@kernel.org>, John Crispin <john@phrozen.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Weijie Gao <hackpascal@gmail.com>, NeilBrown <neil@brown.name>, Paul Fertser <fercerpav@gmail.com> Subject: Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation Date: Sun, 18 Aug 2019 16:26:34 +0800 [thread overview] Message-ID: <CAJsYDVKW9-7ityUn83NXcQYmqJi_t-VSV8F0c+BA14_w+poPkA@mail.gmail.com> (raw) In-Reply-To: <6426d4d2-9961-83f2-d3bc-5834ff36b40d@rempel-privat.de> Hi! On Sun, Aug 18, 2019 at 3:59 PM Oleksij Rempel <linux@rempel-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@rempel-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? Regards, Chuanhong Guo
WARNING: multiple messages have this Message-ID (diff)
From: gch981213@gmail.com (Chuanhong Guo) Subject: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation Date: Sun, 18 Aug 2019 16:26:34 +0800 [thread overview] Message-ID: <CAJsYDVKW9-7ityUn83NXcQYmqJi_t-VSV8F0c+BA14_w+poPkA@mail.gmail.com> (raw) In-Reply-To: <6426d4d2-9961-83f2-d3bc-5834ff36b40d@rempel-privat.de> Hi! On Sun, Aug 18, 2019@3:59 PM Oleksij Rempel <linux@rempel-privat.de> wrote: > > Am 18.08.19 um 09:19 schrieb Chuanhong Guo: > > Hi! > > > > On Sun, Aug 18, 2019@2:10 PM Oleksij Rempel <linux@rempel-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? Regards, Chuanhong Guo
next prev parent reply other threads:[~2019-08-18 8:26 UTC|newest] Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-07-24 2:23 [PATCH v2 0/6] MIPS: ralink: add CPU clock detection for MT7621 Chuanhong Guo 2019-07-24 2:23 ` Chuanhong Guo 2019-07-24 2:23 ` [PATCH v2 1/6] dt-bindings: clock: add dt binding header for mt7621-pll Chuanhong Guo 2019-07-24 2:23 ` Chuanhong Guo 2019-07-24 2:23 ` [PATCH v2 2/6] MIPS: ralink: drop ralink_clk_init for mt7621 Chuanhong Guo 2019-07-24 2:23 ` Chuanhong Guo 2019-07-24 2:23 ` [PATCH v2 3/6] MIPS: ralink: add clock device providing cpu/bus clock " Chuanhong Guo 2019-07-24 2:23 ` Chuanhong Guo 2019-07-24 2:23 ` [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation Chuanhong Guo 2019-07-24 2:23 ` Chuanhong Guo 2019-07-29 17:33 ` Paul Burton 2019-07-29 17:33 ` Paul Burton 2019-08-13 15:51 ` Rob Herring 2019-08-13 15:51 ` Rob Herring 2019-08-13 15:51 ` Rob Herring 2019-08-17 14:42 ` Chuanhong Guo 2019-08-17 14:42 ` Chuanhong Guo 2019-08-17 14:42 ` Chuanhong Guo 2019-08-17 15:39 ` Oleksij Rempel 2019-08-17 15:39 ` Oleksij Rempel 2019-08-17 15:39 ` Oleksij Rempel 2019-08-17 16:22 ` Chuanhong Guo 2019-08-17 16:22 ` Chuanhong Guo 2019-08-17 16:22 ` Chuanhong Guo 2019-08-17 18:05 ` Oleksij Rempel 2019-08-17 18:05 ` Oleksij Rempel 2019-08-17 18:05 ` Oleksij Rempel 2019-08-18 2:29 ` Chuanhong Guo 2019-08-18 2:29 ` Chuanhong Guo 2019-08-18 2:29 ` Chuanhong Guo 2019-08-18 6:10 ` Oleksij Rempel 2019-08-18 6:10 ` Oleksij Rempel 2019-08-18 6:10 ` Oleksij Rempel 2019-08-18 7:19 ` Chuanhong Guo 2019-08-18 7:19 ` Chuanhong Guo 2019-08-18 7:19 ` Chuanhong Guo 2019-08-18 7:59 ` Oleksij Rempel 2019-08-18 7:59 ` Oleksij Rempel 2019-08-18 7:59 ` Oleksij Rempel 2019-08-18 8:26 ` Chuanhong Guo [this message] 2019-08-18 8:26 ` Chuanhong Guo 2019-08-18 8:26 ` Chuanhong Guo 2019-08-18 8:44 ` Chuanhong Guo 2019-08-18 8:44 ` Chuanhong Guo 2019-08-18 8:44 ` Chuanhong Guo 2019-08-18 9:51 ` Oleksij Rempel 2019-08-18 9:51 ` Oleksij Rempel 2019-08-18 9:51 ` Oleksij Rempel 2019-08-18 10:07 ` Chuanhong Guo 2019-08-18 10:07 ` Chuanhong Guo 2019-08-18 10:07 ` Chuanhong Guo 2019-08-17 15:40 ` Oleksij Rempel 2019-08-17 15:40 ` Oleksij Rempel 2019-08-17 15:40 ` Oleksij Rempel 2019-07-24 2:23 ` [PATCH v2 5/6] staging: mt7621-dts: fix register range of memc node in mt7621.dtsi Chuanhong Guo 2019-07-24 2:23 ` Chuanhong Guo 2019-07-24 2:23 ` [PATCH v2 6/6] staging: mt7621-dts: add dt nodes for mt7621-pll Chuanhong Guo 2019-07-24 2:23 ` Chuanhong Guo
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAJsYDVKW9-7ityUn83NXcQYmqJi_t-VSV8F0c+BA14_w+poPkA@mail.gmail.com \ --to=gch981213@gmail.com \ --cc=devel@driverdev.osuosl.org \ --cc=devicetree@vger.kernel.org \ --cc=fercerpav@gmail.com \ --cc=gregkh@linuxfoundation.org \ --cc=hackpascal@gmail.com \ --cc=jhogan@kernel.org \ --cc=john@phrozen.org \ --cc=linux-clk@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mips@vger.kernel.org \ --cc=linux@rempel-privat.de \ --cc=mark.rutland@arm.com \ --cc=mturquette@baylibre.com \ --cc=neil@brown.name \ --cc=paul.burton@mips.com \ --cc=ralf@linux-mips.org \ --cc=robh@kernel.org \ --cc=sboyd@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.