| Message ID | 20201111163013.29412-1-sergio.paracuellos@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | MIPS: ralink: add CPU clock detection and clock gate driver for MT7621 | expand |
Hi! On Thu, Nov 12, 2020 at 12:30 AM Sergio Paracuellos <sergio.paracuellos@gmail.com> wrote: > > This patchset ports CPU clock detection for MT7621 from OpenWrt > and adds a complete clock plan for the mt7621 SOC. > > The documentation for this SOC only talks about two registers > regarding to the clocks: > * SYSC_REG_CPLL_CLKCFG0 - 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. > > No documentation about a probably existant set of dividers for each ip > core is included in the datasheets. So we cannot make anything better, > AFAICT. > > Looking into driver code, there is another frequency which is used in > some drivers (uart, sd...) which for any reason is always hardcoded to > 50 MHz. Taking this into account this patchset provides three main fixed > clocks to the SOC in 'mt7621-pll' which are: > - "cpu": with detected frequency (900 MHz in my board). > - "ahb": cpu / 4 = 225 Mhz. > - "apb": 50 Mhz. > > PLL controller cannot be manipulatedbecause there is no info about > how to do it. Because of this, there is nothing related with registers > in the included binding. > > It also provides a clock gate driver 'mt7621-clk' as a platform driver > to allow to enable and disable some clocks in the different ip cores. > The parent clocks for this clock gates have also set taking into account > existant device tree and driver code resulting in the followings: > - "hsdma": "ahb" > - "fe": "ahb" > - "sp_divtx": "ahb" > - "timer": "cpu" > - "int": "cpu" > - "mc": "ahb" > - "pcm": "ahb" > - "pio": "ahb" > - "gdma": "ahb" > - "nand": "ahb" > - "i2c": "ahb" > - "i2s": "ahb" > - "spi": "ahb" > - "uart1": "apb" > - "uart2": "apb" > - "uart3": "apb" > - "eth": "ahb" > - "pcie0": "ahb" > - "pcie1": "ahb" > - "pcie2": "ahb" > - "crypto": "ahb" > - "shxc": "ahb" > > There was a previous attempt of doing this here[0] but the author > did not wanted to make assumptions of a clock plan for the platform. I've already said in previous threads that clock assignment in current linux kernel is not trustworthy. I've got the clock plan for mt7621 now. (Can't share it, sorry.) Most of your clock assumptions above are incorrect. I've made a clock driver with gate support a few months ago.[0] but I don't have much time to really finish it. Maybe you could rework your clock gate driver based on it. [0] https://github.com/981213/linux/commit/2eca1f045e4c3db18c941135464c0d7422ad8133
On Thu, Nov 12, 2020 at 9:26 AM Chuanhong Guo <gch981213@gmail.com> wrote: > > I've already said in previous threads that clock assignment in > current linux kernel is not trustworthy. > I've got the clock plan for mt7621 now. (Can't share it, sorry.) > Most of your clock assumptions above are incorrect. > I've made a clock driver with gate support a few months ago.[0] > but I don't have much time to really finish it. > Maybe you could rework your clock gate driver based on it. > > [0] https://github.com/981213/linux/commit/2eca1f045e4c3db18c941135464c0d7422ad8133 hsdma/eth/pio clocks are still missing in mediatek doc and I just made them up in the driver. Correct clock frequency for them aren't really important for them to work though. And another part I didn't finish is checking clock support for every drivers mt7621 used. Many drivers don't explicitly enable the clock and may be problematic when kernel gates unused clocks.
Hi Chuanhong, On Thu, Nov 12, 2020 at 2:26 AM Chuanhong Guo <gch981213@gmail.com> wrote: [snip] > > I've already said in previous threads that clock assignment in > current linux kernel is not trustworthy. > I've got the clock plan for mt7621 now. (Can't share it, sorry.) > Most of your clock assumptions above are incorrect. Well, that was of course expected, without a real clock plan this driver was only taking into account Oleksij Rempel suggestions to try to make a driver good enough to properly be maintained :). > I've made a clock driver with gate support a few months ago.[0] > but I don't have much time to really finish it. > Maybe you could rework your clock gate driver based on it. > > [0] https://github.com/981213/linux/commit/2eca1f045e4c3db18c941135464c0d7422ad8133 Thanks for the link, I see there are three more clocks there with frequencies of 125, 250 and 270 Mhz. all of them having main xtal as parent. Ok, I will take this real information into account and will send v2 after a bit of more feedback comes. > -- > Regards, > Chuanhong Guo Best regards, Sergio Paracuellos
Hi, On Thu, Nov 12, 2020 at 2:34 AM Chuanhong Guo <gch981213@gmail.com> wrote: > > On Thu, Nov 12, 2020 at 9:26 AM Chuanhong Guo <gch981213@gmail.com> wrote: > > > > I've already said in previous threads that clock assignment in > > current linux kernel is not trustworthy. > > I've got the clock plan for mt7621 now. (Can't share it, sorry.) > > Most of your clock assumptions above are incorrect. > > I've made a clock driver with gate support a few months ago.[0] > > but I don't have much time to really finish it. > > Maybe you could rework your clock gate driver based on it. > > > > [0] https://github.com/981213/linux/commit/2eca1f045e4c3db18c941135464c0d7422ad8133 > > hsdma/eth/pio clocks are still missing in mediatek doc and > I just made them up in the driver. Correct clock frequency for > them aren't really important for them to work though. > And another part I didn't finish is checking clock support for > every drivers mt7621 used. Many drivers don't explicitly > enable the clock and may be problematic when kernel > gates unused clocks. > Well, I think they are not important either. Also, by default gate register has all the gate bits enabled. When a gate driver is added, the kernel by default will try to disable those clocks that haven't been requested. To avoid weird behaviour because of some drivers are not using properly clocks we have the CLK_IGNORED_UNUSED, which as you can see is currently being used in my code. Using that all seems to work as expected as it is now. > -- > Regards, > Chuanhong Guo Best regards, Sergio Paracuellos
On Thu, Nov 12, 2020 at 1:23 PM Sergio Paracuellos <sergio.paracuellos@gmail.com> wrote: > > To avoid weird behaviour because of some drivers are > not using properly clocks we have the CLK_IGNORED_UNUSED, which as you > can see is currently being used in my code. Using that all seems to > work as expected as it is now. The whole point of having a clock gate driver is to gate unused clocks to save (maybe a tiny bit of) power. It's other peripheral drivers' fault that it doesn't enable clocks properly and we shouldn't just work-around the problem in the clock driver by disallowing auto clock gating.
On Fri, Nov 13, 2020 at 1:40 AM Chuanhong Guo <gch981213@gmail.com> wrote: > > On Thu, Nov 12, 2020 at 1:23 PM Sergio Paracuellos > <sergio.paracuellos@gmail.com> wrote: > > > > To avoid weird behaviour because of some drivers are > > not using properly clocks we have the CLK_IGNORED_UNUSED, which as you > > can see is currently being used in my code. Using that all seems to > > work as expected as it is now. > > The whole point of having a clock gate driver is to gate unused > clocks to save (maybe a tiny bit of) power. It's other peripheral > drivers' fault that it doesn't enable clocks properly and we shouldn't > just work-around the problem in the clock driver by disallowing auto > clock gating. > Totally agreed with what you are saying here but I don't really think using the flag is a workaround. It is just a way to ensure no regressions occurred until all drivers are adapted and also having all of them enabled is the behaviour. For me adapt the rest of driver code should be a different patch set after this driver is properly finished and mainlined. > -- > Regards, > Chuanhong Guo Best regards, Sergio Paracuellos