[PATCH v3 00/17] Add support for Mobileye EyeQ5 system controller

[PATCH v3 00/17] Add support for Mobileye EyeQ5 system controller

[Théo Lebrun]

Hi,

The goal of this series is to add clk, reset and pinctrl support for the
Mobileye EyeQ5 platform [0]. Control of those is grouped inside a
system controller block called "OLB".

About clocks, we replaced the 10 fixed clocks from the initial platform
support series [0] by 10 read-only fixed-factor PLLs provided by our
clock driver. We also provide one table-based divider clock for OSPI.
Two PLLs (for GIC timer & UARTs) are required at of_clk_init() so those
are registered first, the rest comes at platform device probe.

Resets are split in three domains, all dealt with by the same device.
They have some behavior differences:
 - We busy-wait on the first two for hardware LBIST reasons (logic
   built-in self-test).
 - Domains 0 & 2 work in a bit-per-reset fashion while domain 1 works in
   a register-per-reset fashion.

Pin control is about controlling bias, drive strength and muxing. The
latter allows two functions per pin; the first function is always GPIO
while the second one is pin-dependent. There exists two banks, each
handled in a separate driver instance. Each pin maps to one pin group.
That makes pin & group indexes the same, simplifying logic.

This series inherits from the clk V2 [1], reset V1 [2] and pinctrl V1
[3]. Those were unified to simplify handling of dt-bindings. It is
based on the series "[PATCH v6 00/15] Add support for the Mobileye
EyeQ5 SoC" [0] rebased onto v6.8-rc1.

Here is the patch list, split by subsystems:

 - clk:
    - [PATCH V3 01/17] clk: fixed-factor: add optional accuracy support
    - [PATCH V3 02/17] clk: fixed-factor: add fwname-based constructor functions
    - [PATCH V3 05/17] dt-bindings: clock: mobileye,eyeq5-clk: add bindings
    - [PATCH V3 08/17] clk: eyeq5: add platform driver

 - pinctrl:
    - [PATCH V3 03/17] dt-bindings: pinctrl: allow pin controller device without unit address
    - [PATCH V3 07/17] dt-bindings: pinctrl: mobileye,eyeq5-pinctrl: add bindings
    - [PATCH V3 10/17] pinctrl: eyeq5: add platform driver

 - MIPS: (note: dependent on the [0] series)
    - [PATCH V3 04/17] dt-bindings: soc: mobileye: add EyeQ5 OLB system controller
    - [PATCH V3 11/17] MIPS: mobileye: eyeq5: rename olb@e00000 to system-controller@e00000
    - [PATCH V3 12/17] MIPS: mobileye: eyeq5: remove reg-io-width property from OLB syscon
    - [PATCH V3 13/17] MIPS: mobileye: eyeq5: use OLB clocks controller
    - [PATCH V3 14/17] MIPS: mobileye: eyeq5: add OLB reset controller node
    - [PATCH V3 15/17] MIPS: mobileye: eyeq5: add reset properties to uarts
    - [PATCH V3 16/17] MIPS: mobileye: eyeq5: add pinctrl nodes & pinmux function nodes
    - [PATCH V3 17/17] MIPS: mobileye: eyeq5: add pinctrl properties to UART nodes

 - reset:
    - [PATCH V3 06/17] dt-bindings: reset: mobileye,eyeq5-reset: add bindings
    - [PATCH V3 09/17] reset: eyeq5: add platform driver

Thanks to Krzysztof, Rob & Stephen for the previous feedback!

Have a nice day,
Théo Lebrun

[0]: https://lore.kernel.org/lkml/20240118155252.397947-1-gregory.clement@bootlin.com/
[1]: https://lore.kernel.org/lkml/20231227-mbly-clk-v2-0-a05db63c380f@bootlin.com/
[2]: https://lore.kernel.org/lkml/20231218-mbly-reset-v1-0-b4688b916213@bootlin.com/
[3]: https://lore.kernel.org/lkml/20231218-mbly-pinctrl-v1-0-2f7d366c2051@bootlin.com/

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
Changes in v3:
- Unified the three series into one.
- clk: split driver into two for clocks registered at of_clk_init() and
  clocks registered at platform device probe.
- reset/bindings: drop reset dt-bindings header & add comment in driver
  to document known valid resets in each domain.
- pinctrl/bindings: fix pinctrl.yaml to allow non unit addresses for pin
  controller devices.
- all/bindings: remove possibility to use `mobileye,olb` phandle to get
  syscon. All three drivers use their parent node as syscon/regmap.
- MIPS/bindings: fix bindings for OLB. Have single example in parent,
  removing all examples in child.
- all: drop the "probed" logs.
- Link to v2: https://lore.kernel.org/r/20231227-mbly-clk-v2-0-a05db63c380f@bootlin.com

Changes in v2:
- Drop [PATCH 1/5] that was taken by Stephen for clk-next.
- Add accuracy support to fixed-factor that is enabled with a flag.
  Register prototypes were added to exploit this feature.
- Add fw_name support to fixed-factor. This allows pointing to parent
  clocks using the value in `clock-names` in the DT. Register
  prototypes were added for that.
- Bindings were modified to be less dumb: a binding was added for OLB
  and the clock-controller is a child property of it. Removed the
  possibility of pointing to OLB using a phandle. $nodename is the
  generic `clock-controller` and not custom `clocks`. Fix dt-bindings
  examples.
- Fix commit message for the driver patch. Add details, remove useless
  fluff.
- Squash both driver commits together.
- Declare a platform_driver instead of using CLK_OF_DECLARE_DRIVER. This
  also means using `dev_*` for logging, removing `pr_fmt`. We add a
  pointer to device in the private structure.
- Use fixed-factor instead of fixed-rate for PLLs. We don't grab a
  reference to the parent clk, instead using newly added fixed-factor
  register prototypes and fwname.
- NULL is not an error when registering PLLs anymore.
- Now checking the return value of of_clk_add_hw_provider for errors.
- Fix includes.
- Remove defensive conditional at start of eq5c_pll_parse_registers.
- Rename clk_hw_to_ospi_priv to clk_to_priv to avoid confusion: it is
  not part of the clk_hw_* family of symbols.
- Fix negative returns in eq5c_ospi_div_set_rate. It was a typo
  highlighted by Stephen Boyd.
- Declare eq5c_ospi_div_ops as static.
- In devicetree, move the OLB node prior to the UARTs, as platform
  device probe scheduling is dependent on devicetree ordering. This is
  required to declare the driver as a platform driver, else it
  CLK_OF_DECLARE_DRIVER is required.
- In device, create a core0-timer-clk fixed clock to feed to the GIC
  timer. It requires a clock earlier than platform bus type init.
- Link to v1: https://lore.kernel.org/r/20231218-mbly-clk-v1-0-44ce54108f06@bootlin.com

---
Théo Lebrun (17):
      clk: fixed-factor: add optional accuracy support
      clk: fixed-factor: add fwname-based constructor functions
      dt-bindings: pinctrl: allow pin controller device without unit address
      dt-bindings: soc: mobileye: add EyeQ5 OLB system controller
      dt-bindings: clock: mobileye,eyeq5-clk: add bindings
      dt-bindings: reset: mobileye,eyeq5-reset: add bindings
      dt-bindings: pinctrl: mobileye,eyeq5-pinctrl: add bindings
      clk: eyeq5: add platform driver
      reset: eyeq5: add platform driver
      pinctrl: eyeq5: add platform driver
      MIPS: mobileye: eyeq5: rename olb@e00000 to system-controller@e00000
      MIPS: mobileye: eyeq5: remove reg-io-width property from OLB syscon
      MIPS: mobileye: eyeq5: use OLB clocks controller
      MIPS: mobileye: eyeq5: add OLB reset controller node
      MIPS: mobileye: eyeq5: add reset properties to uarts
      MIPS: mobileye: eyeq5: add pinctrl nodes & pinmux function nodes
      MIPS: mobileye: eyeq5: add pinctrl properties to UART nodes

 .../bindings/clock/mobileye,eyeq5-clk.yaml         |  41 ++
 .../bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml   |  77 +++
 .../devicetree/bindings/pinctrl/pinctrl.yaml       |  18 +-
 .../bindings/reset/mobileye,eyeq5-reset.yaml       |  32 ++
 .../bindings/soc/mobileye/mobileye,eyeq5-olb.yaml  |  77 +++
 MAINTAINERS                                        |   8 +
 .../{eyeq5-fixed-clocks.dtsi => eyeq5-clocks.dtsi} |  54 +-
 arch/mips/boot/dts/mobileye/eyeq5-pins.dtsi        | 128 +++++
 arch/mips/boot/dts/mobileye/eyeq5.dtsi             |  37 +-
 drivers/clk/Kconfig                                |  11 +
 drivers/clk/Makefile                               |   1 +
 drivers/clk/clk-eyeq5.c                            | 414 ++++++++++++++
 drivers/clk/clk-fixed-factor.c                     | 103 +++-
 drivers/pinctrl/Kconfig                            |  15 +
 drivers/pinctrl/Makefile                           |   1 +
 drivers/pinctrl/pinctrl-eyeq5.c                    | 595 +++++++++++++++++++++
 drivers/reset/Kconfig                              |  12 +
 drivers/reset/Makefile                             |   1 +
 drivers/reset/reset-eyeq5.c                        | 383 +++++++++++++
 include/dt-bindings/clock/mobileye,eyeq5-clk.h     |  22 +
 include/linux/clk-provider.h                       |  26 +-
 21 files changed, 1995 insertions(+), 61 deletions(-)
---
base-commit: 84f23245916391a55be31e37e48cea4da085b100
change-id: 20231023-mbly-clk-87ce5c241f08

Best regards,
-- 
Théo Lebrun <theo.lebrun@bootlin.com>

[PATCH v3 01/17] clk: fixed-factor: add optional accuracy support

[Théo Lebrun]

Fixed factor clock reports the parent clock accuracy. Add flags and acc
fields to `struct clk_fixed_factor` to support setting a fixed
accuracy. The default if no flag is set is not changed: use the parent
clock accuracy.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/clk/clk-fixed-factor.c | 28 +++++++++++++++++++++-------
 include/linux/clk-provider.h   | 12 +++++++++++-
 2 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
index b3e66202b942..bc2644a9bd7d 100644
--- a/drivers/clk/clk-fixed-factor.c
+++ b/drivers/clk/clk-fixed-factor.c
@@ -57,10 +57,22 @@ static int clk_factor_set_rate(struct clk_hw *hw, unsigned long rate,
 	return 0;
 }
 
+static unsigned long clk_factor_recalc_accuracy(struct clk_hw *hw,
+						unsigned long parent_accuracy)
+{
+	struct clk_fixed_factor *fix = to_clk_fixed_factor(hw);
+
+	if (fix->flags & CLK_FIXED_FACTOR_FIXED_ACCURACY)
+		return fix->acc;
+
+	return parent_accuracy;
+}
+
 const struct clk_ops clk_fixed_factor_ops = {
 	.round_rate = clk_factor_round_rate,
 	.set_rate = clk_factor_set_rate,
 	.recalc_rate = clk_factor_recalc_rate,
+	.recalc_accuracy = clk_factor_recalc_accuracy,
 };
 EXPORT_SYMBOL_GPL(clk_fixed_factor_ops);
 
@@ -81,7 +93,7 @@ __clk_hw_register_fixed_factor(struct device *dev, struct device_node *np,
 		const char *name, const char *parent_name,
 		const struct clk_hw *parent_hw, int index,
 		unsigned long flags, unsigned int mult, unsigned int div,
-		bool devm)
+		unsigned long acc, unsigned int fixflags, bool devm)
 {
 	struct clk_fixed_factor *fix;
 	struct clk_init_data init = { };
@@ -105,6 +117,8 @@ __clk_hw_register_fixed_factor(struct device *dev, struct device_node *np,
 	fix->mult = mult;
 	fix->div = div;
 	fix->hw.init = &init;
+	fix->acc = acc;
+	fix->flags = fixflags;
 
 	init.name = name;
 	init.ops = &clk_fixed_factor_ops;
@@ -152,7 +166,7 @@ struct clk_hw *devm_clk_hw_register_fixed_factor_index(struct device *dev,
 		unsigned int mult, unsigned int div)
 {
 	return __clk_hw_register_fixed_factor(dev, NULL, name, NULL, NULL, index,
-					      flags, mult, div, true);
+					      flags, mult, div, 0, 0, true);
 }
 EXPORT_SYMBOL_GPL(devm_clk_hw_register_fixed_factor_index);
 
@@ -174,7 +188,7 @@ struct clk_hw *devm_clk_hw_register_fixed_factor_parent_hw(struct device *dev,
 		unsigned long flags, unsigned int mult, unsigned int div)
 {
 	return __clk_hw_register_fixed_factor(dev, NULL, name, NULL, parent_hw,
-					      -1, flags, mult, div, true);
+					      -1, flags, mult, div, 0, 0, true);
 }
 EXPORT_SYMBOL_GPL(devm_clk_hw_register_fixed_factor_parent_hw);
 
@@ -184,7 +198,7 @@ struct clk_hw *clk_hw_register_fixed_factor_parent_hw(struct device *dev,
 {
 	return __clk_hw_register_fixed_factor(dev, NULL, name, NULL,
 					      parent_hw, -1, flags, mult, div,
-					      false);
+					      0, 0, false);
 }
 EXPORT_SYMBOL_GPL(clk_hw_register_fixed_factor_parent_hw);
 
@@ -193,7 +207,7 @@ struct clk_hw *clk_hw_register_fixed_factor(struct device *dev,
 		unsigned int mult, unsigned int div)
 {
 	return __clk_hw_register_fixed_factor(dev, NULL, name, parent_name, NULL, -1,
-					      flags, mult, div, false);
+					      flags, mult, div, 0, 0, false);
 }
 EXPORT_SYMBOL_GPL(clk_hw_register_fixed_factor);
 
@@ -240,7 +254,7 @@ struct clk_hw *devm_clk_hw_register_fixed_factor(struct device *dev,
 		unsigned int mult, unsigned int div)
 {
 	return __clk_hw_register_fixed_factor(dev, NULL, name, parent_name, NULL, -1,
-			flags, mult, div, true);
+			flags, mult, div, 0, 0, true);
 }
 EXPORT_SYMBOL_GPL(devm_clk_hw_register_fixed_factor);
 
