Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
Hi! On Sun, Aug 18, 2019 at 5:51 PM Oleksij Rempel wrote: > > lets see more code: > drivers/staging/mt7621-mmc/sd.c > /* clock source for host: global */ > #if defined(CONFIG_SOC_MT7620) > static u32 hclks[] = {4800}; /* +/- by chhung */ > #elif defined(CONFIG_SOC_MT7621) > static u32 hclks[] = {5000}; /* +/- by chhung */ > #endif > > hm.. 50Mhz again. Feels like APB clock. > > ./drivers/staging/mt7621-dts/mt7621.dtsi > cpuclock: cpuclock@0 { > #clock-cells = <0>; > compatible = "fixed-clock"; > > /* FIXME: there should be way to detect this */ > clock-frequency = <88000>; > }; > > Your driver is trying to cover cpuclock > > sysclock: sysclock@0 { > #clock-cells = <0>; > compatible = "fixed-clock"; > > /* This is normally 1/4 of cpuclock */ > clock-frequency = <22000>; > }; > > and most probably system clock alias "bus clock", most probably AHB. This sysclock was the 50MHz clock in OpenWrt. It's set as "bus clock" upstream by an incorrect commit. As already stated in previous reply: I'm not going to make assumption about clock plan using OpenWrt's device tree because it already contains several mistakes on clocks. Since the upstream device tree comes from there, I don't trust it either. You might want to check out patch 6/6 in this series where the original author of this commit in openwrt fixed some clocks and I ported it here. > [...] > > 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. > > Ok, I provided you some productive technical hints how it should be > done. I don't think mt7620 has better documentation then mt7621 and even > in this case it was possible to make more or less good driver. It does. A clock plan for mt7620 is available in MT7620 Programming Guide, Page 14. Regards, Chuanhong Guo ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
Am 18.08.19 um 10:44 schrieb Chuanhong Guo: > On Sun, Aug 18, 2019 at 4:26 PM Chuanhong Guo wrote: >> >> Hi! >> >> On Sun, Aug 18, 2019 at 3:59 PM Oleksij Rempel >> wrote: >>> >>> Am 18.08.19 um 09:19 schrieb Chuanhong Guo: Hi! On Sun, Aug 18, 2019 at 2:10 PM Oleksij Rempel 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/ ok. lets see more code: drivers/staging/mt7621-mmc/sd.c /* clock source for host: global */ #if defined(CONFIG_SOC_MT7620) static u32 hclks[] = {4800}; /* +/- by chhung */ #elif defined(CONFIG_SOC_MT7621) static u32 hclks[] = {5000}; /* +/- by chhung */ #endif hm.. 50Mhz again. Feels like APB clock. ./drivers/staging/mt7621-dts/mt7621.dtsi cpuclock: cpuclock@0 { #clock-cells = <0>; compatible = "fixed-clock"; /* FIXME: there should be way to detect this */ clock-frequency = <88000>; }; Your driver is trying to cover cpuclock sysclock: sysclock@0 { #clock-cells = <0>; compatible = "fixed-clock"; /* This is normally 1/4 of
Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
On Sun, Aug 18, 2019 at 4:26 PM Chuanhong Guo wrote: > > Hi! > > On Sun, Aug 18, 2019 at 3:59 PM Oleksij Rempel wrote: > > > > Am 18.08.19 um 09:19 schrieb Chuanhong Guo: > > > Hi! > > > > > > On Sun, Aug 18, 2019 at 2:10 PM Oleksij Rempel > > > 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
Hi! On Sun, Aug 18, 2019 at 3:59 PM Oleksij Rempel wrote: > > Am 18.08.19 um 09:19 schrieb Chuanhong Guo: > > Hi! > > > > On Sun, Aug 18, 2019 at 2:10 PM Oleksij Rempel > > 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
Am 18.08.19 um 09:19 schrieb Chuanhong Guo: > Hi! > > On Sun, Aug 18, 2019 at 2:10 PM Oleksij Rempel 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. 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. >> 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? -- Regards, Oleksij ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
Hi! On Sun, Aug 18, 2019 at 2:10 PM Oleksij Rempel 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. > 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. Regards, Chuanhong Guo ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
Am 18.08.19 um 04:29 schrieb Chuanhong Guo: > Hi! > > On Sun, Aug 18, 2019 at 2:06 AM Oleksij Rempel wrote: SYSC_REG_CPLL_CLKCFG1 register is a clock gate controller. It is used to enable or disable clocks. Jist wild assumption. All peripheral devices are suing bus clock. >>> >>> This assumption is incorrect. When this patchset is applied in >>> OpenWrt, I asked the author why there's still a fixed clock in >>> mt7621.dtsi, He told me that there's another clock for those unchanged >>> peripherals and he doesn't have time to write a clock provider for it. >> >> Can you please provide a link to this patch or email. > > This discussion is in Chinese and using an IM software so there's no > link available. > >> 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. This code is currently on prototyping phase and you have no information how to make it ready. It means, we cannot expect that this driver will be fixed any time soon. -- Regards, Oleksij ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
Hi! On Sun, Aug 18, 2019 at 2:06 AM Oleksij Rempel wrote: > >> SYSC_REG_CPLL_CLKCFG1 register is a clock gate controller. It is used to > >> enable or disable clocks. > >> Jist wild assumption. All peripheral devices are suing bus clock. > > > > This assumption is incorrect. When this patchset is applied in > > OpenWrt, I asked the author why there's still a fixed clock in > > mt7621.dtsi, He told me that there's another clock for those unchanged > > peripherals and he doesn't have time to write a clock provider for it. > > Can you please provide a link to this patch or email. This discussion is in Chinese and using an IM software so there's no link available. > 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. > > With this information the clk driver will provide gate functionality and > a set of hardcoded clocks. With this driver will work part of power > management and nice devicetree without fixed clocks. Regards, Chuanhong Guo ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
Am 17.08.19 um 18:22 schrieb Chuanhong Guo: > Hi! > > On Sat, Aug 17, 2019 at 11:40 PM Oleksij Rempel wrote: > >> In provided link [0] the ralink_clk_init function is reading >> SYSC_REG_CPLL_CLKCFG0 R/W register. >> This register is used to determine clock source, clock freq and CPU or bus >> clocks. > > This register should only be changed by bootloader, not kernel. So > it's read-only in kernel's perspective. there is no kernel perspective, until you have some kind of privilege separation. There is only: "i decided not to write on to writeable register". >> SYSC_REG_CPLL_CLKCFG1 register is a clock gate controller. It is used to >> enable or disable clocks. >> Jist wild assumption. All peripheral devices are suing bus clock. > > This assumption is incorrect. When this patchset is applied in > OpenWrt, I asked the author why there's still a fixed clock in > mt7621.dtsi, He told me that there's another clock for those unchanged > peripherals and he doesn't have time to write a clock provider for it. Can you please provide a link to this patch or email. > I don't know how many undocumented clocks are there since this piece > of info is missing in datasheet. > >> >> IMO - this information is enough to create full blown >> drivers/clk/mediatek/clk-mt7621.c > > And this information isn't enough because the assumption above is incorrect :P Ok, let's assume I accept this not technical argumentation. 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. With this information the clk driver will provide gate functionality and a set of hardcoded clocks. With this driver will work part of power management and nice devicetree without fixed clocks. -- Regards, Oleksij ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
Hi! On Sat, Aug 17, 2019 at 11:40 PM Oleksij Rempel wrote: > In provided link [0] the ralink_clk_init function is reading > SYSC_REG_CPLL_CLKCFG0 R/W register. > This register is used to determine clock source, clock freq and CPU or bus > clocks. This register should only be changed by bootloader, not kernel. So it's read-only in kernel's perspective. > SYSC_REG_CPLL_CLKCFG1 register is a clock gate controller. It is used to > enable or disable clocks. > Jist wild assumption. All peripheral devices are suing bus clock. This assumption is incorrect. When this patchset is applied in OpenWrt, I asked the author why there's still a fixed clock in mt7621.dtsi, He told me that there's another clock for those unchanged peripherals and he doesn't have time to write a clock provider for it. I don't know how many undocumented clocks are there since this piece of info is missing in datasheet. > > IMO - this information is enough to create full blown > drivers/clk/mediatek/clk-mt7621.c And this information isn't enough because the assumption above is incorrect :P Regards, Chuanhong Guo ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
Hi, Am 17.08.19 um 16:42 schrieb Chuanhong Guo: Hi! On Tue, Aug 13, 2019 at 11:51 PM Rob Herring wrote: [...] +Example: + pll { + compatible = "mediatek,mt7621-pll"; You didn't answer Stephen's question on v1. I thought he was asking why there's a syscon in compatible string. I noticed that the syscon in my previous patch is a copy-paste error from elsewhere and dropped it. Based on this binding, there is no way to control/program the PLL. Is this part of some IP block? The entire section is called "system control" in datasheet and is occupied in arch/mips/ralink/mt7621.c [0] Two clocks provided here is determined by reading some read-only registers in this part. There's another register in this section providing clock gates for every peripherals, but MTK doesn't provide a clock plan in their datasheet. I can't determine corresponding clock frequencies for every peripherals, thus unable to write a working clock driver. In provided link [0] the ralink_clk_init function is reading SYSC_REG_CPLL_CLKCFG0 R/W register. This register is used to determine clock source, clock freq and CPU or bus clocks. SYSC_REG_CPLL_CLKCFG1 register is a clock gate controller. It is used to enable or disable clocks. Jist wild assumption. All peripheral devices are suing bus clock. IMO - this information is enough to create full blown drivers/clk/mediatek/clk-mt7621.c + + #clock-cells = <1>; + clock-output-names = "cpu", "bus"; + }; -- 2.21.0 Regards, Chuanhong Guo [0] https://elixir.bootlin.com/linux/latest/source/arch/mips/ralink/mt7621.c#L156 -- Regards, Oleksij ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
Hi, Am 17.08.19 um 16:42 schrieb Chuanhong Guo: Hi! On Tue, Aug 13, 2019 at 11:51 PM Rob Herring wrote: [...] +Example: + pll { + compatible = "mediatek,mt7621-pll"; You didn't answer Stephen's question on v1. I thought he was asking why there's a syscon in compatible string. I noticed that the syscon in my previous patch is a copy-paste error from elsewhere and dropped it. Based on this binding, there is no way to control/program the PLL. Is this part of some IP block? The entire section is called "system control" in datasheet and is occupied in arch/mips/ralink/mt7621.c [0] Two clocks provided here is determined by reading some read-only registers in this part. There's another register in this section providing clock gates for every peripherals, but MTK doesn't provide a clock plan in their datasheet. I can't determine corresponding clock frequencies for every peripherals, thus unable to write a working clock driver. In provided link [0] the ralink_clk_init function is reading SYSC_REG_CPLL_CLKCFG0 R/W register. This register is used to determine clock source, clock freq and CPU or bus clocks. SYSC_REG_CPLL_CLKCFG1 register is a clock gate controller. It is used to enable or disable clocks. Jist wild assumption. All peripheral devices are suing bus clock. IMO - this information is enough to create full blown drivers/clk/mediatek/clk-mt7621.c + + #clock-cells = <1>; + clock-output-names = "cpu", "bus"; + }; -- 2.21.0 Regards, Chuanhong Guo [0] https://elixir.bootlin.com/linux/latest/source/arch/mips/ralink/mt7621.c#L156 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
Hi! On Tue, Aug 13, 2019 at 11:51 PM Rob Herring wrote: > [...] > > +Example: > > + pll { > > + compatible = "mediatek,mt7621-pll"; > > You didn't answer Stephen's question on v1. I thought he was asking why there's a syscon in compatible string. I noticed that the syscon in my previous patch is a copy-paste error from elsewhere and dropped it. > > Based on this binding, there is no way to control/program the PLL. Is > this part of some IP block? The entire section is called "system control" in datasheet and is occupied in arch/mips/ralink/mt7621.c [0] Two clocks provided here is determined by reading some read-only registers in this part. There's another register in this section providing clock gates for every peripherals, but MTK doesn't provide a clock plan in their datasheet. I can't determine corresponding clock frequencies for every peripherals, thus unable to write a working clock driver. > > > + > > + #clock-cells = <1>; > > + clock-output-names = "cpu", "bus"; > > + }; > > -- > > 2.21.0 > > Regards, Chuanhong Guo [0] https://elixir.bootlin.com/linux/latest/source/arch/mips/ralink/mt7621.c#L156 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
On Wed, Jul 24, 2019 at 10:23:08AM +0800, Chuanhong Guo wrote: > This commit adds device tree binding documentation for MT7621 > PLL controller. > > Signed-off-by: Chuanhong Guo > --- > > Change since v1: > drop useless syscon in compatible string > > .../bindings/clock/mediatek,mt7621-pll.txt | 18 ++ > 1 file changed, 18 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt > > diff --git a/Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt > b/Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt > new file mode 100644 > index ..7dcfbd5283e3 > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt > @@ -0,0 +1,18 @@ > +Binding for Mediatek MT7621 PLL controller > + > +The PLL controller provides the 2 main clocks of the SoC: CPU and BUS. > + > +Required Properties: > +- compatible: has to be "mediatek,mt7621-pll" > +- #clock-cells: has to be one > + > +Optional properties: > +- clock-output-names: should be "cpu", "bus" > + > +Example: > + pll { > + compatible = "mediatek,mt7621-pll"; You didn't answer Stephen's question on v1. Based on this binding, there is no way to control/program the PLL. Is this part of some IP block? > + > + #clock-cells = <1>; > + clock-output-names = "cpu", "bus"; > + }; > -- > 2.21.0 > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
Hi Chuanhong, On Wed, Jul 24, 2019 at 10:23:08AM +0800, Chuanhong Guo wrote: > This commit adds device tree binding documentation for MT7621 > PLL controller. > > Signed-off-by: Chuanhong Guo > --- > > Change since v1: > drop useless syscon in compatible string > > .../bindings/clock/mediatek,mt7621-pll.txt | 18 ++ > 1 file changed, 18 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt This binding needs review from DT maintainers before I apply it, but as a general note it's typical to add the binding *before* its use in the series. That is, this patch should come before patch 3. Personally I'd squash it with patch 1 so the binding & the header file needed to use the binding are added in one patch, then a later patch actually makes use of them. Thanks, Paul > diff --git a/Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt > b/Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt > new file mode 100644 > index ..7dcfbd5283e3 > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt > @@ -0,0 +1,18 @@ > +Binding for Mediatek MT7621 PLL controller > + > +The PLL controller provides the 2 main clocks of the SoC: CPU and BUS. > + > +Required Properties: > +- compatible: has to be "mediatek,mt7621-pll" > +- #clock-cells: has to be one > + > +Optional properties: > +- clock-output-names: should be "cpu", "bus" > + > +Example: > + pll { > + compatible = "mediatek,mt7621-pll"; > + > + #clock-cells = <1>; > + clock-output-names = "cpu", "bus"; > + }; > -- > 2.21.0 > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
This commit adds device tree binding documentation for MT7621 PLL controller. Signed-off-by: Chuanhong Guo --- Change since v1: drop useless syscon in compatible string .../bindings/clock/mediatek,mt7621-pll.txt | 18 ++ 1 file changed, 18 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt diff --git a/Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt b/Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt new file mode 100644 index ..7dcfbd5283e3 --- /dev/null +++ b/Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt @@ -0,0 +1,18 @@ +Binding for Mediatek MT7621 PLL controller + +The PLL controller provides the 2 main clocks of the SoC: CPU and BUS. + +Required Properties: +- compatible: has to be "mediatek,mt7621-pll" +- #clock-cells: has to be one + +Optional properties: +- clock-output-names: should be "cpu", "bus" + +Example: + pll { + compatible = "mediatek,mt7621-pll"; + + #clock-cells = <1>; + clock-output-names = "cpu", "bus"; + }; -- 2.21.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel