Re: [PATCH v1 1/2] clk: intel: Add CGU clock driver for a new SoC

[Martin Blumenstingl]

Hi Rahul,

On Tue, Sep 3, 2019 at 11:54 AM Tanwar, Rahul
<rahul.tanwar@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > + */
> > +
> > +#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/