@@ -267,7 +281,7 @@ static struct clk_hw *_of_fixed_factor_clk_setup(struct device_node *node)
 	of_property_read_string(node, "clock-output-names", &clk_name);
 
 	hw = __clk_hw_register_fixed_factor(NULL, node, clk_name, NULL, NULL, 0,
-					    0, mult, div, false);
+					    0, mult, div, 0, 0, false);
 	if (IS_ERR(hw)) {
 		/*
 		 * Clear OF_POPULATED flag so that clock registration can be
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 1293c38ddb7f..7ddc952c8c67 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -1084,18 +1084,28 @@ void of_fixed_factor_clk_setup(struct device_node *node);
  * @hw:		handle between common and hardware-specific interfaces
  * @mult:	multiplier
  * @div:	divider
+ * @acc:	fixed accuracy in ppb
+ * @flags:	behavior modifying flags
  *
  * Clock with a fixed multiplier and divider. The output frequency is the
  * parent clock rate divided by div and multiplied by mult.
- * Implements .recalc_rate, .set_rate and .round_rate
+ * Implements .recalc_rate, .set_rate, .round_rate and .recalc_accuracy
+ *
+ * Flags:
+ * * CLK_FIXED_FACTOR_FIXED_ACCURACY - Use the value in @acc instead of the
+ *                                     parent clk accuracy.
  */
 
 struct clk_fixed_factor {
 	struct clk_hw	hw;
 	unsigned int	mult;
 	unsigned int	div;
+	unsigned long	acc;
+	unsigned int	flags;
 };
 
+#define CLK_FIXED_FACTOR_FIXED_ACCURACY	BIT(0)
+
 #define to_clk_fixed_factor(_hw) container_of(_hw, struct clk_fixed_factor, hw)
 
 extern const struct clk_ops clk_fixed_factor_ops;

-- 
2.43.0

[PATCH v3 02/17] clk: fixed-factor: add fwname-based constructor functions

[Théo Lebrun]

Add four functions to register clk_hw based on the fw_name field in
clk_parent_data, ie the value in the DT property `clock-names`.

There are variants for devm or not and passing an accuracy or not
passing one:

 - clk_hw_register_fixed_factor_fwname
 - clk_hw_register_fixed_factor_with_accuracy_fwname
 - devm_clk_hw_register_fixed_factor_fwname
 - devm_clk_hw_register_fixed_factor_with_accuracy_fwname

The `struct clk_parent_data` init is extracted from
__clk_hw_register_fixed_factor to each calling function. It is required
to allow each function to pass whatever field they want, not only index.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/clk/clk-fixed-factor.c | 85 +++++++++++++++++++++++++++++++++++-------
 include/linux/clk-provider.h   | 14 +++++++
 2 files changed, 85 insertions(+), 14 deletions(-)

diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
index bc2644a9bd7d..fe0500a1af3e 100644
--- a/drivers/clk/clk-fixed-factor.c
+++ b/drivers/clk/clk-fixed-factor.c
@@ -91,13 +91,12 @@ static void devm_clk_hw_register_fixed_factor_release(struct device *dev, void *
 static struct clk_hw *
 __clk_hw_register_fixed_factor(struct device *dev, struct device_node *np,
 		const char *name, const char *parent_name,
-		const struct clk_hw *parent_hw, int index,
+		const struct clk_hw *parent_hw, const struct clk_parent_data *pdata,
 		unsigned long flags, unsigned int mult, unsigned int div,
 		unsigned long acc, unsigned int fixflags, bool devm)
 {
 	struct clk_fixed_factor *fix;
 	struct clk_init_data init = { };
-	struct clk_parent_data pdata = { .index = index };
 	struct clk_hw *hw;
 	int ret;
 
@@ -128,7 +127,7 @@ __clk_hw_register_fixed_factor(struct device *dev, struct device_node *np,
 	else if (parent_hw)
 		init.parent_hws = &parent_hw;
 	else
-		init.parent_data = &pdata;
+		init.parent_data = pdata;
 	init.num_parents = 1;
 
 	hw = &fix->hw;
@@ -165,7 +164,9 @@ struct clk_hw *devm_clk_hw_register_fixed_factor_index(struct device *dev,
 		const char *name, unsigned int index, unsigned long flags,
 		unsigned int mult, unsigned int div)
 {
-	return __clk_hw_register_fixed_factor(dev, NULL, name, NULL, NULL, index,
+	const struct clk_parent_data pdata = { .index = index };
+
+	return __clk_hw_register_fixed_factor(dev, NULL, name, NULL, NULL, &pdata,
 					      flags, mult, div, 0, 0, true);
 }
 EXPORT_SYMBOL_GPL(devm_clk_hw_register_fixed_factor_index);
@@ -187,8 +188,10 @@ struct clk_hw *devm_clk_hw_register_fixed_factor_parent_hw(struct device *dev,
 		const char *name, const struct clk_hw *parent_hw,
 		unsigned long flags, unsigned int mult, unsigned int div)
 {
+	const struct clk_parent_data pdata = { .index = -1 };
+
 	return __clk_hw_register_fixed_factor(dev, NULL, name, NULL, parent_hw,
-					      -1, flags, mult, div, 0, 0, true);
+					      &pdata, flags, mult, div, 0, 0, true);
 }
 EXPORT_SYMBOL_GPL(devm_clk_hw_register_fixed_factor_parent_hw);
 
@@ -196,9 +199,10 @@ struct clk_hw *clk_hw_register_fixed_factor_parent_hw(struct device *dev,
 		const char *name, const struct clk_hw *parent_hw,
 		unsigned long flags, unsigned int mult, unsigned int div)
 {
-	return __clk_hw_register_fixed_factor(dev, NULL, name, NULL,
-					      parent_hw, -1, flags, mult, div,
-					      0, 0, false);
+	const struct clk_parent_data pdata = { .index = -1 };
+
+	return __clk_hw_register_fixed_factor(dev, NULL, name, NULL, parent_hw,
+					      &pdata, flags, mult, div, 0, 0, false);
 }
 EXPORT_SYMBOL_GPL(clk_hw_register_fixed_factor_parent_hw);
 
@@ -206,11 +210,37 @@ struct clk_hw *clk_hw_register_fixed_factor(struct device *dev,
 		const char *name, const char *parent_name, unsigned long flags,
 		unsigned int mult, unsigned int div)
 {
-	return __clk_hw_register_fixed_factor(dev, NULL, name, parent_name, NULL, -1,
-					      flags, mult, div, 0, 0, false);
+	const struct clk_parent_data pdata = { .index = -1 };
+
+	return __clk_hw_register_fixed_factor(dev, NULL, name, parent_name, NULL,
+					      &pdata, flags, mult, div, 0, 0, false);
 }
 EXPORT_SYMBOL_GPL(clk_hw_register_fixed_factor);
 
+struct clk_hw *clk_hw_register_fixed_factor_fwname(struct device *dev,
+		struct device_node *np, const char *name, const char *fw_name,
+		unsigned long flags, unsigned int mult, unsigned int div)
+{
+	const struct clk_parent_data pdata = { .index = -1, .fw_name = fw_name };
+
+	return __clk_hw_register_fixed_factor(dev, np, name, NULL, NULL,
+			&pdata, flags, mult, div, 0, 0, false);
+}
+EXPORT_SYMBOL_GPL(clk_hw_register_fixed_factor_fwname);
+
+struct clk_hw *clk_hw_register_fixed_factor_with_accuracy_fwname(struct device *dev,
+		struct device_node *np, const char *name, const char *fw_name,
+		unsigned long flags, unsigned int mult, unsigned int div,
+		unsigned long acc)
+{
+	const struct clk_parent_data pdata = { .index = -1, .fw_name = fw_name };
+
+	return __clk_hw_register_fixed_factor(dev, np, name, NULL, NULL,
+			&pdata, flags, mult, div, acc,
+			CLK_FIXED_FACTOR_FIXED_ACCURACY, false);
+}
+EXPORT_SYMBOL_GPL(clk_hw_register_fixed_factor_with_accuracy_fwname);
+
 struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
 		const char *parent_name, unsigned long flags,
 		unsigned int mult, unsigned int div)
@@ -253,16 +283,43 @@ struct clk_hw *devm_clk_hw_register_fixed_factor(struct device *dev,
 		const char *name, const char *parent_name, unsigned long flags,
 		unsigned int mult, unsigned int div)
 {
-	return __clk_hw_register_fixed_factor(dev, NULL, name, parent_name, NULL, -1,
-			flags, mult, div, 0, 0, true);
+	const struct clk_parent_data pdata = { .index = -1 };
+
+	return __clk_hw_register_fixed_factor(dev, NULL, name, parent_name, NULL,
+			&pdata, flags, mult, div, 0, 0, true);
 }
 EXPORT_SYMBOL_GPL(devm_clk_hw_register_fixed_factor);
 
+struct clk_hw *devm_clk_hw_register_fixed_factor_fwname(struct device *dev,
+		struct device_node *np, const char *name, const char *fw_name,
+		unsigned long flags, unsigned int mult, unsigned int div)
+{
+	const struct clk_parent_data pdata = { .index = -1, .fw_name = fw_name };
+
+	return __clk_hw_register_fixed_factor(dev, np, name, NULL, NULL,
+			&pdata, flags, mult, div, 0, 0, true);
+}
+EXPORT_SYMBOL_GPL(devm_clk_hw_register_fixed_factor_fwname);
+
+struct clk_hw *devm_clk_hw_register_fixed_factor_with_accuracy_fwname(struct device *dev,
+		struct device_node *np, const char *name, const char *fw_name,
+		unsigned long flags, unsigned int mult, unsigned int div,
+		unsigned long acc)
+{
+	const struct clk_parent_data pdata = { .index = -1, .fw_name = fw_name };
+
+	return __clk_hw_register_fixed_factor(dev, np, name, NULL, NULL,
+			&pdata, flags, mult, div, acc,
+			CLK_FIXED_FACTOR_FIXED_ACCURACY, true);
+}
+EXPORT_SYMBOL_GPL(devm_clk_hw_register_fixed_factor_with_accuracy_fwname);
+
 #ifdef CONFIG_OF
 static struct clk_hw *_of_fixed_factor_clk_setup(struct device_node *node)
 {
 	struct clk_hw *hw;
 	const char *clk_name = node->name;
+	const struct clk_parent_data pdata = { .index = 0 };
 	u32 div, mult;
 	int ret;
 
@@ -280,8 +337,8 @@ static struct clk_hw *_of_fixed_factor_clk_setup(struct device_node *node)
 
 	of_property_read_string(node, "clock-output-names", &clk_name);
 
-	hw = __clk_hw_register_fixed_factor(NULL, node, clk_name, NULL, NULL, 0,
-					    0, mult, div, 0, 0, false);
+	hw = __clk_hw_register_fixed_factor(NULL, node, clk_name, NULL, NULL,
+					    &pdata, 0, mult, div, 0, 0, false);
 	if (IS_ERR(hw)) {
 		/*
 		 * Clear OF_POPULATED flag so that clock registration can be
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 7ddc952c8c67..4a537260f655 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -1116,10 +1116,24 @@ void clk_unregister_fixed_factor(struct clk *clk);
 struct clk_hw *clk_hw_register_fixed_factor(struct device *dev,
 		const char *name, const char *parent_name, unsigned long flags,
 		unsigned int mult, unsigned int div);
+struct clk_hw *clk_hw_register_fixed_factor_fwname(struct device *dev,
+		struct device_node *np, const char *name, const char *fw_name,
+		unsigned long flags, unsigned int mult, unsigned int div);
+struct clk_hw *clk_hw_register_fixed_factor_with_accuracy_fwname(struct device *dev,
+		struct device_node *np, const char *name, const char *fw_name,
+		unsigned long flags, unsigned int mult, unsigned int div,
+		unsigned long acc);
 void clk_hw_unregister_fixed_factor(struct clk_hw *hw);
 struct clk_hw *devm_clk_hw_register_fixed_factor(struct device *dev,
 		const char *name, const char *parent_name, unsigned long flags,
 		unsigned int mult, unsigned int div);
+struct clk_hw *devm_clk_hw_register_fixed_factor_fwname(struct device *dev,
+		struct device_node *np, const char *name, const char *fw_name,
+		unsigned long flags, unsigned int mult, unsigned int div);
+struct clk_hw *devm_clk_hw_register_fixed_factor_with_accuracy_fwname(struct device *dev,
+		struct device_node *np, const char *name, const char *fw_name,
+		unsigned long flags, unsigned int mult, unsigned int div,
+		unsigned long acc);
 struct clk_hw *devm_clk_hw_register_fixed_factor_index(struct device *dev,
 		const char *name, unsigned int index, unsigned long flags,
 		unsigned int mult, unsigned int div);

-- 
2.43.0

[PATCH v3 03/17] dt-bindings: pinctrl: allow pin controller device without unit address

[Théo Lebrun]

Allow a pin controller device to have no address, therefore no unit
address.

The previous $nodename was enforcing a unit address, but
scripts/dtc/checks.c enforced that names with unit addresses have reg
or ranges:

   Warning (unit_address_vs_reg): .../pinctrl@0: node has a unit
   name, but no reg or ranges property

Fix pinctrl.yaml to adopt a (pinctrl|pinmux)(-[a-z]+)? node name when
neither reg nor ranges are required. Use [a-z]+ to avoid conflicts with
pinctrl-consumer.yaml.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 Documentation/devicetree/bindings/pinctrl/pinctrl.yaml | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml
index d471563119a9..1f1239c9b94d 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml
@@ -26,9 +26,6 @@ description: |
   controller device.
 
 properties:
-  $nodename:
-    pattern: "^(pinctrl|pinmux)(@[0-9a-f]+)?$"
-
   "#pinctrl-cells":
     description: >
       Number of pin control cells in addition to the index within the pin
@@ -42,4 +39,19 @@ properties:
       This property can be set either globally for the pin controller or in
       child nodes for individual pin group control.
 
+if:
+  anyOf:
+    - required:
+       - reg
+    - required:
+       - ranges
+then:
+  properties:
+    $nodename:
+      pattern: "^(pinctrl|pinmux)(@[0-9a-f]+)?$"
+else:
+  properties:
+    $nodename:
+      pattern: "^(pinctrl|pinmux)(-[a-z]+)?$"
+
 additionalProperties: true

-- 
2.43.0

[PATCH v3 04/17] dt-bindings: soc: mobileye: add EyeQ5 OLB system controller

[Théo Lebrun]

Add documentation to describe the "Other Logic Block" syscon.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 .../bindings/soc/mobileye/mobileye,eyeq5-olb.yaml  | 77 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 78 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml
new file mode 100644
index 000000000000..031ef6a532c1
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml
@@ -0,0 +1,77 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/mobileye/mobileye,eyeq5-olb.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Mobileye EyeQ5 SoC system controller
+
+maintainers:
+  - Grégory Clement <gregory.clement@bootlin.com>
+  - Théo Lebrun <theo.lebrun@bootlin.com>
+  - Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>
+
+description:
+  OLB ("Other Logic Block") is a hardware block grouping smaller blocks. Clocks,
+  resets, pinctrl are being handled from here.
+
+properties:
+  compatible:
+    items:
+      - const: mobileye,eyeq5-olb
+      - const: syscon
+      - const: simple-mfd
+
+  reg:
+    maxItems: 1
+
+  clock-controller:
+    $ref: /schemas/clock/mobileye,eyeq5-clk.yaml#
+    type: object
+
+  reset-controller:
+    $ref: /schemas/reset/mobileye,eyeq5-reset.yaml#
+    type: object
+
+  pinctrl-a:
+    $ref: /schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml#
+    type: object
+
+  pinctrl-b:
+    $ref: /schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml#
+    type: object
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    system-controller@e00000 {
+      compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd";
+      reg = <0xe00000 0x400>;
+
+      clock-controller {
+        compatible = "mobileye,eyeq5-clk";
+        #clock-cells = <1>;
+        clocks = <&xtal>;
+        clock-names = "ref";
+      };
+
+      reset-controller {
+        compatible = "mobileye,eyeq5-reset";
+        #reset-cells = <2>;
+      };
+
+      pinctrl-a {
+        compatible = "mobileye,eyeq5-a-pinctrl";
+        #pinctrl-cells = <1>;
+      };
+
+      pinctrl-b {
+        compatible = "mobileye,eyeq5-b-pinctrl";
+        #pinctrl-cells = <1>;
+      };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index cb431c79c7b8..fe1270e8c983 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14786,6 +14786,7 @@ M:	Théo Lebrun <theo.lebrun@bootlin.com>
 L:	linux-mips@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/mips/mobileye.yaml
+F:	Documentation/devicetree/bindings/soc/mobileye/
 F:	arch/mips/boot/dts/mobileye/
 F:	arch/mips/configs/eyeq5_defconfig
 F:	arch/mips/mobileye/board-epm5.its.S

-- 
2.43.0

[PATCH v3 05/17] dt-bindings: clock: mobileye,eyeq5-clk: add bindings

[Théo Lebrun]

Add DT schema bindings for the EyeQ5 clock controller driver.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 .../bindings/clock/mobileye,eyeq5-clk.yaml         | 41 ++++++++++++++++++++++
 MAINTAINERS                                        |  2 ++
 include/dt-bindings/clock/mobileye,eyeq5-clk.h     | 22 ++++++++++++
 3 files changed, 65 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/mobileye,eyeq5-clk.yaml b/Documentation/devicetree/bindings/clock/mobileye,eyeq5-clk.yaml
new file mode 100644
index 000000000000..e1813c010122
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/mobileye,eyeq5-clk.yaml
@@ -0,0 +1,41 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/mobileye,eyeq5-clk.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Mobileye EyeQ5 clock controller
+
+description:
+  The EyeQ5 clock controller handles 10 read-only PLLs derived from the main
+  crystal clock. It also exposes one divider clock, a child of one of the PLLs.
+  Its registers live in a shared region called OLB.
+
+maintainers:
+  - Grégory Clement <gregory.clement@bootlin.com>
+  - Théo Lebrun <theo.lebrun@bootlin.com>
+  - Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>
+
+properties:
+  compatible:
+    const: mobileye,eyeq5-clk
+
+  "#clock-cells":
+    const: 1
+
+  clocks:
+    maxItems: 1
+    description:
+      Input parent clock to all PLLs. Expected to be the main crystal.
+
+  clock-names:
+    items:
+      - const: ref
+
+required:
+  - compatible
+  - "#clock-cells"
+  - clocks
+  - clock-names
+
+additionalProperties: false
diff --git a/MAINTAINERS b/MAINTAINERS
index fe1270e8c983..45ab636b6605 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14785,11 +14785,13 @@ M:	Gregory CLEMENT <gregory.clement@bootlin.com>
 M:	Théo Lebrun <theo.lebrun@bootlin.com>
 L:	linux-mips@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/clock/mobileye,eyeq5-clk.yaml
 F:	Documentation/devicetree/bindings/mips/mobileye.yaml
 F:	Documentation/devicetree/bindings/soc/mobileye/
 F:	arch/mips/boot/dts/mobileye/
 F:	arch/mips/configs/eyeq5_defconfig
 F:	arch/mips/mobileye/board-epm5.its.S
+F:	include/dt-bindings/clock/mobileye,eyeq5-clk.h
 F:	include/dt-bindings/soc/mobileye,eyeq5.h
 
 MODULE SUPPORT
diff --git a/include/dt-bindings/clock/mobileye,eyeq5-clk.h b/include/dt-bindings/clock/mobileye,eyeq5-clk.h
new file mode 100644
index 000000000000..26d8930335e4
--- /dev/null
+++ b/include/dt-bindings/clock/mobileye,eyeq5-clk.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright (C) 2024 Mobileye Vision Technologies Ltd.
+ */
+
+#ifndef _DT_BINDINGS_CLOCK_MOBILEYE_EYEQ5_CLK_H
+#define _DT_BINDINGS_CLOCK_MOBILEYE_EYEQ5_CLK_H
+
+#define EQ5C_PLL_CPU	0
+#define EQ5C_PLL_VMP	1
+#define EQ5C_PLL_PMA	2
+#define EQ5C_PLL_VDI	3
+#define EQ5C_PLL_DDR0	4
+#define EQ5C_PLL_PCI	5
+#define EQ5C_PLL_PER	6
+#define EQ5C_PLL_PMAC	7
+#define EQ5C_PLL_MPC	8
+#define EQ5C_PLL_DDR1	9
+
+#define EQ5C_DIV_OSPI	10
+
+#endif

-- 
2.43.0

[PATCH v3 06/17] dt-bindings: reset: mobileye,eyeq5-reset: add bindings

[Théo Lebrun]

Add DT-Schema bindings for the EyeQ5 reset controller.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 .../bindings/reset/mobileye,eyeq5-reset.yaml       | 32 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 33 insertions(+)

diff --git a/Documentation/devicetree/bindings/reset/mobileye,eyeq5-reset.yaml b/Documentation/devicetree/bindings/reset/mobileye,eyeq5-reset.yaml
new file mode 100644
index 000000000000..481b61f278fa
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/mobileye,eyeq5-reset.yaml
@@ -0,0 +1,32 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reset/mobileye,eyeq5-reset.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Mobileye EyeQ5 reset controller
+
+description:
+  The EyeQ5 reset driver handles three reset domains. Its registers live in a
+  shared region called OLB.
+
+maintainers:
+  - Grégory Clement <gregory.clement@bootlin.com>
+  - Théo Lebrun <theo.lebrun@bootlin.com>
+  - Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>
+
+properties:
+  compatible:
+    const: mobileye,eyeq5-reset
+
+  "#reset-cells":
+    const: 2
+    description:
+      The first cell is the domain (0 to 2 inclusive) and the second one is the
+      reset index inside that domain.
+
+required:
+  - compatible
+  - "#reset-cells"
+
+additionalProperties: false
diff --git a/MAINTAINERS b/MAINTAINERS
index 45ab636b6605..6e0eae9d6243 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14787,6 +14787,7 @@ L:	linux-mips@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/clock/mobileye,eyeq5-clk.yaml
 F:	Documentation/devicetree/bindings/mips/mobileye.yaml
+F:	Documentation/devicetree/bindings/reset/mobileye,eyeq5-reset.yaml
 F:	Documentation/devicetree/bindings/soc/mobileye/
 F:	arch/mips/boot/dts/mobileye/
 F:	arch/mips/configs/eyeq5_defconfig

-- 
2.43.0

[PATCH v3 07/17] dt-bindings: pinctrl: mobileye,eyeq5-pinctrl: add bindings

[Théo Lebrun]

Add dt-schema type bindings for the Mobileye EyeQ5 pin controller.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 .../bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml   | 77 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 78 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml
new file mode 100644
index 000000000000..db62919053b4
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml
@@ -0,0 +1,77 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Mobileye EyeQ5 pin controller
+
+description:
+  The EyeQ5 pin controller handles a pin bank. It is custom to this platform,
+  its registers live in a shared region called OLB.
+  There are two pin banks on the platform, each having a specific compatible.
+  Pins and groups are bijective.
+
+maintainers:
+  - Grégory Clement <gregory.clement@bootlin.com>
+  - Théo Lebrun <theo.lebrun@bootlin.com>
+  - Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>
+
+properties:
+  compatible:
+    enum:
+      - mobileye,eyeq5-a-pinctrl
+      - mobileye,eyeq5-b-pinctrl
+
+  "#pinctrl-cells":
+    const: 1
+
+required:
+  - compatible
+  - "#pinctrl-cells"
+
+patternProperties:
+  "-pins?$":
+    type: object
+    description: Pin muxing configuration.
+    $ref: pinmux-node.yaml#
+    additionalProperties: false
+    properties:
+      pins: true
+      function: true
+      bias-disable: true
+      bias-pull-down: true
+      bias-pull-up: true
+      drive-strength: true
+    required:
+      - pins
+      - function
+
+allOf:
+  - $ref: pinctrl.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: mobileye,eyeq5-a-pinctrl
+    then:
+      patternProperties:
+        "-pins?$":
+          properties:
+            function:
+              enum: [gpio, timer0, timer1, timer2, timer5, uart0, uart1, can0,
+                     can1, spi0, spi1, refclk0]
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: mobileye,eyeq5-b-pinctrl
+    then:
+      patternProperties:
+        "-pins?$":
+          properties:
+            function:
+              enum: [gpio, timer3, timer4, timer6, uart2, can2, spi2, spi3,
+                     mclk0]
+
+additionalProperties: false
diff --git a/MAINTAINERS b/MAINTAINERS
index 6e0eae9d6243..6dc4251e1ac2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14787,6 +14787,7 @@ L:	linux-mips@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/clock/mobileye,eyeq5-clk.yaml
 F:	Documentation/devicetree/bindings/mips/mobileye.yaml
+F:	Documentation/devicetree/bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml
 F:	Documentation/devicetree/bindings/reset/mobileye,eyeq5-reset.yaml
 F:	Documentation/devicetree/bindings/soc/mobileye/
 F:	arch/mips/boot/dts/mobileye/

-- 
2.43.0

[PATCH v3 08/17] clk: eyeq5: add platform driver

[Théo Lebrun]

Add the Mobileye EyeQ5 clock controller driver. It might grow to add
support for other platforms from Mobileye.

It handles 10 read-only PLLs derived from the main crystal on board. It
exposes a table-based divider clock used for OSPI. Other platform
clocks are not configurable and therefore kept as fixed-factor
devicetree nodes.

Two PLLs are required early on and are therefore registered at
of_clk_init(). Those are pll-cpu for the GIC timer and pll-per for the
UARTs.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 MAINTAINERS             |   1 +
 drivers/clk/Kconfig     |  11 ++
 drivers/clk/Makefile    |   1 +
 drivers/clk/clk-eyeq5.c | 414 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 427 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6dc4251e1ac2..3ea96ab7d2b8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14793,6 +14793,7 @@ F:	Documentation/devicetree/bindings/soc/mobileye/
 F:	arch/mips/boot/dts/mobileye/
 F:	arch/mips/configs/eyeq5_defconfig
 F:	arch/mips/mobileye/board-epm5.its.S
+F:	drivers/clk/clk-eyeq5.c
 F:	include/dt-bindings/clock/mobileye,eyeq5-clk.h
 F:	include/dt-bindings/soc/mobileye,eyeq5.h
 
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 50af5fc7f570..d5043ce2a75c 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -218,6 +218,17 @@ config COMMON_CLK_EN7523
 	  This driver provides the fixed clocks and gates present on Airoha
 	  ARM silicon.
 
+config COMMON_CLK_EYEQ5
+	bool "Clock driver for the Mobileye EyeQ5 platform"
+	depends on OF
+	depends on MACH_EYEQ5 || COMPILE_TEST
+	default MACH_EYEQ5
+	help
+		This driver provides the clocks found on the Mobileye EyeQ5 SoC. Its
+		registers live in a shared register region called OLB. It provides 10
+		read-only PLLs derived from the main crystal clock which must be constant
+		and one divider clock based on one PLL.
+
 config COMMON_CLK_FSL_FLEXSPI
 	tristate "Clock driver for FlexSPI on Layerscape SoCs"
 	depends on ARCH_LAYERSCAPE || COMPILE_TEST
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 14fa8d4ecc1f..81c4d11ca437 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_ARCH_CLPS711X)		+= clk-clps711x.o
 obj-$(CONFIG_COMMON_CLK_CS2000_CP)	+= clk-cs2000-cp.o
 obj-$(CONFIG_ARCH_SPARX5)		+= clk-sparx5.o
 obj-$(CONFIG_COMMON_CLK_EN7523)		+= clk-en7523.o
+obj-$(CONFIG_COMMON_CLK_EYEQ5)		+= clk-eyeq5.o
 obj-$(CONFIG_COMMON_CLK_FIXED_MMIO)	+= clk-fixed-mmio.o
 obj-$(CONFIG_COMMON_CLK_FSL_FLEXSPI)	+= clk-fsl-flexspi.o
 obj-$(CONFIG_COMMON_CLK_FSL_SAI)	+= clk-fsl-sai.o
diff --git a/drivers/clk/clk-eyeq5.c b/drivers/clk/clk-eyeq5.c
new file mode 100644
index 000000000000..475dfcdc8af1
--- /dev/null
+++ b/drivers/clk/clk-eyeq5.c
@@ -0,0 +1,414 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * PLL clock driver for the Mobileye EyeQ5 platform.
+ *
+ * This controller handles 10 read-only PLLs, all derived from the same main
+ * crystal clock. It also exposes one divider clock, a child of one of the
+ * PLLs. The parent clock is expected to be constant. This driver's registers
+ * live in a shared region called OLB. Two PLLs must be initialized by
+ * of_clk_init().
+ *
+ * We use eq5c_ as prefix, as-in "EyeQ5 Clock", but way shorter.
+ *
+ * Copyright (C) 2024 Mobileye Vision Technologies Ltd.
+ */
+
+#define pr_fmt(fmt) "clk-eyeq5: " fmt
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/clk-provider.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/clock/mobileye,eyeq5-clk.h>
+
+/*
+ * PLL control & status registers, n=0..1
+ * 0x02c..0x078
+ */
+#define OLB_PCSR_CPU(n)				(0x02C + (n) * 4) /* CPU */
+#define OLB_PCSR_VMP(n)				(0x034 + (n) * 4) /* VMP */
+#define OLB_PCSR_PMA(n)				(0x03C + (n) * 4) /* PMA */
+#define OLB_PCSR_VDI(n)				(0x044 + (n) * 4) /* VDI */
+#define OLB_PCSR_DDR0(n)			(0x04C + (n) * 4) /* DDR0 */
+#define OLB_PCSR_PCI(n)				(0x054 + (n) * 4) /* PCI */
+#define OLB_PCSR_PER(n)				(0x05C + (n) * 4) /* PER */
+#define OLB_PCSR_PMAC(n)			(0x064 + (n) * 4) /* PMAC */
+#define OLB_PCSR_MPC(n)				(0x06c + (n) * 4) /* MPC */
+#define OLB_PCSR_DDR1(n)			(0x074 + (n) * 4) /* DDR1 */
+
+/* In frac mode, it enables fractional noise canceling DAC. Else, no function. */
+#define OLB_PCSR0_DAC_EN			BIT(0)
+/* Fractional or integer mode */
+#define OLB_PCSR0_DSM_EN			BIT(1)
+#define OLB_PCSR0_PLL_EN			BIT(2)
+/* All clocks output held at 0 */
+#define OLB_PCSR0_FOUTPOSTDIV_EN		BIT(3)
+#define OLB_PCSR0_POST_DIV1			GENMASK(6, 4)
+#define OLB_PCSR0_POST_DIV2			GENMASK(9, 7)
+#define OLB_PCSR0_REF_DIV			GENMASK(15, 10)
+#define OLB_PCSR0_INTIN				GENMASK(27, 16)
+#define OLB_PCSR0_BYPASS			BIT(28)
+/* Bits 30..29 are reserved */
+#define OLB_PCSR0_PLL_LOCKED			BIT(31)
+
+#define OLB_PCSR1_RESET				BIT(0)
+#define OLB_PCSR1_SSGC_DIV			GENMASK(4, 1)
+/* Spread amplitude (% = 0.1 * SPREAD[4:0]) */
+#define OLB_PCSR1_SPREAD			GENMASK(9, 5)
+#define OLB_PCSR1_DIS_SSCG			BIT(10)
+/* Down-spread or center-spread */
+#define OLB_PCSR1_DOWN_SPREAD			BIT(11)
+#define OLB_PCSR1_FRAC_IN			GENMASK(31, 12)
+
+static struct clk_hw_onecell_data *eq5c_clk_data;
+static struct regmap *eq5c_olb;
+
+struct eq5c_pll {
+	int		index;
+	const char	*name;
+	u32		reg;
+};
+
+/* Required early for the GIC timer (pll-cpu) and UARTs (pll-per). */
+static const struct eq5c_pll eq5c_early_plls[] = {
+	{ .index = EQ5C_PLL_CPU, .name = "pll-cpu",  .reg = OLB_PCSR_CPU(0), },
+	{ .index = EQ5C_PLL_PER, .name = "pll-per",  .reg = OLB_PCSR_PER(0), },
+};
+
+static const struct eq5c_pll eq5c_plls[] = {
+	{ .index = EQ5C_PLL_VMP,  .name = "pll-vmp",  .reg = OLB_PCSR_VMP(0),  },
+	{ .index = EQ5C_PLL_PMA,  .name = "pll-pma",  .reg = OLB_PCSR_PMA(0),  },
+	{ .index = EQ5C_PLL_VDI,  .name = "pll-vdi",  .reg = OLB_PCSR_VDI(0),  },
+	{ .index = EQ5C_PLL_DDR0, .name = "pll-ddr0", .reg = OLB_PCSR_DDR0(0), },
+	{ .index = EQ5C_PLL_PCI,  .name = "pll-pci",  .reg = OLB_PCSR_PCI(0),  },
+	{ .index = EQ5C_PLL_PMAC, .name = "pll-pmac", .reg = OLB_PCSR_PMAC(0), },
+	{ .index = EQ5C_PLL_MPC,  .name = "pll-mpc",  .reg = OLB_PCSR_MPC(0),  },
+	{ .index = EQ5C_PLL_DDR1, .name = "pll-ddr1", .reg = OLB_PCSR_DDR1(0), },
+};
+
+#define EQ5C_OSPI_DIV_CLK_NAME	"div-ospi"
+
+#define EQ5C_NB_CLKS	(ARRAY_SIZE(eq5c_early_plls) + ARRAY_SIZE(eq5c_plls) + 1)
+
+static int eq5c_pll_parse_registers(u32 r0, u32 r1, unsigned long *mult,
+				    unsigned long *div, unsigned long *acc)
+{
+	if (r0 & OLB_PCSR0_BYPASS) {
+		*mult = 1;
+		*div = 1;
+		*acc = 0;
+		return 0;
+	}
+
+	if (!(r0 & OLB_PCSR0_PLL_LOCKED))
+		return -EINVAL;
+
+	*mult = FIELD_GET(OLB_PCSR0_INTIN, r0);
+	*div = FIELD_GET(OLB_PCSR0_REF_DIV, r0);
+	if (r0 & OLB_PCSR0_FOUTPOSTDIV_EN)
+		*div *= FIELD_GET(OLB_PCSR0_POST_DIV1, r0) *
+			FIELD_GET(OLB_PCSR0_POST_DIV2, r0);
+
+	/* Fractional mode, in 2^20 (0x100000) parts. */
+	if (r0 & OLB_PCSR0_DSM_EN) {
+		*div *= 0x100000;
+		*mult = *mult * 0x100000 + FIELD_GET(OLB_PCSR1_FRAC_IN, r1);
+	}
+
+	if (!*mult || !*div)
+		return -EINVAL;
+
+	/* Spread spectrum. */
+	if (!(r1 & (OLB_PCSR1_RESET | OLB_PCSR1_DIS_SSCG))) {
+		/*
+		 * Spread is 1/1000 parts of frequency, accuracy is half of
+		 * that. To get accuracy, convert to ppb (parts per billion).
+		 */
+		u32 spread = FIELD_GET(OLB_PCSR1_SPREAD, r1);
+		*acc = spread * 500000;
+		if (r1 & OLB_PCSR1_DOWN_SPREAD) {
+			/*
+			 * Downspreading: the central frequency is half a
+			 * spread lower.
+			 */
+			*mult *= 2000 - spread;
+			*div *= 2000;
+		}
+	} else {
+		*acc = 0;
+	}
+
+	return 0;
+}
+
+#define OLB_OSPI_REG		0x11C
+#define OLB_OSPI_DIV_MASK	GENMASK(3, 0)
+#define OLB_OSPI_DIV_MASK_WIDTH	4
+
+static const struct clk_div_table eq5c_ospi_div_table[] = {
+	{ .val = 0, .div = 2 },
+	{ .val = 1, .div = 4 },
+	{ .val = 2, .div = 6 },
+	{ .val = 3, .div = 8 },
+	{ .val = 4, .div = 10 },
+	{ .val = 5, .div = 12 },
+	{ .val = 6, .div = 14 },
+	{ .val = 7, .div = 16 },
+	{} /* sentinel */
+};
+
+struct eq5c_ospi_div {
+	struct clk_hw	hw;
+	struct device	*dev;
+	struct regmap	*olb;
+};
+
+static struct eq5c_ospi_div *clk_to_priv(struct clk_hw *hw)
+{
+	return container_of(hw, struct eq5c_ospi_div, hw);
+}
+
+static unsigned long eq5c_ospi_div_recalc_rate(struct clk_hw *hw,
+					       unsigned long parent_rate)
+{
+	struct eq5c_ospi_div *div = clk_to_priv(hw);
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(div->olb, OLB_OSPI_REG, &val);
+
+	if (ret) {
+		dev_err(div->dev, "regmap_read failed: %d\n", ret);
+		return 0;
+	}
+
+	val = FIELD_GET(OLB_OSPI_DIV_MASK, val);
+
+	return divider_recalc_rate(hw, parent_rate, val,
+				   eq5c_ospi_div_table, 0,
+				   OLB_OSPI_DIV_MASK_WIDTH);
+}
+
+static long eq5c_ospi_div_round_rate(struct clk_hw *hw,
+				     unsigned long rate, unsigned long *prate)
+{
+	return divider_round_rate(hw, rate, prate, eq5c_ospi_div_table,
+				  OLB_OSPI_DIV_MASK_WIDTH, 0);
+}
+
+static int eq5c_ospi_div_determine_rate(struct clk_hw *hw,
+					struct clk_rate_request *req)
+{
+	return divider_determine_rate(hw, req, eq5c_ospi_div_table,
+				      OLB_OSPI_DIV_MASK_WIDTH, 0);
+}
+
+static int eq5c_ospi_div_set_rate(struct clk_hw *hw,
+				  unsigned long rate, unsigned long parent_rate)
+{
+	struct eq5c_ospi_div *div = clk_to_priv(hw);
+	unsigned int val;
+	int value, ret;
+
+	value = divider_get_val(rate, parent_rate, eq5c_ospi_div_table,
+				OLB_OSPI_DIV_MASK_WIDTH, 0);
+	if (value < 0)
+		return value;
+
+	ret = regmap_read(div->olb, OLB_OSPI_REG, &val);
+	if (ret) {
+		dev_err(div->dev, "regmap_read failed: %d\n", ret);
+		return ret;
+	}
+
+	val &= ~OLB_OSPI_DIV_MASK;
+	val |= FIELD_PREP(OLB_OSPI_DIV_MASK, value);
+
+	ret = regmap_write(div->olb, OLB_OSPI_REG, val);
+	if (ret) {
+		dev_err(div->dev, "regmap_write failed: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct clk_ops eq5c_ospi_div_ops = {
+	.recalc_rate = eq5c_ospi_div_recalc_rate,
+	.round_rate = eq5c_ospi_div_round_rate,
+	.determine_rate = eq5c_ospi_div_determine_rate,
+	.set_rate = eq5c_ospi_div_set_rate,
+};
+
+static struct clk_hw *eq5c_init_ospi_div(struct device *dev, const char *name,
+					 struct regmap *olb,
+					 const struct clk_hw *parent)
+{
+	struct eq5c_ospi_div *div;
+	int ret;
+
+	div = kzalloc(sizeof(*div), GFP_KERNEL);
+	if (!div)
+		return ERR_PTR(-ENOENT);
+
+	div->dev = dev;
+	div->olb = olb;
+	div->hw.init = CLK_HW_INIT_HW(name, parent, &eq5c_ospi_div_ops, 0);
+
+	ret = clk_hw_register(dev, &div->hw);
+	if (ret) {
+		dev_err(dev, "failed registering %s: %d\n", name, ret);
+		kfree(div);
+		return ERR_PTR(ret);
+	}
+
+	return &div->hw;
+}
+
+static int eq5c_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct clk_hw *hw;
+	int i;
+
+	if (IS_ERR(eq5c_clk_data))
+		return PTR_ERR(eq5c_clk_data);
+	else if (!eq5c_clk_data)
+		return -EINVAL;
+
+	for (i = 0; i < ARRAY_SIZE(eq5c_plls); i++) {
+		const struct eq5c_pll *pll = &eq5c_plls[i];
+		unsigned long mult, div, acc;
+		u32 r0, r1;
+		int ret;
+
+		regmap_read(eq5c_olb, pll->reg, &r0);
+		regmap_read(eq5c_olb, pll->reg + sizeof(r0), &r1);
+
+		ret = eq5c_pll_parse_registers(r0, r1, &mult, &div, &acc);
+		if (ret) {
+			dev_warn(dev, "failed parsing state of %s\n", pll->name);
+			continue;
+		}
+
+		hw = clk_hw_register_fixed_factor_with_accuracy_fwname(dev, np,
+				pll->name, "ref", 0, mult, div, acc);
+		eq5c_clk_data->hws[pll->index] = hw;
+		if (IS_ERR(hw)) {
+			dev_err(dev, "failed registering %s: %ld\n",
+				pll->name, PTR_ERR(hw));
+		}
+	}
+
+	/*
+	 * Register the OSPI table-based divider clock manually. This is
+	 * equivalent to drivers/clk/clk-divider.c, but using regmap to access
+	 * register.
+	 */
+	hw = eq5c_init_ospi_div(dev, EQ5C_OSPI_DIV_CLK_NAME, eq5c_olb,
+				eq5c_clk_data->hws[EQ5C_PLL_PER]);
+	eq5c_clk_data->hws[EQ5C_DIV_OSPI] = hw;
+
+	return 0;
+}
+
+static const struct of_device_id eq5c_match_table[] = {
+	{ .compatible = "mobileye,eyeq5-clk" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, eq5c_match_table);
+
+static struct platform_driver eq5c_driver = {
+	.probe = eq5c_probe,
+	.driver = {
+		.name = "clk-eyeq5",
+		.of_match_table = eq5c_match_table,
+	},
+};
+
+builtin_platform_driver(eq5c_driver);
+
+static void __init eq5c_init(struct device_node *np)
+{
+	struct device_node *parent_np = of_get_parent(np);
+	int i, ret;
+
+	eq5c_clk_data = kzalloc(struct_size(eq5c_clk_data, hws, EQ5C_NB_CLKS),
+				GFP_KERNEL);
+	if (!eq5c_clk_data) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	eq5c_clk_data->num = EQ5C_NB_CLKS;
+
+	/*
+	 * Mark all clocks as deferred. We register some now and others at
+	 * platform device probe.
+	 */
+	for (i = 0; i < EQ5C_NB_CLKS; i++)
+		eq5c_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
+
+	/*
+	 * Currently, if OLB is not available, we log an error, fail init then
+	 * fail probe. We might want to change this behavior and assume all
+	 * clocks are in bypass mode; this is what is being done in the vendor
+	 * driver.
+	 *
+	 * It is unclear if there are valid situations where the OLB region
+	 * would be inaccessible.
+	 */
+	eq5c_olb = ERR_PTR(-ENODEV);
+	if (parent_np)
+		eq5c_olb = syscon_node_to_regmap(parent_np);
+	if (IS_ERR(eq5c_olb)) {
+		pr_err("failed getting regmap: %ld\n", PTR_ERR(eq5c_olb));
+		ret = PTR_ERR(eq5c_olb);
+		goto err;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(eq5c_early_plls); i++) {
+		const struct eq5c_pll *pll = &eq5c_early_plls[i];
+		unsigned long mult, div, acc;
+		struct clk_hw *hw;
+		u32 r0, r1;
+
+		regmap_read(eq5c_olb, pll->reg, &r0);
+		regmap_read(eq5c_olb, pll->reg + sizeof(r0), &r1);
+
+		ret = eq5c_pll_parse_registers(r0, r1, &mult, &div, &acc);
+		if (ret) {
+			pr_warn("failed parsing state of %s\n", pll->name);
+			continue;
+		}
+
+		hw = clk_hw_register_fixed_factor_with_accuracy_fwname(NULL,
+				np, pll->name, "ref", 0, mult, div, acc);
+		eq5c_clk_data->hws[pll->index] = hw;
+		if (IS_ERR(hw)) {
+			pr_err("failed registering %s: %ld\n",
+			       pll->name, PTR_ERR(hw));
+		}
+	}
+
+	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, eq5c_clk_data);
+	if (ret) {
+		pr_err("failed registering clk provider: %d\n", ret);
+		goto err;
+	}
+
+	return;
+
+err:
+	kfree(eq5c_clk_data);
+	/* Signal to platform driver probe that we failed init. */
+	eq5c_clk_data = ERR_PTR(ret);
+	eq5c_olb = NULL;
+}
+
+CLK_OF_DECLARE_DRIVER(eq5c, "mobileye,eyeq5-clk", eq5c_init);

-- 
2.43.0

[PATCH v3 09/17] reset: eyeq5: add platform driver

[Théo Lebrun]

Add the Mobileye EyeQ5 reset controller driver. It belongs to a syscon
region called OLB. It might grow to add later support of other
platforms from Mobileye.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 MAINTAINERS                 |   1 +
 drivers/reset/Kconfig       |  12 ++
 drivers/reset/Makefile      |   1 +
 drivers/reset/reset-eyeq5.c | 383 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 397 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3ea96ab7d2b8..dd3b5834386f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14794,6 +14794,7 @@ F:	arch/mips/boot/dts/mobileye/
 F:	arch/mips/configs/eyeq5_defconfig
 F:	arch/mips/mobileye/board-epm5.its.S
 F:	drivers/clk/clk-eyeq5.c
+F:	drivers/reset/reset-eyeq5.c
 F:	include/dt-bindings/clock/mobileye,eyeq5-clk.h
 F:	include/dt-bindings/soc/mobileye,eyeq5.h
 
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index ccd59ddd7610..80bfde54c076 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -66,6 +66,18 @@ config RESET_BRCMSTB_RESCAL
 	  This enables the RESCAL reset controller for SATA, PCIe0, or PCIe1 on
 	  BCM7216.
 
+config RESET_EYEQ5
+	bool "Mobileye EyeQ5 reset controller"
+	depends on MFD_SYSCON
+	depends on MACH_EYEQ5 || COMPILE_TEST
+	default MACH_EYEQ5
+	help
+	  This enables the Mobileye EyeQ5 reset controller.
+
+	  It has three domains, with a varying number of resets in each of them.
+	  Registers are located in a shared register region called OLB accessed
+	  through a syscon & regmap.
+
 config RESET_HSDK
 	bool "Synopsys HSDK Reset Driver"
 	depends on HAS_IOMEM
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 8270da8a4baa..4fabe0070390 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_RESET_BCM6345) += reset-bcm6345.o
 obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
 obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o
 obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
+obj-$(CONFIG_RESET_EYEQ5) += reset-eyeq5.o
 obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
 obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
 obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
diff --git a/drivers/reset/reset-eyeq5.c b/drivers/reset/reset-eyeq5.c
new file mode 100644
index 000000000000..2217e42e140b
--- /dev/null
+++ b/drivers/reset/reset-eyeq5.c
@@ -0,0 +1,383 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Reset driver for the Mobileye EyeQ5 platform.
+ *
+ * The registers are located in a syscon region called OLB. We handle three
+ * reset domains. Domains 0 and 2 look similar in that they both use one bit
+ * per reset line. Domain 1 has a register per reset.
+ *
+ * We busy-wait after updating a reset in domains 0 or 1. The reason is hardware
+ * logic built-in self-test (LBIST) that might be enabled.
+ *
+ * We use eq5r_ as prefix, as-in "EyeQ5 Reset", but way shorter.
+ *
+ * Known resets in domain 0:
+ *  3. CAN0
+ *  4. CAN1
+ *  5. CAN2
+ *  6. SPI0
+ *  7. SPI1
+ *  8. SPI2
+ *  9. SPI3
+ * 10. UART0
+ * 11. UART1
+ * 12. UART2
+ * 13. I2C0
+ * 14. I2C1
+ * 15. I2C2
+ * 16. I2C3
+ * 17. I2C4
+ * 18. TIMER0
+ * 19. TIMER1
+ * 20. TIMER2
+ * 21. TIMER3
+ * 22. TIMER4
+ * 23. WD0
+ * 24. EXT0
+ * 25. EXT1
+ * 26. GPIO
+ * 27. WD1
+ *
+ * Known resets in domain 1:
+ * 0. VMP0	(Vector Microcode Processors)
+ * 1. VMP1
+ * 2. VMP2
+ * 3. VMP3
+ * 4. PMA0	(Programmable Macro Array)
+ * 5. PMA1
+ * 6. PMAC0
+ * 7. PMAC1
+ * 8. MPC0	(Multi-threaded Processing Clusters)
+ * 9. MPC1
+ *
+ * Known resets in domain 2:
+ *  0. PCIE0_CORE
+ *  1. PCIE0_APB
+ *  2. PCIE0_LINK_AXI
+ *  3. PCIE0_LINK_MGMT
+ *  4. PCIE0_LINK_HOT
+ *  5. PCIE0_LINK_PIPE
+ *  6. PCIE1_CORE
+ *  7. PCIE1_APB
+ *  8. PCIE1_LINK_AXI
+ *  9. PCIE1_LINK_MGMT
+ * 10. PCIE1_LINK_HOT
+ * 11. PCIE1_LINK_PIPE
+ * 12. MULTIPHY
+ * 13. MULTIPHY_APB
+ * 15. PCIE0_LINK_MGMT
+ * 16. PCIE1_LINK_MGMT
+ * 17. PCIE0_LINK_PM
+ * 18. PCIE1_LINK_PM
+ *
+ * Copyright (C) 2024 Mobileye Vision Technologies Ltd.
+ */
+
+#include <linux/mfd/syscon.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset-controller.h>
+
+/* Offsets into the OLB region as well as masks for domain 1 registers. */
+#define EQ5R_OLB_SARCR0			(0x004)
+#define EQ5R_OLB_SARCR1			(0x008)
+#define EQ5R_OLB_PCIE_GP		(0x120)
+#define EQ5R_OLB_ACRP_REG(n)		(0x200 + 4 * (n)) // n=0..12
+#define EQ5R_OLB_ACRP_PD_REQ		BIT(0)
+#define EQ5R_OLB_ACRP_ST_POWER_DOWN	BIT(27)
+#define EQ5R_OLB_ACRP_ST_ACTIVE		BIT(29)
+
+/* Vendor-provided values. D1 has a long timeout because of LBIST. */
+#define D0_TIMEOUT_POLL			10
+#define D1_TIMEOUT_POLL			40000
+
+/*
+ * Masks for valid reset lines in each domain. This array is also used to get
+ * the domain and reset counts.
+ */
+static const u32 eq5r_valid_masks[] = { 0x0FFFFFF8, 0x00001FFF, 0x0007BFFF };
+
+#define EQ5R_DOMAIN_COUNT ARRAY_SIZE(eq5r_valid_masks)
+
+struct eq5r_private {
+	struct mutex mutexes[EQ5R_DOMAIN_COUNT]; /* We serialize all reset operations. */
+	struct regmap *olb;			 /* Writes go to a syscon regmap. */
+	struct reset_controller_dev rcdev;
+};
+
+static int _eq5r_busy_wait(struct eq5r_private *priv, struct device *dev,
+			   u32 domain, u32 offset, bool assert)
+{
+	unsigned int val, mask;
+	int i;
+
+	lockdep_assert_held(&priv->mutexes[domain]);
+
+	switch (domain) {
+	case 0:
+		for (i = 0; i < D0_TIMEOUT_POLL; i++) {
+			regmap_read(priv->olb, EQ5R_OLB_SARCR1, &val);
+			val = !(val & BIT(offset));
+			if (val == assert)
+				return 0;
+			__udelay(1);
+		}
+		break;
+	case 1:
+		mask = assert ? EQ5R_OLB_ACRP_ST_POWER_DOWN : EQ5R_OLB_ACRP_ST_ACTIVE;
+		for (i = 0; i < D1_TIMEOUT_POLL; i++) {
+			regmap_read(priv->olb, EQ5R_OLB_ACRP_REG(offset), &val);
+			if (val & mask)
+				return 0;
+			__udelay(1);
+		}
+		break;
+	case 2:
+		return 0; /* No busy waiting for domain 2. */
+	default:
+		WARN_ON(1);
+		return -EINVAL;
+	}
+
+	dev_dbg(dev, "%u-%u: timeout\n", domain, offset);
+	return -ETIMEDOUT;
+}
+
+static void _eq5r_assert(struct eq5r_private *priv, u32 domain, u32 offset)
+{
+	lockdep_assert_held(&priv->mutexes[domain]);
+
+	switch (domain) {
+	case 0:
+		regmap_clear_bits(priv->olb, EQ5R_OLB_SARCR0, BIT(offset));
+		break;
+	case 1:
+		regmap_set_bits(priv->olb, EQ5R_OLB_ACRP_REG(offset),
+				EQ5R_OLB_ACRP_PD_REQ);
+		break;
+	case 2:
+		regmap_clear_bits(priv->olb, EQ5R_OLB_PCIE_GP, BIT(offset));
+		break;
+	default:
+		WARN_ON(1);
+	}
+}
+
+static int eq5r_assert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct eq5r_private *priv = dev_get_drvdata(rcdev->dev);
+	u32 offset = id & GENMASK(7, 0);
+	u32 domain = id >> 8;
+	int ret;
+
+	if (WARN_ON(domain >= EQ5R_DOMAIN_COUNT))
+		return -EINVAL;
+
+	dev_dbg(rcdev->dev, "%u-%u: assert request\n", domain, offset);
+
+	mutex_lock(&priv->mutexes[domain]);
+	_eq5r_assert(priv, domain, offset);
+	ret = _eq5r_busy_wait(priv, rcdev->dev, domain, offset, true);
+	mutex_unlock(&priv->mutexes[domain]);
+
+	return ret;
+}
+
+static void _eq5r_deassert(struct eq5r_private *priv, u32 domain, u32 offset)
+{
+	lockdep_assert_held(&priv->mutexes[domain]);
+
+	switch (domain) {
+	case 0:
+		regmap_set_bits(priv->olb, EQ5R_OLB_SARCR0, BIT(offset));
+		break;
+	case 1:
+		regmap_clear_bits(priv->olb, EQ5R_OLB_ACRP_REG(offset),
+				  EQ5R_OLB_ACRP_PD_REQ);
+		break;
+	case 2:
+		regmap_set_bits(priv->olb, EQ5R_OLB_PCIE_GP, BIT(offset));
+		break;
+	default:
+		WARN_ON(1);
+	}
+}
+
+static int eq5r_deassert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct eq5r_private *priv = dev_get_drvdata(rcdev->dev);
+	u32 offset = id & GENMASK(7, 0);
+	u32 domain = id >> 8;
+	int ret;
+
+	if (WARN_ON(domain >= EQ5R_DOMAIN_COUNT))
+		return -EINVAL;
+
+	dev_dbg(rcdev->dev, "%u-%u: deassert request\n", domain, offset);
+
+	mutex_lock(&priv->mutexes[domain]);
+	_eq5r_deassert(priv, domain, offset);
+	ret = _eq5r_busy_wait(priv, rcdev->dev, domain, offset, false);
+	mutex_unlock(&priv->mutexes[domain]);
+
+	return ret;
+}
+
+static int eq5r_reset(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct device *dev = rcdev->dev;
+	struct eq5r_private *priv = dev_get_drvdata(dev);
+	u32 offset = id & GENMASK(7, 0);
+	u32 domain = id >> 8;
+	int ret;
+
+	if (WARN_ON(domain >= EQ5R_DOMAIN_COUNT))
+		return -EINVAL;
+
+	dev_dbg(dev, "%u-%u: reset request\n", domain, offset);
+
+	mutex_lock(&priv->mutexes[domain]);
+
+	_eq5r_assert(priv, domain, offset);
+	ret = _eq5r_busy_wait(priv, dev, domain, offset, true);
+	if (ret) /* don't let an error disappear silently */
+		dev_warn(dev, "%u-%u: reset assert failed: %d\n",
+			 domain, offset, ret);
+
+	_eq5r_deassert(priv, domain, offset);
+	ret = _eq5r_busy_wait(priv, dev, domain, offset, false);
+
+	mutex_unlock(&priv->mutexes[domain]);
+
+	return ret;
+}
+
+static int eq5r_status(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct eq5r_private *priv = dev_get_drvdata(rcdev->dev);
+	u32 offset = id & GENMASK(7, 0);
+	u32 domain = id >> 8;
+	unsigned int val;
+	int ret;
+
+	if (WARN_ON(domain >= EQ5R_DOMAIN_COUNT))
+		return -EINVAL;
+
+	dev_dbg(rcdev->dev, "%u-%u: status request\n", domain, offset);
+
+	mutex_lock(&priv->mutexes[domain]);
+
+	switch (domain) {
+	case 0:
+		regmap_read(priv->olb, EQ5R_OLB_SARCR1, &val);
+		ret = !(val & BIT(offset));
+		break;
+	case 1:
+		regmap_read(priv->olb, EQ5R_OLB_ACRP_REG(offset), &val);
+		ret = !(val & EQ5R_OLB_ACRP_ST_ACTIVE);
+		break;
+	case 2:
+		regmap_read(priv->olb, EQ5R_OLB_PCIE_GP, &val);
+		ret = !(val & BIT(offset));
+		break;
+	}
+
+	mutex_unlock(&priv->mutexes[domain]);
+
+	return ret;
+}
+
+static const struct reset_control_ops eq5r_ops = {
+	.reset	  = eq5r_reset,
+	.assert	  = eq5r_assert,
+	.deassert = eq5r_deassert,
+	.status	  = eq5r_status,
+};
+
+static int eq5r_of_xlate(struct reset_controller_dev *rcdev,
+			 const struct of_phandle_args *reset_spec)
+{
+	u32 domain, offset;
+
+	if (WARN_ON(reset_spec->args_count != 2))
+		return -EINVAL;
+
+	domain = reset_spec->args[0];
+	offset = reset_spec->args[1];
+
+	if (domain >= EQ5R_DOMAIN_COUNT || offset > 31 ||
+	    !(eq5r_valid_masks[domain] & BIT(offset))) {
+		dev_err(rcdev->dev, "%u-%u: invalid reset\n", domain, offset);
+		return -EINVAL;
+	}
+
+	return (domain << 8) | offset;
+}
+
+static int eq5r_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct device_node *parent_np = of_get_parent(np);
+	struct eq5r_private *priv;
+	int ret, i;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, priv);
+
+	priv->olb = ERR_PTR(-ENODEV);
+	if (parent_np) {
+		priv->olb = syscon_node_to_regmap(parent_np);
+		of_node_put(parent_np);
+	}
+	if (IS_ERR(priv->olb))
+		return PTR_ERR(priv->olb);
+
+	for (i = 0; i < EQ5R_DOMAIN_COUNT; i++)
+		mutex_init(&priv->mutexes[i]);
+
+	priv->rcdev.ops = &eq5r_ops;
+	priv->rcdev.owner = THIS_MODULE;
+	priv->rcdev.dev = dev;
+	priv->rcdev.of_node = np;
+	priv->rcdev.of_reset_n_cells = 2;
+	priv->rcdev.of_xlate = eq5r_of_xlate;
+
+	priv->rcdev.nr_resets = 0;
+	for (i = 0; i < EQ5R_DOMAIN_COUNT; i++)
+		priv->rcdev.nr_resets += __builtin_popcount(eq5r_valid_masks[i]);
+
+	ret = reset_controller_register(&priv->rcdev);
+	if (ret) {
+		dev_err(dev, "Failed registering reset controller: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id eq5r_match_table[] = {
+	{ .compatible = "mobileye,eyeq5-reset" },
+	{}
+};
+
+static struct platform_driver eq5r_driver = {
+	.probe = eq5r_probe,
+	.driver = {
+		.name = "eyeq5-reset",
+		.of_match_table = eq5r_match_table,
+	},
+};
+
+static int __init eq5r_init(void)
+{
+	return platform_driver_register(&eq5r_driver);
+}
+
+arch_initcall(eq5r_init);

-- 
2.43.0

[PATCH v3 10/17] pinctrl: eyeq5: add platform driver

[Théo Lebrun]

Add the Mobileye EyeQ5 pin controller driver. It might grow to add later
support of other platforms from Mobileye. It belongs to a syscon region
called OLB.

Existing pins and their function live statically in the driver code
rather than in the devicetree, see compatible match data.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 MAINTAINERS                     |   1 +
 drivers/pinctrl/Kconfig         |  15 +
 drivers/pinctrl/Makefile        |   1 +
 drivers/pinctrl/pinctrl-eyeq5.c | 595 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 612 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index dd3b5834386f..9c423a4feb86 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14794,6 +14794,7 @@ F:	arch/mips/boot/dts/mobileye/
 F:	arch/mips/configs/eyeq5_defconfig
 F:	arch/mips/mobileye/board-epm5.its.S
 F:	drivers/clk/clk-eyeq5.c
+F:	drivers/pinctrl/pinctrl-eyeq5.c
 F:	drivers/reset/reset-eyeq5.c
 F:	include/dt-bindings/clock/mobileye,eyeq5-clk.h
 F:	include/dt-bindings/soc/mobileye,eyeq5.h
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 8163a5983166..abe94de85b3d 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -195,6 +195,21 @@ config PINCTRL_EQUILIBRIUM
 	  desired pin functions, configure GPIO attributes for LGM SoC pins.
 	  Pin muxing and pin config settings are retrieved from device tree.
 
+config PINCTRL_EYEQ5
+	bool "Mobileye EyeQ5 pinctrl driver"
+	depends on OF
+	depends on MACH_EYEQ5 || COMPILE_TEST
+	select PINMUX
+	select GENERIC_PINCONF
+	select MFD_SYSCON
+	default MACH_EYEQ5
+	help
+	  Pin controller driver for the Mobileye EyeQ5 platform. It does both
+	  pin config & pin muxing. It does not handle GPIO.
+
+	  Pin muxing supports two functions for each pin: first is GPIO, second
+	  is pin-dependent. Pin config is about bias & drive strength.
+
 config PINCTRL_GEMINI
 	bool
 	depends on ARCH_GEMINI
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 1071f301cc70..0033940914d9 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_PINCTRL_DA850_PUPD) += pinctrl-da850-pupd.o
 obj-$(CONFIG_PINCTRL_DA9062)	+= pinctrl-da9062.o
 obj-$(CONFIG_PINCTRL_DIGICOLOR)	+= pinctrl-digicolor.o
 obj-$(CONFIG_PINCTRL_EQUILIBRIUM)   += pinctrl-equilibrium.o
+obj-$(CONFIG_PINCTRL_EYEQ5)	+= pinctrl-eyeq5.o
 obj-$(CONFIG_PINCTRL_GEMINI)	+= pinctrl-gemini.o
 obj-$(CONFIG_PINCTRL_INGENIC)	+= pinctrl-ingenic.o
 obj-$(CONFIG_PINCTRL_K210)	+= pinctrl-k210.o
diff --git a/drivers/pinctrl/pinctrl-eyeq5.c b/drivers/pinctrl/pinctrl-eyeq5.c
new file mode 100644
index 000000000000..2d8e5b2168bd
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-eyeq5.c
@@ -0,0 +1,595 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Pinctrl driver for the Mobileye EyeQ5 platform.
+ *
+ * The registers are located in a syscon region called OLB. There are two pin
+ * banks, each being controlled by 5 registers (see enum eq5p_regs) for
+ * pull-down, pull-up, drive strength and muxing.
+ *
+ * For each pin, muxing is between two functions: (0) GPIO or (1) another one
+ * that is pin-dependent. Functions are declared statically in this driver.
+ *
+ * We create pinctrl groups that are 1:1 equivalent to pins: each group has a
+ * single pin, and its index/selector is the pin number/offset.
+ *
+ * We use eq5p_ as prefix, as-in "EyeQ5 Pinctrl", but way shorter.
+ *
+ * Copyright (C) 2024 Mobileye Vision Technologies Ltd.
+ */
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/seq_file.h>
+
+#include "core.h"
+#include "pinctrl-utils.h"
+
+#define GPIO_FUNC_SELECTOR 0
+
+struct eq5p_pinctrl {
+	struct pinctrl_desc desc;
+
+	struct regmap *olb;
+	const unsigned int *regs; /* array of size EQ5P_REG_MAX */
+
+	const struct eq5p_function *funcs;
+	unsigned int nfuncs;
+};
+
+struct eq5p_function {
+	const char *name;
+	const char * const *groups;
+	unsigned int ngroups;
+};
+
+/* OLB registers; those are offsets in an array of address offsets. */
+enum eq5p_regs {
+	EQ5P_PD,
+	EQ5P_PU,
+	EQ5P_DS_LOW,
+	EQ5P_DS_HIGH,
+	EQ5P_IOCR,
+
+	EQ5P_REG_MAX
+};
+
+static int eq5p_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	return pctldev->desc->npins;
+}
+
+static const char *eq5p_pinctrl_get_group_name(struct pinctrl_dev *pctldev,
+					       unsigned int selector)
+{
+	return pctldev->desc->pins[selector].name;
+}
+
+static int eq5p_pinctrl_get_group_pins(struct pinctrl_dev *pctldev,
+				       unsigned int selector,
+				       const unsigned int **pins,
+				       unsigned int *num_pins)
+{
+	*pins = &pctldev->desc->pins[selector].number;
+	*num_pins = 1;
+	return 0;
+}
+
+static int eq5p_pinconf_get(struct pinctrl_dev *pctldev, unsigned int offset,
+			    unsigned long *config);
+
+static void eq5p_pinctrl_pin_dbg_show(struct pinctrl_dev *pctldev,
+				      struct seq_file *s,
+				      unsigned int offset)
+{
+	struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	const char *pin_name = pctrl->desc.pins[offset].name;
+	const char *func_name, *bias;
+	unsigned int val_pd, val_pu, val_iocr;
+	unsigned long ds_config;
+	u32 drive_strength;
+	bool pd, pu;
+	int i, j;
+
+	/* First, let's get the function name. */
+	regmap_read(pctrl->olb, pctrl->regs[EQ5P_IOCR], &val_iocr);
+	if ((val_iocr & BIT(offset)) == 0) {
+		func_name = pctrl->funcs[GPIO_FUNC_SELECTOR].name;
+	} else {
+		/* All pins have only two functions: GPIO and something else. We
+		 * look for this something else.
+		 */
+		func_name = NULL;
+		for (i = 0; i < pctrl->nfuncs; i++) {
+			if (i == GPIO_FUNC_SELECTOR)
+				continue;
+
+			for (j = 0; j < pctrl->funcs[i].ngroups; j++) {
+				/* Groups and pins are the same thing for us. */
+				const char *x = pctrl->funcs[i].groups[j];
+
+				if (strcmp(x, pin_name) == 0) {
+					func_name = pctrl->funcs[i].name;
+					break;
+				}
+			}
+
+			if (func_name)
+				break;
+		}
+
+		/* We have not found the function attached to this pin, this
+		 * should never occur as all pins have exactly two functions.
+		 */
+		if (!func_name)
+			func_name = "unknown";
+	}
+
+	/* Second, we retrieve the bias. */
+	regmap_read(pctrl->olb, pctrl->regs[EQ5P_PD], &val_pd);
+	pd = (val_pd & BIT(offset)) != 0;
+	regmap_read(pctrl->olb, pctrl->regs[EQ5P_PU], &val_pu);
+	pu = (val_pu & BIT(offset)) != 0;
+	if (pd && pu)
+		bias = "both";
+	else if (pd && !pu)
+		bias = "pulldown";
+	else if (!pd && pu)
+		bias = "pullup";
+	else
+		bias = "none";
+
+	/* Third, we get the drive strength. */
+	ds_config = pinconf_to_config_packed(PIN_CONFIG_DRIVE_STRENGTH, 0);
+	eq5p_pinconf_get(pctldev, offset, &ds_config);
+	drive_strength = pinconf_to_config_argument(ds_config);
+
+	seq_printf(s, "function=%s bias=%s drive_strength=%d",
+		   func_name, bias, drive_strength);
+}
+
+static const struct pinctrl_ops eq5p_pinctrl_ops = {
+	.get_groups_count = eq5p_pinctrl_get_groups_count,
+	.get_group_name = eq5p_pinctrl_get_group_name,
+	.get_group_pins = eq5p_pinctrl_get_group_pins,
+	.pin_dbg_show = eq5p_pinctrl_pin_dbg_show,
+	.dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
+	.dt_free_map = pinctrl_utils_free_map,
+};
+
+static int eq5p_pinmux_get_functions_count(struct pinctrl_dev *pctldev)
+{
+	struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	return pctrl->nfuncs;
+}
+
+static const char *eq5p_pinmux_get_function_name(struct pinctrl_dev *pctldev,
+						 unsigned int selector)
+{
+	struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	return pctrl->funcs[selector].name;
+}
+
+static int eq5p_pinmux_get_function_groups(struct pinctrl_dev *pctldev,
+					   unsigned int selector,
+					   const char * const **groups,
+					   unsigned int *num_groups)
+{
+	struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	*groups = pctrl->funcs[selector].groups;
+	*num_groups = pctrl->funcs[selector].ngroups;
+	return 0;
+}
+
+static int eq5p_pinmux_set_mux(struct pinctrl_dev *pctldev,
+			       unsigned int func_selector, unsigned int offset)
+{
+	const char *group_name = pctldev->desc->pins[offset].name;
+	struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	const char *func_name = pctrl->funcs[func_selector].name;
+	bool is_gpio = func_selector == GPIO_FUNC_SELECTOR;
+	unsigned int reg, mask, val;
+
+	dev_dbg(pctldev->dev, "%s: func=%s group=%s\n", __func__, func_name,
+		group_name);
+
+	reg = pctrl->regs[EQ5P_IOCR];
+	mask = BIT(offset);
+	val = is_gpio ? 0 : U32_MAX;
+
+	regmap_update_bits(pctrl->olb, reg, mask, val);
+
+	return 0;
+}
+
+static int eq5p_pinmux_gpio_request_enable(struct pinctrl_dev *pctldev,
+					   struct pinctrl_gpio_range *range,
+					   unsigned int offset)
+{
+	/* Pin offsets and group selectors are the same thing in our case. */
+	return eq5p_pinmux_set_mux(pctldev, GPIO_FUNC_SELECTOR, offset);
+}
+
+static const struct pinmux_ops eq5p_pinmux_ops = {
+	.get_functions_count = eq5p_pinmux_get_functions_count,
+	.get_function_name = eq5p_pinmux_get_function_name,
+	.get_function_groups = eq5p_pinmux_get_function_groups,
+	.set_mux = eq5p_pinmux_set_mux,
+	.gpio_request_enable = eq5p_pinmux_gpio_request_enable,
+	.strict = true,
+};
+
+static int eq5p_pinconf_get(struct pinctrl_dev *pctldev, unsigned int offset,
+			    unsigned long *config)
+{
+	enum pin_config_param param = pinconf_to_config_param(*config);
+	struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	unsigned int reg, val_pd, val_pu, val_ds;
+	bool pd, pu;
+	u32 arg = 0;
+
+	regmap_read(pctrl->olb, pctrl->regs[EQ5P_PD], &val_pd);
+	pd = (val_pd & BIT(offset)) != 0;
+	regmap_read(pctrl->olb, pctrl->regs[EQ5P_PU], &val_pu);
+	pu = (val_pu & BIT(offset)) != 0;
+
+	switch (param) {
+	case PIN_CONFIG_BIAS_DISABLE:
+		arg = !(pd || pu);
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		arg = pd;
+		break;
+	case PIN_CONFIG_BIAS_PULL_UP:
+		arg = pu;
+		break;
+	case PIN_CONFIG_DRIVE_STRENGTH:
+		offset *= 2; /* two bits per pin */
+		if (offset >= 32) {
+			reg = pctrl->regs[EQ5P_DS_HIGH];
+			offset -= 32;
+		} else {
+			reg = pctrl->regs[EQ5P_DS_LOW];
+		}
+		regmap_read(pctrl->olb, reg, &val_ds);
+		arg = (val_ds >> offset) & 0b11;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	*config = pinconf_to_config_packed(param, arg);
+	return 0;
+}
+
+static int eq5p_pinconf_set_drive_strength(struct pinctrl_dev *pctldev,
+					   unsigned int offset, u32 arg)
+{
+	struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	unsigned int reg, mask, val;
+
+	if (arg > 3) {
+		dev_err(pctldev->dev, "Unsupported drive strength: %u\n", arg);
+		return -EINVAL;
+	}
+
+	offset *= 2; /* two bits per pin */
+
+	if (offset >= 32) {
+		reg = pctrl->regs[EQ5P_DS_HIGH];
+		offset -= 32;
+	} else {
+		reg = pctrl->regs[EQ5P_DS_LOW];
+	}
+
+	mask = 0b11 << offset;
+	val = arg << offset;
+	regmap_update_bits(pctrl->olb, reg, mask, val);
+	return 0;
+}
+
+static int eq5p_pinconf_set(struct pinctrl_dev *pctldev, unsigned int offset,
+			    unsigned long *configs, unsigned int num_configs)
+{
+	struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	const char *pin_name = pctldev->desc->pins[offset].name;
+	struct device *dev = pctldev->dev;
+	unsigned int i, val, reg_pd, reg_pu;
+
+	val = BIT(offset);
+	reg_pd = pctrl->regs[EQ5P_PD];
+	reg_pu = pctrl->regs[EQ5P_PU];
+
+	for (i = 0; i < num_configs; i++) {
+		enum pin_config_param param = pinconf_to_config_param(configs[i]);
+		u32 arg = pinconf_to_config_argument(configs[i]);
+
+		switch (param) {
+		case PIN_CONFIG_BIAS_DISABLE:
+			dev_dbg(dev, "%s: pin=%s BIAS_DISABLE\n",
+				__func__, pin_name);
+			regmap_clear_bits(pctrl->olb, reg_pd, val);
+			regmap_clear_bits(pctrl->olb, reg_pu, val);
+			break;
+		case PIN_CONFIG_BIAS_PULL_DOWN:
+			dev_dbg(dev, "%s: pin=%s BIAS_PULL_DOWN arg=%u\n",
+				__func__, pin_name, arg);
+			if (arg == 0) /* cannot connect to GND */
+				return -EOPNOTSUPP;
+			regmap_set_bits(pctrl->olb, reg_pd, val);
+			regmap_clear_bits(pctrl->olb, reg_pu, val);
+			break;
+		case PIN_CONFIG_BIAS_PULL_UP:
+			dev_dbg(dev, "%s: pin=%s BIAS_PULL_UP arg=%u\n",
+				__func__, pin_name, arg);
+			if (arg == 0) /* cannot connect to VDD */
+				return -EOPNOTSUPP;
+			regmap_clear_bits(pctrl->olb, reg_pd, val);
+			regmap_set_bits(pctrl->olb, reg_pu, val);
+			break;
+		case PIN_CONFIG_DRIVE_STRENGTH:
+			dev_dbg(dev, "%s: pin=%s DRIVE_STRENGTH arg=%u\n",
+				__func__, pin_name, arg);
+			eq5p_pinconf_set_drive_strength(pctldev, offset, arg);
+			break;
+		default:
+			dev_err(dev, "Unsupported pinconf %u\n", param);
+			return -EOPNOTSUPP;
+		}
+	}
+
+	return 0;
+}
+
+static const struct pinconf_ops eq5p_pinconf_ops = {
+	.is_generic = true,
+	.pin_config_get = eq5p_pinconf_get,
+	.pin_config_set = eq5p_pinconf_set,
+	/* Pins and groups are equivalent in this driver. */
+	.pin_config_group_get = eq5p_pinconf_get,
+	.pin_config_group_set = eq5p_pinconf_set,
+};
+
+/*
+ * Comments to the right of each pin are the "signal name" in the datasheet.
+ */
+
+static const struct pinctrl_pin_desc eq5p_pins_a[] = { /* Bank A */
+	PINCTRL_PIN(0,  "PA0"),  /* A0_TIMER0_CK */
+	PINCTRL_PIN(1,  "PA1"),  /* A1_TIMER0_EOC */
+	PINCTRL_PIN(2,  "PA2"),  /* A2_TIMER1_CK */
+	PINCTRL_PIN(3,  "PA3"),  /* A3_TIMER1_EOC */
+	PINCTRL_PIN(4,  "PA4"),  /* A4_TIMER2_CK */
+	PINCTRL_PIN(5,  "PA5"),  /* A5_TIMER2_EOC */
+	PINCTRL_PIN(6,  "PA6"),  /* A6_TIMER5_EXT_INCAP1 */
+	PINCTRL_PIN(7,  "PA7"),  /* A7_TIMER5_EXT_INCAP2 */
+	PINCTRL_PIN(8,  "PA8"),  /* A8_TIMER5_EXT_OUTCMP1 */
+	PINCTRL_PIN(9,  "PA9"),  /* A9_TIMER5_EXT_OUTCMP2 */
+	PINCTRL_PIN(10, "PA10"), /* A10_UART_0_TX */
+	PINCTRL_PIN(11, "PA11"), /* A11_UART_0_RX */
+	PINCTRL_PIN(12, "PA12"), /* A12_UART_1_TX */
+	PINCTRL_PIN(13, "PA13"), /* A13_UART_1_RX */
+	PINCTRL_PIN(14, "PA14"), /* A14_CAN_0_TX */
+	PINCTRL_PIN(15, "PA15"), /* A15_CAN_0_RX */
+	PINCTRL_PIN(16, "PA16"), /* A16_CAN_1_TX */
+	PINCTRL_PIN(17, "PA17"), /* A17_CAN_1_RX */
+	PINCTRL_PIN(18, "PA18"), /* A18_SPI_0_DO */
+	PINCTRL_PIN(19, "PA19"), /* A19_SPI_0_DI */
+	PINCTRL_PIN(20, "PA20"), /* A20_SPI_0_CK */
+	PINCTRL_PIN(21, "PA21"), /* A21_SPI_0_CS0 */
+	PINCTRL_PIN(22, "PA22"), /* A22_SPI_0_CS1 */
+	PINCTRL_PIN(23, "PA23"), /* A23_SPI_1_DO */
+	PINCTRL_PIN(24, "PA24"), /* A24_SPI_1_DI */
+	PINCTRL_PIN(25, "PA25"), /* A25_SPI_1_CK */
+	PINCTRL_PIN(26, "PA26"), /* A26_SPI_1_CS0 */
+	PINCTRL_PIN(27, "PA27"), /* A27_SPI_1_CS1 */
+	PINCTRL_PIN(28, "PA28"), /* A28_REF_CLK0 */
+};
+
+static const struct pinctrl_pin_desc eq5p_pins_b[] = { /* Bank B */
+	PINCTRL_PIN(0,  "PB0"),  /* B0_TIMER3_CK */
+	PINCTRL_PIN(1,  "PB1"),  /* B1_TIMER3_EOC */
+	PINCTRL_PIN(2,  "PB2"),  /* B2_TIMER4_CK */
+	PINCTRL_PIN(3,  "PB3"),  /* B3_TIMER4_EOC */
+	PINCTRL_PIN(4,  "PB4"),  /* B4_TIMER6_EXT_INCAP1 */
+	PINCTRL_PIN(5,  "PB5"),  /* B5_TIMER6_EXT_INCAP2 */
+	PINCTRL_PIN(6,  "PB6"),  /* B6_TIMER6_EXT_OUTCMP1 */
+	PINCTRL_PIN(7,  "PB7"),  /* B7_TIMER6_EXT_OUTCMP2 */
+	PINCTRL_PIN(8,  "PB8"),  /* B8_UART_2_TX */
+	PINCTRL_PIN(9,  "PB9"),  /* B9_UART_2_RX */
+	PINCTRL_PIN(10, "PB10"), /* B10_CAN_2_TX */
+	PINCTRL_PIN(11, "PB11"), /* B11_CAN_2_RX */
+	PINCTRL_PIN(12, "PB12"), /* B12_SPI_2_DO */
+	PINCTRL_PIN(13, "PB13"), /* B13_SPI_2_DI */
+	PINCTRL_PIN(14, "PB14"), /* B14_SPI_2_CK */
+	PINCTRL_PIN(15, "PB15"), /* B15_SPI_2_CS0 */
+	PINCTRL_PIN(16, "PB16"), /* B16_SPI_2_CS1 */
+	PINCTRL_PIN(17, "PB17"), /* B17_SPI_3_DO */
+	PINCTRL_PIN(18, "PB18"), /* B18_SPI_3_DI */
+	PINCTRL_PIN(19, "PB19"), /* B19_SPI_3_CK */
+	PINCTRL_PIN(20, "PB20"), /* B20_SPI_3_CS0 */
+	PINCTRL_PIN(21, "PB21"), /* B21_SPI_3_CS1 */
+	PINCTRL_PIN(22, "PB22"), /* B22_MCLK0 */
+};
+
+/* Groups of functions on bank A */
+static const char * const gpioa_groups[] = {
+	"PA0", "PA1", "PA2", "PA3", "PA4", "PA5", "PA6", "PA7", "PA8", "PA9",
+	"PA10", "PA11", "PA12", "PA13", "PA14", "PA15", "PA16", "PA17", "PA18",
+	"PA19", "PA20", "PA21", "PA22", "PA23", "PA24", "PA25", "PA26", "PA27",
+	"PA28",
+};
+static const char * const timer0_groups[] = { "PA0", "PA1" };
+static const char * const timer1_groups[] = { "PA2", "PA3" };
+static const char * const timer2_groups[] = { "PA4", "PA5" };
+static const char * const timer5_groups[] = { "PA6", "PA7", "PA8", "PA9" };
+static const char * const uart0_groups[] = { "PA10", "PA11" };
+static const char * const uart1_groups[] = { "PA12", "PA13" };
+static const char * const can0_groups[] = { "PA14", "PA15" };
+static const char * const can1_groups[] = { "PA16", "PA17" };
+static const char * const spi0_groups[] = { "PA18", "PA19", "PA20", "PA21", "PA22" };
+static const char * const spi1_groups[] = { "PA23", "PA24", "PA25", "PA26", "PA27" };
+static const char * const refclk0_groups[] = { "PA28" };
+
+/* Groups of functions on bank B */
+static const char * const gpiob_groups[] = {
+	"PB0", "PB1", "PB2", "PB3", "PB4", "PB5", "PB6", "PB7", "PB8", "PB9",
+	"PB10", "PB11", "PB12", "PB13", "PB14", "PB15", "PB16", "PB17", "PB18",
+	"PB19", "PB20", "PB21", "PB22",
+};
+static const char * const timer3_groups[] = { "PB0", "PB1" };
+static const char * const timer4_groups[] = { "PB2", "PB3" };
+static const char * const timer6_groups[] = { "PB4", "PB5", "PB6", "PB7" };
+static const char * const uart2_groups[] = { "PB8", "PB9" };
+static const char * const can2_groups[] = { "PB10", "PB11" };
+static const char * const spi2_groups[] = { "PB12", "PB13", "PB14", "PB15", "PB16" };
+static const char * const spi3_groups[] = { "PB17", "PB18", "PB19", "PB20", "PB21" };
+static const char * const mclk0_groups[] = { "PB22" };
+
+#define FUNCTION(a, b) { .name = a, .groups = b, .ngroups = ARRAY_SIZE(b) }
+
+static const struct eq5p_function eq5p_functions_a[] = {
+	/* GPIO having a fixed index is depended upon, see GPIO_FUNC_SELECTOR. */
+	FUNCTION("gpio", gpioa_groups),
+
+	FUNCTION("timer0", timer0_groups),
+	FUNCTION("timer1", timer1_groups),
+	FUNCTION("timer2", timer2_groups),
+	FUNCTION("timer5", timer5_groups),
+	FUNCTION("uart0", uart0_groups),
+	FUNCTION("uart1", uart1_groups),
+	FUNCTION("can0", can0_groups),
+	FUNCTION("can1", can1_groups),
+	FUNCTION("spi0", spi0_groups),
+	FUNCTION("spi1", spi1_groups),
+	FUNCTION("refclk0", refclk0_groups),
+};
+
+static const struct eq5p_function eq5p_functions_b[] = {
+	/* GPIO having a fixed index is depended upon, see GPIO_FUNC_SELECTOR. */
+	FUNCTION("gpio", gpiob_groups),
+
+	FUNCTION("timer3", timer3_groups),
+	FUNCTION("timer4", timer4_groups),
+	FUNCTION("timer6", timer6_groups),
+	FUNCTION("uart2", uart2_groups),
+	FUNCTION("can2", can2_groups),
+	FUNCTION("spi2", spi2_groups),
+	FUNCTION("spi3", spi3_groups),
+	FUNCTION("mclk0", mclk0_groups),
+};
+
+struct eq5p_match {
+	unsigned int regs[EQ5P_REG_MAX];
+	const struct pinctrl_pin_desc *pins;
+	unsigned int npins;
+	const struct eq5p_function *funcs;
+	unsigned int nfuncs;
+};
+
+static int eq5p_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct device_node *parent_np = of_get_parent(np);
+	const struct eq5p_match *match = of_device_get_match_data(dev);
+	struct pinctrl_dev *pctldev;
+	struct eq5p_pinctrl *pctrl;
+	int ret;
+
+	pctrl = devm_kzalloc(dev, sizeof(*pctrl), GFP_KERNEL);
+	if (!pctrl)
+		return -ENOMEM;
+
+	pctrl->olb = ERR_PTR(-ENODEV);
+	if (parent_np)
+		pctrl->olb = syscon_node_to_regmap(parent_np);
+	if (IS_ERR(pctrl->olb))
+		pctrl->olb = syscon_regmap_lookup_by_phandle(np, "mobileye,olb");
+	if (IS_ERR(pctrl->olb))
+		return PTR_ERR(pctrl->olb);
+
+	pctrl->regs = match->regs;
+	pctrl->funcs = match->funcs;
+	pctrl->nfuncs = match->nfuncs;
+
+	pctrl->desc.name = dev_name(dev);
+	pctrl->desc.pins = match->pins;
+	pctrl->desc.npins = match->npins;
+	pctrl->desc.pctlops = &eq5p_pinctrl_ops;
+	pctrl->desc.pmxops = &eq5p_pinmux_ops;
+	pctrl->desc.confops = &eq5p_pinconf_ops;
+	pctrl->desc.owner = THIS_MODULE;
+
+	ret = devm_pinctrl_register_and_init(dev, &pctrl->desc, pctrl, &pctldev);
+	if (ret) {
+		dev_err(dev, "Failed registering pinctrl device: %d\n", ret);
+		return ret;
+	}
+
+	ret = pinctrl_enable(pctldev);
+	if (ret) {
+		dev_err(dev, "Failed enabling pinctrl device: %d\n", ret);
+		return ret;
+	}
+
+	dev_info(dev, "probed\n");
+
+	return 0;
+}
+
+static const struct eq5p_match eq5p_match_a = {
+	.regs = {
+		[EQ5P_PD] = 0x0C0,
+		[EQ5P_PU] = 0x0C4,
+		[EQ5P_DS_LOW] = 0x0D0,
+		[EQ5P_DS_HIGH] = 0x0D4,
+		[EQ5P_IOCR] = 0x0B0,
+	},
+	.pins = eq5p_pins_a,
+	.npins = ARRAY_SIZE(eq5p_pins_a),
+	.funcs = eq5p_functions_a,
+	.nfuncs = ARRAY_SIZE(eq5p_functions_a),
+};
+
+static const struct eq5p_match eq5p_match_b = {
+	.regs = {
+		[EQ5P_PD] = 0x0C8,
+		[EQ5P_PU] = 0x0CC,
+		[EQ5P_DS_LOW] = 0x0D8,
+		[EQ5P_DS_HIGH] = 0x0DC,
+		[EQ5P_IOCR] = 0x0B4,
+	},
+	.pins = eq5p_pins_b,
+	.npins = ARRAY_SIZE(eq5p_pins_b),
+	.funcs = eq5p_functions_b,
+	.nfuncs = ARRAY_SIZE(eq5p_functions_b),
+};
+
+static const struct of_device_id eq5p_match[] = {
+	{ .compatible = "mobileye,eyeq5-a-pinctrl", .data = &eq5p_match_a },
+	{ .compatible = "mobileye,eyeq5-b-pinctrl", .data = &eq5p_match_b },
+	{},
+};
+
+static struct platform_driver eq5p_driver = {
+	.driver = {
+		.name = "eyeq5-pinctrl",
+		.of_match_table = eq5p_match,
+	},
+	.probe = eq5p_probe,
+};
+
+static int __init eq5p_init(void)
+{
+	return platform_driver_register(&eq5p_driver);
+}
+core_initcall(eq5p_init);

