Messages in this thread | | | From | Martin Blumenstingl <> | Date | Tue, 3 Sep 2019 20:53:12 +0200 | Subject | Re: [PATCH v1 1/2] clk: intel: Add CGU clock driver for a new SoC |
| |
Hi Rahul,
On Tue, Sep 3, 2019 at 11:54 AM Tanwar, Rahul <rahul.tanwar@linux.intel.com> wrote: > > > Hi Martin, > > On 3/9/2019 6:20 AM, Martin Blumenstingl wrote: > > Hello, > > > > I only noticed this patchset today and I don't have much time left. > > Here's my initial impressions without going through the code in detail. > > I'll continue my review in the next days (as time permits). > > > > As with all other Intel LGM patches: I don't have access to the > > datasheets, so it's possible that I don't understand <insert topic here> > > feel free to correct me in this case (I appreciate an explanation where > > I was wrong, so I can learn from it) > > > > > > [...] > > --- /dev/null > > +++ b/drivers/clk/intel/Kconfig > > @@ -0,0 +1,13 @@ > > +# SPDX-License-Identifier: GPL-2.0 > > +config INTEL_LGM_CGU_CLK > > + depends on COMMON_CLK > > + select MFD_SYSCON > > can you please explain the reason why you need to use syscon? > > also please see [0] for a comment from Rob on another LGM dt-binding > > regarding syscon > > > Actually, there is no need to use syscon for CGU in LGM. It got carried > over from older SoCs (Falcon Mountain) where CGU was a MFD device > because it included PHY registers as well. And PHY drivers were using > syscon node to access CGU regmap. But for LGM, this is not the case. I see, to me it seems like LGM got a nice set of register cleanups! so I'm all for dropping the syscon compatible
> My understanding is that if we do not use syscon, then there is no > point in using regmap because this driver uses simple 32 bit register > access. Can directly read/write registers using readl() & writel(). > > Would you agree ? if there was only the LGM SoC then I would say: drop regmap
however, last year a driver for the GRX350/GRX550 SoCs was proposed: [0] this was never updated but it seems to use the same "framework" as the LGM driver with this in mind I am for keeping regmap support because. I think it will be easier to add support for old SoCs like GRX350/GRX550 (but also VRX200), because the PLL sub-driver (I am assuming that it is similar on all SoCs) or some other helpers can be re-used across various SoCs instead of "duplicating" code (where one variant would use regmap and the other readl/writel).
[...] > > + select OF_EARLY_FLATTREE > > there's not a single other "select OF_EARLY_FLATTREE" in driver/clk > > I'm not saying this is wrong but it makes me curious why you need this > > > We need OF_EARLY_FLATTREE for LGM. But adding a new x86 > platform for LGM is discouraged because that would lead to too > many platforms. Only differentiating factor for LGM is CPU model > ID but it can differentiate only at run time. So i had no option > other then enabling it with some LGM specific core system module > driver and CGU seemed perfect for this purpose. so when my x86 kernel maintainer enables CONFIG_INTEL_LGM_CGU_CLK then OF_EARLY_FLATTREE is enabled as well. does this hurt any existing x86 platform? if not: why can't we enable it for x86 unconditionally?
[...] > > diff --git a/drivers/clk/intel/clk-cgu.h b/drivers/clk/intel/clk-cgu.h > > new file mode 100644 > > index 000000000000..e44396b4aad7 > > --- /dev/null > > +++ b/drivers/clk/intel/clk-cgu.h > > @@ -0,0 +1,278 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright(c) 2018 Intel Corporation. > > + * Zhu YiXin <Yixin.zhu@intel.com> > > + */ > > + > > +#ifndef __INTEL_CLK_H > > +#define __INTEL_CLK_H > > + > > +struct intel_clk_mux { > > + struct clk_hw hw; > > + struct device *dev; > > + struct regmap *map; > > + unsigned int reg; > > + u8 shift; > > + u8 width; > > + unsigned long flags; > > +}; > > + > > +struct intel_clk_divider { > > + struct clk_hw hw; > > + struct device *dev; > > + struct regmap *map; > > + unsigned int reg; > > + u8 shift; > > + u8 width; > > + unsigned long flags; > > + const struct clk_div_table *table; > > +}; > > + > > +struct intel_clk_ddiv { > > + struct clk_hw hw; > > + struct device *dev; > > + struct regmap *map; > > + unsigned int reg; > > + u8 shift0; > > + u8 width0; > > + u8 shift1; > > + u8 width1; > > + u8 shift2; > > + u8 width2; > > + unsigned int mult; > > + unsigned int div; > > + unsigned long flags; > > +}; > > + > > +struct intel_clk_gate { > > + struct clk_hw hw; > > + struct device *dev; > > + struct regmap *map; > > + unsigned int reg; > > + u8 shift; > > + unsigned long flags; > > +}; > > I know at least two existing regmap clock implementations: > > - drivers/clk/qcom/clk-regmap* > > - drivers/clk/meson/clk-regmap* > > > > it would be great if we could decide to re-use one of those for the > > "generic" clock types (mux, divider and gate). > > Stephen, do you have any preference here? > > personally I like the meson one, but I'm biased because I've used it > > a lot in the past and I haven't used the qcom one at all. > > > I went through meson & qcom regmap clock code. Agree, it can be > reused for mux, divider and gate. But as mentioned above, i am now > considering to move away from using regmap. thank you for evaluating them. let's continue the discussion above whether regmap should be used - after that we decide (if needed) which regmap implementation to use
Martin
[0] https://patchwork.kernel.org/patch/10554401/
| |