| Message ID | 20220515122409.13423-1-arzamas-16@mail.ee (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: mediatek: Fix unused 'ops' field in mtk_pll_data | expand |
Il 15/05/22 14:24, Boris Lysov ha scritto: > From: Boris Lysov <arzamas-16@mail.ee> > > Allow to specify optional clk_ops in mtk_pll_data which will be picked up in > mtk_clk_register_pll. So far no already supported Mediatek SoC needs > non-default clk_ops for PLLs but instead of removing this field it will be > actually used in the future for supporting older SoCs (see [1] for details) > with quirky PLLs. > Hello Boris, I disagree about this change and would rather see the ops pointer removed with fire. I got that you're trying to do something about "quirky PLLs", but is it really about the PLLs that you're mentioning being "quirky", or are they simply a different IP? Also, if it's just about a bit inversion and a bigger delay: 1. Bigger delay: Depending on how bigger, we may simply delay more by default for all PLLs, even the ones that aren't requiring us to wait for longer... ...after all, if it's about waiting for 10/20 *microseconds* more, that's really not going to affect anyone's UX, nor make things slower for real, as the .prepare() ops for MediaTek PLLs are seldom called.. and even if that wasn't true, I don't think that a total of 30uS would be that much detrimental to the system's overall operation latency. Besides, if you see a case of a PLL not just switching on and off, but preparing and unpreparing continuously, there must be some big issue in some driver, or in the clock framework somewhere (and that ain't the case); 2. Bit inversion: that can be solved simply with a flag in the prepare/unprepare ops for this driver... and if you want something that performs even better, sparing you a nanosecond or two, you can always assign an "inverted" callback for managing that single bit; 3. Different IP: mtk_clk_register_(name-of-the-new-ip)_pll() - I don't think that there's anything to explain to that one. Regards, Angelo > This patch depends on series "clk: mediatek: Move to struct clk_hw provider > APIs" [2] by Chen-Yu Tsai. > > [1] https://lists.infradead.org/pipermail/linux-mediatek/2022-February/035093.html > [2] https://lists.infradead.org/pipermail/linux-mediatek/2022-May/040921.html > > Signed-off-by: Boris Lysov <arzamas-16@mail.ee> > --- > drivers/clk/mediatek/clk-pll.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c > index cabdf25a27f3..509959a325f0 100644 > --- a/drivers/clk/mediatek/clk-pll.c > +++ b/drivers/clk/mediatek/clk-pll.c > @@ -347,7 +347,10 @@ static struct clk_hw *mtk_clk_register_pll(const struct mtk_pll_data *data, > > init.name = data->name; > init.flags = (data->flags & PLL_AO) ? CLK_IS_CRITICAL : 0; > - init.ops = &mtk_pll_ops; > + if (data->ops) > + init.ops = data->ops; > + else > + init.ops = &mtk_pll_ops; > if (data->parent_name) > init.parent_names = &data->parent_name; > else >
Hello, Angelo! On Wed, 18 May 2022 14:15:13 +0200 AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > Il 15/05/22 14:24, Boris Lysov ha scritto: > > From: Boris Lysov <arzamas-16@mail.ee> > > > > Allow to specify optional clk_ops in mtk_pll_data which will be picked up in > > mtk_clk_register_pll. So far no already supported Mediatek SoC needs > > non-default clk_ops for PLLs but instead of removing this field it will be > > actually used in the future for supporting older SoCs (see [1] for details) > > with quirky PLLs. > > > > Hello Boris, > > I disagree about this change and would rather see the ops pointer removed > with fire. > > I got that you're trying to do something about "quirky PLLs", but is it > really about the PLLs that you're mentioning being "quirky", or are they > simply a different IP? To be honest I don't know exactly. mt6577 seems to share some common IP patterns such as splitting the entire clock system into few smaller subsystems such as apmixed (PLLs), topckgen (mux control), infra- and pericfg (internal and peripheral gate control). On the other hand, mt6577 is quite an old SoC (more on that in the end) and there are some differences about its operation compared to modern SoCs and their drivers. > Also, if it's just about a bit inversion and a bigger delay: > 1. Bigger delay: Depending on how bigger, we may simply delay more by default > for all PLLs, even the ones that aren't requiring us to wait for longer... > ...after all, if it's about waiting for 10/20 *microseconds* more > { snip } According to the mt6577 datasheet the largest settling time is 10 *milli*seconds for AUDPLL [1]. In my opinion this is way too much to be set as default for all mediatek devices. > 2. Bit inversion: that can be solved simply with a flag in the > prepare/unprepare ops for this driver... and if you want something that > performs even better, sparing you a nanosecond or two, you can always assign > an "inverted" callback for managing that single bit; Not all mt6577 PLLs need bit inversion. 2 PLLs follow the common flow (set a CON0_PWR_ON bit to start). 6 PLLs set this bit to 0 to start. And 1 PLL (which is actually a DDS) needs to write a magic value to specific register (in apmixed region) to start. Is very unfortunate that I can't directly link the vomit-inducing downstream code to prove the PLL situation due to its licensing but it's publicly available on the internet [2] as a part of device manufacturers' obligations to publish source code. > 3. Different IP: mtk_clk_register_(name-of-the-new-ip)_pll() - I don't think > that there's anything to explain to that one. In my opinion this would introduce more duplicate code than just letting a developer set custom clk_ops for a specific platform. Huge thanks for your feedback! P.S As I said above, mt6577 is old and in its current state [3] it's closer to being a personal project than a serious mainlining attempt. I share and agree with your opinion [4] on e-waste and is why I'm trying to put an effort into it. What I don't have enough of is time and, sadly, expertise. Maybe it'd be better for me to stay on github. [1] MT6577 HSPA Smartphone Application Processor Datasheet v0.94, page 1200 [2] any linux v3.4 mt6577 kernel on github, see the 'enable_pll_op' function in mediatek/platform/mt6577/kernel/core/mt6577_clock_manager.c [3] https://github.com/arzam16/linux-mt6577 [4] https://lists.infradead.org/pipermail/linux-mediatek/2022-May/041498.html > Regards, > Angelo > > > This patch depends on series "clk: mediatek: Move to struct clk_hw provider > > APIs" [2] by Chen-Yu Tsai. > > > > [1] > > https://lists.infradead.org/pipermail/linux-mediatek/2022-February/035093.html > > [2] > > https://lists.infradead.org/pipermail/linux-mediatek/2022-May/040921.html > > > > Signed-off-by: Boris Lysov <arzamas-16@mail.ee> > > --- > > drivers/clk/mediatek/clk-pll.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c > > index cabdf25a27f3..509959a325f0 100644 > > --- a/drivers/clk/mediatek/clk-pll.c > > +++ b/drivers/clk/mediatek/clk-pll.c > > @@ -347,7 +347,10 @@ static struct clk_hw *mtk_clk_register_pll(const > > struct mtk_pll_data *data, > > init.name = data->name; > > init.flags = (data->flags & PLL_AO) ? CLK_IS_CRITICAL : 0; > > - init.ops = &mtk_pll_ops; > > + if (data->ops) > > + init.ops = data->ops; > > + else > > + init.ops = &mtk_pll_ops; > > if (data->parent_name) > > init.parent_names = &data->parent_name; > > else > > >
Il 19/05/22 21:27, Boris Lysov ha scritto: > Hello, Angelo! > > On Wed, 18 May 2022 14:15:13 +0200 > AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > >> Il 15/05/22 14:24, Boris Lysov ha scritto: >>> From: Boris Lysov <arzamas-16@mail.ee> >>> >>> Allow to specify optional clk_ops in mtk_pll_data which will be picked up in >>> mtk_clk_register_pll. So far no already supported Mediatek SoC needs >>> non-default clk_ops for PLLs but instead of removing this field it will be >>> actually used in the future for supporting older SoCs (see [1] for details) >>> with quirky PLLs. >>> >> >> Hello Boris, >> >> I disagree about this change and would rather see the ops pointer removed >> with fire. >> >> I got that you're trying to do something about "quirky PLLs", but is it >> really about the PLLs that you're mentioning being "quirky", or are they >> simply a different IP? > > To be honest I don't know exactly. mt6577 seems to share some common IP > patterns such as splitting the entire clock system into few smaller subsystems > such as apmixed (PLLs), topckgen (mux control), infra- and pericfg (internal > and peripheral gate control). On the other hand, mt6577 is quite an old SoC > (more on that in the end) and there are some differences about its operation > compared to modern SoCs and their drivers. > >> Also, if it's just about a bit inversion and a bigger delay: >> 1. Bigger delay: Depending on how bigger, we may simply delay more by default >> for all PLLs, even the ones that aren't requiring us to wait for longer... >> ...after all, if it's about waiting for 10/20 *microseconds* more >> { snip } > > According to the mt6577 datasheet the largest settling time is 10 > *milli*seconds for AUDPLL [1]. In my opinion this is way too much to be set as > default for all mediatek devices. > Wow. 10ms is a huge wait! That doesn't *feel* right, but if that's what it is... Perhaps we should look into that AUDPLL matter later, as audio is one kind of functionality that you want to enable in the immediate term - I agree it's a nice to have, but bringing up the platform comes first, and this means the top clock controllers and eventually multimedia (to get a display up!). >> 2. Bit inversion: that can be solved simply with a flag in the >> prepare/unprepare ops for this driver... and if you want something that >> performs even better, sparing you a nanosecond or two, you can always assign >> an "inverted" callback for managing that single bit; > > Not all mt6577 PLLs need bit inversion. 2 PLLs follow the common flow (set a > CON0_PWR_ON bit to start). 6 PLLs set this bit to 0 to start. And 1 PLL (which > is actually a DDS) needs to write a magic value to specific register (in > apmixed region) to start. That's interesting. > Is very unfortunate that I can't directly link the vomit-inducing downstream > code to prove the PLL situation due to its licensing but it's publicly > available on the internet [2] as a part of device manufacturers' obligations to > publish source code. > >> 3. Different IP: mtk_clk_register_(name-of-the-new-ip)_pll() - I don't think >> that there's anything to explain to that one. > In my opinion this would introduce more duplicate code than just letting a > developer set custom clk_ops for a specific platform. > > Huge thanks for your feedback! > > P.S As I said above, mt6577 is old and in its current state [3] it's closer to > being a personal project than a serious mainlining attempt. I share and agree > with your opinion [4] on e-waste and is why I'm trying to put an effort into it. > What I don't have enough of is time and, sadly, expertise. Maybe it'd be better > for me to stay on github. > Your contribution is definitely welcome and you shouldn't worry about having a relatively low amount of time to spend on these things - that's a very common (non)issue among contributors. A community works like a community: it's not everything on your shoulders! That's why the maintainers are here, that's why I'm here and that's also why other people are here. Everyone of us gives some little bit and, at the end of the day, all these little bits come together to form an entire kernel :-) Therefore, I encourage you (and anyone else reading this) to keep sharing your valuable contributions to the mailing lists as much as you want to. That said, let's get back in topic, shall we? I feel the various concerns that you've raised relative to this PLL matter, I know that older MediaTek SoCs look a bit strange in regard to some clocks and other things, but that obviously doesn't mean that there's no way to do things properly or anyway in a way better manner. Here's my proposal: Try upstreaming the top, very necessary, clocks to achieve a full console boot, as this gives you a good chance to start landing some solid and critical base for your platform. The top clocks should have most of the PLLs, but you can avoid adding some that are a bit problematic, such as that AUDPLL that you were talking about... in my opinion, at least, it's not a big deal if we add these PLLs later when enabling more functionality: that will give you the chance to add the PLLs that are in need of that "enable bit inversion" logic which, from what I understand from your words, covers 6 to 8 PLLs. That's a lot, because that makes you able to add all of the clocks that are in infra, top, mfg, apmixed: like that, you're also getting most of the IP up (timers, i2c, spi, mtk-sd for your eMMC/uSD). Do it step by step - me and other developers will give you reviews and probably make you go through some iterations of your driver, and that's to make sure that your contribution will be valuable for a long time (and will not get broken, and will not be useless after few months). Cheers, Angelo
On Fri, 20 May 2022 10:27:45 +0200 AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > Il 19/05/22 21:27, Boris Lysov ha scritto: > > Hello, Angelo! > > > > On Wed, 18 May 2022 14:15:13 +0200 > > AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > > > >> Il 15/05/22 14:24, Boris Lysov ha scritto: > >>> From: Boris Lysov <arzamas-16@mail.ee> > >>> > >>> Allow to specify optional clk_ops in mtk_pll_data which will be picked up > >>> in mtk_clk_register_pll. So far no already supported Mediatek SoC needs > >>> non-default clk_ops for PLLs but instead of removing this field it will be > >>> actually used in the future for supporting older SoCs (see [1] for > >>> details) with quirky PLLs. > >> > >> { snip } > > > >> Also, if it's just about a bit inversion and a bigger delay: > >> 1. Bigger delay: Depending on how bigger, we may simply delay more by > >> default for all PLLs, even the ones that aren't requiring us to wait for > >> longer... ...after all, if it's about waiting for 10/20 *microseconds* more > >> { snip } > > > > According to the mt6577 datasheet the largest settling time is 10 > > *milli*seconds for AUDPLL [1]. In my opinion this is way too much to be set > > as default for all mediatek devices. > > > > { snip } Perhaps we should look into that AUDPLL matter later, as audio is one > kind of functionality that you want to enable in the immediate term Not gonna lie, I also though so about TVDDS, too. Because considering how unimportant AUDPLL and TVDDS are for basic system usability, I'd rather not put effort into them at first stages. Afaik, TVDDS is barely used even in the genuine downstream code! I just didn't know if it would be okay to make partial PLL support. But now... > The top clocks should have most of the PLLs, but you can avoid adding some > that are a bit problematic, such as that AUDPLL that you were talking > about... in my opinion, at least, it's not a big deal if we add these PLLs > later when enabling more functionality ...I understand, thanks! > - I agree it's a nice to have, but bringing up the platform comes first, and > this means the top clock controllers and eventually multimedia (to get a > display up!). > { snip } Try upstreaming the top, very necessary, clocks to achieve a full > console boot, as this gives you a good chance to start landing some solid and > critical base for your platform. In case of (at least some) mt6577 devices a full console boot is possible even without the clock driver thanks to the work preloader and Uboot do on that matter. If you open https://github.com/arzam16/linux-mt6577 you will see that with fixed-clocks in dts it's possible to start a framebuffer console and even a simple game! On the other hand, I believe implementing a proper mainline clock driver is a must for further development. Hoping for the ancient UBoot filled to the brim with proprietary components to do all the work is not the best idea. > >> 2. Bit inversion: that can be solved simply with a flag in the > >> prepare/unprepare ops for this driver... and if you want something that > >> performs even better, sparing you a nanosecond or two, you can always > >> assign an "inverted" callback for managing that single bit; > > > > Not all mt6577 PLLs need bit inversion. 2 PLLs follow the common flow (set a > > CON0_PWR_ON bit to start). 6 PLLs set this bit to 0 to start. And 1 PLL > > (which is actually a DDS) needs to write a magic value to specific register > > (in apmixed region) to start. > > That's interesting. > { snip } > The top clocks should have most of the PLLs, but you can avoid adding some > that are a bit problematic, such as that AUDPLL that you were talking > about... in my opinion, at least, it's not a big deal if we add these PLLs > later when enabling more functionality: that will give you the chance to add > the PLLs that are in need of that "enable bit inversion" logic which, from > what I understand from your words, covers 6 to 8 PLLs. To put it clear: there are 9 PLLs in mt6577 in total: 1. ARMPLL - write 0 to enable (need bit inversion), important PLL 2. MAINPLL - write 0 to enable (need bit inv.), important PLL 3. IPLL - write 0 to enable (need bit inv.), for "image sensing" subsystem 4. UPLL - write 0 to enable (need bit inv.), not sure what it's for 5. MDPLL - write 0 to enable (need bit inv.), for "modem" subsystem 6. TVDDS - write magic value to enable, for TV-related IP and HDMI 7. WPLL - write 1 to enable, for "modem" subsystem 8. AUDPLL - write 1 to enable, for "audio" subsystem 9. MEMPLL - write 0 to enable (need bit inv.), for "ddr" subsystem As you can see, the most important PLLs (ARM and MAIN) need bit inversion and is why I decided to use 'ops' field. > That's a lot, because that makes you able to > add all of the clocks that are in infra, top, mfg, apmixed: like that, you're > also getting most of the IP up (timers, i2c, spi, mtk-sd for your eMMC/uSD). Here's where it gets interesting. Afaiu mainline kernel expects me to provide all the info on the clock heirarchy. In my case it's almost impossible to figure out any relation between the PLLs and MUXes and clocks. mt6577 datasheet doesn't have any meaningful clock hierarchy diagrams unlike more modern SoCs. The downstream code works in a "fire everything up on boot and don't care" way, it relies on undocumented registers for clock management and also doesn't have any sane clues to figure out the hierarchy. Even the frequency of the core fixed clock is not known clearly - it's either 13 or 26 or 260 MHz - downstream code doesn't give the exact answer (my mainline kernel works fine with 13 MHz, though). This is going to be *hell* to implement in mainline. Related: [1] [1] https://www.spinics.net/lists/linux-clk/msg65449.html
diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c index cabdf25a27f3..509959a325f0 100644 --- a/drivers/clk/mediatek/clk-pll.c +++ b/drivers/clk/mediatek/clk-pll.c @@ -347,7 +347,10 @@ static struct clk_hw *mtk_clk_register_pll(const struct mtk_pll_data *data, init.name = data->name; init.flags = (data->flags & PLL_AO) ? CLK_IS_CRITICAL : 0; - init.ops = &mtk_pll_ops; + if (data->ops) + init.ops = data->ops; + else + init.ops = &mtk_pll_ops; if (data->parent_name) init.parent_names = &data->parent_name; else