-- 
2.43.0

[PATCH v3 11/17] MIPS: mobileye: eyeq5: rename olb@e00000 to system-controller@e00000

[Théo Lebrun]

Node names should be generic. OLB, meaning "Other Logic Block", is a
name specific to this platform. Change the node name to the generic and
often-used "system-controller".

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 arch/mips/boot/dts/mobileye/eyeq5.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/boot/dts/mobileye/eyeq5.dtsi b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
index fd34c49af510..81497febcdee 100644
--- a/arch/mips/boot/dts/mobileye/eyeq5.dtsi
+++ b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
@@ -100,7 +100,7 @@ uart2: serial@a00000 {
 			clock-names = "uartclk", "apb_pclk";
 		};
 
-		olb: olb@e00000 {
+		olb: system-controller@e00000 {
 			compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd";
 			reg = <0 0xe00000 0x0 0x400>;
 			reg-io-width = <4>;

-- 
2.43.0

[PATCH v3 12/17] MIPS: mobileye: eyeq5: remove reg-io-width property from OLB syscon

[Théo Lebrun]

Remove the `reg-io-width` property from the olb@e00000 syscon. The
default memory access width is what we desire: no need to make it
explicit.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 arch/mips/boot/dts/mobileye/eyeq5.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/mips/boot/dts/mobileye/eyeq5.dtsi b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
index 81497febcdee..03e7e942ee22 100644
--- a/arch/mips/boot/dts/mobileye/eyeq5.dtsi
+++ b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
@@ -103,7 +103,6 @@ uart2: serial@a00000 {
 		olb: system-controller@e00000 {
 			compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd";
 			reg = <0 0xe00000 0x0 0x400>;
-			reg-io-width = <4>;
 		};
 
 		gic: interrupt-controller@140000 {

-- 
2.43.0

[PATCH v3 13/17] MIPS: mobileye: eyeq5: use OLB clocks controller

[Théo Lebrun]

We add the clock controller inside the OLB syscon region and removed
previous fixed devicetree nodes representing PLLs exposed by the clock
controller.

Two quirks are present:

 - The OLB node is moved prior to UARTs, as platform device probe
   scheduling is dependent on devicetree ordering.

 - The core0-timer-clk fixed clock is created to feed the GIC timer. It
   requires a clock earlier than platform bus type initialisation (and
   therefore platform device init).

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 .../{eyeq5-fixed-clocks.dtsi => eyeq5-clocks.dtsi} | 54 +++++++---------------
 arch/mips/boot/dts/mobileye/eyeq5.dtsi             |  9 +++-
 2 files changed, 24 insertions(+), 39 deletions(-)

diff --git a/arch/mips/boot/dts/mobileye/eyeq5-fixed-clocks.dtsi b/arch/mips/boot/dts/mobileye/eyeq5-clocks.dtsi
similarity index 88%
rename from arch/mips/boot/dts/mobileye/eyeq5-fixed-clocks.dtsi
rename to arch/mips/boot/dts/mobileye/eyeq5-clocks.dtsi
index 78f5533a95c6..aa6db704a786 100644
--- a/arch/mips/boot/dts/mobileye/eyeq5-fixed-clocks.dtsi
+++ b/arch/mips/boot/dts/mobileye/eyeq5-clocks.dtsi
@@ -3,42 +3,20 @@
  * Copyright 2023 Mobileye Vision Technologies Ltd.
  */
 
+#include <dt-bindings/clock/mobileye,eyeq5-clk.h>
+
 / {
 	/* Fixed clock */
-	pll_cpu: pll-cpu {
-		compatible = "fixed-clock";
-		#clock-cells = <0>;
-		clock-frequency = <1500000000>;
-	};
-
-	pll_vdi: pll-vdi {
-		compatible = "fixed-clock";
-		#clock-cells = <0>;
-		clock-frequency = <1280000000>;
-	};
-
-	pll_per: pll-per {
-		compatible = "fixed-clock";
-		#clock-cells = <0>;
-		clock-frequency = <2000000000>;
-	};
-
-	pll_ddr0: pll-ddr0 {
-		compatible = "fixed-clock";
-		#clock-cells = <0>;
-		clock-frequency = <1857210000>;
-	};
-
-	pll_ddr1: pll-ddr1 {
+	xtal: xtal {
 		compatible = "fixed-clock";
 		#clock-cells = <0>;
-		clock-frequency = <1857210000>;
+		clock-frequency = <30000000>;
 	};
 
 /* PLL_CPU derivatives */
 	occ_cpu: occ-cpu {
 		compatible = "fixed-factor-clock";
-		clocks = <&pll_cpu>;
+		clocks = <&clocks EQ5C_PLL_CPU>;
 		#clock-cells = <0>;
 		clock-div = <1>;
 		clock-mult = <1>;
@@ -101,7 +79,7 @@ mem_clk: mem-clk {
 	};
 	occ_isram: occ-isram {
 		compatible = "fixed-factor-clock";
-		clocks = <&pll_cpu>;
+		clocks = <&clocks EQ5C_PLL_CPU>;
 		#clock-cells = <0>;
 		clock-div = <2>;
 		clock-mult = <1>;
@@ -115,7 +93,7 @@ isram_clk: isram-clk { /* gate ClkRstGen_isram */
 	};
 	occ_dbu: occ-dbu {
 		compatible = "fixed-factor-clock";
-		clocks = <&pll_cpu>;
+		clocks = <&clocks EQ5C_PLL_CPU>;
 		#clock-cells = <0>;
 		clock-div = <10>;
 		clock-mult = <1>;
@@ -130,7 +108,7 @@ si_dbu_tp_pclk: si-dbu-tp-pclk { /* gate ClkRstGen_dbu */
 /* PLL_VDI derivatives */
 	occ_vdi: occ-vdi {
 		compatible = "fixed-factor-clock";
-		clocks = <&pll_vdi>;
+		clocks = <&clocks EQ5C_PLL_VDI>;
 		#clock-cells = <0>;
 		clock-div = <2>;
 		clock-mult = <1>;
@@ -144,7 +122,7 @@ vdi_clk: vdi-clk { /* gate ClkRstGen_vdi */
 	};
 	occ_can_ser: occ-can-ser {
 		compatible = "fixed-factor-clock";
-		clocks = <&pll_vdi>;
+		clocks = <&clocks EQ5C_PLL_VDI>;
 		#clock-cells = <0>;
 		clock-div = <16>;
 		clock-mult = <1>;
@@ -158,7 +136,7 @@ can_ser_clk: can-ser-clk { /* gate ClkRstGen_can_ser */
 	};
 	i2c_ser_clk: i2c-ser-clk {
 		compatible = "fixed-factor-clock";
-		clocks = <&pll_vdi>;
+		clocks = <&clocks EQ5C_PLL_VDI>;
 		#clock-cells = <0>;
 		clock-div = <20>;
 		clock-mult = <1>;
@@ -166,7 +144,7 @@ i2c_ser_clk: i2c-ser-clk {
 /* PLL_PER derivatives */
 	occ_periph: occ-periph {
 		compatible = "fixed-factor-clock";
-		clocks = <&pll_per>;
+		clocks = <&clocks EQ5C_PLL_PER>;
 		#clock-cells = <0>;
 		clock-div = <16>;
 		clock-mult = <1>;
@@ -225,7 +203,7 @@ gpio_clk: gpio-clk {
 	};
 	emmc_sys_clk: emmc-sys-clk {
 		compatible = "fixed-factor-clock";
-		clocks = <&pll_per>;
+		clocks = <&clocks EQ5C_PLL_PER>;
 		#clock-cells = <0>;
 		clock-div = <10>;
 		clock-mult = <1>;
@@ -233,7 +211,7 @@ emmc_sys_clk: emmc-sys-clk {
 	};
 	ccf_ctrl_clk: ccf-ctrl-clk {
 		compatible = "fixed-factor-clock";
-		clocks = <&pll_per>;
+		clocks = <&clocks EQ5C_PLL_PER>;
 		#clock-cells = <0>;
 		clock-div = <4>;
 		clock-mult = <1>;
@@ -241,7 +219,7 @@ ccf_ctrl_clk: ccf-ctrl-clk {
 	};
 	occ_mjpeg_core: occ-mjpeg-core {
 		compatible = "fixed-factor-clock";
-		clocks = <&pll_per>;
+		clocks = <&clocks EQ5C_PLL_PER>;
 		#clock-cells = <0>;
 		clock-div = <2>;
 		clock-mult = <1>;
@@ -265,7 +243,7 @@ mjpeg_core_clk: mjpeg-core-clk { /* gate ClkRstGen_mjpeg_gen */
 	};
 	fcmu_a_clk: fcmu-a-clk {
 		compatible = "fixed-factor-clock";
-		clocks = <&pll_per>;
+		clocks = <&clocks EQ5C_PLL_PER>;
 		#clock-cells = <0>;
 		clock-div = <20>;
 		clock-mult = <1>;
@@ -273,7 +251,7 @@ fcmu_a_clk: fcmu-a-clk {
 	};
 	occ_pci_sys: occ-pci-sys {
 		compatible = "fixed-factor-clock";
-		clocks = <&pll_per>;
+		clocks = <&clocks EQ5C_PLL_PER>;
 		#clock-cells = <0>;
 		clock-div = <8>;
 		clock-mult = <1>;
diff --git a/arch/mips/boot/dts/mobileye/eyeq5.dtsi b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
index 03e7e942ee22..935c095d1423 100644
--- a/arch/mips/boot/dts/mobileye/eyeq5.dtsi
+++ b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
@@ -5,7 +5,7 @@
 
 #include <dt-bindings/interrupt-controller/mips-gic.h>
 
-#include "eyeq5-fixed-clocks.dtsi"
+#include "eyeq5-clocks.dtsi"
 
 / {
 	#address-cells = <2>;
@@ -103,6 +103,13 @@ uart2: serial@a00000 {
 		olb: system-controller@e00000 {
 			compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd";
 			reg = <0 0xe00000 0x0 0x400>;
+
+			clocks: clock-controller {
+				compatible = "mobileye,eyeq5-clk";
+				#clock-cells = <1>;
+				clocks = <&xtal>;
+				clock-names = "ref";
+			};
 		};
 
 		gic: interrupt-controller@140000 {

-- 
2.43.0

[PATCH v3 14/17] MIPS: mobileye: eyeq5: add OLB reset controller node

[Théo Lebrun]

Add the devicetree node for the reset controller on the Mobileye EyeQ5
platform. It appears as a subnode to the OLB syscon as its registers
are located in this shared register region.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 arch/mips/boot/dts/mobileye/eyeq5.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/mips/boot/dts/mobileye/eyeq5.dtsi b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
index 935c095d1423..a246df6e3c85 100644
--- a/arch/mips/boot/dts/mobileye/eyeq5.dtsi
+++ b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
@@ -110,6 +110,11 @@ clocks: clock-controller {
 				clocks = <&xtal>;
 				clock-names = "ref";
 			};
+
+			reset: reset-controller {
+				compatible = "mobileye,eyeq5-reset";
+				#reset-cells = <2>;
+			};
 		};
 
 		gic: interrupt-controller@140000 {

-- 
2.43.0

[PATCH v3 15/17] MIPS: mobileye: eyeq5: add reset properties to uarts

[Théo Lebrun]

UART nodes have been added to the devicetree by the initial platform
support patch series. Add reset properties now that the reset node is
declared.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 arch/mips/boot/dts/mobileye/eyeq5.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/mips/boot/dts/mobileye/eyeq5.dtsi b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
index a246df6e3c85..11cdbaed2b33 100644
--- a/arch/mips/boot/dts/mobileye/eyeq5.dtsi
+++ b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
@@ -78,6 +78,7 @@ uart0: serial@800000 {
 			interrupts = <GIC_SHARED 6 IRQ_TYPE_LEVEL_HIGH>;
 			clocks  = <&uart_clk>, <&occ_periph>;
 			clock-names = "uartclk", "apb_pclk";
+			resets = <&reset 0 10>;
 		};
 
 		uart1: serial@900000 {
@@ -88,6 +89,7 @@ uart1: serial@900000 {
 			interrupts = <GIC_SHARED 6 IRQ_TYPE_LEVEL_HIGH>;
 			clocks  = <&uart_clk>, <&occ_periph>;
 			clock-names = "uartclk", "apb_pclk";
+			resets = <&reset 0 11>;
 		};
 
 		uart2: serial@a00000 {
@@ -98,6 +100,7 @@ uart2: serial@a00000 {
 			interrupts = <GIC_SHARED 6 IRQ_TYPE_LEVEL_HIGH>;
 			clocks  = <&uart_clk>, <&occ_periph>;
 			clock-names = "uartclk", "apb_pclk";
+			resets = <&reset 0 12>;
 		};
 
 		olb: system-controller@e00000 {

-- 
2.43.0

[PATCH v3 16/17] MIPS: mobileye: eyeq5: add pinctrl nodes & pinmux function nodes

[Théo Lebrun]

Pins on this platform have two functions: GPIO or something-else. We
create function nodes for each something-else based on functions.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 arch/mips/boot/dts/mobileye/eyeq5-pins.dtsi | 128 ++++++++++++++++++++++++++++
 arch/mips/boot/dts/mobileye/eyeq5.dtsi      |  11 +++
 2 files changed, 139 insertions(+)

diff --git a/arch/mips/boot/dts/mobileye/eyeq5-pins.dtsi b/arch/mips/boot/dts/mobileye/eyeq5-pins.dtsi
new file mode 100644
index 000000000000..33df4a021863
--- /dev/null
+++ b/arch/mips/boot/dts/mobileye/eyeq5-pins.dtsi
@@ -0,0 +1,128 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+
+/*
+ * Default pin configuration for Mobileye EyeQ5 boards. We mostly create one
+ * pin configuration node per function.
+ */
+
+&pinctrl0 {
+	timer0_pins: timer0-pins {
+		function = "timer0";
+		pins = "PA0", "PA1";
+	};
+	timer1_pins: timer1-pins {
+		function = "timer1";
+		pins = "PA2", "PA3";
+	};
+	timer2_pins: timer2-pins {
+		function = "timer2";
+		pins = "PA4", "PA5";
+	};
+	pps0_pins: pps0-pin {
+		function = "timer2";
+		pins = "PA4";
+	};
+	pps1_pins: pps1-pin {
+		function = "timer2";
+		pins = "PA5";
+	};
+	timer5_ext_pins: timer5-ext-pins {
+		function = "timer5";
+		pins = "PA6", "PA7", "PA8", "PA9";
+	};
+	timer5_ext_input_pins: timer5-ext-input-pins {
+		function = "timer5";
+		pins = "PA6", "PA7";
+	};
+	timer5_ext_incap_a_pins: timer5-ext-incap-a-pin {
+		function = "timer5";
+		pins = "PA6";
+	};
+	timer5_ext_incap_b_pins: timer5-ext-incap-b-pin {
+		function = "timer5";
+		pins = "PA7";
+	};
+	can0_pins: can0-pins {
+		function = "can0";
+		pins = "PA14", "PA15";
+	};
+	can1_pins: can1-pins {
+		function = "can1";
+		pins = "PA16", "PA17";
+	};
+	uart0_pins: uart0-pins {
+		function = "uart0";
+		pins = "PA10", "PA11";
+	};
+	uart1_pins: uart1-pins {
+		function = "uart1";
+		pins = "PA12", "PA13";
+	};
+	spi0_pins: spi0-pins {
+		function = "spi0";
+		pins = "PA18", "PA19", "PA20", "PA21", "PA22";
+	};
+	spi1_pins: spi1-pins {
+		function = "spi1";
+		pins = "PA23", "PA24", "PA25", "PA26", "PA27";
+	};
+	spi1_slave_pins: spi1-slave-pins {
+		function = "spi1";
+		pins = "PA24", "PA25", "PA26";
+	};
+	refclk0_pins: refclk0-pin {
+		function = "refclk0";
+		pins = "PA28";
+	};
+};
+
+&pinctrl1 {
+	timer3_pins: timer3-pins {
+		function = "timer3";
+		pins = "PB0", "PB1";
+	};
+	timer4_pins: timer4-pins {
+		function = "timer4";
+		pins = "PB2", "PB3";
+	};
+	timer6_ext_pins: timer6-ext-pins {
+		function = "timer6";
+		pins = "PB4", "PB5", "PB6", "PB7";
+	};
+	timer6_ext_input_pins: timer6-ext-input-pins {
+		function = "timer6";
+		pins = "PB4", "PB5";
+	};
+	timer6_ext_incap_a_pins: timer6-ext-incap-a-pin {
+		function = "timer6";
+		pins = "PB4";
+	};
+	timer6_ext_incap_b_pins: timer6-ext-incap-b-pin {
+		function = "timer6";
+		pins = "PB5";
+	};
+	can2_pins: can2-pins {
+		function = "can2";
+		pins = "PB10", "PB11";
+	};
+	uart2_pins: uart2-pins {
+		function = "uart2";
+		pins = "PB8", "PB9";
+	};
+	spi2_pins: spi2-pins {
+		function = "spi2";
+		pins = "PB12", "PB13", "PB14", "PB15", "PB16";
+	};
+	spi3_pins: spi3-pins {
+		function = "spi3";
+		pins = "PB17", "PB18", "PB19", "PB20", "PB21";
+	};
+	spi3_slave_pins: spi3-slave-pins {
+		function = "spi3";
+		pins = "PB18", "PB19", "PB20";
+	};
+	mclk0_pins: mclk0-pin {
+		function = "mclk0";
+		pins = "PB22";
+	};
+};
diff --git a/arch/mips/boot/dts/mobileye/eyeq5.dtsi b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
index 11cdbaed2b33..05987c58ed73 100644
--- a/arch/mips/boot/dts/mobileye/eyeq5.dtsi
+++ b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
@@ -118,6 +118,16 @@ reset: reset-controller {
 				compatible = "mobileye,eyeq5-reset";
 				#reset-cells = <2>;
 			};
+
+			pinctrl0: pinctrl-a {
+				compatible = "mobileye,eyeq5-a-pinctrl";
+				#pinctrl-cells = <1>;
+			};
+
+			pinctrl1: pinctrl-b {
+				compatible = "mobileye,eyeq5-b-pinctrl";
+				#pinctrl-cells = <1>;
+			};
 		};
 
 		gic: interrupt-controller@140000 {
@@ -143,3 +153,4 @@ timer {
 	};
 };
 
+#include "eyeq5-pins.dtsi"

-- 
2.43.0

[PATCH v3 17/17] MIPS: mobileye: eyeq5: add pinctrl properties to UART nodes

[Théo Lebrun]

UART nodes are present in the platform devicetree. Add pinctrl to them
now that the pin controller is supported.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 arch/mips/boot/dts/mobileye/eyeq5.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/mips/boot/dts/mobileye/eyeq5.dtsi b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
index 05987c58ed73..bf06d7a801a2 100644
--- a/arch/mips/boot/dts/mobileye/eyeq5.dtsi
+++ b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
@@ -79,6 +79,8 @@ uart0: serial@800000 {
 			clocks  = <&uart_clk>, <&occ_periph>;
 			clock-names = "uartclk", "apb_pclk";
 			resets = <&reset 0 10>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&uart0_pins>;
 		};
 
 		uart1: serial@900000 {
@@ -90,6 +92,8 @@ uart1: serial@900000 {
 			clocks  = <&uart_clk>, <&occ_periph>;
 			clock-names = "uartclk", "apb_pclk";
 			resets = <&reset 0 11>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&uart1_pins>;
 		};
 
 		uart2: serial@a00000 {
@@ -101,6 +105,8 @@ uart2: serial@a00000 {
 			clocks  = <&uart_clk>, <&occ_periph>;
 			clock-names = "uartclk", "apb_pclk";
 			resets = <&reset 0 12>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&uart2_pins>;
 		};
 
 		olb: system-controller@e00000 {

-- 
2.43.0

Re: [PATCH v3 03/17] dt-bindings: pinctrl: allow pin controller device without unit address

[Rob Herring]

On Tue, 23 Jan 2024 19:46:48 +0100, Théo Lebrun wrote:
> Allow a pin controller device to have no address, therefore no unit
> address.
> 
> The previous $nodename was enforcing a unit address, but
> scripts/dtc/checks.c enforced that names with unit addresses have reg
> or ranges:
> 
>    Warning (unit_address_vs_reg): .../pinctrl@0: node has a unit
>    name, but no reg or ranges property
> 
> Fix pinctrl.yaml to adopt a (pinctrl|pinmux)(-[a-z]+)? node name when
> neither reg nor ranges are required. Use [a-z]+ to avoid conflicts with
> pinctrl-consumer.yaml.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  Documentation/devicetree/bindings/pinctrl/pinctrl.yaml | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/pinctrl/pinctrl.yaml:45:8: [warning] wrong indentation: expected 8 but found 7 (indentation)
./Documentation/devicetree/bindings/pinctrl/pinctrl.yaml:47:8: [warning] wrong indentation: expected 8 but found 7 (indentation)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240123-mbly-clk-v3-3-392b010b8281@bootlin.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v3 04/17] dt-bindings: soc: mobileye: add EyeQ5 OLB system controller

[Rob Herring]

On Tue, 23 Jan 2024 19:46:49 +0100, Théo Lebrun wrote:
> Add documentation to describe the "Other Logic Block" syscon.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  .../bindings/soc/mobileye/mobileye,eyeq5-olb.yaml  | 77 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 78 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml:
Error in referenced schema matching $id: http://devicetree.org/schemas/clock/mobileye,eyeq5-clk.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.example.dtb: system-controller@e00000: clock-controller: False schema does not allow {'compatible': ['mobileye,eyeq5-clk'], '#clock-cells': [[1]], 'clocks': [[4294967295]], 'clock-names': ['ref']}
	from schema $id: http://devicetree.org/schemas/soc/mobileye/mobileye,eyeq5-olb.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.example.dtb: system-controller@e00000: reset-controller: False schema does not allow {'compatible': ['mobileye,eyeq5-reset'], '#reset-cells': [[2]]}
	from schema $id: http://devicetree.org/schemas/soc/mobileye/mobileye,eyeq5-olb.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.example.dtb: system-controller@e00000: pinctrl-a: False schema does not allow {'compatible': ['mobileye,eyeq5-a-pinctrl'], '#pinctrl-cells': [[1]]}
	from schema $id: http://devicetree.org/schemas/soc/mobileye/mobileye,eyeq5-olb.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.example.dtb: system-controller@e00000: pinctrl-b: False schema does not allow {'compatible': ['mobileye,eyeq5-b-pinctrl'], '#pinctrl-cells': [[1]]}
	from schema $id: http://devicetree.org/schemas/soc/mobileye/mobileye,eyeq5-olb.yaml#
Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.example.dtb: /example-0/system-controller@e00000/clock-controller: failed to match any schema with compatible: ['mobileye,eyeq5-clk']
Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.example.dtb: /example-0/system-controller@e00000/reset-controller: failed to match any schema with compatible: ['mobileye,eyeq5-reset']
Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.example.dtb: /example-0/system-controller@e00000/pinctrl-a: failed to match any schema with compatible: ['mobileye,eyeq5-a-pinctrl']
Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.example.dtb: /example-0/system-controller@e00000/pinctrl-b: failed to match any schema with compatible: ['mobileye,eyeq5-b-pinctrl']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240123-mbly-clk-v3-4-392b010b8281@bootlin.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v3 00/17] Add support for Mobileye EyeQ5 system controller

[Krzysztof Kozlowski]

On 23/01/2024 19:46, Théo Lebrun wrote:
> Hi,
> 

You miss here, in this place, the most important information which I
asked previously - dependencies/merging:
https://lore.kernel.org/all/db9b2786-29f7-4d45-9087-a9f85b770b6c@linaro.org/

> The goal of this series is to add clk, reset and pinctrl support for the
> Mobileye EyeQ5 platform [0]. Control of those is grouped inside a
> system controller block called "OLB".
Best regards,
Krzysztof

Re: [PATCH v3 05/17] dt-bindings: clock: mobileye,eyeq5-clk: add bindings

[Krzysztof Kozlowski]

On 23/01/2024 19:46, Théo Lebrun wrote:
> Add DT schema bindings for the EyeQ5 clock controller driver.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

(for this patch... your patchset is still broken as pointed out by robot)

Best regards,
Krzysztof

Re: [PATCH v3 06/17] dt-bindings: reset: mobileye,eyeq5-reset: add bindings

[Krzysztof Kozlowski]

On 23/01/2024 19:46, Théo Lebrun wrote:
> Add DT-Schema bindings for the EyeQ5 reset controller.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  .../bindings/reset/mobileye,eyeq5-reset.yaml       | 32 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 33 insertions(+)
> 

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v3 07/17] dt-bindings: pinctrl: mobileye,eyeq5-pinctrl: add bindings

[Krzysztof Kozlowski]

On 23/01/2024 19:46, Théo Lebrun wrote:
> Add dt-schema type bindings for the Mobileye EyeQ5 pin controller.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  .../bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml   | 77 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 78 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml
> new file mode 100644
> index 000000000000..db62919053b4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml
> @@ -0,0 +1,77 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mobileye EyeQ5 pin controller
> +
> +description:
> +  The EyeQ5 pin controller handles a pin bank. It is custom to this platform,
> +  its registers live in a shared region called OLB.
> +  There are two pin banks on the platform, each having a specific compatible.
> +  Pins and groups are bijective.

I don't see much improvements here.

> +
> +maintainers:
> +  - Grégory Clement <gregory.clement@bootlin.com>
> +  - Théo Lebrun <theo.lebrun@bootlin.com>
> +  - Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mobileye,eyeq5-a-pinctrl
> +      - mobileye,eyeq5-b-pinctrl> +
> +  "#pinctrl-cells":
> +    const: 1
> +
> +required:
> +  - compatible
> +  - "#pinctrl-cells"

required: block goes after all properties and patternproperties.

> +
> +patternProperties:
> +  "-pins?$":
> +    type: object
> +    description: Pin muxing configuration.
> +    $ref: pinmux-node.yaml#
> +    additionalProperties: false
> +    properties:
> +      pins: true
> +      function: true
> +      bias-disable: true
> +      bias-pull-down: true
> +      bias-pull-up: true
> +      drive-strength: true
> +    required:
> +      - pins
> +      - function
> +
> +allOf:
> +  - $ref: pinctrl.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: mobileye,eyeq5-a-pinctrl
> +    then:
> +      patternProperties:
> +        "-pins?$":
> +          properties:
> +            function:
> +              enum: [gpio, timer0, timer1, timer2, timer5, uart0, uart1, can0,
> +                     can1, spi0, spi1, refclk0]
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: mobileye,eyeq5-b-pinctrl
> +    then:
> +      patternProperties:
> +        "-pins?$":
> +          properties:
> +            function:
> +              enum: [gpio, timer3, timer4, timer6, uart2, can2, spi2, spi3,
> +                     mclk0]
> +


Best regards,
Krzysztof

Re: [PATCH v3 09/17] reset: eyeq5: add platform driver

[Krzysztof Kozlowski]

On 23/01/2024 19:46, Théo Lebrun wrote:
> Add the Mobileye EyeQ5 reset controller driver. It belongs to a syscon
> region called OLB. It might grow to add later support of other
> platforms from Mobileye.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  MAINTAINERS                 |   1 +
>  drivers/reset/Kconfig       |  12 ++
>  drivers/reset/Makefile      |   1 +
>  drivers/reset/reset-eyeq5.c | 383 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 397 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3ea96ab7d2b8..dd3b5834386f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14794,6 +14794,7 @@ F:	arch/mips/boot/dts/mobileye/
>  F:	arch/mips/configs/eyeq5_defconfig
>  F:	arch/mips/mobileye/board-epm5.its.S
>  F:	drivers/clk/clk-eyeq5.c
> +F:	drivers/reset/reset-eyeq5.c
>  F:	include/dt-bindings/clock/mobileye,eyeq5-clk.h
>  F:	include/dt-bindings/soc/mobileye,eyeq5.h
>  
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index ccd59ddd7610..80bfde54c076 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -66,6 +66,18 @@ config RESET_BRCMSTB_RESCAL
>  	  This enables the RESCAL reset controller for SATA, PCIe0, or PCIe1 on
>  	  BCM7216.
>  
> +config RESET_EYEQ5
> +	bool "Mobileye EyeQ5 reset controller"
> +	depends on MFD_SYSCON
> +	depends on MACH_EYEQ5 || COMPILE_TEST
> +	default MACH_EYEQ5
> +	help
> +	  This enables the Mobileye EyeQ5 reset controller.
> +
> +	  It has three domains, with a varying number of resets in each of them.
> +	  Registers are located in a shared register region called OLB accessed
> +	  through a syscon & regmap.
> +
>  config RESET_HSDK
>  	bool "Synopsys HSDK Reset Driver"
>  	depends on HAS_IOMEM
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 8270da8a4baa..4fabe0070390 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_RESET_BCM6345) += reset-bcm6345.o
>  obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
>  obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o
>  obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
> +obj-$(CONFIG_RESET_EYEQ5) += reset-eyeq5.o
>  obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
>  obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
>  obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
> diff --git a/drivers/reset/reset-eyeq5.c b/drivers/reset/reset-eyeq5.c
> new file mode 100644
> index 000000000000..2217e42e140b
> --- /dev/null
> +++ b/drivers/reset/reset-eyeq5.c
> @@ -0,0 +1,383 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Reset driver for the Mobileye EyeQ5 platform.
> + *
> + * The registers are located in a syscon region called OLB. We handle three
> + * reset domains. Domains 0 and 2 look similar in that they both use one bit
> + * per reset line. Domain 1 has a register per reset.
> + *
> + * We busy-wait after updating a reset in domains 0 or 1. The reason is hardware
> + * logic built-in self-test (LBIST) that might be enabled.
> + *
> + * We use eq5r_ as prefix, as-in "EyeQ5 Reset", but way shorter.
> + *
> + * Known resets in domain 0:
> + *  3. CAN0
> + *  4. CAN1
> + *  5. CAN2
> + *  6. SPI0
> + *  7. SPI1
> + *  8. SPI2
> + *  9. SPI3
> + * 10. UART0
> + * 11. UART1
> + * 12. UART2
> + * 13. I2C0
> + * 14. I2C1
> + * 15. I2C2
> + * 16. I2C3
> + * 17. I2C4
> + * 18. TIMER0
> + * 19. TIMER1
> + * 20. TIMER2
> + * 21. TIMER3
> + * 22. TIMER4
> + * 23. WD0
> + * 24. EXT0
> + * 25. EXT1
> + * 26. GPIO
> + * 27. WD1
> + *
> + * Known resets in domain 1:
> + * 0. VMP0	(Vector Microcode Processors)
> + * 1. VMP1
> + * 2. VMP2
> + * 3. VMP3
> + * 4. PMA0	(Programmable Macro Array)
> + * 5. PMA1
> + * 6. PMAC0
> + * 7. PMAC1
> + * 8. MPC0	(Multi-threaded Processing Clusters)
> + * 9. MPC1
> + *
> + * Known resets in domain 2:
> + *  0. PCIE0_CORE
> + *  1. PCIE0_APB
> + *  2. PCIE0_LINK_AXI
> + *  3. PCIE0_LINK_MGMT
> + *  4. PCIE0_LINK_HOT
> + *  5. PCIE0_LINK_PIPE
> + *  6. PCIE1_CORE
> + *  7. PCIE1_APB
> + *  8. PCIE1_LINK_AXI
> + *  9. PCIE1_LINK_MGMT
> + * 10. PCIE1_LINK_HOT
> + * 11. PCIE1_LINK_PIPE
> + * 12. MULTIPHY
> + * 13. MULTIPHY_APB
> + * 15. PCIE0_LINK_MGMT
> + * 16. PCIE1_LINK_MGMT
> + * 17. PCIE0_LINK_PM
> + * 18. PCIE1_LINK_PM
> + *
> + * Copyright (C) 2024 Mobileye Vision Technologies Ltd.
> + */
> +
> +#include <linux/mfd/syscon.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset-controller.h>
> +
> +/* Offsets into the OLB region as well as masks for domain 1 registers. */
> +#define EQ5R_OLB_SARCR0			(0x004)
> +#define EQ5R_OLB_SARCR1			(0x008)
> +#define EQ5R_OLB_PCIE_GP		(0x120)
> +#define EQ5R_OLB_ACRP_REG(n)		(0x200 + 4 * (n)) // n=0..12
> +#define EQ5R_OLB_ACRP_PD_REQ		BIT(0)
> +#define EQ5R_OLB_ACRP_ST_POWER_DOWN	BIT(27)
> +#define EQ5R_OLB_ACRP_ST_ACTIVE		BIT(29)
> +
> +/* Vendor-provided values. D1 has a long timeout because of LBIST. */
> +#define D0_TIMEOUT_POLL			10
> +#define D1_TIMEOUT_POLL			40000
> +
> +/*
> + * Masks for valid reset lines in each domain. This array is also used to get
> + * the domain and reset counts.
> + */
> +static const u32 eq5r_valid_masks[] = { 0x0FFFFFF8, 0x00001FFF, 0x0007BFFF };
> +
> +#define EQ5R_DOMAIN_COUNT ARRAY_SIZE(eq5r_valid_masks)
> +
> +struct eq5r_private {
> +	struct mutex mutexes[EQ5R_DOMAIN_COUNT]; /* We serialize all reset operations. */
> +	struct regmap *olb;			 /* Writes go to a syscon regmap. */
> +	struct reset_controller_dev rcdev;
> +};
> +
> +static int _eq5r_busy_wait(struct eq5r_private *priv, struct device *dev,
> +			   u32 domain, u32 offset, bool assert)
> +{
> +	unsigned int val, mask;
> +	int i;
> +
> +	lockdep_assert_held(&priv->mutexes[domain]);
> +
> +	switch (domain) {
> +	case 0:
> +		for (i = 0; i < D0_TIMEOUT_POLL; i++) {
> +			regmap_read(priv->olb, EQ5R_OLB_SARCR1, &val);
> +			val = !(val & BIT(offset));
> +			if (val == assert)
> +				return 0;
> +			__udelay(1);

What is even "__udelay"? It is the first use in drivers. Please use
common methods, like fsleep or udelay... but actually you should rather
use regmap_read_poll_timeout() or some variants instead of open-coding it.


> +		}
> +		break;
> +	case 1:
> +		mask = assert ? EQ5R_OLB_ACRP_ST_POWER_DOWN : EQ5R_OLB_ACRP_ST_ACTIVE;
> +		for (i = 0; i < D1_TIMEOUT_POLL; i++) {
> +			regmap_read(priv->olb, EQ5R_OLB_ACRP_REG(offset), &val);
> +			if (val & mask)
> +				return 0;
> +			__udelay(1);
> +		}
> +		break;
> +	case 2:
> +		return 0; /* No busy waiting for domain 2. */
> +	default:
> +		WARN_ON(1);
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(dev, "%u-%u: timeout\n", domain, offset);
> +	return -ETIMEDOUT;
> +}
> +
> +static void _eq5r_assert(struct eq5r_private *priv, u32 domain, u32 offset)

Drop leading _ and name the function in some informative way.

> +{
> +	lockdep_assert_held(&priv->mutexes[domain]);
> +
> +	switch (domain) {
> +	case 0:
> +		regmap_clear_bits(priv->olb, EQ5R_OLB_SARCR0, BIT(offset));
> +		break;
> +	case 1:
> +		regmap_set_bits(priv->olb, EQ5R_OLB_ACRP_REG(offset),
> +				EQ5R_OLB_ACRP_PD_REQ);
> +		break;
> +	case 2:
> +		regmap_clear_bits(priv->olb, EQ5R_OLB_PCIE_GP, BIT(offset));
> +		break;
> +	default:
> +		WARN_ON(1);
> +	}
> +}
> +
> +static int eq5r_assert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct eq5r_private *priv = dev_get_drvdata(rcdev->dev);
> +	u32 offset = id & GENMASK(7, 0);
> +	u32 domain = id >> 8;
> +	int ret;
> +
> +	if (WARN_ON(domain >= EQ5R_DOMAIN_COUNT))
> +		return -EINVAL;
> +
> +	dev_dbg(rcdev->dev, "%u-%u: assert request\n", domain, offset);
> +
> +	mutex_lock(&priv->mutexes[domain]);
> +	_eq5r_assert(priv, domain, offset);
> +	ret = _eq5r_busy_wait(priv, rcdev->dev, domain, offset, true);
> +	mutex_unlock(&priv->mutexes[domain]);
> +
> +	return ret;
> +}
> +
> +static void _eq5r_deassert(struct eq5r_private *priv, u32 domain, u32 offset)
> +{
> +	lockdep_assert_held(&priv->mutexes[domain]);
> +
> +	switch (domain) {
> +	case 0:
> +		regmap_set_bits(priv->olb, EQ5R_OLB_SARCR0, BIT(offset));
> +		break;
> +	case 1:
> +		regmap_clear_bits(priv->olb, EQ5R_OLB_ACRP_REG(offset),
> +				  EQ5R_OLB_ACRP_PD_REQ);
> +		break;
> +	case 2:
> +		regmap_set_bits(priv->olb, EQ5R_OLB_PCIE_GP, BIT(offset));
> +		break;
> +	default:
> +		WARN_ON(1);
> +	}
> +}
> +
> +static int eq5r_deassert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct eq5r_private *priv = dev_get_drvdata(rcdev->dev);
> +	u32 offset = id & GENMASK(7, 0);
> +	u32 domain = id >> 8;
> +	int ret;
> +
> +	if (WARN_ON(domain >= EQ5R_DOMAIN_COUNT))
> +		return -EINVAL;
> +
> +	dev_dbg(rcdev->dev, "%u-%u: deassert request\n", domain, offset);
> +
> +	mutex_lock(&priv->mutexes[domain]);
> +	_eq5r_deassert(priv, domain, offset);
> +	ret = _eq5r_busy_wait(priv, rcdev->dev, domain, offset, false);
> +	mutex_unlock(&priv->mutexes[domain]);
> +
> +	return ret;
> +}
> +
> +static int eq5r_reset(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct device *dev = rcdev->dev;
> +	struct eq5r_private *priv = dev_get_drvdata(dev);
> +	u32 offset = id & GENMASK(7, 0);
> +	u32 domain = id >> 8;
> +	int ret;
> +
> +	if (WARN_ON(domain >= EQ5R_DOMAIN_COUNT))
> +		return -EINVAL;
> +
> +	dev_dbg(dev, "%u-%u: reset request\n", domain, offset);
> +
> +	mutex_lock(&priv->mutexes[domain]);
> +
> +	_eq5r_assert(priv, domain, offset);
> +	ret = _eq5r_busy_wait(priv, dev, domain, offset, true);
> +	if (ret) /* don't let an error disappear silently */
> +		dev_warn(dev, "%u-%u: reset assert failed: %d\n",
> +			 domain, offset, ret);
> +
> +	_eq5r_deassert(priv, domain, offset);
> +	ret = _eq5r_busy_wait(priv, dev, domain, offset, false);
> +
> +	mutex_unlock(&priv->mutexes[domain]);
> +
> +	return ret;
> +}
> +
> +static int eq5r_status(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct eq5r_private *priv = dev_get_drvdata(rcdev->dev);
> +	u32 offset = id & GENMASK(7, 0);
> +	u32 domain = id >> 8;
> +	unsigned int val;
> +	int ret;
> +
> +	if (WARN_ON(domain >= EQ5R_DOMAIN_COUNT))
> +		return -EINVAL;
> +
> +	dev_dbg(rcdev->dev, "%u-%u: status request\n", domain, offset);
> +
> +	mutex_lock(&priv->mutexes[domain]);
> +
> +	switch (domain) {
> +	case 0:
> +		regmap_read(priv->olb, EQ5R_OLB_SARCR1, &val);
> +		ret = !(val & BIT(offset));
> +		break;
> +	case 1:
> +		regmap_read(priv->olb, EQ5R_OLB_ACRP_REG(offset), &val);
> +		ret = !(val & EQ5R_OLB_ACRP_ST_ACTIVE);
> +		break;
> +	case 2:
> +		regmap_read(priv->olb, EQ5R_OLB_PCIE_GP, &val);
> +		ret = !(val & BIT(offset));
> +		break;
> +	}
> +
> +	mutex_unlock(&priv->mutexes[domain]);
> +
> +	return ret;
> +}
> +
> +static const struct reset_control_ops eq5r_ops = {
> +	.reset	  = eq5r_reset,
> +	.assert	  = eq5r_assert,
> +	.deassert = eq5r_deassert,
> +	.status	  = eq5r_status,
> +};
> +
> +static int eq5r_of_xlate(struct reset_controller_dev *rcdev,
> +			 const struct of_phandle_args *reset_spec)
> +{
> +	u32 domain, offset;
> +
> +	if (WARN_ON(reset_spec->args_count != 2))
> +		return -EINVAL;
> +
> +	domain = reset_spec->args[0];
> +	offset = reset_spec->args[1];
> +
> +	if (domain >= EQ5R_DOMAIN_COUNT || offset > 31 ||
> +	    !(eq5r_valid_masks[domain] & BIT(offset))) {
> +		dev_err(rcdev->dev, "%u-%u: invalid reset\n", domain, offset);
> +		return -EINVAL;
> +	}
> +
> +	return (domain << 8) | offset;
> +}
> +
> +static int eq5r_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct device_node *parent_np = of_get_parent(np);
> +	struct eq5r_private *priv;
> +	int ret, i;
> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;

You leak parent.

> +
> +	dev_set_drvdata(dev, priv);
> +
> +	priv->olb = ERR_PTR(-ENODEV);
> +	if (parent_np) {
> +		priv->olb = syscon_node_to_regmap(parent_np);
> +		of_node_put(parent_np);
> +	}
> +	if (IS_ERR(priv->olb))

Also here

> +		return PTR_ERR(priv->olb);

This looks over-complicated. First, you cannot just
dev_get_regmap(pdev->dev.parent)?



> +
> +	for (i = 0; i < EQ5R_DOMAIN_COUNT; i++)
> +		mutex_init(&priv->mutexes[i]);
> +
> +	priv->rcdev.ops = &eq5r_ops;
> +	priv->rcdev.owner = THIS_MODULE;
> +	priv->rcdev.dev = dev;
> +	priv->rcdev.of_node = np;
> +	priv->rcdev.of_reset_n_cells = 2;
> +	priv->rcdev.of_xlate = eq5r_of_xlate;
> +
> +	priv->rcdev.nr_resets = 0;
> +	for (i = 0; i < EQ5R_DOMAIN_COUNT; i++)
> +		priv->rcdev.nr_resets += __builtin_popcount(eq5r_valid_masks[i]);
> +
> +	ret = reset_controller_register(&priv->rcdev);
> +	if (ret) {
> +		dev_err(dev, "Failed registering reset controller: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id eq5r_match_table[] = {
> +	{ .compatible = "mobileye,eyeq5-reset" },
> +	{}
> +};
> +
> +static struct platform_driver eq5r_driver = {
> +	.probe = eq5r_probe,
> +	.driver = {
> +		.name = "eyeq5-reset",
> +		.of_match_table = eq5r_match_table,
> +	},
> +};
> +
> +static int __init eq5r_init(void)
> +{
> +	return platform_driver_register(&eq5r_driver);
> +}
> +
> +arch_initcall(eq5r_init);

This is does not look like arch code, but driver or subsys. Use regular
module_driver. I see there is such pattern in reset but I doubt this is
something good.

Best regards,
Krzysztof

Re: [PATCH v3 10/17] pinctrl: eyeq5: add platform driver

[Krzysztof Kozlowski]

On 23/01/2024 19:46, Théo Lebrun wrote:
> Add the Mobileye EyeQ5 pin controller driver. It might grow to add later
> support of other platforms from Mobileye. It belongs to a syscon region
> called OLB.
> 
> Existing pins and their function live statically in the driver code
> rather than in the devicetree, see compatible match data.
> 

...

> +static int eq5p_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct device_node *parent_np = of_get_parent(np);
> +	const struct eq5p_match *match = of_device_get_match_data(dev);
> +	struct pinctrl_dev *pctldev;
> +	struct eq5p_pinctrl *pctrl;
> +	int ret;
> +
> +	pctrl = devm_kzalloc(dev, sizeof(*pctrl), GFP_KERNEL);
> +	if (!pctrl)
> +		return -ENOMEM;
> +
> +	pctrl->olb = ERR_PTR(-ENODEV);
> +	if (parent_np)
> +		pctrl->olb = syscon_node_to_regmap(parent_np);
> +	if (IS_ERR(pctrl->olb))
> +		pctrl->olb = syscon_regmap_lookup_by_phandle(np, "mobileye,olb");
> +	if (IS_ERR(pctrl->olb))
> +		return PTR_ERR(pctrl->olb);

No, we talked about this, you got comments on this. There is no
mobileye,olb. You cannot have undocumented properties.

> +
> +	pctrl->regs = match->regs;
> +	pctrl->funcs = match->funcs;
> +	pctrl->nfuncs = match->nfuncs;
> +
> +	pctrl->desc.name = dev_name(dev);
> +	pctrl->desc.pins = match->pins;
> +	pctrl->desc.npins = match->npins;
> +	pctrl->desc.pctlops = &eq5p_pinctrl_ops;
> +	pctrl->desc.pmxops = &eq5p_pinmux_ops;
> +	pctrl->desc.confops = &eq5p_pinconf_ops;
> +	pctrl->desc.owner = THIS_MODULE;
> +
> +	ret = devm_pinctrl_register_and_init(dev, &pctrl->desc, pctrl, &pctldev);
> +	if (ret) {
> +		dev_err(dev, "Failed registering pinctrl device: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = pinctrl_enable(pctldev);
> +	if (ret) {
> +		dev_err(dev, "Failed enabling pinctrl device: %d\n", ret);
> +		return ret;
> +	}
> +
> +	dev_info(dev, "probed\n");

I am pretty sure you got comments for these. Drop such debugs from all
of your code. Current and future.

> +
> +	return 0;
> +}
> +
> +static const struct eq5p_match eq5p_match_a = {
> +	.regs = {
> +		[EQ5P_PD] = 0x0C0,
> +		[EQ5P_PU] = 0x0C4,
> +		[EQ5P_DS_LOW] = 0x0D0,
> +		[EQ5P_DS_HIGH] = 0x0D4,
> +		[EQ5P_IOCR] = 0x0B0,
> +	},
> +	.pins = eq5p_pins_a,
> +	.npins = ARRAY_SIZE(eq5p_pins_a),
> +	.funcs = eq5p_functions_a,
> +	.nfuncs = ARRAY_SIZE(eq5p_functions_a),
> +};
> +
> +static const struct eq5p_match eq5p_match_b = {
> +	.regs = {
> +		[EQ5P_PD] = 0x0C8,
> +		[EQ5P_PU] = 0x0CC,
> +		[EQ5P_DS_LOW] = 0x0D8,
> +		[EQ5P_DS_HIGH] = 0x0DC,
> +		[EQ5P_IOCR] = 0x0B4,
> +	},
> +	.pins = eq5p_pins_b,
> +	.npins = ARRAY_SIZE(eq5p_pins_b),
> +	.funcs = eq5p_functions_b,
> +	.nfuncs = ARRAY_SIZE(eq5p_functions_b),
> +};
> +
> +static const struct of_device_id eq5p_match[] = {
> +	{ .compatible = "mobileye,eyeq5-a-pinctrl", .data = &eq5p_match_a },
> +	{ .compatible = "mobileye,eyeq5-b-pinctrl", .data = &eq5p_match_b },
> +	{},
> +};
> +
> +static struct platform_driver eq5p_driver = {
> +	.driver = {
> +		.name = "eyeq5-pinctrl",
> +		.of_match_table = eq5p_match,
> +	},
> +	.probe = eq5p_probe,
> +};
> +
> +static int __init eq5p_init(void)
> +{
> +	return platform_driver_register(&eq5p_driver);
> +}
> +core_initcall(eq5p_init);

No, pins are not a core_initcall. This could be arch_initcall, but
considering you depend on the parent this must be module driver.

Even from this dependency point of view your initcalls are totally wrong
and will lead to issues.

Best regards,
Krzysztof

Re: [PATCH v3 08/17] clk: eyeq5: add platform driver

[Krzysztof Kozlowski]

On 23/01/2024 19:46, Théo Lebrun wrote:
> Add the Mobileye EyeQ5 clock controller driver. It might grow to add
> support for other platforms from Mobileye.
> 
> It handles 10 read-only PLLs derived from the main crystal on board. It
> exposes a table-based divider clock used for OSPI. Other platform
> clocks are not configurable and therefore kept as fixed-factor
> devicetree nodes.
> 
> Two PLLs are required early on and are therefore registered at
> of_clk_init(). Those are pll-cpu for the GIC timer and pll-per for the
> UARTs.
> 


> +#define OLB_PCSR1_RESET				BIT(0)
> +#define OLB_PCSR1_SSGC_DIV			GENMASK(4, 1)
> +/* Spread amplitude (% = 0.1 * SPREAD[4:0]) */
> +#define OLB_PCSR1_SPREAD			GENMASK(9, 5)
> +#define OLB_PCSR1_DIS_SSCG			BIT(10)
> +/* Down-spread or center-spread */
> +#define OLB_PCSR1_DOWN_SPREAD			BIT(11)
> +#define OLB_PCSR1_FRAC_IN			GENMASK(31, 12)
> +
> +static struct clk_hw_onecell_data *eq5c_clk_data;
> +static struct regmap *eq5c_olb;

Drop these two. No file-scope regmaps for drivers. Use private container
structures.

...

> +static void __init eq5c_init(struct device_node *np)
> +{
> +	struct device_node *parent_np = of_get_parent(np);
> +	int i, ret;
> +
> +	eq5c_clk_data = kzalloc(struct_size(eq5c_clk_data, hws, EQ5C_NB_CLKS),
> +				GFP_KERNEL);
> +	if (!eq5c_clk_data) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	eq5c_clk_data->num = EQ5C_NB_CLKS;
> +
> +	/*
> +	 * Mark all clocks as deferred. We register some now and others at
> +	 * platform device probe.
> +	 */
> +	for (i = 0; i < EQ5C_NB_CLKS; i++)
> +		eq5c_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
> +
> +	/*
> +	 * Currently, if OLB is not available, we log an error, fail init then

How it could be not available? Only with broken initcall ordering. Fix
your initcall ordering and then simplify all this weird code.

> +	 * fail probe. We might want to change this behavior and assume all
> +	 * clocks are in bypass mode; this is what is being done in the vendor
> +	 * driver.
> +	 *
> +	 * It is unclear if there are valid situations where the OLB region
> +	 * would be inaccessible.



Best regards,
Krzysztof

Re: [PATCH v3 12/17] MIPS: mobileye: eyeq5: remove reg-io-width property from OLB syscon

[Sergei Shtylyov]

On 1/23/24 9:46 PM, Théo Lebrun wrote:

> Remove the `reg-io-width` property from the olb@e00000 syscon. The

   The diff says it's system-controller@e00000 now... :-)

> default memory access width is what we desire: no need to make it
> explicit.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  arch/mips/boot/dts/mobileye/eyeq5.dtsi | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/mips/boot/dts/mobileye/eyeq5.dtsi b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
> index 81497febcdee..03e7e942ee22 100644
> --- a/arch/mips/boot/dts/mobileye/eyeq5.dtsi
> +++ b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
> @@ -103,7 +103,6 @@ uart2: serial@a00000 {
>  		olb: system-controller@e00000 {
>  			compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd";
>  			reg = <0 0xe00000 0x0 0x400>;
> -			reg-io-width = <4>;
>  		};
>  
>  		gic: interrupt-controller@140000 {

MBR, Sergey

Re: [PATCH v3 09/17] reset: eyeq5: add platform driver

[Philipp Zabel]

On Di, 2024-01-23 at 19:46 +0100, Théo Lebrun wrote:
[...]
> diff --git a/drivers/reset/reset-eyeq5.c b/drivers/reset/reset-eyeq5.c
> new file mode 100644
> index 000000000000..2217e42e140b
> --- /dev/null
> +++ b/drivers/reset/reset-eyeq5.c
> @@ -0,0 +1,383 @@
[...]

> +static int eq5r_assert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct eq5r_private *priv = dev_get_drvdata(rcdev->dev);

rcdev is contained in priv, you can just use container_of instead of
chasing pointers around.

> +	u32 offset = id & GENMASK(7, 0);
> +	u32 domain = id >> 8;
> +	int ret;
> +
> +	if (WARN_ON(domain >= EQ5R_DOMAIN_COUNT))
> +		return -EINVAL;

Reset controls with domain >= EQ5R_DOMAIN_COUNT are already weeded out
during request by of_xlate, so this check is not necessary.

> +	dev_dbg(rcdev->dev, "%u-%u: assert request\n", domain, offset);
> +
> +	mutex_lock(&priv->mutexes[domain]);
> +	_eq5r_assert(priv, domain, offset);
> +	ret = _eq5r_busy_wait(priv, rcdev->dev, domain, offset, true);
> +	mutex_unlock(&priv->mutexes[domain]);
> +
> +	return ret;

Consider using guard(mutex)(&priv->mutexes[domain]) from
linux/cleanup.h to automatically unlock on return.

[...]
> +static int eq5r_reset(struct reset_controller_dev *rcdev, unsigned long id)

Is this used by anything? If unused, I'd prefer this not to be
implemented. If it is used, is no delay required between assert and
deassert by any consumer?

> +{
> +	struct device *dev = rcdev->dev;
> +	struct eq5r_private *priv = dev_get_drvdata(dev);
> +	u32 offset = id & GENMASK(7, 0);
> +	u32 domain = id >> 8;
> +	int ret;
> +
> +	if (WARN_ON(domain >= EQ5R_DOMAIN_COUNT))
> +		return -EINVAL;
> +
> +	dev_dbg(dev, "%u-%u: reset request\n", domain, offset);
> +
> +	mutex_lock(&priv->mutexes[domain]);
> +
> +	_eq5r_assert(priv, domain, offset);
> +	ret = _eq5r_busy_wait(priv, dev, domain, offset, true);
> +	if (ret) /* don't let an error disappear silently */
> +		dev_warn(dev, "%u-%u: reset assert failed: %d\n",
> +			 domain, offset, ret);

Why not return the error though?

> +	_eq5r_deassert(priv, domain, offset);
> +	ret = _eq5r_busy_wait(priv, dev, domain, offset, false);
> +
> +	mutex_unlock(&priv->mutexes[domain]);
> +
> +	return ret;
> +}
[...]
> +static int eq5r_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct device_node *parent_np = of_get_parent(np);
> +	struct eq5r_private *priv;
> +	int ret, i;
> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);

Using devm_kzalloc() avoids leaking this on error return or driver
unbind.

> +	if (!priv)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(dev, priv);
> +
> +	priv->olb = ERR_PTR(-ENODEV);
> +	if (parent_np) {
> +		priv->olb = syscon_node_to_regmap(parent_np);
> +		of_node_put(parent_np);
> +	}
> +	if (IS_ERR(priv->olb))
> +		return PTR_ERR(priv->olb);
> +
> +	for (i = 0; i < EQ5R_DOMAIN_COUNT; i++)
> +		mutex_init(&priv->mutexes[i]);
> +
> +	priv->rcdev.ops = &eq5r_ops;
> +	priv->rcdev.owner = THIS_MODULE;
> +	priv->rcdev.dev = dev;
> +	priv->rcdev.of_node = np;
> +	priv->rcdev.of_reset_n_cells = 2;
> +	priv->rcdev.of_xlate = eq5r_of_xlate;
> +
> +	priv->rcdev.nr_resets = 0;
> +	for (i = 0; i < EQ5R_DOMAIN_COUNT; i++)
> +		priv->rcdev.nr_resets += __builtin_popcount(eq5r_valid_masks[i]);
> +
> +	ret = reset_controller_register(&priv->rcdev);

Similarly, use devm_reset_controller_register() or disable driver
unbind with suppress_bind_attrs.

regards
Philipp

Re: [PATCH v3 04/17] dt-bindings: soc: mobileye: add EyeQ5 OLB system controller

[Rob Herring]

On Tue, Jan 23, 2024 at 07:46:49PM +0100, Théo Lebrun wrote:
> Add documentation to describe the "Other Logic Block" syscon.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  .../bindings/soc/mobileye/mobileye,eyeq5-olb.yaml  | 77 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 78 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml
> new file mode 100644
> index 000000000000..031ef6a532c1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml
> @@ -0,0 +1,77 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/mobileye/mobileye,eyeq5-olb.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mobileye EyeQ5 SoC system controller
> +
> +maintainers:
> +  - Grégory Clement <gregory.clement@bootlin.com>
> +  - Théo Lebrun <theo.lebrun@bootlin.com>
> +  - Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>
> +
> +description:
> +  OLB ("Other Logic Block") is a hardware block grouping smaller blocks. Clocks,
> +  resets, pinctrl are being handled from here.
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: mobileye,eyeq5-olb
> +      - const: syscon
> +      - const: simple-mfd
> +
> +  reg:
> +    maxItems: 1
> +
> +  clock-controller:
> +    $ref: /schemas/clock/mobileye,eyeq5-clk.yaml#
> +    type: object
> +
> +  reset-controller:
> +    $ref: /schemas/reset/mobileye,eyeq5-reset.yaml#
> +    type: object
> +
> +  pinctrl-a:
> +    $ref: /schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml#
> +    type: object
> +
> +  pinctrl-b:
> +    $ref: /schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml#
> +    type: object
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    system-controller@e00000 {
> +      compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd";
> +      reg = <0xe00000 0x400>;
> +
> +      clock-controller {
> +        compatible = "mobileye,eyeq5-clk";
> +        #clock-cells = <1>;
> +        clocks = <&xtal>;
> +        clock-names = "ref";
> +      };
> +
> +      reset-controller {
> +        compatible = "mobileye,eyeq5-reset";
> +        #reset-cells = <2>;
> +      };
> +
> +      pinctrl-a {
> +        compatible = "mobileye,eyeq5-a-pinctrl";
> +        #pinctrl-cells = <1>;

Sure you need this? Generally only pinctrl-single uses this.

> +      };
> +
> +      pinctrl-b {
> +        compatible = "mobileye,eyeq5-b-pinctrl";
> +        #pinctrl-cells = <1>;
> +      };
> +    };

This can all be simplified to:

system-controller@e00000 {
    compatible = "mobileye,eyeq5-olb", "syscon";
    reg = <0xe00000 0x400>;
    #reset-cells = <2>;
    #clock-cells = <1>;
    clocks = <&xtal>;
    clock-names = "ref";

    pins { ... };
};

There is no need for sub nodes unless you have reusable blocks or each 
block has its own resources in DT.

Rob

Re: [PATCH v3 10/17] pinctrl: eyeq5: add platform driver

[Rob Herring]

On Tue, Jan 23, 2024 at 07:46:55PM +0100, Théo Lebrun wrote:
> Add the Mobileye EyeQ5 pin controller driver. It might grow to add later
> support of other platforms from Mobileye. It belongs to a syscon region
> called OLB.
> 
> Existing pins and their function live statically in the driver code
> rather than in the devicetree, see compatible match data.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  MAINTAINERS                     |   1 +
>  drivers/pinctrl/Kconfig         |  15 +
>  drivers/pinctrl/Makefile        |   1 +
>  drivers/pinctrl/pinctrl-eyeq5.c | 595 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 612 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dd3b5834386f..9c423a4feb86 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14794,6 +14794,7 @@ F:	arch/mips/boot/dts/mobileye/
>  F:	arch/mips/configs/eyeq5_defconfig
>  F:	arch/mips/mobileye/board-epm5.its.S
>  F:	drivers/clk/clk-eyeq5.c
> +F:	drivers/pinctrl/pinctrl-eyeq5.c
>  F:	drivers/reset/reset-eyeq5.c
>  F:	include/dt-bindings/clock/mobileye,eyeq5-clk.h
>  F:	include/dt-bindings/soc/mobileye,eyeq5.h
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index 8163a5983166..abe94de85b3d 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -195,6 +195,21 @@ config PINCTRL_EQUILIBRIUM
>  	  desired pin functions, configure GPIO attributes for LGM SoC pins.
>  	  Pin muxing and pin config settings are retrieved from device tree.
>  
> +config PINCTRL_EYEQ5
> +	bool "Mobileye EyeQ5 pinctrl driver"
> +	depends on OF
> +	depends on MACH_EYEQ5 || COMPILE_TEST
> +	select PINMUX
> +	select GENERIC_PINCONF
> +	select MFD_SYSCON
> +	default MACH_EYEQ5
> +	help
> +	  Pin controller driver for the Mobileye EyeQ5 platform. It does both
> +	  pin config & pin muxing. It does not handle GPIO.
> +
> +	  Pin muxing supports two functions for each pin: first is GPIO, second
> +	  is pin-dependent. Pin config is about bias & drive strength.
> +
>  config PINCTRL_GEMINI
>  	bool
>  	depends on ARCH_GEMINI
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> index 1071f301cc70..0033940914d9 100644
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> @@ -22,6 +22,7 @@ obj-$(CONFIG_PINCTRL_DA850_PUPD) += pinctrl-da850-pupd.o
>  obj-$(CONFIG_PINCTRL_DA9062)	+= pinctrl-da9062.o
>  obj-$(CONFIG_PINCTRL_DIGICOLOR)	+= pinctrl-digicolor.o
>  obj-$(CONFIG_PINCTRL_EQUILIBRIUM)   += pinctrl-equilibrium.o
> +obj-$(CONFIG_PINCTRL_EYEQ5)	+= pinctrl-eyeq5.o
>  obj-$(CONFIG_PINCTRL_GEMINI)	+= pinctrl-gemini.o
>  obj-$(CONFIG_PINCTRL_INGENIC)	+= pinctrl-ingenic.o
>  obj-$(CONFIG_PINCTRL_K210)	+= pinctrl-k210.o
> diff --git a/drivers/pinctrl/pinctrl-eyeq5.c b/drivers/pinctrl/pinctrl-eyeq5.c
> new file mode 100644
> index 000000000000..2d8e5b2168bd
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-eyeq5.c
> @@ -0,0 +1,595 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Pinctrl driver for the Mobileye EyeQ5 platform.
> + *
> + * The registers are located in a syscon region called OLB. There are two pin
> + * banks, each being controlled by 5 registers (see enum eq5p_regs) for
> + * pull-down, pull-up, drive strength and muxing.
> + *
> + * For each pin, muxing is between two functions: (0) GPIO or (1) another one
> + * that is pin-dependent. Functions are declared statically in this driver.
> + *
> + * We create pinctrl groups that are 1:1 equivalent to pins: each group has a
> + * single pin, and its index/selector is the pin number/offset.
> + *
> + * We use eq5p_ as prefix, as-in "EyeQ5 Pinctrl", but way shorter.
> + *
> + * Copyright (C) 2024 Mobileye Vision Technologies Ltd.
> + */
> +#include <linux/mfd/syscon.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/seq_file.h>
> +
> +#include "core.h"
> +#include "pinctrl-utils.h"
> +
> +#define GPIO_FUNC_SELECTOR 0
> +
> +struct eq5p_pinctrl {
> +	struct pinctrl_desc desc;
> +
> +	struct regmap *olb;
> +	const unsigned int *regs; /* array of size EQ5P_REG_MAX */
> +
> +	const struct eq5p_function *funcs;
> +	unsigned int nfuncs;
> +};
> +
> +struct eq5p_function {
> +	const char *name;
> +	const char * const *groups;
> +	unsigned int ngroups;
> +};
> +
> +/* OLB registers; those are offsets in an array of address offsets. */
> +enum eq5p_regs {
> +	EQ5P_PD,
> +	EQ5P_PU,
> +	EQ5P_DS_LOW,
> +	EQ5P_DS_HIGH,
> +	EQ5P_IOCR,
> +
> +	EQ5P_REG_MAX
> +};
> +
> +static int eq5p_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
> +{
> +	return pctldev->desc->npins;
> +}
> +
> +static const char *eq5p_pinctrl_get_group_name(struct pinctrl_dev *pctldev,
> +					       unsigned int selector)
> +{
> +	return pctldev->desc->pins[selector].name;
> +}
> +
> +static int eq5p_pinctrl_get_group_pins(struct pinctrl_dev *pctldev,
> +				       unsigned int selector,
> +				       const unsigned int **pins,
> +				       unsigned int *num_pins)
> +{
> +	*pins = &pctldev->desc->pins[selector].number;
> +	*num_pins = 1;
> +	return 0;
> +}
> +
> +static int eq5p_pinconf_get(struct pinctrl_dev *pctldev, unsigned int offset,
> +			    unsigned long *config);
> +
> +static void eq5p_pinctrl_pin_dbg_show(struct pinctrl_dev *pctldev,
> +				      struct seq_file *s,
> +				      unsigned int offset)
> +{
> +	struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +	const char *pin_name = pctrl->desc.pins[offset].name;
> +	const char *func_name, *bias;
> +	unsigned int val_pd, val_pu, val_iocr;
> +	unsigned long ds_config;
> +	u32 drive_strength;
> +	bool pd, pu;
> +	int i, j;
> +
> +	/* First, let's get the function name. */
> +	regmap_read(pctrl->olb, pctrl->regs[EQ5P_IOCR], &val_iocr);
> +	if ((val_iocr & BIT(offset)) == 0) {
> +		func_name = pctrl->funcs[GPIO_FUNC_SELECTOR].name;
> +	} else {
> +		/* All pins have only two functions: GPIO and something else. We
> +		 * look for this something else.
> +		 */
> +		func_name = NULL;
> +		for (i = 0; i < pctrl->nfuncs; i++) {
> +			if (i == GPIO_FUNC_SELECTOR)
> +				continue;
> +
> +			for (j = 0; j < pctrl->funcs[i].ngroups; j++) {
> +				/* Groups and pins are the same thing for us. */
> +				const char *x = pctrl->funcs[i].groups[j];
> +
> +				if (strcmp(x, pin_name) == 0) {
> +					func_name = pctrl->funcs[i].name;
> +					break;
> +				}
> +			}
> +
> +			if (func_name)
> +				break;
> +		}
> +
> +		/* We have not found the function attached to this pin, this
> +		 * should never occur as all pins have exactly two functions.
> +		 */
> +		if (!func_name)
> +			func_name = "unknown";
> +	}
> +
> +	/* Second, we retrieve the bias. */
> +	regmap_read(pctrl->olb, pctrl->regs[EQ5P_PD], &val_pd);
> +	pd = (val_pd & BIT(offset)) != 0;
> +	regmap_read(pctrl->olb, pctrl->regs[EQ5P_PU], &val_pu);
> +	pu = (val_pu & BIT(offset)) != 0;
> +	if (pd && pu)
> +		bias = "both";
> +	else if (pd && !pu)
> +		bias = "pulldown";
> +	else if (!pd && pu)
> +		bias = "pullup";
> +	else
> +		bias = "none";
> +
> +	/* Third, we get the drive strength. */
> +	ds_config = pinconf_to_config_packed(PIN_CONFIG_DRIVE_STRENGTH, 0);
> +	eq5p_pinconf_get(pctldev, offset, &ds_config);
> +	drive_strength = pinconf_to_config_argument(ds_config);
> +
> +	seq_printf(s, "function=%s bias=%s drive_strength=%d",
> +		   func_name, bias, drive_strength);
> +}
> +
> +static const struct pinctrl_ops eq5p_pinctrl_ops = {
> +	.get_groups_count = eq5p_pinctrl_get_groups_count,
> +	.get_group_name = eq5p_pinctrl_get_group_name,
> +	.get_group_pins = eq5p_pinctrl_get_group_pins,
> +	.pin_dbg_show = eq5p_pinctrl_pin_dbg_show,
> +	.dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
> +	.dt_free_map = pinctrl_utils_free_map,
> +};
> +
> +static int eq5p_pinmux_get_functions_count(struct pinctrl_dev *pctldev)
> +{
> +	struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return pctrl->nfuncs;
> +}
> +
> +static const char *eq5p_pinmux_get_function_name(struct pinctrl_dev *pctldev,
> +						 unsigned int selector)
> +{
> +	struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return pctrl->funcs[selector].name;
> +}
> +
> +static int eq5p_pinmux_get_function_groups(struct pinctrl_dev *pctldev,
> +					   unsigned int selector,
> +					   const char * const **groups,
> +					   unsigned int *num_groups)
> +{
> +	struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	*groups = pctrl->funcs[selector].groups;
> +	*num_groups = pctrl->funcs[selector].ngroups;
> +	return 0;
> +}
> +
> +static int eq5p_pinmux_set_mux(struct pinctrl_dev *pctldev,
> +			       unsigned int func_selector, unsigned int offset)
> +{
> +	const char *group_name = pctldev->desc->pins[offset].name;
> +	struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +	const char *func_name = pctrl->funcs[func_selector].name;
> +	bool is_gpio = func_selector == GPIO_FUNC_SELECTOR;
> +	unsigned int reg, mask, val;
> +
> +	dev_dbg(pctldev->dev, "%s: func=%s group=%s\n", __func__, func_name,
> +		group_name);
> +
> +	reg = pctrl->regs[EQ5P_IOCR];
> +	mask = BIT(offset);
> +	val = is_gpio ? 0 : U32_MAX;
> +
> +	regmap_update_bits(pctrl->olb, reg, mask, val);
> +
> +	return 0;
> +}
> +
> +static int eq5p_pinmux_gpio_request_enable(struct pinctrl_dev *pctldev,
> +					   struct pinctrl_gpio_range *range,
> +					   unsigned int offset)
> +{
> +	/* Pin offsets and group selectors are the same thing in our case. */
> +	return eq5p_pinmux_set_mux(pctldev, GPIO_FUNC_SELECTOR, offset);
> +}
> +
> +static const struct pinmux_ops eq5p_pinmux_ops = {
> +	.get_functions_count = eq5p_pinmux_get_functions_count,
> +	.get_function_name = eq5p_pinmux_get_function_name,
> +	.get_function_groups = eq5p_pinmux_get_function_groups,
> +	.set_mux = eq5p_pinmux_set_mux,
> +	.gpio_request_enable = eq5p_pinmux_gpio_request_enable,
> +	.strict = true,
> +};
> +
> +static int eq5p_pinconf_get(struct pinctrl_dev *pctldev, unsigned int offset,
> +			    unsigned long *config)
> +{
> +	enum pin_config_param param = pinconf_to_config_param(*config);
> +	struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +	unsigned int reg, val_pd, val_pu, val_ds;
> +	bool pd, pu;
> +	u32 arg = 0;
> +
> +	regmap_read(pctrl->olb, pctrl->regs[EQ5P_PD], &val_pd);
> +	pd = (val_pd & BIT(offset)) != 0;
> +	regmap_read(pctrl->olb, pctrl->regs[EQ5P_PU], &val_pu);
> +	pu = (val_pu & BIT(offset)) != 0;
> +
> +	switch (param) {
> +	case PIN_CONFIG_BIAS_DISABLE:
> +		arg = !(pd || pu);
> +		break;
> +	case PIN_CONFIG_BIAS_PULL_DOWN:
> +		arg = pd;
> +		break;
> +	case PIN_CONFIG_BIAS_PULL_UP:
> +		arg = pu;
> +		break;
> +	case PIN_CONFIG_DRIVE_STRENGTH:
> +		offset *= 2; /* two bits per pin */
> +		if (offset >= 32) {
> +			reg = pctrl->regs[EQ5P_DS_HIGH];
> +			offset -= 32;
> +		} else {
> +			reg = pctrl->regs[EQ5P_DS_LOW];
> +		}
> +		regmap_read(pctrl->olb, reg, &val_ds);
> +		arg = (val_ds >> offset) & 0b11;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	*config = pinconf_to_config_packed(param, arg);
> +	return 0;
> +}
> +
> +static int eq5p_pinconf_set_drive_strength(struct pinctrl_dev *pctldev,
> +					   unsigned int offset, u32 arg)
> +{
> +	struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +	unsigned int reg, mask, val;
> +
> +	if (arg > 3) {
> +		dev_err(pctldev->dev, "Unsupported drive strength: %u\n", arg);
> +		return -EINVAL;
> +	}
> +
> +	offset *= 2; /* two bits per pin */
> +
> +	if (offset >= 32) {
> +		reg = pctrl->regs[EQ5P_DS_HIGH];
> +		offset -= 32;
> +	} else {
> +		reg = pctrl->regs[EQ5P_DS_LOW];
> +	}
> +
> +	mask = 0b11 << offset;
> +	val = arg << offset;
> +	regmap_update_bits(pctrl->olb, reg, mask, val);
> +	return 0;
> +}
> +
> +static int eq5p_pinconf_set(struct pinctrl_dev *pctldev, unsigned int offset,
> +			    unsigned long *configs, unsigned int num_configs)
> +{
> +	struct eq5p_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +	const char *pin_name = pctldev->desc->pins[offset].name;
> +	struct device *dev = pctldev->dev;
> +	unsigned int i, val, reg_pd, reg_pu;
> +
> +	val = BIT(offset);
> +	reg_pd = pctrl->regs[EQ5P_PD];
> +	reg_pu = pctrl->regs[EQ5P_PU];
> +
> +	for (i = 0; i < num_configs; i++) {
> +		enum pin_config_param param = pinconf_to_config_param(configs[i]);
> +		u32 arg = pinconf_to_config_argument(configs[i]);
> +
> +		switch (param) {
> +		case PIN_CONFIG_BIAS_DISABLE:
> +			dev_dbg(dev, "%s: pin=%s BIAS_DISABLE\n",
> +				__func__, pin_name);
> +			regmap_clear_bits(pctrl->olb, reg_pd, val);
> +			regmap_clear_bits(pctrl->olb, reg_pu, val);
> +			break;
> +		case PIN_CONFIG_BIAS_PULL_DOWN:
> +			dev_dbg(dev, "%s: pin=%s BIAS_PULL_DOWN arg=%u\n",
> +				__func__, pin_name, arg);
> +			if (arg == 0) /* cannot connect to GND */
> +				return -EOPNOTSUPP;
> +			regmap_set_bits(pctrl->olb, reg_pd, val);
> +			regmap_clear_bits(pctrl->olb, reg_pu, val);
> +			break;
> +		case PIN_CONFIG_BIAS_PULL_UP:
> +			dev_dbg(dev, "%s: pin=%s BIAS_PULL_UP arg=%u\n",
> +				__func__, pin_name, arg);
> +			if (arg == 0) /* cannot connect to VDD */
> +				return -EOPNOTSUPP;
> +			regmap_clear_bits(pctrl->olb, reg_pd, val);
> +			regmap_set_bits(pctrl->olb, reg_pu, val);
> +			break;
> +		case PIN_CONFIG_DRIVE_STRENGTH:
> +			dev_dbg(dev, "%s: pin=%s DRIVE_STRENGTH arg=%u\n",
> +				__func__, pin_name, arg);
> +			eq5p_pinconf_set_drive_strength(pctldev, offset, arg);
> +			break;
> +		default:
> +			dev_err(dev, "Unsupported pinconf %u\n", param);
> +			return -EOPNOTSUPP;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct pinconf_ops eq5p_pinconf_ops = {
> +	.is_generic = true,
> +	.pin_config_get = eq5p_pinconf_get,
> +	.pin_config_set = eq5p_pinconf_set,
> +	/* Pins and groups are equivalent in this driver. */
> +	.pin_config_group_get = eq5p_pinconf_get,
> +	.pin_config_group_set = eq5p_pinconf_set,
> +};
> +
> +/*
> + * Comments to the right of each pin are the "signal name" in the datasheet.
> + */
> +
> +static const struct pinctrl_pin_desc eq5p_pins_a[] = { /* Bank A */
> +	PINCTRL_PIN(0,  "PA0"),  /* A0_TIMER0_CK */
> +	PINCTRL_PIN(1,  "PA1"),  /* A1_TIMER0_EOC */
> +	PINCTRL_PIN(2,  "PA2"),  /* A2_TIMER1_CK */
> +	PINCTRL_PIN(3,  "PA3"),  /* A3_TIMER1_EOC */
> +	PINCTRL_PIN(4,  "PA4"),  /* A4_TIMER2_CK */
> +	PINCTRL_PIN(5,  "PA5"),  /* A5_TIMER2_EOC */
> +	PINCTRL_PIN(6,  "PA6"),  /* A6_TIMER5_EXT_INCAP1 */
> +	PINCTRL_PIN(7,  "PA7"),  /* A7_TIMER5_EXT_INCAP2 */
> +	PINCTRL_PIN(8,  "PA8"),  /* A8_TIMER5_EXT_OUTCMP1 */
> +	PINCTRL_PIN(9,  "PA9"),  /* A9_TIMER5_EXT_OUTCMP2 */
> +	PINCTRL_PIN(10, "PA10"), /* A10_UART_0_TX */
> +	PINCTRL_PIN(11, "PA11"), /* A11_UART_0_RX */
> +	PINCTRL_PIN(12, "PA12"), /* A12_UART_1_TX */
> +	PINCTRL_PIN(13, "PA13"), /* A13_UART_1_RX */
> +	PINCTRL_PIN(14, "PA14"), /* A14_CAN_0_TX */
> +	PINCTRL_PIN(15, "PA15"), /* A15_CAN_0_RX */
> +	PINCTRL_PIN(16, "PA16"), /* A16_CAN_1_TX */
> +	PINCTRL_PIN(17, "PA17"), /* A17_CAN_1_RX */
> +	PINCTRL_PIN(18, "PA18"), /* A18_SPI_0_DO */
> +	PINCTRL_PIN(19, "PA19"), /* A19_SPI_0_DI */
> +	PINCTRL_PIN(20, "PA20"), /* A20_SPI_0_CK */
> +	PINCTRL_PIN(21, "PA21"), /* A21_SPI_0_CS0 */
> +	PINCTRL_PIN(22, "PA22"), /* A22_SPI_0_CS1 */
> +	PINCTRL_PIN(23, "PA23"), /* A23_SPI_1_DO */
> +	PINCTRL_PIN(24, "PA24"), /* A24_SPI_1_DI */
> +	PINCTRL_PIN(25, "PA25"), /* A25_SPI_1_CK */
> +	PINCTRL_PIN(26, "PA26"), /* A26_SPI_1_CS0 */
> +	PINCTRL_PIN(27, "PA27"), /* A27_SPI_1_CS1 */
> +	PINCTRL_PIN(28, "PA28"), /* A28_REF_CLK0 */
> +};
> +
> +static const struct pinctrl_pin_desc eq5p_pins_b[] = { /* Bank B */
> +	PINCTRL_PIN(0,  "PB0"),  /* B0_TIMER3_CK */
> +	PINCTRL_PIN(1,  "PB1"),  /* B1_TIMER3_EOC */
> +	PINCTRL_PIN(2,  "PB2"),  /* B2_TIMER4_CK */
> +	PINCTRL_PIN(3,  "PB3"),  /* B3_TIMER4_EOC */
> +	PINCTRL_PIN(4,  "PB4"),  /* B4_TIMER6_EXT_INCAP1 */
> +	PINCTRL_PIN(5,  "PB5"),  /* B5_TIMER6_EXT_INCAP2 */
> +	PINCTRL_PIN(6,  "PB6"),  /* B6_TIMER6_EXT_OUTCMP1 */
> +	PINCTRL_PIN(7,  "PB7"),  /* B7_TIMER6_EXT_OUTCMP2 */
> +	PINCTRL_PIN(8,  "PB8"),  /* B8_UART_2_TX */
> +	PINCTRL_PIN(9,  "PB9"),  /* B9_UART_2_RX */
> +	PINCTRL_PIN(10, "PB10"), /* B10_CAN_2_TX */
> +	PINCTRL_PIN(11, "PB11"), /* B11_CAN_2_RX */
> +	PINCTRL_PIN(12, "PB12"), /* B12_SPI_2_DO */
> +	PINCTRL_PIN(13, "PB13"), /* B13_SPI_2_DI */
> +	PINCTRL_PIN(14, "PB14"), /* B14_SPI_2_CK */
> +	PINCTRL_PIN(15, "PB15"), /* B15_SPI_2_CS0 */
> +	PINCTRL_PIN(16, "PB16"), /* B16_SPI_2_CS1 */
> +	PINCTRL_PIN(17, "PB17"), /* B17_SPI_3_DO */
> +	PINCTRL_PIN(18, "PB18"), /* B18_SPI_3_DI */
> +	PINCTRL_PIN(19, "PB19"), /* B19_SPI_3_CK */
> +	PINCTRL_PIN(20, "PB20"), /* B20_SPI_3_CS0 */
> +	PINCTRL_PIN(21, "PB21"), /* B21_SPI_3_CS1 */
> +	PINCTRL_PIN(22, "PB22"), /* B22_MCLK0 */
> +};
> +
> +/* Groups of functions on bank A */
> +static const char * const gpioa_groups[] = {
> +	"PA0", "PA1", "PA2", "PA3", "PA4", "PA5", "PA6", "PA7", "PA8", "PA9",
> +	"PA10", "PA11", "PA12", "PA13", "PA14", "PA15", "PA16", "PA17", "PA18",
> +	"PA19", "PA20", "PA21", "PA22", "PA23", "PA24", "PA25", "PA26", "PA27",
> +	"PA28",
> +};
> +static const char * const timer0_groups[] = { "PA0", "PA1" };
> +static const char * const timer1_groups[] = { "PA2", "PA3" };
> +static const char * const timer2_groups[] = { "PA4", "PA5" };
> +static const char * const timer5_groups[] = { "PA6", "PA7", "PA8", "PA9" };
> +static const char * const uart0_groups[] = { "PA10", "PA11" };
> +static const char * const uart1_groups[] = { "PA12", "PA13" };
> +static const char * const can0_groups[] = { "PA14", "PA15" };
> +static const char * const can1_groups[] = { "PA16", "PA17" };
> +static const char * const spi0_groups[] = { "PA18", "PA19", "PA20", "PA21", "PA22" };
> +static const char * const spi1_groups[] = { "PA23", "PA24", "PA25", "PA26", "PA27" };
> +static const char * const refclk0_groups[] = { "PA28" };
> +
> +/* Groups of functions on bank B */
> +static const char * const gpiob_groups[] = {
> +	"PB0", "PB1", "PB2", "PB3", "PB4", "PB5", "PB6", "PB7", "PB8", "PB9",
> +	"PB10", "PB11", "PB12", "PB13", "PB14", "PB15", "PB16", "PB17", "PB18",
> +	"PB19", "PB20", "PB21", "PB22",
> +};
> +static const char * const timer3_groups[] = { "PB0", "PB1" };
> +static const char * const timer4_groups[] = { "PB2", "PB3" };
> +static const char * const timer6_groups[] = { "PB4", "PB5", "PB6", "PB7" };
> +static const char * const uart2_groups[] = { "PB8", "PB9" };
> +static const char * const can2_groups[] = { "PB10", "PB11" };
> +static const char * const spi2_groups[] = { "PB12", "PB13", "PB14", "PB15", "PB16" };
> +static const char * const spi3_groups[] = { "PB17", "PB18", "PB19", "PB20", "PB21" };
> +static const char * const mclk0_groups[] = { "PB22" };
> +
> +#define FUNCTION(a, b) { .name = a, .groups = b, .ngroups = ARRAY_SIZE(b) }
> +
> +static const struct eq5p_function eq5p_functions_a[] = {
> +	/* GPIO having a fixed index is depended upon, see GPIO_FUNC_SELECTOR. */
> +	FUNCTION("gpio", gpioa_groups),
> +
> +	FUNCTION("timer0", timer0_groups),
> +	FUNCTION("timer1", timer1_groups),
> +	FUNCTION("timer2", timer2_groups),
> +	FUNCTION("timer5", timer5_groups),
> +	FUNCTION("uart0", uart0_groups),
> +	FUNCTION("uart1", uart1_groups),
> +	FUNCTION("can0", can0_groups),
> +	FUNCTION("can1", can1_groups),
> +	FUNCTION("spi0", spi0_groups),
> +	FUNCTION("spi1", spi1_groups),
> +	FUNCTION("refclk0", refclk0_groups),
> +};
> +
> +static const struct eq5p_function eq5p_functions_b[] = {
> +	/* GPIO having a fixed index is depended upon, see GPIO_FUNC_SELECTOR. */
> +	FUNCTION("gpio", gpiob_groups),
> +
> +	FUNCTION("timer3", timer3_groups),
> +	FUNCTION("timer4", timer4_groups),
> +	FUNCTION("timer6", timer6_groups),
> +	FUNCTION("uart2", uart2_groups),
> +	FUNCTION("can2", can2_groups),
> +	FUNCTION("spi2", spi2_groups),
> +	FUNCTION("spi3", spi3_groups),
> +	FUNCTION("mclk0", mclk0_groups),
> +};
> +
> +struct eq5p_match {
> +	unsigned int regs[EQ5P_REG_MAX];
> +	const struct pinctrl_pin_desc *pins;
> +	unsigned int npins;
> +	const struct eq5p_function *funcs;
> +	unsigned int nfuncs;
> +};
> +
> +static int eq5p_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct device_node *parent_np = of_get_parent(np);
> +	const struct eq5p_match *match = of_device_get_match_data(dev);
> +	struct pinctrl_dev *pctldev;
> +	struct eq5p_pinctrl *pctrl;
> +	int ret;
> +
> +	pctrl = devm_kzalloc(dev, sizeof(*pctrl), GFP_KERNEL);
> +	if (!pctrl)
> +		return -ENOMEM;
> +
> +	pctrl->olb = ERR_PTR(-ENODEV);
> +	if (parent_np)
> +		pctrl->olb = syscon_node_to_regmap(parent_np);
> +	if (IS_ERR(pctrl->olb))
> +		pctrl->olb = syscon_regmap_lookup_by_phandle(np, "mobileye,olb");
> +	if (IS_ERR(pctrl->olb))
> +		return PTR_ERR(pctrl->olb);
> +
> +	pctrl->regs = match->regs;
> +	pctrl->funcs = match->funcs;
> +	pctrl->nfuncs = match->nfuncs;
> +
> +	pctrl->desc.name = dev_name(dev);
> +	pctrl->desc.pins = match->pins;
> +	pctrl->desc.npins = match->npins;
> +	pctrl->desc.pctlops = &eq5p_pinctrl_ops;
> +	pctrl->desc.pmxops = &eq5p_pinmux_ops;
> +	pctrl->desc.confops = &eq5p_pinconf_ops;
> +	pctrl->desc.owner = THIS_MODULE;
> +
> +	ret = devm_pinctrl_register_and_init(dev, &pctrl->desc, pctrl, &pctldev);
> +	if (ret) {
> +		dev_err(dev, "Failed registering pinctrl device: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = pinctrl_enable(pctldev);
> +	if (ret) {
> +		dev_err(dev, "Failed enabling pinctrl device: %d\n", ret);
> +		return ret;
> +	}
> +
> +	dev_info(dev, "probed\n");
> +
> +	return 0;
> +}
> +
> +static const struct eq5p_match eq5p_match_a = {
> +	.regs = {
> +		[EQ5P_PD] = 0x0C0,
> +		[EQ5P_PU] = 0x0C4,
> +		[EQ5P_DS_LOW] = 0x0D0,
> +		[EQ5P_DS_HIGH] = 0x0D4,
> +		[EQ5P_IOCR] = 0x0B0,
> +	},
> +	.pins = eq5p_pins_a,
> +	.npins = ARRAY_SIZE(eq5p_pins_a),
> +	.funcs = eq5p_functions_a,
> +	.nfuncs = ARRAY_SIZE(eq5p_functions_a),
> +};
> +
> +static const struct eq5p_match eq5p_match_b = {
> +	.regs = {
> +		[EQ5P_PD] = 0x0C8,
> +		[EQ5P_PU] = 0x0CC,
> +		[EQ5P_DS_LOW] = 0x0D8,
> +		[EQ5P_DS_HIGH] = 0x0DC,
> +		[EQ5P_IOCR] = 0x0B4,
> +	},

These are all the same relative offsets, so you really only need to 
store the base offset.

The use of 2 compatibles is a bit questionable as the programming model 
appears to be the same and only which pins differ. Surely there are 
some other pinctrl drivers handling mutiple instances.

Rob

Re: [PATCH v3 08/17] clk: eyeq5: add platform driver

[Théo Lebrun]

Hello,

On Wed Jan 24, 2024 at 8:05 AM CET, Krzysztof Kozlowski wrote:
> On 23/01/2024 19:46, Théo Lebrun wrote:
> > Add the Mobileye EyeQ5 clock controller driver. It might grow to add
> > support for other platforms from Mobileye.
> > 
> > It handles 10 read-only PLLs derived from the main crystal on board. It
> > exposes a table-based divider clock used for OSPI. Other platform
> > clocks are not configurable and therefore kept as fixed-factor
> > devicetree nodes.
> > 
> > Two PLLs are required early on and are therefore registered at
> > of_clk_init(). Those are pll-cpu for the GIC timer and pll-per for the
> > UARTs.
> > 
>
>
> > +#define OLB_PCSR1_RESET				BIT(0)
> > +#define OLB_PCSR1_SSGC_DIV			GENMASK(4, 1)
> > +/* Spread amplitude (% = 0.1 * SPREAD[4:0]) */
> > +#define OLB_PCSR1_SPREAD			GENMASK(9, 5)
> > +#define OLB_PCSR1_DIS_SSCG			BIT(10)
> > +/* Down-spread or center-spread */
> > +#define OLB_PCSR1_DOWN_SPREAD			BIT(11)
> > +#define OLB_PCSR1_FRAC_IN			GENMASK(31, 12)
> > +
> > +static struct clk_hw_onecell_data *eq5c_clk_data;
> > +static struct regmap *eq5c_olb;
>
> Drop these two. No file-scope regmaps for drivers. Use private container
> structures.

I wouldn't know how to handle the two steps then. Two clocks and the clk
provider are registered at of_clk_init() using CLK_OF_DECLARE_DRIVER().
The rest is at platform device probe. Without a static, there are no
way to pass the struct clk_hw_onecell_data from one to the other.

I've looked at all clock drivers that do CLK_OF_DECLARE_DRIVER() and
register a platform driver.

 - The following use a static variable:
   drivers/clk/axis/clk-artpec6.c
   drivers/clk/clk-aspeed.c
   drivers/clk/clk-ast2600.c
   drivers/clk/clk-eyeq5.c
   drivers/clk/clk-gemini.c
   drivers/clk/clk-milbeaut.c
   drivers/clk/mediatek/clk-mt2701.c
   drivers/clk/mediatek/clk-mt6797.c
   drivers/clk/mediatek/clk-mt8173-infracfg.c
   drivers/clk/nxp/clk-lpc18xx-creg.c
   drivers/clk/ralink/clk-mt7621.c
   drivers/clk/ralink/clk-mtmips.c
   drivers/clk/sunxi/clk-mod0.c
   drivers/clk/axis/clk-artpec6.c

 - Those two declare different clock providers at init and probe:
   drivers/clk/ralink/clk-mt7621.c
   drivers/clk/sunxi/clk-mod0.c

 - It doesn't register new clocks at probe (only resets) so no need to
   share variables.
   drivers/clk/ralink/clk-mtmips.c

>
> ...
>
> > +static void __init eq5c_init(struct device_node *np)
> > +{
> > +	struct device_node *parent_np = of_get_parent(np);
> > +	int i, ret;
> > +
> > +	eq5c_clk_data = kzalloc(struct_size(eq5c_clk_data, hws, EQ5C_NB_CLKS),
> > +				GFP_KERNEL);
> > +	if (!eq5c_clk_data) {
> > +		ret = -ENOMEM;
> > +		goto err;
> > +	}
> > +
> > +	eq5c_clk_data->num = EQ5C_NB_CLKS;
> > +
> > +	/*
> > +	 * Mark all clocks as deferred. We register some now and others at
> > +	 * platform device probe.
> > +	 */
> > +	for (i = 0; i < EQ5C_NB_CLKS; i++)
> > +		eq5c_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
> > +
> > +	/*
> > +	 * Currently, if OLB is not available, we log an error, fail init then
>
> How it could be not available? Only with broken initcall ordering. Fix
> your initcall ordering and then simplify all this weird code.

of_syscon_register() and regmap_init_mmio() lists many reasons for
it to not be available. Am I missing something?

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 00/17] Add support for Mobileye EyeQ5 system controller

[Théo Lebrun]

Hello,

On Wed Jan 24, 2024 at 7:43 AM CET, Krzysztof Kozlowski wrote:
> You miss here, in this place, the most important information which I
> asked previously - dependencies/merging:
> https://lore.kernel.org/all/db9b2786-29f7-4d45-9087-a9f85b770b6c@linaro.org/

Clearly I still forgot some dt-bindings dependencies. I now get what you
meant at the time. Those explain the robot failures btw.

Should it just be me saying "hey patches X depends on patches A, B, C?"
Or are there even Git message trailers to mention dependencies? Care to
give a patch link that does it the right way?

Sorry about that,

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 09/17] reset: eyeq5: add platform driver

[Théo Lebrun]

Hello,

On Wed Jan 24, 2024 at 8:00 AM CET, Krzysztof Kozlowski wrote:
> On 23/01/2024 19:46, Théo Lebrun wrote:
> > Add the Mobileye EyeQ5 reset controller driver. It belongs to a syscon
> > region called OLB. It might grow to add later support of other
> > platforms from Mobileye.

[...]

> > +static int _eq5r_busy_wait(struct eq5r_private *priv, struct device *dev,
> > +			   u32 domain, u32 offset, bool assert)
> > +{
> > +	unsigned int val, mask;
> > +	int i;
> > +
> > +	lockdep_assert_held(&priv->mutexes[domain]);
> > +
> > +	switch (domain) {
> > +	case 0:
> > +		for (i = 0; i < D0_TIMEOUT_POLL; i++) {
> > +			regmap_read(priv->olb, EQ5R_OLB_SARCR1, &val);
> > +			val = !(val & BIT(offset));
> > +			if (val == assert)
> > +				return 0;
> > +			__udelay(1);
>
> What is even "__udelay"? It is the first use in drivers. Please use
> common methods, like fsleep or udelay... but actually you should rather
> use regmap_read_poll_timeout() or some variants instead of open-coding it.

udelay is an alias to __udelay on MIPS, which is why this didn't look
odd to me. Fixed.

[...]

> > +static void _eq5r_assert(struct eq5r_private *priv, u32 domain, u32 offset)
>
> Drop leading _ and name the function in some informative way.

Fixed by turning `_eq5r_assert` into `eq5r_assert_withlock`, and co.

[...]

> > +
> > +static int eq5r_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *np = dev->of_node;
> > +	struct device_node *parent_np = of_get_parent(np);
> > +	struct eq5r_private *priv;
> > +	int ret, i;
> > +
> > +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
>
> You leak parent.

Fixed in all three clk+reset+pinctrl drivers. They all had this issue.

>
> > +
> > +	dev_set_drvdata(dev, priv);
> > +
> > +	priv->olb = ERR_PTR(-ENODEV);
> > +	if (parent_np) {
> > +		priv->olb = syscon_node_to_regmap(parent_np);
> > +		of_node_put(parent_np);
> > +	}
> > +	if (IS_ERR(priv->olb))
>
> Also here
>
> > +		return PTR_ERR(priv->olb);
>
> This looks over-complicated. First, you cannot just
> dev_get_regmap(pdev->dev.parent)?

No dev_get_regmap() cannot be used as it doesn't pick up syscon regmaps.
I've just tried it.

However I've simplified the logic, it looks better now.

	static int eq5r_probe(struct platform_device *pdev)
	{
		struct device_node *parent_np;

		/* ... */

		parent_np = of_get_parent(np);
		if (!parent_np)
			return -ENODEV;

		priv->olb = syscon_node_to_regmap(parent_np);
		of_node_put(parent_np);
		if (IS_ERR(priv->olb))
			return PTR_ERR(priv->olb);

		/* ... */
	}

[...]

> > +static struct platform_driver eq5r_driver = {
> > +	.probe = eq5r_probe,
> > +	.driver = {
> > +		.name = "eyeq5-reset",
> > +		.of_match_table = eq5r_match_table,
> > +	},
> > +};
> > +
> > +static int __init eq5r_init(void)
> > +{
> > +	return platform_driver_register(&eq5r_driver);
> > +}
> > +
> > +arch_initcall(eq5r_init);
>
> This is does not look like arch code, but driver or subsys. Use regular
> module_driver. I see there is such pattern in reset but I doubt this is
> something good.

Indeed I've moved things to using the builtin_platform_driver() macro.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 10/17] pinctrl: eyeq5: add platform driver

[Théo Lebrun]

Hello,

On Wed Jan 24, 2024 at 8:03 AM CET, Krzysztof Kozlowski wrote:
> On 23/01/2024 19:46, Théo Lebrun wrote:
> > Add the Mobileye EyeQ5 pin controller driver. It might grow to add later
> > support of other platforms from Mobileye. It belongs to a syscon region
> > called OLB.
> > 
> > Existing pins and their function live statically in the driver code
> > rather than in the devicetree, see compatible match data.
> > 
>
> ...
>
> > +static int eq5p_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *np = dev->of_node;
> > +	struct device_node *parent_np = of_get_parent(np);
> > +	const struct eq5p_match *match = of_device_get_match_data(dev);
> > +	struct pinctrl_dev *pctldev;
> > +	struct eq5p_pinctrl *pctrl;
> > +	int ret;
> > +
> > +	pctrl = devm_kzalloc(dev, sizeof(*pctrl), GFP_KERNEL);
> > +	if (!pctrl)
> > +		return -ENOMEM;
> > +
> > +	pctrl->olb = ERR_PTR(-ENODEV);
> > +	if (parent_np)
> > +		pctrl->olb = syscon_node_to_regmap(parent_np);
> > +	if (IS_ERR(pctrl->olb))
> > +		pctrl->olb = syscon_regmap_lookup_by_phandle(np, "mobileye,olb");
> > +	if (IS_ERR(pctrl->olb))
> > +		return PTR_ERR(pctrl->olb);
>
> No, we talked about this, you got comments on this. There is no
> mobileye,olb. You cannot have undocumented properties.

Clearly and I fully agree. It's a versioning issue on my side. It's
been fixed (again, oops).

>
> > +
> > +	pctrl->regs = match->regs;
> > +	pctrl->funcs = match->funcs;
> > +	pctrl->nfuncs = match->nfuncs;
> > +
> > +	pctrl->desc.name = dev_name(dev);
> > +	pctrl->desc.pins = match->pins;
> > +	pctrl->desc.npins = match->npins;
> > +	pctrl->desc.pctlops = &eq5p_pinctrl_ops;
> > +	pctrl->desc.pmxops = &eq5p_pinmux_ops;
> > +	pctrl->desc.confops = &eq5p_pinconf_ops;
> > +	pctrl->desc.owner = THIS_MODULE;
> > +
> > +	ret = devm_pinctrl_register_and_init(dev, &pctrl->desc, pctrl, &pctldev);
> > +	if (ret) {
> > +		dev_err(dev, "Failed registering pinctrl device: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = pinctrl_enable(pctldev);
> > +	if (ret) {
> > +		dev_err(dev, "Failed enabling pinctrl device: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	dev_info(dev, "probed\n");
>
> I am pretty sure you got comments for these. Drop such debugs from all
> of your code. Current and future.

Same thing, it must have been lost in the same fixup patch as the
previous mistake.

[...]

> > +static struct platform_driver eq5p_driver = {
> > +	.driver = {
> > +		.name = "eyeq5-pinctrl",
> > +		.of_match_table = eq5p_match,
> > +	},
> > +	.probe = eq5p_probe,
> > +};
> > +
> > +static int __init eq5p_init(void)
> > +{
> > +	return platform_driver_register(&eq5p_driver);
> > +}
> > +core_initcall(eq5p_init);
>
> No, pins are not a core_initcall. This could be arch_initcall, but
> considering you depend on the parent this must be module driver.
>
> Even from this dependency point of view your initcalls are totally wrong
> and will lead to issues.

Same as reset: moved to using the builtin_platform_driver() macro. No
need for it to be module as it cannot be one.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 12/17] MIPS: mobileye: eyeq5: remove reg-io-width property from OLB syscon

[Théo Lebrun]

Hello,

On Wed Jan 24, 2024 at 9:33 AM CET, Sergei Shtylyov wrote:
> On 1/23/24 9:46 PM, Théo Lebrun wrote:
>
> > Remove the `reg-io-width` property from the olb@e00000 syscon. The
>
>    The diff says it's system-controller@e00000 now... :-)

Oops, nice catch!

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 09/17] reset: eyeq5: add platform driver

[Théo Lebrun]

Hi,

On Wed Jan 24, 2024 at 11:54 AM CET, Philipp Zabel wrote:
> On Di, 2024-01-23 at 19:46 +0100, Théo Lebrun wrote:
> [...]
> > diff --git a/drivers/reset/reset-eyeq5.c b/drivers/reset/reset-eyeq5.c
> > new file mode 100644
> > index 000000000000..2217e42e140b
> > --- /dev/null
> > +++ b/drivers/reset/reset-eyeq5.c
> > @@ -0,0 +1,383 @@
> [...]
>
> > +static int eq5r_assert(struct reset_controller_dev *rcdev, unsigned long id)
> > +{
> > +	struct eq5r_private *priv = dev_get_drvdata(rcdev->dev);
>
> rcdev is contained in priv, you can just use container_of instead of
> chasing pointers around.

That's right. Fixed with this local macro:

#define rcdev_to_priv(rcdev) container_of(rcdev, struct eq5r_private, rcdev)

> > +	u32 offset = id & GENMASK(7, 0);
> > +	u32 domain = id >> 8;
> > +	int ret;
> > +
> > +	if (WARN_ON(domain >= EQ5R_DOMAIN_COUNT))
> > +		return -EINVAL;
>
> Reset controls with domain >= EQ5R_DOMAIN_COUNT are already weeded out
> during request by of_xlate, so this check is not necessary.

It was some defensive programming. I've removed this precautionary
condition from the places it appeared.

>
> > +	dev_dbg(rcdev->dev, "%u-%u: assert request\n", domain, offset);
> > +
> > +	mutex_lock(&priv->mutexes[domain]);
> > +	_eq5r_assert(priv, domain, offset);
> > +	ret = _eq5r_busy_wait(priv, rcdev->dev, domain, offset, true);
> > +	mutex_unlock(&priv->mutexes[domain]);
> > +
> > +	return ret;
>
> Consider using guard(mutex)(&priv->mutexes[domain]) from
> linux/cleanup.h to automatically unlock on return.

Done. I had never used that __cleanup attr feature. It simplifies
returns.

>
> [...]
> > +static int eq5r_reset(struct reset_controller_dev *rcdev, unsigned long id)
>
> Is this used by anything? If unused, I'd prefer this not to be
> implemented. If it is used, is no delay required between assert and
> deassert by any consumer?

Not really, it follows what is done in the downstream vendor kernel.
I've had a quick look in this kernel and I don't see any consumer of
the API. For the moment I'll remove it.

>
> > +{
> > +	struct device *dev = rcdev->dev;
> > +	struct eq5r_private *priv = dev_get_drvdata(dev);
> > +	u32 offset = id & GENMASK(7, 0);
> > +	u32 domain = id >> 8;
> > +	int ret;
> > +
> > +	if (WARN_ON(domain >= EQ5R_DOMAIN_COUNT))
> > +		return -EINVAL;
> > +
> > +	dev_dbg(dev, "%u-%u: reset request\n", domain, offset);
> > +
> > +	mutex_lock(&priv->mutexes[domain]);
> > +
> > +	_eq5r_assert(priv, domain, offset);
> > +	ret = _eq5r_busy_wait(priv, dev, domain, offset, true);
> > +	if (ret) /* don't let an error disappear silently */
> > +		dev_warn(dev, "%u-%u: reset assert failed: %d\n",
> > +			 domain, offset, ret);
>
> Why not return the error though?

The goal was to still run through the deassert even if the assert
returned an error. Goal was to address potential edge case of assert
returning an error but still taking place, in which case we want to try
to deassert to put the peripheral in a de-asserted state (as before the
call).

Not a concern anymore as the function is being removed.

>
> > +	_eq5r_deassert(priv, domain, offset);
> > +	ret = _eq5r_busy_wait(priv, dev, domain, offset, false);
> > +
> > +	mutex_unlock(&priv->mutexes[domain]);
> > +
> > +	return ret;
> > +}
> [...]
> > +static int eq5r_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *np = dev->of_node;
> > +	struct device_node *parent_np = of_get_parent(np);
> > +	struct eq5r_private *priv;
> > +	int ret, i;
> > +
> > +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>
> Using devm_kzalloc() avoids leaking this on error return or driver
> unbind.

Done, thanks.

>
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	dev_set_drvdata(dev, priv);
> > +
> > +	priv->olb = ERR_PTR(-ENODEV);
> > +	if (parent_np) {
> > +		priv->olb = syscon_node_to_regmap(parent_np);
> > +		of_node_put(parent_np);
> > +	}
> > +	if (IS_ERR(priv->olb))
> > +		return PTR_ERR(priv->olb);
> > +
> > +	for (i = 0; i < EQ5R_DOMAIN_COUNT; i++)
> > +		mutex_init(&priv->mutexes[i]);
> > +
> > +	priv->rcdev.ops = &eq5r_ops;
> > +	priv->rcdev.owner = THIS_MODULE;
> > +	priv->rcdev.dev = dev;
> > +	priv->rcdev.of_node = np;
> > +	priv->rcdev.of_reset_n_cells = 2;
> > +	priv->rcdev.of_xlate = eq5r_of_xlate;
> > +
> > +	priv->rcdev.nr_resets = 0;
> > +	for (i = 0; i < EQ5R_DOMAIN_COUNT; i++)
> > +		priv->rcdev.nr_resets += __builtin_popcount(eq5r_valid_masks[i]);
> > +
> > +	ret = reset_controller_register(&priv->rcdev);
>
> Similarly, use devm_reset_controller_register() or disable driver
> unbind with suppress_bind_attrs.

Switched to the devres version, thanks.


Thanks Philipp,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 04/17] dt-bindings: soc: mobileye: add EyeQ5 OLB system controller

[Théo Lebrun]

Hello,

On Wed Jan 24, 2024 at 4:14 PM CET, Rob Herring wrote:
> On Tue, Jan 23, 2024 at 07:46:49PM +0100, Théo Lebrun wrote:
> > Add documentation to describe the "Other Logic Block" syscon.
> > 
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > ---
> >  .../bindings/soc/mobileye/mobileye,eyeq5-olb.yaml  | 77 ++++++++++++++++++++++
> >  MAINTAINERS                                        |  1 +
> >  2 files changed, 78 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml
> > new file mode 100644
> > index 000000000000..031ef6a532c1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml
> > @@ -0,0 +1,77 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/soc/mobileye/mobileye,eyeq5-olb.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Mobileye EyeQ5 SoC system controller
> > +
> > +maintainers:
> > +  - Grégory Clement <gregory.clement@bootlin.com>
> > +  - Théo Lebrun <theo.lebrun@bootlin.com>
> > +  - Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>
> > +
> > +description:
> > +  OLB ("Other Logic Block") is a hardware block grouping smaller blocks. Clocks,
> > +  resets, pinctrl are being handled from here.
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: mobileye,eyeq5-olb
> > +      - const: syscon
> > +      - const: simple-mfd
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clock-controller:
> > +    $ref: /schemas/clock/mobileye,eyeq5-clk.yaml#
> > +    type: object
> > +
> > +  reset-controller:
> > +    $ref: /schemas/reset/mobileye,eyeq5-reset.yaml#
> > +    type: object
> > +
> > +  pinctrl-a:
> > +    $ref: /schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml#
> > +    type: object
> > +
> > +  pinctrl-b:
> > +    $ref: /schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml#
> > +    type: object
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    system-controller@e00000 {
> > +      compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd";
> > +      reg = <0xe00000 0x400>;
> > +
> > +      clock-controller {
> > +        compatible = "mobileye,eyeq5-clk";
> > +        #clock-cells = <1>;
> > +        clocks = <&xtal>;
> > +        clock-names = "ref";
> > +      };
> > +
> > +      reset-controller {
> > +        compatible = "mobileye,eyeq5-reset";
> > +        #reset-cells = <2>;
> > +      };
> > +
> > +      pinctrl-a {
> > +        compatible = "mobileye,eyeq5-a-pinctrl";
> > +        #pinctrl-cells = <1>;
>
> Sure you need this? Generally only pinctrl-single uses this.

You are completely right, it is useless. I naively expected it in the
same vein as other subsystems.

>
> > +      };
> > +
> > +      pinctrl-b {
> > +        compatible = "mobileye,eyeq5-b-pinctrl";
> > +        #pinctrl-cells = <1>;
> > +      };
> > +    };
>
> This can all be simplified to:
>
> system-controller@e00000 {
>     compatible = "mobileye,eyeq5-olb", "syscon";
>     reg = <0xe00000 0x400>;
>     #reset-cells = <2>;
>     #clock-cells = <1>;
>     clocks = <&xtal>;
>     clock-names = "ref";
>
>     pins { ... };
> };
>
> There is no need for sub nodes unless you have reusable blocks or each 
> block has its own resources in DT.

That is right, and it does simplify the devicetree as you have shown.
However, the split nodes gives the following advantages:

 - Devicetree-wise, it allows for one alias per function.
   `clocks = <&clocks EQ5C_PLL_CPU>` is surely more intuitive
   than `clocks = <&olb EQ5C_PLL_CPU>;`. Same for reset.

 - It means an MFD driver must be implemented, adding between 100 to 200
   lines of boilerplate code to the kernel.

 - It means one pinctrl device for the two banks. That addresses your
   comment on [PATCH v3 10/17]. This is often done and would be doable
   on this platform. However it means added logic to each individual
   function of pinctrl-eyeq5.

   Overall it makes for less readable code, for code that already looks
   more complex than it really is.

   My initial non-public version of pinctrl-eyeq5 was using this method
   (a device handling both banks) and I've leaned away from it.

Those are all minor, but I don't have the feeling a few lines and nodes
less in devicetree compensate for those.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 10/17] pinctrl: eyeq5: add platform driver

[Théo Lebrun]

Hello,

On Wed Jan 24, 2024 at 4:19 PM CET, Rob Herring wrote:
> On Tue, Jan 23, 2024 at 07:46:55PM +0100, Théo Lebrun wrote:
> > Add the Mobileye EyeQ5 pin controller driver. It might grow to add later
> > support of other platforms from Mobileye. It belongs to a syscon region
> > called OLB.
> > 
> > Existing pins and their function live statically in the driver code
> > rather than in the devicetree, see compatible match data.
> >
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > ---

[...]

> > diff --git a/drivers/pinctrl/pinctrl-eyeq5.c b/drivers/pinctrl/pinctrl-eyeq5.c

[...]

> > +static const struct eq5p_match eq5p_match_a = {
> > +	.regs = {
> > +		[EQ5P_PD] = 0x0C0,
> > +		[EQ5P_PU] = 0x0C4,
> > +		[EQ5P_DS_LOW] = 0x0D0,
> > +		[EQ5P_DS_HIGH] = 0x0D4,
> > +		[EQ5P_IOCR] = 0x0B0,
> > +	},
> > +	.pins = eq5p_pins_a,
> > +	.npins = ARRAY_SIZE(eq5p_pins_a),
> > +	.funcs = eq5p_functions_a,
> > +	.nfuncs = ARRAY_SIZE(eq5p_functions_a),
> > +};
> > +
> > +static const struct eq5p_match eq5p_match_b = {
> > +	.regs = {
> > +		[EQ5P_PD] = 0x0C8,
> > +		[EQ5P_PU] = 0x0CC,
> > +		[EQ5P_DS_LOW] = 0x0D8,
> > +		[EQ5P_DS_HIGH] = 0x0DC,
> > +		[EQ5P_IOCR] = 0x0B4,
> > +	},
>
> These are all the same relative offsets, so you really only need to 
> store the base offset.

Indeed, and I don't think I had even noticed. Thanks.

> The use of 2 compatibles is a bit questionable as the programming model 
> appears to be the same and only which pins differ. Surely there are 
> some other pinctrl drivers handling mutiple instances.

I can confirm the programming model is the same across both banks. I've
addressed your comment in my answer to yours on [PATCH v3 04/17].


Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 04/17] dt-bindings: soc: mobileye: add EyeQ5 OLB system controller

[Théo Lebrun]

Hello,

On Wed Jan 24, 2024 at 6:28 PM CET, Théo Lebrun wrote:
> Hello,
>
> On Wed Jan 24, 2024 at 4:14 PM CET, Rob Herring wrote:
> > On Tue, Jan 23, 2024 at 07:46:49PM +0100, Théo Lebrun wrote:
> > > Add documentation to describe the "Other Logic Block" syscon.
> > > 
> > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > > ---
> > >  .../bindings/soc/mobileye/mobileye,eyeq5-olb.yaml  | 77 ++++++++++++++++++++++
> > >  MAINTAINERS                                        |  1 +
> > >  2 files changed, 78 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml
> > > new file mode 100644
> > > index 000000000000..031ef6a532c1
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml
> > > @@ -0,0 +1,77 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/soc/mobileye/mobileye,eyeq5-olb.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Mobileye EyeQ5 SoC system controller
> > > +
> > > +maintainers:
> > > +  - Grégory Clement <gregory.clement@bootlin.com>
> > > +  - Théo Lebrun <theo.lebrun@bootlin.com>
> > > +  - Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>
> > > +
> > > +description:
> > > +  OLB ("Other Logic Block") is a hardware block grouping smaller blocks. Clocks,
> > > +  resets, pinctrl are being handled from here.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    items:
> > > +      - const: mobileye,eyeq5-olb
> > > +      - const: syscon
> > > +      - const: simple-mfd
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  clock-controller:
> > > +    $ref: /schemas/clock/mobileye,eyeq5-clk.yaml#
> > > +    type: object
> > > +
> > > +  reset-controller:
> > > +    $ref: /schemas/reset/mobileye,eyeq5-reset.yaml#
> > > +    type: object
> > > +
> > > +  pinctrl-a:
> > > +    $ref: /schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml#
> > > +    type: object
> > > +
> > > +  pinctrl-b:
> > > +    $ref: /schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml#
> > > +    type: object
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    system-controller@e00000 {
> > > +      compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd";
> > > +      reg = <0xe00000 0x400>;
> > > +
> > > +      clock-controller {
> > > +        compatible = "mobileye,eyeq5-clk";
> > > +        #clock-cells = <1>;
> > > +        clocks = <&xtal>;
> > > +        clock-names = "ref";
> > > +      };
> > > +
> > > +      reset-controller {
> > > +        compatible = "mobileye,eyeq5-reset";
> > > +        #reset-cells = <2>;
> > > +      };
> > > +
> > > +      pinctrl-a {
> > > +        compatible = "mobileye,eyeq5-a-pinctrl";
> > > +        #pinctrl-cells = <1>;
> >
> > Sure you need this? Generally only pinctrl-single uses this.
>
> You are completely right, it is useless. I naively expected it in the
> same vein as other subsystems.
>
> >
> > > +      };
> > > +
> > > +      pinctrl-b {
> > > +        compatible = "mobileye,eyeq5-b-pinctrl";
> > > +        #pinctrl-cells = <1>;
> > > +      };
> > > +    };
> >
> > This can all be simplified to:
> >
> > system-controller@e00000 {
> >     compatible = "mobileye,eyeq5-olb", "syscon";
> >     reg = <0xe00000 0x400>;
> >     #reset-cells = <2>;
> >     #clock-cells = <1>;
> >     clocks = <&xtal>;
> >     clock-names = "ref";
> >
> >     pins { ... };
> > };
> >
> > There is no need for sub nodes unless you have reusable blocks or each 
> > block has its own resources in DT.
>
> That is right, and it does simplify the devicetree as you have shown.
> However, the split nodes gives the following advantages:
>
>  - Devicetree-wise, it allows for one alias per function.
>    `clocks = <&clocks EQ5C_PLL_CPU>` is surely more intuitive
>    than `clocks = <&olb EQ5C_PLL_CPU>;`. Same for reset.
>
>  - It means an MFD driver must be implemented, adding between 100 to 200
>    lines of boilerplate code to the kernel.
>
>  - It means one pinctrl device for the two banks. That addresses your
>    comment on [PATCH v3 10/17]. This is often done and would be doable
>    on this platform. However it means added logic to each individual
>    function of pinctrl-eyeq5.
>
>    Overall it makes for less readable code, for code that already looks
>    more complex than it really is.
>
>    My initial non-public version of pinctrl-eyeq5 was using this method
>    (a device handling both banks) and I've leaned away from it.

I had forgotten one other reason:

 - Reusability does count for something. Other Mobileye platforms exist,
   and the system controller stuff is more complex on those. Multiple
   different OLB blocks, etc. But my understanding is that
   per-peripheral logic is reused across versions.

>
> Those are all minor, but I don't have the feeling a few lines and nodes
> less in devicetree compensate for those.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 04/17] dt-bindings: soc: mobileye: add EyeQ5 OLB system controller

[Rob Herring]

On Wed, Jan 24, 2024 at 11:40 AM Théo Lebrun <theo.lebrun@bootlin.com> wrote:
>
> Hello,
>
> On Wed Jan 24, 2024 at 6:28 PM CET, Théo Lebrun wrote:
> > Hello,
> >
> > On Wed Jan 24, 2024 at 4:14 PM CET, Rob Herring wrote:
> > > On Tue, Jan 23, 2024 at 07:46:49PM +0100, Théo Lebrun wrote:
> > > > Add documentation to describe the "Other Logic Block" syscon.
> > > >
> > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > > > ---
> > > >  .../bindings/soc/mobileye/mobileye,eyeq5-olb.yaml  | 77 ++++++++++++++++++++++
> > > >  MAINTAINERS                                        |  1 +
> > > >  2 files changed, 78 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml
> > > > new file mode 100644
> > > > index 000000000000..031ef6a532c1
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml
> > > > @@ -0,0 +1,77 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/soc/mobileye/mobileye,eyeq5-olb.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Mobileye EyeQ5 SoC system controller
> > > > +
> > > > +maintainers:
> > > > +  - Grégory Clement <gregory.clement@bootlin.com>
> > > > +  - Théo Lebrun <theo.lebrun@bootlin.com>
> > > > +  - Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>
> > > > +
> > > > +description:
> > > > +  OLB ("Other Logic Block") is a hardware block grouping smaller blocks. Clocks,
> > > > +  resets, pinctrl are being handled from here.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    items:
> > > > +      - const: mobileye,eyeq5-olb
> > > > +      - const: syscon
> > > > +      - const: simple-mfd
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  clock-controller:
> > > > +    $ref: /schemas/clock/mobileye,eyeq5-clk.yaml#
> > > > +    type: object
> > > > +
> > > > +  reset-controller:
> > > > +    $ref: /schemas/reset/mobileye,eyeq5-reset.yaml#
> > > > +    type: object
> > > > +
> > > > +  pinctrl-a:
> > > > +    $ref: /schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml#
> > > > +    type: object
> > > > +
> > > > +  pinctrl-b:
> > > > +    $ref: /schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml#
> > > > +    type: object
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    system-controller@e00000 {
> > > > +      compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd";
> > > > +      reg = <0xe00000 0x400>;
> > > > +
> > > > +      clock-controller {
> > > > +        compatible = "mobileye,eyeq5-clk";
> > > > +        #clock-cells = <1>;
> > > > +        clocks = <&xtal>;
> > > > +        clock-names = "ref";
> > > > +      };
> > > > +
> > > > +      reset-controller {
> > > > +        compatible = "mobileye,eyeq5-reset";
> > > > +        #reset-cells = <2>;
> > > > +      };
> > > > +
> > > > +      pinctrl-a {
> > > > +        compatible = "mobileye,eyeq5-a-pinctrl";
> > > > +        #pinctrl-cells = <1>;
> > >
> > > Sure you need this? Generally only pinctrl-single uses this.
> >
> > You are completely right, it is useless. I naively expected it in the
> > same vein as other subsystems.
> >
> > >
> > > > +      };
> > > > +
> > > > +      pinctrl-b {
> > > > +        compatible = "mobileye,eyeq5-b-pinctrl";
> > > > +        #pinctrl-cells = <1>;
> > > > +      };
> > > > +    };
> > >
> > > This can all be simplified to:
> > >
> > > system-controller@e00000 {
> > >     compatible = "mobileye,eyeq5-olb", "syscon";
> > >     reg = <0xe00000 0x400>;
> > >     #reset-cells = <2>;
> > >     #clock-cells = <1>;
> > >     clocks = <&xtal>;
> > >     clock-names = "ref";
> > >
> > >     pins { ... };
> > > };
> > >
> > > There is no need for sub nodes unless you have reusable blocks or each
> > > block has its own resources in DT.
> >
> > That is right, and it does simplify the devicetree as you have shown.
> > However, the split nodes gives the following advantages:
> >
> >  - Devicetree-wise, it allows for one alias per function.
> >    `clocks = <&clocks EQ5C_PLL_CPU>` is surely more intuitive
> >    than `clocks = <&olb EQ5C_PLL_CPU>;`. Same for reset.

clocks: resets: pinctrl: system-controller@e00000 {

> >
> >  - It means an MFD driver must be implemented, adding between 100 to 200
> >    lines of boilerplate code to the kernel.

From a binding perspective, not my problem... That's Linux details
defining the binding. What about u-boot, BSD, future versions of Linux
with different structure?

I don't think an MFD is required here. A driver should be able to be
both clock and reset provider. That's pretty common. pinctrl less so.

> >  - It means one pinctrl device for the two banks. That addresses your
> >    comment on [PATCH v3 10/17]. This is often done and would be doable
> >    on this platform. However it means added logic to each individual
> >    function of pinctrl-eyeq5.

If it makes things easier, 2 'pins' sub-nodes is fine. That's just
container nodes.

> >    Overall it makes for less readable code, for code that already looks
> >    more complex than it really is.
> >
> >    My initial non-public version of pinctrl-eyeq5 was using this method
> >    (a device handling both banks) and I've leaned away from it.
>
> I had forgotten one other reason:
>
>  - Reusability does count for something. Other Mobileye platforms exist,
>    and the system controller stuff is more complex on those. Multiple
>    different OLB blocks, etc. But my understanding is that
>    per-peripheral logic is reused across versions.

IME, this stuff never stays exactly the same from chip to chip.

Rob

Re: [PATCH v3 08/17] clk: eyeq5: add platform driver

[Krzysztof Kozlowski]

On 24/01/2024 17:41, Théo Lebrun wrote:
> Hello,
> 
> On Wed Jan 24, 2024 at 8:05 AM CET, Krzysztof Kozlowski wrote:
>> On 23/01/2024 19:46, Théo Lebrun wrote:
>>> Add the Mobileye EyeQ5 clock controller driver. It might grow to add
>>> support for other platforms from Mobileye.
>>>
>>> It handles 10 read-only PLLs derived from the main crystal on board. It
>>> exposes a table-based divider clock used for OSPI. Other platform
>>> clocks are not configurable and therefore kept as fixed-factor
>>> devicetree nodes.
>>>
>>> Two PLLs are required early on and are therefore registered at
>>> of_clk_init(). Those are pll-cpu for the GIC timer and pll-per for the
>>> UARTs.
>>>
>>
>>
>>> +#define OLB_PCSR1_RESET				BIT(0)
>>> +#define OLB_PCSR1_SSGC_DIV			GENMASK(4, 1)
>>> +/* Spread amplitude (% = 0.1 * SPREAD[4:0]) */
>>> +#define OLB_PCSR1_SPREAD			GENMASK(9, 5)
>>> +#define OLB_PCSR1_DIS_SSCG			BIT(10)
>>> +/* Down-spread or center-spread */
>>> +#define OLB_PCSR1_DOWN_SPREAD			BIT(11)
>>> +#define OLB_PCSR1_FRAC_IN			GENMASK(31, 12)
>>> +
>>> +static struct clk_hw_onecell_data *eq5c_clk_data;
>>> +static struct regmap *eq5c_olb;
>>
>> Drop these two. No file-scope regmaps for drivers. Use private container
>> structures.
> 
> I wouldn't know how to handle the two steps then. Two clocks and the clk
> provider are registered at of_clk_init() using CLK_OF_DECLARE_DRIVER().

Right, if some clocks have to be early, CLK_OF_DECLARE_DRIVER needs
static ones. But your commit subject says it is a platform driver and
all other pieces of this code is rather incompatible with this approach.

Do not use CLK_OF_DECLARE_DRIVER for cases where you have dependencies
because it forces you to manually order initcalls, which is exactly what
we do not want.


> The rest is at platform device probe. Without a static, there are no
> way to pass the struct clk_hw_onecell_data from one to the other.
> 
> I've looked at all clock drivers that do CLK_OF_DECLARE_DRIVER() and
> register a platform driver.

Even though the code is correct, using arguments "other did it" will not
work. You want to say that you implement legacy, poor code because you
saw legacy, poor code?

> 
>  - The following use a static variable:
>    drivers/clk/axis/clk-artpec6.c
>    drivers/clk/clk-aspeed.c
>    drivers/clk/clk-ast2600.c
>    drivers/clk/clk-eyeq5.c
>    drivers/clk/clk-gemini.c
>    drivers/clk/clk-milbeaut.c
>    drivers/clk/mediatek/clk-mt2701.c
>    drivers/clk/mediatek/clk-mt6797.c
>    drivers/clk/mediatek/clk-mt8173-infracfg.c
>    drivers/clk/nxp/clk-lpc18xx-creg.c
>    drivers/clk/ralink/clk-mt7621.c
>    drivers/clk/ralink/clk-mtmips.c
>    drivers/clk/sunxi/clk-mod0.c
>    drivers/clk/axis/clk-artpec6.c
> 
>  - Those two declare different clock providers at init and probe:
>    drivers/clk/ralink/clk-mt7621.c
>    drivers/clk/sunxi/clk-mod0.c
> 
>  - It doesn't register new clocks at probe (only resets) so no need to
>    share variables.
>    drivers/clk/ralink/clk-mtmips.c
> 
>>
>> ...
>>
>>> +static void __init eq5c_init(struct device_node *np)
>>> +{
>>> +	struct device_node *parent_np = of_get_parent(np);
>>> +	int i, ret;
>>> +
>>> +	eq5c_clk_data = kzalloc(struct_size(eq5c_clk_data, hws, EQ5C_NB_CLKS),
>>> +				GFP_KERNEL);
>>> +	if (!eq5c_clk_data) {
>>> +		ret = -ENOMEM;
>>> +		goto err;
>>> +	}
>>> +
>>> +	eq5c_clk_data->num = EQ5C_NB_CLKS;
>>> +
>>> +	/*
>>> +	 * Mark all clocks as deferred. We register some now and others at
>>> +	 * platform device probe.
>>> +	 */
>>> +	for (i = 0; i < EQ5C_NB_CLKS; i++)
>>> +		eq5c_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
>>> +
>>> +	/*
>>> +	 * Currently, if OLB is not available, we log an error, fail init then
>>
>> How it could be not available? Only with broken initcall ordering. Fix
>> your initcall ordering and then simplify all this weird code.
> 
> of_syscon_register() and regmap_init_mmio() lists many reasons for
> it to not be available. Am I missing something?

Yes, initcall ordering.

Best regards,
Krzysztof

Re: [PATCH v3 04/17] dt-bindings: soc: mobileye: add EyeQ5 OLB system controller

[Krzysztof Kozlowski]

On 24/01/2024 16:14, Rob Herring wrote:
>> +
>> +      pinctrl-b {
>> +        compatible = "mobileye,eyeq5-b-pinctrl";
>> +        #pinctrl-cells = <1>;
>> +      };
>> +    };
> 
> This can all be simplified to:
> 
> system-controller@e00000 {
>     compatible = "mobileye,eyeq5-olb", "syscon";
>     reg = <0xe00000 0x400>;
>     #reset-cells = <2>;
>     #clock-cells = <1>;
>     clocks = <&xtal>;
>     clock-names = "ref";
> 
>     pins { ... };
> };
> 
> There is no need for sub nodes unless you have reusable blocks or each 
> block has its own resources in DT.

Yes, however I believe there should be resources here: each subnode
should get its address space. This is a bit tied to implementation,
which currently assumes "everyone can fiddle with everything" in this block.

Theo, can you draw memory map?

Best regards,
Krzysztof

Re: [PATCH v3 04/17] dt-bindings: soc: mobileye: add EyeQ5 OLB system controller

[Théo Lebrun]

Hello,

On Thu Jan 25, 2024 at 8:51 AM CET, Krzysztof Kozlowski wrote:
> On 24/01/2024 16:14, Rob Herring wrote:
> >> +
> >> +      pinctrl-b {
> >> +        compatible = "mobileye,eyeq5-b-pinctrl";
> >> +        #pinctrl-cells = <1>;
> >> +      };
> >> +    };
> > 
> > This can all be simplified to:
> > 
> > system-controller@e00000 {
> >     compatible = "mobileye,eyeq5-olb", "syscon";
> >     reg = <0xe00000 0x400>;
> >     #reset-cells = <2>;
> >     #clock-cells = <1>;
> >     clocks = <&xtal>;
> >     clock-names = "ref";
> > 
> >     pins { ... };
> > };
> > 
> > There is no need for sub nodes unless you have reusable blocks or each 
> > block has its own resources in DT.
>
> Yes, however I believe there should be resources here: each subnode
> should get its address space. This is a bit tied to implementation,
> which currently assumes "everyone can fiddle with everything" in this block.
>
> Theo, can you draw memory map?

It would be a mess. I've counted things up. The first 147 registers are
used in this 0x400 block. There are 31 individual blocks, with 7
registers unused (holes to align next block).

Functions are reset, clocks, LBIST, MBIST, DDR control, GPIO,
accelerator control, CPU entrypoint, PDTrace, IRQs, chip info & ID
stuff, control registers for PCIe / eMMC / Eth / SGMII / DMA / etc.

Some will never get used from Linux, others might. Maybe a moderate
approach would be to create ressources for major blocks and make it
evolve organically, without imposing that all uses lead to a new
ressource creation.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 04/17] dt-bindings: soc: mobileye: add EyeQ5 OLB system controller

[Théo Lebrun]

Hello,

On Wed Jan 24, 2024 at 8:22 PM CET, Rob Herring wrote:
> On Wed, Jan 24, 2024 at 11:40 AM Théo Lebrun <theo.lebrun@bootlin.com> wrote:
> > On Wed Jan 24, 2024 at 6:28 PM CET, Théo Lebrun wrote:
> > > On Wed Jan 24, 2024 at 4:14 PM CET, Rob Herring wrote:
> > > > On Tue, Jan 23, 2024 at 07:46:49PM +0100, Théo Lebrun wrote:

[...]

> > > > > +      };
> > > > > +
> > > > > +      pinctrl-b {
> > > > > +        compatible = "mobileye,eyeq5-b-pinctrl";
> > > > > +        #pinctrl-cells = <1>;
> > > > > +      };
> > > > > +    };
> > > >
> > > > This can all be simplified to:
> > > >
> > > > system-controller@e00000 {
> > > >     compatible = "mobileye,eyeq5-olb", "syscon";
> > > >     reg = <0xe00000 0x400>;
> > > >     #reset-cells = <2>;
> > > >     #clock-cells = <1>;
> > > >     clocks = <&xtal>;
> > > >     clock-names = "ref";
> > > >
> > > >     pins { ... };
> > > > };
> > > >
> > > > There is no need for sub nodes unless you have reusable blocks or each
> > > > block has its own resources in DT.
> > >
> > > That is right, and it does simplify the devicetree as you have shown.
> > > However, the split nodes gives the following advantages:
> > >
> > >  - Devicetree-wise, it allows for one alias per function.
> > >    `clocks = <&clocks EQ5C_PLL_CPU>` is surely more intuitive
> > >    than `clocks = <&olb EQ5C_PLL_CPU>;`. Same for reset.
>
> clocks: resets: pinctrl: system-controller@e00000 {
>
> > >
> > >  - It means an MFD driver must be implemented, adding between 100 to 200
> > >    lines of boilerplate code to the kernel.
>
> From a binding perspective, not my problem... That's Linux details
> defining the binding. What about u-boot, BSD, future versions of Linux
> with different structure?
>
> I don't think an MFD is required here. A driver should be able to be
> both clock and reset provider. That's pretty common. pinctrl less so.

@Rob & @Krzysztof: following Krzysztof's question about the memory map
and adding ressources to the system-controller, I was wondering if the
following approach would be more suitable:

	olb: system-controller@e00000 {
		compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd";
		reg = <0 0xe00000 0x0 0x400>;
		#address-cells = <1>;
		#size-cells = <1>;

		clocks: clock-controller {
			compatible = "mobileye,eyeq5-clk";
			reg = <0x02c 0x7C>;
			#clock-cells = <1>;
			clocks = <&xtal>;
			clock-names = "ref";
		};

		reset: reset-controller {
			compatible = "mobileye,eyeq5-reset";
			reg = <0x004 0x08>, <0x120 0x04>, <0x200 0x34>;
			reg-names = "d0", "d2", "d1";
			#reset-cells = <2>;
		};

		pinctrl0: pinctrl-a {
			compatible = "mobileye,eyeq5-a-pinctrl";
			reg = <0x0B0 0x30>;
		};

		pinctrl1: pinctrl-b {
			compatible = "mobileye,eyeq5-b-pinctrl";
			reg = <0x0B0 0x30>;
		};
	};

It highlights that they are in fact separate controllers and not one
device. The common thing between them is that they were
custom-implemented by Mobileye and therefore all registers were put in
a single block.

Else we'll go with the driver that implements both the clock & reset
providers. It'd live in drivers/clk/ I believe, as this is where other
drivers of the sort live.

> > >  - It means one pinctrl device for the two banks. That addresses your
> > >    comment on [PATCH v3 10/17]. This is often done and would be doable
> > >    on this platform. However it means added logic to each individual
> > >    function of pinctrl-eyeq5.
>
> If it makes things easier, 2 'pins' sub-nodes is fine. That's just
> container nodes.
>
> > >    Overall it makes for less readable code, for code that already looks
> > >    more complex than it really is.
> > >
> > >    My initial non-public version of pinctrl-eyeq5 was using this method
> > >    (a device handling both banks) and I've leaned away from it.
> >
> > I had forgotten one other reason:
> >
> >  - Reusability does count for something. Other Mobileye platforms exist,
> >    and the system controller stuff is more complex on those. Multiple
> >    different OLB blocks, etc. But my understanding is that
> >    per-peripheral logic is reused across versions.
>
> IME, this stuff never stays exactly the same from chip to chip.

If it helps, I have access to the downstream vendor kernel to see how
things work there. It supports the next generation of Mobileye
hardware.

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 08/17] clk: eyeq5: add platform driver

[Théo Lebrun]

Hi,

On Thu Jan 25, 2024 at 8:46 AM CET, Krzysztof Kozlowski wrote:
> On 24/01/2024 17:41, Théo Lebrun wrote:
> > Hello,
> > 
> > On Wed Jan 24, 2024 at 8:05 AM CET, Krzysztof Kozlowski wrote:
> >> On 23/01/2024 19:46, Théo Lebrun wrote:
> >>> Add the Mobileye EyeQ5 clock controller driver. It might grow to add
> >>> support for other platforms from Mobileye.
> >>>
> >>> It handles 10 read-only PLLs derived from the main crystal on board. It
> >>> exposes a table-based divider clock used for OSPI. Other platform
> >>> clocks are not configurable and therefore kept as fixed-factor
> >>> devicetree nodes.
> >>>
> >>> Two PLLs are required early on and are therefore registered at
> >>> of_clk_init(). Those are pll-cpu for the GIC timer and pll-per for the
> >>> UARTs.
> >>>
> >>
> >>
> >>> +#define OLB_PCSR1_RESET				BIT(0)
> >>> +#define OLB_PCSR1_SSGC_DIV			GENMASK(4, 1)
> >>> +/* Spread amplitude (% = 0.1 * SPREAD[4:0]) */
> >>> +#define OLB_PCSR1_SPREAD			GENMASK(9, 5)
> >>> +#define OLB_PCSR1_DIS_SSCG			BIT(10)
> >>> +/* Down-spread or center-spread */
> >>> +#define OLB_PCSR1_DOWN_SPREAD			BIT(11)
> >>> +#define OLB_PCSR1_FRAC_IN			GENMASK(31, 12)
> >>> +
> >>> +static struct clk_hw_onecell_data *eq5c_clk_data;
> >>> +static struct regmap *eq5c_olb;
> >>
> >> Drop these two. No file-scope regmaps for drivers. Use private container
> >> structures.
> > 
> > I wouldn't know how to handle the two steps then. Two clocks and the clk
> > provider are registered at of_clk_init() using CLK_OF_DECLARE_DRIVER().
>
> Right, if some clocks have to be early, CLK_OF_DECLARE_DRIVER needs
> static ones. But your commit subject says it is a platform driver and
> all other pieces of this code is rather incompatible with this approach.

That is my bad on the commit subject. What do you refer to by "all other
pieces of this code is rather incompatible with this approach"?

I've tried to minimise the use of static variables. Therefore as soon as
the probe is started, we switch to the usual way of using a private
struct that contains our info.

>
> Do not use CLK_OF_DECLARE_DRIVER for cases where you have dependencies
> because it forces you to manually order initcalls, which is exactly what
> we do not want.

What should I be using? I got confirmation from Stephen that this
mixture of CLK_OF_DECLARE_DRIVER() + platform driver is what I should
be using as review in my V1.

https://lore.kernel.org/lkml/fa32e6fae168e10d42051b89197855e9.sboyd@kernel.org/

>
>
> > The rest is at platform device probe. Without a static, there are no
> > way to pass the struct clk_hw_onecell_data from one to the other.
> > 
> > I've looked at all clock drivers that do CLK_OF_DECLARE_DRIVER() and
> > register a platform driver.
>
> Even though the code is correct, using arguments "other did it" will not
> work. You want to say that you implement legacy, poor code because you
> saw legacy, poor code?

Yes I see what you mean. It's just that this is not the sort of things
that are documented. And learning The Right Way(TM) when you don't know
it can only be done by looking at existing stuff. I'm being exhaustive
to avoid basing my approach on one old-school driver that is using the
wrong approach.

> >> ...
> >>
> >>> +static void __init eq5c_init(struct device_node *np)
> >>> +{
> >>> +	struct device_node *parent_np = of_get_parent(np);
> >>> +	int i, ret;
> >>> +
> >>> +	eq5c_clk_data = kzalloc(struct_size(eq5c_clk_data, hws, EQ5C_NB_CLKS),
> >>> +				GFP_KERNEL);
> >>> +	if (!eq5c_clk_data) {
> >>> +		ret = -ENOMEM;
> >>> +		goto err;
> >>> +	}
> >>> +
> >>> +	eq5c_clk_data->num = EQ5C_NB_CLKS;
> >>> +
> >>> +	/*
> >>> +	 * Mark all clocks as deferred. We register some now and others at
> >>> +	 * platform device probe.
> >>> +	 */
> >>> +	for (i = 0; i < EQ5C_NB_CLKS; i++)
> >>> +		eq5c_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
> >>> +
> >>> +	/*
> >>> +	 * Currently, if OLB is not available, we log an error, fail init then
> >>
> >> How it could be not available? Only with broken initcall ordering. Fix
> >> your initcall ordering and then simplify all this weird code.
> > 
> > of_syscon_register() and regmap_init_mmio() lists many reasons for
> > it to not be available. Am I missing something?
>
> Yes, initcall ordering.

You said the regmap can only not be available with broken initcall
ordering. I say that is not the only reason.

About initcall, I've removed those that used initcall in the three
drivers I'm using except this clk one that requires two clocks at
of_clk_init().

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 04/17] dt-bindings: soc: mobileye: add EyeQ5 OLB system controller

[Andrew Davis]

On 1/25/24 5:01 AM, Théo Lebrun wrote:
> Hello,
> 
> On Thu Jan 25, 2024 at 8:51 AM CET, Krzysztof Kozlowski wrote:
>> On 24/01/2024 16:14, Rob Herring wrote:
>>>> +
>>>> +      pinctrl-b {
>>>> +        compatible = "mobileye,eyeq5-b-pinctrl";
>>>> +        #pinctrl-cells = <1>;
>>>> +      };
>>>> +    };
>>>
>>> This can all be simplified to:
>>>
>>> system-controller@e00000 {
>>>      compatible = "mobileye,eyeq5-olb", "syscon";
>>>      reg = <0xe00000 0x400>;
>>>      #reset-cells = <2>;
>>>      #clock-cells = <1>;
>>>      clocks = <&xtal>;
>>>      clock-names = "ref";
>>>
>>>      pins { ... };
>>> };
>>>
>>> There is no need for sub nodes unless you have reusable blocks or each
>>> block has its own resources in DT.
>>
>> Yes, however I believe there should be resources here: each subnode
>> should get its address space. This is a bit tied to implementation,
>> which currently assumes "everyone can fiddle with everything" in this block.
>>
>> Theo, can you draw memory map?
> 
> It would be a mess. I've counted things up. The first 147 registers are
> used in this 0x400 block. There are 31 individual blocks, with 7
> registers unused (holes to align next block).
> 
> Functions are reset, clocks, LBIST, MBIST, DDR control, GPIO,
> accelerator control, CPU entrypoint, PDTrace, IRQs, chip info & ID
> stuff, control registers for PCIe / eMMC / Eth / SGMII / DMA / etc.
> 
> Some will never get used from Linux, others might. Maybe a moderate
> approach would be to create ressources for major blocks and make it
> evolve organically, without imposing that all uses lead to a new
> ressource creation.
> 

That is usually how nodes are added to DT. If you modeled this
system-controller space as a "simple-bus" instead of a "syscon"
device, you could add nodes as you implement them. Rather than
all at once as you have to by treating this space as one large
blob device.

Andrew

> Thanks,
> 
> --
> Théo Lebrun, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 

Re: [PATCH v3 04/17] dt-bindings: soc: mobileye: add EyeQ5 OLB system controller

[Théo Lebrun]

Hello,

On Thu Jan 25, 2024 at 3:33 PM CET, Andrew Davis wrote:
> On 1/25/24 5:01 AM, Théo Lebrun wrote:
> > Hello,
> > 
> > On Thu Jan 25, 2024 at 8:51 AM CET, Krzysztof Kozlowski wrote:
> >> On 24/01/2024 16:14, Rob Herring wrote:
> >>>> +
> >>>> +      pinctrl-b {
> >>>> +        compatible = "mobileye,eyeq5-b-pinctrl";
> >>>> +        #pinctrl-cells = <1>;
> >>>> +      };
> >>>> +    };
> >>>
> >>> This can all be simplified to:
> >>>
> >>> system-controller@e00000 {
> >>>      compatible = "mobileye,eyeq5-olb", "syscon";
> >>>      reg = <0xe00000 0x400>;
> >>>      #reset-cells = <2>;
> >>>      #clock-cells = <1>;
> >>>      clocks = <&xtal>;
> >>>      clock-names = "ref";
> >>>
> >>>      pins { ... };
> >>> };
> >>>
> >>> There is no need for sub nodes unless you have reusable blocks or each
> >>> block has its own resources in DT.
> >>
> >> Yes, however I believe there should be resources here: each subnode
> >> should get its address space. This is a bit tied to implementation,
> >> which currently assumes "everyone can fiddle with everything" in this block.
> >>
> >> Theo, can you draw memory map?
> > 
> > It would be a mess. I've counted things up. The first 147 registers are
> > used in this 0x400 block. There are 31 individual blocks, with 7
> > registers unused (holes to align next block).
> > 
> > Functions are reset, clocks, LBIST, MBIST, DDR control, GPIO,
> > accelerator control, CPU entrypoint, PDTrace, IRQs, chip info & ID
> > stuff, control registers for PCIe / eMMC / Eth / SGMII / DMA / etc.
> > 
> > Some will never get used from Linux, others might. Maybe a moderate
> > approach would be to create ressources for major blocks and make it
> > evolve organically, without imposing that all uses lead to a new
> > ressource creation.
> > 
>
> That is usually how nodes are added to DT. If you modeled this
> system-controller space as a "simple-bus" instead of a "syscon"
> device, you could add nodes as you implement them. Rather than
> all at once as you have to by treating this space as one large
> blob device.

I see where you are coming from, but in our case modeling our DT node as
a simple-bus would be lying about the hardware behind. There is no such
underlying bus. Let's try to keep the devicetree an abstraction
describing the hardware.

Also, we are having conflicts because multiple such child nodes are
being added at the same time as the base node. Once this initial series
is out (meaning dt-bindings for the OLB will exist) we'll be able to
add new nodes or ressources on a whim.

Have you got an opinion on the approach described in this email?
https://lore.kernel.org/lkml/CYNRCGYA1PJ2.FYENLB4SRJWH@bootlin.com/

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 04/17] dt-bindings: soc: mobileye: add EyeQ5 OLB system controller

[Andrew Davis]

On 1/25/24 8:49 AM, Théo Lebrun wrote:
> Hello,
> 
> On Thu Jan 25, 2024 at 3:33 PM CET, Andrew Davis wrote:
>> On 1/25/24 5:01 AM, Théo Lebrun wrote:
>>> Hello,
>>>
>>> On Thu Jan 25, 2024 at 8:51 AM CET, Krzysztof Kozlowski wrote:
>>>> On 24/01/2024 16:14, Rob Herring wrote:
>>>>>> +
>>>>>> +      pinctrl-b {
>>>>>> +        compatible = "mobileye,eyeq5-b-pinctrl";
>>>>>> +        #pinctrl-cells = <1>;
>>>>>> +      };
>>>>>> +    };
>>>>>
>>>>> This can all be simplified to:
>>>>>
>>>>> system-controller@e00000 {
>>>>>       compatible = "mobileye,eyeq5-olb", "syscon";
>>>>>       reg = <0xe00000 0x400>;
>>>>>       #reset-cells = <2>;
>>>>>       #clock-cells = <1>;
>>>>>       clocks = <&xtal>;
>>>>>       clock-names = "ref";
>>>>>
>>>>>       pins { ... };
>>>>> };
>>>>>
>>>>> There is no need for sub nodes unless you have reusable blocks or each
>>>>> block has its own resources in DT.
>>>>
>>>> Yes, however I believe there should be resources here: each subnode
>>>> should get its address space. This is a bit tied to implementation,
>>>> which currently assumes "everyone can fiddle with everything" in this block.
>>>>
>>>> Theo, can you draw memory map?
>>>
>>> It would be a mess. I've counted things up. The first 147 registers are
>>> used in this 0x400 block. There are 31 individual blocks, with 7
>>> registers unused (holes to align next block).
>>>
>>> Functions are reset, clocks, LBIST, MBIST, DDR control, GPIO,
>>> accelerator control, CPU entrypoint, PDTrace, IRQs, chip info & ID
>>> stuff, control registers for PCIe / eMMC / Eth / SGMII / DMA / etc.
>>>
>>> Some will never get used from Linux, others might. Maybe a moderate
>>> approach would be to create ressources for major blocks and make it
>>> evolve organically, without imposing that all uses lead to a new
>>> ressource creation.
>>>
>>
>> That is usually how nodes are added to DT. If you modeled this
>> system-controller space as a "simple-bus" instead of a "syscon"
>> device, you could add nodes as you implement them. Rather than
>> all at once as you have to by treating this space as one large
>> blob device.
> 
> I see where you are coming from, but in our case modeling our DT node as
> a simple-bus would be lying about the hardware behind. There is no such
> underlying bus. Let's try to keep the devicetree an abstraction
> describing the hardware.

Sure there is a bus, every register is on a bus, all these registers are
memory mapped aren't they? "simple-bus" is just a logical grouping, it
doesn't have to imply the bus is physically separate from the rest of
the system bus. If you don't want these misc registers logically grouped
then add them all as subnodes directly on the main SoC bus node.

Calling that group of miscellaneous registers a "simple-mfd" device is
even more incorrectly modeled IMHO.

We have the same problem on our SoCs (hardware folks just love making
miscellaneous junk drawer register spaces :D). And we decided to model
it as a "syscon", "simple-mfd" too, how simple to just have all the
other nodes point to this space with phandles and pull out whatever
register they need. But that was a mistake we are still working to
unwind.

> 
> Also, we are having conflicts because multiple such child nodes are
> being added at the same time as the base node. Once this initial series
> is out (meaning dt-bindings for the OLB will exist) we'll be able to
> add new nodes or ressources on a whim.
> 

Not to this "system-controller" space you won't. If you keep it as
a "simple-mfd","syscon" you will need to update the binding every
time you add a new node.

> Have you got an opinion on the approach described in this email?
> https://lore.kernel.org/lkml/CYNRCGYA1PJ2.FYENLB4SRJWH@bootlin.com/
> 

Looks better to me, the nodes contain the registers they use which
means you could simply add a ranges property to the parent and
not need to use special accessors and offsets in the drivers too.

Andrew

> Thanks,
> 
> --
> Théo Lebrun, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Re: [PATCH v3 04/17] dt-bindings: soc: mobileye: add EyeQ5 OLB system controller

[Krzysztof Kozlowski]

On 25/01/2024 12:01, Théo Lebrun wrote:
> Hello,
> 
> On Thu Jan 25, 2024 at 8:51 AM CET, Krzysztof Kozlowski wrote:
>> On 24/01/2024 16:14, Rob Herring wrote:
>>>> +
>>>> +      pinctrl-b {
>>>> +        compatible = "mobileye,eyeq5-b-pinctrl";
>>>> +        #pinctrl-cells = <1>;
>>>> +      };
>>>> +    };
>>>
>>> This can all be simplified to:
>>>
>>> system-controller@e00000 {
>>>     compatible = "mobileye,eyeq5-olb", "syscon";
>>>     reg = <0xe00000 0x400>;
>>>     #reset-cells = <2>;
>>>     #clock-cells = <1>;
>>>     clocks = <&xtal>;
>>>     clock-names = "ref";
>>>
>>>     pins { ... };
>>> };
>>>
>>> There is no need for sub nodes unless you have reusable blocks or each 
>>> block has its own resources in DT.
>>
>> Yes, however I believe there should be resources here: each subnode
>> should get its address space. This is a bit tied to implementation,
>> which currently assumes "everyone can fiddle with everything" in this block.
>>
>> Theo, can you draw memory map?
> 
> It would be a mess. I've counted things up. The first 147 registers are
> used in this 0x400 block. There are 31 individual blocks, with 7
> registers unused (holes to align next block).

Holes are not really a problem.

> 
> Functions are reset, clocks, LBIST, MBIST, DDR control, GPIO,
> accelerator control, CPU entrypoint, PDTrace, IRQs, chip info & ID
> stuff, control registers for PCIe / eMMC / Eth / SGMII / DMA / etc.

So they are within separate blocks or not?



Best regards,
Krzysztof

Re: [PATCH v3 04/17] dt-bindings: soc: mobileye: add EyeQ5 OLB system controller

[Krzysztof Kozlowski]

On 25/01/2024 12:40, Théo Lebrun wrote:
> Hello,
> 
> On Wed Jan 24, 2024 at 8:22 PM CET, Rob Herring wrote:
>> On Wed, Jan 24, 2024 at 11:40 AM Théo Lebrun <theo.lebrun@bootlin.com> wrote:
>>> On Wed Jan 24, 2024 at 6:28 PM CET, Théo Lebrun wrote:
>>>> On Wed Jan 24, 2024 at 4:14 PM CET, Rob Herring wrote:
>>>>> On Tue, Jan 23, 2024 at 07:46:49PM +0100, Théo Lebrun wrote:
> 
> [...]
> 
>>>>>> +      };
>>>>>> +
>>>>>> +      pinctrl-b {
>>>>>> +        compatible = "mobileye,eyeq5-b-pinctrl";
>>>>>> +        #pinctrl-cells = <1>;
>>>>>> +      };
>>>>>> +    };
>>>>>
>>>>> This can all be simplified to:
>>>>>
>>>>> system-controller@e00000 {
>>>>>     compatible = "mobileye,eyeq5-olb", "syscon";
>>>>>     reg = <0xe00000 0x400>;
>>>>>     #reset-cells = <2>;
>>>>>     #clock-cells = <1>;
>>>>>     clocks = <&xtal>;
>>>>>     clock-names = "ref";
>>>>>
>>>>>     pins { ... };
>>>>> };
>>>>>
>>>>> There is no need for sub nodes unless you have reusable blocks or each
>>>>> block has its own resources in DT.
>>>>
>>>> That is right, and it does simplify the devicetree as you have shown.
>>>> However, the split nodes gives the following advantages:
>>>>
>>>>  - Devicetree-wise, it allows for one alias per function.
>>>>    `clocks = <&clocks EQ5C_PLL_CPU>` is surely more intuitive
>>>>    than `clocks = <&olb EQ5C_PLL_CPU>;`. Same for reset.
>>
>> clocks: resets: pinctrl: system-controller@e00000 {
>>
>>>>
>>>>  - It means an MFD driver must be implemented, adding between 100 to 200
>>>>    lines of boilerplate code to the kernel.
>>
>> From a binding perspective, not my problem... That's Linux details
>> defining the binding. What about u-boot, BSD, future versions of Linux
>> with different structure?
>>
>> I don't think an MFD is required here. A driver should be able to be
>> both clock and reset provider. That's pretty common. pinctrl less so.
> 
> @Rob & @Krzysztof: following Krzysztof's question about the memory map
> and adding ressources to the system-controller, I was wondering if the
> following approach would be more suitable:

More or less (missing ranges, unit addresses, lower-case hex etc).

> 
> 	olb: system-controller@e00000 {
> 		compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd";
> 		reg = <0 0xe00000 0x0 0x400>;
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 
> 		clocks: clock-controller {
> 			compatible = "mobileye,eyeq5-clk";
> 			reg = <0x02c 0x7C>;
> 			#clock-cells = <1>;
> 			clocks = <&xtal>;
> 			clock-names = "ref";
> 		};
> 
> 		reset: reset-controller {
> 			compatible = "mobileye,eyeq5-reset";
> 			reg = <0x004 0x08>, <0x120 0x04>, <0x200 0x34>;
> 			reg-names = "d0", "d2", "d1";
> 			#reset-cells = <2>;
> 		};
> 
> 		pinctrl0: pinctrl-a {
> 			compatible = "mobileye,eyeq5-a-pinctrl";
> 			reg = <0x0B0 0x30>;
> 		};
> 
> 		pinctrl1: pinctrl-b {
> 			compatible = "mobileye,eyeq5-b-pinctrl";
> 			reg = <0x0B0 0x30>;

Duplicate reg?

> 		};
> 	};
> 
> It highlights that they are in fact separate controllers and not one
> device. The common thing between them is that they were
> custom-implemented by Mobileye and therefore all registers were put in
> a single block.
> 


Best regards,
Krzysztof

Re: [PATCH v3 08/17] clk: eyeq5: add platform driver

[Krzysztof Kozlowski]

On 25/01/2024 12:53, Théo Lebrun wrote:
> Hi,
> 
> On Thu Jan 25, 2024 at 8:46 AM CET, Krzysztof Kozlowski wrote:
>> On 24/01/2024 17:41, Théo Lebrun wrote:
>>> Hello,
>>>
>>> On Wed Jan 24, 2024 at 8:05 AM CET, Krzysztof Kozlowski wrote:
>>>> On 23/01/2024 19:46, Théo Lebrun wrote:
>>>>> Add the Mobileye EyeQ5 clock controller driver. It might grow to add
>>>>> support for other platforms from Mobileye.
>>>>>
>>>>> It handles 10 read-only PLLs derived from the main crystal on board. It
>>>>> exposes a table-based divider clock used for OSPI. Other platform
>>>>> clocks are not configurable and therefore kept as fixed-factor
>>>>> devicetree nodes.
>>>>>
>>>>> Two PLLs are required early on and are therefore registered at
>>>>> of_clk_init(). Those are pll-cpu for the GIC timer and pll-per for the
>>>>> UARTs.
>>>>>
>>>>
>>>>
>>>>> +#define OLB_PCSR1_RESET				BIT(0)
>>>>> +#define OLB_PCSR1_SSGC_DIV			GENMASK(4, 1)
>>>>> +/* Spread amplitude (% = 0.1 * SPREAD[4:0]) */
>>>>> +#define OLB_PCSR1_SPREAD			GENMASK(9, 5)
>>>>> +#define OLB_PCSR1_DIS_SSCG			BIT(10)
>>>>> +/* Down-spread or center-spread */
>>>>> +#define OLB_PCSR1_DOWN_SPREAD			BIT(11)
>>>>> +#define OLB_PCSR1_FRAC_IN			GENMASK(31, 12)
>>>>> +
>>>>> +static struct clk_hw_onecell_data *eq5c_clk_data;
>>>>> +static struct regmap *eq5c_olb;
>>>>
>>>> Drop these two. No file-scope regmaps for drivers. Use private container
>>>> structures.
>>>
>>> I wouldn't know how to handle the two steps then. Two clocks and the clk
>>> provider are registered at of_clk_init() using CLK_OF_DECLARE_DRIVER().
>>
>> Right, if some clocks have to be early, CLK_OF_DECLARE_DRIVER needs
>> static ones. But your commit subject says it is a platform driver and
>> all other pieces of this code is rather incompatible with this approach.
> 
> That is my bad on the commit subject. What do you refer to by "all other
> pieces of this code is rather incompatible with this approach"?

That you depend on syscon.

If it was regular MMIO block in SoC space, then no problem.
If you depend on anything else providing you regmap, then any initcall
ordering is fragile and error-prone. Avoid.


> 
> I've tried to minimise the use of static variables. Therefore as soon as
> the probe is started, we switch to the usual way of using a private
> struct that contains our info.
> 
>>
>> Do not use CLK_OF_DECLARE_DRIVER for cases where you have dependencies
>> because it forces you to manually order initcalls, which is exactly what
>> we do not want.
> 
> What should I be using? I got confirmation from Stephen that this
> mixture of CLK_OF_DECLARE_DRIVER() + platform driver is what I should
> be using as review in my V1.
> 
> https://lore.kernel.org/lkml/fa32e6fae168e10d42051b89197855e9.sboyd@kernel.org/

I see. In such case I believe it is error on relying on syscon.

Best regards,
Krzysztof

Re: [PATCH v3 04/17] dt-bindings: soc: mobileye: add EyeQ5 OLB system controller

[Théo Lebrun]

Hello,

On Fri Jan 26, 2024 at 12:52 PM CET, Krzysztof Kozlowski wrote:
> On 25/01/2024 12:40, Théo Lebrun wrote:
> > Hello,
> > 
> > On Wed Jan 24, 2024 at 8:22 PM CET, Rob Herring wrote:
> >> On Wed, Jan 24, 2024 at 11:40 AM Théo Lebrun <theo.lebrun@bootlin.com> wrote:
> >>> On Wed Jan 24, 2024 at 6:28 PM CET, Théo Lebrun wrote:
> >>>> On Wed Jan 24, 2024 at 4:14 PM CET, Rob Herring wrote:
> >>>>> On Tue, Jan 23, 2024 at 07:46:49PM +0100, Théo Lebrun wrote:
> > 
> > [...]
> > 
> >>>>>> +      };
> >>>>>> +
> >>>>>> +      pinctrl-b {
> >>>>>> +        compatible = "mobileye,eyeq5-b-pinctrl";
> >>>>>> +        #pinctrl-cells = <1>;
> >>>>>> +      };
> >>>>>> +    };
> >>>>>
> >>>>> This can all be simplified to:
> >>>>>
> >>>>> system-controller@e00000 {
> >>>>>     compatible = "mobileye,eyeq5-olb", "syscon";
> >>>>>     reg = <0xe00000 0x400>;
> >>>>>     #reset-cells = <2>;
> >>>>>     #clock-cells = <1>;
> >>>>>     clocks = <&xtal>;
> >>>>>     clock-names = "ref";
> >>>>>
> >>>>>     pins { ... };
> >>>>> };
> >>>>>
> >>>>> There is no need for sub nodes unless you have reusable blocks or each
> >>>>> block has its own resources in DT.
> >>>>
> >>>> That is right, and it does simplify the devicetree as you have shown.
> >>>> However, the split nodes gives the following advantages:
> >>>>
> >>>>  - Devicetree-wise, it allows for one alias per function.
> >>>>    `clocks = <&clocks EQ5C_PLL_CPU>` is surely more intuitive
> >>>>    than `clocks = <&olb EQ5C_PLL_CPU>;`. Same for reset.
> >>
> >> clocks: resets: pinctrl: system-controller@e00000 {
> >>
> >>>>
> >>>>  - It means an MFD driver must be implemented, adding between 100 to 200
> >>>>    lines of boilerplate code to the kernel.
> >>
> >> From a binding perspective, not my problem... That's Linux details
> >> defining the binding. What about u-boot, BSD, future versions of Linux
> >> with different structure?
> >>
> >> I don't think an MFD is required here. A driver should be able to be
> >> both clock and reset provider. That's pretty common. pinctrl less so.
> > 
> > @Rob & @Krzysztof: following Krzysztof's question about the memory map
> > and adding ressources to the system-controller, I was wondering if the
> > following approach would be more suitable:
>
> More or less (missing ranges, unit addresses, lower-case hex etc).

Yeah the details are not really on point, it was only a proposal
highlighting a different way of dealing with the current situation.
Looks like it is suitable to you.

> > 	olb: system-controller@e00000 {
> > 		compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd";
> > 		reg = <0 0xe00000 0x0 0x400>;
> > 		#address-cells = <1>;
> > 		#size-cells = <1>;
> > 
> > 		clocks: clock-controller {
> > 			compatible = "mobileye,eyeq5-clk";
> > 			reg = <0x02c 0x7C>;
> > 			#clock-cells = <1>;
> > 			clocks = <&xtal>;
> > 			clock-names = "ref";
> > 		};
> > 
> > 		reset: reset-controller {
> > 			compatible = "mobileye,eyeq5-reset";
> > 			reg = <0x004 0x08>, <0x120 0x04>, <0x200 0x34>;
> > 			reg-names = "d0", "d2", "d1";
> > 			#reset-cells = <2>;
> > 		};
> > 
> > 		pinctrl0: pinctrl-a {
> > 			compatible = "mobileye,eyeq5-a-pinctrl";
> > 			reg = <0x0B0 0x30>;
> > 		};
> > 
> > 		pinctrl1: pinctrl-b {
> > 			compatible = "mobileye,eyeq5-b-pinctrl";
> > 			reg = <0x0B0 0x30>;
>
> Duplicate reg?

Yes, the mapping is intertwined. Else it could be three ressources per
pinctrl. Just really small ones.

 - 0xB0 mapping   A
 - 0xB4 mapping   B
 - 0xB8
 - 0xBC
 - 0xC0 pull-down A
 - 0xC4 pull-up   A
 - 0xC8 pull-down B
 - 0xCC pull-up   B
 - 0xD0 drive-strength lo A
 - 0xD4 drive-strength hi A
 - 0xD8 drive-strength lo B
 - 0xDC drive-strength hi B

0xB8 is unrelated (I2C speed & SPI CS). 0xBC is a hole.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 04/17] dt-bindings: soc: mobileye: add EyeQ5 OLB system controller

[Krzysztof Kozlowski]

On 26/01/2024 13:28, Théo Lebrun wrote:
>>>
>>> 		pinctrl0: pinctrl-a {
>>> 			compatible = "mobileye,eyeq5-a-pinctrl";
>>> 			reg = <0x0B0 0x30>;
>>> 		};
>>>
>>> 		pinctrl1: pinctrl-b {
>>> 			compatible = "mobileye,eyeq5-b-pinctrl";
>>> 			reg = <0x0B0 0x30>;
>>
>> Duplicate reg?
> 
> Yes, the mapping is intertwined. Else it could be three ressources per
> pinctrl. Just really small ones.
> 
>  - 0xB0 mapping   A
>  - 0xB4 mapping   B
>  - 0xB8
>  - 0xBC
>  - 0xC0 pull-down A
>  - 0xC4 pull-up   A
>  - 0xC8 pull-down B
>  - 0xCC pull-up   B
>  - 0xD0 drive-strength lo A
>  - 0xD4 drive-strength hi A
>  - 0xD8 drive-strength lo B
>  - 0xDC drive-strength hi B
> 
> 0xB8 is unrelated (I2C speed & SPI CS). 0xBC is a hole.

Then maybe Rob's idea of one pinctrl device is better...

Best regards,
Krzysztof