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

[Tanwar, Rahul]

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.

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 ?


Yi Xin, please correct me if you think i am mistaken anywhere. If not,

i will change the driver to not use regmap & instead use 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.


> [...]
> 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.

Regards,

Rahul