lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20190902222015.11360-1-martin.blumenstingl@googlemail.com>
Date:   Tue,  3 Sep 2019 00:20:15 +0200
From:   Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To:     rahul.tanwar@...ux.intel.com
Cc:     andriy.shevchenko@...el.com, cheol.yong.kim@...el.com,
        linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
        mark.rutland@....com, mturquette@...libre.com,
        qi-ming.wu@...el.com, rahul.tanwar@...el.com, robh+dt@...nel.org,
        robhkernel.org@...r.kernel.org, sboyd@...nel.org,
        yixin.zhu@...ux.intel.com
Subject: RE: [PATCH v1 1/2] clk: intel: Add CGU clock driver for a new SoC

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

+	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

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


Martin


[0] https://lkml.org/lkml/2019/8/27/849

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